Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions dotnet/src/Client.cs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,20 @@ public async Task<CopilotSession> CreateSessionAsync(SessionConfig? config = nul
{
var connection = await EnsureConnectedAsync(cancellationToken);

if (!string.IsNullOrEmpty(config?.Model))
{
// ListModelsAsync caches results after the first call, so this validation has minimal overhead
var availableModels = await ListModelsAsync(cancellationToken).ConfigureAwait(false);
var validModelIds = new HashSet<string>(availableModels.Select(m => m.Id), StringComparer.OrdinalIgnoreCase);

if (!validModelIds.Contains(config.Model))
{
throw new ArgumentException(
$"Invalid model '{config.Model}'. Available models: {string.Join(", ", validModelIds)}",
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

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.

}
}

var hasHooks = config?.Hooks != null && (
config.Hooks.OnPreToolUse != null ||
config.Hooks.OnPostToolUse != null ||
Expand Down
17 changes: 16 additions & 1 deletion dotnet/test/SessionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public class SessionTests(E2ETestFixture fixture, ITestOutputHelper output) : E2
[Fact]
public async Task ShouldCreateAndDestroySessions()
{
var session = await Client.CreateSessionAsync(new SessionConfig { Model = "fake-test-model" });
var session = await Client.CreateSessionAsync(new SessionConfig { Model = "claude-sonnet-4.5" });

Assert.Matches(@"^[a-f0-9-]+$", session.SessionId);

Expand Down Expand Up @@ -395,4 +395,19 @@ public async Task Should_Create_Session_With_Custom_Config_Dir()
Assert.NotNull(assistantMessage);
Assert.Contains("2", assistantMessage!.Data.Content);
}

[Fact]
public async Task CreateSessionAsync_WithInvalidModel_ThrowsArgumentException()
{
var exception = await Assert.ThrowsAsync<ArgumentException>(async () =>
{
await Client.CreateSessionAsync(new SessionConfig
{
Model = "INVALID_MODEL_THAT_DOES_NOT_EXIST"
});
});

Assert.Contains("Invalid model", exception.Message);
Assert.Contains("INVALID_MODEL_THAT_DOES_NOT_EXIST", exception.Message);
}
}
24 changes: 24 additions & 0 deletions go/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,30 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
params := make(map[string]any)
if config != nil {
if config.Model != "" {
// Validate model if specified
// ListModels caches results after the first call, so this validation has minimal overhead
availableModels, err := c.ListModels(ctx)
if err != nil {
return nil, fmt.Errorf("failed to list models: %w", err)
}

modelLower := strings.ToLower(config.Model)
modelFound := false
for _, model := range availableModels {
if strings.ToLower(model.ID) == modelLower {
modelFound = true
break
}
}

if !modelFound {
validIDs := make([]string, len(availableModels))
for i, model := range availableModels {
validIDs[i] = model.ID
}
return nil, fmt.Errorf("invalid model '%s'. Available models: %s", config.Model, strings.Join(validIDs, ", "))
}

params["model"] = config.Model
}
if config.SessionID != "" {
Expand Down
38 changes: 38 additions & 0 deletions go/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@ import (
"path/filepath"
"reflect"
"regexp"
"strings"
"testing"
)

// containsIgnoreCase checks if a string contains a substring (case-insensitive)
func containsIgnoreCase(s, substr string) bool {
return strings.Contains(strings.ToLower(s), strings.ToLower(substr))
}

// This file is for unit tests. Where relevant, prefer to add e2e tests in e2e/*.test.go instead

func TestClient_HandleToolCallRequest(t *testing.T) {
Expand Down Expand Up @@ -48,6 +54,38 @@ func TestClient_HandleToolCallRequest(t *testing.T) {
})
}

func TestClient_CreateSession_WithInvalidModel(t *testing.T) {
cliPath := findCLIPathForTest()
if cliPath == "" {
t.Skip("CLI not found")
}

client := NewClient(&ClientOptions{CLIPath: cliPath})
t.Cleanup(func() { client.ForceStop() })

err := client.Start(t.Context())
if err != nil {
t.Fatalf("Failed to start client: %v", err)
}

_, err = client.CreateSession(t.Context(), &SessionConfig{
Model: "INVALID_MODEL_THAT_DOES_NOT_EXIST",
})

if err == nil {
t.Fatal("Expected error when creating session with invalid model, got nil")
}

errorMsg := err.Error()
if !containsIgnoreCase(errorMsg, "invalid model") {
t.Errorf("Expected error message to contain 'invalid model', got: %s", errorMsg)
}

if !containsIgnoreCase(errorMsg, "INVALID_MODEL_THAT_DOES_NOT_EXIST") {
t.Errorf("Expected error message to contain 'INVALID_MODEL_THAT_DOES_NOT_EXIST', got: %s", errorMsg)
}
}

func TestClient_URLParsing(t *testing.T) {
t.Run("should parse port-only URL format", func(t *testing.T) {
client := NewClient(&ClientOptions{
Expand Down
6 changes: 3 additions & 3 deletions go/internal/e2e/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func TestSession(t *testing.T) {
t.Run("should create and destroy sessions", func(t *testing.T) {
ctx.ConfigureForTest(t)

session, err := client.CreateSession(t.Context(), &copilot.SessionConfig{Model: "fake-test-model"})
session, err := client.CreateSession(t.Context(), &copilot.SessionConfig{Model: "claude-sonnet-4.5"})
if err != nil {
t.Fatalf("Failed to create session: %v", err)
}
Expand All @@ -41,8 +41,8 @@ func TestSession(t *testing.T) {
t.Errorf("Expected session.start sessionId to match")
}

if messages[0].Data.SelectedModel == nil || *messages[0].Data.SelectedModel != "fake-test-model" {
t.Errorf("Expected selectedModel to be 'fake-test-model', got %v", messages[0].Data.SelectedModel)
if messages[0].Data.SelectedModel == nil || *messages[0].Data.SelectedModel != "claude-sonnet-4.5" {
t.Errorf("Expected selectedModel to be 'claude-sonnet-4.5', got %v", messages[0].Data.SelectedModel)
}

if err := session.Destroy(); err != nil {
Expand Down
14 changes: 14 additions & 0 deletions nodejs/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,20 @@ export class CopilotClient {
}
}

// Validate model if specified
if (config.model) {
// listModels() caches results, so this has minimal overhead
const availableModels = await this.listModels();
const modelLower = config.model.toLowerCase();

if (!availableModels.some((m) => m.id.toLowerCase() === modelLower)) {
const validIds = availableModels.map((m) => m.id);
throw new Error(
`Invalid model '${config.model}'. Available models: ${validIds.join(", ")}`
);
}
}

const response = await this.connection!.sendRequest("session.create", {
model: config.model,
sessionId: config.sessionId,
Expand Down
14 changes: 14 additions & 0 deletions nodejs/test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ describe("CopilotClient", () => {
});
});

it("throws error when creating session with invalid model", async () => {
const client = new CopilotClient({ cliPath: CLI_PATH });
await client.start();
onTestFinished(() => client.forceStop());

const error = await client
.createSession({ model: "INVALID_MODEL_THAT_DOES_NOT_EXIST" })
.catch((e) => e);

expect(error).toBeInstanceOf(Error);
expect(error.message).toContain("Invalid model");
expect(error.message).toContain("INVALID_MODEL_THAT_DOES_NOT_EXIST");
});

describe("URL parsing", () => {
it("should parse port-only URL format", () => {
const client = new CopilotClient({
Expand Down
4 changes: 2 additions & 2 deletions nodejs/test/e2e/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ describe("Sessions", async () => {
const { copilotClient: client, openAiEndpoint, homeDir, env } = await createSdkTestContext();

it("should create and destroy sessions", async () => {
const session = await client.createSession({ model: "fake-test-model" });
const session = await client.createSession({ model: "claude-sonnet-4.5" });
expect(session.sessionId).toMatch(/^[a-f0-9-]+$/);

expect(await session.getMessages()).toMatchObject([
{
type: "session.start",
data: { sessionId: session.sessionId, selectedModel: "fake-test-model" },
data: { sessionId: session.sessionId, selectedModel: "claude-sonnet-4.5" },
},
]);

Expand Down
13 changes: 13 additions & 0 deletions python/copilot/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,19 @@ async def create_session(self, config: Optional[SessionConfig] = None) -> Copilo

cfg = config or {}

# Validate model if specified
model = cfg.get("model")
if model:
# list_models() caches results, so this has minimal overhead
available_models = await self.list_models()
model_lower = model.lower()

if not any(m.id.lower() == model_lower for m in available_models):
valid_ids = [m.id for m in available_models]
raise ValueError(
f"Invalid model '{model}'. Available models: {', '.join(valid_ids)}"
)

tool_defs = []
tools = cfg.get("tools")
if tools:
Expand Down
11 changes: 9 additions & 2 deletions python/e2e/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@

class TestSessions:
async def test_should_create_and_destroy_sessions(self, ctx: E2ETestContext):
session = await ctx.client.create_session({"model": "fake-test-model"})
session = await ctx.client.create_session({"model": "claude-sonnet-4.5"})
assert session.session_id

messages = await session.get_messages()
assert len(messages) > 0
assert messages[0].type.value == "session.start"
assert messages[0].data.session_id == session.session_id
assert messages[0].data.selected_model == "fake-test-model"
assert messages[0].data.selected_model == "claude-sonnet-4.5"

await session.destroy()

Expand Down Expand Up @@ -456,6 +456,13 @@ def on_event(event):
assistant_message = await get_final_assistant_message(session)
assert "300" in assistant_message.data.content

async def test_create_session_with_invalid_model_raises_value_error(self, ctx: E2ETestContext):
with pytest.raises(ValueError) as exc_info:
await ctx.client.create_session({"model": "INVALID_MODEL_THAT_DOES_NOT_EXIST"})

assert "Invalid model" in str(exc_info.value)
assert "INVALID_MODEL_THAT_DOES_NOT_EXIST" in str(exc_info.value)

async def test_should_create_session_with_custom_config_dir(self, ctx: E2ETestContext):
import os

Expand Down