test(reborn): wave-3 integration coverage — journeys, multi-user isolation, triggered/outbound, budget/comm-context/hooks seams, golden payloads, denied-edge contracts

[henrypark133]

test(reborn): wave-3 integration coverage — journeys, multi-user isolation, triggered/outbound, budget/comm-context/hooks seams, golden payloads, denied-edge contracts

Summary

Behavior-neutral coverage wave for the Reborn backend integration harness. Zero production-behavior changes: the diff is tests, test-support, and #[cfg(any(test, feature = "test-support"))]-gated composition accessors (compiled out of default builds — verified). Every test double substitutes only at seams production actively wires.

What's covered (new)

• Multi-turn journeys (reborn_group_journeys): the canary twin — auth gate → resolve → approval gate → approve → action with the authed+approved tool → follow-up with context carry-over; deny arms (auth deny → model-visible failure, run continues); deny-then-retry; multi-actor gate isolation (gate resolution + resume state bound to the raising actor).
• Multi-user isolation (reborn_group_multiuser): memory and auto-approve/approval-settings non-leak across actors, via a harness flag mirroring production's owner→actor resolution. (Confirmed [QA] Memories in the WebUI workspace are visible to every user in the workspace #5460 is a WebUI read-surface bug; the tool path isolates correctly.)
• Triggered runs: origin propagation contrast arm (ScheduledTrigger vs Inbound, mutation-verified at the sole production propagation site); gates raised mid-fire (approve + deny arms with side-effect proofs); triggered completion + reply persistence through a production-mirroring prompt materializer.
• Slack gate loop (crate tier): trigger fire → DM approval prompt (fake ProtocolHttpEgress at the production seam) → driver-recorded gate route → inbound bare "approve" resolves the gate on the run's foreign scope.
• Outbound target tools (reborn_integration_outbound_target): list/set happy paths, settings-Deny → policy_denied, facade NotFound → invalid_input, approval-gated set (approve applies / deny leaves state untouched) — real production capability code, double only at the facade trait seam.
• Budget + communication-context wiring (reborn_integration_budget, reborn_integration_comm_context): production build_default_budget_accountant and communication_context_provider seams live in the harness (semantics remain crate-tier — not re-authored).
• Hooks infra (reborn_integration_hooks): recording/denying hook factories + wiring; scenarios land #[ignore]d RED on production bug reborn: HookedLoopCheckpointPort does not forward stage_checkpoint_payload/load_checkpoint_payload — any hooks-enabled coordinator turn fails at Checkpoint stage #5572 (checkpoint-forward gap) — un-ignore with the fix.
• Golden inference payloads (reborn_integration_golden_payload): exact-match canonicalized full model-request payloads (insta snapshots; single volatile field = loop-start clock) + exact final-reply asserts — pins end-to-end prompt construction.
• Disabled-capability contract (in reborn_integration_tool_call): disabled ids never offered in the tool surface; hallucinated call to a disabled id → pinned terminal model_error contract (UX follow-up: reborn: hallucinated call to a disabled capability fails the run as model_error instead of a model-visible denial #5583).
• Blocked/denied edge batch (C-DENYEDGE): wrong-scope triggered resume ("turn run not found", with non-vacuity control), cross-tenant secret read (fail-closed UnknownSecret + correct-tenant re-read control), triggered self-create deny (int-tier fix(reborn): scheduled-trigger fires cannot create/mutate triggers (#5505) #5515 twin: never dispatched + trigger_list proves nothing created), busy-lock release after a Failed run (accepted resubmit, wedge-class), double-resolve (NotPending) + stale gate-ref resume ("gate resolution reference mismatch"), MissingGate bare-resolve, MCP output-too-large (output_too_large, run recovers). Four rows skipped with documented findings: never-registered capability ids are silently dropped pre-stage (no observable), extension installs are tenant-wide by design (no user-scoped seam), no drain seam on the int harness, HostInternal visibility deferred.
• MCP wire-framing matrix: crate-tier framing tests + int-tier SSE-framed handshake/tool-call over the loopback mock; salvaged int-tier regressions for the [codex] fix exa mcp sse initialize parsing #5573 fix.
• Multi-turn assertion infra: baseline-sliced assert_tool_error*_since, conversation-history containment asserts (fail-loud on out-of-range baselines), script-discipline docs.

Assertion meticulousness + de-bloat

A dedicated audit reviewed all 113 wave-3 assertions against the #5573 failure class (weak asserts letting legal-but-untested behavior slip). All 9 HIGH findings fixed (2 adapted with traced justification), 13/16 MED, 6 LOW. Separate commits trim comment bloat suite-wide (−362/+170 across 29 files): play-by-play and history narration removed; why-pins, product decisions, and seam contracts kept.

Production findings from this wave (issues, not fixes here)

• reborn: HookedLoopCheckpointPort does not forward stage_checkpoint_payload/load_checkpoint_payload — any hooks-enabled coordinator turn fails at Checkpoint stage #5572 hooks checkpoint-forward gap (tests ride here RED+ignored)
• reborn: skill trust ceiling (attenuate_tools) never ported — Installed skills do not narrow the tool surface #5581 skill trust ceiling never ported to Reborn
• reborn: force_compact_on_next_iteration is a dead letter — ActiveTaskPreservingCompactionStrategy never reads the overflow flag #5582 compaction overflow flag is a dead letter
• reborn: hallucinated call to a disabled capability fails the run as model_error instead of a model-visible denial #5583 hallucinated disabled-tool call fails run as model_error
• Two harness-masked bugs FIXED in test support: auth-resume forwarding on RecordingHostRuntime; resume idempotency key now (run_id, gate_ref)-scoped.

Verification

• cargo build --tests --all-features; cargo clippy --all --tests --examples --all-features -- -D warnings
• All wave-3 bins + regression sweep green under --features integration (0 failures)
• Composition crate --lib (incl. slack-v2-host-beta lane) green at --test-threads=4
• Behavior-neutral proof: default-features cargo build -p ironclaw_reborn_composition compiles all additions out
• Mutation/flip spot-checks per new mechanic (RED for the right reason, reverted)

Follow-ups (tracked, not in this PR)

Consolidation task (group.rs setter extraction, persisted_tool_error_summaries → _since(0) dedup, harness.rs split); chained triggered-origin gated journey; golden parallel-tool-calls + image-parts scenarios; compaction golden (blocked on #5582); needs-seam edge cases (lease-TTL clock, real egress pipeline under harness, admission-cap builder, reopen-resume-through-gate).

🤖 Generated with Claude Code

[Copilot]

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

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/support/reborn/assertions.rs (1)

270-303: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Duplicate collector logic — unify on [baseline..].

persisted_tool_error_summaries and persisted_tool_error_summaries_since are identical except for the slice (full history vs. [baseline..]). The full-history variant can just delegate to the baseline-scoped one with baseline = 0, removing ~30 duplicated lines of decode/truncate logic that would otherwise need synchronized edits.

♻️ Proposed dedup

     async fn persisted_tool_error_summaries(&self) -> HarnessResult<Vec<String>> {
-        let history = self
-            .thread_harness
-            .history(self.binding.thread_id.clone())
-            .await?;
-        history
-            .iter()
-            .filter(|message| message.kind == ironclaw_threads::MessageKind::ToolResultReference)
-            .map(|message| {
-                let Some(content) = message.content.as_deref() else {
-                    return Err("ToolResultReference message missing content".into());
-                };
-                serde_json::from_str::<ironclaw_threads::ToolResultReferenceEnvelope>(content)
-                    .map(|envelope| envelope.safe_summary.as_str().to_string())
-                    .map_err(|err| {
-                        let truncated = match content.char_indices().nth(200) {
-                            Some((cutoff, _)) => format!("{}...[truncated]", &content[..cutoff]),
-                            None => content.to_string(),
-                        };
-                        format!(
-                            "failed to decode ToolResultReferenceEnvelope: {err}; raw: {truncated}"
-                        )
-                        .into()
-                    })
-            })
-            .collect()
+        self.persisted_tool_error_summaries_since(0).await
     }

Also applies to: 693-722

🤖 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 `@tests/support/reborn/assertions.rs` around lines 270 - 303, The two
collectors duplicate the same history filtering and ToolResultReferenceEnvelope
decode/truncation logic; make `persisted_tool_error_summaries` delegate to
`persisted_tool_error_summaries_since` with a baseline of 0. Keep the shared
implementation in `persisted_tool_error_summaries_since` and have both methods
use the same `thread_harness.history` handling,
`MessageKind::ToolResultReference` filter, and error formatting so future edits
stay in one place.

🤖 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_reborn_composition/src/runtime.rs`:
- Around line 906-943: Add the required arch-exempt marker immediately above the
`#[allow(clippy::too_many_arguments)]` on
`wrap_outbound_delivery_capabilities_for_test`; the forwarder currently has only
a doc comment, so include a `// arch-exempt: too_many_args, ... , plan `#NNNN``
tag with a short rationale. Keep the annotation paired with the existing
test-only wrapper in `runtime.rs`, and ensure the tag is placed directly before
the allow attribute so the exemption is recognized.

In
`@crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs`:
- Around line 82-133: The `wrap_outbound_delivery_capabilities_for_test`
function has a `#[allow(clippy::too_many_arguments)]` without the required
arch-exempt tag. Add the missing `// arch-exempt: too_many_args, <reason>, plan
`#NNNN`` comment directly above the allow on this function, using the function
name to locate it; this is the root implementation in
`runtime/local_dev/outbound_delivery.rs`, and the same rule still applies
separately to any downstream forwarders that also carry their own
`#[allow(...)]`.

In `@crates/ironclaw_reborn_composition/src/test_support/outbound_delivery.rs`:
- Around line 26-59: The wrap_outbound_delivery_capabilities_for_test function
is missing the required arch-exempt note for its clippy suppression. Add the
repo-standard `// arch-exempt: too_many_args, <reason>, plan `#NNNN`` comment
immediately associated with the existing `#[allow(clippy::too_many_arguments)]`
in this test-support wrapper so it matches the pattern used in the other
outbound_delivery helpers.

In `@tests/reborn_integration_golden_payload.rs`:
- Around line 136-171: The blocker note documents a real production gap in
ActiveTaskPreservingCompactionStrategy::should_compact, where the
force-compaction flag from
state.compaction_state.force_compact_on_next_iteration is never consulted.
Please verify there is a tracking issue for this dead-letter path and, if not,
add one and reference it from the reborn_integration_golden_payload test comment
so the finding is not left only as narrative text.

In `@tests/reborn_integration_hooks.rs`:
- Around line 1-146: The tests are intentionally RED and should stay ignored
until the checkpoint wrapper bug is fixed; keep the module doc, `#[ignore]`
reasons, and `TODO(reborn-hooks-checkpoint-forward)` aligned on the same root
cause in `HookedLoopCheckpointPort`. If you are doing the follow-up, implement
forwarding for `stage_checkpoint_payload` and `load_checkpoint_payload` through
`ironclaw_hooks::middleware::checkpoint_port::HookedLoopCheckpointPort` to its
inner port, then remove the `#[ignore]`s and update the regression notes so the
tests describe the now-working coordinator-path hook flow.

In `@tests/reborn_integration_skill_activate.rs`:
- Around line 143-187: The final assertion in
skill_activate_over_budget_surfaces_recoverable_failed uses a bare is_err() on
assert_model_request_contains, which can hide unrelated
request-capture/serialization failures. Update that check to follow the same
expect_err(...) plus message-prefix validation pattern already used in
skill_criteria_auto_activation_stays_off_on_coordinator_path, so the test only
passes when the skill instructions were truly not injected and not because of an
infra error.

---

Outside diff comments:
In `@tests/support/reborn/assertions.rs`:
- Around line 270-303: The two collectors duplicate the same history filtering
and ToolResultReferenceEnvelope decode/truncation logic; make
`persisted_tool_error_summaries` delegate to
`persisted_tool_error_summaries_since` with a baseline of 0. Keep the shared
implementation in `persisted_tool_error_summaries_since` and have both methods
use the same `thread_harness.history` handling,
`MessageKind::ToolResultReference` filter, and error formatting so future edits
stay in one place.

🪄 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: 0600beab-9405-4f6e-8f58-cae3d4af9dec

📥 Commits

Reviewing files that changed from the base of the PR and between 2b2e63f and 692f913.

⛔ Files ignored due to path filters (5)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
• tests/snapshots/golden_payload__context_surfacing.snap is excluded by !**/*.snap, !tests/snapshots/**
• tests/snapshots/golden_payload__greeting.snap is excluded by !**/*.snap, !tests/snapshots/**
• tests/snapshots/golden_payload__multi_turn.snap is excluded by !**/*.snap, !tests/snapshots/**
• tests/snapshots/golden_payload__tool_call.snap is excluded by !**/*.snap, !tests/snapshots/**

📒 Files selected for processing (72)

• Cargo.toml
• crates/ironclaw_first_party_extensions/tests/gsuite_core.rs
• crates/ironclaw_mcp/src/lib.rs
• crates/ironclaw_reborn_composition/src/extension_lifecycle.rs
• crates/ironclaw_reborn_composition/src/factory.rs
• crates/ironclaw_reborn_composition/src/oauth_provider_client/tests.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
• crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs
• crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs
• crates/ironclaw_reborn_composition/src/test_support/mod.rs
• crates/ironclaw_reborn_composition/src/test_support/outbound_delivery.rs
• tests/reborn_group_approvals/main.rs
• tests/reborn_group_approvals/scenario_approve_always_persists_cross_thread.rs
• tests/reborn_group_approvals/scenario_concurrent_dual_gate_resume.rs
• tests/reborn_group_approvals/scenario_failure_category_demasked.rs
• tests/reborn_group_approvals/scenario_gate_ref_edge_cases.rs
• tests/reborn_group_approvals/scenario_gate_then_approve.rs
• tests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rs
• tests/reborn_group_journeys/main.rs
• tests/reborn_group_journeys/scenario_auth_deny_then_retry_journey.rs
• tests/reborn_group_journeys/scenario_auth_then_approval_journey.rs
• tests/reborn_group_journeys/scenario_interactive_approval_journey.rs
• tests/reborn_group_journeys/scenario_multi_actor_gate_isolation.rs
• tests/reborn_group_multiuser/main.rs
• tests/reborn_group_multiuser/scenario_auto_approve_isolation_across_actors.rs
• tests/reborn_group_multiuser/scenario_memory_isolation_across_actors.rs
• tests/reborn_group_triggers/main.rs
• tests/reborn_group_triggers/scenario_trigger_self_create_denied.rs
• tests/reborn_group_triggers/scenario_triggered_gate.rs
• tests/reborn_integration_auth_failure.rs
• tests/reborn_integration_auth_gate.rs
• tests/reborn_integration_backend_matrix.rs
• tests/reborn_integration_budget.rs
• tests/reborn_integration_cancel.rs
• tests/reborn_integration_comm_context.rs
• tests/reborn_integration_durable.rs
• tests/reborn_integration_golden_payload.rs
• tests/reborn_integration_hooks.rs
• tests/reborn_integration_http_matcher.rs
• tests/reborn_integration_mcp.rs
• tests/reborn_integration_oauth_connect.rs
• tests/reborn_integration_oauth_refresh.rs
• tests/reborn_integration_outbound_target.rs
• tests/reborn_integration_profile.rs
• tests/reborn_integration_safety.rs
• tests/reborn_integration_secret_injection.rs
• tests/reborn_integration_secrets.rs
• tests/reborn_integration_skill_activate.rs
• tests/reborn_integration_tool_call.rs
• tests/reborn_integration_triggered_submit.rs
• tests/reborn_integration_web_access.rs
• tests/reborn_trace_first_party_tool_coverage.rs
• tests/support/mock_mcp_server.rs
• tests/support/reborn/CLAUDE.md
• tests/support/reborn/assertions.rs
• tests/support/reborn/builder.rs
• tests/support/reborn/capability_backend.rs
• tests/support/reborn/comm_context.rs
• tests/support/reborn/github.rs
• tests/support/reborn/golden.rs
• tests/support/reborn/group.rs
• tests/support/reborn/group_constructors.rs
• tests/support/reborn/harness.rs
• tests/support/reborn/harness_mcp.rs
• tests/support/reborn/harness_web_access.rs
• tests/support/reborn/hooks.rs
• tests/support/reborn/mod.rs
• tests/support/reborn/outbound_preferences.rs
• tests/support/reborn/session_thread.rs
• tests/support/reborn/triggered_submit.rs
• tests/support/trace_llm.rs

💤 Files with no reviewable changes (3)

• tests/reborn_integration_durable.rs
• tests/reborn_integration_backend_matrix.rs
• tests/reborn_integration_safety.rs

[serrrfirat]

Ran code-review-multi --force plus a thermonuclear code-quality pass on 1df3266a8a5bd939e489942c69b92a34eb0ddeb6.

Posted 4 inline findings:

• Medium maintainability: the central Reborn harness absorbs several new capability-specific seams instead of following the existing split-module pattern.
• Medium maintainability: triggered submit test support hand-mirrors the trusted trigger materializer path and can drift from the owning layer.
• Medium test coverage: MCP SSE framing coverage still does not drive tools/list discovery.
• Low concurrency/design: the outbound preferences facade records related state under separate mutexes.

Reviewer summary:

• Security: no findings.
• Bugs: no findings; that reviewer timed out initially and returned [] after a best-effort stop request.
• Performance/concurrency: 1 low finding posted.
• Tests: 1 medium finding posted. I did not post the ignored-hooks finding because the file documents the production checkpoint-forwarding blocker and the removal path.
• Conventions + thermonuclear review: 2 structural findings posted.

Note: the codebase graph was present but still incomplete after reindexing in this worktree, so I used targeted PR-worktree inspection for the final review. No local test suite was run by this review pass.

[serrrfirat]

Thermo/code-quality: this PR adds outbound state, product-auth, bundled-extension activation, synthetic wrappers, and GitHub auth setup into the central harness, taking it from 4709 to 5326 lines. The support guide already treats harness_mcp.rs and group_constructors.rs as split precedent and says harness_auth.rs is tracked. Can we decompose this before merge? A cleaner shape is harness_outbound.rs for OutboundTargetToolsParts/wrapping/assertion handles and harness_auth.rs for GitHub/auth/bundled-extension setup, leaving harness.rs as the shared state/delegation shell. Anchors: tests/support/reborn/CLAUDE.md:99-108, tests/support/reborn/CLAUDE.md:109-114.

[henrypark133]

Confirmed the numbers — harness.rs is 4709→5326 lines this PR. That's genuinely large, but the harness_auth.rs split is already tracked independent of this PR (the CLAUDE.md note predates this branch — verified against origin/main), and the PR body's Follow-ups section explicitly lists the harness.rs split under the consolidation task. Given this branch is 30+ commits deep and every other comment on this file is anchored to exact line numbers, a structural move now would invalidate those anchors mid-review with no correctness gain. Sharpening the follow-up commitment though: new capability-specific state should be born in harness_outbound.rs / harness_auth.rs as pub(super) factories going forward (the harness_mcp.rs pattern) instead of landing inline in harness.rs — so the file stops regrowing wave over wave rather than getting a one-time retroactive split.

[serrrfirat]

This helper now hand-mirrors most of ConversationContentRefMaterializer::materialize_prompt and then intentionally omits authorize_trigger_fire and validate_trusted_trigger_prompt. Even though this is test-only, trusted-trigger materialization is an ownership boundary called out in AGENTS.md:61, and duplicating it here means future changes to trigger binding/thread recording can drift from the integration path. Can we move a crate-owned test-support helper beside the real materializer that returns (TriggerMaterializedPrompt, TurnScope) for scripted gateway registration, then have this test support call that instead of rebuilding the resolve/thread/content-ref sequence by hand? That keeps trusted ingress construction in the owning layer and deletes most of this long function.

[henrypark133]

Agreed this is a real mirror, not a coincidence — submit_triggered_turn_scripted reconstructs the exact shape of trigger_resolve_request + record_trigger_prompt + the thread-message content ref from materialize_prompt (trigger_poller_trusted_submit.rs:171-213), and both helpers are already pub(crate) in that same file, alongside an existing TenantScopedTrustedTriggerFireAuthorizer test double. So a materialize_trigger_prompt_for_test(..) -> (TriggerMaterializedPrompt, TurnScope) helper reusing all three — running the REAL authorize+validate steps instead of skipping them — is a small mechanical extraction, and that's the exact shape planned. Holding it out of THIS diff deliberately: the branch's bar is zero new production-crate surface mid-review, and the extraction deserves its own test-first cycle with a mutation check proving harness output tracks production. Tracking as a fast-follow with this exact shape, not the general backlog.

[serrrfirat]

Thermo-nuclear maintainability review: behaviorally this looks test/support scoped, but I think there are a few structural cleanups worth considering before this harness gets harder to evolve.

[serrrfirat]

[P1] This helper mirrors production trigger materialization instead of reusing it. It reconstructs the trusted inbound binding, resolve request, thread recording, thread-message:<id> content ref, and gateway setup by hand, while the production path already owns most of that shape in ConversationContentRefMaterializer / record_trigger_prompt. This is a drift trap: future trigger-thread shape changes can break production while this harness keeps passing, or vice versa. Can we expose a #[cfg(feature = "test-support")] materializer helper/result from ironclaw_reborn_composition instead of duplicating the field-by-field flow in test support?

[henrypark133]

Agreed this is a real mirror, not a coincidence — submit_triggered_turn_scripted reconstructs the exact shape of trigger_resolve_request + record_trigger_prompt + the thread-message content ref from materialize_prompt (trigger_poller_trusted_submit.rs:171-213), and both helpers are already pub(crate) in that same file, alongside an existing TenantScopedTrustedTriggerFireAuthorizer test double. So a materialize_trigger_prompt_for_test(..) -> (TriggerMaterializedPrompt, TurnScope) helper reusing all three — running the REAL authorize+validate steps instead of skipping them — is a small mechanical extraction, and that's the exact shape planned. Holding it out of THIS diff deliberately: the branch's bar is zero new production-crate surface mid-review, and the extraction deserves its own test-first cycle with a mutation check proving harness output tracks production. Tracking as a fast-follow with this exact shape, not the general backlog.

[serrrfirat]

[P1] HostRuntimeCapabilityHarness is turning into an omnibus fixture with unrelated optional seams: outbound targets, run-owner scoping, product auth, network egress overrides, bundled extension activation, synthetic wrappers, and approval bookkeeping. This file is already over 5k lines after the PR, and this continues the pattern of making it a second composition framework. Can we decompose this first into fixture-family modules/structs, e.g. auth+extension harness, outbound-target harness, approval recording wrapper, and run-owner dispatch policy?

[henrypark133]

Confirmed the numbers — harness.rs is 4709→5326 lines this PR. That's genuinely large, but the harness_auth.rs split is already tracked independent of this PR (the CLAUDE.md note predates this branch — verified against origin/main), and the PR body's Follow-ups section explicitly lists the harness.rs split under the consolidation task. Given this branch is 30+ commits deep and every other comment on this file is anchored to exact line numbers, a structural move now would invalidate those anchors mid-review with no correctness gain. Sharpening the follow-up commitment though: new capability-specific state should be born in harness_outbound.rs / harness_auth.rs as pub(super) factories going forward (the harness_mcp.rs pattern) instead of landing inline in harness.rs — so the file stops regrowing wave over wave rather than getting a one-time retroactive split.

[serrrfirat]

[P2] This adds another capability-specific branch into apply_synthetic_capability_wrappers. The function is becoming the synthetic-capability dispatcher with per-feature special cases. If more synthetic capabilities land, this becomes the central spaghetti switch. A small registry/list of wrapper objects would let each fixture contribute a wrapper without editing this function every wave.

[henrypark133]

Looked hard at whether a registry earns its keep here. The three branches share only the outer 'if let Some(state) = &self.field { wrap_x(...) }' skeleton — the real complexity is the non-uniform wrap_* argument lists (project_create ~6 args; outbound 12 across facade/approval/auto-approve/overrides/policies; skill_activate a different backing-source type). A trait-object registry needs each wrapper to close over its backing state at construction, which just moves today's if-let into a build_wrappers() constructor — every new capability still needs a new struct + wire-up line, so per-wave editing cost is unchanged and we add a trait + boxes + dynamic dispatch on the same amount of code. Declining at n=3; will reconsider if a 4th capability lands with the same single-field shape. The outbound 12-arg signature is being fixed directly (typed parts bundle) per the sibling thread.

[ironloopai]

IronLoop Review Status

Head: 705934b2d046cbea0989a0ceab3d6ddbf86aad5b
Updated: 2026-07-03T17:29:26.994Z
Admission: webhook accepted the request and IronLoop persisted review state before this projection.

Current reviewers:

Reviewer	State	What it means	Last update
none	Queued	No reviewer jobs scheduled yet.	n/a

Recent activity:

Time	Reviewer	State	Detail
n/a	n/a	Waiting	No progress events recorded yet.

Commands:

• @ironloop review
• @ironloop review <agent-alias>
• @ironloop status

[Copilot]

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/support/reborn/harness.rs (1)

2582-2584: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Retain network_egress on the harness

file_and_github_auth_tools() moves the injected Arc<dyn NetworkHttpEgress> into RebornBuildInput, but Self { network_egress: None } drops the harness-side handle. network_http_requests() then stays empty for this fixture, so tests can’t assert the actual outbound GitHub calls. Clone the Arc before passing it into with_network_http_egress_for_test and store the clone on Self.

🤖 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 `@tests/support/reborn/harness.rs` around lines 2582 - 2584, The harness is
dropping its `network_egress` handle in `file_and_github_auth_tools`, so
`network_http_requests()` cannot observe outbound GitHub traffic for this
fixture. Update the `RebornBuildInput` setup to clone the injected `Arc<dyn
NetworkHttpEgress>` before calling `with_network_http_egress_for_test`, and keep
that clone on `Self { network_egress: ... }` so the harness retains access to
the same requests.

🤖 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 `@tests/support/reborn/harness.rs`:
- Around line 2582-2584: The harness is dropping its `network_egress` handle in
`file_and_github_auth_tools`, so `network_http_requests()` cannot observe
outbound GitHub traffic for this fixture. Update the `RebornBuildInput` setup to
clone the injected `Arc<dyn NetworkHttpEgress>` before calling
`with_network_http_egress_for_test`, and keep that clone on `Self {
network_egress: ... }` so the harness retains access to the same requests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 41afe109-b337-4eee-a9ff-b3c1495d16b6

📥 Commits

Reviewing files that changed from the base of the PR and between 1df3266 and 819e66b.

📒 Files selected for processing (8)

• crates/ironclaw_mcp/tests/mcp_adapter_contract.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs
• crates/ironclaw_reborn_composition/src/test_support/mod.rs
• crates/ironclaw_reborn_composition/src/test_support/outbound_delivery.rs
• tests/reborn_integration_skill_activate.rs
• tests/support/reborn/harness.rs
• tests/support/reborn/outbound_preferences.rs

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Cargo.toml (1)

283-285: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Gate ironclaw_hooks out of the default dependency graph. It’s added as a normal [dependencies] entry, so every default build pulls it in; move it behind test-support or into [dev-dependencies] if it’s only for the new test/support path.

🤖 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 `@Cargo.toml` around lines 283 - 285, The Cargo manifest currently pulls
ironclaw_hooks into the default dependency graph through a normal dependency
entry. Move the ironclaw_hooks declaration out of the main [dependencies]
section and place it behind the test-support path or into [dev-dependencies] if
it is only used by tests/support code, keeping the existing intent around the
hooks builders intact.

🤖 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 `@Cargo.toml`:
- Around line 283-285: The Cargo manifest currently pulls ironclaw_hooks into
the default dependency graph through a normal dependency entry. Move the
ironclaw_hooks declaration out of the main [dependencies] section and place it
behind the test-support path or into [dev-dependencies] if it is only used by
tests/support code, keeping the existing intent around the hooks builders
intact.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0f5791a5-9c32-4233-af45-1bf096f2f75b

📥 Commits

Reviewing files that changed from the base of the PR and between 819e66b and 705934b.

⛔ Files ignored due to path filters (3)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
• tests/snapshots/golden_payload__context_surfacing.snap is excluded by !**/*.snap, !tests/snapshots/**
• tests/snapshots/golden_payload__tool_call.snap is excluded by !**/*.snap, !tests/snapshots/**

📒 Files selected for processing (1)

• Cargo.toml