diff --git a/.changeset/zesty-ibex-chatter.md b/.changeset/zesty-ibex-chatter.md new file mode 100644 index 000000000..e9318f57a --- /dev/null +++ b/.changeset/zesty-ibex-chatter.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3693 +--- +**`verify plan-structure` no longer false-flags positively-asserted literals in entity-escaped verify chains** — planners emit `&&` as the chain operator, which the negative-grep gate's segment splitter did not recognize, so a `= 0` clause poisoned `-ge 3` clauses joined to it and pushed authors toward suppressing a real gate. The gate now scans the decoded text the shell would actually run. (#3611) diff --git a/src/verify.cts b/src/verify.cts index 02c70c082..fd074018c 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -289,15 +289,78 @@ function cmdVerifySummary( * verify gate fails on the comment echo rather than a real regression. Conservative: * errors only on a confidently-extracted QUOTED literal; ambiguous (bareword) → warning. */ +/** + * Decode entity-escaped ampersands (& → &) — #3611. Planners emit + * bodies with `&&` as the chain operator (66 occurrences + * vs 0 literal in the reporting repo), and the executing agent reads the + * decoded (rendered) form. Every downstream scan — segment split, + * zero-comparison, literal harvest, echo matching — must operate on the same + * decoded text or a negative clause (`= 0`) poisons the literals of a + * POSITIVE clause (-ge 3) joined to it. Shared by both plan-discipline + * scanners so the two gates cannot drift apart again. + */ +function decodeEntityAmps(s: string): string { + return s.replace(/&/g, '&'); +} + +/** + * Split one shell line into &&/|| segments, QUOTE-AWARE (#3611 adversarial + * review): an operator inside a quoted literal (`grep -c 'a&&b'`) is part of + * the pattern, not a chain operator — a quote-blind split destroys the + * literal and silently disarms the gate for exactly the plans that spell + * patterns with ampersands. Backslash escapes count inside double quotes + * (POSIX single quotes have none, and over-staying a single-quoted span can + * only miss a split, never invent one). + */ +function splitShellSegments(line: string): string[] { + const segments: string[] = []; + let current = ''; + let quote: string | null = null; + for (let i = 0; i < line.length; i++) { + const ch = line[i]; + if (quote === "'") { + if (ch === "'") quote = null; + current += ch; + continue; + } + if (quote === '"') { + if (ch === '\\') { + current += ch + (line[i + 1] ?? ''); + i++; + continue; + } + if (ch === '"') quote = null; + current += ch; + continue; + } + if (ch === "'" || ch === '"') { + quote = ch; + current += ch; + continue; + } + if ((ch === '&' && line[i + 1] === '&') || (ch === '|' && line[i + 1] === '|')) { + segments.push(current.trim()); + current = ''; + i++; + continue; + } + current += ch; + } + segments.push(current.trim()); + return segments.filter((s) => s !== ''); +} + function scanNegativeGrepCommentEcho(content: string): { errors: string[]; warnings: string[] } { const errors: string[] = []; const warnings: string[] = []; // Normalize newlines; join backslash line-continuations so a verify command wrapped // across lines (grep ... \ == 0) is still seen as one segment. - const text = (content || '') + // #3611: decode entity-escaped ampersands (see decodeEntityAmps) so every + // downstream scan reads the same decoded text the executing agent reads. + const text = decodeEntityAmps((content || '') .replace(/\r\n/g, '\n') .replace(/\r/g, '\n') - .replace(/\\\n/g, ' '); + .replace(/\\\n/g, ' ')); // 1. Allowlisted literals: const allow = new Set(); @@ -348,7 +411,7 @@ function scanNegativeGrepCommentEcho(content: string): { errors: string[]; warni // poisoning a negative gate (`== 0`) sharing the same physical line. const seenErr = new Set(); const seenWarn = new Set(); - const segments = text.split('\n').flatMap((line) => line.split(/\s*(?:&&|\|\|)\s*/)); + const segments = text.split('\n').flatMap(splitShellSegments); for (const seg of segments) { if (!/grep(?:\s+-{1,2}[A-Za-z])/.test(seg) || !zeroCmp(seg)) continue; countGrepRe.lastIndex = 0; @@ -396,10 +459,13 @@ function scanFileWideNegativeGateConflict(content: string): { warnings: string[] const warnings: string[] = []; // Normalize newlines; join backslash line-continuations (same as #429). - const text = (content || '') + // #3611: the SAME entity decode as the #429 scanner — the two gates share + // the caller and the input; a decode on one side only let an entity-escaped + // chain poison this detector's harvest exactly the same way. + const text = decodeEntityAmps((content || '') .replace(/\r\n/g, '\n') .replace(/\r/g, '\n') - .replace(/\\\n/g, ' '); + .replace(/\\\n/g, ' ')); // Allowlisted patterns: const allow = new Set(); @@ -547,10 +613,8 @@ function scanFileWideNegativeGateConflict(content: string): { warnings: string[] for (let ai = 0; ai < tasks.length; ai++) { const taskA = tasks[ai]; - // Split gate text into shell segments (split on && / || within lines). - const segments = taskA.gateText.split('\n').flatMap(line => - line.split(/\s*(?:&&|\|\|)\s*/), - ); + // Split gate text into shell segments (quote-aware, shared with #429 — #3611). + const segments = taskA.gateText.split('\n').flatMap(splitShellSegments); for (const seg of segments) { if (!/grep/.test(seg)) continue; diff --git a/tests/verify.test.cjs b/tests/verify.test.cjs index 8a3311d61..5996f7cfb 100644 --- a/tests/verify.test.cjs +++ b/tests/verify.test.cjs @@ -2737,6 +2737,24 @@ describe('scanFileWideNegativeGateConflict — pure unit tests', () => { }); // Case 2: region-scoped via sed → NO warn + test('case 1b — entity-escaped chain: positive clause pattern must not be harvested as a file-wide ban (#3611)', () => { + // Task A bans banned_thing file-wide (== 0) AND positively asserts + // required_thing (-ge 1), joined by &&. Task B mentions only + // required_thing. Pre-fix, the literal-only split kept the chain as ONE + // segment: zeroCmp saw the == 0 and the harvest took BOTH patterns, + // falsely warning that B conflicts with a file-wide ban on required_thing. + const content = makeTwoTaskPlan({ + taskAGate: "grep -c 'banned_thing' app/page.py == 0 && grep -c 'required_thing' app/page.py -ge 1", + taskBAction: 'Introduce required_thing usage the plan asserts positively.', + }); + const result = scan(content); + assert.strictEqual( + result.warnings.filter(w => w.includes('#968')).length, + 0, + `a positively-asserted pattern (-ge 1) joined by && must not warn as a file-wide ban, got: ${JSON.stringify(result.warnings)}`, + ); + }); + test('case 2 — region-scoped via sed pipe → NO warn', () => { const content = makeTwoTaskPlan({ taskAGate: "! sed -n '12,40p' app/page.py | grep -Eq 'await .*refresh'", @@ -4204,6 +4222,107 @@ describe('scanNegativeGrepCommentEcho — pure unit tests', () => { assert.ok(!result.errors[0].includes('presentTok'), `error must NOT name presentTok, got: ${result.errors[0]}`); }); + test('case 12b — mixed gates joined by entity-escaped &&: no false positive for the positive token (#3611)', () => { + // #3611: planners emit bodies with the ampersands entity-escaped + // (&&). The literal-only segment splitter did not match that spelling, + // so the negative clause's `= 0` poisoned count-grep literals from the + // POSITIVE clause in the same chain — a `-ge 3` literal flagged as forbidden. + // Identical to case 12 except for the ampersand spelling. + const lines = [ + '---', + 'phase: 01-test', + 'plan: 01', + 'type: execute', + 'wave: 1', + 'depends_on: []', + 'files_modified: [file.ts]', + 'autonomous: true', + 'must_haves:', + ' - AC1', + '---', + '', + '', + 'Entity-escaped chain task', + '', + 'Use presentTok for the new pattern.', + 'Do not use absentTok any more.', + '', + "test \"$(grep -c 'absentTok' f)\" = 0 && test \"$(grep -c 'presentTok' f)\" -ge 3", + 'Done', + '', + ].join('\n'); + const verify = require(VERIFY_CJS); + const result = verify.scanNegativeGrepCommentEcho(lines); + assert.strictEqual(result.errors.length, 1, `expected exactly 1 error (absentTok only), got: ${JSON.stringify(result.errors)}`); + assert.ok(result.errors[0].includes('absentTok'), `error must name absentTok, got: ${result.errors[0]}`); + assert.ok(!result.errors[0].includes('presentTok'), `a positively-asserted literal (-ge 3) must never be flagged regardless of ampersand spelling, got: ${result.errors[0]}`); + }); + + test('case 12c — entity-escaped literals and action echoes decode consistently (#3611)', () => { + // A literal that itself contains & (e.g. "a&b" as the grep pattern) + // and an action echo carrying the same entity spelling must still match + // after the decode — the flag stays correct for entity-bearing literals. + const lines = [ + '---', + 'phase: 01-test', + 'plan: 01', + 'type: execute', + 'wave: 1', + 'depends_on: []', + 'files_modified: [file.ts]', + 'autonomous: true', + 'must_haves:', + ' - AC1', + '---', + '', + '', + 'Entity literal task', + '', + 'Remove the old a&b join.', + '', + "grep -c 'a&b' f == 0", + 'Done', + '', + ].join('\n'); + const verify = require(VERIFY_CJS); + const result = verify.scanNegativeGrepCommentEcho(lines); + assert.strictEqual(result.errors.length, 1, `the entity-bearing literal must still flag its action echo, got: ${JSON.stringify(result.errors)}`); + assert.ok(result.errors[0].includes('a&b'), `error must carry the decoded literal a&b, got: ${result.errors[0]}`); + }); + + test('case 12d — a quoted literal CONTAINING && is not shattered by the segment split (#3611 review)', () => { + // A negative grep banning a boolean shape (`grep -c 'a&&b' f == 0`) and an + // action echo mentioning a&&b. The split must be quote-aware: splitting on + // the operator inside the quotes would destroy the literal and silently + // disarm the gate — exactly the plans that spell patterns with ampersands. + const lines = [ + '---', + 'phase: 01-test', + 'plan: 01', + 'type: execute', + 'wave: 1', + 'depends_on: []', + 'files_modified: [file.ts]', + 'autonomous: true', + 'must_haves:', + ' - AC1', + '---', + '', + '', + 'Quoted operator literal task', + '', + 'Remove the a&&b join.', + '', + "grep -c 'a&&b' f == 0", + 'Done', + '', + ].join('\n'); + const verify = require(VERIFY_CJS); + const result = verify.scanNegativeGrepCommentEcho(lines); + assert.strictEqual(result.errors.length, 1, `the quoted a&&b literal must still flag its action echo, got: ${JSON.stringify(result.errors)}`); + assert.ok(result.errors[0].includes('a&&b'), `error must carry the intact literal, got: ${result.errors[0]}`); + }); + test('case 13 — grep -c -F (separate count+fixed flags) extracts literal', () => { // Bug 2: grep -c -F 'LIT' was not extracted by the old regex that required -c // immediately before the pattern without intervening flags.