Python: Add AssistantReasoningEvent and AssistantReasoningDeltaEvent support to GitHubCopilotAgent#3566
Python: Add AssistantReasoningEvent and AssistantReasoningDeltaEvent support to GitHubCopilotAgent#3566
Conversation
…to GitHubCopilotAgent Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for GitHub Copilot SDK's AssistantReasoningEvent and AssistantReasoningDeltaEvent to properly handle and preserve model reasoning/thinking output as TextReasoningContent instead of losing it or storing it as generic AIContent.
Changes:
- Added event handlers for reasoning events in both C# and Python implementations
- Created conversion methods to map reasoning events to
TextReasoningContent - Added comprehensive unit tests covering reasoning event handling and null safety
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.GitHub.Copilot/GitHubCopilotAgent.cs | Added case handlers and conversion methods for AssistantReasoningDeltaEvent and AssistantReasoningEvent to create TextReasoningContent |
| dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.UnitTests/GitHubCopilotAgentReasoningTests.cs | Added 6 unit tests covering successful conversion of reasoning events and null handling scenarios |
| python/packages/github_copilot/agent_framework_github_copilot/_agent.py | Added handlers for reasoning event types to create AgentResponseUpdate with TextReasoningContent |
| python/packages/github_copilot/tests/test_github_copilot_agent.py | Added test fixtures and 2 streaming tests for reasoning delta and complete events |
Comments suppressed due to low confidence (3)
python/packages/github_copilot/agent_framework_github_copilot/_agent.py:468
Content.from_text_reasoning()only accepts keyword arguments (note the*,in its signature at python/packages/core/agent_framework/_types.py:533). This call passes a positional argument and will fail with a TypeError. Change toContent.from_text_reasoning(text=event.data.content).
contents=[Content.from_text_reasoning(event.data.content)],
python/packages/github_copilot/tests/test_github_copilot_agent.py:531
Roleis not imported in this test file, which will cause aNameErrorwhen this test runs. Either importRolefromagent_framework, or use the string literal"assistant"directly as per the Role type documentation (python/packages/core/agent_framework/_types.py:1354-1373).
assert responses[0].role == Role.ASSISTANT
python/packages/github_copilot/agent_framework_github_copilot/_agent.py:467
Roleis not imported in this file, which will cause aNameErrorat runtime. The existing code uses string literals like"assistant"(see line 449). ChangeRole.ASSISTANTto"assistant"to match the existing pattern.
role=Role.ASSISTANT,
| if event.data.delta_content: | ||
| update = AgentResponseUpdate( | ||
| role=Role.ASSISTANT, | ||
| contents=[Content.from_text_reasoning(event.data.delta_content)], |
There was a problem hiding this comment.
Content.from_text_reasoning() only accepts keyword arguments (note the *, in its signature at python/packages/core/agent_framework/_types.py:533). This call passes a positional argument and will fail with a TypeError. Change to Content.from_text_reasoning(text=event.data.delta_content).
This issue also appears on line 468 of the same file.
|
|
||
| assert len(responses) == 1 | ||
| assert isinstance(responses[0], AgentResponseUpdate) | ||
| assert responses[0].role == Role.ASSISTANT |
There was a problem hiding this comment.
Role is not imported in this test file, which will cause a NameError when this test runs. Either import Role from agent_framework, or use the string literal "assistant" directly as per the Role type documentation (python/packages/core/agent_framework/_types.py:1354-1373).
This issue also appears on line 531 of the same file.
| elif event.type == SessionEventType.ASSISTANT_REASONING_DELTA: | ||
| if event.data.delta_content: | ||
| update = AgentResponseUpdate( | ||
| role=Role.ASSISTANT, |
There was a problem hiding this comment.
Role is not imported in this file, which will cause a NameError at runtime. The existing code uses string literals like "assistant" (see line 449). Change Role.ASSISTANT to "assistant" to match the existing pattern.
This issue also appears on line 467 of the same file.
Motivation and Context
GitHub Copilot SDK emits
AssistantReasoningEventandAssistantReasoningDeltaEventfor model reasoning/thinking output. GitHubCopilotAgent was not handling these events, causing reasoning content to be lost or stored as genericAIContentinstead of the semantically correctTextReasoningContent.Description
C# Implementation (
GitHubCopilotAgent.cs)AssistantReasoningDeltaEventandAssistantReasoningEventAssistantReasoningDeltaEvent.Data.DeltaContent→TextReasoningContent(streaming)AssistantReasoningEvent.Data.Content→TextReasoningContent(complete)Python Implementation (
_agent.py)SessionEventType.ASSISTANT_REASONING_DELTAandSessionEventType.ASSISTANT_REASONINGAgentResponseUpdatewithContent.from_text_reasoning()Tests
Follows existing event handling patterns (e.g.,
AssistantMessageDeltaEvent→TextContent).Contribution Checklist
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.