From 3d99282897d48016c2d5c8919b4b640f82da6286 Mon Sep 17 00:00:00 2001 From: dtoro Date: Wed, 15 Jul 2026 12:32:02 +0200 Subject: [PATCH] fix(agent): move plan step replacement from reopenSession to setGoal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reopenSession was replacing plan steps on every follow-up message — including approvals ('go ahead') — which destroyed the plan the operator just approved, leaving the agent unable to track step progress and looping run calls until maxIterations. Fix: setGoal is the explicit signal for 'new sub-task' (the agent calls it at the start of each follow-up direction). Step replacement now happens there, not in reopenSession. An approval ('go ahead') does NOT call set_goal, so the plan stays intact and the agent can execute + complete it. --- cmd/nomos/store.go | 62 ++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index e10bcd5..5636f7e 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -452,10 +452,24 @@ func (s *store) taskEntityPtr(ctx context.Context, sessionID string) *uuid.UUID // intermediate state was removed (2026-07-14) — it was indistinguishable from // `active` to the operator and caused sessions to appear stuck when the agent // called set_goal but never propose_plan (observed in production). +// +// P2 (2026-07-15): setGoal also replaces any prior plan steps (from a +// previous sub-task or an incomplete first turn) as `replaced`, clearing the +// way for a fresh propose_plan. This is the ONLY place step replacement +// happens — not in reopenSession — because set_goal is the explicit signal +// for "new sub-task." An approval ("go ahead") does NOT call set_goal, so it +// won't destroy the plan the operator just approved. func (s *store) setGoal(ctx context.Context, sessionID, goal string) error { if s == nil || sessionID == "" || sessionID == "ephemeral" { return nil } + // Replace any prior plan steps (done/running/pending/...) as `replaced`. + // The rows are kept for the generation counter + audit trail; proposePlan + // excludes `replaced` from its in-flight check, so the next propose_plan + // takes the fresh-generation path. + s.pool.Exec(ctx, + `UPDATE session_plan_steps SET status = 'replaced', finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status <> 'replaced'`, + sessionID) if _, err := s.pool.Exec(ctx, `UPDATE agent_sessions SET goal = $2, status = 'executing', last_active_at = now() WHERE id = $1`, sessionID, goal); err != nil { @@ -469,25 +483,17 @@ func (s *store) setGoal(ctx context.Context, sessionID, goal string) error { // reopenSession flips a terminal (done/failed) session back to `executing` // so a follow-up message can start a new sub-task — the iteration path // (P2, 2026-07-15). Without this, a completed session stays `done` forever -// and propose_plan refuses the new sub-task with errPlanInFlight because the -// prior plan's steps are all `done` (status <> 'pending'). reopenSession: +// and the panel shows a stale result. // -// 1. Marks all existing session_plan_steps as `replaced` (a status already -// recognized by updatePlanStep's stamp switch). The rows are KEPT — the -// generation column preserves which plan they belonged to, and the -// audit trail survives. proposePlan's anyStarted check excludes -// `replaced` (see proposePlan), so the next propose_plan takes the -// fresh-generation path rather than being refused with errPlanInFlight. -// This runs for BOTH terminal and executing sessions — a follow-up on -// an executing session (first turn didn't complete_task) is still a -// new direction, and the old plan's steps must not block the new one. -// 2. If the session was terminal, clears outcome/summary and flips status -// to `executing` so the panel doesn't show the old result. -// 3. Stamps last_active_at. +// reopenSession ONLY flips the status + clears outcome/summary. It does NOT +// touch plan steps — that's `setGoal`'s job (see below). The reason: not +// every follow-up is a new sub-task. An approval ("go ahead") is a +// continuation of the current plan, and replacing its steps would destroy +// the plan the operator just approved. `set_goal` is the explicit signal for +// "new sub-task," so step replacement happens there, not here. // // Returns true if the session was actually reopened (was terminal), false if -// it was already active (the plan steps are still replaced — the new -// direction needs a fresh plan — but the status doesn't change). +// it was already active (no-op). func (s *store) reopenSession(ctx context.Context, sessionID string) bool { if s == nil || sessionID == "" || sessionID == "ephemeral" { return false @@ -497,23 +503,15 @@ func (s *store) reopenSession(ctx context.Context, sessionID string) bool { `SELECT status FROM agent_sessions WHERE id = $1`, sessionID).Scan(¤tStatus); err != nil { return false } - // Always replace prior plan steps — the follow-up is a new direction - // regardless of whether the prior turn completed. Without this, an - // executing session with done steps would block proposePlan with - // errPlanInFlight. - s.pool.Exec(ctx, - `UPDATE session_plan_steps SET status = 'replaced', finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status <> 'replaced'`, - sessionID) - if currentStatus == "done" || currentStatus == "failed" { - s.pool.Exec(ctx, - `UPDATE agent_sessions SET status = 'executing', outcome = NULL, summary = NULL, last_active_at = now() WHERE id = $1`, - sessionID) - _ = observability.Event(ctx, sqlcgen.New(s.pool), "task.reopened", s.taskEntityPtr(ctx, sessionID), - "info", "nomos", sessionID, map[string]any{"prior_status": currentStatus}) - return true + if currentStatus != "done" && currentStatus != "failed" { + return false } - s.pool.Exec(ctx, `UPDATE agent_sessions SET last_active_at = now() WHERE id = $1`, sessionID) - return false + s.pool.Exec(ctx, + `UPDATE agent_sessions SET status = 'executing', outcome = NULL, summary = NULL, last_active_at = now() WHERE id = $1`, + sessionID) + _ = observability.Event(ctx, sqlcgen.New(s.pool), "task.reopened", s.taskEntityPtr(ctx, sessionID), + "info", "nomos", sessionID, map[string]any{"prior_status": currentStatus}) + return true } // planStepInput is one step as the agent proposes it.