_ChannelIdFieldMixin removal - #490
Conversation
_ChannelIdFieldMixin removal
There was a problem hiding this comment.
Pull request overview
This pull request refactors channel ID handling across the Microsoft 365 Agents SDK for Python to remove the _ChannelIdFieldMixin and centralize parsing/normalization in the ChannelId type, aiming to simplify model definitions while keeping Bot Framework-compatible serialization behavior.
Changes:
- Removed
_ChannelIdFieldMixinand migratedActivity/ConversationReferenceto explicitchannel_id: Optional[ChannelId]fields. - Enhanced
ChannelIdwith constructor normalization and new utility methods (get_channel,get_sub_channel) and updated core usages to rely on them. - Updated affected tests and internal call sites (e.g., token client, adapter activity creation) to work with the refactor.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/activity/test_channel_id.py | Adds coverage for ChannelId constructor behavior (instance reuse). |
| tests/activity/pydantic/test_channel_id_field_mixin.py | Removes mixin-specific tests after mixin deletion. |
| tests/activity/pydantic/test_activity_io.py | Adjusts Activity IO tests to align with the new channel_id field approach. |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/connector/client/user_token_client.py | Switches base-channel extraction to ChannelId.get_channel. |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/channel_service_adapter.py | Wraps adapter-provided channel_id into ChannelId when building activities. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/conversation_reference.py | Removes mixin inheritance and adds channel_id field typed as ChannelId. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/channel_id.py | Refactors ChannelId construction and introduces get_channel / get_sub_channel. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py | Removes mixin inheritance, adds channel_id field, and updates base-channel extraction logic. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/_channel_id_field_mixin.py | Deletes the mixin implementation. |
| libraries/microsoft-agents-activity/microsoft_agents/activity/init.py | Removes _ChannelIdFieldMixin exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py:156
Activity.channel_idis now a plain field, butAgentsModeldoes not enable assignment validation. Existing code/tests setactivity.channel_id = "msteams:..."after initialization, which will leave a rawstron the model;_serialize_sub_channel_datathen unconditionally accesses.sub_channeland will raiseAttributeErrorduringmodel_dump(_json). Enabling assignment validation onActivityrestores the previous behavior of coercing strings intoChannelId.
type: NonEmptyString
channel_id: Optional[ChannelId] = None
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
libraries/microsoft-agents-activity/microsoft_agents/activity/channel_id.py:124
- ChannelId.get_channel() does not strip whitespace when the input has no ':' (e.g. " msteams "), because it only strips the split result when a colon is present. This contradicts the new unit test expectations and can leak unnormalized channel IDs to callers.
if not channel_id or not channel_id.strip():
return channel_id
if isinstance(channel_id, ChannelId):
return channel_id.channel
libraries/microsoft-agents-activity/microsoft_agents/activity/channel_id.py:114
- ChannelId.get_sub_channel() returns the raw ChannelId.sub_channel when given a ChannelId instance. For values like "msteams:", ChannelId.sub_channel can be "" (empty string), so get_sub_channel(ChannelId("msteams:")) returns "" instead of None, which is inconsistent with get_sub_channel("msteams:") and the method contract.
if not channel_id or not channel_id.strip():
return None
if isinstance(channel_id, ChannelId):
return channel_id.sub_channel
value = channel_id.strip()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py:156
Activity.channel_idis now a plain Pydantic field, butAgentsModeldoes not enablevalidate_assignment. This meansactivity.channel_id = "msteams:sub"will leave a rawstron the model, and_serialize_sub_channel_datalater assumes aChannelId(accesses.sub_channel/.channel), which can raise at runtime. Consider re-enabling assignment validation to preserve the previous mixin’s coercion behavior and keep the serializer’s assumptions safe.
type: NonEmptyString
channel_id: Optional[ChannelId] = None
tests/activity/test_channel_id.py:36
- The old
UserToken._base_channel_idtests covered edge cases like":COPILOT"and whitespace inputs. After moving logic toChannelId.get_channel(), those edge cases are no longer covered here. Adding them to this test helps prevent regressions in normalization behavior for odd/malformed channel IDs.
def test_get_channel_strips_and_drops_sub_channel(self):
assert ChannelId.get_channel(" msteams ") == "msteams"
assert ChannelId.get_channel("msteams:sub") == "msteams"
assert ChannelId.get_channel("msteams:") == "msteams"
assert ChannelId.get_channel(None) is None
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
libraries/microsoft-agents-activity/microsoft_agents/activity/channel_id.py:78
- ChannelId._normalize currently returns the original stripped input string as the instance value even when the parsed sub_channel is empty (e.g., "msteams:"), which leaves a trailing ':' in the canonical string value while sub_channel is None. This can lead to serializing "msteams:" without a ProductInfo entity and makes equality/round-trips inconsistent with get_channel/get_sub_channel semantics.
split = value.split(":", 1)
channel = split[0].strip()
if not channel:
raise ValueError(str(activity_errors.ChannelIdValueMustBeNonEmpty))
sub_channel = (split[1].strip() or None) if len(split) == 2 else None
return value, channel, sub_channel
libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py:159
- Activity.channel_id is now a plain Optional[ChannelId] field, but AgentsModel does not enable assignment validation. As a result, common usage like
activity.channel_id = "msteams:sub"will leave a raw str on the model, and later_serialize_sub_channel_datawill crash when it accessesself.channel_id.sub_channel. Restoring setter-style coercion for channel_id avoids this runtime error and keeps backward-compatible assignment behavior.
type: NonEmptyString
channel_id: Optional[ChannelId] = None
id: Optional[NonEmptyString] = None
timestamp: datetime = None
local_timestamp: datetime = None
8cb8e42
into
main
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
libraries/microsoft-agents-activity/microsoft_agents/activity/channel_id.py:78
- ChannelId normalization currently preserves a trailing ':' when the input has no sub-channel (e.g., "msteams:"). This means
ChannelId("msteams:") != "msteams", and any code comparingactivity.channel_id(aChannelId/str) to channel constants like "msteams" will fail even though.channelis "msteams". Consider canonicalizing the stored string value to omit the ':' whensub_channelis empty, so equality checks and serialization are consistent.
split = value.split(":", 1)
channel = split[0].strip()
if not channel:
raise ValueError(str(activity_errors.ChannelIdValueMustBeNonEmpty))
sub_channel = (split[1].strip() or None) if len(split) == 2 else None
return value, channel, sub_channel
libraries/microsoft-agents-activity/microsoft_agents/activity/activity.py:54
Activity._serialize_sub_channel_dataand_validate_channel_idassumeself.channel_idis aChannelIdinstance (accessing.sub_channel/.channel), butAgentsModeldoes not enablevalidate_assignment. With the mixin removed,activity.channel_id = "msteams:copilot-web"will leave a plainstron the model, which can later raiseAttributeErrorduring validation/serialization. To preserve the prior setter behavior and avoid runtime crashes, enable assignment validation forActivity(so str assignments are coerced toChannelId).
class Activity(AgentsModel):
This pull request removes the
_ChannelIdFieldMixinmixin and refactors the handling ofchannel_idthroughout the codebase to use theChannelIdclass directly. It also improves theChannelIdimplementation to provide static methods for extracting the channel and sub-channel, and updates all usages to leverage these utility methods. These changes simplify the code, improve type safety, and centralize channel ID logic.Channel ID Refactoring
_ChannelIdFieldMixinmixin and its usage fromActivityandConversationReferenceclasses;channel_idis now a direct field of typeOptional[ChannelId]in both classes. [1] [2] [3] [4] [5]__init__.pyexports to remove references to_ChannelIdFieldMixin. [1] [2] [3]ChannelId Utilities and Implementation
ChannelIdclass to include static methodsget_channelandget_sub_channelfor extracting the base channel and sub-channel, and refactored its constructor logic for clarity and correctness. [1] [2] [3] [4]Usage Updates
ChannelId.get_channel(), including in theUserTokenClientand related authentication methods. [1] [2] [3] [4] [5] [6] [7] [8]Activity.get_conversation_referenceto useChannelId.get_channel()for extracting the base channel.Cleanup
These changes make channel ID handling more robust and maintainable by consolidating logic into the
ChannelIdclass and simplifying model definitions.