From abb39bc39859ed4dd8c595252b18421873571987 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 11:07:12 -0800 Subject: [PATCH 1/8] feat(merge)!: Pullfrog's online review gates quest merge in place of DeepSeek quest review runs only the size's pick: opencode, its DeepSeek second reviewer, --harness opencode, and the srt sandbox are gone. quest merge refuses a head until the online reviewer the base's REVIEW.md names in its front matter has reviewed it, or a diff-identical head, and every review thread it opened is resolved by it or a writer. quest status prints the online-review line. Rules, skills, and quests follow. Co-Authored-By: Claude Opus 5.5 --- README.md | 97 +++---- REVIEW.md | 3 + assets/CONTEXT.md | 4 +- assets/skills/merge.md | 3 +- assets/skills/review.md | 4 +- assets/skills/run.md | 2 +- docs/context.md | 4 +- flake.nix | 10 +- justfile | 4 +- nix/sandbox-runtime.nix | 61 ----- quest/m0/pullfrog-setup.md | 3 + quest/m0/routing-evidence/README.md | 4 + quest/m0/token-waste/README.md | 7 + quest/m1/model-check.md | 9 +- src/cli/hold.ts | 16 ++ src/cli/limit.ts | 4 +- src/cli/main.ts | 50 ++-- src/cli/models.ts | 14 +- src/cli/online-review.ts | 199 ++++++++++++++- src/cli/review-record.ts | 2 +- src/cli/review.ts | 377 ++++++---------------------- src/cli/sandbox.ts | 157 ------------ src/cli/status.ts | 49 ++-- src/cli/usage.ts | 29 +-- src/github/models.ts | 10 +- test/hold.test.ts | 150 +++++++++++ test/live/review-sandbox.test.ts | 167 ++---------- test/online-review.test.ts | 23 +- test/review.test.ts | 337 +++---------------------- test/sandbox.test.ts | 41 --- test/status.test.ts | 53 +++- test/usage.test.ts | 36 ++- 32 files changed, 734 insertions(+), 1195 deletions(-) create mode 100644 REVIEW.md delete mode 100644 nix/sandbox-runtime.nix delete mode 100644 src/cli/sandbox.ts delete mode 100644 test/sandbox.test.ts diff --git a/README.md b/README.md index 635050a9..3eab4128 100644 --- a/README.md +++ b/README.md @@ -483,6 +483,7 @@ mergeable conflicting merge-state dirty review-decision - review-record diff-identical 5f1c2e9a8b7d6c5e4f3a2b1c0d9e8f7a6b5c4d3e +online-review current pullfrog[bot] 0ade9d977d54c6f48152410532c58e9ea78db3d0 open=0 auto-merge - merge-queue - landing-stopped - @@ -501,6 +502,7 @@ comes last, and a missing value prints as `-`: | `url`, `state`, `draft`, `head` | The PR URL; `open`, `closed`, or `merged`; `true` or `false`; the head SHA. | | `mergeable`, `merge-state`, `review-decision` | GitHub's values, lowercased. | | `review-record` | Whether the PR body's [`## Review` record](#review-locally) still covers the head, and the head it names last: `current` when it is the head, `diff-identical` when `git diff ...` prints the same as for it, `stale` otherwise, or `unknown` when GitHub cannot compare them, such as a diff too large; `-` when the record names no full SHA. Whose record counts is the reader's call. | +| `online-review` | Where the [online reviewer](#require-an-online-review) the base's `REVIEW.md` names stands on an open PR's head, as `quest merge` reads it: `current` when it reviewed the head, `diff-identical` when it reviewed a head whose diff from the base matches, `running` while its app's check on the head has not finished, or `missing`; then the reviewer, the head it reviewed, and `open=` with the URL of each review thread it opened that neither it nor a writer resolved. `-` when the base names none. | | `auto-merge` | Method, who enabled it, when, and the head it was enabled on. | | `merge-queue` | Queue state, position, and when it was enqueued. | | `landing-stopped` | The reason the latest merge-queue entry, auto-merge, or [`quest land`](#land-a-batch-with-quest-land) queue entry ended (such as `merge_conflict`, `failed_checks`, `manual`, `manually_disabled`, or `merged`), who ended it, and when; `-` once the PR has entered the merge queue or auto-merge again since. | @@ -531,7 +533,7 @@ requirements from the base, matches a required check by name and, when the requirement names one, its app, as GitHub and `quest land` do, and leaves out `quest land`'s check, which no head carries. `--json` prints the same facts as one object with the fields `url`, `state`, `draft`, `head`, -`mergeable`, `mergeState`, `reviewDecision`, `autoMerge`, `mergeQueue`, +`mergeable`, `mergeState`, `reviewDecision`, `reviewRecord`, `onlineReview` (`reviewer`, `head`, `state`, `reviewed`, `open`, or null), `autoMerge`, `mergeQueue`, `landingStopped`, `checks` (`state`, `passed`, `failing`, `pending`), `reviews`, `checkpoints` (`reviewer`, `round`, `url`), `reactions`, `threads`, `holds` (`label`, `token`, `takenAt`, `age`, `eventUrl`), and `starterHold` (`lastActive`, `age`, `url`, or null). Ownership uses the state commands' shared reader without changing labels, and event links match the opaque acquisition token against GitHub's event listing. Long lists are paged; reactions are the newest 100 per PR or review, as in `quest wait`. Any PR state exits 0, invalid arguments exit 2, and other @@ -580,7 +582,9 @@ 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. It refuses a head the base's online reviewer has not reviewed, or +whose findings are open, as [Require an online review](#require-an-online-review) +describes. 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 @@ -626,6 +630,34 @@ only what needs a maintainer: a behavior or design decision gets a diagnosis on PR, and a fix that edits protected paths or that GitHub refuses to push gets a ready commit for `quest land apply `. +### Require an online review + +A repository names the online reviewer whose review gates `quest merge`, such as +[Pullfrog](https://pullfrog.com), in its `REVIEW.md` front matter, read from the +base so a PR cannot drop its own gate: + +```markdown +--- +online-reviewer: pullfrog[bot] +--- +``` + +The name is the reviewer's GitHub login, or the name a review body opens with +as `Automated review by ` for a reviewer posting under a person's account, +whose reviews then count only from accounts that can push. `quest merge` +refuses a head until that reviewer has reviewed it, in a review of the head or +a successful check run of its own GitHub App on it (a re-review that finds +nothing new may post only the check), or reviewed an earlier head whose diff +from the base matches. It also refuses while any review thread the reviewer +opened, on any head, is unresolved, or was resolved by an account that cannot +push, since GitHub lets the PR's author resolve one: fix or refute each finding +in its thread and resolve it. Findings it lists only in a review body are not +read. A reviewer that has not started, or whose run failed, holds the PR until +it reviews the head; ask it to, as its documentation says. A repository whose +`REVIEW.md` names none lands on its local review alone, the +[`quest review`](#review-locally) round its PR body records. Other online +reviewers, such as a capped CodeRabbit, are never required to review. + ## Land a batch with quest land `quest land serve` runs on a maintainer's host, with one process per base branch of a repository. @@ -1391,14 +1423,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 +1446,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 +1471,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 +1484,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 +1512,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 +1528,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 +1624,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/REVIEW.md b/REVIEW.md new file mode 100644 index 00000000..013ba927 --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,3 @@ +--- +online-reviewer: pullfrog[bot] +--- diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index f1e8c09b..2bced6b4 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -68,9 +68,9 @@ Merge the base only when the branch conflicts with it or a landing step, such as Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. -Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. +Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings posted on the head and reply to feedback you disagree with. `quest merge` needs the review of the online reviewer the base's `REVIEW.md` names, if any; no other online review is required. Request a review only from a reviewer whose approval the base requires and that never ran on the head. -Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since such a reviewer may approve only after its threads are resolved. +Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since an open thread holds the merge. The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index f17896e7..4453bcd8 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -15,11 +15,12 @@ The head is landable when: - the PR is not a draft; - a review counts for it, as below; +- when the base's `REVIEW.md` names an online reviewer, its `online-review` line is `current` or `diff-identical` with `open=0`; - every review finding posted on it, including automated reviewers' such as CodeRabbit or Codex, is fixed on the head or answered with a refutation, and no decision is open; - no check failed on it, required or not, its required checks passed, other than the gate's own `quest land` check, which never reports on a head, and it does not conflict with its base; - no review requests changes; an approval its base requires may still be to come, since the queue waits for it. -While its required checks are only pending, report that the pick stands until they pass; do not wait for them. +While its required checks are only pending, or its online review `running` or `missing`, report that the pick stands until they finish; do not wait for them. A change request that stands once its findings are fixed or refuted and their threads resolved is the user's call. Any other head needs `/quest-iterate`: run it, saying what the head needs, such as a review, fixes, a failed check fixed or rerun, flaky ones included, or decisions, then run this skill again once it finishes; if it stops for the user instead, report that. Run it in this session, where its interview can ask the user; under a coordinator such as `/quest-run`, return the PR to the loop, which dispatches it in a sub-agent. 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/assets/skills/run.md b/assets/skills/run.md index 573d4588..8817a783 100644 --- a/assets/skills/run.md +++ b/assets/skills/run.md @@ -40,7 +40,7 @@ Run each `/quest-merge` inline; dispatch each `/quest-iterate` and `/quest-revie Dispatch `/quest-iterate` without asking for a PR that needs work but no decision, such as a conflict, failing checks, unaddressed findings, or no review that counts for its head as `/quest-merge` defines. A PR with open decisions, its `## Waiting` section's or an issue you would solve differently, goes to the user: settle them with the grill skill's rounds, then dispatch `/quest-iterate` with the answers in its brief, since a sub-agent cannot prompt. A PR you would skip or delete rather than fix, such as a superseded one, gets that question instead. -Leave a PR whose required checks are still running for a later round; an automated reviewer is not waited on, since the queue waits for an approval the base requires. +Leave a PR whose required checks or online review are still running for a later round; no other automated reviewer is waited on, since the queue waits for an approval the base requires. A PR whose author has write access and that changes no user-facing behavior carries a standing `/quest-merge` pick, unless the repository's `CONTRIBUTING.md` opts out: run `/quest-merge` on it without asking, and name it as auto-picked in the summary. That covers only tests, benchmarks, CI, internal docs or comments, and behavior-preserving code. Anything users or other repositories get, such as CLI behavior or output, shipped skills or rules, user-facing docs, dependencies, or release artifacts, needs the user's pick, as does adding or expanding quests. diff --git a/docs/context.md b/docs/context.md index 31a6a257..f77881ce 100644 --- a/docs/context.md +++ b/docs/context.md @@ -68,9 +68,9 @@ Merge the base only when the branch conflicts with it or a landing step, such as Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. -Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. +Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings posted on the head and reply to feedback you disagree with. `quest merge` needs the review of the online reviewer the base's `REVIEW.md` names, if any; no other online review is required. Request a review only from a reviewer whose approval the base requires and that never ran on the head. -Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since such a reviewer may approve only after its threads are resolved. +Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since an open thread holds the merge. The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. 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..ecfbeec6 100644 --- a/quest/m0/pullfrog-setup.md +++ b/quest/m0/pullfrog-setup.md @@ -17,6 +17,9 @@ 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 review gates `quest merge` wherever the base's +`REVIEW.md` names it, 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..72ba36dc 100644 --- a/quest/m0/token-waste/README.md +++ b/quest/m0/token-waste/README.md @@ -41,6 +41,13 @@ 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 ✅ the + online reviewer the base's `REVIEW.md` names gates `quest merge` + (rejected: advisory only). - 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..82f69dc7 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -20,6 +20,7 @@ import { import { ALLOWS, LAND_CHECK, METHOD_NAMES, contexts, enqueue } from "./land"; import { QUEUE_CHECK } from "./land-queue"; import type { LandingCheck } from "./landed"; +import { declaredReviewer, onlineGate, onlineRefusal } from "./online-review"; import { classifyCheck } from "./status"; import { Refusal, ago } from "./start"; @@ -253,6 +254,7 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe } }; await passing(labels.github, pr, snapshot.url, sha); + await reviewed(labels.github, pr, snapshot, deps.warn); const findings = await request.check({ remote: `https://github.com/${pr.owner}/${pr.repo}.git`, base: snapshot.base, @@ -361,6 +363,20 @@ async function passing(github: GitHub, pr: PullRequest, url: string, sha: string ); } +/** + * Refuse a head the online reviewer the base's REVIEW.md names has not reviewed, nor a head with the same diff, or + * whose findings it opened on the PR are not all resolved. A base that names none lands on its local review alone. + */ +async function reviewed(github: GitHub, pr: PullRequest, snapshot: Snapshot, warn: (message: string) => void) { + const reviewer = await declaredReviewer(github, pr, snapshot.baseSha); + if (reviewer === null) return; + const gate = await onlineGate(github, pr, reviewer, snapshot.base, warn); + if (gate.head !== snapshot.head) + throw new Refusal(`${snapshot.url} head moved to ${gate.head}; check the new head before landing it`); + const refusal = onlineRefusal(gate); + if (refusal !== null) throw new Refusal(`${snapshot.url} head ${snapshot.head}: ${refusal}`); +} + /** GraphQL error types that fault the token or the request, not the PR GitHub would enqueue. */ export const UNREFUSED = ["FORBIDDEN", "NOT_FOUND", "INSUFFICIENT_SCOPES", "INTERNAL"]; 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..198d76f6 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, @@ -555,10 +554,14 @@ export function main(argv: string[], out: Output): number | Promise { "Print a pull request's head, checks, merge state, reviews, reactions, and unresolved review threads " + "in one call. It reports facts; it does not decide whether the PR is ready or whose review counts.\n\n" + "Plain output prints one record per line, its type first and any free text (a check name or path) last: " + - "url, state, draft, head, mergeable, merge-state, review-decision, review-record, auto-merge, merge-queue, " + + "url, state, draft, head, mergeable, merge-state, review-decision, review-record, online-review, auto-merge, merge-queue, " + "landing-stopped, and checks, whose state and counts follow the checks that count, pending while an open PR's base requires a check or workflow nothing reports; review-record gives whether the PR body's ## Review record " + "covers the head, as current, diff-identical (git diff ... prints the same as for the head it names), " + - "stale, or unknown when GitHub cannot compare them, then the head it names last; auto-merge gives its method, who enabled it and when, and the head it was " + + "stale, or unknown when GitHub cannot compare them, then the head it names last; online-review, on an open PR whose " + + "base's REVIEW.md names an online reviewer in its front matter (online-reviewer: ), gives where that reviewer " + + "stands on the head as quest merge reads it: current, diff-identical, running (its app's check on the head has not " + + "finished), or missing, then the reviewer, the head it reviewed, and open= with the URL of each review thread it " + + "opened that neither it nor a writer resolved; auto-merge gives its method, who enabled it and when, and the head it was " + "enabled on, which differs from head once a push follows, as GitHub keeps auto-merge across a writer's " + "push (- when it cannot be told: a quest land pin names it, and otherwise the head branch's push " + "activity, which GitHub lists up to minutes late); landing-stopped is the latest exit from the merge queue, auto-merge, or " + @@ -642,7 +645,11 @@ 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. When the base's REVIEW.md names an online " + + "reviewer in its front matter (online-reviewer: ), it refuses a head that reviewer has not reviewed, " + + "in a review of it or a successful check run of its own app on it, unless it reviewed a head whose diff from " + + "the base matches, and a head with a review thread it opened on any head that neither it nor an account that " + + "can push resolved; a base that names none lands on its local review alone. 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 +1166,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 +1236,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 +1249,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 +1263,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 +1280,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 +1319,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 +1346,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/online-review.ts b/src/cli/online-review.ts index 3f64eb12..d59d1ac9 100644 --- a/src/cli/online-review.ts +++ b/src/cli/online-review.ts @@ -2,7 +2,18 @@ // thread, and any review whose // body opens with `Automated review by `, as a reviewer posting under a person's account does. A finding is a // review thread one opens, or a `[P]` line in its review body; whoever answers it says whether it was taken. -import { type GitHub, type ObjectValue, type Repository, array, object } from "./github"; +// The repository's own online reviewer, which REVIEW.md names, also gates `quest merge`: see {@link onlineGate}. +import { + type GitHub, + GitHubDeadline, + type ObjectValue, + type PullRequest, + type Repository, + array, + link, + object, + string, +} from "./github"; /** One online reviewer's record on one PR. */ export interface OnlineReview { @@ -49,9 +60,9 @@ export function bodyFindings(body: string): string[] { }); } -const THREAD_COMMENT = "author { __typename login } body pullRequestReview { id }"; +const THREAD_COMMENT = "author { __typename login } body url pullRequestReview { id }"; const FIELDS = { - reviews: "id url author { __typename login } body submittedAt commit { oid }", + reviews: "id url state author { __typename login } body submittedAt commit { oid }", reviewThreads: "id isResolved resolvedBy { __typename login } " + `comments(first: 100) { pageInfo { hasNextPage endCursor } nodes { ${THREAD_COMMENT} } }`, @@ -275,3 +286,185 @@ export async function readOnlineReviews( const pulls = await read(client, repository, numbers); return new Map([...pulls].map(([number, pull]) => [number, onlineReviews(pull)])); } + +/** The key of REVIEW.md's front matter that names the repository's online reviewer. */ +const DECLARATION = "online-reviewer"; + +/** + * The online reviewer REVIEW.md's front matter names, such as `pullfrog[bot]`, or null when it names none. + * A key given twice or left empty throws, so a typo cannot silently drop the gate. + */ +export function reviewerIn(text: string): string | null { + const matter = /^---\r?\n([\s\S]*?)\r?\n---[ \t]*(?:\r?\n|$)/.exec(text); + if (!matter) return null; + const lines = matter[1]!.split(/\r?\n/).filter((line) => line.startsWith(`${DECLARATION}:`)); + if (lines.length === 0) return null; + if (lines.length > 1) throw new Error(`REVIEW.md names its ${DECLARATION} more than once`); + const value = lines[0]! + .slice(DECLARATION.length + 1) + .trim() + .replace(/^(["'])(.*)\1$/, "$2") + .trim(); + if (value === "") throw new Error(`REVIEW.md's ${DECLARATION} is empty`); + return value; +} + +/** The online reviewer REVIEW.md names at `ref`, read from GitHub so a PR cannot drop its own gate; null for none. */ +export async function declaredReviewer(client: GitHub, repository: Repository, ref: string): Promise { + const response = await client.request( + `/repos/${repository.owner}/${repository.repo}/contents/REVIEW.md?ref=${encodeURIComponent(ref)}`, + { accept: "application/vnd.github.raw", allow: [404] }, + ); + return response.status === 404 ? null : reviewerIn(await response.text()); +} + +/** + * Whether two commits have the same diff from `base` (`git diff ...`), or null when GitHub cannot + * compare one, such as a diff too large to render; `warn` says why. + */ +export async function sameDiff( + client: GitHub, + repository: Repository, + base: string, + shas: [string, string], + warn: (message: string) => void, +): Promise { + const diffs: string[] = []; + for (const sha of shas) { + let response: Response; + try { + response = await client.request( + `/repos/${repository.owner}/${repository.repo}/compare/${encodeURIComponent(base)}...${sha}`, + { accept: "application/vnd.github.diff", allow: [404, 406, 422] }, + ); + } catch (error) { + // A deadline is the caller's to handle; a forbidden or failing compare is not. + if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; + warn(`cannot compare ${sha} with ${base}: ${error.message}`); + return null; + } + if (!response.ok) { + warn(`cannot compare ${sha} with ${base}: GitHub answered ${response.status}`); + return null; + } + diffs.push(await response.text()); + } + return diffs[0] === diffs[1]; +} + +/** Where the repository's online reviewer stands on a PR's head, as {@link onlineGate} reads it. */ +export interface OnlineGate { + reviewer: string; + /** The PR's head when it was read. */ + head: string; + /** + * `current`: it reviewed the head, in a review of it or a successful check run of its own app on it, which a + * re-review that finds nothing new leaves without a review; `diff-identical`: its latest review is of an earlier head + * whose diff from the base matches the head's; `running`: its app's check on the head has not finished; + * `missing`: none of these. + */ + state: "current" | "diff-identical" | "running" | "missing"; + /** The head it reviewed, or null. */ + reviewed: string | null; + /** Each review thread it opened on the PR, on any head, that neither it nor a writer resolved, by URL. */ + open: string[]; +} + +/** Permissions that can push, the only ones besides the reviewer's own whose resolution settles a finding. */ +const PUSH = ["admin", "maintain", "write"]; + +/** + * How `reviewer`, the repository's online reviewer, stands on `pr`'s head against `base`: whether it reviewed the head + * or a diff-identical one, and which of its findings, its review threads, are still open. GitHub lets the PR's author + * resolve a thread too, so a resolution counts only from the reviewer or an account that can push. + */ +export async function onlineGate( + client: GitHub, + pr: PullRequest, + reviewer: string, + base: string, + warn: (message: string) => void, +): Promise { + const pull = (await read(client, pr, [pr.number])).get(pr.number)!; + const head = pull.head; + const pushers = new Map(); + const canPush = async (account: string) => { + if (!pushers.has(account)) { + const response = await client.request( + `/repos/${pr.owner}/${pr.repo}/collaborators/${encodeURIComponent(account)}/permission`, + { allow: [404] }, + ); + pushers.set( + account, + response.status !== 404 && PUSH.includes(string(object(await response.json()).permission)), + ); + } + return pushers.get(account)!; + }; + // A bot's reviews are its own account's; a named reviewer's body names it under a writer's account, which only a + // writer's own review may claim, so no one else can forge its review. + const ids = new Set(); + for (const review of pull.reviews) { + if (reviewerOf(review) !== reviewer || review.state === "DISMISSED" || review.state === "PENDING") continue; + const author = login(review.author); + if (reviewer.endsWith("[bot]") ? author === reviewer : author !== null && (await canPush(author))) + ids.add(String(review.id)); + } + const reviews = pull.reviews + .filter((review) => ids.has(String(review.id))) + .map((review) => ({ + commit: review.commit ? string(object(review.commit).oid) : null, + at: Date.parse(String(review.submittedAt)), + })) + .sort((a, b) => a.at - b.at); + + const open: string[] = []; + for (const thread of pull.reviewThreads) { + const first = array(object(thread.comments).nodes)[0]; + if (!first?.pullRequestReview || !ids.has(String(object(first.pullRequestReview).id))) continue; + const resolver = thread.isResolved === true ? login(thread.resolvedBy) : null; + if (resolver === null || (resolver !== reviewer && !(await canPush(resolver)))) + open.push(String(first.url ?? "")); + } + + const gate = (state: OnlineGate["state"], reviewed: string | null): OnlineGate => ({ + reviewer, + head, + state, + reviewed, + open, + }); + if (reviews.some((review) => review.commit === head)) return gate("current", head); + // A GitHub App's bot is `[bot]`; GitHub lists only each check's latest run. + const slug = /^(.+)\[bot\]$/.exec(reviewer)?.[1]; + let running = false; + if (slug !== undefined) { + for ( + let path: string | undefined = `/repos/${pr.owner}/${pr.repo}/commits/${head}/check-runs?per_page=100`; + path; + ) { + const response = await client.request(path); + for (const run of array(object(await response.json()).check_runs)) { + if (run.app == null || object(run.app).slug !== slug) continue; + if (run.status !== "completed") running = true; + else if (run.conclusion === "success") return gate("current", head); + } + path = link(response, "next"); + } + } + const last = reviews.filter((review) => review.commit !== null).at(-1); + if (last && (await sameDiff(client, pr, base, [head, last.commit!], warn))) + return gate("diff-identical", last.commit); + return gate(running ? "running" : "missing", null); +} + +/** Why `quest merge` refuses `gate`'s head, or null when the online reviewer's review lets it land. */ +export function onlineRefusal(gate: OnlineGate): string | null { + const who = `${gate.reviewer}, the online reviewer REVIEW.md names,`; + if (gate.state === "running") return `${who} is still reviewing it; land it once its check finishes`; + if (gate.state === "missing") + return `${who} has not reviewed it or a head with the same diff; land it once it has, asking it to review if it does not start`; + if (gate.open.length > 0) + return `${who} has findings no writer resolved: ${gate.open.join(", ")}; fix or refute each in its thread and resolve it, then land it again`; + return null; +} 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..cfbefb79 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -1,36 +1,29 @@ // `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"; import { SIZES, type Size, branchQuest, parse, size as questSize } from "../core"; -import { HARNESSES, type HarnessName, LINE_LIMIT, LOGINS, boundedLines, strippedEnvironment } from "./agent"; +import { 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. */ @@ -342,12 +335,10 @@ interface State { /** 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 +367,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 +391,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 +455,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 +599,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 +689,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 +723,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 +744,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 +769,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 +799,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 +827,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 +1223,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 +1235,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 +1268,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)) && @@ -1570,22 +1367,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,15 +1396,9 @@ 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); @@ -1638,16 +1416,15 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): 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 +1440,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`, ); @@ -1778,7 +1555,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 +1615,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 +1638,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 +1650,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 +1659,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 +1678,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 +1736,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/status.ts b/src/cli/status.ts index 9668769e..98351722 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -19,6 +19,7 @@ import { appOf, checksState, contextOutcome } from "../github/checks"; import { EJECTED, LAND_CHECK, pinned } from "./land"; import { reviewSection } from "../core"; import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; +import { type OnlineGate, declaredReviewer, onlineGate, sameDiff } from "./online-review"; /** * One query covers the PR; each connection pages by its own cursor, and a @@ -141,6 +142,8 @@ export interface Status { checkpoints: { reviewer: string; round: number; url: string }[]; /** Null when the `## Review` section names no full SHA. */ reviewRecord: ReviewRecord | null; + /** The online reviewer the base's REVIEW.md names, on an open PR; null when it names none. */ + onlineReview: OnlineGate | null; reactions: Reaction[]; /** Unresolved review threads, oldest first. */ threads: Thread[]; @@ -212,6 +215,15 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie : { checks: [], workflows: [] }; const status = summarize(first!, rollup, current, needs, reviewNodes, threadNodes); status.reviewRecord = await reviewRecord(github, pr, first!, deps.warn); + if (first!.state === "OPEN") { + const base = string(first!.baseRefName); + const reviewer = await declaredReviewer(github, pr, base); + if (reviewer !== null) { + status.onlineReview = await onlineGate(github, pr, reviewer, base, deps.warn); + if (status.onlineReview.head !== head) + throw new Error("the PR head changed while reading its online review; rerun quest status"); + } + } if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!, deps.warn); return { ...status, holds: ownership.holds, starterHold: ownership.starter }; } @@ -234,28 +246,10 @@ async function reviewRecord( if (!reviewed) return null; const head = string(pull.headRefOid); if (reviewed === head) return { state: "current", head: reviewed }; - const base = encodeURIComponent(string(pull.baseRefName)); - const diffs: string[] = []; - for (const sha of [head, reviewed]) { - const unknown = (why: string): ReviewRecord => { - warn(`cannot compare ${sha} with the base, so whether the review of ${reviewed} carries is unknown${why}`); - return { state: "unknown", head: reviewed }; - }; - let response: Response; - try { - response = await github.request(`/repos/${pr.owner}/${pr.repo}/compare/${base}...${sha}`, { - accept: "application/vnd.github.diff", - allow: [404, 406, 422], - }); - } catch (error) { - // A deadline is the caller's to handle; a forbidden or failing compare is not. - if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; - return unknown(`: ${error.message}`); - } - if (!response.ok) return unknown(""); - diffs.push(await response.text()); - } - return { state: diffs[0] === diffs[1] ? "diff-identical" : "stale", head: reviewed }; + const same = await sameDiff(github, pr, string(pull.baseRefName), [head, reviewed], (why) => + warn(`${why}, so whether the review of ${reviewed} carries is unknown`), + ); + return { state: same === null ? "unknown" : same ? "diff-identical" : "stale", head: reviewed }; } /** Repository activity types that move a branch to a new head, including merges into it. */ @@ -498,6 +492,7 @@ function summarize( reviews: [...latest.values()], checkpoints: checkpoints(head, submitted), reviewRecord: null, + onlineReview: null, reactions, threads, holds: [], @@ -584,6 +579,16 @@ export function statusLines(status: Status): string[] { status.reviewRecord === null ? "review-record -" : `review-record ${status.reviewRecord.state} ${status.reviewRecord.head}`, + status.onlineReview === null + ? "online-review -" + : [ + "online-review", + status.onlineReview.state, + status.onlineReview.reviewer, + dash(status.onlineReview.reviewed), + `open=${status.onlineReview.open.length}`, + ...status.onlineReview.open, + ].join(" "), status.autoMerge === null ? "auto-merge -" : `auto-merge ${status.autoMerge.method} ${status.autoMerge.enabledBy} ${status.autoMerge.enabledAt} ${dash(status.autoMerge.head)}`, 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/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..b7d8a963 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -63,6 +63,17 @@ class FakePull { /** The error GitHub answers `enqueuePullRequest` with, if any. */ beforeEnqueue?: () => { message: string; type?: string } | undefined; reads = 0; + /** The base's REVIEW.md, absent when null. */ + reviewMd: string | null = null; + /** The PR's reviews and review threads, as the online reviewer's activity read lists them. */ + reviews: ObjectValue[] = []; + threads: ObjectValue[] = []; + /** The head's check runs, as `GET /commits/{sha}/check-runs` lists them. */ + checkRuns: ObjectValue[] = []; + /** Each account's repository permission; absent ones are no collaborators. */ + permissions: Record = {}; + /** Each commit's diff from the base. */ + diffs: Record = {}; private clock = Date.parse("2026-10-21T12:00:00Z"); /** Add a label as GitHub does: re-adding one already on the PR records no event. */ @@ -118,6 +129,18 @@ class FakePull { if (url.pathname === "/repos/example/project/branches/main") return json(200, { protection: {} }); if (url.pathname === "/repos/example/project/actions/runs" && url.searchParams.get("head_sha") === this.head) return json(200, { total_count: this.runs.length, workflow_runs: this.runs }); + if (url.pathname === "/repos/example/project/contents/REVIEW.md" && url.searchParams.get("ref") === this.base) + return this.reviewMd === null ? json(404, { message: "Not Found" }) : new Response(this.reviewMd); + if (url.pathname === `/repos/example/project/commits/${this.head}/check-runs`) + return json(200, { total_count: this.checkRuns.length, check_runs: this.checkRuns }); + const permission = /^\/repos\/example\/project\/collaborators\/(.+)\/permission$/.exec(url.pathname); + if (permission) { + const level = this.permissions[decodeURIComponent(permission[1])]; + return level ? json(200, { permission: level }) : json(404, { message: "Not Found" }); + } + const diff = /^\/repos\/example\/project\/compare\/main\.\.\.(\w+)$/.exec(url.pathname); + if (diff && new Headers(init.headers).get("accept") === "application/vnd.github.diff") + return new Response(this.diffs[diff[1]] ?? ""); if (url.pathname === `/repos/example/project/compare/main...${this.head}`) { this.comparisons.push(`${url.pathname}${url.search}`); return json(200, { status: this.comparison, base_commit: { sha: this.base } }); @@ -164,6 +187,21 @@ class FakePull { return { data: { [name]: { clientMutationId: null } } }; } if (query.includes("node(id: $id)")) return { data: { node: { autoMergeRequest: this.autoMerge } } }; + if (query.includes("p7: pullRequest")) { + const page = (nodes: ObjectValue[]) => ({ pageInfo: { hasNextPage: false, endCursor: null }, nodes }); + return { + data: { + repository: { + p7: { + headRefOid: this.head, + reviews: page(this.reviews), + reviewThreads: page(this.threads), + comments: page([]), + }, + }, + }, + }; + } if (query.includes("statusCheckRollup")) return { data: { @@ -855,3 +893,115 @@ test("iterate and release exit 0 when stdout returns a value", async () => { vi.unstubAllGlobals(); } }); + +describe("merge --land with an online reviewer", () => { + const OLD = "c".repeat(40); + const bot = { __typename: "Bot", login: "pullfrog" }; + const review = (id: string, commit: string, author: ObjectValue = bot, body = "Reviewed.") => ({ + id, + url: `${URL_}#pullrequestreview-${id}`, + state: "COMMENTED", + author, + body, + submittedAt: `2026-10-21T11:0${id}:00Z`, + commit: { oid: commit }, + }); + const thread = (id: string, resolvedBy: ObjectValue | null = null) => ({ + id: `T_${id}`, + isResolved: resolvedBy !== null, + resolvedBy, + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [{ author: bot, body: "A bug.", url: `${URL_}#discussion_r${id}`, pullRequestReview: { id } }], + }, + }); + const run = (status: string, conclusion: string | null, slug = "pullfrog") => ({ + name: slug, + status, + conclusion, + app: { slug }, + }); + const declared = () => { + const pull = new FakePull(); + pull.reviewMd = "---\nonline-reviewer: pullfrog[bot]\n---\n\nBe adversarial.\n"; + return pull; + }; + const merge = (pull: FakePull) => land(PR, { sha: HEAD, method: "merge", check: clean }, pull.deps()); + + test("lands on the local review alone when the base's REVIEW.md names no online reviewer", async () => { + const pull = new FakePull(); + pull.reviewMd = "Be adversarial.\n"; + await merge(pull); + expect(pull.writes).not.toEqual([]); + }); + + test("refuses a head it has not reviewed, and one it is still reviewing", async () => { + const pull = declared(); + expect(await refused(merge(pull))).toBe( + `${URL_} head ${HEAD}: pullfrog[bot], the online reviewer REVIEW.md names, has not reviewed it or a head with the same diff; land it once it has, asking it to review if it does not start`, + ); + // Another app's check says nothing of this reviewer. + pull.checkRuns = [run("in_progress", null), run("completed", "success", "coderabbitai")]; + expect(await refused(merge(pull))).toContain("is still reviewing it"); + pull.checkRuns = [run("completed", "cancelled")]; + expect(await refused(merge(pull))).toContain("has not reviewed it"); + expect(pull.writes).toEqual([]); + }); + + test("counts a review of the head, or its own app's successful check run on it", async () => { + const checked = declared(); + checked.checkRuns = [run("completed", "success")]; + await merge(checked); + expect(checked.writes).not.toEqual([]); + const reviewed = declared(); + reviewed.reviews = [review("1", HEAD)]; + await merge(reviewed); + expect(reviewed.writes).not.toEqual([]); + }); + + test("counts no review another account posts in its name", async () => { + const pull = declared(); + pull.permissions = { outsider: "read" }; + pull.reviews = [ + review("1", HEAD, { __typename: "User", login: "outsider" }, "Automated review by pullfrog[bot]\n\nFine."), + ]; + expect(await refused(merge(pull))).toContain("has not reviewed it"); + }); + + test("counts an earlier head's review only while the head's diff from the base matches it", async () => { + const same = declared(); + same.reviews = [review("1", OLD)]; + same.diffs = { [HEAD]: "diff", [OLD]: "diff" }; + await merge(same); + expect(same.writes).not.toEqual([]); + const changed = declared(); + changed.reviews = [review("1", OLD)]; + changed.diffs = { [HEAD]: "new", [OLD]: "old" }; + expect(await refused(merge(changed))).toContain("has not reviewed it"); + }); + + test("refuses while a thread it opened on any head is unresolved, or resolved by an account that cannot push", async () => { + const pull = declared(); + pull.reviews = [review("1", OLD), review("2", HEAD)]; + pull.threads = [thread("1")]; + expect(await refused(merge(pull))).toContain( + `has findings no writer resolved: ${URL_}#discussion_r1; fix or refute each in its thread and resolve it`, + ); + // GitHub lets the PR's author resolve a thread without write access. + pull.threads = [thread("1", { __typename: "User", login: "author" })]; + pull.permissions = { author: "read" }; + expect(await refused(merge(pull))).toContain(`${URL_}#discussion_r1`); + expect(pull.writes).toEqual([]); + pull.permissions = { author: "write" }; + await merge(pull); + expect(pull.writes).not.toEqual([]); + }); + + test("a thread it resolved itself is settled", async () => { + const pull = declared(); + pull.reviews = [review("1", HEAD)]; + pull.threads = [thread("1", bot)]; + await merge(pull); + expect(pull.writes).not.toEqual([]); + }); +}); 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/online-review.test.ts b/test/online-review.test.ts index 25faf4e7..504ceb52 100644 --- a/test/online-review.test.ts +++ b/test/online-review.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "vitest"; -import { type Activity, bodyFindings, classify, onlineReviews } from "../src/cli/online-review"; +import { type Activity, bodyFindings, classify, onlineReviews, reviewerIn } from "../src/cli/online-review"; const HEAD = "h".repeat(40); const bot = (login: string) => ({ __typename: "Bot", login }); @@ -156,3 +156,24 @@ describe("onlineReviews", () => { ]); }); }); + +describe("reviewerIn", () => { + test("reads the online reviewer from REVIEW.md's front matter", () => { + expect(reviewerIn("---\nonline-reviewer: pullfrog[bot]\n---\n\nBe adversarial.")).toBe("pullfrog[bot]"); + expect(reviewerIn("---\r\ntitle: Review\r\nonline-reviewer: 'review (OpenAI)'\r\n---\r\n")).toBe( + "review (OpenAI)", + ); + }); + + test("names none without front matter or the key, even when the body mentions it", () => { + expect(reviewerIn("# Review\n\nonline-reviewer: pullfrog[bot]\n")).toBeNull(); + expect(reviewerIn("---\ntitle: Review\n---\nonline-reviewer: pullfrog[bot]\n")).toBeNull(); + }); + + test("refuses an empty or repeated name, so a typo cannot drop the gate", () => { + expect(() => reviewerIn("---\nonline-reviewer:\n---\n")).toThrow("is empty"); + expect(() => reviewerIn("---\nonline-reviewer: a[bot]\nonline-reviewer: b[bot]\n---\n")).toThrow( + "more than once", + ); + }); +}); diff --git a/test/review.test.ts b/test/review.test.ts index b4ff46ab..30e7cc83 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 }, @@ -2088,14 +1848,13 @@ test("saved sessions are read only in the current format, or refused naming --ne expect(parseState(shadowed)).toEqual(shadowed); 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: [] }, @@ -2126,53 +1885,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 +1915,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 +1930,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/status.test.ts b/test/status.test.ts index c38be319..40ce8798 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -124,6 +124,10 @@ function setup( noBase?: boolean; /** The head's workflow runs, as `GET /actions/runs` lists them. */ runs?: Node[]; + /** The base's REVIEW.md, absent when unset. */ + reviewMd?: string; + /** The PR's reviews, as the online reviewer's activity read lists them, and the head's check runs. */ + online?: { reviews: Node[]; checkRuns: Node[] }; } = {}, ) { const queries: Record[] = []; @@ -171,6 +175,12 @@ function setup( ? Response.json({ message: "No diff" }, { status: diff }) : new Response(diff); } + if (path.pathname === "/repos/example/project/contents/REVIEW.md") + return options.reviewMd === undefined + ? Response.json({ message: "Not Found" }, { status: 404 }) + : new Response(options.reviewMd); + if (path.pathname === `/repos/example/project/commits/${A}/check-runs`) + return Response.json({ check_runs: options.online?.checkRuns ?? [] }); if (path.pathname !== "/graphql") { expect(path.pathname).toBe("/repos/example/project/issues/1/events"); eventRequests.push(String(url)); @@ -179,6 +189,21 @@ function setup( return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); } const request = JSON.parse(String(init?.body)); + if (request.query.includes("p1: pullRequest")) { + const nodes = (rows: Node[]) => ({ pageInfo: { hasNextPage: false, endCursor: null }, nodes: rows }); + return Response.json({ + data: { + repository: { + p1: { + headRefOid: A, + reviews: nodes(options.online?.reviews ?? []), + reviewThreads: nodes([]), + comments: nodes([]), + }, + }, + }, + }); + } if (request.query.includes("query HoldOwnership(")) { holdQueries.push(request.variables); if (!last) throw new Error("ownership read before PR state"); @@ -260,6 +285,7 @@ test("reports checks by requiredness, each reviewer's latest review against the "merge-state blocked", "review-decision review_required", "review-record -", + "online-review -", "auto-merge -", "merge-queue -", "landing-stopped -", @@ -296,13 +322,14 @@ test("a queued PR with auto-merge and no checks reports both, and JSON is determ threads: page([]), }); const status = await pullRequestStatus(pr, setup([body]).deps); - expect(statusLines(status).slice(2, 12)).toEqual([ + expect(statusLines(status).slice(2, 13)).toEqual([ "draft true", `head ${A}`, "mergeable mergeable", "merge-state blocked", "review-decision -", "review-record -", + "online-review -", "auto-merge squash kixelated 2026-10-07T12:00:00Z -", "merge-queue awaiting_checks 2 2026-10-07T12:01:00Z", "landing-stopped -", @@ -926,6 +953,30 @@ test("refuses a head moved between the paged state and ownership read", async () await expect(pullRequestStatus(pr, fixture.deps)).rejects.toThrow("head changed while reading its holds"); }); +describe("the online review", () => { + const line = async (options: Parameters[1] = {}) => { + const fixture = setup([pull({ checks: null, reviews: page([]), threads: page([]) })], options); + return statusLines(await pullRequestStatus(pr, fixture.deps)).find((l) => l.startsWith("online-review ")); + }; + const reviewMd = "---\nonline-reviewer: pullfrog[bot]\n---\n"; + + test("is absent when the base's REVIEW.md names no online reviewer", async () => { + expect(await line()).toBe("online-review -"); + expect(await line({ reviewMd: "Be adversarial.\n" })).toBe("online-review -"); + }); + + test("reports where the named reviewer stands on the head", async () => { + expect(await line({ reviewMd })).toBe("online-review missing pullfrog[bot] - open=0"); + const run = (status: string, conclusion: string | null) => ({ status, conclusion, app: { slug: "pullfrog" } }); + expect(await line({ reviewMd, online: { reviews: [], checkRuns: [run("in_progress", null)] } })).toBe( + "online-review running pullfrog[bot] - open=0", + ); + expect(await line({ reviewMd, online: { reviews: [], checkRuns: [run("completed", "success")] } })).toBe( + `online-review current pullfrog[bot] ${A} open=0`, + ); + }); +}); + describe("the review record", () => { const C = "c".repeat(40); const line = async (body: string, options: Parameters[1] = {}) => { 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, From 9bca43d585fa7a678f3bf4b76bf23a07e7a88b14 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 11:21:22 -0800 Subject: [PATCH 2/8] fix(merge): count only review passes, keep dismissed reviews' threads, ignore Actions checks Co-Authored-By: Claude Opus 5.5 --- README.md | 5 ++++- src/cli/online-review.ts | 23 +++++++++++++++++++---- test/hold.test.ts | 28 ++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 3eab4128..d84eea5f 100644 --- a/README.md +++ b/README.md @@ -648,7 +648,10 @@ whose reviews then count only from accounts that can push. `quest merge` refuses a head until that reviewer has reviewed it, in a review of the head or a successful check run of its own GitHub App on it (a re-review that finds nothing new may post only the check), or reviewed an earlier head whose diff -from the base matches. It also refuses while any review thread the reviewer +from the base matches. A review that only replies in an older thread, or that +was dismissed, reviews nothing, and a reviewer posting as +`github-actions[bot]` counts only by its reviews, since that app runs every +workflow job. It also refuses while any review thread the reviewer opened, on any head, is unresolved, or was resolved by an account that cannot push, since GitHub lets the PR's author resolve one: fix or refute each finding in its thread and resolve it. Findings it lists only in a review body are not diff --git a/src/cli/online-review.ts b/src/cli/online-review.ts index d59d1ac9..131b69b6 100644 --- a/src/cli/online-review.ts +++ b/src/cli/online-review.ts @@ -403,15 +403,29 @@ export async function onlineGate( }; // A bot's reviews are its own account's; a named reviewer's body names it under a writer's account, which only a // writer's own review may claim, so no one else can forge its review. + // Its own reviews, dismissed ones included, whose threads stay its findings; a pending one is no one else's to see. const ids = new Set(); for (const review of pull.reviews) { - if (reviewerOf(review) !== reviewer || review.state === "DISMISSED" || review.state === "PENDING") continue; + if (reviewerOf(review) !== reviewer || review.state === "PENDING") continue; const author = login(review.author); if (reviewer.endsWith("[bot]") ? author === reviewer : author !== null && (await canPush(author))) ids.add(String(review.id)); } + const opened = new Set( + pull.reviewThreads.flatMap((thread) => { + const review = array(object(thread.comments).nodes)[0]?.pullRequestReview; + return review ? [String(object(review).id)] : []; + }), + ); + // A pass over a head has a body or opens a thread; a reply in an older thread also makes an empty review of the + // head without reviewing it, and a dismissed review is withdrawn. const reviews = pull.reviews - .filter((review) => ids.has(String(review.id))) + .filter( + (review) => + ids.has(String(review.id)) && + review.state !== "DISMISSED" && + (String(review.body ?? "").trim() !== "" || opened.has(String(review.id))), + ) .map((review) => ({ commit: review.commit ? string(object(review.commit).oid) : null, at: Date.parse(String(review.submittedAt)), @@ -435,10 +449,11 @@ export async function onlineGate( open, }); if (reviews.some((review) => review.commit === head)) return gate("current", head); - // A GitHub App's bot is `[bot]`; GitHub lists only each check's latest run. + // A GitHub App's bot is `[bot]`; GitHub lists only each check's latest run. GitHub Actions' app runs every + // workflow job, so its checks say nothing of a review. const slug = /^(.+)\[bot\]$/.exec(reviewer)?.[1]; let running = false; - if (slug !== undefined) { + if (slug !== undefined && slug !== "github-actions") { for ( let path: string | undefined = `/repos/${pr.owner}/${pr.repo}/commits/${head}/check-runs?per_page=100`; path; diff --git a/test/hold.test.ts b/test/hold.test.ts index b7d8a963..fd60b1b0 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -997,6 +997,34 @@ describe("merge --land with an online reviewer", () => { expect(pull.writes).not.toEqual([]); }); + test("counts no reply in an older thread as a review of the head", async () => { + const pull = declared(); + // Replying makes an empty review of the head that opens no thread. + pull.reviews = [review("1", OLD), review("2", HEAD, bot, "")]; + pull.diffs = { [HEAD]: "new", [OLD]: "old" }; + pull.threads = [thread("1", { __typename: "User", login: "maintainer" })]; + pull.permissions = { maintainer: "admin" }; + expect(await refused(merge(pull))).toContain("has not reviewed it"); + }); + + test("keeps a dismissed review's unresolved threads open, though the review no longer counts", async () => { + const pull = declared(); + pull.reviews = [{ ...review("1", HEAD), state: "DISMISSED" }]; + pull.threads = [thread("1")]; + pull.checkRuns = [run("completed", "success")]; + expect(await refused(merge(pull))).toContain(`${URL_}#discussion_r1`); + pull.threads = []; + pull.checkRuns = []; + expect(await refused(merge(pull))).toContain("has not reviewed it"); + }); + + test("counts no GitHub Actions check for a reviewer posting as github-actions[bot], since every job is one", async () => { + const pull = new FakePull(); + pull.reviewMd = "---\nonline-reviewer: github-actions[bot]\n---\n"; + pull.checkRuns = [run("completed", "success", "github-actions")]; + expect(await refused(merge(pull))).toContain("has not reviewed it"); + }); + test("a thread it resolved itself is settled", async () => { const pull = declared(); pull.reviews = [review("1", HEAD)]; From ccfd5b302b7b195e64227c428b1ae791bb00712c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 11:27:32 -0800 Subject: [PATCH 3/8] fix(merge): count verdicts without a body, and earlier heads only its app's check passed Co-Authored-By: Claude Opus 5.5 --- src/cli/online-review.ts | 49 +++++++++++++++++++++++++++++++++++----- test/hold.test.ts | 43 +++++++++++++++++++++++++++++++++++ test/status.test.ts | 8 +++++++ 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/src/cli/online-review.ts b/src/cli/online-review.ts index 131b69b6..445ef22f 100644 --- a/src/cli/online-review.ts +++ b/src/cli/online-review.ts @@ -417,14 +417,17 @@ export async function onlineGate( return review ? [String(object(review).id)] : []; }), ); - // A pass over a head has a body or opens a thread; a reply in an older thread also makes an empty review of the - // head without reviewing it, and a dismissed review is withdrawn. + // A pass over a head gives a verdict, has a body, or opens a thread; a reply in an older thread also makes an empty + // comment review of the head without reviewing it, and a dismissed review is withdrawn. const reviews = pull.reviews .filter( (review) => ids.has(String(review.id)) && review.state !== "DISMISSED" && - (String(review.body ?? "").trim() !== "" || opened.has(String(review.id))), + (review.state === "APPROVED" || + review.state === "CHANGES_REQUESTED" || + String(review.body ?? "").trim() !== "" || + opened.has(String(review.id))), ) .map((review) => ({ commit: review.commit ? string(object(review.commit).oid) : null, @@ -467,12 +470,46 @@ export async function onlineGate( path = link(response, "next"); } } - const last = reviews.filter((review) => review.commit !== null).at(-1); - if (last && (await sameDiff(client, pr, base, [head, last.commit!], warn))) - return gate("diff-identical", last.commit); + const earlier = await earlierReviewed(client, pr, head, slug === "github-actions" ? undefined : slug, reviews); + if (earlier !== null && (await sameDiff(client, pr, base, [head, earlier], warn))) + return gate("diff-identical", earlier); return gate(running ? "running" : "missing", null); } +/** + * The newest earlier head of `pr` the reviewer reviewed: among the PR's last 100 commits, the newest before the head + * with one of its `reviews` or a successful check suite of its app `slug`, else its latest review's commit. + */ +async function earlierReviewed( + client: GitHub, + pr: PullRequest, + head: string, + slug: string | undefined, + reviews: { commit: string | null }[], +): Promise { + const last = reviews.filter((review) => review.commit !== null).at(-1)?.commit ?? null; + if (slug === undefined) return last; + const data = await client.graphql( + "query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { " + + "pullRequest(number: $number) { commits(last: 100) { nodes { commit { oid " + + "checkSuites(first: 50) { nodes { conclusion app { slug } } } } } } } } }", + { owner: pr.owner, name: pr.repo, number: pr.number }, + ); + const commits = array(object(object(object(object(data.repository).pullRequest).commits)).nodes).map((node) => + object(node.commit), + ); + const reviewed = new Set(reviews.flatMap((review) => (review.commit === null ? [] : [review.commit]))); + for (const commit of commits.reverse()) { + const oid = string(commit.oid); + if (oid === head) continue; + const checked = array(object(commit.checkSuites ?? { nodes: [] }).nodes).some( + (suite) => suite.conclusion === "SUCCESS" && suite.app != null && object(suite.app).slug === slug, + ); + if (checked || reviewed.has(oid)) return oid; + } + return last; +} + /** Why `quest merge` refuses `gate`'s head, or null when the online reviewer's review lets it land. */ export function onlineRefusal(gate: OnlineGate): string | null { const who = `${gate.reviewer}, the online reviewer REVIEW.md names,`; diff --git a/test/hold.test.ts b/test/hold.test.ts index fd60b1b0..dc2eeee2 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -74,6 +74,9 @@ class FakePull { permissions: Record = {}; /** Each commit's diff from the base. */ diffs: Record = {}; + /** The PR's commits before its head, oldest first, and the apps whose check suites succeeded on each. */ + history: string[] = []; + suites: Record = {}; private clock = Date.parse("2026-10-21T12:00:00Z"); /** Add a label as GitHub does: re-adding one already on the PR records no event. */ @@ -187,6 +190,21 @@ class FakePull { return { data: { [name]: { clientMutationId: null } } }; } if (query.includes("node(id: $id)")) return { data: { node: { autoMergeRequest: this.autoMerge } } }; + if (query.includes("commits(last: 100)")) { + const commit = (oid: string) => ({ + commit: { + oid, + checkSuites: { + nodes: (this.suites[oid] ?? []).map((slug) => ({ conclusion: "SUCCESS", app: { slug } })), + }, + }, + }); + return { + data: { + repository: { pullRequest: { commits: { nodes: [...this.history, this.head].map(commit) } } }, + }, + }; + } if (query.includes("p7: pullRequest")) { const page = (nodes: ObjectValue[]) => ({ pageInfo: { hasNextPage: false, endCursor: null }, nodes }); return { @@ -980,6 +998,31 @@ describe("merge --land with an online reviewer", () => { expect(await refused(merge(changed))).toContain("has not reviewed it"); }); + test("counts an approval without a body as a review of the head", async () => { + const pull = declared(); + pull.reviews = [{ ...review("1", HEAD, bot, ""), state: "APPROVED" }]; + await merge(pull); + expect(pull.writes).not.toEqual([]); + }); + + test("compares with the newest earlier head its app's check passed on, not only its reviews", async () => { + const MID = "d".repeat(40); + const pull = declared(); + pull.reviews = [review("1", OLD)]; + pull.history = [OLD, MID]; + pull.suites = { [MID]: ["pullfrog"] }; + pull.diffs = { [OLD]: "old", [MID]: "fixed", [HEAD]: "fixed" }; + await merge(pull); + expect(pull.writes).not.toEqual([]); + // Another app's passing suite reviewed nothing. + const other = declared(); + other.reviews = [review("1", OLD)]; + other.history = [OLD, MID]; + other.suites = { [MID]: ["coderabbitai"] }; + other.diffs = { [OLD]: "old", [MID]: "fixed", [HEAD]: "fixed" }; + expect(await refused(merge(other))).toContain("has not reviewed it"); + }); + test("refuses while a thread it opened on any head is unresolved, or resolved by an account that cannot push", async () => { const pull = declared(); pull.reviews = [review("1", OLD), review("2", HEAD)]; diff --git a/test/status.test.ts b/test/status.test.ts index 40ce8798..b41f7897 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -189,6 +189,14 @@ function setup( return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); } const request = JSON.parse(String(init?.body)); + if (request.query.includes("commits(last: 100)")) + return Response.json({ + data: { + repository: { + pullRequest: { commits: { nodes: [{ commit: { oid: A, checkSuites: { nodes: [] } } }] } }, + }, + }, + }); if (request.query.includes("p1: pullRequest")) { const nodes = (rows: Node[]) => ({ pageInfo: { hasNextPage: false, endCursor: null }, nodes: rows }); return Response.json({ From bedc404cdcd6b357086446bc078366a8f3590c43 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 11:38:35 -0800 Subject: [PATCH 4/8] refactor(merge)!: gate on the base's required approval, not REVIEW.md The maintainer moved the online review gate to a GitHub ruleset that requires Pullfrog's approval, so the REVIEW.md declaration, its quest merge refusal, and the online-review status line are gone. quest merge now honors a required approval the PR lacks even on an admin's token: it only arms auto-merge, which waits for it, and refuses where nothing would wait. Co-Authored-By: Claude Opus 5.5 --- CONTRIBUTING.md | 1 + README.md | 43 +----- REVIEW.md | 3 - assets/CONTEXT.md | 4 +- assets/skills/merge.md | 3 +- assets/skills/run.md | 2 +- docs/context.md | 4 +- quest/m0/pullfrog-setup.md | 6 +- quest/m0/token-waste/README.md | 7 +- src/cli/hold.ts | 37 +++-- src/cli/main.ts | 16 +-- src/cli/online-review.ts | 251 +-------------------------------- src/cli/status.ts | 49 +++---- test/hold.test.ts | 247 +++++--------------------------- test/online-review.test.ts | 23 +-- test/status.test.ts | 61 +------- 16 files changed, 106 insertions(+), 651 deletions(-) delete mode 100644 REVIEW.md 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 d84eea5f..3aa73c06 100644 --- a/README.md +++ b/README.md @@ -483,7 +483,6 @@ mergeable conflicting merge-state dirty review-decision - review-record diff-identical 5f1c2e9a8b7d6c5e4f3a2b1c0d9e8f7a6b5c4d3e -online-review current pullfrog[bot] 0ade9d977d54c6f48152410532c58e9ea78db3d0 open=0 auto-merge - merge-queue - landing-stopped - @@ -502,7 +501,6 @@ comes last, and a missing value prints as `-`: | `url`, `state`, `draft`, `head` | The PR URL; `open`, `closed`, or `merged`; `true` or `false`; the head SHA. | | `mergeable`, `merge-state`, `review-decision` | GitHub's values, lowercased. | | `review-record` | Whether the PR body's [`## Review` record](#review-locally) still covers the head, and the head it names last: `current` when it is the head, `diff-identical` when `git diff ...` prints the same as for it, `stale` otherwise, or `unknown` when GitHub cannot compare them, such as a diff too large; `-` when the record names no full SHA. Whose record counts is the reader's call. | -| `online-review` | Where the [online reviewer](#require-an-online-review) the base's `REVIEW.md` names stands on an open PR's head, as `quest merge` reads it: `current` when it reviewed the head, `diff-identical` when it reviewed a head whose diff from the base matches, `running` while its app's check on the head has not finished, or `missing`; then the reviewer, the head it reviewed, and `open=` with the URL of each review thread it opened that neither it nor a writer resolved. `-` when the base names none. | | `auto-merge` | Method, who enabled it, when, and the head it was enabled on. | | `merge-queue` | Queue state, position, and when it was enqueued. | | `landing-stopped` | The reason the latest merge-queue entry, auto-merge, or [`quest land`](#land-a-batch-with-quest-land) queue entry ended (such as `merge_conflict`, `failed_checks`, `manual`, `manually_disabled`, or `merged`), who ended it, and when; `-` once the PR has entered the merge queue or auto-merge again since. | @@ -533,7 +531,7 @@ requirements from the base, matches a required check by name and, when the requirement names one, its app, as GitHub and `quest land` do, and leaves out `quest land`'s check, which no head carries. `--json` prints the same facts as one object with the fields `url`, `state`, `draft`, `head`, -`mergeable`, `mergeState`, `reviewDecision`, `reviewRecord`, `onlineReview` (`reviewer`, `head`, `state`, `reviewed`, `open`, or null), `autoMerge`, `mergeQueue`, +`mergeable`, `mergeState`, `reviewDecision`, `autoMerge`, `mergeQueue`, `landingStopped`, `checks` (`state`, `passed`, `failing`, `pending`), `reviews`, `checkpoints` (`reviewer`, `round`, `url`), `reactions`, `threads`, `holds` (`label`, `token`, `takenAt`, `age`, `eventUrl`), and `starterHold` (`lastActive`, `age`, `url`, or null). Ownership uses the state commands' shared reader without changing labels, and event links match the opaque acquisition token against GitHub's event listing. Long lists are paged; reactions are the newest 100 per PR or review, as in `quest wait`. Any PR state exits 0, invalid arguments exit 2, and other @@ -582,9 +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 refuses a head the base's online reviewer has not reviewed, or -whose findings are open, as [Require an online review](#require-an-online-review) -describes. 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 @@ -630,37 +630,6 @@ only what needs a maintainer: a behavior or design decision gets a diagnosis on PR, and a fix that edits protected paths or that GitHub refuses to push gets a ready commit for `quest land apply `. -### Require an online review - -A repository names the online reviewer whose review gates `quest merge`, such as -[Pullfrog](https://pullfrog.com), in its `REVIEW.md` front matter, read from the -base so a PR cannot drop its own gate: - -```markdown ---- -online-reviewer: pullfrog[bot] ---- -``` - -The name is the reviewer's GitHub login, or the name a review body opens with -as `Automated review by ` for a reviewer posting under a person's account, -whose reviews then count only from accounts that can push. `quest merge` -refuses a head until that reviewer has reviewed it, in a review of the head or -a successful check run of its own GitHub App on it (a re-review that finds -nothing new may post only the check), or reviewed an earlier head whose diff -from the base matches. A review that only replies in an older thread, or that -was dismissed, reviews nothing, and a reviewer posting as -`github-actions[bot]` counts only by its reviews, since that app runs every -workflow job. It also refuses while any review thread the reviewer -opened, on any head, is unresolved, or was resolved by an account that cannot -push, since GitHub lets the PR's author resolve one: fix or refute each finding -in its thread and resolve it. Findings it lists only in a review body are not -read. A reviewer that has not started, or whose run failed, holds the PR until -it reviews the head; ask it to, as its documentation says. A repository whose -`REVIEW.md` names none lands on its local review alone, the -[`quest review`](#review-locally) round its PR body records. Other online -reviewers, such as a capped CodeRabbit, are never required to review. - ## Land a batch with quest land `quest land serve` runs on a maintainer's host, with one process per base branch of a repository. diff --git a/REVIEW.md b/REVIEW.md deleted file mode 100644 index 013ba927..00000000 --- a/REVIEW.md +++ /dev/null @@ -1,3 +0,0 @@ ---- -online-reviewer: pullfrog[bot] ---- diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index 2bced6b4..f1e8c09b 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -68,9 +68,9 @@ Merge the base only when the branch conflicts with it or a landing step, such as Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. -Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings posted on the head and reply to feedback you disagree with. `quest merge` needs the review of the online reviewer the base's `REVIEW.md` names, if any; no other online review is required. +Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. Request a review only from a reviewer whose approval the base requires and that never ran on the head. -Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since an open thread holds the merge. +Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since such a reviewer may approve only after its threads are resolved. The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index 4453bcd8..f17896e7 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -15,12 +15,11 @@ The head is landable when: - the PR is not a draft; - a review counts for it, as below; -- when the base's `REVIEW.md` names an online reviewer, its `online-review` line is `current` or `diff-identical` with `open=0`; - every review finding posted on it, including automated reviewers' such as CodeRabbit or Codex, is fixed on the head or answered with a refutation, and no decision is open; - no check failed on it, required or not, its required checks passed, other than the gate's own `quest land` check, which never reports on a head, and it does not conflict with its base; - no review requests changes; an approval its base requires may still be to come, since the queue waits for it. -While its required checks are only pending, or its online review `running` or `missing`, report that the pick stands until they finish; do not wait for them. +While its required checks are only pending, report that the pick stands until they pass; do not wait for them. A change request that stands once its findings are fixed or refuted and their threads resolved is the user's call. Any other head needs `/quest-iterate`: run it, saying what the head needs, such as a review, fixes, a failed check fixed or rerun, flaky ones included, or decisions, then run this skill again once it finishes; if it stops for the user instead, report that. Run it in this session, where its interview can ask the user; under a coordinator such as `/quest-run`, return the PR to the loop, which dispatches it in a sub-agent. diff --git a/assets/skills/run.md b/assets/skills/run.md index 8817a783..573d4588 100644 --- a/assets/skills/run.md +++ b/assets/skills/run.md @@ -40,7 +40,7 @@ Run each `/quest-merge` inline; dispatch each `/quest-iterate` and `/quest-revie Dispatch `/quest-iterate` without asking for a PR that needs work but no decision, such as a conflict, failing checks, unaddressed findings, or no review that counts for its head as `/quest-merge` defines. A PR with open decisions, its `## Waiting` section's or an issue you would solve differently, goes to the user: settle them with the grill skill's rounds, then dispatch `/quest-iterate` with the answers in its brief, since a sub-agent cannot prompt. A PR you would skip or delete rather than fix, such as a superseded one, gets that question instead. -Leave a PR whose required checks or online review are still running for a later round; no other automated reviewer is waited on, since the queue waits for an approval the base requires. +Leave a PR whose required checks are still running for a later round; an automated reviewer is not waited on, since the queue waits for an approval the base requires. A PR whose author has write access and that changes no user-facing behavior carries a standing `/quest-merge` pick, unless the repository's `CONTRIBUTING.md` opts out: run `/quest-merge` on it without asking, and name it as auto-picked in the summary. That covers only tests, benchmarks, CI, internal docs or comments, and behavior-preserving code. Anything users or other repositories get, such as CLI behavior or output, shipped skills or rules, user-facing docs, dependencies, or release artifacts, needs the user's pick, as does adding or expanding quests. diff --git a/docs/context.md b/docs/context.md index f77881ce..31a6a257 100644 --- a/docs/context.md +++ b/docs/context.md @@ -68,9 +68,9 @@ Merge the base only when the branch conflicts with it or a landing step, such as Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. -Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings posted on the head and reply to feedback you disagree with. `quest merge` needs the review of the online reviewer the base's `REVIEW.md` names, if any; no other online review is required. +Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. Request a review only from a reviewer whose approval the base requires and that never ran on the head. -Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since an open thread holds the merge. +Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since such a reviewer may approve only after its threads are resolved. The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. diff --git a/quest/m0/pullfrog-setup.md b/quest/m0/pullfrog-setup.md index ecfbeec6..3bc8f2da 100644 --- a/quest/m0/pullfrog-setup.md +++ b/quest/m0/pullfrog-setup.md @@ -17,9 +17,9 @@ 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 review gates `quest merge` wherever the base's -`REVIEW.md` names it, in place of the DeepSeek review, so what its runs can -check now decides merges. +Since 2026-11-13 its approval, which a `Pullfrog approval` ruleset requires +on moq-dev/quest and moq-dev/moq, 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/token-waste/README.md b/quest/m0/token-waste/README.md index 72ba36dc..6146002c 100644 --- a/quest/m0/token-waste/README.md +++ b/quest/m0/token-waste/README.md @@ -45,9 +45,10 @@ Decision prompts, chosen options marked ✅: [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 ✅ the - online reviewer the base's `REVIEW.md` names gates `quest merge` - (rejected: advisory only). + 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/src/cli/hold.ts b/src/cli/hold.ts index 82f69dc7..d75e7592 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -20,7 +20,6 @@ import { import { ALLOWS, LAND_CHECK, METHOD_NAMES, contexts, enqueue } from "./land"; import { QUEUE_CHECK } from "./land-queue"; import type { LandingCheck } from "./landed"; -import { declaredReviewer, onlineGate, onlineRefusal } from "./online-review"; import { classifyCheck } from "./status"; import { Refusal, ago } from "./start"; @@ -30,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 } } } @@ -60,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; @@ -254,7 +258,6 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe } }; await passing(labels.github, pr, snapshot.url, sha); - await reviewed(labels.github, pr, snapshot, deps.warn); const findings = await request.check({ remote: `https://github.com/${pr.owner}/${pr.repo}.git`, base: snapshot.base, @@ -299,6 +302,10 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe `${snapshot.url} repository has auto-merge disabled; follow its landing instructions in CONTRIBUTING.md or AGENTS.md (for example, its label queue). Only use --direct if the user explicitly asks`, ); } + if (snapshot.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`, + ); // The batch gate and a merge queue test the current base themselves. Here nothing does, so the head must contain it. const comparison = object( await ( @@ -322,7 +329,10 @@ 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 ( + !snapshot.unapproved && + (repository.allow_auto_merge !== true || immediateMerge(snapshot.mergeState, snapshot.queue)) + ) { await submit("mergePullRequest"); return; } @@ -337,6 +347,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) @@ -363,20 +374,6 @@ async function passing(github: GitHub, pr: PullRequest, url: string, sha: string ); } -/** - * Refuse a head the online reviewer the base's REVIEW.md names has not reviewed, nor a head with the same diff, or - * whose findings it opened on the PR are not all resolved. A base that names none lands on its local review alone. - */ -async function reviewed(github: GitHub, pr: PullRequest, snapshot: Snapshot, warn: (message: string) => void) { - const reviewer = await declaredReviewer(github, pr, snapshot.baseSha); - if (reviewer === null) return; - const gate = await onlineGate(github, pr, reviewer, snapshot.base, warn); - if (gate.head !== snapshot.head) - throw new Refusal(`${snapshot.url} head moved to ${gate.head}; check the new head before landing it`); - const refusal = onlineRefusal(gate); - if (refusal !== null) throw new Refusal(`${snapshot.url} head ${snapshot.head}: ${refusal}`); -} - /** GraphQL error types that fault the token or the request, not the PR GitHub would enqueue. */ export const UNREFUSED = ["FORBIDDEN", "NOT_FOUND", "INSUFFICIENT_SCOPES", "INTERNAL"]; @@ -393,7 +390,8 @@ async function pick(labels: Labels, snapshot: Snapshot, method: Method): Promise (query, variables) => labels.github.graphql(query, variables), snapshot.id, snapshot.head, - snapshot.mergeState, + // A missing approval is pending like a check, so it arms auto-merge, which waits for it, and never enqueues. + snapshot.unapproved ? "BLOCKED" : snapshot.mergeState, method, ); } catch (error) { @@ -562,6 +560,7 @@ class Labels { branch: string(row.headRefName), head: string(row.headRefOid), mergeState: string(row.mergeStateStatus), + unapproved: row.reviewDecision === "REVIEW_REQUIRED" || row.reviewDecision === "CHANGES_REQUESTED", queue: row.isMergeQueueEnabled === true, queued: row.isInMergeQueue === true, labels: array(object(row.labels).nodes) diff --git a/src/cli/main.ts b/src/cli/main.ts index 198d76f6..743bbe23 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -554,14 +554,10 @@ export function main(argv: string[], out: Output): number | Promise { "Print a pull request's head, checks, merge state, reviews, reactions, and unresolved review threads " + "in one call. It reports facts; it does not decide whether the PR is ready or whose review counts.\n\n" + "Plain output prints one record per line, its type first and any free text (a check name or path) last: " + - "url, state, draft, head, mergeable, merge-state, review-decision, review-record, online-review, auto-merge, merge-queue, " + + "url, state, draft, head, mergeable, merge-state, review-decision, review-record, auto-merge, merge-queue, " + "landing-stopped, and checks, whose state and counts follow the checks that count, pending while an open PR's base requires a check or workflow nothing reports; review-record gives whether the PR body's ## Review record " + "covers the head, as current, diff-identical (git diff ... prints the same as for the head it names), " + - "stale, or unknown when GitHub cannot compare them, then the head it names last; online-review, on an open PR whose " + - "base's REVIEW.md names an online reviewer in its front matter (online-reviewer: ), gives where that reviewer " + - "stands on the head as quest merge reads it: current, diff-identical, running (its app's check on the head has not " + - "finished), or missing, then the reviewer, the head it reviewed, and open= with the URL of each review thread it " + - "opened that neither it nor a writer resolved; auto-merge gives its method, who enabled it and when, and the head it was " + + "stale, or unknown when GitHub cannot compare them, then the head it names last; auto-merge gives its method, who enabled it and when, and the head it was " + "enabled on, which differs from head once a push follows, as GitHub keeps auto-merge across a writer's " + "push (- when it cannot be told: a quest land pin names it, and otherwise the head branch's push " + "activity, which GitHub lists up to minutes late); landing-stopped is the latest exit from the merge queue, auto-merge, or " + @@ -645,11 +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. When the base's REVIEW.md names an online " + - "reviewer in its front matter (online-reviewer: ), it refuses a head that reviewer has not reviewed, " + - "in a review of it or a successful check run of its own app on it, unless it reviewed a head whose diff from " + - "the base matches, and a head with a review thread it opened on any head that neither it nor an account that " + - "can push resolved; a base that names none lands on its local review alone. 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. " + diff --git a/src/cli/online-review.ts b/src/cli/online-review.ts index 445ef22f..3f64eb12 100644 --- a/src/cli/online-review.ts +++ b/src/cli/online-review.ts @@ -2,18 +2,7 @@ // thread, and any review whose // body opens with `Automated review by `, as a reviewer posting under a person's account does. A finding is a // review thread one opens, or a `[P]` line in its review body; whoever answers it says whether it was taken. -// The repository's own online reviewer, which REVIEW.md names, also gates `quest merge`: see {@link onlineGate}. -import { - type GitHub, - GitHubDeadline, - type ObjectValue, - type PullRequest, - type Repository, - array, - link, - object, - string, -} from "./github"; +import { type GitHub, type ObjectValue, type Repository, array, object } from "./github"; /** One online reviewer's record on one PR. */ export interface OnlineReview { @@ -60,9 +49,9 @@ export function bodyFindings(body: string): string[] { }); } -const THREAD_COMMENT = "author { __typename login } body url pullRequestReview { id }"; +const THREAD_COMMENT = "author { __typename login } body pullRequestReview { id }"; const FIELDS = { - reviews: "id url state author { __typename login } body submittedAt commit { oid }", + reviews: "id url author { __typename login } body submittedAt commit { oid }", reviewThreads: "id isResolved resolvedBy { __typename login } " + `comments(first: 100) { pageInfo { hasNextPage endCursor } nodes { ${THREAD_COMMENT} } }`, @@ -286,237 +275,3 @@ export async function readOnlineReviews( const pulls = await read(client, repository, numbers); return new Map([...pulls].map(([number, pull]) => [number, onlineReviews(pull)])); } - -/** The key of REVIEW.md's front matter that names the repository's online reviewer. */ -const DECLARATION = "online-reviewer"; - -/** - * The online reviewer REVIEW.md's front matter names, such as `pullfrog[bot]`, or null when it names none. - * A key given twice or left empty throws, so a typo cannot silently drop the gate. - */ -export function reviewerIn(text: string): string | null { - const matter = /^---\r?\n([\s\S]*?)\r?\n---[ \t]*(?:\r?\n|$)/.exec(text); - if (!matter) return null; - const lines = matter[1]!.split(/\r?\n/).filter((line) => line.startsWith(`${DECLARATION}:`)); - if (lines.length === 0) return null; - if (lines.length > 1) throw new Error(`REVIEW.md names its ${DECLARATION} more than once`); - const value = lines[0]! - .slice(DECLARATION.length + 1) - .trim() - .replace(/^(["'])(.*)\1$/, "$2") - .trim(); - if (value === "") throw new Error(`REVIEW.md's ${DECLARATION} is empty`); - return value; -} - -/** The online reviewer REVIEW.md names at `ref`, read from GitHub so a PR cannot drop its own gate; null for none. */ -export async function declaredReviewer(client: GitHub, repository: Repository, ref: string): Promise { - const response = await client.request( - `/repos/${repository.owner}/${repository.repo}/contents/REVIEW.md?ref=${encodeURIComponent(ref)}`, - { accept: "application/vnd.github.raw", allow: [404] }, - ); - return response.status === 404 ? null : reviewerIn(await response.text()); -} - -/** - * Whether two commits have the same diff from `base` (`git diff ...`), or null when GitHub cannot - * compare one, such as a diff too large to render; `warn` says why. - */ -export async function sameDiff( - client: GitHub, - repository: Repository, - base: string, - shas: [string, string], - warn: (message: string) => void, -): Promise { - const diffs: string[] = []; - for (const sha of shas) { - let response: Response; - try { - response = await client.request( - `/repos/${repository.owner}/${repository.repo}/compare/${encodeURIComponent(base)}...${sha}`, - { accept: "application/vnd.github.diff", allow: [404, 406, 422] }, - ); - } catch (error) { - // A deadline is the caller's to handle; a forbidden or failing compare is not. - if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; - warn(`cannot compare ${sha} with ${base}: ${error.message}`); - return null; - } - if (!response.ok) { - warn(`cannot compare ${sha} with ${base}: GitHub answered ${response.status}`); - return null; - } - diffs.push(await response.text()); - } - return diffs[0] === diffs[1]; -} - -/** Where the repository's online reviewer stands on a PR's head, as {@link onlineGate} reads it. */ -export interface OnlineGate { - reviewer: string; - /** The PR's head when it was read. */ - head: string; - /** - * `current`: it reviewed the head, in a review of it or a successful check run of its own app on it, which a - * re-review that finds nothing new leaves without a review; `diff-identical`: its latest review is of an earlier head - * whose diff from the base matches the head's; `running`: its app's check on the head has not finished; - * `missing`: none of these. - */ - state: "current" | "diff-identical" | "running" | "missing"; - /** The head it reviewed, or null. */ - reviewed: string | null; - /** Each review thread it opened on the PR, on any head, that neither it nor a writer resolved, by URL. */ - open: string[]; -} - -/** Permissions that can push, the only ones besides the reviewer's own whose resolution settles a finding. */ -const PUSH = ["admin", "maintain", "write"]; - -/** - * How `reviewer`, the repository's online reviewer, stands on `pr`'s head against `base`: whether it reviewed the head - * or a diff-identical one, and which of its findings, its review threads, are still open. GitHub lets the PR's author - * resolve a thread too, so a resolution counts only from the reviewer or an account that can push. - */ -export async function onlineGate( - client: GitHub, - pr: PullRequest, - reviewer: string, - base: string, - warn: (message: string) => void, -): Promise { - const pull = (await read(client, pr, [pr.number])).get(pr.number)!; - const head = pull.head; - const pushers = new Map(); - const canPush = async (account: string) => { - if (!pushers.has(account)) { - const response = await client.request( - `/repos/${pr.owner}/${pr.repo}/collaborators/${encodeURIComponent(account)}/permission`, - { allow: [404] }, - ); - pushers.set( - account, - response.status !== 404 && PUSH.includes(string(object(await response.json()).permission)), - ); - } - return pushers.get(account)!; - }; - // A bot's reviews are its own account's; a named reviewer's body names it under a writer's account, which only a - // writer's own review may claim, so no one else can forge its review. - // Its own reviews, dismissed ones included, whose threads stay its findings; a pending one is no one else's to see. - const ids = new Set(); - for (const review of pull.reviews) { - if (reviewerOf(review) !== reviewer || review.state === "PENDING") continue; - const author = login(review.author); - if (reviewer.endsWith("[bot]") ? author === reviewer : author !== null && (await canPush(author))) - ids.add(String(review.id)); - } - const opened = new Set( - pull.reviewThreads.flatMap((thread) => { - const review = array(object(thread.comments).nodes)[0]?.pullRequestReview; - return review ? [String(object(review).id)] : []; - }), - ); - // A pass over a head gives a verdict, has a body, or opens a thread; a reply in an older thread also makes an empty - // comment review of the head without reviewing it, and a dismissed review is withdrawn. - const reviews = pull.reviews - .filter( - (review) => - ids.has(String(review.id)) && - review.state !== "DISMISSED" && - (review.state === "APPROVED" || - review.state === "CHANGES_REQUESTED" || - String(review.body ?? "").trim() !== "" || - opened.has(String(review.id))), - ) - .map((review) => ({ - commit: review.commit ? string(object(review.commit).oid) : null, - at: Date.parse(String(review.submittedAt)), - })) - .sort((a, b) => a.at - b.at); - - const open: string[] = []; - for (const thread of pull.reviewThreads) { - const first = array(object(thread.comments).nodes)[0]; - if (!first?.pullRequestReview || !ids.has(String(object(first.pullRequestReview).id))) continue; - const resolver = thread.isResolved === true ? login(thread.resolvedBy) : null; - if (resolver === null || (resolver !== reviewer && !(await canPush(resolver)))) - open.push(String(first.url ?? "")); - } - - const gate = (state: OnlineGate["state"], reviewed: string | null): OnlineGate => ({ - reviewer, - head, - state, - reviewed, - open, - }); - if (reviews.some((review) => review.commit === head)) return gate("current", head); - // A GitHub App's bot is `[bot]`; GitHub lists only each check's latest run. GitHub Actions' app runs every - // workflow job, so its checks say nothing of a review. - const slug = /^(.+)\[bot\]$/.exec(reviewer)?.[1]; - let running = false; - if (slug !== undefined && slug !== "github-actions") { - for ( - let path: string | undefined = `/repos/${pr.owner}/${pr.repo}/commits/${head}/check-runs?per_page=100`; - path; - ) { - const response = await client.request(path); - for (const run of array(object(await response.json()).check_runs)) { - if (run.app == null || object(run.app).slug !== slug) continue; - if (run.status !== "completed") running = true; - else if (run.conclusion === "success") return gate("current", head); - } - path = link(response, "next"); - } - } - const earlier = await earlierReviewed(client, pr, head, slug === "github-actions" ? undefined : slug, reviews); - if (earlier !== null && (await sameDiff(client, pr, base, [head, earlier], warn))) - return gate("diff-identical", earlier); - return gate(running ? "running" : "missing", null); -} - -/** - * The newest earlier head of `pr` the reviewer reviewed: among the PR's last 100 commits, the newest before the head - * with one of its `reviews` or a successful check suite of its app `slug`, else its latest review's commit. - */ -async function earlierReviewed( - client: GitHub, - pr: PullRequest, - head: string, - slug: string | undefined, - reviews: { commit: string | null }[], -): Promise { - const last = reviews.filter((review) => review.commit !== null).at(-1)?.commit ?? null; - if (slug === undefined) return last; - const data = await client.graphql( - "query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { " + - "pullRequest(number: $number) { commits(last: 100) { nodes { commit { oid " + - "checkSuites(first: 50) { nodes { conclusion app { slug } } } } } } } } }", - { owner: pr.owner, name: pr.repo, number: pr.number }, - ); - const commits = array(object(object(object(object(data.repository).pullRequest).commits)).nodes).map((node) => - object(node.commit), - ); - const reviewed = new Set(reviews.flatMap((review) => (review.commit === null ? [] : [review.commit]))); - for (const commit of commits.reverse()) { - const oid = string(commit.oid); - if (oid === head) continue; - const checked = array(object(commit.checkSuites ?? { nodes: [] }).nodes).some( - (suite) => suite.conclusion === "SUCCESS" && suite.app != null && object(suite.app).slug === slug, - ); - if (checked || reviewed.has(oid)) return oid; - } - return last; -} - -/** Why `quest merge` refuses `gate`'s head, or null when the online reviewer's review lets it land. */ -export function onlineRefusal(gate: OnlineGate): string | null { - const who = `${gate.reviewer}, the online reviewer REVIEW.md names,`; - if (gate.state === "running") return `${who} is still reviewing it; land it once its check finishes`; - if (gate.state === "missing") - return `${who} has not reviewed it or a head with the same diff; land it once it has, asking it to review if it does not start`; - if (gate.open.length > 0) - return `${who} has findings no writer resolved: ${gate.open.join(", ")}; fix or refute each in its thread and resolve it, then land it again`; - return null; -} diff --git a/src/cli/status.ts b/src/cli/status.ts index 98351722..9668769e 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -19,7 +19,6 @@ import { appOf, checksState, contextOutcome } from "../github/checks"; import { EJECTED, LAND_CHECK, pinned } from "./land"; import { reviewSection } from "../core"; import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; -import { type OnlineGate, declaredReviewer, onlineGate, sameDiff } from "./online-review"; /** * One query covers the PR; each connection pages by its own cursor, and a @@ -142,8 +141,6 @@ export interface Status { checkpoints: { reviewer: string; round: number; url: string }[]; /** Null when the `## Review` section names no full SHA. */ reviewRecord: ReviewRecord | null; - /** The online reviewer the base's REVIEW.md names, on an open PR; null when it names none. */ - onlineReview: OnlineGate | null; reactions: Reaction[]; /** Unresolved review threads, oldest first. */ threads: Thread[]; @@ -215,15 +212,6 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie : { checks: [], workflows: [] }; const status = summarize(first!, rollup, current, needs, reviewNodes, threadNodes); status.reviewRecord = await reviewRecord(github, pr, first!, deps.warn); - if (first!.state === "OPEN") { - const base = string(first!.baseRefName); - const reviewer = await declaredReviewer(github, pr, base); - if (reviewer !== null) { - status.onlineReview = await onlineGate(github, pr, reviewer, base, deps.warn); - if (status.onlineReview.head !== head) - throw new Error("the PR head changed while reading its online review; rerun quest status"); - } - } if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!, deps.warn); return { ...status, holds: ownership.holds, starterHold: ownership.starter }; } @@ -246,10 +234,28 @@ async function reviewRecord( if (!reviewed) return null; const head = string(pull.headRefOid); if (reviewed === head) return { state: "current", head: reviewed }; - const same = await sameDiff(github, pr, string(pull.baseRefName), [head, reviewed], (why) => - warn(`${why}, so whether the review of ${reviewed} carries is unknown`), - ); - return { state: same === null ? "unknown" : same ? "diff-identical" : "stale", head: reviewed }; + const base = encodeURIComponent(string(pull.baseRefName)); + const diffs: string[] = []; + for (const sha of [head, reviewed]) { + const unknown = (why: string): ReviewRecord => { + warn(`cannot compare ${sha} with the base, so whether the review of ${reviewed} carries is unknown${why}`); + return { state: "unknown", head: reviewed }; + }; + let response: Response; + try { + response = await github.request(`/repos/${pr.owner}/${pr.repo}/compare/${base}...${sha}`, { + accept: "application/vnd.github.diff", + allow: [404, 406, 422], + }); + } catch (error) { + // A deadline is the caller's to handle; a forbidden or failing compare is not. + if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; + return unknown(`: ${error.message}`); + } + if (!response.ok) return unknown(""); + diffs.push(await response.text()); + } + return { state: diffs[0] === diffs[1] ? "diff-identical" : "stale", head: reviewed }; } /** Repository activity types that move a branch to a new head, including merges into it. */ @@ -492,7 +498,6 @@ function summarize( reviews: [...latest.values()], checkpoints: checkpoints(head, submitted), reviewRecord: null, - onlineReview: null, reactions, threads, holds: [], @@ -579,16 +584,6 @@ export function statusLines(status: Status): string[] { status.reviewRecord === null ? "review-record -" : `review-record ${status.reviewRecord.state} ${status.reviewRecord.head}`, - status.onlineReview === null - ? "online-review -" - : [ - "online-review", - status.onlineReview.state, - status.onlineReview.reviewer, - dash(status.onlineReview.reviewed), - `open=${status.onlineReview.open.length}`, - ...status.onlineReview.open, - ].join(" "), status.autoMerge === null ? "auto-merge -" : `auto-merge ${status.autoMerge.method} ${status.autoMerge.enabledBy} ${status.autoMerge.enabledAt} ${dash(status.autoMerge.head)}`, diff --git a/test/hold.test.ts b/test/hold.test.ts index dc2eeee2..8f768951 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. */ @@ -63,20 +65,6 @@ class FakePull { /** The error GitHub answers `enqueuePullRequest` with, if any. */ beforeEnqueue?: () => { message: string; type?: string } | undefined; reads = 0; - /** The base's REVIEW.md, absent when null. */ - reviewMd: string | null = null; - /** The PR's reviews and review threads, as the online reviewer's activity read lists them. */ - reviews: ObjectValue[] = []; - threads: ObjectValue[] = []; - /** The head's check runs, as `GET /commits/{sha}/check-runs` lists them. */ - checkRuns: ObjectValue[] = []; - /** Each account's repository permission; absent ones are no collaborators. */ - permissions: Record = {}; - /** Each commit's diff from the base. */ - diffs: Record = {}; - /** The PR's commits before its head, oldest first, and the apps whose check suites succeeded on each. */ - history: string[] = []; - suites: Record = {}; private clock = Date.parse("2026-10-21T12:00:00Z"); /** Add a label as GitHub does: re-adding one already on the PR records no event. */ @@ -132,18 +120,6 @@ class FakePull { if (url.pathname === "/repos/example/project/branches/main") return json(200, { protection: {} }); if (url.pathname === "/repos/example/project/actions/runs" && url.searchParams.get("head_sha") === this.head) return json(200, { total_count: this.runs.length, workflow_runs: this.runs }); - if (url.pathname === "/repos/example/project/contents/REVIEW.md" && url.searchParams.get("ref") === this.base) - return this.reviewMd === null ? json(404, { message: "Not Found" }) : new Response(this.reviewMd); - if (url.pathname === `/repos/example/project/commits/${this.head}/check-runs`) - return json(200, { total_count: this.checkRuns.length, check_runs: this.checkRuns }); - const permission = /^\/repos\/example\/project\/collaborators\/(.+)\/permission$/.exec(url.pathname); - if (permission) { - const level = this.permissions[decodeURIComponent(permission[1])]; - return level ? json(200, { permission: level }) : json(404, { message: "Not Found" }); - } - const diff = /^\/repos\/example\/project\/compare\/main\.\.\.(\w+)$/.exec(url.pathname); - if (diff && new Headers(init.headers).get("accept") === "application/vnd.github.diff") - return new Response(this.diffs[diff[1]] ?? ""); if (url.pathname === `/repos/example/project/compare/main...${this.head}`) { this.comparisons.push(`${url.pathname}${url.search}`); return json(200, { status: this.comparison, base_commit: { sha: this.base } }); @@ -190,36 +166,6 @@ class FakePull { return { data: { [name]: { clientMutationId: null } } }; } if (query.includes("node(id: $id)")) return { data: { node: { autoMergeRequest: this.autoMerge } } }; - if (query.includes("commits(last: 100)")) { - const commit = (oid: string) => ({ - commit: { - oid, - checkSuites: { - nodes: (this.suites[oid] ?? []).map((slug) => ({ conclusion: "SUCCESS", app: { slug } })), - }, - }, - }); - return { - data: { - repository: { pullRequest: { commits: { nodes: [...this.history, this.head].map(commit) } } }, - }, - }; - } - if (query.includes("p7: pullRequest")) { - const page = (nodes: ObjectValue[]) => ({ pageInfo: { hasNextPage: false, endCursor: null }, nodes }); - return { - data: { - repository: { - p7: { - headRefOid: this.head, - reviews: page(this.reviews), - reviewThreads: page(this.threads), - comments: page([]), - }, - }, - }, - }; - } if (query.includes("statusCheckRollup")) return { data: { @@ -254,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, @@ -912,167 +859,45 @@ test("iterate and release exit 0 when stdout returns a value", async () => { } }); -describe("merge --land with an online reviewer", () => { - const OLD = "c".repeat(40); - const bot = { __typename: "Bot", login: "pullfrog" }; - const review = (id: string, commit: string, author: ObjectValue = bot, body = "Reviewed.") => ({ - id, - url: `${URL_}#pullrequestreview-${id}`, - state: "COMMENTED", - author, - body, - submittedAt: `2026-10-21T11:0${id}:00Z`, - commit: { oid: commit }, - }); - const thread = (id: string, resolvedBy: ObjectValue | null = null) => ({ - id: `T_${id}`, - isResolved: resolvedBy !== null, - resolvedBy, - comments: { - pageInfo: { hasNextPage: false, endCursor: null }, - nodes: [{ author: bot, body: "A bug.", url: `${URL_}#discussion_r${id}`, pullRequestReview: { id } }], +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}`]); }, - }); - const run = (status: string, conclusion: string | null, slug = "pullfrog") => ({ - name: slug, - status, - conclusion, - app: { slug }, - }); - const declared = () => { - const pull = new FakePull(); - pull.reviewMd = "---\nonline-reviewer: pullfrog[bot]\n---\n\nBe adversarial.\n"; - return pull; - }; - const merge = (pull: FakePull) => land(PR, { sha: HEAD, method: "merge", check: clean }, pull.deps()); + ); - test("lands on the local review alone when the base's REVIEW.md names no online reviewer", async () => { + test("arms auto-merge rather than merging at once", async () => { const pull = new FakePull(); - pull.reviewMd = "Be adversarial.\n"; - await merge(pull); - expect(pull.writes).not.toEqual([]); - }); - - test("refuses a head it has not reviewed, and one it is still reviewing", async () => { - const pull = declared(); - expect(await refused(merge(pull))).toBe( - `${URL_} head ${HEAD}: pullfrog[bot], the online reviewer REVIEW.md names, has not reviewed it or a head with the same diff; land it once it has, asking it to review if it does not start`, - ); - // Another app's check says nothing of this reviewer. - pull.checkRuns = [run("in_progress", null), run("completed", "success", "coderabbitai")]; - expect(await refused(merge(pull))).toContain("is still reviewing it"); - pull.checkRuns = [run("completed", "cancelled")]; - expect(await refused(merge(pull))).toContain("has not reviewed it"); - expect(pull.writes).toEqual([]); - }); - - test("counts a review of the head, or its own app's successful check run on it", async () => { - const checked = declared(); - checked.checkRuns = [run("completed", "success")]; - await merge(checked); - expect(checked.writes).not.toEqual([]); - const reviewed = declared(); - reviewed.reviews = [review("1", HEAD)]; - await merge(reviewed); - expect(reviewed.writes).not.toEqual([]); - }); - - test("counts no review another account posts in its name", async () => { - const pull = declared(); - pull.permissions = { outsider: "read" }; - pull.reviews = [ - review("1", HEAD, { __typename: "User", login: "outsider" }, "Automated review by pullfrog[bot]\n\nFine."), - ]; - expect(await refused(merge(pull))).toContain("has not reviewed it"); - }); - - test("counts an earlier head's review only while the head's diff from the base matches it", async () => { - const same = declared(); - same.reviews = [review("1", OLD)]; - same.diffs = { [HEAD]: "diff", [OLD]: "diff" }; - await merge(same); - expect(same.writes).not.toEqual([]); - const changed = declared(); - changed.reviews = [review("1", OLD)]; - changed.diffs = { [HEAD]: "new", [OLD]: "old" }; - expect(await refused(merge(changed))).toContain("has not reviewed it"); - }); - - test("counts an approval without a body as a review of the head", async () => { - const pull = declared(); - pull.reviews = [{ ...review("1", HEAD, bot, ""), state: "APPROVED" }]; - await merge(pull); - expect(pull.writes).not.toEqual([]); - }); - - test("compares with the newest earlier head its app's check passed on, not only its reviews", async () => { - const MID = "d".repeat(40); - const pull = declared(); - pull.reviews = [review("1", OLD)]; - pull.history = [OLD, MID]; - pull.suites = { [MID]: ["pullfrog"] }; - pull.diffs = { [OLD]: "old", [MID]: "fixed", [HEAD]: "fixed" }; - await merge(pull); - expect(pull.writes).not.toEqual([]); - // Another app's passing suite reviewed nothing. - const other = declared(); - other.reviews = [review("1", OLD)]; - other.history = [OLD, MID]; - other.suites = { [MID]: ["coderabbitai"] }; - other.diffs = { [OLD]: "old", [MID]: "fixed", [HEAD]: "fixed" }; - expect(await refused(merge(other))).toContain("has not reviewed it"); + 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 while a thread it opened on any head is unresolved, or resolved by an account that cannot push", async () => { - const pull = declared(); - pull.reviews = [review("1", OLD), review("2", HEAD)]; - pull.threads = [thread("1")]; - expect(await refused(merge(pull))).toContain( - `has findings no writer resolved: ${URL_}#discussion_r1; fix or refute each in its thread and resolve it`, + 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`, ); - // GitHub lets the PR's author resolve a thread without write access. - pull.threads = [thread("1", { __typename: "User", login: "author" })]; - pull.permissions = { author: "read" }; - expect(await refused(merge(pull))).toContain(`${URL_}#discussion_r1`); expect(pull.writes).toEqual([]); - pull.permissions = { author: "write" }; - await merge(pull); - expect(pull.writes).not.toEqual([]); - }); - - test("counts no reply in an older thread as a review of the head", async () => { - const pull = declared(); - // Replying makes an empty review of the head that opens no thread. - pull.reviews = [review("1", OLD), review("2", HEAD, bot, "")]; - pull.diffs = { [HEAD]: "new", [OLD]: "old" }; - pull.threads = [thread("1", { __typename: "User", login: "maintainer" })]; - pull.permissions = { maintainer: "admin" }; - expect(await refused(merge(pull))).toContain("has not reviewed it"); - }); - - test("keeps a dismissed review's unresolved threads open, though the review no longer counts", async () => { - const pull = declared(); - pull.reviews = [{ ...review("1", HEAD), state: "DISMISSED" }]; - pull.threads = [thread("1")]; - pull.checkRuns = [run("completed", "success")]; - expect(await refused(merge(pull))).toContain(`${URL_}#discussion_r1`); - pull.threads = []; - pull.checkRuns = []; - expect(await refused(merge(pull))).toContain("has not reviewed it"); - }); - - test("counts no GitHub Actions check for a reviewer posting as github-actions[bot], since every job is one", async () => { - const pull = new FakePull(); - pull.reviewMd = "---\nonline-reviewer: github-actions[bot]\n---\n"; - pull.checkRuns = [run("completed", "success", "github-actions")]; - expect(await refused(merge(pull))).toContain("has not reviewed it"); - }); - - test("a thread it resolved itself is settled", async () => { - const pull = declared(); - pull.reviews = [review("1", HEAD)]; - pull.threads = [thread("1", bot)]; - await merge(pull); - expect(pull.writes).not.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/online-review.test.ts b/test/online-review.test.ts index 504ceb52..25faf4e7 100644 --- a/test/online-review.test.ts +++ b/test/online-review.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "vitest"; -import { type Activity, bodyFindings, classify, onlineReviews, reviewerIn } from "../src/cli/online-review"; +import { type Activity, bodyFindings, classify, onlineReviews } from "../src/cli/online-review"; const HEAD = "h".repeat(40); const bot = (login: string) => ({ __typename: "Bot", login }); @@ -156,24 +156,3 @@ describe("onlineReviews", () => { ]); }); }); - -describe("reviewerIn", () => { - test("reads the online reviewer from REVIEW.md's front matter", () => { - expect(reviewerIn("---\nonline-reviewer: pullfrog[bot]\n---\n\nBe adversarial.")).toBe("pullfrog[bot]"); - expect(reviewerIn("---\r\ntitle: Review\r\nonline-reviewer: 'review (OpenAI)'\r\n---\r\n")).toBe( - "review (OpenAI)", - ); - }); - - test("names none without front matter or the key, even when the body mentions it", () => { - expect(reviewerIn("# Review\n\nonline-reviewer: pullfrog[bot]\n")).toBeNull(); - expect(reviewerIn("---\ntitle: Review\n---\nonline-reviewer: pullfrog[bot]\n")).toBeNull(); - }); - - test("refuses an empty or repeated name, so a typo cannot drop the gate", () => { - expect(() => reviewerIn("---\nonline-reviewer:\n---\n")).toThrow("is empty"); - expect(() => reviewerIn("---\nonline-reviewer: a[bot]\nonline-reviewer: b[bot]\n---\n")).toThrow( - "more than once", - ); - }); -}); diff --git a/test/status.test.ts b/test/status.test.ts index b41f7897..c38be319 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -124,10 +124,6 @@ function setup( noBase?: boolean; /** The head's workflow runs, as `GET /actions/runs` lists them. */ runs?: Node[]; - /** The base's REVIEW.md, absent when unset. */ - reviewMd?: string; - /** The PR's reviews, as the online reviewer's activity read lists them, and the head's check runs. */ - online?: { reviews: Node[]; checkRuns: Node[] }; } = {}, ) { const queries: Record[] = []; @@ -175,12 +171,6 @@ function setup( ? Response.json({ message: "No diff" }, { status: diff }) : new Response(diff); } - if (path.pathname === "/repos/example/project/contents/REVIEW.md") - return options.reviewMd === undefined - ? Response.json({ message: "Not Found" }, { status: 404 }) - : new Response(options.reviewMd); - if (path.pathname === `/repos/example/project/commits/${A}/check-runs`) - return Response.json({ check_runs: options.online?.checkRuns ?? [] }); if (path.pathname !== "/graphql") { expect(path.pathname).toBe("/repos/example/project/issues/1/events"); eventRequests.push(String(url)); @@ -189,29 +179,6 @@ function setup( return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); } const request = JSON.parse(String(init?.body)); - if (request.query.includes("commits(last: 100)")) - return Response.json({ - data: { - repository: { - pullRequest: { commits: { nodes: [{ commit: { oid: A, checkSuites: { nodes: [] } } }] } }, - }, - }, - }); - if (request.query.includes("p1: pullRequest")) { - const nodes = (rows: Node[]) => ({ pageInfo: { hasNextPage: false, endCursor: null }, nodes: rows }); - return Response.json({ - data: { - repository: { - p1: { - headRefOid: A, - reviews: nodes(options.online?.reviews ?? []), - reviewThreads: nodes([]), - comments: nodes([]), - }, - }, - }, - }); - } if (request.query.includes("query HoldOwnership(")) { holdQueries.push(request.variables); if (!last) throw new Error("ownership read before PR state"); @@ -293,7 +260,6 @@ test("reports checks by requiredness, each reviewer's latest review against the "merge-state blocked", "review-decision review_required", "review-record -", - "online-review -", "auto-merge -", "merge-queue -", "landing-stopped -", @@ -330,14 +296,13 @@ test("a queued PR with auto-merge and no checks reports both, and JSON is determ threads: page([]), }); const status = await pullRequestStatus(pr, setup([body]).deps); - expect(statusLines(status).slice(2, 13)).toEqual([ + expect(statusLines(status).slice(2, 12)).toEqual([ "draft true", `head ${A}`, "mergeable mergeable", "merge-state blocked", "review-decision -", "review-record -", - "online-review -", "auto-merge squash kixelated 2026-10-07T12:00:00Z -", "merge-queue awaiting_checks 2 2026-10-07T12:01:00Z", "landing-stopped -", @@ -961,30 +926,6 @@ test("refuses a head moved between the paged state and ownership read", async () await expect(pullRequestStatus(pr, fixture.deps)).rejects.toThrow("head changed while reading its holds"); }); -describe("the online review", () => { - const line = async (options: Parameters[1] = {}) => { - const fixture = setup([pull({ checks: null, reviews: page([]), threads: page([]) })], options); - return statusLines(await pullRequestStatus(pr, fixture.deps)).find((l) => l.startsWith("online-review ")); - }; - const reviewMd = "---\nonline-reviewer: pullfrog[bot]\n---\n"; - - test("is absent when the base's REVIEW.md names no online reviewer", async () => { - expect(await line()).toBe("online-review -"); - expect(await line({ reviewMd: "Be adversarial.\n" })).toBe("online-review -"); - }); - - test("reports where the named reviewer stands on the head", async () => { - expect(await line({ reviewMd })).toBe("online-review missing pullfrog[bot] - open=0"); - const run = (status: string, conclusion: string | null) => ({ status, conclusion, app: { slug: "pullfrog" } }); - expect(await line({ reviewMd, online: { reviews: [], checkRuns: [run("in_progress", null)] } })).toBe( - "online-review running pullfrog[bot] - open=0", - ); - expect(await line({ reviewMd, online: { reviews: [], checkRuns: [run("completed", "success")] } })).toBe( - `online-review current pullfrog[bot] ${A} open=0`, - ); - }); -}); - describe("the review record", () => { const C = "c".repeat(40); const line = async (body: string, options: Parameters[1] = {}) => { From da00ef6a6322619806a4905250bc6753b571f926 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 11:52:42 -0800 Subject: [PATCH 5/8] fix(merge): no enqueue past a missing approval, on any path Co-Authored-By: Claude Opus 5.5 --- README.md | 3 ++- src/cli/hold.ts | 37 +++++++++++++++---------------- src/cli/land-watch-fix.ts | 14 ++++++------ src/cli/land-watch.ts | 7 ++++-- src/github/merge.ts | 46 +++++++++++++++++++++++++-------------- test/hold.test.ts | 24 ++++++++++++++++++++ test/land-watch.test.ts | 6 +++++ 7 files changed, 91 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 3aa73c06..1028a651 100644 --- a/README.md +++ b/README.md @@ -938,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 diff --git a/src/cli/hold.ts b/src/cli/hold.ts index d75e7592..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, @@ -256,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({ @@ -284,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()); @@ -302,10 +303,6 @@ export async function land(pr: PullRequest, request: LandRequest, deps: GitHubDe `${snapshot.url} repository has auto-merge disabled; follow its landing instructions in CONTRIBUTING.md or AGENTS.md (for example, its label queue). Only use --direct if the user explicitly asks`, ); } - if (snapshot.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`, - ); // The batch gate and a merge queue test the current base themselves. Here nothing does, so the head must contain it. const comparison = object( await ( @@ -320,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!) { @@ -329,10 +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 ( - !snapshot.unapproved && - (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; } @@ -386,14 +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, - // A missing approval is pending like a check, so it arms auto-merge, which waits for it, and never enqueues. - snapshot.unapproved ? "BLOCKED" : 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; @@ -560,7 +557,7 @@ class Labels { branch: string(row.headRefName), head: string(row.headRefOid), mergeState: string(row.mergeStateStatus), - unapproved: row.reviewDecision === "REVIEW_REQUIRED" || row.reviewDecision === "CHANGES_REQUESTED", + 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/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/test/hold.test.ts b/test/hold.test.ts index 8f768951..03d2998f 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -878,6 +878,30 @@ describe("merge --land on a base that requires an approval the head lacks", () = }, ); + 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"; 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]; From e8ebe18596815f1dcccad18ba3193def55ce0e8d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 12:17:27 -0800 Subject: [PATCH 6/8] fix(review): drop a retired reviewer's saved session and tally A saved opencode tally recorded 'Reviewer: undefined', and its open findings had no CLI left to settle them. Reading saved state now drops any reviewer quest no longer runs; the routed reviewer reviews the whole branch. Co-Authored-By: Claude Opus 5.5 --- src/cli/review.ts | 7 +++++-- test/review.test.ts | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/cli/review.ts b/src/cli/review.ts index cfbefb79..09ac4f9e 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -16,7 +16,7 @@ import { dirname, join, posix } from "node:path"; import { promisify } from "node:util"; import { SIZES, type Size, branchQuest, parse, size as questSize } from "../core"; -import { type HarnessName, LINE_LIMIT, LOGINS, boundedLines, strippedEnvironment } from "./agent"; +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 } from "./models"; @@ -1278,7 +1278,10 @@ 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 reviewer this quest no longer runs has no CLI to resume its session or settle its findings, so its tally and + // session are dropped, open findings included: the routed reviewer reviews the whole branch. + const runs = (seat: { reviewer: string }) => (HARNESSES as readonly string[]).includes(seat.reviewer); + return { ...state, sessions: state.sessions!.filter(runs), tallies: state.tallies!.filter(runs) } as State; } /** diff --git a/test/review.test.ts b/test/review.test.ts index 30e7cc83..501ce30a 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -1846,6 +1846,15 @@ 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 session and tally, open findings included, are dropped rather than recorded or held. + const retired = { reviewer: "opencode", model: "opencode-go/deepseek-v4.1-flash", effort: "high" }; + expect( + parseState({ + branch: "feature", + sessions: [session, { ...session, ...retired, session: "ses_1" }], + tallies: [tally, { ...tally, ...retired }], + }), + ).toEqual(current); for (const older of [ null, // One reviewer's session alone, outside a sessions list. From a13dae66a2b960573b7e95e848da19d59c9a8bcc Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 12:18:29 -0800 Subject: [PATCH 7/8] docs(quest): the Pullfrog approval ruleset now covers every repository Co-Authored-By: Claude Opus 5.5 --- quest/m0/pullfrog-setup.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/quest/m0/pullfrog-setup.md b/quest/m0/pullfrog-setup.md index 3bc8f2da..ca8ab580 100644 --- a/quest/m0/pullfrog-setup.md +++ b/quest/m0/pullfrog-setup.md @@ -17,9 +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 a `Pullfrog approval` ruleset requires -on moq-dev/quest and moq-dev/moq, gates merges in place of the DeepSeek -review, so what its runs can check now decides merges. +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; From cb75192f7c5e6228130a10cf9c8730e8265493c6 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 13 Nov 2026 12:23:07 -0800 Subject: [PATCH 8/8] fix(review): keep finding numbers when a retired reviewer's session is dropped Dropping an opencode session on read renumbered the findings after it, so the author's responses reached the wrong reviewer. The run now numbers every saved session as the last round printed them, then drops the retired ones. Co-Authored-By: Claude Opus 5.5 --- src/cli/review.ts | 24 ++++++++++++++++-------- test/review.test.ts | 43 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/src/cli/review.ts b/src/cli/review.ts index 09ac4f9e..0ab88765 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -329,6 +329,7 @@ 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[]; } @@ -1278,12 +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"); - // A reviewer this quest no longer runs has no CLI to resume its session or settle its findings, so its tally and - // session are dropped, open findings included: the routed reviewer reviews the whole branch. - const runs = (seat: { reviewer: string }) => (HARNESSES as readonly string[]).includes(seat.reviewer); - return { ...state, sessions: state.sessions!.filter(runs), tallies: state.tallies!.filter(runs) } 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, @@ -1407,14 +1413,16 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): 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)!, @@ -1544,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]--; } diff --git a/test/review.test.ts b/test/review.test.ts index 501ce30a..edd78467 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -1846,15 +1846,15 @@ 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 session and tally, open findings included, are dropped rather than recorded or held. - const retired = { reviewer: "opencode", model: "opencode-go/deepseek-v4.1-flash", effort: "high" }; + // 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, { ...session, ...retired, session: "ses_1" }], - tallies: [tally, { ...tally, ...retired }], + sessions: [session, retired], + tallies: [tally, { ...tally, reviewer: "opencode" }], }), - ).toEqual(current); + ).toEqual({ ...current, sessions: [session, retired] }); for (const older of [ null, // One reviewer's session alone, outside a sessions list. @@ -1876,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);