Senior Code Review Skill

Use this skill before finalizing meaningful code changes.

Review only the checks relevant to the final diff. Preserve required gates.

Scope and ownership

• unrelated changes
• accidental formatting churn
• generated, vendored, compiled, or package-owned files
• unused imports, variables, types, functions, or files caused by the change
• behavior changes beyond the request

Design and maintainability

• naming clarity
• consistency with existing patterns
• incomplete fixes that minimize the diff while leaving required behavior unresolved
• abstractions without a demonstrated boundary, invariant, meaningful duplication, or variability
• substantial flow replacements without evidence of a significant benefit over improving the existing implementation, accounting for migration, verification, and maintenance costs
• unnecessary change amplification across unrelated components
• material technical debt without its scope, rationale, and follow-up condition
• speculative configurability

Behavior and affected risks

• API compatibility required by supported consumers or explicit commitments
• legacy paths or fallbacks retained without demonstrated dependencies, or removed despite unresolved consumers
• support commitments invented for hypothetical users, obsolete test accounts, fixtures, or intermediate development versions
• code retirement that discards useful data without authority or weakens necessary correctness safeguards
• migration risk
• safety risk
• performance risk
• accessibility regressions

Validation

• missing or weak evidence for the complete affected behavior
• redundant tests, fixtures, or mocks, or tests preserving abandoned fixes or implementation details instead of required behavior
• created unit tests left in the completed change, regardless of purpose or label
• isolated integration tests retained as lasting coverage, unused unit-test support code, or cleanup before corresponding E2E assertions pass

Delegation and worktrees, when used

• subagent claims that were not independently verified
• helper launches or retries that exceeded the recorded finite allowance or were not reconciled
• unnecessary helper use whose concrete benefit does not justify context, coordination, and review overhead
• child execution outside the approved task-based model/effort choices, conflicting profile overrides, or permissions, scope, authority, or workspace expansion beyond the parent assignment
• progress or task-reporting messages that set model, reasoning_effort, thinking, or analogous destination-setting overrides, or altered a parent or peer task
• task-created auxiliary worktrees without integration evidence and a verified removed or exact-blocker preserved disposition

Acceptance and report

Ask:

Would I approve this in code review?

If not, fix the issue or report the remaining risk clearly.

Recommend retained E2E tests only for an identified important behavior or realistic regression risk that existing coverage does not establish, including relevant failure and boundary cases. Every unit test created is temporary, regardless of purpose; none may remain in a completed change. Verify their still-required assertions in passing E2E coverage before removing them and exclusively used support code. Keep migration of existing non-E2E suites within authorized scope. Build, lint, type checking, and static validation remain appropriate. Preserve required assertions and repository gates, and never treat deleting a failing test as resolving its failure.

Final report format:

Summary:
- Changed X to do Y.

Verification:
- Ran: [command or check]
- Result: [passed/failed/not run, with reason]

Notes:
- [Assumptions, unrelated issues, subagent usage, follow-ups, or risk notes if any]