feat(pr-review-toolkit): add specialists, improve signals, optimize collection#73
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThis PR bumps the plugin version, updates review-pr skill instructions, removes the required postability field from findings, adds new specialist reviewers and signal detection, and restructures thread collection, context passing, and synthesis logic. ChangesReview Toolkit Workflow Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant signalsForFile
participant enrichSignalsFromDiff
participant selectReviewers
participant Specialists
Orchestrator->>signalsForFile: classify changed files
Orchestrator->>enrichSignalsFromDiff: scan added diff hunks
enrichSignalsFromDiff-->>Orchestrator: updated signals
Orchestrator->>selectReviewers: pass signals
selectReviewers->>Specialists: enable matching reviewers
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
pr-review-toolkit/skills/review-pr/review-pr.js (1)
984-985: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSubstring
indexOfmatching can over-trigger high-effort specialists.These are naive substring checks, so
'lock'matchesblock/blockchain/adblock,'sync'matchesasync,'cert'matchesconcert/certainty, and'pool'/'ssl'similarly. Sinceconcurrency-reviewer/security-reviewerrun witheffort: 'high'(Lines 602/612), spurious matches select expensive agents for unrelated files. Selection is intentionally liberal, but the short/ambiguous tokens above are the main cost drivers and could be tightened (e.g. delimiter-aware matching).🤖 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 984 - 985, The `signals` detection in `review-pr.js` is over-matching on short substrings, which can incorrectly trigger `security` and `concurrency` specialists for unrelated paths. Update the `p.indexOf(...)` checks in the selection logic to use tighter matching for ambiguous tokens like `lock`, `sync`, `cert`, `pool`, and `ssl` (for example, delimiter-aware or whole-segment matching) while keeping the existing broad intent. Make sure the logic around the `signals.push('security')` and `signals.push('concurrency')` branches preserves true positives but avoids common false positives.
🤖 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 1005-1011: The regex patterns in SIGNAL_PATTERNS for types and
concurrency are over-anchored, so normal cases like `@dataclass`, mu.Lock(), and
mu.RLock() are missed. Update the matching logic in review-pr.js by adjusting
the boundary handling around the alternatives in these two patterns so non-word
tokens and method-style calls can match reliably, while preserving the existing
detections for class/interface/enum and lock/async/thread terms.
---
Nitpick comments:
In `@pr-review-toolkit/skills/review-pr/review-pr.js`:
- Around line 984-985: The `signals` detection in `review-pr.js` is
over-matching on short substrings, which can incorrectly trigger `security` and
`concurrency` specialists for unrelated paths. Update the `p.indexOf(...)`
checks in the selection logic to use tighter matching for ambiguous tokens like
`lock`, `sync`, `cert`, `pool`, and `ssl` (for example, delimiter-aware or
whole-segment matching) while keeping the existing broad intent. Make sure the
logic around the `signals.push('security')` and `signals.push('concurrency')`
branches preserves true positives but avoids common false positives.
🪄 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: 5ed2ba95-4713-4494-bc70-5a6f932dc2ea
📒 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
There was a problem hiding this comment.
Pull request overview
This PR extends the pr-review-toolkit plugin's review-pr workflow with three new specialist reviewers (security, API compatibility, concurrency), adds diff-content-based signal detection so specialists trigger for generically-named files, moves all thread/overlap classification to the synthesis step, parallelizes thread and file collection, and increases the file page size. It also trims verbose/duplicative sections from the existing reviewer prompts and adds an ExitPlanMode step to the skill.
Changes:
- Added
security-reviewer,api-compat-reviewer, andconcurrency-reviewerplussecurity/concurrencysignals and aenrichSignalsFromDiffdiff-content scanner. - Removed the per-finding
postabilityobject fromFINDING_SCHEMA/prompts so synthesis owns overlap classification; parallelized thread collection and raisedFILE_PAGE_SIZE10→30. - Trimmed redundant output-format/rating sections from reviewer prompts, added ExitPlanMode guidance to SKILL.md, and bumped the version to 1.10.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pr-review-toolkit/skills/review-pr/review-pr.js | Adds specialists, diff-content signal enrichment, parallel collection, schema/prompt trimming, larger page size |
| pr-review-toolkit/skills/review-pr/SKILL.md | Adds ExitPlanMode to allowed-tools and an "Exit Plan Mode" step |
| pr-review-toolkit/.claude-plugin/plugin.json | Version bump 1.9.0 → 1.10.0 |
Key review notes:
- The new
concurrencydiff-scan pattern'sLock()/RLock()alternatives (and thetypespattern's@dataclass) can never match due to the trailing/leading\baround non-word characters, undercutting the generically-named-file detection this PR is meant to add. rebuildSummarySignalsis redundant becauseenrichSignalsFromDiffruns beforebuildSummary, which already recomputes signal counts.- The README "Review Agents" table was not updated to document the three new specialists.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cblecker
force-pushed
the
feat/pr-review-toolkit-improvements
branch
from
b5d34e5 to
852f9ef
Compare
cblecker
force-pushed
the
feat/pr-review-toolkit-improvements
branch
from
852f9ef to
43a0165
Compare
…ollection - Add security, API-compat, and concurrency specialist reviewers - Upgrade signal detection with diff-content scanning alongside path-based heuristics for more reliable specialist triggering - Move thread data and overlap classification out of specialists into synthesis only, removing postability from FINDING_SCHEMA - Parallelize thread and file collection for faster wall-clock time - Increase file page size from 10 to 30 to reduce collection agents - Trim redundant output format sections from specialist prompts while preserving all substantive analytical guidance - Add ExitPlanMode to SKILL.md to avoid permission prompts - Bump version to 1.10.0 Assisted-by: Claude:claude-opus-4-6
cblecker
force-pushed
the
feat/pr-review-toolkit-improvements
branch
from
43a0165 to
cd404ea
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
securityandconcurrencysignals plus the existingpublic-apisignalmutex,token,catch), so specialists trigger even when filenames are genericFILE_PAGE_SIZEfrom 10 to 30, reducing collection agent count by ~3xTest plan
/pr-review-toolkit:review-pragainst a PR with diverse file types to validate the full workflow completesclaude plugin validate ./pr-review-toolkitSummary by CodeRabbit