/omk-reviewing

name: omk-reviewing
description: "Code and plan review with multi-angle dispatch. Trigger when user says 'review', 'code review', 'check my code', 'PR review', '@review', or when completing a plan phase that requires review. Also trigger before merge, after major feature completion, or when user asks for feedback on implementation quality."

Trigger Examples

• "@review 看看这个 PR"
• "帮我 review 一下这段代码"
• "check my implementation before I merge"
• "review the plan I just wrote"
• "这个改动有没有问题？"

Reviewing — Request, Execute, Receive

Requesting Review

**When (mandatory):** after completing major feature, before merge, after each task batch.

Plan Review — 4 angles, 4 parallel subagents

Pre-review Risk Identification

Before dispatching reviewers, the main agent MUST run the **Pre-mortem Analysis** defined in `skills/planning/SKILL.md` (Phase 1.5 → Pre-mortem Analysis section). This produces 3 risk questions (Integration / Assumption / Environment) that are injected as "Specific Questions" into each reviewer's dispatch query.

Additionally, craft one canary question per dispatch that requires reading a specific source file.

Dispatch

Dispatch exactly **4 reviewer subagents in parallel** (`agent_name: "reviewer"`, `dangerously_trust_all_tools: true`), one per angle:

| # | Angle | Mission | |---|-------|---------| | 1 | Goal Alignment | Every task maps to goal? Execution order valid? Non-goals respected? | | 2 | Verify Correctness | Each verify command sound? False positives/negatives? | | 3 | Completeness | All modified files covered? Edge cases? Conflicts with other plans? | | 4 | Technical Feasibility | Blockers? Contradictions? Race conditions? Signal safety? |

Each subagent query must include: plan file path + relevant source file paths to read.

Deterministic Pre-check (main agent, before dispatching reviewers)

Before dispatching any reviewer subagent, the **main agent** runs deterministic checks (reviewer subagent only has read/write/shell — no LSP tools):

1. Run `get_diagnostics` on all modified files — collect compiler errors/warnings 2. Run `pattern_search` for known anti-patterns (bare except, subprocess without timeout, etc.) 3. Package results as "Pre-check Findings" to include in each reviewer's dispatch query

Pre-check findings are automatically P0/P1 — they don't need LLM judgment. This reduces the reviewer's workload to reasoning-heavy issues only.

Code Review — size-based dispatch

Choose dispatch mode based on diff size:

**Small PR (<200 lines diff):** Dispatch **1 reviewer subagent** (`agent_name: "reviewer"`, `dangerously_trust_all_tools: true`) with:

• What was implemented
• Plan/requirements reference
• Git diff range (BASE_SHA..HEAD_SHA)
• Pre-check Findings from Deterministic Pre-check

**Large PR (≥200 lines diff):** Dispatch **2 reviewer subagents in parallel** (`agent_name: "reviewer"`, `dangerously_trust_all_tools: true`):

| Agent | Angle | Focus | |-------|-------|-------| | 1 | Correctness + Security | Functional correctness, input validation, auth, injection, race conditions | | 2 | Quality + Architecture | SOLID, code smells, performance, error handling, boundary conditions |

Each agent receives: diff range, pre-check findings, and relevant source file paths. Findings from both agents are merged and deduplicated by the main agent before presenting to user.

Iron Principle: Respect the Existing Codebase

> The existing code is the stable, battle-tested baseline. It may be 85/100 — not perfect — but it works. Your job is to review the **new code**, not to fix the old code through the PR author.

• **Only review new/changed lines.** Do not raise findings against unchanged existing code, even if it has style issues, minor inefficiencies, or non-ideal patterns.
• **Judge new code by the standards of the existing codebase**, not by textbook perfection. If the existing code uses `@Autowired` field injection, don't flag the new code for not using constructor injection. If the existing code swallows certain exceptions with a warn log, the new code doing the same is consistent, not a bug.
• **P2/P3 "style improvement" findings on existing patterns are noise.** Only raise findings on existing code if it's P0/P1 (security vulnerability, data loss, crash) AND directly touched by the PR.
• **"While we're here" refactors are out of scope.** If the reviewer wants to suggest improving old code, it goes in a separate follow-up issue, not as a PR comment blocking merge.

Executing Code Review (for reviewer agent)

1) Preflight context

• Run `git diff --stat` then `git diff` to understand scope
• If diff > 500 lines, batch by file/module — review each batch separately
• Note: file renames, new files, deleted files

2) SOLID + architecture check

• Load `references/solid-checklist.md` for coverage
• Check SRP, OCP, LSP, ISP, DIP violations
• Flag common code smells: long methods, feature envy, data clumps, dead code
• Apply refactor heuristics where applicable

3) Security scan

• Load `references/security-checklist.md` for coverage
• Check: input/output safety (XSS, injection, SSRF, path traversal), auth gaps, secrets in code
• Check: race conditions (concurrent access, check-then-act, TOCTOU, missing locks)
• Call out both **exploitability** and **impact**

4) Code quality scan

• Load `references/code-quality-checklist.md` for coverage
• Check: error handling (swallowed exceptions, overly broad catch, async errors)
• Check: performance (N+1 queries, CPU-intensive ops in hot paths, missing cache, unbounded memory)
• Check: boundary conditions (null/undefined, empty collections, numeric boundaries, off-by-one)
• Flag issues that may cause silent failures or production incidents

5) Removal candidates

• Load `references/removal-plan.md` for template
• Identify dead code, unused imports, deprecated patterns
• Categorize: safe to remove now vs defer with plan

6) Output

• Load `references/output-format.md`