feat(cli): normalize model list response parsing across OpenAI-compatible endpoints#3799
feat(cli): normalize model list response parsing across OpenAI-compatible endpoints#3799B-A-M-N wants to merge 9 commits into
Conversation
Adds a scriptable /model list subcommand that queries the
configured OpenAI-compatible /models endpoint and prints available
model IDs (one per line) for CLI automation and scripting.
Behavior:
- Uses the active content generator config (baseUrl + apiKey)
- Calls {baseUrl}/models with proper trailing-slash normalization
- Sends Authorization header when an API key is configured
- Prints one model ID per line (pipe-friendly output)
- Preserves existing /model, /model <id>, and /model --fast behavior
Testing notes:
- Verified with Groq API (free tier) - /model list returns model list
- Verified with Hugging Face Inference API - /model list works
- Note: Paid API testing (OpenAI, OpenRouter) was not performed
due to account limitations; the implementation follows the same OpenAI-
compatible pattern and is expected to work with any standards-compliant
/models endpoint.
Build/lint/typecheck: all pass.
Closes QwenLM#2412
wenshao
left a comment
There was a problem hiding this comment.
High: The list subcommand action (lines 156-203) has zero tests — 6 code paths are completely uncovered. Every other command with subcommands in this directory tests their subcommand actions via command.subCommands?.find(...).action!(mockContext). Please add a describe('list subcommand', ...) block covering: config missing, contentGeneratorConfig missing, baseUrl missing, fetchModels success, Error instance throw, and non-Error throw.
| type: 'message', | ||
| messageType: 'error', | ||
| content: 'Content generator configuration not available.', | ||
| }; |
There was a problem hiding this comment.
[Critical] The list subcommand only destructures { baseUrl, apiKey } from ContentGeneratorConfig, ignoring authType, proxy, customHeaders, and timeout.
For QWEN_OAUTH users, apiKey is the sentinel string 'QWEN_OAUTH_DYNAMIC_TOKEN' (set in modelsConfig.ts:718 and modelConfigResolver.ts:362), which gets sent as Authorization: Bearer QWEN_OAUTH_DYNAMIC_TOKEN — guaranteeing a 401. The real token is resolved dynamically by the content generator but fetchModels bypasses that flow entirely.
Enterprise users behind a proxy or requiring custom headers will also see silent failures.
| }; | |
| const { baseUrl, apiKey, authType, proxy, customHeaders, timeout } = | |
| contentGeneratorConfig; | |
| if ( | |
| authType && | |
| authType !== AuthType.USE_OPENAI && | |
| authType !== AuthType.USE_ANTHROPIC | |
| ) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: `Model listing is not supported for auth type: ${authType}. Only OpenAI-compatible endpoints are supported.`, | |
| }; | |
| } |
Then pass proxy, customHeaders, and timeout through to fetchModels.
— deepseek-v4-pro via Qwen Code /review
| * - Standard: { data: [{ id: "qwen-plus" }] } | ||
| * - With object field: { object: "list", data: [{ id: "deepseek-chat", ... }] } | ||
| * - Bare array: [{ id: "model" }] (some providers) | ||
| * Extra fields (owned_by, created, etc.) are ignored. |
There was a problem hiding this comment.
[High] fetch() has no timeout or AbortSignal. If the /models endpoint hangs, the CLI blocks forever with no way to cancel. The codebase already has fetchWithTimeout in packages/core/src/utils/fetch.ts.
| * Extra fields (owned_by, created, etc.) are ignored. | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout( | |
| () => controller.abort(new Error('Request timed out')), | |
| 15_000, | |
| ); | |
| try { | |
| const response = await fetch(url, { | |
| method: 'GET', | |
| headers, | |
| signal: controller.signal, | |
| }); |
Also wire up context.abortSignal so users can cancel with Escape.
— deepseek-v4-pro via Qwen Code /review
| Accept: 'application/json', | ||
| }; | ||
|
|
||
| if (apiKey) { |
There was a problem hiding this comment.
[Suggestion] The full HTTP response body is embedded directly into the Error message via response.text() with no truncation. A 502 HTML error page or XML fault from a proxy could dump hundreds of KB into the terminal.
| if (apiKey) { | |
| const errorText = (await response.text()).slice(0, 500); | |
| throw new Error(`Request failed (${response.status}): ${errorText}`); |
Additionally, wrap this in try/catch — if response.text() itself throws (connection reset mid-stream), the HTTP status code is lost.
— deepseek-v4-pro via Qwen Code /review
| const { config } = services; | ||
|
|
||
| if (!config) { | ||
| return { |
There was a problem hiding this comment.
[Suggestion] The list subcommand uses hardcoded English strings for user-visible errors, while the parent action wraps identical strings with t() for i18n. Affected lines: 162, 169, 177, 199 ('Configuration not available.', 'Content generator configuration not available.', 'No baseUrl configured...', `Failed to fetch models: ${errorMessage}`).
| return { | |
| content: t('Configuration not available.'), |
Also change description to a getter: get description() { return t('List available models...'); }
— deepseek-v4-pro via Qwen Code /review
| it('should include Authorization header when apiKey is provided', async () => { | ||
| const mockResponse = { | ||
| ok: true, | ||
| json: vi.fn().mockResolvedValue({ |
There was a problem hiding this comment.
[Suggestion] Both Authorization header tests only assert expect(fetchSpy).toHaveBeenCalled() without checking the actual headers. The comment "header verification requires complex typing" is incorrect — the URL normalization test on line 319 already uses expect.any(Object).
| json: vi.fn().mockResolvedValue({ | |
| expect(fetchSpy).toHaveBeenCalledWith( | |
| 'https://api.example.com/v1/models', | |
| expect.objectContaining({ | |
| headers: expect.objectContaining({ | |
| Authorization: 'Bearer my-key', | |
| }), | |
| }), | |
| ); |
Do the inverse (checking Authorization is NOT present) for the no-apiKey test.
— deepseek-v4-pro via Qwen Code /review
|
|
||
| // Extract the models array from various response shapes. | ||
| // 1. Standard OpenAI: { data: [{ id: "..." }] } | ||
| // 2. With object field: { object: "list", data: [{ id: "...", owned_by: "...", ... }] } |
There was a problem hiding this comment.
[Suggestion] The model extraction loop silently skips items that are not objects or whose id is not a string. If a provider returns a flat string array (["model-1", "model-2"]) or numeric IDs ({ id: 42 }), the result is an empty array with no diagnostic. Consider logging skipped items or handling typeof item === 'string' as a bare model name.
| // 2. With object field: { object: "list", data: [{ id: "...", owned_by: "...", ... }] } | |
| if (item && typeof item === 'object') { | |
| const model = item as Record<string, unknown>; | |
| const id = model['id']; | |
| if (typeof id === 'string' && id.trim().length > 0) { | |
| modelIds.push(id.trim()); | |
| } | |
| } else if (typeof item === 'string' && item.trim().length > 0) { | |
| modelIds.push(item.trim()); | |
| } |
— deepseek-v4-pro via Qwen Code /review
…ible endpoints
Handle multiple response shapes when fetching from /models endpoint:
- Standard: { data: [{ id: "..." }] }
- With object field: { object: "list", data: [{ id: "...", ... }] }
- Bare array: [{ id: "..." }]
Extra fields (owned_by, created, permission, etc.) are ignored.
Only id (non-empty string) is extracted.
Add 16 tests for response normalization covering all shapes and edge cases.
06e14db to
c9a5459
Compare
|
All review comments have been addressed in the latest push:
Force-pushed updated commit to |
wenshao
left a comment
There was a problem hiding this comment.
Suggestion: Missing test coverage for the new abort/timeout paths in fetchModels (AbortController timeout, AbortSignal.any combination, user-provided abortSignal). No existing test triggers the 15s timeout or passes an aborted signal. Consider adding tests with vi.useFakeTimers() and AbortController to cover these paths.
Reminder: The Critical issue from the prior review still stands — list subcommand destructures only { baseUrl, apiKey } from ContentGeneratorConfig without checking authType. QWEN_OAUTH users will get 401 because apiKey is the sentinel string "QWEN_OAUTH_DYNAMIC_TOKEN".
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| return { | ||
| type: 'message', |
There was a problem hiding this comment.
[Critical] AbortError from timeout/user cancel is indistinguishable from real network errors. The catch block at line 212 treats all errors identically via error instanceof Error ? error.message : String(error). A 15s timeout produces "Failed to fetch models: This operation was aborted" — the same message a user sees for a network failure. Users cannot tell if the endpoint is down, slow, or if they pressed Escape.
| type: 'message', | |
| } catch (error) { | |
| let errorMessage: string; | |
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| const isTimeout = !context.abortSignal?.aborted; | |
| errorMessage = isTimeout | |
| ? t('Request timed out. The endpoint may be slow or unreachable.') | |
| : t('Request cancelled.'); | |
| } else { | |
| errorMessage = error instanceof Error ? error.message : String(error); | |
| } | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: `${t('Failed to fetch models:')} ${errorMessage}`, | |
| }; |
— deepseek-v4-pro via Qwen Code /review
| baseUrl, | ||
| apiKey, | ||
| context.abortSignal, | ||
| ); |
There was a problem hiding this comment.
[Critical] When fetchModels returns an empty array (e.g., endpoint returns {"data": []}), models.join('\n') produces an empty string "". The user sees a blank message with no indication the command ran successfully. Silent failure — impossible to know if the endpoint returned nothing or the command hung.
| ); | |
| if (models.length === 0) { | |
| return { | |
| type: 'message', | |
| messageType: 'info', | |
| content: t('No models returned from the endpoint.'), | |
| }; | |
| } | |
| const output = models.join('\n'); |
— deepseek-v4-pro via Qwen Code /review
| ? AbortSignal.any([timeoutController.signal, abortSignal]) | ||
| : timeoutController.signal; | ||
|
|
||
| try { |
There was a problem hiding this comment.
[Suggestion] clearTimeout(timeoutId) is called both inside the try block (line 260, after fetch succeeds) and in finally (line 305). The try-block call is redundant — finally always runs. Removing it simplifies the code. Other files in the codebase (gitUtils.ts, shell.ts) use AbortSignal.timeout() which avoids manual timer management entirely.
| try { | |
| const response = await fetch(url, { | |
| method: 'GET', | |
| headers, | |
| signal, | |
| }); | |
| if (!response.ok) { |
— deepseek-v4-pro via Qwen Code /review
| method: 'GET', | ||
| headers, | ||
| signal, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] In the !response.ok branch, if response.text() itself throws (e.g., body stream already consumed), the catch block silently discards the original exception and substitutes "(unable to read error response)". The root cause is lost — making debugging harder if this edge case occurs.
| }); | |
| let errorText: string; | |
| try { | |
| errorText = (await response.text()).slice(0, 500); | |
| } catch (e) { | |
| console.error( | |
| `[fetchModels] Unable to read error body (status ${response.status}):`, | |
| e, | |
| ); | |
| errorText = '(unable to read error response)'; | |
| } |
— deepseek-v4-pro via Qwen Code /review
| throw new Error(`Request failed (${response.status}): ${errorText}`); | ||
| } | ||
|
|
||
| const body = (await response.json()) as unknown; |
There was a problem hiding this comment.
[Suggestion] response.json() on the HTTP 200 success path has no try-catch. If the endpoint returns 200 OK but non-JSON body (e.g., proxy returns an HTML login page), response.json() throws SyntaxError with a misleading "Failed to fetch models: Unexpected token '<'..." prefix. The !response.ok branch already has try-catch on response.text() — the success path should have equivalent protection.
| const body = (await response.json()) as unknown; | |
| let body: unknown; | |
| try { | |
| body = await response.json(); | |
| } catch { | |
| throw new Error( | |
| 'Invalid JSON response from /models endpoint. Check that the baseUrl points to a valid OpenAI-compatible API.', | |
| ); | |
| } |
— deepseek-v4-pro via Qwen Code /review
- CRITICAL: reject non-OpenAI-compatible auth types (QWEN_OAUTH, etc.) in /model list to prevent 401 from sentinel token - CRITICAL: distinguish AbortError (timeout vs user cancel) in error messages - CRITICAL: show "no models found" for empty response instead of blank output - SUGGESTION: wrap response.json() in try/catch for non-JSON 200 responses - SUGGESTION: fix existing tests to use AuthType enum values instead of string casts - CI: add proper zh.js and zh-TW.js translations for new i18n keys Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The finally block already calls clearTimeout(timeoutId), making the call in the try block after fetch() redundant. Removing it simplifies the code without changing behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the follow-up review. All items from this round have been addressed:
Validation: `npx vitest run packages/cli/src/ui/commands/modelCommand.test.ts` — 34 passed. Pre-commit hooks (prettier + eslint) passed. CI was already green on the prior push (all 11 checks). |
| if ( | ||
| authType && | ||
| authType !== AuthType.USE_OPENAI && | ||
| authType !== AuthType.USE_ANTHROPIC |
There was a problem hiding this comment.
[Critical] USE_ANTHROPIC is allowed through the auth-type guard, but the Anthropic API uses x-api-key for authentication, not Authorization: Bearer. The /v1/models endpoint does not exist on the real Anthropic API either. Users with authType=anthropic will get a confusing 401/404 instead of the clear "not supported" message. The error message itself says "Only OpenAI-compatible endpoints are supported," contradicting the allowance.
| authType !== AuthType.USE_ANTHROPIC | |
| authType !== AuthType.USE_OPENAI |
— pai/glm-5.1 via Qwen Code /review
| }; | ||
| } catch (error) { | ||
| let errorMessage: string; | ||
| if (error instanceof DOMException && error.name === 'AbortError') { |
There was a problem hiding this comment.
[Critical] The DOMException + AbortError catch branch (lines 233-239) has zero test coverage. Timeout is the most likely real-world error for a 15s network call, and the isTimeout logic (!context.abortSignal?.aborted) is subtle — if wrong, users get misleading "Request cancelled" on timeout or vice versa. Please add at least two tests:
- Reject with
new DOMException('The operation was aborted', 'AbortError')+context.abortSignal.aborted === false→ assert content contains "timed out". - Same rejection +
context.abortSignal.aborted === true→ assert content contains "cancelled".
— pai/glm-5.1 via Qwen Code /review
| }; | ||
| } | ||
|
|
||
| const { baseUrl, apiKey, authType } = contentGeneratorConfig; |
There was a problem hiding this comment.
[Critical] fetchModels() lacks proxy support and customHeaders forwarding. The list action destructures only { baseUrl, apiKey, authType }, discarding proxy and customHeaders. Users behind a corporate proxy (who set QWEN_PROXY or modelProviders.*.proxy) will get unexplained connection failures. Users who rely on customHeaders for API gateway auth will have requests rejected. The rest of the codebase uses getOrCreateSharedDispatcher() and buildRuntimeFetchOptions() for proxy-aware fetch — this code path should follow the same pattern.
— pai/glm-5.1 via Qwen Code /review
| return { | ||
| type: 'message', | ||
| messageType: 'info', | ||
| content: t('No models found from the configured endpoint.'), |
There was a problem hiding this comment.
[Suggestion] Missing i18n key: t('No models found from the configured endpoint.') is called here but the key is not defined in any locale file (en.js, zh.js, zh-TW.js). While t() falls back to the key string itself, zh/zh-TW users will see English instead of translated text, and i18n audit tools will flag this.
— pai/glm-5.1 via Qwen Code /review
| abortSignal?: AbortSignal, | ||
| ): Promise<string[]> { | ||
| // Normalize baseUrl to avoid double slash (e.g., "https://api.openai.com/v1/") | ||
| const normalizedUrl = baseUrl.replace(/\/+$/, ''); |
There was a problem hiding this comment.
[Suggestion] baseUrl may already contain /models, causing a double /models/models path. The normalization only strips trailing slashes. If a user configures baseUrl: "https://api.example.com/v1/models", the resulting URL becomes https://api.example.com/v1/models/models.
| const normalizedUrl = baseUrl.replace(/\/+$/, ''); | |
| const normalizedUrl = baseUrl.replace(//+$/, '').replace(//models$/, ''); |
— pai/glm-5.1 via Qwen Code /review
| const { baseUrl, apiKey, authType } = contentGeneratorConfig; | ||
|
|
||
| if ( | ||
| authType && |
There was a problem hiding this comment.
[Suggestion] When authType is undefined, the guard if (authType && ...) silently passes through, allowing an unauthenticated fetch to proceed. The main action method has an explicit !authType early return, but the list subcommand lacks this. Consider inverting the guard to an explicit allowlist:
| authType && | |
| if (!authType || authType !== AuthType.USE_OPENAI) { |
— pai/glm-5.1 via Qwen Code /review
- Remove USE_ANTHROPIC from authType allowlist (Anthropic uses x-api-key, not Bearer, and has no /v1/models endpoint) - Guard against undefined authType in list subcommand - Add proxy + customHeaders support to fetchModels() via getOrCreateSharedDispatcher() — mirrors provider pattern - Fix baseUrl double /models path when baseUrl already ends with /models - Add AbortError test coverage: timeout vs user cancel paths - Add missing i18n key "No models found from the configured endpoint." to all 8 locale files (en/zh/zh-TW + ca/de/fr/ja/pt/ru) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| : defaultHeaders; | ||
|
|
||
| if (apiKey) { | ||
| headers['Authorization'] = `Bearer ${apiKey}`; |
There was a problem hiding this comment.
[Critical] Authorization header case-sensitivity conflict. headers is a plain JS Record<string, string> object (case-sensitive). If customHeaders contains a lowercase authorization, the spread produces an authorization key, then headers['Authorization'] = ... adds a second, distinct key. Both keys are passed to fetch() as independent Authorization headers, causing unpredictable authentication failures.
| headers['Authorization'] = `Bearer ${apiKey}`; | |
| // Remove any authorization header from customHeaders (any casing) before setting apiKey | |
| const authKey = Object.keys(headers).find( | |
| k => k.toLowerCase() === 'authorization' | |
| ); | |
| if (authKey) delete headers[authKey]; | |
| if (apiKey) { | |
| headers['Authorization'] = `Bearer ${apiKey}`; | |
| } |
— deepseek-v4-pro via Qwen Code /review
| : timeoutController.signal; | ||
|
|
||
| // Build proxy-aware fetch options (mirrors provider pattern) | ||
| const runtimeOptions = getOrCreateSharedDispatcher(proxy); |
There was a problem hiding this comment.
[Critical] Missing test coverage: proxy / dispatcher path untested. getOrCreateSharedDispatcher(proxy) is never called with a non-undefined proxy argument in tests, and fetchModels tests never mock this function to verify the dispatcher is passed to fetch. Users who rely on proxy configuration have no regression protection.
Suggested additional tests:
- In
fetchModelstests, mockgetOrCreateSharedDispatcher, pass aproxyargument, and assert thedispatcherappears in the fetch options - In
listsubcommand integration tests, setproxyand verify it is forwarded tofetchModels
— deepseek-v4-pro via Qwen Code /review
| Accept: 'application/json', | ||
| }; | ||
| const headers = customHeaders | ||
| ? { ...defaultHeaders, ...customHeaders } |
There was a problem hiding this comment.
[Critical] Missing test coverage: customHeaders merging untested. The { ...defaultHeaders, ...customHeaders } spread is never verified in tests. fetchModels tests never pass a customHeaders argument, and list subcommand tests never set customHeaders in the mock config.
Suggested additional tests:
- Call
fetchModelswithcustomHeaders: { 'X-Custom': 'value' }and assert the fetch spy receives headers containing bothAcceptandX-Custom - In
listsubcommand integration tests, setcustomHeadersand verify it is forwarded tofetchModels
— deepseek-v4-pro via Qwen Code /review
| ): Promise<string[]> { | ||
| // Normalize baseUrl: strip trailing slashes and trailing /models to avoid | ||
| // double path (e.g., "https://api.example.com/v1/models" → "/models/models") | ||
| const normalizedUrl = baseUrl.replace(/\/+$/, '').replace(/\/models$/, ''); |
There was a problem hiding this comment.
[Suggestion] The /models suffix regex is case-sensitive. .replace(/\/models$/, '') only matches lowercase /models, missing /Models or /MODELS. If a user configures an enterprise endpoint with mixed-case paths (e.g., https://internal.example.com/v1/Models), the URL becomes malformed as .../Models/models.
Also suggest adding a test case where baseUrl ends with /models.
| const normalizedUrl = baseUrl.replace(/\/+$/, '').replace(/\/models$/, ''); | |
| const normalizedUrl = baseUrl.replace(/\/+$/, '').replace(/\/models$/i, ''); |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| export async function fetchModels( | ||
| baseUrl: string, | ||
| apiKey?: string, |
There was a problem hiding this comment.
[Suggestion] FETCH_TIMEOUT_MS is hardcoded to 15 seconds, ignoring the user-configured ContentGeneratorConfig.timeout. When a user configures a longer timeout for a high-latency endpoint, regular API calls respect that setting, but /model list will still time out at 15 seconds — inconsistent behavior.
Consider passing ContentGeneratorConfig.timeout into fetchModels as the timeout value, falling back to 15 seconds only when not configured.
— deepseek-v4-pro via Qwen Code /review
- Case-insensitive /models suffix stripping in baseUrl normalization - Deduplicate authorization header (case-insensitive) before setting Bearer token to prevent duplicate headers when customHeaders has lowercase key - Use ContentGeneratorConfig.timeout with 15s fallback instead of hardcoded - Add test: customHeaders merge into fetch headers - Add test: proxy/dispatcher passed through to fetch - Add test: baseUrl /Models case-insensitive normalization - Add test: lowercase authorization in customHeaders gets replaced Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All five items from this review were already addressed in the round 4 commit (
All 40 tests in |
wenshao
left a comment
There was a problem hiding this comment.
Additional suggestions (test coverage gaps):
- Internal timeout mechanism untested — No test uses
vi.useFakeTimers()to trigger the actualsetTimeout→AbortController.abort()path infetchModels. The timeout path is only exercised indirectly via mocked fetch rejections at the list subcommand level. clearTimeoutin finally never verified — No test assertsclearTimeout(timeoutId)is called. A timer leak could go undetected.- Custom
timeoutparameter never tested — All 20+fetchModels()calls in tests omit the 6thtimeoutparameter. proxy/customHeaders/timeoutpassthrough not integration-tested — The list subcommand's "success" test uses minimal config without these fields; the wiring from config → fetchModels is not verified at the integration level.
— deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| // Set up timeout and optional user abort signal | ||
| const fetchTimeout = timeout ?? DEFAULT_FETCH_TIMEOUT_MS; |
There was a problem hiding this comment.
[Critical] timeout: 0 causes instant timeout. const fetchTimeout = timeout ?? DEFAULT_FETCH_TIMEOUT_MS uses nullish coalescing, which passes 0 through as a valid value. setTimeout(callback, 0) fires on the next event loop tick, aborting the request immediately. Users who set timeout: 0 in their config (a common convention for "no timeout") will get a misleading "Request timed out" error.
| const fetchTimeout = timeout ?? DEFAULT_FETCH_TIMEOUT_MS; | |
| const fetchTimeout = (timeout && timeout > 0) ? timeout : DEFAULT_FETCH_TIMEOUT_MS; |
— deepseek-v4-pro via Qwen Code /review
| // Deduplicate any existing authorization header (case-insensitive) before | ||
| // setting the Bearer token to avoid sending two Authorization headers when | ||
| // customHeaders contains a lowercase 'authorization' key. | ||
| const existingAuthKey = Object.keys(headers).find( |
There was a problem hiding this comment.
[Critical] Authorization header deduplication runs unconditionally, silently dropping customHeaders auth when apiKey is absent. The existingAuthKey find-and-delete logic (lines 291-295) runs before the if (apiKey) guard (line 297). Users who configure auth purely via customHeaders.authorization without setting apiKey lose their auth header with no replacement, causing unauthenticated requests.
| const existingAuthKey = Object.keys(headers).find( | |
| if (apiKey) { | |
| const existingAuthKey = Object.keys(headers).find( | |
| (k) => k.toLowerCase() === 'authorization', | |
| ); | |
| if (existingAuthKey) { | |
| delete headers[existingAuthKey]; | |
| } | |
| headers['Authorization'] = `Bearer ${apiKey}`; | |
| } |
— deepseek-v4-pro via Qwen Code /review
| const { baseUrl, apiKey, authType, proxy, customHeaders, timeout } = | ||
| contentGeneratorConfig; | ||
|
|
||
| if (!authType || authType !== AuthType.USE_OPENAI) { |
There was a problem hiding this comment.
[Suggestion] The authType guard rejects all non-USE_OPENAI types, blocking OpenAI-compatible proxies. Users who route through litellm or similar proxies with authType: 'gemini' or 'anthropic' cannot use /model list, even though their endpoint fully supports GET /models. Consider only rejecting auth types known to lack a /models endpoint (e.g., QWEN_OAUTH), or drop the guard entirely and let the HTTP response determine support.
— deepseek-v4-pro via Qwen Code /review
- Test internal timeout mechanism: vi.useFakeTimers() triggers the setTimeout → AbortController.abort() path and verifies the fetch promise rejects with AbortError - Test clearTimeout is called on successful fetch completion - Test custom timeout parameter (5s) overrides the default 15s - Test proxy/customHeaders/timeout passthrough from config → fetchModels at the list subcommand integration level Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All 4 items from this review have been addressed in the latest push (
All 44 tests in |
Source fixes: - Fix timeout: 0 causing instant abort: use (timeout && timeout > 0) ? timeout : DEFAULT instead of nullish coalescing, so 0 falls back to the 15s default - Fix auth header dedup dropping custom auth when apiKey is absent: move the existing-auth-key deletion inside the if (apiKey) guard so customHeaders authorization is preserved when no apiKey is configured Test additions (46 total): - Verify timeout: 0 falls back to default 15s (not instant abort) - Verify custom Authorization header in customHeaders is preserved when apiKey is not set Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Additional fixes from deeper review of inline comments: Source fixes:
2 new tests (46 total):
All 46 tests pass. Pre-commit hooks (prettier + eslint) pass. |
wenshao
left a comment
There was a problem hiding this comment.
Review scope: Incremental diff from round 5 (8f7bef2..1d99873). Two targeted bug fixes — auth dedup scoping (fixes custom header preservation when apiKey unset) and timeout guard (fixes timeout:0 instant abort). Both correct and well-tested.
Suggestion: The new timeout guard changes behavior for negative timeout values — not explicitly tested. Consider adding a test case.
| method: 'GET', | ||
| headers, | ||
| signal, | ||
| ...(runtimeOptions ? { dispatcher: runtimeOptions } : {}), |
There was a problem hiding this comment.
[Suggestion] getOrCreateSharedDispatcher(proxy) always returns a Dispatcher instance, never undefined/null. The conditional spread ...(runtimeOptions ? { dispatcher: runtimeOptions } : {}) has a dead else branch that misleads readers into thinking the dispatcher might be absent.
| ...(runtimeOptions ? { dispatcher: runtimeOptions } : {}), | |
| dispatcher: runtimeOptions, |
— deepseek-v4-pro via Qwen Code /review
|
Commit: 941a5d4 @wenshao, thank you for your time, patience, and energy in guiding this PR to a production-grade state. I have addressed the final round of feedback:
All functional, security, and architectural requirements are now covered by 40+ tests. I believe this addresses all remaining blockers. Please let me know if there's anything else needed! |
wenshao
left a comment
There was a problem hiding this comment.
This is an incremental review — the PR already has extensive feedback from previous rounds. The findings below are net-new gaps not covered by existing inline comments.
[Suggestion] fetchModels has no debug logging (lines 268-380)
The codebase convention is to use createDebugLogger('MODULE_NAME') (see setupGithubCommand.ts, extensionsCommand.ts, languageCommand.ts). fetchModels performs a network request but records nothing — no URL, proxy status, response code, or duration. A production /model list failure leaves the oncall engineer with zero observability.
Suggested fix: Add createDebugLogger('MODEL_LIST') and log at minimum: constructed URL, whether proxy was used, response status, and elapsed time.
[Suggestion] Missing test: data field present but not an array (line ~344)
The code guards against { data: "not-an-array" } but no test covers this branch. If an endpoint returns a malformed response with data as a non-array, this path is unverified.
[Suggestion] Missing test: subcommand metadata (lines 155-160)
No test asserts list subcommand's name, kind, or supportedModes. The parent modelCommand has these checks; the subcommand should too.
— deepseek-v4-pro via Qwen Code /review
| let errorText: string; | ||
| try { | ||
| errorText = (await response.text()).slice(0, 500); | ||
| } catch { |
There was a problem hiding this comment.
[Suggestion] fetchModels error messages don't include the endpoint URL.
throw new Error(`Request failed (${response.status}): ${errorText}`);When the request returns 401/500, the user sees "Failed to fetch models: Request failed (401): Unauthorized" — with no indication of which URL was hit. In a multi-provider setup, this is a black box.
| } catch { | |
| throw new Error(`Request to ${url} failed (${response.status}): ${errorText}`); |
— deepseek-v4-pro via Qwen Code /review
| // Build headers with customHeaders support (mirrors provider pattern) | ||
| const defaultHeaders: Record<string, string> = { | ||
| Accept: 'application/json', | ||
| }; |
There was a problem hiding this comment.
[Suggestion] baseUrl normalization doesn't handle query parameters.
const normalizedUrl = baseUrl.replace(/\/+$/, '').replace(/\/models$/i, '');
const url = `${normalizedUrl}/models`;If baseUrl is https://api.example.com/v1?version=2, the resulting URL becomes https://api.example.com/v1?version=2/models — appending /models after the query string. Most HTTP servers reject ?query before the path segment.
| }; | |
| const u = new URL(baseUrl); | |
| u.pathname = u.pathname.replace(/\/models$/i, '') + '/models'; | |
| const url = u.toString(); |
— deepseek-v4-pro via Qwen Code /review
| errorMessage = isTimeout | ||
| ? t('Request timed out. The endpoint may be slow or unreachable.') | ||
| : t('Request cancelled.'); | ||
| } else { |
There was a problem hiding this comment.
[Suggestion] Empty error.message produces a trailing-space message with no useful content.
errorMessage = error instanceof Error ? error.message : String(error);If fetchModels throws new Error(''), the user sees "Failed to fetch models: " with nothing after the colon.
| } else { | |
| errorMessage = error instanceof Error && error.message | |
| ? error.message | |
| : String(error); |
— deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Review — PR #3799
This PR is stacked on the unmerged PR #3797. The review below distinguishes the incremental changes from #3797 → #3799 from the final state landing on main if both are merged together.
Critical — security regressions vs the predecessor PR #3797
The PR rewrites fetchModels() end-to-end and drops three security controls that PR #3797 already implemented. If #3799 lands as the final state, the fetchModels on main will be strictly weaker than what #3797 builds:
1. HTTPS enforcement removed
PR #3797 (commit 99cb796, packages/cli/src/ui/commands/modelCommand.ts):
if (parsed.protocol !== 'https:') {
throw new Error('baseUrl must use HTTPS');
}PR #3799 deletes this. http://attacker.com now goes through with the user's Authorization: Bearer <apiKey> over plaintext — direct API token exposure on a user-facing input field.
2. SSRF protection removed
PR #3797's localhost / 127.0.0.1 / ::1 + isPrivateIp(baseUrl) block is gone. Misconfiguration or attacker-controlled config can target internal services (AWS IMDS at 169.254.169.254, internal admin endpoints, etc.).
3. apiKey redaction in error responses removed
PR #3797:
const sanitized = apiKey ? truncated.replaceAll(apiKey, '[REDACTED]') : truncated;
throw new Error(`Request failed (${response.status}): ${sanitized}`);PR #3799:
errorText = (await response.text()).slice(0, 500);
throw new Error(`Request failed (${response.status}): ${errorText}`);If an upstream 401/403 echoes the Authorization header in its error body (some misbehaving gateways do), the entire apiKey leaks to terminal output, logs, and potentially copy-pasted issue reports.
4. Default timeout reduced 30s → 15s without justification
PR #3797 uses FETCH_TIMEOUT_MS = 30_000; PR #3799 introduces DEFAULT_FETCH_TIMEOUT_MS = 15_000. The PR description doesn't explain this change. Slow-proxy / high-latency users will regress. Either keep 30s or document the rationale.
Suggested fix: rebase #3799 onto #3797 as a true incremental change, OR re-add the four controls (new URL(baseUrl) validation + HTTPS enforcement + isPrivateIp check + replaceAll(apiKey, '[REDACTED]') + 30s default) inside the rewritten function.
High
5. Conditional dispatcher spread is dead code in production
const runtimeOptions = getOrCreateSharedDispatcher(proxy);
// ...
...(runtimeOptions ? { dispatcher: runtimeOptions } : {}),getOrCreateSharedDispatcher (packages/core/src/utils/runtimeFetchOptions.ts:148) signature is (proxyUrl?: string): Dispatcher — it always returns a Dispatcher (default Agent when no proxy), never falsy. The ternary's else branch is only reachable through the test mock that returns undefined as never. Use dispatcher: runtimeOptions directly. (Raised in inline comment 3180218442, unaddressed.)
Medium
6. URL normalization breaks on query strings
const normalizedUrl = baseUrl.replace(/\/+$/, '').replace(/\/models$/i, '');
const url = `${normalizedUrl}/models`;If baseUrl = "https://api.example.com/v1?version=2", the result is https://api.example.com/v1?version=2/models — most servers reject this. Use a URL object:
const u = new URL(baseUrl);
u.pathname = u.pathname.replace(/\/+$/, '').replace(/\/models$/i, '') + '/models';
const url = u.toString();(Raised in inline 3182943269, unaddressed.)
7. Error messages don't include the endpoint URL
In multi-provider setups, Request failed (401): Unauthorized doesn't tell the user which baseUrl was hit. Suggest Request to ${url} failed (...). (Inline 3182943251.)
Low / Suggestion
8. Empty error.message produces trailing-space output
error instanceof Error ? error.message : String(error) — when error.message === '', the user sees Failed to fetch models: with nothing after. Use error.message || String(error). (Inline 3182943274.)
9. authType allowlist is overly narrow
Strict USE_OPENAI rejects litellm / self-hosted gateways using gemini / anthropic authType to route to OpenAI-compatible backends — these endpoints often do support /models. Either relax to "non-USE_OPENAI also allowed, let the response decide" or document the restriction explicitly. (Inline 3179274845.)
10. Test mockReturnValue(undefined as never) masks the dead code in #5
After fixing #5, these mocks need to drop the undefined return.
Strengths to keep
- Three response-shape parsing logic is clean; malicious/malformed entries are silently skipped, not thrown.
- AbortError disambiguation (
!context.abortSignal?.aborted) for timeout vs user cancel is correct and explicitly tested. finally { clearTimeout }plus a spy test prevents timer leaks.- Case-insensitive Authorization dedup +
if (apiKey)gating correctly preservescustomHeaders.authorizationwhen apiKey is unset. timeout: 0falls back to default — avoidssetTimeout(cb, 0)instant-abort.- 9-locale i18n keys are consistently added.
- 24 → 46 tests covering fake-timer paths, proxy passthrough, negative-timeout fallback.
Summary
Blockers: 4 items (HTTPS, SSRF, apiKey redaction, default timeout reduction). These are not new vulnerabilities — they're regressions against PR #3797's existing security controls. They become real once both PRs land.
Suggested merge path:
- Re-add the four security controls inside #3799's rewritten
fetchModels, OR - After #3797 lands on main, re-author #3799 as a true incremental PR that extends the existing fetchModels (response shapes, proxy, customHeaders, timeout) instead of rewriting it.
Items 5–9 are non-blocking and can be folded into the same revision or tracked separately.
Security hardening: - Restore default fetch timeout from 15s to 30s (matches QwenLM#3797) - Add redirect:'error' to fetchWithTimeout to prevent SSRF via 302 redirects - Refactor isPrivateIp() to accept hostname string instead of URL — fixes a bug where passing a bare hostname (e.g. "192.168.1.1") to the old URL-parsing version would silently return false, allowing private IP access. Call sites (modelCommand.ts, web-fetch.ts) now parse URL first. - Add IPv4-mapped IPv6 detection to isPrivateIp (::ffff:x.x.x.x form) - Change SSRF error message from "resolves to" to "points to" (isPrivateIp checks hostname string, doesn't do DNS resolution) Error message improvements: - Non-2xx error messages now include the endpoint URL - List subcommand error uses parameterized i18n template with URL and error - Eliminates trailing-space bug when error message is empty URL normalization: - Use new URL() object instead of string replace to correctly handle query strings (e.g. ?version=2 no longer breaks /models append) Debug logging: - Log proxy status alongside URL, response code, and duration at all three debug points in fetchModels (request start, failure, response) - Log request failures with proxy context Core fetchWithTimeout: - Non-positive timeout (<=0) throws FetchError immediately instead of leaking raw AbortError through AbortController - Added clearTimeout in finally block to prevent timer leaks - Accept optional { redirect } parameter i18n: - Update 3 keys per locale (9 files): Failed to fetch models, baseUrl points to, Request to {{url}} failed Tests (48 total, +8 new): - data field present but not an array - baseUrl with query string preserves params - URL in non-2xx error message - "points to" wording in SSRF error - Subcommand metadata (name, kind, supportedModes) - Zero/negative timeout throws FetchError (not raw AbortError) - SSRF tests assert fetch was never called (short-circuits before network) - URL in list subcommand error message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@wenshao — Thank you for your time, patience, and energy in reviewing this PR. We have addressed all items from the latest review round. All four critical security regressions vs #3797 have been resolved:
Additional fixes from your review:
8 new tests added (48 total), including SSRF tests that assert `fetch` is never called for private IPs. Regarding #3797: This PR now subsumes all of #3797's functionality (the /model list subcommand, SSRF hardening, etc.). We will be closing #3797 as this PR is the definitive version that should merge. Thank you for closing the loop on this. Commit: `5266e5e2a` |
|
@wenshao — Thank you for your time, patience, and energy in reviewing this PR. We have addressed all items from the latest review round. All four critical security regressions vs #3797 have been resolved:
Additional fixes from your review:
8 new tests added (48 total), including SSRF tests that assert `fetch` is never called for private IPs. Regarding #3797: This PR (#3799) now subsumes all of #3797's functionality (the /model list subcommand, SSRF hardening, etc.). We have closed #3797 as this PR is the definitive version that should merge. Thank you for closing the loop on this. Commit: `5266e5e2a` |
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
No new issues found. All 29 findings from 5 previous review rounds have been addressed in this commit. The code is well-structured with comprehensive error handling, thorough test coverage (24 tests), proper i18n support across 9 locales, and correct handling of edge cases (timeout:0, AbortError distinction, proxy/customHeaders forwarding, response shape normalization).
Deterministic Checks
- ESLint: ✅ 0 errors on changed files
- TypeScript: ✅ no new type errors
- Build: ✅ passes
- Tests: ✅ 24/24 pass in modelCommand.test.ts
— deepseek-v4-pro via Qwen Code /review
|
@wenshao — I believe all critical security feedback from your 2026-05-04 review have been addressed in commit
Plus: error messages now include the endpoint URL, URL normalization uses 48 tests pass. CI was green on the last push. Could you take another look when you have a moment? Thank you. |
Security hardening: - Restore default fetch timeout from 15s to 30s (matches QwenLM#3797) - Add redirect:'error' to fetchWithTimeout to prevent SSRF via 302 redirects - Refactor isPrivateIp() to accept hostname string instead of URL — fixes a bug where passing a bare hostname (e.g. "192.168.1.1") to the old URL-parsing version would silently return false, allowing private IP access. Call sites (modelCommand.ts, web-fetch.ts) now parse URL first. - Add IPv4-mapped IPv6 detection to isPrivateIp (::ffff:x.x.x.x form) - Change SSRF error message from "resolves to" to "points to" (isPrivateIp checks hostname string, doesn't do DNS resolution) Error message improvements: - Non-2xx error messages now include the endpoint URL - List subcommand error uses parameterized i18n template with URL and error - Eliminates trailing-space bug when error message is empty URL normalization: - Use new URL() object instead of string replace to correctly handle query strings (e.g. ?version=2 no longer breaks /models append) Debug logging: - Log proxy status alongside URL, response code, and duration at all three debug points in fetchModels (request start, failure, response) - Log request failures with proxy context Core fetchWithTimeout: - Non-positive timeout (<=0) throws FetchError immediately instead of leaking raw AbortError through AbortController - Added clearTimeout in finally block to prevent timer leaks - Accept optional { redirect } parameter i18n: - Update 3 keys per locale (9 files): Failed to fetch models, baseUrl points to, Request to {{url}} failed Tests (48 total, +8 new): - data field present but not an array - baseUrl with query string preserves params - URL in non-2xx error message - "points to" wording in SSRF error - Subcommand metadata (name, kind, supportedModes) - Zero/negative timeout throws FetchError (not raw AbortError) - SSRF tests assert fetch was never called (short-circuits before network) - URL in list subcommand error message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test "should use custom timeout instead of the default 15s" called
fetchModels with vi.useFakeTimers() but did not mock fetch(). On Node 24,
the real fetch() rejects the fake AbortSignal synchronously (Throws
"Expected signal to be an instance of AbortSignal"), causing the promise
to settle before the 5s timer fires. The test then fails at
expect(settled).toBe(false).
Fix: mock fetch() to return a pending promise that rejects via the
abort signal listener, matching the pattern used by the other timeout
test ("should abort via internal timeout").
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wenshao
left a comment
There was a problem hiding this comment.
Tested locally against real OpenAI-compatible endpoints (DashScope returned 239 models, DI-Proxy 15 models, both shape-2 with extra fields correctly ignored). All 46 unit tests pass after the latest fake-timer fix. Error paths (404, malformed JSON, missing data array) surface clean messages. Scope is well-contained — additive only, no changes to existing model-switching logic. LGTM.
Suggestion: relax the
|
| Provider | baseUrl | Authorization: Bearer |
x-api-key + anthropic-version |
|---|---|---|---|
| Z.AI GLM | open.bigmodel.cn/api/anthropic |
200 + {code:500,msg:"404 NOT_FOUND"} |
same |
| Moonshot Kimi | api.kimi.com/coding/ |
404 | 404 |
| MiniMax | api.minimax.io/anthropic |
404 | 404 |
| DeepSeek | api.deepseek.com/anthropic |
404 (empty) | 404 (empty) |
So none of these gateways implement /models on their /anthropic subpath today — the result is provider-agnostic. But: the user gets a more accurate message (404 / "missing data array") than the current generic "not supported".
Why header switching matters even if you only relax the gate
If /model list simply allowed USE_ANTHROPIC through with Authorization: Bearer $KEY and no anthropic-version, the official Anthropic endpoint would return the misleading 401 "x-api-key header is required" — making users think their key is wrong. So "relax the gate" without "fix the headers" is a worse experience than today's clean reject.
Suggested options (preference noted, both are fine)
Option A (preferred): support USE_ANTHROPIC by branching on header construction inside fetchModels():
if (authType === AuthType.USE_ANTHROPIC && apiKey) {
headers['x-api-key'] = apiKey;
headers['anthropic-version'] = '2023-06-01';
} else if (apiKey) {
// existing dedup + Bearer logic
}Plus passing authType into fetchModels(). Gateways without /models simply surface as Request failed (404) — strictly more informative than today's blanket rejection.
Option B (minimum): keep the auth check but rewrite the error to point at the cause (endpoint capability, not auth type) — e.g. "`/model list` does not yet support Anthropic-protocol endpoints. Track [issue link]." Avoids misleading users into thinking their endpoint is misconfigured.
USE_GEMINI / USE_VERTEX_AI should stay rejected — those use entirely different model-discovery APIs.
Happy to send a follow-up PR for Option A if you'd like. What's your preference?
|
Thanks for the detailed analysis and the thorough investigation, @wenshao! Your probing of the endpoint and the provider comparison table is really helpful. After reviewing the codebase, Option A is the way to go — it's clean and straightforward. The pattern already exists and the Anthropic endpoint returns the same shape. The only delta is header construction ( + ), which is a minimal change. We'll:
Appreciate your patience and energy on this one — the detailed examples and header-probing work saved a ton of investigation time. Starting implementation now. |
|
Thanks for the detailed analysis and the thorough investigation, @nicholasgasior! Your probing of the endpoint and the provider comparison table is really helpful. After reviewing the codebase, Option A is the way to go — it's clean and straightforward. The We'll:
Appreciate your patience and energy on this one — the detailed examples and header-probing work saved a ton of investigation time. Starting implementation now. |
|
Implementation complete and PR opened: #3863 Changes in
No more blanket rejection for |
Dismissing — end-to-end testing surfaced a UX gap (full unfiltered output for endpoints returning hundreds of models) that should land before merge. Details in the new review.
wenshao
left a comment
There was a problem hiding this comment.
Dismissed prior approval after running /model list end-to-end against real
OpenAI-compatible endpoints — the parser, proxy, timeout/abort, and error paths
are all solid, but the output handling makes the feature unwieldy for its
primary use case. Two changes needed before merge.
Why
Real endpoints return many models. Verified locally:
| Endpoint | Models returned |
|---|---|
| DashScope (Aliyun) | 239 |
| DI-Proxy | 15 |
| DeepSeek | 2 |
| OpenRouter (typical) | 400+ |
A 239-line unfiltered dump in one message is awkward in the transcript and
effectively unusable for catalog discovery. The current implementation drops
args (_args: string) and prints models.join('\n') with no filter, sort,
or truncation. This was already raised in the #3797 review and not addressed
here.
Required
1. Substring filter via args. /model list qwen3 filters case-insensitively
against model id. Empty arg keeps current full-list behavior. No-match returns
an info message (not an error).
2. Pagination / truncation. When the result count exceeds a sensible
threshold, truncate and append a hint such as
... and N more — narrow with "/model list <keyword>". Pick whatever
threshold reads well (50 is a reasonable starting point); the goal is to keep
the transcript usable.
Implementation pointers
packages/cli/src/ui/commands/modelCommand.ts:165— change_args: stringtoargs: stringpackages/cli/src/ui/commands/modelCommand.ts:229-233— apply filter (consider sorting alphabetically for stable output) and truncation beforemodels.join('\n')- Add tests for: substring match (case-insensitive), no-match (info-typed message), and >threshold truncation hint
- New i18n key for the truncation hint and the no-match message in all 9 locales
Smaller follow-ups (do these here or in a follow-up PR — your call)
- Inner
fetchModelsthrown strings (Unexpected response format: missing data array,Request failed (...)) bypasst()while the outerFailed to fetch models:is translated. Wrap them for consistency. - PR description still says "Pairs with
/model list(PR #3797)" — #3797 is closed. Suggest "Supersedes #3797" and bump the validation block to 46 tests.
Re-approving once filter + truncation are in.
|
Thanks for the contribution. Closing this as not planned. After looking at the broader direction, we've decided not to ship The companion PR #3863, which extends this to Anthropic, is being closed for the same reason. Appreciate the effort that went into this — the response-shape analysis and the test coverage were both thorough. If you'd like to contribute elsewhere in the codebase, we're glad to review focused PRs. |
|
@tanzhenxin Thanks for the detailed explanation — that makes sense. I agree that the current shape of the feature is fragile if One possible path that may avoid that issue would be to treat model listing as an optional provider-native capability rather than a command-level fetch helper. For example: interface ContentGenerator {
// existing methods...
listModels?(): Promise<AvailableModel[]>;
}Then each provider/generator would own its own implementation: OpenAI-compatible providers would call the already-configured OpenAI SDK client, rather than constructing /models manually. That would keep auth shape, base URL behavior, proxy behavior, custom headers, and provider quirks inside the provider abstraction where the rest of the request logic already lives. The CLI command would only render the result when the active provider explicitly supports the capability. So instead of: The shape would become: If unsupported, the UX could be explicit: Model listing is not supported by this provider or endpoint. I’m not asking to reopen this PR as-is. I agree the command-level implementation has the failure mode you called out. I’m mainly asking whether a narrower provider-first design proposal would be worth exploring, possibly as a small core-only PR or RFC before any CLI command is added. The goal would be to avoid normalizing a fragmented ecosystem globally, and instead expose model listing only where the configured provider can do it reliably through its own abstraction. |
Summary
Normalize
fetchModels()to handle multiple OpenAI-compatible response shapes when querying/modelsendpoints.Handles
{ data: [{ id: \"qwen-plus\" }] }{ object: \"list\", data: [{ id: \"deepseek-chat\", owned_by: \"...\" }] }[{ id: \"model\" }](some providers skip the wrapper)Extra fields (
owned_by,created,permission, etc.) are ignored — onlyidis extracted.Response Shapes
{ data: [{ id: \"...\" }] }{ object: \"list\", data: [{ id: \"...\", owned_by: \"...\" }] }[{ id: \"...\" }]idstrings indataarrayScope
Pairs with
/model list(PR #3797). Once users can list models, broken provider responses become obvious.Validation
npm run buildpasses (0 errors)npx vitest run src/ui/commands/modelCommand.test.ts— 24 tests pass (16 new for fetchModels)Risk
Low. Isolated to
fetchModels()inpackages/cli/src/ui/commands/modelCommand.ts.