d7b5b2c2b6
* feat(#4906): migrate the ROADMAP.md **Plans:** line onto the PlanningDoc seam Phase 2 of epic #4906. Migrates the two writers of ROADMAP.md's Plans field onto the seam ADR-4910 locks, and deletes both bespoke regexes per Decision 2 — a correct copy of a rule the seam now owns is the same divergence risk as an incorrect one. src/phase.cts's mutateMilestonePhase carried planCountBodyPattern, one capture group, replace-to-end-of-line: this is #4852, still live before this change. src/roadmap.cts's cmdRoadmapUpdatePlanProgress carried the correct three-arm sibling (the #2853/#3584 correction) that phase.cts never adopted. Both now call findField/setFieldValue/serialize against a PlanningDoc parsed from the same milestone/phase-confined substring their existing withPhaseSection / replaceInCurrentMilestone wrappers already compute — those confinement wrappers are unchanged, only the field-write mechanism inside them moved. Verified end-to-end through the real commands against real fixtures, not against an isolated reimplementation of the classification logic: cmdRoadmapUpdatePlanProgress and cmdPhaseComplete both preserve a trailing human annotation across a real count rewrite, and both leave a bracketed human annotation (the #3584 Finding A discriminator) untouched. Found and fixed inline, in the already-merged src/planning-document.cts, rather than deferred: BOLD_FIELD_RE recognized only **Label:** (colon inside the closing bold). gsd-core/templates/roadmap.md ships every field, Plans included, as **Label**: (colon outside) -- migrating roadmap.cts onto the seam as it stood would have silently regressed real generated ROADMAP.md files back to the bug this migration exists to remove. Widened to recognize both spellings; deliberately NOT widened to a bare unbolded Label: form, which would register ordinary prose as a spurious field. roadmap.cts's writer also recognizes a bare singular/plural count (1 plan / 3 plans, no fraction) as an existing count token to overwrite, not template- placeholder or freeform prose -- the template's own single-plan-phase shape and #3584 Finding B's fix. Preserved exactly; this shape is easy to drop by only porting the more common fraction form. Two of Phase 2's three originally-cited defects turned out to be already fixed on next, independent of this epic, and are struck via a dated ADR amendment rather than silently narrowed: #4862 (stateReplaceField's own anchoring hardening already preserves sibling fields) and #4499 (spliceFrontmatter's per-key preservation already keeps block sequences byte-identical). Both reproduced against the built module before being struck, not assumed. STATE.md's field-write engine is re-scoped out of this phase entirely -- not because of its get_impact rating alone (measured the same way, the two sites THIS phase keeps are also CRITICAL, and an earlier draft of the amendment claimed otherwise without checking; corrected) but because updateCore is a multi-field transaction with frontmatter sync and preservation reconciliation that does not map onto PlanningDoc's node model, where migrating it would mean designing that model, not calling an existing seam function. Refs #4852 Refs #4906 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4906): preserve a trailing annotation with no em-dash separator Test authoring surfaced a real regression against an existing #3584 fixture: `**Plans**: 0/1 plans executed (11-16 are gap closure from VERIFICATION)` -- a parenthetical annotation glued on with a bare space, no em-dash -- was left completely untouched by the migrated code instead of being rewritten with the count updated and the parenthetical preserved. Root cause: planning-document.cts's parseBoldFieldLine splits a field's value from its trailing annotation only on the literal " -- " separator. An em-dash-separated annotation already lives outside `value` in `trailingSpan`, untouched by setFieldValue regardless -- that path was never broken. A parenthetical with no em-dash has nowhere to go but inside `value`, and the migrated classification required the WHOLE value to match a count-token shape exactly, so this case fell into "leave untouched." Fixed in the migrated call sites, not in the seam: prefix-match the count token against the field's current value, then re-glue whatever textual suffix follows WITHIN that value onto the new count text before writing. Correct for both shapes with no special-casing -- the em-dash case's suffix-within-value is empty by construction (the annotation already lives outside value), the parenthetical case's suffix is exactly the glued content, preserved verbatim. Deliberately not fixed by widening planning-document.cts's separator grammar to also recognize a bare-space-then-parenthesis: that seam is already-merged and already-tested, and guessing at an open-ended set of annotation shapes at the seam level is exactly what isTemplatePlaceholder already avoids by staying caller-side. "What counts as a Plans-field count token" is domain knowledge about this one field. phase.cts's writePlansField had no arm-2/arm-3 classification before this migration -- its original regex unconditionally overwrote whatever value was present. That unconditional-overwrite behavior is preserved exactly for values with no recognizable count-token prefix; only the recognized-count case gained suffix preservation, matching what phase.cts actually did before. Verified end-to-end via the real cmdRoadmapUpdatePlanProgress and cmdPhaseComplete commands against real fixtures for both the parenthetical and em-dash shapes at both sites. Refs #4906 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4906): cover the migrated Plans-line writers at both sites 15 cases across tests/phase.test.cjs and tests/roadmap.test.cjs, one per row of the phase test matrix, extending the existing describe blocks and helper functions those files already use for these two commands rather than building parallel fixtures. Covers: trailing-prose preservation on a real count rewrite at both sites (the #4852 regression, and the #2853/#3584 non-regression); zero-trailing- content boundary; the bracketed-template-placeholder vs bracketed-human- annotation discriminator (#3584 Finding A); a missing Plans field not crashing either command; an unrelated unreadable sibling node in the same confined section surfacing rather than corrupting the field; confinement holding across sibling phases and milestones; a round trip through the command's own read path; CRLF safety; and a parity assertion that both sites now produce identical Plans-line text for identical inputs, proving one shared mechanism rather than source-grepping for the deleted regex literals. Authoring caught a real regression before it could land silently: an existing #3584 fixture using a parenthetical annotation with no em-dash separator failed against the first version of the migration. Reported rather than edited to match the broken behavior -- see the paired fix commit. That existing test needed no changes once the fix landed; its assertion was verified independently against the real CLI before this commit. Refs #4906 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4906): give phase.cts's Plans writer the same arm-2/arm-3 classification as roadmap.cts Isolated adversarial review (mandatory orthogonal review pass) executed writePlansField against `[Deferred pending re-scope]` and the fresh-template placeholder wording and found the first version of this migration kept phase.cts's OLD unconditional-overwrite behavior for the no-count-prefix case instead of adopting the same isTemplatePlaceholder / arm-3-untouched classification roadmap.cts's sibling site already uses. A bracketed human annotation was being silently rewritten to a new count -- a real content-destroying regression against this phase's own design-doc behavior table, not an accepted trade-off, and exactly the kind of divergence between the two sites Decision 2's parity requirement exists to eliminate. writePlansField now runs the same template-placeholder check and "no count prefix and not a placeholder => leave untouched" branch before ever calling setFieldValue. Added two site-1 tests (rows 4 and 6 of the phase test matrix) mirroring the existing site-2 coverage for this exact discriminator. Refs #4906 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4906): restore plain (non-bold) Plans: line support and fix a test helper mismatch gsd-test (mandatory verification, run before every push) surfaced two real defects the migration's manual CLI checks had not caught: 1. #1163 regression: a hand-edited/pre-template ROADMAP.md can carry a PLAIN (non-bold) `Plans:` line rather than the canonical `**Plans**:`/ `**Plans:**` bold field. The old deleted regexes tolerated this shape; the seam's BOLD_FIELD_RE is deliberately bold-only (widening it would register ordinary prose like "Note: see below" as a spurious field seam-wide), so the migrated writers silently no-op'd on it instead of updating the count -- a real, previously-tested behavior lost. Fixed with a caller-side fallback in both src/roadmap.cts (where the failing #1163 test lives) and src/phase.cts (added for parity, per Decision 2 -- the two sites should not diverge on which legacy shapes they tolerate): when findField finds no boldField Plans node, look for a plain `Plans:` line directly and apply the same arm-1/2/3 classification against it. This is domain knowledge about one field's legacy tolerated shape, the same class of thing isTemplatePlaceholder already keeps caller-side rather than seam grammar. 2. tests/roadmap.test.cjs's new rows 4/5 (site 2) seeded the colon-outside spelling (`**Plans**: ...`) but their plansLineIn() helper only matched colon-inside (`**Plans:**`), so both assertions compared against `undefined` regardless of whether the write logic was correct -- a test-authoring bug, not a source defect. Fixed the helper to recognize both BOLD_FIELD_RE spellings, matching what the production code actually supports. Verified via a real gsd-test run before this fix (outcome: failed, 5 failures, all in tests/roadmap.test.cjs) and will be re-verified via a real gsd-test run on this commit before push, per this repo's non-rationalization rule: a red gate is fixed, never explained away. Refs #4906 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4906): backfill the Phase 2 changeset fragment's PR number pr:0 -> pr:4933, now that gh api POST /pulls has returned the real number. Refs #4906 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
6 lines
558 B
Markdown
6 lines
558 B
Markdown
---
|
|
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)
|