Python: Update hosting agent samples + fixes#5485
Conversation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 84%
✓ Correctness
The PR adds
file_datasupport for theinput_filecontent type in the Responses hosting layer, refactors samples (renaming env vars, switching to DefaultAzureCredential, reorganizing directories), adds integration test infrastructure for foundry_hosting, and updates CI workflows. The core logic in_convert_file_dataand theinput_file/input_imagechanges are correct: data URIs route throughContent.from_uriwhich correctly classifies them as typedata, text/* files are decoded to plain text, and theMessageContentInputFileContentmodel does havefile_dataandfilenameattributes (verified against source). No blocking correctness issues found.
✓ Security Reliability
The PR adds file_data (base64 data URI) support, integration tests, CI workflow changes, and sample updates. The main reliability concern is in
_convert_file_datawherebase64.b64decode().decode('utf-8')can raise unhandledUnicodeDecodeErrororbinascii.Erroron malformed input at the HTTP trust boundary. The remaining changes (env var renames, credential switch toDefaultAzureCredential, README updates, workflow configs) are straightforward and carry no security or reliability risk.
✓ Test Coverage
The new
_convert_file_datafunction has two distinct code paths (text/* decode vs. URI passthrough), but the only unit test exercises the non-text branch (application/pdf). The text/* branch — which base64-decodes content and returnsContent.from_text()— is covered only by integration tests that require live credentials. Additionally, themedia_type="image/*"change toContent.from_uriforinput_imagehas no unit-level verification. The integration test suite (test_responses_int.py) is thorough and well-structured.
✗ Design Approach
I found two design-level issues worth addressing before merging. The new
file_dataconversion path special-cases alltext/*uploads by decoding them as UTF-8 text, which is a losy workaround on top of a content layer that already knows how to preserve data URIs as typed data content. Separately, the new MCP sample now silently drops its only remote tool whenGITHUB_PATis missing, which turns a misconfigured MCP sample into an apparently healthy plain-chat sample instead of failing fast.
Suggestions
- The
media_type="image/*"argument toContent.from_uriforinput_imageprevents_validate_urifrom extracting the specific media type (e.g.image/jpeg) from data URIs, because extraction only occurs whenmedia_type is None(_types.py:247). Consider omittingmedia_typefor data URIs or letting validation extract it automatically.
Automated review by TaoChenOSU's agents
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 63 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
python/samples/04-hosting/foundry-hosted-agents/invocations/README.md:1
- This file is now empty. If the content was intentionally moved to the parent README, consider either deleting this README entirely or leaving a short pointer to the parent README so users who land here don’t get a blank page.
python/samples/04-hosting/foundry-hosted-agents/responses/README.md:1 - This file is now empty. If the content was intentionally moved to the parent README, consider either deleting this README entirely or leaving a short pointer to the parent README so users who land here don’t get a blank page.
Motivation and Context
Current samples don't align with samples in other Azure repositories. There are also defects in the responses host.
Description
Contribution Checklist