Repository navigation
Python: Preserve request metadata in hosted responses - #9745
Aman (its-amann) wants to merge 3 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 6037edd0fa65
Model: gpt-5.6-sol
Overview
The change correctly separates fresh-request seeding from persisted-response recovery and adds broad round-trip coverage for omitted, empty, and populated metadata. The SDK also deep-copies request metadata, and the new tests cover streaming, non-streaming, storage, and recovery precedence. Two residual Medium risks remain because public metadata and the SDK's reserved internal metadata share the same map: callers can inject trusted recovery state, and valid maximum-size metadata can prevent the host from recording an incomplete finish reason.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py
| if persisted_response is not None: | ||
| return ResponseEventStream(response=persisted_response, response_id=context.response_id) | ||
| return ResponseEventStream(response_id=context.response_id) | ||
| return ResponseEventStream(request=request, response_id=context.response_id) |
There was a problem hiding this comment.
Passing the whole request into ResponseEventStream lets clients supply the reserved metadata["_internal_metadata"] bag, which host code later trusts for incomplete status, provider cursors, usage, and citations. A request can therefore forge server-owned recovery state (for example, _incomplete_reason) and alter the returned or persisted response. Reject or strip this reserved key for fresh requests while preserving it only when restoring a trusted persisted response.
| if persisted_response is not None: | ||
| return ResponseEventStream(response=persisted_response, response_id=context.response_id) | ||
| return ResponseEventStream(response_id=context.response_id) | ||
| return ResponseEventStream(request=request, response_id=context.response_id) |
There was a problem hiding this comment.
A request with the valid maximum of 16 metadata entries fills the SDK's public metadata quota. If the agent then finishes with length or content_filter, record_finish_reason() tries to add the reserved _internal_metadata entry, raises ValueError, and turns the truncated response into response.failed instead of response.incomplete. Keep framework-private state outside the public quota, or otherwise preserve all 16 public entries while allowing the host to record its incomplete reason.
Drop a client-supplied _internal_metadata bag when a fresh response is seeded, and keep that bag only for a trusted persisted snapshot. When public metadata already holds 16 entries, still record a truncating finish reason so the response ends incomplete instead of failed.
|
Aman (@its-amann) One material boundary case remains in |
A full public metadata map still ends length and content_filter turns incomplete, with the assistant text and all 16 public keys kept, for both streaming and non-streaming responses.
|
Eduard van Valkenburg (@eavanvalkenburg) thanks, pushed a fix for both findings incl the 16 key case with tests for both modes. ready for review now |
GitHub Actions
Inactive
Motivation & Context
Requests to
ResponsesHostServercan include metadata to label or correlate a turn, but fresh responses do not retain it in their events or stored response. Clients reading the response back cannot recover the values they sent.Description & Review Guide
ResponseEventStreamfor fresh responses. Continue restoring recovery streams from the persisted response. Add HTTP round-trip tests for streaming, non-streaming and stored responses, plus recovery controls.GET /responses/{id}, including an explicitly empty metadata object. Persisted recovery metadata is preserved.requestandresponsetogether. Tests cover omitted, empty and populated metadata, and recovery with or without a persisted response.Related Issue
Fixes #9529
AI Assistance
AI assistance details: Issue analysis, implementation, regression tests and this PR description were assisted.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after a language prefix) — a workflow keeps the label and the title prefix in sync automatically.