diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 9ab10deb3..469a0bfdc 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -19,28 +19,50 @@ import ( const orchestrationGuidance = `## Orchestration -You are the conductor, not the performer. Prefer delegation over doing everything inline. +You are the conductor, not the performer. **Your primary job is to delegate, not to implement.** -Use **spawn** (non-blocking) when: -- Tasks can run in parallel or in the background -- Multiple independent tasks can run simultaneously — spawn each one -- You don't need the result to decide the next step -- The operation is long-running (builds, fetches, analysis, file processing) +### spawn (non-blocking) — DEFAULT choice +Returns immediately. Use for any task that can run independently. +Call the spawn tool with JSON arguments like this: -Use **subagent** (blocking) when: -- You need the result before you can continue -- Correctness of the next step depends on the outcome +Tool: spawn +Arguments: {"task": "Examine pkg/auth/ and report middleware pattern", "preset": "scout", "label": "auth-scout"} -Do inline only when: -- It's a single fast tool call (read a file, quick search) -- Delegation overhead clearly outweighs the benefit +Tool: spawn +Arguments: {"task": "Implement rate limiter in pkg/ratelimit/ with tests", "preset": "coder", "label": "rate-limiter"} -Default bias: if a task involves more than 2-3 tool calls or can run independently, delegate it. -When you spawn, immediately plan what comes next — blocking means you've stopped thinking. -Fork aggressively: explore multiple directions simultaneously. +### subagent (blocking) — only when you need the answer NOW +Blocks until the subagent finishes. Use only when you cannot proceed without the result. +Does not take a preset — it runs with default tools. + +Tool: subagent +Arguments: {"task": "Read pkg/config/config.go and list all SubagentsConfig fields", "label": "config-check"} + +### When to use which +- spawn: parallel tasks, independent work, implementation, long analysis, >2 tool calls +- subagent: you need the result before your next decision +- inline: single quick tool call where delegation overhead is wasteful + +### Presets (for spawn only) +| preset | role | can write | can exec | +|--------|------|-----------|----------| +| scout | explore, investigate | no | no | +| analyst | analyze, run tests | no | go test/vet, git | +| coder | implement + verify | yes (sandbox) | test/lint/fmt | +| worker | build + install | yes (sandbox) | build/package mgr | +| coordinator | orchestrate others | yes (sandbox) | general + spawn | + +### Parallel spawning +Spawn multiple independent tasks at once — do NOT wait between them: + +Tool: spawn +Arguments: {"task": "Analyze error handling patterns in pkg/providers/", "preset": "analyst", "label": "error-patterns"} + +Tool: spawn +Arguments: {"task": "List all HTTP endpoints in pkg/miniapp/", "preset": "scout", "label": "endpoints"} After spawning, record the assignment in ## Orchestration > Delegated in MEMORY.md. -When results come back, synthesize and decide the next fork.` +When results come back, synthesize findings and decide the next fork.` type ContextBuilder struct { workspace string @@ -123,9 +145,23 @@ func (cb *ContextBuilder) getIdentity() string { ` } + // Conditional identity and plan executing rule for orchestration mode + identity := "a helpful AI assistant" + executingRule := `Work through the current Phase's steps. + Mark each "- [x]" via edit_file. The system will auto-advance phases.` + if cb.orchestrationEnabled { + identity = "a conductor AI agent that orchestrates subagents" + executingRule = `Delegate the current Phase's steps to subagents using spawn. + For each step: spawn a subagent with the appropriate preset (scout for investigation, + coder for implementation, analyst for review). Spawn multiple independent steps in parallel. + When a subagent completes, mark "- [x]" via edit_file and record findings in + ## Orchestration > Findings in MEMORY.md. + Only do a step inline if it's a single quick tool call (e.g., reading one file).` + } + return fmt.Sprintf(prompt+`# picoclaw 🦞 -You are picoclaw, a helpful AI assistant. +You are picoclaw, %s. ## Workspace Your workspace is at: %s @@ -148,8 +184,7 @@ Your workspace is at: %s After each answer, use edit_file to save findings to ## Context in memory/MEMORY.md. When you have enough information, add ## Phase sections with "- [ ]" checkbox steps, and ## Commands section below the header. Then change > Status: to "review". - If Status is "review": The plan is awaiting user approval. Do NOT change Status yourself. - - If Status is "executing": Work through the current Phase's steps. - Mark each "- [x]" via edit_file. The system will auto-advance phases. + - If Status is "executing": %s - Plan format (header is written by the system — do NOT delete it): # Active Plan > Task: @@ -176,7 +211,7 @@ Your workspace is at: %s - For architecture/flow, use arrow text: CLI → Pipeline → Adapters 5. **Context summaries** - Conversation summaries provided as context are approximate references only. They may be incomplete or outdated. Always defer to explicit user instructions over summary content.`, - workspacePath, workspacePath, workspacePath, workspacePath, toolsSection) + identity, workspacePath, workspacePath, workspacePath, workspacePath, toolsSection, executingRule) } func (cb *ContextBuilder) buildToolsSection() string { diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index c88701f24..015e1a149 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -115,6 +115,7 @@ type processOptions struct { NoHistory bool // If true, don't load session history (for heartbeat) TaskID string // Unique task ID for background task status tracking Background bool // If true, this is a background task (cron/heartbeat) — enables live task notifications + SystemMessage bool // If true, this is a system message (subagent result) — skip placeholder and plan nudge } const defaultResponse = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json." @@ -302,6 +303,9 @@ func registerSharedTools( return registry.CanSpawnSubagent(currentAgentID, targetAgentID) }) agent.Tools.Register(spawnTool) + // Register blocking subagent tool alongside spawn + subagentTool := tools.NewSubagentTool(subagentManager) + agent.Tools.Register(subagentTool) } // Update context builder with the complete tools registry @@ -796,24 +800,92 @@ func (al *AgentLoop) processSystemMessage(ctx context.Context, msg bus.InboundMe return "", nil } - // Use default agent for system messages + // Inject subagent result into session history without running a full LLM loop. + // The conductor will see the result on its next turn. This avoids: + // - Flooding the chat with a response for every subagent completion + // - Consuming the Telegram "Thinking..." placeholder + // - Wasting LLM tokens on processing each result individually agent := al.registry.GetDefaultAgent() if agent == nil { return "", fmt.Errorf("no default agent for system message") } - // Use the origin session for context sessionKey := routing.BuildAgentMainSessionKey(agent.ID) + historyMsg := fmt.Sprintf("[System: %s] %s", msg.SenderID, msg.Content) + agent.Sessions.AddMessage(sessionKey, "user", historyMsg) + agent.Sessions.MarkDirty(sessionKey) - return al.runAgentLoop(ctx, agent, processOptions{ - SessionKey: sessionKey, + // Send a brief notification (SkipPlaceholder to avoid corrupting status messages) + label := msg.SenderID + if idx := strings.LastIndex(label, ":"); idx >= 0 { + label = label[idx+1:] + } + notification := formatSubagentCompletion(label, msg.Metadata) + _ = al.bus.PublishOutbound(ctx, bus.OutboundMessage{ Channel: originChannel, ChatID: originChatID, - UserMessage: fmt.Sprintf("[System: %s] %s", msg.SenderID, msg.Content), - DefaultResponse: "Background task completed.", - EnableSummary: false, - SendResponse: true, + Content: notification, + SkipPlaceholder: true, }) + + logger.InfoCF("agent", "Subagent result injected into session history", + map[string]any{ + "sender_id": msg.SenderID, + "session_key": sessionKey, + "content_len": len(content), + }) + + return "", nil +} + +// formatSubagentCompletion builds the user-facing notification for a completed subagent. +// If metadata contains duration_ms and tool_calls it produces e.g.: +// +// "📋 scout-1 completed (3.2s, 5 tool calls)." +// +// Without metadata it falls back to the plain "📋 scout-1 completed." format. +func formatSubagentCompletion(label string, metadata map[string]string) string { + if len(metadata) == 0 { + return fmt.Sprintf("📋 %s completed.", label) + } + durationMs, _ := strconv.ParseInt(metadata["duration_ms"], 10, 64) + toolCalls, _ := strconv.Atoi(metadata["tool_calls"]) + + if durationMs <= 0 && toolCalls <= 0 { + return fmt.Sprintf("📋 %s completed.", label) + } + + parts := make([]string, 0, 2) + if durationMs > 0 { + parts = append(parts, formatDurationMs(durationMs)) + } + if toolCalls > 0 { + if toolCalls == 1 { + parts = append(parts, "1 tool call") + } else { + parts = append(parts, fmt.Sprintf("%d tool calls", toolCalls)) + } + } + return fmt.Sprintf("📋 %s completed (%s).", label, strings.Join(parts, ", ")) +} + +// formatDurationMs converts milliseconds to a human-readable duration string. +// Examples: 800 → "0.8s", 1200 → "1.2s", 65000 → "1m5s", 3661000 → "61m1s". +func formatDurationMs(ms int64) string { + if ms < 1000 { + return fmt.Sprintf("%dms", ms) + } + totalSec := ms / 1000 + if totalSec < 60 { + tenths := (ms % 1000) / 100 + return fmt.Sprintf("%d.%ds", totalSec, tenths) + } + mins := totalSec / 60 + sec := totalSec % 60 + if sec == 0 { + return fmt.Sprintf("%dm", mins) + } + return fmt.Sprintf("%dm%ds", mins, sec) } // acquireSessionLock gets or creates a per-session semaphore and acquires it. @@ -1066,16 +1138,49 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt al.promptDirty.Store(false) } - // 5. Run LLM iteration loop - finalContent, iteration, err := al.runLLMIteration(ctx, agent, messages, opts, task, preStatus) - if err != nil { - return "", err - } + // 5. Run LLM iteration loop (with automatic phase transitions) + var finalContent string + var iteration int + const maxPhaseTransitions = 10 + + for phaseLoop := 0; ; phaseLoop++ { + // On phase transition: rebuild system prompt with new phase context + nudge + if phaseLoop > 0 { + messages = agent.ContextBuilder.BuildMessages( + agent.Sessions.GetHistory(opts.SessionKey), + agent.Sessions.GetSummary(opts.SessionKey), + "", nil, opts.Channel, opts.ChatID, + ) + messages = append(messages, providers.Message{ + Role: "user", + Content: fmt.Sprintf( + "[System] Phase %d is now active. Continue working on the next steps.", + agent.ContextBuilder.GetCurrentPhase(), + ), + }) + if len(messages) > 0 { + al.lastSystemPrompt.Store(messages[0].Content) + } + } + + curPlanStatus := preStatus + if phaseLoop > 0 { + curPlanStatus = agent.ContextBuilder.GetPlanStatus() + } + + var err error + finalContent, iteration, err = al.runLLMIteration(ctx, agent, messages, opts, task, curPlanStatus) + if err != nil { + return "", err + } + + // 5a. Auto-advance plan phases after LLM iteration + postStatus := agent.ContextBuilder.GetPlanStatus() + if !agent.ContextBuilder.HasActivePlan() || + !(postStatus == "executing" || postStatus == "review" || postStatus == "completed") { + break + } - // 5a. Auto-advance plan phases after LLM iteration - postStatus := agent.ContextBuilder.GetPlanStatus() - if agent.ContextBuilder.HasActivePlan() && - (postStatus == "executing" || postStatus == "review" || postStatus == "completed") { // Intercept: if AI changed status to executing or review without user approval // (from interviewing or review), validate and hold at "review". if preStatus == "interviewing" || (preStatus == "review" && postStatus == "executing") { @@ -1083,11 +1188,11 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt _ = agent.ContextBuilder.SetPlanStatus("interviewing") logger.WarnCF("agent", "Reverted plan to interviewing: "+err.Error(), map[string]any{"agent_id": agent.ID}) - // Inject rejection into session history so LLM sees it next iteration rejectionMsg := "[System] Plan rejected: " + err.Error() + ". Fix and try again." agent.Sessions.AddMessage(opts.SessionKey, "user", rejectionMsg) } else { _ = agent.ContextBuilder.SetPlanStatus("review") + al.reporter().ReportStateChange(opts.SessionKey, orch.AgentStatePlanReview, "") if !constants.IsInternalChannel(opts.Channel) { planDisplay := agent.ContextBuilder.FormatPlanDisplay() _ = al.bus.PublishOutbound(ctx, bus.OutboundMessage{ @@ -1098,17 +1203,22 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt }) } } - } else if postStatus == "executing" && agent.ContextBuilder.GetTotalPhases() == 0 { - // Safeguard: executing but no phases (shouldn't happen, but be safe). + break + } + + if postStatus == "executing" && agent.ContextBuilder.GetTotalPhases() == 0 { _ = agent.ContextBuilder.SetPlanStatus("interviewing") logger.WarnCF("agent", "Reverted plan to interviewing: no phases defined", map[string]any{"agent_id": agent.ID}) - } else if agent.ContextBuilder.IsPlanComplete() { - // Mark plan as completed (keep memory for review; user can /plan clear) + break + } + + if agent.ContextBuilder.IsPlanComplete() { total := agent.ContextBuilder.GetTotalPhases() _ = agent.ContextBuilder.SetCurrentPhase(total) if preStatus != "completed" { _ = agent.ContextBuilder.SetPlanStatus("completed") + al.reporter().ReportStateChange(opts.SessionKey, orch.AgentStatePlanCompleted, "") // Deactivate worktree on plan completion commitMsg := "plan: " + agent.ContextBuilder.Memory().GetPlanTaskName() @@ -1128,7 +1238,15 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt }) } } - } else if agent.ContextBuilder.IsCurrentPhaseComplete() { + break + } + + if agent.ContextBuilder.IsCurrentPhaseComplete() { + if phaseLoop >= maxPhaseTransitions { + logger.WarnCF("agent", "Max phase transitions reached, stopping", + map[string]any{"agent_id": agent.ID, "transitions": phaseLoop}) + break + } prev := agent.ContextBuilder.GetCurrentPhase() _ = agent.ContextBuilder.AdvancePhase() next := agent.ContextBuilder.GetCurrentPhase() @@ -1140,7 +1258,11 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt SkipPlaceholder: true, }) } + al.notifyStateChange() + continue } + + break } al.notifyStateChange() @@ -1176,9 +1298,10 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt // 8. Optional: send response via bus if opts.SendResponse { _ = al.bus.PublishOutbound(ctx, bus.OutboundMessage{ - Channel: opts.Channel, - ChatID: opts.ChatID, - Content: finalContent, + Channel: opts.Channel, + ChatID: opts.ChatID, + Content: finalContent, + SkipPlaceholder: opts.SystemMessage, // suppress Telegram "Thinking..." for system messages }) } @@ -1259,6 +1382,24 @@ func buildPlanReminder(planStatus string) (providers.Message, bool) { return providers.Message{Role: "user", Content: content}, true } +// buildOrchReminder returns a reminder to use spawn/subagent during plan execution. +// Fires on first iteration and every 3rd iteration to reinforce delegation behavior. +func buildOrchReminder(iteration int) (providers.Message, bool) { + if iteration != 1 && iteration%3 != 0 { + return providers.Message{}, false + } + content := `[System] ORCHESTRATION mode active. You MUST delegate plan steps to subagents. +Use spawn (non-blocking, returns immediately) or subagent (blocking, waits for result). +Do NOT implement steps inline unless they are a single trivial tool call. + +To delegate, call the tool with JSON arguments: + Tool: spawn Arguments: {"task": "...", "preset": "scout", "label": "..."} + Tool: subagent Arguments: {"task": "...", "label": "..."} + +Spawn multiple independent steps in parallel for maximum throughput.` + return providers.Message{Role: "user", Content: content}, true +} + // cdPrefixPattern matches "cd /some/path && " at the start of a shell command. // Group 1 captures the target directory path. var cdPrefixPattern = regexp.MustCompile(`^cd\s+(\S+)\s*&&\s*`) @@ -2055,7 +2196,7 @@ func (al *AgentLoop) runLLMIteration( } // Report waiting state to canvas before each LLM call. - al.reporter().ReportStateChange(opts.SessionKey, "waiting", "") + al.reporter().ReportStateChange(opts.SessionKey, orch.AgentStateWaiting, "") // Retry loop for context/token errors maxRetries := 2 @@ -2447,7 +2588,7 @@ func (al *AgentLoop) runLLMIteration( } // Report toolcall state to canvas. - al.reporter().ReportStateChange(opts.SessionKey, "toolcall", tc.Name) + al.reporter().ReportStateChange(opts.SessionKey, orch.AgentStateToolCall, tc.Name) toolStart := time.Now() toolCtx := ctx @@ -2594,6 +2735,18 @@ func (al *AgentLoop) runLLMIteration( } } + // Inject orchestration nudge during plan execution to encourage spawn usage. + if planSnapshot == "executing" && agent.Subagents != nil && agent.Subagents.Enabled { + if reminder, ok := buildOrchReminder(iteration); ok { + messages = append(messages, reminder) + logger.DebugCF("agent", "Injected orchestration nudge", + map[string]any{ + "agent_id": agent.ID, + "iteration": iteration, + }) + } + } + // Refresh system prompt: tool execution may have changed workDir, // memory, plan status, etc. Update messages[0] so the next LLM // call sees the current state. @@ -3335,6 +3488,7 @@ func (al *AgentLoop) handlePlanCommand(args []string, sessionKey string) (string if err := agent.ContextBuilder.SetPlanStatus("executing"); err != nil { return fmt.Sprintf("Error: %v", err), true } + al.reporter().ReportStateChange(sessionKey, orch.AgentStatePlanExecuting, "") al.planStartPending = true clearHistory := len(args) > 1 && args[1] == "clear" al.planClearHistory = clearHistory diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index b1765f7c4..e7a9e2be9 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -2991,3 +2991,91 @@ func TestHandleReasoning(t *testing.T) { } }) } + +func TestFormatDurationMs(t *testing.T) { + tests := []struct { + ms int64 + want string + }{ + {0, "0ms"}, + {500, "500ms"}, + {999, "999ms"}, + {1000, "1.0s"}, + {1200, "1.2s"}, + {3500, "3.5s"}, + {59900, "59.9s"}, + {60000, "1m"}, + {61000, "1m1s"}, + {65000, "1m5s"}, + {120000, "2m"}, + {3661000, "61m1s"}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%dms", tt.ms), func(t *testing.T) { + got := formatDurationMs(tt.ms) + if got != tt.want { + t.Errorf("formatDurationMs(%d) = %q, want %q", tt.ms, got, tt.want) + } + }) + } +} + +func TestFormatSubagentCompletion(t *testing.T) { + tests := []struct { + name string + label string + metadata map[string]string + want string + }{ + { + "no metadata", + "scout-1", + nil, + "📋 scout-1 completed.", + }, + { + "empty metadata", + "scout-1", + map[string]string{}, + "📋 scout-1 completed.", + }, + { + "duration and tool calls", + "scout-1", + map[string]string{"duration_ms": "3200", "tool_calls": "5"}, + "📋 scout-1 completed (3.2s, 5 tool calls).", + }, + { + "single tool call", + "coder-1", + map[string]string{"duration_ms": "1200", "tool_calls": "1"}, + "📋 coder-1 completed (1.2s, 1 tool call).", + }, + { + "duration only", + "scout-2", + map[string]string{"duration_ms": "65000", "tool_calls": "0"}, + "📋 scout-2 completed (1m5s).", + }, + { + "tool calls only", + "scout-3", + map[string]string{"duration_ms": "0", "tool_calls": "10"}, + "📋 scout-3 completed (10 tool calls).", + }, + { + "zero everything", + "scout-4", + map[string]string{"duration_ms": "0", "tool_calls": "0"}, + "📋 scout-4 completed.", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := formatSubagentCompletion(tt.label, tt.metadata) + if got != tt.want { + t.Errorf("formatSubagentCompletion(%q, %v) = %q, want %q", tt.label, tt.metadata, got, tt.want) + } + }) + } +} diff --git a/pkg/agent/memory.go b/pkg/agent/memory.go index 7245f3952..6c49d599f 100644 --- a/pkg/agent/memory.go +++ b/pkg/agent/memory.go @@ -630,6 +630,14 @@ func (ms *MemoryStore) getPlanContextFrom(content string) string { sb.WriteString("\n") } + // Orchestration section: conductor's delegation tracking (Delegated/Findings/Decisions) + orchContent := ms.extractSection(content, "Orchestration") + if orchContent != "" { + sb.WriteString("### Orchestration\n") + sb.WriteString(orchContent) + sb.WriteString("\n") + } + return sb.String() } diff --git a/pkg/miniapp/static/index.html b/pkg/miniapp/static/index.html index 985d77c7a..410daa7db 100644 --- a/pkg/miniapp/static/index.html +++ b/pkg/miniapp/static/index.html @@ -884,11 +884,16 @@
CNDR
-
+
👩‍💼
SEC
+
+
🕊️
+
HB
+
+
@@ -1810,7 +1815,7 @@ var _orchLastTs = null; var _orchBOB = [0, -1, -2, -1]; var _orchFRAME_MS = {idle:450, waiting:650, toolcall:90, talking:280, entering:220, exiting:220}; var _orchWALK = 55; -var _orchConductor, _orchSecretary, _orchSubagents, _orchSlots, _orchFreeSlots; +var _orchConductor, _orchSecretary, _orchHeartbeat, _orchSubagents, _orchSlots, _orchFreeSlots; function _orchMakeChar(id, emoji, home) { return {id:id, emoji:emoji, x:home.x, y:home.y, home:home, target:null, state:'idle', @@ -1819,7 +1824,10 @@ function _orchMakeChar(id, emoji, home) { function _orchInitChars() { _orchConductor = _orchMakeChar('conductor', '👑', MAP_POSITIONS.conductor); _orchSecretary = _orchMakeChar('secretary', '👩‍💼', MAP_POSITIONS.secretary); - _orchConductor.alive = true; _orchSecretary.alive = true; + _orchHeartbeat = _orchMakeChar('heartbeat', '🕊️', MAP_POSITIONS.heartbeat || {x:230,y:58}); + _orchConductor.alive = true; _orchSecretary.alive = false; _orchHeartbeat.alive = true; + _orchConductor.statusText = null; + _orchHeartbeat.facing = 1; _orchHeartbeat.flipTimer = 0; var ps = [{id:'s0',emoji:'🔍'},{id:'s1',emoji:'📊'},{id:'s2',emoji:'💻'}, {id:'s3',emoji:'🔧'},{id:'s4',emoji:'🎯'}]; _orchSubagents = ps.map(function(p,i){ @@ -1828,7 +1836,7 @@ function _orchInitChars() { }); _orchSlots = {}; _orchFreeSlots = _orchSubagents.slice(); } -function _orchAllChars() { return [_orchConductor, _orchSecretary].concat(_orchSubagents); } +function _orchAllChars() { return [_orchConductor, _orchSecretary, _orchHeartbeat].concat(_orchSubagents); } function _orchSyncBadge(id, state, alive) { var el = document.getElementById('orch-badge-' + id); if (!el) return; @@ -1838,12 +1846,30 @@ function _orchSyncBadge(id, state, alive) { + (state==='toolcall'? ' toolcall' : '') + (state==='waiting' ? ' waiting' : ''); } -function _orchSetState(c, state) { c.state=state; _orchSyncBadge(c.id, state, c.alive); } +function _orchSetState(c, state, tool) { + c.state=state; _orchSyncBadge(c.id, state, c.alive); + if (c === _orchConductor) { + if (state==='waiting') c.statusText='🤔'; + else if (state==='toolcall') c.statusText='⌨'; + else if (state==='user_waiting') c.statusText='⏳'; + else if (state==='plan_interviewing') c.statusText='📋'; + else if (state==='plan_review') c.statusText='🔍'; + else if (state==='plan_executing') c.statusText='▶️'; + else if (state==='plan_completed') c.statusText='✅'; + else c.statusText=null; + // Secretary appears only during plan mode. + var inPlan = state.indexOf('plan_')===0; + if (_orchSecretary.alive !== inPlan) { + _orchSecretary.alive = inPlan; + _orchSyncBadge('secretary', _orchSecretary.state, _orchSecretary.alive); + } + } +} function _orchMoveTo(c, pos, cb) { c.target=pos; c._onArrive=cb||null; } function _orchSay(c, text, ttl) { c.bubble={text:text, ttl:ttl||2200}; } function _orchCharForId(id) { - if (id === 'heartbeat') return _orchSecretary; + if (id === 'heartbeat') return _orchHeartbeat; if (_orchSlots[id]) return _orchSlots[id]; return _orchConductor; } @@ -1865,7 +1891,16 @@ function _orchGC(id) { _orchSetState(c,'exiting'); _orchMoveTo(c, MAP_POSITIONS.door, function(){ c.alive=false; _orchSetState(c,'idle'); }); } else { - var ch=_orchCharForId(id); ch.alive=false; _orchSetState(ch,'idle'); + var ch=_orchCharForId(id); + if (ch === _orchHeartbeat) { + // Heartbeat pigeon is permanent — keep alive, just return to idle. + _orchSetState(ch,'idle'); + } else if (ch === _orchConductor) { + // Conductor is permanent — keep alive, show ⏳ waiting for user. + _orchSetState(ch,'user_waiting'); + } else { + ch.alive=false; _orchSetState(ch,'idle'); + } } } function _orchConverse(fromId, toId, text) { @@ -1885,17 +1920,45 @@ function _orchConverse(fromId, toId, text) { function _orchUpdate(dt) { _orchAllChars().forEach(function(c){ if (!c.alive && c.state!=='entering') return; - c.frameTimer+=dt; - var dur=_orchFRAME_MS[c.state]||450; - if (c.frameTimer>=dur){ c.frame=(c.frame+1)%4; c.frameTimer-=dur; } + // Frame animation (bob): pigeon uses state-specific timing instead of shared table. + if (c === _orchHeartbeat) { + if (c.state === 'idle') { + c.frame = 0; // pin still — no bob when inactive + } else { + c.frameTimer += dt; + var pDur = c.state==='toolcall' ? 130 : 380; + if (c.frameTimer >= pDur) { c.frame=(c.frame+1)%4; c.frameTimer-=pDur; } + } + } else { + c.frameTimer+=dt; + var dur=_orchFRAME_MS[c.state]||450; + if (c.frameTimer>=dur){ c.frame=(c.frame+1)%4; c.frameTimer-=dur; } + } if (c.target){ var dx=c.target.x-c.x, dy=c.target.y-c.y, dist=Math.sqrt(dx*dx+dy*dy); if (dist>1.5){ var spd=_orchWALK*dt/1000; c.x+=dx/dist*spd; c.y+=dy/dist*spd; } else { c.x=c.target.x; c.y=c.target.y; c.target=null; if(c._onArrive){c._onArrive();c._onArrive=null;} } } if (c.bubble){ c.bubble.ttl-=dt; if(c.bubble.ttl<=0) c.bubble=null; } + // Heartbeat pigeon: direction flip rate reflects activity level. + if (c === _orchHeartbeat) { + if (c.target) { + var pdx = c.target.x - c.x; + if (Math.abs(pdx) > 1) c.facing = pdx > 0 ? 1 : -1; + } else { + var flipRate = c.state==='toolcall' ? 280 : c.state==='waiting' ? 600 : 2800; + c.flipTimer += dt; + if (c.flipTimer >= flipRate) { c.flipTimer -= flipRate; c.facing = -c.facing; } + } + } }); } +function _orchDrawStatus(c) { + if (!c.statusText) return; + var yOff=_orchBOB[c.frame], cx=Math.floor(c.x), cy=Math.floor(c.y+yOff)-20; + orchCtx.font='11px serif'; orchCtx.textAlign='center'; orchCtx.textBaseline='middle'; + orchCtx.fillText(c.statusText, cx, cy); +} function _orchDrawBubble(c) { if (!c.bubble) return; var yOff=_orchBOB[c.frame], bx=c.x, by=c.y+yOff-18; @@ -1917,12 +1980,26 @@ function _orchDrawChar(c) { } else if (c.state==='waiting'){ orchCtx.fillStyle='rgba(96,165,250,0.25)'; orchCtx.beginPath(); orchCtx.arc(cx,cy,11,0,Math.PI*2); orchCtx.fill(); + } else if (c.state==='user_waiting' || c.state==='plan_review'){ + orchCtx.fillStyle='rgba(167,139,250,0.18)'; orchCtx.beginPath(); + orchCtx.arc(cx,cy,10,0,Math.PI*2); orchCtx.fill(); + } else if (c.state==='plan_executing'){ + orchCtx.fillStyle='rgba(74,222,128,0.18)'; orchCtx.beginPath(); + orchCtx.arc(cx,cy,10,0,Math.PI*2); orchCtx.fill(); } orchCtx.font='18px serif'; orchCtx.textAlign='center'; orchCtx.textBaseline='middle'; - orchCtx.fillText(c.emoji, cx, cy); + if (c.facing === -1) { + orchCtx.save(); + orchCtx.translate(cx, cy); orchCtx.scale(-1, 1); + orchCtx.fillText(c.emoji, 0, 0); + orchCtx.restore(); + } else { + orchCtx.fillText(c.emoji, cx, cy); + } orchCtx.font='6px Silkscreen,monospace'; orchCtx.textAlign='center'; orchCtx.textBaseline='top'; orchCtx.fillStyle=c.state==='talking'?'#facc15':'#3a4a7a'; orchCtx.fillText(c.id.toUpperCase(), cx, cy+11); + _orchDrawStatus(c); _orchDrawBubble(c); } function _orchRender(ts) { @@ -1964,7 +2041,7 @@ function connectOrchWs() { } else if (msg.type==='event') { var ev=msg.event||{}; if (ev.type==='agent_spawn') _orchSpawn(ev.id); - if (ev.type==='agent_state') { var c=_orchCharForId(ev.id); if(c) _orchSetState(c,ev.state); } + if (ev.type==='agent_state') { var c=_orchCharForId(ev.id); if(c) _orchSetState(c,ev.state,ev.tool); } if (ev.type==='agent_gc') _orchGC(ev.id); if (ev.type==='conversation') _orchConverse(ev.from, ev.to, ev.text); } diff --git a/pkg/miniapp/static/map.js b/pkg/miniapp/static/map.js index 74b56e433..b16f72730 100644 --- a/pkg/miniapp/static/map.js +++ b/pkg/miniapp/static/map.js @@ -27,6 +27,7 @@ var MAP_POSITIONS = { door: { x: 160, y: 314 }, // entry / exit point conductor: { x: 160, y: 58 }, secretary: { x: 108, y: 58 }, + heartbeat: { x: 230, y: 58 }, // pigeon messenger — periodic heartbeat agent meeting: { x: 160, y: 161 }, // neutral zone for conversations stations: [ { x: 40, y: 106 }, // S0 scout diff --git a/pkg/orch/broadcaster.go b/pkg/orch/broadcaster.go index e5c8b85f8..1597a7eca 100644 --- a/pkg/orch/broadcaster.go +++ b/pkg/orch/broadcaster.go @@ -89,8 +89,8 @@ func (b *Broadcaster) ReportSpawn(id, label, task string) { } // ReportStateChange implements AgentReporter. -func (b *Broadcaster) ReportStateChange(id, state, tool string) { - b.Publish(Event{Type: "agent_state", ID: id, State: state, Tool: tool}) +func (b *Broadcaster) ReportStateChange(id string, state AgentState, tool string) { + b.Publish(Event{Type: "agent_state", ID: id, State: string(state), Tool: tool}) } // ReportConversation implements AgentReporter. diff --git a/pkg/orch/reporter.go b/pkg/orch/reporter.go index b8229314d..a717336ee 100644 --- a/pkg/orch/reporter.go +++ b/pkg/orch/reporter.go @@ -4,17 +4,17 @@ package orch // Both Broadcaster (real events) and noopReporter (disabled) implement this. type AgentReporter interface { ReportSpawn(id, label, task string) - ReportStateChange(id, state, tool string) + ReportStateChange(id string, state AgentState, tool string) ReportConversation(from, to, text string) ReportGC(id, reason string) } type noopReporter struct{} -func (n *noopReporter) ReportSpawn(id, label, task string) {} -func (n *noopReporter) ReportStateChange(id, state, tool string) {} -func (n *noopReporter) ReportConversation(from, to, text string) {} -func (n *noopReporter) ReportGC(id, reason string) {} +func (n *noopReporter) ReportSpawn(id, label, task string) {} +func (n *noopReporter) ReportStateChange(id string, state AgentState, tool string) {} +func (n *noopReporter) ReportConversation(from, to, text string) {} +func (n *noopReporter) ReportGC(id, reason string) {} // Noop is the AgentReporter to use when orchestration is disabled. // Allows nil-free code in callers. diff --git a/pkg/orch/reporter_test.go b/pkg/orch/reporter_test.go index 120f03c26..e7428219a 100644 --- a/pkg/orch/reporter_test.go +++ b/pkg/orch/reporter_test.go @@ -10,8 +10,8 @@ var _ AgentReporter = (*Broadcaster)(nil) // orchestration mode. func TestNoop_AllMethods_NoPanic(t *testing.T) { Noop.ReportSpawn("id", "label", "task") - Noop.ReportStateChange("id", "waiting", "") - Noop.ReportStateChange("id", "toolcall", "bash") + Noop.ReportStateChange("id", AgentStateWaiting, "") + Noop.ReportStateChange("id", AgentStateToolCall, "bash") Noop.ReportConversation("conductor", "sub-1", "do something") Noop.ReportGC("id", "completed") } @@ -49,9 +49,9 @@ func TestBroadcaster_ReportStateChange_MapsToAgentStateEvent(t *testing.T) { b.ReportSpawn("agent-1", "coder", "implement it") <-sub.Ch // consume spawn - b.ReportStateChange("agent-1", "toolcall", "bash") + b.ReportStateChange("agent-1", AgentStateToolCall, "bash") ev := <-sub.Ch - if ev.Type != "agent_state" || ev.State != "toolcall" || ev.Tool != "bash" { + if ev.Type != "agent_state" || ev.State != string(AgentStateToolCall) || ev.Tool != "bash" { t.Fatalf("unexpected event: %+v", ev) } snap := b.Snapshot() diff --git a/pkg/orch/state.go b/pkg/orch/state.go new file mode 100644 index 000000000..6a77dd8ca --- /dev/null +++ b/pkg/orch/state.go @@ -0,0 +1,31 @@ +package orch + +// AgentState is a typed string representing the lifecycle state of an agent session. +// Values are sent as-is over the WebSocket event stream to the Mini App canvas. +type AgentState string + +const ( + // AgentStateIdle is the resting state, set automatically by Broadcaster on spawn. + AgentStateIdle AgentState = "idle" + + // AgentStateWaiting means the agent is waiting for an LLM response. + AgentStateWaiting AgentState = "waiting" + + // AgentStateToolCall means the agent is executing a tool. + // The tool name is carried in the Tool field of the event. + AgentStateToolCall AgentState = "toolcall" + + // AgentStatePlanInterviewing means the conductor is in plan-mode interview phase, + // clarifying goals and constraints with the user. + AgentStatePlanInterviewing AgentState = "plan_interviewing" + + // AgentStatePlanReview means the conductor has submitted a plan and is waiting + // for user approval before executing. + AgentStatePlanReview AgentState = "plan_review" + + // AgentStatePlanExecuting means the conductor is executing an approved plan. + AgentStatePlanExecuting AgentState = "plan_executing" + + // AgentStatePlanCompleted means all plan steps have been completed. + AgentStatePlanCompleted AgentState = "plan_completed" +) diff --git a/pkg/tools/registry.go b/pkg/tools/registry.go index 111f3c361..6fa7b0dea 100644 --- a/pkg/tools/registry.go +++ b/pkg/tools/registry.go @@ -227,8 +227,45 @@ func (r *ToolRegistry) GetRuntimeStatus() string { return strings.Join(parts, "\n\n") } +// buildParamHint extracts parameter names from a JSON schema and returns +// a hint string like "(task, label?, preset?)". Required params are bare, +// optional params have a trailing "?". +func buildParamHint(schema map[string]any) string { + props, _ := schema["properties"].(map[string]any) + if len(props) == 0 { + return "" + } + + reqSlice, _ := schema["required"].([]string) + reqSet := make(map[string]bool, len(reqSlice)) + for _, r := range reqSlice { + reqSet[r] = true + } + + names := make([]string, 0, len(props)) + for name := range props { + names = append(names, name) + } + sort.Strings(names) + + parts := make([]string, 0, len(names)) + // Required params first, then optional + for _, name := range names { + if reqSet[name] { + parts = append(parts, name) + } + } + for _, name := range names { + if !reqSet[name] { + parts = append(parts, name+"?") + } + } + + return "(" + strings.Join(parts, ", ") + ")" +} + // GetSummaries returns human-readable summaries of all registered tools. -// Returns a slice of "name - description" strings. +// Returns a slice of "- `name`(params) - description" strings. func (r *ToolRegistry) GetSummaries() []string { r.mu.RLock() defer r.mu.RUnlock() @@ -237,7 +274,8 @@ func (r *ToolRegistry) GetSummaries() []string { summaries := make([]string, 0, len(sorted)) for _, name := range sorted { tool := r.tools[name] - summaries = append(summaries, fmt.Sprintf("- `%s` - %s", tool.Name(), tool.Description())) + hint := buildParamHint(tool.Parameters()) + summaries = append(summaries, fmt.Sprintf("- `%s`%s - %s", tool.Name(), hint, tool.Description())) } return summaries } diff --git a/pkg/tools/registry_test.go b/pkg/tools/registry_test.go index 788a4935a..099b2baeb 100644 --- a/pkg/tools/registry_test.go +++ b/pkg/tools/registry_test.go @@ -337,6 +337,78 @@ func TestToolRegistry_Count(t *testing.T) { } } +func TestBuildParamHint(t *testing.T) { + tests := []struct { + name string + schema map[string]any + want string + }{ + { + name: "required and optional", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "task": map[string]any{"type": "string"}, + "label": map[string]any{"type": "string"}, + }, + "required": []string{"task"}, + }, + want: "(task, label?)", + }, + { + name: "all required", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "command": map[string]any{"type": "string"}, + }, + "required": []string{"command"}, + }, + want: "(command)", + }, + { + name: "no properties", + schema: map[string]any{ + "type": "object", + }, + want: "", + }, + { + name: "empty schema", + schema: map[string]any{}, + want: "", + }, + { + name: "nil schema", + schema: nil, + want: "", + }, + { + name: "multiple optional sorted", + schema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "task": map[string]any{"type": "string"}, + "preset": map[string]any{"type": "string"}, + "label": map[string]any{"type": "string"}, + "agent_id": map[string]any{"type": "string"}, + }, + "required": []string{"task"}, + }, + want: "(task, agent_id?, label?, preset?)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := buildParamHint(tt.schema) + if got != tt.want { + t.Errorf("buildParamHint() = %q, want %q", got, tt.want) + } + }) + } +} + func TestToolRegistry_GetSummaries(t *testing.T) { r := NewToolRegistry() r.Register(newMockTool("read_file", "Reads a file")) @@ -353,6 +425,32 @@ func TestToolRegistry_GetSummaries(t *testing.T) { } } +func TestToolRegistry_GetSummaries_WithParamHint(t *testing.T) { + r := NewToolRegistry() + r.Register(&mockRegistryTool{ + name: "spawn", + desc: "Spawn a subagent", + params: map[string]any{ + "type": "object", + "properties": map[string]any{ + "task": map[string]any{"type": "string"}, + "preset": map[string]any{"type": "string"}, + }, + "required": []string{"task"}, + }, + result: SilentResult("ok"), + }) + + summaries := r.GetSummaries() + if len(summaries) != 1 { + t.Fatalf("expected 1 summary, got %d", len(summaries)) + } + // Should contain param hint + if !strings.Contains(summaries[0], "(task, preset?)") { + t.Errorf("expected param hint in summary, got %q", summaries[0]) + } +} + func TestToolToSchema(t *testing.T) { tool := newMockTool("demo", "demo tool") schema := ToolToSchema(tool) diff --git a/pkg/tools/sandbox.go b/pkg/tools/sandbox.go index 3f5d0455f..4a6306393 100644 --- a/pkg/tools/sandbox.go +++ b/pkg/tools/sandbox.go @@ -22,7 +22,8 @@ func IsValidPreset(p Preset) bool { // ExecPolicy defines which commands are allowed for execution. type ExecPolicy struct { - AllowPattern string // Prefix-match regex; matched commands are allowed + AllowRules []string // Command prefix allowlist (e.g., "go test", "pnpm run test") + LocalNetOnly bool // Restrict curl/wget to localhost and RFC 1918 private addresses } // SandboxConfig describes the sandbox isolation policy for a preset. @@ -43,13 +44,43 @@ type SubagentEnvironment struct { ContextFiles []string // Files to provide as context } -// presetExecPatterns maps presets to command allowlist regexes. -var presetExecPatterns = map[Preset]string{ - PresetScout: ``, // No exec allowed - PresetAnalyst: `^(go\s+(test|vet)|git\s+(log|diff|status)|curl|wget|grep|find)\b`, - PresetCoder: `^(go\s+(test|vet|fmt)|gofmt|goimports|golangci-lint|prettier|eslint|black|ruff|cargo\s+(test|fmt|clippy)|pnpm\s+(test|run\s+(test|lint|format))|bun\s+(test|run\s+(test|lint|format))|uv\s+run\s+)\b`, - PresetWorker: `^(go\s+|pnpm\s+(install|add|run|test|build)|bun\s+(install|add|run|test|build)|uv\s+(run|sync|add|pip\s+install)|pip\s+install|cargo\s+)\b`, - PresetCoordinator: `^(go\s+|pnpm\s+|bun\s+|curl|wget)\b`, +// presetAllowRules maps presets to command prefix allowlists. +// Each entry is a command prefix: the first N words of the executed command +// must match exactly. e.g. "go test" allows "go test ./..." but not "go build". +// A single word like "curl" allows any arguments. +// curl/wget are included where exec is allowed; LocalNetOnly in ExecPolicy +// ensures all curl/wget requests are restricted to localhost and RFC 1918 addresses. +var presetAllowRules = map[Preset][]string{ + PresetScout: nil, // No exec allowed + PresetAnalyst: { + "go test", "go vet", + "git log", "git diff", "git status", + "curl", "wget", "grep", "find", + }, + PresetCoder: { + "go test", "go vet", "go fmt", + "gofmt", "goimports", "golangci-lint", + "prettier", "eslint", "black", "ruff", + "cargo test", "cargo fmt", "cargo clippy", + "pnpm test", "pnpm run test", "pnpm run lint", "pnpm run format", + "bun test", "bun run test", "bun run lint", "bun run format", + "uv run", + "curl", "wget", + }, + PresetWorker: { + "go", + "pnpm install", "pnpm add", "pnpm run", "pnpm test", "pnpm build", + "bun install", "bun add", "bun run", "bun test", "bun build", + "uv run", "uv sync", "uv add", "uv pip install", + "pip install", + "cargo", + "curl", "wget", + }, + PresetCoordinator: { + "go", + "pnpm", "bun", + "curl", "wget", + }, } // presetSpawnablePresets maps presets to which presets they can spawn. @@ -107,11 +138,14 @@ func SandboxConfigForPreset(p Preset, writeRoot string) SandboxConfig { config.WriteRoot = writeRoot } - // Set ExecPolicy if exec is allowed and pattern is non-empty + // Set ExecPolicy if exec is allowed and rules are defined. + // LocalNetOnly is always true: curl/wget in subagents is for local server + // testing only; external HTTP access goes through the web_fetch tool. if allowed["exec"] { - if pattern := presetExecPatterns[p]; pattern != "" { + if rules := presetAllowRules[p]; len(rules) > 0 { config.ExecPolicy = &ExecPolicy{ - AllowPattern: pattern, + AllowRules: rules, + LocalNetOnly: true, } } } diff --git a/pkg/tools/sandbox_test.go b/pkg/tools/sandbox_test.go index c63d93e72..84243aeb9 100644 --- a/pkg/tools/sandbox_test.go +++ b/pkg/tools/sandbox_test.go @@ -1,7 +1,6 @@ package tools import ( - "regexp" "testing" ) @@ -128,6 +127,8 @@ func TestSandboxConfigForPreset_Coder(t *testing.T) { } if config.ExecPolicy == nil { t.Errorf("ExecPolicy: got nil, want non-nil") + } else if !config.ExecPolicy.LocalNetOnly { + t.Errorf("ExecPolicy.LocalNetOnly: got false, want true") } if config.SpawnablePresets != nil { t.Errorf("SpawnablePresets: got non-nil, want nil") @@ -146,6 +147,8 @@ func TestSandboxConfigForPreset_Coordinator(t *testing.T) { } if config.ExecPolicy == nil { t.Errorf("ExecPolicy: got nil, want non-nil") + } else if !config.ExecPolicy.LocalNetOnly { + t.Errorf("ExecPolicy.LocalNetOnly: got false, want true") } if config.SpawnablePresets == nil { t.Errorf("SpawnablePresets: got nil, want non-nil") @@ -163,17 +166,18 @@ func TestSandboxConfigForPreset_Coordinator(t *testing.T) { } } -// TestPresetExecPatterns_Coder validates coder exec allowlist. -func TestPresetExecPatterns_Coder(t *testing.T) { - pattern, ok := presetExecPatterns[PresetCoder] - if !ok || pattern == "" { - t.Fatalf("coder pattern missing or empty") +// TestPresetAllowRules_Coder validates coder exec allowlist. +func TestPresetAllowRules_Coder(t *testing.T) { + rules := presetAllowRules[PresetCoder] + if len(rules) == 0 { + t.Fatalf("coder rules missing or empty") } - re, err := regexp.Compile(pattern) + exec, err := NewExecTool(t.TempDir(), true) if err != nil { - t.Fatalf("failed to compile pattern: %v", err) + t.Fatalf("NewExecTool: %v", err) } + exec.SetAllowRules(rules) tests := []struct { cmd string @@ -181,32 +185,53 @@ func TestPresetExecPatterns_Coder(t *testing.T) { }{ {"go test ./...", true}, {"go vet ./...", true}, + {"go fmt ./...", true}, {"gofmt -w file.go", true}, {"golangci-lint run", true}, + {"cargo test", true}, + {"cargo fmt", true}, + {"cargo clippy", true}, + {"pnpm test", true}, + {"pnpm run test", true}, + {"pnpm run lint", true}, + {"pnpm run format", true}, + {"bun test", true}, + {"bun run test", true}, + {"uv run pytest", true}, + // curl/wget are in the allowlist; LocalNetOnly enforcement is at runtime + {"curl http://localhost:3000/health", true}, + {"wget http://127.0.0.1:8080/status", true}, + // blocked {"go build ./...", false}, {"npm install", false}, - {"pnpm test", true}, + {"pnpm install", false}, + {"pnpm run build", false}, + {"cargo build", false}, + {"pwd", false}, + {"ls", false}, } for _, tt := range tests { - gotOK := re.MatchString(tt.cmd) + result := exec.guardCommand(tt.cmd, t.TempDir()) + gotOK := result == "" if gotOK != tt.wantOK { - t.Errorf("cmd %q: got %v, want %v", tt.cmd, gotOK, tt.wantOK) + t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result) } } } -// TestPresetExecPatterns_Analyst validates analyst exec allowlist. -func TestPresetExecPatterns_Analyst(t *testing.T) { - pattern, ok := presetExecPatterns[PresetAnalyst] - if !ok || pattern == "" { - t.Fatalf("analyst pattern missing or empty") +// TestPresetAllowRules_Analyst validates analyst exec allowlist. +func TestPresetAllowRules_Analyst(t *testing.T) { + rules := presetAllowRules[PresetAnalyst] + if len(rules) == 0 { + t.Fatalf("analyst rules missing or empty") } - re, err := regexp.Compile(pattern) + exec, err := NewExecTool(t.TempDir(), true) if err != nil { - t.Fatalf("failed to compile pattern: %v", err) + t.Fatalf("NewExecTool: %v", err) } + exec.SetAllowRules(rules) tests := []struct { cmd string @@ -216,16 +241,109 @@ func TestPresetExecPatterns_Analyst(t *testing.T) { {"go vet ./...", true}, {"git log --oneline", true}, {"git diff HEAD", true}, + {"git status", true}, {"grep pattern file", true}, + {"find . -name '*.go'", true}, {"curl http://example.com", true}, + // blocked {"go build ./...", false}, {"npm install", false}, + {"git push", false}, + {"git checkout", false}, + {"pwd", false}, + {"ls", false}, } for _, tt := range tests { - gotOK := re.MatchString(tt.cmd) + result := exec.guardCommand(tt.cmd, t.TempDir()) + gotOK := result == "" if gotOK != tt.wantOK { - t.Errorf("cmd %q: got %v, want %v", tt.cmd, gotOK, tt.wantOK) + t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result) + } + } +} + +// TestPresetAllowRules_Worker validates worker exec allowlist. +func TestPresetAllowRules_Worker(t *testing.T) { + rules := presetAllowRules[PresetWorker] + if len(rules) == 0 { + t.Fatalf("worker rules missing or empty") + } + + exec, err := NewExecTool(t.TempDir(), true) + if err != nil { + t.Fatalf("NewExecTool: %v", err) + } + exec.SetAllowRules(rules) + + tests := []struct { + cmd string + wantOK bool + }{ + {"go build ./...", true}, + {"go test ./...", true}, + {"pnpm install", true}, + {"pnpm add lodash", true}, + {"pnpm run dev", true}, + {"bun install", true}, + {"bun build", true}, + {"uv run pytest", true}, + {"uv sync", true}, + {"uv pip install flask", true}, + {"pip install flask", true}, + {"cargo build", true}, + {"cargo test", true}, + {"curl http://localhost:8080", true}, + // blocked + {"npm install", false}, + {"pwd", false}, + } + + for _, tt := range tests { + result := exec.guardCommand(tt.cmd, t.TempDir()) + gotOK := result == "" + if gotOK != tt.wantOK { + t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result) + } + } +} + +// TestPresetAllowRules_Coordinator validates coordinator exec allowlist. +func TestPresetAllowRules_Coordinator(t *testing.T) { + rules := presetAllowRules[PresetCoordinator] + if len(rules) == 0 { + t.Fatalf("coordinator rules missing or empty") + } + + exec, err := NewExecTool(t.TempDir(), true) + if err != nil { + t.Fatalf("NewExecTool: %v", err) + } + exec.SetAllowRules(rules) + + tests := []struct { + cmd string + wantOK bool + }{ + {"go build ./...", true}, + {"go test ./...", true}, + {"pnpm install", true}, + {"pnpm run dev", true}, + {"bun install", true}, + {"bun run dev", true}, + {"curl http://localhost:8080", true}, + {"wget http://127.0.0.1:3000", true}, + // blocked + {"npm install", false}, + {"cargo build", false}, + {"pwd", false}, + } + + for _, tt := range tests { + result := exec.guardCommand(tt.cmd, t.TempDir()) + gotOK := result == "" + if gotOK != tt.wantOK { + t.Errorf("cmd %q: got allowed=%v, want %v (guard: %q)", tt.cmd, gotOK, tt.wantOK, result) } } } diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 9f7091614..dd8a7f29d 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -6,6 +6,8 @@ import ( "errors" "fmt" "io" + "net" + "net/url" "os" "os/exec" "path/filepath" @@ -120,8 +122,9 @@ type ExecTool struct { workingDir string timeout time.Duration denyPatterns []*regexp.Regexp - allowPatterns []*regexp.Regexp + allowRules [][]string // pre-split command prefix allowlist restrictToWorkspace bool + localNetOnly bool // restrict curl/wget to localhost + RFC 1918 // Background process management bgMu sync.Mutex @@ -214,7 +217,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf workingDir: workingDir, timeout: 5 * time.Minute, denyPatterns: denyPatterns, - allowPatterns: nil, + allowRules: nil, restrictToWorkspace: restrict, bgProcesses: make(map[string]*bgProcess), bgCtx: bgCtx, @@ -705,28 +708,29 @@ func (t *ExecTool) guardCommand(command, cwd string) string { } } - if len(t.allowPatterns) > 0 { - allowed := false - for _, pattern := range t.allowPatterns { - if pattern.MatchString(lower) { - allowed = true - break - } - } - if !allowed { + if len(t.allowRules) > 0 { + if !matchAllowRules(lower, t.allowRules) { var b strings.Builder b.WriteString("Command blocked: not in allowlist [") - for i, p := range t.allowPatterns { + for i, rule := range t.allowRules { if i > 0 { b.WriteByte(',') } - b.WriteString(p.String()) + b.WriteString(strings.Join(rule, " ")) } b.WriteByte(']') return b.String() } } + // Restrict curl/wget to localhost and RFC 1918 private addresses. + // External HTTP access is available via the web_fetch tool. + if t.localNetOnly && isCurlOrWget(cmd) { + if errMsg := checkCurlLocalNet(cmd); errMsg != "" { + return errMsg + } + } + if t.restrictToWorkspace { if strings.Contains(cmd, "..\\") || strings.Contains(cmd, "../") { return "Command blocked by safety guard (path traversal detected)" @@ -766,6 +770,12 @@ func (t *ExecTool) guardCommand(command, cwd string) string { if isExecutable(p) { continue } + // Allow /dev/* paths (e.g. /dev/null, /dev/urandom). + // Device files are not regular filesystem paths and pose + // no workspace-escape risk. + if strings.HasPrefix(p, "/dev/") { + continue + } // Agent CLI slash commands: skip non-existent paths // (e.g., "/review" is a command, not a file). if agentCLI { @@ -830,16 +840,90 @@ func (t *ExecTool) SetRestrictToWorkspace(restrict bool) { t.restrictToWorkspace = restrict } -func (t *ExecTool) SetAllowPatterns(patterns []string) error { - t.allowPatterns = make([]*regexp.Regexp, 0, len(patterns)) - for _, p := range patterns { - re, err := regexp.Compile(p) - if err != nil { - return fmt.Errorf("invalid allow pattern %q: %w", p, err) +// SetAllowRules sets the command prefix allowlist. +// Each rule is a space-separated command prefix (e.g. "go test", "pnpm run lint"). +// A command is allowed if its first N words match any rule's N words exactly. +func (t *ExecTool) SetAllowRules(rules []string) { + t.allowRules = make([][]string, 0, len(rules)) + for _, r := range rules { + words := strings.Fields(strings.ToLower(r)) + if len(words) > 0 { + t.allowRules = append(t.allowRules, words) } - t.allowPatterns = append(t.allowPatterns, re) } - return nil +} + +// matchAllowRules checks if cmd matches any prefix in the allowlist. +func matchAllowRules(cmd string, rules [][]string) bool { + cmdWords := strings.Fields(cmd) + for _, ruleWords := range rules { + if len(cmdWords) < len(ruleWords) { + continue + } + match := true + for i, rw := range ruleWords { + if cmdWords[i] != rw { + match = false + break + } + } + if match { + return true + } + } + return false +} + +func (t *ExecTool) SetLocalNetOnly(v bool) { + t.localNetOnly = v +} + +// isCurlOrWget reports whether command is a curl or wget invocation. +func isCurlOrWget(command string) bool { + fields := strings.Fields(command) + if len(fields) == 0 { + return false + } + base := filepath.Base(fields[0]) + return base == "curl" || base == "wget" +} + +// checkCurlLocalNet validates that all http/https URLs in a curl/wget command +// target localhost or RFC 1918 private addresses. +// Returns an error message string, or empty string if the command is allowed. +func checkCurlLocalNet(command string) string { + for _, token := range strings.Fields(command) { + token = strings.Trim(token, "\"'") + if !strings.HasPrefix(token, "http://") && !strings.HasPrefix(token, "https://") { + continue + } + u, err := url.Parse(token) + if err != nil { + continue + } + host := u.Hostname() + if !isLocalHost(host) { + return fmt.Sprintf( + "Command blocked by safety guard "+ + "(curl/wget is restricted to localhost and private network; %q is a public address)", + host, + ) + } + } + return "" +} + +// isLocalHost reports whether host is localhost or a loopback/RFC 1918 private IP. +// DNS resolution is intentionally avoided to prevent DNS rebinding attacks. +func isLocalHost(host string) bool { + if strings.EqualFold(host, "localhost") { + return true + } + ip := net.ParseIP(host) + if ip == nil { + return false + } + return ip.IsLoopback() || ip.IsPrivate() } // SetBgMaxLifetimeForTest overrides bgMaxLifetime for testing purposes. diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 6bf7e05a9..2203beffd 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -573,15 +573,12 @@ func TestGuardCommand_DenyPattern_IncludesPattern(t *testing.T) { } } -// TestGuardCommand_Allowlist_ShowsPatterns verifies that allowlist violation -// messages include all configured patterns. -func TestGuardCommand_Allowlist_ShowsPatterns(t *testing.T) { +// TestGuardCommand_Allowlist_ShowsRules verifies that allowlist violation +// messages include all configured rules. +func TestGuardCommand_Allowlist_ShowsRules(t *testing.T) { workspace := t.TempDir() tool, _ := NewExecTool(workspace, true) - err := tool.SetAllowPatterns([]string{`^go\b`, `^git\b`}) - if err != nil { - t.Fatalf("SetAllowPatterns failed: %v", err) - } + tool.SetAllowRules([]string{"go test", "git"}) result := tool.guardCommand("curl http://example.com", workspace) if result == "" { @@ -590,8 +587,8 @@ func TestGuardCommand_Allowlist_ShowsPatterns(t *testing.T) { if !strings.Contains(result, "not in allowlist") { t.Errorf("expected 'not in allowlist' in message, got: %s", result) } - if !strings.Contains(result, `^go\b`) || !strings.Contains(result, `^git\b`) { - t.Errorf("expected allowlist patterns in message, got: %s", result) + if !strings.Contains(result, "go test") || !strings.Contains(result, "git") { + t.Errorf("expected allowlist rules in message, got: %s", result) } } @@ -983,3 +980,97 @@ func TestExecTool_Bg_RingBufferOverflow(t *testing.T) { t.Errorf("ring buffer exceeded max size: %d > %d", bufLen, bgRingBufSize) } } + +// TestIsLocalHost verifies localhost and RFC 1918 detection using net package. +func TestIsLocalHost(t *testing.T) { + tests := []struct { + host string + want bool + }{ + // Loopback / localhost + {"localhost", true}, + {"LOCALHOST", true}, + {"127.0.0.1", true}, + {"127.0.0.2", true}, + {"::1", true}, + // RFC 1918 private ranges + {"10.0.0.1", true}, + {"10.255.255.255", true}, + {"172.16.0.1", true}, + {"172.31.255.255", true}, + {"192.168.0.1", true}, + {"192.168.1.100", true}, + // Public addresses + {"8.8.8.8", false}, + {"1.1.1.1", false}, + {"example.com", false}, + {"api.github.com", false}, + // Edge: non-private but routable private-looking address + {"172.15.255.255", false}, // just below 172.16/12 + {"172.32.0.0", false}, // just above 172.31/12 + } + + for _, tt := range tests { + got := isLocalHost(tt.host) + if got != tt.want { + t.Errorf("isLocalHost(%q) = %v, want %v", tt.host, got, tt.want) + } + } +} + +// TestCheckCurlLocalNet verifies URL-level enforcement for curl/wget commands. +func TestCheckCurlLocalNet(t *testing.T) { + tests := []struct { + cmd string + wantErr bool + }{ + // Allowed: localhost and private IPs + {"curl http://localhost:3000/health", false}, + {"curl -v http://127.0.0.1:8080/api/status", false}, + {"wget http://192.168.1.10/file.bin", false}, + {"curl -X POST http://10.0.0.5:9000/webhook", false}, + // Blocked: public addresses + {"curl http://example.com", true}, + {"wget https://releases.github.com/v1.tar.gz", true}, + {"curl http://8.8.8.8/data", true}, + // Allowed: no http URL (e.g. --help, --version — no network access) + {"curl --help", false}, + {"curl --version", false}, + {"wget --help", false}, + } + + for _, tt := range tests { + errMsg := checkCurlLocalNet(tt.cmd) + gotErr := errMsg != "" + if gotErr != tt.wantErr { + t.Errorf("checkCurlLocalNet(%q): gotErr=%v wantErr=%v (msg: %q)", + tt.cmd, gotErr, tt.wantErr, errMsg) + } + } +} + +// TestExecTool_LocalNetOnly verifies curl/wget blocking via SetLocalNetOnly. +func TestExecTool_LocalNetOnly(t *testing.T) { + tool, _ := NewExecTool("", false) + tool.SetLocalNetOnly(true) + + tests := []struct { + cmd string + wantErr bool + }{ + {"curl http://localhost:3000", false}, + {"curl http://example.com", true}, + {"echo hello", false}, // non-curl not affected + } + + ctx := context.Background() + for _, tt := range tests { + result := tool.Execute(ctx, map[string]any{"command": tt.cmd}) + if tt.wantErr && !result.IsError { + t.Errorf("cmd %q: expected blocked, but succeeded", tt.cmd) + } + if !tt.wantErr && result.IsError && strings.Contains(result.ForLLM, "safety guard") { + t.Errorf("cmd %q: expected allowed, but safety guard blocked: %s", tt.cmd, result.ForLLM) + } + } +} diff --git a/pkg/tools/spawn.go b/pkg/tools/spawn.go index 7ef9ca489..e83157616 100644 --- a/pkg/tools/spawn.go +++ b/pkg/tools/spawn.go @@ -32,7 +32,7 @@ func (t *SpawnTool) Name() string { } func (t *SpawnTool) Description() string { - return "Spawn a subagent to handle a task in the background. Use this for complex or time-consuming tasks that can run independently. The subagent will complete the task and report back when done." + return "Spawn a subagent that runs NON-BLOCKING in the background and returns immediately. Prefer this over subagent for any task that can run independently. Use preset to control capabilities (scout, analyst, coder, worker, coordinator)." } func (t *SpawnTool) Parameters() map[string]any { @@ -73,26 +73,35 @@ func (t *SpawnTool) SetAllowlistChecker(check func(targetAgentID string) bool) { func (t *SpawnTool) Execute(ctx context.Context, args map[string]any) *ToolResult { task, ok := args["task"].(string) if !ok || strings.TrimSpace(task) == "" { - return ErrorResult("task is required and must be a non-empty string") + return ErrorResult( + `Required parameter "task" (string) is missing. ` + + `Example: {"task": "describe what you need done", "preset": "scout"}`, + ) } label, _ := args["label"].(string) agentID, _ := args["agent_id"].(string) preset, _ := args["preset"].(string) - // Check allowlist if targeting a specific agent or preset - checkTarget := agentID - if checkTarget == "" && preset != "" { - checkTarget = preset - } - if checkTarget != "" && t.allowlistCheck != nil { - if !t.allowlistCheck(checkTarget) { - return ErrorResult(fmt.Sprintf("not allowed to spawn agent '%s' or preset '%s'", agentID, preset)) + // Check allowlist if targeting a specific agent ID. + // Presets (scout, analyst, etc.) are NOT agent IDs — they are validated + // separately by IsValidPreset() in the subagent manager. + if agentID != "" && t.allowlistCheck != nil { + if !t.allowlistCheck(agentID) { + return ErrorResult(fmt.Sprintf("agent %q is not in the allowed agents list", agentID)) } } + // Validate preset name if provided + if preset != "" && !IsValidPreset(Preset(preset)) { + return ErrorResult(fmt.Sprintf( + "preset %q is not valid. Available presets: scout, analyst, coder, worker, coordinator", + preset, + )) + } + if t.manager == nil { - return ErrorResult("Subagent manager not configured") + return ErrorResult("spawn tool is not available in this session (orchestration may be disabled)") } // Pass callback to manager for async completion notification diff --git a/pkg/tools/spawn_test.go b/pkg/tools/spawn_test.go index b5652784a..f36542ec1 100644 --- a/pkg/tools/spawn_test.go +++ b/pkg/tools/spawn_test.go @@ -33,8 +33,8 @@ func TestSpawnTool_Execute_EmptyTask(t *testing.T) { if !result.IsError { t.Error("Expected error for invalid task parameter") } - if !strings.Contains(result.ForLLM, "task is required") { - t.Errorf("Error message should mention 'task is required', got: %s", result.ForLLM) + if !strings.Contains(result.ForLLM, `"task"`) { + t.Errorf("Error message should mention '\"task\"', got: %s", result.ForLLM) } }) } @@ -73,7 +73,7 @@ func TestSpawnTool_Execute_NilManager(t *testing.T) { if !result.IsError { t.Error("Expected error for nil manager") } - if !strings.Contains(result.ForLLM, "Subagent manager not configured") { - t.Errorf("Error message should mention manager not configured, got: %s", result.ForLLM) + if !strings.Contains(result.ForLLM, "spawn tool is not available") { + t.Errorf("Error message should mention spawn tool not available, got: %s", result.ForLLM) } } diff --git a/pkg/tools/subagent.go b/pkg/tools/subagent.go index 08cbb0622..5dd9853ee 100644 --- a/pkg/tools/subagent.go +++ b/pkg/tools/subagent.go @@ -3,7 +3,9 @@ package tools import ( "context" "fmt" - "log" + "sort" + "strconv" + "strings" "sync" "time" @@ -22,6 +24,10 @@ type SubagentTask struct { Status string Result string Created int64 + CompletedAt int64 `json:"-"` + Iterations int `json:"-"` + ToolCalls int `json:"-"` + ToolStats map[string]int `json:"-"` } type SubagentManager struct { @@ -33,7 +39,6 @@ type SubagentManager struct { workspace string tools *ToolRegistry webSearchOpts WebSearchToolOptions - execTool *ExecTool // Shared exec tool for all presets maxIterations int maxTokens int temperature float64 @@ -53,11 +58,6 @@ func NewSubagentManager( if reporter == nil { reporter = orch.Noop } - // Create a shared exec tool for all presets - execTool, err := NewExecTool(workspace, true) - if err != nil { - log.Printf("subagent: failed to create exec tool: %v (exec disabled for subagents)", err) - } return &SubagentManager{ tasks: make(map[string]*SubagentTask), provider: provider, @@ -66,7 +66,6 @@ func NewSubagentManager( workspace: workspace, tools: NewToolRegistry(), webSearchOpts: webSearchOpts, - execTool: execTool, maxIterations: 10, nextID: 1, reporter: reporter, @@ -249,14 +248,19 @@ After completing, provide a clear summary of what was done and how it was verifi } else { task.Status = "completed" task.Result = loopResult.Content + task.CompletedAt = time.Now().UnixMilli() + task.Iterations = loopResult.Iterations + task.ToolCalls = loopResult.ToolCalls + task.ToolStats = loopResult.ToolStats // Notify conductor of the result sm.reporter.ReportConversation(task.ID, "conductor", loopResult.Content) sm.reporter.ReportGC(task.ID, "completed") result = &ToolResult{ ForLLM: fmt.Sprintf( - "Subagent '%s' completed (iterations: %d): %s", + "Subagent '%s' completed (iterations: %d, tool calls: %d): %s", task.Label, loopResult.Iterations, + loopResult.ToolCalls, loopResult.Content, ), ForUser: loopResult.Content, @@ -269,14 +273,23 @@ After completing, provide a clear summary of what was done and how it was verifi // Send announce message back to main agent if sm.bus != nil { announceContent := fmt.Sprintf("Task '%s' completed.\n\nResult:\n%s", task.Label, task.Result) + metadata := map[string]string{ + "duration_ms": strconv.FormatInt(task.CompletedAt-task.Created, 10), + "iterations": strconv.Itoa(task.Iterations), + "tool_calls": strconv.Itoa(task.ToolCalls), + } + if len(task.ToolStats) > 0 { + metadata["tool_stats"] = formatToolStats(task.ToolStats) + } pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second) defer pubCancel() sm.bus.PublishInbound(pubCtx, bus.InboundMessage{ Channel: "system", SenderID: fmt.Sprintf("subagent:%s", task.ID), // Format: "original_channel:original_chat_id" for routing back - ChatID: fmt.Sprintf("%s:%s", task.OriginChannel, task.OriginChatID), - Content: announceContent, + ChatID: fmt.Sprintf("%s:%s", task.OriginChannel, task.OriginChatID), + Content: announceContent, + Metadata: metadata, }) } } @@ -306,12 +319,22 @@ func (sm *SubagentManager) buildPresetRegistry(preset Preset, writeRoot string) registry.Register(NewAppendFileTool(writeRoot, true)) } - // Register exec and bg_monitor if allowed + // Register exec and bg_monitor if allowed. + // Each subagent gets its own ExecTool to avoid mutating the shared instance's + // allowRules (which would leak sandbox restrictions to the conductor). if config.AllowedTools["exec"] { - // Use the shared exec tool but set allow patterns - execTool := sm.execTool + execWorkDir := writeRoot + if execWorkDir == "" { + execWorkDir = sm.workspace + } + execTool, err := NewExecTool(execWorkDir, true) + if err != nil { + // exec disabled for this subagent; skip registration + return registry + } if config.ExecPolicy != nil { - _ = execTool.SetAllowPatterns([]string{config.ExecPolicy.AllowPattern}) + execTool.SetAllowRules(config.ExecPolicy.AllowRules) + execTool.SetLocalNetOnly(config.ExecPolicy.LocalNetOnly) } registry.Register(execTool) @@ -383,7 +406,7 @@ func (t *SubagentTool) Name() string { } func (t *SubagentTool) Description() string { - return "Execute a subagent task synchronously and return the result. Use this for delegating specific tasks to an independent agent instance. Returns execution summary to user and full details to LLM." + return "Run a task in a subagent and BLOCK until it completes, returning the result directly. Use when you need the answer before deciding your next step. For background/parallel tasks, use spawn instead." } func (t *SubagentTool) Parameters() map[string]any { @@ -411,13 +434,17 @@ func (t *SubagentTool) SetContext(channel, chatID string) { func (t *SubagentTool) Execute(ctx context.Context, args map[string]any) *ToolResult { task, ok := args["task"].(string) if !ok { - return ErrorResult("task is required").WithError(fmt.Errorf("task parameter is required")) + return ErrorResult( + `Required parameter "task" (string) is missing. ` + + `Example: {"task": "describe what you need done"}`, + ).WithError(fmt.Errorf("task parameter is required")) } label, _ := args["label"].(string) if t.manager == nil { - return ErrorResult("Subagent manager not configured").WithError(fmt.Errorf("manager is nil")) + return ErrorResult("subagent tool is not available in this session (orchestration may be disabled)"). + WithError(fmt.Errorf("manager is nil")) } // Build messages for subagent @@ -477,8 +504,8 @@ func (t *SubagentTool) Execute(ctx context.Context, args map[string]any) *ToolRe if labelStr == "" { labelStr = "(unnamed)" } - llmContent := fmt.Sprintf("Subagent task completed:\nLabel: %s\nIterations: %d\nResult: %s", - labelStr, loopResult.Iterations, loopResult.Content) + llmContent := fmt.Sprintf("Subagent task completed:\nLabel: %s\nIterations: %d\nTool calls: %d\nResult: %s", + labelStr, loopResult.Iterations, loopResult.ToolCalls, loopResult.Content) return &ToolResult{ ForLLM: llmContent, @@ -488,3 +515,18 @@ func (t *SubagentTool) Execute(ctx context.Context, args map[string]any) *ToolRe Async: false, } } + +// formatToolStats formats a tool stats map as a compact string: "exec:3,read_file:5". +// Keys are sorted alphabetically for deterministic output. +func formatToolStats(stats map[string]int) string { + keys := make([]string, 0, len(stats)) + for k := range stats { + keys = append(keys, k) + } + sort.Strings(keys) + parts := make([]string, 0, len(keys)) + for _, k := range keys { + parts = append(parts, k+":"+strconv.Itoa(stats[k])) + } + return strings.Join(parts, ",") +} diff --git a/pkg/tools/subagent_tool_test.go b/pkg/tools/subagent_tool_test.go index 6291724dc..481ea394e 100644 --- a/pkg/tools/subagent_tool_test.go +++ b/pkg/tools/subagent_tool_test.go @@ -4,6 +4,7 @@ import ( "context" "strings" "testing" + "time" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/orch" @@ -93,8 +94,11 @@ func TestSubagentTool_Description(t *testing.T) { if desc == "" { t.Error("Description should not be empty") } - if !strings.Contains(desc, "subagent") { - t.Errorf("Description should mention 'subagent', got: %s", desc) + if !strings.Contains(desc, "BLOCK") { + t.Errorf("Description should mention 'BLOCK', got: %s", desc) + } + if !strings.Contains(desc, "spawn") { + t.Errorf("Description should contrast with spawn, got: %s", desc) } } @@ -259,9 +263,12 @@ func TestSubagentTool_Execute_MissingTask(t *testing.T) { t.Error("Expected error for missing task parameter") } - // ForLLM should contain error message - if !strings.Contains(result.ForLLM, "task is required") { - t.Errorf("Error message should mention 'task is required', got: %s", result.ForLLM) + // ForLLM should contain helpful error with example + if !strings.Contains(result.ForLLM, `"task"`) { + t.Errorf("Error message should mention '\"task\"', got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "Example") { + t.Errorf("Error message should include usage example, got: %s", result.ForLLM) } // Err should be set @@ -286,8 +293,8 @@ func TestSubagentTool_Execute_NilManager(t *testing.T) { t.Error("Expected error for nil manager") } - if !strings.Contains(result.ForLLM, "Subagent manager not configured") { - t.Errorf("Error message should mention manager not configured, got: %s", result.ForLLM) + if !strings.Contains(result.ForLLM, "not available in this session") { + t.Errorf("Error message should mention 'not available in this session', got: %s", result.ForLLM) } } @@ -349,3 +356,69 @@ func TestSubagentTool_ForUserTruncation(t *testing.T) { t.Error("ForLLM should contain reference to original task") } } + +func TestFormatToolStats(t *testing.T) { + tests := []struct { + name string + stats map[string]int + want string + }{ + {"empty", map[string]int{}, ""}, + {"single", map[string]int{"exec": 3}, "exec:3"}, + { + "multiple sorted", + map[string]int{"read_file": 5, "exec": 3, "write_file": 1}, + "exec:3,read_file:5,write_file:1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := formatToolStats(tt.stats) + if got != tt.want { + t.Errorf("formatToolStats(%v) = %q, want %q", tt.stats, got, tt.want) + } + }) + } +} + +// TestSubagentManager_Spawn_SetsMetadata verifies that the bus message from a +// completed spawn includes execution statistics in Metadata. +func TestSubagentManager_Spawn_SetsMetadata(t *testing.T) { + provider := &MockLLMProvider{} + msgBus := bus.NewMessageBus() + mgr := NewSubagentManager(provider, "test-model", "/tmp/test", msgBus, orch.Noop, WebSearchToolOptions{}) + + _, err := mgr.Spawn( + context.Background(), + "say hello", "meta-test", "", "cli", "direct", "", + nil, + ) + if err != nil { + t.Fatalf("Spawn() error: %v", err) + } + + // Consume the inbound message from the bus + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + received, ok := msgBus.ConsumeInbound(ctx) + if !ok { + t.Fatal("timed out waiting for bus message") + } + + if received.Channel != "system" { + t.Fatalf("expected channel 'system', got %q", received.Channel) + } + if received.Metadata == nil { + t.Fatal("Metadata should not be nil") + } + if received.Metadata["iterations"] != "1" { + t.Errorf("iterations = %q, want %q", received.Metadata["iterations"], "1") + } + if received.Metadata["tool_calls"] != "0" { + t.Errorf("tool_calls = %q, want %q", received.Metadata["tool_calls"], "0") + } + // duration_ms should be a non-negative number + if received.Metadata["duration_ms"] == "" { + t.Error("duration_ms should be present") + } +} diff --git a/pkg/tools/toolloop.go b/pkg/tools/toolloop.go index d8793caef..6351879a0 100644 --- a/pkg/tools/toolloop.go +++ b/pkg/tools/toolloop.go @@ -36,6 +36,8 @@ type ToolLoopConfig struct { type ToolLoopResult struct { Content string Iterations int + ToolCalls int // total tool call count across all iterations + ToolStats map[string]int // tool name → call count } // RunToolLoop executes the LLM + tool call iteration loop. @@ -52,6 +54,8 @@ func RunToolLoop( } iteration := 0 + totalToolCalls := 0 + toolStats := map[string]int{} var finalContent string for iteration < config.MaxIterations { @@ -75,7 +79,7 @@ func RunToolLoop( llmOpts = map[string]any{} } // 3. Call LLM (hook: waiting for response) - reporter.ReportStateChange(config.AgentID, "waiting", "") + reporter.ReportStateChange(config.AgentID, orch.AgentStateWaiting, "") response, err := config.Provider.Chat(ctx, messages, providerToolDefs, config.Model, llmOpts) if err != nil { logger.ErrorCF("toolloop", "LLM call failed", @@ -143,7 +147,9 @@ func RunToolLoop( "tool": tc.Name, "iteration": iteration, }) - reporter.ReportStateChange(config.AgentID, "toolcall", tc.Name) + reporter.ReportStateChange(config.AgentID, orch.AgentStateToolCall, tc.Name) + totalToolCalls++ + toolStats[tc.Name]++ // Execute tool (no async callback for subagents - they run independently) var toolResult *ToolResult @@ -172,5 +178,7 @@ func RunToolLoop( return &ToolLoopResult{ Content: finalContent, Iterations: iteration, + ToolCalls: totalToolCalls, + ToolStats: toolStats, }, nil } diff --git a/pkg/tools/toolloop_reporter_test.go b/pkg/tools/toolloop_reporter_test.go index 199aea169..c4a70bccb 100644 --- a/pkg/tools/toolloop_reporter_test.go +++ b/pkg/tools/toolloop_reporter_test.go @@ -17,14 +17,14 @@ type reporterSpy struct { } type spyCall struct { - state string + state orch.AgentState tool string } func (r *reporterSpy) ReportSpawn(id, label, task string) {} func (r *reporterSpy) ReportConversation(from, to, text string) {} func (r *reporterSpy) ReportGC(id, reason string) {} -func (r *reporterSpy) ReportStateChange(id, state, tool string) { +func (r *reporterSpy) ReportStateChange(id string, state orch.AgentState, tool string) { r.mu.Lock() r.calls = append(r.calls, spyCall{state, tool}) r.mu.Unlock() @@ -117,7 +117,7 @@ func TestToolLoop_Reporter_WaitingBeforeLLM(t *testing.T) { if len(calls) == 0 { t.Fatal("expected at least one ReportStateChange call") } - if calls[0].state != "waiting" { + if calls[0].state != orch.AgentStateWaiting { t.Fatalf("first call must be state=waiting, got %+v", calls[0]) } } @@ -152,17 +152,62 @@ func TestToolLoop_Reporter_ToolcallOrderedAfterWaiting(t *testing.T) { if len(calls) < 3 { t.Fatalf("expected at least 3 calls, got %d: %+v", len(calls), calls) } - if calls[0].state != "waiting" { + if calls[0].state != orch.AgentStateWaiting { t.Fatalf("calls[0] must be waiting, got %+v", calls[0]) } - if calls[1].state != "toolcall" || calls[1].tool != "echo_tool" { + if calls[1].state != orch.AgentStateToolCall || calls[1].tool != "echo_tool" { t.Fatalf("calls[1] must be toolcall(echo_tool), got %+v", calls[1]) } - if calls[2].state != "waiting" { + if calls[2].state != orch.AgentStateWaiting { t.Fatalf("calls[2] must be waiting (2nd LLM iteration), got %+v", calls[2]) } } +// TestToolLoop_ToolCallStats verifies that ToolLoopResult.ToolCalls and +// ToolStats are populated correctly after a tool call iteration. +func TestToolLoop_ToolCallStats(t *testing.T) { + reg := NewToolRegistry() + reg.Register(&echoTool{}) + + result, err := RunToolLoop(context.Background(), ToolLoopConfig{ + Provider: &sequenceMockProvider{}, + Model: "test", + Tools: reg, + MaxIterations: 5, + }, []providers.Message{{Role: "user", Content: "do it"}}, "cli", "direct") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.ToolCalls != 1 { + t.Errorf("ToolCalls = %d, want 1", result.ToolCalls) + } + if result.ToolStats["echo_tool"] != 1 { + t.Errorf("ToolStats[echo_tool] = %d, want 1", result.ToolStats["echo_tool"]) + } + if result.Iterations != 2 { + t.Errorf("Iterations = %d, want 2", result.Iterations) + } +} + +// TestToolLoop_NoToolCalls_ZeroStats verifies that a direct answer (no tool +// calls) produces zero ToolCalls and an empty ToolStats map. +func TestToolLoop_NoToolCalls_ZeroStats(t *testing.T) { + result, err := RunToolLoop(context.Background(), ToolLoopConfig{ + Provider: &MockLLMProvider{}, + Model: "test", + MaxIterations: 1, + }, []providers.Message{{Role: "user", Content: "hi"}}, "cli", "direct") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.ToolCalls != 0 { + t.Errorf("ToolCalls = %d, want 0", result.ToolCalls) + } + if len(result.ToolStats) != 0 { + t.Errorf("ToolStats = %v, want empty", result.ToolStats) + } +} + // TestToolLoop_Reporter_NoopImplementsInterface is a compile-time check that // orch.Noop satisfies the orch.AgentReporter interface accepted by // ToolLoopConfig.Reporter. If Noop ever stops implementing the interface the