From fb9823e1e18a47c50f50eb9284842594d7274259 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 24 Aug 2026 17:43:25 -0400 Subject: [PATCH] fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON (#3828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3689): failing-first coverage for the ledger table/JSON agreement guard `.planning/WINDOWS.md` renders its markdown table from the fenced JSON that is its source of truth, but nothing checks the two still agree before a write overwrites the table. `windows append` / `waive` / `fixed` therefore discard a drifted cell silently, and erase a table-only row entirely, both at exit 0. Adds to tests/broken-windows.test.cjs: - five refusal cases that fail today, covering all three write commands, a drifted cell, a table-only row, and drift on a non-first row; each asserts the typed reason via GSD_JSON_ERRORS and that the file is byte-identical after the refusal, so a guard that refuses only after writing cannot pass - six anti-tightening pins that must stay green: an agreeing ledger, the first-write ENOENT path, #2893 trailing-prose preservation, #3657 3-backtick fence tolerance, escaped pipes and backslashes in a description, and the zero-entry placeholder table - a fast-check property pinning the round trip the guard depends on — extractTableRegion(renderLedger(l)) === renderTable(l.entries) — because a false refusal on a clean ledger would be worse than the bug Fixtures are built by running the real CLI and then perturbing only the table, so frontmatter and JSON stay consistent and the pre-existing counts cross-check still passes; a hand-written ledger would pass these for the wrong reason. Refs #3689 * fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON `.planning/WINDOWS.md` renders its markdown table from the fenced JSON that is its source of truth, and `writeLedgerAtomic` regenerated that table on every `windows append` / `waive` / `fixed` without ever checking the two still agreed. A hand-edited cell was silently reverted; a row that existed only in the table vanished entirely. Both at exit 0, with nothing on stdout to say so. The write seam now compares the on-disk table against `renderTable()` before regenerating anything, and refuses with a typed `windows_ledger_table_drift` error naming the drifted row ids and the remedy. Because the check sits at the single write seam, all three commands inherit it, and the file is left byte-identical on refusal. 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 to diagnose the drift. Two hazards handled explicitly, both discovered in review of the first draft: - The pre-image read now distinguishes ENOENT from every other errno, per the #1950-H2 fail-closed-on-unreadable invariant `readLedgerOrNull` already honors. A bare catch would have let an unreadable pre-image skip the guard and write anyway. - Both the entries baseline and the table extraction pass the pre-image's own frontmatter `total_count` to `locateJsonBlock`. Without that hint the no-expectation fallback binds to the LATEST fenced JSON array in the file, which is the operator's prose block whenever that prose contains one — the exact case #2893 exists for — refusing every write on a ledger that never drifted. A regression test covers it. Also extends the CONTEXT.md Broken Windows Ledger glossary entry: the table is a third projection of the same source, cross-checked at the write seam, and the frozen REASON enum gains WINDOWS_LEDGER_TABLE_DRIFT. Fixes #3689 * fix(#3689): bind prose preservation to the pre-image's own ledger block Found while reviewing the table drift guard: the #2893 trailing-prose preservation in `writeLedgerAtomic` passed `ledger.total_count` — the POST-mutation count — as the disambiguation hint for a lookup over the PRE-image. On an append the pre-image holds N entries while the hint says N+1, so the hint can never match and `locateJsonBlock` falls through to its last-array-shaped-span fallback. When the operator's trailing prose itself contains a fenced JSON array — the ordinary case #2893 was written to protect — that prose block wins the fallback. The preserved region is then computed from the prose fence rather than the ledger fence, and everything between them, including the operator's own text above the array, is silently dropped on the next write. Reproduced against the real CLI: a prose block reading "Operator notes above the array, IMPORTANT DO NOT LOSE THIS TEXT." plus a fenced 3-element array came back empty after one `windows append`. Both the prose lookup and the drift guard now share one pre-image-derived `preImageExpectedTotal`, taken from the pre-image's own frontmatter, so they bind to the same and correct block. The existing trailing-prose regression test is strengthened to assert the prose survives byte-for-byte rather than merely that the command exited 0 — asserting only the exit code is why this was invisible. Refs #3689 * fix(#3689): anchor table extraction on the header row, not a line-prefix scan Independent review found the drift guard could brick a ledger nobody had hand-edited. `validateDescription` accepts a description containing a raw newline, and `renderTable`'s cell escaping covers backslash and pipe but not newlines — so such a description renders a row that physically spans two file lines, the second of which does not begin with `|`. `extractTableRegion` bounded the table by walking backward over the contiguous run of `|`-prefixed lines, so it stopped at that split. In the common case where the row's tail is the last line before the fence it returned null, and every subsequent append/waive/fixed was refused with "table region could not be located" — permanently, with no CLI recovery path, on a ledger that never drifted. A false refusal is worse than the bug this guard exists to fix. The region is now anchored on the header row `renderTable` always emits, running from its last line-start occurrence to the end of the pre-fence text. The boundary is the fence rather than a line prefix, so a multi-line row is captured whole, re-renders byte-identically, and compares equal. The header literal is hoisted to one constant both `renderTable` branches and the extractor share, so the two surfaces cannot drift apart. Deliberately unchanged: `cell()` and `validateDescription`. The cosmetic corruption a newline causes in the rendered table is pre-existing, and either escaping it or rejecting the input would change what existing ledgers render to or what input is accepted. Also closes a coverage gap the standards review raised: the non-ENOENT pre-image read branch — the one that stops an unreadable file from bypassing the guard — now has a behavioral test that injects EACCES by monkeypatching `fs.readFileSync` for that one path and restoring it in a `finally`, never by `chmod 0o000` (root ignores mode bits, so that would pass with zero coverage). The #3689 property generator no longer strips newlines out of descriptions, which is why this was invisible to it. Refs #3689 * chore(changeset): backfill PR number for #3689 fragment * chore(changeset): backfill PR number for #3689 fragment * fix(#3689): terminate the header scan when the match sits at index 0 `extractTableRegion`'s backward search for the table header could loop forever. On a rejected match at index 0 it set `searchFrom = idx - 1`, i.e. `-1`; `String.prototype.lastIndexOf` clamps its position argument into `[0, length]`, so the next iteration searched from 0, found the same match, rejected it identically, and set `-1` again. The loop made no progress. Reachable only through the exported `extractTableRegion` — `writeLedgerAtomic` reaches it after `parseFrontmatterStrict` has already succeeded, so the candidate region begins with the `---` frontmatter fence and a match at index 0 is impossible. Latent rather than live, but an exported `for(;;)` that can fail to advance is not something to ship. Confirmed by running the pre-fix compiled function on `TABLE_HEADER_LINE + 'X\n' + ` as a backgrounded child: it was still alive after five seconds having printed nothing, and had to be killed. Post-fix the same input returns `null` promptly — correct, since the sole header occurrence fails the end-of-line test and no valid header exists. A regression here would stall the suite rather than fail it, so the new test also asserts the returned value rather than relying on termination alone. No wall-clock assertion is involved. Refs #3689 * test(#3034): publish the lane trace before the done-file that releases dependents `preservesSelectionOrderParallelDespiteCompletionOrder` forces a reverse completion order with a dependency chain rather than sleeps: each stub lane waits on `done-` before finishing. It then ended with touch "$RUN_DIR/done-$slug" echo "end:$slug" >> "$TRACE" Those are two unsynchronized operations in separate shell processes. A dependent's `wait_for_file` unblocks the instant the upstream's `touch` lands, but the upstream's own `echo` has not necessarily run — so if the upstream is descheduled between the two, the dependent can run its whole body and append its `end:` line first. The done-file was published before the state it signals. Observed on the remote runner as `[end:claude, end:codex, end:gemini]` where selection order demands `[end:claude, end:gemini, end:codex]`. The failure was in the fixture's own self-check, before it reached the assertion #3034 exists to make. Not a flake and not a wall-clock margin: this branch passed the full suite twice at 14f494644 and 90c5d7a03, and the only delta in the failing run was one added test in tests/broken-windows.test.cjs — an unrelated module. Adding load elsewhere in the suite was enough to invert it, which is what a real race does. Swapping the pair establishes a genuine happens-before: anything a dependent can observe is written before the file that releases it. A comment records why, so the order is not tidied back. The production path is unaffected and was independently confirmed correct — `invoke_reviewers` joins every lane with `wait`, then aggregates by iterating DISPATCH_SLUGS in selection order, reading per-slug result files. It consumes no completion-order signal at all. Refs #3034 --------- Co-authored-by: sim --- .changeset/eager-tigers-zip.md | 5 + .changeset/sturdy-eagles-leap.md | 5 + CONTEXT.md | 2 +- src/broken-windows.cts | 242 ++++++++++- tests/broken-windows.test.cjs | 579 ++++++++++++++++++++++++++- tests/review-parallel-lanes.test.cjs | 8 +- 6 files changed, 830 insertions(+), 11 deletions(-) create mode 100644 .changeset/eager-tigers-zip.md create mode 100644 .changeset/sturdy-eagles-leap.md 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',