Repository navigation
docs(plans): Reborn persistent tenant sandbox & agent-built extension promotion design - #4785
Conversation
…esign Crate-level design for wiring the TenantSandbox process backend into the Reborn binary, making the sandbox environment persistent per scope, brokering package-registry egress, and gating agent-built artifacts into the extension registry without privilege escalation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
- Broker is CONNECT-only: delete forward.rs and all broker-side credential machinery; DomainPattern moves to ironclaw_network as canonical home - Drop the environment_mode wire enum: SandboxEnvironmentProvider from day one, 2a->2b is an internal provider swap with no config surface - Timeout enforcement = host-side container restart (disposable compute), never in-container process-group management - Disk ceiling metered host-side on the volume mountpoint, not via in-container du with agent-controlled PATH - No new runtime-policy axis for promotion: composition-gated with a mandatory validate-style guard; manifest shapes bind to ExtensionManifestV2/CapabilityDeclV2 - Decompose sandbox_process.rs (945 lines) as Phase 2a step 0 - Clean up half-abandoned scope-token deliberation in 3.1 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Applied fixes from a strict design review (second commit). The common thread: four places where the doc stopped applying its own principles, all fixed by shrinking the design:
Plus: no new runtime-policy axis for promotion (composition-gated with a mandatory validate-style startup guard; the 🤖 Generated with Claude Code |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a detailed design document for the Reborn Persistent Tenant Sandbox and Agent-Built Extension Promotion, outlining a four-phase plan to implement persistent environments, an egress broker, and a secure WASM extension promotion pipeline. The review feedback highlights several critical design improvements to align with system guidelines: updating the egress broker's URL validation to support self-hosted setups by allowing private and loopback IPs; using length-prefixed components and collision-resistant digests for deterministic identifier derivation to prevent naming collisions; adding explicit ownership checks at the promotion gate; ensuring primary execution errors are preserved and secondary cleanup failures are logged as warnings during exec timeouts; and utilizing exhaustive match statements without wildcards for mapping network modes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| scope to refusing once over the ceiling; (5) timeout restarts the environment | ||
| and the next command still finds the home volume intact. | ||
|
|
||
| **Exit criteria:** a hosted agent can `npm i -g @some/cli` in one turn and use the CLI |
There was a problem hiding this comment.
According to the general rules, when implementing URL validation for service providers, we should always block cloud-metadata IPs (e.g., 169.254.169.254), link-local, and multicast addresses, but ensure that private and loopback IP ranges remain accepted if they are legitimate for self-hosted configurations.
The current design states that the broker will block RFC1918 (private IP ranges) completely. This would break legitimate self-hosted configurations. Please update the design to ensure that private and loopback IP ranges are accepted when legitimate for self-hosted setups, while still strictly blocking cloud-metadata, link-local, and multicast addresses.
References
- When implementing URL validation for service providers, always block cloud-metadata IPs (e.g., 169.254.169.254), link-local, and multicast addresses. However, ensure that private and loopback IP ranges remain accepted if they are legitimate for self-hosted configurations.
|
|
||
| ```rust | ||
| /// Lifecycle provider beneath the command transport. Backend implementors |
There was a problem hiding this comment.
According to the general rules, deterministic identifiers derived from multiple components should use length-prefixed components combined with a domain-separated collision-resistant digest (such as BLAKE3), rather than raw string concatenation or simple sanitization, to prevent accidental or malicious collisions.
The current design proposes deriving home_volume_name and container names using a simple identity_slug() string format. Since these identifiers are used to enforce security boundaries (such as volume isolation), please update the design to specify that identity_slug() (or the underlying scope key derivation) will use length-prefixed components combined with a domain-separated collision-resistant digest to prevent any potential naming collisions.
References
- Derive deterministic identifiers from multiple components using length-prefixed components combined with a domain-separated collision-resistant digest, rather than raw string concatenation, to prevent accidental or malicious collisions.
|
|
||
| ### 3.4 Tests | ||
|
|
There was a problem hiding this comment.
According to the general rules, when implementing services that interact with user-owned resources, always perform an ownership check (e.g., verifying the authenticated user ID matches the resource owner) before any read or write operations. Ensure the underlying resource coordinator is not invoked if the ownership check fails.
The promotion gate design currently only specifies workspace path resolution checks. Please update the design to explicitly require an ownership check (verifying the authenticated user ID matches the workspace/project owner) before reading the artifact or invoking the promotion coordinator.
References
- When implementing services that interact with user-owned resources, always perform an ownership check (e.g., verifying the authenticated user ID matches the resource owner) before any read or write operations. Ensure the underlying resource coordinator is not invoked if the ownership check fails.
| **Base image.** Extend the worker image (`crates/Dockerfile.sandbox` lineage) with: | ||
| node LTS + npm configured for `~/.npm-global`, python3 + uv, rust toolchain, git, | ||
| ripgrep, build-essential, `agent` user (uid 1000) with writable-volume `$HOME`. | ||
| Default `container_user` becomes `1000:1000` with |
There was a problem hiding this comment.
According to the general rules, when a primary operation fails and a subsequent cleanup or secondary operation also fails, log the secondary failure as a warning and ensure the original primary error is returned to the caller to prevent it from being swallowed.
In the exec timeout/kill design, if the primary command execution fails or times out, and the subsequent process group cleanup (pkill -g) also fails, the design should specify that the cleanup failure is logged as a warning while the original primary execution/timeout error is returned to the caller.
References
- When a primary operation fails and a subsequent cleanup or secondary operation also fails, log the secondary failure as a warning and ensure the original primary error is returned to the caller to prevent it from being swallowed.
| **The broker is CONNECT-only.** It tunnels TLS to allowlisted hosts and does nothing | ||
| else: no plain-HTTP forwarding, no credential resolution, no dependency on the |
There was a problem hiding this comment.
According to the general rules, in functions that map one enum to another, we should use an exhaustive match statement instead of a wildcard _ arm to force a compile-time error when new variants are added.
Please specify in the design that the mapping from NetworkMode to base domain sets in policy.rs will use an exhaustive match statement without a wildcard arm to ensure compile-time safety when new network modes are introduced.
References
- In functions that map one enum to another, use an exhaustive match statement instead of a wildcard _ arm. This forces a compile-time error when new variants are added, ensuring all cases are explicitly handled.
…ection lane Council-accepted (Opus 4.8 + GPT-5.5 XHigh) opt-in lane on the Phase 3 egress broker: per-scope ephemeral CA + combined trust bundle, in-container proxy shim bridging http(s)_proxy to the mounted unix socket, credentials staged at CapabilityHost prepare time and injected host-side on the outer leg (I2 preserved), session-bound lifecycle for keep-alive, streaming origin leg for heavy media, exact-host inject, public CheckedNetworkTargetResolver for SSRF-safe pinned-IP dialing on both legs, and an E2E test plan (fast in-process + Docker-gated). Lets cookie-gated tools (Agent-Reach social CLIs) run in the persistent sandbox without the secret entering the container. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a design plan for persistent TenantSandbox behavior, scoped egress brokering, optional TLS credential injection, and approval-gated promotion of sandbox-built WASM artifacts. It also records backend-independent security invariants, portability requirements, and open implementation items. ChangesReborn Persistent Sandbox & Extension Promotion Plan
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 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 |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/plans/2026-06-12-reborn-persistent-sandbox.md`:
- Around line 82-101: The introductory text claims "Everything below holds for
Docker today" but invariants I5 and I6 are Phase 2 additions, not pre-existing.
Clarify the threat model by reframing the table opening to explicitly
distinguish between pre-existing invariants (I1, I2, I3, I4 that the current
ephemeral sandbox already enforces) and phase-gated additions (I5 for Phase 2
scope isolation via volume naming, I6 for Phase 2 disk and pids ceilings). This
distinction ensures readers understand which security guarantees apply during
Phase 1 rollout versus later phases.
- Line 8: Update the markdown document to correct the factual inaccuracy and
line-number references. First, correct the assertion at line 24 that claims
there are "zero references to `TenantSandboxProcessPort` outside
`ironclaw_host_runtime`" by acknowledging the actual references found in the
`ironclaw_reborn_composition` crate across multiple files including error.rs,
lib.rs, factory/local_dev_host_tests/approval_gates.rs,
production_runtime_policy.rs, runtime.rs (at lines 3805 and 3876), and input.rs.
Second, update all line-number citations in the context section to match current
actual line numbers: change `runtime_policy.rs:403` to line 412 for the
ProcessBackendKind enum, change `sandbox_process.rs:220` to line 221 for
RebornScopedSandboxCommandTransport, and change `invocation_services.rs:209-225`
to reflect the trait definition at line 104 and implementation at line 191.
These corrections ensure the design document provides accurate guidance to
implementors and reviewers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a47da95-6b8b-4333-8b90-ff368fb2bd8f
📒 Files selected for processing (1)
docs/plans/2026-06-12-reborn-persistent-sandbox.md
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 (2)
docs/plans/2026-06-12-reborn-persistent-sandbox.md (2)
102-105: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse neutral path placeholders in committed docs.
This file still embeds
/home/agentand/tmp, which the repo guideline forbids in committed.mdfiles outside tests/scripts. Replace them with symbolic placeholders or env-backed names so the plan stays portable. As per coding guidelines,**/*.{md,py}: Committed.mdand.pyfiles must not contain developer-local absolute paths such as/home/<user>/,/Users/<user>/, or/tmp/(the tests/scripts exception does not apply here).Also applies to: 359-391
🤖 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 `@docs/plans/2026-06-12-reborn-persistent-sandbox.md` around lines 102 - 105, The plan doc contains developer-local paths in committed markdown, which should be replaced with neutral placeholders. Update the persistence examples in the plan to remove `/home/agent` and `/tmp` and use symbolic or env-backed names instead, keeping the guidance portable. Review the affected sections in this document and any similar references so the wording remains consistent without hardcoded local paths.Source: Coding guidelines
95-95: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the Docker-networking carve-out.
- I1 says there is no ambient network, and the later broker section keeps
network_mode: nonewith a Unix-socket broker. This exception conflicts with that contract unless this section states a concrete requirement.- Strip the
/home/...and/tmp/...paths too;.claude/rules/doc-hygiene.mdforbids developer-local absolute paths in committed.mdfiles.🤖 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 `@docs/plans/2026-06-12-reborn-persistent-sandbox.md` at line 95, The I1 environment contract still mentions a Docker-networking carve-out, which conflicts with the later `RebornSandboxConfig::container_network_mode()` and Phase 3 broker design that keep networking disabled. Remove that exception unless you can state a concrete, required condition; otherwise keep the contract aligned with `network_mode: none`. Also strip any developer-local absolute paths from this section, since the committed docs must not include `/home/...` or `/tmp/...` references.
🤖 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 `@docs/plans/2026-06-12-reborn-persistent-sandbox.md`:
- Around line 102-105: The plan doc contains developer-local paths in committed
markdown, which should be replaced with neutral placeholders. Update the
persistence examples in the plan to remove `/home/agent` and `/tmp` and use
symbolic or env-backed names instead, keeping the guidance portable. Review the
affected sections in this document and any similar references so the wording
remains consistent without hardcoded local paths.
- Line 95: The I1 environment contract still mentions a Docker-networking
carve-out, which conflicts with the later
`RebornSandboxConfig::container_network_mode()` and Phase 3 broker design that
keep networking disabled. Remove that exception unless you can state a concrete,
required condition; otherwise keep the contract aligned with `network_mode:
none`. Also strip any developer-local absolute paths from this section, since
the committed docs must not include `/home/...` or `/tmp/...` references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4a412c7-9af6-49eb-a080-57425a5a2310
📒 Files selected for processing (1)
docs/plans/2026-06-12-reborn-persistent-sandbox.md
Reborn integration-tier coverageLine coverage (Reborn crates): 15.27% — 9736 / 63747 lines Per-crate breakdown (11 crates, lowest-covered first)
This signal is informational: coverage never gates the PR — not the percentage, not the per-crate holes, not the 0-coverage callout. |
1 participant
What problem this solves
The Reborn binary's process-execution story is designed but not usable in a hosted deployment, and has no answer for persistent agent environments or agent-built software. Concretely:
The sandbox backend is not wired.
ProcessBackendKind::TenantSandboxis the only implemented backend (RebornScopedSandboxCommandTransport, Docker), butRebornRuntimeProcessBindingdefaults toNoneand nothing inironclaw_reborn_cliever constructs the transport. Every hosted profile (HostedSafe/HostedDev/HostedYoloTenantScoped) resolves toTenantSandboxin the policy matrix — so a hosted deployment today either fails composition validation or has no process effects at all.The sandbox is per-command ephemeral. Every
run_commanddoes container create → start → wait → remove. Nothing the agent installs survives the command that installed it:npm install -g fooin one tool call,foogone in the next. That makes the sandbox unusable for the actual goal — agents that run, build, and test software over time, with CLIs (node, uv, cargo tools, …) installed once and kept.The network broker is half-built. The transport already sets
http_proxyenv vars and bind-mounts broker unix sockets into the container, with tests — but no host-side broker server exists to serve those sockets. Without it, a persistent environment is network-dark and can't install packages; the naive fix (open network) would break the security model.There is no path from "agent built it" to "IronClaw runs it." The eventual goal is agents promoting software they built into IronClaw extensions. Done naively, that's a privilege-escalation pipeline: a prompt-injected or supply-chain-compromised build gets host execution because "we built it."
The design
docs/plans/2026-06-12-reborn-persistent-sandbox.md— a crate-level, implementation-ready plan built around one core principle: the security boundary is the brokers and the promotion gate, not container ephemerality. A persistent environment where the agent installs arbitrary packages is assumed compromised; the system stays secure anyway because six backend-independent invariants (no ambient network, no secrets inside, minimal typed host↔sandbox surface, no implicit trust for sandbox-origin artifacts, scope-isolated state, per-scope resource ceilings) are enforced outside the sandbox technology. That is what makes the design hold for Docker today and SmolVM/SRT/dedicated runners later.Four phases, each independently shippable:
RebornSandboxSettingsinironclaw_reborn_config, abuild_runtime_process_binding()factory inironclaw_reborn_composition, called fromserve.rs. Fail-loud at startup if policy demands a sandbox and Docker is unreachable./home/agent; container hardening (read-only rootfs,cap_drop ALL, no-new-privileges) is kept — installs are user-space. Then aSandboxEnvironmentProvidertrait (acquire/exec/reap_idle/remove) with warm containers +docker exec, which is also the seam future backends implement. Adds pids and disk ceilings, which persistence makes mandatory.ironclaw_network, typedNetworkMode → domain setprofiles (npm/crates.io/PyPI/GitHub underAllowlist). Credentialed API calls stay on the host egress pipeline — the staged-obligation secret injection contract is untouched.extension_promotefirst-party tool: WASM-only (native binaries stay sandbox-local), trust pinned at user level with no parameter to request more, capabilities recorded as requested but granted empty, every grant human-approved through the existing approval machinery, hash-pinned with grants reset on re-promotion. Includes extracting the WASM validator intoironclaw_wasm_validateso reborn crates don't depend on the v1 tree.The doc closes with a threat-model table, per-phase tests and exit criteria, the phase dependency graph, and tracked open items (the honest one: named-volume disk quotas are soft
du-based until xfs project quotas land).Decision worth reviewer attention
apt-getinside the container is unsupported by design — missing system packages go into the base image; agent installs are user-space only. This is what lets read-only-rootfs hardening survive persistence. If root-level installs are wanted, the rootfs decision must be flipped deliberately (the plan's structure survives, the hardening row of the threat model weakens).Scope
Docs only — one new file under
docs/plans/, no code changes.🤖 Generated with Claude Code