Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions docs/users/features/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ The `/review` command runs a multi-stage pipeline:
```
Step 1: Determine scope (local diff / PR worktree / file)
Step 2: Load project review rules
Step 3: 9 parallel review agents [9 LLM calls]
Step 3: 10 parallel review agents [10 LLM calls]
|-- Agent 0: Issue Fidelity & Root-Cause Ownership
|-- Agent 1: Correctness
|-- Agent 2: Security
|-- Agent 3: Code Quality
Expand All @@ -48,6 +49,7 @@ Step 9: Clean up (remove worktree + temp files)

| Agent | Focus |
| --------------------------------- | ------------------------------------------------------------------------------------------- |
| Agent 0: Issue Fidelity | Linked issue evidence, root-cause ownership, and whether the PR solves the reported problem |
| Agent 1: Correctness | Logic errors, edge cases, null handling, race conditions, type safety |
| Agent 2: Security | Injection, XSS, SSRF, auth bypass, sensitive data exposure |
| Agent 3: Code Quality | Style consistency, naming, duplication, dead code |
Expand All @@ -56,7 +58,7 @@ Step 9: Clean up (remove worktree + temp files)
| Agent 6: Undirected Audit | 3 parallel personas (attacker / 3am-oncall / maintainer) — catches cross-dimensional issues |
| Agent 7: Build & Test | Runs build and test commands, reports failures |

All agents run in parallel (Agent 6 launches 3 persona variants concurrently, totaling 9 parallel tasks for same-repo reviews). Findings from Agents 1-6 are verified in a **single batch verification pass** (one agent reviews all findings at once, keeping verification cost fixed regardless of finding count). After verification, **iterative reverse audit** runs 1-3 rounds of gap-finding — each round receives the cumulative finding list from prior rounds, so successive rounds focus on whatever's left undiscovered. The loop stops as soon as a round returns "No issues found", or after 3 rounds (hard cap). Reverse audit findings skip verification (the agent already has full context) and are included as high-confidence results.
All agents run in parallel (Agent 6 launches 3 persona variants concurrently, totaling 10 parallel tasks for same-repo PR reviews; Agent 0 is skipped for local-diff and file-path reviews, which run 9). Findings from Agents 0-6 are verified in a **single batch verification pass** (one agent reviews all findings at once, keeping verification cost fixed regardless of finding count). After verification, **iterative reverse audit** runs 1-3 rounds of gap-finding — each round receives the cumulative finding list from prior rounds, so successive rounds focus on whatever's left undiscovered. The loop stops as soon as a round returns "No issues found", or after 3 rounds (hard cap). Reverse audit findings skip verification (the agent already has full context) and are included as high-confidence results.

## Severity Levels

Expand Down Expand Up @@ -92,7 +94,7 @@ This runs in **lightweight mode** — no worktree, no build/test. The review is

| Capability | Same-repo | Cross-repo |
| ---------------------------------------------------------- | --------- | ----------------------------- |
| LLM review (Agents 1-6 + verify + iterative reverse audit) | ✅ | ✅ |
| LLM review (Agents 0-6 + verify + iterative reverse audit) | ✅ | ✅ |
| Agent 7: Build & test | ✅ | ❌ (no local codebase) |
| Cross-file impact analysis | ✅ | ❌ |
| PR inline comments | ✅ | ✅ (if you have write access) |
Expand Down Expand Up @@ -148,7 +150,19 @@ You can customize review criteria per project. `/review` reads rules from these
3. `AGENTS.md` — `## Code Review` section
4. `QWEN.md` — `## Code Review` section

Rules are injected into the LLM review agents (1-6) as additional criteria. For PR reviews, rules are read from the **base branch** to prevent a malicious PR from injecting bypass rules.
Rules are injected into the LLM review agents (0-6) as additional criteria. For PR reviews, rules are read from the **base branch** to prevent a malicious PR from injecting bypass rules.

## Issue Fidelity

