feat(chat): embedded chat — view and reply to instance conversations (ADR-0016) #11

Merged

dominik.polakovics merged 2 commits from worktree-feat-embedded-chat into main 2026-07-07 07:48:10 +02:00

Conversation 2 Commits 2 Files changed 52 +5344 -35

dominik.polakovics commented 2026-07-07 00:48:17 +02:00

Owner

Copy link

Implements issue #7: the phone-first Chat view promised by ADR-0005's roadmap. A /runs/:id surface renders an instance's conversation and lets the operator reply, answer interactive dialogs, and interrupt — for manual instances and AFK runs alike. It complements the claude.ai deep link, never replaces it. Claude Code is the only provider implementation; the message schema and every mechanism sit behind the AgentProvider seam so a future Codex/Gemini chat is one new implementation, zero refactor.

Design was settled with the operator on 2026-07-06 and is recorded in ADR-0016 (issue #1's GitHub-tracker PR claimed 0015, so this took the next free number as the issue instructed).

What's here

• Read-through only — the chat reads the provider-native transcript (Claude Code's live JSONL, located by worktree-cwd match, the same registry file the deep link uses) mapped behind the seam to a universal text | tool | dialog | lifecycle schema. One new nullable column runs.transcript_path (migration 0003, sqlite + postgres), captured async like deep_link_url so ended runs stay readable. No message table.
• Reply / answer / interrupt via tmux send-keys — bracketed-paste reply + Enter; pinned dialog keystroke recipes (single-question AskUserQuestion; multi-question and ExitPlanMode plan approval degrade to the deep-link hint since their options aren't in the transcript); Escape interrupt. internal/tmuxx gains SendNamedKeys + PasteText.
• internal/chat — the read/act brain plus a self-managing tailer (bus subscriber) that derives conversational state for the instance list and publishes a debounced run.messages.changed; the chat view refetches GET /runs/{id}/messages with an append-only cursor.
• Intervention neutrality — replying to or interrupting an AFK run never touches its budget clock, claim, or three-strikes counter (structural: nothing in the chat path writes a run outcome).
• Four new fragile couplings pinned in internal/compat (§5–§8) against Claude Code 2.1.198, with a captured transcript fixture and snapshot tests.
• Web — /runs/:id chat view (fixed composer, native dialog buttons, thinking toggle, confirm-guarded interrupt, composer states) + live state badges and Chat links on Dashboard and Runs rows.

Docs

ADR-0016; CONTEXT.md (Chat / Transcript / Conversational state); ADR-0005 + agent-brief.md event list now include run.messages.changed; definition-of-done item 10.

Verification

nix flake check passes — Go build/tests (real git+tmux+prlimit), golangci-lint, SPA vitest (266 tests incl. the new RunChat component test), NixOS module eval. Real-tmux integration exercises the new send primitives.

Known boundary (documented, not a gap)

Dialog answering is wired for single-question AskUserQuestion only. Multi-question AskUserQuestion and plan approval degrade to an "open in claude.ai" deep-link hint — their option widgets aren't in the structured transcript input, and the issue's own "never scrape the TUI widget" rule makes that the correct degradation. Recorded in ADR-0016 (Considered options → deferred) and compat.md §7 as revisitable once the widgets are captured live.

Closes #7

🤖 Generated with Claude Code

Implements issue #7: the phone-first **Chat** view promised by ADR-0005's roadmap. A `/runs/:id` surface renders an instance's conversation and lets the operator reply, answer interactive dialogs, and interrupt — for manual instances and AFK runs alike. It **complements** the claude.ai deep link, never replaces it. Claude Code is the only provider implementation; the message schema and every mechanism sit behind the `AgentProvider` seam so a future Codex/Gemini chat is one new implementation, zero refactor. Design was settled with the operator on 2026-07-06 and is recorded in **ADR-0016** (issue #1's GitHub-tracker PR claimed 0015, so this took the next free number as the issue instructed). ## What's here - **Read-through only** — the chat reads the provider-native transcript (Claude Code's live JSONL, located by worktree-cwd match, the same registry file the deep link uses) mapped behind the seam to a universal `text | tool | dialog | lifecycle` schema. One new nullable column `runs.transcript_path` (migration `0003`, sqlite + postgres), captured async like `deep_link_url` so ended runs stay readable. **No message table.** - **Reply / answer / interrupt via `tmux send-keys`** — bracketed-paste reply + Enter; pinned dialog keystroke recipes (single-question `AskUserQuestion`; multi-question and `ExitPlanMode` plan approval degrade to the deep-link hint since their options aren't in the transcript); Escape interrupt. `internal/tmuxx` gains `SendNamedKeys` + `PasteText`. - **`internal/chat`** — the read/act brain plus a self-managing tailer (bus subscriber) that derives conversational state for the instance list and publishes a debounced `run.messages.changed`; the chat view refetches `GET /runs/{id}/messages` with an append-only cursor. - **Intervention neutrality** — replying to or interrupting an AFK run never touches its budget clock, claim, or three-strikes counter (structural: nothing in the chat path writes a run outcome). - **Four new fragile couplings** pinned in `internal/compat` (§5–§8) against Claude Code 2.1.198, with a captured transcript fixture and snapshot tests. - **Web** — `/runs/:id` chat view (fixed composer, native dialog buttons, thinking toggle, confirm-guarded interrupt, composer states) + live state badges and Chat links on Dashboard and Runs rows. ## Docs ADR-0016; CONTEXT.md (Chat / Transcript / Conversational state); ADR-0005 + `agent-brief.md` event list now include `run.messages.changed`; definition-of-done item 10. ## Verification `nix flake check` passes — Go build/tests (real git+tmux+prlimit), golangci-lint, SPA vitest (266 tests incl. the new RunChat component test), NixOS module eval. Real-tmux integration exercises the new send primitives. ## Known boundary (documented, not a gap) Dialog **answering** is wired for single-question `AskUserQuestion` only. Multi-question `AskUserQuestion` and plan approval degrade to an "open in claude.ai" deep-link hint — their option widgets aren't in the structured transcript input, and the issue's own "never scrape the TUI widget" rule makes that the correct degradation. Recorded in ADR-0016 (Considered options → deferred) and compat.md §7 as revisitable once the widgets are captured live. Closes #7 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dominik.polakovics added 1 commit 2026-07-07 00:48:17 +02:00

feat(chat): embedded chat — view and reply to instance conversations (ADR-0016) ... a9467019d4

Add the phone-first Chat view promised by ADR-0005's roadmap (issue #7): a
/runs/:id surface that renders an instance's conversation and lets the operator
reply, answer interactive dialogs, and interrupt — for manual instances and AFK
runs alike. It complements the claude.ai deep link, never replaces it. Claude
Code is the only provider implementation; the message schema and every
mechanism sit behind the AgentProvider seam.

- Read-through only: the chat reads the provider-native transcript (Claude
  Code's live JSONL, located by worktree-cwd match) mapped behind the seam to a
  universal text|tool|dialog|lifecycle schema. One new nullable column,
  runs.transcript_path (migration 0003, sqlite + postgres), captured async like
  deep_link_url so ended runs stay readable; no message table.
- Reply/answer/interrupt via tmux send-keys: bracketed-paste reply + Enter,
  pinned dialog keystroke recipes (AskUserQuestion single-question; multi-
  question and plan approval degrade to the deep-link hint), Escape interrupt.
  internal/tmuxx gains SendNamedKeys + PasteText.
- internal/chat is the read/act brain plus a self-managing tailer (bus
  subscriber) that derives conversational state for the instance list and
  publishes a debounced run.messages.changed; the chat view refetches
  GET /runs/{id}/messages with an append-only cursor.
- Intervention is budget/claim/three-strikes neutral by construction.
- Four new fragile couplings pinned in internal/compat (§5-§8) against Claude
  Code 2.1.198, with a captured transcript fixture and snapshot tests.
- Web: /runs/:id chat view (fixed composer, native dialog buttons, thinking
  toggle, confirm-guarded interrupt, composer states) + live state badges and
  Chat links on Dashboard and Runs rows.

Docs: ADR-0016, CONTEXT.md (Chat/Transcript/Conversational state), ADR-0005 +
agent-brief event list, definition-of-done item 10.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dominik.polakovics commented 2026-07-07 01:03:22 +02:00

Author

Owner

Copy link

This was generated by AI while landing a PR.

/land-pr validation — verdict: FAIL (changes requested)

What was checked. CI has no runner (ci/flake-check pends forever), so nothing vouched for the PR: ran nix flake check locally on head a946701 — all checks pass (Go build+tests, golangci-lint, web build, NixOS module eval). Full diff reviewed (46 files, +4231) against issue #7's settled decisions. Conventions OK: Conventional Commits title, ADR-0016 is the next free number, migration 0003 mirrored sqlite/postgres, Closes #7 present, no conflicts. Each blocker below was verified by hand (code reading + live-transcript checks), not just flagged.

Blockers

1. Failed tools render "ok". internal/provider/claudecode/chat_types.go:65-74,163-192 — real Claude Code 2.x transcripts put is_error on the tool_result block, with content a plain string (verified against a live transcript on this host). The decoder reads is_error only from inner content items and hardcodes false on the string path, so every failing tool shows status "ok" — violating settled decision 9 ("errors always surfaced"). The fixture internal/compat/testdata/transcript-2.1.198.jsonl carries the flag in both places, which is why the compat tests pass while masking this. Fix: add IsError bool to tBlock, use it in patchToolResult, and correct the fixture to the real shape.

2. No scroll management in the chat view. web/src/routes/RunChat.tsx + web/src/base.css:1333,1371 — .chat-page is min-height:100dvh, so .chat-stream's overflow-y:auto never engages: the document scrolls and the view opens at the top, i.e. the oldest message of the latest window — contradicting the settled "latest window first". Appended messages land below the fold with no follow-bottom (live monitoring means manual scrolling after every event), and "Load earlier" prepends up to 60 messages with no scroll anchoring (iOS Safari, the phone-first target, has no overflow-anchor). Needs: a bounded stream (height, not min-height), scroll-to-bottom on mount, follow-bottom on append when already at bottom, prepend compensation.

3. Opening an ended run's chat can permanently capture the successor run's transcript. internal/chat/chat.go:128-163 + internal/provider/claudecode/chat.go:52-62 — Read → resolvePath locates and persists for any outcome, and LocateTranscript ignores the session name, matching the newest live claude whose cwd is the worktree. For an ended run any hit is by definition a different, newer run (worktrees are reused across runs, as deeplink.go's own comment notes) — R1's row then permanently points at R2's transcript. Fix: don't locate (or at least don't persist) when run.Outcome != active; serve the "transcript no longer available" state instead.

Concerns (should fix — several are one-liners)

• internal/chat/chat.go:199-205 — PendingDialog scans for any dialog message (lastDialog, despite its own "if the tail is one" comment) instead of requiring State == StateQuestion like the Reply guard at :225-231. A rare abandoned mid-transcript dialog passes the tool_id match and fires picker keystrokes into a composer. (Trigger is rare — checked 30 live transcripts, interrupts do write synthetic tool_results — but the guard asymmetry is real and the fix is one line.)
• internal/httpapi/chat.go:186 — the tool_id staleness check is opt-in (req.ToolID != ""); a caller omitting it answers whatever dialog is live by blind index, contradicting the handler's docstring. 400 on empty tool_id.
• internal/provider/claudecode/interact.go:108-156 — multi-select validation gaps: empty or entirely out-of-range Selected silently produces a zero-selection confirm and returns 204; nothing rejects selecting the synthesized "Other" row in multi-select (Space there plausibly opens the free-text input and desyncs the recipe). Should be 400s like the single-select range check.
• web/src/routes/RunChat.tsx:61-71 — loadLatest never uses the after=<cursor> protocol that ADR-0016 itself pins; more than 60 messages between refetches (the SSE watchdog tolerates 65s of silence) leaves a silent, unfillable gap mid-stream ("Load earlier" pages before the oldest message, not into the hole).
• web/src/routes/RunChat.tsx:61-98 — concurrent loadLatest calls have no sequencing; a stale response applying last can revert an answered dialog to pending and re-lock the composer, with possibly no further event to unstick it. Request token or AbortController.
• web/src/routes/RunChat.tsx:372-395 — DialogPanel selection state isn't keyed to dialog.tool_id; if the pending dialog changes identity while mounted, stale indices answer the new dialog — and pass the server guard, since props.dialog.tool_id is read live. <Show keyed> on tool_id or reset on change.
• internal/chat/tailer.go:77,169-174 — the deferred remove(sessionName) isn't generation-aware; session-name reuse (stop→start in the same minute reproduces the name) can delete the successor tailer's cancel, leaving an unmanaged tailer double-polling and double-publishing until shutdown. Store a generation token/pointer and delete only on identity match.
• internal/chat/tailer.go:27-49 — the tailer set syncs only on run.changed; one dropped bus event (16-slot buffer shared with chatty event types) desyncs it indefinitely — an ended run keeps a tailer, a new run gets none. A periodic resync ticker bounds the staleness.
• internal/provider/claudecode/chat.go:116-139 — sc.Err() is never checked; a single line over the scanner cap silently truncates the transcript at that point forever (and Cursor goes backwards vs. what clients saw).
• internal/httpapi/chat.go:223-233 — a dead tmux session under a still-active run surfaces as an opaque 500 ("internal error"); map to 409/404 with a "session is gone" message.
• State mislabels: an ended run with no transcript serves StateIdle forever — the outcome override at internal/chat/chat.go:141-144 is unreachable from the path == "" return at :133-136; an active run with transcript: gone shows "This instance has ended" copy (RunChat.tsx:333); an ended never-located run reports transcript:"available" with zero messages (internal/httpapi/chat.go:72-85).
• No per-session serialization of reply/answer/interrupt (interact.go — each is 2..N tmux calls); concurrent POSTs interleave keystrokes at the tmux level (merged replies, wrong option selected). Server.crMu is the in-repo precedent for a keyed mutex.
• docs/definition-of-done.md:53 — §10 claims Dashboard badge coverage under vitest; RunChat.test.tsx doesn't exercise badges, there's no InstanceList badge test, and web/src/lib/conversation.ts has no test (every other lib/*.ts has one).

Nits (fix opportunistically)

validateReply rejects \r, so CRLF API clients get a confusing 400 (interact.go:181) · tmux named buffer leaks when paste fails after load (tmuxx.go:296) · the bracketed-paste integration test passes identically without -p — it doesn't actually pin §6's property (integration_test.go:366) · truncate cuts mid-UTF-8-rune → U+FFFD garbage (chat_types.go:206) · unfiltered run.changed subscription refetches this chat on every run's change fleet-wide (RunChat.tsx:93) · tool chips are 36px and the back button ~20px wide vs the settled 44px targets (base.css:1425) · the chat header drops the repo half of repo~label and has no claude.ai fallback link when deep_link_url is null — the "open in claude.ai" dialog hint then points nowhere (RunChat.tsx:173,220,404) · the seq back-patching vs append-only-cursor tension (tool status / dialog flips mutate seqs ≤ cursor; the client compensates only within its 60-message window) deserves a note in ADR-0016 · dialogPending reads under s.ctx instead of the request context and fails open (chat.go:226).

Verified clean

Escaping is solid (argv-only exec, reply bytes via load-buffer stdin — quotes/newlines/semicolons/unicode safe) · all chat routes requireAuth inside the CSRF-guarded operator tree · intervention neutrality confirmed — the only store write on any chat path is transcript_path; nothing touches outcome, budget, claim, or strikes · migrations 0003 sqlite/postgres parity · no XSS (all message content renders as JSX text nodes) · SSE subscriptions cleaned up on route leave · ADR-0016 follows house style and records issue #7's decisions faithfully · CONTEXT.md gains Chat/Transcript/Conversational state · compat §5–§8 pins are real snapshot tests, with §7's fixture-only provenance honestly documented.

Overall this is a well-built feature — the seam design, neutrality, and compat discipline are exactly as settled in issue #7. The three blockers are small, targeted fixes; re-invoke /land-pr once they land.

> *This was generated by AI while landing a PR.* ## /land-pr validation — verdict: FAIL (changes requested) **What was checked.** CI has no runner (`ci/flake-check` pends forever), so nothing vouched for the PR: ran `nix flake check` locally on head a946701 — **all checks pass** (Go build+tests, golangci-lint, web build, NixOS module eval). Full diff reviewed (46 files, +4231) against issue #7's settled decisions. Conventions OK: Conventional Commits title, ADR-0016 is the next free number, migration 0003 mirrored sqlite/postgres, `Closes #7` present, no conflicts. Each blocker below was verified by hand (code reading + live-transcript checks), not just flagged. ### Blockers 1. **Failed tools render "ok".** `internal/provider/claudecode/chat_types.go:65-74,163-192` — real Claude Code 2.x transcripts put `is_error` on the `tool_result` *block*, with `content` a plain string (verified against a live transcript on this host). The decoder reads `is_error` only from inner content items and hardcodes `false` on the string path, so every failing tool shows status "ok" — violating settled decision 9 ("errors always surfaced"). The fixture `internal/compat/testdata/transcript-2.1.198.jsonl` carries the flag in *both* places, which is why the compat tests pass while masking this. Fix: add `IsError bool` to `tBlock`, use it in `patchToolResult`, and correct the fixture to the real shape. 2. **No scroll management in the chat view.** `web/src/routes/RunChat.tsx` + `web/src/base.css:1333,1371` — `.chat-page` is `min-height:100dvh`, so `.chat-stream`'s `overflow-y:auto` never engages: the *document* scrolls and the view opens at the **top**, i.e. the oldest message of the latest window — contradicting the settled "latest window first". Appended messages land below the fold with no follow-bottom (live monitoring means manual scrolling after every event), and "Load earlier" prepends up to 60 messages with no scroll anchoring (iOS Safari, the phone-first target, has no `overflow-anchor`). Needs: a bounded stream (`height`, not `min-height`), scroll-to-bottom on mount, follow-bottom on append when already at bottom, prepend compensation. 3. **Opening an ended run's chat can permanently capture the successor run's transcript.** `internal/chat/chat.go:128-163` + `internal/provider/claudecode/chat.go:52-62` — `Read` → `resolvePath` locates *and persists* for any outcome, and `LocateTranscript` ignores the session name, matching the newest **live** claude whose cwd is the worktree. For an ended run any hit is by definition a different, newer run (worktrees are reused across runs, as deeplink.go's own comment notes) — R1's row then permanently points at R2's transcript. Fix: don't locate (or at least don't persist) when `run.Outcome != active`; serve the "transcript no longer available" state instead. ### Concerns (should fix — several are one-liners) - `internal/chat/chat.go:199-205` — `PendingDialog` scans for *any* dialog message (`lastDialog`, despite its own "if the tail is one" comment) instead of requiring `State == StateQuestion` like the Reply guard at :225-231. A rare abandoned mid-transcript dialog passes the tool_id match and fires picker keystrokes into a composer. (Trigger is rare — checked 30 live transcripts, interrupts do write synthetic tool_results — but the guard asymmetry is real and the fix is one line.) - `internal/httpapi/chat.go:186` — the tool_id staleness check is opt-in (`req.ToolID != ""`); a caller omitting it answers whatever dialog is live by blind index, contradicting the handler's docstring. 400 on empty `tool_id`. - `internal/provider/claudecode/interact.go:108-156` — multi-select validation gaps: empty or entirely out-of-range `Selected` silently produces a zero-selection confirm and returns 204; nothing rejects selecting the synthesized "Other" row in multi-select (Space there plausibly opens the free-text input and desyncs the recipe). Should be 400s like the single-select range check. - `web/src/routes/RunChat.tsx:61-71` — `loadLatest` never uses the `after=<cursor>` protocol that ADR-0016 itself pins; more than 60 messages between refetches (the SSE watchdog tolerates 65s of silence) leaves a silent, unfillable gap mid-stream ("Load earlier" pages before the oldest message, not into the hole). - `web/src/routes/RunChat.tsx:61-98` — concurrent `loadLatest` calls have no sequencing; a stale response applying last can revert an answered dialog to pending and re-lock the composer, with possibly no further event to unstick it. Request token or AbortController. - `web/src/routes/RunChat.tsx:372-395` — `DialogPanel` selection state isn't keyed to `dialog.tool_id`; if the pending dialog changes identity while mounted, stale indices answer the new dialog — and pass the server guard, since `props.dialog.tool_id` is read live. `<Show keyed>` on tool_id or reset on change. - `internal/chat/tailer.go:77,169-174` — the deferred `remove(sessionName)` isn't generation-aware; session-name reuse (stop→start in the same minute reproduces the name) can delete the successor tailer's cancel, leaving an unmanaged tailer double-polling and double-publishing until shutdown. Store a generation token/pointer and delete only on identity match. - `internal/chat/tailer.go:27-49` — the tailer set syncs only on `run.changed`; one dropped bus event (16-slot buffer shared with chatty event types) desyncs it indefinitely — an ended run keeps a tailer, a new run gets none. A periodic resync ticker bounds the staleness. - `internal/provider/claudecode/chat.go:116-139` — `sc.Err()` is never checked; a single line over the scanner cap silently truncates the transcript at that point forever (and `Cursor` goes backwards vs. what clients saw). - `internal/httpapi/chat.go:223-233` — a dead tmux session under a still-active run surfaces as an opaque 500 ("internal error"); map to 409/404 with a "session is gone" message. - State mislabels: an ended run with no transcript serves `StateIdle` forever — the outcome override at `internal/chat/chat.go:141-144` is unreachable from the `path == ""` return at :133-136; an active run with `transcript: gone` shows "This instance has ended" copy (`RunChat.tsx:333`); an ended never-located run reports `transcript:"available"` with zero messages (`internal/httpapi/chat.go:72-85`). - No per-session serialization of reply/answer/interrupt (`interact.go` — each is 2..N tmux calls); concurrent POSTs interleave keystrokes at the tmux level (merged replies, wrong option selected). `Server.crMu` is the in-repo precedent for a keyed mutex. - `docs/definition-of-done.md:53` — §10 claims Dashboard badge coverage under vitest; RunChat.test.tsx doesn't exercise badges, there's no InstanceList badge test, and `web/src/lib/conversation.ts` has no test (every other `lib/*.ts` has one). ### Nits (fix opportunistically) `validateReply` rejects `\r`, so CRLF API clients get a confusing 400 (`interact.go:181`) · tmux named buffer leaks when paste fails after load (`tmuxx.go:296`) · the bracketed-paste integration test passes identically without `-p` — it doesn't actually pin §6's property (`integration_test.go:366`) · `truncate` cuts mid-UTF-8-rune → U+FFFD garbage (`chat_types.go:206`) · unfiltered `run.changed` subscription refetches this chat on every run's change fleet-wide (`RunChat.tsx:93`) · tool chips are 36px and the back button ~20px wide vs the settled 44px targets (`base.css:1425`) · the chat header drops the repo half of `repo~label` and has no claude.ai fallback link when `deep_link_url` is null — the "open in claude.ai" dialog hint then points nowhere (`RunChat.tsx:173,220,404`) · the seq back-patching vs append-only-cursor tension (tool status / dialog flips mutate seqs ≤ cursor; the client compensates only within its 60-message window) deserves a note in ADR-0016 · `dialogPending` reads under `s.ctx` instead of the request context and fails open (`chat.go:226`). ### Verified clean Escaping is solid (argv-only exec, reply bytes via `load-buffer` stdin — quotes/newlines/semicolons/unicode safe) · all chat routes `requireAuth` inside the CSRF-guarded operator tree · **intervention neutrality confirmed** — the only store write on any chat path is `transcript_path`; nothing touches outcome, budget, claim, or strikes · migrations 0003 sqlite/postgres parity · no XSS (all message content renders as JSX text nodes) · SSE subscriptions cleaned up on route leave · ADR-0016 follows house style and records issue #7's decisions faithfully · CONTEXT.md gains Chat/Transcript/Conversational state · compat §5–§8 pins are real snapshot tests, with §7's fixture-only provenance honestly documented. Overall this is a well-built feature — the seam design, neutrality, and compat discipline are exactly as settled in issue #7. The three blockers are small, targeted fixes; re-invoke `/land-pr` once they land.

dominik.polakovics referenced this pull request from a commit 2026-07-07 01:29:55 +02:00

fix(chat): address land-pr review findings — error mapping, scroll, transcript capture, guards

dominik.polakovics added 1 commit 2026-07-07 01:29:55 +02:00

fix(chat): address land-pr review findings — error mapping, scroll, transcript capture, guards ... fb75ccd403

Blockers from the PR #11 review:
- tool_result is_error is block-level in real 2.x transcripts (content is a
  plain string): read it there — failed tools no longer render "ok". The
  compat fixture is corrected to the real grammar and compat.md §5 documents
  the placement.
- RunChat: the chat page is a bounded viewport (height, not min-height) so
  the stream is the scroll container; scroll-to-bottom on the initial window,
  follow-bottom only near the end, manual prepend anchoring for "Load
  earlier" (iOS Safari has no overflow-anchor).
- Ended runs never locate/persist a transcript path — a cwd-match hit would
  belong to a successor run reusing the worktree; they serve the "gone"
  state instead of capturing someone else's transcript.

Concerns:
- Dialog answering: PendingDialog requires the question state (tail dialog);
  the service re-reads the live dialog under a new per-session lock and
  enforces the tool_id match (tool_id mandatory: 400 missing, 409 mismatch or
  no pending dialog). Reply/interrupt serialize on the same lock, closing the
  keystroke-interleave and check-then-send races.
- Multi-select answers validate: empty or out-of-range selections are 400s;
  the Other row is not toggleable in a multi-select (409, deep-link hint).
- Tailer: generation-aware removal (session-name reuse cannot tear down a
  successor's registration) and a periodic resync bounding a dropped
  run.changed.
- Transcript scan errors surface instead of silently truncating the chat.
- A dead tmux session under an active run maps to 409 via the new
  tmuxx.ErrSessionNotFound (real runner + fake parity).
- RunChat: after=<cursor> tail batches make appends gap-free past the window
  limit; a monotonic fetch token drops stale responses; DialogPanel
  selections reset on tool_id change; the panel gates on state==='question';
  chat state resets on route-param change; run.changed is filtered by repo
  (the wire envelope carries no runID).
- State labels: ended-without-transcript reads "gone" (not idle/"available");
  a live run with a gone transcript gets its own read-only copy.

Nits: CRLF replies normalize to LF; the tmux paste buffer is reaped when a
paste fails; the paste integration test pins the ESC[200~ bracketed framing
(the pane enables mode 2004); truncate cuts at rune boundaries; the chat
header shows repo · label and falls back to the generic claude.ai link; 44px
touch targets on tool chips and the back button; "Load earlier" no longer
resurrects after exhaustion; ADR-0016 records the back-patch/cursor tension;
definition-of-done §10 names the real web test files.

nix flake check green: Go suite (real git/tmux/prlimit), golangci-lint,
SPA vitest (294 tests), NixOS module eval.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NP2Ai6CBkYY7Fj1fujtRDD

dominik.polakovics commented 2026-07-07 01:30:25 +02:00

Author

Owner

Copy link

This was generated by AI while landing a PR.

Review findings addressed — fb75ccd

All three blockers, every concern, and the nits from the validation review are fixed in fb75ccd.

Blockers

1. Failed tools rendered "ok" — is_error is now read from the tool_result block (where real 2.x transcripts put it, string content); the compat fixture was corrected to the real grammar (it previously carried the flag in both places, masking the bug) and compat.md §5 documents the placement.
2. No scroll management — .chat-page is a bounded viewport (height, not min-height) so .chat-stream scrolls; scroll-to-bottom on the initial window, follow-bottom only when already near the end, manual prepend anchoring for "Load earlier". Scroll math lives in web/src/lib/chatStream.ts with unit tests.
3. Ended-run transcript capture — ended runs never locate/persist (resolvePath guards on the active outcome); an ended run that never captured a path serves the "gone" state. Covered by TestRead_endedRunNeverLocates and TestAPI_ChatEndedRun_neverLocatedIsGone.

Concerns — dialog answering re-reads the live dialog under a new per-session lock with a mandatory tool_id match (400 missing, 409 mismatch; PendingDialog gates on the question state); reply/answer/interrupt serialize per session; multi-select answers validate (empty/out-of-range → 400, Other row → 409); the tailer's removal is generation-aware and a periodic resync bounds dropped run.changed events; transcript scan errors surface; a dead tmux session maps to 409 via tmuxx.ErrSessionNotFound; the SPA now uses after=<cursor> tail batches (gap-free past the window limit) plus a monotonic fetch token against stale responses; DialogPanel state keys on tool_id; chat state resets on route-param change; state labels fixed (ended-without-transcript → gone; live-but-gone gets its own copy); DoD §10 now names real tests (new InstanceList.test.tsx badge tests, conversation.test.ts, chatStream.test.ts).

One deliberate deviation from the review text: run.changed cannot be filtered by runID — the wire envelope is repo-scoped ({type, repoID} from all three publishers) — so the chat filters by the run's repo instead.

Nits — CRLF replies normalize; the paste buffer is reaped on a failed paste; the paste integration test now genuinely pins the ESC[200~ framing (the pane enables mode 2004); rune-safe truncation; repo-qualified header title + generic claude.ai fallback link; 44px tool chips and back button; "Load earlier" no longer resurrects; ADR-0016 records the back-patch/cursor tension.

Verification — nix flake check green on fb75ccd: Go suite (real git/tmux/prlimit), golangci-lint, SPA vitest (294 tests, 28 new), NixOS module eval.

> *This was generated by AI while landing a PR.* ## Review findings addressed — fb75ccd All three blockers, every concern, and the nits from the validation review are fixed in `fb75ccd`. **Blockers** 1. *Failed tools rendered "ok"* — `is_error` is now read from the `tool_result` block (where real 2.x transcripts put it, string content); the compat fixture was corrected to the real grammar (it previously carried the flag in both places, masking the bug) and compat.md §5 documents the placement. 2. *No scroll management* — `.chat-page` is a bounded viewport (`height`, not `min-height`) so `.chat-stream` scrolls; scroll-to-bottom on the initial window, follow-bottom only when already near the end, manual prepend anchoring for "Load earlier". Scroll math lives in `web/src/lib/chatStream.ts` with unit tests. 3. *Ended-run transcript capture* — ended runs never locate/persist (`resolvePath` guards on the active outcome); an ended run that never captured a path serves the "gone" state. Covered by `TestRead_endedRunNeverLocates` and `TestAPI_ChatEndedRun_neverLocatedIsGone`. **Concerns** — dialog answering re-reads the live dialog under a new per-session lock with a mandatory `tool_id` match (400 missing, 409 mismatch; `PendingDialog` gates on the question state); reply/answer/interrupt serialize per session; multi-select answers validate (empty/out-of-range → 400, Other row → 409); the tailer's removal is generation-aware and a periodic resync bounds dropped `run.changed` events; transcript scan errors surface; a dead tmux session maps to 409 via `tmuxx.ErrSessionNotFound`; the SPA now uses `after=<cursor>` tail batches (gap-free past the window limit) plus a monotonic fetch token against stale responses; DialogPanel state keys on `tool_id`; chat state resets on route-param change; state labels fixed (ended-without-transcript → gone; live-but-gone gets its own copy); DoD §10 now names real tests (new `InstanceList.test.tsx` badge tests, `conversation.test.ts`, `chatStream.test.ts`). One deliberate deviation from the review text: `run.changed` cannot be filtered by `runID` — the wire envelope is repo-scoped (`{type, repoID}` from all three publishers) — so the chat filters by the run's repo instead. **Nits** — CRLF replies normalize; the paste buffer is reaped on a failed paste; the paste integration test now genuinely pins the ESC[200~ framing (the pane enables mode 2004); rune-safe truncation; repo-qualified header title + generic claude.ai fallback link; 44px tool chips and back button; "Load earlier" no longer resurrects; ADR-0016 records the back-patch/cursor tension. **Verification** — `nix flake check` green on `fb75ccd`: Go suite (real git/tmux/prlimit), golangci-lint, SPA vitest (294 tests, 28 new), NixOS module eval.

dominik.polakovics merged commit 20d1189944 into main 2026-07-07 07:48:10 +02:00

dominik.polakovics referenced this pull request from a commit 2026-07-07 07:48:11 +02:00

Merge pull request 'feat(chat): embedded chat — view and reply to instance conversations (ADR-0016)' (#11) from worktree-feat-embedded-chat into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something is broken

  

chore

  

enhancement

New feature or improvement

  

needs-info

Waiting on reporter for more information

  

needs-triage

Maintainer needs to evaluate this issue

  

ready-for-agent

Fully specified, ready for an AFK agent

  

ready-for-human

Requires human implementation

  

wontfix

Will not be actioned

No labels

bug

chore

enhancement

needs-info

needs-triage

ready-for-agent

ready-for-human

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Cloonar/coding-lab!11

No description provided.