diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0703b1c2..ed73a8d4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,6 +39,7 @@ need no review. PRs land on `main` through GitHub's merge queue, one PR at a time, as squash commits. Queue the reviewed head with `quest merge --land ` (`/quest-merge` does this). +The `Pullfrog approval` ruleset requires Pullfrog's approval of the head, which the queue waits for; other online reviewers are never waited for. The required `Queue` check skips on pull requests; the merge group runs `just check` and `just test` on the self-hosted runner. That build lives in `.github/workflows/queue-build.yml`, which the merge group calls from `main`, so a PR's edit to it runs only after the PR lands. The host runs one `quest land watch` daemon per base, here `main`'s; it runs the Codex fixer when the queue drops a PR for a failed build or a conflict, and re-enqueues healthy heads at the back of the queue. diff --git a/README.md b/README.md index 635050a9..1028a651 100644 --- a/README.md +++ b/README.md @@ -580,7 +580,11 @@ It also refuses a head that fails `quest check --no-stubs --base --branch `, which it runs on git objects it fetches, so nothing from the head runs, and ignores findings the base already has; run a `quest` the PR cannot modify, since the head's own build could skip the check. A base without -quests skips it. It enables auto-merge bound to that SHA, or merges at once when nothing is +quests skips it. While the base requires an approval the PR lacks, such as an +online reviewer's that a ruleset requires (GitHub's review decision is +`review_required` or `changes_requested`), it only arms auto-merge, which waits +for the approval, and refuses where nothing would wait, since an admin's token +would merge or enqueue past the requirement. It enables auto-merge bound to that SHA, or merges at once when nothing is left to wait for. `--squash` or `--rebase` picks the method, and it refuses one the repository disallows; otherwise it uses the only method the repository allows, or else a merge commit, and refuses when merge commits are off and more @@ -934,7 +938,8 @@ ready ([community#209348](https://github.com/orgs/community/discussions/209348)) as for a pick `quest merge --land` armed while required checks ran. Each pass, the watcher enqueues every such PR: open, armed, ready (its merge state is `CLEAN`, `HAS_HOOKS`, or `UNSTABLE`: its required checks passed and it does not -conflict), not in the queue, and not added to it since it was armed. It +conflict), not lacking an approval its base requires, which auto-merge waits +for, not in the queue, and not added to it since it was armed. It enqueues only the head auto-merge was armed on, with `expectedHeadOid` bound to it: GitHub keeps auto-merge across a push, but a push by anyone but the land fixer ends the pick, and a head it cannot tell yet waits for the next pass. An @@ -1391,14 +1396,11 @@ size. Each size ranks its reviewers, the same for every repository: | L | Codex `gpt-6.1-sol` xhigh | Claude Code `claude-opus-5-5` high | | XL | Codex `gpt-6.1-sol` xhigh | Claude Code `claude-opus-5-5` xhigh | -Beside it, opencode `opencode-go/deepseek-v4.1-flash` at `high` reviews the -same head for every size, in its own session, so each head is read by a model -family no writer runs, at about $0.05 a review. The shadow, the pick's model -one effort lower (Sol low for an XS PR), reviews it too, in its own session, so -the routing can be retuned from what each effort step finds over the one below -it. It is skipped when the pick runs at low or at an `--effort` off the ladder -(low, medium, high, xhigh, max). It runs on the branch's first run of the -pair, then reruns only while it has findings to settle, or once the pick +Beside it, the shadow, the pick's model one effort lower (Sol low for an XS +PR), reviews the same head in its own session, so the routing can be retuned +from what each effort step finds over the one below it. It is skipped when the +pick runs at low or at an `--effort` off the ladder (low, medium, high, xhigh, +max). It runs on the branch's first run of the pick, then reruns only while it has findings to settle, or once the pick changed so that it is no longer one step below; its findings are numbered, answered, and settled like any reviewer's. A pick with no step below it seats a shadow at its own effort while the old shadow still has findings, so none @@ -1417,11 +1419,10 @@ Without a record, or when its last line names an `unknown` model, the harness running the command, usually the session that just pushed, is taken to be the latest writer, so Codex reviews from Claude Code and Claude Code from Codex. `--harness` names the only reviewer instead of -the pair and the shadow, at its entry's model and effort, and `--model` and `--effort` +the pick and the shadow, at its entry's model and effort, and `--model` and `--effort` override the size pick's or that reviewer's, as a repository whose `AGENTS.md` -names other reviewers needs. The second reviewer's model is avoided like any -other, which, without `--harness`, is an error. A `--harness` run keeps the other -reviewers' sessions for the pair's next run, and refuses while one has +names other reviewers needs. A `--harness` run keeps the other +reviewers' sessions for the pick's next run, and refuses while one has findings to settle, so a head never passes one reviewer alone while the other's findings are open. When the latest writer changes the size's pick to one with no session on the branch, its first session settles the open findings of the @@ -1443,16 +1444,12 @@ gateway or provider is set. Variables that would change the reviewer's plugins, permissions, model, or configuration stay stripped. A logged-out error names the profile the CLI checked, `CLAUDE_CONFIG_DIR` or `CODEX_HOME`, or the default when unset, and any gateway or provider. -`--harness opencode` runs opencode alone, with `--model` as `provider/model` -and `--effort` as the model's variant, since no size lists it. It keeps the user's own opencode config, such as custom providers, and login, -and counts as logged out when `opencode models` does not list the model. ```sh quest review # round 1 against origin's default branch quest review --responses .tmp/responses.md # round 2: the new head, with fixes and refutations quest review --base origin/release --new # a new session against another base quest review --size S --exclude-model gpt-6.1-sol # an S PR that Sol wrote, with no record in its body quest review --branch quest/m1/foo --responses .tmp/responses.md # a detached checkout of that branch's head -quest review --harness opencode --model opencode-go/deepseek-v4-pro --effort high # opencode alone, on another model ``` Every turn reviews only the branch's own change, from the merge base of `HEAD` @@ -1460,10 +1457,9 @@ with the base's tip, so a branch behind its base draws no findings on code the base gained since, which a diff against its tip shows as reverted. The first round explores the change once. It runs the reviewer's own review, `codex exec review` with custom instructions or Claude Code's `/code-review - ...HEAD` with text after the target, or for opencode, which -has no review mode, a prompt asking for one, given the range, the + ...HEAD` with text after the target, given the range, the base's `REVIEW.md`, if any, and any earlier findings and responses up front. -None returns a JSON schema itself, so the round resumes that session for one +Neither returns a JSON schema itself, so the round resumes that session for one turn that reads nothing and reports the review in one schema, with a score from 0 to 10 for how ready the head is to land and one sentence on why: 10 lands as is, 7 to 9 lands after the findings are fixed, 4 to 6 needs rework, and 0 to 3 @@ -1489,8 +1485,7 @@ reviewer settles only its own findings, and gets `--responses` only when it has some, so on the same head only a reviewer with findings runs. Resuming keeps the reviewer's prompt cache; Claude Code is asked for its 1-hour cache, and Codex keeps its default. Each reviewer's session is kept per branch in the clone's -common Git directory, so any worktree of the clone resumes it, except opencode's, -which stays in the checkout it started in, so another worktree starts a new one; +common Git directory, so any worktree of the clone resumes it; `--new` starts new sessions, but continues each reviewer's tally of its findings on the branch, less those it drops unsettled, unless the saved state is one this quest cannot read. `--branch` names the branch instead of the checked-out one, which a @@ -1506,28 +1501,16 @@ commands. Codex leaves out its skills catalog; it still loads the user's split the login, so the reviewer is told to disregard it. Claude Code loads no settings files, so no allow rule, even the user's own, widens that. Codex skips the user's config, whose project trust would load the checkout's `.codex/config.toml` and start its -MCP servers outside the sandbox. opencode loads no project config, which would -start the checkout's plugins and MCP servers and name its shell, nor Claude -Code's files, and has every tool but its shell denied, after the user's own -rules so none outranks the denial. Its shell runs each command under -sandbox-runtime (`srt` on `PATH`, which this repository's dev shell provides), -with no network, reading only the checkout and its Git directories, and writing -only a private directory; on Linux, `srt` must also refuse Unix sockets, which -it does only with its seccomp helper, so nixpkgs' plain package is refused. -Whether those flags isolate is up to each +MCP servers outside the sandbox. Whether those flags isolate is up to each CLI's release, so `just review-sandbox`, in this repository, plants project MCP -servers and hooks for Codex and Claude Code on a head, for Claude Code local settings, -an allow rule, an `env` shell prefix, an API key helper, and a status line, and -for opencode an MCP server, plugins, and a shell. It -runs `quest review` with the installed CLIs and copies of your logins -(opencode's own, which it shares with every session), and -fails if any of it runs, the review trusts a new directory, the reviewer's -flags let the planted rule allow a command, or opencode's shell writes, reads -your home, reaches the network, or goes unused. `QUEST_LIVE_OPENCODE_MODEL` -names opencode's model, as `provider/model#variant`. A login with no file to copy, such +servers and hooks for both CLIs on a head, and for Claude Code local settings, +an allow rule, an `env` shell prefix, an API key helper, and a status line. It +runs `quest review` with the installed CLIs and copies of your logins, and +fails if any of it runs, the review trusts a new directory, or the reviewer's +flags let the planted rule allow a command. A login with no file to copy, such as one in the macOS keychain, needs the CLI's credential exported instead: `CLAUDE_CODE_OAUTH_TOKEN` from `claude setup-token`, or `CODEX_API_KEY`. It spends real usage, so CI never runs it; run it -after upgrading any of the CLIs. The base is fetched first, since +after upgrading either CLI. The base is fetched first, since the sandbox has no network. The reviewer follows the base's `AGENTS.md` and `CLAUDE.md`, at the root and in each directory holding a changed path, never the checkout's, so a change cannot instruct its own reviewer. The base's `REVIEW.md` is optional, @@ -1614,8 +1597,7 @@ session must also be this repository's, as its Codex log, its checkout's remotes, or, once the checkout is deleted, a Codex log for that exact directory names (never a parent's, which a nested clone may not share); otherwise it stays unattributed. A session is reviewing when the PR's `## Review` record names it or Codex marks it a review, implementing when it began before the PR opened, -and iterating otherwise. An opencode session, whose log quest does not read, is -the reviewing session of the newest PR whose `## Review` record names it. Each PR shows its `## Models` size and implementers, and as `unread:` each Implementer line it cannot read; each reviewer the +and iterating otherwise. Each PR shows its `## Models` size and implementers, and as `unread:` each Implementer line it cannot read; each reviewer the `quest review` record names, with its latest round and score and its tally of findings across every round, marked `shadow` for the shadow, where a merged PR whose last round left only low findings open counts them settled, since `/quest-review` fixes or refutes those diff --git a/assets/skills/review.md b/assets/skills/review.md index f3188bc7..4e082e04 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -23,7 +23,7 @@ Run no new round when a review already counts for the committed head, as `/quest If you judge the change too trivial to review, skip the review instead and record the skip and its reason in `## Review`. Otherwise, review the committed branch with `quest review --base /`, in the foreground; a round takes minutes. -It picks a reviewer other than the model that changed the branch last, at an effort the PR's size sets, with a second reviewer beside it. +It picks a reviewer other than the model that changed the branch last, at an effort the PR's size sets. It reads that model, and off a quest branch the size, from the PR body's `## Models` record: open the draft PR and write the record before the first round, as `Size: M` and `Implementer: Claude Code, claude-opus-5-5, high effort`. Pass `--exclude-model` or `--size` only for a PR whose body cannot hold the record. Where the repository's root instructions name other reviewers, pass them with `--harness`, `--model`, and `--effort`, which runs that reviewer alone. @@ -41,7 +41,7 @@ Step back before another patch: state the problem the PR solves without referenc Report that comparison, recommending to continue, rethink the approach, merge as is, or drop the PR, and let the user decide. Record each design change, including a rethink the user picks, in a PR comment that starts with `Design change:`, and start the next review with `quest review --new`, which restarts the count. -Either reviewer out of quota or logged out is an outage, not a skip: stop reviewing rather than go on with the other alone, keep the PR a draft, hand it back, and report the outage. +A reviewer out of quota or logged out is an outage, not a skip: stop reviewing, keep the PR a draft, hand it back, and report the outage. Its `## Waiting` item names the reviewer, its error, and the check, such as `Codex: out of quota until 2026-11-03 15:00 UTC; check that quest review runs`, with the reset time `quest review` prints when it exits 3 on a usage limit; a logged-out reviewer waits on `Maintainer:`, with the login command `quest review` prints. Once the reset passes or the login is restored, the item has cleared: whoever finds it runs `/quest-review` on the PR. diff --git a/flake.nix b/flake.nix index b0e73c51..33bfa3d5 100644 --- a/flake.nix +++ b/flake.nix @@ -75,16 +75,10 @@ nixfmt # GNU time, which the gate's cancellation tests put between Bash and their leaf. time - # `srt`, which an opencode review runs its commands under, with its seccomp helper on Linux. - (callPackage ./nix/sandbox-runtime.nix { }) ] # `flock` (the land daemon's repository lock) and `script` (the tests' terminal), which - # self-hosted NixOS runners don't otherwise provide, and the bubblewrap and socat srt runs. - ++ pkgs.lib.optionals pkgs.stdenv.hostPlatform.isLinux [ - pkgs.util-linux - pkgs.bubblewrap - pkgs.socat - ]; + # self-hosted NixOS runners don't otherwise provide. + ++ pkgs.lib.optionals pkgs.stdenv.hostPlatform.isLinux [ pkgs.util-linux ]; }; formatter = pkgs.nixfmt; checks = { diff --git a/justfile b/justfile index 9e3c3bcc..b58a2b21 100644 --- a/justfile +++ b/justfile @@ -38,8 +38,8 @@ _test: bash scripts/lifecycle-check.sh "$PWD/dist/quest" # Check that `quest review` runs nothing a reviewed head plants, with the real -# Codex, Claude Code, and opencode CLIs. Uses each CLI's login and spends its usage; -# run it after upgrading any of them. Arguments go to vitest, such as `-t codex`. +# Codex and Claude Code CLIs. Uses each CLI's login and spends its usage; run it +# after upgrading either CLI. Arguments go to vitest, such as `-t codex`. review-sandbox *args: QUEST_LIVE=1 bun run vitest run test/live/review-sandbox {{args}} diff --git a/nix/sandbox-runtime.nix b/nix/sandbox-runtime.nix deleted file mode 100644 index 21bee57d..00000000 --- a/nix/sandbox-runtime.nix +++ /dev/null @@ -1,61 +0,0 @@ -# nixpkgs' sandbox-runtime (`srt`) with its seccomp helper, which nixpkgs leaves out. Without it -# srt runs commands with Unix sockets open, the Nix daemon's included. (User namespaces stay -# refused either way: with no helper, srt passes the bwrap 0.8+ on its PATH `--disable-userns`.) -# The helper is built from srt's own source by its own build script. -{ - lib, - stdenv, - sandbox-runtime, - bun, - glibc, - libseccomp, -}: -let - # srt looks for the helper under its Node architecture's name. - arch = - { - x86_64 = "x64"; - aarch64 = "arm64"; - } - .${stdenv.hostPlatform.parsed.cpu.name}; - applySeccomp = stdenv.mkDerivation { - pname = "sandbox-runtime-apply-seccomp"; - inherit (sandbox-runtime) version src; - nativeBuildInputs = [ bun ]; - # The build links its filter generator and the helper statically, as srt's releases do. - buildInputs = [ - glibc.static - (libseccomp.overrideAttrs { - dontDisableStatic = true; - doCheck = false; - }) - ]; - buildPhase = '' - runHook preBuild - bun vendor/seccomp/build.ts - runHook postBuild - ''; - installPhase = '' - runHook preInstall - install -Dm755 vendor/seccomp/${arch}/apply-seccomp $out/bin/apply-seccomp - runHook postInstall - ''; - meta = sandbox-runtime.meta // { - mainProgram = "apply-seccomp"; - platforms = [ - "x86_64-linux" - "aarch64-linux" - ]; - }; - }; -in -if !stdenv.hostPlatform.isLinux then - sandbox-runtime -else - sandbox-runtime.overrideAttrs (old: { - # Where srt finds it: vendor/seccomp// in its package root. - postInstall = (old.postInstall or "") + '' - install -Dm755 ${lib.getExe applySeccomp} \ - $out/lib/node_modules/@anthropic-ai/sandbox-runtime/vendor/seccomp/${arch}/apply-seccomp - ''; - }) diff --git a/quest/m0/pullfrog-setup.md b/quest/m0/pullfrog-setup.md index 9854bda4..ca8ab580 100644 --- a/quest/m0/pullfrog-setup.md +++ b/quest/m0/pullfrog-setup.md @@ -17,6 +17,10 @@ Pullfrog reviews with Claude since 2026-11-12, its model access configured in the Pullfrog console, and its workflow is back in moq-dev/quest (#659), kixel-ai/rind, moq-dev/rust2, moq-dev/studio, and moq-dev/web-transport after the 2026-11-10 removal [token-waste](/quest/m0/token-waste/README.md) records. +Since 2026-11-13 its approval, which an identical `Pullfrog approval` ruleset +requires in every moq-dev and kixel-ai repository (kept in kixel-ai/dotfiles' +`bin/repo-settings`), gates merges in place of the DeepSeek review, so what its +runs can check now decides merges. Evidence from the trimmed Pullfrog learnings: rind's bot could not fetch its test vector submodules because `.git` is read-only, so about 20 tests failed; diff --git a/quest/m0/routing-evidence/README.md b/quest/m0/routing-evidence/README.md index 5c5e181c..4bb9e80b 100644 --- a/quest/m0/routing-evidence/README.md +++ b/quest/m0/routing-evidence/README.md @@ -97,6 +97,10 @@ Baseline from the 2026-11-11 audit, at API list price: `[P1]`-`[P3]` findings in its review body. About 40% of those PRs merged while its latest review still listed findings. + The table predates Pullfrog's move to Claude (2026-11-12) and its review + replacing the DeepSeek one in `quest review` + ([token waste](/quest/m0/token-waste/README.md), 2026-11-13). + ## Waiting - moq-dev/quest: one week of PRs since both the shadow reviewer diff --git a/quest/m0/token-waste/README.md b/quest/m0/token-waste/README.md index 2b3a86b0..6146002c 100644 --- a/quest/m0/token-waste/README.md +++ b/quest/m0/token-waste/README.md @@ -41,6 +41,14 @@ Decision prompts, chosen options marked ✅: Changed 2026-11-12: Pullfrog reviews with Claude, configured in the Pullfrog console, and its workflow is back in moq-dev/quest, rind, rust2, studio, and web-transport. + Changed 2026-11-13 (maintainer), reversing + [moq-dev/quest#620](https://github.com/moq-dev/quest/pull/620), whose + cost reason Claude through the console removed, after opencode's quota ran + out and failed every `quest review` that day: Pullfrog replaces the + DeepSeek review, so `quest review` runs only the size's pick, and ✅ its + approval gates the merge (rejected: advisory only), ✅ through a GitHub + ruleset requiring an approval, which `quest merge` and `quest land` honor + (rejected: a `REVIEW.md` declaration that `quest merge` checks). - Rules to hooks or lint: first not planned, then ✅ planned the same day (user, adopting practices from the research below). - Adopted 2026-11-05 (user) from a survey of AI-heavy developers' setups diff --git a/quest/m1/model-check.md b/quest/m1/model-check.md index c264481c..614b92e9 100644 --- a/quest/m1/model-check.md +++ b/quest/m1/model-check.md @@ -142,14 +142,7 @@ checkpoints and outages to count by hand: plan a quest if a provider has none (coordinator, /quest-complete follow-ups, 2026-11-03). - Run the live sandbox test, `just review-sandbox`, with the CLIs installed - at the time, through `quest gate`, and plan a quest for any failure. It - covers all three reviewers: opencode since - [opencode reviewers](https://github.com/moq-dev/quest/pull/603), on the - model `QUEST_LIVE_OPENCODE_MODEL` names (default - `opencode-go/deepseek-v4.1-flash#high`), with the dev shell's `srt`. Check - that opencode's `run` still takes no schema (it took none in 2.2.6, so the - findings turn's prompt carries it), and that `OPENCODE_DISABLE_PROJECT_CONFIG` - still skips the checkout's config and `AGENTS.md`. + at the time, through `quest gate`, and plan a quest for any failure. - Check first: - [openai/codex#49333](https://github.com/openai/codex/issues/49333): if `--ignore-user-config` stops skipping the project layer, the Codex diff --git a/src/cli/hold.ts b/src/cli/hold.ts index 48d242f0..0c3fa2de 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -3,7 +3,7 @@ // that added the label is its hold token, since sessions often share an account. // `quest merge` takes no label: the pinned auto-merge or merge-queue entry it creates is the queue signal. import { ACTIVE_LABEL, formatFinding, handedBack, isHoldLabel, latestHold, heldToken, withoutHandBack } from "../core"; -import { cleanAutoMergeRefusal, immediateMerge, queueHead } from "../github/merge"; +import { cleanAutoMergeRefusal, immediateMerge, lacksApproval, queueHead } from "../github/merge"; import { repositoryPath } from "./claims"; import { array, @@ -29,7 +29,7 @@ export type Method = "merge" | "squash" | "rebase"; const READ = `query HoldOwnership($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { - id url state isDraft body updatedAt baseRefName baseRefOid headRefName headRefOid mergeStateStatus isMergeQueueEnabled isInMergeQueue + id url state isDraft body updatedAt baseRefName baseRefOid headRefName headRefOid mergeStateStatus reviewDecision isMergeQueueEnabled isInMergeQueue labels(first: 100) { nodes { name } } timelineItems(last: 100, itemTypes: [LABELED_EVENT]) { nodes { __typename ... on LabeledEvent { id createdAt label { name } } } @@ -59,6 +59,11 @@ interface Snapshot { branch: string; head: string; mergeState: string; + /** + * The base requires an approval the PR lacks, such as an online reviewer's that a ruleset requires. An admin's + * token may merge or enqueue past it, so only auto-merge, which waits for it, may pick such a head. + */ + unapproved: boolean; queue: boolean; /** Whether the PR is in its base's merge queue. */ queued: boolean; @@ -251,6 +256,8 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe if (fresh.base !== snapshot.base) { throw new Refusal(`${snapshot.url} was retargeted from ${snapshot.base} to ${fresh.base}; land it again`); } + // An approval dismissed or a change requested since the first read decides how it may land. + return { ...snapshot, unapproved: fresh.unapproved }; }; await passing(labels.github, pr, snapshot.url, sha); const findings = await request.check({ @@ -279,8 +286,7 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe if (snapshot.queue) { if (named !== undefined && named !== "merge") deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); - await unchanged(); - await pick(labels, snapshot, named ?? "merge"); + await pick(labels, await unchanged(), named ?? "merge"); return; } const repository = object(await (await labels.github.request(repositoryPath(pr))).json()); @@ -311,7 +317,11 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe `${snapshot.url} head ${snapshot.head} does not contain ${snapshot.base} at ${base}; merge the base and rerun the repository's checks before landing`, ); } - await unchanged(); + const { unapproved } = await unchanged(); + if (unapproved && repository.allow_auto_merge !== true) + throw new Refusal( + `${snapshot.url} lacks the approval its base requires, which a merge now would bypass; land it once it is approved`, + ); const submit = (mutation: "mergePullRequest" | "enablePullRequestAutoMerge") => labels.github.graphql( `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { @@ -320,7 +330,7 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, ); // Only --direct reaches here without auto-merge on; it merges at once rather than arm what GitHub refuses. - if (repository.allow_auto_merge !== true || immediateMerge(snapshot.mergeState, snapshot.queue)) { + if (!unapproved && (repository.allow_auto_merge !== true || immediateMerge(snapshot.mergeState, snapshot.queue))) { await submit("mergePullRequest"); return; } @@ -335,6 +345,7 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe if ( fresh.state !== "OPEN" || fresh.draft || + fresh.unapproved || fresh.base !== snapshot.base || !immediateMerge(fresh.mergeState, fresh.queue) || (await requirements(labels.github, pr, fresh.base)).checks.some((check) => check.name === LAND_CHECK) @@ -373,13 +384,13 @@ export const UNREFUSED = ["FORBIDDEN", "NOT_FOUND", "INSUFFICIENT_SCOPES", "INTE */ async function pick(labels: Labels, snapshot: Snapshot, method: Method): Promise { try { - await queueHead( - (query, variables) => labels.github.graphql(query, variables), - snapshot.id, - snapshot.head, - snapshot.mergeState, + await queueHead((query, variables) => labels.github.graphql(query, variables), { + id: snapshot.id, + head: snapshot.head, + mergeState: snapshot.mergeState, + unapproved: snapshot.unapproved, method, - ); + }); } catch (error) { if (!(error instanceof GitHubGraphQLError) || error.types.some((type) => UNREFUSED.includes(type ?? ""))) throw error; @@ -546,6 +557,7 @@ class Labels { branch: string(row.headRefName), head: string(row.headRefOid), mergeState: string(row.mergeStateStatus), + unapproved: lacksApproval(row.reviewDecision), queue: row.isMergeQueueEnabled === true, queued: row.isInMergeQueue === true, labels: array(object(row.labels).nodes) diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index d1ece125..baf66b21 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -4,7 +4,7 @@ import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSy import { basename, join } from "node:path"; import { isHoldLabel } from "../core"; import { array, GitHub, type GitHubDependencies, GitHubGraphQLError, object, type ObjectValue, string } from "./github"; -import { type Queued, queueHead } from "../github/merge"; +import { type Queued, lacksApproval, queueHead } from "../github/merge"; import { UNREFUSED } from "./hold"; import { baseRemote, enqueue, type Entry, type LandMethod, mergeBase, type RemoteRead } from "./land"; import { resolutionSection, type Resolution } from "./land-apply"; @@ -123,7 +123,7 @@ const JUDGED: Record = { }; const PULL = `query($id: ID!) { node(id: $id) { ... on PullRequest { - state baseRefName headRefOid mergeStateStatus mergeQueueEntry { enqueuedAt } autoMergeRequest { enabledAt } + state baseRefName headRefOid mergeStateStatus reviewDecision mergeQueueEntry { enqueuedAt } autoMergeRequest { enabledAt } labels(first: 100) { nodes { name } } headRepository { nameWithOwner } } } }`; @@ -761,13 +761,13 @@ async function requeue( try { if (queue.kind === "own") await enqueue(github, id, head, queue.method); else - await queueHead( - (query, variables) => github.graphql(query, variables), + await queueHead((query, variables) => github.graphql(query, variables), { id, head, - string(pull.mergeStateStatus), - "merge", - ); + mergeState: string(pull.mergeStateStatus), + unapproved: lacksApproval(pull.reviewDecision), + method: "merge", + }); } catch (error) { if (!(error instanceof GitHubGraphQLError) || error.types.some((type) => UNREFUSED.includes(type ?? ""))) throw error; diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index 323b55c5..98816a65 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -14,6 +14,7 @@ import { type Repository, string, } from "./github"; +import { lacksApproval } from "../github/merge"; import type { Entry, LandMethod, Plan } from "./land"; import { authenticatedRead } from "./land-git"; import { jobLog } from "./land-checks"; @@ -164,7 +165,7 @@ const OPEN = `query($owner: String!, $name: String!, $base: String!, $qualified: pullRequests(states: OPEN, baseRefName: $base, first: 50, after: $after) { pageInfo { hasNextPage endCursor } nodes { - id number url title headRefOid headRefName maintainerCanModify mergeStateStatus + id number url title headRefOid headRefName maintainerCanModify mergeStateStatus reviewDecision headRepository { nameWithOwner } labels(first: 100) { nodes { name } } mergeQueueEntry { id } @@ -270,7 +271,8 @@ async function enqueueArmed( const refused: Record = {}; for (const pull of pulls) { if (pull.autoMergeRequest == null || pull.mergeQueueEntry != null) continue; - if (!READY.includes(string(pull.mergeStateStatus))) continue; + // An armed pick waiting on a required approval is auto-merge's to enqueue once it arrives. + if (!READY.includes(string(pull.mergeStateStatus)) || lacksApproval(pull.reviewDecision)) continue; const number = Number(pull.number); const head = string(pull.headRefOid); const armed = string(object(pull.autoMergeRequest).enabledAt); @@ -289,6 +291,7 @@ async function enqueueArmed( now.mergeQueueEntry != null || request === null || string(request.enabledAt) !== armed || + lacksApproval(now.reviewDecision) || // Another session holds it: leave the pick armed, for the next pass once the hold clears. isHeld(now) ) diff --git a/src/cli/limit.ts b/src/cli/limit.ts index 54d79448..704c0362 100644 --- a/src/cli/limit.ts +++ b/src/cli/limit.ts @@ -9,8 +9,8 @@ export const EXHAUSTED = /credit balance is too low|insufficient_quota|exceeded /** * Messages for a spent usage limit or key balance, such as Claude Code's - * "You've hit your weekly limit" or "5-hour limit reached", Codex's "You've hit - * your usage limit", and opencode's providers' errors. A CLIProxyAPI pool whose + * "You've hit your weekly limit" or "5-hour limit reached", and Codex's "You've hit + * your usage limit". A CLIProxyAPI pool whose * every credential is cooling down answers "All credentials for model ... are * cooling down", which Claude Code shows, with the `model_cooldown` code in the * OpenAI-style formats; the proxy has already retried every credential, so it diff --git a/src/cli/main.ts b/src/cli/main.ts index 8a681772..743bbe23 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -26,6 +26,7 @@ import { import { AgentInputError, HARNESSES, + type HarnessName, type Launch, admitAgent, processDependencies, @@ -55,10 +56,8 @@ import { landed, landingCheck } from "./landed"; import { type Report, fixHeadings, fixLinks, moveQuest, removeQuest } from "./move"; import { type ResolveOptions, resolveFile } from "./resolve"; import { hosts } from "../core/doc"; -import { DEFAULT_SIZE, REVIEWERS, SECOND_REVIEWER, WRITERS, questWriters, writerAt } from "./models"; +import { DEFAULT_SIZE, REVIEWERS, WRITERS, questWriters, writerAt } from "./models"; import { - REVIEW_HARNESSES, - type ReviewHarness, ReviewInputError, ReviewLimitError, processDependencies as reviewDependencies, @@ -642,7 +641,9 @@ export function main(argv: string[], out: Output): number | Promise { "once when nothing is left to wait for. It takes no label: the auto-merge or merge-queue entry it " + "creates is the queue signal. It refuses while a session holds the PR, so release your own hold " + "first, once the head has moved, and while the latest run of any check on the head failed, required " + - "or not, naming each; a pending or absent check does not block. It refuses a head on which quest check " + + "or not, naming each; a pending or absent check does not block. While the base requires an approval the PR " + + "lacks (GitHub's review decision is review_required or changes_requested), it only arms auto-merge, which waits " + + "for it, and refuses where nothing would wait, since an admin's token merges or enqueues past the requirement. It refuses a head on which quest check " + "--no-stubs --base --branch finds anything, read from git objects fetched from the base repository, " + "so nothing from the head runs, except findings the base already has; a base without quests skips it. It uses the method --squash or --rebase names, refusing one " + "the repository disallows; otherwise the only method the repository allows, or else a merge commit. " + @@ -1159,8 +1160,7 @@ export function main(argv: string[], out: Output): number | Promise { "checkout's remotes, or a Codex log for its directory names this repository, read from " + "$CLAUDE_CONFIG_DIR (or ~/.config/claude and ~/.claude) and $CODEX_HOME (or ~/.codex); no prompt or reply is read out. Its role is " + "reviewing when the PR's ## Review record names it or Codex marks it a review, implementing when it began " + - "before the PR opened, and iterating otherwise. An opencode session, whose log quest does not read, belongs to the newest PR whose " + - "## Review record names it, as reviewing. Tokens and cost come from ccusage (session --json), the " + + "before the PR opened, and iterating otherwise. Tokens and cost come from ccusage (session --json), the " + "cost an estimate at each model's API list price. Each PR also shows its ## Models size and implementers, and as unread each Implementer line it cannot read, " + "each reviewer the ## Review record names, with its latest round and score and its tally of findings across every round, marked when it is the shadow, " + "where a merged PR whose last round left only low findings open counts them settled, since the author fixes or refutes those before merge, " + @@ -1230,11 +1230,10 @@ export function main(argv: string[], out: Output): number | Promise { .description( "Review the checkout's committed branch against its base with a reviewer its size picks, never the " + `model that changed it last. Each size ranks its reviewers (harness, model, effort): ${reviewers}. The ` + - "first whose model is not avoided reviews, and beside it, on the same head and each in its own session, " + - `${SECOND_REVIEWER.harness} ${SECOND_REVIEWER.model} ${SECOND_REVIEWER.effort} for every size, and the shadow: ` + + "first whose model is not avoided reviews, and beside it, on the same head in its own session, the shadow: " + "the pick's model one effort lower (low, medium, high, xhigh, max), skipped when the pick runs at low or an " + "effort off that ladder, so each effort step can be compared with the one below it. The shadow runs on a " + - "branch's first run of the pair, then only while it has findings to settle or once the pick changed so it is " + + "branch's first run of the pick, then only while it has findings to settle or once the pick changed so it is " + "no longer one step below; its findings are numbered and settled like any reviewer's, its session carries " + "over to the new shadow, and a pick with no step below it seats a shadow at its own effort while the old " + "shadow still has findings. " + @@ -1244,10 +1243,9 @@ export function main(argv: string[], out: Output): number | Promise { "it is any model of the harness running this command, usually the session that just pushed, detected from the " + "variables each sets, so Codex reviews from Claude Code and Claude Code from Codex; with --harness it is " + "none. " + - "--harness names the only reviewer instead of the pair and the shadow, at its entry's model and effort, and --model and " + + "--harness names the only reviewer instead of the pick and the shadow, at its entry's model and effort, and --model and " + "--effort override that reviewer's, or without --harness, the size pick's; a repository whose AGENTS.md names other reviewers " + - "passes them. opencode is no size's pick: --harness opencode needs --model, as provider/model, and " + - "--effort, its variant. A --harness run keeps the other reviewers' sessions for the next run of the pair, and refuses while one has findings to settle, as a run of the pair does while a reviewer only --harness runs has some. When the latest writer changes the size's pick to one with no session on the branch, its first session settles the open findings of the reviewer it replaces. The reviewers run at once, and the first to fail stops the others and fails the " + + "passes them. A --harness run keeps the other reviewers' sessions for the next run of the pick, and refuses while one has findings to settle, as a run of the pick does while a reviewer only --harness runs has some. When the latest writer changes the size's pick to one with no session on the branch, its first session settles the open findings of the reviewer it replaces. The reviewers run at once, and the first to fail stops the others and fails the " + "run, saving no round. The " + "size is --size, else the size of the quest a quest/ branch names, read from the base's tip so a " + `change cannot lower its own review, else the PR record's Size line, else ${DEFAULT_SIZE}. ` + @@ -1259,9 +1257,7 @@ export function main(argv: string[], out: Output): number | Promise { "or CLAUDE_CODE_OAUTH_TOKEN for Claude Code, CODEX_API_KEY or CODEX_ACCESS_TOKEN for Codex) passes only " + "when the CLI has no stored login or a gateway or provider is set, and a provider's own keys " + "(ANTHROPIC_AWS_API_KEY, ANTHROPIC_FOUNDRY_API_KEY, ANTHROPIC_FOUNDRY_AUTH_TOKEN) only while one is set. A logged-out error names the profile it checked, " + - "CLAUDE_CONFIG_DIR or CODEX_HOME, or the default when unset, and any gateway or provider. opencode " + - "keeps the user's own config and login, and counts as logged out when `opencode models` does not " + - "list the model.\n\n" + + "CLAUDE_CONFIG_DIR or CODEX_HOME, or the default when unset, and any gateway or provider.\n\n" + "Every turn reviews only the branch's own change, from the merge base of HEAD with the base's tip, so " + "a branch behind its base draws no findings on the base's newer code. The first round runs the " + "reviewer's own review (`codex exec review` with custom instructions, or Claude Code's " + @@ -1278,18 +1274,14 @@ export function main(argv: string[], out: Output): number | Promise { "starts a new session that reviews the whole change and still weighs --responses. A round that would " + "resume on the head the last round reviewed, when that round found nothing, runs no reviewer and " + "repeats that round, marked unchanged, so on the same head only a reviewer with findings to settle runs. " + - "--new starts new sessions, continuing each reviewer's tally of its findings on the branch, less those it drops unsettled, unless the saved state is one this quest cannot read. Each reviewer's session is kept per branch in the clone's common Git directory, so any worktree of the clone resumes it, except opencode's, which stays in the checkout it started in. " + + "--new starts new sessions, continuing each reviewer's tally of its findings on the branch, less those it drops unsettled, unless the saved state is one this quest cannot read. Each reviewer's session is kept per branch in the clone's common Git directory, so any worktree of the clone resumes it. " + "A checked-out branch with an upstream other than the base continues its upstream branch's session, so a " + "temporary branch tracking the PR's head shares the head's rounds. " + "--branch names the branch instead of the checked-out one, as a detached HEAD requires, such as a " + "checkout of a branch another worktree holds.\n\n" + "The reviewer is read-only: Codex in its read-only sandbox without execpolicy rules, user config, or its " + "skills catalog, Claude Code with " + - "no settings files, only read tools, and the Bash commands it judges read-only, and opencode with no " + - "project config, Claude Code files, or tool but its shell, whose commands run under sandbox-runtime " + - "(`srt` on PATH; on Linux it must refuse Unix sockets) with no network, reading only the checkout " + - "and its Git directories and writing only a private directory. opencode has no review mode, so its " + - "first turn asks for the review in a prompt. It follows the base's AGENTS.md and CLAUDE.md, from the root and " + + "no settings files, only read tools, and the Bash commands it judges read-only. It follows the base's AGENTS.md and CLAUDE.md, from the root and " + "each directory holding a changed path, never the checkout's, so a change cannot instruct its own " + "reviewer, and is told to disregard other instructions and run no builds, tests, or quest commands. " + "The base is fetched first, since the sandbox has no network.\n\n" + @@ -1321,8 +1313,8 @@ export function main(argv: string[], out: Output): number | Promise { .addOption( new Option( "--harness ", - "The only reviewer to run, instead of the size's pick, the second, and the shadow", - ).choices(REVIEW_HARNESSES), + "The only reviewer to run, instead of the size's pick and the shadow", + ).choices(HARNESSES), ) .option("--model ", "Model the size's pick, or --harness's reviewer, runs instead") .option("--effort ", "Reasoning effort the size's pick, or --harness's reviewer, runs at instead") @@ -1348,7 +1340,7 @@ export function main(argv: string[], out: Output): number | Promise { (options: { base?: string; branch?: string; - harness?: ReviewHarness; + harness?: HarnessName; model?: string; effort?: string; excludeModel?: string[]; diff --git a/src/cli/models.ts b/src/cli/models.ts index 0bed0272..a70456cd 100644 --- a/src/cli/models.ts +++ b/src/cli/models.ts @@ -4,7 +4,6 @@ // (quest/m1/model-check.md) audits these lists each milestone. import { type Doc, SIZES, type Size, onMaintainer, size, waits } from "../core"; import type { HarnessName } from "./agent"; -import type { Reviewing } from "./review"; /** One harness running one model at one effort. */ export interface Model { @@ -38,8 +37,7 @@ export const WRITERS: Record = { /** * Each PR size's reviewers, ranked: the first whose model is not the latest - * writer's reviews it, so Claude reviews what Sol changed last, while - * `SECOND_REVIEWER` reviews the same head beside it. + * writer's reviews it, so Claude reviews what Sol changed last. */ export const REVIEWERS: Record = { XS: [sol("medium"), opus("low")], @@ -49,16 +47,6 @@ export const REVIEWERS: Record = { XL: [sol("xhigh"), opus("xhigh")], }; -/** - * The second reviewer of every PR, beside its size's: DeepSeek through opencode, a model family no writer - * runs, for every size, since a review costs it about $0.05. - */ -export const SECOND_REVIEWER: Reviewing = { - harness: "opencode", - model: "opencode-go/deepseek-v4.1-flash", - effort: "high", -}; - /** * The writers that start quest `doc`: its size's list, or the next size's * while a `Maintainer:` wait leaves it a decision to settle. diff --git a/src/cli/review-record.ts b/src/cli/review-record.ts index 4c42d521..076076de 100644 --- a/src/cli/review-record.ts +++ b/src/cli/review-record.ts @@ -7,7 +7,7 @@ import { git } from "./git"; import { requireRemote, string } from "./github"; import { type PullModels, type Round, SEVERITIES, type Tally, located, parseBase, sameSeat } from "./review"; -const HARNESSES: Record = { claude: "Claude Code", codex: "Codex", opencode: "opencode" }; +const HARNESSES: Record = { claude: "Claude Code", codex: "Codex" }; /** * Model text as one inline code span, so a mention, issue reference, or Markdown in it renders as text. diff --git a/src/cli/review.ts b/src/cli/review.ts index e419f369..0ab88765 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -1,17 +1,17 @@ // `quest review`: a model other than the latest writer reviews the committed branch, read-only, -// picked by the PR's size from the lists in `models.ts`, and the second reviewer and the shadow, the -// picked model one effort lower, review the same head beside it, each in its own session. +// picked by the PR's size from the lists in `models.ts`, and the shadow, the picked model one effort +// lower, reviews the same head beside it, each in its own session. // -// A branch's first round runs the reviewer CLI's own review, or for opencode, which -// has none, a review prompt, given the scope, the base's REVIEW.md, and any -// responses up front, then resumes that session for one turn that only reports the -// review in the schema, since no CLI's built-in review returns a schema itself. Later rounds resume the same session with the new +// A branch's first round runs the reviewer CLI's own review, given the scope, the +// base's REVIEW.md, and any responses up front, then resumes that session for one +// turn that only reports the review in the schema, since neither CLI's built-in +// review returns a schema itself. Later rounds resume the same session with the new // head and the author's responses, checking only what changed and the open findings, // so the reviewer keeps its context and its prompt cache, and a finding the author // refuted with evidence does not recur. import { execFile, spawn } from "node:child_process"; import { createHash, randomUUID } from "node:crypto"; -import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { mkdirSync, readFileSync } from "node:fs"; import { dirname, join, posix } from "node:path"; import { promisify } from "node:util"; @@ -19,18 +19,11 @@ import { SIZES, type Size, branchQuest, parse, size as questSize } from "../core import { HARNESSES, type HarnessName, LINE_LIMIT, LOGINS, boundedLines, strippedEnvironment } from "./agent"; import { discoveryEnv, git } from "./git"; import { LIMIT, codexReset } from "./limit"; -import { DEFAULT_SIZE, type Model, REVIEWERS, SECOND_REVIEWER } from "./models"; +import { DEFAULT_SIZE, type Model, REVIEWERS } from "./models"; import { programEnvironment, requireProgram } from "./program"; -import { type Sandbox, openSandbox } from "./sandbox"; import type { recorded } from "../github/models"; import { save } from "./worktree"; -/** The CLIs that review: the agent harnesses, and opencode, which reviews only when named. */ -export const REVIEW_HARNESSES = [...HARNESSES, "opencode"] as const; -export type ReviewHarness = (typeof REVIEW_HARNESSES)[number]; -/** A reviewer: a size list's entry, or one `--harness`, `--model`, and `--effort` name. */ -export type Reviewing = Omit & { harness: ReviewHarness }; - /** A mistake in the arguments, checkout, or environment, reported as a usage error. */ export class ReviewInputError extends Error {} /** `--responses` is missing after a round with findings, or given with no round to answer. */ @@ -39,7 +32,7 @@ export class ResponsesError extends ReviewInputError {} /** The reviewer's account hit its usage limit on `model`, which resets at `until` (epoch milliseconds) if the CLI named it. */ export class ReviewLimitError extends Error { constructor( - readonly harness: ReviewHarness, + readonly harness: HarnessName, readonly until: number | null, readonly model: string, ) { @@ -82,7 +75,7 @@ export interface Usage { /** One reviewer's finished round, as `quest review` prints it. */ export interface Round { - reviewer: ReviewHarness; + reviewer: HarnessName; /** The round is the shadow's: the size's pick one effort lower, whose findings measure what that step buys. */ shadow?: true; model: string; @@ -186,7 +179,7 @@ export function runningHarness(env: NodeJS.ProcessEnv): HarnessName { /** What picks a round's reviewer from its size's list. */ export interface Choice { - harness?: ReviewHarness; + harness?: HarnessName; model?: string; effort?: string; /** The models never to pick; without any, the latest writer's is avoided. */ @@ -200,7 +193,7 @@ export interface Choice { * the harness running this command, which is usually the session that just * pushed, unless `--harness` names the reviewer, which then avoids nothing. */ -export function authored(choice: Choice, env: NodeJS.ProcessEnv, latest?: string): (entry: Reviewing) => boolean { +export function authored(choice: Choice, env: NodeJS.ProcessEnv, latest?: string): (entry: Model) => boolean { const exclude = choice.exclude ?? []; if (exclude.length > 0) return (entry) => exclude.includes(entry.model); if (latest !== undefined && latest !== "unknown") return (entry) => entry.model === latest; @@ -215,7 +208,7 @@ export function authored(choice: Choice, env: NodeJS.ProcessEnv, latest?: string * overriding its model and effort. An override never brings back an avoided * model. */ -export function pickReviewer(size: Size, choice: Choice, avoided: (entry: Reviewing) => boolean): Reviewing { +export function pickReviewer(size: Size, choice: Choice, avoided: (entry: Model) => boolean): Model { const list = REVIEWERS[size]; const entry = choice.harness === undefined ? list.find((e) => !avoided(e)) : list.find((e) => e.harness === choice.harness); @@ -269,7 +262,7 @@ export type ReviewOptions = BaseOptions & { /** The checkout to review. */ root: string; /** The reviewer; defaults to the first of the size's list whose model is not the latest writer's. */ - harness?: ReviewHarness; + harness?: HarnessName; model?: string; effort?: string; /** The models never to pick; without any, the latest writer's is avoided. */ @@ -307,7 +300,7 @@ export const processDependencies = (): ReviewDependencies => ({ env: process.env /** One reviewer's findings across every round on a branch, which a `--new` session continues. */ export interface Tally { - reviewer: ReviewHarness; + reviewer: HarnessName; /** The shadow's tally, kept apart from the size's pick of the same harness. */ shadow?: true; /** Its latest round's model, effort, and session, which the record names even once a run leaves it out. */ @@ -336,18 +329,17 @@ export interface Reviewed { */ interface State { branch: string; + /** As read, these may include a retired reviewer's, which a run only numbers past; see `runs`. */ sessions: Session[]; tallies: Tally[]; } /** One reviewer's session on a branch. */ interface Session { - reviewer: ReviewHarness; + reviewer: HarnessName; /** The shadow's session; the branch keeps at most one. */ shadow?: true; session: string; - /** The checkout the session started in. */ - root: string; /** Each round's head, the merge base its change started from, and its findings. */ rounds: { head: string; mergeBase: string; findings: Finding[] }[]; /** The session's last round, which a round on the same head with nothing to settle repeats. */ @@ -376,8 +368,6 @@ export interface Context { schemaFile: string; /** The gateway Codex sends its requests to, from OPENAI_BASE_URL, which Codex itself no longer reads. */ openaiBaseUrl?: string; - /** opencode's: the sandboxed shell its commands run through, and the file holding `instructions`. */ - sandbox?: { shell: string; instructions: string }; } interface Turn { @@ -402,24 +392,13 @@ interface Reply { interface Reader { line(line: string): void; - /** Whether to stop the CLI now, as when it would wait out a usage limit; the turn then fails. */ - stop?(): boolean; /** The turn's reply once the CLI exited with `code`, given the tail of its stderr. */ reply(code: number, stderr: string): Reply; } interface Reviewer { - /** Whether a session stays in the checkout it started in, so a round from another worktree starts a new one. */ - bound?: true; /** Exits non-zero when the CLI is logged out. */ status: string[]; - /** - * Whether the status check's output lists `model`, for a CLI that may be - * logged in to some providers but not the model's. - */ - lists?(output: string[], model: string): boolean; - /** Variables every run of the CLI gets, its status check's included. */ - env: Record; /** How the user logs in. */ login: string; /** The variable naming the configuration directory, and so the stored login, the status check reads. */ @@ -477,7 +456,6 @@ const CLAUDE_PROVIDERS = ["BEDROCK", "VERTEX", "FOUNDRY", "MANTLE", "ANTHROPIC_A */ const claude: Reviewer = { status: ["claude", "auth", "status"], - env: {}, login: LOGINS.claude.command, profile: "CLAUDE_CONFIG_DIR", credentials: ["ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"], @@ -622,7 +600,6 @@ const claudeEnv = () => ({ CLAUDE_CODE_PROMPT_CACHE_TTL: "1h" }); */ const codex: Reviewer = { status: ["codex", "login", "status"], - env: {}, login: LOGINS.codex.command, profile: "CODEX_HOME", // `codex exec` reads CODEX_API_KEY, though `codex login status` does not count it; it ignores OPENAI_API_KEY. @@ -713,161 +690,7 @@ function codexFlags(context: Context): string[] { ]; } -/** opencode's errors for an account's spent usage limit or budget, which it may retry until the reset. */ -const OPENCODE_LIMITS = new Set([ - "provider.quota", - "provider.go-limit", - "provider.free-limit", - "provider.chatgpt-limit", - "provider.budget-limit", - "provider.model-budget-limit", -]); - -/** - * When an opencode limit error resets, in epoch milliseconds: the `resetsAt` - * its provider's response names, or its `retryAfterMs` from `at`, as - * opencode's own interface reads it. - */ -export function opencodeReset(error: Record, at: number): number | null { - const body = text(record(error.response)?.body) ?? ""; - const resets = Date.parse(/"resetsAt"\s*:\s*"([^"]+)"/.exec(body)?.[1] ?? ""); - if (!Number.isNaN(resets)) return resets; - const after = error.retryAfterMs; - return typeof after === "number" && Number.isFinite(after) && after >= 0 ? at + after : null; -} - -/** A reply that wraps its JSON in a Markdown code fence, as models without an enforced schema may. */ -const unfenced = (reply: string) => /^\s*```(?:json)?\s*\n([\s\S]*?)\n\s*```\s*$/.exec(reply)?.[1] ?? reply; - -/** What opencode is asked to do on a branch's first turn, since it has no review mode of its own. */ -const OPENCODE_REVIEW = - "Review this change for bugs: read it with read-only shell commands, such as `git diff`, `git show`, `cat`, " + - "and `rg`, and report each problem that holds, with its file, line, and the concrete scenario that breaks."; - -/** - * opencode loads no project config, which holds plugins, MCP servers, a - * shell, and instructions that would run or speak in the reviewer, nor Claude - * Code's files, and every tool but its shell is denied: no file writes, web - * access, questions, skills, or Code Mode. The shell runs each command under - * sandbox-runtime (`sandbox.ts`), with no network, writes only to a private - * directory, and reads only the checkout and its Git directories, since the - * server itself runs unconfined. No language servers or formatters run, and - * no snapshot or share. It keeps the user's config, such as custom providers, - * and its login; the denials come last, so none of the user's own rules - * outrank them. Sessions are created archived, out of the user's list. - */ -const opencode: Reviewer = { - status: ["opencode", "models", "--standalone"], - lists: (output, model) => output.some((line) => line.trim() === model), - env: { - OPENCODE_DISABLE_PROJECT_CONFIG: "1", - OPENCODE_DISABLE_CLAUDE_CODE: "1", - OPENCODE_DISABLE_AUTOUPDATE: "1", - }, - login: "opencode auth login", - profile: "XDG_DATA_HOME", - credentials: [], - routeCredentials: [], - connection: [], - routes: () => [], - unauthorized: /\bprovider\.auth\b/, - // A resumed session runs in the directory it started in, and fails once that worktree is gone. - bound: true, - review: (context, _session, prompt) => opencodeTurn(context, [], `${OPENCODE_REVIEW}\n\n${prompt}`), - // `opencode run` takes no schema, so the prompt carries it. - resume: (context, session, prompt) => - opencodeTurn( - context, - ["--session", session], - `${prompt}\n\nThe response schema:\n\n${JSON.stringify(context.schema)}\n\nReply with only the JSON object.`, - ), - // A spent limit fails the turn as an `error` event, or shows up as a `retry` one, which would - // wait for the reset, however far off, so the turn stops there. - reader() { - let session: string | undefined; - // The text since the last tool call, which holds the answer. - let reply: string[] = []; - let usage: Omit | undefined; - let error: string | undefined; - let limited: { until: number | null } | undefined; - let stop = false; - const failed = (value: unknown, at: number) => { - const failure = record(value) ?? {}; - const message = `${text(failure.type) ?? "error"}: ${text(failure.message) ?? "opencode reported an error"}`; - if (OPENCODE_LIMITS.has(text(failure.type) ?? "") || LIMIT.test(message)) - limited ??= { until: opencodeReset(failure, at) }; - return message; - }; - return { - line(line) { - const event = json(line); - if (event === undefined) return; - session ??= text(event.sessionID); - const part = record(event.part); - const at = typeof event.timestamp === "number" ? event.timestamp : Date.now(); - if (event.type === "tool_use") reply = []; - if (event.type === "text") reply.push(text(part?.text) ?? ""); - if (event.type === "step_finish") { - const tokens = record(part?.tokens) ?? {}; - const cache = record(tokens.cache) ?? {}; - usage ??= { input: 0, cached: 0, output: 0 }; - usage.input += count(tokens.input) + count(cache.read) + count(cache.write); - usage.cached += count(cache.read); - usage.output += count(tokens.output) + count(tokens.reasoning); - } - if (event.type === "error") error = failed(event.error, at); - if (event.type === "retry") { - const message = failed(event.error, at); - if (limited || opencode.unauthorized.test(message)) { - error = message; - stop = true; - } - } - }, - stop: () => stop, - reply(_code, stderr) { - if (!limited && error === undefined && LIMIT.test(stderr)) limited = { until: null }; - const answer = reply.join("").trim(); - return { session, text: answer === "" ? undefined : unfenced(answer), usage, error, limited }; - }, - }; - }, -}; - -/** - * One `opencode run` in a private server, configured on every turn since a - * resumed session takes its config from the server, not the session. - */ -export function opencodeTurn(context: Context, resume: string[], prompt: string): Turn { - if (context.sandbox === undefined) throw new Error("opencode reviews only with a sandboxed shell"); - const deny = (action: string) => ({ action, resource: "*", effect: "deny" }); - const config = { - shell: context.sandbox.shell, - instructions: [context.sandbox.instructions], - lsp: false, - formatter: false, - snapshot: false, - share: "disabled", - permissions: [deny("*"), deny("skill"), { action: "shell", resource: "*", effect: "allow" }], - }; - return { - argv: [ - "opencode", - "run", - "--standalone", - "--format", - "json", - "--archived", - "--model", - `${context.model}#${context.effort}`, - ...resume, - ], - stdin: prompt, - env: { OPENCODE_CONFIG_CONTENT: JSON.stringify(config) }, - }; -} - -const CLIS: Record = { claude, codex, opencode }; +const CLIS: Record = { claude, codex }; /** Codex's review turn reports all zeros, which is no measurement. */ const reported = (usage: Omit | undefined): usage is Omit => @@ -901,10 +724,9 @@ function run( reject(new ReviewInputError((error as Error).message)); return; } - // opencode runs in PWD rather than its working directory. const child = spawn(program, argv.slice(1), { cwd, - env: programEnvironment({ ...env, PWD: cwd }), + env: programEnvironment(env), stdio: ["pipe", "pipe", "pipe"], }); const signals = ["SIGINT", "SIGTERM", "SIGHUP"] as const; @@ -923,14 +745,7 @@ function run( clearTimeout(killing); }; let stderr = ""; - let stopped = false; - const lines = boundedLines(LINE_LIMIT, (line) => { - reader?.line(line); - if (!stopped && reader?.stop?.()) { - stopped = true; - abort(); - } - }); + const lines = boundedLines(LINE_LIMIT, (line) => reader?.line(line)); child.stdout.setEncoding("utf8").on("data", (chunk: string) => lines.data(chunk)); child.stdout.once("end", () => lines.end()); child.stderr.setEncoding("utf8").on("data", (chunk: string) => { @@ -955,7 +770,7 @@ const ABORT_GRACE_MS = 10_000; /** The reviewer CLI with the environment it runs in and the error for a login the API rejects. */ interface Cli { - harness: ReviewHarness; + harness: HarnessName; reviewer: Reviewer; env: NodeJS.ProcessEnv; /** Thrown when a turn fails on authentication, as a logged-out status check fails. */ @@ -985,36 +800,21 @@ const pick = (env: NodeJS.ProcessEnv, names: string[]) => * shell's API key never overrides a stored subscription login, so a review is * billed as the user's own CLI sessions are. */ -async function prepare( - harness: ReviewHarness, - model: string, - cwd: string, - env: NodeJS.ProcessEnv, - signal?: AbortSignal, -): Promise { +async function prepare(harness: HarnessName, cwd: string, env: NodeJS.ProcessEnv, signal?: AbortSignal): Promise { const reviewer = CLIS[harness]; - const connected = { ...strippedEnvironment(env), ...pick(env, reviewer.connection), ...reviewer.env }; + const connected = { ...strippedEnvironment(env), ...pick(env, reviewer.connection) }; const routes = reviewer.routes(env); const through = routes.length > 0 ? ` through ${names(routes)}` : ""; const loggedOut = `${harness} is not logged in to ${profile(reviewer, env)}${through}; run \`${reviewer.login}\``; - // A CLI logged in to some providers passes its status check only for their models. - const unlisted = - reviewer.lists === undefined - ? loggedOut - : `${harness} lists no model ${model} for ${profile(reviewer, env)}; run \`${reviewer.login}\` for its provider, or name a model \`${reviewer.status.join(" ")}\` lists`; - const loggedIn = async (env: NodeJS.ProcessEnv) => { - const output: string[] = []; - const listed = { line: (line: string) => void output.push(line), reply: () => ({}) }; - const { code } = await run(reviewer.status, cwd, env, "", listed, signal); - return code === 0 && (reviewer.lists === undefined || reviewer.lists(output, model)); - }; + const loggedIn = async (env: NodeJS.ProcessEnv) => + (await run(reviewer.status, cwd, env, "", undefined, signal)).code === 0; const credentials = [...reviewer.credentials, ...(routes.length > 0 ? reviewer.routeCredentials : [])].filter( (name) => env[name], ); const withCredentials = { ...connected, ...pick(env, credentials) }; const rejected = `${harness} rejected ${names(credentials)}${through}; replace it, or run \`${reviewer.login}\``; if (routes.length > 0) { - if (!(await loggedIn(withCredentials))) throw new ReviewInputError(unlisted); + if (!(await loggedIn(withCredentials))) throw new ReviewInputError(loggedOut); return { harness, reviewer, @@ -1028,18 +828,17 @@ async function prepare( credentials.length > 0 ? `; quest review strips ${names(credentials)} while a stored login exists` : ""; return { harness, reviewer, env: connected, unauthorized: `${loggedOut}${ignored}`, signal }; } - if (credentials.length === 0) throw new ReviewInputError(unlisted); + if (credentials.length === 0) throw new ReviewInputError(loggedOut); return { harness, reviewer, env: withCredentials, unauthorized: rejected, signal }; } -/** The environment `quest review` runs `harness`'s CLI on `model` in, for a caller that runs it with the same login. */ +/** The environment `quest review` runs `harness`'s CLI in, for a caller that runs it with the same login. */ export async function reviewerEnvironment( - harness: ReviewHarness, - model: string, + harness: HarnessName, cwd: string, env: NodeJS.ProcessEnv, ): Promise { - return (await prepare(harness, model, cwd, env)).env; + return (await prepare(harness, cwd, env)).env; } /** @@ -1425,7 +1224,7 @@ function load(path: string): unknown { } /** A reviewer's seat on a branch: its harness, and whether it is the shadow. */ -type SeatId = { reviewer: ReviewHarness; shadow?: true }; +type SeatId = { reviewer: HarnessName; shadow?: true }; const seatKey = (seat: SeatId) => `${seat.reviewer}${seat.shadow ? " shadow" : ""}`; /** Whether two reviewers hold the same seat: the same harness, both the shadow or neither. */ export const sameSeat = (a: SeatId, b: SeatId) => seatKey(a) === seatKey(b); @@ -1437,7 +1236,7 @@ const EFFORTS = ["low", "medium", "high", "xhigh", "max"]; * The shadow of `routed`, the size's pick: its model one effort lower, or null when no lower effort is known, * as for Opus low or an `--effort` off the ladder, such as Codex's `minimal`. */ -export function shadowOf(routed: Reviewing): Reviewing | null { +export function shadowOf(routed: Model): Model | null { const at = EFFORTS.indexOf(routed.effort); return at <= 0 ? null : { ...routed, effort: EFFORTS[at - 1]! }; } @@ -1470,7 +1269,6 @@ export function parseState(value: unknown): State { (session.shadow === undefined || session.shadow === true) && state.tallies!.some((tally) => sameSeat(tally, session)) && typeof session.session === "string" && - typeof session.root === "string" && Array.isArray(session.rounds) && session.rounds.length > 0 && session.rounds.every((round) => Array.isArray(round?.findings)) && @@ -1481,9 +1279,17 @@ export function parseState(value: unknown): State { ) && state.sessions.filter((session) => session.shadow).length <= 1; if (!valid) throw new ReviewInputError("the saved review sessions are not ones quest review reads; pass --new"); - return state as State; + // A retired reviewer's tally is dropped, so the record never names it. Its session stays only until the run numbers + // the last round's findings as that round printed them, so the author's responses keep their numbers. + return { ...state, tallies: state.tallies!.filter(runs) } as State; } +/** + * Whether quest still runs `seat`'s reviewer. A retired one has no CLI to resume its session or settle its findings, + * so its session is dropped, open findings included, and the routed reviewer reviews the whole branch. + */ +const runs = (seat: { reviewer: string }) => (HARNESSES as readonly string[]).includes(seat.reviewer); + /** * The branch whose session a round continues: `name`, checked as a branch * name, or else the checked-out branch's upstream branch, without its remote, @@ -1570,22 +1376,9 @@ function recordedLatest(record: ModelsRecord, options: ReviewOptions): string | return record.implementers.at(-1)?.fields?.model; } -/** The sandbox opencode's commands run in, reading the checkout at `root` and its Git directories. */ -function sandboxFor(root: string, env: NodeJS.ProcessEnv): Sandbox { - const dirs = ["--absolute-git-dir", "--path-format=absolute --git-common-dir"].map((flag) => - git(root, ["rev-parse", ...flag.split(" ")]), - ); - try { - return openSandbox([...new Set([root, ...dirs])], env); - } catch (error) { - throw new ReviewInputError(`opencode reviews only in a sandbox: ${(error as Error).message}`); - } -} - /** - * Review the checkout's committed branch: one round of each reviewer on the same head, the size's pick, - * `SECOND_REVIEWER`, and the pick's shadow while it runs, or only `--harness`'s, each resuming its session on - * the branch unless `fresh`. + * Review the checkout's committed branch: one round of each reviewer on the same head, the size's pick and its + * shadow while it runs, or only `--harness`'s, each resuming its session on the branch unless `fresh`. * Every reviewer runs at once, and the first to fail stops the others and fails the whole, saving no round. */ export async function review(options: ReviewOptions, deps: ReviewDependencies): Promise { @@ -1612,42 +1405,37 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): const sized = reviewSize(root, baseSha, options.sizeBranch ?? branch, options.size, recordedSize(record, options)); const avoided = authored(options, deps.env, recordedLatest(record, options)); const routed = pickReviewer(sized.size, options, avoided); - // A named reviewer replaces the pair and the shadow, as for a repository whose instructions name its own. - const pair = options.harness === undefined; - const picked = [routed]; - if (pair) { - if (avoided(SECOND_REVIEWER)) - throw new ReviewInputError(`${SECOND_REVIEWER.model} is avoided; pass --harness to name a single reviewer`); - picked.push(SECOND_REVIEWER); - } - const shadow = pair ? shadowOf(routed) : null; + // A named reviewer runs without the shadow, as for a repository whose instructions name its own. + const auto = options.harness === undefined; + const shadow = auto ? shadowOf(routed) : null; const statePath = sessionFile(git(root, ["rev-parse", "--path-format=absolute", "--git-common-dir"]), branch); const dir = dirname(statePath); mkdirSync(dir, { recursive: true }); const saved = savedState(statePath, options.fresh); - const sessions = options.fresh || saved === undefined ? [] : saved.sessions; - // The last run numbered each session's findings after those of the sessions before it. + const all = options.fresh || saved === undefined ? [] : saved.sessions; + // The last run numbered each session's findings after those of the sessions before it, a retired reviewer's + // included, which are then dropped. const firsts = new Map(); let next = 1; - for (const session of sessions) { + for (const session of all) { firsts.set(session, next); next += session.rounds.at(-1)!.findings.length; } + const sessions = all.filter(runs); const seated = (session: Session | undefined) => ({ session, first: session === undefined ? 1 : firsts.get(session)!, }); - const seats = picked.map((reviewer): Seat => ({ - reviewer, - ...seated(sessions.find((each) => each.reviewer === reviewer.harness && !each.shadow)), - })); + const seats: Seat[] = [ + { reviewer: routed, ...seated(sessions.find((each) => each.reviewer === routed.harness && !each.shadow)) }, + ]; const unseated = () => sessions.filter((each) => !seats.some((seat) => seat.session === each)); const unsettled = (list: Session[]) => list.filter((each) => each.rounds.at(-1)!.findings.length > 0); // When the latest writer changed the size's pick, the new pick's first session settles the open findings // of the reviewer it replaces, by their numbers, so they are never dropped unanswered. const replaced = - pair && seats[0]!.session === undefined ? unsettled(unseated()).find((each) => !each.shadow) : undefined; + auto && seats[0]!.session === undefined ? unsettled(unseated()).find((each) => !each.shadow) : undefined; if (replaced !== undefined) seats[0] = { ...seats[0]!, ...seated(replaced) }; // The shadow reviews a branch's first run, then reruns only while it has findings to settle, or once the pick // changed so that the shadow it ran is no longer one step below it. Its session carries over to the new @@ -1663,20 +1451,20 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): shadowed.last.model === shadow.model && shadowed.last.effort === shadow.effort ); - if (pair && (shadowed === undefined ? shadow !== null : open || stale)) + if (auto && (shadowed === undefined ? shadow !== null : open || stale)) seats.push({ reviewer: shadow ?? routed, shadow: true, ...seated(shadowed) }); - // A run of the pair drops the other sessions but a shadow's, whose presence keeps it from rerunning. A run of - // --harness's reviewer alone keeps them all for the pair's next run. Either refuses to pass a head while one of - // them still has findings it cannot settle. + // A run of the size's pick drops the other sessions but a shadow's, whose presence keeps it from rerunning. A + // run of --harness's reviewer alone keeps them all for the pick's next run. Either refuses to pass a head while + // one of them still has findings it cannot settle. const others = unseated(); - const kept = pair ? others.filter((each) => each.shadow) : others; + const kept = auto ? others.filter((each) => each.shadow) : others; const held = unsettled(others); if (held.length > 0) { const names = held.map((each) => `${each.reviewer}${each.shadow ? " (shadow)" : ""}`).join(" and "); - // A pair run seats every shadow with findings, so what it holds is a reviewer only --harness runs. - const run = pair + // A run of the size's pick seats every shadow with findings, so what it holds is a reviewer only --harness runs. + const run = auto ? `${[...new Set(held.map((each) => `--harness ${each.reviewer}`))].join(" or ")}, adding --model or --exclude-model when its size's model is avoided,` - : "the pair, or of that reviewer with --harness,"; + : "the size's pick, or of that reviewer with --harness,"; throw new ReviewInputError( `${names} still ${held.length > 1 ? "have" : "has"} findings to settle on this branch; settle them in a run of ${run} with --responses, or pass --new to drop them`, ); @@ -1764,7 +1552,7 @@ function savedState(path: string, fresh: boolean | undefined): State | undefined */ function dropped(saved: State | undefined): Tally[] { const tallies = (saved?.tallies ?? []).map((tally) => ({ ...tally, findings: { ...tally.findings } })); - for (const session of saved?.sessions ?? []) { + for (const session of (saved?.sessions ?? []).filter(runs)) { const tally = tallies.find((each) => sameSeat(each, session))!; for (const finding of session.rounds.at(-1)!.findings) tally.findings[finding.severity]--; } @@ -1778,7 +1566,7 @@ function dropped(saved: State | undefined): Tally[] { */ export function tallied( tallies: Tally[], - rounds: { round: Round; settles?: { reviewer: ReviewHarness; shadow?: true } }[], + rounds: { round: Round; settles?: { reviewer: HarnessName; shadow?: true } }[], ): Tally[] { const by = new Map(tallies.map((tally) => [seatKey(tally), { ...tally, findings: { ...tally.findings } }])); for (const { round, settles } of rounds) { @@ -1838,7 +1626,7 @@ interface Shared { * or the replaced reviewer's, whose findings its new session settles. */ interface Seat { - reviewer: Reviewing; + reviewer: Model; shadow?: true; session?: Session; first: number; @@ -1861,9 +1649,8 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi // Responses answer findings by number; a reviewer with none of its own would read others' as its own. const answered = earlier.length > 0 ? responses : undefined; // A round checks only what changed since the last one when nothing but the head changed: the same - // reviewer, base, instructions, and REVIEW.md, a range the last round's covers, and for a reviewer bound - // to its checkout, the same one. Anything else starts a new session that reviews the whole change, since a - // resumed one keeps its first instructions. + // reviewer, base, instructions, and REVIEW.md, and a range the last round's covers. Anything else starts a + // new session that reviews the whole change, since a resumed one keeps its first instructions. const last = stored?.last; const resume = previous !== undefined && @@ -1874,7 +1661,6 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi last.model === picked.model && last.effort === picked.effort && last.base.ref === shared.base.ref && - (!reviewer.bound || stored!.root === root) && covers(root, previous.mergeBase, change.mergeBase); // The last round already passed this head, and has nothing left to settle; it repeats against the run's base @@ -1884,7 +1670,7 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi return { round: { ...last!, base: shared.base, size, sizeFrom, usage: [], unchanged: true }, session: stored! }; } - const cli = await prepare(harness, picked.model, root, shared.env, shared.signal); + const cli = await prepare(harness, root, shared.env, shared.signal); // Named by its bound, so a round on another branch never rewrites the file this one reads. const replySchema = schema(earlier.length); @@ -1903,32 +1689,22 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi const usage: Usage[] = []; let session: string; let reply: Reply; - const sandbox = harness === "opencode" ? sandboxFor(root, cli.env) : undefined; - try { - if (sandbox !== undefined) { - const instructions = join(sandbox.dir, "instructions.md"); - writeFileSync(instructions, context.instructions, { mode: 0o600 }); - context.sandbox = { shell: sandbox.shell, instructions }; - } - 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, earlier, first)), - root, - context.model, - ); - } else { - session = stored!.session; - const prompt = laterPrompt({ ...change, previous: previous!.head, earlier, first, responses: answered }); - reply = await turn(cli, reviewer.resume(context, session, prompt), root, context.model); - } - } finally { - sandbox?.close(); + 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, earlier, first)), + root, + context.model, + ); + } else { + session = stored!.session; + const prompt = laterPrompt({ ...change, previous: previous!.head, earlier, first, responses: answered }); + reply = await turn(cli, reviewer.resume(context, session, prompt), root, context.model); } if (reported(reply.usage)) usage.push({ turn: "findings", ...reply.usage }); const { score, rationale, findings: found, earlier: answers } = verdict(reply, earlier.length, first); @@ -1971,7 +1747,7 @@ async function reviewWith(shared: Shared, seat: Seat, responses: string | undefi }; return { round, - session: { reviewer: harness, ...(shadow && { shadow }), session, root, rounds, last: round, given }, + session: { reviewer: harness, ...(shadow && { shadow }), session, rounds, last: round, given }, }; } diff --git a/src/cli/sandbox.ts b/src/cli/sandbox.ts deleted file mode 100644 index 484e9f63..00000000 --- a/src/cli/sandbox.ts +++ /dev/null @@ -1,157 +0,0 @@ -// The OS sandbox an opencode reviewer's commands run in, through sandbox-runtime (`srt`): bubblewrap -// on Linux, Seatbelt on macOS. Commands read the checkout, its Git directory, and a private temporary -// directory, write only that directory, read no account home or shared temporary files, reach no -// network host, and on Linux make no Unix sockets, so not the Nix daemon's either. -import { spawnSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; -import { homedir, tmpdir } from "node:os"; -import { isAbsolute, join, relative } from "node:path"; - -import { requireProgram } from "./program"; - -/** Where other users' and this user's private files live, per platform. */ -function hidden(platform: NodeJS.Platform): string[] { - return platform === "darwin" - ? ["/Users", "/var/root", "/tmp", "/private/tmp", "/private/var/folders"] - : ["/home", "/root", "/tmp", "/var/tmp", "/run/user"]; -} - -/** The variables commands keep from the reviewer's environment, so a credential stays out of their reach. */ -const KEPT = - /^(PATH|USER|LOGNAME|SHELL|LANG|LANGUAGE|LC_\w+|TZ|TERM|SSL_CERT_(FILE|DIR)|NIX_SSL_CERT_FILE|__CF_USER_TEXT_ENCODING)$/; - -/** What a sandbox lets commands read and write. */ -export interface Paths { - /** Read only: the checkout and its Git directories. */ - reads: string[]; - /** The private directory, the only one commands write. */ - tmp: string; -} - -const inside = (path: string, dir: string) => { - const rel = relative(dir, path); - return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); -}; - -/** - * The whole environment `srt` runs commands with: the kept variables, with - * HOME and TMPDIR the private directory, and `CLAUDE_CODE_TMPDIR` too, since - * srt replaces a command's TMPDIR with it, or `/tmp/claude` without it. PATH - * keeps only absolute directories outside `reads`, so a command such as `git` - * never resolves to a program the checkout holds. - */ -export function sandboxEnvironment(env: NodeJS.ProcessEnv, paths: Paths): Record { - const kept = Object.fromEntries( - Object.entries(env).filter((entry): entry is [string, string] => KEPT.test(entry[0]) && entry[1] !== undefined), - ); - if (kept.PATH !== undefined) - kept.PATH = kept.PATH.split(":") - .filter((entry) => isAbsolute(entry) && !paths.reads.some((dir) => inside(entry, dir))) - .join(":"); - const { tmp } = paths; - return { ...kept, HOME: tmp, TMPDIR: tmp, CLAUDE_CODE_TMPDIR: tmp }; -} - -/** srt's settings: no network host, and the account homes and shared temporary directories hidden but `paths`. */ -export function sandboxSettings(paths: Paths, platform = process.platform, home = homedir()): Record { - return { - network: { allowedDomains: [], deniedDomains: [], strictAllowlist: true }, - filesystem: { - denyRead: [...new Set([...hidden(platform), home])], - allowRead: [...paths.reads, paths.tmp], - allowWrite: [paths.tmp], - // srt binds its own /tmp/claude in whenever it exists, whoever made it, and no - // denyRead hides it; this keeps commands from writing there, though not from reading it. - denyWrite: ["/tmp/claude"], - }, - enableWeakerNestedSandbox: false, - }; -} - -/** `value` as one word in a POSIX shell. */ -export function quote(value: string): string { - return `'${value.replaceAll("'", `'\\''`)}'`; -} - -/** - * The shell opencode runs every command through, as ` -c `. It - * starts the command from `env` rather than opencode's own environment, which - * may hold provider keys. - */ -export function shellScript(srt: string, settings: string, env: Record): string { - const kept = Object.entries(env).map(([name, value]) => quote(`${name}=${value}`)); - return `#!/bin/sh -if [ "$#" -ne 2 ] || [ "$1" != -c ]; then - echo "usage: $0 -c " >&2 - exit 2 -fi -exec /usr/bin/env -i ${kept.join(" ")} ${quote(srt)} --settings ${quote(settings)} -c "$2" -`; -} - -/** - * Why `srt` would let commands make Unix sockets, or null when it refuses - * them. On Linux, srt refuses them only through its seccomp helper, - * `apply-seccomp`, and runs commands without it when its package lacks one, - * as nixpkgs' does. It runs socat under the sandbox's policy and passes only - * on the filter's own refusal. - */ -export function unixSocketRefusal(srt: string, env: NodeJS.ProcessEnv): string | null { - const dir = realpathSync(mkdtempSync(join(tmpdir(), "quest-probe-"))); - try { - const paths = { reads: [], tmp: dir }; - const settings = join(dir, "srt.json"); - writeFileSync(settings, JSON.stringify(sandboxSettings(paths, "linux"))); - const probe = spawnSync( - srt, - ["--settings", settings, "-c", `exec socat -u OPEN:/dev/null UNIX-CONNECT:${quote(join(dir, "missing"))}`], - { cwd: dir, env: sandboxEnvironment(env, paths), stdio: ["ignore", "ignore", "pipe"], encoding: "utf8" }, - ); - if ( - !probe.error && - probe.status !== 0 && - /\bsocket\(1, \d+, \d+\): Operation not permitted$/m.test(probe.stderr) - ) - return null; - const said = - probe.error?.message ?? (probe.stderr.trim().split("\n").at(-1) || `exit ${probe.status ?? probe.signal}`); - if (/\bconnect\(.*\): No such file or directory$/m.test(probe.stderr)) - return `${srt} lets commands make Unix sockets, such as the Nix daemon's: it has no seccomp helper (apply-seccomp, which it looks for in its package's vendor/seccomp/). Use sandbox-runtime from npm, which ships it, or this repository's dev shell, which builds it`; - return `could not check that ${srt} keeps commands from making Unix sockets (${said})`; - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} - -/** A round's sandbox: the shell opencode runs commands through. */ -export interface Sandbox { - shell: string; - /** - * Holds the shell, its settings, and in `tmp/` the commands' HOME and - * TMPDIR, the only part commands write; `close` removes it. - */ - dir: string; - close(): void; -} - -/** - * Make a sandbox whose commands read `reads`. It sits directly in the - * temporary directory, since srt's bridge sockets live in it and a socket path - * is at most 104 bytes on macOS. Refuses an `srt` missing from PATH, or one - * that would let commands make Unix sockets. - */ -export function openSandbox(reads: string[], env: NodeJS.ProcessEnv): Sandbox { - const srt = requireProgram("srt", env); - if (process.platform === "linux") { - const refused = unixSocketRefusal(srt, env); - if (refused !== null) throw new Error(refused); - } - const dir = realpathSync(mkdtempSync(join(tmpdir(), "quest-review-"))); - const paths = { reads, tmp: join(dir, "tmp") }; - mkdirSync(paths.tmp, { mode: 0o700 }); - const settings = join(dir, "srt.json"); - writeFileSync(settings, JSON.stringify(sandboxSettings(paths)), { mode: 0o600 }); - const shell = join(dir, "sandboxed-sh"); - writeFileSync(shell, shellScript(srt, settings, sandboxEnvironment(env, paths)), { mode: 0o700 }); - return { shell, dir, close: () => rmSync(dir, { recursive: true, force: true }) }; -} diff --git a/src/cli/usage.ts b/src/cli/usage.ts index d2e24089..30f0511e 100644 --- a/src/cli/usage.ts +++ b/src/cli/usage.ts @@ -513,16 +513,14 @@ function owners(logs: (SessionLog | null)[]): (cwd: string) => string[] | null { * Attribute each priced session to the PR whose head branch its log last named, among those still open when the * session reached the branch and headed from the repository its log, checkout, or directory names, and give it a * role: reviewing when a PR's review record names it or Codex marks it a review, implementing when it began before - * its PR opened, and iterating otherwise. A session whose log quest cannot read, such as opencode's, is the - * reviewing session of the PR whose record names it, the newest when a branch's later PR resumed it. Returns the - * sessions on no PR here. + * its PR opened, and iterating otherwise. Returns the sessions on no PR here. */ function attribute( sessions: Priced[], logs: (SessionLog | null)[], pulls: Pull[], rows: Map, - reviewers: Map, + reviewers: Set, ): Priced[] { const byBranch = new Map(); for (const each of [...pulls].sort((a, b) => a.created - b.created)) @@ -530,16 +528,7 @@ function attribute( const unattributed: Priced[] = []; const owned = owners(logs); sessions.forEach((session, index) => { - const id = sessionId(session); - const named = reviewers.get(id); const log = logs[index]; - if (!log && named) { - const row = rows.get(named.number); - const { harness, models } = session; - if (row) row.sessions.push({ harness, session: id, role: "reviewing", effort: named.effort, models }); - else unattributed.push(session); - return; - } const branches = log ? [...log.branches].sort((a, b) => b[1].last - a[1].last) : []; let found: Pull | undefined; for (const [branch, seen] of branches) { @@ -563,10 +552,16 @@ function attribute( unattributed.push(session); return; } + const id = sessionId(session); row.sessions.push({ harness: session.harness, session: id, - role: log.review || named ? "reviewing" : log.started < found.created ? "implementing" : "iterating", + role: + log.review || reviewers.has(id) + ? "reviewing" + : log.started < found.created + ? "implementing" + : "iterating", effort: log.effort, models: session.models, }); @@ -763,11 +758,7 @@ export async function usage(root: string, options: UsageOptions, deps: UsageDepe let unattributed: Usage["unattributed"] = null; if (priced !== null) { // Every PR in the window competes for a session, so narrowing never hands one PR another's sessions. - const reviewers = new Map(); - for (const row of [...allRows].sort((a, b) => a.number - b.number)) - for (const review of row.review) - if (review.session !== null) - reviewers.set(review.session, { number: row.number, effort: review.effort }); + const reviewers = new Set(allRows.flatMap((row) => row.review.flatMap((review) => review.session ?? []))); const left = attribute(priced, sessionLogs(priced, deps), all, byNumber, reviewers); if (!narrowed) { unattributed = { sessions: left.length, ...zero() }; diff --git a/src/github/merge.ts b/src/github/merge.ts index 79f48fbe..c877ab3f 100644 --- a/src/github/merge.ts +++ b/src/github/merge.ts @@ -11,6 +11,13 @@ export function cleanAutoMergeRefusal(message: unknown): boolean { return typeof message === "string" && /\bpull request is in clean status\b/i.test(message); } +/** + * Whether a PR lacks an approval its base requires, by GitHub's `reviewDecision`. An admin's token may merge or enqueue + * past the requirement, so only auto-merge, which waits for it, may pick such a head. + */ +export const lacksApproval = (decision: unknown): boolean => + decision === "REVIEW_REQUIRED" || decision === "CHANGES_REQUESTED"; + /** Merge states in which GitHub may still be running required checks, so only auto-merge can wait for them. */ const PENDING = ["BLOCKED", "UNKNOWN"]; @@ -20,22 +27,29 @@ export type Mutate = (query: string, variables: Record) => Promi /** How {@link queueHead} put a head in the queue: enqueued it, or armed auto-merge to enqueue it once its checks pass. */ export type Queued = "enqueued" | "armed"; +/** The pull request {@link queueHead} queues, as GitHub last reported it. */ +export interface QueuePick { + /** The pull request's node ID. */ + id: string; + /** The head to queue, which must still be the pull request's. */ + head: string; + mergeState: string; + /** It lacks an approval its base requires ({@link lacksApproval}). */ + unapproved: boolean; + /** The auto-merge method GitHub requires, though the queue applies its own. */ + method: string; +} + /** - * Put `head`, which must still be the head of pull request `id`, in its base's merge queue: - * enqueue it, or, while GitHub may still be running its required checks (`mergeState` pending), - * arm auto-merge bound to it, which enqueues it once they pass. Either joins the back of the queue. - * GitHub refuses auto-merge for a PR it finished computing as ready, which is then enqueued. - * Enqueueing directly matters: GitHub often leaves an armed PR that is already ready out of the - * queue (https://github.com/orgs/community/discussions/209348). A refusal throws GitHub's error. - * `method` is the auto-merge method GitHub requires, though the queue applies its own. + * Put `pick.head` in its base's merge queue: enqueue it, or, while GitHub may still be running its required checks + * (`mergeState` pending) or it lacks a required approval, arm auto-merge bound to it, which enqueues it once they pass. + * Either joins the back of the queue. GitHub refuses auto-merge for a PR it finished computing as ready, which is then + * enqueued, unless it lacks the approval, which an admin's enqueue would bypass. Enqueueing directly matters: GitHub + * often leaves an armed PR that is already ready out of the queue + * (https://github.com/orgs/community/discussions/209348). A refusal throws GitHub's error. */ -export async function queueHead( - mutate: Mutate, - id: string, - head: string, - mergeState: string, - method: string, -): Promise { +export async function queueHead(mutate: Mutate, pick: QueuePick): Promise { + const { id, head, mergeState, method } = pick; const enqueue = async (): Promise => { await mutate( `mutation($id: ID!, $sha: GitObjectID!) { @@ -45,7 +59,7 @@ export async function queueHead( ); return "enqueued"; }; - if (!PENDING.includes(mergeState)) return enqueue(); + if (!pick.unapproved && !PENDING.includes(mergeState)) return enqueue(); try { await mutate( `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { @@ -56,7 +70,7 @@ export async function queueHead( return "armed"; } catch (error) { // An UNKNOWN state GitHub finished computing as ready: nothing is left to wait for. - if (!cleanAutoMergeRefusal(error instanceof Error ? error.message : null)) throw error; + if (pick.unapproved || !cleanAutoMergeRefusal(error instanceof Error ? error.message : null)) throw error; return enqueue(); } } diff --git a/src/github/models.ts b/src/github/models.ts index 43c83465..24e0efe3 100644 --- a/src/github/models.ts +++ b/src/github/models.ts @@ -1,7 +1,7 @@ // A pull request body's `## Models` record, which names the PR's size and each session that implemented it. import { SIZES } from "../core"; -import { REVIEWERS, SECOND_REVIEWER, WRITERS } from "../cli/models"; +import { REVIEWERS, WRITERS } from "../cli/models"; const SECTION = /^## Models[ \t]*\r?$/m; @@ -47,7 +47,7 @@ export function recorded(body: string): { } // Display names and IDs compare alike: `Opus 5.5`, `Claude Opus 5.5`, and `claude-opus-5-5` share a key, as do -// `DeepSeek V4.1 Flash` and `opencode-go/deepseek-v4.1-flash`. +// `GPT 6.1 Sol` and `openai/gpt-6.1-sol`. const key = (name: string) => name .toLowerCase() @@ -55,15 +55,13 @@ const key = (name: string) => .replace(/[\s.]+/g, "-") .replace(/^claude-/, ""); const KNOWN = new Map( - [...Object.values(WRITERS), ...Object.values(REVIEWERS), [SECOND_REVIEWER]] - .flat() - .map(({ model }) => [key(model), model]), + [...Object.values(WRITERS), ...Object.values(REVIEWERS)].flat().map(({ model }) => [key(model), model]), ); /** * The model ID `name` means: a listed model by its ID or display name, a Claude harness's ` ` as * its `claude-` ID, else `name` itself when it reads as an ID: one with a version digit, or, where the line's three - * fields place the model (`loose`), any lowercase token, such as `opencode-go/kimi`. + * fields place the model (`loose`), any lowercase token, such as `openrouter/kimi`. */ function modelId(name: string, harness: string, loose = false): string | null { const listed = KNOWN.get(key(name)); diff --git a/test/hold.test.ts b/test/hold.test.ts index e80b3016..03d2998f 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -35,6 +35,8 @@ class FakePull { body = "Summary."; head = HEAD; mergeState = "BLOCKED"; + /** GitHub's review decision, null when the base requires no review. */ + reviewDecision: string | null = null; queue = false; allowAutoMerge: boolean | null = true; /** The repository's merge method settings, hidden from the token when null. */ @@ -198,6 +200,7 @@ class FakePull { headRefName: "quest/m0/example", headRefOid: this.head, mergeStateStatus: this.mergeState, + reviewDecision: this.reviewDecision, isMergeQueueEnabled: this.queue, autoMergeRequest: this.autoMerge, mergeQueueEntry: this.queueEntry, @@ -855,3 +858,70 @@ test("iterate and release exit 0 when stdout returns a value", async () => { vi.unstubAllGlobals(); } }); + +describe("merge --land on a base that requires an approval the head lacks", () => { + // An admin's token sees such a PR as mergeable past the requirement, so only auto-merge may pick it. + test.each(["REVIEW_REQUIRED", "CHANGES_REQUESTED"])( + "arms auto-merge in a merge queue, never enqueueing (%s)", + async (decision) => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "CLEAN"; + pull.reviewDecision = decision; + await land(PR, { sha: HEAD, check: clean }, pull.deps()); + expect(pull.writes).toEqual([`enablePullRequestAutoMerge ${HEAD} MERGE`]); + pull.reviewDecision = "APPROVED"; + pull.autoMerge = null; + pull.writes.length = 0; + await land(PR, { sha: HEAD, check: clean }, pull.deps()); + expect(pull.writes).toEqual([`enqueuePullRequest ${HEAD}`]); + }, + ); + + test("never enqueues when GitHub refuses auto-merge as clean, which an admin's enqueue would bypass", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.reviewDecision = "REVIEW_REQUIRED"; + pull.beforeAutoMerge = () => "Pull request is in clean status"; + expect(await refused(land(PR, { sha: HEAD, check: clean }, pull.deps()))).toContain( + "GitHub refused to enqueue", + ); + // The refused attempt is all: no enqueue follows. + expect(pull.writes).toEqual([`enablePullRequestAutoMerge ${HEAD} MERGE`]); + }); + + test("decides on the last read, so an approval dismissed while it checks still holds", async () => { + const pull = new FakePull(); + pull.mergeState = "CLEAN"; + pull.reviewDecision = "APPROVED"; + pull.afterRead = () => { + pull.reviewDecision = "REVIEW_REQUIRED"; + }; + await land(PR, { sha: HEAD, method: "squash", check: clean }, pull.deps()); + expect(pull.writes).toEqual([`enablePullRequestAutoMerge ${HEAD} SQUASH`]); + expect(pull.state).toBe("OPEN"); + }); + + test("arms auto-merge rather than merging at once", async () => { + const pull = new FakePull(); + pull.mergeState = "CLEAN"; + pull.reviewDecision = "REVIEW_REQUIRED"; + await land(PR, { sha: HEAD, method: "squash", check: clean }, pull.deps()); + expect(pull.writes).toEqual([`enablePullRequestAutoMerge ${HEAD} SQUASH`]); + expect(pull.state).toBe("OPEN"); + }); + + test("refuses --direct with auto-merge off, which would merge past it", async () => { + const pull = new FakePull(); + pull.allowAutoMerge = false; + pull.mergeState = "CLEAN"; + pull.reviewDecision = "REVIEW_REQUIRED"; + expect(await refused(land(PR, { sha: HEAD, method: "squash", direct: true, check: clean }, pull.deps()))).toBe( + `${URL_} lacks the approval its base requires, which a merge now would bypass; land it once it is approved`, + ); + expect(pull.writes).toEqual([]); + pull.reviewDecision = "APPROVED"; + await land(PR, { sha: HEAD, method: "squash", direct: true, check: clean }, pull.deps()); + expect(pull.state).toBe("MERGED"); + }); +}); diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index c78224d2..02116825 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -59,6 +59,8 @@ interface Pull { state?: string; /** `mergeStateStatus`, CLEAN by default. */ mergeState?: string; + /** `reviewDecision`, null by default: the base requires no review. */ + reviewDecision?: string; /** Its base branch, main by default. */ base?: string; /** When it joined the merge queue, while it is in it. */ @@ -247,6 +249,7 @@ function deps(world: World): GitHubDependencies { baseRefName: pull.base ?? "main", headRefOid: headOf(world, pull), mergeStateStatus: pull.mergeState ?? "CLEAN", + reviewDecision: pull.reviewDecision ?? null, mergeQueueEntry: pull.queued ? { enqueuedAt: pull.queued } : null, autoMergeRequest: pull.armed ? { enabledAt: pull.armed } : null, labels: labelsOf(pull), @@ -290,6 +293,7 @@ function deps(world: World): GitHubDependencies { maintainerCanModify: false, headRepository: { nameWithOwner: "upstream/project" }, mergeStateStatus: pull.mergeState ?? "CLEAN", + reviewDecision: pull.reviewDecision ?? null, mergeQueueEntry: pull.queued ? { id: `MQE_${pull.number}` } : null, autoMergeRequest: pull.armed ? { enabledAt: pull.armed, commitBody: "" } : null, labels: labelsOf(pull), @@ -722,6 +726,8 @@ test("an armed pick GitHub never queued is enqueued once ready, only at the head armed(25), // In the queue already, or never armed. armed(26, { queued: at(3) }), + // It lacks an approval its base requires, which auto-merge waits for; an admin's enqueue would bypass it. + armed(28, { reviewDecision: "REVIEW_REQUIRED" }), { number: 27, head: HEAD, enqueues: [], removals: [] }, ]); github.unlisted = [25]; diff --git a/test/live/review-sandbox.test.ts b/test/live/review-sandbox.test.ts index 44373cac..4da233fd 100644 --- a/test/live/review-sandbox.test.ts +++ b/test/live/review-sandbox.test.ts @@ -1,5 +1,5 @@ -// Live: `quest review` with the real Codex, Claude Code, and opencode CLIs runs -// nothing a reviewed head plants, and trusts nothing new. It spends the logins' usage, so +// Live: `quest review` with the real Codex and Claude Code CLIs runs nothing a +// reviewed head plants, and trusts nothing new. It spends the logins' usage, so // only `just review-sandbox` runs it, never `just test` or CI. // // The fake-CLI tests in `test/review.test.ts` check only that quest passes the @@ -26,33 +26,15 @@ import { join } from "node:path"; import { createInterface } from "node:readline"; import { afterAll, beforeAll, describe, expect, test } from "vitest"; -import { strippedEnvironment } from "../../src/cli/agent"; +import { type HarnessName, strippedEnvironment } from "../../src/cli/agent"; import { DEFAULT_SIZE, REVIEWERS } from "../../src/cli/models"; -import { - type ReviewHarness, - ReviewInputError, - claudeFlags, - opencodeTurn, - review, - reviewerEnvironment, - schema, -} from "../../src/cli/review"; -import { openSandbox } from "../../src/cli/sandbox"; - -/** - * opencode's model, which no size picks: QUEST_LIVE_OPENCODE_MODEL, as - * provider/model#variant, or a cheap one OpenCode Go serves. - */ -const [OPENCODE_MODEL, OPENCODE_EFFORT] = ( - process.env.QUEST_LIVE_OPENCODE_MODEL ?? "opencode-go/deepseek-v4.1-flash#high" -).split("#"); +import { ReviewInputError, claudeFlags, review, reviewerEnvironment, schema } from "../../src/cli/review"; /** The model a sizeless review runs on `harness`. */ -const modelOf = (harness: ReviewHarness) => - harness === "opencode" ? OPENCODE_MODEL : REVIEWERS[DEFAULT_SIZE].find((entry) => entry.harness === harness)!.model; +const modelOf = (harness: HarnessName) => REVIEWERS[DEFAULT_SIZE].find((entry) => entry.harness === harness)!.model; -/** The cheapest effort each CLI accepts; the isolation under test does not depend on it. */ -const effortOf = (harness: ReviewHarness) => (harness === "opencode" ? OPENCODE_EFFORT : "low"); +/** The cheapest effort both CLIs accept; the isolation under test does not depend on it. */ +const EFFORT = "low"; interface Fixture { /** @@ -87,7 +69,7 @@ beforeAll(() => { git(clone, "checkout", "-qb", "feature"); for (const [path, text] of Object.entries(plants(clone, markers))) write(clone, path, text); - for (const script of [SHELL_PREFIX, OPENCODE_SHELL]) chmodSync(join(clone, script), 0o755); + chmodSync(join(clone, SHELL_PREFIX), 0o755); write( clone, "src/lib.ts", @@ -115,8 +97,6 @@ const allowed = (markers: string) => `touch ${join(markers, "claude-permissions" * shared settings also enable every project MCP server, allow a command in * the markers' directory, route every Bash command through a planted script * by an `env` variable, and set an API key helper and a status line. - * opencode's project config names an MCP server, a plugin, and a shell, and - * its project directory holds another plugin. */ function plants(clone: string, markers: string): Record { const touch = (name: string) => `touch ${JSON.stringify(join(markers, name))}`; @@ -130,8 +110,6 @@ function plants(clone: string, markers: string): Record { ), }); const server = (name: string) => ({ command: "/bin/sh", args: ["-c", touch(name)] }); - const plugin = (name: string) => - `import { writeFileSync } from "node:fs";\nexport default { id: ${JSON.stringify(name)}, setup: async () => writeFileSync(${JSON.stringify(join(markers, name))}, "") };\n`; return { ".codex/config.toml": [ "[mcp_servers.plant]", @@ -152,21 +130,9 @@ function plants(clone: string, markers: string): Record { ".claude/settings.local.json": hooks("claude-local"), // Claude Code passes the command as one argument or several, so both run. [SHELL_PREFIX]: `#!/bin/sh\n${touch("claude-shell-prefix")}\nexec /bin/sh -c "$*"\n`, - "opencode.json": JSON.stringify({ - mcp: { servers: { plant: { type: "local", command: ["/bin/sh", "-c", touch("opencode-mcp")] } } }, - plugins: [{ package: join(clone, "opencode-plugin") }], - shell: join(clone, OPENCODE_SHELL), - }), - "opencode-plugin/package.json": '{"type":"module"}\n', - "opencode-plugin/server.js": plugin("opencode-plugin"), - ".opencode/plugins/plant.js": plugin("opencode-project-plugin"), - [OPENCODE_SHELL]: `#!/bin/sh\n${touch("opencode-shell")}\nexec /bin/sh "$@"\n`, }; } -/** The planted script opencode's planted config runs every command through. */ -const OPENCODE_SHELL = "bin/opencode-shell.sh"; - /** * What each harness's own plants record when its CLI loads the head's * configuration. Claude Code draws a status line only in an interactive @@ -174,7 +140,7 @@ const OPENCODE_SHELL = "bin/opencode-shell.sh"; * runs it in print mode too. Its API key helper runs only beside a credential * from the environment, which its own test supplies. */ -const CONTROL: Record = { +const CONTROL: Record = { codex: ["codex-config-mcp", "codex-hooks-SessionStart", "codex-hooks-UserPromptSubmit"], claude: [ "claude-local-SessionStart", @@ -185,7 +151,6 @@ const CONTROL: Record = { "claude-shell-prefix", "mcp-json", ], - opencode: ["opencode-mcp", "opencode-plugin", "opencode-project-plugin", "opencode-shell"], }; /** @@ -195,7 +160,7 @@ const CONTROL: Record = { * hide another's. */ interface Home { - harness: ReviewHarness; + harness: HarnessName; /** The variables pointing the CLI at it. */ env: Record; /** The directories the home trusts. */ @@ -204,11 +169,8 @@ interface Home { release(): void; } -async function home(harness: ReviewHarness, trust = true): Promise { +async function home(harness: HarnessName, trust = true): Promise { const { clone } = fixture; - // opencode has no trust to grant, and its login shares a database with every session, so it runs on - // the user's own, as `quest review` does. - if (harness === "opencode") return { harness, env: {}, trusted: () => [], release: () => {} }; const path = newHome(); if (harness === "codex") { const source = process.env.CODEX_HOME || join(homedir(), ".codex"); @@ -322,16 +284,15 @@ function environment(home: Home): NodeJS.ProcessEnv { } /** What a credential variable names when there is no login to copy. */ -const CREDENTIALS: Record = { +const CREDENTIALS: Record = { codex: "CODEX_API_KEY", claude: "CLAUDE_CODE_OAUTH_TOKEN (from `claude setup-token`) or ANTHROPIC_API_KEY", - opencode: "`opencode auth login` to a provider of QUEST_LIVE_OPENCODE_MODEL", }; /** The environment `quest review` runs the reviewer in, with the copied login or else the user's credentials. */ async function reviewerIn(home: Home): Promise { try { - return await reviewerEnvironment(home.harness, modelOf(home.harness), fixture.clone, environment(home)); + return await reviewerEnvironment(home.harness, fixture.clone, environment(home)); } catch (error) { if (!(error instanceof ReviewInputError)) throw error; throw new Error( @@ -358,22 +319,15 @@ function clearMarkers() { */ async function control(home: Home, extra: NodeJS.ProcessEnv = {}) { const model = modelOf(home.harness); - const effort = effortOf(home.harness); - // Without quest's variables, opencode loads the checkout's config. - const env: NodeJS.ProcessEnv = { ...(await reviewerIn(home)), ...extra }; - // opencode runs in PWD, not its working directory. - if (home.harness === "opencode") env.PWD = fixture.clone; - for (const name of Object.keys(env)) if (name.startsWith("OPENCODE_DISABLE_")) delete env[name]; - const argv = { - codex: ["exec", "--json", "-m", model, "-c", `model_reasoning_effort="${effort}"`, "-"], - claude: ["-p", "--output-format", "json", "--model", model, "--effort", effort], - opencode: ["run", "--standalone", "--format", "json", "--archived", "--model", `${model}#${effort}`], - }[home.harness]; - const input = { - codex: "Reply with the single word OK.", - claude: `Run \`${allowed(fixture.markers)}\` with the Bash tool, then reply with the single word OK.`, - opencode: "Run `true` with your shell tool, then reply with the single word OK.", - }[home.harness]; + const env = { ...(await reviewerIn(home)), ...extra }; + const argv = + home.harness === "codex" + ? ["exec", "--json", "-m", model, "-c", `model_reasoning_effort="${EFFORT}"`, "-"] + : ["-p", "--output-format", "json", "--model", model, "--effort", EFFORT]; + const input = + home.harness === "codex" + ? "Reply with the single word OK." + : `Run \`${allowed(fixture.markers)}\` with the Bash tool, then reply with the single word OK.`; return spawnSync(home.harness, argv, { cwd: fixture.clone, env, @@ -385,7 +339,7 @@ async function control(home: Home, extra: NodeJS.ProcessEnv = {}) { const tail = (text: string | null) => (text ?? "").trim().split("\n").slice(-10).join("\n"); -describe.each(["codex", "claude", "opencode"] as const)("%s", (harness) => { +describe.each(["codex", "claude"] as const)("%s", (harness) => { test("loads the planted configuration without quest's flags", async () => { clearMarkers(); const dir = await home(harness); @@ -422,7 +376,7 @@ describe.each(["codex", "claude", "opencode"] as const)("%s", (harness) => { mergeBase: "origin/main", instructions: "Do what the user asks.", model: modelOf("claude"), - effort: effortOf("claude"), + effort: EFFORT, schema: schema(0), schemaFile: "", }; @@ -475,7 +429,7 @@ describe.each(["codex", "claude", "opencode"] as const)("%s", (harness) => { mergeBase: "origin/main", instructions: "Do what the user asks.", model: modelOf("claude"), - effort: effortOf("claude"), + effort: EFFORT, schema: schema(0), schemaFile: "", }; @@ -494,68 +448,6 @@ describe.each(["codex", "claude", "opencode"] as const)("%s", (harness) => { expect(recorded(), `planted configuration ran\n${output}`).toEqual([]); }); - // The sandbox's boundary, checked with commands run through its shell directly, and the reviewer's use - // of that shell, checked by a command that reports whether it ran under srt. - test.runIf(harness === "opencode")("quest's opencode shell runs commands read-only and offline", async () => { - clearMarkers(); - const env = await reviewerIn(await home(harness)); - const sandbox = openSandbox([fixture.clone, join(fixture.clone, ".git")], env); - const secret = join(homedir(), ".quest-live-secret"); - writeFileSync(secret, "SECRET-IN-HOME\n"); - const sh = (command: string) => - spawnSync(sandbox.shell, ["-c", command], { cwd: fixture.clone, encoding: "utf8", timeout: 60_000 }); - let run: SpawnSyncReturns; - try { - for (const path of [join(fixture.clone, "opencode-write"), join(fixture.markers, "opencode-write")]) { - sh(`touch ${path}`); - expect(existsSync(path), `a command wrote ${path}`).toBe(false); - } - const read = sh(`cat ${secret}`); - expect(read.stdout, "a command read the user's home").not.toContain("SECRET-IN-HOME"); - const fetched = sh("curl -sS -m 10 https://example.com"); - expect(fetched.stdout, "a command reached the network").not.toContain("Example Domain"); - const logged = sh("git log --oneline -1"); - expect(logged.stdout, `git could not read the checkout: ${logged.stderr}`).toContain("change"); - - const instructions = join(sandbox.dir, "instructions.md"); - writeFileSync(instructions, "Do what the user asks."); - const context = { - mergeBase: "origin/main", - instructions: "Do what the user asks.", - model: modelOf(harness), - effort: effortOf(harness), - schema: schema(0), - schemaFile: "", - sandbox: { shell: sandbox.shell, instructions }, - }; - const turn = opencodeTurn( - context, - [], - "Run `echo sandboxed=$SANDBOX_RUNTIME` with your shell tool, then reply with its output.", - ); - run = spawnSync(turn.argv[0], turn.argv.slice(1), { - cwd: fixture.clone, - env: { ...env, ...turn.env, PWD: fixture.clone }, - input: turn.stdin, - encoding: "utf8", - timeout: 10 * 60_000, - }); - } finally { - sandbox.close(); - rmSync(secret, { force: true }); - } - const output = `exit ${run.status}\nstdout:\n${tail(run.stdout)}\nstderr:\n${tail(run.stderr)}`; - const said = run.stdout - .split("\n") - .filter(Boolean) - .map((line) => JSON.parse(line) as { type?: string; part?: { tool?: string; state?: { output?: string } } }) - .filter((event) => event.type === "tool_use" && event.part?.tool === "shell") - .map((event) => event.part?.state?.output ?? ""); - expect(said, `the reviewer's command did not run under srt\n${output}`).toContainEqual( - expect.stringContaining("sandboxed=1"), - ); - }); - // A home that already trusts the clone would not show the review trusting it. test("quest review of an untrusted checkout trusts nothing", async () => { const dir = await home(harness, false); @@ -585,14 +477,7 @@ async function reviewIn(dir: Home, verify: () => void) { let outcome: unknown; try { outcome = await review( - { - root: fixture.clone, - base: "origin/main", - harness: dir.harness, - model: modelOf(dir.harness), - effort: effortOf(dir.harness), - fresh: true, - }, + { root: fixture.clone, base: "origin/main", harness: dir.harness, effort: EFFORT, fresh: true }, { env: environment(dir) }, ).catch((error: unknown) => error); } finally { diff --git a/test/review.test.ts b/test/review.test.ts index b4ff46ab..edd78467 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -177,61 +177,6 @@ if (argv[1] === "review") { message(reply); event({ type: "turn.completed", usage: { input_tokens: 100, cached_input_tokens: 80, output_tokens: 7 } }); } -`, - // Logs the config each turn gets, with its instructions file's text, which is gone once the round ends, apart - // from the other CLIs, since it reviews beside them. Its own FAKE_OPENCODE_* variables fail it: LOGGED_OUT lists - // only a free model, REJECTED fails a turn on authentication, and LIMITED has the findings turn retry a spent - // limit until it is stopped. REPLY replaces FAKE_REPLY for it alone. The findings reply comes in a code fence. - opencode: ` -const fs = require("node:fs"); -const argv = process.argv.slice(2); -const stdin = fs.readFileSync(0, "utf8"); -const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(OPENCODE|PWD$)/.test(k))); -const config = process.env.OPENCODE_CONFIG_CONTENT && JSON.parse(process.env.OPENCODE_CONFIG_CONTENT); -const instructions = config && fs.readFileSync(config.instructions[0], "utf8"); -fs.appendFileSync(process.env.FAKE_LOG + ".opencode", JSON.stringify({ argv, stdin, env, config, instructions, cwd: process.cwd() }) + "\\n"); -if (argv[0] === "models") { - console.log("opencode/free"); - if (!process.env.FAKE_OPENCODE_LOGGED_OUT) console.log("opencode-go/deepseek-v4.1-flash\\nopencode-go/kimi"); - process.exit(0); -} -const session = argv.includes("--session") ? argv[argv.indexOf("--session") + 1] : "ses_1"; -const event = (type, fields) => console.log(JSON.stringify({ type, timestamp: 1900000000000, sessionID: session, ...fields })); -if (process.env.FAKE_OPENCODE_REJECTED) { - event("error", { error: { type: "provider.auth", message: "Invalid API key" } }); - process.exit(1); -} -if (process.env.FAKE_OPENCODE_LIMITED && argv.includes("--session")) { - const body = process.env.FAKE_OPENCODE_LIMITED === "unknown" ? "{}" : JSON.stringify({ resetsAt: "2030-03-17T17:46:40Z" }); - event("retry", { attempt: 1, error: { type: "provider.go-limit", message: "Usage limit reached", response: { body } } }); - setInterval(() => {}, 1000); - return; -} -const tokens = (input, read, output) => ({ part: { type: "step-finish", tokens: { input, output, reasoning: 1, cache: { read, write: 0 } } } }); -event("step_start", { part: { type: "step-start" } }); -if (!argv.includes("--session")) { - event("text", { part: { type: "text", text: "Found an off-by-one." } }); - event("step_finish", tokens(10, 0, 5)); -} else { - event("text", { part: { type: "text", text: "Checking the findings." } }); - event("tool_use", { part: { type: "tool", tool: "shell", state: { status: "completed" } } }); - event("step_finish", tokens(10, 5, 2)); - if (process.env.FAKE_OPENCODE_REPLY) process.env.FAKE_REPLY = process.env.FAKE_OPENCODE_REPLY; - ${REPLY} - event("text", { part: { type: "text", text: "\`\`\`json\\n" + reply.slice(0, 20) } }); - event("text", { part: { type: "text", text: reply.slice(20) + "\\n\`\`\`" } }); - event("step_finish", tokens(20, 15, 3)); -} -`, - // sandbox-runtime, whose seccomp filter refuses the Unix socket its probe makes. - srt: ` -const { execFileSync } = require("node:child_process"); -const command = process.argv[process.argv.indexOf("-c") + 1]; -if (command.includes("socat")) { - console.error("2026/11/09 socat[1] E socket(1, 1, 0): Operation not permitted"); - process.exit(1); -} -execFileSync("/bin/sh", ["-c", command], { stdio: "inherit" }); `, }; @@ -250,8 +195,6 @@ interface Setup { bin: string; /** Every logged invocation of the fake Codex and Claude Code. */ calls(): Call[]; - /** Every logged invocation of the fake opencode. */ - opencodeCalls(): Call[]; } interface Call { @@ -259,9 +202,6 @@ interface Call { stdin: string; env: Record; cwd: string; - // opencode's config, and the text of its instructions file. - config?: { shell: string; permissions: unknown }; - instructions?: string; } /** Credentials and routes the running session may hold, cleared so each test sets its own. */ @@ -317,23 +257,12 @@ function setup(host: "CLAUDECODE" | "CODEX_THREAD_ID" | null = "CLAUDECODE"): Se 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", - "XDG_DATA_HOME", - ]; + const unset = [...HOST_CREDENTIALS, "CLAUDECODE", "CODEX_THREAD_ID", "CLAUDE_CONFIG_DIR", "CODEX_HOME"]; for (const name of [ ...unset, "FAKE_LOGGED_OUT", "FAKE_REJECTED", "FAKE_LIMITED", - "FAKE_OPENCODE_LOGGED_OUT", - "FAKE_OPENCODE_REJECTED", - "FAKE_OPENCODE_LIMITED", - "FAKE_OPENCODE_REPLY", "FAKE_HANG", "FAKE_DISPOSITION", ]) { @@ -348,7 +277,7 @@ function setup(host: "CLAUDECODE" | "CODEX_THREAD_ID" | null = "CLAUDECODE"): Se .split("\n") .map((line) => JSON.parse(line)) : []; - return { repo, bin, calls: () => read(log), opencodeCalls: () => read(`${log}.opencode`) }; + return { repo, bin, calls: () => read(log) }; } /** The effort a Codex or Claude Code turn runs at, or null for a status check. */ @@ -454,8 +383,6 @@ fakes("a quest branch is reviewed at its quest's size on the base, which its own expect(result.stderr).toBe(""); expect(result.stdout).toMatch(/^size L quest\/m0\/big.md\n/); expect(result.stdout).toContain("\nreviewer codex gpt-6.1-sol xhigh\n"); - // The second reviewer runs at its own effort, whatever the size. - expect(result.stdout).toContain("\nreviewer opencode opencode-go/deepseek-v4.1-flash high\n"); // --size overrides the quest's. const sized = await quest("--root", s.repo, "review", "--size", "S", "--new"); @@ -487,7 +414,7 @@ fakes("--exclude-model picks the reviewer from the PR's implementers instead of const s = setup(null); const result = await quest("--root", s.repo, "review", "--exclude-model", "gpt-6.1-sol", "--json"); expect(result.stderr).toBe(""); - const [round, second] = JSON.parse(result.stdout).rounds; + const [round] = JSON.parse(result.stdout).rounds; expect([round.reviewer, round.model, round.effort, round.size, round.sizeFrom]).toEqual([ "claude", "claude-opus-5-5", @@ -495,16 +422,11 @@ fakes("--exclude-model picks the reviewer from the PR's implementers instead of "M", "default", ]); - expect(second.model).toBe("opencode-go/deepseek-v4.1-flash"); const both = ["--exclude-model", "gpt-6.1-sol", "--exclude-model", "claude-opus-5-5"]; const none = await quest("--root", s.repo, "review", ...both, "--new"); expect(none.code).toBe(2); expect(none.stderr).toContain("every M reviewer (gpt-6.1-sol, claude-opus-5-5) is avoided"); - // Nor does the second reviewer review a change it is named as writing. - const deepseek = await quest("--root", s.repo, "review", "--exclude-model", "opencode-go/deepseek-v4.1-flash"); - expect(deepseek.code).toBe(2); - expect(deepseek.stderr).toContain("opencode-go/deepseek-v4.1-flash is avoided; pass --harness"); }); fakes("an unknown harness is an error, never a same-model review", async () => { @@ -927,24 +849,6 @@ fakes("a detached checkout continues the session of the branch --branch names", expect(at(s.calls(), "high").filter((c) => c.argv[1] === "review")).toHaveLength(1); }); -fakes("another worktree resumes Codex's session, and starts opencode's anew in its own checkout", async () => { - const s = setup(); - vi.stubEnv("FAKE_OPENCODE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); - expect((await quest("--root", s.repo, "review")).code).toBe(0); - const other = join(s.repo, ".tmp", "other"); - git(s.repo, "worktree", "add", "-q", "--detach", other); - write(other, ".tmp/responses.md", "1. Refuted: src/lib.ts:1 sets it.\n"); - const responses = join(other, ".tmp", "responses.md"); - const round = await quest("--root", other, "review", "--branch", "feature", "--responses", responses); - expect(round.stderr).toBe(""); - const [picked, deepseek] = round.stdout.split("\nreviewer opencode "); - expect(picked).toContain("session thread-1\nround 2\n"); - expect(deepseek).toContain("round 1\n"); - const runs = s.opencodeCalls().filter((call) => call.argv[0] === "run"); - expect(runs.at(-2)!.argv).not.toContain("--session"); - expect(runs.at(-1)!.cwd).toBe(other); -}); - fakes("a differently named branch continues the session --branch names, and without it has its own", async () => { const s = setup(); expect((await quest("--root", s.repo, "review")).stdout).toContain("round 1\n"); @@ -1038,18 +942,6 @@ fakes("codex reviews read-only with the base's instructions, then reports in the "1. high src/lib.ts:2 Off by one", " average([3]) is 0.", "", - // The second reviewer reviews the same head, its findings numbered after the first's. - "reviewer opencode opencode-go/deepseek-v4.1-flash high", - "session ses_1", - "round 1", - "usage review input 10 cached 0 output 6", - "usage findings input 50 cached 20 output 7", - "score 8 Lands once the off-by-one is fixed.", - "findings 1", - "", - "2. high src/lib.ts:2 Off by one", - " average([3]) is 0.", - "", // The shadow, the pick one effort lower, reviews it too, in its own session. "reviewer codex gpt-6.1-sol medium shadow", "session thread-1", @@ -1058,7 +950,7 @@ fakes("codex reviews read-only with the base's instructions, then reports in the "score 8 Lands once the off-by-one is fixed.", "findings 1", "", - "3. high src/lib.ts:2 Off by one", + "2. high src/lib.ts:2 Off by one", " average([3]) is 0.", // The fixture's origin is a local path, which can have no PR. "record skipped: origin is not on GitHub", @@ -1105,9 +997,6 @@ fakes("codex reviews read-only with the base's instructions, then reports in the const replySchema = JSON.parse(readFileSync(schema, "utf8")); expect(replySchema.required).toEqual(["score", "rationale", "findings"]); expect(replySchema.properties.earlier).toBeUndefined(); - const deepseek = s.opencodeCalls().filter((call) => call.argv[0] === "run"); - expect(deepseek).toHaveLength(2); - for (const call of deepseek) expect(call.argv).toContain("opencode-go/deepseek-v4.1-flash#high"); }); fakes("--base reviews against another branch, fetched first, with that branch's instructions", async () => { @@ -1178,107 +1067,6 @@ fakes("claude reviews read-only with the base's instructions, then reports in th } }); -const OPENCODE = ["review", "--harness", "opencode", "--model", "opencode-go/kimi", "--effort", "high"]; - -fakes( - "opencode reviews with only its sandboxed shell and the base's instructions, then reports in the schema", - async () => { - const s = setup(); - const result = await quest("--root", s.repo, ...OPENCODE, "--json"); - expect(result.code, result.stderr).toBe(0); - const [round] = JSON.parse(result.stdout).rounds; - expect(round.reviewer).toBe("opencode"); - expect(round.session).toBe("ses_1"); - expect(round.usage).toEqual([ - { turn: "review", input: 10, cached: 0, output: 6 }, - { turn: "findings", input: 50, cached: 20, output: 7 }, - ]); - expect(round.findings).toEqual([FINDING]); - - const [status, first, second] = s.opencodeCalls(); - expect(status.argv).toEqual(["models", "--standalone"]); - expect(first.stdin).toMatch(/^Review this change for bugs/); - expect(first.stdin).toContain("BASE-REVIEW rule"); - expect(first.argv).not.toContain("--session"); - expect(second.argv.slice(-2)).toEqual(["--session", "ses_1"]); - for (const call of [status, first, second]) { - expect(call.env.OPENCODE_DISABLE_PROJECT_CONFIG).toBe("1"); - expect(call.env.OPENCODE_DISABLE_CLAUDE_CODE).toBe("1"); - // opencode runs in PWD, not its working directory. - expect(call.env.PWD).toBe(s.repo); - } - for (const call of [first, second]) { - expect(call.argv.slice(0, 8)).toEqual( - ["run", "--standalone", "--format", "json", "--archived", "--model", "opencode-go/kimi#high"].concat( - call === first ? [] : ["--session"], - ), - ); - expect(call.config!.permissions).toEqual([ - { action: "*", resource: "*", effect: "deny" }, - { action: "skill", resource: "*", effect: "deny" }, - { action: "shell", resource: "*", effect: "allow" }, - ]); - expect(call.config).toMatchObject({ lsp: false, formatter: false, snapshot: false, share: "disabled" }); - expect(call.instructions).toContain("BASE-ROOT rule"); - expect(call.instructions).not.toContain("HEAD-ROOT rule"); - // The sandbox is gone once the round ends. - expect(existsSync(call.config!.shell)).toBe(false); - } - }, -); - -fakes("opencode names no default model, so --harness opencode needs --model and --effort", async () => { - const s = setup(); - const result = await quest("--root", s.repo, "review", "--harness", "opencode"); - expect(result.code).toBe(2); - expect(result.stderr).toContain("no opencode reviewer for size M; pass --model and --effort"); -}); - -fakes("opencode counts as logged out when it lists no such model, or its provider rejects the login", async () => { - const s = setup(); - vi.stubEnv("FAKE_OPENCODE_LOGGED_OUT", "1"); - const out = await quest("--root", s.repo, ...OPENCODE); - expect(out.code).toBe(2); - expect(out.stderr).toContain( - "opencode lists no model opencode-go/kimi for its default profile (XDG_DATA_HOME is unset); run `opencode auth login` for its provider", - ); - vi.stubEnv("FAKE_OPENCODE_LOGGED_OUT", undefined); - vi.stubEnv("FAKE_OPENCODE_REJECTED", "1"); - const rejected = await quest("--root", s.repo, ...OPENCODE); - expect(rejected.code).toBe(2); - expect(rejected.stderr).toContain( - "opencode is not logged in to its default profile (XDG_DATA_HOME is unset); run `opencode auth login`", - ); -}); - -fakes.for([ - ["reset", "opencode hit its usage limit; resets 2030-03-17T17:46:40Z"], - ["unknown", "opencode hit its usage limit; resets unknown"], -] as const)("opencode stops retrying a spent usage limit and exits 3 (%s)", async ([variant, message]) => { - const s = setup(); - vi.stubEnv("FAKE_OPENCODE_LIMITED", variant); - const result = await quest("--root", s.repo, ...OPENCODE); - expect(result.stderr).toBe(`error: ${message}\n`); - expect(result.code).toBe(3); -}); - -test.runIf(process.platform === "linux")("opencode refuses an srt that lets commands make Unix sockets", async () => { - const s = setup(); - const unfiltered = join(s.bin, "..", "unfiltered"); - mkdirSync(unfiltered); - writeFileSync( - join(unfiltered, "srt"), - `#!/bin/sh\necho 'socat[1] E connect(5, AF=1 "/tmp/missing", 15): No such file or directory' >&2\nexit 1\n`, - ); - chmodSync(join(unfiltered, "srt"), 0o755); - vi.stubEnv("PATH", `${unfiltered}:${process.env.PATH}`); - const result = await quest("--root", s.repo, ...OPENCODE); - expect(result.code).toBe(2); - expect(result.stderr).toContain("lets commands make Unix sockets"); - // Only the status check ran. - expect(s.opencodeCalls()).toHaveLength(1); -}); - fakes("a later round resumes the session with the new head and the author's responses", async () => { const s = setup(); expect((await quest("--root", s.repo, "review")).code).toBe(0); @@ -1301,21 +1089,13 @@ fakes("a later round resumes the session with the new head and the author's resp expect(later.stdout).toContain( `earlier 1 ${first}\n\n1. fixed high src/lib.ts:2 Off by one\n Checked against the head.\n`, ); - // The second reviewer's finding came after the first's, so it keeps that number. + // The shadow had a finding to settle, so it reruns, settling its own after the pick's. expect(later.stdout).toContain( `earlier 1 ${first}\n\n2. fixed high src/lib.ts:2 Off by one\n Checked against the head.\n`, ); - const deepseek = s.opencodeCalls().at(-1)!; - expect(deepseek.argv).toContain("--session"); - expect(deepseek.stdin).toContain("\n2. high src/lib.ts:2 Off by one\n"); - expect(deepseek.stdin).toContain("Fixed at src/lib.ts:1."); - // The shadow had a finding to settle, so it reruns, settling its own after the second reviewer's. - expect(later.stdout).toContain( - `earlier 1 ${first}\n\n3. fixed high src/lib.ts:2 Off by one\n Checked against the head.\n`, - ); const shadow = at(s.calls(), "medium").at(-1)!; expect(shadow.argv.slice(0, 2)).toEqual(["exec", "resume"]); - expect(shadow.stdin).toContain("\n3. high src/lib.ts:2 Off by one\n"); + expect(shadow.stdin).toContain("\n2. high src/lib.ts:2 Off by one\n"); const calls = at(s.calls(), "high"); const resumed = calls.at(-1)!; @@ -1348,10 +1128,9 @@ fakes("--new starts over, and a first round takes no responses", async () => { JSON.parse(restarted.stdout).tallies.map((t: Tally) => [t.reviewer, t.rounds, t.findings.high, t.fixed]), ).toEqual([ ["codex", 2, 1, 0], - ["opencode", 2, 1, 0], ["codex", 2, 1, 0], ]); - expect(JSON.parse(restarted.stdout).tallies.map((t: Tally) => t.shadow)).toEqual([undefined, undefined, true]); + expect(JSON.parse(restarted.stdout).tallies.map((t: Tally) => t.shadow)).toEqual([undefined, true]); }); /** Commit `edit` to origin's `next` branch, leaving the checkout on `feature`; no flags change. */ @@ -1388,10 +1167,8 @@ fakes.each([ const base = flags.includes("--base") ? [] : ["--base", "origin/next"]; const round = await quest("--root", s.repo, "review", ...base, ...flags); expect(round.code, round.stderr).toBe(0); - // --effort sets only the size pick's, so only the second reviewer may repeat its round. - const [picked] = round.stdout.split("\nreviewer opencode "); - expect(picked).toContain("round 1\n"); - expect(picked).not.toContain("unchanged"); + expect(round.stdout).toContain("round 1\n"); + expect(round.stdout).not.toContain("unchanged"); const effort = flags.includes("--effort") ? "xhigh" : "high"; expect(at(s.calls(), effort).at(-2)!.argv[1]).toBe("review"); // A pick at another effort leaves the shadow two steps below it, so the shadow at one step reruns. @@ -1525,13 +1302,6 @@ fakes("each round rewrites the Review section of the branch's open PR, keeping t "- Findings, open until the next round settles them:", " - High, `src/lib.ts:2`: `Off by one`", "", - "- Reviewer: opencode, opencode-go/deepseek-v4.1-flash, high effort", - "- Session: `ses_1`, round 1", - "- Score: 8/10, `Lands once the off-by-one is fixed.`", - "- Rounds: 1; findings 1 (high 1): fixed 0, refuted 0, open 1", - "- Findings, open until the next round settles them:", - " - High, `src/lib.ts:2`: `Off by one`", - "", // The shadow runs the pick one effort lower, and its findings count apart from the pick's. "- Reviewer: Codex, gpt-6.1-sol, low effort, shadow", "- Session: `thread-1`, round 1", @@ -1631,40 +1401,33 @@ fakes("a round on the head the last round passed runs no reviewer and repeats th const again = await quest("--root", s.repo, "review", "--json", "--size", "S"); expect(again.code, again.stderr).toBe(0); expect(s.calls()).toHaveLength(calls); - expect(s.opencodeCalls()).toHaveLength(3); // The shadow found nothing on the branch's first run, so it never runs again. const all: Round[] = JSON.parse(first.stdout).rounds; expect(all.map((round) => [round.effort, round.shadow])).toEqual([ ["medium", undefined], - ["high", undefined], ["low", true], ]); - const rounds = all.slice(0, 2); + const rounds = all.slice(0, 1); expect(JSON.parse(again.stdout).rounds).toEqual( rounds.map((round: object) => ({ ...round, usage: [], unchanged: true })), ); - // XS picks the same reviewers as S, so its rounds repeat and report the size given. + // XS picks the same reviewer as S, so its round repeats and reports the size given. const resized = await quest("--root", s.repo, "review", "--json", "--size", "XS"); expect(s.calls()).toHaveLength(calls); expect(JSON.parse(resized.stdout).rounds).toMatchObject(rounds.map(() => ({ size: "XS", sizeFrom: "--size" }))); const repeated = (await quest("--root", s.repo, "review", "--size", "XS")).stdout; - expect(repeated.split("unchanged round 1 reviewed this head and found nothing; no reviewer ran\n")).toHaveLength(3); + expect(repeated.split("unchanged round 1 reviewed this head and found nothing; no reviewer ran\n")).toHaveLength(2); - // A round with findings to settle runs on the same head, since a refutation needs no new commit, while a - // reviewer with none repeats its round. + // A round with findings to settle runs on the same head, since a refutation needs no new commit. vi.stubEnv("FAKE_REPLY", undefined); const t = setup(); - vi.stubEnv("FAKE_OPENCODE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); expect((await quest("--root", t.repo, "review")).code).toBe(0); - const opencode = t.opencodeCalls().length; write(t.repo, ".tmp/responses.md", "1. Refuted: src/lib.ts:1 sets it.\n"); const refuted = await quest("--root", t.repo, "review", "--responses", join(t.repo, ".tmp", "responses.md")); expect(refuted.code, refuted.stderr).toBe(0); - const [picked, second] = refuted.stdout.split("\nreviewer opencode "); - expect(picked).toContain("round 2\n"); - expect(second).toContain("unchanged round 1"); + expect(refuted.stdout).toContain("session thread-1\nround 2\n"); + expect(refuted.stdout).not.toContain("unchanged"); expect(at(t.calls(), "high").at(-1)!.stdin).toContain("Refuted: src/lib.ts:1 sets it."); - expect(t.opencodeCalls()).toHaveLength(opencode); }); fakes("a later round checks only what changed since the reviewed head, and the open findings", async () => { @@ -1803,7 +1566,7 @@ test("an Implementer line reads as agents write it", () => { expect(read("Claude Code, Opus 5.5, high effort (iterate, XS, arm B)")).toEqual({ ...opus("high"), size: "XS" }); expect(read("unknown, Opus 5.5, high effort")).toEqual(opus("high", "unknown")); expect(read("Claude Code, Fable 5.2, medium effort")?.model).toBe("claude-fable-5-2"); - expect(read("opencode, DeepSeek V4.1 Flash, high effort")?.model).toBe("opencode-go/deepseek-v4.1-flash"); + expect(read("Codex, openai/gpt-6.1-sol, high effort")?.model).toBe("gpt-6.1-sol"); expect(read("Codex, gpt-6.1-sol, high")?.effort).toBe("high"); expect(read("unknown")).toBeNull(); // A lone field reads as a listed display name or an ID, keeping the size its notes give. @@ -1814,8 +1577,8 @@ test("an Implementer line reads as agents write it", () => { expect(read("Claude Code")).toBeNull(); expect(read("claude-opus-5-5, unknown")).toEqual(opus("unknown", "unknown")); expect(read("Claude Code, Opus 5.5,")).toEqual(opus("unknown")); - expect(read("opencode, opencode-go/kimi, high effort")?.model).toBe("opencode-go/kimi"); - expect(read("opencode-go/kimi")).toBeNull(); + expect(read("OpenRouter, openrouter/kimi, high effort")?.model).toBe("openrouter/kimi"); + expect(read("openrouter/kimi")).toBeNull(); expect(read(", claude-opus-5-5, high effort")).toEqual(opus("high", "unknown")); // An ID no list names still reads, so a new model is counted before the lists add it. expect(read("Codex, gpt-7, high effort")?.model).toBe("gpt-7"); @@ -2009,13 +1772,12 @@ fakes("no shadow runs below low, though one settles a shadow's findings the pick expect(opusLow.code, opusLow.stderr).toBe(0); expect(JSON.parse(opusLow.stdout).rounds.map((r: Round) => [r.reviewer, r.effort, r.shadow])).toEqual([ ["claude", "low", undefined], - ["opencode", "high", undefined], ]); - // Sol medium's shadow, Sol low, leaves a finding open, and a --harness run leaves it to the pair. + // Sol medium's shadow, Sol low, leaves a finding open, and a --harness run leaves it to the size's pick. const t = setup(); expect((await quest("--root", t.repo, "review", "--size", "S")).code).toBe(0); - write(t.repo, ".tmp/responses.md", "1. Fixed.\n2. Fixed.\n3. Fixed.\n"); + write(t.repo, ".tmp/responses.md", "1. Fixed.\n2. Fixed.\n"); const responses = join(t.repo, ".tmp", "responses.md"); const alone = await quest( "--root", @@ -2030,7 +1792,7 @@ fakes("no shadow runs below low, though one settles a shadow's findings the pick ); expect(alone.code).toBe(2); expect(alone.stderr).toContain( - "opencode and codex (shadow) still have findings to settle on this branch; settle them in a run of the pair, or of that reviewer with --harness, with --responses", + "codex (shadow) still has findings to settle on this branch; settle them in a run of the size's pick, or of that reviewer with --harness, with --responses", ); // Opus low then replaces Sol, with no step below it, so a shadow at Opus low settles the old shadow's finding. const sol = ["--size", "S", "--exclude-model", "gpt-6.1-sol"]; @@ -2039,8 +1801,7 @@ fakes("no shadow runs below low, though one settles a shadow's findings the pick const rounds: Round[] = JSON.parse(settled.stdout).rounds; expect(rounds.map((r) => [r.reviewer, r.effort, r.shadow, r.earlier?.findings.map((f) => f.number)])).toEqual([ ["claude", "low", undefined, [1]], - ["opencode", "high", undefined, [2]], - ["claude", "low", true, [3]], + ["claude", "low", true, [2]], ]); }); @@ -2063,7 +1824,6 @@ test("saved sessions are read only in the current format, or refused naming --ne const session = { reviewer: "codex", session: "thread-1", - root: "/repo", rounds: [{ head: "a", mergeBase: "m", findings: [finding] }], last: {}, given: { instructions: "i", rules: null }, @@ -2086,16 +1846,24 @@ test("saved sessions are read only in the current format, or refused naming --ne tallies: [tally, { ...tally, shadow: true }], }; expect(parseState(shadowed)).toEqual(shadowed); + // A retired reviewer's tally is dropped so the record never names it; its session stays for the run's numbering. + const retired = { ...session, reviewer: "opencode", session: "ses_1" }; + expect( + parseState({ + branch: "feature", + sessions: [session, retired], + tallies: [tally, { ...tally, reviewer: "opencode" }], + }), + ).toEqual({ ...current, sessions: [session, retired] }); for (const older of [ null, - // One reviewer's session, as quest saved it before the second reviewer. + // One reviewer's session alone, outside a sessions list. { branch: "feature", ...session }, { sessions: [{ ...session, rounds: [{ head: "a", findings: 1 }] }] }, { sessions: [{ ...session, rounds: [] }] }, { sessions: [{ ...session, last: undefined }] }, { sessions: [{ ...session, given: undefined }] }, { sessions: [{ ...session, session: undefined }] }, - { sessions: [{ ...session, root: undefined }] }, // Saved before tallies, or with a session no tally counts. { branch: "feature", sessions: [session] }, { branch: "feature", sessions: [session], tallies: [] }, @@ -2108,6 +1876,39 @@ test("saved sessions are read only in the current format, or refused naming --ne expect(() => parseState(older)).toThrow(/--new/); }); +fakes("a retired reviewer's open findings are dropped, keeping the numbers of the rest", async () => { + const s = setup(); + expect((await quest("--root", s.repo, "review")).code).toBe(0); + // An opencode session from before its removal, numbered between the pick's finding and the shadow's. + const path = sessionFile(join(s.repo, ".git"), "feature"); + const saved = JSON.parse(readFileSync(path, "utf8")); + const [pick, shadow] = saved.sessions; + const opencode = { ...pick, reviewer: "opencode", session: "ses_1" }; + writeFileSync( + path, + JSON.stringify({ + ...saved, + sessions: [pick, opencode, shadow], + tallies: [...saved.tallies, { ...saved.tallies[0], reviewer: "opencode" }], + }), + ); + + write(s.repo, "src/lib.ts", "export const x = 4;\n"); + git(s.repo, "commit", "-qam", "fix"); + write(s.repo, ".tmp/responses.md", "1. Fixed at src/lib.ts:1.\n2. Refuted.\n3. Fixed at src/lib.ts:1.\n"); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); + const later = await quest("--root", s.repo, "review", "--responses", join(s.repo, ".tmp", "responses.md")); + expect(later.code, later.stderr).toBe(0); + // The shadow settles its finding by the number the author answered, not by #2, the dropped one's. + expect(at(s.calls(), "medium").at(-1)!.stdin).toContain( + "\n3. high src/lib.ts:2 Off by one\n", + ); + expect(later.stdout).not.toContain("opencode"); + const after = JSON.parse(readFileSync(path, "utf8")); + expect(after.sessions.map((each: { reviewer: string }) => each.reviewer)).toEqual(["codex", "codex"]); + expect(after.tallies.map((each: { reviewer: string }) => each.reviewer)).not.toContain("opencode"); +}); + fakes("a session saved by an older quest is refused before the reviewer runs", async () => { const s = setup(); expect((await quest("--root", s.repo, "review")).code).toBe(0); @@ -2126,53 +1927,24 @@ fakes("a session saved by an older quest is refused before the reviewer runs", a expect((await quest("--root", s.repo, "review", "--new")).code).toBe(0); }); -fakes("a reviewer's failure stops the other and saves neither round", async () => { +fakes("--harness reviews alone, keeping the size pick's session for its next run", async () => { const s = setup(); - vi.stubEnv("FAKE_OPENCODE_LIMITED", "reset"); - const result = await quest("--root", s.repo, "review"); - expect(result.stderr).toBe("error: opencode hit its usage limit; resets 2030-03-17T17:46:40Z\n"); - expect(result.code).toBe(3); - expect(existsSync(sessionFile(join(s.repo, ".git"), "feature"))).toBe(false); - vi.stubEnv("FAKE_OPENCODE_LIMITED", undefined); - // No round was saved, so the next one needs no responses. - expect((await quest("--root", s.repo, "review")).code).toBe(0); -}); - -fakes("--harness reviews alone, keeping the other reviewer's session unless it has findings to settle", async () => { - const s = setup(); - const clean = JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] }); - vi.stubEnv("FAKE_REPLY", clean); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); expect((await quest("--root", s.repo, "review")).code).toBe(0); - const opencode = s.opencodeCalls().length; write(s.repo, "src/lib.ts", "export const x = 4;\n"); git(s.repo, "commit", "-qam", "more"); - const alone = await quest("--root", s.repo, "review", "--harness", "codex"); + const claude = ["--harness", "claude", "--model", "opus", "--effort", "low"]; + const alone = await quest("--root", s.repo, "review", ...claude, "--json"); expect(alone.code, alone.stderr).toBe(0); - expect(alone.stdout).not.toContain("reviewer opencode"); - expect(s.opencodeCalls()).toHaveLength(opencode); - // The pair's next run resumes the kept session on the new head. - const pair = await quest("--root", s.repo, "review", "--json"); - expect(pair.code, pair.stderr).toBe(0); - expect(JSON.parse(pair.stdout).rounds.map((r: Round) => [r.reviewer, r.round])).toEqual([ - ["codex", 2], - ["opencode", 2], - ]); - - // Its open findings keep a head from passing the size's pick alone. - const t = setup(); - vi.stubEnv("FAKE_REPLY", clean); - vi.stubEnv("FAKE_OPENCODE_REPLY", JSON.stringify({ score: 8, rationale: "One gap.", findings: [FINDING] })); - expect((await quest("--root", t.repo, "review")).code).toBe(0); - const calls = t.calls().length; - const refused = await quest("--root", t.repo, "review", "--harness", "codex"); - expect(refused.code).toBe(2); - expect(refused.stderr).toContain("opencode still has findings to settle on this branch"); - expect(t.calls()).toHaveLength(calls); + expect(JSON.parse(alone.stdout).rounds.map((r: Round) => r.reviewer)).toEqual(["claude"]); + // The size pick's next run resumes its kept session on the new head. + const picked = await quest("--root", s.repo, "review", "--json"); + expect(picked.code, picked.stderr).toBe(0); + expect(JSON.parse(picked.stdout).rounds.map((r: Round) => [r.reviewer, r.round])).toEqual([["codex", 2]]); }); fakes("a new size pick, and its shadow, settle the open findings of the reviewers they replace", async () => { const s = setup(); - vi.stubEnv("FAKE_OPENCODE_REPLY", JSON.stringify({ score: 10, rationale: "Lands as is.", findings: [] })); expect((await quest("--root", s.repo, "review")).code).toBe(0); // Sol wrote the fix, so Opus replaces it as the size's pick. write(s.repo, "src/lib.ts", "export const x = 4;\n"); @@ -2185,7 +1957,7 @@ fakes("a new size pick, and its shadow, settle the open findings of the reviewer const responses = join(s.repo, ".tmp", "responses.md"); const round = await quest("--root", s.repo, "review", ...sol, "--responses", responses, "--json"); expect(round.code, round.stderr).toBe(0); - const [opus, , shadow] = JSON.parse(round.stdout).rounds; + const [opus, shadow] = JSON.parse(round.stdout).rounds; const settled = (r: Round) => [r.reviewer, r.effort, r.shadow, r.round, r.earlier!.findings.map((f) => f.number)]; expect(settled(opus)).toEqual(["claude", "medium", undefined, 1, [1]]); // Sol's shadow had a finding open, so Opus's shadow, one effort below Opus medium, settles it. @@ -2200,7 +1972,6 @@ fakes("a new size pick, and its shadow, settle the open findings of the reviewer JSON.parse(round.stdout).tallies.map((t: Tally) => [t.reviewer, t.shadow, t.rounds, t.findings.high, t.fixed]), ).toEqual([ ["claude", undefined, 1, 1, 0], - ["opencode", undefined, 2, 0, 0], ["claude", true, 1, 1, 0], ["codex", undefined, 1, 1, 1], ["codex", true, 1, 1, 1], diff --git a/test/sandbox.test.ts b/test/sandbox.test.ts deleted file mode 100644 index c2702b74..00000000 --- a/test/sandbox.test.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { expect, test } from "vitest"; - -import { sandboxEnvironment, sandboxSettings, shellScript } from "../src/cli/sandbox"; - -test("commands keep no credential, and PATH keeps no directory the checkout holds", () => { - const env = sandboxEnvironment( - { - PATH: "/repo/node_modules/.bin:bin:/usr/bin:/repo/.git/hooks:/nix/bin", - OPENAI_API_KEY: "secret", - OPENCODE_CONFIG_CONTENT: "{}", - LANG: "C.UTF-8", - }, - { reads: ["/repo", "/repo/.git"], tmp: "/t/tmp" }, - ); - expect(env).toEqual({ - PATH: "/usr/bin:/nix/bin", - LANG: "C.UTF-8", - HOME: "/t/tmp", - TMPDIR: "/t/tmp", - CLAUDE_CODE_TMPDIR: "/t/tmp", - }); -}); - -test("commands reach no host, read the homes only where named, and write only the private directory", () => { - expect(sandboxSettings({ reads: ["/home/u/repo"], tmp: "/tmp/q/tmp" }, "linux", "/home/u")).toEqual({ - network: { allowedDomains: [], deniedDomains: [], strictAllowlist: true }, - filesystem: { - denyRead: ["/home", "/root", "/tmp", "/var/tmp", "/run/user", "/home/u"], - allowRead: ["/home/u/repo", "/tmp/q/tmp"], - allowWrite: ["/tmp/q/tmp"], - denyWrite: ["/tmp/claude"], - }, - enableWeakerNestedSandbox: false, - }); -}); - -test("the shell runs the command alone under srt, from a cleared environment", () => { - const script = shellScript("/bin/srt", "/q/srt.json", { HOME: "/q/it's" }); - expect(script).toContain(`exec /usr/bin/env -i 'HOME=/q/it'\\''s' '/bin/srt' --settings '/q/srt.json' -c "$2"`); - expect(script).toContain(`[ "$1" != -c ]`); -}); diff --git a/test/usage.test.ts b/test/usage.test.ts index 75335418..f04208a5 100644 --- a/test/usage.test.ts +++ b/test/usage.test.ts @@ -206,11 +206,12 @@ describe("PR records", () => { unchanged: false, }; + // The shadow: the pick one effort lower, in its own session. const second: Round = { ...round, - reviewer: "opencode", - model: "opencode-go/deepseek-v4.1-flash", - session: "ses_1", + shadow: true, + effort: "medium", + session: "thread-2", round: 1, score: 10, findings: [], @@ -234,7 +235,7 @@ describe("PR records", () => { }); const tallies = [ tally("codex", 3, { high: 1, medium: 1, low: 2 }, 2, 1), - tally("opencode", 1, { high: 0, medium: 0, low: 0 }, 0, 0), + { ...tally("codex", 1, { high: 0, medium: 0, low: 0 }, 0, 0), shadow: true as const }, // A left-out reviewer still names its session, so `quest usage` counts that session as reviewing. { ...tally("claude", 2, { high: 2, medium: 0, low: 0 }, 1, 0), session: CLAUDE }, ]; @@ -262,11 +263,11 @@ describe("PR records", () => { open: { high: 0, medium: 1, low: 0 }, }, { - harness: "opencode", - model: "opencode-go/deepseek-v4.1-flash", - effort: "high", - shadow: false, - session: "ses_1", + harness: "Codex", + model: "gpt-6.1-sol", + effort: "medium", + shadow: true, + session: "thread-2", round: 1, score: 10, tally: { @@ -310,7 +311,7 @@ describe("PR records", () => { }); test("a record written before tallies has none, and a Rounds line that does not add up fails", () => { - const record = reviewRecord([second], [tally("opencode", 1, { high: 0, medium: 0, low: 0 }, 0, 0)]); + const record = reviewRecord([second], [tallies[1]!]); const older = `## Review\n\n${record.replace(/^- Rounds: .*\n/m, "")}\n`; expect(parseReview(older)[0]!.tally).toBeNull(); // The settled count `quest usage` prints reads back. @@ -458,7 +459,8 @@ describe("usage", () => { "- Rounds: 2; findings 2 (high 1, low 1): fixed 1, refuted 1, open 0", "- Findings: none", "", - "- Reviewer: opencode, opencode-go/deepseek-v4.1-flash, high effort", + // A harness quest review does not run, as a hand-written record may name, reads as any other. + "- Reviewer: Gemini CLI, gemini-3-pro, high effort", "- Session: `ses_1`, round 1", "- Score: 8/10, `Fine.`", "- Rounds: 1; findings 0: fixed 0, refuted 0, open 0", @@ -472,13 +474,9 @@ describe("usage", () => { "- Reviewer: Codex, gpt-6.1-sol, medium effort, shadow", "- Rounds: 1; findings 3 (high 3): fixed 2, refuted 1, open 0", ].join("\n"); - // #9's claim of the same branch named ses_1 before #10 resumed it, so #10, the newer, takes it. - // Its record and #10's tally the same branch, so the rollup counts the tally once, from #10. + // #9's record and #10's tally the same branch, so the rollup counts the tally once, from #10. const body9 = `${body10.slice(body10.indexOf("## Review"))}\n\n## Models\n\n- Size: L\n- Implementer: Claude Code, claude-opus-5-5, high effort\n`; - const fork13 = body10 - .slice(body10.indexOf("## Review")) - .replaceAll(THREAD, "thread-fork") - .replaceAll("ses_1", "ses_fork"); + const fork13 = body10.slice(body10.indexOf("## Review")).replaceAll(THREAD, "thread-fork"); const body11 = "Fix.\n\n## Caused by\n\n- [acme/proj#10](https://github.com/acme/proj/pull/10) - missed a case\n"; const pulls = [ @@ -638,7 +636,6 @@ describe("usage", () => { id: "2026/11/06/rollout-c-22222222-2222-2222-2222-222222222222", models: [spent("gpt-6.1-sol", 3)], }, - { harness: "opencode", id: "ses_1", models: [spent("deepseek", 4)] }, { harness: "claude", id: OWNED, models: [spent("claude-opus-5-5", 7)] }, { harness: "claude", id: GONE, models: [spent("claude-opus-5-5", 8)] }, { harness: "claude", id: BOTH, models: [spent("claude-opus-5-5", 9)] }, @@ -668,8 +665,6 @@ describe("usage", () => { ["claude", CLAUDE, "implementing", "high"], ["codex", THREAD, "reviewing", "high"], ["codex", "11111111-1111-1111-1111-111111111111", "iterating", "high"], - // No log quest reads names opencode's session, but the review record does. - ["opencode", "ses_1", "reviewing", "high"], ["claude", OWNED, "iterating", null], ]); expect(ten.fixedBy).toEqual([11]); @@ -695,7 +690,6 @@ describe("usage", () => { ["L", "iterating", "claude", 7], ["L", "iterating", "codex", 2], ["L", "reviewing", "codex", 1], - ["L", "reviewing", "opencode", 4], ]); const tally = { rounds: 2,