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

fix(#3204): milestone sectioning is vocabulary, not heading position (#3230)

Browse Source 
* test(#3204): failing-first suite for the clobbered phase count

A project declaring six phases with four phase directories on disk had
state.record-session write progress.total_phases: 4 — #2828 regressing at
1.9.1, reported in #3204 with a deterministic reproduction.

Before the fix in the following commit, these rows FAILED (wrote 4, expected
6): a flat roadmap carrying `## Progress`; one carrying `## Overview` and
`## Phase Details`; the CRLF variant of the first. Two more, found by
adversarial review and added after the first fix attempt, failed against that
attempt: structural headings interleaved among flat phase headings, and this
repo's own bundled-template shape (a `## Phases` wrapper around a single
nested milestone).

The #1761 control — sibling milestone sections must keep falling back to the
disk count — passes both before and after, so the fix has something it must
not break.

Assertions read progress.total_phases through the product's own frontmatter
parser via `state json --raw`, never a regex over STATE.md. Rows 12 and 13
are hostile: a phase heading carrying a version token, and a version heading
inside a fenced code block; neither may count as milestone sectioning.

Refs #3185, #3204

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#3204): milestone sectioning is vocabulary, not heading position

buildStateFrontmatter chooses total_phases between the ROADMAP's declared
phase count and the on-disk directory count, and refuses the roadmap count
when hasMilestoneSectioning says the document is milestone-sectioned — because
a whole-document count would then conflate sibling milestones (#1761).

That predicate returned true for ANY non-Phase level-2/3 heading, so a flat
roadmap carrying an ordinary `## Progress` was called sectioned and the disk
count clobbered the declared one: six declared phases, four directories,
total_phases written as 4, converging on the truth only once the last
directory happened to exist. That is #2828 regressing at 1.9.1, and it came
from this epic — #3184 replaced state.cts's hand-rolled #2828 guard with this
predicate, and the replacement is strictly more permissive than the guard it
retired.

Three position-based models were tried and all failed, because position does
not carry milestone-ness:

  - any non-Phase heading (shipped) — over-detects, giving #3204;
  - strict nesting/ownership — misses same-level siblings, regressing #1761,
    and false-positives on the bundled template, where `## Phases` wraps a
    single `### v1.1`;
  - adjacency — reproduced live: `## Overview` and `## Notes` interleaved
    among six phase headings are two owning candidates, so a 6-phase roadmap
    with 2 directories wrote 2.

A heading is now a milestone heading iff it is a non-Phase heading carrying a
milestone signal: a version token, a status marker, or the word Milestone.
Sectioning means two or more, since one cannot conflate siblings.

Known limit, recorded in the doc comment rather than hidden: two milestone
sections carrying none of those three signals are not detected.

Also drops buildStateFrontmatter's local dedup-key regex, flagged in-source as
diverging from the canonical token rule, for phaseKeyFromDir — the remainder
of #3185, since #3222 had already routed the enumeration itself through
listMilestonePhaseDirs.

#1514, #2445 and #3017 are preserved untouched.

Closes #3185
Fixes #3204

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(#3185): changeset, glossary entry and ADR status for the phase-count fix

CONTEXT.md's Roadmap Parser Module entry never named hasMilestoneSectioning,
so the predicate whose semantics this change reverses had no glossary presence
at all — a PR gate for a module/seam change. Added, covering the vocabulary
model, the three position-based models that failed, and the residual limit.

ADR-3180 recorded the fifth enumeration copy as unowned in four places. It is
owned now. Amendment 4's scope table row 1 also carried an error worth keeping
visible rather than rewriting: it claimed Phase 3 merged without routing the
state writers, when #3222 had in fact routed the enumeration — the audit read
Amendment 3's silence about the symbol names as absence of the work. The real
gap was the trust discriminator one layer above, which is what #3204 was.

Changeset is Fixed and leads with the symptom a user sees — a phase count that
shrinks to match how many phase directories happen to exist yet — and carries
the known limit forward rather than leaving it in a source comment.

Refs #3185, #3204

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#3185): stop quoting the retired phase-token regex in a comment

The remote runner failed tests/phase-id-drift-guard.test.cjs: the comment
explaining that the local dedup regex had been replaced by phaseKeyFromDir
quoted that regex verbatim, and scripts/lint-phase-id-drift.cjs scans for the
literal token without caring whether it sits in code or in prose.

That is the guard being right, not over-eager — a quoted pattern is one paste
away from being live again, which is exactly how the copy it replaced spread.
Described in prose instead.

Worth recording: this guard is check:phase-id-drift, which lint:ci does not
run — it is enforced by tests/phase-id-drift-guard.test.cjs. A green lint:ci
is therefore not evidence the drift guards pass.

Refs #3185

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* chore(#3185): backfill changeset PR number (#3230)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-08-08 22:01:20 -04:00
committed by GitHub
parent 86bebcefa2
commit 2a73f53cb3
6 changed files with 906 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
docs/adr/3180-planning-semantic-model-single-owner.md
View File

@@ -260,7 +260,7 @@ This section is that written rule. It is **normative**, and it is what the guard
**Guard.** `lint-milestone-window-drift.cjs`, token set widened by Phase 6 in the same change as the consolidation — never after, since a guard added later measures a surface already cleaned and reports a zero it did not earn. Token (a) now admits a **literal** `#`-run heading anchor in addition to the `#{N,M}` quantifier, but only inside a heading-**matcher** literal (a regex literal, or a string handed to `new RegExp(`), so a heading-**builder** template is not mistaken for a re-derivation. Token (b) additionally admits the grouped `v(\d+(?:\.\d+)+)` shape and an interpolated `${…Ver…}` placeholder.
#### 7.3 Phase enumeration — *Enforced for the four named consumers (Phase 3, #3222); the fifth copy is unowned*
#### 7.3 Phase enumeration — *Enforced (Phase 3, #3222 + #3185)*
**Question.** Which directories under `<planning>/phases/` are phases of milestone `M`?
@@ -270,7 +270,11 @@ This section is that written rule. It is **normative**, and it is what the guard
**Consumers that must route through the owner.** `cmdRoadmapAnalyze`, `cmdProgressRender`, `cmdStats`, `cmdPhasesList`, **and `buildStateFrontmatter` / `syncStateFrontmatter`** — the fifth copy, reached by `state.record-session`, `state.sync`, `phase.complete` and every other state-mutating verb, which the epic's original scope did not name (coverage-audit gap 1).
**Status, precisely.** Phase 3 (#3222) enforced this rule for `cmdRoadmapAnalyze`, `cmdProgressRender`, `cmdStats` and `cmdPhasesList`, and its guard (`scripts/lint-phase-enumeration-drift.cjs`) found 54 violations where the epic scoped 4. **It did not reach `buildStateFrontmatter` / `syncStateFrontmatter`.** That copy is still live, still writes its answer to disk, and is now unowned by any phase — see Amendment 4's scope table, row 1.
**Status, precisely.** Phase 3 (#3222) enforced this rule for `cmdRoadmapAnalyze`, `cmdProgressRender`, `cmdStats` and `cmdPhasesList`, and its guard (`scripts/lint-phase-enumeration-drift.cjs`) found 54 violations where the epic scoped 4. It also routed `buildStateFrontmatter`'s enumeration through `listMilestonePhaseDirs`, which Amendment 4's scope table row 1 did not credit it for — the audit read the *absence of the symbol names from Amendment 3* as the absence of the work. #3185's remainder was therefore smaller than recorded: the local dedup-key regex that survived alongside the routed enumeration, now on `phaseKeyFromDir`.
**What was actually unowned was one layer up.** `buildStateFrontmatter` decides whether it may *trust* the ROADMAP's declared count via `hasMilestoneSectioning` (§7.1), and that predicate — introduced by Phase 2 (#3184) to retire `state.cts`'s hand-rolled #2828 guard — was strictly more permissive than the guard it replaced, so a flat ROADMAP carrying an ordinary `## Progress` heading had its declared phase count discarded for the on-disk directory count (#3204). Fixed in #3185 by deciding milestone-ness from vocabulary rather than heading position; see §7.1 and the `CONTEXT.md` Roadmap Parser Module entry for the three position-based models that failed and why.
**The lesson this phase adds to Amendment 3's standing rule:** a copy count derived from *which symbol names appear in a prior amendment* is as unreliable as one derived from the reported issues. Read the code, not the write-up.
**Note on #3204.** Routing `buildStateFrontmatter` through the owner will not by itself fix #3204: its defect is the discriminator one layer *above* enumeration — "is the ROADMAP's phase count safe to trust" — which misclassifies ordinary `## Overview` / `## Progress` headings as milestone sectioning. That discriminator **is** §7.1's `isMilestoneBoundedInRoadmap`. The enumeration routing and the discriminator replacement must ship together or the defect survives the consolidation.
@@ -372,7 +376,7 @@ One row per derivation. A blank owner is a derivation whose contract is locked (
|---|---|---|---|---|
| Milestone windowing (§7.1) | `roadmap-parser.cts` | `lint-milestone-window-drift.cjs` | `src/` | enforced |
| Milestone identity (§7.2) | `roadmap-parser.cts` (Phase 6) | same guard, token set widened by Phase 6 | `src/` | enforced |
| Phase enumeration (§7.3) | `phase-locator.cts` | `lint-phase-enumeration-drift.cjs` | `src/` | enforced for the four named consumers; `buildStateFrontmatter`/`syncStateFrontmatter` **unowned** |
| Phase enumeration (§7.3) | `phase-locator.cts` | `lint-phase-enumeration-drift.cjs` | `src/` | enforced — the state writers included (#3185) |
| Phase completion (§7.4) | `verification.cts` (Phase 4) | Phase 4 | `src/` | blocked on #2957 |
| Live-plan counting (§7.5) | `plan-scan.cts` | `lint-plan-count-drift.cjs` | `src/` | enforced |
| Live-plan counting, prompt layer (§7.5) | — (Phase 8) | `lint-planning-prompt-drift.cjs` | `gsd-core/workflows`, `commands`, `agents`, `skills` | ratcheted, 7 sites |
@@ -632,7 +636,7 @@ scoped 4 — so it is not restated here.
| # | Change | Why the existing phases do not cover it |
|---|---|---|
| 1 | ~~**Phase 3 widens** to include `buildStateFrontmatter` and `syncStateFrontmatter`~~ — **superseded: Phase 3 merged without them.** The fifth enumeration copy and #3204 are now unowned and need a phase of their own | Phase 3's Done-when named only `cmdProgressRender`, `cmdStats` and `cmdPhasesList`, and #3222 shipped exactly that. `buildStateFrontmatter`/`syncStateFrontmatter` appear nowhere in Amendment 3, and #3204 appears nowhere in this ADR outside this row. Routing alone would not have fixed #3204 anyway — its defect is the "is the ROADMAP count trustworthy" discriminator one layer *above* enumeration, which is §7.1's `isMilestoneBoundedInRoadmap` |
| 1 | ~~**Phase 3 widens** to include `buildStateFrontmatter` and `syncStateFrontmatter`~~ — **twice superseded; closed by #3185.** First reading ("Phase 3 merged without them") was itself wrong: #3222 *had* routed the enumeration, and the audit mistook Amendment 3's silence for absent work. The real gap was the trust discriminator one layer above — see §7.3 | Routing alone would never have fixed #3204: a correctly scoped enumeration still returns the directory count. The defect was `hasMilestoneSectioning`, introduced by Phase 2 (#3184) and more permissive than the #2828 guard it retired |
| 2 | **Phase 4 blocks on #2957** and its guard must fail while a third predicate exists | The audit found `buildStateFrontmatter` computing completed phases from plan scanning alone, ignoring the ROADMAP checkbox `cmdRoadmapAnalyze` honors. Checkbox-override vs disk-strict is an undecided product question, not a consolidation |
| 3 | **Phase 6 — milestone identity** (§7.2): bind `getMilestoneInfo` to `locateMilestoneHeadings`, widen the windowing guard's token set in the same change (#3171, #3197) | A sixth derivation family. Phase 2 consolidated *windowing*; `getMilestoneInfo` hand-rolls its own heading regexes to answer a different question — *which milestone is this and what is it called* — and no named phase touches it |
| 4 | **Phase 7 — completion-ratio scoping** (§7.6 rules 3–4). **Corrected: #3161 is NOT fixed here — Amendment 3 subsumed it.** Phase 7 keeps rule 4 and whatever consumers Phase 3 did not reach | A seventh derivation family. This amendment ships the arithmetic half. Its original claim — that enumeration consolidation "changes nothing here" — was **wrong**, and Phase 3 proved it: routing `cmdStats`/`cmdProgressRender`'s `totalPlans`/`totalSummaries` accumulation through `listMilestonePhaseDirs`'s scoped set *is* §7.6 rule 3 for those two consumers, and it is what closed #3161 |