77dbfb961d
* 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>
2.3 KiB
type, pr
| type | pr |
|---|---|
| Fixed | 3016 |
Deleting a phase's verification report can no longer inflate workstream completion once that report has been seen. Removing a *-VERIFICATION.md file after a failing gaps_found or human_needed verdict was recorded used to be indistinguishable from never having verified the phase at all, so completed_phases and progress_percent silently rose. workstream status/list/progress now remember the last real verdict observed per phase in a new .verification-ledger.json file alongside each workstream's STATE.md, so a failing verdict a prior read has already seen can't be erased by deleting its report. This adds a small write side effect to those previously read-only commands, and the file is a new tracked artifact under .planning/workstreams/<name>/ for projects that commit their planning docs.
The ledger fails closed, not open: once a workstream has adopted it (the ledger file exists), a phase with no remembered entry — including one whose ledger entry can't be read because the file is corrupt or unreadable — is treated as not-yet-verified-and-blocking, not as safe-to-complete. A workstream that has never used the verifier is untouched (no ledger file is ever created for it), which is what keeps every existing project from dropping to in_progress the moment this ships.
Three limitations, disclosed rather than silently left: this is prospective only — a phase verified and its report deleted before this fix ships has no ledger entry and can't be recovered retroactively. Deleting the ledger file itself, not just the report, still returns that phase to pre-adoption behavior; this is inherent to any design where a wholly-absent ledger must be safe (the alternative is gating every never-verified phase in every project on upgrade), and is not something ledger design alone can close. And the ledger is not tamper-proof: anyone with write access to .planning/workstreams/<name>/.verification-ledger.json can hand-edit an entry to "passed" and the remembered value is trusted indefinitely — this is a different and arguably worse way to inflate completion than deleting the ledger (which at least resets to a visibly pre-adoption, untracked state), since an edited entry looks like genuine durable history. Integrity-checking the ledger's own content is out of scope for this fix. (#2645)