fix(sandbox-runtime): seed OpenCode global config dir to skip cold-start reify - #790
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 05a9593d261d5cd517aa62fb3398ed4c2dc594af and 5eb0bbd. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesOpenCode Global Dependency Seeding
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/in/2661131?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #790, fix(sandbox-runtime): seed OpenCode global config dir to skip cold-start reify, by @ColeMurray. Reviewed 2 changed files (+154/-7); the change extracts the OpenCode dependency staging helper and best-effort seeds the global config directory before starting OpenCode.
Critical Issues
None found.
Suggestions
None blocking. The implementation keeps the global seeding best-effort and preserves existing config when node_modules already exists, which matches the intended safety boundary.
Nitpicks
None.
Positive Feedback
- The global config directory resolution is explicit and covered for
OPENCODE_CONFIG_DIR,XDG_CONFIG_HOME, and home fallback. - The copy logic remains non-clobbering and reuses the repo-local staging path instead of duplicating file-copy behavior.
- Tests cover the important safety cases: empty global dir seeding, existing
node_modulesno-op, and missing staging no-op.
Questions
None.
Verification
- Reviewed the full PR diff with
gh pr diff 790. - Created an isolated PR worktree to avoid the dirty local
mainworktree. python -m py_compile packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py packages/sandbox-runtime/tests/test_tool_installation.pypassed.- Could not run targeted
pytestorrufflocally because those commands are not installed in this environment.
Verdict
Approve.
There was a problem hiding this comment.
Deep maintainability review: I found a contract/abstraction issue in the dependency seeding helper reuse, plus an orchestration concern in start_opencode(). No file crosses the 1k-line threshold because of this PR, but entrypoint.py is already very large, so new startup branches should be held to a high bar.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py (1)
513-531: 💤 Low valueConsider logging when skipping due to existing node_modules.
The early return at line 526 when
node_modulesexists is correct defensive behavior, but adding a debug log would improve observability when troubleshooting why global deps weren't seeded.📊 Suggested logging enhancement
config_dir = self._resolve_opencode_global_config_dir() if (config_dir / "node_modules").exists(): + self.log.debug("opencode.global_deps_skip", reason="already_populated", config_dir=str(config_dir)) return # already seeded, or a real global config — never clobber🤖 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/sandbox-runtime/src/sandbox_runtime/entrypoint.py` around lines 513 - 531, The _seed_global_opencode_deps method returns early without logging when node_modules already exists at the config_dir location. Add a debug-level log statement before the early return (in the conditional checking if config_dir / "node_modules" exists) to record that global deps seeding was skipped, including the config_dir path in the log output for better observability during troubleshooting.
🤖 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/sandbox-runtime/src/sandbox_runtime/entrypoint.py`:
- Around line 513-531: The _seed_global_opencode_deps method returns early
without logging when node_modules already exists at the config_dir location. Add
a debug-level log statement before the early return (in the conditional checking
if config_dir / "node_modules" exists) to record that global deps seeding was
skipped, including the config_dir path in the log output for better
observability during troubleshooting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e89c6d9d-4441-40e9-8c47-c1b8803b9338
📒 Files selected for processing (2)
packages/sandbox-runtime/src/sandbox_runtime/entrypoint.pypackages/sandbox-runtime/tests/test_tool_installation.py
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Address open-inspect[bot] review on #790: - _seed_global_opencode_deps seeds only a pristine global config dir: skip (debug-log opencode.global_deps_skip) when it already has node_modules OR a package.json, so our cached modules never land against a foreign manifest. - Extract _prepare_opencode_filesystem(workdir) to own tools/deps/skills/bin plus the best-effort global seed, collapsing the try/except out of start_opencode(). - Add test for the manifest-without-node_modules skip path.
05a9593 to
5eb0bbd
Compare
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
…795) ## Summary Follow-up to #790. That PR fixed the user-visible cold-start `ReadTimeout` (#767) by seeding OpenCode's **global** config dir (`~/.config/opencode`) so the first `POST /session` no longer reifies it — but it did the seeding by **copying `node_modules` at boot**, which is a multi-second cost on every sandbox start (observed as ~9–19s in the `opencode.start → opencode.global_deps_seeded` window). This moves that work to **image-build time**: bake the staged plugin tree into the global config dir once, so the runtime seed becomes a no-op. ## Change **`base.py`** — after staging `/app/opencode-deps`, also copy it into the global config dir at build time (the image fixes `HOME=/root`, so the dir is always `/root/.config/opencode`): ```dockerfile mkdir -p /root/.config/opencode cp -a /app/opencode-deps/. /root/.config/opencode/ ``` `CACHE_BUSTER` is bumped `v51 → v52-bake-opencode-global-deps` to rebuild the base image and stamp a distinct `SANDBOX_VERSION` (so we can confirm from telemetry which sandboxes run the baked image; per `manager.py:416`, repo-images/snapshots don't auto-rebuild on a bump and pick it up as they refresh). **`entrypoint.py`** — `_seed_global_opencode_deps()` is now a documented **fallback**: it skips when `node_modules` is already present (which it now is, thanks to the bake). `OpenCode`'s startup `mkdir(recursive)` won't clear a populated dir, so the baked tree survives and still avoids the reify. The seed stays for environments where the baked dir isn't present (e.g. a different `HOME`). Added timing/visibility so the boot cost is measurable (this was the other half of the ask): - `opencode.repo_deps_staged` `duration_ms` — the pre-existing copy of the same tree into the **repo's** `.opencode/` (the remaining boot cost after this change). - `opencode.global_deps_seeded` `duration_ms` — the fallback seed, when it actually runs. - `opencode.global_deps_skip` `reason=already_present|foreign_manifest` — promoted to `info` so the baked steady-state is visible each boot. ## Effect - **Fresh base-image boots:** the global seed is a no-op (`global_deps_skip reason=already_present`) → that ~9s+ copy is gone from boot. - **Repo-images / snapshots:** pick up the baked dir as they rebuild/refresh; until then the runtime fallback keeps them correct. - No runtime `CACHE_BUSTER` dependency — the bake is build-time content; nothing per-session changes. ## Testing - `ruff check` / `ruff format --check` — clean (sandbox-runtime + `base.py`). - `pytest tests/` — 360 passed. - `mypy src/` — no new errors (mypy-neutral vs `main`: 12 ↔ 12). - `python -m py_compile base.py` — valid. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced logging and performance metrics for dependency operations with duration tracking * Improved observability of dependency staging and global configuration seeding processes <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
A user's first prompt on a freshly-spawned sandbox can hard-fail with "Execution failed" when
the bridge's
POST /sessionto the in-sandbox OpenCode server times out (30 shttpx.ReadTimeout).This seeds OpenCode's global config dir (
~/.config/opencode) with the plugin dependency treewe already stage, so OpenCode's cold-start
npm installis a no-op and the first request no longerblocks on it.
Root cause (confirmed on a live sandbox)
POST /sessionruns OpenCode's per-directory bootstrap. When a plugin is configured (an OpenAI-OAuthsession deploys
codex-auth-plugin.js, and the repo may ship its own.opencode/plugins/*.ts),plugin.init()callsconfig.waitForDependencies(), which makes the request block on a forkednpm install @opencode-ai/pluginfor every directory in OpenCode's config search path.That search path includes
Global.Path.config=~/.config/opencode, which OpenCodemkdirsempty on every startup. We pre-stage
/app/opencode-depsand copy it into the repo's.opencode/(
_install_tools), but we never seed the global dir — so it has nonode_modules, OpenCode'scheckNodeModulesreifies it (a realarborist.reify()npm install, no internal timeout), and on aslow/cold install that exceeds the bridge's 30 s budget the prompt fails.
Verified on a running sandbox —
~/.config/opencode/contains:{ "dependencies": { "@opencode-ai/plugin": "1.14.41" } }plus
node_modules/and a 14Kpackage-lock.json. Thatpackage.jsonhas noname/type— itis arborist's synthesized manifest, not our staged file (
{"name":"opencode-tools","type":"module",…}),proving OpenCode reified this directory itself. (Global
npm install -glands in/usr/lib/node_modules;_install_toolstargets the repo's.opencode/— neither writes here.)This is not the previously-suspected
_install_toolsmixed-tree bug: the incident repo committedonly
.opencode/plugins/skill-audit.ts(no.opencode/package.json), so its repo.opencode/isseeded consistently and does not reify — the unseeded global dir is the culprit.
Fix
entrypoint.py:_stage_opencode_deps(deps_cache, dest_dir)._seed_global_opencode_deps()— resolves OpenCode's global config dir the way OpenCode does(
OPENCODE_CONFIG_DIR→$XDG_CONFIG_HOME/opencode→~/.config/opencode) and copies the stagedtree there only if it has no
node_modules(never clobbers a real/existing config).start_opencoderight after_install_tools, best-effort (a failure only degradesto the slower reify, so it must not crash startup).
It's a plain file copy, runs on every serving boot (fresh / repo-image / snapshot, and Daytona since
the entrypoint is shared), and is robust to
HOME/XDG_CONFIG_HOMEchanges.Why this approach
It removes the specific directory that actually reified, for every session, at zero per-session
cost. For the reported incident it is complete on its own (the repo's
.opencode/was alreadyconsistent). Considered, deliberately not in this PR (tracked as follow-ups):
POST /sessiontimeout + retry (defense in depth) — recommended next.ready" step that pays any reify off the prompt path —useful for repos that ship their own
.opencode/npm deps.Testing
test_tool_installation.py:TestResolveGlobalConfigDir—OPENCODE_CONFIG_DIRoverride,XDG_CONFIG_HOME, and~/.configfallback.
TestSeedGlobalOpencodeDeps— seeds an empty global dir; no-ops whennode_modulesalreadypresent (never clobbers); no-ops when the staging is absent.
pytest tests/— 359 passed.ruff check/ruff format --check— clean.mypy src/— no new errors (mypy-neutral vsmain).Summary by CodeRabbit
Release Notes
package*.jsonandnode_moduleswhen absent, and optionally seeding them into the standard global configuration directory when empty.