-
Notifications
You must be signed in to change notification settings - Fork 165
Add /review-pr skill and wire it into the Claude Code Review workflow #4351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
37520e0
0dcf5cb
120ca11
c84480d
2dc79ba
2598a28
04370d6
827b159
456d4a9
09d2413
b04e4f5
6f58e13
c867cbc
eb4c869
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| --- | ||
| description: Recover historic context for a file:line via `git blame` plus the squash-PR pivot. Wraps `docs/skills/git-blame-historic-context.md` end-to-end and prints the strengthens / weakens / inverts decision so the caller can weight a finding before flagging suspicious-looking code. Read-only. | ||
| --- | ||
|
|
||
| Look up historic context for: $ARGUMENTS | ||
|
|
||
| ## Parse $ARGUMENTS | ||
|
|
||
| Accept any of: | ||
|
|
||
| - `path:line` — single line. e.g. `crates/driver/src/.../settlement.rs:444` | ||
| - `path:start-end` — line range. e.g. `crates/driver/src/.../settlement.rs:434-447` | ||
| - `path line` or `path start-end` — space-separated form (handy when paths contain `:`). | ||
|
|
||
| If `$ARGUMENTS` is empty or unparseable, print the usage block and abort: | ||
|
|
||
| ``` | ||
| Usage: /blame-context <path>:<line> | ||
| /blame-context <path>:<start>-<end> | ||
| ``` | ||
|
|
||
| ## Procedure | ||
|
|
||
| Follow `docs/skills/git-blame-historic-context.md` end-to-end. Concretely: | ||
|
|
||
| 1. `git blame -L <start>,<end> -- <path>` to find the originating commit. | ||
| 2. If the surface looks like a wholesale move/refactor (same author across many lines, recent date, identical hashes), retry with `git blame -w -C -C -C -L <start>,<end> -- <path>` to recover the real authoring commit. | ||
| 3. `git log -1 --format='%s%n%b' <sha>` for the commit body. | ||
| 4. If the subject ends with `(#NNNN)`, pivot to the PR conversation: `gh pr view <NNNN>` (the gh CLI infers the repo from the working directory). The PR body is usually richer than the squash commit alone. | ||
|
|
||
| Then print the report below. | ||
|
|
||
| ## Output | ||
|
|
||
| ``` | ||
| ─── Blame for <path>:<lines> | ||
| <sha> <author> <date> <subject> | ||
|
|
||
| ─── Originating commit / PR | ||
| <commit body, OR PR title + body if a (#NNNN) PR was found> | ||
|
|
||
| ─── Decision | ||
| <Strengthens | Weakens | Inverts> the suspicion that this code is unusual. | ||
| <one-sentence reason naming the concrete signal from the originating PR/commit> | ||
| Action: <keep finding | downgrade to Question | drop> | ||
| ``` | ||
|
|
||
| ## Rules | ||
|
|
||
| - **Read-only.** `git blame`, `git log`, `git show`, `gh pr view`, `gh api` GET-only. No `git commit`, no `git checkout`, no mutating `gh api` verbs. | ||
| - **Don't comment on the PR.** Don't edit files. The caller owns what to do with the decision — this command just supplies the evidence. | ||
| - **Don't invent a decision.** If blame surfaces nothing useful (line is brand new, generator-output, vendored), print `Decision: insufficient signal` and explain why. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| --- | ||
| description: Produce a tight 1–3 paragraph what / why / how synthesis of a single PR. Fetches title, body, linked issue, and diff scope; outputs per `docs/skills/pr-context-synthesis.md`. Read-only. | ||
| --- | ||
|
|
||
| Synthesise PR: $ARGUMENTS | ||
|
|
||
| ## Parse $ARGUMENTS | ||
|
|
||
| Accept any of: | ||
|
|
||
| - A PR number: `4267`/`#4267` | ||
| - A full URL: `https://github.com/cowprotocol/services/pull/4267` | ||
| - An `owner/repo#N` form: `cowprotocol/services#4267` | ||
|
|
||
| Default `owner/repo` to `cowprotocol/services` when only a number is given. | ||
|
|
||
| If `$ARGUMENTS` is empty or unparseable, print: | ||
|
|
||
| ``` | ||
| Usage: /pr-synthesis <PR_NUMBER> | ||
| /pr-synthesis https://github.com/owner/repo/pull/<N> | ||
| /pr-synthesis owner/repo#<N> | ||
| ``` | ||
|
|
||
| and abort. | ||
|
|
||
| ## Procedure | ||
|
|
||
| Fetch in this order — context first, then diff. Lets the synthesis read the diff with the author's intent already in mind, and lets `gh pr view` report `additions`/`deletions` so the diff fetch can be sized appropriately. | ||
|
|
||
| **Step 1 — PR metadata + closing issues.** Cheap; bounded size. | ||
|
|
||
| ```bash | ||
| gh pr view <N> -R <owner>/<repo> --json title,body,files,additions,deletions,labels,baseRefName,headRefName,closingIssuesReferences | ||
| ``` | ||
|
|
||
| **Step 2 — linked issues.** For each entry in `closingIssuesReferences` (GitHub's own parsing of `Fixes #N` / `Closes #N` / `Resolves #N`, more reliable than regexing the body, and follows cross-repo references correctly), fetch the issue: | ||
|
|
||
| ```bash | ||
| gh issue view <issue.number> -R <issue.repository.owner>/<issue.repository.name> --json title,body,labels,state | ||
| ``` | ||
|
|
||
| If `closingIssuesReferences` is empty, proceed without a linked issue — never invent one. | ||
|
|
||
| **Step 3 — diff fetch (size-gated).** Use `additions + deletions` from step 1 to decide: | ||
|
|
||
| - **`additions + deletions <= 2000`** — fetch the full diff: | ||
|
|
||
| ```bash | ||
| gh pr diff <N> -R <owner>/<repo> | ||
| ``` | ||
|
|
||
| - **`additions + deletions > 2000`** — *do not* fetch the full diff. It can blow the context window (e.g. PR #4217 = 376k lines). Get the complete per-file list via the paginated REST endpoint — `gh pr view --json files` silently caps at 100 files, which underreports scope on big PRs: | ||
|
|
||
| ```bash | ||
| gh api --paginate "repos/<owner>/<repo>/pulls/<N>/files" \ | ||
| --jq '.[] | {filename, status, additions, deletions}' | ||
| ``` | ||
|
|
||
| Build `<diff_summary>` from these per-file records (`filename`, `additions`, `deletions`, `status` — added / modified / renamed / removed). Bucket lockfiles, generated bindings, vendored artifacts, and codegen output — call them out as a single line each, not file-by-file. State in the synthesis that the diff was summarised at file-scope only. | ||
|
|
||
| Build: | ||
|
|
||
| - `<diff_summary>` = full hunks (small PR) or per-file scope buckets (large PR). The ground truth. | ||
| - `<pr_text>` = title + body | ||
| - `<linked_issue>` = fetched issue(s), if any | ||
|
|
||
| Then follow `docs/skills/pr-context-synthesis.md` Rules and Shape. Output the synthesis verbatim — no header, no metadata, no separator lines. The caller pastes this directly into a review thread, an incident report, or a Slack message. | ||
|
|
||
| ## Rules | ||
|
|
||
| - **Read-only.** `gh pr view`, `gh pr diff`, `gh issue view`, `gh api` GET-only. No `gh pr review`, no `gh pr comment`, no mutating `gh api` verbs. | ||
| - **Don't fetch a diff you can't read.** The 2000-line gate above is a hard preflight, not a suggestion. Above the gate, the per-file scope is enough to write the synthesis honestly; trying to swallow a 100k-line diff just truncates the context and corrupts the output silently. | ||
| - **Synthesise, don't copy-paste.** If `<pr_text>` is sparse, say so plainly: *"description is minimal; intent inferred from diff"*. | ||
| - **No vague verbs.** *"This PR updates something"* is a failure. Name the component, the change, and the mechanism. The anti-vague-verb rule from the skill doc applies verbatim. | ||
| - **Watch for description-vs-diff drift.** `<pr_text>` must describe `<diff_summary>`'s current state. If a claim is no longer true of the diff, note it in the synthesis as *"description claims X; diff shows Y"*. Fetching the issue + metadata before the diff is what lets you spot these — read the intent first, then check whether the diff matches. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| --- | ||
| description: Produce a structured PR review for cowprotocol/services. Invoked locally as `/review-pr` (diff mode, against current branch vs main) or `/review-pr <N|url>` (PR mode). Same command also runs in CI via `.github/workflows/claude-code-review.yml`, where it posts a single review comment instead of printing to terminal. Read-only in local modes; the user posts any comments manually. | ||
| --- | ||
|
|
||
| Review PR: $ARGUMENTS | ||
|
|
||
| Follow the instructions in `./docs/COW_PR_REVIEW_SKILL.md` to produce the review report. | ||
|
|
||
| ## Prologue (execute in order; abort on any failure) | ||
|
|
||
| ### 1. Detect mode | ||
|
|
||
| The skill runs in one of three modes. Detection: | ||
|
|
||
| - If the environment variable `$GITHUB_ACTIONS == "true"` → `mode = "pr-ci"`. | ||
| - `$ARGUMENTS` MUST be a PR number, URL, or `owner/repo#N` form (the workflow passes it). | ||
| - Else if `$ARGUMENTS` is non-empty → `mode = "pr-local"`. Parse the argument (see [step 2](#2-parse-arguments-pr-modes-only)). | ||
| - Else (`$ARGUMENTS` empty, not in CI) → `mode = "diff"`. No `gh` calls needed; source is `git diff $(git merge-base HEAD origin/main)..HEAD` (the actual command runs in step 3 below, after fetching `origin/main`). | ||
|
|
||
| ### 2. Parse $ARGUMENTS (PR modes only) | ||
|
|
||
| *(Skip in `diff` mode.)* | ||
|
|
||
| Accept any of: | ||
|
|
||
| - A PR number: `4267` | ||
| - A full URL: `https://github.com/cowprotocol/services/pull/4267` | ||
| - An `owner/repo#N` form: `cowprotocol/services#4267` | ||
|
|
||
| Default `owner/repo` to `cowprotocol/services` when only a number is given. | ||
|
|
||
| Extract: `<PR_NUMBER>`, `<owner>`, `<repo>`. | ||
|
|
||
| If unparseable, print and abort: | ||
|
|
||
| ``` | ||
| Usage: /review-pr # diff mode (current branch vs main) | ||
| /review-pr <PR_NUMBER> # PR mode | ||
| /review-pr https://github.com/owner/repo/pull/<N> | ||
| /review-pr owner/repo#<N> | ||
| ``` | ||
|
|
||
| ### 3. Diff-mode preflight | ||
|
|
||
| *(Only in `mode == "diff"`.)* | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| git fetch origin main --quiet | ||
| BASE=$(git merge-base HEAD origin/main) | ||
| git diff --stat "$BASE..HEAD" | ||
| ``` | ||
|
|
||
| If `git diff "$BASE..HEAD"` is empty, print `No diff vs main — nothing to review.` and exit clean (not an error). | ||
|
|
||
| There is **no** clean-tree check, **no** rebase, and **no** `git pull` of main. Diff scope comes from the fetched `origin/main` merge-base. | ||
|
|
||
| ### 4. PR-mode preflight | ||
|
|
||
| *(Only in `mode == "pr-local"` or `mode == "pr-ci"`.)* | ||
|
|
||
| #### 4a. Working tree (pr-local only) | ||
|
|
||
| Run `git status --porcelain`. If non-empty, print it plus: | ||
|
|
||
| ``` | ||
| Working tree is dirty. Stash or commit your changes, then re-run. | ||
|
|
||
| git stash # temporary | ||
| git stash pop # to restore later | ||
| ``` | ||
|
|
||
| Then **abort**. Never auto-stash. | ||
|
|
||
| #### 4b. Save the current branch (pr-local only) | ||
|
|
||
| Save the current branch name to `<prior_branch>` so the report's NEXT STEPS footer can suggest `git switch <prior_branch>` when the review is done. | ||
|
|
||
| #### 4c. Fetch base ref | ||
|
|
||
| Run `git fetch origin --quiet`. This makes the diff comparable to base without rebasing or modifying the user's branch. | ||
|
|
||
| #### 4d. Checkout the PR | ||
|
|
||
| Run `gh pr checkout <PR_NUMBER> -R <owner>/<repo>`. Failure handling: | ||
|
|
||
| - **`gh` not installed** → print `Install gh: https://cli.github.com/` and abort. | ||
| - **Auth error** → print `gh auth status` output plus `Run: gh auth login` and abort. | ||
| - **PR doesn't exist / wrong repo** → surface `gh`'s error verbatim and abort. | ||
| - **Fork without checkout permission** → switch to **degraded static-diff mode**: | ||
| - Set `mode_qualifier = "degraded static-diff"`. | ||
| - In the reference doc's §2, replace `gh pr diff <N>` with `gh pr diff <N> --patch -R <owner>/<repo>`. | ||
| - Flag the qualifier in the report header's `Mode:` line. | ||
| - **Any other error** → surface verbatim and abort. | ||
|
|
||
| In `pr-ci`, the workflow has already checked out the PR branch — skip 4d and just verify `HEAD` matches the expected ref. | ||
|
|
||
| ### 5. Optional-tooling probe | ||
|
|
||
| Detect which optional accelerators are available in the current session: Serena MCP (`mcp__plugin_serena_serena__*`) and `actionbook/rust-skills` (`rust-call-graph`, `rust-symbol-analyzer`, `rust-trait-explorer`, `rust-code-navigator`). | ||
|
|
||
| Build a `loaded_context` list of whichever ones resolved. Pass it through to the reference doc; it prints verbatim in the report header's `Loaded context:` line. | ||
|
|
||
| Do not abort if a skill is missing. Do not print install banners. | ||
|
|
||
| ### 6. Noise filter (before handoff) | ||
|
|
||
| Classify each changed file: | ||
|
|
||
| **Review surface (read fully):** | ||
|
|
||
| - Anything under `crates/*/src/**/*.rs` (excluding `contracts/generated/**`). | ||
| - `crates/e2e/tests/**/*.rs`. | ||
| - `contracts/solidity/**/*.sol` (authored Solidity). | ||
| - Config files: `**/openapi.yml`, `database/sql/**`, `configs/**`, semantically interesting `*.toml`. | ||
|
|
||
| **Noise (skip or skim):** | ||
|
|
||
| - `Cargo.lock` (any). CI validates the resolution; reviewing the lockfile diff is high-cost, low-signal. | ||
| - `contracts/generated/**` and `contracts/artifacts/**` — machine-generated bindings and ABI JSON. | ||
| - Auto-generated `Cargo.toml` entries from contract-binding crates. | ||
| - Binary fixtures, ABI blobs, snapshot files. | ||
|
|
||
| Report the filter in the report's `Scope:` line as `+X −Y across Z files (~N LOC human-written; rest generated/lockfile, filtered)`. | ||
|
|
||
| ### 7. Handoff | ||
|
|
||
| Read `docs/COW_PR_REVIEW_SKILL.md` and follow it from §2 (Metadata Fetch) onward, passing through: | ||
|
|
||
| - `mode` (`diff` / `pr-local` / `pr-ci`) and `mode_qualifier` if set. | ||
| - `<PR_NUMBER>`, `<owner>`, `<repo>` (PR modes only). | ||
| - `prior_branch` (`pr-local` only). | ||
| - `loaded_context` (optional skills detected in step 5). | ||
| - The noise-filter classification from step 6 — the reference doc uses it to decide what to read. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| name: Claude Code Review | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [ready_for_review] | ||
|
|
||
| jobs: | ||
| claude-review: | ||
| if: ${{ !github.event.pull_request.draft }} | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| issues: read | ||
| id-token: write | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| # Full history is needed so the skill's `git blame`-based | ||
| # historic-context step can resolve any line in the diff. | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Run Claude Code Review | ||
| id: claude-review | ||
| uses: anthropics/claude-code-action@e0f2d99545298b87c2f984ab534af3a6534142ae # v1 | ||
| with: | ||
| anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} | ||
| # Invoke the in-repo slash command. The skill detects | ||
| # $GITHUB_ACTIONS=true and switches to `pr-ci` mode, posting | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. w.r.t. my previous comment, it really seems to me that we could have a CI skill specifically
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| # a single consolidated review comment instead of streaming | ||
| # to a terminal. Skill source of truth: docs/COW_PR_REVIEW_SKILL.md | ||
| prompt: '/review-pr ${{ github.repository }}#${{ github.event.pull_request.number }}' | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.