diff --git a/.changeset/2645-verification-deletion-ledger.md b/.changeset/2645-verification-deletion-ledger.md new file mode 100644 index 000000000..b2a2e6342 --- /dev/null +++ b/.changeset/2645-verification-deletion-ledger.md @@ -0,0 +1,9 @@ +--- +type: Fixed +pr: 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//` 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//.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) diff --git a/src/workstream-inventory-builder.cts b/src/workstream-inventory-builder.cts index 0baef82b0..aca4afeed 100644 --- a/src/workstream-inventory-builder.cts +++ b/src/workstream-inventory-builder.cts @@ -21,8 +21,60 @@ function toPosixPath(p: string): string { * EXPLICIT failing verdicts the verifier emits — `missing`/`unknown` (verifier * off / not yet run) and `stale` (mtime-derived, #2348) are intentionally left * untouched so verifier-disabled projects do not regress to never-complete. + * + * #2645: `'unrecorded'` is NOT a verdict the verifier itself ever emits — it + * is an internal sentinel `workstream-inventory.cts`'s verification-deletion + * ledger substitutes for `'missing'` once that workstream has ADOPTED the + * ledger (a `.verification-ledger.json` file exists for it) but has no + * remembered entry for this specific phase. Pre-adoption (no ledger file at + * all) still resolves to plain `'missing'`, which stays OUTSIDE this set — + * that is what keeps a project untouched by this fix until it actually uses + * the verifier at least once. Post-adoption, an unrecorded phase fails + * CLOSED (counted here) rather than open, so a corrupt or evidence-absent + * ledger entry can no longer be read as "safe to complete" the way a bare + * `'missing'` sentinel is. */ -const FAILING_VERIFICATION_STATUSES = new Set(['gaps_found', 'human_needed']); +const FAILING_VERIFICATION_STATUSES = new Set(['gaps_found', 'human_needed', 'unrecorded']); + +/** + * #2562 / Bug #2445 / #2645 review: pick ONE winning item per key from a + * PRE-SORTED list — newest `mtimeMs` wins; on an exact tie the incumbent + * (first-in-sort-order) wins, since only a STRICTLY greater mtime replaces + * it. `includeItem` lets a caller exclude items before comparison (e.g. + * out-of-milestone directories) — critically, the filter runs BEFORE the + * mtime comparison, so an excluded item can never win a tie or a comparison + * against an included one. + * + * Extracted as the SINGLE shared implementation after a #2645 review found + * two independently-written copies of this exact rule had silently + * diverged: `workstream-inventory.cts`'s ledger-winner selection compared + * raw mtimes with no scoping filter, while this module's own `rollupDirByKey` + * (below) filtered out-of-milestone directories first. A stale out-of- + * milestone directory with a newer mtime than the live in-milestone one + * (plausible after a checkout/rebase resets mtimes) could then win the + * LEDGER's selection while losing the BUILDER's — reopening #2645's own + * hole for the phase that actually counts toward `completed_phases`, + * reachable with a plain `rm` and no ledger tampering. A comment asserting + * two hand-written copies "use the same rule" is not a guarantee they do; + * one shared function is. + */ +export function pickRollupWinners( + sortedItems: T[], + keyOf: (item: T) => string, + mtimeOf: (item: T) => number, + includeItem: (item: T) => boolean = () => true, +): Map { + const winners = new Map(); + for (const item of sortedItems) { + if (!includeItem(item)) continue; + const key = keyOf(item); + const incumbent = winners.get(key); + if (incumbent === undefined || mtimeOf(item) > mtimeOf(incumbent)) { + winners.set(key, item); + } + } + return winners; +} export function isCompletedInventory(status: unknown): boolean { const s = (typeof status === 'string' @@ -220,20 +272,16 @@ export function buildWorkstreamInventory(inputs: BuildWorkstreamInventoryInputs) // each add to the numerator while the denominator counts distinct phases — // pushing completed_phases past it, where the old `Math.min` cap silently // rounded the result up to 100% and hid an unstarted phase. Newest-on-disk - // wins, mirroring state.cts's #2445 de-duplication. - const rollupDirByKey = new Map(); - for (const dir of [...phaseDirNames].sort()) { - const entry = countsMap.get(dir); - if (scoped && entry?.inMilestone === false) continue; - const key = entry?.phaseKey ?? dir; - const incumbent = rollupDirByKey.get(key); - if (incumbent === undefined) { - rollupDirByKey.set(key, dir); - continue; - } - const incumbentMtime = countsMap.get(incumbent)?.mtimeMs ?? 0; - if ((entry?.mtimeMs ?? 0) > incumbentMtime) rollupDirByKey.set(key, dir); - } + // wins, mirroring state.cts's #2445 de-duplication. `pickRollupWinners` is + // the SHARED implementation `workstream-inventory.cts`'s ledger-winner + // selection also calls, so the two can never independently diverge again + // (#2645 review). + const rollupDirByKey = pickRollupWinners( + [...phaseDirNames].sort(), + (dir) => countsMap.get(dir)?.phaseKey ?? dir, + (dir) => countsMap.get(dir)?.mtimeMs ?? 0, + (dir) => !(scoped && countsMap.get(dir)?.inMilestone === false), + ); const rollupDirs = new Set(rollupDirByKey.values()); const phases: PhaseStatus[] = []; diff --git a/src/workstream-inventory.cts b/src/workstream-inventory.cts index 08dbed24b..9de9cfd25 100644 --- a/src/workstream-inventory.cts +++ b/src/workstream-inventory.cts @@ -34,7 +34,7 @@ const { phaseKeyFromDir, phaseKeyFromProse, parentPhaseKey } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- roadmap-parser.cjs is an export= CommonJS module import roadmapParserMod = require('./roadmap-parser.cjs'); const { getMilestonePhaseFilter, isMilestoneShippedInRoadmap } = roadmapParserMod; -import { buildWorkstreamInventory, isCompletedInventory } from './workstream-inventory-builder.cjs'; +import { buildWorkstreamInventory, isCompletedInventory, pickRollupWinners } from './workstream-inventory-builder.cjs'; import type { WorkstreamInventory, StateProjection, MilestoneShippedSignal } from './workstream-inventory-builder.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -199,6 +199,208 @@ function countPhaseFiles(phaseDir: string): PhaseFileCounts { return { planCount: scan.planCount, summaryCount: scan.summaryCount }; } +// ─── #2645: verification-deletion ledger ─────────────────────────────────── +// +// #2562's completeness gate (`FAILING_VERIFICATION_STATUSES`, +// workstream-inventory-builder.cts) reads a phase's verification verdict +// fresh from `*-VERIFICATION.md` on every call. That makes "verifier ran, +// found gaps, report later deleted" indistinguishable from "verifier never +// ran" — both collapse to the same `'missing'` sentinel +// (verification.cts's `missingResult()`), which is deliberately NOT in the +// failing set (so verifier-disabled projects can still reach 100%). Deleting +// a failing report is therefore sufficient to silently raise the reported +// completion percentage — a Goodhart hole (#2645). +// +// The fix persists the last REAL (non-'missing') verdict this module has +// ever observed per phase key, in a small ledger file living at the +// WORKSTREAM directory level — never inside the phase directory whose file +// is the thing being deleted, so the same `rm` that triggers the hole +// cannot also erase the memory of it. When a live read comes back 'missing', +// the ledger is consulted as a fallback; when a live read comes back with a +// real verdict — including a later 'passed' that supersedes an earlier +// failing one — the ledger is updated to match, so a genuinely re-verified +// phase is never permanently pinned. +// +// #2645 review: a NAIVE two-state read ("got entries, or nothing") fails +// OPEN — any read/parse failure degraded to `{}`, which is indistinguishable +// from "genuinely never verified", so corrupting the ledger (or deleting it +// alongside the report) silently reopened the exact hole this fix exists to +// close, one level up. THREE states, not two: +// +// 1. `'absent'` — no `.verification-ledger.json` for this workstream at +// all. This is the ONLY state that behaves exactly as pre-#2645 (a +// phase with no live report reads `'missing'`, ungated). Deliberate: +// on the day this ships, EVERY existing project is in this state for +// EVERY workstream, and gating here would drop them all to +// `in_progress` at once. A workstream stays here forever if the +// verifier is never actually used on it (criterion 2/3 — no entry is +// ever written for a `'missing'` live read, so the file itself is +// never created). +// 2. `'corrupt'` — the file exists but could not be read or parsed (I/O +// error, invalid JSON, wrong shape). This is NOT the same as absent: +// an unreadable file is evidence something existed. Treated identically +// to "present, no entry for this phase" below — fails CLOSED, not open. +// 3. `'ok'` — the file exists and parsed. A phase with an entry uses +// it; a phase WITHOUT one is "present, no entry" — this workstream has +// adopted the ledger (some phase in it has a real verdict on record), +// so an unobserved phase can no longer default to the pre-adoption +// "ungated" behavior, or the same evidence-erasure hole reopens for +// THIS phase specifically. Fails CLOSED: resolves to the `'unrecorded'` +// sentinel (`workstream-inventory-builder.cts`'s +// `FAILING_VERIFICATION_STATUSES`), not `'missing'`. +// +// Corrupt-ledger recovery: a corrupt ledger is NOT a permanent wedge. Any +// phase with a REAL live verdict on disk still writes/repairs the ledger on +// this same call (the corrupt content is fully overwritten, never patched), +// so re-running the verifier for even one phase heals the file. A phase with +// no live report and no way to re-verify stays `'unrecorded'` (gated) until +// someone re-verifies it — a deliberate, disclosed cost of failing closed, +// not an accidental one. +// +// Disclosed, ACCEPTED residual gap (not closed by this fix, and not closable +// by ledger design alone): deleting the ledger FILE ITSELF (not just the +// phase's report) returns a workstream to state 1 (`'absent'`) and restores +// pre-#2645 behavior for it. Any durable store that can fail open when +// absent has this property at its own root — the ledger raises the bar from +// "delete one file" to "delete two files in two different directories, +// including one this issue's own reproduction never needed to touch", but a +// deliberately absent ledger is indistinguishable from a never-adopted one +// by design (criterion 2/3 depend on that same indistinguishability). Rail B +// is PROSPECTIVE ONLY: a phase verified and its report deleted BEFORE this +// fix ships has no ledger entry to fall back on and cannot be retroactively +// recovered. +interface VerificationLedger { [phaseKey: string]: string; } + +interface VerificationLedgerRead { + state: 'absent' | 'corrupt' | 'ok'; + entries: VerificationLedger; +} + +function verificationLedgerPath(wsDir: string): string { + return path.join(wsDir, '.verification-ledger.json'); +} + +function readVerificationLedger(wsDir: string): VerificationLedgerRead { + const ledgerPath = verificationLedgerPath(wsDir); + let raw: string; + try { + raw = fs.readFileSync(ledgerPath, 'utf-8'); + } catch (err) { + // `fs.readFileSync` FOLLOWS symlinks, so a broken symlink at this path + // (the entry exists, its target does not) reports the EXACT SAME + // `ENOENT` as genuine absence — `code` alone cannot distinguish + // "nothing was ever here" from "something is here and cannot be read". + // `fs.lstatSync` does NOT follow symlinks, so it still finds the + // symlink entry itself even when its target is gone. Only when NEITHER + // call finds anything is this genuinely `'absent'` (pre-adoption); a + // present-but-broken symlink is evidence something existed and must + // fail CLOSED like any other unreadable ledger, not fall open. + const code = (err as NodeJS.ErrnoException)?.code; + if (code === 'ENOENT') { + try { + fs.lstatSync(ledgerPath); + return { state: 'corrupt', entries: {} }; // a symlink entry exists; its target does not + } catch (lstatErr) { + // #2645 review: every OTHER failure path in this function fails + // CLOSED — this one must too. A failed `lstatSync` is only proof of + // absence when IT ALSO reports `ENOENT`; anything else (a raced + // permission change, a path component that became inaccessible + // between the two calls, …) is not evidence the file was never + // there, and a bare `catch {}` here would silently fall OPEN exactly + // like the two-state design this fix replaced. + const lstatCode = (lstatErr as NodeJS.ErrnoException)?.code; + if (lstatCode === 'ENOENT') return { state: 'absent', entries: {} }; // truly nothing at this path + return { state: 'corrupt', entries: {} }; + } + } + // Any OTHER read failure (EACCES, EISDIR, …) also means the path EXISTS + // in some form but this process cannot see its content right now — that + // is corruption from this reader's point of view, not absence, and must + // fail closed rather than silently falling back to the ungated + // pre-adoption behavior. + return { state: 'corrupt', entries: {} }; + } + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return { state: 'corrupt', entries: {} }; + } + if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) { + return { state: 'corrupt', entries: {} }; + } + const out: VerificationLedger = {}; + for (const [key, value] of Object.entries(parsed as Record)) { + if (typeof value === 'string') out[key] = value; + } + return { state: 'ok', entries: out }; +} + +// #2645 review: Windows can transiently hold the rename target busy (AV +// scanners, indexers) — the SAME retry shape `broken-windows.cts`'s +// `writeLedgerAtomic`/`renameWithRetry` already uses for its own ledger +// write, mirrored here rather than imported (that function is private to +// its module) so this fix does not widen its own blast radius by exporting +// a new cross-module utility. +const LEDGER_RENAME_RETRY_ERRNOS = new Set(['EPERM', 'EBUSY', 'EACCES']); +const LEDGER_RENAME_MAX_ATTEMPTS = 5; +const LEDGER_RENAME_BACKOFF_MS = 25; + +function renameVerificationLedgerWithRetry(tmpPath: string, finalPath: string): void { + let lastErr: unknown; + for (let attempt = 0; attempt < LEDGER_RENAME_MAX_ATTEMPTS; attempt++) { + try { + fs.renameSync(tmpPath, finalPath); + return; + } catch (err: unknown) { + lastErr = err; + const code = (err && typeof err === 'object' && 'code' in err) ? String((err as { code?: unknown }).code) : ''; + if (code && LEDGER_RENAME_RETRY_ERRNOS.has(code) && attempt < LEDGER_RENAME_MAX_ATTEMPTS - 1) { + // Exponential-ish backoff: 25ms, 50ms, 100ms, 200ms. Transient + // Windows locks usually clear well inside that window. + const delay = LEDGER_RENAME_BACKOFF_MS * Math.pow(2, attempt); + const start = Date.now(); + while (Date.now() - start < delay) { + // Deliberate short busy-wait — no async/timer seam is available + // in this synchronous read path. + } + continue; + } + throw err; + } + } + throw lastErr; +} + +function writeVerificationLedger(wsDir: string, ledger: VerificationLedger): void { + // #2645 review: atomic write. A crash or a concurrent read mid-write + // against `verificationLedgerPath(wsDir)` directly would leave (or briefly + // expose) TRUNCATED JSON — and now that `'corrupt'` carries real semantic + // weight (it fails CLOSED, holding every phase with no live report at + // `'unrecorded'`), producing a corrupt file ourselves is a self-inflicted + // version of the exact failure mode this fix exists to survive. Write to a + // sibling temp file in the SAME directory (same filesystem — `rename` is + // only atomic within one) and rename into place: a reader can only ever + // observe the prior complete content or the new complete content. + const finalPath = verificationLedgerPath(wsDir); + const tmpPath = `${finalPath}.${process.pid}.tmp`; + try { + fs.mkdirSync(wsDir, { recursive: true }); + fs.writeFileSync(tmpPath, `${JSON.stringify(ledger, null, 2)}\n`, 'utf-8'); + renameVerificationLedgerWithRetry(tmpPath, finalPath); + } catch { + // Best-effort persistence: a missing parent directory that `mkdirSync` + // itself cannot create, a read-only filesystem, a write failure, or a + // rename failure that exhausts its retries must not break inventory + // reads, which are otherwise pure. Losing this observation only + // re-opens the pre-#2645 window for THIS run; the next successful read + // while a report is on disk repairs it. Clean up a half-written temp + // file so repeated failures cannot accumulate orphaned + // `.verification-ledger.json..tmp` files. + try { fs.unlinkSync(tmpPath); } catch { /* nothing to clean up, or cleanup itself failed — not fatal */ } + } +} + function readStateProjection(statePath: string): StateProjection { try { const stateContent = fs.readFileSync(statePath, 'utf-8'); @@ -366,7 +568,18 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream // Collect per-phase file counts (+ canonical key, milestone membership, // verification verdict). `phaseKey` lets the builder de-duplicate stale // same-numbered directories (Bug #2445's scenario) in the rollup. - const phaseFilesCounts = phaseDirNames.map(dir => { + // + // #2645 review: built from `[...phaseDirNames].sort()`, NOT the raw + // `phaseDirNames` (unsorted `readdirSync` order — `readSubdirectories`'s + // default). `buildWorkstreamInventory`'s own de-dup (`rollupDirByKey`, + // workstream-inventory-builder.cts) iterates the SAME sorted order; the + // ledger-winner tie-break below (incumbent wins unless a later entry has a + // STRICTLY newer mtime) only agrees with the builder's winner on an exact + // mtime tie if both walk entries in the same order. Iterating unsorted + // input here let the two independently pick different "winning" + // directories for one phase key on a tie — reopening the stale-duplicate- + // clobbers-live-verdict hole criterion 4 exists to close. + const rawPhaseEntries = [...phaseDirNames].sort().map(dir => { const phaseDir = path.join(p.phases, dir); const counts = countPhaseFiles(phaseDir); return { @@ -376,7 +589,83 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream planCount: counts.planCount, summaryCount: counts.summaryCount, inMilestone: isDirInCurrentMilestone(dir), - verificationStatus: readVerificationStatus(phaseDir).status, + liveVerificationStatus: readVerificationStatus(phaseDir).status, + }; + }); + + // #2645: only the directory Bug #2445's de-dup rollup would actually pick + // for a phase key may read or write that key's ledger entry. Letting every + // same-keyed directory (including a stale leftover) write would let a + // stale duplicate's stale verdict clobber the live directory's remembered + // one. + // + // #2645 review — CORRECTED: an earlier version of this comment claimed the + // milestone-scoping exclusion (`scoped && entry.inMilestone === false`) + // was safe to drop here as "out of scope", and hand-wrote a scoping-free + // tie-break rule. That was a real bug, not a scope call: in a SCOPED + // workstream, a stale OUT-of-milestone directory sharing a phase key with + // the live IN-milestone one can have a newer mtime (plausible after a + // checkout/rebase resets mtimes) and would then win THIS selection while + // the builder's own `rollupDirByKey` — which DOES apply the scoping filter + // — picks the live directory instead. `isLedgerWinner` would then be false + // for the live directory, so deleting ITS `*-VERIFICATION.md` would never + // consult the ledger and would reopen #2645's exact hole for the phase + // that actually counts toward `completed_phases` — reachable with a plain + // `rm`, no ledger tampering required. Fixed by calling the SAME shared + // `pickRollupWinners` the builder's `rollupDirByKey` now also calls, with + // the identical scoping filter, rather than a second hand-written copy. + const ledgerWinnerByKey = pickRollupWinners( + rawPhaseEntries, + (entry) => entry.phaseKey, + (entry) => entry.mtimeMs, + (entry) => !(scoped && entry.inMilestone === false), + ); + + const ledgerRead = readVerificationLedger(wsDir); + // Both `'corrupt'` and `'ok'` start from whatever entries could actually be + // trusted (empty for `'corrupt'` — nothing in an unparseable file is + // trusted) and get REPAIRED below by any real verdict this call observes; + // only `'absent'` skips the ledger mechanism entirely (pre-adoption). + const verificationLedger = ledgerRead.entries; + let ledgerDirty = false; + for (const winner of ledgerWinnerByKey.values()) { + if (winner.liveVerificationStatus === 'missing') continue; + if (verificationLedger[winner.phaseKey] !== winner.liveVerificationStatus) { + verificationLedger[winner.phaseKey] = winner.liveVerificationStatus; + ledgerDirty = true; + } + } + // A `'corrupt'` read that observes no real verdict this call has nothing to + // repair with — writing an empty `{}` would DESTROY whatever the corrupt + // file's bytes might still hold (a human could recover it by hand; this + // fix must not foreclose that). Only write when there is something real to + // persist, exactly as for `'absent'`/`'ok'`. + if (ledgerDirty) writeVerificationLedger(wsDir, verificationLedger); + + const phaseFilesCounts = rawPhaseEntries.map(entry => { + const isLedgerWinner = ledgerWinnerByKey.get(entry.phaseKey) === entry; + let verificationStatus = entry.liveVerificationStatus; + if (entry.liveVerificationStatus === 'missing' && isLedgerWinner) { + if (ledgerRead.state === 'absent') { + // State 1: pre-adoption. Exactly today's behavior — 'missing' is + // NOT in FAILING_VERIFICATION_STATUSES, so this does not gate. + verificationStatus = 'missing'; + } else { + // States 2/3 ('corrupt' or 'ok'): this workstream has adopted the + // ledger. A remembered entry wins; no entry fails CLOSED to the + // 'unrecorded' sentinel rather than falling open to 'missing'. + const remembered = verificationLedger[entry.phaseKey]; + verificationStatus = remembered !== undefined ? remembered : 'unrecorded'; + } + } + return { + directory: entry.directory, + phaseKey: entry.phaseKey, + mtimeMs: entry.mtimeMs, + planCount: entry.planCount, + summaryCount: entry.summaryCount, + inMilestone: entry.inMilestone, + verificationStatus, }; }); diff --git a/tests/workstream-inventory.test.cjs b/tests/workstream-inventory.test.cjs index 6312bc077..82f810260 100644 --- a/tests/workstream-inventory.test.cjs +++ b/tests/workstream-inventory.test.cjs @@ -11,7 +11,7 @@ const fs = require('fs'); const path = require('path'); const { cleanup } = require('./helpers.cjs'); const { createFixture, seedWorkstream } = require('./fixtures/index.cjs'); -const { buildWorkstreamInventory, isCompletedInventory } = require('../gsd-core/bin/lib/workstream-inventory-builder.cjs'); +const { buildWorkstreamInventory, isCompletedInventory, pickRollupWinners } = require('../gsd-core/bin/lib/workstream-inventory-builder.cjs'); const { inspectWorkstream } = require('../gsd-core/bin/lib/workstream-inventory.cjs'); const { VERIFIER_STATUSES } = require('../gsd-core/bin/lib/verification.cjs'); const { phaseKeyFromDir, phaseKeyFromProse, phaseKeyFromToken } = require('../gsd-core/bin/lib/phase-id.cjs'); @@ -1006,3 +1006,759 @@ describe('#2562 — a shipped marker its own artifacts contradict is not asserte assert.equal(inv.milestone_shipped_unverified, true); }); }); + +// #2645 — deleting a *-VERIFICATION.md must never raise reported completion. +// #2562 gated completeness on the verdict READ FRESH from disk every call: +// "verifier ran gaps_found, report later deleted" and "verifier never ran" +// both read as the internal 'missing' sentinel, and only the fresh read was +// consulted — so deleting the evidence file alone was enough to silently +// raise completed_phases/progress_percent. The fix persists the last REAL +// verdict observed per phase key in a ledger file living at the WORKSTREAM +// level (`.verification-ledger.json`, sibling to STATE.md/ROADMAP.md), +// consulted only when the live read comes back 'missing'. +describe('#2645 — deleting a verification report must not raise completeness', () => { + let tmpDir; + before(() => { tmpDir = createFixture(); }); + after(() => cleanup(tmpDir)); + + const FLAT_STATE = 'status: executing\n'; + function flatRoadmap(rows) { + return [ + '# Roadmap', '', '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- |', + ...rows, + '', + ].join('\n'); + } + + function writePhase(wsDir, slug, { plans = 1, summaries = 1, verification } = {}) { + const dir = path.join(wsDir, 'phases', slug); + fs.mkdirSync(dir, { recursive: true }); + for (let i = 1; i <= plans; i++) fs.writeFileSync(path.join(dir, `0${i}-PLAN.md`), '# plan\n'); + for (let i = 1; i <= summaries; i++) fs.writeFileSync(path.join(dir, `0${i}-SUMMARY.md`), '# summary\n'); + if (verification) fs.writeFileSync(path.join(dir, '01-VERIFICATION.md'), `---\nstatus: ${verification}\n---\n`); + return dir; + } + + function verificationFilePath(wsDir, slug) { + return path.join(wsDir, 'phases', slug, '01-VERIFICATION.md'); + } + + // Row 1 — failing-first regression, the issue's own reproduction sequence. + test('deleting a gaps_found report after it was observed does not raise completeness (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-gaps' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + + const before1 = inspectWorkstream(tmpDir, 'ws-2645-gaps', { active: null }); + assert.ok(before1); + assert.equal(before1.phases[0].status, 'in_progress', 'gaps_found must not count as complete'); + assert.equal(before1.completed_phases, 0); + assert.equal(before1.progress_percent, 0); + + fs.unlinkSync(verificationFilePath(wsDir, '1-foo')); + + const after1 = inspectWorkstream(tmpDir, 'ws-2645-gaps', { active: null }); + assert.ok(after1); + assert.equal(after1.phases[0].status, 'in_progress', + 'deleting the failing report must not flip the phase to complete'); + assert.equal(after1.completed_phases, 0, 'deleting evidence must never raise completed_phases'); + assert.equal(after1.progress_percent, 0, 'deleting evidence must never raise progress_percent'); + }); + + // Row 2 — the other FAILING_VERIFICATION_STATUSES member. + test('deleting a human_needed report after it was observed does not raise completeness (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-human' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'human_needed' }); + + inspectWorkstream(tmpDir, 'ws-2645-human', { active: null }); // observe while present + fs.unlinkSync(verificationFilePath(wsDir, '1-foo')); + + const after = inspectWorkstream(tmpDir, 'ws-2645-human', { active: null }); + assert.ok(after); + assert.equal(after.phases[0].status, 'in_progress'); + assert.equal(after.completed_phases, 0); + assert.equal(after.progress_percent, 0); + }); + + // Row 3 — criterion 2: verifier-disabled projects (no report ever written) + // must still be able to reach 100%. + test('a phase that was never verified still reaches complete (#2645, criterion 2)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-never' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | Complete | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1 }); // no verification file, ever + + const inv = inspectWorkstream(tmpDir, 'ws-2645-never', { active: null }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'complete'); + assert.equal(inv.completed_phases, 1); + assert.equal(inv.progress_percent, 100); + }); + + // Row 4 — criterion 3: same on-disk shape as row 3 (no file), across TWO + // reads, must behave IDENTICALLY — the ledger must not newly gate a phase + // that was simply never verified in the first place. + test('a not-yet-verified phase is not newly gated by the ledger (#2645, criterion 3)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-not-yet' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | Complete | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1 }); + + const first = inspectWorkstream(tmpDir, 'ws-2645-not-yet', { active: null }); + const second = inspectWorkstream(tmpDir, 'ws-2645-not-yet', { active: null }); + assert.equal(first.progress_percent, 100); + assert.equal(second.progress_percent, 100, 'a second read must not change the outcome'); + assert.equal(second.phases[0].status, 'complete'); + }); + + // Row 5 — recovery: a genuinely re-verified phase must not be pinned by an + // earlier failing verdict the ledger remembers. + test('a re-verified passed phase is not pinned by an earlier failing ledger entry (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-recover' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + inspectWorkstream(tmpDir, 'ws-2645-recover', { active: null }); // observe gaps_found + + // Re-verify: overwrite the report with a passing verdict. + fs.writeFileSync(verificationFilePath(wsDir, '1-foo'), '---\nstatus: passed\n---\n'); + const reverified = inspectWorkstream(tmpDir, 'ws-2645-recover', { active: null }); + assert.equal(reverified.phases[0].status, 'complete', 'a genuine re-verify must count'); + + // Delete the now-passing report — the ledger's newest real verdict is + // 'passed', which is not in the failing set, so this must stay complete. + fs.unlinkSync(verificationFilePath(wsDir, '1-foo')); + const afterDelete = inspectWorkstream(tmpDir, 'ws-2645-recover', { active: null }); + assert.equal(afterDelete.phases[0].status, 'complete', + 'the ledger must hold the newest verdict, not an earlier failing one'); + }); + + // Row 6 — criterion 4: the ledger must not live inside the phase directory + // whose file is the one being deleted. + test('ledger file lives outside the phase directory it protects (#2645, criterion 4)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-location' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + const phaseDir = writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + inspectWorkstream(tmpDir, 'ws-2645-location', { active: null }); + + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + assert.ok(fs.existsSync(ledgerPath), 'the ledger must be persisted somewhere'); + assert.ok(!ledgerPath.startsWith(phaseDir + path.sep) && ledgerPath !== phaseDir, + 'the ledger must not live inside the phase directory'); + + // Deleting the ENTIRE phase directory (not just the report) must not + // touch the ledger — this is what "not the same file" is protecting + // against in the realistic case. + cleanup(phaseDir); + assert.ok(fs.existsSync(ledgerPath), 'removing the phase directory must not remove the ledger'); + }); + + // Row 7 — Bug #2445 dedup safety: a stale duplicate directory sharing the + // same phase key must not be able to clobber the WINNING (newest) + // directory's ledger entry with its own stale/leftover verdict. + test('a stale duplicate directory cannot clobber the ledger entry of the live directory (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-dupe' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | Complete | - |', + '| 2. Bar | 0/1 | Not started | - |', + ])); + // Stale leftover directory (older mtime), leftover gaps_found report. + const staleDir = writePhase(wsDir, '01-foo-old', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const oldTime = new Date('2020-01-01T00:00:00Z'); + fs.utimesSync(staleDir, oldTime, oldTime); + // Live/winning directory (newer mtime), currently passed. + const liveDir = writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'passed' }); + const newTime = new Date('2026-01-01T00:00:00Z'); + fs.utimesSync(liveDir, newTime, newTime); + + const before7 = inspectWorkstream(tmpDir, 'ws-2645-dupe', { active: null }); + assert.ok(before7); + assert.equal(before7.completed_phases, 1, 'the winning (newest) directory is passed'); + + // Delete the WINNING directory's report. If the stale directory's + // gaps_found had clobbered the ledger, this phase key would now + // incorrectly read gaps_found and never recover. It must instead read + // the winning directory's own remembered 'passed'. + fs.unlinkSync(path.join(liveDir, '01-VERIFICATION.md')); + + const after7 = inspectWorkstream(tmpDir, 'ws-2645-dupe', { active: null }); + assert.ok(after7); + assert.equal(after7.completed_phases, 1, + "the ledger must remember the WINNING directory's passed verdict, not the stale duplicate's gaps_found"); + }); + + // Row 8 — boundary: an EXACT mtime tie between two same-keyed directories. + // The builder's own tie-break (`rollupDirByKey`) walks + // `[...phaseDirNames].sort()` and keeps the incumbent on a tie + // (first-in-sort-order wins, since only a STRICTLY newer mtime replaces + // it). The ledger's winner selection must walk the SAME sorted order so + // the two deterministically agree on which directory wins — not merely + // "some directory wins" (a prior version of this test asserted only + // `completed_phases === 0 || 1`, which is true regardless of agreement and + // caught nothing; both `01-foo-a`/`01-foo-b` sort deterministically, so the + // outcome here is not a coin flip). + test('an exact mtime tie resolves the ledger winner by sort order, matching the builder (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-tie' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | Complete | - |', + ])); + const tieTime = new Date('2025-06-01T00:00:00Z'); + // '01-foo-a' sorts before '01-foo-b' — with equal mtimes, BOTH the + // builder's rollup and the ledger's winner selection must keep the + // incumbent '01-foo-a' (passed), never adopt '01-foo-b' (gaps_found). + const dirA = writePhase(wsDir, '01-foo-a', { plans: 1, summaries: 1, verification: 'passed' }); + fs.utimesSync(dirA, tieTime, tieTime); + const dirB = writePhase(wsDir, '01-foo-b', { plans: 1, summaries: 1, verification: 'gaps_found' }); + fs.utimesSync(dirB, tieTime, tieTime); + + assert.doesNotThrow(() => inspectWorkstream(tmpDir, 'ws-2645-tie', { active: null })); + const inv = inspectWorkstream(tmpDir, 'ws-2645-tie', { active: null }); + assert.ok(inv); + // Deterministic, not "either is fine": the builder's own rollup counts + // exactly the sort-order incumbent (01-foo-a, passed) as complete. + assert.equal(inv.completed_phases, 1, + 'the sort-order incumbent (01-foo-a, passed) must be the one the builder counts complete'); + + // Concrete cross-check that the LEDGER's winner selection agrees with the + // builder's, not just that this run's numbers happen to match: delete + // 01-foo-a's report (unlink bumps the directory's mtime — restore it to + // the exact tie value so the SECOND read still sees a genuine tie, not a + // newest-mtime win). If the ledger's winner selection had instead picked + // 01-foo-b (the bug this test guards — unsorted iteration disagreeing + // with the builder's sorted `rollupDirByKey`), this phase key's + // remembered verdict would be 'gaps_found' and the phase would flip to + // in_progress. It must instead stay complete, because the ledger + // remembers 01-foo-a's 'passed' — the same directory the builder uses. + fs.unlinkSync(path.join(dirA, '01-VERIFICATION.md')); + fs.utimesSync(dirA, tieTime, tieTime); // restore the tie the unlink disturbed + const afterDelete = inspectWorkstream(tmpDir, 'ws-2645-tie', { active: null }); + assert.equal(afterDelete.completed_phases, 1, + "the ledger must have remembered the sort-order incumbent's 'passed' verdict, not the other directory's gaps_found"); + }); + + // Row 9 — property: whatever sequence of REAL verdicts is observed for a + // phase key, once the file goes missing the ledger must replay exactly the + // LAST one observed — never an earlier one, never a synthesized value. + // + // #2645 review: `'stale'` is deliberately EXCLUDED here, not merely + // forgotten. Writing a literal `status: stale` frontmatter value does NOT + // round-trip as `'stale'` — `readVerificationStatus` + // (`src/verification.cts`) explicitly excludes `'stale'` from its raw-file + // routing lookup (`rawStatus !== 'stale'`) and falls through to the + // "Unknown value" branch, returning `'unknown'` instead. A genuine + // `'stale'` verdict is only reachable via `findStaleVerificationSummary`'s + // mtime comparison (a SUMMARY file newer than the VERIFICATION file), not + // by writing the word into the file. Including `'stale'` in this array + // without accounting for that would silently substitute `'unknown'` on + // every iteration and still pass (both are non-failing) — a docstring + // claiming "real verdicts" coverage it does not actually exercise. + // `'stale'` handling is explicitly out of scope for this issue (#2348) and + // is not gated by `FAILING_VERIFICATION_STATUSES` either way. + test('property: the ledger always replays the most recently observed real verdict after deletion (#2645)', () => { + const REAL_STATUSES = ['passed', 'gaps_found', 'human_needed', 'unknown']; + const FAILING = new Set(['gaps_found', 'human_needed']); + fc.assert(fc.property( + fc.array(fc.constantFrom(...REAL_STATUSES), { minLength: 1, maxLength: 6 }), + (sequence) => { + const wsName = `ws-2645-prop-${Math.random().toString(36).slice(2)}`; + const wsDir = seedWorkstream(tmpDir, { name: wsName }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | Complete | - |', + ])); + const dir = writePhase(wsDir, '1-foo', { plans: 1, summaries: 1 }); + const reportPath = path.join(dir, '01-VERIFICATION.md'); + + let lastReal = null; + for (const status of sequence) { + fs.writeFileSync(reportPath, `---\nstatus: ${status}\n---\n`); + inspectWorkstream(tmpDir, wsName, { active: null }); // observe + lastReal = status; + } + fs.unlinkSync(reportPath); + + const inv = inspectWorkstream(tmpDir, wsName, { active: null }); + const expectComplete = !FAILING.has(lastReal); + assert.equal(inv.phases[0].status === 'complete', expectComplete, + `after observing ${JSON.stringify(sequence)} then deleting, status must reflect the last real verdict (${lastReal})`); + + cleanup(wsDir); + }, + ), { numRuns: 25 }); + }); + + // Row 10 — CONTRIBUTING.md "Filesystem writes and installers": code that + // writes under `.planning` needs fault-injection coverage. A corrupted + // `.verification-ledger.json` (bad JSON, or valid JSON of the wrong shape) + // must degrade to "nothing remembered", never throw and never break the + // read-only commands this ledger is a side effect of. + test('a corrupted ledger file degrades to no memory instead of throwing (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-corrupt-ledger' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + // Not valid JSON at all. + fs.writeFileSync(ledgerPath, 'not json{{{'); + assert.doesNotThrow(() => inspectWorkstream(tmpDir, 'ws-2645-corrupt-ledger', { active: null })); + + // Valid JSON, wrong shape (array instead of object) — must also degrade, + // not partially trust it. + fs.writeFileSync(ledgerPath, '["gaps_found"]'); + const inv = inspectWorkstream(tmpDir, 'ws-2645-corrupt-ledger', { active: null }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', 'the live gaps_found read still governs regardless of ledger corruption'); + }); + + // Row 10b — the path Row 10 does NOT exercise (a live report is present in + // both its cases, so the live read governs regardless of ledger state). + // With the report ALSO absent, a corrupt ledger must fail CLOSED — an + // unreadable/malformed ledger for an ADOPTED workstream (the ledger file + // exists) is treated as "present, no trustworthy entry", exactly like + // "present, no entry for this phase", NOT as "absent" (pre-adoption). This + // is the fail-open→fail-closed distinction the maintainer's review required. + test('a corrupted ledger with no live report fails closed instead of completing (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-corrupt-no-report' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + // Adopt the ledger for real (a genuine observation), matching the shape + // of an actually-used workstream, THEN corrupt it and remove the report — + // this is the realistic "ledger existed, now can't be trusted, AND the + // evidence that would let the live read cover for it is also gone" case. + const before = inspectWorkstream(tmpDir, 'ws-2645-corrupt-no-report', { active: null }); + assert.equal(before.completed_phases, 0, 'guard: starts correctly gated by the live gaps_found verdict'); + + fs.unlinkSync(path.join(wsDir, 'phases', '1-foo', '01-VERIFICATION.md')); + fs.writeFileSync(ledgerPath, 'not json{{{'); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-corrupt-no-report', { active: null }); }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', + 'a corrupt ledger for an adopted workstream must fail closed, not silently permit completion'); + assert.equal(inv.completed_phases, 0, 'the percentage must not rise just because the ledger became unreadable'); + }); + + // Row 13 — "delete the ledger" case #1: the ledger file is removed, but + // the phase's report is STILL PRESENT and still fails verification. This + // must never raise completeness (the live read governs regardless of + // ledger presence), and the deletion must be SELF-HEALING: the very next + // read that observes the still-present real verdict recreates the ledger. + test('deleting only the ledger file (report still present) does not raise completeness and self-heals (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-delete-ledger-only' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + inspectWorkstream(tmpDir, 'ws-2645-delete-ledger-only', { active: null }); // adopt + assert.ok(fs.existsSync(ledgerPath), 'guard: the ledger must exist before deleting it is meaningful'); + + fs.unlinkSync(ledgerPath); + const afterLedgerDelete = inspectWorkstream(tmpDir, 'ws-2645-delete-ledger-only', { active: null }); + assert.equal(afterLedgerDelete.completed_phases, 0, + 'the still-present gaps_found report must keep this gated regardless of the ledger'); + assert.ok(fs.existsSync(ledgerPath), 'a read that observes a real verdict must recreate/self-heal the ledger'); + }); + + // Row 14 — the DISCLOSED, ACCEPTED residual gap, pinned deliberately so it + // is never mistaken for a silent regression: deleting the report AND the + // ledger TOGETHER, after a failing verdict was already observed and + // recorded, returns this phase key to the pre-adoption `'absent'` ledger + // state — indistinguishable, by design, from a workstream that never used + // the verifier at all (criteria 2/3 require exactly that indistinguishability + // for a workstream that HASN'T adopted the ledger). This is the "prospective + // only" limitation named in the changeset: any durable store that must fail + // OPEN when wholly absent (to avoid gating every pre-existing project on + // upgrade) has this property at its own root. The bar is raised from "delete + // one file" to "delete two files in two different directories, one of which + // this issue's own reproduction never needed to touch" — not eliminated. + test('deleting the report AND the ledger together reopens the pre-adoption window (documented, not a regression) (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-delete-both' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + inspectWorkstream(tmpDir, 'ws-2645-delete-both', { active: null }); // adopt + observe gaps_found + assert.ok(fs.existsSync(ledgerPath)); + + fs.unlinkSync(path.join(wsDir, 'phases', '1-foo', '01-VERIFICATION.md')); + fs.unlinkSync(ledgerPath); + + const inv = inspectWorkstream(tmpDir, 'ws-2645-delete-both', { active: null }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'complete', + 'documented limitation: removing BOTH files returns this phase to the pre-adoption/never-verified state — ' + + 'this is the accepted "prospective only" boundary, not a bug in this fix'); + }); + + // Row 11 — fault injection per CONTRIBUTING.md: read-only target directory + // / partial write failure. Monkeypatch `fs.writeFileSync` to throw only for + // the ledger's WRITE TARGET (delegating everything else to the real + // implementation, since STATE.md/ROADMAP.md/phase files also go through + // the same fs module) — `inspectWorkstream` must still return a valid + // inventory rather than propagating the write failure. Restored via + // `t.after()` (never `try/finally` in a test body — CONTRIBUTING.md:344 — + // and never `chmod 0o000`, which root bypasses in CI). + // + // #2645 review: the write target is the ledger's TEMP file + // (`..tmp`), not `ledgerPath` itself — the write is + // atomic (temp file + rename, #2645 review). Matching only the exact + // final path here made this mock a no-op once atomicity landed (the real + // write always succeeded, so the assertion that follows failed): fixed to + // match anything starting with `ledgerPath`, which covers the temp file + // regardless of its exact suffix. + test('a write failure on the ledger file does not break inspectWorkstream (#2645)', (t) => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-write-fail' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + const originalWriteFileSync = fs.writeFileSync; + fs.writeFileSync = (targetPath, ...rest) => { + if (typeof targetPath === 'string' && targetPath.startsWith(ledgerPath)) { + throw new Error('injected: simulated read-only target directory'); + } + return originalWriteFileSync(targetPath, ...rest); + }; + t.after(() => { fs.writeFileSync = originalWriteFileSync; }); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-write-fail', { active: null }); }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', 'the inventory is still correct even though persistence failed'); + assert.ok(!fs.existsSync(ledgerPath), 'the failed write must not have left a partial/corrupt ledger file'); + const leftoverTmp = fs.readdirSync(wsDir).filter(name => name.startsWith('.verification-ledger.json.') && name.endsWith('.tmp')); + assert.deepEqual(leftoverTmp, [], 'a failed temp-file write must not leave an orphaned temp file behind'); + }); + + // Row 12 — the read-side counterpart: a read failure on the ledger path + // specifically (e.g. permission denied) must also degrade rather than + // throw, and must not mask the live on-disk verdict for THIS call. + test('a read failure on the ledger file does not break inspectWorkstream (#2645)', (t) => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-read-fail' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | Complete | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + // Seed a ledger entry via a real (unmocked) observation first. + inspectWorkstream(tmpDir, 'ws-2645-read-fail', { active: null }); + assert.ok(fs.existsSync(ledgerPath), 'guard: the ledger must exist before the read-failure case is meaningful'); + + const originalReadFileSync = fs.readFileSync; + fs.readFileSync = (targetPath, ...rest) => { + if (typeof targetPath === 'string' && targetPath === ledgerPath) { + throw new Error('injected: simulated permission denied'); + } + return originalReadFileSync(targetPath, ...rest); + }; + t.after(() => { fs.readFileSync = originalReadFileSync; }); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-read-fail', { active: null }); }); + assert.ok(inv); + // The ledger read failed (treated as 'corrupt', not 'absent', since the + // file DOES exist) — this workstream has adopted the ledger, so this + // falls CLOSED. The report is still live on disk with a real + // (non-missing) gaps_found verdict though, so the phase is correctly + // in_progress from the LIVE read regardless — the fail-closed path isn't + // even reached for this phase (its live read isn't 'missing'). + assert.equal(inv.phases[0].status, 'in_progress'); + }); + + // Row 15 — CONTRIBUTING.md fault-injection: broken symlink. `readFileSync` + // FOLLOWS symlinks, so a broken symlink at the ledger path reports the + // SAME `ENOENT` as genuine absence via errno alone — `readVerificationLedger` + // disambiguates with `lstatSync` (which does NOT follow symlinks) before + // concluding `'absent'`. This is the realistic worst case: the durable + // memory is unreadable AND there is no live report to fall back on. + test('a broken symlink at the ledger path fails closed instead of falling open to pre-adoption (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-broken-symlink' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + const before = inspectWorkstream(tmpDir, 'ws-2645-broken-symlink', { active: null }); // adopt + assert.equal(before.completed_phases, 0, 'guard: gated by the live gaps_found verdict'); + + fs.unlinkSync(path.join(wsDir, 'phases', '1-foo', '01-VERIFICATION.md')); + fs.unlinkSync(ledgerPath); + fs.symlinkSync(path.join(wsDir, 'does-not-exist-target'), ledgerPath); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-broken-symlink', { active: null }); }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', + 'a broken symlink is evidence something existed — it must fail closed, not fall open to pre-adoption behavior'); + assert.equal(inv.completed_phases, 0, 'the percentage must not rise because the ledger became a broken symlink'); + }); + + // Row 15b — pins the lstat fail-closed fix specifically. The broken-symlink + // disambiguation in `readVerificationLedger` calls `fs.lstatSync` after + // `fs.readFileSync` reports `ENOENT`, to tell "genuinely absent" from "a + // symlink entry exists, its target does not". A REVIEW caught that the + // `catch` around that `lstatSync` call originally treated ANY lstat + // failure as proof of absence — but only `lstatSync` ITSELF reporting + // `ENOENT` is genuine proof; a DIFFERENT lstat failure (a raced permission + // change, a path component that became inaccessible between the two + // calls, …) is not evidence the file was never there, and must fail + // CLOSED like every other path in that function. Double-fault both calls + // to pin this: `readFileSync` throws `ENOENT` (as it does for a genuinely + // missing path or file), `lstatSync` throws something else (`EACCES`) — + // this combination is impossible to produce with a real filesystem state + // (if the path is genuinely gone, `lstatSync` reports `ENOENT` too), so it + // is monkeypatched directly rather than constructed on disk; this is what + // "a future edit could re-widen that catch and fall open again silently" + // means without a test — the double-fault CANNOT be exercised any other + // way. Restored via `t.after()`. + test('a non-ENOENT lstat failure during broken-symlink disambiguation fails closed, not open (#2645)', (t) => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-lstat-double-fault' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + const before = inspectWorkstream(tmpDir, 'ws-2645-lstat-double-fault', { active: null }); // adopt + assert.equal(before.completed_phases, 0, 'guard: gated by the live gaps_found verdict'); + fs.unlinkSync(path.join(wsDir, 'phases', '1-foo', '01-VERIFICATION.md')); + + const originalReadFileSync = fs.readFileSync; + const originalLstatSync = fs.lstatSync; + fs.readFileSync = (targetPath, ...rest) => { + if (typeof targetPath === 'string' && targetPath === ledgerPath) { + throw Object.assign(new Error('injected: simulated missing ledger'), { code: 'ENOENT' }); + } + return originalReadFileSync(targetPath, ...rest); + }; + fs.lstatSync = (targetPath, ...rest) => { + if (typeof targetPath === 'string' && targetPath === ledgerPath) { + throw Object.assign(new Error('injected: simulated raced permission fault'), { code: 'EACCES' }); + } + return originalLstatSync(targetPath, ...rest); + }; + t.after(() => { + fs.readFileSync = originalReadFileSync; + fs.lstatSync = originalLstatSync; + }); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-lstat-double-fault', { active: null }); }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', + 'a non-ENOENT lstat failure is not proof of absence — it must fail closed (corrupt), not fall open (absent)'); + assert.equal(inv.completed_phases, 0, 'the percentage must not rise because of an ambiguous double-fault read'); + }); + + // Row 16 — CONTRIBUTING.md fault-injection: missing/uncreatable parent + // directory for the write side. Monkeypatch `fs.mkdirSync` to throw only + // for the workstream directory (delegating everything else), simulating a + // parent directory `writeVerificationLedger` cannot ensure exists. + test('an uncreatable parent directory on write does not break inspectWorkstream (#2645)', (t) => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-no-parent-dir' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + const originalMkdirSync = fs.mkdirSync; + fs.mkdirSync = (targetPath, ...rest) => { + if (typeof targetPath === 'string' && targetPath === wsDir) { + throw new Error('injected: simulated missing/uncreatable parent directory'); + } + return originalMkdirSync(targetPath, ...rest); + }; + t.after(() => { fs.mkdirSync = originalMkdirSync; }); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-no-parent-dir', { active: null }); }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', 'the live report still governs even though persistence failed'); + assert.ok(!fs.existsSync(ledgerPath), 'a write that could not ensure its directory must not leave a ledger file'); + }); + + // Row 17 — CONTRIBUTING.md fault-injection: rename failure and temp-file + // cleanup, now applicable because the write is atomic (temp file + rename, + // #2645 review). Monkeypatch `fs.renameSync` to throw only for the ledger's + // rename — the temp file must be written, the rename must fail, and the + // orphaned temp file must be cleaned up rather than accumulating. + test('a rename failure during the atomic ledger write cleans up the temp file (#2645)', (t) => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-rename-fail' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ + '| 1. Foo | 1/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + const ledgerPath = path.join(wsDir, '.verification-ledger.json'); + + const originalRenameSync = fs.renameSync; + fs.renameSync = (src, dest) => { + if (typeof dest === 'string' && dest === ledgerPath) { + throw Object.assign(new Error('injected: simulated rename failure'), { code: 'EPERM' }); + } + return originalRenameSync(src, dest); + }; + t.after(() => { fs.renameSync = originalRenameSync; }); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-2645-rename-fail', { active: null }); }); + assert.ok(inv); + assert.equal(inv.phases[0].status, 'in_progress', 'the live report still governs even though persistence failed'); + assert.ok(!fs.existsSync(ledgerPath), 'a failed rename must not leave a ledger file at the final path'); + const leftoverTmp = fs.readdirSync(wsDir).filter(name => name.startsWith('.verification-ledger.json.') && name.endsWith('.tmp')); + assert.deepEqual(leftoverTmp, [], 'a failed rename must not leave an orphaned temp file behind'); + }); + + // Row 18 — BLOCKER regression, and THIS IS THE ONLY ROW THAT PROVES IT + // END TO END. `pickRollupWinners` (`workstream-inventory-builder.cts`) is + // the SINGLE shared implementation both `buildWorkstreamInventory`'s + // `rollupDirByKey` and `inspectWorkstream`'s ledger-winner selection now + // call. An earlier version of the ledger selection was a SEPARATE + // hand-written copy that omitted the `includeItem` (milestone-scoping) + // filter — so in a scoped workstream, a stale OUT-of-milestone directory + // sharing a phase key with the live IN-milestone one, with a newer mtime + // (plausible after a checkout/rebase resets mtimes), could win the + // LEDGER's selection while losing the BUILDER's. `isLedgerWinner` would + // then be false for the live directory, so deleting ITS + // `*-VERIFICATION.md` would never consult the ledger — reopening #2645's + // hole for the phase that actually counts toward `completed_phases`, + // reachable with a plain `rm`, no ledger tampering. + // + // This test constructs that EXACT collision directly — one synthetic key + // shared by two entries, one included (in-milestone) and one excluded + // (out-of-milestone), the excluded one given the larger mtime — and + // proves `pickRollupWinners` applies the filter BEFORE comparing mtimes. + // + // Row 19 (below) does NOT reproduce this same-key collision through the + // real `phaseKeyFromDir` / `isDirInCurrentMilestone` pipeline — extensive + // probing (documented in `10-diagnosis.md`'s "Fourth note") found no + // directory-naming pair that shares a rollup key while diverging in + // milestone membership under the CURRENT roadmap-parser implementation; + // every membership-determining path collapses to the same phase-number + // extraction the rollup key already uses. Row 19 instead pins a narrower, + // real, DISTINCTLY-keyed guarantee (an out-of-milestone phase's verdict is + // never written into the ledger at all) — genuinely useful coverage, but + // NOT a substitute for this row: it does not exercise the collision path, + // and the pre-fix code would have passed it too. + test('pickRollupWinners: an excluded (out-of-milestone) item can never win over an included one, even with a newer mtime (#2645)', () => { + const liveInMilestone = { key: 'shared', mtimeMs: 100, inMilestone: true, label: 'live' }; + const staleOutOfMilestone = { key: 'shared', mtimeMs: 999999, inMilestone: false, label: 'stale' }; // far newer mtime, but excluded + const scoped = true; + + const winners = pickRollupWinners( + [liveInMilestone, staleOutOfMilestone], // pre-sorted input, as both real call sites provide + (item) => item.key, + (item) => item.mtimeMs, + (item) => !(scoped && item.inMilestone === false), + ); + + assert.equal(winners.get('shared'), liveInMilestone, + 'the excluded stale directory must never win the key, regardless of its mtime advantage'); + assert.equal(winners.get('shared').label, 'live'); + + // The mirror image, sorted the OTHER way — order-of-iteration must not + // matter once the filter is applied; only inclusion + mtime should. + const winnersReversed = pickRollupWinners( + [staleOutOfMilestone, liveInMilestone], + (item) => item.key, + (item) => item.mtimeMs, + (item) => !(scoped && item.inMilestone === false), + ); + assert.equal(winnersReversed.get('shared'), liveInMilestone); + + // Unscoped (scoped=false): the exclusion never engages, so the newer + // mtime wins normally — pinning that the filter is scoping-conditional, + // not an unconditional "in-milestone always wins" rule. + const unscoped = false; + const winnersUnscoped = pickRollupWinners( + [liveInMilestone, staleOutOfMilestone], + (item) => item.key, + (item) => item.mtimeMs, + (item) => !(unscoped && item.inMilestone === false), + ); + assert.equal(winnersUnscoped.get('shared'), staleOutOfMilestone, + 'when scoping is off, the plain newest-mtime rule applies with no exclusion'); + }); + + // Row 19 — NOT the collision blocker (see Row 18's comment — this does + // not reproduce it and the pre-fix code would pass this too). What this + // DOES cover: in a genuinely SCOPED workstream (roadmap has a Milestone + // column, STATE.md carries `milestone:`), a DISTINCTLY-keyed + // out-of-milestone phase's real verdict must never be written into the + // ledger at all — proving the REAL `inspectWorkstream` read/write path + // threads `scoped`/`inMilestone` into the ledger write gate correctly, end + // to end, for the (much more common) non-colliding case. `1-old` (v1.0) + // and `2-new` (v2.0) are DIFFERENT phase keys, not a shared one. + test('milestone scoping: a distinctly-keyed out-of-milestone phase is never written into the ledger (#2645)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-scoped-ledger' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), 'milestone: v2.0\nstatus: executing\n'); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + '# Roadmap', '', '## Progress', '', + '| Phase | Milestone | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- | --- |', + '| 1. Old | v1.0 | 1/1 | Complete | - |', + '| 2. New | v2.0 | 1/1 | In Progress | - |', + '', + ].join('\n')); + writePhase(wsDir, '1-old', { plans: 1, summaries: 1, verification: 'gaps_found' }); // prior milestone + writePhase(wsDir, '2-new', { plans: 1, summaries: 1, verification: 'gaps_found' }); // current milestone + + const inv = inspectWorkstream(tmpDir, 'ws-2645-scoped-ledger', { active: null }); + assert.ok(inv); + // Guard: confirm scoping is actually active and phase 1 really is + // excluded from the rollup — otherwise this test would not exercise + // anything. + assert.equal(inv.roadmap_phase_count, 1, 'guard: denominator = v2.0 phases only, scoping is active'); + + const ledgerRaw = fs.readFileSync(path.join(wsDir, '.verification-ledger.json'), 'utf-8'); + const ledger = JSON.parse(ledgerRaw); + assert.ok(!('01' in ledger) && !('1' in ledger), + 'the out-of-milestone phase (1-old) must never be written into the ledger, regardless of its key form'); + assert.ok(('02' in ledger) || ('2' in ledger), + 'the in-milestone phase (2-new) must be recorded normally'); + }); +});