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](#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`:
   ```go
   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:
   ```go
   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 `pendingContinuation`s 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:
```go
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.