Channel spec#5549
Conversation
ADR 0026: - Tighten Decision Outcome Summary so each concept is mentioned once; defer full definitions to the Terminology section. - Update ChannelInvocationHook bullet to match the clarified gap microsoft#7 language (uniform ChannelRequest envelope, hook timing, illustrative examples). - Drop Decision Drivers bullets that just restated Business Goals; cross-link to the goals section instead. - Replace the More Information bullet list with a pointer to Non-Goals. Spec 002: - Trim requirement microsoft#21 to point at the canonical LinkPolicy section instead of restating the full contract. - Add a #linkpolicy-and-trust_level subsection anchor for cross-refs. - Trim the Terminology LinkPolicy entry's two-hosts caveat (canonical version stays in the Key Types section). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 93%
✓ Correctness
This is a well-structured design spec for Python hosting channels. Two correctness issues were found in the illustrative code samples: (1) Scenario 10 calls AgentRunInputs.from_text() but AgentRunInputs is a type alias (str | Content | Message | Sequence[...]), not a class — it has no from_text() method; and (2) Scenarios 3 and 8 use inconsistent mutation patterns on ChannelRequest (dataclasses.replace() vs .replace() method). These would mislead implementors.
✓ Security Reliability
No actionable issues found in this dimension.
✓ Test Coverage
No actionable issues found in this dimension.
✓ Design Approach
The ADR is thoughtful overall, but it bakes in the wrong persistence boundary by making session resolution user-scoped via
isolation_key. The existing framework contracts in both .NET and Python treat sessions as conversation/thread state, not user identity, so this approach would collapse multiple concurrent conversations for the same user into one shared history and make later cross-channel/background delivery semantics fragile. I would request changes to separate identity/linking from conversation/session lookup. The spec is directionally strong, but two design choices look fundamentally off. First, it treatsWorkflowas interchangeable withSupportsAgentRunin the host contract even though the current Python workflow runtime keeps mutable state on the workflow instance and has no session parameter, which would leak state across unrelated users/channels if a host fronts a singleton workflow. Second, the cross-channel continuity story is still keyed offChannelSession.keycompatibility, so the host is not actually owning session identity in a channel-neutral way; linked identities will still fragment whenever channels naturally emit different thread/conversation IDs.
Suggestions
- Scenario 10 (line 957):
AgentRunInputs.from_text(payload["text"])will fail —AgentRunInputsis a type alias (str | Content | Message | Sequence[...]), not a class with afrom_text()classmethod. Usepayload["text"]directly (a plainstrsatisfiesAgentRunInputs) orContent.from_text(payload["text"])if you want rich content.
Automated review by eavanvalkenburg's agents
| -> on completion, deliver to ResponseTarget via destination channel.push(...) | ||
| -> RunHandle updated; available via host.get_run(run_id) and channel poll routes | ||
| ``` | ||
|
|
There was a problem hiding this comment.
AgentRunInputs is a type alias (str | Content | Message | Sequence[str | Content | Message]) defined at python/packages/core/agent_framework/_types.py:1764, not a class — calling .from_text() on it would raise AttributeError. Since a plain str satisfies AgentRunInputs, use payload["text"] directly, or use Content.from_text(payload["text"]) if you want structured content.
| input=payload["text"], |
|
|
||
| | Field / Method | Type | Description | | ||
| |---|---|---| | ||
| | `__init__(target, *, channels, middleware=(), identity_resolver=None, identity_linker=None, debug=False)` | constructor | Composes one host from one **hostable target** (`SupportsAgentRun` or `Workflow`) and a sequence of channels. Optional `identity_resolver` and `identity_linker` provide channel-native-id → `isolation_key` mapping and a connect ceremony for linking new channels to existing identities. The host detects the target kind and dispatches to the appropriate runner. | |
There was a problem hiding this comment.
This collapses Workflow into the same hosting contract as agents, but the current workflow runtime is instance-stateful: Workflow.run() has no session parameter and state is preserved across calls. A host fronting one Workflow object would share mutable workflow state across unrelated users/channels. Consider a workflow factory or checkpoint-based runner that creates or restores a workflow per isolation key or run, rather than treating a singleton Workflow instance as a reusable host target.
moonbox3
left a comment
moonbox3
left a comment
There was a problem hiding this comment.
I approached this review in the context of AG-UI...
| 2. a Responses channel by default forwards official request parameters like `temperature` into agent options and maps `store=False` into disabled session use, | ||
| 3. app authors can override that default per request with an run hook that validates or rewrites the final `ChannelRequest` (for example requiring `temperature`, ignoring `store`, or adapting for `A2AAgent`), | ||
| 4. a Telegram-style message channel can express command metadata, command registration, and either webhook or polling lifecycle behavior through the new channel contract, | ||
| 5. a custom webhook/message channel can be authored only against the new channel contract plus the language's web-framework primitives and lifecycle hooks, |
There was a problem hiding this comment.
Three AG-UI inbound fields have no home in this table:
- state (the AG-UI client's bidirectional state object, sent on every request, mutated mid-run via StateSnapshotEvent/StateDeltaEvent).
- tools (frontend tool catalog the client supplies per request; the agent must see the definitions to let the LLM call them, but execution returns to the client).
- forwarded_props (carries resume, command, and HITL responses; today this drives workflow RequestInfo/RequestResponse round-trips in agent_framework_ag_ui._run_common).
The spec implies these would land in attributes: Mapping[str, Any], but attributes is documented as channel-private and is not consumed by the host. If the contract is "AG-UI uses attributes", we should say so explicitly, and pin a key namespace; otherwise add typed slots. Without one of these, an AG-UI channel author has to thread state/tools/forwarded_props outside the contract, which would defeat the purpose of normalizing on ChannelRequest.
| | `ChannelCommandContext` | `session`, `state`, `raw_event`, `reply(...)`, `run(request)` | Runtime context for command handlers. | | ||
| | `CommandHandler` | `Callable[[ChannelCommandContext], Awaitable[None] \| None]` | Command implementation; may reply locally, mutate state, or invoke the agent. | | ||
|
|
||
| **`HostedRunResult` / `HostedStreamResult`** — outbound results from the host. |
There was a problem hiding this comment.
HostedStreamResult.updates: ResponseStream[...] is a stream of AgentRunResponseUpdate. AG-UI's mid-stream events are not those: StateSnapshotEvent, StateDeltaEvent (JSON Patch-shaped), MessagesSnapshotEvent, ToolCallStartEvent/ToolCallArgsEvent/ToolCallResultEvent, plus synthesized state events from predict_state_config (see _run_common.py). To honor those, an AG-UI channel needs raw access to the underlying agent event stream so it can interleave its own protocol events. The current shape forces the channel to either (a) re-derive AG-UI events from AgentRunResponseUpdate (lossy for state) or (b) skip context.run/stream entirely and call the agent directly (which means the host owns nothing useful for AG-UI: no session resolution, no identity, no policy). Can we document which of those is intended? or expose a raw-event seam?
|
|
||
| #### Conversation history for the Responses channel | ||
|
|
||
| The Responses channel does **not** introduce its own history seam. Conversation history for every channel — Responses, Invocations, Telegram, Teams — flows through the agent's standard core `HistoryProvider` (`agent_framework._sessions.HistoryProvider`). The Responses channel is a *caller-supplied session* channel (see [Channel session-carriage models](#channel-session-carriage-models)): it parses `previous_response_id` (and/or `conversation_id`) off the inbound request and projects it into `ChannelSession.key`. The host then resolves an `AgentSession` for that key and the agent's `HistoryProvider` does the load / append exactly as it would for any other session. |
There was a problem hiding this comment.
AG-UI's per-thread state is durable across turns and is not message history. There is no slot for non-message session state in HistoryProvider, and the spec doesn't cover where AG-UI state lives. Two options to consider:
- state is a channel-package concern, stored separately from the message store
- the host introduces a channel-state hook the way HistoryProvider is introduced for messages
Deferring the decision means each stateful channel author improvises (in-process dict, separate store, sneak it onto Message.additional_properties), and those choices will have to be reconciled when AG-UI lands. Can we pick the boundary now, even if no implementation ships in v1?
- Document FoundryHostedAgentHistoryProvider roundtrip of additional_properties namespaces via the agent_framework container key on stored OutputItems. - Add Foundry storage gap subsection capturing the update_item service ask required for post-push delivery_tracking[] mutation. - Triage open questions: 18 resolved (now in a Resolved Questions decisions log), 3 notes-updated, 6 unchanged. Capture spec-body follow-ups implied by the resolutions in a new Decisions-driven follow-ups subsection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Motivation and Context
Description
Contribution Checklist