Skip to content

Add PR review mode to code-review workflow#46

Open
galel12 wants to merge 1 commit into
mainfrom
code-review-pr-mode
Open

Add PR review mode to code-review workflow#46
galel12 wants to merge 1 commit into
mainfrom
code-review-pr-mode

Conversation

@galel12

@galel12 galel12 commented May 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds /pr and /pr-continue commands for reviewing GitHub Pull Requests with deep cross-file analysis, conversational findings, and optional GitHub review comment posting
  • Keeps the existing local review mode (/start, /continue) unchanged -- the two modes have distinct input models, role models, and output formats
  • Updates clean.md, controller.md, guidelines.md, SKILL.md, and README.md to integrate the new commands

Design decisions

Aspect Local review PR review
Input git diff HEAD gh pr diff + gh api contents
Role model Reviewer + implementor dual-role Single reviewer perspective
Findings Formal table with severity/category Conversational, no labels
Artifacts 6+ files (profile, summary, decisions...) 2 files (metadata + findings)
Code changes Yes (during /continue) No (author fixes on their branch)
Output Decision table for user Optional GitHub review comments

New files

  • skills/pr.md -- Core PR review: parse URL/number, fetch metadata + diff + full files via gh, load existing comments, deep cross-file analysis, conversational findings, optional GitHub posting with preview
  • skills/pr-continue.md -- Iterative re-review: interdiff since last reviewed SHA, progress tracking (fixed/remaining/new), follow-up or approving review posting
  • commands/pr.md -- Thin wrapper for /pr
  • commands/pr-continue.md -- Thin wrapper for /pr-continue

Modified files

  • skills/controller.md -- PR phases, dispatch logic, artifact tables, next-step recommendations, context management note
  • guidelines.md -- PR-specific principles (read-only, single perspective, no quota, depth over breadth), hard limits (preview before posting, no local changes), safety and escalation rules
  • skills/clean.md -- Handles both {branch}/ and pr-{number}/ artifact directories
  • SKILL.md -- Updated description and command routing for /pr, /pr-continue
  • README.md -- PR prerequisites (gh), phases, typical flow, conversational findings docs, artifact layout, directory structure

Skill reviewer results

Ran skill-reviewer /review against the updated skill. Initial review found 7 suggestions (0 blockers), all addressed in this PR:

  1. PR artifact table added to controller
  2. JSON payload written to temp file instead of inline shell quoting
  3. Explicit previous_rounds initialization guidance
  4. clean.md updated for PR artifacts
  5. Large file (>1MB) API handling documented
  6. guidelines.md and review-protocol.md referenced from PR skills
  7. Context Management section notes PR review skips subagents

Made with Cursor

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@galel12, we couldn't start this review because you've used your available PR reviews for now.

Your plan includes 1 review of capacity. Refill in 58 minutes and 7 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: ae035fd9-fe80-46b6-b948-bef12d5500a9

📥 Commits

Reviewing files that changed from the base of the PR and between 58ad70b and 72666cd.

📒 Files selected for processing (9)
  • code-review/README.md
  • code-review/SKILL.md
  • code-review/commands/pr-continue.md
  • code-review/commands/pr.md
  • code-review/guidelines.md
  • code-review/skills/clean.md
  • code-review/skills/controller.md
  • code-review/skills/pr-continue.md
  • code-review/skills/pr.md

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

New /pr and /pr-continue commands for reviewing GitHub Pull Requests
with deep cross-file analysis, conversational findings (no severity
labels or formal tables), and optional GitHub review comment posting.

Key design choices:
- Separate commands from local review (/start, /continue) since PR
  review has a fundamentally different input model, role model, and
  output format
- Single reviewer perspective (no dual-role implementor assessment)
- Minimal artifacts (metadata + findings only) for round continuity
- Full file fetching via gh API for context beyond the diff
- Iterative re-review via /pr-continue with interdiff tracking

Also updates clean.md to handle PR artifact directories, adds PR
artifact table to controller, and references guidelines/review-protocol
from PR skills.

Co-authored-by: Cursor <cursoragent@cursor.com>
@galel12 galel12 force-pushed the code-review-pr-mode branch from 4a4a36b to 72666cd Compare May 25, 2026 10:41
@galel12 galel12 requested review from adalton and amir-yogev-gh May 25, 2026 10:42
@galel12 galel12 self-assigned this May 25, 2026
@adalton

adalton commented May 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, Gal — the design thinking here is solid, and the PR review capability is something worth having. Before getting into structural feedback, I want to raise a more fundamental question.

Does this need to be a workflow?

When a user says "Claude, review PR 123," Claude already:

  • Reads the project's CLAUDE.md/AGENTS.md for conventions
  • Fetches the diff and full files via gh
  • Reasons about cross-file interactions
  • Can post review comments with confirmation

The workflow adds structure around that — existing comment awareness, generated file skipping, preview before posting, a conversational format. But these are behaviors you can encode in a CLAUDE.md section or a lightweight custom slash command. They don't require a controller, artifact schema, and 500 lines of skill specifications.

The multi-round case (/pr-continue) seems like the strongest value proposition, but even that is weaker than it looks. The findings from your previous review are already on the PR as GitHub comments. When Claude re-reviews, it can fetch those comments via gh api, see what's resolved vs. not, and pick up where you left off — all from state that already lives in GitHub. The local artifact files are a less durable copy of that.

