fix: align sandbox base tool staging#816
Conversation
📝 WalkthroughWalkthroughA canonical sandbox toolchain contract is introduced in ChangesSandbox Toolchain Contract and Provider Integration
Sequence Diagram(s)sequenceDiagram
participant Provider as Provider (Modal/Daytona)
participant ToolchainContract as sandbox_runtime.toolchain
participant CommandGen as Command Generators
participant ImageBuild as Base Image Build
Provider->>ToolchainContract: import/load contract
ToolchainContract->>CommandGen: expose opencode_deps, ttyd, git_credential helpers
ImageBuild->>CommandGen: invoke opencode_deps_staging_commands()
CommandGen-->>ImageBuild: return tuple of shell commands
ImageBuild->>CommandGen: invoke ttyd_install_commands()
CommandGen-->>ImageBuild: return tuple with download/verify/install
ImageBuild->>CommandGen: invoke git_credential_helper_commands()
CommandGen-->>ImageBuild: return tuple with wrapper + git config
ImageBuild->>ImageBuild: splice all tuples via run_commands(*)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed PR #816, fix: align sandbox base tool staging, by @ColeMurray. The changes factor repeated sandbox image/bootstrap commands into local helpers, align Daytona staging with Modal/Vercel, and add focused Vercel bootstrap coverage.
Files changed: 4, +118/-45.
Critical Issues
None found.
Suggestions
None blocking.
Nitpicks
None.
Positive Feedback
- The helper extraction keeps the generated image/bootstrap command sequences easier to compare across providers without changing their behavior.
- Daytona now gets the same pinned ttyd install and OpenCode dependency staging path as the other sandbox bases.
- The new bootstrap test covers the key generated-script invariants for git credential helper installation and OpenCode dependency staging.
Verification
npm test -w @open-inspect/control-plane -- src/sandbox/providers/vercel/bootstrap.test.ts src/sandbox/providers/vercel/base-snapshot.test.ts: 2 files passed, 4 tests passed.npm run typecheck -w @open-inspect/control-plane: passed.python -m py_compile packages/modal-infra/src/images/base.py packages/daytona-infra/src/toolchain.py: passed.
Verdict
Approve.
| SANDBOX_VERSION = "daytona-v3-opencode-deps-ttyd" | ||
|
|
||
|
|
||
| def opencode_deps_staging_commands() -> tuple[str, ...]: |
There was a problem hiding this comment.
[deep review] This is the main structural regression in the PR: the change is trying to align Modal, Daytona, and Vercel base tooling, but it does that by creating provider-local copies of the same shell contract (/app/opencode-deps layout, global config seeding, credential-helper shim, ttyd pinning). That makes the code look cleaner inside each file while preserving the real drift problem across three providers. Can we make the sandbox toolchain contract canonical instead, then have each provider consume it? For example, move the shared install snippets/manifests behind a small sandbox-runtime/toolchain module or checked-in shell assets, and keep provider code responsible only for execution mechanics (sudo/image API differences). Otherwise the next OpenCode/ttyd/credential-helper bump still has to be discovered and kept in sync manually in multiple layers.
There was a problem hiding this comment.
Addressed in 8ad3e5d.
Finding: applicable for the Python providers. I moved the sandbox toolchain contract into sandbox_runtime.toolchain so Modal imports the canonical versions/install command builders directly, and Daytona loads the same module from the repo checkout passed to its snapshot builder. I did not make Vercel consume that Python module or a checked-in shell asset because bootstrap.ts is imported from Worker-bundled control-plane code and the original constraint was to avoid Node-only fs/runtime dependencies in that path. Vercel remains local execution mechanics, with the contract now covered by assertions in the bootstrap test.
| it("installs shared sandbox helpers and stages OpenCode deps", () => { | ||
| const script = buildVercelBootstrapScript(); | ||
|
|
||
| expect(script).toContain("install_git_credential_helper() {"); |
There was a problem hiding this comment.
[deep review] This test locks in the helper names and exact string fragments rather than the sandbox bootstrap contract. That makes the new provider-local abstraction harder to change without giving much confidence that the generated script remains valid or aligned with Modal/Daytona. Can we test the contract instead: the generated script installs the credential helper with the expected Python entrypoint, writes the OpenCode package manifest with the pinned version, stages /app/opencode-deps, and seeds /root/.config/opencode? Even a small parser/snapshot of the generated commands would be a stronger boundary than asserting function names.
There was a problem hiding this comment.
Addressed in 8ad3e5d.
Finding: applicable. I rewrote the Vercel bootstrap test to assert the generated sandbox contract instead of helper function names: credential-helper entrypoint and system git config, the pinned OpenCode package manifest, /app/opencode-deps staging, /root/.config/opencode seeding, and sandbox-runtime editable install presence. I also added packages/sandbox-runtime/tests/test_toolchain_contract.py for the new canonical Python contract used by Modal/Daytona.
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/daytona-infra/src/toolchain.py (1)
71-71: 📐 Maintainability & Code Quality | 🔵 TrivialPin
zodversion or add explanatory comments.The unpinned
zodinstall on line 71 is not unique to daytona-infra—it also appears identically inpackages/modal-infra/src/images/base.py:112andpackages/control-plane/src/sandbox/providers/vercel/bootstrap.ts:58. Modal-infra documents why the global install is needed ("This ensures tools can import the plugin without needing to run bun add"), but daytona-infra does not. Either add a similar justification comment to daytona-infra, or consolidate the pinned version ofzodacross all providers to prevent uncontrolled drift during bootstrap.🤖 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/daytona-infra/src/toolchain.py` at line 71, The npm install command installing zod without a pinned version can cause uncontrolled version drift across providers. In the toolchain.py file where the npm install command is defined, either add an explanatory comment (similar to the one documented in modal-infra's base.py that explains why the global install is needed for tools to import the plugin without running bun add) or update the zod package specification to include a specific version number that matches the pinned versions used consistently across other providers like modal-infra and control-plane.packages/control-plane/src/sandbox/providers/vercel/bootstrap.test.ts (1)
5-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating that helper functions are invoked.
The test verifies that
install_git_credential_helperandstage_opencode_depsfunction bodies contain expected commands, but doesn't validate that these functions are actually called in the script. Per the context snippet, these are invoked near the end of the generated script, which is critical for the toolchain to be installed.✅ Suggested addition to validate function invocations
expect(script).toContain("sudo cp -a /app/opencode-deps/. /root/.config/opencode/"); + // Verify the helper functions are actually called + expect(script).toContain("install_git_credential_helper"); + expect(script).toContain("stage_opencode_deps"); + expect(script).toContain( "sudo /usr/bin/python3.12 -m pip install --break-system-packages -e packages/sandbox-runtime" );🤖 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/control-plane/src/sandbox/providers/vercel/bootstrap.test.ts` around lines 5 - 31, The test for buildVercelBootstrapScript validates that individual commands exist in the generated script, but it doesn't verify that the helper functions install_git_credential_helper and stage_opencode_deps are actually invoked. Add additional expect statements to the test that check for the actual function calls being present in the script, ensuring these critical helper functions are being executed as part of the bootstrap process.
🤖 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.
Nitpick comments:
In `@packages/control-plane/src/sandbox/providers/vercel/bootstrap.test.ts`:
- Around line 5-31: The test for buildVercelBootstrapScript validates that
individual commands exist in the generated script, but it doesn't verify that
the helper functions install_git_credential_helper and stage_opencode_deps are
actually invoked. Add additional expect statements to the test that check for
the actual function calls being present in the script, ensuring these critical
helper functions are being executed as part of the bootstrap process.
In `@packages/daytona-infra/src/toolchain.py`:
- Line 71: The npm install command installing zod without a pinned version can
cause uncontrolled version drift across providers. In the toolchain.py file
where the npm install command is defined, either add an explanatory comment
(similar to the one documented in modal-infra's base.py that explains why the
global install is needed for tools to import the plugin without running bun add)
or update the zod package specification to include a specific version number
that matches the pinned versions used consistently across other providers like
modal-infra and control-plane.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14eeba72-a1e5-415a-8af2-e777923216eb
📒 Files selected for processing (5)
packages/control-plane/src/sandbox/providers/vercel/bootstrap.test.tspackages/daytona-infra/src/toolchain.pypackages/modal-infra/src/images/base.pypackages/sandbox-runtime/src/sandbox_runtime/toolchain.pypackages/sandbox-runtime/tests/test_toolchain_contract.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/modal-infra/src/images/base.py
1 participant
Summary
Verification
npm test -w @open-inspect/control-plane -- src/sandbox/providers/vercel/bootstrap.test.ts src/sandbox/providers/vercel/base-snapshot.test.tsnpm run typecheck -w @open-inspect/control-planepython -m py_compile packages/modal-infra/src/images/base.py packages/daytona-infra/src/toolchain.pynpx prettier --check packages/control-plane/src/sandbox/providers/vercel/bootstrap.ts packages/control-plane/src/sandbox/providers/vercel/bootstrap.test.tsNotes
pytest/python -m pytestandpython -m ruffwere unavailable in the shell environment before commit; the commit hook did runruff check --fixandruff formatfor the staged Modal Python file.Created with Open-Inspect
Summary by CodeRabbit
Tests
Refactor
Chores