oikos/plans/done/2026-08-03-nomos-chat-changes-review.md

2026-08-03 — Review: nomos chat reliability/UX changes (F1–F7)

Status: Implemented (P0, P1, P2 all done). See Resolution at the end.

A critical self-review of the uncommitted F1–F7 changeset (plans/2026-08-03-nomos-chat-reliability-and-ux-audit.md Resolution). The change set is mostly sound and builds/tests green, but F1 introduced one real lost-work regression by changing the contract of resumeSession (it can now skip) without updating two callers that mutate state before calling it. That must be fixed before this ships.

What was changed (for orientation)

• F1 cmd/nomos/turngate.go (+test): per-session single-flight; resumeSession acquires non-blocking and skips if a turn is active; handleChat live path acquires with a 5s wait.
• F2/F3 web/src/lib/stores/chat.ts: humanized errors, clearTurnState on terminal task.status, turn-free reconnect.
• F4 streaming in global activityLog + inline ToolCallCard.
• F5 artifact/knowledge deep links; F6 step-first headline; F7 stable layout.

---

P0 — F1 loses finished-execution continuations (must fix before shipping)

Bug. processContinuations (cmd/nomos/continue.go:166-167) calls a.store.markContinued(ctx, p.ExecID) before dispatching continueSession → resumeSession. markContinued sets continued_at, and pendingContinuations (store.go:1763) filters WHERE continued_at IS NULL — so a marked execution is never re-queued.

Before F1, resumeSession always ran, so marking-first was safe. F1 made resumeSession skip when a turn is already active for the session. Now:

• Two executions for one session finish near-simultaneously (the common multi-step case): the loop marks BOTH, spawns two goroutines; goroutine 1 acquires and runs, goroutine 2's resumeSession skips → execution 2 is marked continued but its result is never fed back to the agent. Lost.
• A live turn is streaming when an async execution finishes: continuation marks + dispatches; resumeSession skips (live turn holds the permit) → result lost.

This silently drops auto-continuation — worse than the interleaving F1 set out to fix.

Fix. Make resumeSession report whether it actually ran, and mark-continued only after a successful run; on a busy-skip, leave the execution pending for the next worker tick.

1. cmd/nomos/continue.go — change resumeSession to return bool:

   func (a *agent) resumeSession(ctx context.Context, sessionID, note string) bool {
       if !a.gate.acquire(sessionID, 0) {
           slog.Info("nomos: turn already active, skipping background resume", "session", sessionID)
           return false
       }
       defer a.gate.release(sessionID)
       …existing body…
       return true
   }
   

2. continueSession — mark only after a real run; on skip, leave pending:

   func (a *agent) continueSession(ctx context.Context, p pendingContinuation) {
       slog.Info("nomos: auto-continuing session", "session", p.SessionID, "execution", p.ExecID, "status", p.Status)
       if !a.resumeSession(ctx, p.SessionID, buildContinuationNote(p)) {
           slog.Info("nomos: continuation deferred — a turn is active; will retry next tick", "session", p.SessionID, "execution", p.ExecID)
           return
       }
       a.store.markContinued(ctx, p.ExecID)
   }
   

3. processContinuations — delete the a.store.markContinued(ctx, p.ExecID) line at continue.go:166 (the dispatch safego.Go(... continueSession ...) stays). The markContinued at :162 (the no-assent-window branch, which saves a note and does not call resumeSession) stays as-is — that path intentionally consumes the item.
4. Update every other resumeSession caller to ignore the new return value (/resume, handleAnswerQuestion, the empty-message reconnect in handleChat) — they don't need the bool; a bare call discards it. No behavior change for them (their skip semantics are already correct/desired).

Why this preserves the original "no re-continue loop" guarantee: a resumeSession that runs always returns true (even on its internal LLM failure path — it has already persisted a failure note), so it gets marked and won't loop. Only a busy-skip returns false and stays pending, which is correct (retry once the turn frees). Crash-safety also improves: a crash between acquire and mark leaves the item un-marked → re-queued on restart.

Validation:

• New test: two pendingContinuations for one session dispatched concurrently; assert both are eventually processed (both continued_at set) and at no point do two resumeSession bodies overlap (reuse the turnGate single-flight pattern, or assert via a shared counter in a stubbed chatWith).
• Existing cmd/nomos suite stays green; go vet clean.

---

P1 — F1 can false-auto-close a merely-busy session (low risk, fix for robustness)

Bug. processIdleSweep (continue.go:78-89) bumps completion_nudges before calling resumeSession. If resumeSession skips (busy), the nudge is counted as unanswered; the next sweep sees CompletionNudges >= 1 and auto-closes a session that was just busy.

Likelihood is low because staleGoalSessions (store.go:1336) filters last_active_at < now() - threshold and an active turn keeps updating last_active_at — so a busy session shouldn't appear stale. But the coupling is the same shape as P0 and worth closing.

Fix. Gate the bump on the run, mirroring P0:

safego.Go("nomos:idle-nudge:"+s.ID, func() {
    note := …
    if a.resumeSession(ctx, s.ID, note) {
        if err := a.store.bumpCompletionNudge(ctx, s.ID); err != nil { … }
    }
})

(If skipped, leave completion_nudges at 0 so a genuinely-stale sweep nudges again later.)

---

P2 — Minor / hygiene (optional, can ship without)

• Redundant catch-up turn on reconnect. When the live turn already ended before a dropped-SSE reconnect fires, the empty-message path still runs a "report your state" resumeSession turn the operator didn't ask for. F1 makes it non-concurrent (good) but it's still a spare turn. Consider: in handleChat's empty-message branch, skip the resumeSession if the session is already terminal (done/failed/abandoned) or had activity within the last few seconds — just return 202 and let the poller catch up.
• Top-level side-effect on import. chat.ts now calls subscribeEvents() + liveEvents.subscribe(...) at module top level. It works (and vitest stays green because tests mock ./chat), but a hidden SSE-connect-on-import is fragile for future tests. Prefer a lazy ensureChatEventSync() called from the window mount path, matching how workspace.ts subscribes inside startWorkspace rather than at import.
• F7 follow-up (already documented): the NewTaskChat → SessionChatWindow window-swap on first send still flashes; an in-place handoff would remove it.
• Pre-existing, not introduced: a.chat retries the LLM stream on ctx-cancellation (client disconnect) up to 3×, holding the turn permit a few extra seconds. Out of scope here.

---

Out of scope

• F8 (ordering toggle + live background tool-delta streaming) — deferred in the original plan; its main symptom is removed by F1.
• run execution deep-links (need an execution-view opener).

Recommended order

1. P0 (lost continuations) — blocks shipping F1.
2. P1 (idle-sweep nudge gate) — small, same pattern.
3. P2 items as time allows.
4. Re-run go test ./cmd/nomos/, go vet, web vitest, vite build; keep VERSION at 0.15.0 (these are correctness fixes to the same changeset, not a new bump) — or bump patch to 0.15.1 if shipped as a follow-up commit.

---

Resolution

All review items implemented. The whole batch (F1–F7 + these review fixes) remains one uncommitted changeset at VERSION 0.15.0.

Item	Fix	Where
P0	resumeSession returns bool (false on busy-skip). continueSession marks an execution continued only after the turn ran; on a skip it defers and the next worker tick retries (item stays pending). Removed the pre-dispatch markContinued in processContinuations. Other callers (/resume, answer-question, reconnect) ignore the return.	cmd/nomos/continue.go
P0 test	TestResumeSession_SkipsWhenBusy, TestContinueSession_DefersWhenBusy — DB-free contract tests proving the skip path returns false without running the body (nil provider would panic otherwise).	cmd/nomos/continue_test.go
P1	Idle sweep bumps completion_nudges only after resumeSession actually runs, so a busy-skip can't be counted as an unanswered nudge → no false auto-close.	cmd/nomos/continue.go (processIdleSweep)
P2.1	Empty-message reconnect (now defensive — the frontend no longer POSTs empty messages post-F2) skips a terminal session instead of spawning a spare "report state" turn.	cmd/nomos/main.go (handleChat)
P2.2	Event subscription armed lazily from chatFor() (ensureChatEventSync) instead of at module import — no SSE-connect-on-import side-effect.	web/src/lib/stores/chat.ts

Verification: go test -count=1 ./cmd/nomos/ green (incl. the two new contract tests); go vet clean. Web vitest 70/70; vite build succeeds; no new tsc/eslint errors in any touched file.

Note on the P0 end-to-end test: the full "two continuations both processed, no overlap" scenario needs a live LLM provider (chatWith isn't stubbable without a refactor) and was therefore covered at the contract level (the skip returns false without running the body) plus the existing turnGate single-flight test for serialization, rather than as a DB integration test.