-
Notifications
You must be signed in to change notification settings - Fork 305
Derive meaningful names for external sub-agents instead of using 'root' #2132
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -511,6 +511,8 @@ func configNameFromSource(sourceName string) string { | |
| // References that match a locally-defined agent name are looked up directly. | ||
| // References that are external (OCI or URL) are loaded on-demand and cached | ||
| // in externalAgents so the same reference isn't loaded twice. | ||
| // External references may include an explicit name prefix ("name:ref") or | ||
| // derive a short name from the reference (e.g. "agentcatalog/review-pr" → "review-pr"). | ||
| func resolveAgentRefs( | ||
| ctx context.Context, | ||
| refs []string, | ||
|
|
@@ -538,12 +540,25 @@ func resolveAgentRefs( | |
| continue | ||
| } | ||
|
|
||
| a, err := loadExternalAgent(ctx, ref, runConfig, loadOpts) | ||
| agentName, externalRef := config.ParseExternalAgentRef(ref) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM: External agent caching uses wrong key The cache check at line 534 uses the original reference string ( For example:
Both references normalize to Recommendation: Use // At line 534:
if a, ok := externalAgents[externalRef]; ok {
resolved = append(resolved, a)
continue
}
// At line 560:
externalAgents[externalRef] = a |
||
|
|
||
| // Check for name collisions before loading the external agent. | ||
| if existing, ok := agentsByName[agentName]; ok { | ||
| return nil, fmt.Errorf("external agent %q resolves to name %q which conflicts with agent %q", ref, agentName, existing.Name()) | ||
| } | ||
|
|
||
| a, err := loadExternalAgent(ctx, externalRef, runConfig, loadOpts) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("loading %q: %w", ref, err) | ||
| return nil, fmt.Errorf("loading %q: %w", externalRef, err) | ||
| } | ||
|
|
||
| // Rename the external agent so it doesn't collide with locally-defined | ||
| // agents (external agents typically have the name "root"). | ||
| agent.WithName(agentName)(a) | ||
|
|
||
| *agents = append(*agents, a) | ||
| externalAgents[ref] = a | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM: External agent caching stores with wrong key The cache stores the loaded agent using the original reference string ( This means Recommendation: Store using externalAgents[externalRef] = a |
||
| agentsByName[agentName] = a | ||
| resolved = append(resolved, a) | ||
| } | ||
| return resolved, nil | ||
|
|
||
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.
🟡 MEDIUM: Collision detection incomplete
The validation checks for collisions between external agents and locally-defined agents, but doesn't detect when multiple external references derive the same name.
For example, if a config has both
org1/helperandorg2/helperas sub-agents, both would derive the namehelper, but this collision wouldn't be caught during validation. The collision would only be detected at runtime inresolveAgentRefs.Recommendation: Track external agent names in
validateConfigto detect these collisions early: