refactor: extract shared buildSessionConfig to delete the SESSION_CONFIG drift point#712
Conversation
Every sandbox provider hand-rolled the SESSION_CONFIG payload, which let the shape drift per provider -- the root cause of the Daytona mcp_servers drop fixed in #711. Extract a single typed builder (sandbox/sandbox-env.ts) owning the canonical contract (mirrors the runtime's Python SessionConfig) and have the Daytona and Vercel providers call it instead of assembling ad-hoc objects. Behavior is unchanged: the existing provider tests pass as-is, proving the serialized payload is identical. The drift point is removed so the next provider cannot silently reintroduce the same class of bug.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR centralizes SESSION_CONFIG assembly by adding types and buildSessionConfig, tests that builder, updates ModalClient.restoreSandbox to use it, and migrates Daytona and Vercel providers to call the shared builder instead of constructing session config inline. ChangesSession Config Builder Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
![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 #712, refactor: extract shared buildSessionConfig to delete the SESSION_CONFIG drift point, by @ColeMurray. The change centralizes SESSION_CONFIG assembly for Daytona and Vercel into buildSessionConfig, preserving the existing serialized shape while removing duplicated provider-local construction.
Files changed: 4, additions/deletions: +112/-20.
Critical Issues
None found.
Suggestions
None blocking. The helper intentionally preserves current Daytona/Vercel behavior rather than expanding to every optional Python SessionConfig field, which is appropriate for this refactor scope.
Nitpicks
None.
Positive Feedback
- The shared builder removes the exact drift point that caused the previous provider mismatch.
- Tests cover snake_case mapping, optional branch omission, and JSON serialization behavior for absent MCP servers.
- Existing provider tests continue to assert the end-to-end environment payloads.
Verification
npm test -w @open-inspect/control-plane -- sandbox-env providers/daytona-provider.test.ts providers/vercel/provider.test.ts: 44 tests passed.npm run typecheck -w @open-inspect/control-plane: passed.
Questions
None.
Verdict
Approve.
Addresses deep-review feedback on #712: ModalClient.restoreSandbox hand-rolled the same snake_case session config object, leaving a third drift point the builder did not own. Route it through buildSessionConfig so a new SESSION_CONFIG field cannot be silently missed on the Modal restore path. Behavior-preserving: Modal decodes session_config into the Pydantic SessionConfig (branch/mcp_servers default to None), so the builder omitting absent keys is equivalent to the prior explicit nulls. Adds a regression test asserting restore carries the full session_config.
1 participant
Summary
Responds to the deep-review comment on #711: the one-line fix patched the Daytona omission but preserved the design flaw that caused it — every provider still hand-rolls the
SESSION_CONFIGshape, so the next provider can silently drop a field again.This makes the contract explicit in a single shared typed builder and has the providers call it instead of assembling ad-hoc objects, deleting the drift point rather than patching one more provider-local copy.
Builds on #711 (already merged to
main).What changed
packages/control-plane/src/sandbox/sandbox-env.ts— exportsSessionConfigPayload(the canonical contract, mirroring the runtime's PythonSessionConfiginsandbox-runtime/.../types.py) andbuildSessionConfig(input), the single source of truth for the serialized shape.providers/daytona-provider.ts/providers/vercel/provider.ts— bothbuildEnvVarsmethods now callbuildSessionConfig(config)instead of each hand-rolling the object. Net −20 lines of duplicated assembly.The
mcp_serverskey is built aspresent-but-undefinedsoJSON.stringifyomits it when absent — the runtime treats an absent key and an empty list identically (session_config.get("mcp_servers") or []).Why this is the real fix
The Daytona bug existed because Daytona's local copy of the
SESSION_CONFIGobject simply never addedmcp_servers(and itsRecord<string, string>type even prevented it). With a shared builder there is exactly one place the shape is defined, so a provider cannot diverge — the entire class of bug is removed, not just this instance.Behavior preservation
This is a pure refactor. The existing provider tests pass unchanged (Daytona 28, Vercel 13) — including the
mcp_serversassertions — which proves the serializedSESSION_CONFIGis identical before and after. Added 3 focused unit tests forbuildSessionConfigitself.tsc --noEmit, ESLint, Prettier: cleanScope / notes
SessionConfigPydantic model (not an ad-hoc object), so it was never a drift source.sandbox-env.tsis imported only by the two providers (per-session spawn). The Vercel base-snapshot source hash invercel.tfcoversproviders/vercel/{bootstrap,base-snapshot,client}.tsbut notproviders/vercel/provider.ts, soSESSION_CONFIGassembly is not part of the base image — thepaths=()array does not need updating. (Re: theterraform-source-hash-pathsgotcha.)Follow-ups (deliberately out of scope to keep this reviewable)
Per the post-merge review, the same
sandbox-env.tsis the natural home for the next de-duplication steps (V6): a sharedbuildSystemEnv, and folding the verbatim-duplicatedresolveTunnelPorts/deriveCodeServerPassword/buildTagshelpers into canonical homes. Left for separate focused PRs.Summary by CodeRabbit
Refactor
Bug Fixes
Tests