feat(sandbox): inline open-agents sandbox abstraction (Phase 2.1)#507
Merged
sweetmantech merged 2 commits intotestfrom May 4, 2026
Merged
feat(sandbox): inline open-agents sandbox abstraction (Phase 2.1)#507sweetmantech merged 2 commits intotestfrom
sweetmantech merged 2 commits intotestfrom
Conversation
Phase 2 of the open-agents migration, sub-step 1 of N: drop in the
sandbox abstraction layer from open-agents/packages/sandbox alongside
the existing api lib/sandbox/ helpers. Pure addition — nothing in api
imports the new code yet, so behavior is unchanged.
Files added (13 sources + 2 tests):
lib/sandbox/
abstraction.ts ← was packages/sandbox/index.ts (renamed
to avoid colliding with future lib/sandbox/index.ts)
interface.ts ← Sandbox / SandboxHooks / ExecResult / SnapshotResult
types.ts ← Source / FileEntry / SandboxStatus
factory.ts ← connectSandbox(state, options)
vercel/
index.ts ← barrel re-export
config.ts ← VercelSandboxConfig types
connect.ts ← connectVercel implementation
sandbox.ts ← VercelSandbox class (1192 lines, the meat)
snapshot-refresh.ts ← refreshBaseSnapshot helper
state.ts ← VercelState type
utils.ts ← internal helpers
__tests__/
sandbox.test.ts ← 25 tests (was packages/sandbox/vercel/sandbox.test.ts)
snapshot-refresh.test.ts ← 4 tests
Test framework converted bun:test -> vitest:
- import { ..., mock } from "bun:test" -> import { ..., vi } from "vitest"
- mock.module("foo", factory) -> vi.mock("foo", factory)
- bare mock() -> vi.fn()
- relative paths adjusted for __tests__/ subdir convention
Lint accommodations:
- Added file-level @typescript-eslint/member-ordering disable to
vercel/sandbox.ts (1192 lines); reordering would inflate the diff
with no behavior change. Note in the comment why.
- Reordered SandboxStats interface fields (size/mtimeMs before methods)
to satisfy the rule on the small interface.
Verification:
- New tests: 29/29 pass
- Full test suite: 2391/2391 pass (was 2362, +29 new)
- Lint clean
- No api file imports the new abstraction yet -> zero behavior risk
Up next (separate PR):
- Phase 2.2: refactor api callers to consume the new abstraction
(createSandbox, getActiveSandbox, getOrCreateSandbox, getSandboxStatus,
processCreateSandbox, etc.) — uses connectSandbox() instead of
Sandbox.create / Sandbox.get directly. ~25 caller sites + tests.
- Phase 2.3: delete the now-dead old api lib/sandbox/ helpers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis PR establishes a complete sandbox abstraction system for cloud-based development environments. It defines core TypeScript contracts for sandbox lifecycle and operations, implements Vercel SDK integration with git cloning, timeout orchestration, and GitHub credential brokering, and exports unified factory functions for creating or reconnecting to named persistent sandboxes with flexible fallback strategies. ChangesSandbox Abstraction System
Sequence DiagramsequenceDiagram
actor User
participant connectSandbox as connectSandbox<br/>(factory)
participant connectVercel as connectVercel<br/>(vercel)
participant VercelSandbox as VercelSandbox
participant SDK as `@vercel/sandbox`<br/>SDK
participant Vercel as Vercel<br/>Backend
User->>connectSandbox: connectSandbox({ state, options })
connectSandbox->>connectVercel: connectVercel(state, options)
alt Has sandboxName && !forceCreate
connectVercel->>VercelSandbox: connectNamedSandbox(state, options)
VercelSandbox->>SDK: resume by name
SDK->>Vercel: get sandbox by name
Vercel-->>SDK: session metadata
SDK-->>VercelSandbox: session resumed
rect rgba(200, 150, 100, 0.5)
note over VercelSandbox: Sync GitHub policy,<br/>update timeout from session
end
VercelSandbox-->>connectVercel: VercelSandbox instance
else Create new sandbox
connectVercel->>VercelSandbox: VercelSandbox.create(config)
rect rgba(100, 150, 200, 0.5)
note over VercelSandbox: Init SDK with source<br/>(snapshot/git/base)
end
VercelSandbox->>SDK: create with options
SDK->>Vercel: spawn & configure
Vercel-->>SDK: session + workspace
SDK-->>VercelSandbox: session ready
VercelSandbox->>VercelSandbox: scheduleProactiveStop()
rect rgba(150, 150, 150, 0.5)
note over VercelSandbox: Timer will invoke<br/>hooks.onTimeout before SDK timeout
end
alt source provided && not prebuilt
VercelSandbox->>SDK: clone repo
SDK->>Vercel: git clone
end
VercelSandbox->>VercelSandbox: hooks.afterStart()
VercelSandbox-->>connectVercel: VercelSandbox instance
end
connectVercel-->>connectSandbox: Sandbox (via VercelSandbox)
connectSandbox-->>User: Sandbox ready for exec/readFile/etc
sequenceDiagram
participant Sandbox as VercelSandbox
participant Timer as Proactive<br/>Timeout Timer
participant SDK as SDK<br/>Session
participant Hooks as Hooks<br/>Config
Sandbox->>Sandbox: create() or connect()
Sandbox->>Sandbox: scheduleProactiveStop()
Sandbox->>Timer: setTimeout(onTimeout at T - buffer)
rect rgba(200, 100, 100, 0.5)
note over Timer,Hooks: Proactive window before SDK timeout
end
loop User executes commands
Sandbox->>SDK: exec(), readFile(), etc
SDK-->>Sandbox: results
end
alt Extend timeout before expiry
Sandbox->>Sandbox: extendTimeout(additionalMs)
Sandbox->>SDK: session.extendTimeout()
SDK-->>Sandbox: new expiresAt
Sandbox->>Sandbox: rescheduleProactiveStop()
Sandbox->>Timer: clear old, setTimeout new
Sandbox->>Hooks: onTimeoutExtended(sandbox, additionalMs)
end
alt Proactive timer fires
Timer->>Hooks: onTimeout(sandbox)
Hooks-->>Sandbox: async hook completes
note over Sandbox: Sandbox still running, SDK continues
end
alt User stops explicitly
Sandbox->>Hooks: beforeStop()
Sandbox->>Timer: clearTimeout()
Sandbox->>SDK: session.stop()
SDK-->>Sandbox: stopped
else Proactive + SDK timeout both expire
SDK->>Sandbox: (forced session termination)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 41 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
lib/sandbox/vercel/sandbox.ts (2)
818-823: 💤 Low valueMisleading variable name:
stderris assigned fromstdout().While the comment explains that the SDK combines output, the variable name
stderrreceivingstdout()could confuse future maintainers.Suggested naming clarification
if (result.exitCode !== 0) { - const stderr = await result.stdout(); // stdout contains error in some cases - if (!stderr.includes("File exists") || !options?.recursive) { + const output = await result.stdout(); // SDK combines stdout/stderr + if (!output.includes("File exists") || !options?.recursive) { throw new Error(`Failed to create directory: ${path}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandbox/vercel/sandbox.ts` around lines 818 - 823, The variable named `stderr` is misleading because it is assigned from `result.stdout()`; rename it to something neutral like `output` or `combinedOutput` in the block that checks `result.exitCode` (the local variables `result`, `options?.recursive`, and `path` are the relevant symbols) and update the subsequent check to use that new name; also update the inline comment to explain that stdout may contain error text due to SDK behavior so maintainers understand why `stdout()` is used instead of `stderr()`.
460-684: ⚖️ Poor tradeoffConsider extracting the
create()method into smaller, focused helper functions.At ~224 lines, the
create()static method handles multiple responsibilities: config extraction, SDK instantiation, git clone (two paths: standard and prebuilt), git initialization, remote URL configuration, user config, branch creation, and instance construction.Breaking this into smaller helpers would improve testability and readability:
createSdkInstance(config)- SDK instantiation logicsetupGitRepository(sdk, source, baseSnapshotId)- git clone/init operationsconfigureGitCredentials(sdk, source)- remote URL and user configThis is a recommended improvement rather than a blocker, especially since the code was inlined from an existing package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandbox/vercel/sandbox.ts` around lines 460 - 684, The create() method in VercelSandbox is too large and handles distinct tasks; refactor by extracting focused helpers: move the SDK instantiation branch logic into createSdkInstance(config) (references: VercelSandbox.create and VercelSandboxSDK.create calls), move cloning/fetch/reset/init/checkout logic into setupGitRepository(sdk, source, baseSnapshotId, workingDirectory) (references: git clone, fetch, reset, git init, initial commit, newBranch checkout), and move token/remote URL and user config steps into configureGitCredentials(sdk, source, gitUser, skipGitWorkspaceBootstrap) (references: buildAuthenticatedGitHubUrl, git remote set-url, git config user.name/user.email); wire these helpers into create() so behavior remains identical and add small unit tests for each helper to improve readability and testability.lib/sandbox/factory.ts (1)
56-59: ⚡ Quick win
SandboxConnectConfig.stateduplicatesSandboxState— use the already-defined alias.Line 57 re-inlines
{ type: "vercel" } & VercelState, which is exactly whatSandboxState(line 13) already encodes. If the intersection is ever updated, line 57 will silently drift.♻️ Proposed fix
export type SandboxConnectConfig = { - state: { type: "vercel" } & VercelState; + state: SandboxState; options?: ConnectOptions; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandbox/factory.ts` around lines 56 - 59, The type alias SandboxConnectConfig currently re-inlines the state shape as `{ type: "vercel" } & VercelState` which duplicates the existing SandboxState type; replace the inline intersection in SandboxConnectConfig.state with the SandboxState alias so the code uses the single source of truth (update the SandboxConnectConfig declaration to reference SandboxState and keep options?: ConnectOptions unchanged) to prevent future drift between VercelState/SandboxState and the connect config.lib/sandbox/vercel/snapshot-refresh.ts (1)
76-155: ⚖️ Poor tradeoff
refreshBaseSnapshotis ~80 lines — extract sub-steps to respect the 50-line function guideline.Three natural seams make the extraction straightforward:
Helper Lines Responsibility createAndValidateSandbox(options, connectFn)88–113 Connect + guard snapshot capability executeCommands(sandbox, commands, timeout, log)115–133 Iterate, exec, collect results (inline) 135–144 Call snapshot()+ build return valueAfter extraction
refreshBaseSnapshotbecomes a lean orchestrator well under 30 lines, which also makes each step independently testable.As per coding guidelines: "Keep functions under 50 lines" and "Flag functions longer than 20 lines."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandbox/vercel/snapshot-refresh.ts` around lines 76 - 155, refreshBaseSnapshot is too long; extract the sandbox creation/validation and command execution into two helpers to keep the main function under 50 lines: implement createAndValidateSandbox(options, connectSnapshotSandbox) which performs the connectSnapshotSandbox(...) call, asserts sandbox.snapshot exists (throwing the same error message), and returns the connected SnapshotSandbox; and implement executeCommands(sandbox, commands, commandTimeoutMs, log) which runs the loop currently inside refreshBaseSnapshot, returns an array of RefreshBaseSnapshotCommandResult, and rethrows formatted failures (use the existing formatCommandFailure). Replace the inline logic in refreshBaseSnapshot with calls to createAndValidateSandbox and executeCommands, then call sandbox.snapshot(), set snapshotCreated, and return the same object shape ({ sourceSnapshotId, snapshotId, commandResults }); keep the existing finally block that stops the sandbox when snapshotCreated is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sandbox/factory.ts`:
- Around line 64-81: The file name must match the primary export: move the
exported function connectSandbox into a new file named connectSandbox.ts and
update imports/exports accordingly; also relocate or co-locate the related types
SandboxConnectConfig, SandboxState, ConnectOptions (and the re-exported
SandboxStatus) into a types.ts next to connectSandbox.ts or merge them into the
new file, and ensure the internal call to connectVercel still imports from its
original module; update any modules that import connectSandbox or those types to
point to the new files.
In `@lib/sandbox/vercel/snapshot-refresh.ts`:
- Around line 76-79: The file name snapshot-refresh.ts does not match its
primary exported function refreshBaseSnapshot; rename the file to
refresh-base-snapshot.ts (kebab-case matching refreshBaseSnapshot) or create a
new file named refresh-base-snapshot.ts and move the refreshBaseSnapshot export
and its related types/helpers there so the file name matches the exported
function.
In `@lib/sandbox/vercel/utils.ts`:
- Around line 1-13: Rename the file from utils.ts to configureGitUser.ts so the
filename matches the exported function configureGitUser; update any imports that
reference "./utils" (or the containing path) to the new "./configureGitUser"
name and ensure the exported function configureGitUser and type VercelSandbox
usage remain unchanged.
- Around line 10-12: The git config invocations embed gitUser.name and
gitUser.email directly into shell strings, risking command injection; update the
calls that use sandbox.exec so they pass arguments safely (avoid shell
interpolation) or otherwise escape/quote values robustly—e.g., use an exec API
overload that accepts an argv array or set the values via environment variables
(GIT_AUTHOR_NAME/GIT_AUTHOR_EMAIL) before running git; change the two calls
referencing gitUser.name and gitUser.email to the safer invocation form and
ensure sandbox.exec is used without executing an interpolated shell command.
---
Nitpick comments:
In `@lib/sandbox/factory.ts`:
- Around line 56-59: The type alias SandboxConnectConfig currently re-inlines
the state shape as `{ type: "vercel" } & VercelState` which duplicates the
existing SandboxState type; replace the inline intersection in
SandboxConnectConfig.state with the SandboxState alias so the code uses the
single source of truth (update the SandboxConnectConfig declaration to reference
SandboxState and keep options?: ConnectOptions unchanged) to prevent future
drift between VercelState/SandboxState and the connect config.
In `@lib/sandbox/vercel/sandbox.ts`:
- Around line 818-823: The variable named `stderr` is misleading because it is
assigned from `result.stdout()`; rename it to something neutral like `output` or
`combinedOutput` in the block that checks `result.exitCode` (the local variables
`result`, `options?.recursive`, and `path` are the relevant symbols) and update
the subsequent check to use that new name; also update the inline comment to
explain that stdout may contain error text due to SDK behavior so maintainers
understand why `stdout()` is used instead of `stderr()`.
- Around line 460-684: The create() method in VercelSandbox is too large and
handles distinct tasks; refactor by extracting focused helpers: move the SDK
instantiation branch logic into createSdkInstance(config) (references:
VercelSandbox.create and VercelSandboxSDK.create calls), move
cloning/fetch/reset/init/checkout logic into setupGitRepository(sdk, source,
baseSnapshotId, workingDirectory) (references: git clone, fetch, reset, git
init, initial commit, newBranch checkout), and move token/remote URL and user
config steps into configureGitCredentials(sdk, source, gitUser,
skipGitWorkspaceBootstrap) (references: buildAuthenticatedGitHubUrl, git remote
set-url, git config user.name/user.email); wire these helpers into create() so
behavior remains identical and add small unit tests for each helper to improve
readability and testability.
In `@lib/sandbox/vercel/snapshot-refresh.ts`:
- Around line 76-155: refreshBaseSnapshot is too long; extract the sandbox
creation/validation and command execution into two helpers to keep the main
function under 50 lines: implement createAndValidateSandbox(options,
connectSnapshotSandbox) which performs the connectSnapshotSandbox(...) call,
asserts sandbox.snapshot exists (throwing the same error message), and returns
the connected SnapshotSandbox; and implement executeCommands(sandbox, commands,
commandTimeoutMs, log) which runs the loop currently inside refreshBaseSnapshot,
returns an array of RefreshBaseSnapshotCommandResult, and rethrows formatted
failures (use the existing formatCommandFailure). Replace the inline logic in
refreshBaseSnapshot with calls to createAndValidateSandbox and executeCommands,
then call sandbox.snapshot(), set snapshotCreated, and return the same object
shape ({ sourceSnapshotId, snapshotId, commandResults }); keep the existing
finally block that stops the sandbox when snapshotCreated is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 462af2fe-f9a1-4b93-9d74-29343e705cd6
⛔ Files ignored due to path filters (2)
lib/sandbox/vercel/__tests__/sandbox.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/sandbox/vercel/__tests__/snapshot-refresh.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (11)
lib/sandbox/abstraction.tslib/sandbox/factory.tslib/sandbox/interface.tslib/sandbox/types.tslib/sandbox/vercel/config.tslib/sandbox/vercel/connect.tslib/sandbox/vercel/index.tslib/sandbox/vercel/sandbox.tslib/sandbox/vercel/snapshot-refresh.tslib/sandbox/vercel/state.tslib/sandbox/vercel/utils.ts
Comment on lines
+64
to
+81
| export async function connectSandbox( | ||
| configOrState: SandboxConnectConfig | SandboxState, | ||
| legacyOptions?: ConnectOptions, | ||
| ): Promise<Sandbox> { | ||
| const isNewApi = | ||
| typeof configOrState === "object" && | ||
| "state" in configOrState && | ||
| typeof configOrState.state === "object" && | ||
| "type" in configOrState.state; | ||
|
|
||
| if (isNewApi) { | ||
| const config = configOrState as SandboxConnectConfig; | ||
| return connectVercel(config.state, config.options); | ||
| } | ||
|
|
||
| const state = configOrState as SandboxState; | ||
| return connectVercel(state, legacyOptions); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
File name factory.ts doesn't match the exported function connectSandbox.
Per coding guidelines, the file name must match the primary exported function. connectSandbox should live in connectSandbox.ts. Supporting types (SandboxState, ConnectOptions, SandboxConnectConfig) and the SandboxStatus re-export can move to a co-located types.ts or be merged into the renamed file alongside the function — the important thing is the file→function name alignment.
As per coding guidelines: "File naming rule: The file name MUST match the exported function name. If a new function is defined in a file whose name does not match, leave a review comment telling the developer to create a new file named after the function."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandbox/factory.ts` around lines 64 - 81, The file name must match the
primary export: move the exported function connectSandbox into a new file named
connectSandbox.ts and update imports/exports accordingly; also relocate or
co-locate the related types SandboxConnectConfig, SandboxState, ConnectOptions
(and the re-exported SandboxStatus) into a types.ts next to connectSandbox.ts or
merge them into the new file, and ensure the internal call to connectVercel
still imports from its original module; update any modules that import
connectSandbox or those types to point to the new files.
Comment on lines
+76
to
+79
| export async function refreshBaseSnapshot( | ||
| options: RefreshBaseSnapshotOptions, | ||
| dependencies: RefreshBaseSnapshotDependencies = {}, | ||
| ): Promise<RefreshBaseSnapshotResult> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
File name snapshot-refresh.ts doesn't match the primary exported function refreshBaseSnapshot.
Even in kebab-case the expected name is refresh-base-snapshot.ts, not snapshot-refresh.ts. The word order is transposed, which will make discoverability harder as the codebase grows.
As per coding guidelines: "File naming rule: The file name MUST match the exported function name. If a new function is defined in a file whose name does not match, leave a review comment telling the developer to create a new file named after the function."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandbox/vercel/snapshot-refresh.ts` around lines 76 - 79, The file name
snapshot-refresh.ts does not match its primary exported function
refreshBaseSnapshot; rename the file to refresh-base-snapshot.ts (kebab-case
matching refreshBaseSnapshot) or create a new file named
refresh-base-snapshot.ts and move the refreshBaseSnapshot export and its related
types/helpers there so the file name matches the exported function.
Comment on lines
+1
to
+13
| import type { VercelSandbox } from "./sandbox"; | ||
|
|
||
| /** | ||
| * Configure git user identity on a sandbox. | ||
| * Used after restoring from snapshot since git config isn't preserved. | ||
| */ | ||
| export async function configureGitUser( | ||
| sandbox: VercelSandbox, | ||
| gitUser: { name: string; email: string }, | ||
| ): Promise<void> { | ||
| await sandbox.exec(`git config user.name "${gitUser.name}"`, sandbox.workingDirectory, 10_000); | ||
| await sandbox.exec(`git config user.email "${gitUser.email}"`, sandbox.workingDirectory, 10_000); | ||
| } |
There was a problem hiding this comment.
File naming guideline violation: rename utils.ts to configureGitUser.ts.
Per coding guidelines, the file name must match the exported function name. This file exports configureGitUser but is named utils.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandbox/vercel/utils.ts` around lines 1 - 13, Rename the file from
utils.ts to configureGitUser.ts so the filename matches the exported function
configureGitUser; update any imports that reference "./utils" (or the containing
path) to the new "./configureGitUser" name and ensure the exported function
configureGitUser and type VercelSandbox usage remain unchanged.
Comment on lines
+10
to
+12
| ): Promise<void> { | ||
| await sandbox.exec(`git config user.name "${gitUser.name}"`, sandbox.workingDirectory, 10_000); | ||
| await sandbox.exec(`git config user.email "${gitUser.email}"`, sandbox.workingDirectory, 10_000); |
There was a problem hiding this comment.
Potential command injection if gitUser values contain shell metacharacters.
The name and email values are embedded directly in the shell command string with only double-quote wrapping. If these values contain characters like ", $(), or backticks, command injection could occur.
While gitUser typically comes from trusted configuration rather than user input, consider using a safer approach for defense in depth:
Suggested safer approach using environment variables
export async function configureGitUser(
sandbox: VercelSandbox,
gitUser: { name: string; email: string },
): Promise<void> {
- await sandbox.exec(`git config user.name "${gitUser.name}"`, sandbox.workingDirectory, 10_000);
- await sandbox.exec(`git config user.email "${gitUser.email}"`, sandbox.workingDirectory, 10_000);
+ await sandbox.exec(
+ `git config user.name "$GIT_USER_NAME"`,
+ sandbox.workingDirectory,
+ 10_000,
+ { env: { GIT_USER_NAME: gitUser.name } },
+ );
+ await sandbox.exec(
+ `git config user.email "$GIT_USER_EMAIL"`,
+ sandbox.workingDirectory,
+ 10_000,
+ { env: { GIT_USER_EMAIL: gitUser.email } },
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandbox/vercel/utils.ts` around lines 10 - 12, The git config invocations
embed gitUser.name and gitUser.email directly into shell strings, risking
command injection; update the calls that use sandbox.exec so they pass arguments
safely (avoid shell interpolation) or otherwise escape/quote values
robustly—e.g., use an exec API overload that accepts an argv array or set the
values via environment variables (GIT_AUTHOR_NAME/GIT_AUTHOR_EMAIL) before
running git; change the two calls referencing gitUser.name and gitUser.email to
the safer invocation form and ensure sandbox.exec is used without executing an
interpolated shell command.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 13 files
Confidence score: 2/5
- There is a concrete high-impact risk in
lib/sandbox/vercel/utils.ts: interpolatinggitUserintosandbox.execwithout shell escaping can enable command injection or malformed git config commands. - Test reliability is currently weakened in
lib/sandbox/vercel/__tests__/sandbox.test.tsbecause unawaited.rejectsassertions can produce false-positive passing tests, increasing regression risk. - Several maintainability findings (very large modules/tests and export-name mismatches in
lib/sandbox/vercel/sandbox.ts,lib/sandbox/vercel/config.ts,lib/sandbox/factory.ts, and related files) are likely non-immediate breakages but raise change-risk and review complexity. - Pay close attention to
lib/sandbox/vercel/utils.tsandlib/sandbox/vercel/__tests__/sandbox.test.ts- security-sensitive command construction and potentially ineffective negative-path tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/sandbox/factory.ts">
<violation number="1" location="lib/sandbox/factory.ts:64">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Primary exported function `connectSandbox` does not match filename `factory.ts`</violation>
</file>
<file name="lib/sandbox/vercel/config.ts">
<violation number="1" location="lib/sandbox/vercel/config.ts:3">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
File exceeds the 100-line limit defined in Rule 3 (2647eeb5-3ebd-49a0-9738-8d3ddae48a42). The file is 123 lines, 23 lines over the required maximum. It also combines two distinct config interfaces (`VercelSandboxConfig` and `VercelSandboxConnectConfig`) with verbose JSDoc, which could be split into separate focused modules under 100 lines each.</violation>
</file>
<file name="lib/sandbox/vercel/sandbox.ts">
<violation number="1" location="lib/sandbox/vercel/sandbox.ts:1">
P1: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
New file exceeds the 100-line maximum by more than 11× (1127 lines). The module contains a large class, many helper functions, and interfaces all in one place, violating the rule to limit files to under 100 lines for readability and single-responsibility.</violation>
<violation number="2" location="lib/sandbox/vercel/sandbox.ts:351">
P3: Dead code: the fallback `if (!candidatePorts.includes(80))` is always false because `getCandidatePorts()` unconditionally appends `80` to the set. This block can never execute.</violation>
<violation number="3" location="lib/sandbox/vercel/sandbox.ts:1107">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
File exports multiple top-level items (`VercelSandbox` class and `connectVercelSandbox` function) and the sole exported function name `connectVercelSandbox` does not match the file basename `sandbox`, violating the filename/export consistency rule.</violation>
</file>
<file name="lib/sandbox/vercel/utils.ts">
<violation number="1" location="lib/sandbox/vercel/utils.ts:7">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Primary exported function name (`configureGitUser`) does not match file basename (`utils`) in a single-export module.</violation>
<violation number="2" location="lib/sandbox/vercel/utils.ts:11">
P1: Escape shell arguments before interpolating `gitUser` values into `sandbox.exec` commands to prevent command injection and malformed git config commands.</violation>
</file>
<file name="lib/sandbox/vercel/__tests__/snapshot-refresh.test.ts">
<violation number="1" location="lib/sandbox/vercel/__tests__/snapshot-refresh.test.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
New test file exceeds the 100-line maintainability limit.</violation>
</file>
<file name="lib/sandbox/vercel/snapshot-refresh.ts">
<violation number="1" location="lib/sandbox/vercel/snapshot-refresh.ts:76">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Primary exported function name does not match filename</violation>
</file>
<file name="lib/sandbox/vercel/__tests__/sandbox.test.ts">
<violation number="1" location="lib/sandbox/vercel/__tests__/sandbox.test.ts:1">
P1: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
Test file exceeds the 100-line limit by a wide margin and bundles many unrelated concerns into one module.</violation>
<violation number="2" location="lib/sandbox/vercel/__tests__/sandbox.test.ts:564">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
Unawaited `.rejects` assertions make these negative-path tests false positives.</violation>
<violation number="3" location="lib/sandbox/vercel/__tests__/sandbox.test.ts:583">
P2: Await this `.rejects` assertion so the test actually waits for the rejection.</violation>
<violation number="4" location="lib/sandbox/vercel/__tests__/sandbox.test.ts:611">
P2: Await this `.rejects` assertion so the test actually waits for the rejection.</violation>
</file>
Architecture diagram
sequenceDiagram
participant App as Application Logic
participant Factory as Sandbox Factory
participant VSB as VercelSandbox (Abstraction)
participant SDK as Vercel SDK (@vercel/sandbox)
participant Cloud as Vercel MicroVM
participant GH as GitHub API
Note over App, GH: NEW: Unified Sandbox Connection Flow (Phase 2.1)
App->>Factory: connectSandbox(state, options)
alt Reconnect Flow (Persistent Sandbox)
Factory->>VSB: connect(sandboxName)
VSB->>SDK: get(name)
SDK-->>VSB: Existing Session
else Fresh Creation Flow
Factory->>VSB: create(config)
VSB->>SDK: create(params)
SDK->>Cloud: Provision MicroVM
SDK-->>VSB: New Session
end
Note over VSB, GH: NEW: GitHub Credential Brokering
VSB->>VSB: buildGitHubCredentialBrokeringPolicy(token)
VSB->>SDK: updateNetworkPolicy(Authorization Headers)
SDK->>Cloud: Apply Proxy Rules
opt If Git Source Provided
VSB->>Cloud: NEW: git clone/fetch via brokered auth
Cloud->>GH: Authenticated Git Request
GH-->>Cloud: Repo Data
end
Note over App, Cloud: Command Execution & Environment
App->>VSB: exec(command, cwd, timeout)
VSB->>VSB: NEW: Inject SANDBOX_HOST & Port URLs
VSB->>SDK: runCommand(cmd, env)
SDK->>Cloud: Execute in VM
Cloud-->>VSB: ExecResult (stdout/stderr)
VSB-->>App: ExecResult
Note over App, Cloud: Lifecycle Management
opt Snapshotting
App->>VSB: snapshot()
VSB->>SDK: snapshot()
SDK-->>VSB: snapshotId
VSB-->>App: SnapshotResult
end
App->>VSB: stop()
VSB->>VSB: Run hooks.beforeStop()
VSB->>SDK: stop()
SDK->>Cloud: Terminate/Suspend
VSB-->>App: void
alt Failure Path: VM Timeout
Cloud->>VSB: SDK Timeout Event
VSB->>VSB: NEW: Run hooks.onTimeout()
VSB->>VSB: Run hooks.beforeStop()
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,684 @@ | |||
| import { beforeAll, beforeEach, describe, expect, vi, test } from "vitest"; | |||
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
Unawaited .rejects assertions make these negative-path tests false positives.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/__tests__/sandbox.test.ts, line 564:
<comment>Unawaited `.rejects` assertions make these negative-path tests false positives.</comment>
<file context>
@@ -0,0 +1,684 @@
+ remainingTimeout: 0,
+ });
+
+ expect(sandbox.execDetached("bun run dev", "/vercel/sandbox")).rejects.toThrow("wait failed");
+ });
+
</file context>
| @@ -0,0 +1,684 @@ | |||
| import { beforeAll, beforeEach, describe, expect, vi, test } from "vitest"; | |||
There was a problem hiding this comment.
P1: Custom agent: Enforce Clear Code Style and Maintainability Practices
Test file exceeds the 100-line limit by a wide margin and bundles many unrelated concerns into one module.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/__tests__/sandbox.test.ts, line 1:
<comment>Test file exceeds the 100-line limit by a wide margin and bundles many unrelated concerns into one module.</comment>
<file context>
@@ -0,0 +1,684 @@
+import { beforeAll, beforeEach, describe, expect, vi, test } from "vitest";
+
+const portDomains = new Map<number, string>();
</file context>
| sandbox: VercelSandbox, | ||
| gitUser: { name: string; email: string }, | ||
| ): Promise<void> { | ||
| await sandbox.exec(`git config user.name "${gitUser.name}"`, sandbox.workingDirectory, 10_000); |
There was a problem hiding this comment.
P1: Escape shell arguments before interpolating gitUser values into sandbox.exec commands to prevent command injection and malformed git config commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/utils.ts, line 11:
<comment>Escape shell arguments before interpolating `gitUser` values into `sandbox.exec` commands to prevent command injection and malformed git config commands.</comment>
<file context>
@@ -0,0 +1,13 @@
+ sandbox: VercelSandbox,
+ gitUser: { name: string; email: string },
+): Promise<void> {
+ await sandbox.exec(`git config user.name "${gitUser.name}"`, sandbox.workingDirectory, 10_000);
+ await sandbox.exec(`git config user.email "${gitUser.email}"`, sandbox.workingDirectory, 10_000);
+}
</file context>
| /** | ||
| * Connect to a sandbox based on the provided configuration. | ||
| */ | ||
| export async function connectSandbox( |
There was a problem hiding this comment.
P2: Custom agent: Module should export a single primary function whose name matches the filename
Primary exported function connectSandbox does not match filename factory.ts
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/factory.ts, line 64:
<comment>Primary exported function `connectSandbox` does not match filename `factory.ts`</comment>
<file context>
@@ -0,0 +1,81 @@
+/**
+ * Connect to a sandbox based on the provided configuration.
+ */
+export async function connectSandbox(
+ configOrState: SandboxConnectConfig | SandboxState,
+ legacyOptions?: ConnectOptions,
</file context>
| @@ -0,0 +1,218 @@ | |||
| import { describe, expect, vi, test } from "vitest"; | |||
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
New test file exceeds the 100-line maintainability limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/__tests__/snapshot-refresh.test.ts, line 1:
<comment>New test file exceeds the 100-line maintainability limit.</comment>
<file context>
@@ -0,0 +1,218 @@
+import { describe, expect, vi, test } from "vitest";
+import type { SandboxConnectConfig } from "../../factory";
+import type { ExecResult } from "../../interface";
</file context>
Comment on lines
+611
to
+613
| expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow( | ||
| "Failed to read file", | ||
| ); |
There was a problem hiding this comment.
P2: Await this .rejects assertion so the test actually waits for the rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/__tests__/sandbox.test.ts, line 611:
<comment>Await this `.rejects` assertion so the test actually waits for the rejection.</comment>
<file context>
@@ -0,0 +1,684 @@
+ remainingTimeout: 0,
+ });
+
+ expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow(
+ "Failed to read file",
+ );
</file context>
Suggested change
| expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow( | |
| "Failed to read file", | |
| ); | |
| await expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow( | |
| "Failed to read file", | |
| ); |
Comment on lines
+583
to
+585
| expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow( | ||
| "npm ERR! code ENOENT", | ||
| ); |
There was a problem hiding this comment.
P2: Await this .rejects assertion so the test actually waits for the rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/__tests__/sandbox.test.ts, line 583:
<comment>Await this `.rejects` assertion so the test actually waits for the rejection.</comment>
<file context>
@@ -0,0 +1,684 @@
+ remainingTimeout: 0,
+ });
+
+ expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow(
+ "npm ERR! code ENOENT",
+ );
</file context>
Suggested change
| expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow( | |
| "npm ERR! code ENOENT", | |
| ); | |
| await expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow( | |
| "npm ERR! code ENOENT", | |
| ); |
| } | ||
|
|
||
| // Fallback for cases where no ports were declared but default HTTP route exists. | ||
| if (!candidatePorts.includes(80)) { |
There was a problem hiding this comment.
P3: Dead code: the fallback if (!candidatePorts.includes(80)) is always false because getCandidatePorts() unconditionally appends 80 to the set. This block can never execute.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/sandbox.ts, line 351:
<comment>Dead code: the fallback `if (!candidatePorts.includes(80))` is always false because `getCandidatePorts()` unconditionally appends `80` to the set. This block can never execute.</comment>
<file context>
@@ -0,0 +1,1127 @@
+ }
+
+ // Fallback for cases where no ports were declared but default HTTP route exists.
+ if (!candidatePorts.includes(80)) {
+ try {
+ const domainUrl = this.sdk.domain(80);
</file context>
sweetmantech
commented
May 4, 2026
Contributor
Author
There was a problem hiding this comment.
SRP
- actual: multiple functions defined in this file.
- required: one file per function at lib/sandbox/vercel/connect/[lib].ts
Contributor
Author
There was a problem hiding this comment.
SRP
- actual: multiple functions defined in this file.
- required: one file per function at lib/sandbox/vercel/sandbox/[lib].ts
Contributor
Author
There was a problem hiding this comment.
SRP
- actual: multiple functions defined in this file.
- required: one file per function at lib/sandbox/vercel/snapshot-refresh/[lib].ts
Addresses 4 review comments on PR #507. Each file with multiple function definitions becomes a directory with one file per function. Splits: utils.ts -> configureGitUser.ts (rename — utils.ts is a banned dumping-ground name in this repo) connect.ts -> connect/*.ts (8 files) connectVercel.ts (the public entry) connectNamedSandbox.ts buildCreateConfig.ts getSandboxName.ts getRemainingTimeout.ts isSandboxNotFoundError.ts toErrorMessage.ts types.ts (ConnectOptions interface) snapshot-refresh.ts -> snapshot-refresh/*.ts (5 files) refreshBaseSnapshot.ts (the public entry + DEFAULT timeout const) defaultConnectSnapshotSandbox.ts formatCommandFailure.ts formatCommandOutput.ts types.ts (4 interfaces) sandbox.ts -> sandbox/*.ts (9 files) VercelSandbox.ts (the class — methods can't be split, the class is one entity) connectVercelSandbox.ts (factory function) buildGitHubCredentialBrokeringPolicy.ts syncGitHubCredentialBrokering.ts buildAuthenticatedGitHubUrl.ts isStoppedSessionStatus.ts getRemainingTimeoutFromSession.ts constants.ts (timeout/output limits + DEFAULT_NETWORK_POLICY) types.ts (network-policy interfaces + VercelSandboxSession) Caller updates: - lib/sandbox/vercel/index.ts barrel: paths point at new locations - lib/sandbox/factory.ts: connectVercel import path - lib/sandbox/vercel/configureGitUser.ts: VercelSandbox import path - lib/sandbox/vercel/__tests__/*.test.ts: paths re-rooted Verification: - pnpm lint:check: clean - pnpm test: 2391/2391 pass (no behavior change — pure restructure) The class file kept its file-level @typescript-eslint/member-ordering disable comment since the class itself is still the unit of code (its methods cannot be split across files without breaking the OOP design). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 4, 2026
sweetmantech
added a commit
that referenced
this pull request
May 7, 2026
…open-agents (#522) * feat(sandbox): port POST /api/sandbox + GET /api/sandbox/status from open-agents Implements the two session-scoped sandbox endpoints required to drive the chat "loading sandbox..." UX on session entry — matching the contract documented in recoupable/docs#192 (now merged on main). POST /api/sandbox provisions or resumes a Sandbox via the abstraction inlined in #507. When sessionId is supplied, the deterministic sandboxName ensures resume idempotency and the resolved sandbox state is persisted onto the session row (sandbox_state, lifecycle_state = "active", lifecycle_version bumped, sandbox_expires_at, last_activity_at) so subsequent GET /api/sandbox/status calls report the sandbox as active. GET /api/sandbox/status is DB-only — reads the session row, computes status as "active" when sandbox_state is set and not expired (10s buffer to match open-agents), otherwise "no_sandbox". hasSnapshot is true when snapshot_url is set. Mirrors the lifecycle envelope shape from open-agents so the frontend cutover is byte-identical. Files follow existing api conventions: - Route shells in app/api/sandbox/ delegate to handlers in lib/sandbox/ - Auth via validateAuthContext (Privy Bearer or x-api-key) - Validation via Zod (validateCreateSandboxBody) - Supabase ops in lib/supabase/sessions/ (one fn per file) - Error envelope { status: "error", error } matches sessions PRs TDD red → green: - 7 new test files added covering validator, helper, Supabase wrapper, both handlers, and the two route shells - 30 new tests, all passing (was 2461, now 2491) - pnpm lint:check clean Out of scope (deferred to follow-up PRs): - Org-snapshot lookup / kickBuildOrgSnapshotWorkflow (cold-start opt) - Skill installation (installSessionGlobalSkills) - Lifecycle workflow kick (no workflow infra in api yet) - DELETE /api/sandbox + PUT /api/sandbox/snapshot (no UI callers identified during the open-agents grep audit) - /api/sandbox/{extend,activity,reconnect,snapshot} sub-routes — to follow once these two land and the chat UX is validated Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sandbox): treat type-stub sandbox_state as no_sandbox in /status Smoke test against the preview deployment caught a regression that defeated the entire loading-state UX this PR exists to enable: GET /api/sandbox/status reported `"active"` immediately after POST /api/sessions, before any sandbox had been provisioned. Root cause: POST /api/sessions (PR #515) inserts `sandbox_state` as the type stub `{ type: "vercel" }`. The previous `isSandboxActive` check `if (!row.sandbox_state) return false` saw a truthy object and fell through; with `sandbox_expires_at = null` (no expiry yet), the function returned true. Fix: introduce `hasRuntimeSandboxState(state)` that distinguishes the type stub from real runtime metadata by requiring a non-empty `sandboxName` (set by `getSessionSandboxName(sessionId)` in POST /api/sandbox and preserved by the abstraction's `getState()`). Mirrors open-agents' equivalent helper. TDD red → green: - Regression test pinned to the exact production scenario (sandbox_state = {type:"vercel"}, sandbox_expires_at = null, lifecycle_state = "provisioning") asserting status=no_sandbox - Companion test asserting status=active once sandboxName is set - 6 unit tests for the new helper covering null/undefined, scalars, type stub, populated state, and empty-string sandboxName edge case - Confirmed RED before implementing, GREEN after - Suite: 2491 → 2499 (+8 new tests), pnpm lint:check clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(sandbox): SRP/KISS extractions + Tier 1 correctness fixes Addresses review feedback on PR #522 and the "missing from open-agents" audit: User-flagged review comments: - SRP: extract `buildSource` to lib/sandbox/buildSource.ts - YAGNI: drop `isNewBranch` from POST /api/sandbox — chat never sets it (note: docs PR #192 still documents it; will open follow-up docs PR to drop from sandbox.json) - SRP: extract `isoToEpochMs` to lib/sandbox/isoToEpochMs.ts - SRP: extract `buildLifecycle` to lib/sandbox/buildLifecycle.ts - SRP: extract `isSandboxActive` to lib/sandbox/isSandboxActive.ts - KISS: rename lib/supabase/sessions/updateSessionSandboxState.ts -> updateSession.ts, generalize signature to (id, TablesUpdate<"sessions">) Tier 1 correctness gaps from the open-agents comparison: 1. GitHub URL validation via parseGitHubRepoUrl in validateCreateSandboxBody — bad URLs now return a clean 400 instead of falling through to a confusing 502 from the sandbox provider 2. Service GitHub token plumbed into connectSandbox options via new lib/github/getServiceGithubToken.ts — private repos can now clone 3. snapshot_url + snapshot_created_at cleared on fresh provision so GET /api/sandbox/status no longer surfaces stale snapshot URLs from prior runs TDD red -> green: - 5 new unit-test files for the extracted helpers (buildSource, isoToEpochMs, buildLifecycle, isSandboxActive, getServiceGithubToken) - updateSession.test.ts replaces updateSessionSandboxState.test.ts - Updated validator + handler tests for the contract changes (drop isNewBranch, add bad-URL 400 cases, assert githubToken plumbing, assert snapshot_url/snapshot_created_at: null in update payload) - Confirmed RED before each implementation - Suite: 2499 -> 2516 (+17 net new tests), pnpm lint:check clean Files net delta: -241 / +70 lines (extractions + handler shrinks) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(sandbox): drop branch from POST /api/sandbox contract YAGNI/KISS per review feedback — chat always works off the repo's default branch, so the explicit `branch` input adds no value. - Drop `branch` from createSandboxBodySchema - Inline the now-trivial source object in createSandboxHandler (`{ repo: body.repoUrl }`) and delete `lib/sandbox/buildSource.ts` + its test - Read `currentBranch` for the response from the sandbox handle's own `currentBranch` property (whatever the SDK actually checked out), falling back to "main" — no longer derives from a request field that no longer exists Tests: TDD red -> green. - Validator test asserts `branch` is stripped from the body even if a client still sends it - Handler test asserts `currentBranch` in the response comes from `sandbox.currentBranch` (mocked to "release/v2") not from input - Suite stays at 2516 (-1 from buildSource.test deletion +1 new currentBranch test) Pairs with docs PR recoupable/docs#194 (merged) which already removed `branch` and `isNewBranch` from the published OpenAPI spec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sweetmantech
added a commit
that referenced
this pull request
May 7, 2026
…open-agents (#522) (#524) * feat(sandbox): port POST /api/sandbox + GET /api/sandbox/status from open-agents Implements the two session-scoped sandbox endpoints required to drive the chat "loading sandbox..." UX on session entry — matching the contract documented in recoupable/docs#192 (now merged on main). POST /api/sandbox provisions or resumes a Sandbox via the abstraction inlined in #507. When sessionId is supplied, the deterministic sandboxName ensures resume idempotency and the resolved sandbox state is persisted onto the session row (sandbox_state, lifecycle_state = "active", lifecycle_version bumped, sandbox_expires_at, last_activity_at) so subsequent GET /api/sandbox/status calls report the sandbox as active. GET /api/sandbox/status is DB-only — reads the session row, computes status as "active" when sandbox_state is set and not expired (10s buffer to match open-agents), otherwise "no_sandbox". hasSnapshot is true when snapshot_url is set. Mirrors the lifecycle envelope shape from open-agents so the frontend cutover is byte-identical. Files follow existing api conventions: - Route shells in app/api/sandbox/ delegate to handlers in lib/sandbox/ - Auth via validateAuthContext (Privy Bearer or x-api-key) - Validation via Zod (validateCreateSandboxBody) - Supabase ops in lib/supabase/sessions/ (one fn per file) - Error envelope { status: "error", error } matches sessions PRs TDD red → green: - 7 new test files added covering validator, helper, Supabase wrapper, both handlers, and the two route shells - 30 new tests, all passing (was 2461, now 2491) - pnpm lint:check clean Out of scope (deferred to follow-up PRs): - Org-snapshot lookup / kickBuildOrgSnapshotWorkflow (cold-start opt) - Skill installation (installSessionGlobalSkills) - Lifecycle workflow kick (no workflow infra in api yet) - DELETE /api/sandbox + PUT /api/sandbox/snapshot (no UI callers identified during the open-agents grep audit) - /api/sandbox/{extend,activity,reconnect,snapshot} sub-routes — to follow once these two land and the chat UX is validated * fix(sandbox): treat type-stub sandbox_state as no_sandbox in /status Smoke test against the preview deployment caught a regression that defeated the entire loading-state UX this PR exists to enable: GET /api/sandbox/status reported `"active"` immediately after POST /api/sessions, before any sandbox had been provisioned. Root cause: POST /api/sessions (PR #515) inserts `sandbox_state` as the type stub `{ type: "vercel" }`. The previous `isSandboxActive` check `if (!row.sandbox_state) return false` saw a truthy object and fell through; with `sandbox_expires_at = null` (no expiry yet), the function returned true. Fix: introduce `hasRuntimeSandboxState(state)` that distinguishes the type stub from real runtime metadata by requiring a non-empty `sandboxName` (set by `getSessionSandboxName(sessionId)` in POST /api/sandbox and preserved by the abstraction's `getState()`). Mirrors open-agents' equivalent helper. TDD red → green: - Regression test pinned to the exact production scenario (sandbox_state = {type:"vercel"}, sandbox_expires_at = null, lifecycle_state = "provisioning") asserting status=no_sandbox - Companion test asserting status=active once sandboxName is set - 6 unit tests for the new helper covering null/undefined, scalars, type stub, populated state, and empty-string sandboxName edge case - Confirmed RED before implementing, GREEN after - Suite: 2491 → 2499 (+8 new tests), pnpm lint:check clean * refactor(sandbox): SRP/KISS extractions + Tier 1 correctness fixes Addresses review feedback on PR #522 and the "missing from open-agents" audit: User-flagged review comments: - SRP: extract `buildSource` to lib/sandbox/buildSource.ts - YAGNI: drop `isNewBranch` from POST /api/sandbox — chat never sets it (note: docs PR #192 still documents it; will open follow-up docs PR to drop from sandbox.json) - SRP: extract `isoToEpochMs` to lib/sandbox/isoToEpochMs.ts - SRP: extract `buildLifecycle` to lib/sandbox/buildLifecycle.ts - SRP: extract `isSandboxActive` to lib/sandbox/isSandboxActive.ts - KISS: rename lib/supabase/sessions/updateSessionSandboxState.ts -> updateSession.ts, generalize signature to (id, TablesUpdate<"sessions">) Tier 1 correctness gaps from the open-agents comparison: 1. GitHub URL validation via parseGitHubRepoUrl in validateCreateSandboxBody — bad URLs now return a clean 400 instead of falling through to a confusing 502 from the sandbox provider 2. Service GitHub token plumbed into connectSandbox options via new lib/github/getServiceGithubToken.ts — private repos can now clone 3. snapshot_url + snapshot_created_at cleared on fresh provision so GET /api/sandbox/status no longer surfaces stale snapshot URLs from prior runs TDD red -> green: - 5 new unit-test files for the extracted helpers (buildSource, isoToEpochMs, buildLifecycle, isSandboxActive, getServiceGithubToken) - updateSession.test.ts replaces updateSessionSandboxState.test.ts - Updated validator + handler tests for the contract changes (drop isNewBranch, add bad-URL 400 cases, assert githubToken plumbing, assert snapshot_url/snapshot_created_at: null in update payload) - Confirmed RED before each implementation - Suite: 2499 -> 2516 (+17 net new tests), pnpm lint:check clean Files net delta: -241 / +70 lines (extractions + handler shrinks) * refactor(sandbox): drop branch from POST /api/sandbox contract YAGNI/KISS per review feedback — chat always works off the repo's default branch, so the explicit `branch` input adds no value. - Drop `branch` from createSandboxBodySchema - Inline the now-trivial source object in createSandboxHandler (`{ repo: body.repoUrl }`) and delete `lib/sandbox/buildSource.ts` + its test - Read `currentBranch` for the response from the sandbox handle's own `currentBranch` property (whatever the SDK actually checked out), falling back to "main" — no longer derives from a request field that no longer exists Tests: TDD red -> green. - Validator test asserts `branch` is stripped from the body even if a client still sends it - Handler test asserts `currentBranch` in the response comes from `sandbox.currentBranch` (mocked to "release/v2") not from input - Suite stays at 2516 (-1 from buildSource.test deletion +1 new currentBranch test) Pairs with docs PR recoupable/docs#194 (merged) which already removed `branch` and `isNewBranch` from the published OpenAPI spec. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
sweetmantech
added a commit
that referenced
this pull request
May 7, 2026
#531) * feat(sandbox): port buildOrgSnapshotWorkflow + Vercel Workflow runtime Closes the biggest remaining cutover gap: when a recoupable org repo is requested via POST /api/sandbox and no snapshot exists yet, fire a durable background workflow that builds one. The current request still pays the slow full-clone path; what's protected is that *future* sessions for the same org warm-boot from the new snapshot in seconds. This is the first Vercel Workflow integration in api. Per the direction of using Vercel Workflow as the workflow runner going forward (deprecating Trigger.dev over time), this PR also lands the infra: the `workflow` package, `withWorkflow(nextConfig)` wrapper, and the `app/workflows/` convention. Files: - `next.config.ts` — wraps the config with `withWorkflow` from `workflow/next` so workflow files (with `"use workflow"` / `"use step"` directives) are deployable - `app/workflows/buildOrgSnapshot.ts` — preserves the open-agents workflow code structure exactly. One step that calls api's existing `refreshBaseSnapshot` (inlined via PR #507) to provision a sandbox named `<orgRepoName>`, run `git clone --depth=1 <cloneUrl>`, and snapshot the result - `lib/sandbox/kickBuildOrgSnapshotWorkflow.ts` — fire-and-forget invocation via `start(buildOrgSnapshotWorkflow, [input])` from `workflow/api`. Failures are logged but never surface in the request — the request always falls back to the slow full-clone path - `lib/sandbox/defaultBaseSnapshotId.ts` — `DEFAULT_SANDBOX_BASE_SNAPSHOT_ID` constant, env-overridable. Used by the build workflow as the base image (bun + jq + agent-browser + chromium + code-server) so the workflow only does the clone, not the runtime install - `lib/sandbox/createSandboxHandler.ts` — wired in: when `extractOrgRepoName` returns non-null and `findOrgSnapshot` misses, kick the workflow TDD red -> green: - 3 new createSandboxHandler tests: - kicks the workflow on recoupable miss with the right input - does NOT kick when an existing snapshot is found (hit case) - does NOT kick for non-recoupable repos - All existing tests pass (the mock for kickBuildOrgSnapshotWorkflow is no-op so unchanged tests don't see the kick) - Suite: 2574 -> 2577 (+3). pnpm lint:check + tsc --noEmit clean. Phase 2 (sandboxLifecycleWorkflow + evaluateSandboxLifecycle) follows in a separate PR — that one's much bigger and ports the stateful auto-pause / status-check-overdue logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sandbox): point base snapshot id at the recoup-team-built snapshot The previous hardcoded id (snap_EjsphVxi07bFKrfojljJdIS41KHT) lived in open-agents' Vercel team and returned 404 from api's project, breaking the build-org-snapshot workflow on its first step. Built a recoup-team equivalent snapshot manually via the @vercel/sandbox SDK by provisioning a clean Amazon Linux 2023 sandbox and running: - sudo dnf install -y jq - curl -fsSL https://bun.sh/install | sudo BUN_INSTALL=/usr/local bash - sudo npm install -g agent-browser - curl -fsSL https://code-server.dev/install.sh | sudo sh Then snapshotted via `vercel sandbox snapshot <id> --stop`. Result: snap_RgVtpDO4y1BJHQiUbptMwS3Rt2EQ. Tooling note (in the file comment): chromium is intentionally NOT in this base. Amazon Linux 2023's default repo doesn't carry it, and agent-browser fetches a managed Playwright browser on first use, so having it in the base would just bloat the image. The header comment now also documents the install commands and refresh procedure so the next person who needs to add tooling has a clear path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sandbox): SRP split + shellEscape cloneUrl + drop URL from logs Three review fixes bundled: 1) SRP per sweetmantech: split app/workflows/buildOrgSnapshot.ts into - app/workflows/buildSnapshotStep.ts (the "use step" function) - app/workflows/buildOrgSnapshotWorkflow.ts (the "use workflow" function — filename now matches the export, addressing the cubic P2 about filename mismatch) 2) P0 command injection (cubic): wrap cloneUrl with shellEscape() in the `git clone --depth=1 ...` command. The Zod + parseGitHubRepoUrl validators upstream of this workflow already reject anything that doesn't match `^https:\/\/github\.com\/recoupable\/...`, so this isn't exploitable today — but shell-escaping is defense-in-depth that survives validator changes. Zero behavior change for valid inputs (extra single quotes around an already-clean URL). 3) P1 credential leak (cubic): dropped `cloneUrl` from every log line in the workflow + kick. The `https://user:token@github.com/...` shape is also blocked by the regex validator, but log scrubbing is the cheaper of the two layers if the validator ever loosens. `sandboxName` (the regex-extracted repo name only) remains in logs — uniquely identifies the org for observability without exposing tokens. Out of scope: cubic's P2 about duplicate workflow runs (same fire-and-forget pattern as open-agents — duplicates are wasteful but not incorrect; can add a Vercel Workflow idempotency key in a follow-up if observed in practice). Tests: existing handler tests still pass unchanged (the kick is mocked at the lib path, which didn't change). Suite remains 2577 / 2577. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 of the open-agents → main app migration, sub-step 1 of N. Inlines the sandbox abstraction layer from `open-agents/packages/sandbox` into `api/lib/sandbox/` side-by-side with the existing helpers. Pure addition — nothing in api imports the new code yet, so this PR is provably zero-risk for production behavior.
This is the SRP cut: get the abstraction port reviewed and merged on its own merits. Caller refactors land in subsequent PRs (2.2) and dead-code removal in 2.3.
What's added
13 source files + 2 test files.
Naming choice: `abstraction.ts` instead of `index.ts`
open-agents' top-level export was `packages/sandbox/index.ts`. I renamed it to `abstraction.ts` here so it doesn't auto-resolve as the `@/lib/sandbox` import path — that path still resolves file-by-file (`@/lib/sandbox/createSandbox` etc.) for the existing api helpers. Callers of the new abstraction explicitly write `@/lib/sandbox/abstraction`. Keeps the two surfaces clearly distinguishable during the migration window.
Test framework conversion
Open-agents uses `bun:test`; api uses `vitest`. Mechanical replacements:
Lint accommodations
Verification
Risk
Effectively zero. This PR adds files; it does not modify any existing code path that reaches production. The new files are reachable only by importing them, and nothing imports them. If there's any issue, it's contained to the new files' tests, which themselves prove the abstraction works.
What's next in the chain
🤖 Generated with Claude Code
Summary by cubic
Inlines the Open Agents sandbox abstraction into
api/lib/sandboxand adds a Vercel-backed implementation behind a unifiedconnectSandboxAPI. Follow-up refactor splits helpers into focused modules underlib/sandbox/vercel/*; no callers yet, so behavior is unchanged.New Features
lib/sandbox:interface.ts,types.ts, andfactory.tswithconnectSandbox.VercelSandbox,connectVercel,connectVercelSandbox, andrefreshBaseSnapshot; uses@vercel/sandbox.SANDBOX_HOST/SANDBOX_URL_<PORT>into command env; supportsexecDetached,extendTimeout, and nativesnapshot.abstraction.tsto avoid@/lib/sandboxpath collisions during migration.Refactors
connect/:connectVercel,connectNamedSandbox,buildCreateConfig,getSandboxName,getRemainingTimeout,isSandboxNotFoundError,toErrorMessage,types.snapshot-refresh/:refreshBaseSnapshot(+DEFAULT_*),defaultConnectSnapshotSandbox,formatCommandFailure,formatCommandOutput,types.sandbox/:VercelSandboxclass,connectVercelSandbox,buildAuthenticatedGitHubUrl,buildGitHubCredentialBrokeringPolicy,syncGitHubCredentialBrokering,isStoppedSessionStatus,getRemainingTimeoutFromSession,constants,types.utils.tstoconfigureGitUser.ts; updated barrels and imports.vitest(+29); lint clean; still not imported by app code.Written for commit b8e2b05. Summary will update on new commits.
Summary by CodeRabbit