fix(reborn): treat parked Blocked* triggered runs as terminal-for-delivery - #5222
Conversation
…ivery Triggered-run Slack delivery recorded `Failed` for runs that park in `BlockedApproval` / `BlockedAuth` awaiting the user's approval or re-auth and never resolve. After delivering the actionable gate/auth prompt, the delivery loop re-enters `wait_for_actionable_triggered` to handle the eventual transition to `Completed` (deliver the final reply, delete the stale OAuth prompt). When the user never acts — the common case — that re-wait polled to the 30-minute backstop and the wait-error arm recorded a generic `Failed`, clobbering the `Delivered` it had already earned (the outcome store overwrites blindly via `CasExpectation::Any`). Production evidence (logs.1782348290172): 23× "did not finish before Slack delivery timeout" in ~1.5h, all `outcome=Failed`; 21× `BlockedApproval`, 3× `BlockedAuth` (from `secret expired` → `AuthRequired` on google-calendar.list_events). Two runs traced exactly: `BlockedApproval` at 23:15:39 → timeout at 23:45:45 (30m06s); `BlockedAuth` at 23:30:24 → timeout at 00:00:29 (30m05s). Fix: in `deliver_triggered_run`, when the wait times out AND a blocked prompt was already delivered (`delivered_blocked_marker.is_some()`), the run is parked awaiting the user — a successful, terminal-for-delivery outcome. Record `Delivered` and return instead of polling on and recording `Failed`. This mirrors the live-run path's existing `RunWaitTimedOutAfterNotification` quiet- success semantics. The 30-minute backstop is preserved as the failure signal only for runs that never reach an actionable state (still running / stuck). Not a regression: the blocked-delivery machinery (wait_for_actionable_triggered, the 30-min backstop) shipped together in #4948; this is a latent design defect that never handled the never-resolved case. Reuses the existing `Delivered` outcome (documented as "final reply or gate prompt was delivered") — no new outcome variant, no new config, no new state machine. Regression tests (gated --features slack-v2-host-beta), all proven red on the unpatched arm and green after the fix: - triggered_persistent_blocked_approval_records_delivered_not_failed - triggered_running_then_blocked_approval_records_delivered_not_failed - triggered_persistent_blocked_oauth_auth_records_delivered_not_failed - triggered_never_actionable_run_times_out_failed (backstop preserved: zero posts, Failed) Adds a `ScriptedTurnCoordinator::with_states_clamped` helper (sticky final state) and folds the one-off test coordinator into it. Invariant captured as a doc comment on `deliver_triggered_run`. Follow-up (separate track): converge the local parked-state check onto the canonical `TurnStatus::wait_class()` classifier once it lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughTriggered-run Slack delivery now records a timed-out wait as ChangesSlack triggered-run delivery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
risk: low
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves an issue where Slack triggered runs parked awaiting user action (such as approval or re-authentication) were incorrectly marked as Failed upon timing out. The fix treats these parked runs as successfully Delivered once the actionable prompt has been sent. The changes modify the delivery loop in slack_delivery.rs, introduce new test helpers and regression tests, and add a design plan document. There are no review comments on this pull request, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: b9fcb58813
ℹ️ 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".
| Err(SlackFinalReplyDeliveryError::RunWaitTimedOut { .. }) | ||
| if delivered_blocked_marker.is_some() => | ||
| { |
There was a problem hiding this comment.
When the user resolves the approval/auth prompt, the same run can move back to Queued/Running and then exceed max_wait before producing a final reply. wait_for_actionable_triggered keeps polling those non-blocked states and returns the same RunWaitTimedOut, so this guard records Delivered solely because an old gate prompt was delivered, even though the run is no longer parked awaiting the user and no final reply was sent. Track the last observed state or clear the marker once the run leaves the blocked marker so post-resolution stuck runs still surface as failed deliveries.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 2094-2118: The timeout handling in wait_for_actionable_triggered
is too broad because it records Delivered whenever delivered_blocked_marker is
present, even if the run has already transitioned out of Blocked* and later
timed out in Running. Update wait_for_actionable_triggered to return the last
observed state/marker on timeout, and in slack_delivery.rs only take this
quiet-success branch when that last state is still the delivered
BlockedApproval/BlockedAuth marker. Add a regression at the real caller path for
the BlockedApproval -> Running -> timeout case so it proves the final reply is
not misclassified as Delivered.
🪄 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: a76e8834-4dd4-455a-bf9f-7f24eb3ea13d
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rsdocs/plans/2026-06-25-slack-delivery-blocked-terminal.md
| Err(SlackFinalReplyDeliveryError::RunWaitTimedOut { .. }) | ||
| if delivered_blocked_marker.is_some() => | ||
| { | ||
| // The run is parked in a Blocked* state (awaiting the user's | ||
| // approval or re-auth) AFTER we already delivered its actionable | ||
| // gate/auth prompt. This is the common, expected case — the user | ||
| // simply has not acted within the wait backstop. The prompt is | ||
| // out; the user's resolution arrives later as a separate inbound | ||
| // event and is bridged back via the delivered-gate route. Treat | ||
| // this as a successful, terminal-for-delivery outcome instead of | ||
| // polling to the backstop and recording a generic `Failed` (which | ||
| // also clobbers the `Delivered` we already earned). Mirrors the | ||
| // live-run path's `RunWaitTimedOutAfterNotification` "quiet | ||
| // success" semantics. The auth prompt must remain actionable, so | ||
| // we intentionally do NOT delete `messages_to_delete_after_final` | ||
| // here — they are cleaned up only on a real terminal final reply. | ||
| tracing::debug!( | ||
| target = "ironclaw::reborn::slack_delivery", | ||
| %run_id, | ||
| "triggered run parked awaiting user after delivering blocked prompt; recording Delivered" | ||
| ); | ||
| let outcome = TriggeredRunDeliveryOutcomeKind::Delivered; | ||
| record_triggered_run_outcome(delivery_store, run_id, outcome).await; | ||
| return outcome; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t classify every post-gate timeout as “parked awaiting user.”
This branch keys only on delivered_blocked_marker.is_some(). If the user approves/re-auths, the run leaves Blocked*, returns to Running, and then exceeds max_wait, we still record Delivered and return even though the final reply was never posted. The quiet-success path needs the timed-out state to still be the same blocked marker, not just “we once delivered a gate”.
Please have wait_for_actionable_triggered surface the last observed state/marker on timeout and only take this arm when the last state is still that delivered BlockedApproval/BlockedAuth. Add a caller-level regression for BlockedApproval -> Running -> timeout too. As per path instructions, "Test through the caller: when a helper gates a side effect, require a test driving the real call site (handler/factory/manager), not only the helper."
🤖 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_delivery.rs` around lines 2094 -
2118, The timeout handling in wait_for_actionable_triggered is too broad because
it records Delivered whenever delivered_blocked_marker is present, even if the
run has already transitioned out of Blocked* and later timed out in Running.
Update wait_for_actionable_triggered to return the last observed state/marker on
timeout, and in slack_delivery.rs only take this quiet-success branch when that
last state is still the delivered BlockedApproval/BlockedAuth marker. Add a
regression at the real caller path for the BlockedApproval -> Running -> timeout
case so it proves the final reply is not misclassified as Delivered.
Source: Path instructions
|
🚅 Deployed to the ironclaw-pr-5222 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Treat parked blocked triggered runs as terminal-for-delivery successes so Slack delivery records Delivered instead of Failed after prompts are posted.
Stats: 1 finding (from 2 raw, 1 after live-thread dedupe) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Existing unresolved review threads already cover the High correctness issue where a post-gate timeout is treated as Delivered even if the run resumed; I did not repost that duplicate.
Tests
- Medium Parked-timeout cleanup is not asserted (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2094-2116, confidence 74) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:2094
The new timeout-after-blocked-prompt branch only asserts the final outcome. It never verifies that the already-posted approval/auth prompt stays in place, so a regression that issueschat.deleteor otherwise strips the actionable prompt would still pass the current tests even though the run is supposed to remain awaiting the user.
| .await | ||
| { | ||
| Ok(s) => s, | ||
| Err(SlackFinalReplyDeliveryError::RunWaitTimedOut { .. }) |
There was a problem hiding this comment.
Medium — Parked-timeout cleanup is not asserted.
The new timeout-after-blocked-prompt branch only asserts the final outcome. It never verifies that the already-posted approval/auth prompt stays in place, so a regression that issues chat.delete or otherwise strips the actionable prompt would still pass the current tests even though the run is supposed to remain awaiting the user.
Fix: Add tests::slack_delivery::triggered_blocked_timeout_keeps_actionable_prompt covering parked BlockedApproval/BlockedAuth timeout without any chat.delete calls.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Fix triggered Slack delivery so parked blocked runs count as delivered instead of failed once a gate or auth prompt has already been posted.
Stats: 0 findings (from 0 raw, 0 after dedup) across 0 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
All 8 isolated gpt-5.4-mini reviewers returned clean JSON arrays for this head.
I did not duplicate the existing live unresolved threads on crates/ironclaw_reborn_composition/src/slack_delivery.rs. Those still need resolution separately before this PR is merge-ready:
- Existing timeout-state concern at lines 2096/2118: only record
Deliveredif the timed-out run is still parked in the same blocked marker after the prompt was delivered. - Existing test gap at line 2094: assert the parked timeout path leaves the already-posted actionable prompt in place and does not issue
chat.delete.
This branch was successfully deployed
1 participant
Verified bug (log evidence)
Triggered-run Slack delivery records
Failedfor runs that park inBlockedApproval/BlockedAuth(awaiting the user's approval or re-auth) and never resolve.From
logs.1782348290172:WARN slack_delivery: triggered run wait failed ... did not finish before Slack delivery timeout, all withoutcome=Failed(zeroDelivered/Skipped).BlockedApproval, 3×BlockedAuth(the latter fromlease_once failed err=secret expired→error_kind="AuthRequired"ongoogle-calendar.list_events).58ddc152:status=BlockedApprovalat23:15:39→RunWaitTimedOutat23:45:45(30m06s) →Failed.9461329b:status=BlockedAuthat23:30:24→RunWaitTimedOutat00:00:29(30m05s) →Failed.chat.postMessageevents anywhere in the log.Mechanism (reproduced)
deliver_triggered_runposts the actionable gate/auth prompt, setsdelivered_blocked_marker, andcontinues — re-enteringwait_for_actionable_triggeredto handle the eventual transition toCompleted(deliver the final reply, delete the stale OAuth prompt). When the user never acts (the common case), the run stays in the same blocked state, so the marker never changes and the re-wait polls tomax_wait(30 min) →RunWaitTimedOut. The wait-error arm then recorded a genericFailed, clobbering theDeliveredit had already earned (record_triggered_run_deliveryusesCasExpectation::Any, a blind overwrite).A repro test (
ScriptedTurnCoordinatorreturning a stickyBlockedApproval,max_wait=200ms) confirmed: prompt posted once, then re-wait timeout →Failed.Regression determination
Not a classic regression. The blocked-delivery machinery (
wait_for_actionable_triggered, the 30-minDEFAULT_TRIGGERED_RUN_DELIVERY_MAX_WAIT) all shipped together in #4948. No prior version delivered parked runs correctly and then broke — this is a latent design defect present since the feature shipped: the loop waits for a blocked run to change state but never handled the never-resolved case.The fix
In
deliver_triggered_run, when the wait times out and a blocked prompt was already delivered (delivered_blocked_marker.is_some()), the run is parked awaiting the user — a successful, terminal-for-delivery outcome. RecordDeliveredand return instead of polling to the backstop and recordingFailed. This mirrors the existing live-run path'sRunWaitTimedOutAfterNotificationquiet-success semantics.delivered_blocked_marker.is_none()).None→Skipped; delivery errors →NoDefaultConfigured/Denied/Failedimmediately, no poll) — unchanged.Deliveredoutcome (documented as "final reply or gate prompt was delivered"). No new outcome variant, no new config, no new state machine.Tests (red → green, gated
--features slack-v2-host-beta)All four proven RED on the unpatched arm (
got Failed, expectedDelivered) and GREEN after the fix:triggered_persistent_blocked_approval_records_delivered_not_failedBlockedApproval→ prompt posted, outcomeDeliveredtriggered_running_then_blocked_approval_records_delivered_not_failedRunning→stickyBlockedApproval(production-faithful) →Deliveredtriggered_persistent_blocked_oauth_auth_records_delivered_not_failedBlockedAuth+OAuth → URL posted to DM, outcomeDeliveredtriggered_never_actionable_run_times_out_failedRunning→ zero posts,Failed(backstop preserved)Added
ScriptedTurnCoordinator::with_states_clamped(sticky final state) and folded a one-off test coordinator into it (review feedback).Quality gate
cargo fmt --all: clean (onlyslack_delivery.rstouched).cargo clippy -p ironclaw_reborn_composition --all-targets --features slack-v2-host-beta: zero warnings.cargo test -p ironclaw_reborn_composition --lib --features slack-v2-host-beta -- slack_delivery::tests: 78 passed, 0 failed.factory.rsappear without the feature (not mine); the crate's full suite is flaky under max test-thread parallelism on both base and this branch (different test / SIGSEGV per run) — green deterministically at--test-threads=4(1207 passed).Review
Ran the multi-agent
/code-review(8 reviewers) andthermo-nuclear-code-quality-review. Findings addressed: added the missingBlockedAuth/OAuth regression test; collapsed the duplicate test coordinator into a canonicalwith_states_clampedhelper. Thermo-nuclear verdict: PASS — single match arm in an existing match, no spaghetti; the one structural improvement (unify the quiet-success classification across both wait paths) is correctly deferred.File lane
Touches only
crates/ironclaw_reborn_composition/src/slack_delivery.rs(+ its tests) and the plan doc. No edits toironclaw_turns/src/status.rs,ironclaw_reborn/src/runtime.rs,turn_scheduler.rs, or any config.Follow-up (separate track)
Converge the local parked-state check onto the canonical
TurnStatus::wait_class()classifier once it lands (and consider unifying the triggered + live-run wait paths' quiet-success handling onto the sharedRunWaitTimedOutAfterNotificationvariant).🤖 Generated with Claude Code