feat(slack): dispatch message_changed / message_deleted to lifecycle handlers (#211)

[patrick-chinchill]

Summary

The Slack adapter now passes message edits and deletes to the on_message_updated / on_message_deleted handlers that #196 added to core. Before this change it dropped message_deleted, and it used message_changed only to cache link unfurls.

Upstream commits mapped

• 4ac04551 feat(chat): add message update and delete lifecycle callbacks (feat(chat): add message update and delete lifecycle callbacks vercel/chat#788, chat@4.37.0). This PR ports the Slack half:
  • message_deleted is no longer an ignored subtype and goes to the new handleMessageDeleted.
  • handleMessageChanged now dispatches edits.
  • Adds threadIdForMessageEvent and parseSlackTimestamp.
• 864d9222 fix(slack): keep alert attachment content (fix(slack): keep alert attachment content on normalized messages vercel/chat#846, chat@4.39.0). Only the handleMessageChanged hunk is ported: previousMessage is a lazy async factory that falls back to the sync parse. The attachment-content part belongs to [4.41/SL5] Slack inbound: pasted tables and alert attachments in message content #210.

What changed (src/chat_sdk/adapters/slack/adapter.py, types.py)

• _thread_id_for_message_event(event): new messages, edits and deletes all use this one helper.
  • DM: thread_ts or "". Anything else: thread_ts or ts or "". Upstream uses || here.
  • The agent_view condition is left as a marked hook for [4.41/SL9] Slack agent_view (Agent messaging experience) + declarative agent config #214.
• message_deleted is routed to _handle_message_deleted.
  • deleted_ts is the first non-None of deleted_ts, message.ts and previous_message.ts.
  • previous_message is normalized: it takes channel / channel_type / team / team_id from the outer event when it lacks them (upstream ??).
  • The thread comes from previous.thread_ts / previous.ts.
  • Calls process_message_deleted(MessageDeletedEvent(...)). deleted_at is UTC from event_ts ?? ts, previous_message is a sync parse and raw is the payload.
• message_changed, in this order:
  • Builds a normalized inner message.
  • Ignores an inner tombstone.
  • Caches unfurls as a side step. The caching moved into _cache_unfurls_from_message_changed, and its early returns are gone.
  • Returns for hidden updates that are not edits and for updates that change nothing.
  • Otherwise calls process_message_updated(self, thread_id, factory, previous_message=<async factory or None>, options=options). The previous-message factory tries _parse_slack_message and falls back to _parse_slack_message_sync with a warning on Exception. CancelledError still propagates.
• _parse_slack_timestamp(ts): returns a tz-aware UTC datetime, or None for missing or non-numeric input.
• SlackEvent gains deleted_ts, event_ts, hidden, message and previous_message.

Tests

All new tests are in tests/test_slack_webhook.py. Upstream adapter-slack/src/index.test.ts is not fidelity-mapped.

Ported from upstream, using the message subtype handling and link unfurl enrichment describes at chat@4.41.1:

• dispatches message_changed subtypes as message updates
• forwards the pre-edit message so handlers can diff the change (async path)
• ignores a message_changed where nothing actually changed
• leaves previousMessage undefined when Slack omits it
• dispatches hidden message_changed edits as message updates
• ignores hidden message_changed thread metadata updates after deletes
• dispatches message_deleted subtypes as message deletes
• ignores message_changed tombstone subtypes
• routes the message, its edit, and its delete to one thread id in a flat DM ([4.41/SL9] Slack agent_view (Agent messaging experience) + declarative agent config #214 adds the agent_view case)
• should not re-dispatch message_changed as a new message (hidden unfurl; also checks that the cache is still written)
• should ignore hidden message_changed without unfurl attachments

Python-specific (TestMessageLifecyclePythonSpecific and one test in TestUnfurlMetadata):

• When the user lookup raises, the previous-message factory falls back to the sync parse and logs a warning. CancelledError propagates.
• _parse_slack_timestamp returns None for None, "", "abc", "nan" and "inf", and a UTC datetime for "1700000000.123".
• An edit that also carries unfurls still writes the unfurl cache and dispatches the update.
• The bot's own message_changed makes no users_info call and no process_message_updated call. A companion test checks that another user's edit still resolves its author.
• Delete fallbacks: the id comes from previous_message.ts with the DM thread rule; with no previous message the deleted_ts path is used and deleted_at is None for a non-numeric event_ts; a delete with no ts at all is dropped.
• Inheritance and precedence: a top-level DM edit whose inner message lacks channel_type inherits it from the outer event (slack:D_DM:); hidden edits dispatch when only edited.ts or only the text changes; hidden: "true" (non-boolean) does not suppress; deleted_ts beats previous_message.ts and event_ts beats ts; team / team_id / channel / type are inherited on the edit and delete payloads (the pre-edit snapshot only gets channel / channel_type / type, as upstream); keys absent from both events are not added to raw.
• End to end through a real Chat: the handlers receive (thread, "after", "before") and a delete event with platform="slack", and on_message never fires.

Updated tests that encoded the old behavior:

• TestIgnoredSubtypes drops message_changed / message_deleted and now also asserts that neither lifecycle processor is called.
• The two TestMessageSubtypes tests (test_message_changed_does_not_route_to_process_message, test_ignores_message_deleted) became the upstream "dispatches …" ports.
• test_message_changed_with_no_unfurls_does_not_write_state is unchanged and still passes.

Fidelity: Delta vs committed report (HEAD): missing 151 -> 151 (+0). Slack adapter tests are not in MAPPING / TARGET_MAPPING, so scripts/fidelity_target.json did not change.

Divergences (1)

• Self-edit short-circuit (non-parity row in docs/UPSTREAM_SYNC.md; breadcrumb in the code; pinned by test_bot_own_edit_skips_user_lookup_and_update_dispatch).
  • What it does: _handle_message_changed returns when _is_message_from_self(normalized) is true, before it builds the parse factory.
  • Why it is safe: core's skip uses author.is_me, which comes from that same function, so handlers see exactly the same calls.
  • What it saves: a users.info lookup, a participant state write, and up to 2 s of unfurl polling for every streamed delta (post+edit and native streaming each emit one message_changed).

previous_message is passed as an async factory, as upstream does, because core (#196) accepts factories. There is no sync-only divergence.

Consumer impact

• Slack now calls on_message_updated / on_message_deleted.
  • Edits: handler(thread, message, previous_message). previous_message is None when Slack omits the snapshot.
  • Deletes: MessageDeletedEvent with channel_id, message_id (the deleted ts), thread_id, deleted_at, previous_message and raw.
• Edits and deletes are still never routed as new messages (on_mention / on_subscribed_message / on_message).
• The bot's own streamed edits never reach the handlers.
• With no handler registered, no handler runs, but each user edit is still parsed and dispatched through core, as upstream does (chat.ts processMessageUpdated resolves the factories without checking for handlers): a cached users.info lookup, a thread-participant state write, up to 2 s of unfurl polling when the text has links, and subscription/identity state reads. Previously a message_changed without unfurls did no I/O. Each delete builds a sync-parsed MessageDeletedEvent (no lookup).
• Inherited fields (channel_type, team, team_id) are only added to Message.raw when the inner or outer event carries them, matching upstream ?? -> undefined (no explicit None keys).
• A message, its edit and its delete resolve to the same thread id.
• A live Slack-loop check of edits and deletes is pending.

Validation

ruff check / format, audit_test_quality.py (0 hard failures), --check-docs, strict fidelity at chat@4.31.0 (all TS tests have Python equivalents), pytest tests/: 6945 passed, 24 skipped (after merging origin/main). pyrefly check: 0 errors.

Review (gpt-6-astra)

• Round 1, one [P2]: the pre-edit snapshot does not inherit team / team_id, so in multi-workspace mode an attachment on a serialized and rehydrated previous_message loses its teamId.
  • Not changed, because upstream does the same: chat@4.41.1 adapter-slack/src/index.ts:3670-3675 builds the snapshot with only channel, channel_type and type.
  • This is not specific to Python: upstream's rehydrateAttachment also needs fetchMetadata.teamId.
  • Added a code comment recording the upstream-parity choice instead of a second divergence.
• Round 2: PASS. No actionable findings.

Merge gate

Final HEAD: 05b6e2e, which includes a clean git merge origin/main.

Findings from the independent reviewers (5):

• Fixed: the CHANGELOG and this PR body said "with no handler registered, behavior is unchanged". Two reviewers reported that this is false. The text now says that with no handler registered, edits are still parsed and dispatched through core, which is what upstream does. No short-circuit was added, because it would be a new divergence.

• Fixed: normalized, the pre-edit snapshot and the delete previous stored an explicit None for channel_type / team / team_id when neither event had them. This is the TS hazard where ?? produces undefined. The new _with_inherited helper adds a key only when its resolved value is not None. test_inherited_keys_absent_from_both_events_are_not_added_to_raw fails without the fix.

• Fixed: no test covered a top-level DM edit whose channel_type is only on the outer event. Added test_top_level_dm_edit_inherits_channel_type_from_the_outer_event.

• Fixed: several mutants survived:

  • the hidden-edit edited.ts and text branches
  • deleted_ts and event_ts precedence
  • hidden is True
  • team, team_id, type and channel inheritance

  Added targeted tests. All 12 mutants the reviewer listed (M5, M5b, M7, M8, M9, M9b, M10, M11, M11b, M12, M19, M22) are now killed.

gpt-6-astra: 1 round on 05b6e2e. PASS, no actionable findings. The rounds on the HEAD before this pass are recorded above.

Bots: CodeRabbit posted only rate-limit and draft-skip notices. No bot comments needed action.

CI: all green on 05b6e2e: Tests 3.12 and 3.13, Lint & Type Check (including strict fidelity and pyrefly), and CodeQL.

Local validation: all green.

• ruff check and format
• audit (0 hard failures)
• --check-docs
• strict fidelity at 4.31.0
• pytest: 6945 passed, 24 skipped
• pyrefly: 0 errors

Closes #211
Part of #184

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 43 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 78cf923d-c284-4cb3-8fdb-6716c1df9d4d

📥 Commits

Reviewing files that changed from the base of the PR and between 4e702c1 and d1cb081.

📒 Files selected for processing (6)

• CHANGELOG.md
• docs/UPSTREAM_SYNC.md
• src/chat_sdk/adapters/slack/adapter.py
• src/chat_sdk/adapters/slack/types.py
• tests/test_slack_extended.py
• tests/test_slack_webhook.py

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

Autopilot is currently an internal CodeRabbit preview.

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[patrick-chinchill]

Merge gate: CI green (test 3.12, test 3.13, Lint & Type Check, CodeQL/Analyze python+actions, CodeRabbit) on d1cb081. Local Codex review (gpt-6-astra, xhigh, --base origin/main) on d1cb081 (merge of main incl. #286 Slack inbound normalization and #287 Teams): "No actionable regressions found against the specified merge base. All 999 Slack and message-lifecycle tests passed, and lint passed for the changed Python files." 1 astra round after the main merge (the prior convergence on 05b6e2e was also clean). Main merged cleanly with no conflicts; local validation green (7076 passed, strict fidelity 733/733, pyrefly 0 errors, fidelity target delta +0). CodeRabbit was rate-limited; no outstanding bot review findings. Merging with --admin (Protect Main requires a code-owner approval).