fix(reborn): deliver triggered Slack runs after settlement - #5318
Merged
Merged
Conversation
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5318
June 26, 2026 10:59
Destroyed
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAccepted-fire settlement now flows through a dedicated observer before Slack delivery. Trigger worker wiring exposes the observer dependency and tests it. Postgres fire acceptance/replay now uses explicit success outcomes, with regression coverage for non-canonical RFC3339 slot text. ChangesFire settlement and acceptance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the trigger poller mechanism to stage post-submit delivery hooks in-memory and only dispatch them once the corresponding trigger fire is durably settled in storage, as indicated by the tick report. This ensures that Slack delivery does not precede the persisted run/thread mapping. Additionally, tests have been updated and a new test has been added to verify that pending hooks are correctly dropped if settlement fails. I have no feedback to provide on these changes.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/trigger_poller.rs`:
- Around line 564-570: Add a regression test that exercises the real trigger
poller entry path through run_trigger_poller(), not just
dispatch_settled_submits() directly. The current coverage around build_wrapper
and the helper tests is too isolated, so wire the test through the actual
caller/manager path that reaches the new production hook setup and verifies
Slack delivery is still triggered end-to-end. Use the existing
run_trigger_poller(), build_wrapper(), and PostSubmitHookWrappedSubmitter flow
as the locating symbols when updating the suite.
- Around line 194-199: The trigger_poller bookkeeping log in the
dropped_unsettled branch is too noisy for REPL/TUI use and should not use warn!.
Update the diagnostic inside trigger_poller’s dropped_unsettled check to debug!
(or replace it with a counter/metric) while preserving the existing target and
context so the internal background-task event stays out of the user-facing
terminal UI.
🪄 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: 063c9297-3792-42c9-bb12-358d59e0d734
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/trigger_poller.rs
|
🚅 Deployed to the ironclaw-pr-5318 environment in ironclaw-ci-preview
|
serrrfirat
force-pushed
the
codex/post-settlement-slack-delivery
branch
from
June 26, 2026 12:11
3d3b2ee to
2808768
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5318
June 26, 2026 12:11
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_triggers/src/postgres.rs (1)
1066-1081: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep libSQL in sync with this fix.
This removes the
active_fire_slottext match only for Postgres.crates/ironclaw_triggers/src/libsql.rs:1525-1558still updates withWHERE ... active_fire_slot = ?4 AND active_run_ref IS NULL, so the same equivalent-RFC3339 case can still strand a claimed fire there and leave backend behavior divergent for the same repository contract. As per coding guidelines, "Support both PostgreSQL and libSQL for persistence behavior where applicable."🤖 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_triggers/src/postgres.rs` around lines 1066 - 1081, The libSQL update path still uses the old active_fire_slot text equality check, so it can diverge from the Postgres fix and strand claimed fires. Update the corresponding logic in the libsql.rs persistence/update flow that writes last_run_at, last_fired_slot, last_status, next_run_at, active_fire_slot, and active_run_ref so it matches the Postgres behavior of updating by primary key instead of matching on active_fire_slot. Preserve the same claim semantics for equivalent RFC3339 encodings and keep the backend behavior aligned across PostgreSQL and libSQL.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_triggers/src/postgres.rs`:
- Around line 1066-1081: The libSQL update path still uses the old
active_fire_slot text equality check, so it can diverge from the Postgres fix
and strand claimed fires. Update the corresponding logic in the libsql.rs
persistence/update flow that writes last_run_at, last_fired_slot, last_status,
next_run_at, active_fire_slot, and active_run_ref so it matches the Postgres
behavior of updating by primary key instead of matching on active_fire_slot.
Preserve the same claim semantics for equivalent RFC3339 encodings and keep the
backend behavior aligned across PostgreSQL and libSQL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8d16de0d-6fbe-44c1-98b1-cbccbd85e979
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_triggers/src/postgres.rscrates/ironclaw_triggers/tests/repository_contract.rs
serrrfirat
force-pushed
the
codex/post-settlement-slack-delivery
branch
from
June 26, 2026 12:38
2808768 to
dcde760
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5318
June 26, 2026 12:38
Destroyed
github-actions
Bot
added
size: XL
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/trigger_poller.rs`:
- Around line 75-77: The late-bound hook behavior in trigger_poller.rs is not
guaranteed, so accepted fires can be skipped during startup when hook_slot.get()
is still empty. Fix this by either wiring the hook before calling
spawn_trigger_poller or by buffering/replaying settlements until the hook
exists, and update the settlement handling path that currently drops the fire
when the slot is empty. Make sure the logic around spawn_trigger_poller,
hook_slot.get, and the accepted fire enqueue path enforces the startup guarantee
rather than relying on the comment.
In `@crates/ironclaw_triggers/src/worker/tests.rs`:
- Around line 313-373: The test in
tick_notifies_settlement_observer_after_accepted_fire_persists only checks
settlement state after tick_once() completes, so it does not verify the
notify-after-persistence ordering. Move the active_run_ref and run-history
assertions into RecordingSettlementObserver’s callback path (the
fire_settlement_observer invoked by TriggerPollerWorker::tick_once) so the
observer can confirm the trigger has already been marked accepted before
on_accepted_fire_settled runs. Keep the regression test driving the real worker
call site and use the existing identifiers TriggerPollerWorker,
RecordingSettlementObserver, and tick_once to locate the flow.
In `@crates/ironclaw_triggers/tests/repository_contract.rs`:
- Around line 3704-3780: The new regression test only covers the caller path for
Postgres accepted fires, but this fix also affects the replayed flow, so add an
equivalent caller-level regression for mark_fire_replayed in
repository_contract.rs or convert the test into a table-driven case covering
both FireAcceptedRequest and FireReplayedRequest. Reuse the existing
PostgresTriggerRepository, claim_due_fire, and the manual active_fire_slot
rewrite to a non-canonical but equivalent timestamp string, then assert
mark_fire_replayed settles correctly just like mark_fire_accepted. Keep the test
at the repository caller level and ensure the new case verifies the same
timestamp-text normalization behavior end to end.
🪄 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: 78a3f6d1-de1e-411a-a071-3c3a26d274f6
📒 Files selected for processing (11)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_triggers/src/lib.rscrates/ironclaw_triggers/src/postgres.rscrates/ironclaw_triggers/src/worker.rscrates/ironclaw_triggers/src/worker/config.rscrates/ironclaw_triggers/src/worker/due_fire.rscrates/ironclaw_triggers/src/worker/ports.rscrates/ironclaw_triggers/src/worker/tests.rscrates/ironclaw_triggers/tests/repository_contract.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5318
June 26, 2026 13:12
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_reborn_composition/src/runtime.rs (1)
1326-1350: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc still describes the removed
OnceLockslot.Body now drives
PostSubmitHookDispatch, but the doc above (Lines 1330-1332) still says "slot isNone" / "slot is already occupied". Re-word to the dispatcher semantics.📝 suggested doc fix
- /// idempotent: a second call is silently ignored. Returns `false` when the - /// trigger poller is not enabled (slot is `None`) or the slot is already - /// occupied, `true` on first successful set. + /// idempotent: a second call is silently ignored. Returns `false` when the + /// trigger poller is not enabled (no dispatcher) or a hook is already + /// installed, `true` on first successful install.As per coding guidelines: "When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change."
🤖 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/runtime.rs` around lines 1326 - 1350, Update the doc comment for set_trigger_post_submit_hook to match the current PostSubmitHookDispatch behavior instead of the removed OnceLock slot semantics. Reword the return-value description so it refers to the dispatcher not being enabled and the hook already being installed, using the same terminology as install_hook and post_submit_hook_dispatch. Keep the idempotent behavior note, but remove references to “slot is None” or “slot is already occupied.”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_reborn_composition/src/trigger_poller.rs`:
- Around line 170-185: The `TriggerPoller::dispatch_or_buffer` path currently
buffers into `state.pending` without any limit, so startup delays can cause
unbounded growth; update the pending storage in `TriggerPoller` to enforce the
same 256-item cap as `SlackFinalReplyDeliverySettings::max_pending_deliveries`,
drop the oldest queued `TriggerAcceptedFireSettlement` when full, and emit a
`debug!` message when that happens. Also revise the startup-window comment near
`set_trigger_post_submit_hook` / `dispatch_or_buffer` to document the bounded
guarantee so it’s clear how pending delivery is handled before the hook is
installed.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1326-1350: Update the doc comment for set_trigger_post_submit_hook
to match the current PostSubmitHookDispatch behavior instead of the removed
OnceLock slot semantics. Reword the return-value description so it refers to the
dispatcher not being enabled and the hook already being installed, using the
same terminology as install_hook and post_submit_hook_dispatch. Keep the
idempotent behavior note, but remove references to “slot is None” or “slot is
already occupied.”
🪄 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: a30eef55-5cc2-4b81-9c82-d3baf508bffc
📒 Files selected for processing (8)
crates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_triggers/src/libsql.rscrates/ironclaw_triggers/src/worker/due_fire.rscrates/ironclaw_triggers/src/worker/ports.rscrates/ironclaw_triggers/src/worker/tests.rscrates/ironclaw_triggers/tests/repository_contract.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_reborn_composition/src/slack_delivery.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5318
June 26, 2026 13:42
Destroyed
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 26, 2026
This was referenced Jul 8, 2026
19 of 29 tasks
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
SubmittedRoot cause
PR #5202 correctly detached Slack delivery from the poller tick, but the hook still fired immediately after
TrustedTriggerFireSubmitOutcome::Accepted. That meant Slack could deliver beforemark_fire_acceptedcommittedrun_id/thread_idand cleared the claim-only state. If settlement failed, users saw Slack delivery while Postgres stayed stuck withactive_fire_slotset andtrigger_run_history.run_id/thread_idnull, blocking later fires and WebUI thread access.The WebUI fallback cannot repair that state: it authorizes an already-known trigger thread, but the Automations panel only gets an openable chat link from the persisted
recent_runs[].thread_id.Postgres also had a backend-specific fragility: after locking and parsing the trigger row, the accepted-fire update repeated text predicates on
active_fire_slotandnext_run_at. Equivalent timestamp text encodings could make the SQL update return no row even though the locked record represented the claimed fire. The fix relies on the existingFOR UPDATElock plus parsed validation, then updates by primary key inside the same transaction.Tests
cargo test -p ironclaw_reborn_composition --features slack-v2-host-beta hook_wrapper --lib -- --nocapturecargo check -p ironclaw_reborn_compositioncargo test -p ironclaw_reborn_composition --test trigger_poller_e2e builtin_created_recurring_trigger_fires_again_after_first_run_settles -- --nocapturecargo test -p ironclaw_triggers --features postgres --test repository_contract postgres_repository_mark_fire_accepted_settles_equivalent_active_fire_timestamp_text -- --nocapture(compiled; skipped runtime because Docker/testcontainers unavailable locally)cargo check -p ironclaw_triggers --features postgresgit diff --checkDatabase impact
No schema or migration changes. This changes settlement update behavior and hook dispatch ordering only. Existing stuck rows still need operational cleanup or successful recovery.