dtoro/oikos
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

docs: nomos agent code review — gaps and improvement plan

Browse Source 
Full read-through of cmd/nomos/ (agent.go, store.go, main.go, continue.go,
assent.go, tasks.go). Findings, ranked:

- A1 (confirmed via runnable probe): isAssent/isTypedConfirmation use
  unpadded substring matching for assent/confirm words while negation uses
  word-boundary checks — "yes" matches inside "yesterday", "confirm" matches
  inside "confirmed" with no negation word covering contracted negatives
  ("haven't"). isTypedConfirmation gates DESTRUCTIVE actions specifically.
- A2: chatWith replays a session's ENTIRE message history every turn, no
  windowing/token budget — confirmed unbounded against a documented
  production case (70 tool calls, 106KB messages).
- A3: a live turn's tool-call history is lost entirely if the client
  disconnects mid-stream (single end-of-turn save using the same
  connection-tied, possibly-cancelled context) — resumeSession already has
  the fix pattern (incremental placeholder+update), handleChat doesn't use it.
- B1: zero recover() anywhere in cmd/nomos/internal/mcp/internal/httpapi —
  every explicitly-spawned goroutine (continuation worker, resumeSession,
  executeApprovedViaAPI, sse listeners) crashes the whole process on panic.
- B2: auto-continuation processes its batch sequentially, one full LLM turn
  at a time, undercutting this session's own concurrency work on exactly the
  path autonomous tasks depend on most.
- B3: no terminal state for a permanently-failed auto-continuation.
- C1: nomos's own gateway (port 8092, directly published + mesh-reachable)
  has ZERO authentication on any endpoint — chat, session read/delete,
  chat-assent approval of gated executions, all open to anyone on the LAN.
- D1-D3: dead code (isTaskTool unused), N+1 query in recordTouched, no
  validation on complete_task's outcome enum.
- E: zero automated tests for agent.go/store.go/main.go/tasks.go — including
  today's new safety-critical logic (session-scoped windows, mcpClientPool,
  proposePlan's append-vs-replace), verified only by live manual testing.
- F1: tool list + fleet snapshot re-fetched every turn (minor).

Prioritized implementation order in the doc: A1 → C1 → B1 → B2 → A3 → D1-3 →
A2 → B3/F1, tests landing alongside each fix rather than as a deferred pass.

Also archives the now-fully-shipped concurrent-task-execution plan to done/
(all 3 required fixes deployed this session; fix 4 explicitly deferred per
its own recommendation).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
authentik Default Admin
2026-07-11 19:47:17 +02:00
parent a4ea542f3e
commit df393152f6
3 changed files with 345 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

323
plans/2026-07-11-nomos-agent-code-review.md Normal file
View File

@@ -0,0 +1,323 @@
# 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](../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:133](../docker-compose.yml) publishes port 8092 directly
(`"8092:8092"`, comment: *"mesh-published"*) and
[Caddyfile.oikos](../compose/caddy/Caddyfile.oikos:19,34) 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](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.