|
| 1 | +--- |
| 2 | +name: flow-code-code-review |
| 3 | +description: "Use when reviewing code changes — self-review in Worker Phase 6, impl-review, or PR review. Applies five-axis scoring with severity labels." |
| 4 | +tier: 2 |
| 5 | +user-invocable: true |
| 6 | +--- |
| 7 | +<!-- SKILL_TAGS: review,quality,correctness,security --> |
| 8 | + |
| 9 | +# Five-Axis Code Review |
| 10 | + |
| 11 | +> **Startup:** Follow [Startup Sequence](../_shared/preamble.md) before proceeding. |
| 12 | +
|
| 13 | +## flowctl Setup |
| 14 | + |
| 15 | +```bash |
| 16 | +FLOWCTL="$HOME/.flow/bin/flowctl" |
| 17 | +``` |
| 18 | + |
| 19 | +## Overview |
| 20 | + |
| 21 | +Structured code review across five orthogonal axes. Every finding gets a severity label. Every review ends with a clear verdict. This replaces ad-hoc "looks good" reviews with systematic quality gates. |
| 22 | + |
| 23 | +## When to Use |
| 24 | + |
| 25 | +- Worker Phase 6 (self-review before commit) |
| 26 | +- Implementation review (impl-review phase) |
| 27 | +- PR review before merge |
| 28 | +- Manual code review requests |
| 29 | + |
| 30 | +**When NOT to use:** |
| 31 | +- Plan review (that checks spec alignment, not code quality) |
| 32 | +- Architecture design review (use flow-code-api-design instead) |
| 33 | + |
| 34 | +## The Five Axes |
| 35 | + |
| 36 | +Review every diff across these five dimensions. Each axis is independent — a change can score well on correctness but poorly on security. |
| 37 | + |
| 38 | +### Axis 1: Correctness |
| 39 | + |
| 40 | +- Does the code do what the spec says? |
| 41 | +- Are edge cases handled? (null, empty, boundary values, overflow) |
| 42 | +- Are errors caught and handled appropriately? |
| 43 | +- Do tests cover the actual behavior, not just happy path? |
| 44 | +- Are race conditions possible under concurrent access? |
| 45 | + |
| 46 | +### Axis 2: Readability & Simplicity |
| 47 | + |
| 48 | +- Can a new team member understand this code without explanation? |
| 49 | +- Are names clear and specific? (`processData` → `validateUserInput`) |
| 50 | +- Is the logic straightforward or unnecessarily clever? |
| 51 | +- Are functions under 40 lines? Files under 300 lines? |
| 52 | +- Is there dead code, commented-out code, or TODOs that should be tickets? |
| 53 | + |
| 54 | +### Axis 3: Architecture |
| 55 | + |
| 56 | +- Does the change follow existing project patterns? |
| 57 | +- Are module boundaries respected? (no cross-layer imports) |
| 58 | +- Is the abstraction level appropriate? (not too deep, not too shallow) |
| 59 | +- Would this change make future modifications harder? |
| 60 | +- Are dependencies flowing in the right direction? |
| 61 | + |
| 62 | +### Axis 4: Security |
| 63 | + |
| 64 | +- Is all user input validated at the boundary? |
| 65 | +- Are queries parameterized? |
| 66 | +- Are auth and authorization checks present where needed? |
| 67 | +- Are secrets, tokens, or PII protected? |
| 68 | +- See `flow-code-security` skill for full checklist. |
| 69 | + |
| 70 | +### Axis 5: Performance |
| 71 | + |
| 72 | +- Are there N+1 query patterns? |
| 73 | +- Are there unbounded loops or data fetches? |
| 74 | +- Could this block the main thread or event loop? |
| 75 | +- Are expensive operations cached where appropriate? |
| 76 | +- Are there unnecessary re-renders (React) or recomputations? |
| 77 | + |
| 78 | +## Severity Labels |
| 79 | + |
| 80 | +Every finding MUST have a severity label: |
| 81 | + |
| 82 | +| Label | Meaning | Action Required | |
| 83 | +|-------|---------|-----------------| |
| 84 | +| **Critical** | Blocks merge. Bug, security hole, data loss risk. | Must fix before proceeding. | |
| 85 | +| **Important** | Should fix. Correctness concern or significant quality issue. | Fix unless strong justification. | |
| 86 | +| **Suggestion** | Consider this. Improvement opportunity. | Author decides. | |
| 87 | +| **Nit** | Minor style or naming preference. | Author may ignore. | |
| 88 | +| **FYI** | Informational. No action needed. | For awareness only. | |
| 89 | + |
| 90 | +## Change Sizing |
| 91 | + |
| 92 | +| Lines Changed | Rating | Action | |
| 93 | +|---------------|--------|--------| |
| 94 | +| ~100 | Good | Easy to review thoroughly | |
| 95 | +| ~300 | Acceptable | Review in one sitting | |
| 96 | +| ~500 | Large | Consider splitting | |
| 97 | +| ~1000+ | Too large | Split into smaller changes | |
| 98 | + |
| 99 | +If a diff exceeds 500 lines, flag it: "This change is large (~N lines). Consider splitting into focused commits." |
| 100 | + |
| 101 | +## Review Process |
| 102 | + |
| 103 | +### Step 1: Understand Context |
| 104 | + |
| 105 | +Before looking at code: |
| 106 | +- Read the task spec / PR description |
| 107 | +- Understand WHAT should have changed and WHY |
| 108 | +- Check which files are expected to change |
| 109 | + |
| 110 | +### Step 2: Review Tests First |
| 111 | + |
| 112 | +- Do tests exist for the change? |
| 113 | +- Do they test behavior, not implementation? |
| 114 | +- Are edge cases covered? |
| 115 | +- Would the tests catch a regression if someone reverted the key logic? |
| 116 | + |
| 117 | +### Step 3: Review Implementation |
| 118 | + |
| 119 | +Walk through the diff with all five axes active: |
| 120 | +- Read file-by-file in dependency order (utilities → services → handlers → tests) |
| 121 | +- For each file, check all five axes |
| 122 | +- Note findings with severity labels |
| 123 | + |
| 124 | +### Step 4: Categorize Findings |
| 125 | + |
| 126 | +Group findings by severity: |
| 127 | +``` |
| 128 | +## Critical (must fix) |
| 129 | +- [Correctness] Missing null check on user.email (line 42) — NPE in production |
| 130 | +- [Security] SQL built with template literal (line 89) — injection risk |
| 131 | +
|
| 132 | +## Important (should fix) |
| 133 | +- [Architecture] Direct DB access from handler — should go through service layer |
| 134 | +- [Performance] Fetching all users to count them — use COUNT query |
| 135 | +
|
| 136 | +## Suggestions |
| 137 | +- [Readability] Rename `processData` to `validateOrderInput` for clarity |
| 138 | +- [Readability] Extract lines 120-145 into a named function |
| 139 | +
|
| 140 | +## Nits |
| 141 | +- [Readability] Inconsistent spacing in object literal (line 67) |
| 142 | +``` |
| 143 | + |
| 144 | +### Step 5: Verdict |
| 145 | + |
| 146 | +| Verdict | Criteria | |
| 147 | +|---------|----------| |
| 148 | +| **SHIP** | Zero Critical, zero Important, all Suggestions are minor | |
| 149 | +| **NEEDS_WORK** | Has Critical or Important findings that need fixes | |
| 150 | +| **MAJOR_RETHINK** | Fundamental approach is wrong — need to redesign | |
| 151 | + |
| 152 | +## Multi-Model Review Pattern |
| 153 | + |
| 154 | +For highest quality, use cross-model review: |
| 155 | +1. Model A (Claude) writes the code |
| 156 | +2. Model B (Codex/different model) reviews independently |
| 157 | +3. Model A addresses findings |
| 158 | +4. Human approves final result |
| 159 | + |
| 160 | +This catches blind spots that same-model review misses. |
| 161 | + |
| 162 | +## Dead Code Hygiene |
| 163 | + |
| 164 | +During review, flag: |
| 165 | +- Commented-out code blocks (delete or create a ticket) |
| 166 | +- Unused imports, variables, functions |
| 167 | +- Feature flags for features that shipped months ago |
| 168 | +- `TODO` comments older than 30 days without tickets |
| 169 | + |
| 170 | +## Common Rationalizations |
| 171 | + |
| 172 | +| Rationalization | Reality | |
| 173 | +|---|---| |
| 174 | +| "It works, so it's fine" | Working code can still have security holes, performance issues, and maintenance debt. | |
| 175 | +| "We'll clean it up later" | Later never comes. Fix it now or create a tracked ticket. | |
| 176 | +| "It's just a nit" | Accumulated nits become a maintenance burden. Fix the pattern, not individual instances. | |
| 177 | +| "The tests pass" | Tests only catch what they test for. Review the untested paths. | |
| 178 | +| "I don't have context on this area" | Say so. Ask questions. Don't rubber-stamp code you don't understand. | |
| 179 | + |
| 180 | +## Red Flags |
| 181 | + |
| 182 | +- No tests added for new behavior |
| 183 | +- Error handling that swallows exceptions silently |
| 184 | +- Magic numbers or strings without constants |
| 185 | +- Functions with more than 4 parameters |
| 186 | +- Deeply nested conditionals (3+ levels) |
| 187 | +- Mixed concerns in a single commit (feature + refactor + config change) |
| 188 | +- TODOs without ticket references |
| 189 | +- Changes to generated files |
| 190 | + |
| 191 | +## Verification |
| 192 | + |
| 193 | +After completing a review: |
| 194 | + |
| 195 | +- [ ] All five axes evaluated (correctness, readability, architecture, security, performance) |
| 196 | +- [ ] Every finding has a severity label |
| 197 | +- [ ] Critical/Important findings have specific fix guidance |
| 198 | +- [ ] Clear SHIP / NEEDS_WORK / MAJOR_RETHINK verdict given |
| 199 | +- [ ] Change size noted if >500 lines |
| 200 | +- [ ] Tests reviewed before implementation |
| 201 | +- [ ] No rubber-stamping — every approval reflects genuine review |
0 commit comments