From db61d1c76ab3e39eb4b1ffc84831e9cc6d368286 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Fri, 18 Sep 2026 18:37:01 -0400 Subject: [PATCH] fix(gateguard): warn that parallel-batch siblings may already be applied (#3136) A first-touch Edit/Write denial marks the file checked so the retry passes. Sibling edits to the same file in the same parallel batch are therefore judged against post-denial state and silently apply, leaving the file in a state neither version intended. Hooks see tool calls one at a time, so a batch-wide lock is not possible. Instead make the partial application explicit: the Edit, Write, MultiEdit, and condensed denials now name the file and warn that other edits from the same batch may already have been applied, and SKILL.md tells agents to send dependent edits sequentially and re-read the file after a gated batch. --- scripts/hooks/gateguard-fact-force.js | 20 ++++++ skills/gateguard/SKILL.md | 20 ++++++ tests/hooks/gateguard-fact-force.test.js | 87 ++++++++++++++++++++++++ 3 files changed, 127 insertions(+) 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)) {