Python: add agent-framework-hosting-activity-protocol channel#6758
Python: add agent-framework-hosting-activity-protocol channel#6758eavanvalkenburg wants to merge 2 commits into
Conversation
Introduce the Activity Protocol (Bot Framework) hosting channel as a new alpha package. The channel bridges Bot Framework activities to a hosted agent, with multimodal streaming accumulation (iterating update.contents rather than text-only), Entra credential support, and command parsing. - New package packages/hosting-activity-protocol with channel, README, LICENSE and tests in a unique nested test directory (no tests/__init__.py). - Register the package in the uv workspace (pyproject sources, uv.lock) and PACKAGE_STATUS.md (alpha). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Adds a new Python hosting channel package, agent-framework-hosting-activity-protocol, implementing the Bot Framework Activity Protocol to let agent-framework-hosting targets be reachable via Azure Bot Service (Teams/Slack/Webex/etc.) without per-channel protocol implementations.
Changes:
- Introduces
ActivityProtocolChannel(webhook handling, outbound replies, progressive streaming viaupdateActivitywhen supported, typing indicators, allow-listingserviceUrl, and optional Entra client-secret/cert credentials). - Adds a dedicated unit test suite under a unique nested tests directory.
- Registers the new package in the Python workspace (
pyproject.toml,uv.lock) and marks it asalphainPACKAGE_STATUS.md.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| python/uv.lock | Adds the new package to the workspace lock and dependency graph. |
| python/pyproject.toml | Registers agent-framework-hosting-activity-protocol as a workspace dependency. |
| python/PACKAGE_STATUS.md | Adds an alpha status row for the new package. |
| python/packages/hosting-activity-protocol/pyproject.toml | New package metadata + tooling configuration for tests/typing/linting. |
| python/packages/hosting-activity-protocol/README.md | Package overview and usage snippet for wiring the channel into a host. |
| python/packages/hosting-activity-protocol/LICENSE | MIT license for the new package. |
| python/packages/hosting-activity-protocol/agent_framework_hosting_activity_protocol/init.py | Exposes the channel and isolation-key helper from the package. |
| python/packages/hosting-activity-protocol/agent_framework_hosting_activity_protocol/_channel.py | Core channel implementation: webhook parsing, command dispatch, outbound REST calls, streaming/edit logic, and auth handling. |
| python/packages/hosting-activity-protocol/tests/hosting_activity_protocol/test_channel.py | Unit tests covering parsing, allow-listing, auth header behavior, command behavior, and streaming fallbacks. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 90%
✓ Correctness
Thorough review of the new
agent-framework-hosting-activity-protocolpackage. The implementation is well-structured and handles edge cases carefully. Route mounting follows the established pattern (Telegram channel uses identicalRoute("/", ...)convention). Framework types (ChannelRequest,ChannelIdentity,ChannelSession,ChannelContribution,Content,AgentResponseUpdate) are all used correctly per their definitions in the hosting and core packages. The streaming concurrency model (single-threaded asyncio withasyncio.Eventcoordination) is sound — no data races or deadlocks. The service URL allow-list is a good security measure. Error handling is thoughtful, distinguishing transient outbound failures (502/retry) from deterministic agent failures (200/no-retry). No high-severity correctness bugs found.
✓ Security Reliability
The new Activity Protocol channel is well-structured with good defensive measures (service URL allow-list, inbound auth validator hook, clear transient-vs-deterministic failure separation). The primary security finding is that the
_is_service_url_allowedcheck validates only the hostname — not the URL scheme — so an attacker-controlled inbound activity withhttp://(plain HTTP) as theserviceUrlwould pass the allow-list, and the channel would POST the bearer token unencrypted. The official Bot Framework SDK enforces HTTPS for service URLs as defense-in-depth; this channel should do the same. A secondary concern is thatconversation_idis interpolated into URL paths without percent-encoding, which could allow path traversal to unintended API endpoints on the same allowed host.
✓ Test Coverage
The test suite is solid for the happy paths and key error-recovery scenarios (placeholder failure fallback, 405 edit fallback, service URL allow-list, inbound auth, retry signalling). However, the PR's stated focus area — multimodal streaming accumulation — lacks a dedicated test: every streaming test provides updates with a single text-only Content item, so the non-text filtering logic (
content.type == "text") in_stream_to_conversationand_buffer_and_sendis never exercised with mixed-content updates. This is the primary coverage gap.
✓ Failure Modes
Two concrete failure-mode issues in the new Activity Protocol channel: (1)
_get_token()credential failures fromazure.identityare nothttpx.HTTPErroror_OutboundError, so_handlemisclassifies them as deterministic (returns 200 instead of 502), preventing Bot Service retry on transient token-acquisition errors; (2) all streaming-path final sends swallow exceptions (including transienthttpx.HTTPError) via bareexcept Exception, so outbound failures that the non-streaming path correctly surfaces as 502 are silently lost. The_OutboundErrormarker class exists but is never raised anywhere.
✓ Design Approach
I found two design-level correctness issues in the new Activity Protocol channel. First, the command path bypasses the channel run hook even though the new contract and docstring explicitly promise that commands observe the same hook-resolved request/session state as normal messages. Second, the streaming delivery path swallows transient outbound POST failures on final send, some Bot Framework channels will get a 200 instead of the intended 502 retry signal when delivery fails after the agent has already produced output.
Automated review by eavanvalkenburg's agents
- Validate the webhook JSON body is a mapping (return 400 instead of 500); guard non-mapping `recipient`/`conversation` in untrusted activities. - Wrap `azure.identity` token-acquisition errors as `_OutboundError` so `_handle` returns 502 (retry) rather than dropping the activity. - Enforce https on serviceUrl when a credential is configured, so a real bearer token is never POSTed over plaintext http (dev mode still allows http for the Bot Framework Emulator). - Re-raise transient outbound failures on the final buffered/fallback send so non-edit-channel streaming honours the 502 retry contract. - Fix `_invoke_command` docstring: commands bypass the agent and run_hook (matching the Telegram channel). - Fix README usage snippet (`app_password`, not `client_secret`). - Drop the per-package `[tool.mypy]` config and `mypy` poe task to match the other hosting channel packages (repo-wide typing only). - Add tests for the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation & Context
Adds the Activity Protocol (Bot Framework) channel for the
agent-framework-hostingstack, so the same hosted agent can be reached through Azure Bot Service — Microsoft Teams, Slack, Webex, and any other channel Bot Service fronts — without implementing each channel's native protocol.This is part of the channel-by-channel split of the hosting work tracked in #6265.
Description & Review Guide
What are the major changes?
agent-framework-hosting-activity-protocolpackage withActivityProtocolChannel:messageactivities, outbound replies, and mid-streamupdateActivityedits (progressive streaming) with typing indicators.update.contentsfor text parts instead of reading a singletextattribute, so multi-content updates render correctly.run_hookfor per-channel customisation.[tool.uv.sources]entry,uv.lockstanza, and aPACKAGE_STATUS.mdrow (alpha).tests/hosting_activity_protocol/) with notests/__init__.py, per the test-collection convention.What is the impact of these changes?
What do you want reviewers to focus on?
_stream_to_conversation/_buffer_and_send, and the credential/dev-mode auth branches.Related Issue
Fixes #6589
Refs #6265
Contribution Checklist