bc5c36261032441aca474488091ed82d591009e4
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
77dbfb961d |
fix(#2645): persist verification verdicts so deletion cannot raise completeness (#3016)
* test(#2645): pin failing-first regression coverage for verification-deletion ledger Deleting a *-VERIFICATION.md file after a failing gaps_found/human_needed verdict was recorded silently raises reported workstream completion — the deletion is indistinguishable from "verifier never ran" at the verdict-lookup layer. These tests pin the correct behavior (deletion must never raise completed_phases/progress_percent) and fail against the current code, which has no such protection. * fix(#2645): persist verification verdicts across deletion in workstream rollup FAILING_VERIFICATION_STATUSES gated phase completeness on a verdict read fresh from *-VERIFICATION.md on every call. Deleting that file made "verifier found gaps, report later deleted" indistinguishable from "verifier never ran" (both read the internal 'missing' sentinel), so removing evidence silently raised completed_phases/progress_percent. Persist the last real verdict observed per phase key in a ledger file at the workstream directory level (outside every phase directory, so the same deletion that triggers the hole cannot also erase the memory of it). The ledger is consulted only when a live read comes back 'missing' and is updated whenever a real verdict is observed, so a genuinely re-verified phase is never permanently pinned, and verifier-disabled or not-yet-verified phases are unaffected (no ledger entry is ever created for them). Ledger-winner selection walks phase directories in the same sorted order buildWorkstreamInventory's own duplicate-directory tie-break uses, so a stale same-numbered directory can never clobber the live directory's remembered verdict on an exact mtime tie. Adds fault-injection coverage for the ledger read/write per CONTRIBUTING.md's filesystem-writes rules. * fix(#2645): redesign verification ledger to fail closed, not open The two-state ledger read (any read/parse failure -> "nothing remembered") failed OPEN: deleting the ledger alongside the report, or simply corrupting it while the report was already gone, degraded to the same 'missing' sentinel this issue exists to stop trusting -- silently reopening the completion-inflation hole one level up. Redesigned as a three-state read distinguishing 'absent' (no ledger file at all -- ENOENT specifically, disambiguated from a broken symlink via lstatSync) from 'corrupt' (file exists but unreadable/unparseable/ wrong shape) from 'ok'. Only 'absent' behaves as pre-fix (ungated) -- deliberate, since every existing project is in that state for every workstream on the day this ships. 'corrupt' and 'ok' both fail closed for a phase with no trustworthy entry, via a new internal sentinel 'unrecorded' added to FAILING_VERIFICATION_STATUSES. A corrupt ledger is not a permanent wedge: it is only overwritten when a real verdict is actually observed (never patched with an empty object), so re-verifying even one phase repairs the file. The ledger write is now atomic (temp file + rename, mirroring broken-windows.cts's writeLedgerAtomic/renameWithRetry shape) so this fix does not itself produce the corrupt files it now treats as security-relevant. Disclosed, accepted residual gap, pinned as an explicit test: deleting the ledger file itself (not just the report) still returns a workstream to the pre-adoption 'absent' state. This is inherent to any design where a wholly-absent store must be safe by default -- the alternative is gating every never-verified phase in every project on upgrade. Rail B is prospective only; a phase deleted before this fix shipped cannot be retroactively recovered. Also: dropped 'stale' from the Row 9 property test's REAL_STATUSES (it does not round-trip through readVerificationStatus as written, so including it claimed coverage the test did not have), converted two try/finally test bodies to t.after(), and added the remaining CONTRIBUTING.md fault-injection cases (broken symlink, missing parent directory, rename failure, temp-file cleanup). * fix(#2645): share the rollup winner selection to close a scoping gap BLOCKER: the ledger-winner selection in workstream-inventory.cts compared raw mtimes with no milestone-scoping filter, while the builder's own rollupDirByKey filters out-of-milestone directories before comparing. In a scoped workstream, a stale out-of-milestone duplicate-key directory with a newer mtime could win the ledger's selection while losing the builder's -- so deleting the LIVE directory's report never consulted the ledger, reopening #2645's hole for the phase that actually counts toward completed_phases, reachable with a plain rm. Extracted pickRollupWinners as the single shared implementation both rollupDirByKey and the ledger's winner selection now call, with the identical scoping filter -- two independent hand-written copies of "pick the winner" is what produced the divergence; one implementation makes the bug class structurally impossible rather than merely tested against. Added a unit-level proof (synthetic same-key entries with opposing inclusion/mtime) and an integration-level proof (a scoped workstream asserting an out-of-milestone verdict is never written into the ledger). Also: disclosed a third residual limitation in the changeset (editing a ledger entry by hand plants a permanent false verdict -- worse than deleting the ledger, since it looks like genuine history; not made tamper-proof, that's scope creep here); fixed a non-ENOENT lstatSync failure falling open to 'absent' instead of failing closed like every other path in that function; and fixed two test bugs a real gsd-test run caught -- Row 11's write-failure mock matched only the final ledger path, but the atomic-write refactor moved the real write target to a temp file, so the mock silently stopped intercepting anything and the test's own assertion caught its own staleness. * test(#2645): relabel Row 19 honestly and pin the lstat fail-closed fix Row 19 used two DIFFERENT phase keys (1-old, 2-new), so it never exercised the same-key collision the milestone-scoping blocker fix addresses -- the pre-fix, unshared ledger-winner code would have satisfied it too. Its docstring called it the integration-level proof of the blocker; it is not. Relabeled both rows accurately: Row 18 (synthetic same-key data) is now stated as the only row that proves the collision end to end, and Row 19 is described for what it genuinely covers -- a distinctly-keyed out-of-milestone phase's verdict never reaching the ledger, real coverage but not the collision case. Extensive probing (documented in 10-diagnosis.md) could not construct a natural directory-naming pair that shares a rollup key while diverging in milestone membership under the current roadmap-parser implementation, so the collision proof stays unit-level by necessity, not convenience. Also added a test pinning the lstat fail-closed fix: readVerificationLedger disambiguates a broken symlink (ENOENT from readFileSync) from genuine absence via a follow-up lstatSync call, and only lstatSync itself reporting ENOENT is proof of absence. A double-fault (readFileSync ENOENT, lstatSync a DIFFERENT code) cannot occur on a real filesystem, so it's monkeypatched directly -- without a test, a future edit could re-widen that catch back to "any lstat failure means absent" and fall open again silently. * chore(#2645): backfill changeset PR number to 3016 --------- Co-authored-by: sim <sim@local> |
||
|
Adnan
|
137f3fbb9c |
fix(#2562): scope workstream progress/status to the current milestone (#2588)
* fix(#2562): scope workstream progress/status to the current milestone `workstream progress` / `workstream status` / `workstream list` share one derivation that could report a workstream's CURRENT milestone as "milestone complete" / 100% while phases in that milestone were unstarted, in progress, or failing verification. Three coupled defects: 1. The shipped signal was project-lifetime, not milestone-scoped: workstreamMilestoneShipped() returned true if ANY *-ROADMAP.md snapshot existed or "SHIPPED" appeared anywhere in ROADMAP.md. Every prior shipped milestone leaves a permanent collapsed <summary>✅ … SHIPPED</summary> block, so any post-v1.0 workstream was pinned to "milestone complete" forever (over-correction from #1913). 2. The denominator dropped declared-but-unscaffolded phases, and completed PRIOR-milestone phase directories inflated the numerator, letting progress_percent round to 100 while real work remained. 3. Phase completeness ignored the VERIFICATION verdict — SUMMARY >= PLAN count alone marked a phase complete even with a human_needed verdict. Fix: derive both numerator and denominator from artifacts scoped to the current milestone. The current version comes from the workstream STATE.md `milestone:` field (ROADMAP in-progress markers can be stale); the ROADMAP `## Progress` table maps every phase — including dirless ones — to its milestone, and the matching set is both the denominator and the directory membership filter. The shipped signal now requires the CURRENT version's archived ROADMAP snapshot (REQUIREMENTS snapshots are not accepted; they can be written at milestone start) or the current milestone's own line marked shipped. Phases with an explicit failing verdict (gaps_found/human_needed) count as in_progress; missing/unknown/stale are left untouched so verifier-disabled projects do not regress to never-complete. Greenfield roadmaps with no versioned Progress table, and projects whose current version cannot be determined, keep the prior behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#2562): add changeset for workstream milestone-scoping fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(#2562): parse the Progress table via findTableWithColumns The ad-hoc pipe-table regex tripped the local/no-adhoc-markdown-parsing ESLint rule. Use the canonical markdown-table helper instead: the milestone-grouped RoadmapProgress variant is located by its required `Phase` + `Milestone` columns and cells are addressed by column NAME, so the parser tolerates column reordering and injected columns. The `flat` variant (no Milestone column) yields no attribution, which is the intended fallback to legacy counting. Behaviour is unchanged: verified against a real multi-workstream project (same status/percent/phase and plan counts before and after). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2562): count table-only phases in the unscoped denominator Addresses the reporter's repro detail: a phase declared as a `## Progress` table row with no `### Phase N` heading is missed by countRoadmapPhases EVEN WHEN other headings exist — the heading regex counts 1 for a "1 heading + 1 table-only" roadmap — not just in the zero-heading fallback path. Milestone scoping did not cover this, because a flat Progress table (no Milestone column) carries no per-phase attribution, so greenfield and single-milestone projects kept the old heading-only denominator and the declared phase silently vanished from it. When milestone scoping cannot engage, the denominator is now the union of the Progress table's declared phase numbers and the phase directories, so neither source can shrink it. Verified against the reporter's minimal fixture (phase 1: 1 PLAN + 1 SUMMARY + gaps_found; phase 2: table row only, no heading, no dir), which now reports 0/2 at 0% across all four table/STATE permutations instead of 1/1 at 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2562): attribute dir-only sub-phases to their parent's milestone A sub-phase directory inserted mid-milestone (e.g. `30.1-…` under a table-declared phase 30) usually has no ROADMAP Progress-table row, so it had no milestone attribution and was scoped out of the rollup entirely — its completed work was invisible and it could never hold the percentage below 100. It now inherits its parent phase's milestone and joins BOTH sides of the calculation. Both sides is the load-bearing part: adding it to the numerator alone would let completed_phases exceed a denominator that never counted it, cap back to 100% via Math.min, and reintroduce exactly the defect this issue reports. A regression test pins that failure mode (all declared phases complete + an in-progress dir-only sub-phase → 75%, not 100%). Attribution is deliberately one-directional: a sub-phase counts only when its PARENT is in the current milestone, so a follow-up created in a later milestone under an older parent is excluded rather than misattributed — conservative (under-count) rather than falsely inflating. Verified on a real project: the reported workstream moves from 2/6 (33%) to 3/7 (43%), the 3/7 being the honest figure — a completed sub-phase that was previously invisible now counts, and so does its plan total. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#2562): describe the denominator + sub-phase fixes in the changeset The fragment was written at the first commit and only covered the three original defects. Bring it up to date with what actually ships: the table-only-phase denominator union (heading-only counting dropped a declared phase even when other headings existed) and sub-phase milestone inheritance across both sides of the calculation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(#2562): promote the canonical phase-key surface to phase-id state.cts kept `phaseKeyFromToken`/`phaseKeyFromDir` private, so every other module that had to compare two independently-derived phase references — a ROADMAP table cell against a phase directory, say — wrote its own regex. That is the defect class #2562 reports: a padded `01` and an unpadded `1-slug` land in different key spaces and the comparison silently yields nothing. Move the pair to the phase-id owner module and add `phaseKeyFromProse` (for ROADMAP/STATE prose, markdown emphasis stripped) and `parentPhaseKey` (a sub-phase's parent). state.cts imports them; its call sites are unchanged. * fix(#2562): own milestone-shipped detection and accept a workstream scope Three changes to the module that owns milestone parsing, so its consumers stop reimplementing it: - `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `<summary>` lines only. A bullet that merely names the version (`- [x] 03-01: ship the v2.0 login endpoint`) is prose about a phase, not a milestone verdict. The version token is boundary-matched with `(?![\w.-])` — `\b` does not bound it, since `.` is a non-word character, so a shipped `v2.0.1` heading would otherwise close `v2.0`. - `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name and thread it to `planningDir(cwd, ws)`. A caller iterating workstreams cannot set `GSD_WORKSTREAM` per iteration, which is what the existing resolution falls back to. Omitted, resolution is unchanged. - `getMilestonePhaseFilter` exposes `versionScoped`, true only when the phase set really is one milestone's. On an unversioned roadmap `phaseCount` spans the project's lifetime and must not be read as a current-milestone denominator. The closed/active milestone-marker patterns were kept in three byte-identical copies; they are hoisted to module scope as one `isClosedMilestoneHeading`. * fix(#2562): derive membership and denominator from one phase-key space The milestone scoping added earlier in this PR derived the ROADMAP table key and the phase-directory key with two different regexes, and dropped rows it could not attribute. Each of those was another way to reproduce the symptom this issue reports — a rollup contradicting its own `phases[]` listing: - a padded `| 01. … |` row never matched a `1-slug` directory (and a bespoke `^0*(\d+…)` never matched `PROJ-05-…` at all), so phases fell out of the milestone entirely and the percentage collapsed or pinned; - a blank or malformed Milestone cell deleted the phase from BOTH sides, letting an unstarted phase vanish and the remainder round to 100%; - shipped detection scanned bullets, so any checkmarked line naming the version closed the milestone; - the numerator counted per-directory while the denominator counted distinct phases, so a stale same-numbered directory (Bug #2445's scenario) pushed `completed_phases` past the denominator, where `Math.min` capped it to 100% and hid the unstarted phase. Both sides now key off the phase-id owner module (`phaseKeyFromDir` / `phaseKeyFromProse`), directory membership additionally consults `getMilestonePhaseFilter` when that filter is genuinely version-scoped, and the denominator is the union of the roadmap's declarations with the member directories' keys — so `completed_phases <= denominator` holds by construction. The Builder asserts it and throws; the `Math.min` cap survives only on the legacy unscoped path, where the denominator is a heading count that cannot bound the numerator. An unattributable row degrades over-inclusively (kept, never dropped), matching the degrade direction roadmap-parser already commits to. * test(#2562): boundary coverage for each milestone-scoping reproduction One test per way the scoping could still report "milestone complete"/100% while phases are incomplete: zero-padded rows vs padded dirs (and the mirror), project-code-prefixed dirs, a blank/malformed Milestone cell, a checkmarked bullet naming the version, a shipped `v2.0.1` heading against a current `v2.0`, and a stale same-numbered directory. Plus the current milestone's own shipped heading (the signal must survive the boundary fix), the Builder's numerator-above-denominator throw, a parity check that every non-`passed` verifier status blocks completeness, and a guard that scoping reads the workstream's ROADMAP rather than the project root's. Reverting only `src/` reddens six of them. * docs(#2562): record the milestone-scoped semantics and its consumer impact CONTEXT.md: the Workstream Inventory Module's completion fields now describe the current milestone, not the workstream's lifetime; phase-id owns the canonical phase-key surface; roadmap-parser owns milestone shipped/active classification and takes an optional workstream scope. Changeset: name the behaviour change explicitly — `roadmap_phase_count`, `completed_phases` and `progress_percent` change meaning with no schema signal, and `getOtherActiveWorkstreamInventories` filters on the derived status, so consumers see real movement. * fix(#2562): collapse every zero-padding spelling to one phase key A property test over the key surface — table cell and directory decorated INDEPENDENTLY, which is the point — found a divergence neither review named: `padStart(2, '0')` is a no-op once the input is already ≥2 characters, so `5` normalised to `05` while `005` stayed `005`. A `| 5. … |` row and a `005-slug` directory therefore never compared equal, which is the same failure mode as the padded-vs-unpadded blocker, one level down. The strip belongs in `phaseKeyFromToken`, not in `normalizePhaseName`: applying it to the latter regressed multi-decimal leading-zero plan IDs (`001.10-PLAN.md` capture + wave assignment), which rely on its verbatim rendering. Confining it to the key surface fixes the comparison and leaves rendering untouched. Also tightens `isMilestoneShippedInRoadmap`'s patterns to anchored, complementary character classes so an untrusted ROADMAP cannot drive backtracking, and makes the project-code test discriminating — it previously passed pre-fix, because an unresolvable key collapsed scoping to the whole roadmap and happened to land on the same number. It now carries a prior-milestone directory that a collapse would wrongly admit. * fix(#2562): prefer the milestone-attributing Progress table; pin the seams Three gaps the earlier self-check missed: - Both RoadmapProgress variants carry a `Plans Complete` column, so probing it first picked a FLAT table appearing earlier in the document over the milestone-grouped one that actually carries the attribution. Every row came back unattributed, was treated as current-milestone, and silently re-admitted prior-milestone phases. The attributing shape is probed first; flipping the order reddens the new test. - `lint-phase-id-drift` exempts phase-id.cts by design, so it is silent on `phaseKeyFromToken`'s own segment strip by construction — not evidence. Its interaction with `stripProjectCodePrefix` (which runs AFTER) is pinned across project codes and hyphenated ids, including the pre-existing `M1-46-6` vs `M1-46-6-rs` asymmetry, which is `extractPhaseToken`'s #2043/#2232 slug-word rule and not something to "fix" by accident. - `listWorkstreamInventories` loops every workstream with no try/catch, so a REACHABLE Builder-invariant throw would take down `workstream list`/`status`/ `progress` for all of them. The invariant test only exercised the pure Builder with hand-built inputs. A test now drives `inspectWorkstream` over every adversarial shape at once (prior-milestone dirs, three colliding duplicates, a dirless declaration, an unattributed row, a project-code prefix, a dir-only sub-phase) and asserts it does not throw and the invariant holds — so the throw stays a contract assertion for external callers, not a runtime path. Also covers the active-marker-wins rule (`## v2.0 — 🚧 IN PROGRESS … ✅` must not mark shipped), which nothing exercised. * fix(#2562): scope a declared-but-empty current milestone instead of falling back to history The review's open MAJOR. `STATE.md`'s `milestone:` field updates the moment `/gsd-new-milestone` writes the heading, while the `## Progress` table and phase sections land later. In that window nothing attributes a phase to the current milestone, `scoped` went false, and the fallback counted the project's ENTIRE phase history as both numerator and denominator — a milestone with zero work done reported 100% off its predecessors'. That is #2562's own symptom reached by a different precondition, and none of the 16 tests covered it. Reproduced first, four ROADMAP shapes, at `inspectWorkstream` rather than the Builder — the Builder takes the scoping decision as an input, so a Builder-level test proves it honours a flag, not that the derivation sets it. Three of the four reported 2/2 100% with no phase of the current milestone begun. Which signal witnesses the state depends on the ROADMAP's shape, and no single one covers all three: - `## v3.0` exists but declares no phases. `getMilestonePhaseFilter` DOES locate the section and sets `versionScoped`, then the zero-phase pass-all degrade resets it to false — erasing the only evidence the milestone exists. Neither existing flag survives that path, so this adds `versionSectionFound`, set beside `versionScoped` and deliberately preserved through the degrade. - No section for this version at all, in a roadmap that versions its others — the existing `missingExplicitVersion`, already exposed and tested. - Unversioned headings, but a Progress table attributing every row elsewhere: neither filter flag fires and the table is the only witness. A ROADMAP that attributes NO versions anywhere matches none of them, which is the point. Its rows parse with `version: null`, land in `currentMilestoneKeys`, and never reach the new branch. `readCurrentMilestoneVersion` returns a non-null version for very nearly every project (`getMilestoneInfo` defaults to `v1.0`), so keying off `currentVersion` alone would have zeroed out every free-form legacy project — the condition looks fussy for that reason. A test pins it. Within an empty milestone, membership inverts: a directory belongs unless another milestone's row claims it. Excluding everything would have dropped a phase scaffolded before the roadmap caught up from BOTH sides of the rollup, and hiding real work is the same class of defect as inventing it — this codebase degrades over-inclusive, never under. Scoping is now stated by the caller (`milestoneScoped`) rather than inferred from `currentMilestonePhaseCount > 0`. That inference was the root cause: it cannot represent a milestone that is scoped AND legitimately zero-phase, so the Builder read "no phases yet" as "no scoping" and reopened the whole-history path. The count-derived value stays the default for callers that say nothing. A regression test also pins that a zero denominator does not trip the Builder's `completed_phases <= denominator` throw, since `listWorkstreamInventories` has no try/catch and a crash on every freshly-declared milestone would be worse than a wrong percentage. The changeset and CONTEXT.md no longer claim membership is derived in "ONE" / "a SINGLE" phase-key space. `getMilestonePhaseFilter` still runs its own `normalizePhaseIdSegments`; the signals are OR'd so a divergence can only widen membership, but two normalisers coexist and the docs now say so. * fix(#2562): cross-validate the shipped marker against the milestone's artifacts `status: "milestone complete"` was asserted from the shipped marker alone, so a single payload could report it beside `progress_percent: 67` — this issue's own symptom, reached through `status` rather than the percentage. The marker is now a claim checked against the milestone's own artifacts, and the two signals are checked at DIFFERENT strengths because one check cannot serve both. A `heading` marker (operator-typed, live ROADMAP) is refused on a short completion ratio, which also catches phases declared but never scaffolded. A `snapshot` marker is NOT ratio-gated: `milestone complete` moves the milestone's phase dirs into `milestones/<version>-phases/` (milestone.cts:755-762) while copying — never truncating — the live ROADMAP (:671-674), so a correctly archived milestone reads 0/N by construction and a ratio gate would strip `milestone complete` from every archived milestone. It is refused instead when an in-milestone phase dir is still live and unfinished, reachable because `milestone complete` does not advance STATE's `milestone:` field (state-transition.cts:1335 vs :1224). `legacy` stays ungated — only reachable when scoping is off. A refused marker does not fall through to a STATE field claiming the same thing; against contradicting artifacts neither source may report completion. The refusal surfaces as `milestone_shipped_unverified` rather than staying silent, distinct from `status_conflict` (derived-vs-field only). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * test(#2562): pin both marker strengths, the archived guard, and the owner modules workstream-inventory: four tests, all four red against the prior src and green with it. A live-ROADMAP SHIPPED heading over an incomplete milestone is refused; an archived snapshot SURVIVES its phase dirs being moved out (the regression the obvious single ratio-gate would cause — swapping the snapshot branch to that gate reddens this AND the pre-existing `CURRENT-version snapshot marks the milestone complete` at :321); an archived snapshot is refused once a phase is reopened under it; and a refused marker is not re-asserted by a STATE field claiming the same. roadmap-parser: `isMilestoneShippedInRoadmap` gets unit coverage at its owner module rather than only through the inventory that consumes it, plus two characterisation tests for `getMilestonePhaseFilter`'s legacy call surface — omitting the new trailing `ws` param is indistinguishable from `undefined`/`null`, and the `GSD_WORKSTREAM` env fallback still resolves. These characterise the call surface; they do not stand in for coverage of its individual callers. phase-id: the `phaseKeyFrom*` / `parentPhaseKey` one-key-space contract, incl. a property that padding a directory number never changes its key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * docs(#2562): record the two-strength shipped cross-check + milestone_shipped_unverified Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * fix(#2562): refuse an archived snapshot on a DIRTY archive, not just a live-unfinished dir The snapshot arm checked `liveIncompletePhases > 0`, which misses the shape @davesienkowski reproduced: a COMPLETE live dir beside a phase declared in the Progress table with no directory. Nothing is live-and-unfinished, the marker sails through, and `cmdWorkstreamProgress` returns `{"status":"milestone complete","progress_percent":50}` — the reported symptom verbatim, from one payload. Reproduced at 483a3ba30 before changing anything. His diagnosis is the right one and better than mine: an in-milestone directory outliving the archive means the archive is not CLEAN, and once that is true the completion ratio is meaningful again. So the check is the conjunction — any live in-milestone dir AND `completedPhases < effectivePhaseCount`. That strictly subsumes the old predicate (an incomplete member dir is in the denominator and not the numerator, so the ratio is always short when one exists) and leaves the clean-archive guard green, since a clean archive has no live dirs at all. Also corrects the module comment: the `scoped &&` prefix ungates all three signals, not just `legacy`. That is correct behaviour — unscoped, the denominator is the whole-roadmap count and membership is everything, so there is no current-milestone artifact set to check a current-milestone claim against — but the comment claimed otherwise. And the `milestone.cts` citations were ~28 lines stale after the rebase; they are now :700-702 (copy) and :783-790 (move), re-verified against this head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * fix(#2562): project milestone_shipped_unverified from list, status and progress The inventory carried the field and every renderer dropped it — `workstream.cts` was not in this PR's diff at all — so at the CLI a refused marker looked exactly like no marker: a fallback `status` and nothing saying one was seen and rejected. That is the silent collapse this issue is about, reintroduced one layer up, and it made the changeset's "visible rather than silent" claim false at every surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * test(#2562): pin the dirty-archive shape and the CLI projection Five tests, all five red against the prior src and green with it. The reviewer's repro at the builder: an archived snapshot with a COMPLETE live dir beside a dirless declared phase must be refused, and status must not contradict the percentage. Four at the CLI via runGsdTools, the surface that was dropping the field rather than the builder that already had it: `workstream progress`/`status`/`list` each project `milestone_shipped_unverified: true` for that workstream, and a clean archive still reports `false` with `status: "milestone complete"`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * docs(#2562): correct the snapshot check, the scoped-only caveat and the CLI claim Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
05cd55d43b |
fix(#1913): derive workstream progress status from shipped signals (#1916)
workstream progress trusted the mutable STATE.md `Status` field, so a shipped/archived milestone whose field was left at `executing` was reported as executing — a stale hand-maintained field became the source of truth instead of the authoritative archive/tag/ROADMAP signals. Derive status in the inventory builder from a milestoneShipped signal (archived milestone snapshot under milestones/, or a SHIPPED marker in the workstream ROADMAP), collected by inspectWorkstream. The inventory now reports `status_source` (field|derived) and `status_conflict` (true when the derived value disagrees with the stale field), and a shipped workstream is never reported executing. - src/workstream-inventory-builder.cts: milestoneShipped input + status_source/status_conflict outputs + derivation - src/workstream-inventory.cts: workstreamMilestoneShipped() signal detector wired into inspectWorkstream - tests/workstream-inventory.test.cjs: regression (builder unit + inspectWorkstream integration + negative) Closes #1913 |
||
|
Tom Boucher
|
df04aae5e4 |
enhancement(#537): migrate all hand-written bin/lib/*.cjs to TypeScript source of truth (ADR-457) (#602)
* enhancement(#537): migrate code-review-flags to TS source of truth Collapse the hand-written get-shit-done/bin/lib/code-review-flags.cjs to a TypeScript source of truth (src/code-review-flags.cts), compiled by tsc to a gitignored .cjs build artifact at the same path, per ADR-457 (build-at-publish). Second module after the semver-compare pilot (#541). Behaviour is preserved byte-for-behaviour (characterization test added in tests/code-review-flags.test.cjs locks the parser quirks). Adds compile-time type checking: CodeReviewFlags interface + CodeReviewWorkflow literal union. The require() path is unchanged, so code-review.md and the bug-3727 test keep working. The emitted .cjs is gitignored and eslint-ignored, mirroring the pilot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 9 leaf bin/lib modules to TS source of truth ADR-457 build-at-publish, batch 1 (pure leaf modules, 0 sibling-deps): 001-legacy-orphan-files, context-utilization, redaction, artifacts, command-arg-projection, clock, ui-safety-gate, review-reviewer-selection, clusters. Each moves to src/*.cts (strict TS, typed), compiled by tsc to a gitignored .cjs at the same require() path; behaviour preserved byte-for- behaviour. Adds src/node-globals.d.ts (minimal ambient shim; "types":[]). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#537): add @types/node, drop hand-rolled node-globals shim ADR-457 migration infra: replace the temporary src/node-globals.d.ts ambient shim with @types/node@22 + "types":["node"] in tsconfig.build.json. Unblocks migrating the ~49 remaining bin/lib modules that use node:fs/path/os/ child_process. Build + full suite (3030 pass) + lint all green; no .cts type changes were needed (real Node types matched the shim). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 9 more bin/lib modules to TS (batch 2) ADR-457 build-at-publish. Clean leaves: installer-migration-report, prompt-budget. Type-error-prone leaves (were tsconfig.lint-excluded; now strict-typed and removed from that exclude list): secrets, phase-lifecycle, workstream-name-policy, decisions, validate, schema-detect. Plus runtime-name-policy. Strict type fixes narrow unknown->concrete domain types (no any/ts-ignore); behaviour preserved. Full suite green, lint 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate runtime-slash to TS (cross-import proof) ADR-457. First cross-module TS->TS import: src/runtime-slash.cts imports ./runtime-name-policy.cjs and tsc resolves the sibling .cts types under strict (no declaration files; NodeNext .cjs->.cts mapping), emitting a correct require("./runtime-name-policy.cjs"). Confirms the recipe for coupled modules, which must be migrated in dependency order (leaves-up). Suite green, lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 10 more bin/lib modules to TS (batch 3) ADR-457 build-at-publish, Wave-1 leaves: event, workstream-inventory-builder, plan-scan, fallow-runner, project-root, installer-migration-authoring, update-context, 000-first-time-baseline, runtime-homes, model-catalog. Strict typing fixed real issues (narrowing unknown, qualified fs/path calls, removed unnecessary casts); plan-scan/project-root/workstream-inventory-builder dropped from tsconfig.lint exclude. Behaviour preserved; suite green, lint 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 5 large Wave-1 leaves to TS (batch 4) ADR-457 build-at-publish: configuration, state-document, shell-command- projection (42 dependents), security, command-aliases. shell-command- projection keeps a namespace child_process import for mock-intercept testability. loadConfig/migrateOnDisk emit synchronously (every caller uses them sync; the one awaited migrateOnDisk caller tolerates a non-Promise) — full suite (3030 pass) confirms behaviour preserved. configuration/ state-document/command-aliases dropped from tsconfig.lint exclude. Also fixes the malformed batch-3 changeset frontmatter (type/pr) that failed lint:docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 6 Wave-2 modules to TS (batch 5) ADR-457 build-at-publish: config-schema, model-profiles, 002-codex-legacy-hooks-json, logger, active-workstream-store, adr-parser. First batch importing already-migrated siblings (configuration, model-catalog, shell-command-projection, redaction, security) via ./sibling.cjs specifiers. Strict type narrowing (typeof guards over String(unknown)); behaviour preserved; suite 3030 pass, lint 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 5 large Wave-2 modules to TS (batch 6) ADR-457 build-at-publish: graphify, install-profiles, intel, installer-migrations, worktree-safety. installer-migrations preserves its dynamic require() loader for numbered migration modules (scoped lint suppressions). Strict typing (typeof guards over String(unknown)); behaviour preserved; suite 3030 pass, lint 0 errors. Wave 2 complete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate Wave-3 modules to TS (batch 7) ADR-457 build-at-publish: planning-workspace, runtime-artifact-layout, command-routing-hub, drift. Uses `import x = require()` for export= siblings; drift's lazy require of runtime-slash hoisted to a top-level import (verified non-circular). Behaviour preserved; suite 3030 pass, lint 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate small Wave-4 modules to TS (batch 8) ADR-457 build-at-publish: cjs-command-router-adapter, phase-command-router, surface, roadmap-upgrade. Typed the hub router handler results as the HubResult discriminated union; surface drops 4 genuinely-unused imports. Behaviour preserved; suite 3030 pass, lint 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate core hub (2.5k LOC, 68 dependents) to TS (batch 9) ADR-457 build-at-publish: get-shit-done/bin/lib/core.cjs -> src/core.cts, preserving all 63 exports via export=. All sibling deps already migrated (shell-command-projection, model-profiles, model-catalog, worktree-safety, planning-workspace, project-root, configuration, config-schema). Strict types, no any/ts-ignore; config-schema lazy require hoisted (non-circular). Behaviour preserved (independently verified: core's shard 3030 pass / 0 fail). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#537): make ESLint-coverage + test-sprawl checks migration-aware #551 test hardcoded 12 now-migrated modules as "hand-written, must be linted"; that invariant is obsoleted by the ADR-457 migration. Rewrite it to a filesystem-driven invariant that holds at every stage: a bin/lib/*.cjs must be eslint-ignored IFF it has a src/*.cts source (tsc-generated), else linted (covers package-identity, which has no TS source). Also eslint-ignore config-types.cjs (has a src counterpart) and drop the redundant tests/clock.test.cjs (clock already covered by clock-seam + bug-474 tests), which tripped the lint-test-file-count ratchet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 9 Wave-5 router/inventory modules to TS (batch 10) ADR-457 build-at-publish: phases/verify/init/agent/task/validate/roadmap/state command routers + workstream-inventory. Router handler results typed against core's exported shapes; behaviour preserved (caught+fixed a --verify boolean flag regression mid-migration). Full suite green across all shards (only the 4 local gpg-env changeset-notes failures remain; CI passes them). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 7 Wave-5 modules to TS (batch 11) ADR-457 build-at-publish: gap-checker, docs, check-command-router, frontmatter, learnings, gsd2-import, profile-pipeline. Behaviour preserved; full suite green across all shards (only the 4 local gpg-env failures remain). Also broadens atomic-write-coverage.test.cjs to accept the tsc-compiled namespace-import form while still asserting platformWriteSync is called (safety guard intact). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate config + profile-output to TS (batch 12) ADR-457 build-at-publish: config (729 LOC), profile-output (1142 LOC). All exports preserved; cmdMigrateConfig de-asynced (migrateOnDisk is sync, awaited caller tolerates it). Behaviour preserved; suite green across all shards (only the 4 local gpg-env failures). Wave 5 complete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate 5 Wave-6 modules to TS (batch 13) ADR-457 build-at-publish: template, uat, workstream, roadmap, audit. Behaviour preserved (dead toPosixPath import dropped from audit; inline requires hoisted). Suite green across all shards (only the 4 local gpg-env failures). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate commands + state hubs to TS (batch 14) ADR-457 build-at-publish: commands (1305 LOC), state (2074 LOC, 17 dependents). All exports preserved; inner requires kept non-hoisted where load-order matters (install.js, per-call security); acquireStateLock cast inlined to preserve the err.code source token a structural test inspects. Behaviour preserved; suite green across all shards (only the 4 local gpg-env failures). Wave 6 complete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate milestone to TS (batch 15a, hand-authored) ADR-457 build-at-publish: milestone -> src/milestone.cts. Authored directly (subagent capacity was unavailable). Also relaxes core.output()'s 3rd param to optional, matching its real always-optional call contract (unblocks remaining 2-arg output callers). Behaviour preserved; suite green across all shards (only the 4 local gpg-env failures). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537): migrate phase, verify, init to TS (batch 15, final modules) ADR-457 build-at-publish, Wave 7 (the last hubs): phase (1608 LOC), verify (1615), init (2113). Adds src/package-identity.d.cts so verify can import the permanently value-baked package-identity.cjs under strict TS. Fixes two regressions the migration introduced in verify: restore cmdValidateHealth's `return result` (callers/tests read result.warnings — it is NOT side-effect-only), and make the bug-3384 source-pattern test tolerant of the tsc-compiled bracket-notation form of the git_list_failed->W020 branch (behaviour intact). Full suite green across all shards (only the 4 local gpg-env failures); lint 0 errors. All 86 migratable bin/lib modules are now TypeScript sources. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#537): finalize ADR-457 migration — retire tsconfig.lint.json All hand-written bin/lib/*.cjs are now src/*.cts sources, so the checkJs stopgap tsconfig.lint.json (unused; not wired into eslint, scripts, or CI) is deleted per ADR-457's final step. Also gitignore the tsc-generated config-types.cjs (was still committed) for consistency with every other emitted artifact. package-identity.cjs stays value-baked (declared via src/package-identity.d.cts). Suite green; #551 ESLint-coverage test green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): add prepare script so unpacked/git installs build bin/lib artifacts ADR-457 build-at-publish: bin/lib/*.cjs are now gitignored, built by tsc. The prepack/prepublishOnly hooks cover `npm pack`/publish, but `npm install -g <dir>` and git installs run the `prepare` lifecycle — which was missing — so the unpacked install shipped without the compiled .cjs and failed at startup with "Cannot find module './lib/core.cjs'" (caught by the smoke-unpacked CI job). Add `prepare` mirroring prepublishOnly (build:lib + build:hooks). prepare does NOT run for registry consumers (they get the pre-built tarball), only for source/local/pack installs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): make CI build/lockfile checks work with gitignored bin/lib artifacts ADR-457 build-at-publish exposed two CI assumptions that bin/lib/*.cjs are always present on disk: - check:env's lockfile-sync ran `npm ci --dry-run`, which now triggers the `prepare` build (tsc) — but it runs before deps are installed, so tsc is absent and it misreported the lockfile as out of sync. Add --ignore-scripts (a lockfile check must not build). - the lint-tests job installs with --ignore-scripts (no prepare build), but lint:skill-deps require()s the built install-profiles.cjs. Add an explicit `npm run build:lib` step after install. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): narrow prepare to build:lib only (unbreak packed-smoke pack step) prepare running build:hooks emitted "✓ Copying ..." stdout during `npm pack`, which the install-smoke "Pack root tarball" step captures into $GITHUB_OUTPUT — breaking it with "Invalid format". build:lib (tsc) is silent on success and is all the unpacked/source install needs (the smoke-unpacked assertions exercise gsd-tools, i.e. bin/lib, and tolerate hook setup with `|| true`). Matches prepack. build:hooks still runs on prepublishOnly for real publishes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): wire Stryker mutation gate to build-at-publish layout The gate scored 0.00 because it mutated changed bin/lib/*.cjs that (a) were generated artifacts and (b) included modules with no coverage in the command's test set. Rework: mutation.yml now derives changed COVERED modules from src/*.cts and maps them to their built bin/lib/*.cjs; Stryker mutates those built artifacts with a no-rebuild command (mutating src/*.cts + per-mutant tsc was ~3x over the 30-min CI budget). NOTE: with the gate now correctly measuring the covered modules, their actual mutation score is 42.94% (< break 50) — a pre-existing test-coverage gap (adr-parser/prompt-budget/etc.), not introduced by this behaviour-preserving migration. Reaching 50 needs more tests, a threshold/scope change, or a waiver — a maintainer decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#537): raise mutation coverage of covered modules above the 50 gate Adds focused example-based unit tests that kill surviving mutants in the two lowest-scoring covered modules: - tests/prompt-budget.unit.test.cjs (112 tests): 17.9% -> 97.9% - tests/adr-parser.unit.test.cjs (205 tests): 44.7% -> 89.4% Both wired into stryker.config.mjs's command. Fresh full run over the 6 covered modules now scores 82.25% (>= break 50); every covered module is >= 68%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * enhancement(#537,#609): parallelize mutation gate via dynamic per-module matrix The serial Stryker run timed out at 30 min once the migration's added tests made every mutant re-run ~300 tests. Replace it with a dynamic matrix so the gate completes well under budget — folded into this PR (was tracked as #609) because it's a prerequisite for this PR's mutation gate to pass. - scripts/mutation-matrix.cjs: single source of truth (covered-module -> test files) computing changed covered modules from git diff -> {has_work, matrix}. - mutation.yml: detect -> dynamic `matrix: fromJSON(...)` mutate job (one parallel shard per changed module, scoped via MUTATION_TEST_CMD to only that module's tests, 15-min/shard) -> summary job that KEEPS the legacy check name "Stryker mutation score (changed files only)" so branch protection is unchanged. Per-shard jobs report as "Stryker (<module>)". - stryker.config.mjs: commandRunner.command reads MUTATION_TEST_CMD (falls back to the full command locally). Closes #609. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#537,#609): give each mutation shard ≥50% on its own tests; drop blacksmith note Per-module sharding revealed that active-workstream-store (46.5%) and frontmatter (7.4%) only cleared 50% in the old serial run via timeout-noise from the bloated 300-test command; on their own tests they were below the gate. Add focused unit tests: - tests/active-workstream-store.unit.test.cjs (115 tests): 46.5% -> 81.9% - tests/frontmatter.unit.test.cjs (165 tests): 7.4% -> 63.4% Both wired into scripts/mutation-matrix.cjs (per-module test map) and stryker.config.mjs DEFAULT_TEST_CMD. All 6 covered modules now clear break:50 with only their own tests (config-schema/context-utilization/prompt-budget/ adr-parser already did). Also removes the leftover blacksmith TODO comment — GitHub-hosted runners only; speed comes from parallel per-module shards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#537,#609): strengthen prompt-budget tests to clear the gate on its own tests prompt-budget scored 39.58% when mutation-tested with ONLY its own tests (the way the per-module CI shard runs it) — an earlier ~98% reading was inflated by accidentally running the full multi-module command. Add 96 targeted tests to tests/prompt-budget.unit.test.cjs (exact note-template text, plan-truncation arithmetic/percentages, drop-block strings, noteInjected/hardFailed booleans): scoped score 39.58% -> 68.75% (>= break 50). All 6 covered modules now clear the gate on their own tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |