From 7b066de205bd03bfa0694648f63dd24eaa64d45d Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Sun, 22 Feb 2026 12:36:57 +0900 Subject: [PATCH] =?UTF-8?q?feat:=20validate=20plan=20structure=20before=20?= =?UTF-8?q?interview=E2=86=92review=20transition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the simple GetTotalPhases()==0 check with ValidatePlanStructure() which verifies header, metadata lines, phase sections, and checkbox steps exist before allowing the transition. Invalid plans are rejected with a descriptive error message injected for the LLM to fix. Co-Authored-By: Claude Opus 4.6 --- pkg/agent/context.go | 5 ++ pkg/agent/loop.go | 9 ++- pkg/agent/memory.go | 35 ++++++++++ pkg/agent/memory_test.go | 141 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 188 insertions(+), 2 deletions(-) diff --git a/pkg/agent/context.go b/pkg/agent/context.go index dcef0640e..67056def3 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -421,6 +421,11 @@ func (cb *ContextBuilder) AddStep(phase int, desc string) error { return cb.memory.AddStep(phase, desc) } +// ValidatePlanStructure validates plan structure for interview→review transition. +func (cb *ContextBuilder) ValidatePlanStructure() error { + return cb.memory.ValidatePlanStructure() +} + // SetPlanStatus sets the plan status. func (cb *ContextBuilder) SetPlanStatus(status string) error { return cb.memory.SetStatus(status) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index a3361378c..e2d06eb44 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -828,10 +828,15 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt // 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 { + if err := agent.ContextBuilder.ValidatePlanStructure(); err != nil { _ = agent.ContextBuilder.SetPlanStatus("interviewing") - logger.WarnCF("agent", "Reverted plan to interviewing: no phases defined", + logger.WarnCF("agent", "Reverted plan to interviewing: "+err.Error(), map[string]interface{}{"agent_id": agent.ID}) + // Inject rejection so LLM knows what to fix + messages = append(messages, providers.Message{ + Role: "user", + Content: "[System] Plan rejected: " + err.Error() + ". Fix and try again.", + }) } else { _ = agent.ContextBuilder.SetPlanStatus("review") if !constants.IsInternalChannel(opts.Channel) { diff --git a/pkg/agent/memory.go b/pkg/agent/memory.go index fd6fd7ac0..05ad547f7 100644 --- a/pkg/agent/memory.go +++ b/pkg/agent/memory.go @@ -407,6 +407,41 @@ func (ms *MemoryStore) AddStep(phase int, desc string) error { return ms.WriteLongTerm(strings.Join(newLines, "\n")) } +// ValidatePlanStructure checks that the plan has valid structure for +// transitioning out of the interview phase. Returns nil if valid, +// or an error describing the first problem found. +func (ms *MemoryStore) ValidatePlanStructure() error { + content := ms.ReadLongTerm() + + // 1. Header: # Active Plan must exist + if !reActivePlan.MatchString(content) { + return fmt.Errorf("missing '# Active Plan' header") + } + + // 2. Required metadata lines + if !reStatus.MatchString(content) { + return fmt.Errorf("missing '> Status:' line") + } + if !rePhase.MatchString(content) { + return fmt.Errorf("missing '> Phase:' line") + } + + // 3. At least one phase header (## Phase N: title) + phases := ms.GetPlanPhases() + if len(phases) == 0 { + return fmt.Errorf("no '## Phase N:' sections found") + } + + // 4. Every phase must have at least one checkbox step + for _, p := range phases { + if len(p.Steps) == 0 { + return fmt.Errorf("Phase %d has no checkbox steps (use '- [ ] ...')", p.Number) + } + } + + return nil +} + // ---------- Selective injection methods ---------- // GetPlanWorkDir returns the WorkDir from the plan metadata, or "". diff --git a/pkg/agent/memory_test.go b/pkg/agent/memory_test.go index 12b77c418..13633e442 100644 --- a/pkg/agent/memory_test.go +++ b/pkg/agent/memory_test.go @@ -503,6 +503,147 @@ func TestFormatPlanDisplay(t *testing.T) { } } +func TestValidatePlanStructure(t *testing.T) { + tests := []struct { + name string + content string + wantErr string // "" means nil error expected + }{ + { + name: "valid plan with 1 phase and 1 step", + content: `# Active Plan + +> Task: Do something +> Status: executing +> Phase: 1 + +## Phase 1: Setup +- [ ] Install deps +`, + wantErr: "", + }, + { + name: "missing Active Plan header", + content: `> Status: executing`, + wantErr: "missing '# Active Plan' header", + }, + { + name: "missing Status line", + content: `# Active Plan + +> Phase: 1 + +## Phase 1: Setup +- [ ] Install deps +`, + wantErr: "missing '> Status:' line", + }, + { + name: "missing Phase line", + content: `# Active Plan + +> Status: executing + +## Phase 1: Setup +- [ ] Install deps +`, + wantErr: "missing '> Phase:' line", + }, + { + name: "no Phase sections", + content: `# Active Plan + +> Task: Do something +> Status: executing +> Phase: 1 +`, + wantErr: "no '## Phase N:' sections found", + }, + { + name: "phase with no checkbox steps", + content: `# Active Plan + +> Task: Do something +> Status: executing +> Phase: 1 + +## Phase 1: Setup +Some description without checkboxes +`, + wantErr: "Phase 1 has no checkbox steps", + }, + { + name: "all steps done is valid", + content: `# Active Plan + +> Task: Do something +> Status: executing +> Phase: 1 + +## Phase 1: Setup +- [x] Install deps +- [x] Configure +`, + wantErr: "", + }, + { + name: "multi-phase valid", + content: `# Active Plan + +> Task: Do something +> Status: executing +> Phase: 1 + +## Phase 1: Setup +- [ ] Install deps + +## Phase 2: Build +- [ ] Compile +- [ ] Test +`, + wantErr: "", + }, + { + name: "second phase empty steps", + content: `# Active Plan + +> Task: Do something +> Status: executing +> Phase: 1 + +## Phase 1: Setup +- [ ] Install deps + +## Phase 2: Build +No checkboxes here +`, + wantErr: "Phase 2 has no checkbox steps", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ms, cleanup := newTestMemoryStore(t) + defer cleanup() + + ms.WriteLongTerm(tt.content) + err := ms.ValidatePlanStructure() + + if tt.wantErr == "" { + if err != nil { + t.Errorf("expected nil error, got: %v", err) + } + } else { + if err == nil { + t.Errorf("expected error containing %q, got nil", tt.wantErr) + } else if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("expected error containing %q, got: %v", tt.wantErr, err) + } + } + }) + } +} + func TestMemoryStoreCreation(t *testing.T) { tmpDir, err := os.MkdirTemp("", "memory-test-*") if err != nil {