feat(development): Development surface — top-level machine command center - #2015
Merged
Conversation
…aves 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
…ace-shell # Conflicts: # apps/web/src/components/layout/middle-content/page-views/machine/workspace/SessionLeaves.tsx
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
…regated 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
Contributor
|
Warning Review limit reached
Next review available in: 24 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 (35)
✨ 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: c4f120f9c0
ℹ️ 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".
Comment on lines
+96
to
+104
| {driveId && | ||
| machines.map((machine) => ( | ||
| <MachineTreeSection | ||
| key={machine.id} | ||
| driveId={driveId} | ||
| machineId={machine.id} | ||
| title={machine.title} | ||
| selected={machine.id === selectedMachineId} | ||
| /> |
There was a problem hiding this comment.
When a non-admin drive member can view a non-private Machine page, this renders the full MachineTreeSection in the sidebar even though MachineView deliberately shows “Machine access requires administrator privileges” and mounts no tabs for non-admin users. The tree then calls the view-level machine APIs for projects/branches/terminal sessions from the sidebar, so the new Development surface exposes machine structure and terminal metadata that the existing Machine page hides; add the same admin gate before rendering the tree or filter these machines out for non-admins.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 4a1625b — you were right, and the exposure was broader than the sidebar render.
Gated in three places:
- The API (
GET /api/machines) is now app-admin only and audits the denial (authz.access.denied). This is the one that actually closes it — the UI gate alone would leave the endpoint enumerable. - The sidebar passes a
nulldriveId touseDriveMachinesfor non-admins, so no request is made at all (rather than made and discarded). - The nav entry is hidden for non-admins instead of pointing at a destination that refuses them.
The sidebar now shows the same wording MachineView uses ("Machine access requires administrator privileges"), so the surface and the page refuse a non-admin identically. Admin remains necessary-but-not-sufficient: the list is still filtered per page through canUserViewPage, so a Machine withheld by a page-level grant never appears.
Tests: route.test.ts covers the 403 (asserting listDriveMachines is never called) and the denial audit.
Comment on lines
+35
to
+39
| useEffect(() => { | ||
| if (isLoading) return; | ||
| const driveId = resolveActiveDriveId(drives, lastVisitedDriveId); | ||
| // No drive to develop in — the drive picker is the only useful destination. | ||
| router.replace(driveId ? `/dashboard/${driveId}/development` : '/dashboard/drives'); |
There was a problem hiding this comment.
When /dashboard/development is opened with an empty drive store, the first effect starts fetchDrives(), but this effect still runs from the same render with isLoading === false and drives === [], so it immediately replaces the route with /dashboard/drives before the fetch can populate the user's drives. Fresh sessions or expired/empty caches therefore get sent to the drive picker even when they have an active drive; wait for the fetch promise or track that drive loading has completed before resolving the redirect target.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 4a1625b. Confirmed the race: isLoading is still false on the first render (the fetch has not started yet) and drives is still [], so the redirect effect resolved to no-drive and sent the user to /dashboard/drives — every cold cache, not just genuinely drive-less users.
The redirect now waits for the fetch to settle rather than gating on isLoading:
useEffect(() => {
let cancelled = false;
void fetchDrives().finally(() => { if (!cancelled) setDrivesSettled(true); });
return () => { cancelled = true; };
}, [fetchDrives]);
useEffect(() => {
if (!drivesSettled) return;
const driveId = resolveActiveDriveId(drives, lastVisitedDriveId);
router.replace(driveId ? `/dashboard/${driveId}/development` : "/dashboard/drives");
}, [drives, drivesSettled, lastVisitedDriveId, router]);.finally rather than .then so a failed fetch still resolves to the picker instead of hanging on the spinner, and fetchDrives() no-ops on a warm cache so this costs a returning user nothing. The drive picker is now only reached when the user genuinely has no drive.
…recting 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
…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
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
…e 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
Owner
Author
Self-review found two defects in my own first version — both fixed, both worth a careful lookFlagging these because they changed the architecture of the PR after the initial review, and neither was caught by a reviewer or by CI. 1. Terminals were destroyed on every machine switch ( The detail route rendered The surface now does the same: a 2. Sidebar session clicks silently did nothing ( Expand a machine, click one of its sessions: nothing happened, unless you were already viewing that machine.
An intent now carries the machine that was selected when it was made, so "my navigation hasn't committed yet" (hold) is distinguishable from "the user chose a different machine" (drop). Tests rewritten around the lane race. Also fixed while in there: Known gap, deliberately deferred: clicking a session on the machine you're already viewing lands the session but can't focus the Terminal tab ( |
…he 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
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
…tore 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
…t, 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
Owner
Author
Round 3 of self-review: what I fixed, and one design call I'm deliberately not making hereFixed (
Deliberately deferred — your call, and I'd like a second opinion Review raised a good structural point: the whole intent mechanism exists only because It's a real simplification and I want it. I'm not doing it in this PR because:
Worth noting the same one-line change would also fix a bug that exists on master today, independent of this PR: Radix unmounts inactive The |
…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
…ng 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
…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
…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
…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
…ping 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
Owner
Author
Merged latest master; one consequence of keep-alive worth knowing before mergeMaster is merged in (through #2020/#2013/#2011/#2019) — no conflicts, all 10 checks green, and I reviewed this branch specifically against the sprites/terminal work that just landed. That interaction is clean: #2013's scrollback-replay suppression is keyed to the sprite exec socket's watchdog reconnect, not to the browser's mount lifecycle, so keep-alive can neither double-replay nor lose scrollback. If anything keep-alive benefits from it — a warm, hidden terminal is exactly the regime #2013 fixes. Checking that did surface one thing I want on the record, because this PR makes a pre-existing bug more persistent:
Today that self-heals by accident: navigating away and back remounts the terminal, which reconnects it. Keep-alive is precisely the removal of that remount — so on the Development surface a terminal frozen by a network blip stays frozen until it falls out of the LRU, you switch tabs, or you reload. I'm not fixing it here on purpose: the fix belongs in Also fixed this round ( |
…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
2witstudios
added a commit
that referenced
this pull request
Jul 12, 2026
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
This was referenced Jul 12, 2026
Merged
2witstudios
added a commit
that referenced
this pull request
Jul 12, 2026
…he whole middle view (#2017) * feat(machine): split-and-pick spawn + node-as-workspace panes 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 * fix(machine): target pane writes at their own node, not the active one 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 * fix(machine): a workspace owns a grid; selecting one switches the whole 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 * fix(machine): make persisted workspaces safe, and recoverable 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 * test(machine): prove the persist config survives a real storage round-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 * fix(machine): never anchor a split on a pane that isn't there 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 * fix(machine): a prompt dies with the connect that owned it; open a session 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 * fix(machine): let the bridge say whether it RESUMED an agent, and prompt 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 * fix(machine): make `resumed` a verified fact, and stop the child-session 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 * fix(machine): tag a rejected connect's error with the pane it came from 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 * refactor(machine): drop dead exports and correct two comments the rewrites 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 * fix(machine): the prompt waits to learn what it is talking to 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 * fix(machine): carry the resumed fact onto the session, and don't swallow 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 * fix(machine): bound the liveness check, and stop a guess from becoming 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 * fix(development): point the Development surface at the active workspace 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 * fix(machine): the durable verdict must fail safe too — I had the asymmetry 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 * fix(machine): a verdict must constrain what happens, not merely predict 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 * fix(machine): the abandon window is the whole connect, not just the create 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. * fix(realtime): an abandoned connect must DECLINE to attach, not attach 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. * fix(realtime): make the connectionId collision unrepresentable, and never 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. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7 tasks
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
1 participant
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.
What
A top-level nav destination, Development, whose left sidebar is an aggregated Machine → Project → Branch → session tree for the active drive, with the existing Machine page as its detail pane. Peer to Channels/DMs/Files; a PurePoint-style command center.
Separate from the individual Machine page (opened via the drive file tree), not a replacement. Sub-task 1 of the node (shell + tree); the mobile-sheet treatment is the follow-up.
Routing — drive-in-path, plus a redirect
Deliberately not the Channels/Tasks/Calendar two-tree pattern (each ships a driveless
?driveId=twin of its drive-scoped page — the same view implemented twice):[driveId]/development/withlayout.tsx(the detail region),page.tsx(empty state),[machineId]/page.tsx.fetchDrives()to settle, so a cold cache isn't mistaken for "no drives".MemoizedSidebar's inlineifs became a pureresolveSidebarVariant(pathname), testable without rendering a sidebar.Terminals survive machine switches
The
[machineId]route rendersnull. Machines are drawn byMachineKeepAliveHostfrom the surface's layout, above the[machineId]segment.The route segment remounts on every machine switch, so a
MachineViewrendered from it would unmount each time —MachineWorkspacedisposes its workspace,XtermTerminaltears down its socket — killing the terminals you left running, on the surface built to keep them alive. The drive view already solved this:CenterPanelrenders nothing for MACHINE pages and defers to the same host (bounded LRU, CSS-hidden when inactive).The host gained one optional prop,
machineIds. The drive view infers machines from the page tree; this surface knows them — and the two disagree: a machine absent from the tree (failed tree fetch, or a private machine granted via a custom drive role, which the tree endpoint doesn't resolve) was evicted from the LRU as if trashed, disconnecting a live terminal.Within the surface, what may stay mounted and what is displayed are answered separately: the id set is add-only (a machine can drop out of
/api/machineswithout being deleted — the per-page permission check swallows DB errors), while what's shown comes from the latest fetch. So a blip costs a transient notice, never a dead session; the list polls, so it recovers on its own.Session clicks
A session leaf has to do three things in concert:
MachineView's active tab moved from uncontrolled Radix state into a small store. Otherwise unchanged: a machine with no stored tab shows Terminal.MachineWorkspacerebuilds its workspace on mount, destroying anything written ahead of it.The layout drains the intent once the machine has a workspace and is actually displayed (opening into a hidden machine would
fit()an xterm against a zero-sized box and create the PTY at a bogus geometry). The decision is a pure function (resolvePendingSession) that converges rather than firing once: it re-applies if the workspace is rebuilt underneath it (StrictMode does this on first mount) and clears once satisfied. A mismatch between where the user is and where the intent points waits — the click (a store write, sync lane) androuter.push(a transition) land in different React lanes, so there's a commit holding the new intent and the old pathname; treating that as "navigated away" silently broke every session click on a machine you weren't already viewing.Access control (admin-only, enforced at the API)
Machines are an app-admin feature end to end: only an admin can create a
MACHINEpage, andMachineViewmounts no tabs for anyone else.GET /api/machinesis app-admin only and audits the denial. Admin is necessary but not sufficient — still filtered per page throughcanUserViewPage, so a Machine withheld by a page-level grant never appears.nulldriveId for non-admins: no request is made.(Caught by Codex: without this, a non-admin who could merely view a Machine page could enumerate the drive's machines and their structure.)
Net-new
No "list Machine pages in a drive" query existed. Added
machine-list.ts(pure, DI'd), its runtime binding (hits the existingpages_drive_id_is_trashed_type_idxindex),GET /api/machines?driveId=, anduseDriveMachines..gitattributesMachineKeepAliveHost.tsxcarried a literal NUL byte (pre-existing on master), so git classified it as binary — every change to it rendered asBin N -> M byteswith no hunks, which is exactly how this PR's edit to a file shared with the drive view escaped two review passes. NULs are now escaped, and.gitattributesforces textual diffs on source. That edit is a readable 29/6-line diff.Verification
typecheck,lint,next build— green. All 10 CI checks green.resolvePendingSession(the lane race, the workspace rebuild, the clobber),resolveSidebarVariant,resolveActiveDriveId,parseSelectedMachineId, the tab store, the list-machines service,GET /api/machines(non-admin 403 + audit), and component tests forDevelopmentSidebarand the layout (the sticky set converges; a failed poll doesn't tear down a working list; a vanished machine is un-displayed but not evicted; a session isn't opened into a hidden machine).Reviewer notes
Eleven self-review passes after the initial submission; the first eight each found a real defect (terminals dying on switch, the lane race, an intent leak, a dead click, an unenforced TTL, a test that would pass on broken code, a stale-data regression, a session opening into a hidden machine). All fixed and pinned by tests. Two things I deliberately did not do, both explained in the comments above and worth your call:
MachineWorkspace's dispose-on-unmount, which would let the intent machinery go away entirely. It changes the lifecycle of a component shared with the Machine page (panes would survive leaving a machine, so returning would re-attach terminals — and PTY sessions are metered), and fix(machine): a workspace owns a pane grid — selecting one switches the whole middle view #2017 is actively reworking that store. Wants its own PR.XtermTerminal's connect effect is keyed[socket, sessionId]and socket.io reuses theSocketacross a transport reconnect, so a dropped websocket never re-emitsagent-terminal:connect. Today it self-heals because navigating away and back remounts the terminal — and keep-alive is precisely the removal of that remount. Pre-existing, but this PR makes it more persistent. Recorded on the node.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ybo53bzaiHdp6EGNf9UkYz