Repository navigation
Python: [BREAKING] Guard Copilot runtime connections - #9611
Roger Barreto (rogerbarreto) wants to merge 3 commits into
Conversation
Keep connection settings in application configuration while retaining caller generation options and application defaults.
There was a problem hiding this comment.
🟢 Approval recommended
The narrow change reuses existing validation, has matching regression coverage and documentation, and has no unresolved findings.
No open issues 🎉
What changed in this PR
This PR reinforces the Foundry Invocations configuration boundary: connection settings remain application-owned, while callers can supply per-turn generation options.
Changes:
- Add
mcp_serversandproviderto the existing agent-controlled option validation. - Extend rejection tests across streaming modes, wire formats, and unsupported-options policies, and verify application defaults remain unchanged.
- Document where trusted connection configuration belongs.
| File | Description |
|---|---|
python/packages/foundry_hosting/tests/test_invocations.py |
Extend validation coverage and verify connection defaults are preserved. |
python/packages/foundry_hosting/README.md |
Clarify the application-owned connection configuration boundary. |
python/packages/foundry_hosting/agent_framework_foundry_hosting/_invocations.py |
Reject caller-supplied mcp_servers and provider options. |
Review effort: Balanced
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 7a37342aaeb6
Model: gpt-5.6-sol
Overview
This PR extends Invocations option validation to reject mcp_servers and provider before agent creation or storage access, with broad coverage across request, parser, hook, streaming, wire-format, and unsupported-option paths. The ordering and application-default preservation guards are strong. However, reserving provider for every hosted agent breaks an existing public per-run option contract used by GitHubCopilotAgent, so the change needs explicit compatibility handling or breaking-change treatment.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/foundry_hosting/agent_framework_foundry_hosting/_invocations.py
Keep connection ownership in Copilot agents without reserving option names across every Invocations implementation. BREAKING CHANGE: configure connections in default_options or construct Copilot agents with allow_runtime_connection_overrides=True. Refs microsoft#9611
Host construction configures global SDK telemetry and contaminates later Responses log-sanitization tests. Mock SDK tracing for this HTTP test only, matching Foundry's existing isolation fixture. Refs microsoft#9611
Motivation & Context
Connection option names are agent-specific. Reserving
providerandmcp_serversacross the Invocations host rejects supported Copilot runtime configuration and settings produced by trusted application callbacks. Put default connection protection in the Copilot agent, where those options are interpreted, while preserving an explicit application-owned compatibility path.Description & Review Guide
allow_runtime_connection_overrides=FalsetoGitHubCopilotAgentandRawGitHubCopilotAgent. Reject per-runproviderandmcp_serversbefore Copilot SDK startup and when assembling session configuration; retain developer defaults and ordinary runtime options. Remove the host-wide connection-name restriction. MapAgentInvalidRequestExceptionto sanitized HTTP 400 or typed SSE client errors. Add regression coverage, migration documentation, and a proposed ADR. HTTP regressions isolate AgentServer SDK telemetry so they do not contaminate later tests' process-wide logging state.default_optionsor construct the agent withallow_runtime_connection_overrides=True. Opted-in runtime connections keep their previous precedence. The constructor flag cannot be enabled through incoming run options. Deploy the updated Copilot package alongside the host; older agent versions retain their existing behavior. Other agents keep ownership of their option semantics.Local checks passed on Windows with Python 3.12.10. Commands below were run from
python; typing tasks used the restored worktree environment, pinned uv 0.13.1, andUV_NO_SYNC=true..\.venv\Scripts\python.exe -m ruff check --no-fix packages\foundry_hosting packages\github_copilot- passed..\.venv\Scripts\python.exe -m ruff format --check packages\foundry_hosting packages\github_copilot- passed; 30 files already formatted..\.venv\Scripts\python.exe -m pyright --project packages\github_copilot\pyproject.toml --pythonpath .venv\Scripts\python.exe- passed; zero errors or warnings..\.venv\Scripts\python.exe -m pyright --project packages\foundry_hosting\pyproject.toml --pythonpath .venv\Scripts\python.exe- passed; zero errors or warnings..\.venv\Scripts\uv.exe run --no-sync poe test-typing -P github_copilot- passed for mypy, pyrefly, ty, zuban, and Pyright, including after the SDK telemetry-isolation fix..\.venv\Scripts\uv.exe run --no-sync poe test-typing -P foundry_hosting- passed for mypy, pyrefly, ty, zuban, and Pyright..\.venv\Scripts\python.exe -m pytest packages\foundry_hosting\tests\test_invocations.py packages\github_copilot\tests\test_github_copilot_agent.py -m 'not integration' -o addopts='' -p no:cacheprovider --tb=short -q- passed; 488 tests, six integration tests deselected. The same 488 regressions also passed with Copilot tests running before Invocations after the isolation fix..\.venv\Scripts\python.exe -m scripts.check_md_code_blocks packages\github_copilot\README.md packages\foundry_hosting\README.md packages\github_copilot\AGENTS.md ..\docs\decisions\0046-python-copilot-runtime-connections.md --no-glob- passed under the repository's configured snippet exclusions.Published-head GitHub CI for
3a65ab60fefec2e84709e953f30b6e1cdf89aa51passed the Merge Gatekeeper, all configured Python unit-test matrix jobs (Python 3.10-3.15 on Linux and 3.10-3.14 on Windows), package checks, test typing, samples/Markdown, and the full-workspace coverage gate.The separate API-compatibility job still fails on the existing Griffe crash resolving the unchanged trusted-base
EdgeGroup.builtin_typealias; #9614 proposes an upstream fix. The merge gate explicitly excludes this check. Local reproduction with the trusted base's pinned Griffe 2.4.0 is blocked because the configured package feed only offers versions through 2.3.2. Publication proceeded with this known failure explicitly acknowledged; this is not a claim that every check is green. Live integrations are not claimed as passed.Related Issue
N/A - revises the compatibility approach following review on this existing PR. No separately agreed implementation issue is linked; that checklist item remains unchecked. #9614 addresses the independent API-comparison failure, not this connection policy.
AI Assistance
AI assistance details: Implementation, regression tests, migration documentation, proposed ADR, validation, and this PR description.
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.