fix: include user/project agents in task(subagent_type) resolution#2974
fix: include user/project agents in task(subagent_type) resolution#2974trafgals wants to merge 4 commits intocode-yeongyu:devfrom
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- There is a concrete behavior risk in
src/tools/delegate-task/subagent-resolver.ts: same-named project agents are not overriding user agents, sotask(subagent_type=...)can resolve the wrong target in shadowing repositories. - Given the 6/10 severity with high confidence (9/10), this is more than a cosmetic issue and could cause user-visible misrouting of delegated tasks, so merge risk is moderate rather than minimal.
- Pay close attention to
src/tools/delegate-task/subagent-resolver.ts- merge-order precedence needs to ensure project agents win over same-named user agents.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/delegate-task/subagent-resolver.ts">
<violation number="1" location="src/tools/delegate-task/subagent-resolver.ts:95">
P2: Project agents should override same-named user agents here; the current merge order makes `task(subagent_type=...)` resolve the wrong agent in repositories that shadow a global agent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/delegate-task/subagent-resolver.test.ts">
<violation number="1" location="src/tools/delegate-task/subagent-resolver.test.ts:652">
P2: This test doesn’t actually verify project-over-user precedence. Both candidates are named `my-custom-agent`, and the only assertion checks the shared name, so the test will pass even if the resolver picks the user agent. Add an assertion on a distinguishing field (like the resolved model) to make the precedence check meaningful.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
I have read the CLA Document and I hereby sign the CLA |
Re-implement PR code-yeongyu#2690 against current upstream/dev since original branch became incompatible with codebase refactoring. Changes: - Import loadUserAgents and loadProjectAgents from claude-code-agent-loader - Merge user/project agents with server agents in resolveSubagentExecution() - Server agents take precedence on name collisions - Primary-mode agents from user/project are filtered out Tests: 4 new test cases covering user agent resolution, project agent resolution, server precedence, and primary agent filtering.
Address PR review comment: project agents should take precedence over user agents when both define the same agent name.
Previous fix only changed comments, not logic. Now project agents are added to the map before user agents, so project takes precedence.
851469b to
6440882
Compare
|
I have read the CLA Document and I hereby sign the CLA |
code-yeongyu
left a comment
There was a problem hiding this comment.
QA Results
Tests: 18 pass, 0 fail (bun test src/tools/delegate-task/subagent-resolver.test.ts)
All 18 tests pass including the 4 new test cases:
- User agent resolution from
loadUserAgents() - Project agent resolution from
loadProjectAgents() - Server agent precedence over user/project agents with same name
- Primary-mode agents correctly filtered out
Code review notes:
- The
bun.lockdiff bumps all platform binaries from 3.11.0 → 3.14.0 which is noisy but harmless (rebased on newer dev) - Agent merging logic with proper precedence (server > project > user) is clean
- Case-insensitive matching via
.toLowerCase()is a nice touch
One minor concern: this PR uses vi.mock (vitest syntax) in bun:test. It works in bun but isn't idiomatic — the rest of the test suite uses spyOn. Not a blocker.
Use spyOn which is more idiomatic for bun:test, as suggested in PR review.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Logic for agent merging is sound, but the bun.lock update includes binary dependency bumps (3.11 to 3.14) which cannot be guaranteed 100% regression-free without further context.
Summary
Re-implements the functionality from closed PR #2690 to make user-defined and project-defined agents callable via
task(subagent_type=...).Problem
The original PR #2690 attempted to allow user agents (from
~/.config/opencode/agents/) and project agents (from.claude/agents/) to be callable viatask(subagent_type="my-agent"). However, the branch became incompatible withdevdue to ongoing refactoring while the PR was open.Solution
This PR re-implements the feature against current
upstream/dev:loadUserAgentsandloadProjectAgentsfromclaude-code-agent-loaderresolveSubagentExecution()Changes
src/tools/delegate-task/subagent-resolver.ts: Added agent merging logicsrc/tools/delegate-task/subagent-resolver.test.ts: Added 4 new test casesTest Results
All 17 tests pass (13 existing + 4 new):
loadUserAgents()loadProjectAgents()Summary by cubic
Enable task(subagent_type=...) to resolve user and project agents. We merge user (
~/.config/opencode/agents/) and project (.claude/agents/) agents with server agents; server wins, project overrides user; ignoreprimaryagents.Bug Fixes
loadUserAgents/loadProjectAgentsinresolveSubagentExecution(); enforce server > project > user; filterprimary; resolve models.Refactors
spyOnfor agent loader mocks instead ofvi.mock(bun:test best practice).Written for commit 5441c84. Summary will update on new commits.