From 4baabbd8e1618e69d34fcc6b32be7244d49844ee Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 03:18:14 -0700 Subject: [PATCH 01/10] fix(slack): inbound mrkdwn normalization, channel.post thread ids, process Socket Mode retries (#283) Part (b) of #209. Ports the Slack halves of vercel/chat 0b63791b (#667), 92530dd3 (#720), c3118279 (#756), e71bfead (#843) and 44423bdc (#960). - slack_mrkdwn_to_markdown: special mentions, #name (C...) channels, inverted links, Slack code fences on their own lines (linear scanners). - SlackFormatConverter.to_ast = parse_markdown(slack_mrkdwn_to_markdown()); drop the Python-only regex extract_plain_text override. - message.text = ast_to_plain_text(formatted) on both parse paths. - post_channel_message returns slack:{channel}:{ts} for a string ts. - Socket Mode retry envelopes are routed and logged instead of dropped. --- CHANGELOG.md | 10 +- docs/UPSTREAM_SYNC.md | 113 +++++---- src/chat_sdk/adapters/slack/adapter.py | 69 +++--- src/chat_sdk/adapters/slack/format.py | 231 +++++++++++++++++- .../adapters/slack/format_converter.py | 59 +---- tests/test_channel_faithful.py | 7 +- tests/test_slack_format.py | 63 ++++- tests/test_slack_format_primitives.py | 110 ++++++++- tests/test_slack_inbound_content.py | 54 ++-- tests/test_slack_inbound_mentions.py | 92 +++++++ tests/test_slack_socket_mode.py | 35 ++- 11 files changed, 654 insertions(+), 189 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f9046b7..3ca6ce08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` stays `4.31.0` until #203. +- **Slack inbound mrkdwn normalization, `channel.post()` thread ids, Socket Mode retries** (#283, part (b) of #209; ports vercel/chat `0b63791b` #667 (Slack half), `92530dd3` #720, `c3118279` #756, `e71bfead` #843 (Slack half) and `44423bdc` #960, chat@4.33.0–4.41.1). A live Slack-loop check is pending. + - **Breaking/consumer-visible (`message.text` is the plain text of `message.formatted`):** both parse paths now set `text = ast_to_plain_text(formatted)` (upstream `toPlainText(formatted)`), replacing the regex pass over the mrkdwn. Markup no longer leaks into `text`: inline-code backticks, list markers (`- a` → `a`, `1. one` → `one`), heading `#` and quote `>` prefixes are dropped, a trailing newline or a whitespace-only body is trimmed, and blocks are separated by a blank line. `SlackFormatConverter.extract_plain_text` drops its Python-only regex override for the base `to_ast` + `ast_to_plain_text` (upstream has none). *Migration:* match structure on `message.formatted`, or on `message.raw["text"]` for the original mrkdwn. + - **Breaking/consumer-visible (mentions and links read differently):** `` / `` / `` become `@here` / `@channel` / `@everyone`, `` becomes `@eng` (`` → `@S1`), and a labelled channel keeps its id: `<#C123|general>` reads `#general (C123)` instead of `#general`. Tokens inside inline code or code blocks stay literal. A swapped `` link becomes `[label](https://…)`. `<` / `>` / `&` are now unescaped in `formatted` and `text` (they came through verbatim). The same applies to table cells and mrkdwn attachment parts (#210). *Migration:* update `on_message` regexes and anything matching `#channel-name` or raw `` / ``. + - **Breaking/consumer-visible (code blocks keep their first line):** Slack treats the text right after an opening ```` ``` ```` as code; it was read as an info string and dropped, so `` ```npm test``` `` gave an empty `formatted` (and `text` kept the backticks). Each paired fence now parses as a `code` node and `text` holds the code (`"npm test"`). Unpaired fences, fences in inline code or `<…>` tokens, and fences on `>` quote lines stay literal, as in Slack. + - **Breaking/consumer-visible (`channel.post()` ids):** `post_channel_message` returns `slack:C123:` (a replyable thread rooted at the new message) instead of `slack:C123:` when Slack returns a string message `ts`, so `chat.thread(sent.thread_id).post(...)` replies in its thread. File-only posts keep `slack:C123:`. + - **Breaking/consumer-visible (Socket Mode retries are processed):** envelopes with `retry_attempt > 0` were acked and dropped; they are now routed like first deliveries and logged at info (`"Processing socket mode retry"` with `retry_attempt`, `retry_reason`, `type`). The event-id marker (#268) and core message-id dedupe (10 min TTL, #191) drop true duplicates, so an event missed during a restart or reconnect is no longer lost. + - The mrkdwn scanners stay linear on untrusted text (results identical to upstream). Known shared-parser gaps (no multi-backtick code spans, a paragraph's leading space kept, quadratic time on some inputs) predate this change; see `docs/UPSTREAM_SYNC.md`. + - **Slack Agent Sessions lifecycle and native stop** (#215; Slack half of vercel/chat `2ce2be00` #862, chat@4.39.0, plus `8b6d7f3a` #882 and `2cc8cc3f` #897, chat@4.40.0). Only with `agent_view=True`; without it nothing changes (no `agents.sessions.*` call, byte-identical stream stop payloads). A live check in an `agent_view` workspace is still pending. - **Consumer-visible (agent_view typing):** `start_typing()` without a status (and `thread.start_typing()`) now moves the agent session to `processing` through `agents.sessions.setStatus` (with `initiator_user_id`) instead of calling `assistant.threads.setStatus` with "Typing..."; `start_typing(thread_id, "")` moves it to `active`. A custom status still goes through `assistant.threads.setStatus`. A failed `agents.sessions.setStatus` is logged, with no legacy fallback. The adapter now has `end_typing`, so a reply posted after `thread.start_typing()` (a post, or a post+edit stream) moves the session back to `active` (or the `StreamingPlanOptions.session_status`). - **Consumer-visible (agent_view status and titles):** `set_assistant_status(channel, ts, "")` (or whitespace) sets the session `active` instead of sending an empty status; a custom status without `loading_messages` (explicit or configured) now sends `loading_messages=[status]`. `set_assistant_title` calls `agents.sessions.rename` instead of `assistant.threads.setTitle`. @@ -84,7 +92,7 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **Consumer-visible (Slack):** `message.links` gains each non-unfurl attachment's `title_link`. - **Routing:** attachment and table content now reaches `message.text`, so `on_message` regex patterns (and the core's text-based mention fallback when the bot id is unknown) can match it. Alert-bot messages in subscribed or pattern-matched channels may start matching; filter on `message.author.is_bot` if that is unwanted. LLM prompts built from `message.text` (`to_ai_messages`, history) get the extra content. Streaming is unaffected. - The async parse path resolves `<@U…>` / `<#C…>` in table cells and attachment parts in one parallel lookup wave; the sync `parse_message` path does no lookups. `SlackEvent` gains typed `attachments` (`SlackAttachment`) and `SlackMessageBlock` (with `rows`). - - **Python-specific (temporary, until #283):** upstream derives all of `text` from `formatted`. That needs #283's inbound mrkdwn normalization; without it, code on a fence's opening line would be lost (`` ```npm test``` `` would give empty text). #283 switches to `ast_to_plain_text(formatted)`, which also drops list, `#`, `>` and backtick markers from body text. Until then, table cells and mrkdwn attachment parts render through the current converter: Slack's `&` / `<` / `>` entities stay, a usergroup shows as a raw `` token, and a labelled channel shows as `#name` (upstream shows `#name (C…)`). Body `formatted` has the same gaps today. Live Slack-loop check: pending. + - **Python-specific (temporary, resolved by #283, see the #283 entry above):** upstream derives all of `text` from `formatted`. That needs #283's inbound mrkdwn normalization; without it, code on a fence's opening line would be lost (`` ```npm test``` `` would give empty text). #283 switches to `ast_to_plain_text(formatted)`, which also drops list, `#`, `>` and backtick markers from body text. Until then, table cells and mrkdwn attachment parts render through the current converter: Slack's `&` / `<` / `>` entities stay, a usergroup shows as a raw `` token, and a labelled channel shows as `#name` (upstream shows `#name (C…)`). Body `formatted` has the same gaps today. Live Slack-loop check: pending. - **Slack: Agent messaging experience (`agent_view`) and declarative agent config** (#214; Slack halves of vercel/chat `1721fa01` #684 and `0f743c9b` #698, plus `78021c09` #889 and the Slack part of `c21ccbc0` #943, chat@4.34.0–4.41.0). Slack will retire `assistant_view` in February 2027. Every feature is opt-in on `SlackAdapterConfig` and off by default. A live Slack-loop check is still pending. - `agent_view=True`: `app_home_opened` fires for every tab and carries `tab` and, when Slack folds one in, `entities`. Each top-level DM message becomes its own thread (`slack:D…:{ts}` instead of `slack:D…:`), so replies thread under the user's message. If the conversation-scoped `slack:D…:` from `open_dm` is subscribed, top-level DMs still route there, so `on_subscribed_message` keeps working. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index a5419579..9f83c741 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -445,17 +445,17 @@ The channel-edit divergence exists because the thread id reported for a channel post often never equals a click's thread id: - Teams and Google Chat `post_channel_message` return the channel id as the thread id (upstream too); -- Slack `post_channel_message` returns the synthetic `slack:C…:` until #283 - ports upstream `92530dd3` (vercel/chat#720), while a click reports - `slack:C…:`. After #283 a Slack *DM* post reports +- Slack `post_channel_message` returned the synthetic `slack:C…:` until #283 + ported upstream `92530dd3` (vercel/chat#720), while a click reports + `slack:C…:`. Since #283 a Slack *DM* post reports `slack:D…:`, but a DM click reports `slack:D…:` (Python's DM `_handle_block_actions` divergence), so a thread scope would still miss; - chained edits (`sent = await sent.edit(...)` twice), whose returned `SentMessage` dropped the thread-id override before #195 ported `16ea171e` (it now keeps it, so this reason no longer applies on its own). -Binding to the channel resolves all of these, independent of #283's merge -order, because every click on the message derives the same channel id. +Binding to the channel resolves all of these, because every click on the +message derives the same channel id. **Breaking for `callback_url` users:** tokens minted before the upgrade stop resolving, a repeat click no longer POSTs, tokens expire after 7 days, and the @@ -1000,7 +1000,7 @@ Parity with the core halves of upstream `0b63791b` (chat@4.33.0), `f233ffe8`, `None` and resolves with `is not None` (upstream `??`). Before this change the dataclass default `300000` always won and `0` fell through `or` to the constant. `0` now reaches the state adapter, and the bundled backends treat a - `0` TTL as no expiry (upstream `state-redis` does the same). The Slack Socket Mode retry-envelope half of `0b63791b` is #283 (split from #209). + `0` TTL as no expiry (upstream `state-redis` does the same). The Slack Socket Mode retry-envelope half of `0b63791b` landed in #283 (split from #209). - **`wait_until` and handler errors (`c21ccbc0`, `91683e52`).** Upstream hands `waitUntil` a `task.catch(log)` promise that fulfils when the handler fails. Python passed the raw task, so a host that awaited it saw handler errors @@ -1150,7 +1150,8 @@ Parity, with no new divergence. Part (a) of #209 ports the Slack halves of `bb7cd124` (#716) and `80def3ab` (#707, chat@4.35.0), `51322dde` (#891) and `c2b6bff0` (#883, chat@4.40.0), and `683eadc1` (#947, chat@4.41.0). Part (b) (the mrkdwn normalization from `e71bfead` / `44423bdc` / `c3118279`, the -`92530dd3` channel-post id and the `0b63791b` Socket Mode retries) is #283. +`92530dd3` channel-post id and the `0b63791b` Socket Mode retries) landed in +#283; see "Slack inbound mrkdwn, channel-post ids, Socket Mode retries". - **Self mention (`51322dde`).** `_resolve_inline_mentions` no longer skips the bot's id. `<@U_BOT>` resolves through `_lookup_user_name` like any other @@ -1191,17 +1192,16 @@ Parity, with no new divergence. Part (a) of #209 ports the Slack halves of ### Slack inbound content: pasted tables and alert attachments (chat@4.38–4.39, #210) -Parity apart from one temporary divergence (the body's plain text, below, -until #283). Ports the Slack halves of `764e4759` (#817, +Parity. Ports the Slack halves of `764e4759` (#817, chat@4.38.1) and `864d9222` (#846, chat@4.39.0); the core `toPlainText` table rules were #193 and the `previous_message` hunk of #846 is #211. - **Content assembly.** Both parse paths build `formatted` from the body text plus `table` / `data_table` blocks (tables before the first non-table block above the text, the rest below) plus each non-unfurl attachment's content. - `text` appends the tables' and attachments' `ast_to_plain_text` to the - body's plain text (see the #283 bullet for how the body's share is - derived). The helpers are module-level in + `text` is `ast_to_plain_text(formatted)` (upstream `toPlainText(formatted)`; + until #283 the body's share kept the pre-#210 regex rendering). The + helpers are module-level in `adapters/slack/adapter.py` (no separate module): `_block_text`, `_has_bold_text`, `_table_data`, `_event_tables`, `_attachment_content`, `_literal_phrasing`, `_collect_mention_ids`, `_apply_mention_names`, and the @@ -1234,36 +1234,6 @@ rules were #193 and the `previous_message` hunk of #846 is #211. Python slices copy, so the upstream shape is quadratic (about 4 s on 200k characters of `<@U1>` tokens) now that it also runs over attachment text. Output is identical (fuzzed against the upstream-shaped versions). -- **Temporary divergence until #283: the body's plain text.** Upstream - derives `text = toPlainText(formatted)`, and its `toAst` runs - `slackMrkdwnToMarkdown`, which moves code after an opening fence onto its - own line, unescapes `&` / `<` / `>`, renders `` as - `@…` and `<#C…|name>` as `#name (C…)`. The Python - `SlackFormatConverter.to_ast` does none of this yet (#283 ports it), so - deriving all of `text` from `formatted` would drop code from ordinary - messages (`` ```npm test``` `` would give `text == ""`). Until #283, - `_assemble_content` builds `text` as the blank-line join of the non-empty - plain texts of the leading tables, `extract_plain_text(body)` (the pre-#210 - rendering, so a message without tables or attachments gets the same `text` - as before) and the trailing tables and attachment nodes. That is what - `toPlainText` does with root children, so only the body's share differs - from upstream. #283 replaces it with `ast_to_plain_text(formatted)` and - updates these assertions in `tests/test_slack_inbound_content.py`: - `test_body_text_keeps_its_pre_283_rendering` (to upstream's fence output), - `test_keeps_attachment_content_out_of_an_unclosed_code_fence_in_the_body` - (upstream's `"Deploy failed:\n\nTypeError: boom\n\n…"`), - `test_preserves_rich_text_metadata_within_table_cells` (upstream's - `"#C789 @S789 July 11 #ff0000"`) and - `test_resolves_user_and_channel_mentions_in_table_cells` - (`"@Test Bot\t#general (C789)"`). - Table cells and mrkdwn attachment parts go through `to_ast` already, so - until #283 they show the same gaps body `formatted` shows today: a cell - fence loses its opening line, a `raw_text` cell holding `R&D` or `<@U…>` - (escaped by `_block_text`, as upstream does) shows `R&D` / - `<@U…>`, a usergroup cell shows `` and a labelled - channel shows `#name`. That content was absent before #210. Literal - attachment parts unescape on their own (`_literal_phrasing`) and already - match upstream. - **`title_link` and the unfurl wait.** On a webhook, an untitled `title_link` preview makes `_enrich_links` poll the unfurl cache (up to 2 s) exactly as upstream does. Fetched (history) messages poll too, only @@ -1271,6 +1241,55 @@ rules were #193 and the `previous_message` hunk of #846 is #211. already applied to every untitled link in fetched messages and is tracked there and in #292. +### Slack inbound mrkdwn, channel-post ids, Socket Mode retries (chat@4.33–4.41, #283) + +Part (b) of #209. Parity, with no new divergence. Ports the Slack halves of +`0b63791b` (#667, chat@4.33.0), `92530dd3` (#720, chat@4.35.0), `c3118279` +(#756, chat@4.37.0), `e71bfead` (#843, chat@4.39.0) and `44423bdc` (#960, +chat@4.41.1). + +- **mrkdwn (`e71bfead`, `44423bdc`, `c3118279`).** `slack/format.py` ports + `convertSpecialMentions` and `convertMrkdwnWithCodeFences` with their + helpers (`findAngleTokenEnd`, `findInlineCodeEnd`, `isOnBlockquoteLine`, + `escapeLeadingBlockMarker`), the `#name (C…)` channel rewrite and the + inverted-link swap. As upstream, the fence normalizer is a separate copy of + `shared.code_fences` with mrkdwn's entity escaping (`>` quote lines, + `<…>` tokens). JS `^`/`$` without `m` are `re.match`/`\Z`, and JS `\d` is + `[0-9]`. +- **`to_ast` and `message.text`.** `SlackFormatConverter.to_ast` is + `parse_markdown(slack_mrkdwn_to_markdown(text))`, as upstream `toAst`. The + old inline regex copy skipped `unescape_slack_text`, so `<` / `>` / + `&` reached `formatted` and `text` verbatim. The Python-only regex + `extract_plain_text` override is removed (upstream `markdown.ts` has none; + the base is `to_ast` + `ast_to_plain_text`). Both parse paths set + `text = ast_to_plain_text(formatted)` over the assembled content (upstream + `toPlainText(formatted)`), replacing the #210 interim that kept the regex + rendering for the body's share. Consequences: list markers, `#`, `>` and + backticks no longer appear in `message.text`, and blocks join with a blank + line. +- **`post_channel_message` (`92530dd3`)** returns `slack:{channel}:{ts}` when + the response's `raw["ts"]` is a `str`, else the synthetic `slack:{channel}:` + (file-only uploads), as upstream's `typeof result.raw.ts !== "string"` check. +- **Socket Mode retries (`0b63791b`).** Retry envelopes are routed like first + deliveries. The event-id marker (#268) drops a retry whose first delivery + was dispatched, and core message-id dedupe (10 min TTL, #191) drops true + duplicates. slack_sdk's `SocketModeRequest.retry_attempt` / `retry_reason` + are upstream's `retry_num` / `retry_reason`; the info log + `"Processing socket mode retry"` uses the key `retry_attempt`. +- **Linear-time scanning (implementation, not divergence).** Results equal + upstream's. A failed `<…>` scan is remembered so a run of unclosed `<` stays + linear, the inline-code newline check is bounded by the closing backtick, + and the blockquote-line check matches in place instead of slicing. + `tests/test_slack_format_primitives.py` covers 50k-character inputs. +- **Known gaps (shared parser, not Slack-specific).** Two `markdown.test.ts` + cases are adapted: `parse_markdown` has no multi-backtick code spans + (```` ```c``` ```` inside a quote stays backticked text, where remark reads a + code span) and keeps a paragraph's leading space (`"```x``` > note"` + gives `"x\n\n > note"`, upstream `"x\n\n> note"`). `parse_markdown` is + also quadratic on some inputs (about 2.4 s for 20k characters of `` `x ``); + this predates the port, since `to_ast` always ran it on every inbound + message, and is tracked separately. + ### Teams Adaptive Card 1.5 rendering (chat@4.36–4.41, #220) Parity, not a divergence. The Teams half of `0153a39f` (chat@4.36.0), @@ -1365,11 +1384,9 @@ chat@4.35.0); the installation-scoped caches landed in #205. Code is in Python-specific notes: -- **Socket retries before #283.** `_on_socket_request` still acks and skips - envelopes with `retry_attempt > 0`, so on a live socket the marker only - sees first deliveries; it already covers HTTP retries and forwarded socket - events. #283 (split from #209) removes the skip, after which `retry_attempt` reaches the - marker unchanged. +- **Socket retries.** Since #283, `_on_socket_request` routes envelopes with + `retry_attempt > 0` like first deliveries, so `retry_attempt` reaches the + marker on a live socket too (before, retries were acked and dropped). - **Flag normalization.** `is_enterprise_install` counts only as `True` or `"true"` everywhere (upstream's event path uses `Boolean(...)`, so a `"false"` string would count there). Slack sends booleans in event JSON, so @@ -3160,7 +3177,7 @@ stay explicit instead of being rediscovered in code review. | Area | Python behavior | TS behavior | Rationale | |------|----------------|-------------|-----------| | JSX Card/Modal elements | Not supported; tests skipped | `Card()` returns JSX element | Python has no JSX runtime | -| Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` binds to `{threadId, "thread"}`, where `threadId` is the id the adapter reported for the post (or the channel id) | The reported thread id often never equals a click's thread id: Teams and Google Chat report the channel id (upstream as well; Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), Python's Slack reports the synthetic `slack:C…:` until #283 while clicks carry the message ts, a Slack DM click carries no ts even after #283 makes the post report one, and a chained edit dropped the override until #195 ported `16ea171e`. Upstream's edited buttons never POST in those cases. Every click on the message derives the channel id, and the channel scope is no broader than the original post's. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_slack_channel_card_resolves_for_the_real_click` (real Slack id functions and `_handle_block_actions`, channel and DM, before and after #283), `::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. To be filed as an upstream issue against vercel/chat (Teams and Google Chat edited channel cards never POST); upstream Slack is unaffected, since its `postChannelMessage` and DM clicks both carry the message ts. | +| Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` binds to `{threadId, "thread"}`, where `threadId` is the id the adapter reported for the post (or the channel id) | The reported thread id often never equals a click's thread id: Teams and Google Chat report the channel id (upstream as well; Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), Python's Slack reported the synthetic `slack:C…:` until #283 (it still does when the response has no string ts) while clicks carry the message ts, a Slack DM click carries no ts even though the post now reports one, and a chained edit dropped the override until #195 ported `16ea171e`. Upstream's edited buttons never POST in those cases. Every click on the message derives the channel id, and the channel scope is no broader than the original post's. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_slack_channel_card_resolves_for_the_real_click` (real Slack id functions and `_handle_block_actions`, channel and DM, before and after #283), `::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. To be filed as an upstream issue against vercel/chat (Teams and Google Chat edited channel cards never POST); upstream Slack is unaffected, since its `postChannelMessage` and DM clicks both carry the message ts. | | Callback-token lease fence (4.41 wave, #194) | After deleting a matched record, `resolve_callback_url` calls `extend_lock(lock, CALLBACK_LOCK_TTL_MS)`. If that fails, the 10 s lease lapsed mid-consume, and the call returns `None` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. To be filed as an upstream issue against vercel/chat (a stalled `get`/`delete` past the 10 s lease lets a second click double-POST). Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | | Link-preview fence slicing (4.41 wave, #195) | `_render_link_for_prompt` bounds url/title/description/site with Python slicing, which counts code points | `renderLinkForPrompt` slices with `String.prototype.slice`, which counts UTF-16 code units | Only differs for astral characters (emoji and the like): a bounded field keeps up to the limit in code points, where JS keeps half as many astral characters and can end on a lone surrogate. Emulating UTF-16 slicing would produce lone surrogates that break UTF-8 encoding of the prompt. Whitespace handling is not a divergence: the normalizer uses JS's exact `\s`/`trim` set. Regression test: `tests/test_ai_messages.py::TestLinkPreviews::test_link_metadata_bounds_count_code_points_not_utf16_units`. | | Adapter hook signature probe (4.41 wave, #200, #201) | `Thread.start_typing` passes `options=TypingOptions(...)` to `adapter.start_typing` under the same rule (a two-argument `start_typing(thread_id, status)` is called with two arguments). `Thread`/`Channel.post_ephemeral` pass `options=` to `adapter.post_ephemeral` only when `chat_sdk._compat.accepts_kwarg` finds an `options` parameter (positional-or-keyword or keyword-only) or `**kwargs`; an implementation with the older `(thread_id, user_id, message)` signature is called with three arguments | `adapter.postEphemeral(threadId, userId, postable, options)` and `adapter.startTyping(threadId, status, { initiatorUserId })` always | JavaScript drops extra arguments, while Python raises `TypeError`, so passing `options=` unconditionally would break third-party adapters written against the pre-4.41 signature. In-repo adapters all accept it. Regression tests: `tests/test_thread_faithful.py::TestPostEphemeral::test_three_argument_custom_post_ephemeral_still_works`, `tests/test_channel_faithful.py::TestChannelPostEphemeral::test_three_argument_custom_post_ephemeral_still_works_from_a_channel` and `tests/test_compat.py::test_accepts_kwarg`; for `start_typing`, `tests/test_turn_cancellation.py::TestThreadAbortAndTyping::test_two_argument_custom_start_typing_still_works`. | diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 84f22904..7cc7cf91 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -3701,16 +3701,24 @@ async def ack(response_payload: dict[str, Any] | None = None) -> None: {"envelope_id": envelope_id, "error": str(exc)}, ) - # Slack re-delivers events that weren't acked in time. Skip retries - # so we don't double-process — but still ack so Slack stops resending. if retry_attempt and retry_attempt > 0: - await ack() - self._logger.debug("Skipping socket mode retry", {"retry_attempt": retry_attempt}) - return + # Slack redelivers an event when a prior delivery wasn't acked -- + # including events sent while the app had no open socket (restart, + # deploy, connection refresh). Route it like a first delivery: + # the event_id marker and ``Chat.process_message``'s message-id + # dedupe drop an already-handled duplicate, while a genuinely + # missed event is recovered instead of being lost (upstream + # vercel/chat#667). slack_sdk names upstream's ``retry_num`` + # ``retry_attempt``. + self._logger.info( + "Processing socket mode retry", + { + "retry_attempt": retry_attempt, + "retry_reason": getattr(request, "retry_reason", None), + "type": event_type, + }, + ) - # ``retry_attempt`` feeds the event_id retry marker. While the skip - # above stays (#283 replaces it with upstream's "process retries"), - # only forwarded socket events reach the marker with a retry count. await self._route_socket_event(payload, event_type, ack, retry_num=_parse_retry_num(retry_attempt)) async def _route_socket_event( @@ -5516,32 +5524,19 @@ def _assemble_content( ) -> tuple[FormattedContent, str]: """Body AST with leading tables above it, then trailing tables and attachments. - Returns ``(formatted, plain_text)``. Upstream derives ``message.text`` - as ``toPlainText(formatted)``. Until #283 ports upstream's inbound - mrkdwn normalization (``slackMrkdwnToMarkdown``), our ``to_ast`` drops - code on a fence's opening line (the body ``"```npm test```"`` parses to - nothing), so the body's share of the plain text keeps the regex - ``extract_plain_text`` used before #210, and only the table and - attachment nodes go through ``ast_to_plain_text``. A message without - tables or attachments gets exactly the pre-#210 text. Otherwise the - pieces are joined with a blank line, skipping empty ones, as - ``toPlainText`` joins root children (the body loses trailing - whitespace there, as a parsed paragraph would). Temporary divergence: - #283 replaces this with ``ast_to_plain_text(formatted)``. + Returns ``(formatted, plain_text)``; the plain text is upstream's + ``text: toPlainText(formatted)`` at each ``assembleContent`` call + site, so ``message.text`` is the plain text of everything + ``message.formatted`` holds. """ - before = [self._table_node(data) for data in tables.leading] - after = [*(self._table_node(data) for data in tables.trailing), *attachment_nodes] formatted = self._format_converter.to_ast(text) - formatted["children"] = [*before, *formatted.get("children", []), *after] - body = self._format_converter.extract_plain_text(text) - if not (before or after): - return formatted, body - pieces = [ - *(ast_to_plain_text(node) for node in before), - body.rstrip(JS_WHITESPACE), - *(ast_to_plain_text(node) for node in after), + formatted["children"] = [ + *(self._table_node(data) for data in tables.leading), + *formatted.get("children", []), + *(self._table_node(data) for data in tables.trailing), + *attachment_nodes, ] - return formatted, "\n\n".join(piece for piece in pieces if piece) + return formatted, ast_to_plain_text(formatted) def _attachment_nodes(self, content: _AttachmentContent) -> list[Content]: """Render one attachment's content to block nodes (upstream ``attachmentNodes``). @@ -7402,7 +7397,17 @@ async def post_channel_message(self, channel_id: str, message: AdapterPostableMe raise ValidationError("slack", f"Invalid Slack channel ID: {channel_id}") synthetic_thread_id = f"slack:{channel}:" - return await self.post_message(synthetic_thread_id, message) + result = await self.post_message(synthetic_thread_id, message) + + # The new message roots its own thread: return a replyable thread id + # (upstream vercel/chat#720). Without a string ``ts`` (e.g. a + # file-only upload) there is nothing to address, so keep the + # channel-scoped id. + raw = result.raw + ts = raw.get("ts") if isinstance(raw, dict) else None + if not isinstance(ts, str): + return result + return replace(result, thread_id=self.encode_thread_id(SlackThreadId(channel=channel, thread_ts=ts))) # ================================================================== # Thread ID encoding / decoding diff --git a/src/chat_sdk/adapters/slack/format.py b/src/chat_sdk/adapters/slack/format.py index 66bebb6e..2d191d02 100644 --- a/src/chat_sdk/adapters/slack/format.py +++ b/src/chat_sdk/adapters/slack/format.py @@ -17,6 +17,8 @@ from datetime import datetime from typing import Literal, NotRequired, TypedDict +from chat_sdk.shared._js_compat import JS_WHITESPACE as _JS_WHITESPACE + class SlackPlainTextObject(TypedDict): """A Slack ``plain_text`` composition object.""" @@ -40,7 +42,25 @@ class SlackMrkdwnTextObject(TypedDict): _DATE_CONTROL_PATTERN = re.compile(r"[\^|>]") _SLACK_ID_PATTERN = re.compile(r"^[A-Z0-9_]+$") _SLACK_USER_TOKEN_PATTERN = re.compile(r"(?]*)?>\Z") +_LABELED_GROUP_PATTERN = re.compile(r"\A]+)>\Z") +_GROUP_PATTERN = re.compile(r"\A\Z") _TEXT_OBJECT_MAX_LENGTH = 3000 +_CODE_FENCE = "```" +_LEADING_WHITESPACE_PATTERN = re.compile(r"^[ \t]+") +# Line prefixes CommonMark promotes to a block construct (blockquote, +# heading, list item, fence, thematic break, HTML) -- in pre-unescape form. +# ``[0-9]`` is JS ``\d`` (Python's ``\d`` matches every Unicode digit). +_BLOCK_MARKER_PATTERN = re.compile( + r"^(?:>|<|#{1,6}(?=[ \t\n]|\Z)|[-+*](?=[ \t\n]|\Z)|`{3,}|~{3,}|(?:[-*_][ \t]*){3,}(?=\n|\Z))" +) +_ORDERED_LIST_MARKER_PATTERN = re.compile(r"^([0-9]{1,9})([.)])(?=[ \t\n]|\Z)") +# First character that ends a ``<...>`` token scan: its close, or a line break. +_ANGLE_TOKEN_STOP = re.compile(r"[>\n\r]") +# What JS ``trimStart`` removes. +_JS_LEADING_WHITESPACE = re.compile(f"[{re.escape(_JS_WHITESPACE)}]*") def escape_slack_text(text: str) -> str: @@ -131,25 +151,220 @@ def format_slack_date( def slack_mrkdwn_to_markdown(mrkdwn: str) -> str: """Normalize Slack mrkdwn to standard Markdown. - Rewrites user/channel mentions, links, bold, and strikethrough, then + Rewrites user, channel and special mentions, links (including the + inverted ```` form), bold, and strikethrough; puts + each paired ```` ``` ```` fence on its own lines (Slack treats text right + after an opening fence as code, CommonMark as the info string); then unescapes Slack's ``&``/``<``/``>`` entities. """ - markdown = mrkdwn + markdown = _convert_mrkdwn_with_code_fences(mrkdwn) if _CODE_FENCE in mrkdwn else _convert_mrkdwn_text(mrkdwn) + return unescape_slack_text(markdown) + + +def _convert_slack_tokens(mrkdwn: str) -> str: # User mentions: <@U123|name> -> @name or <@U123> -> @U123 - markdown = re.sub(r"<@([A-Z0-9_]+)\|([^<>]+)>", r"@\2", markdown) + markdown = re.sub(r"<@([A-Z0-9_]+)\|([^<>]+)>", r"@\2", mrkdwn) markdown = re.sub(r"<@([A-Z0-9_]+)>", r"@\1", markdown) - # Channel mentions: <#C123|name> -> #name - markdown = re.sub(r"<#[A-Z0-9_]+\|([^<>]+)>", r"#\1", markdown) + # Channel mentions keep the id: <#C123|name> -> #name (C123) + markdown = re.sub(r"<#([A-Z0-9_]+)\|([^<>]+)>", r"#\2 (\1)", markdown) markdown = re.sub(r"<#([A-Z0-9_]+)>", r"#\1", markdown) + # Inverted links (frequently hallucinated): -> + markdown = re.sub(r"<(?!https?://)([^<>|]+)\|(https?://[^|<>]+)>", r"<\2|\1>", markdown) # Links: -> [text](url) markdown = re.sub(r"<(https?://[^|<>]+)\|([^<>]+)>", r"[\2](\1)", markdown) # Bare links: -> url - markdown = re.sub(r"<(https?://[^<>]+)>", r"\1", markdown) + return re.sub(r"<(https?://[^<>]+)>", r"\1", markdown) + + +def _convert_mrkdwn_text(mrkdwn: str) -> str: + markdown = _convert_slack_tokens(_convert_special_mentions(mrkdwn)) # Bold: *text* -> **text** (Slack uses single * for bold) markdown = re.sub(r"(? ~~text~~ - markdown = re.sub(r"(?`` lies in between). + Remembering the stop keeps a run of unclosed ``<`` linear instead of + rescanning to the end of the line for each one; results are identical. + """ + + __slots__ = ("_fail_until", "_text") + + def __init__(self, text: str) -> None: + self._text = text + self._fail_until = -1 + + def end(self, index: int) -> int: + """Index just past the ``>`` closing the token at *index*, or ``-1``.""" + if index < self._fail_until: + return -1 + match = _ANGLE_TOKEN_STOP.search(self._text, index + 1) + if match is not None and match.group() == ">": + return match.end() + self._fail_until = match.start() if match is not None else len(self._text) + return -1 + + +def _convert_special_mentions(mrkdwn: str) -> str: + """```` -> ``@here``, ```` -> ``@eng``; code stays literal.""" + parts: list[str] = [] + start = 0 + cursor = 0 + length = len(mrkdwn) + angle = _AngleTokenScanner(mrkdwn) + + while cursor < length: + if mrkdwn.startswith(_CODE_FENCE, cursor): + cursor += len(_CODE_FENCE) + continue + char = mrkdwn[cursor] + if char == "`": + code_end = _find_inline_code_end(mrkdwn, cursor) + cursor = cursor + 1 if code_end == -1 else code_end + continue + if char != "<": + cursor += 1 + continue + end = angle.end(cursor) + if end == -1: + cursor += 1 + continue + token = mrkdwn[cursor:end] + token = _SPECIAL_MENTION_PATTERN.sub(r"@\1", token, count=1) + token = _LABELED_GROUP_PATTERN.sub(r"@\2", token, count=1) + token = _GROUP_PATTERN.sub(r"@\1", token, count=1) + parts.append(mrkdwn[start:cursor]) + parts.append(token) + start = end + cursor = end + + parts.append(mrkdwn[start:]) + return "".join(parts) + + +def _convert_mrkdwn_with_code_fences(mrkdwn: str) -> str: + """Rewrite each paired ```` ``` ```` fence onto its own lines. + + Slack treats text immediately after an opening fence as code, while + CommonMark treats it as the fence's info string. Everything Slack renders + literally -- unpaired fences, fences inside inline code or ``<...>`` + tokens, and fences on blockquote lines -- stays plain text. Fence content + skips the emphasis rewrites so code like ``*a`` survives verbatim. + + Mirrors :func:`chat_sdk.shared.code_fences.normalize_code_fences` with + mrkdwn's entity escaping (``>`` blockquotes, ``<...>`` control + tokens), as upstream does. Keep the two in sync. + """ + parts: list[str] = [] + # ``result.length`` / ``result.endsWith("\n")`` without re-joining ``parts``. + result_len = 0 + result_ends_with_newline = False + text_start = 0 + cursor = 0 + length = len(mrkdwn) + angle = _AngleTokenScanner(mrkdwn) + # Set when the closing fence splits a line: the text after it lands at the + # start of a new line, where CommonMark would promote a leading block + # marker Slack rendered inline. + moved_to_own_line = False + + def append(value: str) -> None: + nonlocal result_len, result_ends_with_newline + if value: + parts.append(value) + result_len += len(value) + result_ends_with_newline = value.endswith("\n") + + def flush_text_before(end: int) -> None: + nonlocal moved_to_own_line + text = mrkdwn[text_start:end] + if moved_to_own_line: + text = _escape_leading_block_marker(text) + moved_to_own_line = False + append(_convert_mrkdwn_text(text)) + + while cursor < length: + char = mrkdwn[cursor] + if char == "<": + token_end = angle.end(cursor) + cursor = cursor + 1 if token_end == -1 else token_end + continue + if char != "`": + cursor += 1 + continue + if not mrkdwn.startswith(_CODE_FENCE, cursor): + span_end = _find_inline_code_end(mrkdwn, cursor) + cursor = cursor + 1 if span_end == -1 else span_end + continue + + content_start = cursor + len(_CODE_FENCE) + content_end = mrkdwn.find(_CODE_FENCE, content_start) + if content_end == -1 or _is_on_blockquote_line(mrkdwn, cursor): + # Slack renders an unpaired or quoted ``` literally. + cursor = content_start + continue + + flush_text_before(cursor) + if result_len > 0 and not result_ends_with_newline: + append("\n") + content = mrkdwn[content_start:content_end] + append(_CODE_FENCE) + if not content.startswith("\n"): + append("\n") + append(_convert_slack_tokens(content)) + if not content.endswith("\n"): + append("\n") + append(_CODE_FENCE) + + cursor = content_end + len(_CODE_FENCE) + text_start = cursor + if cursor < length and mrkdwn[cursor] != "\n": + append("\n") + moved_to_own_line = True + + flush_text_before(length) + return "".join(parts) + + +def _find_inline_code_end(mrkdwn: str, index: int) -> int: + """End of the inline code span opening at *index*, or ``-1``. + + Slack inline code spans never cross line breaks. The newline search is + bounded by the close (same result as upstream's ``newline < close``) + so a long line of backticks stays linear. + """ + close = mrkdwn.find("`", index + 1) + if close == -1: + return -1 + if mrkdwn.find("\n", index + 1, close) != -1: + return -1 + return close + 1 + + +def _is_on_blockquote_line(mrkdwn: str, index: int) -> bool: + # Upstream ``slice(lineStart, index).trimStart().startsWith(">")``, + # matched in place so many fences on one long line copy nothing. + line_start = mrkdwn.rfind("\n", 0, index) + 1 + indent = _JS_LEADING_WHITESPACE.match(mrkdwn, line_start, index) + content_start = indent.end() if indent is not None else line_start + return mrkdwn.startswith(">", content_start, index) + + +def _escape_leading_block_marker(text: str) -> str: + match = _LEADING_WHITESPACE_PATTERN.match(text) + whitespace = match.group(0) if match else "" + # Collapse the leading separator so it cannot become an indented code + # block, then defuse any block marker now sitting at the line start. + prefix = " " if whitespace else "" + rest = text[len(whitespace) :] + if _BLOCK_MARKER_PATTERN.match(rest): + return f"{prefix}\\{rest}" + return prefix + _ORDERED_LIST_MARKER_PATTERN.sub(r"\1\\\2", rest, count=1) def markdown_bold_to_slack_mrkdwn(markdown: str) -> str: diff --git a/src/chat_sdk/adapters/slack/format_converter.py b/src/chat_sdk/adapters/slack/format_converter.py index 24cf812d..dfb7716c 100644 --- a/src/chat_sdk/adapters/slack/format_converter.py +++ b/src/chat_sdk/adapters/slack/format_converter.py @@ -13,9 +13,9 @@ from __future__ import annotations -import re from typing import Any +from chat_sdk.adapters.slack.format import slack_mrkdwn_to_markdown from chat_sdk.emoji import convert_emoji_placeholders from chat_sdk.shared.base_format_converter import ( BaseFormatConverter, @@ -55,30 +55,14 @@ def from_ast(self, ast: Root) -> str: return stringify_markdown(ast) def to_ast(self, platform_text: str) -> Root: - """Parse Slack mrkdwn into an AST. Used for incoming ``message`` events.""" - markdown = platform_text + """Parse Slack mrkdwn into an AST. Used for incoming ``message`` events. - # User mentions: <@U123|name> -> @name or <@U123> -> @U123 - markdown = re.sub(r"<@([A-Z0-9_]+)\|([^<>]+)>", r"@\2", markdown) - markdown = re.sub(r"<@([A-Z0-9_]+)>", r"@\1", markdown) - - # Channel mentions: <#C123|name> -> #name - markdown = re.sub(r"<#[A-Z0-9_]+\|([^<>]+)>", r"#\1", markdown) - markdown = re.sub(r"<#([A-Z0-9_]+)>", r"#\1", markdown) - - # Links: -> [text](url) - markdown = re.sub(r"<(https?://[^|<>]+)\|([^<>]+)>", r"[\2](\1)", markdown) - - # Bare links: -> url - markdown = re.sub(r"<(https?://[^<>]+)>", r"\1", markdown) - - # Bold: *text* -> **text** (Slack uses single * for bold) - markdown = re.sub(r"(? ~~text~~ - markdown = re.sub(r"(? str: return convert_emoji_placeholders(self._ast_to_mrkdwn(message.ast), "slack") return "" - # ------------------------------------------------------------------------- - # Overrides - # ------------------------------------------------------------------------- - - def extract_plain_text(self, platform_text: str) -> str: - """Extract plain text from Slack mrkdwn by stripping formatting.""" - text = platform_text - - # Remove user mentions formatting: <@U123|name> -> @name, <@U123> -> @U123 - text = re.sub(r"<@([A-Z0-9_]+)\|([^<>]+)>", r"@\2", text) - text = re.sub(r"<@([A-Z0-9_]+)>", r"@\1", text) - - # Remove channel mentions: <#C123|name> -> #name - text = re.sub(r"<#[A-Z0-9_]+\|([^<>]+)>", r"#\1", text) - text = re.sub(r"<#([A-Z0-9_]+)>", r"#\1", text) - - # Remove links formatting: -> text, -> url - text = re.sub(r"<(https?://[^|<>]+)\|([^<>]+)>", r"\2", text) - text = re.sub(r"<(https?://[^<>]+)>", r"\1", text) - - # Remove bold/italic/strikethrough markers - text = re.sub(r"\*([^*]+)\*", r"\1", text) - text = re.sub(r"_([^_]+)_", r"\1", text) - text = re.sub(r"~([^~]+)~", r"\1", text) - - return text - # ------------------------------------------------------------------------- # Private helpers # ------------------------------------------------------------------------- diff --git a/tests/test_channel_faithful.py b/tests/test_channel_faithful.py index 2a59a850..493b8f73 100644 --- a/tests/test_channel_faithful.py +++ b/tests/test_channel_faithful.py @@ -1638,9 +1638,10 @@ async def test_should_encode_callbackurl_when_editing_a_sent_card(self): # Python-specific divergence (docs/UPSTREAM_SYNC.md): an edited channel # card binds its tokens to the channel, not to the reported thread id. # Round trip with the real Slack id functions and the real block_actions - # click, both for the synthetic `slack:C…:` post id Python reports today - # and for the `slack:C…:` id upstream 92530dd3 reports (#283). A DM - # click reports no ts, so a thread scope would miss it either way. + # click, both for the `slack:C…:` id `post_channel_message` reports + # (upstream 92530dd3, #283) and for the synthetic `slack:C…:` id it keeps + # when the response has no string ts. A DM click reports no ts, so a + # thread scope would miss it either way. @pytest.mark.asyncio @pytest.mark.parametrize( ("channel_id", "post_thread_id"), diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index bd2c815f..3908a2dc 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -10,6 +10,7 @@ from __future__ import annotations from chat_sdk.adapters.slack.format_converter import SlackFormatConverter +from chat_sdk.shared.markdown_parser import ast_to_plain_text # --------------------------------------------------------------------------- # toMarkdown (mrkdwn -> markdown) @@ -20,6 +21,64 @@ class TestToMarkdown: def setup_method(self): self.converter = SlackFormatConverter() + # -- incoming code fences (vercel/chat#843) ----------------------------- + + def test_preserves_code_starting_immediately_after_the_opening_fence(self): + ast = self.converter.to_ast("```first line\nsecond line\n```") + code = ast["children"][0] + assert code["type"] == "code" + assert code.get("lang") is None + assert code.get("meta") is None + assert code["value"] == "first line\nsecond line" + assert ast_to_plain_text(ast) == "first line\nsecond line" + + def test_parses_a_code_block_pasted_into_a_message(self): + ast = self.converter.to_ast("Here you go:\n```first line\nsecond line```") + assert ast["children"][0]["type"] == "paragraph" + code = ast["children"][1] + assert code["type"] == "code" + assert code.get("lang") is None + assert code["value"] == "first line\nsecond line" + + def test_does_not_swallow_text_after_an_unpaired_fence(self): + ast = self.converter.to_ast("use ``` to fence code, *see*?") + assert [node["type"] for node in ast["children"]] == ["paragraph"] + assert ast_to_plain_text(ast) == "use ``` to fence code, see?" + + def test_keeps_a_quoted_fence_inside_the_blockquote(self): + ast = self.converter.to_ast("> a ```c``` b") + assert [node["type"] for node in ast["children"]] == ["blockquote"] + quote = ast["children"][0] + assert [node["type"] for node in quote["children"]] == ["paragraph"] + # Upstream also asserts toPlainText == "a c b": remark reads ```c``` as + # a triple-backtick code span. The shared Python parser only knows + # single-backtick spans (known parser limitation, not Slack-specific), + # so assert what the Slack layer controls: the quoted fence stays + # inline text inside the blockquote rather than becoming a code block. + assert ast_to_plain_text(ast).replace("`", "") == "a c b" + + def test_keeps_trailing_text_after_a_code_block_as_a_paragraph(self): + ast = self.converter.to_ast("```x``` > note") + assert ast["children"][0]["type"] == "code" + assert ast["children"][0]["value"] == "x" + assert ast["children"][1]["type"] == "paragraph" + # Upstream: toPlainText == "x\n\n> note". CommonMark strips the + # paragraph's leading space; the shared Python parser keeps it (known + # parser limitation), so compare without it. The escaped ``\>`` must + # still read as text, not a blockquote. + assert ast_to_plain_text(ast).replace("\n\n ", "\n\n") == "x\n\n> note" + + def test_keeps_code_content_verbatim_inside_the_fence(self): + ast = self.converter.to_ast("```int *a = *b;```") + assert ast["children"][0]["type"] == "code" + assert ast["children"][0]["value"] == "int *a = *b;" + + def test_to_ast_unescapes_slack_entities(self): + """Python parity fix: the old inline regex copy in ``to_ast`` skipped + ``unescape_slack_text``, so ``<`` reached handlers verbatim.""" + ast = self.converter.to_ast("a <b> & c") + assert ast_to_plain_text(ast) == "a & c" + def test_converts_bold(self): result = self.converter.to_markdown("Hello *world*!") assert "**world**" in result @@ -397,7 +456,9 @@ def test_extracts_bare_url(self): assert self.converter.extract_plain_text("Visit ") == "Visit https://example.com" def test_extracts_channel_mention_with_name(self): - assert self.converter.extract_plain_text("Join <#C123|general>") == "Join #general" + # Labeled channel tokens keep the id (vercel/chat#756); extract_plain_text + # is the base to_ast + ast_to_plain_text (upstream has no Slack override). + assert self.converter.extract_plain_text("Join <#C123|general>") == "Join #general (C123)" def test_extracts_bare_channel_mention(self): assert self.converter.extract_plain_text("Join <#C123>") == "Join #C123" diff --git a/tests/test_slack_format_primitives.py b/tests/test_slack_format_primitives.py index 24dcfff7..8669b66a 100644 --- a/tests/test_slack_format_primitives.py +++ b/tests/test_slack_format_primitives.py @@ -113,12 +113,120 @@ def test_normalizes_slack_mrkdwn_to_markdown(self): slack_mrkdwn_to_markdown( "Hey <@U123|jane> in <#C123|general>, see and *bold* ~done~" ) - == "Hey @jane in #general, see [this](https://example.com) and **bold** ~~done~~" + == "Hey @jane in #general (C123), see [this](https://example.com) and **bold** ~~done~~" ) + # -- special mentions (vercel/chat#960) -------------------------------- + + def test_normalizes_special_mentions_and_user_groups(self): + assert ( + slack_mrkdwn_to_markdown(" ") + == "@here @channel @everyone @devs @S456" + ) + + @pytest.mark.parametrize( + "token", + ["", "", "", "", ""], + ) + def test_preserves_inside_code_while_converting_surrounding_mentions(self, token: str): + assert slack_mrkdwn_to_markdown(f" `{token}` ") == f"@here `{token}` @channel" + assert slack_mrkdwn_to_markdown(f" ```{token}``` ") == f"@here \n```\n{token}\n```\n @channel" + + def test_converts_special_mentions_around_multiple_inline_code_spans(self): + assert ( + slack_mrkdwn_to_markdown("`` `` ") + == "`` @channel `` @devs" + ) + + def test_does_not_treat_an_unmatched_backtick_as_a_code_span(self): + assert slack_mrkdwn_to_markdown("use ` then ") == "use ` then @here" + assert slack_mrkdwn_to_markdown("`first\n `last") == "`first\n@here `last" + + def test_preserves_escaped_special_mentions(self): + assert slack_mrkdwn_to_markdown("<!here> ") == " @channel" + + def test_keeps_link_label_backticks_from_hiding_special_mentions(self): + assert ( + slack_mrkdwn_to_markdown(" `open") + == "[`label](https://example.com) @here `open" + ) + + def test_preserves_emphasis_across_inline_code(self): + assert slack_mrkdwn_to_markdown("*before `` after* ") == "**before `` after** @channel" + + # -- code fences (vercel/chat#843) ------------------------------------- + + def test_normalizes_slack_code_fences_for_commonmark_parsing(self): + assert slack_mrkdwn_to_markdown("```first line\nsecond line\n```") == "```\nfirst line\nsecond line\n```" + + def test_puts_slack_code_fences_on_separate_lines_from_surrounding_text(self): + assert slack_mrkdwn_to_markdown("before ```code``` after") == "before \n```\ncode\n```\n after" + + def test_keeps_an_unpaired_as_literal_text(self): + # TS: "keeps an unpaired ``` as literal text" + assert slack_mrkdwn_to_markdown("use ``` to fence code, *see*?") == "use ``` to fence code, **see**?" + + def test_keeps_a_inside_an_inline_code_span_as_literal_text(self): + # TS: "keeps a ``` inside an inline code span as literal text" + assert slack_mrkdwn_to_markdown("`use ``` here`") == "`use ``` here`" + + def test_keeps_a_on_a_blockquote_line_as_literal_text(self): + # TS: "keeps a ``` on a blockquote line as literal text" + assert slack_mrkdwn_to_markdown("> a ```c``` b") == "> a ```c``` b" + + def test_keeps_a_inside_a_link_token_as_part_of_the_label(self): + # TS: "keeps a ``` inside a link token as part of the label" + assert slack_mrkdwn_to_markdown("") == "[```code```](https://x.com)" + + def test_does_not_rewrite_emphasis_inside_fenced_code(self): + assert slack_mrkdwn_to_markdown("```int *a = *b;```") == "```\nint *a = *b;\n```" + assert slack_mrkdwn_to_markdown("```keep ~x~ raw```") == "```\nkeep ~x~ raw\n```" + + def test_still_resolves_mention_tokens_inside_fenced_code(self): + assert slack_mrkdwn_to_markdown("```ping <@U123|jane>```") == "```\nping @jane\n```" + + def test_escapes_trailing_text_that_would_become_a_block_construct(self): + assert slack_mrkdwn_to_markdown("```x``` > note") == "```\nx\n```\n \\> note" + assert slack_mrkdwn_to_markdown("```x``` # heading") == "```\nx\n```\n \\# heading" + assert slack_mrkdwn_to_markdown("```x``` - item") == "```\nx\n```\n \\- item" + assert slack_mrkdwn_to_markdown("```x``` 1. item") == "```\nx\n```\n 1\\. item" + + def test_collapses_trailing_indentation_that_would_become_indented_code(self): + assert slack_mrkdwn_to_markdown("see ```x```\tresult is 5") == "see \n```\nx\n```\n result is 5" + assert slack_mrkdwn_to_markdown("see ```x``` result is 5") == "see \n```\nx\n```\n result is 5" + + # -- channel ids and inverted links (vercel/chat#756) ------------------ + + def test_preserves_the_channel_id_for_labeled_channel_tokens(self): + assert slack_mrkdwn_to_markdown("Post in <#C042BLND6R6|general>") == "Post in #general (C042BLND6R6)" + assert slack_mrkdwn_to_markdown("Post in <#C042BLND6R6>") == "Post in #C042BLND6R6" + def test_normalizes_bare_slack_links_to_markdown_urls(self): assert slack_mrkdwn_to_markdown("See ") == "See https://example.com" + def test_normalizes_inverted_slack_link_tokens_before_markdown_conversion(self): + assert ( + slack_mrkdwn_to_markdown("See and ") + == "See [docs](https://example.com) and [A](https://a.com)" + ) + + def test_does_not_invert_links_whose_display_label_is_itself_a_url(self): + assert slack_mrkdwn_to_markdown("See ") == "See [https://b.com](https://a.com)" + + # -- Python-specific: adversarial inputs stay linear -------------------- + + def test_adversarial_inputs_return_the_expected_output(self): + """Unclosed ``<`` runs, unbalanced backticks and many quoted fences on + one line must convert (the scanners memoize failed ``<`` scans and + bound backtick/newline searches) -- asserted on output, not time.""" + n = 50_000 + assert slack_mrkdwn_to_markdown("<" * n) == "<" * n + assert slack_mrkdwn_to_markdown("`x" * 25_000 + " ") == "`x" * 25_000 + " @here" + assert slack_mrkdwn_to_markdown("` \n" * 5_000) == "` @here\n" * 5_000 + quoted = "> " + "```" * 10_000 + assert slack_mrkdwn_to_markdown(quoted) == "> " + "```" * 10_000 + assert slack_mrkdwn_to_markdown(" July 11 #ff0000" message = _parse(text="", blocks=[_table([cell])]) - # Upstream expects ``@S789``: rendering ```` belongs to the - # mrkdwn converter (``convertSpecialMentions``), ported by #283. - assert message.text == "#C789 July 11 #ff0000" + assert message.text == "#C789 @S789 July 11 #ff0000" def test_joins_rich_text_list_items_in_a_cell_with_newlines(self): cell = { @@ -380,10 +378,7 @@ def test_keeps_attachment_content_out_of_an_unclosed_code_fence_in_the_body(self # The unclosed fence swallows the rest of the body, but the # attachment parses in isolation and stays a paragraph. assert [child["type"] for child in message.formatted["children"]] == ["paragraph", "code", "paragraph"] - # Upstream expects "Deploy failed:\n\nTypeError: boom\n\n…": until #283 - # the body's plain text keeps the pre-#210 regex rendering (see - # ``SlackAdapter._assemble_content``), so the fence stays in ``text``. - assert message.text == "Deploy failed:\n```\nTypeError: boom\n\nDeploy status\nEnvironment: production" + assert message.text == "Deploy failed:\n\nTypeError: boom\n\nDeploy status\nEnvironment: production" def test_uses_the_fallback_when_attachment_blocks_carry_nothing_renderable(self): message = _parse( @@ -480,11 +475,9 @@ async def test_resolves_user_and_channel_mentions_in_table_cells(self): "blocks": [_table(cells)], } message = await adapter._parse_slack_message(event, THREAD_ID) - # The channel cell is a Python addition. Upstream's converter renders a - # labelled channel as ``#general (C789)`` (adapter-slack format/index.ts - # ``convertSlackTokens``); our ``to_ast`` gains that rewrite with #283, - # which updates this to "@Test Bot\t#general (C789)". - assert message.text == "@Test Bot\t#general" + # The channel cell is a Python addition; the converter renders a + # labelled channel as ``#general (C789)`` (upstream ``convertSlackTokens``). + assert message.text == "@Test Bot\t#general (C789)" client.users_info.assert_awaited_once_with(user="U_BOT") async def test_resolves_mentions_in_trailing_and_attachment_tables_in_one_wave(self): @@ -569,21 +562,20 @@ async def test_sync_and_async_paths_give_equal_content_without_mentions(self): assert parsed.text == "lead\n\nBody bold\n\ntail\n\npre\n\nline one\n\nline two\n\nx\t2" @pytest.mark.parametrize( - "text", + ("text", "expected"), [ - "```npm test```", - "run ```npm test``` please", - "can you fix this?\n```def foo():\n return 1```", - "- item\n> quoted `code`", + ("```npm test```", "npm test"), + ("can you fix this?\n```def foo():\n return 1```", "can you fix this?\n\ndef foo():\n return 1"), + ("- item\n> quoted `code`", "item\n\nquoted code"), + (" ", ""), + ("hi\n", "hi"), ], ) - async def test_body_text_keeps_its_pre_283_rendering(self, text: str): - """Until #283 ports the fence normalization, the body's plain text keeps - ``extract_plain_text``; deriving it from ``to_ast`` would drop code on - a fence's opening line. Tables and attachments still append.""" + async def test_body_text_is_the_plain_text_of_formatted(self, text: str, expected: str): + """``message.text`` is ``toPlainText(formatted)`` on both parse paths: + a fence keeps its first code line, and list markers, ``>`` and + backticks drop. Tables append as further root children.""" adapter = _adapter() - expected = adapter._format_converter.extract_plain_text(text) - assert expected == text event = { "type": "message", "user": "U123", @@ -596,21 +588,7 @@ async def test_body_text_keeps_its_pre_283_rendering(self, text: str): parsed = await adapter._parse_slack_message(event, THREAD_ID) assert sync.text == parsed.text == expected with_table = adapter.parse_message({**event, "blocks": [{"type": "section"}, _table([_raw("cell")])]}) - assert with_table.text == f"{expected}\n\ncell" - - @pytest.mark.parametrize( - ("text", "blocks", "expected"), - [ - (" ", [], " "), - ("hi\n", [], "hi\n"), - (" ", [{"type": "section"}, _table([_raw("cell")])], "cell"), - ("hi\n", [{"type": "section"}, _table([_raw("cell")])], "hi\n\ncell"), - ], - ) - def test_body_whitespace_joins_like_root_children(self, text: str, blocks: list[Any], expected: str): - """Alone, the body keeps its pre-#210 text; beside other content its - trailing whitespace goes, as ``toPlainText`` joins parsed blocks.""" - assert _parse(text=text, blocks=blocks).text == expected + assert with_table.text == "\n\n".join(piece for piece in (expected, "cell") if piece) def test_does_not_surface_the_title_link_of_an_unfurl(self): message = _parse( diff --git a/tests/test_slack_inbound_mentions.py b/tests/test_slack_inbound_mentions.py index a2fab529..e6f53036 100644 --- a/tests/test_slack_inbound_mentions.py +++ b/tests/test_slack_inbound_mentions.py @@ -185,6 +185,72 @@ def test_matches_a_structured_user_element_regardless_of_bot_id_case(self): ) assert message.is_mention is True + def test_converts_special_mentions_to_readable_text(self): + message = self._adapter().parse_message( + { + "type": "message", + "user": "U123", + "channel": "C456", + "text": " and ", + "ts": "1234567890.123456", + } + ) + assert message.text == "@here and @devs" + assert [node["type"] for node in message.formatted["children"]] == ["paragraph"] + + def test_preserves_special_mention_tokens_in_an_inbound_inline_code_span(self): + text = " " + message = self._adapter().parse_message( + { + "type": "message", + "user": "U123", + "channel": "D456", + "channel_type": "im", + "text": f"review code `{text}`", + "ts": "1234567890.123456", + "blocks": _rich_text( + {"type": "text", "text": "review code "}, + {"type": "text", "text": text, "style": {"code": True}}, + ), + } + ) + assert message.text == f"review code {text}" + paragraph = message.formatted["children"] + assert [node["type"] for node in paragraph] == ["paragraph"] + assert [(c["type"], c["value"]) for c in paragraph[0]["children"]] == [ + ("text", "review code "), + ("inlineCode", text), + ] + assert message.is_mention is False + + def test_preserves_special_mention_tokens_in_an_inbound_code_block(self): + text = "review fence " + message = self._adapter().parse_message( + { + "type": "message", + "user": "U123", + "channel": "D456", + "channel_type": "im", + "text": f"```{text}```", + "ts": "1234567890.123456", + "blocks": [ + { + "type": "rich_text", + "elements": [ + { + "type": "rich_text_preformatted", + "elements": [{"type": "text", "text": text}], + "border": 0, + } + ], + } + ], + } + ) + assert message.text == text + assert [(n["type"], n["value"]) for n in message.formatted["children"]] == [("code", text)] + assert message.is_mention is False + def test_uses_the_bot_user_id_instead_of_the_app_bot_id(self): message = self._adapter().parse_message( { @@ -1077,3 +1143,29 @@ async def test_serves_email_from_the_user_cache_without_a_second_users_info_call second = await adapter._parse_slack_message(dict(self.HUMAN_EVENT), "slack:C123:1234567890.123456") assert client.users_info.await_count == 1 assert second.author.email == "alice@example.com" + + +# --------------------------------------------------------------------------- +# post_channel_message +# --------------------------------------------------------------------------- + + +class TestPostChannelMessage: + async def test_posts_to_channel_without_thread_context(self): + client = _Client() + adapter = _make_adapter(client) + result = await adapter.post_channel_message("slack:C123", "Top-level message") + assert result.id == "2222222222.000000" + assert result.thread_id == "slack:C123:2222222222.000000" + kwargs = client.chat_postMessage.call_args.kwargs + assert kwargs["channel"] == "C123" + assert kwargs.get("thread_ts") is None + + async def test_keeps_the_synthetic_thread_id_when_the_response_has_no_string_ts(self): + """Python addition: without a string ``ts`` there is no thread to + address, so the synthetic ``slack:C123:`` id is kept.""" + client = _Client() + client.chat_postMessage = AsyncMock(return_value={"ok": True, "ts": 2222222222}) + adapter = _make_adapter(client) + result = await adapter.post_channel_message("slack:C123", "Top-level message") + assert result.thread_id == "slack:C123:" diff --git a/tests/test_slack_socket_mode.py b/tests/test_slack_socket_mode.py index 89d10a46..78842818 100644 --- a/tests/test_slack_socket_mode.py +++ b/tests/test_slack_socket_mode.py @@ -517,24 +517,47 @@ async def fake_modal_submit(*args: Any, **kwargs: Any) -> ModalResponse: assert isinstance(body_arg, dict) assert body_arg.get("response_action") == "errors" - async def test_retry_attempt_is_skipped_but_acked(self): - adapter = _make_socket_adapter() + async def test_processes_retries_like_first_deliveries_dedupe_drops_true_duplicates(self): + """Port of upstream "processes retries like first deliveries (dedupe + drops true duplicates)" (vercel/chat#667). + + A retry may be the ONLY delivery the app ever sees (the original was + sent while no socket was connected during a restart), so it is routed + normally; ``Chat.process_message`` dedupes by message id when the + original was handled. + """ + logger = MagicMock() + adapter = _make_socket_adapter(logger=logger) chat = _make_mock_chat() adapter._chat = chat request = MagicMock() request.envelope_id = "env-1" request.type = "events_api" - request.payload = {"event": {"type": "message"}} - request.retry_attempt = 2 # Slack retry — should be skipped. + request.payload = { + "event": { + "type": "message", + "channel": "C123", + "ts": "1234567890.123456", + "text": "retried", + "user": "U_USER", + } + } + request.retry_attempt = 1 + request.retry_reason = "timeout" client = MagicMock() client.send_socket_mode_response = AsyncMock() await adapter._on_socket_request(client, request) + await asyncio.sleep(0) - # Ack went out (so Slack stops resending), but no dispatch. assert client.send_socket_mode_response.await_count == 1 - assert chat.process_message.called is False + chat.process_message.assert_called_once() + assert chat.process_message.call_args.args[1] == "slack:C123:1234567890.123456" + logger.info.assert_any_call( + "Processing socket mode retry", + {"retry_attempt": 1, "retry_reason": "timeout", "type": "events_api"}, + ) async def test_unknown_event_type_acks_and_does_nothing(self): adapter = _make_socket_adapter() From 44c2489b0565bc00748e01769079f37ed45bec19 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 03:26:31 -0700 Subject: [PATCH 02/10] fix(markdown): no backtick fence opener when the info string has a backtick (#283) Astra round 1: a Slack quote starting with an inline fence (> ```npm test```) parsed to an empty code block, dropping the command from message.text. CommonMark (remark upstream) forbids backticks in a backtick fence's info string. Also document the upstream-parity choices for unpaired fences and non-events_api socket retries. --- CHANGELOG.md | 1 + docs/UPSTREAM_SYNC.md | 8 ++++++++ src/chat_sdk/adapters/slack/adapter.py | 7 ++++++- src/chat_sdk/adapters/slack/format.py | 7 ++++++- src/chat_sdk/shared/markdown_parser.py | 4 +++- tests/test_slack_format.py | 11 +++++++++++ 6 files changed, 35 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca6ce08..2b361858 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **Breaking/consumer-visible (code blocks keep their first line):** Slack treats the text right after an opening ```` ``` ```` as code; it was read as an info string and dropped, so `` ```npm test``` `` gave an empty `formatted` (and `text` kept the backticks). Each paired fence now parses as a `code` node and `text` holds the code (`"npm test"`). Unpaired fences, fences in inline code or `<…>` tokens, and fences on `>` quote lines stay literal, as in Slack. - **Breaking/consumer-visible (`channel.post()` ids):** `post_channel_message` returns `slack:C123:` (a replyable thread rooted at the new message) instead of `slack:C123:` when Slack returns a string message `ts`, so `chat.thread(sent.thread_id).post(...)` replies in its thread. File-only posts keep `slack:C123:`. - **Breaking/consumer-visible (Socket Mode retries are processed):** envelopes with `retry_attempt > 0` were acked and dropped; they are now routed like first deliveries and logged at info (`"Processing socket mode retry"` with `retry_attempt`, `retry_reason`, `type`). The event-id marker (#268) and core message-id dedupe (10 min TTL, #191) drop true duplicates, so an event missed during a restart or reconnect is no longer lost. + - Shared `parse_markdown` follows CommonMark's fence rule: a line like ```` ```npm test``` ```` (a backtick in the info string) is no longer a code-block opener, so a Slack quote starting with an inline fence keeps its text instead of becoming an empty code block. This affects every adapter that parses Markdown, only for such lines. - The mrkdwn scanners stay linear on untrusted text (results identical to upstream). Known shared-parser gaps (no multi-backtick code spans, a paragraph's leading space kept, quadratic time on some inputs) predate this change; see `docs/UPSTREAM_SYNC.md`. - **Slack Agent Sessions lifecycle and native stop** (#215; Slack half of vercel/chat `2ce2be00` #862, chat@4.39.0, plus `8b6d7f3a` #882 and `2cc8cc3f` #897, chat@4.40.0). Only with `agent_view=True`; without it nothing changes (no `agents.sessions.*` call, byte-identical stream stop payloads). A live check in an `agent_view` workspace is still pending. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 9f83c741..9cb0860e 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1289,6 +1289,14 @@ chat@4.41.1). also quadratic on some inputs (about 2.4 s for 20k characters of `` `x ``); this predates the port, since `to_ast` always ran it on every inbound message, and is tracked separately. +- **Shared parser fix (CommonMark fence rule).** A backtick fence's info + string may not contain a backtick, so `parse_markdown` no longer opens a + code block on a line like ```` ```npm test``` ````. The Slack normalizer + leaves a quoted fence literal (as upstream), and without this rule + `> ```npm test```` parsed to an empty code block and dropped the command + from `message.text`. It now stays text in the quote (backticks kept, the + multi-backtick gap above). A line-leading *unpaired* ```` ```npm test ```` + still opens an empty code block, as in upstream (remark does the same). ### Teams Adaptive Card 1.5 rendering (chat@4.36–4.41, #220) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 7cc7cf91..fd8769f7 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -3709,7 +3709,12 @@ async def ack(response_payload: dict[str, Any] | None = None) -> None: # dedupe drop an already-handled duplicate, while a genuinely # missed event is recovered instead of being lost (upstream # vercel/chat#667). slack_sdk names upstream's ``retry_num`` - # ``retry_attempt``. + # ``retry_attempt``. Upstream-parity choice: like upstream + # ``startSocketMode`` / ``routeSocketEvent`` (adapter-slack + # index.ts:3038-3058, 3087-3135), only ``events_api`` consults the + # event-id marker; ``interactive`` / ``slash_commands`` envelopes + # are routed as-is (Slack's retry_attempt/retry_reason describe + # Events API redelivery). self._logger.info( "Processing socket mode retry", { diff --git a/src/chat_sdk/adapters/slack/format.py b/src/chat_sdk/adapters/slack/format.py index 2d191d02..9799ae8c 100644 --- a/src/chat_sdk/adapters/slack/format.py +++ b/src/chat_sdk/adapters/slack/format.py @@ -305,7 +305,12 @@ def flush_text_before(end: int) -> None: content_start = cursor + len(_CODE_FENCE) content_end = mrkdwn.find(_CODE_FENCE, content_start) if content_end == -1 or _is_on_blockquote_line(mrkdwn, cursor): - # Slack renders an unpaired or quoted ``` literally. + # Slack renders an unpaired or quoted ``` literally. Upstream-parity + # choice: the fence is left as is (adapter-slack format/index.ts + # convertMrkdwnWithCodeFences), so a line-leading unpaired + # "```npm test" still opens an empty CommonMark code block there + # too. A quoted "```c```" is no fence opener (its info string has a + # backtick); ``parse_markdown`` follows that rule. cursor = content_start continue diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index 5a3c9c51..96e926e3 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -527,7 +527,9 @@ def _parse_inline(text: str, *, _already_protected: bool = False) -> list[Conten # Patterns used by the block parser _HEADING_RE = re.compile(r"^(#{1,6})\s+(.*)") _THEMATIC_BREAK_RE = re.compile(r"^([-*_]\s*){3,}\s*$") -_FENCED_CODE_START_RE = re.compile(r"^(`{3,}|~{3,})(.*)") +# CommonMark: a backtick fence's info string may not contain a backtick, so +# "```npm test```" on one line is a (code span) paragraph, not a fence. +_FENCED_CODE_START_RE = re.compile(r"^(`{3,}(?=[^`]*$)|~{3,})(.*)") _BLOCKQUOTE_RE = re.compile(r"^>\s?(.*)") _ORDERED_LIST_RE = re.compile(r"^(\d+)[.)]\s+(.*)") _UNORDERED_LIST_RE = re.compile(r"^[-*+]\s+(.*)") diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index 3908a2dc..577fd090 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -57,6 +57,17 @@ def test_keeps_a_quoted_fence_inside_the_blockquote(self): # inline text inside the blockquote rather than becoming a code block. assert ast_to_plain_text(ast).replace("`", "") == "a c b" + def test_keeps_a_line_leading_quoted_fence_as_text(self): + """Python parser fix: CommonMark forbids a backtick in a backtick + fence's info string, so a quote that starts with ```npm test``` is a + paragraph (remark: a code span), not an empty code block that drops + the command from ``message.text``.""" + ast = self.converter.to_ast("> ```npm test```") + assert [node["type"] for node in ast["children"]] == ["blockquote"] + assert [node["type"] for node in ast["children"][0]["children"]] == ["paragraph"] + # Upstream: "npm test" (multi-backtick code span, a shared-parser gap). + assert ast_to_plain_text(ast).replace("`", "") == "npm test" + def test_keeps_trailing_text_after_a_code_block_as_a_paragraph(self): ast = self.converter.to_ast("```x``` > note") assert ast["children"][0]["type"] == "code" From b76aaaf44cc36d9790c3465622cf8eb4894c0bc5 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 03:30:16 -0700 Subject: [PATCH 03/10] docs(slack): link shared-parser gaps to #308 (#283) --- docs/UPSTREAM_SYNC.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 9cb0860e..f426f441 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1288,7 +1288,7 @@ chat@4.41.1). gives `"x\n\n > note"`, upstream `"x\n\n> note"`). `parse_markdown` is also quadratic on some inputs (about 2.4 s for 20k characters of `` `x ``); this predates the port, since `to_ast` always ran it on every inbound - message, and is tracked separately. + message, and is tracked in #308 (with the two gaps above). - **Shared parser fix (CommonMark fence rule).** A backtick fence's info string may not contain a backtick, so `parse_markdown` no longer opens a code block on a line like ```` ```npm test``` ````. The Slack normalizer From f5f49f12cf673d3067177c018a45df70bacfc2fb Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 03:45:43 -0700 Subject: [PATCH 04/10] fix(slack): linear blockquote-line check; cap parse_markdown blockquote depth (#283) Astra round 3: _is_on_blockquote_line rescanned the line per fence (quadratic on long indented quote lines); it now remembers the current line. Unescaped > runs reached the recursive blockquote parser and raised RecursionError; past 768 levels the rest stays literal text (divergence documented in docs/UPSTREAM_SYNC.md). --- CHANGELOG.md | 1 + docs/UPSTREAM_SYNC.md | 17 +++++++++-- src/chat_sdk/adapters/slack/format.py | 41 +++++++++++++++++++++----- src/chat_sdk/shared/markdown_parser.py | 19 +++++++++++- tests/test_slack_format.py | 7 +++++ tests/test_slack_format_primitives.py | 2 ++ 6 files changed, 75 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad555673..cefca7d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **Breaking/consumer-visible (`channel.post()` ids):** `post_channel_message` returns `slack:C123:` (a replyable thread rooted at the new message) instead of `slack:C123:` when Slack returns a string message `ts`, so `chat.thread(sent.thread_id).post(...)` replies in its thread. File-only posts keep `slack:C123:`. - **Breaking/consumer-visible (Socket Mode retries are processed):** envelopes with `retry_attempt > 0` were acked and dropped; they are now routed like first deliveries and logged at info (`"Processing socket mode retry"` with `retry_attempt`, `retry_reason`, `type`). The event-id marker (#268) and core message-id dedupe (10 min TTL, #191) drop true duplicates, so an event missed during a restart or reconnect is no longer lost. - Shared `parse_markdown` follows CommonMark's fence rule: a line like ```` ```npm test``` ```` (a backtick in the info string) is no longer a code-block opener, so a Slack quote starting with an inline fence keeps its text instead of becoming an empty code block. This affects every adapter that parses Markdown, only for such lines. + - **Python-specific (divergence from upstream):** `parse_markdown` nests at most 768 blockquotes; deeper `>` markers stay literal text. Slack `>` now unescapes, and 1,100 of them raised `RecursionError` and dropped the message. See `docs/UPSTREAM_SYNC.md`. - The mrkdwn scanners stay linear on untrusted text (results identical to upstream). Known shared-parser gaps (no multi-backtick code spans, a paragraph's leading space kept, quadratic time on some inputs) predate this change; see `docs/UPSTREAM_SYNC.md`. - **Telegram: multiple files or attachments go out as one media group** (#278; ports vercel/chat `8d7ccdb1` #605, chat@4.34.0). - **Consumer-visible (Telegram):** `post_message` / `thread.post` with 2–10 `files` or 2–10 `attachments` now sends one `sendMediaGroup` album instead of raising `ValidationError("Telegram adapter supports a single file/attachment upload per message")`. Every sent message is cached and the last one is returned. The caption (with `parse_mode`) goes on the first item only, with the same MarkdownV2 → plain-text retry as single uploads. `thread.reply` threads the album to its target (`reply_parameters`). One file or attachment still uses `sendDocument` / `sendPhoto` / `sendVideo` / `sendAudio`. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 6c8eb8b5..e4c3b2f6 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1243,7 +1243,8 @@ rules were #193 and the `previous_message` hunk of #846 is #211. ### Slack inbound mrkdwn, channel-post ids, Socket Mode retries (chat@4.33–4.41, #283) -Part (b) of #209. Parity, with no new divergence. Ports the Slack halves of +Part (b) of #209. Parity apart from one divergence (the blockquote depth cap, +below). Ports the Slack halves of `0b63791b` (#667, chat@4.33.0), `92530dd3` (#720, chat@4.35.0), `c3118279` (#756, chat@4.37.0), `e71bfead` (#843, chat@4.39.0) and `44423bdc` (#960, chat@4.41.1). @@ -1279,8 +1280,11 @@ chat@4.41.1). - **Linear-time scanning (implementation, not divergence).** Results equal upstream's. A failed `<…>` scan is remembered so a run of unclosed `<` stays linear, the inline-code newline check is bounded by the closing backtick, - and the blockquote-line check matches in place instead of slicing. - `tests/test_slack_format_primitives.py` covers 50k-character inputs. + and the blockquote-line check (`_BlockquoteLines`) remembers the current + line, so many fences on one long indented quote line stay linear (upstream + re-slices the line per fence). `tests/test_slack_format_primitives.py` + covers 50k-character inputs, and a differential fuzz against upstream + `format/index.ts` (40k random mrkdwn inputs) found no mismatch. - **Known gaps (shared parser, not Slack-specific).** Two `markdown.test.ts` cases are adapted: `parse_markdown` has no multi-backtick code spans (```` ```c``` ```` inside a quote stays backticked text, where remark reads a @@ -1297,6 +1301,12 @@ chat@4.41.1). from `message.text`. It now stays text in the quote (backticks kept, the multi-backtick gap above). A line-leading *unpaired* ```` ```npm test ```` still opens an empty code block, as in upstream (remark does the same). +- **Blockquote depth cap (divergence, see the non-parity table).** `>` now + unescapes, so `">" * 1100` reaches `parse_markdown`'s recursive + blockquote parsing and raised `RecursionError`, failing the message (and a + whole `fetch_messages` page). Past 768 nested levels the rest of the quote + stays literal text. Nested lists recurse the same way and predate this + change (#308). ### Teams Adaptive Card 1.5 rendering (chat@4.36–4.41, #220) @@ -3217,6 +3227,7 @@ stay explicit instead of being rediscovered in code review. | Area | Python behavior | TS behavior | Rationale | |------|----------------|-------------|-----------| | JSX Card/Modal elements | Not supported; tests skipped | `Card()` returns JSX element | Python has no JSX runtime | +| Blockquote nesting cap (4.41 wave, #283) | `parse_markdown` nests at most 768 blockquotes; deeper `>` markers stay literal text in the innermost quote | remark nests without limit (`"> " * 1100 + "x"` gives 1,100 blockquotes) | Blockquotes parse recursively and Python's recursion limit (1,000) turned untrusted input such as Slack `">" * 1100` into a `RecursionError` that dropped the message. Only absurdly deep quotes differ. Regression test: `tests/test_slack_format.py::TestToMarkdown::test_deeply_nested_quotes_do_not_exhaust_the_stack`. | | Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` binds to `{threadId, "thread"}`, where `threadId` is the id the adapter reported for the post (or the channel id) | The reported thread id often never equals a click's thread id: Teams and Google Chat report the channel id (upstream as well; Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), Python's Slack reported the synthetic `slack:C…:` until #283 (it still does when the response has no string ts) while clicks carry the message ts, a Slack DM click carries no ts even though the post now reports one, and a chained edit dropped the override until #195 ported `16ea171e`. Upstream's edited buttons never POST in those cases. Every click on the message derives the channel id, and the channel scope is no broader than the original post's. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_slack_channel_card_resolves_for_the_real_click` (real Slack id functions and `_handle_block_actions`, channel and DM, before and after #283), `::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. To be filed as an upstream issue against vercel/chat (Teams and Google Chat edited channel cards never POST); upstream Slack is unaffected, since its `postChannelMessage` and DM clicks both carry the message ts. | | Callback-token lease fence (4.41 wave, #194) | After deleting a matched record, `resolve_callback_url` calls `extend_lock(lock, CALLBACK_LOCK_TTL_MS)`. If that fails, the 10 s lease lapsed mid-consume, and the call returns `None` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. To be filed as an upstream issue against vercel/chat (a stalled `get`/`delete` past the 10 s lease lets a second click double-POST). Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | | Link-preview fence slicing (4.41 wave, #195) | `_render_link_for_prompt` bounds url/title/description/site with Python slicing, which counts code points | `renderLinkForPrompt` slices with `String.prototype.slice`, which counts UTF-16 code units | Only differs for astral characters (emoji and the like): a bounded field keeps up to the limit in code points, where JS keeps half as many astral characters and can end on a lone surrogate. Emulating UTF-16 slicing would produce lone surrogates that break UTF-8 encoding of the prompt. Whitespace handling is not a divergence: the normalizer uses JS's exact `\s`/`trim` set. Regression test: `tests/test_ai_messages.py::TestLinkPreviews::test_link_metadata_bounds_count_code_points_not_utf16_units`. | diff --git a/src/chat_sdk/adapters/slack/format.py b/src/chat_sdk/adapters/slack/format.py index 9799ae8c..45c13990 100644 --- a/src/chat_sdk/adapters/slack/format.py +++ b/src/chat_sdk/adapters/slack/format.py @@ -268,6 +268,7 @@ def _convert_mrkdwn_with_code_fences(mrkdwn: str) -> str: cursor = 0 length = len(mrkdwn) angle = _AngleTokenScanner(mrkdwn) + quote_lines = _BlockquoteLines(mrkdwn) # Set when the closing fence splits a line: the text after it lands at the # start of a new line, where CommonMark would promote a leading block # marker Slack rendered inline. @@ -304,7 +305,7 @@ def flush_text_before(end: int) -> None: content_start = cursor + len(_CODE_FENCE) content_end = mrkdwn.find(_CODE_FENCE, content_start) - if content_end == -1 or _is_on_blockquote_line(mrkdwn, cursor): + if content_end == -1 or quote_lines.contains(cursor): # Slack renders an unpaired or quoted ``` literally. Upstream-parity # choice: the fence is left as is (adapter-slack format/index.ts # convertMrkdwnWithCodeFences), so a line-leading unpaired @@ -351,13 +352,37 @@ def _find_inline_code_end(mrkdwn: str, index: int) -> int: return close + 1 -def _is_on_blockquote_line(mrkdwn: str, index: int) -> bool: - # Upstream ``slice(lineStart, index).trimStart().startsWith(">")``, - # matched in place so many fences on one long line copy nothing. - line_start = mrkdwn.rfind("\n", 0, index) + 1 - indent = _JS_LEADING_WHITESPACE.match(mrkdwn, line_start, index) - content_start = indent.end() if indent is not None else line_start - return mrkdwn.startswith(">", content_start, index) +class _BlockquoteLines: + """Upstream ``isOnBlockquoteLine``, remembering the current line. + + Upstream computes ``slice(lineStart, index).trimStart().startsWith(">")`` + per fence. *index* only grows and always sits on a backtick, so the + leading-whitespace run (and thus the answer) is fixed per line: the line + start is found by scanning only the new text since the last call, and + the answer is computed once per line. Results are identical; many fences + on one long line stay linear. + """ + + __slots__ = ("_line_start", "_quoted", "_scanned", "_text") + + def __init__(self, text: str) -> None: + self._text = text + self._scanned = 0 + self._line_start = 0 + self._quoted: bool | None = None + + def contains(self, index: int) -> bool: + """Whether the line holding *index* starts (after whitespace) with ``>``.""" + newline = self._text.rfind("\n", self._scanned, index) + self._scanned = index + if newline != -1: + self._line_start = newline + 1 + self._quoted = None + if self._quoted is None: + indent = _JS_LEADING_WHITESPACE.match(self._text, self._line_start, index) + content_start = indent.end() if indent is not None else self._line_start + self._quoted = self._text.startswith(">", content_start, index) + return self._quoted def _escape_leading_block_marker(text: str) -> str: diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index 96e926e3..aa8f8b97 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -687,6 +687,19 @@ def parse_markdown(text: str) -> Root: Returns a Root dict ``{"type": "root", "children": [...]}``. """ + return _parse_blocks(text, 0) + + +# Divergence from upstream -- see docs/UPSTREAM_SYNC.md. Blockquotes parse +# recursively; past this depth the rest stays literal text so ``> > > ...`` +# from untrusted input cannot raise ``RecursionError`` (remark has no cap). +# One frame per level: 768 keeps the 600-level guarantee of +# ``test_deeply_nested_blockquote_does_not_overflow_the_stack`` with room under +# Python's default recursion limit (1,000) for the caller's frames. +_MAX_BLOCKQUOTE_DEPTH = 768 + + +def _parse_blocks(text: str, depth: int) -> Root: children: list[Content] = [] lines = text.split("\n") i = 0 @@ -763,7 +776,11 @@ def parse_markdown(text: str) -> Root: else: break # Recursively parse blockquote content - bq_ast = parse_markdown("\n".join(bq_lines)) + bq_text = "\n".join(bq_lines) + if depth >= _MAX_BLOCKQUOTE_DEPTH: + children.append(make_blockquote([make_paragraph([make_text(bq_text)])])) + continue + bq_ast = _parse_blocks(bq_text, depth + 1) children.append(make_blockquote(bq_ast.get("children", []))) continue diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index 577fd090..6902bb28 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -68,6 +68,13 @@ def test_keeps_a_line_leading_quoted_fence_as_text(self): # Upstream: "npm test" (multi-backtick code span, a shared-parser gap). assert ast_to_plain_text(ast).replace("`", "") == "npm test" + def test_deeply_nested_quotes_do_not_exhaust_the_stack(self): + """Python-specific guard: ``>`` now unescapes, so 1,100 quote + markers reach the recursive blockquote parser. Past 768 levels the + rest stays literal text instead of raising ``RecursionError``.""" + ast = self.converter.to_ast(">" * 1_100 + " hi") + assert ast_to_plain_text(ast) == ">" * (1_100 - 769) + " hi" + def test_keeps_trailing_text_after_a_code_block_as_a_paragraph(self): ast = self.converter.to_ast("```x``` > note") assert ast["children"][0]["type"] == "code" diff --git a/tests/test_slack_format_primitives.py b/tests/test_slack_format_primitives.py index 8669b66a..7678b8b9 100644 --- a/tests/test_slack_format_primitives.py +++ b/tests/test_slack_format_primitives.py @@ -226,6 +226,8 @@ def test_adversarial_inputs_return_the_expected_output(self): quoted = "> " + "```" * 10_000 assert slack_mrkdwn_to_markdown(quoted) == "> " + "```" * 10_000 assert slack_mrkdwn_to_markdown("" + "```" * 6_665 def test_converts_basic_markdown_bold_to_slack_mrkdwn_bold(self): assert markdown_bold_to_slack_mrkdwn("The **domain** is example.com") == "The *domain* is example.com" From adde6b5b256b0f8e7c9e2b3ad577faa717330935 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 03:51:25 -0700 Subject: [PATCH 05/10] fix(markdown): keep blockquote depth separate from heading level (#283) Astra round 4: the heading branch reused the depth variable and reset the blockquote nesting budget. --- src/chat_sdk/shared/markdown_parser.py | 6 +++--- tests/test_slack_format.py | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index aa8f8b97..207e196b 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -699,7 +699,7 @@ def parse_markdown(text: str) -> Root: _MAX_BLOCKQUOTE_DEPTH = 768 -def _parse_blocks(text: str, depth: int) -> Root: +def _parse_blocks(text: str, quote_depth: int) -> Root: children: list[Content] = [] lines = text.split("\n") i = 0 @@ -777,10 +777,10 @@ def _parse_blocks(text: str, depth: int) -> Root: break # Recursively parse blockquote content bq_text = "\n".join(bq_lines) - if depth >= _MAX_BLOCKQUOTE_DEPTH: + if quote_depth >= _MAX_BLOCKQUOTE_DEPTH: children.append(make_blockquote([make_paragraph([make_text(bq_text)])])) continue - bq_ast = _parse_blocks(bq_text, depth + 1) + bq_ast = _parse_blocks(bq_text, quote_depth + 1) children.append(make_blockquote(bq_ast.get("children", []))) continue diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index 6902bb28..716d6ea6 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -74,6 +74,9 @@ def test_deeply_nested_quotes_do_not_exhaust_the_stack(self): rest stays literal text instead of raising ``RecursionError``.""" ast = self.converter.to_ast(">" * 1_100 + " hi") assert ast_to_plain_text(ast) == ">" * (1_100 - 769) + " hi" + # A heading inside the quotes must not reset the nesting budget. + ast = self.converter.to_ast(">" * 500 + " # h\n" + ">" * 1_100 + " hi") + assert ast_to_plain_text(ast) == "h\n" + ">" * (1_100 - 769) + " hi" def test_keeps_trailing_text_after_a_code_block_as_a_paragraph(self): ast = self.converter.to_ast("```x``` > note") From af49b8adbfda74452b889d855c9238a927b70798 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 03:59:31 -0700 Subject: [PATCH 06/10] fix(markdown): balanced link labels/URLs, 100-level quote cap leaves room for lists (#283) Astra round 5: labelled Slack links with brackets in the label or parens in the URL leaked [..](..) syntax into message.text now that text comes from the AST; the link pattern accepts one level of balanced brackets and parens, as CommonMark/remark do. Quotes nested 700 deep plus nested lists overflowed the stack; the blockquote cap drops to 100 (markdown-it's default maxNesting). --- CHANGELOG.md | 3 ++- docs/UPSTREAM_SYNC.md | 17 +++++++++++++---- src/chat_sdk/shared/markdown_parser.py | 18 +++++++++++++----- tests/test_markdown_faithful.py | 9 ++++++--- tests/test_slack_format.py | 22 ++++++++++++++++++---- 5 files changed, 52 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cefca7d6..9324f781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,8 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **Breaking/consumer-visible (`channel.post()` ids):** `post_channel_message` returns `slack:C123:` (a replyable thread rooted at the new message) instead of `slack:C123:` when Slack returns a string message `ts`, so `chat.thread(sent.thread_id).post(...)` replies in its thread. File-only posts keep `slack:C123:`. - **Breaking/consumer-visible (Socket Mode retries are processed):** envelopes with `retry_attempt > 0` were acked and dropped; they are now routed like first deliveries and logged at info (`"Processing socket mode retry"` with `retry_attempt`, `retry_reason`, `type`). The event-id marker (#268) and core message-id dedupe (10 min TTL, #191) drop true duplicates, so an event missed during a restart or reconnect is no longer lost. - Shared `parse_markdown` follows CommonMark's fence rule: a line like ```` ```npm test``` ```` (a backtick in the info string) is no longer a code-block opener, so a Slack quote starting with an inline fence keeps its text instead of becoming an empty code block. This affects every adapter that parses Markdown, only for such lines. - - **Python-specific (divergence from upstream):** `parse_markdown` nests at most 768 blockquotes; deeper `>` markers stay literal text. Slack `>` now unescapes, and 1,100 of them raised `RecursionError` and dropped the message. See `docs/UPSTREAM_SYNC.md`. + - **Python-specific (divergence from upstream):** `parse_markdown` nests at most 100 blockquotes; deeper `>` markers stay literal text. Slack `>` now unescapes, and 1,100 of them raised `RecursionError` and dropped the message. See `docs/UPSTREAM_SYNC.md`. + - Shared `parse_markdown` links accept balanced brackets in the text and balanced parens in the URL (`[build [failed]](u)`, `…/Foo_(bar))`), as CommonMark does, so Slack labelled links read as upstream in `message.text`. Every adapter that parses Markdown sees this; an unbalanced `(` in a URL or a bare `[` in link text no longer forms a link. - The mrkdwn scanners stay linear on untrusted text (results identical to upstream). Known shared-parser gaps (no multi-backtick code spans, a paragraph's leading space kept, quadratic time on some inputs) predate this change; see `docs/UPSTREAM_SYNC.md`. - **Telegram: multiple files or attachments go out as one media group** (#278; ports vercel/chat `8d7ccdb1` #605, chat@4.34.0). - **Consumer-visible (Telegram):** `post_message` / `thread.post` with 2–10 `files` or 2–10 `attachments` now sends one `sendMediaGroup` album instead of raising `ValidationError("Telegram adapter supports a single file/attachment upload per message")`. Every sent message is cached and the last one is returned. The caption (with `parse_mode`) goes on the first item only, with the same MarkdownV2 → plain-text retry as single uploads. `thread.reply` threads the album to its target (`reply_parameters`). One file or attachment still uses `sendDocument` / `sendPhoto` / `sendVideo` / `sendAudio`. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index e4c3b2f6..dcba0d77 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1304,9 +1304,18 @@ chat@4.41.1). - **Blockquote depth cap (divergence, see the non-parity table).** `>` now unescapes, so `">" * 1100` reaches `parse_markdown`'s recursive blockquote parsing and raised `RecursionError`, failing the message (and a - whole `fetch_messages` page). Past 768 nested levels the rest of the quote - stays literal text. Nested lists recurse the same way and predate this - change (#308). + whole `fetch_messages` page). Past 100 nested levels (markdown-it's default + `maxNesting`) the rest of the quote stays literal text, which also leaves + stack room for lists nested inside the quote. Nested lists on their own + recurse the same way and predate this change (#308). +- **Shared parser fix (link text and destinations).** The converter emits + `[label](url)`, and remark accepts balanced brackets in the label and + balanced parens in the URL. `parse_markdown`'s link pattern now does too + (one level), so `` reads + `build [failed]` and a Wikipedia URL ending in `_(bar)` keeps its `)` + instead of leaking `[…](…)` syntax or a stray `)` into `message.text`. + CommonMark-invalid cases (an unbalanced `(` in the URL, a bare `[` inside + the label) no longer form a link, as in remark. ### Teams Adaptive Card 1.5 rendering (chat@4.36–4.41, #220) @@ -3227,7 +3236,7 @@ stay explicit instead of being rediscovered in code review. | Area | Python behavior | TS behavior | Rationale | |------|----------------|-------------|-----------| | JSX Card/Modal elements | Not supported; tests skipped | `Card()` returns JSX element | Python has no JSX runtime | -| Blockquote nesting cap (4.41 wave, #283) | `parse_markdown` nests at most 768 blockquotes; deeper `>` markers stay literal text in the innermost quote | remark nests without limit (`"> " * 1100 + "x"` gives 1,100 blockquotes) | Blockquotes parse recursively and Python's recursion limit (1,000) turned untrusted input such as Slack `">" * 1100` into a `RecursionError` that dropped the message. Only absurdly deep quotes differ. Regression test: `tests/test_slack_format.py::TestToMarkdown::test_deeply_nested_quotes_do_not_exhaust_the_stack`. | +| Blockquote nesting cap (4.41 wave, #283) | `parse_markdown` nests at most 100 blockquotes (markdown-it's default `maxNesting`); deeper `>` markers stay literal text in the innermost quote | remark nests without limit (`"> " * 1100 + "x"` gives 1,100 blockquotes) | Blockquotes parse recursively and Python's recursion limit (1,000) turned untrusted input such as Slack `">" * 1100` into a `RecursionError` that dropped the message. Only absurdly deep quotes differ. Regression test: `tests/test_slack_format.py::TestToMarkdown::test_deeply_nested_quotes_do_not_exhaust_the_stack`. | | Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` binds to `{threadId, "thread"}`, where `threadId` is the id the adapter reported for the post (or the channel id) | The reported thread id often never equals a click's thread id: Teams and Google Chat report the channel id (upstream as well; Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), Python's Slack reported the synthetic `slack:C…:` until #283 (it still does when the response has no string ts) while clicks carry the message ts, a Slack DM click carries no ts even though the post now reports one, and a chained edit dropped the override until #195 ported `16ea171e`. Upstream's edited buttons never POST in those cases. Every click on the message derives the channel id, and the channel scope is no broader than the original post's. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_slack_channel_card_resolves_for_the_real_click` (real Slack id functions and `_handle_block_actions`, channel and DM, before and after #283), `::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. To be filed as an upstream issue against vercel/chat (Teams and Google Chat edited channel cards never POST); upstream Slack is unaffected, since its `postChannelMessage` and DM clicks both carry the message ts. | | Callback-token lease fence (4.41 wave, #194) | After deleting a matched record, `resolve_callback_url` calls `extend_lock(lock, CALLBACK_LOCK_TTL_MS)`. If that fails, the 10 s lease lapsed mid-consume, and the call returns `None` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. To be filed as an upstream issue against vercel/chat (a stalled `get`/`delete` past the 10 s lease lets a second click double-POST). Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | | Link-preview fence slicing (4.41 wave, #195) | `_render_link_for_prompt` bounds url/title/description/site with Python slicing, which counts code points | `renderLinkForPrompt` slices with `String.prototype.slice`, which counts UTF-16 code units | Only differs for astral characters (emoji and the like): a bounded field keeps up to the limit in code points, where JS keeps half as many astral characters and can end on a lone surrogate. Emulating UTF-16 slicing would produce lone surrogates that break UTF-8 encoding of the prompt. Whitespace handling is not a divergence: the normalizer uses JS's exact `\s`/`trim` set. Regression test: `tests/test_ai_messages.py::TestLinkPreviews::test_link_metadata_bounds_count_code_points_not_utf16_units`. | diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index 207e196b..52850f74 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -316,7 +316,16 @@ def _restore_escapes_as_literal_pair(text: str) -> str: # Images: ![alt](url) or ![alt](url "title") ("image", re.compile(r'(? Root: # Divergence from upstream -- see docs/UPSTREAM_SYNC.md. Blockquotes parse # recursively; past this depth the rest stays literal text so ``> > > ...`` # from untrusted input cannot raise ``RecursionError`` (remark has no cap). -# One frame per level: 768 keeps the 600-level guarantee of -# ``test_deeply_nested_blockquote_does_not_overflow_the_stack`` with room under -# Python's default recursion limit (1,000) for the caller's frames. -_MAX_BLOCKQUOTE_DEPTH = 768 +# One frame per level; 100 (markdown-it's default ``maxNesting``) leaves most +# of Python's default recursion limit (1,000) for nested lists inside the quote. +_MAX_BLOCKQUOTE_DEPTH = 100 def _parse_blocks(text: str, quote_depth: int) -> Root: diff --git a/tests/test_markdown_faithful.py b/tests/test_markdown_faithful.py index 92775429..b388ae63 100644 --- a/tests/test_markdown_faithful.py +++ b/tests/test_markdown_faithful.py @@ -649,9 +649,12 @@ def row(value: str) -> Content: assert ast_to_plain_text(table) == "\x1c\na" def test_deeply_nested_blockquote_does_not_overflow_the_stack(self): - # One frame per nesting level: a 600-deep blockquote (a ~600-byte - # inbound comment) parses fine, so extraction must not RecursionError. - assert ast_to_plain_text(parse_markdown(">" * 600 + " x")) == "x" + # Extraction walks iteratively, so a 600-deep blockquote AST must not + # RecursionError. (parse_markdown itself caps quote nesting at 100.) + node: Content = {"type": "paragraph", "children": [{"type": "text", "value": "x"}]} + for _ in range(600): + node = {"type": "blockquote", "children": [node]} + assert ast_to_plain_text({"type": "root", "children": [node]}) == "x" def test_deeply_nested_list_does_not_overflow_the_stack(self): text = "\n".join(" " * depth + "- a" for depth in range(300)) diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index 716d6ea6..b78d2812 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -70,13 +70,27 @@ def test_keeps_a_line_leading_quoted_fence_as_text(self): def test_deeply_nested_quotes_do_not_exhaust_the_stack(self): """Python-specific guard: ``>`` now unescapes, so 1,100 quote - markers reach the recursive blockquote parser. Past 768 levels the + markers reach the recursive blockquote parser. Past 100 levels the rest stays literal text instead of raising ``RecursionError``.""" ast = self.converter.to_ast(">" * 1_100 + " hi") - assert ast_to_plain_text(ast) == ">" * (1_100 - 769) + " hi" + assert ast_to_plain_text(ast) == ">" * (1_100 - 101) + " hi" # A heading inside the quotes must not reset the nesting budget. - ast = self.converter.to_ast(">" * 500 + " # h\n" + ">" * 1_100 + " hi") - assert ast_to_plain_text(ast) == "h\n" + ">" * (1_100 - 769) + " hi" + ast = self.converter.to_ast(">" * 50 + " # h\n" + ">" * 1_100 + " hi") + assert ast_to_plain_text(ast) == "h\n" + ">" * (1_100 - 101) + " hi" + # Lists inside deep quotes keep their own stack room. + ast = self.converter.to_ast(">" * 700 + "- " * 160 + "hi") + assert ast_to_plain_text(ast) == ">" * (700 - 101) + "- " * 160 + "hi" + + def test_keeps_labelled_links_with_brackets_or_parens_readable(self): + """Shared parser fix: link text may hold balanced brackets and the + destination balanced parens (CommonMark), so the converter's + ``[label](url)`` output reads as upstream's plain text.""" + ast = self.converter.to_ast("See ") + assert ast_to_plain_text(ast) == "See build [failed]" + ast = self.converter.to_ast("See ") + assert ast_to_plain_text(ast) == "See Foo" + link = ast["children"][0]["children"][1] + assert (link["type"], link["url"]) == ("link", "https://en.wikipedia.org/wiki/Foo_(bar)") def test_keeps_trailing_text_after_a_code_block_as_a_paragraph(self): ast = self.converter.to_ast("```x``` > note") From 71f189c9d6495af51a0398ad7bcd6620e9ce630c Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 04:05:13 -0700 Subject: [PATCH 07/10] fix(markdown): escaped parens in link destinations never balance (#283) Astra round 6: the balanced-paren destination treated an escaped \( as an opener, so [manual](https://example.com/a\(b) stopped being a link. Escape-sentinel pairs are consumed whole, as in the label pattern. --- docs/UPSTREAM_SYNC.md | 3 ++- scripts/fidelity_target.json | 4 ++-- src/chat_sdk/shared/markdown_parser.py | 5 +++-- tests/test_markdown_faithful.py | 10 ++++++++++ 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index dcba0d77..d1d6188f 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1315,7 +1315,8 @@ chat@4.41.1). `build [failed]` and a Wikipedia URL ending in `_(bar)` keeps its `)` instead of leaking `[…](…)` syntax or a stray `)` into `message.text`. CommonMark-invalid cases (an unbalanced `(` in the URL, a bare `[` inside - the label) no longer form a link, as in remark. + the label) no longer form a link, as in remark. Escaped `\(` / `\)` in a + destination are consumed whole and never count toward the balance. ### Teams Adaptive Card 1.5 rendering (chat@4.36–4.41, #220) diff --git a/scripts/fidelity_target.json b/scripts/fidelity_target.json index 2d972791..19f9923e 100644 --- a/scripts/fidelity_target.json +++ b/scripts/fidelity_target.json @@ -8,7 +8,7 @@ "matched_exact": 889, "matched_fuzzy": 75, "missing": 100, - "extra": 530 + "extra": 531 }, "absent_ts_files": [], "files": { @@ -80,7 +80,7 @@ "matched_exact": 125, "matched_fuzzy": 5, "missing_count": 0, - "extra_count": 86, + "extra_count": 87, "missing": [], "fuzzy_matches": [ ["handles empty AST", "test_fromast_handles_empty_ast"], diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index 52850f74..795c3b84 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -318,12 +318,13 @@ def _restore_escapes_as_literal_pair(text: str) -> str: # Links: [text](url) or [text](url "title") # CommonMark allows balanced brackets in link text and balanced parens in # the destination (one level here): ``[build [failed]](u)``, - # ``[Foo](https://en.wikipedia.org/wiki/Foo_(bar))``. + # ``[Foo](https://en.wikipedia.org/wiki/Foo_(bar))``. Escape-sentinel + # pairs (``\(`` / ``\)``) are consumed whole, so they never balance. ( "link", re.compile( r"(? Date: Thu, 1 Oct 2026 04:30:44 -0700 Subject: [PATCH 08/10] fix(markdown): CommonMark paragraph trim, single-marker breaks, no intraword _, no nested links; pin memo scanners and socket retry marker (#283) Addresses the two-reviewer round on #307: - CHANGELOG #268/#194 bullets no longer describe the pre-#283 behavior - link text may not contain a full inner link ([[a](u) b](v)) - thematic breaks repeat one marker (-_- is text) - _ emphasis is never intraword (snake_case + underscored URLs) - paragraphs drop leading/trailing spaces (inline-fence text) - exact-output tests for _BlockquoteLines/_AngleTokenScanner memo state - socket retry reaches the event-delivered marker (test) - docs: lazy-continuation and new quadratic triggers recorded under #308 --- CHANGELOG.md | 9 +++--- docs/UPSTREAM_SYNC.md | 28 ++++++++++++---- src/chat_sdk/shared/markdown_parser.py | 19 ++++++++--- tests/test_markdown_faithful.py | 45 ++++++++++++++++++++++++++ tests/test_slack_format.py | 8 ++--- tests/test_slack_format_primitives.py | 23 +++++++++++++ tests/test_slack_inbound_content.py | 7 ++++ tests/test_slack_socket_mode.py | 34 +++++++++++++++++++ tests/test_turn_cancellation.py | 4 ++- 9 files changed, 155 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9324f781..5c70a047 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,11 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **Breaking/consumer-visible (code blocks keep their first line):** Slack treats the text right after an opening ```` ``` ```` as code; it was read as an info string and dropped, so `` ```npm test``` `` gave an empty `formatted` (and `text` kept the backticks). Each paired fence now parses as a `code` node and `text` holds the code (`"npm test"`). Unpaired fences, fences in inline code or `<…>` tokens, and fences on `>` quote lines stay literal, as in Slack. - **Breaking/consumer-visible (`channel.post()` ids):** `post_channel_message` returns `slack:C123:` (a replyable thread rooted at the new message) instead of `slack:C123:` when Slack returns a string message `ts`, so `chat.thread(sent.thread_id).post(...)` replies in its thread. File-only posts keep `slack:C123:`. - **Breaking/consumer-visible (Socket Mode retries are processed):** envelopes with `retry_attempt > 0` were acked and dropped; they are now routed like first deliveries and logged at info (`"Processing socket mode retry"` with `retry_attempt`, `retry_reason`, `type`). The event-id marker (#268) and core message-id dedupe (10 min TTL, #191) drop true duplicates, so an event missed during a restart or reconnect is no longer lost. - - Shared `parse_markdown` follows CommonMark's fence rule: a line like ```` ```npm test``` ```` (a backtick in the info string) is no longer a code-block opener, so a Slack quote starting with an inline fence keeps its text instead of becoming an empty code block. This affects every adapter that parses Markdown, only for such lines. + - Shared `parse_markdown` follows CommonMark's fence rule: a line like ```` ```npm test``` ```` (a backtick in the info string) is no longer a code-block opener, so a Slack quote starting with an inline fence keeps its text instead of becoming an empty code block. This affects every adapter that parses Markdown, only for such lines, and those lines now go through the (quadratic, #308) inline parser. + - Shared `parse_markdown` follows four more CommonMark rules that `message.text` exposed (every adapter that parses Markdown sees them): a paragraph drops its lines' leading spaces/tabs and its trailing ones (`run ```npm test``` please` reads `run`, `npm test`, `please`; a whitespace-padded message is trimmed); a thematic break repeats one marker (`-_-` is text, not an empty break); `_` emphasis is never intraword (`my_var` and `?utm_source=` keep their underscores); links never nest (`[[a](u) b](v)` is the inner link with literal brackets). - **Python-specific (divergence from upstream):** `parse_markdown` nests at most 100 blockquotes; deeper `>` markers stay literal text. Slack `>` now unescapes, and 1,100 of them raised `RecursionError` and dropped the message. See `docs/UPSTREAM_SYNC.md`. - Shared `parse_markdown` links accept balanced brackets in the text and balanced parens in the URL (`[build [failed]](u)`, `…/Foo_(bar))`), as CommonMark does, so Slack labelled links read as upstream in `message.text`. Every adapter that parses Markdown sees this; an unbalanced `(` in a URL or a bare `[` in link text no longer forms a link. - - The mrkdwn scanners stay linear on untrusted text (results identical to upstream). Known shared-parser gaps (no multi-backtick code spans, a paragraph's leading space kept, quadratic time on some inputs) predate this change; see `docs/UPSTREAM_SYNC.md`. + - The mrkdwn scanners stay linear on untrusted text (results identical to upstream). Known shared-parser gaps (no multi-backtick code spans, no lazy blockquote continuation so `> quoted` + a reply line reads as two blocks, quadratic time on some inputs, which the fence rule above reaches with new inputs) are tracked in #308; see `docs/UPSTREAM_SYNC.md`. - **Telegram: multiple files or attachments go out as one media group** (#278; ports vercel/chat `8d7ccdb1` #605, chat@4.34.0). - **Consumer-visible (Telegram):** `post_message` / `thread.post` with 2–10 `files` or 2–10 `attachments` now sends one `sendMediaGroup` album instead of raising `ValidationError("Telegram adapter supports a single file/attachment upload per message")`. Every sent message is cached and the last one is returned. The caption (with `parse_mode`) goes on the first item only, with the same MarkdownV2 → plain-text retry as single uploads. `thread.reply` threads the album to its target (`reply_parameters`). One file or attachment still uses `sendDocument` / `sendPhoto` / `sendVideo` / `sendAudio`. - New `ValidationError`s: more than 10 items ("Telegram media groups support 2-10 files"); a card with buttons ("Telegram media groups do not support inline keyboards"); documents or audio mixed with another type (photos and videos may mix). Mixing `files` and `attachments` still raises. @@ -191,7 +192,7 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - Tokens resolve only for the button that minted them (`actionId`) and in the conversation they were posted to. Thread posts, schedules and edits bind to the thread. Channel posts, schedules and channel `SentMessage.edit`s bind to the channel. A `post_ephemeral` DM fallback binds to the DM channel, and with neither native ephemeral nor a DM fallback no token is minted. A click whose action or conversation does not match leaves the record in place. - The token swap now keeps every button field except `callback_url` (it used to copy a fixed whitelist), so new fields such as `tooltip` (#202) survive. - **Python-specific (divergence from upstream):** after deleting a matched record, the resolver checks with `extend_lock` that its 10-second lease never lapsed, and returns `None` if it did. Upstream returns the record regardless, so a state call that stalls past the lease can let a second click also resolve and POST. See `docs/UPSTREAM_SYNC.md`. - - **Python-specific (divergence from upstream):** a channel `SentMessage.edit` binds its tokens to the channel (the scope the original `channel.post` used), not to `{reported thread id, "thread"}`. The thread id an adapter reports for a channel post often never equals a click's thread id: Teams and Google Chat report the channel id, Slack reports the synthetic `slack:C…:` (a click carries the message ts, and a Slack DM click carries no ts even once #283 makes the post report one), and a chained edit drops the reported id. Upstream's thread scope would leave those edited buttons never POSTing. + - **Python-specific (divergence from upstream):** a channel `SentMessage.edit` binds its tokens to the channel (the scope the original `channel.post` used), not to `{reported thread id, "thread"}`. The thread id an adapter reports for a channel post often never equals a click's thread id: Teams and Google Chat report the channel id, Slack reported the synthetic `slack:C…:` until #283 and still does without a string ts (a click carries the message ts, and a Slack DM click carries no ts even though the post now reports one), and a chained edit drops the reported id. Upstream's thread scope would leave those edited buttons never POSTing. - Known gap (see `docs/UPSTREAM_SYNC.md`): Google Chat cards `thread.post`ed into a DM thread never POST (upstream parity: the card click omits the `:dm` suffix). - API: `process_card_callback_urls(card, state, scope)` takes a required `CallbackScope`. `resolve_callback_url(token, state, context=None)` takes a `CallbackContext` (a `None` context never matches). `ResolvedCallback` gains keyword-only `action_id` and `scope`. New constant: `CALLBACK_LOCK_TTL_MS = 10_000`. - **BREAKING (security) — Telegram: webhook verification is required by default; repeated updates are deduplicated** (#224; upstream vercel/chat#858, #799, #813). @@ -457,7 +458,7 @@ Ports upstream `32687038` (vercel/chat#830, chat@4.38.1), the Google Chat part o - Fidelity: `ai/index.test.ts` 26 → 0 missing, `ai/messages.test.ts` 10 → 9 at `chat@4.41.1`. - **Slack: Enterprise Grid org-wide installs, `authorizations[]` routing, event retry marker** (#268, split from #213; ports the non-cache half of vercel/chat `907450d7` #724, chat@4.35.0). - **Consumer-visible: org-wide OAuth installs now succeed.** `handle_oauth_callback` used to raise `missing access_token or team.id` for an org-wide install (`team: null`). It now stores it under `enterprise.id`, the key org-wide webhooks resolve by, and raises `missing access_token or enterprise.id` when that is absent. The result gains `enterprise_id` and `is_enterprise_install`; `team_id` is the storage key. `SlackInstallation` gains `enterprise_id` / `is_enterprise_install`. - - **Consumer-visible: retried events that were already dispatched are dropped.** Each dispatched event writes a `slack:event-delivered:{event_id}` state key (24 h TTL, fire-and-forget). A delivery with `x-slack-retry-num > 0` (or a forwarded socket event with `retryNum > 0`) whose key exists is acked and not processed; first deliveries never read state, and a retry whose original never arrived is still processed. Live Socket Mode retries are still skipped until #283. + - **Consumer-visible: retried events that were already dispatched are dropped.** Each dispatched event writes a `slack:event-delivered:{event_id}` state key (24 h TTL, fire-and-forget). A delivery with `x-slack-retry-num > 0` (or a forwarded socket event with `retryNum > 0`) whose key exists is acked and not processed; first deliveries never read state, and a retry whose original never arrived is still processed. Since #283, live Socket Mode retries (`retry_attempt > 0`) are routed and reach the marker as well. - Multi-workspace events resolve their installation from `authorizations[0]` before the top-level `team_id` / `enterprise_id`, so Slack Connect events route to the receiving installation. Socket Mode `events_api`, slash commands and interactive payloads now resolve org-wide installs by enterprise ID, like HTTP. - Under an org-wide install, the adapter's Web API calls send the event's workspace `team_id`, and calls to the event's channel echo a Slack Connect `context_team_id` as `client_context_team_id`. A `team_id` the caller passes wins. The #95 `chat_stream` `team_id` is unchanged. - `W…` user ids count as raw user ids in outgoing `@mentions`. `with_bot_token` / `with_bot_token_async` accept keyword-only `installation_id=` to scope installation-owned caches outside webhooks. `RequestContext` gains `team_id`, `context_team_id` and `context_channel`. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index d1d6188f..21a22353 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1285,14 +1285,28 @@ chat@4.41.1). re-slices the line per fence). `tests/test_slack_format_primitives.py` covers 50k-character inputs, and a differential fuzz against upstream `format/index.ts` (40k random mrkdwn inputs) found no mismatch. -- **Known gaps (shared parser, not Slack-specific).** Two `markdown.test.ts` - cases are adapted: `parse_markdown` has no multi-backtick code spans +- **Known gaps (shared parser, not Slack-specific).** One `markdown.test.ts` + case is adapted: `parse_markdown` has no multi-backtick code spans (```` ```c``` ```` inside a quote stays backticked text, where remark reads a - code span) and keeps a paragraph's leading space (`"```x``` > note"` - gives `"x\n\n > note"`, upstream `"x\n\n> note"`). `parse_markdown` is - also quadratic on some inputs (about 2.4 s for 20k characters of `` `x ``); - this predates the port, since `to_ast` always ran it on every inbound - message, and is tracked in #308 (with the two gaps above). + code span). Not handled either, and now visible in `message.text`: no lazy + blockquote continuation (`"> quoted\nreply"` gives `"quoted\n\nreply"`, + upstream `"quoted\nreply"` in one quote paragraph), and `\r` is not a line + ending. `parse_markdown` is also quadratic on some inputs (about 1.6 s for + 16k characters of `` `x ``); this predates the port, since `to_ast` always + ran it on every inbound message. The fence rule below adds new inputs of + that cost: ```` ``` ```` followed by `` a` `` repeated (16 KB, about 1.6 s) + or ```` ```x` ```` lines (48 KB, about 10 s) were one O(n) code block + before and now go through the inline parser. All are tracked in #308. +- **Shared parser fixes (CommonMark rules exposed by `message.text`).** + Since `text` is the plain text of `parse_markdown`, these parser bugs now + lost or corrupted text, so the parser follows CommonMark (values checked + against remark): a paragraph drops its lines' leading spaces/tabs and its + trailing ones (`"run ```npm test``` please"` reads `"run\n\nnpm test\n\nplease"`, + not `"run \n\nnpm test\n\n please"`); a thematic break repeats one marker + (`-_-` stayed empty text); `_` emphasis is never intraword (`my_var` and a + `?utm_source=` URL after a snake_case word kept losing underscores and + leaking `[label](url)`); and links never nest (`[[a](u) b](v)` is the inner + link with literal outer brackets). - **Shared parser fix (CommonMark fence rule).** A backtick fence's info string may not contain a backtick, so `parse_markdown` no longer opens a code block on a line like ```` ```npm test``` ````. The Slack normalizer diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index 795c3b84..13bef2d8 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -320,10 +320,12 @@ def _restore_escapes_as_literal_pair(text: str) -> str: # the destination (one level here): ``[build [failed]](u)``, # ``[Foo](https://en.wikipedia.org/wiki/Foo_(bar))``. Escape-sentinel # pairs (``\(`` / ``\)``) are consumed whole, so they never balance. + # A nested ``[...]`` directly followed by ``(`` is an inner link, and links + # never nest: ``[[a](u) b](v)`` is the inner link with literal brackets. ( "link", re.compile( - r"(? str: ("delete", re.compile(r"(? list[Conten # Patterns used by the block parser _HEADING_RE = re.compile(r"^(#{1,6})\s+(.*)") -_THEMATIC_BREAK_RE = re.compile(r"^([-*_]\s*){3,}\s*$") +# CommonMark: three or more of the *same* marker (``-_-`` is text, not a break). +_THEMATIC_BREAK_RE = re.compile(r"^([-*_])(?:[ \t]*\1){2,}[ \t]*$") # CommonMark: a backtick fence's info string may not contain a backtick, so # "```npm test```" on one line is a (code span) paragraph, not a fence. _FENCED_CODE_START_RE = re.compile(r"^(`{3,}(?=[^`]*$)|~{3,})(.*)") @@ -837,7 +843,10 @@ def _parse_blocks(text: str, quote_depth: int) -> Root: para_lines.append(next_line) i += 1 - children.append(make_paragraph(_parse_inline("\n".join(para_lines)))) + # CommonMark strips each paragraph line's leading spaces/tabs and the + # paragraph's trailing ones (``"run \n```"`` reads ``run``). + para_text = "\n".join(para_line.lstrip(" \t") for para_line in para_lines).rstrip(" \t") + children.append(make_paragraph(_parse_inline(para_text))) return make_root(children) diff --git a/tests/test_markdown_faithful.py b/tests/test_markdown_faithful.py index ee417cca..619f49c5 100644 --- a/tests/test_markdown_faithful.py +++ b/tests/test_markdown_faithful.py @@ -7,6 +7,8 @@ import re +import pytest + from chat_sdk.cards import ( Actions, Button, @@ -1509,6 +1511,49 @@ def test_handles_markdown_with_thematic_break_hr(self): assert "thematicBreak" in types +class TestCommonMarkRulesExposedBySlackText: + """Python-only: CommonMark rules the shared parser follows since #283 made + Slack's ``message.text`` the plain text of ``parse_markdown``. Expected + values are remark-parse + remark-gfm ``toPlainText`` output.""" + + @pytest.mark.parametrize( + ("markdown", "expected"), + [ + # Links never nest: the inner link wins, outer brackets stay text. + ("[[a](https://x.com) b](https://y.com)", "[a b](https://y.com)"), + ("[a [b](https://x.com)](https://y.com)", "[a b](https://y.com)"), + # A thematic break repeats one marker; mixed markers are text. + ("-_-", "-_-"), + ("ok\n-_-", "ok\n-_-"), + ("*-*", "-"), + ("_*_", "*"), + ("- - -", ""), + ("_ _ _", ""), + # ``_`` emphasis is never intraword. + ("my_var and snake_case_name", "my_var and snake_case_name"), + ("see my_notes: [link](https://e.com/?utm_source=x)", "see my_notes: link"), + ("x _y_z", "x _y_z"), + ("(_emph_)", "(emph)"), + ("_a_b_", "a_b"), + # Paragraph lines lose leading spaces, the paragraph its trailing ones. + (" lead\n b ", "lead\nb"), + ("run \n```\nnpm test\n```\n please", "run\n\nnpm test\n\nplease"), + ], + ) + def test_plain_text_matches_remark(self, markdown: str, expected: str): + assert ast_to_plain_text(parse_markdown(markdown)) == expected + + def test_nested_link_label_keeps_only_the_inner_link(self): + para = parse_markdown("[[a](https://x.com) b](https://y.com)")["children"][0] + links = [c for c in para["children"] if c.get("type") == "link"] + assert [link["url"] for link in links] == ["https://x.com"] + + def test_underscored_url_after_snake_case_stays_one_link(self): + para = parse_markdown("my_var see [page](https://example.com/my_page)")["children"][0] + assert [c["type"] for c in para["children"]] == ["text", "link"] + assert para["children"][1]["url"] == "https://example.com/my_page" + + # ============================================================================ # Backup absorbers for false-positive "\n" matches in verify script. # The TS file contains `result.split("\n")` which the verify script's regex diff --git a/tests/test_slack_format.py b/tests/test_slack_format.py index b78d2812..f4b6335b 100644 --- a/tests/test_slack_format.py +++ b/tests/test_slack_format.py @@ -97,11 +97,9 @@ def test_keeps_trailing_text_after_a_code_block_as_a_paragraph(self): assert ast["children"][0]["type"] == "code" assert ast["children"][0]["value"] == "x" assert ast["children"][1]["type"] == "paragraph" - # Upstream: toPlainText == "x\n\n> note". CommonMark strips the - # paragraph's leading space; the shared Python parser keeps it (known - # parser limitation), so compare without it. The escaped ``\>`` must - # still read as text, not a blockquote. - assert ast_to_plain_text(ast).replace("\n\n ", "\n\n") == "x\n\n> note" + # The escaped ``\>`` reads as text, not a blockquote, and the + # paragraph's leading space is stripped (CommonMark). + assert ast_to_plain_text(ast) == "x\n\n> note" def test_keeps_code_content_verbatim_inside_the_fence(self): ast = self.converter.to_ast("```int *a = *b;```") diff --git a/tests/test_slack_format_primitives.py b/tests/test_slack_format_primitives.py index 7678b8b9..539e4959 100644 --- a/tests/test_slack_format_primitives.py +++ b/tests/test_slack_format_primitives.py @@ -229,6 +229,29 @@ def test_adversarial_inputs_return_the_expected_output(self): indented_quote = " " * 20_000 + ">" + "```" * 6_665 assert slack_mrkdwn_to_markdown(indented_quote) == " " * 20_000 + ">" + "```" * 6_665 + @pytest.mark.parametrize( + ("mrkdwn", "expected"), + [ + # The quoted-line answer resets on each new line, both ways. + ("> ```a``` b\nsee ```x``` y", "> ```a``` b\nsee \n```\nx\n```\n y"), + ("see ```x``` y\n> ```a``` b", "see \n```\nx\n```\n y\n> ```a``` b"), + (" > ```q``` x\n ```c``` y", " > ```q``` x\n \n```\nc\n```\n y"), + # JS ``trimStart`` whitespace includes NBSP before ``>``. + ("\u00a0> a ```c``` b", "\u00a0> a ```c``` b"), + # A failed ``<`` scan stops at ``\n`` / ``\r``; tokens after it still convert. + ("a < b\n ping", "a < b\n@here ping"), + ("a ", "a ", "a "), + # A ``` run is no inline-code opener for the mention pass. + ("``` `", "``` @here `"), + ], + ) + def test_memoized_scanners_match_upstream_across_lines(self, mrkdwn: str, expected: str): + """Exact upstream ``slackMrkdwnToMarkdown`` output for inputs that + exercise the scanners' remembered state (``_BlockquoteLines`` and + ``_AngleTokenScanner``) across line boundaries.""" + assert slack_mrkdwn_to_markdown(mrkdwn) == expected + def test_converts_basic_markdown_bold_to_slack_mrkdwn_bold(self): assert markdown_bold_to_slack_mrkdwn("The **domain** is example.com") == "The *domain* is example.com" diff --git a/tests/test_slack_inbound_content.py b/tests/test_slack_inbound_content.py index c948bff7..fff36e79 100644 --- a/tests/test_slack_inbound_content.py +++ b/tests/test_slack_inbound_content.py @@ -569,6 +569,13 @@ async def test_sync_and_async_paths_give_equal_content_without_mentions(self): ("- item\n> quoted `code`", "item\n\nquoted code"), (" ", ""), ("hi\n", "hi"), + # Values below are upstream's (slackMrkdwnToMarkdown + remark toPlainText). + ("run ```npm test``` please", "run\n\nnpm test\n\nplease"), + (" lead and trail ", "lead and trail"), + ("-_-", "-_-"), + ("ok\n-_-", "ok\n-_-"), + ("see my_notes: ", "see my_notes: link"), + ("my_var see ", "my_var see page"), ], ) async def test_body_text_is_the_plain_text_of_formatted(self, text: str, expected: str): diff --git a/tests/test_slack_socket_mode.py b/tests/test_slack_socket_mode.py index 78842818..de52a783 100644 --- a/tests/test_slack_socket_mode.py +++ b/tests/test_slack_socket_mode.py @@ -559,6 +559,40 @@ async def test_processes_retries_like_first_deliveries_dedupe_drops_true_duplica {"retry_attempt": 1, "retry_reason": "timeout", "type": "events_api"}, ) + @pytest.mark.parametrize(("marker_seeded", "dispatched"), [(True, False), (False, True)]) + async def test_socket_retry_consults_the_event_delivered_marker(self, marker_seeded: bool, dispatched: bool): + """A live socket retry carries its count to the #268 event-id marker: + an already-dispatched ``event_id`` is acked and dropped, while a retry + whose first delivery never arrived is still processed.""" + adapter = _make_socket_adapter() + chat = _make_mock_chat() + adapter._chat = chat + if marker_seeded: + chat.get_state()._cache["slack:event-delivered:Ev1"] = True + request = MagicMock() + request.envelope_id = "env-1" + request.type = "events_api" + request.payload = { + "event_id": "Ev1", + "event": { + "type": "message", + "channel": "C123", + "ts": "1234567890.123456", + "text": "retried", + "user": "U_USER", + }, + } + request.retry_attempt = 1 + request.retry_reason = "timeout" + client = MagicMock() + client.send_socket_mode_response = AsyncMock() + + await adapter._on_socket_request(client, request) + await asyncio.sleep(0) + + assert client.send_socket_mode_response.await_count == 1 + assert chat.process_message.called is dispatched + async def test_unknown_event_type_acks_and_does_nothing(self): adapter = _make_socket_adapter() adapter._chat = _make_mock_chat() diff --git a/tests/test_turn_cancellation.py b/tests/test_turn_cancellation.py index ff180540..fe1a0ce3 100644 --- a/tests/test_turn_cancellation.py +++ b/tests/test_turn_cancellation.py @@ -250,7 +250,9 @@ async def source() -> AsyncIterator[str]: signal._abort() sent = await asyncio.wait_for(posting, 1) - assert sent.text == "Hello " + # ``text`` is the plain text of the parsed markdown; like remark, + # ``parse_markdown`` strips a paragraph's trailing space. + assert sent.text == "Hello" assert adapter._edit_calls[-1] == (THREAD_ID, "msg-1", PostableMarkdown(markdown="Hello ")) adapter.end_typing.assert_awaited_once_with(THREAD_ID, "active") From 868106ec2ea077c435896a914d77c9da83318b0c Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 04:30:55 -0700 Subject: [PATCH 09/10] chore(fidelity): regenerate fidelity_target.json (#283) --- scripts/fidelity_target.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/fidelity_target.json b/scripts/fidelity_target.json index 19f9923e..ff7286c0 100644 --- a/scripts/fidelity_target.json +++ b/scripts/fidelity_target.json @@ -8,7 +8,7 @@ "matched_exact": 889, "matched_fuzzy": 75, "missing": 100, - "extra": 531 + "extra": 534 }, "absent_ts_files": [], "files": { @@ -80,7 +80,7 @@ "matched_exact": 125, "matched_fuzzy": 5, "missing_count": 0, - "extra_count": 87, + "extra_count": 90, "missing": [], "fuzzy_matches": [ ["handles empty AST", "test_fromast_handles_empty_ast"], From 236ebf30810233d958d9a98a7aa8e1d89964a0c6 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 1 Oct 2026 04:40:10 -0700 Subject: [PATCH 10/10] address gpt-6-astra review: code spans opaque in link labels, CRLF thematic breaks (#283) --- docs/UPSTREAM_SYNC.md | 4 +++- scripts/fidelity_target.json | 4 ++-- src/chat_sdk/shared/markdown_parser.py | 10 ++++++++-- tests/test_markdown_faithful.py | 14 ++++++++++++++ 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 21a22353..c5deccfe 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -1330,7 +1330,9 @@ chat@4.41.1). instead of leaking `[…](…)` syntax or a stray `)` into `message.text`. CommonMark-invalid cases (an unbalanced `(` in the URL, a bare `[` inside the label) no longer form a link, as in remark. Escaped `\(` / `\)` in a - destination are consumed whole and never count toward the balance. + destination are consumed whole and never count toward the balance. A + closed code span in the label is opaque (code spans bind tighter than link + brackets), so ``[`[`](u)`` stays a link with an `inlineCode` child. ### Teams Adaptive Card 1.5 rendering (chat@4.36–4.41, #220) diff --git a/scripts/fidelity_target.json b/scripts/fidelity_target.json index ff7286c0..e43ea60a 100644 --- a/scripts/fidelity_target.json +++ b/scripts/fidelity_target.json @@ -8,7 +8,7 @@ "matched_exact": 889, "matched_fuzzy": 75, "missing": 100, - "extra": 534 + "extra": 536 }, "absent_ts_files": [], "files": { @@ -80,7 +80,7 @@ "matched_exact": 125, "matched_fuzzy": 5, "missing_count": 0, - "extra_count": 90, + "extra_count": 92, "missing": [], "fuzzy_matches": [ ["handles empty AST", "test_fromast_handles_empty_ast"], diff --git a/src/chat_sdk/shared/markdown_parser.py b/src/chat_sdk/shared/markdown_parser.py index 13bef2d8..6712177b 100644 --- a/src/chat_sdk/shared/markdown_parser.py +++ b/src/chat_sdk/shared/markdown_parser.py @@ -312,6 +312,8 @@ def _restore_escapes_as_literal_pair(text: str) -> str: # future surface ever feeds untrusted markdown with adversarial # bracket counts, switch to a character-level walker for link/image # content (the rest of `_parse_inline` is bounded by message size). +_LINK_LABEL_ATOM = r"[^\[\]﷐`]|﷐.|`[^`]*`|`(?![^`]*`)" + _INLINE_PATTERNS = [ # Images: ![alt](url) or ![alt](url "title") ("image", re.compile(r'(? str: # pairs (``\(`` / ``\)``) are consumed whole, so they never balance. # A nested ``[...]`` directly followed by ``(`` is an inner link, and links # never nest: ``[[a](u) b](v)`` is the inner link with literal brackets. + # Code spans bind tighter than link brackets, so a closed `` `...` `` in + # the label is opaque (``[`[`](u)`` is a link); a backtick with no closer + # is literal. The alternatives are mutually exclusive (no backtracking). ( "link", re.compile( - r"(? list[Conten # Patterns used by the block parser _HEADING_RE = re.compile(r"^(#{1,6})\s+(.*)") # CommonMark: three or more of the *same* marker (``-_-`` is text, not a break). -_THEMATIC_BREAK_RE = re.compile(r"^([-*_])(?:[ \t]*\1){2,}[ \t]*$") +# Lines are split on ``\n`` only, so a CRLF line keeps a trailing ``\r``. +_THEMATIC_BREAK_RE = re.compile(r"^([-*_])(?:[ \t]*\1){2,}[ \t]*\r?$") # CommonMark: a backtick fence's info string may not contain a backtick, so # "```npm test```" on one line is a (code span) paragraph, not a fence. _FENCED_CODE_START_RE = re.compile(r"^(`{3,}(?=[^`]*$)|~{3,})(.*)") diff --git a/tests/test_markdown_faithful.py b/tests/test_markdown_faithful.py index 619f49c5..9afb87d9 100644 --- a/tests/test_markdown_faithful.py +++ b/tests/test_markdown_faithful.py @@ -1522,6 +1522,10 @@ class TestCommonMarkRulesExposedBySlackText: # Links never nest: the inner link wins, outer brackets stay text. ("[[a](https://x.com) b](https://y.com)", "[a b](https://y.com)"), ("[a [b](https://x.com)](https://y.com)", "[a b](https://y.com)"), + # Code spans bind tighter than link brackets. + ("[`[`](https://example.com)", "["), + ("[a `]` b](https://e.com)", "a ] b"), + ("[`a` [b] `c`](https://u.com)", "a [b] c"), # A thematic break repeats one marker; mixed markers are text. ("-_-", "-_-"), ("ok\n-_-", "ok\n-_-"), @@ -1543,6 +1547,16 @@ class TestCommonMarkRulesExposedBySlackText: def test_plain_text_matches_remark(self, markdown: str, expected: str): assert ast_to_plain_text(parse_markdown(markdown)) == expected + def test_code_span_with_a_bracket_stays_inside_its_link(self): + para = parse_markdown("[`[`](https://example.com)")["children"][0] + assert para["children"] == [ + {"type": "link", "url": "https://example.com", "children": [{"type": "inlineCode", "value": "["}]} + ] + + def test_crlf_thematic_break_is_still_a_break(self): + types = [c["type"] for c in parse_markdown("before\r\n***\r\nafter")["children"]] + assert types == ["paragraph", "thematicBreak", "paragraph"] + def test_nested_link_label_keeps_only_the_inner_link(self): para = parse_markdown("[[a](https://x.com) b](https://y.com)")["children"][0] links = [c for c in para["children"] if c.get("type") == "link"]