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

fix(#2316): stop phase complete silently dropping ghost requirement IDs (#2339)

Browse Source 
* test(#2316): fail-first tests for ghost REQ-IDs, v-heading over-match, all-orphan gap check

Red phase: 4 of 10 fail against current source (#2316-1 ghost-ID warning,
-3 requirements_updated honesty, -4a v1-heading suppression, -6b all-orphan gap
rows). The other 6 are controls/boundaries that must keep passing — including the
#1159 deferred-heading guard and the literal "TBD" placeholder boundary.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA

* fix(#2316): stop phase complete silently dropping ghost requirement IDs

phase complete parses a phase's `**Requirements**:` line from ROADMAP.md and
reconciles it into REQUIREMENTS.md. When a cited ID was registered nowhere, every
branch degraded to a no-op and the report was indistinguishable from a run that
applied every update: `requirements_updated: true, warnings: [], has_warnings:
false`, file byte-for-byte unchanged.

Four defects on that path, all long-standing (traced to 2bc295b32d, v1.2.0):

1. The only cross-check compared REQUIREMENTS.md's own body against its own
   Traceability table, so an ID cited by ROADMAP but defined in neither was
   invisible to it. `citedReqIds` is now hoisted out of the `if (reqMatch)` block
   and cross-checked; ghost IDs raise a warning through the existing
   warnings/has_warnings surface that execute-phase.md already prints.
2. `if (reqUpdate.ok)` had no `else`, so a Traceability-row write matching
   nothing was discarded silently. Misses are now recorded.
3. `requirements_updated` was set unconditionally inside `if (existsSync)`,
   reporting "the file was in the transaction" rather than "a write landed". It
   now reflects whether the content actually changed.
4. DEFERRED_HEADING_RE's bare `v\d+` alternative treated an active `## v1
   Requirements` heading as deferred, zeroing the body scan for that section.
   Dropped; `deferred|backlog|future` still match, so #1159's suite still holds.

Also fixes the same class in gap-checker: the `items.length === 0` early return
fired before ghost rows were folded in, so an all-unregistered phase reported
LESS than a partially-unregistered one. It is now gated on ghostReqIds too.

Follows the #2140 precedent in milestone.cts, which hardened this exact class for
`requirements mark-complete`; adapted to this function's single `warnings` array
rather than adding fields execute-phase.md never reads.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA

* fix(#2316): address review — milestone-aware deferred headings, probe-based ghost check

Independent review found the first pass green but wrong — the tests were built to
miss the shape that breaks.

BLOCKER: dropping the `v\d+` alternative from DEFERRED_HEADING_RE regressed #1159
against GSD's OWN shipped template. gsd-core/templates/requirements.md:35 ships
`## v2 Requirements` with the deferred-ness in the BODY PROSE ("Deferred to future
release"), not the heading — so `v\d+` was the only alternative matching it.
Deleting it made every scaffolded project emit a false traceability warning on
every phase complete, forever. The real problem is that `v\d+` matched BOTH the
active v1 and the deferred v2: deleting it swings from suppress-everything to
suppress-nothing. The heading check is now milestone-aware — a `## v<N>` heading
is deferred only when <N> is not the current milestone's major, resolved through
the existing state.cjs seam. Unresolvable milestone fails safe to the old
always-deferred behavior, since suppressing a warning beats spamming every project.

HIGH: the ghost check re-derived membership from two lossy index sets that
disagreed with the case-insensitive write paths, so it warned "not registered
anywhere" about an ID whose checkbox it had just ticked, and about a
case-mismatched ID whose write landed. It now probes the same two write surfaces
the writes use, mirroring milestone.cts's #2140 precedent (probe the write, don't
re-derive it) — which is what the original brief asked for and the first pass
didn't do.

HIGH: the cited-ID tokenizer split the rest of the line on whitespace, so every
trailing word became a "cited REQ-ID". The shipped roadmap.md template puts an
inline HTML comment on that exact line, so a fully correct run told users to
register `<!--` and `-->` as requirements. Cited IDs are now filtered to the
REQ-ID shape, matching what bodyReqIds/tableReqIds already require.

HIGH: gap-checker had the identical early-return defect 34 lines above the one
fixed — the could-not-parse branch gated ghost rows behind the same unguarded
items.length. One malformed <decisions> line made all ghost rows vanish.

Tests: the #2316-5 guard only covered `## Deferred`/`## Backlog`/`## Future` —
headings that trivially still match — omitting `## v2 Requirements`, the only
shape the change altered. Added the shipped-template fixture plus guards for each
finding above.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA

* docs(#2316): backfill PR number 2339 into changeset

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-07-17 06:49:58 -04:00
committed by GitHub
parent ada79bee97
commit 19c1f54a2a
5 changed files with 908 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
src/gap-checker.cts
View File

@@ -334,7 +334,16 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
const mismatchMsg = '## Post-Planning Gap Analysis\n\nextracted 0 of N — possible format mismatch in CONTEXT.md decisions block.\n';
// If there are also requirement items, include them in the return with the
// mismatch summary appended, so the caller still sees requirement coverage.
if (items.length > 0) {
// #2334 HIGH 1: gate on `ghostReqIds.length > 0` too — identical defect to
// the one fixed at #2316-6b (~34 lines below, at the `items.length === 0`
// early return): a phase whose EVERY cited REQ-ID is unregistered has
// `items.length === 0` (all its requirement items were filtered out at
// ~line 297) but still has real ghost rows to report. Without this guard,
// a single malformed `<decisions>` line in CONTEXT.md made an all-ghost
// phase's ghost rows silently vanish (this could-not-parse branch fell
// through to the bare `mismatchMsg`-only return below, dropping ghost
// rows that the general path further down correctly surfaces).
if (items.length > 0 || ghostReqIds.length > 0) {
const rows = sortRows([
...detectCoverage(items, planText),
...ghostReqIds.map(id => ({ source: 'REQUIREMENTS.md', item: id, status: 'Missing from REQUIREMENTS.md' })),
@@ -362,7 +371,13 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
}
// #1365: if no items at all, surface a clean no-check message.
if (items.length === 0) {
// #2316-6b: this must NOT fire when `ghostReqIds` is non-empty — a phase
// whose EVERY cited REQ-ID is unregistered has `items.length === 0` (all
// its requirement items were filtered out at ~line 297) but still has real
// ghost rows to report below. Without this guard, an all-orphan phase
// reported LESS than a partially-orphan one (which falls through to the
// general path further down and correctly surfaces its ghost rows).
if (items.length === 0 && ghostReqIds.length === 0) {
return {
enabled: true,
rows: [],

190
src/phase.cts
View File

@@ -131,6 +131,20 @@ function updateTraceabilityCell(
return { ok: true, value: before + result.value + after };
}
/**
* Extract the MAJOR version segment from a version-ish string: "v1", "v1.3",
* "V1.0", and "1.0" all yield "1"; "v2" yields "2". Used (#2334 BLOCKER fix)
* to compare a `## v<N> ...` REQUIREMENTS.md heading against the current
* milestone's version at MAJOR-version granularity only — "v1" heading vs
* milestone "v1.3" is the SAME major version and must not be treated as a
* version mismatch. Returns null when `raw` has no leading digit run (not a
* version-shaped string), which the caller treats as "cannot resolve".
*/
function extractMajorVersion(raw: string): string | null {
const m = raw.trim().match(/^v?(\d+)/i);
return m ? m[1] : null;
}
function describeNonCanonicalPlans(dirFiles: string[], matchedFiles: string[]): string | null {
const matched = new Set(matchedFiles);
const offenders = dirFiles.filter((f) => looksLikePlanFile(f) && !matched.has(f));
@@ -1919,14 +1933,45 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
const originalReqContent = fs.readFileSync(reqPath, 'utf-8');
let reqContent = originalReqContent;
// #2316: `citedReqIds` — the REQ-IDs ROADMAP's own **Requirements:**
// line for this phase actually cites — is hoisted out of the
// `if (reqMatch)` block (previously scoped only inside it) so the
// ghost-ID cross-check below (~#2316-1) can consult it. `TBD` is the
// literal placeholder `phase.add`/`-batch`/`-insert` seed
// (`**Requirements**: TBD`, src/phase.cts:833,920,1078) — never a
// real REQ-ID, so it is filtered out wherever a cited-ID list feeds
// a warning (#2316-7 boundary).
const isPlaceholderReqId = (id: string): boolean => id.toUpperCase() === 'TBD';
let citedReqIds: string[] = [];
// #2316-1: Traceability-row writes that matched NO row (ghost or
// otherwise) — the `if (reqUpdate.ok)` below previously had no
// `else`, discarding this fact silently instead of surfacing it.
const traceabilityWriteMisses: string[] = [];
if (reqMatch) {
const reqIds = reqMatch[1]
// #2334 HIGH 3: filter the tokenized capture to the REQ-ID SHAPE —
// the SAME shape bodyReqIds (`\*\*([A-Z][A-Z0-9]*-\d+)\*\*`, below)
// and tableReqIds (`([A-Z][A-Z0-9]*-\d+)`, below) already require —
// so the ghost-ID / unregistered comparisons stay shape-symmetric.
// Without this, `[^\n]+` split on `[,\s]+` turned EVERY word after
// the ID list into a "cited REQ-ID": the shipped
// `templates/roadmap.md:32` line
// `**Requirements**: [REQ-01, REQ-02] <!-- brackets optional, ... -->`
// warned to register `<!--`, `brackets`, `optional`, `-->`, etc., and
// `**Requirements:** None` warned to register the literal word
// `None`. This subsumes the `TBD` placeholder special-case (`TBD`
// does not match the REQ-ID shape either); `isPlaceholderReqId` is
// kept below as a defensive no-op for any caller that still hands
// it a raw token.
const REQ_ID_SHAPE_RE = /^[A-Z][A-Z0-9]*-\d+$/i;
citedReqIds = reqMatch[1]
.replace(/[\[\]]/g, '')
.split(/[,\s]+/)
.map((r) => r.trim())
.filter(Boolean);
.filter(Boolean)
.filter((r) => REQ_ID_SHAPE_RE.test(r));
for (const reqId of reqIds) {
for (const reqId of citedReqIds) {
const reqEscaped = escapeRegex(reqId);
reqContent = reqContent.replace(
new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'),
@@ -1951,14 +1996,19 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
// updateTableCell call both probes and writes.
const reqUpdate = updateTraceabilityCell(reqContent, reqRowMatch, 'Status', (current) =>
/^(?:pending|in progress)$/i.test(current.trim()) ? ' Complete ' : current);
if (reqUpdate.ok) reqContent = reqUpdate.value;
if (reqUpdate.ok) {
reqContent = reqUpdate.value;
} else if (!isPlaceholderReqId(reqId)) {
traceabilityWriteMisses.push(reqId);
}
}
}
// #1159 (Defect B): collect requirement IDs only from ACTIVE sections.
// Requirements under headings whose text contains "deferred", "backlog",
// "future", or "v2" (case-insensitive) are explicitly out of current scope
// and must not be flagged as missing from the Traceability table.
// "future", or an OFF-milestone `v<N>` (case-insensitive) are explicitly
// out of current scope and must not be flagged as missing from the
// Traceability table.
//
// Strategy: walk lines, track heading depth, and toggle a "deferred" flag
// when a heading matching the pattern is encountered. A sub-heading (higher
@@ -1966,7 +2016,47 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
// opens a same-or-shallower heading that does NOT match the pattern.
// Lines inside fenced code blocks (``` or ~~~) are treated as content, not
// headings, to avoid false deferred-section detection from code examples.
const DEFERRED_HEADING_RE = /\b(?:deferred|backlog|future|v\d+)\b/i;
//
// #2334 BLOCKER fix (regresses closed bug #1159 against GSD's OWN
// shipped template): #2316-4a dropped the bare `v\d+` alternative
// entirely to stop it over-matching an ACTIVE heading like "## v1
// Requirements" — but the shipped `templates/requirements.md:35`
// scaffold ships `## v2 Requirements` / "Deferred to future release"
// as its ONLY deferred marker, and `v\d+` was the ONLY alternative
// that ever matched a bare version heading (the deferred-ness lives
// in body prose, not the heading text). Dropping it regressed #1159
// for every project scaffolded from the shipped template.
//
// Fix: make the `v<N>` alternative MILESTONE-AWARE instead of
// deleting it. A `## v<N> ...` heading is deferred ONLY when `<N>`
// (MAJOR version only — "v1" vs milestone "v1.3" is the SAME major
// version) does not match the CURRENT milestone's major version,
// resolved via `stateExtractField` against STATE.md's `milestone:`
// frontmatter field (the same seam `getMilestoneInfo`/state.cts's
// frontmatter builder already use — no bespoke frontmatter parsing).
// "## v1 Requirements" while the milestone is v1.x is the ACTIVE
// milestone's own section (#2316's original ask) and must NOT be
// swallowed; "## v2 Requirements" while the milestone is v1.x is a
// genuinely future milestone (#1159's ask, and the literal shipped-
// template shape) and MUST stay suppressed. `deferred`/`backlog`/
// `future` are unaffected by milestone resolution — a genuinely
// deferred heading always spells one of those words too (see
// #2316-5 regression guard: "## Deferred v2 Requirements", "##
// Future Backlog", "## Deferred", "## Backlog", "## Future").
//
// Fail-safe: when the milestone version cannot be resolved at all
// (no STATE.md, or no `milestone:` field), fall back to the OLD
// pre-#2316-4a behavior and treat every `v\d+` heading as deferred.
// A false "deferred" here only ever SUPPRESSES a warning — strictly
// safer than spamming a warning on every v\d+-headed scaffold when
// we cannot tell whether it names the active milestone.
const DEFERRED_KEYWORD_RE = /\b(?:deferred|backlog|future)\b/i;
const HEADING_VERSION_RE = /\bv(\d+)(?:\.\d+)*\b/i;
const stateRawForMilestone = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : null;
const currentMilestoneRaw = stateRawForMilestone
? stateExtractField(stateRawForMilestone, 'milestone')
: null;
const currentMilestoneMajor = currentMilestoneRaw ? extractMajorVersion(currentMilestoneRaw) : null;
const bodyReqIds: string[] = [];
// deferredDepth: the heading level that opened the current deferred block,
// or 0 when we are in an active section.
@@ -1990,10 +2080,19 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
}
// Heading at same level or shallower than current deferred opener,
// or no active deferred block yet.
if (DEFERRED_HEADING_RE.test(text)) {
if (DEFERRED_KEYWORD_RE.test(text)) {
deferredDepth = depth; // enter a deferred block
} else {
deferredDepth = 0; // back in an active section
const versionMatch = text.match(HEADING_VERSION_RE);
if (versionMatch) {
const headingMajor = versionMatch[1];
deferredDepth =
currentMilestoneMajor === null || headingMajor !== currentMilestoneMajor
? depth // unresolved milestone (fail-safe) or off-milestone version -> deferred
: 0; // same major version as the current milestone -> active
} else {
deferredDepth = 0; // back in an active section
}
}
continue;
}
@@ -2031,8 +2130,79 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
);
}
// #2316-1: ghost REQ-IDs — cited by ROADMAP's own **Requirements:**
// line for this phase, but registered NOWHERE in REQUIREMENTS.md
// (neither its body nor its Traceability table). The `unregistered`
// check above only ever compares REQUIREMENTS.md's own body against
// its own Traceability table; it never consults `citedReqIds`, so an
// ID that ROADMAP cites but REQUIREMENTS.md never defines at all was
// previously invisible to every guard. `TBD` (the phase.add/-batch/
// -insert placeholder) is excluded — see #2316-7 boundary.
//
// #2334 HIGH 2: classify "ghost" by PROBING THE ACTUAL WRITE
// SURFACES this same function just wrote to (:1947 checkbox,
// :1967 Traceability row) — case-insensitively — mirroring
// milestone.cts's `notFound`/`hasRow`/`doneCheckbox` classification
// (src/milestone.cts:117-141,209-215), instead of set-differencing
// `bodyReqIds` (deferred-filtered, case-sensitive, bold-only) and
// `tableReqIds` (case-sensitive) against `citedReqIds`. Those two
// indexes can disagree with the writes: an ID under a `##
// Deferred` heading gets its checkbox ticked by the write loop
// above but is deliberately EXCLUDED from `bodyReqIds` by the
// deferred-heading filter (#1159), so the old set-diff reported it
// as an unregistered ghost in the SAME response that just ticked
// its checkbox; a case-mismatched citation (`known-01` vs
// `**KNOWN-01**`) lands its write via the writes' case-insensitive
// regexes but failed the old set-diff's case-SENSITIVE
// `Array.includes`/`Set.has`. An ID whose checkbox OR Traceability
// row actually matched is registered — not a ghost — regardless of
// which section (deferred or not) it lives under.
const reqIsRegisteredAnywhere = (id: string): boolean => {
const reqEscaped = escapeRegex(id);
// Surface 1 — checkbox, EITHER state (`[ ]` or `[x]`), case-
// insensitive: existence check, not the write's space-only match.
if (new RegExp(`-\\s*\\[[ xX]\\]\\s*\\*\\*${reqEscaped}\\*\\*`, 'i').test(reqContent)) {
return true;
}
// Surface 2 — Traceability row exists at all (any Status value),
// via the SAME no-op-probe-through-updateTraceabilityCell
// technique milestone.cts's `hasRow` uses (:210-214): a case-
// insensitive first-cell match, regardless of current Status.
const rowProbeMatch = (row: Record<string, string>): boolean =>
(Object.values(row)[0] ?? '').trim().toLowerCase() === id.toLowerCase();
return updateTraceabilityCell(reqContent, rowProbeMatch, 'Status', (current) => current).ok;
};
const ghostReqIds = citedReqIds.filter(
(id) => !isPlaceholderReqId(id) && !reqIsRegisteredAnywhere(id),
);
if (ghostReqIds.length > 0) {
warnings.push(
`ROADMAP Phase ${phaseNum} cites REQ-ID(s) not registered anywhere in REQUIREMENTS.md (neither body nor Traceability table): ${ghostReqIds.join(', ')} — add them to REQUIREMENTS.md or correct the ROADMAP citation`,
);
}
// #2316-1 cont.: a cited ID whose Traceability-row write matched no
// row for a reason OTHER than being a ghost (e.g. a malformed table)
// still deserves a warning instead of a silent discard — but skip
// IDs already reported above as ghosts to avoid a duplicate message
// for the same root cause.
const traceabilityWriteFailures = traceabilityWriteMisses.filter(
(id) => !ghostReqIds.includes(id),
);
if (traceabilityWriteFailures.length > 0) {
warnings.push(
`REQUIREMENTS.md: Traceability row write skipped for REQ-ID(s) cited by ROADMAP (no matching row found): ${traceabilityWriteFailures.join(', ')}`,
);
}
writes.push({ filePath: reqPath, before: originalReqContent, after: reqContent });
requirementsUpdated = true;
// #2316-3: `requirements_updated` must reflect whether REQUIREMENTS.md
// content actually CHANGED, not merely that the file existed in the
// transaction — mirrors the `writes.push({filePath,before,after})`
// diff-tracking pattern used for the ROADMAP write above. A phase
// whose citations match nothing (ghost REQ-IDs only) must report
// `false`, not a bare "the file was present" `true`.
requirementsUpdated = reqContent !== originalReqContent;
}
}