-
Notifications
You must be signed in to change notification settings - Fork 5
feat(multi-agent-review): spin sub-agents to review specific aspects #64
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: master
Are you sure you want to change the base?
Conversation
|
Claude finished @Cali93's task in 1m 53s —— View job Code Review
Found 3 issue(s)Issue 1: Agent specification files are unusedID: pat-action-unused-agent-files-a1b2
The current implementation creates maintenance burden - agent instructions exist in two places and can drift out of sync. Issue 2: Missing instruction to read agent specification filesID: pat-action-missing-file-refs-c3d4 **For EACH agent prompt, include:**
- Read the agent specification from claude/auto-review/agents/review-{type}.md
- This preamble: \"You are a code reviewer...\"Or document that the .md files are reference material only, not runtime specifications. Issue 3: Test fixture doesn't match actual multi-agent output formatID: bug-multi-agent-fixture-mismatch-8b91 SummaryMulti-agent architecture implementation with agent-specific ID prefixes and parsing logic. Main concerns:
Recommend clarifying whether agent .md files are runtime specs or documentation, and aligning implementation accordingly. |
claude/auto-review/scripts/__tests__/fixtures/claude-comments/multi-agent-output.md
Outdated
Show resolved
Hide resolved
|
FYI - think you will need to rebase due to this base prompt compression I just got over the line, apologies 🙏 #56 |
Resolved conflicts in action.yml by: - Keeping multi-agent review approach - Adopting new condensed Context format (Pattern/Risk/Impact/Trigger) - Updated agent spec files to use new format - Updated test fixtures to match new format Co-Authored-By: Claude Opus 4.5 <[email protected]>
| --- | ||
| ## AUTOMATED CHECKS |
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.
I see this was moved into review-patterns, but let's ensure these checks are preserved in the main prompt if agents only trigger on heuristics like PR size? 🙏 We need them to happen for sure on every PR.
|
|
||
| You are a code reviewer. Provide actionable feedback on code changes. | ||
|
|
||
| **Diffs alone are not enough.** Read the full file(s) being modified to understand context. Code that looks wrong in isolation may be correct given surrounding logic. |
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.
We already have this in the main prompt now based on your prior advice: https://github.com/WalletConnect/actions/pull/64/changes#diff-742c4c628098679724a584600295de6d9bba6d01b7d7b5c0cb60f03ce427288fR61
claude/auto-review/action.yml
Outdated
| 2. PR number, repository, list of changed files | ||
| **Agents:** | ||
| 1. **Bug Agent** - Spec: ${{ github.action_path }}/agents/review-bugs.md - ID prefix: bug- |
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.
It already has lots of explicit guidance on how to generate the IDs for inline commenting deduplication: https://github.com/WalletConnect/actions/pull/64/changes#diff-742c4c628098679724a584600295de6d9bba6d01b7d7b5c0cb60f03ce427288fR142
I don't think we need the ID prefix: specifications here and it doesn't seem to follow them anyways based on what I'm seeing in https://github.com/WalletConnect/gha-playground/pull/12#issuecomment-3774813787
| @@ -0,0 +1,108 @@ | |||
| # Security Review 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.
Maybe we retire the usage of https://github.com/anthropics/claude-code-security-review/ in favour of this long term and use Opus for this agent, somewhat duplicative if we have both in some repos.
- Use consistent ID template placeholders ({filename}-{2-4-key-terms}-{hash})
- Change severity/category separator from | to / to match main prompt
- Rename "ID Format" to "ID Generation" for consistency
- Update recommendation wording to match main action.yml
- Add CRITICAL severity to patterns agent for domain-specific checks
- Remove redundant details block instruction (handled by orchestrator)
Implement heuristic to selectively spawn review agents based on PR characteristics, avoiding unnecessary agent invocation for simple PRs. Key changes: - Add determine-agents.js with file categorization and keyword analysis - Skip review entirely for docs-only, rename-only, and empty PRs - Spawn security agent for workflow/auth/SQL/infra/deps files or keywords - Spawn patterns agent for large files (>300 lines) or >5 code files - Always spawn all agents for large PRs (>500 lines or >15 files) - Add force_all_agents input and full-review/skip-review label overrides - Add comprehensive test suite (47 tests) Heuristic behavior: - Docs/rename/empty PR: Skip review entirely - Lockfile-only: Security agent only - Test-only: Bug agent only - Large PR: All 3 agents - Small code PR: Bug agent only (+ security/patterns if triggered)
Summary
Changes
<details>list with severity ordering.parseClaudeCommentto mapbug-,sec-,pat-ID prefixes to agent names.Taskfor parallel sub‑agent execution.Notes
Test run example here https://github.com/WalletConnect/gha-playground/pull/3#issuecomment-3772901080