diff --git a/CLAUDE.md b/CLAUDE.md index 01b7dd318..17b11438c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,6 +14,11 @@ go vet ./... Lint: `golangci-lint run` +## Plan Mode + +- **Interview tool filtering**: `interviewAllowedTools` in `pkg/agent/loop.go` is the single source of truth for tools available during interview/review phases. Both `filterInterviewTools` (strips definitions before LLM call) and `isToolAllowedDuringInterview` (argument-level gating) reference this map. +- **History clear**: `/plan start clear` wipes session history and summary on transition to executing. The Mini App review UI offers two sliders: standard approve and approve-with-clear. + ## Security TODOs - **Log Fields masking**: `LogEntry.Fields` (`map[string]any`) is exposed via WebSocket (`/miniapp/api/logs/ws`) and snapshots (`/miniapp/api/logs/snapshot`). If any code logs sensitive values (tokens, API keys, passwords) in Fields, they will be visible to Mini App users. Add a sanitizer in `RecentLogs()` and `wsLogs()` that masks values for keys matching patterns like `token`, `key`, `secret`, `password`, `authorization`. Track in: `pkg/logger/logger.go` (RecentLogs), `pkg/miniapp/miniapp.go` (wsLogs stream). diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 10c44c4c8..475376581 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -86,6 +86,7 @@ type AgentLoop struct { channelManager *channels.Manager providerCache map[string]providers.LLMProvider planStartPending bool // set by /plan start to trigger LLM execution + planClearHistory bool // set by /plan start clear to wipe history on transition sessionLocks sync.Map // sessionKey → *sessionSemaphore activeTasks sync.Map // sessionKey → *activeTask sessions *SessionTracker @@ -287,6 +288,17 @@ func (al *AgentLoop) Run(ctx context.Context) error { // the LLM worker actually begins executing the plan. if al.planStartPending { al.planStartPending = false + clearHistory := al.planClearHistory + al.planClearHistory = false + + if clearHistory { + if agent := al.registry.GetDefaultAgent(); agent != nil { + agent.Sessions.SetHistory(msg.SessionKey, nil) + agent.Sessions.SetSummary(msg.SessionKey, "") + _ = agent.Sessions.Save(msg.SessionKey) + } + } + syntheticMeta := map[string]string{"echoed": "1"} for k, v := range msg.Metadata { if k != "source" { @@ -1550,6 +1562,12 @@ func (al *AgentLoop) runLLMIteration( // Build tool definitions providerToolDefs := agent.Tools.ToProviderDefs() + // Interview mode: strip tool definitions the LLM must not use, + // reducing token cost and preventing wasted reject-retry cycles. + if isPlanPreExecution(planSnapshot) { + providerToolDefs = filterInterviewTools(providerToolDefs) + } + // Log LLM request details logger.DebugCF("agent", "LLM request", map[string]any{ @@ -2772,6 +2790,11 @@ func (al *AgentLoop) handlePlanCommand(args []string) (string, bool) { return fmt.Sprintf("Error: %v", err), true } al.planStartPending = true + clearHistory := len(args) > 1 && args[1] == "clear" + al.planClearHistory = clearHistory + if clearHistory { + return "Plan approved. Executing with clean history.", true + } return "Plan approved. Executing.", true case "next": @@ -2802,34 +2825,56 @@ func isPlanPreExecution(status string) bool { return status == "interviewing" || status == "review" } +// interviewAllowedTools is the single source of truth for tool names that may +// be sent to the LLM (and subsequently invoked) during the interview phase. +// filterInterviewTools uses this to strip tool *definitions* before the LLM call, +// while isToolAllowedDuringInterview adds argument-level checks as a second gate. +var interviewAllowedTools = map[string]bool{ + "readfile": true, + "listdir": true, + "websearch": true, + "webfetch": true, + "message": true, + "editfile": true, + "appendfile": true, + "writefile": true, + "exec": true, + "logs": true, +} + +// filterInterviewTools removes tool definitions that are not in the +// interviewAllowedTools whitelist, reducing token usage and preventing the +// LLM from attempting disallowed tool calls during the interview phase. +func filterInterviewTools(defs []providers.ToolDefinition) []providers.ToolDefinition { + filtered := make([]providers.ToolDefinition, 0, len(defs)) + for _, d := range defs { + if interviewAllowedTools[tools.NormalizeToolName(d.Function.Name)] { + filtered = append(filtered, d) + } + } + return filtered +} + // isToolAllowedDuringInterview checks whether a tool call is permitted while the -// plan is in a pre-execution state. Read-type tools are always allowed. Write-type -// tools (edit_file, append_file, write_file) are only allowed when targeting MEMORY.md. -// Uses normalized names so "readfile" matches "read_file", etc. +// plan is in a pre-execution state. Uses the shared interviewAllowedTools map for +// name-level gating, then applies argument-level constraints for write-type tools +// (MEMORY.md only) and exec (read-only commands only). func isToolAllowedDuringInterview(toolName string, args map[string]interface{}) bool { norm := tools.NormalizeToolName(toolName) - - // Read-type tools and communication: always allowed - switch norm { - case "readfile", "listdir", "websearch", "webfetch", "message": - return true + if !interviewAllowedTools[norm] { + return false } - // Write-type tools: allowed only when targeting MEMORY.md + // Argument-level constraints switch norm { case "editfile", "appendfile", "writefile": path, _ := args["path"].(string) return strings.HasSuffix(path, "MEMORY.md") - } - - // exec: allow read-only commands - switch norm { case "exec": cmd, _ := args["command"].(string) return isReadOnlyCommand(cmd) } - - return false + return true } // isReadOnlyCommand returns true when cmd is a safe, read-only shell command diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 407d89e67..0291819ad 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -2504,3 +2504,139 @@ func TestAgentLoop_PlanModel_NotUsedDuringExecuting(t *testing.T) { t.Errorf("Expected normal model 'normal-model' during executing, got %q", provider.models[0]) } } + +func TestPlanCommand_StartClear(t *testing.T) { + al, cleanup := newTestAgentLoop(t) + defer cleanup() + + agent := al.registry.GetDefaultAgent() + + // Create a plan in review status with phases + plan := "# Active Plan\n\n> Task: Test task\n> Status: review\n> Phase: 1\n\n## Phase 1: Setup\n- [ ] Step one\n\n## Context\n" + _ = agent.ContextBuilder.WriteMemory(plan) + + // Seed session history so we can verify it gets cleared + agent.Sessions.AddMessage("test-session", "user", "hello") + agent.Sessions.AddMessage("test-session", "assistant", "world") + agent.Sessions.SetSummary("test-session", "some summary") + + // Approve with clear + response, handled := al.handleCommand(context.Background(), bus.InboundMessage{ + Content: "/plan start clear", + SessionKey: "test-session", + }) + if !handled { + t.Fatal("expected /plan start clear to be handled") + } + if !strings.Contains(response, "clean history") { + t.Errorf("expected 'clean history' in response, got %q", response) + } + if !al.planStartPending { + t.Error("expected planStartPending to be true") + } + if !al.planClearHistory { + t.Error("expected planClearHistory to be true") + } + + // Simulate what Run() does when planStartPending is set + al.planStartPending = false + clearHistory := al.planClearHistory + al.planClearHistory = false + if clearHistory { + agent.Sessions.SetHistory("test-session", nil) + agent.Sessions.SetSummary("test-session", "") + _ = agent.Sessions.Save("test-session") + } + + // Verify history and summary are cleared + history := agent.Sessions.GetHistory("test-session") + if len(history) != 0 { + t.Errorf("expected empty history after clear, got %d messages", len(history)) + } + summary := agent.Sessions.GetSummary("test-session") + if summary != "" { + t.Errorf("expected empty summary after clear, got %q", summary) + } +} + +func TestPlanCommand_StartWithoutClear_PreservesHistory(t *testing.T) { + al, cleanup := newTestAgentLoop(t) + defer cleanup() + + agent := al.registry.GetDefaultAgent() + + // Create a plan in review status with phases + plan := "# Active Plan\n\n> Task: Test task\n> Status: review\n> Phase: 1\n\n## Phase 1: Setup\n- [ ] Step one\n\n## Context\n" + _ = agent.ContextBuilder.WriteMemory(plan) + + // Seed session history + agent.Sessions.AddMessage("test-session", "user", "hello") + agent.Sessions.AddMessage("test-session", "assistant", "world") + agent.Sessions.SetSummary("test-session", "some summary") + + // Approve without clear + response, _ := al.handleCommand(context.Background(), bus.InboundMessage{ + Content: "/plan start", + SessionKey: "test-session", + }) + if strings.Contains(response, "clean history") { + t.Errorf("did not expect 'clean history' in response, got %q", response) + } + if al.planClearHistory { + t.Error("planClearHistory should be false for /plan start without clear") + } + + // Verify history is preserved + history := agent.Sessions.GetHistory("test-session") + if len(history) != 2 { + t.Errorf("expected 2 history messages preserved, got %d", len(history)) + } + summary := agent.Sessions.GetSummary("test-session") + if summary != "some summary" { + t.Errorf("expected summary preserved, got %q", summary) + } +} + +func TestFilterInterviewTools(t *testing.T) { + allDefs := []providers.ToolDefinition{ + {Function: protocoltypes.ToolFunctionDefinition{Name: "read_file"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "list_dir"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "web_search"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "web_fetch"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "message"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "edit_file"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "append_file"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "write_file"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "exec"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "logs"}}, + // These should be filtered out: + {Function: protocoltypes.ToolFunctionDefinition{Name: "spawn_subagent"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "skills_search"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "skills_install"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "bg_monitor"}}, + {Function: protocoltypes.ToolFunctionDefinition{Name: "i2c_transfer"}}, + } + + filtered := filterInterviewTools(allDefs) + + // Should keep exactly the 10 allowed tools + if len(filtered) != 10 { + names := make([]string, len(filtered)) + for i, d := range filtered { + names[i] = d.Function.Name + } + t.Errorf("expected 10 allowed tools, got %d: %v", len(filtered), names) + } + + // Verify none of the disallowed tools slipped through + disallowed := map[string]bool{ + "spawnsubagent": true, "skillssearch": true, + "skillsinstall": true, "bgmonitor": true, "ictransfer": true, + } + for _, d := range filtered { + norm := tools.NormalizeToolName(d.Function.Name) + if disallowed[norm] { + t.Errorf("disallowed tool %q should have been filtered out", d.Function.Name) + } + } +} diff --git a/pkg/miniapp/static/index.html b/pkg/miniapp/static/index.html index 866075467..806de0fdb 100644 --- a/pkg/miniapp/static/index.html +++ b/pkg/miniapp/static/index.html @@ -995,10 +995,16 @@ function renderPlanFromData(data) { } if (data.status === 'review') { html += `
-
+
Slide to Approve
+
+
+
+
+
Approve & Clear History
+
`; } } else { @@ -1017,61 +1023,69 @@ function setupSlideApprove() { slideApproveAC = new AbortController(); var signal = slideApproveAC.signal; - var track = document.querySelector('.slide-approve-track'); - if (!track) return; - var thumb = track.querySelector('.slide-approve-thumb'); - var label = track.querySelector('.slide-approve-label'); - var dragging = false; - var startX = 0; - var thumbStartLeft = 0; + var tracks = document.querySelectorAll('.slide-approve-track'); + if (!tracks.length) return; - function getMaxLeft() { - return track.offsetWidth - thumb.offsetWidth - 6; // 3px padding each side - } + tracks.forEach(function(track) { + var thumb = track.querySelector('.slide-approve-thumb'); + var label = track.querySelector('.slide-approve-label'); + var cmd = track.getAttribute('data-cmd') || '/plan start'; + var dragging = false; + var startX = 0; + var thumbStartLeft = 0; - function onStart(e) { - if (track.classList.contains('approved')) return; - dragging = true; - thumb.classList.add('dragging'); - var clientX = e.touches ? e.touches[0].clientX : e.clientX; - startX = clientX; - thumbStartLeft = thumb.offsetLeft - 3; // subtract initial 3px offset - e.preventDefault(); - } - - function onMove(e) { - if (!dragging) return; - var clientX = e.touches ? e.touches[0].clientX : e.clientX; - var dx = clientX - startX; - var newLeft = Math.max(0, Math.min(thumbStartLeft + dx, getMaxLeft())); - thumb.style.left = (newLeft + 3) + 'px'; - e.preventDefault(); - } - - function onEnd(e) { - if (!dragging) return; - dragging = false; - thumb.classList.remove('dragging'); - var currentLeft = thumb.offsetLeft - 3; - var maxLeft = getMaxLeft(); - if (currentLeft >= maxLeft * 0.8) { - // Approved - track.classList.add('approved'); - label.textContent = 'Approved!'; - thumb.classList.add('hidden'); - sendCommand('/plan start'); - } else { - // Snap back - thumb.style.left = '3px'; + function getMaxLeft() { + return track.offsetWidth - thumb.offsetWidth - 6; } - } - thumb.addEventListener('touchstart', onStart, { passive: false, signal: signal }); - thumb.addEventListener('mousedown', onStart, { signal: signal }); - document.addEventListener('touchmove', onMove, { passive: false, signal: signal }); - document.addEventListener('mousemove', onMove, { signal: signal }); - document.addEventListener('touchend', onEnd, { signal: signal }); - document.addEventListener('mouseup', onEnd, { signal: signal }); + function markAllApproved() { + tracks.forEach(function(t) { + t.classList.add('approved'); + t.querySelector('.slide-approve-label').textContent = 'Approved!'; + t.querySelector('.slide-approve-thumb').classList.add('hidden'); + }); + } + + function onStart(e) { + if (track.classList.contains('approved')) return; + dragging = true; + thumb.classList.add('dragging'); + var clientX = e.touches ? e.touches[0].clientX : e.clientX; + startX = clientX; + thumbStartLeft = thumb.offsetLeft - 3; + e.preventDefault(); + } + + function onMove(e) { + if (!dragging) return; + var clientX = e.touches ? e.touches[0].clientX : e.clientX; + var dx = clientX - startX; + var newLeft = Math.max(0, Math.min(thumbStartLeft + dx, getMaxLeft())); + thumb.style.left = (newLeft + 3) + 'px'; + e.preventDefault(); + } + + function onEnd(e) { + if (!dragging) return; + dragging = false; + thumb.classList.remove('dragging'); + var currentLeft = thumb.offsetLeft - 3; + var maxLeft = getMaxLeft(); + if (currentLeft >= maxLeft * 0.8) { + markAllApproved(); + sendCommand(cmd); + } else { + thumb.style.left = '3px'; + } + } + + thumb.addEventListener('touchstart', onStart, { passive: false, signal: signal }); + thumb.addEventListener('mousedown', onStart, { signal: signal }); + document.addEventListener('touchmove', onMove, { passive: false, signal: signal }); + document.addEventListener('mousemove', onMove, { signal: signal }); + document.addEventListener('touchend', onEnd, { signal: signal }); + document.addEventListener('mouseup', onEnd, { signal: signal }); + }); } function renderSimpleMarkdown(text) {