From 148583e7bb211349075b788c36773da328f8f636 Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Fri, 8 May 2026 22:23:50 +0200 Subject: [PATCH] fix(agent): hide discovery when spawn is unavailable --- docs/guides/configuration.md | 4 +- pkg/agent/discovery.go | 8 ++- pkg/agent/discovery_test.go | 124 ++++++++++++++++++++++++++++++++--- pkg/agent/registry.go | 8 +++ 4 files changed, 132 insertions(+), 12 deletions(-) diff --git a/docs/guides/configuration.md b/docs/guides/configuration.md index 4cbe9dd82..3bec847ba 100644 --- a/docs/guides/configuration.md +++ b/docs/guides/configuration.md @@ -238,7 +238,7 @@ Notes: ### Agent Discovery (Automatic) -When an agent has spawnable peers, PicoClaw injects a structured agent registry into that agent's system prompt on every turn. No extra `list_agents` tool call is required. +When an agent has spawnable peers and can call `spawn`, PicoClaw injects a structured agent registry into that agent's system prompt on every turn. No extra `list_agents` tool call is required. This registry is intended to make delegation concrete and reliable, especially when using `spawn` with a target `agent_id`. @@ -252,7 +252,7 @@ Each entry includes: Important behavior: -- The discovery section includes only peer agents the current agent is permitted to spawn via `subagents.allow_agents`. +- The discovery section appears only when the current agent has the `spawn` tool and includes only peer agents it is permitted to spawn via `subagents.allow_agents`. - The current agent and non-spawnable peers are omitted, so the model does not plan against unavailable agents. - Discovery is intentionally lightweight. It gives the model only the identity it needs to choose a peer: `id`, `name`, and `description`. - `config.json` remains the infrastructure layer: workspace, default agent selection, routing, and subagent permissions. Those permissions also gate discovery visibility. diff --git a/pkg/agent/discovery.go b/pkg/agent/discovery.go index 8c1c5bb82..d2f63bc1f 100644 --- a/pkg/agent/discovery.go +++ b/pkg/agent/discovery.go @@ -60,8 +60,9 @@ func (r *AgentRegistry) ListAgents(workspace string) []AgentDescriptor { return descriptors } -// ListSpawnableAgents returns descriptors only for agents the current agent is -// allowed to spawn. Restricted peers are intentionally omitted from discovery. +// ListSpawnableAgents returns descriptors only when the current agent can call +// spawn, and only for peers it is allowed to spawn. Restricted peers are +// intentionally omitted from discovery. func (r *AgentRegistry) ListSpawnableAgents(agentID string) []AgentDescriptor { r.mu.RLock() defer r.mu.RUnlock() @@ -71,6 +72,9 @@ func (r *AgentRegistry) ListSpawnableAgents(agentID string) []AgentDescriptor { if !ok || parent == nil { return nil } + if !agentHasSpawnTool(parent) { + return nil + } ids := make([]string, 0, len(r.agents)) for id := range r.agents { diff --git a/pkg/agent/discovery_test.go b/pkg/agent/discovery_test.go index bceee54d7..f31a113d8 100644 --- a/pkg/agent/discovery_test.go +++ b/pkg/agent/discovery_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" ) @@ -79,9 +80,13 @@ func TestAgentRegistry_ListSpawnableAgentsRespectsPermissions(t *testing.T) { {ID: "child2"}, {ID: "restricted"}, }) + cfg.Tools.Spawn.Enabled = true + cfg.Tools.Subagent.Enabled = true - registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) - descriptors := registry.ListSpawnableAgents("parent") + al := NewAgentLoop(cfg, bus.NewMessageBus(), &mockRegistryProvider{}) + defer al.Close() + + descriptors := al.GetRegistry().ListSpawnableAgents("parent") if len(descriptors) != 2 { t.Fatalf("expected 2 spawnable descriptors, got %d: %+v", len(descriptors), descriptors) } @@ -90,6 +95,27 @@ func TestAgentRegistry_ListSpawnableAgentsRespectsPermissions(t *testing.T) { } } +func TestAgentRegistry_ListSpawnableAgentsRequiresSpawnTool(t *testing.T) { + cfg := testCfg([]config.AgentConfig{ + { + ID: "parent", + Default: true, + Subagents: &config.SubagentsConfig{ + AllowAgents: []string{"child"}, + }, + }, + {ID: "child"}, + }) + cfg.Tools.Subagent.Enabled = true + + al := NewAgentLoop(cfg, bus.NewMessageBus(), &mockRegistryProvider{}) + defer al.Close() + + if descriptors := al.GetRegistry().ListSpawnableAgents("parent"); len(descriptors) != 0 { + t.Fatalf("expected no spawnable descriptors without spawn tool, got %+v", descriptors) + } +} + func TestContextBuilder_BuildMessagesIncludesAgentDiscoverySection(t *testing.T) { mainWorkspace := setupWorkspace(t, map[string]string{ "AGENT.md": `--- @@ -140,9 +166,13 @@ Handle restricted work. }) cfg.Tools.ReadFile.Enabled = true cfg.Tools.WriteFile.Enabled = true + cfg.Tools.Spawn.Enabled = true + cfg.Tools.Subagent.Enabled = true - registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) - mainAgent, ok := registry.GetAgent("main") + al := NewAgentLoop(cfg, bus.NewMessageBus(), &mockRegistryProvider{}) + defer al.Close() + + mainAgent, ok := al.GetRegistry().GetAgent("main") if !ok || mainAgent == nil { t.Fatal("expected main agent") } @@ -211,9 +241,13 @@ Investigate deeply. {ID: "research", Workspace: researchWorkspace}, }) cfg.Tools.ReadFile.Enabled = true + cfg.Tools.Spawn.Enabled = true + cfg.Tools.Subagent.Enabled = true - registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) - mainAgent, ok := registry.GetAgent("main") + al := NewAgentLoop(cfg, bus.NewMessageBus(), &mockRegistryProvider{}) + defer al.Close() + + mainAgent, ok := al.GetRegistry().GetAgent("main") if !ok || mainAgent == nil { t.Fatal("expected main agent") } @@ -241,6 +275,76 @@ Investigate deeply. } } +func TestContextBuilder_BuildMessagesOmitsAgentDiscoveryWithoutSpawnTool(t *testing.T) { + mainWorkspace := setupWorkspace(t, map[string]string{ + "AGENT.md": `--- +description: Main agent +tools: [read_file] +--- +# Agent + +Generalist. +`, + }) + defer cleanupWorkspace(t, mainWorkspace) + + researchWorkspace := setupWorkspace(t, map[string]string{ + "AGENT.md": `--- +description: Research specialist +--- +# Agent + +Investigate deeply. +`, + }) + defer cleanupWorkspace(t, researchWorkspace) + + cfg := testCfg([]config.AgentConfig{ + { + ID: "main", + Default: true, + Workspace: mainWorkspace, + Subagents: &config.SubagentsConfig{ + AllowAgents: []string{"research"}, + }, + }, + {ID: "research", Workspace: researchWorkspace}, + }) + cfg.Tools.ReadFile.Enabled = true + cfg.Tools.Spawn.Enabled = true + cfg.Tools.Subagent.Enabled = true + + al := NewAgentLoop(cfg, bus.NewMessageBus(), &mockRegistryProvider{}) + defer al.Close() + + mainAgent, ok := al.GetRegistry().GetAgent("main") + if !ok || mainAgent == nil { + t.Fatal("expected main agent") + } + + messages := mainAgent.ContextBuilder.BuildMessages( + nil, + "", + "handle locally", + nil, + "telegram", + "chat-1", + "", + "", + ) + if len(messages) == 0 { + t.Fatal("expected messages") + } + + systemPrompt := messages[0].Content + if strings.Contains(systemPrompt, "# Agent Discovery") { + t.Fatalf("did not expect discovery section without spawn tool, got %q", systemPrompt) + } + if strings.Contains(systemPrompt, `"id": "research"`) { + t.Fatalf("did not expect peer identity without spawn tool, got %q", systemPrompt) + } +} + func TestContextBuilder_BuildMessagesOmitsAgentDiscoverySectionForSingleton(t *testing.T) { mainWorkspace := setupWorkspace(t, map[string]string{ "AGENT.md": `--- @@ -257,9 +361,13 @@ Generalist. {ID: "main", Default: true, Workspace: mainWorkspace}, }) cfg.Tools.ReadFile.Enabled = true + cfg.Tools.Spawn.Enabled = true + cfg.Tools.Subagent.Enabled = true - registry := NewAgentRegistry(cfg, &mockRegistryProvider{}) - mainAgent, ok := registry.GetAgent("main") + al := NewAgentLoop(cfg, bus.NewMessageBus(), &mockRegistryProvider{}) + defer al.Close() + + mainAgent, ok := al.GetRegistry().GetAgent("main") if !ok || mainAgent == nil { t.Fatal("expected main agent") } diff --git a/pkg/agent/registry.go b/pkg/agent/registry.go index a4d1a860d..821ad4187 100644 --- a/pkg/agent/registry.go +++ b/pkg/agent/registry.go @@ -137,6 +137,14 @@ func agentAllowsSubagent(parent *AgentInstance, targetNorm string) bool { return false } +func agentHasSpawnTool(agent *AgentInstance) bool { + if agent == nil || agent.Tools == nil { + return false + } + _, ok := agent.Tools.Get("spawn") + return ok +} + // ForEachTool calls fn for every tool registered under the given name // across all agents. This is useful for propagating dependencies (e.g. // MediaStore) to tools after registry construction.