Node Sandboxes: node-uniform sandboxes, downward cascade, session-family orchestration tools, lazy project-Sprite promotion - #2204
Merged
Conversation
… carry Two failing tests pin the spawn double-row field bug (a stale null-scope server echo / hydrate unbinding a locally-bound pane); three green ones pin the boundaries the fix must not cross (server non-null wins, removed panes stay removed, openTerminal carries kind). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
…choes cannot unbind panes Field report (top bug blocking the PageSpace Agent flow since #2200 made it the primary spawn path): spawning ONE agent showed up as an empty "Workspace N" row PLUS the session as a separate unclaimed row, and clicking that row minted a THIRD artifact — a second workspace named after the session. One root cause, three hats. The palette's spawn did a local create+bind and then TWO unordered, un-awaited server writes: a POST of the still-EMPTY workspace snapshot, then a PATCH with the bound columns. Their socket echoes can reorder, and the merge applied the server's pane scope unconditionally, so a late echo of the pre-bind snapshot unbound the pane locally. Bind lost → empty workspace row + unclaimed session row → adopt-click mints a second workspace. Two changes, either of which alone would fix the field repro; together they also close the general race for every other flow: - workspace-reducer `mergeColumns` now keeps a locally bound pane's scope when the incoming server pane is null. This is a MONOTONE INVARIANT, not a special case: a pane's scope only ever transitions null -> bound, because no unbind flow exists (closing a session removes the pane, it never empties one), so server-null-over-local-bound is always a stale echo. Both merge paths — the `applyServerUpsert` echoes and the full-list hydrate — funnel through here. The rule is load-bearing on "no unbind flow"; the doc says so, and names rev-ordered upserts as the replacement if that ever changes. Panes the server omits are still dropped: the guard defends a bind, it never resurrects a pane closed elsewhere. Also covers `spawnIntoPane`'s cousin hazard (splitRight PATCH with a null-scope pane racing the bind PATCH). - The palette's instant spawn now reuses the existing synced `openTerminal` instead of createWorkspace+bindPaneTerminal. `openTerminal` IS the atomic born-bound create: it materializes via `newWorkspace({ firstPaneScope })` and pushes ONE already-bound snapshot, so there is no empty intermediate state to echo. Its workspace id is derived from the session (`sessionWorkspaceId`), so the adopt-click lands on the SAME row it already has — the "second workspace" failure is structurally impossible, not merely raced-away. The `!bound` failure branch dies with the awaited bind it guarded; error handling collapses to the existing catch (unwind a non-resumed session, toast). Deliberate product change: the sidebar row is therefore named after the session (`pagespace-…`) rather than "Workspace N". That matches the approved design canvas (session-named leaves) and removes the dual-naming confusion the field report described; rows stay renameable. Doc truth, no new code: `useMachineWorkspaceSync`'s claim to be "mounted exactly ONCE per machine" was false — DevelopmentSidebar mounts one instance per machine row AND MachineView mounts one for the open machine. Dual mount is designed for (module-level `declinedBootstraps` + the server claim table), and the two residual races are now documented as accepted: a stale full-replace hydrate landing after a bound `created` echo can still drop a just-created workspace locally (server row intact; a reload recovers it), and a `created` missed on a socket reconnect waits for the next hydrate. The real fix for the first is per-machine, cross-instance hydration state — follow-up, not this change. Tests: the two committed RED reducer/store tests go green; new sync pin proves a stale null-scope `created` echo leaves a just-spawned pane bound; palette tests now assert the single session-named workspace, the reported `sessionWorkspaceId`, and that EVERY workspace write a spawn issues carries only bound panes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtBdU1Y61ZnhTcAZdjLXG4
…tom agent
Field bug on pagespace.ai prod: every machine-pane PageSpace Agent (machine-root
and branch scope) was denied by its own tools — bash/git_* returned "You no
longer have access to the active machine (…)" while shell PTYs on the same
machines worked fine.
api/ai/chat/route.ts sets chatSource = { type: 'page', agentPageId: chatId } for
EVERY page chat, so a machine pane carries the MACHINE page id as its acting
agent. resolveActingAgentId took that at face value, so every canActor* check
ran getAgentAccessLevel(machinePageId, target) → fetchAgentMembership found no
driveAgentMembers row (a machine page is not an agent) → null → deny. All 43
canActor* call sites were poisoned the same way: bash/git_* denied loudly via
isMachineAccessible, and page/task/drive tools silently broken in machine panes.
The authorized user (context.userId) was never consulted — the honest
getUserAccessLevel fallback was unreachable.
resolveActingAgentId now returns undefined unless the agentPageId page is
actually an agent page (PageType.AI_CHAT), so non-agent (and missing) pages fall
through to the authenticated user — the honest actor, matching the PTY path; the
chat route has already authorized that user against the machine page. Real agent
pages are byte-identical: agent-scoped checks and userScopedAccess handling
unchanged. isMachineAccessible needed no edit — canActorViewPage now passes
honestly and the existing machineBinding exemption stays where it is (pinned by
test, not by change).
Zero additional queries: the existing single-row pages select now reads `type`
alongside `userScopedAccess` in one helper serving both gates, keeping one DB
round-trip on a path every tool call runs through.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
…tom agent
Field bug on pagespace.ai prod: every machine-pane PageSpace Agent (machine-root
and branch scope) was denied by its own tools — bash/git_* returned "You no
longer have access to the active machine (…)" while shell PTYs on the same
machines worked fine.
api/ai/chat/route.ts sets chatSource = { type: 'page', agentPageId: chatId } for
EVERY page chat, so a machine pane carries the MACHINE page id as its acting
agent. resolveActingAgentId took that at face value, so every canActor* check
ran getAgentAccessLevel(machinePageId, target) → fetchAgentMembership found no
driveAgentMembers row (a machine page is not an agent) → null → deny. All 43
canActor* call sites were poisoned the same way: bash/git_* denied loudly via
isMachineAccessible, and page/task/drive tools silently broken in machine panes.
The authorized user (context.userId) was never consulted — the honest
getUserAccessLevel fallback was unreachable.
resolveActingAgentId now returns undefined unless the agentPageId page is
actually an agent page (PageType.AI_CHAT), so non-agent (and missing) pages fall
through to the authenticated user — the honest actor, matching the PTY path; the
chat route has already authorized that user against the machine page. Real agent
pages are byte-identical: agent-scoped checks and userScopedAccess handling
unchanged. isMachineAccessible needed no edit — canActorViewPage now passes
honestly and the existing machineBinding exemption stays where it is (pinned by
test, not by change).
Zero additional queries: the existing single-row pages select now reads `type`
alongside `userScopedAccess` in one helper serving both gates, keeping one DB
round-trip on a path every tool call runs through.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
…ounded by the user Answers the Codex P1 review note on #2203: consulted agents inherit the PARENT's actor identity (agent-communication-tools spreads the caller's context; activeMachineAgentPageId documents the same sub-agent rule), so a machine pane's ask_agent chain resolves to the invoking user and is capped by that user's own ACL — never wider than the pane's own tools, and strictly wider than the dead path it replaces (ask_agent's canActorViewPage gate denied at the door before this fix). Documented on resolveActingAgentId and pinned by a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
Dropping the hand-written { type: string } annotation on fetchActingPageRow
keeps drizzle's pgEnum union, so the AI_CHAT comparison is checked against the
real page-type union instead of a widened string.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
…ission chain Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TebaKGKENRMStPXDU6GxJP
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds typed workspace scopes, machine-node targeting, machine-bound session tools and I/O, promoted project Sprites, expanded Sprite lifecycle handling, and subject-based storage measurement and billing. ChangesMachine workspace and synchronization
Machine-bound AI tooling
Project promotion and storage accounting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ChatRoute
participant MachinePaneBinding
participant SessionTools
participant RealtimeSessionIO
ChatRoute->>MachinePaneBinding: derive bound handle set
MachinePaneBinding-->>ChatRoute: reachable machine nodes
ChatRoute->>SessionTools: register session-family tools
SessionTools->>RealtimeSessionIO: read or send PTY session
RealtimeSessionIO-->>SessionTools: liveness, output, or delivery result
sequenceDiagram
participant SpawnAgentTerminal
participant PromoteProject
participant MachineProjectStore
participant ProjectSprite
SpawnAgentTerminal->>PromoteProject: promote project-scoped session
PromoteProject->>MachineProjectStore: CAS promotion update
PromoteProject->>ProjectSprite: provision or attach and clone
ProjectSprite-->>SpawnAgentTerminal: promoted terminal location
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2a75a3ae06
ℹ️ 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".
| // server push. No starting prompt — instant spawn means the prompt is | ||
| // typed in the pane itself, so there is nothing to auto-send. | ||
| openTerminal(paneScope); | ||
| onSpawned(sessionWorkspaceId(paneScope)); |
There was a problem hiding this comment.
When addAgentTerminal resumes a session already displayed in another workspace, openTerminal intentionally selects that existing workspace via workspaceShowing, but this callback always reports the session-derived workspace ID. If that own workspace also exists, TerminalTab immediately switches away to a workspace that does not contain the resumed session; from the sidebar, the nonexistent/wrong ID becomes an unconverged pending selection. Return the actual workspace selected after openTerminal rather than assuming sessionWorkspaceId(paneScope).
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in the latest push — confirmed real and pinned red-first: "a RESUMED session already shown in another workspace reports THAT workspace, not the session-derived one" (NodeActionPalette.test.tsx). The synced openTerminal wrapper already resolved the actual destination (workspaceShowing(...) ?? machine.workspaces[sessionWorkspaceId(...)]) for its own server push; it now returns that id and the palette reports it via onSpawned, with sessionWorkspaceId kept only as the machine-missing fallback.
Codex P2 on #2204: a resumed session another workspace is already showing lands THERE (workspaceShowing), not in its own session-derived workspace — reporting sessionWorkspaceId navigated to a workspace without the session (or a nonexistent one, leaving a pending sidebar selection unconverged). The synced openTerminal wrapper already resolves the destination for its own push; it now returns that id and the palette reports it, falling back to the derived id only on the machine-missing edge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/lib/ai/tools/actor-permissions.ts (1)
49-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typing
typemore precisely thanstring.
fetchActingPageRow's return signature widens thepages.typeenum column tostring, losing the compile-time guarantee that comparisons likerow.type === PageType.AI_CHATare checked against a known set of values.♻️ Optional tightening
-): Promise<{ type: string; userScopedAccess: boolean } | undefined> { +): Promise<{ type: PageType; userScopedAccess: boolean } | undefined> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/ai/tools/actor-permissions.ts` around lines 49 - 57, Update the return type of fetchActingPageRow so type uses the precise pages.type enum/inferred column type instead of string, preserving the database schema’s allowed values and compile-time checking for PageType comparisons. Keep the existing query and undefined behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/lib/ai/tools/actor-permissions.ts`:
- Around line 39-57: Update hasAgentUserScopedAccess to require the fetched
row’s type to be PageType.AI_CHAT before returning userScopedAccess. Reuse the
existing fetchActingPageRow result and preserve false for missing rows or
non-AI_CHAT pages, keeping it aligned with resolveActingAgentId.
---
Nitpick comments:
In `@apps/web/src/lib/ai/tools/actor-permissions.ts`:
- Around line 49-57: Update the return type of fetchActingPageRow so type uses
the precise pages.type enum/inferred column type instead of string, preserving
the database schema’s allowed values and compile-time checking for PageType
comparisons. Keep the existing query and undefined behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 848e21d6-4bee-4802-9f11-4653d3d00d99
📒 Files selected for processing (11)
apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/NodeActionPalette.test.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/NodeActionPalette.tsxapps/web/src/hooks/__tests__/useMachineWorkspaceSync.test.tsapps/web/src/hooks/useMachineWorkspaceSync.tsapps/web/src/lib/ai/tools/__tests__/actor-permissions.test.tsapps/web/src/lib/ai/tools/__tests__/page-write-tools.test.tsapps/web/src/lib/ai/tools/__tests__/sandbox-tools-runtime.test.tsapps/web/src/lib/ai/tools/actor-permissions.tsapps/web/src/stores/machine-workspace/__tests__/useMachineWorkspaceStore.test.tsapps/web/src/stores/machine-workspace/workspace-reducer.ts
…solveActingAgentId CodeRabbit minor on #2204: the two seams answer the same question and MachineDirectoryRuntimeDeps.isUserScopedAgent is documented as mirroring them — a non-agent page row that somehow carried userScopedAccess: true must not diverge the answers. Pinned by a lockstep test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
A pane's checkout is about to become its WORKSPACE's, full stop — the pane
itself stores only { name, kind }. These tests still assert the shape that
change removes, so they are inverted first:
- pane-surface: the foreign-checkout escape hatch ("unresolvable, therefore a
terminal") becomes "the workspace's list is always this pane's list", and
resolvePaneSurface no longer takes a workspaceScope at all.
- TerminalPanes: close-to-kill re-derives (project, branch, name) from the
workspace instead of reading it off the pane; the same-name-elsewhere case
moves from a sibling PANE to a sibling WORKSPACE, which is where it can
actually live now.
- WorkspaceLeaves: the "wanderer" pane bound at a foreign checkout is replaced
by the two facts that succeed it — the kill carries the workspace's checkout,
and a bind naming another node is REJECTED.
- store: a stored wide pane ({projectName, branchName, name}) must project to
{ name, kind } on both merge paths (hydrate and localStorage rehydrate), and
the bind-time node-equality assertion must reject a cross-node bind.
24 failing, 270 passing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNo8eSMt9p2Hs3417BKGxP
A bound conversation is no longer pinned to ONE checkout: deriveMachinePaneBinding now returns a handle set — the bound node plus its downward closure. Machine root → [self + all projects + all branches]; project → [self + its own branches]; branch → [self]. Each handle carries its resolution (branch Sprite, or the machine's Sprite + cwd) and the owning machine page id, which stays the billing/budget key at every depth. Sibling isolation is a property of the derivation, not a rule downstream: a sibling node is never derived, so there is nothing to deny. A branch whose Sprite is confirmed torn down is likewise omitted rather than derived-then-denied — the same fail-closed rule the natively-bound branch path already applied. resolveMachineNodeTarget is the pure lookup later leaves use to resolve a tool call's `target` against the set (bare branch names resolve within self's project, or unambiguously from the machine root). ToolExecutionContext.machineBinding IS the handle set now (the contract phases 4-7 consume); the route, the tool factory, and the machine directory read `binding.self` where they previously read the flat cwd/branchSandbox. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRQ2bAcBc3dExEQWG42gRn
…identity isMachineAccessible's binding exemption and the bound listMachines short-circuit now both read the DERIVED SET rather than `binding.self`. Membership IS the policy: a node the set never contained is a node this check denies, and target resolution (next leaf) addresses nodes out of the same set — so the cascade adds no second place that can decide node access. Existence / trash / type / canActorViewPage are still never bypassed, and the billing pin stays explicit: a branch-bound run keys its payer and its runtime guardrail on the OWNING MACHINE page id, because every handle carries it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRQ2bAcBc3dExEQWG42gRn
A pane's checkout is now its workspace's, full stop. `TerminalPaneState.scope`
keeps only `{ name, kind }`; every (project, branch, name) triple — the kill
address, `paneSessionId`, `sessionWorkspaceId` — is rebuilt at the read site
with the new `paneTerminalScope(workspace.scope, pane.scope)`.
The duplicated checkout made a "foreign" pane representable. Nothing ever wrote
one, but seven defensive branches existed to survive one, and the two copies
could in principle disagree with no rule for which wins. Those branches are
DELETED, not adapted:
- pane-surface's `resolvable` gate (and its whole `workspaceScope` parameter):
the workspace's session list is unconditionally this pane's list. The old
escape hatch resolved a kind-less chat pane to an Xterm.
- TerminalPane's per-pane checkout chip, which existed to make a foreign pane
visible; one checkout per workspace, so it reads the workspace's label.
- close-to-kill and its `boundElsewhere` comparison, WorkspaceLeaves'
`shownElsewhere` / `pendingKillScopes` / `localSessionIds`, `childSessionIds`
and `paneShowing` all re-derive through the OWNING workspace's scope instead
of trusting a stored copy. `paneShowing` checks the node ONCE, at the
workspace, rather than per pane.
`assignPane` gains the bind-time node-equality assertion: a scope naming
another node throws, rather than silently filing the session under a checkout
it does not run in. It throws instead of returning the "pane is gone" false,
which callers answer by killing the session they just created.
Migration is read-time projection only (`projectStoredPaneScope`), applied on
both merge paths — `mergeColumns` for server payloads and `sanitizeMachines`
for localStorage. No backfill and no version bump: a wide pane was
representable but never written disagreeing with its workspace. The monotone
null->bound merge guard is unchanged in shape on the narrow type.
apps/web machine suites: 443 passing. tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNo8eSMt9p2Hs3417BKGxP
…ine root
All four file tools and — through the ONE generator change — all 56 git/gh tools
now take an optional target { project?, branch? }, resolved inside open() against
the derived handle set. cwd and Sprite routing for a resolved target match what a
conversation natively bound at that node would get; an explicit cwd still wins; a
target the set doesn't contain is denied there and nowhere else.
Fixes a pre-existing bug the cascade would have multiplied: git's open() never
threaded the binding onto the actor ctx, and git-tool-runners never forwarded
ctx.branchSandbox to acquireSandbox — so in a branch-bound pane, bash ran in the
branch Sprite while `git status` silently reported the machine checkout. Both
halves are threaded now, with the branch-Sprite acquire pinned red-first.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQ2bAcBc3dExEQWG42gRn
A bound conversation lives at a node, not at a checkout. The prompt now names which node it is (machine root / project / branch, with its cwd), how to address nodes beneath it with `target`, that anything outside the scope is refused, and that list_sessions is the discovery tool for the scope — the frozen contract string; the tool itself ships with the session family. Enumerating the set inline would go stale mid-run, so discovery stays delegated. switch_machine and list_machines remain dropped and the prompt still says so. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRQ2bAcBc3dExEQWG42gRn
…page
Branch-terminal Sprites have their own persistent filesystems but billed
storage nowhere: `acquireBranchSandbox` deliberately omitted `pageId` (its
measurement would have clobbered the owning machine's own measured bytes),
and the storage reconcile only ever enumerated `machine_sessions`.
Splits the two questions the old single `pageId` conflated, in one new
contract module (machine-storage-attribution.ts):
- measurement SUBJECT — which row the bytes persist on (a machine's
`machine_sessions` row vs a branch's own `machine_branches` row);
- attribution KEY — always the owning Machine page, already the payer key
and runtime-guardrail key for every branch-scoped run, and the one field
the per-machine usage breakdown groups on.
So a branch Sprite's storage now appears under the Terminal the user sees,
alongside that machine's own storage and runtime, with each Sprite metered
on its own watermark. The contract is frozen for phase 7: a promoted project
Sprite adds a third subject kind and inherits `storageAttributionPageId`,
the payer, and the grouping unchanged.
The never-wake rule is preserved end to end — the reconcile's deps seam
still exposes no sprite handle, and branch measurement rides existing wake
paths only (spawn-after-clone and reattach, where the handle is already
live), throttled per subject and fully best-effort. Torn-down branches are
excluded from both metering and measurement: their disk is gone.
- packages/db: machine_branches gains storageLastBilledAt /
storageMeasuredBytes / storageMeasuredAt (generated migration 0216).
- reconcile: second row source + branch watermark writer, one metering loop.
- measure/persist: subject-keyed, so neither kind can drift onto its own
throttle, dedup or persist rule.
Red-first tests: branch charges land on the owning page and resolve that
page's payer; only the branch row's watermark advances; a hibernating,
never-measured branch bills the 0 floor with no wake; stale-but-billed
signal; per-subject throttle keys cannot collide; branch bytes never write
to the machine row; spawn/attach fire the seam and never fail on it; and the
usage breakdown rolls branch storage into its owning machine's line.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NE4DKtEx7p3hJ9C9M9crek
…DTOs
`MachineNodeScope` was `{projectName?, branchName?}` — a bag that made two
nonsense shapes expressible (a branch with no project; "machine scope" and
"the caller forgot to pass one" being the same value) and left every consumer
re-deriving which of the three node kinds it held via its own `if
(!projectName)` ladder. It is now
{ level: 'machine' }
| { level: 'project'; projectName }
| { level: 'branch'; projectName; branchName }
so `nodeScopeOf` and `scopeLabelOf` are TOTAL switches: a fourth node kind
fails compilation at both instead of falling into somebody's else-branch.
`isSameNodeScope` and the error-message `scopeKey` likewise.
The discriminant is a client-side modelling device, never a protocol change.
`nodeScopeNames` produces the `{projectName?, branchName?}` half that every
request still sends, and `machineNodeScope` — the client mirror of the
server's `deriveWorkspaceScope` — DERIVES the discriminant back from those
names on read. Because it is always derived, a stored `level` cannot disagree
with the names beside it, and every legacy payload (localStorage, an older
client's server row) reads correctly with no migration step:
`projectStoredNodeScope` applies the same projection on both merge paths that
`projectStoredPaneScope` already covers for panes.
Wire contract:
- `deriveWorkspaceScope` stays the one adapter. `toWorkspaceScopeDTO` calls it
rather than reading the stored `scope` column, so the two can never disagree
on the wire and a row predating the column still serialises.
- `WorkspaceLayoutScopeDTO` (packages/db) narrows to `{ name, kind? }`,
matching what a pane now stores. Its doc records why there is no backfill.
- The sync hook sends `nodeScopeNames(workspace.scope)`, not the union — the
server derives its own discriminant, so sending ours would be a redundant
copy of a derived fact.
apps/web machine-workspace + machine-surface + sync + machines-API suites: 758
passing. tsc clean across web/db/lib; apps/web lint clean. The 16 failures in
admin-role-version / activity-tools / grouping are identical on the base commit
(verified by stash) and untouched by this work.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNo8eSMt9p2Hs3417BKGxP
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/stores/machine-workspace/workspace-reducer.ts (1)
175-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrphaned doc block above
projectStoredNodeScope.The comment at Lines 175-184 documents
projectStoredPaneScope("Read-time projection of a STORED pane scope") but is stacked immediately aboveprojectStoredNodeScope(Line 191), with the node-scope doc (Lines 185-190) right below it.projectStoredPaneScope(Line 204) then has no doc. A maintainer editing the node function reads a comment about the pane function. Move the pane-scope block down to sit aboveprojectStoredPaneScope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/stores/machine-workspace/workspace-reducer.ts` around lines 175 - 190, Move the existing “Read-time projection of a STORED pane scope” documentation block from above projectStoredNodeScope to immediately above projectStoredPaneScope. Keep the node-scope documentation directly above projectStoredNodeScope and leave both comment contents unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/lib/src/services/sandbox/machine-storage-billing.ts`:
- Around line 62-76: Prevent the machineSessions join in listBranchSprites from
producing duplicate branch rows when multiple sessions share a pageId.
Deduplicate the query results before returning them, or otherwise enforce a
unique pageId relationship, so reconcileMachineStorage builds billable with each
branch filesystem only once while preserving the existing lastActiveAt fallback.
---
Nitpick comments:
In `@apps/web/src/stores/machine-workspace/workspace-reducer.ts`:
- Around line 175-190: Move the existing “Read-time projection of a STORED pane
scope” documentation block from above projectStoredNodeScope to immediately
above projectStoredPaneScope. Keep the node-scope documentation directly above
projectStoredNodeScope and leave both comment contents unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3adb3a4e-db57-4968-94f3-caa9989d296a
📒 Files selected for processing (50)
apps/web/src/app/api/ai/chat/__tests__/machine-binding-route.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/components/layout/middle-content/page-views/machine/tabs/TerminalTab.test.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/NodeActionPalette.test.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/NodeActionPalette.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/TerminalPanes.test.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/TerminalPanes.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/WorkspaceLeaves.test.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/WorkspaceLeaves.tsxapps/web/src/components/layout/middle-content/page-views/machine/workspace/pane-surface.test.tsapps/web/src/components/layout/middle-content/page-views/machine/workspace/pane-surface.tsapps/web/src/hooks/__tests__/useMachineWorkspaceSync.test.tsapps/web/src/hooks/useMachineWorkspaceSync.tsapps/web/src/lib/ai/core/types.tsapps/web/src/lib/ai/machine-pane/machine-pane-binding-runtime.tsapps/web/src/lib/ai/tools/__tests__/actor-permissions.test.tsapps/web/src/lib/ai/tools/__tests__/sandbox-git-tools.test.tsapps/web/src/lib/ai/tools/__tests__/sandbox-tools-runtime.test.tsapps/web/src/lib/ai/tools/__tests__/sandbox-tools.test.tsapps/web/src/lib/ai/tools/actor-permissions.tsapps/web/src/lib/ai/tools/sandbox-git-tools.tsapps/web/src/lib/ai/tools/sandbox-git/core/__tests__/generate-tools.test.tsapps/web/src/lib/ai/tools/sandbox-git/generate-tools.tsapps/web/src/lib/ai/tools/sandbox-tools-runtime.tsapps/web/src/lib/ai/tools/sandbox-tools.tsapps/web/src/lib/machines/machine-branches-runtime.tsapps/web/src/lib/machines/machine-workspaces-runtime.tsapps/web/src/lib/subscription/__tests__/usage-breakdown.test.tsapps/web/src/stores/machine-workspace/__tests__/useMachineWorkspaceStore.test.tsapps/web/src/stores/machine-workspace/__tests__/workspace-reducer.test.tsapps/web/src/stores/machine-workspace/useMachineWorkspaceStore.tsapps/web/src/stores/machine-workspace/workspace-reducer.tspackages/db/drizzle/0216_easy_raider.sqlpackages/db/drizzle/meta/0216_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/machine-branches.tspackages/db/src/schema/machine-workspaces.tspackages/lib/src/services/machines/__tests__/machine-branches.test.tspackages/lib/src/services/machines/__tests__/machine-pane-binding.test.tspackages/lib/src/services/machines/machine-branches.tspackages/lib/src/services/machines/machine-pane-binding.tspackages/lib/src/services/sandbox/__tests__/machine-storage-attribution.test.tspackages/lib/src/services/sandbox/__tests__/machine-storage-billing.test.tspackages/lib/src/services/sandbox/__tests__/machine-storage-measure.test.tspackages/lib/src/services/sandbox/__tests__/machine-storage-reconcile.test.tspackages/lib/src/services/sandbox/git-tool-runners.tspackages/lib/src/services/sandbox/machine-storage-attribution.tspackages/lib/src/services/sandbox/machine-storage-billing.tspackages/lib/src/services/sandbox/machine-storage-measure.tspackages/lib/src/services/sandbox/machine-storage-reconcile.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/hooks/tests/useMachineWorkspaceSync.test.ts
- apps/web/src/components/layout/middle-content/page-views/machine/workspace/NodeActionPalette.tsx
- apps/web/src/lib/ai/tools/actor-permissions.ts
- apps/web/src/lib/ai/tools/tests/actor-permissions.test.ts
- apps/web/src/components/layout/middle-content/page-views/machine/workspace/NodeActionPalette.test.tsx
The session family's first two verbs. `list_sessions` reports the whole derived handle set — every node, including empty ones — with each node's views (the ids `placement.splitInto` addresses) and its sessions, whose state comes from ONE function (`readSessionState`): `'streaming'` and real PTY liveness are upgrades behind that same function, not new call sites. `add_session` reserves the agent-terminal row and materializes its manifestation. The layout writer composes the phase-1 CLIENT reducer against a server-loaded view list rather than re-deriving what a workspace looks like — the server is now a second writer of the same blob, and running the same code is the only way the two stay byte-identical (pinned red-first against the real client store: `openTerminal` for the born-bound view, `splitDown` + `bindPaneTerminal` for the split). A shell session is RESERVED until a viewer first connects — the PTY starts there, not here — and both the state and the tool description say so. `prompt` is refused rather than silently dropped until the dispatch engine that can actually deliver it lands. Authorization is set membership and nothing else: every target resolves through `resolveMachineNodeTarget` against the conversation's handle set, the same fact `isMachineAccessible` enforces. No second policy site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHNroxSx4y7LS2mm75URsR
`move_session` re-homes a session's MANIFESTATION by composing the two halves that already exist: close every pane showing it, then run the same placement writer `add_session` uses. No second layout writer, so a move cannot drift from a spawn. A cross-NODE move is refused, not accommodated: a view only ever holds sessions from its own node, and re-homing the pane can never re-home the sandbox the session runs in. Pinned red-first, alongside a stale-echo case that drives the real client store — the move's writes leave the session bound in its new view even when a pre-bind snapshot of that view lands afterwards (the monotone merge guard holds because the destination pane is a fresh id, bound in the same write). `kill_session` kills within the handle set and then closes the session's manifestations, in that order: closing the panes of a still-running session would hide a live, billing process with nothing pointing at it. A node outside the set is denied at the one policy site — set membership — before anything is read or killed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHNroxSx4y7LS2mm75URsR
2witstudios
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| const payload = [NAMESPACE_VERSION, tenantId, machineId, projectName].join('\0'); | ||
| // codeql[js/insufficient-password-hash] not a password hash — a keyed HMAC over SANDBOX_SESSION_SECRET (a >=32-char server secret, never user input) deriving a deterministic Sprite-name pseudonym, same as branch-session.ts's deriveBranchSessionKey | ||
| const digest = createHmac('sha3-256', secret).update(payload).digest('hex'); |
Owner
Author
There was a problem hiding this comment.
False positive, suppressed in-code with justification (project-session.ts line 41): this is not a password hash — it's a keyed HMAC over SANDBOX_SESSION_SECRET (a ≥32-char server secret, never user input) deriving a deterministic Sprite-name pseudonym, byte-identical in pattern to the pre-existing branch-session.ts deriveBranchSessionKey. Leaving the thread open for maintainer dismissal alongside the 3 pre-existing chat-route alerts.
…-coverage gate The route's execution IS audited — promoteProject writes through the same writeCodeExecutionAudit pipeline as machines/branches (provision/clone/ credential propagation onto the project Sprite); the gate entry documents where, matching the sibling machine-route entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
…ved row on refused placement Two CodeRabbit findings on the session family, both red-first: - The session family is the machine BINDING's orchestration surface, not a composer toggle: page.enabledTools saved before the family existed must not silently strip it. Step-3 allowlist filtering extracted into pure filterToolsForAgentAllowlist with the session-family exemption — unbound conversations never carry these names, so nothing can leak. - add_session now PLANS placement (pure) before reserving the session row, so a cross_node/view_not_found refusal leaves nothing behind. Reserve still precedes materialization: a pane must never point at an unreserved session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/lib/src/services/machines/machine-project-promotion.ts (1)
90-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate
MachineAcquireResultfrom promotion deps.
MachineAcquireResultis already declared inpackages/lib/src/services/machines/machine-projects.tsand exported via the machine dependencies.acquireMachineSandbox: (machineId: string) => Promise<MachineAcquireResult>inPromoteProjectDepscan either use that shared type or be normalized alongsideMachineProjectsDeps, so this local duplicate doesn’t add value and creates type-drift risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/src/services/machines/machine-project-promotion.ts` around lines 90 - 92, Remove the local MachineAcquireResult declaration near PromoteProjectDeps and reuse the shared type exported from machine-projects through the machine dependencies. Update the acquireMachineSandbox signature in PromoteProjectDeps to reference that shared type, keeping its existing contract unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/lib/ai/machines/headless-session-run-runtime.ts`:
- Around line 443-459: The send_session usage path currently records
DEFAULT_PROVIDER and DEFAULT_MODEL instead of the model selected by generate().
Update generate() to return its resolved provider and model, using
machinePage.aiProvider/aiModel with the existing defaults, then pass those
values through the caller into trackUsage and use them in
AIMonitoring.trackUsage.
In `@apps/web/src/lib/ai/tools/session-io-agent-runtime.ts`:
- Around line 61-95: Update the chatMessages query in the session transcript
flow to add an SQL role predicate accepting only user or assistant messages
within the existing and(...) WHERE conditions. Then remove the redundant
JavaScript role filter before mapping, while preserving ordering, limit
behavior, and streaming pending markers.
---
Nitpick comments:
In `@packages/lib/src/services/machines/machine-project-promotion.ts`:
- Around line 90-92: Remove the local MachineAcquireResult declaration near
PromoteProjectDeps and reuse the shared type exported from machine-projects
through the machine dependencies. Update the acquireMachineSandbox signature in
PromoteProjectDeps to reference that shared type, keeping its existing contract
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ac8d764-8865-41f0-89cb-aa075410508f
📒 Files selected for processing (54)
apps/realtime/src/__tests__/index.test.tsapps/realtime/src/index.tsapps/realtime/src/terminal/__tests__/agent-terminal-access.test.tsapps/realtime/src/terminal/__tests__/session-io.test.tsapps/realtime/src/terminal/agent-terminal-access.tsapps/realtime/src/terminal/agent-terminal-handler.tsapps/realtime/src/terminal/session-io.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/machines/agent-terminals/route.tsapps/web/src/app/api/machines/projects/promote/__tests__/route.test.tsapps/web/src/app/api/machines/projects/promote/route.tsapps/web/src/lib/ai/machines/__tests__/headless-session-run.test.tsapps/web/src/lib/ai/machines/headless-session-run-runtime.tsapps/web/src/lib/ai/machines/headless-session-run.tsapps/web/src/lib/ai/machines/machine-binding-prompt.tsapps/web/src/lib/ai/tools/__tests__/session-io-agent.test.tsapps/web/src/lib/ai/tools/__tests__/session-io-pty.test.tsapps/web/src/lib/ai/tools/__tests__/session-tools.test.tsapps/web/src/lib/ai/tools/sandbox-git-tools.tsapps/web/src/lib/ai/tools/sandbox-tools-runtime.tsapps/web/src/lib/ai/tools/sandbox-tools.tsapps/web/src/lib/ai/tools/session-io-agent-runtime.tsapps/web/src/lib/ai/tools/session-io-agent.tsapps/web/src/lib/ai/tools/session-io-pty.tsapps/web/src/lib/ai/tools/session-tools-runtime.tsapps/web/src/lib/ai/tools/session-tools.tsapps/web/src/lib/machines/agent-terminals-runtime.tsapps/web/src/lib/machines/machine-projects-runtime.tspackages/db/drizzle/0217_famous_blue_blade.sqlpackages/db/drizzle/0218_smooth_gorilla_man.sqlpackages/db/drizzle/meta/0217_snapshot.jsonpackages/db/drizzle/meta/0218_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/machine-projects.tspackages/lib/package.jsonpackages/lib/src/services/machines/__tests__/agent-terminals.test.tspackages/lib/src/services/machines/__tests__/machine-pane-binding.test.tspackages/lib/src/services/machines/__tests__/machine-project-promotion.test.tspackages/lib/src/services/machines/__tests__/machine-projects.test.tspackages/lib/src/services/machines/agent-terminals.tspackages/lib/src/services/machines/machine-pane-binding.tspackages/lib/src/services/machines/machine-project-promotion.tspackages/lib/src/services/machines/machine-projects-store.tspackages/lib/src/services/machines/project-session.tspackages/lib/src/services/sandbox/__tests__/machine-storage-attribution.test.tspackages/lib/src/services/sandbox/__tests__/machine-storage-billing.test.tspackages/lib/src/services/sandbox/__tests__/machine-storage-reconcile.test.tspackages/lib/src/services/sandbox/__tests__/project-session.test.tspackages/lib/src/services/sandbox/git-tool-runners.tspackages/lib/src/services/sandbox/machine-storage-attribution.tspackages/lib/src/services/sandbox/machine-storage-billing.tspackages/lib/src/services/sandbox/machine-storage-reconcile.tspackages/lib/src/services/sandbox/project-session.tspackages/lib/src/services/sandbox/tool-runners.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/lib/src/services/sandbox/git-tool-runners.ts
- apps/web/src/lib/ai/tools/sandbox-git-tools.ts
- apps/web/src/lib/ai/tools/session-tools-runtime.ts
- packages/lib/src/services/machines/tests/machine-pane-binding.test.ts
- apps/web/src/lib/ai/tools/sandbox-tools.ts
- packages/lib/src/services/machines/machine-pane-binding.ts
- apps/web/src/lib/ai/tools/session-tools.ts
…ock factory The wholesale vi.mock factories replace the module, so the chat route's new import was undefined under them — lifecycle/stream tests failed with the spy never invoked. Passthrough stub, same style as the withSessionFamilyTools stub. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
… path Review blocker: removeProject/deleteMachine/purge all destroyed the machine_projects row while its promoted Sprite kept running — and once the row was gone, listProjectSprites stopped billing it, so the leak was invisible. Worse than the branch case 0209 was written for. Wired the same three layers branches have: - Migration 0219: AFTER DELETE trigger on machine_projects rescues the pointer into machine_sprite_reclaims (skips unpromoted rows — sandboxId NULL — and stamped rows, whose reused name could belong to a replacement VM). - teardownOneMachine: promoted-project rows get the same intent-stamp → identity-guarded kill → CAS spriteTornDownAt sequence as branches. - Orphan reconciler: third row source (machine_projects, teardown-intent tier) + markProjectTornDown CAS; removeProject best-effort-kills the Sprite inline before deleting the row (the trigger is the backstop, not the primary). Real-DB integration suite extended: trigger installed; page/drive/user-erasure cascades rescue the promoted project's pointer; unpromoted rows never enqueue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
…it gate + tool allowlist Review majors — send_session reached the agent loop with two controls missing that chat/route.ts enforces: - canConsumeAI ran nowhere on the dispatched path (only after-the-fact metering), so a user at their limit could drive depth-2 chains for free. The engine now takes checkCredit/releaseHold deps: gate BEFORE the claim (a denied dispatch leaves nothing behind — same rule as the depth cap), hold released on every exit (busy, append-failure, run settle, generate throw). New refusal reason credit_denied, surfaced verbatim to the dispatching agent. - page.enabledTools evaporated when a session was reached by dispatch instead of a browser: the runtime's generate() now applies the same filterToolsForAgentAllowlist as the chat route (session family exempt for the same binding-surface reason). Red-first in headless-session-run.test.ts: denial before claim/append; hold released on success AND on a throwing generate; depth-cap refusal never even checks credit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
…nt-page actor fall-through Two review 'confirm this' items, answered with pins rather than prose: - A promoted-then-torn-down project keeps its node in the handle set on purpose — set membership authorizes the next project-scoped spawn, which RE-promotes (fresh Sprite, fresh clone, teardown marks cleared by the promote CAS). New spawn-level test pins that; the handle-derivation comment now states plainly that the machine-checkout fallback cwd may be gone in the window and why the node must not be dropped. - The actor-permissions AI_CHAT type gate: ANY non-agent page type (pinned with DOCUMENT, not just MACHINE) falls through to the INVOKING USER's own authority — never a phantom agent, and nothing beyond what that user already has in a Global Assistant conversation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/lib/src/services/machines/__tests__/machine-projects.test.ts`:
- Line 496: Remove the duplicate killCalls const declaration in the
machine-projects test scope, leaving a single declaration of the Array<{
sandboxId: string; spriteInstanceId: string | null }> value so the test compiles
without changing its usage.
In `@tasks/reviews/pu-machine-pane-fixes.md`:
- Around line 25-38: Update the review record to remove the stale
promoted-project blocker or mark it resolved, reflecting that
machine-settings-runtime.ts now tears down promoted project Sprites and stamps
spriteTornDownAt, with corresponding tests covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e5ccefb-c3a2-47a8-914c-c4013fbe6307
📒 Files selected for processing (34)
apps/web/src/app/api/__tests__/security-audit-coverage.test.tsapps/web/src/app/api/ai/chat/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/chat/__tests__/mcp-scope.test.tsapps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/conversation-id-resolution.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/v1/chat/completions/__tests__/route-backfill.test.tsapps/web/src/app/api/v1/chat/completions/__tests__/route.test.tsapps/web/src/lib/ai/core/__tests__/context-assembly.test.tsapps/web/src/lib/ai/core/__tests__/tool-filtering.test.tsapps/web/src/lib/ai/core/tool-filtering.tsapps/web/src/lib/ai/machines/__tests__/headless-session-run.test.tsapps/web/src/lib/ai/machines/headless-session-run-runtime.tsapps/web/src/lib/ai/machines/headless-session-run.tsapps/web/src/lib/ai/tools/__tests__/actor-permissions.test.tsapps/web/src/lib/ai/tools/__tests__/session-tools.test.tsapps/web/src/lib/ai/tools/session-io-agent.tsapps/web/src/lib/ai/tools/session-tools.tsapps/web/src/lib/machines/__tests__/machine-orphan-reconcile-runtime.integration.test.tsapps/web/src/lib/machines/__tests__/machine-settings-runtime.test.tsapps/web/src/lib/machines/machine-orphan-reconcile-runtime.tsapps/web/src/lib/machines/machine-projects-runtime.tsapps/web/src/lib/machines/machine-settings-runtime.tspackages/db/drizzle/0219_project_sprite_reclaim_trigger.sqlpackages/db/drizzle/meta/_journal.jsonpackages/lib/src/services/machines/__tests__/agent-terminals.test.tspackages/lib/src/services/machines/__tests__/machine-orphan-reconcile.test.tspackages/lib/src/services/machines/__tests__/machine-projects.test.tspackages/lib/src/services/machines/machine-orphan-reconcile.tspackages/lib/src/services/machines/machine-pane-binding.tspackages/lib/src/services/machines/machine-projects.tstasks/reviews/pu-machine-pane-fixes.md
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/web/src/app/api/ai/chat/tests/stream-socket-events.test.ts
- packages/db/drizzle/meta/_journal.json
- apps/web/src/lib/ai/machines/headless-session-run-runtime.ts
- packages/lib/src/services/machines/tests/agent-terminals.test.ts
- apps/web/src/app/api/ai/chat/route.ts
- apps/web/src/lib/ai/tools/tests/session-tools.test.ts
- apps/web/src/lib/ai/machines/headless-session-run.ts
- apps/web/src/lib/ai/tools/session-io-agent.ts
- apps/web/src/lib/ai/tools/tests/actor-permissions.test.ts
- packages/lib/src/services/machines/machine-pane-binding.ts
- apps/web/src/lib/ai/tools/session-tools.ts
…ne-pane-fixes.md - realtime: one shared readCappedBody (1 MiB, destroy past the cap) fronts all five signed endpoints — an unauthenticated caller can no longer hold memory until the HMAC check finally rejects. Red-first; coverage gate stays >=98%. - machine-root binding derivation: two reads total (projects + listAll branches, grouped in memory) instead of 1 + N per-project branch reads on the hot path of every bound turn. Pinned by a no-per-project-reads test. - BRANCH_REPO_PATH/PROJECT_REPO_PATH now defined beside SANDBOX_ROOT in sandbox-paths.ts (re-exported from the service modules), so handle derivation stops dragging the promotion/branch service graphs in for a string constant. - withNodeTarget throws at factory-construction for a schema that cannot carry target addressing, instead of silently shipping a git tool without it. - headless buildSystemPrompt drops its dead timezone parameter; types.ts gets its trailing newline; the accepted workspace-sync race now cites #2202; the session-family allowlist exemption carries the surfaced-toggle caveat. - review record updated: all 12 findings marked resolved with commit refs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
… push transcript role filters into SQL Two CodeRabbit majors on the dispatched path: - generate() runs machinePage.aiProvider/aiModel but trackUsage recorded the DEFAULTS — every non-default machine agent turn billed at the wrong rate. The generate result now carries the factory resolved provider/modelName, threaded through the engine into trackUsage (defaults remain only for a run that failed before a provider was resolved — no usage to charge there). Red-first: billing-identity test pins the pass-through. - Both transcript reads (read_session tail, loadHistory) filtered roles in JS AFTER `ORDER BY ... LIMIT`, letting system/tool rows consume limit slots and silently shorten the context. The role predicate now lives in the SQL WHERE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
2witstudios
force-pushed
the
pu/machine-pane-fixes
branch
from
July 22, 2026 23:21
ff69c84 to
8511463
Compare
…pu/machine-pane-fixes # Conflicts: # apps/web/src/lib/ai/tools/__tests__/actor-permissions.test.ts # apps/web/src/lib/ai/tools/__tests__/sandbox-tools-runtime.test.ts # apps/web/src/lib/ai/tools/actor-permissions.ts
…aps (review P1/P2 batch) P1 — promotion collision reconcile compares the INSTANCE, not the name: a name is reused across re-creates, so two concurrent provisions can hold two different VMs answering to one sandboxId, and a name-only comparison skipped the kill of the losing instance — an untracked billing VM. The kill stays identity-guarded, so a same-VM false positive is a no-op. P1 — the post-promotion checkout reclaim re-inspects cleanliness IMMEDIATELY before the rm: the original gate ran before the slow provision+clone, and work written into the old checkout during that window was deleted. Anything but a fresh clean skips the reclaim (leftover directory over lost work). P1 — the dispatch conversation claim is now check-insert-recheck: a human stream registering between the liveness pre-check and the claim insert has its own streamId and never collided with the unique index. After the claim row is visible, isClaimContested (pure, unit-tested) re-reads the conversation and the dispatch backs off if any fresh foreign stream appeared — the human always wins, one side yields, no livelock, no table lock. P2 — the dispatched message reaches the model ONCE: loadHistory now takes excludeMessageId (the just-appended user message) since generate() carries that message explicitly; without it every dispatched instruction appeared twice in context. P2 — scrollbackTail enforces the 16 KiB cap even for one newline-free line wider than the cap: keeps the most recent bytes on a UTF-8 boundary with a leading truncation marker, instead of shipping up to the ring 64 KiB. All red-first; realtime branch coverage holds at 98%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
…ane-scope docblock home The reviewer's second pass caught the same four defects mid-fix (blocker + three majors, landed as a89d235 with red-first tests) and one real nit: the Phase-1 pane-scope migration docblock sat stacked on projectStoredNodeScope, describing projectStoredPaneScope twenty lines below. Moved onto its function. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
2witstudios
added a commit
that referenced
this pull request
Jul 23, 2026
fix(machines): close all 15 open review findings from #2204
This was referenced Jul 23, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The Node Sandboxes epic: every machine-tree node (Machine / Project / Branch) is its own sandbox, capability cascades strictly downward, and a machine-bound agent orchestrates its subtree natively through a uniform session family of tools. Started as two prod field fixes (double-created agent rows; machine-pane sandbox denial) and grew into the architecture that removes the bug class.
The eight phases (40 commits)
1 — Pane re-model.
TerminalPaneState.scopenarrowed to{name, kind}; the checkout always derives fromWorkspaceState.scope. Foreign panes are now unrepresentable — 7 defensive branches deleted, bind-time node-equality asserted, read-time projection only (no backfill).2 — Downward cascade.
deriveMachinePaneBindingproduces a derived handle set (machine → self+projects+branches; project → self+its branches; branch → self).bash/file tools/gittake optionaltarget: {project?, branch?}resolved against the set.isMachineAccessibleset-membership is the single policy site — sibling isolation falls out of derivation. Also fixes pre-existing gitopen()missing branchSandbox threading (git silently ran at machine root for branch-bound conversations). Billing/budget stay keyed on the owning machine page (regression-pinned).3 — Storage attribution. Branch Sprites previously billed storage nowhere. Attribution key = owning machine page; opportunistic measurement (never wakes a hibernating Sprite); wired into the usage breakdown.
4 — Session family.
list_sessions(node tree incl. empty nodes + views + sessions + one state-read function) ·add_session(agent|shell; placement: new-view | split-into; born-bound writes byte-matching the client writer) ·move_session(kill-manifestation + placement writer; cross-node refused) ·kill_session·read_session/send_sessionshells dispatching per-surface. Registered for machine-bound conversations only; drive-agent tool set byte-unchanged.ask_agentuntouched.5 — send_session agent engine. Headless loop with the TARGET node's own binding; run-claim vs concurrent client streams;
MAX_AGENT_DEPTH=2; transcript reads with untrusted framing; billing keyed on owning machine page (pinned).6 — PTY IO endpoints. HMAC-signed realtime endpoints (
terminal-activitypattern): scrollback ring tail + honest{live:false}for cold sessions (never fabricated emptiness); stdin via the samesession.command.writea human keystroke uses (echo-correct, counts as activity, resumes the billing clock); control chars delivered verbatim.7 — Lazy project-Sprite promotion. A project's repo stays a checkout on the machine Sprite until the FIRST project-scoped spawn promotes it: CAS-guarded
promoteProject(HMAC key, provision, clone, credential propagation, post-promotion checkout reclaim), dirty-tree refusal that fails the spawn loudly (a silent machine-Sprite fallback would point at a directory the next promotion deletes), promoted-first resolution in lib + realtime, operator routePOST /api/machines/projects/promote, and promoted projects metered for storage as phase 3's third row source (one meter, three subjects).8 — Integration (this PR's final state): phases merged in order with per-merge gates; deferred issues filed; #2202 upgraded to the entity-promotion successor.
Validation
role "test" does not exist).page.enabledToolsallowlists (filterToolsForAgentAllowlistexemption — the family is binding infrastructure, not a composer toggle);add_sessionplans placement before reserving, so a refused placement leaks nothing;machines/projects/promoteregistered in the security-audit route-coverage gate (audited viawriteCodeExecutionAuditinsidepromoteProject).Manual e2e checklist (post-merge / staging):
Known/deferred
js/user-controlled-bypassonapps/web/src/app/api/ai/chat/route.ts(feat(ai): deterministic chat rendering + synchronous send with contextRef #2072 code, outside this diff's hunks) and 1js/insufficient-password-hashonpackages/lib/src/services/machines/project-session.tsthat is a keyed HMAC overSANDBOX_SESSION_SECRETderiving a Sprite-name pseudonym (suppressed in-code with justification, same pattern asbranch-session.ts). Not dismissed here.🤖 Generated with Claude Code
https://claude.ai/code/session_015Lfi3ckdwDHPzKPGFriJik
Summary by CodeRabbit
New Features
Bug Fixes