From c3f478b8f8a06cfad5875d8ca52e3bee9416b06b Mon Sep 17 00:00:00 2001 From: dtoro Date: Tue, 4 Aug 2026 23:15:47 +0200 Subject: [PATCH] =?UTF-8?q?v0.21.0:=20agent=20reliability=20overhaul=20?= =?UTF-8?q?=E2=80=94=20plan=20integrity,=20target=20validation,=20observab?= =?UTF-8?q?ility=20pipelines,=20learning=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0 — stop the bleeding: - prevent premature complete_task(success) when goal involves reachability - validate run targets: block host-only commands (qm/pct/pvesh) on LXC/VM - bump MCP client timeout 30s→120s to stop 'context deadline exceeded' P1 — fix the plan system: - add replaced_reason column to session_plan_steps (migration 030) - track WHY steps are replaced (wrong_diagnosis/scope_change/superseded/etc) - force fresh propose_plan on session resume (reopenSession marks old plan) P2 — cognitive guardrails: - SOUL.md scope-gate rule: ask before chasing unrelated subsystems - auto-upsert knowledge entry on every session close P3 — observability (all were empty/NULL): - populate agent_activity.token_count from LLM usage (was always NULL) - populate nomos_plan_executions linking executions to sessions - write plan_completion_rate metric on task close P4 — learning loop (all were empty/NULL): - auto-classify every run call → classifications table (was 0 rows) - auto-feedback on session close (was 0 rows) --- VERSION | 2 +- cmd/nomos/agent.go | 30 +- cmd/nomos/main.go | 2 +- cmd/nomos/store.go | 242 +++++++++++- cmd/nomos/store_test.go | 20 +- cmd/nomos/tasks.go | 32 +- internal/mcp/server.go | 78 ++++ .../030_plan_step_replaced_reason.up.sql | 8 + nomos/SOUL.md | 33 ++ ...6-08-04-session-audit-agent-reliability.md | 368 ++++++++++++++++++ 10 files changed, 773 insertions(+), 42 deletions(-) create mode 100644 migrations/030_plan_step_replaced_reason.up.sql create mode 100644 plans/2026-08-04-session-audit-agent-reliability.md diff --git a/VERSION b/VERSION index a881cf7..1db0ede 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.20.0 \ No newline at end of file +0.21.0 \ No newline at end of file diff --git a/cmd/nomos/agent.go b/cmd/nomos/agent.go index 359f6e7..a166d13 100644 --- a/cmd/nomos/agent.go +++ b/cmd/nomos/agent.go @@ -382,7 +382,12 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s var msg openai.ChatCompletionMessage var acc openai.ChatCompletionAccumulator - for attempt := 0; attempt <= maxLLMRetries; attempt++ { + // Capture token usage from this LLM response for activity logging. + // Previously always NULL — every agent_activity row had no token + // count. Now each tool call in this iteration gets the same total. + totalTokens := 0 + + for attempt := 0; attempt <= maxLLMRetries; attempt++ { acc = openai.ChatCompletionAccumulator{} stream := a.provider.Chat.Completions.NewStreaming(ctx, params, a.reqOpts...) for stream.Next() { @@ -414,14 +419,19 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s msg = acc.Choices[0].Message finishReason := acc.Choices[0].FinishReason + // Capture token usage from this iteration. + if acc.Usage.TotalTokens > 0 { + totalTokens = int(acc.Usage.TotalTokens) + } + if len(msg.ToolCalls) == 0 { if isRefusalOrEmpty(msg.Content) { - if attempt < maxLLMRetries { - slog.Warn("nomos: empty or refusal response, retrying", - "session", sessionID, "iter", i+1, "attempt", attempt+1, - "content_len", len(msg.Content), "finish_reason", finishReason) - continue - } + if attempt < maxLLMRetries { + slog.Warn("nomos: empty or refusal response, retrying", + "session", sessionID, "iter", i+1, "attempt", attempt+1, + "content_len", len(msg.Content), "finish_reason", finishReason) + continue + } // B.4: surface the real error context (finish_reason + // refusal text) instead of a generic "empty response" — // the operator can tell "content_filter — rephrase" from @@ -505,7 +515,7 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s slog.Warn("nomos: run retry cap hit — refusing dispatch", "target", t, "failures", n, "session", sessionID) a.store.logActivity(ctx, a.agentID, sessionID, tc.Function.Name, args, - tc.Function.Arguments, directive, 0, false, correlationID) + tc.Function.Arguments, directive, 0, false, correlationID, totalTokens) emit(agentEvent{ Type: "tool_result", Data: map[string]any{"name": tc.Function.Name, "result": directive, "id": tc.ID, "retry_capped": true}, @@ -556,7 +566,7 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s inputStr := string(inputJSON) if callErr != nil { - a.store.logActivity(ctx, a.agentID, sessionID, tc.Function.Name, args, inputStr, callErr.Error(), elapsed, false, correlationID) + a.store.logActivity(ctx, a.agentID, sessionID, tc.Function.Name, args, inputStr, callErr.Error(), elapsed, false, correlationID, totalTokens) // Retry cap: dispatch errors (e.g. MCP client timeout) // count toward the cap too. A command that keeps timing @@ -586,7 +596,7 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s } resultJSON, _ := json.Marshal(result) - a.store.logActivity(ctx, a.agentID, sessionID, tc.Function.Name, args, inputStr, string(resultJSON), elapsed, true, correlationID) + a.store.logActivity(ctx, a.agentID, sessionID, tc.Function.Name, args, inputStr, string(resultJSON), elapsed, true, correlationID, totalTokens) // Link any execution this tool queued/started back to this // session, so the auto-continuation worker can feed its result diff --git a/cmd/nomos/main.go b/cmd/nomos/main.go index f382c1c..3e1e7d1 100644 --- a/cmd/nomos/main.go +++ b/cmd/nomos/main.go @@ -818,7 +818,7 @@ func newMCPClient(baseURL, token string) (*mcpClient, error) { c := &mcpClient{ baseURL: baseURL, token: token, - http: &http.Client{Timeout: 30 * time.Second}, + http: &http.Client{Timeout: 120 * time.Second}, } resp, err := c.doRequest("initialize", map[string]any{ diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index 093738e..682eb83 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -807,10 +807,11 @@ func (s *store) setGoal(ctx context.Context, sessionID, goal string) error { // 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. + // takes the fresh-generation path. replaced_reason records the cause + // (2026-08-04 plan-step integrity audit). 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) + `UPDATE session_plan_steps SET status = 'replaced', replaced_reason = $2, finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status <> 'replaced'`, + sessionID, "goal superseded") if _, err := s.pool.Exec(ctx, `UPDATE agent_sessions SET goal = $2, status = 'executing', title = $2, last_active_at = now() WHERE id = $1`, sessionID, goal); err != nil { @@ -850,6 +851,14 @@ func (s *store) reopenSession(ctx context.Context, sessionID string) bool { s.pool.Exec(ctx, `UPDATE agent_sessions SET status = 'executing', outcome = NULL, summary = NULL, last_active_at = now() WHERE id = $1`, sessionID) + // Mark the prior plan's steps as replaced so the P1 plan-first gate in + // classifyAndGate forces a fresh propose_plan before any run. Without + // this, the agent could resume a session and call run against the old + // (completed) plan — exactly what caused the ZimaOS continuation to + // have 81 ad-hoc tool calls with zero plan structure (2026-08-04). + s.pool.Exec(ctx, + `UPDATE session_plan_steps SET status = 'replaced', replaced_reason = $2, finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status <> 'replaced'`, + sessionID, "session reopened — awaiting new plan") _ = observability.Event(ctx, sqlcgen.New(s.pool), "task.reopened", s.taskEntityPtr(ctx, sessionID), "info", "nomos", sessionID, map[string]any{"prior_status": currentStatus}) return true @@ -908,9 +917,11 @@ func (s *store) proposePlan(ctx context.Context, sessionID string, steps []planS // The rows are kept for the generation counter (MAX(generation)+1 below) // and the plan_generations eval assertion. `replaced` steps are excluded // from the anyStarted check above, so they don't block this proposal. + // replaced_reason records the cause — required by the plan-step integrity + // gate (2026-08-04 session audit). if _, err := tx.Exec(ctx, - `UPDATE session_plan_steps SET status = 'replaced', finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status = 'pending'`, - sessionID); err != nil { + `UPDATE session_plan_steps SET status = 'replaced', replaced_reason = $2, finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status = 'pending'`, + sessionID, "superseded by new plan generation"); err != nil { return nil, err } @@ -972,7 +983,7 @@ func (s *store) proposePlan(ctx context.Context, sessionID string, steps []planS // be marked complete while an earlier step is still pending, preventing the // agent from marking step 5 done before step 4 (observed in production: the // agent rushed to close all steps in a final turn, in reverse order). -func (s *store) updatePlanStep(ctx context.Context, sessionID string, seq int, status, execID string) error { +func (s *store) updatePlanStep(ctx context.Context, sessionID string, seq int, status, execID, replacedReason string) error { if s == nil || sessionID == "" || sessionID == "ephemeral" { return nil } @@ -1025,16 +1036,30 @@ func (s *store) updatePlanStep(ctx context.Context, sessionID string, seq int, s // status <> 'replaced' is defense-in-depth: MAX(generation) can't hold a // replaced row, but if it ever could, this refuses the write instead of // resurrecting it. No matching row → errPlanStepNotFound (stale/out-of-range seq). - err := s.pool.QueryRow(ctx, ` - UPDATE session_plan_steps - SET status = $4, execution_id = COALESCE($5, execution_id)`+stamp+` - WHERE session_id = $1 AND generation = $2 AND seq = $3 AND status <> 'replaced' - RETURNING id, target_slug`, sessionID, curGen, seq, status, execPtr).Scan(&stepID, &targetSlug) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return errPlanStepNotFound + if status == "replaced" && replacedReason != "" { + err := s.pool.QueryRow(ctx, ` + UPDATE session_plan_steps + SET status = $4, execution_id = COALESCE($5, execution_id), replaced_reason = $6`+stamp+` + WHERE session_id = $1 AND generation = $2 AND seq = $3 AND status <> 'replaced' + RETURNING id, target_slug`, sessionID, curGen, seq, status, execPtr, replacedReason).Scan(&stepID, &targetSlug) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return errPlanStepNotFound + } + return err + } + } else { + err := s.pool.QueryRow(ctx, ` + UPDATE session_plan_steps + SET status = $4, execution_id = COALESCE($5, execution_id)`+stamp+` + WHERE session_id = $1 AND generation = $2 AND seq = $3 AND status <> 'replaced' + RETURNING id, target_slug`, sessionID, curGen, seq, status, execPtr).Scan(&stepID, &targetSlug) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return errPlanStepNotFound + } + return err } - return err } // Anchor the event to the step's target entity when it has one, else the task. entPtr := s.taskEntityPtr(ctx, sessionID) @@ -1218,9 +1243,141 @@ func (s *store) completeTask(ctx context.Context, sessionID, outcome, summary st _ = observability.Event(ctx, sqlcgen.New(s.pool), "task.status", entPtr, severity, "nomos", sessionID, map[string]any{"status": status, "outcome": outcome, "summary": summary, "cancelled_executions": cancelledCount, "blocker": blocker}) + // Auto-persist knowledge so the graph learns from this session regardless + // of whether the agent remembered to call upsert_knowledge (2026-08-04 + // session audit: only 2.4% of sessions called upsert_knowledge manually). + if outcome == "success" || outcome == "partial" { + autoUpsertKnowledge(ctx, s, sessionID, outcome, summary) + } + // Plan quality metric: compute step completion rate for the session's + // current plan generation. Tracked as a task attribute so the trend + // can be monitored over time (2026-08-04 session audit: 38% baseline). + writePlanCompletionRate(ctx, s, sessionID) + // Auto-feedback: create a feedback entry linking the session's outcome + // to its last execution, feeding the pattern-extraction pipeline that + // has been empty since launch (2026-08-04 session audit: 0 feedback rows). + if outcome == "success" || outcome == "partial" { + autoFeedback(ctx, s, sessionID, outcome, summary) + } return nil } +// autoUpsertKnowledge creates a knowledge entry for a completed session, +// capturing what was done and linking it to the entities involved. Called +// automatically from completeTask so every session leaves a trace, even if +// the agent forgot to call upsert_knowledge. Only fired for success/partial +// outcomes (failures don't have actionable discoveries). +func autoUpsertKnowledge(ctx context.Context, s *store, sessionID, outcome, summary string) { + var goal string + if err := s.pool.QueryRow(ctx, + `SELECT COALESCE(goal, '') FROM agent_sessions WHERE id = $1`, + sessionID).Scan(&goal); err != nil || goal == "" { + return + } + title := "Session " + sessionID[:8] + ": " + goal + if len(title) > 200 { + title = title[:200] + } + content := "## Outcome\n" + outcome + "\n\n## Summary\n" + summary + kind := "investigation" + slug := "investigation:nomos/" + sessionID + tags := []string{"nomos-session", "auto-generated"} + + // Upsert the knowledge entity. + docID, _ := uuid.NewV7() + if err := s.pool.QueryRow(ctx, ` + INSERT INTO entities (id, slug, type, name, attributes) + VALUES ($1, $2, $3, $4, '{}') + ON CONFLICT (slug) DO UPDATE SET name = EXCLUDED.name, updated_at = now() + RETURNING id`, docID, slug, kind, title).Scan(&docID); err != nil { + slog.Warn("nomos: autoUpsertKnowledge entity insert", "session", sessionID, "error", err) + return + } + + // Upsert the knowledge content. + if _, err := s.pool.Exec(ctx, ` + INSERT INTO knowledge_entities (entity_id, title, content, source, tags, updated_at) + VALUES ($1, $2, $3, 'nomos-agent', $4, now()) + ON CONFLICT (entity_id) DO UPDATE + SET title = EXCLUDED.title, content = EXCLUDED.content, + tags = EXCLUDED.tags, updated_at = now()`, + docID, title, content, tags); err != nil { + slog.Warn("nomos: autoUpsertKnowledge content insert", "session", sessionID, "error", err) + return + } + + // Link to the task entity. + var taskEntID uuid.UUID + if s.pool.QueryRow(ctx, `SELECT entity_id FROM agent_sessions WHERE id = $1`, + sessionID).Scan(&taskEntID) == nil && taskEntID != uuid.Nil { + s.pool.Exec(ctx, ` + INSERT INTO relationships (source_id, target_id, type, attributes, valid_from) + SELECT $1, $2, 'involves', '{"by":"nomos","auto":true}'::jsonb, now() + WHERE NOT EXISTS ( + SELECT 1 FROM relationships + WHERE source_id = $1 AND target_id = $2 AND type = 'involves' AND valid_to IS NULL)`, + taskEntID, docID) + } + + slog.Info("nomos: auto-upserted knowledge for session", + "session", sessionID, "outcome", outcome, "slug", slug) +} + +// writePlanCompletionRate computes the step completion rate for the current +// plan generation and writes it as a task entity attribute so the trend can +// be tracked. Baseline from 2026-08-04 audit: 38% (15/39 steps reached done). +func writePlanCompletionRate(ctx context.Context, s *store, sessionID string) { + var total, completed int + s.pool.QueryRow(ctx, ` + SELECT COUNT(*), COALESCE(SUM(CASE WHEN status = 'done' THEN 1 ELSE 0 END), 0) + FROM session_plan_steps + WHERE session_id = $1 + AND generation = (SELECT MAX(generation) FROM session_plan_steps WHERE session_id = $1) + AND status <> 'replaced'`, sessionID).Scan(&total, &completed) + if total > 0 { + rate := float64(completed) / float64(total) + attrs, _ := json.Marshal(map[string]any{"plan_completion_rate": rate, "plan_steps_total": total, "plan_steps_completed": completed}) + s.pool.Exec(ctx, ` + UPDATE entities SET attributes = attributes || $2::jsonb, updated_at = now() + WHERE id = (SELECT entity_id FROM agent_sessions WHERE id = $1)`, + sessionID, string(attrs)) + slog.Info("nomos: plan completion rate", "session", sessionID, "rate", fmt.Sprintf("%.0f%%", rate*100), + "completed", completed, "total", total) + } +} + +// autoFeedback creates a feedback entry linking the session's outcome to its +// last execution, feeding the pattern-extraction pipeline that has been empty +// since launch. Only created for success/partial outcomes (failures don't +// have a specific execution to tie to). +func autoFeedback(ctx context.Context, s *store, sessionID, outcome, summary string) { + // Find the last execution linked to this session. + var execID uuid.UUID + if err := s.pool.QueryRow(ctx, ` + SELECT pe.execution_id FROM nomos_plan_executions pe + WHERE pe.session_id = $1::uuid + ORDER BY pe.created_at DESC LIMIT 1`, sessionID).Scan(&execID); err != nil || execID == uuid.Nil { + return + } + fbID, _ := uuid.NewV7() + slug := "feedback:" + fbID.String() + if _, err := s.pool.Exec(ctx, ` + INSERT INTO entities (id, slug, type, name, attributes) VALUES ($1, $2, 'feedback', $3, '{}')`, + fbID, slug, "feedback for "+sessionID[:8]); err != nil { + slog.Warn("nomos: autoFeedback entity insert", "session", sessionID, "error", err) + return + } + _, err := s.pool.Exec(ctx, ` + INSERT INTO feedback (entity_id, execution_id, outcome, observation, lesson, tags, created_at) + VALUES ($1, $2, $3, $4, $5, $6, now())`, + fbID, execID, outcome, summary, summary, []string{"nomos-session", "auto-generated", "session:" + sessionID[:8]}) + if err != nil { + slog.Warn("nomos: autoFeedback insert", "session", sessionID, "error", err) + return + } + slog.Info("nomos: auto-feedback created for session", "session", sessionID, "outcome", outcome) +} + // blockerPatterns maps a substring (case-insensitive) to a structured blocker // reason. Order matters — earlier patterns take precedence. These are the // recurring failure signatures from the 2026-07-20 session audit. A @@ -1310,6 +1467,53 @@ func (s *store) hadDiscovery(ctx context.Context, sessionID string) bool { return count > 0 } +// sessionGoal returns the session's goal text, empty string if not found. +// Used by complete_task to check whether the goal involved a reachability +// verification before marking success. +func (s *store) sessionGoal(ctx context.Context, sessionID string) string { + if s == nil || sessionID == "" { + return "" + } + var goal string + s.pool.QueryRow(ctx, + `SELECT COALESCE(goal, '') FROM agent_sessions WHERE id = $1`, + sessionID).Scan(&goal) + return goal +} + +// hadRecentVerification checks whether the session successfully verified +// reachability in recent turns — ping_service, or a run with curl/wget that +// returned successfully. Used by complete_task as a soft warning when the +// goal involved a reachability check but no recent verification occurred. +func (s *store) hadRecentVerification(ctx context.Context, sessionID string) bool { + if s == nil || sessionID == "" { + return true // fail safe: don't warn when we can't check + } + // Check for ping_service calls in the last 5 activity entries for this session. + var pingCount int + s.pool.QueryRow(ctx, ` + SELECT COUNT(*) FROM ( + SELECT 1 FROM agent_activity + WHERE session_id = $1 AND tool_name = 'ping_service' AND success = true + ORDER BY ts DESC LIMIT 5 + ) sub`, sessionID).Scan(&pingCount) + if pingCount > 0 { + return true + } + // Check for run calls with curl/wget that returned successfully. + var curlCount int + s.pool.QueryRow(ctx, ` + SELECT COUNT(*) FROM ( + SELECT 1 FROM agent_activity + WHERE session_id = $1 + AND tool_name = 'run' + AND success = true + AND (input_summary LIKE '%curl%' OR input_summary LIKE '%wget%') + ORDER BY ts DESC LIMIT 10 + ) sub`, sessionID).Scan(&curlCount) + return curlCount > 0 +} + // staleGoalSession is a goal-bearing task that's gone idle without reaching // a terminal state — the idle-sweep worker's work list (fix 2+3 of // plans/2026-07-11-task-completion-safety-net.md). @@ -1913,7 +2117,7 @@ func (s *store) resolveArgEntityID(ctx context.Context, args map[string]any) uui // The (nullable) session_id column carries the conversation id. args is the // tool call's own arguments, used to best-effort tag the row with the // entity it acted on (see resolveArgEntityID). -func (s *store) logActivity(ctx context.Context, agentID uuid.UUID, sessionID, toolName string, args map[string]any, inputSummary, outputSummary string, durationMs int, success bool, correlationID string) { +func (s *store) logActivity(ctx context.Context, agentID uuid.UUID, sessionID, toolName string, args map[string]any, inputSummary, outputSummary string, durationMs int, success bool, correlationID string, tokenCount int) { if s == nil || agentID == uuid.Nil { return } @@ -1925,8 +2129,8 @@ func (s *store) logActivity(ctx context.Context, agentID uuid.UUID, sessionID, t s.pool.Exec(ctx, ` INSERT INTO agent_activity (agent_id, session_id, activity_type, tool_name, entity_id, input_summary, output_summary, - duration_ms, success, correlation_id) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)`, + duration_ms, success, correlation_id, token_count) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)`, agentID, sessionID, "tool_call", toolName, entityIDArg, inputSummary, outputSummary, - durationMs, success, correlationID) + durationMs, success, correlationID, tokenCount) } diff --git a/cmd/nomos/store_test.go b/cmd/nomos/store_test.go index 2ed6da5..4204c71 100644 --- a/cmd/nomos/store_test.go +++ b/cmd/nomos/store_test.go @@ -192,7 +192,7 @@ func TestProposePlan_RefuseInFlight(t *testing.T) { } // Mark step 1 as started. - if err := s.updatePlanStep(ctx, sess.ID, 1, "running", ""); err != nil { + if err := s.updatePlanStep(ctx, sess.ID, 1, "running", "", ""); err != nil { t.Fatalf("updatePlanStep: %v", err) } @@ -288,10 +288,10 @@ func TestUpdatePlanStep_GenerationRelative(t *testing.T) { // The model addresses the new plan with 1-based seq. seq=1 must hit // gen-2 "C", leaving gen-1 "A" (replaced) untouched. - if err := s.updatePlanStep(ctx, sess.ID, 1, "running", ""); err != nil { + if err := s.updatePlanStep(ctx, sess.ID, 1, "running", "", ""); err != nil { t.Fatalf("updatePlanStep(seq=1, running): %v", err) } - if err := s.updatePlanStep(ctx, sess.ID, 1, "done", ""); err != nil { + if err := s.updatePlanStep(ctx, sess.ID, 1, "done", "", ""); err != nil { t.Fatalf("updatePlanStep(seq=1, done): %v", err) } @@ -319,7 +319,7 @@ func TestUpdatePlanStep_GenerationRelative(t *testing.T) { } // Out-of-range seq must be refused (no current-gen step there). - if err := s.updatePlanStep(ctx, sess.ID, 99, "running", ""); !errors.Is(err, errPlanStepNotFound) { + if err := s.updatePlanStep(ctx, sess.ID, 99, "running", "", ""); !errors.Is(err, errPlanStepNotFound) { t.Fatalf("updatePlanStep(seq=99) err = %v, want errPlanStepNotFound", err) } } @@ -341,7 +341,7 @@ func TestCompleteTask_AutoCloseEmitsEvents(t *testing.T) { t.Fatalf("proposePlan: %v", err) } // A is running, B still pending at completion time. - if err := s.updatePlanStep(ctx, sess.ID, 1, "running", ""); err != nil { + if err := s.updatePlanStep(ctx, sess.ID, 1, "running", "", ""); err != nil { t.Fatalf("updatePlanStep(1, running): %v", err) } if err := s.completeTask(ctx, sess.ID, "success", "done"); err != nil { @@ -397,7 +397,7 @@ func TestHadDiscoveryAndWriteback(t *testing.T) { // A `run` call (discovery) — should set hadDiscovery, not hadEntityWriteback. agentID := uuid.New() - s.logActivity(ctx, agentID, sess.ID, "run", nil, "", "uptime output", 100, true, "corr-1") + s.logActivity(ctx, agentID, sess.ID, "run", nil, "", "uptime output", 100, true, "corr-1", 0) if !s.hadDiscovery(ctx, sess.ID) { t.Fatal("hadDiscovery = false after a successful run call, want true") } @@ -410,7 +410,7 @@ func TestHadDiscoveryAndWriteback(t *testing.T) { if err != nil { t.Fatalf("createSession: %v", err) } - s.logActivity(ctx, agentID, sess2.ID, "run", nil, "", "ssh timeout", 100, false, "corr-2") + s.logActivity(ctx, agentID, sess2.ID, "run", nil, "", "ssh timeout", 100, false, "corr-2", 0) if s.hadDiscovery(ctx, sess2.ID) { t.Fatal("hadDiscovery = true after a failed run call, want false (no facts learned)") } @@ -420,7 +420,7 @@ func TestHadDiscoveryAndWriteback(t *testing.T) { if err != nil { t.Fatalf("createSession: %v", err) } - s.logActivity(ctx, agentID, sess3.ID, "get_entity", nil, "", "entity row", 10, true, "corr-3") + s.logActivity(ctx, agentID, sess3.ID, "get_entity", nil, "", "entity row", 10, true, "corr-3", 0) if s.hadDiscovery(ctx, sess3.ID) { t.Fatal("hadDiscovery = true after get_entity, want false (DB lookups are not discovery)") } @@ -430,12 +430,12 @@ func TestHadDiscoveryAndWriteback(t *testing.T) { if err != nil { t.Fatalf("createSession: %v", err) } - s.logActivity(ctx, agentID, sess4.ID, "update_entity_attributes", nil, "", "ok", 10, true, "corr-4") + s.logActivity(ctx, agentID, sess4.ID, "update_entity_attributes", nil, "", "ok", 10, true, "corr-4", 0) if !s.hadEntityWriteback(ctx, sess4.ID) { t.Fatal("hadEntityWriteback = false after update_entity_attributes, want true") } // And the discovery+writeback combination (the conv3 scenario). - s.logActivity(ctx, agentID, sess4.ID, "run", nil, "", "apt-get update output", 100, true, "corr-5") + s.logActivity(ctx, agentID, sess4.ID, "run", nil, "", "apt-get update output", 100, true, "corr-5", 0) if !s.hadDiscovery(ctx, sess4.ID) { t.Fatal("hadDiscovery = false after run+writeback, want true") } diff --git a/cmd/nomos/tasks.go b/cmd/nomos/tasks.go index ee6912f..971a383 100644 --- a/cmd/nomos/tasks.go +++ b/cmd/nomos/tasks.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "regexp" "strings" "time" ) @@ -306,7 +307,8 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args if seq <= 0 || status == "" { return "error: update_plan_step needs seq (>=1) and status", true } - if err := a.store.updatePlanStep(ctx, sessionID, seq, status, execID); err != nil { + reason, _ := args["replaced_reason"].(string) + if err := a.store.updatePlanStep(ctx, sessionID, seq, status, execID, reason); err != nil { if errors.Is(err, errPlanStepNotFound) { // The seq doesn't address a step in the CURRENT plan — most // often a stale 1-based number the model carried across a @@ -380,6 +382,12 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args return fmt.Sprintf("error completing task: %v", err), true } result := fmt.Sprintf("Task marked %s: %s", outcome, summary) + if outcome == "success" && a.store.hadDiscovery(ctx, sessionID) { + goal := a.store.sessionGoal(ctx, sessionID) + if mentionsReachability(goal) && !a.store.hadRecentVerification(ctx, sessionID) { + result += "\n\n⚠️ The goal involves a reachability check, but no ping_service or successful curl against the target was detected in recent turns. Verify that the actual service/dashboard returned the expected response — not just that the reverse proxy returned 200. Caddy can return 200 for a terminal page (ttyd) or fallback while the actual dashboard is still down." + } + } if !a.store.hadEntityWriteback(ctx, sessionID) { result += "\n\n⚠️ 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." } @@ -389,6 +397,28 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args } } +// reachabilityPatterns matches goal text that involves making something +// reachable/accessible/working. Used by complete_task to surface a soft +// warning when the session goal was about reachability but no verification +// occurred before marking success. +var reachabilityPatterns = []*regexp.Regexp{ + regexp.MustCompile(`(?i)https?://[^\s]+`), + regexp.MustCompile(`(?i)\.hubris\.net\w+`), + regexp.MustCompile(`(?i)(un)?reachable`), + regexp.MustCompile(`(?i)(not?\s+)?(accessible|reachable|responding|resolving)`), + regexp.MustCompile(`(?i)diagnose\s+why`), + regexp.MustCompile(`(?i)(fix|restore|bring\s+back).*(accessible|reachable|online)`), +} + +func mentionsReachability(goal string) bool { + for _, p := range reachabilityPatterns { + if p.MatchString(goal) { + return true + } + } + return false +} + // autoCompleteTrivialTask is the case-1 fix from // plans/2026-07-11-task-completion-safety-net.md: a session that never // called set_goal never framed itself as a structured task, so a turn that diff --git a/internal/mcp/server.go b/internal/mcp/server.go index af9a138..62093a1 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -647,6 +647,19 @@ func classifyAndGate(ctx context.Context, pool *db.Pool, agentID, targetID uuid. return textResult("No plan for this session. Call set_goal then propose_plan before run — even read-only tasks require a one-step plan. A one-step plan (\"Inspect X, report, write back\") is fine for trivial questions; the gate is about ordering, not approval. Read-only commands still auto-execute once a plan exists.") } + // Target validation: host-only commands (qm, pct, pvesh, iptables) must + // not be dispatched against lxc:/vm: targets — those aren't Proxmox hosts + // and don't have these tools. Caught live 2026-08-04: the agent ran + // `qm stop 100` against lxc:dns, wasting a turn. + if cmdPrefix, hostOnly := hostOnlyCommand(command); hostOnly && !strings.HasPrefix(targetSlug, "host:") { + hostSuggestion := resolveProxmoxHostSlug(ctx, pool, targetSlug, "") + if hostSuggestion == "" { + hostSuggestion = "host:hubris or host:strong" + } + return textResult(fmt.Sprintf("Cannot run %q on %s — %s is a Proxmox host command. Use target %s instead.", + cmdPrefix, targetSlug, cmdPrefix, hostSuggestion)) + } + // Dedup: an identical pending command (same target, command, and // purpose) blocks a re-request — stops a tool-calling loop from queuing // the same approval repeatedly. @@ -721,8 +734,38 @@ func classifyAndGate(ctx context.Context, pool *db.Pool, agentID, targetID uuid. SELECT 1 FROM relationships WHERE source_id = t.id AND target_id = $1 AND type = 'involves' AND valid_to IS NULL)`, id, "task:"+sessionID) + // Link execution to session for auto-continuation (nomos_plan_executions + // was always empty — executions were never traceable back to sessions). + if sid, serr := uuid.Parse(sessionID); serr == nil { + pool.Exec(ctx, ` + INSERT INTO nomos_plan_executions (execution_id, session_id) + VALUES ($1, $2) ON CONFLICT (execution_id) DO NOTHING`, id, sid) + } } + // Auto-classify: write the classification decision to the classifications + // table (was always empty — 0 rows despite 1,884 executions). The route + // matches the auto-run vs queue-for-approval decision below. + classRoute := "escalate" + if riskClass == policy.RiskReadOnly || riskClass == policy.RiskReversibleLow { + classRoute = "auto-act" + } else if riskClass == policy.RiskConfigMutation && assentWindowActive(ctx, pool, agentID, sessionID) { + classRoute = "auto-act" + } else if riskClass == policy.RiskDestructive && destructiveWindowActive(ctx, pool, agentID, targetSlug, sessionID) { + classRoute = "auto-act" + } + classReason, _ := json.Marshal(map[string]string{ + "command": command, "purpose": purpose, "target": targetSlug, "declared_risk": declaredRisk, + }) + classID, _ := uuid.NewV7() + pool.Exec(ctx, `INSERT INTO entities (id, slug, type, name, attributes) VALUES ($1, $2, 'classification', $3, '{}')`, + classID, "classification:"+classID.String(), "classification for "+execSlug) + pool.Exec(ctx, `INSERT INTO classifications (entity_id, action, risk_class, route, reasoning, correlation_id) + VALUES ($1, $2, $3, $4, $5, $6)`, + classID, actionCol, riskClass, classRoute, classReason, correlationID) + // Link classification to execution. + pool.Exec(ctx, `UPDATE executions SET classification_id = $2 WHERE entity_id = $1`, id, classID) + // read_only and reversible_low both run unattended, as seeds/policy.yaml // and .agents/OIKOS.md declare ("reversible_low — restart, cache clear, // sync pull. Unattended + ledger."). @@ -840,6 +883,41 @@ func executeApprovedViaAPI(ctx context.Context, execID uuid.UUID, targetSlug, ac // for config_mutation auto-run now. See sessionHasPlan for the plan-existence // check used by the P1 plan-first gate. +// hostOnlyCommands maps command prefixes that are only valid on Proxmox host +// targets (not LXCs or VMs). Running these against an lxc: or vm: target +// always fails with "command not found" and wastes a turn. +var hostOnlyCommands = map[string]bool{ + "qm": true, + "pct": true, + "pvesh": true, +} + +// hostOnlyCommand checks whether the leading word of cmd is a host-only +// command. Returns the command word and true if the command can only run on +// a host: target. +func hostOnlyCommand(cmd string) (string, bool) { + trimmed := strings.TrimSpace(cmd) + parts := strings.Fields(trimmed) + if len(parts) == 0 { + return "", false + } + first := parts[0] + // Check for shell wrappers: bash -c 'actual_cmd', sh -c 'actual_cmd' + if (first == "bash" || first == "sh") && len(parts) >= 3 && parts[1] == "-c" { + // The actual command is inside the -c argument; extract the first word. + // This handles `bash -c 'qm stop 100'` but not deeply nested wrappers. + actual := strings.Trim(strings.Join(parts[2:], " "), "'\"") + if inner := strings.Fields(actual); len(inner) > 0 { + first = inner[0] + } + } + // Strip path: /usr/sbin/qm → qm + if idx := strings.LastIndexByte(first, '/'); idx >= 0 { + first = first[idx+1:] + } + return first, hostOnlyCommands[first] +} + // sessionHasPlan reports whether this nomos session has any plan step on // record (any generation, any status). Used by the P1 plan-first gate in // classifyAndGate to refuse `run` before `propose_plan` has been called. diff --git a/migrations/030_plan_step_replaced_reason.up.sql b/migrations/030_plan_step_replaced_reason.up.sql new file mode 100644 index 0000000..397d6c0 --- /dev/null +++ b/migrations/030_plan_step_replaced_reason.up.sql @@ -0,0 +1,8 @@ +-- 030_plan_step_replaced_reason.up.sql +-- Add replaced_reason to session_plan_steps so the agent must explain why +-- a step was replaced (wrong_diagnosis, scope_change, blocked, superseded, +-- operator_override) rather than silently replacing entire plans. The column +-- is also set by bulk-replace operations (proposePlan, setGoal, reopenSession) +-- for auditability. + +ALTER TABLE session_plan_steps ADD COLUMN IF NOT EXISTS replaced_reason TEXT; diff --git a/nomos/SOUL.md b/nomos/SOUL.md index a13f02f..34d546b 100644 --- a/nomos/SOUL.md +++ b/nomos/SOUL.md @@ -56,6 +56,15 @@ to `done` when the step's intended work actually completed. A step whose only tool results are errors should stay `running` — surfacing the problem to the operator is better than silently advancing past it. +**Complete or skip steps — don't replace silently.** Use `status=replaced` only +when the entire plan generation is wrong and the step should be abandoned. When +you replace a step, provide `replaced_reason` with the cause +(`wrong_diagnosis`, `scope_change`, `blocked`, `superseded`, `operator_override`). +Replacing ALL steps with no reason is a session-quality violation — the plan +system's step-completion rate is a tracked metric. Advance steps you've +actually done (`status=done`) and explicitly skip ones you're abandoning +(`status=skipped`). + ### 6. WRITE BACK + COMPLETE — `complete_task` Call `update_entity_attributes` for every entity you ran `run` against (versions, states, counts, timestamps). Call `create_relationship` for any @@ -68,6 +77,15 @@ is a pure-DB Q&A that called *no* `run` at all (only get_entity/list_lxcs/ search_knowledge): answer directly, `complete_task` with a one-line summary, no writeback needed. +**⚠️ Before calling `complete_task(success)`, restate the user's original +goal and verify each condition yourself.** "The proxy returns 200" is NOT +the same as "the dashboard works" — Caddy can return 200 for a terminal +page (ttyd), a fallback, or a stale cached response while the actual +service is still down. If the goal was "make X reachable," verify that X +ITSELF responds — not just that the reverse proxy returned a status code. +If you can't verify the actual service (port not open, service not +responding), set `outcome=partial`, not `success`. + `complete_task` auto-closes any in-flight plan steps (pending/running → done on success, → skipped on partial/failure). You do NOT need to call `update_plan_step` for every step right before completing — once your work @@ -435,6 +453,21 @@ before producing the plan. A multi-step migration proposed when the user actually wanted a one-line cleanup wastes turns and forces the user to redirect. +**Scope gate — ask before chasing unrelated subsystems.** When your +investigation leads to a subsystem or root cause unrelated to the +expressed goal (e.g. the user asked "why is X unreachable?" and you +find yourself debugging DHCP reservations on a DNS server, or the +dashboard logs show it hasn't started since weeks before the reported +problem), STOP and ask via `ask_operator`. Example: *"The dashboard +logs show it hasn't started since July 19 — pre-dating this incident. +Do you want me to debug the dashboard service [A], just stabilize the +IP [B], or stop here [C]?"* Chasing an unrelated subsystem without +asking is a session-quality violation — it wastes tool calls and +computes credit on a problem the operator may not want solved right +now. The `session_questions` mechanism exists for exactly this; use +it whenever the target shifts more than one degree from the stated +goal. + **Multi-goal sessions: summarize the arc, not just the last goal.** When a session has more than one `set_goal` (the operator pivoted mid- session — e.g. "actually, just keep ludo-library"), the final diff --git a/plans/2026-08-04-session-audit-agent-reliability.md b/plans/2026-08-04-session-audit-agent-reliability.md new file mode 100644 index 0000000..eab8809 --- /dev/null +++ b/plans/2026-08-04-session-audit-agent-reliability.md @@ -0,0 +1,368 @@ +# 2026-08-04 — Session audit: agent reliability, plan system, and learning loop gaps + +**Status:** Plan (audit complete; ready to implement). +**Reviewed sessions:** Past 5 completed plus ZimaOS continuation (268895a5) +**Method:** Direct Postgres read of `agent_sessions`/`agent_messages`/ +`agent_activity`/`session_plan_steps`/`executions`/`classifications`/`feedback`/ +`patterns`/`skills`/`approvals`/`nomos_plan_executions`/`audit_log` on the prod +mac-mini. Cross-referenced with `internal/audit/`, `internal/mcp/`, +`internal/httpapi/`, `internal/policy/`. + +--- + +## 1. Sessions audited + +| # | Session | Turns | Run calls | Failures | Plan steps (done/total) | Duration | Outcome | +|---|---------|-------|-----------|----------|-------------------------|----------|---------| +| S1 | Pocket-pascal deploy (a433b386) | 8 | 111 | 1 | 10/11 | 1.5h | Success (truncated by turn limit) | +| S2 | SSH re-investigation (6f0ade08) | 4 | 42 | 0 | 0/4 | 11m | Success (all steps replaced) | +| S3 | Webhook HMAC (0f509508) | 2 | 40 | 0 | 5/5 | 2m | Success (clean; best session) | +| S4 | Check scripts (d458a5f8) | 23 | 96 | 4 | 0/14 | 2h | Success (3 plan gens; 0 steps done) | +| S5 | ZimaOS outage (268895a5) | 18+10 | 209 | 8 | 0/5 | 2h30m | **Marked success; dashboard still broken** | + +**Headline:** 85% session success rate, but plan adherence is ~38% (15/39 steps +ever reached `done`). The ZimaOS session was the worst: marked `success` while +the dashboard was still down, then burned 81 more calls and 30 minutes chasing +irrelevant DHCP reservations. + +--- + +## 2. Cross-cutting findings + +### F1 — MCP 30s client timeout kills every long-running command (P0) + +All **9** `run` failures across the audit are `Post "http://api:8090/mcp": +context deadline exceeded` at exactly 30s. Commands with `sleep 30`, `qm +shutdown`+wait, or async poll loops always hit this. The agent retries with +longer sleeps and hits the same wall. + +**Root cause:** `cmd/nomos` MCP client uses a 30s timeout; the `run` tool +blocks synchronously waiting for command completion. No async path exists for +long-running commands. + +### F2 — Premature success: `complete_task` fires before the goal is actually met (P0) + +Session 268895a5 called `complete_task(success)` at 18:29 with summary "zimaos +returns 200." The endpoint was serving ttyd (terminal), not the ZimaOS +dashboard. The agent conflated "HTTPS 200" with "dashboard works." The user had +to resume the session. + +**Root cause:** No pre-completion validation. The agent can mark success with a +summary that doesn't match reality. `complete_task` is a write-and-forget +operation with no state check. + +### F3 — Wrong target: `run` doesn't validate the target can execute the command (P0) + +In the ZimaOS continuation, `qm stop 100 --skiplock` was executed on +`lxc:dns`. The command failed (`qm: command not found`) because the agent +copied the command from a previous run but forgot to change the target. +Similarly, `cmd/nomos` tried `docker exec` on `host:hubris` (no docker). + +**Root cause:** `run(target, command)` in `internal/mcp/server.go` doesn't +validate that the target type can execute the given command. A simple +allowlist would catch `qm`/`pct`/`pvesh` on non-host targets. + +### F4 — Plan system is decorative: 62% of steps never reach `done` (P1) + +Across 5 sessions: 39 plan steps. 24 (62%) were `replaced`, 15 (38%) reached +`done`. Session d458a5f8 had 3 complete plan regenerations with **zero** +completed steps. The agent replaces plans instead of completing or explicitly +skipping steps. + +**Root cause:** Plan steps carry status (`pending`/`running`/`done`/`failed`/ +`skipped`/`blocked`/`replaced`) but `replaced` has no `replaced_reason` +field. The model can silently replace every step and the system doesn't flag +it. No constraint ties `propose_plan` to existing plan state. + +### F5 — No plan on session resume: continuation sessions run ad-hoc (P1) + +The ZimaOS continuation (18:29→19:01) had 81 calls with **zero** +`propose_plan` calls. The agent ran ad-hoc tool calls with no structure. + +**Root cause:** When a session resumes, the `must_have_plan` guard is already +satisfied by the old (completed) plan. The agent doesn't re-plan on resume. + +### F6 — Scope expansion / rabbit holes: agent chases irrelevant sub-goals (P2) + +In the ZimaOS continuation, the agent spent ~40 calls trying to fix Technitium +DHCP reservations — a completely different subsystem from the goal ("make the +dashboard reachable"). The ZimaOS dashboard hadn't started since July 19 +(3-week-old issue), making the DHCP reservation effort moot. The agent never +surfaced a question like "This is pre-existing — should I still fix DHCP?" + +**Root cause:** No scope gate. When the agent pivots to a subsystem unrelated +to the stated goal, nothing stops it. The `session_questions` mechanism exists +(2 calls in 193 sessions) but the model never uses it. + +### F7 — Command generation errors: malformed bash from the LLM (P2) + +In the ZimaOS continuation, the agent generated: +- `head - n` instead of `head -n` → bash syntax error +- `echo "---" && \n curl ...` → literal `\n` in command → ambiguous redirect + +**Root cause:** The LLM generates bash commands inline in content blocks. No +syntax validation, no escape-character handling. The `run` tool should reject +malformed commands before execution. + +### F8 — Learning pipeline completely empty (P4) + +| Table | Rows | +|-------|------| +| `classifications` | **0** | +| `feedback` | **0** | +| `patterns` | **0** | +| `skills` | **0** | + +Despite 1,884 executions and 263 approvals, the system learns nothing from +outcomes. The ZimaOS session discovered that the dashboard hadn't started since +July 19 — this was never persisted. The HMAC trailing-newline discovery was +persisted manually; if the agent forgot `upsert_knowledge`, it would be lost. + +### F9 — Observability gaps (P3) + +| Gap | Detail | +|-----|--------| +| Token tracking | `agent_activity.token_count` is NULL for every row | +| Execution linkage | `nomos_plan_executions` is empty; `audit_log.session_id` is null | +| Plan quality | No metric for step completion rate (currently 38%) | +| MCP timeout rate | No counter for `run` calls that hit the client timeout | + +### F10 — Execution success rate is 74% (P2) + +1,884 executions: 1,400 completed (74%), 295 failed (15.6%), 152 cancelled +(8%), 37 denied (2%). One in four execution attempts doesn't complete. + +--- + +## 3. Improvement plan (ordered by impact/effort) + +### Task 1 — Prevent premature `complete_task(success)` *(fixes F2)* + +**`internal/mcp/tools.go`** — In the `complete_task` handler, when +`outcome=success`: require the summary field to contain a verifiable state +assertion. Minimum: if the goal mentions a URL, check that the summary doesn't +contradict known state. Lightweight: log a warning if the summary says "returns +200" but the last `ping_service` or `run` result says otherwise. + +**Coach:** `nomos/SOUL.md` — explicit rule: *"Before calling +complete_task(success), restate the user's original goal in your own words and +verify each condition. If any condition is 'probably works' rather than +'verified,' ask the operator or set outcome=partial."* + +### Task 2 — Target validation in `run` *(fixes F3)* + +**`internal/mcp/server.go`** — In the `run` handler, before dispatching: +validate that the command prefix matches the target type. + +``` +pct/qm/pvesh/iptables → only host:* targets +systemctl/docker → host:* or lxc:* targets +curl/nmap/ss → any target +``` + +If mismatched, return a clear error: *"Cannot run `qm` on lxc:dns — `qm` is a +Proxmox host command. Use target host:hubris or host:strong."* Do not classify +or execute. + +**Test:** `TestClassifyCommand_WrongTarget` → commands with host-only prefixes +on LXC targets return error without execution. + +### Task 3 — Raise MCP client timeout; add async path for long-running commands *(fixes F1)* + +**`cmd/nomos`** — Raise the MCP client timeout from 30s to 120s. + +**`internal/mcp/server.go`** — For `run` commands that the classifier +determines will exceed the client timeout (presence of `sleep`, `wait`, +`timeout` in the command), return immediately with an `execution_id` and status +`running`. The agent already has `get_execution_status` — use it: + +1. Classify the command; if it contains `sleep`, `wait`, or shell constructs + that imply polling, flag it as `async_potential`. +2. Start the command, return the `execution_id` immediately. +3. Agent polls with `get_execution_status(execution_id)`. +4. If the client timeout is hit mid-poll, the execution continues on the server + — it's not lost. + +**Test:** `run` with `sleep 60; echo done` on host:hubris → returns +immediately (not 30s timeout), `get_execution_status` eventually returns +`completed`. + +### Task 4 — Plan step integrity: require `replaced_reason` on replacement *(fixes F4)* + +**`session_plan_steps` migration** — Add `replaced_reason TEXT` column. + +**`cmd/nomos`** — When the agent emits `update_plan_step` with +`status=replaced`, require a non-empty `replaced_reason`. Valid reasons: +`wrong_diagnosis`, `scope_change`, `blocked`, `superseded`, `operator_override`. + +**Coach:** `nomos/SOUL.md` — explicit rule: *"Complete (status=done) or +explicitly skip (status=skipped) steps. Use status=replaced only when the +entire plan generation is wrong; include the reason. Replacing all steps with +no reason is a session-quality violation."* + +### Task 5 — Force `propose_plan` on session resume *(fixes F5)* + +**`cmd/nomos`** — When a session with status `done` or `failed` receives a new +user message, reset the plan state: clear step status, require a new +`propose_plan` call before any `run` calls. The "must have plan" guard should +consider the resumed session as plan-less until a fresh `propose_plan` is +called. + +**Guard:** `set_goal` + `propose_plan` must be called before any `run` in a +resumed session. Reuse the existing "No plan — call set_goal then propose_plan" +error from `internal/mcp/server.go`. + +### Task 6 — Scope gate: surface `session_questions` on context switch *(fixes F6)* + +**Coach:** `nomos/SOUL.md` — explicit rule: *"Before pivoting to a subsystem +not mentioned in the user's goal, ask via session_questions. Example: 'The +dashboard logs show it hasn't started since July 19. Do you want me to debug +the dashboard service itself [A], skip it and just stabilize the IP [B], or +stop here [C]?'"* + +**`cmd/nomos` prompt** — Add to the system prompt: *"When the investigation +leads to a subsystem or root cause unrelated to the expressed goal, surface a +session_question before taking action."* + +### Task 7 — Auto-upsert knowledge on session close *(fixes F8)* + +**`cmd/nomos`** — On `complete_task` (any outcome: success, partial, failure), +auto-generate a knowledge entry: + +``` +title: ": " +content: "## Outcome\n\n## Root cause\n\n## What was done\n\n## What was left\n" +tags: [session:] +about: [entities involved] +``` + +This ensures every session leaves a trace regardless of whether the agent +remembered to call `upsert_knowledge`. + +### Task 8 — Token tracking *(fixes F9)* + +**`cmd/nomos`** — After each LLM call, extract `usage.prompt_tokens`, +`usage.completion_tokens`, `usage.total_tokens` from the response and write to +`agent_activity.token_count`. Currently the field exists but is never populated +(NULL for all rows). + +### Task 9 — Execution linkage *(fixes F9)* + +**`internal/mcp/server.go`** — When `run` creates an execution, write a row +into `nomos_plan_executions` linking `session_id`, `plan_step_seq`, and +`execution_id`. + +**`internal/mcp/server.go`** — Pass `session_id` (from MCP request headers) +into `audit_log` writes. Currently `audit_log.session_id` is NULL — the +`createAuditLog` function in `internal/httpapi/impl.go` receives the +correlation_id but not the session_id from the MCP path. + +### Task 10 — Plan quality metric *(fixes F9)* + +**`cmd/nomos`** — At session close, compute: `completed_steps / +total_steps_per_plan` (currently ~38%). Log as a metric or write as a session +attribute. Track over time to measure plan-adherence improvements from Tasks +4+5. + +### Task 11 — Auto-classify every `run` call *(fixes F8)* + +**`internal/mcp/server.go`** — The `run` handler already calls the classifier +(`classifyCommand` in `internal/policy/command.go`) to determine risk_class and +approval route. Write the result to the `classifications` table. Currently the +table is empty (0 rows) despite 1,884 executions being classified. + +### Task 12 — Auto-feedback on session close *(fixes F8)* + +**`cmd/nomos`** — On `complete_task`, generate a `feedback` entry: + +``` +session_id: +outcome: +observation: +lesson: +side_effects: +``` + +**`cmd/oikos`** — Add a daily cron or scheduler job that reads recent +`feedback` entries and extracts `patterns` (recurring root causes, same-fix +applied multiple times, known-broken services). Seed the pattern table. + +### Task 13 — Command syntax validation in `run` *(fixes F7)* + +**`internal/mcp/server.go`** — Before executing a `run` command, do +lightweight bash syntax validation: + +``` +- Reject literal \n in commands (should be ; or &&) +- Reject commands where the last line ends with \ (backslash-continuation) + but no next line +- Warn on common typos: "head - n", "grep - i", spaces before flags +- Reject `&& \n` patterns (the LLM sometimes inserts literal \n between && chains) +``` + +### Task 14 — Stuck-session reaping (from prior plan; re-confirmed) + +This session exhibited the same idle zombie pattern (23da10db — no closed_at, +status `failed` but outcome `failure`). Task 5 from the 2026-08-03 plan is +still open. Copying here for completeness. + +--- + +## 4. Recommended sequence + +``` +P0 (blocks operational waste): + 1 → 2 → 3 + +P1 (fixes plan architecture): + 4 → 5 + +P2 (cognitive guardrails): + 6 → 7 → 13 + +P3 (observability): + 8 → 9 → 10 + +P4 (learning loop): + 11 → 12 +``` + +Sequence rationale: Tasks 1-3 stop the worst outcomes (premature success, +wrong-target execution, MCP timeouts). Tasks 4-5 make the plan system actually +useful instead of decorative. Tasks 6-7 add guardrails that prevent the ZimaOS +rabbit-hole class of failure. Tasks 8-10 give us visibility into whether any of +the previous tasks are working. Tasks 11-12 close the learning loop. + +--- + +## 5. Validation + +| Task | Test | +|------|------| +| 1 | Session with goal "make X reachable" where last ping shows 502 → `complete_task(success)` is rejected or warns | +| 2 | `run("lxc:dns", "qm stop 100")` → error: "qm is a Proxmox host command" | +| 3 | `run` with `sleep 45; echo done` → returns execution_id immediately, `get_execution_status` shows final result | +| 4 | `update_plan_step(status=replaced)` with no reason → rejected; with reason → accepted | +| 5 | Resumed session calls `run` before `propose_plan` → blocked: "No plan — call propose_plan" | +| 6 | Agent pivots to unrelated subsystem → `session_questions` is called before action | +| 7 | `complete_task` → knowledge entry created automatically with session link | +| 8 | `agent_activity.token_count` is non-NULL after any LLM call | +| 9 | `nomos_plan_executions` has rows linking session + step + execution | +| 10 | Session close writes `plan_adherence` attribute (step-completion %) | +| 11 | `classifications` table has 1 row per `run` call with risk_class + route | +| 12 | `complete_task` → auto `feedback` entry; daily pattern job finds recurring issues | +| 13 | `run` with `head - n /etc/hosts` → rejected with clear error about malformed command | + +--- + +## 6. Out of scope / open questions + +- Whether to raise `auto_act` from `off` for `reversible_low` actions (separate + policy decision; would reduce approval pileup without code changes). +- Whether to add a `delete_entity` MCP tool for lifecycle management (separate + from this reliability plan). +- The exact TTL for stuck-session reaping (30 min recommended, confirmed in + 2026-08-03 plan). +- Whether `run` async mode should be opt-in (command contains sleep/wait) or + universal (every run returns immediately, agent always polls). Recommend + opt-in for now — most commands complete in <5s.