From 997901847b3cb7b4f12898d0fbd8a6fdcc41145b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 02:46:16 -0400 Subject: [PATCH] fix(#3280): read frontmatter current_phase in w011 and de-force w027 worktree advice (#3452) * fix(#3280): read frontmatter current_phase in w011 and de-force w027 worktree advice * chore(#3280): add changeset * chore(#3280): reference pr 3452 in changeset --------- Co-authored-by: sim --- .changeset/fierce-hawks-climb.md | 5 + .../state-consistency.cts | 24 +++- .../worktree-health.cts | 12 +- src/planning-snapshot.cts | 33 +++-- .../state-consistency.test.cjs | 113 ++++++++++++++++ .../worktree-health.test.cjs | 128 +++++++++++++++++- tests/health-validation.test.cjs | 81 +++++++++++ 7 files changed, 377 insertions(+), 19 deletions(-) create mode 100644 .changeset/fierce-hawks-climb.md diff --git a/.changeset/fierce-hawks-climb.md b/.changeset/fierce-hawks-climb.md new file mode 100644 index 000000000..d8989275b --- /dev/null +++ b/.changeset/fierce-hawks-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3452 +--- +gsd-health's STATE/ROADMAP staleness warning (W011) now reads the current phase from the YAML frontmatter format gsd-tools itself writes (current_phase), in addition to the legacy prose, canonical body, and pipe-table forms, and suppresses the warning when the recorded status reports completion in the state writer's own vocabulary (status: completed). The stale-worktree warning (W027) no longer advises unconditional forced removal: its remediation now directs checking for uncommitted work first (git -C status --porcelain), removing non-destructively when clean, with --force presented as an explicit opt-in to discard changes. diff --git a/src/health-diagnostic-rules/state-consistency.cts b/src/health-diagnostic-rules/state-consistency.cts index 1f88b43ff..a141f35e4 100644 --- a/src/health-diagnostic-rules/state-consistency.cts +++ b/src/health-diagnostic-rules/state-consistency.cts @@ -57,6 +57,9 @@ type PlanningSnapshot = ReturnType --force'), + // #3280: staleness is a pure mtime heuristic and carries no information + // about whether the tree is clean — git's own refusal of a non-forced + // removal on a dirty tree is the safety net, so the remediation must + // direct the operator (or an agent executing this literally) to check + // for uncommitted work FIRST and keep `--force` an explicit discard + // opt-in, never the default instruction. + message: `Stale git worktree: ${finding.path} (last modified ${finding.ageMinutes} minutes ago). Inspect uncommitted work first: git -C ${finding.path} status --porcelain; if clean run: git worktree remove ${finding.path}; only add --force to discard changes`, + remedy: adviseRemedy( + 'git -C status --porcelain; if clean: git worktree remove ; add --force only to discard changes', + ), }); } return diagnostics; diff --git a/src/planning-snapshot.cts b/src/planning-snapshot.cts index e918a96fd..82dc0b848 100644 --- a/src/planning-snapshot.cts +++ b/src/planning-snapshot.cts @@ -307,13 +307,22 @@ function buildStateFields(statePath: string): StateFields { const section = stateCurrentPositionSlice(body); const currentPositionScope = section === null ? SCOPE.TRUNCATED : SCOPE.COMPLETE; - // #1760 fallback ladder (mirrors `state.cts:1499-1500`'s `resolveStatePhase` - // exactly, same `section ?? body` scope for both reads): the legacy bold - // `**Current Phase:**` field (what `verify.cts:2109-2111` originally - // matched, and what pre-template-migration STATE.md fixtures still use) - // takes priority over the current template's bare `Phase: [X] of [Y]` - // field — a document carrying both is read the same way `resolveStatePhase` - // reads it elsewhere. + // #1760 fallback ladder — now a full mirror of `state.cts`'s + // `resolveStatePhase` (its three-source ladder at `state.cts:1494-1516`), + // including the frontmatter step that ladder leads with: + // 1. frontmatter `current_phase` scalar — the machine-readable key + // `gsd-tools state update` / `state begin-phase` persist via + // `syncStateFrontmatter` (`state.cts:2023`), so it takes PRIORITY over + // any body field (#3280: a body-only ladder left W011 structurally + // blind to the one format the product itself writes — a stale body + // `Phase:` remnant even SHADOWED the current frontmatter value). + // 2. the legacy bold `**Current Phase:**` field (what `verify.cts:2109- + // 2111` originally matched, and what pre-template-migration STATE.md + // fixtures still use). + // 3. the current template's bare `Phase: [X] of [Y]` field. + // A document carrying several is read the same way `resolveStatePhase` + // reads it elsewhere — frontmatter first, then body, in that order. + const frontmatterCurrentPhase = stateFieldValue(frontmatter, body, 'current_phase', null); const legacyCurrentPhaseLabel = stateFieldValue(frontmatter, section ?? body, null, 'Current Phase', { scope: currentPositionScope, }); @@ -321,8 +330,14 @@ function buildStateFields(statePath: string): StateFields { scope: currentPositionScope, }); const currentPhaseLabel = { - value: legacyCurrentPhaseLabel.value ?? templateCurrentPhaseLabel.value, - scope: legacyCurrentPhaseLabel.value !== null ? legacyCurrentPhaseLabel.scope : templateCurrentPhaseLabel.scope, + value: + frontmatterCurrentPhase.value ?? legacyCurrentPhaseLabel.value ?? templateCurrentPhaseLabel.value, + scope: + frontmatterCurrentPhase.value !== null + ? frontmatterCurrentPhase.scope + : legacyCurrentPhaseLabel.value !== null + ? legacyCurrentPhaseLabel.scope + : templateCurrentPhaseLabel.scope, }; const stateStatus = stateFieldValue(frontmatter, section ?? body, 'status', 'Status', { scope: currentPositionScope, diff --git a/tests/health-diagnostic-rules/state-consistency.test.cjs b/tests/health-diagnostic-rules/state-consistency.test.cjs index 07d1e2c5d..b7fec633b 100644 --- a/tests/health-diagnostic-rules/state-consistency.test.cjs +++ b/tests/health-diagnostic-rules/state-consistency.test.cjs @@ -303,6 +303,119 @@ describe('W011 — STATE current-phase status disagrees with ROADMAP [x] checkbo const snapshot = buildPlanningSnapshot(cwd); assert.deepEqual(ruleFor('W011').check(snapshot), []); }); + + // ─── #3280 — the machine-readable frontmatter format gsd-tools itself + // writes (`state update` / `state begin-phase` persist `current_phase` + + // `status` via syncStateFrontmatter). W011's phase extraction previously + // never consulted frontmatter, so the exact staleness class it exists to + // catch went undetected on the PRIMARY written format. Fixtures added + // alongside the legacy-prose cases above (not by retrofitting them), so the + // legacy path real user repos still carry keeps its coverage. + + test('#3280 AC1: fires when the current phase is recorded in frontmatter (the format gsd-tools writes), even when a body Phase field would shadow it', (t) => { + const cwd = createTempDir('gsd-3280-w011-fm-1-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '- [x] Phase 2: Auth', '- [ ] Phase 3: Billing', ''].join('\n')); + // The shape `state.cts`'s syncStateFrontmatter persists: frontmatter owns + // current_phase/status; the body's `Phase:` line is a stale remnant the + // frontmatter must override (resolveStatePhase's own ladder order). + writeState( + cwd, + [ + '---', + 'gsd_state_version: \'1.0\'', + 'milestone: v1.0', + 'current_phase: 2', + 'status: executing', + '---', + '', + '## Current Position', + '', + 'Phase: 5 of 5 (Legacy remnant)', + '', + ].join('\n'), + ); + + const snapshot = buildPlanningSnapshot(cwd); + assert.equal(snapshot.currentPhaseLabel.value, '2', 'frontmatter current_phase must win over the body Phase field'); + const diagnostics = ruleFor('W011').check(snapshot); + + assert.equal(diagnostics.length, 1); + assert.equal(diagnostics[0].code, 'W011'); + assert.equal(diagnostics[0].severity, SEVERITY.WARNING); + assert.match(diagnostics[0].message, /STATE\.md says current phase is 2 \(status: executing\) but ROADMAP\.md shows it as \[x\] complete/); + }); + + test('#3280 AC1: fires when frontmatter carries current_phase and the body has no Phase field at all', (t) => { + const cwd = createTempDir('gsd-3280-w011-fm-2-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '- [x] Phase 2: Auth', ''].join('\n')); + writeState( + cwd, + ['---', 'current_phase: 2', 'status: planning', '---', '', '## Session', '', 'Last activity: 2026-08-01', ''].join('\n'), + ); + + const snapshot = buildPlanningSnapshot(cwd); + assert.equal(snapshot.currentPhaseLabel.value, '2'); + const diagnostics = ruleFor('W011').check(snapshot); + assert.equal(diagnostics.length, 1); + assert.equal(diagnostics[0].code, 'W011'); + }); + + test('#3280 AC3: does not fire when frontmatter status reports completion in the state writer\'s own vocabulary (status: completed)', (t) => { + const cwd = createTempDir('gsd-3280-w011-fm-3-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '- [x] Phase 2: Auth', ''].join('\n')); + // `normalizeStateStatus` — the vocabulary `syncStateFrontmatter` persists — + // emits `completed` (not `complete`/`done`), so an exact-token comparison + // here would turn every legitimately completed frontmatter STATE.md into a + // false positive the moment the phase read is fixed. + writeState( + cwd, + ['---', 'current_phase: 2', 'status: completed', '---', '', '## Current Position', '', 'Phase: 5 of 5', ''].join('\n'), + ); + + const snapshot = buildPlanningSnapshot(cwd); + assert.equal(snapshot.currentPhaseLabel.value, '2'); + assert.deepEqual(ruleFor('W011').check(snapshot), []); + }); + + test('#3280 AC3: does not fire when frontmatter status is "done" either', (t) => { + const cwd = createTempDir('gsd-3280-w011-fm-4-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '- [x] Phase 2: Auth', ''].join('\n')); + writeState(cwd, ['---', 'current_phase: 2', 'status: done', '---', ''].join('\n')); + + const snapshot = buildPlanningSnapshot(cwd); + assert.deepEqual(ruleFor('W011').check(snapshot), []); + }); + + test('#3280 AC2 (locked): fires when the current phase is recorded in a pipe table under ## Current Position', (t) => { + const cwd = createTempDir('gsd-3280-w011-pipe-1-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '- [x] Phase 2: Auth', ''].join('\n')); + writeState( + cwd, + [ + '---', + 'status: discussing', + '---', + '', + '## Current Position', + '', + '| Phase | 2 of 5 |', + '| --- | --- |', + '| Status | discussing |', + '', + ].join('\n'), + ); + + const snapshot = buildPlanningSnapshot(cwd); + assert.equal(snapshot.currentPhaseLabel.value, '2 of 5'); + const diagnostics = ruleFor('W011').check(snapshot); + assert.equal(diagnostics.length, 1); + assert.equal(diagnostics[0].code, 'W011'); + }); }); // ─── W021 — phase_id_convention integer-prefix/milestone mismatch ────────── diff --git a/tests/health-diagnostic-rules/worktree-health.test.cjs b/tests/health-diagnostic-rules/worktree-health.test.cjs index ae3e1c4c9..60dc090e9 100644 --- a/tests/health-diagnostic-rules/worktree-health.test.cjs +++ b/tests/health-diagnostic-rules/worktree-health.test.cjs @@ -44,6 +44,7 @@ const { RULES } = worktreeHealth; const { buildPlanningSnapshot } = require('../../gsd-core/bin/lib/planning-snapshot.cjs'); const { SEVERITY, REMEDY_ACTION, REMEDY_RISK } = require('../../gsd-core/bin/lib/health-diagnostic.cjs'); +const { inspectWorktreeHealth } = require('../../gsd-core/bin/lib/worktree-safety.cjs'); function planningDirOf(cwd) { return path.join(cwd, '.planning'); @@ -271,7 +272,7 @@ describe('W017 — orphan git worktree', () => { // ─── W027 — stale git worktree (NEW, split off pre-migration 'W017') ────── describe('W027 — stale git worktree', () => { - test('fires once per stale finding, message carries the interpolated command, args.command is a static template', (t) => { + test('fires once per stale finding; message/remedy check for uncommitted work BEFORE any removal and present --force only as an explicit opt-in (#3280)', (t) => { const cwd = createTempDir('gsd-3309-w027-1-'); t.after(() => cleanup(cwd)); fs.mkdirSync(planningDirOf(cwd), { recursive: true }); @@ -293,17 +294,70 @@ describe('W027 — stale git worktree', () => { d.message.startsWith(`Stale git worktree: ${stalePath} (last modified `), `message must start with the stale-worktree prefix and path: ${d.message}`, ); + // #3280: staleness is a pure mtime heuristic and carries no information + // about whether the tree is clean, so the remediation must establish a + // cleanliness check FIRST and keep --force an explicit opt-in — never an + // unconditional `Run: git worktree remove --force` instruction an + // agent can execute verbatim over uncommitted work. + const cleanlinessIdx = d.message.indexOf(`git -C ${stalePath} status --porcelain`); + const removeIdx = d.message.indexOf('git worktree remove'); + const forceIdx = d.message.indexOf('--force'); + assert.ok(cleanlinessIdx !== -1, `message must include the cleanliness check: ${d.message}`); + assert.ok(removeIdx !== -1, `message must include the removal command: ${d.message}`); assert.ok( - d.message.endsWith(`minutes ago). Run: git worktree remove ${stalePath} --force`), - `message must end with the interpolated remove command: ${d.message}`, + cleanlinessIdx < removeIdx, + `cleanliness check must come BEFORE the removal command: ${d.message}`, + ); + assert.ok( + forceIdx > removeIdx, + `--force must not be part of the base removal instruction: ${d.message}`, + ); + assert.ok( + d.message.includes('if clean run:') && d.message.includes('only add --force to discard changes'), + `removal must be conditional and --force an explicit discard opt-in: ${d.message}`, + ); + assert.ok( + !d.message.endsWith(`Run: git worktree remove ${stalePath} --force`), + `message must not end with the old unconditional forced-removal instruction: ${d.message}`, ); assert.deepEqual(d.remedy, { action: REMEDY_ACTION.ADVISE, risk: REMEDY_RISK.NONE, - args: { command: 'git worktree remove --force' }, + args: { + command: + 'git -C status --porcelain; if clean: git worktree remove ; add --force only to discard changes', + }, }); }); + test('#3280: neither message nor remedy reads as an unconditional forced removal', (t) => { + const cwd = createTempDir('gsd-3280-w027-unconditional-'); + t.after(() => cleanup(cwd)); + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + + const stalePath = path.join(cwd, 'wt-stale'); + fs.mkdirSync(stalePath, { recursive: true }); + fs.utimesSync(stalePath, new Date(), new Date(Date.now() - 2 * 60 * 60 * 1000)); + + mockGitWorktreeListOk(t, buildPorcelain(['/fake/main-repo', stalePath])); + + const snapshot = buildPlanningSnapshot(cwd); + const diagnostics = ruleFor('W027').check(snapshot); + assert.equal(diagnostics.length, 1); + + const unconditionalRe = /(?:^|[.;:]\s*)Run:\s*git worktree remove\s+\S+\s+--force\s*$/; + assert.match(diagnostics[0].message, /git -C \S+ status --porcelain/); + assert.ok(!unconditionalRe.test(diagnostics[0].message), `message must not be an unconditional forced removal: ${diagnostics[0].message}`); + assert.ok( + !/^git worktree remove --force$/.test(diagnostics[0].remedy.args.command), + `remedy must not be the bare forced-removal command: ${diagnostics[0].remedy.args.command}`, + ); + assert.ok( + diagnostics[0].remedy.args.command.startsWith('git -C status --porcelain'), + `remedy must lead with the cleanliness check: ${diagnostics[0].remedy.args.command}`, + ); + }); + test('does not fire for orphan or unverified findings — isolates from W017/W020', (t) => { const cwd = createTempDir('gsd-3309-w027-2-'); t.after(() => cleanup(cwd)); @@ -381,4 +435,70 @@ describe('W027 — stale git worktree', () => { assert.equal(diagnostics.length, 1, 'a stale worktree distinct from the active cwd must still be flagged'); assert.equal(diagnostics[0].code, 'W027'); }); + + // ─── #3280 AC7 — staleness-threshold boundary, through the clock seam ──── + // + // The classification W027 consumes is `ageMs > staleAfterMs` (STRICTLY + // greater) in `snapshotWorktreeInventory` (`src/worktree-safety.cts`). These + // drive the REAL `inspectWorktreeHealth` (the owner of that comparison and + // of the `nowMs` clock-injection seam, #1191) with BOTH clocks fixed — a + // pinned `nowMs` and an mtime set via `fs.utimesSync` — so no wall-clock is + // ever read, and assert the classification at the boundary and just either + // side of it. The git seam is exercised through `deps.execGit` (a real + // parameter of `inspectWorktreeHealth`, unlike `buildPlanningSnapshot`'s + // hardcoded `execGit`). + describe('#3280 AC7 — staleness threshold boundary (fixed clock seam)', () => { + const STALE_AFTER_MS = 60 * 60 * 1000; + const FIXED_NOW_MS = 2 * 60 * 60 * 1000; // arbitrary pinned "now" (epoch + 2h) + + function probeAtAge(t, ageMs) { + const cwd = createTempDir('gsd-3280-w027-boundary-'); + t.after(() => cleanup(cwd)); + const wtPath = path.join(cwd, 'wt-boundary'); + fs.mkdirSync(wtPath, { recursive: true }); + const fixedMtime = new Date(FIXED_NOW_MS - ageMs); + fs.utimesSync(wtPath, fixedMtime, fixedMtime); + + const result = inspectWorktreeHealth( + cwd, + { staleAfterMs: STALE_AFTER_MS, nowMs: FIXED_NOW_MS }, + { + execGit: () => ({ + exitCode: 0, + stdout: buildPorcelain(['/fake/main-repo', wtPath]), + stderr: '', + signal: null, + error: null, + timedOut: false, + }), + }, + ); + return { result, wtPath }; + } + + test('age exactly at the threshold is NOT stale (comparison is strictly greater)', (t) => { + const { result } = probeAtAge(t, STALE_AFTER_MS); + assert.equal(result.ok, true); + assert.deepEqual( + result.findings, + [], + 'a worktree exactly at the staleness threshold must not be classified stale', + ); + }); + + test('age 1ms past the threshold IS stale (the finding kind that drives W027)', (t) => { + const { result, wtPath } = probeAtAge(t, STALE_AFTER_MS + 1); + assert.equal(result.ok, true); + assert.equal(result.findings.length, 1); + assert.equal(result.findings[0].kind, 'stale'); + assert.equal(result.findings[0].path, wtPath); + assert.equal(result.findings[0].ageMinutes, 60); + }); + + test('age 1ms inside the threshold is NOT stale', (t) => { + const { result } = probeAtAge(t, STALE_AFTER_MS - 1); + assert.equal(result.ok, true); + assert.deepEqual(result.findings, [], 'a worktree just inside the staleness threshold must not be classified stale'); + }); + }); }); diff --git a/tests/health-validation.test.cjs b/tests/health-validation.test.cjs index 06b1ed66d..ca1050cfb 100644 --- a/tests/health-validation.test.cjs +++ b/tests/health-validation.test.cjs @@ -112,6 +112,87 @@ describe('W011: STATE/ROADMAP cross-validation', () => { `Should not have W011: ${JSON.stringify(output.warnings)}` ); }); + + // ─── #3280 — the STATE.md format `gsd-tools state update` / + // `state begin-phase` actually persist: YAML frontmatter `current_phase` + + // `status` (syncStateFrontmatter), not the legacy bold-prose fields. New + // fixtures added alongside the legacy cases above (which stay, since real + // user repos still carry the prose format). + + test('#3280: frontmatter current_phase + ROADMAP [x] -> W011 warning', () => { + writeMinimalProjectMd(tmpDir); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [x] Phase 3: Database Layer\n\n### Phase 3: Database Layer\n**Goal:** DB setup\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v1.0', + 'current_phase: 3', + 'current_phase_name: Database Layer', + 'status: executing', + '---', + '', + '# Session State', + '', + '## Current Position', + '', + 'Phase: 3 of 4 (Database Layer)', + '', + ].join('\n') + ); + writeValidConfigJson(tmpDir); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '03-database-layer'), { recursive: true }); + + const result = runGsdTools('validate health', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + output.warnings.some(w => w.code === 'W011'), + `Expected W011 in warnings: ${JSON.stringify(output.warnings)}` + ); + }); + + test('#3280: frontmatter current_phase + status completed (writer vocabulary) -> no W011 warning', () => { + writeMinimalProjectMd(tmpDir); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [x] Phase 3: Database Layer\n\n### Phase 3: Database Layer\n**Goal:** DB setup\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + "gsd_state_version: '1.0'", + 'current_phase: 3', + 'current_phase_name: Database Layer', + 'status: completed', + '---', + '', + '# Session State', + '', + '## Current Position', + '', + 'Phase: 3 of 4 (Database Layer)', + '', + ].join('\n') + ); + writeValidConfigJson(tmpDir); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '03-database-layer'), { recursive: true }); + + const result = runGsdTools('validate health', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + !output.warnings.some(w => w.code === 'W011'), + `Should not have W011 for a completed phase in the writer's own status vocabulary: ${JSON.stringify(output.warnings)}` + ); + }); }); // ─────────────────────────────────────────────────────────────────────────────