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

[AryanGodara]

What

Adds /review-pr, a single Claude Code skill for reviewing PRs in cowprotocol/services. Three modes — locally vs your current branch (no PR yet), locally vs an existing PR, or in CI — share the same reference doc. Only the diff source and the report sink differ.

Also re-introduces the Claude Code Review workflow that #4381 dropped a few days ago. That deletion was a stop-gap because the off-the-shelf code-review@claude-code-plugins was burning too many tokens. This PR replaces it with a workflow that calls a focused in-repo skill (~400 LOC), so the prompt is bounded.

Design

• Single source of truth: docs/COW_PR_REVIEW_SKILL.md. All three modes follow it.
• diff mode covers "review before I open the PR". Runs against git merge-base HEAD origin/main, no gh calls.
• Six universal guardrails — minimal public API, no rightward drift, single responsibility, split big files, no argument bloat, errors carry context — replace the older alloy/openapi sibling docs. The only kept sibling is database-migrations.md, because k8s pod overlap and staging/prod cadence are genuinely CoW-specific.
• Anti-nit rule is mandatory. rustfmt-fixable issues are never findings. LGTM is a valid verdict.
• Read-only in local modes. No auto-stash, no auto-rebase, no auto-comments. The user owns what lands on the PR.
• git blame runs before flagging unusual code, in case it was a deliberate prior fix. Workflow uses fetch-depth: 0 for this.

Files

File	Role
.claude/commands/review-pr.md	Slash-command entry for /review-pr. Mode detection, preflights, handoff.
.claude/commands/blame-context.md	Slash command for /blame-context. Tracked standalone in #4373.
.claude/commands/pr-synthesis.md	Slash command for /pr-synthesis. Tracked standalone in #4374.
docs/COW_PR_REVIEW_SKILL.md	Reference doc Claude follows. Guardrails, execution flow, severity rubric, report templates.
docs/skills/git-blame-historic-context.md	Sibling skill — used before flagging unusual code. Tracked standalone in #4373.
docs/skills/pr-context-synthesis.md	Sibling skill — produces the CONTEXT block. Tracked standalone in #4374.
docs/skills/pr-blame-walk.md	Companion incident-investigation skill (N→1 suspect ranking).
docs/review-context/database-migrations.md	Loaded when database/sql/** is touched.
.github/workflows/claude-code-review.yml	CI wiring. fetch-depth: 0 for git blame.

How to try it

/review-pr                                # diff mode — current branch vs main
/review-pr 4243                           # pr-local mode
/review-pr cowprotocol/services#4243      # explicit form

pr-ci runs automatically when a PR is marked Ready for review.

CI behaviour to expect

The Claude Code Review job on this PR will fail with Workflow validation failed. That's GitHub's security rule against workflow self-modification — a PR modifying a workflow can't run its own modified version. Ignore the failure on this PR; every subsequent PR runs /review-pr as expected.

External-fork PRs silently no-op (secrets aren't available, pull-requests: write is downgraded). Same as the previous workflow.

Feedback

False positives become counter-examples for the anti-nit rule. Send me anything the report flags that you'd close as "not a real finding".

[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.

[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.

[AryanGodara]

Note on CI
The Claude Code Review job on this PR will fail with Workflow validation failed. The workflow file must exist and have identical content to the version on the repository's default branch.

GitHub's own error message says to ignore this on the first PR. After this lands, every subsequent PR will run /review-pr from the repo as expected.

[jmg-duarte]

Great initiative, I'm just worried that merging this is a Sisyphean task

For the more complicated notes, you can follow up after, no need to separate things now, but do keep track of them or open an issue tracking it

[jmg-duarte]

Instead of 3 modes, why not 3 skills?

Even if they're just then called from there instead of having modes that add entropy to the agent

[AryanGodara]

Considered this. The three modes share a lot of logic, ie, codemap, severity rubric, universal guardrails, anti-nit rule, finding shape, optional-tool probe. Splitting into three skills means three copies of all that. Might even need to be kept in sync by hand.

The mode-specific code localizes to four narrow places:

• where the diff comes from (git diff merge-base vs gh pr diff)
• whether to fetch PR metadata
• whether to gh pr checkout
• where the report goes (terminal vs the CI comment posted by the action).

Those are conditionals, not different review logics. So moving them into three skills doesn't remove the entropy, just relocates it.

Happy to revisit if mode-specific divergence grows past two or three places. Right now I'd rather keep one source of truth for the review content.

[jmg-duarte]

w.r.t. my previous comment, it really seems to me that we could have a CI skill specifically

[AryanGodara]

the CI mode is one of those four-place conditionals, not a separate review logic.

Also for context, the decision originated from initial reviews, where consensus was that we can re-purpose the first draft of this PR (which was only to self review/ review already ready for review PRs), into a single source of logic/rules.
And then also use the same skill in the CI, instead of having 2 separate review tools

[jmg-duarte]

This is not very clear and I'd expect this to lead to extra review notes

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive framework for automated PR reviews and historical context analysis, including new Claude commands and detailed skill documentation for database migrations and incident investigation. Feedback was provided to address a missing metadata field required for commit tracking and to clarify that concurrent index creation must be executed outside of transaction blocks to prevent migration failures.

[gemini-code-assist]

PostgreSQL does not allow CREATE INDEX CONCURRENTLY to be executed within a transaction block. Since Flyway migrations (referenced in line 13) are executed within a transaction by default, any migration using this command will fail unless it is explicitly configured to run non-transactionally (e.g., using Flyway's non-transactional configuration).

Suggested change

	3. **Blocking index creation on hot tables.** Use `CREATE INDEX CONCURRENTLY` on anything in the auction/settlement critical path (`orders`, `trades`, `auctions`, `settlements`, `order_events`, `auction_orders`, `quotes`, `order_quotes`). A blocking `CREATE INDEX` on one of these → **High**.
	3. Blocking index creation on hot tables. Use CREATE INDEX CONCURRENTLY on anything in the auction/settlement critical path (orders, trades, auctions, settlements, order_events, auction_orders, quotes, order_quotes). Note that concurrent index creation cannot run inside a transaction; ensure the migration is configured to run non-transactionally. A blocking CREATE INDEX on one of these -> High.

[gemini-code-assist]

The gh pr view command is missing the headRefOid field in the --json argument. This field is necessary for the logic in section 2.5 (line 104), which compares the latest review's commit_id with the current head SHA to determine if new commits have been pushed since the last review round.

Suggested change

	--json title,body,author,labels,files,baseRefName,headRefName,additions,deletions,commits,reviewDecision,isDraft,state
	--json title,body,author,labels,files,baseRefName,headRefName,headRefOid,additions,deletions,commits,reviewDecision,isDraft,state

[squadgazzz]

The Claude Code PR review CI job was removed as being very expensive due to our codebase size.