feat(sandbox): leaf-scoped mount containment + per-user sandbox identity primitives - #6695
Conversation
…ity primitives
Ships two related, unwired-by-design slices of the persistent per-user
sandbox program:
- ironclaw_filesystem: leaf-scoped mount containment. resolve_joined now
returns a per-request containment_root that, for a leaf_scoped mount, is
host_root/<first-tail-segment> instead of the shared host_root — closing
a same-mount cross-leaf symlink escape a plain mount_local containment
check would miss. mount_local_per_leaf is the constructor; a bare-root
request against such a mount is rejected outright (no safe containment
root for "every caller's leaf").
- ironclaw_host_runtime: identity + attribution primitives for the
persistent per-user sandbox container model — RebornSandboxUserKey
({tenant,user}-only container/workspace key), the labels-as-identity
registry (Docker label helpers, SandboxActivityRegistry,
BackgroundJobRegistry), and ConnectionAttributionResolver (source-IP to
{tenant,user} resolution for the shared egress proxy, design decision D9).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…llback docker_gate::docker_available() shells out to the docker CLI, which resolves the daemon through whatever context is active (Colima, Docker Desktop, a remote host). The test then connected directly via Docker::connect_with_local_defaults(), which only honors DOCKER_HOST or the hardcoded /var/run/docker.sock, so the gate could pass while the connection still failed on any machine using a non-default socket. Reuse sandbox_process::connect_docker() instead of reimplementing resolution: it already tries connect_with_local_defaults() then falls back through unix_socket_candidates(), the same path production containers connect through. A connect_docker() failure now prints a SKIP line rather than panicking. Co-Authored-By: Claude Opus 5 (1M context) <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
size: XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds leaf-scoped filesystem mounts, per-user sandbox identity and registries, Docker network attribution with caching, Docker test gating, shared digest encoding, and public runtime re-exports. ChangesLeaf-scoped filesystem containment
Sandbox runtime foundations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Proxy
participant ConnectionAttributionResolver
participant Docker
participant ContainerLabels
Proxy->>ConnectionAttributionResolver: resolve(peer_ip)
ConnectionAttributionResolver->>Docker: containers_on_network(network)
Docker-->>ConnectionAttributionResolver: container summaries and network IPs
ConnectionAttributionResolver->>ContainerLabels: validate tenant/user labels
ContainerLabels-->>ConnectionAttributionResolver: Attributed or Unattributed
ConnectionAttributionResolver-->>Proxy: attribution result
Possibly related issues
Possibly related PRs
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 |
![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: 433c97a0b7
ℹ️ 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".
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | 433c97a0b7a0 |
Head: 433c97a0b7a010dd8d73d2c5d51f5a849e8bc400
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Changes requested: the new sandbox backend drops per-request mount scope, and attribution can return a stale tenant/user after Docker IP reuse.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Derive Docker mount roots from the request scope
Location: crates/ironclaw_process_sandbox/src/docker.rs:485
ProcessSandboxExecutor drops ProcessExecutionRequest.mounts, and this backend binds the static configuration roots for every request. A singleton executor therefore gives distinct tenant/project scopes the same workspace, tools, and cache directories, allowing cross-scope reads and writes. Resolve trusted host roots from the request's scoped mount authority and add a two-scope isolation test.
2. ❌ [HIGH] Do not cache attribution across container lifetimes
Location: crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:188
This returns a positive IP-only cache entry without checking whether Docker has recycled that IP. Teardown invalidation is not wired, so a new user's connection within the five-second TTL can be attributed to the previous user and receive that user's injected egress credential. Make attribution cache-safe across container replacement (or invalidate synchronously on teardown) and add an IP-reuse regression test.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
Inline review fallback
Inline comment projection fell back to a body-only PR Review because GitHub rejected the inline payload.
Reason: Unprocessable Entity: "Path could not be resolved" - https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request
IronLoop preserved the inline review comment payloads below instead of dropping them.
Inline fallback 1: crates/ironclaw_process_sandbox/src/docker.rs:485
ProcessExecutionRequest.mounts is dropped before reaching this backend, and all invocations bind the static config roots here. Since the host holds one ProcessExecutor, requests from distinct tenant/project scopes will share workspace/tools/cache directories. Resolve trusted roots per request from scoped mount authority and add an A/B scope-isolation test.
Inline fallback 2: crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:188
An IP-only cache can return a stale identity after Docker reuses an IP. No teardown invalidation is wired, so a new user's connection within five seconds is returned as the previous user and would receive that user's egress credentials. Do not cache positive attribution across container lifetimes, or make invalidation synchronous and automatic.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add leaf-scoped mount containment and per-user sandbox identity primitives while deferring production wiring and Docker escape coverage.
Stats: 9 findings (from 9 raw, 9 after overlap/same-line dedup) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach (completed sequentially in the parent after platform parallel-agent cap). Reviewers failed: none; parallel fan-out was unavailable after the intent/partial lanes due to the active-agent thread cap. Body-only: 0.
The two High findings need resolution before the deferred transport/composition slices rely on these primitives. The remaining findings are coverage, bounded-cache, or maintainability follow-ups.
Security
- High IP-only attribution cache can cross-attribute a recycled Docker IP (
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:135-138, confidence 92) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:136
The resolver caches an attributed{tenant,user}solely by peer IP and accepts that value for up to five seconds. If a container is torn down and Docker assigns the same IP to another tenant during that window, the next connection from the new container is attributed to the old owner. The module explicitly acknowledges this path, but the consumer is intended to choose credentials from this result, so a bounded stale window is still a cross-tenant credential-confusion vulnerability rather than fail-closed behavior.
Bugs
- High Leaf-scoped create operations reject a brand-new leaf (
crates/ironclaw_filesystem/src/local.rs:188-195, confidence 95) — anchor:crates/ironclaw_filesystem/src/local.rs:188; also flagged by tests/Medium
For a path such as/tmp/<new-user>/file,ensure_existing_ancestor_containedwalks up to the existing sharedhost_rootbecause the leaf does not exist yet, then checks that ancestor againsthost_root/<leaf>. That check necessarily fails, soresolve_for_create_dir_alland the missing-file branch ofresolve_for_writecannot create the first directory for a new user. The newly added tests only cover pre-existing leaves, leaving the normal first-use path broken.
Performance
- Medium Expired attribution entries are never evicted (
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:148-148, confidence 94) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:148
The TTL only causes cache misses; expired entries remain in the HashMap indefinitely. Once the proxy wires this resolver, each previously unseen peer IP permanently retains a CacheEntry, so long-running container churn can grow the cache without bound.
Tests
- Medium Write-path leaf escape is not tested (
crates/ironclaw_filesystem/src/local.rs:143-169, confidence 95) — anchor:crates/ironclaw_filesystem/src/local.rs:159
The new leaf-scoped containment is exercised only through read_file. The distinct resolve_for_write path, including canonicalized-parent validation, has no test proving a cross-leaf symlink cannot redirect writes. - Low Candidate parsing lacks malformed-summary coverage (
crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:97-103, confidence 100) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:97
UserContainerCandidate::from_summary has untested failure branches for a missing container ID and a malformed created_at label; the current test covers only missing labels. - Low Malformed or missing network IPs are untested (
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:261-272, confidence 100) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:261
container_ip_on_network explicitly handles absent network settings, absent network maps, missing network entries, empty strings, and unparseable IPs, but the attribution tests cover only valid IPs and an unknown valid IP. - Low Background job registry has no behavior tests (
crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:183-217, confidence 100) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:183
The new BackgroundJobRegistry is entirely untested, leaving record, per-user isolation, empty lookup, and drop_dead filtering behavior uncovered. - Low Activity registry mutex contention is untested (
crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:135-159, confidence 95) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:139
SandboxActivityRegistry uses a shared Mutex and is intended for concurrent exec transport and reaper access, but existing tests are entirely sequential and do not exercise concurrent touch/read/forget operations.
Maintainability
- Low Remove temporary PR-state details from module comments (
crates/ironclaw_host_runtime/src/sandbox_process.rs:37-46, confidence 85) — anchor:crates/ironclaw_host_runtime/src/sandbox_process.rs:37
This module comment records the PR's sequencing, omitted future files, and anticipated consumers rather than stable code behavior. It will become stale as the follow-up transport wiring lands and duplicates the PR description, making the module header noisier to navigate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/ironclaw_filesystem/src/local.rs`:
- Around line 187-201: Update ensure_existing_ancestor_contained and the
create-directory flow around resolve_joined so the shared mount root is accepted
as a bootstrap ancestor when the target leaf is absent. Run create_dir_all
first, then canonicalize the result and enforce containment with
ensure_contained against the leaf boundary. Add a caller-level regression test
covering creation of a previously absent leaf directory.
In `@crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs`:
- Around line 250-253: Add a tracing::debug! call in the
parse_attribution_labels None branch within the surrounding attribution method
before returning ConnectionAttribution::Unattributed, including enough context
to identify the container or label parsing failure while preserving the existing
fail-closed behavior.
In `@crates/ironclaw_host_runtime/src/sandbox_process/registry.rs`:
- Around line 175-218: Add unit tests for BackgroundJobRegistry covering record
and jobs_for storage/retrieval, including isolation by RebornSandboxUserKey, and
drop_dead retaining only jobs whose PIDs appear in alive_pids. Follow the direct
registry-test style used for SandboxActivityRegistry, without adding production
callers or unrelated refactoring.
- Line 22: Replace the sibling-module super imports with crate-rooted imports to
follow the repository convention: update registry.rs lines 22-22 for
RebornSandboxUserKey and attribution.rs lines 62-62 for label_tenant and
label_user, using crate::sandbox_process paths in both locations.
🪄 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: 11885cc2-e70a-4357-a4a9-611dfb5999af
📒 Files selected for processing (7)
crates/ironclaw_filesystem/src/local.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/sandbox_process.rscrates/ironclaw_host_runtime/src/sandbox_process/attribution.rscrates/ironclaw_host_runtime/src/sandbox_process/registry.rscrates/ironclaw_host_runtime/src/sandbox_process/user_key.rscrates/ironclaw_host_runtime/tests/support/docker_gate.rs
|
🚅 Deployed to the ironclaw-pr-6695 environment in ironclaw-ci-preview
|
…ene, docker CI gate Leaf-scoped mounts rejected a brand-new leaf's first write/create_dir_all (ensure_existing_ancestor_contained had no bootstrap case for the shared host_root when a caller's leaf doesn't exist yet); accept that one ancestor now and add regression coverage for write-path creation, write-path cross-leaf symlink escape, and create_dir_all bootstrap. Attribution cache: sweep expired entries on miss so a long-running resolver doesn't grow the cache unboundedly, and log the missing/malformed-label fail-closed branch like its sibling branches. Add coverage for malformed/ missing container network IPs. Docker CI gate: the attribution real-Docker test's connect_docker() failure branch always skipped, even under IRONCLAW_REQUIRE_DOCKER_TESTS=1 — panic in that mode instead, matching docker_gate's existing fail-closed pattern. Pull busybox:1.36 before create_container so the test doesn't depend on a pre-warmed local image cache (this is what broke it in CI). registry.rs/attribution.rs: crate::-rooted imports per repo convention; trim sandbox_process.rs's module header to stable ownership, not PR-state. Add BackgroundJobRegistry, malformed-candidate-parsing, and concurrent SandboxActivityRegistry coverage. docs/reborn/contracts/host-runtime.md: one forward-pointing sentence noting RebornSandboxUserKey's future coarser identity model doesn't yet supersede the scope-derived identity this contract already documents. architecture ratchet: baseline the two new test/dead-code seams this introduces (attribution.rs dead-code method x4, test-support method x1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressing the one review finding with no inline thread (ironloop's review fell back to body-only because GitHub rejected its inline payload — "Path could not be resolved"): "Derive Docker mount roots from the request scope" ( ironloop's second blocking finding (attribution cache IP reuse, |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_filesystem/src/local.rs (1)
583-634: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winDo not allow
host_rootbootstrap whencontainment_rootalready exists.
ensure_existing_ancestor_containedonly matches the canonical ancestor againstbootstrap_root, so a symlink inside an already-created leaf that points back tohost_rootcan satisfy the exception and letcreate_dir_all(parent)create a directory outsidecontainment_rootbefore the later canonical check rejects the path. Clamp the bootstrap path to!containment_root.exists()and add a regression for this symlink case.Violates
crates/AGENTS.mdbackend containment invariant: backends must remain contained against symlink traversal, mount escape, and raw-host-path access.🤖 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_filesystem/src/local.rs` around lines 583 - 634, The bootstrap exception in ensure_existing_ancestor_contained must only apply when containment_root does not already exist. Require the bootstrap check to be gated by the absence of containment_root, preventing symlinks within an existing leaf from redirecting directory creation to host_root. Add a regression test covering an existing leaf containing a symlink to host_root and verifying creation outside containment_root is rejected.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@crates/ironclaw_filesystem/src/local.rs`:
- Around line 583-634: The bootstrap exception in
ensure_existing_ancestor_contained must only apply when containment_root does
not already exist. Require the bootstrap check to be gated by the absence of
containment_root, preventing symlinks within an existing leaf from redirecting
directory creation to host_root. Add a regression test covering an existing leaf
containing a symlink to host_root and verifying creation outside
containment_root is rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30ac60df-e3ae-4361-a964-5ef64f3dd2ac
📒 Files selected for processing (7)
crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rscrates/ironclaw_filesystem/src/local.rscrates/ironclaw_host_runtime/src/sandbox_process.rscrates/ironclaw_host_runtime/src/sandbox_process/attribution.rscrates/ironclaw_host_runtime/src/sandbox_process/registry.rscrates/ironclaw_host_runtime/tests/support/docker_gate.rsdocs/reborn/contracts/host-runtime.md
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Introduce leaf-scoped filesystem mount containment and per-user sandbox identity primitives as intentionally unwired building blocks for a future persistent per-user sandbox container program.
Stats: 6 findings (from 6 raw, 4 after dedup) across 2 files. Reviewers run: security, performance, tests, conventions. Reviewers failed: bugs, local-patterns, maintainability, approach. Body-only: 0
Security
- High Cached IP attribution can inject credentials for the wrong user (
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:125-143, confidence 90) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:125
The resolver caches an IP-to-user attribution for up to five seconds, but Docker reuses container IPs after teardown. If a new user's container receives the old IP before the TTL expires, requests from that container are attributed to the previous user and the future credential firewall can inject the wrong user's secret.
Fix: Resolve attribution per accepted connection or bind the cache entry to a verified container identity and synchronously invalidate it before IP reuse.
Performance
- Medium Concurrent cache misses all query Docker independently (
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:187-205, confidence 91) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:187
The cache is checked before the async Docker query and only locked afterward, so concurrent resolutions for uncached IPs each issue a full container-list request. During connection bursts this creates a thundering herd against Docker and repeatedly scans the entire network container list.
Fix: Coalesce in-flight resolutions or add a short-lived shared snapshot so concurrent misses await one Docker listing. - Medium Background job registry can grow without bound (
crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:183-210, confidence 88) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:183
Every background launch appends a String-bearing job to a per-user Vec, and jobs_for clones the entire vector. Until drop_dead runs successfully, repeated launches retain all entries and cause O(J) allocation/copying on every foreground lookup, allowing memory and latency to grow with job history.
Fix: Bound retained jobs and command-preview sizes, and return a bounded snapshot or maintain only currently live jobs keyed by PID. - Low Dead-job pruning is O(jobs x alive PIDs) (
crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:213-216, confidence 84) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:213
drop_dead calls alive_pids.contains for every retained job, making cleanup O(JxA). With many tracked background jobs and processes, the reaper repeatedly performs a linear PID scan for each job.
Fix: Convert alive_pids to a HashSet before retain and use constant-time membership checks.
Tests
- Medium Empty alive PID lists are not tested (
crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:213-216, confidence 90) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/registry.rs:213
The collection parameter is tested with multiple live and dead PIDs, but not an empty list. An empty process listing should remove every tracked job; without coverage, stale jobs could remain when the sandbox has no live background processes.
Fix: tests::registry::background_job_registry_drop_dead_with_empty_alive_pids_removes_all_jobs covering an empty alive_pids slice - Low Concurrent attribution cache access is untested (
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:187-205, confidence 75) — anchor:crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:187
The resolver performs an async Docker query outside its Mutex and then reinserts the result, but no test invokes resolve concurrently for the same or different IPs. This leaves the cache miss, query, sweep, and insert interaction under contention unexercised.
Fix: tests::attribution::concurrent_resolve_calls_complete_with_consistent_attribution covering simultaneous resolve calls sharing the resolver cache
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.56% — 308568 / 360641 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
…and concurrent attribution resolve Addresses review 4784267719 on PR #6695: - BackgroundJobRegistry::drop_dead now builds a HashSet once instead of a per-job linear scan of alive_pids (O(J) not O(J x A)). - Add background_job_registry_drop_dead_with_empty_alive_pids_removes_all_jobs. - Add concurrent_resolve_calls_complete_with_consistent_attribution covering simultaneous ConnectionAttributionResolver::resolve calls. - Doc-comment the two known-but-deferred tradeoffs (attribution thundering herd, unbounded BackgroundJobRegistry growth) — both need the not-yet-wired caller (W6 / exec_transport+reaper) to know the right shape, so building a mechanism now would be guessing; left as explicit follow-ups instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs`:
- Around line 682-726: Update
concurrent_resolve_calls_complete_with_consistent_attribution to configure
FakeLookup with a controlled await and Barrier for all 20 lookups, ensuring each
cache miss reaches the synchronization point before any proceeds. Release the
barrier once every spawned resolve is waiting, then retain the existing
attribution assertions and panic/deadlock checks.
🪄 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: 677d7741-67de-4a35-9003-de0cc2284423
📒 Files selected for processing (2)
crates/ironclaw_host_runtime/src/sandbox_process/attribution.rscrates/ironclaw_host_runtime/src/sandbox_process/registry.rs
|
Addressing both findings from this review round (body-only again — no inline thread was created; ironloop's own note confirms the inline payload was rejected: "Path could not be resolved"): 1. [HIGH, blocking] "Do not cache an attributed identity by reusable IP" ( 2. [MEDIUM, note] "Keep Docker sandbox parity marked as unwired" ( No code changes from this round. |
|
@ironloopai review |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 7259abf98ff5 |
Head: 7259abf98ff5326c37af1bd4680a32de02aa4ed8
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Changes requested: the new attribution cache can leak credentials across Docker IP reuse.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Do not cache attributed identities by reusable IP
Location: crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:209-210
This cache is keyed only by peer IP, not a TCP connection or container identity. After container A is removed and Docker assigns its IP to container B, B's new connection can arrive within the 5-second TTL and receive A's cached {tenant,user}; the planned credential injector would then use A's credentials for B. Resolve attribution per accepted connection, or make lifecycle invalidation race-free before allowing any reuse.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
Addresses coderabbitai review 4790451629 on PR #6695: try_exists follows symlinks and reports false for a dangling one, so a pre-planted dangling symlink at the write target fell through to the brand-new-file bootstrap path. write_file/append_file open with O_CREAT, so the OS would create the file wherever the symlink points, escaping leaf containment. resolve_for_write now checks existence via symlink_metadata (lstat) and fails closed with SymlinkEscape when a symlink entry can't be canonicalized, instead of silently falling through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round 5 triage of two body-only findings (GitHub rejected inline payloads, "Path could not be resolved"): coderabbitai — dangling final symlink in resolve_for_write (local.rs:145-189, outside diff range). Valid and fixed in f00f1fe. try_exists follows symlinks and reports false for a dangling one, so a pre-planted dangling symlink at the write target fell through to the "brand new file in this leaf" bootstrap path — write_file/append_file open with O_CREAT, so the OS would create the file wherever the symlink points, escaping leaf containment. Added a red/green regression (leaf_scoped_mount_rejects_dangling_final_symlink_escape_on_write, confirmed it failed against the buggy code first) and switched the existence check to symlink_metadata (lstat), failing closed with SymlinkEscape when the entry is a symlink that cannot be canonicalized. Documented in docs/reborn/contracts/filesystem.md (leaf-scoped bullet list + section 15 coverage list). ironloopai — attribution cache IP-reuse (attribution.rs:209). Already decided (round 4, decision #1): ship the TTL-window cache as-is; the real fix needs the unbuilt reaper, and invalidate()'s doc already says "toward zero" not "to zero" for exactly this reason. Restated on the inline thread (discussion_r3660244126) and resolved rather than re-touched, since this is a repeat of a settled decision. ironloopai — docs/reborn/engine-v2-to-reborn-parity.md:63 (Docker sandbox parity marking). Out of scope: git diff main...HEAD --stat confirms this PR only touches docs/reborn/contracts/filesystem.md and host-runtime.md; the parity doc was last touched by #6670. Not editing it here — flagging as a follow-up for whoever next touches that doc. |
|
@ironloopai review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_filesystem/src/local.rs (1)
190-211: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftMake leaf containment atomic with the filesystem mutation.
Pathname checks do not hold the checked object. A process permitted to alter
leaf-acan replace a checked child directory with a../leaf-bsymlink in the gap: the write flow can create/overwrite sibling data, whilecreate_dir_allcan create sibling directories before its later check rejects the path. Use descriptor/capability-rooted no-follow traversal through the final operation, and add a caller-level race regression.
crates/ironclaw_filesystem/src/local.rs#L190-L211: retain no-follow directory handles through the eventual file open/create.crates/ironclaw_filesystem/src/local.rs#L227-L234: perform directory creation through the same containment-rooted handles, before reporting success.As per coding guidelines,
ironclaw_filesystemmust remain contained against symlink traversal and mount escape, and side-effect gates require caller-level tests.🤖 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_filesystem/src/local.rs` around lines 190 - 211, Replace the pathname-based final containment and mutation flow around the local filesystem write/open operation (crates/ironclaw_filesystem/src/local.rs:190-211) with descriptor/capability-rooted, no-follow traversal that retains directory handles through the eventual file open/create, preventing symlink or mount escapes between validation and mutation. Update directory creation in the corresponding create-directory path (crates/ironclaw_filesystem/src/local.rs:227-234) to use the same containment-rooted handles before reporting success, and add a caller-level race regression test covering replacement of a checked child with a symlink.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@crates/ironclaw_filesystem/src/local.rs`:
- Around line 190-211: Replace the pathname-based final containment and mutation
flow around the local filesystem write/open operation
(crates/ironclaw_filesystem/src/local.rs:190-211) with
descriptor/capability-rooted, no-follow traversal that retains directory handles
through the eventual file open/create, preventing symlink or mount escapes
between validation and mutation. Update directory creation in the corresponding
create-directory path (crates/ironclaw_filesystem/src/local.rs:227-234) to use
the same containment-rooted handles before reporting success, and add a
caller-level race regression test covering replacement of a checked child with a
symlink.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97fe62f9-07eb-43b2-84ae-823b26f49c04
📒 Files selected for processing (2)
crates/ironclaw_filesystem/src/local.rsdocs/reborn/contracts/filesystem.md
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | f00f1fe67966 |
Head: f00f1fe679669397a6a6aed83c61098c68bdcf2e
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The comparison reintroduces the legacy Docker process-sandbox backend that base main removed. Its mount and egress behavior do not satisfy the current sandbox isolation contract.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Derive Docker mounts from the request scope
Location: crates/ironclaw_process_sandbox/src/docker.rs:486
DockerProcessSandboxConfig supplies one fixed workspace_host_path, and this invocation builder never receives SandboxProcessRequest.scope or a trusted MountView. If this re-exported executor is selected, every tenant is therefore given the same workspace bind and can read or modify another scope's files. Keep this removed legacy backend out of the PR, or derive each source from the request's trusted mount grants before creating the container.
2. ❌ [HIGH] Enforce broker-only egress at the network layer
Location: crates/ironclaw_process_sandbox/src/docker.rs:426
Credentialed runs select Docker's default bridge network. The proxy environment variables and IRONCLAW_EGRESS_LOCKDOWN string are opt-in conventions, so sandboxed code can unset/bypass them and connect directly to arbitrary hosts, bypassing runtime_hosts and the claimed direct-egress lockdown. Use an isolated network/socket design that can reach only the broker, and add a real enforcement test.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
Inline review fallback
Inline comment projection fell back to a body-only PR Review because GitHub rejected the inline payload.
Reason: Unprocessable Entity: "Path could not be resolved and Path could not be resolved" - https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request
IronLoop preserved the inline review comment payloads below instead of dropping them.
Inline fallback 1: crates/ironclaw_process_sandbox/src/docker.rs:486
This uses one fixed configured workspace path without request scope or MountView resolution. Selecting this executor would bind the same workspace into every tenant's command. Derive sources from trusted per-request mount grants before container creation.
Inline fallback 2: crates/ironclaw_process_sandbox/src/docker.rs:426
bridge permits direct outbound traffic; proxy environment variables do not enforce broker-only egress. A command can bypass them and ignore runtime_hosts, so use a network/socket design that only permits the broker.
coderabbitai (round 7) flagged that leaf containment checks aren't atomic with the mutation they gate. The gap is real but matches the already-deferred residual documented on resolve_for_write's re-rooting step (full fix needs openat/O_NOFOLLOW/cap-std, tracked via PR #2996 review) — extend that documentation to resolve_for_create_dir_all and delete instead of re-litigating the same heavy-lift follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reply to coderabbitai's review (round 7, review id 4791130037) — body-only "outside diff range" finding on Valid, not new. The TOCTOU window between the containment check and the eventual mutation is real: it's the same residual already called out in the Resolution (b8650a4): extended that same documentation to |
|
Reply to ironloopai's review (round 7, review id 4791164423) — both blocking findings are on Invalid for this PR — file not touched. |
|
@ironloopai review |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | b8650a4d690e |
Head: b8650a4d690ed3e17a762969ae74f26cd6339a6c
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a high-severity cross-tenant credential-attribution flaw: positive attribution is cached solely by reusable Docker IP.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Do not cache attributed identities by reusable IP
Location: crates/ironclaw_host_runtime/src/sandbox_process/attribution.rs:209
A positive cache entry is keyed only by peer_ip. If Docker tears down tenant A's container and assigns that IP to tenant B within the 5-second TTL, B's new connection receives A's cached identity. The future credential-injection consumer would then inject A's credentials into B's request. invalidate does not close this path: no lifecycle caller is wired, and the documented query/invalidate race can reinsert stale data. Do not cache positive attributions by bare IP; revalidate each new connection or bind cache validity to a non-reusable container/lifecycle generation.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
…ed-containment # Conflicts: # crates/ironclaw_host_runtime/src/sandbox_process.rs
…ity primitives (nearai#6695) * feat(sandbox): leaf-scoped mount containment + per-user sandbox identity primitives Ships two related, unwired-by-design slices of the persistent per-user sandbox program: - ironclaw_filesystem: leaf-scoped mount containment. resolve_joined now returns a per-request containment_root that, for a leaf_scoped mount, is host_root/<first-tail-segment> instead of the shared host_root — closing a same-mount cross-leaf symlink escape a plain mount_local containment check would miss. mount_local_per_leaf is the constructor; a bare-root request against such a mount is rejected outright (no safe containment root for "every caller's leaf"). - ironclaw_host_runtime: identity + attribution primitives for the persistent per-user sandbox container model — RebornSandboxUserKey ({tenant,user}-only container/workspace key), the labels-as-identity registry (Docker label helpers, SandboxActivityRegistry, BackgroundJobRegistry), and ConnectionAttributionResolver (source-IP to {tenant,user} resolution for the shared egress proxy, design decision D9). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): attribution real-docker test reuses connect_docker() fallback docker_gate::docker_available() shells out to the docker CLI, which resolves the daemon through whatever context is active (Colima, Docker Desktop, a remote host). The test then connected directly via Docker::connect_with_local_defaults(), which only honors DOCKER_HOST or the hardcoded /var/run/docker.sock, so the gate could pass while the connection still failed on any machine using a non-default socket. Reuse sandbox_process::connect_docker() instead of reimplementing resolution: it already tries connect_with_local_defaults() then falls back through unix_socket_candidates(), the same path production containers connect through. A connect_docker() failure now prints a SKIP line rather than panicking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address PR review — leaf-creation bug, attribution hygiene, docker CI gate Leaf-scoped mounts rejected a brand-new leaf's first write/create_dir_all (ensure_existing_ancestor_contained had no bootstrap case for the shared host_root when a caller's leaf doesn't exist yet); accept that one ancestor now and add regression coverage for write-path creation, write-path cross-leaf symlink escape, and create_dir_all bootstrap. Attribution cache: sweep expired entries on miss so a long-running resolver doesn't grow the cache unboundedly, and log the missing/malformed-label fail-closed branch like its sibling branches. Add coverage for malformed/ missing container network IPs. Docker CI gate: the attribution real-Docker test's connect_docker() failure branch always skipped, even under IRONCLAW_REQUIRE_DOCKER_TESTS=1 — panic in that mode instead, matching docker_gate's existing fail-closed pattern. Pull busybox:1.36 before create_container so the test doesn't depend on a pre-warmed local image cache (this is what broke it in CI). registry.rs/attribution.rs: crate::-rooted imports per repo convention; trim sandbox_process.rs's module header to stable ownership, not PR-state. Add BackgroundJobRegistry, malformed-candidate-parsing, and concurrent SandboxActivityRegistry coverage. docs/reborn/contracts/host-runtime.md: one forward-pointing sentence noting RebornSandboxUserKey's future coarser identity model doesn't yet supersede the scope-derived identity this contract already documents. architecture ratchet: baseline the two new test/dead-code seams this introduces (attribution.rs dead-code method x4, test-support method x1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): O(1) drop_dead pruning + coverage for empty-alive-pids and concurrent attribution resolve Addresses review 4784267719 on PR nearai#6695: - BackgroundJobRegistry::drop_dead now builds a HashSet once instead of a per-job linear scan of alive_pids (O(J) not O(J x A)). - Add background_job_registry_drop_dead_with_empty_alive_pids_removes_all_jobs. - Add concurrent_resolve_calls_complete_with_consistent_attribution covering simultaneous ConnectionAttributionResolver::resolve calls. - Doc-comment the two known-but-deferred tradeoffs (attribution thundering herd, unbounded BackgroundJobRegistry growth) — both need the not-yet-wired caller (W6 / exec_transport+reaper) to know the right shape, so building a mechanism now would be guessing; left as explicit follow-ups instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): force real cache-miss overlap in attribution concurrency test FakeLookup::containers_on_network returned immediately with no yield point, so the 20 spawned resolve() tasks could run to sequential completion without ever actually overlapping in the miss/query/insert window the test claims to exercise. Add an optional Barrier that all callers wait on inside containers_on_network, forcing genuine concurrent cache misses before any insert proceeds. Addresses CodeRabbit review 4788508880. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address new PR nearai#6695 review round (CI-hang test, silent-ok docs, IPv6 attribution, typed mount resolution, key codec dedup - attribution.rs: bound the concurrent-resolve barrier test with a timeout so a regression can't hang CI; add silent-ok rationale comments on the Docker-boundary .ok() parses; fix container_addresses_on_network to also read bollard's global_ipv6_address field (an IPv6-only peer was previously never matchable); add empty-listing and IPv6 coverage. - registry.rs: module header now names all three responsibilities (label codec, activity registry, background-job registry); add a concurrent record/jobs_for/drop_dead test for BackgroundJobRegistry to match the existing SandboxActivityRegistry coverage. - user_key.rs: clarify that RebornSandboxScopeKey remains authoritative for the currently-wired transport; RebornSandboxUserKey is reserved for the future persistent per-user transport. - docker_gate.rs: header now describes the daemon-only gate accurately instead of overclaiming a required ironclaw-worker image. - local.rs: resolve_joined now returns a typed ResolvedMountPath (joined, containment_root, bootstrap_root) instead of a positional tuple plus a separately re-derived bootstrap_root at two of three call sites. - New key_codec module: shared length-prefixed encoding + SHA-256 digest for RebornSandboxScopeKey and RebornSandboxUserKey, replacing two independently-maintained copies of the same framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(sandbox): split attribution.rs test module into its own file My prior fixes pushed attribution.rs from 951 to 1029 lines, crossing the hard 1000-line threshold. Moved #[cfg(test)] mod tests (FakeLookup harness, 17 unit tests, the gated real-Docker integration test) into a sibling attribution_tests.rs via #[path], matching the file's existing docker_gate #[path] pattern. Pure move, no behavior change: production attribution.rs is now 367 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sandbox): address round-6 review findings on nearai#6695 - crate::-qualify key_codec imports in user_key.rs/scope_key.rs (matches the PR's own crate:: convention already used for registry/attribution) - add missing silent-ok rationale for the dropped created_at parse error in UserContainerCandidate::from_summary - gate the two cross-leaf symlink-escape tests in local.rs with #[cfg(unix)] (std::os::unix::fs::symlink does not compile on the windows release target) - document mount_local_per_leaf's bare-root-denial, first-use-bootstrap, and per-leaf symlink-containment contract in filesystem.md - add missing-tenant and malformed-tenant-label fail-closed tests to attribution_tests.rs (existing coverage only exercised the user field) - correct the ConnectionAttributionResolver doc comments: invalidate() collapses staleness "toward" zero, not "to" zero — a concurrent in-flight resolve() can still re-insert a stale entry after invalidate() removes it. Not fixed (no caller exists yet to fix a race against), but the doc must not overclaim a guarantee it does not have. * fix(filesystem): reject dangling final symlink in resolve_for_write Addresses coderabbitai review 4790451629 on PR nearai#6695: try_exists follows symlinks and reports false for a dangling one, so a pre-planted dangling symlink at the write target fell through to the brand-new-file bootstrap path. write_file/append_file open with O_CREAT, so the OS would create the file wherever the symlink points, escaping leaf containment. resolve_for_write now checks existence via symlink_metadata (lstat) and fails closed with SymlinkEscape when a symlink entry can't be canonicalized, instead of silently falling through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(filesystem): document TOCTOU residual on create_dir_all/delete coderabbitai (round 7) flagged that leaf containment checks aren't atomic with the mutation they gate. The gap is real but matches the already-deferred residual documented on resolve_for_write's re-rooting step (full fix needs openat/O_NOFOLLOW/cap-std, tracked via PR nearai#2996 review) — extend that documentation to resolve_for_create_dir_all and delete instead of re-litigating the same heavy-lift follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Two related, unwired-by-design slices toward the persistent per-user sandbox
container program:
ironclaw_filesystem: leaf-scoped mount containment.resolve_joinednow returns a per-request
containment_rootthat, for aleaf_scopedmount, is
host_root/<first-tail-segment>instead of the sharedhost_root— closing a same-mount cross-leaf symlink escape that a plainmount_localcontainment check (host_root only) would not catch. Abare-mount-root request against such a mount is rejected outright (there is
no safe containment root for "every caller's leaf").
mount_local_per_leafis the constructor.
ironclaw_host_runtime: per-user sandbox identity + attributionprimitives.
RebornSandboxUserKey(a{tenant, user}-only container/workspace key — every thread/project/agent for the same user shares one
container), the labels-as-identity
registrymodule (Docker labelhelpers,
SandboxActivityRegistry,BackgroundJobRegistry), andConnectionAttributionResolver(source-IP →{tenant, user}resolutionfor the shared sandbox egress proxy, design decision D9 — fail-closed on
any ambiguity: duplicate IP, missing/malformed labels, or a query error
all collapse to
Unattributed, never a guess).Scope limits — read before reviewing
leaf_scopedcontainment tests(
leaf_scoped_mount_rejects_bare_mount_root_request,leaf_scoped_mount_rejects_cross_leaf_symlink_escape) prove the boundaryat unit tier only.
sandbox_cross_tenant_escape.rs— the composition-tiertest that drives the actual end-to-end cross-tenant attack through a real
Docker container — is NOT included in this PR. Its imports
(
RebornSandboxConfig,RebornScopedSandboxCommandTransport,CommandExecutionRequest) resolve to the full exec-based sandboxtransport (
exec_transport.rs,connect.rs,egress_proxy.rs,reaper.rs— roughly 4500 new lines), which is not onmainand is faroutside a reviewable PR size. The end-to-end cross-tenant read/write
escape is therefore not proven by this PR — it ships with the transport
slice later, driven through the same harness.
mount_local_per_leaf,RebornSandboxUserKey,SandboxActivityRegistry, and the registry helpershave no production caller on
maintoday — their consumers are theexec-based transport's per-user container reuse and Task A5's reaper
(
exec_transport, not in this PR).ConnectionAttributionResolverisconsumed by W6 (egress-proxy TLS termination + credential injection), also
not built yet. This is deliberate: the sandbox program lands skeleton
pieces unwired and profile-gates them later, to avoid stacked-PR drift.
Every such item is
pub/re-exported (no dead-code lint fires) or carriesa targeted
#[allow(dead_code)]naming its real future consumer — no#[allow]was added without a named consumer, and no fake caller wasinvented to silence a lint.
mount_localbehavior is unchanged:containment_rootis only mutatedif mount.leaf_scoped && index == 0,so it stays equal to
host_rooton every existing path. Pre-existingTOCTOU between canonicalize and use is unchanged and out of scope.
Fix applied after review
attribution's real-Docker test originally gated ondocker_gate:: docker_available()(which shells out to thedockerCLI — context-aware:Colima, Docker Desktop, a remote host) but then connected directly via
Docker::connect_with_local_defaults(), which only honorsDOCKER_HOSTorthe hardcoded
/var/run/docker.sock. That let the gate pass while theconnection still failed on any non-default-socket machine (reproduced
locally on a Colima-backed dev machine). Fixed by reusing
sandbox_process::connect_docker()— the sameconnect_with_local_defaults()→
unix_socket_candidates()(~/.colima/default/docker.sock,~/.rd/docker.sock, etc.) fallback production containers already connectthrough — instead of reimplementing resolution. A
connect_docker()failurenow prints a
SKIP:line rather than panicking, since even that broaderfallback can't cover every possible Docker context.
Verified with
DOCKER_HOSTunset: the test now genuinely passes (not askip) via the Colima socket fallback.
Test plan
cargo fmt --all --check— cleancargo clippy -p ironclaw_filesystem --tests— 0 warningscargo clippy -p ironclaw_host_runtime --tests— 0 warningsIRONCLAW_DISABLE_OS_KEYCHAIN=1 cargo test -p ironclaw_filesystem— all pass, including the two new leaf-containment testsIRONCLAW_DISABLE_OS_KEYCHAIN=1 cargo test -p ironclaw_host_runtime --lib— 412 passed; 5 pre-existing failures unrelated to this diff (3 insandbox_process.rs's existing test module that hardcode/var/run/docker.sock, unaffected by any file this PR touches; 2 infirst_party_tools::trace_commons, unrelated). None of these 5 are new or introduced by this PR.🤖 Generated with Claude Code