Review code

review-code

Overview

Handle both sides of code review: requesting it (dispatching a reviewer subagent) and receiving it (processing feedback from humans or external reviewers). In both cases, the standard is technical evaluation, not emotional performance. Fix what is wrong. Push back on what is not. Skip the theater.

Announcement

When this skill activates, announce:

I'm using enggenie:review-code to [request a review / process review feedback].

Pick the correct variant based on the trigger.

---

Jira Ticket Entry

When the user references a Jira ticket for review (e.g., "Review PROJ-1234", "Check the PR for PROJ-1234"):

1. Read the Jira ticket using MCP tools
2. Find the "Dev Handoff" comment - extract the PR link, what was built, spec deviations, and known limitations
3. Find the spec link - understand what was supposed to be built so you can review against requirements, not just code quality
4. Open the PR from the Dev Handoff and use it as the review target

This gives the reviewer full context: what was specified (PM), what was planned (Architect), and what was built (Dev). Review against all three, not just the diff.

If Jira MCP is not available, ask: "I can't read PROJ-1234 directly. Can you share the PR link and spec?"

---

Entry Condition

This skill activates when:

1. A dev-implement task completes and needs review (automatic in subagent flow)
2. Human PR review feedback is received
3. User explicitly requests a code review
4. User references a Jira ticket for review

---

Mode A: Requesting Review

When you need a review of completed work, dispatch a Code Reviewer subagent.

Step 1 -- Get Git SHAs

BASE_SHA=$(git merge-base HEAD main)
HEAD_SHA=$(git rev-parse HEAD)

Use the actual base branch if it is not main. The diff between these two SHAs is what gets reviewed.

Step 2 -- Dispatch Three Lens Subagents in Parallel

Dispatch all 3 lens subagents simultaneously (model: sonnet). Each lens sees the same diff but reports ONLY within its domain - no lens crosses into another's territory.

Lens	Agent template	Domain
Security	agents/security-lens-agent.md	Auth paths, secrets, input validation, injection, token storage, permissions
Engineering	agents/engineering-lens-agent.md	SOLID violations, naming, coupling, YAGNI, god classes, error hiding
Test	agents/test-lens-agent.md	Coverage gaps, missing failure paths, mocking anti-patterns, test naming

Provide each lens with the same inputs:

1. The diff -- git diff $BASE_SHA $HEAD_SHA
2. The spec/requirements -- inline the full task description or ticket. Do not pass a file reference.
3. The round number -- Round 1 (fresh review) or Round 2+ (re-review - see Review Etiquette below)

Do not wait for one lens before dispatching the next. All three run concurrently.

Step 3 -- Consolidation

After all 3 lenses complete, capture their full outputs and dispatch the consolidation agent (agents/code-reviewer-agent.md).

Provide the consolidation agent with:

1. Security lens findings (full output)
2. Engineering lens findings (full output)
3. Test lens findings (full output)
4. The original diff and spec

The consolidation agent deduplicates overlapping findings (the same issue may appear in two lenses from different angles), ranks by severity, and produces a single structured report with three tiers:

• Blocking -- must fix before merge (security holes, broken functionality, missing required tests)
• Advisory -- worth addressing before shipping (code smell, missing edge case tests, tight coupling)
• Observation -- noted for awareness, author decides (naming preferences, style, minor refactor opportunities)

Step 4 -- Act on Consolidated Findings

Categorize each consolidated finding and act:

Severity	Action
Blocking	Fix immediately. Do not proceed until resolved.
Advisory	Fix before moving to the next task.
Observation	Note for later. Author decides. Do not block progress.
Wrong	Push back with reasoning. See "Push Back When Justified" below.

---

Review Etiquette -- Rounds 2+

On the first review of a PR, all lenses report every finding within their domain.

On round 2 or later, each lens checks whether the finding existed in the previous review:

• Code unchanged in that area: do not surface the finding again. The author has seen it and made a decision.
• Code changed and issue got worse: surface it again, clearly marking it as a regression.
• New since the previous review: surface it as a new finding.

