feat: add onListModels handler to CopilotClientOptions for BYOK mode#730
feat: add onListModels handler to CopilotClientOptions for BYOK mode#730
Conversation
✅ Cross-SDK Consistency ReviewI've reviewed PR #730 for consistency across all 4 SDK implementations. Overall, this PR maintains excellent cross-SDK consistency! Feature Parity SummaryAll 4 SDKs (Node.js, Python, Go, .NET) implement the same feature with:
API Naming ConsistencyThe naming follows proper language conventions:
Handler Signature Differences (Expected & Appropriate)Each language uses idiomatic patterns:
These differences are appropriate because:
Test CoverageAll SDKs include tests for:
Minor observation: Go and .NET only have 2 tests each (basic + caching), while Node.js and Python have 3 (adding explicit async handler tests). This is fine since Go's signature is always async with context, and .NET's signature is always async with Task, so there's no sync/async distinction to test. DocumentationThe Verdict: No consistency issues found. This PR successfully adds the
|
There was a problem hiding this comment.
Pull request overview
Adds a client-level onListModels override across the Node, Python, Go, and .NET SDKs so BYOK users can provide their own ModelInfo[] without calling the CLI models.list RPC.
Changes:
- Added an optional
onListModels/on_list_modelshandler to client options types in all SDKs. - Updated each SDK’s
listModels()/list_models()to use the handler (with existing caching/locking behavior) instead of RPC when set. - Added unit tests and BYOK documentation examples for the new handler.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| python/test_client.py | Adds tests covering custom on_list_models handler behavior. |
| python/copilot/types.py | Adds on_list_models to CopilotClientOptions. |
| python/copilot/client.py | Implements handler short-circuit in list_models() with caching. |
| nodejs/test/client.test.ts | Adds tests for onListModels handler usage and caching. |
| nodejs/src/types.ts | Adds onListModels to CopilotClientOptions. |
| nodejs/src/client.ts | Implements handler short-circuit in listModels() with caching. |
| go/types.go | Adds OnListModels to ClientOptions (and imports context). |
| go/client_test.go | Adds tests for OnListModels handler usage and caching. |
| go/client.go | Implements handler short-circuit in ListModels() with caching. |
| dotnet/test/ClientTests.cs | Adds tests for OnListModels handler usage and caching. |
| dotnet/src/Types.cs | Adds OnListModels to CopilotClientOptions and copies it in clone ctor. |
| dotnet/src/Client.cs | Implements handler short-circuit in ListModelsAsync() with caching. |
| docs/auth/byok.md | Documents “Custom Model Listing” with examples for all SDKs. |
Comments suppressed due to low confidence (3)
docs/auth/byok.md:373
- The Go example uses
ModelCapabilitiesSupports/ModelCapabilitiesLimits, but the Go SDK types areModelSupports/ModelLimits. Update the snippet to use the actual exported types so it compiles.
```go
client := copilot.NewClient(&copilot.ClientOptions{
OnListModels: func(ctx context.Context) ([]copilot.ModelInfo, error) {
return []copilot.ModelInfo{
{
ID: "my-custom-model",
Name: "My Custom Model",
Capabilities: copilot.ModelCapabilities{
Supports: copilot.ModelCapabilitiesSupports{Vision: false, ReasoningEffort: false},
Limits: copilot.ModelCapabilitiesLimits{MaxContextWindowTokens: 128000},
},
},
python/copilot/client.py:942
- When
on_list_modelsis used, the returned list instance is stored directly in_models_cache. Since callers often keep a reference to the list they returned, later mutations can unexpectedly change the cached value (unlike the RPC path, which builds a fresh list). Consider caching a shallow copy (e.g.,list(models)) to keep the cache isolated from external mutation.
# Update cache before releasing lock
self._models_cache = models
docs/auth/byok.md:352
- The Python example uses type names
ModelCapabilitiesSupports/ModelCapabilitiesLimits, but the Python SDK types areModelSupports/ModelLimits(as used in tests). As written, the snippet won’t run; update the imports and constructor calls to the correct type names.
```python
from copilot import CopilotClient
from copilot.types import ModelInfo, ModelCapabilities, ModelCapabilitiesSupports, ModelCapabilitiesLimits
client = CopilotClient({
"on_list_models": lambda: [
ModelInfo(
id="my-custom-model",
name="My Custom Model",
capabilities=ModelCapabilities(
supports=ModelCapabilitiesSupports(vision=False, reasoning_effort=False),
limits=ModelCapabilitiesLimits(max_context_window_tokens=128000),
),
)
docs/auth/byok.md
Outdated
| Supports = new ModelCapabilitiesSupports { Vision = false, ReasoningEffort = false }, | ||
| Limits = new ModelCapabilitiesLimits { MaxContextWindowTokens = 128000 } |
There was a problem hiding this comment.
The .NET example uses ModelCapabilitiesSupports / ModelCapabilitiesLimits, but the public SDK types are ModelSupports / ModelLimits (see dotnet/src/Types.cs). As written, the snippet won’t compile unless consumers reference internal/generated RPC types; update to the public types used elsewhere in the SDK/tests.
This issue also appears in the following locations of the same file:
- line 362
- line 339
| Supports = new ModelCapabilitiesSupports { Vision = false, ReasoningEffort = false }, | |
| Limits = new ModelCapabilitiesLimits { MaxContextWindowTokens = 128000 } | |
| Supports = new ModelSupports { Vision = false, ReasoningEffort = false }, | |
| Limits = new ModelLimits { MaxContextWindowTokens = 128000 } |
nodejs/src/client.ts
Outdated
| } | ||
|
|
||
| // Update cache before releasing lock | ||
| this.modelsCache = models; |
There was a problem hiding this comment.
listModels() caches the array returned from onListModels directly (this.modelsCache = models). Since handler implementations often return an array they also retain/mutate, this makes the cache externally mutable (unlike the RPC path, where the SDK owns the parsed array). Consider storing a shallow copy in the cache (and/or copying before caching) to keep cache contents stable.
| this.modelsCache = models; | |
| this.modelsCache = [...models]; |
| expect(handler).toHaveBeenCalledTimes(1); | ||
| expect(models).toEqual(customModels); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The new onListModels tests always call client.start(), but the PR description states no CLI connection should be required when the handler is provided. Adding a test that calls listModels() without start() would prevent regressions where listModels() accidentally starts/requires the CLI even with onListModels set.
| it("does not require client.start when onListModels is provided", async () => { | |
| const customModels: ModelInfo[] = [ | |
| { | |
| id: "no-start-model", | |
| name: "No Start Model", | |
| capabilities: { | |
| supports: { vision: false, reasoningEffort: false }, | |
| limits: { max_context_window_tokens: 128000 }, | |
| }, | |
| }, | |
| ]; | |
| const handler = vi.fn().mockReturnValue(customModels); | |
| const client = new CopilotClient({ onListModels: handler }); | |
| const models = await client.listModels(); | |
| expect(handler).toHaveBeenCalledTimes(1); | |
| expect(models).toEqual(customModels); | |
| }); |
| [Fact] | ||
| public async Task ListModels_WithCustomHandler_CallsHandler() | ||
| { | ||
| var customModels = new List<ModelInfo> | ||
| { | ||
| new() | ||
| { | ||
| Id = "my-custom-model", | ||
| Name = "My Custom Model", | ||
| Capabilities = new ModelCapabilities | ||
| { | ||
| Supports = new ModelSupports { Vision = false, ReasoningEffort = false }, | ||
| Limits = new ModelLimits { MaxContextWindowTokens = 128000 } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| var callCount = 0; | ||
| await using var client = new CopilotClient(new CopilotClientOptions | ||
| { | ||
| OnListModels = (ct) => | ||
| { | ||
| callCount++; | ||
| return Task.FromResult(customModels); | ||
| } | ||
| }); | ||
| await client.StartAsync(); | ||
|
|
||
| var models = await client.ListModelsAsync(); | ||
| Assert.Equal(1, callCount); | ||
| Assert.Single(models); | ||
| Assert.Equal("my-custom-model", models[0].Id); | ||
| } |
There was a problem hiding this comment.
The new tests exercise OnListModels, but they still call StartAsync(). Since the intended behavior is that ListModelsAsync() works without a CLI connection when OnListModels is provided, add a test that calls ListModelsAsync() before StartAsync() to lock in that guarantee.
| finally: | ||
| await client.force_stop() | ||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
These tests call await client.start() even though the documented behavior for on_list_models is that list_models() should not require a CLI connection when the handler is present. Adding a test that calls await client.list_models() without calling start() would help prevent regressions.
| @pytest.mark.asyncio | |
| @pytest.mark.asyncio | |
| async def test_list_models_with_custom_handler_without_start(self): | |
| """Test that on_list_models works without starting the CLI connection""" | |
| custom_models = [ | |
| ModelInfo( | |
| id="my-custom-model", | |
| name="My Custom Model", | |
| capabilities=ModelCapabilities( | |
| supports=ModelSupports(vision=False, reasoning_effort=False), | |
| limits=ModelLimits(max_context_window_tokens=128000), | |
| ), | |
| ) | |
| ] | |
| handler_calls = [] | |
| def handler(): | |
| handler_calls.append(1) | |
| return custom_models | |
| client = CopilotClient({"cli_path": CLI_PATH, "on_list_models": handler}) | |
| models = await client.list_models() | |
| assert len(handler_calls) == 1 | |
| assert models == custom_models | |
| @pytest.mark.asyncio |
python/copilot/client.py
Outdated
| if self._on_list_models: | ||
| # Use custom handler instead of CLI RPC | ||
| result = self._on_list_models() | ||
| if asyncio.iscoroutine(result) or asyncio.isfuture(result): |
There was a problem hiding this comment.
list_models() checks asyncio.iscoroutine()/isfuture() to decide whether to await the on_list_models handler result, but the option type allows any Awaitable. For consistency with the rest of the codebase (e.g., tool/permission handlers), use inspect.isawaitable(result) instead so custom awaitables are handled correctly.
| if asyncio.iscoroutine(result) or asyncio.isfuture(result): | |
| if inspect.isawaitable(result): |
| var models []ModelInfo | ||
| if c.onListModels != nil { | ||
| // Use custom handler instead of CLI RPC | ||
| var err error | ||
| models, err = c.onListModels(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| if c.client == nil { | ||
| return nil, fmt.Errorf("client not connected") | ||
| } | ||
| // Cache miss - fetch from backend while holding lock | ||
| result, err := c.client.Request("models.list", listModelsRequest{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var response listModelsResponse | ||
| if err := json.Unmarshal(result, &response); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal models response: %w", err) | ||
| var response listModelsResponse | ||
| if err := json.Unmarshal(result, &response); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal models response: %w", err) | ||
| } | ||
| models = response.Models | ||
| } | ||
|
|
||
| // Update cache before releasing lock | ||
| c.modelsCache = response.Models | ||
| c.modelsCache = models | ||
|
|
There was a problem hiding this comment.
When OnListModels is set, the returned slice is stored directly in c.modelsCache. If the handler returns a slice backed by an array it later mutates, the cached value can change unexpectedly. Consider copying the slice before caching (e.g., allocate a new slice and copy) so cache contents are owned by the client, matching the RPC path behavior.
See below for a potential fix:
// Update cache with a copy before releasing lock so the cache owns its data
cache := make([]ModelInfo, len(models))
copy(cache, models)
c.modelsCache = cache
// Return a copy to prevent callers from mutating the cached slice
result := make([]ModelInfo, len(cache))
copy(result, cache)
| List<ModelInfo> models; | ||
| if (_onListModels is not null) | ||
| { | ||
| // Use custom handler instead of CLI RPC | ||
| models = await _onListModels(cancellationToken); | ||
| } | ||
| else | ||
| { | ||
| var connection = await EnsureConnectedAsync(cancellationToken); | ||
|
|
||
| // Cache miss - fetch from backend while holding lock | ||
| var response = await InvokeRpcAsync<GetModelsResponse>( | ||
| connection.Rpc, "models.list", [], cancellationToken); | ||
| models = response.Models; | ||
| } | ||
|
|
||
| // Update cache before releasing lock | ||
| _modelsCache = response.Models; | ||
| _modelsCache = models; | ||
|
|
||
| return [.. response.Models]; // Return a copy to prevent cache mutation | ||
| return [.. models]; // Return a copy to prevent cache mutation |
There was a problem hiding this comment.
In the custom handler path, the list returned by _onListModels is assigned directly to _modelsCache. Because the caller likely owns that List<ModelInfo> instance, later mutations can affect cached results unexpectedly. Consider caching a copy (e.g., models.ToList()) so the cache is isolated from external changes, consistent with the RPC path.
patniko
force-pushed
the
feature/on-list-models-handler
branch
2 times, most recently
from
7b9062a to
bfe84d8
Compare
Add an optional onListModels handler to CopilotClientOptions across all 4 SDKs (Node, Python, Go, .NET). When provided, client.listModels() calls the handler instead of sending the models.list RPC to the CLI server. This enables BYOK users to return their provider's available models in the standard ModelInfo format. - Handler completely replaces CLI RPC when set (no fallback) - Results cached identically to CLI path (same locking/thread-safety) - No connection required when handler is provided - Supports both sync and async handlers - 10 new unit tests across all SDKs - Updated BYOK docs with usage examples in all 4 languages Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
patniko
force-pushed
the
feature/on-list-models-handler
branch
from
bfe84d8 to
37de126
Compare
✅ Cross-SDK Consistency Review: PASSEDI've completed a thorough review of PR #730 for cross-language consistency. This PR adds the Summary of ChangesAll four SDKs (Node.js, Python, Go, .NET) receive:
API Naming ConsistencyThe naming follows proper language conventions:
Handler SignaturesEach language uses idiomatic patterns for sync/async support:
The differences in async handling are intentional and idiomatic to each language's patterns. Test CoverageAll SDKs include appropriate test coverage:
Implementation ConsistencyAll four implementations:
DocumentationThe BYOK documentation ( No consistency issues found. This PR maintains excellent feature parity across all SDK implementations. 🎉
|
Summary
Adds an optional
onListModelshandler toCopilotClientOptionsacross all 4 SDKs. When provided,client.listModels()calls the handler instead of sending themodels.listRPC to the CLI server. This enables BYOK users to return their provider's available models in the standardModelInfoformat.Closes #729
Changes
All 4 SDKs (Node, Python, Go, .NET):
CopilotClientOptionsonListModels?: () => Promise<ModelInfo[]> | ModelInfo[]on_list_models: Callable[[], list[ModelInfo] | Awaitable[list[ModelInfo]]]OnListModels func(ctx context.Context) ([]ModelInfo, error)Func<CancellationToken, Task<List<ModelInfo>>>? OnListModelslistModels()checks for handler before RPC. When set, handler completely replaces the CLI call. Same caching/locking/thread-safety.Docs: Added "Custom Model Listing" section to
docs/auth/byok.mdwith examples in all 4 languages.Design Decisions
listModels()is a client method