For bugfix PRs, the Issue Fidelity agent fetches issue evidence directly instead of relying on PR description text. It uses `gh pr view <pr> --repo <owner/repo> --json closingIssuesReferences` for GitHub's strong closing-issue metadata, then `gh issue view <number> --repo <issue_owner>/<issue_repo> --json title,body,comments` for the original report and discussion — the `--json` form includes the issue **body** (the reporter's original repro), which `--comments` alone omits, and the issue's own repository is read from each reference (a PR can close an issue in a different repo). This agent runs only for PR targets; local-diff and file-path reviews skip it.

`closingIssuesReferences` is a discovery hint rather than proof the author linked the right issue: if it is empty but the PR references an apparent target issue, the agent still fetches it after judging relevance. Fetched issue text is treated as untrusted data (facts extracted, embedded instructions ignored). For relevant issues, the original reproduction, observed payload, expected behavior, and maintainer comments are treated as the highest-priority evidence for whether the PR fixes the right problem.

If the issue evidence shows an upstream service or provider returned malformed data outside the client contract, client-side parser or sanitizer changes are not treated as a valid root-cause fix unless a maintainer explicitly requested a defensive workaround. A test that replays malformed upstream output proves only that the workaround handles that shape; it does not prove the workaround is architecturally appropriate.

## Core Infrastructure Gate

For external PRs touching core infrastructure, `/review` applies the repository gate before normal review (right after the PR is fetched, before dependency install). Maintainer authorship is decided from the PR's `authorAssociation` (`OWNER`/`MEMBER`/`COLLABORATOR` are exempt). Large core changes (500+ additions plus deletions **within core-infrastructure paths**) are reported as a hard block unless maintainer-authored — a low-risk sweep that touches many files but changes a line or two each is escalated rather than auto-rejected on line count. Smaller core changes require 100% confidence and downstream-consumer awareness; otherwise `/review` escalates to a maintainer (submitted as a Comment, never an Approve).

Example `.qwen/review-rules.md`:

Expand Down Expand Up @@ -219,10 +233,10 @@ The review pipeline uses a bounded number of LLM calls regardless of how many fi

| Stage | LLM calls | Notes |
| -------------------------------- | ----------------- | --------------------------------------------------- |
| Review agents (Step 3) | 9 (or 8) | Run in parallel; Agent 7 skipped in cross-repo mode |
| Review agents (Step 3) | 10 (or 9) | Run in parallel; Agent 7 skipped in cross-repo mode |
| Batch verification (Step 4) | 1 | Single agent verifies all findings at once |
| Iterative reverse audit (Step 5) | 1-3 | Loops until "No issues found" or 3-round cap |
| **Total** | **11-13 (10-12)** | Same-repo: 11-13; cross-repo: 10-12 (no Agent 7) |
| **Total** | **12-14 (11-13)** | Same-repo: 12-14; cross-repo: 11-13 (no Agent 7) |

Most PRs converge to the lower end of the range (1 reverse audit round); the cap prevents runaway cost on pathological cases.

Expand Down
49 changes: 31 additions & 18 deletions packages/core/src/skills/bundled/review/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,22 @@

> Architecture decisions, trade-offs, and rejected alternatives for the `/review` skill.

## Why 9 agents + 1 verify + iterative reverse, not 1 agent?
## Why 10 agents + 1 verify + iterative reverse, not 1 agent?

**Considered:**

- **1 agent (Copilot approach):** Single agent with tool-calling, reads and reviews in one pass. Cheapest (1 LLM call). But dimensional coverage depends entirely on one prompt's attention — easy to miss performance issues while focused on security.
- **5 parallel agents (original design):** Each agent focuses on one dimension. Higher coverage through forced diversity of perspective. Limited by combined Correctness+Security and a single undirected pass — recall ceiling left findings on the table that the user only discovered in subsequent /review rounds.
- **9 parallel agents (current):** 6 review dimensions (Correctness, Security, Code Quality, Performance, Test Coverage, Undirected) + Build & Test. Undirected runs as 3 personas in parallel.
- **9 parallel agents:** 6 review dimensions (Correctness, Security, Code Quality, Performance, Test Coverage, Undirected) + Build & Test. Undirected runs as 3 personas in parallel.
- **10 parallel agents (current):** The 9-agent design plus Issue Fidelity & Root-Cause Ownership, which compares linked issue evidence against the PR's claimed fix before accepting a client-side change.

