feat(ai-sandbox): add Blaxel provider - #1065
Conversation
Signed-off-by: Michael Stolarz <146425971+SystemSculpt@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds the ChangesBlaxel provider implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BlaxelProvider
participant BlaxelSDK
participant BlaxelHandle
Client->>BlaxelProvider: create(config)
BlaxelProvider->>BlaxelSDK: create sandbox
BlaxelProvider->>BlaxelHandle: prepare workspace
BlaxelHandle-->>Client: return sandbox handle
Client->>BlaxelProvider: resume(id)
BlaxelProvider->>BlaxelSDK: inspect sandbox
BlaxelProvider-->>Client: return handle or null
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/ai-sandbox-blaxel/tests/blaxel.test.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace these tests alongside their source modules.
Both files use Vitest correctly, but both are outside the required colocated test layout.
packages/ai-sandbox-blaxel/tests/blaxel.test.ts#L1-L3: Move this test beside its relevant module underpackages/ai-sandbox-blaxel/src/and update its relative import.packages/ai-sandbox-blaxel/tests/journal.conformance.test.ts#L1-L6: Move this test beside its relevant module underpackages/ai-sandbox-blaxel/src/and update its relative import.As per coding guidelines, “Test files should be placed alongside source code as *.test.ts files using Vitest with happy-dom for DOM testing.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-blaxel/tests/blaxel.test.ts` around lines 1 - 3, Move packages/ai-sandbox-blaxel/tests/blaxel.test.ts into the relevant source directory under packages/ai-sandbox-blaxel/src/ and update its relative import of blaxelSandbox. Move packages/ai-sandbox-blaxel/tests/journal.conformance.test.ts alongside its relevant source module under the same src directory and update its relative import; retain the existing Vitest test behavior and naming.Source: Coding guidelines
packages/ai-sandbox-blaxel/src/index.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
BlaxelSandboxLikewithBlaxelHandleDeps.
BlaxelHandleDeps.sandboxhas typeBlaxelSandboxLike, but that type is not re-exported. A consumer who constructsBlaxelHandledirectly cannot name or implement the dependency type without deep-importing./handle.♻️ Proposed export addition
-export type { BlaxelHandleDeps } from './handle' +export type { + BlaxelHandleDeps, + BlaxelSandboxLike, + BlaxelDirectoryLike, + BlaxelProcessLike, + BlaxelProcessRequestLike, + BlaxelPreviewLike, + BlaxelWatchEventLike, +} from './handle'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-blaxel/src/index.ts` around lines 3 - 4, Update the package exports alongside BlaxelHandleDeps to re-export the BlaxelSandboxLike type from the handle module, allowing consumers to name or implement the sandbox dependency without deep imports.packages/ai-sandbox-blaxel/src/handle.ts (1)
383-399: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape
\nconsistently inside the generated shell script.Lines 385, 398, and 399 are template literals, so
'\n'becomes a real newline character in the emitted script rather than the two characters\andn. The resultingprintfstill prints a newline, because a literal newline inside single quotes is valid shell. Line 442 uses'%s\\n'and emits the escape sequence instead. The two forms are equivalent today, but the embedded raw newlines split one logical script line into two and break if the script text is ever normalized or re-indented.Use
\\nin all three places for consistency with Line 442.♻️ Proposed change
- ` printf '%s' ${q(recordPrefix)}`, + ` printf '%s' ${q(recordPrefix)}`, ` base64 < ${chunkFile} | tr -d '\r\n'`, - ` printf '\n'`, + ` printf '\\n'`, @@ - ` printf '%s\n' ${q(label)} >> ${limitsFile}`, - ` printf '%s\n' ${q(overflowMarker)}`, + ` printf '%s\\n' ${q(label)} >> ${limitsFile}`, + ` printf '%s\\n' ${q(overflowMarker)}`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-blaxel/src/handle.ts` around lines 383 - 399, Update the generated shell-script strings in the surrounding chunk-processing logic to use escaped backslash-n sequences consistently in all three affected printf calls, matching the existing format used by the limits-file output. Ensure the emitted script contains the two characters \ and n rather than embedding literal newlines.packages/ai-sandbox-blaxel/tests/provider.test.ts (1)
347-370: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCreate the rejected
deleteGatelazily to avoid an unhandled rejection.
deleteGateis assigned an already-rejected promise, but no handler attaches untilSandboxInstance.deleteruns, which happens after several awaited steps. Node can reportunhandledRejectionin that window and make the test flaky. Make the fake produce the rejection whendeleteis called.♻️ Proposed change: reject inside the fake
-let deleteGate: Promise<Record<string, never>> | undefined +let deleteGate: (() => Promise<Record<string, never>>) | undefined @@ delete: async (name: string) => { calls.deleted.push(name) - return deleteGate ?? {} + return deleteGate ? await deleteGate() : {} },Then set
deleteGate = () => Promise.reject({ status: 500, message: 'delete failed' })in the affected tests. Apply the same pattern to the eagerly rejectedcreateGatevalues at Lines 331 and 348.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-blaxel/tests/provider.test.ts` around lines 347 - 370, Update the fake gate used by SandboxInstance.delete to create its rejected promise only when delete is invoked, then change the affected tests to assign a rejection-producing function instead of an already-rejected promise. Apply the same lazy-rejection pattern to createGate in both affected create-error tests, preserving their existing error assertions and call tracking.packages/ai-sandbox-blaxel/tests/handle.test.ts (1)
703-725: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the live child-process cleanup failure-safe.
rmSync(outputDir!)and the child termination run only on the success path. Ifvi.waitForat Line 707 or Line 718 fails, the test leaves a real/tmp/tanstack-ai-output-*directory and possibly livesleep 30process groups on the machine. Move the cleanup into atry/finally.The test also depends on
bashbeing installed, because the generated wrapper ends withexec bash <supervisor>. On a runner withoutbash, thepidsfile never appears and the failure mode is an opaquewaitFortimeout.♻️ Proposed change
const child = spawnChild('/bin/sh', ['-c', script], { stdio: 'ignore', }) - const pidsPath = `${outputDir!}/pids` - await vi.waitFor(() => expect(existsSync(pidsPath)).toBe(true)) - const pids = readFileSync(pidsPath, 'utf8') - .trim() - .split(/\s+/) - .map(Number) - const exited = new Promise<void>((resolve, reject) => { - child.once('error', reject) - child.once('exit', () => resolve()) - }) - child.kill('SIGTERM') - await exited - await vi.waitFor(() => { - for (const pid of pids) { - expect(() => process.kill(-pid, 0)).toThrow() - } - }) - rmSync(outputDir!, { recursive: true, force: true }) + try { + const pidsPath = `${outputDir!}/pids` + await vi.waitFor(() => expect(existsSync(pidsPath)).toBe(true)) + const pids = readFileSync(pidsPath, 'utf8') + .trim() + .split(/\s+/) + .map(Number) + const exited = new Promise<void>((resolve, reject) => { + child.once('error', reject) + child.once('exit', () => resolve()) + }) + child.kill('SIGTERM') + await exited + await vi.waitFor(() => { + for (const pid of pids) { + expect(() => process.kill(-pid, 0)).toThrow() + } + }) + } finally { + child.kill('SIGKILL') + rmSync(outputDir!, { recursive: true, force: true }) + } resolveWait({ exitCode: 0, stdout: '', stderr: '' })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-blaxel/tests/handle.test.ts` around lines 703 - 725, Make the child-process test cleanup failure-safe by wrapping the spawn, PID-file wait, termination, and process-group assertions in a try/finally, ensuring the child is terminated and outputDir is removed even when either vi.waitFor fails. Also make the test explicitly skip or guard execution when bash is unavailable, since the generated wrapper invokes bash and otherwise produces a timeout.packages/ai-sandbox-blaxel/src/provider.ts (1)
255-260: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReconciliation can delay the rejection by about 29 seconds and ignores
input.signal.
findOwnedSandboxpolls up toCREATE_RECONCILE_ATTEMPTS(30) times with a 1000 ms sleep. ThemayHaveCreatedSandboxbranch at Line 259 awaits that reconciliation beforecreate()rejects. A caller that receives a 504 therefore waits about 29 seconds, and an abort raised during that window has no effect becauseinput.signalis not passed intofindOwnedSandbox.Consider passing
input.signalinto the reconciliation loop and detaching the wait from the caller promise, as the abort path at Line 245 already does.♻️ Sketch: stop polling once the caller aborts
private async findOwnedSandbox( name: string, attemptId: string, + signal?: AbortSignal, ): Promise<BlaxelSandboxLike | undefined> { for (let attempt = 0; attempt < CREATE_RECONCILE_ATTEMPTS; attempt += 1) { + if (signal?.aborted) return undefined try {Also applies to: 290-306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-blaxel/src/provider.ts` around lines 255 - 260, Update the mayHaveCreatedSandbox reconciliation path around cleanupOwnedSandbox so create() rejects immediately instead of awaiting polling. Run cleanupOwnedSandbox in a detached, safely handled promise as in the existing abort path, and propagate input.signal through cleanupOwnedSandbox to findOwnedSandbox so reconciliation stops when the caller aborts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai-sandbox-blaxel/src/handle.ts`:
- Around line 956-958: Update the token creation flow around
preview.tokens.create so the expiry is derived from the configured
BlaxelSandboxConfig.previewTtl instead of the fixed PREVIEW_TOKEN_TTL_MS value.
Preserve the existing default behavior when previewTtl is unset, and ensure
longer configured preview lifetimes produce tokens that remain valid for the
full preview duration.
In `@packages/ai-sandbox-blaxel/tests/blaxel.test.ts`:
- Line 156: Update the test around provider.create({}) to register the returned
sandbox handle with the suite cleanup tracker immediately after creation,
ensuring afterAll can retry destroy even if later assertions or cleanup fail.
In `@packages/ai-sandbox/README.md`:
- Line 56: Update the provider installation documentation around the
`@tanstack/ai-sandbox-blaxel` entry to explicitly show installing the selected
provider package separately, or clearly state that provider packages must be
installed independently of the base package. Ensure the example uses the
provider package name shown in the table.
---
Nitpick comments:
In `@packages/ai-sandbox-blaxel/src/handle.ts`:
- Around line 383-399: Update the generated shell-script strings in the
surrounding chunk-processing logic to use escaped backslash-n sequences
consistently in all three affected printf calls, matching the existing format
used by the limits-file output. Ensure the emitted script contains the two
characters \ and n rather than embedding literal newlines.
In `@packages/ai-sandbox-blaxel/src/index.ts`:
- Around line 3-4: Update the package exports alongside BlaxelHandleDeps to
re-export the BlaxelSandboxLike type from the handle module, allowing consumers
to name or implement the sandbox dependency without deep imports.
In `@packages/ai-sandbox-blaxel/src/provider.ts`:
- Around line 255-260: Update the mayHaveCreatedSandbox reconciliation path
around cleanupOwnedSandbox so create() rejects immediately instead of awaiting
polling. Run cleanupOwnedSandbox in a detached, safely handled promise as in the
existing abort path, and propagate input.signal through cleanupOwnedSandbox to
findOwnedSandbox so reconciliation stops when the caller aborts.
In `@packages/ai-sandbox-blaxel/tests/blaxel.test.ts`:
- Around line 1-3: Move packages/ai-sandbox-blaxel/tests/blaxel.test.ts into the
relevant source directory under packages/ai-sandbox-blaxel/src/ and update its
relative import of blaxelSandbox. Move
packages/ai-sandbox-blaxel/tests/journal.conformance.test.ts alongside its
relevant source module under the same src directory and update its relative
import; retain the existing Vitest test behavior and naming.
In `@packages/ai-sandbox-blaxel/tests/handle.test.ts`:
- Around line 703-725: Make the child-process test cleanup failure-safe by
wrapping the spawn, PID-file wait, termination, and process-group assertions in
a try/finally, ensuring the child is terminated and outputDir is removed even
when either vi.waitFor fails. Also make the test explicitly skip or guard
execution when bash is unavailable, since the generated wrapper invokes bash and
otherwise produces a timeout.
In `@packages/ai-sandbox-blaxel/tests/provider.test.ts`:
- Around line 347-370: Update the fake gate used by SandboxInstance.delete to
create its rejected promise only when delete is invoked, then change the
affected tests to assign a rejection-producing function instead of an
already-rejected promise. Apply the same lazy-rejection pattern to createGate in
both affected create-error tests, preserving their existing error assertions and
call tracking.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a619c30d-f13a-4915-9c71-75bf2d5c4d53
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/add-ai-sandbox-blaxel.mddocs/config.jsondocs/sandbox/providers.mdpackages/ai-sandbox-blaxel/CHANGELOG.mdpackages/ai-sandbox-blaxel/package.jsonpackages/ai-sandbox-blaxel/src/handle.tspackages/ai-sandbox-blaxel/src/index.tspackages/ai-sandbox-blaxel/src/provider.tspackages/ai-sandbox-blaxel/src/utils.tspackages/ai-sandbox-blaxel/tests/blaxel.test.tspackages/ai-sandbox-blaxel/tests/handle.test.tspackages/ai-sandbox-blaxel/tests/journal.conformance.test.tspackages/ai-sandbox-blaxel/tests/provider.test.tspackages/ai-sandbox-blaxel/tsconfig.jsonpackages/ai-sandbox-blaxel/vite.config.tspackages/ai-sandbox/README.md
|
Thanks, these were useful catches. I fixed the preview-token lifetime, made the live cleanup tests safer, clarified the provider install step, and cleaned up the two smaller test and shell-script issues. I kept the tests in Everything passes again: the full uncached PR gate across all 74 projects, plus 93 live Blaxel tests with 3 expected capability skips. No test sandboxes were left behind. |
1 participant
🎯 Changes
@tanstack/ai-sandbox-blaxelas a managed cloud sandbox provider.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Safety and scope
killableProcessesfalse until child process-group termination has direct proof.Test plan
pnpm test:pracross 74 projectsexec, andspawntanstack-ai-*sandbox remained after the credentialed suiteorigin/maindurability failureBrowser baseline note
The only browser failure is
durable runs - takeover - a real disconnect, then an attach, continues the stream. It expectsdetachedSinceto be cleared after completion.The same assertion fails on every retry in a clean detached
origin/mainworktree after all 53 upstream packages are rebuilt. This PR does not change the test, route, persistence code, sandbox core, or durability code. That fix should stay outside this provider PR.Review notes
@blaxel/coreuses^0.3.10, the current stable release at validation time.Summary by CodeRabbit
New Features
Documentation