promote: test → main (open-agents sandbox abstraction)#508
promote: test → main (open-agents sandbox abstraction)#508sweetmantech merged 3 commits intomainfrom
Conversation
Phase 2 prerequisite. Aligns api with the version open-agents uses, so a
later PR can absorb open-agents' packages/sandbox into api/lib/sandbox/
without a dep mismatch.
The v1 -> v2 API change relevant to api: Sandbox.sandboxId is renamed
to Sandbox.name, and Sandbox.get's params likewise take { name } instead
of { sandboxId }. api's internal vocabulary keeps "sandboxId" — it's in
the public HTTP response shape (SandboxCreatedResponse, the JSDoc on
app/api/sandboxes/route.ts) and in lib/trigger/triggerPromptSandbox's
own argument schema. The translation only happens at the v2 boundary
inside lib/sandbox/:
sandbox.sandboxId -> sandbox.name
Sandbox.get({ sandboxId }) -> Sandbox.get({ name })
Sandbox["sandboxId"] type -> Sandbox["name"]
10 production-code edits across 6 files; tests updated to mock Sandbox
objects with `name` and to assert `Sandbox.get({ name })`.
Verification:
- pnpm install: clean
- pnpm exec tsc --noEmit: clean for lib/sandbox/ (preexisting unrelated
type errors elsewhere left untouched, since the api repo's CI runs
eslint not tsc)
- pnpm lint:check: clean
- pnpm test lib/sandbox: 152/152 pass
- pnpm test (full): 2362/2362 pass
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sandbox): inline open-agents sandbox abstraction (Phase 2.1)
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>
* refactor(sandbox): split inlined files per api SRP convention
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>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (30)
✨ 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 11 minutes and 23 seconds.Comment |
There was a problem hiding this comment.
13 issues found across 32 files
Confidence score: 3/5
- There is concrete regression/security risk in
lib/sandbox/vercel/configureGitUser.ts: interpolating rawgitUserinto shell commands can lead to command injection or malformed command execution if values are not escaped/validated. lib/sandbox/vercel/sandbox/buildAuthenticatedGitHubUrl.tshas a host-matching bug (substring-based) that can accept non-GitHub domains, which is user-impacting behavior rather than just style.- Most other findings are maintainability-focused (100-line rule in
lib/sandbox/vercel/sandbox/VercelSandbox.ts,lib/sandbox/interface.ts,lib/sandbox/vercel/config.ts, and long test files), so this is not a guaranteed blocker but does increase review and future-change risk. - Pay close attention to
lib/sandbox/vercel/configureGitUser.ts,lib/sandbox/vercel/sandbox/buildAuthenticatedGitHubUrl.ts, andlib/sandbox/vercel/__tests__/sandbox.test.ts- shell safety, URL validation correctness, and missingawaitonrejectsassertions are the highest-impact fixes before merge.
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/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 is far over the 100-line limit and bundles too many unrelated concerns into one module; it should be split into smaller focused test files.</violation>
<violation number="2" location="lib/sandbox/vercel/__tests__/sandbox.test.ts:564">
P2: Await this `rejects` assertion so the test actually waits for the rejection check.</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 check.</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 check.</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 repository’s 100-line maximum.</violation>
</file>
<file name="lib/sandbox/vercel/config.ts">
<violation number="1" location="lib/sandbox/vercel/config.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
File exceeds the 100-line maximum required by the maintainability rule.</violation>
</file>
<file name="lib/sandbox/interface.ts">
<violation number="1" location="lib/sandbox/interface.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
New file exceeds the 100-line maximum required by Rule 3.</violation>
</file>
<file name="lib/sandbox/vercel/sandbox/VercelSandbox.ts">
<violation number="1" location="lib/sandbox/vercel/sandbox/VercelSandbox.ts:1">
P1: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
File exceeds the 100-line limit required by the maintainability rule.</violation>
</file>
<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**
Single exported function `connectSandbox` does not match the module basename `factory`.</violation>
</file>
<file name="lib/sandbox/vercel/index.ts">
<violation number="1" location="lib/sandbox/vercel/index.ts:2">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Barrel module exports multiple top-level functions instead of a single primary function matching the filename.</violation>
</file>
<file name="lib/sandbox/abstraction.ts">
<violation number="1" location="lib/sandbox/abstraction.ts:16">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Module exports multiple top-level functions instead of a single primary export matching the filename.</violation>
</file>
<file name="lib/sandbox/vercel/sandbox/buildAuthenticatedGitHubUrl.ts">
<violation number="1" location="lib/sandbox/vercel/sandbox/buildAuthenticatedGitHubUrl.ts:2">
P2: The GitHub URL matcher is too permissive and can accept non-GitHub hosts (e.g. `notgithub.com`) due to substring matching.</violation>
</file>
<file name="lib/sandbox/vercel/configureGitUser.ts">
<violation number="1" location="lib/sandbox/vercel/configureGitUser.ts:11">
P1: Avoid interpolating raw `gitUser` values into shell commands; quote/escape or validate them before passing to `sandbox.exec`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant C as Caller (API/Agent)
participant F as Factory (connectSandbox)
participant VI as VercelSandbox (Implementation)
participant SDK as @vercel/sandbox (SDK)
participant GH as GitHub (External)
Note over C,GH: Sandbox Connection Flow (NEW Abstraction)
C->>F: connectSandbox(state, options)
alt state.type == "vercel"
F->>VI: NEW: connectVercel(state, options)
alt Reconnecting to Named Sandbox
VI->>SDK: Sandbox.get({ name, resume })
else Creating Fresh Sandbox
VI->>SDK: Sandbox.create(config)
end
SDK-->>VI: session/sdk instance
opt GitHub Token Provided
VI->>VI: NEW: buildGitHubCredentialBrokeringPolicy()
VI->>SDK: NEW: updateNetworkPolicy(authRules)
Note right of VI: Injects Basic/Bearer auth into VM network layer
end
opt Hooks Configured
VI->>C: NEW: hooks.afterStart(sandbox)
end
VI-->>F: Sandbox Interface
end
F-->>C: Sandbox Interface
Note over C,GH: Runtime Operation Flow
C->>VI: exec(command, options)
VI->>VI: NEW: Ingest SANDBOX_HOST & SANDBOX_URL_<PORT>
Note right of VI: Dynamic env var injection for preview URLs
VI->>SDK: session.runCommand(command, env)
SDK-->>VI: ExecResult
VI-->>C: ExecResult
Note over C,GH: Snapshot Refresh Flow (Maintenance)
C->>VI: NEW: refreshBaseSnapshot(options)
VI->>F: connectSandbox(baseSnapshotId)
F-->>VI: temporary sandbox
VI->>VI: exec(setup commands)
VI->>SDK: NEW: snapshot()
SDK-->>VI: SnapshotResult (snapshotId)
VI->>VI: stop()
VI-->>C: RefreshBaseSnapshotResult
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: Enforce Clear Code Style and Maintainability Practices
Test file is far over the 100-line limit and bundles too many unrelated concerns into one module; it should be split into smaller focused test files.
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 is far over the 100-line limit and bundles too many unrelated concerns into one module; it should be split into smaller focused test files.</comment>
<file context>
@@ -0,0 +1,684 @@
+import { beforeAll, beforeEach, describe, expect, vi, test } from "vitest";
+
+const portDomains = new Map<number, string>();
</file context>
| @@ -0,0 +1,976 @@ | |||
| /* eslint-disable @typescript-eslint/member-ordering -- Inlined from open-agents/packages/sandbox; preserving original member order to keep the diff against the source minimal. */ | |||
There was a problem hiding this comment.
P1: Custom agent: Enforce Clear Code Style and Maintainability Practices
File exceeds the 100-line limit required by the maintainability rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/sandbox/VercelSandbox.ts, line 1:
<comment>File exceeds the 100-line limit required by the maintainability rule.</comment>
<file context>
@@ -0,0 +1,976 @@
+/* eslint-disable @typescript-eslint/member-ordering -- Inlined from open-agents/packages/sandbox; preserving original member order to keep the diff against the source minimal. */
+import { Sandbox as VercelSandboxSDK } from "@vercel/sandbox";
+import type { Dirent } from "fs";
</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: Avoid interpolating raw gitUser values into shell commands; quote/escape or validate them before passing to sandbox.exec.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/configureGitUser.ts, line 11:
<comment>Avoid interpolating raw `gitUser` values into shell commands; quote/escape or validate them before passing to `sandbox.exec`.</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>
| @@ -0,0 +1,221 @@ | |||
| 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 repository’s 100-line maximum.
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 repository’s 100-line maximum.</comment>
<file context>
@@ -0,0 +1,221 @@
+import { describe, expect, vi, test } from "vitest";
+import type { SandboxConnectConfig } from "../../factory";
+import type { ExecResult } from "../../interface";
</file context>
| @@ -0,0 +1,123 @@ | |||
| import type { SandboxHooks } from "../interface"; | |||
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
File exceeds the 100-line maximum required by the maintainability rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/config.ts, line 1:
<comment>File exceeds the 100-line maximum required by the maintainability rule.</comment>
<file context>
@@ -0,0 +1,123 @@
+import type { SandboxHooks } from "../interface";
+
+export interface VercelSandboxConfig {
</file context>
| @@ -0,0 +1,33 @@ | |||
| // interface | |||
There was a problem hiding this comment.
P2: Custom agent: Module should export a single primary function whose name matches the filename
Module exports multiple top-level functions instead of a single primary export matching the filename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/abstraction.ts, line 16:
<comment>Module exports multiple top-level functions instead of a single primary export matching the filename.</comment>
<file context>
@@ -0,0 +1,33 @@
+export type { Source, FileEntry, SandboxStatus } from "./types";
+
+// factory
+export {
+ connectSandbox,
+ type SandboxState,
</file context>
| @@ -0,0 +1,10 @@ | |||
| export function buildAuthenticatedGitHubUrl(repoUrl: string, token: string): string | null { | |||
| const githubUrlMatch = repoUrl.match(/github\.com[/:]([^/]+)\/([^/]+?)(?:\.git)?$/); | |||
There was a problem hiding this comment.
P2: The GitHub URL matcher is too permissive and can accept non-GitHub hosts (e.g. notgithub.com) due to substring matching.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sandbox/vercel/sandbox/buildAuthenticatedGitHubUrl.ts, line 2:
<comment>The GitHub URL matcher is too permissive and can accept non-GitHub hosts (e.g. `notgithub.com`) due to substring matching.</comment>
<file context>
@@ -0,0 +1,10 @@
+export function buildAuthenticatedGitHubUrl(repoUrl: string, token: string): string | null {
+ const githubUrlMatch = repoUrl.match(/github\.com[/:]([^/]+)\/([^/]+?)(?:\.git)?$/);
+
+ if (!githubUrlMatch) {
</file context>
| const githubUrlMatch = repoUrl.match(/github\.com[/:]([^/]+)\/([^/]+?)(?:\.git)?$/); | |
| const githubUrlMatch = repoUrl.match(/(?:^|\/\/|git@)github\.com[/:]([^/]+)\/([^/]+?)(?:\.git)?$/); |
| remainingTimeout: 0, | ||
| }); | ||
|
|
||
| expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow( |
There was a problem hiding this comment.
P2: Await this rejects assertion so the test actually waits for the rejection check.
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 check.</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>
| expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow( | |
| await expect(sandbox.execDetached("npm run dev", "/vercel/sandbox")).rejects.toThrow( |
| remainingTimeout: 0, | ||
| }); | ||
|
|
||
| expect(sandbox.execDetached("bun run dev", "/vercel/sandbox")).rejects.toThrow("wait failed"); |
There was a problem hiding this comment.
P2: Await this rejects assertion so the test actually waits for the rejection check.
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>Await this `rejects` assertion so the test actually waits for the rejection check.</comment>
<file context>
@@ -0,0 +1,684 @@
+ remainingTimeout: 0,
+ });
+
+ expect(sandbox.execDetached("bun run dev", "/vercel/sandbox")).rejects.toThrow("wait failed");
+ });
+
</file context>
| expect(sandbox.execDetached("bun run dev", "/vercel/sandbox")).rejects.toThrow("wait failed"); | |
| await expect(sandbox.execDetached("bun run dev", "/vercel/sandbox")).rejects.toThrow("wait failed"); |
| remainingTimeout: 0, | ||
| }); | ||
|
|
||
| expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow( |
There was a problem hiding this comment.
P2: Await this rejects assertion so the test actually waits for the rejection check.
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 check.</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>
| expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow( | |
| await expect(sandbox.readFile("/vercel/sandbox/missing.txt", "utf-8")).rejects.toThrow( |
Promotes #507 (Phase 2.1 — inline open-agents sandbox abstraction side-by-side with existing helpers) from `test` to `main`.
New content vs main
(The other two commits showing in the compare diff — `6e4242b` and `a8bb14f` — are tree-equivalent to content already on main from prior squash promotions earlier in this session.)
Risk
Effectively zero. PR #507 added 28 new files under `lib/sandbox/` and `lib/sandbox/vercel/` but nothing in api imports any of them yet. The new abstraction is purely additive — old code paths are untouched. If something is wrong, only the new files' own unit tests can fail (they don't, 29/29 pass; full suite 2391/2391).
What this enables
PR 2.2 (caller refactor) can now consume `connectSandbox()` from `@/lib/sandbox/abstraction` and replace api's existing direct `Sandbox.create` / `Sandbox.get` calls. That's where regression risk lives — isolated to the next PR.
Summary by cubic
Adds a provider‑agnostic sandbox abstraction and a Vercel‑backed implementation under
lib/sandbox/. Purely additive;apidoes not import this yet, so runtime behavior is unchanged.Sandboxinterface andconnectSandbox()factory inlib/sandbox/.VercelSandbox,connectVercelSandbox, GitHub credential brokering, preview URL env injection (SANDBOX_HOST,SANDBOX_URL_<PORT>), background exec, timeout extension, and file ops.refreshBaseSnapshot()and typed configs/state for creating base images.Written for commit 28748c7. Summary will update on new commits.