diff --git a/AGENTS.md b/AGENTS.md index 843cad04..4d2e4e75 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,10 @@ Do not edit `CONTRIBUTING.md`, `assets/CONTEXT.md`, `assets/AGENTS.md`, or PR, or the standing one /quest-run gives a PR with no user-facing behavior change that adds or expands no quests; and only after repository checks and reviews pass. +- A change to the reviewers in `src/cli/models.ts`, the review prompts in + `src/cli/review.ts`, or a `REVIEW.md` runs `quest eval run` with the base's + build and the head's, and puts both scores and their cost in its PR's + `## Validation`. - Pull requests land as squash merges. - Do not comment on repositories outside kixelated/moq-dev without approval. diff --git a/README.md b/README.md index 06090911..e0ed87f3 100644 --- a/README.md +++ b/README.md @@ -1558,6 +1558,33 @@ the reason of the first to judge it so. The record names it after the base: tier, and a reviewer that raised it adds `- Raised the risk to high: ` to its block. +### Score the reviewer + +`quest eval` scores the reviewer on the repository's past PRs, kept in +`review-eval.json` at its root: culprits, each with the places at its last head +of the defect a later fix's `## Caused by` named, and clean PRs that broke +nothing. Score a change to the reviewer table, the prompts, or `REVIEW.md` +before it lands, and put the score and its cost in its PR. + +```sh +quest eval add 570 --fix 704 --what "It counts every link as a culprit." \ + --defect src/cli/usage.ts:395-399 --defect src/cli/usage.ts:408-410 +quest eval add 503 --clean +quest eval run # the size's pick and its shadow on every case +quest eval run --harness claude --effort high --pr 570 +``` + +`add` reads the PR's last head and base from GitHub and records its merge base, +size, latest writer, and tier, and a culprit already in the set gains each +further defect. `run` fetches each case from the set's repository, so a fork's +clone scores its upstream's set, and reviews it at its head in a detached checkout, +giving the reviewers the first round `quest review` gives, with the base's +`AGENTS.md` and `CLAUDE.md` and this checkout's committed `REVIEW.md`. A +finding on any line of a defect's places finds it; every finding on a clean PR +is noise. It prints defects found and clean findings by severity for the pick, +its shadow, and either, by size and by tier, each case's rounds, and the cost +token-max prices for the run's sessions. + ## Audit cost and accuracy `quest usage` reports `origin`'s PRs updated since `--since` (a local diff --git a/quest/m0/benchmark/reviewer-eval.md b/quest/m0/benchmark/reviewer-eval.md deleted file mode 100644 index d0905e9a..00000000 --- a/quest/m0/benchmark/reviewer-eval.md +++ /dev/null @@ -1,40 +0,0 @@ -# [L] Score reviewers against the PRs that broke something - -## Goal - -The culprit PRs on record, each with the defect's file and line taken from -its fix, form a test set, beside a set of PRs known to be clean. A command -runs a reviewer over both and reports how many defects it found and how -many findings on the clean set would have led to a harmful change. A -change to a reviewer's prompt, model, effort, or the repository's -`REVIEW.md` is scored on the set before it lands, and the score, with its -cost, is in that change's PR. - -## Plan - -From the post: "Incidents become an eval. Past incident-causing PRs, with -their fix-forwards and reverts, were collected," and a change to the -reviewer "is kept only if the score goes up". The same post measures noise -by running the reviewer on known-clean PRs. - -Review scores here are saturated, with about 90% of PRs scoring 9 or 10 -([routing evidence](/quest/m0/routing-evidence/README.md)), and live -shadow reviews need weeks of PRs per comparison. A fixed set answers the -same question in one run. - -Agent's choices, for the implementer to revisit: - -- Start from the escapes the mistake audit judged catchable from the diff; - a runtime escape no reviewer could see only adds noise to the score. -- Confirm each defect's location by a stronger model reading the culprit - and its fix, and by hand for a sample, before trusting the set. -- Keep the set outside this public repository where its PRs are private. -- Run on demand and before a reviewer change, not on a schedule: a full - run costs tokens, and the score moves only when the reviewer does. -- Report recall by tier and size, so a reviewer effort can be cut where it - finds nothing more. - -## Related - -- [Model check](/quest/m1/model-check.md) - retunes the reviewer table - each milestone, and can score candidates on this set diff --git a/quest/m0/mistake-audit.md b/quest/m0/mistake-audit.md index 4abb0735..3b90cc5f 100644 --- a/quest/m0/mistake-audit.md +++ b/quest/m0/mistake-audit.md @@ -66,6 +66,11 @@ Agent's choices, for the implementer to revisit. Each run: owns that gate, so it goes in the run PR's `## Follow-ups` for `/quest-plan` there, as the guide's Creation section says (user, 2026-11-11, [routing evidence](/quest/m0/routing-evidence/README.md)). +- Add each escape classified catchable or maybe to its repository's eval + set with `quest eval add`, its defect located at the culprit's head from + the fix's diff and confirmed against the culprit's own diff, so `quest + eval run` scores reviewers on it. A private repository's set stays in + that repository. - Before proposing a fix, find the quest, check, or rule that already owns the class, and report whether its count fell since that landed. A rule whose class still recurs is the case for moving it up a level. diff --git a/quest/m1/model-check.md b/quest/m1/model-check.md index 4c78e9c1..b0149f3e 100644 --- a/quest/m1/model-check.md +++ b/quest/m1/model-check.md @@ -42,8 +42,9 @@ checkpoints and outages to count by hand: - Move a size to a stronger model or effort where its PRs needed clearly more rounds or fixes than the size above, and to a cheaper one where its reviews come back clean. Do the same for the review efforts in `src/cli/models.ts` - from what later reviews and follow-ups found that a review missed. Record the tallies in - the next run's Plan as its baseline. + from what later reviews and follow-ups found that a review missed, and score each + reviewer change with `quest eval run` before recommending it (`README.md`'s Score the + reviewer). Record the tallies in the next run's Plan as its baseline. - Recommend each change to a list's order, harness, or effort to the maintainer with its tally, as a decision in the PR, rather than changing it unasked. Replacing a stale model id with the same vendor's newer one, diff --git a/review-eval.json b/review-eval.json new file mode 100644 index 00000000..b45a49f1 --- /dev/null +++ b/review-eval.json @@ -0,0 +1,498 @@ +{ + "repository": "moq-dev/quest", + "cases": [ + { + "pull": 160, + "base": "d37da2e1cc070c5383566b5627a6d431d32700e1", + "head": "630e2abe3fd6696267be16772dd11cfeba424fdc", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 178, + "base": "e39c9186d1205fa2b58069aff6908530ca0a99bf", + "head": "6fc09242196c908c5f40898a208f85c8c0d696d5", + "size": "M", + "tier": "high", + "reason": "it changes .github/workflows/check.yml, which is always high: workflows run with the repository's secrets and decide what merges and ships", + "writer": null, + "defects": [ + { + "fix": 589, + "what": "alive() treats only ENOENT from reading /proc//stat as a gone process, so a process that exits mid-read (Linux reports ESRCH) makes the liveness check throw instead of returning false.", + "at": [ + { + "file": "src/cli/supervise.ts", + "from": 34, + "to": 36 + } + ] + } + ] + }, + { + "pull": 186, + "base": "a689e26019bed1364e6d5d1be222ebd63f99d9ad", + "head": "4d04fe58411c6b26bb2d07a3c1f4f8d5320c37fd", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "defects": [ + { + "fix": 632, + "what": "worktree remove counts an unreached commit as preserved only when a merged PR's head is exactly that commit, so a finished checkout whose merged head has later commits on top cannot be removed without --force.", + "at": [ + { + "file": "src/cli/github.ts", + "from": 239, + "to": 239 + } + ] + }, + { + "fix": 632, + "what": "worktree remove refuses any ignored file outside the scratch declared at creation, so a finished checkout holding build output cannot be removed without --force.", + "at": [ + { + "file": "src/cli/worktree.ts", + "from": 563, + "to": 568 + } + ] + } + ] + }, + { + "pull": 217, + "base": "572de71330ddd174a0aa0b9808d2b2737a0fc61a", + "head": "8346d7f32bcb5a8c2b94aa6d5303fbef9bf65619", + "size": "XS", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 282, + "base": "01d051cf5dca8cd29c664bf8bcbcab63cdc76445", + "head": "7081251fdc9e7e3143411ab674aae2f42723e1ed", + "size": "M", + "tier": "high", + "reason": "it changes .github/workflows/land.yml, which is always high: workflows run with the repository's secrets and decide what merges and ships", + "writer": null, + "clean": true + }, + { + "pull": 289, + "base": "1ef2ca70053ed8bc4785ddaaafb2a6c445e87614", + "head": "33fa0f425b1ec556d93492fdfbd3af2020684d3c", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 297, + "base": "96f6d2e5ad8a9dc76934c809420a9c7c59751901", + "head": "0edb89770c88a6b5a85f9fbe593d5a06a6640333", + "size": "L", + "tier": "high", + "reason": "it changes .github/workflows/land.yml, which is always high: workflows run with the repository's secrets and decide what merges and ships", + "writer": null, + "defects": [ + { + "fix": 666, + "what": "Any failed landing push of a same-repository head or of the base is treated as a permanent refusal that ejects the PR as merge_refused, with no retry for transient GitHub failures such as its intermittent \"Repository not found\".", + "at": [ + { + "file": "src/cli/land-head.ts", + "from": 158, + "to": 163 + }, + { + "file": "src/cli/land-head.ts", + "from": 200, + "to": 208 + } + ] + } + ] + }, + { + "pull": 312, + "base": "f8612ed4686c0a2a57cefac0d3224e89c013a74e", + "head": "123be0b5074d18cbd493f4292358dab9fdce3e5e", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "defects": [ + { + "fix": 627, + "what": "Every fixer handback comment quotes the tail of the fixer agent's own output log rather than the gate's failing output, so the PR shows the fixer's chatter instead of why the gate dropped it.", + "at": [ + { + "file": "src/cli/land-fixer.ts", + "from": 533, + "to": 533 + } + ] + } + ] + }, + { + "pull": 335, + "base": "12f4af006315bbadb882745c77cf2d0ba148f14f", + "head": "ac774f2442eba7ef4b528427cbc0956e1426ad8c", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 337, + "base": "3c8cdce73b8c8f5315ec529902c3746aaca50502", + "head": "71ec112a1fe17544781f654b70e76ac52a93eb70", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 356, + "base": "4bc19ccf7b9db47efb5a419ba7b50bbebf19c1af", + "head": "0b842d48559baaa44adb835c670d514ced8b4c2a", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "defects": [ + { + "fix": 593, + "what": "The fixer prompt requires building and testing before any fixed or requeue while forbidding contact with any host but dependency registries, so a repository whose tests reach outside services can never be requeued and is always handed back.", + "at": [ + { + "file": "assets/land-fix.md", + "from": 39, + "to": 40 + }, + { + "file": "assets/land-fix.md", + "from": 42, + "to": 46 + } + ] + } + ] + }, + { + "pull": 361, + "base": "5f6df528b417b0fb460b4bc8638b14b477b44569", + "head": "40daaba35c6136b6b32acbecd7d09f4456e79fd9", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 389, + "base": "662954529415620dd596634f0f258a4e1fbe36ef", + "head": "388a0e530b7cda4b452f58b4431671a2e9e0576a", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "defects": [ + { + "fix": 627, + "what": "When the fixer failed for its own reasons (crash, refused launch, timeout, invalid result), the watcher's handback quoted the tail of the fixer's own job log instead of the gate's failing output, so the comment showed only the wrapper's error and hid why the queue dropped the PR.", + "at": [ + { + "file": "src/cli/land-watch-fix.ts", + "from": 548, + "to": 548 + } + ] + } + ] + }, + { + "pull": 392, + "base": "e0509fbe219ee09c2816de20a76d2c82e8141b52", + "head": "1fb89e12cb269d48a0561eaf83117a4b8e15171e", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 418, + "base": "acb8682041c7c3774d715a68ba9ed9f5da25593b", + "head": "7e4bdf74e2e8c6f9ddd3ad7c032f7990d6018a22", + "size": "XS", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 419, + "base": "e4e64f9b9a6ddd5dfcc38d6a476343af1d341bd2", + "head": "a6b7dede826608360e8358997bbcf759e7ebb8cd", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 429, + "base": "acb8682041c7c3774d715a68ba9ed9f5da25593b", + "head": "eaee36a8d4789afbd65159e6154b11b06178f777", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "defects": [ + { + "fix": 624, + "what": "The new gate cancellation tests run their phase under `just` (and GNU time), but Release's macOS job runs test/gate.test.ts with only setup-bun, outside the Nix dev shell, so those tests fail there for want of Just.", + "at": [ + { + "file": "test/gate.test.ts", + "from": 643, + "to": 648 + }, + { + "file": ".github/workflows/release.yml", + "from": 133, + "to": 138 + } + ] + } + ] + }, + { + "pull": 475, + "base": "58696bc097754afd7eddf0671655098681a48478", + "head": "5563559335f282820decb8ce5e0a41a41f297202", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 491, + "base": "5397c573ab026747c640b867099ed09ab84ae2a3", + "head": "9770aa588c4f22f8803d647da46906bd8b22c7fa", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": "claude-opus-5-5", + "clean": true + }, + { + "pull": 492, + "base": "ba352ea8595b2ad23468358b244ffb15a4d44918", + "head": "37984404b86dfdc0ca62ba811027cf52f8b48e01", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": "claude-opus-5-5", + "defects": [ + { + "fix": 503, + "what": "agent.ts defines its own private LIMIT regex duplicating the exported one in src/cli/limit.ts, and the copy lacks `free usage exceeded`, so that spent-limit message is not classified as a limit.", + "at": [ + { + "file": "src/cli/agent.ts", + "from": 94, + "to": 104 + } + ] + } + ] + }, + { + "pull": 503, + "base": "5397c573ab026747c640b867099ed09ab84ae2a3", + "head": "b4d17c2305667defd8c508d826d607be504ec977", + "size": "XS", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "clean": true + }, + { + "pull": 526, + "base": "19a2438cd80cf968dddfbf664570a2c8b9a254bf", + "head": "07124a927a40408e75bccf207692b7f47bd2a16d", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": null, + "defects": [ + { + "fix": 589, + "what": "processStart treats only ENOENT as the process being gone, so reading /proc//stat for a process that exits mid-read throws ESRCH, which is rethrown instead of returning undefined.", + "at": [ + { + "file": "src/cli/gate.ts", + "from": 285, + "to": 287 + } + ] + } + ] + }, + { + "pull": 570, + "base": "738bbdecbd0e4482e038a4aac2a0107b815e8985", + "head": "ff36d184dbdd4093ed866bfd510592729710d29f", + "size": "L", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": "claude-opus-5-5", + "defects": [ + { + "fix": 704, + "what": "causes() counted every repository PR URL anywhere in a `## Caused by` section as a culprit, and pullRows credited fixedBy from every PR body, including open or closed-unmerged fixes.", + "at": [ + { + "file": "src/cli/usage.ts", + "from": 395, + "to": 399 + }, + { + "file": "src/cli/usage.ts", + "from": 408, + "to": 410 + } + ] + } + ] + }, + { + "pull": 585, + "base": "133ab156f1dfbb9f8295c5287624bef7d58a9d37", + "head": "ece891a3bc5d4ed3908cdf3cb7c07d5db4edb04a", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": "claude-opus-5-5", + "defects": [ + { + "fix": 629, + "what": "A gate inside a Quest scope now runs in a bound sibling scope and forwards signals by spawning a `systemctl kill` helper, which starts in the enclosing Quest scope's cgroup while that scope's owner is signalling it, so the helper can die of the very signal it forwards.", + "at": [ + { + "file": "src/cli/supervise.ts", + "from": 299, + "to": 299 + }, + { + "file": "src/cli/supervise.ts", + "from": 366, + "to": 376 + }, + { + "file": "src/cli/gate.ts", + "from": 692, + "to": 692 + } + ] + }, + { + "fix": 685, + "what": "After dropping the delegated child cgroup, the `quest gate` and `quest worktree run` help texts still say a gate inside a Quest scope owns its command through a child cgroup and that the scope is delegated.", + "at": [ + { + "file": "src/cli/gate.ts", + "from": 681, + "to": 686 + }, + { + "file": "src/cli/main.ts", + "from": 140, + "to": 140 + }, + { + "file": "src/cli/main.ts", + "from": 851, + "to": 852 + } + ] + } + ] + }, + { + "pull": 655, + "base": "93c9c2ef65f074e82094bf5272076db44ac19030", + "head": "85b0b490516f494bd65d998ded89c96195306d73", + "size": "S", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": "claude-opus-5-5", + "defects": [ + { + "fix": 658, + "what": "`quest check` now always compares skill stubs against the running binary's skills, but the merge queue runs the base's binary on a PR whose stubs follow the PR's own skills, so any PR changing a skill fails the queue.", + "at": [ + { + "file": "src/cli/main.ts", + "from": 203, + "to": 205 + }, + { + "file": ".github/workflows/queue-build.yml", + "from": 51, + "to": 51 + } + ] + } + ] + }, + { + "pull": 704, + "base": "2a0f4663f0d6947be716dff56f2734935fe86221", + "head": "c5c1c77d78d571d8a291dd2ab2d8785a53255e73", + "size": "M", + "tier": "medium", + "reason": "the base has no RISK.md to name what is always high or customer-facing, so every change is medium", + "writer": "claude-opus-5-5", + "defects": [ + { + "fix": 721, + "what": "The token-max integration drops each session's `parent`, so a sub-agent in a detached checkout cannot inherit its parent's PR and stays unattributed, and a review's sub-agents get implementing/iterating roles instead of reviewing.", + "at": [ + { + "file": "src/cli/usage.ts", + "from": 222, + "to": 226 + }, + { + "file": "src/cli/usage.ts", + "from": 470, + "to": 473 + }, + { + "file": "src/cli/usage.ts", + "from": 481, + "to": 486 + } + ] + } + ] + } + ] +} diff --git a/src/cli/main.ts b/src/cli/main.ts index bbc7e912..62c850c5 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -75,6 +75,7 @@ import { nextStep, roundLines, } from "./review"; +import { EVAL_FILE, addCase, evalLines, evaluate } from "./review-eval"; import { type Recorded, pullModels, recordRounds } from "./review-record"; import { BASH_MAX_TIMEOUT_MS, @@ -1391,6 +1392,15 @@ export function main(argv: string[], out: Output): number | Promise { (size) => `${size} ${REVIEWERS[size].map((e) => `${e.harness} ${e.model} ${e.effort}`).join(", then ")}`, ).join("; "); const classifiers = CLASSIFIERS.map((e) => `${e.harness} ${e.model} ${e.effort}`).join(", then "); + /** quest review's exit code for `error`: 2 for a usage error, 3 at a usage limit, else 1. */ + const reviewFailure = (error: unknown): number => { + if (error instanceof ReviewInputError || error instanceof ReviewLimitError) { + out.stderr(`error: ${error.message}\n`); + return error instanceof ReviewInputError ? 2 : 3; + } + warn(error instanceof Error ? error.message : String(error)); + return 1; + }; program .command("review") .summary("Have a model other than the latest writer review the committed branch, read-only") @@ -1548,52 +1558,156 @@ export function main(argv: string[], out: Output): number | Promise { models: (dir, base, branch) => pullModels(dir, base, branch, github), }, ); - pending = run.then( - async (reviewed) => { - const { rounds, tallies, risk } = reviewed; - // The rounds are saved in their sessions either way, so their report prints before a failed record. - let record: Recorded | undefined; - let failure: unknown; - try { - record = await recordRounds(root(), reviewed, github); - } catch (error) { - failure = error; - } - if (options.json) - print( - JSON.stringify({ - rounds, - tallies, - risk, - record: record ?? null, - next: nextStep(rounds), - }), - ); - else { - for (const line of roundLines(reviewed)) print(line); - if (record !== undefined) - print("url" in record ? `record ${record.url}` : `record skipped: ${record.skipped}`); - print(nextStep(rounds)); - } - if (failure === undefined) return 0; - warn( - `the round finished, but recording it in the PR body failed: ${failure instanceof Error ? failure.message : String(failure)}`, + pending = run.then(async (reviewed) => { + const { rounds, tallies, risk } = reviewed; + // The rounds are saved in their sessions either way, so their report prints before a failed record. + let record: Recorded | undefined; + let failure: unknown; + try { + record = await recordRounds(root(), reviewed, github); + } catch (error) { + failure = error; + } + if (options.json) + print( + JSON.stringify({ + rounds, + tallies, + risk, + record: record ?? null, + next: nextStep(rounds), + }), ); - return 1; + else { + for (const line of roundLines(reviewed)) print(line); + if (record !== undefined) + print("url" in record ? `record ${record.url}` : `record skipped: ${record.skipped}`); + print(nextStep(rounds)); + } + if (failure === undefined) return 0; + warn( + `the round finished, but recording it in the PR body failed: ${failure instanceof Error ? failure.message : String(failure)}`, + ); + return 1; + }, reviewFailure); + }, + ); + + const evalCommand = program + .command("eval") + .summary("Score the reviewer on the repository's past culprits and clean PRs"); + evalCommand + .command("run") + .summary("Review every case of the eval set and score the reviewers") + .description( + `Review each case of the eval set, ${EVAL_FILE} at the checkout's root unless --set names another, at its PR's ` + + "last head, fetched from the set's repository (a remote that names it, else its GitHub URL in origin's " + + "scheme, so a fork's clone scores its upstream's set), in a detached checkout under the clone's common Git " + + "directory that is removed afterwards, and " + + "score the reviewers. A culprit is a PR a later fix named under ## Caused by, with each defect's places, a " + + "file's lines at its head; a finding on any of them finds it, and one naming no line finds nothing. A clean " + + "PR broke nothing, and every finding on it is noise. Each case gets the first round quest review gives a new " + + "session: the base's AGENTS.md and CLAUDE.md at the case's merge base, its classified tier, and the prompts of " + + "this build, with the review rules of this checkout, its committed REVIEW.md at HEAD, so a change to the " + + "reviewer table, the prompts, or REVIEW.md is scored before it lands. The reviewers are the size's pick, " + + "avoiding the case's writer, and its shadow, as quest review runs them, or --harness's alone, at its " + + "entry's model and effort unless --model and --effort override them. Cases run one at a time, their reviewers " + + "at once, and the first failure stops the run and fails it.\n\n" + + "Prints the set's counts and rules, a line per seat (pick, shadow, and, without --harness, either: what " + + "the case's seats found together, as a run of the pick and its shadow finds it) with defects found and clean findings by severity, the same by size and tier, " + + "each case's rounds with score, defects found, findings, tokens the CLI reported, and session, and the " + + "run's cost at API list price as token-max prices its sessions, or unpriced without token-max. --json prints " + + "one object. Exits as quest review does: 2 for invalid arguments or a set that cannot be read, 3 at a " + + "usage limit, 1 for other failures.", + ) + .option("--set ", `The eval set (default: ${EVAL_FILE} at the checkout's root)`) + .addOption( + new Option("--harness ", "The only reviewer, instead of the pick and shadow").choices(HARNESSES), + ) + .option("--model ", "Model --harness's reviewer runs instead of its size's") + .option("--effort ", "Reasoning effort --harness's reviewer runs at instead of its size's", effortLevel) + .option("--pr ", "Only this case; repeat for each", (value: string, previous: number[] = []) => [ + ...previous, + positiveInteger("--pr")(value), + ]) + .option("--json", "Print one JSON object") + .action( + (options: { + set?: string; + harness?: HarnessName; + model?: string; + effort?: string; + pr?: number[]; + json?: boolean; + }) => { + const run = evaluate( + { + root: root(), + set: options.set, + harness: options.harness, + model: options.model, + effort: options.effort, + only: options.pr, + env: process.env, + progress: (index, total, pull) => warn(`case ${index}/${total}: #${pull}`), }, - (error: unknown) => { - if (error instanceof ReviewInputError) { - out.stderr(`error: ${error.message}\n`); - return 2; - } - if (error instanceof ReviewLimitError) { - out.stderr(`error: ${error.message}\n`); - return 3; - } - warn(error instanceof Error ? error.message : String(error)); - return 1; + (since) => runTokenMax(since, []), + ); + pending = run.then((report) => { + if (options.json) print(JSON.stringify(report)); + else for (const line of evalLines(report)) print(line); + if (report.cost === null) warn(TOKEN_MAX_MISSING); + return 0; + }, reviewFailure); + }, + ); + evalCommand + .command("add") + .summary("Add a culprit's defect or a clean PR to the eval set") + .argument("", "The merged PR", positiveInteger("")) + .description( + "Add a merged PR of the set's repository, or for a new set the one origin's fork chain leads to as quest " + + "start finds it, to the eval set: --clean for one that broke nothing, or --defect with " + + "--fix and --what for a culprit's defect, which a culprit already in the set gains, repeating --defect for each " + + "place the one defect is. Locate a defect at the culprit's last head from the diff of the fix that repaired " + + "it. The first time, it reads the PR's last head " + + "and base from GitHub, fetches them, and records the merge base, the size and latest writer its ## Models " + + "record names (the size as quest review picks it), and the tier the classifier sets, which may run a model " + + "as quest review's does. Prints the case as JSON.\n\nUses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Exits 2 " + + "for invalid arguments, a PR not in or already in the set as asked, or lines past the file's end.", + ) + .option("--set ", `The eval set (default: ${EVAL_FILE} at the checkout's root)`) + .option("--clean", "Record a PR that broke nothing") + .option( + "--defect ", + "Where a defect is at the PR's last head, as path:from-to or path:line; repeat for each place it is", + (value: string, previous: string[] = []) => [...previous, value], + ) + .option("--fix ", "The PR whose ## Caused by names it", positiveInteger("--fix")) + .option("--what ", "One sentence on what was wrong", nonEmpty) + .action( + ( + pr: number, + options: { set?: string; clean?: boolean; defect?: string[]; fix?: number; what?: string }, + ) => { + const given = [options.defect, options.fix, options.what].filter((each) => each !== undefined).length; + if (given !== 0 && given !== 3) throw new UsageError("--defect, --fix, and --what go together"); + const run = addCase( + { + root: root(), + set: options.set, + pull: pr, + clean: options.clean, + defect: + given === 3 ? { at: options.defect!, fix: options.fix!, what: options.what! } : undefined, + env: process.env, }, + { github: () => gitHubDependencies(warn), warn }, ); + pending = run.then((entry) => { + print(JSON.stringify(entry)); + return 0; + }, reviewFailure); }, ); diff --git a/src/cli/review-eval.ts b/src/cli/review-eval.ts new file mode 100644 index 00000000..0e92d911 --- /dev/null +++ b/src/cli/review-eval.ts @@ -0,0 +1,612 @@ +// `quest eval`: score the reviewer on a repository's past PRs. Culprits, whose defects a later fix located, +// measure how many defects it finds; clean PRs, which broke nothing, measure how many findings it raises on code +// that needed none. Each case is reviewed at its PR's last head with the prompts `quest review` gives, the review +// rules of the checkout running the eval, and this build's reviewer table, so a change to any of them is scored +// before it lands. +import { randomUUID } from "node:crypto"; +import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; + +import { SIZES, type Size, TIERS, type Tier } from "../core"; +import { parser } from "../core/parse"; +import { type GitHub, type Repository, object, repositoryPath } from "../github/client"; +import type { HarnessName } from "./agent"; +import { git } from "./git"; +import { type ClaimDependencies, upstream } from "./claims"; +import { readRemote, remoteRepository, requireRemote } from "./github"; +import type { Model } from "./models"; +import { recorded } from "./models-record"; +import { + type Assessment, + type Finding, + ReviewInputError, + SEVERITIES, + type Usage, + baseInstructions, + classify, + firstRound, + pickReviewer, + reviewSize, + shadowOf, +} from "./review"; +import type { Session } from "./usage"; + +/** Where a repository keeps its set, from its root. */ +export const EVAL_FILE = "review-eval.json"; + +/** Lines of a file at a culprit's head, first and last inclusive. */ +export interface Place { + file: string; + from: number; + to: number; +} + +/** A culprit's defect, as the fix that repaired it shows. */ +export interface Defect { + /** The PR whose `## Caused by` names the culprit. */ + fix: number; + /** One sentence on what was wrong. */ + what: string; + /** Each place the defect is at the culprit's head, such as two call sites; a finding on any of their lines finds it. */ + at: Place[]; +} + +/** One past PR, as its last head was reviewed. */ +export interface EvalCase { + pull: number; + /** The merge base of the head with its base branch: the change is `base..head`. */ + base: string; + head: string; + size: Size; + /** The head's tier as the classifier set it, which the reviewer is told and may raise. */ + tier: Tier; + reason: string; + /** The model of its latest Implementer line, which the size's pick avoids, or null when it names none. */ + writer: string | null; + /** Its defects, at least one; absent on a clean PR. */ + defects?: Defect[]; + /** A PR that broke nothing, on which every finding is noise. */ + clean?: true; +} + +/** A repository's eval set. */ +export interface EvalSet { + /** Lowercase `owner/repo`, whose PRs the cases are. */ + repository: string; + cases: EvalCase[]; +} + +const SHA = /^[0-9a-f]{40}$/; + +/** `value` as an eval set, failing on any missing, extra, or ill-typed field, or a case neither culprit nor clean. */ +export function parseEvalSet(value: unknown): EvalSet { + const { fail, isObject, text, oneOf, exact, pull, repository } = parser("eval set"); + const sha = (value: unknown, field: string) => (SHA.test(text(value, field)) ? (value as string) : fail(field)); + const line = (value: unknown, field: string) => + Number.isSafeInteger(value) && (value as number) > 0 ? (value as number) : fail(field); + if (!isObject(value)) return fail("not an object"); + exact(value, ["repository", "cases"], "the set"); + if (!Array.isArray(value.cases)) return fail("cases"); + const seen = new Set(); + const cases = value.cases.map((item: unknown): EvalCase => { + if (!isObject(item)) return fail("a case"); + const number = pull(item.pull, "a case's pull"); + const at = `case #${number}`; + exact(item, ["pull", "base", "head", "size", "tier", "reason", "writer", "defects", "clean"], at); + if (seen.has(number)) fail(`${at} is listed twice`); + seen.add(number); + const each: EvalCase = { + pull: number, + base: sha(item.base, `${at}'s base`), + head: sha(item.head, `${at}'s head`), + size: oneOf(item.size, SIZES, `${at}'s size`), + tier: oneOf(item.tier, TIERS, `${at}'s tier`), + reason: text(item.reason, `${at}'s reason`), + writer: item.writer === null ? null : text(item.writer, `${at}'s writer`), + }; + if (item.clean !== undefined) { + if (item.clean !== true || item.defects !== undefined) fail(`${at} is clean and has defects`); + return { ...each, clean: true }; + } + if (!Array.isArray(item.defects) || item.defects.length === 0) fail(`${at} has no defects and is not clean`); + const place = (p: unknown): Place => { + if (!isObject(p)) return fail(`a defect's place in ${at}`); + exact(p, ["file", "from", "to"], `a defect's place in ${at}`); + const from = line(p.from, `a defect's first line in ${at}`); + const to = line(p.to, `a defect's last line in ${at}`); + if (to < from) fail(`a defect in ${at} ends before it starts`); + return { file: text(p.file, `a defect's file in ${at}`), from, to }; + }; + const defects = (item.defects as unknown[]).map((d): Defect => { + if (!isObject(d)) return fail(`a defect of ${at}`); + exact(d, ["fix", "what", "at"], `a defect of ${at}`); + if (!Array.isArray(d.at) || d.at.length === 0) fail(`a defect in ${at} is nowhere`); + return { + fix: pull(d.fix, `a defect's fix in ${at}`), + what: text(d.what, `a defect's description in ${at}`), + at: (d.at as unknown[]).map(place), + }; + }); + return { ...each, defects }; + }); + return { repository: repository(value.repository, "repository"), cases }; +} + +/** The set at `path`, or null when there is none. */ +export function readEvalSet(path: string): EvalSet | null { + let text: string; + try { + text = readFileSync(path, "utf8"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; + throw error; + } + try { + return parseEvalSet(JSON.parse(text)); + } catch (error) { + throw new ReviewInputError(`${path}: ${(error as Error).message}`); + } +} + +const writeEvalSet = (path: string, set: EvalSet) => + writeFileSync( + path, + `${JSON.stringify({ ...set, cases: [...set.cases].sort((a, b) => a.pull - b.pull) }, null, "\t")}\n`, + ); + +const lower = (repository: Repository) => `${repository.owner}/${repository.repo}`.toLowerCase(); + +/** + * Where `root` fetches `repository`'s commits from: a remote that names it, else its GitHub URL in origin's scheme, + * so a contributor's clone of a fork scores the set of the repository it came from. + */ +export function source(root: string, repository: string): string { + const remotes = git(root, ["remote"]).split("\n").filter(Boolean); + const named = (name: string) => { + const remote = readRemote(root, name); + const found = remote && remoteRepository(remote).repository; + return found ? lower(found) : null; + }; + const remote = remotes.find((name) => named(name) === repository); + if (remote !== undefined) return remote; + const origin = remotes.includes("origin") ? remoteRepository(requireRemote(root, "origin")).url : ""; + return /^(?:ssh:\/\/|[^@/:]+@)/.test(origin) + ? `git@github.com:${repository}.git` + : `https://github.com/${repository}.git`; +} + +/** Whether `sha` is a commit the clone holds. */ +function holds(root: string, sha: string): boolean { + try { + git(root, ["cat-file", "-e", `${sha}^{commit}`]); + return true; + } catch { + return false; + } +} + +/** Fetch each of `refs` the clone lacks from `from`; GitHub serves a merged PR's head and any reachable commit. */ +function fetch(root: string, from: string, refs: string[]): void { + const missing = refs.filter((ref) => !SHA.test(ref) || !holds(root, ref)); + if (missing.length > 0) git(root, ["fetch", "--quiet", "--no-tags", from, ...missing]); +} + +/** What `quest eval add` records. */ +export interface AddOptions { + root: string; + /** The set's path; defaults to {@link EVAL_FILE} at the checkout's root. */ + set?: string; + pull: number; + /** Record it as a PR that broke nothing. */ + clean?: boolean; + /** A defect to add to a culprit: which PR fixed it, what was wrong, and each place it is at the head. */ + defect?: Omit & { at: string[] }; + env: NodeJS.ProcessEnv; +} + +/** Where a defect sits, from `path:from-to` or `path:line`. */ +export function defectLines(at: string): Place { + const match = /^(.+):(\d+)(?:-(\d+))?$/.exec(at); + const from = Number(match?.[2]); + const to = match?.[3] === undefined ? from : Number(match[3]); + if (match === null || from < 1 || to < from) + throw new ReviewInputError(`--defect takes path:from-to or path:line, not ${at}`); + return { file: match[1]!, from, to }; +} + +/** + * Add a PR to the set: as clean, or with a defect, which a culprit already listed gains. The first time, it reads the + * PR's last head and base from GitHub, fetches them, and records its merge base, its size and latest writer from its + * `## Models` record, and its tier as the classifier sets it, which may run a model. + */ +export async function addCase(options: AddOptions, deps: ClaimDependencies): Promise { + const root = git(options.root, ["rev-parse", "--show-toplevel"]); + const path = options.set ?? join(root, EVAL_FILE); + if (Boolean(options.clean) === (options.defect !== undefined)) + throw new ReviewInputError("pass either --clean or --defect with --fix and --what"); + // Before anything costs a request or a classifier session. + const places = options.defect?.at.map(defectLines); + // A new set is the repository's that origin's fork chain leads to, as quest start claims there. + const up = await upstream(root, requireRemote(root, "origin"), deps); + if (up.github === null) throw new ReviewInputError("origin is not on GitHub"); + const set = readEvalSet(path) ?? { repository: lower(up.github.repository), cases: [] }; + const [owner, repo] = set.repository.split("/") as [string, string]; + const repository = { owner, repo }; + const from = source(root, set.repository); + let entry = set.cases.find((each) => each.pull === options.pull); + if (entry !== undefined && (options.clean || entry.clean)) + throw new ReviewInputError( + `#${options.pull} is already in the set${entry.clean ? " as clean" : " as a culprit"}`, + ); + if (entry === undefined) { + entry = await resolveCase(root, repository, from, options, up.github.client); + if (options.clean) entry.clean = true; + set.cases.push(entry); + } + if (options.defect !== undefined && places !== undefined) { + const { fix, what } = options.defect; + // A culprit from the set may be squash-merged, its head in no clone yet. + fetch(root, from, [entry.head]); + for (const place of places) { + const lines = fileLines(root, entry.head, place.file); + if (place.to > lines) throw new ReviewInputError(`${place.file} ends at line ${lines} at ${entry.head}`); + } + entry.defects = [...(entry.defects ?? []), { fix, what, at: places }]; + } + writeEvalSet(path, set); + return entry; +} + +/** How many lines `file` has at `sha`, failing when it is not there. */ +function fileLines(root: string, sha: string, file: string): number { + let text: string; + try { + text = git(root, ["show", `${sha}:${file}`]); + } catch { + throw new ReviewInputError(`${file} is not in the head ${sha}`); + } + return text.split("\n").length; +} + +/** A new case for PR `options.pull`, with no defects yet. */ +async function resolveCase( + root: string, + repository: Repository, + from: string, + options: AddOptions, + client: GitHub, +): Promise { + const response = await client.request(`${repositoryPath(repository)}/pulls/${options.pull}`, { allow: [404] }); + if (response.status === 404) throw new ReviewInputError(`${lower(repository)} has no PR #${options.pull}`); + const row = object(await response.json()); + if (!row.merged_at) throw new ReviewInputError(`#${options.pull} never merged`); + const head = String(object(row.head).sha); + const tip = String(object(row.base).sha); + const branch = String(object(row.head).ref); + fetch(root, from, [`refs/pull/${options.pull}/head`, tip]); + if (!holds(root, head)) throw new ReviewInputError(`origin serves no head ${head} for #${options.pull}`); + const base = git(root, ["merge-base", tip, head]); + const own = recorded(typeof row.body === "string" ? row.body : ""); + const named = own.sizes.find((size) => (SIZES as readonly string[]).includes(size)) as Size | undefined; + // The quest's size at the base's tip, as quest review reads it. + const size = reviewSize(root, tip, branch, undefined, named ?? null).size; + const writer = own.implementers.at(-1)?.fields?.model ?? null; + const dir = evalDir(root); + const { value } = await classify({ + root, + baseSha: base, + change: { base, mergeBase: base, head }, + dir, + env: options.env, + }); + return { + pull: options.pull, + base, + head, + size, + tier: value.tier, + reason: value.reason, + writer: writer === "unknown" ? null : writer, + }; +} + +/** The clone's directory for eval checkouts and reply schemas. */ +function evalDir(root: string): string { + const dir = join(git(root, ["rev-parse", "--path-format=absolute", "--git-common-dir"]), "quest", "eval"); + mkdirSync(dir, { recursive: true }); + return dir; +} + +/** What `quest eval run` runs. */ +export interface EvalOptions { + root: string; + /** The set's path; defaults to {@link EVAL_FILE} at the checkout's root. */ + set?: string; + /** The only reviewer, at its size's entry, with `model` and `effort` overriding it; else the pick and its shadow. */ + harness?: HarnessName; + model?: string; + effort?: string; + /** Only these PRs of the set. */ + only?: number[]; + env: NodeJS.ProcessEnv; + signal?: AbortSignal; + /** Called as each case starts, with its place in the run. */ + progress?: (index: number, total: number, pull: number) => void; +} + +/** One reviewer's round on a case. */ +export interface Seated { + reviewer: HarnessName; + model: string; + effort: string; + /** The size's pick one effort lower, beside the pick as `quest review` runs it. */ + shadow?: true; + session: string; + usage: Usage[]; + score: number; + findings: Finding[]; + /** The index in the case's defects of each defect a finding names a line of. */ + found: number[]; +} + +/** A case and every reviewer's round on it. */ +export interface Scored { + case: EvalCase; + seats: Seated[]; +} + +/** Defects found and noise raised by one group of rounds. */ +export interface Tally { + /** Culprits reviewed, and their defects found and in all. */ + culprits: number; + found: number; + defects: number; + /** Clean PRs reviewed, and their findings by severity. */ + clean: number; + noise: Record<(typeof SEVERITIES)[number], number>; +} + +export interface EvalReport { + repository: string; + /** The review rules the reviewers were given: the checkout's REVIEW.md at its HEAD, or null without one. */ + rules: { sha: string } | null; + scored: Scored[]; + /** + * Each seat over every case: `pick`, `shadow`, and, unless `--harness` named one reviewer, `either`, what the + * case's seats found together, as a run of the pick and its shadow finds it. + */ + totals: Record; + /** Each seat by the case's size and tier, as `pick M` or `shadow high`. */ + by: Record; + /** The cost of the run's sessions at API list price, as token-max prices them, or null when it is not installed. */ + cost: { usd: number; complete: boolean; priced: number; sessions: number } | null; +} + +/** The reviewers a case gets: `harness`'s alone, or the size's pick, avoiding its writer, and the pick's shadow. */ +export function evalSeats(entry: EvalCase, options: Pick) { + if (options.harness !== undefined) + return [{ reviewer: pickReviewer(entry.size, options, () => false), shadow: undefined }]; + const pick = pickReviewer(entry.size, {}, (model: Model) => model.model === entry.writer); + const shadow = shadowOf(pick); + return [{ reviewer: pick, shadow: undefined }, ...(shadow ? [{ reviewer: shadow, shadow: true as const }] : [])]; +} + +/** The indexes of `defects` a finding names a line of. */ +export function foundDefects(defects: Defect[], findings: Finding[]): number[] { + const on = (f: Finding, p: Place) => f.file === p.file && f.line !== null && f.line >= p.from && f.line <= p.to; + return defects.flatMap((defect, index) => (findings.some((f) => defect.at.some((p) => on(f, p))) ? [index] : [])); +} + +/** + * Review every case of the set at its head, one case at a time with its reviewers at once, and score them. The first + * failure stops the run and fails it whole, as a partial score would read as the reviewer's. + */ +export async function evaluate(options: EvalOptions, sessions: (since: Date) => Session[] | null): Promise { + if ((options.model !== undefined || options.effort !== undefined) && options.harness === undefined) + throw new ReviewInputError("--model and --effort name --harness's reviewer; pass --harness too"); + const root = git(options.root, ["rev-parse", "--show-toplevel"]); + const path = options.set ?? join(root, EVAL_FILE); + const set = readEvalSet(path); + if (set === null) throw new ReviewInputError(`no eval set at ${path}; add cases with quest eval add`); + const { repository } = set; + const unknown = (options.only ?? []).filter((pull) => !set.cases.some((each) => each.pull === pull)); + if (unknown.length > 0) throw new ReviewInputError(`not in the set: ${unknown.map((n) => `#${n}`).join(", ")}`); + const cases = set.cases.filter((each) => options.only === undefined || options.only.includes(each.pull)); + if (cases.length === 0) throw new ReviewInputError("the set has no cases"); + // Every reviewer is picked before any runs, so an argument that picks none fails before tokens are spent. + const seats = new Map(cases.map((each) => [each, evalSeats(each, options)])); + let rules: { sha: string; text: string } | null = null; + try { + rules = { sha: git(root, ["rev-parse", "HEAD"]), text: git(root, ["show", "HEAD:REVIEW.md"]) }; + } catch { + // No committed REVIEW.md: no rules. + } + const dir = evalDir(root); + const started = new Date(); + fetch( + root, + source(root, repository), + cases.flatMap((each) => [each.base, each.head]), + ); + + const scored: Scored[] = []; + for (const [index, entry] of cases.entries()) { + options.progress?.(index + 1, cases.length, entry.pull); + scored.push({ case: entry, seats: await reviewCase(root, dir, entry, seats.get(entry)!, rules, options) }); + } + return { + repository, + rules: rules && { sha: rules.sha }, + scored, + ...tallies(scored, options.harness === undefined), + cost: priced(sessions(started), scored), + }; +} + +/** Every reviewer's round on `entry`, in a detached checkout of its head removed afterwards. */ +async function reviewCase( + root: string, + dir: string, + entry: EvalCase, + seats: ReturnType, + rules: { sha: string; text: string } | null, + options: EvalOptions, +): Promise { + // Its own name, so a concurrent run of the same case keeps its checkout. + const checkout = join(dir, `${entry.pull}-${randomUUID()}`); + git(root, ["worktree", "add", "--quiet", "--detach", checkout, entry.head]); + try { + const changed = git(root, ["diff", "--name-only", "-z", `${entry.base}..${entry.head}`]) + .split("\0") + .filter(Boolean); + const risk: Assessment = { tier: entry.tier, reason: entry.reason }; + const opening = { + root: checkout, + change: { base: entry.base, mergeBase: entry.base, head: entry.head }, + files: baseInstructions(root, entry.base, changed), + rules, + risk, + dir, + env: options.env, + }; + const controller = new AbortController(); + const signal = options.signal ? AbortSignal.any([options.signal, controller.signal]) : controller.signal; + const results = await Promise.allSettled( + seats.map(({ reviewer, shadow }) => + firstRound(reviewer, { ...opening, signal }).then( + ({ session, usage, verdict }): Seated => ({ + reviewer: reviewer.harness, + model: reviewer.model, + effort: reviewer.effort, + ...(shadow && { shadow }), + session, + usage, + score: verdict.score, + findings: verdict.findings, + found: foundDefects(entry.defects ?? [], verdict.findings), + }), + (error: unknown) => { + controller.abort(error); + throw error; + }, + ), + ), + ); + if (controller.signal.aborted) throw controller.signal.reason; + return results.map((result) => (result as PromiseFulfilledResult).value); + } finally { + git(root, ["worktree", "remove", "--force", checkout]); + } +} + +const empty = (): Tally => ({ culprits: 0, found: 0, defects: 0, clean: 0, noise: { high: 0, medium: 0, low: 0 } }); + +function count(tally: Tally, entry: EvalCase, found: number[], findings: Finding[]): void { + if (entry.clean) { + tally.clean++; + for (const finding of findings) tally.noise[finding.severity]++; + } else { + tally.culprits++; + tally.defects += entry.defects!.length; + tally.found += found.length; + } +} + +/** + * The run's tallies: each seat in all, and by size and tier, with `either` for what a run of the pick and its shadow + * found together when `combined`, every case counting, as one whose pick has no shadow. + */ +export function tallies(scored: Scored[], combined: boolean): Pick { + const totals: Record = {}; + const by: Record = {}; + const add = (seat: string, entry: EvalCase, found: number[], findings: Finding[]) => { + for (const [into, key] of [ + [totals, seat], + [by, `${seat} ${entry.size}`], + [by, `${seat} ${entry.tier}`], + ] as const) + count((into[key] ??= empty()), entry, found, findings); + }; + for (const { case: entry, seats } of scored) { + for (const seat of seats) add(seat.shadow ? "shadow" : "pick", entry, seat.found, seat.findings); + // A case whose pick has no shadow, as one at low effort, still counts toward what the run found. + if (combined) { + const found = [...new Set(seats.flatMap((seat) => seat.found))]; + // Both seats' findings on a clean PR, as a run of both raises them; the same defect twice is still one. + add("either", entry, found, entry.clean ? seats.flatMap((seat) => seat.findings) : []); + } + } + return { totals, by }; +} + +/** + * The cost of `scored`'s sessions and their descendants, such as the sub-session Codex runs its review in, among those + * token-max exported, or null when it is not installed. It is a lower bound when a seat's session is missing or one + * is incomplete. + */ +export function priced(exported: Session[] | null, scored: Scored[]): EvalReport["cost"] { + if (exported === null) return null; + const id = (harness: string, session: string | null) => JSON.stringify([harness, session]); + const seats = scored.flatMap(({ seats }) => seats.map((seat) => id(seat.reviewer, seat.session))); + const ours = new Set(seats); + const matched = new Set(); + for (let grew = true; grew;) { + grew = false; + for (const each of exported) + if ( + !matched.has(each) && + (ours.has(id(each.harness, each.id)) || ours.has(id(each.harness, each.parent))) + ) { + matched.add(each); + // A session naming no ID has no children to find, and adding it would claim every top session. + if (each.id !== null) ours.add(id(each.harness, each.id)); + grew = true; + } + } + const found = seats.filter((seat) => [...matched].some((each) => id(each.harness, each.id) === seat)).length; + return { + usd: [...matched].reduce((sum, each) => sum + each.cost, 0), + complete: found === seats.length && [...matched].every((each) => each.complete), + priced: found, + sessions: seats.length, + }; +} + +const share = ({ found, defects }: Tally) => + `${found}/${defects} defects${defects ? ` (${Math.round((100 * found) / defects)}%)` : ""}`; +const noise = ({ clean, noise }: Tally) => + `${noise.high + noise.medium + noise.low} findings on ${clean} clean (${noise.high} high, ${noise.medium} medium, ${noise.low} low)`; +/** Each turn's tokens as the CLI reported them; Codex reports none for its review turn, which runs in a sub-session. */ +const tokens = (usage: Usage[]) => + usage.length === 0 + ? "no usage reported" + : usage.map((u) => `${u.turn} ${u.input} in (${u.cached} cached), ${u.output} out`).join(", "); + +/** The report as `quest eval run` prints it. */ +export function evalLines(report: EvalReport): string[] { + const culprits = report.scored.filter(({ case: c }) => !c.clean).length; + const lines = [ + `${report.repository}: ${report.scored.length} cases, ${culprits} culprits and ${report.scored.length - culprits} clean; rules ${report.rules ? `REVIEW.md at ${report.rules.sha.slice(0, 12)}` : "none"}`, + ]; + for (const [seat, tally] of Object.entries(report.totals)) lines.push(`${seat}: ${share(tally)}; ${noise(tally)}`); + for (const [key, tally] of Object.entries(report.by).sort(([a], [b]) => a.localeCompare(b))) + lines.push(` ${key}: ${share(tally)}; ${noise(tally)}`); + for (const { case: entry, seats } of report.scored) { + const kind = entry.clean ? "clean" : `${entry.defects!.length} defect(s)`; + lines.push(`#${entry.pull} ${entry.size} ${entry.tier} ${kind}`); + for (const seat of seats) { + const name = `${seat.reviewer} ${seat.model} ${seat.effort}${seat.shadow ? " shadow" : ""}`; + const result = entry.clean ? "" : ` found ${seat.found.length}/${entry.defects!.length},`; + lines.push( + ` ${name}: score ${seat.score},${result} ${seat.findings.length} findings; ${tokens(seat.usage)}; session ${seat.session}`, + ); + } + } + const { cost } = report; + lines.push( + cost === null + ? "cost: unpriced, since token-max is not installed" + : `cost: ~$${cost.usd.toFixed(2)} for ${cost.priced} of ${cost.sessions} sessions${cost.complete ? "" : ", a lower bound"}`, + ); + return lines; +} diff --git a/src/cli/review.ts b/src/cli/review.ts index a3e1a4fd..119c711b 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -2109,17 +2109,12 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi let session: string; let reply: Reply; if (!resume) { - const prompt = reviewPrompt(change, rules, earlier, answered, first); - const opened = await turn(cli, reviewer.review(context, randomUUID(), prompt), root, context.model); - if (opened.session === undefined) throw new Error(`${harness} did not report its session`); - session = opened.session; - if (reported(opened.usage)) usage.push({ turn: "review", ...opened.usage }); - reply = await turn( - cli, - reviewer.resume(context, session, reportPrompt(change.head, shared.classified, earlier, first)), - root, - context.model, - ); + const opened = await open(cli, context, root, { + review: reviewPrompt(change, rules, earlier, answered, first), + report: reportPrompt(change.head, shared.classified, earlier, first), + }); + ({ session, reply } = opened); + usage.push(...opened.usage); } else { session = stored!.session; const prompt = laterPrompt({ @@ -2178,6 +2173,59 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi }; } +/** A new session's two turns: the CLI's own review, then the report of it in the schema, with the review turn's usage. */ +async function open( + cli: Cli, + context: Context, + root: string, + prompts: { review: string; report: string }, +): Promise<{ session: string; reply: Reply; usage: Usage[] }> { + const opened = await turn(cli, cli.reviewer.review(context, randomUUID(), prompts.review), root, context.model); + if (opened.session === undefined) throw new Error(`${cli.harness} did not report its session`); + const session = opened.session; + const reply = await turn(cli, cli.reviewer.resume(context, session, prompts.report), root, context.model); + return { session, reply, usage: reported(opened.usage) ? [{ turn: "review", ...opened.usage }] : [] }; +} + +/** What a lone first round reads: a checkout at the head, the change, the base's instructions, the rules, and the tier. */ +export interface Opening { + root: string; + change: Change; + files: { path: string; text: string }[]; + rules: Rules | null; + /** The head's tier as classified before review. */ + risk: Assessment; + /** Where the reply schema is saved. */ + dir: string; + env: NodeJS.ProcessEnv; + signal?: AbortSignal; +} + +/** One reviewer's first round on a change, in a new session no branch keeps, with the prompts `quest review` gives. */ +export async function firstRound( + reviewer: Model, + opening: Opening, +): Promise<{ session: string; usage: Usage[]; verdict: Verdict }> { + const { root, change } = opening; + const cli = await prepare(reviewer.harness, root, opening.env, opening.signal); + const replySchema = schema(0); + const context: Context = { + mergeBase: change.mergeBase, + instructions: instructionsPrompt(change.mergeBase, opening.files), + model: reviewer.model, + effort: reviewer.effort, + schema: replySchema, + schemaFile: saveSchema(opening.dir, replySchema), + ...(cli.env.OPENAI_BASE_URL && { openaiBaseUrl: cli.env.OPENAI_BASE_URL }), + }; + const { session, reply, usage } = await open(cli, context, root, { + review: reviewPrompt(change, opening.rules), + report: reportPrompt(change.head, opening.risk), + }); + if (reported(reply.usage)) usage.push({ turn: "findings", ...reply.usage }); + return { session, usage, verdict: verdict(reply, 0) }; +} + /** * The author's next step after a run's `rounds`, printed last so each step of the * procedure arrives when it applies: handling findings that are all low diff --git a/test/eval.test.ts b/test/eval.test.ts new file mode 100644 index 00000000..6937c70b --- /dev/null +++ b/test/eval.test.ts @@ -0,0 +1,421 @@ +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { afterEach, expect, test, vi } from "vitest"; + +import { + type Defect, + type EvalCase, + type Scored, + type Seated, + defectLines, + evalSeats, + foundDefects, + parseEvalSet, + priced, + tallies, +} from "../src/cli/review-eval"; +import type { Finding } from "../src/cli/review"; +import type { Session } from "../src/cli/usage"; +import { type Call, effortOf, fakes, git, quest, setup, write } from "./review-fakes"; + +afterEach(() => { + vi.unstubAllEnvs(); + vi.unstubAllGlobals(); +}); + +const A = "a".repeat(40); +const B = "b".repeat(40); +const base = { base: A, head: B, size: "M", tier: "medium", reason: "It changes a command.", writer: null }; +const defect: Defect = { fix: 9, what: "It drops the last item.", at: [{ file: "src/a.ts", from: 10, to: 12 }] }; +const finding = (file: string, line: number | null, severity: Finding["severity"] = "high"): Finding => ({ + file, + line, + severity, + title: "t", + body: "b", +}); + +test("an eval set names each case once, as a culprit with defects or as clean", () => { + const set = { + repository: "acme/widget", + cases: [ + { pull: 3, ...base, defects: [defect] }, + { pull: 4, ...base, clean: true }, + ], + }; + expect(parseEvalSet(set)).toEqual(set); + const bad = (cases: unknown[], message: string) => + expect(() => parseEvalSet({ repository: "acme/widget", cases })).toThrow(message); + bad([{ pull: 3, ...base }], "case #3 has no defects and is not clean"); + bad([{ pull: 3, ...base, defects: [] }], "case #3 has no defects and is not clean"); + bad([{ pull: 3, ...base, clean: true, defects: [defect] }], "case #3 is clean and has defects"); + bad([{ pull: 3, ...base, clean: false }], "case #3 is clean and has defects"); + bad( + [ + { pull: 3, ...base, clean: true }, + { pull: 3, ...base, clean: true }, + ], + "case #3 is listed twice", + ); + bad([{ pull: 3, ...base, head: "main", clean: true }], "case #3's head"); + bad( + [{ pull: 3, ...base, defects: [{ ...defect, at: [{ file: "src/a.ts", from: 12, to: 10 }] }] }], + "a defect in case #3 ends before it starts", + ); + bad([{ pull: 3, ...base, defects: [{ ...defect, line: 4 }] }], "has an unknown field line"); + bad([{ pull: 3, ...base, defects: [{ ...defect, at: [] }] }], "a defect in case #3 is nowhere"); + bad([{ pull: 3, ...base, tier: "severe", clean: true }], "case #3's tier"); + expect(() => parseEvalSet({ repository: "Acme/Widget", cases: [] })).toThrow("lowercase owner/repo"); +}); + +test("a defect is a path and a line or an inclusive range", () => { + expect(defectLines("src/a.ts:4")).toEqual({ file: "src/a.ts", from: 4, to: 4 }); + expect(defectLines("src/a:b.ts:4-9")).toEqual({ file: "src/a:b.ts", from: 4, to: 9 }); + for (const at of ["src/a.ts", "src/a.ts:0", "src/a.ts:9-4", "src/a.ts:x"]) + expect(() => defectLines(at)).toThrow("--defect takes path:from-to or path:line"); +}); + +test("a finding finds a defect only on one of its places' lines in its file", () => { + const other: Defect = { + ...defect, + at: [ + { file: "src/b.ts", from: 1, to: 1 }, + { file: "src/c.ts", from: 5, to: 6 }, + ], + }; + expect(foundDefects([defect, other], [finding("src/c.ts", 6)])).toEqual([1]); + expect(foundDefects([defect, other], [finding("src/a.ts", 10)])).toEqual([0]); + expect(foundDefects([defect, other], [finding("src/a.ts", 12), finding("src/b.ts", 1)])).toEqual([0, 1]); + expect(foundDefects([defect], [finding("src/a.ts", 9), finding("src/a.ts", 13)])).toEqual([]); + expect(foundDefects([defect], [finding("src/a.ts", null), finding("src/c.ts", 11)])).toEqual([]); +}); + +test("the pick avoids the case's writer, and --harness runs alone at its size's entry", () => { + const entry = { pull: 3, ...base, size: "S", clean: true } as EvalCase; + const names = (seats: ReturnType) => + seats.map(({ reviewer, shadow }) => `${reviewer.model} ${reviewer.effort}${shadow ? " shadow" : ""}`); + expect(names(evalSeats(entry, {}))).toEqual(["gpt-6.1-sol medium", "gpt-6.1-sol low shadow"]); + expect(names(evalSeats({ ...entry, writer: "gpt-6.1-sol" }, {}))).toEqual(["claude-opus-5-5 low"]); + expect(names(evalSeats({ ...entry, writer: "gpt-6.1-sol" }, { harness: "codex" }))).toEqual(["gpt-6.1-sol medium"]); + expect(names(evalSeats(entry, { harness: "claude", effort: "high" }))).toEqual(["claude-opus-5-5 high"]); +}); + +const seat = (found: number[], findings: Finding[], shadow = false): Seated => ({ + reviewer: "codex", + model: "m", + effort: shadow ? "low" : "medium", + ...(shadow && { shadow: true as const }), + session: shadow ? "s2" : "s1", + usage: [], + score: 8, + findings, + found, +}); + +test("tallies count defects found and clean findings per seat, size, and tier, and what either seat found", () => { + const culprit = { + pull: 3, + ...base, + defects: [defect, { ...defect, at: [{ file: "src/a.ts", from: 20, to: 20 }] }], + } as EvalCase; + const clean = { pull: 4, ...base, size: "S", tier: "low", clean: true } as EvalCase; + const scored: Scored[] = [ + { case: culprit, seats: [seat([0], [finding("src/a.ts", 10)]), seat([1], [finding("src/a.ts", 20)], true)] }, + { + case: clean, + seats: [ + seat([], [finding("x", 1, "high"), finding("x", 2, "low")]), + seat([], [finding("x", 1, "medium")], true), + ], + }, + ]; + const { totals, by } = tallies(scored, true); + expect(totals.pick).toEqual({ culprits: 1, found: 1, defects: 2, clean: 1, noise: { high: 1, medium: 0, low: 1 } }); + expect(totals.shadow).toEqual({ + culprits: 1, + found: 1, + defects: 2, + clean: 1, + noise: { high: 0, medium: 1, low: 0 }, + }); + expect(totals.either).toEqual({ + culprits: 1, + found: 2, + defects: 2, + clean: 1, + noise: { high: 1, medium: 1, low: 1 }, + }); + expect(by["pick M"]).toMatchObject({ culprits: 1, found: 1, clean: 0 }); + expect(by["pick low"]).toMatchObject({ culprits: 0, clean: 1 }); + expect(Object.keys(by).sort()).toEqual( + ["either M", "either S", "either low", "either medium", "pick M", "pick S", "pick low", "pick medium"] + .concat(["shadow M", "shadow S", "shadow low", "shadow medium"]) + .sort(), + ); +}); + +test("either counts a case whose pick has no shadow, and a run of one named reviewer has none", () => { + const lone = { pull: 5, ...base, defects: [defect] } as EvalCase; + const scored: Scored[] = [{ case: lone, seats: [seat([0], [finding("src/a.ts", 11)])] }]; + expect(tallies(scored, true).totals.either).toMatchObject({ culprits: 1, found: 1, defects: 1 }); + expect(tallies(scored, false).totals).toEqual({ pick: expect.objectContaining({ found: 1 }) }); +}); + +test("the cost sums the run's sessions token-max priced, a lower bound when one is missing or incomplete", () => { + const scored: Scored[] = [ + { case: { pull: 4, ...base, clean: true } as EvalCase, seats: [seat([], []), seat([], [], true)] }, + ]; + const session = (id: string, cost: number, complete = true) => + ({ harness: "codex", id, parent: null, cost, complete }) as Session; + expect(priced(null, scored)).toBeNull(); + expect(priced([session("s1", 1.5), session("s2", 0.25), session("s3", 9)], scored)).toEqual({ + usd: 1.75, + complete: true, + priced: 2, + sessions: 2, + }); + expect(priced([session("s1", 1.5)], scored)).toMatchObject({ usd: 1.5, complete: false, priced: 1 }); + expect(priced([session("s1", 1.5), session("s2", 1, false)], scored)).toMatchObject({ complete: false }); + expect(priced([{ ...session("s1", 1.5), harness: "claude" }], scored)).toMatchObject({ priced: 0 }); + // Codex reviews in a sub-session of the seat's, which may spawn its own. + const child = (id: string, parent: string, cost: number) => ({ ...session(id, cost), parent }); + expect( + priced( + [child("c2", "c1", 0.5), session("s1", 1), child("c1", "s1", 2), child("x", "y", 9), session("s2", 1)], + scored, + ), + ).toEqual({ usd: 4.5, complete: true, priced: 2, sessions: 2 }); + // A sub-session naming no ID is counted, but claims no top session as its child. + const anonymous = { ...child("c1", "s1", 2), id: null }; + expect(priced([session("s1", 1), anonymous, session("x", 9)], scored)).toMatchObject({ usd: 3, priced: 1 }); +}); + +/** + * The fixture clone on GitHub as acme/widget: PR 7 is its feature branch, a culprit, and PR 8 another branch, clean. + * The API answers each PR's head and base, and the bodies name its size and writer. + */ +function onGitHub(repo: string) { + const local = git(repo, "remote", "get-url", "origin"); + git(repo, "remote", "set-url", "origin", "https://github.com/acme/widget.git"); + git(repo, "config", `url.${local}.insteadOf`, "https://github.com/acme/widget.git"); + const main = git(repo, "rev-parse", "origin/main"); + git(repo, "push", "-q", "origin", "feature:refs/pull/7/head"); + git(repo, "checkout", "-qb", "other", "origin/main"); + write(repo, "src/other.ts", "export const y = 1;\n"); + git(repo, "add", "."); + git(repo, "commit", "-qm", "other"); + git(repo, "push", "-q", "origin", "other:refs/pull/8/head"); + const heads: Record = { 7: git(repo, "rev-parse", "feature"), 8: git(repo, "rev-parse", "other") }; + git(repo, "checkout", "-q", "feature"); + // The fixture's commits are pushed, so the eval fetches them as it would a real PR's. + git(repo, "branch", "-qD", "other"); + vi.stubEnv("GH_TOKEN", "token"); + const info = (login: string, parent: string | null) => ({ + owner: { login }, + name: "widget", + fork: parent !== null, + ...(parent === null ? {} : { parent: { owner: { login: parent }, name: "widget" } }), + default_branch: "main", + clone_url: `https://github.com/${login}/widget.git`, + ssh_url: `git@github.com:${login}/widget.git`, + }); + vi.stubGlobal("fetch", async (input: URL) => { + const url = new URL(input); + if (url.pathname === "/repos/acme/widget") return new Response(JSON.stringify(info("acme", null))); + // A contributor's fork, whose parent keeps the quest tree. + if (url.pathname === "/repos/me/widget") return new Response(JSON.stringify(info("me", "acme"))); + if (url.pathname === "/repos/acme/widget/contents/quest/README.md") return new Response("{}"); + const pull = /^\/repos\/acme\/widget\/pulls\/(\d+)$/.exec(url.pathname)?.[1]; + if (pull === undefined || heads[pull] === undefined) return new Response("{}", { status: 404 }); + return new Response( + JSON.stringify({ + merged_at: "2026-11-01T00:00:00Z", + head: { sha: heads[pull], ref: pull === "7" ? "feature" : "other" }, + base: { sha: main }, + body: `## Models\n\n- Size: S\n- Implementer: Claude Code, claude-opus-5-5, high effort\n`, + }), + ); + }); + return { heads, local }; +} + +fakes( + "quest eval adds cases from GitHub and scores each reviewer at the case's head, leaving no checkout", + async () => { + const s = setup(null); + const { heads, local } = onGitHub(s.repo); + const set = join(s.repo, "review-eval.json"); + // token-max lists the fake Codex seats' session, the sub-session it reviews in, and another session, so the cost + // does not depend on whether the host has token-max installed. + const listed = (session: string, parent: string | null, costUsd: number) => + JSON.stringify({ + ...{ host: null, harness: "codex", session, parent, review: false, cwd: null, repositories: null }, + ...{ branches: [], start: "2026-11-14T00:00:00Z", end: "2026-11-14T00:01:00Z", models: [], costUsd }, + ...{ complete: true, skippedLines: 0, files: [] }, + }); + const tokenMax = [listed("thread-1", null, 0.25), listed("review-1", "thread-1", 0.5), listed("x", null, 9)]; + writeFileSync(join(s.bin, "token-max"), `#!/bin/sh\nprintf '%s\\n' '${tokenMax.join("' '")}'\n`, { + mode: 0o755, + }); + + const added = await quest( + "--root", + s.repo, + "eval", + "add", + "7", + "--defect", + "src/lib.ts:1", + "--fix", + "9", + "--what", + "It sets x to 2.", + ); + expect(added.stderr).toBe(""); + expect(added.code).toBe(0); + expect(JSON.parse(added.stdout)).toMatchObject({ + pull: 7, + base: git(s.repo, "rev-parse", "origin/main"), + head: heads[7], + size: "S", + tier: "medium", + writer: "claude-opus-5-5", + defects: [{ fix: 9, what: "It sets x to 2.", at: [{ file: "src/lib.ts", from: 1, to: 1 }] }], + }); + expect((await quest("--root", s.repo, "eval", "add", "8", "--clean")).code).toBe(0); + expect(await quest("--root", s.repo, "eval", "add", "8", "--clean")).toMatchObject({ + code: 2, + stderr: "error: #8 is already in the set as clean\n", + }); + expect( + await quest("--root", s.repo, "eval", "add", "7", "--defect", "src/lib.ts:2", "--fix", "9", "--what", "x"), + ).toMatchObject({ + code: 2, + stderr: `error: src/lib.ts ends at line 1 at ${heads[7]}\n`, + }); + // Both are checked before GitHub is asked about #6, which it does not have. + const typo = ["--fix", "9", "--what"]; + expect( + await quest("--root", s.repo, "eval", "add", "6", "--defect", "src/lib.ts:2-1", ...typo, "x"), + ).toMatchObject({ + code: 2, + stderr: "error: --defect takes path:from-to or path:line, not src/lib.ts:2-1\n", + }); + expect( + await quest("--root", s.repo, "eval", "add", "6", "--defect", "src/lib.ts:1", ...typo, ""), + ).toMatchObject({ + code: 2, + }); + expect(await quest("--root", s.repo, "eval", "add", "6")).toMatchObject({ + code: 2, + stderr: "error: pass either --clean or --defect with --fix and --what\n", + }); + expect(await quest("--root", s.repo, "eval", "add", "6", "--clean")).toMatchObject({ + code: 2, + stderr: "error: acme/widget has no PR #6\n", + }); + const second = ["--defect", "AGENTS.md:1", "--defect", "src/lib.ts:1", "--fix", "10", "--what", "Both."]; + expect((await quest("--root", s.repo, "eval", "add", "7", ...second)).code).toBe(0); + const saved = parseEvalSet(JSON.parse(readFileSync(set, "utf8"))); + expect(saved.cases.map((c) => c.pull)).toEqual([7, 8]); + expect(saved.cases[0]!.defects!.map((d) => d.at.map((p) => p.file))).toEqual([ + ["src/lib.ts"], + ["AGENTS.md", "src/lib.ts"], + ]); + + // The rules are the running checkout's, not the case's base's. + write(s.repo, "REVIEW.md", "ROOT-REVIEW rule\n"); + git(s.repo, "commit", "-qam", "rules"); + + vi.stubEnv( + "FAKE_REPLY", + JSON.stringify({ score: 6, rationale: "r", findings: [{ ...finding("src/lib.ts", 1), body: "x is 2." }] }), + ); + const run = await quest("--root", s.repo, "eval", "run", "--json"); + expect(run.code).toBe(0); + const report = JSON.parse(run.stdout); + expect(report.totals.pick).toEqual({ + culprits: 1, + found: 2, + defects: 2, + clean: 1, + noise: { high: 1, medium: 0, low: 0 }, + }); + expect(report.totals.either).toEqual({ + culprits: 1, + found: 2, + defects: 2, + clean: 1, + noise: { high: 2, medium: 0, low: 0 }, + }); + expect(report.rules).toEqual({ sha: git(s.repo, "rev-parse", "HEAD") }); + expect(report.cost).toEqual({ usd: 0.75, complete: true, priced: 4, sessions: 4 }); + expect(run.stderr).toContain("case 1/2: #7"); + + // Two seats, each a review turn and a findings turn, at each case's head in a checkout since removed. + const turns = s.calls().filter((call: Call) => effortOf(call) !== null); + expect(turns.map(effortOf).sort()).toEqual([ + "low", + "low", + "low", + "low", + "medium", + "medium", + "medium", + "medium", + ]); + const reviews = turns.filter((call: Call) => call.stdin.includes("The change under review")); + expect(reviews.map((call) => call.cwd.includes(join(".git", "quest", "eval")))).toEqual([ + true, + true, + true, + true, + ]); + for (const call of reviews) { + expect(call.stdin).toContain("ROOT-REVIEW rule"); + expect(call.stdin).not.toContain("BASE-REVIEW rule"); + expect(call.argv.join(" ") + call.stdin).not.toContain("HEAD-ROOT rule"); + expect(existsSync(call.cwd)).toBe(false); + } + expect(git(s.repo, "worktree", "list").split("\n")).toHaveLength(1); + + const lines = await quest("--root", s.repo, "eval", "run", "--pr", "8", "--harness", "claude"); + expect(lines.code).toBe(0); + expect(lines.stdout).toContain("pick: 0/0 defects; 1 findings on 1 clean (1 high, 0 medium, 0 low)"); + expect(lines.stdout).toContain( + "claude claude-opus-5-5 low: score 6, 1 findings; review 101 in (0 cached), 51 out, findings 100 in (80 cached), 7 out;", + ); + expect(await quest("--root", s.repo, "eval", "run", "--pr", "5")).toMatchObject({ + code: 2, + stderr: "error: not in the set: #5\n", + }); + + // A contributor's clone of a fork, which holds no PR head yet, fetches the set's from the repository it names. + const fork = join(s.repo, "..", "fork"); + git(s.repo, "clone", "-q", "--no-local", local, fork); + git(fork, "remote", "set-url", "origin", "https://github.com/me/widget.git"); + git(fork, "config", "--add", `url.${local}.insteadOf`, "https://github.com/me/widget.git"); + git(fork, "config", "--add", `url.${local}.insteadOf`, "https://github.com/acme/widget.git"); + expect(() => git(fork, "cat-file", "-e", heads[7]!)).toThrow(); + const more = await quest( + "--root", + fork, + "eval", + "add", + "7", + "--set", + set, + "--defect", + "src/lib.ts:1", + "--fix", + "11", + "--what", + "Again.", + ); + expect(more.stderr).toBe(""); + expect(JSON.parse(more.stdout).defects).toHaveLength(3); + const scored = await quest("--root", fork, "eval", "run", "--set", set, "--pr", "7", "--json"); + expect(scored.stderr).toContain("case 1/1: #7"); + expect(JSON.parse(scored.stdout).totals.pick).toMatchObject({ culprits: 1, found: 3, defects: 3 }); + }, +); diff --git a/test/review-fakes.ts b/test/review-fakes.ts new file mode 100644 index 00000000..67788021 --- /dev/null +++ b/test/review-fakes.ts @@ -0,0 +1,361 @@ +// The fake Codex and Claude Code CLIs and the fixture clone the review tests run them in. +import { execFileSync } from "node:child_process"; +import { chmodSync, mkdirSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { test, vi } from "vitest"; + +import { type Output, main } from "../src/cli/main"; +import { tempDir } from "./fixture"; + +/** The fake CLIs are shebang scripts found on a POSIX PATH, which Windows cannot run. */ +export const fakes = test.skipIf(process.platform === "win32"); + +export const FINDING = { + file: "src/lib.ts", + line: 2, + severity: "high", + title: "Off by one", + body: "average([3]) is 0.", +}; + +export const CODEX_LOGGED_OUT = + "codex is not logged in to its default profile (CODEX_HOME is unset); run `codex login`"; +export const CLAUDE_LOGGED_OUT = + "claude is not logged in to its default profile (CLAUDE_CONFIG_DIR is unset); run `claude auth login`"; + +/** + * The findings turn's reply, `reply`: FAKE_REPLY, or one finding. A JSON reply without `earlier` settles + * each earlier finding the prompt numbers as FAKE_DISPOSITION says, `fixed` by default, and one without `risk` + * reports FAKE_RAISE's tier, else the tier the prompt says the change was classified. FAKE_RAISE as + * `@` raises only in turns at that effort. + */ +const REPLY = ` +const listed = /\\n([\\s\\S]*?)\\n<\\/earlier-findings>/.exec(stdin)?.[1] ?? ""; +const earlier = [...listed.matchAll(/^(\\d+)\\. /gm)].map((m) => ({ + finding: Number(m[1]), + disposition: process.env.FAKE_DISPOSITION ?? "fixed", + reason: "Checked against the head.", +})); +const classified = /was classified (low|medium|high) risk/.exec(stdin)?.[1] ?? "medium"; +const [raise, raiseAt] = (process.env.FAKE_RAISE ?? "").split("@"); +const effort = argv.includes("--effort") + ? argv[argv.indexOf("--effort") + 1] + : JSON.parse((argv.find((arg) => arg.startsWith("model_reasoning_effort=")) ?? "=null").split("=")[1]); +const risk = raise && (!raiseAt || raiseAt === effort) + ? { tier: raise, reason: "It rewrites the wire format peers read." } + : { tier: classified, reason: "As classified." }; +let reply = process.env.FAKE_REPLY ?? JSON.stringify({ score: 8, rationale: "Lands once the off-by-one is fixed.", findings: [${JSON.stringify(FINDING)}] }); +try { + const parsed = JSON.parse(reply); + if (parsed && typeof parsed === "object" && !("earlier" in parsed)) reply = JSON.stringify({ ...parsed, earlier }); + if (parsed && typeof parsed === "object" && !("risk" in parsed)) reply = JSON.stringify({ ...JSON.parse(reply), risk }); +} catch {} +`; + +/** The classifier's reply: FAKE_RISK, or medium. */ +const RISK_REPLY = `JSON.stringify(process.env.FAKE_RISK ? JSON.parse(process.env.FAKE_RISK) : { tier: "medium", reason: "It changes what the library returns." })`; + +/** + * Records one call as its own file in FAKE_LOG, a directory, renamed into place + * once whole, named by its microsecond start so the files sort in call order. A + * fake the review aborts can die mid-write, and a partial line in one shared + * log would break the reader. + */ +const LOG = ` +const logged = (call) => { + const name = Math.round((performance.timeOrigin + performance.now()) * 1000).toString() + "-" + process.pid; + const file = require("node:path").join(process.env.FAKE_LOG, name); + fs.writeFileSync(file + ".tmp", JSON.stringify(call)); + fs.renameSync(file + ".tmp", file + ".json"); +}; +`; + +/** + * Fake reviewer CLIs. Each logs its argv, stdin, and the variables it was + * given, then answers as the FAKE_* variables say. FAKE_LOGGED_OUT removes the + * stored login, so only a credential variable the real CLI reads, or Claude + * Code's switch to a cloud provider, authenticates; like the real ones, + * `claude auth status` counts such a variable, and `codex login status` counts + * CODEX_ACCESS_TOKEN but not CODEX_API_KEY. FAKE_REJECTED has the API + * reject the login in each turn with the real CLI's error, in the style it + * names. FAKE_LIMITED fails a turn on the account's usage limit with the real + * CLI's error, naming its reset unless set to `unknown`. FAKE_CLASSIFY fails + * the classifier's turn of each `-rejected` or `-limited` it + * lists, the same way. FAKE_REPLY replaces the findings reply. + */ +export const FAKES: Record = { + claude: ` +const fs = require("node:fs");${LOG} +const argv = process.argv.slice(2); +const stdin = fs.readFileSync(0, "utf8"); +const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|CODEX|ANTHROPIC|FAKE_HOST|PATH$)/.test(k))); +logged({ argv, stdin, env, cwd: process.cwd() }); +const credential = ["ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"].some((k) => process.env[k]); +const provider = Object.entries(process.env).some(([k, v]) => k.startsWith("CLAUDE_CODE_USE_") && v === "1"); +const loggedIn = !process.env.FAKE_LOGGED_OUT || credential || provider; +if (argv[0] === "auth") process.exit(loggedIn ? 0 : 1); +const at = (flag) => argv[argv.indexOf(flag) + 1]; +const result = (fields) => console.log(JSON.stringify({ type: "result", is_error: false, ...fields })); +const rejected = process.env.FAKE_REJECTED ?? (loggedIn ? undefined : "status"); +if (rejected) { + const session_id = at("--session-id") ?? at("--resume"); + if (rejected === "status") { + result({ session_id, is_error: true, api_error_status: 401, result: "Failed to authenticate. API Error: 401 Invalid bearer token" }); + } else if (rejected === "stderr") { + console.error("Login expired · Please run /login"); + } else { + result({ session_id, is_error: true, result: "OAuth token revoked · Please run /login" }); + } + process.exit(1); +} +// The limit hits the findings turn, after the review turn passed. +if (process.env.FAKE_LIMITED && argv.includes("--resume")) { + if (process.env.FAKE_LIMITED !== "unknown") { + console.log(JSON.stringify({ type: "rate_limit_event", rate_limit_info: { status: "rejected", resetsAt: 1900000000 } })); + } + result({ session_id: at("--resume"), is_error: true, result: "You've hit your weekly limit · resets Nov 5, 3pm (UTC)" }); + process.exit(1); +} +const usage = { input_tokens: 0, cache_read_input_tokens: 0, cache_creation_input_tokens: 0, output_tokens: 0 }; +const failing = (process.env.FAKE_CLASSIFY ?? "").split(","); +if (argv.includes("--session-id") && argv.includes("--json-schema")) { + if (failing.includes("claude-rejected")) { + result({ session_id: at("--session-id"), is_error: true, api_error_status: 401, result: "Failed to authenticate. API Error: 401 Invalid bearer token" }); + process.exit(1); + } + if (failing.includes("claude-limited")) { + console.log(JSON.stringify({ type: "rate_limit_event", rate_limit_info: { status: "rejected", resetsAt: 1900000000 } })); + result({ session_id: at("--session-id"), is_error: true, result: "You've hit your weekly limit · resets Nov 5, 3pm (UTC)" }); + process.exit(1); + } + const reply = failing.includes("claude-unusable") ? JSON.stringify({ tier: "huge", reason: "x" }) : ${RISK_REPLY}; + result({ + session_id: at("--session-id"), + result: reply, + structured_output: JSON.parse(reply), + usage: { ...usage, input_tokens: 40, output_tokens: 3 }, + }); +} else if (argv.includes("--session-id")) { + result({ + session_id: at("--session-id"), + result: "Found an off-by-one.", + usage, + modelUsage: { + "claude-small": { inputTokens: 1, outputTokens: 1, cacheReadInputTokens: 0, cacheCreationInputTokens: 0 }, + "claude-big": { inputTokens: 10, outputTokens: 50, cacheReadInputTokens: 0, cacheCreationInputTokens: 90 }, + }, + }); +} else { + ${REPLY} + result({ + session_id: at("--resume"), + result: reply, + structured_output: process.env.FAKE_REPLY ? undefined : JSON.parse(reply), + usage: { ...usage, input_tokens: 5, cache_read_input_tokens: 80, cache_creation_input_tokens: 15, output_tokens: 7 }, + modelUsage: { "claude-big": { inputTokens: 15, outputTokens: 57 } }, + }); +} +`, + codex: ` +const fs = require("node:fs");${LOG} +const argv = process.argv.slice(2); +const stdin = fs.readFileSync(0, "utf8"); +const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|CODEX|ANTHROPIC|FAKE_HOST|PATH$)/.test(k))); +logged({ argv, stdin, env, cwd: process.cwd() }); +if (argv[0] === "login") process.exit(process.env.FAKE_LOGGED_OUT && !process.env.CODEX_ACCESS_TOKEN ? 1 : 0); +if (process.env.FAKE_HANG) { + // It takes a while to exit when asked to, as a CLI saving its session does. + process.on("SIGTERM", () => setTimeout(() => process.exit(143), 300)); + fs.writeFileSync(process.env.FAKE_HANG, String(process.pid)); + setInterval(() => {}, 1000); + return; +} +const event = (value) => console.log(JSON.stringify(value)); +const loggedIn = !process.env.FAKE_LOGGED_OUT || process.env.CODEX_API_KEY || process.env.CODEX_ACCESS_TOKEN; +const rejected = process.env.FAKE_REJECTED ?? (loggedIn ? undefined : "turn"); +if (rejected === "turn") { + const message = "unexpected status 401 Unauthorized: Missing bearer or basic authentication in header, url: https://api.openai.com/v1/responses"; + event({ type: "thread.started", thread_id: "thread-1" }); + event({ type: "error", message: "Reconnecting... 1/5 (" + message + ")" }); + event({ type: "turn.failed", error: { message } }); + process.exit(1); +} +if (rejected) { + console.error("Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again."); + process.exit(1); +} +if (process.env.FAKE_LIMITED) { + const message = process.env.FAKE_LIMITED === "retried" + ? "exceeded retry limit, last status: 429 Too Many Requests" + : process.env.FAKE_LIMITED === "unknown" + ? "You've hit your usage limit." + : "You've hit your usage limit. Visit https://chatgpt.com/settings/usage to purchase more credits or try again at Nov 5th, 2026 3:15 PM."; + event({ type: "thread.started", thread_id: "thread-1" }); + event({ type: "error", message }); + event({ type: "turn.failed", error: { message } }); + process.exit(1); +} +const message = (text) => event({ type: "item.completed", item: { type: "agent_message", text } }); +const zero = { input_tokens: 0, cached_input_tokens: 0, output_tokens: 0 }; +if (argv[1] === "review") { + event({ type: "thread.started", thread_id: "thread-1" }); + message("Found an off-by-one."); + event({ type: "turn.completed", usage: zero }); +} else if (argv[1] !== "resume") { + event({ type: "thread.started", thread_id: "thread-classify" }); + const failing = (process.env.FAKE_CLASSIFY ?? "").split(","); + if (failing.includes("codex-rejected")) { + event({ type: "turn.failed", error: { message: "unexpected status 401 Unauthorized: Missing bearer or basic authentication in header" } }); + process.exit(1); + } + if (failing.includes("codex-limited")) { + const message = "You've hit your usage limit. Visit https://chatgpt.com/settings/usage to purchase more credits or try again at Nov 5th, 2026 3:15 PM."; + event({ type: "turn.failed", error: { message } }); + process.exit(1); + } + message(failing.includes("codex-unusable") ? "not JSON" : ${RISK_REPLY}); + event({ type: "turn.completed", usage: { input_tokens: 40, cached_input_tokens: 0, output_tokens: 3 } }); +} else { + event({ type: "thread.started", thread_id: argv[argv.length - 2] }); + message("Checking the findings."); + ${REPLY} + message(reply); + event({ type: "turn.completed", usage: { input_tokens: 100, cached_input_tokens: 80, output_tokens: 7 } }); +} +`, +}; + +export const git = (dir: string, ...args: string[]) => + execFileSync("git", ["-C", dir, "-c", "user.name=Fixture", "-c", "user.email=fixture@localhost", ...args], { + encoding: "utf8", + }).trim(); + +export function write(dir: string, path: string, text: string) { + mkdirSync(join(dir, path, ".."), { recursive: true }); + writeFileSync(join(dir, path), text); +} + +export interface Setup { + repo: string; + bin: string; + /** Every logged invocation of the fake Codex and Claude Code. */ + calls(): Call[]; +} + +export interface Call { + argv: string[]; + stdin: string; + env: Record; + cwd: string; +} + +/** Credentials and routes the running session may hold, cleared so each test sets its own. */ +const HOST_CREDENTIALS = [ + "ANTHROPIC_API_KEY", + "ANTHROPIC_AUTH_TOKEN", + "ANTHROPIC_AWS_API_KEY", + "ANTHROPIC_BASE_URL", + "ANTHROPIC_CUSTOM_HEADERS", + "ANTHROPIC_FOUNDRY_API_KEY", + "ANTHROPIC_FOUNDRY_AUTH_TOKEN", + "CLAUDE_CODE_OAUTH_TOKEN", + ...["BEDROCK", "VERTEX", "FOUNDRY", "MANTLE", "ANTHROPIC_AWS", "ANTHROPIC_GOOGLE_CLOUD"].map( + (provider) => `CLAUDE_CODE_USE_${provider}`, + ), + "CODEX_API_KEY", + "CODEX_ACCESS_TOKEN", + "CODEX_CA_CERTIFICATE", +]; + +/** + * A clone whose origin's main holds instructions and review rules, checked out + * on a branch that edits its own AGENTS.md and a file in `src/`. + */ +export function setup(host: "CLAUDECODE" | "CODEX_THREAD_ID" | null = "CLAUDECODE"): Setup { + const dir = tempDir(); + const bin = join(dir, "bin"); + mkdirSync(bin); + for (const [name, body] of Object.entries(FAKES)) { + writeFileSync(join(bin, name), `#!${process.execPath}\n${body}`); + chmodSync(join(bin, name), 0o755); + } + const origin = join(dir, "origin.git"); + execFileSync("git", ["init", "-q", "--bare", "-b", "main", origin]); + const repo = join(dir, "repo"); + execFileSync("git", ["init", "-q", "-b", "main", repo]); + write(repo, "AGENTS.md", "BASE-ROOT rule\n"); + write(repo, "src/AGENTS.md", "BASE-SRC rule\n"); + write(repo, "docs/AGENTS.md", "BASE-DOCS rule\n"); + write(repo, "REVIEW.md", "BASE-REVIEW rule\n"); + write(repo, "src/lib.ts", "export const x = 1;\n"); + write(repo, ".gitignore", ".tmp/\n"); + git(repo, "add", "."); + git(repo, "commit", "-qm", "base"); + git(repo, "remote", "add", "origin", origin); + git(repo, "push", "-q", "origin", "main"); + git(repo, "checkout", "-qb", "feature"); + write(repo, "AGENTS.md", "HEAD-ROOT rule\n"); + write(repo, "src/lib.ts", "export const x = 2;\n"); + git(repo, "commit", "-qam", "change"); + + const log = join(dir, "calls"); + mkdirSync(log); + vi.stubEnv("PATH", `${bin}:${process.env.PATH}`); + vi.stubEnv("FAKE_LOG", log); + // The session's own profile would change the logged-out errors. + const unset = [ + ...HOST_CREDENTIALS, + "CLAUDECODE", + "CODEX_THREAD_ID", + "CLAUDE_CONFIG_DIR", + "CODEX_HOME", + "OPENAI_BASE_URL", + ]; + for (const name of [ + ...unset, + "FAKE_LOGGED_OUT", + "FAKE_REJECTED", + "FAKE_LIMITED", + "FAKE_HANG", + "FAKE_DISPOSITION", + "FAKE_RISK", + "FAKE_RAISE", + ]) { + vi.stubEnv(name, undefined); + } + if (host === "CLAUDECODE") vi.stubEnv("CLAUDECODE", "1"); + if (host === "CODEX_THREAD_ID") vi.stubEnv("CODEX_THREAD_ID", "parent"); + // Only whole calls: a fake's unfinished write stays a .tmp file. + const read = (path: string) => + readdirSync(path) + .filter((name) => name.endsWith(".json")) + .sort() + .map((name) => JSON.parse(readFileSync(join(path, name), "utf8"))); + return { repo, bin, calls: () => read(log) }; +} + +/** The effort a Codex or Claude Code turn runs at, or null for a status check. */ +export function effortOf(call: Call): string | null { + const flag = call.argv.indexOf("--effort"); + if (flag >= 0) return call.argv[flag + 1]!; + const setting = call.argv.find((arg) => arg.startsWith("model_reasoning_effort=")); + return setting ? JSON.parse(setting.slice("model_reasoning_effort=".length)) : null; +} + +/** The turns at `effort`, so the size's pick and its shadow, which run at once on the same CLI, read apart. */ +export const at = (calls: Call[], effort: string) => calls.filter((call) => effortOf(call) === effort); + +export async function quest(...args: string[]): Promise<{ code: number; stdout: string; stderr: string }> { + let stdout = ""; + let stderr = ""; + const out: Output = { + stdout: (text) => { + stdout += text; + }, + stderr: (text) => { + stderr += text; + }, + }; + const code = await main(args, out); + return { code, stdout, stderr }; +} diff --git a/test/review.test.ts b/test/review.test.ts index 980ebbe6..b228e61a 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -1,9 +1,8 @@ import { execFileSync } from "node:child_process"; -import { chmodSync, existsSync, mkdirSync, readFileSync, readdirSync, symlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, readFileSync, symlinkSync, writeFileSync } from "node:fs"; import { basename, dirname, join } from "node:path"; import { afterEach, expect, test, vi } from "vitest"; -import { type Output, main } from "../src/cli/main"; import { REVIEWERS } from "../src/cli/models"; import { SIZES, recordedRisk } from "../src/core"; import { @@ -30,357 +29,24 @@ import { import { withReview } from "../src/cli/review-record"; import { implementer, recorded } from "../src/cli/models-record"; import { tempDir } from "./fixture"; +import { + CLAUDE_LOGGED_OUT, + CODEX_LOGGED_OUT, + FINDING, + type Call, + at, + effortOf, + fakes, + git, + quest, + setup, + write, +} from "./review-fakes"; afterEach(() => { vi.unstubAllEnvs(); }); -/** The fake CLIs are shebang scripts found on a POSIX PATH, which Windows cannot run. */ -const fakes = test.skipIf(process.platform === "win32"); - -const FINDING = { file: "src/lib.ts", line: 2, severity: "high", title: "Off by one", body: "average([3]) is 0." }; - -const CODEX_LOGGED_OUT = "codex is not logged in to its default profile (CODEX_HOME is unset); run `codex login`"; -const CLAUDE_LOGGED_OUT = - "claude is not logged in to its default profile (CLAUDE_CONFIG_DIR is unset); run `claude auth login`"; - -/** - * The findings turn's reply, `reply`: FAKE_REPLY, or one finding. A JSON reply without `earlier` settles - * each earlier finding the prompt numbers as FAKE_DISPOSITION says, `fixed` by default, and one without `risk` - * reports FAKE_RAISE's tier, else the tier the prompt says the change was classified. FAKE_RAISE as - * `@` raises only in turns at that effort. - */ -const REPLY = ` -const listed = /\\n([\\s\\S]*?)\\n<\\/earlier-findings>/.exec(stdin)?.[1] ?? ""; -const earlier = [...listed.matchAll(/^(\\d+)\\. /gm)].map((m) => ({ - finding: Number(m[1]), - disposition: process.env.FAKE_DISPOSITION ?? "fixed", - reason: "Checked against the head.", -})); -const classified = /was classified (low|medium|high) risk/.exec(stdin)?.[1] ?? "medium"; -const [raise, raiseAt] = (process.env.FAKE_RAISE ?? "").split("@"); -const effort = argv.includes("--effort") - ? argv[argv.indexOf("--effort") + 1] - : JSON.parse((argv.find((arg) => arg.startsWith("model_reasoning_effort=")) ?? "=null").split("=")[1]); -const risk = raise && (!raiseAt || raiseAt === effort) - ? { tier: raise, reason: "It rewrites the wire format peers read." } - : { tier: classified, reason: "As classified." }; -let reply = process.env.FAKE_REPLY ?? JSON.stringify({ score: 8, rationale: "Lands once the off-by-one is fixed.", findings: [${JSON.stringify(FINDING)}] }); -try { - const parsed = JSON.parse(reply); - if (parsed && typeof parsed === "object" && !("earlier" in parsed)) reply = JSON.stringify({ ...parsed, earlier }); - if (parsed && typeof parsed === "object" && !("risk" in parsed)) reply = JSON.stringify({ ...JSON.parse(reply), risk }); -} catch {} -`; - -/** The classifier's reply: FAKE_RISK, or medium. */ -const RISK_REPLY = `JSON.stringify(process.env.FAKE_RISK ? JSON.parse(process.env.FAKE_RISK) : { tier: "medium", reason: "It changes what the library returns." })`; - -/** - * Records one call as its own file in FAKE_LOG, a directory, renamed into place - * once whole, named by its microsecond start so the files sort in call order. A - * fake the review aborts can die mid-write, and a partial line in one shared - * log would break the reader. - */ -const LOG = ` -const logged = (call) => { - const name = Math.round((performance.timeOrigin + performance.now()) * 1000).toString() + "-" + process.pid; - const file = require("node:path").join(process.env.FAKE_LOG, name); - fs.writeFileSync(file + ".tmp", JSON.stringify(call)); - fs.renameSync(file + ".tmp", file + ".json"); -}; -`; - -/** - * Fake reviewer CLIs. Each logs its argv, stdin, and the variables it was - * given, then answers as the FAKE_* variables say. FAKE_LOGGED_OUT removes the - * stored login, so only a credential variable the real CLI reads, or Claude - * Code's switch to a cloud provider, authenticates; like the real ones, - * `claude auth status` counts such a variable, and `codex login status` counts - * CODEX_ACCESS_TOKEN but not CODEX_API_KEY. FAKE_REJECTED has the API - * reject the login in each turn with the real CLI's error, in the style it - * names. FAKE_LIMITED fails a turn on the account's usage limit with the real - * CLI's error, naming its reset unless set to `unknown`. FAKE_CLASSIFY fails - * the classifier's turn of each `-rejected` or `-limited` it - * lists, the same way. FAKE_REPLY replaces the findings reply. - */ -const FAKES: Record = { - claude: ` -const fs = require("node:fs");${LOG} -const argv = process.argv.slice(2); -const stdin = fs.readFileSync(0, "utf8"); -const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|CODEX|ANTHROPIC|FAKE_HOST|PATH$)/.test(k))); -logged({ argv, stdin, env, cwd: process.cwd() }); -const credential = ["ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"].some((k) => process.env[k]); -const provider = Object.entries(process.env).some(([k, v]) => k.startsWith("CLAUDE_CODE_USE_") && v === "1"); -const loggedIn = !process.env.FAKE_LOGGED_OUT || credential || provider; -if (argv[0] === "auth") process.exit(loggedIn ? 0 : 1); -const at = (flag) => argv[argv.indexOf(flag) + 1]; -const result = (fields) => console.log(JSON.stringify({ type: "result", is_error: false, ...fields })); -const rejected = process.env.FAKE_REJECTED ?? (loggedIn ? undefined : "status"); -if (rejected) { - const session_id = at("--session-id") ?? at("--resume"); - if (rejected === "status") { - result({ session_id, is_error: true, api_error_status: 401, result: "Failed to authenticate. API Error: 401 Invalid bearer token" }); - } else if (rejected === "stderr") { - console.error("Login expired · Please run /login"); - } else { - result({ session_id, is_error: true, result: "OAuth token revoked · Please run /login" }); - } - process.exit(1); -} -// The limit hits the findings turn, after the review turn passed. -if (process.env.FAKE_LIMITED && argv.includes("--resume")) { - if (process.env.FAKE_LIMITED !== "unknown") { - console.log(JSON.stringify({ type: "rate_limit_event", rate_limit_info: { status: "rejected", resetsAt: 1900000000 } })); - } - result({ session_id: at("--resume"), is_error: true, result: "You've hit your weekly limit · resets Nov 5, 3pm (UTC)" }); - process.exit(1); -} -const usage = { input_tokens: 0, cache_read_input_tokens: 0, cache_creation_input_tokens: 0, output_tokens: 0 }; -const failing = (process.env.FAKE_CLASSIFY ?? "").split(","); -if (argv.includes("--session-id") && argv.includes("--json-schema")) { - if (failing.includes("claude-rejected")) { - result({ session_id: at("--session-id"), is_error: true, api_error_status: 401, result: "Failed to authenticate. API Error: 401 Invalid bearer token" }); - process.exit(1); - } - if (failing.includes("claude-limited")) { - console.log(JSON.stringify({ type: "rate_limit_event", rate_limit_info: { status: "rejected", resetsAt: 1900000000 } })); - result({ session_id: at("--session-id"), is_error: true, result: "You've hit your weekly limit · resets Nov 5, 3pm (UTC)" }); - process.exit(1); - } - const reply = failing.includes("claude-unusable") ? JSON.stringify({ tier: "huge", reason: "x" }) : ${RISK_REPLY}; - result({ - session_id: at("--session-id"), - result: reply, - structured_output: JSON.parse(reply), - usage: { ...usage, input_tokens: 40, output_tokens: 3 }, - }); -} else if (argv.includes("--session-id")) { - result({ - session_id: at("--session-id"), - result: "Found an off-by-one.", - usage, - modelUsage: { - "claude-small": { inputTokens: 1, outputTokens: 1, cacheReadInputTokens: 0, cacheCreationInputTokens: 0 }, - "claude-big": { inputTokens: 10, outputTokens: 50, cacheReadInputTokens: 0, cacheCreationInputTokens: 90 }, - }, - }); -} else { - ${REPLY} - result({ - session_id: at("--resume"), - result: reply, - structured_output: process.env.FAKE_REPLY ? undefined : JSON.parse(reply), - usage: { ...usage, input_tokens: 5, cache_read_input_tokens: 80, cache_creation_input_tokens: 15, output_tokens: 7 }, - modelUsage: { "claude-big": { inputTokens: 15, outputTokens: 57 } }, - }); -} -`, - codex: ` -const fs = require("node:fs");${LOG} -const argv = process.argv.slice(2); -const stdin = fs.readFileSync(0, "utf8"); -const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|CODEX|ANTHROPIC|FAKE_HOST|PATH$)/.test(k))); -logged({ argv, stdin, env, cwd: process.cwd() }); -if (argv[0] === "login") process.exit(process.env.FAKE_LOGGED_OUT && !process.env.CODEX_ACCESS_TOKEN ? 1 : 0); -if (process.env.FAKE_HANG) { - // It takes a while to exit when asked to, as a CLI saving its session does. - process.on("SIGTERM", () => setTimeout(() => process.exit(143), 300)); - fs.writeFileSync(process.env.FAKE_HANG, String(process.pid)); - setInterval(() => {}, 1000); - return; -} -const event = (value) => console.log(JSON.stringify(value)); -const loggedIn = !process.env.FAKE_LOGGED_OUT || process.env.CODEX_API_KEY || process.env.CODEX_ACCESS_TOKEN; -const rejected = process.env.FAKE_REJECTED ?? (loggedIn ? undefined : "turn"); -if (rejected === "turn") { - const message = "unexpected status 401 Unauthorized: Missing bearer or basic authentication in header, url: https://api.openai.com/v1/responses"; - event({ type: "thread.started", thread_id: "thread-1" }); - event({ type: "error", message: "Reconnecting... 1/5 (" + message + ")" }); - event({ type: "turn.failed", error: { message } }); - process.exit(1); -} -if (rejected) { - console.error("Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again."); - process.exit(1); -} -if (process.env.FAKE_LIMITED) { - const message = process.env.FAKE_LIMITED === "retried" - ? "exceeded retry limit, last status: 429 Too Many Requests" - : process.env.FAKE_LIMITED === "unknown" - ? "You've hit your usage limit." - : "You've hit your usage limit. Visit https://chatgpt.com/settings/usage to purchase more credits or try again at Nov 5th, 2026 3:15 PM."; - event({ type: "thread.started", thread_id: "thread-1" }); - event({ type: "error", message }); - event({ type: "turn.failed", error: { message } }); - process.exit(1); -} -const message = (text) => event({ type: "item.completed", item: { type: "agent_message", text } }); -const zero = { input_tokens: 0, cached_input_tokens: 0, output_tokens: 0 }; -if (argv[1] === "review") { - event({ type: "thread.started", thread_id: "thread-1" }); - message("Found an off-by-one."); - event({ type: "turn.completed", usage: zero }); -} else if (argv[1] !== "resume") { - event({ type: "thread.started", thread_id: "thread-classify" }); - const failing = (process.env.FAKE_CLASSIFY ?? "").split(","); - if (failing.includes("codex-rejected")) { - event({ type: "turn.failed", error: { message: "unexpected status 401 Unauthorized: Missing bearer or basic authentication in header" } }); - process.exit(1); - } - if (failing.includes("codex-limited")) { - const message = "You've hit your usage limit. Visit https://chatgpt.com/settings/usage to purchase more credits or try again at Nov 5th, 2026 3:15 PM."; - event({ type: "turn.failed", error: { message } }); - process.exit(1); - } - message(failing.includes("codex-unusable") ? "not JSON" : ${RISK_REPLY}); - event({ type: "turn.completed", usage: { input_tokens: 40, cached_input_tokens: 0, output_tokens: 3 } }); -} else { - event({ type: "thread.started", thread_id: argv[argv.length - 2] }); - message("Checking the findings."); - ${REPLY} - message(reply); - event({ type: "turn.completed", usage: { input_tokens: 100, cached_input_tokens: 80, output_tokens: 7 } }); -} -`, -}; - -const git = (dir: string, ...args: string[]) => - execFileSync("git", ["-C", dir, "-c", "user.name=Fixture", "-c", "user.email=fixture@localhost", ...args], { - encoding: "utf8", - }).trim(); - -function write(dir: string, path: string, text: string) { - mkdirSync(join(dir, path, ".."), { recursive: true }); - writeFileSync(join(dir, path), text); -} - -interface Setup { - repo: string; - bin: string; - /** Every logged invocation of the fake Codex and Claude Code. */ - calls(): Call[]; -} - -interface Call { - argv: string[]; - stdin: string; - env: Record; - cwd: string; -} - -/** Credentials and routes the running session may hold, cleared so each test sets its own. */ -const HOST_CREDENTIALS = [ - "ANTHROPIC_API_KEY", - "ANTHROPIC_AUTH_TOKEN", - "ANTHROPIC_AWS_API_KEY", - "ANTHROPIC_BASE_URL", - "ANTHROPIC_CUSTOM_HEADERS", - "ANTHROPIC_FOUNDRY_API_KEY", - "ANTHROPIC_FOUNDRY_AUTH_TOKEN", - "CLAUDE_CODE_OAUTH_TOKEN", - ...["BEDROCK", "VERTEX", "FOUNDRY", "MANTLE", "ANTHROPIC_AWS", "ANTHROPIC_GOOGLE_CLOUD"].map( - (provider) => `CLAUDE_CODE_USE_${provider}`, - ), - "CODEX_API_KEY", - "CODEX_ACCESS_TOKEN", - "CODEX_CA_CERTIFICATE", -]; - -/** - * A clone whose origin's main holds instructions and review rules, checked out - * on a branch that edits its own AGENTS.md and a file in `src/`. - */ -function setup(host: "CLAUDECODE" | "CODEX_THREAD_ID" | null = "CLAUDECODE"): Setup { - const dir = tempDir(); - const bin = join(dir, "bin"); - mkdirSync(bin); - for (const [name, body] of Object.entries(FAKES)) { - writeFileSync(join(bin, name), `#!${process.execPath}\n${body}`); - chmodSync(join(bin, name), 0o755); - } - const origin = join(dir, "origin.git"); - execFileSync("git", ["init", "-q", "--bare", "-b", "main", origin]); - const repo = join(dir, "repo"); - execFileSync("git", ["init", "-q", "-b", "main", repo]); - write(repo, "AGENTS.md", "BASE-ROOT rule\n"); - write(repo, "src/AGENTS.md", "BASE-SRC rule\n"); - write(repo, "docs/AGENTS.md", "BASE-DOCS rule\n"); - write(repo, "REVIEW.md", "BASE-REVIEW rule\n"); - write(repo, "src/lib.ts", "export const x = 1;\n"); - write(repo, ".gitignore", ".tmp/\n"); - git(repo, "add", "."); - git(repo, "commit", "-qm", "base"); - git(repo, "remote", "add", "origin", origin); - git(repo, "push", "-q", "origin", "main"); - git(repo, "checkout", "-qb", "feature"); - write(repo, "AGENTS.md", "HEAD-ROOT rule\n"); - write(repo, "src/lib.ts", "export const x = 2;\n"); - git(repo, "commit", "-qam", "change"); - - const log = join(dir, "calls"); - mkdirSync(log); - vi.stubEnv("PATH", `${bin}:${process.env.PATH}`); - vi.stubEnv("FAKE_LOG", log); - // The session's own profile would change the logged-out errors. - const unset = [ - ...HOST_CREDENTIALS, - "CLAUDECODE", - "CODEX_THREAD_ID", - "CLAUDE_CONFIG_DIR", - "CODEX_HOME", - "OPENAI_BASE_URL", - ]; - for (const name of [ - ...unset, - "FAKE_LOGGED_OUT", - "FAKE_REJECTED", - "FAKE_LIMITED", - "FAKE_HANG", - "FAKE_DISPOSITION", - "FAKE_RISK", - "FAKE_RAISE", - ]) { - vi.stubEnv(name, undefined); - } - if (host === "CLAUDECODE") vi.stubEnv("CLAUDECODE", "1"); - if (host === "CODEX_THREAD_ID") vi.stubEnv("CODEX_THREAD_ID", "parent"); - // Only whole calls: a fake's unfinished write stays a .tmp file. - const read = (path: string) => - readdirSync(path) - .filter((name) => name.endsWith(".json")) - .sort() - .map((name) => JSON.parse(readFileSync(join(path, name), "utf8"))); - return { repo, bin, calls: () => read(log) }; -} - -/** The effort a Codex or Claude Code turn runs at, or null for a status check. */ -function effortOf(call: Call): string | null { - const flag = call.argv.indexOf("--effort"); - if (flag >= 0) return call.argv[flag + 1]!; - const setting = call.argv.find((arg) => arg.startsWith("model_reasoning_effort=")); - return setting ? JSON.parse(setting.slice("model_reasoning_effort=".length)) : null; -} - -/** The turns at `effort`, so the size's pick and its shadow, which run at once on the same CLI, read apart. */ -const at = (calls: Call[], effort: string) => calls.filter((call) => effortOf(call) === effort); - -async function quest(...args: string[]): Promise<{ code: number; stdout: string; stderr: string }> { - let stdout = ""; - let stderr = ""; - const out: Output = { - stdout: (text) => { - stdout += text; - }, - stderr: (text) => { - stderr += text; - }, - }; - const code = await main(args, out); - return { code, stdout, stderr }; -} - test("the reviewer is the harness this command is not running under", () => { expect(runningHarness({ CLAUDECODE: "1" })).toBe("claude"); expect(runningHarness({ CODEX_THREAD_ID: "t" })).toBe("codex");