Add /review-pr skill and wire it into the Claude Code Review workflow

[AryanGodara]

Description

Adds a /review-pr slash command for cowprotocol/services PRs that produces a structured review report (codemap → context → severity-ranked findings) and wires the same skill into .github/workflows/claude-code-review.yml so CI uses identical review logic.

The goal is one source of truth for PR review behaviour, plus the ability to run that same review locally, both before opening a PR (against your feature branch) and against an existing PR, without leaving the terminal.

How it works at a high level

Three operating modes from one skill

The slash-command entry (.claude/commands/review-pr.md) auto-detects which mode to run in based on $ARGUMENTS and $GITHUB_ACTIONS. The reference doc (docs/COW_PR_REVIEW_SKILL.md) is shared across all three; only the diff source and the report sink differ.

• diff mode: /review-pr with no arguments. Source is git diff $(git merge-base HEAD origin/main)..HEAD, i.e. the full feature-branch worth of work vs. main. No gh calls, no checkout. Output is printed to the terminal. Use this before opening a PR.
• pr-local mode: /review-pr <N|url|owner/repo#N>. Saves the current branch, runs gh pr checkout, fetches metadata via gh pr view + gh pr diff in parallel, prints the report, and ends with a git switch <prior_branch> hint (never auto-switches). On a fork without checkout permission, degrades cleanly to a static-diff mode and flags the qualifier in the report header.
• pr-ci mode: selected automatically when $GITHUB_ACTIONS=true. The workflow has already checked out the PR; the skill skips checkout, runs the same review, and posts a single consolidated review comment via anthropics/claude-code-action@v1.

In all three modes, the report content is the same: header, codemap, context synthesis, severity-ranked findings, verdict.

Execution flow (shared across modes)

