Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a Modal-based deployment: adds a Modal ASGI entrypoint and Modal image for the FastAPI server, converts sandbox creation and lifecycle to Modal async APIs, rewrites the sandbox image build and project sync, and removes local Docker artifacts and CONTRIBUTING.md. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI as FastAPI (web_app / serve)
participant ModalImage as Modal Image (api_image)
participant ModalSandbox as Modal Sandbox
Client->>FastAPI: POST /runs (create)
FastAPI->>ModalImage: ensure image / start as needed
FastAPI->>ModalSandbox: await create_cua_sandbox(...)
ModalSandbox-->>FastAPI: sandbox handle
FastAPI->>ModalSandbox: await sandbox.tunnels.aio() / poll.aio()
FastAPI->>Client: stream events / return status
Client->>FastAPI: POST /runs/{id}/stop
FastAPI->>ModalSandbox: await handle.sandbox.terminate.aio()
ModalSandbox-->>FastAPI: terminated
FastAPI->>Client: final status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sandbox/image.py (1)
79-86: Update docstring to reflect async API.The docstring references
sandbox.tunnels()but callers should now useawait sandbox.tunnels.aio()to match the async pattern. Consider updating for consistency.📝 Suggested docstring update
"""Create a Modal sandbox configured for a CUA run. Returns the sandbox immediately — startup is asynchronous. - Use sandbox.tunnels() to get the status API URL. + Use ``await sandbox.tunnels.aio()`` to get the status API URL. ``extra_env`` is merged into the sandbox environment — used to propagate OTel trace context (TRACEPARENT, TRACESTATE) and OTel config vars. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sandbox/image.py` around lines 79 - 86, Update the docstring to reflect the async tunnels API: replace guidance that callers should use sandbox.tunnels() with instructions to await the async accessor using await sandbox.tunnels.aio(), and mention that startup remains asynchronous and sandbox.tunnels.aio() returns the status API URL; update any phrasing around synchronous usage to indicate the new async pattern and keep the note about extra_env merging for trace context unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sandbox/image.py`:
- Around line 79-86: Update the docstring to reflect the async tunnels API:
replace guidance that callers should use sandbox.tunnels() with instructions to
await the async accessor using await sandbox.tunnels.aio(), and mention that
startup remains asynchronous and sandbox.tunnels.aio() returns the status API
URL; update any phrasing around synchronous usage to indicate the new async
pattern and keep the note about extra_env merging for trace context unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83d4e427-19aa-4b5f-9d42-132ffbb05cb1
📒 Files selected for processing (2)
api/server.pysandbox/image.py
Replace 15+ individual add_local_dir/add_local_file calls with one add_local_dir of the project root per image, using ignore patterns to exclude .git, .venv, tests, __pycache__, etc. This follows Modal's recommended pattern from their docs and reduces the mount list from 15+ entries to 2.
Deployment is fully on Modal now. Remove Docker references from image ignore lists too.
…ores - Remove _sandbox_image_warmup function (no longer needed) - Add CUA_API_KEY + ENVIRONMENT to llm-secret for auth in production - Use ~FilePatternMatcher allowlist + explicit dir excludes for image mounts - Exclude output/, tests/, llm/, .git/, playbooks/definitions/ from images - Include only source extensions: .py .js .json .yaml .yml .toml .lock .sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/server.py (1)
132-150: Consider adding synchronization to prevent TOCTOU race conditions.With async operations, there's a time-of-check to time-of-use gap: two concurrent requests can both retrieve the same handle at line 133, both pass the
Nonecheck, and both proceed to poll/remove. While currently benign (the removal usesdict.popwith default), this pattern becomes problematic if future changes add operations like terminate to cleanup.The
InMemoryRunRegistry(per context snippet 2) has no synchronization primitives. Consider using anasyncio.Lockper run_id or a registry-level lock for atomic check-and-remove operations.Example approach using a lock
# In InMemoryRunRegistry or at module level _cleanup_locks: dict[str, asyncio.Lock] = {} async def _cleanup_finished_sandbox(run_id: str) -> bool: lock = _cleanup_locks.setdefault(run_id, asyncio.Lock()) async with lock: handle = _run_registry.get(run_id) if handle is None: return False try: exit_code = await handle.sandbox.poll.aio() except Exception: log.exception("Failed to poll sandbox for run %s", run_id) return False if exit_code is None: return False log.info("Cleaning up finished sandbox for run %s (exit code %s)", run_id, exit_code) _remove_run_registry(run_id) return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/server.py` around lines 132 - 150, The cleanup function _cleanup_finished_sandbox currently reads from _run_registry then acts on the handle, creating a TOCTOU race; fix by introducing synchronization (either an asyncio.Lock per run_id or a registry-level asyncio.Lock) so the get/poll/check/pop sequence is atomic: create a lock map (e.g., _cleanup_locks: dict[str, asyncio.Lock]) or add locking into InMemoryRunRegistry, acquire the appropriate lock before retrieving handle, await handle.sandbox.poll.aio() while holding the lock (or use a registry method that atomically pops if finished), then call _remove_run_registry(run_id) inside the lock and release; update _cleanup_finished_sandbox, InMemoryRunRegistry (or _remove_run_registry) accordingly to ensure no concurrent cleanup/terminate interleavings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/server.py`:
- Around line 132-150: The cleanup function _cleanup_finished_sandbox currently
reads from _run_registry then acts on the handle, creating a TOCTOU race; fix by
introducing synchronization (either an asyncio.Lock per run_id or a
registry-level asyncio.Lock) so the get/poll/check/pop sequence is atomic:
create a lock map (e.g., _cleanup_locks: dict[str, asyncio.Lock]) or add locking
into InMemoryRunRegistry, acquire the appropriate lock before retrieving handle,
await handle.sandbox.poll.aio() while holding the lock (or use a registry method
that atomically pops if finished), then call _remove_run_registry(run_id) inside
the lock and release; update _cleanup_finished_sandbox, InMemoryRunRegistry (or
_remove_run_registry) accordingly to ensure no concurrent cleanup/terminate
interleavings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0038ff31-daeb-424e-a709-5000da34016f
📒 Files selected for processing (7)
.dockerignore.gitignoreCONTRIBUTING.mdDockerfileapi/server.pydocker-compose.ymlsandbox/image.py
💤 Files with no reviewable changes (4)
- CONTRIBUTING.md
- .dockerignore
- docker-compose.yml
- Dockerfile
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- sandbox/image.py
- Move image and secrets to modal.App() constructor (like echo pattern) - Add uv_sync extra_options='--no-dev' to skip test dependencies - Rename FastAPI app → web_app to avoid modal_app collision - Remove redundant per-function image/secrets config - Add DEBIAN_FRONTEND=noninteractive before apt-get in sandbox image
This was referenced Mar 30, 2026
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 by CodeRabbit
New Features
Refactor
Documentation
Chores