refactor(review-pr): delegate diff collection to workflow agents#49
refactor(review-pr): delegate diff collection to workflow agents#49cblecker wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Changesreview-pr diff collection and agent orchestration refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)pr-review-toolkit/skills/review-pr/review-pr.jsFile contains syntax errors that prevent linting: Line 562: Illegal return statement outside of a function 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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the /review-pr workflow so each review agent collects its own diff context (via diffPreamble()), while the orchestrator selects which agents to run based on git diff --name-status, reducing large diff payloads passed through the workflow layer.
Changes:
- Moved diff collection responsibilities into workflow agents via a new
diffPreamble()prompt block. - Replaced in-workflow regex agent selection with orchestrator-provided
agents. - Updated skill documentation and bumped plugin version to
1.2.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pr-review-toolkit/skills/review-pr/SKILL.md | Updates allowed tools and rewrites Phase 1 to fetch base ref + select agents via git diff --name-status. |
| pr-review-toolkit/skills/review-pr/review-pr.js | Removes centralized diff/context building; adds diffPreamble() and switches to orchestrator-provided agents. |
| pr-review-toolkit/.claude-plugin/plugin.json | Bumps plugin version to 1.2.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const results = await parallel(selected.map(name => () => { | ||
| const prompt = PROMPTS[name] + buildContext() | ||
| const prompt = diffPreamble() + PROMPTS[name] | ||
| const opts = { label: name, schema: FINDING_SCHEMA, phase: 'Analyze', effort: 'max' } |
There was a problem hiding this comment.
Good catch. Fixed — swapped to PROMPTS[name] + '\n\n' + diffPreamble() so role/identity comes first. Also updated the stale code-reviewer reference from "Review the diff provided in the review context below" to "Review the PR diff collected in the setup section above."
| const selected = config.agents | ||
| log('Running ' + selected.length + ' review agents: ' + selected.join(', ')) |
There was a problem hiding this comment.
Addressed. Added validation that falls back to ['code-reviewer'] when config.agents is missing/non-array, and filters for agent names with valid entries in the PROMPTS object.
| allowed-tools: | ||
| - Workflow | ||
| - Bash(git rev-parse *) | ||
| - Bash(git fetch *) | ||
| - Bash(git diff *) | ||
| - Bash(jq *) | ||
| - Bash(git diff --name-status *) |
There was a problem hiding this comment.
Not applicable — the allowed-tools in SKILL.md constrain the orchestrator (skill runner), not the workflow subagents. Workflow agents get the default workflow subagent toolset which includes all Bash and MCP tools. The orchestrator only needs git diff --name-status for agent selection; it never runs full diffs or jq itself.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr-review-toolkit/skills/review-pr/review-pr.js`:
- Around line 533-537: The code does not validate that config.agents exists
before accessing its .length property, which will crash if config.agents is
undefined, and it does not verify that agent names in config.agents have
corresponding prompts in the PROMPTS object before dispatching them. Add a guard
to ensure config.agents is provided with a fallback to an empty array if
missing, then filter the selected array to only include agent names where
PROMPTS[name] is defined before passing them to the parallel function. This
prevents crashes and ensures only valid agents with defined prompts are
executed.
- Around line 60-63: The remote collection instructions currently use
get_file_contents which loses the actual diff-hunk context and fails on deleted
or renamed files. Replace the instruction that uses get_file_contents with
instructions to retrieve and use patch hunks or diffs from the pull_request_read
method instead. This will preserve the true diff context, properly handle file
changes (including deletions and renames), and ensure line comments map
correctly to the actual PR diff lines.
In `@pr-review-toolkit/skills/review-pr/SKILL.md`:
- Line 13: The Bash skill definition in this file is too restrictive and only
permits `git diff --name-status`, but review-pr.js now requires running full
`git diff` commands with branch references and file filters (e.g., `git diff
origin/<baseRef>...HEAD -- <files>`). Update the Bash skill definition to allow
the complete git diff syntax needed for patch collection by removing the
restrictive `--name-status` flag constraint and permitting the full git diff
command with branch comparisons and file path arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cblecker/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80c4c1b0-6e52-450b-9099-f8046db1b351
📒 Files selected for processing (3)
pr-review-toolkit/.claude-plugin/plugin.jsonpr-review-toolkit/skills/review-pr/SKILL.mdpr-review-toolkit/skills/review-pr/review-pr.js
cblecker
force-pushed
the
refactor/review-pr-delegate-diff-collection
branch
from
ac807bb to
aeae0cc
Compare
| const selected = Array.isArray(config.agents) | ||
| ? config.agents.filter(name => typeof PROMPTS[name] === 'string') | ||
| : ['code-reviewer'] |
Move diff fetching from the orchestrator into each workflow agent via a diffPreamble() that generates setup instructions. Agents now collect their own patches using git (local) or GitHub MCP (remote), eliminating the token cost of shuttling large diffs through the orchestration layer. Agent selection moves from regex matching in the workflow to the orchestrator, which can evaluate whether each specialist agent is warranted based on the nature of the changes. Assisted-by: Claude:claude-opus-4-6
cblecker
force-pushed
the
refactor/review-pr-delegate-diff-collection
branch
from
aeae0cc to
e79a9ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr-review-toolkit/skills/review-pr/review-pr.js`:
- Line 542: The prompt assembly is concatenating PROMPTS[name] before
diffPreamble(), but the role prompt expects setup instructions to appear above
it. Reverse the order of concatenation in the prompt assignment so that
diffPreamble() comes first, followed by PROMPTS[name], to match the expectation
referenced in the role prompt definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9829e394-648b-4792-885f-12f091499876
📒 Files selected for processing (3)
pr-review-toolkit/.claude-plugin/plugin.jsonpr-review-toolkit/skills/review-pr/SKILL.mdpr-review-toolkit/skills/review-pr/review-pr.js
✅ Files skipped from review due to trivial changes (1)
- pr-review-toolkit/.claude-plugin/plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- pr-review-toolkit/skills/review-pr/SKILL.md
|
|
||
| const results = await parallel(selected.map(name => () => { | ||
| const prompt = PROMPTS[name] + buildContext() | ||
| const prompt = PROMPTS[name] + '\n\n' + diffPreamble() |
There was a problem hiding this comment.
Place diffPreamble() before agent instructions.
At Line 542, prompt assembly appends setup instructions after the role prompt, but the role prompt (Line 88) explicitly references setup “above.” This ordering mismatch can reduce reliability of agent behavior.
Proposed fix
- const prompt = PROMPTS[name] + '\n\n' + diffPreamble()
+ const prompt = diffPreamble() + '\n\n' + PROMPTS[name]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const prompt = PROMPTS[name] + '\n\n' + diffPreamble() | |
| const prompt = diffPreamble() + '\n\n' + PROMPTS[name] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review-toolkit/skills/review-pr/review-pr.js` at line 542, The prompt
assembly is concatenating PROMPTS[name] before diffPreamble(), but the role
prompt expects setup instructions to appear above it. Reverse the order of
concatenation in the prompt assignment so that diffPreamble() comes first,
followed by PROMPTS[name], to match the expectation referenced in the role
prompt definition.
| 1. Run `git diff --name-status origin/<baseRef>...HEAD` to get the list of | ||
| changed files with statuses (cheap — filenames only, no patches). |
| 3. Run `git fetch origin <baseRef>` to ensure the merge-base is available | ||
| locally for agents. | ||
|
|
||
| Note: `base.sha` from the API is the tip of the base branch at PR creation |
Summary
diffPreamble()— local agents usegit diff, remote agents use GitHub MCPget_files/get_file_contents. Eliminates shuttling large diffs through the orchestration layer.selectAgents()in the workflow with intelligent selection by the orchestrator usinggit diff --name-statusand judgment about which specialist agents are warranted.baseRefandagentsinstead ofchangedFiles/excludedFileSummary— agents handle file filtering (vendor, generated) themselves.Test plan
/review-pron a local PR (whereisLocalis true) and verify agents collect diffs via git/review-pron a remote PR (whereisLocalis false) and verify agents collect diffs via GitHub MCPcode-revieweralways runs, optional agents selected based on file changesclaude plugin validate ./pr-review-toolkit— passesnode -c pr-review-toolkit/skills/review-pr/review-pr.js— no syntax errorsSummary by CodeRabbit