Python: Fix broken samples for GitHub Copilot, declarative, and Responses API#4915
Python: Fix broken samples for GitHub Copilot, declarative, and Responses API#4915giles17 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
…nses API - Add missing on_permission_request handler to github_copilot_basic and github_copilot_with_session samples (required by copilot SDK) - Increase timeout for remote MCP query in github_copilot_with_mcp sample - Soften session isolation claim in github_copilot_with_session sample - Fix inline_yaml sample: pass project_endpoint via client_kwargs instead of relying on YAML connection block (AzureAIClient expects project_endpoint, not endpoint) - Handle raw JSON schemas in Responses client _convert_response_format so declarative outputSchema works with the Responses API 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
Fixes broken Python samples and improves the OpenAI Responses client’s handling of declarative output schemas after the OpenAI package extraction and Copilot SDK changes.
Changes:
- Add required
on_permission_requesthandler usage to GitHub Copilot samples and adjust MCP timeout for slower remote calls. - Update declarative inline YAML sample to pass
project_endpointviaclient_kwargs(instead of YAML connection mapping). - Extend
OpenAIChatClientresponse_format conversion to accept raw JSON schemas by wrapping them into ajson_schemaformat config.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/samples/02-agents/providers/github_copilot/github_copilot_with_session.py | Adds permission request handler wiring and clarifies session isolation note. |
| python/samples/02-agents/providers/github_copilot/github_copilot_with_mcp.py | Increases per-call timeout for remote MCP query. |
| python/samples/02-agents/providers/github_copilot/github_copilot_basic.py | Adds permission request handler wiring for session creation. |
| python/samples/02-agents/declarative/inline_yaml.py | Passes project_endpoint via client_kwargs to align with Azure AI client expectations. |
| python/packages/openai/agent_framework_openai/_chat_client.py | Adds raw JSON-schema wrapping support in _convert_response_format. |
giles17
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 86%
✓ Correctness
This PR adds handling for raw JSON schemas in
_convert_response_format(needed for declarativeoutputSchemato work with the Responses API), fixes theinline_yamlsample to passproject_endpointviaclient_kwargs, and addson_permission_requesthandlers to GitHub Copilot samples. The core logic change is correct: raw schemas like{"type": "object", "properties": {...}}are properly detected (after all other format types are checked) and wrapped in thejson_schemaenvelope. The shallow copy avoids mutating the input. Thestrict: TrueandadditionalProperties: falseadditions align with OpenAI Structured Outputs requirements. One notable omission: the identical_convert_response_formatmethod inpython/packages/core/agent_framework/openai/_responses_client.py(used byRawOpenAIResponsesClientand its subclassRawAzureAIClient) is not updated, meaning the same raw-schema issue would occur when declarative agents use the Azure AI client path.
✓ Security Reliability
This PR adds a convenience wrapper in
_convert_response_formatto handle raw JSON schemas (dicts with apropertieskey) by auto-wrapping them in ajson_schemaenvelope, updates GitHub Copilot samples to include permission-request handlers, and adjusts theinline_yaml.pysample to pass the project endpoint viaclient_kwargsinstead of YAML. No blocking security or reliability issues were found. Theproperties-based heuristic for detecting raw schemas is loose but positioned after all explicit type checks, so misclassification risk is minimal — invalid schemas would be rejected by the downstream API. The sample changes are straightforward additions of interactive permission prompts.
✗ Test Coverage
The PR adds a new code path in
_convert_response_formatto handle raw JSON schemas (dicts with apropertieskey) by wrapping them into thejson_schemaenvelope. This is non-trivial logic with multiple branches (title extraction,additionalPropertiesdefaulting, strict mode), but the PR includes zero unit tests for the new behavior. The remaining changes are sample code updates (permission handlers, endpoint config, timeout increase) which don't require unit tests. The Responses client's equivalent_convert_response_formatmethod has thorough test coverage (title fallback, strict mode, missing schema, unsupported types), and the same level of coverage should be provided here.
✗ Design Approach
The main design concern is in
_convert_response_format: the heuristic of checking for a top-level"properties"key to detect raw JSON schemas is unreliable. It fails silently for schemas without"properties"(e.g.,{"type": "object"},{"anyOf": [...]},{"$defs": {...}), and could falsely match other dict shapes that happen to include"properties". The remaining changes — movingon_permission_requestintodefault_options, adding a per-call timeout for MCP, and shiftingproject_endpointout of YAML intoclient_kwargs— are straightforward and well-structured. The duplication ofprompt_permissionacross two sample files is a minor concern.
Flagged Issues
- The new raw JSON schema handling in
_convert_response_format(lines 630–641 of_chat_client.py) has no unit tests. This code has at least 5 distinct behaviors: (1) wrapping a raw schema into ajson_schemaenvelope, (2) addingadditionalProperties: falsefortype: objectschemas, (3) preserving existingadditionalProperties, (4) extractingtitleas the schemaname, and (5) defaulting name to"response". Comparable tests exist for the Responses client's version (seetest_openai_responses_client.pylines 1428–1562) and should be mirrored here. - The
"properties" in response_formatheuristic is both over-inclusive and under-inclusive as a raw-schema detector. It mises valid schemas lacking a top-levelpropertieskey (e.g.,{"type": "object"},{"anyOf": [...]},{"$ref": ".."}) and could incorrectly match non-schema dicts. A more robust approach is to key onformat_typebeing a JSON Schema primitive type ("object","array","string", etc.) or to check for known schema keywords (anyOf,oneOf,allOf,$ref,$defs,properties).
Suggestions
- The identical
_convert_response_formatmethod inpython/packages/core/agent_framework/openai/_responses_client.py(line 408) is not updated.RawAzureAIClientextendsRawOpenAIResponsesClient, so declarative agents usingAzureAIClientwithoutputSchemawould still hit theUnsupported response_formatexception. Apply the same fix there for consistency. - The
prompt_permissionfunction is duplicated verbatim ingithub_copilot_basic.pyandgithub_copilot_with_session.py. Consider extracting it into a shared sample helper (e.g.,github_copilot/_shared.py) to keep the samples DRY and prevent drift. - The
strictfield is unconditionally hardcoded toTruein the new raw-schema branch, unlike the existingjson_schemabranch which conditionally includes it from the input. Consider allowing calers to opt out of strict mode via astrictkey in the raw schema dict. - Add a parametrized test covering the key branches: raw schema with title, without title, with existing
additionalProperties, withtype!=object(verifyingadditionalPropertiesis not injected), and confirmingstrict: Trueis always set. - The
schema.pop("title", None)call silently stripstitlefrom the schema forwarded to the API. While necessary for strict mode, a brief comment explaining the removal would make the intent clearer to future readers.
Automated review by giles17's agents
- Broaden raw schema detection to handle anyOf, oneOf, allOf, $ref, $defs keywords and JSON Schema primitive types, not just 'properties' - Apply same raw schema handling to azure-ai _shared.py for consistency - Add unit tests for both openai and azure-ai response_format conversion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
giles17
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 86%
✓ Correctness
This PR adds handling for raw JSON schemas in
_convert_response_format(needed for declarativeoutputSchemato work with the Responses API), fixes theinline_yamlsample to passproject_endpointviaclient_kwargs, and addson_permission_requesthandlers to GitHub Copilot samples. The core logic change is correct: raw schemas like{"type": "object", "properties": {...}}are properly detected (after all other format types are checked) and wrapped in thejson_schemaenvelope. The shallow copy avoids mutating the input. Thestrict: TrueandadditionalProperties: falseadditions align with OpenAI Structured Outputs requirements. One notable omission: the identical_convert_response_formatmethod inpython/packages/core/agent_framework/openai/_responses_client.py(used byRawOpenAIResponsesClientand its subclassRawAzureAIClient) is not updated, meaning the same raw-schema issue would occur when declarative agents use the Azure AI client path.
✓ Security Reliability
This PR adds a convenience wrapper in
_convert_response_formatto handle raw JSON schemas (dicts with apropertieskey) by auto-wrapping them in ajson_schemaenvelope, updates GitHub Copilot samples to include permission-request handlers, and adjusts theinline_yaml.pysample to pass the project endpoint viaclient_kwargsinstead of YAML. No blocking security or reliability issues were found. Theproperties-based heuristic for detecting raw schemas is loose but positioned after all explicit type checks, so misclassification risk is minimal — invalid schemas would be rejected by the downstream API. The sample changes are straightforward additions of interactive permission prompts.
✗ Test Coverage
The PR adds a new code path in
_convert_response_formatto handle raw JSON schemas (dicts with apropertieskey) by wrapping them into thejson_schemaenvelope. This is non-trivial logic with multiple branches (title extraction,additionalPropertiesdefaulting, strict mode), but the PR includes zero unit tests for the new behavior. The remaining changes are sample code updates (permission handlers, endpoint config, timeout increase) which don't require unit tests. The Responses client's equivalent_convert_response_formatmethod has thorough test coverage (title fallback, strict mode, missing schema, unsupported types), and the same level of coverage should be provided here.
✗ Design Approach
The main design concern is in
_convert_response_format: the heuristic of checking for a top-level"properties"key to detect raw JSON schemas is unreliable. It fails silently for schemas without"properties"(e.g.,{"type": "object"},{"anyOf": [...]},{"$defs": {...}), and could falsely match other dict shapes that happen to include"properties". The remaining changes — movingon_permission_requestintodefault_options, adding a per-call timeout for MCP, and shiftingproject_endpointout of YAML intoclient_kwargs— are straightforward and well-structured. The duplication ofprompt_permissionacross two sample files is a minor concern.
Flagged Issues
- The new raw JSON schema handling in
_convert_response_format(lines 630–641 of_chat_client.py) has no unit tests. This code has at least 5 distinct behaviors: (1) wrapping a raw schema into ajson_schemaenvelope, (2) addingadditionalProperties: falsefortype: objectschemas, (3) preserving existingadditionalProperties, (4) extractingtitleas the schemaname, and (5) defaulting name to"response". Comparable tests exist for the Responses client's version (seetest_openai_responses_client.pylines 1428–1562) and should be mirrored here. - The
"properties" in response_formatheuristic is both over-inclusive and under-inclusive as a raw-schema detector. It mises valid schemas lacking a top-levelpropertieskey (e.g.,{"type": "object"},{"anyOf": [...]},{"$ref": ".."}) and could incorrectly match non-schema dicts. A more robust approach is to key onformat_typebeing a JSON Schema primitive type ("object","array","string", etc.) or to check for known schema keywords (anyOf,oneOf,allOf,$ref,$defs,properties).
Suggestions
- The identical
_convert_response_formatmethod inpython/packages/core/agent_framework/openai/_responses_client.py(line 408) is not updated.RawAzureAIClientextendsRawOpenAIResponsesClient, so declarative agents usingAzureAIClientwithoutputSchemawould still hit theUnsupported response_formatexception. Apply the same fix there for consistency. - The
prompt_permissionfunction is duplicated verbatim ingithub_copilot_basic.pyandgithub_copilot_with_session.py. Consider extracting it into a shared sample helper (e.g.,github_copilot/_shared.py) to keep the samples DRY and prevent drift. - The
strictfield is unconditionally hardcoded toTruein the new raw-schema branch, unlike the existingjson_schemabranch which conditionally includes it from the input. Consider allowing calers to opt out of strict mode via astrictkey in the raw schema dict. - Add a parametrized test covering the key branches: raw schema with title, without title, with existing
additionalProperties, withtype!=object(verifyingadditionalPropertiesis not injected), and confirmingstrict: Trueis always set. - The
schema.pop("title", None)call silently stripstitlefrom the schema forwarded to the API. While necessary for strict mode, a brief comment explaining the removal would make the intent clearer to future readers.
Automated review by giles17's agents
Description
GitHub Copilot samples
on_permission_requesthandler, now required by the copilot SDK when creating sessions.Declarative samples
connectionblock and passedproject_endpointdirectly viaclient_kwargs. The declarative loader maps connection endpoints asendpoint, butAzureAIClientexpectsproject_endpoint.Responses API client
_convert_response_format. The declarative loader'soutputSchemaproduces raw schemas which the Responses client previously rejected. The client now auto-wraps them in the expectedjson_schemaenvelope withstrict: trueandadditionalProperties: false.Checklist