Repository navigation
[experimental] feat(runtime): add ACP harness executor - #7648
serrrfirat wants to merge 7 commits into
Conversation
github-actions
Bot
added
scope: sandbox
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
risk: medium
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 6m 2s IronLoop completed the review and posted it to GitHub. 🔗 Result |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
Found two actionable issues in the ACP harness security boundary and its CI coverage routing.
Findings: 🔴 High 1 · 🟠 Medium 1
🔴 High · Prevent workspace-controlled session-file link traversal
Inline on crates/loop/ironclaw_turn_runner/src/harness_turn_run_executor.rs:364. See the inline comment for details.
🟠 Medium · Route ACP wiring changes through the Docker integration lane
Inline on scripts/ci/reborn_pr_test_plan.py:187. See the inline comment for details.
Validation
- ✅ Formatting — Workspace formatting check completed successfully.
Review details
- Run:
fe36ed0f-debb-4c0d-9c9a-57e522b62302 - Workflow: Review
- Attempts: 1
| match tokio::fs::read_to_string(path).await { | ||
| Ok(raw) => { | ||
| let value = raw.trim(); | ||
| if value.is_empty() || value.len() > 1024 { | ||
| return Err(agent_client_protocol::Error::internal_error() | ||
| .data("persisted ACP session id is invalid")); | ||
| } | ||
| Ok(Some(SessionId::new(value.to_string()))) | ||
| } | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), | ||
| Err(error) => Err(agent_client_protocol::Error::internal_error().data(format!( | ||
| "persisted ACP session id could not be read: {error}" | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| async fn persist_session_id( | ||
| path: &Path, | ||
| session_id: &SessionId, | ||
| ) -> agent_client_protocol::Result<()> { | ||
| let temporary = path.with_extension("tmp"); | ||
| tokio::fs::write(&temporary, session_id.to_string()) |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🔴 High · Prevent workspace-controlled session-file link traversal
The ACP process has write access to this persistent workspace, so it can create `.ironclaw-acp-session.tmp` as a symlink before responding to `session/new`. The host then follows that link in `write`, allowing a Docker-contained agent to overwrite any file writable by the service account. On a later turn it can likewise symlink the session file itself and have `read_to_string` send arbitrary host-file contents back to the agent as a session ID. Keep ACP-owned state outside the agent-writable mount, or use no-follow, directory-confined file operations for both reads and writes.
| "crates/kernel/ironclaw_runtime_policy/src/resolver.rs", | ||
| "crates/lanes/ironclaw_sandbox/tests/support/docker_gate.rs", | ||
| "crates/lanes/ironclaw_sandbox/tests/user_sandbox_docker_live.rs", | ||
| "crates/loop/ironclaw_turn_runner/src/agent_placement.rs", |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Route ACP wiring changes through the Docker integration lane
The new selector marks the placement and executor files as Docker-sensitive, but omits the composition and runner wiring files changed by this PR. A future edit to `crates/app/ironclaw_composition/src/runtime.rs`, `runtime_input.rs`, or `crates/loop/ironclaw_turn_runner/src/runtime.rs` selects only crate buckets and skips the ACP Docker integration test; the planner currently returns `run_sandbox_docker: false` for each. Add these wiring seams to this set and extend the selector regression test, otherwise a disconnected harness setting can merge without exercising host/Docker parity.
This branch has not been deployed
1 participant
Summary
Arc<dyn TurnRunExecutor>, with the canonical Rust executor as its default and replaceable executor registrations.AgentPlacement; host and Docker placements expose the same bounded stdio and teardown contract.Change Type
Linked Issue
Closes #7624
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— targeted clippy was started but intentionally stopped; CI will run the complete lint gate.cargo build— covered through compiled test targets rather than a separate build command.cargo test -p <owning-crate> --features integration— not applicable; this path uses the root Reborn integration harness and Docker placement test rather than a database feature gate.@agentclientprotocol/claude-agent-acp0.67.0 two-turn session, including cross-containersession/load, plus WebUI launch.review-prorpr-shepherd --fixwas run before requesting review — deferred while this remains an experimental draft.Test Strategy
User behavior: Given an explicitly routed profile, a turn runs through an ACP agent under host or Docker placement, streams cumulative text updates through the normal live projection, produces the normal finalized thread reply, and resumes the ACP session on the next turn. Unrouted profiles remain on the canonical loop.
Risk areas:
Tests added or updated:
What the tests prove: Explicit routing is opt-in; both placements share executor behavior; sessions survive process/container replacement; failures terminate without requeue; live chunks use the existing ephemeral projection and final replies use the existing durable thread path; configuration and credential/environment fences fail closed.
Commands run:
cargo fmt --all -- --checkcargo test -p ironclaw_turn_runner(254 passed; one unrelated pre-existing trace-capture queue-directory assertion failed and also failed alone)cargo test -p ironclaw_turn_runner harness_turn_run_executorcargo test -p ironclaw_turn_runner agent_placementcargo test -p ironclaw_configcargo test -p ironclaw_integration_tests --test reborn_integration_acp_harnesscargo test -p ironclaw_architecture_testspython3.11 scripts/ci/test_reborn_pr_test_plan.pypython3.11 scripts/ci/test_docs_publication_boundary.pypython3.11 scripts/ci/docs_publication_boundary.pygit diff --checkSecurity Impact
This adds process execution, filesystem access, open network egress, and environment credential injection behind explicit configuration. Ambient environment inheritance is cleared,
HOME/PATHoverrides are rejected, protocol/update sizes are bounded, workspaces are keyed from typed thread IDs, Docker containers are force-removed on terminal paths, and no tenant/customer secret store is reachable from the harness configuration. ACP permission requests are auto-approved by explicit experimental policy and logged at debug level.Reborn Trust-Boundary Checklist
rg -n "TurnRunExecutor|execute_claimed_run|LoopExit" crates/loop/ironclaw_turn_runner.serde(default)fields fail closed or have migration tests: optional harness config leaves behavior unchanged; supplied config is strictly validated.AgentPlacement; Docker/Bollard details remain inironclaw_sandbox.Database Impact
None. ACP session identity is stored in the per-thread workspace; no schema or migration changes.
Blast Radius
Opt-in runner composition, configuration parsing, Docker sandbox process transport, thread finalization, dependency graph, and CI selection. With
[harness]absent, all profiles retain the existing executor. The sandbox wrapper is intentionally a narrowHarnessContainerTemplate, not a generalized process-lane API.Rollback Plan
Remove or disable the
[harness]section to immediately restore canonical execution without data migration. Reverting this commit removes the executor, placement adapters, image, and tests; existing thread history remains valid. Per-thread ACP workspace files can be left inert or removed separately.Review Follow-Through
This is intentionally an experimental draft for evaluating loop quality. Reviewer judgment is requested on the placement seam, credential fence, minimal event contract, streamed-update behavior, and whether findings justify a subsequent production-hardening rung. ACP text chunks now feed the existing live text projection as bounded, sanitized cumulative updates; only the finalized reply is persisted.
Review track: C (runtime/security/CI)
Live Paired Evaluation
On 2026-08-14, six isolated tasks (11 turns per lane) were run through the ACP harness and canonical Rust loop with a developer Anthropic key. Both lanes used
claude-sonnet-4-6, the same committed seed regressions, and verified task-local workspaces. No local tests or external eval PRs were run or created.#[async_trait]on the generated test double. Rust also received partial credit after fixing the code during the diagnose-only turn and timing out before its second reply.Full task definitions, per-task numbers, usage, semantic findings, and isolation caveats are in
docs/internal/reborn/2026-08-harness-v0-findings.md.