fix: preserve extras and annotations in _send_update()#670
fix: preserve extras and annotations in _send_update()#670mattheworiordan wants to merge 3 commits intomainfrom
Conversation
The _send_update() method in both RestChannel and RealtimeChannel reconstructed the Message object without copying extras or annotations from the user-supplied message. This violated RSL15b/RTL32b which require "whatever fields were in the user-supplied Message" to be sent on the wire. Bug was introduced in 1723f5d (REST) and 0b93c10 (Realtime).
WalkthroughPreserves Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/mutable_message_test.py (1)
137-152: This doesn’t cover the regression path forannotations.The bug fixed in this PR was in
Channel._send_update()reconstructing a newMessage, while this test only re-checksMessage.as_dict(). Ifannotationsare dropped during update-message construction again, this test still passes. A focusedupdate_message()regression test forannotationswould close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/mutable_message_test.py` around lines 137 - 152, Add a focused regression test that exercises the Channel._send_update code path (not just Message.as_dict) to ensure annotations survive message reconstruction: create a Message with MessageAnnotations, invoke Channel._send_update (or the public method that triggers it) using that Message, capture the reconstructed/returned Message, and assert its annotations.summary['reaction:distinct.v1'] == {'thumbsup': 5}; reference Message, MessageAnnotations, and Channel._send_update to locate the code to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/mutable_message_test.py`:
- Around line 123-135: The test test_message_extras_none_excluded_from_as_dict
should assert the absence of the 'extras' key rather than allowing a present
None value; update the assertion in that test to explicitly check that 'extras'
is not in msg_dict (referencing the Message instance and its as_dict() output)
so the contract "excluded from output" is enforced for Message.as_dict.
---
Nitpick comments:
In `@test/unit/mutable_message_test.py`:
- Around line 137-152: Add a focused regression test that exercises the
Channel._send_update code path (not just Message.as_dict) to ensure annotations
survive message reconstruction: create a Message with MessageAnnotations, invoke
Channel._send_update (or the public method that triggers it) using that Message,
capture the reconstructed/returned Message, and assert its
annotations.summary['reaction:distinct.v1'] == {'thumbsup': 5}; reference
Message, MessageAnnotations, and Channel._send_update to locate the code to
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afc79b3d-8a38-4e80-b28f-86bfc9a27edb
📒 Files selected for processing (5)
ably/realtime/channel.pyably/rest/channel.pytest/ably/realtime/realtimechannelmutablemessages_test.pytest/ably/rest/restchannelmutablemessages_test.pytest/unit/mutable_message_test.py
This is wrong. See https://ably.com/docs/messages/updates-deletes#update: "When updating a message, any It is however true that we do not do any deep mixins that require parsing/inspecting a customer-provided payload. |
ttypic
left a comment
There was a problem hiding this comment.
We shouldn't send annotations, it only contains summaries, it will be ignored anyway. I looked at the spec, js and other sdk implementations, we serialize annotations field and send it back. We rarely send received messages back, so it's not a problem.
@SimonWoolf what do you think? should we mention in the spec for update, that we shouldn't send annotations or we can filter out annotations filed when we serialize message
I don't think there's much reason to have update treated any differently from publish here. In both cases the server will ignore the annotations field of incoming messages similarly to timestamp, connectionId, etc. But the spec could certainly say that the sdk shouldn't bother to serialize those fields when it publishes/updates a message (and in general I think it would be good if the spec had different types for outgoing and incoming messages rather than use one for both, so it could be clearer what fields are allowed & required in each, I think there's an issue for that) |
|
I agree we should remove annotations from @SimonWoolf / @ttypic will you update the spec and share the update so I can reflect the changes here? I assume we will need to update JS / others as well? |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/mutable_message_test.py (1)
99-121: Redundant nested assertion.Line 120 is redundant given the full equality check on line 119. If
msg_dict['extras'] == extraspasses, the nested value is implicitly verified.Suggested simplification
msg_dict = message.as_dict() assert msg_dict['extras'] == extras - assert msg_dict['extras']['headers']['status'] == 'complete'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/mutable_message_test.py` around lines 99 - 121, The test_message_extras_preserved_in_as_dict test contains a redundant nested assertion: after asserting msg_dict['extras'] == extras, the subsequent assert msg_dict['extras']['headers']['status'] == 'complete' is unnecessary; remove the nested assertion to simplify the test and keep only the full equality check in the test function test_message_extras_preserved_in_as_dict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/mutable_message_test.py`:
- Around line 99-121: The test_message_extras_preserved_in_as_dict test contains
a redundant nested assertion: after asserting msg_dict['extras'] == extras, the
subsequent assert msg_dict['extras']['headers']['status'] == 'complete' is
unnecessary; remove the nested assertion to simplify the test and keep only the
full equality check in the test function
test_message_extras_preserved_in_as_dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c719e75-1426-435b-aa3e-da0b2657d51b
📒 Files selected for processing (1)
test/unit/mutable_message_test.py
Summary
_send_update()whereextrasandannotationswere silently dropped when callingupdate_message(),delete_message(), orappend_message()on both REST and Realtime channels.extrasandannotations.1723f5d(REST, Jan 13 2026) and0b93c10(Realtime, Jan 15 2026).Why this matters
This is essential functionality for AI Transport use cases. A key pattern for AI token streaming is signaling message completion via
extras, e.g.:Without this fix, the
extrasfield is silently dropped — subscribers never receive the completion signal. This blocks AI Transport demos and production patterns that rely onextrasmetadata on update/append/delete operations.Changes
Fix (2 lines each):
ably/realtime/channel.py— Passextrasandannotationsthrough in_send_update()ably/rest/channel.py— Same fixTests:
test/unit/mutable_message_test.py) — regression tests for extras/annotations serializationNote: current update semantics are whole-message replacement
Today,
update_message()replaces the entire message — there is no partial/field-level update support. If you send onlyextras, thedatafield will be whatever you provide (or empty). Similarly,append_message()only supports string concatenation of the entiredatafield.For AI Transport use cases, more granular semantics would be valuable — e.g. appending to a specific field within a JSON
databody, or updatingextraswithout replacingdata. Related tickets tracking these improvements:append()to a specific field in a messagemessage.appendthat can skip/close the appendRollupWindowTest plan
uv run --with ruff ruff check— linter cleanuv run --extra dev python -m pytest test/unit/mutable_message_test.py— 11 passeduv run --extra dev python -m pytest test/ably/rest/restchannelmutablemessages_test.py test/ably/realtime/realtimechannelmutablemessages_test.py -k "not encryption"— 42 passed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests