test(reborn): StaleSurface same-run refresh pin + extension-remove channel-cleanup integration coverage - #6055
henrypark133 wants to merge 3 commits into
Conversation
…el cleanup (#5950/#6026 salvage, #5851 salvage) A1: prove RefreshingLocalDevCapabilityPort's surface refresh makes a just-activated extension's capability dispatchable within the SAME run (install -> activate -> dispatch in one turn), not just across a new run like scenario 5 already covered. B: salvage int-tier coverage for #5851's extension-remove channel cleanup (extension_lifecycle.rs::remove -> cleanup_channel_before_remove -> disconnect_channel_for_cleanup), previously only pinned by a slack-v2-host-beta-gated crate test. Adds a test-support accessor to fill the harness's deferred ChannelConnectionFacade slot, plus positive (slack, Required-kind, unconditional disconnect) and negative (google-drive, IfConnectionFacadeSupportsChannel, facade-gated disconnect) scenarios. All four new scenarios are mutation-verified against their production call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vage Fold scenario 8 (google-drive, no-disconnect) into scenario 7 as a second install/remove pair in the shared group, keeping the negative case exact- equality-checked against the same disconnects() vector instead of standing up a second group boot for one assertion (consolidate-don't-proliferate). Extract the byte-identical facade-slot-fill body shared by RebornRuntime::set_channel_connection_facade (prod, slack-v2-host-beta) and the test-only accessor into one RebornServices::fill_channel_connection_facade_slot recipe so the two can never drift, and condense two over-long module/doc comments to invariant + issue-ref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
size: S
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change centralizes channel facade slot initialization, adds test-support access, introduces a recording facade double, and expands integration coverage for channel-extension removal cleanup and same-run Google Calendar activation and dispatch. ChangesExtension lifecycle behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 refactors the channel-connection facade slot management by centralizing its setup in RebornServices and introducing test-only accessors. It also adds integration tests to verify channel disconnection on extension removal and capability surface refreshing within a single run. The review feedback suggests simplifying the test double by removing a redundant Arc indirection, cleaning up assertions in the new integration tests to avoid unnecessary cloning, and serializing tests that modify shared global state using a mutex to prevent race conditions.
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.
| #[derive(Default)] | ||
| pub(crate) struct RecordingChannelConnectionFacade { | ||
| connections: HashMap<String, bool>, | ||
| disconnects: Arc<Mutex<Vec<(String, String)>>>, | ||
| } | ||
|
|
||
| impl RecordingChannelConnectionFacade { | ||
| pub(crate) fn with_connections(entries: &[(&str, bool)]) -> Self { | ||
| Self { | ||
| connections: entries | ||
| .iter() | ||
| .map(|(channel, connected)| ((*channel).to_string(), *connected)) | ||
| .collect(), | ||
| disconnects: Arc::new(Mutex::new(Vec::new())), | ||
| } | ||
| } |
There was a problem hiding this comment.
The disconnects field is wrapped in an Arc (Arc<Mutex<Vec<(String, String)>>>), but RecordingChannelConnectionFacade itself is already instantiated and shared via Arc<RecordingChannelConnectionFacade> (and coerced to Arc<dyn ChannelConnectionFacade>) in the test. This results in a redundant double-Arc indirection.
We can simplify this by removing the inner Arc and making disconnects a direct Mutex<Vec<(String, String)>>.
| #[derive(Default)] | |
| pub(crate) struct RecordingChannelConnectionFacade { | |
| connections: HashMap<String, bool>, | |
| disconnects: Arc<Mutex<Vec<(String, String)>>>, | |
| } | |
| impl RecordingChannelConnectionFacade { | |
| pub(crate) fn with_connections(entries: &[(&str, bool)]) -> Self { | |
| Self { | |
| connections: entries | |
| .iter() | |
| .map(|(channel, connected)| ((*channel).to_string(), *connected)) | |
| .collect(), | |
| disconnects: Arc::new(Mutex::new(Vec::new())), | |
| } | |
| } | |
| #[derive(Default)] | |
| pub(crate) struct RecordingChannelConnectionFacade { | |
| connections: HashMap<String, bool>, | |
| disconnects: Mutex<Vec<(String, String)>>, | |
| } | |
| impl RecordingChannelConnectionFacade { | |
| pub(crate) fn with_connections(entries: &[(&str, bool)]) -> Self { | |
| Self { | |
| connections: entries | |
| .iter() | |
| .map(|(channel, connected)| ((*channel).to_string(), *connected)) | |
| .collect(), | |
| disconnects: Mutex::new(Vec::new()), | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 62bc6c8 — inner Arc removed, disconnects is now a direct Mutex<Vec<..>>.
| let expected_user = capability_harness.capability_user_id().as_str().to_string(); | ||
| let disconnects = facade.disconnects(); | ||
| if disconnects != vec![(expected_user.clone(), "slack".to_string())] { | ||
| return Err(format!( | ||
| "builtin.extension_remove must disconnect the run owner's slack channel binding \ | ||
| exactly once (Required cleanup) and never google-drive \ | ||
| (IfConnectionFacadeSupportsChannel, key absent); expected \ | ||
| [({expected_user:?}, \"slack\")], got {disconnects:?}" | ||
| ) | ||
| .into()); |
There was a problem hiding this comment.
We can simplify the assertion and avoid cloning expected_user by defining the expected vector directly and asserting against it. This also makes the error message cleaner by formatting the entire expected vector.
| let expected_user = capability_harness.capability_user_id().as_str().to_string(); | |
| let disconnects = facade.disconnects(); | |
| if disconnects != vec![(expected_user.clone(), "slack".to_string())] { | |
| return Err(format!( | |
| "builtin.extension_remove must disconnect the run owner's slack channel binding \ | |
| exactly once (Required cleanup) and never google-drive \ | |
| (IfConnectionFacadeSupportsChannel, key absent); expected \ | |
| [({expected_user:?}, \"slack\")], got {disconnects:?}" | |
| ) | |
| .into()); | |
| let expected_user = capability_harness.capability_user_id().as_str().to_string(); | |
| let disconnects = facade.disconnects(); | |
| let expected = vec![(expected_user, "slack".to_string())]; | |
| if disconnects != expected { | |
| return Err(format!( | |
| "builtin.extension_remove must disconnect the run owner's slack channel binding \ | |
| exactly once (Required cleanup) and never google-drive \ | |
| (IfConnectionFacadeSupportsChannel, key absent); expected \ | |
| {expected:?}, got {disconnects:?}" | |
| ) | |
| .into()); | |
| } |
There was a problem hiding this comment.
Fixed in 62bc6c8 — expected vec built once, compared directly, error message formats {expected:?}.
| if !services.set_channel_connection_facade_for_test(facade.clone()) { | ||
| return Err("channel-connection facade slot already filled or no local runtime".into()); | ||
| } |
There was a problem hiding this comment.
Since services is shared across all scenarios in extensions_group_e2e(), setting the OnceLock via set_channel_connection_facade_for_test modifies shared global state. To prevent race conditions and flakiness when tests are run in parallel, ensure that tests modifying this shared global state are serialized using a mutex.
References
- Tests that modify shared global state should be serialized using a mutex to prevent race conditions and flakiness when run in parallel.
There was a problem hiding this comment.
Declined — no parallelism exists here. All group scenarios run sequentially inside the single extensions_group_e2e #[tokio::test] (report.record(scenario::run(&g).await) in order), and channel_connection_facade_slot is a per-runtime-instance Arc<OnceLock<..>> (constructed per RebornLocalRuntimeServices build), not process-global state. A serializing mutex would guard against a race that cannot occur.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.38% — 296976 / 347845 lines Per-crate breakdown (63 crates, lowest-covered first)
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)
|
|
🚅 Deployed to the ironclaw-pr-6055 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add integration coverage for same-run stale-surface refresh and extension-removal channel cleanup without changing production behavior.
Stats: 2 findings (from 2 raw, 2 after dedup) across 2 files. Reviewers run: security, performance, tests. Reviewer dispatch unavailable: bugs, conventions, local-patterns, maintainability, approach (agent-thread capacity). Body-only: 0.
Tests
-
Medium Facade slot failure paths are untested (
crates/ironclaw_reborn_composition/src/factory.rs:542-552, confidence 90) — anchor:crates/ironclaw_reborn_composition/src/factory.rs:542.The new shared setter returns
falsewhen no local runtime exists and when theOnceLockis already occupied, but current tests only assert the first successful set. These branches protect production/test wiring and idempotence semantics. -
Medium Keep the single-thread scenario out of the group test (
tests/integration/group_extensions/main.rs:27, confidence 100) — anchor:tests/integration/CLAUDE.md:411-412.same_run_activation_refresh_dispatchessubmits and asserts one thread only, while the integration-test guide requires one-thread scenarios to be flat tests; group tests are reserved for shared state or shared-runtime behavior.
| Arc::clone(&self.secret_store) | ||
| } | ||
|
|
||
| /// Fill the deferred per-caller channel-connection facade slot |
There was a problem hiding this comment.
Medium — Facade slot failure paths are untested.
The new shared setter returns false when no local runtime exists and when the OnceLock is already occupied, but current tests only assert the first successful set. These branches protect production/test wiring and idempotence semantics.
Fix: Add a focused factory test covering no-local-runtime and already-filled-slot cases.
There was a problem hiding this comment.
Fixed in 62bc6c8 — added fill_channel_connection_facade_slot_rejects_second_fill_and_keeps_first (second fill → false, first facade's Arc pointer identity preserved) and fill_channel_connection_facade_slot_without_local_runtime_returns_false (via RebornServices::disabled(), fail-closed).
| ); | ||
|
|
||
| // Scenario 6 (harness-port-seam P2, A1): install -> activate -> dispatch | ||
| // within ONE run, proving the RefreshingLocalDevCapabilityPort surface |
There was a problem hiding this comment.
Medium — Keep the single-thread scenario out of the group test.
The new same_run_activation_refresh_dispatches scenario submits and asserts one thread only, but the integration-test guide requires scenarios that submit and assert in one thread to be flat tests, reserving groups for shared state or shared-runtime behavior.
Fix: Move this scenario to its own flat integration test binary, or make it exercise shared group state/runtime behavior.
There was a problem hiding this comment.
Fixed in 62bc6c8 — scenario moved out of the group into the flat tests/integration/extension_visibility.rs binary per the guide rule (one-thread submit+assert = flat test). Discriminating property preserved (google-calendar inactive at start, absent from initial surface); mutation re-verified after the move: forcing capability_may_change_visible_surface to false fails exactly this test.
…ation, slot failure-path pins - recording_channel_connection_facade.rs: drop redundant inner Arc<Mutex<..>> (the facade is already shared via the outer Arc callers hold). - scenario_remove_channel_extension_disconnects_channel.rs: build an `expected` vec and compare/format against it instead of re-cloning expected_user into the message. - Move the single-thread same-run-activation-refresh scenario out of group_extensions into a flat #[tokio::test] in extension_visibility.rs, per tests/integration/CLAUDE.md; delete the scenario file + its group_extensions/main.rs registration. - factory.rs: add two focused tests pinning fill_channel_connection_facade_slot's failure paths — second fill on an occupied slot returns false and keeps the first facade, and no local runtime returns false rather than panicking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
size: M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@tests/integration/extension_visibility.rs`:
- Around line 76-126: Reduce the body of
same_run_activation_refresh_dispatches_newly_activated_capability by extracting
the scripted replies and credential seeding into a focused helper, while
preserving the existing lifecycle setup and assertions. Keep the test’s main
flow in the documented build → submit_turn → assert shape, with the helper
returning or preparing the configured harness before submission.
🪄 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: 18a022a5-b3cb-4fce-b037-cf9f8f595184
📒 Files selected for processing (5)
crates/ironclaw_reborn_composition/src/factory.rstests/integration/extension_visibility.rstests/integration/group_extensions/main.rstests/integration/group_extensions/scenario_remove_channel_extension_disconnects_channel.rstests/integration/support/doubles/recording_channel_connection_facade.rs
| #[tokio::test] | ||
| async fn same_run_activation_refresh_dispatches_newly_activated_capability() { | ||
| let group = RebornIntegrationGroup::extension_lifecycle() | ||
| .await | ||
| .expect("extension-lifecycle group builds"); | ||
| let h = group | ||
| .thread("ext-same-run-activation-refresh") | ||
| .script([ | ||
| RebornScriptedReply::tool_call( | ||
| "builtin.extension_install", | ||
| json!({"extension_id": "google-calendar"}), | ||
| ), | ||
| RebornScriptedReply::tool_call( | ||
| "builtin.extension_activate", | ||
| json!({"extension_id": "google-calendar"}), | ||
| ), | ||
| RebornScriptedReply::tool_call("google-calendar.list_calendars", json!({})), | ||
| RebornScriptedReply::text("calendars listed"), | ||
| ]) | ||
| .build() | ||
| .await | ||
| .expect("thread builds"); | ||
| // Credential material must exist BEFORE submit_turn — activation's | ||
| // credential gate resolves inline only if an account is already seeded, | ||
| // else the run would park at BlockedAuth instead of completing this turn. | ||
| h.seed_capability_credential_account( | ||
| "google", | ||
| "itest google calendar", | ||
| &[GOOGLE_CALENDAR_READONLY_SCOPE, GOOGLE_CALENDAR_EVENTS_SCOPE], | ||
| ) | ||
| .await | ||
| .expect("credential account seeds"); | ||
|
|
||
| h.submit_turn("install, activate, and list my calendars") | ||
| .await | ||
| .expect("turn completes"); | ||
| h.assert_tool_result_contains("\"installed\":true") | ||
| .await | ||
| .expect("install reported success"); | ||
| h.assert_tool_result_contains("\"activated\":true") | ||
| .await | ||
| .expect("activate reported success"); | ||
| // The discriminating proof: dispatch reached the capability port in the | ||
| // SAME run that activated it, i.e. the surface refresh fired. | ||
| h.assert_tool_invoked("google-calendar.list_calendars") | ||
| .await | ||
| .expect("newly-activated capability dispatched within the same run"); | ||
| h.assert_reply_contains("calendars listed") | ||
| .await | ||
| .expect("run completed with the expected reply"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Test body far exceeds the flat-integration-test size guidance.
The function body runs ~48 lines against the documented "keep the body small and readable (about 3–12 lines)" convention for tests/integration/*.rs. The multi-step lifecycle chain (install → activate → seed credential → dispatch → 4 assertions) plausibly justifies more than 12 lines, but consider extracting the script/credential setup into a small helper to bring the submit_turn/assert portion closer to the documented shape.
As per path instructions, "Write Reborn integration tests as flat tests/integration/<name>.rs binaries that follow the build → submit_turn → assert shape, use RebornIntegrationHarness, and keep the body small and readable (about 3–12 lines)."
🤖 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/integration/extension_visibility.rs` around lines 76 - 126, Reduce the
body of same_run_activation_refresh_dispatches_newly_activated_capability by
extracting the scripted replies and credential seeding into a focused helper,
while preserving the existing lifecycle setup and assertions. Keep the test’s
main flow in the documented build → submit_turn → assert shape, with the helper
returning or preparing the configured harness before submission.
What this adds
Integration-tier coverage only — no production behavior change. Follow-up to the harness port-seam program (#5950, #6026): now that the integration harness consumes production's LocalDev capability-port factory, these tests pin two previously-uncovered production paths through it.
1. StaleSurface same-run refresh (new scenario 6 in
reborn_group_extensions)One run performs
builtin.extension_install→builtin.extension_activate→google-calendar.list_calendars. Pins the production refresh mechanism end-to-end:SurfaceTrackingLoopCapabilityPort::capability_may_change_visible_surfaceclears the cached surface on activate, and the next loop iteration'svisible_capabilitiesrebuild picks up the newly-activated extension — the capability dispatches in the same run instead of failing as stale/unknown.Mutation-verified: forcing
capability_may_change_visible_surfacetofalsefails exactly this scenario (capability "google-calendar.list_calendars" was not invoked).2. Extension-remove channel cleanup (extended scenario 7 in
reborn_group_extensions)#5851 unified channel cleanup on remove (
extension_lifecycle.rs::remove→cleanup_channel_before_remove→disconnect_channel_for_cleanup) but shipped with zero default-CI coverage (its positive crate test isslack-v2-host-beta-gated). This backfills at the integration tier, both directions in one scenario:slack(RemovableChannelCleanup::Required) disconnects the channel exactly once.google-drive(credentialed, non-channel →IfConnectionFacadeSupportsChannel, no connection-map key) fires zero disconnects.Final assertion is exact equality
disconnects == [(user, "slack")], so both regressions are discriminating: neutralizingcleanup_channel_before_remove(reproducing #5851's original bug) drops the slack tuple; forcingshould_disconnect = trueunconditionally adds a google-drive tuple. Both mutations were run and failed this scenario only.Support changes
tests/integration/support/doubles/recording_channel_connection_facade.rs: recordingChannelConnectionFacadedouble (salvaged from the parked ext-remove worktree; its production changes were superseded by Unify Slack cleanup for extension remove tool #5851 and are NOT included).RebornServices::set_channel_connection_facade_for_test(#[cfg(feature = "test-support")]): fills the existingchannel_connection_facade_slotOnceLockthat production already carries — zero new production logic. Sharedpub(crate)recipefill_channel_connection_facade_slotbacks both this accessor and the productionRebornRuntime::set_channel_connection_facadeso the two can't drift.Investigated and deliberately not covered here
openai_compat_serve.rs), unreachable viasubmit_turn; production chat wires an emptyInMemoryExternalToolCatalogby design. Full loop is covered at crate tier where the surface actually lives. Adding harness injection would be test-only wiring for a path production chat never runs.CapabilitySurfaceProfileResolver: production LocalDev resolver is a privateAllowAllCapabilitySurfaceResolverwith a single branchless behavior the existing harness double reproduces exactly; swapping it in requires new public surface for zero behavioral difference.ExternalChanneldisconnect gap) remains open, out of scope.Verification
cargo test --test reborn_group_extensions: 13/13 (scenarios 1–7 insideextensions_group_e2e)reborn_integration_surface_disclosure17/17,reborn_integration_tool_call23/23,reborn_integration_wiring_parity17/17cargo clippy --all --benches --tests --examples --all-features: zero warningscargo test -p ironclaw_reborn_composition --features test-support --lib: 1137/1137test-support/slack-v2-host-beta/both — warning-free🤖 Generated with Claude Code