Python: Fix GitHubCopilotAgent to include tools added by ContextProvider.before_run in session creation#5780
Conversation
…ft#5736) _create_session and _resume_session only forwarded self._tools (constructor tools) to CopilotClient.create_session, dropping any tools contributed by context providers via session_context.extend_tools() during before_run. Merge provider-contributed tools into runtime_options in both _run_impl and _stream_updates before session creation, mirroring how RawAgent handles the merge at lines 1435-1440 in _agents.py. Update _create_session and _resume_session to combine self._tools with the merged runtime tools. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…der.before_run in session creation Fixes microsoft#5736
giles17
left a comment
giles17
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 74% | Result: All clear
Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach
Automated review by giles17's agents
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Fixes GitHubCopilotAgent session creation/resume so that tools added by ContextProvider.before_run (e.g., SkillsProvider’s load_skill / read_skill_resource) are included in the Copilot SDK session tool registry, preventing provider-contributed tools from being silently dropped.
Changes:
- Merge
SessionContext.toolsinto per-run runtime options before creating/resuming Copilot sessions (both non-streaming and streaming paths). - Update
_create_session/_resume_sessionto combine constructor tools with runtime-option tools when building the Copilot SDK session config. - Add unit tests validating provider tool forwarding/merging in both non-streaming and streaming runs.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/github_copilot/agent_framework_github_copilot/_agent.py | Merges provider-contributed tools into runtime options and ensures session creation/resume receives the combined tool set. |
| python/packages/github_copilot/tests/test_github_copilot_agent.py | Adds coverage to ensure provider-added tools are forwarded and merged correctly (streaming + non-streaming). |
| python/samples/02-agents/skills/mixed_skills/mixed_skills.py | Minor formatting adjustments (blank lines) in the skills sample. |
| python/uv.lock | Lockfile metadata ordering change for github-copilot-sdk specifier. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 92%
✓ Correctness
The PR correctly addresses the stated goal of forwarding provider-contributed tools to Copilot session creation. The
_create_sessionand_resume_sessionmerging logic (all_tools = list(self._tools or []) + list(opts.get("tools") or [])) is sound and avoids double-counting. However, the already-flagged shallow-copy mutation bug at lines 528 and 618 remains the only correctness issue:opts["tools"].extend(session_context.tools)mutates the caller's original list (sincedict(options)is a shallow copy) and will crash ifoptions["tools"]is a non-list Sequence like a tuple. The existing unresolved review comments accurately describe this issue and the suggested fix (build a new list instead of mutating in place) is correct.
✓ Security Reliability
The PR correctly addresses the core bug of forwarding provider-contributed tools to session creation. The existing unresolved review comments about
.extend()mutating the caller's tools list through the shallow-copiedoptsdict remain the primary reliability concern. No additional security or reliability issues were found beyond those already flaged.
✓ Test Coverage
The new tests cover provider tools in the non-streaming create path (with and without constructor tools) and in the streaming create path (provider-only). However, the
_resume_sessionmethod received an identical production code change (combiningself._toolswithopts.get("tools")) and has zero new test coverage for provider-contributed tools. Since the resume path is exercised whensession.service_session_idis already set, this is a realistic scenario (multi-turn conversations) that deserves a dedicated test.
✗ Design Approach
The fix addresses the reported gap, but the new session-tool merge path bypasses the repo's existing tool-merging contract and can silently register duplicate tool names when constructor tools overlap with runtime/provider tools. That is a design regression because the core agent merge logic treats duplicate names as an error rather than allowing ambiguous double-registration.
Flagged Issues
-
_agent.pynow concatenates constructor tools with runtime/provider tools directly during session creation/resume, bypassing the repo's normal unique-merge path. The core framework (_agents.py:107-114) uses_append_unique_tools(..., duplicate_error_message="Tool names must be unique.")and core tests assert that duplicate tool names must raise. As written, a provider injecting a tool with the same name as a constructor tool will silently pass both through to Copilot session creation instead of failing fast.
Automated review by giles17's agents
- Replace in-place .extend() with fresh list creation in both _run_impl and _stream_updates paths to prevent mutating the caller-provided options['tools'] list (shallow copy issue) - Also handles immutable Sequence types (e.g. tuple) correctly - Add test for provider tools forwarded via _resume_session path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation and Context
SkillsProvider.before_runregisters tools likeload_skillviacontext.extend_tools(), butGitHubCopilotAgentonly forwarded constructor-supplied tools (self._tools) when creating or resuming Copilot sessions. This caused provider-contributed tools to be silently dropped, so the model would see skill instructions in the system prompt but fail to invoke them.Fixes #5736
Description
The root cause was that
_create_sessionand_resume_sessiononly passedself._toolstoCopilotClient, ignoring any tools that context providers added toSessionContextduringbefore_run. The fix mergessession_context.toolsinto the runtime options dict before session creation in both the synchronous and streamingrunpaths, and updates_create_session/_resume_sessionto combine constructor tools with the runtime-option tools. Tests verify that provider-contributed tools appear in session creation for both non-streaming and streaming runs, and that they merge correctly with constructor-supplied tools.Contribution Checklist