From a4101bcf1b9dcc86b8fd9d67dc97bb4f8f077c3a Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 21:32:10 +0200 Subject: [PATCH 01/10] docs(skills, babysit, handoff-to): handoff-to and babysit set a Codex review's effort from the diff size (high, xhigh above about 50 files), because medium reviews were measured shallow --- .claude-plugin/marketplace.json | 4 ++-- .claude/skills/babysit/SKILL.md | 9 ++++++--- plugins/genesis-tools/.claude-plugin/plugin.json | 2 +- plugins/genesis-tools/skills/handoff-to/SKILL.md | 11 ++++++++++- .../skills/handoff-to/references/codex.md | 2 ++ 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index b2e07a2a35..64d2031326 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,7 +1,7 @@ { "$schema": "https://anthropic.com/claude-code/marketplace.schema.json", "name": "genesis-tools", - "version": "1.0.114", + "version": "1.0.115", "description": "Plugins for GenesisTools CLI development and management", "owner": { "name": "genesiscz", @@ -11,7 +11,7 @@ { "name": "genesis-tools", "description": "Skills and utilities for working with GenesisTools CLI toolkit. Provides guidance for discovering, executing, and troubleshooting genesis tools with integrated workflow support.", - "version": "1.0.114", + "version": "1.0.115", "author": { "name": "GenesisTools" }, diff --git a/.claude/skills/babysit/SKILL.md b/.claude/skills/babysit/SKILL.md index 697d09dac4..cfc1732026 100644 --- a/.claude/skills/babysit/SKILL.md +++ b/.claude/skills/babysit/SKILL.md @@ -53,7 +53,8 @@ Write it to `/tmp/cc///babysit/run.json` at the start; the scr | Field | Default | Notes | |---|---|---| | `prs` | asked | PR numbers. Small PRs first; never a PR another session owns (step 3.1). | -| `worker` per PR | `grok` | `grok` (model `grok-4.7-build-fast`, `--auth subscription`) or `codex` (model `gpt-6.1-sol`, effort `medium`, `--write ask`). Mix freely; one worker per PR. | +| `worker` per PR | `grok` | `grok` (model `grok-4.7-build-fast`, `--auth subscription`) or `codex` (model `gpt-6.1-sol`, `--write ask`). Mix freely; one worker per PR. | +| `effort` per PR | from the work | Codex `--effort`: `low`, `medium`, `high`, `xhigh` or `max`. A fix round: `medium`, `high` for a Critical or a cross-file finding. An independent review (step 7a): `high`, `xhigh` above about 50 changed files. Medium reviews were measured shallow (handoff-to, "Effort is the review's depth"). | | `mode` | from `scripts/mode.ts` | `worktree` or `unified`, step 2. | | `maxParallel` | 3 | Workers at once. The master is the bottleneck, not the workers. | | `post` | ask | Post the round summary and replies after the push (review-loop step 4), or hold them in the vault. | @@ -175,7 +176,7 @@ summary go to `.claude/work/babysit/` inside that cwd (ignored by git; Grok's ja - Grok: `tools grok spawn --name bs--grok --cwd -m grok-4.7-build-fast --auth subscription --label "babysit pr" --prompt-file ` with Bash `run_in_background` (it blocks for the whole turn). -- Codex: `tools codex spawn --name bs--sol --model gpt-6.1-sol --effort medium --write ask --cwd --session +- Codex: `tools codex spawn --name bs--sol --model gpt-6.1-sol --effort --write ask --cwd --session --label "babysit pr" --prompt-file `. The `--label` tags every cost-ledger row of the session, so a run's spend is one query later (step 9). @@ -323,7 +324,9 @@ merge bar. When the user asks for a dedicated review of a PR, before or after it review: `bun .claude/skills/babysit/scripts/review-brief.ts --run --pr --wt [--post-merge] [--swift]`, then `steer.ts` with the printed file to a worker that did NOT fix this PR (Sol; Astra only for a risky cross-system PR, -by the model rule in step 1). Judge its findings like a bot's. Before a merge they become the next fix round (`fill-brief --judge --json` (`cost.lastTurn`: `durationMs`, `toolCalls`, `tokens.reasoning`). Judge its findings like a bot's. Before a merge they become the next fix round (`fill-brief --judge --dedicated-review`); after a merge they go to a follow-up PR from master (2026-10-14: #518 carried #509's after-merge findings). Never count a reply-only round, or a label, as a fix round: on 2026-10-14 the report said "#500 merged after 3 rounds" when GitHub showed two round summaries. diff --git a/plugins/genesis-tools/.claude-plugin/plugin.json b/plugins/genesis-tools/.claude-plugin/plugin.json index 68d79199bf..9d01c1c762 100644 --- a/plugins/genesis-tools/.claude-plugin/plugin.json +++ b/plugins/genesis-tools/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "genesis-tools", - "version": "1.0.114", + "version": "1.0.115", "description": "Skills and utilities for working with GenesisTools CLI toolkit. Provides guidance for discovering, executing, and troubleshooting genesis tools with integrated workflow support.", "author": { "name": "GenesisTools" diff --git a/plugins/genesis-tools/skills/handoff-to/SKILL.md b/plugins/genesis-tools/skills/handoff-to/SKILL.md index a184769f03..dea9c84987 100644 --- a/plugins/genesis-tools/skills/handoff-to/SKILL.md +++ b/plugins/genesis-tools/skills/handoff-to/SKILL.md @@ -83,10 +83,19 @@ Set a model explicitly; an Astra parent should not make every worker Astra. |---|---|---| | Mechanical extraction, formatting, simple lookups, or a fully specified small edit | `gpt-6-luna` | low | | Bounded repository exploration, tracing an established call path, or gathering evidence | `gpt-6-luna` | medium | -| Normal implementation, reproducible debugging, and code review | `gpt-6.1-sol` | medium; high for complex reviews | +| Normal implementation and reproducible debugging | `gpt-6.1-sol` | medium | +| Code review of a PR or branch | `gpt-6.1-sol` | high; xhigh above about 50 changed files | | Difficult cross-system reasoning, ambiguous failures, or an evidence-backed escalation | `gpt-6-astra` | high | `gpt-6.1-sol` accepts `low` to `max` effort only; `none` and `minimal` return an error, as on Astra. + +**Effort is the review's depth: set it from the diff size, never leave a review at medium.** Measured on +2026-10-22 with nine Sol reviews at `medium`: 1 to 4 minutes each, 8 to 26 tool calls, and 358 to 2,399 +reasoning tokens, also for a 125-file, 6,200-line PR. The findings were real, but no worker could have read +every file. Pass `--effort high` for a review, `xhigh` above about 50 changed files, `max` only for a risky +cross-system PR, and tell the worker in the brief to read every changed file and to end with `Files reviewed: + of `. Check the depth afterwards: `tools codex status --name --json` carries the turn's +`durationMs`, `toolCalls` and `tokens.reasoning` under `cost.lastTurn`. Astra is the escalation tier, not the default for exploration. Keep the cheaper worker's findings and failed verification when escalating. If a model is unavailable, disclose the fallback and select the nearest suitable tier instead of silently choosing Astra. diff --git a/plugins/genesis-tools/skills/handoff-to/references/codex.md b/plugins/genesis-tools/skills/handoff-to/references/codex.md index 62852ab47e..4961a0b639 100644 --- a/plugins/genesis-tools/skills/handoff-to/references/codex.md +++ b/plugins/genesis-tools/skills/handoff-to/references/codex.md @@ -15,6 +15,8 @@ The `tools codex spawn` command accepts an explicit model string. Availability s depends on the selected account and backend. Verify the recorded model after dispatch; do not treat a requested model or the parent's config as proof of what ran. Code-review workers should explicitly select Sol; `review_model` does not route arbitrary workers. +Give a review `--effort high`, or `xhigh` above about 50 changed files: at `medium` a review spends about +two thousand reasoning tokens however large the diff is (parent skill, "Effort is the review's depth"). ## Choose the orchestration layer From f72736a583a5d7ad140e8e2aad35d60e44d5c10b Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:02:58 +0200 Subject: [PATCH 02/10] feat(worker): the shared worker contract carries a per-tier review block and the diff manifest, so a reviewer on any backend gets the same depth rules and output shape --- src/utils/worker/contract.test.ts | 63 +++++++++++++++++- src/utils/worker/contract.ts | 104 ++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 1 deletion(-) diff --git a/src/utils/worker/contract.test.ts b/src/utils/worker/contract.test.ts index a444cb10a0..824921372e 100644 --- a/src/utils/worker/contract.test.ts +++ b/src/utils/worker/contract.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { buildWorkerContract, WORKER_REPORT_KEYS } from "./contract"; +import { buildWorkerContract, type ReviewManifest, reviewLines, WORKER_REPORT_KEYS } from "./contract"; const bus = { agentName: "codex_task", leadName: "lead", rendezvousSession: "s-1" }; @@ -78,4 +78,65 @@ describe("buildWorkerContract", () => { "loaded for reference" ); }); + + test("review: each tier stacks the rules below it, and low is triage without them", () => { + const low = reviewLines("low").join("\n"); + const medium = reviewLines("medium").join("\n"); + const high = reviewLines("high").join("\n"); + const xhigh = reviewLines("xhigh").join("\n"); + const max = reviewLines("max").join("\n"); + + expect(low).toContain("Triage only: name the three riskiest files"); + expect(low).not.toContain("Read every hunk"); + expect(medium).toContain("Read every hunk of every file in the manifest."); + expect(medium).not.toContain("Open the callers"); + expect(high).toContain("Read every hunk"); + expect(high).toContain("Open the callers and the tests of every changed function."); + expect(high).toContain("Label each finding observed"); + expect(high).not.toContain("per-file pass"); + expect(xhigh).toContain("Label each finding observed"); + expect(xhigh).toContain("Probe each finding (`bun -e`, a test) before you report it, or mark it inferred."); + expect(xhigh).toContain("second pass over every file that touches secrets, processes, concurrency or I/O"); + expect(xhigh).not.toContain("adversarial pass"); + expect(max).toContain("Probe each finding"); + expect(max).toContain("Read the area rules of every touched folder."); + expect(max).toContain("Make an adversarial pass"); + + for (const text of [low, medium, high, xhigh, max]) { + expect(text).toContain( + "`1. [Severity] path:line - defect. Scenario: . Fix: .`" + ); + expect(text).toContain("`Files reviewed: of `"); + expect(text).toContain("`Not reviewed: none`"); + } + }); + + test("review: the manifest and tier ride after the report shape; without review the text is unchanged", () => { + const manifest: ReviewManifest = { + base: "origin/master", + files: [ + { path: "src/a.ts", added: 40, deleted: 2 }, + { path: "assets/logo.png", added: 0, deleted: 0, binary: true }, + ], + added: 40, + deleted: 2, + }; + + for (const backend of ["codex", "grok", "claude"] as const) { + const plain = buildWorkerContract({ backend, sandbox: "read-only" }); + const reviewer = buildWorkerContract({ + backend, + sandbox: "read-only", + review: { effort: "high", manifest }, + }); + + expect(buildWorkerContract({ backend, sandbox: "read-only", review: null })).toBe(plain); + // Without surfaces the review block is the only addition, appended after the report shape. + expect(reviewer.startsWith(`${plain}\n`)).toBe(true); + expect(reviewer).toContain("You are a code REVIEWER at the high tier."); + expect(reviewer).toContain("Files to review (2, +40 -2):\n- src/a.ts +40 -2\n- assets/logo.png (binary)"); + expect(reviewer).toContain("`git diff origin/master...HEAD -- `"); + expect(reviewer.indexOf("OPEN: ")).toBeLessThan(reviewer.indexOf("code REVIEWER")); + } + }); }); diff --git a/src/utils/worker/contract.ts b/src/utils/worker/contract.ts index 1adcbe82e5..2550af5df4 100644 --- a/src/utils/worker/contract.ts +++ b/src/utils/worker/contract.ts @@ -17,6 +17,109 @@ export interface WorkerContractInput { bus?: WorkerBus | null; /** Which personal surfaces the worker loaded; on by default for every backend that can honour it. */ surfaces?: { skills: boolean; rules: boolean }; + /** Present when the worker is a code reviewer: the tier rules, the diff manifest and the review output shape. */ + review?: WorkerReviewInput | null; +} + +/** Review depth tiers, shallowest first. They reuse the reasoning-effort names the workers already take. */ +export const REVIEW_EFFORTS = ["low", "medium", "high", "xhigh", "max"] as const; +export type ReviewEffort = (typeof REVIEW_EFFORTS)[number]; + +export function isReviewEffort(value: unknown): value is ReviewEffort { + return typeof value === "string" && (REVIEW_EFFORTS as readonly string[]).includes(value); +} + +/** One changed file of `git diff --numstat ...HEAD`. */ +export interface ReviewManifestFile { + path: string; + added: number; + deleted: number; + /** git printed `-` for both counts: there is no text to read. */ + binary?: boolean; +} + +/** The files a review must cover, computed by the harness before the worker starts. */ +export interface ReviewManifest { + base: string; + files: ReviewManifestFile[]; + added: number; + deleted: number; +} + +export interface WorkerReviewInput { + effort: ReviewEffort; + manifest: ReviewManifest; +} + +/** Added plus deleted lines: the size the reasoning floor is measured against. */ +export function manifestChangedLines(manifest: Pick): number { + return manifest.added + manifest.deleted; +} + +/** `Files to review (3, +40 -2):` and one `path +a -d` line per file. */ +export function manifestLines(manifest: ReviewManifest): string[] { + return [ + `Base: ${manifest.base}. Read each change with \`git diff ${manifest.base}...HEAD -- \`.`, + `Files to review (${manifest.files.length}, +${manifest.added} -${manifest.deleted}):`, + ...manifest.files.map((file) => + file.binary ? `- ${file.path} (binary)` : `- ${file.path} +${file.added} -${file.deleted}` + ), + ]; +} + +/** What each tier adds over the one below it; `reviewLines` stacks them up to the chosen tier. */ +const TIER_RULES: Record = { + low: ["Triage only: name the three riskiest files of the manifest and why each one is risky."], + medium: [ + "Read every hunk of every file in the manifest.", + "Every finding names a concrete scenario that triggers it.", + ], + high: [ + "Open the callers and the tests of every changed function.", + "Run the focused tests your sandbox allows.", + "Label each finding observed (name the command that showed it) or inferred.", + ], + xhigh: [ + "Make a per-file pass over correctness, error paths, security, platform differences and tests.", + "Probe each finding (`bun -e`, a test) before you report it, or mark it inferred.", + "Make a second pass over every file that touches secrets, processes, concurrency or I/O.", + "Check the change's own follow-up fixes for new defects.", + ], + max: [ + "Read the area rules of every touched folder.", + "Trace interactions across files: process lifecycle, stacked branches, shared state.", + "Make an adversarial pass: how does this change break in production?", + ], +}; + +/** + * The review contract of one tier: the rules of every tier up to it (high includes medium's), then the + * output shape the depth gate parses. `low` is triage and has no medium rules under it. The shape lines + * come before the five report lines of {@link reportShapeLines}, which still end the message. + */ +export function reviewLines(effort: ReviewEffort): string[] { + const upTo = REVIEW_EFFORTS.indexOf(effort); + const rules = + effort === "low" ? TIER_RULES.low : REVIEW_EFFORTS.slice(1, upTo + 1).flatMap((tier) => TIER_RULES[tier]); + + return [ + `You are a code REVIEWER at the ${effort} tier. Do not change any file; report defects.`, + ...rules, + "Before the five report lines, your final message has this shape:", + "Numbered findings, one per item: `1. [Severity] path:line - defect. Scenario: . Fix: .`", + "Severity is Critical, High, Medium or Low. With no findings, write the line `Findings: none`.", + "Then the line `Ran: `.", + "Then the line `Files reviewed: of `, where N counts every file of the manifest.", + "Then the line `Not reviewed: (), ...` naming every file you did not read, or `Not reviewed: none`.", + ]; +} + +function reviewBlock(input: WorkerContractInput): string[] { + if (!input.review) { + return []; + } + + return [...reviewLines(input.review.effort), ...manifestLines(input.review.manifest)]; } /** The keys every worker's final message carries, in this order. `tools …` printers read RESULT. */ @@ -154,6 +257,7 @@ export function buildWorkerContract(input: WorkerContractInput): string { ...checkpointLines(), ...sandboxLines(input), ...reportShapeLines(), + ...reviewBlock(input), ...surfaceLines(input), ].join("\n"); } From 859e7f0f8171341d45145d6ad9bd2ffbf884771a Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:03:12 +0200 Subject: [PATCH 03/10] feat(codex): spawn --mode review takes a base, injects the diff manifest and the tier contract, picks the tier from the diff size, and gates the depth after each turn (one steer, one rerun a tier higher, then shallow), logging every turn and decision to the review ledger --- src/codex/daemon.ts | 6 + src/codex/lib/driver.ts | 48 ++- src/codex/lib/review/controller.ts | 198 ++++++++++ src/codex/lib/review/escalate.ts | 67 ++++ src/codex/lib/review/gate.ts | 330 ++++++++++++++++ src/codex/lib/review/ledger.ts | 129 +++++++ src/codex/lib/review/prepare.ts | 309 +++++++++++++++ src/codex/lib/review/report.ts | 194 ++++++++++ src/codex/lib/review/review.test.ts | 568 ++++++++++++++++++++++++++++ src/codex/lib/review/types.ts | 44 +++ src/codex/lib/seed-instructions.ts | 4 +- src/codex/lib/session.test.ts | 123 ++++++ src/codex/lib/session.ts | 139 ++++++- src/codex/lib/spawn.ts | 23 +- src/codex/lib/store.ts | 3 + 15 files changed, 2172 insertions(+), 13 deletions(-) create mode 100644 src/codex/lib/review/controller.ts create mode 100644 src/codex/lib/review/escalate.ts create mode 100644 src/codex/lib/review/gate.ts create mode 100644 src/codex/lib/review/ledger.ts create mode 100644 src/codex/lib/review/prepare.ts create mode 100644 src/codex/lib/review/report.ts create mode 100644 src/codex/lib/review/review.test.ts create mode 100644 src/codex/lib/review/types.ts diff --git a/src/codex/daemon.ts b/src/codex/daemon.ts index 6f238a0d49..8200b3a1cd 100644 --- a/src/codex/daemon.ts +++ b/src/codex/daemon.ts @@ -14,6 +14,7 @@ import { computerUseOverrides } from "./lib/computer-use"; import { ControlLogCursor, respondToControl } from "./lib/control-channel"; import { buildAccountLaunchOptions } from "./lib/launch-options"; import { sessionControlPath, sessionDaemonLogPath, sessionLaunchPath } from "./lib/paths"; +import { escalateCodexReview } from "./lib/review/escalate"; import { CodexSessionRuntime } from "./lib/session"; import type { LaunchConfig } from "./lib/spawn"; import { CodexSessionStore } from "./lib/store"; @@ -131,6 +132,11 @@ async function run(): Promise { store, meta: (await store.readMeta(name)) ?? meta, ...(launch.project ? { project: launch.project } : {}), + escalateReview: (request) => escalateCodexReview(request, launch), + onReviewGate: async (event) => { + store.appendEvent(name, { source: "daemon", method: "review_gate", params: event }); + await bridge?.publishEvent(event); + }, onApprovalRequest: async (notice) => { store.appendEvent(name, { source: "daemon", method: "approval_request", params: notice }); if (!meta.agentsEnabled) { diff --git a/src/codex/lib/driver.ts b/src/codex/lib/driver.ts index 95dba8be46..759aa82692 100644 --- a/src/codex/lib/driver.ts +++ b/src/codex/lib/driver.ts @@ -8,6 +8,7 @@ import { WorkerDeliveryRejectedError, workerSourceHome } from "@genesiscz/utils/ import type { WorkerDriver, WorkerVerbOutcome } from "@genesiscz/utils/worker/driver"; import { type CodexControl, parseControlTarget } from "./control"; import { sendControlRequest } from "./control-channel"; +import { prepareReview } from "./review/prepare"; import { isCodexDaemonPid, parseWritePolicy, schemaDriftWarning, spawnCodexSession } from "./spawn"; import { type CodexSessionMeta, CodexSessionStore, deriveSessionStatus } from "./store"; import { followSessionEvents } from "./tail-events"; @@ -52,12 +53,28 @@ export const codexDriver: WorkerDriver = { command .option("--computer-use", "Configure installed official Mac Computer Use for this worker") .option("--home ", "CODEX_HOME override") - .option("--effort ", "Reasoning effort") + .option( + "--effort ", + "Reasoning effort; for --mode review a tier (high | xhigh | max, or low | medium with --allow-shallow), default from the diff size" + ) .option( "--write ", "ask | allow | deny | full (full = no sandbox, refused in a main checkout: use a throwaway worktree)" ) - .option("--mode ", "review | task", "task") + .option( + "--mode ", + "review | task. review: injects the diff manifest and the tier's review contract, then gates the depth after each turn", + "task" + ) + .option( + "--base ", + "--mode review: the ref to diff against (git diff ...HEAD); default the branch's PR target, else its upstream" + ) + .option("--allow-shallow", "--mode review: accept --effort low or medium, which the gate never tunes up") + .option( + "--paths ", + "--mode review: limit the manifest to these paths (one slice of a split review)" + ) .option("--no-agents", `Disable ${toolCommand("agents")} integration`) .option("--session ", `Parent ${toolCommand("agents")} session id`) .option("--writable-root ", "Additional writable roots") @@ -95,12 +112,19 @@ export const codexDriver: WorkerDriver = { session?: string; writableRoot?: string[]; project?: string; + base?: string; + allowShallow?: boolean; + paths?: string[]; }; if (extras.mode !== "review" && extras.mode !== "task") { throw new Error("--mode must be review or task"); } + if (extras.mode === "task" && (extras.base || extras.allowShallow || extras.paths)) { + throw new Error("--base, --allow-shallow and --paths apply to --mode review only"); + } + // A silent no-op here reads as isolation that was applied. It never was, so this // refuses instead of pretending: a caller who asked for isolation and did not get it // must find out now, not from a worker that turned out to load every skill. @@ -116,6 +140,25 @@ export const codexDriver: WorkerDriver = { ); } + const prepared = + extras.mode === "review" + ? await prepareReview({ + cwd: input.cwd, + ...(extras.base ? { base: extras.base } : {}), + ...(extras.effort === undefined ? {} : { effort: extras.effort }), + allowShallow: extras.allowShallow === true, + ...(extras.paths ? { paths: extras.paths } : {}), + }) + : null; + + for (const note of prepared?.notes ?? []) { + out.log.info(note); + } + + for (const warning of prepared?.warnings ?? []) { + out.log.warn(warning); + } + const meta = await spawnCodexSession({ name: input.name, cwd: input.cwd, @@ -125,6 +168,7 @@ export const codexDriver: WorkerDriver = { ...(extras.computerUse === undefined ? {} : { computerUse: extras.computerUse }), ...(extras.home === undefined ? {} : { home: extras.home }), ...(extras.effort === undefined ? {} : { effort: extras.effort }), + ...(prepared ? { review: { ...prepared.config, step: 0, chain: input.name } } : {}), write: parseWritePolicy(extras.write), mode: extras.mode, agents: extras.agents, diff --git a/src/codex/lib/review/controller.ts b/src/codex/lib/review/controller.ts new file mode 100644 index 0000000000..5f77f3022a --- /dev/null +++ b/src/codex/lib/review/controller.ts @@ -0,0 +1,198 @@ +import type { CodexFinishedTurn } from "@genesiscz/utils/codex/turn-cost"; +import { logger } from "@genesiscz/utils/logger"; +import type { ReviewEffort } from "@genesiscz/utils/worker/contract"; +import { sumPartTokens } from "@genesiscz/utils/worker/cost"; +import { checkReview, decideGate, type GateDecision, parseReviewReport, type ReviewGateChecks } from "./gate"; +import { appendReviewRecord, type ReviewGateRecord, reviewLedgerBase } from "./ledger"; +import type { CodexReviewConfig, CodexReviewSpent, ReviewVerdict } from "./types"; + +const log = logger.child({ component: "codex:review-gate" }); + +const UNREAD_RECORDED = 40; + +export type ReviewGateAction = + | { kind: "none" } + | { kind: "steer"; text: string } + | { kind: "escalate"; effort: ReviewEffort; name: string }; + +export interface ReviewTurnOutcome { + /** The session's review record after this turn: spent, step and verdict updated. */ + review: CodexReviewConfig; + action: ReviewGateAction; + /** Absent when the gate did not run: a failed or interrupted turn, or a review whose verdict is final. */ + decision?: GateDecision; + checks?: ReviewGateChecks; +} + +export interface ReviewTurnInput { + name: string; + label?: string; + review: CodexReviewConfig; + finished: CodexFinishedTurn; + /** The final agent message of the turn. */ + report: string; + costUsd: number | null; + ledgerPath?: string; +} + +/** The name an escalated review runs under: the chain's first name and the new tier. */ +export function escalatedReviewName(review: Pick, effort: ReviewEffort): string { + return `${review.chain}-${effort}`; +} + +function addSpent(spent: CodexReviewSpent | undefined, finished: CodexFinishedTurn, costUsd: number | null) { + const tokens = sumPartTokens(finished.parts); + const before = spent ?? { turns: 0, durationMs: 0, toolCalls: 0, reasoningTokens: 0, outputTokens: 0, costUsd: 0 }; + + return { + turns: before.turns + 1, + durationMs: before.durationMs + (finished.durationMs ?? 0), + toolCalls: before.toolCalls + finished.toolCalls, + reasoningTokens: before.reasoningTokens + tokens.reasoning, + outputTokens: before.outputTokens + tokens.output, + costUsd: before.costUsd === null || costUsd === null ? null : before.costUsd + costUsd, + } satisfies CodexReviewSpent; +} + +/** A review whose verdict is final takes follow-up turns without the gate: the lead asked something else. */ +function gateIsOpen(review: CodexReviewConfig): boolean { + return review.verdict === undefined || review.verdict === "steered"; +} + +/** + * Books one finished review turn and decides the gate step. Writes one `turn` line, and one `gate` line when the + * gate ran, to the review ledger. The caller performs the action: a steer into the same session, or an escalated + * spawn under {@link escalatedReviewName}. Nothing here spawns or steers, so the whole decision is testable. + */ +export function recordReviewTurn(input: ReviewTurnInput): ReviewTurnOutcome { + const { finished, report } = input; + const tokens = sumPartTokens(finished.parts); + const knownTokens = finished.parts.length > 0; + const spent = addSpent(input.review.spent, finished, input.costUsd); + const parsed = parseReviewReport(report); + const base = reviewLedgerBase({ name: input.name, review: input.review, label: input.label, at: finished.endedAt }); + + appendReviewRecord( + base, + { + kind: "turn", + turn: finished.turn, + outcome: finished.outcome, + model: finished.model, + durationMs: finished.durationMs, + toolCalls: finished.toolCalls, + reasoningTokens: knownTokens ? tokens.reasoning : null, + outputTokens: knownTokens ? tokens.output : null, + costUsd: input.costUsd, + coverage: parsed.coverage, + }, + input.ledgerPath + ); + + const counted: CodexReviewConfig = { ...input.review, spent }; + + if (finished.outcome !== "completed" || !gateIsOpen(input.review)) { + return { review: counted, action: { kind: "none" } }; + } + + const checks = checkReview({ + report, + manifest: input.review.manifest, + effort: input.review.effort, + reasoningTokens: knownTokens || spent.reasoningTokens > 0 ? spent.reasoningTokens : null, + }); + const decision = decideGate(checks, input.review); + const escalatedTo = + decision.verdict === "escalated" && decision.nextEffort + ? escalatedReviewName(input.review, decision.nextEffort) + : undefined; + const review: CodexReviewConfig = { + ...counted, + verdict: decision.verdict, + reason: decision.reason, + step: + decision.verdict === "steered" || decision.verdict === "escalated" + ? input.review.step + 1 + : input.review.step, + ...(escalatedTo ? { escalatedTo } : {}), + }; + + writeGateRecord({ input, review, checks, decision, escalatedTo }); + log.debug( + { name: input.name, verdict: decision.verdict, reason: decision.reason, step: input.review.step }, + "review gate decided" + ); + + if (decision.verdict === "steered" && decision.steer) { + return { review, action: { kind: "steer", text: decision.steer }, decision, checks }; + } + + if (decision.verdict === "escalated" && decision.nextEffort && escalatedTo) { + return { + review, + action: { kind: "escalate", effort: decision.nextEffort, name: escalatedTo }, + decision, + checks, + }; + } + + return { review, action: { kind: "none" }, decision, checks }; +} + +function writeGateRecord(options: { + input: Pick; + review: CodexReviewConfig; + checks: ReviewGateChecks | null; + decision: Pick; + escalatedTo?: string; +}): ReviewGateRecord | null { + const { input, review, checks, decision } = options; + const record = appendReviewRecord( + reviewLedgerBase({ name: input.name, review, label: input.label }), + { + kind: "gate", + step: review.step, + verdict: decision.verdict, + reason: decision.reason, + coverage: { + reviewed: checks?.coverage.reviewed ?? null, + total: checks?.coverage.total ?? review.manifest.files.length, + }, + reasoningPer100Lines: checks?.reasoning.per100Lines ?? null, + floor: checks?.reasoning.floor ?? 0, + findings: checks?.shape.findings ?? 0, + malformedFindings: checks?.shape.malformed.length ?? 0, + unread: checks?.coverage.unread.slice(0, UNREAD_RECORDED) ?? [], + ...(options.escalatedTo ? { escalatedTo: options.escalatedTo } : {}), + spent: review.spent ?? { + turns: 0, + durationMs: 0, + toolCalls: 0, + reasoningTokens: 0, + outputTokens: 0, + costUsd: null, + }, + }, + input.ledgerPath + ); + + return record?.kind === "gate" ? record : null; +} + +/** + * The gate wanted a step the runtime could not take (the steer was refused, the escalated spawn failed): the review + * ends `shallow` with that reason, in the session record and as one more gate line. + */ +export function markReviewShallow(input: { + name: string; + label?: string; + review: CodexReviewConfig; + reason: string; + ledgerPath?: string; +}): CodexReviewConfig { + const verdict: ReviewVerdict = "shallow"; + const review: CodexReviewConfig = { ...input.review, verdict, reason: input.reason, escalatedTo: undefined }; + writeGateRecord({ input, review, checks: null, decision: { verdict, reason: input.reason } }); + log.warn({ name: input.name, reason: input.reason }, "review marked shallow"); + return review; +} diff --git a/src/codex/lib/review/escalate.ts b/src/codex/lib/review/escalate.ts new file mode 100644 index 0000000000..3170698324 --- /dev/null +++ b/src/codex/lib/review/escalate.ts @@ -0,0 +1,67 @@ +import { logger } from "@genesiscz/utils/logger"; +import type { EscalateReviewRequest } from "../session"; +import { type LaunchConfig, spawnCodexSession } from "../spawn"; +import type { CodexReviewConfig } from "./types"; + +const log = logger.child({ component: "codex:review-escalate" }); + +const FALLBACK_PROMPT = "Review the change between the base and HEAD that the manifest lists."; + +/** + * The review record of the escalated session: the same base, manifest and prompt one tier higher, at step 2 so + * its own miss ends the chain `shallow`. Its verdict, reason and spend start empty. + */ +export function escalatedReview( + review: CodexReviewConfig, + request: Pick +): CodexReviewConfig { + return { + base: review.base, + baseSource: review.baseSource, + baseDetail: review.baseDetail, + ...(review.branch ? { branch: review.branch } : {}), + ...(review.pr === undefined ? {} : { pr: review.pr }), + manifest: review.manifest, + effort: request.effort, + effortSource: "flag", + allowShallow: review.allowShallow, + step: 2, + chain: review.chain, + parent: request.meta.name, + ...(review.prompt ? { prompt: review.prompt } : {}), + }; +} + +/** + * Starts the escalated review from inside the daemon: the same cwd, home, account, write policy, swarm and + * writable roots as the session that missed, under the name the gate recorded. A steer cannot change a running + * thread's effort, which is why the higher tier needs a new session. + */ +export async function escalateCodexReview(request: EscalateReviewRequest, launch: LaunchConfig): Promise { + const { meta } = request; + + if (!meta.review) { + throw new Error(`${meta.name} is not a review session`); + } + + const review = escalatedReview(meta.review, request); + log.info({ from: meta.name, to: request.name, effort: request.effort }, "starting the escalated review"); + + await spawnCodexSession({ + name: request.name, + cwd: meta.cwd, + ...(meta.home ? { home: meta.home } : {}), + ...(meta.model ? { model: meta.model } : {}), + ...(launch.accountId ? { account: launch.accountId } : {}), + ...(launch.computerUse ? { computerUse: true } : {}), + ...(launch.project ? { project: launch.project } : {}), + ...(meta.label ? { label: meta.label } : {}), + write: meta.writePolicy, + mode: "review", + agents: meta.agentsEnabled, + rendezvousSession: meta.rendezvousSession, + writableRoots: launch.writableRoots, + prompt: review.prompt ?? FALLBACK_PROMPT, + review, + }); +} diff --git a/src/codex/lib/review/gate.ts b/src/codex/lib/review/gate.ts new file mode 100644 index 0000000000..ca6b2fcdd4 --- /dev/null +++ b/src/codex/lib/review/gate.ts @@ -0,0 +1,330 @@ +import { + manifestChangedLines, + REVIEW_EFFORTS, + type ReviewEffort, + type ReviewManifest, +} from "@genesiscz/utils/worker/contract"; +import type { CodexReviewConfig, ReviewVerdict } from "./types"; + +/** + * Reasoning tokens per 100 changed lines a tier must reach. Measured 2026-10-22 on a 6,200-line diff: medium + * spent about 40, xhigh about 390. First guesses from one day of data; tune them from `tools codex reviews`. + */ +export const REASONING_FLOOR_PER_100_LINES: Record = { + low: 0, + medium: 0, + high: 100, + xhigh: 250, + max: 250, +}; + +/** The most unread files one steer names; the rest are counted. */ +const STEER_FILE_LIMIT = 40; + +export interface ParsedFinding { + index: number; + text: string; + ok: boolean; +} + +export interface ParsedReviewReport { + coverage: { reviewed: number; total: number } | null; + notReviewed: Array<{ path: string; reason: string | null }>; + findings: ParsedFinding[]; + findingsNone: boolean; + ran: boolean; +} + +/** Markdown emphasis and code ticks a model wraps around the fixed lines; they carry no meaning here. */ +function plain(line: string): string { + return line.replace(/[*_`]/g, "").trim(); +} + +const FINDING_START = /^(\d+)[.)]\s+(.*)$/; +const FINDING_SHAPE = /^\[(critical|high|medium|low)\]\s+\S+:\d+/i; +const SECTION_END = /^(ran|files reviewed|not reviewed|result|at|changed|verify|open|findings):/i; + +function parseNotReviewed(value: string): ParsedReviewReport["notReviewed"] { + if (/^none\.?$/i.test(value.trim())) { + return []; + } + + return value + .split(/,\s+(?![^()]*\))/) + .map((entry) => entry.trim()) + .filter(Boolean) + .map((entry) => { + const match = /^(.*?)\s*\((.*)\)\s*$/.exec(entry); + return match ? { path: match[1].trim(), reason: match[2].trim() || null } : { path: entry, reason: null }; + }); +} + +/** Reads the review shape the contract asks for (`reviewLines`): numbered findings, `Ran:`, `Files reviewed:`, `Not reviewed:`. */ +export function parseReviewReport(report: string): ParsedReviewReport { + const parsed: ParsedReviewReport = { + coverage: null, + notReviewed: [], + findings: [], + findingsNone: false, + ran: false, + }; + let current: ParsedFinding | null = null; + + for (const raw of report.split("\n")) { + const line = plain(raw); + const coverage = /^files reviewed:\s*(\d+)\s*(?:of|\/)\s*(\d+)/i.exec(line); + + if (coverage) { + parsed.coverage = { reviewed: Number(coverage[1]), total: Number(coverage[2]) }; + } + + const notReviewed = /^not reviewed:\s*(.*)$/i.exec(line); + + if (notReviewed) { + parsed.notReviewed = parseNotReviewed(notReviewed[1]); + } + + if (/^ran:/i.test(line)) { + parsed.ran = true; + } + + if (/^findings:\s*none\b/i.test(line)) { + parsed.findingsNone = true; + } + + if (SECTION_END.test(line)) { + current = null; + continue; + } + + const start = FINDING_START.exec(line); + + if (start && !parsed.ran && !parsed.coverage) { + current = { index: Number(start[1]), text: start[2], ok: false }; + parsed.findings.push(current); + continue; + } + + if (current && line) { + current.text = `${current.text} ${line}`; + } + } + + for (const finding of parsed.findings) { + finding.ok = FINDING_SHAPE.test(finding.text) && /scenario:/i.test(finding.text) && /fix:/i.test(finding.text); + } + + return parsed; +} + +export interface GateCheck { + ok: boolean; + detail: string; +} + +export interface ReviewGateChecks { + coverage: GateCheck & { reviewed: number | null; total: number; unread: string[] }; + reasoning: GateCheck & { per100Lines: number | null; floor: number }; + shape: GateCheck & { findings: number; malformed: number[] }; +} + +/** Reasoning tokens per 100 changed lines, rounded; null when the tokens are unknown. */ +export function reasoningPer100Lines(reasoningTokens: number | null, manifest: ReviewManifest): number | null { + if (reasoningTokens === null) { + return null; + } + + return Math.round((reasoningTokens * 100) / Math.max(1, manifestChangedLines(manifest))); +} + +function checkCoverage( + parsed: ParsedReviewReport, + manifest: ReviewManifest, + report: string +): ReviewGateChecks["coverage"] { + const total = manifest.files.length; + + if (!parsed.coverage) { + return { + ok: false, + detail: "no `Files reviewed: n of N` line", + reviewed: null, + total, + unread: manifest.files.map((file) => file.path), + }; + } + + const { reviewed } = parsed.coverage; + const binary = new Set(manifest.files.filter((file) => file.binary).map((file) => file.path)); + // A binary file has no text to read: named with a reason, it does not count as unread. + const excused = parsed.notReviewed.filter((entry) => entry.reason && binary.has(entry.path)).length; + const ok = parsed.coverage.total === total && reviewed + excused >= total; + + if (ok) { + return { ok, detail: `${reviewed} of ${total} files reviewed`, reviewed, total, unread: [] }; + } + + const named = parsed.notReviewed.map((entry) => entry.path).filter((path) => !binary.has(path)); + const missing = total - reviewed - excused; + // The worker named fewer files than it skipped: the manifest files its report never mentions are the best guess. + const unmentioned = + named.length >= missing + ? [] + : manifest.files.map((file) => file.path).filter((path) => !named.includes(path) && !report.includes(path)); + const detail = + parsed.coverage.total !== total + ? `reported ${reviewed} of ${parsed.coverage.total}, but the manifest has ${total} files` + : `${reviewed} of ${total} files reviewed`; + + return { ok, detail, reviewed, total, unread: [...new Set([...named, ...unmentioned])] }; +} + +function checkShape(parsed: ParsedReviewReport): ReviewGateChecks["shape"] { + const malformed = parsed.findings.filter((finding) => !finding.ok).map((finding) => finding.index); + const problems: string[] = []; + + if (parsed.findings.length === 0 && !parsed.findingsNone) { + problems.push("no numbered findings and no `Findings: none` line"); + } + + if (malformed.length > 0) { + problems.push( + `findings ${malformed.map((index) => `#${index}`).join(", ")} lack severity, path:line, Scenario or Fix` + ); + } + + if (!parsed.ran) { + problems.push("no `Ran:` line"); + } + + return { + ok: problems.length === 0, + detail: problems.length === 0 ? `${parsed.findings.length} findings parse` : problems.join("; "), + findings: parsed.findings.length, + malformed, + }; +} + +/** The three depth checks of one review turn: coverage against the manifest, the reasoning floor, and finding shape. */ +export function checkReview(input: { + report: string; + manifest: ReviewManifest; + effort: ReviewEffort; + /** Reasoning tokens of every review turn of the session so far; null when the backend did not report them. */ + reasoningTokens: number | null; +}): ReviewGateChecks { + const parsed = parseReviewReport(input.report); + const floor = REASONING_FLOOR_PER_100_LINES[input.effort]; + const per100Lines = reasoningPer100Lines(input.reasoningTokens, input.manifest); + const reasoningOk = per100Lines === null || per100Lines >= floor; + + return { + coverage: checkCoverage(parsed, input.manifest, input.report), + reasoning: { + ok: reasoningOk, + detail: + per100Lines === null + ? "reasoning tokens unknown" + : `${per100Lines} reasoning tokens per 100 changed lines, ${input.effort} floor ${floor}`, + per100Lines, + floor, + }, + shape: checkShape(parsed), + }; +} + +export function nextReviewEffort(effort: ReviewEffort): ReviewEffort | null { + const index = REVIEW_EFFORTS.indexOf(effort); + return index >= 0 && index < REVIEW_EFFORTS.length - 1 ? REVIEW_EFFORTS[index + 1] : null; +} + +function failedDetails(checks: ReviewGateChecks): string[] { + return (["coverage", "reasoning", "shape"] as const) + .filter((key) => !checks[key].ok) + .map((key) => `${key}: ${checks[key].detail}`); +} + +/** The one steer the gate sends: the unread files by name, the reasoning gap, the findings that do not parse. */ +export function steerText(checks: ReviewGateChecks, effort: ReviewEffort): string { + const lines = ["The review gate found this review incomplete. Continue the same review now:"]; + const unread = checks.coverage.unread; + + if (!checks.coverage.ok && unread.length > 0) { + const named = unread.slice(0, STEER_FILE_LIMIT).join(", "); + const more = + unread.length > STEER_FILE_LIMIT ? `, and ${unread.length - STEER_FILE_LIMIT} more manifest files` : ""; + lines.push(`You did not read: ${named}${more}. Review them now with the same output shape.`); + } else if (!checks.coverage.ok) { + lines.push(`Coverage: ${checks.coverage.detail}. Account for every file of the manifest.`); + } + + if (!checks.reasoning.ok) { + lines.push( + `Depth: ${checks.reasoning.detail}. Go deeper at the ${effort} tier: open the callers and tests of the riskiest changes and probe each finding.` + ); + } + + if (!checks.shape.ok) { + lines.push( + `Shape: ${checks.shape.detail}. Write each finding as \`N. [Severity] path:line - defect. Scenario: ... Fix: ...\`.` + ); + } + + lines.push( + `End with the full findings list (earlier ones included), the Ran line, \`Files reviewed: of ${checks.coverage.total}\`, the Not reviewed line and the five report lines.` + ); + return lines.join("\n"); +} + +export interface GateDecision { + verdict: ReviewVerdict; + reason: string; + /** For `steered`: the text sent to the same session. */ + steer?: string; + /** For `escalated`: the tier of the new session. */ + nextEffort?: ReviewEffort; +} + +/** + * What the gate does after a review turn. At most two automatic steps per review chain: the first miss steers the + * same session (step 0), the second reruns one tier higher in a new session (step 1), and a miss after that, or a + * miss with no tier left, is `shallow`. A shallow tier the caller asked for (`--allow-shallow`) is never tuned up. + */ +export function decideGate( + checks: ReviewGateChecks, + config: Pick +): GateDecision { + const misses = failedDetails(checks); + + if (config.effort === "low") { + return { verdict: "shallow", reason: "low is triage only (--allow-shallow); the depth gate does not apply" }; + } + + if (misses.length === 0) { + return { + verdict: "pass", + reason: `${checks.coverage.detail}; ${checks.reasoning.detail}; ${checks.shape.detail}`, + }; + } + + const missed = misses.join("; "); + + if (config.effort === "medium" && config.allowShallow) { + return { verdict: "shallow", reason: `${missed} (medium with --allow-shallow: no automatic step)` }; + } + + if (config.step === 0) { + return { verdict: "steered", reason: missed, steer: steerText(checks, config.effort) }; + } + + const next = nextReviewEffort(config.effort); + + if (config.step === 1 && next) { + return { verdict: "escalated", reason: `${missed}; rerun at ${next}`, nextEffort: next }; + } + + return { + verdict: "shallow", + reason: `${missed}; ${config.step >= 2 ? "both automatic steps are spent" : `no tier above ${config.effort}`}`, + }; +} diff --git a/src/codex/lib/review/ledger.ts b/src/codex/lib/review/ledger.ts new file mode 100644 index 0000000000..8d5b5e30f9 --- /dev/null +++ b/src/codex/lib/review/ledger.ts @@ -0,0 +1,129 @@ +import { join } from "node:path"; +import { codexRoot } from "@genesiscz/utils/codex/worker-paths"; +import { readJsonlRows } from "@genesiscz/utils/jsonl"; +import { JsonlWriter } from "@genesiscz/utils/log-session/jsonl-writer"; +import { logger } from "@genesiscz/utils/logger"; +import { isObject } from "@genesiscz/utils/object"; +import { manifestChangedLines, type ReviewEffort } from "@genesiscz/utils/worker/contract"; +import type { CodexReviewConfig, CodexReviewSpent, ReviewVerdict } from "./types"; + +const log = logger.child({ component: "codex:review-ledger" }); + +/** `~/.genesis-tools/codex/reviews.jsonl`: one line per review start, review turn and gate decision. */ +export function reviewLedgerPath(): string { + return join(codexRoot(), "reviews.jsonl"); +} + +interface ReviewLedgerBase { + v: 1; + ts: string; + name: string; + chain: string; + label?: string; + pr?: number; + branch?: string; + base: string; + files: number; + lines: number; + effort: ReviewEffort; +} + +export interface ReviewStartRecord extends ReviewLedgerBase { + kind: "start"; + cwd: string; + effortSource: CodexReviewConfig["effortSource"]; + allowShallow: boolean; + parent?: string; +} + +export interface ReviewTurnRecord extends ReviewLedgerBase { + kind: "turn"; + turn: number; + outcome: "completed" | "failed" | "interrupted"; + model: string | null; + durationMs: number | null; + toolCalls: number; + reasoningTokens: number | null; + outputTokens: number | null; + costUsd: number | null; + /** The `Files reviewed: n of N` line of the turn's final message; null when it had none. */ + coverage: { reviewed: number; total: number } | null; +} + +export interface ReviewGateRecord extends ReviewLedgerBase { + kind: "gate"; + step: number; + verdict: ReviewVerdict; + reason: string; + coverage: { reviewed: number | null; total: number }; + reasoningPer100Lines: number | null; + floor: number; + findings: number; + malformedFindings: number; + /** The unread files the gate named, first 40. */ + unread: string[]; + escalatedTo?: string; + spent: CodexReviewSpent; +} + +export type ReviewLedgerRecord = ReviewStartRecord | ReviewTurnRecord | ReviewGateRecord; + +type Distribute = T extends ReviewLedgerRecord ? Omit : never; +export type ReviewLedgerEntry = Distribute; + +/** The fields every record of one review session repeats, so a single line answers the reviews filters. */ +export function reviewLedgerBase(input: { name: string; review: CodexReviewConfig; label?: string; at?: Date }) { + const { review } = input; + return { + v: 1, + ts: (input.at ?? new Date()).toISOString(), + name: input.name, + chain: review.chain, + ...(input.label ? { label: input.label } : {}), + ...(review.pr === undefined ? {} : { pr: review.pr }), + ...(review.branch ? { branch: review.branch } : {}), + base: review.base, + files: review.manifest.files.length, + lines: manifestChangedLines(review.manifest), + effort: review.effort, + } satisfies ReviewLedgerBase; +} + +/** + * Appends one record. Never throws: the ledger is a record of the review, and a failed write must not fail the + * review turn it describes. The failure is logged with the record's kind and session. + */ +export function appendReviewRecord( + base: ReturnType, + entry: ReviewLedgerEntry, + path = reviewLedgerPath() +): ReviewLedgerRecord | null { + const record: ReviewLedgerRecord = { ...base, ...entry }; + + try { + new JsonlWriter(path).append({ ...record }); + log.debug({ path, kind: record.kind, name: record.name }, "review ledger record written"); + return record; + } catch (err) { + log.warn({ err, path, kind: record.kind, name: record.name }, "review ledger write failed"); + return null; + } +} + +function isLedgerRecord(value: unknown): value is ReviewLedgerRecord { + return ( + isObject(value) && + value.v === 1 && + (value.kind === "start" || value.kind === "turn" || value.kind === "gate") && + typeof value.name === "string" && + typeof value.ts === "string" && + typeof value.base === "string" + ); +} + +/** Every well-formed record, oldest first; torn or foreign lines are counted in `skipped`. */ +export function readReviewRecords(path = reviewLedgerPath()): { records: ReviewLedgerRecord[]; skipped: number } { + const { rows, skipped } = readJsonlRows(path); + const records = rows.filter(isLedgerRecord); + return { records, skipped: skipped + rows.length - records.length }; +} diff --git a/src/codex/lib/review/prepare.ts b/src/codex/lib/review/prepare.ts new file mode 100644 index 0000000000..4c070f3974 --- /dev/null +++ b/src/codex/lib/review/prepare.ts @@ -0,0 +1,309 @@ +import { toolCommand } from "@genesiscz/utils/cli/tool-command"; +import { createGit, getCurrentBranch, type NumstatEntry, originDriver, type PrInfo } from "@genesiscz/utils/git"; +import { logger } from "@genesiscz/utils/logger"; +import { + isReviewEffort, + manifestChangedLines, + REVIEW_EFFORTS, + type ReviewEffort, + type ReviewManifest, +} from "@genesiscz/utils/worker/contract"; +import type { CodexReviewConfig, ReviewBaseSource } from "./types"; + +const log = logger.child({ component: "codex:review" }); + +/** A diff up to this size gets `high` by default; anything larger gets `xhigh`. */ +export const HIGH_TIER_MAX = { files: 15, lines: 800 } as const; +/** Above either bound one reviewer is too thin, and the spawn proposes a split by folder. */ +export const SPLIT_THRESHOLD = { files: 60, lines: 4000 } as const; + +/** Everything the review preparation reads from git and the hosting service; injected so tests never spawn. */ +export interface ReviewGitReaders { + currentBranch(cwd: string): Promise; + /** `origin/` when it exists, else the local branch, else null. */ + resolveBranch(cwd: string, name: string): Promise; + refExists(cwd: string, ref: string): Promise; + /** The newest PR of the branch; null when there is none or the host cannot answer. */ + pullRequest(cwd: string, branch: string): Promise; + upstream(cwd: string, branch: string): Promise; + /** `git diff --numstat ...HEAD`, limited to `paths` when given. */ + numstat(cwd: string, base: string, paths?: string[]): Promise; +} + +export function defaultReviewGitReaders(): ReviewGitReaders { + return { + currentBranch: (cwd) => getCurrentBranch(cwd), + async resolveBranch(cwd, name) { + const git = createGit({ cwd }); + const short = name.replace(/^origin\//, ""); + + if (await git.refExists(`refs/remotes/origin/${short}`)) { + return `origin/${short}`; + } + + return (await git.refExists(`refs/heads/${short}`)) ? short : null; + }, + refExists: (cwd, ref) => createGit({ cwd }).refExists(`${ref}^{commit}`), + async pullRequest(cwd, branch) { + const driver = await originDriver(cwd); + + if (!driver) { + return null; + } + + const lookup = await driver.prForHead(branch); + + if (lookup.error) { + log.debug({ branch, error: lookup.error }, "review: PR lookup failed"); + } + + return lookup.pr; + }, + upstream: (cwd, branch) => createGit({ cwd }).getTrackingBranch(branch), + async numstat(cwd, base, paths) { + const git = createGit({ cwd }); + const mergeBase = await git.mergeBase(base, "HEAD"); + return git.numstat({ from: mergeBase, to: "HEAD", ...(paths?.length ? { paths } : {}) }); + }, + }; +} + +export interface PrepareReviewInput { + cwd: string; + base?: string; + effort?: string; + allowShallow?: boolean; + /** Limit the manifest to these paths: one slice of a split review. */ + paths?: string[]; + readers?: ReviewGitReaders; +} + +export interface PreparedReview { + /** The session's review record, without the chain fields the spawn adds. */ + config: Omit; + /** Lines the spawn prints as information: which base and why, which tier and why. */ + notes: string[]; + /** Lines the spawn prints as warnings: the split suggestion. */ + warnings: string[]; +} + +interface ChosenBase { + ref: string; + source: ReviewBaseSource; + detail: string; + pr?: PrInfo; +} + +async function chooseBase(input: PrepareReviewInput & { readers: ReviewGitReaders; branch: string | null }) { + const { readers, cwd, branch } = input; + + if (input.base) { + if (!(await readers.refExists(cwd, input.base))) { + throw new Error( + `--base ${input.base} does not resolve in ${cwd}; run git fetch first or pick another ref.` + ); + } + + const pr = branch ? await readers.pullRequest(cwd, branch) : null; + return { ref: input.base, source: "flag", detail: "--base", ...(pr ? { pr } : {}) } satisfies ChosenBase; + } + + if (!branch) { + throw new Error("--mode review needs --base : HEAD is detached, so there is no PR or upstream to read."); + } + + const pr = await readers.pullRequest(cwd, branch); + + if (pr) { + const ref = await readers.resolveBranch(cwd, pr.target); + + if (ref) { + return { + ref, + source: "pr", + detail: `${pr.state} PR #${pr.number} targets ${pr.target}`, + pr, + } satisfies ChosenBase; + } + + log.debug({ branch, pr }, "review: PR target does not resolve locally"); + } + + const upstream = await readers.upstream(cwd, branch); + + // A branch tracking its own remote copy has no base there: the diff against it is the unpushed commits only. + if (upstream && upstream !== `origin/${branch}` && upstream !== branch) { + return { ref: upstream, source: "upstream", detail: `upstream of ${branch}` } satisfies ChosenBase; + } + + throw new Error( + `--mode review needs --base : ${branch} has no PR whose target resolves and no upstream other than itself. \`${toolCommand("git base")}\` names a likely base.` + ); +} + +export function buildManifest(base: string, entries: readonly NumstatEntry[]): ReviewManifest { + const files = entries.map((entry) => ({ + path: entry.path, + added: entry.insertions, + deleted: entry.deletions, + ...(entry.binary ? { binary: true } : {}), + })); + + return { + base, + files, + added: files.reduce((sum, file) => sum + file.added, 0), + deleted: files.reduce((sum, file) => sum + file.deleted, 0), + }; +} + +/** `high` for a diff of up to 15 files and 800 changed lines, `xhigh` above that. */ +export function defaultReviewEffort(manifest: ReviewManifest): ReviewEffort { + const small = manifest.files.length <= HIGH_TIER_MAX.files && manifestChangedLines(manifest) <= HIGH_TIER_MAX.lines; + return small ? "high" : "xhigh"; +} + +function chooseEffort( + flag: string | undefined, + allowShallow: boolean, + manifest: ReviewManifest +): { effort: ReviewEffort; source: "flag" | "diff-size" } { + if (flag === undefined) { + return { effort: defaultReviewEffort(manifest), source: "diff-size" }; + } + + if (!isReviewEffort(flag)) { + throw new Error(`--effort for a review must be one of ${REVIEW_EFFORTS.join(", ")}; got ${flag}.`); + } + + if ((flag === "low" || flag === "medium") && !allowShallow) { + throw new Error( + `--effort ${flag} is refused for a review: medium reviews spent 358 to 2,399 reasoning tokens whatever the diff size and missed defects the xhigh pass found. Omit --effort to pick high or xhigh from the diff size, or pass --allow-shallow to accept a shallow review.` + ); + } + + return { effort: flag, source: "flag" }; +} + +export interface ManifestSlice { + folder: string; + files: number; + added: number; + deleted: number; +} + +function folderOf(path: string, depth: number): string { + const parts = path.split("/"); + return parts.length > depth ? parts.slice(0, depth).join("/") : parts.slice(0, -1).join("/") || "."; +} + +/** + * One slice per folder, split deeper until each slice is at or under the file threshold or cannot split + * further. A repo with everything under `src/` would otherwise get one useless `src` slice. + */ +export function splitManifest(manifest: ReviewManifest, depth = 1): ManifestSlice[] { + const groups = new Map(); + + for (const file of manifest.files) { + const folder = folderOf(file.path, depth); + groups.set(folder, [...(groups.get(folder) ?? []), file]); + } + + return [...groups.entries()] + .flatMap(([folder, files]) => { + const deeper = files.some((file) => file.path.split("/").length > depth + 1); + + if (files.length > SPLIT_THRESHOLD.files && deeper) { + const sub = splitManifest({ ...manifest, files }, depth + 1); + + if (sub.length > 1) { + return sub; + } + } + + return [ + { + folder, + files: files.length, + added: files.reduce((sum, file) => sum + file.added, 0), + deleted: files.reduce((sum, file) => sum + file.deleted, 0), + }, + ]; + }) + .sort((a, b) => b.files - a.files || a.folder.localeCompare(b.folder)); +} + +export function splitSuggestion(manifest: ReviewManifest): string | null { + const lines = manifestChangedLines(manifest); + + if (manifest.files.length <= SPLIT_THRESHOLD.files && lines <= SPLIT_THRESHOLD.lines) { + return null; + } + + // A diff under one folder (`src/`) only splits a level or two down. + let depth = 1; + let slices = splitManifest(manifest, depth); + + while (slices.length < 2 && depth < 4) { + depth += 1; + slices = splitManifest(manifest, depth); + } + + if (slices.length < 2) { + return null; + } + + const listed = slices + .slice(0, 12) + .map((slice) => `${slice.folder} (${slice.files} files, +${slice.added} -${slice.deleted})`) + .join(", "); + const more = slices.length > 12 ? `, +${slices.length - 12} more` : ""; + + return `${manifest.files.length} files, ${lines} changed lines: above about ${SPLIT_THRESHOLD.files} files or ${SPLIT_THRESHOLD.lines} lines one reviewer covers the diff thinly. Split suggestion, one reviewer per folder at xhigh, in parallel: ${listed}${more}. Each one: ${toolCommand("codex spawn")} --mode review --base ${manifest.base} --paths --name -.`; +} + +/** + * Everything `spawn --mode review` decides before a worker starts: the base (the flag, else the branch's PR + * target, else its upstream, and the notes say which), the manifest from `git diff --numstat ...HEAD`, + * the tier (the flag, else the diff size) and the split suggestion for a large diff. + */ +export async function prepareReview(input: PrepareReviewInput): Promise { + const readers = input.readers ?? defaultReviewGitReaders(); + const allowShallow = input.allowShallow === true; + const branch = await readers.currentBranch(input.cwd); + const base = await chooseBase({ ...input, readers, branch }); + const manifest = buildManifest(base.ref, await readers.numstat(input.cwd, base.ref, input.paths)); + + if (manifest.files.length === 0) { + throw new Error( + `Nothing to review: git diff --numstat ${base.ref}...HEAD is empty in ${input.cwd}${input.paths?.length ? ` for ${input.paths.join(", ")}` : ""}.` + ); + } + + const { effort, source } = chooseEffort(input.effort, allowShallow, manifest); + const warnings = [splitSuggestion(manifest)].filter((line): line is string => line !== null); + const notes = [ + `review base ${base.ref} (${base.source}: ${base.detail})`, + `review manifest ${manifest.files.length} files, +${manifest.added} -${manifest.deleted}; effort ${effort} (${source === "flag" ? "--effort" : "from the diff size"})`, + ]; + log.debug( + { cwd: input.cwd, base: base.ref, source: base.source, files: manifest.files.length, effort }, + "review prepared" + ); + + return { + config: { + base: base.ref, + baseSource: base.source, + baseDetail: base.detail, + ...(branch ? { branch } : {}), + ...(base.pr ? { pr: base.pr.number } : {}), + manifest, + effort, + effortSource: source, + allowShallow, + }, + notes, + warnings, + }; +} diff --git a/src/codex/lib/review/report.ts b/src/codex/lib/review/report.ts new file mode 100644 index 0000000000..4ed8837a57 --- /dev/null +++ b/src/codex/lib/review/report.ts @@ -0,0 +1,194 @@ +import { parseSince } from "@genesiscz/utils/date"; +import { parseDuration } from "@genesiscz/utils/format"; +import type { ReviewEffort } from "@genesiscz/utils/worker/contract"; +import type { ReviewLedgerRecord } from "./ledger"; +import type { ReviewVerdict } from "./types"; + +/** One review session, folded from its ledger lines: what `tools codex reviews` prints as a row. */ +export interface ReviewSummary { + name: string; + chain: string; + label?: string; + pr?: number; + branch?: string; + base: string; + files: number; + lines: number; + effort: ReviewEffort; + startedAt: string; + lastAt: string; + turns: number; + durationMs: number; + toolCalls: number; + reasoningTokens: number; + outputTokens: number; + /** API-equivalent dollars; null when any turn was unpriced. */ + costUsd: number | null; + reasoningPer100Lines: number | null; + /** The last `Files reviewed` line: `12/14`. */ + coverage: string | null; + /** The last gate verdict; `open` while no gate line exists yet (running, or a turn that failed). */ + verdict: ReviewVerdict | "open"; + reason?: string; + escalatedTo?: string; + parent?: string; +} + +export interface ReviewFilters { + since?: Date; + pr?: number; + label?: string; + effort?: string; + verdict?: string; + name?: string; +} + +export interface ReviewTotals { + reviews: number; + turns: number; + durationMs: number; + toolCalls: number; + reasoningTokens: number; + costUsd: number; + unpricedReviews: number; + verdicts: Record; +} + +function emptySummary(record: ReviewLedgerRecord): ReviewSummary { + return { + name: record.name, + chain: record.chain, + ...(record.label ? { label: record.label } : {}), + ...(record.pr === undefined ? {} : { pr: record.pr }), + ...(record.branch ? { branch: record.branch } : {}), + base: record.base, + files: record.files, + lines: record.lines, + effort: record.effort, + startedAt: record.ts, + lastAt: record.ts, + turns: 0, + durationMs: 0, + toolCalls: 0, + reasoningTokens: 0, + outputTokens: 0, + costUsd: 0, + reasoningPer100Lines: null, + coverage: null, + verdict: "open", + }; +} + +/** + * Folds the ledger into one summary per review session. A `start` line opens a new review, so a name that was + * reused later shows twice; turn and gate lines without a start (a ledger cut by hand) open one too. + */ +export function summarizeReviews(records: readonly ReviewLedgerRecord[]): ReviewSummary[] { + const open = new Map(); + const all: ReviewSummary[] = []; + + for (const record of records) { + let summary = open.get(record.name); + + if (!summary || record.kind === "start") { + summary = emptySummary(record); + open.set(record.name, summary); + all.push(summary); + } + + summary.lastAt = record.ts; + + if (record.kind === "start") { + if (record.parent) { + summary.parent = record.parent; + } + + continue; + } + + if (record.kind === "turn") { + summary.turns += 1; + summary.durationMs += record.durationMs ?? 0; + summary.toolCalls += record.toolCalls; + summary.reasoningTokens += record.reasoningTokens ?? 0; + summary.outputTokens += record.outputTokens ?? 0; + summary.costUsd = + summary.costUsd === null || record.costUsd === null ? null : summary.costUsd + record.costUsd; + summary.effort = record.effort; + + if (record.coverage) { + summary.coverage = `${record.coverage.reviewed}/${record.coverage.total}`; + } + + summary.reasoningPer100Lines = Math.round((summary.reasoningTokens * 100) / Math.max(1, summary.lines)); + continue; + } + + summary.verdict = record.verdict; + summary.reason = record.reason; + + if (record.escalatedTo) { + summary.escalatedTo = record.escalatedTo; + } else { + delete summary.escalatedTo; + } + } + + return all; +} + +/** `24h`, `90m`, `7d` back from now; otherwise the `--since` grammar of the other reports (`HH:MM`, a date). */ +export function parseReviewSince(value: string, now = new Date()): Date | null { + const text = value.trim(); + + if (/^\d+\s*[a-z]/i.test(text) && !/^\d{4}-/.test(text)) { + const ms = parseDuration(text); + return ms > 0 ? new Date(now.getTime() - ms) : null; + } + + return parseSince(text, now); +} + +export function filterReviews(summaries: readonly ReviewSummary[], filters: ReviewFilters): ReviewSummary[] { + const label = filters.label?.toLowerCase(); + const name = filters.name?.toLowerCase(); + + return summaries.filter( + (summary) => + (!filters.since || Date.parse(summary.lastAt) >= filters.since.getTime()) && + (filters.pr === undefined || summary.pr === filters.pr) && + (!label || (summary.label ?? "").toLowerCase().includes(label)) && + (!filters.effort || summary.effort === filters.effort) && + (!filters.verdict || summary.verdict === filters.verdict) && + (!name || summary.name.toLowerCase().includes(name) || summary.chain.toLowerCase().includes(name)) + ); +} + +export function reviewTotals(summaries: readonly ReviewSummary[]): ReviewTotals { + const totals: ReviewTotals = { + reviews: summaries.length, + turns: 0, + durationMs: 0, + toolCalls: 0, + reasoningTokens: 0, + costUsd: 0, + unpricedReviews: 0, + verdicts: {}, + }; + + for (const summary of summaries) { + totals.turns += summary.turns; + totals.durationMs += summary.durationMs; + totals.toolCalls += summary.toolCalls; + totals.reasoningTokens += summary.reasoningTokens; + totals.verdicts[summary.verdict] = (totals.verdicts[summary.verdict] ?? 0) + 1; + + if (summary.costUsd === null) { + totals.unpricedReviews += 1; + } else { + totals.costUsd += summary.costUsd; + } + } + + return totals; +} diff --git a/src/codex/lib/review/review.test.ts b/src/codex/lib/review/review.test.ts new file mode 100644 index 0000000000..c30a91404a --- /dev/null +++ b/src/codex/lib/review/review.test.ts @@ -0,0 +1,568 @@ +import { describe, expect, test } from "bun:test"; +import { appendFileSync, mkdtempSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { CodexFinishedTurn } from "@genesiscz/utils/codex/turn-cost"; +import type { NumstatEntry, PrInfo } from "@genesiscz/utils/git"; +import { SafeJSON } from "@genesiscz/utils/json"; +import type { ReviewManifest } from "@genesiscz/utils/worker/contract"; +import { codexDriver } from "../driver"; +import { spawnCodexSession } from "../spawn"; +import { escalatedReviewName, markReviewShallow, recordReviewTurn } from "./controller"; +import { checkReview, decideGate, parseReviewReport } from "./gate"; +import { type ReviewLedgerRecord, readReviewRecords } from "./ledger"; +import { prepareReview, type ReviewGitReaders } from "./prepare"; +import { filterReviews, parseReviewSince, reviewTotals, summarizeReviews } from "./report"; +import type { CodexReviewConfig } from "./types"; + +function entry(path: string, insertions: number, deletions = 0, binary = false): NumstatEntry { + return { path, insertions, deletions, binary }; +} + +function readers(options: { + branch?: string | null; + pr?: PrInfo | null; + upstream?: string | null; + numstat?: NumstatEntry[]; + refs?: string[]; +}): ReviewGitReaders & { calls: string[] } { + const calls: string[] = []; + return { + calls, + currentBranch: async () => (options.branch === undefined ? "feat/x" : options.branch), + resolveBranch: async (_cwd, name) => + (options.refs ?? ["origin/master"]).includes(`origin/${name}`) ? `origin/${name}` : null, + refExists: async (_cwd, ref) => (options.refs ?? ["origin/master"]).includes(ref), + pullRequest: async () => options.pr ?? null, + upstream: async () => options.upstream ?? null, + numstat: async (_cwd, base, paths) => { + calls.push(`numstat ${base}${paths ? ` ${paths.join(",")}` : ""}`); + return options.numstat ?? [entry("src/a.ts", 40, 2)]; + }, + }; +} + +const PR: PrInfo = { number: 12, state: "OPEN", target: "master", url: "https://example.com/pr/12" }; + +describe("prepareReview", () => { + test("--base is verified and used; the PR number is still recorded", async () => { + const fake = readers({ pr: PR }); + const prepared = await prepareReview({ cwd: "/repo", base: "origin/master", readers: fake }); + + expect(prepared.config).toMatchObject({ base: "origin/master", baseSource: "flag", pr: 12, branch: "feat/x" }); + expect(prepared.notes[0]).toBe("review base origin/master (flag: --base)"); + expect(fake.calls).toEqual(["numstat origin/master"]); + await expect(prepareReview({ cwd: "/repo", base: "origin/nope", readers: fake })).rejects.toThrow( + "--base origin/nope does not resolve" + ); + }); + + test("without --base: the PR target first, then an upstream that is not the branch itself", async () => { + const fromPr = await prepareReview({ cwd: "/repo", readers: readers({ pr: PR }) }); + expect(fromPr.config).toMatchObject({ base: "origin/master", baseSource: "pr", pr: 12 }); + expect(fromPr.notes[0]).toBe("review base origin/master (pr: OPEN PR #12 targets master)"); + + const fromUpstream = await prepareReview({ cwd: "/repo", readers: readers({ upstream: "origin/develop" }) }); + expect(fromUpstream.config).toMatchObject({ base: "origin/develop", baseSource: "upstream" }); + + await expect(prepareReview({ cwd: "/repo", readers: readers({ upstream: "origin/feat/x" }) })).rejects.toThrow( + "--mode review needs --base : feat/x has no PR" + ); + await expect(prepareReview({ cwd: "/repo", readers: readers({ branch: null }) })).rejects.toThrow( + "HEAD is detached" + ); + }); + + test("an empty diff is refused; --paths reaches the numstat", async () => { + await expect( + prepareReview({ cwd: "/repo", base: "origin/master", readers: readers({ numstat: [] }) }) + ).rejects.toThrow("Nothing to review"); + + const fake = readers({}); + await prepareReview({ cwd: "/repo", base: "origin/master", paths: ["src/codex"], readers: fake }); + expect(fake.calls).toEqual(["numstat origin/master src/codex"]); + }); + + test("the tier comes from the diff size; low and medium need --allow-shallow", async () => { + const small = await prepareReview({ cwd: "/repo", base: "origin/master", readers: readers({}) }); + expect(small.config).toMatchObject({ effort: "high", effortSource: "diff-size" }); + + const many = Array.from({ length: 16 }, (_, i) => entry(`src/f${i}.ts`, 10)); + const large = await prepareReview({ cwd: "/repo", base: "origin/master", readers: readers({ numstat: many }) }); + expect(large.config.effort).toBe("xhigh"); + + const long = await prepareReview({ + cwd: "/repo", + base: "origin/master", + readers: readers({ numstat: [entry("src/a.ts", 801)] }), + }); + expect(long.config.effort).toBe("xhigh"); + + const base = { cwd: "/repo", base: "origin/master", readers: readers({}) }; + await expect(prepareReview({ ...base, effort: "medium" })).rejects.toThrow("--effort medium is refused"); + await expect(prepareReview({ ...base, effort: "low" })).rejects.toThrow("--allow-shallow"); + await expect(prepareReview({ ...base, effort: "huge" })).rejects.toThrow("must be one of low, medium"); + expect((await prepareReview({ ...base, effort: "medium", allowShallow: true })).config).toMatchObject({ + effort: "medium", + effortSource: "flag", + allowShallow: true, + }); + expect((await prepareReview({ ...base, effort: "max" })).config.effort).toBe("max"); + }); + + test("above 60 files the spawn proposes one reviewer per folder, split below a shared src/", async () => { + const files = [ + ...Array.from({ length: 40 }, (_, i) => entry(`src/codex/f${i}.ts`, 5)), + ...Array.from({ length: 30 }, (_, i) => entry(`src/utils/g${i}.ts`, 5)), + ]; + const prepared = await prepareReview({ + cwd: "/repo", + base: "origin/master", + readers: readers({ numstat: files }), + }); + + expect(prepared.warnings).toHaveLength(1); + expect(prepared.warnings[0]).toContain("src/codex (40 files, +200 -0), src/utils (30 files, +150 -0)"); + expect(prepared.warnings[0]).toContain("--paths "); + + const smallDiff = await prepareReview({ cwd: "/repo", base: "origin/master", readers: readers({}) }); + expect(smallDiff.warnings).toEqual([]); + }); +}); + +const MANIFEST: ReviewManifest = { + base: "origin/master", + files: [ + { path: "src/a.ts", added: 60, deleted: 10 }, + { path: "src/b.ts", added: 20, deleted: 0 }, + { path: "src/c.ts", added: 10, deleted: 0 }, + { path: "assets/logo.png", added: 0, deleted: 0, binary: true }, + ], + added: 90, + deleted: 10, +}; + +const GOOD_REPORT = [ + "1. **[High]** src/a.ts:12 - the lock is never released. Scenario: a throw inside the callback. Fix: release in finally.", + "2. [Low] src/b.ts:3 - stale comment.", + " Scenario: a reader trusts it. Fix: delete it.", + "Ran: bun test src/a.test.ts", + "Files reviewed: 3 of 4", + "Not reviewed: assets/logo.png (binary image)", + "RESULT: done", +].join("\n"); + +describe("the depth gate", () => { + test("parses the review shape, markdown and wrapped findings included", () => { + const parsed = parseReviewReport(GOOD_REPORT); + + expect(parsed.coverage).toEqual({ reviewed: 3, total: 4 }); + expect(parsed.notReviewed).toEqual([{ path: "assets/logo.png", reason: "binary image" }]); + expect(parsed.findings.map((finding) => [finding.index, finding.ok])).toEqual([ + [1, true], + [2, true], + ]); + expect(parsed.ran).toBe(true); + }); + + test("passes a full review: a named binary file with a reason is excused", () => { + const checks = checkReview({ report: GOOD_REPORT, manifest: MANIFEST, effort: "high", reasoningTokens: 400 }); + + expect(checks.coverage).toMatchObject({ ok: true, reviewed: 3, total: 4, unread: [] }); + expect(checks.reasoning).toMatchObject({ ok: true, per100Lines: 400, floor: 100 }); + expect(checks.shape.ok).toBe(true); + expect(decideGate(checks, { effort: "high", step: 0, allowShallow: false }).verdict).toBe("pass"); + }); + + test("names the unread files: the Not reviewed line, else the manifest files the report never mentions", () => { + const named = checkReview({ + report: GOOD_REPORT.replace("Files reviewed: 3 of 4", "Files reviewed: 2 of 4").replace( + "Not reviewed: assets/logo.png (binary image)", + "Not reviewed: src/c.ts (ran out of time), assets/logo.png (binary image)" + ), + manifest: MANIFEST, + effort: "high", + reasoningTokens: 400, + }); + expect(named.coverage).toMatchObject({ ok: false, unread: ["src/c.ts"] }); + + const silent = checkReview({ + report: GOOD_REPORT.replace("Files reviewed: 3 of 4", "Files reviewed: 2 of 4").replace( + "Not reviewed: assets/logo.png (binary image)", + "Not reviewed: none" + ), + manifest: MANIFEST, + effort: "high", + reasoningTokens: 400, + }); + expect(silent.coverage.unread).toEqual(["src/c.ts", "assets/logo.png"]); + + const missing = checkReview({ report: "1. nothing", manifest: MANIFEST, effort: "high", reasoningTokens: 400 }); + expect(missing.coverage).toMatchObject({ + ok: false, + reviewed: null, + detail: "no `Files reviewed: n of N` line", + }); + expect(missing.shape).toMatchObject({ ok: false, malformed: [1] }); + }); + + test("holds each tier to its reasoning floor per 100 changed lines", () => { + const thin = checkReview({ report: GOOD_REPORT, manifest: MANIFEST, effort: "xhigh", reasoningTokens: 200 }); + expect(thin.reasoning).toMatchObject({ ok: false, per100Lines: 200, floor: 250 }); + expect( + checkReview({ report: GOOD_REPORT, manifest: MANIFEST, effort: "high", reasoningTokens: 99 }).reasoning.ok + ).toBe(false); + expect( + checkReview({ report: GOOD_REPORT, manifest: MANIFEST, effort: "high", reasoningTokens: null }).reasoning + ).toMatchObject({ ok: true, detail: "reasoning tokens unknown" }); + }); + + test("steers once, escalates once, then ends shallow; --allow-shallow tiers are never tuned up", () => { + const miss = checkReview({ + report: "Ran: nothing\nFindings: none", + manifest: MANIFEST, + effort: "high", + reasoningTokens: 10, + }); + + const first = decideGate(miss, { effort: "high", step: 0, allowShallow: false }); + expect(first.verdict).toBe("steered"); + expect(first.steer).toContain("You did not read: src/a.ts, src/b.ts, src/c.ts, assets/logo.png."); + expect(first.steer).toContain("Depth: 10 reasoning tokens per 100 changed lines, high floor 100."); + expect(first.steer).toContain("`Files reviewed: of 4`"); + + expect(decideGate(miss, { effort: "high", step: 1, allowShallow: false })).toMatchObject({ + verdict: "escalated", + nextEffort: "xhigh", + }); + expect(decideGate(miss, { effort: "max", step: 1, allowShallow: false }).reason).toContain("no tier above max"); + expect(decideGate(miss, { effort: "xhigh", step: 2, allowShallow: false })).toMatchObject({ + verdict: "shallow", + }); + expect(decideGate(miss, { effort: "medium", step: 0, allowShallow: true }).verdict).toBe("shallow"); + expect(decideGate(miss, { effort: "low", step: 0, allowShallow: true }).verdict).toBe("shallow"); + }); +}); + +function finishedTurn(over: Partial = {}): CodexFinishedTurn { + return { + turn: 1, + outcome: "completed", + startedAt: new Date("2026-10-22T10:00:00Z"), + endedAt: new Date("2026-10-22T10:05:00Z"), + durationMs: 300_000, + parts: [{ model: "gpt-x", tokens: { input: 900, cacheRead: 0, cacheWrite: 0, output: 600, reasoning: 50 } }], + toolCalls: 12, + model: "gpt-x", + auth: "subscription", + ...over, + }; +} + +function reviewConfig(over: Partial = {}): CodexReviewConfig { + return { + base: "origin/master", + baseSource: "pr", + baseDetail: "OPEN PR #12 targets master", + branch: "feat/x", + pr: 12, + manifest: MANIFEST, + effort: "high", + effortSource: "diff-size", + allowShallow: false, + step: 0, + chain: "rv", + ...over, + }; +} + +describe("recordReviewTurn", () => { + test("a miss steers the same session and logs a turn line and a gate line", () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-ledger-")), "reviews.jsonl"); + const outcome = recordReviewTurn({ + name: "rv", + label: "babysit pr12", + review: reviewConfig(), + finished: finishedTurn(), + report: "Ran: nothing\nFindings: none\nFiles reviewed: 1 of 4\nNot reviewed: none", + costUsd: 0.4, + ledgerPath, + }); + + expect(outcome.action.kind).toBe("steer"); + expect(outcome.review).toMatchObject({ verdict: "steered", step: 1 }); + expect(outcome.review.spent).toEqual({ + turns: 1, + durationMs: 300_000, + toolCalls: 12, + reasoningTokens: 50, + outputTokens: 600, + costUsd: 0.4, + }); + + const { records } = readReviewRecords(ledgerPath); + expect(records.map((record) => record.kind)).toEqual(["turn", "gate"]); + expect(records[0]).toMatchObject({ + name: "rv", + label: "babysit pr12", + pr: 12, + base: "origin/master", + files: 4, + lines: 100, + effort: "high", + coverage: { reviewed: 1, total: 4 }, + reasoningTokens: 50, + }); + expect(records[1]).toMatchObject({ verdict: "steered", step: 1, reasoningPer100Lines: 50, floor: 100 }); + + const second = recordReviewTurn({ + name: "rv", + review: outcome.review, + finished: finishedTurn({ turn: 2 }), + report: "Ran: nothing\nFindings: none\nFiles reviewed: 2 of 4\nNot reviewed: none", + costUsd: 0.2, + ledgerPath, + }); + expect(second.action).toEqual({ + kind: "escalate", + effort: "xhigh", + name: escalatedReviewName({ chain: "rv" }, "xhigh"), + }); + expect(second.review).toMatchObject({ verdict: "escalated", step: 2, escalatedTo: "rv-xhigh" }); + expect(second.review.spent?.reasoningTokens).toBe(100); + }); + + test("a failed turn and a follow-up after a final verdict are booked without the gate", () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-ledger-")), "reviews.jsonl"); + const failed = recordReviewTurn({ + name: "rv", + review: reviewConfig(), + finished: finishedTurn({ outcome: "failed" }), + report: "", + costUsd: null, + ledgerPath, + }); + expect(failed).toMatchObject({ action: { kind: "none" } }); + expect(failed.decision).toBeUndefined(); + expect(failed.review.spent?.costUsd).toBeNull(); + + const followUp = recordReviewTurn({ + name: "rv", + review: reviewConfig({ verdict: "pass" }), + finished: finishedTurn(), + report: "", + costUsd: 0.1, + ledgerPath, + }); + expect(followUp.decision).toBeUndefined(); + expect(readReviewRecords(ledgerPath).records.map((record) => record.kind)).toEqual(["turn", "turn"]); + + const shallow = markReviewShallow({ + name: "rv", + review: reviewConfig({ verdict: "steered" }), + reason: "steer failed", + ledgerPath, + }); + expect(shallow.verdict).toBe("shallow"); + expect(readReviewRecords(ledgerPath).records.at(-1)).toMatchObject({ + kind: "gate", + verdict: "shallow", + reason: "steer failed", + }); + }); +}); + +function ledgerFixture(): ReviewLedgerRecord[] { + const base = { + v: 1 as const, + chain: "rv", + base: "origin/master", + files: 4, + lines: 100, + pr: 12, + label: "babysit pr12", + }; + const spent = { + turns: 2, + durationMs: 600_000, + toolCalls: 30, + reasoningTokens: 600, + outputTokens: 900, + costUsd: 0.9, + }; + return [ + { + ...base, + kind: "start", + ts: "2026-10-22T10:00:00Z", + name: "rv", + effort: "high", + cwd: "/repo", + effortSource: "diff-size", + allowShallow: false, + }, + { + ...base, + kind: "turn", + ts: "2026-10-22T10:05:00Z", + name: "rv", + effort: "high", + turn: 1, + outcome: "completed", + model: "gpt-x", + durationMs: 300_000, + toolCalls: 10, + reasoningTokens: 50, + outputTokens: 300, + costUsd: 0.4, + coverage: { reviewed: 1, total: 4 }, + }, + { + ...base, + kind: "gate", + ts: "2026-10-22T10:05:01Z", + name: "rv", + effort: "high", + step: 1, + verdict: "escalated", + reason: "coverage: 1 of 4", + coverage: { reviewed: 1, total: 4 }, + reasoningPer100Lines: 50, + floor: 100, + findings: 0, + malformedFindings: 0, + unread: ["src/b.ts"], + escalatedTo: "rv-xhigh", + spent, + }, + { + ...base, + kind: "start", + ts: "2026-10-22T10:06:00Z", + name: "rv-xhigh", + effort: "xhigh", + cwd: "/repo", + effortSource: "flag", + allowShallow: false, + parent: "rv", + }, + { + ...base, + kind: "turn", + ts: "2026-10-22T10:20:00Z", + name: "rv-xhigh", + effort: "xhigh", + turn: 1, + outcome: "completed", + model: "gpt-x", + durationMs: 840_000, + toolCalls: 60, + reasoningTokens: 3000, + outputTokens: 900, + costUsd: 1.5, + coverage: { reviewed: 4, total: 4 }, + }, + { + ...base, + kind: "gate", + ts: "2026-10-22T10:20:01Z", + name: "rv-xhigh", + effort: "xhigh", + step: 2, + verdict: "pass", + reason: "4 of 4 files reviewed", + coverage: { reviewed: 4, total: 4 }, + reasoningPer100Lines: 3000, + floor: 250, + findings: 3, + malformedFindings: 0, + unread: [], + spent, + }, + ]; +} + +describe("the review ledger report", () => { + test("folds one row per review session, with the escalation linked both ways", () => { + const reviews = summarizeReviews(ledgerFixture()); + + expect(reviews.map((review) => [review.name, review.verdict, review.coverage, review.turns])).toEqual([ + ["rv", "escalated", "1/4", 1], + ["rv-xhigh", "pass", "4/4", 1], + ]); + expect(reviews[0]).toMatchObject({ escalatedTo: "rv-xhigh", reasoningPer100Lines: 50, costUsd: 0.4, pr: 12 }); + expect(reviews[1]).toMatchObject({ parent: "rv", effort: "xhigh", reasoningPer100Lines: 3000, toolCalls: 60 }); + expect(reviewTotals(reviews)).toMatchObject({ + reviews: 2, + turns: 2, + toolCalls: 70, + costUsd: 1.9, + verdicts: { escalated: 1, pass: 1 }, + }); + }); + + test("a reused session name starts a new review row at its start line", () => { + const [start, turn] = ledgerFixture(); + const again = { ...start, ts: "2026-10-23T09:00:00Z" }; + const reviews = summarizeReviews([start, turn, again]); + + expect(reviews.map((review) => [review.name, review.turns, review.startedAt])).toEqual([ + ["rv", 1, "2026-10-22T10:00:00Z"], + ["rv", 0, "2026-10-23T09:00:00Z"], + ]); + }); + + test("filters by PR, label, tier, verdict, name and time", () => { + const reviews = summarizeReviews(ledgerFixture()); + const names = (filters: Parameters[1]) => + filterReviews(reviews, filters).map((r) => r.name); + + expect(names({ pr: 12 })).toEqual(["rv", "rv-xhigh"]); + expect(names({ pr: 13 })).toEqual([]); + expect(names({ label: "PR12" })).toEqual(["rv", "rv-xhigh"]); + expect(names({ effort: "xhigh" })).toEqual(["rv-xhigh"]); + expect(names({ verdict: "escalated" })).toEqual(["rv"]); + expect(names({ name: "xhigh" })).toEqual(["rv-xhigh"]); + expect(names({ since: new Date("2026-10-22T10:10:00Z") })).toEqual(["rv-xhigh"]); + }); + + test("--since takes a duration back, a time today or a date", () => { + const now = new Date("2026-10-22T12:00:00Z"); + + expect(parseReviewSince("24h", now)?.toISOString()).toBe("2026-10-21T12:00:00.000Z"); + expect(parseReviewSince("90m", now)?.toISOString()).toBe("2026-10-22T10:30:00.000Z"); + expect(parseReviewSince("2026-10-01", now)?.getDate()).toBe(1); + expect(parseReviewSince("bogus", now)).toBeNull(); + }); + + test("a torn ledger line is skipped and counted, never fatal", () => { + const path = join(mkdtempSync(join(tmpdir(), "gt-review-ledger-")), "reviews.jsonl"); + const [start] = ledgerFixture(); + appendFileSync(path, `${SafeJSON.stringify(start, { strict: true })}\n{"v":1,"kind":"tu\n{"other":true}\n`); + + expect(readReviewRecords(path)).toMatchObject({ records: [{ name: "rv", kind: "start" }], skipped: 2 }); + expect(readFileSync(path, "utf8").split("\n")).toHaveLength(4); + }); +}); + +describe("spawn guards", () => { + test("--base, --allow-shallow and --paths are refused outside --mode review", async () => { + const spawn = (extras: Record) => + codexDriver.spawn({ + name: "x", + cwd: "/repo", + surfaces: { skills: true, rules: true }, + extras: { mode: "task", ...extras }, + }); + + await expect(spawn({ base: "origin/master" })).rejects.toThrow("apply to --mode review only"); + await expect(spawn({ allowShallow: true })).rejects.toThrow("apply to --mode review only"); + }); + + test("a review session cannot start without its review record, and a task session cannot carry one", async () => { + await expect(spawnCodexSession({ name: "x", mode: "review" })).rejects.toThrow("needs its review record"); + await expect(spawnCodexSession({ name: "x", mode: "task", review: reviewConfig() })).rejects.toThrow( + "needs its review record" + ); + }); +}); diff --git a/src/codex/lib/review/types.ts b/src/codex/lib/review/types.ts new file mode 100644 index 0000000000..bf630babc7 --- /dev/null +++ b/src/codex/lib/review/types.ts @@ -0,0 +1,44 @@ +import type { ReviewEffort, ReviewManifest } from "@genesiscz/utils/worker/contract"; + +/** What the depth gate decided about one review turn. */ +export const REVIEW_VERDICTS = ["pass", "steered", "escalated", "shallow"] as const; +export type ReviewVerdict = (typeof REVIEW_VERDICTS)[number]; + +/** Where the base ref came from; printed at spawn, because a derived base is a guess the caller must see. */ +export type ReviewBaseSource = "flag" | "pr" | "upstream"; + +/** What the review turns of one session spent, summed; the gate's reasoning floor reads it. */ +export interface CodexReviewSpent { + turns: number; + durationMs: number; + toolCalls: number; + reasoningTokens: number; + outputTokens: number; + /** API-equivalent dollars; null while any turn was unpriced. */ + costUsd: number | null; +} + +/** The `--mode review` part of a codex session record. */ +export interface CodexReviewConfig { + base: string; + baseSource: ReviewBaseSource; + /** One human sentence on how the base was chosen. */ + baseDetail: string; + branch?: string; + pr?: number; + manifest: ReviewManifest; + effort: ReviewEffort; + effortSource: "flag" | "diff-size"; + allowShallow: boolean; + /** Automatic gate steps already taken in this review chain: 0 fresh, 1 after the steer, 2 in the escalated session. */ + step: number; + /** The first session of the chain; an escalated session keeps its parent's chain. */ + chain: string; + parent?: string; + /** The first prompt of the review, kept so an escalation can rerun it. */ + prompt?: string; + verdict?: ReviewVerdict; + reason?: string; + escalatedTo?: string; + spent?: CodexReviewSpent; +} diff --git a/src/codex/lib/seed-instructions.ts b/src/codex/lib/seed-instructions.ts index e6873f597f..73de67f703 100644 --- a/src/codex/lib/seed-instructions.ts +++ b/src/codex/lib/seed-instructions.ts @@ -1,4 +1,4 @@ -import { buildWorkerContract, type WorkerSandbox } from "@genesiscz/utils/worker/contract"; +import { buildWorkerContract, type WorkerReviewInput, type WorkerSandbox } from "@genesiscz/utils/worker/contract"; /** The contract's name for a codex sandbox mode: `danger-full-access` (`--write full`) has no sandbox at all. */ export function contractSandbox(sandbox: string | undefined): WorkerSandbox { @@ -19,10 +19,12 @@ export function buildAgentInstructions(options: { rendezvousSession: string; leadName: string; sandbox?: string; + review?: WorkerReviewInput | null; }): string { return buildWorkerContract({ backend: "codex", sandbox: contractSandbox(options.sandbox), bus: { agentName: options.agentName, leadName: options.leadName, rendezvousSession: options.rendezvousSession }, + ...(options.review ? { review: options.review } : {}), }); } diff --git a/src/codex/lib/session.test.ts b/src/codex/lib/session.test.ts index b28d66cb78..a0b15c9aac 100644 --- a/src/codex/lib/session.test.ts +++ b/src/codex/lib/session.test.ts @@ -10,6 +10,7 @@ import { SafeJSON } from "@genesiscz/utils/json"; import type { ThreadTurnsListParams, Turn } from "./_generated/protocol"; import { CodexAccountBinding } from "./account"; import { isOwnBusCommand, isReadOnlyCommand } from "./bus-approval"; +import { readReviewRecords } from "./review/ledger"; import type { RpcClient } from "./session"; import { CodexSessionRuntime } from "./session"; import { type CodexSessionMeta, CodexSessionStore } from "./store"; @@ -1168,3 +1169,125 @@ describe("an ask worker's read-only commands", () => { }); }); }); + +describe("a --mode review session", () => { + const report = (reviewed: number) => + `Findings: none\nRan: nothing\nFiles reviewed: ${reviewed} of 2\nNot reviewed: none`; + + async function finishTurn(runtime: CodexSessionRuntime, turn: string, text: string, reasoning = 5): Promise { + const params = { threadId: "thread-1" }; + await runtime.handleNotification({ method: "turn/started", params: { ...params, turn: { id: turn } } }); + await runtime.handleNotification({ + method: "thread/tokenUsage/updated", + params: { + ...params, + tokenUsage: { last: { inputTokens: 100, outputTokens: 50, reasoningOutputTokens: reasoning } }, + }, + }); + await runtime.handleNotification({ + method: "item/completed", + params: { ...params, item: { type: "agentMessage", id: `${turn}-msg`, text } }, + }); + await runtime.handleNotification({ + method: "turn/completed", + params: { ...params, turn: { id: turn, status: "completed" } }, + }); + } + + test("gets its tier and manifest, steers once on a miss, escalates on the second, and ends shallow when that fails", async () => { + const home = mkdtempSync(join(tmpdir(), "gt-codex-runtime-review-")); + + await env.testing.withOverrides({ GENESIS_TOOLS_HOME: home }, async () => { + const store = new CodexSessionStore(); + const meta: CodexSessionMeta = { + ...makeMeta(home), + name: "rv", + agentsEnabled: false, + writePolicy: "deny", + sandbox: "read-only", + effort: "high", + review: { + base: "origin/master", + baseSource: "flag", + baseDetail: "--base", + pr: 12, + manifest: { + base: "origin/master", + files: [ + { path: "src/a.ts", added: 40, deleted: 2 }, + { path: "src/b.ts", added: 8, deleted: 0 }, + ], + added: 48, + deleted: 2, + }, + effort: "high", + effortSource: "diff-size", + allowShallow: false, + step: 0, + chain: "rv", + }, + }; + store.writeMeta(meta); + const client = new FakeRpcClient(); + const escalations: string[] = []; + const gateEvents: unknown[] = []; + const runtime = new CodexSessionRuntime({ + client, + store, + meta, + escalateReview: async (request) => { + escalations.push(`${request.name} at ${request.effort}`); + throw new Error('Codex session "rv-xhigh" is already active'); + }, + onReviewGate: (event) => { + gateEvents.push(event.verdict); + }, + }); + + await runtime.start({ prompt: "Review PR 12." }); + const threadStart = client.requests.find((request) => request.method === "thread/start"); + const startParams = threadStart?.params as { developerInstructions?: string } | undefined; + const instructions = String(startParams?.developerInstructions); + expect(instructions).toContain("You are a code REVIEWER at the high tier."); + expect(instructions).toContain("Files to review (2, +48 -2):\n- src/a.ts +40 -2\n- src/b.ts +8 -0"); + expect((await store.readMeta("rv"))?.review?.prompt).toBe("Review PR 12."); + + await finishTurn(runtime, "turn-1", report(1)); + const steers = client.requests.filter((request) => request.method === "turn/start"); + expect(steers).toHaveLength(2); + const steerText = (steers[1].params as { input: Array<{ text: string }> }).input[0].text; + expect(steerText).toContain("You did not read: src/a.ts, src/b.ts."); + // The session never reads as idle between the missed turn and its steer. + expect((await store.readMeta("rv"))?.status).toBe("running"); + expect((await store.readMeta("rv"))?.review).toMatchObject({ verdict: "steered", step: 1 }); + + await finishTurn(runtime, "turn-2", report(1)); + expect(client.requests.filter((request) => request.method === "turn/start")).toHaveLength(2); + expect(escalations).toEqual(["rv-xhigh at xhigh"]); + + const after = await store.readMeta("rv"); + expect(after?.status).toBe("ready"); + expect(after?.review?.verdict).toBe("shallow"); + expect(after?.review?.reason).toContain('the escalate failed: Codex session "rv-xhigh" is already active'); + expect(after?.review?.spent).toMatchObject({ turns: 2, reasoningTokens: 10 }); + expect(gateEvents).toEqual(["steered", "escalated", "shallow"]); + expect( + readReviewRecords().records.map( + (record) => `${record.kind}:${"verdict" in record ? record.verdict : ""}` + ) + ).toEqual(["turn:", "gate:steered", "turn:", "gate:escalated", "gate:shallow"]); + + // A passing review needs no step: a full report with enough reasoning ends the gate at once. + const passing = new CodexSessionRuntime({ + client: new FakeRpcClient(), + store, + meta: { ...meta, name: "rv-ok", review: { ...meta.review!, chain: "rv-ok", prompt: "x" } }, + }); + store.writeMeta({ ...meta, name: "rv-ok" }); + await passing.start({}); + await finishTurn(passing, "turn-1", report(2), 500); + expect((await store.readMeta("rv-ok"))?.review).toMatchObject({ verdict: "pass", step: 0 }); + expect((await store.readMeta("rv-ok"))?.status).toBe("ready"); + }); + }); +}); diff --git a/src/codex/lib/session.ts b/src/codex/lib/session.ts index fe3755f849..9b923aecbb 100644 --- a/src/codex/lib/session.ts +++ b/src/codex/lib/session.ts @@ -1,11 +1,11 @@ import { relative } from "node:path"; import { formatCostLine } from "@app/ai-spend/lib/workers/cost"; -import { recordWorkerTurnCost } from "@app/ai-spend/lib/workers/ledger"; +import { recordWorkerTurnCost, type WorkerCostRow } from "@app/ai-spend/lib/workers/ledger"; import { withTimeout } from "@genesiscz/utils/async"; import { type CodexFinishedTurn, CodexTurnCostTracker } from "@genesiscz/utils/codex/turn-cost"; import { codexTurnOutcome } from "@genesiscz/utils/codex/turn-outcome"; import { logger } from "@genesiscz/utils/logger"; -import { buildWorkerContract } from "@genesiscz/utils/worker/contract"; +import { buildWorkerContract, type ReviewEffort } from "@genesiscz/utils/worker/contract"; import { WorkerDeliveryRejectedError, workerSourceHome } from "@genesiscz/utils/worker/delivery"; import type { InitializeParams, @@ -26,6 +26,7 @@ import type { RpcNotification, RpcServerRequest } from "./app-server-client"; import { isOwnBusCommand, isReadOnlyCommand } from "./bus-approval"; import type { CodexControl } from "./control"; import { assignThreadToProject, type CodexProject, resolveProjectSelector } from "./desktop-project"; +import { markReviewShallow, recordReviewTurn } from "./review/controller"; import { buildAgentInstructions, contractSandbox } from "./seed-instructions"; import type { CodexSessionMeta, CodexSessionStore } from "./store"; @@ -46,6 +47,16 @@ interface RuntimeOptions { unsubscribeTimeoutMs?: number; /** `tools codex spawn --project`: a Desktop project id, name or root. Without it the cwd decides. */ project?: string; + /** Starts the escalated review session the depth gate asked for; the daemon wires it to `spawnCodexSession`. */ + escalateReview?: (request: EscalateReviewRequest) => Promise; + /** Told about every gate decision, so the daemon can put it on the agents bus. */ + onReviewGate?: (event: Record) => void | Promise; +} + +export interface EscalateReviewRequest { + meta: CodexSessionMeta; + effort: ReviewEffort; + name: string; } interface ThreadStartResult { @@ -119,6 +130,10 @@ export class CodexSessionRuntime { private readonly costTracker: CodexTurnCostTracker; /** The cost line of the last booked turn, for the lead's `turn_completed` bus event. */ private lastTurnCost: { turnId?: string; line: string } | null = null; + /** The current turn's last agent message: a review's report, which the depth gate reads. */ + private turnMessage = ""; + private readonly escalateReview: RuntimeOptions["escalateReview"]; + private readonly onReviewGate: RuntimeOptions["onReviewGate"]; constructor(options: RuntimeOptions) { this.account = options.account; @@ -128,6 +143,8 @@ export class CodexSessionRuntime { this.onApprovalRequest = options.onApprovalRequest; this.unsubscribeTimeoutMs = options.unsubscribeTimeoutMs ?? 1000; this.projectSelector = options.project; + this.escalateReview = options.escalateReview; + this.onReviewGate = options.onReviewGate; // A pinned account is a ChatGPT login; `account/updated` confirms or corrects it. this.costTracker = new CodexTurnCostTracker({ model: options.meta.model ?? null, @@ -159,23 +176,28 @@ export class CodexSessionRuntime { this.chosenProject = resolveProjectSelector(projects.data ?? [], this.projectSelector); } + const review = this.meta.review + ? { effort: this.meta.review.effort, manifest: this.meta.review.manifest } + : null; const threadParams: ThreadStartParams = { cwd: this.meta.cwd, sandbox: this.meta.sandbox, approvalPolicy: this.meta.approvalPolicy, ...(this.meta.model ? { model: this.meta.model } : {}), - // Every worker gets the shared contract (checkpoints, report shape); - // the bus identity and commands only when the swarm is enabled. + // Every worker gets the shared contract (checkpoints, report shape); the bus identity and + // commands only when the swarm is enabled; a reviewer its tier rules and the diff manifest. developerInstructions: this.meta.agentsEnabled ? buildAgentInstructions({ agentName: this.meta.agentName, rendezvousSession: this.meta.rendezvousSession, leadName: "lead", sandbox: this.meta.sandbox, + review, }) : buildWorkerContract({ backend: "codex", sandbox: contractSandbox(this.meta.sandbox), + review, }), }; const thread = await this.client.request("thread/start", threadParams); @@ -198,7 +220,9 @@ export class CodexSessionRuntime { ...(this.meta.effort ? { effort: this.meta.effort } : {}), }; const result = await this.client.request("turn/start", params); - await this.updateMeta({ activeTurnId: result.turn.id, status: "running" }); + // A review keeps its first prompt, so an escalated session can rerun the same review one tier higher. + const review = this.meta.review && !this.meta.review.prompt ? { ...this.meta.review, prompt: body } : undefined; + await this.updateMeta({ activeTurnId: result.turn.id, status: "running", ...(review ? { review } : {}) }); return result.turn.id; } @@ -222,10 +246,9 @@ export class CodexSessionRuntime { await this.fileUnderProject(notification); this.trackFileChange(notification); + this.trackAgentMessage(notification); const finished = this.costTracker.observe(notification, new Date()); - if (finished) { - await this.bookTurnCost(finished); - } + const booked = finished ? await this.bookTurnCost(finished) : null; if (notification.method === "turn/started") { await this.updateMeta({ @@ -238,6 +261,12 @@ export class CodexSessionRuntime { const terminal = codexTurnOutcome(notification); if (terminal) { + // A steered review keeps running: the session never reads as ready between the turn and its steer. + if (finished && this.meta.review && (await this.gateReviewTurn(finished, booked))) { + await this.updateMeta({ lastEventAt }); + return; + } + await this.updateMeta({ activeTurnId: undefined, status: "ready", lastEventAt }); if (terminal.outcome !== "interrupted") { await this.deliverQueuedSteer(); @@ -505,8 +534,99 @@ export class CodexSessionRuntime { } } + /** The last agent message of the current turn, which a review's gate parses. */ + private trackAgentMessage(notification: RpcNotification): void { + if (notification.method === "turn/started") { + this.turnMessage = ""; + return; + } + + const item = nestedRecord(notification.params, "item"); + + if ( + notification.method === "item/completed" && + item?.type === "agentMessage" && + typeof item.text === "string" + ) { + this.turnMessage = item.text; + } + } + + /** + * Runs the depth gate on a finished review turn and takes its step. True when the review continues in this + * session (the steer started a turn). An escalation starts a new session and this one ends; a step the runtime + * cannot take ends the review `shallow` with the reason. + */ + private async gateReviewTurn(finished: CodexFinishedTurn, booked: WorkerCostRow | null): Promise { + const current = this.meta.review; + + if (!current) { + return false; + } + + const outcome = recordReviewTurn({ + name: this.meta.name, + ...(this.meta.label ? { label: this.meta.label } : {}), + review: current, + finished, + report: this.turnMessage, + costUsd: booked?.cost.apiEquivalentUsd ?? null, + }); + this.turnMessage = ""; + await this.updateMeta({ review: outcome.review }); + + if (outcome.decision) { + await this.publishReviewGate(outcome.review); + } + + const action = outcome.action; + + try { + if (action.kind === "steer") { + await this.startTurn(action.text); + return true; + } + + if (action.kind === "escalate") { + if (!this.escalateReview) { + throw new Error("this runtime has no way to start an escalated session"); + } + + await this.escalateReview({ meta: this.meta, effort: action.effort, name: action.name }); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + log.warn({ err, name: this.meta.name, action: action.kind }, "review gate step failed"); + const review = markReviewShallow({ + name: this.meta.name, + ...(this.meta.label ? { label: this.meta.label } : {}), + review: outcome.review, + reason: `${outcome.review.reason}; the ${action.kind} failed: ${message}`, + }); + await this.updateMeta({ review }); + await this.publishReviewGate(review); + } + + return false; + } + + private async publishReviewGate(review: NonNullable): Promise { + try { + await this.onReviewGate?.({ + event: "review_gate", + name: this.meta.name, + verdict: review.verdict, + reason: review.reason, + step: review.step, + ...(review.escalatedTo ? { escalatedTo: review.escalatedTo } : {}), + }); + } catch (err) { + log.warn({ err, name: this.meta.name }, "publishing the review gate decision failed"); + } + } + /** Never throws: `recordWorkerTurnCost` logs its own failure and the turn goes on without a cost line. */ - private async bookTurnCost(finished: CodexFinishedTurn): Promise { + private async bookTurnCost(finished: CodexFinishedTurn): Promise { const row = await recordWorkerTurnCost({ backend: "codex", name: this.meta.name, @@ -529,6 +649,7 @@ export class CodexSessionRuntime { this.lastTurnCost = row ? { ...(finished.turnId ? { turnId: finished.turnId } : {}), line: formatCostLine(row.cost, "codex") } : null; + return row; } /** `{ cost }` for the bus event of the turn just booked; undefined for any other notification. */ diff --git a/src/codex/lib/spawn.ts b/src/codex/lib/spawn.ts index 162c8e0f23..77383f802e 100644 --- a/src/codex/lib/spawn.ts +++ b/src/codex/lib/spawn.ts @@ -16,6 +16,8 @@ import { CODEX_SCHEMA_VERSION } from "./_generated/protocol"; import { CodexAccountBinding } from "./account"; import { computerUseOverrides } from "./computer-use"; import { sessionDaemonLogPath, sessionLaunchPath } from "./paths"; +import { appendReviewRecord, reviewLedgerBase } from "./review/ledger"; +import type { CodexReviewConfig } from "./review/types"; import { type CodexSessionMeta, CodexSessionStore, type CodexWritePolicy } from "./store"; import { detectCodexVersion } from "./version"; @@ -37,6 +39,8 @@ export interface SpawnOptions { label?: string; /** A Codex Desktop project id, name or root to show the thread under; the cwd decides without it. */ project?: string; + /** Required with `mode: "review"`: what `prepareReview` decided, plus the chain fields. */ + review?: CodexReviewConfig; } export interface LaunchConfig { @@ -269,6 +273,10 @@ export interface CodexSpawnResult extends CodexSessionMeta { } export async function spawnCodexSession(options: SpawnOptions): Promise { + if ((options.mode === "review") !== (options.review !== undefined)) { + throw new Error("A review session needs its review record, and only a review session takes one"); + } + const cwd = resolve(options.cwd ?? process.cwd()); const checkout = checkoutKindSync(cwd); assertWritePolicyAllowed({ write: options.write, checkout }); @@ -335,6 +343,8 @@ export async function spawnCodexSession(options: SpawnOptions): Promise { try { return Bun.spawn({ diff --git a/src/codex/lib/store.ts b/src/codex/lib/store.ts index c0f05cb0ff..4828ed578c 100644 --- a/src/codex/lib/store.ts +++ b/src/codex/lib/store.ts @@ -7,6 +7,7 @@ import { JsonlWriter } from "@genesiscz/utils/log-session/jsonl-writer"; import { logger } from "@genesiscz/utils/logger"; import { WorkerMetaStore } from "@genesiscz/utils/worker/meta-store"; import { sessionEventsPath, sessionMetaPath, sessionsDir } from "./paths"; +import type { CodexReviewConfig } from "./review/types"; const log = logger.child({ component: "codex:store" }); @@ -81,6 +82,8 @@ export interface CodexSessionMeta { desktopProject?: { id: string; name: string }; /** The `spawn --label` tag every cost-ledger row of the session carries. */ label?: string; + /** `spawn --mode review`: the base, the diff manifest, the tier and the depth gate's state. */ + review?: CodexReviewConfig; } export interface CodexEventRecord { From 3fd83eca3cb23b46ff9546c6133e4da7b66c265e Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:03:24 +0200 Subject: [PATCH 04/10] feat(codex): tools codex reviews lists every review run from the ledger with filters and totals, and review-depth shows one review's coverage, reasoning per 100 lines and gate history --- src/codex/README.md | 31 ++- src/codex/commands/reviews.ts | 384 ++++++++++++++++++++++++++++++++++ src/codex/index.ts | 2 + 3 files changed, 416 insertions(+), 1 deletion(-) create mode 100644 src/codex/commands/reviews.ts diff --git a/src/codex/README.md b/src/codex/README.md index 4f89b2ac03..d8c9990537 100644 --- a/src/codex/README.md +++ b/src/codex/README.md @@ -240,7 +240,8 @@ Sources: [app-server protocol](https://learn.chatgpt.com/docs/app-server), tools codex spawn \ --name reviewer \ --mode review \ - --prompt "Review the current working tree" + --base origin/master \ + --prompt "Review this branch" tools codex status --name reviewer tools codex tail --name reviewer --follow @@ -266,6 +267,34 @@ worker verbs (`src/ai/commands/agent/worker.ts`); what makes them codex is `code (`src/codex/lib/driver.ts`). `steer` still answers to its older `--body` / `--body-file` spelling, hidden from help. +### Review mode (`--mode review`) + +`--mode review` needs a base: `--base `, else the branch's PR target, else its upstream (never the branch's own +remote copy); the spawn prints which. It runs `git diff --numstat ...HEAD` in the worker's cwd (limited to +`--paths ` when given) and refuses an empty diff. The worker's developer instructions carry the shared worker +contract plus `reviewLines(effort)` from `src/utils/worker/contract.ts` and the manifest (`Files to review (N, +A -D)` +and one line per file). + +The tier is `--effort`, else the diff size: `high` up to 15 files and 800 changed lines, `xhigh` above. `max` only when +named. `low` (triage) and `medium` are refused unless `--allow-shallow`. Above 60 files or 4,000 lines the spawn prints +a split suggestion, one reviewer per folder with `--paths`. + +After each review turn the daemon runs the depth gate (`src/codex/lib/review/gate.ts`): the `Files reviewed: n of N` +line against the manifest (a binary file named with a reason is excused), reasoning tokens per 100 changed lines +against the tier floor (high 100, xhigh and max 250), and the finding shape (`N. [Severity] path:line - defect. +Scenario: ... Fix: ...`, then `Ran:`). The first miss steers the same session once with the unread files; the second +starts `-` as a new session (a steer cannot change a thread's effort), at step 2; a miss after that, +or a miss at `max`, ends the review `shallow`. A medium review with `--allow-shallow` is never tuned up. Each decision +also goes to the agents bus as a `review_gate` event. + +Every review start, review turn and gate decision is one line of `~/.genesis-tools/codex/reviews.jsonl`: + +```bash +tools codex reviews [--since 24h] [--pr ] [--label ] [--effort ] [--verdict pass|steered|escalated|shallow|open] [--name ] [--json] +tools codex review-depth [--json] # one review: manifest, spend, coverage, reasoning per 100 lines, verdict, gate history +tools codex review-depth --base # the same checks for a session spawned without --mode review +``` + `login` and `usage` are doors onto the provider-neutral account core: the same code runs behind `tools ai accounts login --provider codex` and `tools ai usage --provider codex`. `tools ai accounts discover --provider codex` lists every `~/.codex*` profile on the machine and `--bind` turns the unbound ones into accounts. Sessions are read-only by default. For implementation work, use `--write ask` for supervised approvals or diff --git a/src/codex/commands/reviews.ts b/src/codex/commands/reviews.ts new file mode 100644 index 0000000000..cfa94646ec --- /dev/null +++ b/src/codex/commands/reviews.ts @@ -0,0 +1,384 @@ +import { sessionCostSummary } from "@app/ai-spend/lib/workers/ledger"; +import { isInteractive, suggestEnumFlag } from "@genesiscz/utils/cli"; +import { toolCommand } from "@genesiscz/utils/cli/tool-command"; +import { formatDuration, formatTokens } from "@genesiscz/utils/format"; +import { logger, out } from "@genesiscz/utils/logger"; +import { isObject } from "@genesiscz/utils/object"; +import * as p from "@genesiscz/utils/prompts/p"; +import { createBoxTable, formatDotStatus, renderCliHeader, truncateDisplay } from "@genesiscz/utils/table"; +import { + isReviewEffort, + manifestChangedLines, + REVIEW_EFFORTS, + type ReviewEffort, +} from "@genesiscz/utils/worker/contract"; +import type { Command } from "commander"; +import pc from "picocolors"; +import { checkReview, type ReviewGateChecks } from "../lib/review/gate"; +import { + type ReviewGateRecord, + type ReviewLedgerRecord, + readReviewRecords, + reviewLedgerPath, +} from "../lib/review/ledger"; +import { buildManifest, defaultReviewGitReaders } from "../lib/review/prepare"; +import { + filterReviews, + parseReviewSince, + type ReviewSummary, + reviewTotals, + summarizeReviews, +} from "../lib/review/report"; +import { REVIEW_VERDICTS } from "../lib/review/types"; +import { type CodexSessionMeta, CodexSessionStore } from "../lib/store"; + +const { log } = logger.scoped("codex-reviews"); + +const VERDICT_FILTERS = [...REVIEW_VERDICTS, "open"] as const; + +interface ReviewsOptions { + since?: string; + pr?: string; + label?: string; + effort?: string | true; + verdict?: string | true; + name?: string; + json?: boolean; +} + +function plural(count: number, noun: string): string { + return `${count} ${noun}${count === 1 ? "" : "s"}`; +} + +function money(usd: number | null): string { + return usd === null ? "unpriced" : `$${usd.toFixed(2)}`; +} + +function verdictCell(verdict: ReviewSummary["verdict"]): string { + const tone = verdict === "pass" ? "ok" : verdict === "shallow" ? "err" : verdict === "open" ? "dim" : "warn"; + return formatDotStatus(tone, verdict); +} + +/** + * A closed-set flag given without a value or with an unknown one: a TTY picks from the values, anything else gets + * the values and a filled command line, and the run stops with exit 1. + */ +async function enumFilter(flag: string, raw: string | true | undefined, values: readonly string[]) { + if (raw === undefined) { + return { ok: true as const, value: undefined }; + } + + if (raw !== true && values.includes(raw)) { + return { ok: true as const, value: raw }; + } + + if (isInteractive()) { + const picked = await p.select({ + message: `${flag}`, + options: values.map((value) => ({ value, label: value })), + }); + + if (!p.isCancel(picked) && typeof picked === "string") { + return { ok: true as const, value: picked }; + } + } + + out.log.error(suggestEnumFlag("tools codex", flag, values, raw === true ? {} : { given: raw })); + process.exitCode = 1; + return { ok: false as const }; +} + +async function runReviews(options: ReviewsOptions): Promise { + const effort = await enumFilter("--effort", options.effort, REVIEW_EFFORTS); + const verdict = await enumFilter("--verdict", options.verdict, VERDICT_FILTERS); + + if (!effort.ok || !verdict.ok) { + return; + } + + const since = options.since === undefined ? undefined : parseReviewSince(options.since); + + if (since === null) { + out.log.error(`--since ${options.since} is not a duration (24h, 90m, 7d), a time (HH:MM) or a date.`); + process.exitCode = 1; + return; + } + + const pr = options.pr === undefined ? undefined : Number(options.pr.replace(/^#/, "")); + + if (pr !== undefined && !Number.isInteger(pr)) { + out.log.error(`--pr ${options.pr} is not a PR number.`); + process.exitCode = 1; + return; + } + + const path = reviewLedgerPath(); + const { records, skipped } = readReviewRecords(path); + const reviews = filterReviews(summarizeReviews(records), { + ...(since ? { since } : {}), + ...(pr === undefined ? {} : { pr }), + ...(options.label ? { label: options.label } : {}), + ...(effort.value ? { effort: effort.value } : {}), + ...(verdict.value ? { verdict: verdict.value } : {}), + ...(options.name ? { name: options.name } : {}), + }); + const totals = reviewTotals(reviews); + log.debug({ path, records: records.length, skipped, shown: reviews.length }, "codex reviews read"); + + if (options.json) { + out.result({ ledger: path, skipped, reviews, totals }); + return; + } + + if (skipped > 0) { + out.log.warn(`${skipped} ledger line(s) in ${path} did not parse and were skipped.`); + } + + if (reviews.length === 0) { + out.log.info(`No review runs match${records.length === 0 ? `: ${path} has none yet` : ""}.`); + return; + } + + renderCliHeader("Codex reviews", `${reviews.length} review(s) · ledger ${path}`); + const table = createBoxTable([ + "NAME", + "PR", + "FILES", + "EFFORT", + "TIME", + "TOOLS", + "RSN/100L", + "COVERAGE", + "VERDICT", + "COST", + ]); + + for (const review of reviews) { + table.push([ + truncateDisplay(review.label ? `${review.name} ${pc.dim(review.label)}` : review.name, 36), + review.pr === undefined ? (review.branch ? truncateDisplay(review.branch, 18) : "—") : `#${review.pr}`, + `${review.files} (${review.lines}L)`, + review.effort, + formatDuration(review.durationMs), + String(review.toolCalls), + review.reasoningPer100Lines === null ? "—" : String(review.reasoningPer100Lines), + review.coverage ?? "—", + review.escalatedTo ? `${verdictCell(review.verdict)} → ${review.escalatedTo}` : verdictCell(review.verdict), + money(review.costUsd), + ]); + } + + out.println(table.toString()); + const verdicts = Object.entries(totals.verdicts) + .map(([name, count]) => `${count} ${name}`) + .join(", "); + out.println( + `Totals: ${plural(totals.reviews, "review")} (${verdicts}) · ${plural(totals.turns, "turn")} · ${formatDuration(totals.durationMs)} · ${totals.toolCalls} tool calls · ${formatTokens(totals.reasoningTokens)} reasoning · $${totals.costUsd.toFixed(2)} API-equivalent${totals.unpricedReviews > 0 ? ` (+${totals.unpricedReviews} unpriced)` : ""}` + ); + out.println(pc.dim(`One review in depth: ${toolCommand("codex review-depth")} `)); +} + +interface DepthOptions { + base?: string; + json?: boolean; +} + +function checkLine(label: string, ok: boolean, detail: string): string { + return `${ok ? pc.green("✓") : pc.red("✗")} ${label.padEnd(9)} ${detail}`; +} + +function gateLines(gate: ReviewGateRecord): string[] { + const coverageOk = gate.coverage.reviewed !== null && gate.coverage.reviewed >= gate.coverage.total; + const reasoningOk = gate.reasoningPer100Lines === null || gate.reasoningPer100Lines >= gate.floor; + + return [ + checkLine( + "coverage", + coverageOk, + `${gate.coverage.reviewed ?? "no line"} of ${gate.coverage.total} files${gate.unread.length > 0 ? `; unread: ${gate.unread.slice(0, 8).join(", ")}${gate.unread.length > 8 ? ", …" : ""}` : ""}` + ), + checkLine( + "reasoning", + reasoningOk, + `${gate.reasoningPer100Lines ?? "unknown"} tokens per 100 changed lines (floor ${gate.floor})` + ), + checkLine( + "shape", + gate.malformedFindings === 0, + `${gate.findings} findings, ${gate.malformedFindings} malformed` + ), + ]; +} + +function checksLines(checks: ReviewGateChecks): string[] { + return [ + checkLine("coverage", checks.coverage.ok, checks.coverage.detail), + checkLine("reasoning", checks.reasoning.ok, checks.reasoning.detail), + checkLine("shape", checks.shape.ok, checks.shape.detail), + ]; +} + +/** The last agent message of the session's own thread, from its event log. */ +async function lastAgentMessage(store: CodexSessionStore, meta: CodexSessionMeta): Promise { + const events = await store.readEvents(meta.name); + + for (const event of events.toReversed()) { + const params = event.params; + + if (event.method !== "item/completed" || !isObject(params) || !isObject(params.item)) { + continue; + } + + const { item, threadId } = params; + + if ( + item.type === "agentMessage" && + typeof item.text === "string" && + (threadId === undefined || threadId === meta.threadId) + ) { + return item.text; + } + } + + return ""; +} + +/** A session spawned without `--mode review`: the gate's checks against a manifest computed now from `--base`. */ +async function adHocDepth(store: CodexSessionStore, meta: CodexSessionMeta, options: DepthOptions): Promise { + if (!options.base) { + out.log.error( + `${meta.name} was not spawned with --mode review, so it has no manifest. Pass --base to measure it against git diff ...HEAD in ${meta.cwd}.` + ); + process.exitCode = 1; + return; + } + + const readers = defaultReviewGitReaders(); + const manifest = buildManifest(options.base, await readers.numstat(meta.cwd, options.base)); + const effort: ReviewEffort = isReviewEffort(meta.effort) ? meta.effort : "high"; + const cost = sessionCostSummary({ backend: "codex", name: meta.name, sessionId: meta.threadId }); + const reasoning = cost ? cost.session.tokens.reasoning : null; + const checks = checkReview({ + report: await lastAgentMessage(store, meta), + manifest, + effort, + reasoningTokens: reasoning, + }); + + if (options.json) { + out.result({ name: meta.name, review: null, base: options.base, effort, manifest, checks, cost }); + return; + } + + out.println(`${pc.bold(meta.name)} ${pc.dim("not a --mode review session; measured now")} · effort ${effort}`); + out.println( + `manifest ${manifest.files.length} files, +${manifest.added} -${manifest.deleted} against ${options.base}` + ); + + if (cost) { + out.println( + `spent ${cost.session.turns} turns · ${formatDuration(cost.session.durationMs)} · ${cost.session.toolCalls} tool calls · ${formatTokens(cost.session.tokens.reasoning)} reasoning · $${cost.session.apiEquivalentUsd.toFixed(2)}` + ); + } + + for (const line of checksLines(checks)) { + out.println(line); + } +} + +async function runReviewDepth(name: string, options: DepthOptions): Promise { + const store = new CodexSessionStore(); + const meta = await store.readMeta(name); + + if (!meta) { + out.log.error(`No codex session named ${name}. ${toolCommand("codex sessions")} lists them.`); + process.exitCode = 1; + return; + } + + const review = meta.review; + + if (!review) { + await adHocDepth(store, meta, options); + return; + } + + const records = readReviewRecords().records.filter( + (record: ReviewLedgerRecord) => record.name === name && record.chain === review.chain + ); + const gates = records.filter((record): record is ReviewGateRecord => record.kind === "gate"); + const lastGate = gates.at(-1); + + if (options.json) { + out.result({ name, review, gates, turns: records.filter((record) => record.kind === "turn") }); + return; + } + + const spent = review.spent; + const lines = manifestChangedLines(review.manifest); + out.println( + `${pc.bold(name)} review · effort ${review.effort} (${review.effortSource === "flag" ? "--effort" : "diff size"}) · ${verdictCell(review.verdict ?? "open")}${review.escalatedTo ? ` → ${review.escalatedTo}` : ""}` + ); + out.println( + `base ${review.base} (${review.baseSource}: ${review.baseDetail})${review.branch ? ` · branch ${review.branch}` : ""}${review.pr === undefined ? "" : ` · PR #${review.pr}`}${review.parent ? ` · escalated from ${review.parent}` : ""}` + ); + out.println( + `manifest ${review.manifest.files.length} files, +${review.manifest.added} -${review.manifest.deleted} (${lines} lines)` + ); + + if (spent) { + out.println( + `spent ${spent.turns} turns · ${formatDuration(spent.durationMs)} · ${spent.toolCalls} tool calls · ${formatTokens(spent.reasoningTokens)} reasoning · ${money(spent.costUsd)}` + ); + } + + if (lastGate) { + for (const line of gateLines(lastGate)) { + out.println(line); + } + } + + if (review.reason) { + out.println(`reason: ${review.reason}`); + } + + if (gates.length > 1) { + out.println(pc.dim("gate history:")); + + for (const gate of gates) { + // Local time, like every time this tool prints; the ledger keeps UTC. + out.println( + pc.dim( + ` ${new Date(gate.ts).toLocaleString("sv-SE")} step ${gate.step} ${gate.verdict}: ${gate.reason}` + ) + ); + } + } +} + +export function registerReviewsCommands(program: Command): void { + program + .command("reviews") + .description("Every --mode review run from the review ledger: one row per review, with totals") + .option("--since ", "Only reviews active since: a duration back (24h, 7d), HH:MM today, or a date") + .option("--pr ", "Only reviews of this PR number") + .option("--label ", "Only reviews whose spawn --label contains this text") + .option("--effort [tier]", `Only reviews at this tier: ${REVIEW_EFFORTS.join(" | ")}`) + .option("--verdict [verdict]", `Only reviews with this gate verdict: ${VERDICT_FILTERS.join(" | ")}`) + .option("--name ", "Only reviews whose session or chain name contains this text") + .option("--json", "Emit the reviews and totals as JSON") + .action(async (options: ReviewsOptions) => { + await runReviews(options); + }); + + program + .command("review-depth ") + .description( + "One review's depth: manifest, effort, spend, coverage, reasoning per 100 lines and the gate verdict" + ) + .option("--base ", "For a session spawned without --mode review: the base to measure it against") + .option("--json", "Emit as JSON") + .action(async (name: string, options: DepthOptions) => { + await runReviewDepth(name, options); + }); +} diff --git a/src/codex/index.ts b/src/codex/index.ts index 75bff2694a..f59e455e27 100644 --- a/src/codex/index.ts +++ b/src/codex/index.ts @@ -9,6 +9,7 @@ import { registerLogsCommand } from "./commands/logs"; import { registerMigrateHomeCommand } from "./commands/migrate-home"; import { registerCodexMessageCommands } from "./commands/queue"; import { registerReviewCommand } from "./commands/review"; +import { registerReviewsCommands } from "./commands/reviews"; import { registerRollbackCommand } from "./commands/rollback"; import { codexSpec } from "./lib/spec"; @@ -24,6 +25,7 @@ registerMigrateHomeCommand(program); registerDesktopCommand(program); registerRollbackCommand(program); registerReviewCommand(program); +registerReviewsCommands(program); registerApprovalCommands(program); registerLogsCommand(program); From 9d41011e0230e4966d287205192b81adbd6c8957 Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:03:26 +0200 Subject: [PATCH 05/10] docs(skills, babysit, handoff-to): reviews spawn with --mode review --base and leave depth to its contract and gate, and both skills name tools codex reviews; plugin 1.0.116 --- .claude-plugin/marketplace.json | 4 +- .claude/skills/babysit/SKILL.md | 16 +++-- .../skills/babysit/scripts/review-brief.ts | 72 ++++++++++++++++--- .../skills/babysit/scripts/scripts.test.ts | 31 +++++++- .../genesis-tools/.claude-plugin/plugin.json | 2 +- .../genesis-tools/skills/handoff-to/SKILL.md | 19 +++-- .../skills/handoff-to/references/codex.md | 10 ++- 7 files changed, 125 insertions(+), 29 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 64d2031326..c741905f35 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,7 +1,7 @@ { "$schema": "https://anthropic.com/claude-code/marketplace.schema.json", "name": "genesis-tools", - "version": "1.0.115", + "version": "1.0.116", "description": "Plugins for GenesisTools CLI development and management", "owner": { "name": "genesiscz", @@ -11,7 +11,7 @@ { "name": "genesis-tools", "description": "Skills and utilities for working with GenesisTools CLI toolkit. Provides guidance for discovering, executing, and troubleshooting genesis tools with integrated workflow support.", - "version": "1.0.115", + "version": "1.0.116", "author": { "name": "GenesisTools" }, diff --git a/.claude/skills/babysit/SKILL.md b/.claude/skills/babysit/SKILL.md index cfc1732026..7b65c9dce5 100644 --- a/.claude/skills/babysit/SKILL.md +++ b/.claude/skills/babysit/SKILL.md @@ -54,7 +54,7 @@ Write it to `/tmp/cc///babysit/run.json` at the start; the scr |---|---|---| | `prs` | asked | PR numbers. Small PRs first; never a PR another session owns (step 3.1). | | `worker` per PR | `grok` | `grok` (model `grok-4.7-build-fast`, `--auth subscription`) or `codex` (model `gpt-6.1-sol`, `--write ask`). Mix freely; one worker per PR. | -| `effort` per PR | from the work | Codex `--effort`: `low`, `medium`, `high`, `xhigh` or `max`. A fix round: `medium`, `high` for a Critical or a cross-file finding. An independent review (step 7a): `high`, `xhigh` above about 50 changed files. Medium reviews were measured shallow (handoff-to, "Effort is the review's depth"). | +| `effort` per PR | from the work | Codex `--effort`: `low`, `medium`, `high`, `xhigh` or `max`. A fix round: `medium`, `high` for a Critical or a cross-file finding. An independent review (step 7a): no effort; `--mode review` picks high or xhigh from the diff size and gates the depth (handoff-to, "Effort is the review's depth"). | | `mode` | from `scripts/mode.ts` | `worktree` or `unified`, step 2. | | `maxParallel` | 3 | Workers at once. The master is the bottleneck, not the workers. | | `post` | ask | Post the round summary and replies after the push (review-loop step 4), or hold them in the vault. | @@ -322,11 +322,15 @@ A round counts only when it fixed or answered real findings. The round policy (s rounds: after two rounds with no Critical or High, switch to merge prep (review-loop "Round policy"), and land by the merge bar. When the user asks for a dedicated review of a PR, before or after its merge, run an independent read-only review: `bun -.claude/skills/babysit/scripts/review-brief.ts --run --pr --wt [--post-merge] [--swift]`, then -`steer.ts` with the printed file to a worker that did NOT fix this PR (Sol; Astra only for a risky cross-system PR, -by the model rule in step 1), at the run card's review `effort` (`high`, `xhigh` above about 50 changed files). A steer -keeps the session's effort, so spawn the reviewer at that effort; check its depth afterwards in `tools codex status ---name --json` (`cost.lastTurn`: `durationMs`, `toolCalls`, `tokens.reasoning`). Judge its findings like a bot's. Before a merge they become the next fix round (`fill-brief --judge --pr --wt [--post-merge] [--swift]`. It +prints the brief path and then the spawn line: `tools codex spawn --mode review --base origin/` (after a +merge `--base ^ --paths `) with the brief as `--prompt-file`, for a worker that did NOT +fix this PR (Sol; Astra only for a risky cross-system PR, by the model rule in step 1). Leave out `--effort`: the +harness picks high or xhigh from the diff size, gives the worker the manifest and the tier's contract, and gates the +depth after the turn (one steer with the unread files, then one rerun a tier higher as `bs--review-`, then +`shallow`). Read the result with `tools codex review-depth bs--review`; a `shallow` verdict is not a finished +review. Every run of the round is one command away: `tools codex reviews --pr ` (or `--label babysit`). Judge its +findings like a bot's. Before a merge they become the next fix round (`fill-brief --judge --dedicated-review`); after a merge they go to a follow-up PR from master (2026-10-14: #518 carried #509's after-merge findings). Never count a reply-only round, or a label, as a fix round: on 2026-10-14 the report said "#500 merged after 3 rounds" when GitHub showed two round summaries. diff --git a/.claude/skills/babysit/scripts/review-brief.ts b/.claude/skills/babysit/scripts/review-brief.ts index 6e3ac48e18..17e62eeb1a 100644 --- a/.claude/skills/babysit/scripts/review-brief.ts +++ b/.claude/skills/babysit/scripts/review-brief.ts @@ -2,10 +2,15 @@ /** * The brief for an independent read-only review round, which runs when the user asks for one (SKILL.md 7a): on an * open PR, or on a PR that already merged. Its findings become the next fix round (`fill-brief --judge`) - * or, after a merge, a follow-up PR. Writes `/steer-review-.md` and prints the path for steer.ts. + * or, after a merge, a follow-up PR. Writes `/steer-review-.md`, prints its path, then the + * `tools codex spawn --mode review` line that starts the reviewer with it. * * bun review-brief.ts --run --pr 517 --wt [--base master] [--post-merge] [--swift] [--fresh] * + * The depth rules (read every file, probe each finding, the coverage line) are not in this brief: `--mode review` + * gives the worker its tier's contract and the diff manifest, and gates the depth after the turn + * (`tools codex reviews --pr ` lists every run). + * * `--fresh`: a PR opened minutes ago, reviewed next to the bots (the first round of a newly cut PR). * * Hand-edited copies of this text sent a reviewer to another PR's description (2026-10-14: a sed pass left @@ -46,13 +51,13 @@ export function reviewBrief({ pr, title, wt, base, scratch, postMerge, swift, fr fresh ? `- The PR has no answered threads yet. Do not wait for the bots and do not read their threads: review the code on your own.` : `- Earlier review threads were already answered; read them with \`tools github review ${pr} --llm\` so you do not report a finding that was fixed or deliberately declined (a declined one is fair game only with new evidence).`, - `- Check every claim: run the relevant tests (\`timeout 600 bun scripts/test.ts \`) or a small read-only probe under \`${scratch}\`, and paste the real output. A finding without evidence is a guess; say so.`, + `- Tests in this repository run as \`timeout 600 bun scripts/test.ts \`; put any read-only probe under \`${scratch}\`.`, ...(swift ? [ "- Swift: do not run `swift build` or `swift test` (the lead runs them); a Swift claim needs a code quote as evidence.", ] : []), - `- Write \`${join(wt, ".claude/work/babysit", `${pr}-review.md`)}\` (create the folder): one section per finding with severity (Critical, High, Medium, Low), file:line, the defect, the evidence, why it matters, and the smallest fix. End with a one-line verdict.`, + `- Write \`${join(wt, ".claude/work/babysit", `${pr}-review.md`)}\` (create the folder): the same numbered findings, \`Ran\`, \`Files reviewed\` and \`Not reviewed\` lines as your final message, with the evidence under each finding, then a one-line verdict.`, "- Change NO source file, commit nothing, post nothing to GitHub. Do NOT run `git checkout`, `git switch`, `bun install` or the hook installer. No app builds, installs, launches or relaunches. Another reviewer may read this same folder, so write only your own review file.", "", "Then stop with a report: the finding count by severity and the review file path, then RESULT / AT / CHANGED / VERIFY / OPEN.", @@ -60,6 +65,32 @@ export function reviewBrief({ pr, title, wt, base, scratch, postMerge, swift, fr return `${lines.join("\n")}\n`; } +export interface ReviewSpawnInput { + pr: number; + wt: string; + /** The PR's base branch, without `origin/`. */ + base: string; + briefPath: string; + /** After a merge the worktree sits at master: the manifest is the PR's files since its merge commit's parent. */ + postMerge?: { mergeCommit: string; files: string[] }; +} + +/** The reviewer's spawn line: `--mode review` with the base, so the harness computes the manifest and gates the depth. */ +export function reviewSpawnCommand({ pr, wt, base, briefPath, postMerge }: ReviewSpawnInput): string { + const target = postMerge + ? `--base ${postMerge.mergeCommit}^ --paths ${postMerge.files.join(" ")}` + : `--base origin/${base}`; + + return `tools codex spawn --mode review ${target} --name bs-${pr}-review --model gpt-6.1-sol --write ask --cwd ${wt} --label "babysit pr${pr} review" --prompt-file ${briefPath}`; +} + +interface PrView { + title: string; + baseRefName?: string; + mergeCommit?: { oid?: string } | null; + files?: Array<{ path: string }>; +} + if (import.meta.main) { const run = loadRun(); const pr = Number(flag("--pr")); @@ -72,17 +103,21 @@ if (import.meta.main) { process.exit(2); } - const view = Bun.spawnSync(["gh", "pr", "view", String(pr), "--repo", run.repo, "--json", "title"], { - stdout: "pipe", - stderr: "pipe", - }); + const view = Bun.spawnSync( + ["gh", "pr", "view", String(pr), "--repo", run.repo, "--json", "title,baseRefName,mergeCommit,files"], + { stdout: "pipe", stderr: "pipe" } + ); if (view.exitCode !== 0) { console.error(`✖ gh pr view ${pr}: ${view.stderr.toString().trim()}`); process.exit(1); } - const { title } = SafeJSON.parse(view.stdout.toString(), { strict: true }) as { title: string }; + const { title, baseRefName, mergeCommit, files } = SafeJSON.parse(view.stdout.toString(), { + strict: true, + }) as PrView; + const base = flag("--base") ?? baseRefName ?? "master"; + const postMerge = has("--post-merge"); const out = join(run.dir, `steer-review-${pr}.md`); await Bun.write( out, @@ -90,12 +125,29 @@ if (import.meta.main) { pr, title, wt, - base: flag("--base") ?? "master", + base, scratch: reviewScratch(run.dir, pr), - postMerge: has("--post-merge"), + postMerge, swift: has("--swift"), fresh: has("--fresh"), }) ); console.log(out); + + if (postMerge && !mergeCommit?.oid) { + console.error(`✖ PR #${pr} has no merge commit, so a post-merge review has no base.`); + process.exit(1); + } + + console.log( + reviewSpawnCommand({ + pr, + wt, + base, + briefPath: out, + ...(postMerge && mergeCommit?.oid + ? { postMerge: { mergeCommit: mergeCommit.oid, files: (files ?? []).map((file) => file.path) } } + : {}), + }) + ); } diff --git a/.claude/skills/babysit/scripts/scripts.test.ts b/.claude/skills/babysit/scripts/scripts.test.ts index 306164d364..fcc0084437 100644 --- a/.claude/skills/babysit/scripts/scripts.test.ts +++ b/.claude/skills/babysit/scripts/scripts.test.ts @@ -26,7 +26,7 @@ import { appendEvent, readEvents, renderMarkdown } from "./ledger"; import { continueVersionPick } from "./pick-unified"; import { summarize, testEvidence } from "./plant"; import { hitsInAddedLines, probeAddedLines } from "./probe"; -import { reviewBrief } from "./review-brief"; +import { reviewBrief, reviewSpawnCommand } from "./review-brief"; import { loadRun, runPaths } from "./run"; import { trySwiftSlot } from "./swift-slot"; import { parseSwiftTargets, typecheckPackage } from "./swift-typecheck"; @@ -656,6 +656,35 @@ describe("independent review brief", () => { expect(fresh).not.toContain("already answered"); expect(reviewBrief(input)).toContain("found nothing more"); }); + + test("the reviewer starts in --mode review against the PR base, and the brief leaves depth to its contract", () => { + const input = { + pr: 498, + title: "t", + wt: "/w", + base: "feat/parent", + scratch: "/s", + postMerge: false, + swift: false, + }; + expect(reviewBrief(input)).not.toContain("A finding without evidence is a guess"); + expect(reviewBrief(input)).toContain("`Files reviewed` and `Not reviewed` lines"); + + expect( + reviewSpawnCommand({ pr: 498, wt: "/w", base: "feat/parent", briefPath: "/run/steer-review-498.md" }) + ).toBe( + 'tools codex spawn --mode review --base origin/feat/parent --name bs-498-review --model gpt-6.1-sol --write ask --cwd /w --label "babysit pr498 review" --prompt-file /run/steer-review-498.md' + ); + expect( + reviewSpawnCommand({ + pr: 498, + wt: "/w", + base: "master", + briefPath: "/b.md", + postMerge: { mergeCommit: "abc123", files: ["src/a.ts", "src/b.ts"] }, + }) + ).toContain("--mode review --base abc123^ --paths src/a.ts src/b.ts --name bs-498-review"); + }); }); describe("planted-regression summary", () => { test("reads bun counts and the XCTest total, and says so when there is none", () => { diff --git a/plugins/genesis-tools/.claude-plugin/plugin.json b/plugins/genesis-tools/.claude-plugin/plugin.json index 9d01c1c762..f7e21e794f 100644 --- a/plugins/genesis-tools/.claude-plugin/plugin.json +++ b/plugins/genesis-tools/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "genesis-tools", - "version": "1.0.115", + "version": "1.0.116", "description": "Skills and utilities for working with GenesisTools CLI toolkit. Provides guidance for discovering, executing, and troubleshooting genesis tools with integrated workflow support.", "author": { "name": "GenesisTools" diff --git a/plugins/genesis-tools/skills/handoff-to/SKILL.md b/plugins/genesis-tools/skills/handoff-to/SKILL.md index dea9c84987..98793c348f 100644 --- a/plugins/genesis-tools/skills/handoff-to/SKILL.md +++ b/plugins/genesis-tools/skills/handoff-to/SKILL.md @@ -84,18 +84,25 @@ Set a model explicitly; an Astra parent should not make every worker Astra. | Mechanical extraction, formatting, simple lookups, or a fully specified small edit | `gpt-6-luna` | low | | Bounded repository exploration, tracing an established call path, or gathering evidence | `gpt-6-luna` | medium | | Normal implementation and reproducible debugging | `gpt-6.1-sol` | medium | -| Code review of a PR or branch | `gpt-6.1-sol` | high; xhigh above about 50 changed files | +| Code review of a PR or branch | `gpt-6.1-sol` | `--mode review --base `: high up to 15 files and 800 lines, xhigh above | | Difficult cross-system reasoning, ambiguous failures, or an evidence-backed escalation | `gpt-6-astra` | high | `gpt-6.1-sol` accepts `low` to `max` effort only; `none` and `minimal` return an error, as on Astra. -**Effort is the review's depth: set it from the diff size, never leave a review at medium.** Measured on +**Effort is the review's depth: spawn every Codex review with `--mode review --base `.** Measured on 2026-10-22 with nine Sol reviews at `medium`: 1 to 4 minutes each, 8 to 26 tool calls, and 358 to 2,399 reasoning tokens, also for a 125-file, 6,200-line PR. The findings were real, but no worker could have read -every file. Pass `--effort high` for a review, `xhigh` above about 50 changed files, `max` only for a risky -cross-system PR, and tell the worker in the brief to read every changed file and to end with `Files reviewed: - of `. Check the depth afterwards: `tools codex status --name --json` carries the turn's -`durationMs`, `toolCalls` and `tokens.reasoning` under `cost.lastTurn`. +every file. `--mode review` now does the depth work the brief used to ask for: it computes the manifest +(`git diff --numstat ...HEAD`), gives the worker its tier's review contract (read every hunk, callers and +tests, probes, the `Files reviewed: n of N` line), picks the tier from the diff size when `--effort` is omitted +(high up to about 15 files and 800 lines, xhigh above; `max` only when you name it for a risky cross-system PR; +`low` and `medium` need `--allow-shallow`), and proposes a split by folder above about 60 files (`--paths`). +After each turn it gates the depth (coverage against the manifest, reasoning tokens per 100 changed lines, +finding shape): a miss steers the same session once with the unread files, a second miss reruns one tier higher +in a new session (`-`), and after that the review is `shallow`, never done. The brief names the +milestone, the PR and the negative constraints; it no longer restates depth rules. See every review run with +`tools codex reviews [--since 24h] [--pr ] [--label ] [--verdict shallow]`, and one in depth with +`tools codex review-depth `. Astra is the escalation tier, not the default for exploration. Keep the cheaper worker's findings and failed verification when escalating. If a model is unavailable, disclose the fallback and select the nearest suitable tier instead of silently choosing Astra. diff --git a/plugins/genesis-tools/skills/handoff-to/references/codex.md b/plugins/genesis-tools/skills/handoff-to/references/codex.md index 4961a0b639..87208d6cc2 100644 --- a/plugins/genesis-tools/skills/handoff-to/references/codex.md +++ b/plugins/genesis-tools/skills/handoff-to/references/codex.md @@ -15,8 +15,12 @@ The `tools codex spawn` command accepts an explicit model string. Availability s depends on the selected account and backend. Verify the recorded model after dispatch; do not treat a requested model or the parent's config as proof of what ran. Code-review workers should explicitly select Sol; `review_model` does not route arbitrary workers. -Give a review `--effort high`, or `xhigh` above about 50 changed files: at `medium` a review spends about -two thousand reasoning tokens however large the diff is (parent skill, "Effort is the review's depth"). +Spawn a review with `--mode review --base ` (or let it read the branch's PR target or upstream) and omit +`--effort`: the harness picks high or xhigh from the diff size, injects the manifest and the tier's contract, and +gates the depth after each turn (parent skill, "Effort is the review's depth"). At `medium` a review spent about +two thousand reasoning tokens however large the diff was, so `low` and `medium` need `--allow-shallow`. Every run +lands in `~/.genesis-tools/codex/reviews.jsonl`: `tools codex reviews` lists them, `tools codex review-depth ` +shows one. ## Choose the orchestration layer @@ -116,7 +120,7 @@ tools codex spawn --name --account work \ Named login defaults to a new vault-owned grant. `tools codex login work`, `tools ai codex login work`, and `tools ai accounts login work --provider codex` use one core. `--broker` is a compatibility alias. Explicit `--auth-file`/`--home` and `--import-native` are native-file opt-ins, not worker-startup repairs. Do not run login, import or migration without authorization; stop and report auth failures. -Other flags: `--model` / `--effort`, `--mode review|task`, `--session ` when no parent session can be discovered (resolution order: the explicit flag, then `$GENESIS_AGENTS_SESSION`, then `$GT_RENDEZVOUS_SESSION`, and only then the host session id of whoever runs the spawn — Claude Code, Codex or grok. An assigned swarm always outranks an inherited host id, so a nested worker cannot re-parent its children), `--no-agents` to disable the bus (do not — the bus is the point), `--project ` to set the thread's app-server project (without it the project that owns the cwd is used, and a worktree falls back to its main checkout). ⚠️ Codex Desktop ignores that id while its `localProjectTaskMembership` flag is off: it groups by cwd, and it sees a worker's thread only after it re-reads its thread list (`src/codex/README.md`). +Other flags: `--model` / `--effort`, `--mode review|task` (`review` with `--base `, `--allow-shallow`, `--paths `), `--session ` when no parent session can be discovered (resolution order: the explicit flag, then `$GENESIS_AGENTS_SESSION`, then `$GT_RENDEZVOUS_SESSION`, and only then the host session id of whoever runs the spawn — Claude Code, Codex or grok. An assigned swarm always outranks an inherited host id, so a nested worker cannot re-parent its children), `--no-agents` to disable the bus (do not — the bus is the point), `--project ` to set the thread's app-server project (without it the project that owns the cwd is used, and a worktree falls back to its main checkout). ⚠️ Codex Desktop ignores that id while its `localProjectTaskMembership` flag is off: it groups by cwd, and it sees a worker's thread only after it re-reads its thread list (`src/codex/README.md`). Worker records live in `~/.genesis-tools/codex/sessions/.*` (event log, metadata and daemon log). With `--account`, the worker stays bound to that GenesisTools account; without it, native authentication follows the effective home. Worker event logs are separate from native conversation history. From 365c86659c909281d37d599af73a4b314200dfdb Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:14:46 +0200 Subject: [PATCH 06/10] feat(codex): tools codex reviews import backfills the review ledger from sessions that ran before it, with the verdict their coverage line implies and a manifest up to the commit the review saw --- src/codex/README.md | 8 + src/codex/commands/reviews.ts | 85 ++++--- src/codex/lib/review/import.ts | 335 ++++++++++++++++++++++++++++ src/codex/lib/review/ledger.ts | 24 +- src/codex/lib/review/prepare.ts | 29 ++- src/codex/lib/review/report.ts | 16 +- src/codex/lib/review/review.test.ts | 177 ++++++++++++++- src/codex/lib/review/types.ts | 7 +- 8 files changed, 627 insertions(+), 54 deletions(-) create mode 100644 src/codex/lib/review/import.ts diff --git a/src/codex/README.md b/src/codex/README.md index d8c9990537..499a8a632a 100644 --- a/src/codex/README.md +++ b/src/codex/README.md @@ -293,8 +293,16 @@ Every review start, review turn and gate decision is one line of `~/.genesis-too tools codex reviews [--since 24h] [--pr ] [--label ] [--effort ] [--verdict pass|steered|escalated|shallow|open] [--name ] [--json] tools codex review-depth [--json] # one review: manifest, spend, coverage, reasoning per 100 lines, verdict, gate history tools codex review-depth --base # the same checks for a session spawned without --mode review +tools codex reviews import [--base ] [--no-cwd-from-session] [--json] ``` +`reviews import` backfills sessions that ran before the ledger: a start line, one turn line per booked turn of the +worker cost ledger, and a gate line marked `imported: true`. Its verdict comes from the final agent message (from the +session's event log): `pass` for `Files reviewed: n of n`, `shallow` for fewer, `imported` with no such line. The +manifest is `...` in the session's cwd, so fix commits made after +the review do not count; with no base (no `--base`, no PR target, no upstream) or a removed cwd its counts are null. +A session already in the ledger is skipped, so a repeated import adds nothing. + `login` and `usage` are doors onto the provider-neutral account core: the same code runs behind `tools ai accounts login --provider codex` and `tools ai usage --provider codex`. `tools ai accounts discover --provider codex` lists every `~/.codex*` profile on the machine and `--bind` turns the unbound ones into accounts. Sessions are read-only by default. For implementation work, use `--write ask` for supervised approvals or diff --git a/src/codex/commands/reviews.ts b/src/codex/commands/reviews.ts index cfa94646ec..a8cce6488f 100644 --- a/src/codex/commands/reviews.ts +++ b/src/codex/commands/reviews.ts @@ -3,7 +3,6 @@ import { isInteractive, suggestEnumFlag } from "@genesiscz/utils/cli"; import { toolCommand } from "@genesiscz/utils/cli/tool-command"; import { formatDuration, formatTokens } from "@genesiscz/utils/format"; import { logger, out } from "@genesiscz/utils/logger"; -import { isObject } from "@genesiscz/utils/object"; import * as p from "@genesiscz/utils/prompts/p"; import { createBoxTable, formatDotStatus, renderCliHeader, truncateDisplay } from "@genesiscz/utils/table"; import { @@ -15,6 +14,7 @@ import { import type { Command } from "commander"; import pc from "picocolors"; import { checkReview, type ReviewGateChecks } from "../lib/review/gate"; +import { importReviewSessions, lastAgentMessage } from "../lib/review/import"; import { type ReviewGateRecord, type ReviewLedgerRecord, @@ -157,7 +157,7 @@ async function runReviews(options: ReviewsOptions): Promise { table.push([ truncateDisplay(review.label ? `${review.name} ${pc.dim(review.label)}` : review.name, 36), review.pr === undefined ? (review.branch ? truncateDisplay(review.branch, 18) : "—") : `#${review.pr}`, - `${review.files} (${review.lines}L)`, + review.files === null ? "—" : `${review.files} (${review.lines}L)`, review.effort, formatDuration(review.durationMs), String(review.toolCalls), @@ -178,6 +178,47 @@ async function runReviews(options: ReviewsOptions): Promise { out.println(pc.dim(`One review in depth: ${toolCommand("codex review-depth")} `)); } +interface ImportOptions { + base?: string; + cwdFromSession: boolean; + json?: boolean; +} + +async function runImport(names: string[], options: ImportOptions): Promise { + const results = await importReviewSessions({ + names, + ...(options.base ? { base: options.base } : {}), + cwdFromSession: options.cwdFromSession, + }); + + if (options.json) { + out.result({ ledger: reviewLedgerPath(), results }); + return; + } + + const table = createBoxTable(["NAME", "STATUS", "VERDICT", "MANIFEST", "NOTES"]); + + for (const result of results) { + table.push([ + result.name, + result.status, + result.verdict ? verdictCell(result.verdict) : "—", + result.manifest + ? `${result.manifest.files} files, ${result.manifest.lines}L (${result.manifest.base}...${result.manifest.head.slice(0, 10)})` + : "—", + truncateDisplay(result.notes.join("; "), 70), + ]); + } + + out.println(table.toString()); + const imported = results.filter((result) => result.status === "imported").length; + out.println(`${imported} imported, ${results.length - imported} not, into ${reviewLedgerPath()}`); + + if (results.some((result) => result.status === "missing")) { + process.exitCode = 1; + } +} + interface DepthOptions { base?: string; json?: boolean; @@ -218,31 +259,6 @@ function checksLines(checks: ReviewGateChecks): string[] { ]; } -/** The last agent message of the session's own thread, from its event log. */ -async function lastAgentMessage(store: CodexSessionStore, meta: CodexSessionMeta): Promise { - const events = await store.readEvents(meta.name); - - for (const event of events.toReversed()) { - const params = event.params; - - if (event.method !== "item/completed" || !isObject(params) || !isObject(params.item)) { - continue; - } - - const { item, threadId } = params; - - if ( - item.type === "agentMessage" && - typeof item.text === "string" && - (threadId === undefined || threadId === meta.threadId) - ) { - return item.text; - } - } - - return ""; -} - /** A session spawned without `--mode review`: the gate's checks against a manifest computed now from `--base`. */ async function adHocDepth(store: CodexSessionStore, meta: CodexSessionMeta, options: DepthOptions): Promise { if (!options.base) { @@ -357,7 +373,7 @@ async function runReviewDepth(name: string, options: DepthOptions): Promise", "Only reviews active since: a duration back (24h, 7d), HH:MM today, or a date") @@ -371,6 +387,19 @@ export function registerReviewsCommands(program: Command): void { await runReviews(options); }); + reviews + .command("import ") + .description( + "Backfill the ledger from codex sessions that ran before it: start, turns from the cost ledger, and a gate line whose verdict the final Files reviewed line implies" + ) + .option("--base ", "The base of every named session; default each branch's PR target, else its upstream") + .option("--cwd-from-session", "Read the manifest in each session's own cwd (the default)", true) + .option("--no-cwd-from-session", "Leave every manifest null") + .option("--json", "Emit the import results as JSON") + .action(async (names: string[], options: ImportOptions) => { + await runImport(names, options); + }); + program .command("review-depth ") .description( diff --git a/src/codex/lib/review/import.ts b/src/codex/lib/review/import.ts new file mode 100644 index 0000000000..088eb2c03c --- /dev/null +++ b/src/codex/lib/review/import.ts @@ -0,0 +1,335 @@ +import { existsSync } from "node:fs"; +import { readWorkerCostRows, type WorkerCostRow } from "@app/ai-spend/lib/workers/ledger"; +import { logger } from "@genesiscz/utils/logger"; +import { isObject } from "@genesiscz/utils/object"; +import { isReviewEffort, manifestChangedLines, type ReviewManifest } from "@genesiscz/utils/worker/contract"; +import { type CodexSessionMeta, CodexSessionStore } from "../store"; +import { parseReviewReport, REASONING_FLOOR_PER_100_LINES, reasoningPer100Lines } from "./gate"; +import { + appendReviewRecord, + type ReviewLedgerBase, + type ReviewLedgerRecord, + readReviewRecords, + reviewLedgerPath, +} from "./ledger"; +import { buildManifest, type ChosenBase, chooseBase, defaultReviewGitReaders, type ReviewGitReaders } from "./prepare"; +import type { CodexReviewSpent, ReviewVerdict } from "./types"; + +const log = logger.child({ component: "codex:review-import" }); + +/** The last agent message of the session's own thread, from its event log; "" when it has none. */ +export async function lastAgentMessage(store: CodexSessionStore, meta: CodexSessionMeta): Promise { + const events = await store.readEvents(meta.name); + + for (const event of events.toReversed()) { + const params = event.params; + + if (event.method !== "item/completed" || !isObject(params) || !isObject(params.item)) { + continue; + } + + const { item, threadId } = params; + + if ( + item.type === "agentMessage" && + typeof item.text === "string" && + (threadId === undefined || threadId === meta.threadId) + ) { + return item.text; + } + } + + return ""; +} + +/** Everything an import reads; injected so tests need no codex, no git and no network. */ +export interface ReviewImportReaders { + readMeta(name: string): Promise; + /** The session's booked turns from the worker cost ledger, oldest first. */ + costRows(meta: CodexSessionMeta): WorkerCostRow[]; + lastAgentMessage(meta: CodexSessionMeta): Promise; + cwdExists(path: string): boolean; + git: ReviewGitReaders; + /** The review ledger as it is now, for the already-imported check. */ + ledgerRecords(): ReviewLedgerRecord[]; +} + +export function defaultReviewImportReaders(ledgerPath = reviewLedgerPath()): ReviewImportReaders { + const store = new CodexSessionStore(); + + return { + readMeta: async (name) => store.readMeta(name), + costRows: (meta) => + readWorkerCostRows() + .filter( + (row) => + row.backend === "codex" && + row.name === meta.name && + (!meta.threadId || !row.sessionId || row.sessionId === meta.threadId) + ) + .sort((a, b) => Date.parse(a.ts) - Date.parse(b.ts)), + lastAgentMessage: (meta) => lastAgentMessage(store, meta), + cwdExists: (path) => existsSync(path), + git: defaultReviewGitReaders(), + ledgerRecords: () => readReviewRecords(ledgerPath).records, + }; +} + +export interface ReviewImportManifest { + base: string; + /** The commit the manifest ends at: the last one before the session started, else HEAD. */ + head: string; + files: number; + lines: number; +} + +export interface ReviewImportResult { + name: string; + status: "imported" | "skipped" | "missing"; + verdict?: ReviewVerdict; + manifest: ReviewImportManifest | null; + /** Why something is missing or approximate, in one sentence per fact. */ + notes: string[]; +} + +export interface ReviewImportInput { + names: string[]; + /** One base for every named session; without it the PR target or upstream of each session's branch. */ + base?: string; + /** Read the manifest in the session's own cwd (default). False leaves every manifest null. */ + cwdFromSession?: boolean; + readers?: ReviewImportReaders; + ledgerPath?: string; +} + +interface ManifestOutcome { + manifest: ReviewManifest | null; + chosen: ChosenBase | null; + branch: string | null; + head: string | null; + notes: string[]; +} + +async function importManifest(meta: CodexSessionMeta, input: ReviewImportInput, git: ReviewGitReaders) { + const none = (note: string): ManifestOutcome => ({ + manifest: null, + chosen: null, + branch: null, + head: null, + notes: [note], + }); + + if (input.cwdFromSession === false) { + return none("manifest skipped (--no-cwd-from-session): file and line counts are null"); + } + + const cwd = meta.cwd; + const branch = await git.currentBranch(cwd); + let chosen: ChosenBase; + + try { + chosen = await chooseBase({ cwd, ...(input.base ? { base: input.base } : {}), readers: git, branch }); + } catch (err) { + return none(`no base: ${err instanceof Error ? err.message : String(err)} File and line counts are null.`); + } + + // The worktree may have gained fix commits since the review: diff up to the commit the review saw. + const seen = await git.commitBefore(cwd, meta.startedAt); + const notes = seen + ? [ + `manifest ${chosen.ref}...${seen.slice(0, 10)} (the last commit before the session started at ${meta.startedAt})`, + ] + : [`no commit before ${meta.startedAt}; the manifest runs to HEAD and may include later commits`]; + const head = seen ?? "HEAD"; + const manifest = buildManifest(chosen.ref, await git.numstat(cwd, chosen.ref, undefined, head)); + return { manifest, chosen, branch, head, notes } satisfies ManifestOutcome; +} + +function spentOf(rows: readonly WorkerCostRow[]): CodexReviewSpent { + return { + turns: rows.length, + durationMs: rows.reduce((sum, row) => sum + (row.durationMs ?? 0), 0), + toolCalls: rows.reduce((sum, row) => sum + (row.toolCalls ?? 0), 0), + reasoningTokens: rows.reduce((sum, row) => sum + (row.tokens?.reasoning ?? 0), 0), + outputTokens: rows.reduce((sum, row) => sum + (row.tokens?.output ?? 0), 0), + costUsd: rows.some((row) => row.cost.apiEquivalentUsd === null) + ? null + : rows.reduce((sum, row) => sum + (row.cost.apiEquivalentUsd ?? 0), 0), + }; +} + +/** The verdict the final message's coverage line implies: n of n passes, fewer is shallow, none is `imported`. */ +function impliedVerdict( + coverage: { reviewed: number; total: number } | null, + manifest: ReviewManifest | null +): { verdict: ReviewVerdict; reason: string } { + if (!coverage) { + return { + verdict: "imported", + reason: "imported from a session that ran before the ledger; its final message has no Files reviewed line", + }; + } + + const mismatch = + manifest && coverage.total !== manifest.files.length + ? `; the reviewed manifest has ${manifest.files.length} files` + : ""; + + if (coverage.reviewed >= coverage.total) { + return { + verdict: "pass", + reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}${mismatch}`, + }; + } + + return { + verdict: "shallow", + reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}, ${coverage.total - coverage.reviewed} unread${mismatch}`, + }; +} + +async function importOne(name: string, input: ReviewImportInput, readers: ReviewImportReaders, ledgerPath: string) { + const meta = await readers.readMeta(name); + + if (!meta) { + return { + name, + status: "missing", + manifest: null, + notes: ["no codex session record with that name"], + } satisfies ReviewImportResult; + } + + if (readers.ledgerRecords().some((record) => record.name === name)) { + return { + name, + status: "skipped", + manifest: null, + notes: ["already in the review ledger"], + } satisfies ReviewImportResult; + } + + const outcome = readers.cwdExists(meta.cwd) + ? await importManifest(meta, input, readers.git) + : { + manifest: null, + chosen: null, + branch: null, + head: null, + notes: [`${meta.cwd} is gone: file and line counts are null`], + }; + const { manifest, chosen } = outcome; + const rows = readers.costRows(meta); + const report = await readers.lastAgentMessage(meta); + const parsed = parseReviewReport(report); + const notes = [...outcome.notes]; + + if (rows.length === 0) { + notes.push("no booked turns in the worker cost ledger"); + } + + if (!report) { + notes.push("no final agent message in the session's event log"); + } + + const effort = meta.effort ?? "default"; + const base: ReviewLedgerBase = { + v: 1, + ts: meta.startedAt, + name, + chain: name, + ...(meta.label ? { label: meta.label } : {}), + ...(chosen?.pr ? { pr: chosen.pr.number } : {}), + ...(outcome.branch ? { branch: outcome.branch } : {}), + base: chosen?.ref ?? input.base ?? null, + files: manifest ? manifest.files.length : null, + lines: manifest ? manifestChangedLines(manifest) : null, + effort, + imported: true, + ...(outcome.head ? { head: outcome.head } : {}), + }; + const at = (ts: string) => ({ ...base, ts }); + + appendReviewRecord( + base, + { kind: "start", cwd: meta.cwd, effortSource: "imported", allowShallow: false }, + ledgerPath + ); + + rows.forEach((row, index) => { + appendReviewRecord( + at(row.ts), + { + kind: "turn", + turn: row.turn ?? index + 1, + outcome: row.outcome ?? "completed", + model: row.model, + durationMs: row.durationMs, + toolCalls: row.toolCalls ?? 0, + reasoningTokens: row.tokens?.reasoning ?? null, + outputTokens: row.tokens?.output ?? null, + costUsd: row.cost.apiEquivalentUsd, + // Only the final message survives, so only the last turn carries a coverage line. + coverage: index === rows.length - 1 ? parsed.coverage : null, + }, + ledgerPath + ); + }); + + const spent = spentOf(rows); + const { verdict, reason } = impliedVerdict(parsed.coverage, manifest); + appendReviewRecord( + at(rows.at(-1)?.ts ?? meta.lastEventAt), + { + kind: "gate", + step: 0, + verdict, + reason, + coverage: { + reviewed: parsed.coverage?.reviewed ?? null, + total: parsed.coverage?.total ?? manifest?.files.length ?? 0, + }, + reasoningPer100Lines: manifest ? reasoningPer100Lines(spent.reasoningTokens, manifest) : null, + floor: isReviewEffort(effort) ? REASONING_FLOOR_PER_100_LINES[effort] : 0, + findings: parsed.findings.length, + malformedFindings: parsed.findings.filter((finding) => !finding.ok).length, + unread: parsed.notReviewed.map((entry) => entry.path).slice(0, 40), + spent, + }, + ledgerPath + ); + log.info({ name, verdict, files: base.files, base: base.base, head: base.head }, "review imported"); + + return { + name, + status: "imported", + verdict, + manifest: + manifest && chosen + ? { + base: chosen.ref, + head: outcome.head ?? "HEAD", + files: manifest.files.length, + lines: manifestChangedLines(manifest), + } + : null, + notes, + } satisfies ReviewImportResult; +} + +/** + * Backfills the review ledger from codex sessions that ran before it existed: one start line, one turn line per + * booked turn, and one gate line whose verdict the final message's coverage line implies. A session already in the + * ledger is skipped, so a second import of the same names adds nothing. + */ +export async function importReviewSessions(input: ReviewImportInput): Promise { + const ledgerPath = input.ledgerPath ?? reviewLedgerPath(); + const readers = input.readers ?? defaultReviewImportReaders(ledgerPath); + const results: ReviewImportResult[] = []; + + for (const name of [...new Set(input.names)]) { + results.push(await importOne(name, input, readers, ledgerPath)); + } + + return results; +} diff --git a/src/codex/lib/review/ledger.ts b/src/codex/lib/review/ledger.ts index 8d5b5e30f9..5ff7fd99d2 100644 --- a/src/codex/lib/review/ledger.ts +++ b/src/codex/lib/review/ledger.ts @@ -4,7 +4,7 @@ import { readJsonlRows } from "@genesiscz/utils/jsonl"; import { JsonlWriter } from "@genesiscz/utils/log-session/jsonl-writer"; import { logger } from "@genesiscz/utils/logger"; import { isObject } from "@genesiscz/utils/object"; -import { manifestChangedLines, type ReviewEffort } from "@genesiscz/utils/worker/contract"; +import { manifestChangedLines } from "@genesiscz/utils/worker/contract"; import type { CodexReviewConfig, CodexReviewSpent, ReviewVerdict } from "./types"; const log = logger.child({ component: "codex:review-ledger" }); @@ -14,7 +14,7 @@ export function reviewLedgerPath(): string { return join(codexRoot(), "reviews.jsonl"); } -interface ReviewLedgerBase { +export interface ReviewLedgerBase { v: 1; ts: string; name: string; @@ -22,16 +22,22 @@ interface ReviewLedgerBase { label?: string; pr?: number; branch?: string; - base: string; - files: number; - lines: number; - effort: ReviewEffort; + /** Null only on an imported review whose base or worktree could not be found. */ + base: string | null; + files: number | null; + lines: number | null; + /** A review tier; an imported session keeps whatever effort it ran at. */ + effort: string; + /** Backfilled by `tools codex reviews import` from a session that ran before the ledger existed. */ + imported?: true; + /** Imported: the commit the review is taken to have read (the last one before the session started). */ + head?: string; } export interface ReviewStartRecord extends ReviewLedgerBase { kind: "start"; cwd: string; - effortSource: CodexReviewConfig["effortSource"]; + effortSource: CodexReviewConfig["effortSource"] | "imported"; allowShallow: boolean; parent?: string; } @@ -94,7 +100,7 @@ export function reviewLedgerBase(input: { name: string; review: CodexReviewConfi * review turn it describes. The failure is logged with the record's kind and session. */ export function appendReviewRecord( - base: ReturnType, + base: ReviewLedgerBase, entry: ReviewLedgerEntry, path = reviewLedgerPath() ): ReviewLedgerRecord | null { @@ -117,7 +123,7 @@ function isLedgerRecord(value: unknown): value is ReviewLedgerRecord { (value.kind === "start" || value.kind === "turn" || value.kind === "gate") && typeof value.name === "string" && typeof value.ts === "string" && - typeof value.base === "string" + (typeof value.base === "string" || value.base === null) ); } diff --git a/src/codex/lib/review/prepare.ts b/src/codex/lib/review/prepare.ts index 4c070f3974..f161f9eb43 100644 --- a/src/codex/lib/review/prepare.ts +++ b/src/codex/lib/review/prepare.ts @@ -26,8 +26,10 @@ export interface ReviewGitReaders { /** The newest PR of the branch; null when there is none or the host cannot answer. */ pullRequest(cwd: string, branch: string): Promise; upstream(cwd: string, branch: string): Promise; - /** `git diff --numstat ...HEAD`, limited to `paths` when given. */ - numstat(cwd: string, base: string, paths?: string[]): Promise; + /** `git diff --numstat ...` (head defaults to HEAD), limited to `paths` when given. */ + numstat(cwd: string, base: string, paths?: string[], head?: string): Promise; + /** The newest commit of HEAD's history committed at or before `at` (ISO time); null when there is none. */ + commitBefore(cwd: string, at: string): Promise; } export function defaultReviewGitReaders(): ReviewGitReaders { @@ -60,10 +62,20 @@ export function defaultReviewGitReaders(): ReviewGitReaders { return lookup.pr; }, upstream: (cwd, branch) => createGit({ cwd }).getTrackingBranch(branch), - async numstat(cwd, base, paths) { + async numstat(cwd, base, paths, head = "HEAD") { const git = createGit({ cwd }); - const mergeBase = await git.mergeBase(base, "HEAD"); - return git.numstat({ from: mergeBase, to: "HEAD", ...(paths?.length ? { paths } : {}) }); + const mergeBase = await git.mergeBase(base, head); + return git.numstat({ from: mergeBase, to: head, ...(paths?.length ? { paths } : {}) }); + }, + async commitBefore(cwd, at) { + const result = await createGit({ cwd }).executor.exec([ + "log", + "-1", + "--format=%H", + `--before=${at}`, + "HEAD", + ]); + return result.success && result.stdout ? result.stdout : null; }, }; } @@ -87,14 +99,17 @@ export interface PreparedReview { warnings: string[]; } -interface ChosenBase { +export interface ChosenBase { ref: string; source: ReviewBaseSource; detail: string; pr?: PrInfo; } -async function chooseBase(input: PrepareReviewInput & { readers: ReviewGitReaders; branch: string | null }) { +/** The base ladder of `--mode review`: the flag (verified), else the branch's PR target, else its upstream. */ +export async function chooseBase( + input: Pick & { readers: ReviewGitReaders; branch: string | null } +): Promise { const { readers, cwd, branch } = input; if (input.base) { diff --git a/src/codex/lib/review/report.ts b/src/codex/lib/review/report.ts index 4ed8837a57..e5b4ab5a80 100644 --- a/src/codex/lib/review/report.ts +++ b/src/codex/lib/review/report.ts @@ -1,6 +1,5 @@ import { parseSince } from "@genesiscz/utils/date"; import { parseDuration } from "@genesiscz/utils/format"; -import type { ReviewEffort } from "@genesiscz/utils/worker/contract"; import type { ReviewLedgerRecord } from "./ledger"; import type { ReviewVerdict } from "./types"; @@ -11,10 +10,11 @@ export interface ReviewSummary { label?: string; pr?: number; branch?: string; - base: string; - files: number; - lines: number; - effort: ReviewEffort; + base: string | null; + files: number | null; + lines: number | null; + effort: string; + imported?: true; startedAt: string; lastAt: string; turns: number; @@ -65,6 +65,7 @@ function emptySummary(record: ReviewLedgerRecord): ReviewSummary { files: record.files, lines: record.lines, effort: record.effort, + ...(record.imported ? { imported: true as const } : {}), startedAt: record.ts, lastAt: record.ts, turns: 0, @@ -120,7 +121,10 @@ export function summarizeReviews(records: readonly ReviewLedgerRecord[]): Review summary.coverage = `${record.coverage.reviewed}/${record.coverage.total}`; } - summary.reasoningPer100Lines = Math.round((summary.reasoningTokens * 100) / Math.max(1, summary.lines)); + summary.reasoningPer100Lines = + summary.lines === null + ? null + : Math.round((summary.reasoningTokens * 100) / Math.max(1, summary.lines)); continue; } diff --git a/src/codex/lib/review/review.test.ts b/src/codex/lib/review/review.test.ts index c30a91404a..d6e9174932 100644 --- a/src/codex/lib/review/review.test.ts +++ b/src/codex/lib/review/review.test.ts @@ -2,14 +2,17 @@ import { describe, expect, test } from "bun:test"; import { appendFileSync, mkdtempSync, readFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import type { WorkerCostRow } from "@app/ai-spend/lib/workers/ledger"; import type { CodexFinishedTurn } from "@genesiscz/utils/codex/turn-cost"; import type { NumstatEntry, PrInfo } from "@genesiscz/utils/git"; import { SafeJSON } from "@genesiscz/utils/json"; import type { ReviewManifest } from "@genesiscz/utils/worker/contract"; import { codexDriver } from "../driver"; import { spawnCodexSession } from "../spawn"; +import type { CodexSessionMeta } from "../store"; import { escalatedReviewName, markReviewShallow, recordReviewTurn } from "./controller"; import { checkReview, decideGate, parseReviewReport } from "./gate"; +import { importReviewSessions, type ReviewImportReaders } from "./import"; import { type ReviewLedgerRecord, readReviewRecords } from "./ledger"; import { prepareReview, type ReviewGitReaders } from "./prepare"; import { filterReviews, parseReviewSince, reviewTotals, summarizeReviews } from "./report"; @@ -25,6 +28,7 @@ function readers(options: { upstream?: string | null; numstat?: NumstatEntry[]; refs?: string[]; + commitBefore?: (at: string) => string | null; }): ReviewGitReaders & { calls: string[] } { const calls: string[] = []; return { @@ -35,10 +39,11 @@ function readers(options: { refExists: async (_cwd, ref) => (options.refs ?? ["origin/master"]).includes(ref), pullRequest: async () => options.pr ?? null, upstream: async () => options.upstream ?? null, - numstat: async (_cwd, base, paths) => { - calls.push(`numstat ${base}${paths ? ` ${paths.join(",")}` : ""}`); + numstat: async (_cwd, base, paths, head) => { + calls.push(`numstat ${base}${head ? `...${head}` : ""}${paths ? ` ${paths.join(",")}` : ""}`); return options.numstat ?? [entry("src/a.ts", 40, 2)]; }, + commitBefore: async (_cwd, at) => (options.commitBefore ? options.commitBefore(at) : null), }; } @@ -566,3 +571,171 @@ describe("spawn guards", () => { ); }); }); + +function importedMeta(name: string, over: Partial = {}): CodexSessionMeta { + return { + name, + daemonPid: 0, + cwd: `/wt/${name}`, + sandbox: "read-only", + approvalPolicy: "never", + writePolicy: "deny", + status: "closed", + agentName: `codex_${name}`, + rendezvousSession: "s", + agentsEnabled: true, + startedAt: "2026-10-22T19:08:07.954Z", + lastEventAt: "2026-10-22T19:30:24.784Z", + codexVersion: "0.162.1", + pendingApprovals: {}, + effort: "medium", + label: `pr-split-review ${name}`, + threadId: `thread-${name}`, + ...over, + }; +} + +function costRow(name: string, turn: number, over: Partial = {}): WorkerCostRow { + return { + v: 1, + ts: `2026-10-22T19:1${turn}:00.000Z`, + backend: "codex", + name, + sessionId: `thread-${name}`, + turn, + model: "gpt-6.1-sol", + effort: "medium", + auth: "subscription", + account: null, + durationMs: 100_000, + toolCalls: 10, + tokens: { input: 1000, cacheRead: 0, cacheWrite: 0, output: 400, reasoning: 300 }, + cost: { + apiEquivalentUsd: 0.5, + basis: "catalog", + pricedAs: ["gpt-6.1-sol"], + longContextRequests: 0, + auth: "subscription", + billedUsd: 0, + claudeEquivalent: { estimate: true, opusModel: "o", sonnetModel: "s", opusUsd: 1, sonnetUsd: 0.5 }, + }, + cwd: `/wt/${name}`, + outcome: "completed", + ...over, + }; +} + +function importReaders(ledgerPath: string, options: { gone?: string[]; noBase?: boolean } = {}) { + const git = readers({ + pr: options.noBase ? null : PR, + numstat: [entry("src/a.ts", 30, 5), entry("src/b.ts", 10)], + commitBefore: (at) => (at.startsWith("2026-10-22") ? "abc1234567890" : null), + }); + const messages: Record = { + "rv-full": + "1. [High] src/a.ts:3 - bug. Scenario: x. Fix: y.\nRan: bun test\nFiles reviewed: 2 of 2\nNot reviewed: none", + "rv-part": "Findings: none\nRan: nothing\nFiles reviewed: 1 of 2\nNot reviewed: src/b.ts (time)", + "rv-gone": "a summary without the coverage line", + }; + const fake: ReviewImportReaders = { + readMeta: async (name) => (name in messages ? importedMeta(name) : null), + costRows: (meta) => + meta.name === "rv-part" ? [costRow(meta.name, 1), costRow(meta.name, 2)] : [costRow(meta.name, 1)], + lastAgentMessage: async (meta) => messages[meta.name] ?? "", + cwdExists: (path) => !(options.gone ?? []).some((name) => path.endsWith(name)), + git, + ledgerRecords: () => readReviewRecords(ledgerPath).records, + }; + return { fake, git }; +} + +describe("reviews import", () => { + test("backfills start, turns and a gate whose verdict the coverage line implies, diffing up to the commit the review saw", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-")), "reviews.jsonl"); + const { fake, git } = importReaders(ledgerPath, { gone: ["rv-gone"] }); + const results = await importReviewSessions({ + names: ["rv-full", "rv-part", "rv-gone", "rv-none", "rv-full"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + + expect(results.map((result) => [result.name, result.status, result.verdict ?? null])).toEqual([ + ["rv-full", "imported", "pass"], + ["rv-part", "imported", "shallow"], + ["rv-gone", "imported", "imported"], + ["rv-none", "missing", null], + ]); + expect(results[0].manifest).toEqual({ base: "origin/master", head: "abc1234567890", files: 2, lines: 45 }); + expect(results[2].manifest).toBeNull(); + expect(results[2].notes[0]).toContain("/wt/rv-gone is gone"); + expect(git.calls).toEqual(["numstat origin/master...abc1234567890", "numstat origin/master...abc1234567890"]); + + const records = readReviewRecords(ledgerPath).records; + expect(records.map((record) => `${record.name}:${record.kind}`)).toEqual([ + "rv-full:start", + "rv-full:turn", + "rv-full:gate", + "rv-part:start", + "rv-part:turn", + "rv-part:turn", + "rv-part:gate", + "rv-gone:start", + "rv-gone:turn", + "rv-gone:gate", + ]); + expect(records.every((record) => record.imported === true)).toBe(true); + expect(records[2]).toMatchObject({ + kind: "gate", + pr: 12, + files: 2, + lines: 45, + effort: "medium", + head: "abc1234567890", + }); + expect(records[6]).toMatchObject({ + kind: "gate", + reason: "imported: Files reviewed 1 of 2, 1 unread", + unread: ["src/b.ts"], + }); + expect(records[9]).toMatchObject({ + base: "origin/master", + files: null, + lines: null, + reasoningPer100Lines: null, + }); + + const summaries = summarizeReviews(records); + expect(summaries.find((review) => review.name === "rv-part")).toMatchObject({ + turns: 2, + toolCalls: 20, + costUsd: 1, + coverage: "1/2", + reasoningPer100Lines: 1333, + verdict: "shallow", + }); + expect(summaries.find((review) => review.name === "rv-gone")?.reasoningPer100Lines).toBeNull(); + + // A second import of the same names adds nothing. + const again = await importReviewSessions({ names: ["rv-full", "rv-part"], readers: fake, ledgerPath }); + expect(again.map((result) => result.status)).toEqual(["skipped", "skipped"]); + expect(readReviewRecords(ledgerPath).records).toHaveLength(10); + }); + + test("without a base or with --no-cwd-from-session the manifest is null and the note says why", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-")), "reviews.jsonl"); + const { fake } = importReaders(ledgerPath, { noBase: true }); + const [derived] = await importReviewSessions({ names: ["rv-full"], readers: fake, ledgerPath }); + expect(derived.manifest).toBeNull(); + expect(derived.notes[0]).toContain("no base: --mode review needs --base "); + + const [skipped] = await importReviewSessions({ + names: ["rv-part"], + cwdFromSession: false, + readers: fake, + ledgerPath, + }); + expect(skipped.notes[0]).toContain("manifest skipped"); + expect(readReviewRecords(ledgerPath).records.filter((record) => record.kind === "gate")).toHaveLength(2); + }); +}); diff --git a/src/codex/lib/review/types.ts b/src/codex/lib/review/types.ts index bf630babc7..582038caba 100644 --- a/src/codex/lib/review/types.ts +++ b/src/codex/lib/review/types.ts @@ -1,7 +1,10 @@ import type { ReviewEffort, ReviewManifest } from "@genesiscz/utils/worker/contract"; -/** What the depth gate decided about one review turn. */ -export const REVIEW_VERDICTS = ["pass", "steered", "escalated", "shallow"] as const; +/** + * What the depth gate decided about one review turn. `imported` is never decided by the gate: it marks a review + * backfilled from a session that ran before the ledger and whose final message has no coverage line. + */ +export const REVIEW_VERDICTS = ["pass", "steered", "escalated", "shallow", "imported"] as const; export type ReviewVerdict = (typeof REVIEW_VERDICTS)[number]; /** Where the base ref came from; printed at spawn, because a derived base is a guess the caller must see. */ From 0ac817380f44a083cd1041167aac54fcdcfc9bf3 Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 22 Oct 2026 23:15:37 +0200 Subject: [PATCH 07/10] fix(codex): reviews import logs each import to the day log only, since its table already prints the result --- src/codex/lib/review/import.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/codex/lib/review/import.ts b/src/codex/lib/review/import.ts index 088eb2c03c..1f4084d00e 100644 --- a/src/codex/lib/review/import.ts +++ b/src/codex/lib/review/import.ts @@ -298,7 +298,7 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review }, ledgerPath ); - log.info({ name, verdict, files: base.files, base: base.base, head: base.head }, "review imported"); + log.debug({ name, verdict, files: base.files, base: base.base, head: base.head }, "review imported"); return { name, From f2aa258c8e71592afdf12d5872053bb1354c3739 Mon Sep 17 00:00:00 2001 From: Martin Date: Fri, 23 Oct 2026 14:38:46 +0200 Subject: [PATCH 08/10] fix(codex, babysit): #507 followup - the review gate keeps underscores and spaces in paths and fails a named unread file, records each check for review-depth, which reads only the current run, an import fails loudly and redoes a partial one, and the review spawn line quotes every value --- .../skills/babysit/scripts/review-brief.ts | 18 +- .../skills/babysit/scripts/scripts.test.ts | 28 ++- src/codex/commands/reviews.ts | 46 +--- src/codex/lib/review/controller.ts | 9 + src/codex/lib/review/gate.ts | 43 +++- src/codex/lib/review/import.ts | 123 +++++++--- src/codex/lib/review/ledger.ts | 30 ++- src/codex/lib/review/report.ts | 72 +++++- src/codex/lib/review/review.test.ts | 223 +++++++++++++++++- 9 files changed, 497 insertions(+), 95 deletions(-) diff --git a/.claude/skills/babysit/scripts/review-brief.ts b/.claude/skills/babysit/scripts/review-brief.ts index 17e62eeb1a..9a2ae26908 100644 --- a/.claude/skills/babysit/scripts/review-brief.ts +++ b/.claude/skills/babysit/scripts/review-brief.ts @@ -19,6 +19,7 @@ import { join } from "node:path"; import { SafeJSON } from "@genesiscz/utils/json"; +import { shellWord } from "@genesiscz/utils/shell/quote"; import { flag, has, loadRun, reviewScratch } from "./run"; export interface ReviewBriefInput { @@ -75,13 +76,22 @@ export interface ReviewSpawnInput { postMerge?: { mergeCommit: string; files: string[] }; } -/** The reviewer's spawn line: `--mode review` with the base, so the harness computes the manifest and gates the depth. */ +/** + * The reviewer's spawn line: `--mode review` with the base, so the harness computes the manifest and gates the depth. + * Every value is one shell word: a file name, branch or path from the repository may hold a space or `$(...)`, and + * the lead runs this line as printed. + */ export function reviewSpawnCommand({ pr, wt, base, briefPath, postMerge }: ReviewSpawnInput): string { const target = postMerge - ? `--base ${postMerge.mergeCommit}^ --paths ${postMerge.files.join(" ")}` - : `--base origin/${base}`; + ? ["--base", `${postMerge.mergeCommit}^`, "--paths", ...postMerge.files] + : ["--base", `origin/${base}`]; + const argv = [ + ...["tools", "codex", "spawn", "--mode", "review", ...target], + ...["--name", `bs-${pr}-review`, "--model", "gpt-6.1-sol", "--write", "ask", "--cwd", wt], + ...["--label", `babysit pr${pr} review`, "--prompt-file", briefPath], + ]; - return `tools codex spawn --mode review ${target} --name bs-${pr}-review --model gpt-6.1-sol --write ask --cwd ${wt} --label "babysit pr${pr} review" --prompt-file ${briefPath}`; + return argv.map(shellWord).join(" "); } interface PrView { diff --git a/.claude/skills/babysit/scripts/scripts.test.ts b/.claude/skills/babysit/scripts/scripts.test.ts index fcc0084437..d023e87657 100644 --- a/.claude/skills/babysit/scripts/scripts.test.ts +++ b/.claude/skills/babysit/scripts/scripts.test.ts @@ -673,7 +673,7 @@ describe("independent review brief", () => { expect( reviewSpawnCommand({ pr: 498, wt: "/w", base: "feat/parent", briefPath: "/run/steer-review-498.md" }) ).toBe( - 'tools codex spawn --mode review --base origin/feat/parent --name bs-498-review --model gpt-6.1-sol --write ask --cwd /w --label "babysit pr498 review" --prompt-file /run/steer-review-498.md' + "tools codex spawn --mode review --base origin/feat/parent --name bs-498-review --model gpt-6.1-sol --write ask --cwd /w --label 'babysit pr498 review' --prompt-file /run/steer-review-498.md" ); expect( reviewSpawnCommand({ @@ -683,7 +683,31 @@ describe("independent review brief", () => { briefPath: "/b.md", postMerge: { mergeCommit: "abc123", files: ["src/a.ts", "src/b.ts"] }, }) - ).toContain("--mode review --base abc123^ --paths src/a.ts src/b.ts --name bs-498-review"); + ).toContain("--mode review --base 'abc123^' --paths src/a.ts src/b.ts --name bs-498-review"); + }); + + test("every value of the spawn line stays one literal shell word: spaces and $(...) in names do not split or run", () => { + const command = reviewSpawnCommand({ + pr: 498, + wt: "/tmp/my wt", + base: "feat/$(id)", + briefPath: "/run/brief file.md", + postMerge: { mergeCommit: "abc123", files: ["src/my file.ts", "$(touch /tmp/review-probe).ts"] }, + }); + expect(command).toContain("--paths 'src/my file.ts' '$(touch /tmp/review-probe).ts' --name"); + expect(command).toContain("--cwd '/tmp/my wt'"); + expect(command).toContain("--prompt-file '/run/brief file.md'"); + + // The shell itself reads it back as the intended argv, without running the substitution. + const parsed = Bun.spawnSync(["sh", "-c", `printf '%s\\n' ${command}`], { env: process.env, stdout: "pipe" }); + const argv = parsed.stdout.toString().trimEnd().split("\n"); + expect(argv).toContain("src/my file.ts"); + expect(argv).toContain("$(touch /tmp/review-probe).ts"); + expect(argv).toContain("/tmp/my wt"); + expect(argv[argv.indexOf("--label") + 1]).toBe("babysit pr498 review"); + expect(reviewSpawnCommand({ pr: 498, wt: "/w", base: "feat/$(id)", briefPath: "/b.md" })).toContain( + "--base 'origin/feat/$(id)'" + ); }); }); describe("planted-regression summary", () => { diff --git a/src/codex/commands/reviews.ts b/src/codex/commands/reviews.ts index a8cce6488f..009d3512f3 100644 --- a/src/codex/commands/reviews.ts +++ b/src/codex/commands/reviews.ts @@ -15,15 +15,12 @@ import type { Command } from "commander"; import pc from "picocolors"; import { checkReview, type ReviewGateChecks } from "../lib/review/gate"; import { importReviewSessions, lastAgentMessage } from "../lib/review/import"; -import { - type ReviewGateRecord, - type ReviewLedgerRecord, - readReviewRecords, - reviewLedgerPath, -} from "../lib/review/ledger"; +import { type ReviewGateRecord, readReviewRecords, reviewLedgerPath } from "../lib/review/ledger"; import { buildManifest, defaultReviewGitReaders } from "../lib/review/prepare"; import { + currentReviewRecords, filterReviews, + gateCheckRows, parseReviewSince, type ReviewSummary, reviewTotals, @@ -184,13 +181,19 @@ interface ImportOptions { json?: boolean; } -async function runImport(names: string[], options: ImportOptions): Promise { +/** Exported for the test of both output formats; the CLI reaches it through `reviews import`. */ +export async function runImport(names: string[], options: ImportOptions): Promise { const results = await importReviewSessions({ names, ...(options.base ? { base: options.base } : {}), cwdFromSession: options.cwdFromSession, }); + // Before either output: automation reading --json needs the same exit status as a person reading the table. + if (results.some((result) => result.status === "missing" || result.status === "failed")) { + process.exitCode = 1; + } + if (options.json) { out.result({ ledger: reviewLedgerPath(), results }); return; @@ -213,10 +216,6 @@ async function runImport(names: string[], options: ImportOptions): Promise out.println(table.toString()); const imported = results.filter((result) => result.status === "imported").length; out.println(`${imported} imported, ${results.length - imported} not, into ${reviewLedgerPath()}`); - - if (results.some((result) => result.status === "missing")) { - process.exitCode = 1; - } } interface DepthOptions { @@ -229,26 +228,7 @@ function checkLine(label: string, ok: boolean, detail: string): string { } function gateLines(gate: ReviewGateRecord): string[] { - const coverageOk = gate.coverage.reviewed !== null && gate.coverage.reviewed >= gate.coverage.total; - const reasoningOk = gate.reasoningPer100Lines === null || gate.reasoningPer100Lines >= gate.floor; - - return [ - checkLine( - "coverage", - coverageOk, - `${gate.coverage.reviewed ?? "no line"} of ${gate.coverage.total} files${gate.unread.length > 0 ? `; unread: ${gate.unread.slice(0, 8).join(", ")}${gate.unread.length > 8 ? ", …" : ""}` : ""}` - ), - checkLine( - "reasoning", - reasoningOk, - `${gate.reasoningPer100Lines ?? "unknown"} tokens per 100 changed lines (floor ${gate.floor})` - ), - checkLine( - "shape", - gate.malformedFindings === 0, - `${gate.findings} findings, ${gate.malformedFindings} malformed` - ), - ]; + return gateCheckRows(gate).map((row) => checkLine(row.label, row.ok, row.detail)); } function checksLines(checks: ReviewGateChecks): string[] { @@ -319,9 +299,7 @@ async function runReviewDepth(name: string, options: DepthOptions): Promise record.name === name && record.chain === review.chain - ); + const records = currentReviewRecords(readReviewRecords().records, name, review.chain); const gates = records.filter((record): record is ReviewGateRecord => record.kind === "gate"); const lastGate = gates.at(-1); diff --git a/src/codex/lib/review/controller.ts b/src/codex/lib/review/controller.ts index 5f77f3022a..9f09b87a4e 100644 --- a/src/codex/lib/review/controller.ts +++ b/src/codex/lib/review/controller.ts @@ -163,6 +163,15 @@ function writeGateRecord(options: { findings: checks?.shape.findings ?? 0, malformedFindings: checks?.shape.malformed.length ?? 0, unread: checks?.coverage.unread.slice(0, UNREAD_RECORDED) ?? [], + ...(checks + ? { + checks: { + coverage: { ok: checks.coverage.ok, detail: checks.coverage.detail }, + reasoning: { ok: checks.reasoning.ok, detail: checks.reasoning.detail }, + shape: { ok: checks.shape.ok, detail: checks.shape.detail }, + }, + } + : {}), ...(options.escalatedTo ? { escalatedTo: options.escalatedTo } : {}), spent: review.spent ?? { turns: 0, diff --git a/src/codex/lib/review/gate.ts b/src/codex/lib/review/gate.ts index ca6b2fcdd4..8e9222c164 100644 --- a/src/codex/lib/review/gate.ts +++ b/src/codex/lib/review/gate.ts @@ -35,13 +35,27 @@ export interface ParsedReviewReport { ran: boolean; } -/** Markdown emphasis and code ticks a model wraps around the fixed lines; they carry no meaning here. */ +/** + * A pair of `*`, `**`, `_` or `__` around a phrase: opened after a space or the line start, closed before a space, + * punctuation or the line end. An underscore inside a word (`app_icon.png`, `__init__.py`) is not emphasis. + */ +const EMPHASIS = /(^|[\s([])(\*{1,2}|_{1,2})(?=\S)(.*?\S)\2(?=$|[\s)\]:,;!?]|\.(?:\s|$))/g; + +/** Markdown emphasis, code ticks and a bullet a model wraps around the fixed lines; they carry no meaning here. */ function plain(line: string): string { - return line.replace(/[*_`]/g, "").trim(); + let text = line.replace(/`/g, "").replace(/^\s*[*-]\s+/, ""); + + // Twice at most: `***both***` is one pair inside another. + for (let pass = 0; pass < 2; pass++) { + text = text.replace(EMPHASIS, "$1$3"); + } + + return text.trim(); } const FINDING_START = /^(\d+)[.)]\s+(.*)$/; -const FINDING_SHAPE = /^\[(critical|high|medium|low)\]\s+\S+:\d+/i; +/** `[Severity] path:line`: the path runs up to its `:line`, so a path with spaces in it (`src/my file.ts:12`) parses. */ +const FINDING_SHAPE = /^\[(critical|high|medium|low)\]\s+\S(?:[^:]*?\S)?:\d+(?=$|[\s:,;)\-\u2013\u2014])/i; const SECTION_END = /^(ran|files reviewed|not reviewed|result|at|changed|verify|open|findings):/i; function parseNotReviewed(value: string): ParsedReviewReport["notReviewed"] { @@ -137,7 +151,12 @@ export function reasoningPer100Lines(reasoningTokens: number | null, manifest: R return Math.round((reasoningTokens * 100) / Math.max(1, manifestChangedLines(manifest))); } -function checkCoverage( +/** + * Coverage against the manifest: the reported total is the manifest's, the reviewed count plus the excused binary + * files reach it, and the Not reviewed line names no text file. Exported for `reviews import`, which reaches the + * same verdict from a backfilled final message. + */ +export function checkCoverage( parsed: ParsedReviewReport, manifest: ReviewManifest, report: string @@ -156,15 +175,19 @@ function checkCoverage( const { reviewed } = parsed.coverage; const binary = new Set(manifest.files.filter((file) => file.binary).map((file) => file.path)); - // A binary file has no text to read: named with a reason, it does not count as unread. - const excused = parsed.notReviewed.filter((entry) => entry.reason && binary.has(entry.path)).length; - const ok = parsed.coverage.total === total && reviewed + excused >= total; + // A binary file has no text to read: named with a reason, it does not count as unread. Each one counts once, + // so naming the same binary twice cannot make up for a text file left unread. + const excused = new Set( + parsed.notReviewed.filter((entry) => entry.reason && binary.has(entry.path)).map((entry) => entry.path) + ).size; + // A text file on the Not reviewed line is an admitted gap, whatever the count says. + const named = [...new Set(parsed.notReviewed.map((entry) => entry.path).filter((path) => !binary.has(path)))]; + const ok = parsed.coverage.total === total && reviewed + excused >= total && named.length === 0; if (ok) { return { ok, detail: `${reviewed} of ${total} files reviewed`, reviewed, total, unread: [] }; } - const named = parsed.notReviewed.map((entry) => entry.path).filter((path) => !binary.has(path)); const missing = total - reviewed - excused; // The worker named fewer files than it skipped: the manifest files its report never mentions are the best guess. const unmentioned = @@ -174,7 +197,9 @@ function checkCoverage( const detail = parsed.coverage.total !== total ? `reported ${reviewed} of ${parsed.coverage.total}, but the manifest has ${total} files` - : `${reviewed} of ${total} files reviewed`; + : reviewed + excused >= total + ? `${reviewed} of ${total} files reviewed, but Not reviewed names ${named.length} text file(s)` + : `${reviewed} of ${total} files reviewed`; return { ok, detail, reviewed, total, unread: [...new Set([...named, ...unmentioned])] }; } diff --git a/src/codex/lib/review/import.ts b/src/codex/lib/review/import.ts index 1f4084d00e..c19cbcd98f 100644 --- a/src/codex/lib/review/import.ts +++ b/src/codex/lib/review/import.ts @@ -4,9 +4,15 @@ import { logger } from "@genesiscz/utils/logger"; import { isObject } from "@genesiscz/utils/object"; import { isReviewEffort, manifestChangedLines, type ReviewManifest } from "@genesiscz/utils/worker/contract"; import { type CodexSessionMeta, CodexSessionStore } from "../store"; -import { parseReviewReport, REASONING_FLOOR_PER_100_LINES, reasoningPer100Lines } from "./gate"; import { - appendReviewRecord, + checkCoverage, + type ParsedReviewReport, + parseReviewReport, + REASONING_FLOOR_PER_100_LINES, + reasoningPer100Lines, +} from "./gate"; +import { + appendReviewRecords, type ReviewLedgerBase, type ReviewLedgerRecord, readReviewRecords, @@ -85,7 +91,8 @@ export interface ReviewImportManifest { export interface ReviewImportResult { name: string; - status: "imported" | "skipped" | "missing"; + /** `failed`: the ledger write failed, nothing of this review is in the ledger, and a rerun imports it. */ + status: "imported" | "skipped" | "missing" | "failed"; verdict?: ReviewVerdict; manifest: ReviewImportManifest | null; /** Why something is missing or approximate, in one sentence per fact. */ @@ -158,11 +165,18 @@ function spentOf(rows: readonly WorkerCostRow[]): CodexReviewSpent { }; } -/** The verdict the final message's coverage line implies: n of n passes, fewer is shallow, none is `imported`. */ +/** + * The verdict the final message's coverage line implies: fewer files than its own total is `shallow`; a pass needs + * the gate's coverage check against the rebuilt manifest. A full count whose scope cannot be confirmed (no manifest, + * another total, an unread text file named) stays `imported`, and so does a message with no coverage line. + */ function impliedVerdict( - coverage: { reviewed: number; total: number } | null, - manifest: ReviewManifest | null + parsed: ParsedReviewReport, + manifest: ReviewManifest | null, + report: string ): { verdict: ReviewVerdict; reason: string } { + const { coverage } = parsed; + if (!coverage) { return { verdict: "imported", @@ -175,17 +189,39 @@ function impliedVerdict( ? `; the reviewed manifest has ${manifest.files.length} files` : ""; - if (coverage.reviewed >= coverage.total) { + if (coverage.reviewed < coverage.total) { return { - verdict: "pass", - reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}${mismatch}`, + verdict: "shallow", + reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}, ${coverage.total - coverage.reviewed} unread${mismatch}`, }; } - return { - verdict: "shallow", - reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}, ${coverage.total - coverage.reviewed} unread${mismatch}`, - }; + if (!manifest) { + return { + verdict: "imported", + reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}, but no manifest to confirm the scope`, + }; + } + + const check = checkCoverage(parsed, manifest, report); + + return check.ok + ? { verdict: "pass", reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}` } + : { verdict: "imported", reason: `imported: ${check.detail}; the scope is not confirmed` }; +} + +/** + * Whether the ledger already holds this name: a live review line, or an import that reached its gate line. An + * imported start without a gate is a partial import (an older run whose later writes failed): it is redone. + */ +function ledgerState(records: readonly ReviewLedgerRecord[], name: string): "absent" | "present" | "partial" { + const mine = records.filter((record) => record.name === name); + + if (mine.length === 0) { + return "absent"; + } + + return mine.some((record) => !record.imported || record.kind === "gate") ? "present" : "partial"; } async function importOne(name: string, input: ReviewImportInput, readers: ReviewImportReaders, ledgerPath: string) { @@ -200,7 +236,9 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review } satisfies ReviewImportResult; } - if (readers.ledgerRecords().some((record) => record.name === name)) { + const state = ledgerState(readers.ledgerRecords(), name); + + if (state === "present") { return { name, status: "skipped", @@ -224,6 +262,10 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review const parsed = parseReviewReport(report); const notes = [...outcome.notes]; + if (state === "partial") { + notes.push("an earlier import of this name has no gate line; this import replaces it"); + } + if (rows.length === 0) { notes.push("no booked turns in the worker cost ledger"); } @@ -248,18 +290,14 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review imported: true, ...(outcome.head ? { head: outcome.head } : {}), }; - const at = (ts: string) => ({ ...base, ts }); - - appendReviewRecord( - base, - { kind: "start", cwd: meta.cwd, effortSource: "imported", allowShallow: false }, - ledgerPath - ); - - rows.forEach((row, index) => { - appendReviewRecord( - at(row.ts), - { + const spent = spentOf(rows); + const { verdict, reason } = impliedVerdict(parsed, manifest, report); + const records: ReviewLedgerRecord[] = [ + { ...base, kind: "start", cwd: meta.cwd, effortSource: "imported", allowShallow: false }, + ...rows.map( + (row, index): ReviewLedgerRecord => ({ + ...base, + ts: row.ts, kind: "turn", turn: row.turn ?? index + 1, outcome: row.outcome ?? "completed", @@ -271,16 +309,11 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review costUsd: row.cost.apiEquivalentUsd, // Only the final message survives, so only the last turn carries a coverage line. coverage: index === rows.length - 1 ? parsed.coverage : null, - }, - ledgerPath - ); - }); - - const spent = spentOf(rows); - const { verdict, reason } = impliedVerdict(parsed.coverage, manifest); - appendReviewRecord( - at(rows.at(-1)?.ts ?? meta.lastEventAt), + }) + ), { + ...base, + ts: rows.at(-1)?.ts ?? meta.lastEventAt, kind: "gate", step: 0, verdict, @@ -296,8 +329,19 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review unread: parsed.notReviewed.map((entry) => entry.path).slice(0, 40), spent, }, - ledgerPath - ); + ]; + // One write for the whole review: a failure leaves nothing behind that the already-imported check would trust. + const written = appendReviewRecords(records, ledgerPath); + + if (!written.ok) { + return { + name, + status: "failed", + manifest: null, + notes: [...notes, `the review ledger write failed (${written.error}); nothing was imported`], + } satisfies ReviewImportResult; + } + log.debug({ name, verdict, files: base.files, base: base.base, head: base.head }, "review imported"); return { @@ -319,8 +363,9 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review /** * Backfills the review ledger from codex sessions that ran before it existed: one start line, one turn line per - * booked turn, and one gate line whose verdict the final message's coverage line implies. A session already in the - * ledger is skipped, so a second import of the same names adds nothing. + * booked turn, and one gate line whose verdict the final message's coverage line implies, written together. A + * session already in the ledger is skipped, so a second import of the same names adds nothing; one whose earlier + * import never reached its gate line is imported again. */ export async function importReviewSessions(input: ReviewImportInput): Promise { const ledgerPath = input.ledgerPath ?? reviewLedgerPath(); diff --git a/src/codex/lib/review/ledger.ts b/src/codex/lib/review/ledger.ts index 5ff7fd99d2..a94d538caa 100644 --- a/src/codex/lib/review/ledger.ts +++ b/src/codex/lib/review/ledger.ts @@ -1,10 +1,13 @@ -import { join } from "node:path"; +import { appendFileSync, mkdirSync } from "node:fs"; +import { dirname, join } from "node:path"; import { codexRoot } from "@genesiscz/utils/codex/worker-paths"; +import { SafeJSON } from "@genesiscz/utils/json"; import { readJsonlRows } from "@genesiscz/utils/jsonl"; import { JsonlWriter } from "@genesiscz/utils/log-session/jsonl-writer"; import { logger } from "@genesiscz/utils/logger"; import { isObject } from "@genesiscz/utils/object"; import { manifestChangedLines } from "@genesiscz/utils/worker/contract"; +import type { GateCheck } from "./gate"; import type { CodexReviewConfig, CodexReviewSpent, ReviewVerdict } from "./types"; const log = logger.child({ component: "codex:review-ledger" }); @@ -68,6 +71,11 @@ export interface ReviewGateRecord extends ReviewLedgerBase { malformedFindings: number; /** The unread files the gate named, first 40. */ unread: string[]; + /** + * Each check's own result and sentence, as the gate decided it. Absent on a gate line written before this field, + * on an imported one and on a `markReviewShallow` one; `review-depth` then rebuilds an approximation. + */ + checks?: { coverage: GateCheck; reasoning: GateCheck; shape: GateCheck }; escalatedTo?: string; spent: CodexReviewSpent; } @@ -116,6 +124,26 @@ export function appendReviewRecord( } } +/** + * Appends several records in one write, so they land together or not at all (a torn last line is skipped by the + * reader). Unlike {@link appendReviewRecord} it reports the failure: `reviews import` must not call a review + * imported when its lines never reached the ledger. + */ +export function appendReviewRecords( + records: readonly ReviewLedgerRecord[], + path = reviewLedgerPath() +): { ok: true } | { ok: false; error: string } { + try { + mkdirSync(dirname(path), { recursive: true }); + appendFileSync(path, records.map((record) => `${SafeJSON.stringify(record, { jsonl: true })}\n`).join("")); + log.debug({ path, records: records.length }, "review ledger records written"); + return { ok: true }; + } catch (err) { + log.warn({ err, path, records: records.length }, "review ledger write failed"); + return { ok: false, error: err instanceof Error ? err.message : String(err) }; + } +} + function isLedgerRecord(value: unknown): value is ReviewLedgerRecord { return ( isObject(value) && diff --git a/src/codex/lib/review/report.ts b/src/codex/lib/review/report.ts index e5b4ab5a80..a9f13205df 100644 --- a/src/codex/lib/review/report.ts +++ b/src/codex/lib/review/report.ts @@ -1,6 +1,6 @@ import { parseSince } from "@genesiscz/utils/date"; import { parseDuration } from "@genesiscz/utils/format"; -import type { ReviewLedgerRecord } from "./ledger"; +import type { ReviewGateRecord, ReviewLedgerRecord } from "./ledger"; import type { ReviewVerdict } from "./types"; /** One review session, folded from its ledger lines: what `tools codex reviews` prints as a row. */ @@ -82,7 +82,8 @@ function emptySummary(record: ReviewLedgerRecord): ReviewSummary { /** * Folds the ledger into one summary per review session. A `start` line opens a new review, so a name that was - * reused later shows twice; turn and gate lines without a start (a ledger cut by hand) open one too. + * reused later shows twice; turn and gate lines without a start (a ledger cut by hand) open one too. An imported + * review that never got its gate line is a partial import: the next import of that name replaces its row. */ export function summarizeReviews(records: readonly ReviewLedgerRecord[]): ReviewSummary[] { const open = new Map(); @@ -92,6 +93,16 @@ export function summarizeReviews(records: readonly ReviewLedgerRecord[]): Review let summary = open.get(record.name); if (!summary || record.kind === "start") { + if ( + summary && + record.kind === "start" && + record.imported && + summary.imported && + summary.verdict === "open" + ) { + all.splice(all.indexOf(summary), 1); + } + summary = emptySummary(record); open.set(record.name, summary); all.push(summary); @@ -141,6 +152,63 @@ export function summarizeReviews(records: readonly ReviewLedgerRecord[]): Review return all; } +/** + * The ledger lines of one review session's current run. A reused name (and a fresh spawn, whose chain is its name) + * matches every earlier run too, so only the lines from its last `start` on count; with no start line, all of them. + */ +export function currentReviewRecords( + records: readonly ReviewLedgerRecord[], + name: string, + chain: string +): ReviewLedgerRecord[] { + const mine = records.filter((record) => record.name === name && record.chain === chain); + const start = mine.findLastIndex((record) => record.kind === "start"); + return start < 0 ? mine : mine.slice(start); +} + +export interface GateCheckRow { + label: "coverage" | "reasoning" | "shape"; + ok: boolean; + detail: string; +} + +/** + * The three checks of one gate line as `review-depth` prints them: the result and sentence the gate recorded, or + * for an older line without them an approximation from its counts. + */ +export function gateCheckRows(gate: ReviewGateRecord): GateCheckRow[] { + const unread = + gate.unread.length > 0 + ? `; unread: ${gate.unread.slice(0, 8).join(", ")}${gate.unread.length > 8 ? ", …" : ""}` + : ""; + + if (gate.checks) { + return [ + { label: "coverage", ...gate.checks.coverage, detail: `${gate.checks.coverage.detail}${unread}` }, + { label: "reasoning", ...gate.checks.reasoning }, + { label: "shape", ...gate.checks.shape }, + ]; + } + + return [ + { + label: "coverage", + ok: gate.coverage.reviewed !== null && gate.coverage.reviewed >= gate.coverage.total, + detail: `${gate.coverage.reviewed ?? "no line"} of ${gate.coverage.total} files${unread}`, + }, + { + label: "reasoning", + ok: gate.reasoningPer100Lines === null || gate.reasoningPer100Lines >= gate.floor, + detail: `${gate.reasoningPer100Lines ?? "unknown"} tokens per 100 changed lines (floor ${gate.floor})`, + }, + { + label: "shape", + ok: gate.malformedFindings === 0, + detail: `${gate.findings} findings, ${gate.malformedFindings} malformed`, + }, + ]; +} + /** `24h`, `90m`, `7d` back from now; otherwise the `--since` grammar of the other reports (`HH:MM`, a date). */ export function parseReviewSince(value: string, now = new Date()): Date | null { const text = value.trim(); diff --git a/src/codex/lib/review/review.test.ts b/src/codex/lib/review/review.test.ts index d6e9174932..5049155615 100644 --- a/src/codex/lib/review/review.test.ts +++ b/src/codex/lib/review/review.test.ts @@ -1,21 +1,31 @@ -import { describe, expect, test } from "bun:test"; -import { appendFileSync, mkdtempSync, readFileSync } from "node:fs"; +import { describe, expect, spyOn, test } from "bun:test"; +import { appendFileSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { WorkerCostRow } from "@app/ai-spend/lib/workers/ledger"; import type { CodexFinishedTurn } from "@genesiscz/utils/codex/turn-cost"; +import { env } from "@genesiscz/utils/env"; import type { NumstatEntry, PrInfo } from "@genesiscz/utils/git"; import { SafeJSON } from "@genesiscz/utils/json"; +import { out } from "@genesiscz/utils/logger"; import type { ReviewManifest } from "@genesiscz/utils/worker/contract"; +import { runImport } from "../../commands/reviews"; import { codexDriver } from "../driver"; import { spawnCodexSession } from "../spawn"; import type { CodexSessionMeta } from "../store"; import { escalatedReviewName, markReviewShallow, recordReviewTurn } from "./controller"; import { checkReview, decideGate, parseReviewReport } from "./gate"; import { importReviewSessions, type ReviewImportReaders } from "./import"; -import { type ReviewLedgerRecord, readReviewRecords } from "./ledger"; +import { type ReviewGateRecord, type ReviewLedgerRecord, readReviewRecords } from "./ledger"; import { prepareReview, type ReviewGitReaders } from "./prepare"; -import { filterReviews, parseReviewSince, reviewTotals, summarizeReviews } from "./report"; +import { + currentReviewRecords, + filterReviews, + gateCheckRows, + parseReviewSince, + reviewTotals, + summarizeReviews, +} from "./report"; import type { CodexReviewConfig } from "./types"; function entry(path: string, insertions: number, deletions = 0, binary = false): NumstatEntry { @@ -247,6 +257,67 @@ describe("the depth gate", () => { expect(decideGate(miss, { effort: "medium", step: 0, allowShallow: true }).verdict).toBe("shallow"); expect(decideGate(miss, { effort: "low", step: 0, allowShallow: true }).verdict).toBe("shallow"); }); + + test("a text file on the Not reviewed line fails coverage even when the count is full; a repeated binary counts once", () => { + const admitted = checkReview({ + report: GOOD_REPORT.replace("Files reviewed: 3 of 4", "Files reviewed: 4 of 4").replace( + "Not reviewed: assets/logo.png (binary image)", + "Not reviewed: src/c.ts (ran out of time)" + ), + manifest: MANIFEST, + effort: "high", + reasoningTokens: 400, + }); + expect(admitted.coverage).toMatchObject({ ok: false, unread: ["src/c.ts"] }); + expect(admitted.coverage.detail).toContain("Not reviewed names 1 text file(s)"); + + const twoBinaries: ReviewManifest = { + ...MANIFEST, + files: [...MANIFEST.files, { path: "assets/icon.png", added: 0, deleted: 0, binary: true }], + }; + const repeated = checkReview({ + report: GOOD_REPORT.replace("Files reviewed: 3 of 4", "Files reviewed: 3 of 5").replace( + "Not reviewed: assets/logo.png (binary image)", + "Not reviewed: assets/logo.png (binary image), assets/logo.png (binary image)" + ), + manifest: twoBinaries, + effort: "high", + reasoningTokens: 400, + }); + expect(repeated.coverage.ok).toBe(false); + }); + + test("markdown is stripped around words, never inside a path: underscores and spaces in paths survive", () => { + const manifest: ReviewManifest = { + base: "origin/master", + files: [ + { path: "src/my_mod.ts", added: 60, deleted: 10 }, + { path: "src/my file.ts", added: 30, deleted: 0 }, + { path: "assets/app_icon.png", added: 0, deleted: 0, binary: true }, + ], + added: 90, + deleted: 10, + }; + const report = [ + "1. **[High]** src/my_mod.ts:12 - the lock leaks. Scenario: a throw. Fix: release in finally.", + "2. [Medium] src/my file.ts:7 - stale read. Scenario: a retry. Fix: reload.", + "__Ran:__ `bun test src/my_mod.test.ts`", + "**Files reviewed:** 2 of 3", + "_Not reviewed:_ `assets/app_icon.png` (binary image)", + ].join("\n"); + const parsed = parseReviewReport(report); + + expect(parsed.notReviewed).toEqual([{ path: "assets/app_icon.png", reason: "binary image" }]); + expect(parsed.findings.map((finding) => [finding.ok, finding.text.split(" - ")[0]])).toEqual([ + [true, "[High] src/my_mod.ts:12"], + [true, "[Medium] src/my file.ts:7"], + ]); + expect(parsed.ran).toBe(true); + + const checks = checkReview({ report, manifest, effort: "high", reasoningTokens: 400 }); + expect(checks.coverage).toMatchObject({ ok: true, unread: [] }); + expect(checks.shape.ok).toBe(true); + }); }); function finishedTurn(over: Partial = {}): CodexFinishedTurn { @@ -319,6 +390,11 @@ describe("recordReviewTurn", () => { reasoningTokens: 50, }); expect(records[1]).toMatchObject({ verdict: "steered", step: 1, reasoningPer100Lines: 50, floor: 100 }); + expect((records[1] as ReviewGateRecord).checks).toEqual({ + coverage: { ok: false, detail: "1 of 4 files reviewed" }, + reasoning: { ok: false, detail: "50 reasoning tokens per 100 changed lines, high floor 100" }, + shape: { ok: true, detail: "0 findings parse" }, + }); const second = recordReviewTurn({ name: "rv", @@ -517,6 +593,42 @@ describe("the review ledger report", () => { ]); }); + test("review-depth reads only the current run of a reused name", () => { + const [start, turn, gate] = ledgerFixture(); + const again = { ...start, ts: "2026-10-23T09:00:00Z" }; + + expect(currentReviewRecords([start, turn, gate, again], "rv", "rv")).toEqual([again]); + expect(currentReviewRecords([turn, gate], "rv", "rv")).toEqual([turn, gate]); + }); + + test("review-depth prints the checks the gate recorded, and approximates only for a line without them", () => { + const gate = ledgerFixture()[2] as ReviewGateRecord; + // 3 of 4 with an excused binary passed the gate; a missing Ran line failed shape with no malformed finding. + const recorded: ReviewGateRecord = { + ...gate, + coverage: { reviewed: 3, total: 4 }, + unread: [], + checks: { + coverage: { ok: true, detail: "3 of 4 files reviewed" }, + reasoning: { ok: true, detail: "400 reasoning tokens per 100 changed lines, high floor 100" }, + shape: { ok: false, detail: "no `Ran:` line" }, + }, + }; + expect(gateCheckRows(recorded).map((row) => [row.label, row.ok, row.detail])).toEqual([ + ["coverage", true, "3 of 4 files reviewed"], + ["reasoning", true, "400 reasoning tokens per 100 changed lines, high floor 100"], + ["shape", false, "no `Ran:` line"], + ]); + + // Without the recorded checks the counts are all there is: coverage reads as a miss, shape as clean. + const { checks: _dropped, ...older } = recorded; + expect(gateCheckRows(older).map((row) => [row.label, row.ok])).toEqual([ + ["coverage", false], + ["reasoning", false], + ["shape", true], + ]); + }); + test("filters by PR, label, tier, verdict, name and time", () => { const reviews = summarizeReviews(ledgerFixture()); const names = (filters: Parameters[1]) => @@ -739,3 +851,106 @@ describe("reviews import", () => { expect(readReviewRecords(ledgerPath).records.filter((record) => record.kind === "gate")).toHaveLength(2); }); }); + +describe("reviews import: verdicts and failed writes", () => { + test("a full count whose total differs from the rebuilt manifest stays imported, never pass", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-")), "reviews.jsonl"); + const { fake } = importReaders(ledgerPath); + const mismatched: ReviewImportReaders = { + ...fake, + lastAgentMessage: async () => + "1. [High] src/a.ts:3 - bug. Scenario: x. Fix: y.\nRan: bun test\nFiles reviewed: 1 of 1\nNot reviewed: none", + }; + const [result] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: mismatched, + ledgerPath, + }); + + expect(result.verdict).toBe("imported"); + expect(summarizeReviews(readReviewRecords(ledgerPath).records)[0].verdict).toBe("imported"); + expect(readReviewRecords(ledgerPath).records.at(-1)).toMatchObject({ + kind: "gate", + reason: "imported: reported 1 of 1, but the manifest has 2 files; the scope is not confirmed", + }); + }); + + test("a failed ledger write is reported, and a partial earlier import is redone instead of skipped", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-review-import-")); + const blocker = join(dir, "not-a-folder"); + writeFileSync(blocker, ""); + const unwritable = join(blocker, "reviews.jsonl"); + const { fake: failing } = importReaders(unwritable); + const [failed] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: { ...failing, ledgerRecords: () => [] }, + ledgerPath: unwritable, + }); + expect(failed.status).toBe("failed"); + expect(failed.notes.at(-1)).toContain("the review ledger write failed"); + + // An older import wrote its start line and then failed: the rerun completes it and its row replaces the stub. + const ledgerPath = join(dir, "reviews.jsonl"); + const { fake } = importReaders(ledgerPath); + const stub = { + v: 1, + ts: "2026-10-22T19:00:00.000Z", + name: "rv-full", + chain: "rv-full", + base: "origin/master", + files: 2, + lines: 45, + effort: "medium", + imported: true, + kind: "start", + cwd: "/wt/rv-full", + effortSource: "imported", + allowShallow: false, + }; + appendFileSync(ledgerPath, `${SafeJSON.stringify(stub)}\n`); + const [redone] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + expect(redone).toMatchObject({ status: "imported", verdict: "pass" }); + expect(redone.notes).toContain("an earlier import of this name has no gate line; this import replaces it"); + expect( + summarizeReviews(readReviewRecords(ledgerPath).records).map((review) => [review.name, review.verdict]) + ).toEqual([["rv-full", "pass"]]); + + const [again] = await importReviewSessions({ names: ["rv-full"], readers: fake, ledgerPath }); + expect(again.status).toBe("skipped"); + }); + + test("tools codex reviews import exits 1 for a missing session in table and --json output alike", async () => { + const home = mkdtempSync(join(tmpdir(), "gt-review-import-cli-")); + const previousExitCode = process.exitCode; + const printed: unknown[] = []; + const result = spyOn(out, "result").mockImplementation((value) => { + printed.push(value); + }); + const println = spyOn(out, "println").mockImplementation(() => {}); + // Read through a call: after the reset below, TypeScript would narrow a direct read to undefined. + const exitCode = (): typeof process.exitCode => process.exitCode; + + try { + await env.testing.withOverrides({ GENESIS_TOOLS_HOME: home }, async () => { + for (const json of [true, false]) { + process.exitCode = undefined; + await runImport(["absent-review"], { cwdFromSession: true, json }); + expect(exitCode()).toBe(1); + } + }); + expect(printed).toHaveLength(1); + expect(printed[0]).toMatchObject({ results: [{ name: "absent-review", status: "missing" }] }); + } finally { + result.mockRestore(); + println.mockRestore(); + process.exitCode = previousExitCode; + } + }); +}); From 8209c8f72a1e16d6d51485d077fcb72f2491e0b6 Mon Sep 17 00:00:00 2001 From: Martin Date: Fri, 23 Oct 2026 14:49:42 +0200 Subject: [PATCH 09/10] fix(codex): #507 followup - a review ledger append after a torn last line starts on its own line, so a redone import never folds the old batch's turns into its row --- src/codex/lib/review/ledger.ts | 31 +++++++++++++++++++++++++++-- src/codex/lib/review/review.test.ts | 31 +++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/codex/lib/review/ledger.ts b/src/codex/lib/review/ledger.ts index a94d538caa..190e094909 100644 --- a/src/codex/lib/review/ledger.ts +++ b/src/codex/lib/review/ledger.ts @@ -1,4 +1,4 @@ -import { appendFileSync, mkdirSync } from "node:fs"; +import { appendFileSync, closeSync, existsSync, fstatSync, mkdirSync, openSync, readSync } from "node:fs"; import { dirname, join } from "node:path"; import { codexRoot } from "@genesiscz/utils/codex/worker-paths"; import { SafeJSON } from "@genesiscz/utils/json"; @@ -135,7 +135,13 @@ export function appendReviewRecords( ): { ok: true } | { ok: false; error: string } { try { mkdirSync(dirname(path), { recursive: true }); - appendFileSync(path, records.map((record) => `${SafeJSON.stringify(record, { jsonl: true })}\n`).join("")); + // A torn last line (a full disk or a killed writer) must stay its own line: appended onto, it would swallow + // this batch's first record, and a redone import's turns would fold into the old start's row. + const lead = endsMidLine(path) ? "\n" : ""; + appendFileSync( + path, + lead + records.map((record) => `${SafeJSON.stringify(record, { jsonl: true })}\n`).join("") + ); log.debug({ path, records: records.length }, "review ledger records written"); return { ok: true }; } catch (err) { @@ -144,6 +150,27 @@ export function appendReviewRecords( } } +/** True when the file exists, is not empty and its last byte is not a newline. */ +function endsMidLine(path: string): boolean { + if (!existsSync(path)) { + return false; + } + + const fd = openSync(path, "r"); + try { + const size = fstatSync(fd).size; + if (size === 0) { + return false; + } + + const last = Buffer.alloc(1); + readSync(fd, last, 0, 1, size - 1); + return last[0] !== 0x0a; + } finally { + closeSync(fd); + } +} + function isLedgerRecord(value: unknown): value is ReviewLedgerRecord { return ( isObject(value) && diff --git a/src/codex/lib/review/review.test.ts b/src/codex/lib/review/review.test.ts index 5049155615..3d078616bb 100644 --- a/src/codex/lib/review/review.test.ts +++ b/src/codex/lib/review/review.test.ts @@ -926,6 +926,37 @@ describe("reviews import: verdicts and failed writes", () => { expect(again.status).toBe("skipped"); }); + test("a redo after a torn last line keeps the line boundary, so the old batch's turns never fold into the new row", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-torn-")), "reviews.jsonl"); + const { fake } = importReaders(ledgerPath); + const [first] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + expect(first.status).toBe("imported"); + const clean = summarizeReviews(readReviewRecords(ledgerPath).records); + + // The same batch again, torn inside its gate line: complete start and turn lines, then an unterminated fragment. + const text = readFileSync(ledgerPath, "utf8"); + const gateAt = text.lastIndexOf('{"v":1'); + writeFileSync(ledgerPath, text.slice(0, gateAt + 20)); + const [redone] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + expect(redone.status).toBe("imported"); + + const { records, skipped } = readReviewRecords(ledgerPath); + expect(skipped).toBe(1); + expect(summarizeReviews(records).map((row) => [row.name, row.verdict, row.turns, row.costUsd])).toEqual( + clean.map((row) => [row.name, row.verdict, row.turns, row.costUsd]) + ); + }); + test("tools codex reviews import exits 1 for a missing session in table and --json output alike", async () => { const home = mkdtempSync(join(tmpdir(), "gt-review-import-cli-")); const previousExitCode = process.exitCode; From 2b6bdbbba86e476e981c648472cba5601b1f9c38 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 26 Oct 2026 01:57:11 +0100 Subject: [PATCH 10/10] fix(codex, babysit): #592 followup - a review pins its range and PR, an import recovers the historical base and head, and live ledger writes survive a torn line --- .../skills/babysit/scripts/review-brief.ts | 2 +- .../skills/babysit/scripts/scripts.test.ts | 4 +- src/codex/README.md | 10 +- src/codex/lib/driver.ts | 7 +- src/codex/lib/review/import.ts | 73 ++++--- src/codex/lib/review/ledger.ts | 15 +- src/codex/lib/review/prepare.ts | 51 +++-- src/codex/lib/review/review.test.ts | 186 +++++++++++++++++- src/utils/worker/contract.test.ts | 15 +- src/utils/worker/contract.ts | 9 +- 10 files changed, 303 insertions(+), 69 deletions(-) diff --git a/.claude/skills/babysit/scripts/review-brief.ts b/.claude/skills/babysit/scripts/review-brief.ts index 9a2ae26908..9f976df1e0 100644 --- a/.claude/skills/babysit/scripts/review-brief.ts +++ b/.claude/skills/babysit/scripts/review-brief.ts @@ -86,7 +86,7 @@ export function reviewSpawnCommand({ pr, wt, base, briefPath, postMerge }: Revie ? ["--base", `${postMerge.mergeCommit}^`, "--paths", ...postMerge.files] : ["--base", `origin/${base}`]; const argv = [ - ...["tools", "codex", "spawn", "--mode", "review", ...target], + ...["tools", "codex", "spawn", "--mode", "review", "--pr", String(pr), ...target], ...["--name", `bs-${pr}-review`, "--model", "gpt-6.1-sol", "--write", "ask", "--cwd", wt], ...["--label", `babysit pr${pr} review`, "--prompt-file", briefPath], ]; diff --git a/.claude/skills/babysit/scripts/scripts.test.ts b/.claude/skills/babysit/scripts/scripts.test.ts index d023e87657..35bc041b51 100644 --- a/.claude/skills/babysit/scripts/scripts.test.ts +++ b/.claude/skills/babysit/scripts/scripts.test.ts @@ -673,7 +673,7 @@ describe("independent review brief", () => { expect( reviewSpawnCommand({ pr: 498, wt: "/w", base: "feat/parent", briefPath: "/run/steer-review-498.md" }) ).toBe( - "tools codex spawn --mode review --base origin/feat/parent --name bs-498-review --model gpt-6.1-sol --write ask --cwd /w --label 'babysit pr498 review' --prompt-file /run/steer-review-498.md" + "tools codex spawn --mode review --pr 498 --base origin/feat/parent --name bs-498-review --model gpt-6.1-sol --write ask --cwd /w --label 'babysit pr498 review' --prompt-file /run/steer-review-498.md" ); expect( reviewSpawnCommand({ @@ -683,7 +683,7 @@ describe("independent review brief", () => { briefPath: "/b.md", postMerge: { mergeCommit: "abc123", files: ["src/a.ts", "src/b.ts"] }, }) - ).toContain("--mode review --base 'abc123^' --paths src/a.ts src/b.ts --name bs-498-review"); + ).toContain("--mode review --pr 498 --base 'abc123^' --paths src/a.ts src/b.ts --name bs-498-review"); }); test("every value of the spawn line stays one literal shell word: spaces and $(...) in names do not split or run", () => { diff --git a/src/codex/README.md b/src/codex/README.md index 499a8a632a..730817755b 100644 --- a/src/codex/README.md +++ b/src/codex/README.md @@ -273,7 +273,8 @@ spelling, hidden from help. remote copy); the spawn prints which. It runs `git diff --numstat ...HEAD` in the worker's cwd (limited to `--paths ` when given) and refuses an empty diff. The worker's developer instructions carry the shared worker contract plus `reviewLines(effort)` from `src/utils/worker/contract.ts` and the manifest (`Files to review (N, +A -D)` -and one line per file). +and one line per file). The manifest pins the merge-base and head commits, so a steer or escalation reviews the +same range after either branch moves. Pass `--pr ` to record an explicit PR identity in detached reviews. The tier is `--effort`, else the diff size: `high` up to 15 files and 800 changed lines, `xhigh` above. `max` only when named. `low` (triage) and `medium` are refused unless `--allow-shallow`. Above 60 files or 4,000 lines the spawn prints @@ -299,8 +300,11 @@ tools codex reviews import [--base ] [--no-cwd-from-session] [--j `reviews import` backfills sessions that ran before the ledger: a start line, one turn line per booked turn of the worker cost ledger, and a gate line marked `imported: true`. Its verdict comes from the final agent message (from the session's event log): `pass` for `Files reviewed: n of n`, `shallow` for fewer, `imported` with no such line. The -manifest is `...` in the session's cwd, so fix commits made after -the review do not count; with no base (no `--base`, no PR target, no upstream) or a removed cwd its counts are null. +manifest uses the session's saved pinned commits when available, even if its cwd is gone. For older sessions it +reconstructs both HEAD and the chosen base from their reflogs at the session's start time, then diffs their +merge-base to that historical head. Missing reflogs, an unknown base or a removed cwd leave counts null and scope +unconfirmed. Commit dates alone cannot establish when a ref moved. Named binary files with a reason get the same +coverage exemption as live reviews. A session already in the ledger is skipped, so a repeated import adds nothing. `login` and `usage` are doors onto the provider-neutral account core: the same code runs behind `tools ai accounts login --provider codex` and `tools ai usage --provider codex`. `tools ai accounts discover --provider codex` lists every `~/.codex*` profile on the machine and `--bind` turns the unbound ones into accounts. diff --git a/src/codex/lib/driver.ts b/src/codex/lib/driver.ts index 759aa82692..751f6e2dda 100644 --- a/src/codex/lib/driver.ts +++ b/src/codex/lib/driver.ts @@ -70,6 +70,7 @@ export const codexDriver: WorkerDriver = { "--base ", "--mode review: the ref to diff against (git diff ...HEAD); default the branch's PR target, else its upstream" ) + .option("--pr ", "--mode review: explicit PR identity, including detached post-merge reviews") .option("--allow-shallow", "--mode review: accept --effort low or medium, which the gate never tunes up") .option( "--paths ", @@ -113,6 +114,7 @@ export const codexDriver: WorkerDriver = { writableRoot?: string[]; project?: string; base?: string; + pr?: string; allowShallow?: boolean; paths?: string[]; }; @@ -121,8 +123,8 @@ export const codexDriver: WorkerDriver = { throw new Error("--mode must be review or task"); } - if (extras.mode === "task" && (extras.base || extras.allowShallow || extras.paths)) { - throw new Error("--base, --allow-shallow and --paths apply to --mode review only"); + if (extras.mode === "task" && (extras.base || extras.pr !== undefined || extras.allowShallow || extras.paths)) { + throw new Error("--base, --pr, --allow-shallow and --paths apply to --mode review only"); } // A silent no-op here reads as isolation that was applied. It never was, so this @@ -145,6 +147,7 @@ export const codexDriver: WorkerDriver = { ? await prepareReview({ cwd: input.cwd, ...(extras.base ? { base: extras.base } : {}), + ...(extras.pr === undefined ? {} : { pr: Number(extras.pr) }), ...(extras.effort === undefined ? {} : { effort: extras.effort }), allowShallow: extras.allowShallow === true, ...(extras.paths ? { paths: extras.paths } : {}), diff --git a/src/codex/lib/review/import.ts b/src/codex/lib/review/import.ts index c19cbcd98f..cbe2a36ca0 100644 --- a/src/codex/lib/review/import.ts +++ b/src/codex/lib/review/import.ts @@ -83,7 +83,7 @@ export function defaultReviewImportReaders(ledgerPath = reviewLedgerPath()): Rev export interface ReviewImportManifest { base: string; - /** The commit the manifest ends at: the last one before the session started, else HEAD. */ + /** The saved review head, or HEAD's value in the reflog when the session started. */ head: string; files: number; lines: number; @@ -130,6 +130,16 @@ async function importManifest(meta: CodexSessionMeta, input: ReviewImportInput, return none("manifest skipped (--no-cwd-from-session): file and line counts are null"); } + if (meta.review?.manifest.head) { + return { + manifest: meta.review.manifest, + chosen: { ref: meta.review.manifest.base, source: meta.review.baseSource, detail: meta.review.baseDetail }, + branch: meta.review.branch ?? null, + head: meta.review.manifest.head, + notes: ["manifest restored from the session's pinned review commits"], + } satisfies ManifestOutcome; + } + const cwd = meta.cwd; const branch = await git.currentBranch(cwd); let chosen: ChosenBase; @@ -140,16 +150,24 @@ async function importManifest(meta: CodexSessionMeta, input: ReviewImportInput, return none(`no base: ${err instanceof Error ? err.message : String(err)} File and line counts are null.`); } - // The worktree may have gained fix commits since the review: diff up to the commit the review saw. - const seen = await git.commitBefore(cwd, meta.startedAt); - const notes = seen - ? [ - `manifest ${chosen.ref}...${seen.slice(0, 10)} (the last commit before the session started at ${meta.startedAt})`, - ] - : [`no commit before ${meta.startedAt}; the manifest runs to HEAD and may include later commits`]; - const head = seen ?? "HEAD"; - const manifest = buildManifest(chosen.ref, await git.numstat(cwd, chosen.ref, undefined, head)); - return { manifest, chosen, branch, head, notes } satisfies ManifestOutcome; + // Both refs must be historical: today's target can already contain the reviewed head. + const head = await git.commitBefore(cwd, meta.startedAt); + const historicalBase = await git.commitBefore(cwd, meta.startedAt, chosen.ref); + + if (!head || !historicalBase) { + return { + ...none( + `historical head or base before ${meta.startedAt} cannot be established; file and line counts are null` + ), + chosen, + branch, + }; + } + + const base = await git.mergeBase(cwd, historicalBase, head); + const manifest = { ...buildManifest(base, await git.numstat(cwd, base, undefined, head)), head }; + const notes = [`manifest ${base}...${head} reconstructed from both reflogs at ${meta.startedAt}`]; + return { manifest, chosen: { ...chosen, ref: base }, branch, head, notes } satisfies ManifestOutcome; } function spentOf(rows: readonly WorkerCostRow[]): CodexReviewSpent { @@ -189,6 +207,12 @@ function impliedVerdict( ? `; the reviewed manifest has ${manifest.files.length} files` : ""; + const check = manifest ? checkCoverage(parsed, manifest, report) : null; + + if (check?.ok) { + return { verdict: "pass", reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}` }; + } + if (coverage.reviewed < coverage.total) { return { verdict: "shallow", @@ -203,11 +227,7 @@ function impliedVerdict( }; } - const check = checkCoverage(parsed, manifest, report); - - return check.ok - ? { verdict: "pass", reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}` } - : { verdict: "imported", reason: `imported: ${check.detail}; the scope is not confirmed` }; + return { verdict: "imported", reason: `imported: ${check?.detail}; the scope is not confirmed` }; } /** @@ -247,15 +267,16 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review } satisfies ReviewImportResult; } - const outcome = readers.cwdExists(meta.cwd) - ? await importManifest(meta, input, readers.git) - : { - manifest: null, - chosen: null, - branch: null, - head: null, - notes: [`${meta.cwd} is gone: file and line counts are null`], - }; + const outcome = + meta.review?.manifest.head || readers.cwdExists(meta.cwd) + ? await importManifest(meta, input, readers.git) + : { + manifest: null, + chosen: null, + branch: null, + head: null, + notes: [`${meta.cwd} is gone: file and line counts are null`], + }; const { manifest, chosen } = outcome; const rows = readers.costRows(meta); const report = await readers.lastAgentMessage(meta); @@ -281,7 +302,7 @@ async function importOne(name: string, input: ReviewImportInput, readers: Review name, chain: name, ...(meta.label ? { label: meta.label } : {}), - ...(chosen?.pr ? { pr: chosen.pr.number } : {}), + ...(meta.review?.pr !== undefined ? { pr: meta.review.pr } : chosen?.pr ? { pr: chosen.pr.number } : {}), ...(outcome.branch ? { branch: outcome.branch } : {}), base: chosen?.ref ?? input.base ?? null, files: manifest ? manifest.files.length : null, diff --git a/src/codex/lib/review/ledger.ts b/src/codex/lib/review/ledger.ts index 190e094909..c5c62afbe3 100644 --- a/src/codex/lib/review/ledger.ts +++ b/src/codex/lib/review/ledger.ts @@ -3,7 +3,6 @@ import { dirname, join } from "node:path"; import { codexRoot } from "@genesiscz/utils/codex/worker-paths"; import { SafeJSON } from "@genesiscz/utils/json"; import { readJsonlRows } from "@genesiscz/utils/jsonl"; -import { JsonlWriter } from "@genesiscz/utils/log-session/jsonl-writer"; import { logger } from "@genesiscz/utils/logger"; import { isObject } from "@genesiscz/utils/object"; import { manifestChangedLines } from "@genesiscz/utils/worker/contract"; @@ -33,7 +32,7 @@ export interface ReviewLedgerBase { effort: string; /** Backfilled by `tools codex reviews import` from a session that ran before the ledger existed. */ imported?: true; - /** Imported: the commit the review is taken to have read (the last one before the session started). */ + /** The pinned review head, or the historical HEAD recovered for an import. */ head?: string; } @@ -96,7 +95,8 @@ export function reviewLedgerBase(input: { name: string; review: CodexReviewConfi ...(input.label ? { label: input.label } : {}), ...(review.pr === undefined ? {} : { pr: review.pr }), ...(review.branch ? { branch: review.branch } : {}), - base: review.base, + base: review.manifest.base, + ...(review.manifest.head ? { head: review.manifest.head } : {}), files: review.manifest.files.length, lines: manifestChangedLines(review.manifest), effort: review.effort, @@ -114,14 +114,7 @@ export function appendReviewRecord( ): ReviewLedgerRecord | null { const record: ReviewLedgerRecord = { ...base, ...entry }; - try { - new JsonlWriter(path).append({ ...record }); - log.debug({ path, kind: record.kind, name: record.name }, "review ledger record written"); - return record; - } catch (err) { - log.warn({ err, path, kind: record.kind, name: record.name }, "review ledger write failed"); - return null; - } + return appendReviewRecords([record], path).ok ? record : null; } /** diff --git a/src/codex/lib/review/prepare.ts b/src/codex/lib/review/prepare.ts index f161f9eb43..f7f34a8ce1 100644 --- a/src/codex/lib/review/prepare.ts +++ b/src/codex/lib/review/prepare.ts @@ -23,13 +23,15 @@ export interface ReviewGitReaders { /** `origin/` when it exists, else the local branch, else null. */ resolveBranch(cwd: string, name: string): Promise; refExists(cwd: string, ref: string): Promise; + resolveCommit(cwd: string, ref: string): Promise; + mergeBase(cwd: string, base: string, head: string): Promise; /** The newest PR of the branch; null when there is none or the host cannot answer. */ pullRequest(cwd: string, branch: string): Promise; upstream(cwd: string, branch: string): Promise; /** `git diff --numstat ...` (head defaults to HEAD), limited to `paths` when given. */ numstat(cwd: string, base: string, paths?: string[], head?: string): Promise; - /** The newest commit of HEAD's history committed at or before `at` (ISO time); null when there is none. */ - commitBefore(cwd: string, at: string): Promise; + /** The ref's value at `at` (ISO time), from its reflog; null when the historical value is unavailable. */ + commitBefore(cwd: string, at: string, ref?: string): Promise; } export function defaultReviewGitReaders(): ReviewGitReaders { @@ -46,6 +48,8 @@ export function defaultReviewGitReaders(): ReviewGitReaders { return (await git.refExists(`refs/heads/${short}`)) ? short : null; }, refExists: (cwd, ref) => createGit({ cwd }).refExists(`${ref}^{commit}`), + resolveCommit: (cwd, ref) => createGit({ cwd }).getSha(`${ref}^{commit}`), + mergeBase: (cwd, base, head) => createGit({ cwd }).mergeBase(base, head), async pullRequest(cwd, branch) { const driver = await originDriver(cwd); @@ -67,15 +71,24 @@ export function defaultReviewGitReaders(): ReviewGitReaders { const mergeBase = await git.mergeBase(base, head); return git.numstat({ from: mergeBase, to: head, ...(paths?.length ? { paths } : {}) }); }, - async commitBefore(cwd, at) { - const result = await createGit({ cwd }).executor.exec([ - "log", - "-1", - "--format=%H", - `--before=${at}`, - "HEAD", - ]); - return result.success && result.stdout ? result.stdout : null; + async commitBefore(cwd, at, ref = "HEAD") { + const git = createGit({ cwd }); + const resolved = await git.executor.exec(["rev-parse", "--symbolic-full-name", ref]); + const reflogRef = ref === "HEAD" ? "HEAD" : resolved.stdout; + + if (!resolved.success || !reflogRef) { + return null; + } + + const exists = await git.executor.exec(["reflog", "exists", reflogRef]); + + if (!exists.success) { + return null; + } + + const result = await git.executor.exec(["rev-parse", "--verify", `${reflogRef}@{${at}}`]); + // Git returns the oldest entry with a warning when the requested date predates the reflog. + return result.success && result.stdout && !result.stderr ? result.stdout : null; }, }; } @@ -83,6 +96,8 @@ export function defaultReviewGitReaders(): ReviewGitReaders { export interface PrepareReviewInput { cwd: string; base?: string; + /** Explicit identity for detached reviews; the base still comes from --base. */ + pr?: number; effort?: string; allowShallow?: boolean; /** Limit the manifest to these paths: one slice of a split review. */ @@ -287,7 +302,17 @@ export async function prepareReview(input: PrepareReviewInput): Promise string | null; + commitBefore?: (at: string, ref?: string) => string | null; }): ReviewGitReaders & { calls: string[] } { const calls: string[] = []; return { calls, + resolveCommit: async (_cwd, ref) => (ref === "HEAD" ? "head-sha" : ref), + mergeBase: async (_cwd, base) => base, currentBranch: async () => (options.branch === undefined ? "feat/x" : options.branch), resolveBranch: async (_cwd, name) => (options.refs ?? ["origin/master"]).includes(`origin/${name}`) ? `origin/${name}` : null, @@ -53,7 +63,7 @@ function readers(options: { calls.push(`numstat ${base}${head ? `...${head}` : ""}${paths ? ` ${paths.join(",")}` : ""}`); return options.numstat ?? [entry("src/a.ts", 40, 2)]; }, - commitBefore: async (_cwd, at) => (options.commitBefore ? options.commitBefore(at) : null), + commitBefore: async (_cwd, at, ref) => (options.commitBefore ? options.commitBefore(at, ref) : null), }; } @@ -66,7 +76,7 @@ describe("prepareReview", () => { expect(prepared.config).toMatchObject({ base: "origin/master", baseSource: "flag", pr: 12, branch: "feat/x" }); expect(prepared.notes[0]).toBe("review base origin/master (flag: --base)"); - expect(fake.calls).toEqual(["numstat origin/master"]); + expect(fake.calls).toEqual(["numstat origin/master...head-sha"]); await expect(prepareReview({ cwd: "/repo", base: "origin/nope", readers: fake })).rejects.toThrow( "--base origin/nope does not resolve" ); @@ -95,7 +105,53 @@ describe("prepareReview", () => { const fake = readers({}); await prepareReview({ cwd: "/repo", base: "origin/master", paths: ["src/codex"], readers: fake }); - expect(fake.calls).toEqual(["numstat origin/master src/codex"]); + expect(fake.calls).toEqual(["numstat origin/master...head-sha src/codex"]); + }); + + test("a detached review keeps its explicit PR identity in the ledger and filters", async () => { + const prepared = await prepareReview({ + cwd: "/repo", + base: "origin/master", + pr: 592, + readers: readers({ branch: null }), + }); + const review = reviewConfig(prepared.config); + const record: ReviewLedgerRecord = { + ...reviewLedgerBase({ name: "detached", review }), + kind: "start", + cwd: "/repo", + effortSource: "diff-size", + allowShallow: false, + }; + expect(filterReviews(summarizeReviews([record]), { pr: 592 })).toHaveLength(1); + expect(prepared.config.pr).toBe(592); + for (const pr of [0, -1, 1.5, Number.NaN]) { + await expect( + prepareReview({ cwd: "/repo", base: "origin/master", pr, readers: readers({}) }) + ).rejects.toThrow("--pr must be a positive integer"); + } + }); + + test("pins both endpoints before counting, and escalation keeps them after refs move", async () => { + let head = "head-before"; + let base = "base-before"; + const fake = readers({}); + fake.resolveCommit = async (_cwd, ref) => (ref === "HEAD" ? head : base); + fake.mergeBase = async (_cwd, from, to) => { + expect([from, to]).toEqual(["base-before", "head-before"]); + return "merge-before"; + }; + const prepared = await prepareReview({ cwd: "/repo", base: "origin/master", readers: fake }); + head = "head-after"; + base = "base-after"; + expect(fake.calls).toEqual(["numstat merge-before...head-before"]); + expect(prepared.config.manifest).toMatchObject({ base: "merge-before", head: "head-before" }); + const escalated = escalatedReview(reviewConfig(prepared.config), { effort: "xhigh", meta: importedMeta("rv") }); + expect(escalated.manifest).toEqual(prepared.config.manifest); + expect(reviewLedgerBase({ name: "rv-xhigh", review: escalated })).toMatchObject({ + base: "merge-before", + head: "head-before", + }); }); test("the tier comes from the diff size; low and medium need --allow-shallow", async () => { @@ -652,6 +708,24 @@ describe("the review ledger report", () => { expect(parseReviewSince("bogus", now)).toBeNull(); }); + test("live starts, turns and gates recover a torn last line and keep a reused name's runs separate", () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-live-torn-")), "reviews.jsonl"); + const [start, turn, gate] = ledgerFixture(); + for (const record of [start, turn, gate]) { + appendFileSync(ledgerPath, '{"v":1,"kind":"torn'); + expect(appendReviewRecord(record, record, ledgerPath)).toEqual(record); + } + const recovered = readReviewRecords(ledgerPath); + expect(recovered.skipped).toBe(3); + expect(recovered.records).toEqual([start, turn, gate]); + appendFileSync(ledgerPath, '{"v":1'); + expect(appendReviewRecord(start, start, ledgerPath)).toEqual(start); + const runs = summarizeReviews(readReviewRecords(ledgerPath).records); + expect(runs).toHaveLength(2); + expect(runs.at(-1)?.turns).toBe(0); + expect(appendReviewRecord(start, start, join(ledgerPath, "invalid"))).toBeNull(); + }); + test("a torn ledger line is skipped and counted, never fatal", () => { const path = join(mkdtempSync(join(tmpdir(), "gt-review-ledger-")), "reviews.jsonl"); const [start] = ledgerFixture(); @@ -674,6 +748,7 @@ describe("spawn guards", () => { await expect(spawn({ base: "origin/master" })).rejects.toThrow("apply to --mode review only"); await expect(spawn({ allowShallow: true })).rejects.toThrow("apply to --mode review only"); + await expect(spawn({ pr: "592" })).rejects.toThrow("apply to --mode review only"); }); test("a review session cannot start without its review record, and a task session cannot carry one", async () => { @@ -741,7 +816,7 @@ function importReaders(ledgerPath: string, options: { gone?: string[]; noBase?: const git = readers({ pr: options.noBase ? null : PR, numstat: [entry("src/a.ts", 30, 5), entry("src/b.ts", 10)], - commitBefore: (at) => (at.startsWith("2026-10-22") ? "abc1234567890" : null), + commitBefore: (at, ref) => (at.startsWith("2026-10-22") ? (ref ? "base1234567890" : "abc1234567890") : null), }); const messages: Record = { "rv-full": @@ -778,10 +853,10 @@ describe("reviews import", () => { ["rv-gone", "imported", "imported"], ["rv-none", "missing", null], ]); - expect(results[0].manifest).toEqual({ base: "origin/master", head: "abc1234567890", files: 2, lines: 45 }); + expect(results[0].manifest).toEqual({ base: "base1234567890", head: "abc1234567890", files: 2, lines: 45 }); expect(results[2].manifest).toBeNull(); expect(results[2].notes[0]).toContain("/wt/rv-gone is gone"); - expect(git.calls).toEqual(["numstat origin/master...abc1234567890", "numstat origin/master...abc1234567890"]); + expect(git.calls).toEqual(["numstat base1234567890...abc1234567890", "numstat base1234567890...abc1234567890"]); const records = readReviewRecords(ledgerPath).records; expect(records.map((record) => `${record.name}:${record.kind}`)).toEqual([ @@ -834,6 +909,99 @@ describe("reviews import", () => { expect(readReviewRecords(ledgerPath).records).toHaveLength(10); }); + test("real reflogs recover a pre-merge base even when today's base already contains the reviewed head", async () => { + const repo = await TestRepo.create(); + const base = await repo.sha(); + const head = await repo.commit({ file: "reviewed.ts", content: "export const reviewed = true;\n" }); + await repo.branch("target", head); + const identity = "Fixture "; + writeFileSync( + join(repo.dir, ".git/logs/refs/heads/target"), + `${"0".repeat(40)} ${base} ${identity} 1792454400 +0000\tbase\n${base} ${head} ${identity} 1792800000 +0000\tmerged\n` + ); + const git = { ...defaultReviewGitReaders(), currentBranch: async () => null }; + expect(await git.numstat(repo.dir, "target")).toEqual([]); + expect(await git.commitBefore(repo.dir, "2026-10-22T10:00:00Z", "target")).toBe(base); + expect(await git.commitBefore(repo.dir, "2000-01-01T00:00:00Z", "target")).toBeNull(); + const ledgerPath = join(repo.root, "reviews.jsonl"); + const { fake } = importReaders(ledgerPath); + fake.git = git; + fake.readMeta = async (name) => importedMeta(name, { cwd: repo.dir }); + fake.lastAgentMessage = async () => "Findings: none\nRan: nothing\nFiles reviewed: 1 of 1\nNot reviewed: none"; + const [result] = await importReviewSessions({ names: ["rv-full"], base: "target", readers: fake, ledgerPath }); + expect(result.manifest).toEqual({ base, head, files: 1, lines: 1 }); + expect(result.verdict).toBe("pass"); + }); + + test("import uses the live gate's binary exemption before deciding that a review is shallow", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-binary-")), "reviews.jsonl"); + const { fake, git } = importReaders(ledgerPath); + git.numstat = async () => [entry("src/a.ts", 10), entry("logo.png", 0, 0, true)]; + fake.lastAgentMessage = async () => + "Findings: none\nRan: nothing\nFiles reviewed: 1 of 2\nNot reviewed: logo.png (binary)"; + const [result] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + expect(result.verdict).toBe("pass"); + fake.lastAgentMessage = async () => + "Findings: none\nRan: nothing\nFiles reviewed: 1 of 2\nNot reviewed: src/a.ts (time)"; + const [unread] = await importReviewSessions({ + names: ["rv-part"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + expect(unread.verdict).toBe("shallow"); + }); + + test("historical base missing after a merge leaves scope unconfirmed instead of trusting today's empty diff", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-history-")), "reviews.jsonl"); + const { fake, git } = importReaders(ledgerPath); + git.commitBefore = async (_cwd, _at, ref) => (ref ? null : "reviewed-head"); + const [result] = await importReviewSessions({ + names: ["rv-full"], + base: "origin/master", + readers: fake, + ledgerPath, + }); + expect(result.manifest).toBeNull(); + expect(result.verdict).toBe("imported"); + expect(result.notes[0]).toContain("cannot be established"); + expect(git.calls).toEqual([]); + }); + + test("saved pinned manifests import even after the worktree is gone or detached", async () => { + const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-pinned-")), "reviews.jsonl"); + const { fake, git } = importReaders(ledgerPath, { gone: ["rv-full"], noBase: true }); + fake.readMeta = async (name) => + importedMeta(name, { + review: reviewConfig({ + manifest: { + base: "pinned-base", + head: "pinned-head", + files: [ + { path: "src/a.ts", added: 30, deleted: 5 }, + { path: "src/b.ts", added: 10, deleted: 0 }, + ], + added: 40, + deleted: 5, + }, + }), + }); + const [result] = await importReviewSessions({ names: ["rv-full"], readers: fake, ledgerPath }); + expect(result.manifest).toEqual({ base: "pinned-base", head: "pinned-head", files: 2, lines: 45 }); + expect(result.verdict).toBe("pass"); + expect(readReviewRecords(ledgerPath).records[0]).toMatchObject({ + pr: 12, + base: "pinned-base", + head: "pinned-head", + }); + expect(git.calls).toEqual([]); + }); + test("without a base or with --no-cwd-from-session the manifest is null and the note says why", async () => { const ledgerPath = join(mkdtempSync(join(tmpdir(), "gt-review-import-")), "reviews.jsonl"); const { fake } = importReaders(ledgerPath, { noBase: true }); diff --git a/src/utils/worker/contract.test.ts b/src/utils/worker/contract.test.ts index 824921372e..6a98579da8 100644 --- a/src/utils/worker/contract.test.ts +++ b/src/utils/worker/contract.test.ts @@ -111,6 +111,19 @@ describe("buildWorkerContract", () => { } }); + test("a pinned review contract never substitutes today's HEAD for its saved endpoint", () => { + const text = buildWorkerContract({ + backend: "codex", + review: { + effort: "high", + manifest: { base: "base-sha", head: "head-sha", files: [], added: 0, deleted: 0 }, + }, + }); + expect(text).toContain("git diff --no-textconv --no-ext-diff base-sha...head-sha -- "); + expect(text).toContain("git show --no-textconv --no-ext-diff head-sha:"); + expect(text).not.toContain("...HEAD"); + }); + test("review: the manifest and tier ride after the report shape; without review the text is unchanged", () => { const manifest: ReviewManifest = { base: "origin/master", @@ -135,7 +148,7 @@ describe("buildWorkerContract", () => { expect(reviewer.startsWith(`${plain}\n`)).toBe(true); expect(reviewer).toContain("You are a code REVIEWER at the high tier."); expect(reviewer).toContain("Files to review (2, +40 -2):\n- src/a.ts +40 -2\n- assets/logo.png (binary)"); - expect(reviewer).toContain("`git diff origin/master...HEAD -- `"); + expect(reviewer).toContain("`git diff --no-textconv --no-ext-diff origin/master...HEAD -- `"); expect(reviewer.indexOf("OPEN: ")).toBeLessThan(reviewer.indexOf("code REVIEWER")); } }); diff --git a/src/utils/worker/contract.ts b/src/utils/worker/contract.ts index 2550af5df4..57c1858030 100644 --- a/src/utils/worker/contract.ts +++ b/src/utils/worker/contract.ts @@ -41,6 +41,8 @@ export interface ReviewManifestFile { /** The files a review must cover, computed by the harness before the worker starts. */ export interface ReviewManifest { base: string; + /** Pinned commit for new reviews; absent only on older saved manifests. */ + head?: string; files: ReviewManifestFile[]; added: number; deleted: number; @@ -59,7 +61,12 @@ export function manifestChangedLines(manifest: Pick\`.`, + `Base: ${manifest.base}. Read each change with \`git diff --no-textconv --no-ext-diff ${manifest.base}...${manifest.head ?? "HEAD"} -- \`.`, + ...(manifest.head + ? [ + `Review the pinned commits above even if HEAD or the base branch moves. Read file versions with \`git show --no-textconv --no-ext-diff ${manifest.head}:\`.`, + ] + : []), `Files to review (${manifest.files.length}, +${manifest.added} -${manifest.deleted}):`, ...manifest.files.map((file) => file.binary ? `- ${file.path} (binary)` : `- ${file.path} +${file.added} -${file.deleted}`