feat(nomos): retry cap, vm: targets, inspect_path, goal supersession, runbooks
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.
This commit is contained in:
@@ -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 <target>: 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.
|
||||
|
||||
170
cmd/nomos/retrycap.go
Normal file
170
cmd/nomos/retrycap.go
Normal file
@@ -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 <target>: 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 <cmd>`, " +
|
||||
"`lsof <path>`, `strace -f -p <pid>` or `strace -f <cmd>`, " +
|
||||
"`mount | grep <path>`, `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:])
|
||||
}
|
||||
129
cmd/nomos/retrycap_test.go
Normal file
129
cmd/nomos/retrycap_test.go
Normal file
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 <pve_id> -- ...` for an LXC.
|
||||
// connection: identity for a host, `pct exec <pve_id> -- ...` for an LXC,
|
||||
// `qm guest exec <pve_id> -- ...` 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@<vm_ip> '...'`),
|
||||
// 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:<slug> or lxc:<slug>", 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 <id> -- /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:<slug>, lxc:<slug>, or vm:<slug>", 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
|
||||
@@ -995,3 +1064,82 @@ func createApproval(ctx context.Context, pool *db.Pool, execID, targetID uuid.UU
|
||||
_ = observability.Event(ctx, sqlcgen.New(pool), "approval.created", &execID, "warning", "mcp", "",
|
||||
map[string]any{"action": action, "params": params, "risk_class": riskClass})
|
||||
}
|
||||
|
||||
// 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}
|
||||
}
|
||||
|
||||
@@ -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:<slug> (e.g. host:strong) or lxc:<slug> (e.g. lxc:caddy). LXC commands run via pct exec on its Proxmox host automatically."},
|
||||
prop{"target", "string", "Target entity slug: host:<slug> (e.g. host:strong), lxc:<slug> (e.g. lxc:caddy), or vm:<slug> (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"},
|
||||
|
||||
@@ -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 <cmd>` — are there already-zombie copies piling up?
|
||||
- `lsof <path>` — is something holding the file/dir open?
|
||||
- `strace -f -p <pid>` or `timeout 5 strace -f <cmd>` — what syscall is
|
||||
it stuck on? (e.g. `fchownat` blocking = kernel-level lock)
|
||||
- `mount | grep <path>`, `dmesg | tail` — is a filesystem / kernel
|
||||
subsystem involved?
|
||||
- `exportfs -v`, `ss -tn`, `systemctl status <svc>` — 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
|
||||
|
||||
414
plans/2026-07-18-session-review-three-sessions.md
Normal file
414
plans/2026-07-18-session-review-three-sessions.md
Normal file
@@ -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 <cmd>`, `strace -f -p <pid>`,
|
||||
`lsof <path>`) — 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:<slug>` 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 <cmd>`, `strace -f -p <pid>`, `lsof <path>`)
|
||||
— 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 <client>:<path>` → `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:<slug>` 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)
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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' <exported_path> 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 <client>:<path> # e.g. exportfs -u 192.168.8.0/24:/mnt/media_local
|
||||
```
|
||||
3. **Apply the mutation** (now that knfsd has released the lock):
|
||||
```
|
||||
chown :<gid> <path> && chmod <mode> <path>
|
||||
```
|
||||
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/<mountpoint> # on a client
|
||||
touch /media/<mountpoint>/.test && mv /media/<mountpoint>/.test /media/<mountpoint>/.moved && rm /media/<mountpoint>/.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 <cmd>` (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
|
||||
|
||||
Reference in New Issue
Block a user