Skip to content

fix: preserve extras and annotations in _send_update()#670

Open
mattheworiordan wants to merge 3 commits intomainfrom
fix/preserve-extras-in-send-update
Open

fix: preserve extras and annotations in _send_update()#670
mattheworiordan wants to merge 3 commits intomainfrom
fix/preserve-extras-in-send-update

Conversation

@mattheworiordan
Copy link
Member

@mattheworiordan mattheworiordan commented Mar 6, 2026

Summary

  • Fixes a spec violation in _send_update() where extras and annotations were silently dropped when calling update_message(), delete_message(), or append_message() on both REST and Realtime channels.
  • The spec (RSL15b, RTL32b) requires "whatever fields were in the user-supplied Message" to be sent on the wire. The implementation was cherry-picking fields and omitting extras and annotations.
  • Bug was introduced in 1723f5d (REST, Jan 13 2026) and 0b93c10 (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.:

await channel.update_message(Message(
    serial=serial,
    extras={"headers": {"status": "complete"}},
))

Without this fix, the extras field is silently dropped — subscribers never receive the completion signal. This blocks AI Transport demos and production patterns that rely on extras metadata on update/append/delete operations.

Changes

Fix (2 lines each):

  • ably/realtime/channel.py — Pass extras and annotations through in _send_update()
  • ably/rest/channel.py — Same fix

Tests:

  • 3 new unit tests (test/unit/mutable_message_test.py) — regression tests for extras/annotations serialization
  • 1 new REST integration test — end-to-end: publish → update with extras → verify extras in history
  • 1 new Realtime integration test — end-to-end: publish → update with extras → verify extras via subscription
  • All tests reference spec items (RSL15b, RTL32b, TM2i, TM2u)

Note: 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 only extras, the data field will be whatever you provide (or empty). Similarly, append_message() only supports string concatenation of the entire data field.

For AI Transport use cases, more granular semantics would be valuable — e.g. appending to a specific field within a JSON data body, or updating extras without replacing data. Related tickets tracking these improvements:

  • AIT-532 — Support append() to a specific field in a message
  • AIT-477 — Define a mechanism to mark a message as completed when being appended to
  • AIT-505 — Well-known "end" header on message.append that can skip/close the appendRollupWindow

Test plan

  • uv run --with ruff ruff check — linter clean
  • uv run --extra dev python -m pytest test/unit/mutable_message_test.py — 11 passed
  • uv 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

    • Message updates now preserve extras and annotations metadata when modifying published messages in both realtime and REST channels, ensuring metadata consistency across the message lifecycle.
  • Tests

    • Added coverage validating extras and annotations are preserved during message updates and in message serialization for realtime, REST, and unit-level behaviors.

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).
@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

Walkthrough

Preserves extras and annotations when sending message updates for both realtime and REST channels; adds unit and integration tests to verify serialization and event propagation of these fields.

Changes

Cohort / File(s) Summary
Realtime channel update
ably/realtime/channel.py
_send_update now copies extras and annotations from the original message into the constructed update message.
REST channel update
ably/rest/channel.py
_send_update now includes extras and annotations when building the update payload sent to the REST API.
Realtime integration test
test/ably/realtime/realtimechannelmutablemessages_test.py
Added test_update_message_preserves_extras to publish, update (with extras), and assert received MESSAGE_UPDATE preserves extras.
REST integration test
test/ably/rest/restchannelmutablemessages_test.py
Added test_update_message_preserves_extras to publish and update a message with extras, then assert the updated message and MESSAGE_UPDATE event contain the extras.
Unit tests for message serialization
test/unit/mutable_message_test.py
Added tests verifying as_dict() preserves extras and annotations, and omits extras when None.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

"I nibble on extras, annotations in sight,
Hopped through the code to keep them polite,
Updates now carry each tiny delight,
Tests clap their paws in the soft evening light. 🐇"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: preserving extras and annotations in the _send_update() method across REST and Realtime channels, which is the core fix addressed in all code changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/preserve-extras-in-send-update

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/unit/mutable_message_test.py (1)

137-152: This doesn’t cover the regression path for annotations.

The bug fixed in this PR was in Channel._send_update() reconstructing a new Message, while this test only re-checks Message.as_dict(). If annotations are dropped during update-message construction again, this test still passes. A focused update_message() regression test for annotations would 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

📥 Commits

Reviewing files that changed from the base of the PR and between 324a9f6 and ecd1dd4.

📒 Files selected for processing (5)
  • ably/realtime/channel.py
  • ably/rest/channel.py
  • test/ably/realtime/realtimechannelmutablemessages_test.py
  • test/ably/rest/restchannelmutablemessages_test.py
  • test/unit/mutable_message_test.py

@SimonWoolf
Copy link
Member

Today, update_message() replaces the entire message — there is no partial/field-level update support. If you send only extras, the data field will be whatever you provide (or empty). ... more granular semantics would be valuable — e.g. ... or updating extras without replacing data

This is wrong. See https://ably.com/docs/messages/updates-deletes#update: "When updating a message, any data, name, and extras you specify in the update will replace the corresponding fields in the existing message. Any you leave out remain as they were, so you get a shallow mixin. For example, if a message has { name: "greeting", data: "hello" }, and you update it with { data: "hi" }, the result will be { name: "greeting", data: "hi" }.". You can absolutely update the payload without affecting extras or vice versa, in all of update, delete, and append.

It is however true that we do not do any deep mixins that require parsing/inspecting a customer-provided payload.

@github-actions github-actions bot temporarily deployed to staging/pull/670/features March 9, 2026 09:46 Inactive
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SimonWoolf
Copy link
Member

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)

@mattheworiordan
Copy link
Member Author

I agree we should remove annotations from _send_update(), however the the spec should be updated to explicitly exclude it. For now this in accordance with the the current spec wording and JS SDK behaviour.

@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>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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'] == extras passes, 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

📥 Commits

Reviewing files that changed from the base of the PR and between ecd1dd4 and fac1fe5.

📒 Files selected for processing (1)
  • test/unit/mutable_message_test.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants