docs(sync): record Slack DM empty-thread_ts stream() divergence (#94)#107
Conversation
The empty threadTs for top-level DMs is intentional upstream behavior (matches openDM subscriptions), not a bug. The real defect was upstream stream() handing that legitimate value to chat.startStream, which rejects it. The Python post_message degradation (#94) is a Python-side fix ahead of upstream; document it in the non-parity table so the next sync does not silently revert it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a single documentation entry to the ChangesSlack stream() empty thread_ts parity
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates docs/UPSTREAM_SYNC.md to document a divergence in the Slack adapter's stream() behavior when replying to a top-level DM with an empty thread_ts. The Python implementation normalizes this empty thread ID to None and falls back to a single accumulated post_message call to prevent Slack from rejecting the request with an invalid_thread_ts error. There are no review comments, and I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
) Upstream handleBlockActions falls back to messageTs even for DMs (phantom thread); we empty-case DMs to match upstream's own handleMessageEvent convention (index.ts:2158). Documented per the project divergence rule; cf. PR #107.
…) (#137) * fix(slack): don't thread block-action responses in DMs _handle_block_actions fell back to the clicked message's ts for thread_ts in DMs, so HITL approval result cards posted via event.thread.post became phantom "1 reply" threads in the DM. Mirror _handle_message_event's DM handling: keep a real in-DM thread_ts but never fall back to message_ts for a top-level DM click. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(slack): restore thread_ts str-coalesce + drop committed .DS_Store Address two defects from local review of the DM block-action threading fix: 1. pyrefly bad-argument-type regression: the block-action handler replaced the established `thread_ts or message_ts or ""` coalesce with bare `thread_ts`, letting `str | None` flow into SlackThreadId(thread_ts: str). Restore the coalesce as `thread_ts or ""`, which both preserves the DM empty-thread_ts intent and guarantees `str` (thread_ts already folds in the message_ts fallback for channels). pyrefly src/ back to 0 errors. 2. Remove three committed .DS_Store binaries (./, src/, src/chat_sdk/) and add .DS_Store to .gitignore so they cannot return. Also add a channel-path counterpart test (test_channel_block_action_threads_under_clicked_message) so the DM-only guard is proven not to leak into channels, alongside the existing DM regression test. * fix(slack): use explicit None-coalesce for DM thread_ts at encode time Per CLAUDE.md port rule, prefer 'x if x is not None else default' over 'x or default'. Functionally identical here (only falsy value is the intended empty string) and keeps the SlackThreadId(thread_ts: str) guarantee that pyrefly checks. * docs(sync): record Slack DM block-action threading divergence (#133/#137) Upstream handleBlockActions falls back to messageTs even for DMs (phantom thread); we empty-case DMs to match upstream's own handleMessageEvent convention (index.ts:2158). Documented per the project divergence rule; cf. PR #107. --------- Co-authored-by: tony-chinchill <tony@chinchill.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Adds a
Known Non-Parityrow todocs/UPSTREAM_SYNC.mdfor the Slack DM empty-thread_tsstream degradation (issue #94, shipped in 0.4.27).Why
A recent investigation (chinchill-api #1146) characterized the empty
thread_tsfor top-level DMs as an "underlying SDK bug not fixed in v0.4.27." That framing conflates two separate things:_handle_message_eventencodingthreadTs=""for top-level DMs is intentional, faithful to upstream (adapter-slack/src/index.ts:const threadTs = isDM ? event.thread_ts || "" : event.thread_ts || event.tswith the comment "top-level messages use empty threadTs (matches openDM subscriptions)"). Not a bug.stream()handed that legitimate empty value straight tochat.startStream, which rejects it. The Python SDK fixed this in 0.4.27 via thepost_messagedegradation (Slack DM streaming replies are dropped — stream() raises for empty thread_ts but _handle_stream never falls back #94) — and that degradation is a Python-side divergence (upstream 4.26.0stream()has no such fallback).That divergence wasn't recorded in
UPSTREAM_SYNC.md, so the next upstream sync could silently revert it. This PR documents it.Follow-ups (not in this PR)
stream()still has the same latent bug; fixing it there restores parity and lets this divergence row be removed.MultiTenantSlackAdapter.stream()override now duplicates this SDK behavior and can be retired (see Slack DM streaming replies are dropped — stream() raises for empty thread_ts but _handle_stream never falls back #94: "Folding it into the SDK lets that override be deleted").Test plan
Docs-only change. No code or version bump.
Generated by Claude Code
Summary by CodeRabbit