feat(sandbox): add ark sandbox#17
Conversation
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (42)
📝 WalkthroughWalkthroughThis PR introduces the complete Changesark sandbox Docker isolation
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as ark-cli
participant Config as SandboxConfig
participant Engine as SandboxEngine
participant Docker as docker
participant Container
User->>CLI: ark sandbox create --slug task_slug
CLI->>Config: load_or_default()
Config->>Config: validate() + apply overrides
CLI->>Engine: select_engine() for Docker
Engine->>Docker: docker_info_ok() availability
Docker-->>Engine: daemon OK
CLI->>Engine: create(&spec) with image + mounts
Engine->>Docker: run_docker(pull image)
Docker-->>Engine: image ready
Engine->>Docker: run_docker(docker run detached)
Docker-->>Container: container created + running
Engine->>Container: rewrite /workspace/.git gitdir
Container-->>Engine: gitdir rewritten
Engine-->>CLI: SandboxHandle (container/volume/slug/branch)
CLI->>User: summary: created slug in container_name
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new ark sandbox feature that runs a task’s worktree inside an isolated, persistent Docker container, including configuration, CLI wiring, and a Docker image publishing workflow.
Changes:
- Introduces a sandbox engine abstraction in
ark-corewith a v1 Docker backend and a single sanctioned Docker spawn site (io::docker). - Adds sandbox command implementations (
create,enter,rm,list,warmup) plus resolution logic (focus/slug, worktree discovery, git/common-dir mounts, env passthrough). - Adds user-facing configuration defaults/docs and a CI workflow + Dockerfile for publishing
ghcr.io/<owner>/ark-sandbox:<ark-version>.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/ark/config.toml | Adds [sandbox] config defaults and documentation (engine, image override, env passthrough, host config sharing). |
| sandbox/Dockerfile | Defines the published ark-sandbox image contents (tools, user, workspace, git safe.directory). |
| .github/workflows/sandbox-image.yml | Builds and publishes the sandbox image to GHCR on release tags / manual dispatch. |
| crates/ark-cli/src/main.rs | Adds ark sandbox CLI surface (subcommands/flags) and dispatch into ark-core. |
| crates/ark-core/src/lib.rs | Exposes sandbox options/summaries and sandbox command entrypoints from ark-core. |
| crates/ark-core/src/error.rs | Adds typed errors for Docker spawning, backend availability, sandbox lifecycle, and agent platform selection. |
| crates/ark-core/src/io/mod.rs | Registers io::docker as the single sanctioned Command::new("docker") site. |
| crates/ark-core/src/io/docker.rs | Implements captured + interactive Docker invocation helpers and a daemon availability probe. |
| crates/ark-core/src/state/mod.rs | Re-exports new checkout helper for focus/slug resolution. |
| crates/ark-core/src/state/checkout/mod.rs | Re-exports resolve_focus_slug from checkout I/O. |
| crates/ark-core/src/state/checkout/io.rs | Adds resolve_focus_slug helper and tests for slug-vs-focus precedence and NoFocus behavior. |
| crates/ark-core/src/commands/sandbox/mod.rs | Defines sandbox option/summary types and module structure for the sandbox feature. |
| crates/ark-core/src/commands/sandbox/config.rs | Loads/validates sandbox configuration and default image/engine behavior. |
| crates/ark-core/src/commands/sandbox/engine.rs | Defines the SandboxEngine trait and shared types (SandboxSpec, RemoveOpts, etc.). |
| crates/ark-core/src/commands/sandbox/engines/docker.rs | Implements the Docker-backed sandbox engine: pull/run/exec/list/remove and gitdir handling. |
| crates/ark-core/src/commands/sandbox/resolve.rs | Resolves CLI/options into a concrete SandboxSpec (worktree, git mounts, env passthrough, naming). |
| crates/ark-core/src/commands/sandbox/create.rs | Implements sandbox_create flow (probe, resolve, pull/run, optional recreate). |
| crates/ark-core/src/commands/sandbox/enter.rs | Implements entering a sandbox (shell/agent/platform selection, interactive exec). |
| crates/ark-core/src/commands/sandbox/list.rs | Implements listing sandboxes from the backend. |
| crates/ark-core/src/commands/sandbox/rm.rs | Implements sandbox teardown with optional config-volume retention. |
| crates/ark-core/src/commands/sandbox/warmup.rs | Implements backend warmup (Docker pull) to reduce first-create latency. |
| crates/ark-core/src/commands/sandbox/naming.rs | Implements stable naming/labeling for containers/volumes keyed by slug/branch. |
| crates/ark-core/src/commands/sandbox/gitmounts.rs | Derives git mount paths needed for worktree/common-dir access inside the container. |
| crates/ark-core/src/commands/sandbox/platform_argv.rs | Defines platform-specific argv used by enter --agent / platform launches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Other errors are infrastructure-level (e.g. manifest read failure); | ||
| // surfacing them as a shell fallback would hide a real problem. | ||
| Err(_) => shell_argv(), |
| let rm = docker::run_docker("rm", &["rm", "-f", &handle.container], Path::new("."))?; | ||
| let container_removed = rm.is_success() && !rm.stdout.trim().is_empty(); |
| push: | ||
| tags: | ||
| - "v[0-9]+.[0-9]+.[0-9]+" | ||
| - "v[0-9]+.[0-9]+.[0-9]+-**" |
| # The container runs as a fixed `ark-sandbox` user (uid 1000) baked into the | ||
| # image, so /etc/passwd is well-formed and tools that read it (bash prompt, | ||
| # git, claude CLI) see a real name. Workspace files written from inside the | ||
| # box are owned by uid 1000 on the host — visible and editable for the host | ||
| # user, even if not literally owned by them. |
| //! (mirroring [`crate::io::git::run_shell`]), and [`docker_info_ok`] / | ||
| //! [`docker_info_ok`] probes the daemon. Spawn failure (binary missing, | ||
| //! permissions) is a hard [`Error::DockerSpawn`]; a completed non-zero exit is |
| #[derive(clap::Args)] | ||
| struct SandboxRmCliArgs { | ||
| #[command(flatten)] | ||
| target: TargetArgs, | ||
| /// Task slug (defaults to this checkout's focused task). | ||
| #[arg(long)] | ||
| slug: Option<String>, | ||
| /// Keep the config volume (the one-time login token lives there). | ||
| #[arg(long = "keep-volume", default_value_t = false)] | ||
| keep_volume: bool, | ||
| } |
| /// Preserves the config volume unless `--keep-volume` is absent; an in-use | ||
| /// volume warns (reported as `volume_removed: false`) rather than failing. |
| let summary = sandbox_rm(SandboxRmOptions { | ||
| project_root: root, | ||
| slug: c.slug, | ||
| keep_volume: c.keep_volume, |
| assert!(matches!( | ||
| parse(&["sandbox", "rm", "--keep-volume"]), | ||
| SandboxCommand::Rm(c) if c.keep_volume | ||
| )); |
| /// Keep the config volume (the one-time login token lives there). | ||
| #[arg(long = "keep-volume", default_value_t = false)] | ||
| keep_volume: bool, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.ark/specs/features/ark-sandbox/SPEC.md:
- Line 19: Several fenced code blocks open with bare ``` (triggers MD040);
locate the three occurrences of the opening fence token "```" in SPEC.md and
annotate each with the correct language identifier (for example `text`, `rust`,
or `bash`) appropriate to the block contents so they read like ```text or
```rust etc.; update all three fences consistently to resolve MD040 and improve
rendering/tooling support.
In @.ark/tasks/ark-sandbox/00_PLAN.md:
- Line 63: The markdown contains unlabeled fenced code blocks (triple backticks)
at the noted locations; update each unlabeled block in the document to include
an appropriate language tag (for example ```bash, ```sh, ```json, ```yaml, or
```text as applicable) so they conform to MD040 and improve syntax
highlighting—scan and replace the unlabeled ``` occurrences around the examples
(previously at the noted positions) with the correct language identifiers.
In @.ark/tasks/ark-sandbox/01_PLAN.md:
- Line 83: Three fenced code blocks use unlabeled triple-backticks ("```") and
trigger markdownlint MD040; replace each unlabeled fence with an explicit
language label (for example ```bash, ```sh, ```yaml, or ```text as appropriate
for the block contents) so the code block semantics are clear and linting
passes; locate the three occurrences of the bare "```" fences in the document
and update each opening fence to include the correct language tag, keeping the
existing block content unchanged.
In @.ark/tasks/ark-sandbox/02_PLAN.md:
- Line 71: Three fenced code blocks currently use bare ``` without a language
tag; update those three blocks to use explicit language identifiers to satisfy
MD040 by replacing the opening fences with ```text, ```rust, and ```bash
respectively (choose the appropriate tag matching the block content). Locate the
three occurrences of bare triple-backtick fences in the file (the blocks that
currently start with just ```) and edit their opening lines to include the
correct language identifier so each fenced block is annotated.
In `@crates/ark-core/src/commands/sandbox/enter.rs`:
- Around line 61-64: The match in resolve_enter_argv handles Err(_) by calling
shell_argv(), which silently swallows infrastructure errors contrary to the
comment; update the code so the error is observable: either change the comment
to state that fallback is intentionally silent, or (preferred minimal fix) emit
a diagnostic (e.g., eprintln!, tracing::warn!, or process_logger.warn/error)
inside the Err(_) arm before calling shell_argv(), referencing
resolve_enter_argv and the Err(_) => shell_argv() branch so the manifest/read
failure is logged; alternatively, to truly propagate failures change
resolve_enter_argv to return Result<Vec<String>, Error> and bubble the error
instead of falling back.
In `@sandbox/Dockerfile`:
- Around line 8-12: The documentation comment states the baked-in user has uid
1000 but the Dockerfile actually creates the user "ark-sandbox" with uid 2000;
update one of them so they match: either change the user creation call that
creates "ark-sandbox" (the RUN useradd/adduser/--uid invocation) to use uid
1000, or update the top-of-file comment to state uid 2000; ensure the unique
symbol "ark-sandbox" and the useradd/user creation line are consistent and
update the comment text accordingly.
In `@templates/ark/config.toml`:
- Around line 80-86: The env_passthrough default list in
templates/ark/config.toml only includes uppercase proxy variables; update the
env_passthrough array (the env_passthrough setting) to also include the
lowercase equivalents "http_proxy", "https_proxy", and "no_proxy" alongside the
existing uppercase names so sandboxed processes receive proxy settings whether
the host exports uppercase or lowercase env vars.
🪄 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: d5fec10a-a0e1-40b7-8b9b-ce4eaf51c6c1
📒 Files selected for processing (42)
.ark/specs/features/INDEX.md.ark/specs/features/ark-sandbox/SPEC.md.ark/tasks/ark-sandbox/00_PLAN.md.ark/tasks/ark-sandbox/00_REVIEW.md.ark/tasks/ark-sandbox/01_PLAN.md.ark/tasks/ark-sandbox/01_REVIEW.md.ark/tasks/ark-sandbox/02_PLAN.md.ark/tasks/ark-sandbox/02_REVIEW.md.ark/tasks/ark-sandbox/PRD.md.ark/tasks/ark-sandbox/VERIFY.md.ark/tasks/ark-sandbox/research/agent-sandbox-technology.md.ark/tasks/ark-sandbox/task.toml.ark/workspace/Anekoique/index.md.ark/workspace/Anekoique/journal-1.md.ark/workspace/index.md.github/workflows/sandbox-image.ymlcrates/ark-cli/src/main.rscrates/ark-core/src/commands/context/mod.rscrates/ark-core/src/commands/mod.rscrates/ark-core/src/commands/sandbox/config.rscrates/ark-core/src/commands/sandbox/create.rscrates/ark-core/src/commands/sandbox/engine.rscrates/ark-core/src/commands/sandbox/engines/docker.rscrates/ark-core/src/commands/sandbox/engines/mod.rscrates/ark-core/src/commands/sandbox/enter.rscrates/ark-core/src/commands/sandbox/gitmounts.rscrates/ark-core/src/commands/sandbox/list.rscrates/ark-core/src/commands/sandbox/mod.rscrates/ark-core/src/commands/sandbox/naming.rscrates/ark-core/src/commands/sandbox/platform_argv.rscrates/ark-core/src/commands/sandbox/resolve.rscrates/ark-core/src/commands/sandbox/rm.rscrates/ark-core/src/commands/sandbox/warmup.rscrates/ark-core/src/error.rscrates/ark-core/src/io/docker.rscrates/ark-core/src/io/mod.rscrates/ark-core/src/lib.rscrates/ark-core/src/state/checkout/io.rscrates/ark-core/src/state/checkout/mod.rscrates/ark-core/src/state/mod.rssandbox/Dockerfiletemplates/ark/config.toml
|
|
||
| [**Architecture**] | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Line 19, Line 62, and Line 88 open fenced blocks without a language tag, which triggers MD040 and weakens rendering/tooling support. Please annotate them (for example: text, rust, or bash as appropriate).
Also applies to: 62-62, 88-88
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.ark/specs/features/ark-sandbox/SPEC.md at line 19, Several fenced code
blocks open with bare ``` (triggers MD040); locate the three occurrences of the
opening fence token "```" in SPEC.md and annotate each with the correct language
identifier (for example `text`, `rust`, or `bash`) appropriate to the block
contents so they read like ```text or ```rust etc.; update all three fences
consistently to resolve MD040 and improve rendering/tooling support.
|
|
||
| [**Architecture**] | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify languages for fenced code blocks.
Line 63, Line 105, Line 129, Line 142, and Line 155 use unlabeled triple-backtick blocks. Please add language tags to satisfy MD040 and improve readability/tooling behavior.
Also applies to: 105-105, 129-129, 142-142, 155-155
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.ark/tasks/ark-sandbox/00_PLAN.md at line 63, The markdown contains
unlabeled fenced code blocks (triple backticks) at the noted locations; update
each unlabeled block in the document to include an appropriate language tag (for
example ```bash, ```sh, ```json, ```yaml, or ```text as applicable) so they
conform to MD040 and improve syntax highlighting—scan and replace the unlabeled
``` occurrences around the examples (previously at the noted positions) with the
correct language identifiers.
|
|
||
| [**Architecture**] | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Annotate fenced blocks with explicit languages.
Line 71, Line 114, and Line 140 start code fences without language identifiers. Please add appropriate tags (text/rust/bash) to resolve MD040.
Also applies to: 114-114, 140-140
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.ark/tasks/ark-sandbox/02_PLAN.md at line 71, Three fenced code blocks
currently use bare ``` without a language tag; update those three blocks to use
explicit language identifiers to satisfy MD040 by replacing the opening fences
with ```text, ```rust, and ```bash respectively (choose the appropriate tag
matching the block content). Locate the three occurrences of bare
triple-backtick fences in the file (the blocks that currently start with just
```) and edit their opening lines to include the correct language identifier so
each fenced block is annotated.
| env_passthrough = [ | ||
| "ANTHROPIC_API_KEY", | ||
| "OPENAI_API_KEY", | ||
| "HTTP_PROXY", | ||
| "HTTPS_PROXY", | ||
| "NO_PROXY", | ||
| ] |
There was a problem hiding this comment.
Include lowercase proxy env names in defaults.
Only uppercase proxy names are forwarded by default. On hosts that export lowercase-only (http_proxy, https_proxy, no_proxy), network access in the sandbox can fail unexpectedly.
Suggested diff
env_passthrough = [
"ANTHROPIC_API_KEY",
"OPENAI_API_KEY",
"HTTP_PROXY",
"HTTPS_PROXY",
"NO_PROXY",
+ "http_proxy",
+ "https_proxy",
+ "no_proxy",
]📝 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.
| env_passthrough = [ | |
| "ANTHROPIC_API_KEY", | |
| "OPENAI_API_KEY", | |
| "HTTP_PROXY", | |
| "HTTPS_PROXY", | |
| "NO_PROXY", | |
| ] | |
| env_passthrough = [ | |
| "ANTHROPIC_API_KEY", | |
| "OPENAI_API_KEY", | |
| "HTTP_PROXY", | |
| "HTTPS_PROXY", | |
| "NO_PROXY", | |
| "http_proxy", | |
| "https_proxy", | |
| "no_proxy", | |
| ] |
🤖 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 `@templates/ark/config.toml` around lines 80 - 86, The env_passthrough default
list in templates/ark/config.toml only includes uppercase proxy variables;
update the env_passthrough array (the env_passthrough setting) to also include
the lowercase equivalents "http_proxy", "https_proxy", and "no_proxy" alongside
the existing uppercase names so sandboxed processes receive proxy settings
whether the host exports uppercase or lowercase env vars.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622f9e3f27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // "I have no name!" behavior. Users on a custom image who want | ||
| // host-uid ownership of /workspace writes can pass the flag manually | ||
| // via a future `--user` CLI option; v1 ships with the baked-in user. | ||
| None |
There was a problem hiding this comment.
On a Linux host where the developer UID is not the image UID (the Dockerfile creates ark-sandbox as uid 2000, while most developer accounts are uid 1000), returning None here makes Docker write new files in the bind-mounted /workspace as uid 2000. After an in-box agent edits or creates files, the host user may be unable to edit/delete them without sudo, which breaks the normal sandbox workflow; the engine needs to pass the host uid/gid (or otherwise align ownership) for the workspace mount.
Useful? React with 👍 / 👎.
| #[arg(long = "keep-volume", default_value_t = false)] | ||
| keep_volume: bool, |
There was a problem hiding this comment.
With this default, ark sandbox rm (the documented teardown command after a one-time in-box login) calls the engine with keep_volume: false, so Docker removes the config volume that stores the agent login/session. Users who run the ordinary rm command then lose their persisted sandbox credentials and must log in again on the next create; this should default to preserving the volume or expose the destructive behavior via an explicit flag.
Useful? React with 👍 / 👎.
| } | ||
| // Other errors are infrastructure-level (e.g. manifest read failure); | ||
| // surfacing them as a shell fallback would hide a real problem. | ||
| Err(_) => shell_argv(), |
There was a problem hiding this comment.
This catch-all turns infrastructure errors from select_platform (for example a corrupt .ark/.installed.json manifest) into a successful bash fallback, even though the surrounding comment says those errors should surface. In that scenario ark sandbox enter hides the broken Ark installation and exits through an interactive shell instead of failing, so resolve_enter_argv should return a Result and only fall back for the explicitly tolerated no-platform/unsupported-yolo cases.
Useful? React with 👍 / 👎.
| if spec.host_config_mounts.is_empty() { | ||
| a.push("-v".into()); | ||
| a.push(format!("{}:{GUEST_CONFIG_DIR}", spec.config_volume)); |
There was a problem hiding this comment.
In the default isolated mode this only persists /home/ark-sandbox/.claude, but the same feature code treats ~/.claude.json, ~/.codex, and ~/.codex.toml as login/config locations when share_host_config is enabled. A user who logs in inside the default sandbox and then uses --recreate or removes/recreates while preserving the volume can still lose any session/config written to those unmounted sidecar paths, so the persistent storage needs to cover the whole agent home/config set rather than just the Claude directory.
Useful? React with 👍 / 👎.
|
|
||
| /// Derives the container/volume names for `slug` on `branch`. | ||
| pub fn derive(slug: &str, branch: &str) -> SandboxNames { | ||
| let container = format!("{PREFIX}-{}-{}", sanitize(branch), hash8(branch)); |
There was a problem hiding this comment.
The Docker container/volume names are derived only from the branch, so two different Ark checkouts on the same Docker daemon that both create a sandbox for feat/foo compute the same ark-sandbox-feat-foo-… name. In that case the second project either reports an existing sandbox or fails docker run --name, and slug-based enter/rm can also find another project's container because the labels are not project-scoped; include a project identity/hash in the name and lookup labels.
Useful? React with 👍 / 👎.
| && useradd -m -u 2000 -s /bin/bash ark-sandbox \ | ||
| && mkdir -p /workspace \ | ||
| && chown ark-sandbox:ark-sandbox /workspace |
There was a problem hiding this comment.
In the default isolated path the engine mounts a fresh named volume at /home/ark-sandbox/.claude, but this image never creates that directory before switching to the non-root ark-sandbox user. When Docker initializes a named volume at a missing target, the mountpoint/volume root is created by the daemon as root, so claude /login inside the sandbox can hit permission denied writing its config; create and chown the agent config mountpoints in the image before USER ark-sandbox.
Useful? React with 👍 / 👎.
| let script = format!( | ||
| "[ -f {GUEST_WORKSPACE}/.git ] && printf '%s\\n' '{new_line}' > {GUEST_WORKSPACE}/.git || \ | ||
| true" | ||
| ); | ||
| let _ = docker::run_docker("exec", &["exec", container, "sh", "-c", &script], cwd)?; |
There was a problem hiding this comment.
A valid git branch/worktree name can contain a single quote, and that basename is interpolated directly into a single-quoted shell script here. For a task created on such a branch (or under a project path containing '), the docker exec sh -c rewrite either fails silently or can run unintended shell tokens, leaving /workspace/.git pointing at the host path and breaking in-box git; pass the value without shell interpolation or quote it safely and check the exec status.
Useful? React with 👍 / 👎.
|
Claude finished @Anekoique's task in 4m 4s —— View job PR Review: feat(sandbox): add ark sandboxTodo list:
Review Findings🔴 HIGH — Blocks functionality1. CI workflow tag pattern never fires ( tags:
- "v[0-9]+.[0-9]+.[0-9]+"
- "v[0-9]+.[0-9]+.[0-9]+-**"GitHub Actions uses glob syntax, not regex. Fix: use glob wildcards instead of regex quantifiers. - "v[0-9]*.[0-9]*.[0-9]*"
- "v[0-9]*.[0-9]*.[0-9]*-*"2. #[arg(long = "keep-volume", default_value_t = false)]
keep_volume: bool,With The correct UX is an opt-in destructive flag: // CLI: rename to --drop-volume
#[arg(long = "drop-volume", default_value_t = false)]
drop_volume: bool,
// Dispatch (main.rs ~line 799):
keep_volume: !c.drop_volume,🟡 MEDIUM — Correctness issues3. Infrastructure errors silently fall back to a shell ( // Other errors are infrastructure-level (e.g. manifest read failure);
// surfacing them as a shell fallback would hide a real problem.
Err(_) => shell_argv(),The comment says this hides a real problem — and then does exactly that. A corrupt Err(e) => {
eprintln!("ark sandbox: {e}; opening a shell instead");
shell_argv()
}Or, prefer propagating via 4. Shell injection in let script = format!(
"[ -f {GUEST_WORKSPACE}/.git ] && printf '%s\\n' '{new_line}' > {GUEST_WORKSPACE}/.git || true"
);
Fix: use 5. Container/volume names not scoped to the project ( Names are The fix is to include a project-identity component in the name (e.g., a short hash of the project root or the parent repo remote URL). This is a design-level issue that needs a SPEC amendment, but it's a real multi-project collision risk. ⚪ LOW — Documentation / minor6. Dockerfile uid comment says 1000, code creates uid 2000 ( The header comment will mislead users debugging host file ownership. Update the comment to match uid 2000. 7. Copy-paste duplicate in the module doc comment. 8. No stderr warning when volume removal is silently skipped ( SPEC C-10 says "a volume-in-use error warns rather than fails." The code sets Summary
The two HIGH items (CI trigger and |
Anekoique
force-pushed
the
feat/ark-sandbox
branch
from
622f9e3 to
c2f8c0e
Compare
|
Claude finished @Anekoique's task in 4m 0s —— View job PR Review: feat(sandbox): add ark sandboxTodo list:
Previously Reported HIGHs — all resolved ✅The two HIGH findings from the prior review have been addressed in this update:
The three MEDs and three LOWs from that review are also resolved: Remaining Findings🟡 MEDIUM — Shell injection in
|
| Severity | Finding | Fixed? |
|---|---|---|
| 🔴 HIGH | CI tag glob broken — image never published | ✅ Fixed |
| 🔴 HIGH | --keep-volume default wiped the volume |
✅ Fixed |
| 🟡 MED | enter catch-all swallowed infra errors silently |
✅ Fixed |
| 🟡 MED | rewrite_gitdir shell injection via paths with ' |
Still open |
| 🟡 MED | Container names not project-scoped — collision on shared daemon | Still open |
| ⚪ LOW | Dockerfile uid comment mismatch | ✅ Fixed |
| ⚪ LOW | Duplicate docker_info_ok in module doc |
✅ Fixed |
| ⚪ LOW | Volume-in-use path had no stderr warning | ✅ Fixed |
| ⚪ LOW | warmup verb not in SPEC |
New |
| ⚪ LOW | SPEC C-22 divergence (host_user always None) |
New |
The two remaining MEDs are the ones to fix before merge. The rest of the implementation — engine abstraction, config loading, deny_unknown_fields placement, git-mount derivation via rev-parse, share_host_config opt-in with full host-config-pair list, proxy loopback rewrite, idempotent teardown, --drop-volume semantics, and the CI workflow — looks solid.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2f8c0ecfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let handle = engine.resolve_handle(&task.slug, &task.branch)?; | ||
| engine.remove(&handle, &RemoveOpts { keep_volume: true })?; |
There was a problem hiding this comment.
When --recreate is used and an existing sandbox is found, this removes the current container before engine.create() has pulled or started the replacement; DockerEngine::create begins with docker pull. If the registry is unavailable or the configured image tag is missing, the command exits with ImagePullFailed after deleting an otherwise usable sandbox, leaving the user with no container. Pull or otherwise validate the replacement image before removing the old sandbox.
Useful? React with 👍 / 👎.
| cache-to: type=gha,mode=max | ||
| # Linux amd64 + arm64 so the image works on both x86 CI and Apple | ||
| # Silicon laptops without an emulation tax. | ||
| platforms: linux/amd64,linux/arm64 |
There was a problem hiding this comment.
This workflow asks an ubuntu-22.04 runner to build linux/arm64 as well as linux/amd64, but it only sets up Buildx and never installs QEMU/binfmt first. Docker's official multi-platform GitHub Actions example places docker/setup-qemu-action before setup-buildx-action for this exact platform pair; because this Dockerfile has RUN apt-get and RUN npm install steps, the arm64 build leg has to execute arm64 binaries and will fail on the amd64 runner without emulation. Add docker/setup-qemu-action before Buildx or split arm64 onto an arm runner.
Useful? React with 👍 / 👎.
| let rm = docker::run_docker("rm", &["rm", "-f", &handle.container], Path::new("."))?; | ||
| let container_removed = rm.is_success() && !rm.stdout.trim().is_empty(); |
There was a problem hiding this comment.
If docker rm -f completes with a non-zero status for anything other than the container already being gone, this still returns Ok with container_removed: false, so ark sandbox rm can print container absent while the sandbox is still present and Docker already provided the failure details in rm.stderr. Keep the idempotent no-such-container case, but return an error for other removal failures so teardown does not silently fail.
Useful? React with 👍 / 👎.
| value | ||
| .replace("://127.0.0.1", "://host.docker.internal") | ||
| .replace("://localhost", "://host.docker.internal") |
There was a problem hiding this comment.
When a forwarded proxy is local and includes userinfo, e.g. HTTPS_PROXY=http://user:pass@127.0.0.1:7897, this replacement does not see the loopback host because it only matches immediately after ://. The value is then passed into the container unchanged, where 127.0.0.1 refers to the container itself, so users behind an authenticated local proxy still get broken network despite the proxy-forwarding support; parse the URL host or also handle the userinfo form.
Useful? React with 👍 / 👎.
| if let Some(slug) = slug { | ||
| return Ok(slug); |
There was a problem hiding this comment.
An explicit --slug is accepted here without the validate_slug check used by the agent task commands, and resolve_local_worktree later joins it under .ark/tasks. With a slash-containing value such as archive/2026-05/foo, a sandbox command can load an archived task's task.toml; if that task was worktree-backed, the local resolver treats the current checkout as the worktree and can create/enter the wrong sandbox instead of rejecting the invalid slug.
Useful? React with 👍 / 👎.
This pull request introduces the new Ark sandbox feature, adding comprehensive containerized task isolation to the project. The sandbox system allows users to create, enter, remove, and list sandboxes, each confining a worktree inside a Docker container with persistent configuration and integrated Git support. The implementation covers core architectural modules, error handling, configuration, and CLI integration, while clearly specifying goals, non-goals, and constraints for v1.
The most important changes are:
New Feature: Ark Sandbox System
ark sandboxcommand suite with subcommands forcreate,enter,rm, andlist, enabling container-based task isolation using Docker. The sandbox persists configuration, supports agent login, and integrates with Git worktrees.ark-sandboxto the features index, documenting its addition and scope.Architecture and Implementation
ark-corewith new modules for sandbox engine abstraction, Docker integration, configuration, naming, Git mounts, and CLI argument parsing. Only the Docker backend is implemented in v1, with a clear trait for future engines.io::dockermodule, ensuring a single spawn site and supporting both captured and interactive command execution.Error Handling and API Surface
sandbox_create,sandbox_enter,sandbox_rm,sandbox_list) and configuration management, with summaries for each CLI operation.Constraints and Non-goals
.ark/, no custom worktree management, no network confinement in v1, and idempotent removal. Non-goals and limitations for the initial release are explicitly stated.Summary by CodeRabbit
Release Notes
New Features
ark sandboxcommand suite:create,enter,rm,list, andwarmupfor Docker-backed task confinement.Documentation
Configuration
[sandbox]section in config templates with Docker engine, image, git mounting, and credential-sharing options.Infrastructure