Repository navigation
Conversation
c77991e to
28f5b17
Compare
|
Rebased onto current Conflict resolution kept
Checks on
|
|
Hi, a gentle ping on this one when you have time. It still merges cleanly into current I know ~1.2k lines is a lot to review in one go. If it helps, I can split it into three smaller PRs:
Happy to do either. Also, #421 touches the same connection-test path, so I'd suggest landing this one first. I'll rebase #421 on top of it. |
Declare modelDiscoveryMode on provider capabilities so listing, Settings pickers, imports, and diagnostics stop treating GET /models as universal. Fixes OpenCoworkAI#210
28f5b17 to
1e2f3c8
Compare
There was a problem hiding this comment.
Findings
- [Minor] Every Codex config import is blanket-stamped
infer-only, which drops model listing for OpenAI-compatible endpoints that do exposeGET /models—packages/shared/src/model-discovery.ts(discoveryModeForImport:if (source === 'codex') return 'infer-only'), applied atapps/desktop/src/main/imports/codex-config.ts:241. Before this PR these entries carried nocapabilities, soresolveProviderCapabilitiesderivedmodelsand the Settings picker fetched/models. Endpoints in the Codex import allowlist (DEEPSEEK_API_KEY,GROQ_API_KEY,MISTRAL_API_KEY,XAI_API_KEY,OPENAI_API_KEY, …) are standard OpenAI-compatible hosts where listing works, so this is a user-visible downgrade: the picker now forces manual ID entry and the connection test skips/modelseven though listing is available. The Claude Code path already handles this correctly by branching on host (isDefaultAnthropicApiHost).
Suggested fix / next action: narrow this the same way — forsource === 'codex', returnmodelswhenentry.baseUrlresolves to a known listing-capable host (e.g.api.openai.com,api.deepseek.com,api.groq.com,api.mistral.ai,api.x.ai), and only fall back toinfer-onlyfor unknown/proxy hosts. If the blanket choice is deliberate, please note the rationale (and the lost listing) in the PR body.
Questions
ConnectionDiagnosticPanelgained an optionalmodelDiscoveryModeprop (apps/desktop/src/renderer/src/components/ConnectionDiagnosticPanel.tsx), but I could not find a render site passing it. Because it is optional, TypeScript will not flag a missing pass-through, and if it is not wired the newdiagnose('404', …)branch (packages/shared/src/diagnostics.ts, themodel-discovery-degradedearly return) only fires in unit tests, not in the real UI. Can you confirm the caller passesmodelDiscoveryMode={row.modelDiscoveryMode}(or equivalent)?
Summary
Review mode: initial
The change direction is sound and additive: modelDiscoveryMode is an optional on-disk field, schemaVersion is unchanged, ProviderModelDiscoveryModeSchema only gains infer-only (old configs still parse), supportsModelsEndpoint is now derived in lockstep with the mode instead of drifting from it, and no new dependencies or direct provider-SDK imports are introduced. Test coverage is strong for the new paths (helpers, list-for-provider plans, connection-test modes, parsers, imports, picker state, diagnostics). The one substantive concern is the blanket Codex-import stamp above.
Residual observations (non-blocking):
discoveryModeForImporttreatsopencodeandgeminias alwaysmodelsregardless of base URL, whereasclaude-codebranches on host. If an OpenCode/Gemini entry can point at a local/custom proxy, it will keep probing/models; worth confirming the mappings are official-only.infer-onlyandmanualare near-duplicates for picker/connection-test behavior (settingsModelPickerKind,connectionTestProbesModelsEndpoint,resolveListForProviderPlan); the distinction is semantic only. Acceptable, but consider documenting it so future call sites do not assume behavioral differences.- I could not independently fetch the body/acceptance criteria of the linked issue #210 in this run; the diff is on-topic for “
/modelsis not universal,” but verify the specific acceptance criteria against the mode matrix before treatingFixes #210as satisfied.
Testing
Not run (automation). Coverage added: packages/shared/src/model-discovery.test.ts, packages/shared/src/config.test.ts, packages/shared/src/diagnostics.test.ts, apps/desktop/src/main/connection-ipc.test.ts, apps/desktop/src/main/onboarding/provider-parsers.test.ts, apps/desktop/src/main/provider-settings.test.ts, and import/Settings suites. The PR acknowledges no live Electron click-through for the Settings picker; an E2E walk of the manual-vs-select flows would close that gap but is not required to merge.
Open-CoDesign Bot
Codex imports no longer stamp every provider infer-only. Official hosts that expose GET /models keep listing; custom and proxy hosts stay infer-only, matching the Claude Code host split. Settings connection failures now render ConnectionDiagnosticPanel with the row's modelDiscoveryMode so a /models 404 follows the declared mode.
Summary
Provider model discovery still mostly assumed a listing endpoint, even though real providers split into GET
/models, a static catalog, manual IDs, inference-only gateways, and imports with a single known default. This adds a formalmodelDiscoveryModeand drives listing, Settings pickers, import defaults, connection tests, and diagnostics from it so “no/models” is no longer a uniform hard failure.Fixes #210
Mode enum:
models|static-hint|manual|infer-onlymodels:v1:list-for-providermodels/models/models; 404 + working inference staysdegradedstatic-hintmodelsHint+defaultModel/models(ChatGPT Codex OAuth still uses its existing token check)manual[defaultModel]locally/models, probe inference →compatibleinfer-only/models; missing listing is expected, not degradedBuilt-in / import defaults:
modelsstatic-hintapi.anthropic.com→models; custom/proxy base →infer-onlymodelsinfer-onlymodels; listing failed →infer-only; user chose manual →manualIndependent of sibling PRs #419 / #420 / #421; additive types only.
Type of change
Linked issue
Fixes #210
Checklist
pnpm lint && pnpm typecheck && pnpm testpasses locallypnpm changeset) if user-visiblePrinciples 5b
supportsModelsEndpoint/modelsHintstill work;modelDiscoveryModeis optional on disk and older configs derive a modeschemaVersionunchanged; Zod accepts the new optional capability field and locksteps it with listingTests
Cover mode helpers,
list-for-providerplans, infer-only/manual connection tests (skip/models, success iscompatible), Settings picker state, parser payloads, builtin/import stamps, and diagnostics that treat listing 404 as info when the mode does not expect/models.Screenshots / recordings (UI changes)
Settings picker and custom-provider test copy changed. This environment cannot drive the Electron Settings UI, so those flows were verified with unit tests rather than a live click-through.