fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON (#3828)

* 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(<entries parsed from the on-disk JSON>)` 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' + <a valid json fence>` 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-<dep>` 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 <sim@local>

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(<on-disk JSON entries>)` 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<string, string> => {
+    const lines = table.split('\n');
+    const map = new Map<string, string>();
+    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<string>();
+  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);