diff --git a/.changeset/eager-tigers-zip.md b/.changeset/eager-tigers-zip.md new file mode 100644 index 000000000..056f0032a --- /dev/null +++ b/.changeset/eager-tigers-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3828 +--- +**`windows append`/`waive`/`fixed` no longer silently erase a hand-edited ledger table** — `.planning/WINDOWS.md` renders its table from the fenced JSON that is its source of truth, and every write regenerated that table without ever checking the two still agreed. A hand-edited cell was reverted and a table-only row vanished entirely, both at exit 0 with nothing on stdout. The write is now refused with a `windows_ledger_table_drift` error naming the offending row ids, and the file is left untouched. (#3689) diff --git a/.changeset/sturdy-eagles-leap.md b/.changeset/sturdy-eagles-leap.md new file mode 100644 index 000000000..cd9270a4d --- /dev/null +++ b/.changeset/sturdy-eagles-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3828 +--- +**Trailing prose below the ledger's JSON block is no longer destroyed when that prose contains its own fenced JSON array** — `writeLedgerAtomic` located the block to preserve prose after by passing the POST-mutation entry count as its disambiguation hint, which can never match the pre-image's own count. The lookup fell back to the last array-shaped fenced block in the file, so an operator's notes containing a ```json array bound the preservation to the wrong fence and everything above it was dropped on the next write — the exact loss the preservation exists to prevent. (#3689) diff --git a/CONTEXT.md b/CONTEXT.md index 662b027da..9c5c89b9b 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -534,7 +534,7 @@ The producer half of the async external-job contract (#1164, part of #1105). Def Default-off Capability (`capabilities/live-dom-uat/capability.json`, `role: feature`, `tier: full`, `runtimeCompat.supported: ["*"]`, `activationKey: workflow.live_dom_uat`) that confines browser MCP reach to ONE purpose-built agent (#2856). Owns the boolean key `workflow.live_dom_uat` (default `false`), the agent `gsd-dom-verifier` (`tools:` carries `mcp__chrome-devtools__*` + `mcp__claude-in-chrome__*`, and deliberately NO `Bash` and NO `mcp__playwright__*`), and one `step` hook at `execute:wave:post` (`ref.agent`, `fragment: fragments/execute-wave-post.md`, `produces: DOM-VERIFY.md`, `consumes: PLAN.md`, `when: workflow.live_dom_uat`, `onError: skip`) — additive per `RULESET.CAPABILITY.step-additive-gate-blocks`; `gates: []`, it can never halt a wave. **`agents/gsd-executor.md` is NOT widened, in any configuration** — the shape the report proposed and triage refused: for a first-party agent the static `tools:` list is the only control that exists (ADR-1244 D2 forbids a capability granting tools to one; ADR-857 D4's `contribution` injects prose, never permissions; there is no per-dispatch tool override), and ADR-1244 D5's *"there is no sandbox"* governs installed capabilities, not what a spawned subagent does with a granted tool. Containment is TWO independent fail-closed gates: `isCapabilityActive` renders a hook only on `state.active === true` (an installed-but-config-disabled capability renders nothing), plus the step's own `when`. Tool presence alone never activates it — a browser MCP configured for unrelated work is the exact case the default-off key exists for. The orchestrator half extends `gsd-core/workflows/verify-work/steps/automated-ui-verification.md` with a `` block naming both new families AND the key; **the pre-existing `mcp__playwright__*` branch keeps its prior gating (presence + `state:ui-phase-active`) and stays OUTSIDE that block** — pulling it behind a default-off key would silently remove working behavior from every current Playwright-MCP user on upgrade (Hyrum). The glob list is a SHARED CONSTANT across two surfaces (agent frontmatter + workflow block), so `tests/live-dom-uat.test.cjs` carries a parity assertion per `DEFECT.GENERATIVE-FIX-DIVERGENCE`. Landing it also closed a host gap: `execute:wave:post` dispatched only `contribution` + `gate`, so ANY registered `step` was declared and silently never run — `execute-phase.md` step 5.75 now dispatches every `kind == "step"` per `gsd-core/references/loop-hook-dispatch.md`, the same single-kind hand-roll that document names. **Browser-profile lock is tolerated, never coordinated**: `chrome-devtools-mcp` holds an exclusive lock on `$HOME/.cache/chrome-devtools-mcp/chrome-profile` and `--isolated` is a flag on the OPERATOR's own MCP-server registration that GSD neither launches nor parameterizes, so the verifier reports `could_not_look`/`profile_locked`, names the flag, and stops — no retry, no wait, no lease manager over a resource GSD does not own. `DOM-VERIFY.md` frontmatter is scalars only with a closed reason enum (`ok | no_criteria | no_browser_mcp | profile_locked | target_unreachable`) under a closed `outcome` (`verified | nothing_to_report | could_not_look`); **`nothing_to_report` and `could_not_look` are never conflated** — a report claiming "no issues" that never opened a browser is the ambiguous-run-notes defect the issue was filed about. Test seam: `tests/live-dom-uat.test.cjs`. Docs: `docs/how-to/enable-live-dom-verification.md`, `docs/explanation/live-dom-uat-capability.md`. ### Broken Windows Ledger -The enforced cross-phase defect register operationalizing GSD's no-defer discipline as a tracked artifact (#1950). Markdown file at `.planning/WINDOWS.md` (project-level, cross-phase) with YAML frontmatter carrying scalar counts (`schema_version`, `open_count`, `waived_count`, `fixed_count`, `total_count`, `last_updated`) for the FAST path the gate reads via jq without parsing JSON, plus a JSON code block as the AUTHORITATIVE entries source; the two cross-check and fail closed on drift. Each entry: `{ id, kind, phase, file, line, description, status, reason, recorded_at, resolved_at }`; kinds are closed (`stub | todo | fixme | skipped-test | lint-warning | unmet-truth | unrun-verify | deviation`); statuses are closed (`open | waived | fixed`). The `broken-windows` Capability (`capabilities/broken-windows/capability.json`) registers one `ship:pre` gate with predicate `artifact-frontmatter-equals WINDOWS.md open_count == 0`; federated config key `workflow.windows_enforce` (default `false` — opt-in enforcement, tracking-only by default so a project can adopt the ledger before turning the gate on). Population is best-effort and never blocks execution: `agents/gsd-executor.md` appends stubs/skipped-tests/unrun-verifies via `gsd_run windows append` after writing SUMMARY.md. Source of truth: `src/broken-windows.cts` → `gsd-core/bin/lib/broken-windows.cjs` (pure `parseLedger`/`renderLedger`/`appendWindow`/`markWaived`/`markFixed` + I/O `cmdWindowsStatus`/`Append`/`Waive`/`MarkFixed`); CLI surface `gsd-tools windows status|append|waive|fixed`. Ship gate enforcement is a `capId == "broken-windows"` named specialization inside `gsd-core/workflows/ship.md` preflight's generic `kind == "gate"` dispatch loop (sibling to the `security` specialization; #3559 made that loop generic, so every OTHER capability's `ship:pre` gate is now evaluated through `gsd_run check predicate` instead of being resolved and silently dropped, while these two keep their bespoke fail-closed reads and are each visited exactly once); it reads `gsd_run windows status --raw` and fails closed on a non-zero/non-numeric `open_count` (an unparseable ledger is itself a broken window). `/gsd:progress` surfaces the open+waived count. The ledger is optional and backward-compatible: a project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly, and with `workflow.windows_enforce=false` (the default) ship never blocks on it. Frozen `REASON` enum: `WINDOWS_LEDGER_MISSING | WINDOWS_LEDGER_MALFORMED | WINDOWS_ID_NOT_FOUND | WINDOWS_ALREADY_RESOLVED | WINDOWS_WAIVE_REASON_EMPTY | WINDOWS_INVALID_KIND | WINDOWS_INVALID_FILE | WINDOWS_INVALID_ID | WINDOWS_APPEND_MISSING_FIELD | WINDOWS_USAGE | WINDOWS_OK` — surfaced through `--json-errors` for typed test assertions. Test seam: `tests/broken-windows.test.cjs`. Origin: *The Pragmatic Programmer* Topic 3 (Hunt & Thomas — software transplant of Wilson & Kelling's broken-windows metaphor) plus Cunningham's debt metaphor (decay accrues interest ⇒ accounting, not just habit). +The enforced cross-phase defect register operationalizing GSD's no-defer discipline as a tracked artifact (#1950). Markdown file at `.planning/WINDOWS.md` (project-level, cross-phase) with YAML frontmatter carrying scalar counts (`schema_version`, `open_count`, `waived_count`, `fixed_count`, `total_count`, `last_updated`) for the FAST path the gate reads via jq without parsing JSON, plus a JSON code block as the AUTHORITATIVE entries source; the two cross-check and fail closed on drift. The rendered markdown table is a THIRD projection of that same source and is cross-checked the same way, but at the WRITE seam rather than the read seam (#3689): `writeLedgerAtomic` compares the on-disk table against `renderTable()` and refuses with `WINDOWS_LEDGER_TABLE_DRIFT` before writing, so a hand-edited cell is never silently reverted and a table-only row is never silently erased. Deliberately NOT enforced in `parseLedger`: hardening the read would break `windows status` and the ship gate on exactly the ledgers an operator needs to inspect. Each entry: `{ id, kind, phase, file, line, description, status, reason, recorded_at, resolved_at }`; kinds are closed (`stub | todo | fixme | skipped-test | lint-warning | unmet-truth | unrun-verify | deviation`); statuses are closed (`open | waived | fixed`). The `broken-windows` Capability (`capabilities/broken-windows/capability.json`) registers one `ship:pre` gate with predicate `artifact-frontmatter-equals WINDOWS.md open_count == 0`; federated config key `workflow.windows_enforce` (default `false` — opt-in enforcement, tracking-only by default so a project can adopt the ledger before turning the gate on). Population is best-effort and never blocks execution: `agents/gsd-executor.md` appends stubs/skipped-tests/unrun-verifies via `gsd_run windows append` after writing SUMMARY.md. Source of truth: `src/broken-windows.cts` → `gsd-core/bin/lib/broken-windows.cjs` (pure `parseLedger`/`renderLedger`/`appendWindow`/`markWaived`/`markFixed` + I/O `cmdWindowsStatus`/`Append`/`Waive`/`MarkFixed`); CLI surface `gsd-tools windows status|append|waive|fixed`. Ship gate enforcement is a `capId == "broken-windows"` named specialization inside `gsd-core/workflows/ship.md` preflight's generic `kind == "gate"` dispatch loop (sibling to the `security` specialization; #3559 made that loop generic, so every OTHER capability's `ship:pre` gate is now evaluated through `gsd_run check predicate` instead of being resolved and silently dropped, while these two keep their bespoke fail-closed reads and are each visited exactly once); it reads `gsd_run windows status --raw` and fails closed on a non-zero/non-numeric `open_count` (an unparseable ledger is itself a broken window). `/gsd:progress` surfaces the open+waived count. The ledger is optional and backward-compatible: a project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly, and with `workflow.windows_enforce=false` (the default) ship never blocks on it. Frozen `REASON` enum: `WINDOWS_LEDGER_MISSING | WINDOWS_LEDGER_MALFORMED | WINDOWS_LEDGER_TABLE_DRIFT | WINDOWS_ID_NOT_FOUND | WINDOWS_ALREADY_RESOLVED | WINDOWS_WAIVE_REASON_EMPTY | WINDOWS_INVALID_KIND | WINDOWS_INVALID_FILE | WINDOWS_INVALID_ID | WINDOWS_APPEND_MISSING_FIELD | WINDOWS_USAGE | WINDOWS_OK` — surfaced through `--json-errors` for typed test assertions. Test seam: `tests/broken-windows.test.cjs`. Origin: *The Pragmatic Programmer* Topic 3 (Hunt & Thomas — software transplant of Wilson & Kelling's broken-windows metaphor) plus Cunningham's debt metaphor (decay accrues interest ⇒ accounting, not just habit). ### Emitted Artifact Provenance Cross-seam principle (ADR-2719, epic #2719): a committed artifact that is a pure function of the source tree is not reviewable state — it is derived state wearing a review costume, and it must be *attributable* rather than *pinned*. Concept, not a Module: it ships nothing, so it takes no `Module` suffix (follows the `### Resolution Provenance` precedent). Scope is the emitted-artifact family named by `RULESET.EMITTED_ATTRIBUTION`. The principle: every emitted path whose hash moved between `next` HEAD and PR HEAD must be attributable — through a declarative provenance table — to a path the pull request actually changed; unattributable deltas are a hard failure that *names them* rather than an anomaly a reviewer must notice inside 7,500 lines of hex. Totality is enforced, so an emitted path matching no rule fails loudly instead of passing through unattributed. The escape hatch is a committed acknowledgment — a per-PR fragment under `tests/emitted-drift-acks/` (#2914; the legacy single `tests/emitted-drift-ack.json` is still read and unioned in for pre-#2914 branches, with a duplicate key across two sources a hard, loudly-reported error rather than silent last-wins), deliberately not a flag or env var — a fragment appears in the changed-files list ONLY when something rippled unexpectedly, so adding one IS the alarm, whereas today 100% of emitted-byte changes touch fixtures and touching them signals nothing. Fragments exist because the single legacy file, rewritten wholesale by every PR needing an ack, was a guaranteed merge-conflict cell between any two such PRs (5 of 6 conflicting PRs in one open queue collided on it and nothing else) — the same shape `.changeset/` already solves the same way. Fragments end the FILE conflict but not the KEY conflict: two sources may never name the same path, so a fully-spent fragment left on `next` still walls off every path it owns until it is swept (#3078; see `RULESET.EMITTED_ATTRIBUTION`). The same differential machine carries the size ratchet: growth is reported with exact byte deltas and needs the same acknowledgment, so anti-creep survives without pinning a number. Distinguish from the absolute check that remains: `tests/fixtures/install-tree/*.json` stays committed and normally-merged (ADR-2719 §7) because "the installer stopped shipping X" must fail with no attribution reasoning involved. Delivery was phased — #2721 naming + interim merge relief, #2722 provenance table + totality guard, #2723 differential check dual-run beside `golden-install-parity.test.cjs`, #2724 cutover (COMPLETE: the dual-run window observed agreement on real PRs after fixing #2750/#2760, and the golden fixtures/test/generator/merge-driver bridge are now deleted; the differential is the sole gate). The table LANDED in #2722 as `tests/helpers/emitted-provenance.cjs` (19 rules, guarded by `tests/emitted-provenance.test.cjs`); it maps emitted path → repo source and is TOTAL over EVERY emitted path in all 19 manifests — exactly one rule per path, with zero-match, two-match, AND dead-rule (a rule matching nothing) all hard failures, so table rot is loud in both directions. Deliberately NO path/family counts are recorded here: those move with every shipped-content edit, and a hand-maintained number in glossary canon is the exact silent-drift failure this whole seam exists to end. The guard recomputes them from the fixtures on every run — read them from a failure message, never from prose. What IS stable is the rule count, which changes only when a new emitted family or host appears. Note the surface is materially wider than #2722 estimated from `claude.json` alone (its "13 families / 15-20 rules" was a single-runtime sample; the 19-manifest surface spans runtime-specific roots — `.agents/`, `.kimi/hooks/`, `command/`, `agents/subagents/`, `.clinerules/`, `plugins/`, `extensions/`, `.gsd/`, the hermes `skills/gsd/` category and the #69 nested `skills//skills//` layout). Two design invariants carry forward to #2723: emitted SHAPES are hard-coded (deriving them from the installer would make the guard tautological — it would follow any installer change silently), while source PATHS may read a first-party descriptor where that descriptor is the sole declaration (`hostBehaviors.nativePlugin.source`); and attribution is keyed on `(rel, runtime)`, never `rel` alone, because one emitted path has different sources per host (`plugins/gsd-core.js` ← `.opencode/` vs `.kilo/`). Emitted skills attribute to `commands/gsd/*.md`, NEVER the repo `skills/` dir — that dir is itself generated from `commands/gsd` by `scripts/gen-plugin-skills.cjs`, so attributing to it is false attribution that still passes totality. Totality does NOT catch a rule pointing at the WRONG source (the recorded residual); the guard against that is the companion assertion that every attributed source EXISTS in the repo, which caught three real cases while the table was built (Copilot's `.agent.md` rename, Kimi's code-literal `agents/gsd.{yaml,md}` root agent, and Copilot's `hooks/gsd-session.json`). A `sources` entry ending in `/` is a PREFIX, not a file — and prefix matching is SEGMENT-AWARE, so a source of `agents/` must not attribute `agentsfoo/x.md`. The differential check LANDED in #2723 as `tests/helpers/emitted-diff.cjs` (the conservation law, a PURE function — no fs/git/installer/clock) + `tests/helpers/emitted-baseline.cjs` (baseline resolution), guarded by `tests/emitted-attribution.test.cjs`. It ran DUAL beside `golden-install-parity.test.cjs` through the #2723 dual-run window with both green and fixtures untouched; #2724 deleted the golden fixtures/test/generator and the check is now the sole gate. Purity is deliberate and load-bearing: the naive one-big-integration-test shape would need ~38 installer spawns per assertion, so the four failing-first criteria would not in practice get written — which is exactly how a phase ships promised-but-not-built. Buckets are CONSERVED: every moved emitted path lands in exactly one of `attributed | unattributable | acked` (property-tested), and a path the provenance table cannot resolve surfaces as an ERROR rather than a silent skip. Four asymmetries worth knowing: an ADDED emitted key is a ripple too (not just modified ones); `synthesized` paths are exempt but `code-derived` ones are NOT (that is why Phase 2 refused to mark them exempt — exempt means permanently blind); SHRINKAGE needs no ack while growth does (gating shrinkage would punish what the ratchet wants); and a STALE ack is a hard failure — but ONLY for an ack THIS diff wrote or reworded (#2789). An ack is SCOPED TO THE DIFF THAT INTRODUCED IT: `diffEmitted` takes the document at the base ref (`baseAck`, read by `readAckFileAtRef`) alongside the working-tree one, and an entry already present at the base is SPENT — its ripple is absorbed into the base, so it can no longer clear a delta and is never reported stale (surfaced as `spentAcks`, informational, gating nothing). Before #2789 the ack set was the one ABSOLUTE input to an otherwise base-relative machine — `baseline` vs `current`, `changedPaths` from `git diff base...HEAD` — and that mismatch made a MERGED ack indistinguishable from one that never explained anything, since `staleAcks` asks only "did a delta consume you?": merging an ack the PR lane had already accepted reddened `next` and every PR branching off it (#2768). Making spent entries inert is also what finally closes the pre-clearing hazard the original design NAMED but could not prevent — a leftover ack used to silently clear the next ripple on its path; now that ripple must be explained on its own terms, and a reworded reason is how a contributor re-arms an ack deliberately. `baseAck` is REQUIRED once an ack DECLARES ENTRIES (omission is an error, never a silent "inherit nothing", same discipline as `changedPaths`; an entry is the only thing that can be misclassified, so an empty-but-legal document needs no base side). Absent AT THE REF returns null — the healthy steady state — but every other read failure THROWS, and that asymmetry is load-bearing in the direction that is easy to invert: returning null looks armed because every entry stays LIVE, yet a live entry's defining power is that it CONSUMES a delta, so null is armed on the staleness axis and DISARMED on the consumption axis — a genuinely new unexplained ripple on a path carrying an already-merged ack would come back `acked` instead of `unattributable`, silently restoring the whole pre-#2789 gate. `git show` cannot tell absence from fault (both say "does not exist in"), so absence is established with `ls-tree`. Re-arming a spent ack is legitimate and deliberate, but it costs ACTUAL PROSE: the comparison collapses internal whitespace and ignores `runtime`, because a doubled space or a decorative field would otherwise re-arm an ack whose recorded justification still describes the PREVIOUS ripple, showing a reviewer nothing new in the diff. Because a corrupt document ON THE BASE is expensive (it reds every PR carrying an ack until repaired), `scripts/lint-emitted-drift-ack.cjs` runs in `lint:ci` and refuses the merge before one can land — invalid JSON, a non-object, a bad version, a reasonless entry, or a present-but-entryless/`null` document. It is deliberately STANDALONE rather than importing `parseAck` (`scripts/` ships in the npm package and `tests/` does not, so the require would be MODULE_NOT_FOUND once published); the duplication is bounded by a parity test that runs both surfaces over one corpus and fails on any disagreement about schema validity. The two are MEANT to differ on exactly one axis: an entryless or `null` document is legal to PARSE (it is the gate's own absent-equals-no-acks sentinel) and still refused for COMMIT. Deadlock is separately foreclosed at the call site — a tree carrying no ack never reads the base at all, so the PR that DELETES a corrupt file still lands. Each ack source — a fragment under `tests/emitted-drift-acks/`, or the legacy `tests/emitted-drift-ack.json` (#2914; both read and UNIONED via `mergeAckSources`/`readAckSources`/`readAckSourcesAtRef` in `tests/helpers/emitted-diff.cjs` / `emitted-runtime.cjs`, a duplicate key across sources a hard error) — follows the same rule: absent = no acks; a LIVE entry is the alarm, a spent one is inert cruft; requires a non-empty `reason` per path — "name them and say why" is the contract, and a document that parses but is not an object is rejected rather than read as "no acks", which would silently disarm the gate. Baseline is CACHED not committed, keyed on the `next` sha; a stale key is REFUSED, never used — absence fails loudly and gets fixed, whereas staleness produces a confident wrong answer. An explicitly-pointed-at (`GSD_EMITTED_BASELINE`) stale baseline is a hard stop, while a stale CACHE falls through to the in-job build. No baseline-unavailable path may `return` (in `node:test` that is a PASS, not a skip — ADR-2719 §6). Supersedes ADR-2264 §2–§4 and its Amendment; ADR-2264 Phase 1 (`buildParityManifest` and the exclusion constants in `tests/helpers/install-shared.cjs`) is retained and depended upon. diff --git a/src/broken-windows.cts b/src/broken-windows.cts index d0a9d5793..e1162154e 100644 --- a/src/broken-windows.cts +++ b/src/broken-windows.cts @@ -70,6 +70,10 @@ export const REASON = Object.freeze({ WINDOWS_INVALID_ID: 'windows_invalid_id', WINDOWS_APPEND_MISSING_FIELD: 'windows_append_missing_field', WINDOWS_USAGE: 'windows_usage', + // #3689: the rendered markdown table disagreed with the fenced JSON (the + // sole source of truth) at the pre-write seam — refuse rather than silently + // reconcile by overwriting the operator's hand-edit or dropping a row. + WINDOWS_LEDGER_TABLE_DRIFT: 'windows_ledger_table_drift', }); /** Allowed window kinds. Aligned with the issue's enumerated sources. */ @@ -379,6 +383,15 @@ const JSON_FENCE_OPEN = '````json'; const JSON_FENCE_CLOSE = '````'; const FORBIDDEN_BACKTICK_RUN = '````'; +/** + * #3689: the ledger table's fixed header row literal. `renderTable` emits it + * on both the empty and non-empty branches; `extractTableRegion` anchors on + * it to bound the table region. Hoisted to one constant so the two surfaces + * cannot drift (see "Generative Fix Divergence" — CONTRIBUTING.md). + */ +const TABLE_HEADER_LINE = + '| id | phase | kind | file | line | description | status | reason | recorded_at | resolved_at |'; + // Reader-side fence tolerance (#3657): CommonMark formatters (Prettier et al.) // normalize the written 4-backtick fence down to the shortest legal width (3) // whenever the block body holds no backtick run — and a canonical-JSON ledger @@ -686,16 +699,16 @@ export function renderLedger(ledger: Ledger): string { return [fm, header, table, '', jsonBlock].join('\n'); } -function renderTable(entries: WindowEntry[]): string { +export function renderTable(entries: WindowEntry[]): string { if (entries.length === 0) { return [ - '| id | phase | kind | file | line | description | status | reason | recorded_at | resolved_at |', + TABLE_HEADER_LINE, '|----|-------|------|------|------|-------------|--------|--------|-------------|-------------|', '| _(none)_ | | | | | _No windows recorded._ | | | | |', ].join('\n'); } const rows = [ - '| id | phase | kind | file | line | description | status | reason | recorded_at | resolved_at |', + TABLE_HEADER_LINE, '|----|-------|------|------|------|-------------|--------|--------|-------------|-------------|', ]; for (const e of entries) { @@ -720,6 +733,115 @@ function renderTable(entries: WindowEntry[]): string { return rows.join('\n'); } +/** + * #3689: extract the exact markdown table region a rendered ledger emits — + * the text `renderTable` produced, byte-for-byte — from a raw ledger file. + * Used by `writeLedgerAtomic`'s drift guard to compare the on-disk table + * against `renderTable()` without a table parser. + * + * Locates the JSON block with the same tolerant `locateJsonBlock` helper the + * rest of the module uses (#3657), so a formatter-normalized 3-backtick + * fence still resolves. Everything before the opening fence line, with + * trailing blank lines dropped, is the candidate region. + * + * #3689: the region is bounded by finding the LAST occurrence of the fixed + * `TABLE_HEADER_LINE` literal (anchored at a line start) within that + * candidate text, then taking everything from there through its end — NOT + * by scanning backward for a contiguous run of `|`-prefixed lines. A `|` + * prefix scan cannot bound the region: `validateDescription` rejects only + * empty strings and 4-backtick runs, so a description may contain a raw + * `\n`, and `renderTable`'s `cell()` escapes `\` and `|` but not newlines. + * Such a description renders a row that physically spans multiple file + * lines, and the continuation line does not start with `|` — a prefix scan + * either truncates the table or, when the row's tail is the last pre-fence + * line, returns null immediately, bricking every subsequent write with + * `WINDOWS_LEDGER_TABLE_DRIFT` on a ledger nobody hand-edited. Anchoring on + * the header instead includes any such row whole, so `renderTable` + * regenerates byte-identical text for it and the drift comparison passes. + * + * Returns null when the JSON block cannot be located, or no header line is + * present. + * + * `expectedTotal` (#3689 review finding 2) is threaded straight into + * `locateJsonBlock` so callers with trailing prose can disambiguate the real + * ledger block from an unrelated fenced JSON array a user pasted below the + * closing fence — without it, `locateJsonBlock`'s no-hint fallback picks the + * LATEST array-shaped span, which is the prose block, not the ledger, and + * every drift comparison then binds to the wrong table/JSON pairing. + */ +export function extractTableRegion(raw: string, expectedTotal?: number): string | null { + const span = locateJsonBlock(raw, expectedTotal); + if (!span.ok) return null; + // bodyStart sits right before the newline (or CR) ending the opening fence + // line; walk back to the start of that line. + const fenceLineStart = raw.lastIndexOf('\n', span.span.bodyStart - 1) + 1; + const before = raw.slice(0, fenceLineStart).replace(/\r\n/g, '\n'); + let trimmedEnd = before.length; + while (trimmedEnd > 0 && before[trimmedEnd - 1] === '\n') { + trimmedEnd--; + } + const candidate = before.slice(0, trimmedEnd); + // Find the LAST occurrence of TABLE_HEADER_LINE anchored at a line start — + // a plain string scan rather than a regex, since the module is a leaf + // (imports only node:fs/node:path) and cannot pull in the shared + // escapeRegex() helper for a one-off fixed-literal search. + let headerIndex = -1; + let searchFrom = candidate.length; + for (;;) { + const idx = candidate.lastIndexOf(TABLE_HEADER_LINE, searchFrom); + if (idx === -1) break; + const atLineStart = idx === 0 || candidate[idx - 1] === '\n'; + const atLineEnd = + idx + TABLE_HEADER_LINE.length === candidate.length || + candidate[idx + TABLE_HEADER_LINE.length] === '\n'; + if (atLineStart && atLineEnd) { + headerIndex = idx; + break; + } + // #3689: lastIndexOf clamps a negative position into [0, length] per + // spec, so `searchFrom = -1` would re-search from 0 and re-find the same + // rejected match at idx===0 forever. Stop explicitly once there is + // nowhere left to search — this makes the bound strictly decrease each + // iteration, so the loop terminates within candidate.length steps. + if (idx === 0) break; + searchFrom = idx - 1; + } + if (headerIndex === -1) return null; + return candidate.slice(headerIndex); +} + +/** + * #3689: diff two `renderTable` outputs by row id (the first cell of each + * data row), skipping the header + separator lines (always exactly two). + * A row whose line text differs between the two tables, or that is present + * in only one of them, contributes its id to the result — this is what lets + * the drift-guard error message name the specific drifted/table-only row(s) + * rather than just saying "the table disagrees". + */ +function diffTableRowIds(expectedTable: string, actualTable: string): string[] { + const rowId = (line: string): string => (line.split('|')[1] ?? '').trim(); + const dataRows = (table: string): Map => { + const lines = table.split('\n'); + const map = new Map(); + for (let i = 2; i < lines.length; i++) { + const line = lines[i]; + if (!line.startsWith('|')) continue; + map.set(rowId(line), line); + } + return map; + }; + const expectedRows = dataRows(expectedTable); + const actualRows = dataRows(actualTable); + const ids = new Set(); + for (const [id, line] of expectedRows) { + if (actualRows.get(id) !== line) ids.add(id); + } + for (const id of actualRows.keys()) { + if (!expectedRows.has(id)) ids.add(id); + } + return Array.from(ids).sort(); +} + // ─── I/O entry points ────────────────────────────────────────────────────── function ledgerPath(cwd: string): string { @@ -805,21 +927,125 @@ function writeLedgerAtomic(cwd: string, ledger: Ledger): void { // trailing prose that users may have written below the closing fence. // Without this, every append/waive/fixed silently destroys that prose. let trailingProse = ''; + // #3689: read the pre-image once into `existing` outside the catch, rather + // than doing every subsequent step inside a bare try/catch, so that a + // WindowsError thrown by the drift guard below propagates instead of being + // swallowed by the ENOENT handler meant only for "no ledger yet". + let existing: string | null = null; try { - const existing = fs.readFileSync(p, 'utf8'); + existing = fs.readFileSync(p, 'utf8'); + } catch (e: unknown) { + // #1950-H2 / #3689: ENOENT is the only "no ledger yet" case — mirror + // readLedgerOrNull's discipline exactly. A bare catch here would let + // EACCES/EIO/ENOTDIR/etc. fall through as "no pre-image", silently + // skipping BOTH the #2893 prose preservation and the drift guard below + // and proceeding to overwrite an unreadable file — a guard bypassable by + // making the pre-image unreadable is not a guard. + const code = (e && typeof e === 'object' && 'code' in e) + ? String((e as { code?: unknown }).code) + : ''; + if (code !== 'ENOENT') { + throw new WindowsError( + REASON.WINDOWS_LEDGER_MALFORMED, + `Could not read ledger at ${p} (${code || 'unknown fs error'}): ${(e as Error).message}.`, + ); + } + // File doesn't exist yet (first write) — no prose to preserve, and + // nothing on disk to disagree with, so the drift guard below is skipped. + } + if (existing !== null) { + // #3689 review finding 2 / #3689 bug discovery: both the #2893 prose + // span AND the drift guard below must disambiguate `locateJsonBlock` + // against the SAME pre-image ledger block, so this is computed ONCE, + // hoisted above both uses. expectedTotal MUST be derived from the + // PRE-IMAGE's own frontmatter (never `ledger.total_count`, which is + // already post-mutation — e.g. N+1 on an append): #2893 exists precisely + // because operators may paste prose below the closing fence, and that + // prose can itself contain a fenced JSON array of a different length. + // Passing the post-mutation total here (as a since-fixed #3689 review + // pass once did for the guard alone) makes locateJsonBlock's expectedTotal + // scan find nothing against the pre-image — no span has N+1 entries yet — + // so it silently falls through to the no-hint fallback, which binds to + // the LATEST array-shaped span: the prose block, not the ledger. Left + // unfixed, that means the #2893 prose-preservation span itself would + // resolve to the prose fence's `afterClose`, silently dropping + // everything between the real ledger block and the prose block — + // including the operator's own prose ABOVE that array — on every + // append. This is exactly the failure #2893 was written to prevent, + // reintroduced through the disambiguation hint; it is caught here by + // deriving the hint from the pre-image, not the post-mutation ledger, + // for BOTH call sites below. If the pre-image frontmatter cannot be + // parsed unambiguously, that is itself the ambiguous case — fail closed + // rather than falling back to the no-hint scan. + let preImageExpectedTotal: number; + try { + const preFm = parseFrontmatterStrict(existing); + if (typeof preFm.total_count !== 'number' || !Number.isInteger(preFm.total_count)) { + throw new WindowsError( + REASON.WINDOWS_LEDGER_MALFORMED, + `Ledger frontmatter total_count in ${p} is not an integer; refusing to write — ` + + 'the ledger JSON block cannot be identified unambiguously.', + ); + } + preImageExpectedTotal = preFm.total_count; + } catch (e) { + if (e instanceof WindowsError) throw e; + throw new WindowsError( + REASON.WINDOWS_LEDGER_MALFORMED, + `Ledger frontmatter in ${p} could not be parsed (${(e as Error).message}); refusing ` + + 'to write — the ledger JSON block cannot be identified unambiguously.', + ); + } + // #2893: search for the CLOSING fence starting AFTER the opening fence. // The span is located with the same tolerant + disambiguated fence rules // parseJsonBlock uses (#3657), so a formatter-normalized 3-backtick ledger // keeps its prose too — a literal-width search here would find no block - // and silently drop everything below the ledger on the next write. - const span = locateJsonBlock(existing, ledger.total_count); + // and silently drop everything below the ledger on the next write. The + // hint passed here is `preImageExpectedTotal` (pre-image derived, see + // above) — NOT `ledger.total_count` — so this binds to the same span the + // drift guard below does. + const span = locateJsonBlock(existing, preImageExpectedTotal); if (span.ok) { const afterFence = existing.slice(span.span.afterClose); // Drop leading newlines; keep the rest as prose. trailingProse = afterFence.replace(/^(?:\r?\n)+/, ''); } - } catch { - // File doesn't exist yet (first write) — no prose to preserve. + + // #3689 review finding 2: refuse the write if the on-disk table has + // drifted from the on-disk JSON — the source of truth — BEFORE anything + // is regenerated. Baseline is the ON-DISK entries, not `ledger` (already + // the post-mutation state: an appended entry or a changed status); + // comparing against `ledger` would report drift on every legitimate + // write. + const onDiskEntries = parseJsonBlock(existing, preImageExpectedTotal); + const expectedTable = renderTable(onDiskEntries); + const actualTable = extractTableRegion(existing, preImageExpectedTotal); + if (actualTable === null) { + throw new WindowsError( + REASON.WINDOWS_LEDGER_TABLE_DRIFT, + `Ledger table region could not be located in ${p}; refusing to write. Edit the ` + + 'fenced JSON block directly — the sole source of truth — or delete the corrupted ' + + 'table region and let gsd-tools regenerate it; never hand-edit the rendered table.', + ); + } + if (actualTable !== expectedTable) { + const driftedIds = diffTableRowIds(expectedTable, actualTable); + // #3689 review finding 3: a header/separator-only drift (e.g. a + // hand-edited column name or mangled separator) produces no data-row + // diffs, so driftedIds is empty — naming nothing would read "...for + // row id(s): .". Say what actually differs instead. + const driftDescription = driftedIds.length > 0 + ? `for row id(s): ${driftedIds.join(', ')}` + : "in its header or separator row (no data row differs from the expected rendering)"; + throw new WindowsError( + REASON.WINDOWS_LEDGER_TABLE_DRIFT, + `Ledger table in ${p} disagrees with the fenced JSON entries (the sole source of ` + + `truth) ${driftDescription}. Edit the fenced JSON block ` + + 'directly, or discard the table edit and re-run the command so gsd-tools ' + + 'regenerates the table; never hand-edit the rendered table.', + ); + } } const rendered = renderLedger(ledger); diff --git a/tests/broken-windows.test.cjs b/tests/broken-windows.test.cjs index 56ce91513..0924d3dc5 100644 --- a/tests/broken-windows.test.cjs +++ b/tests/broken-windows.test.cjs @@ -28,6 +28,7 @@ const path = require('node:path'); const { createTempDir, cleanup, runGsdTools } = require('./helpers.cjs'); const fc = require('./helpers/fast-check-setup.cjs'); +const brokenWindowsLib = require('../gsd-core/bin/lib/broken-windows.cjs'); const { REASON, WindowsError, @@ -39,7 +40,8 @@ const { markWaived, markFixed, openCount, -} = require('../gsd-core/bin/lib/broken-windows.cjs'); + cmdWindowsAppend, +} = brokenWindowsLib; // --------------------------------------------------------------------------- // Fixtures @@ -1053,3 +1055,578 @@ describe('#3116: parseLedger handles CRLF ledgers', () => { assert.deepEqual(crlfParsed, lfParsed); }); }); + +// --------------------------------------------------------------------------- +// #3689: writeLedgerAtomic table-vs-JSON drift guard +// +// `.planning/WINDOWS.md`'s markdown table is a rendered VIEW of the JSON +// fence (the sole source of truth). writeLedgerAtomic re-reads the file only +// to preserve trailing prose (#2893) and then writes renderLedger(ledger) +// unconditionally, with no check that the on-disk table agreed with the JSON +// beforehand — so a hand-edited table cell is silently reverted, and a +// table-only row silently vanishes, on the next append/waive/fixed. See +// .gsd/bug/fix-3689-windows-ledger-table-drift-guard/repro.cjs. +// --------------------------------------------------------------------------- + +describe('#3689: windows ledger table-vs-JSON drift guard', () => { + /** Build a pristine, real-CLI-written two-entry ledger; return its raw text. */ + function seedPristineLedger(t) { + const seedCwd = createTempDir('bw-3689-seed-'); + t.after(() => cleanup(seedCwd)); + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '1', '--description', 'first entry', '--file', 'a/one.sh'], + seedCwd, + ); + assert.ok(r1.success, `seed append 1 failed: ${r1.error || ''}`); + const r2 = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '2', '--description', 'second entry', '--file', 'b/two.sh'], + seedCwd, + ); + assert.ok(r2.success, `seed append 2 failed: ${r2.error || ''}`); + return fs.readFileSync(path.join(seedCwd, '.planning', LEDGER_FILE_NAME), 'utf8'); + } + + /** Index of the line opening the JSON fence (the fenced ```json line), or -1. */ + function jsonFenceLineIndex(lines) { + return lines.findIndex((l) => /^`{3,}json[ \t]*$/.test(l.trim())); + } + + /** Flip a table row's `| from |` cell to `| to |`, touching only the table region. */ + function flipTableStatus(raw, rowId, from, to) { + const lines = raw.split('\n'); + const fenceIdx = jsonFenceLineIndex(lines); + let flipped = false; + const out = lines.map((line, idx) => { + if (flipped || (fenceIdx !== -1 && idx >= fenceIdx)) return line; + const rowRe = new RegExp(`^\\|\\s*${rowId}\\s*\\|`); + if (rowRe.test(line) && line.includes(`| ${from} |`)) { + flipped = true; + return line.replace(`| ${from} |`, `| ${to} |`); + } + return line; + }); + assert.ok(flipped, `must have found row ${rowId} with status "${from}" to flip`); + return out.join('\n'); + } + + /** Insert an extra data row (present only in the table, not the JSON) before the fence. */ + function insertTableOnlyRow(raw, rowLine) { + const lines = raw.split('\n'); + const fenceIdx = jsonFenceLineIndex(lines); + assert.ok(fenceIdx > 0, 'must locate the JSON fence to insert before'); + let insertAt = fenceIdx; + while (insertAt > 0 && lines[insertAt - 1].trim() === '') insertAt -= 1; + lines.splice(insertAt, 0, rowLine); + return lines.join('\n'); + } + + function writeLedgerFile(tmp, content) { + fs.mkdirSync(path.join(tmp, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(tmp, '.planning', LEDGER_FILE_NAME), content, 'utf8'); + } + + function readLedgerFile(tmp) { + return fs.readFileSync(path.join(tmp, '.planning', LEDGER_FILE_NAME), 'utf8'); + } + + test('windows append refuses when the rendered table has drifted from the JSON (#3689)', (t) => { + const pristine = seedPristineLedger(t); + const tmp = createTempDir('bw-3689-drift-append-'); + t.after(() => cleanup(tmp)); + const drifted = flipTableStatus(pristine, 1, 'open', 'fixed'); + writeLedgerFile(tmp, drifted); + const before = readLedgerFile(tmp); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '99', '--description', 'third entry', '--file', 'c/three.sh'], + tmp, + { GSD_JSON_ERRORS: '1' }, + ); + + assert.equal(res.success, false, 'append must refuse on table drift'); + const parsed = JSON.parse(res.error); + assert.equal(parsed.ok, false, `structured error must carry ok:false: ${res.error}`); + // #3689: the typed reason distinguishes table drift from a generic + // WINDOWS_LEDGER_MALFORMED parse failure. String literal (not + // REASON.WINDOWS_LEDGER_TABLE_DRIFT) because that constant does not + // exist on the shipped module today — referencing it would compare + // undefined === undefined and pass vacuously before the fix lands. + assert.equal(parsed.reason, 'windows_ledger_table_drift', `expected typed drift reason, got: ${res.error}`); + assert.match(parsed.message, /\b1\b/, 'failure message must name the drifted row id'); + assert.equal(readLedgerFile(tmp), before, 'the file must be byte-identical to the pre-image after a refusal'); + }); + + test('windows append refuses a table-only row instead of erasing it (#3689)', (t) => { + const pristine = seedPristineLedger(t); + const tmp = createTempDir('bw-3689-tableonly-'); + t.after(() => cleanup(tmp)); + const extraRow = '| 99 | 42 | deviation | z/table-only.sh | - | table only row | open | - | - | - |'; + const withExtraRow = insertTableOnlyRow(pristine, extraRow); + writeLedgerFile(tmp, withExtraRow); + const before = readLedgerFile(tmp); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '7', '--description', 'fourth entry', '--file', 'd/four.sh'], + tmp, + { GSD_JSON_ERRORS: '1' }, + ); + + assert.equal(res.success, false, 'append must refuse rather than silently drop the table-only row'); + const parsed = JSON.parse(res.error); + assert.equal(parsed.ok, false, `structured error must carry ok:false: ${res.error}`); + assert.equal(parsed.reason, 'windows_ledger_table_drift', `expected typed drift reason, got: ${res.error}`); + assert.match(parsed.message, /\b99\b/, 'failure message must name the drifted (table-only) row id'); + assert.ok(readLedgerFile(tmp).includes('table only row'), 'the table-only row must still be present after refusal'); + assert.equal(readLedgerFile(tmp), before, 'the file must be byte-identical to the pre-image after a refusal'); + }); + + test('windows waive refuses on table drift (#3689)', (t) => { + const pristine = seedPristineLedger(t); + const tmp = createTempDir('bw-3689-drift-waive-'); + t.after(() => cleanup(tmp)); + const drifted = flipTableStatus(pristine, 1, 'open', 'fixed'); + writeLedgerFile(tmp, drifted); + const before = readLedgerFile(tmp); + + const res = runGsdTools(['windows', 'waive', '2', 'covered by manual QA'], tmp, { GSD_JSON_ERRORS: '1' }); + + assert.equal(res.success, false, 'waive must refuse on table drift'); + const parsed = JSON.parse(res.error); + assert.equal(parsed.ok, false, `structured error must carry ok:false: ${res.error}`); + assert.equal(parsed.reason, 'windows_ledger_table_drift', `expected typed drift reason, got: ${res.error}`); + assert.match(parsed.message, /\b1\b/, 'failure message must name the drifted row id'); + assert.equal(readLedgerFile(tmp), before, 'the file must be byte-identical to the pre-image after a refusal'); + }); + + test('windows fixed refuses on table drift (#3689)', (t) => { + const pristine = seedPristineLedger(t); + const tmp = createTempDir('bw-3689-drift-fixed-'); + t.after(() => cleanup(tmp)); + const drifted = flipTableStatus(pristine, 1, 'open', 'fixed'); + writeLedgerFile(tmp, drifted); + const before = readLedgerFile(tmp); + + const res = runGsdTools(['windows', 'fixed', '2'], tmp, { GSD_JSON_ERRORS: '1' }); + + assert.equal(res.success, false, 'fixed must refuse on table drift'); + const parsed = JSON.parse(res.error); + assert.equal(parsed.ok, false, `structured error must carry ok:false: ${res.error}`); + assert.equal(parsed.reason, 'windows_ledger_table_drift', `expected typed drift reason, got: ${res.error}`); + assert.match(parsed.message, /\b1\b/, 'failure message must name the drifted row id'); + assert.equal(readLedgerFile(tmp), before, 'the file must be byte-identical to the pre-image after a refusal'); + }); + + test('windows append detects drift on a non-first row (#3689)', (t) => { + const pristine = seedPristineLedger(t); + const tmp = createTempDir('bw-3689-drift-second-row-'); + t.after(() => cleanup(tmp)); + const drifted = flipTableStatus(pristine, 2, 'open', 'fixed'); + writeLedgerFile(tmp, drifted); + const before = readLedgerFile(tmp); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '5', '--description', 'fifth entry', '--file', 'e/five.sh'], + tmp, + { GSD_JSON_ERRORS: '1' }, + ); + + assert.equal(res.success, false, 'append must detect drift on the second data row, not just the first'); + const parsed = JSON.parse(res.error); + assert.equal(parsed.ok, false, `structured error must carry ok:false: ${res.error}`); + assert.equal(parsed.reason, 'windows_ledger_table_drift', `expected typed drift reason, got: ${res.error}`); + assert.match(parsed.message, /\b2\b/, 'failure message must name the drifted row id (2), not just row 1'); + assert.equal(readLedgerFile(tmp), before, 'the file must be byte-identical to the pre-image after a refusal'); + }); + + // --- Anti-tightening / negative-space pins: must stay green before AND after the fix --- + + test('windows append still succeeds when the table agrees with the JSON (#3689)', (t) => { + const tmp = createTempDir('bw-3689-agree-'); + t.after(() => cleanup(tmp)); + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '1', '--description', 'first entry'], + tmp, + ); + assert.ok(r1.success, `seed append failed: ${r1.error || ''}`); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '2', '--description', 'second entry'], + tmp, + ); + assert.equal(res.success, true, `append must succeed on an agreeing table: ${res.error || ''}`); + const obj = JSON.parse(res.output); + assert.equal(obj.entry.id, 2); + assert.equal(obj.ledger.total_count, 2); + }); + + test('windows append still creates the ledger when none exists (#3689)', (t) => { + const tmp = createTempDir('bw-3689-nofile-'); + t.after(() => cleanup(tmp)); + assert.equal(fs.existsSync(path.join(tmp, '.planning', LEDGER_FILE_NAME)), false); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '1', '--description', 'first ever entry'], + tmp, + ); + assert.equal(res.success, true, `append must create the ledger with no pre-image to disagree with: ${res.error || ''}`); + assert.equal(fs.existsSync(path.join(tmp, '.planning', LEDGER_FILE_NAME)), true); + }); + + test('windows append preserves trailing prose when the guard passes (#2893 + #3689)', (t) => { + const tmp = createTempDir('bw-3689-prose-'); + t.after(() => cleanup(tmp)); + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '1', '--description', 'prose carrier'], + tmp, + ); + assert.ok(r1.success, `seed append failed: ${r1.error || ''}`); + const ledgerPath = path.join(tmp, '.planning', LEDGER_FILE_NAME); + fs.writeFileSync(ledgerPath, fs.readFileSync(ledgerPath, 'utf8') + 'Operator notes below the ledger.\n', 'utf8'); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '2', '--description', 'second entry'], + tmp, + ); + assert.equal(res.success, true, `append must succeed when the table agrees: ${res.error || ''}`); + assert.ok( + fs.readFileSync(ledgerPath, 'utf8').includes('Operator notes below the ledger.'), + 'trailing prose must survive an append that passes the drift guard', + ); + }); + + test('windows append tolerates a 3-backtick fence when locating the table (#3657 + #3689)', (t) => { + const tmp = createTempDir('bw-3689-narrowfence-'); + t.after(() => cleanup(tmp)); + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '1', '--description', 'narrowed fence entry'], + tmp, + ); + assert.ok(r1.success, `seed append failed: ${r1.error || ''}`); + const ledgerPath = path.join(tmp, '.planning', LEDGER_FILE_NAME); + fs.writeFileSync( + ledgerPath, + fs.readFileSync(ledgerPath, 'utf8') + .replace(/^````json$/m, '```json') + .replace(/^````$/m, '```'), + 'utf8', + ); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '2', '--description', 'second entry'], + tmp, + ); + assert.equal(res.success, true, `append must tolerate a 3-backtick fence when the table agrees: ${res.error || ''}`); + }); + + test('windows append does not trip the guard on escaped pipes and backslashes (#3689)', (t) => { + const tmp = createTempDir('bw-3689-escaping-'); + t.after(() => cleanup(tmp)); + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '1', + '--description', 'path with \\| separator and | pipe and \\ backslash'], + tmp, + ); + assert.ok(r1.success, `seed append with escaped content failed: ${r1.error || ''}`); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '2', '--description', 'second entry'], + tmp, + ); + assert.equal(res.success, true, `append must not false-positive on escaped pipes/backslashes: ${res.error || ''}`); + }); + + test('windows append tolerates the empty-ledger table rendering (#3689)', (t) => { + const tmp = createTempDir('bw-3689-emptytable-'); + t.after(() => cleanup(tmp)); + writeLedgerFile(tmp, renderLedger(emptyLedger('2026-08-24T00:00:00Z'))); + assert.ok( + readLedgerFile(tmp).includes('_(none)_'), + 'precondition: seeded ledger renders the empty-table placeholder row', + ); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'stub', '--phase', '1', '--description', 'first real entry'], + tmp, + ); + assert.equal(res.success, true, `append must succeed against the empty-ledger placeholder table: ${res.error || ''}`); + assert.equal(JSON.parse(res.output).entry.id, 1); + }); + + test('windows append tolerates trailing prose that itself contains a fenced JSON array (#2893 + #3689)', (t) => { + const pristine = seedPristineLedger(t); + const tmp = createTempDir('bw-3689-prose-jsonarray-'); + t.after(() => cleanup(tmp)); + // The pristine ledger has 2 entries. The trailing prose's fenced JSON + // array below has a DIFFERENT length (3) than the real entries list, so + // a wrong binding (matching the prose block instead of the ledger block) + // is unambiguous: it would make onDiskEntries.length disagree with the + // real 2-entry table, tripping the drift guard on a ledger that never + // drifted. + const withProse = `${pristine}Operator notes below the ledger.\n\n` + + '```json\n[{"note": "a"}, {"note": "b"}, {"note": "c"}]\n```\n'; + writeLedgerFile(tmp, withProse); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '3', '--description', 'third entry', '--file', 'c/three.sh'], + tmp, + { GSD_JSON_ERRORS: '1' }, + ); + + assert.equal(res.success, true, `append must succeed — the ledger table agrees with the real JSON entries, not the unrelated prose array: ${res.error || ''}`); + const obj = JSON.parse(res.output); + assert.equal(obj.entry.description, 'third entry'); + assert.equal(obj.ledger.total_count, 3); + const written = readLedgerFile(tmp); + assert.ok(written.includes('third entry'), 'new entry must be present in the written ledger'); + + // #3689 bug discovery: the trailing prose text ABOVE the fenced array + // must survive byte-for-byte. A wrong binding (locateJsonBlock resolving + // to the prose's own fenced array instead of the real ledger block) + // computes `trailingProse` from the PROSE fence's afterClose, silently + // dropping everything between the real ledger block and the prose + // block — including "Operator notes below the ledger." itself. Asserting + // only append-succeeds (as this test did before) cannot catch that: the + // write still succeeds, it just discards the operator's prose. + const trailingProse = 'Operator notes below the ledger.\n\n' + + '```json\n[{"note": "a"}, {"note": "b"}, {"note": "c"}]\n```\n'; + assert.ok( + written.includes(trailingProse), + 'trailing prose above and including the fenced JSON array must survive byte-for-byte', + ); + }); + + test('windows append tolerates a description containing a newline (#3689)', (t) => { + const tmp = createTempDir('bw-3689-newline-desc-'); + t.after(() => cleanup(tmp)); + // validateDescription (src/broken-windows.cts:198) rejects only empty + // strings and 4-backtick runs, not \n — and renderTable's cell() escapes + // `\` and `|` but not newlines, so this row physically spans two file + // lines. A `|`-prefix scan of the pre-fence text stops dead at that + // continuation line; the header-anchored fix must not. + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '1', '--description', 'line one\nline two', '--file', 'a/one.sh'], + tmp, + ); + assert.ok(r1.success, `seed append with newline description failed: ${r1.error || ''}`); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '2', '--description', 'second entry', '--file', 'b/two.sh'], + tmp, + ); + assert.equal( + res.success, + true, + `append must succeed on a ledger whose only row has an embedded newline, not brick with windows_ledger_table_drift: ${res.error || ''}`, + ); + const obj = JSON.parse(res.output); + assert.equal(obj.ledger.total_count, 2); + assert.equal(obj.ledger.entries[0].description, 'line one\nline two'); + assert.equal(obj.ledger.entries[1].description, 'second entry'); + }); + + test('windows append still detects drift on a ledger whose description contains a newline (#3689)', (t) => { + const tmp = createTempDir('bw-3689-newline-desc-drift-'); + t.after(() => cleanup(tmp)); + const r1 = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '1', '--description', 'line one\nline two', '--file', 'a/one.sh'], + tmp, + ); + assert.ok(r1.success, `seed append 1 failed: ${r1.error || ''}`); + const r2 = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '2', '--description', 'second entry', '--file', 'b/two.sh'], + tmp, + ); + assert.ok(r2.success, `seed append 2 failed: ${r2.error || ''}`); + + // Hand-edit a DIFFERENT row's (row 2, single-line) status cell. Proves the + // wider header-anchored region does not blind the guard: row 1's embedded + // newline must not swallow row 2's drift. + const pristine = readLedgerFile(tmp); + const drifted = flipTableStatus(pristine, 2, 'open', 'fixed'); + writeLedgerFile(tmp, drifted); + const before = readLedgerFile(tmp); + + const res = runGsdTools( + ['windows', 'append', '--kind', 'deviation', '--phase', '3', '--description', 'third entry', '--file', 'c/three.sh'], + tmp, + { GSD_JSON_ERRORS: '1' }, + ); + + assert.equal(res.success, false, 'append must still detect drift on row 2 even though row 1 spans multiple physical lines'); + const parsed = JSON.parse(res.error); + assert.equal(parsed.ok, false, `structured error must carry ok:false: ${res.error}`); + assert.equal(parsed.reason, 'windows_ledger_table_drift', `expected typed drift reason, got: ${res.error}`); + assert.match(parsed.message, /\b2\b/, 'failure message must name the drifted row id (2)'); + assert.equal(readLedgerFile(tmp), before, 'the file must be byte-identical to the pre-image after a refusal'); + }); + + test('extractTableRegion terminates when the header literal starts the candidate region (#3689)', () => { + // #3689: the backward header search's fallback bound `searchFrom = idx - 1` + // becomes -1 when the ONLY candidate match sits at index 0 and fails the + // atLineEnd check. String.prototype.lastIndexOf clamps a negative position + // to 0 per spec, so the next iteration re-finds the same rejected match at + // idx 0 forever — a candidate that STARTS with the header literal followed + // by a non-newline character reproduces this exactly. This must return + // promptly (a regression here hangs the test process, not fail it). + const TABLE_HEADER_LINE = + '| id | phase | kind | file | line | description | status | reason | recorded_at | resolved_at |'; + const raw = `${TABLE_HEADER_LINE}X\n\`\`\`\`json\n[]\n\`\`\`\`\n`; + const result = brokenWindowsLib.extractTableRegion(raw); + // No line-anchored header match exists (the only occurrence is followed by + // "X", not a newline/EOF), so the corrected backward search must exhaust + // its bound and report "no header found" rather than hang. + assert.equal(result, null, 'extractTableRegion must return null when no line-anchored header match exists'); + }); +}); + +// --------------------------------------------------------------------------- +// #3689 property: table region extraction round-trips to renderTable +// +// CONTRACT PIN (not a guess — the fix MUST match this exactly): +// The #3689 fix must export from src/broken-windows.cts: +// - `renderTable(entries: WindowEntry[]): string` — the existing private +// renderer, promoted to an export. +// - `extractTableRegion(raw: string): string | null` — returns the exact +// table text of a rendered ledger, or null when no table region can be +// located. +// The property below asserts +// extractTableRegion(renderLedger(ledger)) === renderTable(ledger.entries) +// for every generated ledger. Neither symbol is exported by the shipped +// module today, so this property fails immediately on the `typeof` +// assertions below — that is a correct failure (the contract this test +// encodes does not exist yet), not a flake. +// --------------------------------------------------------------------------- + +describe('#3689 property: table region extraction round-trip', () => { + const arbPropKind = fc.constantFrom( + 'stub', 'todo', 'fixme', 'skipped-test', 'lint-warning', 'unmet-truth', 'unrun-verify', 'deviation', + ); + // #3689: descriptions CAN contain an embedded newline — validateDescription + // rejects only empty strings and 4-backtick runs (src/broken-windows.cts:198) + // — which is exactly why the prior `|`-prefix table-region scan could brick + // a clean ledger. Strip only `\r` (CRLF-normalize) so `\n` survives into the + // generated description and this property exercises the multi-physical-line + // row case the header-anchored fix must round-trip. + const arbPropDescription = fc.oneof( + fc.constant(''), + fc.string({ maxLength: 40 }), + fc.constant('has | a pipe'), + fc.constant('has \\ a backslash'), + fc.constant('both \\| combined'), + fc.constant('line one\nline two'), + ).map((s) => s.replace(/\r/g, '')); + + const arbPropEntry = fc.record({ + id: fc.integer({ min: 1, max: 500 }), + kind: arbPropKind, + phase: fc.integer({ min: 0, max: 99 }).map(String), + file: fc.oneof(fc.constant(''), fc.constant('src/x.ts')), + line: fc.oneof(fc.constant(null), fc.integer({ min: 1, max: 9999 })), + description: arbPropDescription, + status: fc.constantFrom('open', 'waived', 'fixed'), + reason: fc.oneof(fc.constant(''), fc.constant('justified')), + recorded_at: fc.constant('2026-08-24T00:00:00Z'), + resolved_at: fc.oneof(fc.constant(null), fc.constant('2026-08-24T01:00:00Z')), + }); + + test('property: the table region extracted from a rendered ledger round-trips to renderTable (#3689)', () => { + fc.assert(fc.property(fc.array(arbPropEntry, { maxLength: 5 }), (entries) => { + assert.equal( + typeof brokenWindowsLib.extractTableRegion, + 'function', + 'extractTableRegion must be exported by the #3689 fix — writeLedgerAtomic\'s ' + + 'drift guard needs it to parse the on-disk table region independently of the ' + + 'JSON block; not yet exported, so this property fails today for the right reason.', + ); + assert.equal( + typeof brokenWindowsLib.renderTable, + 'function', + 'renderTable must be exported so this property can compare against the real ' + + 'renderer instead of a test-side reimplementation; not yet exported (module-private today).', + ); + + const ledger = { + schema_version: 1, + open_count: entries.filter((e) => e.status === 'open').length, + waived_count: entries.filter((e) => e.status === 'waived').length, + fixed_count: entries.filter((e) => e.status === 'fixed').length, + total_count: entries.length, + last_updated: '2026-08-24T00:00:00Z', + entries, + }; + const rendered = renderLedger(ledger); + const extracted = brokenWindowsLib.extractTableRegion(rendered); + const expected = brokenWindowsLib.renderTable(entries); + assert.equal(extracted, expected, 'extracted table region must match renderTable(entries) exactly'); + })); + }); +}); + +// --------------------------------------------------------------------------- +// #1950-H2 / #3689 review finding: writeLedgerAtomic's pre-image read must +// not treat every fs error as "no ledger yet". A bare catch there would let +// EACCES/EIO/etc. fall through as if the file were absent, silently skipping +// the drift guard and overwriting an unreadable pre-image — a guard that can +// be bypassed by making the file unreadable is not a guard. This had no +// coverage. +// +// Injection method: monkeypatch `fs.readFileSync` and restore it in a +// `finally` (CONTRIBUTING.md fault-injection convention; mirrors +// tests/verify-command-grounding.test.cjs "row 24 — unreadable phase +// degrades, never throws"). `fs.chmodSync(path, 0o000)` is not used: root +// (how CI/Docker run) bypasses mode bits entirely, so that approach would +// pass with zero real coverage. This must go in-process (not through +// runGsdTools) because a monkeypatch in the parent process is invisible to a +// child process. +// --------------------------------------------------------------------------- + +describe('#1950-H2 / #3689: writeLedgerAtomic pre-image read failure', () => { + test('windows append refuses when the pre-image is unreadable rather than silently overwriting it (#1950-H2 + #3689)', (t) => { + const tmp = createTempDir('bw-3689-unreadable-preimage-'); + t.after(() => cleanup(tmp)); + + // Seed a real, on-disk ledger via a genuine (unmocked) append. The + // branch under test is reached only when readFileSync throws something + // OTHER than ENOENT, which requires a pre-image to actually exist. + cmdWindowsAppend(tmp, ['--kind', 'stub', '--phase', '1', '--description', 'seed entry'], {}); + const ledgerPath = path.join(tmp, '.planning', LEDGER_FILE_NAME); + assert.ok(fs.existsSync(ledgerPath), 'guard: seed append must have written a ledger file'); + const pristine = fs.readFileSync(ledgerPath, 'utf8'); + + const originalReadFileSync = fs.readFileSync; + let caught; + try { + fs.readFileSync = (p, ...rest) => { + if (typeof p === 'string' && path.resolve(p) === path.resolve(ledgerPath)) { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + return originalReadFileSync.call(fs, p, ...rest); + }; + + try { + cmdWindowsAppend(tmp, ['--kind', 'stub', '--phase', '2', '--description', 'second entry'], {}); + } catch (e) { + caught = e; + } + } finally { + fs.readFileSync = originalReadFileSync; + } + + assert.ok(caught, 'an unreadable pre-image must throw, not proceed to overwrite the file'); + assert.ok(caught instanceof WindowsError, 'must surface as a typed WindowsError, not a bare fs error'); + assert.equal(caught.reason, REASON.WINDOWS_LEDGER_MALFORMED); + assert.match(caught.message, /EACCES/, 'message must name the errno that made the pre-image unreadable'); + assert.ok( + caught.message.includes(ledgerPath), + `message must name the unreadable path (${ledgerPath}): ${caught.message}`, + ); + + // Fail-closed: the on-disk ledger must be byte-identical to the + // pre-image seeded above — no partial or silent overwrite occurred. + assert.equal( + fs.readFileSync(ledgerPath, 'utf8'), + pristine, + 'an unreadable pre-image must not be overwritten', + ); + }); +}); diff --git a/tests/review-parallel-lanes.test.cjs b/tests/review-parallel-lanes.test.cjs index ac166a924..5257d9bff 100644 --- a/tests/review-parallel-lanes.test.cjs +++ b/tests/review-parallel-lanes.test.cjs @@ -209,8 +209,14 @@ const STUB_PREAMBLE = [ ' if ! in_list "$slug" "$STUB_SILENT"; then', ' printf \'{"slug":"%s","pad":"%s"}\\n\' "$slug" "$pad"', ' fi', - ' touch "$RUN_DIR/done-$slug"', + // #3689: the done-file is a cross-process happens-before edge — a + // dependent lane unblocks the instant this file appears (wait_for_file + // above just polls for its existence), so everything a dependent may + // observe (the "end:$slug" trace line) must be written BEFORE the file + // that releases it. touch-then-echo let a descheduled upstream lose the + // race to its own dependent, inverting the #3034 completion-order trace. ' echo "end:$slug" >> "$TRACE"', + ' touch "$RUN_DIR/done-$slug"', ' if in_list "$slug" "$STUB_FAIL"; then', ' return 1', ' fi',