dtoro
39e9227fdb
The agent could run two turns for one session at once (a reconnect resumed
while the live turn was still going), and their interleaved tool calls
corrupted the activity panel, fabricated a confusing "parallel/nested"
sequence, and made tasks feel stuck/never-ending. Several UX gaps compounded it.
Turn serialization (F1):
- turnGate: at most one in-flight turn per session. Background resume paths
(continuation worker, idle sweep, answer-question, /resume, reconnect)
skip non-blocking when busy; the live chat path waits briefly then bails
cleanly instead of stacking a second turn.
- resumeSession returns whether it ran; continueSession marks an execution
"continued" only after a real run (review P0) so a busy-skip can't lose a
finished-execution result. Idle nudge bumps only after delivery (P1).
Connection state (F2/F3, web):
- humanize/bucket raw errors ("model connection dropped..."); one surface
per drop; a terminal task.status event clears stuck streaming/disconnected
state and dismisses the connection toast. Reconnect no longer spawns turns.
Streaming where you look (F4, web):
- live command output in the global activity timeline and in the inline
tool card (auto-opened, tail-pinned) -- not just the per-window rail.
Other (web): artifact/knowledge deep links (F5); step-first stable
"thinking" headline (F6); stable chat layout, no empty->content reflow (F7);
lazy event sync (P2.2); reconnect skips a terminal session (P2.1).
VERSION: 0.14.2 -> 0.15.0
185 lines
9.7 KiB
Markdown
185 lines
9.7 KiB
Markdown
# 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.
|