Skip to content

refactor(process_sandbox): retire dead DockerProcessSandboxBackend - #6693

Merged
henrypark133 merged 5 commits into
mainfrom
sandbox/pr2-retire-docker-backend
Jul 27, 2026
Merged

henrypark133 merged 5 commits into
mainfrom
sandbox/pr2-retire-docker-backend

Conversation

@henrypark133

Copy link
Copy Markdown
Collaborator

What & why

DockerProcessSandboxBackend has no production constructor — it is only instantiated in its own crate's tests. It is superseded by the persistent per-user sandbox and is already non-functional under the current posture: it needs root + NET_ADMIN, while sandbox containers now run as uid 1000 with cap_drop: ALL.

Behavior is unchanged

process_executor.rs, production.rs, and services.rs are untouched by this commit. PROCESS_SANDBOX_CAPABILITY_ID dispatch is identical before and after: the executor was already None in production, so system.process_sandbox.run returns missing_process_sandbox_executor exactly as it did before this change.

What was kept and why

PROCESS_SANDBOX_CAPABILITY_ID, SandboxProcessPlan, ValidatedSandboxProcessPlan, plan.rs, and validation.rs — the plan/validation types production still uses — plus tests/docker_security.rs.

What was deleted

docker.rs (604 lines), backend.rs (159), broker.rs (146), approval.rs (120); trims to lib.rs/tests.rs (-577); drops the now-orphaned dependencies async-trait, ironclaw_processes, secrecy, serde_json, tokio, tempfile.

Review status

A code-review pass returned zero findings, verifying: zero live references to any deleted symbol anywhere in the workspace, identical capability dispatch before/after, and that each dropped dependency is genuinely unused by the surviving code. Re-verified independently in this worktree with a dangling-symbol grep (DockerProcessSandboxBackend, ProcessSandboxBroker, process_sandbox::{backend,docker,broker,approval}) — zero matches.

Series context

