Context-rich review mode: isolated reviewer subagent with a rubric and structured findings

[emal-avala]

Problem

/review today is a single line — one sentence handed to the main conversation:

Some("review") => CommandResult::Prompt(
    "Review the current git diff. Look for bugs, security issues, \
     code quality problems, and suggest improvements."
        .to_string(),
),

That inherits every weakness of the ambient session: the reviewer sees whatever context the conversation already accumulated (including its own earlier reasoning about the code it is now reviewing), there is no rubric, no target selection, no structured output, and no way for another tool to consume the result. It also cannot say "this patch is correct" — there is nothing to say it about.

The gap is not "we should feed it a bigger diff". A useful reviewer is an agent with repo access and a narrow brief, not a diff summarizer.

What a context-rich review actually requires

I studied the reference implementation in codex-rs (core/src/tasks/review.rs, core/src/session/review.rs, prompts/src/review_request.rs, prompts/templates/review/rubric.md). Six design decisions carry the weight, and each maps onto something agent-code either lacks or has in a weaker form.

1. The review runs in an isolated sub-agent thread, not the current conversation.
run_codex_thread_one_shot(..., SubAgentSource::Review, ...) starts a fresh thread with its own history. This is the single most important property: a reviewer that has been in the room while the code was written is compromised — it will rationalize its own choices. Fresh context is what makes the finding trustworthy.

2. The sub-agent's configuration is constrained, not merely configured.

sub_agent_config.web_search_mode.set(WebSearchMode::Disabled);
sub_agent_config.features.disable(Feature::Collab);
sub_agent_config.base_instructions = Some(REVIEW_PROMPT.to_string());
sub_agent_config.permissions.approval_policy =
    Constrained::allow_only(AskForApproval::Never);

base_instructions is replaced, not appended — the reviewer is not a general assistant wearing a hat. approval_policy = Never matters for a different reason than it looks: a review must never block on a prompt, because nobody is watching it.

3. Targets are prompts that teach the agent how to find the diff.

pub enum ReviewTarget {
    UncommittedChanges,
    BaseBranch { branch: String },
    Commit { sha: String, title: Option<String> },
    Custom { instructions: String },
}

The base-branch prompt does not embed a diff. It computes the merge base and tells the agent to go get it:

"The merge base commit for this comparison is {{merge_base_sha}}. Run git diff {{merge_base_sha}} to inspect the changes relative to {{base_branch}}."

This is the "more context than the diff" part. The reviewer has tools, so it can open the files around a hunk, read the callers, check whether a test exists — the things that separate a real finding from a plausible one.

4. A rubric that mostly says what not to flag. The 95-line rubric is largely negative space, and that is why it works:

• the bug must have been introduced in this change (pre-existing bugs are out of scope)
• not speculative — "one must identify the other parts of the code that are provably affected"
• not a demand for rigor absent from the rest of the codebase
• "If there is no finding that a person would definitely love to see and fix, prefer outputting no findings"
• priority tags P0–P3 with P0 reserved for issues that "do not depend on any assumptions about the inputs"

It also requires attribution against repository instruction files (AGENTS.md and scoped equivalents, with precedence), and only counts a finding as rule-supported when the rule "materially contributes repository-specific scope, an invariant, remedy, convention, or confirmation behavior beyond generic correctness advice".

5. Structured output, not prose.

{
  "findings": [{
    "title": "...", "body": "...",
    "confidence_score": 0.0,
    "priority": 0,
    "code_location": {
      "absolute_file_path": "...",
      "line_range": {"start": 0, "end": 0}
    }
  }],
  "overall_correctness": "patch is correct" | "patch is incorrect",
  "overall_explanation": "...",
  "overall_confidence_score": 0.0
}

This is what makes the review consumable — by an inline UI, by CI, or by another agent. Prose findings cannot be filtered, ranked, deduplicated, or resolved.

6. Results re-enter the parent thread as a bounded, labelled block, not as free chat:

