code-review-loop

Code Review Loop ================ Three reviewers, one scope contract. The work is already done and roughly correct; this skill is the finishing pass that raises it to committable quality. When OpenCode is installed, a cheap DeepSeek Flash pass catches the obvious defects first. Codex then reviews and you iterate with it, Claude gives the final independent read, then you commit. The cheap pass exists to protect quota, not to replace anyone. ChatGPT and Claude subscriptions run out of weekly quota, and a Codex round spent on an off-by-one that a small model would have caught is a round not spent on the problems only a frontier model finds. The Codex loop and the Claude pass run in full whatever the first pass reports. The failure mode this skill exists to prevent is scope creep. Every LLM reviewer will find *something* — that is what it is built to do. Each individual finding looks reasonable on its own, so a loop that goes straight from “finding” to “fix” ratchets outward until the patch has swallowed refactors, API changes and pre-existing bugs nobody asked about. The defense is a scope contract written *before* the first review and a triage step between every finding and every edit. Step 0: Write the scope contract -------------------------------- Do this before running any reviewer. Once findings start arriving it is too late to define scope honestly — you will rationalize each expansion as obviously necessary. Derive the contract from whatever established the intent: the conversation that produced the work, the branch's commits, a linked issue or PR, the diff itself. Write it to a scratch file so you can re-read it verbatim later in the loop, when the temptation to widen is strongest: ~~~~ bash REPO_ROOT=$(git rev-parse --show-toplevel) WORK=$(mktemp -d -t review-loop.XXXXXX) # prompts, JSON output, scope contract SCOPE_FILE="$WORK/scope.md" ~~~~ Every file this skill generates lives in `$WORK`, never in the repository. A prompt or a `review.json` written to the working tree shows up as an untracked file, which dirties a clean post-commit review and can get swept into the commit by a `git add -A` in pre-commit mode. Remove `$WORK` when the loop finishes. Keep it to four short sections: ~~~~ markdown ## Goal One to three sentences. What this change set is for. ## In scope The files, modules or behaviors this change set is allowed to touch. ## Non-goals Known problems nearby that this change set is deliberately not fixing. Name them explicitly — an unnamed non-goal gets fixed by accident. ## Done when The concrete condition that makes this finishable. ~~~~ State the contract back to the user in three to five lines and keep going. Do not block on approval; the user corrects you if it is wrong. But if you cannot tell what the change set is *for* from any available source, ask before reviewing — a confidently wrong contract is worse than none, because it launders scope creep as compliance. Step 1: Determine mode and review scope --------------------------------------- ~~~~ bash git status --short ~~~~ - **Pre-commit mode** — uncommitted changes exist (staged, unstaged or untracked). All review-driven fixes accumulate into one final commit. - **Post-commit mode** — the working tree is clean. Each fix batch gets its own commit. For post-commit mode, resolve the review range: ~~~~ bash BRANCH=$(git branch --show-current) DEFAULT_BRANCH=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's#^origin/##') # If empty, fall back to main, then master, whichever exists locally or on origin. ~~~~ Resolve the base to a ref that actually exists before using it. A bare `main` is not always present — worktrees and fresh clones often have only `origin/main`, and a branch may have no upstream at all: ~~~~ bash # Feature branch: try the local default branch, then the remote one. BASE=$(git merge-base HEAD "$DEFAULT_BRANCH" 2>/dev/null || \ git merge-base HEAD "origin/$DEFAULT_BRANCH" 2>/dev/null) # Default branch: the unpushed commits, if an upstream is configured. UPSTREAM=$(git rev-parse --abbrev-ref --symbolic-full-name '@{u}' 2>/dev/null) ~~~~ - **Feature branch** — scope is `$BASE..HEAD`. - **Default branch** — scope is `$UPSTREAM..HEAD`. **Distinguish an empty range from a failed lookup.** If `BASE` or `UPSTREAM` came back empty, the range did not resolve; `git log` on it will also produce nothing, which looks identical to “no commits to review”. Reporting “nothing to review” when a branch is in fact full of unreviewed commits is the worst outcome this skill can produce, because it reads as a clean bill of health. Say which ref failed to resolve and ask the user what to review against. If the range resolved and is genuinely empty, say there is nothing to review and stop. Step 2: Resolve the reviewer models ----------------------------------- Codex and Claude run on the current top-tier frontier model of their vendor. Resolve rather than hardcode, so the skill keeps working after the next release. **OpenCode** — the one exception: the first pass is deliberately *not* a frontier model, and its model list is fixed in a deterministic preference order. See the pre-stage below. **Codex** — the OpenAI docs publish the current flagship in page frontmatter: ~~~~ bash CODEX_MODEL=$(curl -sL https://developers.openai.com/api/docs/guides/latest-model.md \ | sed -n 's/^ *model: *//p' | head -1) ~~~~ That yields an ID like `gpt-5.6-sol`. If the fetch fails, fall back to the `model` value in `~/.codex/config.toml`, and say in your report which source you used. `codex review` echoes `model:` in its header — confirm it matches before trusting the output. **Claude** — pass the moving aliases and read the exact ID back out of the response, which is both simpler and more accurate than guessing the version suffix. See Stage B. Pre-stage: the OpenCode first pass ---------------------------------- A small, cheap model reads the change set before Codex does. Its job is to catch the defects that are obvious once someone looks (wrong conditions, off-by-one errors, unhandled errors, typos in identifiers) so that Codex and Claude spend their rounds on the rest. It is advice from a weaker reviewer: every finding goes through triage like any other, and nothing about this stage shortens, skips or replaces Stage A or Stage B. All of the mechanics live in `scripts/opencode-review.sh` next to this file. Use the script rather than retyping its commands: the read-only guard, the repository snapshot and the result checks only hold if every round runs them the same way, and shell state does not survive between separate tool calls. ~~~~ bash OC_REVIEW="<this skill's directory>/scripts/opencode-review.sh" bash "$OC_REVIEW" models ~~~~ ### Availability and model preference `models` prints the usable models, one per line, in this fixed order: 1. `deepseek/deepseek-flash` — DeepSeek's own API, under its canonical model ID. 2. `opencode-go/deepseek-v4.1-flash` — the same model through the OpenCode Go subscription. OpenCode Go's catalog has no `deepseek-flash` entry, so the call has to use this ID even though DeepSeek no longer uses versioned IDs. DeepSeek's first-party API comes first because it is one hop with no proxy in between. OpenCode Go has had bugs of its own with DeepSeek models (dropped tool-call names, a second turn failing with a missing-field 400), and that is exactly the path a multi-round, tool-heavy review exercises. Both passed a resumed two-round review with tool calls on OpenCode 1.18.31, so Go is a real fallback, not a placeholder. Never add a legacy alias such as `deepseek-v4-flash` to the list: DeepSeek routes old IDs to whatever is current, so a pinned legacy ID records a model that did not run. If `opencode` is not installed, or neither model is configured, `models` exits 3 with a one-line reason on stderr. Say “OpenCode pre-stage skipped: <reason>” in one line and go straight to Stage A. That is the *unavailable* outcome, and it is not a failure of anything. ### Running a round Build `$WORK/opencode-prompt.txt` from the “OpenCode — initial review” template in `references/review-prompts.md`, with the same range and scope contract the other stages use. As with them, never paste a diff or file contents into it; the reviewer reads the repository itself. Then run round 1 on the first model `models` printed: ~~~~ bash REPO_ROOT="$REPO_ROOT" WORK="$WORK" bash "$OC_REVIEW" run "$OC_MODEL" 1 ~~~~ The script prints `STATUS=<status> MODEL=<id> SESSION=<id>`, then any `REASON=` and `DENIED=` lines, and saves the final answer to `$WORK/opencode-r1.review.txt` (`r2` for round 2). Its exit code is the status: | Exit | Status | Meaning | What you do | | ---- | ------------- | --------------------------------------------------------- | ---------------------------------------------- | | 0 | `clean` | The final answer is exactly `NO ACTIONABLE FINDINGS` | End the stage | | 10 | `findings` | A complete review with findings | Triage, fix, re-review (below) | | 20 | `failed` | The reviewer ran, but the round is not a usable review | End the stage; report it as failed | | 21 | `blocked` | A complete answer, but a tool call was denied | As `findings`, but never count it as clean | | 30 | `unavailable` | The provider returned nothing (401, unknown model, quota) | Round 1: next model. Later rounds: as `failed` | | 40 | `mutated` | The repository changed while the reviewer ran | Stop and show the user, as for any reviewer | On round 1, `unavailable` moves on to the next model in the list, starting a fresh session. If every model comes back `unavailable`, the stage is skipped as unavailable, and the report lists each model's `REASON=` lines. Once a model has served round 1, it is the stage's model: record `MODEL` and `SESSION` from the status line and use both for the next round. Never switch models mid-stage; a different model resuming the session is a different reviewer. `failed` covers everything that looks like an answer but is not one. The script treats each of these as a failure: - `opencode run` exits non-zero, or `timeout` stops it (`OC_TIMEOUT`, default 900 seconds) - an `error` event in the JSON stream, or an error on an assistant message, such as `APIError 401` - no assistant message in the round - any assistant message whose `providerID/modelID`, variant or agent is not the one requested - a final step that finished with anything but `stop`, such as `length` - an empty final text: DeepSeek models through OpenCode sometimes finish their reasoning and return no answer at all, and an empty reply must never read as “nothing to report” A clean result needs the exact sentinel, so silence can never pass for one. `blocked` exists because a reviewer that was refused a read still writes a confident review of whatever it managed to see — the same quiet failure Stage B guards against with `permission_denials`. Its findings are still worth triaging, but a `blocked` round that says `NO ACTIONABLE FINDINGS` is unknown, not clean. ### Re-review For each round: triage every finding against the contract, verify each against the code, and apply only the in-scope fixes, exactly as in Stage A. In post-commit mode, commit the batch before resuming (Stage C rules), for the same stale-range reason Stage B gives. Then rewrite `$WORK/opencode-prompt.txt` from the “OpenCode — re-review” template and resume the same session: ~~~~ bash REPO_ROOT="$REPO_ROOT" WORK="$WORK" \ bash "$OC_REVIEW" run "$OC_MODEL" 2 "$OC_SESSION" ~~~~ **Cap at two rounds**: one review and one re-review of the fixes. This stage is a filter in front of reviewers that will read the same code again, so a third round buys little, and a small model asked to keep looking drifts into nitpicks. When round 2 still returns in-scope findings, verify and apply them, then go on to Stage A; Codex reviews those fixes as part of the range, so they do not stay unverified the way post-cap fixes elsewhere do. Say in the report that the stage ended at its cap. When the stage ends as `failed`, `mutated`, or `unavailable` on every model, go on to Stage A all the same (after the user has seen a mutation). Never substitute your own reading and report it as OpenCode's verdict. ### How the script keeps the reviewer read-only The guard is enforced by OpenCode's permission system, not by the prompt: - **A dedicated agent**, `review-loop-flash`, passed through `OPENCODE_CONFIG_CONTENT` and pinned with `--agent`. Its permissions start from `"*": "deny"`; OpenCode applies the last matching rule, so everything not allowed afterwards stays denied, including `edit`, `write`, `task`, `webfetch`, `websearch`, `skill`, `lsp` and `question`. OpenCode removes tools denied this way from the model's tool list altogether. - **Reads stay in the repository.** `read`, `glob`, `grep` and `list` are allowed, `.env` files are denied, and `external_directory` is denied except for OpenCode's own `tool-output` directory, where it stores long command output for the reviewer to page through. - **Bash is an allowlist of read-only git commands** (`git status`, `git diff`, `git log`, `git show`, `git blame`, `git grep`, and a few plumbing queries), plus deny rules for `>` redirection, `--output`, `--no-index`, `--contents` and `git grep -O`. This is the opposite of Stage B's deny list, for a reason: OpenCode parses a compound command, pipeline or command substitution into its separate commands and checks each one, including its redirections, so an allowlist does not break on `git diff; git status` the way a Claude Code prefix rule does. `git -C`, `git -c`, `cd`, environment assignments and every non-git command match no allow rule and are refused. - **No outside configuration.** `OPENCODE_DISABLE_PROJECT_CONFIG` ignores the reviewed repository's own *opencode.json* and *.opencode/*, `--pure` loads no plugins, Claude Code prompts and external skills are off, sharing and auto-update are off, and every MCP server in the user's global config is disabled by name, so the reviewer has no GitHub, browser or other tools. - **A preflight check.** Before each round the script asks `opencode debug agent` for the resolved agent and refuses to run unless the catch-all rule is `deny` and the edit, write, task and web tools are off. An unknown or shadowed agent would otherwise run with default permissions. - **The snapshot check** from Stage B runs around every round, and a change ends the round as `mutated` before its output is read. As there, the permission rules are the first line and the snapshot is the guarantee. **Model evidence** comes from `opencode export <session>`: each assistant message records the `providerID`, `modelID`, `variant` and `agent` it ran with. The JSON event stream does not carry the model, which is why the script exports the session after every round. That record is the model OpenCode requested from the provider; an alias resolved on the provider's side is not visible, which is one more reason to keep only canonical IDs in the preference list. The reviewer runs with `--variant high` on both models. OpenCode keeps each review session in its own session list; the script never deletes them, so `opencode export <session>` still works if you need to look at a round again. ### Attribution When a first-pass finding holds up under your verification and you change the code because of it, the commit that carries that change gets an OpenCode trailer: ~~~~ Assisted-by: OpenCode:deepseek-flash ~~~~ Write `deepseek-flash` whichever provider ran the rounds. DeepSeek has announced that it no longer uses versioned model IDs such as `deepseek-v4.1-flash`, and `deepseek-flash` is the canonical name for the model both providers serve. The provider-specific ID is how the script reaches the model through OpenCode Go, not what the trailer records. Check the session export before writing the trailer all the same. It must show one of the two models in the preference list; if it shows anything else, the script has already failed the round and no trailer applies. The trailer records what shaped the commit, the same rule Stage A and Stage B follow. A round that ran but changed nothing gets no trailer: a clean `NO ACTIONABLE FINDINGS`, findings you rejected, or a round that ended `failed`, `unavailable` or `mutated`. A `blocked` round whose findings you verified and applied does get one. Stage C gives the order of trailers when more than one reviewer contributed. Stage A: the Codex loop ----------------------- `codex review` is one-shot; there is no session to resume, so every round is a fresh review and you carry the continuity in the prompt. Note two CLI constraints that shape the commands below: - `--uncommitted`, `--base` and `--commit` are **mutually exclusive with a custom prompt**. Since the scope contract has to reach the reviewer, always use the custom-prompt form and describe the range in the prompt text. - Pass the prompt on stdin with `-` rather than as an argument. Prompts of this size are awkward to quote safely. ~~~~ bash cd "$REPO_ROOT" codex review \ -c model="$CODEX_MODEL" \ -c model_reasoning_effort="high" \ - < "$WORK/codex-prompt.txt" ~~~~ Build `$WORK/codex-prompt.txt` from the template in `references/review-prompts.md` (section “Codex — initial review”), substituting the range and the scope contract. Findings appear at the end of the output; the final block is repeated once, so read it, do not count it twice. Then, for each round: 1. **Triage every finding** against the contract — see below. Never edit straight from a finding. 2. Apply the in-scope fixes. Verify each one against the actual code first; findings are advice, not ground truth, and a confidently wrong one that you apply blindly is how correct code becomes broken code. 3. In post-commit mode, commit the batch now (Stage C rules). 4. Re-run `codex review` with the “Codex — re-review” template, which names the previous round's findings so the fresh session does not re-litigate what you already rejected. **Stop when** a round returns `NO ACTIONABLE FINDINGS`, or when everything it returns is out of scope or fails triage. Cap the loop at **three rounds** — three rounds of new in-scope findings means the change set has a problem that iteration is not converging on, and more rounds drift into nitpicking rather than close the gap. When you hit a cap with findings still open, the loop ends but the work does