fix(slack): prevent duplicate rich-text message content - #80240
fix(slack): prevent duplicate rich-text message content#80240nikitaBarkov wants to merge 1 commit into
Conversation
d83ec1c to
6fcfec9
Compare
ae151cd to
6c24001
Compare
Slack sends an authored message twice: flat in `event.text` and structurally in `event.blocks`. The blocks are rendered so quoted and forwarded content is not lost, and whatever the render carries beyond the flat text is appended to the message. That comparison had several ways to fail on the *same* sentence, each of which showed the author their own words a second time: 1. HTML entities — the flat copy escapes `&`/`<`/`>` while `blocks[].link.url` stays raw, so any link with query parameters (every "Copy link" on a thread) mismatched. 2. Permalink unfurls — the live inbound path skips `is_msg_unfurl` attachments, thread/parent hydration did not, so the linked message's body was appended again. 3. The Block Kit dump — it serialized the authored `rich_text` alongside the UI blocks it exists for, and its allowlist drops `url`, so the sentence reappeared with every link removed. 4. Unknown inline elements — the renderer knew eight types and silently dropped the rest. A pasted message permalink arrives as `message_mention`, so the link vanished from the render and the sides stopped comparing equal. 5. `message_mention` without a url — `url` is optional on that element while `channel_id` and `message_ts` are not, so the element rendered as nothing and the sentence came back with a blank in the link's place. 6. `date` elements — `fallback` and `url` are both optional, and the flat `<!date^…>` form was never read down to what the rich text renders. 7. Labelled mentions — Slack may attach a label (`<@U…|name>`, `<#C…|general>`, `<!subteam^S…|@marketing>`, `<!here|@here>`) in the flat text while the blocks carry the bare id. The bot's own mention is one of these, and stripping only its bare form left it in the flat copy. 8. Autolink schemes — only `https` and `mailto` were matched, so a `tel:` link kept its angle brackets and mismatched too. Unknown inline types are now read by their `url`/`text`/`fallback` so a type Slack adds later still renders, and `team`, `color` and a fallback-less `date` render into the flat form Slack sends. Every field is read as a string or not at all: Block Kit carries text as an object in many places, and a non-string one reaches the renderer's `str.join` and raises there, which costs the whole message. `channel_id` and `message_ts` are the permalink's own components, so a url-less `message_mention` renders the permalink's tail; the workspace host and the thread query cannot be rebuilt from the element, so a permalink on either side is reduced to that same tail. Canonicalization is used for matching only -- the authored text still reaches the agent verbatim, so a mistake here can cost an unrendered element, never an altered or missing message. An element carrying neither a url nor a label still renders as nothing, and a message containing one is still appended twice. Suppressing such a render was tried and is worse: an app message whose body lives only in the blocks disappears, and a forwarded quote is dropped. Genuinely additional content -- quotes, lists, code blocks, attachments, interactive bot blocks -- is unaffected throughout. Tests cover both merge sites (live inbound and thread hydration) and the negative cases.
6c24001 to
672ecb3
Compare
fix(slack): prevent duplicate rich-text message content A well-tested, well-documented normalization effort (HTML-entity unescape, link/date/permalink/mention canonicalization, fenced/inline code, inline styles, message-unfurl skip, block-dump scoping). Points:
|
|
Thanks for the read. I checked both actionable points against the code and against a measurement rather than reasoning about them, and neither holds up as a defect — details below so the conclusion is falsifiable. Points 3–5 I agree with as stated. 1. Substring dedupe — deliberate, and it cannot drop content the agent would otherwise seeTwo facts make this safe: The flat text is always delivered. Exact match would reintroduce the bug. The one thing that would genuinely be additional-yet-substring is a repetition Slack itself put in the flat text as well — so it is still not lost. 2. Nested inline styles — measured, and the premise is wrongThe loop does not peel one level per pass. Slack's message limit is 40k characters, and the 60k row above is already past what can be posted. A realistic message costs ~0.2 ms; the adversarial 40k cases cost under 3 ms, on a path that then makes network calls. There is no pathological input to cap against, and an iteration cap would silently leave styled text un-normalized — turning a non-problem into a real one (a message that fails to dedupe and gets duplicated). 3. Unfurl skip is flag-based — agreed, and left as is on purpose
4 & 5Agreed — the permalink path is workspace-agnostic by design (the same message is linked from different hosts and with/without |
What does this PR do?
Stops one authored Slack message from reaching the agent twice.
Slack sends the same message flat in
event.textand structurally inevent.blocks. The blocks are rendered so quoted and forwarded content is not lost, and whatever the render carries beyond the flat text is appended. That comparison had several ways to fail on the same sentence, each of which showed the author their own words a second time:&/</>whileblocks[].link.urlstays raw, so any link with query parameters — every "Copy link" on a thread — mismatched.is_msg_unfurlattachments; thread/parent hydration did not, so the linked message's body was appended again.rich_textalongside the UI blocks it exists for, and its allowlist dropsurl, so the sentence reappeared with every link removed. Scoped back to non-rich_textblocks, which is what fix: inline Slack block and attachment context #11426 described._render_inline_elements()knew eight types and silently dropped the rest. A pasted message permalink arrives asmessage_mention, so the link vanished from the render and the two sides stopped comparing equal.message_mentionwithout a url.urlis optional on that element whilechannel_idandmessage_tsare not, so it rendered as nothing and the sentence came back with a blank in the link's place.dateelements.fallbackandurlare both optional, and the flat<!date^…>form was never read down to what the rich text renders.<@U…|name>,<#C…|general>,<!subteam^S…|@marketing>,<!here|@here>) in the flat text while the blocks carry the bare id. The bot's own mention is one of these, and stripping only its bare form left it in the flat copy.httpsandmailtowere matched, so atel:link kept its angle brackets and mismatched too.label (url), and reducing the permalink to its tail consumed everything up to the next space — the closing parenthesis included — so the two sides mismatched again whenever only one of them carried?thread_ts=….Cause 4 is the one confirmed against a live gateway log: one
inbound messageline per message (a rendering bug, not repeated delivery), the transcript carrying the sentence twice with a blank where the URL had been, and the same messages arriving once after the fix. The rest are reproducible with the adapter's own helpers.Related Issue
Follows the same bug class as #66204, #26309 and #59903, which fixed individual mismatches without test coverage on the comparison path.
Type of Change
Changes Made
plugins/platforms/slack/adapter.py: inline-element rendering moved into_render_slack_inline_element().message_mentionis read likelink, and any unknown type by itsurl/text/fallback, so a type Slack adds later still renders;team,colorand a fallback-lessdaterender into the flat form Slack sends.plugins/platforms/slack/adapter.py:channel_idandmessage_tsare the permalink's own components, so a url-lessmessage_mentionrenders the permalink's tail; the workspace host and the thread query cannot be rebuilt from the element, so a permalink on either side is reduced to that same tail, stopping at the query so alabel (url)form keeps its closing parenthesis.plugins/platforms/slack/adapter.py:_normalize_slack_text_for_dedupe()unescapes&/</>before canonicalizing links, reads down the<!date^…>form and the optional mention label, strips the bot's mention after that label, and matches any autolink scheme.plugins/platforms/slack/adapter.py:_extract_text_from_slack_attachments()skipsis_msg_unfurl, matching the live path;_serialize_slack_blocks_for_agent()serializes only non-rich_textblocks.plugins/platforms/slack/adapter.py: every field is read as a string or not at all, through one helper. Block Kit carries text as an object in many places, so an element -- an unknown one above all -- may hold one where a string belongs, and it would raise in the renderer'sstr.joinand cost the whole message. Four such payloads raise onmainand none do here.tests/gateway/test_slack.py:TestSlackAuthoredTextDeduplication— 45 tests over both merge sites.Canonicalization is used for matching only: the authored text still reaches the agent verbatim, so a mistake there can cost an unrendered element, never an altered or missing message.
Known gap, deliberately left open: an element carrying neither a url nor a label still renders as nothing, and a message containing one is still appended twice. Suppressing such a render is worse — an app message whose body lives only in the blocks disappears, and a forwarded quote is dropped. An unrendered element is the lesser cost.
How to Test
.../archives/C…/p…) and send it to the bot — it arrives once, with the link intact.?thread_ts=…&cid=…), plain and with a label of your own — one copy each.@here, and one with atel:link — one copy each.scripts/run_tests.sh tests/gateway/test_slack.py— 222 passed.Checklist
Code
Documentation & Housekeeping
cli-config.yaml.example— N/A, no config changesCONTRIBUTING.md/AGENTS.md— N/A, no architecture or workflow changesNotes
Out of scope: Slack also re-delivers a message as
message_changedwhen it attaches an unfurl preview, while_processed_message_ts[ts]is written only at the end of_handle_slack_message, after every longawait. That is repeated delivery rather than a duplicated render, it did not occur in the logs behind this report, and a naive fix breaks "an @mention added by an edit still wakes the bot" (#64957). #73450 already rewrites that branch.