diff --git a/docs/README.md b/docs/README.md index 34779ed651..98666d3134 100644 --- a/docs/README.md +++ b/docs/README.md @@ -93,6 +93,9 @@ 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`) +- [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/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 | 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 | 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 | 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 | 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 | diff --git a/docs/ledger-id-scheme-proposal.md b/docs/ledger-id-scheme-proposal.md new file mode 100644 index 0000000000..39196d8e4b --- /dev/null +++ b/docs/ledger-id-scheme-proposal.md @@ -0,0 +1,149 @@ +# 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 new file mode 100644 index 0000000000..7910f17f9a --- /dev/null +++ b/docs/no-unchecked-indexed-access-migration-plan.md @@ -0,0 +1,213 @@ +# `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/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/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" + } +} 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. diff --git a/docs/scripts-index.md b/docs/scripts-index.md index 74c080e44a..390cb1bab3 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 d98e1e0328..dae68aafb9 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..053179ca53 --- /dev/null +++ b/scripts/audit-merge-loss.mjs @@ -0,0 +1,342 @@ +#!/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/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); + }); +}); diff --git a/tests/merge-loss-audit.test.ts b/tests/merge-loss-audit.test.ts new file mode 100644 index 0000000000..b8320d098f --- /dev/null +++ b/tests/merge-loss-audit.test.ts @@ -0,0 +1,152 @@ +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); + }); +});