docs(plans): reconcile plan statuses; archive 10 done plans
Move ten completed plans from plans/ to plans/done/ and update the index:
- 2026-07-18 session-review-three-sessions, 2026-07-20 desktop-mascot,
2026-07-20 session-review-ten-sessions, 2026-07-21 chat-full-polish,
2026-07-29 health-check-reality-and-knowledge-graph,
2026-07-30 session-review-plan-drift, and the four 2026-08-03 chat plans
(changes-review, reliability-and-ux-audit, cyberspace-style-adoption,
working-visibility).
- Refresh two stale statuses: cyberspace-style-adoption ("Draft" -> shipped as
full replacement in v0.16.0/757ef2f) and health-check-reality ("ready for
implementation" -> shipped across the v0.14.x-0.16.x check commits).
- .gitignore: ignore local tooling artifacts (.playwright-mcp/, config-screen.png).
No code change. index.md Active/Done tables now match the filesystem (no orphans).
VERSION: 0.17.0 -> 0.17.1
This commit is contained in:
184
plans/done/2026-08-03-nomos-chat-changes-review.md
Normal file
184
plans/done/2026-08-03-nomos-chat-changes-review.md
Normal file
@@ -0,0 +1,184 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user