fix: preserve extras and annotations in _send_update()#670
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
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 docstrings
🧪 Generate unit tests (beta)
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 |
mattheworiordan
requested review from
mschristensen,
owenpearson,
ttypic and
zknill
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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? |
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
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).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ttypic
force-pushed
the
fix/preserve-extras-in-send-update
branch
from
fac1fe5 to
c0807ab
Compare
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
Merging this PR as is, without spec changes. The annotation will be ignored anyway, and it’s a very rare use case to have a message with annotations in append (usually we construct a new message for append and use only the serial from publish to retrieve it).
I don’t want to add a special exception for annotations in the spec. I tried, but it looks awkward. It ends up scattered across several parts of the spec and feels unnatural.
I agree with @SimonWoolf that there should be a clear separation in the spec between inbound and outbound messages. This is quite a significant change, so I’m tracking it as an SDK initiative. I’ll create an RFC later outlining the scope and migration process for the SDK.
4 participants
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