diff --git a/.changeset/steady-newts-sprint.md b/.changeset/steady-newts-sprint.md new file mode 100644 index 000000000..e3a36154f --- /dev/null +++ b/.changeset/steady-newts-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4933 +--- +**ROADMAP.md's `**Plans:**` line no longer drops hand-written trailing notes when a phase completes.** Both writers of the Plans field — `phase complete` and `roadmap update-plan-progress` — now go through the parse -> mutate -> serialize seam ADR-4910 locks instead of their own regexes, so a phase-complete count bump can no longer replace-to-end-of-line and silently drop a trailing human annotation, and both writers now treat a fresh-template placeholder, a real count, and freeform/bracketed prose identically. (#4852) diff --git a/docs/adr/4910-planning-document-seam.md b/docs/adr/4910-planning-document-seam.md index c52cfe1db..70ee1da58 100644 --- a/docs/adr/4910-planning-document-seam.md +++ b/docs/adr/4910-planning-document-seam.md @@ -613,3 +613,83 @@ Raised by a maintainer ruling on 2026-09-21, after §5 shipped in recorded in that PR as an interpretation of #4906's *"an unparseable shape surfaces `could-not-parse` with the offending span"* — a sentence that carries no read-or-write qualifier — and the interpretation was made without examining the write side at all. + +## Amendment (2026-09-22): two of Phase 2's cited defects are already fixed, and STATE.md's field-write engine is re-scoped out + +Phase 2's evidence list read *#4852, #4862, #4499*. Two of those three are **already fixed on +`next`, independently of this epic**, and the third — #4862 — exposed a subsystem whose blast radius +disqualifies it from a mechanical migration. All three claims below were reproduced against the +built module, not inferred from reading source. + +### #4499 is already fixed + +`src/frontmatter.cts`'s `spliceFrontmatter` already preserves untouched top-level keys verbatim, +per-key, comparing structural equality before deciding whether to regenerate a key's raw text. +Reproduced: a document with `must_haves` and `tags` block sequences, with only `wave` changed, +round-trips those two keys **byte-identically**. This predates this epic — the mechanism (`#1572` +in its own comments) already implements the identity-preservation rule Decision 3 asks for, for +YAML frontmatter specifically. + +**Struck from Phase 2's evidence.** Frontmatter is a different grammar from the body-field grammar +this seam models (`boldField` / `table` / `checklist`) — Decision 1 treats it as one opaque region, +supplied by `frontmatter.cts` as a layer, not decomposed into writable nodes. `spliceFrontmatter`'s +internal per-key YAML splicing is therefore not a "verb writes a field with its own regex" instance +in the sense this phase targets, and it is not broken. No migration is owed here. + +### #4862 is already fixed at its own level, and its subsystem is re-scoped out + +`state-document.cts`'s `stateReplaceField` already anchors its bold-field pattern to line start with +same-line-only leading whitespace (its own comments cite `#4243`). Reproduced: writing `Last +Activity` leaves a sibling `**Last Activity Description:**` field and the `state_head` frontmatter +key both intact. The exact symptom #4862 reported does not reproduce. + +**What #4862's site actually is, measured rather than assumed:** `stateReplaceField` / +`stateReplaceFieldWithFallback` carries a **CRITICAL** `get_impact(direction=both)` rating — 190+ +affected symbols, truncated as a lower bound. So, measured the same way, do `phase.cts`'s +`mutateMilestonePhase` (121) and `roadmap.cts`'s `cmdRoadmapUpdatePlanProgress` (200) — the two +sites Phase 2 *does* keep. **The CRITICAL label does not distinguish these groups**, and an earlier +draft of this amendment claimed it did without checking the second two; corrected here. All three +symbols live in large, single-file modules (`phase.cts` at 4,800+ lines, `roadmap.cts` at 1,600+, +`state-transition.cts` at 3,500+), and `direction: both` walks into every sibling function such a +file touches — a known measurement artifact of bidirectional impact on a large shared module, not +evidence specific to any one of these three symbols' actual behavior. + +**What genuinely distinguishes them is architectural, and this is the actual basis for the +re-scoping:** `stateReplaceField` is one building block inside `updateCore` +(`state-transition.cts`), which is a full read-modify-write transaction — session-vs-body field +routing (`sessionLabelsForBodyField`), a three-condition frontmatter-fallback case (`#3699` case D), +frontmatter reconstruction and re-sync, and post-write preservation reconciliation +(`readModifyWriteStateMd`). Its own comments cite four prior hardening passes against exactly the +corruption classes this epic worries about — `#3374`, `#3699`, `#4010`, `#4243` — predating #4906. +`mutateMilestonePhase` and `cmdRoadmapUpdatePlanProgress`, by contrast, are each **one field, one +grammar, a three-arm decision that collapses onto a single `setFieldValue` call plus a caller-side +pre-check**, inside a confinement window another module already computes — a substitution of +mechanism with the same inputs and outputs, not a design task. + +This is not "a verb brings its own regex to a field write." `updateCore` is a proven, +actively-maintained transactional engine that already defends against silent corruption, and it +does not map onto `PlanningDoc`'s current node model at all: there is no node concept for a +multi-field transaction, a frontmatter-derived-from-body sync pass, or a session-scoped write with +an archive-shadowing guard. Migrating it would mean designing that model, not calling an existing +seam function. + +**The STATE.md field-write engine is re-scoped out of Phase 2** on that architectural basis. It is +not defective, so there is no urgency, and its migration — if ever undertaken — needs its own design +phase with its own node-model design, not a slot inside a phase whose other deliverable is a +same-mechanism substitution in `phase.cts`/`roadmap.cts`. + +**Struck from Phase 2's evidence.** + +### What Phase 2 actually delivers + +With both struck, Phase 2's census is exactly the `**Plans:**` line: `src/phase.cts`'s +`planCountBodyPattern` (still live — one capture group, confined by `withPhaseSection` but still a +regex the seam should own) and `src/roadmap.cts`'s `planCountPattern` (the correct three-arm sibling, +still a duplicate implementation under Decision 2's "two copies that agree today are the same +defect" rule). Both write the same field on the same artifact and migrate together onto one seam +call. `#4852` remains the phase's fail-first evidence; `Refs #4852`, since it is already closed +`NOT_PLANNED`. + +No new phase number is opened for the STATE.md engine. If a future contributor wants to bring +`STATE.md` under this seam, that is new work requiring its own issue, its own design, and its own +`get_impact` accounting — not an unclaimed fragment of this phase. diff --git a/src/phase.cts b/src/phase.cts index 81661588a..17663b12c 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -75,6 +75,7 @@ import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module import stateMod = require('./state.cjs'); import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync, contentChangedAfterNormalize } from './shell-command-projection.cjs'; +import { parsePlanningDoc, findField, readNode, setFieldValue, serialize } from './planning-document.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; import { transitionCore } from './state-transition.cjs'; @@ -3728,7 +3729,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // whole-slice `.replace()` onto the seam. Applied per single physical // line by updateBullet, so the pattern no longer needs the `m` flag // (it never sees more than one line at a time); see - // planCountBodyPattern below for the sites that were migrated onto + // writePlansField below for the sites that were migrated onto // withPhaseSection instead. // // #2245 review Fix 6: this is behaviour-preserving for GSD-GENERATED @@ -3787,12 +3788,109 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // ADR-2143 §4: the plan-count write is now routed through // withPhaseSection (see mutateMilestonePhase below), which hands this - // pattern ONLY phase N's own detail-section body — so the pattern no - // longer needs its own `#{2,4}\s*Phase\s+N` anchor + skip-ahead-past- - // interior-headings lookahead; the section boundary itself confines - // the match (the #2067/#2200 boundary-crossing class is now - // structurally impossible for this site rather than regex-enforced). - const planCountBodyPattern = /(\*\*Plans:\*\*\s*)[^\n]+/i; + // seam call ONLY phase N's own detail-section body — the section + // boundary itself confines the write (the #2067/#2200 boundary- + // crossing class is structurally impossible for this site). + // + // #4906 Phase 2 (#4917/ADR-4910): migrated off the one-capture-group + // regex that replaced to end of line, dropping any hand-written + // trailing prose after the count (#4852) — onto the PlanningDoc + // `boldField` write seam, whose `valueSpan`/`trailingSpan` split + // never touches the trailing annotation. + const writePlansField = (body: string): string => { + const parsed = parsePlanningDoc(body, 'ROADMAP.md'); + if (!parsed.ok) { + preservationWarnings.push({ field: 'Plans', reason: parsed.reason }); + return body; + } + const fieldId = findField(parsed.value, 'Plans'); + if (!fieldId) { + // #4906 regression (#1163 parity, caught by gsd-test against + // roadmap.cts's sibling site): a hand-edited or pre-template + // ROADMAP.md may carry a PLAIN (non-bold) `Plans:` line rather + // than the canonical `**Plans**:`/`**Plans:**` bold field. + // BOLD_FIELD_RE stays bold-only (widening it would register + // ordinary prose as a spurious field seam-wide) — this fallback + // mirrors roadmap.cts's identical one, kept in parity per + // Decision 2 rather than letting the two sites diverge on which + // legacy shapes they tolerate. + const plainMatch = body.match(/^([ \t]*)Plans:([ \t]*)([^\r\n]*)$/m); + if (!plainMatch) { + // No `**Plans:**`/`**Plans**:`/plain `Plans:` line in this + // phase's section — nothing to write; not a failure (mirrors + // the old regex's silent no-match no-op). + return body; + } + const [whole, indent, spacing, plainValue] = plainMatch; + const plainCountPrefixMatch = plainValue.match( + /^(?:\d+\s*\/\s*\d+\s+plans(?:\s+(?:complete|executed))?|\d+\s+plans?)/i, + ); + const plainIsTemplatePlaceholder = /^\[\s*Number of plans\b[\s\S]*\]$/i.test(plainValue.trim()); + if (!plainCountPrefixMatch && !plainIsTemplatePlaceholder) { + // Arm 3: freeform prose, TBD, a bracketed human annotation, or + // an empty value — leave the field exactly as it was. + return body; + } + const plainNewCountText = `${summaryCount}/${planCount} plans complete`; + const plainSuffix = plainCountPrefixMatch ? plainValue.slice(plainCountPrefixMatch[0].length) : ''; + const newPlainLine = `${indent}Plans:${spacing}${plainNewCountText}${plainSuffix}`; + const start = plainMatch.index ?? body.indexOf(whole); + return body.slice(0, start) + newPlainLine + body.slice(start + whole.length); + } + // #4906 regression fix: PREFIX-match the existing value's count + // token and re-glue whatever follows it VERBATIM — a glued-on + // annotation with no ` — ` separator (e.g. a parenthetical like + // `0/1 plans executed (11-16 are gap closure from VERIFICATION)`) + // lives entirely inside `value` (`TRAILING_SEPARATOR_RE` in + // planning-document.cts only splits on ` — `, unchanged/correct), + // so overwriting `value` outright previously destroyed it. + // + // #4906 review finding (isolated adversarial pass): the prior + // version of this migration preserved this site's OLD + // unconditional-overwrite behavior for the no-count-prefix case, + // which clobbers arm 3 (freeform prose / TBD / a bracketed human + // annotation like `[Deferred pending re-scope]`) — a real + // regression against the design doc's own Behavior table row 4, + // not an accepted trade-off. Fixed here by adopting the SAME + // template-placeholder / arm-3-untouched classification + // roadmap.cts's sibling site already uses (isTemplatePlaceholder + + // "no count prefix and not a placeholder => leave untouched"), + // rather than letting the two migrated sites diverge on this. + const newCountText = `${summaryCount}/${planCount} plans complete`; + const current = readNode(parsed.value, fieldId); + if (!current.ok) { + return body; + } + const currentValue = current.value; + const countPrefixMatch = currentValue.match( + /^(?:\d+\s*\/\s*\d+\s+plans(?:\s+(?:complete|executed))?|\d+\s+plans?)/i, + ); + const isTemplatePlaceholder = /^\[\s*Number of plans\b[\s\S]*\]$/i.test(currentValue.trim()); + if (!countPrefixMatch && !isTemplatePlaceholder) { + // Arm 3: freeform prose, TBD, a bracketed human annotation, or an + // empty value — leave the field exactly as it was. + return body; + } + const newValueToWrite = countPrefixMatch + ? newCountText + currentValue.slice(countPrefixMatch[0].length) + : newCountText; + const staged = setFieldValue(parsed.value, fieldId, newValueToWrite); + if (!staged.ok) { + preservationWarnings.push({ field: 'Plans', reason: staged.reason }); + return body; + } + const out = serialize(staged.value); + if (!out.ok) { + // `hasUnreadableNodes` refusal (ADR-4910 amendment) — a ragged + // SIBLING node elsewhere in this same section refuses the whole + // splice. Never throw / crash the phase-complete transaction over + // a node unrelated to this write; surface it and leave `body` + // unchanged, same as any other preservation warning. + preservationWarnings.push({ field: 'Plans', reason: out.reason }); + return body; + } + return out.value; + }; const phaseInfoSummaries = phaseInfo['summaries'] as string[]; @@ -3839,7 +3937,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // section's body, so neither regex can escape into a sibling // phase's section, a shipped milestone, or a Backlog entry. s = withPhaseSection(s, phaseNum, (body) => { - let b = body.replace(planCountBodyPattern, `$1${summaryCount}/${planCount} plans complete`); + let b = writePlansField(body); for (const summaryFile of phaseInfoSummaries) { const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); if (!planId) continue; diff --git a/src/planning-document.cts b/src/planning-document.cts index 758aef507..6c95852de 100644 --- a/src/planning-document.cts +++ b/src/planning-document.cts @@ -229,7 +229,15 @@ function fencedLineIndices(lines: LineInfo[]): Set { return set; } -const BOLD_FIELD_RE = /^(\s*)(\*\*[^*\r\n]+:\*\*)([ \t]*)([^\r\n]*)$/; +/** Matches both shipped bold-field spellings: colon-inside (`**Label:**`, + * the original grammar) and colon-outside (`**Label**:`, the canonical form + * used throughout `templates/roadmap.md`). Each alternative's trailing + * marker is exactly 3 characters (`:**` or `**:`), so `token.slice(2, -3)` + * in `parseBoldFieldLine` strips the leading `**` and the spelling-specific + * trailing marker identically for both, yielding the same `label` either + * way. Deliberately excludes a bare unbolded `Label:` form — see Phase 1's + * prose-vs-field disambiguation design. */ +const BOLD_FIELD_RE = /^(\s*)(\*\*[^*\r\n]+(?::\*\*|\*\*:))([ \t]*)([^\r\n]*)$/; /** Boundary marking a hand-written trailing annotation on a field line — * the token owner must never destroy prose past this separator. */ const TRAILING_SEPARATOR_RE = / — /; diff --git a/src/roadmap.cts b/src/roadmap.cts index a098b5f77..edcfc2cc4 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -50,6 +50,9 @@ const { extractFrontmatter, parseMustHavesBlock } = frontmatter; // eslint-disable-next-line @typescript-eslint/no-require-imports import verificationMod = require('./verification.cjs'); const { isPhaseComplete } = verificationMod; +// #4906 Phase 2 (#4917/ADR-4910): the PlanningDoc parse -> mutate -> serialize +// seam, mirroring phase.cts's already-migrated `writePlansField` site. +import { parsePlanningDoc, findField, readNode, setFieldValue, serialize } from './planning-document.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -1082,7 +1085,7 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und let roadmapContent = originalContent; const phasePattern = phaseMarkdownRegexSource(phaseNum); // #4247: ONE local source for the ATX phase-heading anchor that every - // section-scoped writer below (`planCountPattern`, + // section-scoped writer below (`planSectionPattern`, // `insertRowsPatternA|B`) starts with — extracted so the target-detection // gate below reads the SAME grammar the writers anchor on, and a future // edit to one cannot drift from the other three copies. @@ -1095,7 +1098,7 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und const gateActiveRegion = gateDetailsClose === -1 ? originalContent : originalContent.slice(gateDetailsClose + ''.length); - // Heading target: the exact grammar `planCountPattern` / + // Heading target: the exact grammar `planSectionPattern` / // `insertRowsPatternA|B` anchor on (an ATX phase heading for this phase). const headingTargetFound = new RegExp(phaseHeadingAnchor, 'i').test(gateActiveRegion); // Checklist target: when the phase is complete, its own checklist bullet @@ -1193,9 +1196,16 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und // `_match` unchanged. An untouched first line cannot orphan its own // continuation on the next line, since the pattern never spans past // `\n` in the first place. - const planCountPattern = new RegExp( - `(${phaseHeadingAnchor}(?:(?!\\n#{1,4}\\s)[\\s\\S])*?(?:\\*\\*Plans\\*\\*:|\\*\\*Plans:\\*\\*|(?:^|\\n)Plans:)\\s*)(\\d+\\s*\\/\\s*\\d+\\s+plans(?:\\s+(?:complete|executed))?|\\d+\\s+plans?)?([^\\r\\n]*)`, - 'i' + // #4906 Phase 2 (#4917/ADR-4910): migrated off the one-capture-group + // regex that replaced to end of line onto the PlanningDoc `boldField` + // write seam. `planSectionPattern` scopes the match to phase N's OWN + // detail section (heading + body up to the next heading) — the same + // window `phaseHeadingAnchor`'s siblings anchor on — and the callback + // below runs parse -> classify -> (maybe) mutate -> serialize entirely + // within that section text, mirroring phase.cts's `writePlansField`. + const planSectionPattern = new RegExp( + `${phaseHeadingAnchor}(?:(?!\\n#{1,4}\\s)[\\s\\S])*`, + 'i', ); const planCountText = isComplete ? `${summaryCount}/${planCount} plans complete` @@ -1205,24 +1215,107 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und // gsd-core/templates/roadmap.md actually ships, not to "anything in // brackets" — a bracketed human annotation like `[Deferred pending // re-scope]` is structurally bracketed too but carries none of this - // wording, so it correctly falls through to arm 3 untouched. + // wording, so it correctly falls through to arm 3 untouched. Kept as a + // caller-side classifier (NOT seam grammar) reused below against the + // `boldField` node's own parsed `value`. const isTemplatePlaceholder = (value: string): boolean => { const trimmed = value.trim(); return /^\[\s*Number of plans\b[\s\S]*\]$/i.test(trimmed); }; - roadmapContent = replaceInCurrentMilestone(roadmapContent, planCountPattern, (_match, label, existingCount, trailing) => { - if (existingCount) { - // Arm 1: real count token — rewrite it, preserve the trailing annotation. - return `${label}${planCountText}${trailing}`; + // Positive detectors for the two "real count token" shapes (#2853 / + // #3584 Finding B): a fraction count (`N/M plans complete|executed`) or a + // bare singular/plural count (`N plan`/`N plans`, the fresh single-plan + // template shape at gsd-core/templates/roadmap.md:62). Either one is an + // existing count token to overwrite (arm 1), never template placeholder + // (arm 2) or freeform prose (arm 3). + // #4906 regression fix: PREFIX match (not full-string) — a real count + // token may have a glued-on annotation with no ` — ` separator (e.g. + // `0/1 plans executed (11-16 are gap closure from VERIFICATION)`), which + // `parseBoldFieldLine`'s em-dash-only trailing-content split leaves + // entirely inside `value` (TRAILING_SEPARATOR_RE in planning-document.cts + // is unchanged and correct — this is a caller-side classification fix, + // not a seam fix). Returns the matched prefix length, or -1 if no match. + const fractionCountPrefixLength = (value: string): number => { + const m = value.match(/^\d+\s*\/\s*\d+\s+plans(?:\s+(?:complete|executed))?/i); + return m ? m[0].length : -1; + }; + const bareCountPrefixLength = (value: string): number => { + const m = value.match(/^\d+\s+plans?/i); + return m ? m[0].length : -1; + }; + roadmapContent = replaceInCurrentMilestone(roadmapContent, planSectionPattern, (sectionText: string): string => { + const parsed = parsePlanningDoc(sectionText, 'ROADMAP.md'); + if (!parsed.ok) { + // Unreadable section (e.g. an unterminated frontmatter fence) — + // leave it byte-identical rather than throwing. + return sectionText; } - if (isTemplatePlaceholder(trailing)) { - // Arm 2: fresh-template placeholder — replace with the count. - return `${label}${planCountText}`; + const fieldId = findField(parsed.value, 'Plans'); + if (!fieldId) { + // #4906 regression (#1163, caught by gsd-test): a hand-edited or + // pre-template ROADMAP.md may carry a PLAIN (non-bold) `Plans:` line + // rather than the canonical `**Plans**:`/`**Plans:**` bold field. + // BOLD_FIELD_RE is deliberately bold-only (widening it to any bare + // `Label:` would register ordinary prose like "Note: see below" as a + // spurious field seam-wide) — this is domain knowledge about ONE + // field's legacy tolerated shape, the same class of thing + // `isTemplatePlaceholder` above already keeps caller-side rather + // than seam grammar, so the fallback lives here, not in + // planning-document.cts. + const plainMatch = sectionText.match(/^([ \t]*)Plans:([ \t]*)([^\r\n]*)$/m); + if (!plainMatch) { + // No `**Plans:**`/`**Plans**:`/plain `Plans:` line in this phase's + // own section — nothing to write; not a failure (mirrors the old + // regex's silent no-match no-op). + return sectionText; + } + const [whole, indent, spacing, plainValue] = plainMatch; + const plainFractionLen = fractionCountPrefixLength(plainValue); + const plainBareLen = bareCountPrefixLength(plainValue); + const plainCountPrefixLen = plainFractionLen >= 0 ? plainFractionLen : plainBareLen; + if (plainCountPrefixLen < 0 && !isTemplatePlaceholder(plainValue)) { + // Arm 3: freeform prose, TBD, a bracketed human annotation, or an + // empty value — leave the section exactly as it was. + return sectionText; + } + const plainSuffix = plainCountPrefixLen >= 0 ? plainValue.slice(plainCountPrefixLen) : ''; + const newPlainLine = `${indent}Plans:${spacing}${planCountText}${plainSuffix}`; + const start = plainMatch.index ?? sectionText.indexOf(whole); + return sectionText.slice(0, start) + newPlainLine + sectionText.slice(start + whole.length); } - // Arm 3: freeform prose, TBD, a bracketed human annotation, a wrapped - // sentence's first line, or an empty value — leave the line exactly as - // it was. - return _match; + const current = readNode(parsed.value, fieldId); + if (!current.ok) { + return sectionText; + } + const currentValue = current.value; + const fractionPrefixLen = fractionCountPrefixLength(currentValue); + const barePrefixLen = bareCountPrefixLength(currentValue); + const countPrefixLen = fractionPrefixLen >= 0 ? fractionPrefixLen : barePrefixLen; + if (countPrefixLen < 0 && !isTemplatePlaceholder(currentValue)) { + // Arm 3: freeform prose, TBD, a bracketed human annotation, or an + // empty value — leave the section exactly as it was. + return sectionText; + } + // Arm 1 (real count token, possibly with a glued-on no-separator + // annotation re-attached verbatim as `suffix`) or arm 2 (fresh-template + // placeholder, whole value replaced): write the computed count. + // `setFieldValue`'s valueSpan/trailingSpan split additionally preserves + // any EM-DASH-separated trailing annotation (#2853) automatically — no + // separate "preserve trailing" branch needed for that shape. + const suffix = countPrefixLen >= 0 ? currentValue.slice(countPrefixLen) : ''; + const newValueToWrite = planCountText + suffix; + const staged = setFieldValue(parsed.value, fieldId, newValueToWrite); + if (!staged.ok) { + return sectionText; + } + const out = serialize(staged.value); + if (!out.ok) { + // `hasUnreadableNodes` refusal (ADR-4910 amendment) — a ragged + // SIBLING node elsewhere in this same section refuses the whole + // splice. Never throw; leave the section unchanged. + return sectionText; + } + return out.value; }); // If complete: check checkbox diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index bcb6da0c7..6d85ba3ba 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -11697,6 +11697,228 @@ describe('issue #1159 (Defect B): deferred/future requirement IDs must not trigg }, ); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #4906 Phase 2 (.gsd/phase/feat-4906-phase2-field-writes/50-test-matrix.md): +// cmdPhaseComplete's `**Plans:**` write migrated onto the PlanningDoc +// parse -> setFieldValue -> serialize seam (src/planning-document.cjs). Reuses +// createFixture/capturePhaseComplete (site-1 helpers, defined earlier in this +// same folded scope) rather than hand-rolling a new invocation path. Row +// numbers refer to the locked matrix; row 6 (bracketed human annotation vs +// template placeholder) is a site-2-only row and is already covered at +// tests/phase.test.cjs's "case 11" test (bug #3584 describe block above, +// "a bracketed human annotation is preserved verbatim, not mistaken for the +// template placeholder") — not duplicated here. +// ───────────────────────────────────────────────────────────────────────────── +describe('#4906 Phase 2: cmdPhaseComplete Plans-line seam migration (site 1)', () => { + function fixtureWithPlansLines(phase1PlansLine, phase2PlansLine = '**Plans:** 0/1 plans') { + const tmpDir = createFixture('gsd-4906-phase-'); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const before = fs.readFileSync(roadmapPath, 'utf-8'); + const after = before + .replace('**Plans:** 1 plans', phase1PlansLine) + .replace('**Goal:** Build the API', `**Goal:** Build the API\n${phase2PlansLine}`); + fs.writeFileSync(roadmapPath, after); + return { tmpDir, roadmapPath }; + } + + function plansLines(content) { + return content.split(/\r?\n/).filter((l) => l.trim().startsWith('**Plans:**')); + } + + test('row 1 (#4852 regression): trailing prose on the Plans line survives a count bump', (t) => { + const annotation = '— replanned per review'; + const { tmpDir, roadmapPath } = fixtureWithPlansLines(`**Plans:** 1 plans ${annotation}`); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const [line] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, `**Plans:** 1/1 plans complete ${annotation}`, + 'annotation must survive byte-for-byte after the count bump'); + }); + + test('row 3: a Plans line with a real count and no trailing content updates cleanly', (t) => { + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** 1 plans'); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const [line] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans:** 1/1 plans complete'); + }); + + test('row 7: a missing Plans field does not crash phase.complete', (t) => { + const tmpDir = createFixture('gsd-4906-row7-'); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const before = fs.readFileSync(roadmapPath, 'utf-8'); + // Drop the `**Plans:**` line from Phase 01's own section entirely. + const after = before.replace('**Plans:** 1 plans\n', ''); + fs.writeFileSync(roadmapPath, after); + t.after(() => cleanup(tmpDir)); + // Must not throw — capturePhaseComplete throws on a non-zero exit. + assert.doesNotThrow(() => capturePhaseComplete(t, tmpDir, '1')); + }); + + test('row 8: an unreadable sibling node in the confined window is surfaced, not silently dropped', (t) => { + // Ragged table shape reused verbatim from tests/planning-document.test.cjs's + // raggedTableSource() (row 8 there): a data row with fewer cells than the + // header and no valid delimiter row for that mismatch — parsePlanningDoc + // accepts it as a table NODE but marks it unreadable, so `serialize` + // refuses the whole section splice (ADR-4910 amendment). + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** 1 plans'); + const before = fs.readFileSync(roadmapPath, 'utf-8'); + const ragged = ['', '| A | B |', '|---|---|', '| onlyone |', ''].join('\n'); + fs.writeFileSync(roadmapPath, before.replace('**Plans:** 1 plans\n', `**Plans:** 1 plans\n${ragged}`)); + t.after(() => cleanup(tmpDir)); + const stdout = capturePhaseComplete(t, tmpDir, '1'); + const result = JSON.parse(stdout); + assert.deepEqual(result.preservation_warnings, [{ field: 'Plans', reason: 'unreadable-nodes' }], + 'the refusal must be surfaced via preservation_warnings, not silently swallowed'); + const [line] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans:** 1 plans', 'the Plans field itself must stay unchanged, not corrupted'); + }); + + test('row 9: the write does not escape its confinement window into a sibling phase', (t) => { + const annotation = '— do not touch'; + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** 1 plans', `**Plans:** 0/1 plans ${annotation}`); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const [phase1Line, phase2Line] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(phase1Line, '**Plans:** 1/1 plans complete', 'phase 1 (the target) is rewritten'); + assert.equal(phase2Line, `**Plans:** 0/1 plans ${annotation}`, + "phase 2's own Plans line must stay untouched by phase 1's write"); + }); + + test('row 10: a migrated Plans write round-trips identically through the seam\'s own read path', (t) => { + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** 1 plans'); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const written = fs.readFileSync(roadmapPath, 'utf-8'); + const { parsePlanningDoc, findField, readNode } = require('../gsd-core/bin/lib/planning-document.cjs'); + const parsed = parsePlanningDoc(written, 'ROADMAP.md'); + assert.ok(parsed.ok, 'the written ROADMAP.md must remain parseable by the same seam'); + const fieldId = findField(parsed.value, 'Plans'); + assert.ok(fieldId, 'the Plans field must still be locatable'); + const read = readNode(parsed.value, fieldId); + assert.ok(read.ok, 'the Plans field must remain readable'); + assert.equal(read.value, '1/1 plans complete', 'round-tripped value matches exactly what was computed'); + }); + + test('row 12: the Plans-line migration is CRLF-safe', (t) => { + const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + const annotation = '— crlf annotation'; + const { tmpDir, roadmapPath } = fixtureWithPlansLines(`**Plans:** 1 plans ${annotation}`); + const lf = fs.readFileSync(roadmapPath, 'utf-8'); + fs.writeFileSync(roadmapPath, splitLines(lf).join('\r\n')); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const written = fs.readFileSync(roadmapPath, 'utf-8'); + const [line] = splitLines(written).filter((l) => l.trim().startsWith('**Plans:**')); + assert.equal(line, `**Plans:** 1/1 plans complete ${annotation}`, + 'CRLF source must not corrupt the offset or strand/duplicate the annotation'); + }); + + test('row 13: phase.cts and roadmap.cts write identical Plans-line text for the same inputs', (t) => { + // Parity proof (Decision 2): both sites route through the same + // planning-document seam and must produce byte-identical text for the + // same summaryCount/planCount/isComplete inputs — never asserted by + // grepping for the deleted regex literals. + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** 1 plans'); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const [phaseSiteLine] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + + const roadmapMod = require('../gsd-core/bin/lib/roadmap.cjs'); + const rmTmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4906-row13-roadmap-')); + t.after(() => cleanup(rmTmp)); + const planningDir = path.join(rmTmp, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync(path.join(planningDir, 'config.json'), JSON.stringify({ project_code: 'TEST' })); + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', '', + '- [ ] **Phase 10: Test Phase**', '', + '### Phase 10: Test Phase', + '**Goal:** goal', + '**Plans:** 1 plans', '', + '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 10 Test Phase | 0/1 | Not started | - |', '', + ].join('\n'), + ); + const rmPhaseDir = path.join(planningDir, 'phases', '10-test-phase'); + fs.mkdirSync(rmPhaseDir, { recursive: true }); + fs.writeFileSync(path.join(rmPhaseDir, '10-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(rmPhaseDir, '10-01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync(path.join(rmPhaseDir, '10-VERIFICATION.md'), '---\nstatus: passed\n---\n\n# Verification\n'); + let captured = ''; + const origWriteSync = fs.writeSync; + t.mock.method(fs, 'writeSync', (fd, ...args) => { + if (fd === 1) { + const buf = args[0]; + captured += Buffer.isBuffer(buf) ? buf.toString('utf-8') : String(buf); + return Buffer.isBuffer(buf) ? buf.length : Buffer.byteLength(String(buf)); + } + if (fd === 2) return Buffer.isBuffer(args[0]) ? args[0].length : Buffer.byteLength(String(args[0])); + return origWriteSync.call(fs, fd, ...args); + }); + roadmapMod.cmdRoadmapUpdatePlanProgress(rmTmp, '10', false); + void captured; + const [roadmapSiteLine] = fs.readFileSync(path.join(planningDir, 'ROADMAP.md'), 'utf-8') + .split(/\r?\n/) + .filter((l) => l.trim().startsWith('**Plans:**')); + + assert.equal(phaseSiteLine, '**Plans:** 1/1 plans complete'); + assert.equal(roadmapSiteLine, '**Plans:** 1/1 plans complete', + 'both sites must produce byte-identical Plans-line text for the same 1/1-complete inputs'); + assert.equal(phaseSiteLine, roadmapSiteLine); + }); + + test('row 6 (site-1 arm 3, review finding): a bracketed human annotation is left untouched, not overwritten', (t) => { + // Isolated adversarial review (#4906 Phase 2) caught that this site's + // migration preserved its OLD unconditional-overwrite behavior instead + // of adopting arm 3 from roadmap.cts's sibling classification — a + // freeform/bracketed annotation with no count-token prefix must stay + // untouched, exactly as the design doc's Behavior table row 4 requires + // and exactly as roadmap.cts's own "case 11" (#3584 Finding A) already + // proves for site 2. + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** [Deferred pending re-scope]'); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const [line] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans:** [Deferred pending re-scope]', + 'a bracketed human annotation is arm 3 (freeform), never a template placeholder or a count token, and must not be overwritten'); + }); + + test('row 6 (site-1 arm 2): the fresh-template placeholder wording is still replaced with the computed count', (t) => { + const { tmpDir, roadmapPath } = fixtureWithPlansLines('**Plans:** [Number of plans, e.g., "3 plans" or "TBD"]'); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const [line] = plansLines(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans:** 1/1 plans complete', + 'the template placeholder is arm 2 and must still be replaced with the real count'); + }); + + test('#1163 parity: a plain (non-bold) Plans: line still gets its count updated', (t) => { + // roadmap.cts's sibling site has a real, pre-existing regression test + // for this exact legacy shape (tests/roadmap.test.cjs, "regressions: + // insert missing plan rows (#1163)") — caught missing here by gsd-test, + // since this migration's first version only handled the BOLD_FIELD_RE + // grammar and silently no-op'd on a plain `Plans:` line. Fixed in + // src/phase.cts's writePlansField with the same caller-side fallback + // roadmap.cts uses, kept in parity per Decision 2. + const tmpDir = createFixture('gsd-4906-plain-plans-'); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const before = fs.readFileSync(roadmapPath, 'utf-8'); + const after = before.replace('**Plans:** 1 plans', 'Plans: 0/1 plans executed'); + fs.writeFileSync(roadmapPath, after); + t.after(() => cleanup(tmpDir)); + capturePhaseComplete(t, tmpDir, '1'); + const written = fs.readFileSync(roadmapPath, 'utf-8'); + const line = written.split(/\r?\n/).find((l) => l.trim().startsWith('Plans:')); + assert.equal(line, 'Plans: 1/1 plans complete', + 'a plain (non-bold) Plans: line must still be updated, not silently skipped'); + }); +}); }); } @@ -16083,3 +16305,4 @@ describe('phase complete skips already-complete phases as next_phase (#4699)', ( 'without the fix scope change: an unchecked phase 3 stays a valid candidate (disk spelling wins)'); }); }); + diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index 2bce4d229..34ae63897 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -5486,3 +5486,193 @@ describe('#4801: init.manager resolves archived phase directories', () => { `live plan-less phase stays incomplete; got ${row.disk_status}`); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #4906 Phase 2 (.gsd/phase/feat-4906-phase2-field-writes/50-test-matrix.md): +// cmdRoadmapUpdatePlanProgress's `**Plans:**` write migrated onto the +// PlanningDoc parse -> setFieldValue -> serialize seam (src/planning-document. +// cjs). Row numbers refer to the locked matrix. +// +// Row 6 (a bracketed HUMAN annotation, e.g. `[Deferred pending re-scope]`, +// must not be mistaken for the fresh-template placeholder) is #3584 Finding +// A's own regression fixture and is already covered — see tests/phase.test. +// cjs's "case 11 — a bracketed human annotation is preserved verbatim, not +// mistaken for the template placeholder" in the `bug #3584` describe block — +// not duplicated here per the fixture-provenance rule. +// +// Row 13 (parity between phase.cts and roadmap.cts for identical numeric +// inputs) is asserted once, at tests/phase.test.cjs's "row 13" test above; +// not duplicated here. +// ───────────────────────────────────────────────────────────────────────────── +describe('#4906 Phase 2: cmdRoadmapUpdatePlanProgress Plans-line seam migration (site 2)', () => { + let tmpDir; + let roadmapPath; + + beforeEach(() => { + tmpDir = createTempProject('gsd-4906-roadmap-'); + roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function seedPhase10(plansLine, { verified = true } = {}) { + fs.writeFileSync( + roadmapPath, + [ + '# Roadmap', '', + '- [ ] **Phase 10: Test Phase**', '', + '### Phase 10: Test Phase', + '**Goal:** goal', + plansLine, '', + '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 10 Test Phase | 0/1 | Not started | - |', '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '10-test-phase'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '10-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '10-01-SUMMARY.md'), '# Summary\n'); + if (verified) { + fs.writeFileSync(path.join(phaseDir, '10-VERIFICATION.md'), '---\nstatus: passed\n---\n\n# Verification\n'); + } + return phaseDir; + } + + function plansLineIn(content) { + // Recognizes both BOLD_FIELD_RE spellings (colon inside `**Plans:**` and + // colon outside `**Plans**:`, the canonical templates/roadmap.md shape) — + // rows 4/5 below seed the colon-outside spelling, and a finder anchored + // to only one spelling silently reports "field not found" (`undefined`) + // rather than a real assertion failure, masking whichever bug is actually + // present. Caught by gsd-test (#4906 Phase 2 verification run). + return content.split(/\r?\n/).find((l) => /^\*\*Plans(?::\*\*|\*\*:)/.test(l.trim())); + } + + test('row 2 (non-regression of #2853/#3584): trailing prose survives migration', () => { + const annotation = '— replanned per review'; + seedPhase10(`**Plans:** 1 plans ${annotation}`); + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const line = plansLineIn(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, `**Plans:** 1/1 plans complete ${annotation}`, + 'annotation must survive byte-for-byte after the count bump'); + }); + + test('row 3: a Plans line with a real count and no trailing content updates cleanly', () => { + seedPhase10('**Plans:** 1 plans'); + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const line = plansLineIn(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans:** 1/1 plans complete'); + }); + + test('row 4: the fresh-template placeholder is replaced with the real count', () => { + seedPhase10('**Plans**: [Number of plans, e.g., "3 plans" or "TBD"]'); + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const line = plansLineIn(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans**: 1/1 plans complete'); + }); + + test('row 5: freeform prose (not the template placeholder, not a real count) is left untouched', () => { + seedPhase10('**Plans**: TBD — annotation'); + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const line = plansLineIn(fs.readFileSync(roadmapPath, 'utf-8')); + assert.equal(line, '**Plans**: TBD — annotation', 'freeform prose must never be classified as a writable count'); + }); + + test('row 7: a missing Plans field does not crash roadmap.update-plan-progress', () => { + fs.writeFileSync( + roadmapPath, + [ + '# Roadmap', '', + '- [ ] **Phase 10: Test Phase**', '', + '### Phase 10: Test Phase', + '**Goal:** goal', '', + '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 10 Test Phase | 0/1 | Not started | - |', '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '10-test-phase'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '10-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '10-01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync(path.join(phaseDir, '10-VERIFICATION.md'), '---\nstatus: passed\n---\n\n# Verification\n'); + + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `must not crash on a missing Plans field: ${result.error}`); + }); + + test('row 9: the write does not escape its confinement window into a sibling phase', () => { + const annotation = '— do not touch'; + fs.writeFileSync( + roadmapPath, + [ + '# Roadmap', '', + '- [ ] **Phase 10: Test Phase**', + '- [ ] **Phase 11: Sibling Phase**', '', + '### Phase 10: Test Phase', + '**Goal:** goal', + '**Plans:** 1 plans', '', + '### Phase 11: Sibling Phase', + '**Goal:** sibling goal', + `**Plans:** 0/1 plans ${annotation}`, '', + '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 10 Test Phase | 0/1 | Not started | - |', + '| 11 Sibling Phase | 0/1 | Not started | - |', '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '10-test-phase'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '10-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '10-01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync(path.join(phaseDir, '10-VERIFICATION.md'), '---\nstatus: passed\n---\n\n# Verification\n'); + + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const lines = fs.readFileSync(roadmapPath, 'utf-8') + .split(/\r?\n/) + .filter((l) => l.trim().startsWith('**Plans:**')); + assert.equal(lines[0], '**Plans:** 1/1 plans complete', 'phase 10 (the target) is rewritten'); + assert.equal(lines[1], `**Plans:** 0/1 plans ${annotation}`, + "phase 11's own Plans line must stay untouched by phase 10's write"); + }); + + test('row 10: a migrated Plans write round-trips identically through the seam\'s own read path', () => { + seedPhase10('**Plans:** 1 plans'); + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const written = fs.readFileSync(roadmapPath, 'utf-8'); + const { parsePlanningDoc, findField, readNode } = require('../gsd-core/bin/lib/planning-document.cjs'); + const parsed = parsePlanningDoc(written, 'ROADMAP.md'); + assert.ok(parsed.ok, 'the written ROADMAP.md must remain parseable by the same seam'); + const fieldId = findField(parsed.value, 'Plans'); + assert.ok(fieldId, 'the Plans field must still be locatable'); + const read = readNode(parsed.value, fieldId); + assert.ok(read.ok, 'the Plans field must remain readable'); + assert.equal(read.value, '1/1 plans complete', 'round-tripped value matches exactly what was computed'); + }); + + test('row 12: the Plans-line migration is CRLF-safe', () => { + const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + const annotation = '— crlf annotation'; + seedPhase10(`**Plans:** 1 plans ${annotation}`); + const lf = fs.readFileSync(roadmapPath, 'utf-8'); + fs.writeFileSync(roadmapPath, splitLines(lf).join('\r\n')); + const result = runGsdTools('roadmap update-plan-progress 10', tmpDir); + assert.ok(result.success, `command failed: ${result.error}`); + const written = fs.readFileSync(roadmapPath, 'utf-8'); + const line = splitLines(written).find((l) => l.trim().startsWith('**Plans:**')); + assert.equal(line, `**Plans:** 1/1 plans complete ${annotation}`, + 'CRLF source must not corrupt the offset or strand/duplicate the annotation'); + }); +});