diff --git a/.agents/skills/aicr-cross-review/SKILL.md b/.agents/skills/aicr-cross-review/SKILL.md index 3645d03d6..db10f0b4c 100644 --- a/.agents/skills/aicr-cross-review/SKILL.md +++ b/.agents/skills/aicr-cross-review/SKILL.md @@ -16,7 +16,7 @@ user-invocable: true # automatic path that removing the explicit nested call did not. disallowed-tools: Skill argument-hint: "" -version: 0.3.0 +version: 0.3.19 --- # AICR Cross-Review: Multi-Agent PR Review with Consensus @@ -217,15 +217,38 @@ the consensus mechanics): - **Review** — Claude Code (reviews the pinned diff directly; it deliberately does *not* delegate to the `code-review` command, whose step 8 instructs its agent to - `gh pr comment` the result back to the PR), Codex (background dispatch, 10-min - bounded wait), CodeRabbit (CLI against a detached worktree at `HEAD_SHA`, explicit - 600000 ms timeout — the Bash tool caps at 10 minutes, so a longer timebox cannot be - enforced), and integration analysis (bounded to `changeList`). Every lane is a + `gh pr comment` the result back to the PR), Codex (background dispatch, a 9-min + bounded wait plus one continuation wait when the job is still running — about 18 min + for a live job), CodeRabbit (CLI against a detached worktree at `HEAD_SHA`, explicit + 600000 ms timeout — the Bash tool caps any single call at 10 minutes, which is why + Codex exceeds it by waiting twice rather than waiting longer), and integration + analysis (bounded to `changeList`). Every lane is a `general-purpose` agent. All parallel, schema-validated, and none may execute the reviewed commit's code. -- **Merge** — dedupe by `path:line:normalized-summary`; duplicates merge to the - highest severity; a finding citing a file the reporter never listed in - `filesChecked` is flagged for extra scrutiny. +- **Merge** — dedupe by `path:line:normalized-summary:consumerPath:consumerLine`; + duplicates merge to the highest severity and union their sources; a finding citing a file + the reporter never listed in `filesChecked` is flagged for extra scrutiny. + + **Two lanes wording one defect differently stay separate candidates, by design.** Keying + on location alone was tried and reverted: it did merge those duplicates, but the + evaluation schema permits exactly one verdict per candidate id, so a merged pair of + *distinct* same-line defects has no correct verdict — confirming the real one also + confirms the false one, and refuting the false one dismisses the real one. Retaining both + summaries prevented data loss but not mis-adjudication, which is the worse failure. + + Instead, candidates sharing a location — `path:line` **and** the same + `consumerPath`/`consumerLine` — are **flagged** as possible duplicates. The consumer half + matters: one changed declaration breaking two callers is deliberately two candidates, and + hinting that they might be duplicates would push reviewers to collapse a distinction the + key exists to preserve. The flag + reaches the cross-review candidate list and the refuter prompt, so reviewers decide + whether the two are one defect and evaluate them consistently. Equivalence stays an + explicit judgement rather than an assumption from a shared line number. + + Merging also stops once candidates are presented: a late finding that merged into an + already-evaluated id would inherit votes cast before it existed. Late findings always + become their own candidate and, being unpresented, stay contested for the human. + - **Cross-review (one round, Claude + Codex only)** — each re-reviews independently first (anti-anchoring), then returns AGREE/DISAGREE/OPEN_QUESTION per candidate. CodeRabbit does *not* take part: its CLI is a slow blocking cloud call and it @@ -244,6 +267,17 @@ the consensus mechanics): the `unresolved` array). `consensusReached` is true only when both `contested` and `unresolved` are empty — a finding that reached consensus but failed verification is an open question, not a settled one. + + **Read `adjudication` on every contested entry.** The bucket holds two different states + and they need different things from you. `evaluated` means the finding was presented, + the reviewers voted, and they did not reach 2-of-3 — a genuine split, so break the tie. + `raised-late` means it was raised *during* the cross-review round, after candidates were + presented, so nobody cross-evaluated it and its only position is its reporter's — it just + needs reading. Measured on a real run: 8 of 8 contested findings were `raised-late`, each + with a single AGREE and NONE elsewhere, so the count read as eight disagreements when + there were none. `consensusReached` counts both, deliberately: a late finding is + unadjudicated, and letting it report consensus would be the same overstatement this + skill exists to avoid. - **Report incomplete and stop** — Claude, Codex and integration analysis are required in round 1, and Claude and Codex must each return exactly one evaluation per candidate in the cross-review round. A missing lane, a missing evaluation, a @@ -252,18 +286,220 @@ the consensus mechanics): only best-effort lane: when it does not run, its vote slot records `NONE`, which raises the bar (Claude and Codex must then agree) rather than lowering it. + One deliberate exception, at the level of a **finding** rather than a lane. An + integration finding claims a specific consumer breaks, so one lacking + `consumerPath`/`consumerLine` cannot be verified and never enters consensus — but it is + dropped on its own, with a `log()` naming what went, rather than failing the run. The + earlier all-or-nothing rule was disproportionate: on a real run the lane returned + several findings, one of them a genuine evidenced defect, plus a stale-comment finding + that legitimately has no consumer, and the review reported `incomplete` with all four + lanes `ok` and no report produced. The run still stops when **every** integration + finding is unusable, which is the case that motivated the check — silently dropping the + lane's only finding once yielded `consensusReached: true` while a required lane had + contributed nothing. + + "Unusable" is measured on what survives `intake()`, not on the coordinate check alone. + `intake()` independently drops a finding whose `evidence` is blank, so gating on + coordinates let a coordinate-complete, whitespace-evidence finding pass the filter and + then vanish inside `intake()` — zero candidates, `status: ok`, `consensusReached: true`. + That is the same false-clean, in a narrower form. One rule now covers both drop reasons: + a non-empty integration result that yields no accepted finding stops the run, and the + message says how many went for each reason. + + **Coordinates are validated by a single shared rule**, `hasCoords`, applied to a + finding's own `path`/`line` in `intake()` — every lane, not just integration — and to + `consumerPath`/`consumerLine` for the integration pair. The response schema **requires** + `path` and `line` and leaves `consumerPath`/`consumerLine` optional — deliberately, since + only integration findings carry a consumer — but it constrains none of the four, so + `""`, `" "`, `0` and `-1` all satisfy it and all passed a truthiness/null check. + Tightening the schema instead would fail a whole lane on one bad field, which is the + all-or-nothing behavior this section exists to remove. It is one helper rather than two + call-site conditions for a specific reason: every earlier version of this guard fixed the + pair it was shown and left the other, and a shared rule is what stops the next field pair + from repeating that. + + **The zero-survivor rule applies to every required round-1 batch**, not just integration. + Claude's and Codex's round-1 findings go through `intakeBatch`, which reports what + survived. A required lane whose round-1 findings are *all* malformed contributed + nothing, and letting them vanish silently is the same false-clean one lane over. Mixed + batches still proceed. CodeRabbit is exempt: a total loss there records `NONE` and + raises the bar, exactly as a lane that never ran. Rejection reasons are counted + separately (`unlocatable` vs `unevidenced`) rather than inferred from a subtraction, so + the message names the defect the reader should go looking for. + + **It deliberately does NOT apply to cross-review `newFindings`.** Those go through the + same `intakeBatch` gate — a malformed late finding still never enters the tally — but a + total loss there is not fatal. The rule tests "this lane contributed nothing", which + round 1 can assert because findings are the lane's whole output. In the cross-review + round the lane's output is its *evaluations*, and the completeness gate has already + returned `incomplete` unless the lane evaluated every presented candidate; `newFindings` + are supplementary and volunteered. Aborting there would throw away a full set of + adjudications and the verification round over one imprecise extra finding — the same + disproportionate total loss seen on PR 1908, one round over. Malformed late findings are + dropped individually and each drop is logged with its reason. + **Operational notes:** - The workflow runs in the background — wait for its completion notification. - If it dies mid-run, **resume, don't restart**: `Workflow({scriptPath: ..., resumeFromRunId: ""})` — completed lanes replay from cache. Empty or odd result → read `/journal.jsonl` first. -- A dead Codex broker surfaces as the lane timing out at its bounded wait and - returning `unavailable`. Codex is required, so the run reports `incomplete` and - stops; re-run rather than interpreting a partial result. +- The Codex lane fails in three distinct ways, and the dispatch protocol treats them + differently: + - **Lookup miss** — the status call exits 1 with empty stdout and `No job found` on + stderr. Companion state is keyed by workspace root and each Bash call is a fresh + shell, so an unpinned lookup resolves to a different workspace and reports a live job + as unknown; the miss is **not** evidence the job died. **Always recheck exactly once** + with `--cwd` pinned, whether or not the missing call already carried it — two causes + produce the identical message and only one is settled by adding the flag. The other is + transient: in companion v1.0.2 `saveState` writes `state.json` with a plain + `fs.writeFileSync` (truncate-then-write, no temp-and-rename) while `loadState` wraps + `JSON.parse` in a bare `catch` returning the **default** state, whose `jobs` list is + empty. A read landing inside that write window yields a well-formed `No job found` + rather than an error, and the background worker is rewriting that file precisely while + the status call runs. So an identical repeat need not return an identical answer. Once + a pinned recheck has also missed, return `unavailable` saying the job could not be + located — never re-dispatch (the original may still be running) and never record it as + exhausted budget. + - **Fast transient failure** — retried **once**, and only when all three hold: + `.job.status` is `failed` (never `cancelled`), the job died in **under 60 seconds** + by its own timestamps, and the error names a known-retryable cause such as an upstream + capacity rejection (`Selected model is at capacity`) or a transient dispatch fault. + The threshold is a number rather than a judgment because the observed cases are far + apart — a capacity rejection at ~10s against a genuine timeout at 10m19s — and an + undefined "quickly" is how a one-retry budget erodes. + A retry then costs seconds and these clear on their own. + **`cancelled` is never retried** — the companion emits it only for explicit + cancellation, so a retry would restart work someone deliberately stopped. Nor is a + late failure: `.waitTimedOut` false only means the job became terminal before the + inner deadline, which a failure at 8:59 also satisfies while having burned the whole + window. An unrecognised error is not assumed retryable either. + - **Wait elapsed, job still alive** (`.waitTimedOut` true with `.job.status` still + `queued`/`running`) — not the end of the lane. The job is dispatched in the background + and outlives the Bash call waiting on it, so it needs more **time**, not another + attempt — and the protocol now gives it exactly that: **one continuation wait** on the + *same* job id in a fresh call. That is not a retry; nothing is re-dispatched and no + work is duplicated. A second `.waitTimedOut` is then genuinely exhausted budget, and + the lane returns `unavailable` with the job id so the result can be fetched later. + Measured on a real run: the lane timed out at the full 540000 ms while the job was + demonstrably mid-work, the job was **still** `running` long after the review was + abandoned, and its result stayed retrievable — so reporting exhausted budget there + discarded a required lane, and the whole review with it, over a job that had merely + not finished. + - **Exhausted budget** — a second `.waitTimedOut`, or no parseable JSON at all because + the outer timeout killed the call (a dead broker). No retry, no further wait. + + The ceiling is not tunable — the wait runs inside a Bash call, and that tool silently + kills any foreground command at 600000 ms. Exceeding 10 minutes requires polling across + several calls, which is exactly what the continuation wait above does: a live job gets + two waits, roughly eighteen minutes, without a longer single call. The inner wait is + therefore 540000 ms, + deliberately **below** the outer cap: were the two equal, Bash could kill the command + before it printed its JSON, leaving no `.waitTimedOut` to classify on. That is why an + unclassifiable kill counts as exhausted-budget rather than a fast failure — guessing + wrong there costs another full window for nothing. +- Codex is required, so a lane that is still unavailable after its retry makes the run + report `incomplete`; re-run rather than interpreting a partial result. - CodeRabbit slow runs: check the newest file in `~/.coderabbit/logs/` (429/queue lines mean cloud-side queueing) and confirm `which -a coderabbit` resolves to the brew-managed binary — a stale `~/.local/bin` copy shadows it. +- **A sandboxed CodeRabbit run hangs instead of failing.** `~/.coderabbit` is outside the + sandbox write allowlist, so the CLI cannot create its log or review store; it stalls at + `connecting_to_review_service` until the timebox kills it. The lane therefore runs the + `coderabbit` command with sandbox bypass, and only that command. + + **Why bypass rather than an allowlist entry.** Adding `~/.coderabbit` to the sandbox + write allowlist is the narrower grant and was considered first. It was rejected because + this skill is checked into the repo and has to work on a contributor's machine as + written: an allowlist entry lives in each person's local settings, so anyone who has not + made the same edit gets the silent ten-minute hang above rather than a usable lane. The + bypass is portable and self-documenting at the call site. Its cost is a permission + prompt on step 2 of every round, which is accepted. Adding the allowlist entry locally + is still worthwhile if you run this often — it does not conflict with the bypass — but + the skill must not depend on it. + + **Diagnose it by absence:** a stall at `connecting` *with no new file in + `~/.coderabbit/logs/`* is sandbox denial — a process killed mid-run still flushes a + partial log, so zero bytes means it never created one. A real cloud problem leaves a log + with 429/queue lines. Do not read this stall as an outage or as contention with another + session: an unsandboxed run succeeding while a sandboxed one hangs looks exactly like + contention and is not. +- **Persisted-store fallback.** If a run still fails, the lane checks + `~/.coderabbit/reviews/*/*/reviews/*/git.json`, whichever session produced the record. + Acceptance is `head` plus **the pinned change itself** — never `baseCommitId`. Measured: + the CLI writes `baseCommitId` from the base it resolves locally in that working + directory, not from the `--base-commit` the lane passes. In one real store the same head + carried two different values (a main tip, and a commit not on main at all), while three + other valid records carried the stale local `main` rather than the base Phase 1 fetched + from the canonical repo. Gating on equality discarded good reviews whenever the local ref + lagged — the common case in the fork layout this skill supports. `baseCommitId` is now + reported in `statusNote` and never branched on. + + What identifies the review instead is the **blob OIDs the record already stores**. + Acceptance is layered, cheapest first, and only the last step is identity: + + 1. Same paths, from `git diff --numstat -z` over the **three-dot** range Phase 1 saved. + Two-dot would compare the base tip to the head and pull in everything that landed on + the base after branching — on a real pair, 13 files against the valid record's 59. + `-z` matters independently: a plain `split()` shreds a path containing a space into + separate members and would reject every record for such a PR. + 2. Same per-file line counts, against the record's `linesAdded` / `linesRemoved`. + 3. The committed-only `lanes` shape below. + 4. The **file modes and blob OIDs** the record's own patch states, equal to what + `git diff --raw --no-abbrev` reports for the pinned range. + + Steps 1–2 are a prefilter, never a result: two different patches collide on a numstat + trivially — a `beta`→`BETA` edit and a `gamma`→`GAMMA` edit in the same file both report + one added and one removed, and a review of this head against another base can leave + exactly such a record. + + Step 4 compares OIDs rather than the stored patch text, which was the obvious move and + is wrong. Bare `git diff` output moves with the reader's gitconfig — measured, the text + differs under `diff.noprefix` and `diff.context`, and fails outright under + `diff.external` — so a contributor with ordinary settings gets a permanent mismatch and + this fallback silently never fires. That is the same works-on-my-machine class the + sandbox decision rejected an allowlist for. Pinning flags (`--no-ext-diff`, `--no-color`, + `-U3`, `--src-prefix`) fixes *our* side only; if the CLI produced the record under that + same config, hardening makes the mismatch worse. Blob OIDs are content hashes: stable on + both sides, and exact content identity rather than a rendering of it. Both git calls + still pass `--no-ext-diff --no-color`, since the `--raw` call needs them. + + **Modes are checked before OIDs, because OIDs alone are not identity.** With a `chmod +x` + in one commit and an edit in the next, a stored review of the *edit alone* carries the + same path, counts, `lanes` and **both** blob OIDs as the pinned range covering both + changes — verified. Only the modes differ: pinned `100644 → 100755` against the record's + `100755 → 100755`. Git states modes in one of four shapes (`old mode`/`new mode`, + `new file mode`, `deleted file mode`, or the trailing mode on the `index` line), and the + trailing mode is absent exactly when the `old`/`new` pair is present, so the cases do not + overlap. A record that states no mode at all cannot prove identity and is skipped. + + An entry with **no** `index` line is not automatically rejected. Git omits that line + exactly when the blob is unchanged — for a `chmod +x` the mode lines carry the + information instead, and the mode check above has already matched them. Such an entry is + accepted when the pinned diff agrees the blob is unchanged (`src == dst`), and rejected + as `no-index-line` otherwise. Rejecting on sight would have disabled this fallback for + any PR that merely marks a script executable, which this repo does routinely. A + rename or copy in the pinned range is rejected outright as `rename-unverifiable`, by an + explicit status check rather than incidentally: the record is keyed on the post-rename + path and carries no source path, so an edit-only review of that path is indistinguishable + from one that covered the rename — measured, a pinned `R077 old.txt -> new.txt` and a + stored `M new.txt` share modes, blobs and line counts. The scope prefilter usually rejects + such a record first, since the CLI scopes + its stored patch and counts to the post-rename path, so the record's numstat differs from + the pinned range's and the scope prefilter rejects it first. + + Every entry must additionally prove it was **committed-only** — `lanes` is a boolean map + (`{"committed": true, "uncommitted": false}`), so the check tests the values and requires + that exact shape. Merely rejecting a truthy `uncommitted` was not enough: a record from + an older CLI that omits `lanes`, or carries a non-dict, would pass as clean and + contribute comments on code outside the pinned head. Absent proof of clean, skip. + + The pinned diff itself is computed **fail-closed**: an unreachable base exits non-zero + with empty output, and treating that as an empty result would reject every record and + then report a clean "nothing matched" — so the scan aborts with an `ERROR` line instead. + Several records can also pass every check at once (a re-run after a transient failure + produces a twin), so candidates are ordered by mtime and exactly one `MATCH` is emitted: + newest wins, since a re-run supersedes what it replaced. ## Phase 3: CI status for the pinned commit @@ -388,13 +624,43 @@ a shell variable, so substitute the literal path here. - Severity scale: critical (must fix) > major (should fix) > medium > minor. - Keep the report concise — actionable findings, not noise. - Never set `dangerouslyDisableSandbox` for reviewer or companion commands; they run - fine sandboxed. One exception, kept in sync with the Codex dispatch protocol in - `scripts/workflow.mjs`: the companion writes its job log under - `~/.claude/plugins/data`, which is sandbox-denied — if dispatch fails on that write, - bypass for that call only, never for anything touching the working copy or GitHub. -- **Clean up before finishing:** `rm -f ` and delete the two scoped refs - captured in Phase 1 (`git -C "" update-ref -d "$PRREF"`, same for - `"$BASEREF"` — use the exact names echoed there, not a guess). Confirm - no `$TMPDIR/cr-rabbit.*` worktree path remains in - `git worktree list` — the CodeRabbit lane cleans its own via trap, but verify. Do not - compare total worktree counts; concurrent sessions change the total legitimately. + fine sandboxed. **Exactly two exceptions**, both kept in sync with the protocols in + `scripts/workflow.mjs`, and both scoped to a single command that performs no Git + operation, no working-copy mutation, and no GitHub write. (They do *read* files — + CodeRabbit necessarily reads the detached worktree it was pointed at. The rule bars + bypassing calls that **act on** the working copy, not calls that read a path): + - **Codex companion** — it writes its job log under `~/.claude/plugins/data`, which is + sandbox-denied. If dispatch fails on that write, bypass for that call only. + - **CodeRabbit review** — `~/.coderabbit` is outside the write allowlist, so a + sandboxed CLI cannot create its log or review store and hangs at + `connecting_to_review_service` until the timebox kills it. Bypass **step 2 only** of + the three-step CodeRabbit protocol, which is a lone `coderabbit review` command; + worktree setup and cleanup live in steps 1 and 3 and stay sandboxed. + + Anything else stays sandboxed. In particular, never bypass a call that also performs + `git` operations — that is why the CodeRabbit protocol is split into three calls rather + than the single invocation it used to be. +- **The tool shell is zsh, and every prompt that hands out a shell command says so.** + `scripts/workflow.mjs` defines a single `SHELL_CONTRACT` constant, interpolated in + **exactly one place** — `NO_EXECUTION`, which every prompt builder composes exactly + once. So all six assembled prompts carry it exactly once. Do not add a second + interpolation: `NO_EXECUTION` already embeds `PINNED_READS`, so putting it in + `PINNED_READS` or `CODEX_LEAN` as well silently doubles it in every lane. That is how + the first attempt got it wrong, and no block-level check can see it — the duplication + only appears once the prompts are assembled. It covers the + three differences that bite silently: `shopt` and other bash builtins are simply absent, + an unmatched glob is a hard error that aborts the command list rather than passing + through literally, and `status` is readonly. Keep it in one place — the two blocks that + once held hand-written copies each acquired theirs only after a zsh bug had already + shipped in them, and a new lane must not be able to omit it by accident. +- **Clean up before finishing:** `rm -f ` and delete the + two scoped refs captured in Phase 1 (`git -C "" update-ref -d "$PRREF"`, + same for `"$BASEREF"` — use the exact names echoed there, not a guess). Confirm no + `${TMPDIR:-/tmp}/cr-rabbit.*` worktree path remains in `git worktree list` — write the + fallback out, since setup creates the worktree under `${TMPDIR:-/tmp}` and with `TMPDIR` + unset a bare `$TMPDIR/cr-rabbit.*` names `/cr-rabbit.*` while the leak sits in `/tmp` — + the CodeRabbit + lane cleans its own in step 3, but verify, since a lane killed between steps 2 and 3 + leaves one behind. Step 1 of the next run reaps `cr-rabbit.*` directories older than + 120 minutes, which bounds the leak but does not clear it now. Do not compare total + worktree counts; concurrent sessions change the total legitimately. diff --git a/.agents/skills/aicr-cross-review/scripts/workflow.mjs b/.agents/skills/aicr-cross-review/scripts/workflow.mjs index 79f17273d..0cbb56422 100644 --- a/.agents/skills/aicr-cross-review/scripts/workflow.mjs +++ b/.agents/skills/aicr-cross-review/scripts/workflow.mjs @@ -29,7 +29,20 @@ export const meta = { // ---------- args ---------- // Tolerate args arriving as a JSON string (observed 2026-07-21: the harness // delivered `args` stringified even when the tool call passed a real object). -const parsedArgs = typeof args === 'string' ? JSON.parse(args) : (args || {}) +// The parse is wrapped because its bare failure is undiagnosable, unlike the +// missing-arg loop below: it throws "JSON Parse error: Unterminated string" naming +// neither this skill nor `args`, no agent ever spawns, and the run dies before Phase 1. +// Observed while passing args as a JSON-encoded string, which the Workflow tool +// explicitly does not want — nothing in the message pointed at that. +let parsedArgs +try { + parsedArgs = typeof args === 'string' ? JSON.parse(args) : (args || {}) +} catch (err) { + throw new Error( + `aicr-cross-review: could not parse the "args" input as JSON (${err.message}). ` + + 'Pass args as a real object in the Workflow tool call, not a JSON-encoded string — ' + + 'a stringified object with embedded quotes is the usual cause.') +} const { pr, repo, repoPath, headSha, baseSha, diffPath, prType, changeList, repoNotes } = parsedArgs for (const [k, v] of Object.entries({ pr, repo, repoPath, headSha, baseSha, diffPath, prType })) { if (!v) throw new Error(`missing required arg: ${k}`) @@ -122,6 +135,13 @@ Repo working copy: ${repoPath}.` // they go inside SINGLE quotes. Double quotes stop globbing but still expand $(...) // and backticks, so a filename like docs/$(id).md would execute — the exact thing this // skill promises not to run. Single quotes are the only literal form in sh/bash/zsh. +// Interpolated in exactly ONE place: NO_EXECUTION, which every prompt builder composes +// exactly once, so every assembled prompt carries this once. Do not also add it to +// PINNED_READS or CODEX_LEAN — NO_EXECUTION already embeds PINNED_READS, so a second +// interpolation silently doubles it in every lane (that is how the first attempt got +// it wrong, and a block-level check cannot see it). +const SHELL_CONTRACT = `Shell contract: the tool shell is zsh, not bash. Never name a shell variable "status" (readonly in zsh; the assignment silently fails). Do not reach for bash builtins such as "shopt" — they are simply not found. An unmatched glob is a hard error that aborts the whole command list, not a literal passthrough as in bash, so prefer doing the work in python3 whenever a pattern might match nothing.` + const PINNED_READS = ` read: git -C "${repoPath}" show '${headSha}:' search: git -C "${repoPath}" grep -n -e '' ${headSha} -- '' SINGLE quotes are required and must not be changed to double quotes: they are what @@ -144,7 +164,8 @@ Execution rules: - ALLOWED: the trusted commands this prompt explicitly prescribes anywhere (including any detached worktree or CLI invocation it spells out), read-only "gh" queries, and the saved diff plus pinned reads/searches: -${PINNED_READS}` +${PINNED_READS} +${SHELL_CONTRACT}` const OUTPUT_RULES = ` Reporting rules: @@ -167,14 +188,34 @@ const CODEX_DISPATCH = ` Codex dispatch protocol (mandatory): 1. comp=$(ls -t ~/.claude/plugins/cache/openai-codex/codex/*/scripts/codex-companion.mjs | head -1) 2. Dispatch the review as a BACKGROUND job (pass --background to codex-companion task) so it returns a job id immediately. NEVER run foreground: a foreground call hangs forever if the broker dies mid-turn. + Pass --cwd "${repoPath}" on this call and on EVERY later status/result call. Companion state is keyed by the workspace root derived from the working directory, and each Bash call is a fresh shell that may start somewhere else — an unpinned lookup silently resolves to a different workspace and reports the job as unknown. 3. Wait for it with the companion's own bounded wait — do NOT hand-roll a poll loop: - node "$comp" status --wait --timeout-ms 600000 --poll-interval-ms 30000 --json - Read the returned JSON yourself. The job state is at .job.status (NOT a top-level "status" field), and .waitTimedOut is true if the 10-minute cap expired while the job was still queued/running. -4. .job.status == "completed" → fetch the payload with: node "$comp" result --json (NOT status, which returns only a job summary). - Anything else (failed, cancelled, or .waitTimedOut) → return status:"unavailable" with the observed .job.status in statusNote. Do NOT retry and do NOT substitute your own review — an unavailable Codex lane is the expected outcome here. -Every Codex task you dispatch must carry the execution rules verbatim in its own prompt — Codex runs in its own shell and does not inherit this one's constraints. -Shell note: never name a shell variable "status" (readonly in zsh; the assignment silently fails). -Sandbox: the review itself runs sandboxed. The one known exception is the companion's own job log under ~/.claude/plugins/data, which is sandbox-denied — if dispatch fails on that write, bypass for that call only. Never bypass for anything else, and never for a call that touches the working copy or GitHub.` + node "$comp" status --cwd "${repoPath}" --wait --timeout-ms 540000 --poll-interval-ms 30000 --json + Run that Bash call with a timeout of 600000 ms. The inner wait is deliberately 60s SHORTER than the outer cap: the two must not be equal, or Bash can kill the command before it prints its JSON, leaving you with no .waitTimedOut to classify on — and an unclassifiable kill is the one case step 5 must never guess at, since retrying a genuine timeout doubles the wall clock for nothing. + Read the returned JSON yourself. The job state is at .job.status (NOT a top-level "status" field), and .waitTimedOut is true if the inner deadline expired while the job was still queued/running. + Absent JSON has TWO causes and they are not interchangeable — distinguish them before classifying: + - Exit status 1 with EMPTY stdout and "No job found" on stderr (measured on companion v1.0.2; the message goes to stderr even with --json, so capture both streams before deciding): this is a LOOKUP MISS. It is NOT evidence the job died — it may still be running. Never classify a lookup miss as exhausted budget; doing so abandons a live required lane. Note the exit status is a reliable discriminator here, so read it directly rather than inferring from output — and do not read it through a pipe, where $? is the last stage's status, not the companion's. + ALWAYS recheck EXACTLY ONCE with --cwd "${repoPath}" pinned, whether or not the call that missed already carried it. Two independent causes produce the identical message, and only one of them is settled by adding the flag: + - An unpinned lookup resolves to a different workspace and reports a live job as unknown. Pinning fixes it. + - A pinned lookup can miss TRANSIENTLY. In companion v1.0.2, saveState writes state.json with a plain fs.writeFileSync (truncate-then-write, no temp-file-and-rename), and loadState wraps JSON.parse in a bare catch that returns the DEFAULT state — whose jobs list is empty. A read landing inside that write window therefore yields a well-formed "No job found" rather than an error, and the background worker is updating that file precisely while the status call runs. So an identical repeat is NOT guaranteed to return an identical answer; a brief pause before the recheck makes it likelier to clear. + Once a pinned recheck has also missed, return status:"unavailable" saying the job could not be located, and stop. Do NOT re-dispatch the task: the original may still be running, and a second dispatch doubles the work while the first result becomes unreachable. Do NOT call it exhausted budget either; the distinction belongs in statusNote. + - No output at all because your Bash call hit its 600000 ms timeout: that IS exhausted budget. Do not retry; say so in statusNote. +4. .job.status == "completed" → fetch the payload with: node "$comp" result --cwd "${repoPath}" --json (NOT status, which returns only a job summary). +5. Otherwise classify, and retry ONLY a failure that is demonstrably both fast and transient. All three conditions must hold: + a. .job.status is "failed" — NOT "cancelled". The companion emits "cancelled" only for explicit cancellation, so retrying it restarts work something or someone deliberately stopped. + b. .waitTimedOut is false AND the job died in UNDER 60 SECONDS — compute elapsed from the job's own timestamps, not from the absence of a timeout. Use the number, not a judgment call: the two observed cases sit far apart (an upstream capacity rejection landed at ~10s, a genuine timeout at 10m19s), and leaving "quickly" undefined is how a one-retry budget turns into retry-whenever-it-feels-right. A job that fails at 8:59 also satisfies .waitTimedOut == false while having consumed nearly the whole window; that is not a transient blip and retrying it costs another full window. + c. The error text names a known-retryable cause: an upstream capacity rejection ("Selected model is at capacity"), a broker startup error, or a transient dispatch fault. An unrecognised error is not assumed retryable. + All three true → dispatch ONE new background job per step 2 and wait once more per step 3. If the second attempt also fails, return status:"unavailable" with BOTH observed .job.status values and the broker error text in statusNote. + Anything else — "cancelled", a late failure, or an unrecognised error — is a deliberate stop or an unexplained failure. Do NOT retry. Return status:"unavailable" with .job.status, .waitTimedOut, elapsed time, and whether the job log showed active progress. + Retry budget is exactly one, and only when a, b and c all hold. Never loop. +6. .waitTimedOut true is DIFFERENT from all of the above, and is NOT the end of the lane. It means only that the inner wait elapsed — it says nothing about the job, which is dispatched in the background and outlives the Bash call that was waiting on it. Re-read .job.status: + - Still "queued" or "running" → the job is ALIVE and needs more TIME, not another attempt. Wait once more on the SAME job id, in a NEW Bash call, exactly as in step 3 (same 540000 ms inner wait, same 600000 ms outer timeout). This is a CONTINUATION, not a retry: no second job is dispatched, no work is duplicated, and the first job's result stays the one you collect. Then classify the second wait's outcome by these same rules — except that this continuation is granted ONCE, so a second .waitTimedOut IS exhausted budget: return status:"unavailable" saying the job was still running after both waits, and give the job id so the result can be fetched later with: node "$comp" result --cwd "${repoPath}" --json + - Any terminal state ("completed", "failed", "cancelled") → it finished while you were between calls. Handle it under step 4 or step 5; do not treat the earlier timeout as the verdict. + Why this exists: measured on a real run, the lane hit .waitTimedOut at the full 540000 ms while the job was demonstrably mid-work (its log showed pinned git greps completing, exit 0), the job was STILL "running" long after the review was abandoned, and its result remained retrievable. Reporting "exhausted budget" there discarded a required lane, and with it the whole review, over a job that simply had not finished yet. Re-dispatching would have been wrong — waiting again was not. + Never substitute your own review for an unavailable Codex lane in either case — an unavailable lane is an expected outcome the workflow handles. + The 600000 ms outer ceiling is not tunable: the wait runs inside a Bash call, and the Bash tool silently kills any foreground command at that point. The inner wait is 540000 ms precisely so it finishes and prints its JSON before that happens — do not raise it to match. Step 6 is how a job legitimately exceeds ten minutes: not a longer wait, but a second one across a fresh call, which is the "polling across several calls" the ceiling leaves open. Total budget for a live job is therefore two waits, about eighteen minutes. +Every Codex task you dispatch must carry the execution rules verbatim in its own prompt — Codex runs in its own shell and does not inherit this one's constraints. That includes the shell contract stated elsewhere in this prompt: it arrives via NO_EXECUTION, which every prompt builder composes exactly once, so it is deliberately not repeated here. +Sandbox: the review itself runs sandboxed. The one known exception is the companion's own job log under ~/.claude/plugins/data, which is sandbox-denied — if dispatch fails on that write, bypass for that call only. Never bypass for anything else, and never for a call that performs a Git operation, mutates the working copy, or writes to GitHub. Reading a path is not the boundary; acting on it is.` const REVIEW_FOCUS = { 'code-change': `Review for bugs, regressions, broken consumers, security issues, and instruction/config compliance. @@ -244,15 +285,285 @@ Translate Codex's raw output into the structured result yourself.` // The CodeRabbit CLI runs exactly once, in round 1; the cross-review round replays // its saved findings instead of paying for another slow cloud call. -const CODERABBIT_RUN = `Run this WHOLE sequence in a single Bash invocation so the EXIT trap covers agent death or timeout: +const CODERABBIT_RUN = `Run this as THREE separate Bash calls. Only the middle one is sandbox-bypassed, so git never runs unsandboxed against the working copy. + +STEP 1 — setup (SANDBOXED, normal Bash call): set -euo pipefail + { find "\${TMPDIR:-/tmp}" -maxdepth 1 -type d -name 'cr-rabbit.*' -mmin +120 -print0 | + while IFS= read -r -d '' STALE; do + git -C "${repoPath}" worktree remove --force "$STALE/head" 2>/dev/null || true + rm -rf "$STALE" 2>/dev/null || true + done + git -C "${repoPath}" worktree prune; } || true CRROOT=$(mktemp -d "\${TMPDIR:-/tmp}/cr-rabbit.XXXXXX") - trap 'git -C "${repoPath}" worktree remove --force "$CRROOT/head" 2>/dev/null || true; rm -rf "$CRROOT"' EXIT git -C "${repoPath}" worktree add --detach "$CRROOT/head" ${headSha} - # Blocks until the cloud review completes; do NOT use "-t uncommitted". - coderabbit review --agent --base-commit ${baseSha} --dir "$CRROOT/head" -Accept the run ONLY if its reported baseCommit equals ${baseSha}; otherwise discard it and return status:"unavailable" with what it reported. Ignore currentBranch and baseBranch — a detached worktree correctly reports currentBranch:"HEAD" and the CLI's inferred baseBranch stays unrelated even when --base-commit is honored. -Timebox: run this Bash call with an explicit timeout of 600000 ms. The Bash tool defaults to 2 minutes and is capped at 10, so an unset or larger timeout silently kills the run. If it does not finish inside that window, check the newest file in ~/.coderabbit/logs/ for 429/queue lines and return status:"unavailable" with what you saw in statusNote — CodeRabbit is best-effort and the review continues without it.` + echo "CRROOT=$CRROOT" +Record the echoed CRROOT: shell variables do not survive between Bash calls, so steps 2 and 3 must use the literal path. +The find/prune pair is the bounded reaper for STEP 3's known leak. Two limits make the FIND half safe: it matches ONLY the cr-rabbit.* prefix this lane creates, and only entries older than 120 minutes — twelve times the 600000 ms (10 minute) cap a single review can occupy, so it can never reach another session's live worktree. Do not widen either limit. "find" is used rather than a glob because an unmatched glob aborts the command list under zsh; find prints nothing and exits 0. The whole reaper — find, the removal loop AND the prune — is wrapped so it cannot fail the call. Under set -e any of them aborts STEP 1 before the mktemp and the worktree add, and both halves have been seen to fail: an unremovable stale directory where TMPDIR is unset and /tmp is shared between users, and "git worktree prune" itself exiting "Operation not permitted" on .git/worktrees when the sandbox profile, computed at session start, does not cover a worktree created later in that session. Neither is a reason not to review. Only the reaper is neutralised; the worktree add below stays fatal, since STEP 2 must not review the wrong tree. +The PRUNE half is repo-wide, not prefix-scoped — be precise about that rather than reading the two limits onto it. It is included to collect the admin entries whose directories the loop just removed, and it is near-harmless because prune only reaps entries whose directory is ALREADY gone. The residual case is an unrelated worktree whose directory is transiently absent (unmounted volume, an rm in flight): its entry would be pruned too. That is accepted; re-adding such a worktree is cheap, and the alternative is leaving this lane's own stale entries to accumulate. +One more rough edge, cosmetic: a cr-rabbit.* directory left by a DIFFERENT clone is removed by the rm -rf while its admin entry lives in that other repo, which "git -C ${repoPath} worktree remove" cannot reach. That entry is then collected by the other repo's own next prune, so it self-heals — but the removal is less clean than the within-repo case. + +STEP 2 — the review (THIS CALL ONLY runs with dangerouslyDisableSandbox): + coderabbit review --agent --committed --base-commit ${baseSha} --dir "/head" +Nothing else belongs in this call: no Git operation, no working-copy mutation, no GitHub write. Reading is not the boundary — this very command reads the detached worktree it is pointed at. What the bypass must never cover is a call that ACTS on the working copy or GitHub. Note its exit status rather than assuming success — a non-zero exit must still reach RECOVERY, which is the whole point of the fallback. Do NOT use "-t uncommitted". +Timebox: give THIS call an explicit timeout of 600000 ms. The Bash tool defaults to 2 minutes and is capped at 10, so an unset or larger timeout silently kills the run. Steps 1 and 3 are fast and need no explicit timeout. +Accept the run ONLY if its reported baseCommit equals ${baseSha}; otherwise discard it and return status:"unavailable" with what it reported. Ignore currentBranch, baseBranch, and workingDirectory — a detached worktree correctly reports currentBranch:"HEAD", the CLI's inferred baseBranch stays unrelated even when --base-commit is honored, and the commit pair is what identifies the reviewed context. + +STEP 3 — cleanup (SANDBOXED, normal Bash call). Run it ALWAYS: after success, after failure, and after a timebox kill. + git -C "${repoPath}" worktree remove --force "/head" 2>/dev/null || true + rm -rf "" +STEP 3 carries no "set -euo pipefail": every command in it is already \`|| true\`-guarded or idempotent, and failing the call on a cleanup that partially succeeded would abort the rest of the cleanup. STEP 1 does set it because a failed worktree add there must stop the lane before STEP 2 reviews the wrong tree. STEP 2 is a single command, so the flags would change nothing. +There is no EXIT trap any more — a single invocation could carry one, three cannot. So STEP 3 is the only PROMPT cleanup there is: if you die between steps 2 and 3, the worktree leaks and STEP 3 will not run. Do not skip STEP 3 on the assumption that something else will. +That is the accepted trade, and STEP 1's age-gated reaper is its backstop, not its excuse: it collects such a leak only on the NEXT run of this lane and only once the leak is two hours old, so between those points the worktree is really there. Phase 1's hygiene check COUNTS worktrees and stops to ask you — it deliberately does not remove any, since a clean detached-HEAD worktree may be another session's live review — and a bare \`git worktree prune\` walks past a fresh leak entirely, because prune only reaps admin entries whose directory is already gone. Leaking a worktree is still far better than running git unsandboxed, which has no backstop at all — but it is a real cost, not a free one. + +SANDBOX — this is the single most common reason this lane fails, and it is NOT a CodeRabbit problem. ~/.coderabbit is outside the sandbox write allowlist, so a sandboxed CLI cannot create its log or its review store. It does not fail fast: it hangs at + {"type":"status","phase":"connecting","status":"connecting_to_review_service"} +until the timebox kills it, and writes NO log file at all. +Diagnose it by absence: if the run stalls at "connecting" and ~/.coderabbit/logs/ gained NO new file, it was sandbox-denied. A process killed mid-run still flushes a partial log — zero bytes means it never created one. A genuine cloud problem leaves a log containing 429 or queue lines. +Therefore run STEP 2 — and only STEP 2 — with sandbox bypass (dangerouslyDisableSandbox) from the start; the observed evidence above is the justification the sandbox rules require. That step contains a single coderabbit command by design: steps 1 and 3 hold every git and working-copy operation and stay sandboxed. Never widen the bypass to cover them. +Do NOT read a "connecting" stall as an outage, a 429, or contention with another session — a concurrent run on the same account was ruled out as the cause (an unsandboxed run succeeded while a sandboxed one hung, which merely looks like contention). + +RECOVERY — always try this before returning unavailable, and also whenever your own run stalls, exits non-zero, or is killed at the timebox. Run it as a SEPARATE Bash call, never appended to the steps above: STEP 2 may already have been killed at its timebox, so anything chained after it would never execute. The CLI persists every completed review to ~/.coderabbit/reviews///reviews//, where git.json carries "head", "baseCommitId" and a per-file "diff" list holding each file's full patch text, including its "index .." line. Search for a record whose head is ${headSha} and whose blob OIDs equal the pinned diff exactly: +Do the matching in Python, NOT with a shell glob. This is the shell contract stated elsewhere in this prompt, applied to the case that actually bit: under zsh a shell glob here aborts the whole command list on no-match, stranding the log fallback below in exactly the situation it exists for. + python3 - <<'PY' +import glob, json, os, re, subprocess, sys +head = "${headSha}" +# [.][.] rather than a backslash-escaped dot: an unrecognised escape is dropped when this +# block is assembled into the JS template literal, so the pattern would silently become +# "index (hex)..(hex)" with two wildcards. Same class of hazard as the chr() note below. +INDEX = re.compile('^index ([0-9a-f]+)[.][.]([0-9a-f]+)(?: ([0-7]+))?', re.M) +# Modes are part of the identity, not decoration. A chmod +x in one commit followed by an +# edit in the next produces a stored review of the EDIT ALONE whose path, line counts, +# lanes and both blob OIDs are identical to the pinned range covering BOTH changes — +# verified — so OIDs without modes accept a record that never saw the mode change. Git +# states the modes in one of four shapes, and the trailing mode on the index line is +# ABSENT exactly when old/new mode lines are present, so the cases do not overlap. +OLDMODE = re.compile('^old mode ([0-7]+)', re.M) +NEWMODE = re.compile('^new mode ([0-7]+)', re.M) +NEWFILE = re.compile('^new file mode ([0-7]+)', re.M) +DELFILE = re.compile('^deleted file mode ([0-7]+)', re.M) +NOMODE = '000000' # what --raw reports for the absent side of an add or a delete +def storedmodes(text, mo): + om, nm = OLDMODE.search(text), NEWMODE.search(text) + if om and nm: + return (om.group(1), nm.group(1)) + nf = NEWFILE.search(text) + if nf: + return (NOMODE, nf.group(1)) + df = DELFILE.search(text) + if df: + return (df.group(1), NOMODE) + if mo is not None and mo.group(3): + return (mo.group(3), mo.group(3)) # unchanged mode, stated once on the index line + return None # the record cannot prove its modes: skip it +# chr() rather than backslash escapes, for EVERY control character used below: this block +# is assembled inside a JS template literal, which consumes a backslash escape before the +# prompt is built — the agent would receive a raw control character where Python needs the +# two-character sequence. A literal newline landing mid-string-literal is a hard +# SyntaxError, so this is not a style preference; verify by extracting the block from the +# ASSEMBLED prompt and running it, never by reading the source here. +NUL, TAB, LF = chr(0), chr(9), chr(10) +# Identity is the PINNED DIFF ITSELF, never the record's baseCommitId. MEASURED: the CLI +# writes baseCommitId from the base it resolves locally in that working directory, NOT +# from the --base-commit we pass. In one real store the same head carried two different +# values — one a main tip, one a commit not on main at all — while three other valid +# records carried the stale local "main" rather than the base Phase 1 fetched from the +# canonical repo. Gating on equality therefore discards good reviews whenever the local +# ref lags that base, which in the fork layout this skill supports is the common case. +# Keep baseCommitId for statusNote; do not branch on it. +# --numstat -z earns its place three times over: -z keeps a path containing spaces +# intact (a plain split() shreds "with space.txt" into two members and rejects every +# record for such a PR), the per-file counts reproduce the record's linesAdded and +# linesRemoved exactly (verified against a real record), and comparing counts as well as +# paths catches a same-scope review of different content that a path set alone would let +# through. THREE dots, matching the diff Phase 1 saved: two-dot compares the base tip to +# the head and so includes everything that landed on the base after branching — on a real +# pair that was 13 files against the valid record's 59. +RANGE = '${baseSha}...${headSha}' +# --no-ext-diff --no-color on BOTH git calls. diff.external replaces the diff engine and +# makes a patch-producing call fail outright; color.diff=always injects ANSI. Measured: +# --numstat itself is immune to both, but the flags cost nothing and keep the two calls +# reading identically — the --raw call below genuinely needs them. +GITDIFF = ['git', '-C', '${repoPath}', 'diff', '--no-ext-diff', '--no-color'] +def rungit(extra): + o = subprocess.run(GITDIFF + extra + [RANGE], capture_output=True, text=True) + # FAIL CLOSED. An unreachable base (shallow clone, pruned ref, wrong repo path) exits + # non-zero with empty stdout; taking that as an empty result would SKIP every record + # and then report "no stored review matches", laundering a broken-git condition into a + # clean negative. A genuine PR diff is never empty either, so treat both as an error. + # Doing this ONCE up front also means a git failure can never later be mislabelled as + # a content mismatch, which would send the reader hunting a difference that is not there. + if o.returncode != 0 or not o.stdout.strip(): + print('ERROR git-diff-failed rc=%d args=%s stderr=%s' + % (o.returncode, ' '.join(extra), o.stderr.strip()[:200])) + sys.exit(2) + return o.stdout +def numstat(out): + toks = out.split(NUL) + if toks and toks[-1] == '': + toks.pop() + m, i = {}, 0 + while i < len(toks): + # maxsplit=2, not a bare split: under -z a path is emitted raw and unquoted, so a + # filename containing a TAB would yield four fields and raise ValueError, exiting + # 1 with a traceback and no ERROR line — a shape the reader below has no rule for. + # Capping the split keeps such a path whole in the third field instead. + added, removed, path = toks[i].split(TAB, 2) + i += 1 + if path == '': # rename/copy: old and new paths follow as their own tokens + path = toks[i + 1] + i += 2 # record stores the post-rename path, so keep the new one + m[path] = (added, removed) + return m +want = numstat(rungit(['--numstat', '-z'])) +def rawmap(out): + # ": " NUL "" NUL, with a second + # path token for a rename or copy. Blob OIDs are CONTENT HASHES, which is the whole + # point: unlike patch text they do not move with core.abbrev, diff.context, + # diff.noprefix, diff.algorithm, color.diff or diff.external. + toks = out.split(NUL) + if toks and toks[-1] == '': + toks.pop() + m, i = {}, 0 + while i < len(toks): + meta = toks[i].split(' ') + i += 1 + # meta[0] carries a leading ':' — strip it, the modes are compared literally. + srcmode, dstmode = meta[0][1:], meta[1] + src, dst, status = meta[2], meta[3], meta[4] + path = toks[i] + i += 1 + if status[:1] in ('R', 'C'): + path = toks[i] + i += 1 # record stores the post-rename path, so keep the new one + # KEEP the status. Dropping it collapsed a pinned rename-plus-edit onto an + # edit-only record: R070 old.txt->new.txt with blobs A->B and an M new.txt with + # blobs A->B are byte-identical once the status and the source path are discarded, + # so a stored review that never saw the rename could be accepted for a pinned range + # that contains one. Reachable exactly where this fallback is meant to help — the + # same head reviewed against a different base. + m[path] = (srcmode, dstmode, src, dst, status) + return m +wantoid = rawmap(rungit(['--raw', '--no-abbrev', '-z'])) +candidates = [] +for p in glob.glob(os.path.expanduser('~/.coderabbit/reviews/*/*/reviews/*/git.json')): + try: + d = json.load(open(p)) + except Exception: + continue # half-written or malformed record: skip, never abort the scan + if d.get('head') != head: + continue + entries = d.get('diff') or [] + # Establish filePath up front so BOTH loops below can rely on it. Guarding only the + # comprehension and not the identity loop would leave a None to reach an argv list — + # a TypeError traceback with no ERROR line, the same uncovered shape the capped TAB + # split exists to remove. + if not entries or not all(e.get('filePath') for e in entries): + print('SKIP malformed-entries %s (an entry carries no filePath)' % p) + continue + got = {e['filePath']: (str(e.get('linesAdded')), str(e.get('linesRemoved'))) + for e in entries} + # Paths and counts are a cheap PREFILTER, not identity. Two different patches collide + # on a numstat trivially — verified: a beta->BETA edit and a gamma->GAMMA edit in the + # same file both report "1 added, 1 removed". A review of this head against another + # base can leave exactly such a record, so counts alone would admit it. + if got != want: + print('SKIP scope-mismatch %s (%d files, want %d)' % (p, len(got), len(want))) + continue + # Matching scope is still not the same review: the CLI reviews tracked edits by + # default, so a dirty worktree can leave the path set identical while the patch + # differs. Each entry carries "lanes", a BOOLEAN MAP — {"committed": true, + # "uncommitted": false} — not a list, so test the VALUES: key membership is always + # true and would accept everything. Require the committed-only shape EXPLICITLY + # rather than merely rejecting a truthy "uncommitted": a record from an older CLI + # that omits lanes, or one carrying a non-dict, would otherwise pass as clean and + # contribute comments on code outside the pinned head. Absent proof of clean, skip. + if not all(isinstance(e.get('lanes'), dict) + and e['lanes'].get('committed') is True + and e['lanes'].get('uncommitted') is False for e in entries): + print('SKIP not-committed-only %s (%d entries lack an explicit committed-only lanes map)' + % (p, len(entries))) + continue + # EXACT identity, via the BLOB OIDs the record already carries on its "index a..b" line. + # Comparing the stored patch TEXT was the obvious move and is wrong: bare git diff + # output moves with the reader's gitconfig, so a contributor with diff.noprefix, + # diff.context or diff.external gets a permanent mismatch and this fallback silently + # never fires — the same works-on-my-machine class the sandbox-bypass decision rejected + # an allowlist for. Pinning flags (--no-ext-diff --no-color -U3 --src-prefix …) fixes + # OUR side only; if the CLI produced the record under that same config, hardening makes + # the mismatch worse, not better. Blob OIDs are content hashes, so they are stable on + # BOTH sides and are exact content identity rather than a rendering of it. Measured: + # identical under diff.noprefix, diff.context, diff.algorithm, diff.external and + # color.diff, where bare patch text differed under three of the five. + # The record abbreviates its OIDs and we ask git for full ones, hence startswith. + bad = None + for e in entries: + pair = wantoid.get(e['filePath']) + if pair is None: + bad = ('path-not-in-pinned-diff', e['filePath']) + break + wsrcmode, wdstmode, wsrc, wdst, wstatus = pair + # A rename or copy in the PINNED range cannot be verified from a record keyed on the + # post-rename path alone: the record carries no source path, so an edit-only review + # of that same path is indistinguishable from one that covered the rename. The docs + # already call renames a false-negative; fail closed so that is actually true. + if wstatus[:1] in ('R', 'C'): + bad = ('rename-unverifiable', e['filePath']) + break + text = e.get('diff') or '' + mo = INDEX.search(text) + # MODES FIRST. Blob OIDs alone accept a record that never saw a mode change: with + # chmod +x in one commit and an edit in the next, a stored review of the edit alone + # carries the same path, counts, lanes and BOTH OIDs as the pinned range covering + # both. Only the modes differ — pinned 100644->100755 against the record's + # 100755->100755. A record that states no mode at all cannot prove identity either. + smodes = storedmodes(text, mo) + if smodes is None or smodes != (wsrcmode, wdstmode): + bad = ('mode-mismatch', e['filePath']) + break + if mo is None: + # Git omits "index a..b" exactly when the BLOB IS UNCHANGED — for a mode change + # (chmod +x) the old/new mode lines carry the information instead, and the check + # above has already matched them. This repo ships executable scripts, so a + # chmod is routine; rejecting the record outright would permanently disable + # this fallback for any PR containing one. Content identity for such an entry + # is therefore src == dst. A missing index line with CHANGED content stays a + # genuine mismatch. (A 100%-similarity rename also omits the line, but never + # reaches here: the CLI scopes its stored patch and counts to the post-rename + # path, so the record's numstat differs from the pinned range's and the scope + # prefilter rejects it first — renames stay the documented false-negative.) + if wsrc != wdst: + bad = ('no-index-line', e['filePath']) + break + continue + if not wsrc.startswith(mo.group(1)) or not wdst.startswith(mo.group(2)): + bad = ('blob-mismatch', e['filePath']) + break + # "is not None", not truthiness: an empty string is a real value here and must not read + # as a pass. The two labels stay distinct so the reader is not sent hunting a content + # difference when the record simply lacks the line — and note a git failure cannot + # reach this point at all, having already exited 2 above. + if bad is not None: + print('SKIP %s %s (%s)' % (bad[0], p, bad[1])) + continue + candidates.append((os.path.getmtime(p), p)) +# Several records can pass every check at once — a re-run after a transient failure +# produces a second record covering the same change, which is precisely the situation +# this fallback exists for. Emit ONE MATCH so there is nothing to choose between: newest +# by mtime wins, since a re-run supersedes the attempt it replaced. +candidates.sort(reverse=True) +for i, (_, p) in enumerate(candidates): + print('MATCH' if i == 0 else 'SKIP superseded-duplicate', p) +PY +This is shell-agnostic, exits 0 with empty output when nothing matches, and keeps the malformed-record guard. An empty result means no stored review matches the pinned change — that is the normal case, not an error, and the log fallback below must still run. An ERROR line with exit status 2 is NOT that case: the pinned diff could not be computed, so no verdict was reached at all — report that in statusNote rather than saying no record matched. +Generalise that: only "exit 0 with no MATCH" means no record matched. ANY non-zero exit — the ERROR line above, or an unexpected traceback from a record shape not anticipated here — means the scan reached no verdict. Report it as such and still run the log fallback; never let a non-zero exit read as a clean negative. +There is at most ONE MATCH line by construction, and it is a genuine CodeRabbit review of exactly the pinned change, so USE IT no matter which session, working directory or resolved base produced it — read the record's baseCommitId into statusNote for the reader, but do not treat a difference as disqualifying. A SKIP line is a review of a different change, one that cannot prove it was committed-only, or a superseded earlier run — never report any of them as this lane's result. +Acceptance is layered, cheapest first, and only the last one is identity: same paths, then same per-file line counts, then the committed-only lanes shape, then the FILE MODES AND BLOB OIDs the record's own patch states, equal to git's for the pinned range. The first two are a prefilter — two different patches collide on a numstat trivially — so never treat a scope match alone as a result. Modes are part of identity, not decoration: with a chmod +x in one commit and an edit in the next, a stored review of the edit alone carries the same paths, counts, lanes and both blob OIDs as the pinned range covering both, and only the modes differ. Identity is deliberately NOT the stored patch text: that text moves with the reader's gitconfig, so a contributor with diff.noprefix or diff.context would get a permanent mismatch and this fallback would silently never fire. Modes and blob OIDs do not move with it. +An entry whose patch carries no "index a..b" line is NOT automatically rejected: git omits that line precisely when the blob is unchanged, which for a record that reaches this check means an ordinary mode change (chmod +x). Such an entry is accepted when the pinned diff agrees the blob is unchanged (src == dst) and rejected as no-index-line otherwise. Rejecting them outright would have disabled this fallback for any PR that so much as marks a script executable. A 100%-similarity rename omits the line too, but a record covering one never reaches this check at all — see the rename note below, and do not read this paragraph as accepting one. +So SKIP no-index-line is a genuine mismatch, not a false negative: it means the record omitted "index a..b" for a file whose content DID change over the pinned range, i.e. it is not a review of this change. The other SKIP labels read as: scope-mismatch = different file set or line counts; not-committed-only = the record cannot prove it excluded uncommitted work; mode-mismatch = same content but a different file mode, or a record that states no mode at all — the ordinary cause is a chmod landing in the pinned range but not in the record's; blob-mismatch = same files, different content; path-not-in-pinned-diff = the record covers a file this range does not touch; malformed-entries = an entry carries no filePath; rename-unverifiable = the pinned range renames or copies this path, which a record keyed on the post-rename path cannot corroborate; superseded-duplicate = an older run of the same change. +Known false-negatives, all erring toward skipping, which is the safe direction: a binary file makes git print "-" for both counts where the record stores numbers; and a rename never matches, because a rename or copy in the pinned range is rejected outright as rename-unverifiable: the record is keyed on the post-rename path and carries no source path, so an edit-only review of that path cannot be told apart from one that covered the rename (measured: pinned R077 old.txt -> new.txt and stored M new.txt share modes, blobs and counts). The CLI also scopes its stored patch and counts to the post-rename path, so such a record usually fails the scope prefilter first; the status check is what makes the rejection guaranteed rather than incidental. Do NOT expect a 100%-similarity rename to be rescued by the no-index-line branch — it never reaches it. Neither case is observed in practice, but a SKIP scope-mismatch on a PR with renames or binaries, or a SKIP blob-mismatch on a file whose content you believe is right, is the likely cause. In every case the log fallback below still runs. +The findings live in the UUID-named sibling files only. Take a sibling *.json as a comment ONLY if it parses and carries both "fileName" and "comment"; skip anything else. A real record also contains git.json, incrementalDiff.json and internalState.json, which are metadata — ingesting them as findings would invent comments that do not exist. Do not reinterpret the stored diff yourself; use the comments as written (fileName, startLine, severity, commentCategory, title, comment). Say in statusNote that the findings came from the persisted store rather than your own run, and give the record path, its baseCommitId, and the fact that the base was not used to select it. +Only if no matching record exists: check the newest ~/.coderabbit/logs/ file for 429/queue lines and return status:"unavailable" with what you saw. CodeRabbit is best-effort and the review continues without it.` function coderabbitReviewPrompt() { return `You are the CodeRabbit reviewer in a multi-reviewer cross-review. @@ -401,17 +712,35 @@ if (missing.length) { // ---------- Merge & dedupe into candidates ---------- const candidates = [] const byKey = new Map() +// IDs already handed to the cross-review round for evaluation. Merging into one of these +// is unsafe: its votes were cast on the pre-merge content, so anything appended afterwards +// would inherit a verdict nobody gave it. Populated when the candidate list is presented. +const presentedIds = new Set() // The dedup key includes a normalized summary so two distinct bugs at the same // path:line stay separate candidates, and the consumer coordinates so one changed // declaration breaking two callers stays two candidates — each gets its own // adversarial verification instead of the second consumer being silently dropped. +// Keying on location ALONE was tried and reverted. It did merge the real duplicates +// (two lanes wording one defect differently), but the evaluation schema permits exactly +// one verdict per candidate id, so a merged pair of DISTINCT defects has no correct +// verdict: confirming the real one also confirms the false one, and refuting the false +// one dismisses the real one. Retaining both summaries prevented data loss but not +// mis-adjudication, which is the worse failure. Duplicates are now surfaced as a flag +// (see addCandidate) so reviewers can adjudicate equivalence themselves rather than +// having it assumed from a shared line number. const normSummary = (s) => (s || '').toLowerCase().replace(/\s+/g, ' ').trim() const SEV_RANK = { critical: 3, major: 2, medium: 1, minor: 0 } function addCandidate(f, source) { const key = `${f.path}:${f.line}:${normSummary(f.summary)}:${f.consumerPath || ''}:${f.consumerLine ?? ''}` const existing = byKey.get(key) - if (existing) { + // NEVER merge into an already-presented candidate, even on an exact key match. A finding + // raised DURING cross-review that merged into a presented id would be tallied with + // evaluations cast before it existed — two prior AGREEs would confirm a defect nobody + // read, and the single adversarial refuter cannot split a composite afterwards. A late + // finding therefore becomes its own candidate; being unpresented, it lands in `still` and + // stays contested for the human, which is what the cross-review round already documents. + if (existing && !presentedIds.has(existing.id)) { if (!existing.sources.includes(source)) existing.sources.push(source) // Merge the duplicate's DATA, not only its source: first-reporter ordering must // not downgrade severity or discard stronger evidence/impact. Consumer coordinates @@ -422,8 +751,33 @@ function addCandidate(f, source) { return existing } const c = { ...f, id: `F${candidates.length + 1}`, sources: [source], flags: [] } - byKey.set(key, c) + // Only the first holder owns the key. When the refusal above fired, the key still maps + // to the presented candidate, so a second late finding at the same spot also becomes its + // own candidate rather than silently attaching to either. + if (!existing) byKey.set(key, c) candidates.push(c) + // Same location, different wording — the case location-only keying was meant to fix. + // Flag it instead of merging: the flag reaches both the cross-review candidate list and + // the refuter prompt, so reviewers decide whether the two are one defect and evaluate + // them consistently. That keeps equivalence an explicit judgement rather than an + // assumption from a shared line number, and keeps one verdict per real defect. + // The hint must respect the SAME identity the key does, consumer coordinates included. + // One changed declaration breaking two callers is deliberately two candidates; hinting + // that they are possible duplicates would push reviewers to collapse a distinction the + // key exists to preserve. Only a genuine same-location, same-consumer pair is hinted. + const sameSpot = (a, b) => a.path === b.path && a.line === b.line + && (a.consumerPath || '') === (b.consumerPath || '') + && (a.consumerLine ?? null) === (b.consumerLine ?? null) + for (const other of candidates) { + if (other === c || !sameSpot(other, c)) continue + const where = c.consumerPath ? `${c.path}:${c.line} -> ${c.consumerPath}:${c.consumerLine}` : `${c.path}:${c.line}` + const pair = [[other, c], [c, other]] + for (const [a, b] of pair) { + if (!a.flags.some((s) => s.startsWith(`possible duplicate of ${b.id}`))) { + a.flags.push(`possible duplicate of ${b.id} (same ${where}, different wording) — decide whether they are one defect and evaluate both consistently`) + } + } + } return c } @@ -446,8 +800,21 @@ function flagUnchecked(c, f, source, filesChecked) { // its reviewer as a source, after which evidenceFor() hands it the co-reporter's // citation and it silently tips the 2-of-3 tally. Rejecting at intake means an // unevidenced report never becomes a vote anywhere. +// ONE coordinate rule, used for a finding's own path/line and for an integration +// finding's consumerPath/consumerLine. The schema requires both fields but constrains +// neither — no minLength, no minimum — so "", " ", 0 and -1 all satisfy it and all +// used to pass a truthiness/null check. Every previous version of this guard fixed the +// pair it was shown and left the other, which is why this is a shared helper rather than +// two call-site conditions: the next field pair added should reuse it, not re-derive it. +const hasCoords = (p, l) => typeof p === 'string' && p.trim() !== '' && Number.isInteger(l) && l > 0 + function intake(f, source, filesChecked) { - if (!(f.evidence || '').trim()) { + const why = dropReason(f) + if (why === 'unlocatable') { + log(`Dropped unlocatable finding from ${source} — path=${JSON.stringify(f.path)} line=${JSON.stringify(f.line)} — "${f.summary}" (a real path and a 1-based line are required)`) + return null + } + if (why === 'unevidenced') { log(`Dropped unevidenced finding from ${source} at ${f.path}:${f.line} — "${f.summary}" (evidence is required)`) return null } @@ -456,29 +823,108 @@ function intake(f, source, filesChecked) { return c } +// Why a finding cannot enter consensus, or null if it can. Separate from intake() so a +// caller can report the ACTUAL reason instead of inferring one from a subtraction. +// Coordinates first: a finding nobody can locate cannot be verified, cross-reviewed or +// acted on, and it would still occupy a candidate slot and carry a vote. Both checks +// cover EVERY lane — the schema is shared, so any hole in it is shared too. +function dropReason(f) { + if (!hasCoords(f.path, f.line)) return 'unlocatable' + if (!(f.evidence || '').trim()) return 'unevidenced' + return null +} + +// Take a whole batch and report what survived. EVERY required-lane batch goes through +// this, because the zero-survivor rule was never specific to the integration lane: all +// lanes share one schema, so a required reviewer whose only finding is malformed +// contributes nothing while the run reports ok / consensusReached true — the same +// false-clean, one lane over. Mixed batches still proceed; only a total loss stops. +function intakeBatch(list, source, filesChecked) { + const findings = list || [] + const counts = { returned: findings.length, accepted: 0, unlocatable: 0, unevidenced: 0, candidates: [] } + for (const f of findings) { + const why = dropReason(f) + if (why) counts[why] += 1 + const c = intake(f, source, filesChecked) + if (c) { counts.accepted += 1; counts.candidates.push(c) } + } + return counts +} + +// One message shape, so every lane reports the real reasons rather than a subtraction. +function zeroSurvivors(source, c, extra = '') { + return `Required lane ${source} returned ${c.returned} finding(s) and none survived intake: ${c.unlocatable} lacked a usable path/line, ${c.unevidenced} lacked evidence${extra}. The lane contributed nothing usable, so no consensus was computed.` +} + for (const k of participants) { // R[k] is null for a lane that did not run. Only CodeRabbit can be null here — the // required lanes already returned incomplete above — but participants is a fixed // slot list now, so the guard is what keeps the empty slot from being dereferenced. const r = R[k] if (!r) continue - for (const f of r.findings || []) intake(f, k, r.filesChecked) + const b = intakeBatch(r.findings, k, r.filesChecked) + // CodeRabbit is best-effort: a total loss there records NONE and raises the bar for the + // others, exactly as a lane that did not run at all. Claude and Codex are required, so + // a total loss there is the false-clean this guard exists to prevent. + if (k !== 'coderabbit' && b.returned && !b.accepted) return incomplete(zeroSurvivors(k, b)) } -// Integration is a required lane, so malformed output from it stops the run rather -// than being dropped: silently discarding its only finding produced a result that -// looked clean and reported consensusReached: true while the required lane had in -// effect contributed nothing. The schema cannot make these conditionally required. +// An integration finding is a claim that a specific consumer breaks, so one lacking +// consumerPath/consumerLine cannot be verified and must not enter consensus. The schema +// cannot make those conditionally required, hence the check here. +// Drop them INDIVIDUALLY. Failing the whole run on the first one is what this did before +// and it is disproportionate: observed on PR 1908, the lane returned several findings — +// one a real, evidenced stale UAT fixture — plus a stale-doc-comment finding that +// legitimately has no consumer, and the run reported incomplete with all four lanes ok, +// ~400k subagent tokens spent, and no report produced at all. +// Failing closed still matters for the case that motivated it: silently dropping the +// lane's ONLY finding once yielded consensusReached: true while a required lane had in +// effect contributed nothing. So stop only when nothing usable survives. +// Measure that on what SURVIVES intake(), not on what passes the coordinate check. +// intake() independently drops a finding with blank evidence, so gating on the +// coordinate filter alone let a coordinate-complete, whitespace-evidence finding through +// it and then silently out of intake() — zero candidates, status ok, consensusReached +// true. That is the same false-clean this guard exists to prevent, in a narrower form. +// One rule covers both drop reasons: a non-empty integration result that yields no +// accepted finding means the required lane contributed nothing. +const integUsable = [], integDropped = [] for (const f of integ.findings || []) { - if (!f.consumerPath || f.consumerLine == null) { - return incomplete(`Integration lane returned a finding at ${f.path}:${f.line} ("${f.summary}") without consumerPath/consumerLine. An integration finding is a claim that a specific consumer breaks and cannot be verified without both.`) - } + // Presence is not usability, and the CONSUMER side needs the same rule as the finding's + // own coordinates — same schema, same absent constraints, same false-clean if it slips. + // hasCoords is shared with intake() precisely so the two cannot drift apart again. + // The finding's own path/line are checked in intake(), so a finding reaching consensus + // has both ends locatable. + if (!hasCoords(f.consumerPath, f.consumerLine)) integDropped.push(f) + else integUsable.push(f) +} +if (integDropped.length) { + // Never silent: an unreported drop is how a thinned lane passes for a clean one. + log(`Integration lane: dropped ${integDropped.length} finding(s) lacking consumerPath/consumerLine — ${integDropped.map((f) => `${f.path}:${f.line}`).join(', ')}`) +} +// Same batch rule as every other required lane; the only extra is the consumer-side +// filter above, whose count is reported separately. Do NOT infer a reason by subtracting +// counts: own-coordinate and evidence failures are distinct and were being reported as +// one, which sent the reader after the wrong defect. +const integBatch = intakeBatch(integUsable, 'integration', integ.filesChecked) +const integReturned = (integ.findings || []).length +if (integReturned && !integBatch.accepted) { + return incomplete(zeroSurvivors('integration', { ...integBatch, returned: integReturned }, + `, and ${integDropped.length} lacked a usable consumerPath/consumerLine. An integration finding is a claim that a specific consumer breaks and cannot be verified without both ends located and evidence`)) } -for (const f of integ.findings || []) intake(f, 'integration', integ.filesChecked) log(`Merged: ${candidates.length} unique candidate finding(s)`) // ---------- Consensus (round 1 reports, then one cross-review round) ---------- const evals = { claude: {}, codex: {}, coderabbit: {} } // reviewer -> id -> latest evaluation const resolution = {} // id -> 'confirmed' | 'dismissed' | 'open' +// Ids that reached `contested` because they were raised DURING cross-review and so were +// never presented for evaluation — as opposed to being presented, evaluated, and left +// split. Both land in the same bucket, and a reader cannot otherwise tell "the reviewers +// disagreed" from "nobody has looked at this yet", which are different asks: one needs a +// tie broken, the other needs someone to read it. Observed on a real run: 8 of 8 +// contested findings were late, each carrying a single AGREE from its reporter and NONE +// elsewhere — zero actual disagreements, reported as eight contested. +// Annotation only: membership, resolution values and consensusReached are unchanged, so +// nothing that already consumes this result can regress. +const lateIds = new Set() const dismissReason = {} // Latest cross-review evaluation wins over original-reporter status, so a reporter @@ -532,6 +978,9 @@ if (pending.length) { // Without this, dropping the old crossChecked gate would let a candidate nobody // actually evaluated ride its initial source votes straight to "confirmed". const presented = new Set(items.map((c) => c.id)) + // Mirror into the module-level set addCandidate consults, so a late finding cannot be + // merged into an id whose evaluations predate it. + for (const id of presented) presentedIds.add(id) for (let i = 0; i < crossParticipants.length; i++) { const k = crossParticipants[i] const res = ok(results[i]) @@ -560,12 +1009,21 @@ if (pending.length) { } evals[k][e.id] = e } - // newFindings go through the same intake gate as round 1 — an unevidenced - // late finding must not enter the tally either. - for (const f of res.newFindings || []) { - const c = intake(f, k, res.filesChecked) - if (c && !pending.includes(c.id)) pending.push(c.id) - } + // newFindings go through the same intake gate as round 1 — an unevidenced or + // unlocatable late finding must not enter the tally either. But NOT the + // zero-survivor rule, which does not transfer to this round. That rule tests + // "the lane contributed nothing", and round 1 can assert that because findings + // are the lane's whole output. Here the lane's output is its evaluations, and + // the completeness gate above already returned incomplete unless this lane + // evaluated EVERY presented candidate — so by this line it has provably + // contributed. newFindings are strictly supplementary and volunteered. + // Failing the run on them would discard a complete set of adjudications plus + // the verification round over one imprecise extra finding: the same + // disproportionate total-loss observed on PR 1908, reintroduced one round over. + // So drop malformed late findings individually — intake() logs each with its + // reason, so this is never silent — exactly as the integration lane does. + const nb = intakeBatch(res.newFindings, k, res.filesChecked) + for (const c of nb.candidates) if (!pending.includes(c.id)) pending.push(c.id) }) if (bad) return incomplete(bad) const still = [] @@ -576,7 +1034,7 @@ if (pending.length) { // lanes independently raising the same late finding would otherwise reach two // AGREEs with zero cross-checks, contradicting the one-evaluation-per-candidate // contract. They stay contested for the human; we do not add another round. - if (!presented.has(id)) { still.push(id); continue } + if (!presented.has(id)) { still.push(id); lateIds.add(id); continue } const t = tally(c) if (t === 'confirmed' || t === 'dismissed') { resolution[id] = t @@ -593,7 +1051,8 @@ if (pending.length) { } pending = still const counts = Object.values(resolution) - log(`Tally: confirmed=${counts.filter((v) => v === 'confirmed').length}, dismissed=${counts.filter((v) => v === 'dismissed').length}, contested=${pending.length}`) + const lateCount = pending.filter((id) => lateIds.has(id)).length + log(`Tally: confirmed=${counts.filter((v) => v === 'confirmed').length}, dismissed=${counts.filter((v) => v === 'dismissed').length}, contested=${pending.length} (${pending.length - lateCount} evaluated-but-split, ${lateCount} raised late)`) } // Still unresolved after the cross-review round: no valid vote at all -> open question; @@ -701,6 +1160,11 @@ const contested = candidates .filter((c) => contestedIds.includes(c.id)) .map((c) => ({ ...emit(c), + // Why this is unsettled, which the positions alone do not say. + adjudication: lateIds.has(c.id) ? 'raised-late' : 'evaluated', + why: lateIds.has(c.id) + ? 'raised during the cross-review round, after candidates were presented — never cross-evaluated, so the only position is its reporter\'s' + : 'presented and evaluated, but the reviewers did not reach 2-of-3', positions: Object.fromEntries( participants.map((k) => [ k, @@ -724,7 +1188,8 @@ const unresolved = candidates .filter((c) => resolution[c.id] === 'open') .map((c) => ({ ...emit(c), why: 'reached consensus but did not survive verification, or drew no valid vote — see openQuestions' })) -log(`Done: ${confirmed.length} confirmed, ${contested.length} contested, ${unresolved.length} unresolved, ${dismissed.length} dismissed, ${openQuestions.length} open questions`) +const contestedLate = contested.filter((c) => c.adjudication === 'raised-late').length +log(`Done: ${confirmed.length} confirmed, ${contested.length} contested (${contested.length - contestedLate} evaluated-but-split, ${contestedLate} raised late), ${unresolved.length} unresolved, ${dismissed.length} dismissed, ${openQuestions.length} open questions`) return { status: 'ok',