golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

fix(#3280): read frontmatter current_phase in w011 and de-force w027 worktree advice (#3452)

Browse Source 
* 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 <sim@local>
This commit is contained in:
Tom Boucher
2026-08-14 02:46:16 -04:00
committed by GitHub
parent 8fba67a674
commit 997901847b
7 changed files with 377 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/fierce-hawks-climb.md Normal file
View File

@@ -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 <path> status --porcelain), removing non-destructively when clean, with --force presented as an explicit opt-in to discard changes.

24
src/health-diagnostic-rules/state-consistency.cts
View File

@@ -57,6 +57,9 @@ type PlanningSnapshot = ReturnType<typeof planningSnapshotMod.buildPlanningSnaps
import phaseIdMod = require('../phase-id.cjs');
const { getMilestoneFromPhaseId, matchPhaseDirs, normalizePhaseName, extractPhaseToken, PHASE_NUMBER_TOKEN_SOURCE } =
phaseIdMod;
// eslint-disable-next-line @typescript-eslint/no-require-imports -- the single owner of the persisted status vocabulary
import stateDocumentMod = require('../state-document.cjs');
const { normalizeStateStatus } = stateDocumentMod;
// ─── W024 — STATE.md commit-age freshness (DELIBERATELY INERT) ─────────────
@@ -197,7 +200,12 @@ const RULE_W002: Rule = {
* (which uses `Phase: [X] of [Y] ([Phase name])` under `## Current
* Position`) — `currentPhaseLabel` is the parsed owner of that exact field,
* so extracting its leading number is the equivalent-intent read against
* the template STATE.md actually ships.
* the template STATE.md actually ships. #3280: the label's ladder
* (`buildStateFields`, `src/planning-snapshot.cts`) now leads with the
* frontmatter `current_phase` scalar — the key `gsd-tools state update` /
* `state begin-phase` persist — so a bare `"2"` is the expected value on
* the machine-readable format, and this leading-number extraction handles
* it identically.
*/
function currentPhaseIdFromLabel(label: string | null): string | null {
if (!label) return null;
@@ -215,13 +223,21 @@ const RULE_W011: Rule = {
if (phaseId === null) return [];
const checked = snapshot.roadmapPhaseCheckboxes.value[phaseId];
if (checked !== true) return [];
const statusVal = (snapshot.stateStatus.value ?? '').trim().toLowerCase();
if (statusVal === 'complete' || statusVal === 'done') return [];
// #3280: the state writer persists `status` through `normalizeStateStatus`
// (`state.cts`'s syncStateFrontmatter), whose completion token is
// `completed` — an exact `'complete' || 'done'` comparison rejects the
// exact vocabulary the product writes and turns every legitimately
// completed frontmatter STATE.md into a false positive. Route the
// comparison through the same seam that owns the vocabulary
// (`state-document.cjs`'s `normalizeStateStatus`) rather than growing a
// second bespoke token list here.
const statusVal = (snapshot.stateStatus.value ?? '').trim();
if (normalizeStateStatus(statusVal, null) === 'completed') return [];
return [
{
code: 'W011',
severity: SEVERITY.WARNING,
message: `STATE.md says current phase is ${phaseId} (status: ${statusVal || 'unknown'}) but ROADMAP.md shows it as [x] complete — state files may be out of sync`,
message: `STATE.md says current phase is ${phaseId} (status: ${statusVal.toLowerCase() || 'unknown'}) but ROADMAP.md shows it as [x] complete — state files may be out of sync`,
remedy: adviseRemedy('Run /gsd-progress to re-derive current position, or manually update STATE.md'),
},
];

12
src/health-diagnostic-rules/worktree-health.cts
View File

@@ -153,8 +153,16 @@ function checkW027(snapshot: PlanningSnapshot): Diagnostic[] {
diagnostics.push({
code: 'W027',
severity: SEVERITY.WARNING,
message: `Stale git worktree: ${finding.path} (last modified ${finding.ageMinutes} minutes ago). Run: git worktree remove ${finding.path} --force`,
remedy: adviseRemedy('git worktree remove <path> --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 <path> status --porcelain; if clean: git worktree remove <path>; add --force only to discard changes',
),
});
}
return diagnostics;

33
src/planning-snapshot.cts
View File

@@ -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,

113
tests/health-diagnostic-rules/state-consistency.test.cjs
View File

@@ -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 ──────────

128
tests/health-diagnostic-rules/worktree-health.test.cjs
View File

@@ -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 <path> 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 <path> --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 <path> --force' },
args: {
command:
'git -C <path> status --porcelain; if clean: git worktree remove <path>; 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 <path> --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 <path> 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');
});
});
});

81
tests/health-validation.test.cjs
View File

@@ -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)}`
);
});
});
// ─────────────────────────────────────────────────────────────────────────────