fix(reborn): gate triggered Slack OAuth URL on a verified personal DM - #4953
Merged
Merged
Conversation
Security follow-up from PR #4946 review. On the triggered-run delivery path, a `BlockedAuth` run with a link-based OAuth challenge posted the OAuth `authorization_url` into Slack assuming the target was the creator's private DM. The triggered target resolves from the creator's personal communication preference and is NOT guaranteed to be a DM — shared-channel outbound targets advertise `auth_prompts: true` (`slack_outbound_targets.rs` `entry_for_shared_channel_route`), so the OAuth setup URL could leak onto a shared Slack surface. Fix (fail closed): OAuth `authorization_url` may only be posted to a verified personal DM. - Add `slack_reply_target_is_personal_dm()` in `slack_outbound_targets.rs`: strictly parses the binding-ref segment chain and returns true only for a personal-DM ref (trailing `actor_kind=slack_user`/`actor` segments present AND a `D`-prefixed channel id). Any parse failure → false. - In `deliver_triggered_run`, resolve the creator's preference once and compute `target_is_verified_dm` against the EFFECTIVE auth target — `auth_prompt_target.or(final_reply_target)`, mirroring `resolution_engine.rs` `PreferenceTargetKind::AuthPrompt`. A looser "any stored target is a DM" check would wrongly pass when `auth_prompt_target` is a shared channel but `final_reply_target` is a DM. Preference read failure / no target → false (fail closed). - The `BlockedAuth` arm keeps `authorization_url` only when `target_is_verified_dm`; otherwise it cancels the run and posts the existing `SLACK_AUTH_UNAVAILABLE_MESSAGE` notice (same as the non-OAuth manual-token path). Tests: shared-channel target suppresses the URL + posts the notice; personal-DM target still posts the URL; auth_prompt_target=shared with final_reply_target=DM suppresses (precedence guard); 5 unit tests for the DM-detection helper. Note: `slack_host_beta::…wires_trigger_delivery_hook_writes_record` is a pre-existing flaky (timeout under parallel load; flakes on origin/main too), unrelated to this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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
WalkthroughAdds centralized Slack ChangesFail-closed OAuth DM Gate for Triggered Runs
Sequence DiagramsequenceDiagram
participant Trigger as Triggered<br/>Run Event
participant Check as AuthPrompt<br/>Detector
participant Auth as TriggeredSlackReply<br/>TargetAuthority
participant Resolve as Resolver
participant DM as DM<br/>Predicate
participant Deliver as Message<br/>Delivery
participant Reply as Terminal<br/>FinalReply
participant Run as Run<br/>Cancellation
Trigger->>Check: inspect notification for OAuth URL
Check->>Check: require_direct_message_target = has_auth_url
Check->>Auth: resolve with require flag
Auth->>Resolve: resolve target binding
Resolve->>Resolve: lookup conversation metadata
Auth->>DM: slack_reply_target_is_personal_dm?
alt Target is Personal DM
DM-->>Auth: true
Auth-->>Check: OK, proceed
Check->>Deliver: send notification
else Target is Shared Channel
DM-->>Auth: false
Auth-->>Check: OutboundTargetNotDirectMessage
Check->>Run: cancel blocked run
Check->>Reply: post auth-unavailable
Reply-->>Check: Delivered (OAuth suppressed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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 |
github-actions
Bot
added
risk: low
![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: 2590fbef08
ℹ️ 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".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (1)
2148-2154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop after sending the auth-unavailable terminal notice.
The suppressed OAuth path returns
FinalReplyReady, but the outer loop still sees the originalBlockedAuthmarker and continues. With a realcancel_run, the next poll can recordSkippedor wait until timeout after already posting the terminal notice.Proposed control-flow fix
- if let Some(marker) = next_blocked_marker { + if let Some(marker) = next_blocked_marker + && event_kind != RunNotificationEventKind::FinalReplyReady + { if event_kind == RunNotificationEventKind::AuthRequired { messages_to_delete_after_final.extend(posted_messages); } delivered_blocked_marker = Some(marker); // Loop again to wait for the next actionable state. continue; }Add a caller-level regression where the coordinator stays/cancels after
BlockedAuth; expect one auth-unavailable post and a terminal delivery outcome.Also applies to: 2393-2399, 2408-2426
🤖 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 2148 - 2154, After posting the auth-unavailable terminal notice when handling the BlockedAuth marker with RunNotificationEventKind::AuthRequired event, the loop should break instead of continuing to prevent further polling attempts. This issue occurs in multiple locations where BlockedAuth is handled: check the blocks at lines 2148-2154 (the anchor location), 2393-2399, and 2408-2426, and replace the continue statement with a break statement in each location where the auth-unavailable terminal notice has been posted, ensuring the loop terminates after the FinalReplyReady signal is processed and the terminal delivery outcome is properly recorded.
🤖 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 2030-2036: The tracing::warn! call for internal diagnostics in the
spawned delivery path should be changed to tracing::debug! since these are
background/internal diagnostics, not user-facing warnings that belong in the
TUI. Replace the warn! macro with debug! at the current location (lines
2030-2036) where the communication preference loading failure is logged.
Additionally, apply the same change at lines 2401-2406 where similar internal
diagnostic warnings appear, changing warn! to debug! in those locations as well
to maintain consistency with the coding guidelines that reserve info!/warn! for
user-facing messages and use debug! for internal diagnostics.
- Around line 2000-2026: The `target_is_verified_dm` flag computed at lines
2000-2026 in crates/ironclaw_reborn_composition/src/slack_delivery.rs can become
stale before the actual delivery, as the auth_prompt_target may change from a DM
to a shared channel between the initial snapshot and when the target is resolved
again during delivery at lines 2500-2518 or before rendering the OAuth URL at
lines 2612-2617. Fix this by either: (1) re-validating with
`slack_reply_target_is_personal_dm()` on the actual
`ValidatedReplyTargetBinding` at the delivery/rendering sites (lines 2500-2518
and 2612-2617) instead of relying on the stale early check, or (2) carrying the
exact verified `ReplyTargetBindingRef` through the authority and explicitly
rejecting any `AuthRequired` candidate at those same sites if the resolved
target differs from the originally verified one. Apply the chosen fix at all
three locations (anchor and siblings) to ensure auth fails closed and OAuth URLs
never leak to shared channels.
- Line 2291: The #[allow(clippy::too_many_arguments)] attribute on the non-trait
function is missing the required architecture exemption comment. Add a comment
to this attribute that follows the format: // arch-exempt: too_many_args,
<reason naming the missing aggregation>, plan `#NNNN`. This comment must explain
why the function requires multiple arguments and reference the relevant plan
number to justify the exemption from the clippy lint rule.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 2148-2154: After posting the auth-unavailable terminal notice when
handling the BlockedAuth marker with RunNotificationEventKind::AuthRequired
event, the loop should break instead of continuing to prevent further polling
attempts. This issue occurs in multiple locations where BlockedAuth is handled:
check the blocks at lines 2148-2154 (the anchor location), 2393-2399, and
2408-2426, and replace the continue statement with a break statement in each
location where the auth-unavailable terminal notice has been posted, ensuring
the loop terminates after the FinalReplyReady signal is processed and the
terminal delivery outcome is properly recorded.
🪄 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: 31221060-b6a4-474f-b336-58d3e447332a
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_outbound_targets.rs
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Prevent triggered Slack OAuth authorization URLs from being posted unless the effective target is a verified personal DM.
Stats: 4 findings (from 7 raw, 4 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, approach. Reviewers failed: maintainability. Body-only: 0
performance
- High DM pre-check can race the actual Slack target resolution (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2000-2006, confidence 91) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2501
The new gate reads the creator preference once before the delivery loop and cachestarget_is_verified_dm, but the actual send path later callsprepare_and_render_product_outbound, which resolves the target fromcommunication_preferencesagain. If the preference changes from a DM to a shared channel while the triggered run is waiting forBlockedAuth, the stale true pre-check still permits the OAuth payload while the later resolution can deliver it to the new shared-channel target. Also flagged by: bugs/High, security/Medium, approach/Medium.
tests
- Medium Preference-read failure lacks OAuth fail-closed coverage (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2029-2038, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2029
The new DM pre-check treats load_communication_preference errors as non-DM before the BlockedAuth OAuth gate, but the added triggered OAuth tests only exercise successful preference reads. A caller-level test should prove a backend preference-read error suppresses the authorization_url. - Low DM parser rejection conditions are not fully covered (
crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:881-890, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:890
The new parser handles external binding-ref input and tests valid DM, shared C-channel, non-D channel, malformed syntax, and empty actor, but it does not test the remaining reject predicates: actor_kind other than slack_user and otherwise-valid DM refs with trailing segments.
conventions
- Medium Missing arch-exempt for too_many_arguments allow (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2291-2291, confidence 100) — anchor: .claude/rules/architecture.md:43
The diff adds#[allow(clippy::too_many_arguments)]without the requiredarch-exemptannotation on the line above it. The architecture rule explicitly flags any added#[allow(clippy::too_many_arguments)]without anarch-exemptannotation on the line above it.
Addresses #4953 review. The OAuth-URL DM gate was a snapshot read once before the triggered poll loop, but delivery re-resolves the target from `communication_preferences` at send time. If the creator's preference flipped DM→shared-channel while the run waited in BlockedAuth, the stale-true snapshot let the OAuth `authorization_url` get built and posted to the channel (codex P1 / coderabbit Critical). Backstop (airtight, in-crate): `TriggeredSlackReplyTargetAuthority` gains `require_personal_dm_for_oauth`, set per-delivery when the payload carries an `authorization_url`. `resolve_product_outbound_target_metadata` — which sees the EXACT binding resolved at send time — fails closed when the flag is set and the binding is not a personal DM. On trip, the run is canceled and the `SLACK_AUTH_UNAVAILABLE_MESSAGE` notice is posted (graceful deny, matching the non-OAuth path); the URL is never posted. Also from review: - warn! → debug! on the two new background-delivery diagnostics (CLAUDE.md: background tasks must not warn!). - arch-exempt annotation on `triggered_notification_for_state`'s too_many_arguments allow. - helper tests: wrong actor_kind + trailing-segment rejection. Tests: snapshot-DM-but-channel-at-send suppresses the URL (race); preference-read error fails closed; existing DM/shared/precedence cases retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (2)
1999-2047:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the stale preference-stability comment and mark this fallback
silent-ok.Line 2003 says the preference “does not change during delivery,” but the new send-time backstop and tests exist because it can change while the run waits. The read-error fallback is acceptable because it fail-closes to non-DM, but repo rules require an inline
// silent-ok:justification for accepted preference/settings read fallbacks.Suggested comment fix
- // here (it does not change during delivery) so `triggered_notification_for_state` - // can gate the OAuth post without performing an async lookup on every loop - // iteration. Fail closed: if the preference cannot be read or the binding ref - // does not parse as a personal DM, treat the target as non-DM. + // here so `triggered_notification_for_state` can gate the OAuth post without + // performing an async lookup on every loop iteration. The preference can still + // change before send time; `TriggeredSlackReplyTargetAuthority` revalidates + // the resolved binding when an OAuth URL is present. Fail closed: if the + // preference cannot be read or the binding ref does not parse as a personal + // DM, treat the target as non-DM. ... Err(err) => { + // silent-ok: communication preference read failure fail-closes OAuth DM + // verification to non-DM, suppressing authorization_url emission. tracing::debug!(As per coding guidelines, “Fail loud” requires justified
// silent-ok: <reason>comments for acceptable DB/settings read fallbacks, and changed behavior must keep adjacent comments current.🤖 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 1999 - 2047, Update the stale comment in the target_is_verified_dm block that currently claims the preference "does not change during delivery" — this is no longer accurate since the code now includes a send-time backstop and tests for preference changes during delivery, so reword this comment to reflect the current behavior. Additionally, add a `// silent-ok: <reason>` inline comment to the Err(err) branch in the load_communication_preference match statement to justify why this fallback to false (fail-closed, treating target as non-DM) is an acceptable error handling approach per repository guidelines for DB/settings read fallbacks.Source: Coding guidelines
2168-2174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat the auth-unavailable
FinalReplyReadyas terminal.This
BlockedAuthbranch returns a terminal notice, but the caller still seesnext_blocked_marker = Some(...)from the original blocked state and continues polling. After a realcancel_run, the next terminalCancelledstate will produceOk(None)and recordSkippedafter already posting the notice; delayed cancellation can instead time out and recordFailed.Suggested control-flow fix
- if let Some(marker) = next_blocked_marker { + if matches!( + event_kind, + RunNotificationEventKind::ApprovalNeeded | RunNotificationEventKind::AuthRequired + ) && let Some(marker) = next_blocked_marker { if event_kind == RunNotificationEventKind::AuthRequired { messages_to_delete_after_final.extend(posted_messages); } delivered_blocked_marker = Some(marker); // Loop again to wait for the next actionable state.As per coding guidelines, auth paths must fail closed; once this branch cancels the auth-blocked run and emits the sanitized unavailable notice, that notice must be the terminal delivery outcome.
Also applies to: 2477-2504
🤖 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 2168 - 2174, The BlockedAuth branch currently posts a terminal auth-unavailable notice but then continues looping because next_blocked_marker remains Some(...), allowing subsequent state changes to either record Skipped (if cancelled) or Failed (if timed out). To fix this and ensure auth paths fail closed, modify the AuthRequired event handling to break from the polling loop immediately after posting the FinalReplyReady notice and extending messages_to_delete_after_final, rather than continuing to the next iteration. This ensures the terminal auth-unavailable notice is the definitive delivery outcome with no further state polling.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/slack_delivery.rs`:
- Around line 2197-2235: The code currently swallows errors from both
cancel_auth_blocked_run and deliver_triggered_notification (the latter using let
_ =) while unconditionally recording TriggeredRunDeliveryOutcomeKind::Delivered.
Instead, capture the result from deliver_triggered_notification instead of
ignoring it, and only record the Delivered outcome if both
cancel_auth_blocked_run succeeds and deliver_triggered_notification succeeds. If
either operation fails, record an appropriate failure outcome using
record_triggered_run_outcome rather than falsely claiming success.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 1999-2047: Update the stale comment in the target_is_verified_dm
block that currently claims the preference "does not change during delivery" —
this is no longer accurate since the code now includes a send-time backstop and
tests for preference changes during delivery, so reword this comment to reflect
the current behavior. Additionally, add a `// silent-ok: <reason>` inline
comment to the Err(err) branch in the load_communication_preference match
statement to justify why this fallback to false (fail-closed, treating target as
non-DM) is an acceptable error handling approach per repository guidelines for
DB/settings read fallbacks.
- Around line 2168-2174: The BlockedAuth branch currently posts a terminal
auth-unavailable notice but then continues looping because next_blocked_marker
remains Some(...), allowing subsequent state changes to either record Skipped
(if cancelled) or Failed (if timed out). To fix this and ensure auth paths fail
closed, modify the AuthRequired event handling to break from the polling loop
immediately after posting the FinalReplyReady notice and extending
messages_to_delete_after_final, rather than continuing to the next iteration.
This ensures the terminal auth-unavailable notice is the definitive delivery
outcome with no further state polling.
🪄 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: 0480aaf3-e22e-4975-9fb1-5a7e6b4c3a50
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_outbound_targets.rs
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Restrict Slack OAuth authorization_url delivery to verified personal DMs and fail closed on ambiguous or shared-channel targets.
Stats: 2 findings (from 5 raw, 2 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Existing unresolved review threads were checked and not duplicated here.
Maintainability
- Medium Personal-DM detection duplicates the binding-ref parse contract (
crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:854-890, confidence 84) — anchor:crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:854
The newslack_reply_target_is_personal_dmhelper replays the same reply-target segment walk thatslack_conversation_id_from_reply_target_binding_refalready performs immediately above it. That leaves the reply-target fingerprint format hardcoded in two independent parsers, so future changes to the segment layout need manual edits in more than one place.
Local Patterns
- Low Drop the nonexistent fallback from the classification comment (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2630-2635, confidence 89) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:2630-2635
The new comment says ato_string().containsfallback below would catch wrappedBindingResolutionFailederrors, but the match arm has no such fallback. That leaves the comment describing behavior the code does not actually implement in this OAuth backstop path.
#4953 review round 2: - coderabbit (Major): the OAuth backstop arm recorded `Delivered` unconditionally, swallowing both `cancel_auth_blocked_run` failure and the notice-delivery result — which could leave the run blocked / the user silent while the store claims success. Now: cancel failure records `Failed` and returns; the notice delivery result is mapped to the matching `TriggeredRunDeliveryOutcomeKind` (Delivered / NoDefaultConfigured / Denied / Failed). - henrypark (Medium): `slack_reply_target_is_personal_dm` and `slack_conversation_id_from_reply_target_binding_ref` independently walked the same reply-target segment format. Factored a single `decode_slack_reply_target_binding_ref` → `DecodedSlackReplyTarget`; both functions now derive from it (behavior-preserving). - henrypark (Low): corrected the `classify_delivery_error` comment that described a `to_string().contains` fallback the code never had — the resolver error is matched directly as `Workflow { source: BindingResolutionFailed { reason } }`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/slack_delivery.rs`:
- Around line 2232-2250: The terminal path in the match statement handling
deliver_triggered_notification errors (specifically the
Err(TriggeredNotificationFailure::OAuthTargetNotDm) and
Err(TriggeredNotificationFailure::Other(_)) branches) bypasses the normal
cleanup that occurs earlier in the function at lines 2176-2179, causing stale
OAuth authorization messages to remain in Slack. Before calling
record_triggered_run_outcome and returning outcome in this backstop path, apply
the same cleanup logic used in the normal flow to delete any prior OAuth prompts
that were sent to DMs. Additionally, add a regression test at the caller level
that verifies this cleanup occurs: create a test scenario where one auth-blocked
state posts an OAuth prompt to a DM, then a second auth-blocked state flips to a
shared-channel and triggers this backstop path, then assert that the original
authorization message has been properly deleted.
In `@crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs`:
- Around line 839-844: The adapter segment is being consumed and dropped without
validation, allowing non-Slack reply bindings to bypass the oauth security gate
in slack_reply_target_is_personal_dm(). In the code handling the "reply:" prefix
where take_product_binding_segment is called for "adapter", capture the adapter
value returned and validate that it equals SLACK_V2_ADAPTER_ID before continuing
to parse the remaining segments. If the adapter does not match, return None
(fail closed). Apply this same validation at the second affected location (lines
918-935) in the file where adapter segments are similarly parsed and validated.
🪄 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: 8f041205-69ad-47ee-be7b-04ac330a11a5
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_outbound_targets.rs
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Prevent Slack OAuth authorization URLs from being posted unless the triggered run resolves to a verified personal DM.
Stats: 3 findings (from 6 raw, 3 after dedupe/filter) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Filtered as already covered or disproven: the existing live threads already cover the stale DM pre-check race; the constructor/predicate round-trip tests pass on this head; the preference-read error path is already covered.
tests
- Medium Missing OAuth suppression test when no communication preference exists (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2035-2035, confidence 79) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2035
The new DM pre-check treatsload_communication_preferencereturningOk(None)as non-DM, but the added triggered OAuth tests cover shared-channel, personal-DM, preference-read error, and the DM-to-channel send-time race. The no-preference branch is distinct and should be covered so this fail-closed behavior stays locked in. - Low Missing parser test for refs without a topic segment (
crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:925-930, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:928
slack_reply_target_is_personal_dmexplicitly rejects decoded refs wheretopic_presentis false, but the new predicate tests do not cover that fail-closed branch. Since this helper gates whether Slack may receive an OAuth URL, the externally parseable malformed-ref shapes should be pinned directly.
maintainability
- Medium Move the OAuth DM guard out of shared authority state (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1996-1996, confidence 87) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:1996
The OAuth suppression rule is split across the pre-loop preference snapshot, a mutableAtomicBoolonTriggeredSlackReplyTargetAuthority, and a sentinel string mapped back intoOAuthTargetNotDm. That makes one policy depend on three representations and two match sites, which is easy to drift as the delivery loop changes.
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Restrict Slack OAuth authorization URLs to verified personal DMs and fail closed for shared-channel or ambiguous delivery targets.
Stats: 2 findings posted (from 7 raw, 2 after live-thread dedupe) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1.
Bugs
- High Pre-check looks up the wrong preference key in owner-scoped runs (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2007-2025, confidence 88) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:2008
The OAuth DM pre-check loadsCommunicationPreferenceKey::personal(scope.tenant_id, actor.user_id), but this delivery path and its tests seed and resolve preferences byscope.explicit_owner_user_id()when present. For owner-scoped triggered runs where the acting principal differs from the owner, the pre-check can read a different preference record and fail closed even though the owner has a valid personal-DM auth target.
Tests
- Medium OAuth backstop never tests cancel_run failure (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2197-2213, confidence 86) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:2197(no precise diff position — body only)
The new OAuth suppression branch explicitly handlescancel_auth_blocked_run(...).awaitfailing by recordingTriggeredRunDeliveryOutcomeKind::Failed, but the current Slack delivery tests only cover successful cancel-and-notice paths. A regression in this terminal failure fallback would go unobserved.
Deduped against existing open review threads for the stale preference snapshot race, parser duplication/hardening, preference-read fail-closed coverage, no-preference coverage, and the mutable-flag/sentinel design note.
#4953 review round 3: - henrypark (High): the OAuth DM pre-check keyed the preference by `actor.user_id`, but this path + tests seed/resolve by `scope.explicit_owner_user_id()`. Owner-scoped runs (acting principal ≠ owner) read the wrong record and over-suppressed. Key now uses `explicit_owner_user_id().unwrap_or(actor.user_id)`. - coderabbit (Major, security): the shared `decode_slack_reply_target_binding_ref` consumed the `adapter` segment without validating it. A non-Slack `reply:` binding shaped like a DM could pass `slack_reply_target_is_personal_dm` (the OAuth-URL gate). Now fails closed unless `adapter == SLACK_V2_ADAPTER_ID`. + wrong-adapter regression test. - coderabbit (Major, security): the OAuth backstop terminal path skipped the prior-prompt cleanup, leaving a stale `authorization_url` DM after cancel. Now drains `messages_to_delete_after_final` before returning. - tests: no-preference (`Ok(None)`) fail-closed suppression; missing-`topic` rejection. Note: the two-state backstop-deletes-prior-DM-prompt regression test is a known gap (needs DM-then-channel preference + dual BlockedAuth scripted states); the cleanup code itself is covered by review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (1)
2176-2182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop after delivering the auth-unavailable terminal notice.
next_blocked_markeris derived from the originalBlockedAuthstate, so Line 2176 treats the suppressed-OAuthFinalReplyReadynotice from Lines 2523-2534 as another blocked prompt and continues polling. With a real cancel, the next terminal state can recordSkipped; with the current scripted tests, it can post an extra final reply after the cancel notice. Continue only for actual gate prompts and letFinalReplyReadyfall through to cleanup +Delivered.Proposed fix
- if let Some(marker) = next_blocked_marker { + if let Some(marker) = next_blocked_marker + && matches!( + event_kind, + RunNotificationEventKind::ApprovalNeeded + | RunNotificationEventKind::AuthRequired + ) + { if event_kind == RunNotificationEventKind::AuthRequired { messages_to_delete_after_final.extend(posted_messages); } delivered_blocked_marker = Some(marker); // Loop again to wait for the next actionable state.As per coding guidelines, auth paths must fail closed and bug fixes should be tested through the caller.
Also applies to: 2523-2534
🤖 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 2176 - 2182, The current condition at the `next_blocked_marker` check incorrectly treats the suppressed-OAuth FinalReplyReady notice as another blocked prompt and continues polling instead of falling through to cleanup and delivery. Refine the condition to only continue for actual gate prompts (real BlockedAuth states) and exclude the FinalReplyReady terminal state from triggering the continue statement. This allows FinalReplyReady to properly fall through to cleanup logic and mark the delivery as Delivered, preventing extra final replies from being posted after the cancel notice in the delivery flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 2176-2182: The current condition at the `next_blocked_marker`
check incorrectly treats the suppressed-OAuth FinalReplyReady notice as another
blocked prompt and continues polling instead of falling through to cleanup and
delivery. Refine the condition to only continue for actual gate prompts (real
BlockedAuth states) and exclude the FinalReplyReady terminal state from
triggering the continue statement. This allows FinalReplyReady to properly fall
through to cleanup logic and mark the delivery as Delivered, preventing extra
final replies from being posted after the cancel notice in the delivery flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92289fc5-ef85-41d6-a645-47bd6945bcde
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_outbound_targets.rs
#4953 review (henrypark, Medium): the OAuth-DM backstop classified its trip by matching a magic reason string (`OAUTH_TARGET_NOT_DM_MARKER`) on the cross-crate `ProductWorkflowError` inside `classify_delivery_error`. Replace that with a typed handshake: the authority sets a dedicated `oauth_target_not_dm: AtomicBool` on trip, and `deliver_triggered_notification` reads-and-clears it to return `TriggeredNotificationFailure::OAuthTargetNotDm` before falling through to `classify_delivery_error`. The sentinel const and its string-match arm are removed. Behavior is unchanged (the resolver still returns a plain, human-readable `BindingResolutionFailed` reason and the URL is never posted to a non-DM); the classification no longer depends on string contents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Restrict Slack OAuth authorization URLs to verified personal DMs on the triggered-run delivery path.
Stats: 2 new findings after live-thread dedupe (7 raw findings across reviewers). Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Existing unresolved review threads already cover the send-time DM race, arch-exempt annotation, preference/no-preference tests, parser coverage gaps, parser duplication, owner-scoped preference key, and the broader OAuth guard state-shape concern, so this review does not repost those duplicates.
Bugs
-
Medium Empty space segment is treated as a valid DM ref (
crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:871-877, confidence 86) — anchor:crates/ironclaw_reborn_composition/src/slack_outbound_targets.rs:873
slack_reply_target_is_personal_dmaccepts a structurally malformed reply target withspace:0:because the decoder turns an empty space segment intoNoneand the DM predicate never checks it. -
Medium Backstop deletes auth prompts before cancellation succeeds (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2199-2224, confidence 64) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:2199
The OAuth backstop drains and deletes prior auth prompts beforecancel_auth_blocked_runsucceeds; if cancellation fails, the blocked run can remain without a usable Slack prompt or replacement notice. Also flagged by: tests/Medium.
…s (review) #4953 review round 4 (henrypark, Medium ×2): - Backstop ordering: the OAuth backstop arm deleted prior auth-prompt messages BEFORE `cancel_auth_blocked_run`. A transient cancel failure then recorded `Failed` having already removed the prompt the user could have used. Now: cancel first (on failure, record Failed and return without deleting); delete the stale prompts only after a successful cancel and after the replacement notice has been attempted. - DM gate: `slack_reply_target_is_personal_dm` accepted a ref with an empty `space` segment (`space:0:` → space_id None). A forged/corrupted preference missing the team/space binding could satisfy the OAuth-DM gate. Now rejects `space_id.is_none()`. + `space:0:` regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Restrict Slack OAuth authorization URLs to verified personal DMs on triggered-run delivery paths.
Stats: 2 findings (from 10 raw, 2 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
tests
- Medium Owner-scoped OAuth lookup is untested when actor differs (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2003-2012, confidence 91) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2003
The new DM pre-check intentionally reads the personal communication preference from scope.explicit_owner_user_id() before falling back to actor.user_id. All added triggered-OAuth delivery tests use minimal_trigger_fire(), whose creator_user_id matches personal_turn_scope()'s owner, so the new owner-vs-actor branch is not exercised. A regression here would silently read the wrong preference and suppress or leak OAuth URLs for owner-scoped runs.
approach
- Medium Snapshotting the DM check duplicates the send-time authority (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:2743-2754, confidence 87) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2743
The pre-loop target_is_verified_dm snapshot decides whether to emit an AuthPrompt, then the send path separately rechecks the exact resolved binding through TriggeredSlackReplyTargetAuthority and classifies OAuthTargetNotDm. That makes one policy depend on two DM/non-DM decisions plus atomic handoff state. The simpler shape is to treat any authorization_url auth prompt as requiring the resolver backstop and let the send-time resolver be the single place that decides whether the current binding is a personal DM.
Also flagged by: maintainability/Medium, performance/Medium
#4953 review (henrypark, Medium + maintainability + performance): the DM rule lived in two places — a pre-loop `target_is_verified_dm` preference snapshot AND the send-time resolver backstop. Drop the snapshot; the send-time `TriggeredSlackReplyTargetAuthority` resolver is now the single place that decides whether the current binding is a personal DM. `prepare_and_render_product_outbound` runs the resolver BEFORE it renders/posts, so a non-DM OAuth delivery fails closed before the URL is ever posted — the snapshot was only an optimization. The `BlockedAuth` arm now builds the OAuth AuthPrompt whenever `authorization_url` is present and lets the backstop suppress it (cancel + auth-unavailable notice) for non-DM targets. Removes: the ~57-line snapshot block, the `target_is_verified_dm` param (triggered_notification_for_state is back under the arg limit, so its too_many_args allow is gone too), the owner-key branch, and 2 snapshot-specific tests + their 4 preference-double helpers. The shared-channel suppression case is still covered end-to-end via the backstop. 65/65 slack_delivery tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (1)
5877-5923: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert the OAuth backstop’s terminal side effects in the caller tests.
Lines 5948, 6178, and 7239 only wait for any delivery record. A regression that skips
cancel_auth_blocked_run()or recordsFailedafter posting the sanitized notice would still pass, despite these tests documenting cancellation and terminal delivery. Keep a typed coordinator clone and assertcancel_call_count() == 1plusTriggeredRunDeliveryOutcomeKind::Deliveredin the suppression/backstop cases.As per coding guidelines, “Test through the caller” and “Every bug fix must include a regression test.”
Assertion pattern
- let services = make_services( - coordinator, + let services = make_services( + coordinator.clone(), thread_service, egress.clone(), outbound, install, ); ... - wait_for_delivery_record(&delivery_store, run_id).await; + let record = wait_for_delivery_record(&delivery_store, run_id).await; + assert_eq!( + coordinator.cancel_call_count(), + 1, + "OAuth suppression must cancel the blocked run" + ); + assert_eq!( + record.outcome, + TriggeredRunDeliveryOutcomeKind::Delivered, + "successful auth-unavailable notice delivery must record Delivered" + );Apply the same assertion shape to the auth-target precedence and authority-backstop regressions.
Also applies to: 5944-5948, 6119-6156, 6175-6178, 7171-7215, 7236-7240
🤖 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 5877 - 5923, Add stronger assertions to the auth-backstop test cases to verify terminal side effects. At the locations where tests currently only check for any delivery record (lines 5944-5948, 6119-6156, 6175-6178, 7171-7215, and 7236-7240), also assert that the typed coordinator (created with ScriptedTurnCoordinator::with_states) has cancel_call_count() equal to 1 to verify cancel_auth_blocked_run() was called, and assert that the delivery_store contains a record with TriggeredRunDeliveryOutcomeKind::Delivered to ensure the outcome is properly marked as delivered rather than failed. This prevents regressions where the cancellation logic is skipped or the wrong outcome kind is recorded.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 5877-5923: Add stronger assertions to the auth-backstop test cases
to verify terminal side effects. At the locations where tests currently only
check for any delivery record (lines 5944-5948, 6119-6156, 6175-6178, 7171-7215,
and 7236-7240), also assert that the typed coordinator (created with
ScriptedTurnCoordinator::with_states) has cancel_call_count() equal to 1 to
verify cancel_auth_blocked_run() was called, and assert that the delivery_store
contains a record with TriggeredRunDeliveryOutcomeKind::Delivered to ensure the
outcome is properly marked as delivered rather than failed. This prevents
regressions where the cancellation logic is skipped or the wrong outcome kind is
recorded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d910edaa-f759-4bd9-bfa0-d2951bfee64e
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs
…er (review) Multi-reviewer code-review pass on #4953: - BUG (correctness): a triggered run that hit a NON-OAuth (manual credential) auth gate posted the auth-unavailable notice and canceled the run, but recorded `Skipped` instead of `Delivered`. The deny branch returns a terminal `FinalReply`, yet the loop saw `next_blocked_marker` derived from the blocked *state* and continued; the next poll (Cancelled) returned `Ok(None)` → `Skipped`. Now the keep-waiting `continue` only fires for blocked *prompt* notifications (`ApprovalNeeded`/`AuthRequired`), so terminal `FinalReply` deliveries fall through to `Delivered`. - actor_id: the decoder stored `Some("")` for an empty `actor` segment, contradicting its doc-comment. Now mapped to `None` at assignment; the DM predicate's actor check simplifies to `actor_id.is_some()`. Identical behavior, honest type. - Documented that `slack_conversation_id_from_reply_target_binding_ref` now enforces adapter identity via the shared decoder. - Removed dead `_thread_id` binding. Tests: non-OAuth denial records Delivered (pins the bug); OAuth backstop cancel-failure records Failed with no stale-prompt deletion; missing-actor -segment rejection; non-Slack-adapter → None at the conversation-id caller. Note: the two-state "backstop deletes a prior DM auth prompt" e2e test remains a known harness-heavy gap (needs a preference double that flips DM→channel between resolutions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… error #4953 code-review (multi-reviewer): the OAuth-DM requirement was threaded between the delivery loop and the target resolver via two AtomicBool scratch fields on TriggeredSlackReplyTargetAuthority (require_personal_dm_for_oauth in, oauth_target_not_dm out, read-and- cleared, plus a manual reset) — a side-channel forced by the resolver trait being unable to carry the requirement or return a typed reason. Replace with data + a typed error: - ProductOutboundDeliveryRequest gains `require_direct_message_target`; the resolver trait method gains a `require_direct_message` param; prepare_and_render threads the flag through. - New typed ProductWorkflowError::OutboundTargetNotDirectMessage; both Slack resolvers (triggered + live, defense in depth) fail closed with it when require_direct_message && !is_personal_dm. - deliver_triggered_notification derives the flag from the payload and classify_delivery_error maps the typed variant → OAuthTargetNotDm. Both AtomicBools, the read-and-clear, the manual reset, and the per- delivery store are gone. Behavior identical; the URL still never reaches a non-DM target. Rippled into 3 exhaustive arms (ProductAdapterError 403, internal_invariant, terminal_ack None). Tests: 67 slack_delivery, 25 slack_outbound_targets, 24 outbound_delivery _contract, full product_workflow suite; 4 named OAuth security tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (1)
539-595:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet the DM gate for live OAuth auth prompts too.
Line 594 hard-codes
require_direct_message_target: false, so the newObservedSlackReplyTargetAuthoritycheck at Lines 1448-1451 never runs even when this path delivers anAuthPromptcarrying anauthorization_url. Compute the flag frompayloadbefore moving it, matching the triggered path.Proposed fix
let SlackActionableNotification { event_kind, payload, gate_ref_for_routing: _, } = notification; + let require_direct_message_target = matches!( + &payload, + ProductOutboundPayload::AuthPrompt(view) if view.authorization_url.is_some() + ); let reply_target = state.reply_target_binding_ref.clone(); @@ delivery_sink: self.services.delivery_sink.as_ref(), - require_direct_message_target: false, + require_direct_message_target, },As per coding guidelines, auth must fail closed and private URLs must not be exposed across public surfaces. The outbound-delivery contract also requires OAuth
authorization_urlpayloads to set this flag.🤖 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 539 - 595, The require_direct_message_target field in the ProductOutboundDeliveryRequest is hardcoded to false, preventing the authorization check from running for AuthPrompt payloads with authorization URLs. Compute the require_direct_message_target flag from the payload before it is moved into the ProductOutboundDeliveryRequest, checking if the payload contains an AuthPrompt with an authorization_url, and set the flag to true when such sensitive URLs are present. This ensures the ObservedSlackReplyTargetAuthority validation runs to prevent exposing OAuth URLs across public surfaces, matching the pattern used in the triggered delivery path.Source: Coding guidelines
crates/ironclaw_product_workflow/src/outbound_delivery.rs (1)
294-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDM-gate rejection is misclassified as
Unknownfailure kind.Line 301 currently drops
ProductWorkflowError::OutboundTargetNotDirectMessageinto_ => DeliveryFailureKind::Unknown. That error is a deterministic auth-policy denial (mapped to 403/non-retryable incrates/ironclaw_product_workflow/src/error.rs), so this should beRejectedfor fail-closed behavior.Suggested fix
fn delivery_failure_kind_for_workflow_error(error: &ProductWorkflowError) -> DeliveryFailureKind { match error { ProductWorkflowError::Transient { .. } => DeliveryFailureKind::TransportUnavailable, ProductWorkflowError::BindingAccessDenied | ProductWorkflowError::BindingRequired { .. } | ProductWorkflowError::UnknownInstallation - | ProductWorkflowError::InvalidBindingRequest { .. } => DeliveryFailureKind::Rejected, + | ProductWorkflowError::InvalidBindingRequest { .. } + | ProductWorkflowError::OutboundTargetNotDirectMessage => DeliveryFailureKind::Rejected, _ => DeliveryFailureKind::Unknown, } }As per coding guidelines, “Fail closed for auth, approvals, trust … and event records.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_product_workflow/src/outbound_delivery.rs` around lines 294 - 302, The function delivery_failure_kind_for_workflow_error misclassifies ProductWorkflowError::OutboundTargetNotDirectMessage as Unknown failure kind via the catch-all pattern, but it should be classified as Rejected since it represents a deterministic auth-policy denial. Add ProductWorkflowError::OutboundTargetNotDirectMessage to the match arm that already contains ProductWorkflowError::BindingAccessDenied and related variants that return DeliveryFailureKind::Rejected to ensure auth denial errors fail closed as per coding guidelines.Source: Coding guidelines
crates/ironclaw_product_workflow/src/workflow.rs (1)
237-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
OutboundTargetNotDirectMessageis currently treated as retry/unsettled in terminal ack logic.Line 1604 places this deterministic policy error in the
Nonepath; Lines 249-255 then release idempotency state instead of settling a durable rejection. That conflicts with its non-retryable unauthorized mapping inerror.rs.Suggested fix
ProductWorkflowError::BeforeInboundPolicyFailed { reason, permanent: true, } => Some(ProductInboundAck::Rejected(ProductRejection::permanent( ProductRejectionKind::PolicyDenied, reason.clone(), ))), + ProductWorkflowError::OutboundTargetNotDirectMessage => { + Some(ProductInboundAck::Rejected(ProductRejection::permanent( + ProductRejectionKind::AccessDenied, + "outbound target is not a direct message but the payload requires one", + ))) + } ProductWorkflowError::BindingResolutionFailed { .. } | ProductWorkflowError::TurnSubmissionRejected { .. } | ProductWorkflowError::TurnSubmissionFailed { .. } | ProductWorkflowError::TurnResumeRejected { .. } | ProductWorkflowError::AuthContinuationRejected { .. } | ProductWorkflowError::ApprovalInteractionRejected { .. } | ProductWorkflowError::AuthInteractionRejected { .. } | ProductWorkflowError::TurnResumeDenied { .. } | ProductWorkflowError::Transient { .. } | ProductWorkflowError::BeforeInboundPolicyFailed { permanent: false, .. } - | ProductWorkflowError::OutboundTargetNotDirectMessage | ProductWorkflowError::DuplicateAction { .. } => None,As per coding guidelines, “Fail closed for auth, approvals, trust …” and invariant “Fail loud” for deterministic failures.
Also applies to: 1592-1605
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_product_workflow/src/workflow.rs` around lines 237 - 255, The OutboundTargetNotDirectMessage error is currently treated as retryable by falling into the else branch at line 249 which releases the idempotency ledger, but it should be treated as a deterministic terminal rejection that settles the ledger instead. Modify the terminal_ack_for_error function to return Some(ack) for OutboundTargetNotDirectMessage (rather than None) so it follows the settlement path in the if let Some(ack) block at lines 237-248, ensuring consistency with its non-retryable unauthorized mapping defined in error.rs and aligning with the fail-closed-for-auth and fail-loud-for-deterministic-failures guidelines.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_product_workflow/tests/outbound_delivery_contract.rs`:
- Around line 220-224: The `_require_direct_message` parameter in the
`resolve_product_outbound_target_metadata` method is currently ignored due to
the underscore prefix. Remove the underscore prefix to make it
`require_direct_message` and then add test assertions within this resolver to
verify that the flag value is properly propagated through the call chain to
`prepare_and_render_product_outbound`. This ensures the test will catch any
regression where the flag stops being forwarded in the production code.
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Line 2510: The `#[allow(clippy::too_many_arguments)]` attribute at line 2510
is missing the required architecture exemption comment. Add a comment in the
format `// arch-exempt: too_many_args, <reason naming the missing aggregation>,
plan `#NNNN`` immediately after the `#[allow(clippy::too_many_arguments)]`
attribute to satisfy the coding guidelines for clippy exemptions.
---
Outside diff comments:
In `@crates/ironclaw_product_workflow/src/outbound_delivery.rs`:
- Around line 294-302: The function delivery_failure_kind_for_workflow_error
misclassifies ProductWorkflowError::OutboundTargetNotDirectMessage as Unknown
failure kind via the catch-all pattern, but it should be classified as Rejected
since it represents a deterministic auth-policy denial. Add
ProductWorkflowError::OutboundTargetNotDirectMessage to the match arm that
already contains ProductWorkflowError::BindingAccessDenied and related variants
that return DeliveryFailureKind::Rejected to ensure auth denial errors fail
closed as per coding guidelines.
In `@crates/ironclaw_product_workflow/src/workflow.rs`:
- Around line 237-255: The OutboundTargetNotDirectMessage error is currently
treated as retryable by falling into the else branch at line 249 which releases
the idempotency ledger, but it should be treated as a deterministic terminal
rejection that settles the ledger instead. Modify the terminal_ack_for_error
function to return Some(ack) for OutboundTargetNotDirectMessage (rather than
None) so it follows the settlement path in the if let Some(ack) block at lines
237-248, ensuring consistency with its non-retryable unauthorized mapping
defined in error.rs and aligning with the fail-closed-for-auth and
fail-loud-for-deterministic-failures guidelines.
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 539-595: The require_direct_message_target field in the
ProductOutboundDeliveryRequest is hardcoded to false, preventing the
authorization check from running for AuthPrompt payloads with authorization
URLs. Compute the require_direct_message_target flag from the payload before it
is moved into the ProductOutboundDeliveryRequest, checking if the payload
contains an AuthPrompt with an authorization_url, and set the flag to true when
such sensitive URLs are present. This ensures the
ObservedSlackReplyTargetAuthority validation runs to prevent exposing OAuth URLs
across public surfaces, matching the pattern used in the triggered delivery
path.
🪄 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: f9f62fea-23df-4731-8bb3-917109f00d9f
📒 Files selected for processing (6)
crates/ironclaw_product_workflow/src/error.rscrates/ironclaw_product_workflow/src/outbound_delivery.rscrates/ironclaw_product_workflow/src/reborn_services/lifecycle_setup.rscrates/ironclaw_product_workflow/src/workflow.rscrates/ironclaw_product_workflow/tests/outbound_delivery_contract.rscrates/ironclaw_reborn_composition/src/slack_delivery.rs
#4953 multi-reviewer pass on the typed-data refactor: - `delivery_failure_kind_for_workflow_error`: `OutboundTargetNotDirectMessage` now maps to `Rejected` (deterministic non-retryable denial) instead of falling through to `Unknown`, fixing the outbound audit-trail class. - Extracted the duplicated DM guard into a shared `enforce_direct_message_if_required(target, require_direct_message)` helper; both Slack resolvers call it (one source of truth). - arch-exempt annotation on the new `too_many_arguments` allow. - Dead `OAuthTargetNotDm` arm in the outer failure match → `unreachable!()`. - Tests: contract-level coverage that `require_direct_message_target: true` threads through `prepare_and_render_product_outbound` to the resolver and surfaces the typed error (+ the false case); 3 unit tests for the shared DM-guard helper (shared-channel/true→Err, /false→Ok, DM/true→Ok). 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 1416-1421: Replace the boolean parameter `require_direct_message`
in the function `enforce_direct_message_if_required` with a typed enum called
`DirectMessageTargetRequirement` that has two variants: `Required` and
`NotRequired`. Create this enum type and update the condition check to compare
against `DirectMessageTargetRequirement::Required` instead of checking a boolean
true value. Apply this same enum type change to the
`ProductOutboundDeliveryRequest` and `ProductOutboundTargetResolver` structures,
and update all call sites throughout the file (including those around lines
1452-1462, 2521-2530, 2572-2583, 2697-2708, and 7510-7547) to pass the enum
variants instead of boolean literals.
🪄 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: 3c57d7ab-e41f-4a8f-b25a-b6a9754b07f6
📒 Files selected for processing (3)
crates/ironclaw_product_workflow/src/outbound_delivery.rscrates/ironclaw_product_workflow/tests/outbound_delivery_contract.rscrates/ironclaw_reborn_composition/src/slack_delivery.rs
Comment on lines
+1416
to
+1421
| fn enforce_direct_message_if_required( | ||
| target: &ReplyTargetBindingRef, | ||
| require_direct_message: bool, | ||
| ) -> Result<(), ProductWorkflowError> { | ||
| if require_direct_message && !slack_reply_target_is_personal_dm(target) { | ||
| return Err(ProductWorkflowError::OutboundTargetNotDirectMessage); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use a typed DM requirement, not a boolean security mode.
require_direct_message / require_direct_message_target is the switch that decides whether an OAuth authorization_url can leave a personal DM. Make this an enum such as DirectMessageTargetRequirement::{Required, NotRequired} across ProductOutboundDeliveryRequest and ProductOutboundTargetResolver so new call sites cannot silently choose the unsafe-looking false literal.
As per coding guidelines, “Use enums for units, shapes, and modes … instead of booleans plus magic strings.”
Also applies to: 1452-1462, 2521-2530, 2572-2583, 2697-2708, 7510-7547
🤖 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 1416 -
1421, Replace the boolean parameter `require_direct_message` in the function
`enforce_direct_message_if_required` with a typed enum called
`DirectMessageTargetRequirement` that has two variants: `Required` and
`NotRequired`. Create this enum type and update the condition check to compare
against `DirectMessageTargetRequirement::Required` instead of checking a boolean
true value. Apply this same enum type change to the
`ProductOutboundDeliveryRequest` and `ProductOutboundTargetResolver` structures,
and update all call sites throughout the file (including those around lines
1452-1462, 2521-2530, 2572-2583, 2697-2708, and 7510-7547) to pass the enum
variants instead of boolean literals.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
Valid concern, but deferring intentionally. This bool becomes a typed visibility enum as part of the cross-adapter generalization (lifting the "OAuth URL → personal-DM only" gate out of the Slack-specific path into a shared outbound contract, done when we add the next adapter — Telegram/Discord). At that point the flag is replaced by OutboundTargetVisibility { Unrestricted, PersonalDmOnly } on the shared VerifiedProductOutboundTargetMetadata / resolver contract — which is the same fail-closed safety this asks for, under the name the generalization will use rather than a Slack/OAuth-flavored DirectMessageTargetRequirement that we'd immediately rename. Keeping this PR scoped to the Slack fix. Tracked.
There was a problem hiding this comment.
@henrypark133 acknowledged. The invariant concern stands (“Use enums for units, shapes, and modes”), but I’m good with deferring here since the intended follow-up type is the broader cross-adapter contract shape, not a Slack-local enum that would churn immediately.
I’ll treat require_direct_message_target: bool in this PR as a scoped interim for the Slack security fix, with the expected tracked migration to OutboundTargetVisibility { Unrestricted, PersonalDmOnly } on the shared resolver/metadata contract.
🐇↪️
✏️ Learnings added
Learnt from: henrypark133
Repo: nearai/ironclaw PR: 4953
File: crates/ironclaw_reborn_composition/src/slack_delivery.rs:1416-1421
Timestamp: 2026-06-17T04:20:06.501Z
Learning: In nearai/ironclaw, the `require_direct_message_target` / `require_direct_message` boolean added around PR `#4953` for Slack OAuth URL personal-DM enforcement is an intentional interim scoped to the Slack fix. Future cross-adapter generalization for Telegram/Discord should replace it with a typed shared visibility contract named `OutboundTargetVisibility { Unrestricted, PersonalDmOnly }` on `VerifiedProductOutboundTargetMetadata` / the outbound target resolver contract, rather than introducing a Slack/OAuth-flavored `DirectMessageTargetRequirement` enum.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: zmanian
Repo: nearai/ironclaw PR: 4559
File: crates/ironclaw_reborn_composition/src/runtime.rs:4763-4859
Timestamp: 2026-06-15T13:25:49.116Z
Learning: In this crate’s Trace Commons autonomous capture tests, trace attribution must use the event-derived composite scope `trace_scope_key(tenant, owner)`. When validating behavior for non-runtime-owner callers, the tests should fail if attribution is set using only a bare `owner` id (without `tenant`) or if attribution targets a runtime-wide owner scope instead of the caller’s non-runtime owner scope.
Learnt from: henrypark133
Repo: nearai/ironclaw PR: 4953
File: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2520-2520
Timestamp: 2026-06-17T04:13:18.772Z
Learning: When suppressing Clippy with `#[allow(clippy::too_many_arguments)]`, require an “architecture exemption” comment immediately above the attribute that explains why the many-arguments signature is justified (e.g., what bundled concepts the function needs) and provides a concrete rationale for the exemption. Avoid silent `#[allow(...)]` for this lint.
henrypark133
commented
Jun 17, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Restrict Slack OAuth authorization_url delivery to verified personal DMs and suppress it on shared-channel or unverified triggered-run targets.
Stats: 2 findings (from 4 raw, 2 after validation/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Tests
-
Medium No direct test covers the new adapter-error mapping (
crates/ironclaw_product_workflow/src/error.rs:246-255, confidence 91) — anchor:crates/ironclaw_product_workflow/src/error.rs:246
The newProductWorkflowError::OutboundTargetNotDirectMessagearm maps to a 403WorkflowRejected, but nothing in this diff asserts that conversion. If this arm regresses, callers that rely onProductAdapterErrorwill misclassify the failure and may stop failing closed. -
Medium New terminal-ack exclusion branch has no regression test (
crates/ironclaw_product_workflow/src/workflow.rs:1604-1605, confidence 88) — anchor:crates/ironclaw_product_workflow/src/workflow.rs:1604
terminal_ack_for_errornow treatsOutboundTargetNotDirectMessageas unsettled (None), but the existing tests only cover unsupported actions and other retryable paths. Without a direct assertion, this branch can silently start emitting a terminal inbound ack and change settlement semantics.
| ProductWorkflowError::UnsupportedActionKind { kind } => ProductAdapterError::Internal { | ||
| detail: RedactedString::new(format!("unsupported action kind: {kind}")), | ||
| }, | ||
| ProductWorkflowError::OutboundTargetNotDirectMessage => { |
Collaborator
Author
There was a problem hiding this comment.
Medium — No direct test covers the new adapter-error mapping.
The new ProductWorkflowError::OutboundTargetNotDirectMessage arm maps to a 403 WorkflowRejected, but nothing in this diff asserts that conversion. If this arm regresses, callers that rely on ProductAdapterError will misclassify the failure and may stop failing closed.
Fix: Add tests::error::outbound_target_not_direct_message_maps_to_workflow_rejected covering ProductWorkflowError::OutboundTargetNotDirectMessage -> ProductAdapterError::WorkflowRejected(Unauthorized, 403, non-retryable).
Collaborator
Author
There was a problem hiding this comment.
Added in 0caf216: tests::error::outbound_target_not_direct_message_maps_to_workflow_rejected asserts OutboundTargetNotDirectMessage -> WorkflowRejected{Unauthorized, 403, non-retryable}. Passing.
| | ProductWorkflowError::BeforeInboundPolicyFailed { | ||
| permanent: false, .. | ||
| } | ||
| | ProductWorkflowError::OutboundTargetNotDirectMessage |
Collaborator
Author
There was a problem hiding this comment.
Medium — New terminal-ack exclusion branch has no regression test.
terminal_ack_for_error now treats OutboundTargetNotDirectMessage as unsettled (None), but the existing tests only cover unsupported actions and other retryable paths. Without a direct assertion, this branch can silently start emitting a terminal inbound ack and change settlement semantics.
Fix: Add tests::workflow::terminal_ack_for_error_keeps_outbound_target_not_direct_message_unsettled covering terminal_ack_for_error(ProductWorkflowError::OutboundTargetNotDirectMessage) == None.
Collaborator
Author
There was a problem hiding this comment.
Added in 0caf216: tests::workflow::terminal_ack_for_error_keeps_outbound_target_not_direct_message_unsettled asserts terminal_ack_for_error(OutboundTargetNotDirectMessage) == None. Passing.
…rminal-ack exclusion Add the two regression tests requested in #4953 review: - error: OutboundTargetNotDirectMessage -> WorkflowRejected{Unauthorized,403,non-retryable} - workflow: terminal_ack_for_error keeps OutboundTargetNotDirectMessage unsettled (None) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 17, 2026
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…nearai#4953) * fix(reborn): gate triggered Slack OAuth URL on a verified personal DM Security follow-up from PR nearai#4946 review. On the triggered-run delivery path, a `BlockedAuth` run with a link-based OAuth challenge posted the OAuth `authorization_url` into Slack assuming the target was the creator's private DM. The triggered target resolves from the creator's personal communication preference and is NOT guaranteed to be a DM — shared-channel outbound targets advertise `auth_prompts: true` (`slack_outbound_targets.rs` `entry_for_shared_channel_route`), so the OAuth setup URL could leak onto a shared Slack surface. Fix (fail closed): OAuth `authorization_url` may only be posted to a verified personal DM. - Add `slack_reply_target_is_personal_dm()` in `slack_outbound_targets.rs`: strictly parses the binding-ref segment chain and returns true only for a personal-DM ref (trailing `actor_kind=slack_user`/`actor` segments present AND a `D`-prefixed channel id). Any parse failure → false. - In `deliver_triggered_run`, resolve the creator's preference once and compute `target_is_verified_dm` against the EFFECTIVE auth target — `auth_prompt_target.or(final_reply_target)`, mirroring `resolution_engine.rs` `PreferenceTargetKind::AuthPrompt`. A looser "any stored target is a DM" check would wrongly pass when `auth_prompt_target` is a shared channel but `final_reply_target` is a DM. Preference read failure / no target → false (fail closed). - The `BlockedAuth` arm keeps `authorization_url` only when `target_is_verified_dm`; otherwise it cancels the run and posts the existing `SLACK_AUTH_UNAVAILABLE_MESSAGE` notice (same as the non-OAuth manual-token path). Tests: shared-channel target suppresses the URL + posts the notice; personal-DM target still posts the URL; auth_prompt_target=shared with final_reply_target=DM suppresses (precedence guard); 5 unit tests for the DM-detection helper. Note: `slack_host_beta::…wires_trigger_delivery_hook_writes_record` is a pre-existing flaky (timeout under parallel load; flakes on origin/main too), unrelated to this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): close DM-gate race with a send-time authority backstop Addresses nearai#4953 review. The OAuth-URL DM gate was a snapshot read once before the triggered poll loop, but delivery re-resolves the target from `communication_preferences` at send time. If the creator's preference flipped DM→shared-channel while the run waited in BlockedAuth, the stale-true snapshot let the OAuth `authorization_url` get built and posted to the channel (codex P1 / coderabbit Critical). Backstop (airtight, in-crate): `TriggeredSlackReplyTargetAuthority` gains `require_personal_dm_for_oauth`, set per-delivery when the payload carries an `authorization_url`. `resolve_product_outbound_target_metadata` — which sees the EXACT binding resolved at send time — fails closed when the flag is set and the binding is not a personal DM. On trip, the run is canceled and the `SLACK_AUTH_UNAVAILABLE_MESSAGE` notice is posted (graceful deny, matching the non-OAuth path); the URL is never posted. Also from review: - warn! → debug! on the two new background-delivery diagnostics (CLAUDE.md: background tasks must not warn!). - arch-exempt annotation on `triggered_notification_for_state`'s too_many_arguments allow. - helper tests: wrong actor_kind + trailing-segment rejection. Tests: snapshot-DM-but-channel-at-send suppresses the URL (race); preference-read error fails closed; existing DM/shared/precedence cases retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): harden backstop outcome + DRY binding-ref parse (review) nearai#4953 review round 2: - coderabbit (Major): the OAuth backstop arm recorded `Delivered` unconditionally, swallowing both `cancel_auth_blocked_run` failure and the notice-delivery result — which could leave the run blocked / the user silent while the store claims success. Now: cancel failure records `Failed` and returns; the notice delivery result is mapped to the matching `TriggeredRunDeliveryOutcomeKind` (Delivered / NoDefaultConfigured / Denied / Failed). - henrypark (Medium): `slack_reply_target_is_personal_dm` and `slack_conversation_id_from_reply_target_binding_ref` independently walked the same reply-target segment format. Factored a single `decode_slack_reply_target_binding_ref` → `DecodedSlackReplyTarget`; both functions now derive from it (behavior-preserving). - henrypark (Low): corrected the `classify_delivery_error` comment that described a `to_string().contains` fallback the code never had — the resolver error is matched directly as `Workflow { source: BindingResolutionFailed { reason } }`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): harden OAuth DM gate — adapter, owner key, cleanup (review) nearai#4953 review round 3: - henrypark (High): the OAuth DM pre-check keyed the preference by `actor.user_id`, but this path + tests seed/resolve by `scope.explicit_owner_user_id()`. Owner-scoped runs (acting principal ≠ owner) read the wrong record and over-suppressed. Key now uses `explicit_owner_user_id().unwrap_or(actor.user_id)`. - coderabbit (Major, security): the shared `decode_slack_reply_target_binding_ref` consumed the `adapter` segment without validating it. A non-Slack `reply:` binding shaped like a DM could pass `slack_reply_target_is_personal_dm` (the OAuth-URL gate). Now fails closed unless `adapter == SLACK_V2_ADAPTER_ID`. + wrong-adapter regression test. - coderabbit (Major, security): the OAuth backstop terminal path skipped the prior-prompt cleanup, leaving a stale `authorization_url` DM after cancel. Now drains `messages_to_delete_after_final` before returning. - tests: no-preference (`Ok(None)`) fail-closed suppression; missing-`topic` rejection. Note: the two-state backstop-deletes-prior-DM-prompt regression test is a known gap (needs DM-then-channel preference + dual BlockedAuth scripted states); the cleanup code itself is covered by review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): replace OAuth-DM sentinel string with a typed signal nearai#4953 review (henrypark, Medium): the OAuth-DM backstop classified its trip by matching a magic reason string (`OAUTH_TARGET_NOT_DM_MARKER`) on the cross-crate `ProductWorkflowError` inside `classify_delivery_error`. Replace that with a typed handshake: the authority sets a dedicated `oauth_target_not_dm: AtomicBool` on trip, and `deliver_triggered_notification` reads-and-clears it to return `TriggeredNotificationFailure::OAuthTargetNotDm` before falling through to `classify_delivery_error`. The sentinel const and its string-match arm are removed. Behavior is unchanged (the resolver still returns a plain, human-readable `BindingResolutionFailed` reason and the URL is never posted to a non-DM); the classification no longer depends on string contents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): backstop delete-after-cancel + reject empty-space DM refs (review) nearai#4953 review round 4 (henrypark, Medium ×2): - Backstop ordering: the OAuth backstop arm deleted prior auth-prompt messages BEFORE `cancel_auth_blocked_run`. A transient cancel failure then recorded `Failed` having already removed the prompt the user could have used. Now: cancel first (on failure, record Failed and return without deleting); delete the stale prompts only after a successful cancel and after the replacement notice has been attempted. - DM gate: `slack_reply_target_is_personal_dm` accepted a ref with an empty `space` segment (`space:0:` → space_id None). A forged/corrupted preference missing the team/space binding could satisfy the OAuth-DM gate. Now rejects `space_id.is_none()`. + `space:0:` regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): single-source the OAuth-DM gate at the resolver nearai#4953 review (henrypark, Medium + maintainability + performance): the DM rule lived in two places — a pre-loop `target_is_verified_dm` preference snapshot AND the send-time resolver backstop. Drop the snapshot; the send-time `TriggeredSlackReplyTargetAuthority` resolver is now the single place that decides whether the current binding is a personal DM. `prepare_and_render_product_outbound` runs the resolver BEFORE it renders/posts, so a non-DM OAuth delivery fails closed before the URL is ever posted — the snapshot was only an optimization. The `BlockedAuth` arm now builds the OAuth AuthPrompt whenever `authorization_url` is present and lets the backstop suppress it (cancel + auth-unavailable notice) for non-DM targets. Removes: the ~57-line snapshot block, the `target_is_verified_dm` param (triggered_notification_for_state is back under the arg limit, so its too_many_args allow is gone too), the owner-key branch, and 2 snapshot-specific tests + their 4 preference-double helpers. The shared-channel suppression case is still covered end-to-end via the backstop. 65/65 slack_delivery tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): correct non-OAuth triggered-auth outcome + tighten decoder (review) Multi-reviewer code-review pass on nearai#4953: - BUG (correctness): a triggered run that hit a NON-OAuth (manual credential) auth gate posted the auth-unavailable notice and canceled the run, but recorded `Skipped` instead of `Delivered`. The deny branch returns a terminal `FinalReply`, yet the loop saw `next_blocked_marker` derived from the blocked *state* and continued; the next poll (Cancelled) returned `Ok(None)` → `Skipped`. Now the keep-waiting `continue` only fires for blocked *prompt* notifications (`ApprovalNeeded`/`AuthRequired`), so terminal `FinalReply` deliveries fall through to `Delivered`. - actor_id: the decoder stored `Some("")` for an empty `actor` segment, contradicting its doc-comment. Now mapped to `None` at assignment; the DM predicate's actor check simplifies to `actor_id.is_some()`. Identical behavior, honest type. - Documented that `slack_conversation_id_from_reply_target_binding_ref` now enforces adapter identity via the shared decoder. - Removed dead `_thread_id` binding. Tests: non-OAuth denial records Delivered (pins the bug); OAuth backstop cancel-failure records Failed with no stale-prompt deletion; missing-actor -segment rejection; non-Slack-adapter → None at the conversation-id caller. Note: the two-state "backstop deletes a prior DM auth prompt" e2e test remains a known harness-heavy gap (needs a preference double that flips DM→channel between resolutions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): replace OAuth-DM atomic handshake with typed data + error nearai#4953 code-review (multi-reviewer): the OAuth-DM requirement was threaded between the delivery loop and the target resolver via two AtomicBool scratch fields on TriggeredSlackReplyTargetAuthority (require_personal_dm_for_oauth in, oauth_target_not_dm out, read-and- cleared, plus a manual reset) — a side-channel forced by the resolver trait being unable to carry the requirement or return a typed reason. Replace with data + a typed error: - ProductOutboundDeliveryRequest gains `require_direct_message_target`; the resolver trait method gains a `require_direct_message` param; prepare_and_render threads the flag through. - New typed ProductWorkflowError::OutboundTargetNotDirectMessage; both Slack resolvers (triggered + live, defense in depth) fail closed with it when require_direct_message && !is_personal_dm. - deliver_triggered_notification derives the flag from the payload and classify_delivery_error maps the typed variant → OAuthTargetNotDm. Both AtomicBools, the read-and-clear, the manual reset, and the per- delivery store are gone. Behavior identical; the URL still never reaches a non-DM target. Rippled into 3 exhaustive arms (ProductAdapterError 403, internal_invariant, terminal_ack None). Tests: 67 slack_delivery, 25 slack_outbound_targets, 24 outbound_delivery _contract, full product_workflow suite; 4 named OAuth security tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): code-review polish — failure-kind, shared guard, tests nearai#4953 multi-reviewer pass on the typed-data refactor: - `delivery_failure_kind_for_workflow_error`: `OutboundTargetNotDirectMessage` now maps to `Rejected` (deterministic non-retryable denial) instead of falling through to `Unknown`, fixing the outbound audit-trail class. - Extracted the duplicated DM guard into a shared `enforce_direct_message_if_required(target, require_direct_message)` helper; both Slack resolvers call it (one source of truth). - arch-exempt annotation on the new `too_many_arguments` allow. - Dead `OAuthTargetNotDm` arm in the outer failure match → `unreachable!()`. - Tests: contract-level coverage that `require_direct_message_target: true` threads through `prepare_and_render_product_outbound` to the resolver and surfaces the typed error (+ the false case); 3 unit tests for the shared DM-guard helper (shared-channel/true→Err, /false→Ok, DM/true→Ok). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): cover OutboundTargetNotDirectMessage error mapping + terminal-ack exclusion Add the two regression tests requested in nearai#4953 review: - error: OutboundTargetNotDirectMessage -> WorkflowRejected{Unauthorized,403,non-retryable} - workflow: terminal_ack_for_error keeps OutboundTargetNotDirectMessage unsettled (None) 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 was referenced Jul 3, 2026
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
Security follow-up from #4946 review (coderabbit). On the triggered-run Slack delivery path, a
BlockedAuthrun with a link-based OAuth challenge posted the OAuthauthorization_urlinto Slack on the assumption that a triggered run always delivers to the creator's private DM.That assumption is not enforced: the triggered delivery target resolves from the creator's personal communication preference, and shared-channel outbound targets advertise
auth_prompts: true(slack_outbound_targets.rsentry_for_shared_channel_route). So if a creator's preference resolves to a shared channel, the OAuth setup URL leaks onto a shared Slack surface.Fix (fail closed)
OAuth
authorization_urlmay only be posted to a verified personal DM.slack_outbound_targets.rs— newslack_reply_target_is_personal_dm(): strictly parses the binding-ref segment chain, returningtrueonly for a personal-DM ref (trailingactor_kind=slack_user/actorsegments present and aD-prefixed channel id). Any parse failure →false.slack_delivery.rs—deliver_triggered_runresolves the creator's preference once and computestarget_is_verified_dmagainst the effective auth target:auth_prompt_target.or(final_reply_target), mirroringresolution_engine.rsPreferenceTargetKind::AuthPrompt. A looser "any stored target is a DM" check would wrongly pass whenauth_prompt_targetis a shared channel butfinal_reply_targetis a DM. Preference-read failure or no target →false(fail closed).BlockedAutharm keepsauthorization_urlonly whentarget_is_verified_dm; otherwise it cancels the run and posts the existingSLACK_AUTH_UNAVAILABLE_MESSAGEnotice (same treatment as the non-OAuth manual-token path).Tests
triggered_oauth_auth_to_shared_channel_suppresses_authorization_url— URL suppressed, notice posted, no gate route recorded.triggered_oauth_auth_to_personal_dm_posts_authorization_url— DM target still posts the URL.triggered_oauth_auth_prefers_auth_target_over_dm_fallback— precedence guard:auth_prompt_target=shared +final_reply_target=DM ⇒ suppressed.slack_reply_target_is_personal_dm(DM→true; shared→false; non-Dchannel→false; malformed→false; empty actor→false).cargo fmt/clippy --features slack-v2-host-beta --testsclean.Not in this PR
AuthFlowon Slack auto-deny (Slack triggered/auth auto-deny leaves AuthFlow record stale (bypasses cancel_flow) #4952): the auth auto-deny path cancels the run viaTurnCoordinatorbut skipsAuthFlowManager::cancel_flow. Wiring an auth-flow/auth-service handle intoSlackFinalReplyDeliveryServicesis a separate dependency change; tracked in Slack triggered/auth auto-deny leaves AuthFlow record stale (bypasses cancel_flow) #4952.🤖 Generated with Claude Code