Part of an incremental series landing sandbox work into main in independent, reviewable slices (the first was #6689). Deliberately not stacked.

Refs #6686

🤖 Generated with Claude Code

DockerProcessSandboxBackend, its ProcessSandboxBackend trait, and the
broker/approval helpers around it (docker.rs, broker.rs, approval.rs,
backend.rs) had zero consumers outside this crate and were never
constructed in production: `HostProcessExecutor::process_sandbox_executor`
defaults to `None` (services.rs), and `with_process_sandbox_executor` is
only ever called from test fixtures. Dispatching
`system.process_sandbox.run` already returned `missing_process_sandbox_executor`
before this change; it still does after, since production wiring in
production.rs/process_executor.rs/services.rs is untouched. The dynamic
process-compatibility lane is superseded by the persistent per-user
sandbox and was already non-functional under the non-root/zero-cap
posture.

Kept: `PROCESS_SANDBOX_CAPABILITY_ID` (still routed by ID), the
`SandboxProcessPlan`/`ValidatedSandboxProcessPlan` types and their
validation (plan.rs, validation.rs), and `DEFAULT_PROCESS_SANDBOX_IMAGE`
plus tests/docker_security.rs, which exercises the built sandbox image
directly and has no dependency on the removed backend. Dropped the
async-trait/ironclaw_processes/secrecy/serde_json/tokio/tempfile deps
that only the deleted code used.

Refs #6686

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 27, 2026 04:32
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@ironloopai

ironloopai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

🔎 IronLoop Review Status

Head: 08d0ccda2e5e3e0f3e59a0c73e3955bc431db877
Result: One or more review results were superseded by a newer PR head.
Next: Run @ironloopai review on the latest PR head.
Updated: 2026-07-27T05:13:02.567Z

Current reviewers:

Reviewer State Verdict Findings Last update
ironloop/common-reviewer (reviewer) Superseded N/A N/A 2026-07-27T05:03:29.147Z
Reviewer summaries
Reviewer Detail
ironloop/common-reviewer (reviewer) Superseded by a newer PR head. New head: 672eff0. Previous verdict: Needs validation.
Recent activity
Time Reviewer State Detail
2026-07-27T04:56:15.469Z ironloop/common-reviewer (reviewer) Superseded A newer PR head replaced this review (f3fbc8e).
2026-07-27T04:57:55.962Z ironloop/common-reviewer (reviewer) Queued Accepted review request for head f3fbc8e.
2026-07-27T04:57:55.962Z ironloop/common-reviewer (reviewer) Queued Waiting for this reviewer lane to become available.
2026-07-27T04:57:56.139Z ironloop/common-reviewer (reviewer) Started Reviewer worker started.
2026-07-27T04:57:58.735Z ironloop/common-reviewer (reviewer) Workspace ready Prepared isolated checkout (merge_ref) at 918eb5d.
2026-07-27T05:01:20.418Z ironloop/common-reviewer (reviewer) Result captured Needs validation; 0 blocking findings.
2026-07-27T05:01:20.418Z ironloop/common-reviewer (reviewer) Completed Review completed and terminal status was persisted.
2026-07-27T05:03:29.147Z ironloop/common-reviewer (reviewer) Superseded A newer PR head replaced this review (672eff0).
Available commands
  • @ironloopai help
  • @ironloopai agents
  • @ironloopai review
  • @ironloopai review --agent <agent>
Run metadata

Admission: webhook accepted the request and IronLoop persisted reviewer state before this projection.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6693 July 27, 2026 04:32 Destroyed
@github-actions github-actions Bot added scope: dependencies Dependency updates size: XL 500+ changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels Jul 27, 2026
@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Breaking Changes
    • Removed Docker-based process sandbox execution and related execution-result APIs.
    • Removed approval summary, credential/header brokering, and broker rewriting logic from the sandbox package.
    • The crate now focuses solely on typed sandbox process plan definitions and validation.
  • Documentation
    • Updated capability parity and crate guidance to reflect that no production execution backend is wired yet.
  • Tests & Maintenance
    • Reduced/trimmed sandbox test coverage by removing executor/Docker-related tests.
    • Cleaned up sandbox crate dependencies and adjusted internal output/timeout limit constants.

Walkthrough

The process sandbox crate was narrowed to typed plan definitions and validation. Executor, Docker backend, broker, approval-summary modules, related dependencies, public re-exports, and Docker/executor tests were removed. Repository guidance now records that no production execution backend is wired.

Changes

Process sandbox retirement

Layer / File(s) Summary
Reduce crate to plan validation
crates/ironclaw_process_sandbox/Cargo.toml, crates/ironclaw_process_sandbox/src/lib.rs, crates/ironclaw_process_sandbox/src/{approval,backend,broker,docker}.rs
Dependencies and module wiring now support plan validation only; execution, Docker, broker, approval APIs, and prior limit defaults were removed.
Retain plan validation tests
crates/ironclaw_process_sandbox/src/tests.rs
Test setup was narrowed to plan validation, while Docker and executor fixtures and tests were removed.
Align repository contracts
FEATURE_PARITY.md, crates/AGENTS.md, crates/ironclaw_process_sandbox/CLAUDE.md, crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs, docs/reborn/engine-v2-to-reborn-parity.md
Documentation, crate guardrails, and the architecture baseline now identify Docker sandbox execution as not covered and keep execution wiring outside this crate.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the change, but it misses the repository's required template sections and most checklist fields. Add the template's Summary, Change Type, Linked Issue, Validation, Test Strategy, Security Impact, Trust-Boundary, DB, Blast Radius, Rollback, and Review Track sections.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses Conventional Commits style and accurately summarizes the removal of the DockerProcessSandboxBackend refactor.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_process_sandbox/src/lib.rs`:
- Around line 1-8: Update the rust-ratchet expectations in
reborn_struct_test_support_ratchet.rs to remove the stale reference to the
deleted ironclaw_process_sandbox/src/docker.rs module, while preserving all
remaining crate-file-graph and contract entries.
🪄 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: 4e322223-df74-42f2-b118-19f97fbf36d2

📥 Commits

Reviewing files that changed from the base of the PR and between a091876 and f2a0168.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
📒 Files selected for processing (7)
  • crates/ironclaw_process_sandbox/Cargo.toml
  • crates/ironclaw_process_sandbox/src/approval.rs
  • crates/ironclaw_process_sandbox/src/backend.rs
  • crates/ironclaw_process_sandbox/src/broker.rs
  • crates/ironclaw_process_sandbox/src/docker.rs
  • crates/ironclaw_process_sandbox/src/lib.rs
  • crates/ironclaw_process_sandbox/src/tests.rs
💤 Files with no reviewable changes (5)
  • crates/ironclaw_process_sandbox/Cargo.toml
  • crates/ironclaw_process_sandbox/src/approval.rs
  • crates/ironclaw_process_sandbox/src/broker.rs
  • crates/ironclaw_process_sandbox/src/backend.rs
  • crates/ironclaw_process_sandbox/src/docker.rs

Comment thread crates/ironclaw_process_sandbox/src/lib.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2a0168387

ℹ️ 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".

Comment thread crates/ironclaw_process_sandbox/src/lib.rs
Comment thread crates/ironclaw_process_sandbox/src/lib.rs

@ironloopai ironloopai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ IronLoop Review: reviewer

Review at a glance

Verdict Blocking Notes Inline Head
❌ Changes requested 1 2 3 f2a01683878e

Head: f2a01683878e0701453b89aadb93ec47d353b4dc
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 deletion leaves a mandatory architecture test ratchet stale, so the architecture test suite will fail. The nested latency workspace lockfile and sandbox-parity documentation also still describe the removed backend.

Findings

Blocking: 1 / Notes: 2

Blocking findings

1. ❌ [MEDIUM] Remove the deleted Docker module from the architecture ratchet

Location: crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs:307-312
This frozen entry requires one test-support method in crates/ironclaw_process_sandbox/src/docker.rs, but this PR deletes that file. The ratchet scans the source tree and then asserts that no frozen count has decreased, so cargo test -p ironclaw_architecture will fail with this entry in its removed list. Delete or update the corresponding frozen entry.

Non-blocking notes (2)
1. 💬 [LOW] Regenerate the latency runner lockfile

Location: harness/latency/runner/Cargo.lock:3046-3058
The standalone latency-runner workspace reaches ironclaw_process_sandbox transitively, but its committed lockfile still records the removed async-trait, ironclaw_processes, secrecy, serde_json, and tokio dependencies. Regenerate this lockfile so locked builds do not require a lockfile update and normal latency runs do not dirty the checkout.

2. 💬 [LOW] Update Reborn sandbox capability documentation

Location: docs/reborn/engine-v2-to-reborn-parity.md:63
This row still marks the per-project Docker sandbox as covered and says backend-neutral sandbox backends and hardened Docker command construction exist. Those types and implementation are deleted by this PR, while production has no configured executor. Update this document and the matching claims in FEATURE_PARITY.md and the process-sandbox crate guidance to describe the remaining plan-validation-only surface.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when 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, 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_architecture/tests/reborn_struct_test_support_ratchet.rs:307

IronLoop reviewer: [MEDIUM] Remove the deleted Docker module from the architecture ratchet

This frozen entry requires one test-support method in crates/ironclaw_process_sandbox/src/docker.rs, but this PR deletes that file. The ratchet scans the source tree and then asserts that no frozen count has decreased, so cargo test -p ironclaw_architecture will fail with this entry in its removed list. Delete or update the corresponding frozen entry.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when the fix may affect multiple areas.
#### Inline fallback 2: harness/latency/runner/Cargo.lock:3046

IronLoop reviewer: [LOW] Regenerate the latency runner lockfile

The standalone latency-runner workspace reaches ironclaw_process_sandbox transitively, but its committed lockfile still records the removed async-trait, ironclaw_processes, secrecy, serde_json, and tokio dependencies. Regenerate this lockfile so locked builds do not require a lockfile update and normal latency runs do not dirty the checkout.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when the fix may affect multiple areas.
#### Inline fallback 3: docs/reborn/engine-v2-to-reborn-parity.md:63

IronLoop reviewer: [LOW] Update Reborn sandbox capability documentation

This row still marks the per-project Docker sandbox as covered and says backend-neutral sandbox backends and hardened Docker command construction exist. Those types and implementation are deleted by this PR, while production has no configured executor. Update this document and the matching claims in FEATURE_PARITY.md and the process-sandbox crate guidance to describe the remaining plan-validation-only surface.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when the fix may affect multiple areas.

@railway-app

railway-app Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

🚅 Deployed to the ironclaw-pr-6693 environment in ironclaw-ci-preview

Service Status Web Updated (UTC)
ironclaw ✅ Success (View Logs) Web Jul 27, 2026 at 5:13 am

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (multi-agent)

Intent: Retire the unused Docker process sandbox backend and orphaned dependencies while preserving behavior and capability dispatch.

Stats: 2 findings (from 3 raw, 2 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1.

Build

  1. High Remove stale architecture ratchet entry (crates/ironclaw_process_sandbox/src/docker.rs:1-1, confidence 100) — anchor: crates/ironclaw_process_sandbox/src/docker.rs:1.
    Deleting docker.rs leaves the frozen test-support baseline in reborn_struct_test_support_ratchet.rs claiming one method exists there. The architecture test now fails on every run until that baseline entry is removed. Fix: Delete the stale crates/ironclaw_process_sandbox/src/docker.rs entry from FROZEN_PATH_COUNTS. (no diff position — body only; also flagged by approach/Medium)

Guidance

  1. Medium Update stale process-sandbox crate guidance (crates/ironclaw_process_sandbox/src/lib.rs:1-8, confidence 95) — anchor: crates/ironclaw_process_sandbox/CLAUDE.md:3-8.
    The diff changes this crate to plan validation only, but its crate-local guidance still says it owns Docker execution, brokered phases, mount roots, and ProcessExecutor integration. Fix: Update the crate guidance and crates map to describe plan validation only, or remove the crate-local guidance if it is no longer applicable.

Comment thread crates/ironclaw_process_sandbox/src/lib.rs
henrypark133 and others added 2 commits July 26, 2026 21:53
The frozen-path ratchet is shrink-only by design: it pins per-path
test-support counts so they can only go down, never back up. The
`docker.rs` entry tracked a method in the process-sandbox Docker
backend that PR #6693 deletes outright, so the file no longer exists
and the baseline entry is dead weight, not a real regression risk.

Removing the entry is the fix, and the ratchet test itself is the
regression test: `cargo test -p ironclaw_architecture` now passes
because there is no stale path left to check for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #6693 deleted the Docker process-sandbox backend, but four docs
still described it as present: the crate's own CLAUDE.md guardrails,
the per-crate row in crates/AGENTS.md, FEATURE_PARITY.md's Docker
sandbox row, and the Reborn parity doc's per-project-sandbox row (plus
its accompanying note). Rewrite all four to match the crate's own
lib.rs doc comment: ironclaw_process_sandbox owns only the typed
SandboxProcessPlan contract and validation, no production backend is
wired for the capability today, and the parity docs now honestly mark
the capability as not covered rather than silently dropping the row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 27, 2026 04:56
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6693 July 27, 2026 04:56 Destroyed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the scope: docs Documentation label Jul 27, 2026
@henrypark133

Copy link
Copy Markdown
Collaborator Author

Addressing the three review findings raised across coderabbitai/codex/ironloopai/henrypark133:

  1. Ratchet entry (coderabbitai #3654428166, codex #3654429490, ironloopai blocking Move whatsapp channel source to channels-src/ for consistency #1, code-review-skill Build finding) — fixed in 0e9a007: removed the stale docker.rs FrozenPathCount entry from reborn_struct_test_support_ratchet.rs. The ratchet is shrink-only by design; this baseline tracked a method in a now-deleted file, so removing it is correct. cargo test -p ironclaw_architecture is green.

  2. Stale docs (codex #3654429492, henrypark133 #3654459231, ironloopai note feat: adding Web UI #2, code-review-skill Guidance finding) — fixed in f3fbc8e: rewrote crates/ironclaw_process_sandbox/CLAUDE.md, the crates/AGENTS.md row, FEATURE_PARITY.md's Docker sandbox row, and docs/reborn/engine-v2-to-reborn-parity.md's per-project-sandbox row (+note) to describe plan-validation-only, matching lib.rs's own doc comment. Parity rows now honestly say not-covered rather than dropping them.

  3. Nested lockfile (ironloopai note Move whatsapp channel source to channels-src/ for consistency #1) — investigated: harness/latency/runner/Cargo.lock is a standalone workspace lockfile that turned out to be stale well beyond ironclaw_process_sandbox (regenerating it also churns ironclaw_dispatcher removal, ironclaw_operator addition, and dependency lists for ironclaw_extension_host/ironclaw_runner/etc. — pre-existing drift unrelated to this PR). Regenerating it here would produce a large unrelated diff, so leaving it as-is; the crate's own lockfile entry is stale but harmless (Cargo will re-resolve as needed) and cleaning up that nested workspace's lockfile is better done as its own follow-up.

Requesting re-review from @ironloopai.

@henrypark133

Copy link
Copy Markdown
Collaborator Author

@ironloopai review

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/reborn/engine-v2-to-reborn-parity.md`:
- Line 63: Reconcile the sandbox parity row with the retired backend by labeling
ironclaw_processes, ironclaw_wasm, and tests/integration/process_port.rs as
adjacent or non-backend support, and point the evidence to the retained
SandboxProcessPlan validation tests. Update the Known Gaps and conclusion
sections to list sandbox execution as uncovered and remove the claim that every
runtime concern maps to Reborn, ensuring all documentation guarantees match the
implemented code and tests.
🪄 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: 3f73ec9b-2a96-4a85-ab48-77bf8e513cb7

📥 Commits

Reviewing files that changed from the base of the PR and between f2a0168 and f3fbc8e.

📒 Files selected for processing (5)
  • FEATURE_PARITY.md
  • crates/AGENTS.md
  • crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs
  • crates/ironclaw_process_sandbox/CLAUDE.md
  • docs/reborn/engine-v2-to-reborn-parity.md
💤 Files with no reviewable changes (1)
  • crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs

Comment thread docs/reborn/engine-v2-to-reborn-parity.md Outdated

@ironloopai ironloopai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IronLoop Review: reviewer

Review at a glance

Verdict Blocking Notes Inline Head
⚠️ Needs validation 0 2 2 f3fbc8e2a87d

Head: f3fbc8e2a87d723ad4a39e6fc5ad8f11118e6543
Next: Human review or validation is required before merging.

Run details

Status: Current
Needs human: no
Needs validation: yes

Summary

Static review found no blocking runtime regression: production still constructs no process-sandbox executor, and deleted API usage was confined to the retired crate/tests. Two non-blocking maintenance issues remain.

Findings

Blocking: 0 / Notes: 2

Non-blocking notes (2)
1. 💬 [NOTE] Remove obsolete Docker-backend rustdoc

Location: crates/ironclaw_process_sandbox/src/plan.rs:87-89
These comments still describe a currently running Docker MVP, but this PR removes that backend and lib.rs now documents the crate as plan-validation-only. Update this and the corresponding network comment so the public plan documentation does not claim obsolete execution behavior.

2. 💬 [LOW] Refresh the latency runner's standalone lockfile

Location: harness/latency/runner/Cargo.lock:3047-3056
Flags: needs validation
This standalone workspace lockfile still records the removed ironclaw_process_sandbox dependencies. A runner build using --locked will require a lockfile refresh rather than reproducing the manifest graph.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when 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/plan.rs:87

IronLoop reviewer: [NOTE] Remove obsolete Docker-backend rustdoc

These comments still describe a currently running Docker MVP, but this PR removes that backend and lib.rs now documents the crate as plan-validation-only. Update this and the corresponding network comment so the public plan documentation does not claim obsolete execution behavior.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when the fix may affect multiple areas.
#### Inline fallback 2: harness/latency/runner/Cargo.lock:3047

IronLoop reviewer: [LOW] Refresh the latency runner's standalone lockfile

This standalone workspace lockfile still records the removed ironclaw_process_sandbox dependencies. A runner build using --locked will require a lockfile refresh rather than reproducing the manifest graph.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when the fix may affect multiple areas.

CodeRabbit flagged that the sandbox row said Not covered but the Known
Gaps list and Conclusion still claimed every runtime concern (incl.
sandbox) maps cleanly to Reborn, and cited ironclaw_processes/
ironclaw_wasm/process_port.rs as parity evidence for a backend that
doesn't exist. Label those as non-backend/adjacent, point evidence at
the crate's own plan-validation tests, and add sandbox to Known Gaps
and the Conclusion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 27, 2026 05:03
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6693 July 27, 2026 05:03 Destroyed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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/reborn/engine-v2-to-reborn-parity.md`:
- Line 63: Update the parity document’s status legend and summary to include the
new “Not covered” status and its meaning, ensuring the summary accounts for this
fourth status alongside the existing capabilities. Keep the per-project Docker
sandbox row labeled “Not covered” and make the legend, summary, and table
consistent.
- Line 63: Update the per-project Docker sandbox row in the parity document to
remove any claim that sandbox mounts or bind mounts are realized. Mark the
capability as unavailable or plan-validation-only, consistent with
ironclaw_process_sandbox having no wired production execution backend and no
tracked sandbox Dockerfile; retain the existing references only if they
accurately describe the inert validation behavior.
- Around line 169-172: Update the “Per-project Docker sandbox” entry and its
nearby “These correspond to…” cross-reference so the documentation remains
accurate: either add the appropriate sandbox follow-up/source ticket or section,
or narrow “These” to refer only to the automation/model-parity entries actually
covered by the listed references. Keep the existing parity status and unrelated
cutover references unchanged.
🪄 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: 1b700208-58f3-4dc7-b586-91a5213b3daa

📥 Commits

Reviewing files that changed from the base of the PR and between f3fbc8e and 672eff0.

📒 Files selected for processing (1)
  • docs/reborn/engine-v2-to-reborn-parity.md

Comment thread docs/reborn/engine-v2-to-reborn-parity.md
Comment thread docs/reborn/engine-v2-to-reborn-parity.md
Address CodeRabbit review 4783753432 on PR #6693: define the new "Not
covered" legend status, correct the WorkspaceReader row's stale claim
that per-project mounts are realized by the process-sandbox bind-mount
path (they're realized by the RootFilesystem mount catalog instead),
and narrow the "These correspond to" cross-reference so it doesn't
misattribute the sandbox gap to unrelated tracked epics.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 27, 2026 05:13
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6693 July 27, 2026 05:13 Destroyed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/reborn/engine-v2-to-reborn-parity.md`:
- Around line 184-187: Revise the blanket statement in the parity document so it
applies only to the learning/cutover gaps, or explicitly excludes the
per-project Docker sandbox gap. Preserve the engine-v2-specific scope
established in the execution-path discussion around lines 171-174.
🪄 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: 0825f285-fdd5-4eec-8c7e-b646e57eb8d4

📥 Commits

Reviewing files that changed from the base of the PR and between 672eff0 and 08d0ccd.

📒 Files selected for processing (1)
  • docs/reborn/engine-v2-to-reborn-parity.md

Comment on lines +184 to +187
above. Critically, **none of these gaps are engine-v2-specific**: they are
Reborn-vs-legacy-product deltas that exist independently of whether engine v2
is present, because engine v2 is gated off and no shipping path depends on
it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve the engine-v2 scope of the sandbox gap.

Lines 171-174 correctly describe the missing execution path for an engine-v2 capability, but this blanket statement says none of the gaps are engine-v2-specific. Narrow it to the learning/cutover gaps or explicitly exempt the per-project Docker sandbox.

Proposed wording
-Critically, **none of these gaps are engine-v2-specific**:
+Critically, the learning-mission and cutover gaps are not engine-v2-specific;
+the per-project Docker sandbox remains an uncovered engine-v2 capability:
📝 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.

Suggested change
above. Critically, **none of these gaps are engine-v2-specific**: they are
Reborn-vs-legacy-product deltas that exist independently of whether engine v2
is present, because engine v2 is gated off and no shipping path depends on
it.
above. Critically, the learning-mission and cutover gaps are not engine-v2-specific;
the per-project Docker sandbox remains an uncovered engine-v2 capability: they are
Reborn-vs-legacy-product deltas that exist independently of whether engine v2 is
present, because engine v2 is gated off and no shipping path depends on
it.
🤖 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/reborn/engine-v2-to-reborn-parity.md` around lines 184 - 187, Revise the
blanket statement in the parity document so it applies only to the
learning/cutover gaps, or explicitly excludes the per-project Docker sandbox
gap. Preserve the engine-v2-specific scope established in the execution-path
discussion around lines 171-174.

Source: Coding guidelines

@github-actions

Copy link
Copy Markdown
Contributor

Coverage ratchet

Ratchet mode: ENFORCING

RATCHET PASS: global
  observed: 85.51% (307055 / 359081 lines)
  floor:    80.81% (tolerance 0.5pp -> effective floor 80.31%)
  denominator: 359081 lines now vs 377084 at floor capture (-18003 lines, -4.77%) — not a material change

⚠️ 2 Reborn crate(s) have 0 int-tier coverage (target: 0) — ironclaw_prompt_envelope, ironclaw_scripts

Reborn integration-tier coverage

Line coverage (Reborn crates): 85.51% — 307055 / 359081 lines

Per-crate breakdown (60 crates, lowest-covered first)
Crate Line % Covered / Total
ironclaw_prompt_envelope 0% 0 / 88
ironclaw_scripts 0% 0 / 345
ironclaw_process_sandbox 33.91% 118 / 348
ironclaw_host_ingress 42.5% 17 / 40
ironclaw_event_projections 43.71% 684 / 1565
ironclaw_observability 61.54% 16 / 26
ironclaw_telegram_v2_adapter 62.35% 631 / 1012
ironclaw_authorization 62.98% 609 / 967
ironclaw_memory 70.15% 919 / 1310
ironclaw_trust 73.21% 664 / 907
ironclaw_filesystem 73.65% 4584 / 6224
ironclaw_wasm_limiter 74.6% 47 / 63
ironclaw_extractors 74.72% 538 / 720
ironclaw_capabilities 75.45% 2879 / 3816
ironclaw_mcp 76.2% 775 / 1017
ironclaw_projects 76.48% 400 / 523
ironclaw_reborn_cli 78.19% 10670 / 13647
ironclaw_telegram_extension 78.59% 962 / 1224
ironclaw_llm 78.95% 20989 / 26584
ironclaw_wasm 79.72% 735 / 922
ironclaw_memory_native 80.97% 3114 / 3846
ironclaw_auth 81.88% 6679 / 8157
ironclaw_first_party_extensions 82.38% 6682 / 8111
ironclaw_events 82.47% 1604 / 1945
ironclaw_host_api 82.65% 9120 / 11035
ironclaw_processes 83.3% 933 / 1120
ironclaw_reborn_identity 83.8% 450 / 537
ironclaw_operator 84.37% 5558 / 6588
ironclaw_secrets 84.56% 2798 / 3309
ironclaw_reborn_config 85.24% 2102 / 2466
ironclaw_skills 85.27% 4493 / 5269
ironclaw_extension_host 85.39% 18814 / 22032
ironclaw_reborn_composition 85.67% 24387 / 28465
ironclaw_run_state 85.77% 458 / 534
ironclaw_webui 85.87% 10914 / 12710
ironclaw_triggers 85.92% 2783 / 3239
ironclaw_network 85.97% 913 / 1062
ironclaw_reborn_event_store 86.51% 1251 / 1446
ironclaw_hooks 86.63% 9931 / 11464
ironclaw_extensions 86.98% 3669 / 4218
ironclaw_common 86.99% 1772 / 2037
ironclaw_approvals 87.18% 1543 / 1770
ironclaw_threads 87.2% 4851 / 5563
ironclaw_product 87.52% 19698 / 22507
ironclaw_reborn_traces 88.13% 11986 / 13600
ironclaw_turns 88.3% 14311 / 16208
ironclaw_slack_extension 88.47% 1934 / 2186
ironclaw_host_runtime 88.49% 18983 / 21451
ironclaw_reborn_openai_compat 89.32% 3780 / 4232
ironclaw_conversations 90.01% 3164 / 3515
ironclaw_resources 90.84% 4474 / 4925
ironclaw_runner 90.87% 17192 / 18920
ironclaw_event_streams 91.24% 1063 / 1165
ironclaw_loop_host 91.9% 16503 / 17958
ironclaw_attachments 93.06% 630 / 677
ironclaw_outbound 93.91% 4101 / 4367
ironclaw_agent_loop 94.88% 9837 / 10368
ironclaw_safety 95.28% 3858 / 4049
ironclaw_first_party_extension_ports 95.62% 3672 / 3840
ironclaw_runtime_policy 96.56% 813 / 842

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)
Module / Crate Reason Issue
crate: ironclaw_embeddings v1-only: consumed only by root ironclaw (src/app.rs, src/tools/builtin/memory.rs, src/workspace/mod.rs, src/config/{mod,embeddings}.rs); no crates/* dependents. Covered by "Tests (Legacy)". #5657
crate: ironclaw_gateway v1-only: consumed only by root ironclaw (src/channels/web/platform/static_files.rs, src/channels/web/handlers/frontend.rs); no crates/* dependents. Covered by "Tests (Legacy)". #5657
crate: ironclaw_tui v1-only: consumed only by root ironclaw (src/main.rs, src/channels/tui.rs); no crates/* dependents. Crate's own doc comment confirms it bridges INTO v1, not Reborn. Covered by "Tests (Legacy)". #5657

@henrypark133
henrypark133 merged commit 1cb8b1a into main Jul 27, 2026
64 checks passed
@henrypark133
henrypark133 deleted the sandbox/pr2-retire-docker-backend branch July 27, 2026 05:38
l3ocifer pushed a commit to l3ocifer/frick-ironclaw that referenced this pull request Sep 3, 2026
…earai#6693)

* fix(process_sandbox): retire dead DockerProcessSandboxBackend

DockerProcessSandboxBackend, its ProcessSandboxBackend trait, and the
broker/approval helpers around it (docker.rs, broker.rs, approval.rs,
backend.rs) had zero consumers outside this crate and were never
constructed in production: `HostProcessExecutor::process_sandbox_executor`
defaults to `None` (services.rs), and `with_process_sandbox_executor` is
only ever called from test fixtures. Dispatching
`system.process_sandbox.run` already returned `missing_process_sandbox_executor`
before this change; it still does after, since production wiring in
production.rs/process_executor.rs/services.rs is untouched. The dynamic
process-compatibility lane is superseded by the persistent per-user
sandbox and was already non-functional under the non-root/zero-cap
posture.

Kept: `PROCESS_SANDBOX_CAPABILITY_ID` (still routed by ID), the
`SandboxProcessPlan`/`ValidatedSandboxProcessPlan` types and their
validation (plan.rs, validation.rs), and `DEFAULT_PROCESS_SANDBOX_IMAGE`
plus tests/docker_security.rs, which exercises the built sandbox image
directly and has no dependency on the removed backend. Dropped the
async-trait/ironclaw_processes/secrecy/serde_json/tokio/tempfile deps
that only the deleted code used.

Refs nearai#6686

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(architecture): drop stale docker.rs ratchet entry

The frozen-path ratchet is shrink-only by design: it pins per-path
test-support counts so they can only go down, never back up. The
`docker.rs` entry tracked a method in the process-sandbox Docker
backend that PR nearai#6693 deletes outright, so the file no longer exists
and the baseline entry is dead weight, not a real regression risk.

Removing the entry is the fix, and the ratchet test itself is the
regression test: `cargo test -p ironclaw_architecture` now passes
because there is no stale path left to check for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(process-sandbox): describe crate as plan-validation-only

PR nearai#6693 deleted the Docker process-sandbox backend, but four docs
still described it as present: the crate's own CLAUDE.md guardrails,
the per-crate row in crates/AGENTS.md, FEATURE_PARITY.md's Docker
sandbox row, and the Reborn parity doc's per-project-sandbox row (plus
its accompanying note). Rewrite all four to match the crate's own
lib.rs doc comment: ironclaw_process_sandbox owns only the typed
SandboxProcessPlan contract and validation, no production backend is
wired for the capability today, and the parity docs now honestly mark
the capability as not covered rather than silently dropping the row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(reborn): reconcile sandbox parity row with Known Gaps/conclusion

CodeRabbit flagged that the sandbox row said Not covered but the Known
Gaps list and Conclusion still claimed every runtime concern (incl.
sandbox) maps cleanly to Reborn, and cited ironclaw_processes/
ironclaw_wasm/process_port.rs as parity evidence for a backend that
doesn't exist. Label those as non-backend/adjacent, point evidence at
the crate's own plan-validation tests, and add sandbox to Known Gaps
and the Conclusion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(reborn): fix parity doc legend, stale mount claim, gap cross-refs

Address CodeRabbit review 4783753432 on PR nearai#6693: define the new "Not
covered" legend status, correct the WorkspaceReader row's stale claim
that per-project mounts are realized by the process-sandbox bind-mount
path (they're realized by the RootFilesystem mount catalog instead),
and narrow the "These correspond to" cross-reference so it doesn't
misattribute the sandbox gap to unrelated tracked epics.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

This branch was successfully deployed

No deployments
ironclaw-ci-preview / ironclaw-pr-6693 — 08d0ccda Deployed Jul 27, 2026 by railway-app[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules scope: dependencies Dependency updates scope: docs Documentation size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants