Skip to content

feat(slack): port Discord parity — mentions, multibot, bot turn limits#487

Merged
thepagent merged 14 commits into
openabdev:mainfrom
dogzzdogzz:feat/slack-parity-bundle
Apr 22, 2026
Merged

feat(slack): port Discord parity — mentions, multibot, bot turn limits#487
thepagent merged 14 commits into
openabdev:mainfrom
dogzzdogzz:feat/slack-parity-bundle

Conversation

@dogzzdogzz
Copy link
Copy Markdown
Contributor

@dogzzdogzz dogzzdogzz commented Apr 20, 2026

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1494739741600387204/1495409492089769984

Summary

Ports recent Discord-side work to the Slack adapter so both platforms share the same safety and feature surface, plus a Discord race-condition fix. Slack slash-command support was attempted and then removed — see row 4 below.

# Area What changes Why
1 Mention resolution strip_slack_mentionresolve_slack_mentions(text, bot_id): strips only the bot's own <@UID>, preserves other user mentions. Correctness: the old function wiped every <@…>, so the LLM could never @-mention users in its reply (Discord's resolve_mentions already works this way).
2 MultibotMentions AllowUsers::MultibotMentions was accepted by config but fell through to Involved with no other-bot check. Added a multibot_threads cache populated eagerly from incoming bot events, widened bot_participated_in_thread to return (involved, other_bot), and split the gating arm. Advertised behaviour now actually fires — mirrors Discord #481 / #483.
3 Bot turn limits Ported the soft/hard BotTurnTracker to Slack, running before self-check so every bot message counts. Reset on human message. New max_bot_turns field on SlackConfig (default 20). Discord's local copy moves into a new src/bot_turns.rs so both adapters share it. Safety: Slack allowed bot-to-bot loops with only a 10-msg cap; now matches Discord's soft+hard 100 cap with human-reset.
4 Slash commands Not supported on Slack. The Slack adapter ack-and-drops slash_commands / interactive envelopes with a debug log. Discord slash commands (/models, /agents, /cancel) are unchanged. Slack's UI blocks third-party slash commands invoked from a thread reply composer ("/cmd is not supported in threads. Sorry!"), and the channel-level payload carries no thread_ts so it can't route to a per-thread ACP session. Most ACP agents (notably claude-code) also don't emit configOptions for model/agent switching, making the menu empty even when routing works. The feature was added and then removed in this same PR (see the revert(slack): remove slash command support commit) because there was no working path end-to-end.
5 Docs / example config Updated config.toml.example and docs/slack-bot-howto.md with max_bot_turns and the multibot-mentions variant. (Slash-command registration steps were added and later removed in line with row 4.) User-facing surface changed.
6 Chore Added CLAUDE.md to .gitignore (local AI context file shouldn't land in the repo).
7 Discord bug fix On create_thread_from_message failing with error 160004 ("A thread has already been created for this message"), refetch the trigger message and join the thread the sibling bot just created instead of giving up. Reproduced in prod: "@bob say hi to @patrick" → both bots race to create a thread from the same trigger; the loser used to log ERROR failed to create thread: A thread has already been created for this message and drop the turn.

Test plan

  • cargo clippy --all-targets -- -D warnings — clean
  • cargo test81 pass / 0 fail (8 Slack tests + 3 Discord tests for the thread-race matcher + 12 BotTurnTracker tests relocated to the shared module)
  • Smoke test: start a bot-to-bot loop with allow_bot_messages = "all" and max_bot_turns = 5 → loop halts at the soft cap with a warning; human message resets
  • Smoke test: allow_user_messages = "multibot-mentions" with two bots in a thread → @mention required after the second bot posts
  • Smoke test: Discord message @bot-a say hi to @bot-b → both bots post in the same thread (no failed to create thread error)

Commit history

  1. feat(slack): port Discord parity — mentions, multibot, turn limits, slash commands — initial bundle
  2. chore: ignore CLAUDE.md
  3. fix(discord): join existing thread when racing a sibling bot
  4. feat: force send-once per thread when a trigger message tags other members — experimental (reverted)
  5. Revert "feat: force send-once per thread when a trigger message tags other members" — revert of fix: pull --rebase before push in bump-chart #4
  6. revert(slack): remove slash command support — row 4 above

Net effect vs main: rows 1, 2, 3, 5, 6, 7 are live; row 4 is a no-op in the diff.

Notes for reviewers

  • Single PR per author request. History has a noisy pair (force send-once + its revert) that cancels out in the diff.
  • Helm chart isn't updated — Slack's max_bot_turns falls back to the Rust default (20), same as Discord currently does in the chart.
  • multibot-mentions validation already exists in charts/openab/templates/configmap.yaml:84 — no chart change needed to unlock the Slack behaviour.
  • Row 7's fix is matched by error-string heuristic (160004 or "already been created"); covered by unit tests so serenity formatting changes will be caught.
  • If Slack eventually removes the thread restriction on third-party slash commands, restoring row 4 is straightforward — the deleted helpers live only in git history.

🤖 Generated with Claude Code

…lash commands

Extend the Slack adapter to match recent Discord work so both platforms share
the same safety/feature surface.

**Mention resolution (correctness fix)**: `strip_slack_mention` wiped every
`<@uid>`, which meant the LLM could never @-mention users back. Replaced with
`resolve_slack_mentions(text, bot_id)` that strips only the bot's own trigger
mention and preserves other user mentions — mirrors Discord's `resolve_mentions`.

**MultibotMentions completion**: the `AllowUsers::MultibotMentions` enum variant
was accepted by config but fell through to `Involved` with no other-bot check.
Added an eager `multibot_threads` cache populated from incoming bot events
(matches Discord openabdev#481 ordering) and split the gating arm to require @mention
when another bot has posted in the thread.

**Bot turn limits**: ported the soft/hard `BotTurnTracker` to Slack, running
before self-check so every bot message counts (matches Discord openabdev#483). Reset
on human message. Added `max_bot_turns` to `SlackConfig` (default 20). The
tracker moves to a new shared `bot_turns` module so Discord and Slack use the
same implementation; Discord's local copy and tests are removed.

**Slash commands**: handle Socket Mode `slash_commands` and `interactive`
envelope types. `/models`, `/agents`, `/cancel` surface ACP `configOptions`
via a `static_select` block, using `response_url` for the ephemeral reply
and follow-up. Requires the user to register the commands in the Slack app
manifest — added guidance in `docs/slack-bot-howto.md`.

Tests: 82 pass (8 new for mention resolution, truncate, select-block build,
plus the existing 12 `BotTurnTracker` tests relocated to the shared module).
`cargo clippy --all-targets -- -D warnings` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@dogzzdogzz dogzzdogzz requested a review from thepagent as a code owner April 20, 2026 09:03
@github-actions github-actions Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label Apr 20, 2026
Local AI assistant context file — shouldn't land in the repo.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@github-actions github-actions Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label Apr 20, 2026
When a user @-mentions two bots in the same message, both bots try to
create a thread from that trigger message. Discord allows only one thread
per message, so the second bot's `create_thread_from_message` call fails
with error code 160004 ("A thread has already been created for this
message") and the bot gives up.

On that specific error, refetch the trigger message, read `message.thread`,
and continue with the thread the sibling just created. Other errors still
propagate unchanged, so we don't silently mask permission failures or rate
limits.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@shaun-agent
Copy link
Copy Markdown
Contributor

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

Screening report ## Intent

This PR is trying to close the feature and safety gap between the Slack adapter and the Discord adapter. The concrete operator-visible problem is that Slack currently behaves differently in several important areas: it strips all mentions instead of only the bot's own mention, does not correctly enforce the advertised multibot-mentions behavior, has weaker bot-loop protection, lacks slash-command UX that Discord already has, and still leaves a known Discord thread-race bug unresolved.

In practical terms, the item is about making Slack safer to run in multi-bot environments and more usable for admins and end users, while also fixing one production-relevant Discord edge case discovered during parity testing.

Feat

This is primarily a feature-parity PR with bundled safety fixes, one bug fix, and accompanying docs/config updates.

Behaviorally, it does five real things:

  • Makes Slack mention resolution preserve user mentions while stripping only the current bot's own mention.
  • Implements actual MultibotMentions gating logic on Slack instead of silently falling back to a looser mode.
  • Ports the shared bot turn-limit logic to Slack so bot-to-bot loops are bounded the same way as Discord.
  • Adds Slack slash-command handling for /models, /agents, and /cancel.
  • Fixes a Discord race where two bots can try to create the same thread and one used to drop the turn.

Classification: feature + fix + docs improvement, bundled into one PR.

Who It Serves

The primary beneficiaries are Slack deployers and multi-bot runtime operators, because they need Slack to behave predictably and safely under the same policies already established on Discord.

Secondary beneficiaries are:

  • Slack end users, who get usable mentions and slash-command controls
  • Maintainers and reviewers, who benefit from shared bot-turn logic and reduced platform drift
  • Discord operators, because the thread-race bug fix prevents dropped turns in real multi-bot use

Rewritten Prompt

Bring the Slack adapter to parity with current Discord behavior in four scoped areas: mention resolution, multibot mention gating, bot turn limits, and slash-command handling. Preserve non-bot user mentions in Slack input, implement real multibot-mentions enforcement using thread participation state, reuse a shared BotTurnTracker module across Discord and Slack with configurable max_bot_turns, and add Socket Mode handling for /models, /agents, and /cancel using ephemeral select-based responses.

Also fix the Discord thread-creation race by detecting the "thread already exists" case, refetching the trigger message, and joining the existing thread instead of dropping the turn. Update config/docs for all new or exposed behavior, and cover each area with unit tests plus smoke-test notes for Slack command flow, multibot gating, loop stopping, and Discord dual-bot thread creation.

Merge Pitch

This item is worth advancing because it reduces platform inconsistency in a place where inconsistency directly creates operator confusion and runtime risk. OpenAB already has the intended safety model and interaction model on Discord; this PR mostly makes Slack stop being the odd one out.

The risk profile is moderate. The main reviewer concern will be bundling: multiple behavior changes across Slack plus a Discord fix in one PR makes regression review harder. The substance is good, but the merge discussion should focus on whether to land as one bundle or split into smaller reviewable slices.

Best-Practice Comparison

Relevant OpenClaw principles:

  • Explicit delivery routing: relevant. Mention resolution and multibot gating both affect whether the system responds to the right actor in the right context.
  • Retry/backoff and run logs: only partially relevant. The Discord thread-race fix is in the same reliability family, but this PR is not introducing broader retry or observability structure.
  • Gateway-owned scheduling: not relevant.
  • Durable job persistence: not relevant.
  • Isolated executions: not directly relevant, though bot turn limits do reduce cross-bot runaway behavior.

Relevant Hermes Agent principles:

  • Fresh session per scheduled run: not relevant.
  • Self-contained prompts for scheduled tasks: not relevant.
  • File locking to prevent overlap: conceptually relevant to the Discord race fix, in the sense that overlapping work must be handled safely, but this PR resolves overlap by fallback recovery rather than locking.
  • Atomic writes for persisted state: only lightly relevant if the new Slack thread/bot caches are persisted, which does not appear to be the core design here.
  • Gateway daemon tick model: not relevant.

Best-practice read:

  • The strongest alignment is with operational safety and explicit routing, not with scheduling architecture.
  • The Slack bot-turn tracker and multibot gating move OpenAB toward clearer execution boundaries.
  • The Discord race fix is pragmatic and production-oriented, but it is still heuristic-driven rather than a stronger coordination primitive.

Implementation Options

Option 1: Merge the PR largely as-is, with targeted review focus

Keep the bundled parity approach. Review it as one parity/safety package, verify the unit tests, and require the listed smoke tests before merge or immediately after behind maintainer validation.

This is the fastest path and preserves the author's intended "single PR" packaging, but it keeps review complexity high.

Option 2: Split into two merge units

Separate the work into:

  • Slack parity and safety changes: mention resolution, multibot gating, bot turn limits, slash commands, docs/config
  • Discord bug fix: thread creation race handling

This keeps the logical theme mostly intact while isolating the unrelated Discord fix. It reduces review noise and makes rollback safer if Slack behavior needs iteration.

Option 3: Split into three narrower tracks with shared-core extraction

Separate into:

  • Shared bot_turns extraction and Slack bot turn enforcement
  • Slack interaction parity: mentions, multibot gating, slash commands, docs/config
  • Discord thread-race fix

This is the most maintainable review shape and best for long-term change history. It makes each behavior set easier to test and reason about, but costs more maintainer overhead now.

Comparison Table

Option Speed to ship Complexity Reliability Maintainability User impact Fit for OpenAB right now
Option 1: Merge as bundled parity PR High Medium-High in review Medium Medium-Low High Medium
Option 2: Split Slack parity from Discord fix Medium-High Medium High High High High
Option 3: Split into three narrower tracks Medium-Low High process overhead High Very High High Medium-High

Recommendation

Recommend Option 2: split the Slack parity/safety work from the Discord thread-race fix, then advance both.

That is the right step for merge discussion because it preserves nearly all delivery momentum while removing the clearest review objection: one PR currently mixes a broad Slack parity bundle with a separate Discord production bug fix. The Slack bundle is coherent enough to stand on its own, and the Discord fix is small, concrete, and independently valuable. If further splitting is needed after review, the next split should be to isolate slash commands from the Slack safety/core parity path, but that should be a second-order decision rather than the default starting point.

dogzzdogzz and others added 3 commits April 20, 2026 17:53
…mbers

When a user (or bot) posts a message tagging another member besides this
bot, switch that thread to send-once mode for all subsequent agent replies
regardless of the adapter's `use_streaming()` hint.

Rationale: Slack's default behaviour on `allow_bot_messages=off` is to
post a "…" placeholder and stream via `chat.update`. In a thread where
multiple members are addressed (e.g. `@bot-a say hi to @bob`), the agent's
reply is likely to contain mention strings, and repeated edits in that
context are noisy and non-deterministic for sibling bots watching the
thread. Discord already behaves send-once globally — this change makes
multi-tag threads behave consistently across both adapters without
removing streaming from single-bot / single-target threads.

The detection lives in the router: both adapters already preserve
non-bot `<@uid>` in the resolved prompt (Discord's `resolve_mentions`
and Slack's `resolve_slack_mentions`), so any remaining match on
`<@!?[A-Za-z0-9]+>` means another member was tagged. The mark is
per-thread and persists for the session lifetime.

Tests: 6 new cases for the detection regex (Discord `<@123>`, Discord
legacy `<@!123>`, Slack `<@u123>`, plain text, `@(role)` / `@(user)`
placeholders).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…other members"

This reverts commit b0a8c4425ec0580beb7edb7c386a809b44ccb7f3.
Drop /models, /agents, /cancel from the Slack adapter. The feature as
implemented does not work end-to-end:

- Slack's UI blocks third-party slash commands invoked from a thread's
  reply composer with "/<cmd> is not supported in threads. Sorry!" —
  this is a platform-level restriction, not a configuration issue.
- Invoked from a channel's main composer, the slash command payload
  has no thread_ts, so the handler cannot route to the per-thread ACP
  session (sessions are keyed by thread_ts / trigger-message ts, never
  by channel_id).
- For /models and /agents specifically, most ACP agents (notably
  claude-code) do not emit configOptions, so even when routing
  succeeded the menu would be empty.

The net effect was a user-facing "No model options available" error or
Slack's own thread-rejection message, with no path to success. Remove
the handlers, the select-block builder, the ephemeral response helpers,
the related tests, and the docs section advising users to register the
commands. The WS loop now ack-and-drops slash_commands / interactive
envelopes with a debug log.

Discord's slash commands are unchanged — Discord's thread model is a
separate channel, so routing works there.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@dogzzdogzz dogzzdogzz changed the title feat(slack): port Discord parity — mentions, multibot, bot turn limits, slash commands feat(slack): port Discord parity — mentions, multibot, bot turn limits Apr 20, 2026
…lack

Add a dedicated section documenting the three independent reasons the
/models, /agents, /cancel commands work on Discord but not on Slack:

1. Slack blocks third-party slash commands invoked from a thread's
   reply composer at the client level — no app setting overrides this.
2. Channel-level slash command payloads lack thread_ts, so there's no
   way to route the command to the right per-thread ACP session.
3. Most ACP agents (claude-code, codex, gemini, etc.) don't expose
   configOptions, so /models would show an empty menu even if routing
   worked. Only kiro-cli emits the expected format.

Also suggests practical workarounds for Slack users: restart the pod
to change agent, set env vars for claude-code model selection.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@chaodu-agent
Copy link
Copy Markdown
Collaborator

🔴 MultibotMentions gating bug — multi-bot threads silently drop all messages

The MultibotMentions arm in slack.rs does if other_bot { continue; } without checking mentions_bot. This means in a multi-bot thread, even if the user explicitly @mentions the bot, the message is dropped.

Expected flow:

┌─────────────────────────────┐
│  Thread message arrives     │
│  (allow_user_messages =     │
│   MultibotMentions)         │
└─────────────┬───────────────┘
              │
              ▼
       ┌──────────────┐
       │  has_thread?  │──── No ───▶ continue (skip)
       └──────┬───────┘
              │ Yes
              ▼
       ┌──────────────┐
       │   involved?   │──── No ───▶ continue (skip)
       └──────┬───────┘
              │ Yes
              ▼
       ┌───────────────┐
       │  other_bot?   │──── No ───▶ ✅ process (single-bot thread, no mention needed)
       └──────┬────────┘
              │ Yes (multi-bot thread)
              ▼
     ┌─────────────────┐
     │  mentions_bot?  │──── Yes ──▶ ✅ process (user @mentioned this bot, allow)
     └────────┬────────┘
              │ No
              ▼
        ❌ continue (skip)
        "multi-bot thread without @mention → ignore"

Current PR code (bug):

       ┌───────────────┐
       │  other_bot?   │──── No ───▶ ✅ process
       └──────┬────────┘
              │ Yes
              ▼
        ❌ continue (skip)    ◀── 🐛 missing mentions_bot check
                                    drops ALL messages in multi-bot threads

Suggested fix — one-line change:

- if other_bot {
+ if other_bot && !mentions_bot {
      continue;
  }

Without this, MultibotMentions on Slack = bot goes completely silent in any thread where another bot has posted, even when the user explicitly @mentions it. Discord's implementation correctly gates on !mentions_bot, so this would be a parity gap.

@chaodu-agent
Copy link
Copy Markdown
Collaborator

🟡 NIT: Bot turn result handling is duplicated across adapters

BotTurnTracker is now shared in src/bot_turns.rs (nice!), but the ~50-line match block that acts on TurnResult is copy-pasted in both discord.rs and slack.rs. The warning messages, log levels, and warn-once semantics are nearly identical — only the ChannelRef construction differs.

Discord side (simplified):

match tracker.on_bot_message(&thread_id) {
    TurnResult::HardLimit => {
        warn!(channel_id, "hard bot turn limit reached");
        adapter.send_message(&channel_ref,
            &format!("🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). ...")
        ).await;
        return;
    }
    TurnResult::SoftLimit(n) => {
        info!(channel_id, turns = n, "soft bot turn limit reached");
        adapter.send_message(&channel_ref,
            &format!("⚠️ Bot turn limit reached ({n}/{max}). ...")
        ).await;
        return;
    }
    TurnResult::Throttled | TurnResult::Stopped => return,
    TurnResult::Ok => {}
}

Slack side (simplified) — almost identical:

match tracker.on_bot_message(&turn_key) {
    TurnResult::HardLimit => {
        warn!(channel_id, "hard bot turn limit reached");
        adapter.send_message(&warn_channel,
            &format!("🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). ...")
        ).await;
        continue;
    }
    TurnResult::SoftLimit(n) => {
        info!(channel_id, turns = n, "soft bot turn limit reached");
        adapter.send_message(&warn_channel,
            &format!("⚠️ Bot turn limit reached ({n}/{max}). ...")
        ).await;
        continue;
    }
    TurnResult::Throttled | TurnResult::Stopped => continue,
    TurnResult::Ok => {}
}

The only real difference is how ChannelRef / warn_channel is built. It might be worth considering extracting the message formatting into a shared helper in bot_turns.rs, e.g.:

// in src/bot_turns.rs
pub enum TurnAction {
    Continue,
    WarnAndStop(String),  // warning message to send
    SilentStop,
}

impl BotTurnTracker {
    pub fn check_and_format(&mut self, thread_id: &str, soft_limit: u32) -> TurnAction {
        match self.on_bot_message(thread_id) {
            TurnResult::Ok => TurnAction::Continue,
            TurnResult::SoftLimit(n) => TurnAction::WarnAndStop(
                format!("⚠️ Bot turn limit reached ({n}/{soft_limit}). \
                         A human must reply in this thread to continue.")
            ),
            TurnResult::HardLimit => TurnAction::WarnAndStop(
                format!("🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). \
                         A human must reply to continue.")
            ),
            TurnResult::Throttled | TurnResult::Stopped => TurnAction::SilentStop,
        }
    }
}

Then both adapters shrink to:

match tracker.check_and_format(&key, max_bot_turns) {
    TurnAction::WarnAndStop(msg) => { adapter.send_message(&ch, &msg).await; continue; }
    TurnAction::SilentStop => continue,
    TurnAction::Continue => {}
}

Not a blocker for this PR — the current code works. But as more adapters land (Teams? Telegram?), maintaining identical warning text and log semantics in N places will get painful.

dogzzdogzz added a commit to dogzzdogzz/openab that referenced this pull request Apr 22, 2026
- Fix slack.rs MultibotMentions gating: require !mentions_bot so the
  logic matches Discord parity and survives changes to the earlier
  mention-dedup path (comment 4290258138).
- Extract shared TurnAction/TurnSeverity helper in bot_turns.rs so
  Discord and Slack stop duplicating the ~30-line TurnResult match
  (comment 4290267743). Added byte-for-byte and per-thread independence
  tests for classify_bot_message.
dogzzdogzz added a commit to dogzzdogzz/openab that referenced this pull request Apr 22, 2026
- Fix slack.rs MultibotMentions gating: require !mentions_bot so the
  logic matches Discord parity and survives changes to the earlier
  mention-dedup path (comment 4290258138).
- Extract shared TurnAction/TurnSeverity helper in bot_turns.rs so
  Discord and Slack stop duplicating the ~30-line TurnResult match
  (comment 4290267743). Added byte-for-byte and per-thread independence
  tests for classify_bot_message.
dogzzdogzz added a commit to dogzzdogzz/openab that referenced this pull request Apr 22, 2026
- Fix slack.rs MultibotMentions gating: require !mentions_bot so the
  logic matches Discord parity and survives changes to the earlier
  mention-dedup path (comment 4290258138).
- Extract shared TurnAction/TurnSeverity helper in bot_turns.rs so
  Discord and Slack stop duplicating the ~30-line TurnResult match
  (comment 4290267743). Added byte-for-byte and per-thread independence
  tests for classify_bot_message.
@dogzzdogzz dogzzdogzz force-pushed the feat/slack-parity-bundle branch from 0c80b4e to c245915 Compare April 22, 2026 02:26
dogzzdogzz added a commit to dogzzdogzz/openab that referenced this pull request Apr 22, 2026
… gating

Addresses PR openabdev#487 review comments:

1. (bug) src/slack.rs MultibotMentions arm did `if other_bot { continue; }`
   without checking `mentions_bot` — logic didn't match the 'requiring
   @mention' debug message and could diverge from Discord parity.

2. (nit) The ~50-line `match tracker.on_bot_message(..) { TurnResult::.. }`
   block was duplicated in src/discord.rs and src/slack.rs. Extracted into
   `BotTurnTracker::classify_bot_message` returning a new `TurnAction` enum
   so adapters collapse to a 4-arm match. Severity hint keeps info! (soft)
   vs warn! (hard) log levels at the call site. Added 4 unit tests for the
   new helper.
@dogzzdogzz dogzzdogzz force-pushed the feat/slack-parity-bundle branch from b8a7aaf to ba064a1 Compare April 22, 2026 02:37
dogzzdogzz added a commit to dogzzdogzz/openab that referenced this pull request Apr 22, 2026
… gating

Addresses PR openabdev#487 review comments:

1. (bug) src/slack.rs MultibotMentions arm did `if other_bot { continue; }`
   without checking `mentions_bot` — logic didn't match the 'requiring
   @mention' debug message and diverged from Discord parity. Fix adds the
   explicit `!mentions_bot` guard. In practice the earlier mention-dedup
   already skips @mention message events (app_mention handles them
   separately), so this is defense-in-depth documented in the comment.

2. (nit) The ~50-line `match tracker.on_bot_message(..) { TurnResult::.. }`
   block was duplicated in src/discord.rs and src/slack.rs. Extracted into
   `BotTurnTracker::classify_bot_message` returning a new `TurnAction` enum
   { Continue | WarnAndStop{severity, turns, user_message} | SilentStop }.
   Adapters collapse to a 4-arm match. TurnSeverity preserves info! (soft)
   vs warn! (hard) log levels; turns preserves the structured log field.
   Added 6 unit tests for the new helper, including byte-for-byte message
   equality, per-thread independence, and human-reset resume.
@dogzzdogzz dogzzdogzz force-pushed the feat/slack-parity-bundle branch from f25cb23 to 3f03fa8 Compare April 22, 2026 03:04
… gating

Addresses PR openabdev#487 review comments:

1. (bug) src/slack.rs MultibotMentions arm did 'if other_bot { continue; }'
   without checking mentions_bot — logic didn't match the 'requiring
   @mention' debug message and diverged from Discord parity. Fix adds the
   explicit !mentions_bot guard. Defense-in-depth + parity.

2. (nit) The ~50-line match tracker.on_bot_message(..) { TurnResult::.. }
   block was duplicated in src/discord.rs and src/slack.rs. Extracted into
   BotTurnTracker::classify_bot_message returning a new TurnAction enum
   { Continue | WarnAndStop{severity, turns, user_message} | SilentStop }.
   Adapters collapse to a 4-arm match. TurnSeverity preserves info! (soft)
   vs warn! (hard) log levels; turns preserves the structured log field.
   Added 6 unit tests.
… with openabdev#497)

PR openabdev#498 fixed the bot-turn-reset bug in Discord by gating on_human_message
behind MessageType::Regular | InlineReply + non-empty content. Slack had
the same class of bug but only a partial defense: the skip_subtype blacklist
dropped channel_join/leave/topic/purpose, message_changed/deleted, but many
system-like subtypes still reached on_human_message and reset the counter:

  - pinned_item / unpinned_item
  - channel_name / channel_archive / channel_unarchive
  - group_join / group_leave / group_topic / group_purpose  (private channels)
  - reminder_add
  - tombstone

Extract is_plain_user_message(subtype, text) — a whitelist of
"" | me_message | thread_broadcast | file_share with non-empty content —
and gate the reset on it. Unit tests cover empty-text, whitelist, and the
full system-subtype blacklist.
…bdev#291)

Discord reads text file attachments inline into the prompt since PR openabdev#291
(feat/text-file-attachments). Slack only handled audio (STT) and images.
This is user-visible parity — the same message with a .log or .csv
attachment behaves differently depending on the adapter.

media::is_text_file() and media::download_and_read_text_file() are
already generalized in src/media.rs; Slack just needs to call them with
Slack-specific field names (mimetype, name, size, url_private_download)
and the bot_token auth header. The 5-files / 1 MB caps match Discord
byte-for-byte so operator mental model stays consistent.
…loop

Address codex review findings on the text-file attachment port:

1. (bug) The 1 MB TEXT_TOTAL_CAP could be bypassed when Slack reports
   size == 0 for a file (happens with externally-backed files).
   media::download_and_read_text_file only per-file-caps at 512 KB,
   so up to 5 × 512 KB = 2.5 MB could slip past the 1 MB total. Add an
   authoritative post-download check on actual_bytes, gate the
   pre-check on size > 0, and drop files that would overrun the cap.

2. (hygiene) Extract two pure helpers so the file loop is testable:
   - slack_file_download_url — url_private_download → url_private fallback
   - strip_mime_params — drops ; charset=… so media type-checkers
     see the bare form (Slack sometimes sends text/plain; charset=utf-8)
   Use them in the message handler and cover 7 cases in tests
   (URL preference, fallback, external-only; MIME bare, charset-param,
   empty, whitespace).
Copy link
Copy Markdown
Contributor

@masami-agent masami-agent left a comment

Choose a reason for hiding this comment

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

PR Review: #487

Summary

  • Problem: Slack adapter lacked several safety and feature-parity items that Discord already had: correct mention resolution, multibot-mentions gating, shared bot turn limits, text-file attachments, and a Discord thread-race fix.
  • Approach: Port each feature from Discord to Slack, extract shared logic (BotTurnTrackerbot_turns.rs), and fix a Discord race condition where two bots create threads from the same trigger message.
  • Risk level: Medium — touches core message routing in both adapters, but well-tested (81+ tests) and CI passes.

Core Assessment

  1. Problem clearly stated: ✅ — PR description is thorough with a table of changes, rationale, and test plan.
  2. Approach appropriate: ✅ — Extracting shared bot_turns.rs is the right call. Porting feature-by-feature with matching semantics is clean.
  3. Alternatives considered: ✅ — Slash commands were attempted and deliberately reverted with clear reasoning (Slack platform limitations).
  4. Best approach for now: ✅ — Minimal, focused changes with good parity reasoning.

Findings

src/bot_turns.rs (new shared module)

Clean extraction. The classify_bot_messageTurnAction helper is a good abstraction that eliminates the duplicated match block in both adapters. Tests are comprehensive (20 tests covering soft/hard limits, reset, warn-once, per-thread independence, classify API).

One observation: on_human_message resets both soft and hard counters to 0. The doc comment on HARD_BOT_TURN_LIMIT says "the hard counter also tracks the lifetime total and caps it regardless" — but the implementation resets hard on human message too. This is consistent with the existing Discord behavior (same code was moved), so it's not a regression, but the doc comment is slightly misleading. The hard limit is really "max consecutive bot turns without human intervention", not a lifetime cap.

src/discord.rs — thread race fix

The is_thread_already_exists_error heuristic (string matching on 160004 or "already been created") is pragmatic. The fallback path (refetch message → read .thread → join) is correct. Unit tests cover the detection. One edge case worth noting: if the refetch succeeds but message.thread is None (e.g., the sibling bot's thread was deleted between the error and the refetch), the error message is clear and propagates correctly.

src/slack.rs — multibot-mentions

The MultibotMentions arm now correctly checks !mentions_bot before continuing — this was the bug fix from the follow-up commit. The eager note_other_bot_in_thread call before self-check mirrors Discord #481 ordering. The bot_participated_in_thread return type change to (bool, bool) is clean.

src/slack.rs — bot turn tracking

The turn key construction uses thread_ts when in a thread, else channel:ts. For non-thread messages, channel:ts is unique per message, so the tracker would never accumulate — but this is correct because non-thread bot messages can't form loops. The tracker lock scope is tight (released before the handle_message call).

src/slack.rsis_plain_user_message whitelist

Good defensive approach. The whitelist ("" | me_message | thread_broadcast | file_share) is safer than the previous blacklist approach. Tests cover the full system-subtype list. Parity with Discord's MessageType::Regular | InlineReply gate.

src/slack.rs — text-file attachments

Clean port of Discord PR #291. The strip_mime_params helper handles Slack's text/plain; charset=utf-8 edge case. The post-download cap check (text_file_bytes + actual_bytes > TEXT_TOTAL_CAP) correctly handles the size == 0 bypass for externally-backed files. The slack_file_download_url helper with url_private_downloadurl_private fallback is well-tested.

src/slack.rsresolve_slack_mentions

Core correctness fix: the old strip_slack_mention wiped all <@UID> patterns, preventing the LLM from @-mentioning users back. The new function strips only the bot's own mention. Simple and correct.

src/slack.rshandle_message signature change

The strip_mentions: bool parameter is removed and replaced with always calling resolve_slack_mentions. This is correct — both app_mention and message events should resolve mentions the same way (strip bot, preserve others).

Review Summary

🔴 Blockers

(none)

💬 Questions

  1. The Cargo.lock diff shows openab version changing from 0.7.80.8.1. This appears to come from the merge commits pulling in main's Cargo.toml version bump. Can you confirm this is just a merge artifact and not an intentional version bump in this PR? (The PR itself doesn't modify Cargo.toml version.)

  2. The PR description mentions "Single PR per author request" — was there a reason not to split the Discord thread-race fix (row 7) into its own PR? It's an independent bug fix that could be merged separately. Not blocking, just curious about the rationale.

  3. No Closes #XX in the PR description — are there tracking issues for any of these features (multibot-mentions on Slack, bot turn limits on Slack, the Discord thread-race bug)?

🔧 Suggested Changes

  1. Doc comment on HARD_BOT_TURN_LIMIT — The comment says "the hard counter also tracks the lifetime total and caps it regardless" but on_human_message resets both counters. Consider updating the comment to say something like "Absolute per-thread cap on consecutive bot turns without human intervention" to match the actual behavior.

  2. MAX_CONSECUTIVE_BOT_TURNS is now dead codesrc/discord.rs line 23 still has const MAX_CONSECUTIVE_BOT_TURNS: u8 = 10; which was the old Slack-only cap. It's no longer referenced anywhere in the diff. Should be removed.

ℹ️ Info

  • CI is all green (check + 7 smoke tests pass).
  • The commit history has a noisy add/revert pair (force send-once + its revert) that cancels out in the net diff — confirmed, no residual changes.
  • The slash command removal is well-documented with three independent technical reasons. The docs/slack-bot-howto.md section explaining why is a nice touch for users.

⚪ Nits

(none — code is clean and well-commented)

Verdict

COMMENT — no blockers, but would like answers to the questions above (especially Q1 on the Cargo.lock version and Q3 on issue tracking) before approving.

Copy link
Copy Markdown
Collaborator

@obrutjack obrutjack left a comment

Choose a reason for hiding this comment

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

PR Review: #487

Summary

  • Problem: Slack adapter lacked Discord's safety features — bot turn limits, multibot mention gating, correct mention resolution, text-file attachments
  • Approach: Port Discord safety features to Slack, extract shared BotTurnTracker into bot_turns.rs, fix Discord thread-creation race
  • Risk level: Low

Core Assessment

  1. Problem clearly stated: ✅
  2. Approach appropriate: ✅
  3. Alternatives considered: ✅ (slash commands tried and reverted with clear reasoning)
  4. Best approach for now: ✅

Review Summary

🔴 Blockers

None.

ℹ️ Info

  • BotTurnTracker extracted identically from discord.rs → bot_turns.rs, no logic change
  • resolve_slack_mentions correctly strips only bot's own <@UID> — fixes the old strip_slack_mention that wiped all mentions
  • MultibotMentions now has explicit !mentions_bot guard — correct Discord parity
  • is_plain_user_message whitelist prevents system subtypes from resetting counter (#497 parity)
  • Text file caps (5 files / 1MB) match Discord; post-download enforcement handles Slack's size==0 edge case
  • Thread race fix uses string heuristic for error 160004, covered by unit tests
  • max_bot_turns has serde(default) → backward compatible with existing config.toml
  • turn_key for non-thread Slack messages is unique per message (channel:ts) — effectively a no-op counter, correct since bot loops only happen in threads
  • MAX_CONSECUTIVE_BOT_TURNS (legacy history-based cap) still used in discord.rs for AllowBots::All — separate from BotTurnTracker, not touched by PR
  • CI all green (10/10 checks), 81 tests pass, no secrets/PII in diff

Verdict

APPROVE — clean PR with thorough test coverage, well-documented commit history, and faithful Discord→Slack parity.

…mments

- bot_turns.rs: fix HARD_BOT_TURN_LIMIT doc comment — not a lifetime total,
  resets on human message (consecutive without human intervention)
- slack.rs: add comment explaining non-thread turn key is intentional
  (unique per message, counter never accumulates — loops only in threads)
- discord.rs: add comment explaining string heuristic in
  is_thread_already_exists_error (serenity has no structured error code)

Co-authored-by: 超渡法師 <[email protected]>
Co-authored-by: 普渡法師 <[email protected]>
Co-authored-by: 擺渡法師 <[email protected]>
@thepagent
Copy link
Copy Markdown
Collaborator

Thank you @masami-agent ! You did great jobs!

@thepagent thepagent merged commit c314175 into openabdev:main Apr 22, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants