Repository navigation
feat: add setup phase execution - #138
Merged
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 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 server-only production validation for remote_phases.setup; provider runs a restricted one-shot ChangesDocker remote_phases.setup provisioning and cleanup
Sequence DiagramsequenceDiagram
participant Client as HTTP client
participant HTTPServer as _validate_production_remote_phases
participant Provision as provision_cloud_workspace_binding
participant Setup as _run_docker_setup_phase_if_configured
participant Engine as Docker engine
participant Agent as Agent container
participant FS as Workspace FS
Client->>HTTPServer: send production config with remote_phases.setup
HTTPServer->>HTTPServer: validate network, timeout_seconds, commands, secret_env_allowlist
HTTPServer-->>Client: accept / reject
Client->>Provision: request workspace provision
Provision->>Setup: run setup phase if configured
Setup->>Engine: docker run --rm --name ws-...-setup (setup cmd + allowed env)
Engine-->>Setup: exit 0 / non-zero
Setup-->>Provision: success / raise (redacted stdout/stderr on error)
Provision->>Engine: docker run (agent container) -- only on setup success
Engine-->>Agent: agent running
Provision->>FS: workspace ready
Client->>Provision: request cleanup
Provision->>Engine: docker rm -f ws-...-setup
Provision->>Engine: docker rm -f ws-...
Engine-->>Provision: containers removed
Provision->>FS: rmdir workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py (1)
813-869: 💤 Low valueStrengthen failure-path cleanup assertion to verify container identities and order.
The current assertions only check the count of
rm -fcalls (== 2) and that norun -dwas attempted. They won't catch regressions like duplicate removal of the same container, removal in the wrong order (agent before setup), or removal of an unrelated container — all of which the neighboringtest_docker_workspace_provider_cleanup_removes_setup_and_agent_containerstest (lines 1500-1561) guards against with a precise list assertion.Consider tracking the removed container names and asserting the exact sequence, mirroring the cleanup test:
♻️ Proposed refinement
workspace_root = tmp_path / "workspaces" workspace_root.mkdir() commands: list[list[str]] = [] + removed_containers: list[str] = [] def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: del kwargs commands.append(command) if command[1:3] == ["run", "--rm"]: raise subprocess.CalledProcessError( returncode=1, cmd=command, output="", stderr="setup failed", ) if command[1:3] == ["rm", "-f"]: + removed_containers.append(command[3]) return subprocess.CompletedProcess(command, 0, stdout="", stderr="") ... assert [command[1:3] for command in commands].count(["run", "-d"]) == 0 - assert [command[1:3] for command in commands].count(["rm", "-f"]) == 2 + # Setup must be cleaned up before the (never-started) agent container. + assert removed_containers[0].endswith("-setup") + assert len(removed_containers) == 2 assert not any(workspace_root.iterdir())🤖 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 `@coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py` around lines 813 - 869, Replace the loose count-based assertion in test_docker_workspace_provider_setup_failure_does_not_start_agent_container with a strict check of which containers were removed and in what order: after calling provision_cloud_binding_from_config (used in this test), extract the container names from the captured commands list (commands where command[1:3] == ["rm", "-f"]) and assert that their sequence equals the exact expected list of removed container names (constructed from the test's container_name_prefix and expected setup/agent suffixes), while still asserting that no ["run","-d"] occurred; update the test to build the expected names consistently with how _docker_config/provision_cloud_binding_from_config derive them so the assertion verifies identity and order rather than just count.
🤖 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 `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 1071-1074: The code silently replaces non-list
secret_env_allowlist values with [], which hides misconfigurations; update the
handling around the setup_phase variable so you validate that
setup_phase.get("secret_env_allowlist") is a list before calling
_setup_phase_env — if it's None allow [], but if it's present and not a list
raise a clear ValueError (or TypeError) with the actual type and context (e.g.,
include the offending value or type and mention "secret_env_allowlist" in the
message) so callers know the policy is misconfigured; reference the
secret_env_allowlist variable, the _setup_phase_env call, and the setup_phase
source in your change.
---
Nitpick comments:
In
`@coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py`:
- Around line 813-869: Replace the loose count-based assertion in
test_docker_workspace_provider_setup_failure_does_not_start_agent_container with
a strict check of which containers were removed and in what order: after calling
provision_cloud_binding_from_config (used in this test), extract the container
names from the captured commands list (commands where command[1:3] == ["rm",
"-f"]) and assert that their sequence equals the exact expected list of removed
container names (constructed from the test's container_name_prefix and expected
setup/agent suffixes), while still asserting that no ["run","-d"] occurred;
update the test to build the expected names consistently with how
_docker_config/provision_cloud_binding_from_config derive them so the assertion
verifies identity and order rather than just count.
🪄 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 Plus
Run ID: 15c5e329-d46f-449d-ba07-b4280fe8b42b
📒 Files selected for processing (5)
coding-agent/docs/remote-sandbox-production.mdcoding-agent/src/coding_agent/environment/docker_workspace_provider.pycoding-agent/src/coding_agent/ui/http_server.pycoding-agent/tests/coding_agent/environment/test_docker_workspace_provider.pycoding-agent/tests/ui/test_http_server.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
coding-agent/src/coding_agent/environment/docker_workspace_provider.py (1)
1108-1116: ⚡ Quick winSurface setup container output when
docker runfails.This call captures stdout/stderr, but a non-zero exit or timeout will bubble up without those streams being included in the outer provisioning log. That makes setup-phase failures much harder to debug in production.
Suggested fix
- _ = subprocess.run( - setup_command, - shell=False, - capture_output=True, - text=True, - timeout=setup_timeout, - env=None, - check=True, - ) + try: + _ = subprocess.run( + setup_command, + shell=False, + capture_output=True, + text=True, + timeout=setup_timeout, + env=None, + check=True, + ) + except subprocess.CalledProcessError as exc: + if exc.stdout: + exc.add_note(f"setup phase stdout:\n{exc.stdout}") + if exc.stderr: + exc.add_note(f"setup phase stderr:\n{exc.stderr}") + raise + except subprocess.TimeoutExpired as exc: + if exc.stdout: + exc.add_note(f"setup phase stdout:\n{exc.stdout}") + if exc.stderr: + exc.add_note(f"setup phase stderr:\n{exc.stderr}") + raise🤖 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 `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py` around lines 1108 - 1116, The subprocess.run call that executes setup_command (with shell=False, capture_output=True, text=True, timeout=setup_timeout, check=True) swallows stdout/stderr on failure; update the code around that call (in docker_workspace_provider.py where setup_command is executed) to catch subprocess.CalledProcessError and subprocess.TimeoutExpired, extract and surface the captured stdout/stderr (from e.stdout/e.stderr or the CompletedProcess) into the provisioning log or include them in the raised error message, then re-raise or raise a new exception so the original failure still propagates while preserving the output for debugging.
🤖 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 `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 1121-1129: Validate each entry in secret_env_allowlist against the
existing _ENV_NAME_RE before calling os.environ.get: inside the loop that
iterates secret_env_allowlist, first ensure item is a non-empty string, then
check _ENV_NAME_RE.match(item.strip()) and raise a clear ValueError (e.g.
"remote_phases.setup.secret_env_allowlist entries must be valid environment
variable names") for invalid names; only after that read value =
os.environ.get(item.strip()) and continue populating env as before.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 1108-1116: The subprocess.run call that executes setup_command
(with shell=False, capture_output=True, text=True, timeout=setup_timeout,
check=True) swallows stdout/stderr on failure; update the code around that call
(in docker_workspace_provider.py where setup_command is executed) to catch
subprocess.CalledProcessError and subprocess.TimeoutExpired, extract and surface
the captured stdout/stderr (from e.stdout/e.stderr or the CompletedProcess) into
the provisioning log or include them in the raised error message, then re-raise
or raise a new exception so the original failure still propagates while
preserving the output for debugging.
🪄 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 Plus
Run ID: 1eb98d8e-4aca-4c2a-a1e5-94441b13a91a
📒 Files selected for processing (2)
coding-agent/src/coding_agent/environment/docker_workspace_provider.pycoding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This was referenced May 13, 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
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
Test Plan
Summary by CodeRabbit
New Features
Documentation
Tests