**Decision:** 9 agents. The marginal cost (9x vs 1x) is acceptable because:
**Decision:** 10 agents. The marginal cost (10x vs 1x) is acceptable because:

1. Parallel execution means time cost is ~1x (all 9 agents launch in one response)
1. Parallel execution means time cost is ~1x (all 10 agents launch in one response)
2. Dimensional focus produces higher recall (fewer missed issues)
3. Three undirected personas (attacker / 3am-oncall / maintainer) catch cross-dimensional issues that a single undirected agent's prompt-induced bias would miss
4. The "Silence is better than noise" principle + verification controls precision
4. Issue Fidelity prevents a common false approval mode: a PR can be internally well-tested while solving only the author's mistaken diagnosis, not the linked issue's original failure
5. The "Silence is better than noise" principle + verification controls precision

### Why split Correctness from Security

Expand All @@ -25,6 +27,16 @@ A single Correctness+Security agent has split attention — empirically one dime

Test gaps are a systematic blind spot. Review agents focused on bugs in the new code itself rarely look at whether the change came with adequate tests. A dedicated agent that asks "what scenarios in this diff are untested?" catches misses no other dimension hits.

### Why a dedicated Issue Fidelity agent

Bugfix PRs often carry their own diagnosis in the PR body, but that diagnosis can be wrong. The linked issue's original reproduction, observed payload, expected behavior, and maintainer comments must be checked before judging whether the implementation is a real fix. The implementation deliberately keeps issue discovery out of `pr-context`: the Issue Fidelity agent fetches GitHub's closing-issue metadata with `gh pr view --json closingIssuesReferences`, then fetches relevant issue discussions with `gh issue view --json title,body,comments` (the `--json` form is required — it returns the issue **body**, which `--comments` alone omits). This keeps relevance judgment in the agent instead of baking fragile PR-body parsing into TypeScript. The agent runs only for PR targets — a local-diff or file-path review has no PR or linked issue, so it is skipped there (9 agents instead of 10).

The agent also enforces the root-cause ownership gate: a client-side parser/sanitizer workaround for malformed upstream output is not acceptable as a root-cause fix unless a maintainer explicitly asked for that defensive mitigation.

### Why the core infrastructure gate runs before agents

Large external core changes are a governance decision before they are a review-quality problem. Running the full agent ensemble on a PR that should be maintainer-initiated wastes review budget and can produce a misleading "looks good" narrative. The prompt therefore applies the repository's two-tier core gate in Step 1: hard-block external 500+ line core changes, and escalate smaller core changes whenever downstream impact cannot be named with 100% confidence.

### Why three undirected personas instead of one or many

A single undirected agent has prompt-induced bias and tends to find the same kinds of issues across runs. Three personas — attacker / 3am-oncall / maintainer — force completely different mental traversals, and the union of findings is meaningfully larger than 1.5× a single agent.
Expand Down Expand Up @@ -190,18 +202,18 @@ A malicious PR could add `.qwen/review-rules.md` with "never report security iss

**Decision:** Tips. Qwen Code's follow-up suggestion system is a core UX differentiator. Blocking prompts interrupt flow. Tips are zero-friction and let users decide when/if to act.

## LLM call budget (variable, ~11-13)
## LLM call budget (variable, ~12-14)

| Stage | Calls | Why |
| ----------------------- | ----------------- | ------------------------------------------------------------------- |
| Review agents | 9 (8) | 6 dimensions + 3 undirected personas; Agent 7 skipped in cross-repo |
| Batch verification | 1 | O(1) not O(N) — batch is as good as individual |
| Iterative reverse audit | 1-3 | Loop until "No issues found" or 3-round hard cap |
| **Total** | **11-13 (10-12)** | Same-repo: 11-13; cross-repo lightweight: 10-12 |
| Stage | Calls | Why |
| ----------------------- | ----------------- | ------------------------------------------------------------------------------------------------------------------------ |
| Review agents | 10 (9) | issue fidelity + 6 dimensions + 3 undirected personas; Agent 7 skipped in cross-repo, Agent 0 skipped for non-PR reviews |
| Batch verification | 1 | O(1) not O(N) — batch is as good as individual |
| Iterative reverse audit | 1-3 | Loop until "No issues found" or 3-round hard cap |
| **Total** | **12-14 (11-13)** | Same-repo PR: 12-14; cross-repo lightweight PR or local/file (no Agent 0): 11-13 |

