From 5c7243e54b6d1352576513995eccb3cee7a1e00f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 2 Sep 2026 06:03:17 -0400 Subject: [PATCH] fix(#3995): derive the review diff base from the phase directory (#4181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3995): diff base keys on the phase directory, not commit subjects All three derivation sites (Tier 3, spawn_reviewer, fallow pre-pass) must anchor on the phase directory's first commit; the milestone-blind repro (an archived milestone's same-numbered phase commit capturing the base) is the failing-first row. #3191/#3503 rows reworked to the directory-anchor contract; T6 docs-parity forbids any remaining phase-scope message-grep site. * fix(#3995): derive the review diff base from the phase directory A phase number is unique within a milestone, not a repository; the message grep had no milestone bound and tail -1 deliberately selected the oldest same-numbered subject, dragging archived milestones phases into the scope (7 files to 3388 plus the >50 depth downgrade). All three lockstep sites now anchor on the first commit that added anything under the phase own directory — the same anchor class git-base-branch phaseStartCommit uses. ShellCheck baseline gains the escaped fragment shifted parse signature. Emitted-Drift-Ack-Growth: code-review.md — phase-directory anchor replaces the message-grep derivation at both sites (#3995) * chore(#3995): backfill changeset pr number --------- Co-authored-by: sim --- .changeset/zesty-seals-frolic.md | 5 + gsd-core/workflows/code-review.md | 61 ++-- .../code-review/steps/structural-pre-pass.md | 24 +- .../lint-workflow-shellcheck-baseline.json | 5 + .../code-review-pipeline-regression.test.cjs | 289 ++++++++---------- 5 files changed, 189 insertions(+), 195 deletions(-) create mode 100644 .changeset/zesty-seals-frolic.md diff --git a/.changeset/zesty-seals-frolic.md b/.changeset/zesty-seals-frolic.md new file mode 100644 index 000000000..2546352e9 --- /dev/null +++ b/.changeset/zesty-seals-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4181 +--- +**Code-review scope no longer balloons on repos with past milestones** — the diff base for a phase now derives from the phase directory own first commit instead of a milestone-blind commit-subject grep that selected the OLDEST same-numbered phase in history. A 7-file phase could review 3388 files at downgraded depth. All three derivation sites move in lockstep. (#3995) diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 52c2a0451..3d8856c22 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -225,17 +225,27 @@ surface — so a partial SUMMARY result can no longer silently mask the rest of # #3191: stay POSIX-ERE portable — the boundary is the closing paren + colon, # never \b (not a POSIX ERE token; under --extended-regexp it silently matches # nothing on macOS regex(3), making this fallback dead on Apple platforms). -PHASE_SCOPE_NUM="${PADDED_PHASE}" -case "${PADDED_PHASE}" in - 0[0-9]*) PHASE_SCOPE_NUM="${PADDED_PHASE#0}|${PADDED_PHASE}" ;; -esac -PHASE_COMMITS=$(git log --oneline --all --extended-regexp --grep="^[[:alpha:]]+!?\((phase-)?(${PHASE_SCOPE_NUM})(-[0-9]+)?\)!?:" --format="%H" 2>/dev/null) +# #3995: a phase number is unique within a MILESTONE, not a repository. The +# former message grep had no milestone bound, and its tail -1 deliberately +# selected the OLDEST matching subject — dragging in previous milestones' +# same-numbered phases and taking a 7-file phase to a 3388-file scope (plus +# the >50 depth downgrade). The phase's own directory is the unique identity: +# base = the parent of the first commit that added anything under PHASE_DIR +# (the same anchor class git-base-branch's phaseStartCommit uses for +# complexity triggering). Message subjects demonstrably do not carry enough +# information to identify a phase — this was the grep's fifth failure. +# KNOWN RESIDUAL: git log -- does not follow renames, so a LATER +# milestone that reuses BOTH number and slug re-creates the same literal +# path and the oldest A-commit is the previous occupant's. Number+slug +# reuse is the narrow trigger; the reported archived-milestone case (dirs +# move under milestones/ on archive) is closed. +PHASE_START=$(git log --format="%H" --diff-filter=A -- "${PHASE_DIR}" 2>/dev/null | tail -1) DIFF_BASE="" -if [ -n "$PHASE_COMMITS" ]; then - DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1)^ - # Verify the parent commit exists (first commit in repo has no parent) - if ! git rev-parse "${DIFF_BASE}" >/dev/null 2>&1; then - DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1) +if [ -n "$PHASE_START" ]; then + if git rev-parse "${PHASE_START}^" >/dev/null 2>&1; then + DIFF_BASE="${PHASE_START}^" + else + DIFF_BASE="${PHASE_START}" fi fi @@ -520,22 +530,27 @@ Compute the review output path: REVIEW_PATH="${PHASE_DIR}/${PADDED_PHASE}-REVIEW.md" ``` -Compute DIFF_BASE for agent context (in case agent needs it). #3191/#3503: this -must be the SAME anchored, POSIX-portable conventional-commit-scope derivation -the Tier-3 scope step uses — the reviewer agent consumes `diff_base` exactly +Compute DIFF_BASE for agent context (in case agent needs it). #3191/#3995: this +must be the SAME phase-directory-anchor derivation the Tier-3 scope step uses — +the reviewer agent consumes `diff_base` exactly when `files:` is empty, i.e. the same fail-closed scenario Tier 3 protects, so a divergent recomputation here re-arms the mis-scoping one tier down: ```bash -PHASE_SCOPE_NUM="${PADDED_PHASE}" -case "${PADDED_PHASE}" in - 0[0-9]*) PHASE_SCOPE_NUM="${PADDED_PHASE#0}|${PADDED_PHASE}" ;; -esac -PHASE_COMMITS=$(git log --oneline --all --extended-regexp --grep="^[[:alpha:]]+!?\((phase-)?(${PHASE_SCOPE_NUM})(-[0-9]+)?\)!?:" --format="%H" 2>/dev/null) -if [ -n "$PHASE_COMMITS" ]; then - DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1)^ - # Verify the parent commit exists (first commit in repo has no parent) - if ! git rev-parse "${DIFF_BASE}" >/dev/null 2>&1; then - DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1) +# #3995: a phase number is unique within a MILESTONE, not a repository. The +# former message grep had no milestone bound, and its tail -1 deliberately +# selected the OLDEST matching subject — dragging in previous milestones' +# same-numbered phases and taking a 7-file phase to a 3388-file scope (plus +# the >50 depth downgrade). The phase's own directory is the unique identity: +# base = the parent of the first commit that added anything under PHASE_DIR +# (the same anchor class git-base-branch's phaseStartCommit uses for +# complexity triggering). Message subjects demonstrably do not carry enough +# information to identify a phase — this was the grep's fifth failure. +PHASE_START=$(git log --format="%H" --diff-filter=A -- "${PHASE_DIR}" 2>/dev/null | tail -1) +if [ -n "$PHASE_START" ]; then + if git rev-parse "${PHASE_START}^" >/dev/null 2>&1; then + DIFF_BASE="${PHASE_START}^" + else + DIFF_BASE="${PHASE_START}" fi else DIFF_BASE="" diff --git a/gsd-core/workflows/code-review/steps/structural-pre-pass.md b/gsd-core/workflows/code-review/steps/structural-pre-pass.md index f256b1077..10c033629 100644 --- a/gsd-core/workflows/code-review/steps/structural-pre-pass.md +++ b/gsd-core/workflows/code-review/steps/structural-pre-pass.md @@ -27,21 +27,19 @@ FALLOW_STDERR_TMP=$(mktemp) # Phase scope uses fallow's native changed-files scoping (--changed-since ). # Derive the phase base commit; if none is found, fall back to repo scope (fallow # auto-detects the base branch). #3191/#3503: the grep is the SAME anchored, -# POSIX-portable conventional-commit-scope derivation the workflow's Tier-3 -# scope step uses (subject-line `type((phase-)?N(-plan)?):`, padded or unpadded -# phase spelling). Free prose in commit bodies — "deferred to Phase N per -# D-09", "### Phase N" format examples — never captures the base, and a bare -# digit substring matches version strings, dates, and other phases; the oldest -# such false match would silently widen --changed-since far past the phase. +# Phase-directory-anchor derivation, lockstep with the workflow's Tier-3 +# scope step (#3191/#3995): base = the parent of the first commit that added +# anything under the phase's own directory. Commit subjects carry no milestone +# bound — a same-numbered phase in a previous milestone used to win the grep. FALLOW_SCOPE_ARGS=() if [ \"$FALLOW_SCOPE\" = \"phase\" ]; then - PHASE_SCOPE_NUM=\"${PADDED_PHASE}\" - case \"$PADDED_PHASE\" in - 0[0-9]*) PHASE_SCOPE_NUM=\"${PADDED_PHASE#0}|${PADDED_PHASE}\" ;; - esac - FALLOW_PHASE_COMMITS=$(git log --oneline --all --extended-regexp --grep=\"^[[:alpha:]]+!?\((phase-)?(${PHASE_SCOPE_NUM})(-[0-9]+)?\)!?:\" --format=\"%H\" 2>/dev/null) - if [ -n \"$FALLOW_PHASE_COMMITS\" ]; then - FALLOW_BASE=$(echo \"$FALLOW_PHASE_COMMITS\" | tail -1)^ + # #3995: phase-directory anchor — same derivation as the Tier-3 scope step + # (lockstep per #3191). A phase number is unique within a milestone, not a + # repository; the former message grep matched previous milestones' + # same-numbered phases and tail -1 selected the oldest. + FALLOW_PHASE_START=$(git log --format=\"%H\" --diff-filter=A -- \"${PHASE_DIR}\" 2>/dev/null | tail -1) + if [ -n \"$FALLOW_PHASE_START\" ]; then + FALLOW_BASE=\"${FALLOW_PHASE_START}^\" FALLOW_SCOPE_ARGS=(--changed-since \"$FALLOW_BASE\") fi fi diff --git a/scripts/lint-workflow-shellcheck-baseline.json b/scripts/lint-workflow-shellcheck-baseline.json index 9352c61ad..dd913bbb4 100644 --- a/scripts/lint-workflow-shellcheck-baseline.json +++ b/scripts/lint-workflow-shellcheck-baseline.json @@ -229,6 +229,11 @@ "code": "1073", "message": "Couldn't parse this command expansion. Fix to allow more checks." }, + { + "file": "gsd-core/workflows/code-review/steps/structural-pre-pass.md", + "code": "1083", + "message": "This { is literal. Check expression (missing ;/\\n?) or quote it." + }, { "file": "gsd-core/workflows/code-review/steps/structural-pre-pass.md", "code": "2086", diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs index 75959936e..8805c3f9f 100644 --- a/tests/code-review-pipeline-regression.test.cjs +++ b/tests/code-review-pipeline-regression.test.cjs @@ -771,7 +771,7 @@ function extractSpawnReviewerDerivation() { const src = readFileNormalized(WORKFLOW_PATH); const spawnIdx = src.indexOf(''); assert.ok(spawnIdx !== -1, 'code-review.md must have a spawn_reviewer step'); - return fenceContaining(src, 'PHASE_COMMITS=$(git log', spawnIdx); + return fenceContaining(src, 'PHASE_START=$(git log', spawnIdx); } // The fallow phase-scope derivation, from the step fragment. The fragment @@ -784,7 +784,7 @@ function extractSpawnReviewerDerivation() { // to just before the gsd_run invocation (which needs the real binary). function extractFallowDerivation() { const src = readFileNormalized(PRE_PASS_STEP_PATH); - const fence = fenceContaining(src, 'FALLOW_PHASE_COMMITS=$(git log'); + const fence = fenceContaining(src, 'FALLOW_PHASE_START=$(git log'); const scopeStart = fence.indexOf('FALLOW_SCOPE_ARGS=()'); assert.ok(scopeStart !== -1, 'fallow fence must define FALLOW_SCOPE_ARGS=()'); const cut = fence.indexOf('gsd_run run-with-timeout'); @@ -799,10 +799,14 @@ function extractFallowDerivation() { function runDerivation(repo, snippet, phase) { const script = [ `PADDED_PHASE=${phase}`, + // #3995: the derivations anchor on the phase's own directory, not a + // commit-subject grep — the fixture commits each phase's directory at + // its first scope commit. + `PHASE_DIR=${repo}/.planning/phases/${phase}-ctx`, 'FALLOW_SCOPE=phase', snippet, - 'echo "===PHASE_COMMITS==="', - 'printf \'%s\\n\' "$PHASE_COMMITS"', + 'echo "===PHASE_START==="', + 'printf \'%s\\n\' "$PHASE_START"', 'echo "===DIFF_BASE==="', 'printf \'%s\\n\' "$DIFF_BASE"', 'echo "===FALLOW_BASE==="', @@ -855,6 +859,24 @@ function parseSentinel(stdout, name) { // JS reimplementation. Running the real `git log` (not a regex shim) is what // keeps platform-level regex holes (the #3191 macOS `\b` no-op) visible. // --------------------------------------------------------------------------- +// Shared: the fixture phase directory every derivation anchors on (#3995). +const PHASE06_PLAN_REL = path.join('.planning', 'phases', '06-ctx', '06-PLAN.md'); + +// Shared history builder (was local to the #3503 describe; the #3995 rows +// reuse it). Each entry is [relPath, subject, body?]; parent dirs are created. +function buildHistory(prefix, commits) { + const repo = createTempGitProject(prefix); + const hashes = {}; + for (const [file, message, body] of commits) { + fs.mkdirSync(path.dirname(path.join(repo, file)), { recursive: true }); + fs.writeFileSync(path.join(repo, file), `${message}\n`); + gitOrThrow(['add', file], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', message, ...(body ? ['-m', body] : [])], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); + hashes[file] = gitOrThrow(['rev-parse', 'HEAD'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim(); + } + return { repo, hashes }; +} + describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all three diff-base sites', () => { const SKIP_WIN32 = { skip: process.platform === 'win32' }; @@ -862,17 +884,23 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre // from the issue — version string + date, another phase's plan whose scope // number is a digit-superset, a prose "Phase N" mention in another phase's // subject — plus the phase's real first scope commit and an unrelated HEAD. - function buildFixture(prefix, phaseCommitMessage) { + function buildFixture(prefix, phaseCommitMessage, opts = {}) { + // opts.skipPhaseDir: the fail-closed row (T5) commits NO phase directory, + // so the directory anchor must resolve nothing. + const commitPhaseDir = opts.skipPhaseDir !== true; const repo = createTempGitProject(prefix); + const phaseDir = path.join(repo, '.planning', 'phases', '06-ctx'); + fs.mkdirSync(phaseDir, { recursive: true }); const commits = [ ['c1.txt', 'chore: bump to v2.06.0 on 2026-01-05'], ['c2.txt', 'docs(60-01): unrelated phase-plan work'], - ['c3.txt', phaseCommitMessage], + [commitPhaseDir ? PHASE06_PLAN_REL : 'c3.txt', phaseCommitMessage], ['c4.txt', 'chore: Phase 60 cleanup'], ['c5.txt', 'docs: touch README'], ]; const hashes = {}; for (const [file, message] of commits) { + fs.mkdirSync(path.dirname(path.join(repo, file)), { recursive: true }); fs.writeFileSync(path.join(repo, file), `${message}\n`); gitOrThrow(['add', file], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); gitOrThrow(['commit', '-m', message], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); @@ -889,19 +917,19 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre try { const result = runDerivation(repo, extractTier3Derivation(), '06'); assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`); - const phaseCommits = parseSentinel(result.stdout, 'PHASE_COMMITS'); + const phaseStart = parseSentinel(result.stdout, 'PHASE_START'); const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); // AC: the phase's real commits are a small minority of digit-containing // commits; the derivation must resolve to an ancestor near the phase's // actual first commit (c3^) — never the older v2.06.0/docs(06-01) hits. assert.deepStrictEqual( - phaseCommits, - [hashes['c3.txt']], - `Tier-3 grep must match only the phase's real scope commit; got: ${JSON.stringify(phaseCommits)}` + phaseStart, + [hashes[PHASE06_PLAN_REL]], + `Tier-3 anchor must resolve to the phase dir's first commit; got: ${JSON.stringify(phaseStart)}` ); assert.deepStrictEqual( diffBase, - [`${hashes['c3.txt']}^`], + [`${hashes[PHASE06_PLAN_REL]}^`], 'Tier-3 DIFF_BASE must be the phase first-commit parent' ); } finally { @@ -918,19 +946,19 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre try { const result = runDerivation(repo, extractSpawnReviewerDerivation(), '06'); assert.equal(result.status, 0, `snippet exited ${result.status}; stderr=${result.stderr}`); - const phaseCommits = parseSentinel(result.stdout, 'PHASE_COMMITS'); + const phaseStart = parseSentinel(result.stdout, 'PHASE_START'); const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); // Pre-fix this matches c1 and c2 as well and tail -1 picks c1 — the // oldest unrelated match — feeding a bogus diff_base to the reviewer // agent exactly when files: is empty (the fail-closed scenario). assert.deepStrictEqual( - phaseCommits, - [hashes['c3.txt']], - `spawn_reviewer grep must match only the phase's real scope commit; got: ${JSON.stringify(phaseCommits)}` + phaseStart, + [hashes[PHASE06_PLAN_REL]], + `spawn_reviewer anchor must resolve to the phase dir's first commit; got: ${JSON.stringify(phaseStart)}` ); assert.deepStrictEqual( diffBase, - [`${hashes['c3.txt']}^`], + [`${hashes[PHASE06_PLAN_REL]}^`], 'spawn_reviewer DIFF_BASE must be the phase first-commit parent' ); } finally { @@ -953,7 +981,7 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre // and widens the structural pre-pass far beyond the phase. assert.deepStrictEqual( fallowBase, - [`${hashes['c3.txt']}^`], + [`${hashes[PHASE06_PLAN_REL]}^`], `FALLOW_BASE must be the phase first-commit parent, got: ${JSON.stringify(fallowBase)}` ); } finally { @@ -966,7 +994,7 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre 'T5: with no genuine phase scope commit, every derivation yields NO base (fail-closed preserved)', SKIP_WIN32, () => { - const { repo } = buildFixture('gsd-3191-closed-', 'feat: scanner core'); // no phase-06 scope commit anywhere + const { repo } = buildFixture('gsd-3191-closed-', 'feat: scanner core', { skipPhaseDir: true }); // no committed phase dir anywhere try { for (const [label, snippet] of [ ['tier3', extractTier3Derivation()], @@ -975,10 +1003,10 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre ]) { const result = runDerivation(repo, snippet, '06'); assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`); - const phaseCommits = parseSentinel(result.stdout, 'PHASE_COMMITS'); + const phaseStart = parseSentinel(result.stdout, 'PHASE_START'); const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE'); - assert.deepStrictEqual(phaseCommits, [], `${label}: no substring-only matches may survive`); + assert.deepStrictEqual(phaseStart, [], `${label}: no phase dir committed — anchor must stay empty`); assert.deepStrictEqual(diffBase, [], `${label}: DIFF_BASE must stay empty (no bogus base)`); assert.deepStrictEqual(fallowBase, [], `${label}: FALLOW_BASE must stay unset`); } @@ -988,118 +1016,50 @@ describe('Bug 5 (#3191) — same anchored, portable phase-scope grep at all thre } ); - // T6 docs-parity anti-revert: every `git log --grep` derivation in both - // files must use the SAME (#3191 lockstep) #3503 scope-anchored pattern — a - // subject-line conventional-commit phase scope, both padded and unpadded - // spellings via PHASE_SCOPE_NUM — and must not use `\b` under - // --extended-regexp (which silently no-ops on macOS regex(3)). - test('T6 docs-parity: all git-log grep derivations use the identical scope-anchored, POSIX-portable pattern', () => { + // T6 docs-parity anti-revert (#3191/#3995): every diff-base derivation in + // both files must use the SAME phase-directory anchor — and no message-grep + // derivation may return (a subject carries no milestone bound; that class + // failed five times: #2989/#3191/#3503/#3995). + test('T6 docs-parity: all diff-base derivations use the identical phase-directory anchor; no --grep site remains', () => { const sources = [ readFileNormalized(WORKFLOW_PATH), readFileNormalized(PRE_PASS_STEP_PATH).replace(/\\"/g, '"'), ]; - const grepLines = []; + for (const src of sources) { + assert.ok( + src.includes('PHASE_START=$(git log --format="%H" --diff-filter=A -- "${PHASE_DIR}"'), + 'each file must derive the base from the phase directory\'s first commit (#3995)' + ); + } + const grepSites = []; for (const src of sources) { for (const m of src.matchAll(/^\s*[A-Z_]+=\$\(git log[^\n]*--grep=[^\n]*$/gm)) { - grepLines.push(m[0]); + grepSites.push(m[0]); } } - assert.ok( - grepLines.length >= 3, - `expected at least 3 git-log grep derivation sites (Tier 3, spawn_reviewer, fallow); found ${grepLines.length}` + assert.deepStrictEqual( + grepSites.filter((l) => l.includes('PHASE_SCOPE_NUM') || /phase-\)?\(/.test(l)), + [], + 'no phase-scope message-grep derivation may remain — subjects carry no milestone bound (#3995)' ); - const SCOPE_GREP = '--grep="^[[:alpha:]]+!?\\((phase-)?(${PHASE_SCOPE_NUM})(-[0-9]+)?\\)!?:"'; - for (const line of grepLines) { - assert.ok( - line.includes(SCOPE_GREP), - `grep derivation must be anchored to GSD's own conventional-commit phase scope (#3503), not free prose:\n${line}` - ); - assert.ok( - line.includes('--extended-regexp'), - `grep derivation must pass --extended-regexp:\n${line}` - ); - assert.ok( - !line.includes('\\b'), - `grep derivation must not use \\b under --extended-regexp — it is not POSIX ERE and silently matches nothing on macOS (#3191):\n${line}` - ); - } - // Lockstep (#3191): all three sites must carry byte-identical grep text — - // and the padded/unpadded PHASE_SCOPE_NUM prep that feeds it. - for (const src of sources) { - assert.ok( - src.includes('PHASE_SCOPE_NUM="${PADDED_PHASE}"'), - 'each file must derive PHASE_SCOPE_NUM from PADDED_PHASE (padded/unpadded alternation)' - ); - assert.ok( - src.includes('0[0-9]*) PHASE_SCOPE_NUM="${PADDED_PHASE#0}|${PADDED_PHASE}"'), - 'each file must accept the UNPADDED phase spelling GSD workflows emit (docs(phase-6):)' - ); - } }); }); -// --------------------------------------------------------------------------- -// Bug 6 (#3503) — the diff-base grep must key on GSD's own commit scopes, -// not free prose. -// -// The #2989/#3191 anchor ("[Pp]hase N" + POSIX boundary) still resolves the -// base ~4 phases early on real repos: `git log --grep` searches FULL commit -// bodies, and `tail -1` deliberately keeps the OLDEST match — so a single -// prose mention of the phase anywhere in history (a planning commit that -// forward-references it: "deferred to Phase N per D-09"; a doc commit that -// uses "### Phase N" as a format EXAMPLE) silently captures the base, while -// GSD's own commits — which never contain the literal "Phase N", they use -// conventional-commit scopes: docs(phase-6): from execute-phase.md, -// feat(6-01):/test(6-01): from references/tdd.md, docs(6): plan commits — -// are matched by nothing. The wrong base silently inflates the reviewer's -// reading list (the #2666 SUMMARY/diff union) and widens fallow's -// --changed-since with no warning. -// -// Same behavioral style as Bug 5: the SHIPPED bash is extracted from the -// workflow .md files by content anchor and executed against a real git -// fixture whose history contains every false-positive class from the issue, -// in commit BODIES (which is where the old pattern's damage lives). -// --------------------------------------------------------------------------- -describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose mentions', () => { +describe('Bug 6 (#3503/#3995) — diff base keys on the phase directory, not commit subjects', () => { const SKIP_WIN32 = { skip: process.platform === 'win32' }; - // Commit [file, subject, body?] tuples; bodies use a second -m so they are - // real commit bodies (what `git log --grep` searches beyond the subject). - function buildHistory(prefix, commits) { - const repo = createTempGitProject(prefix); - const hashes = {}; - for (const [file, subject, body] of commits) { - fs.writeFileSync(path.join(repo, file), `${subject}\n`); - gitOrThrow(['add', file], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); - const args = body === undefined - ? ['commit', '-m', subject] - : ['commit', '-m', subject, '-m', body]; - gitOrThrow(args, { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }); - hashes[file] = gitOrThrow(['rev-parse', 'HEAD'], { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim(); - } - return { repo, hashes }; - } - - // The #3503 repro history: every prose false-positive class from the issue - // — a version-string digit substring, a planning commit whose BODY - // forward-references the phase, a doc commit whose BODY uses "### Phase N" - // as a format example — followed by the phase's GENUINE scope-style commits - // in all three spellings GSD emits (padded docs(06):, plan feat(06-01):, - // and the UNPADDED docs(phase-6): that execute-phase.md actually writes, - // since workflows interpolate the unpadded roadmap number while - // PADDED_PHASE is zero-padded). const REPRO_HISTORY = [ ['c1.txt', 'chore: bump to v2.06.0 on 2026-01-05'], ['c2.txt', 'feat(60-01): probe wiring', 'The EF path still uses it, fenced to Phase 06 per D-09.'], ['c3.txt', 'docs: commit message format', 'Phase headers use the form:\n\n### Phase 06 (Cluster B): Title\n\nin ROADMAP detail sections.'], - ['c4.txt', 'docs(06): capture phase context'], + [PHASE06_PLAN_REL, 'docs(06): capture phase context'], ['c5.txt', 'feat(06-01): implement scanner core'], ['c6.txt', 'docs(phase-6): update tracking after wave 1'], ['c7.txt', 'docs: touch README'], ]; test( - 'T1: prose forward-references and doc-format examples never capture the base — it resolves to the phase first scope commit at all three sites', + 'T1: prose forward-references and doc-format examples never capture the base — it resolves to the phase dir first commit at all three sites', SKIP_WIN32, () => { const { repo, hashes } = buildHistory('gsd-3503-scope-', REPRO_HISTORY); @@ -1112,34 +1072,22 @@ describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose menti for (const [label, snippet] of sites) { const result = runDerivation(repo, snippet, '06'); assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`); - const phaseCommits = parseSentinel(result.stdout, 'PHASE_COMMITS'); + const phaseStart = parseSentinel(result.stdout, 'PHASE_START'); const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE'); - // Pre-fix (#3503): the prose matches in c2/c3 bodies are older than - // the phase and tail -1 keeps the oldest, so DIFF_BASE resolves to - // c2^ — unboundedly before the phase — at every site. (The fallow - // snippet computes FALLOW_PHASE_COMMITS, not PHASE_COMMITS; its - // matched-set is asserted via FALLOW_BASE below.) + const dirFirst = hashes[PHASE06_PLAN_REL]; if (label !== 'fallow') { assert.deepStrictEqual( - new Set(phaseCommits || []), - new Set([hashes['c4.txt'], hashes['c5.txt'], hashes['c6.txt']]), - `${label}: grep must match exactly the phase's three scope commits; got: ${JSON.stringify(phaseCommits)}` + phaseStart, + [dirFirst], + `${label}: anchor must resolve to the phase dir's first commit; got: ${JSON.stringify(phaseStart)}` ); } - const expected = [`${hashes['c4.txt']}^`]; + const expected = [`${dirFirst}^`]; if (label === 'fallow') { - assert.deepStrictEqual( - fallowBase, - expected, - `${label}: base must be the FIRST (oldest) scope commit's parent` - ); + assert.deepStrictEqual(fallowBase, expected, `${label}: base must be the phase dir first commit's parent`); } else { - assert.deepStrictEqual( - diffBase, - expected, - `${label}: base must be the FIRST (oldest) scope commit's parent` - ); + assert.deepStrictEqual(diffBase, expected, `${label}: base must be the phase dir first commit's parent`); } } } finally { @@ -1149,12 +1097,12 @@ describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose menti ); test( - 'T2: unpadded scope spellings (docs(phase-6):, feat(6-01):) resolve identically — PADDED_PHASE is zero-padded but GSD emits the unpadded number', + 'T2: subject spellings are irrelevant to the directory anchor — unpadded and padded histories resolve identically', SKIP_WIN32, () => { const { repo, hashes } = buildHistory('gsd-3503-unpadded-', [ ['c1.txt', 'feat(60-01): probe wiring', 'Deferred to Phase 06 per D-09.'], - ['c2.txt', 'docs(phase-6): capture phase context'], + [PHASE06_PLAN_REL, 'docs(phase-6): capture phase context'], ['c3.txt', 'feat(6-01): implement scanner core'], ['c4.txt', 'test(6): persist human verification items as UAT'], ['c5.txt', 'docs: touch README'], @@ -1167,31 +1115,13 @@ describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose menti ]) { const result = runDerivation(repo, snippet, '06'); assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`); - const phaseCommits = parseSentinel(result.stdout, 'PHASE_COMMITS'); const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE'); - // (The fallow snippet computes FALLOW_PHASE_COMMITS, not - // PHASE_COMMITS; its matched set is asserted via FALLOW_BASE below.) - if (label !== 'fallow') { - assert.deepStrictEqual( - new Set(phaseCommits || []), - new Set([hashes['c2.txt'], hashes['c3.txt'], hashes['c4.txt']]), - `${label}: unpadded scope spellings must all match; got: ${JSON.stringify(phaseCommits)}` - ); - } - const expected = [`${hashes['c2.txt']}^`]; + const expected = [`${hashes[PHASE06_PLAN_REL]}^`]; if (label === 'fallow') { - assert.deepStrictEqual( - fallowBase, - expected, - `${label}: base must be the first unpadded scope commit's parent` - ); + assert.deepStrictEqual(fallowBase, expected, `${label}: base must be the phase dir first commit's parent`); } else { - assert.deepStrictEqual( - diffBase, - expected, - `${label}: base must be the first unpadded scope commit's parent` - ); + assert.deepStrictEqual(diffBase, expected, `${label}: base must be the phase dir first commit's parent`); } } } finally { @@ -1201,12 +1131,15 @@ describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose menti ); test( - 'T3: prose mentions WITHOUT any scope-style commit fail closed (no silent arbitrary base)', + 'T3: no committed phase dir fails closed (no silent arbitrary base)', SKIP_WIN32, () => { - const { repo } = buildHistory('gsd-3503-closed-', REPRO_HISTORY.slice(0, 3).concat([ + const { repo } = buildHistory('gsd-3503-closed-', [ + ['c1.txt', 'chore: bump to v2.06.0'], + ['c2.txt', 'feat(60-01): probe wiring', 'Deferred to Phase 06 per D-09.'], + ['c3.txt', 'docs(06): capture phase context'], ['c4.txt', 'docs: touch README'], - ])); + ]); try { for (const [label, snippet] of [ ['tier3', extractTier3Derivation()], @@ -1215,13 +1148,9 @@ describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose menti ]) { const result = runDerivation(repo, snippet, '06'); assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`); - const phaseCommits = parseSentinel(result.stdout, 'PHASE_COMMITS'); const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE'); - // Pre-fix (#3503): the prose bodies match, so the derivation picks a - // bogus base instead of failing closed behind the workflow warning. - assert.deepStrictEqual(phaseCommits, [], `${label}: prose mentions may not match`); - assert.deepStrictEqual(diffBase, [], `${label}: DIFF_BASE must stay empty`); + assert.deepStrictEqual(diffBase, [], `${label}: DIFF_BASE must stay empty without a committed phase dir`); assert.deepStrictEqual(fallowBase, [], `${label}: FALLOW_BASE must stay unset`); } } finally { @@ -1229,4 +1158,46 @@ describe('Bug 6 (#3503) — diff base keys on GSD commit scopes, not prose menti } } ); + + // #3995: the milestone-blind repro. A PREVIOUS milestone's phase-02 commit + // exists in history with a perfectly anchored subject; the current + // milestone's phase 02 has its own directory. The old derivation's + // unbounded grep + tail -1 selected the archived milestone's commit and + // took a 7-file phase to a 3388-file scope; the directory anchor cannot. + test( + "T4 (#3995): a previous milestone's same-numbered phase commit never captures the base", + SKIP_WIN32, + () => { + const oldMilestonePhase = path.join('.planning', 'milestones', 'v1.1-phases', '02-old', '02-PLAN.md'); + const currentPhase = path.join('.planning', 'phases', '02-ctx', '02-PLAN.md'); + const { repo, hashes } = buildHistory('gsd-3995-milestone-', [ + [oldMilestonePhase, 'feat(02-01): research-project command, workflow, and template'], + ['mid.txt', 'chore: close milestone v1.1'], + [currentPhase, 'feat(02-01): current milestone phase 02 plan 01'], + ['c4.txt', 'docs: touch README'], + ]); + try { + for (const [label, snippet] of [ + ['tier3', extractTier3Derivation()], + ['spawn_reviewer', extractSpawnReviewerDerivation()], + ['fallow', extractFallowDerivation()], + ]) { + const result = runDerivation(repo, snippet, '02'); + assert.equal(result.status, 0, `${label} exited ${result.status}; stderr=${result.stderr}`); + const diffBase = parseSentinel(result.stdout, 'DIFF_BASE'); + const fallowBase = parseSentinel(result.stdout, 'FALLOW_BASE'); + const expected = [`${hashes[currentPhase]}^`]; + if (label === 'fallow') { + assert.deepStrictEqual(fallowBase, expected, + `${label}: base must be the CURRENT phase dir's first commit, never the archived milestone's (#3995)`); + } else { + assert.deepStrictEqual(diffBase, expected, + `${label}: base must be the CURRENT phase dir's first commit, never the archived milestone's (#3995)`); + } + } + } finally { + cleanup(repo); + } + } + ); });