The author's choice to defer or deprioritize a finding stands. Lenses do not relitigate prior decisions. They catch new problems, not recycle old opinions.

How to implement round 2+: Before dispatching lenses, ask the user: "Can you share the previous review's findings? I'll configure each lens to surface only new or worsened issues." If prior findings are not available, run as a fresh round 1 review.

---

Mode B: Receiving Review (Human or External Reviewer)

When feedback arrives from a human reviewer or external tool.

Source Trust Levels

Not all review feedback is equal:

• Your user (human partner): Trusted. Implement their feedback after understanding it. They know the codebase context.
• Team reviewers (PR comments): Mostly trusted. Verify against codebase before implementing - they may not have full context.
• External reviewers (automated tools, AI reviewers): Verify everything. They lack project context. Check if suggestions are appropriate for THIS codebase.

Step 1 -- Read Everything First

Read ALL feedback before acting on any of it. Do not start fixing the first comment while there are unread comments below it. Context from later comments may change how you handle earlier ones.

Step 2 -- Evaluate Each Item

For each piece of feedback:

1. Understand -- What specifically is the reviewer asking for? If unclear, mark it for clarification.
2. Verify -- Check the codebase. Is the reviewer correct about the current state? Reviewers sometimes comment on stale diffs or misread context.
3. Evaluate -- Is this change technically correct for THIS codebase? Not in theory. Not in general. Here, now, with these constraints.

Verification Checklist for External Feedback

Before implementing any suggestion from a team reviewer or external tool, run through this checklist:

1. Technically correct for THIS codebase? -- Does the suggestion work with our patterns, dependencies, and constraints?
2. Breaks existing functionality? -- Will applying this change break callers, consumers, or integrations?
3. Reason for current implementation? -- Is there a reason the code was written this way? Check git blame, PR descriptions, and comments.
4. Works across all contexts? -- Does it work in all environments, platforms, and versions we support?
5. Reviewer has full context? -- Does the reviewer understand the full picture, or are they commenting on an isolated snippet?

If you cannot verify a suggestion: say so. "I cannot verify this without [specific thing]. Should I [investigate further / ask the reviewer / proceed with caution]?" Admitting uncertainty is better than implementing blindly.

Step 3 -- Clarify Before Implementing

If ANY item is unclear: STOP.

Ask for clarification on ALL unclear items at once. Do not implement some items while waiting for clarification on others. Partial implementation based on partial understanding creates more review cycles.

Step 4 -- Implement in Order

Once all items are understood:

1. Blocking issues first
2. Simple fixes next (typos, naming, formatting)
3. Complex changes last

Step 5 -- Test Each Fix

Test each fix individually. Do not batch fixes and hope they all work.

Step 6 -- Verify No Regressions

Run the full test suite after all fixes are applied. New fixes must not break existing behavior.

---

Technical Evaluation, Not Emotional Performance

Review is a technical process. Treat it like one.

Never Do This

• "You're absolutely right!" -- Just fix it.
• "Great point!" or "Excellent feedback!" -- Performative. Skip it.
• "Thanks for catching that!" -- Gratitude is not a code review action.
• Agree before understanding -- Check if the suggestion is technically correct for THIS codebase first.

Acknowledging Feedback

Good:

• "Fixed. [Brief description of what changed]."
• "Good catch -- [specific issue]. Fixed in [location]."
• Fix it silently. The commit speaks for itself.

Bad:

• "You're absolutely right!"
• "Great point!"
• "Thanks for catching that!"
• Any gratitude expression dressed up as a review response.

Correcting Your Own Pushback

When you pushed back and the reviewer was right:

Good: "You were right -- I checked [X] and it does [Y]. Implementing now."

Bad: Long apology. Defending why you pushed back. Over-explaining your reasoning for being wrong.

---

When Feedback Conflicts

If review feedback contradicts:

