Skip to content

Commit 9efad1d

Browse files
author
Motta Kin
committed
fix(subagents): restrict delegation to configured profiles
Avoid advertising delegation without callable subagents and prevent skills or invented profile names from being used as delegation targets.
1 parent 0b745c0 commit 9efad1d

6 files changed

Lines changed: 171 additions & 15 deletions

File tree

internal/cli/repl/tooling/tool_registry.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,5 +69,14 @@ func SetupToolRegistry(
6969
GetSkillsCatalog: appState.SkillsCatalog,
7070
Activity: activity,
7171
}
72-
appState.RegisterTool(tools.NewDelegateTool(runner))
72+
profiles := appState.GetSubagents().Profiles
73+
agentNames := make([]string, 0, len(profiles))
74+
for _, profile := range profiles {
75+
if !profile.Hidden {
76+
agentNames = append(agentNames, profile.Name)
77+
}
78+
}
79+
if len(agentNames) > 0 {
80+
appState.RegisterTool(tools.NewDelegateTool(runner, agentNames))
81+
}
7382
}
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package tooling
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"reflect"
7+
"testing"
8+
9+
replappstate "github.com/user/keen-code/internal/cli/repl/appstate"
10+
replpermissions "github.com/user/keen-code/internal/cli/repl/permissions"
11+
"github.com/user/keen-code/internal/config"
12+
)
13+
14+
func TestSetupToolRegistryOmitsDelegateToolWithoutProfiles(t *testing.T) {
15+
t.Setenv("HOME", t.TempDir())
16+
workingDir := t.TempDir()
17+
state := replappstate.New(nil, workingDir)
18+
19+
SetupToolRegistry(
20+
workingDir,
21+
state,
22+
replpermissions.NewAutoApproveRequester(),
23+
NewDiffEmitter(),
24+
nil,
25+
&config.ResolvedConfig{Model: "model"},
26+
config.DefaultGlobalConfig(),
27+
nil,
28+
)
29+
30+
if _, ok := state.GetToolRegistry().Get("delegate_task"); ok {
31+
t.Fatal("delegate_task should not be registered without subagent profiles")
32+
}
33+
}
34+
35+
func TestSetupToolRegistryRegistersDelegateToolForVisibleProfiles(t *testing.T) {
36+
t.Setenv("HOME", t.TempDir())
37+
workingDir := t.TempDir()
38+
agentsDir := filepath.Join(workingDir, ".agents", "agents")
39+
if err := os.MkdirAll(agentsDir, 0o755); err != nil {
40+
t.Fatalf("create agents directory: %v", err)
41+
}
42+
if err := os.WriteFile(filepath.Join(agentsDir, "worker.md"), []byte(`---
43+
name: worker
44+
description: Handles focused work.
45+
---
46+
`), 0o644); err != nil {
47+
t.Fatalf("write visible profile: %v", err)
48+
}
49+
if err := os.WriteFile(filepath.Join(agentsDir, "hidden.md"), []byte(`---
50+
name: hidden
51+
description: Hidden work.
52+
hidden: true
53+
---
54+
`), 0o644); err != nil {
55+
t.Fatalf("write hidden profile: %v", err)
56+
}
57+
58+
state := replappstate.New(nil, workingDir)
59+
SetupToolRegistry(
60+
workingDir,
61+
state,
62+
replpermissions.NewAutoApproveRequester(),
63+
NewDiffEmitter(),
64+
nil,
65+
&config.ResolvedConfig{Model: "model"},
66+
config.DefaultGlobalConfig(),
67+
nil,
68+
)
69+
70+
tool, ok := state.GetToolRegistry().Get("delegate_task")
71+
if !ok {
72+
t.Fatal("delegate_task should be registered with a visible profile")
73+
}
74+
75+
properties := tool.InputSchema()["properties"].(map[string]any)
76+
tasks := properties["tasks"].(map[string]any)
77+
items := tasks["items"].(map[string]any)
78+
agent := items["properties"].(map[string]any)["agent"].(map[string]any)
79+
if !reflect.DeepEqual(agent["enum"], []string{"worker"}) {
80+
t.Fatalf("agent enum = %#v, want [worker]", agent["enum"])
81+
}
82+
}

internal/subagents/discover.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ func Catalog(profiles []Profile) string {
9090
var sb strings.Builder
9191
sb.WriteString("## Available Subagents\n\n")
9292
sb.WriteString("You can delegate up to 10 bounded tasks to named subagents and run them in parallel. ")
93+
sb.WriteString("Only the profile names listed below are valid delegate_task agents. Skills are not subagents; do not use a skill name as an agent name. ")
9394
sb.WriteString("Use a subagent only when the work can be handed off as a self-contained task with a clear objective. ")
9495
sb.WriteString("Choose profiles according to their descriptions and configured capabilities, and pass relevant paths, inputs, constraints, and expected results. ")
9596
sb.WriteString("Each delegated task is a one-shot run: you cannot ask the child follow-up questions or resume its context, so include everything needed in the initial task. ")

internal/subagents/discover_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ func TestCatalogSupportsCapableWorkerProfiles(t *testing.T) {
8383
}})
8484
for _, expected := range []string{
8585
"up to 10 bounded tasks",
86+
"Only the profile names listed below are valid delegate_task agents",
87+
"Skills are not subagents",
8688
"descriptions and configured capabilities",
8789
"relevant paths, inputs, constraints, and expected results",
8890
"one-shot run",

internal/tools/delegate_tool.go

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"sort"
8+
"strings"
79
"sync"
810
)
911

