-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add session.setModel() for mid-session model switching #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
a0367b8
6936cce
b91f69c
03467b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -576,3 +576,20 @@ func (s *Session) Abort(ctx context.Context) error { | |
|
|
||
| return nil | ||
| } | ||
|
|
||
| // SetModel changes the model for this session. | ||
| // The new model takes effect for the next message. Conversation history is preserved. | ||
| // | ||
| // Example: | ||
| // | ||
| // if err := session.SetModel(context.Background(), "gpt-4.1"); err != nil { | ||
| // log.Printf("Failed to set model: %v", err) | ||
| // } | ||
| func (s *Session) SetModel(ctx context.Context, model string) error { | ||
| _, err := s.RPC.Model.SwitchTo(ctx, &rpc.SessionModelSwitchToParams{ModelID: model}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to set model: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage gap: The Go SDK is missing a test for Consider adding a test in Example pattern (based on Python/Node.js tests): func TestSession_SetModel(t *testing.T) {
// Mock the RPC call and verify session.model.switchTo is called
// with correct sessionId and modelId parameters
}This would ensure cross-SDK test parity and prevent regressions.
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Can you find some way to make this test show the model was changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in b91f69c. The test now subscribes for
SessionModelChangeEventbefore callingSetModelAsync, then assertsmodelChanged.Data.NewModel == "gpt-4.1". This uses the existingTestHelper.GetNextEventOfTypeAsyncinfrastructure.Note: the test will still fail if the CLI doesn't emit
session.model_changein response tosession.model.switchTo. The CLI-side handler is a pre-existing gap (same reasonRpcTests.Should_Call_Session_Rpc_Model_SwitchTois skipped). Without a working server-side handler, there's no way to verify the model was changed using existing test infrastructure.