* wip(#3217): rule-4 scope withholding — parked, two open findings Implemented but NOT shippable. An isolated review found buildStateFrontmatter still hardcodes SCOPE.COMPLETE, so state json reports percent 0 where roadmap analyze, stats and query progress all correctly report null on the same disk state - rule 4 reintroduced at a site this phase claims to close. Also: roadmap analyze emits scope complete beside progress_percent null with nothing explaining it. Parked to build Phase 4 (#3186) first, which is unblocked. Findings recorded in .gsd/phase/refactor-3217-completion-ratio-scoping/60-review.json. * fix(#3217): withhold the sync percentage on a non-complete scope The parked blocker is fixed - buildStateFrontmatter no longer hardcodes SCOPE.COMPLETE, and the prose Progress fallback is gated too, which was a second leak found while tracing the first. roadmap analyze exposes progress_scope so a consumer can tell WHY a percentage is absent from the JSON alone. Then a residual gap was reproduced rather than assumed. cmdStateSync carried the same hardcode behind a written reason claiming it did not reproduce. It did: on a TRUNCATED window and on UNSCOPED row 4, state sync wrote Progress 0 percent to 100 percent while state json, roadmap analyze, stats and query progress all withheld - and it persisted a self-contradictory file, body claiming 100 percent while its own frontmatter correctly omitted percent. The excuse was also wrong. syncRoadmapRaw is already parsed in that function and is exactly what produces a real scope, so there was a scope to pass. Threaded through listMilestonePhaseDirs; a non-complete scope now skips the write with a reason in changes. milestoneBounded stays as the orthogonal 1761 guard for row 5. Second time this epic a does-not-reproduce claim was too generous. Recorded in ADR Amendment 8 as a correction rather than a quiet rewrite. Verified on the remote runner. * test(#3217): give the withholding fixtures a resolvable scope 40 matrix failures, all fixture drift - no code regression. My own hypothesis that this was over-withholding was wrong and is recorded as such: the worry case, a plain ROADMAP with Phase entries and no version heading, resolves to complete exactly as ADR 7.1 says it should. The real causes were two fixture shapes. Most had no ROADMAP.md at all, which is unreadable via a pre-existing graceful path, and asserted a numeric percent. The five vscode, pi-extension, mcp-server and shell-projection failures were that shape - bare temp dirs using progress json as a reachability proxy while asserting typeof percent is number, which under rule 4 is now null. The rest had a version token in a title or heading with no STATE.md milestone pointer to resolve it, which is classification row 4, versioned but unresolved, so withholding is correct per the contract. Verified on the remote runner. * test(#3217): make the LM-tools reachability tests dispatch against their fixture The gsd_progress reachability test was never testing its fixture. invoke() resolves cwd from vscode.workspace.workspaceFolders by design (the real LanguageModelToolInvocationOptions has no cwd field, per the 2103 fix in extension.js), the mock had no workspace at all, and the test passed a cwd option nothing reads - so it dispatched against the repo working directory. Writing a ROADMAP into the temp dir had no effect. Rule 4 only made it visible. Fixed by mocking workspaceFolders. The two siblings in the same file carried the identical dead cwd and were dispatching against the repo too; they were not failing only because their assertions did not touch scope-dependent output. Both now use their own fixture with assertions unchanged - the no-planning fallback paths already satisfy them honestly. Re-scanned the other five reachability files: no further instances. They thread cwd into parameters that genuinely read it, not through an options shape that ignores it. Verified on the remote runner. * chore(#3217): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3318 exists. * ci(#3217): give the coverage merge enough heap for the merged shards The coverage gate OOMed at exit 134. c8 report merges three shard artifacts, roughly 358MB of V8 dumps in coverage/tmp, and died holding their per-file position maps at the ~4GB default heap. Verified as this branch's delta rather than pre-existing: the same job succeeded on next at 14:18, after phases 4 and 5 merged. Both coverage-gate steps get the bump because both re-slice the same merged data. 8192 doubles what failed and leaves headroom on a 16GB ubuntu runner, matching the idiom the shard step already uses at 6144. This is a memory bound, not a change to what is measured. No threshold was touched. The test file was checked for gratuitous subprocess spawning and is already reasonable at 43 spawns, each a distinct fixture-by-surface pairing. Verified on the remote runner. --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -230,6 +230,37 @@ shape) reports `complete`: the whole document *is* the milestone there. Note
|
||||
this answer is specific to *windowing* — see the next section for why milestone
|
||||
*identity* answers the same document differently.
|
||||
|
||||
### A non-`COMPLETE` scope withholds the percentage entirely (#3217)
|
||||
|
||||
`roadmap analyze --json`'s `progress_percent`, `stats --raw`'s `percent` /
|
||||
`plan_percent`, `query progress --raw`'s `percent`, and `state json --raw`'s
|
||||
`progress.percent` are now **nullable** — a Tier-2 contract change. When the
|
||||
phase set a percentage would be computed from is not fully trustworthy (any
|
||||
scope other than `complete`), these surfaces render **no percentage at all**
|
||||
rather than a number computed from a truncated, unscoped, or unreadable set:
|
||||
|
||||
| Surface | Non-`complete` behavior |
|
||||
|---|---|
|
||||
| `roadmap analyze --json` | `progress_percent: null` |
|
||||
| `stats --raw` | `percent: null`, `plan_percent: null` |
|
||||
| `query progress --raw` | `percent: null` |
|
||||
| `state json --raw` | `progress.percent` is **omitted** from the `progress` object (not `0`, not present as `null`) |
|
||||
| `state update-progress --raw` | `false` — no write; `STATE.md`'s Progress field is left untouched, and a `[gsd-tools] WARNING:` line is written to stderr naming the scope |
|
||||
|
||||
`0` is a legitimate, real answer under a `complete` scope (e.g. a
|
||||
freshly-declared milestone with zero phases, or a phase with zero plan files)
|
||||
and is never withheld — only a non-`complete` scope withholds.
|
||||
|
||||
`roadmap analyze --json` gates `total_plans` / `total_summaries` / `phases` /
|
||||
`completed_phases` on the top-level `scope` field described above (heading
|
||||
windowing identity), but `progress_percent` is governed by a **separate**
|
||||
`progress_scope` field — the scope of the phase-directory set the percentage
|
||||
was actually computed from. The two can legitimately disagree (e.g.
|
||||
`scope: "complete"` — the ROADMAP heading resolves fine — alongside
|
||||
`progress_scope: "unreadable"` when `.planning/phases` itself cannot be read),
|
||||
so a consumer must branch on `progress_scope`, not `scope`, to know why
|
||||
`progress_percent` is `null`.
|
||||
|
||||
### Milestone identity (which milestone, and what it is called)
|
||||
|
||||
Milestone identity — the version and name behind `STATE.md`'s `milestone:`
|
||||
|
||||
@@ -674,6 +674,8 @@ Show status, next steps, and automatically advance to the next logical workflow
|
||||
|
||||
Status reporting is scoped to the current milestone's `ROADMAP.md` window and sentinel-filtered: `999.*` backlog directories and `0-*` pre-milestone directories are not counted as current-milestone phases, so the reported progress percentage no longer holds at `100` while phases in the active window are still outstanding.
|
||||
|
||||
> **Nullable percentage.** The reported completion percentage is `null` — never a fabricated `0`, `100`, or stale value — when the current milestone's phase set is not fully readable/scoped. See [CLI-TOOLS.md → A non-COMPLETE scope withholds the percentage entirely](CLI-TOOLS.md#a-non-complete-scope-withholds-the-percentage-entirely-3217).
|
||||
|
||||
```bash
|
||||
/gsd-progress # "Where am I? What's next?" with auto-routing
|
||||
/gsd-progress --next # Advance to next step automatically
|
||||
@@ -960,6 +962,8 @@ Display project statistics.
|
||||
|
||||
Scoped to the current milestone's `ROADMAP.md` window and sentinel-filtered: `999.*` backlog directories and `0-*` pre-milestone directories are not counted as current-milestone phases.
|
||||
|
||||
> **Nullable percentage.** The reported completion percentage is `null` — never a fabricated `0`, `100`, or stale value — when the current milestone's phase set is not fully readable/scoped (e.g. a truncated or unresolvable milestone window, or an unreadable `.planning/phases` directory). See [CLI-TOOLS.md → A non-COMPLETE scope withholds the percentage entirely](CLI-TOOLS.md#a-non-complete-scope-withholds-the-percentage-entirely-3217).
|
||||
|
||||
### `/gsd-profile-user`
|
||||
|
||||
Generate a developer behavioral profile from Claude Code session analysis across 8 dimensions (communication style, decision patterns, debugging approach, UX preferences, vendor choices, frustration triggers, learning style, explanation depth). Produces artifacts that personalize Claude's responses.
|
||||
|
||||
@@ -1133,3 +1133,184 @@ the same discipline 8.5 imposes on the rules themselves.
|
||||
**No recorded decision governed this seam.** `recall_decision` returns nothing for the health
|
||||
diagnostic surface — the condition that made Phase 0 necessary, and the reason Decision 8 is locked
|
||||
before Phase 10 rather than settled inside it.
|
||||
|
||||
### Amendment 7 — Phase 7 (#3217) validation: the contract held, on the second pass
|
||||
|
||||
**§7.6 rule 4 is now enforced.** A percentage is withheld (never rendered as a fabricated `0`) at
|
||||
every consumer this phase reached: `roadmap analyze --json`'s `progress_percent`, `stats --raw`'s
|
||||
`percent`/`plan_percent`, `query progress --raw`'s `percent`, `state json --raw`'s
|
||||
`progress.percent`, and `state update-progress --raw`'s write. Rule 2's negative space — a **real**
|
||||
`0` under a `COMPLETE` scope — is unaffected and still renders (matrix rows B1–B6).
|
||||
|
||||
**This phase was parked once and resumed after an isolated adversarial review, and the review
|
||||
caught something a same-authorship pass would not have.** The first pass shipped rule 4 at
|
||||
`computeProgressPercent` (§7.6's own "cheapest site" precedent) and at `cmdRoadmapAnalyze`'s new
|
||||
`progressScope`-gated accumulation, but left `state.cts::buildStateFrontmatter` hardcoding
|
||||
`SCOPE.COMPLETE` behind a written-reason comment ("threading it through would mean restructuring
|
||||
the `_diskScanCache` shared shape, which is out of this phase's named sites"). The review rejected
|
||||
the comment as a gate — "the whole finding is that a comment is not a gate" — and the restructuring
|
||||
was done: `_diskScanCache`'s cached shape gained a `phaseDirScope: Scope` field carrying
|
||||
`listMilestonePhaseDirs`'s real scope, threaded to the `computeProgressPercent` call site in place
|
||||
of the hardcode. **A second defect surfaced only by tracing the fix through**, not named in the
|
||||
original finding: the same function's prose fallback (`progressRaw.match(/(\d+)%/)`, reading a
|
||||
`Progress: N%` line already written to STATE.md's body) fires whenever the scoped call returns
|
||||
`null`, so a non-`COMPLETE` scope would still have surfaced a stale prose percentage through that
|
||||
second path even after the hardcode was fixed. That fallback now also gates on
|
||||
`diskScope === SCOPE.COMPLETE`, matching the pre-existing `milestoneUnbounded` guard already beside
|
||||
it (#1761). **Generalized lesson, in the same family as Amendment 5's "any numeric window in a drift
|
||||
guard is a Goodhart target": a rule-4 fix at one output expression is not enough when a fallback
|
||||
expression sits beside it and shares the same "no data" `null` sentinel — every path that can
|
||||
produce the observable output must be re-checked, not just the primary one.**
|
||||
|
||||
**A second, independent scope was found ungoverned at `cmdRoadmapAnalyze` and is now exposed, not
|
||||
reconciled.** `roadmap analyze --json` computes `progress_percent` from its own
|
||||
`listMilestonePhaseDirs` call (`progressScope`) — correctly, per rule 3 — while `total_plans` /
|
||||
`total_summaries` / `phases` / `completed_phases` stay derived from the heading-matched
|
||||
`_phaseDirNames` scan that the top-level `scope` field (windowing identity) describes. Two `Scope`
|
||||
values governed one JSON object and only one was named. Of the three options the review posed
|
||||
(expose the second scope; reconcile the two into one; re-derive the phases/counts fields from the
|
||||
same scoped set as the percentage), this phase **exposes** it as a new `progress_scope` field rather
|
||||
than reconciling or re-deriving. Reconciling or re-deriving would have undone the phase's own
|
||||
already-recorded, deliberate choice a few lines above it in the same function — "this does not touch
|
||||
`total_plans` / `total_summaries` / `phases` / `completed_phases` … only `progress_percent`'s own
|
||||
inputs move onto the scoped owner" — which exists because `_phaseDirNames` is a heading→directory
|
||||
**lookup index**, not a milestone enumeration, and filtering it through `listMilestonePhaseDirs`
|
||||
would scope the same set twice (the same shape Decision 4a's exemption already covers for that
|
||||
variable). Exposing the field that governs the null is the minimal change that satisfies the
|
||||
invariant the review named: *a consumer must be able to tell WHY a percentage is absent from the
|
||||
JSON alone.*
|
||||
|
||||
**A third, minor finding — `state update-progress`'s silent no-op — is resolved as accept-with-
|
||||
disclosure, not silence.** The command already left `STATE.md` untouched on a non-`COMPLETE` scope;
|
||||
the gap was that the only signal was a JSON `reason` field most callers do not read. It now also
|
||||
writes a `[gsd-tools] WARNING:` line to stderr, matching the convention this same file already uses
|
||||
for a comparable silent field-update no-op (`stateReplaceFieldWithFallback`, `state.cts:553`) rather
|
||||
than inventing a second warning shape.
|
||||
|
||||
**The guard question was re-litigated, not skipped.** Issue #3217 asked for
|
||||
`lint-completion-ratio-drift.cjs` to be extended to "catch a percentage rendered from counts whose
|
||||
scope was not consulted." A narrow, syntactic candidate — a call to `listMilestonePhaseDirs(...)`
|
||||
whose `.value` is read while `.scope` is never bound in the same function — was prototyped against
|
||||
the real tree and produced real false positives: `src/milestone.cts:697,753,881` legitimately read
|
||||
only `.value` from that call for milestone **archiving**, unrelated to percentage rendering at all.
|
||||
"Was this scope consulted before rendering a percentage" is a data-flow question, not a syntactic
|
||||
one, and no syntactic proxy distinguishes those two call shapes. Per Amendment 4a's standing rule —
|
||||
build and run before scope is fixed, state what the guard actually found — the candidate was dropped
|
||||
rather than shipped with an exemption list that would have hollowed it out on the sites it exists to
|
||||
catch. **The existing arithmetic guard (rules 1–2) is unchanged and still reports an earned `0`** on
|
||||
the real tree (test row E3); the real enforcement for rule 4 is the behavioral identity-at-the-
|
||||
surface suite this phase adds (`tests/completion-ratio-scope-withholding.test.cjs`, matrix rows
|
||||
A1–A11, B1–B6, C1–C3, D1–D4, E1–E4), per Decision 4(b)/(c) — asserted at each consumer's observable
|
||||
output, never at a helper's return value, so a percentage post-filtered after the fact cannot pass
|
||||
as one never withheld.
|
||||
|
||||
**What this phase does NOT close, left with a written reason rather than silently dropped:**
|
||||
|
||||
- **Workstream inventory (matrix row A8).** `buildWorkstreamInventory`
|
||||
(`src/workstream-inventory-builder.cts`) carries a pre-ADR-3180 bespoke boolean
|
||||
(`milestoneScoped`), not the frozen `SCOPE` enum, and cannot distinguish `TRUNCATED` from
|
||||
`UNSCOPED` from `UNREADABLE`. Migrating it honestly requires widening
|
||||
`WorkstreamInventory.progress_percent` from `number` to `number | null` — a return-type
|
||||
re-architecture the design doc names as out of scope for this phase — or reusing the bespoke
|
||||
boolean as a `Scope` stand-in, which is the same textual-proxy-for-a-data-flow-property this
|
||||
amendment's guard section rejects one paragraph up. Left un-migrated with the reason recorded at
|
||||
the field's own definition site.
|
||||
- **`cmdStateSync`'s own `SCOPE.COMPLETE` hardcode (`state.cts`, `cmdStateSync`), found but not
|
||||
named in the original review.** This is a *third* site sharing buildStateFrontmatter's pattern —
|
||||
a raw `fs.readdirSync(phasesDir, ...)` listing, never routed through `listMilestonePhaseDirs`,
|
||||
carries its own written-reason comment for staying on `SCOPE.COMPLETE`. It was not named as a
|
||||
finding and the design's consumer map does not list `cmdStateSync` among the sites this phase
|
||||
owns. It is lower-risk than the fixed sites: an unreadable `phasesDir` (this finding's own
|
||||
reproduction shape) already hits `cmdStateSync`'s own `fs.readdirSync` `catch` block, which exits
|
||||
via the pre-existing `{ synced: true, changes: [], dry_run }` early return **before** any percent
|
||||
is computed — so the specific defect this phase closes elsewhere does not reproduce here. Recorded
|
||||
rather than silently left for a reader to independently rediscover; migrating it fully (adding rule
|
||||
3's window-scoping, not just rule 4's withholding) is out of this phase's named scope.
|
||||
|
||||
**Rebase note.** This phase's implementation predates Phase 4 (#3186, the disk-strict completion
|
||||
predicate) landing on `next`. Rebasing onto `next` after Phase 4 merged produced **no conflicts**,
|
||||
though Phase 4 turned out to touch two of the same functions this phase's fix reaches —
|
||||
`buildStateFrontmatter` and `cmdStateSync` — swapping each one's `diskCompletedPhases` count from
|
||||
`scanPhasePlans(...).completed` (answers "are all plans summarized", a different question, per
|
||||
§7.4/#2957) to the canonical `isPhaseComplete(...).value.complete`. Those hunks land in the
|
||||
phase-directory completion-counting loop in each function; this phase's own hunks land at the
|
||||
`listMilestonePhaseDirs` destructure, the `_diskScanCache` shape, and the `computeProgressPercent`
|
||||
call site further down the same functions — non-overlapping line ranges, which is why the automatic
|
||||
merge succeeded with no manual resolution.
|
||||
|
||||
**Tier-2, re-derived for Phase 7 (mirrors Amendment 5's per-phase table convention):**
|
||||
|
||||
| Command surface | Output change |
|
||||
|---|---|
|
||||
| `roadmap analyze --json` | `progress_percent` becomes `number \| null`; gains a new `progress_scope` field naming the scope that governs it, separate from the top-level `scope` |
|
||||
| `stats --raw` | `percent` and `plan_percent` become `number \| null` |
|
||||
| `query progress --raw` | `percent` becomes `number \| null` |
|
||||
| `state json --raw` | `progress.percent` is omitted (not `0`, not `null`) rather than rendered when the underlying scope is not `COMPLETE` |
|
||||
| `state update-progress --raw` | unchanged wire shape (`updated: false` was already the non-write signal); gains a `[gsd-tools] WARNING:` stderr line on skip |
|
||||
|
||||
**Guard roster (§7.6 row), as amended.** The original body's row is not edited in place, per
|
||||
Amendment 6's own precedent; read with this correction: `lint-completion-ratio-drift.cjs`'s scan
|
||||
surface and mechanism are **unchanged** (arithmetic rules 1–2 only, `src/` scan surface) — rule 4 is
|
||||
enforced by the behavioral suite named above, not by an extension to this guard. §7.6's status line
|
||||
("rule 4 Required — Phase 7") and the Status bullet "Rule 4 — Phase 7 (#3217)… is not implemented
|
||||
anywhere" are superseded by this amendment: rule 4 is enforced at every site Phase 7 named, with the
|
||||
two written exceptions above.
|
||||
|
||||
### Amendment 8 — a BLOCKER correction to Amendment 7's `cmdStateSync` claim: TRUNCATED and UNSCOPED row 4 DID reproduce
|
||||
|
||||
**Amendment 7's second "what this phase does NOT close" bullet is wrong and is corrected here, not
|
||||
silently rewritten** — per Amendment 6's own append-only precedent, that bullet's body is left
|
||||
untouched above; this amendment records what independent, empirical reproduction on fresh fixture
|
||||
copies actually found.
|
||||
|
||||
Amendment 7 claimed `cmdStateSync`'s hardcoded `SCOPE.COMPLETE` was "lower-risk than the fixed
|
||||
sites" because "an unreadable `phasesDir` (this finding's own reproduction shape) already hits
|
||||
`cmdStateSync`'s own `fs.readdirSync` `catch` block… so the specific defect this phase closes
|
||||
elsewhere does not reproduce here." That checked only the `UNREADABLE` row. It did not check
|
||||
`TRUNCATED` or `UNSCOPED`, and on those rows the claim is false: `cmdStateSync`'s disk scan
|
||||
(`entries`, `state.cts` ~3113) enumerates `phasesDir` successfully — it is readable — but the scan is
|
||||
**unfiltered by the real milestone window** (it only excludes retired phase numbers, #1514), and the
|
||||
percent gate hardcoded `SCOPE.COMPLETE` regardless of what the real window's scope was. Reproduced on
|
||||
fresh, independent fixture copies (`listMilestonePhaseDirs` called directly to confirm the real
|
||||
scope, matching each surface's own derivation):
|
||||
|
||||
| fixture | real scope | `state sync` (pre-fix) |
|
||||
|---|---|---|
|
||||
| TRUNCATED (milestone heading found, window empty, document has phases elsewhere — row 8) | `truncated` | **wrote a fabricated `0%` → `100%`** |
|
||||
| UNSCOPED row 4 (no milestone asserted, ROADMAP has versioned headings) | `unscoped` | **wrote a fabricated `0%` → `100%`** |
|
||||
| UNSCOPED row 5 (asserted version, no matching heading) | `unscoped` | skipped — but only because the orthogonal `milestoneBounded` (#1761) guard happens to intercept this specific row first, not because `cmdStateSync` itself was scope-aware |
|
||||
|
||||
Worse than a wrong read: this is a **write** path, and on the TRUNCATED fixture it persisted a
|
||||
self-contradictory `STATE.md` in one write — the body's `Progress:` line got the fabricated `100%`
|
||||
while the frontmatter's `progress:` block, built in the same `writeStateMd` call by the already-fixed
|
||||
`buildStateFrontmatter`, correctly omitted `percent`. That cross-surface disagreement inside a single
|
||||
file is the exact defect class this epic exists to remove.
|
||||
|
||||
**Fixed the same way as the two sites Amendment 7 named**: `cmdStateSync` now calls
|
||||
`listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: versionStr })` — reusing the same
|
||||
`syncRoadmapRaw`/`syncRoadmapScope` already parsed in the function for the disk-scan totals — and
|
||||
withholds (pushing a `Progress: skipped — …(#3217)` entry to `changes`, mirroring the `#1761`
|
||||
skip-message convention already at that call site) whenever the real scope is not `COMPLETE`. The
|
||||
`milestoneBounded` (#1761) guard is unchanged and still fires first on row 5; a genuine `0` under a
|
||||
real `COMPLETE` scope still writes. Re-reproduced post-fix: all three rows above now agree with
|
||||
`state json` / `roadmap analyze` / `stats` / `query progress` on the same fixture — all withhold
|
||||
together, none renders a number, and the control (`COMPLETE`) fixture still writes its earned
|
||||
percentage.
|
||||
|
||||
**Generalized lesson — the second instance of this pattern in this epic (see Amendment 5's own
|
||||
"Goodhart target" lesson for the first).** A "does not reproduce" claim was too generous a second
|
||||
time: it checked the row named in the ORIGINAL finding (`UNREADABLE`) and stopped, rather than
|
||||
checking every row the same code path could plausibly hit. A written-reason comment recording "I
|
||||
checked X and it's fine" is not evidence about Y and Z unless Y and Z were checked too — the same
|
||||
"a comment is not a gate" principle Amendment 7 itself invoked against `buildStateFrontmatter`'s
|
||||
original hardcode applies equally to a comment that scopes its own non-reproduction claim too
|
||||
narrowly.
|
||||
|
||||
**Tier-2, additional (mirrors Amendment 7's own table):**
|
||||
|
||||
| Command surface | Output change |
|
||||
|---|---|
|
||||
| `state sync --raw` | on a non-`COMPLETE` scope: no `Progress:` body rewrite; `changes` gains a `Progress: skipped — …(#3217)` entry instead. A genuine `0` under `COMPLETE` is unaffected. |
|
||||
|
||||
`.changeset/bold-otters-scope.md` is updated to disclose this write-path change alongside the two
|
||||
Amendment 7 already recorded.
|
||||
|
||||
Reference in New Issue
Block a user