Repository navigation
fix: avoid docker cleanup before container start - #141
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extends Docker workspace provisioning with optional callback-based tracking of whether Docker is invoked during the remote setup phase. The callback enables conditional cleanup behavior when setup validation fails before any containers are started. ChangesSetup-phase Docker invocation tracking callback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 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
🤖 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/tests/coding_agent/environment/test_docker_workspace_provider.py`:
- Around line 931-933: The pytest.raises assertion uses an unescaped regex which
lets dots match any char; update the match argument in the pytest.raises call to
use a raw string and escape the literal dots so the pattern is exact (e.g.,
r'remote_phases\.setup\.network must be "none" or "bridge"') so the test only
passes for the precise error message referenced in the
test_docker_workspace_provider.py assertion.
🪄 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: 1357d8f2-38ac-4abb-ab43-ad76e6a2a16e
📒 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
Comment on lines
+931
to
+933
| with pytest.raises( | ||
| ValueError, match='remote_phases.setup.network must be "none" or "bridge"' | ||
| ): |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the regex-pattern lint warning is resolved for this file.
uv run ruff check coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py --select RUF043 -nRepository: delete-cloud/Coding-Agent-Loop
Length of output: 117
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and read the relevant lines
cat -n coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py | sed -n '920,940p'Repository: delete-cloud/Coding-Agent-Loop
Length of output: 963
🏁 Script executed:
#!/bin/bash
# Search for the ValueError with this message pattern
rg -n 'remote_phases.*setup.*network must be' coding-agent/Repository: delete-cloud/Coding-Agent-Loop
Length of output: 643
🏁 Script executed:
#!/bin/bash
# Get more context around where the ValueError is raised
cat -n coding-agent/src/coding_agent/environment/docker_workspace_provider.py | sed -n '1080,1090p'Repository: delete-cloud/Coding-Agent-Loop
Length of output: 603
🏁 Script executed:
#!/bin/bash
# Check if ruff has RUF043 rule and what it does
python3 <<'EOF'
# Let's verify pytest.raises match behavior
import re
pattern = 'remote_phases.setup.network must be "none" or "bridge"'
test_message = 'remote_phases.setup.network must be "none" or "bridge"'
test_message_bad = 'remote_phasesXsetupXnetwork must be "none" or "bridge"'
# This is how pytest.raises match works - it uses re.search
try:
if re.search(pattern, test_message):
print("✓ Pattern matches correct message")
else:
print("✗ Pattern does NOT match correct message")
except Exception as e:
print(f"Error: {e}")
try:
if re.search(pattern, test_message_bad):
print("✗ Pattern INCORRECTLY matches bad message (unescaped dots are the problem)")
else:
print("✓ Pattern does NOT match bad message")
except Exception as e:
print(f"Error: {e}")
# Now test with escaped version
escaped_pattern = r'remote_phases\.setup\.network must be "none" or "bridge"'
print("\nWith escaped pattern:")
try:
if re.search(escaped_pattern, test_message):
print("✓ Escaped pattern matches correct message")
else:
print("✗ Escaped pattern does NOT match correct message")
except Exception as e:
print(f"Error: {e}")
try:
if re.search(escaped_pattern, test_message_bad):
print("✗ Escaped pattern INCORRECTLY matches bad message")
else:
print("✓ Escaped pattern does NOT match bad message (correct behavior)")
except Exception as e:
print(f"Error: {e}")
EOFRepository: delete-cloud/Coding-Agent-Loop
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Search for other pytest.raises with match parameter in this test file
rg -n "pytest.raises.*match=" coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.pyRepository: delete-cloud/Coding-Agent-Loop
Length of output: 522
🏁 Script executed:
#!/bin/bash
python3 <<'EOF'
import re
patterns = [
("docker exec command timed out after 1s", "docker exec command timed out after 1s"),
("docker exec command timed out after 1s", "docker execXcommand timed out after 1s"), # unescaped dots test
("Path is outside docker workspace: /etc", "Path is outside docker workspace: /etc"),
("Path is outside docker workspace: /etc", "PathXisXoutsideXdockerXworkspaceXXXetc"), # unescaped dots test
]
for pattern, test_msg in patterns:
if re.search(pattern, test_msg):
status = "✓ matches"
else:
status = "✗ no match"
print(f"{status}: '{pattern}' on '{test_msg}'")
EOFRepository: delete-cloud/Coding-Agent-Loop
Length of output: 460
🏁 Script executed:
#!/bin/bash
# Check the exact behavior with the problematic pattern
python3 <<'EOF'
import re
# The actual pattern from line 932
pattern = 'remote_phases.setup.network must be "none" or "bridge"'
# Test cases
test_cases = [
('remote_phases.setup.network must be "none" or "bridge"', "Correct message"),
('remote_phasesXsetupXnetwork must be "none" or "bridge"', "Incorrect - dots unescaped match any char"),
]
print("Current UNESCAPED pattern behavior:")
for msg, description in test_cases:
match = re.search(pattern, msg)
result = "MATCHES" if match else "no match"
print(f" {result}: {description}")
print("\n" + "="*70)
# The suggested fix
escaped_pattern = r'remote_phases\.setup\.network must be "none" or "bridge"'
print("\nSuggested ESCAPED pattern behavior:")
for msg, description in test_cases:
match = re.search(escaped_pattern, msg)
result = "MATCHES" if match else "no match"
print(f" {result}: {description}")
EOFRepository: delete-cloud/Coding-Agent-Loop
Length of output: 380
Escape regex metacharacters in pytest.raises(match=...) to prevent unintended matches.
The unescaped dots in the pattern will match any character, allowing the test to pass on incorrect error messages (e.g., 'remote_phasesXsetupXnetwork...'). Use a raw string with escaped dots for precision.
Suggested fix
with pytest.raises(
- ValueError, match='remote_phases.setup.network must be "none" or "bridge"'
+ ValueError,
+ match=r'remote_phases\.setup\.network must be "none" or "bridge"',
):📝 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
| with pytest.raises( | |
| ValueError, match='remote_phases.setup.network must be "none" or "bridge"' | |
| ): | |
| with pytest.raises( | |
| ValueError, | |
| match=r'remote_phases\.setup\.network must be "none" or "bridge"', | |
| ): |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 932-932: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 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 931 - 933, The pytest.raises assertion uses an unescaped regex
which lets dots match any char; update the match argument in the pytest.raises
call to use a raw string and escape the literal dots so the pattern is exact
(e.g., r'remote_phases\.setup\.network must be "none" or "bridge"') so the test
only passes for the precise error message referenced in the
test_docker_workspace_provider.py assertion.
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
Validation
Summary by CodeRabbit
Bug Fixes
Tests