<user_action>
  <context>User initiated a review task. Here's the full review output from reviewer model.
           User may select one or more comments to resolve.</context>
  <action>review</action>
  <results>{{results}}</results>
</user_action>

Proposed implementation for agent-code

A. A bundled skill is the wrong primary home; a task kind is the right one

crates/lib/src/skills/bundled/ would give us the rubric and the prompt for free, and a skill is the fastest way to prototype the rubric. But a skill runs inside the current conversation, which forfeits design decision #1 — the property that makes the review worth having.

Proposal: implement review as a first-class task that spawns a constrained subagent, and keep a thin skill as the entry point so it is discoverable and overridable per project.

We already have the pieces:

• TaskKind::LocalAgent and TaskManager for a detached, isolated run (crates/lib/src/services/background.rs)
• PermissionMode::Plan / read-only tool profiles to constrain the reviewer
• AMR (crates/lib/src/amr/) already does whole-repo agentic map-reduce with a confined read_scope — the review reviewer wants exactly that confinement
• PermissionChecker::with_read_scope to stop a prompt-injected file from making the reviewer read ~/.ssh

B. Concretely

1. ReviewTarget enum + prompt resolution in crates/lib/src/review/ — Uncommitted, BaseBranch{branch}, Commit{sha}, Custom{instructions}. Base-branch resolution computes the merge base with git merge-base HEAD <base>@{upstream} and passes the SHA, so the reviewer diffs against the right point rather than guessing.
2. A rubric prompt as a bundled asset, overridable by .agent/review-rubric.md so a project can tighten it. Ours should encode this repo's own conventions — AGENTS.md §5, the CI gate, "no ad-hoc RGB" — since a reviewer that does not know the house rules produces generic findings.
3. A constrained subagent run: fresh history, review rubric as base instructions, approval_policy = Deny-equivalent (never prompt), read-only tool profile plus git, no network tools, and review_model config so a stronger model can be used for review than for editing.
4. A ReviewFinding struct + JSON schema with the fields above, parsed from the subagent's final message, with a lenient parser: a review that produces unparseable output should degrade to "here is the prose" rather than being lost.
5. Surfaces: /review [uncommitted|base <branch>|commit <sha>|<instructions>] in the TUI, rendering findings as navigable cards (we now have the tasks-pane drill-in and transcript search to build on), plus agent review --json for CI.

C. The Claude Code interop the issue title asks for

agent serve already exposes POST /message (crates/cli/src/serve.rs). Two additions make agent-code drivable as a review backend from Claude Code or any other client:

• POST /review taking {target, instructions?, model?} and returning the structured findings JSON — the same shape the TUI renders. This is the clean integration point: Claude Code shells out or posts, gets machine-readable findings back, and can present or act on them.
• A bundled skill (review.md) so the flow is discoverable from inside agent-code itself and so a project can override the rubric without a rebuild.

The reason to expose it over the existing serve API rather than only as a CLI is that a review is long-running and streams: the caller wants progress, and serve already has the event plumbing.

Why this is worth doing

We already ship an unusually strong substrate for it — AMR proves the confined-agentic-scan pattern works here, and the permission work landed this cycle (per-invocation command matching, path-traversal normalization, read-scope confinement) means a reviewer subagent can be given repo access without giving it the machine.

The current /review is one sentence. Everything above is a design that a reviewer would actually trust.

Acceptance criteria

• ReviewTarget with the four variants; base-branch resolves a real merge base
• Rubric asset, project-overridable, encoding this repo's conventions
• Review runs in a subagent with fresh history and a replaced (not appended) system prompt
• Reviewer cannot prompt for approval and cannot reach the network
• Structured findings parse, with graceful degradation to prose
• /review renders findings; agent review --json prints them
• POST /review on agent serve returns the same structure
• Tests: prompt resolution per target, merge-base computation, JSON parsing incl. malformed output, and a test that the reviewer's history does not inherit the parent conversation

Non-goals

• Posting findings to GitHub (the existing bot covers that)
• Auto-fixing findings — the rubric explicitly says "Do not generate a PR fix"
• Replacing /security-review