From 30c40e0fe21aadc3da2371f9718600dc24fd964c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 4 Sep 2026 21:35:30 -0400 Subject: [PATCH] fix(#4302): bound restatement detector's deferral window structurally, not by a flat char count (#4303) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4302): add failing regression test for deferral-window paragraph leak RED: hasNearbyDeferralMarker's 500-char trailing window has no structural boundary, so a compact citation-free restatement immediately followed by an unrelated section that cites tdd.md borrows that neighbor's citation and evades restatesCycleStructurally(). Reproduces the exact shape confirmed via mutation testing against the real agents/gsd-executor.md. Co-Authored-By: Claude Sonnet 5 * fix(#4302): bound the deferral window at the next heading, not a flat char count hasNearbyDeferralMarker's 500-char trailing window had no structural boundary, so a compact citation-free restatement immediately followed by an unrelated section that cites tdd.md borrowed that neighbor's citation and evaded restatesCycleStructurally() (confirmed on the real gsd-executor.md, whose next section after the cycle pointer has its own independent citation 82 chars past the REFACTOR anchor). First attempt bounded at the next blank line instead, and was rejected after it broke a real case caught by direct execution before committing: execute-plan.md's FIRST RED/GREEN/REFACTOR occurrence is a numbered list's own intro sentence, separated by a genuine blank line from the list item that actually carries the citation — a blank line is not reliably "still the same statement" once list structure is involved. Bounds at the next markdown heading (`\n#`) instead, capped at 500 chars as before when no heading appears. Verified against both real files, the new regression fixture, and every pre-existing #4268 fixture/property test. Co-Authored-By: Claude Sonnet 5 * fix(#4302): close a narrower same-section evasion found by orthogonal review The heading-only fix left a narrower gap: a citation-free restatement followed by a blank line and an unrelated PROSE paragraph (no heading) that cites tdd.md for an unrelated reason still evaded detection — confirmed by the reviewer via execution. Generalizes the boundary rule: any blank line whose following line is NOT a markdown list continuation is now also a boundary (in addition to the existing heading boundary), reconciling with the earlier-rejected flat blank-line bound by adding the list-continuation exception that execute-plan.md's real shape (a numbered list's intro sentence, then a blank line, then the list item carrying the citation) needs. Verified against both real files, all three restatement fixtures (heading-bounded, prose-bounded, list-continuation negative-space), every pre-existing #4268 fixture, and the fast-check property test (200 runs). Also restores an honest, updated limitation disclosure describing the one narrower residual case not resolved by this design (a citation reachable via exactly one list-item hop from an unrelated list) — matching this repo's disclosed-not-silently-deferred practice from the original #4268 PR. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: sim Co-authored-by: Claude Sonnet 5 --- tests/tdd-single-statement.test.cjs | 150 +++++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 16 deletions(-) diff --git a/tests/tdd-single-statement.test.cjs b/tests/tdd-single-statement.test.cjs index ede9f56d5..2e83e6b1c 100644 --- a/tests/tdd-single-statement.test.cjs +++ b/tests/tdd-single-statement.test.cjs @@ -78,13 +78,35 @@ function restatesCycle(text) { // or list-marker shape. This is what makes the signal non-evadable by a // compact, reworded restatement: shortening or dropping list markers does // nothing to manufacture a citation that was never there. -// - Trailing window: 500 characters past the REFACTOR anchor. Measured -// empirically against the real files (2026-09-04): execute-plan.md's -// citation of `~/.claude/gsd-core/references/tdd.md` sits 228 chars past -// the REFACTOR anchor; gsd-executor.md's citation of -// `gsd-core/references/tdd.md` sits 82 chars past it. 500 clears both -// with wide margin while staying well short of the whole-file scan the -// #4228 postmortem (see comment above restatesCycle()) warns against. +// - Trailing window: the EARLIEST of (a) 500 characters past the REFACTOR +// anchor, (b) the next markdown HEADING line (`\n#`), or (c) the next +// blank line whose FOLLOWING line does not continue a markdown list +// (`- `, `* `, `N. `/`N) `) — never further. +// #4302: a flat char count with no structural boundary let a +// citation-free restatement borrow an UNRELATED later section's tdd.md +// citation (on the real gsd-executor.md, the very next section after the +// cycle pointer — "## Plan-Level TDD Gate Enforcement" — has its own, +// independent tdd.md citation 82 chars past the pointer's REFACTOR +// anchor, well inside a flat 500-char window). A first attempt bounded +// ONLY at the next heading; an orthogonal review then constructed a +// narrower variant of the same evasion — a citation-free restatement +// followed by a blank line and an unrelated PROSE paragraph (no heading) +// that cites tdd.md — and confirmed by execution that heading-only +// bounding still misses it. Bounding on EVERY blank line unconditionally +// was tried and rejected earlier for a different reason: the real +// execute-plan.md pointer's FIRST RED/GREEN/REFACTOR occurrence is a +// numbered list's own intro sentence, separated by a genuine blank line +// from the list item that carries the citation — a blank line is not +// reliably "still the same statement" UNLESS what follows it is a list +// continuation. The list-continuation exception is what reconciles both: +// a blank line followed by a list-marker line is skipped (same block); a +// blank line followed by anything else — prose, a heading, or the end of +// the section — is the boundary. Confirmed against both real files (no +// boundary before either one's own citation), the heading-borrowing +// fixture, and the narrower prose-borrowing fixture the review +// constructed. 500 chars remains the OUTER cap when no boundary appears +// at all, short of the whole-file scan the #4228 postmortem (see comment +// above restatesCycle()) warns against. // - SECONDARY signals (defense in depth, OR-ed in, kept from the original // design): span > 200 chars, or three DISTINCT markdown list-marker // lines (`- `, `* `, or `N. `/`N) `) each carrying one of RED/GREEN/ @@ -94,14 +116,18 @@ function restatesCycle(text) { // Threshold 200 and the list-marker shape are unchanged from the // original measurement: real pointers span 10-14 chars; a realistic // paraphrased restatement spans 424 chars. -// Limitation (stated honestly, not resolved by this design): a restatement -// that is BOTH compact/no-list-marker AND happens to mention "tdd.md" (or -// "canonical") somewhere within the 500-char trailing window — without that -// mention actually deferring the procedure to it — would still pass. The -// deferral check is a presence check on the marker text, not a semantic -// check that the marker is doing deferral work. Closing that gap needs a -// stronger check (e.g. requiring the marker to sit in the same sentence/ -// clause as the cycle words) that is not implemented here. +// Limitation (stated honestly, not resolved by this design): a citation +// belonging to an unrelated LATER statement that is itself only ONE +// list-marker hop away from the restatement (e.g. the restatement's own +// paragraph is immediately followed by a blank line, then a list item, then +// ANOTHER blank line, then the unrelated citation) would still be found, +// because the list-continuation exception does not distinguish "the same +// list the restatement belongs to" from "a different list that happens to +// start right after it." This is narrower than the gap #4302 closed (it +// requires the coincidence of an intervening list, not just any adjacent +// prose or heading) and is judged acceptable residual risk rather than +// grounds for a third boundary rule; a stronger fix would need to identify +// list membership by shared indentation/enumeration, not presence alone. const RESTATEMENT_SPAN_THRESHOLD = 200; const DEFERRAL_WINDOW_TRAILING = 500; const DEFERRAL_MARKER_RE = /tdd\.md|references\/tdd|canonical/; @@ -132,8 +158,41 @@ function measureCycleSpan(text) { return { redIdx, greenIdx, refactorIdx, refactorEndIdx, span: refactorIdx - redIdx }; } +// Find the first blank line (at or after `fromIdx`, within `[fromIdx, capIdx)`) +// whose immediately following line is NOT a list-marker continuation. +// Returns the absolute index of that blank line, or -1 if none qualifies +// before `capIdx`. A blank-line-then-list-item is skipped (same block) so a +// numbered list's intro sentence can still reach a citation carried by one +// of its own later items (the real execute-plan.md shape); any other blank +// line is a genuine boundary. Linear scan via a global regex + manual +// advance — no backtracking risk even on a large region. +function findListAwareBlankBoundary(text, fromIdx, capIdx) { + const region = text.slice(fromIdx, capIdx); + const BLANK_RE = /\n[ \t]*\n/g; + let m; + while ((m = BLANK_RE.exec(region))) { + const afterBlank = region.slice(m.index + m[0].length); + const nextLine = afterBlank.slice(0, afterBlank.indexOf('\n') === -1 ? undefined : afterBlank.indexOf('\n')); + if (LIST_MARKER_RE.test(nextLine)) continue; + return fromIdx + m.index; + } + return -1; +} + function hasNearbyDeferralMarker(text, cycle) { - const windowEnd = Math.min(text.length, cycle.refactorEndIdx + DEFERRAL_WINDOW_TRAILING); + const cappedEnd = Math.min(text.length, cycle.refactorEndIdx + DEFERRAL_WINDOW_TRAILING); + // #4302 (+ review-found narrower variant): stop at the EARLIEST of a + // markdown heading or a list-aware blank-line boundary, at or after the + // REFACTOR anchor — a citation belonging to a LATER, unrelated statement + // must not count as this restatement's own deferral. See the design + // comment above for why a blank line alone is not always a valid + // boundary (a numbered list's intro sentence vs. its own later item). + const region = text.slice(cycle.refactorEndIdx, cappedEnd); + const headingMatch = region.match(/\n#/); + const headingEnd = headingMatch ? cycle.refactorEndIdx + headingMatch.index : cappedEnd; + const blankBoundary = findListAwareBlankBoundary(text, cycle.refactorEndIdx, cappedEnd); + const blankEnd = blankBoundary === -1 ? cappedEnd : blankBoundary; + const windowEnd = Math.min(headingEnd, blankEnd); const window = text.slice(cycle.redIdx, windowEnd); return DEFERRAL_MARKER_RE.test(window); } @@ -324,4 +383,63 @@ describe('#4268 — reworded restatement detection (structural, not literal)', ( { numRuns: 20 }, ); }); + + // #4302: hasNearbyDeferralMarker's 500-char trailing window is a flat + // character count with no structural boundary — on the real + // agents/gsd-executor.md, a compact citation-free restatement sits + // immediately before an UNRELATED "## Plan-Level TDD Gate Enforcement" + // section that carries its own, independent tdd.md citation 82 chars past + // the REFACTOR anchor. The restatement borrows that neighbor's citation and + // evades detection. This fixture reproduces the same shape: a restatement + // paragraph with NO citation of its own, followed by a blank line and an + // unrelated section that does cite tdd.md within the 500-char window. + test('does not borrow a citation from an unrelated LATER heading-bounded section', () => { + const fixture = [ + 'RED: write a failing test proving the bug exists and record it with a test-scoped commit.', + 'GREEN: implement the smallest change that makes the test pass and record that with a', + 'feat-scoped commit. REFACTOR: simplify the implementation now that it is proven correct,', + 'without changing observable behavior.', + '', + '## Unrelated Section', + '', + 'This paragraph cites the canonical `gsd-core/references/tdd.md` reference for a completely', + 'different purpose and must not be borrowed by the restatement above.', + ].join('\n'); + assert.ok(restatesCycleStructurally(fixture), + "a citation belonging to a different, later heading-bounded section must not count as this restatement's own deferral"); + }); + + // #4302 follow-up (orthogonal review): heading-only bounding still missed a + // NARROWER variant — a citation-free restatement followed by a blank line + // and an unrelated PROSE paragraph (no heading at all) that cites tdd.md + // for an unrelated reason. Confirmed by execution that the heading-only + // fix from the first commit misses this; closed by treating any blank line + // NOT followed by a list continuation as a boundary too (see the design + // comment above hasNearbyDeferralMarker). + test('does not borrow a citation from an unrelated LATER paragraph with no heading at all', () => { + const fixture = [ + 'RED: write a failing test. GREEN: make it pass. REFACTOR: clean it up.', + '', + 'This unrelated paragraph mentions the canonical tdd.md reference for a totally different reason.', + ].join('\n'); + assert.ok(restatesCycleStructurally(fixture), + "a citation belonging to an unrelated later paragraph (no heading) must not count as this restatement's own deferral"); + }); + + // Negative-space companion to the above: a blank line immediately followed + // by a LIST ITEM (not a new unrelated paragraph) must still be treated as + // the SAME statement, matching the real execute-plan.md shape (a numbered + // list's intro sentence, then a blank line, then the list item that + // carries the citation) — this is exactly what the first #4302 fix attempt + // (a flat blank-line boundary) got wrong. + test('does not treat a blank-line-then-list-item as a boundary (the execute-plan.md shape)', () => { + const fixture = [ + 'For `type: tdd` plans — RED-GREEN-REFACTOR:', + '', + '1. Some unrelated setup step.', + '2. Execute the cycle exactly as the canonical `references/tdd.md` reference specifies.', + ].join('\n'); + assert.ok(!restatesCycleStructurally(fixture), + 'a citation carried by a later item of the SAME list (reached via a blank-line-then-list-item hop) must still count as this restatement\'s own deferral'); + }); });