diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index bfd2b11c6..568cf58b2 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -1101,6 +1101,21 @@ function isReadOnlyGitIntrospection(command) { // --- Gate messages --- +/** + * Batch-consistency warning (#3136). A first-touch denial marks the file + * checked so the retry passes; a parallel batch of edits to one + * not-yet-touched file therefore partially applies (first call denied, + * siblings allowed). Hooks see calls one at a time and cannot lock a + * batch, so the denial must say this out loud: name the file and tell + * the agent that siblings may already have been applied. + */ +function batchSiblingWarning(safePath) { + return ( + `If this call was sent in a parallel batch, other edits to ${safePath} from that batch ` + + 'may already have been applied. Re-read the file before building on them.' + ); +} + function editGateMsg(filePath) { const safe = sanitizePath(filePath); return [ @@ -1113,6 +1128,8 @@ function editGateMsg(filePath) { '3. If this file reads/writes data files, show field names, structure, and date format (use redacted or synthetic values, not raw production data)', "4. Quote the user's current instruction verbatim", '', + batchSiblingWarning(safe), + '', 'Present the facts, then retry the same operation.' ].join('\n'); } @@ -1129,6 +1146,8 @@ function writeGateMsg(filePath) { '3. If this file reads/writes data files, show field names, structure, and date format (use redacted or synthetic values, not raw production data)', "4. Quote the user's current instruction verbatim", '', + batchSiblingWarning(safe), + '', 'Present the facts, then retry the same operation.' ].join('\n'); } @@ -1143,6 +1162,7 @@ function condensedGateMsg(action, filePath, ordinal) { return ( `[Fact-Forcing Gate] (denial #${ordinal} this session) First ${action} of ${safe}: ` + "briefly state importers/callers, affected API, data schemas if any, and the user's verbatim instruction, then retry. " + + `${batchSiblingWarning(safe)} ` + '(Use GATEGUARD_EXEMPT_GLOBS for path-scoped exemptions; ECC_GATEGUARD=off disables this gate.)' ); } diff --git a/skills/gateguard/SKILL.md b/skills/gateguard/SKILL.md index e7ebc5cec..be96d2689 100644 --- a/skills/gateguard/SKILL.md +++ b/skills/gateguard/SKILL.md @@ -89,6 +89,26 @@ Triggers on: `rm -rf`, `git reset --hard`, `git push --force`, `drop table`, etc 2. What this specific command verifies or produces ``` +## Parallel Batches and Partial Application + +The first-touch gate evaluates each tool call independently. When several +edits to a file that has not been touched yet are sent in one parallel +batch, the first call is denied and the denial marks the file as checked, +so the sibling edits in that batch are applied. Nothing is rolled back: +the file can end up holding the sibling edits without the denied one. + +The denial message names the file and warns that batch siblings may +already have been applied. Treat it literally: + +- Send dependent edits to a not-yet-touched file sequentially, not in a + parallel batch. A definition and its first use, or an import and its + call site, must not ride in the same batch. +- After a first-touch denial, present the facts, retry the denied edit, + and re-read the file before building on anything else from the batch. + +A batch-wide lock is not possible: hooks see tool calls one at a time, so +the gate cannot know which calls arrived together. + ## Quick Start ### Option A: Use the ECC hook (zero install) diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index 495928691..df6e4e28c 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -3104,6 +3104,93 @@ function runTests() { passed++; else failed++; + // --- Batch consistency (#3136): a parallel batch of edits to one --- + // not-yet-touched file partially applies: the first denial marks the + // file checked, so sibling edits in the same batch are allowed. Hooks + // see calls one at a time and cannot lock a batch, so the contract is + // that the denial itself names the file and warns that batch siblings + // may already have been applied. + clearState(); + if ( + test('first-touch Edit denial warns about applied batch siblings (#3136)', () => { + // Two edits to the same unchecked file, sent as a parallel batch. + // Each hook invocation is its own process, exactly as in a batch. + const editA = { + tool_name: 'Edit', + tool_input: { file_path: '/src/batch-target.js', old_string: 'a', new_string: 'b' } + }; + const editB = { + tool_name: 'Edit', + tool_input: { file_path: '/src/batch-target.js', old_string: 'c', new_string: 'd' } + }; + + const first = parseOutput(runHook(editA).stdout); + assert.strictEqual(first.hookSpecificOutput.permissionDecision, 'deny', 'first edit of the batch is gated'); + const firstReason = first.hookSpecificOutput.permissionDecisionReason; + assert.ok(firstReason.includes('/src/batch-target.js'), 'denial names the exact file'); + assert.ok( + firstReason.includes('parallel batch'), + 'denial warns that batch siblings may already have been applied' + ); + assert.ok( + firstReason.includes('Re-read'), + 'denial tells the agent to re-read the file before building on siblings' + ); + + // Sibling edit in the same batch: judged against post-denial state, + // so it applies. The warning above is what makes this visible. + const second = parseOutput(runHook(editB).stdout); + if (second && second.hookSpecificOutput) { + assert.notStrictEqual(second.hookSpecificOutput.permissionDecision, 'deny', 'batch sibling is not re-gated'); + } + }) + ) + passed++; + else failed++; + + clearState(); + if ( + test('condensed Edit denial also warns about applied batch siblings (#3136)', () => { + writeState({ checked: [], last_active: Date.now(), fact_force_denials: 3 }); + const result = runHook({ tool_name: 'Edit', tool_input: { file_path: '/src/batch-condensed.js' } }); + const output = parseOutput(result.stdout); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny'); + const reason = output.hookSpecificOutput.permissionDecisionReason; + assert.ok(reason.includes('parallel batch'), 'condensed denial keeps the batch-sibling warning'); + assert.ok(!reason.includes('\n'), 'condensed denial stays a single line'); + }) + ) + passed++; + else failed++; + + clearState(); + if ( + test('first-touch Write and MultiEdit denials warn about applied batch siblings (#3136)', () => { + const writeOut = parseOutput( + runHook({ tool_name: 'Write', tool_input: { file_path: '/src/batch-new.js', content: 'x' } }).stdout + ); + assert.strictEqual(writeOut.hookSpecificOutput.permissionDecision, 'deny'); + assert.ok( + writeOut.hookSpecificOutput.permissionDecisionReason.includes('parallel batch'), + 'Write denial carries the batch-sibling warning' + ); + + const multiOut = parseOutput( + runHook({ + tool_name: 'MultiEdit', + tool_input: { edits: [{ file_path: '/src/batch-multi.js', old_string: 'a', new_string: 'b' }] } + }).stdout + ); + assert.strictEqual(multiOut.hookSpecificOutput.permissionDecision, 'deny'); + assert.ok( + multiOut.hookSpecificOutput.permissionDecisionReason.includes('parallel batch'), + 'MultiEdit denial carries the batch-sibling warning' + ); + }) + ) + passed++; + else failed++; + // Cleanup only the temp directory created by this test file. try { if (fs.existsSync(stateDir)) {