Python: forward GitHubCopilotOptions verbatim to create_session#7155
Python: forward GitHubCopilotOptions verbatim to create_session#7155giles17 wants to merge 2 commits into
Conversation
Refactor the GitHub Copilot agent to forward the full options dict to the Copilot SDK's create_session/resume_session instead of hand-mapping a fixed subset. GitHubCopilotOptions stays as the curated, typed surface, but any other create_session parameter (reasoning_effort, context_tier, enable_citations, ...) is now passed through verbatim. Unknown keys surface as TypeError from the SDK instead of being silently dropped. De-duplicates the near-identical _create_session/_resume_session bodies into a shared _build_session_kwargs helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9d016d5-4d8c-43f0-a8fb-f3cf3d1ad7eb
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR updates the Python agent-framework-github-copilot agent to forward GitHubCopilotOptions (and any additional Copilot SDK create_session kwargs) through to the underlying Copilot SDK, eliminating the previous “fixed subset + silent drop” behavior and reducing ongoing maintenance for newly supported SDK options.
Changes:
- Forward merged
default_options+ per-runoptionsverbatim intocreate_session/resume_session, so any supported SDK kwarg works without new plumbing. - Introduce
_build_session_kwargsto centralize session kwarg construction and replace duplicated logic in_create_session/_resume_session. - Update tests to assert omission of unset passthrough keys (rather than passing explicit
None) and to reflect removal of per-key internal attributes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/github_copilot/agent_framework_github_copilot/_agent.py | Adds _build_session_kwargs and switches session creation/resumption to passthrough-style kwarg forwarding with a few special-cases. |
| python/packages/github_copilot/tests/test_github_copilot_agent.py | Updates tests to validate new passthrough behavior and the updated internal storage model for defaults. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 87%
✓ Correctness
The refactoring correctly simplifies the session-kwargs construction, but
_build_session_kwargsforwards all keys fromruntime_options(per-runoptions=) to the SDK without stripping agent-internal keys that are notcreate_sessionparameters. Specifically,on_pre_tool_use(which existing tests pass per-run, e.g. line 2971) andtimeout(consumed at_run_implline 616) survive the merge and will cause aTypeErrorfrom the real SDK because they are not validcreate_sessionkeyword arguments. The old code was immune because it only explicitly extracted known SDK keys fromruntime_options.
✓ Security Reliability
The refactoring correctly maintains the secure default for
on_permission_request(deny-all) and properly overideshooks,tools,model, andstreaming. However,on_pre_tool_usefrom runtime options is consumed by_build_session_hooks(placed inside thehooksdict) but also remains inkwargsfrom the passthrough spread at line 1138. This meanscreate_sessionwould receive it as both a top-level kwarg AND insidehooks, risking either a TypeError from the SDK (if it's not a valid top-level param) or conflicting configuration (if it is). The same pattern applies totimeoutwhich the run method consumes locally but does not remove before forwarding.
✓ Test Coverage
The PR refactors session option handling to forward all options verbatim to the Copilot SDK. Existing tests were correctly updated to match the new behavior (omitting unset keys rather than passing None). However, the core new capability — forwarding arbitrary non-curated keys (e.g. reasoning_effort, context_tier) to create_session — lacks a dedicated test proving that an arbitrary key supplied in default_options or runtime options actually arrives in the SDK call. Similarly, the per-run override of such arbitrary keys is untested.
✓ Failure Modes
The new
_build_session_kwargsmethod forwardsruntime_optionsverbatim to the SDK via{**self._default_options, **opts}. However,_run_implpasses its entireoptsdict (including agent-internal keys liketimeoutandon_pre_tool_usethat are NOTcreate_sessionparameters) asruntime_options. In the old code these were harmlessly ignored because only an explicit set of keys was forwarded. Now they leak intocreate_session(**kwargs), likely causingTypeErrorin production. Tests don't catch this because the mock accepts any kwargs.
✗ Design Approach
I found one blocking design regression. The new verbatim session-kwargs merge now forwards adapter-consumed runtime options directly into the Copilot SDK session factory, so previously supported code paths like per-run
timeoutandon_pre_tool_usenow raiseTypeErroragainst the realCopilotClientsignature.
Flagged Issues
-
_build_session_kwargsat line 1138 splats the full runtimeoptsdict intocreate_session/resume_session, butCopilotClient.create_sessiondoes not accept adapter-local keys liketimeout,on_pre_tool_use, oron_function_approval. This breaks documented/tested runtime options such asoptions={"timeout": 30}andoptions={"on_pre_tool_use": hook}, which will raiseTypeErrorin production.
Automated review by giles17's agents
|
Flagged issue
Source: automated DevFlow PR review |
- Strip agent-internal/client-level keys (on_pre_tool_use, on_function_approval, timeout, cli_path, log_level, base_directory) from the forwarded kwargs so they cannot leak into create_session/resume_session and raise TypeError. - Source caller tools from the merged options layer so tools supplied via default_options are honored instead of silently dropped. - Honor a caller-supplied native 'hooks' dict in _build_session_hooks (composing with the on_pre_tool_use shortcut) instead of unconditionally overwriting it. - Validate mock create_session/resume_session calls against the real SDK signatures in tests so invalid kwargs surface as TypeError, and add regression tests for the passthrough contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9d016d5-4d8c-43f0-a8fb-f3cf3d1ad7eb
| kwargs["tools"] = self._prepare_tools(all_tools) if all_tools else None | ||
|
|
||
| kwargs["streaming"] = streaming | ||
| kwargs["model"] = opts.get("model") or self._settings.get("model") or None |
There was a problem hiding this comment.
this will already be set to opts['model'] so this is doing double work, we can check if model is present in kwargs, and if so skip, otherwise set to self._settings....
Motivation & Context
The
agent-framework-github-copilotpackage exposesGitHubCopilotOptions, aTypedDictwhose keys are hand-mapped onto the Copilot SDK'screate_session.That SDK accepts ~70 keyword parameters, but the agent only forwarded a fixed
subset (~8). Every time a user needed another option (e.g.
reasoning_effort,context_tier,enable_citations), it required a four-touch code change(TypedDict key +
__init__pop + two duplicated session-builder mappings), andany key the agent didn't recognize was silently dropped — the root cause of
recurring "my option isn't being applied" reports.
This change removes that maintenance treadmill and the silent-drop footgun so
the package can move toward a stable release without a stream of additive
option changes. It aligns the package with the sibling
ClaudeAgent, whichalready forwards its options dict verbatim to its underlying agent SDK.
Description & Review Guide
What are the major changes?
_create_session/_resume_sessionnow forward the entire mergedoptions dict to the Copilot SDK instead of enumerating a fixed subset. Any
create_sessionparameter works without further code changes._build_session_kwargshelper that replaces the twonear-identical session-builder bodies. It layers agent
default_optionsthen per-run
options, and special-cases only what needs it:tools(merge + convert),
model,on_permission_request(secure default), andhooks._mcp_servers,_provider,_instruction_directories,_skill_directories,_disabled_skills) andtheir pops; these now ride through
default_options.GitHubCopilotOptionsremains the curated, typed, documented surface — itsdocstring now clarifies it is a common-case subset and that any other SDK
create_sessionparameter is forwarded verbatim (unknown names raiseTypeErrorrather than being silently ignored).Nonepassed for unset keys) to assert the new passthrough behavior.
What is the impact of these changes?
create_sessionparameters are usable immediately; no per-optionplumbing going forward.
TypeErrorfrom the SDK instead ofbeing silently dropped.
TypeError, landing it before the stable release keeps it additive; thecommon, documented usage remains backward compatible.
What do you want reviewers to focus on?
_build_session_kwargs(tools,model,on_permission_request,hooks,streaming) are the correct minimal set,and that precedence (per-run
options> agentdefault_options> settings)is preserved.
New usage
Agent-wide defaults — curated keys and previously-unsupported passthrough keys
now work side by side:
Per-run overrides via
options=(take precedence over agent defaults):Unknown/misspelled keys now fail fast instead of being silently dropped:
Related Issue
Fixes #7154
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.