fix(slack): match both bare and labelled mention forms in mentions_bot

[howie]

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1504107600520806501

Summary

Fixes bot-to-bot message detection under allow_bot_messages="mentions" when the sender uses Slack's canonical labelled mention form <@UID|handle>.

Root cause: mentions_bot and parent_mentions_bot both used str::contains("<@UID>") which is not a substring of <@UID|handle>, silently dropping all bot messages that used the recommended labelled form.

Changes:

• text_mentions_uid(text, uid) -- new pure helper using match_indices + byte-boundary check to accept both wire forms
• mentions_bot / parent_mentions_bot -- both call sites updated to use text_mentions_uid
• resolve_slack_mentions -- rewritten to strip labelled form too (detection and stripping must be symmetric)
• get_bot_user_id -- added .inspect_err() so auth.test failures emit a warn! instead of silently degrading mention detection
• src/acp/pool.rs -- drive-by: clarified per-connection vs pool-level lock semantics in with_connection doc comment (doc-only, no logic change)

Test Coverage

All branches covered at 93% (13/14 paths):

Function	Branches	Covered
text_mentions_uid	5	5/5
resolve_slack_mentions	7	7/7
get_bot_user_id .inspect_err	2	1/2 (G2 network path not unit-testable)

Tests: 27 -> 29 (+2 new edge-case coverage tests added during ship review)

No separate integration test for the mentions_bot / parent_mentions_bot call sites: each is a 1:1 substitution of the old str::contains call with text_mentions_uid, and text_mentions_uid has exhaustive unit coverage (5/5 branches including both wire forms and false-positive guard). An integration test would duplicate that coverage without adding signal.

Pre-Landing Review

2-voice mob review (Claude + Codex gpt-5.5) + adversarial pass. Quality score: 9.5/10.

• Logic: LGTM -- byte-index arithmetic correct, loop terminates in all branches
• 1 informational: inspect_err warn fires per-call under sustained auth.test failure -- intentional signal

Test plan

• 356 Rust tests pass (0 failures)
• 29 slack::tests tests pass

Fixes #800

Generated with Claude Code

[howie]

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491969620754567270/1504107600520806501

[antigenius0910]

Confirmed the bug reproduces on 0.8.3-beta.9 and verified the fix shape is correct against the actual repro. Quick E2E — two bots, allow_bot_messages="mentions", side-by-side from the same author/channel:

text	mentions_bot (pre-fix)
<@UB> …	true
<@UB|handle_b> …	false ← dropped at src/slack.rs:722

Tracing through the new text_mentions_uid against my TC-2 input: prefix <@UB, byte after the UID is | → matches Some(b'|') → true. Fix would resolve the bug.

Things I liked

• Helper extraction (text_mentions_uid) means the bug now has a name and a focused test suite — much easier to extend than a sprawling substring check.
• Byte-boundary match via match_indices + as_bytes().get(i + prefix.len()) is the right primitive: cheap, no regex, no UTF-8 hazard (Slack UIDs are ASCII so the byte index is the char index here).
• Test set is genuinely thorough: the mentions_uid_no_false_positive_on_uid_prefix case (U123 vs <@U123BOT>) is the easy-to-miss one and you covered it.
• Updating both mentions_bot (L582) and parent_mentions_bot (L282) call sites in the same PR — easy to forget the second one.
• inspect_err on get_bot_user_id is a nice observability touch for the "why is mention detection suddenly silent?" failure mode.

Things worth addressing

1. src/acp/pool.rs change looks unrelated

The diff is doc-comment-only on with_connection, clarifying the per-connection vs pool-level lock semantics. Strictly correct and benign, but it's not connected to #800. Two clean options: (a) split into its own PR, or (b) call it out in the description ("drive-by: clarify pool locking comment while in the file"). Either avoids reviewer head-scratching.

2. Cargo.lock adds serde to chrono's deps

Looks like fallout from a regenerate, not an intentional change. If a chrono feature was enabled in Cargo.toml, it should be visible there; if not, this is probably stale and should be reverted to keep the diff scoped. Could you double-check?

3. Helper-level tests only; no integration assertion

Totally fair given the change is a 1:1 substitution of two call sites by the new helper, and the helper has exhaustive unit coverage. Worth one line in the PR description making that argument explicit ("two call sites swapped; helper covers them exhaustively"), so a reviewer doesn't go looking for the missing end-to-end test.

4. resolve_slack_mentions whitespace after mid-string strip

