Repository navigation
fix(reborn): cancel stale AuthFlow on Slack auth auto-deny (#4952) - #5014
Conversation
When a Slack run blocked on interactive auth is auto-denied (non-OAuth challenge the Slack surface can't satisfy, or an OAuth URL suppressed by the send-time DM backstop), the delivery path cancelled the run directly via TurnCoordinator but skipped AuthFlowManager::cancel_flow — unlike the canonical AuthInteractionService deny path. The AuthFlow record lingered non-terminal (Pending/AwaitingUser) until expiry. State-drift / cleanliness gap, no security exposure. Fix: add a narrow BlockedAuthFlowCanceller port (impl by RebornProductAuthServices over its flow_record_source + flow_manager) and thread it through the shared cancel_auth_blocked_run helper so the flow is cancelled alongside the run at all three sites (live observer, triggered non-OAuth arm, OAuth send-time backstop). - include_terminal:false → already-terminal/Completed flows resolve to None and are a graceful no-op (handles the OAuth-callback race). - Best-effort: a flow-cancel failure is debug-logged and does not block the run cancellation (the user-visible terminal action). - None when no flow_record_source is wired (Slack without product-auth) — skips flow cancel, still cancels the run. Backward-compatible. - Regression test drives the live observer caller with a recording fake canceller (test-through-the-caller). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address local review (bugs/security/maintainability/tests):
- Make cancel_auth_blocked_run's gate_ref an explicit Option<&str> and skip
the flow cancel when absent, instead of passing an empty-string sentinel
that silently no-ops inside the facade (was unwrap_or_default()).
- Add regression coverage for the two previously-untested call sites and the
facade impl:
* triggered_non_oauth_auth_cancels_stale_auth_flow (triggered non-OAuth arm)
* triggered_oauth_backstop_cancels_stale_auth_flow (OAuth send-time backstop)
* cancel_blocked_auth_flow_{cancels_non_terminal_flow,is_noop_when_flow_absent,
is_noop_without_flow_record_source} (facade unit tests)
- Assert the cancelled run_id in blocked_auth_cancels_stale_auth_flow.
Security review: no findings (owner+gate scoped lookup, fail-closed terminal
guard, best-effort cancel does not block run cancellation).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
🚅 Deployed to the ironclaw-pr-5014 environment in ironclaw-ci-preview
|
github-actions
Bot
added
size: XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces a ChangesBlockedAuthFlowCanceller trait, implementation, and Slack delivery wiring
Sequence Diagram(s)sequenceDiagram
participant SlackInbound as Slack Inbound / Triggered Delivery
participant cancel_auth_blocked_run
participant TurnCoordinator
participant BlockedAuthFlowCanceller
participant flow_record_source
participant flow_manager
SlackInbound->>cancel_auth_blocked_run: scope, gate_ref, auth_flow_canceller
cancel_auth_blocked_run->>TurnCoordinator: cancel_run (idempotent)
TurnCoordinator-->>cancel_auth_blocked_run: Ok(...)
alt canceller present and gate_ref provided
cancel_auth_blocked_run->>BlockedAuthFlowCanceller: cancel_blocked_auth_flow(scope, owner_user_id, run_id, gate_ref)
BlockedAuthFlowCanceller->>flow_record_source: flow_for_turn_gate(include_terminal: false)
flow_record_source-->>BlockedAuthFlowCanceller: Some(flow) | None
alt non-terminal flow found
BlockedAuthFlowCanceller->>flow_manager: cancel_flow(flow_id)
flow_manager-->>BlockedAuthFlowCanceller: Ok(()) or terminal race
end
BlockedAuthFlowCanceller-->>cancel_auth_blocked_run: Ok(()) best-effort
end
cancel_auth_blocked_run-->>SlackInbound: cancel_run result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69d0734620
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let (Some(canceller), Some(gate_ref)) = (auth_flow_canceller, gate_ref) { | ||
| let owner_user_id = scope.explicit_owner_user_id().unwrap_or(&actor.user_id); | ||
| if let Err(error) = canceller | ||
| .cancel_blocked_auth_flow(scope, owner_user_id, run_id, gate_ref) | ||
| .await |
There was a problem hiding this comment.
When cancel_run fails after these lines succeed (the OAuth backstop already treats cancel failure as possible and deliberately leaves the Slack prompt in place), the run remains blocked but its backing AuthFlow has already been marked canceled, so the user can no longer complete the still-visible auth prompt and a retry sees no active flow. Keep the flow cancel after a successful run cancellation, or otherwise avoid making the auth flow terminal when the run cancellation did not take effect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 315b850. Reordered cancel_auth_blocked_run to cancel the run FIRST and cancel the AuthFlow only after cancel_run succeeds (it's idempotent via slack-auth-block:{run_id}). A failed run-cancel now returns early and leaves both the flow and the still-usable prompt intact — the OAuth backstop relies on that. Regression: blocked_auth_cancel_run_failure_leaves_auth_flow_intact (cancel_run fails → recorder asserts zero flow-cancel calls).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/auth.rs`:
- Around line 1425-1426: The map_err closure in the AuthGateRef::new() call is
discarding the underlying error cause with |_| and using
AuthProductError::BackendUnavailable which is semantically incorrect for input
validation failures. Replace the map_err(|_|
AuthProductError::BackendUnavailable) pattern with map_err that captures the
actual error and converts it to AuthProductError::InvalidRequest with the reason
field populated from the error message. Apply this same fix to all occurrences
mentioned in the comment (including lines around 1444-1445).
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 1159-1179: Reorder the AuthFlow cancellation to occur after the
run cancellation to prevent state drift when run cancellation fails. Currently,
the if block checking auth_flow_canceller and gate_ref that calls
cancel_blocked_auth_flow executes before the run is cancelled, so if cancel_run
fails, the run remains in BlockedAuth state while its backing flow is already
terminal. Move this entire best-effort flow cleanup block to execute after a
successful run cancellation attempt, ensuring the run reaches a terminal state
first. Additionally, add a regression test using cancel_should_fail that
verifies no flow cancellation occurs when the run cancellation fails.
- Around line 4473-4483: The cancel_blocked_auth_flow method signature accepts
four key parameters (scope, owner_user_id, run_id, and gate_ref) but the test
double only records run_id and gate_ref when pushing to self.calls, dropping the
scope and owner_user_id arguments. Modify the data structure that self.calls
holds to capture all four arguments instead of just two, update the push
statement in cancel_blocked_auth_flow to include scope and owner_user_id
alongside run_id and gate_ref, and add assertions in at least one live test that
verify the recorded scope and owner_user_id match the expected values to prevent
regressions.
🪄 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: 8d4e8f91-6ce7-4cd5-9a8f-df4777e42986
📒 Files selected for processing (7)
crates/ironclaw_reborn_composition/src/auth.rscrates/ironclaw_reborn_composition/src/auth_prompt.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_host_beta.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Cancel stale Slack auth flows when auto-denying blocked auth runs.
Stats: 2 findings (from 6 raw, 2 after verification/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1.
Bugs
-
Medium Flow is canceled before the run, so a run-cancel failure strands state (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1159-1179, confidence 84) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1159(already open: #5014 (comment))cancel_auth_blocked_runclears the durable auth-flow record before it attemptscancel_run. Ifcancel_runfails, the run can remain blocked while the backing auth flow is already terminal, so the still-live auth prompt can no longer be completed and a retry no longer sees an active flow.
Conventions
-
Medium New flow-cancel path drops parse errors with
map_err(|_| ...)(crates/ironclaw_reborn_composition/src/auth.rs:1425-1445, confidence 100) — anchor:.claude/rules/error-handling.md:16-32The new
cancel_blocked_auth_flowimplementation maps bothAuthGateRef::new(...)andTurnRunRef::new(...)failures to a genericBackendUnavailablewithmap_err(|_| ...). The repo error-handling rule explicitly rejects this shape because it drops the underlying cause.
| run_id: TurnRunId, | ||
| gate_ref: &str, | ||
| ) -> Result<(), AuthProductError> { | ||
| let gate_ref = AuthGateRef::new(gate_ref.to_string()) |
There was a problem hiding this comment.
Medium — New flow-cancel path drops parse errors with map_err(|_| ...).
The new cancel_blocked_auth_flow implementation maps both AuthGateRef::new(...) and TurnRunRef::new(...) failures to a generic BackendUnavailable with map_err(|_| ...). The repo error-handling rule explicitly rejects this shape because it drops the underlying cause.
Fix: Preserve the bound error with an error constructor that carries/logs the source, or log the bound error before converting to BackendUnavailable.
There was a problem hiding this comment.
Fixed in 315b850 — both parses in cancel_blocked_auth_flow now use InvalidRequest { reason: format!("…: {err}") } preserving the source error.
…review)
Address PR review:
- ORDERING (codex P2 / coderabbit Major): cancel the run FIRST, then cancel the
durable AuthFlow only on a successful/idempotent cancel_run. Cancelling the flow
first meant a failed cancel_run left the run BlockedAuth with a terminal flow —
the inverse state drift this PR fixes, and it broke the OAuth backstop contract
that a failed cancel leaves the prompt usable. Regression test:
blocked_auth_cancel_run_failure_leaves_auth_flow_intact (cancel_run fails →
recorder asserts NO flow cancellation).
- ERROR CAUSE (coderabbit/henry): replace map_err(|_| BackendUnavailable) on the
new AuthGateRef/TurnRunRef parses with AuthProductError::InvalidRequest{reason}
carrying the bound error.
- TEST DOUBLE (coderabbit): RecordingBlockedAuthFlowCanceller now captures all four
args (scope, owner_user_id, run_id, gate_ref); live + triggered tests assert the
resolved owner_user_id and scope, not just run_id/gate_ref.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Cancel stale AuthFlow records when Slack auth auto-denies a blocked run, while keeping run cancellation behavior intact.
Stats: 3 findings (from 5 raw, 3 kept after validation) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1.
Conventions
- Medium cancel_flow terminal race can still surface as an error (
crates/ironclaw_reborn_composition/src/auth.rs:1459-1462, confidence 90) — anchor:crates/ironclaw_reborn_composition/src/auth.rs:1460
The new canceller is documented to treat an already-terminal flow as a graceful no-op, butcancel_flowcan still returnCanceledorFlowAlreadyTerminalif the flow terminalizes after the read and before the cancel.
Tests
- Medium Best-effort auth-flow cancel failure is untested (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1195-1207, confidence 91) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1195
Existing tests cover successful canceller wiring andcancel_runfailure, but not the branch wherecancel_runsucceeds and the best-effort flow cleanup fails and is swallowed.
Local Patterns
- Low Update the helper doc to mention flow cancellation (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1145-1150, confidence 87) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1145(no diff position — body only)
The doc comment abovecancel_auth_blocked_runstill describes only run-levelcancel_runbehavior, but the function now also performs best-effortBlockedAuthFlowCancellercleanup after the run succeeds.
| return Ok(()); | ||
| }; | ||
| self.flow_manager | ||
| .cancel_flow(&flow.scope, flow.id) |
There was a problem hiding this comment.
Medium — cancel_flow terminal race can still surface as an error.
The new canceller is documented to treat an already-terminal flow as a graceful no-op, but it first reads a non-terminal flow with include_terminal=false and then calls cancel_flow. If an OAuth callback or another canceller terminalizes the flow between those steps, cancel_flow returns Canceled or FlowAlreadyTerminal instead of Ok, so the documented no-op race contract is not actually upheld.
Fix: Normalize AuthProductError::Canceled and AuthProductError::FlowAlreadyTerminal from cancel_flow to Ok(()) in cancel_blocked_auth_flow, while still propagating real lookup/scope/backend errors.
There was a problem hiding this comment.
Fixed in 3abeb20. cancel_blocked_auth_flow now normalizes AuthProductError::Canceled and AuthProductError::FlowAlreadyTerminal from cancel_flow to Ok(()), so a flow that terminalizes between the non-terminal read and the cancel honors the documented no-op race contract. Real lookup/scope/backend errors still propagate. Test cancel_blocked_auth_flow_treats_terminal_race_as_ok covers both terminal variants plus a negative case (BackendUnavailable still propagates).
| .await?; | ||
|
|
||
| // Run is now terminal — cancel the stale `AuthFlow` record alongside it (#4952). | ||
| // Best-effort cleanliness: a flow-cancel failure does not surface, since the |
There was a problem hiding this comment.
Medium — Best-effort auth-flow cancel failure is untested.
The new branch that swallows BlockedAuthFlowCanceller::cancel_blocked_auth_flow errors has no caller-level test. Existing tests cover successful canceller wiring and cancel_run failure, but not the intended best-effort case where cancel_run succeeds and flow cleanup fails without breaking Slack auto-denial.
Fix: Add tests::blocked_auth_canceller_failure_is_swallowed covering cancel_auth_blocked_run continuing after a canceller error.
There was a problem hiding this comment.
Added in 3abeb20: blocked_auth_canceller_failure_is_swallowed — cancel_run succeeds, the BlockedAuthFlowCanceller returns Err, and the live observer path still cancels the run once and posts SLACK_AUTH_UNAVAILABLE_MESSAGE, proving the flow-cancel failure is swallowed without breaking auto-denial.
…wallow (PR #5014 review) - TERMINAL RACE (review Medium): cancel_blocked_auth_flow read the flow with include_terminal:false then called cancel_flow; if the flow terminalized in between (concurrent OAuth callback / canceller), cancel_flow returned Canceled/FlowAlreadyTerminal, breaking the documented graceful-no-op contract. Normalize those two to Ok(()); real lookup/scope/backend errors still propagate. Test cancel_blocked_auth_flow_treats_terminal_race_as_ok (both terminal variants + a negative case proving BackendUnavailable still propagates). - BEST-EFFORT SWALLOW (review Medium): add caller-level test blocked_auth_canceller_failure_is_swallowed — cancel_run succeeds, flow-cancel errors, auto-denial still cancels the run and posts the unavailable notice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 4679-4764: The test `blocked_auth_canceller_failure_is_swallowed`
does not verify that the `cancel_blocked_auth_flow` method was actually invoked
on the `FailingBlockedAuthFlowCanceller`. Modify the
`FailingBlockedAuthFlowCanceller` struct to track invocation counts (using an
atomic counter or similar), increment the counter when
`cancel_blocked_auth_flow` is called, and add an assertion in the test to verify
that the canceller was invoked exactly once. This ensures the test will fail if
the code stops calling `cancel_blocked_auth_flow`.
🪄 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: 531568d4-418d-4035-b48d-f43a03a84aa4
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/auth.rscrates/ironclaw_reborn_composition/src/slack_delivery.rs
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Cancel stale AuthFlow records when Slack auth runs are auto-denied so the run and durable flow state stay aligned.
Stats: 4 findings (from 6 raw, 4 after filtering/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Tests
- Medium Malformed gate refs in auth-flow cancel are untested (
crates/ironclaw_reborn_composition/src/auth.rs:1425-1429, confidence 95) — anchor:crates/ironclaw_reborn_composition/src/auth.rs:1425
cancel_blocked_auth_flowmaps an invalidgate_refintoAuthProductError::InvalidRequest, but no test forces that parse failure. - Medium Flow lookup failures are not covered (
crates/ironclaw_reborn_composition/src/auth.rs:1438-1455, confidence 94) — anchor:crates/ironclaw_reborn_composition/src/auth.rs:1438
The newflow_for_turn_gate(...).await?propagation is never forced to fail in tests.
Maintainability
- Medium Collapse the second flow-record facade (
crates/ironclaw_reborn_composition/src/auth.rs:757-765, confidence 77) — anchor:crates/ironclaw_reborn_composition/src/auth.rs:757
as_blocked_auth_flow_cancellerrepeats the sameflow_record_source.is_some()gating already used byas_auth_challenge_provider.
Local Patterns
- Low Auth-flow canceller doc is narrower than the shared helper (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:130-134, confidence 78) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:130
The field comment framesauth_flow_cancelleras only handling non-OAuth auth challenges, but the shared helper also handles the OAuth backstop path.
| run_id: TurnRunId, | ||
| gate_ref: &str, | ||
| ) -> Result<(), AuthProductError> { | ||
| let gate_ref = AuthGateRef::new(gate_ref.to_string()).map_err(|err| { |
There was a problem hiding this comment.
Medium — Malformed gate refs in auth-flow cancel are untested.
cancel_blocked_auth_flow maps an invalid gate_ref into AuthProductError::InvalidRequest, but no test forces that parse failure. If the gate-ref validation changes or stops surfacing the right error, this branch would go unverified.
Fix: Add tests::auth::cancel_blocked_auth_flow_rejects_invalid_gate_ref covering an invalid gate ref and asserting the InvalidRequest reason preserves the parse failure.
There was a problem hiding this comment.
Added in a187119: cancel_blocked_auth_flow_rejects_invalid_gate_ref (empty gate_ref → AuthProductError::InvalidRequest with reason containing "invalid gate ref for auth-flow cancel").
| // one) resolves to `None`, so the OAuth-callback race — where the flow | ||
| // completes just before auto-deny — is a graceful no-op rather than an | ||
| // error. We only ever cancel a flow that is still non-terminal. | ||
| let flow = source |
There was a problem hiding this comment.
Medium — Flow lookup failures are not covered.
The new flow_for_turn_gate(...).await? propagation is never forced to fail in tests. A stub AuthFlowRecordSource that returns Err(BackendUnavailable) should be exercised so this helper does not accidentally swallow backend lookup failures.
Fix: Add tests::auth::cancel_blocked_auth_flow_propagates_flow_source_error using a failing AuthFlowRecordSource and asserting BackendUnavailable propagates.
There was a problem hiding this comment.
Added in a187119: cancel_blocked_auth_flow_propagates_flow_source_error using a local AlwaysFailingFlowSource (flow_for_turn_gate → Err(BackendUnavailable)), asserting the lookup error propagates.
| /// projection source. In that case the Slack path simply skips flow cancel and | ||
| /// still cancels the run, which is backward-compatible. | ||
| #[doc(hidden)] | ||
| pub fn as_blocked_auth_flow_canceller( |
There was a problem hiding this comment.
Medium — Collapse the second flow-record facade.
as_blocked_auth_flow_canceller repeats the same flow_record_source.is_some() gating already used by as_auth_challenge_provider, so RebornProductAuthServices now exposes two nearly identical optional facades over one underlying capability. That adds another public composition hop for Slack wiring without hiding new complexity.
Fix: Keep one auth-flow facade/accessor for flow-backed auth services, or have runtime composition derive both Slack handles from the same helper instead of adding a second near-identical accessor.
There was a problem hiding this comment.
Deduped in 66a6830: extracted the shared flow_record_source.is_some() precondition behind a private has_flow_record_source() that both accessors call, so the gate can't drift. Kept them as two accessors rather than collapsing to one: they return distinct capability ports (AuthChallengeProvider vs BlockedAuthFlowCanceller), and the pre-existing as_auth_challenge_provider has several callers outside Slack (runtime ×2, DCR/factory tests) — a full collapse would churn those for no behavior gain. The only real duplication was the one-line gate, now defined once.
| /// challenges are surfaced in Slack; other challenge kinds are denied (see the | ||
| /// `BlockedAuth` arm of `notification_for_actionable_state`). | ||
| pub auth_challenges: Option<Arc<dyn AuthChallengeProvider>>, | ||
| /// Cancels the durable `AuthFlow` record when a `BlockedAuth` run is auto-denied |
There was a problem hiding this comment.
Low — Auth-flow canceller doc is narrower than the shared helper.
The field comment frames auth_flow_canceller as only handling Slack auto-deny of non-OAuth auth challenges, but the same handle is now threaded through the shared cancel_auth_blocked_run helper and used by the OAuth backstop too. That makes the comment stale for the next person touching this path.
Fix: Rewrite the comment to describe the shared blocked-auth cancellation contract across all callers, not just the non-OAuth Slack branch.
There was a problem hiding this comment.
Fixed in a187119 — rewrote the auth_flow_canceller doc to describe the shared blocked-auth cancellation contract across all three callers (live non-OAuth deny, triggered non-OAuth deny, OAuth send-time backstop), cancelled after the run cancel succeeds.
… widen field doc (PR #5014 review) - coderabbit: blocked_auth_canceller_failure_is_swallowed now asserts the failing canceller was actually invoked (call_count == 1) so the swallow test can't pass if the wiring stops calling cancel_blocked_auth_flow. - review: add cancel_blocked_auth_flow_rejects_invalid_gate_ref (invalid gate_ref -> InvalidRequest{reason} preserving the parse context). - review: add cancel_blocked_auth_flow_propagates_flow_source_error (flow_for_turn_gate backend error propagates, not swallowed). - review: rewrite the auth_flow_canceller field doc to describe the shared blocked-auth cancellation contract across all three callers (live non-OAuth, triggered non-OAuth, OAuth send-time backstop), not just the non-OAuth branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract the shared `flow_record_source.is_some()` precondition behind a private `has_flow_record_source()` so the two flow-backed facades (as_auth_challenge_provider / as_blocked_auth_flow_canceller) cannot drift. Kept as two accessors since they expose distinct capability ports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Cancel stale Slack auth flows when a blocked run is auto-denied, while keeping run cancellation behavior unchanged.
Stats: 2 findings (from 4 raw, 2 after filtering/dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
tests
- Medium Missing test for actor-user fallback on blocked-auth cancel (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1169-1173, confidence 81) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1172
The new owner-resolution branch incancel_auth_blocked_runfalls back toactor.user_idwhenscope.explicit_owner_user_id()isNone, but the added Slack caller tests only assert the explicit-owner path. A regression here would break stale-flow cancellation for scopes without an explicit owner while the current tests would still pass.
local-patterns
- Low Use the module's named error field in the best-effort cancel log (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1208-1212, confidence 93) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:687-707
This new debug log formats the failure as%error, while surrounding logs inslack_delivery.rsconsistently use named fields such aserror = %error. That drift makes the new cancellation log harder to filter with the rest of the module's structured tracing.
| let flow_cancel_target = match (auth_flow_canceller, gate_ref) { | ||
| (Some(canceller), Some(gate_ref)) => { | ||
| let owner_user_id = scope | ||
| .explicit_owner_user_id() |
There was a problem hiding this comment.
Medium — Missing test for actor-user fallback on blocked-auth cancel.
The new owner-resolution branch in cancel_auth_blocked_run falls back to actor.user_id when scope.explicit_owner_user_id() is None, but the added Slack caller tests only assert the explicit-owner path. A regression here would break stale-flow cancellation for scopes without an explicit owner while the current tests would still pass.
Fix: Add tests::slack_delivery::blocked_auth_cancels_stale_auth_flow_falls_back_to_actor_user_when_scope_has_no_owner covering blocked-auth auto-deny with a scope that has no explicit owner.
| tracing::debug!( | ||
| target = "ironclaw::reborn::slack_delivery", | ||
| %run_id, | ||
| %error, |
There was a problem hiding this comment.
Low — Use the module's named error field in the best-effort cancel log.
This new debug log formats the failure as %error, while surrounding logs in slack_delivery.rs consistently use named fields such as error = %error. That drift makes the new cancellation log harder to filter with the rest of the module's structured tracing.
Fix: Change the field to error = %error so the log matches the established structured-tracing style in this module.
…) (nearai#5014) * fix(reborn): cancel stale AuthFlow on Slack auth auto-deny (nearai#4952) When a Slack run blocked on interactive auth is auto-denied (non-OAuth challenge the Slack surface can't satisfy, or an OAuth URL suppressed by the send-time DM backstop), the delivery path cancelled the run directly via TurnCoordinator but skipped AuthFlowManager::cancel_flow — unlike the canonical AuthInteractionService deny path. The AuthFlow record lingered non-terminal (Pending/AwaitingUser) until expiry. State-drift / cleanliness gap, no security exposure. Fix: add a narrow BlockedAuthFlowCanceller port (impl by RebornProductAuthServices over its flow_record_source + flow_manager) and thread it through the shared cancel_auth_blocked_run helper so the flow is cancelled alongside the run at all three sites (live observer, triggered non-OAuth arm, OAuth send-time backstop). - include_terminal:false → already-terminal/Completed flows resolve to None and are a graceful no-op (handles the OAuth-callback race). - Best-effort: a flow-cancel failure is debug-logged and does not block the run cancellation (the user-visible terminal action). - None when no flow_record_source is wired (Slack without product-auth) — skips flow cancel, still cancels the run. Backward-compatible. - Regression test drives the live observer caller with a recording fake canceller (test-through-the-caller). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): code-review fixes for nearai#4952 stale-auth-flow cancel Address local review (bugs/security/maintainability/tests): - Make cancel_auth_blocked_run's gate_ref an explicit Option<&str> and skip the flow cancel when absent, instead of passing an empty-string sentinel that silently no-ops inside the facade (was unwrap_or_default()). - Add regression coverage for the two previously-untested call sites and the facade impl: * triggered_non_oauth_auth_cancels_stale_auth_flow (triggered non-OAuth arm) * triggered_oauth_backstop_cancels_stale_auth_flow (OAuth send-time backstop) * cancel_blocked_auth_flow_{cancels_non_terminal_flow,is_noop_when_flow_absent, is_noop_without_flow_record_source} (facade unit tests) - Assert the cancelled run_id in blocked_auth_cancels_stale_auth_flow. Security review: no findings (owner+gate scoped lookup, fail-closed terminal guard, best-effort cancel does not block run cancellation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): cancel AuthFlow only after cancel_run succeeds (PR nearai#5014 review) Address PR review: - ORDERING (codex P2 / coderabbit Major): cancel the run FIRST, then cancel the durable AuthFlow only on a successful/idempotent cancel_run. Cancelling the flow first meant a failed cancel_run left the run BlockedAuth with a terminal flow — the inverse state drift this PR fixes, and it broke the OAuth backstop contract that a failed cancel leaves the prompt usable. Regression test: blocked_auth_cancel_run_failure_leaves_auth_flow_intact (cancel_run fails → recorder asserts NO flow cancellation). - ERROR CAUSE (coderabbit/henry): replace map_err(|_| BackendUnavailable) on the new AuthGateRef/TurnRunRef parses with AuthProductError::InvalidRequest{reason} carrying the bound error. - TEST DOUBLE (coderabbit): RecordingBlockedAuthFlowCanceller now captures all four args (scope, owner_user_id, run_id, gate_ref); live + triggered tests assert the resolved owner_user_id and scope, not just run_id/gate_ref. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): normalize terminal cancel_flow race + test best-effort swallow (PR nearai#5014 review) - TERMINAL RACE (review Medium): cancel_blocked_auth_flow read the flow with include_terminal:false then called cancel_flow; if the flow terminalized in between (concurrent OAuth callback / canceller), cancel_flow returned Canceled/FlowAlreadyTerminal, breaking the documented graceful-no-op contract. Normalize those two to Ok(()); real lookup/scope/backend errors still propagate. Test cancel_blocked_auth_flow_treats_terminal_race_as_ok (both terminal variants + a negative case proving BackendUnavailable still propagates). - BEST-EFFORT SWALLOW (review Medium): add caller-level test blocked_auth_canceller_failure_is_swallowed — cancel_run succeeds, flow-cancel errors, auto-denial still cancels the run and posts the unavailable notice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test+doc(reborn): cover facade error paths, assert canceller invoked, widen field doc (PR nearai#5014 review) - coderabbit: blocked_auth_canceller_failure_is_swallowed now asserts the failing canceller was actually invoked (call_count == 1) so the swallow test can't pass if the wiring stops calling cancel_blocked_auth_flow. - review: add cancel_blocked_auth_flow_rejects_invalid_gate_ref (invalid gate_ref -> InvalidRequest{reason} preserving the parse context). - review: add cancel_blocked_auth_flow_propagates_flow_source_error (flow_for_turn_gate backend error propagates, not swallowed). - review: rewrite the auth_flow_canceller field doc to describe the shared blocked-auth cancellation contract across all three callers (live non-OAuth, triggered non-OAuth, OAuth send-time backstop), not just the non-OAuth branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): dedup flow-backed facade gate (PR nearai#5014 review) Extract the shared `flow_record_source.is_some()` precondition behind a private `has_flow_record_source()` so the two flow-backed facades (as_auth_challenge_provider / as_blocked_auth_flow_canceller) cannot drift. Kept as two accessors since they expose distinct capability ports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Closes #4952.
Problem
When a Slack run blocked on interactive auth is auto-denied — a non-OAuth challenge the Slack surface can't satisfy, or an OAuth URL suppressed by the #4953 send-time DM backstop — the delivery path cancels the run directly via
TurnCoordinator::cancel_runbut skipsAuthFlowManager::cancel_flow. Unlike the canonicalAuthInteractionServicedeny path (which cancels the flow first), this leaves the durableAuthFlowrecord non-terminal (Pending/AwaitingUser) until it expires.Severity: low/cleanliness — no security exposure. The run itself terminates correctly. The stale flow is flow-ID-keyed, so it does not block a new auth flow and does not pollute the WebUI pending list (the run is no longer
BlockedAuth). The harm is a dangling storage record plus a confusing error if an out-of-order OAuth callback lands on the dead flow_id.Fix
A narrow
BlockedAuthFlowCancellerport (implemented byRebornProductAuthServicesover its existingflow_record_source+flow_manager) threaded through the sharedcancel_auth_blocked_runhelper, so the flow is cancelled alongside the run at all three call sites:BlockedAutharm,OAuthTargetNotDm).Design notes:
include_terminal: false, so an already-terminal/Completedflow (the OAuth-callback-just-landed race) resolves toNoneand is a no-op.debug!-logged and never blocks the run cancellation (the user-visible terminal action).Option<Arc<dyn …>>and isNonewhen noflow_record_sourceis wired (Slack without product-auth) — the run is still cancelled.AuthFlowManagerwiring; not a gateway handler, so the ToolDispatcher mandate doesn't apply.Tests
blocked_auth_cancels_stale_auth_flow— live observer caller (test-through-the-caller; asserts cancelled run_id + gate).triggered_non_oauth_auth_cancels_stale_auth_flow— triggered non-OAuth arm.triggered_oauth_backstop_cancels_stale_auth_flow— OAuth send-time backstop arm.cancel_blocked_auth_flow_{cancels_non_terminal_flow,is_noop_when_flow_absent,is_noop_without_flow_record_source}— facade unit tests.fmt + clippy clean (
--features slack-v2-host-beta).Review
Local 4-reviewer pass (bugs/security/maintainability/tests). Security: no findings. Bugs/maintainability flagged an empty-string
gate_refsentinel in the backstop arm — fixed by makinggate_refan explicitOption<&str>that skips the flow-cancel when absent. Thebool→visibility-enum generalization remains deferred (tracked separately) and is out of scope here.🤖 Generated with Claude Code