Harden pr-review-toolkit workflow agents#55
Conversation
Split workflow agent policies between strict GitHub collection and read-only analysis, add adaptive file-page retries, and document the updated flow. Assisted-by: Codex:gpt-5
📝 WalkthroughWalkthroughTwo new agent configuration files ( ChangesAgent Boundaries and Manifest Collection Reliability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 1584: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agents/pr-review-analysis-readonly.md`:
- Around line 4-15: The analysis agent config is still relying on a denylist, so
new mutating tools could slip through unnoticed. Update the readonly agent
definition to use an explicit tools allowlist matching the collector config, and
keep the existing disallowedTools block only as backup defense; use the analysis
agent config symbols where the tool policy is defined.
In `@pr-review-toolkit/skills/review-pr/review-pr.js`:
- Around line 1221-1229: The retry path in filePagesFor/collectFilePages
currently falls back to perPage=1 and can spawn one high-effort collector per
changed file, which is too expensive for large PRs. Update the retry logic
around filePagesFor, collectFilePages, and collectFilePage so the last attempt
uses bounded concurrency and/or a lower effort level, and add a threshold that
prevents switching to perPage=1 when the page count is large. Keep the recovery
behavior, but cap the number of parallel collectors created from filePages to
avoid exhausting workflow/runtime limits.
🪄 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: 53be47b1-8e39-4e18-b93b-73110eba3e47
📒 Files selected for processing (5)
pr-review-toolkit/.claude-plugin/plugin.jsonpr-review-toolkit/README.mdpr-review-toolkit/agents/pr-review-analysis-readonly.mdpr-review-toolkit/agents/pr-review-github-collector.mdpr-review-toolkit/skills/review-pr/review-pr.js
| disallowedTools: | ||
| - Bash | ||
| - Write | ||
| - Edit | ||
| - MultiEdit | ||
| - NotebookEdit | ||
| - Task | ||
| - WebFetch | ||
| - WebSearch | ||
| - mcp__plugin_github_github__pull_request_review_write | ||
| - mcp__plugin_github_github__add_comment_to_pending_review | ||
| --- |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Use an explicit allowlist for the analysis agent.
This boundary is still open-ended because it relies on disallowedTools only. If the runtime/plugin adds another mutating tool later, this agent silently gains it until someone updates the blocklist. That undercuts the hardening this PR is trying to add. Mirror the collector config with an explicit read-only tools: allowlist and keep disallowedTools as defense in depth if you want.
🤖 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/agents/pr-review-analysis-readonly.md` around lines 4 - 15,
The analysis agent config is still relying on a denylist, so new mutating tools
could slip through unnoticed. Update the readonly agent definition to use an
explicit tools allowlist matching the collector config, and keep the existing
disallowedTools block only as backup defense; use the analysis agent config
symbols where the tool policy is defined.
| async function collectFilePages(pr, filePages, perPage, effort, labelSuffix) { | ||
| return parallel(filePages.map(page => () => collectFilePage(pr, page, perPage, effort, labelSuffix))) | ||
| } | ||
|
|
||
| function filePagesFor(total, perPage) { | ||
| const pageCount = Math.max(1, Math.ceil((total || 1) / perPage)) | ||
| const pages = [] | ||
| for (let page = 1; page <= pageCount; page++) pages.push(page) | ||
| return { pageCount: pageCount, pages: pages } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cap the perPage=1 fallback before it turns into one high-effort agent per file.
On the last retry, filePagesFor(pr.changedFiles, 1) makes collectFilePages spawn one collector agent for every changed file. For a large PR, that means dozens or hundreds of extra high-effort runs on the failure path, which is likely to hit workflow/runtime limits instead of recovering. Batch these retries with bounded concurrency, lower the retry effort, or stop before perPage=1 once the page count crosses a threshold.
Also applies to: 1251-1274
🤖 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` around lines 1221 - 1229,
The retry path in filePagesFor/collectFilePages currently falls back to
perPage=1 and can spawn one high-effort collector per changed file, which is too
expensive for large PRs. Update the retry logic around filePagesFor,
collectFilePages, and collectFilePage so the last attempt uses bounded
concurrency and/or a lower effort level, and add a threshold that prevents
switching to perPage=1 when the page count is large. Keep the recovery behavior,
but cap the number of parallel collectors created from filePages to avoid
exhausting workflow/runtime limits.
There was a problem hiding this comment.
Pull request overview
This PR hardens the pr-review-toolkit workflow by giving the agents that the bundled review-pr.js workflow spawns explicit, phase-specific tool boundaries, and by making the changed-file collection more resilient. Because workflow agent() calls cannot pass per-call tool allowlists, the change introduces two dedicated plugin agent types: a strict GitHub-only collector (used for PR metadata, file pages, and review threads) and a read-only analysis agent (used for specialist reviews and synthesis). It also adds adaptive retries that re-collect changed files with progressively smaller page sizes and report duplicate-page diagnostics, and bumps the plugin to 1.6.0.
Changes:
- Add
pr-review-github-collector(allowlisted topull_request_read) andpr-review-analysis-readonlyagent types and route all spawned agents through them viaagentType. - Refactor file collection into
collectCompleteFileManifestwithperPageretries ([5, 1]), per-page validation threaded withperPage, and richer duplicate-path error diagnostics. - Document the control flow / agent boundaries in the README and bump the version to
1.6.0.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pr-review-toolkit/skills/review-pr/review-pr.js | Adds agent-type constants, retry-based file manifest collection, duplicate-page diagnostics, and threads perPage through validation/merge; routes collect/analyze/synthesize agents to the new agent types and bumps thread-collection effort. |
| pr-review-toolkit/agents/pr-review-github-collector.md | New GitHub-only collector agent with a strict tools allowlist and a denylist for shell/local/web tools. |
| pr-review-toolkit/agents/pr-review-analysis-readonly.md | New read-only analysis agent denying shell/edit/web and two GitHub write tools while allowing repo and read-only MCP inspection. |
| pr-review-toolkit/README.md | Documents the control-flow diagram and the responsibilities of the two new agent types. |
| pr-review-toolkit/.claude-plugin/plugin.json | Version bump 1.5.0 → 1.6.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const STANDARDIZATION_SUFFIX = `Return only high-signal candidate findings. For each finding, provide a concise title, a concrete claim, structured evidence, specialist reasoning, why it matters, and a specific suggested fix when applicable. Include a postability recommendation for human review only: recommended_to_post, possible_plus_one, partial_overlap, discussion_only, already_covered, or discard. Preserve concrete evidence from patches, files, and existing threads; do not collapse reasoning into generic summaries. Use a neutral technical voice and do not reference yourself, your role, or your review methodology.` | ||
|
|
||
| const FILE_PAGE_SIZE = 10 | ||
| const FILE_RETRY_PAGE_SIZES = [5, 1] |
| disallowedTools: | ||
| - Bash | ||
| - Write | ||
| - Edit | ||
| - MultiEdit | ||
| - NotebookEdit | ||
| - Task | ||
| - WebFetch | ||
| - WebSearch | ||
| - mcp__plugin_github_github__pull_request_review_write | ||
| - mcp__plugin_github_github__add_comment_to_pending_review |
Summary
Validation
claude plugin validate ./pr-review-toolkitclaude plugin validate .uvx skillsaw --strictnpx markdownlint-cli2 --config /opt/gopath/src/github.com/cblecker/claude-plugins/.markdownlint-cli2.jsonc "pr-review-toolkit/**/*.md"Summary by CodeRabbit
New Features
Bug Fixes
Documentation