fix: improve sandbox model handling and diagnostics#544
fix: improve sandbox model handling and diagnostics#544Zeyuzhao wants to merge 3 commits intoColeMurray:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR normalizes model handling across session lifecycle endpoints through validation functions, introduces structured logging to sandbox creation and API responses for improved observability, updates the Docker image cache-buster for OpenCode refresh, and adds support for a new "sandbox" integration type in the settings UI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
![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/modal-infra/src/web_api.py (1)
188-241: LGTM — structured diagnostics around sandbox creation.Start/success/error logs include the identifiers needed to correlate control-plane spawn attempts with Modal-side outcomes (session_id, sandbox_id, trace_id, request_id, repo). Payload is unchanged, and the
finallyblock still emits the uniformmodal.http_requestsummary.Minor nit (optional):
request.get("session_id")is repeated across the three log sites; hoisting it into a local near the top of the handler would DRY it up and guarantee all three logs reflect the same value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/modal-infra/src/web_api.py` around lines 188 - 241, Hoist the repeated request.get("session_id") into a local variable at the start of the handler (e.g., session_id = request.get("session_id")) and use that variable in all three log sites instead of calling request.get each time; update the session_id parameter in the "api.create_sandbox.start" and "api.create_sandbox.success" log.info calls and the "api.error" log.error call so they reference the new session_id local (the affected symbols are manager.create_sandbox, the "api.create_sandbox.start"/"api.create_sandbox.success" log.info calls, and the "api.error" log.error call).packages/control-plane/src/session/http/handlers/session-lifecycle.handler.ts (1)
164-179: LGTM — state response now returns a guaranteed-valid model.Normalizing
session.modelviagetValidModelOrDefaultand revalidatingreasoning_effortagainst the normalized model (omitting it when invalid) makes the response self-consistent even when stored DB rows contain stale/unsupported model IDs. This pairs cleanly with the matching change inSandboxLifecycleManager.resolveProviderAndModel.One thing worth considering (optional): when the stored
session.modeldiffers from the normalized one returned here, clients may overwrite their local state with the default silently. If that's undesired, consider persisting the sanitized value back to the session row (or surfacing a warning log) so the divergence doesn't persist across reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/session/http/handlers/session-lifecycle.handler.ts` around lines 164 - 179, Response normalization currently uses getValidModelOrDefault and revalidates with deps.validateReasoningEffort, but if the sanitized model differs from the stored session.model clients may silently drift; update the session row when the normalized model differs by calling the session persistence/update routine used elsewhere (or add a call to a SessionRepository.updateSession field) from the session-lifecycle handler after computing model, or alternatively emit a warning log via processLogger (or the existing logger) indicating the mismatch; reference getValidModelOrDefault, deps.validateReasoningEffort, and SandboxLifecycleManager.resolveProviderAndModel to locate the related logic and ensure the persistence/log path runs before returning the Response.json payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/control-plane/src/session/http/handlers/session-lifecycle.handler.ts`:
- Around line 164-179: Response normalization currently uses
getValidModelOrDefault and revalidates with deps.validateReasoningEffort, but if
the sanitized model differs from the stored session.model clients may silently
drift; update the session row when the normalized model differs by calling the
session persistence/update routine used elsewhere (or add a call to a
SessionRepository.updateSession field) from the session-lifecycle handler after
computing model, or alternatively emit a warning log via processLogger (or the
existing logger) indicating the mismatch; reference getValidModelOrDefault,
deps.validateReasoningEffort, and
SandboxLifecycleManager.resolveProviderAndModel to locate the related logic and
ensure the persistence/log path runs before returning the Response.json payload.
In `@packages/modal-infra/src/web_api.py`:
- Around line 188-241: Hoist the repeated request.get("session_id") into a local
variable at the start of the handler (e.g., session_id =
request.get("session_id")) and use that variable in all three log sites instead
of calling request.get each time; update the session_id parameter in the
"api.create_sandbox.start" and "api.create_sandbox.success" log.info calls and
the "api.error" log.error call so they reference the new session_id local (the
affected symbols are manager.create_sandbox, the
"api.create_sandbox.start"/"api.create_sandbox.success" log.info calls, and the
"api.error" log.error call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca8a88f9-494c-42c3-9b5b-55fded09cade
📒 Files selected for processing (7)
packages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.test.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.tspackages/modal-infra/src/images/base.pypackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/src/web_api.pypackages/web/src/app/(app)/settings/integrations/[id]/page.tsx
| if (integrationId === "github") return <GitHubIntegrationSettings />; | ||
| if (integrationId === "linear") return <LinearIntegrationSettings />; | ||
| if (integrationId === "code-server") return <CodeServerIntegrationSettings />; | ||
| if (integrationId === "sandbox") return <SandboxSettingsPage />; |
There was a problem hiding this comment.
this exists already in the main settings and would be duplicated here
2 participants
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores