Clean up more argument validation#1328
Merged
Merged
Conversation
Keep CopilotSession instances rooted by the owning client until explicit cleanup, route generated session RPC APIs through CopilotSession, and add generated argument validation plus lifetime regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
stephentoub
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET SDK session lifetime model so CopilotClient strongly roots active sessions for event routing/cleanup, and adjusts generated session RPC APIs to operate through CopilotSession.
Changes:
- Adds session registration/removal lifecycle changes in
CopilotClient/CopilotSession. - Regenerates .NET RPC APIs with lazy API groups, session-backed leaf APIs, disposal checks, and required-argument null checks.
- Adds focused .NET unit coverage for session rooting, cleanup, and post-disposal RPC behavior.
Show a summary per file
| File | Description |
|---|---|
scripts/codegen/csharp.ts |
Updates C# RPC generation for lazy API groups, session-backed RPC APIs, and null validation. |
dotnet/src/Generated/Rpc.cs |
Regenerated RPC surface reflecting the codegen changes. |
dotnet/src/Client.cs |
Adds strong session registry semantics and identity-aware session registration/removal helpers. |
dotnet/src/Session.cs |
Stores parent client/session RPC state, adds disposal checks, and removes sessions from the client on cleanup. |
dotnet/src/Polyfills/DownlevelExtensions.cs |
Adds downlevel helpers for disposal and exception dispatch APIs. |
dotnet/test/Unit/ClientSessionLifetimeTests.cs |
Adds regression tests for rooted sessions, disposal cleanup, stop cleanup, and disposed generated RPC calls. |
Copilot's findings
- Files reviewed: 5/6 changed files
- Comments generated: 4
Allow a resumed session to replace the client's current session entry so same-client resumes continue to work while stale session disposal cannot remove the replacement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Restore StopAsync cleanup error propagation, clear any sessions missed by concurrent registration during stop, and remove an unused Session using. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Delay removing sessions from the client until session.destroy completes so destroy-time callbacks such as session_fs can still route to the session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Give mode handler callbacks the same 30 second allowance as their corresponding events to avoid Windows timing failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject same-client resumes for session ids already tracked by the client, and start session event processing only after successful registration so failed duplicate registration does not leave an untracked event loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Update resume-focused E2E tests to use an active session from a second TCP client, while keeping same-client active resume covered as an intentional rejection. Clean up permission resume tests so the resumed handler is the only active handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR contains exclusively .NET SDK internal improvements with no cross-SDK consistency issues. What changed:
Why no cross-SDK action needed:
|
SteveSandersonMS
approved these changes
May 19, 2026
Contributor
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Looks great. I'm guessing there are equivalents to this for other languages?
Collaborator
Author
Yup |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clean up argument validation and disposal handling in CopilotClient/Session and the various generated Rpc wrappers. Also lazily allocate lots of objects.