Add Docker support for Modal sandboxes#697
Open
ColeMurray wants to merge 12 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional Docker Engine support for Modal-backed sandboxes: typed runtime settings and image profiles, Modal launch orchestration, Docker daemon supervisor and runtime integration, DB schema/profile scoping, control-plane batching and lifecycle compatibility, scheduler/profile-aware builds, web UI controls, docs, and tests. ChangesDocker Engine Support for Modal Sandboxes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ControlPlane
participant Scheduler
participant ModalInfra
participant SandboxRuntime
Client->>ControlPlane: POST /repo-images/trigger (owner/name)
ControlPlane->>ControlPlane: RepoImageBuildPlanner.plan(repo) => sandboxSettings,imageProfile
ControlPlane->>ControlPlane.DB: INSERT repo_images (image_profile)
ControlPlane->>ModalInfra: buildRepoImage(sandboxSettings, image_profile)
ModalInfra->>Scheduler: spawn build (sandbox_settings, image_profile)
Scheduler->>ModalInfra: create_build_sandbox(image_profile, runtime_settings)
ModalInfra->>SandboxRuntime: start sandbox (runtime_settings, image_profile)
SandboxRuntime->>SandboxRuntime: RuntimeServices.start_before_hooks()
SandboxRuntime->>SandboxRuntime: DockerService.start() if image_profile == "docker"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class RuntimeLaunchOptions: |
Contributor
There was a problem hiding this comment.
[deep review] This is the right concept, but it is landing inside an already broad 870-line SandboxManager, so image selection, Docker Modal kwargs, env injection, and port planning now all live in the lifecycle class. Can we move RuntimeLaunchOptions into sandbox/settings.py (or a small launch_options.py) and make SandboxManager just call build_modal_create_kwargs(...)? That keeps the manager as orchestration and stops it becoming the kitchen-sink place for every new runtime feature.
| try { | ||
| const repos = await metadataStore.getImageBuildEnabledRepos(); | ||
| return json({ repos }); | ||
| const sandboxSettings = (await resolveSandboxSettingsForRepos(env.DB, repos)).map((settings) => |
Contributor
There was a problem hiding this comment.
[deep review] This adds another copy of “resolve sandbox settings -> normalize -> derive image profile” in the route layer, while trigger-build above repeats the same derivation again. The repo-image subsystem now has an implicit build identity spread across route handlers and the scheduler response shape. Can we push this behind a repo-image build planner/store method that returns { repo, sandboxSettings, imageProfile } and use that both for scheduler listing and trigger registration? The current structure makes it too easy for a future profile-affecting setting to update one path and miss the other.
| ? (data as GlobalSettingsResponse | undefined)?.settings?.defaults | ||
| : globalData?.settings?.defaults; | ||
| const repoSettings = isGlobal ? undefined : (data as RepoSettingsResponse | undefined)?.settings; | ||
| const canEditDockerSettings = getPublicSandboxProvider() === "modal"; |
Contributor
There was a problem hiding this comment.
[deep review] This makes Docker editability depend on a client-side public provider flag, but the integration setting itself is provider-agnostic and the backend accepts/persists dockerEnabled regardless. That split is a leaky boundary: the UI can hide an existing repo/global Docker setting while buildSettingsPayload still preserves it from baseSettings, and the control plane/Modal path remains the actual authority. Can we remove the client provider gate and let the backend/sandbox provider decide whether the setting is actionable, or return capability metadata from the settings API? Otherwise this becomes scattered feature gating.
Contributor
There was a problem hiding this comment.
Summary
PR #697, "Add Docker support for Modal sandboxes" by @ColeMurray, changes 88 files with +4928/-2417. I reviewed the Docker runtime/service lifecycle, Modal launch settings, control-plane settings/image-profile plumbing, snapshot profile handling, repo-image scheduling, settings UI, and the session route split.
Critical Issues
None found.
Suggestions
None blocking. The Docker image-profile separation, normalized runtime settings boundary, and snapshot profile compatibility checks address the main correctness risks I would expect from this change.
Nitpicks
None.
Positive Feedback
The profile-aware repo image lookup and snapshot provenance handling are a good guard against restoring or reusing images with incompatible Docker support.
The runtime settings normalization is consistently applied before crossing into providers, which keeps stringly/untrusted settings from enabling privileged runtime features.
The tests added around repo-image profiles, sandbox lifecycle settings, and UI payload preservation cover the important regression paths.
Questions
None.
Verdict
Approve. I did not identify blocking correctness, security, or maintainability issues in the diff.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/control-plane/src/routes/session-prompt.ts (1)
77-86: 💤 Low valueRename shadowing
errorcallback param.The catch parameter
errorshadows theerrorhelper imported at Line 8. It's harmless here but may trip ESLintno-shadowand hurts readability. Considererr.♻️ Suggested rename
- store.touchUpdatedAt(sessionId).catch((error) => { + store.touchUpdatedAt(sessionId).catch((err) => { logger.error("session_index.touch_updated_at.background_error", { session_id: sessionId, trace_id: ctx.trace_id, request_id: ctx.request_id, - error, + error: err, }); })🤖 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/routes/session-prompt.ts` around lines 77 - 86, The catch callback passed to ctx.executionCtx?.waitUntil uses the parameter name "error" which shadows the imported helper "error"; change the catch parameter to a non-shadowing name (e.g., "err") and update its use inside the logger.error call so the thrown error is logged via the new name; locate the block around ctx.executionCtx?.waitUntil(...) that calls store.touchUpdatedAt(sessionId).catch(...) and replace the parameter and references consistently (e.g., err) to avoid shadowing.docs/IMAGE_PREBUILD.md (1)
115-128: 💤 Low valueConsider adding rationale for stopping containers.
The documentation states "stop them before the script exits" but doesn't explain why this is necessary. Adding a brief explanation (e.g., "to avoid capturing stale running container state in the snapshot") would help users understand the importance of this step.
📝 Suggested enhancement
For Docker Compose projects, keep long-running services in `.openinspect/start.sh` and use `.openinspect/setup.sh` for install, build, pull, and cache-warming work. If setup starts containers -as part of a build, stop them before the script exits, for example with `docker compose down`, so -the snapshot contains reusable image and volume data without stale running container state. +as part of a build, stop them before the script exits (e.g., with `docker compose down`). This ensures +the snapshot contains reusable image and volume data without stale running container state, which would +prevent the containers from starting cleanly when the snapshot is restored.🤖 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 `@docs/IMAGE_PREBUILD.md` around lines 115 - 128, The doc asks users to "stop them before the script exits" but omits the rationale; update the Docker-Enabled Images section to add a short explanation after the instruction to stop containers (referencing .openinspect/setup.sh and .openinspect/start.sh) clarifying that stopping containers ensures snapshots do not capture stale running container state or ephemeral processes and that volumes and images are left in a reusable state for subsequent runs.packages/sandbox-runtime/src/sandbox_runtime/docker_service.py (2)
138-160: ⚡ Quick winClean up
dockerdand the log task when readiness fails.If
wait_until_ready()raises (early exit or timeout), the spawneddockerdprocess is never terminated and leaks as an orphaned daemon — a subsequent retry can then collide on/var/run/docker.sock. Additionally, the_forward_logs()task is created without retaining a reference; per the asyncio docs the event loop only keeps a weak reference, so the task may be garbage-collected mid-execution.♻️ Proposed fix
self.process = await asyncio.create_subprocess_exec( "dockerd", f"--data-root={self.data_root}", "--host=unix:///var/run/docker.sock", "--iptables=false", "--ip6tables=false", stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, ) - asyncio.create_task(self._forward_logs()) - await self.wait_until_ready() - self.log.info("docker.ready", data_root=str(self.data_root)) + self._log_task = asyncio.create_task(self._forward_logs()) + try: + await self.wait_until_ready() + except Exception: + await self.stop() + raise + self.log.info("docker.ready", data_root=str(self.data_root))🤖 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/docker_service.py` around lines 138 - 160, The current startup creates self.process and fires off asyncio.create_task(self._forward_logs()) without keeping a reference and never shuts down the process or task if wait_until_ready() fails, causing orphaned dockerd and potential sock collisions; fix by storing the log task (e.g., self._log_task = asyncio.create_task(self._forward_logs())), and ensure any cleanup path (in wait_until_ready() on early exit or timeout and in the caller that launches dockerd) terminates the daemon and cancels the log task: if self.process exists and readiness fails, call self.process.terminate()/kill() and await self.process.wait(), cancel and await self._log_task (handling asyncio.CancelledError), then re-raise the original error; update references in wait_until_ready, the code that creates the process, and any teardown to use self._log_task for cancellation.
176-180: ⚡ Quick winExtract the shutdown timeout into a named constant.
5.0is a bare timeout literal. Define it once as a named, unit-encoded constant (e.g.STOP_TIMEOUT_SECONDS = 5.0) alongsideREADY_TIMEOUT_SECONDS.♻️ Proposed fix
STALE_DATA_DIRS = ("network", "containers", "containerd", "runtimes", "tmp") READY_TIMEOUT_SECONDS = 30.0 + STOP_TIMEOUT_SECONDS = 5.0- await asyncio.wait_for(self.process.wait(), timeout=5.0) + await asyncio.wait_for(self.process.wait(), timeout=self.STOP_TIMEOUT_SECONDS)As per coding guidelines: "Define each default timeout value exactly once as a named constant and import it everywhere rather than restating literal values."
🤖 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/docker_service.py` around lines 176 - 180, Replace the bare timeout literal 5.0 used in the asyncio.wait_for call with a named, unit-encoded module-level constant (e.g. STOP_TIMEOUT_SECONDS = 5.0) defined alongside READY_TIMEOUT_SECONDS; update the wait logic in the method that calls asyncio.wait_for(self.process.wait(), ...) to use STOP_TIMEOUT_SECONDS, and ensure any other occurrences of the same shutdown timeout use that constant (and are imported) instead of repeating the literal.
🤖 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/control-plane/src/routes/session-child-spawn.ts`:
- Around line 44-48: The code calls resolveSandboxSettings and accesses
parentSession properties without verifying parentSession exists; update the
logic around sessionStore.get(parentId) to check for a missing parentSession and
return an explicit error (or throw) early instead of proceeding: if
parentSession is null/undefined, do not call resolveSandboxSettings, set
parentUserId to null and respond with a clear error message referencing parentId
(or throw), otherwise keep the existing behavior of computing parentUserId from
parentSession.userId and calling resolveSandboxSettings(parentSession.repoOwner,
parentSession.repoName).
In `@packages/control-plane/src/routes/session-prompt.ts`:
- Around line 22-34: The handler currently calls await request.json() directly
which throws on empty or non-JSON bodies; wrap the JSON parse in a try/catch
around the request.json() call in session-prompt.ts (the code that assigns to
the body variable) and return error("invalid JSON body") (or similar 400
response) when parsing fails or the body is missing, then proceed with the
existing check for body.content (the error("content is required") branch);
ensure you catch and handle both malformed JSON and empty bodies before any
further processing.
In `@packages/control-plane/src/routes/session-ws-token.ts`:
- Around line 20-33: The handler currently calls await request.json() unguarded
so malformed JSON throws and becomes a 500; wrap the request.json() call in a
try/catch around the assignment to body (the body variable populated from
request.json()), and on a parse error return a client error (use error("Invalid
JSON", 400) or similar) instead of letting the exception propagate; keep the
existing shape check for body.userId afterwards and only proceed when parsing
succeeds.
In `@packages/modal-infra/src/images/base.py`:
- Around line 248-259: The runc binary is downloaded without integrity checks;
update the image build where .run_commands constructs the runc installation
(reference RUNC_VERSION and the runc.amd64 filename) to download to a temporary
file, compute and verify a SHA-256 against a new RUNC_SHA256 constant, and
abort/log error if the checksum does not match before making it executable and
moving it to /usr/local/bin/runc; ensure the steps replace the direct
wget->chmod->mv sequence with download->sha256sum verification->chmod->mv so the
build fails on mismatch.
In `@packages/modal-infra/src/sandbox/manager.py`:
- Around line 133-143: select_runtime_image currently loads snapshots with
modal.Image.from_registry but restore_from_snapshot uses modal.Image.from_id and
snapshot_filesystem() returns an object_id; change select_runtime_image to load
snapshots using modal.Image.from_id(snapshot_id) instead of from_registry so
snapshot_id is treated as a Modal image object ID and behavior matches
restore_from_snapshot and snapshot_filesystem; update any docstring or callers
if snapshot_id semantics differ.
In `@packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py`:
- Line 1547: Wrap the call to self.runtime_services.stop() with an
asyncio.wait_for to enforce a timeout (use the existing SIDECAR_TIMEOUT_SECONDS
constant for consistency), e.g. await
asyncio.wait_for(self.runtime_services.stop(), SIDECAR_TIMEOUT_SECONDS), and
catch asyncio.TimeoutError to log a warning/error and continue shutdown; ensure
asyncio is imported and use the same logger used elsewhere in this method (e.g.
self.process_logger or process_logger) to record the timeout event.
---
Nitpick comments:
In `@docs/IMAGE_PREBUILD.md`:
- Around line 115-128: The doc asks users to "stop them before the script exits"
but omits the rationale; update the Docker-Enabled Images section to add a short
explanation after the instruction to stop containers (referencing
.openinspect/setup.sh and .openinspect/start.sh) clarifying that stopping
containers ensures snapshots do not capture stale running container state or
ephemeral processes and that volumes and images are left in a reusable state for
subsequent runs.
In `@packages/control-plane/src/routes/session-prompt.ts`:
- Around line 77-86: The catch callback passed to ctx.executionCtx?.waitUntil
uses the parameter name "error" which shadows the imported helper "error";
change the catch parameter to a non-shadowing name (e.g., "err") and update its
use inside the logger.error call so the thrown error is logged via the new name;
locate the block around ctx.executionCtx?.waitUntil(...) that calls
store.touchUpdatedAt(sessionId).catch(...) and replace the parameter and
references consistently (e.g., err) to avoid shadowing.
In `@packages/sandbox-runtime/src/sandbox_runtime/docker_service.py`:
- Around line 138-160: The current startup creates self.process and fires off
asyncio.create_task(self._forward_logs()) without keeping a reference and never
shuts down the process or task if wait_until_ready() fails, causing orphaned
dockerd and potential sock collisions; fix by storing the log task (e.g.,
self._log_task = asyncio.create_task(self._forward_logs())), and ensure any
cleanup path (in wait_until_ready() on early exit or timeout and in the caller
that launches dockerd) terminates the daemon and cancels the log task: if
self.process exists and readiness fails, call self.process.terminate()/kill()
and await self.process.wait(), cancel and await self._log_task (handling
asyncio.CancelledError), then re-raise the original error; update references in
wait_until_ready, the code that creates the process, and any teardown to use
self._log_task for cancellation.
- Around line 176-180: Replace the bare timeout literal 5.0 used in the
asyncio.wait_for call with a named, unit-encoded module-level constant (e.g.
STOP_TIMEOUT_SECONDS = 5.0) defined alongside READY_TIMEOUT_SECONDS; update the
wait logic in the method that calls asyncio.wait_for(self.process.wait(), ...)
to use STOP_TIMEOUT_SECONDS, and ensure any other occurrences of the same
shutdown timeout use that constant (and are imported) instead of repeating the
literal.
🪄 Autofix (Beta)
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
Run ID: d0b69b91-106f-465e-9c24-ca1e203ecf93
📒 Files selected for processing (88)
.github/workflows/terraform.ymlREADME.mddocs/GETTING_STARTED.mddocs/HOW_IT_WORKS.mddocs/IMAGE_PREBUILD.mdpackages/control-plane/package.jsonpackages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/db/repo-images.test.tspackages/control-plane/src/db/repo-images.tspackages/control-plane/src/router.create-session.test.tspackages/control-plane/src/router.provider-identities.test.tspackages/control-plane/src/router.tspackages/control-plane/src/routes/provider-identities.test.tspackages/control-plane/src/routes/provider-identities.tspackages/control-plane/src/routes/repo-images.tspackages/control-plane/src/routes/session-child-spawn.tspackages/control-plane/src/routes/session-children.tspackages/control-plane/src/routes/session-create.tspackages/control-plane/src/routes/session-index.test.tspackages/control-plane/src/routes/session-index.tspackages/control-plane/src/routes/session-media-artifacts.tspackages/control-plane/src/routes/session-media-stream.tspackages/control-plane/src/routes/session-media-upload.tspackages/control-plane/src/routes/session-media.tspackages/control-plane/src/routes/session-prompt.tspackages/control-plane/src/routes/session-route.tspackages/control-plane/src/routes/session-runtime-proxy.test.tspackages/control-plane/src/routes/session-runtime-proxy.tspackages/control-plane/src/routes/session-ws-token.tspackages/control-plane/src/routes/sessions.tspackages/control-plane/src/sandbox/client.test.tspackages/control-plane/src/sandbox/client.tspackages/control-plane/src/sandbox/lifecycle/manager.test.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/sandbox/provider.tspackages/control-plane/src/sandbox/providers/daytona-provider.tspackages/control-plane/src/session/contracts.test.tspackages/control-plane/src/session/create-session-input.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/http/handlers/child-sessions.handler.test.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.test.tspackages/control-plane/src/session/identity.test.tspackages/control-plane/src/session/identity.tspackages/control-plane/src/session/integration-settings-resolution.tspackages/control-plane/src/session/repository.test.tspackages/control-plane/src/session/repository.tspackages/control-plane/src/session/runtime-client.test.tspackages/control-plane/src/session/runtime-client.tspackages/control-plane/src/session/schema.tspackages/control-plane/src/session/types.tspackages/control-plane/src/session/websocket-manager.test.tspackages/control-plane/src/storage/object-storage.tspackages/control-plane/test/integration/integration-settings.test.tspackages/control-plane/test/integration/repo-images.test.tspackages/control-plane/tsconfig.jsonpackages/control-plane/vitest.config.tspackages/control-plane/vitest.integration.config.tspackages/modal-infra/.env.examplepackages/modal-infra/README.mdpackages/modal-infra/src/app.pypackages/modal-infra/src/images/__init__.pypackages/modal-infra/src/images/base.pypackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/src/sandbox/settings.pypackages/modal-infra/src/scheduler/image_builder.pypackages/modal-infra/src/web_api.pypackages/modal-infra/tests/test_build_sandbox.pypackages/modal-infra/tests/test_image_builder_v2.pypackages/modal-infra/tests/test_sandbox_env_vars.pypackages/modal-infra/tests/test_scheduler.pypackages/modal-infra/tests/test_ttyd.pypackages/modal-infra/tests/test_tunnel_ports.pypackages/modal-infra/tests/test_web_api_create_sandbox.pypackages/sandbox-runtime/src/sandbox_runtime/docker_service.pypackages/sandbox-runtime/src/sandbox_runtime/entrypoint.pypackages/sandbox-runtime/src/sandbox_runtime/runtime_services.pypackages/sandbox-runtime/tests/test_docker_service.pypackages/shared/src/types/integrations.test.tspackages/shared/src/types/integrations.tspackages/web/src/components/settings/sandbox-settings-model.tspackages/web/src/components/settings/sandbox-settings.test.tsxpackages/web/src/components/settings/sandbox-settings.tsxterraform/README.mdterraform/d1/migrations/0022_add_repo_image_profile.sqlterraform/environments/production/modal.tfterraform/environments/production/terraform.tfvars.exampleterraform/environments/production/variables.tf
Comment on lines
+44
to
+48
| const parentSession = await sessionStore.get(parentId); | ||
| const parentUserId = parentSession?.userId ?? null; | ||
| const childSandboxSettings = parentSession | ||
| ? await resolveSandboxSettings(env.DB, parentSession.repoOwner, parentSession.repoName) | ||
| : {}; |
There was a problem hiding this comment.
Add null check for parent session.
If the parent session doesn't exist, accessing its properties on lines 46-48 will pass undefined to resolveSandboxSettings. While the spawn context fetch would eventually fail, it's better to return an explicit error early.
🛡️ Proposed fix
const parentSession = await sessionStore.get(parentId);
+if (!parentSession) {
+ return error("Parent session not found", 404);
+}
const parentUserId = parentSession?.userId ?? null;
const childSandboxSettings = parentSession
? await resolveSandboxSettings(env.DB, parentSession.repoOwner, parentSession.repoName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const parentSession = await sessionStore.get(parentId); | |
| const parentUserId = parentSession?.userId ?? null; | |
| const childSandboxSettings = parentSession | |
| ? await resolveSandboxSettings(env.DB, parentSession.repoOwner, parentSession.repoName) | |
| : {}; | |
| const parentSession = await sessionStore.get(parentId); | |
| if (!parentSession) { | |
| return error("Parent session not found", 404); | |
| } | |
| const parentUserId = parentSession?.userId ?? null; | |
| const childSandboxSettings = parentSession | |
| ? await resolveSandboxSettings(env.DB, parentSession.repoOwner, parentSession.repoName) | |
| : {}; |
🤖 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/routes/session-child-spawn.ts` around lines 44 -
48, The code calls resolveSandboxSettings and accesses parentSession properties
without verifying parentSession exists; update the logic around
sessionStore.get(parentId) to check for a missing parentSession and return an
explicit error (or throw) early instead of proceeding: if parentSession is
null/undefined, do not call resolveSandboxSettings, set parentUserId to null and
respond with a clear error message referencing parentId (or throw), otherwise
keep the existing behavior of computing parentUserId from parentSession.userId
and calling resolveSandboxSettings(parentSession.repoOwner,
parentSession.repoName).
Comment on lines
+22
to
+34
| const body = (await request.json()) as { | ||
| content: string; | ||
| authorId?: string; | ||
| source?: string; | ||
| model?: string; | ||
| reasoningEffort?: string; | ||
| attachments?: Array<{ type: string; name: string; url?: string }>; | ||
| callbackContext?: CallbackContext; | ||
| }; | ||
|
|
||
| if (!body.content) { | ||
| return error("content is required"); | ||
| } |
There was a problem hiding this comment.
Guard against malformed JSON bodies.
await request.json() throws on an empty or non-JSON body, surfacing as an unhandled error / 500 rather than a clean 400. Wrap the parse and return error(...).
🛡️ Proposed guard
- const body = (await request.json()) as {
- content: string;
- authorId?: string;
- source?: string;
- model?: string;
- reasoningEffort?: string;
- attachments?: Array<{ type: string; name: string; url?: string }>;
- callbackContext?: CallbackContext;
- };
+ let body: {
+ content: string;
+ authorId?: string;
+ source?: string;
+ model?: string;
+ reasoningEffort?: string;
+ attachments?: Array<{ type: string; name: string; url?: string }>;
+ callbackContext?: CallbackContext;
+ };
+ try {
+ body = await request.json();
+ } catch {
+ return error("Invalid JSON body");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const body = (await request.json()) as { | |
| content: string; | |
| authorId?: string; | |
| source?: string; | |
| model?: string; | |
| reasoningEffort?: string; | |
| attachments?: Array<{ type: string; name: string; url?: string }>; | |
| callbackContext?: CallbackContext; | |
| }; | |
| if (!body.content) { | |
| return error("content is required"); | |
| } | |
| let body: { | |
| content: string; | |
| authorId?: string; | |
| source?: string; | |
| model?: string; | |
| reasoningEffort?: string; | |
| attachments?: Array<{ type: string; name: string; url?: string }>; | |
| callbackContext?: CallbackContext; | |
| }; | |
| try { | |
| body = await request.json(); | |
| } catch { | |
| return error("Invalid JSON body"); | |
| } | |
| if (!body.content) { | |
| return error("content is required"); | |
| } |
🤖 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/routes/session-prompt.ts` around lines 22 - 34,
The handler currently calls await request.json() directly which throws on empty
or non-JSON bodies; wrap the JSON parse in a try/catch around the request.json()
call in session-prompt.ts (the code that assigns to the body variable) and
return error("invalid JSON body") (or similar 400 response) when parsing fails
or the body is missing, then proceed with the existing check for body.content
(the error("content is required") branch); ensure you catch and handle both
malformed JSON and empty bodies before any further processing.
Comment on lines
+20
to
+33
| const body = (await request.json()) as { | ||
| userId: string; | ||
| scmUserId?: string; | ||
| scmLogin?: string; | ||
| scmName?: string; | ||
| scmEmail?: string; | ||
| scmToken?: string; | ||
| scmTokenExpiresAt?: number; | ||
| scmRefreshToken?: string; | ||
| }; | ||
|
|
||
| if (!body.userId) { | ||
| return error("userId is required"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for a global try/catch or error mapping around handler dispatch in the router.
fd -t f 'router.ts' packages/control-plane/src | xargs -I{} sh -c 'echo "== {} =="; rg -nP -C3 "catch|handler\(|\.json\(\)" {}'Repository: ColeMurray/background-agents
Length of output: 901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the router dispatch catch block and how it maps thrown errors to responses.
sed -n '360,520p' packages/control-plane/src/router.tsRepository: ColeMurray/background-agents
Length of output: 2754
Wrap request.json() to return a 4xx on malformed JSON
packages/control-plane/src/router.ts catches any exception thrown by route.handler(...) and always responds with error("Internal server error", 500). Since this handler reads the body with an unguarded await request.json(), malformed JSON will throw and become a 500 instead of a client error.
🛡️ Proposed guard
- const body = (await request.json()) as {
- userId: string;
- scmUserId?: string;
- scmLogin?: string;
- scmName?: string;
- scmEmail?: string;
- scmToken?: string;
- scmTokenExpiresAt?: number;
- scmRefreshToken?: string;
- };
-
- if (!body.userId) {
+ let body: {
+ userId: string;
+ scmUserId?: string;
+ scmLogin?: string;
+ scmName?: string;
+ scmEmail?: string;
+ scmToken?: string;
+ scmTokenExpiresAt?: number;
+ scmRefreshToken?: string;
+ };
+ try {
+ body = await request.json();
+ } catch {
+ return error("Invalid JSON body");
+ }
+
+ if (!body.userId) {
return error("userId is required");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const body = (await request.json()) as { | |
| userId: string; | |
| scmUserId?: string; | |
| scmLogin?: string; | |
| scmName?: string; | |
| scmEmail?: string; | |
| scmToken?: string; | |
| scmTokenExpiresAt?: number; | |
| scmRefreshToken?: string; | |
| }; | |
| if (!body.userId) { | |
| return error("userId is required"); | |
| } | |
| let body: { | |
| userId: string; | |
| scmUserId?: string; | |
| scmLogin?: string; | |
| scmName?: string; | |
| scmEmail?: string; | |
| scmToken?: string; | |
| scmTokenExpiresAt?: number; | |
| scmRefreshToken?: string; | |
| }; | |
| try { | |
| body = await request.json(); | |
| } catch { | |
| return error("Invalid JSON body"); | |
| } | |
| if (!body.userId) { | |
| return error("userId is required"); | |
| } |
🤖 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/routes/session-ws-token.ts` around lines 20 - 33,
The handler currently calls await request.json() unguarded so malformed JSON
throws and becomes a 500; wrap the request.json() call in a try/catch around the
assignment to body (the body variable populated from request.json()), and on a
parse error return a client error (use error("Invalid JSON", 400) or similar)
instead of letting the exception propagate; keep the existing shape check for
body.userId afterwards and only proceed when parsing succeeds.
ColeMurray
force-pushed
the
feat/modal-docker-compose-support
branch
from
2cb61ec to
0a4d3d0
Compare
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/sandbox-runtime/src/sandbox_runtime/docker_service.py`:
- Around line 73-83: The _run_command helper can hang because it never applies a
timeout; change _run_command(self, *args: str, check: bool = True,
timeout_seconds: float = DEFAULT_TIMEOUT_SECONDS) to use asyncio.wait_for around
proc.communicate() (asyncio.wait_for(proc.communicate(), timeout_seconds)), and
on asyncio.TimeoutError ensure you terminate/kill the subprocess
(proc.kill()/await proc.wait()) and raise a clear RuntimeError indicating the
command timed out; define DEFAULT_TIMEOUT_SECONDS (or
READY_TIMEOUT_SECONDS_SECONDS per naming rule) as a module-level constant used
by wait_until_ready and other callers instead of repeating literal timeouts, and
update wait_until_ready and any call sites (e.g., where docker info is invoked)
to pass or rely on the new timeout_seconds parameter.
- Around line 138-149: The code currently starts dockerd (self.process) and
spawns asyncio.create_task(self._forward_logs()) but if await
self.wait_until_ready() raises, the dockerd process and the log task are left
running; wrap the wait_until_ready() call in a try/except/finally: capture the
task returned by asyncio.create_task(self._forward_logs()) into a variable
(e.g., self._log_task), then try: await self.wait_until_ready(); except
Exception as e: if self._log_task: self._log_task.cancel(); if self.process:
call self.process.terminate() (or kill()), await self.process.wait() and set
self.process = None; re-raise the exception; on success continue to log
docker.ready. Ensure you reference the existing symbols self.process,
self._forward_logs, wait_until_ready, and docker.ready when making the changes.
🪄 Autofix (Beta)
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
Run ID: 4b50e4a1-e19c-4c79-a8cc-c93f62adef5b
📒 Files selected for processing (61)
.github/workflows/terraform.ymlREADME.mddocs/GETTING_STARTED.mddocs/HOW_IT_WORKS.mddocs/IMAGE_PREBUILD.mdpackages/control-plane/package.jsonpackages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/db/repo-images.test.tspackages/control-plane/src/db/repo-images.tspackages/control-plane/src/routes/repo-images.tspackages/control-plane/src/sandbox/client.test.tspackages/control-plane/src/sandbox/client.tspackages/control-plane/src/sandbox/lifecycle/manager.test.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/sandbox/provider.tspackages/control-plane/src/sandbox/providers/daytona-provider.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/http/handlers/child-sessions.handler.test.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.test.tspackages/control-plane/src/session/integration-settings-resolution.tspackages/control-plane/src/session/repository.test.tspackages/control-plane/src/session/repository.tspackages/control-plane/src/session/schema.tspackages/control-plane/src/session/types.tspackages/control-plane/src/session/websocket-manager.test.tspackages/control-plane/test/integration/integration-settings.test.tspackages/control-plane/test/integration/repo-images.test.tspackages/control-plane/tsconfig.jsonpackages/control-plane/vitest.config.tspackages/control-plane/vitest.integration.config.tspackages/modal-infra/.env.examplepackages/modal-infra/README.mdpackages/modal-infra/src/app.pypackages/modal-infra/src/images/__init__.pypackages/modal-infra/src/images/base.pypackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/src/sandbox/settings.pypackages/modal-infra/src/scheduler/image_builder.pypackages/modal-infra/src/web_api.pypackages/modal-infra/tests/test_build_sandbox.pypackages/modal-infra/tests/test_image_builder_v2.pypackages/modal-infra/tests/test_sandbox_env_vars.pypackages/modal-infra/tests/test_scheduler.pypackages/modal-infra/tests/test_ttyd.pypackages/modal-infra/tests/test_tunnel_ports.pypackages/modal-infra/tests/test_web_api_create_sandbox.pypackages/sandbox-runtime/src/sandbox_runtime/docker_service.pypackages/sandbox-runtime/src/sandbox_runtime/entrypoint.pypackages/sandbox-runtime/src/sandbox_runtime/runtime_services.pypackages/sandbox-runtime/tests/test_docker_service.pypackages/shared/src/types/integrations.test.tspackages/shared/src/types/integrations.tspackages/web/src/components/settings/sandbox-settings-model.tspackages/web/src/components/settings/sandbox-settings.test.tsxpackages/web/src/components/settings/sandbox-settings.tsxterraform/README.mdterraform/d1/migrations/0022_add_repo_image_profile.sqlterraform/environments/production/modal.tfterraform/environments/production/terraform.tfvars.exampleterraform/environments/production/variables.tf
✅ Files skipped from review due to trivial changes (8)
- packages/modal-infra/.env.example
- packages/modal-infra/README.md
- packages/control-plane/src/session/http/handlers/session-lifecycle.handler.test.ts
- packages/control-plane/tsconfig.json
- packages/modal-infra/src/app.py
- packages/modal-infra/src/images/init.py
- docs/IMAGE_PREBUILD.md
- docs/HOW_IT_WORKS.md
🚧 Files skipped from review as they are similar to previous changes (48)
- packages/control-plane/src/session/websocket-manager.test.ts
- terraform/README.md
- packages/control-plane/vitest.integration.config.ts
- terraform/environments/production/terraform.tfvars.example
- .github/workflows/terraform.yml
- packages/control-plane/src/sandbox/provider.ts
- packages/modal-infra/tests/test_build_sandbox.py
- packages/control-plane/src/session/http/handlers/child-sessions.handler.test.ts
- packages/control-plane/src/session/repository.test.ts
- packages/modal-infra/tests/test_image_builder_v2.py
- packages/control-plane/src/session/types.ts
- terraform/environments/production/variables.tf
- terraform/d1/migrations/0022_add_repo_image_profile.sql
- packages/control-plane/src/sandbox/client.test.ts
- terraform/environments/production/modal.tf
- packages/control-plane/src/session/schema.ts
- packages/sandbox-runtime/src/sandbox_runtime/runtime_services.py
- packages/control-plane/test/integration/integration-settings.test.ts
- packages/shared/src/types/integrations.test.ts
- packages/control-plane/package.json
- packages/control-plane/src/db/integration-settings.ts
- packages/control-plane/src/session/integration-settings-resolution.ts
- packages/control-plane/test/integration/repo-images.test.ts
- packages/shared/src/types/integrations.ts
- packages/sandbox-runtime/tests/test_docker_service.py
- packages/modal-infra/tests/test_ttyd.py
- packages/modal-infra/src/web_api.py
- packages/web/src/components/settings/sandbox-settings-model.ts
- docs/GETTING_STARTED.md
- packages/control-plane/src/db/repo-images.test.ts
- packages/modal-infra/tests/test_web_api_create_sandbox.py
- packages/control-plane/src/routes/repo-images.ts
- packages/control-plane/vitest.config.ts
- packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py
- packages/control-plane/src/db/repo-images.ts
- packages/modal-infra/tests/test_tunnel_ports.py
- packages/web/src/components/settings/sandbox-settings.test.tsx
- packages/modal-infra/tests/test_sandbox_env_vars.py
- packages/modal-infra/src/images/base.py
- packages/modal-infra/tests/test_scheduler.py
- packages/control-plane/src/sandbox/client.ts
- packages/modal-infra/src/scheduler/image_builder.py
- packages/control-plane/src/db/integration-settings.test.ts
- packages/control-plane/src/sandbox/lifecycle/manager.ts
- packages/modal-infra/src/sandbox/settings.py
- packages/control-plane/src/sandbox/lifecycle/manager.test.ts
- packages/modal-infra/src/sandbox/manager.py
- packages/web/src/components/settings/sandbox-settings.tsx
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/modal-infra/src/sandbox/manager.py (2)
688-694: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate parameter name once
build_modal_create_kwargsis fixed.The
timeout=parameter will need to be renamed totimeout_seconds=after the fix inlaunch_options.pyis applied. This is a follow-up to the timeout naming issue in that module.🤖 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/modal-infra/src/sandbox/manager.py` around lines 688 - 694, The call building create_kwargs uses the old parameter name timeout=; update the invocation of build_modal_create_kwargs in manager.py (the create_kwargs assignment) to pass timeout_seconds=timeout_seconds instead of timeout=timeout_seconds so it matches the corrected signature in build_modal_create_kwargs/launch_options code; ensure any other callers in this file referencing timeout= are similarly updated to use timeout_seconds=.
446-452: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate parameter name once
build_modal_create_kwargsis fixed.The
timeout=parameter will need to be renamed totimeout_seconds=after the fix inlaunch_options.pyis applied. This is a follow-up to the timeout naming issue in that module.🤖 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/modal-infra/src/sandbox/manager.py` around lines 446 - 452, The call to build_modal_create_kwargs currently passes timeout=BUILD_TIMEOUT_SECONDS but that parameter will be renamed to timeout_seconds; update the argument name in the create_kwargs call to timeout_seconds=BUILD_TIMEOUT_SECONDS (keeping the same value and other args like image=select_base_image(launch_options.image_profile), secrets=[], env_vars=env_vars) so it matches the new build_modal_create_kwargs signature once launch_options.py is fixed.
🤖 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/control-plane/src/repo-images/build-planner.ts`:
- Around line 38-47: The planMany function assumes
resolveSandboxSettingsForRepos(this.db, repos) returns a settings array aligned
to repos; validate that contract by checking the returned array length and
element correspondence before using sandboxSettings[index] in planMany, e.g.,
after computing sandboxSettings (from resolveSandboxSettingsForRepos and
normalizeSandboxRuntimeSettings) ensure sandboxSettings.length === repos.length
and that no entry is undefined (or add a mapping keyed by repo id to match
entries), and if validation fails throw a clear error or handle the mismatch so
resolveSandboxImageProfile never receives undefined; update planMany (and
related callers if needed) to perform this explicit alignment/validation using
function names resolveSandboxSettingsForRepos, normalizeSandboxRuntimeSettings,
and resolveSandboxImageProfile as reference points.
In `@packages/modal-infra/src/sandbox/launch_options.py`:
- Around line 74-103: Rename the function parameter timeout to timeout_seconds
in build_modal_create_kwargs and use timeout_seconds wherever the previous
timeout variable was referenced (e.g., when setting create_kwargs["timeout"]).
Update the function signature and all callers to pass a value named/treated as
seconds (timeout_seconds) so the unit is encoded per guidelines; keep the
create_kwargs key "timeout" unchanged if the downstream API expects seconds.
In `@packages/modal-infra/src/scheduler/image_builder.py`:
- Around line 59-62: Remove the duplicated parser _parse_image_profile and
instead import and use the existing parse_sandbox_image_profile from
settings.py; update the function references where _parse_image_profile is called
(the two call sites using it) to call parse_sandbox_image_profile, ensure the
imported symbol is added to the module imports, and keep the return type as
SandboxImageProfile where annotated so the type expectations remain unchanged.
---
Duplicate comments:
In `@packages/modal-infra/src/sandbox/manager.py`:
- Around line 688-694: The call building create_kwargs uses the old parameter
name timeout=; update the invocation of build_modal_create_kwargs in manager.py
(the create_kwargs assignment) to pass timeout_seconds=timeout_seconds instead
of timeout=timeout_seconds so it matches the corrected signature in
build_modal_create_kwargs/launch_options code; ensure any other callers in this
file referencing timeout= are similarly updated to use timeout_seconds=.
- Around line 446-452: The call to build_modal_create_kwargs currently passes
timeout=BUILD_TIMEOUT_SECONDS but that parameter will be renamed to
timeout_seconds; update the argument name in the create_kwargs call to
timeout_seconds=BUILD_TIMEOUT_SECONDS (keeping the same value and other args
like image=select_base_image(launch_options.image_profile), secrets=[],
env_vars=env_vars) so it matches the new build_modal_create_kwargs signature
once launch_options.py is fixed.
🪄 Autofix (Beta)
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
Run ID: abe7f7f4-6472-42ef-be2b-662c3d831a4f
📒 Files selected for processing (26)
packages/control-plane/src/repo-images/build-planner.tspackages/control-plane/src/routes/repo-images.tspackages/control-plane/src/sandbox/client.test.tspackages/control-plane/src/sandbox/client.tspackages/control-plane/src/sandbox/lifecycle/manager.test.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/sandbox/provider.tspackages/control-plane/src/sandbox/providers/daytona-provider.test.tspackages/control-plane/src/sandbox/providers/modal-provider.test.tspackages/control-plane/src/sandbox/providers/modal-provider.tspackages/modal-infra/src/images/base.pypackages/modal-infra/src/sandbox/launch_options.pypackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/src/sandbox/settings.pypackages/modal-infra/src/scheduler/image_builder.pypackages/modal-infra/src/web_api.pypackages/modal-infra/tests/test_build_sandbox.pypackages/modal-infra/tests/test_image_builder_v2.pypackages/modal-infra/tests/test_sandbox_env_vars.pypackages/modal-infra/tests/test_web_api_create_sandbox.pypackages/sandbox-runtime/src/sandbox_runtime/docker_service.pypackages/sandbox-runtime/src/sandbox_runtime/entrypoint.pypackages/sandbox-runtime/tests/test_docker_service.pypackages/web/src/components/settings/sandbox-settings-model.tspackages/web/src/components/settings/sandbox-settings.test.tsxpackages/web/src/components/settings/sandbox-settings.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/control-plane/src/sandbox/providers/daytona-provider.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/control-plane/src/sandbox/client.test.ts
- packages/modal-infra/tests/test_build_sandbox.py
- packages/modal-infra/tests/test_web_api_create_sandbox.py
- packages/modal-infra/src/images/base.py
- packages/modal-infra/src/web_api.py
- packages/web/src/components/settings/sandbox-settings-model.ts
- packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py
- packages/control-plane/src/sandbox/lifecycle/manager.ts
- packages/control-plane/src/sandbox/provider.ts
- packages/control-plane/src/sandbox/lifecycle/manager.test.ts
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/modal-infra/src/scheduler/image_builder.py (1)
251-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix ignored
sandbox_settingsinbuild_repo_image
build_repo_imageacceptssandbox_settings, but the function body never uses it; it also doesn’t forward it toSandboxManager.create_build_sandbox, which expectssettings: SandboxRuntimeSettings | None. Either remove the unusedsandbox_settingssurface (and update docs/tests accordingly) or pass it through assettings=...when callingcreate_build_sandbox.🤖 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/modal-infra/src/scheduler/image_builder.py` around lines 251 - 307, The function build_repo_image currently accepts sandbox_settings but never uses it; update the call to SandboxManager.create_build_sandbox to forward the value as settings=sandbox_settings (ensuring the type matches SandboxRuntimeSettings | None) so the passed-in sandbox_settings is applied, and remove any dead/duplicated doc/test expectations only if you instead choose to drop the parameter; locate references to sandbox_settings in the build_repo_image signature and inside the try block around the create_build_sandbox invocation and add settings=sandbox_settings to that call (or remove the sandbox_settings parameter entirely and update docs/tests if you opt to delete it).
🧹 Nitpick comments (1)
packages/control-plane/src/repo-images/build-planner.test.ts (1)
11-24: ⚡ Quick winAdd a happy-path test for
planMany/plan.The suite only exercises the misalignment error branch. Consider covering the aligned case so the core
repo↔settings↔imageProfilemapping (andresolveSandboxImageProfileper index) is regression-protected.🤖 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/repo-images/build-planner.test.ts` around lines 11 - 24, Add a happy-path unit test for RepoImageBuildPlanner that verifies planMany (and optionally plan) correctly maps repos → settings → imageProfile: mock resolveSandboxSettingsForRepos to return an array of settings whose length matches the repos array, mock resolveSandboxImageProfile to return distinct profiles per index, instantiate RepoImageBuildPlanner, call planner.planMany([...]) and assert the returned plans contain one entry per repo with the expected imageProfile and settings (and that resolveSandboxImageProfile was called with the correct index for each repo); you can also add a small separate test for planner.plan to verify single-repo mapping using the same mocked helpers.
🤖 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.
Outside diff comments:
In `@packages/modal-infra/src/scheduler/image_builder.py`:
- Around line 251-307: The function build_repo_image currently accepts
sandbox_settings but never uses it; update the call to
SandboxManager.create_build_sandbox to forward the value as
settings=sandbox_settings (ensuring the type matches SandboxRuntimeSettings |
None) so the passed-in sandbox_settings is applied, and remove any
dead/duplicated doc/test expectations only if you instead choose to drop the
parameter; locate references to sandbox_settings in the build_repo_image
signature and inside the try block around the create_build_sandbox invocation
and add settings=sandbox_settings to that call (or remove the sandbox_settings
parameter entirely and update docs/tests if you opt to delete it).
---
Nitpick comments:
In `@packages/control-plane/src/repo-images/build-planner.test.ts`:
- Around line 11-24: Add a happy-path unit test for RepoImageBuildPlanner that
verifies planMany (and optionally plan) correctly maps repos → settings →
imageProfile: mock resolveSandboxSettingsForRepos to return an array of settings
whose length matches the repos array, mock resolveSandboxImageProfile to return
distinct profiles per index, instantiate RepoImageBuildPlanner, call
planner.planMany([...]) and assert the returned plans contain one entry per repo
with the expected imageProfile and settings (and that resolveSandboxImageProfile
was called with the correct index for each repo); you can also add a small
separate test for planner.plan to verify single-repo mapping using the same
mocked helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fef07781-3af4-42a3-aa49-9981ecb94912
📒 Files selected for processing (5)
packages/control-plane/src/repo-images/build-planner.test.tspackages/control-plane/src/repo-images/build-planner.tspackages/modal-infra/src/sandbox/launch_options.pypackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/src/scheduler/image_builder.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/control-plane/src/repo-images/build-planner.ts
- packages/modal-infra/src/sandbox/launch_options.py
- packages/modal-infra/src/sandbox/manager.py
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
ColeMurray
commented
Jun 3, 2026
| sandbox_id=self.sandbox_id, | ||
| session_id=session_id, | ||
| ) | ||
| self.runtime_services = RuntimeServices.from_env(self.log) |
Owner
Author
There was a problem hiding this comment.
this looks like it is included the Docker service regardless of whether the setting is checked. it does appear to be gated within the docker service itself, but this is probably not the ideal approach
ColeMurray
commented
Jun 3, 2026
|
|
||
| while not self.shutdown_event.is_set(): | ||
| if not await self.runtime_services.ensure_healthy(self._report_fatal_error): | ||
| self.shutdown_event.set() |
Owner
Author
There was a problem hiding this comment.
should probably log here
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Documentation
Deployment / Config
Tests