Merge pull request #2132 from dgageot/board/sub-agents-can-be-oci-reference-but-they-134a0dbe

Derive meaningful names for external sub-agents instead of using 'root'

Author: dgageot

agent-schema.json

@@ -157,14 +157,14 @@
         },
         "sub_agents": {
           "type": "array",
-          "description": "List of sub-agents. Can be names of agents defined in this config or external references (OCI images like 'namespace/repo' or URLs).",
+          "description": "List of sub-agents. Can be names of agents defined in this config, external references (OCI images like 'namespace/repo' or URLs), or named external references using 'name:reference' syntax (e.g. 'reviewer:agentcatalog/review-pr'). External agents without an explicit name are named after their last path segment.",
           "items": {
             "type": "string"
           }
         },
         "handoffs": {
           "type": "array",
-          "description": "List of agents this agent can hand off the conversation to. Can be names of agents defined in this config or external references (OCI images like 'namespace/repo' or URLs).",
+          "description": "List of agents this agent can hand off the conversation to. Can be names of agents defined in this config, external references (OCI images like 'namespace/repo' or URLs), or named external references using 'name:reference' syntax (e.g. 'reviewer:agentcatalog/review-pr'). External agents without an explicit name are named after their last path segment.",
           "items": {
             "type": "string"
           }

examples/sub-agents-from-catalog.yaml

@@ -3,6 +3,11 @@
 # This example demonstrates using agents from the catalog as sub-agents.
 # Sub-agents can be defined locally in the same config, or referenced from
 # external sources such as OCI registries (e.g., the Docker agent catalog).
+#
+# External sub-agents are automatically named after their last path segment
+# (e.g., "agentcatalog/pirate" becomes "pirate"). You can also give them
+# an explicit name using the "name:reference" syntax:
+#   - reviewer:agentcatalog/review-pr
 
 models:
   model:
@@ -17,7 +22,7 @@ agents:
       You are a coordinator agent. You have access to both local and external sub-agents.
 
       - Use the "local_helper" agent for simple tasks.
-      - Use the "agentcatalog/pirate" agent when users want responses in a pirate style.
+      - Use the "pirate" agent when users want responses in a pirate style.
 
       Delegate tasks to the most appropriate sub-agent based on the user's request.
     sub_agents:

pkg/config/config.go

@@ -130,15 +130,27 @@ func validateConfig(cfg *latest.Config) error {
 	}
 
 	for _, agent := range cfg.Agents {
-		for _, subAgentName := range agent.SubAgents {
-			if _, exists := allNames[subAgentName]; !exists && !IsExternalReference(subAgentName) {
-				return fmt.Errorf("agent '%s' references non-existent sub-agent '%s'", agent.Name, subAgentName)
+		for _, subAgentRef := range agent.SubAgents {
+			if _, exists := allNames[subAgentRef]; !exists && !IsExternalReference(subAgentRef) {
+				return fmt.Errorf("agent '%s' references non-existent sub-agent '%s'", agent.Name, subAgentRef)
+			}
+			if IsExternalReference(subAgentRef) {
+				name, _ := ParseExternalAgentRef(subAgentRef)
+				if allNames[name] {
+					return fmt.Errorf("agent '%s': external sub-agent '%s' resolves to name '%s' which conflicts with a locally-defined agent", agent.Name, subAgentRef, name)
+				}
 			}
 		}
 
-		for _, handoffName := range agent.Handoffs {
-			if _, exists := allNames[handoffName]; !exists && !IsExternalReference(handoffName) {
-				return fmt.Errorf("agent '%s' references non-existent handoff agent '%s'", agent.Name, handoffName)
+		for _, handoffRef := range agent.Handoffs {
+			if _, exists := allNames[handoffRef]; !exists && !IsExternalReference(handoffRef) {
+				return fmt.Errorf("agent '%s' references non-existent handoff agent '%s'", agent.Name, handoffRef)
+			}
+			if IsExternalReference(handoffRef) {
+				name, _ := ParseExternalAgentRef(handoffRef)
+				if allNames[name] {
+					return fmt.Errorf("agent '%s': external handoff '%s' resolves to name '%s' which conflicts with a locally-defined agent", agent.Name, handoffRef, name)
+				}
 			}
 		}
 

pkg/config/config_test.go

@@ -533,6 +533,60 @@ func TestValidateConfig_ExternalSubAgentReferences(t *testing.T) {
 			},
 			wantErr: "non-existent handoff agent 'does_not_exist'",
 		},
+		{
+			name: "named OCI reference in sub_agents is allowed",
+			cfg: &latest.Config{
+				Agents: []latest.AgentConfig{
+					{Name: "root", Model: "openai/gpt-4o", SubAgents: []string{"reviewer:agentcatalog/review-pr"}},
+				},
+			},
+		},
+		{
+			name: "named URL reference in sub_agents is allowed",
+			cfg: &latest.Config{
+				Agents: []latest.AgentConfig{
+					{Name: "root", Model: "openai/gpt-4o", SubAgents: []string{"myagent:https://example.com/agent.yaml"}},
+				},
+			},
+		},
+		{
+			name: "named OCI reference in handoffs is allowed",
+			cfg: &latest.Config{
+				Agents: []latest.AgentConfig{
+					{Name: "root", Model: "openai/gpt-4o", Handoffs: []string{"reviewer:agentcatalog/review-pr"}},
+				},
+			},
+		},
+		{
+			name: "external sub-agent name collides with local agent",
+			cfg: &latest.Config{
+				Agents: []latest.AgentConfig{
+					{Name: "root", Model: "openai/gpt-4o", SubAgents: []string{"pirate", "agentcatalog/pirate"}},
+					{Name: "pirate", Model: "openai/gpt-4o"},
+				},
+			},
+			wantErr: "conflicts with a locally-defined agent",
+		},
+		{
+			name: "named external sub-agent collides with local agent",
+			cfg: &latest.Config{
+				Agents: []latest.AgentConfig{
+					{Name: "root", Model: "openai/gpt-4o", SubAgents: []string{"helper", "helper:agentcatalog/review-pr"}},
+					{Name: "helper", Model: "openai/gpt-4o"},
+				},
+			},
+			wantErr: "conflicts with a locally-defined agent",
+		},
+		{
+			name: "external handoff name collides with local agent",
+			cfg: &latest.Config{
+				Agents: []latest.AgentConfig{
+					{Name: "root", Model: "openai/gpt-4o", Handoffs: []string{"agentcatalog/pirate"}},
+					{Name: "pirate", Model: "openai/gpt-4o"},
+				},
+			},
+			wantErr: "conflicts with a locally-defined agent",
+		},
 		{
 			name: "local handoff to another agent passes",
 			cfg: &latest.Config{

pkg/config/resolve.go

@@ -216,6 +216,83 @@ func fileNameWithoutExt(path string) string {
 // (OCI image or URL) rather than a local agent name defined in the same config.
 // Local agent names never contain "/", so the slash check distinguishes them
 // from OCI references like "agentcatalog/pirate" or "docker.io/org/agent:v1".
+// It also handles the "name:ref" syntax (e.g. "reviewer:agentcatalog/review-pr").
 func IsExternalReference(input string) bool {
+	_, ref := ParseExternalAgentRef(input)
+	return isExternalRef(ref)
+}
+
+// ParseExternalAgentRef parses an external agent reference that may include an
+// explicit name prefix. The syntax is "name:reference" where name is a simple
+// identifier (no slashes) and reference is an OCI reference or URL.
+//
+// If no explicit name is provided, the base name is derived from the reference:
+//   - OCI refs: last path segment without tag (e.g. "agentcatalog/review-pr" → "review-pr")
+//   - URLs: filename without extension (e.g. "https://example.com/agent.yaml" → "agent")
+//
+// Examples:
+//
+//	ParseExternalAgentRef("reviewer:agentcatalog/review-pr") → ("reviewer", "agentcatalog/review-pr")
+//	ParseExternalAgentRef("agentcatalog/review-pr") → ("review-pr", "agentcatalog/review-pr")
+//	ParseExternalAgentRef("docker.io/myorg/myagent:v1") → ("myagent", "docker.io/myorg/myagent:v1")
+//	ParseExternalAgentRef("https://example.com/agent.yaml") → ("agent", "https://example.com/agent.yaml")
+func ParseExternalAgentRef(input string) (agentName, ref string) {
+	// If the whole input is already a valid external reference, derive the name
+	// from it without trying to split on ":".
+	if isExternalRef(input) {
+		return externalRefBaseName(input), input
+	}
+
+	// Check for explicit "name:reference" syntax.
+	// A name prefix is identified by not containing "/" (distinguishing it from
+	// OCI references or URLs which always contain slashes).
+	if i := strings.Index(input, ":"); i > 0 {
+		candidate := input[:i]
+		if !strings.Contains(candidate, "/") {
+			remainder := input[i+1:]
+			if isExternalRef(remainder) {
+				return candidate, remainder
+			}
+		}
+	}
+
+	// Fallback: return input as both name and ref (for local agent names).
+	return input, input
+}
+
+// isExternalRef is the core check for whether a string is an external reference.
+// It is used by both IsExternalReference and ParseExternalAgentRef to avoid
+// circular dependencies.
+func isExternalRef(input string) bool {
 	return IsURLReference(input) || (strings.Contains(input, "/") && IsOCIReference(input))
 }
+
+// externalRefBaseName extracts a short agent name from an external reference.
+//
+//   - OCI: last path segment, tag/digest stripped
+//     "agentcatalog/review-pr" → "review-pr"
+//     "docker.io/myorg/myagent:v1" → "myagent"
+//
+//   - URL: filename without extension
+//     "https://example.com/agent.yaml" → "agent"
+func externalRefBaseName(ref string) string {
+	if IsURLReference(ref) {
+		return fileNameWithoutExt(ref)
+	}
+
+	// OCI reference: strip tag or digest, then take last path segment.
+	base := ref
+	if i := strings.LastIndex(base, "@"); i >= 0 {
+		base = base[:i]
+	}
+	if i := strings.LastIndex(base, ":"); i >= 0 {
+		// Only strip if the colon is after the last slash (i.e. it's a tag, not a port).
+		if j := strings.LastIndex(base, "/"); j < i {
+			base = base[:i]
+		}
+	}
+	if i := strings.LastIndex(base, "/"); i >= 0 {
+		base = base[i+1:]
+	}
+	return base
+}

pkg/config/resolve_test.go

@@ -665,6 +665,16 @@ func TestIsExternalReference(t *testing.T) {
 			input:    "",
 			expected: false,
 		},
+		{
+			name:     "named OCI reference is external",
+			input:    "reviewer:agentcatalog/review-pr",
+			expected: true,
+		},
+		{
+			name:     "named URL reference is external",
+			input:    "myagent:https://example.com/agent.yaml",
+			expected: true,
+		},
 	}
 
 	for _, tt := range tests {
@@ -676,3 +686,85 @@ func TestIsExternalReference(t *testing.T) {
 		})
 	}
 }
+
+func TestParseExternalAgentRef(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name         string
+		input        string
+		expectedName string
+		expectedRef  string
+	}{
+		{
+			name:         "simple OCI reference derives base name",
+			input:        "agentcatalog/pirate",
+			expectedName: "pirate",
+			expectedRef:  "agentcatalog/pirate",
+		},
+		{
+			name:         "OCI reference with tag derives base name without tag",
+			input:        "docker.io/myorg/myagent:v1",
+			expectedName: "myagent",
+			expectedRef:  "docker.io/myorg/myagent:v1",
+		},
+		{
+			name:         "OCI reference with digest derives base name",
+			input:        "docker.io/myorg/myagent@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			expectedName: "myagent",
+			expectedRef:  "docker.io/myorg/myagent@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+		},
+		{
+			name:         "explicit name prefix",
+			input:        "reviewer:agentcatalog/review-pr",
+			expectedName: "reviewer",
+			expectedRef:  "agentcatalog/review-pr",
+		},
+		{
+			name:         "explicit name with tagged OCI ref",
+			input:        "myreviewer:docker.io/myorg/review-pr:v2",
+			expectedName: "myreviewer",
+			expectedRef:  "docker.io/myorg/review-pr:v2",
+		},
+		{
+			name:         "URL reference derives filename",
+			input:        "https://example.com/agent.yaml",
+			expectedName: "agent",
+			expectedRef:  "https://example.com/agent.yaml",
+		},
+		{
+			name:         "named URL reference",
+			input:        "myagent:https://example.com/agent.yaml",
+			expectedName: "myagent",
+			expectedRef:  "https://example.com/agent.yaml",
+		},
+		{
+			name:         "simple name without slash is not split",
+			input:        "my_agent",
+			expectedName: "my_agent",
+			expectedRef:  "my_agent",
+		},
+		{
+			name:         "OCI ref with registry port is not confused with name prefix",
+			input:        "localhost:5000/test/agent",
+			expectedName: "agent",
+			expectedRef:  "localhost:5000/test/agent",
+		},
+		{
+			name:         "deeply nested OCI path",
+			input:        "registry.example.com/org/sub/agent:latest",
+			expectedName: "agent",
+			expectedRef:  "registry.example.com/org/sub/agent:latest",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			name, ref := ParseExternalAgentRef(tt.input)
+			assert.Equal(t, tt.expectedName, name)
+			assert.Equal(t, tt.expectedRef, ref)
+		})
+	}
+}

pkg/teamloader/teamloader.go

@@ -509,6 +509,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,
@@ -536,12 +538,25 @@ func resolveAgentRefs(
 			continue
 		}
 
-		a, err := loadExternalAgent(ctx, ref, runConfig, loadOpts)
+		agentName, externalRef := config.ParseExternalAgentRef(ref)
+
+		// 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
+		agentsByName[agentName] = a
 		resolved = append(resolved, a)
 	}
 	return resolved, nil