feat(sandbox): persistent per-user sandbox container (V1 Phase A) - #6584
henrypark133 wants to merge 15 commits into
Conversation
… onto f7da7dd) Ephemeral sandbox wiring: sandboxed profile, sandbox_boot, quota, reaper, egress allowlist scaffolding, shell limits, escape tests. Pre-rebase checkpoint; PR split happens after review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y map Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ant_sandbox_process_binding Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r-exec transport Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dboxed-profile composition seam Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…uota Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ss footer Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o-stage idle/retention lifecycle Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ant to ResourceAccount::user Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/gh/tmux and workspace-persistent HOME Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…stage reaper API
A5 rewrote SandboxReaperConfig/SandboxReaper (dropped orphan_threshold +
RunStateStore, added idle/retention/forced-recycle) but verified only via
--lib, leaving the docker-gated integration test uncompilable. Rewrite it to
the new {tenant,user,created_at}-label model and exercise all three ReapActions
(survive / forced-recycle stop / forced-recycle remove) via the created_at
label, since SandboxActivityRegistry::touch is crate-private to integration tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-FS root Sandboxed-profile builds register RebornSandboxUserKey::workspace_path as a CompositeRootFilesystem mount at /workspace, and redirect the Workspace capability MountView's /workspace grant onto it, so read_file/write_file and the sandbox container's own /workspace bind (Task A3) resolve the same host directory. Fixed boot-owner mount only; per-user dynamic mounts are a follow-up if a multi-user hosted-sandboxed profile is added. Also adds "/workspace" to ironclaw_host_api::path::VIRTUAL_ROOTS: VirtualPath validation rejected it as an unknown root, which the RED test caught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Proves the persistent sandbox container's shell writes and the composition /workspace mount (previous commit) resolve the same host directory, in both directions. Skips with a visible SKIP line where no Docker daemon is reachable (dev machines); runs for real on CI/hosted Docker runners. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: sandbox
github-actions
Bot
added
size: XL
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces a persistent per-user Docker sandbox execution model replacing per-command containers: an exec-based transport, container reaper, network allowlist, shell timeout/output clamping, and background execution. It adds a new ChangesSandbox Runtime and Profile
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | a988b6947fbd |
Head: a988b6947fbd9c2d49442c5209e00abcb8c1e4be
Reason: The diff spans 348 files with 22,387 additions and 26,396 deletions across sandboxing plus extensive unrelated composition, workflow, WebUI, extension, and test rewrites. A complete correctness and security review cannot be performed reliably within this review scope.
Next: Re-submit the sandbox work as a bounded layer (or provide its direct parent/base SHA) that isolates the persistent-sandbox implementation, required wiring, and focused tests; then request a fresh review of that comparison.
Run details
Status: Current
Trustworthy review produced: no
Summary
Review declined: the supplied main-to-head comparison is an oversized, cross-cutting mega-diff rather than a reliably reviewable Phase A sandbox layer.
![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: a988b6947f
ℹ️ 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 path = ironclaw_host_runtime::RebornSandboxUserKey::from_scope(&owner_scope) | ||
| .workspace_path(&root); |
There was a problem hiding this comment.
For HostedSingleTenantVolumeSandboxed, this computes the abstract /workspace mount once from the boot owner and mounts that single directory into the shared root filesystem. In a hosted single-tenant deployment with multiple authenticated users, builtin.read_file/builtin.write_file for every non-owner user will resolve /workspace to the owner's sandbox directory, while that user's shell container still derives its bind from request.scope.user_id; this both leaks the owner workspace and breaks shell/filesystem parity for other users.
AGENTS.md reference: crates/AGENTS.md:L207-L210
Useful? React with 👍 / 👎.
| if stderr.is_empty() { | ||
| Ok(stdout) | ||
| } else if stdout.is_empty() { | ||
| Ok(stderr) | ||
| } else { |
There was a problem hiding this comment.
When the tenant-sandbox path is used, Docker stdout/stderr are returned directly from collect_exec_output; unlike the host process path (capture_command_output), this never runs the command-output LeakDetector that blocks or redacts secret-looking output. A sandboxed shell command that prints a .env value or API key-shaped token can therefore send it back to the model verbatim.
AGENTS.md reference: crates/ironclaw_host_runtime/AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
| crate::sandbox_quota::apply_sandbox_user_ceiling( | ||
| &inputs.resource_governor, | ||
| sandbox_tenant_id, | ||
| inputs.owner_user_id, | ||
| crate::sandbox_quota::sandbox_max_concurrent_from_env(), |
There was a problem hiding this comment.
This sets the sandbox SpawnProcess ceiling only for the boot owner. The ReserveResources obligation later reserves against the actual invocation's ResourceScope user, so any other authenticated user in the hosted single-tenant sandboxed profile has no configured ceiling and can launch containers without the intended per-user cap.
AGENTS.md reference: crates/AGENTS.md:L207-L209
Useful? React with 👍 / 👎.
| echo "## Docker Images — skipped" | ||
| echo "" | ||
| echo "Current commit already built for \`${IMAGE_NAME}:staging\`." | ||
| echo "Current commit already built for \`${IMAGE_NAME}:staging\` / \`${IMAGE_NAME_WORKER}:staging\`." |
There was a problem hiding this comment.
The skipped summary now claims both ironclaw:staging and ironclaw-worker:staging are current, but the preceding skip check only pulls/inspects IMAGE_NAME:staging. If a prior run pushed the main image and failed before pushing or smoking the worker image, the next scheduled/staging run will set skip=true, skip the worker build and smoke job, and leave the worker tag missing or stale.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_host_runtime/src/process_port.rs (1)
680-695: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the stale truncation assertion.
This transport returns
COMMAND_MAX_OUTPUT_SIZE + 1bytes (16,385), but an unsetoutput_limit_bytesnow clamps to 65,536. No truncation occurs, so this assertion fails. Set the request limit toCOMMAND_MAX_OUTPUT_SIZEor increase the fixture past the new default.🤖 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 `@crates/ironclaw_host_runtime/src/process_port.rs` around lines 680 - 695, Update the CommandExecutionRequest in the truncation test before port.run_command so output_limit_bytes is explicitly set to COMMAND_MAX_OUTPUT_SIZE, ensuring the existing truncated-output assertion remains valid.
🤖 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 @.github/workflows/docker.yml:
- Around line 260-262: Update the staging-image skip gate in the workflow to
pull and inspect both IMAGE_NAME:staging and IMAGE_NAME_WORKER:staging,
including each image’s ironclaw.git.sha label. Only skip the builds and worker
smoke job when both labels match SOURCE_SHA; otherwise continue the build path.
In `@crates/ironclaw_host_runtime/src/first_party_tools/shell.rs`:
- Around line 41-43: The documented lower-bound behavior for output_limit
conflicts with its zero-value rejection. In
crates/ironclaw_host_runtime/src/first_party_tools/shell.rs lines 41-43, either
revise the description to cover only valid positive numeric values or update the
implementation to accept and clamp zero; in
crates/ironclaw_host_runtime/src/first_party_tools/shell_core.rs lines 151-167,
align the parsing comments with the chosen behavior by removing claims that
under-floor values are clamped and that only malformed values are rejected when
zero remains invalid.
In `@crates/ironclaw_host_runtime/src/process_output.rs`:
- Around line 576-587: The truncate_output_to function currently budgets limit
bytes for content without reserving space for the truncation marker, allowing
the result to exceed the requested cap. Calculate the marker length first,
budget the head and tail from limit minus that length, and ensure the returned
truncated output never exceeds limit, including for the 4,096-byte case.
In `@crates/ironclaw_host_runtime/src/process_port.rs`:
- Around line 248-254: Update execute_local_command and process_output so the
request’s output_limit bounds captured and saved process output on the host
path, instead of using only the fixed preview/save split. Preserve the existing
timeout clamp and ensure the shell caller exercises the per-call limit,
including a small limit such as 1 KiB.
In `@crates/ironclaw_host_runtime/src/sandbox_process/exec_transport.rs`:
- Around line 299-352: Update the background launch flow around launch_script
and BackgroundLaunch so the shell emits both the background child PID ($!) and
the actual log path used by the redirection. Parse the two whitespace-separated
values from pid_output, use the first as pid, and store the second as log_path
instead of reconstructing the path from the PID.
In `@crates/ironclaw_host_runtime/src/sandbox_process/network_allowlist.rs`:
- Around line 55-66: Extract the comma-separated domain parsing from
sandbox_extra_allowed_domains into a pure parse_extra_allowed_domains(raw:
Option<&str>) -> Vec<String> helper, following the pattern of
resolve_duration_secs_from_raw. Have sandbox_extra_allowed_domains pass the
environment value to this helper, and update the related tests to call the
parser directly without mutating process environment variables.
In `@crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs`:
- Around line 3659-3667: The test
builtin_shell_clamps_rather_than_rejects_timeout_above_the_600s_ceiling only
verifies command success, not timeout clamping. Update it to invoke the real
shell caller with a recording process port and assert the port receives
timeout_secs: Some(600), preserving the existing oversized timeout input and
successful command behavior.
In `@crates/ironclaw_host_runtime/tests/sandbox_cross_tenant_escape.rs`:
- Around line 126-152: Strengthen the assertions in the tenant B read attempt
around read_output by verifying that both "--relative--" and "--escape--"
markers appear in the command output before checking marker_secret is absent.
Keep the existing exit-code and secret-content assertions, ensuring the test
proves both cross-tenant read paths actually executed.
In `@crates/ironclaw_reborn_composition/src/deployment.rs`:
- Around line 540-542: “Add an outermost-caller regression test covering the new
HostedSingleTenantVolumeSandboxed dispatch arm.” Extend the tests around
local_runtime_build_input_with_options and the existing
local_runtime_build_input_with_options_with_volume_profile test pattern to
invoke RebornCompositionProfile::HostedSingleTenantVolumeSandboxed through
local_runtime_build_input_with_options, asserting the same fail-closed behavior
when the required environment is unset; keep the existing
HostedSingleTenantVolume scenario unchanged.
In `@crates/ironclaw_reborn_composition/src/sandbox_boot.rs`:
- Around line 25-67: Update the network configuration around
SANDBOX_HTTP_PROXY_URL_ENV and SANDBOX_HTTP_PROXY_PORT_ENV so configuring either
variable cannot enable bridge networking or imply enforced egress before a real
allowlist proxy exists. Preserve the fail-closed RebornRuntimeProcessBinding
behavior by retaining the --network none posture unless network-level proxy
enforcement is verified and explicitly supported. Remove or disable the soft
proxy environment wiring in the TenantSandbox setup and revise nearby
documentation to match the safe behavior.
In `@crates/ironclaw_reborn_composition/src/sandbox_quota.rs`:
- Around line 1-14: Update the module documentation and the
SANDBOX_MAX_CONCURRENT_ENV documentation to describe the concurrency ceiling as
scoped per user, not tenant-wide or per-tenant. Keep the implementation in
apply_sandbox_user_ceiling unchanged, including its ResourceAccount::user
scoping, and align the wording with sandbox_composition.rs.
In `@docker/process-sandbox-entrypoint.sh`:
- Around line 52-54: Update the persistent-home initialization commands in the
entrypoint to propagate mkdir and copy failures instead of suppressing them.
Stage the /home/sandbox/.cargo and /home/sandbox/.rustup copies in temporary
destinations under /workspace/.home, then atomically move each completed stage
into place so failed copies cannot leave directories that prevent retries.
In `@Dockerfile.process-sandbox`:
- Line 57: Update the Rust bootstrap RUN command in Dockerfile.process-sandbox
to download the installer with curl into a temporary file first, then execute
that file with sh using the existing rustup arguments. Preserve strict curl
failure behavior and ensure the installer is only run after a successful
download.
---
Outside diff comments:
In `@crates/ironclaw_host_runtime/src/process_port.rs`:
- Around line 680-695: Update the CommandExecutionRequest in the truncation test
before port.run_command so output_limit_bytes is explicitly set to
COMMAND_MAX_OUTPUT_SIZE, ensuring the existing truncated-output assertion
remains valid.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 377fe449-1ede-40ea-a9ec-bf2e99f3170f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (53)
.github/workflows/docker.ymlDockerfile.process-sandboxcrates/ironclaw_authorization/src/lib.rscrates/ironclaw_authorization/tests/capability_access_contract.rscrates/ironclaw_host_api/src/path.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/first_party_tools/shell.rscrates/ironclaw_host_runtime/src/first_party_tools/shell_core.rscrates/ironclaw_host_runtime/src/invocation_services.rscrates/ironclaw_host_runtime/src/invocation_services/tests.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/post_edit_check.rscrates/ironclaw_host_runtime/src/process_output.rscrates/ironclaw_host_runtime/src/process_port.rscrates/ironclaw_host_runtime/src/sandbox_process.rscrates/ironclaw_host_runtime/src/sandbox_process/connect.rscrates/ironclaw_host_runtime/src/sandbox_process/exec_transport.rscrates/ironclaw_host_runtime/src/sandbox_process/network_allowlist.rscrates/ironclaw_host_runtime/src/sandbox_process/reaper.rscrates/ironclaw_host_runtime/src/sandbox_process/registry.rscrates/ironclaw_host_runtime/src/sandbox_process/shell_limits.rscrates/ironclaw_host_runtime/src/sandbox_process/user_key.rscrates/ironclaw_host_runtime/src/services/tests.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/sandbox_cross_tenant_escape.rscrates/ironclaw_host_runtime/tests/sandbox_reaper_docker.rscrates/ironclaw_host_runtime/tests/sandbox_workspace_fs_parity_docker.rscrates/ironclaw_host_runtime/tests/support/docker_gate.rscrates/ironclaw_host_runtime/tests/support/sandbox_transport.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/deployment.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/factory/local_dev_host_tests.rscrates/ironclaw_reborn_composition/src/factory/tests.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/local_dev_mounts.rscrates/ironclaw_reborn_composition/src/readiness.rscrates/ironclaw_reborn_composition/src/root/profile.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/sandbox_boot.rscrates/ironclaw_reborn_composition/src/sandbox_composition.rscrates/ironclaw_reborn_composition/src/sandbox_quota.rscrates/ironclaw_reborn_composition/src/sandbox_reaper_task.rscrates/ironclaw_reborn_composition/src/webui/facade.rscrates/ironclaw_reborn_composition/tests/profile_acceptance.rscrates/ironclaw_reborn_composition/tests/sandbox_two_user_composition.rscrates/ironclaw_reborn_config/src/profile.rscrates/ironclaw_reborn_config/tests/profile_contract.rsdocker/process-sandbox-entrypoint.shdocs/plans/composition-pubuse.snapshot
| echo "Current commit already built for \`${IMAGE_NAME}:staging\` / \`${IMAGE_NAME_WORKER}:staging\`." | ||
| echo "- sha: \`${SOURCE_SHA}\`" | ||
| } >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify the worker image before skipping.
The skip gate only pulls and inspects ${IMAGE_NAME}:staging. If ironclaw-worker:staging is missing or stale, this run skips its build and the worker smoke job while claiming both images are current. Inspect the worker image’s ironclaw.git.sha too, and skip only when both labels match SOURCE_SHA.
🤖 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 @.github/workflows/docker.yml around lines 260 - 262, Update the
staging-image skip gate in the workflow to pull and inspect both
IMAGE_NAME:staging and IMAGE_NAME_WORKER:staging, including each image’s
ironclaw.git.sha label. Only skip the builds and worker smoke job when both
labels match SOURCE_SHA; otherwise continue the build path.
| `timeout` (seconds) and `output_limit` (bytes) are model-adjustable per call: timeout defaults to \ | ||
| 120s and is clamped to a 600s ceiling, output_limit defaults to 64 KiB and is clamped to a 1 MiB \ | ||
| ceiling — values outside these ranges are clamped, not rejected.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the lower-bound clamping contract. output_limit: 0 is rejected, but the manifest and parser docs promise values outside the range are clamped and only malformed values are rejected.
crates/ironclaw_host_runtime/src/first_party_tools/shell.rs#L41-L43: limit the claim to valid positive numeric values, or accept zero and clamp it.crates/ironclaw_host_runtime/src/first_party_tools/shell_core.rs#L151-L167: correct “under-floor values are clamped” and “only rejects malformed value” to match the zero-value rejection.
As per coding guidelines, “Comments promising cross-layer guarantees must be enforced by code or tests, or softened to describe intent.”
📍 Affects 2 files
crates/ironclaw_host_runtime/src/first_party_tools/shell.rs#L41-L43(this comment)crates/ironclaw_host_runtime/src/first_party_tools/shell_core.rs#L151-L167
🤖 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 `@crates/ironclaw_host_runtime/src/first_party_tools/shell.rs` around lines 41
- 43, The documented lower-bound behavior for output_limit conflicts with its
zero-value rejection. In
crates/ironclaw_host_runtime/src/first_party_tools/shell.rs lines 41-43, either
revise the description to cover only valid positive numeric values or update the
implementation to accept and clamp zero; in
crates/ironclaw_host_runtime/src/first_party_tools/shell_core.rs lines 151-167,
align the parsing comments with the chosen behavior by removing claims that
under-floor values are clamped and that only malformed values are rejected when
zero remains invalid.
Source: Coding guidelines
| pub(crate) fn truncate_output_to(s: &str, limit: usize) -> String { | ||
| if s.len() <= limit { | ||
| s.to_string() | ||
| } else { | ||
| let half = COMMAND_MAX_OUTPUT_SIZE / 2; | ||
| let half = limit / 2; | ||
| let head_end = floor_char_boundary(s, half); | ||
| let tail_start = floor_char_boundary(s, s.len() - half); | ||
| format!( | ||
| "{}\n\n... [truncated {} bytes] ...\n\n{}", | ||
| &s[..head_end], | ||
| s.len() - COMMAND_MAX_OUTPUT_SIZE, | ||
| s.len() - limit, | ||
| &s[tail_start..] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reserve space for the truncation marker.
The function retains limit bytes of head/tail plus the marker, so it exceeds the requested cap. For the new test’s 4,106-byte input and 4,096-byte limit, the result is larger than the input, making Line 640 fail. Budget head/tail from limit - marker.len() and assert truncated.len() <= limit.
As per path instructions, “Apply explicit limits to user-controlled … process output.”
🤖 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 `@crates/ironclaw_host_runtime/src/process_output.rs` around lines 576 - 587,
The truncate_output_to function currently budgets limit bytes for content
without reserving space for the truncation marker, allowing the result to exceed
the requested cap. Calculate the marker length first, budget the head and tail
from limit minus that length, and ensure the returned truncated output never
exceeds limit, including for the 4,096-byte case.
Source: Path instructions
| // Same operator ceiling as the sandboxed transport (see | ||
| // `sandbox_process::shell_limits`), applied here so the unsandboxed | ||
| // host path can't be used to bypass the model-adjustable `timeout` | ||
| // clamp. `output_limit` is not honored on this path: it captures via | ||
| // a fixed preview/save-to-file split (`process_output`) independent | ||
| // of the sandbox's inline-capture cap. | ||
| let timeout = clamp_shell_timeout_secs(request.timeout_secs); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Honor output_limit on the host-process path.
The shared shell schema advertises this as a per-call captured-output bound, but this branch explicitly ignores it; a local-profile request for 1 KiB can still capture/save the fixed-size output. Enforce the same bound in execute_local_command/process_output, and cover it through the shell caller.
As per path instructions, “Apply explicit limits to user-controlled … process output.”
🤖 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 `@crates/ironclaw_host_runtime/src/process_port.rs` around lines 248 - 254,
Update execute_local_command and process_output so the request’s output_limit
bounds captured and saved process output on the host path, instead of using only
the fixed preview/save split. Preserve the existing timeout clamp and ensure the
shell caller exercises the per-call limit, including a small limit such as 1
KiB.
Source: Path instructions
| let launch_script = format!( | ||
| "mkdir -p /workspace/.ironclaw && {} >>/workspace/.ironclaw/bg-$$.log 2>&1 & echo $!", | ||
| wrap_command_for_pgid_isolation(&command), | ||
| ); | ||
| let exec = docker | ||
| .create_exec( | ||
| container_id, | ||
| CreateExecOptions { | ||
| cmd: Some(vec!["sh".to_string(), "-c".to_string(), launch_script]), | ||
| attach_stdout: Some(true), | ||
| attach_stderr: Some(true), | ||
| working_dir: Some(workdir.into_string()), | ||
| env: Some(env), | ||
| ..Default::default() | ||
| }, | ||
| ) | ||
| .await | ||
| .map_err(|error| { | ||
| RuntimeProcessError::ExecutionFailed(format!( | ||
| "sandbox background launch failed: {error}" | ||
| )) | ||
| })?; | ||
| let launch_timeout = Duration::from_secs(10); | ||
| let pid_output = tokio::time::timeout(launch_timeout, async { | ||
| match docker | ||
| .start_exec( | ||
| &exec.id, | ||
| Some(StartExecOptions { | ||
| detach: false, | ||
| tty: false, | ||
| ..Default::default() | ||
| }), | ||
| ) | ||
| .await | ||
| .map_err(|error| { | ||
| RuntimeProcessError::ExecutionFailed(format!( | ||
| "sandbox background launch start failed: {error}" | ||
| )) | ||
| })? { | ||
| StartExecResults::Attached { output, .. } => collect_exec_output(output, 256).await, | ||
| StartExecResults::Detached => Ok(String::new()), | ||
| } | ||
| }) | ||
| .await | ||
| .map_err(|_| RuntimeProcessError::Timeout(launch_timeout))??; | ||
| let pid: u32 = pid_output.trim().parse().map_err(|_| { | ||
| RuntimeProcessError::ExecutionFailed(format!( | ||
| "sandbox background launch did not report a pid: {pid_output:?}" | ||
| )) | ||
| })?; | ||
| Ok(BackgroundLaunch { | ||
| pid, | ||
| log_path: format!("/workspace/.ironclaw/bg-{pid}.log"), | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Background log path is reconstructed from the wrong PID ($$ vs $!).
The launch script redirects to bg-$$.log (the launcher sh PID), but the returned pid comes from echo $! (the backgrounded child) and log_path is rebuilt as bg-{pid}.log. $$ != $!, so the reported log path names a file that was never written. The "Started in background: pid X, log Y" message points the model at a dead path.
Emit the resolved path from the script rather than reconstructing it in Rust:
🐛 Proposed fix: echo pid + actual log path, parse both
- let launch_script = format!(
- "mkdir -p /workspace/.ironclaw && {} >>/workspace/.ironclaw/bg-$$.log 2>&1 & echo $!",
- wrap_command_for_pgid_isolation(&command),
- );
+ let launch_script = format!(
+ "mkdir -p /workspace/.ironclaw && {} >>/workspace/.ironclaw/bg-$$.log 2>&1 & \
+ echo \"$! /workspace/.ironclaw/bg-$$.log\"",
+ wrap_command_for_pgid_isolation(&command),
+ );Then parse pid and log_path from the two whitespace-separated tokens instead of format!("…/bg-{pid}.log").
🤖 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 `@crates/ironclaw_host_runtime/src/sandbox_process/exec_transport.rs` around
lines 299 - 352, Update the background launch flow around launch_script and
BackgroundLaunch so the shell emits both the background child PID ($!) and the
actual log path used by the redirection. Parse the two whitespace-separated
values from pid_output, use the first as pid, and store the second as log_path
instead of reconstructing the path from the PID.
| if profile == RebornCompositionProfile::HostedSingleTenantVolumeSandboxed { | ||
| return hosted_single_tenant_volume_sandboxed_build_input(owner_id, root); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Missing "+1" caller-level test for the Sandboxed dispatch arm.
The new dispatch arm at Line 540-542 routes HostedSingleTenantVolumeSandboxed to hosted_single_tenant_volume_sandboxed_build_input, but the only outermost-caller regression test (local_runtime_build_input_with_options_fails_closed_for_volume_profile_when_env_unset, Line 1309-1332) exercises RebornCompositionProfile::HostedSingleTenantVolume only — never the new Sandboxed arm through local_runtime_build_input_with_options. This is exactly the "distinct scenario" the neighboring Volume test was written to protect, and its own module doc frames it as the build-through-the-caller check.
✅ Proposed additional test
+ #[test]
+ fn local_runtime_build_input_with_options_fails_closed_for_sandboxed_profile_when_env_unset() {
+ let dir = tempfile::tempdir().expect("tempdir");
+ let root = dir.path().join("data-root");
+
+ let error = match local_runtime_build_input_with_options(
+ RebornCompositionProfile::HostedSingleTenantVolumeSandboxed,
+ "sandboxed-owner",
+ root,
+ RebornRuntimeProfileOptions::default(),
+ ) {
+ Ok(_) => panic!("the outermost production entry point must also fail closed"),
+ Err(error) => error,
+ };
+
+ assert!(
+ matches!(
+ &error,
+ RebornRuntimeProfileError::MissingSecretMasterKeyEnv { env_name }
+ if env_name == "IRONCLAW_REBORN_SECRET_MASTER_KEY"
+ ),
+ "expected MissingSecretMasterKeyEnv, got {error:?}"
+ );
+ }Based on coding guidelines, "Every new feature and bug fix must begin with a test... add a new test only for a genuinely distinct scenario and explain why an existing test could not absorb it" and "Test through the caller when a helper gates a side effect."
Also applies to: 627-671, 1302-1332
🤖 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 `@crates/ironclaw_reborn_composition/src/deployment.rs` around lines 540 - 542,
“Add an outermost-caller regression test covering the new
HostedSingleTenantVolumeSandboxed dispatch arm.” Extend the tests around
local_runtime_build_input_with_options and the existing
local_runtime_build_input_with_options_with_volume_profile test pattern to
invoke RebornCompositionProfile::HostedSingleTenantVolumeSandboxed through
local_runtime_build_input_with_options, asserting the same fail-closed behavior
when the required environment is unset; keep the existing
HostedSingleTenantVolume scenario unchanged.
Source: Coding guidelines
| /// Full proxy URL (e.g. `http://allowlist-proxy.internal:3128`) the | ||
| /// sandboxed shell container's `http_proxy`/`https_proxy` env should point | ||
| /// at. Takes priority over [`SANDBOX_HTTP_PROXY_PORT_ENV`] when both are set. | ||
| /// | ||
| /// Soft-enforcement model (mirrors legacy IronClaw's sandbox): the container | ||
| /// keeps normal bridge networking and is steered through this proxy, which | ||
| /// is expected to enforce the egress allowlist | ||
| /// (`ironclaw_host_runtime::sandbox_allowed_domains`) — see the follow-up | ||
| /// note below. | ||
| const SANDBOX_HTTP_PROXY_URL_ENV: &str = "IRONCLAW_SANDBOX_HTTP_PROXY"; | ||
|
|
||
| /// Port of an allowlist proxy reachable via the Docker host-gateway address | ||
| /// (`172.17.0.1` on Linux, `host.docker.internal` elsewhere — see | ||
| /// `RebornSandboxConfig::with_network_broker_port`). Used only when | ||
| /// [`SANDBOX_HTTP_PROXY_URL_ENV`] is unset; lets an operator run the proxy on | ||
| /// the host without hardcoding its address. | ||
| const SANDBOX_HTTP_PROXY_PORT_ENV: &str = "IRONCLAW_SANDBOX_HTTP_PROXY_PORT"; | ||
|
|
||
| /// Connect to the Docker daemon and build a `TenantSandbox` process-port | ||
| /// binding rooted at `sandbox_workspaces_root`. Fails closed: any Docker | ||
| /// connect failure returns `Err`, never a silent | ||
| /// `RebornRuntimeProcessBinding::none()` fallback (which would mean running | ||
| /// sandbox-profile shell commands unsandboxed on the host) — see | ||
| /// `docs/safety-and-sandbox.md`. | ||
| /// | ||
| /// Network egress: if [`SANDBOX_HTTP_PROXY_URL_ENV`] or | ||
| /// [`SANDBOX_HTTP_PROXY_PORT_ENV`] names a reachable proxy, the container | ||
| /// gets normal (bridge) networking plus `http_proxy`/`https_proxy` env | ||
| /// pointing at it, so pip/npm/git/curl workflows can reach the allowlisted | ||
| /// registries (`ironclaw_host_runtime::sandbox_allowed_domains`). Without | ||
| /// either env var the sandbox falls back to the prior `--network none` | ||
| /// posture — no egress at all, but still a safe default rather than a | ||
| /// build failure — until a proxy address is configured. | ||
| /// | ||
| /// TODO(follow-up, not built here): this only points the container at a | ||
| /// proxy address; it does not stand one up. The actual host-side allowlist | ||
| /// **proxy server** — a forward proxy that enforces | ||
| /// `ironclaw_host_runtime::sandbox_allowed_domains` and is reachable from | ||
| /// the sandbox container at the configured address — still needs to be | ||
| /// built and deployed. Until it lands, setting either env var here routes | ||
| /// the container's traffic at *something*, but that something must already | ||
| /// exist and enforce the allowlist, or the container effectively gets open | ||
| /// egress. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Soft-enforcement egress proxy: opt-in knob outruns its enforcement point.
SANDBOX_HTTP_PROXY_URL_ENV/SANDBOX_HTTP_PROXY_PORT_ENV only set http_proxy/https_proxy on an otherwise normal bridge-networked container; nothing in this wiring enforces that egress actually goes through the named proxy. Any in-container process that ignores those env vars (raw sockets, curl --noproxy, etc.) gets open egress unless the proxy address given also happens to be backed by a real network-level enforcement point — which the TODO admits doesn't exist yet. Until the allowlist-enforcing proxy lands, enabling either env var trades the safe --network none default for an egress posture that looks allowlisted but isn't actually bounded.
As per coding guidelines: "Fail closed for authentication, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity" and "Do not weaken authentication, origin checks, body limits, rate limits, allowlists, approval leases, secret mediation, or redaction guarantees."
🤖 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 `@crates/ironclaw_reborn_composition/src/sandbox_boot.rs` around lines 25 - 67,
Update the network configuration around SANDBOX_HTTP_PROXY_URL_ENV and
SANDBOX_HTTP_PROXY_PORT_ENV so configuring either variable cannot enable bridge
networking or imply enforced egress before a real allowlist proxy exists.
Preserve the fail-closed RebornRuntimeProcessBinding behavior by retaining the
--network none posture unless network-level proxy enforcement is verified and
explicitly supported. Remove or disable the soft proxy environment wiring in the
TenantSandbox setup and revise nearby documentation to match the safe behavior.
Source: Coding guidelines
| //! Tenant-level concurrency ceiling for the `hosted-single-tenant-volume-sandboxed` | ||
| //! profile (D3-2). | ||
| //! | ||
| //! `ironclaw_authorization::obligations_for_grant` already emits a | ||
| //! `ReserveResources` obligation for every `EffectKind::SpawnProcess` | ||
| //! capability grant (D3-1), and `ironclaw_host_runtime::obligations:: | ||
| //! reserve_resource_obligation` already reserves against whatever | ||
| //! `ResourceGovernor` composition wires in. Both are no-ops today because no | ||
| //! deployment ever calls `set_limit` for a `SpawnProcess`-relevant account — | ||
| //! this module is the one caller that does, for the sandboxed profile only. | ||
| //! | ||
| //! Kept as its own module (not inlined into `factory.rs`, which is already | ||
| //! thousands of lines) so the boot call site stays a single line. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Stale "tenant-level"/"per-tenant" doc wording contradicts the per-user implementation.
The module doc and SANDBOX_MAX_CONCURRENT_ENV's doc comment (lines 20-22) both describe this as a tenant-level/per-tenant ceiling, but apply_sandbox_user_ceiling scopes via ResourceAccount::user(...), and this file's own test explicitly proves a sibling user in the same tenant is unaffected. sandbox_composition.rs correctly documents the same feature as "scoped per-user (not per-tenant)". Update the stale wording here to avoid misleading future readers about the actual quota boundary.
📝 Proposed doc fix
-//! Tenant-level concurrency ceiling for the `hosted-single-tenant-volume-sandboxed`
+//! Per-user concurrency ceiling for the `hosted-single-tenant-volume-sandboxed`
//! profile (D3-2).-/// Overrides the sandboxed profile's per-tenant concurrent `SpawnProcess`
+/// Overrides the sandboxed profile's per-user concurrent `SpawnProcess`
/// ceiling. Unset, or set to a non-positive/unparseable value, falls back to
/// [`DEFAULT_SANDBOX_MAX_CONCURRENT`].📝 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.
| //! Tenant-level concurrency ceiling for the `hosted-single-tenant-volume-sandboxed` | |
| //! profile (D3-2). | |
| //! | |
| //! `ironclaw_authorization::obligations_for_grant` already emits a | |
| //! `ReserveResources` obligation for every `EffectKind::SpawnProcess` | |
| //! capability grant (D3-1), and `ironclaw_host_runtime::obligations:: | |
| //! reserve_resource_obligation` already reserves against whatever | |
| //! `ResourceGovernor` composition wires in. Both are no-ops today because no | |
| //! deployment ever calls `set_limit` for a `SpawnProcess`-relevant account — | |
| //! this module is the one caller that does, for the sandboxed profile only. | |
| //! | |
| //! Kept as its own module (not inlined into `factory.rs`, which is already | |
| //! thousands of lines) so the boot call site stays a single line. | |
| //! Per-user concurrency ceiling for the `hosted-single-tenant-volume-sandboxed` | |
| //! profile (D3-2). | |
| //! | |
| //! `ironclaw_authorization::obligations_for_grant` already emits a | |
| //! `ReserveResources` obligation for every `EffectKind::SpawnProcess` | |
| //! capability grant (D3-1), and `ironclaw_host_runtime::obligations:: | |
| //! reserve_resource_obligation` already reserves against whatever | |
| //! `ResourceGovernor` composition wires in. Both are no-ops today because no | |
| //! deployment ever calls `set_limit` for a `SpawnProcess`-relevant account — | |
| //! this module is the one caller that does, for the sandboxed profile only. | |
| //! | |
| //! Kept as its own module (not inlined into `factory.rs`, which is already | |
| //! thousands of lines) so the boot call site stays a single line. |
🤖 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 `@crates/ironclaw_reborn_composition/src/sandbox_quota.rs` around lines 1 - 14,
Update the module documentation and the SANDBOX_MAX_CONCURRENT_ENV documentation
to describe the concurrency ceiling as scoped per user, not tenant-wide or
per-tenant. Keep the implementation in apply_sandbox_user_ceiling unchanged,
including its ResourceAccount::user scoping, and align the wording with
sandbox_composition.rs.
| mkdir -p "$HOME" 2>/dev/null || true | ||
| [ -d /home/sandbox/.cargo ] && [ ! -d /workspace/.home/.cargo ] && cp -a /home/sandbox/.cargo /workspace/.home/.cargo 2>/dev/null || true | ||
| [ -d /home/sandbox/.rustup ] && [ ! -d /workspace/.home/.rustup ] && cp -a /home/sandbox/.rustup /workspace/.home/.rustup 2>/dev/null || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not hide persistent-home initialization failures.
A failed cp can leave a partial destination directory; later starts see it and never retry, leaving that user’s Rust environment permanently broken. Remove || true and stage copies atomically before moving them into place.
As per path instructions, the Fail loud invariant requires propagating initialization errors rather than continuing with poisoned state.
🤖 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 `@docker/process-sandbox-entrypoint.sh` around lines 52 - 54, Update the
persistent-home initialization commands in the entrypoint to propagate mkdir and
copy failures instead of suppressing them. Stage the /home/sandbox/.cargo and
/home/sandbox/.rustup copies in temporary destinations under /workspace/.home,
then atomically move each completed stage into place so failed copies cannot
leave directories that prevent retries.
Source: Path instructions
| # additionally sets CARGO_HOME=/workspace/.home/.cargo, RUSTUP_HOME=/workspace/.home/.rustup | ||
| # with a first-run copy step in the entrypoint script — see entrypoint change below. | ||
| USER sandbox | ||
| RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make Rust bootstrap fail on download errors.
With /bin/sh, a failed curl can feed an empty script to sh, which exits successfully; the image then builds without Rust. Download first, then execute the installer.
Proposed fix
-RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal
+RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs -o /tmp/rustup-init \
+ && sh /tmp/rustup-init -y --profile minimal \
+ && /home/sandbox/.cargo/bin/cargo --version \
+ && rm /tmp/rustup-initAs per path instructions, the Fail loud invariant rejects silent-failure patterns.
📝 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.
| RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal | |
| RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs -o /tmp/rustup-init \ | |
| && sh /tmp/rustup-init -y --profile minimal \ | |
| && /home/sandbox/.cargo/bin/cargo --version \ | |
| && rm /tmp/rustup-init |
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 57-57: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check
(DL4006)
🤖 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 `@Dockerfile.process-sandbox` at line 57, Update the Rust bootstrap RUN command
in Dockerfile.process-sandbox to download the installer with curl into a
temporary file first, then execute that file with sh using the existing rustup
arguments. Preserve strict curl failure behavior and ensure the installer is
only run after a successful download.
Sources: Path instructions, Linters/SAST tools
|
🚅 Deployed to the ironclaw-pr-6584 environment in ironclaw-ci-preview
|
Stack 1/3 — Phase A of the persistent sandbox program. Next:
sandbox/v1-cli-session, thensandbox/v1-egress.Replaces ephemeral per-command containers with a persistent container keyed by
{tenant, user}, so live processes (dev servers now, CLIs later) survive across commands.RebornSandboxUserKeyidentity + labels-as-identity registry with a push-based activity mapbackground: truedetached execution with a live-process footerResourceAccount::user, was tenant)/workspace/workspacemounted at the abstract-FS root with byte-parity coverageDocker-gated tests self-skip without a daemon (
SKIP:line, never#[ignore]) — CI is the arbiter for those.🤖 Generated with Claude Code