Repository navigation
.NET: Fix CopySessionConfig() and CopyResumeSessionConfig() to preserve SessionConfig.Streaming value - #6463
Conversation
…value (microsoft#4732) CopySessionConfig() and CopyResumeSessionConfig() hardcoded Streaming = true, ignoring the caller's explicitly set SessionConfig.Streaming value. This made it impossible to disable streaming when using AsAIAgent() with the GitHub Copilot SDK. Changed both methods to use source.Streaming ?? true (and source?.Streaming ?? true for the nullable overload), preserving the caller's value when set while maintaining backward compatibility by defaulting to true when unset. Co-authored-by: Copilot <[email protected]>
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Fixes GitHubCopilotAgent’s session-config copy helpers so an explicitly configured SessionConfig.Streaming = false is preserved (instead of being overwritten to true), while keeping the historical default behavior of streaming enabled when the caller leaves it unset.
Changes:
- Update
CopySessionConfig()to preservesource.Streaming(defaulting totruewhennull). - Update
CopyResumeSessionConfig()to preservesource.Streaming(defaulting totruewhennull). - Add unit tests covering
Streaming=falsepreservation andStreaming=nulldefaulting behavior for both copy methods.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.GitHub.Copilot/GitHubCopilotAgent.cs | Preserve SessionConfigBase.Streaming when copying configs, defaulting to true only when unset. |
| dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.UnitTests/GitHubCopilotAgentTests.cs | Add regression tests ensuring Streaming is preserved when explicitly disabled and defaults to true when unset. |
Giles Odigwe (giles17)
left a comment
Giles Odigwe (giles17)
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 95%
✓ Correctness
The PR correctly fixes the bug where CopySessionConfig and CopyResumeSessionConfig were hardcoding Streaming=true, ignoring the caller's explicit value. The fix uses null-coalescing (source.Streaming ?? true / source?.Streaming ?? true) which preserves an explicit false, preserves an explicit true, and defaults to true when unset. The Clone() call in CopySessionConfig already copies all properties, but the subsequent assignment correctly handles the null→true default. Tests cover all meaningful cases. No issues found.
✓ Security Reliability
This PR correctly fixes the streaming configuration preservation bug by replacing hardcoded
Streaming = truewith null-coalescing logic (source.Streaming ?? trueandsource?.Streaming ?? true). The change properly preserves an explicitly setfalsewhile defaulting totruewhen unset. No security or reliability concerns: no injection risks, no resource leaks, null handling is correct (CopyResumeSessionConfig usessource?.Streamingfor its nullable parameter), and backward compatibility is maintained for calers that don't explicitly set Streaming.
✓ Test Coverage
The test coverage for this change is thorough. The PR adds four new tests covering both
CopySessionConfigandCopyResumeSessionConfigfor the two key scenarios:Streaming = false(the bug fix) andStreaming = null(backward-compatible default totrue). The existing tests (CopySessionConfig_CopiesAllProperties,CopyResumeSessionConfig_CopiesAllProperties,CopyResumeSessionConfig_WithNullSource_ReturnsDefaults) continue to validate backward compatibility. The explicitStreaming = truecase is not separately tested, but it's logically equivalent to the null-defaults-to-true path from an output perspective, making it a non-gap. Assertions are meaningful and specific (Assert.True/Assert.False on the boolean property).
✓ Failure Modes
The PR correctly fixes the streaming value preservation bug. Both CopySessionConfig (using Clone + null-coalescing override) and CopyResumeSessionConfig (using manual construction with null-conditional + null-coalescing) properly preserve explicit Streaming=false while defaulting to true when unset. No silent failure modes, lost errors, or operational issues found.
✗ Design Approach
I found one blocking design gap. This PR now preserves
Streaming=false, but the agent’s response-conversion path still assumes assistant text was already delivered via delta events. In the non-streaming case enabled by this change, that assumption is false, so final assistant text can be silently dropped instead of surfaced in the response.
Flagged Issues
- Preserving
SessionConfig.Streaming = falseis not end-to-end supported:GitHubCopilotAgent.cs:280and:310now allow non-streaming sessions, butGitHubCopilotAgent.cs:329-342convertsAssistantMessageEventinto rawAIContentonly, andGitHubCopilotAgentTests.cs:291-309explicitly documents that this is safe only because the text was 'already delivered via delta events'. With streaming disabled, that premise no longer holds, so the assistant's final text can be lost.
Automated review by giles17's agents
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…crosoft#4732) The config-copy fix (preserving Streaming=false via null-coalescing) was already in place, but ConvertToAgentResponseUpdate(AssistantMessageEvent) always emitted raw AIContent without text—assuming delta events had already delivered it. When streaming is disabled there are no delta events, so the assistant's final text was silently dropped. Changes: - Add isStreaming parameter to ConvertToAgentResponseUpdate for AssistantMessageEvent so it emits TextContent in non-streaming mode. - Capture the resolved streaming flag in RunCoreStreamingAsync and pass it through the event subscription closure. - Add/update unit tests for both streaming and non-streaming paths. Co-authored-by: Copilot <[email protected]>
GitHub Actions
Inactive
GitHub Actions
Inactive
Giles Odigwe (giles17)
left a comment
Giles Odigwe (giles17)
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 91%
✓ Correctness
The PR correctly adds conditional text content emission based on streaming mode. When streaming, text was already delivered via delta events so only raw metadata is emitted (avoiding duplicates). When not streaming, the full message text is properly emitted as TextContent. The isStreaming variable is correctly derived from the resolved sessionConfig.Streaming value, captured safely by the event subscription closure, and the null-conditional handling of assistantMessage.Data?.Content is correct. No correctness issues found.
✓ Security Reliability
The PR cleanly adds non-streaming support to ConvertToAgentResponseUpdate by threading an isStreaming boolean derived from the session config. Null handling is defensive (Data?.Content ?? string.Empty), the captured variable in the closure is a value type so there's no race condition, and tests cover both streaming and non-streaming paths including the empty-content edge case. No security or reliability issues found.
✓ Test Coverage
The PR adds good test coverage for the new
isStreamingparameter onConvertToAgentResponseUpdate. The streaming path, non-streaming path, and empty-content edge case are all tested with meaningful assertions (content equality, type checks, reference identity). One minor gap: the production code handlesassistantMessage.Data?.Content ?? string.Empty(null-coalescing onData), but no test exercises theData = nullscenario in non-streaming mode. This is a low-severity gap since the null-coalescing ensures it won't crash, but a test would document the expected behavior.
✓ Failure Modes
The PR correctly handles the streaming/non-streaming distinction for AssistantMessageEvent conversion. The
isStreamingflag is derived from the same session config sent to the server, null handling is proper throughout (Data?.Content ?? string.Empty), and the code paths for both modes are well-tested. No concrete failure modes or silent error paths were identified.
✓ Design Approach
I did not find a design-approach issue in this diff. The change is narrowly scoped to the demonstrated bug:
AgentResponseUpdate.Textis defined as the concatenation ofTextContentitems inContents(dotnet/src/Microsoft.Agents.AI.Abstractions/AgentResponseUpdate.cs:91-97), so emittingTextContentonly for non-streaming assistant messages is consistent with the framework contract and with the new tests. I also did not find repo evidence that a better pattern should be used here instead of keying off the session's streaming configuration.
Automated review by giles17's agents
…t#4732) Add a regression test covering the null-propagation path where AssistantMessageEvent.Data is null. The production code already handles this via ?. operators, but no test previously verified the behavior. Co-authored-by: Copilot <[email protected]>
GitHub Actions
Inactive
GitHub Actions
Inactive
Giles Odigwe (giles17)
left a comment
Giles Odigwe (giles17)
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 94% | Result: All clear
Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach
Automated review by giles17's agents
Motivation and Context
CopySessionConfig()andCopyResumeSessionConfig()inGitHubCopilotAgenthardcodedStreaming = true, making it impossible to disable streaming when usingAsAIAgent()with the GitHub Copilot SDK even thoughSessionConfig.Streamingis a settable property.Fixes #4732
Description
The root cause was that both copy methods unconditionally set
Streaming = trueon the output config instead of propagating the caller's value. The fix changes both methods to usesource.Streaming ?? true, preserving an explicitly setfalsevalue while defaulting totruewhen unset (maintaining backward compatibility). Unit tests are added to verify thatStreaming = falseis preserved and that a null value defaults totruefor bothCopySessionConfigandCopyResumeSessionConfig.Contribution Checklist