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

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

**Status:** In Progress — 2026-07-11. Every finding except C1 (A1-A3, B1-B3,
D1-D3, E, F1) is fixed, tested, and verified live against the running stack.
C1 (unauthenticated nomos gateway) is explicitly deferred per operator
instruction ("leave auth out for these round of fixes") — the one item
keeping this out of `done/`.

- A1 `3919ec3`, B1+B2 `c5ffaec`, A3 `926969a`, D1-D3 `76f7630`,
  A2 `c390164`, B3 `6d4f6de`, F1 `11c18e8`.
- New `internal/safego` package (B1) and `cmd/nomos/store_test.go` (A2, plus
  a regression test for the earlier plan-append fix) are the first automated
  tests for any of this package's core logic — closing part of finding E,
  though full coverage of agent.go/main.go remains future work.
- C1 remains open — nomos's gateway (port 8092) still has no authentication.
  Revisit separately.

## 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](../cmd/nomos/assent.go). `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](../cmd/nomos/agent.go)) — 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](../cmd/nomos/agent.go): every single turn (`chatWith`)
calls `a.store.getMessages(ctx, sessionID)` — [store.go:218-239](../cmd/nomos/store.go),
`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](../cmd/nomos/store.go))
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](done/2026-07-09-chat-sessions-improvements.md))
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](../cmd/nomos/agent.go), 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](../cmd/nomos/main.go): `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](../cmd/nomos/continue.go)),
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](../cmd/nomos/continue.go): `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](../cmd/nomos/continue.go)). 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](../cmd/nomos/continue.go): 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:144](../docker-compose.yml) publishes port 8092 directly
(`"8092:8092"`, comment: *"mesh-published"*) and
[Caddyfile.oikos:52-54](../compose/caddy/Caddyfile.oikos) reverse-proxies to
it — as of the client/server split
([2026-07-12-wails-desktop-app.md](2026-07-12-wails-desktop-app.md)), only
from `nomos.hubris.network` now, not two routes: `/agent/*` on
`oikos.hubris.network` was repointed to go through `api`'s own authenticated
proxy mount instead of straight to nomos:8092, but that's `combinedAuth`
authenticating the *hop into api*, not anything nomos itself checks — this
finding is unaffected by that change, still fully open. `grep -n
"Authorization\|Bearer\|auth" cmd/nomos/main.go` still returns **nothing**
for nomos's inbound routes (nomos did gain outbound auth as *part of* the
client/server split — it now sends `Authorization: Bearer
$OIKOS_MCP_BEARER_TOKEN` on its own calls to `api` — but that's the opposite
direction from this finding) — `/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](2026-07-08-oikos-gaps-and-improvements.md)
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](../cmd/nomos/tasks.go). The actual dispatch in
[agent.go:370](../cmd/nomos/agent.go) 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](../cmd/nomos/store.go): 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](../cmd/nomos/tasks.go) declares an `enum` in the tool
schema (`success|failure|partial`) but [store.go:428-457](../cmd/nomos/store.go)
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](../cmd/nomos/agent.go): `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.