fix(agent): refuse plan re-proposal + emit done on error (close divergence chain)
Operator-reported bug: on 'proceed with the rest' the agent re-proposed the
plan, duplicating it in the sidebar. Root cause was a three-bug chain, not
one bug:
1. Trigger — model empty-response on 'proceed' (approval vocabulary didn't
list 'proceed', so the agent wasn't sure it was approved and no-op'd).
2. Amplifier — chatWith emitted 'error' without 'done' on empty response
(agent.go:370). The frontend's onComplete saw !receivedDone and
misclassified the model failure as a network disconnect, calling
handleDisconnect -> resumeSession.
3. Divergence — the reconnect note was generic ('report your state'), so
the agent re-proposed + re-executed instead of advancing the plan.
Fixes (shipped, e2e-validated against the live agent on oikos-nomos-1):
- A.2: proposePlan refuses re-proposal once a step has started (returns
errPlanInFlight). Drops the append-mode safety net (commit 5384499) that
was the direct source of the sidebar duplication. The agent must advance
with update_plan_step + run; the tool result directs it.
- A.1: proposePlan sets the 'generation' column on INSERT (migration 020
added the column + frontend grouping, but the INSERT never wired it).
- A.3: propose_plan tool description restated as a crisp contract (ONCE,
STOP and wait, REFUSES once a step started, advance with update_plan_step).
- F.3: approval vocabulary expanded to approved/yes/go/proceed/continue/ok/
go ahead; propose_plan result string tightened to an imperative.
- B.1: chatWith emits 'done' after 'error' on every terminal path via a new
emitError helper. The frontend now treats model errors as ended (not
disconnected), so no auto-reconnect -> resumeSession fires.
- B.2: reconnect/resume note carries the operator's last message + an
explicit 'advance the plan, do NOT call propose_plan again' directive when
a plan is in flight. Wired into all 4 resume entry points (reconnect,
/resume, idle-sweep, question-answer) via enrichResumeNote.
- B.3: resumeSession escalates the recovery note across its 3 attempts (final
retry: 'pick the lowest-pending step, mark it running, call run — do that
now') instead of 3 identical notes -> 3 identical empties.
Verification: TestProposePlan_RefuseInFlight replaces TestProposePlan_
AppendVsReplace. e2e conversations against the rebuilt container:
conv2 ('proceed with the rest') -> 0 propose_plan calls, plan stayed at
3 steps (was 6+ before), update_plan_step x5 + run x2 + complete_task.
conv3 (full plan, 'go ahead') -> apt-get update on lxc:dns auto-ran under
the plan window, update_entity_attributes writeback, clean complete_task.
nomos logs show zero reconnect/resume entries for the plan-proposing
sessions (the three-bug chain is closed).
Remaining (not in this commit): D.1 refuse complete_task without writeback
(next blocker), C.1/C.2, F.1/F.2 SOUL.md consolidation, B.4-B.6, E.1/E.2.
See plans/2026-07-14-post-fix-session-remainders.md.
Also: re-audit 2026-07-10-general-gated-execution.md — request_execution enum
retirement (60effcb) closes item 9; only auto-act revival (item 10) remains.
Version 0.4.1 -> 0.5.0 (minor: new structural behavior, not a bugfix).
This commit is contained in:
@@ -187,9 +187,30 @@ func (a *agent) chat(ctx context.Context, sessionID, message string, emit func(a
|
||||
func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject string, emit func(agentEvent)) {
|
||||
correlationID := uuid.New().String()
|
||||
|
||||
// emitError emits an error event followed by a done event. The done
|
||||
// event is CRITICAL on every terminal path: the frontend's
|
||||
// onComplete handler (chat.ts) treats a missing `done` as a severed
|
||||
// network connection and triggers an auto-reconnect → resumeSession.
|
||||
// Before this fix, a model empty-response (the most common case here)
|
||||
// returned without `done`, was misclassified as a network drop, and
|
||||
// the reconnect logic re-invoked the agent with a generic "report
|
||||
// your state" note — which caused the agent to re-propose the plan
|
||||
// and duplicate it in the sidebar (operator-reported 2026-07-14).
|
||||
// Every error return below must go through emitError so the frontend
|
||||
// shows the error inline instead of silently reconnecting.
|
||||
emitError := func(data string) {
|
||||
emit(agentEvent{Type: "error", Data: data, SessionID: sessionID})
|
||||
emit(agentEvent{Type: "done", Data: map[string]any{
|
||||
"session_id": sessionID,
|
||||
"correlation_id": correlationID,
|
||||
"iterations": 0,
|
||||
"error": true,
|
||||
}, SessionID: sessionID})
|
||||
}
|
||||
|
||||
tools, err := a.buildTools(sessionID)
|
||||
if err != nil {
|
||||
emit(agentEvent{Type: "error", Data: fmt.Sprintf("build tools: %v", err), SessionID: sessionID})
|
||||
emitError(fmt.Sprintf("build tools: %v", err))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -345,7 +366,7 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s
|
||||
slog.Warn("nomos: llm stream error, retrying", "error", err, "attempt", attempt+1, "session", sessionID)
|
||||
continue
|
||||
}
|
||||
emit(agentEvent{Type: "error", Data: fmt.Sprintf("llm: %v", err), SessionID: sessionID})
|
||||
emitError(fmt.Sprintf("llm: %v", err))
|
||||
return
|
||||
}
|
||||
if len(acc.Choices) == 0 {
|
||||
@@ -353,7 +374,7 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s
|
||||
slog.Warn("nomos: no choices in response, retrying", "attempt", attempt+1, "session", sessionID)
|
||||
continue
|
||||
}
|
||||
emit(agentEvent{Type: "error", Data: "no choices in response", SessionID: sessionID})
|
||||
emitError("no choices in response")
|
||||
return
|
||||
}
|
||||
|
||||
@@ -367,7 +388,7 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s
|
||||
"content_len", len(msg.Content))
|
||||
continue
|
||||
}
|
||||
emit(agentEvent{Type: "error", Data: "Nomos returned an empty or unusable response — please retry.", SessionID: sessionID})
|
||||
emitError("Nomos returned an empty or unusable response — please retry.")
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
@@ -80,11 +80,12 @@ func (a *agent) processIdleSweep(ctx context.Context) {
|
||||
slog.Error("nomos: idle nudge bump failed", "session", s.ID, "error", err)
|
||||
return
|
||||
}
|
||||
note := fmt.Sprintf("[System: this task ('%s') has been idle for %s with no complete_task call. "+
|
||||
"If the goal is done (or can't be completed), call complete_task now with the outcome and a "+
|
||||
"one-line summary. If you're still genuinely working through the plan, ignore this and continue.]",
|
||||
s.Goal, idleTaskThreshold)
|
||||
a.resumeSession(ctx, s.ID, note)
|
||||
note := fmt.Sprintf("[System: this task ('%s') has been idle for %s with no complete_task call. "+
|
||||
"If the goal is done (or can't be completed), call complete_task now with the outcome and a "+
|
||||
"one-line summary. If you're still genuinely working through the plan, ignore this and continue.]",
|
||||
s.Goal, idleTaskThreshold)
|
||||
note = a.store.enrichResumeNote(ctx, s.ID, note)
|
||||
a.resumeSession(ctx, s.ID, note)
|
||||
})
|
||||
continue
|
||||
}
|
||||
|
||||
@@ -177,7 +177,8 @@ func handleChat(w http.ResponseWriter, r *http.Request, a *agent, st *store) {
|
||||
if req.Message == "" && req.SessionID != "" {
|
||||
slog.Info("nomos: reconnect", "session", req.SessionID)
|
||||
safego.Go("nomos:reconnect:"+req.SessionID, func() {
|
||||
note := "[System: the operator's connection was re-established. The task may have progressed in the background — report your current state and progress.]"
|
||||
base := "[System: the operator's connection was re-established. The task may have progressed in the background.]"
|
||||
note := st.enrichResumeNote(context.Background(), req.SessionID, base)
|
||||
a.resumeSession(context.Background(), req.SessionID, note)
|
||||
})
|
||||
// Return 202 so the frontend doesn't try to consume an SSE stream
|
||||
@@ -338,7 +339,8 @@ func handleSessionDetail(w http.ResponseWriter, r *http.Request, st *store, a *a
|
||||
|
||||
// POST /sessions/{id}/resume — the operator asks the agent to continue.
|
||||
if len(parts) == 2 && parts[1] == "resume" && r.Method == http.MethodPost {
|
||||
note := "[System: the operator wants you to continue. Pick up where you left off — execute the next step of the plan, diagnose and fix any failures, or report progress if everything is done.]"
|
||||
base := "[System: the operator wants you to continue. Pick up where you left off — execute the next step of the plan, diagnose and fix any failures, or report progress if everything is done.]"
|
||||
note := st.enrichResumeNote(context.Background(), id, base)
|
||||
safego.Go("nomos:resume-session", func() { a.resumeSession(context.Background(), id, note) })
|
||||
w.WriteHeader(202)
|
||||
return
|
||||
@@ -409,8 +411,9 @@ func handleAnswerQuestion(w http.ResponseWriter, r *http.Request, st *store, a *
|
||||
return
|
||||
}
|
||||
if a != nil {
|
||||
note := fmt.Sprintf("[System: the operator answered your question %q with: %q. "+
|
||||
base := fmt.Sprintf("[System: the operator answered your question %q with: %q. "+
|
||||
"Continue the task from here — do not re-ask.]", prompt, req.Answer)
|
||||
note := st.enrichResumeNote(context.Background(), sessionID, base)
|
||||
safego.Go("nomos:resume-session", func() { a.resumeSession(context.Background(), sessionID, note) })
|
||||
}
|
||||
w.WriteHeader(202)
|
||||
|
||||
@@ -3,6 +3,7 @@ package main
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"regexp"
|
||||
@@ -17,6 +18,13 @@ import (
|
||||
|
||||
const maxToolResultSize = 4096
|
||||
|
||||
// errPlanInFlight is returned by proposePlan when called again after a step
|
||||
// has already started. The agent must advance the existing plan with
|
||||
// update_plan_step + run instead of re-proposing — re-proposing was the
|
||||
// source of duplicate plans in the sidebar (operator-reported 2026-07-14).
|
||||
// The caller translates this into a directive tool result.
|
||||
var errPlanInFlight = errors.New("plan already in flight")
|
||||
|
||||
type store struct {
|
||||
pool *pgxpool.Pool
|
||||
}
|
||||
@@ -150,6 +158,76 @@ func (s *store) updateMessage(ctx context.Context, id uuid.UUID, content json.Ra
|
||||
return err
|
||||
}
|
||||
|
||||
// lastUserMessage returns the most recent user message text for a session,
|
||||
// or "" if none. Used to build a context-rich reconnect/resume note: instead
|
||||
// of a generic "report your state," the note can say "the operator's last
|
||||
// message was X — advance the plan" so the agent doesn't re-propose or
|
||||
// re-execute on a reconnect (the operator-reported 2026-07-14 divergence).
|
||||
func (s *store) lastUserMessage(ctx context.Context, sessionID string) string {
|
||||
if s == nil || sessionID == "" || sessionID == "ephemeral" {
|
||||
return ""
|
||||
}
|
||||
var content json.RawMessage
|
||||
if err := s.pool.QueryRow(ctx,
|
||||
`SELECT content FROM agent_messages
|
||||
WHERE session_id = $1 AND role = 'user'
|
||||
ORDER BY created_at DESC LIMIT 1`, sessionID).Scan(&content); err != nil {
|
||||
return ""
|
||||
}
|
||||
var m struct {
|
||||
Text string `json:"text"`
|
||||
}
|
||||
if err := json.Unmarshal(content, &m); err != nil {
|
||||
return ""
|
||||
}
|
||||
return m.Text
|
||||
}
|
||||
|
||||
// hasPlanInFlight reports whether a session has a plan with at least one
|
||||
// step in a non-terminal state (pending/running). Used to direct the
|
||||
// reconnect/resume note: if a plan is in flight, the note says "advance
|
||||
// the plan with update_plan_step + run" instead of the generic "report
|
||||
// your state" (which caused the agent to re-propose and duplicate the plan
|
||||
// in the sidebar — operator-reported 2026-07-14).
|
||||
func (s *store) hasPlanInFlight(ctx context.Context, sessionID string) bool {
|
||||
if s == nil || sessionID == "" || sessionID == "ephemeral" {
|
||||
return false
|
||||
}
|
||||
var exists bool
|
||||
if err := s.pool.QueryRow(ctx,
|
||||
`SELECT EXISTS(SELECT 1 FROM session_plan_steps
|
||||
WHERE session_id = $1 AND status IN ('pending', 'running'))`, sessionID).Scan(&exists); err != nil {
|
||||
return false
|
||||
}
|
||||
return exists
|
||||
}
|
||||
|
||||
// enrichResumeNote appends session context to a base resume/reconnect note:
|
||||
// the operator's last user message and, if a plan is in flight, an explicit
|
||||
// directive to advance it with update_plan_step + run (not re-propose). The
|
||||
// generic "report your state" note caused the agent to re-propose and
|
||||
// duplicate the plan on a reconnect (operator-reported 2026-07-14); this
|
||||
// enrichment gives the agent enough context to do the right thing even
|
||||
// through the reconnect path.
|
||||
func (s *store) enrichResumeNote(ctx context.Context, sessionID, base string) string {
|
||||
if s == nil || sessionID == "" || sessionID == "ephemeral" {
|
||||
return base
|
||||
}
|
||||
last := s.lastUserMessage(ctx, sessionID)
|
||||
inFlight := s.hasPlanInFlight(ctx, sessionID)
|
||||
if last == "" && !inFlight {
|
||||
return base
|
||||
}
|
||||
note := base
|
||||
if last != "" {
|
||||
note += fmt.Sprintf(" The operator's last message was: %q.", last)
|
||||
}
|
||||
if inFlight {
|
||||
note += " A plan is in flight — advance it with update_plan_step (status=running) + run for the next step's target. Do NOT call propose_plan again."
|
||||
}
|
||||
return note
|
||||
}
|
||||
|
||||
func truncateToolResults(content json.RawMessage) json.RawMessage {
|
||||
var m map[string]any
|
||||
if err := json.Unmarshal(content, &m); err != nil {
|
||||
@@ -404,14 +482,14 @@ type planStepInput struct {
|
||||
// Two modes, chosen by whether any existing step has left 'pending':
|
||||
// - Fresh/revise (no step started yet): full replace (delete + insert). This
|
||||
// covers the first call, and a genuine re-plan before any work began.
|
||||
// - Mid-flight (some step is running/done/failed/…): APPEND the new steps
|
||||
// after the current max seq instead of wiping. The model is instructed to
|
||||
// propose the whole plan in one call, but nothing stops it from calling
|
||||
// propose_plan again per-step as it goes — a destructive replace in that
|
||||
// case would erase every already-completed step, leaving the operator
|
||||
// seeing only the most recent single step ("1/1") instead of real
|
||||
// progress. Appending makes the panel's step history correct regardless
|
||||
// of how the model chooses to call the tool.
|
||||
// - Mid-flight (some step is running/done/failed/…): REFUSE the call.
|
||||
// The agent must advance the existing plan with update_plan_step + run
|
||||
// instead of re-proposing. The previous append-mode safety net (commit
|
||||
// 5384499) preserved history but produced a confusing duplicate sidebar
|
||||
// when the agent re-proposed on "proceed" (operator-reported 2026-07-14).
|
||||
// Refusing is the correct default — the tool result tells the agent how
|
||||
// to advance, and the generation column tracks revisions if a genuine
|
||||
// re-plan is ever allowed.
|
||||
func (s *store) proposePlan(ctx context.Context, sessionID string, steps []planStepInput) ([]map[string]any, error) {
|
||||
if s == nil || sessionID == "" || sessionID == "ephemeral" {
|
||||
return nil, nil
|
||||
@@ -429,23 +507,33 @@ func (s *store) proposePlan(ctx context.Context, sessionID string, steps []planS
|
||||
FROM session_plan_steps WHERE session_id = $1`, sessionID).Scan(&startSeq, &anyStarted); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if !anyStarted {
|
||||
if _, err := tx.Exec(ctx, `DELETE FROM session_plan_steps WHERE session_id = $1`, sessionID); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
startSeq = 0
|
||||
} else {
|
||||
// Mid-flight plan revision: mark any still-pending steps from the
|
||||
// previous generation as 'replaced' so the panel doesn't show them
|
||||
// as incomplete forever. Only pending steps — already-running or
|
||||
// done steps from the prior plan are preserved as history.
|
||||
if _, err := tx.Exec(ctx, `
|
||||
UPDATE session_plan_steps
|
||||
SET status = 'replaced', finished_at = now()
|
||||
WHERE session_id = $1 AND status = 'pending'`, sessionID); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if anyStarted {
|
||||
// A plan is already in flight (a step is running/done/failed/...).
|
||||
// Refuse the re-proposal — the agent must advance with
|
||||
// update_plan_step + run. The caller surfaces a directive.
|
||||
return nil, errPlanInFlight
|
||||
}
|
||||
// Fresh/revise: delete any prior pending steps and start a new
|
||||
// generation. The DELETE covers the genuine pre-execution revision
|
||||
// case (operator asked to revise before any step started).
|
||||
if _, err := tx.Exec(ctx, `DELETE FROM session_plan_steps WHERE session_id = $1`, sessionID); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
startSeq = 0
|
||||
|
||||
// Resolve the generation number for this plan. Generation 1 is the
|
||||
// initial plan; a genuine revise (which currently goes through the same
|
||||
// fresh-start path above because all steps were pending) resets to 1
|
||||
// since the DELETE wiped the prior rows. The column is wired here so a
|
||||
// future explicit mid-flight revise path can increment it.
|
||||
var nextGen int
|
||||
if err := tx.QueryRow(ctx, `
|
||||
SELECT COALESCE(MAX(generation), 0) + 1
|
||||
FROM session_plan_steps WHERE session_id = $1`, sessionID).Scan(&nextGen); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
// After the DELETE above, no rows remain, so MAX(generation) is NULL →
|
||||
// nextGen = 1. (Keep the query for the future revise path; it's cheap.)
|
||||
|
||||
out := make([]map[string]any, 0, len(steps))
|
||||
for i, st := range steps {
|
||||
@@ -456,14 +544,15 @@ func (s *store) proposePlan(ctx context.Context, sessionID string, steps []planS
|
||||
seq := startSeq + i + 1
|
||||
var id uuid.UUID
|
||||
if err := tx.QueryRow(ctx, `
|
||||
INSERT INTO session_plan_steps (session_id, seq, title, detail, target_slug)
|
||||
VALUES ($1, $2, $3, $4, $5) RETURNING id`,
|
||||
sessionID, seq, st.Title, st.Detail, targetSlug).Scan(&id); err != nil {
|
||||
INSERT INTO session_plan_steps (session_id, seq, title, detail, target_slug, generation)
|
||||
VALUES ($1, $2, $3, $4, $5, $6) RETURNING id`,
|
||||
sessionID, seq, st.Title, st.Detail, targetSlug, nextGen).Scan(&id); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
out = append(out, map[string]any{
|
||||
"id": id.String(), "seq": seq, "title": st.Title,
|
||||
"detail": st.Detail, "target_slug": st.TargetSlug,
|
||||
"generation": nextGen,
|
||||
})
|
||||
}
|
||||
if _, err := tx.Exec(ctx,
|
||||
@@ -474,10 +563,10 @@ func (s *store) proposePlan(ctx context.Context, sessionID string, steps []planS
|
||||
return nil, err
|
||||
}
|
||||
// Event after commit so subscribers only ever see a persisted plan.
|
||||
// appended=true tells the panel to add these steps to its existing list
|
||||
// rather than replace it (mirrors the mid-flight append above).
|
||||
// appended=false (always now — we refuse mid-flight re-proposals) tells
|
||||
// the panel to replace its list with these steps.
|
||||
_ = observability.Event(ctx, sqlcgen.New(s.pool), "plan.proposed", s.taskEntityPtr(ctx, sessionID),
|
||||
"info", "nomos", sessionID, map[string]any{"steps": out, "appended": anyStarted})
|
||||
"info", "nomos", sessionID, map[string]any{"steps": out, "appended": false, "generation": nextGen})
|
||||
// Record a plan-proposed window so the `run` handler knows a plan is
|
||||
// pending approval and can skip per-action approval for config_mutation
|
||||
// commands within the plan. Transitions to 'active' when the operator
|
||||
@@ -703,6 +792,7 @@ type planStep struct {
|
||||
TargetSlug *string `json:"target_slug,omitempty"`
|
||||
StartedAt *string `json:"started_at,omitempty"`
|
||||
FinishedAt *string `json:"finished_at,omitempty"`
|
||||
Generation int `json:"generation"`
|
||||
}
|
||||
|
||||
// getPlanSteps returns a task's plan in order — REST hydration for the context
|
||||
@@ -714,7 +804,7 @@ func (s *store) getPlanSteps(ctx context.Context, sessionID string) ([]planStep,
|
||||
rows, err := s.pool.Query(ctx, `
|
||||
SELECT id::text, seq, title, detail, status,
|
||||
execution_id::text, target_slug,
|
||||
started_at::text, finished_at::text
|
||||
started_at::text, finished_at::text, generation
|
||||
FROM session_plan_steps WHERE session_id = $1 ORDER BY seq`, sessionID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -725,7 +815,7 @@ func (s *store) getPlanSteps(ctx context.Context, sessionID string) ([]planStep,
|
||||
var st planStep
|
||||
var execID, target, started, finished *string
|
||||
if err := rows.Scan(&st.ID, &st.Seq, &st.Title, &st.Detail, &st.Status,
|
||||
&execID, &target, &started, &finished); err != nil {
|
||||
&execID, &target, &started, &finished, &st.Generation); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
st.ExecutionID, st.TargetSlug, st.StartedAt, st.FinishedAt = execID, target, started, finished
|
||||
|
||||
@@ -10,6 +10,7 @@ package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"math/rand"
|
||||
"os"
|
||||
@@ -159,19 +160,19 @@ func TestGetRecentMessages_Truncation(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestProposePlan_AppendVsReplace is the concrete proof for the plan-append
|
||||
// fix (commit 5384499, "plan panel showed only the latest step, not the full
|
||||
// plan"): proposePlan must REPLACE the step list only while every existing
|
||||
// step is still 'pending' (a genuine pre-execution revision), and APPEND
|
||||
// once any step has started — otherwise a model that calls propose_plan once
|
||||
// per step (rather than once with the full list, as instructed) erases every
|
||||
// already-completed step each time, and the operator only ever sees the
|
||||
// latest single step instead of real progress.
|
||||
func TestProposePlan_AppendVsReplace(t *testing.T) {
|
||||
// TestProposePlan_RefuseInFlight is the concrete proof for the plan-drift
|
||||
// fix (2026-07-14, "plan added twice in the sidebar"): proposePlan must
|
||||
// REPLACE the step list only while every existing step is still 'pending'
|
||||
// (a genuine pre-execution revision), and REFUSE the call once any step has
|
||||
// started. The prior append-mode safety net (commit 5384499) preserved
|
||||
// history but duplicated the plan in the sidebar when the agent re-proposed
|
||||
// on "proceed". Refusing is the correct default — the agent must advance
|
||||
// with update_plan_step + run.
|
||||
func TestProposePlan_RefuseInFlight(t *testing.T) {
|
||||
s := newTestStore(t)
|
||||
ctx := context.Background()
|
||||
|
||||
sess, err := s.createSession(ctx, "plan append test")
|
||||
sess, err := s.createSession(ctx, "plan refuse test")
|
||||
if err != nil {
|
||||
t.Fatalf("createSession: %v", err)
|
||||
}
|
||||
@@ -185,39 +186,37 @@ func TestProposePlan_AppendVsReplace(t *testing.T) {
|
||||
if len(out1) != 1 || out1[0]["seq"] != 1 {
|
||||
t.Fatalf("proposePlan #1 = %+v, want one step at seq 1", out1)
|
||||
}
|
||||
if out1[0]["generation"] != 1 {
|
||||
t.Fatalf("proposePlan #1 generation = %v, want 1", out1[0]["generation"])
|
||||
}
|
||||
|
||||
// Mark step 1 as started.
|
||||
if err := s.updatePlanStep(ctx, sess.ID, 1, "running", ""); err != nil {
|
||||
t.Fatalf("updatePlanStep: %v", err)
|
||||
}
|
||||
|
||||
// Second call, simulating a model that (against instructions) calls
|
||||
// propose_plan again per-step instead of once with the full list: since
|
||||
// step 1 has left 'pending', this MUST append, not replace.
|
||||
out2, err := s.proposePlan(ctx, sess.ID, []planStepInput{{Title: "Step B"}})
|
||||
if err != nil {
|
||||
t.Fatalf("proposePlan #2: %v", err)
|
||||
}
|
||||
if len(out2) != 1 || out2[0]["seq"] != 2 {
|
||||
t.Fatalf("proposePlan #2 = %+v, want one step at seq 2 (appended after the running step 1)", out2)
|
||||
// Second call, simulating a model that re-proposes mid-flight (the
|
||||
// operator-reported "proceed" bug): since step 1 has left 'pending',
|
||||
// this MUST refuse with errPlanInFlight, not append or replace.
|
||||
_, err = s.proposePlan(ctx, sess.ID, []planStepInput{{Title: "Step B"}})
|
||||
if !errors.Is(err, errPlanInFlight) {
|
||||
t.Fatalf("proposePlan #2: err = %v, want errPlanInFlight (refuse mid-flight re-proposal)", err)
|
||||
}
|
||||
|
||||
// The original step 1 must be untouched — not erased, not appended to.
|
||||
steps, err := s.getPlanSteps(ctx, sess.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("getPlanSteps: %v", err)
|
||||
}
|
||||
if len(steps) != 2 {
|
||||
t.Fatalf("got %d persisted steps, want 2 (step 1 must survive the second propose_plan call)", len(steps))
|
||||
if len(steps) != 1 {
|
||||
t.Fatalf("got %d persisted steps, want 1 (refused call must not mutate the plan)", len(steps))
|
||||
}
|
||||
if steps[0].Title != "Step A" || steps[0].Status != "running" {
|
||||
t.Errorf("step 1 = %+v, want Step A still running (not erased)", steps[0])
|
||||
}
|
||||
if steps[1].Title != "Step B" || steps[1].Status != "pending" {
|
||||
t.Errorf("step 2 = %+v, want Step B pending", steps[1])
|
||||
t.Errorf("step 1 = %+v, want Step A still running (refused call must not touch it)", steps[0])
|
||||
}
|
||||
|
||||
// Third call BEFORE anything runs on a fresh session: every step is
|
||||
// still pending, so this must REPLACE, not append.
|
||||
// still pending, so this must REPLACE, not refuse.
|
||||
sess2, err := s.createSession(ctx, "plan replace test")
|
||||
if err != nil {
|
||||
t.Fatalf("createSession: %v", err)
|
||||
@@ -233,6 +232,9 @@ func TestProposePlan_AppendVsReplace(t *testing.T) {
|
||||
t.Fatalf("getPlanSteps: %v", err)
|
||||
}
|
||||
if len(revisedSteps) != 1 || revisedSteps[0].Title != "Revised" {
|
||||
t.Fatalf("got %+v, want a single 'Revised' step (pre-execution revise must replace, not append)", revisedSteps)
|
||||
t.Fatalf("got %+v, want a single 'Revised' step (pre-execution revise must replace, not refuse)", revisedSteps)
|
||||
}
|
||||
if revisedSteps[0].Generation != 1 {
|
||||
t.Fatalf("revised step generation = %d, want 1 (fresh-start after DELETE resets generation)", revisedSteps[0].Generation)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"strings"
|
||||
@@ -34,20 +35,16 @@ func taskToolDefs() []toolDef {
|
||||
},
|
||||
{
|
||||
Name: "propose_plan",
|
||||
Description: "Lay out ALL the ordered steps you'll take to reach the goal, in ONE " +
|
||||
"call, listing every step end-to-end — not just the next one. The operator " +
|
||||
"sees the full list in the context panel and watches it progress; a plan " +
|
||||
"with only 1 step looks broken to them even if you intend to add more later. " +
|
||||
"Your FIRST step should be research (prior knowledge, relations, blast radius " +
|
||||
"— not just this target's status) and your LAST step should be writing back " +
|
||||
"what you learned (update_entity_attributes / create_relationship / " +
|
||||
"upsert_knowledge) BEFORE complete_task — this is what keeps the knowledge " +
|
||||
"graph from drifting out of date. " +
|
||||
"Call this ONCE, before you start executing (after gathering what you need). " +
|
||||
"As you work, call update_plan_step (not propose_plan again) to advance each " +
|
||||
"step. Only re-call propose_plan if the plan itself has fundamentally changed " +
|
||||
"(e.g. a new approach is needed) — in that case new steps are appended after " +
|
||||
"whatever already ran, never erasing completed work.",
|
||||
Description: "Propose the full ordered plan for this task. Call ONCE, before any " +
|
||||
"execution, with EVERY step end-to-end (not one step at a time). FIRST step: " +
|
||||
"research (prior knowledge, relations, blast radius). LAST step: write back " +
|
||||
"(update_entity_attributes + create_relationship + upsert_knowledge) BEFORE " +
|
||||
"complete_task — this keeps the knowledge graph from drifting. After this " +
|
||||
"call: STOP and wait for operator approval (approval vocabulary: approved, " +
|
||||
"yes, go, proceed, continue, ok, go ahead). Once a step has started " +
|
||||
"(running/done/...), this tool REFUSES further calls — advance with " +
|
||||
"update_plan_step + run instead. Re-propose only if the operator explicitly " +
|
||||
"asks you to revise the whole plan.",
|
||||
InputSchema: map[string]any{
|
||||
"type": "object",
|
||||
"properties": map[string]any{
|
||||
@@ -205,6 +202,15 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args
|
||||
}
|
||||
persisted, err := a.store.proposePlan(ctx, sessionID, steps)
|
||||
if err != nil {
|
||||
if errors.Is(err, errPlanInFlight) {
|
||||
// The plan is already in flight — refuse the re-proposal.
|
||||
// The agent must advance the existing plan with
|
||||
// update_plan_step + run. This is the structural fix for
|
||||
// the "plan added twice" sidebar drift the operator
|
||||
// reported: instead of appending (which duplicated) or
|
||||
// wiping (which lost progress), we refuse and direct.
|
||||
return "Plan already in flight — refusing duplicate proposal. Steps exist and at least one has started (running/done/...). To advance: call update_plan_step(seq=K, status=\"running\") then run(...) for step K's target, then update_plan_step(seq=K, status=\"done\"). Do NOT call propose_plan again. Re-propose only if the operator explicitly asks you to revise the whole plan, and say so in your reply before calling it.", true
|
||||
}
|
||||
return fmt.Sprintf("error proposing plan: %v", err), true
|
||||
}
|
||||
// Nudge: if the last step doesn't mention entity writeback tools,
|
||||
@@ -212,7 +218,7 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args
|
||||
lastStep := steps[len(steps)-1]
|
||||
hasWriteback := strings.Contains(lastStep.Title+lastStep.Detail, "update_entity_attributes") ||
|
||||
strings.Contains(lastStep.Title+lastStep.Detail, "create_relationship")
|
||||
result := fmt.Sprintf("Plan set: %d step(s). Now STOP and present the plan to the operator — do NOT call run yet. Wait for them to approve (they will type 'approved'/'yes'). Once approved, all config_mutation commands will auto-execute without individual approval popups.", len(persisted))
|
||||
result := fmt.Sprintf("Plan set (%d steps). STOP. Wait for operator approval — do not call run yet. Approval vocabulary: \"approved\", \"yes\", \"go\", \"proceed\", \"continue\", \"ok\", \"go ahead\". On approval, advance with update_plan_step + run. Do not call propose_plan again.", len(persisted))
|
||||
if !hasWriteback {
|
||||
result += "\n\n⚠️ The final step doesn't mention update_entity_attributes or create_relationship. Without those, any facts you discovered about entities (IPs, versions, hosts, states) will be LOST — the next session starts from scratch. Consider revising the last step to include entity writeback BEFORE completing the task."
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user