Repository navigation
fix: clean workspace after snapshot import failure - #140
Conversation
📝 WalkthroughWalkthroughDocker workspace provisioning now tracks snapshot extraction success with a ChangesDocker cleanup conditional logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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}
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 `@coding-agent/src/coding_agent/environment/docker_workspace_provider.py`:
- Around line 413-414: The docker_cleanup_needed flag is set before calling
_run_docker_setup_phase_if_configured, causing cleanup to run even when that
helper exits early during pre-subprocess validations; change the logic so
docker_cleanup_needed is set only when Docker execution actually begins (e.g.,
after the code path that invokes subprocess.run or by returning a
boolean/raising a specific signal from _run_docker_setup_phase_if_configured
indicating “Docker invoked”). Update either the call site that currently sets
docker_cleanup_needed or modify _run_docker_setup_phase_if_configured to return
an indicator (true when subprocess.run was called) and set docker_cleanup_needed
based on that, ensuring pre-validation failures
(network/timeout/secret_env_allowlist/missing env) do not trigger cleanup.
In
`@coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py`:
- Around line 878-902: The test currently lets any Exception satisfy
pytest.raises and thus can pass when fake_run raises RuntimeError; modify
fake_run to record calls (e.g., set a local flag like docker_run_called=True)
instead of raising, or have it set the flag and raise a clear sentinel if you
prefer; then call provision_cloud_binding_from_config as before inside
pytest.raises(...) for the specific snapshot-import error, and after the call
assert the docker_run_called flag is False (or assert subprocess.run was not
called via monkeypatch). Update references to fake_run,
monkeypatch.setattr(subprocess, "run", fake_run), and
provision_cloud_binding_from_config to implement and assert the call-tracking so
the test fails if cleanup was invoked.
🪄 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: 9bd2bd4e-a6f4-4304-9c8e-1925ad9ef832
📒 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
| docker_cleanup_needed = True | ||
| _run_docker_setup_phase_if_configured(provider_config, config, binding) |
There was a problem hiding this comment.
Delay docker_cleanup_needed until Docker execution is actually possible.
Line 413 sets the flag before _run_docker_setup_phase_if_configured(), but that helper still has several pre-subprocess.run() validation exits (network, timeout_seconds, secret_env_allowlist, missing env vars). Those failures will still take the Docker cleanup path even though no container phase started, and a broken/missing Docker CLI can then leave workspace_root behind again. Consider splitting setup validation from execution, or otherwise deriving this flag from “we actually invoked Docker” rather than “we finished snapshot extraction.”
🤖 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 413 - 414, The docker_cleanup_needed flag is set before calling
_run_docker_setup_phase_if_configured, causing cleanup to run even when that
helper exits early during pre-subprocess validations; change the logic so
docker_cleanup_needed is set only when Docker execution actually begins (e.g.,
after the code path that invokes subprocess.run or by returning a
boolean/raising a specific signal from _run_docker_setup_phase_if_configured
indicating “Docker invoked”). Update either the call site that currently sets
docker_cleanup_needed or modify _run_docker_setup_phase_if_configured to return
an indicator (true when subprocess.run was called) and set docker_cleanup_needed
based on that, ensuring pre-validation failures
(network/timeout/secret_env_allowlist/missing env) do not trigger cleanup.
| def fake_run( | ||
| command: list[str], **kwargs: object | ||
| ) -> subprocess.CompletedProcess[str]: | ||
| del kwargs | ||
| raise RuntimeError( | ||
| f"docker cleanup should not run before containers: {command}" | ||
| ) | ||
|
|
||
| monkeypatch.setattr(subprocess, "run", fake_run) | ||
|
|
||
| with pytest.raises(Exception): | ||
| _ = provision_cloud_binding_from_config( | ||
| _docker_config( | ||
| { | ||
| "workspace_root": str(workspace_root), | ||
| "docker_binary": "/usr/bin/docker", | ||
| } | ||
| ), | ||
| { | ||
| "kind": "docker", | ||
| "snapshot_archive_base64": "not a valid workspace archive", | ||
| }, | ||
| ) | ||
|
|
||
| assert list(workspace_root.iterdir()) == [] |
There was a problem hiding this comment.
This regression test still passes if cleanup is incorrectly invoked.
Line 888 accepts any Exception, including the RuntimeError raised by fake_run() when Docker cleanup is attempted. That means the test can go green on the exact behavior it is meant to catch. Track whether subprocess.run was called and assert it stayed false, or narrow the expected exception to the snapshot-import failure itself.
Suggested tightening
+ cleanup_attempted = False
+
def fake_run(
command: list[str], **kwargs: object
) -> subprocess.CompletedProcess[str]:
+ nonlocal cleanup_attempted
del kwargs
+ cleanup_attempted = True
raise RuntimeError(
f"docker cleanup should not run before containers: {command}"
)
monkeypatch.setattr(subprocess, "run", fake_run)
with pytest.raises(Exception):
_ = provision_cloud_binding_from_config(
_docker_config(
@@
},
)
+ assert cleanup_attempted is False
assert list(workspace_root.iterdir()) == []🧰 Tools
🪛 Ruff (0.15.12)
[warning] 888-888: Do not assert blind exception: Exception
(B017)
🤖 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 878 - 902, The test currently lets any Exception satisfy
pytest.raises and thus can pass when fake_run raises RuntimeError; modify
fake_run to record calls (e.g., set a local flag like docker_run_called=True)
instead of raising, or have it set the flag and raise a clear sentinel if you
prefer; then call provision_cloud_binding_from_config as before inside
pytest.raises(...) for the specific snapshot-import error, and after the call
assert the docker_run_called flag is False (or assert subprocess.run was not
called via monkeypatch). Update references to fake_run,
monkeypatch.setattr(subprocess, "run", fake_run), and
provision_cloud_binding_from_config to implement and assert the call-tracking so
the test fails if cleanup was invoked.
Summary
Validation
Summary by CodeRabbit
Bug Fixes
Tests