Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
zmanian
left a comment
There was a problem hiding this comment.
The core idea is good -- users shouldn't have to manually figure out Docker image building. The secrets key auto-write is also a nice UX improvement.
Issues to address:
1. Blocking std::process::Command in async context (must fix). build_image() uses std::process::Command::output() -- a blocking call. Docker builds can take minutes and will starve the tokio runtime thread. Use tokio::process::Command instead, which is the pattern used elsewhere in the codebase.
2. Path traversal in build_image() public API (should fix). The method accepts arbitrary &Path for the Dockerfile with no validation. Since docker build executes arbitrary RUN commands from the Dockerfile, this is a code execution vector if any future caller passes user-controlled input. At minimum add a doc comment warning the path must be trusted.
3. Hardcoded image name (minor). ensure_worker_image() hardcodes "ironclaw-worker:latest" while the rest of the sandbox system uses SANDBOX_IMAGE from config. Should read from config.
4. Bundled concerns. Docker build + secrets key auto-write are unrelated features. The PR title only mentions Docker build. Consider splitting or updating the title/description.
Thanks for the review. The secret key is from another PR #669 . I'll fix these issues. |
After confirming Docker is available, the setup wizard now checks if the ironclaw-worker:latest image exists locally. If not found, it offers to build it from Dockerfile.worker or provides manual build instructions. This fixes the job failures caused by missing Docker images when users enable the sandbox feature through the setup wizard. Fixes #459 Co-Authored-By: Claude <noreply@anthropic.com>
- Use tokio::process::Command instead of std::process::Command in build_image() to avoid blocking the async runtime during Docker builds - Add security doc warning that dockerfile_path must be trusted (Docker builds execute arbitrary RUN commands) - Use settings.sandbox.image instead of hardcoded "ironclaw-worker:latest" to respect SANDBOX_IMAGE env var configuration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
zmanian
left a comment
There was a problem hiding this comment.
Review: feat(setup): build ironclaw-worker Docker image in setup wizard
Good feature that fills a real gap -- users enabling sandbox in the wizard would hit missing image errors. The graceful fallback to manual build instructions is well-designed. A few issues to address:
Issues
1. Wrong error variant for connect_docker() failure (bug)
In ensure_worker_image(), the connect_docker() error is mapped to SetupError::Auth:
let docker = connect_docker()
.await
.map_err(|e| SetupError::Auth(e.to_string()))?;You already added SetupError::Sandbox(#[from] crate::sandbox::error::SandboxError) -- use it:
let docker = connect_docker().await?;The From impl will handle the conversion automatically. Using Auth is semantically misleading and will confuse anyone debugging setup failures.
2. ContainerRunner::new(docker, image_name.clone(), 0) -- proxy_port=0
Passing 0 for proxy_port works because build_image() doesn't use it, but it's a code smell. Consider either:
- Adding a comment explaining why 0 is safe here, or
- Adding a
ContainerRunner::for_build(docker, image)constructor that makes the intent clear
Not blocking, but worth addressing.
3. No tests (project policy)
The project's review discipline requires regression tests with fixes. At minimum, a unit test for build_image() error handling (e.g., mocking a failed docker build command) would be valuable. The ensure_worker_image() integration with the wizard is harder to test, but the build_image method itself is testable.
Minor
- The
Sandboxvariant inSetupErroruses#[from]which is good, but double-check that it doesn't conflict with any otherFromimpls that might also convertSandboxError.
What looks good
- Security doc comment on
build_image()about trusted Dockerfile paths - Build failure is non-fatal (prints manual instructions, returns Ok)
- Context directory logic is reasonable
- Two Dockerfile candidate paths is pragmatic
- Use tokio::process::Command instead of std::process::Command in build_image() to avoid blocking the async runtime during Docker builds - Add security doc warning that dockerfile_path must be trusted (Docker builds execute arbitrary RUN commands) - Use settings.sandbox.image instead of hardcoded "ironclaw-worker:latest" to respect SANDBOX_IMAGE env var configuration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix path resolution bug in build_image(): canonicalize the Dockerfile path before deriving context_dir, preventing double-resolution for nested paths like "docker/sandbox.Dockerfile" - Use SetupError::Sandbox (via From impl) instead of SetupError::Auth for connect_docker() failures - Add ContainerRunner::for_image_ops() constructor to avoid passing a bogus proxy_port=0 when only image operations are needed - Replace .unwrap_or(-1) with .map_or() to avoid unwrap in production - Add unit test for build_image() error handling on nonexistent path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(setup): build ironclaw-worker Docker image in setup wizard After confirming Docker is available, the setup wizard now checks if the ironclaw-worker:latest image exists locally. If not found, it offers to build it from Dockerfile.worker or provides manual build instructions. This fixes the job failures caused by missing Docker images when users enable the sandbox feature through the setup wizard. Fixes #459 Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandbox): address PR #714 review feedback - Use tokio::process::Command instead of std::process::Command in build_image() to avoid blocking the async runtime during Docker builds - Add security doc warning that dockerfile_path must be trusted (Docker builds execute arbitrary RUN commands) - Use settings.sandbox.image instead of hardcoded "ironclaw-worker:latest" to respect SANDBOX_IMAGE env var configuration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(sandbox): address remaining PR #714 review feedback - Fix path resolution bug in build_image(): canonicalize the Dockerfile path before deriving context_dir, preventing double-resolution for nested paths like "docker/sandbox.Dockerfile" - Use SetupError::Sandbox (via From impl) instead of SetupError::Auth for connect_docker() failures - Add ContainerRunner::for_image_ops() constructor to avoid passing a bogus proxy_port=0 when only image operations are needed - Replace .unwrap_or(-1) with .map_or() to avoid unwrap in production - Add unit test for build_image() error handling on nonexistent path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR #1757 review comments [skip-regression-check] - Stream docker build output via tokio BufReader instead of silent .output(), providing real-time build progress via tracing::info - Remove docker/sandbox.Dockerfile from candidates (wrong image, no worker entrypoint) — only Dockerfile.worker produces the correct image - Respect auto_pull_image config: attempt pull before offering local build; skip build prompt entirely for registry-style images (contain '/') - Graceful fallback when connect_docker() fails in ensure_worker_image (handles Windows check_docker/connect_docker mismatch) - Fix test to use Docker::connect_with_http_defaults() so it runs without a Docker daemon (canonicalize fails before any daemon call) - Cap stderr capture at 4KB for build error messages Regression test for build_image() error path was added in prior commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Tianer Zhou <ezhoureal@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
|
Thanks for contribution. Closing as #1757 merged. |
…ai#1757) * feat(setup): build ironclaw-worker Docker image in setup wizard After confirming Docker is available, the setup wizard now checks if the ironclaw-worker:latest image exists locally. If not found, it offers to build it from Dockerfile.worker or provides manual build instructions. This fixes the job failures caused by missing Docker images when users enable the sandbox feature through the setup wizard. Fixes nearai#459 Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandbox): address PR nearai#714 review feedback - Use tokio::process::Command instead of std::process::Command in build_image() to avoid blocking the async runtime during Docker builds - Add security doc warning that dockerfile_path must be trusted (Docker builds execute arbitrary RUN commands) - Use settings.sandbox.image instead of hardcoded "ironclaw-worker:latest" to respect SANDBOX_IMAGE env var configuration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(sandbox): address remaining PR nearai#714 review feedback - Fix path resolution bug in build_image(): canonicalize the Dockerfile path before deriving context_dir, preventing double-resolution for nested paths like "docker/sandbox.Dockerfile" - Use SetupError::Sandbox (via From impl) instead of SetupError::Auth for connect_docker() failures - Add ContainerRunner::for_image_ops() constructor to avoid passing a bogus proxy_port=0 when only image operations are needed - Replace .unwrap_or(-1) with .map_or() to avoid unwrap in production - Add unit test for build_image() error handling on nonexistent path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR nearai#1757 review comments [skip-regression-check] - Stream docker build output via tokio BufReader instead of silent .output(), providing real-time build progress via tracing::info - Remove docker/sandbox.Dockerfile from candidates (wrong image, no worker entrypoint) — only Dockerfile.worker produces the correct image - Respect auto_pull_image config: attempt pull before offering local build; skip build prompt entirely for registry-style images (contain '/') - Graceful fallback when connect_docker() fails in ensure_worker_image (handles Windows check_docker/connect_docker mismatch) - Fix test to use Docker::connect_with_http_defaults() so it runs without a Docker daemon (canonicalize fails before any daemon call) - Cap stderr capture at 4KB for build error messages Regression test for build_image() error path was added in prior commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Tianer Zhou <ezhoureal@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
* feat(setup): build ironclaw-worker Docker image in setup wizard After confirming Docker is available, the setup wizard now checks if the ironclaw-worker:latest image exists locally. If not found, it offers to build it from Dockerfile.worker or provides manual build instructions. This fixes the job failures caused by missing Docker images when users enable the sandbox feature through the setup wizard. Fixes #459 Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandbox): address PR #714 review feedback - Use tokio::process::Command instead of std::process::Command in build_image() to avoid blocking the async runtime during Docker builds - Add security doc warning that dockerfile_path must be trusted (Docker builds execute arbitrary RUN commands) - Use settings.sandbox.image instead of hardcoded "ironclaw-worker:latest" to respect SANDBOX_IMAGE env var configuration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(sandbox): address remaining PR #714 review feedback - Fix path resolution bug in build_image(): canonicalize the Dockerfile path before deriving context_dir, preventing double-resolution for nested paths like "docker/sandbox.Dockerfile" - Use SetupError::Sandbox (via From impl) instead of SetupError::Auth for connect_docker() failures - Add ContainerRunner::for_image_ops() constructor to avoid passing a bogus proxy_port=0 when only image operations are needed - Replace .unwrap_or(-1) with .map_or() to avoid unwrap in production - Add unit test for build_image() error handling on nonexistent path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR #1757 review comments [skip-regression-check] - Stream docker build output via tokio BufReader instead of silent .output(), providing real-time build progress via tracing::info - Remove docker/sandbox.Dockerfile from candidates (wrong image, no worker entrypoint) — only Dockerfile.worker produces the correct image - Respect auto_pull_image config: attempt pull before offering local build; skip build prompt entirely for registry-style images (contain '/') - Graceful fallback when connect_docker() fails in ensure_worker_image (handles Windows check_docker/connect_docker mismatch) - Fix test to use Docker::connect_with_http_defaults() so it runs without a Docker daemon (canonicalize fails before any daemon call) - Cap stderr capture at 4KB for build error messages Regression test for build_image() error path was added in prior commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Tianer Zhou <ezhoureal@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
…ai#1757) * feat(setup): build ironclaw-worker Docker image in setup wizard After confirming Docker is available, the setup wizard now checks if the ironclaw-worker:latest image exists locally. If not found, it offers to build it from Dockerfile.worker or provides manual build instructions. This fixes the job failures caused by missing Docker images when users enable the sandbox feature through the setup wizard. Fixes nearai#459 Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandbox): address PR nearai#714 review feedback - Use tokio::process::Command instead of std::process::Command in build_image() to avoid blocking the async runtime during Docker builds - Add security doc warning that dockerfile_path must be trusted (Docker builds execute arbitrary RUN commands) - Use settings.sandbox.image instead of hardcoded "ironclaw-worker:latest" to respect SANDBOX_IMAGE env var configuration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(sandbox): address remaining PR nearai#714 review feedback - Fix path resolution bug in build_image(): canonicalize the Dockerfile path before deriving context_dir, preventing double-resolution for nested paths like "docker/sandbox.Dockerfile" - Use SetupError::Sandbox (via From impl) instead of SetupError::Auth for connect_docker() failures - Add ContainerRunner::for_image_ops() constructor to avoid passing a bogus proxy_port=0 when only image operations are needed - Replace .unwrap_or(-1) with .map_or() to avoid unwrap in production - Add unit test for build_image() error handling on nonexistent path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR nearai#1757 review comments [skip-regression-check] - Stream docker build output via tokio BufReader instead of silent .output(), providing real-time build progress via tracing::info - Remove docker/sandbox.Dockerfile from candidates (wrong image, no worker entrypoint) — only Dockerfile.worker produces the correct image - Respect auto_pull_image config: attempt pull before offering local build; skip build prompt entirely for registry-style images (contain '/') - Graceful fallback when connect_docker() fails in ensure_worker_image (handles Windows check_docker/connect_docker mismatch) - Fix test to use Docker::connect_with_http_defaults() so it runs without a Docker daemon (canonicalize fails before any daemon call) - Cap stderr capture at 4KB for build error messages Regression test for build_image() error path was added in prior commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Tianer Zhou <ezhoureal@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
Summary
After confirming Docker is available, the setup wizard now checks if the
ironclaw-worker:latestimage exists locally. If not found, it offers to build it fromDockerfile.workeror provides manual build instructions.This fixes the job failures caused by missing Docker images when users enable the sandbox feature through the setup wizard.
Changes
src/sandbox/container.rs: Addedbuild_image()method to build Docker images from Dockerfilessrc/setup/wizard.rs:ensure_worker_image()methodSandboxerror variant toSetupErrorstep_docker_sandbox()write_bootstrap_envTesting
cargo fmt,cargo clippy(zero warnings)Fixes #459
🤖 Generated with Claude Code