oikos/plans/2026-07-11-nomos-agent-code-review.md

2026-07-11 — Nomos agent code review: gaps and improvement plan

Status: Planned

Scope

A full read-through of cmd/nomos/ (agent.go, store.go, main.go, continue.go, assent.go, tasks.go — 3,120 lines) plus targeted checks of its HTTP exposure, goroutine safety, and test coverage. Every finding below is grounded in a specific file:line or a runnable reproduction — two of the sharper ones (A1, A2) were empirically confirmed with throwaway test probes before being written up, not just read and assumed.

This is a review, not an implementation — findings are ranked by severity with a proposed fix per item; nothing here has been changed yet.

---

A. Correctness bugs (confirmed, not theoretical)

A1. Chat-assent word matching has real substring false positives

assent.go:73-103. isAssent/isTypedConfirmation pad the message with spaces and word-boundary-check the negation list (strings.Contains(m, " "+w+" ")), but the assent/confirm checks use bare strings.Contains(m, w) — no word boundary at all. Confirmed live via a test probe:

• isAssent("not sure, maybe yesterday's logs show something useful") → true ("yes" matches inside "yesterday"; "not" alone isn't in negationWords, only the phrase "not yet" is).
• isTypedConfirmation("I haven't confirmed anything yet, let me think") → true ("confirm" matches inside "confirmed"; "haven't" isn't in negationWords, which only has "don't"/"do not", not other contracted negatives).

The second one is the serious half: isTypedConfirmation is the sole gate for DESTRUCTIVE actions (agent.go:220-223) — a message that merely mentions not having confirmed something yet can read as an explicit confirmation.

Fix: apply the same space-padded word-boundary check to the assent/confirm word lists that negation already uses. Expand negationWords to cover contracted negatives (haven't, hasn't, isn't, wasn't, can't, won't, not as a standalone word, not just "not yet"). Add both reproduced cases as permanent regression tests in assent_test.go.

A2. Unbounded conversation history replay — no windowing, no token budget

agent.go:185-207: every single turn (chatWith) calls a.store.getMessages(ctx, sessionID) — store.go:218-239, SELECT ... WHERE session_id=$1 ORDER BY created_at ASC with no LIMIT, no windowing, no summarization — and replays the entire history into the LLM call every time. truncateToolResults (store.go:152-185) caps each individual tool result at 4KB, but caps nothing else: not tool args, not the number of tool calls in one message, not the total message count, not total tokens.

This isn't theoretical — an earlier production audit (see chat-sessions-improvements) found a single turn with 70 tool calls and messages up to 106KB. Every subsequent turn of a long-running or heavily-autonomous task (exactly what auto-continuation is built for) re-sends that ever-growing history in full. This is a real cost, latency, and eventual context-length-limit risk that compounds specifically for the tasks the system is designed to run longest.

Fix: at minimum, cap replayed history to the most recent N messages or a token budget, with older turns either dropped or collapsed into a short system-message summary (finalSummary's existing one-shot summarization pattern, agent.go:481-492, could be reused for this). Needs a decision on where the cutoff lives (see open questions).

A3. A live turn's tool-call history is lost entirely if the client disconnects mid-stream

main.go handleChat: toolCalls/finalText accumulate only in local closure variables; st.saveMessage(...) runs exactly once, after a.chat(...) returns, using ctx := r.Context() — the same context that cancels the instant the client disconnects (Stop button, tab close, network blip). If a.chat returns early because that context was cancelled, the final saveMessage call runs with an already-cancelled context and its error return is never checked — the whole turn's tool-call history (already real: executions launched, knowledge possibly written) is silently lost from the persisted transcript.

Contrast with resumeSession/continueSession (continue.go:96-166), which insert a placeholder row immediately and update it after every single tool call — exactly the incremental-persistence pattern handleChat lacks. Verified live this session: my own Stop-button test showed the turn's actual tool calls (6 of them) were visible in the UI only because the SSE stream had already pushed them to the browser's in-memory store before the abort — none of that would have survived a page reload, since nothing was persisted.

Fix: bring handleChat in line with resumeSession's pattern — insert a placeholder row before the turn starts, update it after each tool call using a context not tied to the client connection for the write itself (or at minimum, persist with context.Background() in a deferred cleanup so a cancelled request context doesn't take the DB write down with it).

---

B. Robustness

B1. Zero panic recovery on any background goroutine

Every explicitly-spawned goroutine across the agent surface has no recover():

cmd/nomos/main.go:78   go nAgent.runContinuationWorker(ctx)
cmd/nomos/main.go:80   go func() { ...sweep ticker... }()
cmd/nomos/main.go:117  go func() { ...http server... }()
cmd/nomos/main.go:347  go a.resumeSession(context.Background(), sessionID, note)
internal/mcp/server.go:477,495  go executeApprovedViaAPI(...)
internal/mcp/server.go:1134     go func() { ... }()
internal/httpapi/phase3.go:119,1456
internal/httpapi/server.go:81,533

grep -rn "recover()" cmd/nomos/ internal/mcp/ internal/httpapi/ returns nothing. Go's default behavior for a panic in any goroutine — not just the one handling an HTTP request, which the stdlib does recover — is to crash the entire process. runContinuationWorker and resumeSession in particular run complex, unattended agent logic (JSON unmarshaling of model output, tool result parsing, map/slice indexing) with no operator watching; a single edge case (a malformed tool result, an unexpected nil) takes down nomos for every concurrently-running task, not just the one that hit it. This is more consequential post-concurrency (today's work): more simultaneous unattended goroutines running agent code means more surface area for one bad input to end everyone's session.

Fix: wrap every explicitly-spawned goroutine body in a defer func() { if r := recover(); r != nil { slog.Error(...) } }(). A small helper (safeGo(func())) would make this consistent and hard to forget at new call sites.

B2. Auto-continuation processes its batch sequentially, one full turn at a time

continue.go:58-75: processContinuations fetches up to 5 pending items and runs a.continueSession(ctx, p) for each in a plain for loop, in the single runContinuationWorker goroutine. Each continueSession is a full LLM turn that can run for minutes (10-minute timeout, continue.go:134). If 3 different tasks' executions finish in the same 4-second tick, task #3's continuation waits for #1 and #2 to completely finish first — undercutting today's whole concurrency effort specifically on the auto-continuation path, which is the mechanism autonomous multi-step tasks depend on most.

Fix: spawn each pending continuation as its own goroutine (with B1's panic recovery), bounded by a small semaphore if unbounded parallelism here is a concern.

B3. No terminal state for a permanently-failed auto-continuation

continue.go:162-165: if the resumed LLM call errors on both the initial attempt and its one retry, the code logs an error and returns — the task is left in whatever status it was in (typically executing), with no outcome set and no operator-visible signal beyond an inert message buried in the transcript. There's no give-up-after-N-retries or dead-letter marking; the task just looks silently stuck.

Fix: on final failure, call the same path complete_task would use to set outcome='failure' with a summary explaining the resume failed, so the task board reflects reality instead of showing a task that looks perpetually "executing."

---

C. Security

C1. Nomos's own HTTP gateway has zero authentication

docker-compose.yml:133 publishes port 8092 directly ("8092:8092", comment: "mesh-published") and Caddyfile.oikos reverse-proxies to it from two routes. grep -n "Authorization\|Bearer\|auth" cmd/nomos/main.go returns nothing — /chat, /sessions, /sessions/{id} (including DELETE), and /query have no credential check of any kind. Anyone who can reach the LAN or mesh network can converse with Nomos directly: start tasks, read/delete any session, answer pending questions, and — via chat-assent — approve gated executions by typing "yes" or "I confirm" to whatever the agent proposes, with no authentication at all. This is the same class of gap oikos-gaps-and-improvements flagged for the api/MCP surface (items B1-B5), but specifically for nomos's own port, which doesn't sit behind combinedAuth the way api's routes do.

Fix: put nomos's gateway behind the same auth the api process uses (shared bearer token check at minimum), or stop publishing 8092 directly and route all traffic through the already-authenticated api proxy exclusively.

---

D. Code quality

D1. Dead code: isTaskTool is defined, never called

tasks.go:139-146. The actual dispatch in agent.go:370 calls a.handleTaskTool(...) directly and checks its handled return value — isTaskTool is unused. Fix: delete it, or use it in buildTools/dispatch if a cheaper pre-check is actually wanted.

D2. N+1 query in recordTouched

store.go:720-742: loops over every slug found in a tool call's args and issues a separate SELECT id, type FROM entities WHERE slug = $1 per slug. Fine for the common case (1-3 slugs) but doesn't batch for tool calls naming many entities. Fix: one SELECT id, slug, type FROM entities WHERE slug = ANY($1) for all collected slugs, then loop over the results in memory.

D3. complete_task's outcome isn't validated

tasks.go:248-257 declares an enum in the tool schema (success|failure|partial) but store.go:428-457 never checks it — an out-of-enum value (a model typo, or a weaker model not respecting the schema) silently persists as-is; only "failure" is special-cased (else status="done"), so a stray value still "completes" the task but with a value the frontend's status/outcome rendering doesn't recognize. Fix: validate against the three allowed values in handleTaskTool before calling store.completeTask, defaulting unrecognized values to "partial" (safer than silently treating them as "success").

---

E. Test coverage

Zero automated tests exist for agent.go, store.go, main.go, or tasks.go. Only assent.go's and continue.go's pure string-parsing helpers have unit tests (assent_test.go, continue_test.go) — confirmed by grep -l "func Test" cmd/nomos/*.go matching only those two files. This means today's session added substantial new, safety-critical logic — session-scoped assent/destructive windows, the mcpClientPool's creation-race handling and eviction sweep, proposePlan's replace-vs-append branching — verified only by live manual testing (curl + browser), with no regression protection against a future change silently reintroducing the cross-task assent bleed or breaking the pool's session isolation.

Fix (highest-value additions first):

1. store_test.go: proposePlan's append-vs-replace branch (the exact bug fixed earlier today) — needs a real DB (integration-style, matching internal/db/integration_test.go's pattern) or a query-mocking layer.
2. main_test.go: mcpClientPool.get()'s concurrent-creation race path (two goroutines racing to create a client for the same new session id) and sweep()'s eviction logic — these are pure in-memory logic, no DB needed, straightforward to unit test.
3. assent_test.go: the two confirmed false-positive cases from A1.

---

F. Efficiency (minor)

F1. Tool list + fleet snapshot re-fetched every single turn

agent.go:174,181: buildTools (tools/list MCP round-trip) and fleetSnapshot (get_health_summary call) both run at the start of every chatWith call — including auto-continuation resumes, which can fire many times per task. The tool list changes only on an api process restart; the fleet snapshot is a live "as of now" read, which is arguably the point of it, but re-fetching the tool list every turn is avoidable. Fix: cache buildTools' result (e.g., in mcpClientPool, invalidated on a client's re-initialize) — worth doing only if profiling shows it matters; low priority relative to A-C.

---

Implementation order

1. A1 (assent false positives) — smallest, highest-severity-per-line-of- code fix; ships with regression tests same-PR.
2. C1 (unauthenticated gateway) — security-critical, independent of everything else here.
3. B1 (panic recovery) — cheap, broad safety net; do before B2 touches the continuation worker's goroutine structure anyway.
4. B2 (parallel auto-continuation) — natural follow-on to B1 since it's restructuring the same goroutine.
5. A3 (incremental persistence for live turns) — moderate effort, real user-visible correctness gain.
6. D1-D3 (small cleanups) — bundle together, low risk.
7. A2 (history windowing) — needs a design decision (see below) before implementation; largest single change.
8. B3, F1 — lower urgency, do opportunistically.
9. E (tests) — ideally lands alongside each fix above (A1's tests with A1, etc.) rather than as one giant deferred test-writing pass.

Verification

• A1: the two probe cases (isAssent on the "yesterday" message, isTypedConfirmation on the "haven't confirmed" message) become permanent tests in assent_test.go, asserting false post-fix.
• A2: after adding windowing, replay a session with 70+ tool calls (the documented production case) and confirm the message payload sent to the LLM stays under a fixed token/byte ceiling regardless of session length.
• A3: reproduce the Stop-button-mid-turn scenario, reload the page, and confirm the tool calls made before the abort are still present in the persisted transcript (currently: they vanish).
• B1: inject a deliberate panic in a test build of resumeSession (or a fault-injection flag), confirm the process survives and logs the recovered panic instead of exiting.
• C1: confirm an unauthenticated curl to nomos's /chat from off-mesh is rejected once auth lands (currently: succeeds).
• D1-D3: go vet/build clean, complete_task with a bogus outcome value now rejected or defaulted rather than silently persisted.

Open questions

• A2's cutoff mechanism: a fixed N-message window, a token-budget-aware trim, or LLM-summarization of dropped history? Summarization preserves the most context but costs an extra LLM call per trim; a fixed window is simplest but could drop something the agent still needs mid-task. Leaning fixed window + summarize-on-trim as a middle ground, but this needs a decision before implementation, not during.
• C1's auth mechanism: reuse api's existing static bearer token (simplest, matches an existing pattern) or route everything through api's proxy and stop publishing 8092 at all (removes the surface entirely, but changes the deploy topology)? Leaning the latter if nothing else on the LAN legitimately needs to reach nomos directly — worth confirming with the operator before picking.
• B2's concurrency bound: unbounded goroutines-per-tick vs. a small semaphore? Given the continuation batch is already capped at 5 per tick (pendingContinuations(ctx, 5)), unbounded is probably fine, but worth a sanity check against real task-completion clustering patterns.