Skip to content

Fixes #250 - All SDKs Validate Session Config Model#378

Open
john-mckillip wants to merge 12 commits intogithub:mainfrom
john-mckillip:bugfix/dotnet-sdk-validate-session-config-model
Open

Fixes #250 - All SDKs Validate Session Config Model#378
john-mckillip wants to merge 12 commits intogithub:mainfrom
john-mckillip:bugfix/dotnet-sdk-validate-session-config-model

Conversation

@john-mckillip
Copy link

@john-mckillip john-mckillip commented Feb 5, 2026 •
edited
Loading

This pull request introduces model validation when creating a session in the Copilot SDK clients across all supported languages (Node.js, Python, Go, and .NET). Now, if a user attempts to create a session with a model name that does not exist, a clear and descriptive error is thrown, listing available models. Additionally, comprehensive tests have been added to ensure this validation works as intended. Some test cases have also been updated to use a real model name instead of a placeholder.

Model validation and error handling improvements:

  • All SDK clients (nodejs, python, go, dotnet) now validate the model parameter in createSession/CreateSessionAsync/CreateSession methods. If an invalid model is specified, an informative error is thrown, listing available models. [1] [2] [3] [4]

Testing enhancements:

  • Added new unit tests in all SDKs to verify that creating a session with an invalid model raises the appropriate error and that the error message includes both "Invalid model" and the invalid model name. [1] [2] [3] [4]

Test updates for valid model usage:

  • Updated end-to-end and integration tests in all SDKs to use the real model name "claude-sonnet-4.5" instead of the placeholder "fake-test-model". [1] [2] [3] [4] [5]

Utility improvements (Go SDK):

  • Added a helper function contains for case-insensitive substring checks in Go tests.

These changes improve the robustness and user experience of the Copilot SDKs by ensuring that only valid models can be used when creating sessions and by providing clear feedback when an invalid model is specified.

@john-mckillip
Add model validation to CreateSessionAsync
596500a 
Validate that SessionConfig.Model is a valid model name before creating
a session. When an invalid model is specified, throw an ArgumentException
with the invalid model name and list of available models.

Fixes github#250
@john-mckillip
Removed temp code I added for debugging.
af07711
@john-mckillip john-mckillip requested a review from a team as a code owner February 5, 2026 02:46
Copilot AI review requested due to automatic review settings February 5, 2026 02:46
Copilot AI reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds model validation to the .NET SDK's CreateSessionAsync method to ensure that only valid model IDs are used when creating sessions. The implementation throws an ArgumentException with a helpful error message listing available models when an invalid model is specified.

Changes:

  • Added validation logic in Client.cs to verify the model ID exists in the list of available models before creating a session
  • Updated existing test to use a valid model name (claude-sonnet-4.5) instead of a placeholder
  • Added new test to verify that invalid model names trigger an ArgumentException with appropriate error details

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
dotnet/src/Client.cs Implements model validation by checking the provided model against available models and throwing ArgumentException if invalid
dotnet/test/SessionTests.cs Updates test with valid model name and adds new test case for invalid model validation

@john-mckillip john-mckillip mentioned this pull request Feb 5, 2026
Open
@john-mckillip
Optimize model validation with HashSet for O(1) lookup
2a64e9d
@john-mckillip
Added a comment explaining that ListModelsAsync caches its results, c…
e747e18 
…larifying the minimal performance overhead
@john-mckillip
Add model validation to create_session
285e803 
Validate that the model specified in SessionConfig exists in the
list of available models before creating a session. Raises
ValueError with the invalid model name and available alternatives.

Uses list_models() which caches results after the first call,
so validation adds minimal overhead.

Fixes github#250
@john-mckillip
Add model validation to Go SDK CreateSession
8f49e1d 
Validate that the specified model exists in the available models list
when creating a new session. This brings the Go SDK in line with the
Python and .NET implementations.

Changes:
- Add model validation in Client.CreateSession that checks against
  ListModels output using case-insensitive comparison
- Return descriptive error listing available models on validation failure
- Add TestClient_CreateSession_WithInvalidModel unit test
- Refactor to use strings.ToLower and strings.Join directly for performance
- Reuse cached model list from ListModels to minimize overhead

The validation ensures users receive immediate feedback if they specify
an invalid model name, rather than encountering errors later in the
session lifecycle.
@john-mckillip
Add model validation to Node.js SDK createSession
3031fec 
Validate that the specified model exists in the available models list
when creating a new session. This brings the Node.js SDK in line with
the Python, .NET, and Go implementations.

