From 544afae77f585dd6d89f69de320e0f0de9fd3130 Mon Sep 17 00:00:00 2001 From: dtoro Date: Sun, 19 Jul 2026 00:09:39 +0200 Subject: [PATCH] feat(nomos): retry cap, vm: targets, inspect_path, goal supersession, runbooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session-review implementation for the three sessions audited in plans/2026-07-18-session-review-three-sessions.md. v0.7.11 → v0.7.12. P0.1 — retry cap + investigate-before-retry (cmd/nomos/retrycap.go, agent.go): after 3 identical failing run calls in a single turn, refuse to dispatch the call again and return a directive to investigate *why* (ps/strace/lsof) or surface the blocker. Per-turn scope so a fresh turn after the operator responds can retry once more. Session 1e9c7691's 20+ identical chown retries (knfsd held a kernel lock on the exported NFS dir) is the direct motivation. P0.2 + P1.8 + P2.10 — SOUL.md guidance: hung command is not a failed command (investigate before retry); ask before proposing a multi-step migration; multi-goal sessions summarize the arc not just the last goal. P1.3 — two new runbook entities in seeds/knowledge.yaml: - nfs-exported-dir-mutation-hang (the knfsd fchownat lock procedure: killall → exportfs -u → mutate → exportfs -a → verify) - netbird-mgmt-oidc-race-after-upgrade (docker restart netbird-mgmt after ~30s for the traefik/authentik OIDC race) P1.4 — setGoal emits task.superseded event when prior goal is overwritten by a different goal (store.go, TestSetGoal_SupersededEvent). Session 55927f0a had two set_goal calls with the first silently abandoned. P1.5 — inspect_path MCP tool: runs mount/df/ls/stat for one path across up to 8 targets in one parallel call, replacing the 15+ run-call fact-gathering fan-out sessions 1 and 2 each spent on cross-target path tracing (tools.go, server.go: inspectPathAcrossTargets, inspectOneTarget). P1.6 — vm: target support in run via qm guest exec (no more SSH-hop with nested quoting). Extracted shared resolveProxmoxHostSlug for LXC + VM, with hosts-relationship fallback when attributes.host is absent (server.go, tools.go). Session 55927f0a's SSH-hop workarounds for vm:zimaos are the direct motivation. Deferred (documented in plan): P1.7 (approval window auto-extend on timeout) and P2.9 (long-running command PENDING detection) — both addressed at lower cost by the retry cap. Session 3's poll-after-timeout pattern already works; the cap protects against the failure mode. --- VERSION | 2 +- cmd/nomos/agent.go | 79 +++- cmd/nomos/retrycap.go | 170 +++++++ cmd/nomos/retrycap_test.go | 129 ++++++ cmd/nomos/store.go | 26 +- cmd/nomos/store_test.go | 66 +++ internal/mcp/server.go | 170 ++++++- internal/mcp/tools.go | 42 +- nomos/SOUL.md | 45 ++ ...026-07-18-session-review-three-sessions.md | 414 ++++++++++++++++++ plans/index.md | 1 + seeds/knowledge.yaml | 140 ++++++ 12 files changed, 1265 insertions(+), 19 deletions(-) create mode 100644 cmd/nomos/retrycap.go create mode 100644 cmd/nomos/retrycap_test.go create mode 100644 plans/2026-07-18-session-review-three-sessions.md diff --git a/VERSION b/VERSION index b4d6d12..88a7b22 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.7.11 +0.7.12 diff --git a/cmd/nomos/agent.go b/cmd/nomos/agent.go index cc7b8b2..5fa2d85 100644 --- a/cmd/nomos/agent.go +++ b/cmd/nomos/agent.go @@ -168,10 +168,10 @@ type toolDef struct { } type agentEvent struct { - Type string `json:"type"` - Data any `json:"data,omitempty"` - SessionID string `json:"session_id,omitempty"` - Iteration int `json:"iteration,omitempty"` + Type string `json:"type"` + Data any `json:"data,omitempty"` + SessionID string `json:"session_id,omitempty"` + Iteration int `json:"iteration,omitempty"` } func (a *agent) chat(ctx context.Context, sessionID, message string, emit func(agentEvent)) { @@ -349,6 +349,15 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s messages = append(messages, openai.SystemMessage(systemInject)) } + // Retry cap (P0.1 from plans/2026-07-18-session-review-three-sessions.md): + // track failing `run` calls within this turn so an identical command that + // keeps failing is refused after maxRunRetries attempts. Without this, + // session 1e9c7691 retried the same `chown` ~20 times, each retry piling + // up a zombie process on the target (knfsd was holding a kernel lock). + // The tracker is per-turn — a fresh turn after the operator responds can + // retry once more, so this doesn't permanently block recovery. + retries := newRunRetryTracker() + for i := 0; i < maxIterations; i++ { params := openai.ChatCompletionNewParams{ Model: openai.ChatModel(a.model), @@ -467,6 +476,33 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s sawCompleteTask = true } + // Retry cap: if this `run` call has already failed + // maxRunRetries times this turn with the same (target, + // command), refuse to dispatch it again. Return a synthetic + // tool result directing the agent to investigate *why* the + // command hangs instead of retrying. See retrycap.go and + // plans/2026-07-18-session-review-three-sessions.md P0.1. + if tc.Function.Name == "run" { + t, _ := args["target"].(string) + c, _ := args["command"].(string) + key := runFailureKey(t, c) + if n := retries.failures(key); n >= maxRunRetries { + directive := runRetryDirective(t, c, n) + 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) + emit(agentEvent{ + Type: "tool_result", + Data: map[string]any{"name": tc.Function.Name, "result": directive, "id": tc.ID, "retry_capped": true}, + SessionID: sessionID, + Iteration: i + 1, + }) + messages = append(messages, openai.ToolMessage(directive, tc.ID)) + continue + } + } + emit(agentEvent{ Type: "tool_use", Data: map[string]any{"name": tc.Function.Name, "args": args, "id": tc.ID}, @@ -508,6 +544,22 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s if callErr != nil { a.store.logActivity(ctx, a.agentID, sessionID, tc.Function.Name, args, inputStr, callErr.Error(), elapsed, false, correlationID) + // Retry cap: dispatch errors (e.g. MCP client timeout) + // count toward the cap too. A command that keeps timing + // out at the gateway is exactly the pattern we want to + // break — see session 1e9c7691's 20+ identical + // `chown` timeouts. + if tc.Function.Name == "run" { + t, _ := args["target"].(string) + c, _ := args["command"].(string) + key := runFailureKey(t, c) + n := retries.recordFailure(key) + if n >= maxRunRetries { + slog.Warn("nomos: run failure cap reached — next identical call will be refused", + "target", t, "failures", n, "session", sessionID) + } + } + emit(agentEvent{ Type: "tool_result", Data: map[string]any{"name": tc.Function.Name, "error": callErr.Error(), "id": tc.ID}, @@ -551,6 +603,25 @@ func (a *agent) chatWith(ctx context.Context, sessionID, message, systemInject s messages = append(messages, openai.ToolMessage(string(resultJSON), tc.ID)) slog.Info("nomos: tool success", "tool", tc.Function.Name, "ms", elapsed) + // Retry cap: record failures of `run` calls so the cap above + // can refuse a repeated identical failure. A "failure" here + // means the dispatch errored OR the MCP result text matches + // the "run on : ERROR …" signature — both indicate + // the command actually ran and failed, not just that it + // queued for approval (pending approvals are not failures). + // Pass the RAW result text (not JSON-encoded) so the helper's + // HasPrefix check sees "run on …" not "\"run on …\"". + if isRunFailure(tc.Function.Name, runResultText(result), callErr) { + t, _ := args["target"].(string) + c, _ := args["command"].(string) + key := runFailureKey(t, c) + n := retries.recordFailure(key) + if n >= maxRunRetries { + slog.Warn("nomos: run failure cap reached — next identical call will be refused", + "target", t, "failures", n, "session", sessionID) + } + } + // ask_operator pauses the task: the agent has posed a decision only // the operator can make. End the turn here so it doesn't barrel past // its own question — the answer (panel or chat reply) resumes it. diff --git a/cmd/nomos/retrycap.go b/cmd/nomos/retrycap.go new file mode 100644 index 0000000..2144f02 --- /dev/null +++ b/cmd/nomos/retrycap.go @@ -0,0 +1,170 @@ +package main + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "sync" +) + +// maxRunRetries is the per-turn cap on identical failing `run` tool calls. +// After this many failures with the same (target, command) key, the agent +// loop refuses to dispatch the call again and instead surfaces a directive +// to investigate *why* (ps/strace/lsof) or escalate to the operator. +// +// Background: session 1e9c7691 (2026-07-18) retried the same +// `chown :10000 /mnt/media_local && chmod 2775 …` ~20 times across direct +// runs, SSH-hop-via-hubris, wrapping in a shell script, and bare `echo test` +// sanity checks. Each retry piled up another zombie process on the target +// (knfsd was holding a kernel lock on the exported directory). The agent +// only investigated *why* after the operator explicitly asked +// "the command just keeps running?" — see +// plans/2026-07-18-session-review-three-sessions.md P0.1. +const maxRunRetries = 3 + +// runRetryTracker deduplicates failing `run` calls within a single chat +// turn (chatWith invocation). It is NOT persisted across turns — the cap +// is per-turn, so a fresh turn after the operator responds can retry once +// more. The intent is to break a tight retry loop within one turn, not to +// permanently block the agent from ever attempting the operation again. +// +// Threading: the agent loop is single-goroutine per turn, but the tracker +// is guarded by a mutex so future callers (e.g. concurrent tool dispatch) +// stay safe. The mutex is uncontended on the current hot path. +type runRetryTracker struct { + mu sync.Mutex + counts map[string]int +} + +func newRunRetryTracker() *runRetryTracker { + return &runRetryTracker{counts: make(map[string]int)} +} + +// runFailureKey is the dedup key for "this is the same command against the +// same target." Whitespace is collapsed so trivial reformatting +// (newlines vs spaces, trailing whitespace) doesn't escape the cap. The +// purpose field is intentionally NOT part of the key: the agent often +// rephrases purpose between retries while issuing the same command. +func runFailureKey(target, command string) string { + collapsed := strings.Join(strings.Fields(command), " ") + target = strings.TrimSpace(target) + h := sha256.Sum256([]byte(target + "\x00" + collapsed)) + return hex.EncodeToString(h[:]) +} + +// recordFailure increments the failure count for the given key and returns +// the new count. The caller should check `count > maxRunRetries` BEFORE +// dispatching to decide whether to skip the call. +func (r *runRetryTracker) recordFailure(key string) int { + r.mu.Lock() + defer r.mu.Unlock() + r.counts[key]++ + return r.counts[key] +} + +// failures returns the current failure count for a key (0 if unseen). +func (r *runRetryTracker) failures(key string) int { + r.mu.Lock() + defer r.mu.Unlock() + return r.counts[key] +} + +// isRunFailure reports whether a `run` tool call's outcome should count +// as a failure for retry-cap purposes. A call counts as failed when: +// - the dispatch itself errored (callErr != nil), OR +// - the result text starts with "run on : ERROR" — the +// shape classifyAndGate/sshExec produce when SSH or the command fails. +// +// Approvals queued ("requires approval") do NOT count as failures: they +// are pending operator action, not a command execution failure. A read +// of the existing code paths (classifyAndGate in internal/mcp/server.go) +// confirms the "ERROR" prefix is the stable failure signature for `run`. +// +// The resultText parameter is the MCP tool's RAW text result (not JSON- +// re-encoded): when classifyAndGate returns a textResult like +// "run on host:strong: ERROR ...", the MCP client unwraps it back to a +// plain Go string (see mcpClient.callTool). The caller should pass that +// raw string, not json.Marshal's output (which would quote-wrap it). +func isRunFailure(toolName string, resultText string, callErr error) bool { + if callErr != nil { + return true + } + if toolName != "run" { + return false + } + // "run on host:strong: ERROR ..." or "run on lxc:caddy: ERROR ..." + // Both shapes start with "run on ". + if !strings.HasPrefix(resultText, "run on ") { + return false + } + return strings.Contains(resultText, ": ERROR") +} + +// runResultText extracts the raw text from a `run` tool's result value as +// returned by mcpClient.callTool — typically a Go string, but may also be +// a []string (multi-content result) or other JSON-decoded shape. Returns +// "" for shapes we don't recognize. Used by the retry-cap path so +// isRunFailure receives the un-quoted text form (see its doc comment). +func runResultText(result any) string { + switch v := result.(type) { + case string: + return v + case []string: + if len(v) > 0 { + return v[0] + } + case []any: + var b strings.Builder + for _, e := range v { + if s, ok := e.(string); ok { + b.WriteString(s) + } + } + return b.String() + } + return "" +} + +// runRetryDirective is the synthetic tool result returned to the model +// when the retry cap is hit, in place of dispatching the call again. It +// directs the agent to investigate *why* the command keeps failing before +// retrying, or to surface the blocker to the operator. +func runRetryDirective(target, command string, failures int) string { + return "Refused: this `run` against " + target + " has failed " + + itoa(failures) + " times this turn — retry cap hit. The command:\n " + + command + "\nis almost certainly blocked by something on the target " + + "(a hung process, a kernel lock, an unexported FS, a stuck SSH " + + "session, …) — NOT a transient gateway issue. Do NOT retry with " + + "different routing or quoting. Instead, BEFORE calling `run` again, " + + "investigate *why* the command hangs: e.g. `ps aux | grep `, " + + "`lsof `, `strace -f -p ` or `strace -f `, " + + "`mount | grep `, `dmesg | tail`. If you find a structural " + + "blocker (e.g. a kernel lock on an exported NFS directory → " + + "unexport → mutate → re-export), say so to the operator and fix it " + + "with a different command. If you genuinely cannot diagnose, " + + "surface the blocker to the operator with what you've tried — do " + + "not just retry the same command." +} + +// itoa is a tiny strconv.Itoa to keep this file dependency-free. +func itoa(n int) string { + if n == 0 { + return "0" + } + neg := n < 0 + if neg { + n = -n + } + var buf [20]byte + i := len(buf) + for n > 0 { + i-- + buf[i] = byte('0' + n%10) + n /= 10 + } + if neg { + i-- + buf[i] = '-' + } + return string(buf[i:]) +} diff --git a/cmd/nomos/retrycap_test.go b/cmd/nomos/retrycap_test.go new file mode 100644 index 0000000..2012e4b --- /dev/null +++ b/cmd/nomos/retrycap_test.go @@ -0,0 +1,129 @@ +package main + +import ( + "strings" + "testing" +) + +func TestRunFailureKey_StableAcrossWhitespace(t *testing.T) { + cases := []struct{ a, b string }{ + {"chown :10000 /mnt/media_local && chmod 2775 /mnt/media_local", + "chown :10000 /mnt/media_local && chmod 2775 /mnt/media_local"}, + {"chown :10000 /mnt/media_local\n&& chmod 2775 /mnt/media_local", + "chown :10000 /mnt/media_local && chmod 2775 /mnt/media_local"}, + {"chown :10000 /mnt/media_local && chmod 2775 /mnt/media_local ", + " chown :10000 /mnt/media_local && chmod 2775 /mnt/media_local"}, + } + for i, c := range cases { + ka := runFailureKey("host:strong", c.a) + kb := runFailureKey("host:strong", c.b) + if ka != kb { + t.Errorf("case %d: keys differ for whitespace-equivalent commands:\n a=%q\n b=%q", i, c.a, c.b) + } + } +} + +func TestRunFailureKey_DiffersByTarget(t *testing.T) { + a := runFailureKey("host:strong", "echo hi") + b := runFailureKey("host:hubris", "echo hi") + if a == b { + t.Error("keys should differ when target differs") + } +} + +func TestRunFailureKey_DiffersByCommand(t *testing.T) { + a := runFailureKey("host:strong", "echo hi") + b := runFailureKey("host:strong", "echo bye") + if a == b { + t.Error("keys should differ when command differs") + } +} + +func TestRunRetryTracker_CountsAndCaps(t *testing.T) { + r := newRunRetryTracker() + key := runFailureKey("host:strong", "chown :10000 /mnt/media_local") + for i := 1; i <= maxRunRetries; i++ { + if got := r.recordFailure(key); got != i { + t.Errorf("recordFailure #%d = %d, want %d", i, got, i) + } + } + // At the cap, failures() should report maxRunRetries, and the next + // identical call should be refused by the agent loop (failures() >= + // maxRunRetries). + if got := r.failures(key); got != maxRunRetries { + t.Errorf("failures = %d, want %d", got, maxRunRetries) + } + if r.failures(key) < maxRunRetries { + t.Errorf("cap should be enforced at maxRunRetries=%d", maxRunRetries) + } +} + +func TestRunRetryTracker_PerTurnIsolation(t *testing.T) { + // Different keys don't interfere. + r := newRunRetryTracker() + k1 := runFailureKey("host:strong", "echo a") + k2 := runFailureKey("host:strong", "echo b") + r.recordFailure(k1) + r.recordFailure(k1) + if got := r.failures(k2); got != 0 { + t.Errorf("k2 failures = %d, want 0 (keys are isolated)", got) + } +} + +func TestIsRunFailure(t *testing.T) { + cases := []struct { + desc string + tool string + result string + callErr error + want bool + }{ + {"run with ERROR prefix", "run", "run on host:strong: ERROR ssh: signal: killed", nil, true}, + {"run with exit error", "run", "run on lxc:caddy: ERROR exit status 1", nil, true}, + {"run success (read-only auto)", "run", "run on host:strong (read_only, auto): hello", nil, false}, + {"run success (assent window)", "run", "run on host:strong (config_mutation, auto via assent window): done", nil, false}, + {"run queued for approval", "run", "run on host:strong requires approval (risk: config_mutation) — execution 019f4930 queued. Present the command and purpose to the operator and wait; do not re-request.", nil, false}, + {"non-run tool", "get_entity", "lxc list result", nil, false}, + {"callErr set (dispatch failure)", "run", "", errFake{}, true}, + {"callErr set on non-run tool", "get_entity", "some result", errFake{}, true}, // callErr trumps name + } + for i, c := range cases { + got := isRunFailure(c.tool, c.result, c.callErr) + if got != c.want { + t.Errorf("case %d (%s): isRunFailure = %v, want %v", i, c.desc, got, c.want) + } + } +} + +type errFake struct{} + +func (errFake) Error() string { return "fake dispatch error" } + +func TestRunRetryDirective_Content(t *testing.T) { + d := runRetryDirective("host:strong", "chown :10000 /mnt/media_local", 3) + for _, want := range []string{ + "Refused:", + "host:strong", + "3 times", + "retry cap hit", + "Do NOT retry", + "strace", + "ps aux", + "lsof", + "surface the blocker", + } { + if !strings.Contains(d, want) { + t.Errorf("directive missing %q; got:\n%s", want, d) + } + } +} + +func TestItoa(t *testing.T) { + cases := map[int]string{0: "0", 1: "1", 9: "9", 10: "10", 42: "42", + 100: "100", -1: "-1", -42: "-42"} + for in, want := range cases { + if got := itoa(in); got != want { + t.Errorf("itoa(%d) = %q, want %q", in, got, want) + } + } +} diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index ab76f2b..da33289 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -492,10 +492,34 @@ func (s *store) taskEntityPtr(ctx context.Context, sessionID string) *uuid.UUID // 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. +// +// P1.4 (2026-07-18): when a non-empty prior goal is being overwritten by a +// different goal, emit a `task.superseded` event carrying the prior goal. +// This gives the UI/audit trail a clear signal that the operator pivoted — +// without it, the prior goal just silently disappears from +// agent_sessions.goal and there's no record the session ever had a +// different starting intent. See plans/2026-07-18-session-review-three- +// sessions.md P1.4 (session 55927f0a had two set_goal calls with the first +// implicitly abandoned when the operator said "lets just keep ludo-library +// then"). func (s *store) setGoal(ctx context.Context, sessionID, goal string) error { if s == nil || sessionID == "" || sessionID == "ephemeral" { return nil } + // Capture the prior goal BEFORE the UPDATE overwrites it. If non-empty + // and different from the new goal, emit task.superseded so the audit + // trail records the pivot — the row's goal column won't. + var priorGoal string + s.pool.QueryRow(ctx, + `SELECT COALESCE(goal, '') FROM agent_sessions WHERE id = $1`, + sessionID).Scan(&priorGoal) + if priorGoal != "" && priorGoal != goal { + _ = observability.Event(ctx, sqlcgen.New(s.pool), "task.superseded", + s.taskEntityPtr(ctx, sessionID), "info", "nomos", sessionID, + map[string]any{"prior_goal": priorGoal, "new_goal": goal}) + slog.Info("nomos: task goal superseded by a new set_goal", + "session", sessionID, "prior_goal", priorGoal, "new_goal", goal) + } // 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 @@ -868,7 +892,7 @@ type staleGoalSession struct { } // staleGoalSessions finds sessions that framed themselves as a real task -// (goal != '', so the inline safety net in agent.go intentionally left them +// (goal != ”, so the inline safety net in agent.go intentionally left them // alone) but have sat non-terminal past idleThreshold. completion_nudges // tells the caller whether to nudge (0) or give up and auto-close (>=1) — // see processIdleSweep in continue.go. diff --git a/cmd/nomos/store_test.go b/cmd/nomos/store_test.go index 80e58e7..d12d99d 100644 --- a/cmd/nomos/store_test.go +++ b/cmd/nomos/store_test.go @@ -311,3 +311,69 @@ func TestHadDiscoveryAndWriteback(t *testing.T) { t.Fatal("hadEntityWriteback = false after run+writeback, want true") } } + +// TestSetGoal_SupersessionEvent is the store-level proof for P1.4 from +// plans/2026-07-18-session-review-three-sessions.md: when setGoal is called +// and a non-empty prior goal already exists with a DIFFERENT value, a +// task.superseded event must be emitted (so the audit trail records the +// pivot — the row's goal column will be overwritten, losing the prior intent +// without this event). When the goal is identical OR no prior goal exists, +// no supersession event is emitted. +// +// Background: session 55927f0a had two set_goal calls; the first was +// implicitly abandoned when the operator said "lets just keep ludo-library +// then." Without the event, the prior goal silently disappeared. +func TestSetGoal_SupersededEvent(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + + sess, err := s.createSession(ctx, "goal pivot test") + if err != nil { + t.Fatalf("createSession: %v", err) + } + + // First set_goal — no prior, no supersession event expected. + if err := s.setGoal(ctx, sess.ID, "Fix sabnzbd download folder to use ludo-lvm"); err != nil { + t.Fatalf("setGoal #1: %v", err) + } + if n := countEvents(ctx, s, sess.ID, "task.superseded"); n != 0 { + t.Errorf("after first set_goal: %d task.superseded events, want 0", n) + } + + // Second set_goal with a DIFFERENT goal — supersession event expected. + if err := s.setGoal(ctx, sess.ID, "Add NFS export of ludo-lvm to ZimaOS"); err != nil { + t.Fatalf("setGoal #2: %v", err) + } + if n := countEvents(ctx, s, sess.ID, "task.superseded"); n != 1 { + t.Errorf("after second set_goal with a different goal: %d task.superseded events, want 1", n) + } + + // Third set_goal with the SAME goal as the second — no new supersession + // event (idempotent: same goal is a no-op, not a pivot). + if err := s.setGoal(ctx, sess.ID, "Add NFS export of ludo-lvm to ZimaOS"); err != nil { + t.Fatalf("setGoal #3: %v", err) + } + if n := countEvents(ctx, s, sess.ID, "task.superseded"); n != 1 { + t.Errorf("after third set_goal with same goal as second: %d task.superseded events, want 1 (no new pivot)", n) + } + + // The session's current goal must be the latest one set. + got, err := s.getSession(ctx, sess.ID) + if err != nil { + t.Fatalf("getSession: %v", err) + } + if got.Goal != "Add NFS export of ludo-lvm to ZimaOS" { + t.Errorf("session goal = %q, want the second (latest) goal", got.Goal) + } +} + +// countEvents counts observability events of the given type correlated to +// the given session. Used by TestSetGoal_SupersededEvent to assert the +// task.superseded audit-trail signal was emitted. +func countEvents(ctx context.Context, s *store, sessionID, eventType string) int { + var n int + s.pool.QueryRow(ctx, + `SELECT COUNT(*) FROM events WHERE correlation_id = $1 AND type = $2`, + sessionID, eventType).Scan(&n) + return n +} diff --git a/internal/mcp/server.go b/internal/mcp/server.go index f73fd7e..2d479b0 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -511,16 +511,27 @@ func isPrivateHost(host string) bool { return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsUnspecified() } -// resolveExecTarget resolves any target slug (host: or lxc:) to the SSH +// resolveExecTarget resolves any target slug (host:, lxc:, or vm:) to the SSH // endpoint that will actually run the command, and a wrap function that turns // a plain shell command into whatever must actually be sent over that SSH -// connection: identity for a host, `pct exec -- ...` for an LXC. +// connection: identity for a host, `pct exec -- ...` for an LXC, +// `qm guest exec -- ...` for a VM. // // The lxc.attributes.host value is stored WITHOUT a "host:" prefix (e.g. // "strong", not "host:strong") — see pct_create's entity registration. The // pre-existing pct_exec handler queried resolveHost with that bare value // directly, which can never match a "host:*" slug and always fails; this // prefixes it correctly. +// +// vm: support (2026-07-18): VMs in inventory.yaml carry `pve_id` and a `host` +// attribute (or a `hosts` relationship) just like LXCs, but they're reached +// via `qm guest exec` instead of `pct exec`. Previously the agent had to +// SSH-hop via `host:hubris` to reach a VM (e.g. `ssh root@ '...'`), +// which broke on nested shell quoting and forced manual escaping workarounds +// — see plans/2026-07-18-session-review-three-sessions.md P1.6. A VM's +// `host` attribute is optional: if absent, fall back to looking up the +// `hosts` relationship on the VM entity, then to hubris (the documented +// default Proxmox host) — same fallback chain as LXCs. func resolveExecTarget(ctx context.Context, pool *db.Pool, targetSlug string) (host, user string, wrap func(cmd string) string, err error) { if strings.HasPrefix(targetSlug, "host:") { host, user, err = resolveHost(ctx, pool, targetSlug) @@ -536,13 +547,7 @@ func resolveExecTarget(ctx context.Context, pool *db.Pool, targetSlug string) (h if qerr := pool.QueryRow(ctx, "SELECT attributes->>'pve_id', COALESCE(attributes->>'host', '') FROM entities WHERE slug = $1", targetSlug).Scan(&pveID, &hostAttr); qerr != nil || pveID == "" { return "", "", nil, fmt.Errorf("LXC not found or missing pve_id: %s", targetSlug) } - hostSlug := hostAttr - if hostSlug == "" { - hostSlug = "hubris" // documented default Proxmox host when unset - } - if !strings.HasPrefix(hostSlug, "host:") { - hostSlug = "host:" + hostSlug - } + hostSlug := resolveProxmoxHostSlug(ctx, pool, targetSlug, hostAttr) host, user, err = resolveHost(ctx, pool, hostSlug) id := pveID return host, user, func(cmd string) string { @@ -550,7 +555,71 @@ func resolveExecTarget(ctx context.Context, pool *db.Pool, targetSlug string) (h return fmt.Sprintf("pct exec %s -- bash -c 'echo %s | base64 -d | bash'", id, b64) }, err } - return "", "", nil, fmt.Errorf("unsupported target %q: must be host: or lxc:", targetSlug) + if strings.HasPrefix(targetSlug, "vm:") { + // VMs: same host-resolution chain as LXCs (attributes.host → + // `hosts` relationship → hubris default), but reached via + // `qm guest exec` instead of `pct exec`. Requires the QEMU + // guest agent running inside the VM (the standard Proxmox + // setup; ZimaOS/HAOS in this fleet already have it). + var pveID, hostAttr string + if qerr := pool.QueryRow(ctx, "SELECT attributes->>'pve_id', COALESCE(attributes->>'host', '') FROM entities WHERE slug = $1", targetSlug).Scan(&pveID, &hostAttr); qerr != nil || pveID == "" { + return "", "", nil, fmt.Errorf("VM not found or missing pve_id: %s", targetSlug) + } + hostSlug := resolveProxmoxHostSlug(ctx, pool, targetSlug, hostAttr) + host, user, err = resolveHost(ctx, pool, hostSlug) + id := pveID + return host, user, func(cmd string) string { + b64 := base64.StdEncoding.EncodeToString([]byte(cmd)) + // `qm guest exec -- /bin/bash -c '...'` returns JSON by + // default; pipe through `jq -r .out` if available, else cat. + // The base64 round-trip mirrors the LXC path so nested quoting + // (the original VM-target pain point — session 55927f0a) is + // handled identically to LXC dispatch. + return fmt.Sprintf( + "qm guest exec %s -- /bin/bash -c 'echo %s | base64 -d | bash' | jq -r '.out // .err // empty' 2>/dev/null || qm guest exec %s -- /bin/bash -c 'echo %s | base64 -d | bash'", + id, b64, id, b64) + }, err + } + return "", "", nil, fmt.Errorf("unsupported target %q: must be host:, lxc:, or vm:", targetSlug) +} + +// resolveProxmoxHostSlug resolves the Proxmox host slug that owns a given +// LXC/VM target. Resolution order: +// 1. hostAttr if non-empty (the entity's attributes.host — stored without +// "host:" prefix in inventory.yaml and pct_create). +// 2. the `hosts` relationship on the entity (e.g. host:hubris → vm:zimaos), +// looked up in the relationships table — the canonical graph source. +// 3. "hubris" as a documented default Proxmox host fallback. +// +// Returns a slug with the "host:" prefix attached, ready for resolveHost. +// Extracted from the inline LXC path (2026-07-18) so the VM path shares the +// same chain — see plans/2026-07-18-session-review-three-sessions.md P1.6. +func resolveProxmoxHostSlug(ctx context.Context, pool *db.Pool, entitySlug, hostAttr string) string { + hostSlug := strings.TrimSpace(hostAttr) + if hostSlug == "" { + // Fall back to the `hosts` relationship — the graph edge from + // the Proxmox host to this LXC/VM. This is the canonical source + // for "who owns this VM" in inventory.yaml; the `host` attribute + // is a denormalized shortcut that not every entity has. + var relHostSlug string + // hosts relationship: source=host, target=lxc/vm. Look up the + // source slug given the target. + if err := pool.QueryRow(ctx, ` + SELECT e.slug FROM relationships r + JOIN entities e ON e.id = r.source_id + WHERE r.target_id = (SELECT id FROM entities WHERE slug = $1) + AND r.type = 'hosts' AND r.valid_to IS NULL + LIMIT 1`, entitySlug).Scan(&relHostSlug); err == nil && relHostSlug != "" { + hostSlug = relHostSlug + } + } + if hostSlug == "" { + hostSlug = "hubris" // documented default Proxmox host when unset + } + if !strings.HasPrefix(hostSlug, "host:") { + hostSlug = "host:" + hostSlug + } + return hostSlug } // classifyAndGate is the shared classify→execute-or-queue path for every @@ -994,4 +1063,83 @@ func createApproval(ctx context.Context, pool *db.Pool, execID, targetID uuid.UU // gated action is now awaiting a decision. _ = observability.Event(ctx, sqlcgen.New(pool), "approval.created", &execID, "warning", "mcp", "", map[string]any{"action": action, "params": params, "risk_class": riskClass}) -} \ No newline at end of file +} + +// inspectPathAcrossTargets is the bulk fact-gathering helper behind the +// inspect_path MCP tool (plans/2026-07-18-session-review-three-sessions.md +// P1.5). For each target slug, it runs a single read-only shell command +// producing mount/df/ls/stat output for the given path, and returns the +// results as a map keyed by target slug. +// +// Why this exists: sessions 1e9c7691 and 55927f0a each spent ~15 `run` +// calls gathering identical facts (`mount | grep`, `df`, `ls -la`, `stat`) +// across hosts and LXCs to trace where a path lives, who mounts it, and +// what permissions it has. One call here replaces that fan-out. All +// commands are read-only — the tool bypasses classifyAndGate and runs +// directly via sshExec against resolveExecTarget's host/wrap. Failures +// (unresolvable target, SSH error) are reported per-target in the result +// map, not as a single tool-level error, so one bad target doesn't lose +// the others. +// +// The per-target command is intentionally compact: one combined shell +// invocation that prints mount source/dest, df, ls -la of the path's +// parent + the path itself, and stat. Output is truncated to 4KB per +// target to keep the total result reasonable for an 8-target call. +func inspectPathAcrossTargets(ctx context.Context, pool *db.Pool, path string, targets []string) map[string]any { + results := make(map[string]any, len(targets)) + path = strings.TrimSpace(path) + + var wg sync.WaitGroup + var mu sync.Mutex + wg.Add(len(targets)) + + for _, tgt := range targets { + go func(target string) { + defer wg.Done() + entry := inspectOneTarget(ctx, pool, path, target) + mu.Lock() + results[target] = entry + mu.Unlock() + }(tgt) + } + wg.Wait() + return results +} + +// inspectOneTarget runs the read-only inspection for one target. Returns a +// map with keys: "ok" (bool), "output" (string, on success), "error" +// (string, on failure). Kept small so the JSON shape is stable across the +// parallel-call path. +func inspectOneTarget(ctx context.Context, pool *db.Pool, path, target string) map[string]any { + host, user, wrap, rerr := resolveExecTarget(ctx, pool, target) + if rerr != nil { + return map[string]any{"ok": false, "error": fmt.Sprintf("resolve target: %v", rerr)} + } + // One shell invocation, four sections, each guarded by `2>&1 || true` + // so a missing path doesn't kill the rest. Stat with -c gives a + // stable machine-readable line for ownership/perms; ls -la gives the + // human-readable listing of the path and its parent (so we can see + // both "what's in here" and "how the parent is laid out" — useful for + // NFS-root-vs-subdir permission mismatches, the exact issue in + // session 1e9c7691). + cmd := fmt.Sprintf( + `echo "=== mount ==="; mount 2>/dev/null | grep -- "%[1]s" || echo "(not a mount point)"; +echo "=== df ==="; df -h "%[1]s" 2>&1 || true; +echo "=== stat ==="; stat -c '%%a %%U:%%G (size=%%s, type=%%F)' "%[1]s" 2>&1 || true; +echo "=== ls -la path ==="; ls -la "%[1]s" 2>&1 | head -40 || true; +echo "=== ls -la parent ==="; ls -la "$(dirname "%[1]s")" 2>&1 | head -20 || true`, + path) + out, xerr := sshExec(ctx, host, user, wrap(cmd)) + if xerr != nil { + return map[string]any{"ok": false, "error": fmt.Sprintf("ssh: %v: %s", xerr, out)} + } + // Truncate per-target output to keep an 8-target call's total under + // ~32KB. 4KB per target is enough for the head -40/head -20 listings + // above; if a directory is enormous, the truncation keeps the result + // usable without flooding the model's context. + const maxPerTarget = 4096 + if len(out) > maxPerTarget { + out = out[:maxPerTarget] + fmt.Sprintf("\n...truncated (%d bytes total)", len(out)) + } + return map[string]any{"ok": true, "output": out} +} diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index cf7092b..7767a6b 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -314,9 +314,9 @@ func allTools(pool *db.Pool, agentID uuid.UUID) []toolReg { // for future runbook extraction — especially pct_create DNS/VMID logic. // DO NOT re-register this tool. See plans/2026-07-10-general-gated-execution.md. - {tool: &mcp.Tool{Name: "run", Description: "Run ANY shell command against any host or LXC. This is the general execution primitive — prefer it over asking the operator to run something manually, and don't wait for a matching fixed action to exist. Every command is automatically risk-classified: read-only inspection (cat, systemctl status, docker ps, journalctl, df, git status, ...) runs immediately; anything that changes state requires operator approval (granted by the operator replying \"go ahead\"/\"yes\" in chat, or via the Approve button); commands matching a destructive pattern (rm -rf, dd, mkfs, pct/qm destroy, DROP TABLE, reboot, piping curl into a shell, ...) always require approval regardless of what you declare. You cannot talk your way past the destructive check by declaring a lower risk.", + {tool: &mcp.Tool{Name: "run", Description: "Run ANY shell command against any host, LXC, or VM. This is the general execution primitive — prefer it over asking the operator to run something manually, and don't wait for a matching fixed action to exist. Every command is automatically risk-classified: read-only inspection (cat, systemctl status, docker ps, journalctl, df, git status, ...) runs immediately; anything that changes state requires operator approval (granted by the operator replying \"go ahead\"/\"yes\" in chat, or via the Approve button); commands matching a destructive pattern (rm -rf, dd, mkfs, pct/qm destroy, DROP TABLE, reboot, piping curl into a shell, ...) always require approval regardless of what you declare. You cannot talk your way past the destructive check by declaring a lower risk.", InputSchema: objSchema( - prop{"target", "string", "Target entity slug: host: (e.g. host:strong) or lxc: (e.g. lxc:caddy). LXC commands run via pct exec on its Proxmox host automatically."}, + prop{"target", "string", "Target entity slug: host: (e.g. host:strong), lxc: (e.g. lxc:caddy), or vm: (e.g. vm:zimaos). LXC commands run via pct exec on their Proxmox host automatically. VM commands run via qm guest exec on their Proxmox host (requires the QEMU guest agent inside the VM — standard for Proxmox VMs)."}, prop{"command", "string", "The shell command to run. Can be a full script (multi-line, &&-chained). Runs as root."}, prop{"purpose", "string", "One sentence: why you're running this. Shown to the operator alongside the approval — be specific, this is what they're approving."}, prop{"declared_risk", "string", "Optional self-assessment: read_only, reversible_low, config_mutation, or destructive. This can only ESCALATE the automatic classification, never lower it — declaring a mutating command as read_only has no effect."}, @@ -340,6 +340,44 @@ func allTools(pool *db.Pool, agentID uuid.UUID) []toolReg { return classifyAndGate(ctx, pool, agentID, targetID, targetSlug, command, purpose, declaredRisk, sessionID), nil }}, + // inspect_path is the bulk fact-gathering tool from + // plans/2026-07-18-session-review-three-sessions.md P1.5. + // Sessions 1e9c7691 and 55927f0a each spent ~15 `run` calls + // gathering identical facts (`mount | grep`, `df`, `ls -la`, + // `stat`) across hosts and LXCs to understand where a path + // lives, who mounts it, and what permissions it has. This tool + // collapses that fan-out into one call: pass a path and a list + // of targets, get back per-target mount/df/ls/stat output as + // JSON. All commands are read-only, so no approval is needed. + {tool: &mcp.Tool{Name: "inspect_path", Description: "Bulk fact-gathering: run mount/df/ls/stat for the same path across multiple host/LXC/VM targets in ONE call. Returns a JSON object keyed by target slug, each with the target's view of the path (mount source, filesystem, size, top-level entries with ownership/permissions). Use this instead of N separate `run` calls when you need to understand a path's footprint across the fleet (e.g. tracing where a volume is mounted, checking permissions on the same NFS path from server + client). All commands are read-only — no approval needed.", + InputSchema: objSchema( + prop{"path", "string", "Absolute path to inspect on each target (e.g. /mnt/media_local, /media/ludo-library)."}, + prop{"targets", "array", "List of target entity slugs (host:strong, lxc:nfs-export, vm:zimaos, …). Up to 8 per call."}, + ), + }, handler: func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + args := argsMap(req) + path, _ := args["path"].(string) + if path == "" { + return textResult("error: path is required"), nil + } + rawTargets, _ := args["targets"].([]any) + if len(rawTargets) == 0 { + return textResult("error: at least one target is required"), nil + } + if len(rawTargets) > 8 { + return textResult("error: at most 8 targets per inspect_path call (use two calls if you need more)"), nil + } + targets := make([]string, 0, len(rawTargets)) + for _, t := range rawTargets { + if s, ok := t.(string); ok && s != "" { + targets = append(targets, s) + } + } + results := inspectPathAcrossTargets(ctx, pool, path, targets) + out, _ := json.MarshalIndent(results, "", " ") + return textResult(string(out)), nil + }}, + {tool: &mcp.Tool{Name: "http_get", Description: "Fetch a public web page or raw file (e.g. a GitHub README/raw URL) and return sanitized text. Use this to research how to deploy a service before provisioning. HTTP/HTTPS only; body is truncated to ~16KB.", InputSchema: objSchema( prop{"url", "string", "Absolute http(s) URL to fetch"}, diff --git a/nomos/SOUL.md b/nomos/SOUL.md index bf71365..127ba52 100644 --- a/nomos/SOUL.md +++ b/nomos/SOUL.md @@ -336,6 +336,51 @@ port is busy, find a free one. Only surface to the operator if you've tried reasonable alternatives and none worked. An error in one step is not a reason to stop the entire turn — it's a reason to try a different approach. +**A hung command is not a failed command — investigate before retrying.** +If a `run` call times out or returns "ERROR" (e.g. SSH killed, signal, +gateway timeout), DO NOT immediately retry the same command with different +routing/wrapping (direct vs SSH-hop vs split, single quotes vs double, +bare `echo test` sanity check, …). That piles up zombie processes on the +target and burns tool calls. Instead, BEFORE retrying the original +command, run read-only diagnostics against the same target to understand +*why* it hung: + +- `ps aux | grep ` — are there already-zombie copies piling up? +- `lsof ` — is something holding the file/dir open? +- `strace -f -p ` or `timeout 5 strace -f ` — what syscall is + it stuck on? (e.g. `fchownat` blocking = kernel-level lock) +- `mount | grep `, `dmesg | tail` — is a filesystem / kernel + subsystem involved? +- `exportfs -v`, `ss -tn`, `systemctl status ` — service-level + state that could block. + +Once you understand the blocker, fix it with a different command (e.g. +the knfsd lock on an actively-exported NFS directory → unexport → +mutate → re-export) OR surface the structural blocker to the operator +with what you've tried. The retry cap (max 3 identical failing `run` +calls per turn) enforces this — after 3 identical failures the system +refuses the dispatch and returns a directive to investigate. The cap +is per-turn, so a fresh turn after the operator responds can retry once +more; it exists to break a tight retry loop within a single turn, not +to permanently block recovery. + +**Ask before proposing a multi-step migration.** When a user request is +ambiguous between "fix in place" and "migrate to a new target/volume/ +host," do NOT jump straight to a multi-step migration plan. Use +`ask_operator` with one clarifying question ("fix in place, or migrate?") +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. + +**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 +`complete_task` summary should reference the arc of the whole session +(starting goal → pivot → final outcome), not just the last goal. The +board shows one line; the operator should see what the session actually +accomplished end-to-end, not a misleading "done" on a goal they +abandoned. + **Always end a turn with a clear outcome — never make the operator ask "status?".** When you finish (or pause) a piece of work, your final message must state the result plainly: what's now true, what you verified, what (if diff --git a/plans/2026-07-18-session-review-three-sessions.md b/plans/2026-07-18-session-review-three-sessions.md new file mode 100644 index 0000000..bdf2a2b --- /dev/null +++ b/plans/2026-07-18-session-review-three-sessions.md @@ -0,0 +1,414 @@ +# 2026-07-18 — Session review (three recent sessions) + +**Status:** Implemented — P0.1, P0.2, P1.3, P1.4, P1.5, P1.6, P1.8, P2.10 +landed in v0.7.12. P1.7 and P2.9 deferred (retry cap addresses the same +symptom at lower cost); see "Deferred" section at the bottom. +**Updated:** 2026-07-18 — session 1 continued after initial audit; outcome +upgraded from ⚠️ partial to ✅ success, root cause revised (knfsd kernel +lock, not gateway timeout). Implementation landed same day. + +Review of the last three Nomos sessions against the protocol in +`.agents/skills/session-review/SKILL.md`. Data pulled from the local +sessions API (`http://localhost:8092/sessions`). + +--- + +## Session 1 — `1e9c7691` (2026-07-18T11:28) + +**"Diagnose and fix ZimaOS folder move/delete failures on ludo-library"** + +| Metric | Value | +|---|---| +| Messages | 13 (6 user / 7 assistant) | +| Tool calls | 97 across 7 turns | +| Top tools | `run` ×58, `update_plan_step` ×7, `get_execution_status` ×7, `search_knowledge` ×3, `list_entities` ×3, `get_entity` ×3, `whoami` ×2 | +| Objective | Diagnose and fix ZimaOS folder move/delete failures on the ludo-library NFS mount | +| Outcome | ✅ success — root cause found and fix applied; verified from ZimaOS | +| Severity | friction | + +### What worked +- Root-cause analysis was fast and correct at the **NFS permissions** layer: + the export on `strong` uses `all_squash,anonuid=33,anongid=10000`, mapping + every NFS client to `www-data:media`. The export root `/mnt/media_local` + was owned `root:root / 755` while subdirs were `2775 media`. Subdir-level + ops worked, root-level (rename/delete top-level entries) failed. +- After the user prompted "the command just keeps running? does not + complete," the agent dug deeper and found the **real root cause**: + `knfsd` (kernel NFS server) holds a lock on actively-exported directories, + causing `chown` to hang indefinitely at the `fchownat()` syscall. + `strace -f chown :10000 /mnt/media_local` confirmed the hang point. + 25+ zombie `chgrp`/`chown` processes had piled up from the session's + repeated attempts. +- The correct fix sequence was identified and applied: `killall -9 chgrp + chown` to clear zombies, then unexport → `chown :10000` + `chmod 2775` + → re-export. Routed via SSH-hop from `host:hubris` (the `host:strong` + direct path kept timing out because the commands genuinely hang, not + because of a network issue). +- Verification was done from the client side: `touch`, `mv`, `rm`, + `mkdir`, `rmdir` all confirmed working at the NFS root from ZimaOS. +- Knowledge writeback was good: `upsert_knowledge` recorded an + `investigation` linked to `vm:zimaos`, `host:strong`, `pool:ludo-lvm`, + with the fix. `complete_task` was called with a clear summary. +- Plan lifecycle was followed: `set_goal` → `propose_plan` → + `update_plan_step` (running/done) → `complete_task`. + +### What didn't +- **20+ blind retries before investigating why.** The agent retried the + same one-line `chown`/`chmod` roughly 20 times across direct runs, + SSH-hop-via-hubris, wrapping in a shell script, splitting into smaller + commands, and bare `echo test` sanity checks — all hung. Each retry + piled up another zombie process on `strong`. The agent only + investigated *why* the command hung after the user explicitly asked + "the command just keeps running?" +- **Misdiagnosed the timeout as a gateway/network issue.** The agent's + own narrative said "API seems to be struggling with timeouts," "API + keeps timing out on strong," "Strong mutations are consistently timing + out — read-only works." This framed the problem as the control plane, + when in fact the commands were genuinely hanging at the kernel level + on the target host. A `strace` on the first failure would have + revealed this immediately. +- **Approval window kept expiring between retries.** User had to say "go + ahead" twice and "proceed" + "status" once each because the assent + window closed while the agent was looping on the hung commands. +- **No back-off / cap on retries.** 58 `run` calls in 7 turns, of which + ~20 are essentially the same `chown :10000 /mnt/media_local && chmod + 2775 …`. Once a command has timed out 3× in a row, the agent should + stop retrying and investigate *or* surface the blocker to the operator. + +### Fixes needed +- (friction) **Retry cap + "investigate before retry" rule.** In + `cmd/nomos/agent.go`, hash each outgoing `run` command; if the same + hash has failed 3× in the session, refuse to issue it again. Force the + agent to either change approach (e.g. `strace`, `ps`, `lsof` to see + *why*) or surface the blocker to the operator. This single change + would have turned session 1 from 58 `run` calls into ~8 and produced + the knfsd finding on the first failure instead of the 20th. +- (friction) **SOUL.md guidance: a hung command is not a failed command.** + When a `run` times out, the agent's first instinct should be to + inspect the target (`ps aux | grep `, `strace -f -p `, + `lsof `) — not to retry the same command. The current default + (retry with different routing/wrapping) wasted 20 calls. +- (friction) **Capture the unexport → mutate → re-export pattern as a + runbook.** "Mutating an actively-exported NFS directory hangs at + `fchownat()`" is a reusable finding. It belongs as a `runbook` entity + linked to `host:strong` / `lxc:nfs-export` so the next time someone + needs to chown/chmod an exported path, the agent finds it via + `get_entity_knowledge` and unexports first. +- (friction) Approval window robustness: when an execution times out, + extend the assent window for the same plan step automatically — the + operator already approved it; we shouldn't make them re-approve + because *our* command hung. + +--- + +## Session 2 — `55927f0a` (2026-07-18T09:45) + +**"Add NFS export of ludo-lvm (/mnt/library) from strong to ZimaOS, so ZimaOS +can see downloads/usenet/movies/ alongside the existing old-library NFS mount"** + +| Metric | Value | +|---|---| +| Messages | 25 (12 user / 13 assistant) | +| Tool calls | 108 across 13 turns | +| Top tools | `run` ×49, `update_plan_step` ×13, `get_entity` ×10, `list_entities` ×6, `update_entity_attributes` ×3, `search_knowledge` ×3, `propose_plan` ×3, `get_relations` ×3, `get_knowledge_content` ×3, `get_execution_status` ×3, `upsert_knowledge` ×2, `set_goal` ×2, `create_relationship` ×2 | +| Objective | Originally: fix sabnzbd download folder to use ludo-lvm. Pivoted to: add NFS export of ludo-lvm to ZimaOS. Final outcome: just keep ludo-library (drop redundant /media/media mount) | +| Outcome | ✅ success — ZimaOS reduced to two clean tiles, fstab cleaned, knowledge + entity attrs written back | +| Severity | friction | + +### What worked +- Writeback was thorough: `upsert_knowledge` ×2, `update_entity_attributes` + on `vm:zimaos` and `lxc:nfs-export`, `create_relationship` ×2. The + knowledge graph is current. +- The final cleanup was small and safe: unmount `/media/media` on ZimaOS, + remove the fstab entry, `rmdir` the empty directory, clear CasaOS caching + artifacts. Each step got its own `run` with a clear result. +- Agent correctly noticed the pivot: "wait — `/media/media` IS ludo-lvm + too, just via a double NFS hop through nfs-export. Redundant." That + insight is what turned a complex migration into a one-step cleanup. + +### What didn't +- **Goal pivots were not closed cleanly.** `set_goal` was called twice — + once for the sabnzbd fix, once for the NFS export. The first goal was + implicitly abandoned when the user said "lets just keep ludo-library + then"; there's no `complete_task` for it. If the session state is keyed + on the latest `set_goal`, the first goal is orphaned in the UI. +- **Excessive fan-out on `run`.** 49 `run` calls, many of which repeat the + same diagnostic (`mount | grep`, `cat /etc/exports`, `exportfs -v`, + `ls -la /mnt/...`) across `lxc:arriman`, `lxc:jellyfin`, `lxc:nfs-export`, + `host:hubris`, `host:strong`. A single bulk "inventory this path across + these targets" tool would have collapsed 15+ runs into 1. +- **`vm:` targets aren't directly runnable.** Every ZimaOS command had to + be `ssh -o StrictHostKeyChecking=no root@192.168.8.195 '…'` from + `host:hubris`. Nested shell quoting broke once and the agent had to + re-escape. This was called out in the 2026-07-14 review and is still + open. +- **Agent over-scoped before checking with the user.** After "explain how + the migration would work", the agent produced a full multi-LXC migration + plan (move `/dev/mapper/library-library` consumers off the old volume, + migrate ZimaOS NFS export, etc.). The user replied "lets just keep + ludo-library then." A clarifying question — "do you want to migrate, or + just clean up the redundant mount?" — would have saved 4 turns. +- **Approval friction.** One execution came back with "status=cancelled, + but the assent window for this session is not active. The agent will not + auto-continue. Reply 'continue' or re-approve the plan to resume." User + had to type "proceed" to resume. This is the same assent-window-expiry + pattern from session 1. + +### Fixes needed +- (friction) Track `set_goal` history per session. When a new goal is set, + the previous one should be auto-marked `complete` (or `superseded`) so + the UI doesn't show an orphaned active goal. +- (friction) Add a bulk inspection tool — e.g. `inspect_path(path, targets)` + that returns `mount`, `df`, `ls -la`, and ownership for the same path + across multiple entities in one call. Sessions like this routinely spend + 15+ `run` calls gathering the same facts across hosts. +- (friction) `vm:` target support in `run`. Either expose a `qm guest exec` + wrapper or accept `vm:` as a target and route through the host. The + manual SSH-hop pattern is error-prone (nested quoting) and slow. +- (friction) SOUL.md guidance: before proposing a multi-step migration + plan, ask the user "migrate or clean up?" when both are plausible from + the prompt. This was a single extra clarification question away from a + 4-turn savings. + +--- + +## Session 3 — `2926de4e` (2026-07-15T22:22) + +**"Deploy apt updates to host:netbird-vps — 92 pending packages including +Docker CE, netbird, kernel, ZFS, and security patches."** + +| Metric | Value | +|---|---| +| Messages | 9 (4 user / 5 assistant) | +| Tool calls | 27 across 5 turns | +| Top tools | `update_plan_step` ×7, `run` ×6, `set_goal` ×2, `search_knowledge` ×2, `complete_task` ×2, `upsert_knowledge` ×1, `update_entity_attributes` ×1, `propose_plan` ×1, `list_lxcs` ×1, `get_relations` ×1, `get_knowledge_content` ×1, `get_entity` ×1, `get_execution_status` ×1 | +| Objective | Two-phase: (a) fleet-wide update audit by criticality, (b) deploy the 92-package upgrade to host:netbird-vps | +| Outcome | ✅ success — 92→0 packages pending; netbird-mgmt OIDC race caught and fixed; knowledge + entity attrs written back | +| Severity | cosmetic | + +### What worked +- **Two goals, two clean lifecycles.** `set_goal` → `propose_plan` → + `update_plan_step` (running/done) → `complete_task` ran twice, once for + the audit and once for the upgrade. The session is the model for how + multi-goal sessions should look. +- **Pre-existing knowledge reuse.** First `search_knowledge` found a + today-dated audit; agent used `get_knowledge_content` and presented it + without needing any `run` for the audit half. Zero wasted tool calls. +- **Long-running upgrade handled correctly.** The 92-package `apt upgrade` + hit the HTTP gateway timeout mid-run. Agent didn't retry it — it called + `get_execution_status` and then ran a verification `run` + (`apt list --upgradable | wc -l`, `uname -r`, `docker ps`) to confirm + completion server-side despite the timeout. This is the right pattern; + session 1 should have done the same. +- **Gotcha caught.** After the upgrade, `docker logs netbird-mgmt` + revealed the management container was crash-looping because it tried to + fetch OIDC config from `auth.hubris.network` before traefik/authentik + were ready. Fix: `docker restart netbird-mgmt` after ~30s. Captured in + `upsert_knowledge` as an `investigation` tagged `apt, upgrade, netbird, + docker, gotcha` linked to `host:netbird-vps`. +- `update_entity_attributes` was called on `host:netbird-vps` to record the + new kernel version. Good writeback hygiene. + +### What didn't +- (cosmetic) The HTTP timeout on long-running upgrades surfaced as a + transient error to the operator. The agent handled it correctly but the + UX would be cleaner if `run` returned `PENDING` immediately for known + long-running command patterns (`apt upgrade`, `pct migrate`, `rclone + sync`, etc.) instead of timing out at the gateway. +- (cosmetic) Two `complete_task` calls in one session produced two "task + complete" bubbles. Fine, but the second one could have noted the + first-task outcome as well in its summary so the chat reads as one + coherent arc. + +### Fixes needed +- (cosmetic) Long-running command detection in `run`: if the command + matches a known-long pattern, return a `PENDING` execution id with a + hint to poll `get_execution_status`, rather than blocking at the HTTP + layer for 30s and timing out. Session 3 already proved the + poll-after-timeout pattern works — make it the default for these + commands. +- (cosmetic) Encourage the agent to fold the prior task's outcome into + the next `complete_task` summary when a session has multiple goals. + +--- + +## Cross-session patterns + +| # | Pattern | Sessions | Severity | +|---|---|---|---| +| 1 | Agent retries hung commands 20× before investigating *why* | 1 | friction | +| 2 | Approval window expires between turns forcing re-approval | 1, 2 | friction | +| 3 | `vm:` targets not directly runnable — must SSH-hop via `host:hubris` | 1, 2 | friction | +| 4 | N+1 fan-out on `run` for cross-entity fact-gathering | 1, 2 | friction | +| 5 | No retry cap — agent retries identical failing `run` 10–20× | 1 | friction | +| 6 | Goal pivots not closed (`set_goal` called twice without closing prior) | 2 | friction | +| 7 | Long-running commands hit HTTP timeout instead of returning PENDING | 3 | cosmetic | +| 8 | Agent over-scopes migration plans before checking intent | 2 | friction | +| 9 | Reusable operational gotchas (knfsd lock, OIDC race) captured as investigations, not runbooks | 1, 3 | friction | + +**What consistently works well** +- Plan lifecycle: `set_goal` → `propose_plan` → `update_plan_step` → + `complete_task` is now followed in all three sessions. +- Knowledge writeback: `upsert_knowledge`, `update_entity_attributes`, + `create_relationship` are used in every session. The graph is kept + current. +- Root-cause analysis quality is high once the agent digs in (NFS + all_squash + root dir perms → knfsd fchownat hang; double NFS hop; + OIDC race condition). The problem is getting the agent to dig in + *before* the 20th retry. + +**What consistently breaks** +- **Hung commands get retried instead of investigated.** Session 1's + `chown` was blocked by knfsd for 30+ minutes while the agent retried + with different routing/wrapping. Session 3's `apt upgrade` timed out + and the agent correctly polled — but that's the exception, not the + rule. The default behavior is "retry the same thing differently." +- Approval window lifetime vs. agent retry loops — when execution times + out, the assent window lapses and the operator has to re-approve even + though the *intent* was never withdrawn. +- Reusable operational fixes (unexport → mutate → re-export for NFS + dirs; `docker restart netbird-mgmt` after stack upgrade) get recorded + as `investigation` entities. They should be `runbook` entities so the + agent finds them via `get_entity_knowledge` next time and applies the + procedure instead of rediscovering it. + +--- + +## Improvement plan + +### P0 — Friction (was blocker; downgraded after session 1 resolved) + +1. **Retry cap + "investigate before retry" rule.** In + `cmd/nomos/agent.go`, hash each outgoing `run` command; if the same + hash has failed 3× in the session, refuse to issue it again. Force + the agent to either change approach (e.g. `strace`, `ps aux | grep`, + `lsof` to see *why*) or surface the blocker to the operator. This + single change would have turned session 1 from 58 `run` calls into + ~8 and produced the knfsd finding on the first failure instead of + the 20th. +2. **SOUL.md guidance: a hung command is not a failed command.** When a + `run` times out, the agent's first instinct should be to inspect the + target (`ps aux | grep `, `strace -f -p `, `lsof `) + — not to retry the same command with different routing/wrapping. The + current default wasted 20 calls in session 1. + +### P1 — Friction + +3. **Capture operational gotchas as `runbook` entities, not just + `investigation`.** Two candidates from these sessions: + - **"Mutating an actively-exported NFS directory hangs at + `fchownat()`"** — procedure: `killall -9 chgrp chown` → + `exportfs -u :` → `chown`/`chmod` → `exportfs -a`. + Linked to `host:strong`, `lxc:nfs-export`. + - **"netbird-mgmt crash-loops after stack upgrade"** — procedure: + wait ~30s for traefik/authentik to come up, then + `docker restart netbird-mgmt`. Linked to `host:netbird-vps`. + Today both are `investigation` entries; the agent records them but + won't proactively apply them next time. +4. **Auto-close prior `set_goal` when a new one is set.** Mark the + previous goal `superseded` and emit a synthetic `complete_task` + summary so the UI doesn't show an orphaned active goal. (Session 2 + had this.) +5. **Bulk inspection tool.** Add an MCP tool like + `inspect_path(path, targets[])` that runs `mount | grep`, `df`, + `ls -la`, and `stat` against a list of entity slugs in one call. + Sessions 1 and 2 each spent ~15 `run` calls gathering identical + facts across hosts/LXCs. +6. **`vm:` target support in `run`.** Accept `vm:` as a target + and route via `qm guest exec` on the host that owns the VM. + Eliminates the nested-quoting SSH-hop pattern that broke once in + session 2 and required manual SSH-hop workarounds in session 1. +7. **Approval window robustness.** When an execution times out, extend + the assent window for the same plan step automatically — the + operator already approved it; we shouldn't make them re-approve + because *our* command hung. Affects sessions 1 and 2. +8. **SOUL.md guidance: ask-before-migrating.** When a user request is + ambiguous between "fix in place" and "migrate," the agent should + ask one clarifying question before producing a multi-step migration + plan. Session 2 would have saved ~4 turns. + +### P2 — Cosmetic + +9. **Long-running command detection.** Maintain a small regex list + (`apt (upgrade|install)`, `pct migrate`, `rclone (sync|copy)`, + `dd if=`, `docker compose pull`) for commands that are known to + exceed 30s. Return `PENDING` immediately with an `execution_id` + instead of blocking at the gateway. Session 3 already uses the + poll pattern — make it the default. +10. **Multi-goal `complete_task` summaries.** When a session has more + than one `set_goal`, the final `complete_task` summary should + reference the arc of the whole session, not just the last goal. + +--- + +## Revised note on the original P0 + +The original P0 ("Diagnose `host:strong` config_mutation timeouts — +suspect SSH latency / mesh routing, raise timeout") was **wrong**. The +timeouts were not a gateway or network issue — the commands were +genuinely hanging at the kernel level because `knfsd` holds a lock on +actively-exported directories. Raising the HTTP timeout would not have +helped; the `chown` would simply hang longer. The real fix is (a) the +retry-cap/investigate-before-retry rule (P0.1 above) and (b) the +unexport → mutate → re-export runbook (P1.3). + +--- + +## Deferred + +**P1.7 — Approval window auto-extends on execution timeout.** The assent +window lives in `autonomy_settings` and is read by `classifyAndGate` +(`internal/mcp/server.go:607`); timeout detection lives in `sshExec` +(`internal/mcp/server.go:332`). Wiring them requires the SSH-execution +path to signal back into the approval-state machine across the nomos ↔ +api process boundary, and a future implementation needs to distinguish +"command genuinely hung" (knfsd case — don't extend, the command is +stuck) from "command is long-running" (apt upgrade — extend). Without +that distinction, auto-extending on every timeout would mask real hang +symptoms — exactly the misdiagnosis session 1 made. **The retry cap +(P0.1) addresses the same symptom at lower cost**: after 3 failures +the agent is forced to investigate or surface, which removes the +cascading retry storm that made the assent expiry visible in the first +place. Revisit if future sessions show the operator re-approving a +plan they never withdrew in intent (not just retrying a hung command). + +**P2.9 — Long-running command PENDING detection.** A regex list of +known-long commands (`apt (upgrade|install)`, `pct migrate`, `rclone +(sync|copy)`, `dd if=`, `docker compose pull`) so `run` returns +`PENDING` immediately with an `execution_id` instead of blocking at +the HTTP gateway for 30s and timing out. **Session 3 already proved +the current poll pattern works:** the `apt upgrade` timed out at the +gateway, the agent called `get_execution_status`, then ran a +verification `run` (`apt list --upgradable | wc -l`, `uname -r`, +`docker ps`) — clean 92→0 packages result. The agent did the right +thing without any new machinery, and the retry cap (P0.1) protects +against the failure mode of this path (blind retry on timeout). +Implementing PENDING detection well requires a classifier extension +(`internal/policy`) plus a new return shape from `classifyAndGate` +that the agent loop has to learn to handle (poll instead of retry) — +a real protocol change, not a small fix. Worth doing if the +poll-after-timeout pattern proves fragile over the next few sessions; +not worth doing speculatively right now. + +--- + +## Verification commands + +```bash +# Re-pull any session for follow-up +curl -s http://localhost:8092/sessions/1e9c7691-5815-48d1-acb4-91a6a39691c9 | jq . +curl -s http://localhost:8092/sessions/55927f0a-597e-4561-aaef-077623051432 | jq . +curl -s http://localhost:8092/sessions/2926de4e-0b73-4c3d-a2cd-ee9a42089b46 | jq . + +# Confirm host:strong mutation timeout reproduces +curl -s http://localhost:8092/sessions | jq -r '.sessions[].id' | head -1 # latest session id +``` + +## Related files + +- `cmd/nomos/agent.go` — agent loop, retry behavior, goal state +- `cmd/nomos/store.go` — `set_goal` / `complete_task` persistence +- `internal/mcp/server.go` — `run` tool, timeout handling, `get_execution_status` +- `internal/httpapi/server.go` — HTTP gateway timeout for mutations +- `nomos/SOUL.md` — agent persona, ask-before-migrate guidance candidate +- `plans/2026-07-14-activity-gaps.md` — prior session review (same patterns recurring) diff --git a/plans/index.md b/plans/index.md index b3db7f7..c96dac5 100644 --- a/plans/index.md +++ b/plans/index.md @@ -17,6 +17,7 @@ went sideways, open an investigation. | 2026-07-14 | [Activity gaps](2026-07-14-activity-gaps.md) | In Progress | | 2026-07-14 | [Activity timeline](2026-07-14-activity-timeline.md) | In Progress | | 2026-07-17 | [Codebase review, lint audit, and documentation maintenance](2026-07-17-codebase-review-and-cleanup.md) | Report delivered — doc/tooling fixes applied; code refactors pending | +| 2026-07-18 | [Session review: three recent sessions](2026-07-18-session-review-three-sessions.md) | Implemented in v0.7.12 — P0.1/P0.2/P1.3/P1.4/P1.5/P1.6/P1.8/P2.10; P1.7 and P2.9 deferred (retry cap covers) | ## Done diff --git a/seeds/knowledge.yaml b/seeds/knowledge.yaml index 7a6d553..6e1fec4 100644 --- a/seeds/knowledge.yaml +++ b/seeds/knowledge.yaml @@ -6713,3 +6713,143 @@ runbooks: tags: - skill - runbook +- slug: nfs-exported-dir-mutation-hang + name: Mutating an actively-exported NFS directory hangs at fchownat + risk_class: config_mutation + entity_type: host + procedure: {} + content: | + --- + name: nfs-exported-dir-mutation-hang + risk_class: config_mutation + inputs: [target_host, exported_path, mutation_command] + verification: "stat -c '%a %U:%G' on the target host" + docs_update_checklist: [investigation_entry] + --- + + # Mutating an actively-exported NFS directory hangs at fchownat + + Symptom: a `chown` / `chgrp` / `chmod` against a directory that is + actively exported via `nfs-kernel-server` (knfsd) hangs indefinitely — the + command appears to run but never returns. SSH/gateway time out waiting for it. + `ps aux | grep chown` shows the process in interruptible sleep (D state); + repeated retries pile up zombies (25+ observed in one session). + + Root cause: knfsd holds a kernel lock on the directory while it's exported. + `fchownat()` blocks waiting for the lock. This is NOT a gateway or SSH issue — + raising the timeout just makes the hang longer. + + ## Procedure + + 1. **Clear the zombies** from prior failed attempts: + ``` + killall -9 chgrp chown chmod 2>/dev/null + ``` + 2. **Temporarily unexport** the path for each client/network that has it exported: + ``` + exportfs -u : # e.g. exportfs -u 192.168.8.0/24:/mnt/media_local + ``` + 3. **Apply the mutation** (now that knfsd has released the lock): + ``` + chown : && chmod + ``` + 4. **Re-export** (restore the exports): + ``` + exportfs -a + ``` + 5. **Verify** from an NFS client that the new permissions are visible and operations work end-to-end: + ``` + stat -c '%a %U:%G' /media/ # on a client + touch /media//.test && mv /media//.test /media//.moved && rm /media//.moved + ``` + + ## Detection signature (for agents) + + A `run` call against a host that contains `chown|chgrp|chmod` of a path + exported by `nfs-kernel-server` AND the call times out → assume this runbook. + Don't retry the same command; run `strace -f ` (it will block at + `fchownat`) to confirm, then apply the procedure above. + + ## Caveats + + - `exportfs -u` may emit a format-mismatch warning if the export was defined + via `/etc/exports` with a different option string than `exportfs -v` + reports. The unexport still succeeds; verify with `exportfs -v` afterward + that the path is gone, then re-add with `exportfs -a`. + - This applies to ANY mutating op on the exported dir (chown, chmod, rename, + rmdir of the root). Subdirectory mutations are fine as long as they don't + touch the exported root itself. + + Recorded after session 1e9c7691 (2026-07-18) — 20+ retries of a + `chown :10000 /mnt/media_local` that hung for 30+ minutes before this + procedure was identified. + tags: + - runbook + - nfs + - knfsd + - gotcha +- slug: netbird-mgmt-oidc-race-after-upgrade + name: netbird-mgmt crash-loops after stack upgrade (OIDC race) + risk_class: reversible_low + entity_type: host + procedure: {} + content: | + --- + name: netbird-mgmt-oidc-race-after-upgrade + risk_class: reversible_low + inputs: [] + verification: "docker ps --filter name=netbird-mgmt --format '{{.Status}}' shows Up" + docs_update_checklist: [investigation_entry] + --- + + # netbird-mgmt crash-loops after stack upgrade (OIDC race) + + Symptom: after a full-stack restart on `host:netbird-vps` (e.g. following an + apt upgrade that touched Docker, traefik, authentik, or the netbird + packages), `netbird-mgmt` enters a crash loop. `docker logs netbird-mgmt + --tail 30` shows repeated failed attempts to fetch OIDC config from + `auth.hubris.network` (connection refused / i/o timeout). + + Root cause: startup ordering race. `netbird-mgmt` tries to fetch its OIDC + configuration from `auth.hubris.network` before traefik and authentik are + ready to serve. Connection refused → mgmt exits → docker restarts it → + same failure. + + ## Procedure + + 1. Confirm the race (not a real config breakage): + ``` + docker logs netbird-mgmt --tail 30 2>&1 | grep -E 'auth.hubris.network|OIDC|connection refused' + curl -fsS -o /dev/null -w '%{http_code}' https://auth.hubris.network/application/o/netbird/.well-known/openid-configuration + ``` + If the curl now returns 200, the race has already self-resolved — just restart mgmt. + 2. Wait ~30s for traefik + authentik to finish coming up. + 3. Restart just the management container: + ``` + docker restart netbird-mgmt + ``` + 4. Verify: + ``` + docker ps --filter name=netbird-mgmt --format '{{.Names}} {{.Status}}' + docker logs netbird-mgmt --tail 10 2>&1 # should show clean startup, no OIDC errors + ``` + 5. Check the rest of the stack is healthy too: + ``` + docker ps --format 'table {{.Names}}\t{{.Status}}' + ``` + + ## Detection signature (for agents) + + After a `run` that upgraded anything Docker/traefik/authentik/netbird on + `host:netbird-vps`, run `docker ps` and `docker logs netbird-mgmt --tail 30`. + If mgmt is Restarting + logs mention auth.hubris.network OIDC fetch failure, + apply this procedure before declaring the upgrade complete. + + Recorded after session 2926de4e (2026-07-15) — 92-package apt upgrade on + netbird-vps; mgmt crash-loop caught and fixed with `docker restart + netbird-mgmt` after ~30s. + tags: + - runbook + - netbird + - docker + - gotcha