review-code

name	review-code
description	Code Review Skill: Deep code review that traces data flow, checks callers, and catches regressions. MANDATORY TRIGGERS: review, code review, peer review, review PR, check code, approve, review changes.
argument-hint	<ticket ID to review>

Bobby Review Skill

Deep code review. Reads the full context around every change to catch regressions, broken callers, misused APIs, and LLM-typical mistakes. Separate agent from build for fresh perspective.

Pre-flight Checks

1. Check learnings — Read .claude/skills/bobby-review/learnings.md + .claude/skills/bobby-review/learnings.local.md and .claude/skills/bobby-shared/learnings.md + .claude/skills/bobby-shared/learnings.local.md in parallel
2. Branch check — git branch --show-current must NOT be main/master
3. Hygiene check — git status --short should show no uncommitted source files (build agent left dirty state if it does)
4. Load context — Read these in parallel:
   • The ticket's ticket.md — acceptance criteria
   • The ticket's plan.md — intended approach
   • The ticket's test-cases.md — expected test coverage
5. Understand what was built — git log --oneline main..HEAD to see all branch commits

Investigation First

<investigation_rules> Before forming any opinion about the code:

• Read ticket.md and plan.md in parallel — understand what was supposed to be built and the intended approach
• Read every file referenced in plan.md that was supposed to change
• If the diff touches files not in the plan, read those files to understand why
• Do not approve or reject based on the diff alone — verify against the actual codebase </investigation_rules>

Deep Code Review

This is the core of your job. Don't skim diffs — investigate the code like you're debugging a production incident before it happens.

1. Trace the change boundaries

• Run git diff main...HEAD --stat — get an overview of every file touched
• For each changed file, read the full file (not just the diff hunk) to understand context
• Cross-reference against plan.md:
  • Files in the plan that were NOT changed — were they supposed to be?
  • Changed files NOT in the plan — why were they touched? Is this scope creep?

2. Check for breakage in callers and dependents

For every function, component, or API endpoint that was modified or renamed:

• Grep the codebase for its callers/consumers
• Verify callers still work with the new signature, return type, or behavior
• Check if any imports were added — do the imported modules actually export what's being used?
• Check if any exports were removed or renamed — are there consumers that still reference the old name?

3. Look for LLM-typical mistakes

<llm_mistakes> These are patterns that LLM-generated code frequently gets wrong:

• Hallucinated APIs: Method calls on libraries/frameworks that don't exist (e.g., router.pushState() instead of router.push(), db.findOneBy() instead of db.findOne()). When you see an unfamiliar API call, verify it exists.
• Stale references: Code referencing variables, functions, or files that were renamed or removed elsewhere in the codebase.
• Copy-paste drift: Similar code added in multiple places that is subtly inconsistent (different arg order, missing a condition, wrong variable name).
• Incomplete migrations: A pattern was changed (renamed function, new API) but only in the new code — old call sites still use the previous version.
• Wrong assumptions about existing code: The builder may have misread how an existing function works and used it incorrectly. Verify the usage matches the actual implementation. </llm_mistakes>

4. Performance and resource concerns

<performance_review> Look for these specific patterns in the diff:

• N+1 queries: Database/API calls inside loops — each iteration triggers a separate query
• Unbounded fetches: Queries without LIMIT, pagination, or reasonable caps
• Missing cleanup: Event listeners, intervals, subscriptions, or timers that are added but never removed (memory leaks)
• Synchronous blocking: Heavy operations (file I/O, network calls, large data processing) that should be async
• Redundant work: Computing the same value multiple times when it could be computed once
• Large objects in memory: Holding entire datasets when only a subset is needed </performance_review>

5. Verify test quality

Don't just check that tests exist — check that they would actually catch bugs:

• Do assertions check the right thing? A test that asserts result !== null passes even if the result is completely wrong.
• Are there tests that would pass even if the feature was completely broken? (e.g., only testing that a function doesn't throw)
• Do tests cover the sad path, not just the happy path? (invalid input, empty data, error conditions)
• Read test-cases.md and confirm each documented test case has a corresponding test in the diff
• Are test descriptions accurate? A test named "should filter by status" that doesn't actually test filtering is misleading.

Decisions Check

Run bobby decision list to see the active decisions (it hides invalidated ones). Scan the changed code for violations. For each violation found, flag it by decision id in review.md under a "Decision Violations" section. A decision violation is a rejection reason.

If the change establishes a new constraint the codebase should be held to, record it with bobby decision add --id … --fact … --why … --ticket {ID} rather than editing .bobby/decisions.yaml.

Examples of what to look for based on the decision fact field:

• "Never call the database directly from UI components" → grep changed files for DB calls in component code
• "All API calls must go through the service layer" → check for direct fetch/axios calls in components
• Any project-specific constraint in decisions.yaml

AC Verification

After the deep review, verify the minimum bar:

1. Re-read each acceptance criterion from ticket.md
2. For each AC, identify the specific code that satisfies it
3. If any AC lacks coverage, that's a rejection

Run Tests and Lint

Run these independently to verify the build agent's work in a fresh context:

1. Tests: npm test
2. Lint: npm run lint

Show the output as evidence. If tests can't run (missing deps, broken environment), this is a BLOCK, not an approval.

Review Artifact

Write review.md in the ticket directory with this structure:

## Review — TKT-{ID}

### Verdict: Approved / Approved with Notes / Rejected / Blocked

### Files Reviewed
- `path/to/file.js` — {what changed, what you verified}

### Code Concerns
- {any issues found — broken callers, performance issues, LLM mistakes, etc.}
- {or "None found" if clean}

### Decision Violations
- {any violations of .bobby/decisions.yaml entries}
- {or "None" if decisions.yaml doesn't exist or no violations found}

### AC Verification
- [x] AC 1: {how it's satisfied}
- [x] AC 2: {how it's satisfied}

### Test/Lint Output
- Tests: {pass/fail, count}
- Lint: {pass/fail, summary}

### Notes
- {anything the next agent or human should know}

Decision

Approve

All checks pass, no concerns:

1. Write review.md in the ticket directory
2. Commit any changes you made (lint fixes, debug cleanup): TKT-{ID}: review fixes
3. Add comment: bobby ticket comment {ID} --by bobby-review "Approved: {summary}"
4. If you discovered a pattern future reviews should catch: bobby learn bobby-review "pattern" "description"
5. Move to testing: bobby ticket move {ID} test

Approve with Notes

Code works and ACs are met, but you have observations worth recording — minor style drift, a pattern that could become a problem, a suggestion for future work. These do NOT block the ticket:

1. Write review.md with notes in the "Code Concerns" and "Notes" sections
2. Commit any changes you made: TKT-{ID}: review fixes
3. Add comment: bobby ticket comment {ID} --by bobby-review "Approved with notes: {summary}. Notes: {observations}"
4. Move to testing: bobby ticket move {ID} test

Reject

Broken code, missing ACs, or serious quality issues:

1. Write review.md documenting every issue found
2. Move back with specific feedback: bobby ticket move {ID} reject "Specific issue description"
3. Be specific — the build agent needs to know exactly what to fix

`bobby ticket move TKT-042 reject "AC #3 not met: 'Filter persists after page refresh' — tested by refreshing /leads?status=active and the filter dropdown resets to 'All'. The filterByStatus function reads URL params but doesn't call setFilter on mount. Also: getLeadsByStatus() in leads-service.js is called by DashboardWidget (line 47) which expects an array return type, but the new implementation returns an object. Fix both."` `bobby ticket move TKT-042 reject "Filtering doesn't work properly"` Why this fails: The build agent won't know what specifically to fix or which AC failed.

Block

Can't complete review — tests won't run, environment broken, missing context:

1. Write review.md documenting what you were able to check and what blocked you
2. bobby ticket move {ID} block "reason"

If Something Goes Wrong

• If you discover a systemic pattern: bobby learn bobby-review "pattern" "description"

---

Project overrides

If .claude/skills/bobby-review/SKILL.local.md exists, read it and follow it. It holds this project's own instructions for this skill and wins wherever it conflicts with anything above.

SKILL.md is shipped by Bobby and is replaced on every upgrade — edits here are lost. SKILL.local.md is yours and is never overwritten.