Follow-up: improve Docker daemon image-build errors#66
Open
aniketmaurya wants to merge 1 commit intomainfrom
Open
Follow-up: improve Docker daemon image-build errors#66aniketmaurya wants to merge 1 commit intomainfrom
aniketmaurya wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request enhances Docker availability error handling by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/smolvm/build.py (1)
103-108:⚠️ Potential issue | 🟠 MajorHarden Docker probes against PATH hijacking and hangs.
Both probes execute
dockervia PATH and withouttimeout. That leaves a command-injection surface (Ruff S607) and a potential indefinite hang on daemon/host misconfiguration.🔧 Suggested hardening patch
def check_docker(self) -> bool: """Check if Docker is available and the daemon is reachable.""" + docker_bin = shutil.which("docker") + if docker_bin is None: + return False try: subprocess.run( - ["docker", "info"], + [docker_bin, "info"], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + timeout=10, ) return True except (subprocess.CalledProcessError, FileNotFoundError): return False def docker_requirement_error(self) -> ImageError: """Create a helpful Docker availability error.""" @@ - if shutil.which("docker") is None: + docker_bin = shutil.which("docker") + if docker_bin is None: return ImageError(f"Docker is required to build images. {install_hint}") @@ subprocess.run( - ["docker", "info"], + [docker_bin, "info"], check=True, capture_output=True, text=True, + timeout=10, )Also applies to: 129-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/smolvm/build.py` around lines 103 - 108, Replace the PATH-invoked, potentially-hanging docker probes with an explicit resolved docker binary and a timeout: use shutil.which("docker") to get an absolute path (raise a clear error if None), then call subprocess.run([docker_path, "info"], check=True, timeout=10, stdout=..., stderr=...) instead of subprocess.run(["docker", "info"], ...); apply the same change to the second probe (the other subprocess.run([... "docker", "info" ...]) occurrence) so both use the resolved docker_path and a timeout to mitigate PATH hijacking (Ruff S607) and indefinite hangs.
🧹 Nitpick comments (1)
tests/test_build.py (1)
50-70: Add a permission/socket-denied diagnostic test to lock the third branch.The helper distinguishes three Docker failure classes; this suite currently asserts two. Add a dedicated permission-denied socket case to prevent regression in branch ordering/classification.
🧪 Suggested test addition
class TestDockerDiagnostics: @@ `@patch`("smolvm.build.subprocess.run") def test_docker_requirement_error_when_daemon_unreachable( self, mock_subprocess_run: MagicMock, tmp_path: Path ) -> None: @@ assert "Start Docker Desktop or the Docker service" in str(error) assert "Cannot connect to the Docker daemon" in str(error) + + `@patch`("smolvm.build.subprocess.run") + def test_docker_requirement_error_when_socket_permission_denied( + self, mock_subprocess_run: MagicMock, tmp_path: Path + ) -> None: + builder = ImageBuilder(cache_dir=tmp_path / "images") + mock_subprocess_run.side_effect = subprocess.CalledProcessError( + 1, + ["docker", "info"], + stderr=( + "error during connect: permission denied while trying to connect " + "to the Docker daemon socket at unix:///var/run/docker.sock" + ), + ) + + with patch("smolvm.build.shutil.which", return_value="/usr/bin/docker"): + error = builder.docker_requirement_error() + + assert "cannot access the Docker daemon socket" in str(error) + assert "docker.sock" in str(error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_build.py` around lines 50 - 70, The test suite lacks a case for the third Docker failure class (permission/socket-denied); add a new test similar to test_docker_requirement_error_when_daemon_unreachable that patches subprocess.run to raise subprocess.CalledProcessError for ImageBuilder.docker_requirement_error() with stderr indicating a permission/socket-denied message (e.g. "permission denied" or "connect: permission denied"), ensure shutil.which is patched to return a docker path, call builder.docker_requirement_error(), and assert the returned error string contains the permission/socket-denied diagnostic (e.g. "permission denied" and a guidance about socket/file permissions) so the permission branch in docker_requirement_error is covered and locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/smolvm/build.py`:
- Around line 140-151: The permission/socket error branch is being bypassed
because the broader `"error during connect"` check runs first; update the
conditional order in build.py so the permission-specific check (the `if
"permission denied" in details_lower and "docker.sock" in details_lower:` branch
that returns ImageError using permission_hint) appears before the `"error during
connect"` / daemon-unreachable branch (the block that returns ImageError using
start_hint), ensuring socket permission errors are matched to the
permission_hint; keep the same predicates and return values (using details,
details_lower, permission_hint, start_hint, and ImageError) but swap their
positions so the permission check is evaluated first.
---
Outside diff comments:
In `@src/smolvm/build.py`:
- Around line 103-108: Replace the PATH-invoked, potentially-hanging docker
probes with an explicit resolved docker binary and a timeout: use
shutil.which("docker") to get an absolute path (raise a clear error if None),
then call subprocess.run([docker_path, "info"], check=True, timeout=10,
stdout=..., stderr=...) instead of subprocess.run(["docker", "info"], ...);
apply the same change to the second probe (the other subprocess.run([...
"docker", "info" ...]) occurrence) so both use the resolved docker_path and a
timeout to mitigate PATH hijacking (Ruff S607) and indefinite hangs.
---
Nitpick comments:
In `@tests/test_build.py`:
- Around line 50-70: The test suite lacks a case for the third Docker failure
class (permission/socket-denied); add a new test similar to
test_docker_requirement_error_when_daemon_unreachable that patches
subprocess.run to raise subprocess.CalledProcessError for
ImageBuilder.docker_requirement_error() with stderr indicating a
permission/socket-denied message (e.g. "permission denied" or "connect:
permission denied"), ensure shutil.which is patched to return a docker path,
call builder.docker_requirement_error(), and assert the returned error string
contains the permission/socket-denied diagnostic (e.g. "permission denied" and a
guidance about socket/file permissions) so the permission branch in
docker_requirement_error is covered and locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63df860d-4421-47eb-b9d6-7370235134d1
📒 Files selected for processing (2)
src/smolvm/build.pytests/test_build.py
Comment on lines
+140
to
+151
| if ( | ||
| "cannot connect to the docker daemon" in details_lower | ||
| or "is the docker daemon running" in details_lower | ||
| or "error during connect" in details_lower | ||
| ): | ||
| return ImageError( | ||
| f"{start_hint} Original Docker error: {details or 'unknown error.'}" | ||
| ) | ||
| if "permission denied" in details_lower and "docker.sock" in details_lower: | ||
| return ImageError( | ||
| f"{permission_hint} Original Docker error: {details or 'unknown error.'}" | ||
| ) |
There was a problem hiding this comment.
Permission/socket errors can be misclassified as daemon-unreachable.
The "error during connect" check runs before the permission check. Common socket permission failures can include that phrase and hit the wrong branch, defeating the “distinct permission diagnostics” goal.
🔁 Suggested branch-order fix
- if (
- "cannot connect to the docker daemon" in details_lower
- or "is the docker daemon running" in details_lower
- or "error during connect" in details_lower
- ):
- return ImageError(
- f"{start_hint} Original Docker error: {details or 'unknown error.'}"
- )
if "permission denied" in details_lower and "docker.sock" in details_lower:
return ImageError(
f"{permission_hint} Original Docker error: {details or 'unknown error.'}"
)
+ if (
+ "cannot connect to the docker daemon" in details_lower
+ or "is the docker daemon running" in details_lower
+ or "error during connect" in details_lower
+ ):
+ return ImageError(
+ f"{start_hint} Original Docker error: {details or 'unknown error.'}"
+ )📝 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
| if ( | |
| "cannot connect to the docker daemon" in details_lower | |
| or "is the docker daemon running" in details_lower | |
| or "error during connect" in details_lower | |
| ): | |
| return ImageError( | |
| f"{start_hint} Original Docker error: {details or 'unknown error.'}" | |
| ) | |
| if "permission denied" in details_lower and "docker.sock" in details_lower: | |
| return ImageError( | |
| f"{permission_hint} Original Docker error: {details or 'unknown error.'}" | |
| ) | |
| if "permission denied" in details_lower and "docker.sock" in details_lower: | |
| return ImageError( | |
| f"{permission_hint} Original Docker error: {details or 'unknown error.'}" | |
| ) | |
| if ( | |
| "cannot connect to the docker daemon" in details_lower | |
| or "is the docker daemon running" in details_lower | |
| or "error during connect" in details_lower | |
| ): | |
| return ImageError( | |
| f"{start_hint} Original Docker error: {details or 'unknown error.'}" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/smolvm/build.py` around lines 140 - 151, The permission/socket error
branch is being bypassed because the broader `"error during connect"` check runs
first; update the conditional order in build.py so the permission-specific check
(the `if "permission denied" in details_lower and "docker.sock" in
details_lower:` branch that returns ImageError using permission_hint) appears
before the `"error during connect"` / daemon-unreachable branch (the block that
returns ImageError using start_hint), ensuring socket permission errors are
matched to the permission_hint; keep the same predicates and return values
(using details, details_lower, permission_hint, start_hint, and ImageError) but
swap their positions so the permission check is evaluated first.
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.
fixes #63
Motivation
Docker is required to build imageserror even when Docker is installed but the daemon is not reachable, which provides poor guidance for common failures.Description
ImageBuilder.check_dockerto usedocker infoso the check verifies the daemon is reachable as well as the client binary being present (src/smolvm/build.py).ImageBuilder.docker_requirement_error()to produce richer, actionableImageErrormessages for three cases: Docker not installed, Docker daemon unreachable (include original Docker error text), and permission/socket-access issues (src/smolvm/build.py).ImageErrorraises in the image build entry points to call the new diagnostic helper so all image-build paths (build_alpine_ssh,build_alpine_ssh_key,build_debian_ssh_key, and similar) surface the improved message (src/smolvm/build.py).tests/test_build.py).Testing
ruff check src/smolvm/build.py tests/test_build.pyand it passed.PYTHONPATH=src pytest tests/test_build.pyand all tests passed (7 passed).pytest tests/test_build.pywithoutPYTHONPATH=srcfails in this environment due to the package not being installed into the interpreter path (import error), which is unrelated to the code changes.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests