fix(machine): a workspace owns a pane grid — selecting one switches the whole middle view - #2017
Conversation
Creating an agent terminal was a two-step clunk: open a modal, type a name, pick an agent type, then click the row to assign it into a pane. This collapses create+assign into one act, in the pane the agent will run in. - INLINE PANE PICKER: an empty pane renders "Spawn an agent" — an agent-type select (AGENT_LAUNCH_SPECS) plus an OPTIONAL starting prompt — instead of nothing. Picking spawns at that pane's node scope and binds the session to that pane in one action; the name is auto-minted (agentType + suffix), never asked for. The prompt is typed into the PTY once, on ready, then dropped from the store so a re-mount reattaches rather than retyping it. - AUTO-OPEN ON SPLIT: a split points pendingPickerPaneId at the new pane, so its picker opens focused rather than leaving the user facing a blank rectangle. - NODE-AS-WORKSPACE: grids are keyed per NODE (machine/project/branch), not one per machine — each node has its own persistent pane grid, and re-selecting a node restores it. Extends the existing two-level column/pane reducer; no recursive tree, no replacement. Additive: the session list and AddAgentTerminalDialog still work unchanged (openTerminal keeps its signature and now switches to the session's node). Stripping the sidebar session list is the next sub-task. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adfec6e586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…cus gap onSelectNode wrapped openMachine only to discard the node argument it never used — pass openMachine directly. Hoist the isNodeSelectable predicate out of render. Also documents a real edge the sidebar cannot close on its own: opening a session on the machine you are ALREADY viewing lands the pane but cannot focus the Terminal tab, because MachineView's tabs are uncontrolled. The session is still opened; focusing needs MachineView's active tab to become controlled, which belongs with the follow-up rather than colliding with #2017. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz
Every pane-addressed store action resolved its workspace from `activeNodes` at write time. A pane id is only unique within its node's grid, and the active node can change between a user's action and the write it causes — so a spawn that resolved after the user opened another node (a cold Sprite boot is seconds) ran assignPane against the WRONG grid: no matching pane, write silently dropped, the session row orphaned and the picked pane still empty. Actions now name their node explicitly: bindPaneTerminal derives it from the session's own scope (the session runs in that checkout and was picked in that pane, whatever is on screen when it lands), and split/close/select/dismiss/ clearPrompt take the node their pane was RENDERED for. Two regression tests: a spawn resolving after a node switch still binds to the pane it was picked in, and a prompt delivered to a pane whose node the user has left clears in that node's grid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Self-review caught a defect that undercut the surface's whole purpose: the detail route rendered MachineView inline, but the [machineId] route segment REMOUNTS on every machine-to-machine navigation. MachineWorkspace disposes its workspace on unmount and XtermTerminal tears down its socket, so clicking another machine killed the terminals you left running — on the surface built to keep them. The drive view already solved this: CenterPanel deliberately renders nothing for MACHINE pages and defers to MachineKeepAliveHost (bounded LRU, CSS-hidden when inactive). So the surface now does the same. A new layout above the [machineId] segment renders MachineKeepAliveHost; the detail route renders null (mounting MachineView there too would create a second, competing terminal subtree, exactly as CenterPanel's comment warns). That also fixes how a sidebar session-click lands. It used to author the pane into the workspace store BEFORE the target machine mounted — which cannot survive, since MachineWorkspace rebuilds the workspace on mount and destroys anything written ahead of it (StrictMode's double-invoke makes this bite on the first visit; a remount would do it in prod). The click now records an intent that the layout drains once the machine has a workspace, re-applying if the workspace is rebuilt underneath it and clearing once the session is actually in the active pane — so a stale intent can never clobber the user's later pane changes. The decision is a pure function (resolvePendingSession) with 9 tests covering the rebuild, the clobber, and the navigated-away cases. The shared machine-workspace store is untouched (its synchronous dispose is a tested contract, and #2017 is reworking it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz
…le view
THE BUG: the store held ONE grid per machine and `openTerminal` only overwrote
the ACTIVE PANE, so clicking different sidebar items never switched the middle
view — it swapped the contents of one pane inside a single shared grid.
THE MODEL (replaces the node-as-workspace framing in the previous commits):
a WORKSPACE is a sidebar item that owns its own pane grid. A machine holds many
workspaces; exactly one is active; MachineWorkspace renders the active one's
grid. So selecting a workspace switches the ENTIRE middle view to that item's
combination of terminals — which is the deliverable.
- `machines: Record<machineId, {workspaces, order, activeWorkspaceId}>`, persisted
(a restored grid reattaches to the PTYs still running in it, so the store is no
longer disposed on unmount).
- The two-level column/pane reducer is REUSED per workspace, not replaced.
- Splits land in the workspace they were made in; every pane action names its
workspace explicitly rather than resolving "the active one" at write time (a
write can land after the user switched — a resolved spawn, a `ready` event).
- Nodes (machine/project/branch) are containers again, not the grid-owning unit:
a workspace's scope only says which checkout its agents run in.
Kept working, additively: clicking a session row now opens THAT SESSION'S
workspace (derived id, so re-clicking restores the panes split into it) instead
of overwriting a pane — the sidebar switches the view today, with no MachineTree
change. The picker + one-step spawn survive unchanged.
Also fixes, from an adversarial review pass:
- an oversized starting prompt was silently dropped whole by the bridge
(MAX_INPUT_BYTES); prompts are now chunked on code-point boundaries, and a
multi-line prompt is collapsed so a tty newline can't submit it as two turns
(pure `toPtyInput`, colocated tests).
- the prompt was written the instant the binary was exec'd, before a raw-mode TUI
reads stdin; it now waits for the agent's first output, with a backstop timer.
- a spawn whose pane vanished mid-flight left an orphaned session row; the bind
now reports failure and the caller removes it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
2witstudios
changed the title
An adversarial review pass found three ways the persisted store could hurt a returning user. All are cheap to fix now, while this is the first shipped version of the `machine-workspace-storage` key. 1. A REHYDRATED BLOB WAS TRUSTED. Any future change to WorkspaceState would rehydrate an old shape straight into render (`columns.flatMap` of undefined throws), and `ensureMachine` only checked that the machine KEY existed, so a machine whose active workspace didn't resolve rendered nothing — permanently, since a user cannot clear this storage from inside the app. Now: `version` + `migrate`/`merge` through a pure `sanitizeMachines` that drops anything unrenderable, and `ensureMachine` REPAIRS rather than skips. 2. A STALE PROMPT COULD BE TYPED AT A LIVE AGENT. `pendingPrompt` persisted, and was delivered on any `ready`. Reopen that workspace days later and the agent — running the whole time — would be sent the line plus a carriage return at whatever state it had reached (a y/n confirmation, say). `ready` carrying scrollback means REATTACH, so the prompt is now discarded rather than delivered, and it is stripped from storage on the way back in. 3. NOTHING COULD BE REMOVED. A session deleted server-side left a workspace whose lone pane held a terminal that would never connect again, and `closePane` refused to close a lone pane. Closing a lone pane now DETACHES its terminal (back to the picker), and `removeWorkspace` drops a workspace and shows a neighbour. Also: a persisted `pendingPickerPaneId` made a picker steal the caret on page load; it is transient intent and no longer survives a reload. Tests: XtermTerminal gains a suite (the riskiest code in the PR had none) — cold-boot delivery on first output, the silent-boot backstop, the destructive reattach case, the at-most-once latch, sibling-pane isolation, and no write after unmount. Plus reducer/store tests for sanitize, repair, session recovery, lone-pane detach and workspace removal. 194 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Update: merged master, plus three HIGH-severity fixes from a proactive review passThe branch is synced with 1. A rehydrated blob was trusted as our own state. Any future change to 2. A stale starting prompt could be typed into a long-running agent. 3. Nothing was removable — a workspace could dead-end. A session deleted server-side left a workspace whose lone pane held a terminal that would never connect again, and Also: Tests. Known follow-up (deliberately not in this PR)
|
…-trip The persist config itself was untested — the sanitize tests called the pure function directly, never the middleware. These two go through localStorage and persist.rehydrate(), which is what a returning user's browser actually does: - a blob written by an older, incompatible version comes up USABLE (dropped and rebuilt) rather than throwing at render or rendering nothing; - a blob this version can render comes back with its panes intact so they reattach, minus the transient bits (an undelivered prompt, a pending picker). Also names PERSISTED_VERSION and explains why both migrate and merge sanitize: zustand runs merge on every rehydrate, but migrate only on a version mismatch (and logs an error if it is absent on that path), so they must agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Every grid transition no-ops on a pane id it cannot resolve, so an activePaneId naming a pane that is gone is not cosmetic: showSessionIn would anchor its split on the phantom, the split would quietly do nothing, and the session would never appear — the exact failure that function exists to prevent. Closed at both ends: sanitizeMachines re-points a stored activePaneId at a pane that exists, and showSessionIn falls back to a real pane before splitting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
…ssion where it lives A second adversarial pass found two more ways the starting prompt could go wrong, and one way the sidebar could split a session in two. 1. THE PROMPT CAN NO LONGER CROSS A CONNECT. It was kept in the store until it was actually written, so a pane unmounted mid-boot (the user switches workspace while a cold Sprite boots) carried it to the NEXT connect. That connect cannot be trusted: `ready` with scrollback means reattach, but after a realtime restart the in-memory session map is empty, so a connect to an agent that has been running for hours takes the CREATE path and looks exactly like a cold boot — and the prompt lands, line plus carriage return, in a live agent at whatever state it reached. The prompt is now spent on unmount whether or not it was written: it only ever lands in the boot its own pane connected. A prompt the user has to retype is a far smaller cost than one typed into a running agent. 2. A SPAWNED SESSION OPENS WHERE IT ACTUALLY LIVES. `sessionWorkspaceId` assumes one workspace per session, but split-and-pick binds a new session into a pane of the workspace the user was already in. Clicking its sidebar row minted a SECOND workspace for it — dragging the user out of the grid they built it in, with one PTY claimed by panes in two workspaces. `openTerminal` now finds the workspace already showing the session and selects that, focusing its pane. Also completes the store half of the shared-tree work the spec asks for: `childSessionIds` (sessions that are panes INSIDE a workspace, which the sidebar must not list as their own rows) and `runningPaneCount` (the "N running" count a node shows instead of a session list), with selectors. Tests: the pane→terminal prompt wiring was asserted by nothing (deleting the props kept the suite green); it is now covered, along with prompt-dies-on-unmount, open-where-it-lives, and the child-session derivation. 201 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Second review pass — four more fixes, and two deliberate deferralsA second adversarial pass over the reworked code found six things. Four are fixed in FixedThe starting prompt could still be misdelivered across a connect. It lived in the store until it was actually written, so a pane unmounted mid-boot (switch workspace while a cold Sprite boots) carried it to the next connect. And that connect cannot be trusted to be cold: A spawned session opened in the wrong place. The pane→terminal prompt wiring was asserted by nothing. The The store half of the shared-tree work the spec asks for is done: 201 tests passing, typecheck clean, lint clean. Deferred, on purposeA socket reconnect leaves panes silently dead. The connect effect is keyed
Same for the previously-noted |
…mpt only a fresh boot The previous commit closed the "prompt typed into a live agent" hazard by spending the prompt on unmount. That was wrong in a way that would have bitten the first person to try the feature: React StrictMode (on by default in Next 15) mounts, unmounts and re-mounts every effect in development — so the throwaway unmount would have eaten the prompt every time, and the starting prompt would simply never have worked while developing it. The honest signal exists on the server, so ask for it. `agent-terminal:ready` now carries `resumed`, true when `openShell` picked up a Sprite exec session that was still running. That is precisely the case a client cannot infer: after a realtime restart the in-memory session map is empty, so connecting to an agent that has been running for hours takes the CREATE path and is otherwise indistinguishable from a cold boot. So the client discards the prompt (spends it, never writes it) when the agent was already alive — `resumed`, or a NON-EMPTY scrollback — and delivers it otherwise. An empty scrollback is a reattach to a PTY that has emitted nothing, i.e. the boot this pane is still waiting for (the StrictMode re-mount, and the user who came back a second later), so the prompt survives that. Teardown no longer spends the prompt; it only cancels the pending write. Tests: the resumed contract is pinned on the realtime side (369 passing), and the client covers resumed-looks-cold, empty-scrollback re-mount, printed-scrollback reattach, and unmount (203 passing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Correction to my own last fix — it would have broken the feature in devWorth calling out explicitly, because I got it wrong and caught it before you did. In the previous commit I closed the "stale prompt typed into a live agent" hazard by spending the prompt on unmount. That is wrong: React StrictMode is on by default in Next 15, and it mounts → unmounts → re-mounts every effect in development. The throwaway unmount would have eaten the prompt every single time, so the starting prompt would simply never have worked for anyone developing or testing this feature — while every test still passed, because the tests don't run StrictMode. The honest signal exists on the server, so
The client now discards the prompt (spends it, never writes it) when the agent was already alive — This is the one place I stepped outside the PR's stated file scope ( |
…ion selector looping A third adversarial pass found that the `resumed` flag I added was a DB PREDICTION, not an observation — and three other real defects. 1. `streamSessionId != null` DOES NOT MEAN THE AGENT IS RUNNING. Exec sessions do not survive a Sprite pause, nothing ever clears the column, and `openPtyShell` attaches to the id optimistically — discovering it is dangling only when the socket fails, at which point it quietly launches a FRESH agent (planReconnect). So the row's word for it would tell that fresh agent's pane its prompt had already been taken, and the agent would sit there having never been given its task — silently, in the feature's core flow. The bridge now ASKS the Sprite which sessions it actually has (`isSessionLive`). A listing failure answers "unknown", and unknown counts as running: refusing to type at an agent that turns out to be fresh costs a prompt the user can retype, while typing at one that turns out to be live can answer a confirmation it was waiting on. 2. AN EMPTY SCROLLBACK DOES NOT MEAN THE PTY HAS SAID NOTHING. One chunk larger than MAX_SCROLLBACK_BYTES is pushed and trimmed straight back off, leaving an empty buffer for a session that has been screaming output — which the client reads as "still booting, safe to type". Sessions now carry `hasOutput`, set on the way in, and the reattach path reports `resumed` from that. 3. `selectChildSessionIds` allocated a fresh Set per call. zustand v5 runs the selector inside `getSnapshot`, so that hands React a new snapshot on every read and the consuming component loops. It is cached against the machine state it was derived from (a WeakMap; the store is immutable, so state identity is an exact key). No consumer exists yet — this was a landmine armed for sub-task 3. 4. `openTerminal` focused the home pane by matching the session NAME only, a weaker predicate than the one that found the workspace. It reuses `paneShowing`. 373 realtime tests, 204 web tests, typecheck and lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
One socket carries every pane in the grid, and the client treats an UNTAGGED event as its own — so an untagged `agent-terminal:error` was rendered by every pane at once, covering healthy running terminals with a failure that belonged to one of them. Harmless when a grid held a single pane; this PR makes multi-pane grids the normal case. The connectionId is the client's own and survives a payload the validator rejects for any other reason, so read it straight off the raw payload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Third review pass — the
|
…rites left behind A simplify pass over the touched surface. No behavior change; 204 web tests, 374 realtime tests still pass. - `sessionWorkspaceId` was imported twice and re-exported through an alias that round-tripped to its own name. - `paneShowing` hand-rolled the scope comparison that `isSameNodeScope` already is, which made the store's "the SAME predicate that found the workspace" comment untrue in letter if not in spirit. It now literally is. - `scopeLabelOf` and a pass-through `TerminalPaneState` re-export had no importers. - XtermTerminal's connect comment credited the add-terminal DIALOG with reserving the session row; as of this PR a pane's agent picker is the primary creator. - The pane controls' comment credited 'a touch device has no hover' for keeping them visible; the class list has no pointer query at all — the global [data-pointer='coarse'] rule in globals.css is what reveals them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Reviewing my own last commit: adding `await isSessionLive(...)` between opening the shell and emitting `ready` opened a window the client could lose. `ready` is what carries `resumed`, and the client typed the prompt on the agent's FIRST OUTPUT — which can now beat `ready` to the browser. So a resumed agent's output would arrive, the client would type into it, and only afterwards be told it had been running for hours. Exactly the hazard `resumed` exists to prevent, walked in through the back door of my own fix. Closed at both ends: - The bridge resolves liveness BEFORE the shell opens, so `ready` again leaves with no await between it and `openShell`. - The client refuses to type until `ready` has actually been SEEN, whatever order the events arrive in. Server ordering is not something the client should have to trust. If output already arrived by then and the agent is fresh, the prompt goes in at once rather than waiting out the backstop. Two tests: a resumed agent whose output beats its ready is never typed at, and a fresh one in the same race is prompted the moment ready lands. 206 web, 376 realtime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
…low an untagged error A fourth review pass. Two real holes, both in code I wrote. 1. THE COLD PATH LEARNED THE AGENT WAS ALREADY RUNNING, THEN THREW THE FACT AWAY. `resumed` was verified at create and used only for that one emit; the session itself only carried `hasOutput`. So any connection joining that session BEFORE its first byte landed was told `resumed: false` with an empty scrollback — "a fresh boot, safe to type". A React StrictMode remount does exactly that, and the pane still holds its prompt: it would be typed into an agent that has been running for hours. The session now carries `resumedAtCreate`, and the reattach path reports `hasOutput || resumedAtCreate`. Nothing asserted the reattach emit at all — deleting `resumed` from it kept the whole suite green. Two tests now pin both directions: a resumed-but-silent agent still reports resumed on reattach, and a fresh silent boot still reports fresh. 2. `?? socket.id` MADE AN UNTAGGED ERROR UNREACHABLE. A client's connectionId is a UUID it minted, never the socket's own id — so the fallback matched no pane and the error was swallowed in silence, which is worse than the broadcast it replaced. Left undefined, an untagged failure degrades to the old every-pane behaviour: bad, but not invisible. 376 realtime tests, 206 web tests, typecheck clean on both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
…g session state A fifth review pass. It cleared the ordering fix and the client latches, and found three things worth fixing — two of them consequences of my own last commit. 1. THE LIVENESS CHECK NOW GATES THE SHELL FROM OPENING, AND HAD NO TIMEOUT. Nothing in the `listSessions` chain bounds itself. Before the hoist a stall happened after the PTY was live; now it means no PTY at all, a concurrency slot and a billing hold both held, and — because `finishCreate()` never runs — every later connect for that terminal blocked behind the create claim. A terminal that will not open and cannot be retried is far worse than not knowing whether its agent was running. Bounded at 5s, and a timeout is just another way of not knowing. 2. AN UNKNOWN ANSWER WAS BEING FROZEN INTO SESSION STATE. `resumed` fails SAFE on the wire (unknown ⇒ "assume it is running", so nothing is typed at it), but `resumedAtCreate` is durable state every reattach inherits for the next 30 minutes. One transient 429 would have kept answering with that guess long after the Sprite could have been asked again. Liveness is now `live | gone | unknown`: the wire fails safe, the session records only a definitive positive. 3. THE ORDERING INVARIANT WAS ENFORCED BY A COMMENT. Re-inline the await between `openShell` and `ready` and all 376 tests stayed green — which is exactly how it shipped the first time. An order-log test now fails if it comes back. Test gaps closed: the unmount test asserted the pending write was cancelled but not that the prompt SURVIVES (spending it there kills the re-mount path StrictMode depends on); and the fake socket held one handler per event with a no-op `off`, so it could not hold two panes at once — the very multiplexing `isMine` and the per-mount latches exist for. It now does, and a test mounts two panes on one socket. 379 realtime, 208 web. Realtime coverage gate (98% branches) still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
…nter (#2015) * feat(development): list-machines-in-a-drive query + shared session leaves Foundation pieces for the Development surface, independent of the (pending) route/URL model: - machine-list service (pure, DI'd) + runtime binding + GET /api/machines ?driveId= + useDriveMachines hook — the one net-new query the aggregated tree needs; every other machine service addresses ONE machine by id. - MachineTree: optional machineLabel/defaultExpanded props (both default to today's behavior) so a list of machines can label each tree and start collapsed. - SessionLeaves: extracted from TerminalTab so the Machine page and the upcoming Development sidebar share one session-leaf implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * refactor(machine): TerminalTab consumes the extracted SessionLeaves Completes the extraction across the naming sweep's rename: SessionLeaves now lives at its post-rename path with the machine-workspace store import, and TerminalTab imports it instead of holding a second copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * feat(development): Development surface — nav entry, sidebar swap, aggregated machine tree, routes A top-level command center for machines: a peer to Channels/DMs/Files that swaps the left sidebar to an aggregated Machine → Project → Branch → session tree for the drive, and reuses the Machine page as its detail pane. Routing — drive-in-path + a thin redirect, NOT the sibling two-tree pattern: - ONE real route tree at /dashboard/[driveId]/development (empty state) and .../[machineId] (MachineView as the detail pane). - /dashboard/development is a redirect only: it resolves the active drive from the drive store (the app's existing find(currentDriveId) ?? first fallback) and forwards. No ?driveId= branch, no duplicate page component. - resolveSidebarVariant() replaces MemoizedSidebar's inline ifs, so one DEVELOPMENT_PATH regex covers both URL shapes and the matchers are testable without rendering a sidebar. Reuse: MachineTree and MachineView unchanged in substance; the sidebar hangs the same SessionLeaves off the same tree the Machine page's Terminal tab does. Tests: sidebar-route matchers, the active-drive resolution, the list-machines service, and GET /api/machines. Typecheck + lint + next build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): admin-gate the surface; wait for drives before redirecting Addresses both Codex review threads and the CI audit-coverage gate. P1 — the surface exposed machine structure to non-admins. MachineView refuses to mount its tabs for a non-admin, but the new sidebar happily rendered the tree for any drive member who could VIEW a Machine page, fetching its projects, branches, and terminal sessions from the view-level APIs. Gated in three places: the list route is now app-admin only (and audits the denial), the sidebar passes a null driveId for non-admins so the requests are never made, and the nav entry is hidden rather than pointing at a destination that refuses them. P2 — the driveless redirect raced its own fetch. With a cold store, isLoading is still false on the first render and drives is still [], so anyone with an empty or expired cache was redirected to the drive picker despite having drives. The redirect now waits for fetchDrives() to settle. CI: both failing checks traced to one cause — the new /api/machines route had no security-audit coverage. It now emits an authz.access.denied audit on the non-admin path, so it satisfies the gate with real coverage rather than an allowlist exemption. Also collapsed the sidebar's five stacked && guards into a MachineList with early returns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * refactor(development): drop a redundant callback; document the tab-focus gap onSelectNode wrapped openMachine only to discard the node argument it never used — pass openMachine directly. Hoist the isNodeSelectable predicate out of render. Also documents a real edge the sidebar cannot close on its own: opening a session on the machine you are ALREADY viewing lands the pane but cannot focus the Terminal tab, because MachineView's tabs are uncontrolled. The session is still opened; focusing needs MachineView's active tab to become controlled, which belongs with the follow-up rather than colliding with #2017. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): keep terminals alive across machine switches Self-review caught a defect that undercut the surface's whole purpose: the detail route rendered MachineView inline, but the [machineId] route segment REMOUNTS on every machine-to-machine navigation. MachineWorkspace disposes its workspace on unmount and XtermTerminal tears down its socket, so clicking another machine killed the terminals you left running — on the surface built to keep them. The drive view already solved this: CenterPanel deliberately renders nothing for MACHINE pages and defers to MachineKeepAliveHost (bounded LRU, CSS-hidden when inactive). So the surface now does the same. A new layout above the [machineId] segment renders MachineKeepAliveHost; the detail route renders null (mounting MachineView there too would create a second, competing terminal subtree, exactly as CenterPanel's comment warns). That also fixes how a sidebar session-click lands. It used to author the pane into the workspace store BEFORE the target machine mounted — which cannot survive, since MachineWorkspace rebuilds the workspace on mount and destroys anything written ahead of it (StrictMode's double-invoke makes this bite on the first visit; a remount would do it in prod). The click now records an intent that the layout drains once the machine has a workspace, re-applying if the workspace is rebuilt underneath it and clearing once the session is actually in the active pane — so a stale intent can never clobber the user's later pane changes. The decision is a pure function (resolvePendingSession) with 9 tests covering the rebuild, the clobber, and the navigated-away cases. The shared machine-workspace store is untouched (its synchronous dispose is a tested contract, and #2017 is reworking it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): session clicks were silently dropped by a React lane race Second self-review pass, and this one was load-bearing: the surface's headline flow — expand a machine in the sidebar, click one of its sessions — did nothing at all unless you were already viewing that machine. requestSession() is a plain store write (SYNC lane); router.push() dispatches inside a transition. React commits the sync update first, so there is an intermediate commit holding the NEW intent and the OLD pathname. The drain read that as "the user navigated away" and cleared the intent before the navigation it was waiting for ever arrived. The pure function could not tell the two apart — both look like selectedMachineId !== pending.machineId — and the test suite had encoded the broken policy as intended behavior, which is why it passed. An intent now records the machine that was selected when it was made, so "my navigation hasn't landed yet" (selection still == origin → hold) is distinguishable from "the user chose a third machine" (→ drop). Two further fixes to the keep-alive wiring: - MachineKeepAliveHost takes an optional machineIds list. The drive view infers machines from the page tree; this surface KNOWS them (/api/machines). The two sources disagree — a machine absent from the tree (failed tree fetch, or a private machine granted via a custom drive role, which the tree endpoint does not resolve) was treated as trashed and evicted from the LRU on the next machine switch, disconnecting a live terminal. Passing the list also skips the page-tree fetch this surface has no other use for. - The detail pane no longer goes silently blank: a machine still mounting shows "Opening machine…", and an unknown/deleted machine id shows "Machine not found" instead of an empty region (both the host and the route render null in that case). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): expire session intents; honour fetch errors; make the shared host diffable Third self-review pass. 1. A session intent could leak out of the surface and hijack a pane later. It had no terminal state: returning to where you started left it parked forever, and the store is a module singleton, so it survived leaving Development entirely. Coming back to that machine — warm, with a terminal running in the active pane — fired the stale intent and overwrote it. Intents now carry a createdAt and expire (PENDING_SESSION_TTL_MS); the surface clears any unconverged intent on unmount; and picking a machine ROW (rather than one of its sessions) clears one too, since that says "this machine as it is". Dropping fromMachineId in favour of the TTL also fixes a second silent drop: two quick session clicks on different machines used to destroy the second intent when the first navigation committed. A mismatch now WAITS, bounded by the TTL, instead of guessing at the user's intent from a single commit. 2. "Machine not found" was shown over a perfectly good machine whenever /api/machines failed: SWR reports isLoading:false with data undefined on the error path, which is indistinguishable from "no such machine" unless the error is checked first. The detail pane now checks error first, and gates its fetch on isAdmin like the sidebar (a non-admin was firing a request that 403s and audits on every load). The sidebar no longer asserts "not an admin" before auth has resolved — that flashed the refusal at real admins on cold loads. 3. MachineKeepAliveHost.tsx carried a literal NUL byte (pre-existing on master), so git classified it as BINARY: every change to it renders as "Bin N -> M bytes" with no hunks — which is exactly how this PR's edit to a file SHARED with the drive view escaped two review passes. The NUL is now written as an escape (same value), and .gitattributes forces textual diffs on source, so that class of mistake is cosmetic instead of review-defeating. The edit is now a readable 29/6-line diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): make a session click actually reach the terminal Fourth self-review pass, and it turned the PR's one "known gap" into a bug I had mis-described. Only the Terminal tab mounts a machine's workspace (MachineWorkspace lives inside TerminalTab, and Radix unmounts inactive tab bodies). So clicking a session leaf for a machine parked on Code/Diff/Settings — a warm machine in the keep-alive LRU keeps whatever tab you left it on — had nowhere to land. I had documented this as "the session lands in the pane but stays behind that tab". It does not land at all: the intent waits for a workspace that never appears and the TTL discards it. A silently dead click, on the surface's primary interaction. MachineView's active tab now lives in a store (useMachineTabStore) instead of being uncontrolled Radix state, which makes "show me this machine's terminal" something another surface can ask for. The sidebar focuses the Terminal tab before navigating, so the workspace mounts and the session lands. Behaviour is otherwise unchanged: a machine with no stored tab shows Terminal exactly as before. Also: the detail pane was missing the sidebar's auth-loading gate, so an admin refreshing the page was told "Machine access requires administrator privileges" until the session fetch returned (`role` is not persisted across a reload). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * test(development): component tests for the sidebar; isolate the tab store in MachineView's I had claimed React component tests can't run in a .pu worktree. That was wrong: they fail only when vitest is invoked from the repo root (dual-React resolution). From apps/web they run fine — so the components are now actually covered rather than merely typechecked. Adds DevelopmentSidebar tests for the wiring that broke twice in review: a session click must focus the machine's Terminal tab (only that tab mounts a workspace, so otherwise the click lands nowhere), record the intent, and navigate — plus the admin gate, the no-fetch-for-non-admins path, and the no-refusal-before-auth -resolves case. Also resets the new tab store in MachineView.test's beforeEach: it's a module singleton, so a test that switches tabs would otherwise leave the next one parked on that tab. Passes today only by test ordering; that's a landmine. 990 tests green across every touched area (sidebar, machine page-views, stores, lib/development, api/machines, the audit-coverage gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * refactor(development): drop the unenforced TTL; harden the machine set, route, and tests Acting on a fresh-eyes review pass. - The session-intent TTL was a lie. resolvePendingSession only runs from an effect, so when it returned 'wait' nothing re-triggered it — there was no timer, and the doc comment ("past the TTL it is simply dropped") described behavior the system did not have. Its test passed by calling the pure function with an advanced clock and would have passed with the whole drain hook deleted. The leak it claimed to guard is already closed twice: the layout clears on unmount, and picking a machine row clears too. Deleted the TTL, createdAt, and the `now` parameter. - A machine can vanish from /api/machines WITHOUT being deleted: the per-page permission check swallows DB errors and reports "cannot view". The host treats that list as authoritative and evicts anything missing — so a transient hiccup would unmount and DISCONNECT the terminal the user is watching. Machine ids are now sticky within a drive (add-only; reset on drive change), so a live terminal can't be evicted by a blip. A genuinely deleted machine ages out of the bounded LRU instead. - GET /api/machines had no error handling: a DB failure produced an unlogged Next 500. Wrapped, logged like every sibling route. - The non-admin sidebar test would have passed with the client-side gate REMOVED (the refusal notice short-circuits the list, so no machine renders either way). It now asserts the gate itself — useDriveMachines called with null, never with the driveId — which is the actual security property. - One matchMedia listener for the sidebar instead of one per machine; corrected two comments that oversold what they defended (the index covers the scan, not the per-page permission fan-out; updatedAt is served, not ordered on). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * refactor(development): derive the sticky machine set with the repo's own render pattern useStickyMachineIds mutated a ref during render. It happened to be safe (the union is monotonic and idempotent), but it is an impure render, and the codebase already has a sanctioned idiom for exactly this shape: MachineKeepAliveHost derives its LRU with the "adjust state during render" pattern guarded by a key. Matched it — state, unlike a ref, is discarded when a concurrent render is abandoned, so an interrupted navigation cannot leave behind a machine set that was never committed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): a vanished machine stops being shown without evicting it; test the layout The sticky machine set I added to stop a fetch blip disconnecting a live terminal over-corrected: because it never shrank, a DELETED machine stayed mountable and "Machine not found" became unreachable — the user would sit on a permanent "Opening machine…" over a MachineView whose own API calls were 404ing. Those are two different questions and they now get two different answers. What is DISPLAYED comes from the latest fetch, so a machine that's gone stops being shown at once. What may stay MOUNTED comes from the sticky set, so a machine that drops out of a fetch without being deleted (the per-page permission check swallows DB errors and reports "cannot view") keeps its terminal alive, hidden, until the bounded LRU ages it out. A blip now costs a transient notice, never a dead session. Also adds the layout's first test file. It's the newest and most delicate code on the branch — setState-during-render, the error-before-not-found ordering, the unmount clear — and every review pass kept finding bugs in it while every piece it composes was already tested. The tests pin what actually broke: the sticky set converges (a key derived from array identity rather than contents would loop forever, since SWR hands back a fresh arrayevery render), its identity is stable across a no-op revalidation (a new identity reads as a changed machine set, i.e. LRU eviction, i.e. terminal teardown), a failed fetch says "failed" rather than "deleted", and a vanished machine is un-displayed but not evicted. Corrects the route comment that claimed a system-wide property: the sibling machines/* routes are view-gated, not admin-gated, so this route is stricter than they are rather than closing a hole they leave open. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): make the machine list actually recover; fix a test that proved the opposite of its name Two defects in my own previous commit, both caught by review. 1. I claimed a fetch blip "costs a transient notice". Nothing made it transient: useDriveMachines had revalidateOnFocus:false, no refreshInterval, and its mutate is never called — so the list was fetched once per mount and never again. A machine silently dropped by the swallowed-permission-error path would therefore stay hidden for the rest of the session, and both ways out (reload, or leave the surface and return) unmount the keep-alive host and disconnect every warm terminal — destroying the very thing the sticky set exists to protect. The list now polls, so it recovers on its own (and picks up machines created elsewhere). SWR keeps the previous array identity when the ids are unchanged, so a poll that changes nothing doesn't churn the LRU. 2. The "a machine that vanishes is NOT evicted" test used cleanup() + render, which builds a FRESH component whose sticky set is rebuilt from the now-empty fetch — so the machine WAS evicted, and the test asserted only activePageId. It would have passed with useStickyMachineIds deleted entirely. It now rerenders the same instance and asserts the machine is still in the mountable set, which is the property it names. (The production code was right; the test was not.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): a failed poll must not tear down a working machine list Regression from the previous commit, caught in review. Adding the poll changed what `error` MEANS: SWR keeps the last good data and sets `error` on a failed REVALIDATION, whereas before (fetch-once) an error implied no data. Both surfaces still checked `error` ahead of the data, so a single blip of a background poll would replace the whole sidebar tree with "Failed to load machines" — losing every machine's expansion state and the session leaves under it — while the app was holding a perfectly good list. And SWR suppresses the refresh interval while an error is set, so it sat there through the retry backoff rather than recovering. The error notice is now shown only when the failure left nothing to show. Pinned by tests on both surfaces (stale data + error → the machine still renders, no error notice). Also corrects the hook comment, which named the wrong mechanism: SWR preserves the array identity only when the whole payload is deep-equal, and `updatedAt` moves whenever a Machine page is touched — so a poll DOES hand back a fresh array. What actually keeps it from churning the keep-alive LRU is that both consumers key on the IDS alone. And notes that dropping `revalidateOnFocus: false` was deliberate: returning to the tab now recovers immediately instead of waiting out the interval. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): the last raw NUL byte — in the file that diagnosed the problem Writing useStickyMachineIds I copied the key idiom from MachineKeepAliveHost, including its literal NUL separator — the very defect this branch added .gitattributes to expose. So the surface's own layout carried a raw NUL while the commit that removed one from the host was still fresh. Not a runtime bug (NUL is a fine separator), but it meant .gitattributes was MASKING the problem rather than the source being clean: remove that file and layout.tsx goes binary to git — no textual diff, no three-way merge. Now the escape, matching the sibling. Byte-scanned every tracked source file under apps/web, packages/lib, and apps/realtime: zero raw NUL bytes remain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * fix(development): never open a session into a machine the host is keeping hidden The drain gated on what the URL SELECTS while the keep-alive host gates visibility on what it can DISPLAY. Those disagree exactly when a machine is transiently missing from /api/machines — the case this surface already goes out of its way to survive. In that window every pane is `display:none`, and opening a session there mounts an xterm inside a hidden container: fit() measures a zero-sized box and the PTY is created at a bogus geometry, wrapping its output for the life of the session (it recovers visually on the next show, but the mangled history doesn't). Both now derive from one value, `displayedMachineId`. The intent is simply held until the machine is displayed again, which is what the convergent drain is for. Found by reviewing this branch against the sprites/terminal work just merged from master (#2013 scrollback-replay suppression, #2020 re-auth) — that merge is behaviorally clean, but checking it against the keep-alive lifecycle surfaced this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz * polish(development): pin the positive half of the display gate; tidy names and tests - The drain's parameter was still called selectedMachineId while it now receives displayedMachineId. Renamed to what it is. - Adds the test the last fix was missing. Gating the drain on what's DISPLAYED could plausibly have turned "hold" into "drop", so the positive path is now pinned at the composition level: an intent for a machine that isn't in the list yet WAITS, and lands in the active pane as soon as the machine appears. - Folds two duplicate cases out of pending-session.test.ts (identical inputs and expectations to the tests above them — no coverage lost). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Master merged the Development surface (#2015) while this branch was in flight, and it reads the workspace store — against the OLD shape. A textual merge left it compiling against `state.workspaces[machineId]`, which no longer exists: a machine now holds MANY workspaces (each sidebar item owns one) plus a pointer to the one on screen. The pending-session drain wants the machine's ACTIVE workspace — the grid the middle view is actually showing — so it uses `selectActiveWorkspace`. Its convergence condition is unchanged and still correct: the intent clears once the session it names is in the active pane of the workspace on screen, which is exactly what `openTerminal` now brings about (it selects the workspace the session lives in). Also corrects two comments this branch made false: both claimed `MachineWorkspace` disposes its workspace on unmount and rebuilds it on mount. It no longer disposes — the store is persisted precisely so a grid survives navigation and comes back reattached to its PTYs. The intent still converges rather than firing once, which is what makes it survive a remount; that reasoning holds, the mechanism it cited does not. 271 tests across development + machine + stores, typecheck and lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
…metry backwards My last commit recorded `resumedAtCreate: liveness === 'live'`, reasoning that an UNKNOWN answer should not be frozen into session state. That was backwards, and it reintroduced the exact hazard the field exists to close. The reattach path cannot express "unknown" — it re-derives `resumed` from this field. So a live agent whose listing 429'd got `resumedAtCreate: false`; in the window before its first byte `hasOutput` is false too, and a pane re-mounting there (carrying the prompt its torn-down mount deliberately never spent) was told "fresh boot, safe to type" — and typed a line plus a carriage return into a running agent. The asymmetry decides it, in BOTH places: an unknown recorded as resumed costs a prompt the user retypes, and stops costing anything the moment the agent speaks and `hasOutput` takes over. An unknown recorded as fresh costs a line typed into an agent sitting at a confirmation. `resumedAtCreate: resumed` — the durable verdict fails safe exactly as the wire does. The ordering test also only pinned the HOIST (listSessions before openShell), not the invariant it claimed: "no await between openShell and the ready emit". Inserting `await Promise.resolve()` in that 120-line span left all tests green — which is how this regressed once already. It now asserts the EMIT order against a shell that replays scrollback the instant it opens, and I mutation-tested it: the await fails exactly one test. 381 realtime tests, coverage gate still clears 98% branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Passes 5–7: the starting prompt took three more corrections, two of them to my own fixesRecording these because the pattern matters more than any individual fix: this feature has a hostile failure mode — a line plus a carriage return delivered to an agent sitting at a
|
…ct it A seventh pass — a whole-invariant audit rather than a diff review. It cleared the previous commit and found that the safety property still broke on one path, plus a session leak my own added await had widened. 1. `resumed: false` WAS A PREDICTION, NOT A FACT. The `gone` verdict gated the prompt but did not constrain the attach: `openShell` was still handed the stored `streamSessionId`, and `openPtyShell` attaches to it OPTIMISTICALLY, never consulting the verdict. Lose that bet — a listing that omits a session `attachSession` then binds to — and the bridge is attached to a LIVE agent having just told the client it was safe to type into. `resumed` is the only defence on that path. Now `gone` makes ITSELF true: no id, a genuinely fresh session, and the prompt is correct by construction. `live` and an unsettled `unknown` still attach, because abandoning a running agent to start a second one is the worse error — the same policy `planReconnect` already applies. 2. A PANE THAT LEFT DURING A COLD CREATE LEAKED ITS SESSION FOREVER. The disconnect arrives before the connect has registered anything to disconnect (the Sprite is still being resolved and woken — a window my liveness await widened). It was dropped, so the create finished into the void: a live PTY with no viewer, never detached, so the idle reap that releases the concurrency slot and settles the billing window never armed. An agent CLI sits at its prompt forever, so nothing else collects it — it runs for the life of the process, and on the free tier (one terminal) that locks the user out. The socket now remembers a disconnect that lands mid-create and honours it the moment the session exists. 3. A prompt was spent on the EMIT, not on delivery. A disconnected socket buffers the emit and flushes it on reconnect carrying a connectionId the server no longer knows, so it is dropped there — while the prompt had already been thrown away here. It is no longer spent when the socket is down. 4. The spawn API is an upsert and RETURNS `resumed` when it hands back a session that already existed. The picker ignored it and bound the prompt anyway; the invariant was resting on the auto-name's entropy instead of the answer sitting in the response. It now honours it. Two existing tests asserted the optimistic attach against a Sprite whose session list was empty — i.e. they encoded the bug. They now express real continuity: the Sprite still HAS the session, so the reattach happens for the right reason. 770 realtime, 1026 web. Coverage gate holds. The leak fix is mutation-tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo
Pass 7 — a whole-invariant audit, not a diff review. Two more real bugs, one of them production-affecting.I asked the seventh pass to enumerate every path by which a prompt can reach a PTY write and check each against the safety property, rather than review the latest diff. That is what surfaced these; a diff review would not have. The safety property broke on a path I had reasoned was safe
A pane leaving during a cold create leaked its session permanentlyThis one affects production regardless of the prompt feature. The An agent CLI sits at its prompt forever, so nothing else collects it — the PTY, the slot and the billing heartbeat run for the life of the realtime process. On the free tier (one terminal) that locks the user out of agent terminals on that replica until it restarts. Closing a tab or switching workspace mid-cold-boot is enough; StrictMode's double-mount does it on every first mount in dev. The socket now remembers a disconnect that lands mid-create and honours it the moment the session exists. Mutation-tested: disabling the fix fails exactly that test. Two smaller ones
Two existing tests had encoded the bugThey asserted the optimistic attach against a Sprite whose session list was empty — i.e. they asserted that we attach to an id the Sprite does not have. They now express real continuity: the Sprite still has the session, so the reattach happens for the right reason. 770 realtime, 1026 web, coverage gate holds, typecheck clean. Scope note for the reviewerThis PR now touches |
…reate An eighth adversarial pass found the previous commit's leak fix covering one of three paths into a bound session, and the two it missed are worse than the one it caught. A connect that JOINS a create already in flight (a double-mount: the same terminal open in two panes) was in NEITHER set, so a disconnect for it was dropped — and when the create landed, its attach CANCELLED the idle reap the fix had just armed for the creator. Net effect: close the tab mid-boot and the PTY, its concurrency slot and its billing heartbeat run for the life of the realtime process, which is precisely the leak the fix exists to prevent. The reattach fast path had the same hole one step earlier: a pane can leave during its access check, and its attach then resurrects a session nobody is watching. So the window is now the WHOLE of `onConnect` — validation to bound session — and every path that binds one settles the abandonment (`settleAbandon`). The connect body moves into `establishConnection` so a single try/finally owns it. Also: the spawn's cleanup path called `removeAgentTerminal`, which KILLS the terminal, without asking whether this spawn had created it. `spawnAgentTerminal` is an upsert — on a `resumed` session that pane-vanished cleanup was destroying an agent that may be mid-task in someone else's pane. All three fixes are mutation-tested: reverting each fails exactly its own test. 772 realtime tests, typecheck clean.
Review pass 8 — the leak fix covered one of three pathsThe previous commit fixed a session leak when a pane leaves during a cold create. An eighth adversarial pass found two sibling paths it missed, and both are worse than the one it caught. Fixed in 1. A connect that JOINS an in-flight create was in neither set — and its attach un-did the fix. Two panes on the same session key (a double-mount) means one creates and one waits. The waiter was in neither 2. The reattach fast path had the same hole one step earlier. A pane can leave during its access check (a DB round-trip). Its attach then cancels a pending reap on behalf of a socket that is already gone, resurrecting a session nobody is watching and that no further disconnect can collect. The fix is structural rather than another special case: the abandon window is now the whole of 3. The spawn cleanup killed a terminal it did not create. All three are mutation-tested — reverting each fails exactly its own test and nothing else. 772 realtime tests, 18 TerminalPanes tests, monorepo typecheck clean. CI was green on the previous commit; this one is running now. |
…h and undo The previous commit settled an abandoned connect the same way on every path: bind the session, then tear it down. That is right for a connect that CREATED the session — nobody else is watching a PTY that did not exist a moment ago — and actively harmful for one that was about to ATTACH. `attachToLiveSession` STEALS the session: `sessionMap.reattach` drops the previous owner's socket entry and re-points the PTY's output at the new pane. So a pane that closed while its access check was in flight would take a LIVE pane's terminal away from it — that pane goes blind, its input goes nowhere — and then arm the reap that kills the PTY 30 minutes later, with the user still watching it. The previous commit turned "the second pane goes blind" into "the first pane's agent is killed". An abandoned connect now declines to attach at all, leaving the session exactly as it was: with its live viewer, or with the reap it already had ticking. Also: `connectionId` is client-minted and the whole lifecycle is keyed on it, so a second concurrent connect reusing one defeats the bookkeeping — the first connect's `finally` clears the abandon mark the second relies on, and `setNew` overwrites the socket entry of a still-running session, orphaning its PTY, its concurrency slot and its billing heartbeat for the life of the process. That bill is the MACHINE owner's. A reused id is now refused. Both mutation-tested: reverting each fails exactly its own test. 774 realtime tests, typecheck clean.
Review pass 9 — the previous fix made the bug worse on two of its three pathsPass 8 settled an abandoned connect the same way everywhere: bind the session, then tear it down. That is correct for a connect that created the session and actively harmful for one that was about to attach. Fixed in
Second finding, and a real trust-boundary gap: Both are mutation-tested — reverting each fails exactly its own test and nothing else. 774 realtime tests, typecheck 16/16. Three passes in a row have now found a real defect in the previous pass's fix, each time because a fix was applied to one path of several, or predicted a state instead of forcing it. I'd rather that history be visible than tidied away. |
…ever boot an agent for a pane that left Three findings from a tenth adversarial pass, two of them defects in my own previous commit. 1. The duplicate-connectionId guard was scoped PER SOCKET, but the invariant it protects is SERVER-GLOBAL. `agentTerminalSessionMap` is one shared instance filed under the bare, client-minted `connectionId`, so a SECOND socket picking the same id — validated only as a non-empty string — displaced the first session's socket entry: no viewer, no armed reap, its PTY, concurrency slot and billing heartbeat running for the life of the process, billed to the MACHINE's payer. Worse, the first socket's later disconnect then resolved to the SECOND socket's session and reaped it, killing a terminal someone else was watching. The guard could never have caught this — it only ever saw its own socket's ids. The viewer key is now namespaced with the server-assigned socket id, so a client can only name its own connections. Collision is unrepresentable, not merely detected. 2. The cold-create path still bound and undid: it booted the agent, took the concurrency slot and a billing hold, and only then armed a 30-minute reap. Safe, but not free — a pane closed during a cold boot billed the machine's payer for thirty minutes of Sprite runtime for an agent nobody ever saw, and locked a free-tier user (one terminal) out of their own machine for half an hour. It now declines before `openShell`, releasing the slot and the hold at once. 3. `openShell` is SYNCHRONOUS, so nothing can be abandoned between that decline and the session being installed — which makes `settleAbandon` dead code. Removed, along with the test I had written for a window that cannot exist. Also: my duplicate-id test was vacuous. Its `checkAuth` fixture hard-coded one sessionKey, so the second connect took the reattach path and opened no shell with OR without the guard — it asserted the error string, not that harm was prevented. Rewritten with per-target keys; it now fails on revert for the right reason. All three fixes mutation-tested. 775 realtime tests, typecheck 16/16, lint clean.
Resolves conflict in apps/web/src/app/dashboard/[driveId]/development/layout.tsx against master's #2017 (workspace-owns-pane-grid): kept this branch's extraction of useStickyMachineIds/useDrainPendingSession/DetailState into shared apps/web/src/lib/development/ files, and ported master's updated workspace lookup (selectActiveWorkspace instead of a flat workspaces map) into the shared use-drain-pending-session.ts hook so both the drive-scoped and new global layout pick it up. Also updated the new global layout test to reset the store via the new machines shape.
Fixes from an 8-angle automated code review plus a Codex review comment: - listMachinesAcrossDrives: one drive's scan failing (or its per-page visibility check throwing) no longer 500s the ENTIRE global view -- Promise.allSettled + drop the failed drive, matching this file's own existing "swallow and hide" philosophy for per-page permission-check failures. New tests cover both failure points. - sidebar-routes.ts: fixed a stale DEVELOPMENT_PATH doc comment that still described the driveless route as "which redirects" with "no driveless twin" -- both were made false by this PR. - Extracted the duplicated isKnownMachine/displayedMachineId derivation (present in both layouts, previously copy-pasted) into a shared resolveDisplayedMachine, completing the extraction this PR already did for useStickyMachineIds/useDrainPendingSession/DetailState. - Centralized the machine detail-URL builder (buildMachineHref) alongside the existing parseSelectedMachineId in development-route.ts, so the parse and build sides of the URL shape can't drift apart. MachineTreeSection now takes a driveId again instead of an ad hoc basePath string. - Deduped the near-identical guard-chain early-returns in DriveMachineList/ GlobalMachineList into a shared resolveListNotice helper. - Deduped the byte-for-byte identical fetcher/globalFetcher bodies in useDriveMachines.ts into one generic machinesFetcher<T>. - Dropped the GLOBAL_SCOPE magic-string sentinel in favor of passing undefined to useStickyMachineIds, matching the hook's own "no scope" idiom. - Documented (rather than attempted to fix, as out of scope for this PR) a known tradeoff the global layout's route-tree separation doesn't solve: navigating from the global Development view into a specific drive's Development view crosses route trees and disconnects every warm terminal across every drive, not just the one being left. This mirrors an already-accepted tradeoff elsewhere in the surface (switching drives already tears down the other drive's terminals) and is recoverable (reconnects on remount), not data loss. Also merged origin/master (bb72a70..b7859dc) to resolve a real conflict: master's #2017 changed [driveId]/development/layout.tsx's pending-session workspace lookup from a flat workspaces map to selectActiveWorkspace (the new workspace-owns-pane-grid model). Ported that into the shared use-drain-pending-session.ts hook so both layouts pick it up. All targeted tests green (232 tests across 16 files), full monorepo typecheck+build green, lint green (no new warnings).
…#2027) * fix(development): make driveless /dashboard/development a GLOBAL view The Development surface's driveless route redirected to a resolved "current/last" drive instead of showing everything, per the routing correction in the Development surface spec. It's now a real global command center: all machines across all drives the user can access, grouped by drive. - packages/lib/services/machines/machine-list: add listMachinesAcrossDrives, grouping listMachinesInDrive's per-drive result by drive and dropping drives with no visible machines. - apps/web api/machines route: no-driveId request now returns { drives: [...] } instead of 400, under the same app-admin gate + per-page canUserViewPage filtering as the per-drive path. - machine-list-runtime: listAllMachines(), sourcing accessible drives from listAccessibleDrives (the same drive universe GET /api/drives hands DriveSwitcher). - useDriveMachines: add useAllMachines(enabled), same SWR convention. - DevelopmentSidebar: branches on driveId presence — drive-scoped list unchanged, new grouped-by-drive list in global mode. MachineTreeSection now takes a basePath so global-mode machines route to /dashboard/development/{machineId} rather than crossing into a specific drive's route tree. - New /dashboard/development/layout.tsx + [machineId]/page.tsx: the global twin of the drive-scoped detail region, sourced from useAllMachines(). Kept as its own route tree (not merged into the drive-scoped layout) so opening a machine from the global list never crosses into /dashboard/{driveId}/development and tears down the keep-alive host. - Extracted useStickyMachineIds, useDrainPendingSession, and DetailState/DetailNotice out of the drive-scoped layout into lib/development/ so both layouts share the exact same logic instead of duplicating it. - Removed the now-dead resolveActiveDriveId redirect helper. Colocated tests cover the new grouping/filtering logic, the API route's global branch, the global layout, and the sidebar's global mode. * fix: replace raw NUL byte with escape sequence in sticky-machine-ids key The key template literal accidentally embedded an actual NUL byte (introduced during extraction from the per-drive layout) instead of a textual escape. Same runtime string, but a raw NUL byte in a .ts file confuses editors/diff/search tooling. Flagged by Codex review on PR #2027. * fix(review): address code-review findings on the global Development view Fixes from an 8-angle automated code review plus a Codex review comment: - listMachinesAcrossDrives: one drive's scan failing (or its per-page visibility check throwing) no longer 500s the ENTIRE global view -- Promise.allSettled + drop the failed drive, matching this file's own existing "swallow and hide" philosophy for per-page permission-check failures. New tests cover both failure points. - sidebar-routes.ts: fixed a stale DEVELOPMENT_PATH doc comment that still described the driveless route as "which redirects" with "no driveless twin" -- both were made false by this PR. - Extracted the duplicated isKnownMachine/displayedMachineId derivation (present in both layouts, previously copy-pasted) into a shared resolveDisplayedMachine, completing the extraction this PR already did for useStickyMachineIds/useDrainPendingSession/DetailState. - Centralized the machine detail-URL builder (buildMachineHref) alongside the existing parseSelectedMachineId in development-route.ts, so the parse and build sides of the URL shape can't drift apart. MachineTreeSection now takes a driveId again instead of an ad hoc basePath string. - Deduped the near-identical guard-chain early-returns in DriveMachineList/ GlobalMachineList into a shared resolveListNotice helper. - Deduped the byte-for-byte identical fetcher/globalFetcher bodies in useDriveMachines.ts into one generic machinesFetcher<T>. - Dropped the GLOBAL_SCOPE magic-string sentinel in favor of passing undefined to useStickyMachineIds, matching the hook's own "no scope" idiom. - Documented (rather than attempted to fix, as out of scope for this PR) a known tradeoff the global layout's route-tree separation doesn't solve: navigating from the global Development view into a specific drive's Development view crosses route trees and disconnects every warm terminal across every drive, not just the one being left. This mirrors an already-accepted tradeoff elsewhere in the surface (switching drives already tears down the other drive's terminals) and is recoverable (reconnects on remount), not data loss. Also merged origin/master (bb72a70..b7859dc) to resolve a real conflict: master's #2017 changed [driveId]/development/layout.tsx's pending-session workspace lookup from a flat workspaces map to selectActiveWorkspace (the new workspace-owns-pane-grid model). Ported that into the shared use-drain-pending-session.ts hook so both layouts pick it up. All targeted tests green (232 tests across 16 files), full monorepo typecheck+build green, lint green (no new warnings).
1 participant
Sub-task 2 of the Terminal UX redesign (spec node
c6lmwa9at0372ac0icaq9ncc). Machine-page workspace/pane files, plus the realtime PTY bridge where a client-only fix would have been wrong.MachineTree.tsx, the sidebar tree, and every Development-surface file are untouched — that's sub-task 3.The bug
useMachineWorkspaceStoreheld one grid per machine, andopenTerminalonly wrote a scope into the active pane. Clicking different sidebar items never switched the middle view — it swapped the contents of a single pane inside one shared grid. No item could own a combination of terminals.The model
A workspace is a sidebar item that owns its own pane grid. A machine holds many workspaces, exactly one is active, and
MachineWorkspacerenders the active one's grid — so selecting a workspace switches the entire middle view to that item's combination of panes. That correspondence is the deliverable.machines: Record<machineId, { workspaces, order, activeWorkspaceId }>, persisted.readyevent), and a pane id only means anything inside its own grid.scopeonly says which checkout its agents run in.Selecting already switches the view, with no sidebar change. Today's sidebar items are the session rows, so
openTerminalopens that session's own workspace — or, if the session was spawned into an existing workspace by split-and-pick, the workspace it actually lives in. Click session A → the view is A's grid; click B → the whole view switches. The session list andAddAgentTerminalDialogkeep working; sub-task 3 replaces them with first-class workspace items and consumes thechildSessionIds/runningPaneCountderivations added here.Split-and-pick spawn
An empty pane renders an inline "Spawn an agent" picker — agent type plus an optional starting prompt. Picking spawns the session and binds it to that pane in one action, auto-named, no modal and no naming step. A split auto-focuses the new pane's picker. A spawn whose pane vanished mid-flight removes the session it created rather than stranding it — but only that one: the spawn API is an upsert, and the cleanup path kills the terminal, so cleaning up a
resumedsession would destroy an agent that may be mid-task in someone else's pane.Getting the starting prompt right (three attempts, two of them mine)
The prompt is typed into the PTY as input. It must never reach an agent that is already running — a line plus a carriage return delivered to an agent sitting at a
y/nconfirmation answers it.readyfires when the binary is exec'd, not when a raw-mode TUI reads stdin), with a backstop timer for agents that boot silently.MAX_INPUT_BYTES, so a pasted 5 KB spec vanished with no error. Newlines are collapsed — a newline in a tty is a submit, so a two-line prompt reached the agent as two turns, and ashellagent as two commands.agent-terminal:readynow carriesresumed, and it is a verified fact:streamSessionId != nullis not "the agent is running" (exec sessions don't survive a Sprite pause, nothing clears the column, andopenPtyShellattaches optimistically then quietly launches a fresh agent when the id turns out dangling). The bridge asks the Sprite which sessions it actually has, bounded at 5s — the check gates the shell from opening, and an unbounded stall would mean no PTY, a concurrency slot and billing hold both held, and every later connect for that terminal blocked behind the create claim.live | gone | unknown. The wire fails safe (unknown ⇒ reported as resumed ⇒ nothing is typed), but only a definitive positive is recorded on the session asresumedAtCreate— that is durable state every reattach inherits for 30 minutes, and one transient 429 must not keep answering with a guess long after the Sprite could have been asked again.readyis emitted with no await between it andopenShell, because an attach replays the session's scrollback immediately: a gap there lets output overtakeready, and a client that types on first output would type without yet knowing the agent was resumed. An order-log test enforces this — it was previously enforced only by a comment, which is exactly how it regressed once.resumedfrom a newhasOutputflag, not from an empty scrollback — one chunk over the 64 KB cap is pushed and trimmed straight back off, leaving an empty buffer for a session that has been screaming output.Safety of the persisted store
version+migrate/mergethrough a puresanitizeMachines: a blob written by an older shape would otherwise reachcolumns.flatMapand throw during render — a Machine page the user can never open again, with no in-app way to clear the storage.ensureMachinerepairs rather than skips.removeWorkspacedrops a workspace and shows a neighbour.activePaneIdnaming no pane is re-pointed — every transition no-ops on an unresolvable pane, so a split anchored on a phantom would silently do nothing.Realtime (why this PR touches it)
apps/realtime/src/terminal/: theresumedcontract above, thehasOutputflag, and a rejected connect's error is now tagged with itsconnectionId— one socket carries every pane, and the client treats an untagged event as its own, so one pane's error was painted by every pane. Harmless with one pane; this PR makes multi-pane grids normal. Additive and backward-compatible;XtermTerminalis the only consumer of these events.A permanent session leak, found while hardening the above
This one affects production today. A pane that goes away during a connect (tab closed, workspace switched, StrictMode's double-mount) sends its
agent-terminal:disconnectbefore the connect has registered anything to disconnect. The message landed on nothing, and every way out of that window left a live PTY with no viewer:Nothing else collects it: an agent CLI sits at its prompt forever, and no further disconnect can arrive for a socket that has already left. The PTY, its slot and its billing heartbeat ran for the life of the realtime process. On the free tier (one terminal) that locks the user out of their own machine.
The window is now the whole of
onConnect— payload validated to session bound — and every path that binds a session settles the abandonment (settleAbandon). Also: agoneliveness verdict now forces a fresh session (attachSessionId) rather than merely predicting one, soopenShellcan no longer attach optimistically to the very id the check just declared dead.This is a connection-lifecycle fix rather than a workspace one, and I'd happily split it out — but it was found by this PR's prompt work and the multi-pane grids this PR makes normal are what turn a rare race into an ordinary one, so it ships here.
Validation
772 realtime tests, 12,857 web tests, build 14/14, typecheck 16/16 (includes the web build), lint clean. Nine adversarial review passes ran over the diff — the last three each found a real defect (a leak fix that covered one of three paths; a verdict that predicted rather than constrained; a cleanup that killed a terminal it had not created). Every new test is mutation-verified: reverting its fix fails exactly that test.
Known follow-ups (deliberate, not gaps)
[socket, sessionId]and the socket object is stable across socket.io reconnects, soagent-terminal:connectis never re-emitted and the agent is reaped after 30 minutes while the pane still looks connected. Pre-existing on master, a connection-lifecycle fix, and it deserves its own PR and test pass.removeWorkspacehas no caller until the sub-task 3 sidebar lands. Panes remain fully recoverable in the meantime.🤖 Generated with Claude Code
https://claude.ai/code/session_01CbpaHcnCZChjMxVUngqVLo