Compare this to the local review workflow, where the dual-role model, artifact-driven iteration loop, and decision tables are things you genuinely can't replicate with a one-liner. That workflow earns its complexity. I'm not sure this one does.

If there's a concrete scenario where the workflow catches something a direct ask misses — not just "more consistently" but "at all" — I'd want to hear it. That would change my thinking.

If the workflow does move forward, I'd suggest we make it a separate pr-review workflow rather than a second mode inside code-review. The design table in your PR description shows these modes share almost nothing: different inputs, role models, output formats, artifact structures, safety constraints, and audiences. Merging them forces every touchpoint (controller, guidelines, clean, README) to branch on mode, and that conditional complexity compounds with future changes.

I also found a few implementation issues in the skills that should be addressed regardless:

  1. Fork PRs will break. gh api repos/{owner}/{repo}/contents/{path}?ref={headSHA} uses the base repo, but for fork PRs the head SHA lives in the fork. Use headRepositoryOwner/headRepository from gh pr view.

  2. Diff position is fragile. GitHub's API supports line + side as alternatives — much easier to compute correctly.

  3. Interdiff doesn't handle rebases. The compare API shows all rebased commits if the author rebased. The skill should detect this and fall back to comparing the full PR diff against previous findings.

@galel12

galel12 commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for this review, @adalton. The fundamental question is fair and I want to address it directly.

Does this need to be a workflow?

You're right that Claude can do a PR review if you just ask. Where the workflow earns its keep is in the consistency and repeatability across team members and sessions — not in capability that doesn't otherwise exist. Specifically:

  • Existing comment awareness — a direct ask won't systematically fetch and cross-reference prior review comments to avoid duplicates. The workflow does this every time.
  • Binary/generated file filtering — without explicit instructions, the model will happily review generated code. The workflow skips it consistently.
  • Structured posting with confirmation — the workflow builds the review payload, shows it, and waits. A direct ask is more likely to post line comments one at a time or skip confirmation.
  • Team discoverability/pr is a single entry point that any team member can use without knowing the right incantation.

That said, your point about /pr-continue is well taken. The previous round's findings already live on GitHub as comments. The local metadata file is a less durable copy. I could simplify /pr-continue to always reconstruct context from GitHub state rather than depending on local artifacts.

If you still feel a CLAUDE.md section or lightweight command is the right call after considering the above, I'm open to that. But I think the structured approach catches edge cases that a freeform ask misses in practice.

Separate pr-review workflow vs. second mode

Agree. Looking at the implementation, the two modes share the controller dispatch pattern and clean.md — everything else branches on mode. A separate pr-review workflow would be cleaner and avoid the conditional complexity in controller.md, guidelines.md, and README.md. Happy to split it out.

Implementation bugs — all three are valid

  1. Fork PRs: Will fix the contents API call to use headRepositoryOwner/headRepository from gh pr view instead of assuming the base repo.

  2. Diff positionline + side: Agreed, line + side is more robust and easier to compute. Will switch.

  3. Interdiff + rebases: Will add rebase detection (compare commit count / check if merge base changed) and fall back to full-diff comparison against previous findings when a rebase is detected.

I'll push fixes for the three implementation bugs regardless of which direction we go on the workflow-vs-command question. Want to sync on that decision before I do the split?

@adalton

adalton commented May 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for the follow-up, @galel12. Your four arguments are fair — existing comment awareness in particular is a behavior that a direct ask won't do without being told to. The question is what the right form factor is for encoding that.

The behaviors this workflow guarantees — fetching existing comments, skipping generated files, previewing before posting, calibrating against project conventions — are all valuable. But they're declarative rules, not procedural logic. They don't need a controller, artifact schema, or multi-round state management. Compare to the local review workflow, where the dual-role model, decision tables, and artifact-driven iteration loop are structural capabilities that can't be expressed as a few lines of guidance.

For PR review, the lightest effective solution is a section in the target project's AGENTS.md. For example, in flightctl's AGENTS.md:

## PR Review

When reviewing a Pull Request:

- Fetch existing review comments (`gh api repos/{owner}/{repo}/pulls/{number}/comments`)
  and review bodies (`gh api repos/{owner}/{repo}/pulls/{number}/reviews`) before
  analyzing. Do not duplicate feedback that has already been given.
- Skip generated files (`*.gen.go`, `*.pb.go`, `vendor/`, `node_modules/`, lock files).
- Read full files for context, not just diffs.
- For fork PRs, use `headRepositoryOwner`/`headRepository` from `gh pr view` for
  content API calls — the head SHA lives in the fork repo, not the base.
- Preview the full review before posting. Never post without confirmation.
- Use `line` + `side` instead of `position` for line-level review comments.
- Calibrate against the project's conventions in this file and CONTRIBUTING.md,
  not generic preferences.

This encodes every valuable behavior from the workflow in ~15 lines. It's discoverable by any AI tool that reads AGENTS.md (not just ones with this specific workflow installed), and it lives alongside the rest of the project's conventions.

I'd suggest closing this PR and instead adding a PR review section to the target project(s) where the team wants this behavior. If we later find that declarative rules aren't enough — that there's a procedural need the model can't handle from guidance alone — that's evidence for revisiting the workflow approach.

The implementation bug fixes (fork PRs, line+side, rebase detection) are good catches regardless — they're worth capturing in the AGENTS.md guidance, which this example does for the first two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants