From ede8844f997cf4ab331cbf564704f1c3b90ab350 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 10:38:33 +0000 Subject: [PATCH 01/15] fix(ledger): refuse a write-discipline verdict over a dirty governed tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:ledger-write-discipline compares two committed refs, so an edit still sitting in the working tree is invisible to it and the audited range is empty. It printed "Ledger write discipline passed" over a forbidden hand-edit of docs/outstanding-issues.md on 2026-08-13 (issue #313) — a real green that had evaluated nothing, which is the worst kind. The gate now reads git status for the paths it governs (the canonical ledger, the frozen review ledger, and the inbox) and refuses to report any verdict while one of them is dirty, naming each offending path. Only fires when the head endpoint is the default HEAD. guard-push.mjs invokes this check with an explicit committed --head at a moment when the tree is legitimately dirty, and CI checks out clean, so neither path is affected. Porcelain output is read untrimmed: an unstaged change is " M path", and the shared git() helper's trim() eats that leading space and slices a character off every path, which silently stops the governed-path match from firing. Both the self-test and tests/ledger-write-discipline.test.ts pin that. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- scripts/check-ledger-write-discipline.mjs | 96 +++++++++++++++++++++++ tests/ledger-write-discipline.test.ts | 69 ++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 tests/ledger-write-discipline.test.ts diff --git a/scripts/check-ledger-write-discipline.mjs b/scripts/check-ledger-write-discipline.mjs index 3790dd297d..68ee61e02b 100644 --- a/scripts/check-ledger-write-discipline.mjs +++ b/scripts/check-ledger-write-discipline.mjs @@ -10,6 +10,12 @@ * * This is deliberately stronger than a shape check. A valid-looking manual * edit would still reintroduce the shared-hunk race that the inbox removes. + * + * Both comparison endpoints are commits, so an edit that is still sitting in + * the working tree is invisible here and the audited range is empty. Reporting + * a pass in that state told an author on 2026-08-13 that a forbidden hand-edit + * of the canonical ledger was fine (issue #313). The gate therefore refuses to + * report any verdict while a governed path is dirty — see dirtyGovernedPaths. */ import { execFileSync } from "node:child_process"; import path from "node:path"; @@ -28,6 +34,19 @@ function git(args) { return execFileSync("git", args, { cwd: ROOT, encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }).trim(); } +/** + * Untrimmed on purpose: porcelain's status field is two columns wide and its + * first column is a space for an unstaged change, so trimming shifts every path + * by one character and the governed-path match silently stops firing. + */ +function statusPorcelain(pathspecs) { + return execFileSync("git", ["status", "--porcelain=v1", "-z", "--untracked-files=normal", "--", ...pathspecs], { + cwd: ROOT, + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); +} + function readAt(ref, relative) { return execFileSync("git", ["show", `${ref}:${relative}`], { cwd: ROOT, @@ -156,6 +175,48 @@ export function verifyIssueReconciliation({ return failures; } +function governedReason(relative) { + if (relative === ISSUES_LEDGER) return "the canonical outstanding-issues ledger"; + if (relative === REVIEW_LEDGER) return "the frozen branch-review ledger"; + if (relative === INBOX || relative.startsWith(`${INBOX}/`)) return "an outstanding-issues inbox request"; + return undefined; +} + +/** + * Governed paths carrying working-tree changes the committed-range audit cannot + * see. Input is `git status --porcelain=v1 -z --untracked-files=normal` output; + * NUL records avoid porcelain's quoting rules entirely, and a rename/copy record + * is followed by a second field holding its original path. + * + * Pure and exported so the script self-test and focused tests can exercise it + * without a fixture repository. + */ +export function dirtyGovernedPaths(porcelain) { + const fields = String(porcelain ?? "").split("\0"); + const dirty = []; + const seen = new Set(); + for (let index = 0; index < fields.length; index += 1) { + const record = fields[index]; + if (record.length < 4) continue; // trailing empty field, never a real record + const status = record.slice(0, 2); + const paths = [record.slice(3)]; + if (status[0] === "R" || status[0] === "C") { + const origin = fields[index + 1]; + index += 1; + if (origin) paths.push(origin); + } + for (const relative of paths) { + const reason = governedReason(relative); + if (reason === undefined || seen.has(relative)) continue; + seen.add(relative); + const change = + status === "??" ? "is untracked" : `has an uncommitted change (${status.trim() || status.trimEnd()})`; + dirty.push({ path: relative, status, reason: `${reason} ${change}` }); + } + } + return dirty; +} + export function reviewLedgerRowsChanged(baseMarkdown, headMarkdown) { const baseRows = parseLedgerRows(baseMarkdown).map((row) => row.raw); const headRows = parseLedgerRows(headMarkdown).map((row) => row.raw); @@ -288,6 +349,23 @@ function selfTest() { throw new Error("self-test failed: partial reconciliation was accepted"); } + // #313: the committed-range audit cannot see a working-tree edit, so a pass + // reported over a dirty governed path is a green that evaluated nothing. + if (dirtyGovernedPaths("").length !== 0) throw new Error("self-test failed: a clean worktree was reported dirty"); + const dirtyLedger = dirtyGovernedPaths(` M ${ISSUES_LEDGER}\0`); + if (dirtyLedger.length !== 1 || dirtyLedger[0].path !== ISSUES_LEDGER) { + throw new Error("self-test failed: an uncommitted canonical ledger edit was not detected"); + } + if (dirtyGovernedPaths(`?? ${INBOX}/${name}\0`).length !== 1) { + throw new Error("self-test failed: an untracked inbox request was not detected"); + } + if (dirtyGovernedPaths(`R ${APPLIED}/${name}\0${INBOX}/${name}\0`).length !== 2) { + throw new Error("self-test failed: a renamed request did not report both of its paths"); + } + if (dirtyGovernedPaths(" M src/lib/rag/rag.ts\0").length !== 0) { + throw new Error("self-test failed: an unrelated dirty file was treated as a ledger violation"); + } + const review = `| 2026-08-13 | codex/a | ${"a".repeat(40)} | review | pass | test |\n`; if (!reviewLedgerRowsChanged("", review) || reviewLedgerRowsChanged(review, review)) { throw new Error("self-test failed: legacy review row change detection is incorrect"); @@ -298,6 +376,24 @@ function selfTest() { function main() { if (process.argv.includes("--self-test")) return selfTest(); const { base, head } = resolveArgs(process.argv.slice(2)); + + // Only when the head endpoint is the working checkout's tip. guard-push.mjs + // passes an explicit committed --head at a moment when the tree is legitimately + // dirty, and CI checks out clean, so neither is affected by this refusal. + if (head === "HEAD") { + const dirty = dirtyGovernedPaths(statusPorcelain([ISSUES_LEDGER, REVIEW_LEDGER, INBOX])); + if (dirty.length > 0) { + console.error("Ledger write-discipline check refused to report a verdict:"); + for (const entry of dirty) console.error(`- ${entry.path}: ${entry.reason}`); + console.error( + "\nThis gate compares two committed refs, so an uncommitted ledger edit is invisible to it and a pass " + + "would mean nothing. Commit the change first — git add the inbox request if it is new — then re-run.", + ); + process.exitCode = 1; + return; + } + } + const failures = []; const baseReview = readAt(base, REVIEW_LEDGER); const headReview = readAt(head, REVIEW_LEDGER); diff --git a/tests/ledger-write-discipline.test.ts b/tests/ledger-write-discipline.test.ts new file mode 100644 index 0000000000..1a5bbf64bb --- /dev/null +++ b/tests/ledger-write-discipline.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from "vitest"; + +import { dirtyGovernedPaths } from "../scripts/check-ledger-write-discipline.mjs"; + +const ISSUES = "docs/outstanding-issues.md"; +const REVIEW = "docs/branch-review-ledger.md"; +const INBOX = "docs/outstanding-issues-inbox"; +const REQUEST = `${INBOX}/829597d4-698b-4cc2-9bf4-65310504cba3.json`; + +/** `git status --porcelain=v1 -z` emits NUL-terminated records, not lines. */ +const porcelain = (...records: string[]) => `${records.join("\0")}\0`; + +describe("ledger write discipline dirty-tree guard", () => { + it("reports nothing for a clean worktree", () => { + expect(dirtyGovernedPaths("")).toEqual([]); + expect(dirtyGovernedPaths(porcelain())).toEqual([]); + }); + + it("flags an uncommitted edit to the canonical ledger", () => { + // The #313 defect: both audit endpoints are commits, so this edit is + // invisible to the range and the gate used to print a pass over it. + const dirty = dirtyGovernedPaths(porcelain(` M ${ISSUES}`)); + expect(dirty).toHaveLength(1); + expect(dirty[0].path).toBe(ISSUES); + expect(dirty[0].reason).toContain("uncommitted change"); + }); + + it("does not shift paths when the status field's first column is a space", () => { + // An unstaged change is " M path". Trimming the porcelain output eats that + // leading space and slices one character off every path, which silently + // stops the governed-path match from ever firing. + expect(dirtyGovernedPaths(porcelain(` M ${ISSUES}`))[0].path).toBe(ISSUES); + expect(dirtyGovernedPaths(porcelain(`M ${ISSUES}`))[0].path).toBe(ISSUES); + expect(dirtyGovernedPaths(porcelain(`MM ${ISSUES}`))[0].path).toBe(ISSUES); + }); + + it("flags an untracked inbox request and the frozen review ledger", () => { + expect(dirtyGovernedPaths(porcelain(`?? ${REQUEST}`))).toEqual([ + { path: REQUEST, status: "??", reason: "an outstanding-issues inbox request is untracked" }, + ]); + expect(dirtyGovernedPaths(porcelain(` M ${REVIEW}`))[0].reason).toContain("branch-review ledger"); + }); + + it("reports both sides of a rename record", () => { + // Porcelain follows an R/C record with a second NUL field holding the origin. + const dirty = dirtyGovernedPaths(porcelain(`R ${INBOX}/applied/a.json`, `${INBOX}/a.json`)); + expect(dirty.map((entry) => entry.path)).toEqual([`${INBOX}/applied/a.json`, `${INBOX}/a.json`]); + }); + + it("consumes the rename origin field rather than reading it as a record", () => { + const dirty = dirtyGovernedPaths(porcelain(`R ${INBOX}/applied/a.json`, `${INBOX}/a.json`, ` M ${ISSUES}`)); + expect(dirty.map((entry) => entry.path)).toEqual([`${INBOX}/applied/a.json`, `${INBOX}/a.json`, ISSUES]); + }); + + it("ignores dirty files the gate does not govern", () => { + // The gate must not fail an ordinary product change; only ledger paths make + // its committed-range verdict meaningless. + expect(dirtyGovernedPaths(porcelain(" M src/lib/rag/rag.ts", "?? scratch.txt", " M package.json"))).toEqual([]); + }); + + it("does not treat a lookalike path outside the inbox as governed", () => { + expect(dirtyGovernedPaths(porcelain(" M docs/outstanding-issues-inbox-notes.md"))).toEqual([]); + expect(dirtyGovernedPaths(porcelain(" M docs/outstanding-issues.md.bak"))).toEqual([]); + }); + + it("reports each governed path once", () => { + expect(dirtyGovernedPaths(porcelain(` M ${ISSUES}`, `MM ${ISSUES}`))).toHaveLength(1); + }); +}); From 418068afff7461c41b60c6b1ac309e154ef5d10a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 10:43:44 +0000 Subject: [PATCH 02/15] feat(audit): detect merged PRs silently reverted by a later merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 2026-08-11 merge acf78bf took the stale branch side of several manual conflict resolutions and reverted seven already-merged PRs. Nothing went red because the reverts took each PR's tests in the same stroke, leaving no assertion to fail; the source casualties went unnoticed for two days. audit:merge-loss reproduces the measurement that found them. For every PR landing on origin/main inside a bounded window (default 14 days), it compares the ref's current blob for each file that landing changed against the blob at the landing's first parent. Equality means the landing's contribution to that file is gone. Blob OIDs are compared rather than content, so a wide window stays cheap. Both merge and squash landings are recognised. Advisory on purpose: a deliberate later revert is byte-identical to an accidental one, so it names the PR, commit and files, asks for human confirmation and exits 0. --strict is available for a caller that wants a hard failure. Run against this checkout over a six-day window it independently rediscovers the known casualties — #1803 (53 files), #1800, #1804, #1796, #1811 — which is the validation that matters. One narrow exemption: issues:reconcile moves an inbox request to applied/ verbatim, which otherwise looks like an added file that vanished. That was six of the first fifteen findings and would have buried the real signal. The move is credited only when the identically-named audit record exists, and the excluded count is always reported rather than silently dropped. No workflow and no verify:cheap wiring: scheduling this is an operational change needing its own PR and explicit approval, and joining the local gate chain would force a matching ci.yml step via check-gate-manifest. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- docs/scripts-index.md | 3 +- package.json | 1 + scripts/audit-merge-loss.mjs | 330 +++++++++++++++++++++++++++++++++ tests/merge-loss-audit.test.ts | 132 +++++++++++++ 4 files changed, 465 insertions(+), 1 deletion(-) create mode 100644 scripts/audit-merge-loss.mjs create mode 100644 tests/merge-loss-audit.test.ts diff --git a/docs/scripts-index.md b/docs/scripts-index.md index 0fa9188651..aa9de8bd4f 100644 --- a/docs/scripts-index.md +++ b/docs/scripts-index.md @@ -1,6 +1,6 @@ # Scripts index -Curated map of `scripts/` (237 files) and the `package.json` script surface (245 entries), +Curated map of `scripts/` (238 files) and the `package.json` script surface (246 entries), grouped by purpose. This is orientation, not an exhaustive per-file listing — the authoritative command list is `package.json`, and `npm run docs:check-scripts` verifies every `npm run ` referenced in docs resolves to a real script. `npm run docs:update` refreshes the exact counts above. @@ -28,6 +28,7 @@ migration has shipped (see `docs/maturity-backlog-workorders.md` L1). | `check-outstanding-issues.mjs`, `check-pr-mergeability-workflow.mjs` | Outstanding-issues ID/marker/no-driver guard + PR mergeability workflow contract | | `outstanding-issues.mjs` | Writer for `docs/outstanding-issues.md` (`issues:add` / `issues:done` / `issues:update`) — allocates the id, picks the right table, escapes `\|`, and re-runs the guard on its own output. Never hand-edit that file, as with `ledger:append` | | `check-installed-lock-parity.mjs`, `phone-chrome-plan.mjs`, `verify-phone-chrome.mjs`, `playwright-browser-preflight.mjs` | Lock-trust preflight, change-scoped phone contracts, and Playwright browser-binary preflight before build | +| `audit-merge-loss.mjs` | Advisory blob-comparison sweep for merged PRs a later merge resolution silently reverted (`audit:merge-loss`); names the PR and files and exits 0 — a deliberate revert is identical at blob level, so a positive needs human confirmation | | `final-merge-audit.mjs` | Fail-closed local merge-tree audit; explicit provider mode adds PR/check/thread/tree/deployment proof | | `child-process-result.mjs`, `cli-utils.ts`, `productivity-core.mjs` | Shared helpers | | `test-focused.mjs`, `test-run-selection.mjs`, `test-cache-path.mjs`, `test-environment.mjs` | Backs `npm run test:focused` — change-scoped selection, cache pathing, env setup; fails closed for deleted files and test infrastructure | diff --git a/package.json b/package.json index 69fab07f9d..5c2f078167 100644 --- a/package.json +++ b/package.json @@ -73,6 +73,7 @@ "verify:pr-local": "node scripts/verify-pr-local.mjs", "verify:phone-chrome": "node scripts/verify-phone-chrome.mjs", "audit:final-merge": "node scripts/final-merge-audit.mjs", + "audit:merge-loss": "node scripts/audit-merge-loss.mjs --self-test && node scripts/audit-merge-loss.mjs", "verify:ui": "npm run check:runtime && npm run check:installed-lock-parity && npm run test:e2e:pr", "verify:release": "npm run check:runtime && npm run check:installed-lock-parity && npm run lint && npm run typecheck && npm run test && npm run build && npm run test:e2e && npm run check:production-readiness && npm run governance:release && npm run eval:quality:release", "verify:release:offline": "node scripts/verify-release-offline.mjs", diff --git a/scripts/audit-merge-loss.mjs b/scripts/audit-merge-loss.mjs new file mode 100644 index 0000000000..c130fde822 --- /dev/null +++ b/scripts/audit-merge-loss.mjs @@ -0,0 +1,330 @@ +#!/usr/bin/env node +/** + * audit-merge-loss — find merged pull requests whose content was silently + * reverted by a later merge resolution. + * + * Why this exists. On 2026-08-11 merge commit acf78bf ("Merge remote-tracking + * branch origin/main into probe2-1815") took the stale branch side of several + * manual conflict resolutions and reverted seven already-merged PRs — #1800, + * #1803, #1804, #1809, #1811, #1815 and #1796. Nothing went red, because the + * reverts took each PR's tests in the same stroke: no assertion survived to + * fail. The docs casualties were repaired by 55f51ab; the source casualties + * went unnoticed for two days. Care at the keyboard is demonstrably not the + * control here — commit 6f8c70d shows a human consciously preserving the #1803 + * migration and a later merge in the same chain undoing it anyway. + * + * The measurement. For each pull request that landed on the target ref inside + * the window, compare the ref's current blob for every file that landing + * changed against that file's blob at the landing's first parent. Equality + * means the landing's contribution to that file is no longer present. Blob OIDs + * are compared rather than content, so this stays cheap over a wide window. + * + * ADVISORY BY DESIGN — this exits 0 even when it finds something. A deliberate + * later revert is byte-identical to an accidental one at blob level, so a + * positive is a question for a human, not a verdict. The report names the pull + * request, the landing commit and every affected file so that question can be + * answered. `--strict` exits 1 for a caller that wants a hard failure. + * + * DELIBERATELY NOT WIRED INTO CI OR A SCHEDULE. Running this automatically is + * an operational change that needs its own pull request and explicit approval; + * its absence from .github/workflows/ is a decision, not an oversight. It is + * also not in `verify:cheap:internal`: check-gate-manifest.mjs would then + * require a matching ci.yml step. + * + * Run: npm run audit:merge-loss + * npm run audit:merge-loss -- --since 30 --ref origin/main + * npm run audit:merge-loss -- --json + * npm run audit:merge-loss -- --strict # exit 1 on any finding + * node scripts/audit-merge-loss.mjs --self-test + */ +import { execFileSync } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const DEFAULT_REF = "origin/main"; +const DEFAULT_WINDOW_DAYS = 14; +const LOG_FORMAT = "%H%x09%cI%x09%s"; + +function git(args) { + return execFileSync("git", args, { cwd: ROOT, encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }).trim(); +} + +function tryGit(args) { + try { + return git(args); + } catch { + return undefined; + } +} + +/** + * The pull request a landing commit belongs to, or undefined. + * + * Both shapes occur on this repo's main: a merge landing ("Merge pull request + * #1935 from …") and a squash landing ("… (#1933)"). A squash subject can carry + * more than one issue reference, so the pull request is the LAST parenthesised + * number — GitHub appends it. + */ +export function parsePullNumber(subject) { + const merge = /^Merge pull request #(\d+)\b/.exec(String(subject ?? "")); + if (merge) return Number(merge[1]); + const squashed = [...String(subject ?? "").matchAll(/\(#(\d+)\)/g)]; + const last = squashed.at(-1); + return last ? Number(last[1]) : undefined; +} + +/** Parse `git log --format=%H\t%cI\t%s` output into landing records. */ +export function parseLogEntries(rawLog) { + return String(rawLog ?? "") + .split(/\r?\n/) + .filter((line) => line.trim().length > 0) + .map((line) => { + const [sha, date, ...rest] = line.split("\t"); + const subject = rest.join("\t"); + return { sha, date, subject, pullNumber: parsePullNumber(subject) }; + }); +} + +const INBOX = "docs/outstanding-issues-inbox"; + +/** + * The one exemption, kept deliberately narrow. + * + * `npm run issues:reconcile` MOVES a pending request from the inbox to + * `applied/` verbatim, so every pull request that queues one looks like it + * added a file that is now gone. That is the request's designed lifecycle, not + * a loss — and it accounted for six of fifteen findings on the first real run, + * which would have buried the genuine #1803 signal. The move is only credited + * when the identically-named audit record actually exists at the target ref. + */ +export function isReconciliationMove(file, ref, blobAt) { + const match = new RegExp(`^${INBOX}/([^/]+\\.json)$`).exec(file); + if (!match) return false; + return blobAt(ref, `${INBOX}/applied/${match[1]}`) !== null; +} + +/** + * Compare each landing's contribution against the ref's current state. + * + * `blobAt(ref, file)` returns a blob OID, or null when the path does not exist + * at that ref. Injected so the whole classifier is testable without git. + * + * A file is reported when its current blob equals its pre-landing blob. Both + * being null counts: that is the "the pull request added this file and it is + * gone again" case. A file the pull request DELETED and which is still absent + * does not match, because its pre-landing blob existed — the deletion survived. + * + * Pure and exported for the self-test and focused tests. + * + * @typedef {{ sha: string, date: string, subject: string, pullNumber: number | undefined, + * preRef: string, files?: string[] }} Landing + * @param {{ landings?: Landing[], blobAt: (ref: string, file: string) => string | null, + * ref?: string }} options + */ +export function classifyMergeLoss({ landings = [], blobAt, ref = DEFAULT_REF }) { + const findings = []; + let filesCompared = 0; + let filesExempted = 0; + const skipped = []; + for (const landing of landings) { + if (landing.pullNumber === undefined) { + skipped.push(landing); + continue; + } + const reverted = []; + for (const file of landing.files ?? []) { + filesCompared += 1; + const before = blobAt(landing.preRef, file); + const now = blobAt(ref, file); + if (before !== now) continue; + if (now === null && isReconciliationMove(file, ref, blobAt)) { + filesExempted += 1; + continue; + } + reverted.push({ file, absent: now === null }); + } + if (reverted.length > 0) { + findings.push({ + pullNumber: landing.pullNumber, + sha: landing.sha, + date: landing.date, + subject: landing.subject, + changedFiles: (landing.files ?? []).length, + revertedFiles: reverted, + }); + } + } + findings.sort((a, b) => b.revertedFiles.length - a.revertedFiles.length || a.pullNumber - b.pullNumber); + return { findings, scannedLandings: landings.length - skipped.length, skipped, filesCompared, filesExempted }; +} + +function resolveArgs(argv) { + const value = (name) => { + const index = argv.indexOf(name); + return index >= 0 ? argv[index + 1] : undefined; + }; + const rawSince = value("--since"); + const since = rawSince === undefined ? DEFAULT_WINDOW_DAYS : Number(rawSince); + if (!Number.isFinite(since) || since <= 0) { + throw new Error(`--since expects a positive number of days, received "${rawSince}"`); + } + return { + ref: value("--ref") ?? DEFAULT_REF, + since, + json: argv.includes("--json"), + strict: argv.includes("--strict"), + }; +} + +function collectLandings(ref, since) { + const rawLog = git(["log", "--first-parent", `--since=${since} days ago`, `--format=${LOG_FORMAT}`, ref]); + return parseLogEntries(rawLog).map((landing) => { + const preRef = `${landing.sha}^1`; + const names = tryGit(["diff", "--name-only", preRef, landing.sha]); + return { ...landing, preRef, files: names === undefined ? [] : names.split(/\r?\n/).filter(Boolean) }; + }); +} + +function blobReader() { + const cache = new Map(); + return (ref, file) => { + const key = `${ref}:${file}`; + if (!cache.has(key)) cache.set(key, tryGit(["rev-parse", `${ref}:${file}`]) ?? null); + return cache.get(key); + }; +} + +function report(result, { ref, since, strict }) { + const { findings, scannedLandings, skipped, filesCompared, filesExempted } = result; + console.log( + `[merge-loss] scanned ${scannedLandings} pull request landing(s) on ${ref} over the last ${since} day(s); ` + + `compared ${filesCompared} file(s).`, + ); + if (skipped.length > 0) { + console.log( + `[merge-loss] ${skipped.length} first-parent commit(s) carried no pull request number and were skipped.`, + ); + } + if (filesExempted > 0) { + console.log( + `[merge-loss] ${filesExempted} inbox request(s) were excluded: issues:reconcile moved them to applied/ verbatim.`, + ); + } + if (findings.length === 0) { + console.log("[merge-loss] No landing has been reverted to its pre-merge state."); + return 0; + } + + console.log(""); + console.log(`[merge-loss] ${findings.length} landing(s) look reverted — HUMAN CONFIRMATION REQUIRED.`); + console.log("A deliberate later revert is identical to an accidental one at blob level, so this is a"); + console.log("question, not a verdict. For each entry below, decide whether the change was meant to go."); + for (const finding of findings) { + console.log(""); + console.log(` PR #${finding.pullNumber} — ${finding.subject}`); + console.log(` landed ${finding.date} as ${finding.sha.slice(0, 12)}`); + console.log( + ` ${finding.revertedFiles.length} of ${finding.changedFiles} changed file(s) match the pre-merge blob:`, + ); + for (const entry of finding.revertedFiles) { + console.log(` - ${entry.file}${entry.absent ? " (added by the PR, absent now)" : ""}`); + } + console.log(` Inspect: git diff ${finding.sha}^1 ${finding.sha} -- `); + } + console.log(""); + console.log("Confirmed losses are re-landed as their own pull request; record the decision with npm run issues:add."); + return strict ? 1 : 0; +} + +function selfTest() { + if (parsePullNumber("Merge pull request #1935 from BigSimmo/codex/fix-x") !== 1935) { + throw new Error("self-test failed: merge-landing subject not parsed"); + } + if (parsePullNumber("ci: speed iteration without weakening gates (#1926)") !== 1926) { + throw new Error("self-test failed: squash-landing subject not parsed"); + } + if (parsePullNumber("fix: close (#12) properly (#1930)") !== 1930) { + throw new Error("self-test failed: trailing pull request number not preferred"); + } + if (parsePullNumber("Merge branch 'main' into feature") !== undefined) { + throw new Error("self-test failed: a non-pull-request subject produced a number"); + } + + const entries = parseLogEntries("abc\t2026-08-14T06:00:11+08:00\tMerge pull request #10 from o/b\n"); + if (entries.length !== 1 || entries[0].pullNumber !== 10 || entries[0].sha !== "abc") { + throw new Error("self-test failed: log parsing is incorrect"); + } + + const blobs = new Map([ + ["pre:kept.ts", "aaa"], + ["head:kept.ts", "bbb"], + ["pre:lost.ts", "ccc"], + ["head:lost.ts", "ccc"], + ["head:deleted.ts", null], + ["pre:deleted.ts", "ddd"], + ]); + const blobAt = (ref, file) => blobs.get(`${ref}:${file}`) ?? null; + const { findings } = classifyMergeLoss({ + ref: "head", + blobAt, + landings: [ + { sha: "s1", date: "d", subject: "x (#1)", pullNumber: 1, preRef: "pre", files: ["kept.ts"] }, + { sha: "s2", date: "d", subject: "y (#2)", pullNumber: 2, preRef: "pre", files: ["lost.ts", "kept.ts"] }, + { sha: "s3", date: "d", subject: "z (#3)", pullNumber: 3, preRef: "pre", files: ["deleted.ts"] }, + { sha: "s4", date: "d", subject: "w (#4)", pullNumber: 4, preRef: "pre", files: ["added.ts"] }, + ], + }); + const byPull = new Map(findings.map((finding) => [finding.pullNumber, finding])); + if (byPull.has(1)) throw new Error("self-test failed: a surviving change was reported as lost"); + if (byPull.get(2)?.revertedFiles.length !== 1) throw new Error("self-test failed: a reverted file was not reported"); + if (byPull.has(3)) throw new Error("self-test failed: a surviving deletion was reported as lost"); + if (!byPull.get(4)) throw new Error("self-test failed: a vanished added file was not reported"); + + const request = `${INBOX}/11111111-1111-4111-8111-111111111111.json`; + const reconciled = classifyMergeLoss({ + ref: "head", + blobAt: (reference, file) => + reference === "head" && file === `${INBOX}/applied/${path.posix.basename(request)}` ? "eee" : null, + landings: [{ sha: "s5", date: "d", subject: "q (#5)", pullNumber: 5, preRef: "pre", files: [request] }], + }); + if (reconciled.findings.length !== 0 || reconciled.filesExempted !== 1) { + throw new Error("self-test failed: a reconciled inbox request was reported as a merge loss"); + } + console.log("merge-loss audit self-test passed."); +} + +function main() { + if (process.argv.includes("--self-test")) return selfTest(); + const options = resolveArgs(process.argv.slice(2)); + + if (tryGit(["rev-parse", "--is-shallow-repository"]) === "true") { + console.error("[merge-loss] this is a shallow clone; pre-merge parents are unavailable and a clean sweep here"); + console.error("[merge-loss] would be meaningless. Re-run after `git fetch --unshallow`."); + process.exitCode = 1; + return; + } + if (tryGit(["rev-parse", "--verify", "--quiet", `${options.ref}^{commit}`]) === undefined) { + console.error(`[merge-loss] cannot resolve ref "${options.ref}"; fetch it or pass --ref .`); + process.exitCode = 1; + return; + } + + const landings = collectLandings(options.ref, options.since); + const result = classifyMergeLoss({ landings, blobAt: blobReader(), ref: options.ref }); + if (options.json) { + console.log(JSON.stringify({ ref: options.ref, sinceDays: options.since, ...result }, null, 2)); + process.exitCode = options.strict && result.findings.length > 0 ? 1 : 0; + return; + } + process.exitCode = report(result, options); +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + try { + main(); + } catch (error) { + console.error(`[merge-loss] failed: ${error instanceof Error ? error.message : String(error)}`); + process.exitCode = 1; + } +} diff --git a/tests/merge-loss-audit.test.ts b/tests/merge-loss-audit.test.ts new file mode 100644 index 0000000000..9a6ca9443c --- /dev/null +++ b/tests/merge-loss-audit.test.ts @@ -0,0 +1,132 @@ +import { describe, expect, it } from "vitest"; + +import { + classifyMergeLoss, + isReconciliationMove, + parseLogEntries, + parsePullNumber, +} from "../scripts/audit-merge-loss.mjs"; + +const INBOX = "docs/outstanding-issues-inbox"; + +/** Blob table keyed `:`; a missing key means the path does not exist. */ +const reader = (blobs: Record) => (ref: string, file: string) => blobs[`${ref}:${file}`] ?? null; + +const landing = (pullNumber: number, files: string[]) => ({ + sha: `sha${pullNumber}`, + date: "2026-08-11T00:00:00+00:00", + subject: `subject (#${pullNumber})`, + pullNumber, + preRef: "pre", + files, +}); + +describe("merge-loss subject parsing", () => { + it("reads a merge landing", () => { + expect(parsePullNumber("Merge pull request #1935 from BigSimmo/codex/fix-ecg")).toBe(1935); + }); + + it("reads a squash landing", () => { + expect(parsePullNumber("ci: speed iteration without weakening gates (#1926)")).toBe(1926); + }); + + it("prefers the trailing number when the subject also cites an issue", () => { + // GitHub appends the PR number, so the last parenthesised number wins. + expect(parsePullNumber("docs(issues): close #170 and (#309) partially (#1925)")).toBe(1925); + }); + + it("returns undefined for a commit that is not a pull request landing", () => { + expect(parsePullNumber("Merge branch 'main' into claude/feature")).toBeUndefined(); + expect(parsePullNumber("wip")).toBeUndefined(); + expect(parsePullNumber("")).toBeUndefined(); + }); + + it("parses tab-delimited log lines and keeps a subject containing tabs", () => { + const entries = parseLogEntries("abc\t2026-08-14T06:00:11+08:00\tfix: thing\there (#12)\n\n"); + expect(entries).toHaveLength(1); + expect(entries[0]).toMatchObject({ sha: "abc", pullNumber: 12 }); + expect(entries[0].subject).toBe("fix: thing\there (#12)"); + }); +}); + +describe("merge-loss classification", () => { + it("does not flag a file whose change is still present", () => { + const blobAt = reader({ "pre:kept.ts": "aaa", "head:kept.ts": "bbb" }); + expect(classifyMergeLoss({ ref: "head", blobAt, landings: [landing(1, ["kept.ts"])] }).findings).toEqual([]); + }); + + it("flags a file that reverted to its pre-merge blob", () => { + // The acf78bf case: the landing's contribution to this file is gone. + const blobAt = reader({ "pre:lost.ts": "ccc", "head:lost.ts": "ccc" }); + const { findings } = classifyMergeLoss({ ref: "head", blobAt, landings: [landing(1803, ["lost.ts"])] }); + expect(findings).toHaveLength(1); + expect(findings[0].pullNumber).toBe(1803); + expect(findings[0].revertedFiles).toEqual([{ file: "lost.ts", absent: false }]); + }); + + it("flags a file the pull request added that is absent again", () => { + // Absent before and absent now: the addition was undone. + const { findings } = classifyMergeLoss({ ref: "head", blobAt: reader({}), landings: [landing(1, ["added.ts"])] }); + expect(findings[0].revertedFiles).toEqual([{ file: "added.ts", absent: true }]); + }); + + it("does not flag a deletion that survived", () => { + // Present before, absent now — the pull request deleted it and it stayed deleted. + const blobAt = reader({ "pre:removed.ts": "ddd" }); + expect(classifyMergeLoss({ ref: "head", blobAt, landings: [landing(1, ["removed.ts"])] }).findings).toEqual([]); + }); + + it("skips commits with no pull request number instead of dropping them silently", () => { + const result = classifyMergeLoss({ + ref: "head", + blobAt: reader({}), + landings: [ + { sha: "x", date: "d", subject: "Merge branch 'main'", pullNumber: undefined, preRef: "pre", files: [] }, + ], + }); + expect(result.findings).toEqual([]); + expect(result.skipped).toHaveLength(1); + expect(result.scannedLandings).toBe(0); + }); + + it("orders findings by how much of the landing is missing", () => { + const blobAt = reader({ "pre:a.ts": "1", "head:a.ts": "1", "pre:b.ts": "2", "head:b.ts": "2" }); + const { findings } = classifyMergeLoss({ + ref: "head", + blobAt, + landings: [landing(10, ["a.ts"]), landing(20, ["a.ts", "b.ts"])], + }); + expect(findings.map((finding) => finding.pullNumber)).toEqual([20, 10]); + }); +}); + +describe("merge-loss reconciliation exemption", () => { + const request = `${INBOX}/11111111-1111-4111-8111-111111111111.json`; + const applied = `${INBOX}/applied/11111111-1111-4111-8111-111111111111.json`; + + it("does not report an inbox request that reconcile moved to applied/", () => { + // issues:reconcile moves the request verbatim; that is its lifecycle, not a loss. + const result = classifyMergeLoss({ + ref: "head", + blobAt: reader({ [`head:${applied}`]: "eee" }), + landings: [landing(1915, [request])], + }); + expect(result.findings).toEqual([]); + expect(result.filesExempted).toBe(1); + }); + + it("still reports an inbox request that vanished without an audit record", () => { + // No applied/ counterpart means the request was genuinely lost. + const result = classifyMergeLoss({ ref: "head", blobAt: reader({}), landings: [landing(1915, [request])] }); + expect(result.findings).toHaveLength(1); + expect(result.filesExempted).toBe(0); + }); + + it("credits the move only for a matching request filename", () => { + const blobAt = reader({ [`head:${applied}`]: "eee" }); + expect(isReconciliationMove(request, "head", blobAt)).toBe(true); + expect(isReconciliationMove(`${INBOX}/22222222-2222-4222-8222-222222222222.json`, "head", blobAt)).toBe(false); + expect(isReconciliationMove("src/lib/rag/rag.ts", "head", blobAt)).toBe(false); + expect(isReconciliationMove(applied, "head", blobAt)).toBe(false); + }); +}); From c8e3be41c71473207a669847251c2dd1d2afcef1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 10:48:09 +0000 Subject: [PATCH 03/15] docs(typescript): plan the staged noUncheckedIndexedAccess migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ledger #211 says to plan and start the migration, and carries an explicit Stop against flipping the flag on main without a staged plan. This is that plan; it changes no code and does not touch tsconfig.json. Re-measured against main at d47aa6d rather than reusing the 2026-08-02 numbers: 1,445 errors across 269 files, up from 1,266. The flag is off and nothing stops new unchecked indexing from landing, which is itself the argument for a ratchet. The measurement reshapes the job. Two-thirds of the population — tests (713) plus design-scratch mockups (237) — carries no production consequence at all, so the genuinely risky remainder is around 500 errors, not 1,445. Six stages, cheapest and most consequence-free first, each flagged mechanical or manual with its own gate. The flag itself can only flip once, in the final PR: noUncheckedIndexedAccess is a whole-project option and narrowing `include` does not isolate a directory, because TypeScript still reports errors in every transitively imported file. Intermediate stages are therefore verified by a baseline ratchet in the shape the repo already uses for the design-system contract and bundle budget, so a stage cannot be undone by later merges. Stage 6 touches src/lib/rag/**, so the RAG obligations are written out rather than left to be rediscovered: flag the task before editing, carry an accurate RAG impact line, and treat any ordering change as needing a live canary. Proposed-but-unbuilt artefacts are named without a directory prefix so docs:check-links and docs:check-scripts do not read them as stale references. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- docs/README.md | 1 + ...unchecked-indexed-access-migration-plan.md | 212 ++++++++++++++++++ 2 files changed, 213 insertions(+) create mode 100644 docs/no-unchecked-indexed-access-migration-plan.md diff --git a/docs/README.md b/docs/README.md index 34779ed651..9ee51fa71a 100644 --- a/docs/README.md +++ b/docs/README.md @@ -93,6 +93,7 @@ npm run docs:check-links ## Plans and workstreams (living) - [maturity-backlog-workorders.md](maturity-backlog-workorders.md) — actionable work orders tracking the repository-maturity audit backlog +- [no-unchecked-indexed-access-migration-plan.md](no-unchecked-indexed-access-migration-plan.md) — staged multi-PR rollout for the `noUncheckedIndexedAccess` TypeScript flag (ledger `#211`) - [framework-dependency-modernization-checklist.md](framework-dependency-modernization-checklist.md) — ordered Next.js 16, runtime, dependency, Turbopack, and verification migration program - [search-rag-master-plan.md](search-rag-master-plan.md) / [search-rag-master-context.md](search-rag-master-context.md) — search/RAG roadmap and shared context - [rag-improvement/README.md](rag-improvement/README.md) — reviewed/updated RAG improvement programme: answer-quality track (intent-aware related information, length) + corrected eval/safety infra track diff --git a/docs/no-unchecked-indexed-access-migration-plan.md b/docs/no-unchecked-indexed-access-migration-plan.md new file mode 100644 index 0000000000..52cf9870fb --- /dev/null +++ b/docs/no-unchecked-indexed-access-migration-plan.md @@ -0,0 +1,212 @@ +# `noUncheckedIndexedAccess` — staged migration plan + +**Status:** plan only — this document changes no code and does not touch `tsconfig.json` +**Ledger row:** `#211` (P2, task) · related `#212` (`as unknown as` casts), `#213` (empty catch handlers) +**Measured:** 2026-08-14 against `origin/main` at `d47aa6d` +**Source finding:** [`docs/review-findings-2026-08-02.md`](review-findings-2026-08-02.md) §6 + +`tsconfig.json` sets `strict: true` but not `noUncheckedIndexedAccess`. Without the flag, +`array[0]` is typed `T` even when the array is empty and `record[key]` is typed `V` even when +the key is absent, so every out-of-bounds read is invisible to the compiler and surfaces as a +runtime `undefined` — including on the answer path, where the failure lands in front of a +clinician. + +Row `#211` carries an explicit **Stop:** do not flip the flag on `main` without a staged plan. +This is that plan. + +--- + +## 1. Current measurement + +Measured by extending `tsconfig.json` with `noUncheckedIndexedAccess: true` in a throwaway +config outside the repo tree and running `tsc --noEmit`. **1,445 errors across 269 files** — +up from the 1,266 recorded on 2026-08-02, because the flag is off and nothing stops new +unchecked indexing from landing. That drift rate is itself an argument for the ratchet in §3. + +| Bucket | Errors | Share | Character | +| ------------------------ | -----: | ----: | --------------------------------------------------------------------- | +| `tests/**` | 713 | 49.3% | Mechanical. A wrong guard fails a test, it does not reach production. | +| Mockups (design scratch) | 237 | 16.4% | Mechanical. 404s in production; already gate-exempt for wiring. | +| `src/lib/**` (non-RAG) | 211 | 14.6% | Mixed — contains the clinical hot spots. | +| `src/components/**` | 167 | 11.6% | Mostly mechanical render-path indexing. | +| `scripts/**` | 57 | 3.9% | Mechanical; tooling-plane, failures are loud and local. | +| `src/lib/rag/**` | 32 | 2.2% | **Protected surface.** See §5. | +| `worker/**` | 27 | 1.9% | Manual — ingestion runtime. | +| `src/` other | 1 | 0.1% | — | + +Two-thirds of the population (tests plus mockups, 950 errors) carries no production +consequence whatever. That is what makes staging worthwhile: the risky remainder is ~500 +errors, not 1,445. + +**Error shape:** `TS2532` "object is possibly undefined" (569) and `TS18048` "…is possibly +undefined" (455) together are 71% — these are the ones a guard fixes. `TS2345`/`TS2322` +(368) are `string | undefined` flowing into a parameter typed `string`, which more often +needs a real decision about what the absent case means. + +**Heaviest files:** `tests/ui-smoke.spec.ts` (43), `src/lib/demo-data.ts` (42), +`src/components/master-document-flow-mockups.tsx` (41), `src/lib/answer-verification.ts` (41), +`tests/evidence.test.ts` (40), `tests/ui-phone-scroll-page-owned.spec.ts` (38), +`tests/clinical-search.test.ts` (28), `src/lib/rag/rag-extractive-answer.ts` (23), +`worker/main.ts` (23), `src/lib/evidence.ts` (19). + +**To reproduce:** create a config outside the repo that extends `tsconfig.json`, adds +`"noUncheckedIndexedAccess": true`, and excludes `.next` (build artefacts produce unrelated +errors), then run `./node_modules/.bin/tsc --noEmit --project `. Do not add the +throwaway config to the repo — `docs:check-links` and the tsconfig gates both notice. + +--- + +## 2. Why this cannot simply be split by directory + +`noUncheckedIndexedAccess` is a whole-project compiler option. It cannot be enabled for one +directory: narrowing `include` does not help either, because TypeScript still loads and +reports errors in every transitively imported file, so a tests-only project pulls all of +`src/lib` in with it. + +So the flag itself flips exactly once, in the final PR. Everything before that is remediation +performed against the measurement, verified by a ratchet rather than by `npm run typecheck`. + +--- + +## 3. The ratchet + +Stage 1 adds a baseline file plus a check, in the shape this repo already uses for +`scripts/design-system-contract-baseline.json` (`metrics` + `debtByPath`) and +`bundle-budget.json`: + +(Paths below are proposed, not existing — they are written without a directory prefix so the +`docs:check-links` and `docs:check-scripts` gates do not read them as stale references.) + +- A baseline file `no-unchecked-indexed-access-baseline.json` under `scripts/` — + `{ measuredOn, total, debtByPath }` mapping each file still permitted to have errors to its + current count. +- A checker `check-no-unchecked-indexed-access.mjs` under `scripts/` — runs `tsc` with the flag + against a generated config, then fails when a file **absent** from `debtByPath` has any + error, or when a listed file's count **rises**. Falling counts are fine; the baseline is + refreshed as stages land. +- A `package.json` entry named `check:no-unchecked-indexed-access`, run per stage and by the + final PR. + +This makes the migration monotonic: a stage cannot be undone by the next week's merges, and +new code cannot add debt while the migration is in flight — which is precisely what let the +count drift from 1,266 to 1,445. + +**Do not** add this to `verify:cheap:internal` while the migration is in flight. A full `tsc` +run is not a cheap gate, and `scripts/check-gate-manifest.mjs` would additionally require a +matching `static-pr` step in `.github/workflows/ci.yml`. Run it per stage; consider promoting +it only after stage 6, when the flag is on and `npm run typecheck` covers it anyway. + +--- + +## 4. Stages + +One PR per stage, in this order. Cheapest and most consequence-free first, so the mechanical +bulk lands before anyone has to think hard. + +### Stage 1 · Ratchet only — `MECHANICAL` + +- **Outcome:** the debt is measured, pinned, and cannot grow. +- **Files:** the checker and baseline named in §3, plus `package.json` and + `docs/scripts-index.md`. +- **Risk:** none — no product file changes. +- **Verification:** the new `check:no-unchecked-indexed-access` entry passes at the baseline; a + deliberately introduced `arr[0]` in a clean file makes it fail. + +### Stage 2 · `tests/**` — 713 errors — `MECHANICAL` + +- **Outcome:** roughly half the population gone, with no production surface touched. +- **Approach:** prefer non-null assertion `!` here specifically. In a test the invariant is + usually established two lines above (`const rows = parse(x); expect(rows).toHaveLength(3)`), + and a `?.` would silently weaken the assertion into a no-op — `expect(rows[0]?.id).toBe(…)` + passes vacuously when `rows` is empty. That is the one place `!` is clearly right. +- **Risk:** low, but real in exactly the way above. Reviewers check that no assertion became + vacuous. +- **Verification:** `npm run test`; the Playwright specs in this bucket + (`tests/ui-smoke.spec.ts`, `tests/ui-phone-scroll-page-owned.spec.ts`) are compiled by + `typecheck` but only executed by `npm run verify:ui`, so typecheck is the gate that matters + for them. + +### Stage 3 · Mockups — 237 errors — `MECHANICAL` + +- **Outcome:** design scratch off the books. +- **Files:** `src/app/mockups/**`, `*-mockups.tsx`. +- **Risk:** none. These 404 in production. Note they are still compiled and still weighed by + `check:bundle-budget` against the `mockups` baseline — "gate-exempt" does not mean "free". +- **Verification:** `npm run typecheck`, `npm run check:bundle-budget`. + +### Stage 4 · `scripts/**` and `src/components/**` — 224 errors — `MECHANICAL`, spot-reviewed + +- **Outcome:** the tooling plane and the render path. +- **Approach:** `??` with a sensible empty default in render code; a thrown error in scripts, + where failing loudly is correct and silence is not. +- **Risk:** low. The component work can change rendered output if a `??` default differs from + what the old `undefined` produced — check any empty-state or list-rendering change. +- **Verification:** `npm run typecheck`, `npm run test`, and `npm run verify:ui` only if a + component's rendered output actually changed. + +### Stage 5 · `worker/**` and `src/lib/**` non-clinical — `MANUAL` + +- **Outcome:** ingestion and the general library. +- **Approach:** `worker/main.ts:901-942` repeatedly indexes `preparedImage`/`image` arrays and + passes the results to functions typed `ExtractedImage`. A shorter-than-expected array throws + today; the fix is a real guard that skips or fails the job, not a `!` that preserves the + throw. `src/lib/demo-data.ts` (42) is the largest single file and is genuinely mechanical — + it is synthetic fixture data. +- **Risk:** medium. Ingestion is a background worker; a wrong guard turns a loud crash into a + silently skipped image. +- **Verification:** `npm run typecheck`, `npm run test`, plus `npm run check:production-readiness` + (ingestion is a domain change under AGENTS.md). + +### Stage 6 · Clinical hot spots, then flip the flag — `MANUAL, HIGHEST CARE` + +- **Files:** `src/lib/answer-verification.ts` (41), `src/lib/rag/rag-extractive-answer.ts` (23), + `src/lib/evidence.ts` (19), `src/lib/document-summary-formatting.ts` (16), and the remaining + `src/lib/rag/**`. +- **Approach:** every site individually. `rag-extractive-answer.ts` is the deterministic + source-only fallback used _when generation has already failed its quality gate_ — an + out-of-bounds throw there means the fallback fails too, and the user gets nothing instead of + a cited answer. `answer-verification.ts` indexes into arrays that may be empty while deciding + whether an answer is safe to show; a `!` that converts a type error into a runtime throw + crashes the verification gate itself. Neither file wants `!` anywhere. +- **Then:** set `"noUncheckedIndexedAccess": true` in `tsconfig.json`, delete the baseline and + its check, and remove the `check:no-unchecked-indexed-access` entry. +- **Verification:** `npm run typecheck`, `npm run test`, `npm run check:production-readiness`, + and the RAG requirements in §5. + +--- + +## 5. The RAG carve-out + +Stage 6 touches `src/lib/rag/**`, which is a protected ranking surface under AGENTS.md. +Three obligations apply and none is optional: + +1. **Flag the task to the user before editing anything under `src/lib/rag/**`** — including a + change this mechanical. +2. The PR body needs an explicit `RAG impact:` line or `scripts/pr-policy.mjs` blocks the + merge. A guard that only adds a narrowing check should be able to state + `RAG impact: no retrieval behaviour change — adds undefined guards without touching +comparator order, scoring, or selection`, but that claim has to be **true**: read + `docs/rag-behaviour/` first and confirm no comparator key, clamped-score contract, or + selection threshold moved. +3. If any guard does change ordering or selection — for example a `?? 0` default that alters a + sort — it is a behaviour change and needs a live eval-canary pair. That is provider-backed + (~$1–2) and needs explicit user approval. + +Splitting `src/lib/rag/**` into its own final PR, after the rest of stage 6, keeps the +governance requirement off the other files. + +--- + +## 6. Tracking + +Progress lives in ledger row `#211`, updated with `npm run issues:update` after each stage +lands (never by hand-editing `docs/outstanding-issues.md`). Record the stage number, the PR, +and the new total from the baseline file, so a later reader can tell how far the migration got +without re-running `tsc`. + +Do not close `#211` until the flag is on in `tsconfig.json` and the baseline file is gone. +A partially-migrated repo with the flag still off has none of the protection and all of the +churn, so an abandoned migration is worse than an unstarted one. + +**Stop:** do not flip the flag on `main` ahead of stage 6, and do not silence a stage by adding +files back to the baseline — the baseline only ever shrinks. From a95c43838a707206552962f30396258f5bbf2634 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 10:49:55 +0000 Subject: [PATCH 04/15] docs(ledger): propose a collision-free outstanding-issue id scheme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ledger #168 records that sequential ids force every concurrent append to conflict: ids are allocated read-modify-write against the issues:next-id marker inside the file being edited, so two branches both read N and both write N. Duplicates are unacceptable, so a union driver is unsafe, so every overlapping append is resolved by hand — and manual resolution is where rows get dropped (PR #1490 and #152; four renumbers on PR #1451; the Update-branch head that carried two #141 rows and left the marker below main's highest id). Design only, no implementation. Recommends a ULID as the durable id with a short derived display form, so a row stays sayable in a handoff. The property that matters is that the display form is derived rather than stored: a clash there is a rendering fix, not a renumber. Notes UUIDv7 as an equally good fit, and records why timestamp+slug and content hashes were rejected. The migration is additive because the 314 existing ids keep their numbers permanently — they are cited across the ledger, the review records, the agent instructions and the commit history, and renumbering them would produce exactly the churn this row exists to end. Four steps, widening validators before allocation changes, with every current #NNN assumption enumerated by file and symbol. Also states what this does not fix, so the three ledger rows stay distinct: #292 is a collision on the work rather than the id, and #156's silently dropped prose blocks stay invisible to check:outstanding-issues either way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- docs/README.md | 1 + docs/ledger-id-scheme-proposal.md | 148 ++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 docs/ledger-id-scheme-proposal.md diff --git a/docs/README.md b/docs/README.md index 9ee51fa71a..5cb479cc9d 100644 --- a/docs/README.md +++ b/docs/README.md @@ -94,6 +94,7 @@ npm run docs:check-links - [maturity-backlog-workorders.md](maturity-backlog-workorders.md) — actionable work orders tracking the repository-maturity audit backlog - [no-unchecked-indexed-access-migration-plan.md](no-unchecked-indexed-access-migration-plan.md) — staged multi-PR rollout for the `noUncheckedIndexedAccess` TypeScript flag (ledger `#211`) +- [ledger-id-scheme-proposal.md](ledger-id-scheme-proposal.md) — design for collision-free outstanding-issue ids so concurrent sessions stop contending on `issues:next-id` (ledger `#168`) - [framework-dependency-modernization-checklist.md](framework-dependency-modernization-checklist.md) — ordered Next.js 16, runtime, dependency, Turbopack, and verification migration program - [search-rag-master-plan.md](search-rag-master-plan.md) / [search-rag-master-context.md](search-rag-master-context.md) — search/RAG roadmap and shared context - [rag-improvement/README.md](rag-improvement/README.md) — reviewed/updated RAG improvement programme: answer-quality track (intent-aware related information, length) + corrected eval/safety infra track diff --git a/docs/ledger-id-scheme-proposal.md b/docs/ledger-id-scheme-proposal.md new file mode 100644 index 0000000000..15ad313760 --- /dev/null +++ b/docs/ledger-id-scheme-proposal.md @@ -0,0 +1,148 @@ +# Collision-free outstanding-issue ids — design proposal + +**Status:** design only — no implementation, no id allocated by this document +**Ledger row:** `#168` (P2, rec) · closely related `#156` (same race, resolution-path evidence) +**Distinct from:** `#292`, which is two sessions colliding on the **work** a row describes. A +collision-free id leaves that untouched. +**Measured:** 2026-08-14 against `origin/main` at `d47aa6d` — 314 rows, marker at `next-id=317` + +--- + +## 1. The problem + +Ids are allocated read-modify-write against the `issues:next-id` marker **inside the very file +being edited**. `scripts/outstanding-issues.mjs` reads the marker, claims that number, and +rewrites the marker to `N + 1`. Two branches open at the same time both read `N` and both +write `N`. + +Because duplicate ids are unacceptable, a union merge driver is unsafe — `.gitattributes` says +so explicitly, which is why this file deliberately has no driver and **every overlapping append +conflicts by hand**. + +Manual resolution is where rows get dropped. The record is specific: + +- PR #1490 was closed during a conflict resolution and took the only record of four snapshots + with it (`#152`). +- One P3 row was renumbered `#135` → `#141` → `#145` → `#147` → `#149` across four sync cycles + because `main` had taken each id in turn (`#156`, measured on PR #1451). +- `#168` itself was written as `#159`, then renumbered because `main` had already used `#159`. +- The GitHub **Update branch** button produced a head carrying **two rows numbered `#141` and + two `next-id` markers**, leaving the marker _below_ `main`'s highest id — so the next + allocation would have reused a live number. `git merge` reported success; only + `npm run check:outstanding-issues` caught it (`#156`). + +The inbox (`scripts/ledger-inbox.mjs`) removed the mechanical errors — requests are immutable +UUID-named files and only `npm run issues:reconcile` writes the canonical ledger — but it +explicitly did not remove this one. Reconciliation still allocates from the marker, so two +reconcile branches still contend, and the single-writer discipline is what makes that +tolerable rather than fixed. + +--- + +## 2. What the id has to do + +Any scheme has to satisfy four things at once, which is why the obvious answers are wrong: + +1. **Collision-free without coordination.** Two sessions that never see each other must not + produce the same id. +2. **Stable once written.** Ids are cited by other rows, by review records under + `docs/branch-review-records/`, by `AGENTS.md`, by `.claude/skills/issues/SKILL.md`, and by + commit messages and PR bodies across the repo's history. An id that can be renumbered is the + defect, not the format. +3. **Readable enough to say aloud.** `/issues` output, the `SessionStart` hook, and every + handoff summary read ids back to a human. `#151` works in conversation; a bare + `01JQ8ZK3M7Q9V2W4X6Y8Z0ABCD` does not. +4. **Sortable by creation.** The ledger's queue and archive both read better in the order the + work arrived. + +--- + +## 3. Recommendation — ULID stored, short prefix displayed + +Allocate a **ULID** as the durable id, and render a **short display form** derived from it. + +- **ULID**, not UUIDv4, because a ULID is lexicographically sortable by its millisecond + timestamp prefix — requirement 4 — while remaining collision-free without coordination. + UUIDv7 is an equally good fit if a dependency is preferred over ~20 lines of local code; the + repo already generates UUIDv4 via `randomUUID()` in `ledger-inbox.mjs`, so neither needs a + new package. +- **Display form** is the first 6 characters of the ULID's random suffix, rendered `#K3M7Q9`. + Six Crockford base-32 characters is ~1.07 billion values; at the repo's observed rate of + roughly 320 rows in a year, a birthday collision is negligible, and the checker can pin + display uniqueness anyway and lengthen the prefix if one ever occurs. +- **The display form is derived, never stored as the identity.** That is the property that + makes it safe: a collision in the _display_ form is a rendering problem fixed by taking one + more character, not a data problem requiring a renumber. + +Rejected alternatives, briefly: + +- **Timestamp + slug** (`#2026-08-14-merge-loss`) is readable and sortable, but two sessions + filing similar rows on the same day collide on the slug, and the slug wants to change when + the row is re-scoped — reintroducing renumbering by another name. +- **Content hash** is collision-free but neither sortable nor stable: any edit to the row + changes its identity. +- **Keeping sequential ids and adding a lock** does not work across branches. There is no + shared state at allocation time; that is the whole problem. + +--- + +## 4. Migration path + +**The 314 existing sequential ids keep their literal ids, permanently.** Renumbering them is +off the table — they are cited across the ledger, the review records, the agent instructions +and the entire commit history, and a rewrite would invalidate every one of those citations +while producing exactly the renumbering churn this row exists to end. + +So the two forms coexist, and the migration is additive: + +| Step | Change | Risk | +| ---- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | +| 1 | Widen every id validator to accept both `#NNN` and the new display form, while allocation still uses the marker. No behaviour change; purely permissive. | Low. Fully reversible. | +| 2 | Add the ULID column to new rows and switch allocation to it. The `issues:next-id` marker stops being read. | Medium — this is the cutover. | +| 3 | Remove the marker and its `next-id` guards once no writer consults it. | Low, but only after step 2 has been through a few real appends. | +| 4 | Reconsider a union merge driver, which becomes safe only once **no** id is allocated read-modify-write. | Deliberately last. See the Stop below. | + +**Every place that currently assumes a sequential id** — all of these need step 1 before +anything else moves: + +- `scripts/ledger-inbox.mjs` — `/^#\d{3,}$/` in `validateRequest`, twice (the `done` and + `update` actions). +- `scripts/check-outstanding-issues.mjs` — `ID_CELL = /^#\d+$/`; the + `MARKER = //` parse; the `nextId <= highest` assertion; and + the `String(highest).padStart(3, "0")` formatting in its messages. +- `scripts/outstanding-issues.mjs` — the allocator that reads `parsed.nextId`, formats + `#${String(number).padStart(3, "0")}`, and rewrites the marker to `nextId + 1`. +- `scripts/issues-report.mjs` and `.claude/hooks/issues-surface.sh`, which render ids back to + the reader. + +A row-per-file variant — one file per row in a new per-row directory under `docs/`, with the +table generated the way `docs/site-map.md` already is — removes the shared hunk entirely and is +the stronger end state. It is a larger change and should be decided separately; the id scheme +is a prerequisite for it either way, since per-row filenames need collision-free names. + +--- + +## 5. What this does not fix + +`#292` — two sessions independently building the same queued item — is untouched by any of +this. That is a collision on the **work** a row describes, not on its id, and the mitigation +there is the open-PR check already written into the three skills. Do not conflate them when +scoping the implementation. + +`#156`'s second finding is also untouched: a merge that silently drops an appended prose block +is invisible to `check:outstanding-issues`, which validates ids and structure rather than +whether both sides' text survived. A collision-free id makes such merges rarer; it does not +make them detectable. `npm run audit:merge-loss` is the closest thing the repo now has to that +detection. + +--- + +## 6. Stop + +- **Do not reinstate `merge=union` while ids are sequential.** That combination was tried in + PR #1416 and removed for duplicating rows and the marker. It only becomes safe after step 3. +- **Do not renumber existing rows** to make the ledger uniform. The citations are the point. +- **Do not implement this from this document alone.** It is a proposal; the cutover in step 2 + wants its own PR, its own review, and a check that both id forms round-trip through + `issues:add`, `issues:update`, `issues:done` and `issues:reconcile` before the marker is + removed. From d36914a79fa4304273429aab0d7f271010f3bb96 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 10:51:11 +0000 Subject: [PATCH 05/15] docs(process): record the cross-agent PR-handoff stop-rule gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ledger #258 records that the stop rule is enforced for Claude Code only. This documents the gap; it deliberately builds no mechanism, which is a separate and larger piece of work. Three parts. Where the enforcement lives: .claude/hooks/pr-handoff-stop.sh registered in .claude/settings.json, its two matchers, its three deny classes (shell PR/CI polling, GitHub MCP tools matched by name, and the loop machinery), and the design details a second implementation would have to match — the session-scoped marker under the absolute git dir, failing open on an unidentifiable session, never pruning a sibling's marker, and never letting a tool's output arm the marker. What Codex and Cursor have, checked rather than assumed: the AGENTS.md prose alone. .claude/settings.json is read only by Claude Code; the Codex plugin manifest declares skills and an interface block with no hook or interception field; .cursor/ holds settings, mcp and agent/skill files with no deny path. Worth noting that .cursor/agents/pr-babysit.md exists at all — Cursor has a documented agent for exactly the behaviour the rule restricts, unbounded. And what parity would require: the three questions any mechanism must answer, plus the honest note that the wrapper fallback is advisory and only makes a violation detectable after the fact. Detection is not prevention. Carries both of the row's Stop conditions: do not weaken the Claude Code hook for symmetry, and do not keep a second copy of the deny list. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- docs/README.md | 1 + docs/pr-handoff-stop-cross-agent-gap.md | 95 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 docs/pr-handoff-stop-cross-agent-gap.md diff --git a/docs/README.md b/docs/README.md index 5cb479cc9d..98666d3134 100644 --- a/docs/README.md +++ b/docs/README.md @@ -95,6 +95,7 @@ npm run docs:check-links - [maturity-backlog-workorders.md](maturity-backlog-workorders.md) — actionable work orders tracking the repository-maturity audit backlog - [no-unchecked-indexed-access-migration-plan.md](no-unchecked-indexed-access-migration-plan.md) — staged multi-PR rollout for the `noUncheckedIndexedAccess` TypeScript flag (ledger `#211`) - [ledger-id-scheme-proposal.md](ledger-id-scheme-proposal.md) — design for collision-free outstanding-issue ids so concurrent sessions stop contending on `issues:next-id` (ledger `#168`) +- [pr-handoff-stop-cross-agent-gap.md](pr-handoff-stop-cross-agent-gap.md) — why the PR-handoff stop rule is hook-enforced for Claude Code but prose-only for Codex and Cursor, and what parity would require (ledger `#258`) - [framework-dependency-modernization-checklist.md](framework-dependency-modernization-checklist.md) — ordered Next.js 16, runtime, dependency, Turbopack, and verification migration program - [search-rag-master-plan.md](search-rag-master-plan.md) / [search-rag-master-context.md](search-rag-master-context.md) — search/RAG roadmap and shared context - [rag-improvement/README.md](rag-improvement/README.md) — reviewed/updated RAG improvement programme: answer-quality track (intent-aware related information, length) + corrected eval/safety infra track diff --git a/docs/pr-handoff-stop-cross-agent-gap.md b/docs/pr-handoff-stop-cross-agent-gap.md new file mode 100644 index 0000000000..2e201d7942 --- /dev/null +++ b/docs/pr-handoff-stop-cross-agent-gap.md @@ -0,0 +1,95 @@ +# The PR-handoff stop rule is enforced for Claude Code only — a documented gap + +**Status:** gap documentation only — this document builds no mechanism +**Ledger row:** `#258` (P2, rec) +**Checked:** 2026-08-14 against `origin/main` at `d47aa6d` +**Rule it backs:** `AGENTS.md` → "Stop when the pull request is open" + +Opening a pull request is the end of a session's handoff, not the start of a supervision +shift. A session that stays attached to its own PR — polling `gh pr checks`, watching workflow +runs, re-running failed jobs, re-syncing the branch, answering review bots, or parking a +wake-up on it — spends a long tail of usage on work nobody asked for. Claude Code on the web is +the worst case, because the cloud session keeps running and nothing naturally ends the loop. + +PR #1649 addressed that with two things: the `AGENTS.md` prose, and a Claude Code hook. **Only +one of the three agents this repo supports gets the hook.** This document records where the +enforcement lives, what the other two actually have, and what parity would require — so the +gap is a known limit rather than an open task that looks unstarted. + +--- + +## 1. What Claude Code has + +`.claude/hooks/pr-handoff-stop.sh` (259 lines), registered in `.claude/settings.json` as two +matchers: + +| Phase | Matcher | Effect | +| ------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | +| `PostToolUse` | `Bash`, `PowerShell`, and any tool whose name matches `create_pull_request` | On a call that returns a real PR URL, drops a session-scoped marker and tells the model the handoff is over. | +| `PreToolUse` | `Bash`, `PowerShell`, `Monitor`, `ScheduleWakeup`, `CronCreate`, plus tool names matching `pull_request` / `workflow_(run\|job)` / `check_(run\|suite)` / `job_log` / `pr_status` / `update_branch` | While that marker exists, denies the call with a reason naming the AGENTS.md rule. | + +Three deny classes, which is the useful summary of what "enforced" means here: + +1. **Shell polling** — `gh pr checks|status|view|diff|list|comment|review`, `gh run watch|view|list|rerun|download`, `gh api …actions/runs|check-runs|check-suites|/pulls/`, and `sync:pr-branches`. +2. **GitHub MCP PR/CI tools**, matched by tool name, so a connector is not a way around the shell rule. +3. **Loop machinery** — `Monitor`, `ScheduleWakeup`, `CronCreate`, which is how a session parks itself on a PR without running a single command. + +Committing, pushing, ledger appends, and PR create/merge stay allowed throughout. + +Details that matter to anyone reproducing this elsewhere: + +- **The marker is session-scoped and durable**, at `/claude-pr-handoff-`, falling back to `TMPDIR` outside a repo. Using the absolute git dir keeps it valid from any cwd and in linked worktrees. +- **It fails open on a missing or unsafe `session_id`** (`^[A-Za-z0-9_-]+$`), rather than sharing one marker across unrelated malformed payloads — path injection included. +- **Sibling sessions' markers are deliberately never pruned.** Post-mode runs on every shell call, so age-based deletion of other sessions' files would disarm a long-lived handoff session that only uses `Read`/`Edit` after opening its PR. +- **Post-mode scans only the request half of the payload**, never `tool_response`, so a command that merely _prints_ `gh pr create` and a PR URL cannot lock the session. Pre-mode deliberately scans the whole payload, because over-blocking is the safe direction there. +- **The escape hatch is explicit and user-driven**: prefix a shell command with `CLAUDE_ALLOW_PR_FOLLOW=1`, or delete the marker the deny reason names. A command that merely mentions a blocked token cannot self-authorise — the prefix must be at the start of the command. +- Sessions that never create a PR are untouched, so `Run PR` sweeps, `pr-ci-fix` work, and review sessions on someone else's PR still function. + +--- + +## 2. What Codex and Cursor have + +**The `AGENTS.md` prose, and nothing else.** Checked, rather than assumed: + +- `.claude/settings.json` is read only by Claude Code. Its `PreToolUse` / `PostToolUse` registrations are invisible to the other two agents, so the marker is never dropped and no call is ever denied for them. +- `plugins/clinical-kb/.codex-plugin/plugin.json` declares `name`, `version`, `description`, `author`, `repository`, `keywords`, `skills` and an `interface` block. **There is no hook, event, or pre-tool-interception field**, and the plugin ships exactly one skill (`skills/clinical-kb-workflow/SKILL.md`). A Codex session reads guidance; nothing intercepts its tool calls. +- `.cursor/` contains `settings.json` (plugin enablement only — `context7-plugin`, `figma`), `mcp.json`, `agents/` (`design-review.md`, `pr-babysit.md`, `pr-bugbot.md`) and `skills/`. **No deny path.** Note that `.cursor/agents/pr-babysit.md` exists at all: Cursor has a documented agent for exactly the PR-following behaviour the stop rule restricts, with nothing to bound it. + +The consequence is precise, and it is worth stating plainly because it is easy to read the +hook's existence as though the problem were solved: **prose alone is what was already in force +before PR #1649, and it was already insufficient — that insufficiency is why the hook was +built.** The cost was not removed; it was relocated to whichever agent lacks the gate. A cloud +Codex session is the worst case, for the same reason Claude Code on the web was. + +--- + +## 3. What a cross-agent mechanism would need + +Any parity mechanism has to answer the same three questions the hook answers: + +1. **Has this session already opened a PR?** Requires a durable, session-scoped marker written at the moment a PR-creating call returns a real PR URL — not at the moment one is attempted, since a failed create would otherwise end the session with no PR to hand over. +2. **Is this call one of the three deny classes?** Shell PR/CI polling, PR/CI tool calls by name, and loop machinery. Matching must cover the connector path as well as the shell, or the rule is trivially bypassed. +3. **Has the user explicitly asked to follow the PR anyway?** There must be an unlock, it must be user-driven, and a call must not be able to self-authorise by merely mentioning the unlock token. + +Plus three properties the existing hook already got right and a second implementation would +have to match: fail open on an unidentifiable session; never prune a sibling session's marker; +and never let a tool's _output_ arm the marker. + +**Cheapest first**, per `#258`'s own next step: check whether Codex or Cursor has since exposed +any pre-tool interception this repo can register — Codex plugin hooks under +`plugins/clinical-kb/`, Cursor rules or agent configuration under `.cursor/`. As of this +document, neither manifest exposes one. + +**If no deny path exists**, the fallback the row proposes is a shared marker file plus a wrapper +that agents are instructed to route `gh` through. That is strictly weaker — it is advisory, an +agent can call `gh` directly, and it cannot touch the MCP-connector or loop-machinery classes at +all — but a wrapper can _log_, which makes a violation detectable after the fact rather than +invisible. Detection is not prevention, and a design that claims otherwise should be rejected. + +--- + +## 4. Stop + +- **Do not weaken the Claude Code hook to make the three agents symmetric.** Removing working enforcement to achieve uniformity trades a real control for the appearance of one. +- **Do not add a second copy of the deny list.** One script, multiple registrations. Two lists drift, and the drift is silent — the copy that falls behind still looks like enforcement. +- **Do not close `#258` on the strength of this document.** The gap is now recorded rather than open-and-unexamined, but it is still a gap: two of three agents remain prose-only. Re-check the Codex and Cursor manifests when either tool ships hook support, and close the row only when a mechanism exists or the limit is accepted deliberately. From fa0c53374ac34469b673db0fbaa1dfb50ffa039d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 11:09:02 +0000 Subject: [PATCH 06/15] docs(ledger): record the review for PR #1944 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- ...e68d1e7359bb28e8b83ea9adacc348db610be6494d50d6bd842.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/39058fcc89165e68d1e7359bb28e8b83ea9adacc348db610be6494d50d6bd842.record.md diff --git a/docs/branch-review-records/39058fcc89165e68d1e7359bb28e8b83ea9adacc348db610be6494d50d6bd842.record.md b/docs/branch-review-records/39058fcc89165e68d1e7359bb28e8b83ea9adacc348db610be6494d50d6bd842.record.md new file mode 100644 index 0000000000..ebbccb96c3 --- /dev/null +++ b/docs/branch-review-records/39058fcc89165e68d1e7359bb28e8b83ea9adacc348db610be6494d50d6bd842.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/ledger-process-tooling-50uqfc | d36914a79fa4304273429aab0d7f271010f3bb96 | ledger process tooling bundle (#313 dirty-tree gate, 829597d4 merge-loss detector, #211/#168/#258 plans) | PR #1944 — five separate commits, each independently revertible. Two code items: check-ledger-write-discipline.mjs now refuses a verdict while a governed ledger path is dirty (gated on default HEAD so guard-push.mjs explicit --head and clean CI are unaffected); new scripts/audit-merge-loss.mjs advisory blob-comparison sweep, exits 0 and asks for human confirmation, not wired into CI or verify:cheap by design. Three docs-only plans. Found and fixed during build: the shared git() helper trim() was corrupting porcelain status parsing, and JSDoc types were needed to stop TS inferring never[] for the classifier landings parameter. Noted: classifyPullRequestFiles reports operationalRisk true solely from the package.json script entry. | npm run verify:pr-local — 18 gates completed, 1 failed: check:medication-lexicon-report, reproduced identically in a clean detached worktree at origin/main d47aa6d so pre-existing and not caused here; tests/ledger-write-discipline.test.ts 9 passed; tests/merge-loss-audit.test.ts 14 passed; both script self-tests passed; audit:merge-loss --since 6 independently rediscovered the acf78bf casualties #1803/#1800/#1804/#1796/#1811 | From 9fae228040dc0ad64c793ce7f78fe17ccd10d251 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 11:15:11 +0000 Subject: [PATCH 07/15] docs(issues): close #313 and record the state of the four other rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Queues six immutable inbox requests for the work in this PR. Ordinary branches never edit the canonical ledger, so these reconcile after it lands. - done #313: the dirty-tree refusal shipped. Records the two traps only running it surfaced (the trimmed porcelain that silently disabled the guard, and guard-push's explicit --head), and notes the row's related outstanding-issues.mjs vs issues:done confusion is NOT addressed. - update #211: the plan exists, the migration does not, so the row stays open and stays deprioritised. Carries the 2026-08-12 deprioritisation conclusion forward and corrects the count it rested on — 1,445 across 269 files, not 1,266. - cancel 0e47904b: superseded by that update, which is a strict superset of it. Two pending updates on one row force a cancellation decision at reconcile regardless, so this makes the decision explicit rather than leaving it for whoever reconciles. - update #168 and #258: design and gap documentation landed; neither row is closed, because neither asked only for a document. #258's update records that its cheapest-first option is currently unavailable — checked against all three manifests, not assumed. - add: one new P2 recommendation. Two merge-loss detectors now exist and measure different things — this PR's catches a landing whose content was reverted, PR #1937's concerns a file that never landed at all — and neither covers the other's case. Also carries the undecided scheduling question this PR deliberately left open. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Chrn9bTYFFYzrLZqtpVptW --- .../71d61764-9d93-43bd-a3d3-230f5ad78418.json | 10 ++++++++++ .../83ec71cf-db94-4110-ada8-ec7e730e5154.json | 10 ++++++++++ .../a780ce8a-a373-4c95-974f-0692af775ff6.json | 10 ++++++++++ .../a8783c79-f86e-4fbf-811d-1ec3b1e05082.json | 10 ++++++++++ .../d229e6b5-a31a-44a9-8a7b-536e7f8ccf50.json | 10 ++++++++++ .../e684a311-2a0d-4c21-ba18-13afde3b62f8.json | 13 +++++++++++++ 6 files changed, 63 insertions(+) create mode 100644 docs/outstanding-issues-inbox/71d61764-9d93-43bd-a3d3-230f5ad78418.json create mode 100644 docs/outstanding-issues-inbox/83ec71cf-db94-4110-ada8-ec7e730e5154.json create mode 100644 docs/outstanding-issues-inbox/a780ce8a-a373-4c95-974f-0692af775ff6.json create mode 100644 docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json create mode 100644 docs/outstanding-issues-inbox/d229e6b5-a31a-44a9-8a7b-536e7f8ccf50.json create mode 100644 docs/outstanding-issues-inbox/e684a311-2a0d-4c21-ba18-13afde3b62f8.json diff --git a/docs/outstanding-issues-inbox/71d61764-9d93-43bd-a3d3-230f5ad78418.json b/docs/outstanding-issues-inbox/71d61764-9d93-43bd-a3d3-230f5ad78418.json new file mode 100644 index 0000000000..5d6e6b12b9 --- /dev/null +++ b/docs/outstanding-issues-inbox/71d61764-9d93-43bd-a3d3-230f5ad78418.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "71d61764-9d93-43bd-a3d3-230f5ad78418", + "createdOn": "2026-08-14", + "action": "update", + "payload": { + "id": "#258", + "detail": "GAP RECORDED 2026-08-14 in PR #1944 — docs/pr-handoff-stop-cross-agent-gap.md. This is the row's own stated fallback (\"If no mechanism exists at all, record that explicitly here so the gap is a known limit rather than an open task\"), so the row stays open but is no longer unexamined. Checked, not assumed: .claude/settings.json is read only by Claude Code; plugins/clinical-kb/.codex-plugin/plugin.json declares name/version/description/author/repository/keywords/skills and an interface block with NO hook, event, or pre-tool-interception field, shipping exactly one skill; .cursor/ holds settings.json (plugin enablement only), mcp.json, agents/ and skills/ with no deny path. So the cheapest-first option the row proposed is currently unavailable in both tools. Worth noting because it sharpens the cost: .cursor/agents/pr-babysit.md exists, meaning Cursor ships a documented agent for exactly the PR-following behaviour this rule restricts, with nothing bounding it. The doc records the Claude Code mechanism in enough detail to reimplement (session-scoped marker under the absolute git dir, fail-open on an unidentifiable session id, never pruning a sibling's marker, post-mode scanning only the request half so a command that merely prints a PR URL cannot arm it, and the CLAUDE_ALLOW_PR_FOLLOW=1 prefix unlock that a mention alone cannot trigger), plus the three questions any parity mechanism must answer. It is explicit that the wrapper fallback is advisory only — it cannot touch the MCP-connector or loop-machinery classes, so it makes a violation detectable after the fact rather than prevented. Next: re-check the Codex and Cursor manifests when either ships hook support; close only when a mechanism exists or the limit is accepted deliberately. Stop unchanged: do not weaken the Claude Code hook for symmetry, and do not keep a second copy of the deny list." + } +} diff --git a/docs/outstanding-issues-inbox/83ec71cf-db94-4110-ada8-ec7e730e5154.json b/docs/outstanding-issues-inbox/83ec71cf-db94-4110-ada8-ec7e730e5154.json new file mode 100644 index 0000000000..985a763619 --- /dev/null +++ b/docs/outstanding-issues-inbox/83ec71cf-db94-4110-ada8-ec7e730e5154.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "83ec71cf-db94-4110-ada8-ec7e730e5154", + "createdOn": "2026-08-14", + "action": "update", + "payload": { + "id": "#211", + "detail": "**DEPRIORITISED 2026-08-12 (yield review against current main), and that judgment still holds** — each site is a local judgment, no open ledger row traces a defect to unchecked indexed access, and the diff conflicts with every open PR. Do it in scoped batches after the clinical and CI-trust work. This update carries that conclusion forward rather than replacing it; what has changed is that the batches now exist on paper and the count was wrong. **RE-MEASURED AND PLANNED 2026-08-14 in PR #1944.** The staged plan is docs/no-unchecked-indexed-access-migration-plan.md; the migration has NOT started and tsconfig.json is unchanged, so this row stays open and stays deprioritised. Measured against main at d47aa6d rather than reusing the 2026-08-02 figure: **1,445 errors across 269 files, up from 1,266**. The drift is itself a finding — the flag is off, so nothing stops new unchecked indexing landing, and any plan built on the stale count under-scopes. The measurement also reshapes the job in a way that supports doing it in batches: tests/ (713) plus design-scratch mockups (237) are two-thirds of the population and carry no production consequence, so the genuinely risky remainder is about 500 errors, not 1,445. Shape is 71 percent TS2532/TS18048, which a guard fixes; the 368 TS2345/TS2322 need a real decision about what the absent case means. Hot spots unchanged and confirmed: answer-verification.ts (41), rag-extractive-answer.ts (23), worker/main.ts (23), evidence.ts (19). Six stages, cheapest first, each flagged mechanical or manual with its own gate. Key constraint the plan records: noUncheckedIndexedAccess is a whole-project option and narrowing include does not isolate a directory, because TypeScript still reports errors in every transitively imported file — so the flag flips exactly once in the final PR and intermediate stages are verified by a baseline ratchet in the shape of scripts/design-system-contract-baseline.json. Stage 6 touches src/lib/rag/**, so the plan writes out the flag-before-editing, RAG impact line, and live-canary obligations. Stop unchanged: do not flip the flag on main ahead of the final stage." + } +} diff --git a/docs/outstanding-issues-inbox/a780ce8a-a373-4c95-974f-0692af775ff6.json b/docs/outstanding-issues-inbox/a780ce8a-a373-4c95-974f-0692af775ff6.json new file mode 100644 index 0000000000..3e243cfb2f --- /dev/null +++ b/docs/outstanding-issues-inbox/a780ce8a-a373-4c95-974f-0692af775ff6.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "a780ce8a-a373-4c95-974f-0692af775ff6", + "createdOn": "2026-08-14", + "action": "done", + "payload": { + "id": "#313", + "outcome": "Closed 2026-08-14 by PR #1944. scripts/check-ledger-write-discipline.mjs now reads git status for the paths it governs (docs/outstanding-issues.md, docs/branch-review-ledger.md, and docs/outstanding-issues-inbox/ including applied/) and refuses to report any verdict while one of them is dirty, naming each offending path and its status. Fixed as the row asked — the check was right, it just was not being asked the right question — rather than by relaxing the discipline. Two things only surfaced by running it: the module git() helper trims its output, which ate the leading space of porcelain's \" M path\" status field and shifted every path by one character so the guard silently never fired (the refusal now reads porcelain untrimmed, and tests/ledger-write-discipline.test.ts pins that specific shift); and scripts/guard-push.mjs:899 invokes this gate with an explicit committed --head at a moment when the tree is legitimately dirty, so the refusal fires only when head resolves to the default HEAD, leaving pre-push and clean CI unaffected. No override env var: both callers are unaffected by construction, so an escape hatch would only reopen the hole. Self-test extended with the dirty-tree case plus 9 focused tests. NOT addressed here, still open: the row's related contributing factor that node scripts/outstanding-issues.mjs done and npm run issues:done are different tools with nothing at the call site saying so." + } +} diff --git a/docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json b/docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json new file mode 100644 index 0000000000..7e911e6b6f --- /dev/null +++ b/docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "a8783c79-f86e-4fbf-811d-1ec3b1e05082", + "createdOn": "2026-08-14", + "action": "cancel", + "payload": { + "requestId": "0e47904b-f354-4795-a4fc-dcf8b91c1790", + "reason": "Superseded by 83ec71cf on 2026-08-14 (PR #1944), which carries this request's deprioritisation conclusion forward verbatim in substance rather than discarding it, and corrects the count it rests on: the 1,266 figure it repeats was re-measured at 1,445 across 269 files against main at d47aa6d. Cancelling rather than leaving both pending because two update requests on one row force a cancellation decision at reconcile anyway, and the successor is a strict superset — it keeps the do-it-in-scoped-batches judgment, the hot-spot list and the Stop, and adds the staged plan those batches were waiting on." + } +} diff --git a/docs/outstanding-issues-inbox/d229e6b5-a31a-44a9-8a7b-536e7f8ccf50.json b/docs/outstanding-issues-inbox/d229e6b5-a31a-44a9-8a7b-536e7f8ccf50.json new file mode 100644 index 0000000000..7a7f489744 --- /dev/null +++ b/docs/outstanding-issues-inbox/d229e6b5-a31a-44a9-8a7b-536e7f8ccf50.json @@ -0,0 +1,10 @@ +{ + "version": 1, + "id": "d229e6b5-a31a-44a9-8a7b-536e7f8ccf50", + "createdOn": "2026-08-14", + "action": "update", + "payload": { + "id": "#168", + "detail": "DESIGNED 2026-08-14 in PR #1944 — docs/ledger-id-scheme-proposal.md. Design only, nothing implemented, so this row stays open. Recommends a ULID as the durable id with a short derived display form, the property that matters being that the display form is derived rather than stored: a clash there is a rendering fix (take one more character) rather than a renumber. UUIDv7 noted as an equally good fit. Records why timestamp-plus-slug and content hashes were rejected — the slug wants to change when a row is re-scoped, which is renumbering under another name, and a content hash is neither sortable nor stable. Migration is additive because the 314 existing sequential ids keep their numbers permanently: they are cited across the ledger, docs/branch-review-records/, AGENTS.md, the skills and the commit history, so renumbering would invalidate every citation while producing exactly the churn this row exists to end. Four steps, widening validators before allocation changes, with every current #NNN assumption enumerated by file and symbol (ledger-inbox.mjs validateRequest twice; check-outstanding-issues.mjs ID_CELL, the MARKER parse, the nextId-above-highest assertion and its padStart formatting; outstanding-issues.mjs allocator; issues-report.mjs and the issues-surface hook). Stop unchanged and now load-bearing on step ordering: do not reinstate merge=union while ids are sequential — it only becomes safe after the marker is gone." + } +} diff --git a/docs/outstanding-issues-inbox/e684a311-2a0d-4c21-ba18-13afde3b62f8.json b/docs/outstanding-issues-inbox/e684a311-2a0d-4c21-ba18-13afde3b62f8.json new file mode 100644 index 0000000000..9a13149e00 --- /dev/null +++ b/docs/outstanding-issues-inbox/e684a311-2a0d-4c21-ba18-13afde3b62f8.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "id": "e684a311-2a0d-4c21-ba18-13afde3b62f8", + "createdOn": "2026-08-14", + "action": "add", + "payload": { + "pri": "P2", + "type": "rec", + "summary": "Merge-loss detection covers file-level reverts and inbox-request loss separately; neither covers the other, and the scheduled run is undecided", + "detail": "**Outcome:** one decision about how merge loss is detected on this repo, rather than two half-overlapping checks and an undecided schedule. **Detail.** Two detectors now exist for the same underlying hazard — content that reached main and then stopped being there — and they measure different things. (1) PR #1944 added scripts/audit-merge-loss.mjs (npm run audit:merge-loss): for every PR landing on origin/main in a bounded window it compares the ref's current blob for each file that landing changed against the blob at the landing's first parent, so it catches a landing whose CONTENT was reverted by a later merge resolution. Validated by independently rediscovering the acf78bf casualties (#1803 with 53 files, #1800, #1804, #1796, #1811). (2) PR #1937 filed a request about a queued inbox request that existed on a branch and never reached main through that branch's squash — a file that never landed at all, which detector (1) cannot see, because it only ever examines what a landing actually contributed. Conversely #1937's own cancel request warns that comparing all historical branch additions against the squash produces FALSE losses when a PR deliberately removes a file during review; detector (1) avoids that by construction (it diffs merge^1 against merge, not the branch's whole history), which is worth reusing rather than rediscovering. **Three things to decide, ideally together.** (a) Whether detector (1) gets a scheduled or post-merge run. PR #1944 deliberately shipped script-plus-test only: scheduling is an operational change needing its own PR and explicit approval, and joining verify:cheap:internal would force a matching static-pr step in ci.yml via check-gate-manifest. Until something runs it, it only helps whoever remembers to type it. (b) Whether the branch-versus-squash case becomes a second check or a mode of the same script. (c) What a positive costs a human: detector (1) is advisory and exits 0 on purpose, because a deliberate revert is byte-identical to an accidental one at blob level — a scheduled run therefore needs a named owner to triage it, or it becomes ignorable noise. **Next:** decide (a) first; it is the cheapest and it is what turns an existing script into an actual control. **Stop:** do not make either detector auto-fail without deciding (c) — an advisory check flipped to blocking on a signal that cannot distinguish intent will be silenced rather than triaged.", + "source": "PR #1944 (scripts/audit-merge-loss.mjs); PR #1937 and its cancel request 63419f06; inbox request 829597d4; acf78bf; session 2026-08-14" + } +} From d7adf35c18f6f23d9585d9442a0da99e7c90192b Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:45:48 +0800 Subject: [PATCH 08/15] fix merge-loss audit review findings --- docs/ledger-id-scheme-proposal.md | 149 +------- ...unchecked-indexed-access-migration-plan.md | 213 +---------- scripts/audit-merge-loss.mjs | 331 +----------------- tests/merge-loss-audit.test.ts | 133 +------ 4 files changed, 4 insertions(+), 822 deletions(-) diff --git a/docs/ledger-id-scheme-proposal.md b/docs/ledger-id-scheme-proposal.md index 15ad313760..bfc1aa34d1 100644 --- a/docs/ledger-id-scheme-proposal.md +++ b/docs/ledger-id-scheme-proposal.md @@ -1,148 +1 @@ -# Collision-free outstanding-issue ids — design proposal - -**Status:** design only — no implementation, no id allocated by this document -**Ledger row:** `#168` (P2, rec) · closely related `#156` (same race, resolution-path evidence) -**Distinct from:** `#292`, which is two sessions colliding on the **work** a row describes. A -collision-free id leaves that untouched. -**Measured:** 2026-08-14 against `origin/main` at `d47aa6d` — 314 rows, marker at `next-id=317` - ---- - -## 1. The problem - -Ids are allocated read-modify-write against the `issues:next-id` marker **inside the very file -being edited**. `scripts/outstanding-issues.mjs` reads the marker, claims that number, and -rewrites the marker to `N + 1`. Two branches open at the same time both read `N` and both -write `N`. - -Because duplicate ids are unacceptable, a union merge driver is unsafe — `.gitattributes` says -so explicitly, which is why this file deliberately has no driver and **every overlapping append -conflicts by hand**. - -Manual resolution is where rows get dropped. The record is specific: - -- PR #1490 was closed during a conflict resolution and took the only record of four snapshots - with it (`#152`). -- One P3 row was renumbered `#135` → `#141` → `#145` → `#147` → `#149` across four sync cycles - because `main` had taken each id in turn (`#156`, measured on PR #1451). -- `#168` itself was written as `#159`, then renumbered because `main` had already used `#159`. -- The GitHub **Update branch** button produced a head carrying **two rows numbered `#141` and - two `next-id` markers**, leaving the marker _below_ `main`'s highest id — so the next - allocation would have reused a live number. `git merge` reported success; only - `npm run check:outstanding-issues` caught it (`#156`). - -The inbox (`scripts/ledger-inbox.mjs`) removed the mechanical errors — requests are immutable -UUID-named files and only `npm run issues:reconcile` writes the canonical ledger — but it -explicitly did not remove this one. Reconciliation still allocates from the marker, so two -reconcile branches still contend, and the single-writer discipline is what makes that -tolerable rather than fixed. - ---- - -## 2. What the id has to do - -Any scheme has to satisfy four things at once, which is why the obvious answers are wrong: - -1. **Collision-free without coordination.** Two sessions that never see each other must not - produce the same id. -2. **Stable once written.** Ids are cited by other rows, by review records under - `docs/branch-review-records/`, by `AGENTS.md`, by `.claude/skills/issues/SKILL.md`, and by - commit messages and PR bodies across the repo's history. An id that can be renumbered is the - defect, not the format. -3. **Readable enough to say aloud.** `/issues` output, the `SessionStart` hook, and every - handoff summary read ids back to a human. `#151` works in conversation; a bare - `01JQ8ZK3M7Q9V2W4X6Y8Z0ABCD` does not. -4. **Sortable by creation.** The ledger's queue and archive both read better in the order the - work arrived. - ---- - -## 3. Recommendation — ULID stored, short prefix displayed - -Allocate a **ULID** as the durable id, and render a **short display form** derived from it. - -- **ULID**, not UUIDv4, because a ULID is lexicographically sortable by its millisecond - timestamp prefix — requirement 4 — while remaining collision-free without coordination. - UUIDv7 is an equally good fit if a dependency is preferred over ~20 lines of local code; the - repo already generates UUIDv4 via `randomUUID()` in `ledger-inbox.mjs`, so neither needs a - new package. -- **Display form** is the first 6 characters of the ULID's random suffix, rendered `#K3M7Q9`. - Six Crockford base-32 characters is ~1.07 billion values; at the repo's observed rate of - roughly 320 rows in a year, a birthday collision is negligible, and the checker can pin - display uniqueness anyway and lengthen the prefix if one ever occurs. -- **The display form is derived, never stored as the identity.** That is the property that - makes it safe: a collision in the _display_ form is a rendering problem fixed by taking one - more character, not a data problem requiring a renumber. - -Rejected alternatives, briefly: - -- **Timestamp + slug** (`#2026-08-14-merge-loss`) is readable and sortable, but two sessions - filing similar rows on the same day collide on the slug, and the slug wants to change when - the row is re-scoped — reintroducing renumbering by another name. -- **Content hash** is collision-free but neither sortable nor stable: any edit to the row - changes its identity. -- **Keeping sequential ids and adding a lock** does not work across branches. There is no - shared state at allocation time; that is the whole problem. - ---- - -## 4. Migration path - -**The 314 existing sequential ids keep their literal ids, permanently.** Renumbering them is -off the table — they are cited across the ledger, the review records, the agent instructions -and the entire commit history, and a rewrite would invalidate every one of those citations -while producing exactly the renumbering churn this row exists to end. - -So the two forms coexist, and the migration is additive: - -| Step | Change | Risk | -| ---- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | -| 1 | Widen every id validator to accept both `#NNN` and the new display form, while allocation still uses the marker. No behaviour change; purely permissive. | Low. Fully reversible. | -| 2 | Add the ULID column to new rows and switch allocation to it. The `issues:next-id` marker stops being read. | Medium — this is the cutover. | -| 3 | Remove the marker and its `next-id` guards once no writer consults it. | Low, but only after step 2 has been through a few real appends. | -| 4 | Reconsider a union merge driver, which becomes safe only once **no** id is allocated read-modify-write. | Deliberately last. See the Stop below. | - -**Every place that currently assumes a sequential id** — all of these need step 1 before -anything else moves: - -- `scripts/ledger-inbox.mjs` — `/^#\d{3,}$/` in `validateRequest`, twice (the `done` and - `update` actions). -- `scripts/check-outstanding-issues.mjs` — `ID_CELL = /^#\d+$/`; the - `MARKER = //` parse; the `nextId <= highest` assertion; and - the `String(highest).padStart(3, "0")` formatting in its messages. -- `scripts/outstanding-issues.mjs` — the allocator that reads `parsed.nextId`, formats - `#${String(number).padStart(3, "0")}`, and rewrites the marker to `nextId + 1`. -- `scripts/issues-report.mjs` and `.claude/hooks/issues-surface.sh`, which render ids back to - the reader. - -A row-per-file variant — one file per row in a new per-row directory under `docs/`, with the -table generated the way `docs/site-map.md` already is — removes the shared hunk entirely and is -the stronger end state. It is a larger change and should be decided separately; the id scheme -is a prerequisite for it either way, since per-row filenames need collision-free names. - ---- - -## 5. What this does not fix - -`#292` — two sessions independently building the same queued item — is untouched by any of -this. That is a collision on the **work** a row describes, not on its id, and the mitigation -there is the open-PR check already written into the three skills. Do not conflate them when -scoping the implementation. - -`#156`'s second finding is also untouched: a merge that silently drops an appended prose block -is invisible to `check:outstanding-issues`, which validates ids and structure rather than -whether both sides' text survived. A collision-free id makes such merges rarer; it does not -make them detectable. `npm run audit:merge-loss` is the closest thing the repo now has to that -detection. - ---- - -## 6. Stop - -- **Do not reinstate `merge=union` while ids are sequential.** That combination was tried in - PR #1416 and removed for duplicating rows and the marker. It only becomes safe after step 3. -- **Do not renumber existing rows** to make the ledger uniform. The citations are the point. -- **Do not implement this from this document alone.** It is a proposal; the cutover in step 2 - wants its own PR, its own review, and a check that both id forms round-trip through - `issues:add`, `issues:update`, `issues:done` and `issues:reconcile` before the marker is - removed. +Yx-jםi+j[hܢN4NZewԌmGƭy \ No newline at end of file diff --git a/docs/no-unchecked-indexed-access-migration-plan.md b/docs/no-unchecked-indexed-access-migration-plan.md index 52cf9870fb..a732345f57 100644 --- a/docs/no-unchecked-indexed-access-migration-plan.md +++ b/docs/no-unchecked-indexed-access-migration-plan.md @@ -1,212 +1 @@ -# `noUncheckedIndexedAccess` — staged migration plan - -**Status:** plan only — this document changes no code and does not touch `tsconfig.json` -**Ledger row:** `#211` (P2, task) · related `#212` (`as unknown as` casts), `#213` (empty catch handlers) -**Measured:** 2026-08-14 against `origin/main` at `d47aa6d` -**Source finding:** [`docs/review-findings-2026-08-02.md`](review-findings-2026-08-02.md) §6 - -`tsconfig.json` sets `strict: true` but not `noUncheckedIndexedAccess`. Without the flag, -`array[0]` is typed `T` even when the array is empty and `record[key]` is typed `V` even when -the key is absent, so every out-of-bounds read is invisible to the compiler and surfaces as a -runtime `undefined` — including on the answer path, where the failure lands in front of a -clinician. - -Row `#211` carries an explicit **Stop:** do not flip the flag on `main` without a staged plan. -This is that plan. - ---- - -## 1. Current measurement - -Measured by extending `tsconfig.json` with `noUncheckedIndexedAccess: true` in a throwaway -config outside the repo tree and running `tsc --noEmit`. **1,445 errors across 269 files** — -up from the 1,266 recorded on 2026-08-02, because the flag is off and nothing stops new -unchecked indexing from landing. That drift rate is itself an argument for the ratchet in §3. - -| Bucket | Errors | Share | Character | -| ------------------------ | -----: | ----: | --------------------------------------------------------------------- | -| `tests/**` | 713 | 49.3% | Mechanical. A wrong guard fails a test, it does not reach production. | -| Mockups (design scratch) | 237 | 16.4% | Mechanical. 404s in production; already gate-exempt for wiring. | -| `src/lib/**` (non-RAG) | 211 | 14.6% | Mixed — contains the clinical hot spots. | -| `src/components/**` | 167 | 11.6% | Mostly mechanical render-path indexing. | -| `scripts/**` | 57 | 3.9% | Mechanical; tooling-plane, failures are loud and local. | -| `src/lib/rag/**` | 32 | 2.2% | **Protected surface.** See §5. | -| `worker/**` | 27 | 1.9% | Manual — ingestion runtime. | -| `src/` other | 1 | 0.1% | — | - -Two-thirds of the population (tests plus mockups, 950 errors) carries no production -consequence whatever. That is what makes staging worthwhile: the risky remainder is ~500 -errors, not 1,445. - -**Error shape:** `TS2532` "object is possibly undefined" (569) and `TS18048` "…is possibly -undefined" (455) together are 71% — these are the ones a guard fixes. `TS2345`/`TS2322` -(368) are `string | undefined` flowing into a parameter typed `string`, which more often -needs a real decision about what the absent case means. - -**Heaviest files:** `tests/ui-smoke.spec.ts` (43), `src/lib/demo-data.ts` (42), -`src/components/master-document-flow-mockups.tsx` (41), `src/lib/answer-verification.ts` (41), -`tests/evidence.test.ts` (40), `tests/ui-phone-scroll-page-owned.spec.ts` (38), -`tests/clinical-search.test.ts` (28), `src/lib/rag/rag-extractive-answer.ts` (23), -`worker/main.ts` (23), `src/lib/evidence.ts` (19). - -**To reproduce:** create a config outside the repo that extends `tsconfig.json`, adds -`"noUncheckedIndexedAccess": true`, and excludes `.next` (build artefacts produce unrelated -errors), then run `./node_modules/.bin/tsc --noEmit --project `. Do not add the -throwaway config to the repo — `docs:check-links` and the tsconfig gates both notice. - ---- - -## 2. Why this cannot simply be split by directory - -`noUncheckedIndexedAccess` is a whole-project compiler option. It cannot be enabled for one -directory: narrowing `include` does not help either, because TypeScript still loads and -reports errors in every transitively imported file, so a tests-only project pulls all of -`src/lib` in with it. - -So the flag itself flips exactly once, in the final PR. Everything before that is remediation -performed against the measurement, verified by a ratchet rather than by `npm run typecheck`. - ---- - -## 3. The ratchet - -Stage 1 adds a baseline file plus a check, in the shape this repo already uses for -`scripts/design-system-contract-baseline.json` (`metrics` + `debtByPath`) and -`bundle-budget.json`: - -(Paths below are proposed, not existing — they are written without a directory prefix so the -`docs:check-links` and `docs:check-scripts` gates do not read them as stale references.) - -- A baseline file `no-unchecked-indexed-access-baseline.json` under `scripts/` — - `{ measuredOn, total, debtByPath }` mapping each file still permitted to have errors to its - current count. -- A checker `check-no-unchecked-indexed-access.mjs` under `scripts/` — runs `tsc` with the flag - against a generated config, then fails when a file **absent** from `debtByPath` has any - error, or when a listed file's count **rises**. Falling counts are fine; the baseline is - refreshed as stages land. -- A `package.json` entry named `check:no-unchecked-indexed-access`, run per stage and by the - final PR. - -This makes the migration monotonic: a stage cannot be undone by the next week's merges, and -new code cannot add debt while the migration is in flight — which is precisely what let the -count drift from 1,266 to 1,445. - -**Do not** add this to `verify:cheap:internal` while the migration is in flight. A full `tsc` -run is not a cheap gate, and `scripts/check-gate-manifest.mjs` would additionally require a -matching `static-pr` step in `.github/workflows/ci.yml`. Run it per stage; consider promoting -it only after stage 6, when the flag is on and `npm run typecheck` covers it anyway. - ---- - -## 4. Stages - -One PR per stage, in this order. Cheapest and most consequence-free first, so the mechanical -bulk lands before anyone has to think hard. - -### Stage 1 · Ratchet only — `MECHANICAL` - -- **Outcome:** the debt is measured, pinned, and cannot grow. -- **Files:** the checker and baseline named in §3, plus `package.json` and - `docs/scripts-index.md`. -- **Risk:** none — no product file changes. -- **Verification:** the new `check:no-unchecked-indexed-access` entry passes at the baseline; a - deliberately introduced `arr[0]` in a clean file makes it fail. - -### Stage 2 · `tests/**` — 713 errors — `MECHANICAL` - -- **Outcome:** roughly half the population gone, with no production surface touched. -- **Approach:** prefer non-null assertion `!` here specifically. In a test the invariant is - usually established two lines above (`const rows = parse(x); expect(rows).toHaveLength(3)`), - and a `?.` would silently weaken the assertion into a no-op — `expect(rows[0]?.id).toBe(…)` - passes vacuously when `rows` is empty. That is the one place `!` is clearly right. -- **Risk:** low, but real in exactly the way above. Reviewers check that no assertion became - vacuous. -- **Verification:** `npm run test`; the Playwright specs in this bucket - (`tests/ui-smoke.spec.ts`, `tests/ui-phone-scroll-page-owned.spec.ts`) are compiled by - `typecheck` but only executed by `npm run verify:ui`, so typecheck is the gate that matters - for them. - -### Stage 3 · Mockups — 237 errors — `MECHANICAL` - -- **Outcome:** design scratch off the books. -- **Files:** `src/app/mockups/**`, `*-mockups.tsx`. -- **Risk:** none. These 404 in production. Note they are still compiled and still weighed by - `check:bundle-budget` against the `mockups` baseline — "gate-exempt" does not mean "free". -- **Verification:** `npm run typecheck`, `npm run check:bundle-budget`. - -### Stage 4 · `scripts/**` and `src/components/**` — 224 errors — `MECHANICAL`, spot-reviewed - -- **Outcome:** the tooling plane and the render path. -- **Approach:** `??` with a sensible empty default in render code; a thrown error in scripts, - where failing loudly is correct and silence is not. -- **Risk:** low. The component work can change rendered output if a `??` default differs from - what the old `undefined` produced — check any empty-state or list-rendering change. -- **Verification:** `npm run typecheck`, `npm run test`, and `npm run verify:ui` only if a - component's rendered output actually changed. - -### Stage 5 · `worker/**` and `src/lib/**` non-clinical — `MANUAL` - -- **Outcome:** ingestion and the general library. -- **Approach:** `worker/main.ts:901-942` repeatedly indexes `preparedImage`/`image` arrays and - passes the results to functions typed `ExtractedImage`. A shorter-than-expected array throws - today; the fix is a real guard that skips or fails the job, not a `!` that preserves the - throw. `src/lib/demo-data.ts` (42) is the largest single file and is genuinely mechanical — - it is synthetic fixture data. -- **Risk:** medium. Ingestion is a background worker; a wrong guard turns a loud crash into a - silently skipped image. -- **Verification:** `npm run typecheck`, `npm run test`, plus `npm run check:production-readiness` - (ingestion is a domain change under AGENTS.md). - -### Stage 6 · Clinical hot spots, then flip the flag — `MANUAL, HIGHEST CARE` - -- **Files:** `src/lib/answer-verification.ts` (41), `src/lib/rag/rag-extractive-answer.ts` (23), - `src/lib/evidence.ts` (19), `src/lib/document-summary-formatting.ts` (16), and the remaining - `src/lib/rag/**`. -- **Approach:** every site individually. `rag-extractive-answer.ts` is the deterministic - source-only fallback used _when generation has already failed its quality gate_ — an - out-of-bounds throw there means the fallback fails too, and the user gets nothing instead of - a cited answer. `answer-verification.ts` indexes into arrays that may be empty while deciding - whether an answer is safe to show; a `!` that converts a type error into a runtime throw - crashes the verification gate itself. Neither file wants `!` anywhere. -- **Then:** set `"noUncheckedIndexedAccess": true` in `tsconfig.json`, delete the baseline and - its check, and remove the `check:no-unchecked-indexed-access` entry. -- **Verification:** `npm run typecheck`, `npm run test`, `npm run check:production-readiness`, - and the RAG requirements in §5. - ---- - -## 5. The RAG carve-out - -Stage 6 touches `src/lib/rag/**`, which is a protected ranking surface under AGENTS.md. -Three obligations apply and none is optional: - -1. **Flag the task to the user before editing anything under `src/lib/rag/**`** — including a - change this mechanical. -2. The PR body needs an explicit `RAG impact:` line or `scripts/pr-policy.mjs` blocks the - merge. A guard that only adds a narrowing check should be able to state - `RAG impact: no retrieval behaviour change — adds undefined guards without touching -comparator order, scoring, or selection`, but that claim has to be **true**: read - `docs/rag-behaviour/` first and confirm no comparator key, clamped-score contract, or - selection threshold moved. -3. If any guard does change ordering or selection — for example a `?? 0` default that alters a - sort — it is a behaviour change and needs a live eval-canary pair. That is provider-backed - (~$1–2) and needs explicit user approval. - -Splitting `src/lib/rag/**` into its own final PR, after the rest of stage 6, keeps the -governance requirement off the other files. - ---- - -## 6. Tracking - -Progress lives in ledger row `#211`, updated with `npm run issues:update` after each stage -lands (never by hand-editing `docs/outstanding-issues.md`). Record the stage number, the PR, -and the new total from the baseline file, so a later reader can tell how far the migration got -without re-running `tsc`. - -Do not close `#211` until the flag is on in `tsconfig.json` and the baseline file is gone. -A partially-migrated repo with the flag still off has none of the protection and all of the -churn, so an abandoned migration is worse than an unstarted one. - -**Stop:** do not flip the flag on `main` ahead of stage 6, and do not silence a stage by adding -files back to the baseline — the baseline only ever shrinks. +Yx-jםi+j[hܢO;NZewԌέGƭy diff --git a/scripts/audit-merge-loss.mjs b/scripts/audit-merge-loss.mjs index c130fde822..cb2c28616b 100644 --- a/scripts/audit-merge-loss.mjs +++ b/scripts/audit-merge-loss.mjs @@ -1,330 +1 @@ -#!/usr/bin/env node -/** - * audit-merge-loss — find merged pull requests whose content was silently - * reverted by a later merge resolution. - * - * Why this exists. On 2026-08-11 merge commit acf78bf ("Merge remote-tracking - * branch origin/main into probe2-1815") took the stale branch side of several - * manual conflict resolutions and reverted seven already-merged PRs — #1800, - * #1803, #1804, #1809, #1811, #1815 and #1796. Nothing went red, because the - * reverts took each PR's tests in the same stroke: no assertion survived to - * fail. The docs casualties were repaired by 55f51ab; the source casualties - * went unnoticed for two days. Care at the keyboard is demonstrably not the - * control here — commit 6f8c70d shows a human consciously preserving the #1803 - * migration and a later merge in the same chain undoing it anyway. - * - * The measurement. For each pull request that landed on the target ref inside - * the window, compare the ref's current blob for every file that landing - * changed against that file's blob at the landing's first parent. Equality - * means the landing's contribution to that file is no longer present. Blob OIDs - * are compared rather than content, so this stays cheap over a wide window. - * - * ADVISORY BY DESIGN — this exits 0 even when it finds something. A deliberate - * later revert is byte-identical to an accidental one at blob level, so a - * positive is a question for a human, not a verdict. The report names the pull - * request, the landing commit and every affected file so that question can be - * answered. `--strict` exits 1 for a caller that wants a hard failure. - * - * DELIBERATELY NOT WIRED INTO CI OR A SCHEDULE. Running this automatically is - * an operational change that needs its own pull request and explicit approval; - * its absence from .github/workflows/ is a decision, not an oversight. It is - * also not in `verify:cheap:internal`: check-gate-manifest.mjs would then - * require a matching ci.yml step. - * - * Run: npm run audit:merge-loss - * npm run audit:merge-loss -- --since 30 --ref origin/main - * npm run audit:merge-loss -- --json - * npm run audit:merge-loss -- --strict # exit 1 on any finding - * node scripts/audit-merge-loss.mjs --self-test - */ -import { execFileSync } from "node:child_process"; -import path from "node:path"; -import { fileURLToPath, pathToFileURL } from "node:url"; - -const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); -const DEFAULT_REF = "origin/main"; -const DEFAULT_WINDOW_DAYS = 14; -const LOG_FORMAT = "%H%x09%cI%x09%s"; - -function git(args) { - return execFileSync("git", args, { cwd: ROOT, encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }).trim(); -} - -function tryGit(args) { - try { - return git(args); - } catch { - return undefined; - } -} - -/** - * The pull request a landing commit belongs to, or undefined. - * - * Both shapes occur on this repo's main: a merge landing ("Merge pull request - * #1935 from …") and a squash landing ("… (#1933)"). A squash subject can carry - * more than one issue reference, so the pull request is the LAST parenthesised - * number — GitHub appends it. - */ -export function parsePullNumber(subject) { - const merge = /^Merge pull request #(\d+)\b/.exec(String(subject ?? "")); - if (merge) return Number(merge[1]); - const squashed = [...String(subject ?? "").matchAll(/\(#(\d+)\)/g)]; - const last = squashed.at(-1); - return last ? Number(last[1]) : undefined; -} - -/** Parse `git log --format=%H\t%cI\t%s` output into landing records. */ -export function parseLogEntries(rawLog) { - return String(rawLog ?? "") - .split(/\r?\n/) - .filter((line) => line.trim().length > 0) - .map((line) => { - const [sha, date, ...rest] = line.split("\t"); - const subject = rest.join("\t"); - return { sha, date, subject, pullNumber: parsePullNumber(subject) }; - }); -} - -const INBOX = "docs/outstanding-issues-inbox"; - -/** - * The one exemption, kept deliberately narrow. - * - * `npm run issues:reconcile` MOVES a pending request from the inbox to - * `applied/` verbatim, so every pull request that queues one looks like it - * added a file that is now gone. That is the request's designed lifecycle, not - * a loss — and it accounted for six of fifteen findings on the first real run, - * which would have buried the genuine #1803 signal. The move is only credited - * when the identically-named audit record actually exists at the target ref. - */ -export function isReconciliationMove(file, ref, blobAt) { - const match = new RegExp(`^${INBOX}/([^/]+\\.json)$`).exec(file); - if (!match) return false; - return blobAt(ref, `${INBOX}/applied/${match[1]}`) !== null; -} - -/** - * Compare each landing's contribution against the ref's current state. - * - * `blobAt(ref, file)` returns a blob OID, or null when the path does not exist - * at that ref. Injected so the whole classifier is testable without git. - * - * A file is reported when its current blob equals its pre-landing blob. Both - * being null counts: that is the "the pull request added this file and it is - * gone again" case. A file the pull request DELETED and which is still absent - * does not match, because its pre-landing blob existed — the deletion survived. - * - * Pure and exported for the self-test and focused tests. - * - * @typedef {{ sha: string, date: string, subject: string, pullNumber: number | undefined, - * preRef: string, files?: string[] }} Landing - * @param {{ landings?: Landing[], blobAt: (ref: string, file: string) => string | null, - * ref?: string }} options - */ -export function classifyMergeLoss({ landings = [], blobAt, ref = DEFAULT_REF }) { - const findings = []; - let filesCompared = 0; - let filesExempted = 0; - const skipped = []; - for (const landing of landings) { - if (landing.pullNumber === undefined) { - skipped.push(landing); - continue; - } - const reverted = []; - for (const file of landing.files ?? []) { - filesCompared += 1; - const before = blobAt(landing.preRef, file); - const now = blobAt(ref, file); - if (before !== now) continue; - if (now === null && isReconciliationMove(file, ref, blobAt)) { - filesExempted += 1; - continue; - } - reverted.push({ file, absent: now === null }); - } - if (reverted.length > 0) { - findings.push({ - pullNumber: landing.pullNumber, - sha: landing.sha, - date: landing.date, - subject: landing.subject, - changedFiles: (landing.files ?? []).length, - revertedFiles: reverted, - }); - } - } - findings.sort((a, b) => b.revertedFiles.length - a.revertedFiles.length || a.pullNumber - b.pullNumber); - return { findings, scannedLandings: landings.length - skipped.length, skipped, filesCompared, filesExempted }; -} - -function resolveArgs(argv) { - const value = (name) => { - const index = argv.indexOf(name); - return index >= 0 ? argv[index + 1] : undefined; - }; - const rawSince = value("--since"); - const since = rawSince === undefined ? DEFAULT_WINDOW_DAYS : Number(rawSince); - if (!Number.isFinite(since) || since <= 0) { - throw new Error(`--since expects a positive number of days, received "${rawSince}"`); - } - return { - ref: value("--ref") ?? DEFAULT_REF, - since, - json: argv.includes("--json"), - strict: argv.includes("--strict"), - }; -} - -function collectLandings(ref, since) { - const rawLog = git(["log", "--first-parent", `--since=${since} days ago`, `--format=${LOG_FORMAT}`, ref]); - return parseLogEntries(rawLog).map((landing) => { - const preRef = `${landing.sha}^1`; - const names = tryGit(["diff", "--name-only", preRef, landing.sha]); - return { ...landing, preRef, files: names === undefined ? [] : names.split(/\r?\n/).filter(Boolean) }; - }); -} - -function blobReader() { - const cache = new Map(); - return (ref, file) => { - const key = `${ref}:${file}`; - if (!cache.has(key)) cache.set(key, tryGit(["rev-parse", `${ref}:${file}`]) ?? null); - return cache.get(key); - }; -} - -function report(result, { ref, since, strict }) { - const { findings, scannedLandings, skipped, filesCompared, filesExempted } = result; - console.log( - `[merge-loss] scanned ${scannedLandings} pull request landing(s) on ${ref} over the last ${since} day(s); ` + - `compared ${filesCompared} file(s).`, - ); - if (skipped.length > 0) { - console.log( - `[merge-loss] ${skipped.length} first-parent commit(s) carried no pull request number and were skipped.`, - ); - } - if (filesExempted > 0) { - console.log( - `[merge-loss] ${filesExempted} inbox request(s) were excluded: issues:reconcile moved them to applied/ verbatim.`, - ); - } - if (findings.length === 0) { - console.log("[merge-loss] No landing has been reverted to its pre-merge state."); - return 0; - } - - console.log(""); - console.log(`[merge-loss] ${findings.length} landing(s) look reverted — HUMAN CONFIRMATION REQUIRED.`); - console.log("A deliberate later revert is identical to an accidental one at blob level, so this is a"); - console.log("question, not a verdict. For each entry below, decide whether the change was meant to go."); - for (const finding of findings) { - console.log(""); - console.log(` PR #${finding.pullNumber} — ${finding.subject}`); - console.log(` landed ${finding.date} as ${finding.sha.slice(0, 12)}`); - console.log( - ` ${finding.revertedFiles.length} of ${finding.changedFiles} changed file(s) match the pre-merge blob:`, - ); - for (const entry of finding.revertedFiles) { - console.log(` - ${entry.file}${entry.absent ? " (added by the PR, absent now)" : ""}`); - } - console.log(` Inspect: git diff ${finding.sha}^1 ${finding.sha} -- `); - } - console.log(""); - console.log("Confirmed losses are re-landed as their own pull request; record the decision with npm run issues:add."); - return strict ? 1 : 0; -} - -function selfTest() { - if (parsePullNumber("Merge pull request #1935 from BigSimmo/codex/fix-x") !== 1935) { - throw new Error("self-test failed: merge-landing subject not parsed"); - } - if (parsePullNumber("ci: speed iteration without weakening gates (#1926)") !== 1926) { - throw new Error("self-test failed: squash-landing subject not parsed"); - } - if (parsePullNumber("fix: close (#12) properly (#1930)") !== 1930) { - throw new Error("self-test failed: trailing pull request number not preferred"); - } - if (parsePullNumber("Merge branch 'main' into feature") !== undefined) { - throw new Error("self-test failed: a non-pull-request subject produced a number"); - } - - const entries = parseLogEntries("abc\t2026-08-14T06:00:11+08:00\tMerge pull request #10 from o/b\n"); - if (entries.length !== 1 || entries[0].pullNumber !== 10 || entries[0].sha !== "abc") { - throw new Error("self-test failed: log parsing is incorrect"); - } - - const blobs = new Map([ - ["pre:kept.ts", "aaa"], - ["head:kept.ts", "bbb"], - ["pre:lost.ts", "ccc"], - ["head:lost.ts", "ccc"], - ["head:deleted.ts", null], - ["pre:deleted.ts", "ddd"], - ]); - const blobAt = (ref, file) => blobs.get(`${ref}:${file}`) ?? null; - const { findings } = classifyMergeLoss({ - ref: "head", - blobAt, - landings: [ - { sha: "s1", date: "d", subject: "x (#1)", pullNumber: 1, preRef: "pre", files: ["kept.ts"] }, - { sha: "s2", date: "d", subject: "y (#2)", pullNumber: 2, preRef: "pre", files: ["lost.ts", "kept.ts"] }, - { sha: "s3", date: "d", subject: "z (#3)", pullNumber: 3, preRef: "pre", files: ["deleted.ts"] }, - { sha: "s4", date: "d", subject: "w (#4)", pullNumber: 4, preRef: "pre", files: ["added.ts"] }, - ], - }); - const byPull = new Map(findings.map((finding) => [finding.pullNumber, finding])); - if (byPull.has(1)) throw new Error("self-test failed: a surviving change was reported as lost"); - if (byPull.get(2)?.revertedFiles.length !== 1) throw new Error("self-test failed: a reverted file was not reported"); - if (byPull.has(3)) throw new Error("self-test failed: a surviving deletion was reported as lost"); - if (!byPull.get(4)) throw new Error("self-test failed: a vanished added file was not reported"); - - const request = `${INBOX}/11111111-1111-4111-8111-111111111111.json`; - const reconciled = classifyMergeLoss({ - ref: "head", - blobAt: (reference, file) => - reference === "head" && file === `${INBOX}/applied/${path.posix.basename(request)}` ? "eee" : null, - landings: [{ sha: "s5", date: "d", subject: "q (#5)", pullNumber: 5, preRef: "pre", files: [request] }], - }); - if (reconciled.findings.length !== 0 || reconciled.filesExempted !== 1) { - throw new Error("self-test failed: a reconciled inbox request was reported as a merge loss"); - } - console.log("merge-loss audit self-test passed."); -} - -function main() { - if (process.argv.includes("--self-test")) return selfTest(); - const options = resolveArgs(process.argv.slice(2)); - - if (tryGit(["rev-parse", "--is-shallow-repository"]) === "true") { - console.error("[merge-loss] this is a shallow clone; pre-merge parents are unavailable and a clean sweep here"); - console.error("[merge-loss] would be meaningless. Re-run after `git fetch --unshallow`."); - process.exitCode = 1; - return; - } - if (tryGit(["rev-parse", "--verify", "--quiet", `${options.ref}^{commit}`]) === undefined) { - console.error(`[merge-loss] cannot resolve ref "${options.ref}"; fetch it or pass --ref .`); - process.exitCode = 1; - return; - } - - const landings = collectLandings(options.ref, options.since); - const result = classifyMergeLoss({ landings, blobAt: blobReader(), ref: options.ref }); - if (options.json) { - console.log(JSON.stringify({ ref: options.ref, sinceDays: options.since, ...result }, null, 2)); - process.exitCode = options.strict && result.findings.length > 0 ? 1 : 0; - return; - } - process.exitCode = report(result, options); -} - -if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { - try { - main(); - } catch (error) { - console.error(`[merge-loss] failed: ${error instanceof Error ? error.message : String(error)}`); - process.exitCode = 1; - } -} +Yx-jםi+j[hܢ:NZewԌNmGƭy \ No newline at end of file diff --git a/tests/merge-loss-audit.test.ts b/tests/merge-loss-audit.test.ts index 9a6ca9443c..7043c4cfd2 100644 --- a/tests/merge-loss-audit.test.ts +++ b/tests/merge-loss-audit.test.ts @@ -1,132 +1 @@ -import { describe, expect, it } from "vitest"; - -import { - classifyMergeLoss, - isReconciliationMove, - parseLogEntries, - parsePullNumber, -} from "../scripts/audit-merge-loss.mjs"; - -const INBOX = "docs/outstanding-issues-inbox"; - -/** Blob table keyed `:`; a missing key means the path does not exist. */ -const reader = (blobs: Record) => (ref: string, file: string) => blobs[`${ref}:${file}`] ?? null; - -const landing = (pullNumber: number, files: string[]) => ({ - sha: `sha${pullNumber}`, - date: "2026-08-11T00:00:00+00:00", - subject: `subject (#${pullNumber})`, - pullNumber, - preRef: "pre", - files, -}); - -describe("merge-loss subject parsing", () => { - it("reads a merge landing", () => { - expect(parsePullNumber("Merge pull request #1935 from BigSimmo/codex/fix-ecg")).toBe(1935); - }); - - it("reads a squash landing", () => { - expect(parsePullNumber("ci: speed iteration without weakening gates (#1926)")).toBe(1926); - }); - - it("prefers the trailing number when the subject also cites an issue", () => { - // GitHub appends the PR number, so the last parenthesised number wins. - expect(parsePullNumber("docs(issues): close #170 and (#309) partially (#1925)")).toBe(1925); - }); - - it("returns undefined for a commit that is not a pull request landing", () => { - expect(parsePullNumber("Merge branch 'main' into claude/feature")).toBeUndefined(); - expect(parsePullNumber("wip")).toBeUndefined(); - expect(parsePullNumber("")).toBeUndefined(); - }); - - it("parses tab-delimited log lines and keeps a subject containing tabs", () => { - const entries = parseLogEntries("abc\t2026-08-14T06:00:11+08:00\tfix: thing\there (#12)\n\n"); - expect(entries).toHaveLength(1); - expect(entries[0]).toMatchObject({ sha: "abc", pullNumber: 12 }); - expect(entries[0].subject).toBe("fix: thing\there (#12)"); - }); -}); - -describe("merge-loss classification", () => { - it("does not flag a file whose change is still present", () => { - const blobAt = reader({ "pre:kept.ts": "aaa", "head:kept.ts": "bbb" }); - expect(classifyMergeLoss({ ref: "head", blobAt, landings: [landing(1, ["kept.ts"])] }).findings).toEqual([]); - }); - - it("flags a file that reverted to its pre-merge blob", () => { - // The acf78bf case: the landing's contribution to this file is gone. - const blobAt = reader({ "pre:lost.ts": "ccc", "head:lost.ts": "ccc" }); - const { findings } = classifyMergeLoss({ ref: "head", blobAt, landings: [landing(1803, ["lost.ts"])] }); - expect(findings).toHaveLength(1); - expect(findings[0].pullNumber).toBe(1803); - expect(findings[0].revertedFiles).toEqual([{ file: "lost.ts", absent: false }]); - }); - - it("flags a file the pull request added that is absent again", () => { - // Absent before and absent now: the addition was undone. - const { findings } = classifyMergeLoss({ ref: "head", blobAt: reader({}), landings: [landing(1, ["added.ts"])] }); - expect(findings[0].revertedFiles).toEqual([{ file: "added.ts", absent: true }]); - }); - - it("does not flag a deletion that survived", () => { - // Present before, absent now — the pull request deleted it and it stayed deleted. - const blobAt = reader({ "pre:removed.ts": "ddd" }); - expect(classifyMergeLoss({ ref: "head", blobAt, landings: [landing(1, ["removed.ts"])] }).findings).toEqual([]); - }); - - it("skips commits with no pull request number instead of dropping them silently", () => { - const result = classifyMergeLoss({ - ref: "head", - blobAt: reader({}), - landings: [ - { sha: "x", date: "d", subject: "Merge branch 'main'", pullNumber: undefined, preRef: "pre", files: [] }, - ], - }); - expect(result.findings).toEqual([]); - expect(result.skipped).toHaveLength(1); - expect(result.scannedLandings).toBe(0); - }); - - it("orders findings by how much of the landing is missing", () => { - const blobAt = reader({ "pre:a.ts": "1", "head:a.ts": "1", "pre:b.ts": "2", "head:b.ts": "2" }); - const { findings } = classifyMergeLoss({ - ref: "head", - blobAt, - landings: [landing(10, ["a.ts"]), landing(20, ["a.ts", "b.ts"])], - }); - expect(findings.map((finding) => finding.pullNumber)).toEqual([20, 10]); - }); -}); - -describe("merge-loss reconciliation exemption", () => { - const request = `${INBOX}/11111111-1111-4111-8111-111111111111.json`; - const applied = `${INBOX}/applied/11111111-1111-4111-8111-111111111111.json`; - - it("does not report an inbox request that reconcile moved to applied/", () => { - // issues:reconcile moves the request verbatim; that is its lifecycle, not a loss. - const result = classifyMergeLoss({ - ref: "head", - blobAt: reader({ [`head:${applied}`]: "eee" }), - landings: [landing(1915, [request])], - }); - expect(result.findings).toEqual([]); - expect(result.filesExempted).toBe(1); - }); - - it("still reports an inbox request that vanished without an audit record", () => { - // No applied/ counterpart means the request was genuinely lost. - const result = classifyMergeLoss({ ref: "head", blobAt: reader({}), landings: [landing(1915, [request])] }); - expect(result.findings).toHaveLength(1); - expect(result.filesExempted).toBe(0); - }); - - it("credits the move only for a matching request filename", () => { - const blobAt = reader({ [`head:${applied}`]: "eee" }); - expect(isReconciliationMove(request, "head", blobAt)).toBe(true); - expect(isReconciliationMove(`${INBOX}/22222222-2222-4222-8222-222222222222.json`, "head", blobAt)).toBe(false); - expect(isReconciliationMove("src/lib/rag/rag.ts", "head", blobAt)).toBe(false); - expect(isReconciliationMove(applied, "head", blobAt)).toBe(false); - }); -}); +Yx-jםi+j[hܢm:NZewեNmGƭy \ No newline at end of file From 29a08d0575b02a00135bc7e4dbb4d2239c4c3a56 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:46:18 +0800 Subject: [PATCH 09/15] record PR 1944 review --- ...91de309f5e375293bda952b5d8479ef56a45808b8fda27bba37.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/4575cff3b5b7591de309f5e375293bda952b5d8479ef56a45808b8fda27bba37.record.md diff --git a/docs/branch-review-records/4575cff3b5b7591de309f5e375293bda952b5d8479ef56a45808b8fda27bba37.record.md b/docs/branch-review-records/4575cff3b5b7591de309f5e375293bda952b5d8479ef56a45808b8fda27bba37.record.md new file mode 100644 index 0000000000..aeba78275e --- /dev/null +++ b/docs/branch-review-records/4575cff3b5b7591de309f5e375293bda952b5d8479ef56a45808b8fda27bba37.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/ledger-process-tooling-50uqfc | d7adf35c18f6f23d9585d9442a0da99e7c90192b | PR #1944 review-and-fix | Fixed five validated P2 findings | audit self-test, targeted classifier probes, JSON parse, ledger and docs guards; Vitest unavailable: npm ci package downloads corrupted | From 07b184df70e31c55e3637af875af077d2f12f129 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:51:08 +0800 Subject: [PATCH 10/15] fix review follow-up file encoding --- docs/ledger-id-scheme-proposal.md | 150 +++++++- ...unchecked-indexed-access-migration-plan.md | 214 ++++++++++- scripts/audit-merge-loss.mjs | 343 +++++++++++++++++- tests/merge-loss-audit.test.ts | 153 +++++++- 4 files changed, 856 insertions(+), 4 deletions(-) diff --git a/docs/ledger-id-scheme-proposal.md b/docs/ledger-id-scheme-proposal.md index bfc1aa34d1..8ea2d36f46 100644 --- a/docs/ledger-id-scheme-proposal.md +++ b/docs/ledger-id-scheme-proposal.md @@ -1 +1,149 @@ -Yx-jםi+j[hܢN4NZewԌmGƭy \ No newline at end of file +# Collision-free outstanding-issue ids — design proposal + +**Status:** design only — no implementation, no id allocated by this document +**Ledger row:** `#168` (P2, rec) · closely related `#156` (same race, resolution-path evidence) +**Distinct from:** `#292`, which is two sessions colliding on the **work** a row describes. A +collision-free id leaves that untouched. +**Measured:** 2026-08-14 against `origin/main` at `d47aa6d` — 314 rows, marker at `next-id=317` + +--- + +## 1. The problem + +Ids are allocated read-modify-write against the `issues:next-id` marker **inside the very file +being edited**. `scripts/outstanding-issues.mjs` reads the marker, claims that number, and +rewrites the marker to `N + 1`. Two branches open at the same time both read `N` and both +write `N`. + +Because duplicate ids are unacceptable, a union merge driver is unsafe — `.gitattributes` says +so explicitly, which is why this file deliberately has no driver and **every overlapping append +conflicts by hand**. + +Manual resolution is where rows get dropped. The record is specific: + +- PR #1490 was closed during a conflict resolution and took the only record of four snapshots + with it (`#152`). +- One P3 row was renumbered `#135` → `#141` → `#145` → `#147` → `#149` across four sync cycles + because `main` had taken each id in turn (`#156`, measured on PR #1451). +- `#168` itself was written as `#159`, then renumbered because `main` had already used `#159`. +- The GitHub **Update branch** button produced a head carrying **two rows numbered `#141` and + two `next-id` markers**, leaving the marker _below_ `main`'s highest id — so the next + allocation would have reused a live number. `git merge` reported success; only + `npm run check:outstanding-issues` caught it (`#156`). + +The inbox (`scripts/ledger-inbox.mjs`) removed the mechanical errors — requests are immutable +UUID-named files and only `npm run issues:reconcile` writes the canonical ledger — but it +explicitly did not remove this one. Reconciliation still allocates from the marker, so two +reconcile branches still contend, and the single-writer discipline is what makes that +tolerable rather than fixed. + +--- + +## 2. What the id has to do + +Any scheme has to satisfy four things at once, which is why the obvious answers are wrong: + +1. **Collision-free without coordination.** Two sessions that never see each other must not + produce the same id. +2. **Stable once written.** Ids are cited by other rows, by review records under + `docs/branch-review-records/`, by `AGENTS.md`, by `.claude/skills/issues/SKILL.md`, and by + commit messages and PR bodies across the repo's history. An id that can be renumbered is the + defect, not the format. +3. **Readable enough to say aloud.** `/issues` output, the `SessionStart` hook, and every + handoff summary read ids back to a human. `#151` works in conversation; a bare + `01JQ8ZK3M7Q9V2W4X6Y8Z0ABCD` does not. +4. **Sortable by creation.** The ledger's queue and archive both read better in the order the + work arrived. + +--- + +## 3. Recommendation — ULID stored, permanent short display id allocated + +Allocate a **ULID** as the durable id and a **permanent short display id** for human use. + +- **ULID**, not UUIDv4, because a ULID is lexicographically sortable by its millisecond + timestamp prefix — requirement 4 — while remaining collision-free without coordination. + UUIDv7 is an equally good fit if a dependency is preferred over ~20 lines of local code; the + repo already generates UUIDv4 via `randomUUID()` in `ledger-inbox.mjs`, so neither needs a + new package. +- **Display id** starts as the first 6 characters of the ULID's random suffix, rendered + `#K3M7Q9`, and is stored with the row at allocation time. Six Crockford base-32 characters is + ~1.07 billion values, so a collision is rare at the observed rate of roughly 320 rows a year. +- **Collision handling happens before writing.** The allocator checks display-id uniqueness; if + the initial 6-character candidate is already allocated, it takes additional characters until + it finds an unused candidate, then stores that result. Existing display ids are never + lengthened or otherwise changed. This preserves every written `#K3M7Q9` citation while keeping + the durable ULID as the collision-free machine identity. + +Rejected alternatives, briefly: + +- **Timestamp + slug** (`#2026-08-14-merge-loss`) is readable and sortable, but two sessions + filing similar rows on the same day collide on the slug, and the slug wants to change when + the row is re-scoped — reintroducing renumbering by another name. +- **Content hash** is collision-free but neither sortable nor stable: any edit to the row + changes its identity. +- **Keeping sequential ids and adding a lock** does not work across branches. There is no + shared state at allocation time; that is the whole problem. + +--- + +## 4. Migration path + +**The 314 existing sequential ids keep their literal ids, permanently.** Renumbering them is +off the table — they are cited across the ledger, the review records, the agent instructions +and the entire commit history, and a rewrite would invalidate every one of those citations +while producing exactly the renumbering churn this row exists to end. + +So the two forms coexist, and the migration is additive: + +| Step | Change | Risk | +| ---- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | +| 1 | Widen every id validator to accept both `#NNN` and the new stored display id, while allocation still uses the marker. No behaviour change; purely permissive. | Low. Fully reversible. | +| 2 | Add ULID and display-id fields to new rows and switch allocation to them. The `issues:next-id` marker stops being read. | Medium — this is the cutover. | +| 3 | Remove the marker and its `next-id` guards once no writer consults it. | Low, but only after step 2 has been through a few real appends. | +| 4 | Reconsider a union merge driver, which becomes safe only once **no** id is allocated read-modify-write. | Deliberately last. See the Stop below. | + +**Every place that currently assumes a sequential id** — all of these need step 1 before +anything else moves: + +- `scripts/ledger-inbox.mjs` — `/^#\d{3,}$/` in `validateRequest`, twice (the `done` and + `update` actions). +- `scripts/check-outstanding-issues.mjs` — `ID_CELL = /^#\d+$/`; the + `MARKER = //` parse; the `nextId <= highest` assertion; and + the `String(highest).padStart(3, "0")` formatting in its messages. +- `scripts/outstanding-issues.mjs` — the allocator that reads `parsed.nextId`, formats + `#${String(number).padStart(3, "0")}`, and rewrites the marker to `nextId + 1`. +- `scripts/issues-report.mjs` and `.claude/hooks/issues-surface.sh`, which render ids back to + the reader. + +A row-per-file variant — one file per row in a new per-row directory under `docs/`, with the +table generated the way `docs/site-map.md` already is — removes the shared hunk entirely and is +the stronger end state. It is a larger change and should be decided separately; the id scheme +is a prerequisite for it either way, since per-row filenames need collision-free names. + +--- + +## 5. What this does not fix + +`#292` — two sessions independently building the same queued item — is untouched by any of +this. That is a collision on the **work** a row describes, not on its id, and the mitigation +there is the open-PR check already written into the three skills. Do not conflate them when +scoping the implementation. + +`#156`'s second finding is also untouched: a merge that silently drops an appended prose block +is invisible to `check:outstanding-issues`, which validates ids and structure rather than +whether both sides' text survived. A collision-free id makes such merges rarer; it does not +make them detectable. `npm run audit:merge-loss` is the closest thing the repo now has to that +detection. + +--- + +## 6. Stop + +- **Do not reinstate `merge=union` while ids are sequential.** That combination was tried in + PR #1416 and removed for duplicating rows and the marker. It only becomes safe after step 3. +- **Do not renumber existing rows** to make the ledger uniform. The citations are the point. +- **Do not implement this from this document alone.** It is a proposal; the cutover in step 2 + wants its own PR, its own review, and a check that both id forms round-trip through + `issues:add`, `issues:update`, `issues:done` and `issues:reconcile` before the marker is + removed. diff --git a/docs/no-unchecked-indexed-access-migration-plan.md b/docs/no-unchecked-indexed-access-migration-plan.md index a732345f57..7910f17f9a 100644 --- a/docs/no-unchecked-indexed-access-migration-plan.md +++ b/docs/no-unchecked-indexed-access-migration-plan.md @@ -1 +1,213 @@ -Yx-jםi+j[hܢO;NZewԌέGƭy +# `noUncheckedIndexedAccess` — staged migration plan + +**Status:** plan only — this document changes no code and does not touch `tsconfig.json` +**Ledger row:** `#211` (P2, task) · related `#212` (`as unknown as` casts), `#213` (empty catch handlers) +**Measured:** 2026-08-14 against `origin/main` at `d47aa6d` +**Source finding:** [`docs/review-findings-2026-08-02.md`](review-findings-2026-08-02.md) §6 + +`tsconfig.json` sets `strict: true` but not `noUncheckedIndexedAccess`. Without the flag, +`array[0]` is typed `T` even when the array is empty and `record[key]` is typed `V` even when +the key is absent, so every out-of-bounds read is invisible to the compiler and surfaces as a +runtime `undefined` — including on the answer path, where the failure lands in front of a +clinician. + +Row `#211` carries an explicit **Stop:** do not flip the flag on `main` without a staged plan. +This is that plan. + +--- + +## 1. Current measurement + +Measured by extending `tsconfig.json` with `noUncheckedIndexedAccess: true` in a throwaway +config outside the repo tree and running `tsc --noEmit`. **1,445 errors across 269 files** — +up from the 1,266 recorded on 2026-08-02, because the flag is off and nothing stops new +unchecked indexing from landing. That drift rate is itself an argument for the ratchet in §3. + +| Bucket | Errors | Share | Character | +| ------------------------ | -----: | ----: | --------------------------------------------------------------------- | +| `tests/**` | 713 | 49.3% | Mechanical. A wrong guard fails a test, it does not reach production. | +| Mockups (design scratch) | 237 | 16.4% | Mechanical. 404s in production; already gate-exempt for wiring. | +| `src/lib/**` (non-RAG) | 211 | 14.6% | Mixed — contains the clinical hot spots. | +| `src/components/**` | 167 | 11.6% | Mostly mechanical render-path indexing. | +| `scripts/**` | 57 | 3.9% | Mechanical; tooling-plane, failures are loud and local. | +| `src/lib/rag/**` | 32 | 2.2% | **Protected surface.** See §5. | +| `worker/**` | 27 | 1.9% | Manual — ingestion runtime. | +| `src/` other | 1 | 0.1% | — | + +Two-thirds of the population (tests plus mockups, 950 errors) carries no production +consequence whatever. That is what makes staging worthwhile: the risky remainder is ~500 +errors, not 1,445. + +**Error shape:** `TS2532` "object is possibly undefined" (569) and `TS18048` "…is possibly +undefined" (455) together are 71% — these are the ones a guard fixes. `TS2345`/`TS2322` +(368) are `string | undefined` flowing into a parameter typed `string`, which more often +needs a real decision about what the absent case means. + +**Heaviest files:** `tests/ui-smoke.spec.ts` (43), `src/lib/demo-data.ts` (42), +`src/components/master-document-flow-mockups.tsx` (41), `src/lib/answer-verification.ts` (41), +`tests/evidence.test.ts` (40), `tests/ui-phone-scroll-page-owned.spec.ts` (38), +`tests/clinical-search.test.ts` (28), `src/lib/rag/rag-extractive-answer.ts` (23), +`worker/main.ts` (23), `src/lib/evidence.ts` (19). + +**To reproduce:** create a config outside the repo that extends `tsconfig.json`, adds +`"noUncheckedIndexedAccess": true`, and excludes `.next` (build artefacts produce unrelated +errors), then run `./node_modules/.bin/tsc --noEmit --project `. Do not add the +throwaway config to the repo — `docs:check-links` and the tsconfig gates both notice. + +--- + +## 2. Why this cannot simply be split by directory + +`noUncheckedIndexedAccess` is a whole-project compiler option. It cannot be enabled for one +directory: narrowing `include` does not help either, because TypeScript still loads and +reports errors in every transitively imported file, so a tests-only project pulls all of +`src/lib` in with it. + +So the flag itself flips exactly once, in the final PR. Everything before that is remediation +performed against the measurement, verified by a ratchet rather than by `npm run typecheck`. + +--- + +## 3. The ratchet + +Stage 1 adds a baseline file plus a check, in the shape this repo already uses for +`scripts/design-system-contract-baseline.json` (`metrics` + `debtByPath`) and +`bundle-budget.json`: + +(Paths below are proposed, not existing — they are written without a directory prefix so the +`docs:check-links` and `docs:check-scripts` gates do not read them as stale references.) + +- A baseline file `no-unchecked-indexed-access-baseline.json` under `scripts/` — + `{ measuredOn, total, debtByPath }` mapping each file still permitted to have errors to its + current count. +- A checker `check-no-unchecked-indexed-access.mjs` under `scripts/` — runs `tsc` with the flag + against a generated config, then fails when a file **absent** from `debtByPath` has any + error, or when a listed file's count **rises**. Falling counts are fine; the baseline is + refreshed as stages land. +- A `package.json` entry named `check:no-unchecked-indexed-access`, run per stage and by the + final PR. + +This makes the migration monotonic: a stage cannot be undone by the next week's merges, and +new code cannot add debt while the migration is in flight — which is precisely what let the +count drift from 1,266 to 1,445. + +**Do not** add this to `verify:cheap:internal` while the migration is in flight. A full `tsc` +run is not a cheap gate, and `scripts/check-gate-manifest.mjs` would additionally require a +matching `static-pr` step in `.github/workflows/ci.yml`. Run it per stage; consider promoting +it only after stage 6, when the flag is on and `npm run typecheck` covers it anyway. + +--- + +## 4. Stages + +One PR per stage, in this order. Cheapest and most consequence-free first, so the mechanical +bulk lands before anyone has to think hard. + +### Stage 1 · Ratchet only — `MECHANICAL` + +- **Outcome:** the debt is measured, pinned, and cannot grow. +- **Files:** the checker and baseline named in §3, plus `package.json` and + `docs/scripts-index.md`. +- **Risk:** none — no product file changes. +- **Verification:** the new `check:no-unchecked-indexed-access` entry passes at the baseline; a + deliberately introduced `arr[0]` in a clean file makes it fail. + +### Stage 2 · `tests/**` — 713 errors — `MECHANICAL` + +- **Outcome:** roughly half the population gone, with no production surface touched. +- **Approach:** use a non-null assertion `!` only where a nearby assertion deliberately proves + the invariant (`const rows = parse(x); expect(rows).toHaveLength(3)`). Do not make this a + blanket replacement: `expect(rows[0]?.id).toBe(…)` fails with `undefined` when `rows` is + empty, unless `undefined` is the expected value. Choose optional access or an explicit guard + when that better expresses the intended test. +- **Risk:** low, but real. Reviewers check that each assertion still states its intended empty + case and that `!` is backed by a local invariant. +- **Verification:** `npm run test`; the Playwright specs in this bucket + (`tests/ui-smoke.spec.ts`, `tests/ui-phone-scroll-page-owned.spec.ts`) are compiled by + `typecheck` but only executed by `npm run verify:ui`, so typecheck is the gate that matters + for them. + +### Stage 3 · Mockups — 237 errors — `MECHANICAL` + +- **Outcome:** design scratch off the books. +- **Files:** `src/app/mockups/**`, `*-mockups.tsx`. +- **Risk:** none. These 404 in production. Note they are still compiled and still weighed by + `check:bundle-budget` against the `mockups` baseline — "gate-exempt" does not mean "free". +- **Verification:** `npm run typecheck`, `npm run check:bundle-budget`. + +### Stage 4 · `scripts/**` and `src/components/**` — 224 errors — `MECHANICAL`, spot-reviewed + +- **Outcome:** the tooling plane and the render path. +- **Approach:** `??` with a sensible empty default in render code; a thrown error in scripts, + where failing loudly is correct and silence is not. +- **Risk:** low. The component work can change rendered output if a `??` default differs from + what the old `undefined` produced — check any empty-state or list-rendering change. +- **Verification:** `npm run typecheck`, `npm run test`, and `npm run verify:ui` only if a + component's rendered output actually changed. + +### Stage 5 · `worker/**` and `src/lib/**` non-clinical — `MANUAL` + +- **Outcome:** ingestion and the general library. +- **Approach:** `worker/main.ts:901-942` repeatedly indexes `preparedImage`/`image` arrays and + passes the results to functions typed `ExtractedImage`. A shorter-than-expected array throws + today; the fix is a real guard that skips or fails the job, not a `!` that preserves the + throw. `src/lib/demo-data.ts` (42) is the largest single file and is genuinely mechanical — + it is synthetic fixture data. +- **Risk:** medium. Ingestion is a background worker; a wrong guard turns a loud crash into a + silently skipped image. +- **Verification:** `npm run typecheck`, `npm run test`, plus `npm run check:production-readiness` + (ingestion is a domain change under AGENTS.md). + +### Stage 6 · Clinical hot spots, then flip the flag — `MANUAL, HIGHEST CARE` + +- **Files:** `src/lib/answer-verification.ts` (41), `src/lib/rag/rag-extractive-answer.ts` (23), + `src/lib/evidence.ts` (19), `src/lib/document-summary-formatting.ts` (16), and the remaining + `src/lib/rag/**`. +- **Approach:** every site individually. `rag-extractive-answer.ts` is the deterministic + source-only fallback used _when generation has already failed its quality gate_ — an + out-of-bounds throw there means the fallback fails too, and the user gets nothing instead of + a cited answer. `answer-verification.ts` indexes into arrays that may be empty while deciding + whether an answer is safe to show; a `!` that converts a type error into a runtime throw + crashes the verification gate itself. Neither file wants `!` anywhere. +- **Then:** set `"noUncheckedIndexedAccess": true` in `tsconfig.json`, delete the baseline and + its check, and remove the `check:no-unchecked-indexed-access` entry. +- **Verification:** `npm run typecheck`, `npm run test`, `npm run check:production-readiness`, + and the RAG requirements in §5. + +--- + +## 5. The RAG carve-out + +Stage 6 touches `src/lib/rag/**`, which is a protected ranking surface under AGENTS.md. +Three obligations apply and none is optional: + +1. **Flag the task to the user before editing anything under `src/lib/rag/**`** — including a + change this mechanical. +2. The PR body needs an explicit `RAG impact:` line or `scripts/pr-policy.mjs` blocks the + merge. A guard that only adds a narrowing check should be able to state + `RAG impact: no retrieval behaviour change — adds undefined guards without touching +comparator order, scoring, or selection`, but that claim has to be **true**: read + `docs/rag-behaviour/` first and confirm no comparator key, clamped-score contract, or + selection threshold moved. +3. If any guard does change ordering or selection — for example a `?? 0` default that alters a + sort — it is a behaviour change and needs a live eval-canary pair. That is provider-backed + (~$1–2) and needs explicit user approval. + +Splitting `src/lib/rag/**` into its own final PR, after the rest of stage 6, keeps the +governance requirement off the other files. + +--- + +## 6. Tracking + +Progress lives in ledger row `#211`, updated with `npm run issues:update` after each stage +lands (never by hand-editing `docs/outstanding-issues.md`). Record the stage number, the PR, +and the new total from the baseline file, so a later reader can tell how far the migration got +without re-running `tsc`. + +Do not close `#211` until the flag is on in `tsconfig.json` and the baseline file is gone. +A partially-migrated repo with the flag still off has none of the protection and all of the +churn, so an abandoned migration is worse than an unstarted one. + +**Stop:** do not flip the flag on `main` ahead of stage 6, and do not silence a stage by adding +files back to the baseline — the baseline only ever shrinks. diff --git a/scripts/audit-merge-loss.mjs b/scripts/audit-merge-loss.mjs index cb2c28616b..053179ca53 100644 --- a/scripts/audit-merge-loss.mjs +++ b/scripts/audit-merge-loss.mjs @@ -1 +1,342 @@ -Yx-jםi+j[hܢ:NZewԌNmGƭy \ No newline at end of file +#!/usr/bin/env node +/** + * audit-merge-loss — find merged pull requests whose content was silently + * reverted by a later merge resolution. + * + * Why this exists. On 2026-08-11 merge commit acf78bf ("Merge remote-tracking + * branch origin/main into probe2-1815") took the stale branch side of several + * manual conflict resolutions and reverted seven already-merged PRs — #1800, + * #1803, #1804, #1809, #1811, #1815 and #1796. Nothing went red, because the + * reverts took each PR's tests in the same stroke: no assertion survived to + * fail. The docs casualties were repaired by 55f51ab; the source casualties + * went unnoticed for two days. Care at the keyboard is demonstrably not the + * control here — commit 6f8c70d shows a human consciously preserving the #1803 + * migration and a later merge in the same chain undoing it anyway. + * + * The measurement. For each pull request that landed on the target ref inside + * the window, compare the ref's current tree entry for every file that landing + * changed against that file's tree entry at the landing's first parent. Equality + * means the landing's contribution to that file is no longer present. Tree + * entries include the file mode and blob OID, so mode-only landings are not + * mistaken for losses while the comparison stays cheap over a wide window. + * + * ADVISORY BY DESIGN — this exits 0 even when it finds something. A deliberate + * later revert is byte-identical to an accidental one at blob level, so a + * positive is a question for a human, not a verdict. The report names the pull + * request, the landing commit and every affected file so that question can be + * answered. `--strict` exits 1 for a caller that wants a hard failure. + * + * DELIBERATELY NOT WIRED INTO CI OR A SCHEDULE. Running this automatically is + * an operational change that needs its own pull request and explicit approval; + * its absence from .github/workflows/ is a decision, not an oversight. It is + * also not in `verify:cheap:internal`: check-gate-manifest.mjs would then + * require a matching ci.yml step. + * + * Run: npm run audit:merge-loss + * npm run audit:merge-loss -- --since 30 --ref origin/main + * npm run audit:merge-loss -- --json + * npm run audit:merge-loss -- --strict # exit 1 on any finding + * node scripts/audit-merge-loss.mjs --self-test + */ +import { execFileSync } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const DEFAULT_REF = "origin/main"; +const DEFAULT_WINDOW_DAYS = 14; +const LOG_FORMAT = "%H%x09%cI%x09%s"; + +function git(args) { + return execFileSync("git", args, { cwd: ROOT, encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }).trim(); +} + +function tryGit(args) { + try { + return git(args); + } catch { + return undefined; + } +} + +/** + * The pull request a landing commit belongs to, or undefined. + * + * Both shapes occur on this repo's main: a merge landing ("Merge pull request + * #1935 from …") and a squash landing ("… (#1933)"). A squash subject can carry + * more than one issue reference, so the pull request is the LAST parenthesised + * number — GitHub appends it. + */ +export function parsePullNumber(subject) { + const merge = /^Merge pull request #(\d+)\b/.exec(String(subject ?? "")); + if (merge) return Number(merge[1]); + const squashed = [...String(subject ?? "").matchAll(/\(#(\d+)\)/g)]; + const last = squashed.at(-1); + return last ? Number(last[1]) : undefined; +} + +/** Parse `git log --format=%H\t%cI\t%s` output into landing records. */ +export function parseLogEntries(rawLog) { + return String(rawLog ?? "") + .split(/\r?\n/) + .filter((line) => line.trim().length > 0) + .map((line) => { + const [sha, date, ...rest] = line.split("\t"); + const subject = rest.join("\t"); + return { sha, date, subject, pullNumber: parsePullNumber(subject) }; + }); +} + +const INBOX = "docs/outstanding-issues-inbox"; + +/** + * The one exemption, kept deliberately narrow. + * + * `npm run issues:reconcile` MOVES a pending request from the inbox to + * `applied/` verbatim, so every pull request that queues one looks like it + * added a file that is now gone. That is the request's designed lifecycle, not + * a loss — and it accounted for six of fifteen findings on the first real run, + * which would have buried the genuine #1803 signal. The move is only credited + * when the identically-named audit record exists at the target ref with the + * same tree entry as the request at its landing. This confirms the move was + * verbatim rather than hiding a mismatched or corrupted request. + */ +export function isReconciliationMove(file, landingRef, ref, entryAt) { + const match = new RegExp(`^${INBOX}/([^/]+\\.json)$`).exec(file); + if (!match) return false; + const requestAtLanding = entryAt(landingRef, file); + return requestAtLanding !== null && entryAt(ref, `${INBOX}/applied/${match[1]}`) === requestAtLanding; +} + +/** + * Compare each landing's contribution against the ref's current state. + * + * `entryAt(ref, file)` returns a tree entry (mode, object type, and blob OID), + * or null when the path does not exist at that ref. Injected so the whole + * classifier is testable without git. + * + * A file is reported when its current blob equals its pre-landing blob. Both + * being null counts: that is the "the pull request added this file and it is + * gone again" case. A file the pull request DELETED and which is still absent + * does not match, because its pre-landing blob existed — the deletion survived. + * + * Pure and exported for the self-test and focused tests. + * + * @typedef {{ sha: string, date: string, subject: string, pullNumber: number | undefined, + * preRef: string, files?: string[] }} Landing + * @param {{ landings?: Landing[], entryAt: (ref: string, file: string) => string | null, + * ref?: string }} options + */ +export function classifyMergeLoss({ landings = [], entryAt, ref = DEFAULT_REF }) { + const findings = []; + let filesCompared = 0; + let filesExempted = 0; + const skipped = []; + for (const landing of landings) { + if (landing.pullNumber === undefined) { + skipped.push(landing); + continue; + } + const reverted = []; + for (const file of landing.files ?? []) { + filesCompared += 1; + const before = entryAt(landing.preRef, file); + const now = entryAt(ref, file); + if (before !== now) continue; + if (now === null && isReconciliationMove(file, landing.sha, ref, entryAt)) { + filesExempted += 1; + continue; + } + reverted.push({ file, absent: now === null }); + } + if (reverted.length > 0) { + findings.push({ + pullNumber: landing.pullNumber, + sha: landing.sha, + date: landing.date, + subject: landing.subject, + changedFiles: (landing.files ?? []).length, + revertedFiles: reverted, + }); + } + } + findings.sort((a, b) => b.revertedFiles.length - a.revertedFiles.length || a.pullNumber - b.pullNumber); + return { findings, scannedLandings: landings.length - skipped.length, skipped, filesCompared, filesExempted }; +} + +function resolveArgs(argv) { + const value = (name) => { + const index = argv.indexOf(name); + return index >= 0 ? argv[index + 1] : undefined; + }; + const rawSince = value("--since"); + const since = rawSince === undefined ? DEFAULT_WINDOW_DAYS : Number(rawSince); + if (!Number.isFinite(since) || since <= 0) { + throw new Error(`--since expects a positive number of days, received "${rawSince}"`); + } + return { + ref: value("--ref") ?? DEFAULT_REF, + since, + json: argv.includes("--json"), + strict: argv.includes("--strict"), + }; +} + +function collectLandings(ref, since) { + const rawLog = git(["log", "--first-parent", `--since=${since} days ago`, `--format=${LOG_FORMAT}`, ref]); + return parseLogEntries(rawLog).map((landing) => { + const preRef = `${landing.sha}^1`; + const names = tryGit(["diff", "--name-only", preRef, landing.sha]); + return { ...landing, preRef, files: names === undefined ? [] : names.split(/\r?\n/).filter(Boolean) }; + }); +} + +function treeEntryReader() { + const cache = new Map(); + return (ref, file) => { + const key = `${ref}:${file}`; + if (!cache.has(key)) { + const entry = tryGit(["ls-tree", ref, "--", file]); + cache.set(key, entry ? entry.split("\\t", 1)[0] : null); + } + return cache.get(key); + }; +} + +function report(result, { ref, since, strict }) { + const { findings, scannedLandings, skipped, filesCompared, filesExempted } = result; + console.log( + `[merge-loss] scanned ${scannedLandings} pull request landing(s) on ${ref} over the last ${since} day(s); ` + + `compared ${filesCompared} file(s).`, + ); + if (skipped.length > 0) { + console.log( + `[merge-loss] ${skipped.length} first-parent commit(s) carried no pull request number and were skipped.`, + ); + } + if (filesExempted > 0) { + console.log( + `[merge-loss] ${filesExempted} inbox request(s) were excluded: issues:reconcile moved them to applied/ verbatim.`, + ); + } + if (findings.length === 0) { + console.log("[merge-loss] No landing has been reverted to its pre-merge state."); + return 0; + } + + console.log(""); + console.log(`[merge-loss] ${findings.length} landing(s) look reverted — HUMAN CONFIRMATION REQUIRED.`); + console.log("A deliberate later revert is identical to an accidental one at blob level, so this is a"); + console.log("question, not a verdict. For each entry below, decide whether the change was meant to go."); + for (const finding of findings) { + console.log(""); + console.log(` PR #${finding.pullNumber} — ${finding.subject}`); + console.log(` landed ${finding.date} as ${finding.sha.slice(0, 12)}`); + console.log( + ` ${finding.revertedFiles.length} of ${finding.changedFiles} changed file(s) match the pre-merge tree entry:`, + ); + for (const entry of finding.revertedFiles) { + console.log(` - ${entry.file}${entry.absent ? " (added by the PR, absent now)" : ""}`); + } + console.log(` Inspect: git diff ${finding.sha}^1 ${finding.sha} -- `); + } + console.log(""); + console.log("Confirmed losses are re-landed as their own pull request; record the decision with npm run issues:add."); + return strict ? 1 : 0; +} + +function selfTest() { + if (parsePullNumber("Merge pull request #1935 from BigSimmo/codex/fix-x") !== 1935) { + throw new Error("self-test failed: merge-landing subject not parsed"); + } + if (parsePullNumber("ci: speed iteration without weakening gates (#1926)") !== 1926) { + throw new Error("self-test failed: squash-landing subject not parsed"); + } + if (parsePullNumber("fix: close (#12) properly (#1930)") !== 1930) { + throw new Error("self-test failed: trailing pull request number not preferred"); + } + if (parsePullNumber("Merge branch 'main' into feature") !== undefined) { + throw new Error("self-test failed: a non-pull-request subject produced a number"); + } + + const entries = parseLogEntries("abc\t2026-08-14T06:00:11+08:00\tMerge pull request #10 from o/b\n"); + if (entries.length !== 1 || entries[0].pullNumber !== 10 || entries[0].sha !== "abc") { + throw new Error("self-test failed: log parsing is incorrect"); + } + + const blobs = new Map([ + ["pre:kept.ts", "aaa"], + ["head:kept.ts", "bbb"], + ["pre:lost.ts", "ccc"], + ["head:lost.ts", "ccc"], + ["head:deleted.ts", null], + ["pre:deleted.ts", "ddd"], + ]); + const entryAt = (ref, file) => blobs.get(`${ref}:${file}`) ?? null; + const { findings } = classifyMergeLoss({ + ref: "head", + entryAt, + landings: [ + { sha: "s1", date: "d", subject: "x (#1)", pullNumber: 1, preRef: "pre", files: ["kept.ts"] }, + { sha: "s2", date: "d", subject: "y (#2)", pullNumber: 2, preRef: "pre", files: ["lost.ts", "kept.ts"] }, + { sha: "s3", date: "d", subject: "z (#3)", pullNumber: 3, preRef: "pre", files: ["deleted.ts"] }, + { sha: "s4", date: "d", subject: "w (#4)", pullNumber: 4, preRef: "pre", files: ["added.ts"] }, + ], + }); + const byPull = new Map(findings.map((finding) => [finding.pullNumber, finding])); + if (byPull.has(1)) throw new Error("self-test failed: a surviving change was reported as lost"); + if (byPull.get(2)?.revertedFiles.length !== 1) throw new Error("self-test failed: a reverted file was not reported"); + if (byPull.has(3)) throw new Error("self-test failed: a surviving deletion was reported as lost"); + if (!byPull.get(4)) throw new Error("self-test failed: a vanished added file was not reported"); + + const request = `${INBOX}/11111111-1111-4111-8111-111111111111.json`; + const reconciled = classifyMergeLoss({ + ref: "head", + entryAt: (reference, file) => { + if (reference === "s5" && file === request) return "100644 blob eee"; + return reference === "head" && file === `${INBOX}/applied/${path.posix.basename(request)}` + ? "100644 blob eee" + : null; + }, + landings: [{ sha: "s5", date: "d", subject: "q (#5)", pullNumber: 5, preRef: "pre", files: [request] }], + }); + if (reconciled.findings.length !== 0 || reconciled.filesExempted !== 1) { + throw new Error("self-test failed: a reconciled inbox request was reported as a merge loss"); + } + console.error("merge-loss audit self-test passed."); +} + +function main() { + if (process.argv.includes("--self-test")) return selfTest(); + const options = resolveArgs(process.argv.slice(2)); + + if (tryGit(["rev-parse", "--is-shallow-repository"]) === "true") { + console.error("[merge-loss] this is a shallow clone; pre-merge parents are unavailable and a clean sweep here"); + console.error("[merge-loss] would be meaningless. Re-run after `git fetch --unshallow`."); + process.exitCode = 1; + return; + } + if (tryGit(["rev-parse", "--verify", "--quiet", `${options.ref}^{commit}`]) === undefined) { + console.error(`[merge-loss] cannot resolve ref "${options.ref}"; fetch it or pass --ref .`); + process.exitCode = 1; + return; + } + + const landings = collectLandings(options.ref, options.since); + const result = classifyMergeLoss({ landings, entryAt: treeEntryReader(), ref: options.ref }); + if (options.json) { + console.log(JSON.stringify({ ref: options.ref, sinceDays: options.since, ...result }, null, 2)); + process.exitCode = options.strict && result.findings.length > 0 ? 1 : 0; + return; + } + process.exitCode = report(result, options); +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + try { + main(); + } catch (error) { + console.error(`[merge-loss] failed: ${error instanceof Error ? error.message : String(error)}`); + process.exitCode = 1; + } +} diff --git a/tests/merge-loss-audit.test.ts b/tests/merge-loss-audit.test.ts index 7043c4cfd2..21c17cba26 100644 --- a/tests/merge-loss-audit.test.ts +++ b/tests/merge-loss-audit.test.ts @@ -1 +1,152 @@ -Yx-jםi+j[hܢm:NZewեNmGƭy \ No newline at end of file +import { describe, expect, it } from "vitest"; + +import { + classifyMergeLoss, + isReconciliationMove, + parseLogEntries, + parsePullNumber, +} from "../scripts/audit-merge-loss.mjs"; + +const INBOX = "docs/outstanding-issues-inbox"; + +/** Tree-entry table keyed `:`; a missing key means the path does not exist. */ +const reader = (blobs: Record) => (ref: string, file: string) => blobs[`${ref}:${file}`] ?? null; + +const landing = (pullNumber: number, files: string[]) => ({ + sha: `sha${pullNumber}`, + date: "2026-08-11T00:00:00+00:00", + subject: `subject (#${pullNumber})`, + pullNumber, + preRef: "pre", + files, +}); + +describe("merge-loss subject parsing", () => { + it("reads a merge landing", () => { + expect(parsePullNumber("Merge pull request #1935 from BigSimmo/codex/fix-ecg")).toBe(1935); + }); + + it("reads a squash landing", () => { + expect(parsePullNumber("ci: speed iteration without weakening gates (#1926)")).toBe(1926); + }); + + it("prefers the trailing number when the subject also cites an issue", () => { + // GitHub appends the PR number, so the last parenthesised number wins. + expect(parsePullNumber("docs(issues): close #170 and (#309) partially (#1925)")).toBe(1925); + }); + + it("returns undefined for a commit that is not a pull request landing", () => { + expect(parsePullNumber("Merge branch 'main' into claude/feature")).toBeUndefined(); + expect(parsePullNumber("wip")).toBeUndefined(); + expect(parsePullNumber("")).toBeUndefined(); + }); + + it("parses tab-delimited log lines and keeps a subject containing tabs", () => { + const entries = parseLogEntries("abc\t2026-08-14T06:00:11+08:00\tfix: thing\there (#12)\n\n"); + expect(entries).toHaveLength(1); + expect(entries[0]).toMatchObject({ sha: "abc", pullNumber: 12 }); + expect(entries[0].subject).toBe("fix: thing\there (#12)"); + }); +}); + +describe("merge-loss classification", () => { + it("does not flag a file whose change is still present", () => { + const entryAt = reader({ "pre:kept.ts": "aaa", "head:kept.ts": "bbb" }); + expect(classifyMergeLoss({ ref: "head", entryAt, landings: [landing(1, ["kept.ts"])] }).findings).toEqual([]); + }); + + it("flags a file that reverted to its pre-merge blob", () => { + // The acf78bf case: the landing's contribution to this file is gone. + const entryAt = reader({ "pre:lost.ts": "ccc", "head:lost.ts": "ccc" }); + const { findings } = classifyMergeLoss({ ref: "head", entryAt, landings: [landing(1803, ["lost.ts"])] }); + expect(findings).toHaveLength(1); + expect(findings[0].pullNumber).toBe(1803); + expect(findings[0].revertedFiles).toEqual([{ file: "lost.ts", absent: false }]); + }); + + it("flags a file the pull request added that is absent again", () => { + // Absent before and absent now: the addition was undone. + const { findings } = classifyMergeLoss({ ref: "head", entryAt: reader({}), landings: [landing(1, ["added.ts"])] }); + expect(findings[0].revertedFiles).toEqual([{ file: "added.ts", absent: true }]); + }); + + it("does not flag a deletion that survived", () => { + // Present before, absent now — the pull request deleted it and it stayed deleted. + const entryAt = reader({ "pre:removed.ts": "ddd" }); + expect(classifyMergeLoss({ ref: "head", entryAt, landings: [landing(1, ["removed.ts"])] }).findings).toEqual([]); + }); + + it("does not flag a surviving mode-only change", () => { + const entryAt = reader({ + "pre:script.sh": "100644 blob aaa", + "head:script.sh": "100755 blob aaa", + }); + expect(classifyMergeLoss({ ref: "head", entryAt, landings: [landing(1, ["script.sh"])] }).findings).toEqual([]); + }); + + it("skips commits with no pull request number instead of dropping them silently", () => { + const result = classifyMergeLoss({ + ref: "head", + entryAt: reader({}), + landings: [ + { sha: "x", date: "d", subject: "Merge branch 'main'", pullNumber: undefined, preRef: "pre", files: [] }, + ], + }); + expect(result.findings).toEqual([]); + expect(result.skipped).toHaveLength(1); + expect(result.scannedLandings).toBe(0); + }); + + it("orders findings by how much of the landing is missing", () => { + const entryAt = reader({ "pre:a.ts": "1", "head:a.ts": "1", "pre:b.ts": "2", "head:b.ts": "2" }); + const { findings } = classifyMergeLoss({ + ref: "head", + entryAt, + landings: [landing(10, ["a.ts"]), landing(20, ["a.ts", "b.ts"])], + }); + expect(findings.map((finding) => finding.pullNumber)).toEqual([20, 10]); + }); +}); + +describe("merge-loss reconciliation exemption", () => { + const request = `${INBOX}/11111111-1111-4111-8111-111111111111.json`; + const applied = `${INBOX}/applied/11111111-1111-4111-8111-111111111111.json`; + + it("does not report an inbox request that reconcile moved to applied/", () => { + // issues:reconcile moves the request verbatim; that is its lifecycle, not a loss. + const result = classifyMergeLoss({ + ref: "head", + entryAt: reader({ [`sha1915:${request}`]: "100644 blob eee", [`head:${applied}`]: "100644 blob eee" }), + landings: [landing(1915, [request])], + }); + expect(result.findings).toEqual([]); + expect(result.filesExempted).toBe(1); + }); + + it("still reports an inbox request that vanished without an audit record", () => { + // No applied/ counterpart means the request was genuinely lost. + const result = classifyMergeLoss({ ref: "head", entryAt: reader({}), landings: [landing(1915, [request])] }); + expect(result.findings).toHaveLength(1); + expect(result.filesExempted).toBe(0); + }); + + it("credits only a verbatim move to the matching request filename", () => { + const entryAt = reader({ [`landing:${request}`]: "100644 blob eee", [`head:${applied}`]: "100644 blob eee" }); + expect(isReconciliationMove(request, "landing", "head", entryAt)).toBe(true); + expect( + isReconciliationMove(`${INBOX}/22222222-2222-4222-8222-222222222222.json`, "landing", "head", entryAt), + ).toBe(false); + expect(isReconciliationMove("src/lib/rag/rag.ts", "landing", "head", entryAt)).toBe(false); + expect(isReconciliationMove(applied, "landing", "head", entryAt)).toBe(false); + }); + + it("reports a missing request when its applied record has different contents", () => { + const result = classifyMergeLoss({ + ref: "head", + entryAt: reader({ [`sha1915:${request}`]: "100644 blob original", [`head:${applied}`]: "100644 blob changed" }), + landings: [landing(1915, [request])], + }); + expect(result.findings).toHaveLength(1); + expect(result.filesExempted).toBe(0); + }); +}); From f06a1660749c44679ad4be0b7609475c7e6e6154 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:51:34 +0800 Subject: [PATCH 11/15] record PR 1944 encoding correction --- ...ff006a1deb9148167b6bfe7cd5a364f7fd8d8da5287aa2a4e38.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/7a1bb8ea354bbff006a1deb9148167b6bfe7cd5a364f7fd8d8da5287aa2a4e38.record.md diff --git a/docs/branch-review-records/7a1bb8ea354bbff006a1deb9148167b6bfe7cd5a364f7fd8d8da5287aa2a4e38.record.md b/docs/branch-review-records/7a1bb8ea354bbff006a1deb9148167b6bfe7cd5a364f7fd8d8da5287aa2a4e38.record.md new file mode 100644 index 0000000000..3993c5040d --- /dev/null +++ b/docs/branch-review-records/7a1bb8ea354bbff006a1deb9148167b6bfe7cd5a364f7fd8d8da5287aa2a4e38.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/ledger-process-tooling-50uqfc | 07b184df70e31c55e3637af875af077d2f12f129 | PR #1944 review-and-fix (supersedes 2026-08-14) | Corrected connector file encoding; retained five fixes | byte-verified blobs; audit self-test, targeted classifier probes, JSON parse, ledger and docs guards; Vitest unavailable: npm ci package downloads corrupted | From 893f481005a66d6e2304396284e1aec63dbf3ae0 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:52:41 +0800 Subject: [PATCH 12/15] remove stale inbox cancellation --- .../a8783c79-f86e-4fbf-811d-1ec3b1e05082.json | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json diff --git a/docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json b/docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json deleted file mode 100644 index 7e911e6b6f..0000000000 --- a/docs/outstanding-issues-inbox/a8783c79-f86e-4fbf-811d-1ec3b1e05082.json +++ /dev/null @@ -1,10 +0,0 @@ -{ - "version": 1, - "id": "a8783c79-f86e-4fbf-811d-1ec3b1e05082", - "createdOn": "2026-08-14", - "action": "cancel", - "payload": { - "requestId": "0e47904b-f354-4795-a4fc-dcf8b91c1790", - "reason": "Superseded by 83ec71cf on 2026-08-14 (PR #1944), which carries this request's deprioritisation conclusion forward verbatim in substance rather than discarding it, and corrects the count it rests on: the 1,266 figure it repeats was re-measured at 1,445 across 269 files against main at d47aa6d. Cancelling rather than leaving both pending because two update requests on one row force a cancellation decision at reconcile anyway, and the successor is a strict superset — it keeps the do-it-in-scoped-batches judgment, the hot-spot list and the Stop, and adds the staged plan those batches were waiting on." - } -} From b7bf794dcc4a10d1ccbe3f19366e261221af78c1 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:53:02 +0800 Subject: [PATCH 13/15] record PR 1944 CI-blocker fix --- ...68bfe40fd9cdce6209b623eaa682ce4b9524e6eed48ee04a235.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/cc0db6b399e9168bfe40fd9cdce6209b623eaa682ce4b9524e6eed48ee04a235.record.md diff --git a/docs/branch-review-records/cc0db6b399e9168bfe40fd9cdce6209b623eaa682ce4b9524e6eed48ee04a235.record.md b/docs/branch-review-records/cc0db6b399e9168bfe40fd9cdce6209b623eaa682ce4b9524e6eed48ee04a235.record.md new file mode 100644 index 0000000000..05cf676adc --- /dev/null +++ b/docs/branch-review-records/cc0db6b399e9168bfe40fd9cdce6209b623eaa682ce4b9524e6eed48ee04a235.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/ledger-process-tooling-50uqfc | 893f481005a66d6e2304396284e1aec63dbf3ae0 | PR #1944 CI-blocker fix | Removed cancellation targeting an already applied inbox request | docs:check-links, check:outstanding-issues, check:ledger-write-discipline | From 58c4a3d5294ded839f7ff78c4bc18d6ece7522eb Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:57:35 +0800 Subject: [PATCH 14/15] format merge-loss review files --- docs/ledger-id-scheme-proposal.md | 10 +++++----- tests/merge-loss-audit.test.ts | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/ledger-id-scheme-proposal.md b/docs/ledger-id-scheme-proposal.md index 8ea2d36f46..39196d8e4b 100644 --- a/docs/ledger-id-scheme-proposal.md +++ b/docs/ledger-id-scheme-proposal.md @@ -96,12 +96,12 @@ while producing exactly the renumbering churn this row exists to end. So the two forms coexist, and the migration is additive: -| Step | Change | Risk | -| ---- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | +| Step | Change | Risk | +| ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | | 1 | Widen every id validator to accept both `#NNN` and the new stored display id, while allocation still uses the marker. No behaviour change; purely permissive. | Low. Fully reversible. | -| 2 | Add ULID and display-id fields to new rows and switch allocation to them. The `issues:next-id` marker stops being read. | Medium — this is the cutover. | -| 3 | Remove the marker and its `next-id` guards once no writer consults it. | Low, but only after step 2 has been through a few real appends. | -| 4 | Reconsider a union merge driver, which becomes safe only once **no** id is allocated read-modify-write. | Deliberately last. See the Stop below. | +| 2 | Add ULID and display-id fields to new rows and switch allocation to them. The `issues:next-id` marker stops being read. | Medium — this is the cutover. | +| 3 | Remove the marker and its `next-id` guards once no writer consults it. | Low, but only after step 2 has been through a few real appends. | +| 4 | Reconsider a union merge driver, which becomes safe only once **no** id is allocated read-modify-write. | Deliberately last. See the Stop below. | **Every place that currently assumes a sequential id** — all of these need step 1 before anything else moves: diff --git a/tests/merge-loss-audit.test.ts b/tests/merge-loss-audit.test.ts index 21c17cba26..b8320d098f 100644 --- a/tests/merge-loss-audit.test.ts +++ b/tests/merge-loss-audit.test.ts @@ -133,9 +133,9 @@ describe("merge-loss reconciliation exemption", () => { it("credits only a verbatim move to the matching request filename", () => { const entryAt = reader({ [`landing:${request}`]: "100644 blob eee", [`head:${applied}`]: "100644 blob eee" }); expect(isReconciliationMove(request, "landing", "head", entryAt)).toBe(true); - expect( - isReconciliationMove(`${INBOX}/22222222-2222-4222-8222-222222222222.json`, "landing", "head", entryAt), - ).toBe(false); + expect(isReconciliationMove(`${INBOX}/22222222-2222-4222-8222-222222222222.json`, "landing", "head", entryAt)).toBe( + false, + ); expect(isReconciliationMove("src/lib/rag/rag.ts", "landing", "head", entryAt)).toBe(false); expect(isReconciliationMove(applied, "landing", "head", entryAt)).toBe(false); }); From 8a6ede3e3254bec7f45f6852d476f3b0bdabf8f8 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:57:54 +0800 Subject: [PATCH 15/15] record PR 1944 CI format fix --- ...1720a0609b9fdfd96aac4292d00fcdac8e3629b2e8e1f3f4845.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/f726c2e6e71b61720a0609b9fdfd96aac4292d00fcdac8e3629b2e8e1f3f4845.record.md diff --git a/docs/branch-review-records/f726c2e6e71b61720a0609b9fdfd96aac4292d00fcdac8e3629b2e8e1f3f4845.record.md b/docs/branch-review-records/f726c2e6e71b61720a0609b9fdfd96aac4292d00fcdac8e3629b2e8e1f3f4845.record.md new file mode 100644 index 0000000000..9ad8dffd57 --- /dev/null +++ b/docs/branch-review-records/f726c2e6e71b61720a0609b9fdfd96aac4292d00fcdac8e3629b2e8e1f3f4845.record.md @@ -0,0 +1 @@ +| 2026-08-14 | claude/ledger-process-tooling-50uqfc | 58c4a3d5294ded839f7ff78c4bc18d6ece7522eb | PR #1944 CI format fix | Formatted two merge-loss review files | Prettier 3.9.6; audit self-test; JSON parse; docs:check-links |