resolve_mentions_strips_labelled_mid_sentence asserts "please ask to run" (two spaces). That's the function's actual behavior, so the test is consistent, but a // note: surrounding whitespace preserved to avoid mangling adjacent content; LLM input tolerates double spaces next to the test (or in the fn doc) would save a future reader from wondering whether it's intentional. Pure nit.

5. Paired sibling concern (out of scope for this PR)

While running the repro I also tripped bot not in trusted_bot_ids, ignoring … because resolve_bot_user_id(B…) returned None (our bot token apparently lacks users:read for bots.info). That's tracked separately as #811 with a fix in #814 — flagging here only because operators using mentions + trusted_bot_ids together will need both #808 and #814 for a clean relay; either alone leaves the other gate broken. Not asking you to change scope, just noting for whoever sequences the merges.

Nice fix. The core change is exactly what the bug needs and the tests are well thought through.

[howie]

Thanks for the thorough E2E trace and the clear breakdown.

#1 pool.rs drive-by — added a note to the PR description: "drive-by: clarified per-connection vs pool-level lock semantics in with_connection doc comment (doc-only, no logic change)".

#2 Cargo.lock serde — intentional. Cargo.toml:30 has chrono = { version = "0.4.44", features = ["serde"] } explicitly, so cargo added serde to chrono's resolved deps when the lock was regenerated. No stale artifact here.

#3 Integration test rationale — added to the PR description: each call site is a 1:1 substitution of str::contains with text_mentions_uid, and text_mentions_uid has exhaustive unit coverage (5/5 branches). A separate integration test would duplicate coverage without adding signal.

#4 Double-space nit — added a three-line comment above the assert_eq! in resolve_mentions_strips_labelled_mid_sentence explaining the behavior is intentional.

#5 trusted_bot_ids + resolve_bot_user_id pairing — noted; will coordinate merge order once #814 lands.

[shaun-agent]

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 🙏

• Title: fix(slack): match both bare and labelled mention forms in mentions_bot
• Source: fix(slack): match both bare and labelled mention forms in mentions_bot #808
• Status: moved to PR-Screening
• Generated at: 2026-05-14T14:24:47.223Z
• Discord thread: not available

Screening report

## Intent

PR #808 fixes Slack bot-message filtering when allow_bot_messages="mentions" is enabled.

The operator-visible problem: OpenAB may silently ignore Slack bot messages that mention the bot using Slack’s canonical labelled mention format, <@UID|handle>, because the current detection only matches the bare form <@UID>.

Feat

This is a Slack bug fix.

Behavior change: mention detection now accepts both Slack mention wire forms:

• Bare: <@UID>
• Labelled: <@UID|handle>

It also makes mention stripping symmetric by updating resolve_slack_mentions, and adds warning logs when auth.test fails while resolving the bot user ID.

There is one unrelated doc-only cleanup in src/acp/pool.rs.

Who It Serves

Primary beneficiary: Slack users and OpenAB deployers using bot-to-bot workflows.

Secondary beneficiaries:

• agent runtime operators, because dropped Slack bot messages are hard to diagnose
• maintainers, because mention parsing becomes centralized and tested
• reviewers, because the bug is narrow and the fix is mostly pure helper logic

Rewritten Prompt

Fix Slack mention detection for allow_bot_messages="mentions" so OpenAB recognizes both <@BOT_UID> and <@BOT_UID|handle> as valid mentions of the bot.

Implement a small pure helper for Slack UID mention matching, use it in both direct message and parent-message mention checks, and update mention resolution/stripping so detection and cleanup handle the same forms. Add focused unit tests for bare mentions, labelled mentions, malformed labels, and false positives where the UID appears inside another token. Log a warning when bot user ID lookup fails, without changing fallback behavior.

Keep unrelated changes out unless they are documentation-only and clearly separated.

Merge Pitch

This is worth advancing because it fixes a real Slack interoperability gap: Slack commonly emits labelled mentions, and the current substring check misses them entirely.

Risk profile is low to moderate. The core change is small and testable, but mention parsing is easy to get subtly wrong around byte boundaries and malformed Slack markup. Reviewer attention should focus on whether the helper rejects false positives, whether stripping remains symmetric with detection, and whether the warning log could become noisy under repeated auth.test failure.

The pool doc comment change is harmless but should be reviewed as unrelated scope.

Best-Practice Comparison

OpenClaw principles mostly do not apply here. This PR is not about scheduling, persistence, isolated executions, delivery routing, retries, or run logs.

Relevant OpenClaw-adjacent principle:

• explicit delivery routing: yes, loosely. Correctly recognizing whether a Slack message is addressed to the bot is part of routing inbound work to the agent.

Hermes Agent principles also mostly do not apply. This is not a daemon tick, locking, atomic persistence, session isolation, or scheduled prompt issue.

Relevant Hermes-adjacent principle:

• self-contained prompts for scheduled tasks: not directly relevant.
• fresh session per run: not relevant.
• file locking / atomic writes: not relevant.

The best-practice frame here is simpler: Slack protocol parsing should be centralized, symmetric between detection and normalization, and covered by focused edge-case tests.

Implementation Options

Option 1: Conservative fix only
Replace contains("<@UID>") with a helper that recognizes <@UID> and <@UID|handle>, then add unit tests for the helper. Leave resolve_slack_mentions unchanged unless tests prove it is part of the bug.

Option 2: Balanced parser cleanup
Add text_mentions_uid, update both mentions_bot and parent_mentions_bot, update resolve_slack_mentions to strip both forms, and test detection plus stripping together. Keep the warning log for failed auth.test. Drop or split the unrelated pool doc comment if maintainers want strict scope.

Option 3: Ambitious Slack mention parser
Introduce a small Slack token parser that recognizes user mentions, channel refs, special mentions, and labelled forms through one reusable API. Refactor detection and replacement to consume parsed tokens rather than doing targeted string scans.

Comparison Table

Option	Speed to ship	Complexity	Reliability	Maintainability	User impact	Fit for OpenAB right now
Conservative fix only	High	Low	Medium	Medium	Medium	Good
Balanced parser cleanup	Medium-high	Medium	High	High	High	Best
Ambitious Slack mention parser	Low	High	High if done well	High long-term	High eventually	Too large for this PR

Recommendation

Advance the balanced parser cleanup.

It fixes the actual bug, keeps detection and stripping symmetric, and adds enough targeted coverage to make the behavior reviewable without turning this into a broader Slack parsing refactor. For merge discussion, I would ask reviewers to verify the byte-boundary logic and decide whether the src/acp/pool.rs doc comment should stay or be split out.

Do not expand this PR into a full Slack parser. That can be a follow-up if more Slack token edge cases appear.

[antigenius0910]

Verified canonical mention parsing (<@U…|handle> form) end-to-end on a 0.8.3-beta.9-equivalent rig, with #814 applied on top. Bot-to-bot relay reached claude-agent-acp via the labelled mention path; bare mentions remain unaffected. Pairs with #814 — neither PR alone closes the relay path; both are required.

[chaodu-agent]

LGTM ✅ — Correct fix for labelled mention detection with thorough test coverage.

What This PR Does

Fixes bot-to-bot message detection under allow_bot_messages="mentions" when the sender uses Slack's labelled mention form <@UID|handle>. The old str::contains("<@UID>") silently missed these.

How It Works

• New text_mentions_uid(text, uid) helper uses match_indices + byte-boundary check to accept both <@UID> and <@UID|handle> wire forms without false positives on UID prefixes.
• resolve_slack_mentions rewritten to strip both forms symmetrically (loop-based parser handles bare, labelled, and malformed cases).
• .inspect_err() added to get_bot_user_id so auth.test failures emit a warning instead of silently degrading.

Findings

#	Severity	Finding	Location
1	🟢	Byte-boundary check correctly prevents false positives on UID prefixes	src/slack.rs:1209
2	🟢	Excellent test coverage — 13 new tests covering bare, labelled, mixed, malformed, and prefix-guard cases	src/slack.rs tests
3	🟢	resolve_slack_mentions handles unclosed labels gracefully (preserved verbatim)	src/slack.rs:1163
4	🟢	Drive-by pool.rs doc comment is harmless and accurate	src/acp/pool.rs:260

Baseline Check

• PR opened: 2026-05-13
• Main has: text.contains(&format!("<@{bot_id}>")) — only matches bare form
• Net-new value: Correctly detects both Slack mention wire forms, preventing silent message drops

What's Good (🟢)

• Logic is sound — match_indices approach is efficient and correct
• Detection and stripping are symmetric (both handle the same forms)
• Exhaustive unit tests with edge cases (empty label, malformed unclosed, prefix false-positive guard)
• .inspect_err adds observability without changing control flow

Note: This PR and #814 both modify src/slack.rs. Whichever merges second will need a trivial rebase.

[chaodu-agent]

LGTM ✅