fix(slack): surface files_upload_v2 confirmation in SentMessage.raw#117
Conversation
SlackAdapter._upload_files already returns the list of Slack-confirmed file IDs, but post_message discarded the return and ThreadImpl._create_sent_message hardcoded raw=None, so the confirmation never reached SentMessage.raw. Slack was the only file-capable adapter to drop upload confirmation; discord/telegram upload inline and expose the platform response naturally. - post_message: capture _upload_files' return and augment RawMessage.raw with "uploaded_file_ids" on every return path that can carry files (file-only, card, table, text), via a new _augment_raw_with_uploads helper. None means no upload; empty list means Slack confirmed zero attachments. raw is augmented, not replaced — non-breaking. - thread.py: _create_sent_message accepts and propagates a raw parameter; the post() call site passes raw_msg.raw through. Platform-agnostic, so discord/telegram real platform responses now flow through too. Tests: 3 in test_slack_api.py (file-only, text+files, text-only no-augment) and 1 end-to-end in test_thread_faithful.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR captures Slack-uploaded file IDs in the Slack adapter, augments adapter RawMessage.raw with that data when present, propagates the adapter raw through ThreadImpl into SentMessage.raw, updates message-history persistence to null raw, and adds tests and changelog/version bump. ChangesUpload confirmation metadata propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 introduces a fix to surface Slack-confirmed file IDs through the post() method. Previously, SlackAdapter._upload_files computed these IDs but they were discarded, and ThreadImpl._create_sent_message hardcoded raw=None. The changes update post_message in the Slack adapter to augment RawMessage.raw with "uploaded_file_ids", and update ThreadImpl._create_sent_message to accept and propagate the adapter's raw payload into SentMessage.raw. Corresponding unit and integration tests have been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/chat_sdk/thread.py (1)
1125-1136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
SentMessage.rawacrosssent.edit()calls.
post()now correctly setsraw, but_edit()rebuilds a newSentMessagewithout forwarding it, sorawis lost after the first edit.Suggested fix
async def _edit(new_content: Any) -> SentMessage: await adapter.edit_message(thread_id, message_id, new_content) - return thread_impl._create_sent_message(message_id, new_content) + return thread_impl._create_sent_message(message_id, new_content, thread_id, raw=raw)Also applies to: 1164-1164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/thread.py` around lines 1125 - 1136, The edit helper _edit currently rebuilds a SentMessage via thread_impl._create_sent_message without preserving the original SentMessage.raw, so ensure the raw payload is forwarded: capture the existing raw value from the surrounding scope (the raw parameter) and pass it into thread_impl._create_sent_message when returning the edited SentMessage in _edit; apply the same change to the other edit helper at the second occurrence (around the _edit at 1164) so SentMessage.raw is preserved across sent.edit() calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/chat_sdk/thread.py`:
- Around line 1125-1136: The edit helper _edit currently rebuilds a SentMessage
via thread_impl._create_sent_message without preserving the original
SentMessage.raw, so ensure the raw payload is forwarded: capture the existing
raw value from the surrounding scope (the raw parameter) and pass it into
thread_impl._create_sent_message when returning the edited SentMessage in _edit;
apply the same change to the other edit helper at the second occurrence (around
the _edit at 1164) so SentMessage.raw is preserved across sent.edit() calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e086b0a5-d494-4e60-8f66-d5d31f9b1435
📒 Files selected for processing (6)
CHANGELOG.mdpyproject.tomlsrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/thread.pytests/test_slack_api.pytests/test_thread_faithful.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4ad7267ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| files = extract_files(message) | ||
| if files: | ||
| await self._upload_files(files, channel, thread_ts or None) | ||
| uploaded_file_ids = await self._upload_files(files, channel, thread_ts or None) |
There was a problem hiding this comment.
When this new path surfaces _upload_files()'s return value, real slack_sdk uploads will usually report uploaded_file_ids: []: the SDK's files_upload_v2 returns the files.completeUploadExternal response and puts uploaded files at completion["files"][i]["id"] (plus completion.data["file"] for a singleton), while _upload_files() only scans a nested uploaded["files"] list at lines 4069-4070. In production Slack uploads, consumers gating on this confirmation will see “zero attachments confirmed” even after successful uploads.
Useful? React with 👍 / 👎.
Post-merge review of PR #117 caught that _MessageHistoryCache.append in chat.py did not null out raw before persisting (unlike its sibling MessageHistoryCache.append in message_history.py, which already does). Without this, the platform raw payload (Slack team_id/user_id, Discord guild IDs, etc.) that #117 now correctly populates on SentMessage.raw would persist to Redis/Postgres-backed state on every reply, inflating storage size and PII surface. Mirrors the null-out in message_history.py:62.
CHANGELOG: - Drop false gchat file-delivery bullet (PR #112 never merged; upstream itself does not implement gchat outbound file delivery) - Fix Slack DM block-action citation to merged PR #137 (supersedes #133) - Re-point 0.4.27.1 vehicle from unmerged #120 to tag v0.4.27.1 + #117 - Drop numberless Twilio 'scaffolding PR' (only #142 exists) UPSTREAM_SYNC: - Reframe Google Chat outbound file delivery as parity (upstream also logs 'not yet supported' + media.upload TODO, index.ts:1282-1289) - Correct Teams cert-auth row: TS is not 'Supported' — config throws at startup in both SDKs (types.ts:31, config.ts:13); parity, issue #58 - Add Messenger get_user row: upstream has no getUser; raising stub is parity, Graph-API impl tracked as #132
…PR 4/4) (#146) * chore(release): cut 0.4.30 — Teams SDK migration + 4.30.0 parity (#93 PR 4/4) Final PR of the Teams SDK migration (issue #93). PRs 1-3 (inbound+auth, outbound, native streaming) are merged; this cuts the 0.4.30 release. - Version bump: pyproject 0.4.29 -> 0.4.30; UPSTREAM_PARITY "4.29.0" -> "4.30.0". - Fidelity re-pin chat@4.29.0 -> chat@4.30.0 in lint.yml + verify_test_fidelity.py (docstring, default parity fallback, clone hint). packages/chat/src is byte-for-byte identical between the two tags, so zero new test ports: strict fidelity stays 100% (732/732, 0 missing) against chat@4.30.0. Baseline regenerated (ts_parity -> chat@4.30.0; the recorded total_ts_tests literal 731 -> 732 corrects a stale count from the merged adapter waves, not the re-pin — the count is identical against both tags). - Docs: project-instructions version map + fidelity pin; README status line; CHANGELOG 0.4.30 entry (Twilio adapter, Telegram streaming, Slack subpaths, WhatsApp/Slack/gchat fixes, Teams #93 PRs 1-4); UPSTREAM_SYNC.md parity header + the Teams deferral row flipped to delivered. - Version-label normalization: malformed `adapter-teams@chat@4.30.0` and loose `adapter-teams@4.30.0` -> `@chat-adapter/teams@4.30.0` in adapter.py (5), bridge.py (1), UPSTREAM_SYNC.md (4). Comment/doc-only. Does NOT tag/publish — the release is a separate maintainer-gated step (live Teams 429 streaming check + PyPI authorization). * docs(release): clarify 0.4.30 core-parity wording + finish label normalization Review fast-follow for PR 4. The CHANGELOG now states the mapped core is content-identical *between the chat@4.29.0 and chat@4.30.0 upstream tags* (verified: thread.ts/types.ts/thread.test.ts and the full packages/chat/src tree are byte-identical) — the prior 'unchanged from 4.29.0' phrasing was accurate but misread as a claim about our code. Also sweeps the two remaining old-style `adapter-teams@chat@4.30.0` labels in the Teams test docstrings to the canonical `@chat-adapter/teams@4.30.0` npm tag, completing the normalization the PR's scope called for. No logic change. * docs(release): document 0.4.30 parity-audit wave + remaining exceptions CHANGELOG: add 'Pre-existing parity gaps closed (4.30 audit)' subsection covering the 7 ported fixes (PRs #147-#150) and a documented-exceptions note (Linear agent-sessions #151 deferred to 4.31 / #152; adapter-web and the GitHub/Linear native-client + message.subject halves stay Known Non-Parity). UPSTREAM_SYNC: - file-mapping table: add state-ioredis -> redis.py (IoRedisStateAdapter) and state-pg -> postgres.py rows (both ported + tested, were missing). - Known Non-Parity (platform gaps): add Linear agent-sessions row (#151) and an adapter-web row that splits the portable server-side WebAdapter (deferred) from the genuinely browser-only client subpaths, correcting the earlier over-broad 'browser-only; no Python runtime' note. - Correct the Google Chat file-uploads row: inbound attachment parsing is fully implemented (_create_attachment), so the gap is outbound file delivery only (post_message still logs 'not yet supported'). * docs(release): correct phantom-feature framing before PyPI cut CHANGELOG: - Drop false gchat file-delivery bullet (PR #112 never merged; upstream itself does not implement gchat outbound file delivery) - Fix Slack DM block-action citation to merged PR #137 (supersedes #133) - Re-point 0.4.27.1 vehicle from unmerged #120 to tag v0.4.27.1 + #117 - Drop numberless Twilio 'scaffolding PR' (only #142 exists) UPSTREAM_SYNC: - Reframe Google Chat outbound file delivery as parity (upstream also logs 'not yet supported' + media.upload TODO, index.ts:1282-1289) - Correct Teams cert-auth row: TS is not 'Supported' — config throws at startup in both SDKs (types.ts:31, config.ts:13); parity, issue #58 - Add Messenger get_user row: upstream has no getUser; raising stub is parity, Graph-API impl tracked as #132 * docs(changelog): clarify 0.4.30 audit-gap count (8 closed + 1 deferred = 9)
2 participants
Problem — consistency gap
Slack is the only file-capable adapter that discards upload confirmation.
SlackAdapter._upload_filescallsfiles_upload_v2separately and already computes the list of Slack-confirmed file IDs, butpost_messagethrew that return away, andThreadImpl._create_sent_messagehardcodedraw=None— so the confirmation never reached theSentMessagereturned to callers.discord and telegram don't have this gap: they upload inline, so their
RawMessage.rawis the real platform response and flows through naturally.SentMessagealready has arawfield; it was simply never populated on thepost()path.Changes
src/chat_sdk/adapters/slack/adapter.py—post_message:_upload_files' return (uploaded_file_ids)._augment_raw_with_uploadshelper adds an"uploaded_file_ids"key toRawMessage.rawon every return path that can carry files (file-only early return, card, table, text). The Slack response dict is augmented, never replaced — Slack never returns that key, so it's additive and non-breaking.None= no upload occurred; an empty list = Slack confirmed zero attachments (a real signal, preserved by guarding onis not None).src/chat_sdk/thread.py:_create_sent_messagegains araw: Any = Noneparameter and setsraw=rawon the returnedSentMessage(was hardcodedNone).post()call site passesraw_msg.rawthrough. Platform-agnostic — discord/telegram real platform responses now flow through toSentMessage.rawtoo, which is the intended use of the field.Why now
Unblocks a chinchill consumer that needs to gate UX on actual Slack delivery success (how many files Slack confirmed attaching), not just how many were requested.
An upstream
vercel/chatissue is being filed in parallel for convergence (same gap exists inpackages/adapter-slack/src/index.ts).Test plan
uv run pytest tests/ -q→ 4046 passed, 3 skipped (was ~4042; +4 new).tests/test_slack_api.py: file-only post surfaces confirmed IDs; text+files augments thechat_postMessageraw; text-only does not add the key.tests/test_thread_faithful.py: end-to-endpost()propagatesRawMessage.raw→SentMessage.raw.uv run ruff format+uv run ruff checkclean.0.4.29a1→0.4.29a2; CHANGELOG entry added.🤖 Generated with Claude Code
Summary by CodeRabbit
Release
Bug Fixes
Tests