Skip to content

fix(slack): insert resolved display names literally when humanizing mentions - #78837

Closed
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/slack-mention-name-regex-escape
Closed

fix(slack): insert resolved display names literally when humanizing mentions#78837
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/slack-mention-name-regex-escape

Conversation

@Drexuxux

@Drexuxux Drexuxux commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What

SlackAdapter._humanize_user_mentions turns Slack's opaque <@U123> tokens into @DisplayName so the agent can tell participants apart — the fix from 5d747a9 (2026-07-22, fix(slack): humanize inbound user mentions + ground bot identity), salvaged from #55340 by @benbarclay. It resolves the name, then substitutes:

display = (name or uid).strip() or uid
text = re.sub(rf"<@{uid}(?:\|[^>]*)?>", f"@{display}", text)

The second argument to re.sub is a replacement template, not a literal — re parses backslash escapes in it. A Slack display name is arbitrary user-set text, so those backslashes are the user's characters, not regex syntax. Running the real substitution:

  name='dev\\ops'           -> error: bad escape \o at position 4
  name='a\\1b'              -> error: invalid group reference 1 at position 3
  name='back\\slash'        -> error: bad escape \s at position 5
  name='\\g<0>'             -> OK   'hey @<@U123> can you look at this'
  name='plain name'         -> OK   'hey @plain name can you look at this'

Two distinct failures:

  • The raise. The trigger-text call site (_handle_slack_message) is not inside any try — I checked the enclosing function's AST, no Try node covers it — and both Bolt handlers (@self._app.event("message") and @self._app.event("app_mention")) await self._handle_slack_message(...) bare. So re.error unwinds the whole inbound path: every message mentioning that person is dropped, with no reply, for as long as their name has a backslash in it. The reply_to_text call a few lines earlier is wrapped, which is what makes the unwrapped one easy to miss.
  • The silent one. \g<0> expands to the entire match, so the raw <@U123> gets written straight back. No error, and the agent is handed exactly the opaque token this method exists to remove — the "bot thinks it's @Someone-Else" bug the original fix was for, quietly restored for that user.

The pattern side is fine: uid comes from re.findall(r"<@([A-Z0-9]+)..."), so it can only be [A-Z0-9]+. Only the replacement is unsafe.

The fix

Pass the replacement as a function. re performs no template parsing on a callable's return value, so the name lands verbatim:

text = re.sub(
    rf"<@{uid}(?:\|[^>]*)?>",
    lambda _m, _name=f"@{display}": _name,
    text,
)

This is the shape the Matrix adapter already uses for its outbound mention rewrite (_OUTBOUND_MENTION_RE.sub(lambda match: ..., protected)), so it matches existing practice rather than introducing a new convention. The default argument binds the name per iteration instead of closing over the loop variable.

Names without backslashes produce byte-identical output, so the existing behaviour is untouched.

Scope: this is the only site in the Slack adapter that puts resolved user text into a replacement template — every other re.sub there uses a literal, a group reference, or a callable — so there is no sibling to sweep.

Tests

Added to tests/gateway/test_slack_mention_humanization.py, reusing its _adapter_with_names harness:

  • a name containing dev\ops no longer raises and renders as @dev\ops
  • a\1b is inserted literally instead of raising on the group reference
  • \g<0> renders literally and the assertion "<@" not in out pins that the raw ID is never re-injected
  • one odd name alongside a normal one leaves the normal mention correct — the failure previously took the whole message down, including other people's mentions

Results:

7 passed

The 3 pre-existing tests in the file are unchanged and still green.

Red without the source change (tests kept, plugins/platforms/slack/adapter.py reverted):

FAILED test_backslash_in_display_name_does_not_raise
E   re.error: bad escape \o at position 4
FAILED test_group_reference_in_display_name_is_literal
E   re.error: invalid group reference 1 at position 3
FAILED test_named_group_reference_does_not_reinject_the_raw_id
E   AssertionError: assert '<@' not in 'hi @<@U07ODD>'
FAILED test_one_odd_name_does_not_break_the_other_mentions
E   re.error: bad escape \o at position 4
4 failed, 3 passed

Regression run over 159 test files (everything under tests/ mentioning Slack), against the same files on main:

main:      43 failed, 3099 passed   (39 pre-existing, plus the 4 new tests failing as above)
with fix:  41 failed, 3101 passed

The set difference is two tests — one in tests/honcho_plugin/test_pin_peer_name.py and one in tests/test_mcp_serve.py::TestEventBridgePollE2E — and both are flaky rather than caused by this change: run in isolation they fail 2, 1, 2 times across three consecutive runs on unmodified main and 2, 2, 1 with the fix applied. Neither touches Slack.

tests/gateway/test_teams.py is excluded from both runs: it errors at collection on an unrelated missing Teams dependency and aborts the session before any test executes. scripts/check-windows-footguns.py passes on both changed files.

…entions

_humanize_user_mentions rewrites <@uid> to @DisplayName by passing the
resolved name as re.sub's replacement, where re parses it as a template.
A display name is arbitrary user-set text, so the escapes in it are the
user's characters, not regex syntax:

  dev\ops  -> re.error: bad escape \o
  a\1b     -> re.error: invalid group reference 1
  \g<0>    -> expands to the whole match, silently putting the opaque
              <@uid> back — the token this method exists to remove

The trigger-text call site sits in _handle_slack_message outside any try,
and both Bolt event handlers await it bare, so the raise takes the whole
inbound message down: every message mentioning that person is dropped.

Pass the replacement as a function instead — re does no template parsing
on the return value, so the name lands verbatim. Same shape the Matrix
adapter already uses for its outbound mention rewrite.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 4, 2026
@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Merged via #81738 with your commit and authorship preserved (rebase-merge, now commit 7c02bfc on main) — thanks for the clean fix and the regression tests, the lambda-replacement approach was exactly right.

Closing this original since the salvage PR carried it in. Verified live: dev\ops-style display names no longer take down inbound message processing.

@teknium1 teknium1 closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants