test(reborn): composition test-support accessors for WebUI approval/auth interaction services - #5654
Conversation
…uth interaction services Adds `RebornServices::local_dev_approval_interaction_service_for_test` / `local_dev_auth_interaction_service_for_test`, unblocking W5-WEBUI-API-2 (RESOLVE_GATE coverage for both approval and auth gate kinds) — the #5174 bug class. A harness that builds its own planned runtime directly (e.g. via `build_default_planned_runtime`, bypassing `build_reborn_runtime`) previously had no way to get a real `DefaultApprovalInteractionService` / auth-interaction-service pair, only the fail-closed `Rejecting*`/`Unavailable*` fallbacks. Production-crate touch: `crates/ironclaw_reborn_composition` gains a new `runtime/test_support.rs` file (the two accessors, ~75 lines) plus a 3-line `#[cfg(feature = "test-support")] #[path = "runtime/test_support.rs"] mod test_support;` in `runtime.rs`. Both the module declaration and every method inside are gated behind `#[cfg(feature = "test-support")]` (off by default; confirmed via rlib symbol diff that the module compiles to zero bytes without the feature). The accessors live in a `runtime`-tree submodule rather than `factory.rs` because the recipe they mirror depends on five module-private types only reachable from code inside `crate::runtime` (Rust's private-item visibility extends to descendant modules, not just the defining file) — and it's a new file rather than inlined into the 10k+-line `runtime.rs` for the same reason this crate already splits its own unit tests into `runtime/tests/*.rs` submodules. Zero behavior change: every call reuses the exact production constructors (`DefaultApprovalInteractionService::new`, `ApprovalResolverPort::new`, `LocalDevApprovalLeaseTermsProvider::new`, `build_webui_auth_interaction_service`) with the exact arguments the production `build_reborn_runtime` recipe already assembles. `turn_coordinator` is an explicit `Arc<dyn TurnCoordinator>` parameter, not `self.turn_coordinator`: a `RebornServices` built by `build_reborn_services` alone carries a different `TurnCoordinator` instance than a caller-built planned runtime (e.g. `RebornIntegrationGroup`'s own coordinator) drives its turns through. Exercised by a new smoke test in `crates/ironclaw_reborn_composition/tests/runtime.rs` (`local_dev_test_support_interaction_service_accessors_build_real_services`) that builds a live local-dev runtime, calls both accessors with the runtime's own coordinator, and asserts each returns a real, working service (`Ok` with an empty pending list) rather than a fail-closed fallback — mutation-verified by temporarily forcing each accessor to its fail-closed path and confirming the test goes red, then reverting. The actual RESOLVE_GATE test scenarios these accessors unblock are W5-WEBUI-API-2's own follow-on PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
size: M
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a feature-gated ChangesLocal-dev test-support accessors
Estimated code review effort: 4 (Complex) | ~35 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant RebornServices
participant LocalDevRuntime
participant DefaultApprovalInteractionService
participant WebUIAuthFactory
Test->>RebornServices: local_dev_approval_interaction_service_for_test(turn_coordinator)
RebornServices->>LocalDevRuntime: build local-dev policy + read model
RebornServices->>DefaultApprovalInteractionService: construct approval service
RebornServices-->>Test: Ok(Some(...))
Test->>RebornServices: local_dev_auth_interaction_service_for_test(turn_coordinator)
RebornServices->>LocalDevRuntime: read turn state
RebornServices->>WebUIAuthFactory: build_webui_auth_interaction_service
RebornServices-->>Test: Some(...)
Test->>DefaultApprovalInteractionService: resolve approval gate
DefaultApprovalInteractionService->>RebornServices: use supplied turn_coordinator
RebornServices-->>Test: resume_turn observed once
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 test-support accessors on RebornServices to build real, working instances of DefaultApprovalInteractionService and AuthInteractionService for local-dev testing, along with a smoke test to verify their construction and documentation in CLAUDE.md. The review feedback suggests replacing silent fallbacks like .ok()? with explicit .expect(...) calls within these test-support methods to ensure setup failures are reported with clear context rather than silently returning None.
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.
| turn_coordinator: Arc<dyn TurnCoordinator>, | ||
| ) -> Option<Arc<dyn ApprovalInteractionService>> { | ||
| let local_runtime = self.local_runtime.as_ref()?; | ||
| let local_dev_capability_policy = Arc::new(local_dev_capability_policy().ok()?); |
There was a problem hiding this comment.
In Rust tests, prefer using .expect() with a descriptive message instead of fallbacks like .ok()? or .unwrap() to ensure that setup failures are explicitly reported with clear context, rather than silently returning None and causing downstream test failures or unexpected behavior.
| let local_dev_capability_policy = Arc::new(local_dev_capability_policy().ok()?); | |
| let local_dev_capability_policy = Arc::new( | |
| local_dev_capability_policy() | |
| .expect("failed to resolve local-dev capability policy in test accessor") | |
| ); |
References
- In Rust tests, prefer using
.expect()with a descriptive message instead of.unwrap()or fallbacks likeunwrap_or_else()to ensure that failures are explicitly reported with clear context.
There was a problem hiding this comment.
Superseded in 0f4b69f by a broader fix: rather than .expect() (which would only turn a silent None into a test panic), the fallible construction now propagates via Result<Option<...>, RebornRuntimeError>, matching how build_reborn_runtime itself handles this error.
| .with_persistent_grantee_resolver(Arc::new( | ||
| RegistryPersistentApprovalGranteeResolver::new(Arc::clone( | ||
| &local_runtime.extension_registry, | ||
| )) | ||
| .ok()?, | ||
| )) |
There was a problem hiding this comment.
In Rust tests, prefer using .expect() with a descriptive message instead of fallbacks like .ok()? or .unwrap() to ensure that setup failures are explicitly reported with clear context, rather than silently returning None and causing downstream test failures or unexpected behavior.
| .with_persistent_grantee_resolver(Arc::new( | |
| RegistryPersistentApprovalGranteeResolver::new(Arc::clone( | |
| &local_runtime.extension_registry, | |
| )) | |
| .ok()?, | |
| )) | |
| .with_persistent_grantee_resolver(Arc::new( | |
| RegistryPersistentApprovalGranteeResolver::new(Arc::clone( | |
| &local_runtime.extension_registry, | |
| )) | |
| .expect("failed to build persistent grantee resolver in test accessor") | |
| )) |
References
- In Rust tests, prefer using
.expect()with a descriptive message instead of.unwrap()or fallbacks likeunwrap_or_else()to ensure that failures are explicitly reported with clear context.
There was a problem hiding this comment.
Same as the sibling comment on line 24 — superseded in 0f4b69f by propagating via Result<Option<...>, RebornRuntimeError> instead of .expect(), since the grantee-resolver construction now lives inside the shared build_local_dev_approval_interaction_service helper.
![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 `@crates/ironclaw_reborn_composition/src/runtime/test_support.rs`:
- Around line 39-82: The test-only accessor in
local_dev_approval_interaction_service_for_test duplicates the approval-service
wiring from the production runtime path, which risks drift. Extract the shared
construction recipe for DefaultApprovalInteractionService and its attached
lease-terms provider, read model, resolver, persistent policy/grantee stores,
and tool override store into a private helper shared by
build_reborn_runtime/local_runtime_parts and this test-support method. Keep the
helper in crate::runtime so both production and test wiring use the same symbols
and stay identical.
- Around line 39-82: The `local_dev_approval_interaction_service_for_test`
helper is collapsing construction failures into `None` via `.ok()?`, which hides
real errors from `local_dev_capability_policy()` and
`RegistryPersistentApprovalGranteeResolver::new`. Change this path to surface
failures explicitly, ideally by returning a `Result` with context instead of
`Option`, so callers can distinguish missing `local_runtime` from
policy/resolver initialization errors. Keep the fix localized around
`local_dev_approval_interaction_service_for_test` and the
`ApprovalInteractionService` assembly chain.
🪄 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: 001252ac-6769-44ed-af15-9abd4a15b5eb
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/test_support.rscrates/ironclaw_reborn_composition/tests/runtime.rstests/integration/CLAUDE.md
Reborn integration-tier coverageLine coverage (Reborn crates): 28.54% — 49278 / 172648 lines Per-crate breakdown (62 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. Exemptions (0 file(s) excluded from the accounting above)No exemptions configured. |
|
🚅 Deployed to the ironclaw-pr-5654 environment in ironclaw-ci-preview
|
Compress test_support.rs and runtime.rs comments/doc-comments from multi-paragraph narratives to their load-bearing crux (audit-sink divergence, turn_coordinator instance rationale, module-privacy note); tighten the corresponding CLAUDE.md reference bullets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add test-support accessors so WebUI approval/auth interaction services are reachable in Reborn composition integration tests.
Stats: 4 findings (from 6 raw, 4 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Conventions
- Medium Do not collapse test-support wiring errors into None (
crates/ironclaw_reborn_composition/src/runtime/test_support.rs:24-24, confidence 90) — anchor:.claude/rules/error-handling.md:13
The approval accessor documentsNoneas meaning there is no local-dev runtime, but the new.ok()?calls turn an invalid local-dev capability policy or grantee resolver construction failure into the sameNone. This also overlaps a maintainability concern: the approval accessor duplicates the production approval-service wiring recipe instead of sharing one helper. - Low Accessor docs omit the required test-only note (
crates/ironclaw_reborn_composition/src/runtime/test_support.rs:15-17, confidence 75) — anchor:crates/ironclaw_reborn_composition/CLAUDE.md:6
The new public*_for_testrustdoc names the production wiring it mirrors, but the per-method docs do not explicitly say the handles are tests-only and gated behindtest-support.
Tests
- Medium Accessor test does not prove supplied coordinator is used (
crates/ironclaw_reborn_composition/src/runtime/test_support.rs:51-76, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/runtime/test_support.rs:51
The smoke test callslist_pending, but both interaction services use the suppliedTurnCoordinatoron resolve/resume paths. A regression that ignored the parameter or used a stale coordinator would still pass. - Low Non-local runtime None behavior is untested (
crates/ironclaw_reborn_composition/src/runtime/test_support.rs:23-72, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/runtime/test_support.rs:23
Both accessors document returningNonewithout a local-dev runtime, but the added test only covers the local-devSomepath.
| turn_coordinator: Arc<dyn TurnCoordinator>, | ||
| ) -> Option<Arc<dyn ApprovalInteractionService>> { | ||
| let local_runtime = self.local_runtime.as_ref()?; | ||
| let local_dev_capability_policy = Arc::new(local_dev_capability_policy().ok()?); |
There was a problem hiding this comment.
Medium — Do not collapse test-support wiring errors into None.
The accessor documents None as meaning there is no local-dev runtime, but this .ok()? also turns a local-dev capability-policy construction failure into None; the later grantee resolver .ok()? does the same. That silently drops wiring errors and diverges from production, which propagates these failures. It is also a maintainability smell because the approval-service construction is now hand-copied from production wiring.
Fix: Extract the production approval interaction construction into a private helper shared by build_reborn_runtime and this accessor, and return Result<Option<Arc<dyn ApprovalInteractionService>>, RebornRuntimeError> so only an absent local runtime maps to Ok(None).
Also flagged by: maintainability/Medium
There was a problem hiding this comment.
Fixed in 0f4b69f: extracted build_local_dev_approval_interaction_service (shared by build_reborn_runtime + this accessor); accessor now returns Result<Option<...>, RebornRuntimeError> so capability-policy/grantee-resolver failures propagate instead of collapsing to None.
| ), | ||
| )), | ||
| approval_resolver, | ||
| turn_coordinator, |
There was a problem hiding this comment.
Medium — Accessor test does not prove supplied coordinator is used.
The new accessors take a caller-supplied TurnCoordinator, but the added smoke test only calls list_pending. Both approval and auth interaction services use this coordinator on resolve/resume paths, so a regression that ignored the parameter or used a stale service-owned coordinator would still pass.
Fix: Add a caller-level test, for example tests::runtime::local_dev_test_support_interaction_services_use_supplied_turn_coordinator_on_resolve, that drives approval/auth resolve through services built with a supplied coordinator.
There was a problem hiding this comment.
Fixed in 0f4b69f: added local_dev_test_support_interaction_services_use_supplied_turn_coordinator_on_resolve — drives a real builtin.write_file approval gate to BlockedApproval and resolves it through a service built with a spy TurnCoordinator wrapping the runtime's own; asserts only the spy's resume_turn fires.
| &self, | ||
| turn_coordinator: Arc<dyn TurnCoordinator>, | ||
| ) -> Option<Arc<dyn ApprovalInteractionService>> { | ||
| let local_runtime = self.local_runtime.as_ref()?; |
There was a problem hiding this comment.
Low — Non-local runtime None behavior is untested.
Both new public test-support accessors document returning None without a local-dev runtime, but the added test only covers the local-dev Some path. No adjacent test covers the non-local/disabled-services branch for either accessor.
Fix: Add tests::runtime::local_dev_test_support_interaction_service_accessors_return_none_without_local_dev_runtime covering both accessors on non-local services.
There was a problem hiding this comment.
Fixed in 0f4b69f: added local_dev_test_support_interaction_service_accessors_return_none_without_local_dev_runtime, covering both accessors against RebornServices::disabled().
| use super::*; | ||
|
|
||
| impl RebornServices { | ||
| /// Real `DefaultApprovalInteractionService` wired like `build_reborn_runtime`. |
There was a problem hiding this comment.
Low — Accessor docs omit the required test-only note.
The rustdoc names the production wiring it mirrors, but the per-method docs do not explicitly say the handle is tests-only and gated behind test-support; that caveat currently lives only in file-level comments/PR text, so generated docs and hover text miss it.
Fix: Add a doc-comment sentence to both accessors, for example: For tests only -- gated behind test-support, ships zero bytes in production builds.
Also flagged by: local-patterns/Low
There was a problem hiding this comment.
Fixed in 0f4b69f: both accessors now have an explicit "For tests only -- gated behind test-support, ships zero bytes in production builds." doc line.
…s into None local_dev_approval_interaction_service_for_test masked local-dev capability-policy and grantee-resolver construction failures behind `.ok()?`, diverging from production (which propagates via `?`), and hand-duplicated the DefaultApprovalInteractionService wiring recipe. Extract build_local_dev_approval_interaction_service as the single shared recipe for build_reborn_runtime and the test accessor; the accessor now returns Result<Option<...>, RebornRuntimeError> so only a genuinely-absent local-dev runtime maps to Ok(None). Also covers two other review gaps: a caller-level test proving the supplied TurnCoordinator (not a stale/runtime-owned one) drives approval resolve/resume, and a test for the non-local-dev None branch on both *_for_test accessors. Doc comments now note test-only/ test-support gating explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
github-actions
Bot
added
size: L
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 0f4b69ff8b1dc3cba248388f0c52824d85539881
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the reviewed diff. The changes keep the approval/auth interaction accessors behind test-support, share the production local-dev approval wiring, and add caller-level tests for the supplied TurnCoordinator path.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
Summary
Enabler for the W5-WEBUI-API-2 coverage lane (mid-gate credential refresh scenarios over BOTH approval and auth gate kinds — the #5174 bug class). The integration harness builds its runtime via
build_default_planned_runtimeand never constructs composition's real interaction services, so gate-dispatch paths are unreachable at the integration tier by construction. This PR adds the two accessors that close that gap; the test lane consuming them follows in a separate PR.This PR touches
crates/ironclaw_reborn_composition— deliberately minimal and inert in production builds:src/runtime/test_support.rs(~104 lines) declared via a 3-line#[cfg(feature = "test-support")] #[path = ...] mod test_support;inruntime.rs, following the existing#[path = "runtime/tests/*.rs"]submodule convention in the same file.runtime.rsitself grows by 3 lines.RebornServices:local_dev_approval_interaction_service_for_testandlocal_dev_auth_interaction_service_for_test, matching the existinglocal_dev_*_for_testpattern. Both taketurn_coordinatoras an explicit parameter because harness callers build their planned runtime independently ofbuild_reborn_runtime, soself's coordinator would be a different instance.test-supportedge to this crate in the workspace is under[dev-dependencies](root, crate-self, reborn CLI). Verified by building the crate with and without the feature and grepping the rlib for the accessor symbols: 0 occurrences without, 8 with.approval_audit_sink(defaultsNone, audit-recording only) is omitted; noted in the accessor doc comment as a drift guard.If this touch is unwanted, the single commit drops cleanly without affecting sibling W5 lanes.
Test plan
local_dev_test_support_interaction_service_accessors_build_real_servicesextends the existingtests/runtime.rssuite: builds a live local-dev runtime, calls both accessors with the runtime's own coordinator, asserts each answersOkwith an empty pending list — discriminating vs the harness'sRejecting*/Unavailable*stubs, which return errors.Nonereturn / droppedproduct_authwiring → RED with the expected panics; reverted → GREEN).cargo test -p ironclaw_reborn_composition --test runtime --features test-support→ 9 passedcargo clippy --all --benches --tests --examples --all-features→ zero warnings; workspacecargo buildclean🤖 Generated with Claude Code