• Prior user decisions: STOP. Show the user both the feedback and the prior decision. Let them decide.
• Other review comments: Group the conflicting items. Present them together. Ask which direction to take.
• Existing codebase patterns: Note the conflict. Ask: "The reviewer suggests X, but the codebase consistently uses Y. Which should we follow?"

Never silently resolve conflicts by picking a side.

---

Push Back When Justified

Not every review comment is correct. Push back when:

1. Breaks existing functionality -- "This change would break [specific thing] because [reason]. The current approach handles [edge case] that the suggested approach does not."
2. Reviewer lacks context -- "This looks wrong in isolation, but [module X] depends on this behavior. See [file:line]."
3. Violates YAGNI -- "This abstraction is not needed yet. Only one caller exists. Extracting it adds indirection without value."
4. Technically incorrect -- "This would actually cause [specific problem] because [technical reason]."

Be specific. Cite code. Do not push back with vibes.

When pushing back feels uncomfortable: If pushing back publicly in a PR thread would create friction or political problems, flag the concern privately to your user instead. Let the user decide how to handle it. Silence is not politeness -- it is a bug that ships.

---

YAGNI Check

When a reviewer suggests "implementing properly" or "adding an abstraction" or "future-proofing":

grep -r "FunctionOrClassName" --include="*.{ts,js,py,cs,go}" .

• If unused or single caller: "This is only called from [one place]. Adding an abstraction here adds indirection without value. Remove it? (YAGNI)"
• If multiple callers: The reviewer has a point. Implement properly.

Do not build for hypothetical future callers. Build for the callers that exist.

Both you and the reviewer serve the user's goals. If a feature is not needed, the user decides whether to add it -- not the reviewer. Escalate YAGNI disagreements to the user rather than debating in the PR thread.

---

GitHub Thread Handling

When replying to inline PR comments, reply in the thread:

gh api repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies \
  -f body="Fixed. [description]"

Do NOT post a top-level comment when responding to an inline review comment. The reviewer left feedback on a specific line. Reply on that line.

---

Gut Check -- Are You Doing Review Right?

Pause and check yourself:

• Saying "You're absolutely right!" -- Stop. Just fix it.
• Implementing without understanding -- Stop. Ask what they mean.
• Agreeing with everything -- Stop. Check if it is technically correct for THIS codebase.
• Afraid to push back -- If it breaks something, say so. Silence is not politeness. It is a bug.
• Partially implementing -- Stop. Clarify ALL unclear items first, then implement.
• Writing a paragraph where a sentence would do -- Stop. "Fixed." is a complete response.

---

Subagent Prompt Templates

The review uses four agent templates:

agents/security-lens-agent.md    -- Auth, secrets, input validation, injection, token storage
agents/engineering-lens-agent.md -- SOLID, naming, coupling, YAGNI, god classes, error hiding
agents/test-lens-agent.md        -- Coverage gaps, failure paths, mocking anti-patterns
agents/code-reviewer-agent.md    -- Consolidation: deduplicates and ranks findings from all 3 lenses

Dispatch the 3 lens agents simultaneously. Feed their combined output to the consolidation agent after all 3 complete.

---

Subagent Context Preservation

When all subagents complete, explicitly capture their findings back to the main conversation before acting:

• Security lens: auth findings, secrets, injection vectors
• Engineering lens: SOLID violations, naming, coupling issues
• Test lens: coverage gaps, missing failure paths, anti-patterns
• Consolidation output: ranked findings (Blocking / Advisory / Observation)

Do not assume the orchestrating agent retains subagent context automatically. Extract the full review before acting on it.

---

Recommended Model

Primary: sonnet Why: Code review requires understanding patterns, spotting bugs, and evaluating design decisions. Sonnet provides sufficient depth without the cost of opus.

This is a recommendation. Ask the user: "Confirm model selection or override?" Do not proceed until the user responds.

---

Exit Condition

Review is complete when:

• All Blocking and Advisory items are resolved
• All fixes are tested individually
• Full test suite passes with no regressions
• All GitHub threads are replied to (if applicable)

After exit, resume the calling workflow.