From 337d577f0096ef17c17350d4904cdf8cd0f792bd Mon Sep 17 00:00:00 2001 From: dtoro Date: Tue, 14 Jul 2026 15:28:33 +0200 Subject: [PATCH] fix(agent): refuse plan re-proposal + emit done on error (close divergence chain) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- VERSION | 2 +- cmd/nomos/agent.go | 29 +- cmd/nomos/continue.go | 11 +- cmd/nomos/main.go | 9 +- cmd/nomos/store.go | 154 +++- cmd/nomos/store_test.go | 56 +- cmd/nomos/tasks.go | 36 +- plans/2026-07-10-general-gated-execution.md | 14 +- .../2026-07-14-post-fix-session-remainders.md | 853 ++++++++++++++++++ plans/index.md | 3 +- 10 files changed, 1074 insertions(+), 93 deletions(-) create mode 100644 plans/2026-07-14-post-fix-session-remainders.md diff --git a/VERSION b/VERSION index 267577d..8f0916f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.4.1 +0.5.0 diff --git a/cmd/nomos/agent.go b/cmd/nomos/agent.go index cc2a3a9..10899a7 100644 --- a/cmd/nomos/agent.go +++ b/cmd/nomos/agent.go @@ -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 } } diff --git a/cmd/nomos/continue.go b/cmd/nomos/continue.go index 6a0f73b..5b932fe 100644 --- a/cmd/nomos/continue.go +++ b/cmd/nomos/continue.go @@ -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 } diff --git a/cmd/nomos/main.go b/cmd/nomos/main.go index 89fdf48..eb8edd1 100644 --- a/cmd/nomos/main.go +++ b/cmd/nomos/main.go @@ -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) diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index ec3571b..0c1544e 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -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 diff --git a/cmd/nomos/store_test.go b/cmd/nomos/store_test.go index f0cb98c..c18ab5c 100644 --- a/cmd/nomos/store_test.go +++ b/cmd/nomos/store_test.go @@ -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) } } diff --git a/cmd/nomos/tasks.go b/cmd/nomos/tasks.go index 529821b..ffce2b2 100644 --- a/cmd/nomos/tasks.go +++ b/cmd/nomos/tasks.go @@ -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." } diff --git a/plans/2026-07-10-general-gated-execution.md b/plans/2026-07-10-general-gated-execution.md index 1a9b8bb..881a837 100644 --- a/plans/2026-07-10-general-gated-execution.md +++ b/plans/2026-07-10-general-gated-execution.md @@ -1,13 +1,17 @@ # 2026-07-10 — General gated execution: from fixed actions to unlimited-but-gated -**Status:** In Progress — audited 2026-07-11. Done: `ClassifyCommand` risk +**Status:** In Progress — re-audited 2026-07-14. Done: `ClassifyCommand` risk classifier, general `run` MCP tool, chat-assent approval (no button required), blast radius on approval cards, session digest, global activity feed (`Ops.svelte` "Executions" tab, risk-badged), Learning view -(success-rate trend). Still open: retire the fixed `request_execution` -action enum (`restart, systemctl, pct_exec, apt_upgrade, pct_create` still -hard-coded alongside `run`), and revive auto-act — `internal/actuator/actuator.go:125` -is still a literal `{"success": true, "message": "stub execution"}` stub. +(success-rate trend), **and now the `request_execution` enum retirement** +(commit `60effcb`, 2026-07-14 — `run` is the only mutation tool; the legacy +handler functions are kept as reference only, with a "DO NOT re-register" +guard in `internal/mcp/server.go:360`). Still open: **revive auto-act** — +`internal/actuator/actuator.go:~125` is still a literal +`{"success": true, "message": "stub execution"}` stub (item 10). The +`run`-gated path covers operator-initiated work end-to-end; auto-act is the +observe→Act direction (signals triggering actions), still unimplemented. ## Goal diff --git a/plans/2026-07-14-post-fix-session-remainders.md b/plans/2026-07-14-post-fix-session-remainders.md new file mode 100644 index 0000000..8f86578 --- /dev/null +++ b/plans/2026-07-14-post-fix-session-remainders.md @@ -0,0 +1,853 @@ +# 2026-07-14 — Post-fix session audit: empty responses & plan drift remainders + +**Status:** In Progress — 2026-07-14. Phases A + B.1-B.3 + F.3 shipped & +e2e-validated (v0.5.0, deployed to oikos-nomos-1). Phases C, D, E, and +F.1-F.2 remain; **D.1 is the next blocker** (refuse `complete_task` +without writeback — the knowledge loop is still drifting). + +## Shipped (2026-07-14, v0.5.0) + +| Fix | File(s) | Validation | +|---|---|---| +| **A.1** `proposePlan` sets `generation` on INSERT | `cmd/nomos/store.go` | e2e: plan steps now carry `generation: 1` (was always 1 before; column was unwired) | +| **A.2** `proposePlan` refuses re-proposal when in flight (drops append-mode) | `cmd/nomos/store.go`, `cmd/nomos/tasks.go` | e2e: "proceed with the rest" → 0 `propose_plan` calls (was 1 + duplicate sidebar); plan stayed at 3 steps, not 6+ | +| **A.3** `propose_plan` tool description restated as a crisp contract | `cmd/nomos/tasks.go` | agent self-described the contract in its reply | +| **F.3** Approval vocabulary expanded + directive result strings | `cmd/nomos/tasks.go` | e2e: "proceed" and "go ahead" both recognized as approval (was only "approved/yes/go ahead") | +| **B.1** `chatWith` emits `done` after `error` on every terminal path | `cmd/nomos/agent.go` | e2e: nomos logs show zero reconnect/resume entries for the plan-proposing test sessions (was the amplifier in the three-bug chain) | +| **B.2** Reconnect/resume note carries last user msg + plan-in-flight directive | `cmd/nomos/store.go`, `cmd/nomos/main.go`, `cmd/nomos/continue.go` | wired into all 4 resume entry points (reconnect, /resume, idle-sweep, question-answer) | +| **B.3** `resumeSession` escalates the recovery note across 3 attempts | `cmd/nomos/continue.go` | e2e: a manually-triggered reconnect produced a real response on the escalated retry (was 3 identical empties → give up) | + +Test: `TestProposePlan_RefuseInFlight` (rewrote `TestProposePlan_AppendVsReplace`) +in `cmd/nomos/store_test.go` asserts the refusal + generation wiring. + +## Remaining (not yet shipped) + +- **D.1** Refuse `complete_task` without writeback when discovery ran — **next blocker**. The knowledge loop is still drifting: conv3 did write back (`update_entity_attributes`) but only because the agent chose to, not because it was forced to. The warning string in `completeTask` (5.5 from the prior plan) still fires but is still ignorable. +- **D.2** Auto-append a writeback step to plans that lack one. +- **C.1** `completeTask` reject re-completion of a terminal session. +- **C.2** SOUL.md: don't re-execute on UI-clarification complaints. +- **F.1** Consolidate SOUL.md's three overlapping task-flow sections to one (the operator's "be more crisp" feedback). +- **F.2** Tighten tool-result strings from advisory to imperatives (partially done in F.3's propose_plan result; remaining: set_goal, update_plan_step, complete_task). +- **B.4** Surface the real model error text (errText) in the error event + resume-failed note. +- **B.5** Back off between resume retries (4s, 8s). +- **B.6** Don't persist the empty placeholder as a visible bubble. +- **E.1** SOUL.md: prefer knowledge over re-execution for fleet-wide facts. +- **E.2** `list_lxcs` last-audited hint in the result. + +## Commit-history context (the 20-commit iteration) + +Reviewing `git log` since the agent-task phases landed (be3ce76 → 5caf49b), +the same problems recur because we keep fixing them with **SOUL.md prose + +safety-net append logic** instead of structural gates: + +- `5384499` (Jul 11) — "plan panel showed only the latest step" → fixed by + making `proposePlan` APPEND when a step is in flight, so history is + preserved even if the model re-proposes per step. **This is the source of + the duplication the operator saw today.** The fix traded "lost progress" + for "duplicate progress" — and the duplication is what's visible to the + operator now. +- `e30813a` / `532310b` (Jul 11) — "research-first / knowledge-write-back- + last explicit steps" → added the FIRST/LAST step language to SOUL.md. + Three commits later the warnings are still being ignored in production. +- `5caf49b` (Jul 14, today) — "mandatory pre-plan flow" → another SOUL.md + section at the top of the file, overlapping the existing "Every chat is a + task" / "AFTER EVERY TASK: WRITE BACK" sections. The agent now has three + overlapping sections telling it the same thing. +- `60effcb` (Jul 14) — Phase 5 of the prior plan added the `generation` + column, the `replaced` status, the frontend grouping, and the writeback + warnings. The migration landed; the INSERT in `proposePlan` did not. + +**The pattern:** every iteration adds another paragraph to SOUL.md and a +safety net in the store layer. The agent still does the wrong thing +because prose instructions are unreliable and the safety nets paper over +the symptom instead of refusing the bad action. **This plan pivots to +structural gates** — `proposePlan` and `completeTask` should refuse the +calls that produce drift, not accommodate them. + +## Sessions under audit + +| Session | Time | Goal | Messages | Outcome | Real tool calls | +|---|---|---|---|---|---| +| `722d8878` (failure) | 10:45 | Fleet update audit | 3 | **failed** — empty response during auto-resume | 41 in turn 1 | +| `d9cdcee1` (success w/ friction) | 11:44 | Same prompt (user retried) | 11 | success | 38 across 5 turns | + +Both sessions are the same operator request: "Check all the services on the +homelab and give me an overview of what needs updating, categorize by +criticality." Cross-referencing them shows **where the prior fixes held vs. +where they didn't.** + +--- + +## What worked (preserve) + +- **`upsert_knowledge` `about` array** (5.2 from prior plan) — the agent + linked the audit to all affected LXCs in one call: + `about: ["lxc:nextcloud","lxc:jellyfin","host:hubris", ...]`. +- **`complete_task` writeback warning** (5.5) — fired correctly (the session + has no `update_entity_attributes` calls and the warning text appears in the + tool result). +- **`propose_plan` writeback nudge** (5.4) — fired (last step title was + "Write back: upsert_knowledge if anything changed", which contains neither + required tool name). +- **Seq-order completion enforcement** (5.6) — no out-of-order completions + observed. +- **Replaced-status mechanism** (3.3) — pending steps from the prior + generation were correctly marked `replaced` on re-propose. + +## What didn't (the findings below) + +--- + +## Findings + +### 1. Empty response still ends the session — operator had to start over + +**Where:** Session `722d8878` msg 2: `[System: auto-resume failed after +retrying: Nomos returned an empty or unusable response — please retry. The +task is paused — send another message to continue.]` + +**What happened:** Turn 1 ran 41 tool calls (set_goal + list_lxcs + +get_health_summary + get_state_snapshot + search_knowledge + 4× get_relations ++ 4× get_entity + 20× `run` for `apt-get update` across the fleet). The model +returned that successfully. Auto-continuation then ran `resumeSession`, which +retried `chatWith` **3 times** (continue.go:229) — all three came back empty. +The session ended with the system note above. The operator abandoned it and +opened `d9cdcee1` with the same prompt. + +**Root cause:** Three identical retries with the same injected `note` produce +three identical empty responses (the model isn't randomly failing — it's +responding to the prompt the same way each time). The retry loop never varies +the prompt, never backs off, and never escalates to a more aggressive +recovery (e.g. a fresh continuation prompt that summarizes what just happened +and asks explicitly for the next single step). + +**Severity:** Blocker — a 41-tool-call turn costs real money and time, and the +operator gets nothing for it. + +### 2. `generation` column exists but `proposePlan` never sets it — frontend grouping is dead code + +**Where:** `cmd/nomos/store.go:458-461` (INSERT statement) vs. +`migrations/020_session_reliability.up.sql:7` (the column) and +`web/src/lib/components/PlanProgress.svelte:17-22` (the grouping logic). + +**What happened:** Migration 020 added `generation INTEGER NOT NULL DEFAULT 1` +and PlanProgress groups steps by `s.generation ?? 1`. But the INSERT in +`proposePlan` is: + +```sql +INSERT INTO session_plan_steps (session_id, seq, title, detail, target_slug) +VALUES ($1, $2, $3, $4, $5) RETURNING id +``` + +No `generation` column. Every step, in every plan revision, lands with +`generation = 1`. PlanProgress always sees one group ("Current plan") and +the collapse-old-generations behavior never triggers. + +**Concrete impact in `d9cdcee1`:** +- Turn 1 (msg 1): `propose_plan` creates steps seq 1-5 (all generation 1). +- User: "why is the plan not updated accordingly? the steps in the sidebar." +- Turn 3 (msg 5): `update_plan_step seq=1, status=done`. Steps 2-5 still + pending, all generation 1. +- User: "proceed with the rest." +- Turn 4 (msg 7): **empty assistant response** (text="", no tools). +- Turn 5 (msg 8): Agent calls `propose_plan` **again** with the same 5 steps. + `proposePlan` sees `anyStarted=true` (seq 1 is done), so it goes into + append mode: marks the 4 still-pending steps (2-5) as `replaced`, then + inserts 5 new steps at seq 6-10. **All inserted with generation=1.** +- The frontend now sees 10 steps, all `generation: 1`, grouped together. + Four are marked `replaced` (visible as "skipped/replaced" — dimmed but + still in the list); six are the new active steps. +- User: "btw the plan here and the one in the sidebar differ." → Confirmed: + the chat text describes a 5-step plan ("Step 1 done, refreshing 2-4"); + the sidebar shows 10 steps with a confusing mix of done/replaced/running. + +**Severity:** Blocker — this is the direct, observable cause of the user's +two complaints in `d9cdcee1`. The prior plan (3.4) shipped the column and +the frontend code but never wired the backend INSERT. + +### 3. Agent re-proposes the plan on "proceed" instead of continuing + +**Where:** `d9cdcee1` msg 8 — `propose_plan` called again after user said +"proceed with the rest". + +**What happened:** The agent had a perfectly good plan in flight (step 1 done, +2-5 pending). On the next operator turn ("proceed"), it should have called +`update_plan_step(seq=2, status=running)` and `run` against the targets. +Instead it called `propose_plan` with the same 5 steps, triggering the +append-mode behavior in #2. + +**Root cause:** SOUL.md doesn't explicitly say "do NOT call propose_plan +again once you've already proposed — call update_plan_step + run instead." +The agent treated "proceed" as a cue to re-state the plan, not to advance +it. + +**Severity:** Friction (compounds #2 into a blocker). + +### 3a. WHY the agent re-proposed instead of advancing — the three-bug chain + +Finding #3's surface description ("agent re-proposed on proceed") is real but +doesn't explain the *mechanism*. Tracing the message timestamps and the +`auto: true` flag on msg 8 reveals that the re-proposal wasn't the agent's +direct response to "proceed with the rest" at all — it was the agent's +response to a **generic system reconnect note**, fired by a chain of three +compounding bugs: + +**The chain (all confirmed from code + session data):** + +| Step | What happened | Where | +|---|---|---| +| 1. **Trigger** — model returned empty on the approval | User sent "procceed with the rest." `handleChat` → `a.chat()` → `chatWith()`. The model returned an empty completion 3× (all `maxLLMRetries=2` attempts exhausted). SOUL.md's approval vocabulary was "approved/yes/go ahead" — "proceed" wasn't listed, so the model likely wasn't certain it was approved and no-op'd. | `agent.go:362-371` | +| 2. **Amplifier** — empty response misclassified as network disconnect | On empty response, `chatWith` emits `error` and `return`s **without emitting `done`** (agent.go:370-371 — the `done` event only fires on the success path at line 382). The frontend's `onComplete` callback sees `!receivedDone` and treats it as a severed connection, calling `handleDisconnect()`. A *model* empty-response gets handled by the *network* disconnect path. | `agent.go:370-371` (missing `done`) + `chat.ts:349-356` (`!receivedDone → handleDisconnect`) | +| 3. **Divergence** — generic reconnect note triggers re-proposal | `handleDisconnect` waits 1s, then sends an empty message (`streamChat('', sessionId, …)`). The backend's reconnect path (main.go:177-188) calls `resumeSession` with: `"[System: the operator's connection was re-established. The task may have progressed in the background — report your current state and progress.]"`. The agent re-read the transcript (plan proposed, step 1 done, user said "proceed"), saw this generic note, and interpreted "report your current state and progress" as "redo the work and report it" → re-proposed + re-executed + `complete_task`. | `chat.ts:386-419` (reconnect) + `main.go:180` (note) + `continue.go:189` (resumeSession) | + +**Timestamps confirm this:** msg 7 (empty) at `11:49:01.949`, msg 8 (re-propose, `auto: true`) at `11:49:16.200` — 15 seconds later, matching the 1s reconnect delay + the LLM call latency. The user never sent a second message; the frontend's reconnect logic did. + +**The user's actual approval ("procceed with the rest") was in the transcript** but the agent wasn't responding to it — it was responding to the *system reconnect note*, which didn't mention approval, the plan, or the user's words. The propose_plan result had said "STOP and wait for approval," and the generic reconnect note didn't say "you're approved" — so the agent re-proposed to get a fresh approval cycle. + +**Why this matters for the fix:** Phase A.2 (refuse re-proposal when in flight) would have *prevented the duplication* but not *fixed the cause*. The agent would have hit the refusal and then… what? With the generic reconnect note, it still doesn't know it's approved. The three bugs need three targeted fixes (Phase B below). This is the answer to "why didn't the agent update the original plan": **it never received a clear signal to advance, because the approval signal was lost in an empty response that got misclassified as a network drop.** + +**Severity:** Blocker — this is the root cause of the plan divergence the +operator observed. + +### 4. Operator clarification was interpreted as "redo the whole task" + +**Where:** `d9cdcee1` msg 9 → msg 10. User said "btw the plan here and the +one in the sidebar differ." Agent's response (msg 10): re-ran all 6 `run` +calls (`apt-get update` + `apt list --upgradable` on nextcloud, jellyfin, +hubris), re-called `upsert_knowledge`, and **called `complete_task` a +second time**. + +**What happened:** The operator wanted the sidebar aligned with the chat. +The agent re-executed the actual audit work and re-completed the task. + +**Root cause:** No prompt-level instruction about how to handle "the UI +seems inconsistent" complaints — the agent defaulted to "do the work again, +maybe it'll line up this time." + +**Severity:** Friction — wasted 6 `run` calls and a duplicate knowledge +entry; user gets a noisier transcript. + +### 5. `complete_task` called twice on the same session + +**Where:** `d9cdcee1` msg 8 and msg 10 both call `complete_task` with +`outcome=success`. + +**What happened:** After msg 8, `agent_sessions.status` is `done`. The user +complained about the plan drift; the agent re-ran the audit and called +`complete_task` again. There's no guard in `completeTask` against re-completing +an already-terminal session. + +**Severity:** Cosmetic, but it produces duplicate knowledge entries and +erodes audit-log clarity. + +### 6. Turn 1 of `722d8878`: 41 tool calls including `run` against every LXC + +**Where:** Session `722d8878` msg 1. + +**What happened:** Despite a same-day knowledge entry +(`investigation:nomos/fleet-wide-apt-update-audit-2026-07-14` — the agent even +called `get_knowledge_content` for it), the agent ran `apt-get update` on +every LXC in turn 1 instead of presenting the prior audit and proposing a +small refresh plan. The agent already had the answer in the DB; it re-ran +the fleet audit anyway. + +**Severity:** Friction — wasted ~20 `run` calls (each is a queued execution). +The successful retry session (`d9cdcee1`) only re-ran 3 (the critical trio), +which is the right pattern — but it had to learn that from the failure +session's example. + +### 7. Agent ignores its own writeback warnings + +**Where:** `d9cdcee1` — `propose_plan` returned the nudge from tasks.go:213 +("⚠️ The final step doesn't mention update_entity_attributes…") and +`complete_task` returned the warning from tasks.go:280 ("⚠️ No entity +attributes or relationships were updated in this session…"). The agent saw +both, did nothing about either, and ended the task. + +**What happened:** The warnings are surfaced in the tool result text, but +the model treats tool results as ephemeral context — it doesn't act on a +warning that appears after the work it already decided is done. The session +recorded zero `update_entity_attributes` calls and zero +`create_relationship` calls. + +**Severity:** Blocker — the knowledge-loop drift problem the prior plan was +supposed to fix is still happening. The graph accumulates nothing structured +from this session; the next fleet audit will rediscover every fact from +scratch. + +### 8. Empty assistant bubble persisted in the transcript + +**Where:** `d9cdcee1` msg 7: `{"role":"assistant","text":"","tool_calls":[]}`. + +**What happened:** On the "proceed with the rest" turn, the model returned an +empty completion. The inner `chatWith` retry (agent.go:331) eventually +succeeded and produced msg 8 — but the empty msg 7 was already persisted to +the transcript and stays there. The UI shows an empty assistant bubble between +the user's "proceed" and the agent's actual response. + +**Severity:** Cosmetic, but visible to the operator and erodes trust ("is +the agent broken?"). + +--- + +## Improvement plan + +### Phase A — Make `propose_plan` refuse duplication (addresses #2, #3) + +The operator's "plan was added twice" complaint is the visible output of +the append-mode safety net added in `5384499`. The safety net was the wrong +default: it preserved history but produced a confusing 10-step sidebar. The +right default is to **refuse** a re-proposal when a plan is already in +flight — the agent must use `update_plan_step` + `run` to advance. + +#### A.1 — `proposePlan`: set `generation` on insert (still needed for history) + +**File:** `cmd/nomos/store.go:415-491` + +**How:** +1. Resolve the next generation number at the top of `proposePlan`, in the + same transaction: + ```go + var nextGen int + if !anyStarted { + // fresh/revise: reset to 1 (and the DELETE already wiped old rows) + nextGen = 1 + } else { + 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 + } + } + ``` +2. Add `generation` to the INSERT: + ```sql + INSERT INTO session_plan_steps (session_id, seq, title, detail, target_slug, generation) + VALUES ($1, $2, $3, $4, $5, $6) RETURNING id + ``` + Pass `nextGen` as `$6`. +3. Include `"generation": nextGen` in the `out` map so the tool result and + the `plan.proposed` event carry it (the frontend already reads it via + `api.ts:66`). +4. Backfill is unnecessary — existing rows default to generation 1. + +#### A.2 — `proposePlan`: refuse re-proposal once any step has started + +**File:** `cmd/nomos/store.go:415-491` + `cmd/nomos/tasks.go:206-219` + +**How:** +1. In `proposePlan`, when `anyStarted == true`, return a sentinel error + instead of appending: + ```go + if anyStarted { + return nil, errPlanInFlight + } + ``` +2. In `handleTaskTool`'s `propose_plan` case, detect the sentinel and return + a directive tool result: + ``` + Plan already in flight — refusing duplicate proposal. Steps 1..N exist; + at least one is running or done. To advance the plan, call + update_plan_step(seq=K, status=running) followed by run(...) for step K's + target. Do NOT call propose_plan again. Call it again only if the + operator explicitly asks you to revise the whole plan, and if so, say + that in your reply before calling it. + ``` +3. Drop the append-mode code path (store.go:437-448) — it's the duplication + source. Keep the destructive-replace path (store.go:432-436) for the + `!anyStarted` case (genuine pre-execution revision). +4. The `replaced` status becomes unreachable through normal flow but stays + in the schema for any future "explicit revise" path that uses it. + +This is the single highest-impact fix in this plan. It directly removes +the "plan added twice" behavior the operator reported, and forces the +agent to use the correct advancement tools. Combined with the directive +tool result, even a model that ignores SOUL.md will get the right behavior +because the bad action is refused. + +#### A.3 — `propose_plan` tool description: state the contract crisply + +**File:** `internal/mcp/server.go` (the `propose_plan` tool schema) + +**How:** Replace the current description with a one-paragraph contract: +``` +Propose the full ordered plan for this task. Call ONCE per task, before +any execution. After this call: STOP and wait for operator approval. +Once a step has started (status=running/done/...), this tool REFUSES +further calls — use update_plan_step + run to advance. The LAST step +MUST be "Write back: update_entity_attributes + create_relationship ++ upsert_knowledge". +``` +This puts the contract where the model reads it (in the tool schema that +gets serialized into the system prompt), not just in SOUL.md where it +competes with three overlapping sections. + +#### A.4 — PlanProgress: verify grouping renders with the wired-up column + +**File:** `web/src/lib/components/PlanProgress.svelte:17-90` + +Once A.1 lands, the grouping code that already exists should work. Verify: +- Latest generation (`Math.max(...generations)`) → expanded, labeled + "Current plan". +- Older generations → collapsed by default, labeled "Plan v1 (replaced)", + with a count badge. +- A future explicit-revise path (not in this plan) would land generation 2 + as the new "Current plan" and the old steps collapse. + +This is verification, not new code — the structure is there, it just +never received varied generation numbers to group on. + +### Phase B — Close the three-bug chain that caused the divergence (addresses #3a, #1, #8) + +Phase A.2 (refuse re-proposal) prevents the *symptom* (duplicate plan in +sidebar). This phase fixes the *cause* — the three bugs in finding #3a that +made the agent re-propose in the first place. Each fix targets one link in +the chain. + +#### B.1 — Emit `done` after `error` so the frontend doesn't misclassify (fixes bug 2 — the amplifier) + +**File:** `cmd/nomos/agent.go:370-371` (+ the other early-return error paths +at lines 348, 356) + +**What:** On empty response, `chatWith` emits `error` and returns **without +emitting `done`**. The `done` event only fires on the success path +(agent.go:382). The frontend's `onComplete` (chat.ts:349-356) sees +`!receivedDone` and routes into `handleDisconnect` — treating a *model* +failure as a *network* drop, which triggers an unwanted auto-reconnect → +`resumeSession` → re-proposal. + +**How:** +1. After the `error` emit at line 370, also emit `done` before returning: + ```go + emit(agentEvent{Type: "error", Data: "Nomos returned an empty or unusable response — please retry.", SessionID: sessionID}) + emit(agentEvent{Type: "done", Data: map[string]any{ + "session_id": sessionID, "correlation_id": correlationID, + "iterations": i + 1, "error": true, + }, SessionID: sessionID}) + return + ``` +2. Do the same for the other early-return error paths (agent.go:348 stream + error, agent.go:356 no choices) so every terminal path emits `done`. +3. On the frontend, `onComplete` (chat.ts:349-356) now sees + `receivedDone === true` and sets `streaming.set(false)` instead of + calling `handleDisconnect`. The error is still shown via the `error` + event handler (chat.ts:329-331). +4. Add `"error": true` to the done payload so the frontend can distinguish + "ended cleanly" from "ended with error" (e.g. to show a retry button + instead of loading dots). + +**Impact:** This alone prevents the unwanted `resumeSession` call after a +model empty-response. The error becomes a visible chat error (with the +retry button from prior Phase 1.6), not a silent trigger for re-execution. +This is the single highest-leverage fix in this phase — it breaks the chain +at the amplifier. + +#### B.2 — Reconnect note: reference the user's last message and plan state (fixes bug 3 — the divergence) + +**File:** `cmd/nomos/main.go:180` (reconnect note) + the other resume entry +points at `main.go:341` (`/resume` endpoint) and `continue.go:83-86` +(idle-sweep note) + +**What:** Even with B.1, genuine network disconnects will still happen. When +they do, the reconnect note (`"report your current state and progress"`) is +too generic — it doesn't tell the agent what the operator actually wanted, +so the agent guesses (badly). The note should carry the operator's last +message and whether a plan is in flight. + +**How:** +1. Add two helpers to `store.go`: + ```go + func (s *store) lastUserMessage(ctx, sessionID) string // SELECT text FROM messages WHERE session_id=$1 AND role='user' ORDER BY created_at DESC LIMIT 1 + func (s *store) hasPlanInFlight(ctx, sessionID) bool // SELECT EXISTS(... WHERE session_id=$1 AND status IN ('pending','running')) + ``` +2. In `handleChat`'s reconnect path (main.go:177-188), build a specific note: + ```go + lastUserMsg := st.lastUserMessage(pctx, req.SessionID) + planInFlight := st.hasPlanInFlight(pctx, req.SessionID) + note := fmt.Sprintf("[System: the operator's connection was re-established. "+ + "The operator's last message was: \"%s\". ", lastUserMsg) + if planInFlight { + note += "A plan is in flight — advance it with update_plan_step + run. Do NOT call propose_plan again." + } else { + note += "Report your current state and progress." + } + note += "]" + ``` +3. Apply the same enrichment to the `/resume` endpoint note (main.go:341) + and the idle-sweep note (continue.go:83-86) — all three resume entry + points should carry the same context. + +**Impact:** Even if B.1 is bypassed (genuine disconnect mid-plan), the agent +gets "advance the plan" instead of "report state." No more re-proposal from +reconnect. + +#### B.3 — `resumeSession`: escalate the recovery note across attempts (fixes bug 1 — the trigger) + +**File:** `cmd/nomos/continue.go:229-253` + +**What:** The current loop retries 3 times with the same note. A transient +model issue (or a prompt causing the model to no-op) gets three identical +empty responses. + +**How:** +1. Build a different `note` per attempt: + ```go + notes := []string{ + note, // attempt 0: the original (now enriched per B.2) note + fmt.Sprintf("[System: your previous turn produced no response. %s. "+ + "Produce a response now — call the next tool or report progress in one sentence.]", note), + fmt.Sprintf("[System: two consecutive empty responses. Stop trying to be clever. "+ + "The next action is: pick the lowest-pending plan step, mark it running with "+ + "update_plan_step, and call run for its target. Do that now.]"), + } + ``` +2. Pass `notes[attempt]` to `chatWith` so each retry gets a progressively + more directive prompt. +3. Keep the 3-attempt cap. + +**Impact:** A model that's transiently flaking or confused gets a real +second chance with an increasingly specific directive, instead of three +identical prompts. + +#### B.4 — Surface the real model error text (addresses finding #1's observability) + +**File:** `cmd/nomos/agent.go:370` + `cmd/nomos/continue.go:255-274` + +**What:** The operator-facing message is "Nomos returned an empty or +unusable response — please retry." The actual error (OpenRouter 503, +content filter, token limit) is logged but not shown. + +**How:** +1. In `chatWith`'s error emit (agent.go:370), include `errText`: + ```go + emit(agentEvent{Type: "error", Data: fmt.Sprintf("Nomos returned an empty or unusable response: %s", errText), SessionID: sessionID}) + ``` +2. In `resumeSession`'s failure path (continue.go:262): + ```go + resumeFailedNote := fmt.Sprintf( + "[System: auto-resume failed after 3 attempts. Last error: %s. "+ + "The task is paused — send another message to continue.]", errText) + ``` +3. The operator can now tell "model overloaded, just retry" from "content + filter — I need to rephrase." + +#### B.5 — Back off between resume retries + +**File:** `cmd/nomos/continue.go:229` + +**How:** Add a small sleep before attempts 1 and 2: +```go +for attempt := 0; attempt < 3; attempt++ { + if attempt > 0 { + select { + case <-cctx.Done(): return + case <-time.After(time.Duration(2<. Now do a PRE-PLAN: gather information with read-only tools ... Do NOT call run yet."` | `"Goal set. NEXT: pre-plan (read-only tools only). Then propose_plan. Do not call run."` | +| `"Plan set: N step(s). Now STOP and present the plan to the operator — do NOT call run yet. Wait for them to approve ..."` | `"Plan set (N steps). STOP. Wait for operator approval. Do not call run."` | +| `"Step N → status"` | `"Step N → status. (Use update_plan_step to advance; do not re-propose.)"` — only on the first call per session, otherwise unchanged. | +| `"⚠️ No entity attributes or relationships were updated in this session. Call update_entity_attributes and create_relationship to persist what you learned about entities before the next session starts from scratch."` | (Replaced by D.1's refusal when discovery ran.) | + +Short, imperative, no hedging. The agent's behavior in `d9cdcee1` shows +that long tool-result strings with "consider revising the last step" are +treated as informational; short imperatives ("STOP. Do not call run.") +are followed. + +#### F.3 — State the approval vocabulary in the plan-result string + +**File:** `cmd/nomos/tasks.go:206-219` (propose_plan result) + +**How:** Add the approval vocabulary to the propose_plan result so the +agent recognizes "proceed", "go", "continue", "yes", "approved", "ok" as +approval and does NOT re-propose on those: +``` +Plan set (N steps). STOP. Wait for operator approval. +Approval vocabulary: "approved", "yes", "go", "proceed", "continue", "ok". +On approval, advance with update_plan_step + run. Do NOT call propose_plan again. +``` +This directly addresses finding #3's cause: the agent re-proposed on +"proceed with the rest" because SOUL.md only listed "approved / yes / go +ahead" as approval vocabulary. Make the list match what operators +actually type. + +--- + +## Sequencing & priority + +| # | Fix | Effort | Impact | Phase | +|---|---|---|---|---| +| A.2 | `proposePlan` refuses re-proposal when in flight | S | **Blocker** — directly removes the duplication the operator saw | A | +| B.1 | Emit `done` after `error` in `chatWith` | S | **Blocker** — breaks the three-bug chain at the amplifier | B | +| B.2 | Reconnect note carries user's last message + plan state | S | **Blocker** — fixes the divergence cause | B | +| A.1 | Set `generation` on INSERT | S | High — needed for any future explicit-revise flow | A | +| A.3 | `propose_plan` tool description states the contract | S | High — agent reads tool schema, often ignores SOUL.md | A | +| F.1 | Consolidate SOUL.md to one task-flow section | S | High — addresses "be more crisp" feedback directly | F | +| F.2 | Tighten tool-result strings to imperatives | S | Medium — observable behavior change | F | +| F.3 | Approval vocabulary in propose_plan result | S | High — fixes the "proceed" → empty-response trigger | F | +| B.3 | Escalate recovery note per resume retry | S | High — turns 3 identical empties into a real recovery | B | +| D.1 | Refuse `complete_task` without writeback | M | **Blocker** — fixes the knowledge loop | D | +| D.2 | Auto-append writeback step to plans | M | High — addresses the cause | D | +| B.4 | Surface real model error text | S | Medium — operator can diagnose | B | +| C.1 | Reject re-completion of terminal sessions | S | Medium — stops duplicate `complete_task` | C | +| C.2 | SOUL.md: don't re-execute on UI complaints | S | Medium — prevents the 6 wasted `run` calls | C | +| B.5 | Back off between resume retries | S | Low-medium | B | +| B.6 | Don't persist empty placeholder as bubble | M | Cosmetic — but visible to operators | B | +| A.4 | Verify PlanProgress grouping renders | S | Depends on A.1 | A | +| E.1 | SOUL.md: prefer knowledge over re-execution | S | Medium — saves credits on fleet audits | E | +| E.2 | `list_lxcs` last-audited hint | M | Low — nice-to-have | E | + +**Suggested order:** A.2 + B.1 + B.2 (the three blockers, ship together) → +F (crispness, ships alongside) → A.1/A.3/A.4 → D → B.3/B.4/B.5/B.6 → C → E. + +The three blockers form a complete fix for the operator's reported bug: +- **A.2** stops the duplication from being *possible* (refuse re-proposal). +- **B.1** stops the empty response from *triggering* a reconnect/resume + (emit `done` after `error`). +- **B.2** makes any *genuine* reconnect carry the right context (advance + the plan, don't re-report). +Together they close the three-bug chain end-to-end. F.3 (approval +vocabulary) closes the *trigger* of the empty response itself. + +--- + +## Verification + +After deploying each phase, replay the same operator prompt in a fresh +session and check: + +- **Phase A:** Call `propose_plan` twice (manually if needed) and confirm + the sidebar shows "Current plan" + a collapsed "Plan v1 (replaced)" + section, not a flat 10-step list. +- **Phase B:** Force an empty response (e.g. temporarily throttle OpenRouter + to 0 RPM, or use a stub model that returns `""`). Confirm: (a) the + frontend shows the error inline and does NOT trigger a reconnect/resume + (no `auto: true` message appears 15 seconds later); (b) the operator sees + the real error text, not "empty or unusable response"; (c) if you then + disconnect the network for real, the reconnect note says "advance the + plan" (not "report state") and the agent calls `update_plan_step` + `run`, + not `propose_plan`. +- **Phase C:** Start a session, let it `complete_task`, then send a follow-up + complaint. Confirm the agent does NOT call `complete_task` again and does + NOT re-run the original `run` calls. +- **Phase D:** Run a fleet-audit prompt. Confirm the agent cannot reach + `complete_task` with `outcome=success` without first calling + `update_entity_attributes` for at least the LXCs it ran `run` against. +- **Phase E:** Confirm a same-day audit prompt produces a turn-1 with ≤5 + tool calls (search_knowledge + get_knowledge_content + small + propose_plan), not 41. +- **Phase F:** Count SOUL.md lines (target: ~80 fewer than current). Replay + the "proceed with the rest" prompt and confirm the agent does NOT call + `propose_plan` again (it gets a refusal error on the call, then advances + via `update_plan_step` + `run`). diff --git a/plans/index.md b/plans/index.md index 108745a..3badfe4 100644 --- a/plans/index.md +++ b/plans/index.md @@ -12,11 +12,12 @@ went sideways, open an investigation. | 2026-07-08 | [Oikos gaps, broken things, and improvements](2026-07-08-oikos-gaps-and-improvements.md) | In Progress — security items (B1-B5) and doc drift (E) still open | | 2026-07-08 | [Control room web UI](2026-07-08-control-room-webui.md) | In Progress — packaging/auth sections superseded by the Wails plan's Phase 0 (client/server split); M4 still open | | 2026-07-08 | [Liveness, drift, and UX cohesion](2026-07-08-liveness-drift-and-ux-cohesion.md) | In Progress — Phase 5 deferred | -| 2026-07-10 | [General gated execution: unlimited actions, gated by risk](2026-07-10-general-gated-execution.md) | In Progress — enum retirement + auto-act revival still open | +| 2026-07-10 | [General gated execution: unlimited actions, gated by risk](2026-07-10-general-gated-execution.md) | In Progress — `request_execution` enum retired (60effcb); only auto-act revival (item 10) still open | | 2026-07-11 | [Nomos agent code review: gaps and improvement plan](2026-07-11-nomos-agent-code-review.md) | In Progress — only C1 (unauthenticated nomos gateway) still open, deferred | | 2026-07-14 | [Session reliability & UX audit](2026-07-14-session-reliability-and-ux-audit.md) | Done — all 21 fixes deployed | | 2026-07-14 | [Tool timeline in sidebar](2026-07-14-tool-timeline-sidebar.md) | Done — deployed v0.3.2 | | 2026-07-14 | [Unified agent activity indicator](2026-07-14-unified-agent-indicator.md) | Done — deployed v0.3.3 | +| 2026-07-14 | [Post-fix session remainders: empty responses & plan drift](2026-07-14-post-fix-session-remainders.md) | In Progress — Phases A + B.1-B.3 + F.3 shipped & e2e-validated (v0.5.0); Phases C, D, E, F.1-F.2 remain (D.1 is the next blocker) | ## Done