Repository navigation
refactor(slack,auth): delete the Slack connection-epoch slot — the auth-flow record owns attempt liveness - #6169
Merged
Merged
Conversation
Supersede-on-start (RFC 9700 4.7.1) previously lived at the start seams: start_setup_oauth_flow, start_dcr_setup_oauth_flow, and the Slack start handler each had to remember to call cancel_superseded_setup_flows before minting a flow. #6130 found the gap that pattern invites (the DCR/Notion route had omitted it) and patched it per-route. Move the supersede inside AuthFlowManager::create_flow itself (durable store + in-memory fake), keyed off is_setup_class_continuation on the request the flow already carries: creating a setup-class flow now structurally cancels the prior live setup-class flows for the same owner root + provider before the new flow becomes visible, so no start route can forget it. TurnGateResume/ProductActionResume creations are excluded in both directions - a parked turn or action is never superseded by, and never supersedes, the setup surface. The seam-level loops are deleted. Their only residual duty was eagerly deleting superseded flows' setup PKCE verifiers; those verifiers are stored with TTL = the dead flow's own expires_at, so the store's TTL backstop reclaims them (cleanup_credentials_for_lifecycle keeps its eager deletes - the cleanup report still names canceled flows). New tests (watched red first): the fake-tier contract pair create_flow_supersedes_prior_live_setup_class_flows / create_flow_for_a_parked_turn_gate_does_not_supersede_setup_flows and the durable twin create_flow_supersedes_prior_live_setup_class_flows_in_the_durable_store. Two existing contract tests staged two live setup-class flows for the same owner+provider by driving create_flow below the route seam - a state the production routes already made unreachable; they are re-derived to their real invariants (supersede-walk ids/idempotence, and cleanup's lifecycle_package selector discriminating packages on a shared provider) under the new contract. Route-level behavior is unchanged: open_close_reopen_supersedes_prior_setup_flow_across_both_start_routes and the slack_personal_* serve suite stay green untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZEUvrn7Aj3HvnswPz99WN
…-flow record owns attempt liveness When a user connected Slack, two durable records described the same OAuth attempt: the provider-agnostic auth-flow record and a Slack-private connection epoch claimed at start time (Connecting/Active + pending_connection slot semantics in FilesystemSlackHostState). Nothing tied them together structurally - the start handler had to supersede the flow, remember to abandon its epoch, begin the new epoch, and unwind in order on each failure arm, and the two layers implemented opposite policies for a reopen (flow: new attempt wins; epoch: 409 connection_conflict until TTL). That seam produced the reopen-409 bug and the stranded-epoch bug that #6130 patched with begin_connection_reconciling_stranded_epoch. Delete the second liveness authority; keep the fence: - The OAuth start path no longer writes ANY Slack connection state. begin_connection, the ConnectionInProgress 409, the pending_connection slot, the supersede-then-abandon loop, the #6130 reconcile bridge, abandon_connection, connection_owner_for_epoch, and the gate provider's slot-driven select_reusable_flow/publish_flow/abandon_flow overrides (plus SlackPersonalOAuthGateLifecycle and its setup-slot wiring) are all gone. Slack's gate provider now runs on the shared driver's defaults, exactly like Google. - The connection record survives as a generation REGISTRY written by the callback's identity bind (record_active_generation): Active@epoch is what ingress authorization checks, and Disconnecting/Disconnected journal the fenced sweeps. bind_user_identity_for_epoch's old attempt-vs-attempt StaleEpoch fence (record must be Connecting at my epoch) is replaced by the one irreducible write fence: reject a bind only while a disconnect sweep is running, since an AllOwned sweep could otherwise delete the fresh rows. Reconnect-over-active replaces the generation at bind time; flow-driven rollback restores the previous generation. - The callback hooks derive the installation from the same SlackPersonalConnectionScopeResolver the start handler validates, instead of reading it back from a start-time record; the binding service's installation/proof checks make a mid-flight setup drift fail closed rather than bind under stale configuration. - The failed-connection-cleanup journal (begin/complete_failed_connection_cleanup), the disconnect journal (begin/complete_disconnect, SlackDisconnectFence, SlackConnectionCleanupSelector), the row-level generation stamp (SlackConnectionEpoch), and the ingress Active-at-generation check are kept and documented as what they are: a generation fence, not a liveness record. Legacy Connecting records (and their serde pending_connection field) deserialize fine and read as a stale, never-activated generation. Behavioral changes, called out explicitly: - Reopening/concurrent connect attempts never 409: the flow layer's supersede-on-start is the single policy (new attempt wins). AuthErrorCode::ConnectionConflict is now unproduced (variant kept for wire stability; follow-up may retire it). - A connect landing mid-disconnect starts fine and fails closed at the callback bind while the sweep runs (retryable), instead of 409ing at start (DisconnectInProgress is now the bind-fence rejection, not a start-time conflict). - Two blocked turn gates each own their flow instead of joining one caller-wide attempt through the slot; a settings-card start no longer 409s against a live gate flow (gate flows are never superseded). - A mid-flight Slack setup drift now fails the callback closed instead of binding under the start-time installation capture. New regression tests (watched red first): slack_personal_start_writes_no_connection_state (the strand is unrepresentable - replaces the deleted #6130 bridge test), filesystem_slack_host_state_callback_bind_creates_the_active_connection_record, filesystem_slack_host_state_reconnect_bind_replaces_the_active_generation, filesystem_slack_host_state_bind_is_fenced_while_disconnect_cleanup_runs, filesystem_slack_host_state_legacy_connecting_record_reads_as_stale_generation, slack_personal_callback_fails_closed_when_setup_drifts_mid_flight, failed_cleanup_journal_rejects_a_generation_the_record_moved_past. Slot-mechanism tests were re-derived to their surviving invariants or deleted where the premise (a pre-bind staged attempt) is now unrepresentable. The full connect->use->remove->reconnect->reinstall integration scenario (reborn_group_extensions, incl. #6092 Phase 6 reconnect-after-disconnect) passes unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZEUvrn7Aj3HvnswPz99WN
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 16, 2026 22:19
Destroyed
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (53)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds durable OAuth PKCE storage, setup-flow supersession, package-scoped cleanup, extension-store concurrency protection, bind-time Slack generations, expiry projection, reorganized integration tests, and a shared-provider extension auth-gate scenario. Extension lifecycle update APIs are replaced with registry upsert behavior. ChangesAuth and OAuth lifecycle
Extension persistence and lifecycle
Slack lifecycle
Integration coverage and wiring
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
github-actions
Bot
added
size: XL
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
…all journeys; group auth bins under tests/integration/auth/ Two additions at the in-process integration tier, then a layout pass so the auth and extension-lifecycle user journeys read as coherent folders instead of accreting flat files. New journey coverage (all green): - tests/integration/auth/oauth_popup_journeys.rs — the connect-popup walk over the real product-auth boundary: closed_popup_reopen_supersedes_abandoned_flow_then_completes (close the popup, click Connect again: creation supersedes the abandoned flow, the late callback from the old tab dies at claim as Canceled, exactly one account survives) and denied_consent_terminalizes_flow_and_fresh_retry_connects (Deny on the provider page: sanitized non-retryable ProviderDenied, durable Failed record, no exchange, immediate fresh Connect succeeds). The pre-existing expired-retry and replay-idempotency journeys move in beside them unchanged. - tests/integration/group_extensions/ scenario_google_family_install_gate_and_shared_account.rs (scenario 10) — the chat install-and-connect walk for OAuth packages sharing one provider: installing google-calendar parks activation at a renderable google gate even though a gmail-scoped google account already exists (scope coverage is enforced); installing google-drive parks its OWN independent gate and denying it leaves calendar's gate parked; denial leaves a clean retry path with no error-shaped tool result; and after one correctly-scoped google account exists, BOTH packages activate off it. Layout: the auth user-journey bins move to tests/integration/auth/ (oauth_connect, oauth_popup_journeys, oauth_refresh, auth_gate, auth_failure, reopen_resume_through_gate) with shared fixtures in auth/common.rs; every [[test]] binary NAME is unchanged, only Cargo manifest paths moved, so CI invocations are untouched. oauth_connect.rs is split so the happy-path/guard bin and the popup-journey bin each stay small. tests/integration/CLAUDE.md's layout note, one scenario doc-comment, and one path in docs/reborn/engine-v2-to-reborn-parity.md are updated to the new locations. Suites run: reborn_integration_oauth_connect (2), reborn_integration_oauth_popup_journeys (4), reborn_integration_auth_gate, reborn_integration_auth_failure, reborn_integration_reopen_resume_through_gate, reborn_integration_oauth_refresh (--features libsql), and reborn_group_extensions (10 scenarios incl. the new one) — all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZEUvrn7Aj3HvnswPz99WN
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 17, 2026 19:35
Destroyed
github-actions
Bot
added
scope: dependencies
… lease-store #6197, conformance suite) Two conflict resolutions, both at the #6130-vs-revert seam: - product_auth/serve/mod.rs: keep the durable-PKCE world (no ExpiringLruCache / StoredPkceVerifier route cache - #6130 replaced it with secret-store verifiers), while adopting #6194's pub visibility on ProductAuthRouteMount, which the consolidated ironclaw_webui crate now consumes cross-crate. - tests/integration/auth/oauth_popup_journeys.rs: rename detection mapped main's oauth_connect.rs additions onto the moved popup bin; resolved to our split layout and ported main's new durable_flow_manager_satisfies_shared_oauth_flow_conformance test into tests/integration/auth/oauth_connect.rs. The shared conformance suite creates flows strictly sequentially with every prior flow terminal, so it is compatible with the supersede-on-create_flow contract. Also guards node_modules/ via .gitignore: the webui frontend's package install materialized during local verification and must never be committed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZEUvrn7Aj3HvnswPz99WN
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 17, 2026 19:50
Destroyed
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.36% — 321776 / 372597 lines Per-crate breakdown (65 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-6169 environment in ironclaw-ci-preview
|
BenKurrek
marked this pull request as ready for review
July 17, 2026 20:28
Collaborator
Author
|
@coderabbitai full review |
Collaborator
Author
|
/gemini review |
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
1 similar comment
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
✅ Action performedFull review finished. |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | 0329b4ab577e |
Head: 0329b4ab577e8309776dc6eb285265d3cec1f866
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
Reviewed the exact 52-file diff across auth-flow creation, durable storage, Slack generation fencing, lifecycle cleanup, routes, and tests. Two blocking concurrency issues remain: setup-flow supersession is non-atomic, and Slack failure cleanup can target a different installation than the callback bound.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Serialize setup-flow supersession with creation
Location: crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs:53-55
The supersede scan and insertion are separate operations. Two concurrent starts for the same owner/provider can both scan before either insert, then each write a distinct AwaitingUser flow because CasExpectation::Absent only fences each flow-id path. Both callbacks can consequently claim and race credential/Slack binding writes, violating the new-wins contract. Make cancel-plus-insert atomic or serialize it on an owner-root/provider key across writers, and add a concurrent-start regression test.
2. ❌ [HIGH] Pin Slack failure cleanup to the installation actually bound
Location: crates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rs:268-279
The identity hook resolves the installation when it binds, but this terminal hook resolves mutable setup again. If setup is changed or removed after the identity bind and before a continuation failure, cleanup targets the new owner—or returns success on None—while the original identity row and active generation remain authorized despite the flow/account compensation failing. Carry the bound installation/owner into the cleanup journal or recover it by epoch, and test setup drift after binding but before continuation failure.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs (1)
1035-1039: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not let failed cleanup release a user disconnect fence.
A concurrent
begin_disconnectcan already own this matchingDisconnectingrecord. The failed-callback path then treats it as idempotently its own and changes it toDisconnected, allowing a reconnect beforedisconnect_channel_for_callerfinishes its lateAllOwnedsweep; that fresh binding can then be deleted.Persist/check the journal kind, or reject a disconnect-owned journal. Add a caller-level race regression covering disconnect, failed callback cleanup, reconnect, and the legacy owner-wide sweep.
As per coding guidelines, “Test through the caller when a helper gates persistence or side effects.”
Also applies to: 1074-1077
🤖 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 `@crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs` around lines 1035 - 1039, Update the failed-callback cleanup handling around the Disconnecting/Disconnected checks to distinguish its own journal from a disconnect-owned journal, rejecting or preserving the latter instead of marking it Disconnected. Ensure disconnect_channel_for_caller’s late AllOwned sweep cannot delete a binding created by a reconnect after concurrent begin_disconnect. Add a caller-level regression covering disconnect, failed callback cleanup, reconnect, and the legacy owner-wide sweep.Source: Coding guidelines
🤖 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_auth/src/fakes.rs`:
- Around line 184-190: The setup-flow supersession must be atomic so concurrent
starts cannot both remain live. In crates/ironclaw_auth/src/fakes.rs:184-190,
update AuthFlowManager::create_flow to hold the same state lock across
cancel_superseded_setup_flows and insertion; durable implementations must use an
equivalent atomic transaction or CAS. In
crates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rs:1202-1409,
add a concurrent-start regression test asserting exactly one setup flow remains
live.
In `@crates/ironclaw_product_workflow/src/auth_interaction/service.rs`:
- Around line 309-316: Move the chrono::Utc::now() snapshot in the auth
interaction flow to after the awaited read_model.auth_gates(&scope) call and
before projecting gates with to_view(now), so expiry is evaluated against the
post-read time. Add a caller-level regression test using a pausing read model
that crosses the expiry boundary and verifies the returned interaction is
expired or excluded as expected.
In
`@crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs`:
- Around line 130-194: Update rollback handling in
extension_installation_store.rs, including restore_installation_row,
restore_manifest_row, remove_inserted_installation_row, and
remove_inserted_manifest_row, so a rollback failure either rebuilds inner from
retained snapshot bytes or permanently rejects subsequent operations until
reload instead of only logging and continuing; add coverage in
crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store/tests.rs:748-803
that injects rollback failure and verifies later reads/writes fail closed or use
a rebuilt snapshot.
- Around line 95-126: Replace the TOCTOU read/check/write flow in
ensure_snapshot_current and save_snapshot with the shared bounded
expected-version CAS path, without holding process-local mutexes across backend
I/O; apply the same CAS handling to the canonicalization rewrite at
crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs:53-54.
Update the writer-concurrency tests at
crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store/tests.rs:203-256
to synchronize both writers after baseline reads, then assert exactly one
conflict and that the successful writer’s state is not clobbered.
In `@crates/ironclaw_reborn_composition/src/product_auth/api/auth_dcr_tests.rs`:
- Around line 194-198: Update the documentation comment covering DCR start-route
superseding to remove the incorrect RFC 9700 §4.7.1 attribution and cite the
internal AuthFlowManager::create_flow contract instead. Keep the existing
description of preventing concurrent authorization requests and ensure the
comment reflects the behavior covered by the implementation and tests.
- Around line 200-275: The test dcr_setup_restart_supersedes_prior_flow
currently bypasses the real route and cannot verify scope derivation or secret
cleanup. Replace direct start_dcr_setup_oauth_flow calls with two requests
through extension_oauth_start_handler, using the route’s required
installation/context setup, then assert the first flow is canceled and its
PKCE/DCR secret handles are removed from the configured secret store.
In `@crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rs`:
- Around line 230-233: Update the OAuth callback flow around
ensure_oauth_callback_flow_known and verifier lookup to classify backend
failures as retryable, preserving the durable PKCE verifier and avoiding flow
terminalization until a durable terminal outcome is established. Make
delete_setup_pkce_verifier failures observable as contextual cleanup errors, and
delete the verifier only after terminal success or failure. Add a fail-once
backend/secret-store regression test covering retry behavior and state
preservation.
In `@crates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rs`:
- Around line 268-294: Update the Slack terminal cleanup flow to use the
callback-time installation recorded during bind rather than resolving the
current setup via resolve_personal_connection_scope. Ensure setup removal or
drift does not return success without cleanup or construct an incorrect
SlackConnectionOwner/prefix, and fail closed when the callback identity or
binding cannot be determined. Add a regression test through the terminal failure
caller covering setup drift after bind and verifying the original generation is
cleaned.
- Around line 173-178: Update the connection-scope resolution chain in the
personal OAuth flow to capture the error in map_err, log the bound backend error
at debug! before converting it to
ProductAuthRouteFailure::backend_unavailable(), and preserve the existing
sanitized route failure returned to callers.
In `@crates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rs`:
- Around line 2480-2496: Update the restart scenario in the Google OAuth
durability test around RebornProductAuthServices::from_shared and
build_app_with_product_auth_service_and_config: recreate
RebornProductAuthServices after the initial flow using the same
filesystem-backed flow store and durable SecretStore, rather than reusing
product_auth. Build the restarted router with the recreated service and run the
callback through that router, preserving the existing persisted-state
assertions.
In `@tests/integration/auth/oauth_popup_journeys.rs`:
- Around line 341-370: Update the abandoned and reopened flow setup in the test
to invoke the production Connect seam,
RebornProductAuthServices::start_setup_oauth_flow (or its WebUI caller), instead
of calling flow_manager().create_flow directly. Ensure both attempts exercise
the shared setup behavior, including fail-closed PKCE persistence, while
preserving the existing durable cancellation and late-callback assertions.
In
`@tests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs`:
- Around line 78-89: Update the calendar scope assertion in the parked
requirement validation to require exact presence of both
GOOGLE_CALENDAR_READONLY_SCOPE and GOOGLE_CALENDAR_EVENTS_SCOPE in
provider_scopes. Replace the broad contains("calendar") check while preserving
the existing error reporting for missing required scopes.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs`:
- Around line 1035-1039: Update the failed-callback cleanup handling around the
Disconnecting/Disconnected checks to distinguish its own journal from a
disconnect-owned journal, rejecting or preserving the latter instead of marking
it Disconnected. Ensure disconnect_channel_for_caller’s late AllOwned sweep
cannot delete a binding created by a reconnect after concurrent
begin_disconnect. Add a caller-level regression covering disconnect, failed
callback cleanup, reconnect, and the legacy owner-wide sweep.
🪄 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: 6b7c5138-d768-46a5-99e6-ca3670bf02e4
📒 Files selected for processing (52)
.gitignoreCargo.tomlcrates/ironclaw_auth/src/cleanup.rscrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/src/flow.rscrates/ironclaw_auth/src/lib.rscrates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rscrates/ironclaw_auth/tests/auth_product_contract/refresh_contract.rscrates/ironclaw_event_projections/tests/extension_lifecycle_projection_contract.rscrates/ironclaw_extensions/src/installations.rscrates/ironclaw_extensions/src/lifecycle.rscrates/ironclaw_extensions/src/registry.rscrates/ironclaw_extensions/tests/extension_contract.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_product_workflow/src/auth_interaction/types.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rscrates/ironclaw_reborn_composition/src/extension_host/extension_installation_store/tests.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth_dcr_tests.rscrates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rscrates/ironclaw_reborn_composition/src/product_auth/durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth/durable/mod.rscrates/ironclaw_reborn_composition/src/product_auth/durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth/serve/lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth/serve/mod.rscrates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_channel_connection.rscrates/ironclaw_reborn_composition/src/slack/slack_host_beta.rscrates/ironclaw_reborn_composition/src/slack/slack_host_state.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_binding.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_setup.rscrates/ironclaw_reborn_composition/src/test_support/slack_channel_connection.rscrates/ironclaw_reborn_composition/tests/auth_lifecycle.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rscrates/ironclaw_webui/src/auth/pending.rsdocs/reborn/engine-v2-to-reborn-parity.mdtests/integration/CLAUDE.mdtests/integration/auth/auth_failure.rstests/integration/auth/auth_gate.rstests/integration/auth/common.rstests/integration/auth/oauth_connect.rstests/integration/auth/oauth_popup_journeys.rstests/integration/auth/oauth_refresh.rstests/integration/auth/reopen_resume_through_gate.rstests/integration/group_extensions/main.rstests/integration/group_extensions/scenario_extension_activation_reauth_gate.rstests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs
💤 Files with no reviewable changes (3)
- crates/ironclaw_reborn_composition/src/slack/slack_setup.rs
- crates/ironclaw_reborn_composition/src/slack/slack_host_beta.rs
- crates/ironclaw_extensions/src/lifecycle.rs
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/product_auth/api/auth.rs`:
- Around line 1648-1654: Replace the incorrect RFC 9700 §4.7.1 attribution in
the comments near the setup-class flow and the corresponding block near the
later authorization flow. Reference the AuthFlowManager::create_flow
supersede-on-start contract, matching the wording or attribution already
established in flows.rs, while preserving the existing explanation of
cancellation and verifier cleanup.
In `@crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs`:
- Around line 1180-1192: Persist the predecessor connection and identity
snapshots, currently held only by the rollback future as activation.previous and
previous_identity, in the durable reconfiguration state created through
StoredSlackConnection::new. Ensure terminal reconciliation can reconstruct and
restore generation A after interruption instead of only disconnecting generation
B, covering the related flows at the other affected sites. Add a callback-level
restart regression that exercises failed reconfiguration through its caller and
verifies compensation side effects.
In `@crates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rs`:
- Around line 281-287: Change the logging level in the Err branch of the Slack
terminal cleanup scope-resolution flow from tracing::warn! to tracing::debug!,
preserving the existing error, flow_id, message, and ProductAuthRouteFailure
return behavior.
In `@tests/integration/CLAUDE.md`:
- Around line 151-161: Update the integration test layout documentation around
the flat-bin description to state that bins may live directly under
tests/integration/ or within domain folders, with Cargo.toml paths reflecting
their locations. Revise the following module-path guidance to distinguish path
attributes for flat bins versus bins nested one or more levels deep, while
preserving the existing auth and support-tree examples.
In
`@tests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs`:
- Around line 110-115: Strengthen the assertions after the drive call to inspect
the blocked state represented by drive_gate, requiring provider "google" and the
exact scopes GOOGLE_DRIVE_READONLY_SCOPE and GOOGLE_DRIVE_SCOPE. Keep the
existing drive_run versus calendar_run independence check, and fail the test
when the gate’s provider or scopes differ.
🪄 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: 178bf765-889b-410b-aae6-a87393e827fd
📒 Files selected for processing (52)
.gitignoreCargo.tomlcrates/ironclaw_auth/src/cleanup.rscrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/src/flow.rscrates/ironclaw_auth/src/lib.rscrates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rscrates/ironclaw_auth/tests/auth_product_contract/refresh_contract.rscrates/ironclaw_event_projections/tests/extension_lifecycle_projection_contract.rscrates/ironclaw_extensions/src/installations.rscrates/ironclaw_extensions/src/lifecycle.rscrates/ironclaw_extensions/src/registry.rscrates/ironclaw_extensions/tests/extension_contract.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_product_workflow/src/auth_interaction/types.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rscrates/ironclaw_reborn_composition/src/extension_host/extension_installation_store/tests.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth_dcr_tests.rscrates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rscrates/ironclaw_reborn_composition/src/product_auth/durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth/durable/mod.rscrates/ironclaw_reborn_composition/src/product_auth/durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth/serve/lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth/serve/mod.rscrates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_channel_connection.rscrates/ironclaw_reborn_composition/src/slack/slack_host_beta.rscrates/ironclaw_reborn_composition/src/slack/slack_host_state.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_binding.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_setup.rscrates/ironclaw_reborn_composition/src/test_support/slack_channel_connection.rscrates/ironclaw_reborn_composition/tests/auth_lifecycle.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rscrates/ironclaw_webui/src/auth/pending.rsdocs/reborn/engine-v2-to-reborn-parity.mdtests/integration/CLAUDE.mdtests/integration/auth/auth_failure.rstests/integration/auth/auth_gate.rstests/integration/auth/common.rstests/integration/auth/oauth_connect.rstests/integration/auth/oauth_popup_journeys.rstests/integration/auth/oauth_refresh.rstests/integration/auth/reopen_resume_through_gate.rstests/integration/group_extensions/main.rstests/integration/group_extensions/scenario_extension_activation_reauth_gate.rstests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs
💤 Files with no reviewable changes (3)
- crates/ironclaw_extensions/src/lifecycle.rs
- crates/ironclaw_reborn_composition/src/slack/slack_host_beta.rs
- crates/ironclaw_reborn_composition/src/slack/slack_setup.rs
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
crates/ironclaw_product_workflow/src/auth_interaction/service.rs (1)
309-314: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCapture the expiry snapshot after the awaited read.
Line 309 samples time before
auth_gates().await; a slow read can therefore return an interaction already expired when the response is projected.Proposed fix
- let now = chrono::Utc::now(); - let mut auth = self + let gates = self .read_model .auth_gates(&scope) - .await? + .await?; + let now = chrono::Utc::now(); + let mut auth = gates .into_iter()🤖 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 `@crates/ironclaw_product_workflow/src/auth_interaction/service.rs` around lines 309 - 314, Move the `chrono::Utc::now()` assignment in the authentication interaction flow to after the awaited `self.read_model.auth_gates(&scope)` call completes, so expiry comparisons use a post-read timestamp. Keep the existing auth-gate projection and iteration behavior unchanged.
🤖 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_auth/src/flow.rs`:
- Around line 295-304: Update the documentation for the durable auth flow
creation contract and the cancel_superseded_setup_flows function to remove the
incorrect RFC 9700 §4.7.1 citation and describe supersession using the local
contract or implementation invariant instead. Preserve the existing behavior and
distinctions between setup-class and turn/action resume flows.
In `@crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs`:
- Around line 2496-2509: Add a raw JSON legacy-shape fixture to
filesystem_slack_host_state_legacy_connecting_record_reads_as_stale_generation,
including the retired pending_connection field and connecting state, and
deserialize it with serde_json::from_str instead of constructing only
StoredSlackConnection::new(...). Assert it still reads as a stale,
never-activated generation.
In `@crates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rs`:
- Around line 582-604: Update live_flows_for_provider and flow_status_by_id to
accept AuthProviderId and AuthFlowId instead of &str, and compare those typed
identifiers directly without converting flow.id to a string. Parse the router
JSON identifier once into AuthFlowId before calling these helpers, using
AuthProviderId for provider values throughout the affected test flow.
- Around line 2391-2402: Update durable create_flow so superseding prior flows
and inserting the new flow occur atomically through the shared CAS/transaction
path, with bounded retry handling for conflicts. Add a barrier-driven
caller-level regression test around the simultaneous start_oauth_flow setup to
verify concurrent starts for the same owner and provider leave only one live
flow.
In
`@tests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs`:
- Around line 132-138: After calendar.deny_auth_gate and the subsequent
wait_for_status(calendar_run, TurnStatus::Completed), add an assertion against
the calendar run’s resulting tool output to verify it remains clean and is not
error-shaped. Reuse the existing result-validation helper or assertion pattern
in this scenario, anchoring the change to the Phase 3 flow and preserving the
current completion wait.
---
Duplicate comments:
In `@crates/ironclaw_product_workflow/src/auth_interaction/service.rs`:
- Around line 309-314: Move the `chrono::Utc::now()` assignment in the
authentication interaction flow to after the awaited
`self.read_model.auth_gates(&scope)` call completes, so expiry comparisons use a
post-read timestamp. Keep the existing auth-gate projection and iteration
behavior 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: 178bf765-889b-410b-aae6-a87393e827fd
📒 Files selected for processing (52)
.gitignoreCargo.tomlcrates/ironclaw_auth/src/cleanup.rscrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/src/flow.rscrates/ironclaw_auth/src/lib.rscrates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rscrates/ironclaw_auth/tests/auth_product_contract/refresh_contract.rscrates/ironclaw_event_projections/tests/extension_lifecycle_projection_contract.rscrates/ironclaw_extensions/src/installations.rscrates/ironclaw_extensions/src/lifecycle.rscrates/ironclaw_extensions/src/registry.rscrates/ironclaw_extensions/tests/extension_contract.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_product_workflow/src/auth_interaction/types.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rscrates/ironclaw_reborn_composition/src/extension_host/extension_installation_store/tests.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth_dcr_tests.rscrates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rscrates/ironclaw_reborn_composition/src/product_auth/durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth/durable/mod.rscrates/ironclaw_reborn_composition/src/product_auth/durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth/serve/lifecycle.rscrates/ironclaw_reborn_composition/src/product_auth/serve/mod.rscrates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_channel_connection.rscrates/ironclaw_reborn_composition/src/slack/slack_host_beta.rscrates/ironclaw_reborn_composition/src/slack/slack_host_state.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_binding.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_setup.rscrates/ironclaw_reborn_composition/src/test_support/slack_channel_connection.rscrates/ironclaw_reborn_composition/tests/auth_lifecycle.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rscrates/ironclaw_webui/src/auth/pending.rsdocs/reborn/engine-v2-to-reborn-parity.mdtests/integration/CLAUDE.mdtests/integration/auth/auth_failure.rstests/integration/auth/auth_gate.rstests/integration/auth/common.rstests/integration/auth/oauth_connect.rstests/integration/auth/oauth_popup_journeys.rstests/integration/auth/oauth_refresh.rstests/integration/auth/reopen_resume_through_gate.rstests/integration/group_extensions/main.rstests/integration/group_extensions/scenario_extension_activation_reauth_gate.rstests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs
💤 Files with no reviewable changes (3)
- crates/ironclaw_extensions/src/lifecycle.rs
- crates/ironclaw_reborn_composition/src/slack/slack_host_beta.rs
- crates/ironclaw_reborn_composition/src/slack/slack_setup.rs
…d terminal cleanup Review follow-ups on #6169, each with a regression test that failed first: 1. create_flow's supersede+insert is now one critical section per owner root. Two racing setup creates could each run the supersede walk before either insert was visible and both survive as live flows, breaking the "<=1 live setup-class flow" contract at its own seam. Fake: the walk moved inside the single lock_state() acquisition (shared supersede_setup_flows_locked). Durable: a per-owner-root lock_for("flow-root:...") held across walk+validate+write; ordering is root -> per-flow, nothing takes the reverse. Pinned by concurrent_setup_creates_leave_exactly_one_live_flow (contract suite, 8 racers x 20 rounds) and its durable twin (6 racers x 10 rounds) — both raced to 2+ live flows before the fix. 2. Slack terminal-failure cleanup derives its owner from the stamped rows, not a fresh resolution. The hook re-resolved the connection scope at cleanup time; an operator repointing Slack setup between the bind and the hook made cleanup target an installation this generation never touched, orphaning the real row until a full owner disconnect. Rows carrying the failed generation now name their own installation (parse_slack_user_identity_provider_user_id) and are reclaimed through the journaled sweep whatever the reported failure stage — a post-bind completion failure classifies as Terminal through the blanket error conversion, and rows-at-epoch is the proof a bind committed. The resolver is consulted only when no rows survive, to settle a possibly still-active generation record. Pinned by slack_personal_terminal_cleanup_targets_the_bound_installation_after_setup_drift (queued resolver drifts between bind and hook; rollback delete scripted to fail; the alpha-stamped row survived before the fix). slack_personal_oauth_failed_identity_rollback_allows_disconnect_then_reconnect pinned the pre-fix residue ("row waits for explicit disconnect") and is re-derived to the surviving invariants as ..._failed_identity_rollback_is_reclaimed_then_reconnects: hook reclaims immediately, ingress never active, disconnect still converges, reconnect starts clean. 3. The Slack start handler's resolver failure now logs the cause at debug before mapping to backend_unavailable (was silently dropped). 4. Scenario 10's parked-requirement assertion requires the exact calendar scopes (GOOGLE_CALENDAR_READONLY_SCOPE + GOOGLE_CALENDAR_EVENTS_SCOPE), not scope.contains("calendar"). 5. RFC 9700 §4.7.1 citations (6 sites) replaced with the real authority: AuthFlowManager::create_flow's supersede contract. The section number did not say what we cited it for. 6. list_pending captures `now` after the awaited auth_gates() read so a slow read cannot render gates unexpired that expired mid-await. Suites: ironclaw_auth (117), composition slack-v2-host-beta lib product_auth+slack (607), default-feature lib (1582), webui_v2_product_auth (46), auth_lifecycle (7), reborn_group_extensions (13), six reborn_integration_* auth binaries incl. --features libsql refresh, ironclaw_architecture (42), ironclaw_product_workflow (222+28), clippy -D warnings on all three touched crates, pre-commit safety gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZEUvrn7Aj3HvnswPz99WN
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 17, 2026 21:31
Destroyed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rs (1)
243-262: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the bind-time installation recoverable until lifecycle settlement succeeds.
If row deletion succeeds at Lines 426-434 but lifecycle settlement fails at Lines 443-453, the retry finds no stamped rows. It then either returns success for
Terminalor resolves mutable current setup, leaving the original lifecycle epoch unsettled.Persist/recover the installation from the cleanup journal until settlement completes, and add a callback-level regression covering settlement failure followed by setup removal or drift.
As per coding guidelines, persisted state must remain reconstructible after interruption and partial failures must be tested through the public seam.
Also applies to: 320-453
🤖 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 `@crates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rs` around lines 243 - 262, Update the Slack personal OAuth cleanup flow, including reconcile_failed_lifecycle_oauth and the journaled failed-connection sweep, to persist the bind-time installation until lifecycle settlement succeeds. On retries after row deletion or interruption, reconstruct and reuse that journaled installation instead of returning success for Terminal or resolving mutable current setup; remove the persisted state only after settlement completes. Add a callback-level regression covering settlement failure followed by setup removal or configuration drift.Source: Coding guidelines
🤖 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 `@crates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rs`:
- Around line 243-262: Update the Slack personal OAuth cleanup flow, including
reconcile_failed_lifecycle_oauth and the journaled failed-connection sweep, to
persist the bind-time installation until lifecycle settlement succeeds. On
retries after row deletion or interruption, reconstruct and reuse that journaled
installation instead of returning success for Terminal or resolving mutable
current setup; remove the persisted state only after settlement completes. Add a
callback-level regression covering settlement failure followed by setup removal
or configuration drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 859cd232-dc1b-480a-b4a6-73cfcdf65c6b
📒 Files selected for processing (13)
crates/ironclaw_auth/Cargo.tomlcrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/src/flow.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth.rscrates/ironclaw_reborn_composition/src/product_auth/api/auth_dcr_tests.rscrates/ironclaw_reborn_composition/src/product_auth/durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth/durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rstests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs
…te scopes, log level Follow-ups from CodeRabbit's second pass on #6169: - The legacy-record test now writes the RAW pre-deletion JSON (including the retired pending_connection object no current struct can express) instead of seeding through StoredSlackConnection::new, so it pins serde's unknown-field tolerance — the actual backward-compat claim in the doc comment — not just the connecting-state mapping. - Scenario 10 Phase 2 reads the drive gate's blocked state and requires a google requirement carrying both exact drive scopes (GOOGLE_DRIVE_READONLY_SCOPE + GOOGLE_DRIVE_SCOPE), mirroring the calendar-side assertion; wait_for_status alone proved only that some gate opened. - Phase 3 documents why there is deliberately NO post-denial no-error-shape assertion: running the suggested check showed an explicit user denial legitimately lands as an error-status observation ("auth gate denied by user") — the #5878 rule bans errors standing in for gates, not denial markers. - The terminal-cleanup resolver-failure diagnostic drops warn! for debug! per the logging rule (internal backend state, not user-facing status). - tests/integration/CLAUDE.md no longer promises a flat-only bin layout; flat and domain-folder bins are both described, matching the registered Cargo.toml paths. Suites: composition slack-v2-host-beta lib product_auth+slack (607), reborn_group_extensions (13), clippy -D warnings clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZEUvrn7Aj3HvnswPz99WN
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 17, 2026 21:43
Destroyed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs (1)
1569-1575: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate or explicitly comment the dropped database error.
The
write_recordresult is silently dropped withlet _ =. As per coding guidelines, DB operation fallbacks must either fail loudly with?(propagating the error) or include an inline// silent-ok: <reason>comment if the failure is genuinely safe to ignore.Since this is inside a rollback flow and you successfully matched
VersionMismatchjust above (lines 1509-1510), you should handle it symmetrically.🛠️ Proposed fix to handle the fallback explicitly
- let _ = self - .write_record( - &identity_path, - &identity_target.tombstone(), - CasExpectation::Version(identity_version), - ) - .await; + match self + .write_record( + &identity_path, + &identity_target.tombstone(), + CasExpectation::Version(identity_version), + ) + .await + { + Ok(_) | Err(FilesystemError::VersionMismatch { .. }) => {} + Err(error) => return Err(map_binding_fs_error(error)), + }🤖 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 `@crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs` around lines 1569 - 1575, Update the rollback flow containing write_record to handle its database error explicitly: propagate the result with ? in the VersionMismatch recovery path, matching the successful VersionMismatch handling above. If the error is intentionally safe to ignore instead, retain the discard only with an inline // silent-ok: <reason> comment explaining why.Source: Coding guidelines
🤖 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 `@crates/ironclaw_reborn_composition/src/slack/slack_host_state.rs`:
- Around line 1569-1575: Update the rollback flow containing write_record to
handle its database error explicitly: propagate the result with ? in the
VersionMismatch recovery path, matching the successful VersionMismatch handling
above. If the error is intentionally safe to ignore instead, retain the discard
only with an inline // silent-ok: <reason> comment explaining why.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e7c6a0a-4848-488a-824c-065d662232a2
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/slack/slack_host_state.rscrates/ironclaw_reborn_composition/src/slack/slack_personal_oauth.rstests/integration/CLAUDE.mdtests/integration/group_extensions/scenario_google_family_install_gate_and_shared_account.rs
Resolve the Slack OAuth test and storage API conflicts, reconcile the Google extension scenario with current provider readiness, and allow expired auth continuations to be acknowledged during extension removal.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 20, 2026 17:08
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_auth/src/fakes.rs (1)
1369-1395: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake turn-gate cleanup idempotent before acknowledgment. Re-running uninstall on the same flow before
mark_continuation_dispatchedwill match the same terminalTurnGateResumeagain and enqueue a freshAuthContinuationEventfor the sameflow_id, so the API can dispatch the denial twice. The lifecycle cleanup path is meant to be idempotent; add a retry-before-ack regression and skip already-emitted turn-gate flows when buildingcanceled_turn_gate_continuations.🤖 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 `@crates/ironclaw_auth/src/fakes.rs` around lines 1369 - 1395, Update the lifecycle cleanup logic around the flow filter and canceled_turn_gate_continuations construction to exclude terminal TurnGateResume flows whose continuation_emitted_at is already set, while still including unacknowledged terminal flows on the first cleanup. Add a regression test that repeats uninstall before mark_continuation_dispatched and verifies no duplicate AuthContinuationEvent is enqueued for the same flow_id.
🤖 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_auth/tests/auth_product_contract/cleanup_contract.rs`:
- Around line 837-912: Add coverage to
expired_unacknowledged_turn_gate_cleanup_emits_once_then_converges by invoking
cleanup_for_lifecycle a second time before mark_continuation_dispatched and
asserting no duplicate canceled_turn_gate_continuations entry is emitted. Keep
the existing acknowledgment and post-acknowledgment retry assertions, ensuring
the test verifies convergence both before and after acknowledgment.
---
Outside diff comments:
In `@crates/ironclaw_auth/src/fakes.rs`:
- Around line 1369-1395: Update the lifecycle cleanup logic around the flow
filter and canceled_turn_gate_continuations construction to exclude terminal
TurnGateResume flows whose continuation_emitted_at is already set, while still
including unacknowledged terminal flows on the first cleanup. Add a regression
test that repeats uninstall before mark_continuation_dispatched and verifies no
duplicate AuthContinuationEvent is enqueued for the same flow_id.
🪄 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: 21445c20-87a8-4792-a2e4-0587c152258d
📒 Files selected for processing (4)
Cargo.tomlcrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rscrates/ironclaw_extensions/tests/extension_contract.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6169
July 20, 2026 19:06
Destroyed
Collaborator
Author
|
@coderabbitai full review |
Collaborator
Author
|
@ironloopai review --agent reviewer |
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | ad6ef669ae3d |
Head: ad6ef669ae3daa792c8624a556058e0985fbb528
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
One blocking durability issue: a failed replacement setup flow can cancel the user's previous live setup flow before the new request is validated or persisted.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Make setup-flow replacement failure-atomic
Location: crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs:177
Supersession is committed before the replacement request is validated or written. If its update binding has gone stale, or the final flow write fails, this returns an error only after canceling every existing setup flow, leaving the owner with no usable OAuth popup. Validate/reserve the replacement before canceling predecessors, or compensate the cancellations on every failure path; add regressions for stale binding and write failure preserving the prior live flow.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| request: NewAuthFlow, | ||
| ) -> Result<AuthFlowRecord, AuthProductError> { | ||
| if is_setup_class_continuation(&request.continuation) { | ||
| self.supersede_setup_flows(&request.scope, &request.provider) |
Contributor
There was a problem hiding this comment.
Supersession is committed before the request is validated or the replacement flow is persisted. A stale update binding or write_flow failure therefore returns an error after canceling every existing setup flow, leaving no usable popup. Validate/reserve first, or roll back the cancellations on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs (1)
618-652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame terminal-status duplication as
fakes.rs.Lines 633-641 re-enumerate
Completed | Canceled | Failed | Expiredinstead of callingis_terminal_status(record.status), mirroring the identical pattern incrates/ironclaw_auth/src/fakes.rs. See consolidated comment.🤖 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 `@crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs` around lines 618 - 652, Update mark_continuation_dispatched to use the existing is_terminal_status(record.status) helper instead of re-enumerating Completed, Canceled, Failed, and Expired. Preserve the current FlowAlreadyTerminal error and all subsequent idempotent continuation-marking behavior.crates/ironclaw_auth/src/fakes.rs (1)
578-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
is_terminal_statusinstead of re-enumerating the terminal set.This inlines
Completed | Canceled | Failed | Expiredinstead of callingcrate::is_terminal_status(record.status), which already encodes the exact same set and is used elsewhere in this file. Two independent enumerations of "terminal" now exist and can silently drift.♻️ Proposed fix
- if !matches!( - record.status, - AuthFlowStatus::Completed - | AuthFlowStatus::Canceled - | AuthFlowStatus::Failed - | AuthFlowStatus::Expired - ) { - return Err(AuthProductError::FlowAlreadyTerminal); - } + if !crate::is_terminal_status(record.status) { + return Err(AuthProductError::FlowAlreadyTerminal); + }🤖 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 `@crates/ironclaw_auth/src/fakes.rs` around lines 578 - 586, Update the terminal-status check near the AuthFlowStatus handling to call the existing crate::is_terminal_status(record.status) helper instead of re-enumerating Completed, Canceled, Failed, and Expired. Preserve the current FlowAlreadyTerminal error behavior for non-terminal statuses.
♻️ Duplicate comments (1)
crates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rs (1)
2496-2555: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRestart test still reuses the same
InMemoryAuthProductServicesArc — flow-store durability remains unproven.The prior ask was explicit: reconstruct
RebornProductAuthServicesfrom a newFilesystemAuthProductServicesover the same durable filesystem +SecretStore. Hereshared(line 2504) is the sameArc<InMemoryAuthProductServices>passed to both the original andrestarted_product_auth(line 2526) — itsMutex<AuthState>is never dropped, so the flow record's "survival" across this test is unconditional. Only theSecretStorewrapper is genuinely rebuilt (newFilesystemSecretStoreover the retainedsecret_backend), which does validate the PKCE-verifier durability claim inCLAUDE.md, but not full flow-record restart durability.Either use
FilesystemAuthProductServices(fromdurable/flows.rs) forsharedas previously requested, or narrow this test's name/doc to state it only proves SecretStore-layer restart durability.🤖 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 `@crates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rs` around lines 2496 - 2555, The restart test currently reuses the same InMemoryAuthProductServices instance, so flow-record persistence is not exercised. Update google_oauth_callback_completes_after_restart_rebuilds_route_state to construct both service bundles with a new FilesystemAuthProductServices handle over the same durable filesystem, while retaining the rebuilt SecretStore handle; ensure the original service instance is dropped before reconstruction so the test verifies flow durability across restart.
🤖 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/product_auth/durable/flows.rs`:
- Around line 252-275: Update create_flow so the setup-creation lease acquired
by acquire_setup_creation is owned by a Drop guard that performs fire-and-forget
release when the future is cancelled or exits, while preserving the existing
timeout and result behavior. Ensure normal completion does not double-release,
and add a cancellation test that drops create_flow while
create_flow_after_coordination is in flight and verifies the lease becomes
available.
---
Outside diff comments:
In `@crates/ironclaw_auth/src/fakes.rs`:
- Around line 578-586: Update the terminal-status check near the AuthFlowStatus
handling to call the existing crate::is_terminal_status(record.status) helper
instead of re-enumerating Completed, Canceled, Failed, and Expired. Preserve the
current FlowAlreadyTerminal error behavior for non-terminal statuses.
In `@crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs`:
- Around line 618-652: Update mark_continuation_dispatched to use the existing
is_terminal_status(record.status) helper instead of re-enumerating Completed,
Canceled, Failed, and Expired. Preserve the current FlowAlreadyTerminal error
and all subsequent idempotent continuation-marking behavior.
---
Duplicate comments:
In `@crates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rs`:
- Around line 2496-2555: The restart test currently reuses the same
InMemoryAuthProductServices instance, so flow-record persistence is not
exercised. Update
google_oauth_callback_completes_after_restart_rebuilds_route_state to construct
both service bundles with a new FilesystemAuthProductServices handle over the
same durable filesystem, while retaining the rebuilt SecretStore handle; ensure
the original service instance is dropped before reconstruction so the test
verifies flow durability across restart.
🪄 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: cb82d258-c2a0-428c-b82c-c785f6382515
📒 Files selected for processing (10)
Cargo.tomlcrates/ironclaw_auth/src/fakes.rscrates/ironclaw_auth/src/flow.rscrates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/product_auth/durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth/durable/paths.rscrates/ironclaw_reborn_composition/src/product_auth/durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rs
💤 Files with no reviewable changes (3)
- crates/ironclaw_auth/src/flow.rs
- crates/ironclaw_auth/tests/auth_product_contract/oauth_flow_contract.rs
- crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rs
Comment on lines
+252
to
+275
| async fn create_flow(&self, request: NewAuthFlow) -> Result<AuthFlowRecord, AuthProductError> { | ||
| if !is_setup_class_continuation(&request.continuation) { | ||
| return self.create_flow_after_coordination(request).await; | ||
| } | ||
|
|
||
| let coordination_path = | ||
| setup_creation_coordination_path(&request.scope, &request.provider)?; | ||
| let coordination_scope = request.scope.resource.clone(); | ||
| let holder = AuthFlowId::new(); | ||
| self.acquire_setup_creation(&coordination_scope, &coordination_path, holder) | ||
| .await?; | ||
| let result = match tokio::time::timeout( | ||
| SETUP_CREATION_OPERATION_TIMEOUT, | ||
| self.create_flow_after_coordination(request), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(result) => result, | ||
| Err(_) => Err(AuthProductError::BackendUnavailable), | ||
| }; | ||
| self.release_setup_creation(&coordination_scope, &coordination_path, holder) | ||
| .await; | ||
| result | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any HTTP handler wraps create_flow (directly or via start_oauth_flow-style
# helpers) in a context that can cancel the future (timeout/select!) before completion.
rg -nP 'create_flow\(' crates/ironclaw_reborn_composition/src/product_auth/serve --type rust -A3 -B3Repository: nearai/ironclaw
Length of output: 3624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the create_flow caller and lease implementation around the reported lines.
sed -n '1500,1565p' crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rs
printf '\n----\n'
sed -n '1,220p' crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs
printf '\n----\n'
sed -n '220,310p' crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rsRepository: nearai/ironclaw
Length of output: 15047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the actual OAuth handler call sites for create_flow and any surrounding
# timeout/select/cancellation boundaries.
rg -n -C 4 'create_flow\(|tokio::time::timeout|select!\s*\{' crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rsRepository: nearai/ironclaw
Length of output: 1716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the OAuth start handler body around the create_flow call and surrounding awaits.
sed -n '1460,1595p' crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rsRepository: nearai/ironclaw
Length of output: 5777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all create_flow call sites in oauth.rs and show the surrounding code,
# especially any handler bodies with timeout/select/cancellation boundaries.
rg -n -C 6 '\.create_flow\(' crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rs
printf '\n---- timeout/select in file ----\n'
rg -n -C 3 'tokio::time::timeout|select!\s*\{' crates/ironclaw_reborn_composition/src/product_auth/serve/oauth.rsRepository: nearai/ironclaw
Length of output: 1980
Setup-creation lease needs a drop guard. (crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs:252-275) create_flow acquires the durable coordination record, then releases it only on the normal return path. If the future is cancelled after acquire_setup_creation succeeds, the lease stays held until SETUP_CREATION_LEASE_SECONDS, blocking retries for the same owner+provider. Wrap the lease in a Drop guard that releases fire-and-forget, and add a cancellation test that drops the future mid-flight.
🤖 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 `@crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs` around
lines 252 - 275, Update create_flow so the setup-creation lease acquired by
acquire_setup_creation is owned by a Drop guard that performs fire-and-forget
release when the future is cancelled or exits, while preserving the existing
timeout and result behavior. Ensure normal completion does not double-release,
and add a cancellation test that drops create_flow while
create_flow_after_coordination is in flight and verifies the lease becomes
available.
BenKurrek
added a commit
that referenced
this pull request
Jul 20, 2026
…th-flow liveness) 26 conflicts, resolved by re-expressing #6169's semantics onto this branch's architecture per the standing reconciliation philosophy: - The 7 composition slack/* modify/delete conflicts keep this branch's deletions (the extension runtime replaced composition-Slack wholesale; this branch has no connection-epoch concept to delete). - ironclaw_auth: adopted #6169's SecretCleanupRequest.lifecycle_package (package-keyed uninstall cancel for shared-provider extensions), CanceledCleanupFlow + SecretCleanupReport.canceled_flows, the AwaitingUser->Expired terminal set, and the create_flow-seam setup-class supersede (fake + durable store, with main's durable CAS-coordinated creation lease). The #5957 continuation-dispatch machinery this branch's owner decision already rejected (claim/settle, OAuthCompletionCompensation*, OAuthExchangeCleanup*) stays out; main's continuation-dispatch contract test is re-expressed over this branch's fail_completed_continuation equivalent. - Durable cleanup: this branch's Deactivate+Uninstall cancel breadth (owner decision 2026-07-15) composed with #6169's package-keyed selection and canceled_flows reporting. - product_workflow auth_interaction: main's read-then-clock ordering and inclusive expires_at boundary adopted; this branch's A2a labels kept. - Serve/product-auth API keeps this branch's engine-based thin path (main's 1.2k-line rework targets the fat in-composition OAuth this branch deleted). The process-local PKCE cache remains; porting #6169's durable per-flow setup-PKCE verifiers (and consuming canceled_flows to eagerly drop them) onto the engine-based path is an explicit follow-up recorded in composition CLAUDE.md and the PR body. The callback verifier read keeps its cache-then-gate-driver fallback; the extension OAuth start re-stores the verifier post-create. - The obsolete durable supersede test (SetupOnly-only semantics) is replaced by main's create_flow-seam setup-class version; main's new google-family shared-account scenario is adapted to this branch's VendorId rename; the grafted channel-identity-hook journey keeps its imports; composition's DCR test file stays deleted (the auth engine owns DCR here, with its own caller-level suite incl. the PSL fix). Verified: workspace check clean; ironclaw_auth 130+ tests, composition 1356, product_workflow/event_projections/architecture, the three extension integration suites, all six auth-domain suites, and group_extensions (incl. the new google-family scenario) all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vc5Mqbiyk2bACEkZnQhozh
BenKurrek
added a commit
that referenced
this pull request
Jul 21, 2026
… dedup, legacy-slack startup guard Closes the three audit findings from the eighth-fold window review: - Durable setup-PKCE verifiers (#6169 port, the documented follow-up): start_setup_oauth_flow now writes the raw verifier to the injected SecretStore under product-auth-setup-pkce-{flow_id} (TTL = the flow's expires_at) BEFORE creating the flow, the callback read tries the setup-lane handle before the gate-store fallback, terminal callback outcomes discard the durable copy alongside the process-local cache (early defensive cache evictions deliberately do not), and cleanup_credentials_for_lifecycle eagerly drops verifiers for report.canceled_flows. The serve-layer cache is now a same-process fast path only. Pinned red-first by vendor_oauth_callback_completes_after_route_state_restart (fresh route state over the same services = restart/replica hand-off). - cancel_superseded_setup_flows deleted (trait method + durable + fake impls + the start-seam call): create_flow owns setup-class supersession in both impls, so the seam call was a strict-subset duplicate executed twice per setup start. - serve rejects populated legacy [slack] setup fields at startup with a pointer to the WebUI extensions page (restores the guard lost with the host-beta lane; [slack].enabled stays tolerated and the config-set knob keeps working, with the guidance no longer naming the retired redirect-URI env var). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Connectingslot and its reopen/conflict choreography.AuthFlowManager::create_flow, including durable cross-replica CAS coordination so concurrent starts leave exactly one live setup flow.TurnGateResumecleanup.Change Type
Linked Issue
None. This re-lands the reverted #6130 work and completes the structural fix that removes the duplicate Slack attempt-liveness record.
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo build(covered by the workspace all-targets clippy and test builds)cargo test -p ironclaw_auth(117 tests)cargo test -p ironclaw_reborn_composition(full crate and integration suite; 1,671 library tests plus integration targets)cargo test -p ironclaw_architecturecargo test --features integrationif database-backed or integration behavior changed (the owning composition integration targets were run through the full package suite; no database schema changed)Security Impact
OAuth attempt liveness and Slack binding cleanup are security-sensitive. The change removes a second, conflicting liveness authority while retaining generation fences for disconnect and ingress authorization. Callback claims remain scope-checked; setup drift fails closed; secrets remain host-side in the mediated
SecretStore; no authentication, origin, rate-limit, or redaction policy is weakened.Reborn Trust-Boundary Checklist
create_flowowns the invariant.Expiredterminal flows are now accepted bymark_continuation_dispatchedin both durable and fake implementations.serde(default)fields fail closed or have migration tests. The coordination record has no permissive defaults; malformed storage maps to backend unavailable.Database Impact
No SQL migration or schema change. Durable setup creation adds a small filesystem coordination record beneath the existing auth-flow owner root. Older code ignores the subdirectory, and expired leases are safely reclaimable, so rollback does not require wiping or migrating a volume.
Blast Radius
The installation-store tenant/disk model is unchanged from
mainby the final review fixes.Rollback Plan
Revert this PR if the lifecycle refactor must be rolled back. The new coordination files are inert to the previous reader and may remain on disk. No database rollback or volume wipe is required. Reverting only the final CAS commit is not recommended because it would restore the cross-replica concurrent-start race.
Review Follow-Through
RebornProductAuthServicesand the filesystem secret-store handle.mark_continuation_dispatched; acknowledging before dispatch could strand a turn gate after a crash, while downstream gate denial is idempotent.Review track: C (security-sensitive OAuth lifecycle and durable coordination)
Before / after architecture
Before this PR, one Slack connection attempt was represented by both a provider-neutral auth-flow record and a Slack-private
Connectingslot. The two stores implemented conflicting reopen policies and required route-specific abandon/reconcile choreography.After this PR, the auth-flow record alone answers whether an attempt is live. Starting a new setup flow supersedes the prior setup-class flow for the same owner and provider. The Slack generation remains only as a fence around durable bindings and disconnect cleanup, where no auth flow can replace it.
Setup starts perform no Slack connection-state write. A successful callback binds the identity and records the active generation. A canceled, failed, or expired flow can be cleaned up and durably acknowledged. Disconnect uses the retained generation journal to stop ingress authorization immediately and to prevent an older sweep from deleting a newer binding.
This keeps setup liveness, connection authorization, and cleanup fencing as separate responsibilities without adding mirror DTOs, new
dynseams, or local-only production implementations.