Skip to content

feat(slack): implement on_broadcast and fix message tool hints - #2113

Merged
serrrfirat merged 3 commits into
stagingfrom
feat/slack-broadcast-and-message-hints
Apr 7, 2026
Merged

serrrfirat merged 3 commits into
stagingfrom
feat/slack-broadcast-and-message-hints

Conversation

@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

  • Implement on_broadcast in the Slack WASM channel (channels-src/slack/src/lib.rs): the agent can now send proactive messages to arbitrary Slack channels/users via the message tool. The user_id parameter carries the broadcast target (e.g. #general, C0123ABC, U0123); a leading # is stripped since the Slack API expects bare names or IDs.
  • Fix message tool channel parameter hint (src/tools/builtin/message.rs): replace 'slack-relay' with 'slack' and clarify that "target" refers to Slack channels (not "Slack channel ID").
  • Add 5 unit tests for the extracted helpers (resolve_broadcast_target, build_broadcast_payload).

Test plan

  • cargo test in channels-src/slack/ passes (16 tests including 5 new)
  • cargo clippy in channels-src/slack/ passes with zero warnings
  • cargo fmt in channels-src/slack/ passes
  • cargo clippy --all --benches --tests --examples --all-features passes in main repo
  • cargo test passes in main repo (pre-existing e2e_spot_checks SIGABRT confirmed on staging)
  • Manual: send a message via message tool targeting a Slack channel to verify end-to-end

🤖 Generated with Claude Code

@github-actions github-actions Bot added scope: tool/builtin Built-in tools size: M 50-199 changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels Apr 7, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the on_broadcast functionality for the Slack channel, enabling the agent to send proactive messages. It introduces helper functions for target resolution and payload construction, adds unit tests for these helpers, and updates the MessageTool schema description. A review comment suggests tracking these broadcasted messages in the active threads registry to ensure the agent can correctly handle subsequent replies in those threads.

.unwrap_or_else(|| "unknown".to_string())
));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Proactive messages sent via on_broadcast should be tracked in the active threads registry, similar to how on_respond handles them. This ensures that if a user replies to the proactive message, the agent will be able to receive and process the reply even if it isn't explicitly mentioned (as the bot is now a participant in that thread).

Suggested change
if let Some(ts) = response.thread_id.as_deref().or(slack_response.ts.as_deref()) {
track_active_thread(&target, ts)?;
}

@serrrfirat
serrrfirat force-pushed the feat/slack-broadcast-and-message-hints branch from 45a28f1 to 27a994e Compare April 7, 2026 15:57
Implement the on_broadcast callback for the Slack WASM channel, enabling
proactive message delivery to Slack channels/users via the message tool.
Previously this was a stub returning "not implemented".

The implementation:
- Uses the user_id parameter as the broadcast target (channel ID or user ID)
- Strips leading # from targets for convenience
- Warns when target doesn't look like a Slack ID (C/U/D/G prefix)
- Posts via chat.postMessage with host-injected Bearer token
- Tracks active threads for broadcast replies (consistent with on_respond)

Also fixes the message tool's channel parameter description to list 'slack'
alongside 'slack-relay' and clarifies that Slack targets must be IDs.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@serrrfirat
serrrfirat force-pushed the feat/slack-broadcast-and-message-hints branch from 27a994e to c61e279 Compare April 7, 2026 16:12
henrypark133
henrypark133 previously approved these changes Apr 7, 2026
Address review findings on the slack broadcast implementation:

- Extract `post_slack_message()` shared by `on_respond` and `on_broadcast`,
  eliminating ~40 lines of duplicated HTTP-call-then-parse logic.
- Log `track_active_thread` errors at Warn level instead of silently
  swallowing them with `let _ =` (restores observability lost in original).
- Make non-ID broadcast targets a hard error instead of a soft warning —
  consistent with the message tool schema that says "must be an ID, not a
  name".
- Fix empty-target error message to not assume a name was provided.
- `resolve_broadcast_target` now returns `&str` (avoids allocation).
- Add 2 tests covering the resolve+validate pipeline end-to-end.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size: L 200-499 changed lines and removed size: M 50-199 changed lines labels Apr 7, 2026
Address Gemini review: broadcast messages now track the Slack-returned
timestamp as an active thread (falling back from response.thread_id to
the posted message ts). This ensures that if a user replies to a
broadcast, the agent recognizes the reply as an active thread.

Also fix stale doc comment on looks_like_slack_id (was missing W prefix).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@serrrfirat
serrrfirat merged commit 86c1590 into staging Apr 7, 2026
14 checks passed
@serrrfirat
serrrfirat deleted the feat/slack-broadcast-and-message-hints branch April 7, 2026 18:23
@claude

claude Bot commented Apr 7, 2026

Copy link
Copy Markdown

Code review

