--- name: code-review description: Run a comprehensive code review --- # Code Review Skill Conduct a thorough, read-only code review for quality, security, and maintainability with severity-rated feedback. ## When to Use This skill activates when: - User requests "review this code", "code review" - Before merging a pull request - After implementing a major feature - User wants quality assessment Plain `/omagy:code-review` is read-only. It does not edit files, run automatic fixes, persist workflow gate state, or promise follow-up implementation. If the user explicitly asks to review and fix, run this review first and then route the findings into `/omagy:ralph` or `/omagy:ultrawork`. ## GPT-5.5 Guidance Alignment - Default to outcome-first progress and completion reporting: state the target result, evidence, validation status, and stop condition before adding process detail. - Treat newer user task updates as local overrides for the active workflow branch while preserving earlier non-conflicting constraints. - If correctness depends on additional inspection, retrieval, execution, or verification, keep using the relevant tools until the review is grounded; stop once enough evidence exists. - Continue through clear, low-risk, reversible next steps automatically; ask only when the next step is materially branching, destructive, credentialed, external-production, or preference-dependent. Delegates to the `code-reviewer` and `architect` agents in parallel for a two-lane review. When the scoped files or task are security-sensitive, add a conditional `security-reviewer` lane instead of using a standalone `/omagy:security-review` command. 1. **Identify Changes** - Run `git diff` to find changed files when the current scope is a git repository. - Determine scope of review: specific files, current project directory, explicit path, branch, PR, or entire change set. - If `git diff` is unavailable but the current directory is clearly the project under review, use the current project directory as the scope. - Do not scan sibling repositories to pick a target implicitly. - Stay inside the current review scope. Do not inspect Antigravity brain, transcript, conversation, or generated log directories unless the user explicitly includes them in the review scope. - If there is no changed-file or PR scope and the review falls back to the current project directory, treat the result as a broad project audit, not a merge gate for a specific change. In audit mode, report dormant, internal, or future-workflow risks as WATCH unless they break a current entrypoint or create a concrete security, data-loss, or production incident risk. 2. **Launch Parallel Review Lanes** - **`code-reviewer` lane** - owns spec compliance, security, code quality, performance, and maintainability findings - **`architect` lane** - owns the devil's-advocate / design-tradeoff perspective - **`security-reviewer` lane** - conditional lane for auth, secrets, permissions, injection, sandbox, plugin, MCP, subprocess, dependency, supply-chain, PII, or compliance risk - Both lanes run in parallel and produce distinct outputs before final synthesis 3. **Review Categories** - **Security** - Hardcoded secrets, injection risks, XSS, CSRF - **Code Quality** - Function size, complexity, nesting depth - **Performance** - Algorithm efficiency, N+1 queries, caching - **Best Practices** - Naming, documentation, error handling - **Maintainability** - Duplication, coupling, testability 4. **Severity Rating** - **CRITICAL** - Security vulnerability (must fix before merge) - **HIGH** - Bug or major code smell (should fix before merge) - **MEDIUM** - Minor issue (fix when possible) - **LOW** - Style/suggestion (consider fixing) 5. **Architectural Status Contract** - **CLEAR** - No unresolved architectural blocker was found - **WATCH** - Non-blocking design/tradeoff concern that must appear in the final synthesis - **BLOCK** - Unresolved design concern that prevents a merge-ready verdict 6. **Specific Recommendations** - File:line locations for each issue - Concrete fix suggestions - Code examples where applicable 7. **Final Synthesis** - Combine the `code-reviewer` recommendation and the architect status into one final verdict - Deterministic merge gating rules: - If architect status is **BLOCK**, final recommendation is **REQUEST CHANGES** - If security status is **BLOCK**, final recommendation is **REQUEST CHANGES** - Else if `code-reviewer` recommendation is **REQUEST CHANGES**, final recommendation is **REQUEST CHANGES** - Else if security status is **WATCH**, final recommendation is **COMMENT** - Else if architect status is **WATCH**, final recommendation is **COMMENT** - Else final recommendation follows the `code-reviewer` lane - The final report must make architect blockers impossible to miss - The final report must make security blockers impossible to miss ## Agent Delegation Use agy's native subagent capability directly. The intended call shape is: ```json { "Subagents": [ { "TypeName": "code-reviewer", "Role": "code-reviewer", "Prompt": "CODE REVIEW TASK\n\nReview code changes for quality, security, and maintainability.\n\nThis is the code/spec/security lane. Do not absorb architectural ownership.\n\nScope: [git diff, current project directory, PR, or specific files]\n\nStay inside the scope. Do not inspect Antigravity brain, transcript, conversation, or generated log directories unless explicitly included.\n\nReview Checklist:\n- Security vulnerabilities (OWASP Top 10)\n- Code quality (complexity, duplication)\n- Performance issues (N+1, inefficient algorithms)\n- Best practices (naming, documentation, error handling)\n- Maintainability (coupling, testability)\n\nOutput: Code review report with:\n- Files reviewed count\n- Issues by severity (CRITICAL, HIGH, MEDIUM, LOW)\n- Specific file:line locations\n- Fix recommendations\n- Approval recommendation (APPROVE / REQUEST CHANGES / COMMENT)" }, { "TypeName": "architect", "Role": "architect", "Prompt": "ARCHITECTURE / DEVIL'S-ADVOCATE REVIEW TASK\n\nReview the same code changes from the architecture/tradeoff perspective.\n\nScope: [same scope as code-reviewer]\n\nStay inside the scope. Do not inspect Antigravity brain, transcript, conversation, or generated log directories unless explicitly included.\n\nFocus:\n- System boundaries and interfaces\n- Hidden coupling or long-term maintainability risks\n- Tradeoff tension the main reviewer might miss\n- Strongest counterargument against approving as-is\n\nBLOCK only when the scoped change or current audited entrypoint is not merge-ready because of a concrete unresolved design risk. In broad project audit mode, dormant/internal/future-workflow risks are WATCH unless they break a current entrypoint or create concrete security, data-loss, or production incident risk.\n\nOutput:\n- Architectural Status: CLEAR / WATCH / BLOCK\n- File:line evidence for each concern\n- Concrete tradeoff or design recommendation" } ] } ``` Run both lanes in parallel, then synthesize them with the deterministic rules above. ## External Model Consultation (Preferred) The code-reviewer agent SHOULD consult another approved model or reviewer surface for cross-validation when one is already available in the agy session. ### Protocol 1. **Form your OWN review FIRST** - Complete the review independently 2. **Consult for validation** - Cross-check findings with the external reviewer 3. **Critically evaluate** - Never blindly adopt external findings 4. **Graceful fallback** - Never block if tools unavailable ### When to Consult - Security-sensitive code changes - Complex architectural patterns - Unfamiliar codebases or languages - High-stakes production code ### When to Skip - Simple refactoring - Well-understood patterns - Time-critical reviews - Small, isolated changes ### Tool Usage Prefer native `code-reviewer` agent consultation or available CLI-backed ask surfaces when enabled. Optional MCP compatibility ask tools may be used only when already enabled. If consultation tools are unavailable, fall back to the `code-reviewer` and `architect` agents. ## Output Format ```text CODE REVIEW REPORT ================== Scope Mode: DIFF_REVIEW Scope: git diff / PR / explicit files Files Reviewed: 8 Total Issues: 12 Architectural Status: WATCH CRITICAL (0) ----------- (none) HIGH (0) -------- (none) MEDIUM (7) ---------- 1. src/api/auth.ts:42 Issue: Email normalization logic is duplicated instead of reusing the shared helper Risk: Validation rules can drift between authentication paths Fix: Route both paths through the shared normalization helper 2. src/components/UserProfile.tsx:89 Issue: Derived permissions are recalculated on every render Risk: Avoidable work during profile refreshes Fix: Memoize the derived permissions list or compute it upstream 3. src/utils/validation.ts:15 Issue: Form-layer and server-layer validation messages are defined separately Risk: User-facing validation guidance can become inconsistent Fix: Share one validation message helper across both call sites LOW (5) ------- ... ARCHITECTURE WATCHLIST ---------------------- - src/review/orchestrator.ts:88 Concern: Review result synthesis relies on implicit ordering rather than an explicit blocker contract Status: WATCH Recommendation: Define deterministic merge gating before expanding reviewers SYNTHESIS --------- - code-reviewer recommendation: COMMENT - architect status: WATCH - final recommendation: COMMENT RECOMMENDATION: COMMENT Address any WATCH concerns before treating the change as merge-ready. ``` When there is no diff/PR/explicit change set and the review falls back to the current project directory, report audit mode explicitly: ```text Scope Mode: PROJECT_AUDIT Scope: current project directory Files Reviewed: 4 focused files Scope Note: Broad project audit, not a merge gate for a specific change. SYNTHESIS --------- - code-reviewer recommendation: REQUEST CHANGES / COMMENT / APPROVE - architect status: CLEAR / WATCH / BLOCK - audit recommendation: REQUEST CHANGES / COMMENT / APPROVE Audit Recommendation: REQUEST CHANGES / COMMENT / APPROVE Merge Recommendation: N/A - no specific diff, PR, or change set was reviewed. ``` In `PROJECT_AUDIT` mode, never present the final verdict as a direct merge recommendation. Use `Audit Recommendation` for the project-level quality result and keep `Merge Recommendation` as `N/A` unless a specific diff, PR, or explicit change set was reviewed. Do not write `final recommendation` or bare `RECOMMENDATION` in `PROJECT_AUDIT` mode. ## Review Checklist The `code-reviewer` lane checks: ### Security - [ ] No hardcoded secrets (API keys, passwords, tokens) - [ ] All user inputs sanitized - [ ] SQL/NoSQL injection prevention - [ ] XSS prevention (escaped outputs) - [ ] CSRF protection on state-changing operations - [ ] Authentication/authorization properly enforced ### Code Quality - [ ] Functions < 50 lines (guideline) - [ ] Cyclomatic complexity < 10 - [ ] No deeply nested code (> 4 levels) - [ ] No duplicate logic (DRY principle) - [ ] Clear, descriptive naming ### Performance - [ ] No N+1 query patterns - [ ] Appropriate caching where applicable - [ ] Efficient algorithms (avoid O(n²) when O(n) possible) - [ ] No unnecessary re-renders (React/Vue) ### Best Practices - [ ] Error handling present and appropriate - [ ] Logging at appropriate levels - [ ] Documentation for public APIs - [ ] Tests for critical paths - [ ] No commented-out code ## Architect Lane Checklist The `architect` lane checks: - [ ] Boundary or interface changes are explicit - [ ] New coupling/tradeoff risks are surfaced - [ ] Long-horizon maintainability concerns are evidence-backed - [ ] Architectural status is one of `CLEAR`, `WATCH`, or `BLOCK` - [ ] Any `BLOCK` concern cites the reason merge-ready status should be withheld ## Approval Criteria **APPROVE** - `code-reviewer` returns APPROVE and architect status is `CLEAR` **REQUEST CHANGES** - `code-reviewer` returns REQUEST CHANGES or architect status is `BLOCK` **COMMENT** - `code-reviewer` returns COMMENT with architect status `CLEAR`, architect status is `WATCH`, or only LOW/MEDIUM improvements remain ## Scenario Examples **Good:** The user says `continue` after the workflow already has a clear next step. Continue the current branch of work instead of restarting or re-asking the same question. **Good:** The user changes only the output shape or downstream delivery step (for example `make a PR`). Preserve earlier non-conflicting workflow constraints and apply the update locally. **Bad:** The user says `continue`, and the workflow restarts discovery or stops before the missing verification/evidence is gathered. ## Use with Other Skills **With Team:** ```text /omagy:team "review recent auth changes and report findings" ``` Includes coordinated review execution across specialized agents. **With Ralph:** ```text /omagy:ralph code-review then fix all issues ``` On the explicit Ralph path, review findings should flow into automatic fix follow-up without another permission prompt. Plain `code-review` itself remains read-only and does **not** promise auto-fix. **With Ultrawork:** ```text /omagy:ultrawork review all files in src/ ``` Parallel code review across multiple files. Plain code-review does not write workflow gate state. Structured review gate state belongs to higher-level workflow or pipeline commands, not the default code-review skill path.