|
| 1 | +--- |
| 2 | +description: Perform a staff-level code review of implementation changes, focused on correctness, security, performance, and spec compliance. |
| 3 | +scripts: |
| 4 | + sh: scripts/bash/check-prerequisites.sh --json --require-tasks --include-tasks |
| 5 | + ps: scripts/powershell/check-prerequisites.ps1 -Json -RequireTasks -IncludeTasks |
| 6 | +--- |
| 7 | + |
| 8 | +## User Input |
| 9 | + |
| 10 | +```text |
| 11 | +$ARGUMENTS |
| 12 | +``` |
| 13 | + |
| 14 | +You **MUST** consider the user input before proceeding (if not empty). |
| 15 | + |
| 16 | +## Pre-Execution Checks |
| 17 | + |
| 18 | +**Check for extension hooks (before review)**: |
| 19 | +- Check if `.specify/extensions.yml` exists in the project root. |
| 20 | +- If it exists, read it and look for entries under the `hooks.before_review` key |
| 21 | +- If the YAML cannot be parsed or is invalid, skip hook checking silently and continue normally |
| 22 | +- Filter out hooks where `enabled` is explicitly `false`. Treat hooks without an `enabled` field as enabled by default. |
| 23 | +- For each remaining hook, do **not** attempt to interpret or evaluate hook `condition` expressions: |
| 24 | + - If the hook has no `condition` field, or it is null/empty, treat the hook as executable |
| 25 | + - If the hook defines a non-empty `condition`, skip the hook and leave condition evaluation to the HookExecutor implementation |
| 26 | +- For each executable hook, output the following based on its `optional` flag: |
| 27 | + - **Optional hook** (`optional: true`): |
| 28 | + ``` |
| 29 | + ## Extension Hooks |
| 30 | +
|
| 31 | + **Optional Pre-Hook**: {extension} |
| 32 | + Command: `/{command}` |
| 33 | + Description: {description} |
| 34 | +
|
| 35 | + Prompt: {prompt} |
| 36 | + To execute: `/{command}` |
| 37 | + ``` |
| 38 | + - **Mandatory hook** (`optional: false`): |
| 39 | + ``` |
| 40 | + ## Extension Hooks |
| 41 | +
|
| 42 | + **Automatic Pre-Hook**: {extension} |
| 43 | + Executing: `/{command}` |
| 44 | + EXECUTE_COMMAND: {command} |
| 45 | +
|
| 46 | + Wait for the result of the hook command before proceeding to the Outline. |
| 47 | + ``` |
| 48 | +- If no hooks are registered or `.specify/extensions.yml` does not exist, skip silently |
| 49 | +
|
| 50 | +## Goal |
| 51 | +
|
| 52 | +Conduct a thorough, staff-engineer-level code review of all changes made during the implementation phase. This review acts as a quality gate before shipping, catching bugs, security issues, performance regressions, and deviations from the specification. The review is **read-only** — it produces a structured report but does NOT modify code. |
| 53 | +
|
| 54 | +## Operating Constraints |
| 55 | +
|
| 56 | +**STRICTLY READ-ONLY**: Do **not** modify any source files. Output a structured review report to the reviews directory. If critical issues are found, recommend specific fixes but do NOT apply them. |
| 57 | +
|
| 58 | +**Constitution Authority**: The project constitution (`/memory/constitution.md`) defines non-negotiable quality standards. Any violation is automatically a 🔴 Blocker. |
| 59 | +
|
| 60 | +## Outline |
| 61 | +
|
| 62 | +1. Run `{SCRIPT}` from repo root and parse FEATURE_DIR and AVAILABLE_DOCS list. All paths must be absolute. For single quotes in args like "I'm Groot", use escape syntax: e.g 'I'\''m Groot' (or double-quote if possible: "I'm Groot"). |
| 63 | +
|
| 64 | +2. **Gather Review Context**: |
| 65 | + - **REQUIRED**: Read `spec.md` for functional requirements and acceptance criteria |
| 66 | + - **REQUIRED**: Read `plan.md` for architecture decisions and technical constraints |
| 67 | + - **REQUIRED**: Read `tasks.md` for the full task list and implementation scope |
| 68 | + - **IF EXISTS**: Read `/memory/constitution.md` for quality standards and principles |
| 69 | + - **IF EXISTS**: Read any existing review reports in FEATURE_DIR/reviews/ for context |
| 70 | +
|
| 71 | +3. **Identify Changes to Review**: |
| 72 | + - Run `git diff main --stat` (or appropriate base branch) to identify all changed files |
| 73 | + - If no git diff is available, use the file paths referenced in `tasks.md` as the review scope |
| 74 | + - Group changes by category: source code, tests, configuration, documentation |
| 75 | + - Prioritize review order: security-sensitive files → core logic → API surfaces → tests → config → docs |
| 76 | +
|
| 77 | +4. **Review Pass 1 — Correctness & Logic**: |
| 78 | + - Verify each implementation matches its corresponding requirement in `spec.md` |
| 79 | + - Check for off-by-one errors, null/undefined handling, boundary conditions |
| 80 | + - Validate error handling: are errors caught, logged, and surfaced appropriately? |
| 81 | + - Check for race conditions in concurrent/async code |
| 82 | + - Verify data validation at trust boundaries (user input, API responses, file reads) |
| 83 | + - Ensure state management is consistent (no orphaned state, proper cleanup) |
| 84 | +
|
| 85 | +5. **Review Pass 2 — Security**: |
| 86 | + - Check for injection vulnerabilities (SQL, XSS, command injection, path traversal) |
| 87 | + - Verify authentication and authorization checks on all protected endpoints |
| 88 | + - Look for hardcoded secrets, credentials, or API keys |
| 89 | + - Validate input sanitization and output encoding |
| 90 | + - Check for insecure dependencies or known vulnerability patterns |
| 91 | + - Verify CORS, CSP, and other security header configurations |
| 92 | + - Check for sensitive data exposure in logs, error messages, or responses |
| 93 | +
|
| 94 | +6. **Review Pass 3 — Performance & Scalability**: |
| 95 | + - Identify N+1 query patterns or unnecessary database calls |
| 96 | + - Check for unbounded loops, missing pagination, or large in-memory collections |
| 97 | + - Look for blocking operations in async contexts |
| 98 | + - Verify caching strategies are appropriate and cache invalidation is correct |
| 99 | + - Check for resource leaks (file handles, connections, event listeners) |
| 100 | + - Validate that performance-critical paths noted in `plan.md` are optimized |
| 101 | +
|
| 102 | +7. **Review Pass 4 — Spec Compliance & Architecture**: |
| 103 | + - Cross-reference each functional requirement (FR-###) against implementation |
| 104 | + - Verify architecture decisions from `plan.md` are followed (patterns, layers, boundaries) |
| 105 | + - Check API contracts match specification (request/response shapes, status codes) |
| 106 | + - Validate that acceptance criteria from user stories are testable and tested |
| 107 | + - Flag any implemented functionality NOT in the spec (scope creep) |
| 108 | + - Flag any spec requirements NOT implemented (missing coverage) |
| 109 | +
|
| 110 | +8. **Review Pass 5 — Test Quality**: |
| 111 | + - Verify test coverage for critical paths and edge cases |
| 112 | + - Check test assertions are meaningful (not just "doesn't throw") |
| 113 | + - Validate test isolation (no shared mutable state between tests) |
| 114 | + - Verify mock/stub usage is appropriate and doesn't hide real bugs |
| 115 | + - Check that acceptance criteria have corresponding test cases |
| 116 | + - Flag untested error paths and boundary conditions |
| 117 | +
|
| 118 | +9. **Severity Classification**: |
| 119 | + Apply the following severity levels to each finding: |
| 120 | +
|
| 121 | + - 🔴 **Blocker**: Security vulnerability, data corruption risk, crashes, constitution violation, missing core functionality. **Must fix before shipping.** |
| 122 | + - 🟡 **Warning**: Performance issue, incomplete error handling, missing edge case coverage, test gap, architectural deviation. **Should fix before shipping.** |
| 123 | + - 🟢 **Suggestion**: Code clarity improvement, refactoring opportunity, documentation gap, minor style inconsistency. **Nice to fix but non-blocking.** |
| 124 | +
|
| 125 | +10. **Generate Review Report**: |
| 126 | + Create the review report at `FEATURE_DIR/reviews/review-{timestamp}.md` using the review report template. The report must include: |
| 127 | +
|
| 128 | + - **Executive Summary**: Overall assessment (APPROVED / APPROVED WITH CONDITIONS / CHANGES REQUIRED) |
| 129 | + - **Findings Table**: All findings with ID, severity, file, line(s), description, recommendation |
| 130 | + - **Spec Coverage Matrix**: Requirements vs implementation status |
| 131 | + - **Test Coverage Assessment**: Coverage gaps relative to acceptance criteria |
| 132 | + - **Metrics**: Total findings by severity, files reviewed, spec coverage percentage |
| 133 | + - **Recommended Actions**: Prioritized list of fixes, grouped by severity |
| 134 | +
|
| 135 | +11. **Provide Verdict**: |
| 136 | + Based on findings, provide one of: |
| 137 | + - ✅ **APPROVED**: No blockers, minimal warnings. Safe to proceed to `/speckit.ship` |
| 138 | + - ⚠️ **APPROVED WITH CONDITIONS**: No blockers, but warnings should be addressed. List conditions. |
| 139 | + - ❌ **CHANGES REQUIRED**: Blockers found. Must fix and re-review before shipping. List blocking items. |
| 140 | +
|
| 141 | +## Post-Review Actions |
| 142 | +
|
| 143 | +Suggest next steps based on verdict: |
| 144 | +- If APPROVED: "Run `/speckit.ship` to prepare the release" |
| 145 | +- If APPROVED WITH CONDITIONS: "Address warnings, then run `/speckit.ship`" |
| 146 | +- If CHANGES REQUIRED: "Fix blocker issues, then run `/speckit.review` again" |
| 147 | +
|
| 148 | +**Check for extension hooks (after review)**: |
| 149 | +- Check if `.specify/extensions.yml` exists in the project root. |
| 150 | +- If it exists, read it and look for entries under the `hooks.after_review` key |
| 151 | +- If the YAML cannot be parsed or is invalid, skip hook checking silently and continue normally |
| 152 | +- Filter out hooks where `enabled` is explicitly `false`. Treat hooks without an `enabled` field as enabled by default. |
| 153 | +- For each remaining hook, do **not** attempt to interpret or evaluate hook `condition` expressions: |
| 154 | + - If the hook has no `condition` field, or it is null/empty, treat the hook as executable |
| 155 | + - If the hook defines a non-empty `condition`, skip the hook and leave condition evaluation to the HookExecutor implementation |
| 156 | +- For each executable hook, output the following based on its `optional` flag: |
| 157 | + - **Optional hook** (`optional: true`): |
| 158 | + ``` |
| 159 | + ## Extension Hooks |
| 160 | +
|
| 161 | + **Optional Hook**: {extension} |
| 162 | + Command: `/{command}` |
| 163 | + Description: {description} |
| 164 | +
|
| 165 | + Prompt: {prompt} |
| 166 | + To execute: `/{command}` |
| 167 | + ``` |
| 168 | + - **Mandatory hook** (`optional: false`): |
| 169 | + ``` |
| 170 | + ## Extension Hooks |
| 171 | +
|
| 172 | + **Automatic Hook**: {extension} |
| 173 | + Executing: `/{command}` |
| 174 | + EXECUTE_COMMAND: {command} |
| 175 | + ``` |
| 176 | +- If no hooks are registered or `.specify/extensions.yml` does not exist, skip silently |
0 commit comments