fix(slack): inbound mrkdwn normalization, channel.post thread ids, Socket Mode retries (#283)

[patrick-chinchill]

Summary

Part (b) of #209: Slack inbound mrkdwn normalization, channel.post() thread ids, and Socket Mode retries. It starts from wip/4.41-sl4-full (c0788056) minus what #286 (part a) and #291 (#210) already merged. The format modules and their tests applied cleanly. The adapter hunks were re-applied by hand on current main.

Upstream commits mapped

Upstream	chat@	Python
0b63791b fix(slack): process Socket Mode retry envelopes (#667), Slack half	4.33.0	_on_socket_request no longer acks and drops retry_attempt > 0. It logs "Processing socket mode retry" at info (retry_attempt, retry_reason, type) and routes the envelope like a first delivery. retry_num reaches the #268 event-id marker.
92530dd3 fix(slack): return replyable thread ID from channel posts (#720)	4.35.0	post_channel_message returns slack:{channel}:{ts} when raw["ts"] is a str, and otherwise the synthetic slack:{channel}:.
c3118279 fix(slack): preserve channel id, normalize hallucinated link format (#756)	4.37.0	<#C1|gen> becomes #gen (C1). The inverted link <label|https://…> is swapped back.
e71bfead fix(slack): preserve first line of incoming code blocks (#843), Slack half	4.39.0	_convert_mrkdwn_with_code_fences and its helpers (_find_inline_code_end, _is_on_blockquote_line, _escape_leading_block_marker).
44423bdc fix(slack): convert special mentions to readable text (#960)	4.41.1	_convert_special_mentions and _AngleTokenScanner (upstream findAngleTokenEnd).

Also:

• SlackFormatConverter.to_ast is now parse_markdown(slack_mrkdwn_to_markdown(text)), as upstream toAst. This fixes a missing unescape: <, > and & used to reach formatted and text verbatim.
• The Python-only regex extract_plain_text override is removed. Upstream markdown.ts has no such override.
• _assemble_content returns ast_to_plain_text(formatted) (upstream text: toPlainText(formatted)). This replaces the [4.41/SL5] Slack inbound: pasted tables and alert attachments in message content #210 interim and its four interim assertions.

Tests ported

• tests/test_slack_format_primitives.py: the 10 e71bfead format tests, the 7 from 44423bdc (including the it.each, as a parametrized test), the 3 from c3118279, the updated "normalizes Slack mrkdwn to Markdown" test, and a Python adversarial case with 50k characters (unclosed < runs, unbalanced backticks, quoted fences). The adversarial case asserts output, not timing.
• tests/test_slack_format.py: the 6 e71bfead markdown.test.ts cases, plus a Python test that to_ast unescapes entities.
• tests/test_slack_inbound_mentions.py: "converts special mentions to readable text", "preserves special mention tokens in an inbound inline code span", "preserves special mention tokens in an inbound code block", "posts to channel without thread context", and a Python case where the response has no string ts.
• tests/test_slack_socket_mode.py: "processes retries like first deliveries (dedupe drops true duplicates)" replaces test_retry_attempt_is_skipped_but_acked.
• tests/test_slack_inbound_content.py: three [4.41/SL5] Slack inbound: pasted tables and alert attachments in message content #210 interim assertions now expect upstream's values (#C789 @S789 …, Deploy failed:\n\nTypeError: boom… and #general (C789)). The two pre-[4.41/SL4b] Slack inbound mrkdwn normalization, channel.post thread ids, Socket Mode retries (remainder of #209) #283 guards are replaced by test_body_text_is_the_plain_text_of_formatted.

I also differential-fuzzed slack_mrkdwn_to_markdown against upstream format/index.ts 4.41.1 run under node: 40,000 random inputs built from mrkdwn atoms, with 0 mismatches.

Fidelity

Delta vs committed report (HEAD): missing 100 -> 100 (+0). The ported files are adapter tests, which are outside MAPPING and TARGET_MAPPING. fidelity_target.json changes only in its extra-test count (Python-only parser tests in test_markdown_faithful.py; the convergence round re-ran it and the delta stayed missing 100 -> 100 (+0)). Strict at 4.31.0 passes ("All TS tests have Python equivalents"; 730 real tests plus 3 absorbers).

Divergences

There is one divergence: the parse_markdown blockquote depth cap described above. It has a row in the non-parity table, a breadcrumb at the code site, a regression test and a CHANGELOG entry. Two Python-specific details, both documented in docs/UPSTREAM_SYNC.md under "Slack inbound mrkdwn, channel-post ids, Socket Mode retries":

• The socket retry log uses the key retry_attempt, which is slack_sdk's name for upstream's retry_num.
• The scanners are linear: a failed <…> scan is memoized, the newline search is bounded, and the blockquote check matches in place. Their results are identical to upstream.

Shared parse_markdown fixes (prompted by astra). Each one closes a way the AST-derived message.text lost or garbled content:

1. Fence rule (round 1). A backtick fence's info string may not contain a backtick (CommonMark). Without it, a Slack quote that starts with an inline fence (> ```npm test````) parsed to an empty code block, and the command was dropped from message.text. With the fix it stays text. This is a one-line regex change in _FENCED_CODE_START_RE` and affects every adapter, but only on such lines.
2. Blockquote depth cap (rounds 3-5; the one documented divergence). > now unescapes, so ">" * 1100 raised RecursionError in the recursive blockquote parser. That dropped the message, or a whole fetch_messages page. Past 100 levels (markdown-it's default maxNesting) the rest of the quote stays literal text, which leaves stack room for lists nested inside quotes. The nesting depth is a separate variable from the heading level. The existing [4.41/C2b] Shared text utilities: bare-mention scanner, code-fence helpers, to_plain_text structural whitespace #193 test that extraction handles a 600-deep AST now builds that AST directly.
3. Link pattern (rounds 5-6). It now accepts one level of balanced brackets in the label and balanced parens in the URL, as CommonMark does: <https://example.com|build [failed]> reads build [failed], and …/Foo_(bar) keeps its ). Escaped \( and \) are consumed whole. A differential run against main's parser (60k random link-ish inputs) differs only on malformed inputs, where main built broken URLs such as \.

Also from round 3: the blockquote-line check in the fence normalizer now remembers the current line (_BlockquoteLines), so a long indented quote line full of fences stays linear.

Known shared-parser gaps (not Slack-specific, and not caused by this PR), now tracked in #308:

• parse_markdown has no multi-backtick code spans and keeps a paragraph's leading space. Two markdown.test.ts assertions are adapted because of this.
• parse_markdown is quadratic on some inputs, about 2.4 s for 20k characters of `x. to_ast already ran parse_markdown on every inbound message before this change.

Consumer impact (Slack, high)

• The shape of message.text changes. It is now the plain text of message.formatted. List markers, heading #, quote > and inline-code backticks are dropped. A trailing newline is trimmed. Blocks are joined by a blank line.
• Mentions and entities. <!here>, <!channel> and <!everyone> become @here, @channel and @everyone. <!subteam^S1|@eng> becomes @eng, and <!subteam^S1> becomes @S1. <#C123|general> becomes #general (C123). Tokens inside code stay literal. <, > and & are unescaped. This also applies to table cells and mrkdwn attachment parts.
• Code blocks keep their first line. Before, ```npm test``` gave an empty formatted. Now it gives a code node, and text is "npm test".
• channel.post() returns slack:C123:<ts> (replyable) instead of slack:C123:. File-only posts keep slack:C123:.
• Socket Mode retries are processed instead of dropped. The [4.41/SL8b] Slack: Enterprise Grid org-wide installs, authorizations[] routing, retry marker (split from #213) #268 event-id marker and core message-id dedupe drop true duplicates.
• Migration. Update on_message regexes that matched #channel-name, raw <!here>, list markers or backticks. Match on message.formatted or message.raw["text"] when you need the structure.

A live Slack-loop check is pending (DM and channel: a code block on its first line, <!here>, #channel, a channel.post() reply in its thread, and a socket reconnect retry).

Astra review

• Round 1, two P2 findings.
  • Non-events_api socket retries are not deduped. This matches upstream: routeSocketEvent consults the marker only for events_api (adapter-slack index.ts:3087-3135). I added a code comment and no new machinery.
  • Skipped fences lose content. There were two cases:
    • A line-leading unpaired ```npm test gives an empty code block. Upstream's remark does the same, so this is parity; I added a code comment.
    • A quoted ```npm test``` lost its content because of a Python parser bug. It is fixed by the CommonMark fence rule above, with a regression test.
• Round 2: PASS (no actionable findings).
• Round 3 (after merging main): RecursionError on deep > nesting, and the quadratic blockquote-line check. Both fixed.
• Round 4: the heading level reset the nesting budget. Fixed.
• Round 5: quotes plus nested lists overflowed the stack, and labelled links with [] or () leaked syntax into the text. Fixed (cap of 100, link pattern).
• Round 6: escaped \( in a link destination. Fixed.
• Round 7: PASS (no actionable findings).

Validation

ruff check, ruff format --check, audit_test_quality (0 hard failures), verify_test_fidelity --check-docs, --strict at 4.31.0, pytest (7971 passed, 24 skipped at 236ebf3), and pyrefly (0 errors).

Merge gate

Final HEAD: 236ebf3.

Review round (two independent reviewers, 9 findings). All 9 were fixed; none were declined. Each fix has a test that fails without it. Parser fixes were checked against remark-parse + remark-gfm toPlainText.

1. CHANGELOG [4.41/SL8b] Slack: Enterprise Grid org-wide installs, authorizations[] routing, retry marker (split from #213) #268 and [4.41/CB] Callback tokens: consume once, bind to conversation, shorter TTL, preserve button fields #194 bullets contradicted [4.41/SL4b] Slack inbound mrkdwn normalization, channel.post thread ids, Socket Mode retries (remainder of #209) #283. Both are rewritten (socket retries now reach the marker; slack:C…: is reported only until [4.41/SL4b] Slack inbound mrkdwn normalization, channel.post thread ids, Socket Mode retries (remainder of #209) #283 or without a string ts).
2. Nested link in link text ([[a](u) b](v)). A nested [...] may not be directly followed by (, so the inner link wins, as in remark.
3. Undocumented message.text gaps. Trailing whitespace is now fixed (see item 9). Lazy blockquote continuation, \r line endings and the new quadratic triggers are recorded in the docs/UPSTREAM_SYNC.md known gaps, the CHANGELOG and Shared parse_markdown: multi-backtick code spans, paragraph leading space, quadratic inputs (found in #283) #308 (comment).
4. -_- gave an empty message.text. A thematic break must now repeat one marker (CommonMark).
5. Snake_case words corrupted underscored link URLs. _ emphasis is never intraword now, so my_var and ?utm_source= keep their underscores.
6. Tests could not catch memo-scanner mutations. Added test_memoized_scanners_match_upstream_across_lines (8 exact upstream outputs). All 5 reported mutants are now killed.
7. New quadratic triggers from the fence rule. These are documented (wording corrected) and added to Shared parse_markdown: multi-backtick code spans, paragraph leading space, quadratic inputs (found in #283) #308 with timings. They are not fixed here: the per-byte cost equals the existing `x trigger on main, and the inline-parser rewrite belongs to Shared parse_markdown: multi-backtick code spans, paragraph leading space, quadratic inputs (found in #283) #308.
8. The socket retry was not tested against the event-id marker. Added test_socket_retry_consults_the_event_delivered_marker (seeded marker gives ack without dispatch; no marker gives dispatch). It kills the retry_num=0 mutant.
9. Leading and trailing spaces at paragraph edges. parse_markdown now strips each paragraph line's leading spaces/tabs and the paragraph's trailing ones, as CommonMark does. The run ```npm test``` please case is restored. The Shared parse_markdown: multi-backtick code spans, paragraph leading space, quadratic inputs (found in #283) #308 leading-space adaptation in test_slack_format.py is removed, and the test now asserts upstream's exact value.

gpt-6-astra (convergence): 2 rounds.

• Round 1 raised two P2s, both fixed with tests:
  • CRLF *** thematic breaks: a trailing \r is allowed.
  • Code spans in link labels: a closed code span in a label is opaque, as in CommonMark ([`[`](u)). The alternatives are mutually exclusive, so there is no backtracking blowup.
• Round 2: PASS ("No actionable regressions found").

Bots: CodeRabbit APPROVED 236ebf3 with no actionable comments. There were no gemini comments.

CI on 236ebf3: all checks green (Lint & Type Check, test 3.12/3.13, CodeQL).

Local validation on 236ebf3: all green. ruff, format, audit (0 hard failures), --check-docs, --strict 4.31.0, pytest (7971 passed), pyrefly (0 errors). Delta vs committed report (HEAD): missing 100 -> 100 (+0).

Closes #283
Part of #184

Summary by CodeRabbit

• New Features

  • Slack message text now reflects formatted content more consistently, including mentions, links, tables, attachments, and code blocks.
  • Slack channel posts can return a replyable thread ID when a timestamp is available.
  • Socket Mode retries are processed, while already-delivered events are not dispatched again.

• Bug Fixes

  • Improved Markdown parsing for links, emphasis, code fences, and blockquotes, including a limit on deeply nested quotes.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 8658f7f0-3fb3-4300-b2c9-fb0ab4e1927e

📥 Commits

Reviewing files that changed from the base of the PR and between 2aa43af and 236ebf3.

📒 Files selected for processing (15)

• CHANGELOG.md
• docs/UPSTREAM_SYNC.md
• scripts/fidelity_target.json
• src/chat_sdk/adapters/slack/adapter.py
• src/chat_sdk/adapters/slack/format.py
• src/chat_sdk/adapters/slack/format_converter.py
• src/chat_sdk/shared/markdown_parser.py
• tests/test_channel_faithful.py
• tests/test_markdown_faithful.py
• tests/test_slack_format.py
• tests/test_slack_format_primitives.py
• tests/test_slack_inbound_content.py
• tests/test_slack_inbound_mentions.py
• tests/test_slack_socket_mode.py
• tests/test_turn_cancellation.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Slack inbound parsing now normalizes mrkdwn before shared Markdown parsing and derives plain text from the formatted AST. Channel posts use a timestamp-based thread ID when Slack returns a string timestamp. Socket Mode retries are routed for processing and duplicate-event checks.

Changes

Slack message handling

Layer / File(s)	Summary
Slack mrkdwn conversion and Markdown parsing src/chat_sdk/adapters/slack/format.py, src/chat_sdk/adapters/slack/format_converter.py, src/chat_sdk/shared/markdown_parser.py, tests/test_slack_format*.py, tests/test_markdown_faithful.py, docs/UPSTREAM_SYNC.md, scripts/fidelity_target.json	Slack mrkdwn conversion handles special mentions, channel IDs, inverted links, and code fences before Markdown parsing. The shared parser adds balanced-link handling, CommonMark-related parsing changes, and a 100-level blockquote limit. Tests and parser documentation cover these changes.
Assembled inbound plain text src/chat_sdk/adapters/slack/adapter.py, tests/test_slack_inbound_content.py, tests/test_turn_cancellation.py, docs/UPSTREAM_SYNC.md	The adapter derives message text from the completed formatted AST. Tests cover normalized text from message bodies, tables, and attachments.
Channel post IDs and Socket Mode retries src/chat_sdk/adapters/slack/adapter.py, tests/test_slack_inbound_mentions.py, tests/test_slack_socket_mode.py, tests/test_channel_faithful.py, CHANGELOG.md, docs/UPSTREAM_SYNC.md	Channel posts return a timestamp-based thread ID when the response contains a string timestamp. Socket Mode retry envelopes are logged and routed; event routing can suppress retries whose event IDs are already marked delivered. Documentation and tests reflect these behaviors.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant SlackAdapter
  participant SlackFormatConverter
  participant slack_mrkdwn_to_markdown
  participant parse_markdown
  participant ast_to_plain_text
  SlackAdapter->>SlackFormatConverter: to_ast(text)
  SlackFormatConverter->>slack_mrkdwn_to_markdown: Normalize mrkdwn
  slack_mrkdwn_to_markdown-->>SlackFormatConverter: Markdown
  SlackFormatConverter->>parse_markdown: Parse normalized Markdown
  parse_markdown-->>SlackFormatConverter: Formatted AST
  SlackAdapter->>ast_to_plain_text: Derive text from formatted AST

Loading

Merge Risk: 🔵 Low · up to 236eb

This change alters how Slack inbound text is derived, returns replyable thread IDs for channel posts, and routes Socket Mode retries. Automated validation reportedly passes. The live Slack-loop check is still pending, so owners should confirm it before or shortly after merge.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 236eb

Retry recovery improves delivery of missed events, but non-message callbacks have weaker replay protection than messages. Duplicate delivery or delivery-marker failures can repeat application handlers. The security impact depends on whether those handlers perform non-idempotent actions; no authorization bypass was established.

Retained concerns

• Medium · reliability · inferred: Newly routed Socket Mode retries can repeat reaction callbacks when concurrent deliveries pass the separate marker lookup or marker storage fails. The marker is written asynchronously before callback completion, and reaction dispatch has no equivalent of message-ID deduplication. Applications using reaction handlers for non-idempotent security decisions or privileged operations can therefore replay effects. Default message deduplication is strong counterevidence for messages, but does not cover reactions; downstream privileged effects remain application-dependent.

Security review details

Security Blast Radius

• inferred — The supported replay exposure reaches application reaction handlers through a valid Slack event and provider redelivery. It does not require forging installation identity. Effects are bounded by registered handlers and their available authority; access to additional services or data stores was not established.

Security Findings and Attack Paths

• inferred — A delivery-marker race or storage failure can admit a previously dispatched reaction again, reaching the same application handler. This supports a replay concern, not a verified privilege-escalation finding. Interactive and slash-command retries also lack equivalent deduplication, but whether Slack supplies positive retry metadata for those envelope types remains unresolved.

Trust Boundaries and Controls

• observed — Socket Mode uses the configured app-level credential. Events API dispatch retains installation resolution, preferring authorization identity and dropping unresolved installations. Timestamp-derived reply IDs use the existing channel/thread representation rather than introducing a separate authorization path.

Resilience and Maintainability Implications

• observed — Retry tests cover recovery of an unmarked message and suppression of an already-marked event. They do not establish callback-completion recovery: acknowledgement precedes dispatch, and a marker can remain even if the subsequent callback fails.

Hardening Proposals

• proposed — Define callback-level idempotency for retriable non-message events, with an atomic event claim and an explicit failure/recovery policy before non-idempotent handlers run. Distinguish dispatched work from completed effects so replay prevention does not silently suppress recoverable failures.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR changes the shared src/chat_sdk/shared/markdown_parser.py behavior for balanced links, intraword underscores, thematic breaks, blockquote depth, fence info strings, and paragraph whitespace. …	Split the shared Markdown parser behavior changes into a separate issue or pull request, or remove them from this PR. Keep only shared-parser changes that are demonstrated as necessary for the Slack normalization requirements.
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 100 functions across 12 files. (3 skipped…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #283 coding requirements are implemented. Slack normalization now handles special mentions, channel labels and IDs, inverted links, entities, and code fences. SlackFormatConverter.to_ast uses …
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main Slack changes: inbound mrkdwn normalization, channel.post thread IDs, and Socket Mode retries.

Full details: Out of Scope Changes check

Explanation

The PR changes the shared src/chat_sdk/shared/markdown_parser.py behavior for balanced links, intraword underscores, thematic breaks, blockquote depth, fence info strings, and paragraph whitespace. Issue #283 identifies shared parser gaps as non-Slack-specific and requires them to be recorded or filed separately. The issue scope lists Slack adapter changes, not general parser behavior. The PR does not establish that these broad parser changes are required for the Slack objectives.

Full details: Docstring Coverage

Explanation

Docstring coverage is 28.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 100 functions across 12 files. (3 skipped: 3 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit read the Slack text stream,
And parsed each fence, link, and gleam.
“Here’s plain text from the AST,”
It said, then hopped through threads with glee.
Retries came; the checks kept track,
The rabbit nibbled clover, then hopped back.}

_{Comment @coderabbitai help to get the list of available commands.}

[patrick-chinchill]

Merge gate: CI green (Lint & Type Check, test (3.12), test (3.13), Analyze (python), Analyze (actions), CodeQL, CodeRabbit); local Codex review (gpt-6-astra, xhigh, --base origin/main) on 236ebf3: "No actionable regressions found against the specified merge base. The full test suite passed: 7,971 passed and 24 skipped"; 2 astra rounds (convergence; round 1 P2s fixed with tests); CodeRabbit APPROVED with no actionable comments, no gemini comments. origin/main is an ancestor of HEAD, so the reviewed code is the merged code. Merging with --admin (Protect Main requires a code-owner approval).