From cdd12c60235f5a5e30fe97a3f6c6626112d687ab Mon Sep 17 00:00:00 2001 From: Cole Murray Date: Sun, 7 Jun 2026 23:14:57 -0700 Subject: [PATCH 1/2] refactor: extract shared buildSessionConfig for SESSION_CONFIG 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. --- .../src/sandbox/providers/daytona-provider.ts | 13 +--- .../src/sandbox/providers/vercel/provider.ts | 11 +--- .../src/sandbox/sandbox-env.test.ts | 45 +++++++++++++ .../control-plane/src/sandbox/sandbox-env.ts | 63 +++++++++++++++++++ 4 files changed, 112 insertions(+), 20 deletions(-) create mode 100644 packages/control-plane/src/sandbox/sandbox-env.test.ts create mode 100644 packages/control-plane/src/sandbox/sandbox-env.ts diff --git a/packages/control-plane/src/sandbox/providers/daytona-provider.ts b/packages/control-plane/src/sandbox/providers/daytona-provider.ts index 1f9134abb..c91463add 100644 --- a/packages/control-plane/src/sandbox/providers/daytona-provider.ts +++ b/packages/control-plane/src/sandbox/providers/daytona-provider.ts @@ -10,6 +10,7 @@ import { createLogger } from "../../logger"; import type { SourceControlProviderName } from "../../source-control"; import type { DaytonaRestClient, DaytonaCreateSandboxParams } from "../daytona-rest-client"; import { DaytonaApiError, DaytonaNotFoundError } from "../daytona-rest-client"; +import { buildSessionConfig } from "../sandbox-env"; import { SandboxProviderError, type CreateSandboxConfig, @@ -194,17 +195,7 @@ export class DaytonaSandboxProvider implements SandboxProvider { // Start with user env vars (repo secrets), then overlay system vars const envVars: Record = { ...(config.userEnvVars ?? {}) }; - const sessionConfig: Record = { - session_id: config.sessionId, - repo_owner: config.repoOwner, - repo_name: config.repoName, - provider: config.provider, - model: config.model, - mcp_servers: config.mcpServers, - }; - if (config.branch) { - sessionConfig.branch = config.branch; - } + const sessionConfig = buildSessionConfig(config); Object.assign(envVars, { PYTHONUNBUFFERED: "1", diff --git a/packages/control-plane/src/sandbox/providers/vercel/provider.ts b/packages/control-plane/src/sandbox/providers/vercel/provider.ts index 1f6b17757..f1d3a6668 100644 --- a/packages/control-plane/src/sandbox/providers/vercel/provider.ts +++ b/packages/control-plane/src/sandbox/providers/vercel/provider.ts @@ -6,6 +6,7 @@ import { computeHmacHex, MAX_TUNNEL_PORTS, type SandboxSettings } from "@open-in import { createLogger } from "../../../logger"; import type { CorrelationContext } from "../../../logger"; import type { SourceControlProviderName } from "../../../source-control"; +import { buildSessionConfig } from "../../sandbox-env"; import { DEFAULT_SANDBOX_TIMEOUT_SECONDS, SandboxProviderError, @@ -308,15 +309,7 @@ export class VercelSandboxProvider implements SandboxProvider { } ): Promise> { const envVars: Record = { ...(config.userEnvVars ?? {}) }; - const sessionConfig: Record = { - session_id: config.sessionId, - repo_owner: config.repoOwner, - repo_name: config.repoName, - provider: config.provider, - model: config.model, - mcp_servers: config.mcpServers, - }; - if (config.branch) sessionConfig.branch = config.branch; + const sessionConfig = buildSessionConfig(config); Object.assign(envVars, { HOME: "/root", diff --git a/packages/control-plane/src/sandbox/sandbox-env.test.ts b/packages/control-plane/src/sandbox/sandbox-env.test.ts new file mode 100644 index 000000000..3f182fc5b --- /dev/null +++ b/packages/control-plane/src/sandbox/sandbox-env.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect } from "vitest"; +import { buildSessionConfig } from "./sandbox-env"; + +const baseInput = { + sessionId: "session-123", + repoOwner: "testowner", + repoName: "testrepo", + provider: "anthropic", + model: "anthropic/claude-sonnet-4-5", +}; + +describe("buildSessionConfig", () => { + it("maps provider inputs to the snake_case runtime contract", () => { + const mcpServers = [{ id: "mcp-1", name: "Tool", type: "local" as const, enabled: true }]; + + expect(buildSessionConfig({ ...baseInput, branch: "feature/x", mcpServers })).toEqual({ + session_id: "session-123", + repo_owner: "testowner", + repo_name: "testrepo", + provider: "anthropic", + model: "anthropic/claude-sonnet-4-5", + mcp_servers: mcpServers, + branch: "feature/x", + }); + }); + + it("omits branch when not provided", () => { + expect(buildSessionConfig(baseInput)).not.toHaveProperty("branch"); + }); + + it("serializes to a SESSION_CONFIG that omits undefined mcp_servers", () => { + // With no MCP servers configured, the key must not appear in the serialized + // payload — the runtime treats an absent key and an empty list identically. + const parsed = JSON.parse(JSON.stringify(buildSessionConfig(baseInput))); + + expect(parsed).toEqual({ + session_id: "session-123", + repo_owner: "testowner", + repo_name: "testrepo", + provider: "anthropic", + model: "anthropic/claude-sonnet-4-5", + }); + expect(parsed).not.toHaveProperty("mcp_servers"); + }); +}); diff --git a/packages/control-plane/src/sandbox/sandbox-env.ts b/packages/control-plane/src/sandbox/sandbox-env.ts new file mode 100644 index 000000000..b007eeff4 --- /dev/null +++ b/packages/control-plane/src/sandbox/sandbox-env.ts @@ -0,0 +1,63 @@ +import type { McpServerConfig } from "@open-inspect/shared"; + +/** + * Shared assembly for the sandbox environment contract. + * + * The runtime decodes the `SESSION_CONFIG` env var into a single canonical + * shape (see the Python `SessionConfig` in + * `packages/sandbox-runtime/src/sandbox_runtime/types.py`). Every provider used + * to hand-roll that object independently, which let fields silently diverge — + * the Daytona provider dropped `mcp_servers` entirely because its local copy + * never added the key. This module is the single source of truth for the shape + * so providers serialize it instead of reassembling ad-hoc objects. + * + * The runtime reads `session_id`, `branch`, `provider`, `model`, and + * `mcp_servers` from this payload; `repo_owner` / `repo_name` are included to + * mirror the full contract. + */ + +/** Canonical `SESSION_CONFIG` payload handed to the sandbox runtime. */ +export interface SessionConfigPayload { + session_id: string; + repo_owner: string; + repo_name: string; + provider: string; + model: string; + /** Omitted from the serialized payload when undefined. */ + mcp_servers?: McpServerConfig[]; + /** Omitted from the serialized payload when undefined. */ + branch?: string; +} + +/** Provider-agnostic inputs needed to assemble a {@link SessionConfigPayload}. */ +export interface SessionConfigInput { + sessionId: string; + repoOwner: string; + repoName: string; + provider: string; + model: string; + mcpServers?: McpServerConfig[]; + branch?: string; +} + +/** + * Build the canonical `SESSION_CONFIG` payload from provider inputs. + * + * `mcp_servers` is always set (left undefined when absent) so `JSON.stringify` + * omits it — matching how the runtime treats an absent key and an empty list + * identically. `branch` is only set when provided. + */ +export function buildSessionConfig(input: SessionConfigInput): SessionConfigPayload { + const payload: SessionConfigPayload = { + session_id: input.sessionId, + repo_owner: input.repoOwner, + repo_name: input.repoName, + provider: input.provider, + model: input.model, + mcp_servers: input.mcpServers, + }; + if (input.branch) { + payload.branch = input.branch; + } + return payload; +} From 047b1ee94551282598ca70702bc1543d40a47068 Mon Sep 17 00:00:00 2001 From: Cole Murray Date: Sun, 7 Jun 2026 23:27:26 -0700 Subject: [PATCH 2/2] refactor: route Modal restore session_config through buildSessionConfig 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. --- .../control-plane/src/sandbox/client.test.ts | 33 +++++++++++++++++++ packages/control-plane/src/sandbox/client.ts | 11 ++----- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/packages/control-plane/src/sandbox/client.test.ts b/packages/control-plane/src/sandbox/client.test.ts index 32e4f509f..dd56eed5b 100644 --- a/packages/control-plane/src/sandbox/client.test.ts +++ b/packages/control-plane/src/sandbox/client.test.ts @@ -88,4 +88,37 @@ describe("ModalClient", () => { "https://acme-prod-web--open-inspect-api-health.modal.run" ); }); + + it("routes the restore session_config through buildSessionConfig (carries mcp_servers)", async () => { + const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({ success: true, data: { sandbox_id: "sb-1" } }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }) + ); + + const client = createModalClient("secret", "acme", "prod-web"); + await client.restoreSandbox({ + snapshotImageId: "img-1", + sessionId: "session-123", + sandboxId: "sandbox-456", + sandboxAuthToken: "auth-token", + controlPlaneUrl: "https://control-plane.test", + repoOwner: "testowner", + repoName: "testrepo", + provider: "anthropic", + model: "anthropic/claude-sonnet-4-5", + mcpServers: [{ id: "mcp-1", name: "Tool", type: "local", enabled: true }], + }); + + const body = JSON.parse((fetchMock.mock.calls[0]?.[1] as RequestInit).body as string); + expect(body.session_config).toEqual({ + session_id: "session-123", + repo_owner: "testowner", + repo_name: "testrepo", + provider: "anthropic", + model: "anthropic/claude-sonnet-4-5", + mcp_servers: [{ id: "mcp-1", name: "Tool", type: "local", enabled: true }], + }); + }); }); diff --git a/packages/control-plane/src/sandbox/client.ts b/packages/control-plane/src/sandbox/client.ts index af7e95e04..342d10ecf 100644 --- a/packages/control-plane/src/sandbox/client.ts +++ b/packages/control-plane/src/sandbox/client.ts @@ -9,6 +9,7 @@ import { generateInternalToken, type SandboxSettings } from "@open-inspect/share import type { McpServerConfig } from "@open-inspect/shared"; import { createLogger } from "../logger"; import type { CorrelationContext } from "../logger"; +import { buildSessionConfig } from "./sandbox-env"; const log = createLogger("modal-client"); @@ -332,15 +333,7 @@ export class ModalClient { headers, body: JSON.stringify({ snapshot_image_id: request.snapshotImageId, - session_config: { - session_id: request.sessionId, - repo_owner: request.repoOwner, - repo_name: request.repoName, - provider: request.provider, - model: request.model, - branch: request.branch || null, - mcp_servers: request.mcpServers || null, - }, + session_config: buildSessionConfig(request), sandbox_id: request.sandboxId, control_plane_url: request.controlPlaneUrl, sandbox_auth_token: request.sandboxAuthToken,