122 lines
8.2 KiB
Markdown
122 lines
8.2 KiB
Markdown
---
|
|
name: senior-code-review
|
|
description: Perform a thorough, read-only review of a pull request, branch, commit, working-tree diff, or named files. Use when asked to review code.
|
|
---
|
|
|
|
# Senior Code Review
|
|
|
|
Perform a high-recall review followed by a high-precision verification pass. Behave like a senior engineer who must understand the local system before judging the change.
|
|
|
|
## Operating contract
|
|
|
|
- Treat the review as read-only. Do not edit files, apply fixes, change git state, commit, or post comments unless the user explicitly asks in a separate instruction.
|
|
- Review the change, not the author's process. Do not excuse a defect because tests pass or because the implementation was difficult.
|
|
- Prefer concrete, falsifiable findings over generic advice. Do not report style preferences unless they violate repository conventions or materially harm comprehension.
|
|
- Keep distinct findings separate; combine repeated instances of one root cause.
|
|
- Investigate enough surrounding code to determine whether a suspected problem is real. The diff is an index, not the full review surface.
|
|
- Report no findings when none survive verification. Never invent issues to make a review look thorough.
|
|
|
|
Read [references/review-lenses.md](references/review-lenses.md) before reviewing. When independent subagents are available, also read [references/reviewer-briefs.md](references/reviewer-briefs.md).
|
|
|
|
## 1. Establish scope
|
|
|
|
Use the user's explicit target when supplied. Otherwise choose the first applicable scope:
|
|
|
|
1. Staged, unstaged, and relevant untracked changes when the working tree has changes.
|
|
2. The current branch against its merge base with the default or upstream branch.
|
|
3. The most recent commit when no other change exists.
|
|
|
|
Resolve the base and head before reviewing. Inspect the diff summary, changed paths, commits, and rename/binary status. Stop with a concise explanation if the target is invalid or the diff is empty.
|
|
|
|
Exclude generated, vendored, lock, and snapshot files from line-by-line review when appropriate, but inspect their semantic effects when dependency, schema, API, or snapshot changes are relevant.
|
|
|
|
## 2. Build codebase context
|
|
|
|
Before judging implementation details:
|
|
|
|
1. Read the applicable repository instructions, including root and path-scoped `AGENTS.md`, `CLAUDE.md`, `REVIEW.md`, `CONTRIBUTING.md`, and relevant engineering or architecture documents.
|
|
2. Determine the intended behavior from the user's request, PR or issue text when available, commit messages, specifications, and tests. State any important requirement that remains unavailable.
|
|
3. Read each changed source and test file as a whole when feasible, not only the edited hunks.
|
|
4. Trace affected entry points, callers, callees, types, data flows, configuration, and persistence boundaries far enough to understand observable behavior.
|
|
5. Search for sibling implementations, established helpers, test conventions, error-handling patterns, and architectural precedents.
|
|
6. Use focused git history or blame when a surprising invariant, compatibility shim, or architectural choice may be intentional.
|
|
|
|
Produce a short internal context map: change intent, affected behavior, relevant invariants, architectural boundaries, and verification mechanisms. Share it with review subagents; do not clutter the final response with it unless it explains a finding.
|
|
|
|
## 3. Inspect tests before implementation
|
|
|
|
Read changed and nearby tests first. Infer the promised behavior, then challenge the tests:
|
|
|
|
- Would a plausible broken implementation still pass?
|
|
- Do assertions observe externally meaningful behavior or merely repeat mocks, fixtures, implementation details, or values constructed by the test itself?
|
|
- Do mocks preserve the real contract, ordering, failure modes, and data shape?
|
|
- Does a regression test fail without the fix for the intended reason?
|
|
- Are negative, boundary, authorization, error, concurrency, and integration paths covered where risk warrants them?
|
|
- Can the test pass vacuously because code was not called, an async result was not awaited, an exception was swallowed, or the assertion is too broad?
|
|
|
|
Do not equate coverage with correctness. Treat missing tests as a finding only when you can name the meaningful behavior or regression that remains unprotected.
|
|
|
|
## 4. Run independent review passes
|
|
|
|
For a non-trivial change, use four independent review seats in parallel when the host supports subagents:
|
|
|
|
1. Correctness and unintended behavior.
|
|
2. Test validity and verification gaps.
|
|
3. Security and defensive coding.
|
|
4. Architecture, abstractions, and maintainability.
|
|
|
|
Give every seat the same scope, intent, context map, changed-file list, and read-only constraint. Keep their reasoning isolated. Use the exact contracts in `references/reviewer-briefs.md` and allow them to inspect the repository rather than pasting an enormous diff into each prompt.
|
|
|
|
If subagents are unavailable or the change is very small, perform the four passes sequentially yourself. Finish one lens before starting the next so concerns do not collapse into a shallow general scan.
|
|
|
|
Add focused passes for performance, concurrency, migrations, compatibility, observability, accessibility, or dependency risk when the changed surfaces make them relevant.
|
|
|
|
## 5. Verify candidate findings
|
|
|
|
Collect candidate findings from every pass, deduplicate by root cause, then challenge them with a fresh verification pass. Use an independent verifier subagent when available.
|
|
|
|
Keep a bug, security, or test finding only when all applicable statements are true:
|
|
|
|
- The finding identifies a precise changed or directly affected location.
|
|
- The reviewer read the surrounding implementation, callers, guards, types, and relevant tests.
|
|
- A concrete input, state, or execution path leads to a specific bad outcome.
|
|
- Existing validation, framework behavior, or another layer does not already prevent it.
|
|
- The issue is introduced by the change or is directly relevant to safely merging it. Clearly label significant pre-existing issues.
|
|
- The severity matches realistic impact and likelihood.
|
|
- The proposed direction addresses the root cause without creating a larger problem.
|
|
|
|
Keep an architecture or maintainability finding only when it identifies a concrete cost such as an invalid dependency direction, duplicated source of truth, leaky boundary, misleading abstraction, excessive coupling, hidden invariant, or materially harder future change. Explain the tradeoff and label non-blocking improvements as suggestions.
|
|
|
|
Require at least 80% confidence for reported defects. Drop refuted, purely speculative, or unactionable candidates. When uncertainty is important and cannot be resolved, state the missing evidence rather than asserting a defect.
|
|
|
|
## 6. Report findings
|
|
|
|
Lead with findings ordered by severity and then by confidence. Use these levels:
|
|
|
|
- `P0 Critical`: immediate security compromise, data loss, corruption, or broad outage.
|
|
- `P1 High`: concrete bug, security vulnerability, broken contract, or invalid test that should block merge.
|
|
- `P2 Medium`: real but narrower defect, meaningful defensive-coding gap, or structural problem likely to cause defects.
|
|
- `P3 Suggestion`: non-blocking architectural or maintainability improvement with a concrete benefit.
|
|
|
|
Format each finding as:
|
|
|
|
```markdown
|
|
### [P1][correctness] Short imperative title
|
|
`path/to/file.ext:line`
|
|
|
|
- Evidence: What the code does and the surrounding fact that makes it wrong.
|
|
- Failure scenario: The triggering input/state and observable outcome.
|
|
- Recommendation: The smallest sound direction for fixing the root cause.
|
|
- Confidence: 92%
|
|
```
|
|
|
|
For test findings, name the mutation or broken implementation that would still pass. For security findings, name the trust boundary, attacker-controlled input or capability, and impact. For architecture findings, name the boundary or invariant and compare the proposed structure with the current cost.
|
|
|
|
After findings, include:
|
|
|
|
- `Verdict`: `Do not merge`, `Merge after fixes`, or `Ready to merge`.
|
|
- `Review coverage`: the scope and lenses actually inspected.
|
|
- `Verification gaps`: commands, runtime checks, specifications, or environment access that were unavailable.
|
|
|
|
If no finding survives verification, say `No verified findings.` and still list material verification gaps. Do not add praise, a diff summary, or generic best-practice advice unless it helps the user act on the review.
|