fix(run_delivery): deliver triggered run failures to the creator (#6896) - #7131
Conversation
Scheduled/triggered runs that ended in Failed, Cancelled, or RecoveryRequired produced no user-visible notification: the triggered delivery driver minted notifications only for Completed / BlockedApproval / BlockedAuth and recorded every other terminal status as Skipped. A run that timed out before reaching an actionable state only logged a warn and recorded Failed, leaving the creator in silence. Delivery: - triggered_notification_for_state now mints a FinalReplyReady notification for Failed and RecoveryRequired using the existing per-category failure summaries (reborn_failure_summary_for_category) over state.failure.category(), with a generic fallback when no category is present. - Cancelled mints the same notification, preferring a failure-category summary when one is present and falling back to a fixed cancellation notice otherwise. - The RunWaitTimedOut branch with no prior blocked marker now delivers the timeout notice as a terminal reply instead of recording Failed. - The wildcard arm is replaced with explicit non-actionable statuses (Queued, Running, CancelRequested, BlockedResource, BlockedDependentRun, BlockedExternalTool) so a future status fails to compile rather than silently skipping. Observer: - TriggerFireSettlementObserver gains on_failed_fire_settled as a default no-op method, plus a TriggerFailedFireSettlement event carrying tenant/trigger/fire-slot/run-id/history-status. Noop and existing implementors keep compiling. - The active-cleanup sweep fires on_failed_fire_settled when clear_active_fire succeeds with TriggerRunHistoryStatus::Error, so post-accept failures are observable for automation health. Ok, Running, and already-cleared fires do not fire the hook. Tests: - run_delivery_contract: Failed+model_error, Failed without category, Cancelled, and timeout-before-actionable all assert a Delivered outcome with the expected notice text and footer. - worker tests: a terminal-Error active fire fires exactly one on_failed_fire_settled; a terminal-Ok active fire fires none. The larger retry/redrive budget for failed post-accept fires (retry_disposition has zero production callers) is intentionally left for a follow-up; it is out of scope for this surgical delivery fix.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the pr-a7ad01-7131 environment in ironclaw-ci-preview
|
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTriggered runs now deliver terminal failure, recovery-required, cancellation, and timeout notices. Active trigger cleanup emits failed-run settlement events after newly clearing terminal errors without invoking post-submit delivery. ChangesTriggered failure delivery
Trigger settlement reporting
Composition budget
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TriggeredRunDelivery
participant TurnRunState
participant PreferenceTarget
TriggeredRunDelivery->>TurnRunState: wait for terminal or timeout state
TurnRunState-->>TriggeredRunDelivery: failure, cancellation, or timeout
TriggeredRunDelivery->>PreferenceTarget: deliver terminal notice
sequenceDiagram
participant ActiveCleanup
participant TriggerFireSettlementObserver
participant TriggerPoller
ActiveCleanup->>TriggerFireSettlementObserver: report failed settlement identifiers
TriggerFireSettlementObserver->>TriggerPoller: log failed settlement warning
TriggerPoller-->>ActiveCleanup: do not invoke post-submit delivery
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: L
🔎 Review · PR #7131
The target changed before this Run could finish. Automatic · PR opened + CI failed · attempt 0 of 3 · cancelled after <1s Run details
|
![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_product/tests/run_delivery_contract.rs`:
- Around line 2138-2228: Add caller-level tests alongside the existing triggered
delivery tests for RecoveryRequired and Cancelled with Some(SanitizedFailure),
using on_trigger_submitted as the exercised seam. For each branch, assert
Delivered, the branch-specific failure summary, the triggered footer, and
routing to the decoded creator preference target, reusing the existing harness
and assertion patterns.
🪄 Autofix
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: 2a55030e-f3f7-46e9-bf6b-82488d69a8a4
📒 Files selected for processing (8)
crates/ironclaw_product/src/run_delivery/prompts.rscrates/ironclaw_product/src/run_delivery/triggered.rscrates/ironclaw_product/tests/run_delivery_contract.rscrates/ironclaw_triggers/src/lib.rscrates/ironclaw_triggers/src/worker.rscrates/ironclaw_triggers/src/worker/active_cleanup.rscrates/ironclaw_triggers/src/worker/ports.rscrates/ironclaw_triggers/src/worker/tests.rs
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Deliver sanitized creator notifications when triggered or scheduled runs fail, require recovery, are cancelled, or time out.
Shape: normal with no modifiers — 8 files / 477 changed lines, no generated, mechanical, mega, or stacked characteristics.
Coverage: complete via exact local-git diff. Buckets: 7 production files, 1 test file. Packetization not needed. All 8 reviewers completed with no limitations.
Stats: 5 findings from 9 raw findings after overlap dedup, across 3 files. Reviewers run: Security, Bugs, Performance/Concurrency, Tests, Conventions, Local Patterns, Maintainability, Approach. Reviewers failed: none. Body-only: 0.
Conventions / correctness
- Medium Default hook silently drops every production failure settlement (
crates/ironclaw_triggers/src/worker/ports.rs:146, confidence 100) — anchor:AGENTS.md:186. The production observer inherits the no-op, so the worker's new health event is discarded. Also flagged by Bugs, Local Patterns, Maintainability, and Approach.
Tests
- Medium RecoveryRequired terminal delivery has no contract test (
crates/ironclaw_product/src/run_delivery/triggered.rs:782-800, confidence 100) — the shared terminal arm is only exercised withFailed. - Low Categorized cancellation summary branch is untested (
crates/ironclaw_product/src/run_delivery/triggered.rs:810-814, confidence 100) — only the uncategorized fallback is exercised. - Medium Timeout notice failure mapping is happy-path-only (
crates/ironclaw_product/src/run_delivery/triggered.rs:389-403, confidence 100) — no test proves missing-target mapping toNoDefaultConfigured. - Medium Failed-fire observer is not tested against a lost clear race (
crates/ironclaw_triggers/src/worker/active_cleanup.rs:169-179, confidence 100) — duplicate suppression after a lost clear race is not pinned.
Security and performance/concurrency found no additional issues.
# Conflicts: # crates/product/ironclaw_assistant/src/run_delivery/triggered.rs
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Railway preview QA — BLOCKED (fallback-target journey executed)Tested head SHA: Retest with the fallback delivery targetThe preview's only available delivery default is the fallback "Web app only (no external delivery)" target (no channel extension is installed or configured). This fallback was verified as the active/selected state on
Live fallback-target journey (executed through real UI + deployment)
Status derivation
Hermetic Reborn integration suite (deterministic, offline, full product path)Run on PR head
Total deterministic coverage: 269 tests passing on PR head (33 contract + 184 triggers + 52 hermetic integration), including the delivered-notice branch the live preview cannot run. Regression resultDelivered-notice branch: not verifiable in preview (no channel extension/credentials). Recording + no-target branches: verified live against the fallback target — a failing triggered run was delivered-skipped with an explicit logged reason, recorded as Failed in run history (UI-visible), and settled by the poller with no replacement run. Remaining risk: actual outbound delivery (sanitized summary + triggered footer) to a real channel, covered by the deterministic contract suite above. Skipped cases
CleanupTest automation |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Deliver sanitized terminal notices to the creator's preference target for triggered runs that fail, require recovery, are cancelled, or time out, instead of recording them silently as Skipped.
Shape: primary mode normal; no modifiers. 10 changed files (8 production, 1 test, 1 docs), 653 changed lines (628+/25-), diff source local-git (7a2e7bf...f6cdf615).
Coverage: complete — all 8 reviewers read all 3 packets through EOF (production-001, tests-001, docs-001), packet hashes verified against manifest, worktree at head f6cdf6158e7de5927699f54afddac664bcaabaec. Reviewers failed: none. Body-only: 0.
Stats: 10 findings (from 10 raw, 5 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach.
Bugs (3)
- Low Timeout arm delivers timeout copy to runs mid-cancel/failure; terminal copy never sent (
crates/product/ironclaw_assistant/src/run_delivery/triggered.rs:354-406, confidence 60) — anchor: triggered.rs:354
The newRunWaitTimedOutarm fires for any run not terminal/actionable within max_wait, includingCancelRequestedor about-to-fail runs; it deliversDELIVERY_TIMEOUT_MESSAGEand the watcher exits, so the eventualCancelled/Failedterminal copy is never sent. Small window, strictly better than silence, but the recordedDeliveredoutcome claims final delivery while the run is still non-terminal. Fix: short grace-period watch after the timeout notice (or suppress when the state is alreadyCancelRequested). - Low Cancelled-with-category arm unreachable in production; failure copy would mislabel a cancel (
triggered.rs:800-831, confidence 55) — anchor: triggered.rs:800
Cancelled runs carry noSanitizedFailurein the real system (failure is attached only toFailed/RecoveryRequiredexits), so theSome(category)branch is exercised only by the scripted contract test. If a future cancel path attaches a failure, the delivered copy would mislabel an operator cancel as a failure and instruct retry. Fix: restrict to cancel-specific categories or drop the branch. - Nit Final
Ok(None)arm comment promises silence for blocked states the timeout arm actually notifies (triggered.rs:826-842, confidence 75) — anchor: triggered.rs:833
wait_for_actionable_statenever returns in-flight/blocked states to this function; they hit the timeout arm instead, so "stay silent / records Skipped" describes behavior the timeout arm overrides. Fix: comment as exhaustiveness-only.
Tests (1)
- Low Timeout-notice arm's Denied/Other outcome branches untested (
triggered.rs:395-400, confidence 60) — anchor: triggered.rs:395
OnlyDeliveredandNoDefaultConfiguredtimeout outcomes are asserted. TheDeniedbranch is reachable via the existing project-scoped denial fixture, andOther -> Failedvia a failed delivery report. These determine the recorded outcome kind, the core observable #6896 changes. Fix: two new contract scenarios (..._records_denied,..._records_failed).
Maintainability (2)
- Medium Timeout arm triplicates failure-to-outcome mapping and final-notice literal (
triggered.rs:375-406, confidence 75) — anchor: triggered.rs:388
The failure→outcome match now exists three times indeliver_triggered_run(timeout arm, OAuth-backstop arm, generic Err arm) and the final-reply literal six times; the timeout arm also hand-rebuilds the 7-field context the loop already constructs. Any future outcome-kind change must be applied in three places. Fix: extractdeliver_terminal_notice+final_reply_noticehelpers and route all arms through them (~35 lines deleted). (Also flagged by: bugs, tests, conventions) - Low Failed/RecoveryRequired and Cancelled arms identical except fallback text (
triggered.rs:780-833, confidence 55) — anchor: triggered.rs:780-833
Two adjacent arms differing by one constant. Fix: merge into one arm with per-status fallback selection.
Conventions (2)
- Low
deliver_triggered_runinvariant doc now contradicts new timeout arm semantics (triggered.rs:273-278, confidence 60) — anchor: AGENTS.md "Update the owning contract/docs when behavior changes"
Doc still claims the backstop is recorded as a failure signal; the new timeout arm recordsDelivered/NoDefaultConfigured/Deniedand only delivery failure recordsFailed. Fix: update the invariant doc. - Nit Timeout arm adds a third copy of the failure→outcome mapping match (
triggered.rs:391-400, confidence 50) — anchor: triggered.rs:558-567, :589-596
Duplicate of the OAuth-backstop and generic Err mappings. Fix: extractdelivery_outcome_kindhelper.
Local Patterns (1)
- Low Stale doc count: "Only three outputs" but five bullets in triggered surface contract (
triggered.rs:611-621, confidence 90) — anchor: triggered.rs:611-621
The diff added two terminal-output bullets without updating the lead-in count. Fix: drop the count or update to five.
Performance (1)
- Low Inline await of
on_failed_fire_settledstalls active-cleanup sweep (crates/domains/ironclaw_triggers/src/worker/active_cleanup.rs:169-178, confidence 60) — anchor: active_cleanup.rs:169
The new hook is awaited inline in the sweep loop; the siblingon_accepted_fire_settledpath deliberately decouples via bounded spawn. Today's impls are cheap, but the trait is open — a future heavy observer stalls cleanup for the whole tick. Fix: bounded spawn (mirroring the sibling path) or document the cheap-observer contract.
Security (0) / Approach (0)
No findings. Security verified: static sanitized-summary table only (validated category, no injection path), scope+actor enforcement on target resolution, settlement fires only after durable clear, no production unwraps added. Approach: root cause of #6896 confirmed in base; PR reuses existing facilities (reborn_failure_summary_for_category, observer port, notification plumbing); all implementors enumerated; docs updated.
| let mut delivered_blocked_marker: Option<BlockedActionableMarker> = None; | ||
| let mut messages_to_delete_after_final: Vec<DeliveredChannelMessage> = Vec::new(); | ||
| // The trigger label is stable for the whole run; compute it once so the | ||
| // timeout-with-no-marker arm can build a terminal notice without waiting |
There was a problem hiding this comment.
Medium — Timeout arm triplicates failure-to-outcome mapping and final-notice literal.
The new RunWaitTimedOut arm hand-builds a FinalReplyReady/TriggeredDelivery TriggeredNotification literal and repeats the deliver_triggered_notification failure->outcome match. That outcome match now exists three times in deliver_triggered_run (timeout arm, OAuth-backstop arm, generic Err arm) and the final-reply literal shape exists six times (new timeout, Failed/RecoveryRequired, Cancelled, plus pre-existing Completed, unserviceable-auth, OAuth-backstop arms). The timeout arm also hand-rebuilds the 7-field TriggeredNotificationContext that the loop body already constructs. A reader must diff the literals to confirm they are identical, and any future change to the mapping must be applied in three places.
Fix: Extract deliver_terminal_notice(services, ctx, text, trigger_label) -> TriggeredRunDeliveryOutcomeKind that builds the final notice (with a final_reply_notice(text, trigger_label, attachments, intent) constructor), delivers it, and maps TriggeredNotificationFailure to TriggeredRunDeliveryOutcomeKind once. Route the timeout arm, the OAuth-backstop arm, and the generic Err arm through it. Net deletion: ~35 lines of repeated match + literal.
Also flagged by: bugs/Low, tests/Low, conventions/Nit
| authority: &authority, | ||
| }; | ||
| let notice = TriggeredNotification { | ||
| event_kind: RunNotificationEventKind::FinalReplyReady, |
There was a problem hiding this comment.
Nit — Final Ok(None) arm comment promises silence for blocked states the timeout arm actually notifies.
The trailing TurnStatus::Queued | Running | CancelRequested | BlockedResource | BlockedDependentRun | BlockedExternalTool => Ok(None) arm's comment says these states stay silent here and are surfaced through the WebUI, and Returning None records Skipped. But wait_for_actionable_state never returns any of these states to triggered_notification_for_state — it only returns terminal states or newly-blocked-actionable states. Non-actionable blocked and in-flight runs instead hit the RunWaitTimedOut arm and receive the timeout notice with a Delivered/NoDefaultConfigured/Denied/Failed outcome — never Skipped. The arm is compile-required exhaustiveness only; the comment describes behavior the timeout arm overrides.
Fix: Update the comment to state that these states never reach this function in the triggered loop (exhaustiveness only) and that they are handled by the timeout arm.
Also flagged by: bugs/Low, maintainability/Low
| use async_trait::async_trait; | ||
| use chrono::Utc; | ||
| use ironclaw_extension_contracts::channel_adapter::OutboundPart; | ||
| use ironclaw_host_api::failure::summary::reborn_failure_summary_for_category; |
There was a problem hiding this comment.
Low — deliver_triggered_run invariant doc now contradicts new timeout arm semantics.
Doc block on deliver_triggered_run still states the backstop is the failure signal ONLY for runs that never reached an actionable state at all, distinguished by delivered_blocked_marker. The new RunWaitTimedOut-without-marker arm now delivers the timeout notice and records Delivered/NoDefaultConfigured/Denied when the channel is reachable, so the backstop is no longer recorded as a failure signal for never-actionable runs; Failed is recorded only when the notice delivery itself fails. The invariant doc predates the change and was not updated.
Fix: Update the invariant doc block: the never-actionable backstop now delivers a terminal timeout notice and records the delivery outcome, not Failed.
| /// - `BlockedAuth` → auth prompt (OAuth link) or, for non-OAuth, a | ||
| /// cancel + final-reply carrying the auth-unavailable notice | ||
| /// - `Completed` → final reply | ||
| /// - `Failed` / `RecoveryRequired` → final reply carrying the per-category |
There was a problem hiding this comment.
Low — Stale doc count: 'Only three outputs' but five bullets in triggered surface contract.
The diff extended the triggered_notification_for_state doc comment with two new terminal-output bullets (Failed/RecoveryRequired, Cancelled) but left the lead-in 'Only three outputs are minted here:' counting the pre-change three bullets (BlockedApproval, BlockedAuth, Completed). The comment now contradicts the code it documents; the staleness was introduced by this change.
Fix: Reword the lead-in so it cannot go stale with future arms: drop the count or update it to five.
| fire_slot, | ||
| run_id, | ||
| }) | ||
| .await; |
There was a problem hiding this comment.
Low — Inline await of on_failed_fire_settled stalls active-cleanup sweep.
The new failure-settlement hook is awaited inline inside the per-record sweep loop, between the durable clear_active_fire and report/cursor advancement. Today's impls are cheap (production logs one warn; Noop does nothing; tests push to a Mutex), and the loop already awaits backend I/O per record, so marginal latency is negligible. But the trait is async and open (Send+Sync, any implementor); the sibling on_accepted_fire_settled path deliberately decouples via bounded spawn_post_submit_delivery + bounded buffer, while this path couples sweep latency and scan-cursor progress to observer behavior. A future heavy observer (e.g., network telemetry sink) would stall active-fire cleanup for the whole tick. No lock is held across the await, so no deadlock is introduced.
Fix: Detach the observer call with a bounded spawn (mirroring on_accepted_fire_settled), or harden the trait contract docs to require cheap non-blocking observers.
- Extract shared terminal-notice helpers (final_reply_notice, outcome_for_delivery_failure, deliver_terminal_notice) so the timeout, OAuth-backstop, and generic failure arms share one notice shape and outcome taxonomy instead of a third hand-rolled copy. - Add a bounded race-grace window after the wait backstop: a run that crosses into a terminal state during the final wait (cancellation in flight, failure landing after the last poll) now delivers the correct terminal notice instead of the timeout copy. - Cancelled runs always deliver the fixed cancellation notice; the failure-category branch was unreachable in production and would have mislabeled a host/operator cancel as a failure. - Update the stale invariant doc, the five-output surface contract count, and the exhaustiveness-only comment on the non-actionable arm. - Document the cheap/non-blocking contract on TriggerFireSettlementObserver (the worker awaits it inline in the poller sweep) and note it at the active-cleanup call site. - Add contract coverage for the timeout arm's delivery-failure outcome (Failed) and a regression test proving the race-grace path delivers the cancellation notice; the cancelled-with-category test now asserts the cancellation notice wins.
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/product/ironclaw_assistant/src/run_delivery/triggered.rs (1)
736-739: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep the grace poll within the configured deadline.
When
grace_deadlineis shorter thansettings.poll_interval, this branch can sleep past the deadline and accept a terminal result that arrived after the intended race-window. Capsleep()bygrace_deadline - Instant::now(), and apply a per-call/remaining timeout toget_run_statebefore accepting that result. This also keepscrates/product/**/*.rsguarded behind product-mediated deadlines and timeouts.🤖 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/product/ironclaw_assistant/src/run_delivery/triggered.rs` around lines 736 - 739, Update the grace-period polling loop around grace_deadline and get_run_state to cap each sleep at the remaining duration until grace_deadline, preventing poll_interval from overshooting the deadline. Apply the same per-call remaining-time timeout to get_run_state, and only accept its terminal result when the call completes within the configured grace window.
🤖 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/product/ironclaw_assistant/src/run_delivery/triggered.rs`:
- Around line 736-739: Update the grace-period polling loop around
grace_deadline and get_run_state to cap each sleep at the remaining duration
until grace_deadline, preventing poll_interval from overshooting the deadline.
Apply the same per-call remaining-time timeout to get_run_state, and only accept
its terminal result when the call completes within the configured grace window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a77ecda4-786a-4df5-b7a1-218d3a81163e
📒 Files selected for processing (5)
crates/app/ironclaw_architecture_tests/tests/reborn_restructure_baselines.rscrates/product/ironclaw_assistant/src/run_delivery/prompts.rscrates/product/ironclaw_assistant/src/run_delivery/triggered.rscrates/product/ironclaw_assistant/tests/run_delivery_contract.rsscripts/ci/composition-budget.toml
# Conflicts: # crates/app/ironclaw_architecture_tests/tests/reborn_restructure_baselines.rs # scripts/ci/composition-budget.toml
…iring race test The unbound-Telegram pairing race test (merged from main via #7131) matched the working indicator by the substring "is thinking", which the varied copy removed. Identify it structurally instead — the race-chat message anchored to 618 that is not the final reply — so it survives the copy change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ
…rogress nudges (nearai#7446) * feat(channels): vary the "working" notice per run Replace the single "Ironclaw is thinking..." working indicator with a small rotation of warm notices ("On it!", "Let me look into that…", …), picked deterministically per run (by run id) so a shared channel with several concurrent runs does not fill with identical lines while one run keeps a single voice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * test(channels): assert working-indicator structure, not its varied copy The varied per-run working notice broke three suites that pinned the exact "Ironclaw is thinking..." literal. The literal is volatile content now; pin the copy in one place (the prompts unit test) and assert *structure* everywhere else — a distinct working indicator is posted, then retracted, then the reply. - run_delivery_contract.rs: a non-empty working notice distinct from the final reply precedes it (3 sites). - e2e_tests.rs (extension_host): the running turn posts a non-empty working indicator (2 sites) + generalized a stale doc comment. - extension_delivery.rs (root integration): select the working sendMessage by the call, not the words — the model is paused so it is the only /sendMessage before release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * feat(channels): rich working indicator — reactions, failure states, progress nudges Batches the full shared-channel working-indicator UX onto the varied-copy PR: - Reactions on the triggering message track the run: 👀 working → ✅ done, →⚠️ when parked on an approval/auth prompt (and back to 👀 on resume), → ❌ on failure/timeout. New OutboundPart::React + neutral RunReaction / ReactionAction in ironclaw_extension_contracts; Slack reactions.add/remove, Telegram setMessageReaction (allowlist-mapped), egress updated in both manifests; web-push reports unsupported; DeliveryIntent::Reaction (notice-class). - Failure/timeout states now reach the channel: a terminal failed/cancelled run retracts the stuck "thinking" indicator and posts a brief, diagnostic-free failure notice (source-routed through the same reliable path) instead of going silent; timeouts retract the indicator too. - Progress nudges: a long run refreshes its indicator in place with escalating "still working" copy — first at 30s, then each gap doubling — so it never looks stalled. The source message's vendor ref already rides ExternalConversationRef, so no new ingress plumbing was needed. The reaction lifecycle is a small state machine (set_source_reaction) in the delivery observer. Also addresses prior review: auth-flow assertions now require the working indicator distinct from the auth prompt too (CodeRabbit); reaction / failure / needs-input / nudge lifecycles are covered through observe_ack at the adapter seam (caller-level coverage). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * fix(channels): keep neutral code vendor-agnostic in reaction comments The reaction doc/comments named Telegram and Slack in the neutral channel contract and the delivery observer, tripping the reborn_generic_code_names_no_concrete_extension architecture gate. Reword to generic phrasing (vendor reaction APIs / the originating channel). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * test(channels): identify the working indicator structurally in the pairing race test The unbound-Telegram pairing race test (merged from main via nearai#7131) matched the working indicator by the substring "is thinking", which the varied copy removed. Identify it structurally instead — the race-chat message anchored to 618 that is not the final reply — so it survives the copy change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * test(channels): admit the new reaction egress paths in the manifest allowlist The reaction feature added reactions.add/remove (Slack) and setMessageReaction (Telegram) to the channel egress allowlists; the first-party manifest parity tests pin those lists exactly (a security boundary — a new egress path must be a reviewed change), so add the new paths to the expected sets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…rai#6896) (nearai#7131) * fix(run_delivery): deliver triggered run failures to the creator (nearai#6896) Scheduled/triggered runs that ended in Failed, Cancelled, or RecoveryRequired produced no user-visible notification: the triggered delivery driver minted notifications only for Completed / BlockedApproval / BlockedAuth and recorded every other terminal status as Skipped. A run that timed out before reaching an actionable state only logged a warn and recorded Failed, leaving the creator in silence. Delivery: - triggered_notification_for_state now mints a FinalReplyReady notification for Failed and RecoveryRequired using the existing per-category failure summaries (reborn_failure_summary_for_category) over state.failure.category(), with a generic fallback when no category is present. - Cancelled mints the same notification, preferring a failure-category summary when one is present and falling back to a fixed cancellation notice otherwise. - The RunWaitTimedOut branch with no prior blocked marker now delivers the timeout notice as a terminal reply instead of recording Failed. - The wildcard arm is replaced with explicit non-actionable statuses (Queued, Running, CancelRequested, BlockedResource, BlockedDependentRun, BlockedExternalTool) so a future status fails to compile rather than silently skipping. Observer: - TriggerFireSettlementObserver gains on_failed_fire_settled as a default no-op method, plus a TriggerFailedFireSettlement event carrying tenant/trigger/fire-slot/run-id/history-status. Noop and existing implementors keep compiling. - The active-cleanup sweep fires on_failed_fire_settled when clear_active_fire succeeds with TriggerRunHistoryStatus::Error, so post-accept failures are observable for automation health. Ok, Running, and already-cleared fires do not fire the hook. Tests: - run_delivery_contract: Failed+model_error, Failed without category, Cancelled, and timeout-before-actionable all assert a Delivered outcome with the expected notice text and footer. - worker tests: a terminal-Error active fire fires exactly one on_failed_fire_settled; a terminal-Ok active fire fires none. The larger retry/redrive budget for failed post-accept fires (retry_disposition has zero production callers) is intentionally left for a follow-up; it is out of scope for this surgical delivery fix. * style: cargo fmt the nearai#6896 delivery fix * fix(triggers): address terminal delivery review feedback * fix(assistant): drop unused UserId import after merge * fix(run_delivery): address multi-agent review findings - Extract shared terminal-notice helpers (final_reply_notice, outcome_for_delivery_failure, deliver_terminal_notice) so the timeout, OAuth-backstop, and generic failure arms share one notice shape and outcome taxonomy instead of a third hand-rolled copy. - Add a bounded race-grace window after the wait backstop: a run that crosses into a terminal state during the final wait (cancellation in flight, failure landing after the last poll) now delivers the correct terminal notice instead of the timeout copy. - Cancelled runs always deliver the fixed cancellation notice; the failure-category branch was unreachable in production and would have mislabeled a host/operator cancel as a failure. - Update the stale invariant doc, the five-output surface contract count, and the exhaustiveness-only comment on the non-actionable arm. - Document the cheap/non-blocking contract on TriggerFireSettlementObserver (the worker awaits it inline in the poller sweep) and note it at the active-cleanup call site. - Add contract coverage for the timeout arm's delivery-failure outcome (Failed) and a regression test proving the race-grace path delivers the cancellation notice; the cancelled-with-category test now asserts the cancellation notice wins. * fix(run_delivery): address review comments and restore CI gates Review fixes (CodeRabbit on 01e887f/f8af109): - Grace loop fails loud: log the bound TurnError on state-poll failure and the RunDeliveryError on terminal-notice build failure before falling back to the timeout copy, with silent-ok markers on both intentional fallbacks. - Hoist TriggeredReplyTargetAuthority, CodecChannelTargetResolver, and TriggeredNotificationContext to one construction before the watcher loop; the race-grace arm, timeout arm, and loop body now share it. - Collapse the duplicated failure-summary expression into one closure and name TurnStatus::Failed explicitly so future statuses are compiler-visible. - Drop the stale "Only three states" count from the surface-contract doc. - Test fixture: encode the late-terminal flip as one Option<(usize, ScriptedRunState)> field instead of two correlated Options with an expect. - Terminal-crossing test: document why flip_after=30 deterministically outruns the wait poll budget and assert the grace loop issues no cancellation (cancel_calls == 0). CI: - composition-budget: re-seed loc_ceiling 40432 -> 40593 (measured on the merged tree; the nearai#7131 settlement observer adds +161 governed LOC of wiring) and move the arch-test record with it. - trigger_poller: use the colon-form tracing target required by nearai#7146. * ci: re-trigger pull_request workflows for c2460ed * fix(composition): capture the settlement health warn in the observer test The traced_test default filter is {crate}=trace, which drops events whose metadata target is `ironclaw::reborn::…`. The observer warning is emitted with the colon-form target (required by nearai#7146 — the equals form recorded a field and never matched RUST_LOG target filters), so the test saw an empty buffer. Enable tracing-test's no-env-filter feature, the same pattern the capabilities/host-runtime/mcp/loop crates use for cross-target assertions. Re-seed the composition budget to the merged-tree measurement (40747 -> 40867): nearai#7131's observer wiring lands on top of post-measurement mainline inflow; measured with the gate, set to current. The arch-test record moves with the manifest. * fix(run_delivery): merge main and adapt to notice_discriminator String - Merge origin/main (nearai#7377 run-acts-as-invoker, nearai#7323, nearai#7382, nearai#6938, nearai#7280, nearai#7393, nearai#7389, nearai#7364, nearai#7228, nearai#7371, nearai#7399). - main's nearai#7377 landed a narrower terminal arm (generic failure notice for TurnStatus::Failed only); keep the nearai#6896 arm, which covers Failed and RecoveryRequired with sanitized per-category summaries plus Cancelled and the timeout grace path, and adapt to the Option<String> notice_discriminator main introduced. - Re-seed the composition budget to the merged-tree measurement (40811 -> 40861, the run-failure settlement observer lands +50 governed LOC); the arch-test record moves with the manifest.
…rogress nudges (nearai#7446) * feat(channels): vary the "working" notice per run Replace the single "Ironclaw is thinking..." working indicator with a small rotation of warm notices ("On it!", "Let me look into that…", …), picked deterministically per run (by run id) so a shared channel with several concurrent runs does not fill with identical lines while one run keeps a single voice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * test(channels): assert working-indicator structure, not its varied copy The varied per-run working notice broke three suites that pinned the exact "Ironclaw is thinking..." literal. The literal is volatile content now; pin the copy in one place (the prompts unit test) and assert *structure* everywhere else — a distinct working indicator is posted, then retracted, then the reply. - run_delivery_contract.rs: a non-empty working notice distinct from the final reply precedes it (3 sites). - e2e_tests.rs (extension_host): the running turn posts a non-empty working indicator (2 sites) + generalized a stale doc comment. - extension_delivery.rs (root integration): select the working sendMessage by the call, not the words — the model is paused so it is the only /sendMessage before release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * feat(channels): rich working indicator — reactions, failure states, progress nudges Batches the full shared-channel working-indicator UX onto the varied-copy PR: - Reactions on the triggering message track the run: 👀 working → ✅ done, →⚠️ when parked on an approval/auth prompt (and back to 👀 on resume), → ❌ on failure/timeout. New OutboundPart::React + neutral RunReaction / ReactionAction in ironclaw_extension_contracts; Slack reactions.add/remove, Telegram setMessageReaction (allowlist-mapped), egress updated in both manifests; web-push reports unsupported; DeliveryIntent::Reaction (notice-class). - Failure/timeout states now reach the channel: a terminal failed/cancelled run retracts the stuck "thinking" indicator and posts a brief, diagnostic-free failure notice (source-routed through the same reliable path) instead of going silent; timeouts retract the indicator too. - Progress nudges: a long run refreshes its indicator in place with escalating "still working" copy — first at 30s, then each gap doubling — so it never looks stalled. The source message's vendor ref already rides ExternalConversationRef, so no new ingress plumbing was needed. The reaction lifecycle is a small state machine (set_source_reaction) in the delivery observer. Also addresses prior review: auth-flow assertions now require the working indicator distinct from the auth prompt too (CodeRabbit); reaction / failure / needs-input / nudge lifecycles are covered through observe_ack at the adapter seam (caller-level coverage). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * fix(channels): keep neutral code vendor-agnostic in reaction comments The reaction doc/comments named Telegram and Slack in the neutral channel contract and the delivery observer, tripping the reborn_generic_code_names_no_concrete_extension architecture gate. Reword to generic phrasing (vendor reaction APIs / the originating channel). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * test(channels): identify the working indicator structurally in the pairing race test The unbound-Telegram pairing race test (merged from main via nearai#7131) matched the working indicator by the substring "is thinking", which the varied copy removed. Identify it structurally instead — the race-chat message anchored to 618 that is not the final reply — so it survives the copy change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ * test(channels): admit the new reaction egress paths in the manifest allowlist The reaction feature added reactions.add/remove (Slack) and setMessageReaction (Telegram) to the channel egress allowlists; the first-party manifest parity tests pin those lists exactly (a security boundary — a new egress path must be a reviewed change), so add the new paths to the expected sets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USZCuPuTtrrhDc8XnqnapQ --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Summary
Failed,RecoveryRequired, orCancellednow deliver sanitized terminal notices to the creator's configured preference target instead of being recorded silently asSkipped.Change Type
Linked Issue
Fixes #6896
Validation
cargo fmt --all -- --checkcargo clippy -p ironclaw_product -p ironclaw_triggers -p ironclaw_reborn_composition --all-targets --all-features -- -D warningscargo build(covered by clippy and test builds)cargo test --features integrationif database-backed or integration behavior changed — Not applicable: no schema/backend contract changed; focused composition coverage exercises the production observerTest Strategy
User behavior:
A scheduled run that fails, requires recovery, is cancelled, or times out now sends one safe terminal notice to the creator's configured target instead of disappearing silently.
Risk areas:
Tests added or updated:
on_trigger_submitted.ironclaw_triggerssuite, including successful failure settlement, successful terminal non-emission, and lost-clear-race non-emission.What the tests prove:
NoDefaultConfiguredand sends nothing.Commands run:
Security Impact
None. Outbound failure text comes only from the existing static sanitized-summary table. Raw provider/runtime failures are never delivered. The settlement callback carries typed identities only and cannot mint runs or mutate trigger state.
Reborn Trust-Boundary Checklist
TriggerFailedFireSettlementis constructed by active cleanup only after the exact fire is durably cleared.serde(default)fields fail closed: Not applicable; no serialized fields changed.Database Impact
None. No migration or persisted schema changes. Existing
TriggerRunHistoryStatus::Errorhistory remains authoritative.Blast Radius
Touches triggered-run outbound delivery, trigger active-fire cleanup observation, composition health telemetry, and their tests. The observer method is required so all production, no-op, and test implementations remain explicit.
Rollback Plan
Revert the PR. No schema, configuration, or persisted-format migration needs rollback.
Review Follow-Through
All currently accessible CodeRabbit and multi-agent review findings are addressed in commit
8ab564656. Retry/redrive policy remains intentionally out of scope and does not bypass canonical delivery.Review track: B