--- name: fp-review-pr description: Review a pull request through the lens of fastpace domain context (architecture, decisions, patterns, conventions, learnings) and guardrails — not generic "is this code good" feedback. Categorizes findings as blocking / suggestion / praise, with citations. Triggers on /fp-review-pr . --- # fp-review-pr Review a PR as the team's most senior engineer — one who's read every decision, every ADR, and every post-incident learning this repo has. Every finding is citable; nothing is generic. ## Preflight 1. Resolve the PR diff. Preference order: - `gh pr diff ` (if `gh` is installed and the user is authed) - `git diff ...` (if running inside the PR's branch checkout) - Ask the user for a URL or diff paste as a last resort. 2. If the diff is > 2000 lines, ask the user: "This is a large diff. Review all of it, or focus on a subsystem?" 3. Identify the linked PRD/ERD if referenced in the PR description (`prd: `, `erd: `, or a link to `fastpace/docs/…`). Load it. ## Context to load (always) - `fastpace/context/architecture.md` — module boundaries, entry points. - `fastpace/context/conventions.md` — naming, commits, test placement. - `fastpace/context/decisions.md` — prior decisions this PR could violate. - `fastpace/context/patterns.md` — existing idioms to reuse or extend. - `fastpace/context/learnings.md` — past mistakes. **Filter to `status: reviewed` entries** (see "Learnings filter" below). If this PR is about to repeat one, escalate. - `fastpace/docs/adr/*.md` — formal decisions. - `fastpace/fastpace.config.yaml` — guardrails to enforce. ## Finding severities Every finding gets exactly one label: | Severity | When | Effect | |---|---|---| | `BLOCK` | Violates a decision, ADR, guardrail, or introduces a known-bad pattern from `learnings.md`. | Must be fixed before merge. | | `SUGGEST` | Better pattern exists, naming is off, tests could be stronger, docs drift. Not a violation. | Optional for author. | | `PRAISE` | Genuinely well-done. Reinforces good patterns; future reviews notice. | Morale + precedent. | | `QUESTION` | You need info to decide. Use sparingly; prefer reading the code. | Author answers. | Aim for: **≤ 5 BLOCK** (focus), **≤ 10 SUGGEST** (signal-to-noise), **≥ 1 PRAISE** per review (real praise, not manufactured). ## Automatic-block triggers (always BLOCK) Never let these through, regardless of author seniority: - Tests required + no test files changed → `BLOCK` against `guardrails.require_tests`. - Security scan required + ERD has empty security section → `BLOCK` against `guardrails.require_security_scan`. - PR targets a protected branch directly from a non-feature branch → `BLOCK` against `guardrails.protected_branches`. - New npm dep outside `approved_scopes` → `BLOCK` against `hooks.dependency-alert.approved_scopes`. - Adds secrets / API keys / private keys → `BLOCK` (secret-scanner pattern). - Adds a TODO that blocks a stated acceptance criterion → `BLOCK`. - Directly contradicts a decision in `decisions.md` or an ADR without also updating that decision → `BLOCK`. ## Finding format Every finding has this shape: ``` [SEV] where: <path>:<line> (or "PR metadata" for cross-cutting) why: <1-2 sentences referencing a specific context file or ADR> fix: <what to do; code suggestion if < 5 lines, otherwise a pointer> cite: <fastpace/context/decisions.md § 2025-02-14> (or N/A) ``` Example: ``` [BLOCK] ORM calls in handler layer where: src/handlers/checkout.ts:62 why: Decision 2025-02-14 moved all query code out of handlers into src/repo/*; handlers must call the repo, not prisma directly. fix: Replace `prisma.user.findUnique` with `userRepo.findById(id)`. Repo already has this method — see src/repo/user.ts:24. cite: fastpace/context/decisions.md § 2025-02-14 ``` ## Output structure Produce the review in this order: 1. **Summary** (3-5 lines): - "PR #<n>: <title>" - "Files changed: N · Lines +N/-N" - "BLOCK: N · SUGGEST: N · PRAISE: N · QUESTION: N" - One sentence on whether it's mergeable as-is. 2. **Blocking issues** (all `BLOCK` findings, most severe first). 3. **Suggestions** (`SUGGEST`). 4. **Praise** (`PRAISE`). 5. **Questions** (`QUESTION`) if any. 6. **Guardrail checklist** — quick table: - require_tests: ✓/✗ - require_review: ✓/✗ (your review counts) - require_security_scan: ✓/✗ - branch_before_changes: ✓/✗ - pr_description present: ✓/✗ 7. **Acceptance-criteria check** — if an ERD is linked, a table of each criterion with ✓ (satisfied by this diff), ✗ (violated), `?` (cannot verify from diff). ## What NOT to flag - Style issues the linter/formatter would catch (whitespace, semicolons, import order). - Personal preferences ("I'd name this differently") — unless `conventions.md` codifies it. - Missing tests for code that genuinely doesn't need them (type-only changes, pure renames caught by compiler). Explain the exemption. - "This could be faster" speculation without a benchmark or a prior learning to cite. - Nits on generated files or vendored code. - Repeating the same finding across 12 files — consolidate to one BLOCK with a list. ## Learnings filter `learnings.md` entries each carry an `<!-- fp-entry · status: ... -->` comment above their heading: - `status: reviewed` — a human approved this. **Cite in findings; use for the Repeated-mistake check below.** - `status: unreviewed` — auto-captured by `/fp-extract-learnings`, not yet approved. **Skip by default** — they'd produce blocking findings on evidence a human hasn't signed off on. - Entries with no comment — legacy; treat as `reviewed`. If the user explicitly runs `/fp-review-pr --include-unreviewed <n>`, include them but tag the finding as `[from unreviewed learning]` so the author knows the basis is soft. ## Repeated mistakes — escalate If the PR repeats a mistake captured in `learnings.md`: - Open the review with a dedicated section: `⚠ Repeated mistake`. - Cite the learning by date. - Make the finding `BLOCK` regardless of blast radius. - Suggest the author read the learning before rewriting. This is what fastpace is *for*. Don't soften it. ## When the PR is good Don't manufacture findings. A review of "3 PRAISE, 0 BLOCK, 1 SUGGEST" is a great review if the PR is good. Say so explicitly: "Clean PR — mergeable as-is once a reviewer signs off." ## Integration - After posting the review, if you caught something worth preserving for future reviews, suggest the author run `/fp-remember` or `/fp-write-adr`. - If the `learnings.md` check found nothing but your review surfaced a new class of mistake, suggest `/fp-remember` to capture it so the *next* PR gets flagged. - Read-only: **never** push commits, edit files in the PR, or close/merge anything. You analyze; humans decide. ## Exit criteria - Every finding carries a severity, a location, a "why" with citation, and a fix. - Summary header first. Nothing else before it. - Zero files modified in the repo. Zero git operations executed.