Python: Add hosting protocol helper surface - #6891
Conversation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR introduces a helper-first Python hosting surface: agent-framework-hosting now provides shared execution/session state primitives for app-owned routes, and agent-framework-hosting-responses provides OpenAI Responses protocol conversion helpers (request → run args, run result → Responses JSON/SSE). It also updates the local Responses hosting sample to use native FastAPI routing to demonstrate the intended developer experience.
Changes:
- Added
AgentFrameworkState,SessionStore, and run-args TypedDicts (AgentRunArgs,WorkflowRunArgs) toagent-framework-hosting. - Added Responses helper functions (
responses_to_run,responses_from_run,responses_stream_events_from_run,responses_session_id,create_response_id) and expanded output rendering to map richer AF content into Responses output items. - Updated the
local_responsessample + docs to use FastAPI routes and the new helper/state surface (streaming via SSE and non-streaming JSON).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/samples/04-hosting/af-hosting/README.md | Updates sample index text to reflect the helper-first + FastAPI route approach. |
| python/samples/04-hosting/af-hosting/local_responses/README.md | Reframes the sample around Responses helpers + app-owned FastAPI routes instead of channel run hooks. |
| python/samples/04-hosting/af-hosting/local_responses/pyproject.toml | Adds FastAPI dependency for the updated sample. |
| python/samples/04-hosting/af-hosting/local_responses/call_server_af.py | Updates the AF-backed client script to match the new (non-streaming) interaction pattern. |
| python/samples/04-hosting/af-hosting/local_responses/app.py | Replaces channel host usage with a native FastAPI /responses route using AgentFrameworkState + Responses helpers. |
| python/packages/hosting/tests/hosting/test_state.py | Adds unit tests for SessionStore and AgentFrameworkState behaviors. |
| python/packages/hosting/README.md | Updates package positioning and quickstart toward app-owned hosting with shared state helpers. |
| python/packages/hosting/agent_framework_hosting/_state.py | Introduces the new state/session primitives and TypedDict run-args surfaces. |
| python/packages/hosting/agent_framework_hosting/init.py | Exports the new state/session/run-args surface from the package. |
| python/packages/hosting-responses/tests/hosting_responses/test_parsing.py | Adds tests for new Responses helper functions and richer output-item mapping. |
| python/packages/hosting-responses/README.md | Updates docs from “channel” framing to “helpers for app-owned routes” with an example. |
| python/packages/hosting-responses/agent_framework_hosting_responses/_parsing.py | Adds helper APIs and full Responses output/SSE rendering backed by OpenAI SDK Pydantic models. |
| python/packages/hosting-responses/agent_framework_hosting_responses/init.py | Re-exports the new helper functions. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 85%
✓ Correctness
The PR adds hosting protocol helpers (AgentFrameworkState, SessionStore, and Responses conversion functions) with a clean separation between framework-owned state and app-owned routing. The implementation is correct: ResponseStream iteration semantics are properly handled (auto-finalization on consumption, safe re-call of get_final_response), AgentResponse.messages is always a list[Message] matching the Sequence check in result_to_output_items, and the session_id routing logic (conv prefix → conversation field, resp_ prefix → omitted) is consistent. No bugs, race conditions, or incorrect API usage found.
✓ Security Reliability
The PR introduces hosting protocol helpers and a shared state surface. The code is generally well-structured with appropriate input validation (empty session_id checks, type guards on body fields). One moderate reliability concern:
AgentFrameworkState.get_target()has a race condition for async target factories under concurrent access — the check-then-await-then-set sequence lacks synchronization, so concurrent calers can bypass the intended once-only initialization promise.
✓ Test Coverage
Test coverage for the new helper functions is reasonable for the core happy paths (text, reasoning, function_call, function_result, streaming) but leaves a substantial rendering surface untested. The ~600 lines of content-type rendering code in
_parsing.py(code_interpreter, image_generation, mcp, shell, approval, media) are duplicated from_channel.pyand NOT covered by the existing channel tests since those exercise the channel's own copy. TheAgentFrameworkStatetests cover the main paths but missreset_session()and documented error paths (ValueError, TypeError).
✓ Failure Modes
The new helper surface is well-structured and the rendering logic is thorough. The primary failure-mode concern is a TOCTOU race in
AgentFrameworkState.get_target()andget_session_store()when async target factories are used under concurrent requests: theawaitat line 145 of_state.pycreates a yield point between the cache check and cache write, which can lead to duplicate target creation and, more critically, splitSessionStoreinstances where sessions created in the first store are silently lost to subsequent requests that use the second (cached) store.
✗ Design Approach
I found two design-level gaps in the new helper-first surface. First, the sample and README wire
SessionStoredirectly toprevious_response_id, which only preserves continuity for the second turn and silently starts a freshAgentSessionon turn 3+ unless callers also invent a stableconversation_id. Second, the new streaming helper only emits text deltas, so streamed reasoning/tool-call updates are dropped until the finalresponse.completedevent even though the existing channel/client in this repo already support richer Responses event shapes.
Flagged Issues
- Session continuity in the helper-first sample breaks after turn 2 because
local_responses/app.py:111keysSessionStorebyresponses_session_id(...), and_parsing.py:177-196returns the changingprevious_response_idwhile_state.py:31-47only reuses sessions for identical keys. - The new SSE helper in
_parsing.py:906-914streams onlyresponse.output_text.delta, while the existing Responses channel already emits per-content events in_channel.py:368-456and447-756; streamed tool/reasoning updates are therefore silently suppressed until completion.
Automated review by eavanvalkenburg's agents
|
Flagged issue Session continuity in the helper-first sample breaks after turn 2 because Source: automated DevFlow PR review |
|
Flagged issue The new SSE helper in Source: automated DevFlow PR review |
d31619f to
32f771f
Compare
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 77%
✓ Correctness
The code is consistent, error handling is present, and the samples clearly document their non-production status. The workflow sample's checkpoint cursor logic has a minor fragility around the conversation_id prefix heuristic, but it works correctly for the documented and demonstrated use cases. No blocking correctness issues found.
✓ Security Reliability
The new
_state.pyintroducesAgentStateandWorkflowStateclasses with aget_target()method that has a race condition whentargetis a raw awaitable (coroutine object): concurrent callers before the first resolution completes will both attempt toawaitthe same coroutine, causing aRuntimeError. TheSessionStoreand session management have proper input validation. The removal of the old_host.pyand associated infrastructure is consistent with the PR's stated direction. The samples have appropriate production-readiness disclaimers documenting the need for auth and session partitioning. No blocking security or reliability issues found. The lock file changes are mechanical sys_platform marker additions.
✓ Test Coverage
Test coverage for the new helper-first API is solid on happy paths (parsing, session continuity, round-trip, state holders). Two gaps stand out: (1) the streaming helper
responses_from_streaming_runhas no error-path test — the old channel testedresponse.failedemission on mid-stream exceptions, but the new helper propagates exceptions without any test verifying that behavior or showing the app-level pattern; (2) the_model_from_updatelogic that captures model ids from raw streaming chunks and overrides the finalized response model is entirely untested. The PR replaces the oldResponsesChannel(with its comprehensive test suite) with standalone helper functions and new tests. The new test coverage adequately exercises the happy paths for parsing, session continuity, and streaming. However, there is a notable gap:responses_from_streaming_runhas no error handling for mid-stream failures, and unlike the oldtest_channel.pywhich hadtest_sse_emits_failed_when_stream_raisesverifying aresponse.failedevent, the new tests include no equivalent coverage for error propagation during streaming. The hosting package's API surface change from channel-based to state-helper-based is well tested for core paths (SessionStore CRUD, target resolution, session creation). The Telegram channel deletion is intentional and its tests were appropriately co-deleted. Minor test coverage asymetry exists: WorkflowState lacks parity tests for cache_target=False and bare-awaitable rejection that AgentState has, though the underlying code paths are nearly identical. The new_state.pymodule (AgentState, WorkflowState, SessionStore) has solid test coverage intest_state.pycovering all major paths: CRUD operations with validation, target resolution from instances/callables/awaitables/builders, session create-once semantics, and session aliasing. The TypedDicts (AgentRunArgs, WorkflowRunArgs) are exercised through the hosting-responses helpers tested in test_parsing.py and test_http_round_trip.py. One minor gap: the RuntimeError guardrail on the synchronous.targetproperty (raised when callable/awaitable targets haven't been resolved yet) lacks a direct test. The deleted test files (test_host.py, test_host_disk.py, test_isolation.py, test_types.py) covered production code that was also removed in this PR, so no coverage regression on existing code. The new test_state.py provides solid coverage of SessionStore, AgentState, and WorkflowState core behaviors. Minor coverage gap: the.targetproperty's RuntimeError guard path (when accessed before async resolution) is untested.
✓ Failure Modes
The
responses_from_streaming_runhelper has no error handling around the stream iteration loop. If the agent's stream raises an exception mid-iteration, the SSE stream terminates without a terminalresponse.completedorresponse.failedevent, violating the Responses SSE protocol contract. The old channel code (deleted in this PR) explicitly caught stream errors and emittedresponse.failed. Neither the sample nor the test fixture wrap the helper in try/except, confirming this is the intended (and broken) usage pattern. The new_state.pyintroducesAgentState.get_or_create_sessionwith a get-check-create-set pattern that lacks the atomic protection the deleted_host.pyexplicitly documented. The old code useddict.setdefaultto prevent concurrent calers from silently overwriting each other's sessions. While the default in-memorySessionStorewith a pre-resolved target has no real async yield points (so no race in CPython today), the class is designed for subclassing with real async I/O backends (Redis, databases — mentioned in theSessionStoredocstring), where the race becomes real.
✗ Design Approach
I did not find another design-level issue in the shown changes that met the evidence bar. The new helper-first parsing/rendering split is moving in the right direction, but the streaming helper currently drops important parts of the Responses SSE contract: it does not emit a terminal failure event when the upstream stream raises, and it only streams plain text deltas, so reasoning/tool/media updates appear only in the final envelope. As written, the package no longer exposes the prior host/channel API, which conflicts with the PR rationale that the helper/state surface is being introduced while the existing hosting code remains available. The new helper-first state surface is mostly coherent, but
AgentState.get_or_create_sessiondropped the old host's concurrency protection. That makes concurrent requests for the same app-selected session id race to create separateAgentSessionobjects, so one request can run against a session instance that is no longer the stored canonical session for later turns. The new helper-first state surface is mostly internally consistent, but the SessionStore design currently locks in a mismatch between its documented continuation semantics and the behavior the tests endorse. In particular, earlier response IDs are documented as independently resumable, yet the new tests explicitly alias multiple IDs to the same mutable AgentSession object, which means older IDs will silently resume from the latest mutated state instead of their original continuation point. I found two design-level problems in the new workflow sample. First, the route caches oneWorkflowinstance for all requests even though the core workflow runtime explicitly forbids concurrent runs on the same instance, so overlapping/responsescalls will fail instead of behaving like independent conversations. Second, the sample recordsresponse_id -> checkpoint_idby asking a shared checkpoint directory for the workflow’s global latest checkpoint, which can bind one response id to another conversation’s checkpoint under concurrent traffic; the repo already uses per-context checkpoint storage to avoid that class of mix-up.
Flagged Issues
-
python/packages/hosting/tests/hosting/test_state.py:116-124enshrines same-object aliasing for multiple response IDs, but_state.pyandREADME.mdpromise calers can continue from any earlier point in a conversation. BecauseAgentSessionis a mutable state container, this approach cannot preserve historical continuation points — either snapshot/clone per stored id, or narrow the docs/tests to no longer promise arbitrary historical resume. -
python/samples/04-hosting/af-hosting/local_responses_workflow/app.py:210caches a single workflow instance behind the FastAPI route, butWorkflow.run()raisesWorkflowExceptionon overlapping executions. Two concurrentPOST /responsescalls will produce a 500. The route needs a fresh workflow per request (with a stable explicit workflow name for checkpoint compatibility). -
python/samples/04-hosting/af-hosting/local_responses_workflow/app.py:237resolves the cursor withget_latest(workflow_name=target.name)from one shared checkpoint directory. SinceFileCheckpointStorage.get_latest()is only scoped by workflow name, different conversations for the same workflow can race and alias the wrong checkpoint. Scope checkpoint storage per conversation/response id, or capture the checkpoint produced by the specific run.
Automated review by eavanvalkenburg's agents
|
Flagged issue
Source: automated DevFlow PR review |
|
Flagged issue
Source: automated DevFlow PR review |
|
Flagged issue
Source: automated DevFlow PR review |
Introduce AgentFrameworkState and SessionStore for app-owned hosting routes, add Responses run conversion/rendering helpers, and update the local Responses sample to use native FastAPI routing with streaming support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix constrained TargetT TypeVar in AgentFrameworkState: split __init__ into per-shape overloads (instance/sync factory/async factory/awaitable) since a bound TypeVar combined with one big Callable/Awaitable union parameter was unsolvable across pyright/pyrefly/ty/zuban. - Fix _FakeAgent test fixtures to structurally satisfy SupportsAgentRun (matching attribute types and overloaded run()), which the above surfaced. - Add SessionStore.put() to alias an additional session id to an already-resolved session, and use it in the local_responses sample to fix a real session-continuity bug: previous_response_id rotates every turn, so without aliasing the newly minted response id, turn 3+ of a conversation silently lost all prior history. Verified against a live Foundry model across a 3-turn conversation. - Fix responses_stream_events_from_run to report the real model instead of the "agent" fallback: AgentResponse.from_updates never carries a raw representation forward, so capture model from the individual streamed updates' raw representations instead. Verified live. - Add response_model=None to the sample's FastAPI route (it could not boot at all: FastAPI tried to build a Pydantic response model from the JSONResponse | StreamingResponse return annotation). - Map responses_to_run's ValueError to HTTP 400 instead of a 500. - Add HTTP round-trip integration tests (packages/hosting-responses) that exercise the same FastAPI + AgentFrameworkState + Responses helper wiring as the sample via httpx.ASGITransport, including a regression test for the session-continuity fix. - Add Workflow-target test coverage, SessionStore.put/reset_session tests, and TypeError-path coverage to packages/hosting/tests/hosting/test_state.py. - Extend call_server.py / call_server_af.py to a third conversation turn so they actually exercise the continuity chain (previous scripts stopped at turn 2, which would never have revealed the bug above). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per feedback: the growth of SessionStore was not the problem -- it's intentional, since OpenAI's previous_response_id is designed to let a caller continue (fork) from any earlier response, not just the latest one, so every response id has to stay independently resolvable. That part stays as-is. What was too complex was the call site: routes had to manually fetch a session and then conditionally alias it with a separate put() call. Folded that into a single get(session_id, alias=...) call instead: - SessionStore.get() gains an optional `alias` keyword that registers an additional id for the same session in the same call (no-op if alias is None or equal to session_id). Removed the separate put() method. - AgentFrameworkState.get_session() passes `alias` through. - local_responses sample and the HTTP round-trip integration tests now do `await state.get_session(lookup_id, alias=response_id)` instead of pulling the store out and orchestrating get()/put() by hand. - Documented that this in-memory SessionStore intentionally never evicts (by design, to support forking), and that a storage-backed replacement (Redis, a database, ...) is responsible for its own TTL/eviction policy. Verified against a live Foundry model across a 3-turn previous_response_id chain after the simplification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split the shared state surface into AgentState and WorkflowState, keep SessionStore and CheckpointStore as plain storage, and make state helpers responsible for get-or-create behavior. Update the Responses sample and HTTP round-trip tests to store the post-run session explicitly under the minted response id, and support WorkflowBuilder/orchestration-style builders via structural build() support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Widen fake agents' get_session service_session_id parameter to match the SupportsAgentRun protocol under the Python 3.11 test typing checkers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename responses_stream_events_from_run to responses_stream_from_run across exports, tests, docs, and the local Responses sample to align with the generic <protocol>_stream_from_run helper convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add AgentState.set_session and WorkflowState.set_checkpoint_storage so app code can pair get-or-create helpers with explicit post-run storage without reaching into the underlying stores. Update Responses docs, tests, and sample to use state.set_session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove CheckpointStore from WorkflowState so workflow checkpointing uses the existing CheckpointStorage abstraction directly. Keep WorkflowState focused on resolving workflow targets, including builders, and update hosting docs/tests accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename responses_stream_from_run to responses_from_streaming_run across the hosting-responses exports, tests, docs, and local Responses sample. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rewrite SPEC-002 to match the accepted helper-first hosting ADR and the implementation PR: AgentState, WorkflowState, SessionStore, Responses helpers, app-owned security/state responsibilities, and the minimal FastAPI Responses shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the unreleased AgentFrameworkHost/channel implementation, the old hosting-telegram package, and old host/channel samples. Keep agent-framework-hosting focused on AgentState, WorkflowState, and SessionStore, and keep hosting-responses focused on helper-first Responses conversion. Update SPEC-002 to match the accepted helper-first ADR and the implementation surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rebuild the local Responses workflow sample on the protocol-helper surface, add production-readiness cautions to the local hosting samples, and align file-backed workflow checkpoint/cursor storage under one sample storage root. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Handle streaming failures as terminal Responses SSE events, guard concurrent target/session initialization, and scope workflow sample checkpoint storage per continuation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e2eb3f4 to
bbca4ee
Compare
Document unknown conversation_id behavior in the agent sample and make the workflow sample explicitly reject conversation_id while continuing to use responses_session_id for previous_response_id. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
One conflicted file, @eavanvalkenburg |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation & Context
The Python hosting packages are moving toward a lighter helper-first shape where Agent Framework owns protocol-to-run conversion and shared execution state, while apps keep their native web framework routes, auth, middleware, background work, deployment shape, and response construction.
This implements the first slice of that direction for the existing hosting and hosting-responses packages, removes the earlier unreleased host/channel surface, and updates the local Responses samples so reviewers can inspect the intended developer experience for both agents and workflows.
Description & Review Guide
AgentState,WorkflowState,SessionStore,AgentRunArgs, andWorkflowRunArgstoagent-framework-hosting.create_response_id,responses_session_id,responses_to_run,responses_from_run, andresponses_from_streaming_run.local_responsesto use native FastAPI routing,AgentState,SessionStore, non-streaming JSON, and streaming viaResponseStream.local_responses_workflowas a helper-first FastAPI workflow sample usingWorkflowState, explicitFileCheckpointStorage, and an app-owned checkpoint cursor.AgentState,WorkflowState,SessionStore, and the Responses helper functions.Related Issue
Closes #6585. Refs #6588 for the follow-up Telegram helper-first work. Related ADR: #6837. Checked for similar open PRs; existing hosting/channel PRs target the previous channel model or other protocols.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.