Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 6 additions & 11 deletions packages/core/src/skills/bundled/review/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ Copilot's production data (60M+ reviews): 29% return zero comments. This is by d

Applied throughout:

- Linter warnings → terminal only, not PR comments
- Low-confidence findings → terminal only ("Needs Human Review")
- Nice to have → never posted as PR comments
- Uncertain issues → rejected, not reported
Expand Down Expand Up @@ -130,7 +129,7 @@ Line-based classification was chosen because it's deterministic, cheap, and catc

## Why downgrade APPROVE when CI is non-green

**Original behavior:** if Step 7 resolved verdict to `APPROVE`, the API event was submitted as `APPROVE` without any check on CI status.
**Original behavior:** if Step 6 resolved verdict to `APPROVE`, the API event was submitted as `APPROVE` without any check on CI status.

**Problem:** the LLM review pipeline reads the diff and surrounding code statically. It does not run tests, does not exercise integration boundaries, and does not see runtime failures. CI does. A PR with red CI but no static red flags is **the worst case** for an LLM `APPROVE` — the human reader sees an Approve badge from a tool that didn't actually verify the change runs.

Expand All @@ -153,7 +152,7 @@ Line-based classification was chosen because it's deterministic, cheap, and catc

## Why the deterministic checks live as `qwen review` subcommands
Comment thread
wenshao marked this conversation as resolved.
Resolved

**Original behavior:** Step 9's three pre-submission checks (self-PR detection, CI status, existing-comment classification) and Step 11's cleanup were inlined in SKILL.md as `gh api` / `git` shell commands. The LLM ran each command itself, parsed the output, and applied the classification logic.
**Original behavior:** Step 7's three pre-submission checks (self-PR detection, CI status, existing-comment classification) and Step 9's cleanup were inlined in SKILL.md as `gh api` / `git` shell commands. The LLM ran each command itself, parsed the output, and applied the classification logic.

**Problems with inlining:**

Expand Down Expand Up @@ -191,13 +190,10 @@ 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.

**Exception:** Autofix uses a blocking y/n because it modifies code — higher stakes require explicit consent.

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

| Stage | Calls | Why |
| ----------------------- | ----------------- | ------------------------------------------------------------------- |
| Deterministic analysis | 0 | Shell commands — ground truth for free |
| 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 |
Expand All @@ -209,11 +205,11 @@ Competitors: Copilot uses 1 call, Gemini uses 2, Claude /ultrareview uses 5-20 (

## Why cross-repo uses lightweight mode

CLI tools are inherently repo-local. Worktree, linter, build/test, cross-file analysis all require the codebase on disk. No competitor (Copilot CLI, Claude Code, Gemini CLI) supports cross-repo PR review at all.
CLI tools are inherently repo-local. Worktree, build/test, cross-file analysis all require the codebase on disk. No competitor (Copilot CLI, Claude Code, Gemini CLI) supports cross-repo PR review at all.

Our lightweight mode is the best a CLI can do: GitHub API calls work cross-repo (`gh pr diff <url>`, `gh pr view <url>`, `gh api .../comments`), so LLM review and PR comment posting work. Everything that needs local files is skipped. This is strictly better than "not supported."

Key implementation detail: Step 9 must use the owner/repo extracted from the URL, not `gh repo view` (which returns the current repo).
Key implementation detail: Step 7 must use the owner/repo extracted from the URL, not `gh repo view` (which returns the current repo).

## Why auto-discover tools from CI config instead of user configuration

Expand All @@ -222,7 +218,7 @@ Key implementation detail: Step 9 must use the owner/repo extracted from the URL
- **`.qwen/review-tools.md`**: Let projects define custom lint/build/test commands. Precise, but requires users to learn a new config format and maintain it.
Comment thread
wenshao marked this conversation as resolved.
Resolved
- **Auto-discovery from CI config (chosen)**: Read `.github/workflows/*.yml`, `Makefile`, etc. to find what commands the project already runs in CI. Zero user effort.

**Decision:** Auto-discovery. Every project already defines its tool chain in CI config. Reading those files leverages existing knowledge without asking users to duplicate it. The LLM is capable of parsing YAML workflow files and extracting the relevant commands. Falls back gracefully: if no CI config exists, Step 3 is simply skipped and LLM agents still review the diff.
**Decision:** Auto-discovery. Every project already defines its tool chain in CI config. Reading those files leverages existing knowledge without asking users to duplicate it. The LLM is capable of parsing YAML workflow files and extracting the relevant commands. Falls back gracefully: if no CI config exists, the build/test discovery is simply skipped and LLM agents still review the diff.

## Rejected alternatives

Expand All @@ -231,7 +227,6 @@ Key implementation detail: Step 9 must use the owner/repo extracted from the URL
| `.qwen/review-tools.md` for custom tool config | Requires users to learn a new format. Auto-discovery from CI config achieves the same result with zero user effort. |
| Use fast model for verification/reverse audit | User requirement: quality first. Fast models may miss subtle issues. |
| Reduce to 2 agents (like Gemini) | Loses dimensional focus. Gemini compensates with deterministic tasks; we already have those AND want higher LLM coverage. |
Comment thread
wenshao marked this conversation as resolved.
Resolved
| Auto-approve PR after autofix | Remote PR still has original code until push. Approving unfixed code is misleading. |
| `mktemp` for temp files | Over-engineering for a prompt. `{target}` suffix is sufficient for CLI concurrent sessions. |
| Mermaid diagrams in docs | Only renders on GitHub. ASCII diagrams are universally compatible. |
| `gh pr checkout --detach` for worktree | It modifies the current working tree, defeating the purpose of worktree isolation. |
Expand Down Expand Up @@ -276,7 +271,7 @@ With Fork + prompt cache sharing:

**Additional benefits for /review:**

- Forked agents inherit Step 3 linter results, PR context, review rules — no need to repeat in each agent prompt
- Forked agents inherit PR context and review rules — no need to repeat in each agent prompt
- SKILL.md workaround "Do NOT paste the full diff into each agent's prompt" becomes unnecessary — fork already has the context
- Verification and reverse audit agents inherit all prior findings naturally
- Agent 6 personas can fork from a shared diff-loaded base, paying only the persona-framing delta
Expand Down
Loading
Loading