Changes:
- Add model validation in CopilotClient.createSession that checks
  against listModels output using case-insensitive comparison
- Throw descriptive error listing available models on validation failure
- Add unit test "throws error when creating session with invalid model"
  in client.test.ts
- Optimize performance using Array.some() for short-circuit search and
  single lowercase conversion of input model
- Reuse cached model list from listModels() to minimize overhead

The validation ensures users receive immediate feedback if they specify
an invalid model name, rather than encountering errors later in the
session lifecycle.
@john-mckillip john-mckillip marked this pull request as draft February 6, 2026 00:46
@john-mckillip john-mckillip changed the title Fixes #250 - Dotnet SDK Validate Session Config Model Fixes #250 - All SDKs Validate Session Config Model Feb 6, 2026
@john-mckillip john-mckillip marked this pull request as ready for review February 6, 2026 00:54
@john-mckillip john-mckillip requested a review from Copilot February 6, 2026 00:57
Copilot AI reviewed Feb 6, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

@john-mckillip john-mckillip requested a review from Copilot February 6, 2026 01:08
Copilot AI reviewed Feb 6, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

@john-mckillip john-mckillip requested a review from Copilot February 6, 2026 01:13
Copilot AI reviewed Feb 6, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

@john-mckillip
Refactor: use LINQ Any() for model validation in CreateSessionAsync
c1fed7d 
Replace HashSet-based model validation with a more idiomatic LINQ Any()
expression using case-insensitive string comparison. This improves
readability and aligns with other SDK implementations.

- Remove intermediate HashSet creation and validModelIds variable
- Use string.Equals with StringComparison.OrdinalIgnoreCase for consistency
- Update error message to use availableModels.Select(m => m.Id) inline
@john-mckillip john-mckillip requested a review from Copilot February 6, 2026 01:21
Copilot AI reviewed Feb 6, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

dotnet/src/Client.cs
{
throw new ArgumentException(
$"Invalid model '{config.Model}'. Available models: {string.Join(", ", availableModels.Select(m => m.Id))}",
nameof(config));
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nameof(config) should be nameof(config.Model) since the validation is specifically for the Model property, not the entire config object. This provides more precise error information to the caller.

Suggested change
nameof(config));
nameof(config.Model));

Copilot uses AI. Check for mistakes.
Copy link
Author

@john-mckillip john-mckillip Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! However, I need to push back on this suggestion. The ArgumentException
constructor's paramName parameter should reference the actual method parameter name, not a
property of that parameter.

According to Microsoft's ArgumentException documentation,
the paramName should be "the name of the parameter that caused the current exception."

In this case:

  • The method parameter is config (of type SessionConfig?)
  • There is no parameter named Model in the method signature
  • Using nameof(config.Model) causes SonarQube warning: "The parameter name 'Model' is not
    declared in the argument list"

The error message itself already clearly indicates which property is invalid:
"Invalid model '{config.Model}'. Available models: ..."

This is consistent with standard .NET practices where you reference the parameter, and the
error message provides the specific details about what's wrong with it.

@friggeri
Copy link
Collaborator

friggeri commented Feb 6, 2026

I'm worried about the interaction between this and BYOK. @jmoseley is this something you can look at?

@jmoseley
Copy link
Contributor

jmoseley commented Feb 6, 2026

I agree with @friggeri . We cannot assume that, for the BYOK scenario, listModels will return anything or even work.

Can you help me understand the need here?

@john-mckillip
Copy link
Author

john-mckillip commented Feb 6, 2026

The original intent for using listModels was to validate the model being passed when a session is created. The current functionality is a bit confusing because it allows you to pass anything for the model name. I didn't consider BYOK at all. It makes sense that my fix here would not work if that is being used. I apologize if I wasted anyone's time with this PR.

@john-mckillip
Copy link
Author

john-mckillip commented Feb 7, 2026

Hello again. Thinking about this more. I could check if the ProviderConfig has a value and ensure that BYOK is not being utilized before calling listModels and validating the session config model that is set. Let the other providers handle the validation on their end. Let me know if that sounds viable!

@beatdroppaelite54321-lang
Copy link

beatdroppaelite54321-lang commented Feb 9, 2026 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@john-mckillip @friggeri @jmoseley @beatdroppaelite54321-lang