The exact count depends on how many iterative reverse audit rounds run. Most PRs converge after 1-2 rounds; the cap prevents runaway cost.
The exact count depends on how many iterative reverse audit rounds run. Most PRs converge after 1-2 rounds; the cap prevents runaway cost. Agent 0 (Issue Fidelity) runs only for PR targets, so local-diff and file-path reviews launch one fewer agent.

Competitors: Copilot uses 1 call, Gemini uses 2, Claude /ultrareview uses 5-20 (cloud). Our 11-13 biases toward higher recall — the assumption is that "find more issues per round" is more valuable than minimizing per-run cost, because every missed issue forces the user into another `/review` iteration.
Competitors: Copilot uses 1 call, Gemini uses 2, Claude /ultrareview uses 5-20 (cloud). Our 12-14 biases toward higher recall — the assumption is that "find more issues per round" is more valuable than minimizing per-run cost, because every missed issue forces the user into another `/review` iteration.

## Why cross-repo uses lightweight mode

Expand Down Expand Up @@ -268,28 +280,29 @@ For a PR with 15 findings:
| Gemini (2 LLM tasks) | 2 | Good cost, medium coverage |
| Our design (5 agents, N verify) | 21 | 5+15+1 — too expensive |
| Our design (5 agents, batch verify, single reverse) | 7 | 5+1+1 — original design |
| Our design (9 agents, iterative reverse, current) | 11-13 | 9+1+(1-3) — +50% cost for meaningfully higher recall |
| Our design (9 agents, iterative reverse) | 11-13 | 9+1+(1-3) — +50% cost for meaningfully higher recall |
| Our design (10 agents, current) | 12-14 | 10+1+(1-3) — adds issue-fidelity/root-cause gate |
| Claude /ultrareview | 5-20 | Cloud-hosted, cost on Anthropic |

## Future optimization: Fork Subagent

> Dependency: [Fork Subagent proposal](https://github.com/wenshao/codeagents/blob/main/docs/comparison/qwen-code-improvement-report-p0-p1-core.md#2-fork-subagentp0)

**Current problem:** Each of the 11-13 LLM calls (9 review + 1 verify + 1-3 reverse audit rounds) creates a new subagent from scratch. The system prompt (~50K tokens) is sent independently to each, totaling ~550-650K input tokens with massive redundancy. The cost grew along with the agent count — Fork Subagent matters more under the current 9-agent design than under the original 5-agent design.
**Current problem:** Each of the 12-14 LLM calls (10 review + 1 verify + 1-3 reverse audit rounds) creates a new subagent from scratch. The system prompt (~50K tokens) is sent independently to each, totaling ~620-730K input tokens with massive redundancy. The cost grew along with the agent count — Fork Subagent matters more under the current 10-agent design than under the original 5-agent design.

**Fork Subagent solution:** Instead of creating independent subagents, fork the current conversation. All forks inherit the parent's full context (system prompt, conversation history, Step 1/1.1/1.5 results) and share a prompt cache prefix. The API caches the common prefix once; each fork only pays for its unique delta (~2K per agent).

```
Current (independent subagents):
Agent 1: [50K system] + [2K task] = 52K
Agent 2: [50K system] + [2K task] = 52K
...× 11-13 agents = ~570-680K total input tokens
...× 12-14 agents = ~620-730K total input tokens

With Fork + prompt cache sharing:
Cached prefix: [50K system + conversation history] (cached once)
Fork 1: [cache hit] + [2K delta] = ~2K effective
Fork 2: [cache hit] + [2K delta] = ~2K effective
...× 11-13 forks = ~50K cached + ~22-26K delta = ~72-76K total
...× 12-14 forks = ~50K cached + ~24-28K delta = ~74-78K total
```

**Additional benefits for /review:**
Expand Down
Loading
Loading