Found 6 issues:

  1. [HIGH:75] Slack ID validation incomplete — only validates first two characters. A malicious target like C0\n<injected> would pass the looks_like_slack_id() check. While the immediate usage (JSON serialization) is safe, the weak validation creates a latent vulnerability if the target is ever used in string interpolation, SQL queries, or command construction elsewhere.

https://github.com/anthropics/ironclaw/blob/86c1590350c3e444fd20be1f17890447e57ac72f/channels-src/slack/src/lib.rs#L853-L860

Recommendation: Validate entire string with full alphanumeric check or use regex like ^[CUDGW][a-zA-Z0-9]+$.

  1. [HIGH:75] Behavioral change in error propagation undocumented — on_respond() previously returned an error if track_active_thread() failed (fatal). Now errors are logged as warnings and the function succeeds. This is a semantic change that affects thread tracking reliability and should be documented with an explanation of why thread-tracking failures are now non-fatal.

https://github.com/anthropics/ironclaw/blob/86c1590350c3e444fd20be1f17890447e57ac72f/channels-src/slack/src/lib.rs#L276-L283

  1. [MEDIUM:75] Inconsistent error handling across channels — Slack now silently logs thread-tracking failures while other channels (e.g., Telegram) propagate errors as fatal. This creates inconsistent semantics in the multi-channel abstraction. Either document that thread tracking is optional for Slack, or align the behavior.

https://github.com/anthropics/ironclaw/blob/86c1590350c3e444fd20be1f17890447e57ac72f/channels-src/slack/src/lib.rs#L276-L283

  1. [MEDIUM:50] Redundant prune_active_threads() call — track_active_thread() calls prune before and after inserting a new thread, but no mutations happen between the two calls. The second prune is redundant and causes O(256) duplicate work per message.

https://github.com/anthropics/ironclaw/blob/86c1590350c3e444fd20be1f17890447e57ac72f/channels-src/slack/src/lib.rs#L609-L616

Recommendation: Remove the second prune_active_threads() call after the insert.

  1. [LOW:75] Logging level misalignment with CLAUDE.md — Per CLAUDE.md, warn! output appears in the REPL and corrupts the terminal UI. Internal diagnostics like thread-tracking failures should use LogLevel::Debug, not Warn.

https://github.com/anthropics/ironclaw/blob/86c1590350c3e444fd20be1f17890447e57ac72f/channels-src/slack/src/lib.rs#L278-L281

  1. [LOW:50] Missing design documentation — on_broadcast() rejects non-ID targets (e.g., #general) with an error, but there's no comment explaining why names are prohibited (e.g., immutability, security, avoiding name-resolution latency). Add a brief comment documenting the design decision.

https://github.com/anthropics/ironclaw/blob/86c1590350c3e444fd20be1f17890447e57ac72f/channels-src/slack/src/lib.rs#L309-L315

@ironclaw-ci ironclaw-ci Bot mentioned this pull request Apr 10, 2026
@ironclaw-ci ironclaw-ci Bot mentioned this pull request Apr 18, 2026
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…i#2113)

* feat(slack): implement on_broadcast and fix message tool channel hints

Implement the on_broadcast callback for the Slack WASM channel, enabling
proactive message delivery to Slack channels/users via the message tool.
Previously this was a stub returning "not implemented".

The implementation:
- Uses the user_id parameter as the broadcast target (channel ID or user ID)
- Strips leading # from targets for convenience
- Warns when target doesn't look like a Slack ID (C/U/D/G prefix)
- Posts via chat.postMessage with host-injected Bearer token
- Tracks active threads for broadcast replies (consistent with on_respond)

Also fixes the message tool's channel parameter description to list 'slack'
alongside 'slack-relay' and clarifies that Slack targets must be IDs.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(slack): extract shared post helper, harden broadcast validation

Address review findings on the slack broadcast implementation:

- Extract `post_slack_message()` shared by `on_respond` and `on_broadcast`,
  eliminating ~40 lines of duplicated HTTP-call-then-parse logic.
- Log `track_active_thread` errors at Warn level instead of silently
  swallowing them with `let _ =` (restores observability lost in original).
- Make non-ID broadcast targets a hard error instead of a soft warning —
  consistent with the message tool schema that says "must be an ID, not a
  name".
- Fix empty-target error message to not assume a name was provided.
- `resolve_broadcast_target` now returns `&str` (avoids allocation).
- Add 2 tests covering the resolve+validate pipeline end-to-end.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(slack): track broadcast message ts so replies are recognized

Address Gemini review: broadcast messages now track the Slack-returned
timestamp as an active thread (falling back from response.thread_id to
the posted message ts). This ensures that if a user replies to a
broadcast, the agent recognizes the reply as an active thread.

Also fix stale doc comment on looks_like_slack_id (was missing W prefix).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: tool/builtin Built-in tools size: L 200-499 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants