From db8bc53265dca6e6405a5935da86d8311bf9591a Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Sat, 21 Feb 2026 02:20:16 +0900 Subject: [PATCH] fix: add safeguards to prevent /plan from executing without a complete plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AI could transition to "executing" status without writing phases, commands, or context to MEMORY.md, causing empty plan execution. Adds multiple defense layers: - Block non-read/non-MEMORY.md tool calls during interview and review - Cap LLM iterations to 3 during pre-execution phases - Validate phases exist before /plan start allows transition - Auto-revert to interviewing if executing with no phases - Hijack interviewing→executing transition to "review" status, showing the plan to the user for approval via /plan start - Inject staleness nudge after 2 consecutive turns without MEMORY.md update - Add GetReviewContext() for review-phase-only system prompt injection Co-Authored-By: Claude Opus 4.6 --- pkg/agent/context.go | 9 ++- pkg/agent/instance.go | 4 ++ pkg/agent/loop.go | 126 +++++++++++++++++++++++++++++++++++++---- pkg/agent/loop_test.go | 61 +++++++++++++++++--- pkg/agent/memory.go | 27 ++++++++- 5 files changed, 204 insertions(+), 23 deletions(-) diff --git a/pkg/agent/context.go b/pkg/agent/context.go index e7d2f7170..0f001594d 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -83,8 +83,8 @@ Your workspace is at: %s 3. **Memory & Plans** - Use memory/MEMORY.md for structured plans. - If Status is "interviewing": Ask clarifying questions. - Update Context with answers via edit_file. - When ready, organize into Phases and set Status to "executing". + After each answer, use edit_file to save findings to ## Context in memory/MEMORY.md. + When you have enough information, write ## Phase and ## Commands sections into MEMORY.md, then set Status to "executing". - If Status is "executing": Work through the current Phase's steps. Mark each [x] via edit_file. The system will auto-advance phases. - Plan format: @@ -339,6 +339,11 @@ func (cb *ContextBuilder) GetCurrentPhase() int { return cb.memory.GetCurrentPhase() } +// GetTotalPhases returns the total number of phases in the plan. +func (cb *ContextBuilder) GetTotalPhases() int { + return cb.memory.GetTotalPhases() +} + // FormatPlanDisplay returns a user-facing display of the full plan. func (cb *ContextBuilder) FormatPlanDisplay() string { return cb.memory.FormatPlanDisplay() diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 83a3cc0a6..f5976b15b 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -30,6 +30,10 @@ type AgentInstance struct { Subagents *config.SubagentsConfig SkillsFilter []string Candidates []providers.FallbackCandidate + + // Interview staleness tracking: consecutive turns where MEMORY.md was not updated. + interviewStaleCount int + interviewMemoryLen int } // NewAgentInstance creates an agent instance from config. diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 1b245e147..033e92039 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -492,6 +492,23 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt opts.ChatID, ) + // 2b. Interview staleness nudge: if MEMORY.md hasn't been updated for + // several consecutive turns, inject a reminder so the AI writes its findings. + const interviewStaleThreshold = 2 + if agent.ContextBuilder.GetPlanStatus() == "interviewing" && agent.interviewStaleCount >= interviewStaleThreshold { + messages = append(messages, providers.Message{ + Role: "user", + Content: "[System] You have been interviewing for several turns without updating memory/MEMORY.md. Please use edit_file now to save your findings to the ## Context section, or organize the plan into Phases if you have enough information.", + }) + } + + // 2c. Snapshot plan status and MEMORY.md size before LLM iteration. + preStatus := agent.ContextBuilder.GetPlanStatus() + var preMemoryLen int + if preStatus == "interviewing" { + preMemoryLen = len(agent.ContextBuilder.ReadMemory()) + } + // 3. Save user message to session (use compact form if available) historyMsg := opts.UserMessage if opts.HistoryMessage != "" { @@ -514,8 +531,33 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt // This is controlled by the tool's Silent flag and ForUser content // 5a. Auto-advance plan phases after LLM iteration - if agent.ContextBuilder.HasActivePlan() && agent.ContextBuilder.GetPlanStatus() == "executing" { - if agent.ContextBuilder.IsPlanComplete() { + postStatus := agent.ContextBuilder.GetPlanStatus() + if agent.ContextBuilder.HasActivePlan() && postStatus == "executing" { + // Intercept: if AI changed status from interviewing to executing, + // hijack to "review" and show the plan for user approval. + if preStatus == "interviewing" { + if agent.ContextBuilder.GetTotalPhases() == 0 { + _ = agent.ContextBuilder.SetPlanStatus("interviewing") + logger.WarnCF("agent", "Reverted plan to interviewing: no phases defined", + map[string]interface{}{"agent_id": agent.ID}) + } else { + _ = agent.ContextBuilder.SetPlanStatus("review") + if !constants.IsInternalChannel(opts.Channel) { + planDisplay := agent.ContextBuilder.FormatPlanDisplay() + al.bus.PublishOutbound(bus.OutboundMessage{ + Channel: opts.Channel, + ChatID: opts.ChatID, + Content: planDisplay + "\n\nUse /plan start to approve, or continue chatting to refine.", + SkipPlaceholder: true, + }) + } + } + } else if agent.ContextBuilder.GetTotalPhases() == 0 { + // Safeguard: executing but no phases (shouldn't happen, but be safe). + _ = agent.ContextBuilder.SetPlanStatus("interviewing") + logger.WarnCF("agent", "Reverted plan to interviewing: no phases defined", + map[string]interface{}{"agent_id": agent.ID}) + } else if agent.ContextBuilder.IsPlanComplete() { _ = agent.ContextBuilder.ClearMemory() if !constants.IsInternalChannel(opts.Channel) { al.bus.PublishOutbound(bus.OutboundMessage{ @@ -540,7 +582,21 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt } } - // 5b. Handle empty response + // 5b. Interview staleness detection: compare MEMORY.md size after iteration. + if agent.ContextBuilder.GetPlanStatus() == "interviewing" { + postMemoryLen := len(agent.ContextBuilder.ReadMemory()) + if postMemoryLen == preMemoryLen { + agent.interviewStaleCount++ + } else { + agent.interviewStaleCount = 0 + } + agent.interviewMemoryLen = postMemoryLen + } else { + // Reset counter when not interviewing. + agent.interviewStaleCount = 0 + } + + // 5c. Handle empty response if finalContent == "" { finalContent = opts.DefaultResponse } @@ -616,14 +672,24 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, agent *AgentInstance, var finalContent string lastReminderIdx := -1 - for iteration < agent.MaxIterations { + // During pre-execution plan modes (interviewing/review), cap iterations + // to prevent runaway tool loops. + maxIter := agent.MaxIterations + if isPlanPreExecution(agent.ContextBuilder.GetPlanStatus()) { + const interviewMaxIter = 3 + if maxIter > interviewMaxIter { + maxIter = interviewMaxIter + } + } + + for iteration < maxIter { iteration++ logger.DebugCF("agent", "LLM iteration", map[string]interface{}{ "agent_id": agent.ID, "iteration": iteration, - "max": agent.MaxIterations, + "max": maxIter, }) // Build tool definitions @@ -770,7 +836,7 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, agent *AgentInstance, al.bus.PublishOutbound(bus.OutboundMessage{ Channel: opts.Channel, ChatID: opts.ChatID, - Content: fmt.Sprintf("🔧 %s (%d/%d)", strings.Join(toolNames, ", "), iteration, agent.MaxIterations), + Content: fmt.Sprintf("🔧 %s (%d/%d)", strings.Join(toolNames, ", "), iteration, maxIter), IsStatus: true, }) } @@ -825,7 +891,14 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, agent *AgentInstance, } } - toolResult := agent.Tools.ExecuteWithContext(ctx, tc.Name, tc.Arguments, opts.Channel, opts.ChatID, asyncCallback) + // Block non-allowed tools during plan interview mode. + // Only read-type tools and MEMORY.md writes are permitted. + var toolResult *tools.ToolResult + if isPlanPreExecution(agent.ContextBuilder.GetPlanStatus()) && !isToolAllowedDuringInterview(tc.Name, tc.Arguments) { + toolResult = tools.ErrorResult("Interview mode: only read tools and MEMORY.md edits are allowed. Focus on asking questions and updating the plan.") + } else { + toolResult = agent.Tools.ExecuteWithContext(ctx, tc.Name, tc.Arguments, opts.Channel, opts.ChatID, asyncCallback) + } // Send ForUser content to user immediately if not Silent if !toolResult.Silent && toolResult.ForUser != "" && opts.SendResponse { @@ -884,7 +957,7 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, agent *AgentInstance, // If max iterations exhausted with tool calls still pending, // make one final LLM call without tools to force a text response. - if finalContent == "" && iteration >= agent.MaxIterations { + if finalContent == "" && iteration >= maxIter { logger.WarnCF("agent", "Max iterations reached, forcing final response without tools", map[string]interface{}{ "agent_id": agent.ID, @@ -1465,13 +1538,20 @@ func (al *AgentLoop) handlePlanCommand(args []string) (string, bool) { if !agent.ContextBuilder.HasActivePlan() { return "No active plan.", true } - if agent.ContextBuilder.GetPlanStatus() != "interviewing" { + status := agent.ContextBuilder.GetPlanStatus() + if status == "executing" { return "Plan is already executing.", true } + if status != "interviewing" && status != "review" { + return fmt.Sprintf("Cannot start from status %q.", status), true + } + if agent.ContextBuilder.GetTotalPhases() == 0 { + return "Cannot start: no phases defined yet. Complete the interview first.", true + } if err := agent.ContextBuilder.SetPlanStatus("executing"); err != nil { return fmt.Sprintf("Error: %v", err), true } - return "Plan status changed to executing.", true + return "Plan approved. Executing.", true case "next": if !agent.ContextBuilder.HasActivePlan() { @@ -1495,6 +1575,32 @@ func (al *AgentLoop) handlePlanCommand(args []string) (string, bool) { } } +// isPlanPreExecution returns true if the plan is in a pre-execution state +// (interviewing or review) where tool restrictions and iteration caps apply. +func isPlanPreExecution(status string) bool { + return status == "interviewing" || status == "review" +} + +// 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. +func isToolAllowedDuringInterview(toolName string, args map[string]interface{}) bool { + // Read-type tools: always allowed + switch toolName { + case "read_file", "list_dir", "web_search", "web_fetch": + return true + } + + // Write-type tools: allowed only when targeting MEMORY.md + switch toolName { + case "edit_file", "append_file", "write_file": + path, _ := args["path"].(string) + return strings.HasSuffix(path, "MEMORY.md") + } + + return false +} + // expandPlanCommand detects "/plan " (new plan start) and: // - writes the interview seed to MEMORY.md // - rewrites the message content for the LLM diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 6bffdec03..37cabb961 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -995,18 +995,60 @@ func TestPlanCommand_Start(t *testing.T) { al, cleanup := newTestAgentLoop(t) defer cleanup() - // Create interviewing plan + agent := al.registry.GetDefaultAgent() + + // Create interviewing plan with phases (start requires phases) + plan := "# Active Plan\n\n> Task: Test task\n> Status: interviewing\n> Phase: 1\n\n## Phase 1: Setup\n- [ ] Step one\n\n## Context\n" + _ = agent.ContextBuilder.WriteMemory(plan) + + // Transition to executing via /plan start + response, _ := al.handleCommand(context.Background(), bus.InboundMessage{Content: "/plan start"}) + if !strings.Contains(response, "approved") { + t.Errorf("expected 'approved', got %q", response) + } + + if status := agent.ContextBuilder.GetPlanStatus(); status != "executing" { + t.Errorf("expected 'executing', got %q", status) + } +} + +func TestPlanCommand_StartFromReview(t *testing.T) { + al, cleanup := newTestAgentLoop(t) + defer cleanup() + + agent := al.registry.GetDefaultAgent() + + // Create a plan in review status + 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) + + // Approve via /plan start + response, _ := al.handleCommand(context.Background(), bus.InboundMessage{Content: "/plan start"}) + if !strings.Contains(response, "approved") { + t.Errorf("expected 'approved', got %q", response) + } + + if status := agent.ContextBuilder.GetPlanStatus(); status != "executing" { + t.Errorf("expected 'executing', got %q", status) + } +} + +func TestPlanCommand_StartNoPhases(t *testing.T) { + al, cleanup := newTestAgentLoop(t) + defer cleanup() + + // Create interviewing plan without phases al.expandPlanCommand(bus.InboundMessage{Content: "/plan Test task"}) - // Transition to executing + // Should be blocked because no phases exist response, _ := al.handleCommand(context.Background(), bus.InboundMessage{Content: "/plan start"}) - if !strings.Contains(response, "executing") { - t.Errorf("expected 'executing', got %q", response) + if !strings.Contains(response, "no phases") { + t.Errorf("expected 'no phases' error, got %q", response) } agent := al.registry.GetDefaultAgent() - if status := agent.ContextBuilder.GetPlanStatus(); status != "executing" { - t.Errorf("expected 'executing', got %q", status) + if status := agent.ContextBuilder.GetPlanStatus(); status != "interviewing" { + t.Errorf("expected status to remain 'interviewing', got %q", status) } } @@ -1014,8 +1056,11 @@ func TestPlanCommand_StartAlreadyExecuting(t *testing.T) { al, cleanup := newTestAgentLoop(t) defer cleanup() - // Create interviewing plan then start - al.expandPlanCommand(bus.InboundMessage{Content: "/plan Test task"}) + agent := al.registry.GetDefaultAgent() + + // Create interviewing plan with phases, then start + plan := "# Active Plan\n\n> Task: Test task\n> Status: interviewing\n> Phase: 1\n\n## Phase 1: Setup\n- [ ] Step one\n\n## Context\n" + _ = agent.ContextBuilder.WriteMemory(plan) al.handleCommand(context.Background(), bus.InboundMessage{Content: "/plan start"}) // Try start again diff --git a/pkg/agent/memory.go b/pkg/agent/memory.go index f7540d6cd..8abafe3bf 100644 --- a/pkg/agent/memory.go +++ b/pkg/agent/memory.go @@ -375,7 +375,11 @@ func (ms *MemoryStore) GetInterviewContext() string { sb.WriteString("- Environment (OS, language, runtime versions)\n") sb.WriteString("- Tooling preferences (test framework, linter, formatter, CI)\n") sb.WriteString("- Key commands the user already runs (build, test, deploy)\n") - sb.WriteString("When ready, organize into 2-5 phases with 3-5 steps each.\n") + sb.WriteString("\n### Rules\n") + sb.WriteString("- After each answer, use edit_file to append findings to the ## Context section of memory/MEMORY.md.\n") + sb.WriteString("- When you have enough information, use edit_file to write ## Phase, ## Commands, and ## Context sections into memory/MEMORY.md.\n") + sb.WriteString("- Organize into 2-5 phases with 3-5 steps each.\n") + sb.WriteString("- After writing Phases, set Status to executing. The system will handle the rest.\n") sb.WriteString("\n### Target Format\n") sb.WriteString("```\n") sb.WriteString("## Phase 1: \n") @@ -392,6 +396,20 @@ func (ms *MemoryStore) GetInterviewContext() string { return sb.String() } +// GetReviewContext returns context for injection during the review phase. +// Shows the full plan and instructs the AI to wait for user approval. +func (ms *MemoryStore) GetReviewContext() string { + content := ms.ReadLongTerm() + + var sb strings.Builder + sb.WriteString("## Active Plan (awaiting approval)\n\n") + sb.WriteString(content) + sb.WriteString("\n\nThe plan is awaiting user approval.\n") + sb.WriteString("- If the user requests changes, update memory/MEMORY.md via edit_file.\n") + sb.WriteString("- Do NOT change Status yourself. The user will run /plan start to approve.\n") + return sb.String() +} + // GetPlanContext returns context for injection during the executing phase. // Only the current phase is shown in detail; completed phases are compressed // to one-line summaries; future phases are omitted. @@ -572,9 +590,12 @@ func (ms *MemoryStore) GetMemoryContext() string { if longTerm != "" { if ms.HasActivePlan() { status := ms.GetPlanStatus() - if status == "interviewing" { + switch status { + case "interviewing": parts = append(parts, ms.GetInterviewContext()) - } else { + case "review": + parts = append(parts, ms.GetReviewContext()) + default: parts = append(parts, ms.GetPlanContext()) } } else {