feat: validate plan structure before interview→review transition
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 <noreply@anthropic.com>
This commit is contained in:
parent
f9fbfb61e2
commit
7b066de205
4 changed files with 188 additions and 2 deletions
|
|
@ -421,6 +421,11 @@ func (cb *ContextBuilder) AddStep(phase int, desc string) error {
|
||||||
return cb.memory.AddStep(phase, desc)
|
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.
|
// SetPlanStatus sets the plan status.
|
||||||
func (cb *ContextBuilder) SetPlanStatus(status string) error {
|
func (cb *ContextBuilder) SetPlanStatus(status string) error {
|
||||||
return cb.memory.SetStatus(status)
|
return cb.memory.SetStatus(status)
|
||||||
|
|
|
||||||
|
|
@ -828,10 +828,15 @@ func (al *AgentLoop) runAgentLoop(ctx context.Context, agent *AgentInstance, opt
|
||||||
// Intercept: if AI changed status from interviewing to executing,
|
// Intercept: if AI changed status from interviewing to executing,
|
||||||
// hijack to "review" and show the plan for user approval.
|
// hijack to "review" and show the plan for user approval.
|
||||||
if preStatus == "interviewing" {
|
if preStatus == "interviewing" {
|
||||||
if agent.ContextBuilder.GetTotalPhases() == 0 {
|
if err := agent.ContextBuilder.ValidatePlanStructure(); err != nil {
|
||||||
_ = agent.ContextBuilder.SetPlanStatus("interviewing")
|
_ = 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})
|
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 {
|
} else {
|
||||||
_ = agent.ContextBuilder.SetPlanStatus("review")
|
_ = agent.ContextBuilder.SetPlanStatus("review")
|
||||||
if !constants.IsInternalChannel(opts.Channel) {
|
if !constants.IsInternalChannel(opts.Channel) {
|
||||||
|
|
|
||||||
|
|
@ -407,6 +407,41 @@ func (ms *MemoryStore) AddStep(phase int, desc string) error {
|
||||||
return ms.WriteLongTerm(strings.Join(newLines, "\n"))
|
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 ----------
|
// ---------- Selective injection methods ----------
|
||||||
|
|
||||||
// GetPlanWorkDir returns the WorkDir from the plan metadata, or "".
|
// GetPlanWorkDir returns the WorkDir from the plan metadata, or "".
|
||||||
|
|
|
||||||
|
|
@ -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) {
|
func TestMemoryStoreCreation(t *testing.T) {
|
||||||
tmpDir, err := os.MkdirTemp("", "memory-test-*")
|
tmpDir, err := os.MkdirTemp("", "memory-test-*")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue