feat: share one workspace across several TUIs - #241
Merged
Merged
Conversation
Adds the wire types phase 1 of the multi-client work needs: a client id on attach, batched resize and pane-size frames, client geometry, take control, list clients, and broadcast dismiss/seen marks. A conn can now be held off pane_output, which the daemon uses to deliver a new client's replay and live output exactly once.
The attached-conn set becomes a client registry (clients.go), keyed by conn, with each client's id, first-attach time, RAW window size, cwd, last input time and overlay claims. A size master is elected over it: - The oldest attached client whose raw geometry is paintable (the TUI's 40x10 floor) wins; ties go to the smaller id. A 0x0 or 1x1 attach is never elected, even though handleAttach still defaults clientSize to 80x24. A master shrunk below the floor hands over at once. - A master whose link is LOST keeps its slot for master_grace_minutes (default 3, clamped 0-60), but only while a client attached at the loss is still attached; a lone master is replaced at once. The same id reattaching inside grace takes the slot back with no change. - detach now means a clean exit: the client is removed and the election runs with no reservation. - take_control makes an eligible attached sender the master. - The master id is written to workspace.json as size_master and restored as a min(grace, 30s) reservation, so a daemon restart resizes nothing when the previous master reattaches. Disconnects caused by our own shutdown are not treated as lost links, so the final snapshot still records the master. A master change broadcasts state once, outside the registry lock. pane_input, switch_tab, create_tab, create_pane, update_layout, update_pane and take_control stamp the sender's last input time. Part of #235
resize_pane and the new batched resize_panes share one implementation, applyResizes. It applies a resize only when the sender is the size master, or when no master is elected and no slot is reserved (the single-client behaviour). A refused resize is dropped with no log line. Before any PTY is resized, every follower gets ONE pane_sizes frame for the whole batch on its must-deliver queue, which is drained ahead of pane output, so its VT holds the new size before the child's repaint arrives. A failed Resize sends a second frame with the previous size. A pane named twice in one batch is resized once, to its last size. Workspace-state broadcasts now carry size_master and clients; the workspace.json map still carries neither. A new pane starts at the master's raw window size instead of the last client to attach. Also: attach and client_geometry sizes are clamped to 1000x1000, Stop disarms the grace/reserve timer, and an ignored take_control logs at debug level.
A client attaching while panes write got its OutputBuf replay and the live stream racing each other: live frames landed mid-replay, and bytes written after the replay snapshot arrived twice or out of place. handleAttach now holds the conn off live pane_output (Conn's hold flag) from before the state frame until the replay is queued. Every flush during the hold is copied into the conn's hold with its stream position (Pane.outPos, advanced in the same PluginMu span as the OutputBuf write). The release sends the held bytes through SendBlocking, cutting what an OutputBuf replay already covered, then clears the flag. A ghostsnap or skipped replay records no end, so all held bytes follow it. A pane whose held bytes pass 4 MiB loses them and gets a redraw kick instead; the conn is never closed for it. onClientDisconnect and any early return from handleAttach drop the hold. Two ordering gaps closed beyond the plain hold: - holdGate (RWMutex) makes a flush's hold append and broadcast one step against a hold starting or ending, so a straddling flush is neither sent twice nor lost. - Conn.QueuedOutput and Conn.Done let the hold wait (up to 2 s) for live frames queued before it; the critical-first sendLoop would otherwise write them behind the state frame, mid-replay.
An attach that changed the size master broadcast the workspace state after answering the attach. The attaching conn therefore received two state frames back to back: its own attach state, which is built after registration and already names the new master, and the broadcast, which said nothing new. That was must-deliver queue pressure for no information, and it made two broadcast-counting tests flaky under -race (the second frame landed inside their counting window). The master change now goes only to the other attached clients. A conn that never attached (an MCP bridge) has no use for it either. Part of #235
Review follow-ups for the attach output hold. - A flush can append after the release sees its last empty batch but before the hold ends; finishOutputHold's recheck sends it round again. A beforeFinishHold seam pins it: without the recheck those bytes were lost. - beginOutputHold's drain wait ends on conn close, pinned by closing a client with a backed-up queue; it uses a ticker and one timer. - Flushes skip holdMu when nothing is held (holdCount, changed only under holdGate for write). - dropOutputHold takes holdGate like every other hold change.
The workspace state carries the attached-client count, and each TUI shows [master]/[follower] only while that count is 2 or more. But the other clients were sent a state only when the MASTER changed, so an attach, a detach or a lost link that left the master alone left the count stale on every other TUI: a second TUI attaching did not make the first one show its role. The registry now reports whether the count changed as well as the master. An attach of a new client, a detach and a lost link each send one state frame to the other attached clients, also when the master changed in the same event. Bridges never attach, so they change nothing. A disconnect during shutdown still leaves the registry alone. A same-id reattach inside the grace now sends the follower a state with the new count; it still never changes size_master. Part of #235
Give each tab a LayoutRev counter and gate SetTabLayout on it: a write's BaseRev must match the tab's current revision (or be absent, for an older client) or it is refused outright, with no change to either the layout or the revision. An accepted write bumps the revision, persists in workspace.json, and is broadcast so every other attached client can adopt it. The broadcast is coalesced (broadcast_coalesce.go): a burst of accepted writes - several tabs re-sent after a split-drag release, or several clients editing at once - produces exactly one workspace-state frame at the end of a 50ms window instead of one per write, which is the queue-pressure shape multi-client sync has to avoid on every broadcast path. This also removes the old "no broadcast, to avoid a feedback loop" restriction: with per-client revision tracking a client only ever writes after its own change and only adopts a broadcast that is strictly newer than what it already holds, so echoing an accepted write back to its own sender cannot make it send again.
set_active_pane and close_tui used to broadcast to every attached TUI, which was fine before several TUIs could share a daemon and became "steal another window's focus" or "close somebody else's window" the moment they could. Both now resolve a target conn: an explicit client id from the new list_clients MCP tool, or, implicitly, the client with the most recent input (falling back to the oldest attached client when nobody has typed yet). A headless daemon with no attached client drops the command with a log line instead of sending anything. defaultCWD is now per-client: each attached client's own directory is tried first, then the size master's, then the most-recently-active client's, before falling back to the daemon's own working directory. The old single clientCWD field could not express "which client is asking" once more than one TUI could be attached at once. Dismissing a notification and clearing a pane's unseen mark now broadcast to every attached client, so acting on either in one TUI's sidebar is reflected in a second one instead of leaving a stale card or mark behind. update_pane's automatic reports (an OSC 7 CWD change, overlay visibility, the unseen mark) no longer count as user input for picking the implicit MCP target - only a field the user actually touched does.
defaultCWD probed the same directory up to three times in the ordinary single-TUI case, since the requesting conn, the size master and the most-recently-active client all name the same client there - each probe paid its own spawnDirProbeTimeout and abandoned its own claimBlockingFSCall permit against a dead directory. Candidates are now deduped by path and share one deadline, so three different unreachable candidates together cost no more than one spawnDirProbeTimeout instead of three paid serially. set_active_pane now logs when it is given an explicit client id that is not attached, matching close_tui's existing log line for the same case. Test fixes: the close_tui target test previously let "typed last" and "attached last" coincide on the same client, so it could not tell which one the daemon was actually choosing by; it now separates them, and a new test covers the case where nobody has typed at all. A new test drives create_pane_req from a non-master client to prove the create path (not just the read-only browse path) resolves against the requesting conn, not the master's. The headless-daemon test now reads back the sending conn itself to confirm neither command is echoed to its own sender. list_clients' version-floor test is renamed to match what it actually covers (the daemon capability list, not the floor number), with a new test pinning the floor itself and its allowed-below counterpart. update_pane's stamping test now also covers Muted, Eager, PinnedAttention and MarkedForDeletion, not just Name. Several tests called the deadline-based readFor/roundTrip readers more than once on one conn; a call whose deadline lapses mid-frame discards whatever it had already read, corrupting every later read on that conn. Tests needing more than one checkpoint on a conn now either read once with a decoded assertion that cannot be satisfied by earlier noise, or use a new no-deadline matched reader safe to call repeatedly. Doc comments for SetActivePanePayload.Client, CloseTUIPayload, EventDismissedPayload and PaneSeenPayload no longer describe the old broadcast-to-every-TUI behavior these replaced.
Give the TUI a stable per-process client id (sent on every attach) and track each destination's size master and attached-client count from its broadcasts. A follower stops driving PTY sizes: resizeAllPanes, diffResizes and overlayResizeCmd — the three producers of pane resizes — each gate on isFollower and send nothing for a destination this client does not own, while diffResizes also leaves sizedOnce untouched so a later election still owes every pane its first-resize kick. Batch every resize pass into one MsgResizePanes frame per destination instead of one MsgResizePane per pane, so a window resize or a split-drag release across dozens of panes can never put one must-deliver frame per pane on a follower's queue. Report this client's own raw window size (client_geometry) after every resize pass, to every connected destination, so a master that shrinks below the paintable floor is noticed and handed off. On becoming a destination's master, clear its sizedOnce entries and resize every pane at once, since the sizes a previous master or follower state left behind are not this client's own. Add Take control (client.take_control): an early-tier keymap action with no default binding, plus a palette command, that asks the active destination's daemon to make this client the master immediately. Send a clean-exit detach before closing each connection, so a normal quit does not cost the next master a lost-link grace period. Show a [master]/[follower] marker in the status bar once a destination has more than one attached client.
resizeAllPanes' returned tea.Cmd read m.sizeMaster (via isFollower) from inside the closure Bubble Tea runs on its own goroutine, while applyWorkspaceState mutates that same map in place on the Update goroutine. Go treats a concurrent map read/write as fatal, not merely a -race finding, and this could crash the TUI outright. Compute the per-destination resize batches synchronously before returning the closure, so it only ever touches a fresh, unshared local map. Drop the redundant resizeAllPanes() call from the becoming-master branch in applyWorkspaceState: clearing sizedOnce for that destination is enough on its own, since diffResizes runs immediately afterward, scoped to the same destination, and resends every one of its panes. Calling resizeAllPanes() there was wrong rather than merely redundant, since it walks every destination and would resize ones this broadcast never mentioned. Send each connection's clean-exit detach concurrently and bound the wait at 500ms before closing, instead of sending them one at a time. ipc.Client.Send can block up to 5s against a wedged peer, so detaching several dead remote hosts in sequence could turn quitting the TUI into a multi-second hang; a conn still wedged past the budget is closed anyway. Also: sentCounts (broadcast_echo_test.go) now fails the test on a decode error instead of silently counting it as zero, and a couple of stale comments still naming the pre-batching MsgResizePane are corrected. Tests: a regression test drives resizeAllPanes' returned closure against concurrent size-master writes under -race; a wedged-connection test proves CloseClient no longer blocks on a dead peer; the becoming-master test now asserts exactly one resize frame, addressed to the destination that actually changed; CloseClient's detach test asserts the closer actually ran; a new test covers client_geometry reporting 0x0 below the paintable floor; and a mixed-session test covers being master on one destination while following another.
A follower TUI now sizes each pane's VT to the size the daemon's master chose for it, never to its own box. targetVTSize is the single decision point, used by resizeNode and sizePaneFull (layout leaves, focus mode and overlays); the resize producers keep using paneVTSize, since a master sends its own rect size and a follower sends nothing. A pane with no daemon size yet falls back to its rect. The follower flag and the daemon size reach PaneModel through syncPaneMeta (a new follower parameter) and, between broadcasts, through the pane_sizes frame. The listener decodes MsgPaneSizes beside set_active_pane, and Update applies it synchronously, so the repaint that follows the master's resize lands in a VT of the new size. applyWorkspaceState now records a destination's size master before it rebuilds the panes, or the first broadcast naming another master would still size the VTs by the previous one. previewMode is true for a follower whose grid exceeds its box in either dimension, reusing the preview's left-edge crop and bottom anchor. A grid that fits takes the native path, drawn top-left and padded. The top border marks a cut with a corner replaced by an ellipsis: top-left for hidden rows, top-right for hidden columns. Wheel forwarding to a tracking app maps box coordinates to grid coordinates, and a notch over the padding sends nothing. toggle_wrap now reaches follower panes too.
A follower grid cut in both dimensions must carry the marker in both top-border corners, exactly two of them, with the border still the pane's exact width and the right columns still cropped.
applyResizes announces a batch in pane_sizes before any PTY resize and records Cols/Rows only after each one. A workspace broadcast built in that window carries the old sizes but reaches a follower after the frame, so the follower resized its VT back to the old size with no PTY redraw to pair it, and kept it until an unrelated broadcast. The daemon now numbers every announced size per pane (sizeSeq, taken before the frame leaves and carried on each pane_sizes entry as size_seq) and records which number Cols/Rows hold (colsSeq), which the broadcast reports as size_seq. The two are separate because they are written at different moments: a broadcast built mid-batch carries the old size with the old number, never the new number. A failed resize's rollback takes a new number; a spawn size from newPaneSession takes one too. The TUI adopts a daemon size only when its number is not lower than the one it holds, and forgets the number on reattach, since a restarted daemon counts from 1 again. Tests: the frame carries the number and a mid-batch broadcast is older; the rollback is numbered above the size it undoes; a stale broadcast and a stale pane_sizes leave a follower's VT alone while newer ones still apply; a reattach accepts a lower number. Also: the row-width test's scrolled case now has scrollback and a too-wide but short grid, the sidebar steps run through their real keys, a dead assignment is gone and the applyPaneSizes comment says what happens.
Each tab now tracks the daemon's layout revision. A client sends its tree only when its own user changed it (split fill, own close, arrange, pane-drag drop, border-drag release, a template's first tree), with BaseRev set to the revision it was built on, and adopts any broadcast carrying a higher revision, reusing PaneModels by id. It never sends because the stored tree merely disagrees, so two clients can no longer re-send each other's trees. Arrivals and prunes nobody on this client asked for are placed locally and awaited; only when the next broadcast's stored tree still lacks them does the client send. Adoption cancels a drag armed on the tab and re-seats this client's pendingSplit beside its original sibling. A change made while a write is in flight is held and sent on that write's echo. armReattachReset zeroes the revisions so the daemon's tree wins after a reattach, even at a lower revision. diffLayouts, layoutAgrees, sendAllLayouts and sendTabLayout are gone; every write is marshalled on the Update goroutine.
Adopting another client's tree re-seated this client's pendingSplit placeholder, but the same rebuild pass then pruned it as an unfilled placeholder: pendingSplit pointed at a detached node, the requested pane filled it invisibly and its model leaked. Adoption now reports the re-seat and the pass spares the placeholder, as it does for a moved pane. Also: - the first broadcast after a reattach is adopted whatever its revision (adoptNext), so a rev-0 stored tree is not ignored; - a border click released without motion stores nothing; - close requests are keyed by destination and dropped on reattach; - tests for the replace and spiral-fallback re-seats, and release tests that install their recorder before Update.
An ordinary pendingSplit entry was cleared only by a fill, so when a pass pruned its placeholder the entry stayed: tabLayoutBusy reported the tab busy for the rest of the session, the next fresh arrival filled a node no tree held, and an adoption re-seated the abandoned placeholder as an empty slot. The prune now deletes an entry whose placeholder it detached, with its sibling/direction record, and adoption re-seats only a reservation still in the tree it replaces. Worktree creates keep their exemption.
When another attached client switches this client's active tab, a keystroke or paste already "in flight" must not land in whatever pane the switch happened to make active. Model.requestedTab records the tab id THIS client asked for (per dest+project), via switchTab/ switchTabBy and sendCreateTab's pendingTabCreateToken; applyWorkspaceState compares the daemon's adopted ActiveTab against it to tell a local switch's own echo from a genuinely remote one. A remote change arms remoteSwitchAt/guardPaneID (the previously active pane) and flashes "Tab switched by another client"; enqueueKeyInput (the entry point for typed keys and both paste paths, never wheel input) redirects to that pane for remoteSwitchGuardWindow (250ms) or until it stops existing. A pane focused only by a remote switch also must not read as "seen" on every attached client the instant one of them switches — ackFocusedPane skips its unseen-clearing report while remoteFocusUnacked is set, which Update's prologue clears the moment local input (a key or a mouse click) actually arrives. Also wires the client side of the two small daemon broadcasts this depends on: event_dismissed removes a notification card (or all, when empty) from the sidebar, and pane_seen clears a pane's local unseen mark without echoing anything back.
Review round 1 on the typing-guard commit found four correctness gaps: - requestedTab's token was only retired inside a "moved" branch, so an ORDINARY echo of a local switchTab (fromTab already equals targetTab, since the client's own index moves synchronously) never cleared it. The stale token could then wrongly match a later, unrelated remote switch to the same tab id and suppress the guard. applyTabMoveGuard now runs once per broadcast for the active project, move or not, and retires a matching token unconditionally. - Destroying, moving, or dissolving the active tab locally (Ctrl+W, Move to project, a last-pane dissolve) looked identical to a remote switch away from it, arming a false "Tab switched by another client" flash and an unseen-ack hold on ordinary tab actions. The guard now only arms when the FROM tab is still part of the broadcast's tab list for that project — a vanished source was taken away by this client, not switched away from by another. - A redirected keystroke or paste encoded and answered against the PRE-redirect pane (ResetScroll, answerBlockedByInput, interruptWorkingPane, and pastePayload's bracketed-paste mode) while the bytes themselves went to the guarded pane — crediting the wrong pane with input it never received, and risking an unbracketed multi-line paste into a shell. guardedInputPane resolves the actual target pane once, before any of that runs. - event_dismissed with an empty id (dismiss all) cleared every stored notification regardless of which destination broadcast it, so one daemon's dismiss-all wiped every other attached daemon's cards too. DismissByID now scopes the empty-id case to events whose pane resolves to the broadcasting destination. Also: the create_tab pending token now matches only a genuinely new tab id and is spent after one broadcast either way; sendCreateTab records the token only when the create's destination matches the active project's own; armReattachReset retires a reattaching destination's requestedTab entries and guard state; a lazygit-style overlay's own pane is guarded instead of the tree pane behind it; and a listener decode failure for either new message type falls back to listenContinueMsg instead of propagating a zero-value message.
…rom-tab echoes Two more typing-guard correctness gaps found in review round 2, both in applyTabMoveGuard now that it runs on every broadcast rather than only on a detected move (round 1's fix for the stale-token bug): - The pending create_tab token was deleted unconditionally the moment it was inspected, then checked against existedBefore. An ordinary broadcast that changes nothing (the git ticker, an OSC 7 CWD update, another client's unrelated action) reports the SAME active tab this client was already on, which trivially "existed before" — so it silently spent the token, and the create's own tab landing moments later read as a stranger's remote switch: a false flash and 250ms of redirected typing right after Ctrl+T. The token is now kept whenever the active tab hasn't actually changed, and spent only against a genuine change either way (its own landing, or something else that beat it there). - requestedTab's value is now a small struct (pendingSwitch: target, from, at) instead of a bare tab id. A broadcast already in flight when switchTab runs still names the tab this client just left; with no way to tell that apart from a genuine switch back to it, the tab visibly jumped to the old one for the width of one round trip before the requester's own echo corrected it. Recording the pre-switch tab and a timestamp lets applyTabMoveGuard reject a broadcast naming it outright — holding the active tab at what was requested, arming no guard, keeping the token — for up to requestedSwitchStaleWindow (2s). Past that the local switch is assumed lost and such a broadcast is adopted normally, guard included. applyTabMoveGuard now returns the tab id to actually treat as active alongside the guard cmd, since a rejected stale broadcast must not have its reported ActiveTab adopted into proj.activeTab at all.
Document the multi-client sync feature (Tasks 1-10 on this branch): the client registry and size-master election, the resize batching and per-pane size generation, the output hold on attach, layout sync by revision, the typing guard across a remote tab switch, and the MCP unicast targets and list_clients tool. - .claude/rules/daemon-lifecycle.md: new "Multi-client" section; name the registry (clientRegistry) in "ATTACHED clients vs CONNECTED conns", which used to describe a bare attachedConns set. - .claude/rules/tui-rendering.md: new "Multi-client" section (resize gates and batching, follower rendering, layout sync, typing guard); replace the two "Known limit" passages that described the bug this fixes with what actually landed. - .claude/CLAUDE.md: rewrite the layout-persistence and MsgResizePane invariant bullets for the new behaviour; bump the MCP tool count to 36 and add list_clients and the client field; fix a stale reference to the removed Daemon.clientCWD. - docs/features.md, docs/configuration.md (master_grace_minutes), docs/keybindings.md (client.take_control), docs/mcp.md (list_clients, the client field): user-facing documentation of the same feature. - changelog.d/added-multi-client-sync.md: release notes fragment. Verification: dev.sh test (internal/ipc, internal/daemon, internal/tui, internal/keymap, internal/config, cmd/quil), dev.sh test-race (internal/daemon, internal/tui), dev.sh vet, the integration-tagged daemon suite under golang:1.25, and the changelog fragment gate all pass. dev.sh docs-size reports every file within its limit.
Review found two errors in the previous docs commit and a scope gap: - SetTabLayout lives in internal/daemon/session.go (SessionManager method), not project.go. - sendAllLayouts no longer exists; a split-drag release or an arrangement action only ever marks the ONE tab it changed (finishSplitDrag/applyTabArrangement -> markLayoutChanged). Rewrote the coalescer paragraph in daemon-lifecycle.md, the layout-persistence bullet in CLAUDE.md, and the Split-border drag-resize paragraph in tui-rendering.md around what actually produces a burst: several tabs sent from one client's own broadcast-reconciliation pass, or several clients accepting a write inside the same 50ms window. - Bumped the remaining "35 tools"/"35 MCP" mentions to 36 in docs/roadmap.md, docs/quick-start.md, docs/README.md, docs/prd.md, docs/competitive-analysis.md and .claude/rules/templates.md.
A daemon restart kept the previous master's slot for min(grace, 30 s) with no condition. After an unclean stop (reboot, kill) the next TUI is a new process with a new id, so it sat as a follower for the whole reserve with nobody else attached to protect. AttachPayload gains Reattach. The TUI sets it only on the attach its reconnect path sends. While a restart reserve is active, a FIRST attach from a different id that is the only attached client clears the reserve and is elected at once. Reconnecting clients still respect the reserve, and the reserved id reattaching still reclaims it.
jumpToPane moves this client's active tab for MCP set_active_pane, sidebar clicks, Alt+Backspace, the palette and attention jumps, but it recorded no requestedTab token. A broadcast in flight from before the jump named the old tab and read as another client switching back: the tab jumped back, the typing guard armed and the flash showed. The jump now records the same token switchTab does, with the project's own previous tab as the stale-echo candidate. A jump that keeps the project's tab records nothing, so it cannot overwrite a pending token.
A broadcast naming the tab this client just left is rejected as a stale echo, holding the requested tab. If another client destroyed that tab inside the window, the held id matched nothing and the project fell back to tab index 0, a tab nobody chose. The reject now holds the tab this client is actually on, and only while the broadcast still lists it. Otherwise the token is retired and the daemon's tab is adopted.
TestHold_FlushStraddlingTheBeginArrivesOnce slept 50 ms and hoped beginOutputHold had reached holdGate.Lock by then. Under load it may not have, and the test then passed without exercising the straddle. Poll until holdGate.TryRLock fails instead: the paused flush holds the gate for read, and a pending writer blocks new readers, so a failed TryRLock proves the begin is parked. A read lock the probe does get is released at once.
- eventDismissedMsg: DismissByID now scopes a dismiss-all to the sending daemon; the comment still said the sidebar was unscoped. - ClientInfo.Role: MCP bridges never attach, so they are never listed; the comment said they shared the attach path. - sendStateToOtherClients logged "attach:" from the detach and lost-link paths too; the log prefix now names the caller. - defaultCWD: state the accepted shared-deadline trade-off, where a dead earlier candidate can make a later live one fall through to the daemon's own directory.
- daemon-lifecycle.md: SetTabLayout writes now broadcast through the 50 ms coalescer; document the Reattach flag and the cold-start exit from the restart reserve; record the accepted one-time unpaired VT correction for a follower attaching inside a resize batch. - projects.md: sendAllLayouts is gone; name markLayoutChanged. - tui-rendering.md: a new TUI against an older dev daemon sends only resize_panes, which that daemon drops. - site: the MCP tool count is 36, not 35, in all five places. - features.md and the changelog fragment: note the single-window costs (one coalesced state frame per layout change, and an attach that can wait up to 2 s for a busy output queue to drain).
Alt+Shift+A (jumpToNextBlocked) moves activeTab by hand instead of through jumpToPane, because switchProject does work jumpToPane does not. It recorded no requestedTab token, so a broadcast in flight from before the jump named the old tab and read as another client switching back: the tab jumped back, the typing guard armed and "Tab switched by another client" flashed. Record the token exactly as jumpToPane does, keyed on the project's own previous tab. Correct the two comments that claimed the attention queue already routed through jumpToPane.
A TUI older than multi-client sync sends neither ClientID nor Reattach, so its reconnect after a daemon restart read exactly like a cold start from a new process. Alone on the daemon, it cleared the restart reserve and lost the previous master's slot. The reserve now yields only when the attach carried its own id; an id-less attach waits out the reserve like any reconnecting client.
A destination unreachable at launch gets its FIRST attach through finishReconnect, which always sent Reattach=true. After that host's unclean restart, the lone TUI then waited out the whole restart reserve as a follower. The Model now keeps attachedOnce, a per-destination set written on the Update goroutine by both attach paths (attachAllDests, which adoptDest also uses, and finishReconnect) and never cleared. Reattach is attachedOnce[dest], read before the mark. The AttachPayload comment now states the rule and the ClientID requirement the daemon applies.
The page still said 35 in its summary, its section heading and its acceptance list, and its TUI cooperation table did not list list_clients. Add the row and describe set_active_pane and close_tui as acting on one attached window.
daemon-lifecycle.md said the TUI sets Reattach only on its reconnect path. It now sets it once the process has attached to that destination before, whichever path sends it, and the daemon lets only an attach that carries a ClientID clear the restart reserve.
A dev daemon or dev TUI left running made `dev.sh build` refuse, and each refusal cost a round trip. build and clean now stop the dev variant from this directory first, then run refuse_if_binaries_held unchanged, which keeps the last word. Scope is exactly quil-dev and quild-dev in the project directory, matched by full executable path (Win32_Process via powershell.exe on Windows, /proc/<pid>/exe on Linux, lsof on macOS), never by name, as every worktree has its own quil-dev. The dev daemon stops first and gracefully through `quil-dev daemon stop` with QUIL_HOME set to the project .quil, and only when .quil/quild.pid names a live dev daemon from this directory. What remains is stopped by pid alone, never as a tree. Production quil/quild and quil-debug/quild-debug are never stopped, and ~/.quil is never read. A fast path skips the process listing when both dev files are free, so an ordinary build pays nothing. After a stop, the build waits up to 5 s for Windows to release the image files.
- dev.sh refusal text no longer claims the dev processes were stopped; it says any found were, and keeps the manual daemon stop command. - AttachPayload.ClientID: an anon client with a paintable geometry can be elected; it only cannot clear a restart reserve. - CLAUDE.md and dev-environment.md: the auto-stop also ends quil-dev mcp bridges, can leave a killed dev TUI window in mouse-tracking or alt-screen mode, ends a shell in a pane of this folder's dev daemon, and its macOS branch is best-effort and untested. - Rename the attach test to state the rule: reattach only after this process already attached there.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #241 +/- ##
==========================================
+ Coverage 73.57% 74.34% +0.77%
==========================================
Files 264 269 +5
Lines 38495 39718 +1223
==========================================
+ Hits 28323 29530 +1207
+ Misses 8296 8249 -47
- Partials 1876 1939 +63 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
code-spire-beaver
left a comment
code-spire-beaver
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the multi-client synchronization change at 6e4587c. Three reproducible correctness issues remain: duplicate output across an attach boundary, keyboard redirection surviving explicit local navigation, and reconnect master election using pane dimensions instead of window dimensions. The affected package suites and static checks pass, but their current coverage misses these cases.
Findings: 1 HIGH, 2 MEDIUM. Reviewed head 6e4587cf47 as @code-spire-beaver.
code-spire-beaver
requested changes
Sep 26, 2026
code-spire-beaver
left a comment
code-spire-beaver
left a comment
Collaborator
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Request changes at 6e4587cf4763f9035b4071170beb6b36ab555a91.
Three inline findings remain:
- HIGH — code-quality/H-1: a flush spanning attach can be replayed and broadcast twice; the deterministic scheduling probe receives
ONEONEENDinstead ofONEEND. - MEDIUM — code-quality/M-1: remote switch → explicit local Alt+3 → typing still sends input to the old hidden pane within the guard window.
- MEDIUM — code-quality/M-2: reconnect registers pane-interior geometry as raw window geometry, so a paintable 80×12 master reports 78×8 and loses eligibility.
Validation: all six affected package suites passed (3,752 top-level tests plus 2,274 subtests; nine skipped), and go vet ./... passed. Targeted race checks passed: 126 top-level tests and 24 subtests across daemon/TUI (IPC compiled but the selector matched no tests). No race reports.
The three additional reproductions fail at this head; the output replay reproduction uses an isolated scheduling hook before holdGate acquisition. Checks ran in temporary Linux copies. Explicit integration-tag tests and manual Windows/macOS/SSH terminal sessions were not run. The working checkout and production Quil state were left unchanged.
Agent findings: 0 resolved, 3 open. CI: 9/9 checks passing. Head 6e4587cf47, reviewed by @code-spire-beaver.
Open findings:
- [code-quality/H-1] HIGH — #241 (comment)
- [code-quality/M-1] MEDIUM — #241 (comment)
- [code-quality/M-2] MEDIUM — #241 (comment)
code-spire-beaver
added
the
review: changes-requested
A flush wrote OutputBuf and advanced outPos under PluginMu but took holdGate only later, after the detectors and plugin handlers. A whole handleAttach could run in that gap: its replay already carried the new bytes (end = outPos), its hold never saw the flush, and the resumed broadcast sent the same bytes again to the now-unheld conn. The flush now takes holdGate for read before PluginMu and releases it right after the hold append and broadcast, so publication and delivery are one step against an attach's hold. Lock order is holdGate then PluginMu everywhere. The mouse-mode broadcastState (still decided inside the PluginMu span) and the bell, hand-start, OSC 133 and plugin detectors run after the gate, so it is never held across a spawn. Regression: TestHold_AttachInsideAPublishedFlushGetsItsBytesOnce, via the new afterFlushPublish seam, received "ONEONEEND" before the fix.
A remote tab switch arms a 250 ms guard that keeps typed keys and pastes on the pane the user was in. It survived explicit local navigation: after a remote switch, Alt+3 moved focus to t3 while the next key still went to the pane on t1, and a pane click cleared only the unseen-ack hold. retireTypingGuard clears guardPaneID and remoteSwitchAt at three sites: any mouse click (a click on the remote-focused pane changes no focus, so only this tells that choice apart); a key press whose handling changed the local focus (dest, project, tab, input pane), measured by a defer on Update's named return, which covers pane-navigation keys, the palette, the attention queue and pane history; and switchTab, so choosing the already-focused tab counts. Uninterrupted typing across the remote switch stays guarded. Regression: TestTypingGuard_*RetiresTheGuard (tab key, tab key to the remote tab, pane click on either pane, Alt+Right, project.next), each checking a typed key and a paste.
attachClient read AttachPayload.Cols/Rows as the raw window for master eligibility, but attachMessage sends the pane interior (the window less the sidebar, chrome and borders): a paintable 80x12 window sent 78x8 and was ineligible against the 40x10 floor. The first WindowSizeMsg's client_geometry hid it at launch; a reconnect or runtime host attach sends no geometry after it, so a returning master near the floor lost its slot to a follower, and list_clients showed interior sizes. AttachPayload gains WinCols/WinRows (win_cols/win_rows, omitempty): the raw window, 0x0 below the paintable floor exactly like client_geometry. Cols/Rows stay the first pane's spawn size. The daemon feeds the registry from the raw pair when present and falls back to Cols/Rows for an older client. Regression: TestClientDispatch_ReattachNearTheFloorKeepsTheSlot (master went to B, list_clients showed 78x8) and TestAttach_ReconnectCarriesTheRawWindow.
The mouse-mode note claimed the state frame always reaches the client before the pane chunk. sendLoop prefers the must-deliver queue only when both queues hold a frame at once, so an idle one usually writes the chunk first. State the real cost instead: enabling is unaffected, and disabling leaves a few-millisecond window in which a wheel notch can reach a program that just turned tracking off, accepted as far smaller than the existing 250 ms mouseModeBroadcastCooldown gap. Also move retireTypingGuard's doc comment off guardedInputTarget's, so each function keeps its own. Comment-only; no logic change.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed 26d75d0 against 6e4587c. Verified all three previous findings are fixed and added verification replies to their resolved threads. No new findings in the 11-file fix delta after security, correctness, project-rule and test review. All three affected package suites pass (3,409 tests plus 2,007 subtests; eight skips); focused race checks pass (21 tests plus 5 subtests).
Findings: none. Reviewed head 26d75d0311 as @code-spire-beaver.
code-spire-beaver
approved these changes
Sep 26, 2026
code-spire-beaver
left a comment
code-spire-beaver
left a comment
Collaborator
There was a problem hiding this comment.
✅ APPROVED
Approved at 26d75d031122a3523c0c4131861a67d79b6e3f2c.
All three previous findings are verified fixed: the replay publication race, typing-guard retirement after explicit navigation, and raw window geometry on reconnect. No new findings in the fix delta.
Validation: daemon, TUI and IPC suites passed (3,409 tests plus 2,007 subtests; eight skipped). Focused race checks passed (21 tests plus 5 subtests); go vet ./..., context-document size checks and GitHub CI passed.
Reviewed and tested in an isolated Linux archive; no manual Windows/macOS/SSH terminal run or explicit integration-tag suite. Working checkout and production state unchanged.
Agent findings: 3 resolved, 0 open. CI: 9/9 checks passing. Head 26d75d0311, reviewed by @code-spire-beaver.
code-spire-beaver
added
review: approved
This was referenced Sep 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of the multi-client epic. Several TUIs attached to one daemon now share ONE workspace and stop corrupting each other. The state stays shared (projects, tabs, panes, layout, active tab); the daemon now knows each client and resolves only the parts where clients conflicted.
Ref #235. Part of #234.
What changed
Client identity and the size master (daemon)
internal/daemon/clients.go).[daemon] master_grace_minutes(default 3) while another client attached at the loss is still there. A clean quit sendsdetachand hands over at once.min(grace, 30 s); a newly started lone TUI takes it at once.client.take_control, unbound by default) and palette command.[master]/[follower]while 2 or more clients are attached.Followers (TUI)
…on the top-right corner for hidden columns, top-left for hidden rows) or padded, through the existing preview renderer. The mouse wheel is translated to the master's grid.resize_panesframe per destination; the daemon answers each applied batch with ONEpane_sizesframe to each follower, queued beforepty.Resize, so the follower's grid is resized before the repaint arrives. Per-pane size numbers stop a stale broadcast from undoing a newer size.Clean attach
Layout sync
Shared active tab, safely
MCP
close_tuiandset_active_panenow reach ONE client (an explicitclientid, or the one with the most recent input) instead of every TUI.list_clients(36 tools). It needs daemon 1.80.0 or later.Dev tooling
./scripts/dev.sh buildandcleannow stop this folder's dev daemon (gracefully) and its dev TUIs /quil-dev mcpbridges before building, matched by full executable path. Production and debug binaries are never stopped; the "binary in use" refusal stays as the last check.Docs
.claude/rules/daemon-lifecycle.md,tui-rendering.md,dev-environment.md,.claude/CLAUDE.mddocs/features.md,configuration.md,keybindings.md,mcp.md, roadmap pages, and the site (36 MCP tools)changelog.d/added-multi-client-sync.mdTest plan
./scripts/dev.sh testfor internal/ipc, internal/daemon, internal/tui, internal/keymap, internal/config, cmd/quil./scripts/dev.sh test-racefor internal/daemon and internal/tuigo test -race -tags integration ./internal/daemon/..../scripts/dev.sh vet,docs-size,promote-changelog.sh --validatedev.sh buildstopped a running dev daemon and two dev TUIs, then built; production processes untouched