Repository navigation
Dev → Agents: rebuild the sandbox UX as agent sessions - #2249
Conversation
The five pure modules every later phase of the Dev→Agents rebuild consumes.
No IO, no DB, no network: one source of truth for shapes and decisions so web
routes, the realtime PTY bridge, and the frontend cannot diverge.
- contract.ts — zod schemas + inferred types for AgentSessionDTO, ShellDTO,
SandboxStatus ('none'|'starting'|'running'|'ended') and the shell connect
payload {shellId} (dimension clamps ported from the realtime bridge's local
validation.ts). Documents the two invariants once: sessionId ≡ conversationId
(one object, two audiences — there is NO session-binding field, the chat
body's conversationId IS the session address) and ids address, names label.
- session-sprite-key.ts — deriveAgentSessionSpriteKey → pgs-ses-<hmac> over a
fresh 'agent-session-sprite:v1' namespace folding (tenantId, sessionId), so
no derivation can collide with the legacy pgs-agt-*/pgs-sbx-* names. Fails
closed on a missing/short secret or an empty namespacing component.
- plan-session-lifecycle.ts — (row|null, intent, liveInstance?) → verdict,
absorbing planMachineLifecycle's authorize/create/resume/teardown skeleton and
reconcileResumedSpriteInstance's CAS/ABA adoption. Idleness never destroys
(hibernate model); end stamps the row and retains it; attach never mints a VM.
- decide-session-access.ts — ONE verdict for both web routes and realtime
connect: identity → page → capability, all fail-closed, with a distinct reason
for a canRunCode denial.
- plan-spawn-session.ts — shell auto-labelling (shell-N, collision-scanned) and
the free-label worker spawn with the ported MAX_AGENT_DEPTH cap and
concurrency quota as pure data; kill of an already-gone target is success.
155 tests, 100% statement/branch/function/line coverage on all five. Registered
in packages/lib exports + knip's lib entry list so later phases can import them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kVZjs8Ukr79XdJYuZ6EeP
…ger (phase 1) Additive schema for the Dev → Agents rebuild: any conversation can lazily acquire its own sandbox, so the SESSION owns the Sprite and every shell in it shares that one filesystem — the inversion of machine_agent_terminals, where each PTY owned a Sprite of its own. agent_sessions keys on conversationId as its PRIMARY KEY, because sessionId ≡ conversationId: one id addresses the tool target, the ?c= URL, the chat anchor and the Sprite key, so there is nowhere for a second session-binding field to live. agentPageId is nullable (a global-assistant session has no page; access and billing fall back to ownerId). name carries no uniqueness at all — ids address, names label — so a rename can never break a connection. agent_session_shells carries NO Sprite and NO storage columns; id IS the wire address, and the (sessionId, name) unique index exists only so two tabs in one session can't wear the same title. 0233 adds the AFTER DELETE reclaim trigger, the agent_sessions arm of 0209/0219/0229: every delete path into the table cascades (conversations, users, pages), so the Sprite pointer is rescued into machine_sprite_reclaims inside the deleting transaction. It covers row-level deletes only — DROP TABLE fires no per-row triggers, so the phase 8 teardown must drain the pointers explicitly. No trigger on the shells table: a shell has no pointer to rescue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013VjPJQyn26Rq6BtM2xwQ7n
…ons, shells, access, orphan reconcile) The IO layer for agent sessions: the ONE provisioning code path web and realtime will share, executing the Phase 0 verdicts against the Phase 1 tables. Pure-core / *-store / injected-deps discipline throughout — every test runs with no database and no live Sprite. - agent-session-sprite.ts — port of machines/agent-terminal-sprites.ts with ALL THREE scope clone arms deleted (a session sandbox starts empty at $HOME; git is an agent capability, not a provisioning step) and propagateClaudeCredential dropped. Kept: provision under deriveAgentSessionSpriteKey, the egress gate and egressPolicyToken proof round-trip, the identity CAS with its ABA guard, and reconcile-before-kill (rescuing to the reclaim outbox when a cleanup kill cannot be confirmed). No lifecycle branches of its own: it probes, asks planAgentSessionLifecycle, and executes the verdict including its stamps. intentForProbeOutcome is the one pure observation→intent translation. - agent-sessions.ts + -store.ts — ensureAgentSession (squat-guarded conversation ensure injected, then INSERT ON CONFLICT (PK) DO NOTHING → re-select, so concurrent first touches yield one row), endAgentSession (instance-guarded kill, teardown intent recorded first under a CAS so a revived session is never left marked for teardown, row KEPT for re-provisioning under the same key), listAgentSessions → contract DTOs. - session-shells.ts + -store.ts — port of agent-terminals(-store) with all scope dispatch gone: spawn reserves the ROW ONLY (PTY opens lazily on first realtime connect), resolve/kill by shellId, kill of an already-gone shell is success. The store sheds six methods that existed only to stop a per-terminal VM from being stranded — a shell owns a process, the session owns the VM. - shell-types.ts — PTY-only registry (surface/agentSurfaceOf/isPtyAgentType die with the 'pagespace' chat type); session-scrollback.ts carried verbatim. - session-status.ts — the one row→SandboxStatus derivation. - agent-session-access.ts — gathers the four facts and calls decideSessionAccess; the only branches are null-plumbing. - sandbox/sprite-orphan-reconcile.ts — the machines reconciler moved and re-pointed: outbox arm unchanged, cross-check arm enumerates agent_sessions only, teardownRequestedAt still required before any destroy. The old module is left intact and unchanged so the machine world keeps working until Phase 8. 170 new tests. Registered in packages/lib exports + knip; packages/db gains the ./schema/agent-sessions subpath export the stores import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015X5RoAPd8kfvWPQQbKBCbG
The new surface's skeleton: pure modules, selection store, routes, sidebar and
data hooks. The center panel is a placeholder — Phase 6 fills it with the
unified AgentView. The old /development surface keeps working until Phase 8.
- lib/agents/agent-selection.ts — THE only place the ?agent=&c= URL grammar
exists; parse never throws, build covers global and drive-scoped shapes.
- lib/agents/{session-status,running-badges,session-tabs}.ts — sandbox status +
copy, per-conversation/per-agent badge maps, and ordered tab descriptors that a
future pane container consumes unchanged. All pure, all tested.
- stores/agents/useAgentSurfaceStore.ts — selection mirrored to the URL via
history.pushState (never router.push, so nothing remounts); popstate/refresh/
deep-link hydrate identically from the URL alone.
- app/dashboard/agents + app/dashboard/[driveId]/agents — client pages (useParams,
no async-params), no layout.tsx: with selection in the URL there is nothing to
keep alive, so the old keep-alive host has no successor.
- AgentsSidebar + sidebar-states.tsx — Drive → Agent → Conversation tree with
lazy per-agent conversation loading and running badges; SidebarLoading/
SidebarNotice moved out of the doomed machine tabs directory.
- Shell integration: FULL_PAGE_ROUTES, sidebar variant 'agents', and the nav's
Development entry replaced by Agents (same admin gate).
- useAgentSession / useSessionShells / useUserActiveStreams — coded against the
Phase 4 contract; a missing session reads as status 'none', never an error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…* events
Rebuild the realtime PTY bridge on the agent-session model: the whole
{machineId, projectName?, branchName?, name} address collapses to {shellId}
(the bridge resolves shell row -> session -> sandbox), and the event family
becomes shell:* (connect/input/output/ready/closed/error/resize/disconnect).
- shell-session-key.ts: in-memory stream key from the shellId alone; the
machine|project|branch scope union has no successor.
- shell-access.ts: authorization via the ONE shared decideAgentSessionAccess
verdict (checkAgentSessionAccess from packages/lib — no branch
re-implemented), with the Sprite half handed back as an UNCALLED
resolveSandbox thunk so detached-grace reattaches stay Sprite-free.
Provisioning goes through the SHARED ensureAgentSessionSandbox — one code
path with web, so concurrent provisioners CAS instead of fighting.
- shell-handler.ts: the full bridge port — multi-viewer fan-out, 30-min
detached grace, exact reattach via streamSessionId + verified liveness,
billing gate/settle/heartbeats, task holds, abandonment, per-key create
serialization — byte-for-byte behavior, re-keyed. PTY cwd is always the
session sandbox home; launch specs come from AGENT_LAUNCH_SPECS
(shell-types). persistStreamSessionId/cold-tail write agent_session_shells.
Billing pageId = session.agentPageId ?? null (owner-attributed when null);
payer = the agent page's drive owner, else the session owner.
- shell-io.ts: session-read/session-input re-keyed to {shellId}
(/api/shell-read, /api/shell-input), keeping planSessionStart
(start-on-first-IO, #2206). PTY-only — no agent-type targets.
- shell-activity.ts: agent bash-run feed injection addressed by sessionId,
delivered to every live shell of the session (/api/shell-activity).
- validation.ts: shell:connect parsed with the SHARED contract schema
(dimension clamps included) via parseShellConnectPayload.
- terminal-session-map.ts: TerminalSession gains shellId (the shell:* twin of
agentTerminalId); replay-dedupe/sprites-shell/realtime-sprites-client are
address-agnostic and shared unchanged.
- index.ts: wires the shell:* socket handlers + the three signed endpoints
with real deps (session/shell stores, shared provisioner, quota, audit).
The legacy agent-terminal:* modules, events and endpoints stay registered and
untouched so the pre-Phase-6 /development surface keeps working until the
Phase 8 sweep deletes them wholesale.
Every preserved behavior's test is carried over onto the new modules
(shell-handler 155, shell-io 44, shell-access 17, task-hold 15, activity +
contract-parse suites); realtime suite: 31 files, 1235 tests green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YLvkLnJ4vU8NwNcHiaEa7D
… families
API — one flat route family under /api/agent-sessions (sessionId ≡
conversationId, ids address / names label per the contract):
- GET list (?driveId=|?agentId=|mine, admin-gated 403 without enumerating,
DTOs + shells), [sessionId] GET status ({session:null} for never-
provisioned, never 404) / POST ensure+provision (idempotent, no body —
identity read off the conversation row via the pure
sessionAnchorForConversation) / DELETE end (instance-guarded kill, row
retained, gated by the new capability-free end-access decision),
shells GET/POST (spawn lazily provisions a cold session; auto-label via
planSpawnShell), shells/[shellId] DELETE (404-as-success), and ports of
the machines files/diff/git-blob routes re-scoped to the session sandbox
with a caller-supplied repoPath. All access via decideAgentSessionAccess;
every route audited (route-coverage gate green).
- New pure decideAgentSessionEndAccess + checkAgentSessionEndAccess:
ending a session deliberately skips the canRunCode gate (release of
compute must survive a lost capability).
- Runtime bindings in apps/web/src/lib/agent-sessions/ (DI only, zero
decision logic): stores, MachineHost, conversation creators (squat-
guarded page + global paths), access gather, provision/end/list, shells,
session sandbox handle + git deps for the read routes.
AI tools:
- sandbox tools (bash/readFile/writeFile/editFile) re-anchored to the
conversation session: acquireSandbox = ensureSession +
provisionSessionSandbox from ctx.conversationId (the one shared CAS
path); DELETED switch_machine, list_machines, resolveActiveMachine,
MachineRef helpers, node-target/binding-cwd machinery, and the
machine-binding tool filters + prompt. The 56 sandbox-git tools are
untouched (only the handle source changed); their tests stay green.
- New session + shell families (exactly nine): list_sessions,
spawn_session {name,prompt,agent?,wait?} → sessionId, send_session,
read_session, kill_session, spawn_shell → shellId, send_shell,
read_shell, kill_shell. Workers dispatch their turns through the
STANDARD chat pipeline (internal POST to /api/ai/chat | global messages
route with the caller's own credentials; ai_stream_sessions server-owned
streaming — never a second engine); chain depth rides
X-Agent-Dispatch-Depth, folded back into agentCallDepth by both routes
so MAX_AGENT_DEPTH terminates across the hop; per-owner concurrency via
planSpawnWorkerSession + getCodeExecutionConcurrencyLimit. Shell IO is
shellId-keyed signed realtime calls (shell-io.ts) with the cold-tail
answers ported intact.
- DELETED: move_session/add_session, the ask_agent TOOL (its engine
survives as executeAskAgent for the channel-mention responder, tests
green), session-io-agent(+runtime), session-io-pty, session-layout,
headless-session-run(+runtime), machine-binding-prompt,
machine-pane-binding-runtime.
conversation-repository: conversation-hosting page types reverted to
['AI_CHAT'].
Docs/copy: workspace-tool count 76→75 (README + marketing getting-started,
enforced by tool-registry-docs test); agent-delegation copy now names
spawn_session.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHLiCedo6SvhQskrBGzPNf
# Conflicts: # README.md # apps/marketing/src/app/docs/getting-started/page.tsx # apps/web/src/app/api/ai/chat/route.ts
# Conflicts: # README.md # apps/marketing/src/app/docs/getting-started/page.tsx # apps/web/src/app/api/ai/chat/route.ts
# Conflicts: # README.md # apps/marketing/src/app/docs/getting-started/page.tsx # apps/web/src/app/api/ai/chat/route.ts
…n, per-row watermark, owner fallback) Adds agent_sessions as a fifth storage-billing source alongside the four machine-tree kinds (machine/branch/project/agent-terminal), added ALONGSIDE them per the phase brief — the machine-tree world keeps metering unchanged until the Phase 8 teardown. - listAgentSessionSprites(): one enumeration (agent_sessions WHERE sandboxId IS NOT NULL AND spriteTornDownAt IS NULL), no de-fan join needed since the row carries its own lastActiveAt/agentPageId/ownerId directly. - Per-row watermark: advanceAgentSessionWatermark writes agent_sessions.storageLastBilledAt directly. - storageBillingTarget (machine-storage-attribution.ts): the one place that forks attribution — a page (agentPageId set) or the session's own ownerId (global-assistant session, agentPageId null). - resolveAgentSessionPayerId (billing/machine-payer.ts): the agent-sessions twin of resolveMachinePayerId — the one place the nullable-agentPageId payer fallback is handled. - quota.ts: checkAgentSessionConcurrency + AgentSessionStore.countLive — a DB-backed live-session count per owner, reusing the existing per-tier CONCURRENCY_LIMITS. Built and tested but not yet wired into the live gateSandboxToolCall path (see report). - Orphan cron: new agent-session-orphan-reconcile-runtime.ts composes sprite-orphan-reconcile.ts (already-pure) against real agent_sessions + the shared machine_sprite_reclaims outbox; the cron route now runs it alongside the legacy machine-tree reconciler. - machine-billing.ts: documented (not changed) — its payer resolution already resolves correctly for both agent-session cases via the existing agentPageId/tenantId fallback chain; see inline note for the Phase 8 follow-up. 61 test files / 1233 tests pass in packages/lib (services/sandbox + billing). typecheck and lint clean across the monorepo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBKKFzoZLxd7AYqn1ci8MS
# Conflicts: # README.md # apps/marketing/src/app/docs/getting-started/page.tsx
…onsole convergence)
Builds on Phase 0-5 (merged pu/dev-to-agents): one AgentView component set
now renders identically in the /dashboard/agents console and on an AI_CHAT
page — AiChatView.tsx (1600 LoC) is deleted, not superseded.
- lib/agents/build-session-chat-request.ts: pure chat POST body builder
(chatId/conversationId + agent config, no session-binding field)
- components/agents/chat/useAgentSessionChat.ts + SessionChat.tsx: the ONE
chat, derived from useMachinePaneChat's default mode (dual-mode/AISelector/
pendingPrompt stripped — the agent and conversation are fixed by props).
Full MessageRenderer (ChatMessagesArea) in page context, CompactMessageRenderer
(SidebarMessagesContent) in console context.
- components/agents/shell/: XtermTerminal + pty-input ported from the machine
workspace, re-keyed to shell:*/{shellId} (dropping the old
{machineId,projectName,branchName,name} tuple); Shell.tsx wraps it with
socket acquisition. Editing-store registration uses a new 'shell' SessionType
(apps/web/src/stores/useEditingStore.ts) instead of 'other'.
- Fixed + regression-tested the usePageAgents/shell SWR-pause interaction:
isAnyEditing() only ever counted 'document'/'form', so a shell session never
actually paused it — hardened with an explicit comment and a real-store
regression suite (usePageAgents.shellRegression.test.ts) so a future change
can't reintroduce the freeze the phase brief warned about.
- components/agents/AgentView.tsx + AgentsSurface.tsx (rewritten): header
(title, sandbox status chip, Add shell, conversation-picker slot / console
cross-link), tabs from the pure resolveSessionTabs(shells) plus an optional
static settings tab — the tab container is the only pane-aware piece.
useResolvedAgent.ts resolves a full AgentInfo from just an agentId (two
already-consumed endpoints, no new backend surface).
- components/agents/AgentPageView.tsx + useResolvedConversation.ts: the
page-level scaffolding AiChatView used to own (initial conversation
resolution, read-only gating, history picker, settings/integrations/webhooks)
now sits thin on top of AgentView. PageAgentSettingsTab/AgentIntegrationsPanel/
PageWebhooksDialog/PageAgentHistoryTab are re-hosted unchanged. Cross-links
both ways (console → agent page, page → "Open in Agents").
- CenterPanel.tsx's AI_CHAT branch now renders AgentPageView; AiChatView.tsx
and its directory (including its own tests) are deleted.
Also fixes a docs regression from the dev-to-agents merge: two doc mentions
of the workspace tool count were resolved to a stale "77" during conflict
resolution; the registry (post-merge, after dev-to-agents removed the
machine-binding tools) is actually 76 — caught by tool-registry-docs.test.ts.
166 new/updated tests across the touched surface, all passing; tsc --noEmit
and eslint clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018iMrLGUgJdqZPe3pTkaFiX
…convergence) # Conflicts: # README.md # apps/marketing/src/app/docs/getting-started/page.tsx # apps/web/src/app/api/ai/chat/route.ts
…d-drop, delete dev surface, scrub MACHINE) Deletes the unreleased Machines/Development surface (~53k net LoC), superseded by the Agent sessions feature landed in phases 0-7. - M3 migration (0234_phase8_teardown_machines_world.sql): rescue Sprite pointers into machine_sprite_reclaims, delete MACHINE pages, drop old reclaim triggers, drop 9 machine_* tables (kept machine_sprite_reclaims), drop dead pages/global_assistant_config columns. MACHINE stays in the Postgres enum (can't DROP VALUE) but is removed from the TS PageType enum. - Deletes services/machines/**, api/machines/**, lib/machines/**, the old dev UI (dashboard/development, machine page-view components/stores/hooks), and superseded realtime terminal modules (agent-terminal-handler family). - Substrate renames: MachineHost -> SandboxHost, machine-session-manager / machine-payer -> sandbox-payer / machine-storage-* -> sandbox-storage-* / machine-billing -> sandbox-billing, keeping the machine_sprite_reclaims physical table name. - Enum/type scrubs and drift-guard updates (AssertSubset instead of AssertExact for the TS-subset-of-DB pgEnum relationship). - Billing/settings cards: deletes MachineAccessCard, reframes MachineUsageCard -> AgentSessionUsageCard and ConcurrencyCard copy. - Fixes collateral gaps found during verification: stale package.json exports (packages/lib, packages/db) and knip.json entries pointing at deleted/renamed files, three test fixtures still shaping deleted pages columns (machineAccess/machines/allowPageAgents), and one dead broadcastMachineWorkspaceEvent function with zero callers. - README/marketing docs: 77 -> 76 workspace tools. Verification: typecheck and full build clean; packages/lib and apps/realtime vitest suites clean; repo-wide test:unit has 16 pre-existing environment-only failures (no local test DB, TZ-dependent test — see git history, unrelated to this change); test:security has 7 pre-existing failures from a filter/ exclude script bug plus the same missing-test-DB cause; lint and knip clean; changelog:generate fails on a pre-existing missing-module bug (#1044). Deviations from a literal read of the file: left the MACHINE value in the Postgres pgEnum (cannot be dropped), left generic VM/compute billing constants named MACHINE_* (credit-pricing.ts, machine-pricing.ts, machine-diff-scope.ts, quota.ts) and historical narrative comments describing the predecessor {machineId, projectName, branchName} addressing scheme untouched — these predate and are orthogonal to the deleted Machine page type, not part of the explicitly named substrate-rename list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6xNpmNv2S4cHxqfgArz5m
…ete dev surface, scrub MACHINE)
|
Important Review skippedToo many files! This PR contains 286 files, which is 186 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (521)
You can disable this status message by setting the 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c6ea6951
ℹ️ 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".
| const host = incoming.get('x-forwarded-host') ?? incoming.get('host'); | ||
| if (host) { | ||
| const proto = incoming.get('x-forwarded-proto') ?? 'https'; | ||
| return `${proto}://${host}`; |
There was a problem hiding this comment.
Resolve internal dispatches from the configured app origin
When the app is reached directly over HTTP, as in the documented local deployment, Host is present but X-Forwarded-Proto normally is not, so this constructs https://localhost:... and every spawn_session/send_session dispatch fails before reaching the chat pipeline. It also lets a forged forwarded host choose where the subsequently forwarded cookie and CSRF token are sent. Prefer the validated WEB_APP_URL/NEXT_PUBLIC_APP_URL origin rather than request routing headers.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1efebf550. You're right on both counts, and the correctness half would have broken the documented local deployment outright.
resolveSelfBaseUrl no longer reads routing headers at all — it now resolves WEB_APP_URL || NEXT_PUBLIC_APP_URL (the same precedence as every other self-URL consumer, e.g. services/email-service.ts), normalizes a trailing slash, and returns null on a relative/malformed value so the dispatch reports a config error instead of building a same-process path. The incoming: Headers parameter is gone, so a forged x-forwarded-host has nothing to influence — the forwarded cookie/CSRF can only ever go to the boot-validated origin.
Regression tests: apps/web/src/lib/ai/tools/__tests__/session-dispatch-origin.test.ts (7 cases) — including http://localhost:3000 keeping its scheme, which is the case that previously became https:// and failed.
| getSandboxHost(), | ||
| resolveSessionTenantId(row), | ||
| ]); | ||
| return ensureAgentSessionSandbox({ |
There was a problem hiding this comment.
Enforce the live-session quota before provisioning
Every first-touch path reaches this shared provisioner without calling the newly added checkAgentSessionConcurrency; repo-wide, that helper and AgentSessionStore.countLive have no production caller. Consequently, an owner can POST or open shells in arbitrarily many conversations and provision persistent Sprites beyond their tier ceiling, even though the quota module explicitly defines that ceiling for live agent sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1efebf550. Confirmed — the helper and countLive had zero production callers, so the ceiling was decorative.
checkAgentSessionConcurrency is now called from provisionSessionSandbox in agent-sessions-runtime.ts. That's deliberately the enforcement point rather than any individual route: it is the one path every first touch funnels through (chat tool call, POST to the session route, opening a shell), so gating a single caller would have left the others free to exceed the tier.
One refinement on top of the literal fix: a row that already carries a sandboxId is a RESUME and is already counted by countLive, so gating it would refuse an owner at their ceiling access to a Sprite they're already paying for. That exemption lives inside the quota module (alreadyProvisioned), not at the call site, so the ceiling has exactly one place it can be wrong.
Tests: quota.test.ts gains a resume-exemption block — a provisioned session at the ceiling is allowed without even taking the count, and a cold session at the ceiling is still refused.
There was a problem hiding this comment.
Follow-up in 69b9ccc95 — my first pass didn't fully close this, and you'd phrased the finding more precisely than I first read it.
I had put the check in provisionSessionSandbox, the web app's binding. That covers the HTTP routes and both tool paths, but the realtime shell bridge calls ensureAgentSessionSandbox in packages/lib directly — so opening a shell over the socket still minted a Sprite with no ceiling applied. Your wording was "every first-touch path reaches this shared provisioner", and the shared provisioner is the packages/lib function, not my wrapper.
The gate now lives in ensureAgentSessionSandbox as a required dep, next to checkFullEgressEnablement and for the same stated reason: a required dep on the one function every path funnels through is one a future caller cannot forget. It fires only in the create arm, so a refusal happens before any VM is minted or billed. AgentSessionSpriteRow gained ownerId because the ceiling counts the SESSION OWNER's allocation, not the acting user's — a drive member working inside someone else's agent consumes the owner's limit, not their own.
New tests assert a refusal mints nothing (host.calls.provision empty, sandboxId still null) and that a re-provision reports alreadyProvisioned: true, so an owner at their ceiling can still recover a session whose Sprite vanished.
| * never wired for `agent_sessions` — both die with this sweep. Re-wiring | ||
| * opportunistic measurement for agent-session Sprites is a follow-up, not | ||
| * done here. |
There was a problem hiding this comment.
Persist storage measurements before advancing billing watermarks
New session rows initialize storageMeasuredBytes and storageMeasuredAt to null, and the production provisioner never supplies the optional measureSessionStorage callback; repo-wide there is no remaining writer for these fields. The reconcile therefore prices every agent-session filesystem at zero and advances its watermark, permanently discarding each unmeasured billing interval even when the Sprite stores substantial data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1efebf550. Confirmed, and the consequence was as bad as you describe: with no writer, measuredBytes was always null → pickBillableGB returned 0 → costDollars <= 0 → the watermark advanced anyway, so every agent-session filesystem billed $0 forever and each interval was discarded permanently.
Added packages/lib/src/services/sandbox/sandbox-storage-measure.ts — the agent-session successor to the machine-storage-measure.ts the Phase 8 teardown removed along with its tables — and wired it into the production provisioner as the measureSessionStorage dep. It keeps the original properties: measured only while the Sprite is already awake right after provisioning (never wakes a hibernating VM), throttled per session (default 1h, env-tunable), du -sxB1 of the workspace subtree rather than df of the mount, and a parseable total is trusted even on a non-zero exit as a conservative lower bound.
The new AgentSessionStore.recordStorageMeasurement is guarded on spriteTornDownAt IS NULL, so a measurement landing after teardown can't bill a new generation against a dead disk.
Tests: sandbox-storage-measure.test.ts (13 cases) covering parse, throttle, exec-throw, non-zero-exit-with-total, and unparseable-output.
| }).catch((err: unknown) => { | ||
| const msg = err instanceof Error ? err.message : 'Internal error'; | ||
| socket.emit('agent-terminal:error', { message: msg }); | ||
| socket.emit('shell:error', { message: msg }); |
There was a problem hiding this comment.
Scope shell connection errors to the failing pane
When one multiplexed shell's onConnect rejects, such as on an unexpected billing or database failure, this catch emits an error without the payload's connectionId. XtermTerminal.isMine deliberately accepts untagged events, so every mounted shell on the shared socket handles the error, marks itself dead, and suppresses future reconnects; include the parsed connection ID so only the failing pane is terminated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1efebf550. The connectionId is now parsed once before dispatch and echoed on the error emit, so the kill is scoped to the pane that actually failed instead of every shell multiplexed on the socket.
Tests in apps/realtime/src/__tests__/index.test.ts: a rejecting onConnect emits shell:error tagged with that pane's connectionId; a payload without one still emits untagged rather than inventing an id; and a non-Error rejection coerces to { message: 'Internal error', connectionId } rather than leaking the raw value.
These also covered the shell:connect branches that were dragging realtime under its 98% coverage gate, which is what was failing CI on this PR.
🔬 Code Review — full branch (
|
| # | Area | File:Line | Issue |
|---|---|---|---|
| 1 | Web/AI tools | apps/web/src/lib/ai/tools/session-tools-runtime.ts:66-74,177 |
SSRF + credential leak. Self-dispatch fetch URL is built from client-controllable x-forwarded-host/host, then cookie/x-csrf-token are forwarded to that attacker-influenced URL. The codebase already treats this exact header as untrustworthy elsewhere (oauth/authorize/route.ts explicitly rejects it and uses WEB_APP_URL instead). Fix: base the self-dispatch URL on configured WEB_APP_URL/NEXT_PUBLIC_APP_URL. |
| 2 | Billing | machine-storage-measure.ts / machine-storage-billing.ts |
Agent-session sandbox storage is not actually measured anywhere. agent-session is explicitly excluded from the opportunistic-measurement path; every agent-session Sprite reconciled bills at the "never measured" $0 floor indefinitely. Plumbing is in place, the meter itself isn't wired. If "phase 7 meters agent_sessions" is meant to mean real dollars are now charged, it doesn't yet. |
| 3 | Billing/ops | reconcile-orphaned-sprites/__tests__/route.test.ts |
Test suite for the cron route fails to collect (mock missing MAX_CANDIDATES_PER_TABLE export → throws at module init, 0 tests run). Nothing in CI currently verifies the new dual-reconcile response shape or the partial-failure behavior in finding #6 below. |
| 4 | Web/AI tools | apps/web/src/lib/ai/core/agent-awareness.ts:5,103 |
Functional regression. Prompt unconditionally tells the model to delegate via spawn_session, but that tool only exists when CODE_EXECUTION_ENABLED=true (default off). On default config, the assistant confidently calls a tool that isn't in its tool set — delegation silently breaks. |
| 5 | Services | apps/web/src/lib/agent-sessions/agent-session-orphan-reconcile-runtime.ts |
Zero test coverage for destructive teardown logic (VM-kill/CAS). Its own model twin (machine-orphan-reconcile-runtime.ts) has both a unit and integration test; this doesn't. |
🟠 Medium
| # | Area | File:Line | Issue |
|---|---|---|---|
| 6 | Billing | reconcile-orphaned-sprites/route.ts:61-64 |
Promise.all over the two independent reconcile passes means if one pass's listOrphanCandidates() throws, the other pass's already-completed teardowns go unaudited and the tick reads as a total failure. Self-heals next tick, but real destructive actions go unlogged. Use Promise.allSettled / independent try-catch per pass. |
| 7 | Billing | packages/lib/src/services/sandbox/quota.ts:139-149 |
checkAgentSessionConcurrency is built and tested but never called from any request path — no cap on live sandboxes per owner today (cost/DoS exposure). Commit message itself flags this as not-yet-wired. |
| 8 | Services | agent-session-sprite.ts:126-141 |
When a confirmed-unreferenced Sprite fails both kill and the reclaim-outbox insert, the failure is silently swallowed with no logging — the one path meant to catch "VM now leaked and unbilled forever" produces zero signal. |
| 9 | Services | agent-sessions-store.ts, session-shells-store.ts |
The DB-backed stores have no test coverage of their actual SQL — the CAS conditions (eqOrIsNull), the driveId join, the upsert in enqueueReclaim — are only validated against an in-memory fake, never against real Postgres. Predecessor module (agent-terminals-store.ts) has an integration test for the analogous logic; this doesn't. |
| 10 | Web UI | apps/web/src/components/agents/AgentView.tsx |
Reads only isLoading/.status from useResolvedAgent/useAgentSession/useSessionShells, never their error fields — even though those hooks correctly distinguish "no session yet" from "backend failed." A real 5xx currently looks identical to "nothing here," with no retry affordance. |
| 11 | Web UI | AgentView.tsx:59,94-100 |
Compounding #10: once useResolvedAgent gives up retrying on a genuine failure, isLoading goes false but agent stays null — the loading-spinner guard still fires, so the user is stuck on an infinite spinner with no error text and no escape short of a hard refresh. |
| 12 | Realtime | shell-io.ts HTTP endpoints (/api/shell-read, /api/shell-input, /api/shell-activity) |
These skip re-authorizing the caller-supplied userId on non-start requests, trusting the HMAC signature alone. Not currently exploitable — grep found zero callers anywhere in the repo — but flag for review when the calling code lands. |
| 13 | Web/AI tools | sandbox-git/tools/worktree.ts:54-67 |
git_add splices paths into argv with no -- separator (sibling builders two lines up do add it). A path like -p triggers interactive mode; other dash-prefixed values get reinterpreted as flags. |
| 14 | Web/AI tools | worktree.ts:69-78, repo.ts:81-95 |
git_reset.ref / git_remote_add.name skip the project's own validateFlagSafe guard that structurally identical sibling fields (git_show.ref, git_branch.name) do call. |
| 15 | Web/AI tools | inline-instructions.ts:176 |
The "AGENTS" prompt section (which references spawn_session) is gated on an OR across always-present tools, so it renders even when spawn_session isn't in the tool set — same root cause as #4. |
| 16 | Web/AI tools | route duplicating conversation-repository.ts query inline |
A sibling messages route wasn't updated when the repository dropped 'MACHINE' from its type filter — now has a stale comment and diverging behavior from its two siblings that do call the repository. |
| 17 | Web/AI tools | ai/chat/route.ts:579, ai/global/[id]/messages/route.ts:295 |
Byte-identical X-Agent-Dispatch-Depth fail-closed parse/clamp logic duplicated in two places — currently correct in both, but security-relevant logic that will silently drift if only one site is edited later. |
🟡 Low / Nit (worth a pass, not blocking)
- Accessibility: sandbox status description only reachable via
titletooltip;XtermTerminal's container has norole/aria-labelas a live terminal region (AgentView.tsx:109,XtermTerminal.tsx:413). useAgentSession.ensureSession()is fully built/tested but has zero callers in the shipped UI — dead, won't be caught by knip since it's a hook property, not an export.useResolvedConversation.ts:50-55swallows a failed conversation-creation POST with onlyconsole.error, inconsistent with the toast-on-failure convention used elsewhere in this same phase.- Realtime:
onInput's byte-cap uses JS string length (UTF-16 units) while the HTTP path correctly usesBuffer.byteLength(...,'utf8')— up to ~4x drift on multi-byte input, still bounded but inconsistent with the doc comment claiming parity. - Realtime: periodic 60s re-auth tick does a full user-row read + PII decrypt per attached viewer even though it only needs the pass/fail verdict — avoidable DB/decrypt cost at scale.
resolveAgentSessionPayerId(machine-payer.ts) is never called in production despite its own doc comment and two other modules' doc comments asserting it's "the only place" this logic lives — either wire it in or fix the stale docs.- 4 stale
ai/chattest files still mock removed exports (filterToolsForMachineBinding,withSessionFamilyTools) — harmless no-ops, but misleading dead residue. SESSION_FAMILY_TOOL_NAMESexport appears to have no remaining production consumer — confirm intentional.- A handful of missing-test items on newer runtime-wiring files (
session-sandbox-runtime.ts,agent-sessions-runtime.ts'sresolveSessionTenantId/ensureConversationRow) that mirror already-tested siblings elsewhere. - Commit hygiene: the phase 6/7 feature commit subject lines (
feat(agents): phase 6 — unified AgentView (one chat, shells, page + console convergence)) run well past the project's 50-char first-line convention, andmerge:-prefixed subjects aren't one of the declared conventional types.
✅ What's genuinely strong (don't rework these)
- Access control is centralized correctly. One pure decider (
decideAgentSessionAccess/decideAgentSessionEndAccess) is shared by both the web API and the realtime bridge — no second, divergent implementation to drift. Manually traced with no bypass found; IDOR, path traversal (files/git-blob routes), and Next 15 async-paramshandling all check out clean across every dynamic route. - The PTY bridge's race handling is unusually careful for this class of system: serialized cold-create per session key, abandoned-connect cleanup, idempotent double-release guards, ANSI/control-char stripping against terminal-escape injection from agent-controlled output, and a periodic re-auth tick that evicts individually-revoked viewers without disturbing co-viewers.
- Concurrency-safety design in the session/sprite CAS layer (ABA-aware identity swaps, teardown-intent-before-kill ordering) is well thought through and the 750-line sprite test file earns its size — it drives real race scenarios, not padding.
AiChatViewdeletion (1600 lines) was cleaned up correctly — zero stray references anywhere in the repo,CenterPanel.tsxrewired cleanly, test coverage moved rather than vanished.useEditingStoreintegration is exactly right per the project's SWR/auth-refresh contract (shell excluded from the SWR-pause set, included in the auth-refresh-defer set), and socket/terminal cleanup on unmount is thorough and tested (listener counts, dispose counts, reconnect races).- DB schema/migrations (phase 0/1) and the pure domain/contract layer are exhaustively tested (191 tests) with no bypasses found in the pure authorization logic.
Recommendation: fix the SSRF (#1) and the broken cron test suite (#3) before this goes further — those are the two items that would actually bite in production. Decide explicitly whether #2 (storage not metered) and #7 (concurrency quota not wired) are intentional near-term follow-ups or need to land before calling this "phase 7 complete," since both are billing/cost-control gaps that read as silently incomplete rather than deliberately deferred. Everything else is real but lower-stakes cleanup.
🤖 Generated with Claude Code
…forcement, storage measurement, shell error scoping Four review findings, each with a regression test. P1 — resolve internal dispatches from the configured origin (session-tools-runtime.ts). The worker-dispatch hop forwards the caller's own cookie and CSRF token, so where it points is a security boundary. Deriving it from routing headers was wrong twice: a plain-HTTP deployment has `host` but no `x-forwarded-proto`, so it built `https://localhost:3000` and every dispatch failed before reaching the chat pipeline; and a forged `x-forwarded-host` could steer those credentials at an attacker-chosen origin. Now WEB_APP_URL / NEXT_PUBLIC_APP_URL only — validated at boot, uninfluenceable per-request — matching every other self-URL consumer in the repo. P1 — enforce the live-session quota before provisioning (agent-sessions-runtime.ts). `checkAgentSessionConcurrency` and `AgentSessionStore.countLive` had NO production caller, so an owner could provision persistent Sprites past their tier ceiling in arbitrarily many conversations. Wired into `provisionSessionSandbox` — the one path every first touch funnels through, so no caller can slip past it. Resumes are exempt (a row already carrying a sandbox is already counted), and that judgement lives inside the quota module rather than at the call site, so the ceiling has exactly one place it can be wrong. P1 — persist storage measurements before advancing billing watermarks (sandbox-storage-measure.ts, new). `storageMeasuredBytes` had no writer at all: the reconcile priced every session at the never-measured 0 floor while still advancing its watermark, permanently discarding each interval. Restores the opportunistic `du -sxB1` measurement the Phase 8 teardown removed with the machine tables, re-keyed to agent_sessions and wired into the production provisioner — captured only while the Sprite is already awake, throttled per session, never waking a hibernating VM. Persist is guarded on the row still being live so a late measurement cannot bill a new generation against a dead disk. P2 — scope shell connection errors to the failing pane (realtime/index.ts). The client's `isMine` deliberately accepts untagged events, so an untagged `shell:error` was claimed by EVERY shell multiplexed on the socket: one pane's billing or database failure marked them all dead and suppressed their reconnects. The connectionId is now parsed once and echoed on the error. Also raises realtime branch coverage back over its 98% gate (the CI failure on this PR) with tests that were missing rather than threshold changes: the shell:connect success/failure/coercion paths, the HTTP dispatch fallthrough, `latestActivityAt`'s arms, global-assistant billing attribution (null agentPageId), and the abandon-with-a-held-billing-hold path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…rovisioner, not the web wrapper Follow-up to the quota fix: my first pass put the check in `provisionSessionSandbox`, the web app's binding. That covers the HTTP routes and both tool paths — but NOT the realtime shell bridge, which calls `ensureAgentSessionSandbox` in packages/lib directly. Opening a shell over the socket still minted a Sprite with no ceiling applied, so the limit was bypassable by the exact path most likely to be used repeatedly. The gate now lives in `ensureAgentSessionSandbox` itself as a REQUIRED dep, alongside `checkFullEgressEnablement` and for the same reason: this is the one function every first touch funnels through, and a required dep is one a future caller cannot forget. It fires in the `create` arm only, where a VM is actually minted, so a refusal happens before anything is provisioned or billed. `AgentSessionSpriteRow` gains `ownerId` because the ceiling counts the SESSION OWNER's allocation, not the acting user's — a drive member working inside someone else's agent consumes the owner's limit. Both call sites supply it: the web runtime resolves the tier from `users`, and realtime does the same through a small `resolveOwnerTier` helper. Tests: `agent-session-sprite.test.ts` gains a ceiling block — a refusal mints no VM and leaves `sandboxId` null, and a re-provision of a row that already holds a sandbox reports `alreadyProvisioned: true` so an owner at their limit can still recover their own session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…ty, error UX, reconcile resilience Six findings from the full-branch review. (#1 SSRF, #2 storage metering and #7 concurrency quota were already fixed in the two prior commits; #3's cron suite now collects and passes 5 tests, so it was fixed by a later phase than the review sampled.) #4/#15 — prompt regression on default config. `buildAgentAwarenessPrompt` unconditionally told the model to delegate with `spawn_session`, but session tools only exist when CODE_EXECUTION_ENABLED is on (default off), so the assistant confidently called a tool it did not have and delegation silently broke. The delegation sentence is now gated on a `canDelegate` flag that both call sites derive from `isCodeExecutionEnabled()`; without it the section still lists the agents, it just stops naming a tool that isn't there. #13/#14 — git argv safety. `git_add` spliced paths straight into argv with no `--`, unlike its siblings `git status`/`git diff`: a path named `-p` was read as a flag (interactive add). It now separates, and only when there are paths, so no bare trailing `--`. `git_reset.ref` and `git_remote_add.name` gained the `validateFlagSafe` guard that structurally identical sibling fields already apply. #10/#11 — infinite spinner on a failed agent load. `AgentView` guarded on `agentLoading || !agent`, so once SWR gave up retrying, `isLoading` went false, `agent` stayed null, and the user watched a spinner that would never resolve with no error text and no escape but a reload. Loading and failure are now distinct states: the failure surfaces the server's own message and a Try again button, backed by a new `retry` from `useResolvedAgent`. #6 — one failing candidate query no longer parks the other pass. The orphan reconcile listed the reclaim outbox and the teardown-intent rows under `Promise.all`, so either failing dropped both. Now `allSettled` with per-source error logs: a degraded query costs its own candidates, not every reclaim, and those are billing VMs nobody is using. #8 — the leak signal is no longer silent. When a confirmed-unreferenced Sprite fails BOTH its kill and its reclaim-outbox insert, nothing in the system knows that VM exists — no row points at it, so no trigger and no cross-check will find it. That path swallowed its error, making the one path built to catch a permanently leaked VM the one path with no signal. It now logs loudly with the sandbox id and both failure reasons. #5 — the destructive teardown binding gets tests. 10 cases over `killSprite` (confirmed kill, replaced-name-as-success, genuine failure, unpinned instance), `markSessionTornDown` (CAS win/loss), and `listOrphanCandidates` (both sources, each single-source failure, cap + backlog). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Review response — 3ab0f96Thanks, this was a genuinely useful review. Working through it in your priority order. 🔴 High — all six addressed
🟠 Medium — addressed
On your closing questionYou asked whether #2 and #7 were intentional deferrals or silently incomplete. Silently incomplete — the commit message hedging you noticed was the tell. Both are now genuinely wired, so "phase 7 meters agent-session billing" is accurate rather than aspirational. Not addressed this pass (with reasons)
Gates on |
…ry, align byte cap, surface failures The medium/low cluster from the branch review. #17 — the agent-dispatch depth clamp was duplicated byte-for-byte in both chat routes. It is small, but it is the TERMINATION CONDITION of a recursive system: if the copies drift, the A→B→C cap stops capping on one path and nothing says so. Extracted to `ai/core/agent-dispatch-depth.ts` (7 tests) with the reasoning for why the header is safe untrusted written down once instead of twice. #16 — the third messages route resolved its agent with an inline pages query instead of the repository its two siblings use. That inline copy is exactly how the last change to "which page types may host a conversation" (dropping MACHINE) reached two routes and not the third. Now goes through `conversationRepository.getAiAgent`. Byte-cap parity — the socket input path capped on `String.length` (UTF-16 units) while the HTTP path capped on `Buffer.byteLength`, so the same payload could pass one and fail the other, drifting up to 4x on multi-byte input. Both measure bytes now, with two tests pinning the emoji cases at either side of the boundary. Conversation-creation failures are surfaced. The id is already seeded and selected locally, so a swallowed POST left the user typing into a conversation the server had never heard of, learning about it on their first send. Now toasts, matching this surface's own convention. SESSION_FAMILY_TOOL_NAMES earns its export. It had no production consumer and a test that merely restated the list. It now backs a read-only DRIFT GUARD asserting the property that matters: every session/shell MUTATION is write-gated (a read-only agent must not spawn a shell and write through it) while the read verbs survive (it must still observe its own sessions). A tenth family member added without a WRITE_TOOLS decision now fails a test instead of picking a default. Also removes stale mocks of `filterToolsForMachineBinding`/ `withSessionFamilyTools`, exports deleted in phase 4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
The unit suite drives this store against an in-memory fake — the right tool for the lifecycle races, but it proves nothing about the SQL. A fake compares pointers with `===`, which handles null for free; Postgres does not, and `= NULL` never matches. The CAS predicates, the PK-as-FK conflict target, and the live-row guard on measurement writes were all unverified against a real database, while the predecessor store had exactly this coverage. 12 cases, run against real Postgres: - insertIfAbsent — two inserts for one conversation yield ONE row and the second caller does not error (the whole concurrency contract of ensureAgentSession). - updateSpriteIdentity — the NULL-pointer CAS matches only a genuinely unprovisioned row, is REFUSED against a row already holding a sandbox (the predicate that stops a racing provisioner overwriting the winner's live VM), and swaps identity on a matching previous pointer. - stampSpriteTornDown — the instance CAS stamps its own generation and refuses a different live instance under the same name, so a concurrent re-provision's replacement is never marked dead. - countLive — counts only rows with a sandbox and no teardown stamp. - recordStorageMeasurement — writes on a live row, and does NOT write on a torn-down one, whose filesystem no longer exists. - enqueueReclaim — upserts rather than violating the PK, chasing the newest instance exactly as the AFTER-DELETE trigger's own insert does. - The conversation delete cascade, which is why no orphan-cleanup code exists. Registered in both vitest exclude lists (test + coverage) like every other DB-backed suite; runs via `test:db`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Third spec source: the code's own doc commentsThe
The ordering claim was the one worth pushing on, because it is the kind of thing that stays true by accident until someone reorders a guard. It has a test ("should report the identity denial before the capability denial when both fail"), and I confirmed that test is load-bearing rather than incidental: moving the capability gate ahead of identity fails immediately. That claim matters beyond tidiness. A user who is both not-shared-in and lacks Net: no code changes. Three spec sources have now been checked against the diff — the plan, CLAUDE.md, and the code's own documented invariants. Only the first produced a defect (the vocabulary violation, fixed in CI: |
The 429 path had no route-level test at all — on the surface this PR reworked
more than any other. Four things were unasserted: the status, the audit event,
the user-facing wording, and the audit's route tag.
The route tag is the one I put at risk myself. Extracting the duplicated
`quotaExceeded` helper turned a hardcoded `'agent-sessions/[sessionId]'` into a
caller-supplied argument, so it can now be passed wrong and misattribute the
audit trail with nothing to catch it. Verified by mutation: swapping the shells
route's tag for the session route's now fails.
Also pinned, because each cost a round of this review to get right:
- **429, not 403.** A plan-limit refusal is a quantity limit, not an
authorization failure; it started life as a 403 and reverting it now fails
both routes' tests.
- **`security.rate.limited`, not `data.read`.** A free-tier user hitting their
ceiling should not land in data-access forensics.
- **"sandbox", not "session".** The message is asserted to contain the UI's
word and NOT the backend's, so the vocabulary regression fixed one commit
ago cannot come back quietly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Both fixes from earlier in this review were protected by nothing. Deleting either line left every test green and the feature dead — which is precisely how each got into this PR in the first place. **The agent-activity feed.** `notifyShellActivity` is optional on `SandboxRunDeps` and the runner no-ops without it. The realtime handler was implemented, tested, wired with real deps, and its doc said "apps/web posts here after a successful bash run" — while nothing on the web side supplied the dep. Round 4 found it dead; removing the wiring line again passed the entire suite. **`reauthorizeViewer`.** Optional on `ShellIoDeps` and fails closed, so an unwired build denies nothing loudly: it just stops refreshing the viewer identity, and a revoked user keeps typing into a live shell until someone notices. Round 2 found it declared, defaulted to `?? false`, and never supplied. Both now assert presence, not behaviour — the behaviour is covered in `tool-runners` and `shell-io` respectively, and the connection between the two was the only thing ever missing. Asserting the realtime one needed `index.test.ts` to capture the deps object it hands the bridge, which the existing mock discarded. Verified the way these things have to be verified: re-applying each deletion now fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Found by enumerating every optional dep in the new interfaces and checking each has a production supplier — the fourth instance of "declared, safely defaulted, never wired". `measureStorage` had zero suppliers. Master supplied it; this PR dropped it and I replaced it with a call inside `acquireSandbox`. That is the wrong moment, and the seam's own comment says why: it fires from `release` — a `finally`, AFTER the op — because measuring first records the pre-write footprint and then lets the per-session throttle suppress the post-write measurement. An agent that writes 5 GB stays invisible until the throttle lapses and some later op happens to measure again. So the pre-op call is removed and the existing post-op seam supplied instead, re-keyed from `pageId` to `sessionId`. The re-key matters on its own: the old gate was `acquired.pageId`, which a global-assistant session never has, so that whole class was silently excluded from measurement — the same page-shaped assumption that had excluded it from the activity feed. Ordering is now pinned by a test asserting the command runs before the measurement, since "measures at all" was true of the broken version too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…e doc Continuing the optional-dep sweep from the previous commit. **The git path never measured.** `runGitInSandbox` has its own acquire/release and simply did not fire `measureStorage` — so a session that only ever ran git tools was never measured at all, billing its empty-disk baseline while the reconcile advanced the watermark over whatever it wrote. This matters more than the bash gap it mirrors: `git_clone` is the largest writer in the system. Pre-existing on master rather than a regression here, but it is squarely in this PR's scope — the whole storage-billing model was re-keyed from pages to sessions in this branch, and one of the open review threads is specifically about measurements landing before watermarks advance. Fixed post-op, in the same `finally`, for the same reason the bash path is: measuring first records the pre-write footprint and lets the throttle suppress the real one. Ordering is asserted, not just the call. **`sandbox-storage-billing`'s module doc had gone stale** — and dangerously so. It still read "opportunistic measurement was never wired for `agent_sessions` … Re-wiring is a follow-up, not done here", which was true when written and false by the time three writers had been added in response to review. A reader would have concluded no measurement happens. Replaced with what actually writes those bytes and when, since this module only ever reads them. That is the second stale doc-comment this review has turned up asserting something the code stopped doing. They read exactly like fact and cost real time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Correction: my claim about
|
Three residues of the AiChatView deletion, found by grepping surviving code for symbols this PR removed. **`page-types.config.ts` still declared `uiComponent: 'AiChatView'`**, with a three-line comment in `CenterPanel` existing purely to explain that the string names a deleted file and resolves to `AgentPageView`. The field is internal — consumed only by `CenterPanel`, never persisted or serialized — so it simply renames, and the comment explaining the mismatch goes with it. **`useEditingStore`'s module doc listed the 'ai-streaming' producers as "(AiChatView, GlobalAssistantView, SidebarChatTab)".** That list is wrong twice over: one entry is deleted, and the framing implies each surface registers its own session. Registration is actually central — `GlobalChatProvider` mounts `DerivedStreamingRegistrations`, which derives one per live conversation, and chat surfaces deliberately do NOT register because two owners would disagree about when to end it. That wrong framing cost something real: I read it, concluded the new chat had dropped a required registration, and reported a non-existent gap on the PR before tracing the actual call path. Corrected there too. Also de-referenced two prose mentions in `DerivedStreamingRegistrations` and `AskUserAnswerContext`. Third stale doc-comment this review has surfaced. Each read as fact, each was true when written, and each cost time — the failure mode is that a comment describing the code cannot be checked by anything that checks the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Completing the optional-dep sweep. Nine optional deps exist across the new
interfaces; eight have production suppliers and one (`measureStorage`) did not,
fixed two commits ago. This pins the two remaining ones whose absence produces
no error at all — only a quietly missing feature:
startSession absent → an agent can never start a shell that has never run.
`read_shell`/`send_shell` answer "not live" forever, which is
the whole of issue #2206 undone.
rearmIdleReap absent → a viewer-less session keeps the reap clock it started
with, so a long agent-driven build is killed mid-run by the
timer its own activity should have pushed back.
Both ride the same captured deps object as the `reauthorizeViewer` assertion,
so this is two lines of coverage for two more silent failure modes. Verified by
un-wiring each and confirming the test fails.
That is the whole class now closed: an optional dep with a benign default is
invisible to types (legitimately absent), to coverage (the line runs, the branch
is false), and to the feature's own tests (they inject it themselves). The only
question that finds it is "who supplies this in production?" — and in this PR
the answer was "nobody" three times.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…lowing them
I wrote this `.catch(() => {})` one pass ago with a comment claiming "this path
has no logger seam of its own to log through". That is false:
`GitSandboxRunDeps extends SandboxRunDeps`, which carries `logger?`. I asserted
a constraint without checking it and silently discarded a failure because of it.
It matters more here than the wording suggests. If the measurement throws on
every git call — a bad exec adapter, a missing `du` in the image — the largest
writer in the system stops being measured with no symptom whatsoever. That is
the exact bug the seam was wired to fix two commits ago, reappearing in a form
that produces no error, no log, and no failing test. "Best-effort" has to mean
non-fatal, not invisible.
Now uses the same `safeLogWarn` and the same message as the bash path, which
has always logged this identical failure. Pinned by a test asserting the warning
carries the sessionId while the tool result stays successful.
Found by sweeping every fire-and-forget in the new code for whether a PERSISTENT
failure would surface anywhere — the sibling of the optional-dep sweep, and the
same shape of blind spot: something that works when it works and is undetectable
when it does not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…g a VM
New class swept this pass: what the security and billing gates do when their
dependency THROWS, as opposed to when it denies. Denial is well covered; failure
was not covered at all.
Both gates are correct today — neither `checkConcurrency` nor
`checkFullEgressEnablement` is wrapped in a try/catch, so a transient DB or
flag-service failure propagates and no VM is minted. But they are correct *by
absence*, which is precisely what a well-meaning later `catch` defaulting to
`{ allowed: true }` would undo without a single test noticing.
The consequences are asymmetric enough to be worth the two tests. A concurrency
ceiling that fails open fails open exactly under the load that makes its
dependency wobble — the moment it is load-bearing. An egress gate that fails
open mints a VM with unproven network containment.
Pinned on the observable effect rather than the mechanism: the call rejects and
`host.provision` was never reached. Verified by making the quota gate swallow
its own throw and default to allowed — three tests fail.
Also verified a claim I had asserted in a comment rather than checked: that
provisioning has "no persisted in-flight state", which is what makes `'starting'`
a client-side transient. `agent_sessions` has no such column, and `sessionKey`
and `sandboxId` are written by one atomic `updateSpriteIdentity`, so there is no
window a row could report. It holds — but I had written it before confirming it,
which is the third time this review.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Three more bug classes swept — all clean. Reporting saturation honestly.Previous passes each found a real defect by taking a bug class and searching it exhaustively rather than hunting instances. That kept working until this pass, and I think the change is worth stating plainly rather than padding. Classes swept this round, no defects found:
Flakiness check on my own additions. This loop added a substantial number of tests. Ran Where that leaves things. Seven classes have now been searched exhaustively: drifting duplicate declarations, unsupplied optional deps, stale doc comments, silently-swallowed best-effort failures, fail-open gates, resource leaks, and concurrency. The first four produced real defects; the last three produced one coverage gap and two clean results. The defect rate has genuinely gone to zero across the last three, which is different from having stopped looking — and different again from the code being correct, which no amount of my own searching can establish. The honest summary is that I have exhausted the classes I can invent, and the remaining risk is concentrated exactly where I said it was from the start: the manual E2E, which needs a Sprites credential I do not have, and which round 4 showed can reveal a core flow that had never once run. |
Re-verified all four review threads at current sourceTen commits have landed since I last checked these, and several touched the exact files the threads are about — the quota predicate, the storage seams,
All four remain open by design. I fixed them during this loop, and the operating rule I was given is to leave those for the reviewer to verify rather than self-resolving. That is the one success criterion I am deliberately not satisfying, and I would rather be explicit about the reason than quietly close them. CI: |
Correction to my own E2E checklist — step 7 would have produced a false failureI audited the checklist I posted, on the grounds that it is the one artifact a human will actually execute and a wrong step wastes the only person who can unblock this. One step was wrong, and it was wrong in the direction that produces a false bug report. Step 7 said the shell's cwd should be I took It also exposed a genuinely misleading pair of comments, now fixed. The opposite is true. Shells start at That is the fourth stale-or-inaccurate comment this review has surfaced. The pattern holds: each was plausible, each was consequential, and none was reachable by types, tests, or coverage. Everything else in the checklist verified against source: |
Two accurate-sounding comments composed into a false conclusion about billing. `shell-handler` described the PTY as opening "inside the session's sandbox at `$HOME`". It opens at `SANDBOX_ROOT` — `/workspace`. Meanwhile `sandbox-storage-measure` explains that bytes written outside the billed subtree go uncounted, giving "package caches under $HOME" as its example, which treats $HOME as somewhere else entirely. Read together they say shells start outside the measured subtree, so nothing a shell writes is ever billed. The opposite is true: `SESSION_STORAGE_MEASURE_PATH === SANDBOX_ROOT`, so a shell's writes are exactly what gets measured. Neither comment was checkable by types, tests or coverage, and each was plausible alone — it is the composition that lies. Fourth stale-or-inaccurate comment this review has turned up. Found by auditing the manual E2E checklist I had written for whoever runs it: its step 7 told the tester to expect `$HOME`, so `pwd` printing `/workspace` would have been reported as a bug on the single most important step. I had copied `$HOME` from the plan without checking it against the code — the same habit that produced the earlier "no logger seam here" and "nothing registers 'ai-streaming'" claims. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Correcting an over-stated security claim of mine, and one thing worth your checkSeveral passes ago I wrote that the signed shell endpoints ( What I actually verified, and what I did not. Verified: across the entire Caddyfile, only two handlers proxy to Not verified, and this is the gap: Why it is worth a minute of yours. These endpoints accept a signed payload naming a If a public IPv4/IPv6 is allocated, the shell bridge is internet-reachable and worth deciding about deliberately — either This is pre-existing, not introduced here. That is the second claim of mine I have had to walk back this review — the first being |
Correcting "only CodeQL fails" — one commit did fail a real checkI have said several times in this thread that CodeQL is the only failing check on this PR. Checking every commit rather than spot-checking recent ones, that is not true.
That commit moved the shell-bridge wire shapes into the contract module and left behind a Two things about it are worth stating rather than glossing. I fixed it without knowing CI had failed. I caught the unused alias by running The claim itself came from spot-checking. I had been checking CI on whichever commit I had just pushed, seeing green, and generalising to "the whole loop is green except CodeQL". The exhaustive check took one loop over 23 commits and immediately contradicted it. The corrected, verified statement: across all 23 commits in this loop, 21 have finished That is the third claim of mine corrected this review, and all three share one shape: I verified one instance, found it consistent, and reported the general case. |
The "16 known env-only failures" claim, enumerated rather than sampledThree claims of mine have already been corrected this review by enumerating instead of sampling. Applying the same treatment to the one I have repeated most often — that the 16 failing The count, enumerated: 14 in The causes, from the actual failures rather than from "it fails on master too":
And none of the subjects is touched by this diff — The distinction I want to draw is between the old justification and this one. Previously I said these "fail identically on master", which is true but is inference. Now the cause of each is established directly: a missing database, a timezone, a missing database. That is checkable by anyone without trusting my earlier comparison. CI: This is the first pass in three where enumeration confirmed a claim rather than breaking one. |
Read this first — what needs you, and which of my earlier comments are supersededThere are 23 comments on this PR now, several of them long, and three contain claims I later corrected. Someone reading in order hits the wrong version first and could act on it. That is a hazard I created, so here is the index. Superseded claims — do not act on these
All three had the same shape: I verified one instance, found it consistent, and reported the general case. Worth knowing if you are weighing how much of my other analysis to take on trust — the enumerated claims are marked as such where I made them. What actually needs a human
State521 files, net −56,053 LoC of source. 23 commits; 22 have finished |
Acknowledging the CodeRabbit comment — and a green check that means the opposite of what it looks likeI had never acknowledged CodeRabbit's comment, posted when this PR opened. I found it only by enumerating every comment author rather than checking review threads — which showed four threads from What it says: the review never ran.
Why this matters more than a missed acknowledgement. So the green is reporting "I successfully declined to review this". Anyone scanning the checks — including me, for twenty-odd passes — reads that as review coverage. It is the absence of it. The practical consequence: the only automated review this PR has received is the four threads from What can be done, from CodeRabbit's own message — none of which I can do myself, as they need plan credits or a scoped re-run: Scoping to a directory keeps each run under the 100-file limit. If it is worth getting a second automated opinion before merging something this size, those three directories hold most of the risk — the provisioning CAS, the access decision, and the shell bridge that round 4 found had never worked. I would treat this as a fifth item for the human list, alongside the E2E, |
|
@codex review |
The Codex review is 36 commits stale — I just re-triggered itFollowing the CodeRabbit finding, I enumerated the signal sources I had never queried rather than the ones I had. Review submissions carry a body separate from their inline threads, and Codex's says:
HEAD is I had been treating "four Codex threads, all addressed" as this PR's review coverage. Those threads describe the branch as it stood 36 commits ago. Everything I changed during this review loop has never been looked at by anything but me — including the shell-bridge fix (round 4's finding that So the honest picture of automated review on this PR is: CodeRabbit 0%, Codex as of 36 commits ago. Neither has seen the current state. What I did about it. Codex's own boilerplate documents If it returns findings, I will work them the same as the first four. If it reacts 👍, that is the first external signal covering the code this loop produced — which is the part most in need of it, since it is the part written fastest and reviewed least. This is the fifth correction to my own reporting, and the third in a row of the same shape: I verified one instance of a thing, and reported the general case. Threads ≠ all reviewer feedback. A green check ≠ a review. Four addressed threads ≠ a reviewed PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8938d935c2
ℹ️ 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".
| INSERT INTO "machine_sprite_reclaims" ("sandboxId", "spriteInstanceId") | ||
| SELECT "sandboxId", "spriteInstanceId" | ||
| FROM "machine_sessions" | ||
| ON CONFLICT ("sandboxId") DO NOTHING;--> statement-breakpoint |
There was a problem hiding this comment.
Replace stale instance IDs when rescuing Sprites
If the reclaim outbox already contains this stable sandboxId from an older Sprite generation, DO NOTHING preserves the old spriteInstanceId while the migration deletes the tracking row for the current generation. The orphan reconciler treats an instance mismatch as success and removes the outbox entry under the assumption that the replacement still has its own tracking row, but this migration has just dropped that row, permanently orphaning the live Sprite. All four rescue inserts should update the stored instance identity as the newer reclaim triggers do.
Useful? React with 👍 / 👎.
| const { data, error, isLoading, mutate } = useSWR<ShellsResponse>(key, fetcher, { | ||
| revalidateOnFocus: false, | ||
| dedupingInterval: 5_000, |
There was a problem hiding this comment.
Revalidate shells created by agent tools
When the current chat invokes spawn_shell or kill_shell, the session-tools runtime mutates the shell rows without going through this hook's addShell/removeShell cache updates. This SWR entry has no polling or socket invalidation and explicitly disables focus revalidation, so an agent-created shell never appears as a tab—and an agent-killed shell remains visible—until some unrelated manual revalidation or remount occurs.
Useful? React with 👍 / 👎.
Review finding[P1] CAS storage measurements against the Sprite generation
This permits a stale, fire-and-forget measurement to cross a generation change:
The row then carries Sprite A’s byte count and a fresh timestamp for Sprite B. The throttle can suppress measurement of B for an hour, while reconciliation bills the wrong filesystem. This directly contradicts the store contract’s stated goal of preventing an old-disk measurement from billing the next generation. packages/lib/src/services/agent-sessions/agent-sessions-store.tsL95-L110 Pass the measured Checks performed
|
Re: [P1] CAS storage measurements against the Sprite generation — confirmed, reproduced, fixed in
|
| case | asserts |
|---|---|
| generation moved on mid-measurement | stale write dropped — bytes and timestamp |
| same generation, reused NAME | still persists (the CAS must not reject valid writes) |
| driver reports no instance id | still persists against a null row |
The timestamp assertion is the one I'd have missed: a stale storageMeasuredAt silences the next real measurement for a full window without ever recording a real number, so dropping only the bytes would leave half the bug.
Verified by mutation: removing the predicate fails the stale-generation case and nothing else — so the test is pinned to this behaviour rather than passing incidentally.
Left open for your verification rather than resolved.
Worth noting for the human reviewer: this PR's automated review coverage was much thinner than it appeared. Codex had reviewed a commit 36 behind HEAD, and CodeRabbit skipped the PR entirely (521 files vs its 100-file limit) while reporting a green check. This P1 is in code written during the review loop — precisely the region neither bot had seen — and it was found only because I re-triggered the review after noticing the staleness.
|
Review found four blocking issues:
These should be addressed before merge because the current patch does not typecheck and permits read-only collaborators to mutate or terminate another user’s sandbox. |
Why
The unreleased Machines/"Development" surface made git the information architecture (Machine → Project/repo → Branch). It competed with PageSpace's own filesystem semantics — three parallel trees on screen (drive page tree, repo/branch tree, sandbox POSIX tree) — and needed routing hacks (twin route trees + a keep-alive host) just to stop navigation from killing warm PTYs.
The insight driving the rebuild: the global assistant already worked well as a dev. So the primitive isn't "a dev environment," it's an agent session — any conversation that can lazily acquire a sandbox. That one primitive solves the dev UX and generalizes it: coding agents, data-analysis agents, file-processing agents, anything needing compute plus a filesystem gets the same flow with zero dev ceremony. Git becomes an agent capability, never user topology; dev-specific magic (branch isolation, repo pinning) can layer in later without touching the session model.
What changed
Topology: Drive → Agent (
AI_CHATpage) → Conversation. Each conversation lazily owns one sandbox (Fly Sprite); shells are PTYs inside it. TheMACHINEpage type is gone.Two invariants, documented once in
packages/lib/src/agent-sessions/contract.tsand enforced everywhere:sessionId ≡ conversationId—agent_sessionsuses PK = FK onconversationId. One id addresses the tool call, the URL?c=, chat anchoring, and the Sprite-key fold. There is no session-binding field anywhere; the chat body'sconversationIdis the session address.Surface: new
/dashboard/agents(global + drive-scoped). Selection lives in the URL (?agent=&c=) viapushState, so nothing remounts on switch — the keep-alive host has no successor. One UI everywhere: opening anAI_CHATpage and selecting an agent in the console render the sameAgentView.AiChatView.tsx(1,600 LoC) is deleted.Tools: two verb families, nine tools —
list/spawn/send/read/kill_session+spawn/send/read/kill_shell.spawn_sessionrequires a prompt and dispatches through the standard chat pipeline, so a spawned worker is a normal conversation visible live in the sidebar — never a second engine.ask_agentis gone as a tool (its sync consult is nowwait: true); its internal engine is retained unchanged for the channel-mention responder.move_session,add_session,switch_machine,list_machines, and the headless-run engine are deleted. All 56 git/gh tools kept verbatim — the simplification came from deleting topology, not hardened wrappers.Purity discipline (AIDD: clean seams now, not incrementally): every lifecycle, access, naming, and status decision lives in a pure, exhaustively-tested module. Notably
decideSessionAccessis one function consumed by both the web routes and the realtime bridge — the two tiers structurally cannot diverge. Both tiers also provision through the single sharedensureAgentSessionSandbox, so they can't CAS-fight.Numbers
521 files changed · +26,828 / −82,881 → net −56,053 LoC of source (the +64,841 in the raw diffstat is Drizzle migration meta snapshots, which are generated).
Phases
agent_sessions(PK=conversationId),agent_session_shells, reclaim triggershell:*events addressed by{shellId}; 1,235 tests/api/agent-sessions, nine tools; −4,098 LoCAiChatViewdeleted; page + console convergencelistAgentSessionSprites()replaces a three-table de-fanMACHINEscrubbed repo-wideMigration safety
DROP TABLEdoes not fire per-rowAFTER DELETEtriggers, so dropping the machine tables outright would have stranded live billing VMs forever. Migration0234therefore rescues every live Sprite pointer intomachine_sprite_reclaimsfirst, then deletes MACHINE pages, then drops triggers, tables, and columns. The FK-less outbox is deliberately kept — the orphan cron still drains it. Postgres can'tDROP VALUEfrom an enum, so the deadMACHINEvalue stays in the pg enum (commented) while being removed from the TypeScript enum and every config, validator, and drift guard.The feature was never released, so there is no data migration and no compatibility layer.
Review rounds (post-open)
Two rounds: an automated review plus a full-branch review, then an adversarial re-review of the fixes themselves. All findings are addressed; threads stay open for reviewer verification.
Round 1 — reviewer findings
Security / correctness
x-forwarded-host/hostand forwarded the caller's cookie/CSRF to it. Now uses the configured origin only, restricted to http/https (new URL('localhost:3000')parses with protocollocalhost:and would otherwise slip through). Also fixed a silent break on plain-HTTP deploys.shell:errorwas claimed by every shell on the socket.git_addseparates pathspecs with--;git_reset.ref/git_remote_add.namegained the flag guard their siblings had.Functional / UX — the delegation prompt no longer names a tool absent under the active gates (both
CODE_EXECUTION_ENABLEDand read-only); a failed agent load shows an error + retry instead of an infinite spinner; terminal panes are ARIA-labelled; conversation-creation failures surface.Robustness — orphan reconcile sources no longer take each other down; a doubly-failed teardown (kill and outbox insert) logs loudly instead of silently leaking a billing VM; the dispatch-depth clamp is one tested module; the third messages route uses the repository like its siblings.
Round 2 — adversarial re-review of those fixes
Three of the round-1 fixes were incomplete, which the re-review (and my own verification) caught:
sandboxId !== null, butcountLivealso requiresspriteTornDownAt IS NULL, and teardown leavessandboxIdset. So every ended session claimed the exemption: end N, re-provision N, hold N live sandboxes past the ceiling. The predicate now mirrors the count exactly.createarm, on a VM minted milliseconds earlier — so a session could clone a 3 GB repo and still bill ≈$0. Measurement now also runs where an agent is about to do real work in an awake sandbox, throttled per session.reauthorizeViewerwas never wired, so the fail-closed default meant the viewer identity was never refreshed: revoking the original owner would evict a different member's running work. Now wired to the same check the socket connect uses, and the previously ungated read path is gated identically.Also from round 2: a quota refusal was being delivered as a 403 authorization denial (now a 429 with the real message and a non-authz audit event); over-cap input vanished silently (now refused with a tagged
shell:error); a permanently failing reconcile source was invisible in the result (now reportsincomplete); and thearia-livehalf of the terminal a11y change was inert, since xterm marks its own rowsaria-hidden— removed rather than left as an unearned guarantee.Round 3 — the refusal reported as three different wrong things
The ceiling itself was now enforced correctly, but every surface described it differently, and none of them described it as a plan limit:
Shell access denied: session_limit_reachedsend_shell/read_shell){live: false, delivered: false}, no reasonreasonCould not provision a sandbox for this run.session_limit_reached, its own denial reasoneventType: 'data.read'eventType: 'security.rate.limited'Each was wrong in its own way. "Access denied" sends a user who has every right to that shell hunting a permissions bug that doesn't exist. No reason at all is worse than a wrong one — the agent can't tell a transient miss from a wall it will hit every time, so it retries forever;
startSessionnow returns a typedShellStartRefusal, withundefinedstill meaning "refused with nothing to say" (notably an abandoned request), so existing callers are unchanged. The tool path'sreasonFromAcquirewas a translation seam that had never translated anything;session_limit_reachedis now deliberately distinct fromconcurrency_limit, because that one clears when a sibling run finishes and this one doesn't clear until somebody ends a session — so "wait and retry" is precisely the wrong advice. Anddata.readfiled every free-tier user's "new session" click into data-access forensics for a request that read nothing.The web tool client also stopped guessing: it used to tell the agent the payer "may be out of credits or at the concurrent terminal limit", a disjunction it cannot act on since only one arm is worth waiting out.
Also this round: the store integration test now reads sandbox pointers back out of the DB before deleting sessions, so the FK-less reclaim rows its own
AFTER DELETEtrigger enqueues are actually cleaned up — the file's comment claimed this, but it left five behind, each a name the orphan cron would later try to destroy.Round 4 — the agent shell tools had never worked
Found while auditing round 3's own changes, and the most serious thing in the PR.
Phase 3 renamed the realtime bridge's routes from
/api/session-*to/api/shell-*. The web tool client written in phase 4 kept posting to the old names, so everyread_shellandsend_shell404'd,postSignedmapped the non-2xx tonull, and the agent was told "Could not reach the terminal service" — about a service that was running fine.The read path was wrong three ways, not one:
/api/session-read/api/shell-read{shellId}{shellIds: []}(it also serves the multi-shell liveness sweep){live, output, …}{shells: [...]}, one entry per idA missing entry for the id we named is now "no answer" rather than "not live" — reporting a running build as dead is the one wrong answer this tool must never give.
Why nothing caught it, and what actually got fixed. Each side is unit-tested against a mock of the other, so both suites were green against a hop that could not connect. Patching the path would have left that intact, so the routes and the read/send payload and response shapes now live in
agent-sessions/contract.ts, imported by both apps. That module's rule is that no shape is declared twice, and a string two services must agree on is a shape. It earned its keep immediately: the compiler caught thatpostSignednarrowedstarttotruewhere the wire type allowsboolean.The same drift had quietly killed the agent-activity feed. The realtime handler was re-keyed to
sessionId, fully implemented, tested, and wired with real deps — its own doc says "apps/webposts here after a successful bash run" — but the caller was never rewritten. It still posted a machine-era{tenantId, driveId, pageId}body to/api/terminal-activity, andsandbox-tools-runtimehad stopped supplying the seam at all, so the feature was dead on both ends. Re-keyed end to end and wired. Its gate also moved from the agent page id to the session id, fixing a silent exclusion on top: a global-assistant session has no agent page and so could never have appeared in this feed regardless.Adjacent surfaces audited for the same class of drift and found consistent: the four
shell:*client→server and four server→client socket event names, the shared connect-payload schema, every/api/agent-sessions/**path the frontend fetches, the nine session/shell tool names against their prompt references and registration, and the cron paths (deliberately unchanged, and documented as such).Round 5 — hunting the pattern instead of waiting for it
Rounds 3 and 4 were both "one concept, two implementations." Looking for that deliberately turned up two more.
spawn_session's pre-check disagreed with the gate it pre-checks. Both enforce the same concurrency ceiling; the provisioner callsstore.countLive, the pre-check re-derived it in JS with an extraendedAt === nullclause under a comment claiming identical semantics. That clause undercounts —endedAtis intent,spriteTornDownAtis the VM being gone, and they diverge whenever a kill fails and teardown falls back to the reclaim outbox, while the Sprite is still billing. The pre-check would wave a spawn through that the provisioner then refused. It now callscountLiveinstead of restating it (also turning a full listing plus JS filter into a SQLCOUNT), with the disagreement case pinned in the real-Postgres suite.'starting'was documented but never existed. The backend derivation explains it as a client-side transient "shown while its own ensure request is in flight". Nothing implemented it, so through an entire cold Sprite boot the chip read "No sandbox — one starts the first time the agent needs to run something", false precisely then, before jumping to "Ready". Wiring it surfaced a second layer:useAgentSession.ensureSessionhas no component caller, since provisioning is lazy and there is no explicit start affordance — the cold start a user actually watches is Add shell. The rule now lives in a pureresolveDisplayStatus, and only ever upgrades from'none', so a second shell on a live sandbox never reads as a boot.Checked and sound, no change needed: migration
0234's rescue-before-drop ordering; themachine_branchesrescue predicate (its missingIS NOT NULLguard mirrors that column being.notNull()where its siblings are nullable — an asymmetry worth knowing before someone "fixes" it); theagent_sessionsreclaim trigger, whose live predicate matchescountLive, storage billing and the provisioning exemption — five sites now agreeing; every pure module the plan specified having real non-test consumers, includingdecideAgentSessionAccessgenuinely being the one access decision for both web and realtime; the plan's flagged sidebar-freeze bug being fixed in production code (isAnyEditing()excludes'shell'), not only in its test; and the signed shell endpoints, which are internal-only (Caddy routes just/socket.io/*to realtime), HMAC-signed with a 5-minute window and constant-time comparison, matching the pre-existing/api/broadcastposture.Round 6 — one bug class, searched exhaustively
Rounds 4 and 5 were each an instance of the same thing without my noticing: an optional dependency, declared with a safe default, that nothing ever supplied.
notifyShellActivity(the agent-activity feed) andreauthorizeViewer(the identity re-check) were both found one at a time. So I enumerated every optional dep in the new interfaces and checked each for a production supplier.measureStoragehad zero. Master supplied it; this branch dropped it, and my earlier replacement called measurement fromacquireSandbox— before the op. The seam's own comment explains why that is wrong: it fires fromrelease, after the op, because measuring first records the pre-write footprint and then lets the per-session throttle suppress the real one. An agent writing 5 GB stayed invisible until the throttle lapsed. Removed the pre-op call, supplied the post-op seam, re-keyed it frompageIdtosessionId(a global-assistant session has no page — the same page-shaped assumption that had excluded those sessions from the activity feed), and pinned the ordering with a test, since "measures at all" was true of the broken version too.The same seam was then wired on the git path, which had never measured at all — it has its own acquire/release. That gap is pre-existing rather than a regression, but
git_cloneis the largest writer in the system, and a session that only ran git tools billed its empty-disk baseline forever.Why this class is invisible. An optional dep with a safe default cannot be caught by types (it is legitimately absent), by coverage (the line runs, the branch is just false), or by the feature's own tests (they inject the dep themselves). Only asking "who supplies this in production?" finds it. All three are now pinned by presence assertions, each verified by deleting the wiring and confirming the test fails.
Three stale doc-comments also surfaced, each asserting something the code had stopped doing, each true when written:
sandbox-storage-billingclaiming measurement was "a follow-up, not done here";AiChatViewclaiming it registered'ai-streaming'; anduseEditingStorelisting per-surface registrants. The last one cost the most — I read it, concluded the new chat had dropped a required registration, and reported a non-existent gap on this PR before tracing the actual call path (registration is central, inDerivedStreamingRegistrations). Corrected in the thread. A comment describing the code is the one thing no check on the code can verify.Verification
14cef80a8: typecheck 16/16 · lint 14/14 · knip ratchet 4/4 ·@pagespace/lib8,623 passing ·apps/web14,939 passing across 1,020 files · security suite 1,115 + 767 passing · realtime 934 passing at 98.04% branch coverage. The 16apps/webfailures are the known env-only trio (Postgres-dependent auth suites,activity-tools, TZ-dependentgrouping), each verified identical on this branch without these changesrecoverable-error fix (an over-cap paste bricking the terminal pane) — a real gap, now covered, each new test verified to kill its mutationresumeBillingClock/settle were verified redundant rather than untested: both readers ofconnectedAtguard onpayerIdindependently, andcalculateMachineCostDollarsalready floors at 0. Deliberately left untested — asserting an internal field with no user-visible consequence tests implementation, not behaviour14cef80a8), this repo's CLAUDE.md rules (asyncparams, noany,isOnPrem()gating, no migration SQL edits — all clean), and the new code's own documented invariants (Datenever on the wire, names never addresses, denial ordering). Worth separating from the test results: these find code that is consistently wrong, which no amount of self-consistency checking can reachvi.mockfactory. Both came from trusting targeted local runs; the full suite is now the pre-push check.activity-tools,groupingapi/ai/chat/route.tsare alerts 228/229/230, opened 2026-07-03 againstrefs/heads/master— weeks before this branch — and re-anchored by this PR's large diff to that file; this PR only removes code there. Conversely this PR deletes two files carrying open alerts:machine-workspace/workspace-reducer.ts(fix(security): remediate all 73 CodeQL vulnerability alerts #269, remote-property-injection) andservices/machines/project-session.ts(fix(security): default CSRF origin validation to block mode #266, insufficient-password-hash)0234applied and a working Sprites credential. Written up as a 15-step copy-pasteable checklist in a PR comment (prerequisites, pass condition per step, and what each step proves) so it costs ~10 minutes. It matters more than it did when this PR opened: round 4 found thatread_shell/send_shellhad never once worked, and no static tracing would reveal whether a Sprite actually provisions under real credentials. Step 8 (agent reads a live shell) is the single most important oneDocs: workspace tool count recounted from the registry (77 to 76).
Generated with Claude Code