1. Metadata fetch (PR modes) — parallel gh pr view + gh pr diff + gh issue view for any Fixes/Closes/Resolves #N references in the body. Closed/merged/draft PRs proceed with a one-line warning.
2. Conditional context — path-classified sibling docs are loaded once per matching diff. Today the only sibling is docs/review-context/database-migrations.md, loaded when database/sql/** is touched. An inline OpenAPI breaking-change rule fires when any openapi.yml is modified.
3. Codemap phase — before reading file bodies, map the symbols the diff touches: new public types/traits/functions, modified function signatures (with caller counts), new trait impls, error-type changes. Tools are tried cheapest-first via a Status table: always-available (rg, git blame), optional LSP-backed (Serena MCP), optional cross-crate skills (actionbook/rust-skills), full-file Read as last resort. Every codemap step works with just rg + git; the optional tools are accelerators.
4. Context synthesis — 1–3 paragraphs combining title, description, linked issue(s), and intent. A description must describe the diff's current state; if a claim no longer matches the diff, that's a finding (Action: update the PR description to match the current diff).
5. Review and severity — applies, in order: Universal Guardrails (minimal public API, no rightward drift, single responsibility, split big files, no argument bloat, errors carry context), the conditional context loaded in step 2, CLAUDE.md conventions, and any optional Rust skills present (m06-error-handling, m07-concurrency, m04-zero-cost, m15-anti-pattern, unsafe-checker). Includes a git blame step for code that "looks unusual" — blame the lines and read the originating commit before suggesting a cleanup, so a deliberate fix from six months ago doesn't get rolled back. Anti-nit rule is mandatory: rustfmt-fixable issues are never findings; LGTM is the right verdict on a clean PR.
6. Report emission — fixed terminal template in local modes (header, codemap, context, verdict, findings grouped by High/Medium/Small, optional verification block, NEXT STEPS footer with the git switch hint). In pr-ci, the same body is rendered as Markdown and posted as a single comment.
7. Verification offer (local modes only) — after printing the report, offers to run cargo check / cargo clippy / cargo +nightly fmt --check in the background. Issues inside files this PR modified fold into the findings; warnings from untouched files are ignored.

Noise filter

Before handoff, the slash command classifies each changed file. Review surface: crates/*/src/**/*.rs (excluding generated bindings), crates/e2e/tests/**/*.rs, authored contracts/solidity/**/*.sol, **/openapi.yml, database/sql/**, configs/**, semantically interesting *.toml. Noise (skipped or skimmed): Cargo.lock, contracts/generated/**, contracts/artifacts/**, auto-generated binding Cargo.toml entries, binary fixtures. The Scope: line in the report names the human-written LOC count after filtering.

Optional-tooling probe

Step 5 of the slash command detects which optional accelerators are present in the current session (Serena MCP, actionbook/rust-skills, ra-qm-skills, the m04/m06/m07/m15 modules, unsafe-checker) and passes the list to the reference doc, where it appears verbatim in the report header's Loaded context: line. Missing optional tooling never aborts and never prints an install banner — the skill degrades to rg + general Rust knowledge.

CI workflow

.github/workflows/claude-code-review.yml now invokes the in-repo slash command (/review-pr <owner/repo>#<N>) instead of an off-the-shelf plugin. fetch-depth: 0 is set so the git blame historic-context step can resolve any line in the diff.

Files

File	Role
.claude/commands/review-pr.md	Slash-command entry. Mode detection, argument parsing, preflights, optional-tooling probe, noise filter, handoff.
docs/COW_PR_REVIEW_SKILL.md	Reference doc Claude follows. Core principles, Universal Guardrails, execution flow, severity rubric, anti-nit rule, report templates.
docs/review-context/database-migrations.md	Loaded when the diff touches database/sql/**. Rollout-shape preamble (k8s pod overlap; staging/production cadence + shadow-environment caveat) plus reversibility, n-1 compat, and CONCURRENTLY checks on hot tables.
.github/workflows/claude-code-review.yml	CI wiring: invokes /review-pr, posts one consolidated review comment per PR.

How to test

# diff mode — current branch vs main
/review-pr

# pr-local mode — against an existing PR
/review-pr 4243
/review-pr https://github.com/cowprotocol/services/pull/4243
/review-pr cowprotocol/services#4243

# pr-ci mode — runs automatically when a PR is marked Ready for review

[gemini-code-assist]

Code Review

This pull request introduces a structured local PR review skill for Claude, including command definitions, settings, and comprehensive review context documentation for Rust, database migrations, and OpenAPI specifications. Two high-severity issues were identified: the inclusion of personal plugin configurations in the settings file which could conflict with other team members' environments, and a broken setup instruction that references a non-existent documentation file.

[gemini-code-assist]

The referenced file docs/superpowers/plans/2026-04-21-pr-review-skill.notes.md is missing from the repository. This prevents the maintainer from locating the absolute paths needed to replace the placeholders in the script, rendering the setup instructions incomplete.

References

1. Every comment must include a clear, actionable suggestion for improvement and focus on logic that deviates from goals. ^(link)
2. The instruction is broken because it references a non-existent file, preventing the maintainer from completing the setup.

[gemini-code-assist]

This file contains personal plugin configurations and marketplace settings. As mentioned in the PR description, these are not required for the skill to function. Committing personal settings to the repository can cause conflicts and unintended plugin activations for other team members.

References

1. Prioritize findings related to unnecessary complexity. ^(link)
2. Committing personal configuration files adds unnecessary complexity and potential conflicts for other users.

[MartinquaXD]

This PR feels well intentioned but misguided.
Code reviews rely a lot on context and historic knowledge. Assembling a list of highly specific things to look out for will always be incomplete and hard to keep up-to-date.
IMO the play would be to agree as a team on a set of high level goals / rules-of-thumb to apply in the code base and let the AI judge if it there were followed.

I think we should probably only add highly specific things if we care deeply about them and the AI always misses them.

One thing we should probably have in this workflow is to tell the AI when to use git blame to uncover historic context. Often time when code looks very unusual it's like that for a reason so a PR that seemingly cleans up some mess could re-introduce very subtle errors that were hard to identify and fix in the first place.

[gemini-code-assist]

Code Review

This pull request introduces a structured PR review skill for the CoW Protocol services repository, including an entry-point command, a core review instruction document, and specialized context for database migrations. The review feedback identifies several critical issues regarding the reliability of the git merge-base detection, inconsistencies in severity levels that would cause important findings to be filtered out by existing automation, and a failure to account for 'degraded static-diff' mode when using local filesystem tools. Suggestions were provided to use 'origin/main' for diffs, elevate breaking API and database documentation changes to 'High' severity, and explicitly restrict tool usage when a PR is not checked out locally.

[gemini-code-assist]

The diff mode detection uses a local main branch as the merge base. This is unreliable as the local main may be stale or non-existent. Step 3 correctly uses origin/main after fetching; this definition should be consistent to prevent execution failure.

Suggested change

	- Else (`$ARGUMENTS` empty, not in CI) → `mode = "diff"`. No `gh` calls needed; source is `git diff $(git merge-base HEAD main)..HEAD`.
	- Else ($ARGUMENTS empty, not in CI) → mode = "diff". No gh calls needed; source is git diff $(git merge-base HEAD origin/main)..HEAD.

[AryanGodara]

It's a pre-req that the latest relevant branches are fetched/checked out already.
Fixing that would go beyond the scope of the PR.

[gemini-code-assist]

Using main as the merge base for the diff source is unreliable. It should use origin/main to ensure consistency with the preflight fetch logic defined in the entry command.

Suggested change

	- `diff` — local, no PR yet. Source is `git diff $(git merge-base HEAD main)..HEAD` (the whole feature-branch worth of work). No PR metadata, no `gh` calls. Output to terminal.
	- diff — local, no PR yet. Source is git diff $(git merge-base HEAD origin/main)..HEAD (the whole feature-branch worth of work). No PR metadata, no gh calls. Output to terminal.

[gemini-code-assist]

Disclosed breaking changes are currently assigned Medium severity. According to the repository style guide (Rule 6), Medium findings are treated as Low and ignored by the noise filter. Breaking changes are critical logic/contract deviations and must be High to ensure they are surfaced in CI reports.

Suggested change

When a PR touches **any `openapi.yml`**: scan for breaking changes (removed/renamed/typed-changed fields, new required request fields, narrowed enums, changed auth or HTTP method). If any are present, ask whether the goal could be achieved non-breakingly (additive field, new optional, deprecation window) and whether the affected consumer teams (Frontend, SAFE, etc.) have been notified. Severity: High when undisclosed in the PR description; Medium otherwise.

When a PR touches any openapi.yml: scan for breaking changes (removed/renamed/typed-changed fields, new required request fields, narrowed enums, changed auth or HTTP method). If any are present, ask whether the goal could be achieved non-breakingly (additive field, new optional, deprecation window) and whether the affected consumer teams (Frontend, SAFE, etc.) have been notified. Severity: High.

References

1. Only surface findings that meet the High or Critical severity criteria. Treat Medium findings as Low and ignore them unless they represent a direct logic error. ^(link)

[gemini-code-assist]

The fallback rule does not account for the degraded static-diff mode qualifier. In this mode, the PR branch is not checked out locally. Using rg, git blame, or Read on the local filesystem will incorrectly analyze the prior_branch code instead of the PR content. The skill must be explicitly instructed to avoid local filesystem tools in this mode.

Suggested change

	**Fallback rule:** in CI (or any environment where the optional tools aren't installed), every codemap step still works with `rg` and `git`. The optional tools are accelerators, not requirements.
	Fallback rule: in CI or local modes where the PR is checked out, codemap steps use rg and git. In degraded static-diff mode, the PR is NOT checked out; the skill MUST NOT use local filesystem tools (which reflect the prior_branch) and should instead rely solely on the patch content or gh api.

[gemini-code-assist]

The Non-negotiables section header states these items are High by default, but items 4 and 5 are explicitly marked as Medium. This contradiction will cause these findings to be silenced by the repository's noise filter. They should be elevated to High to match the section's intent and ensure visibility.

Suggested change

	4. **Authoritative table list.** New tables must appear in `crates/database/src/lib.rs` (search for the top-level table list). Missing → **Medium** (CI may also catch it; still the author's responsibility).
	5. **README.** Schema changes update the SQL or DB README, whichever is the convention at review time. Missing → **Medium**.
	4. Authoritative table list. New tables must appear in crates/database/src/lib.rs (search for the top-level table list). Missing → High (CI may also catch it; still the author's responsibility).
	5. README. Schema changes update the SQL or DB README, whichever is the convention at review time. Missing → High.

References

1. Only surface findings that meet the High or Critical severity criteria. Treat Medium findings as Low and ignore them. ^(link)