@@ -14,7 +16,8 @@ type SubagentRunner interface {
1416
const maxDelegateTasks = 10
1517

1618
type DelegateTool struct {
17-
runner SubagentRunner
19+
runner SubagentRunner
20+
namedAgents []string
1821
}
1922

2023
type delegateInput struct {
@@ -32,8 +35,11 @@ type delegateResult struct {
3235
Error string `json:"error,omitempty"`
3336
}
3437

35-
func NewDelegateTool(runner SubagentRunner) *DelegateTool {
36-
return &DelegateTool{runner: runner}
38+
func NewDelegateTool(runner SubagentRunner, namedAgents []string) *DelegateTool {
39+
return &DelegateTool{
40+
runner: runner,
41+
namedAgents: normalizedAgentNames(namedAgents),
42+
}
3743
}
3844

3945
func (t *DelegateTool) Name() string {
@@ -60,7 +66,8 @@ func (t *DelegateTool) InputSchema() map[string]any {
6066
"properties": map[string]any{
6167
"agent": map[string]any{
6268
"type": "string",
63-
"description": "Name of the subagent profile to run.",
69+
"enum": append([]string(nil), t.namedAgents...),
70+
"description": "Name of the subagent profile to run. Use only one of the listed profile names; skills are not subagents.",
6471
},
6572
"task": map[string]any{
6673
"type": "string",
@@ -74,8 +81,11 @@ func (t *DelegateTool) InputSchema() map[string]any {
7481
}
7582

7683
func (t *DelegateTool) ValidateInput(_ context.Context, input any) error {
77-
_, err := parseDelegateInput(input)
78-
return err
84+
parsed, err := parseDelegateInput(input)
85+
if err != nil {
86+
return err
87+
}
88+
return t.validateAgents(parsed)
7989
}
8090

8191
func (t *DelegateTool) Execute(ctx context.Context, input any) (any, error) {
@@ -86,6 +96,9 @@ func (t *DelegateTool) Execute(ctx context.Context, input any) (any, error) {
8696
if err != nil {
8797
return nil, err
8898
}
99+
if err := t.validateAgents(parsed); err != nil {
100+
return nil, err
101+
}
89102

90103
results := make([]delegateResult, len(parsed.Tasks))
91104
counts := make(map[string]int, len(parsed.Tasks))
@@ -127,6 +140,35 @@ func (t *DelegateTool) Execute(ctx context.Context, input any) (any, error) {
127140
}, nil
128141
}
129142

143+
func (t *DelegateTool) validateAgents(parsed delegateBatchInput) error {
144+
for i, task := range parsed.Tasks {
145+
if !containsAgent(t.namedAgents, task.Agent) {
146+
return fmt.Errorf("invalid input: tasks[%d].agent must be one of: %s", i, strings.Join(t.namedAgents, ", "))
147+
}
148+
}
149+
return nil
150+
}
151+
152+
func normalizedAgentNames(agentNames []string) []string {
153+
seen := make(map[string]bool, len(agentNames))
154+
result := make([]string, 0, len(agentNames))
155+
for _, name := range agentNames {
156+
name = strings.TrimSpace(name)
157+
if name == "" || seen[name] {
158+
continue
159+
}
160+
seen[name] = true
161+
result = append(result, name)
162+
}
163+
sort.Strings(result)
164+
return result
165+
}
166+
167+
func containsAgent(agentNames []string, agent string) bool {
168+
index := sort.SearchStrings(agentNames, agent)
169+
return index < len(agentNames) && agentNames[index] == agent
170+
}
171+
130172
func parseDelegateInput(input any) (delegateBatchInput, error) {
131173
var parsed delegateBatchInput
132174
params, ok := input.(map[string]any)

internal/tools/delegate_tool_test.go

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (m *mockSubagentRunner) recordedCalls() []delegateCall {
5252
}
5353

5454
func TestDelegateTool_Metadata(t *testing.T) {
55-
tool := NewDelegateTool(&mockSubagentRunner{})
55+
tool := NewDelegateTool(&mockSubagentRunner{}, []string{"explorer", "implementer"})
5656

5757
if tool.Name() != "delegate_task" {
5858
t.Fatalf("Name() = %q, want %q", tool.Name(), "delegate_task")
@@ -63,7 +63,7 @@ func TestDelegateTool_Metadata(t *testing.T) {
6363
}
6464

6565
func TestDelegateTool_InputSchema(t *testing.T) {
66-
tool := NewDelegateTool(&mockSubagentRunner{})
66+
tool := NewDelegateTool(&mockSubagentRunner{}, []string{"explorer", "implementer"})
6767
schema := tool.InputSchema()
6868

6969
if schema["type"] != "object" {
@@ -95,11 +95,15 @@ func TestDelegateTool_InputSchema(t *testing.T) {
9595
if _, ok := itemProperties["timeout_seconds"]; ok {
9696
t.Fatal("item properties should not include timeout_seconds")
9797
}
98+
agent := itemProperties["agent"].(map[string]any)
99+
if !reflect.DeepEqual(agent["enum"], []string{"explorer", "implementer"}) {
100+
t.Fatalf("agent enum = %#v, want [explorer implementer]", agent["enum"])
101+
}
98102
}
99103

100104
func TestDelegateTool_ExecutePassesTasksToRunner(t *testing.T) {
101105
runner := &mockSubagentRunner{result: map[string]any{"status": "completed"}}
102-
tool := NewDelegateTool(runner)
106+
tool := NewDelegateTool(runner, []string{"explorer", "implementer"})
103107

104108
result, err := tool.Execute(context.Background(), delegateTasks(
105109
map[string]any{"agent": "explorer", "task": "Inspect internal/tools."},
@@ -133,7 +137,7 @@ func TestDelegateTool_ExecutePassesTasksToRunner(t *testing.T) {
133137

134138
func TestDelegateTool_AssignsPerAgentInstancePositions(t *testing.T) {
135139
runner := &mockSubagentRunner{}
136-
tool := NewDelegateTool(runner)
140+
tool := NewDelegateTool(runner, []string{"explorer", "implementer"})
137141
_, err := tool.Execute(context.Background(), delegateTasks(
138142
map[string]any{"agent": "explorer", "task": "inspect"},
139143
map[string]any{"agent": "implementer", "task": "one"},
@@ -168,7 +172,7 @@ func TestDelegateTool_ExecuteRunsTasksInParallel(t *testing.T) {
168172
started: make(chan struct{}, taskCount),
169173
release: make(chan struct{}),
170174
}
171-
tool := NewDelegateTool(runner)
175+
tool := NewDelegateTool(runner, []string{"explorer", "implementer"})
172176
done := make(chan error, 1)
173177

174178
go func() {
@@ -199,7 +203,7 @@ func TestDelegateTool_ExecuteReturnsPerTaskErrors(t *testing.T) {
199203
result: map[string]any{"status": "error"},
200204
err: wantErr,
201205
}
202-
tool := NewDelegateTool(runner)
206+
tool := NewDelegateTool(runner, []string{"explorer", "implementer"})
203207

204208
result, err := tool.Execute(context.Background(), delegateTasks(
205209
map[string]any{"agent": "explorer", "task": "Inspect docs."},
@@ -241,12 +245,13 @@ func TestDelegateTool_ValidateInputRejectsInvalidInput(t *testing.T) {
241245
{name: "too many tasks", input: map[string]any{"tasks": tooMany}, wantErr: "at most 10 tasks"},
242246
{name: "missing agent", input: delegateTasks(map[string]any{"task": "Inspect docs."}), wantErr: "tasks[0].agent"},
243247
{name: "missing task", input: delegateTasks(map[string]any{"agent": "explorer"}), wantErr: "tasks[0].task"},
248+
{name: "unknown agent", input: delegateTasks(map[string]any{"agent": "reviewer", "task": "Review changes."}), wantErr: "must be one of: explorer, implementer"},
244249
}
245250

246251
for _, tt := range tests {
247252
t.Run(tt.name, func(t *testing.T) {
248253
runner := &mockSubagentRunner{}
249-
tool := NewDelegateTool(runner)
254+
tool := NewDelegateTool(runner, []string{"explorer", "implementer"})
250255

251256
err := tool.ValidateInput(context.Background(), tt.input)
252257
if err == nil {
@@ -262,8 +267,23 @@ func TestDelegateTool_ValidateInputRejectsInvalidInput(t *testing.T) {
262267
}
263268
}
264269

270+
func TestDelegateTool_ExecuteRejectsUnknownAgent(t *testing.T) {
271+
runner := &mockSubagentRunner{}
272+
tool := NewDelegateTool(runner, []string{"explorer"})
273+
274+
_, err := tool.Execute(context.Background(), delegateTasks(
275+
map[string]any{"agent": "reviewer", "task": "Review changes."},
276+
))
277+
if err == nil || !strings.Contains(err.Error(), "must be one of: explorer") {
278+
t.Fatalf("Execute() error = %v, want unknown-agent validation error", err)
279+
}
280+
if len(runner.recordedCalls()) != 0 {
281+
t.Fatal("runner should not be called for an unknown agent")
282+
}
283+
}
284+
265285
func TestDelegateTool_ExecuteRejectsMissingRunner(t *testing.T) {
266-
tool := NewDelegateTool(nil)
286+
tool := NewDelegateTool(nil, []string{"explorer"})
267287

268288
_, err := tool.Execute(context.Background(), delegateTasks(
269289
map[string]any{"agent": "explorer", "task": "Inspect docs."},

0 commit comments

Comments
 (0)