fix(opencode-go): fetch models unconditionally — the /models endpoint is public#437
Conversation
… is public
The Opencode Go model fetch in `requestRouterModels` was gated behind
`if (opencodeGoApiKey)` on the assumption that its `/models` endpoint requires
auth. It does not: the endpoint returns the full model list (HTTP 200) with no
Authorization header. Gating meant `routerModels["opencode-go"]` stayed `{}`
whenever the key wasn't present in `apiConfiguration` at fetch time, so the
model picker showed an empty list, fell back to the hardcoded default
(`glm-5.1`), and offered no model to select.
Fetch Opencode Go unconditionally like the other public routers
(`openrouter`, `vercel-ai-gateway`), forwarding the API key when present and
still flushing the cache when a new key is supplied. Updates the affected
`requestRouterModels` tests and adds a regression test for the keyless path.
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
📝 WalkthroughWalkthroughOpencode Go model fetching is changed to always include ChangesOpencode Go public models endpoint with refresh UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/webview/webviewMessageHandler.ts`:
- Line 1021: The const declaration for opencodeGoApiKey inside the switch case
has function-level scoping risk; wrap that switch case's body in a block (add {
} around the case's statements) so opencodeGoApiKey is block-scoped and cannot
be accessed by other switch clauses—locate the switch in
webviewMessageHandler.ts where opencodeGoApiKey is assigned and enclose the
case's statements in a local block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15b8cd41-b80a-4fd5-9a1c-283fa3c6b2c7
📒 Files selected for processing (2)
src/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.ts
| // above. Gating it behind a key meant the picker stayed empty (and fell back to the default | ||
| // model) whenever the key wasn't yet in apiConfiguration at fetch time. The key is still | ||
| // forwarded when present. | ||
| const opencodeGoApiKey = message?.values?.opencodeGoApiKey ?? apiConfiguration.opencodeGoApiKey |
There was a problem hiding this comment.
Wrap the variable declaration in a block to fix scoping issue.
The const opencodeGoApiKey declaration inside the switch case should be wrapped in curly braces to prevent potential scoping conflicts with other switch clauses.
🔧 Proposed fix
- const opencodeGoApiKey = message?.values?.opencodeGoApiKey ?? apiConfiguration.opencodeGoApiKey
-
- // Refresh the cache when a new key is explicitly provided (e.g. the Refresh Models button).
- if (message?.values?.opencodeGoApiKey) {
- await flushModels({ provider: "opencode-go", apiKey: opencodeGoApiKey }, true)
- }
-
- candidates.push({
- key: "opencode-go",
- options: { provider: "opencode-go", apiKey: opencodeGoApiKey },
- })
+ {
+ const opencodeGoApiKey = message?.values?.opencodeGoApiKey ?? apiConfiguration.opencodeGoApiKey
+
+ // Refresh the cache when a new key is explicitly provided (e.g. the Refresh Models button).
+ if (message?.values?.opencodeGoApiKey) {
+ await flushModels({ provider: "opencode-go", apiKey: opencodeGoApiKey }, true)
+ }
+
+ candidates.push({
+ key: "opencode-go",
+ options: { provider: "opencode-go", apiKey: opencodeGoApiKey },
+ })
+ }As per coding guidelines, Biome flagged: "Other switch clauses can erroneously access this declaration. Wrap the declaration in a block to restrict its access to the switch clause."
🧰 Tools
🪛 Biome (2.4.16)
[error] 1021-1021: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/webviewMessageHandler.ts` at line 1021, The const
declaration for opencodeGoApiKey inside the switch case has function-level
scoping risk; wrap that switch case's body in a block (add { } around the case's
statements) so opencodeGoApiKey is block-scoped and cannot be accessed by other
switch clauses—locate the switch in webviewMessageHandler.ts where
opencodeGoApiKey is assigned and enclose the case's statements in a local block.
…ode-go fetch Mirror changes already made in webviewMessageHandler.spec.ts: - successful responses: expect opencode-go call, assert mockModels - individual provider failures: add mockResolvedValueOnce for opencode-go - skips LiteLLM: assert mockModels instead of empty object
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/webview/__tests__/ClineProvider.spec.ts (1)
2469-2469: ⚡ Quick winUse exact match assertion for consistency.
Line 2469 uses
expect.objectContaining({ provider: "opencode-go" })while other provider assertions in the same test use exact object matches (lines 2459-2467). Since the implementation always passesapiKeyeven when undefined, and this test has noopencodeGoApiKeyconfigured, the assertion should be exact:- // Opencode Go's /models endpoint is public, so it is fetched like the other no-auth routers. - expect(getModels).toHaveBeenCalledWith(expect.objectContaining({ provider: "opencode-go" })) + expect(getModels).toHaveBeenCalledWith({ provider: "opencode-go", apiKey: undefined })This matches the pattern used for other public providers like
openrouter(line 2459) and aligns with the regression test assertion in webviewMessageHandler.spec.ts (context snippet 3).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/__tests__/ClineProvider.spec.ts` at line 2469, Update the assertion for getModels in ClineProvider.spec.ts to use an exact object match instead of expect.objectContaining so it mirrors other provider assertions; specifically change the call to expect(getModels).toHaveBeenCalledWith(...) to assert the full object including provider: "opencode-go" and apiKey: undefined (since the implementation always passes apiKey even when undefined).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.spec.ts`:
- Line 2469: Update the assertion for getModels in ClineProvider.spec.ts to use
an exact object match instead of expect.objectContaining so it mirrors other
provider assertions; specifically change the call to
expect(getModels).toHaveBeenCalledWith(...) to assert the full object including
provider: "opencode-go" and apiKey: undefined (since the implementation always
passes apiKey even when undefined).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d94be830-5074-4c9c-9c97-d0e81b52692f
📒 Files selected for processing (1)
src/core/webview/__tests__/ClineProvider.spec.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
proyectoauraorg
left a comment
proyectoauraorg
left a comment
There was a problem hiding this comment.
Code Review: PR #437 — fix(opencode-go): fetch models unconditionally
Summary
This PR fixes a real and reproducible bug where the Opencode Go model picker stayed empty because the /models fetch was gated behind an API key that wasn't always present in apiConfiguration at fetch time. The fix is well-scoped, the root cause analysis in the PR description is thorough, and the approach of aligning Opencode Go with the other public routers (openrouter, vercel-ai-gateway) is the correct one.
What works well
- Correct root cause identification. The PR description clearly demonstrates with
curland test output that the/modelsendpoint is public, making the gate unnecessary. - Minimal, focused change. Only 3 files modified, +54/-16 lines. The behavioral change is a single logical operation (removing the
if (opencodeGoApiKey)gate), which reduces blast radius. - Good comment update. The misleading comment ("its /models endpoint requires auth") is replaced with a detailed explanation of why the unconditional fetch is correct and what the previous behavior caused.
- Cache flush logic preserved. The
flushModelscall is correctly kept conditional onmessage?.values?.opencodeGoApiKey, so the cache is only invalidated when the user explicitly provides a new key (Refresh Models button). This avoids unnecessary re-fetches. - Test coverage is solid. The regression test in
webviewMessageHandler.spec.tsexplicitly verifies the keyless path (apiKey: undefined), and all three existing test suites are updated consistently.
Issues found
Minor — Block-scoping for const opencodeGoApiKey in switch case
File: src/core/webview/webviewMessageHandler.ts line 1021
CodeRabbit already flagged this in review #4404026771 but it has not been addressed in the second commit. The const opencodeGoApiKey declaration inside the switch case is function-scoped per JavaScript semantics. While it won't cause a runtime error (no other case declares a variable with the same name), it violates the noSwitchDeclarations lint rule and is inconsistent with the defensive pattern the other providers use.
The existing providers (litellm, poe, deepseek) avoid this by wrapping their logic in if blocks, which implicitly create block scope. Since Opencode Go's unconditional fetch removes the if gate, the declaration now sits directly in the switch case body.
Suggested fix: Wrap the Opencode Go block in curly braces to create explicit block scope:
{
const opencodeGoApiKey = message?.values?.opencodeGoApiKey ?? apiConfiguration.opencodeGoApiKey
// Refresh the cache when a new key is explicitly provided (e.g. the Refresh Models button).
if (message?.values?.opencodeGoApiKey) {
await flushModels({ provider: "opencode-go", apiKey: opencodeGoApiKey }, true)
}
candidates.push({
key: "opencode-go",
options: { provider: "opencode-go", apiKey: opencodeGoApiKey },
})
}Suggestion — Use exact assertion in ClineProvider.spec.ts line 2469
File: src/core/webview/__tests__/ClineProvider.spec.ts line 2469
The assertion uses expect.objectContaining({ provider: "opencode-go" }) while the equivalent assertions for other providers in the same test block use exact object matches. Since the implementation always passes apiKey (even when undefined), the assertion should be exact for consistency and to catch potential regressions where extra unexpected properties are introduced:
// Instead of:
expect(getModels).toHaveBeenCalledWith(expect.objectContaining({ provider: "opencode-go" }))
// Use:
expect(getModels).toHaveBeenCalledWith({ provider: "opencode-go", apiKey: undefined })This also matches the pattern used in the regression test at webviewMessageHandler.spec.ts.
CI Status
All checks pass except codecov/patch, which reports a coverage delta. This is expected for a behavioral change that shifts code from "not reached" to "reached" — the new regression test should cover the keyless path. Worth verifying the uncovered lines are the flushModels conditional (which is covered by the existing "Refresh Models" test path).
Edge cases verified
- No key configured: Fetch proceeds with
apiKey: undefined— correct for a public endpoint. - Empty string key: The
??operator only catchesnull/undefined, so an empty string would be passed as-is. This is harmless since the endpoint ignores the header. - Network failure: The
safeGetModelswrapper catches errors and returns{}, so the UI gracefully falls back to the default model. Documented in the PR description as a separate follow-up concern. - Performance: The additional fetch runs in parallel via
Promise.allSettledwith all other providers — no sequential bottleneck introduced.
Verdict
Approve with minor comments. The core fix is correct, well-tested, and low-risk. The two issues above are non-blocking but should be addressed before merge for lint compliance and test consistency.
Reviewed by: DevOps Orquestador Maestro — zSys
|
Need to bump up coverage. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/components/settings/providers/__tests__/OpenCodeGo.spec.tsx (1)
147-154: ⚡ Quick winAdd a loading-state regression for non-Opencode-Go
routerModels.Please add a test where refresh is loading, then dispatch
routerModelswithvalues.providerset to another provider (for example"openrouter"), and assert success is not shown. This closes the cross-provider message-bus gap.As per coding guidelines, “Use
webview-uitests for React rendering, hooks, component state, forms, validation, and webview UI wiring.”Also applies to: 223-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/providers/__tests__/OpenCodeGo.spec.tsx` around lines 147 - 154, Add a new regression test after the existing "shows success state when routerModels arrives while loading" test that verifies success is not shown when routerModels is dispatched with a different provider. Create a test that calls renderComponent with opencodeGoApiKey set, clicks the refresh-button to start loading, then dispatches a routerModels message with values.provider set to a different provider like "openrouter" (instead of the current provider), and assert that the success message does not appear in the document. This ensures that routerModels messages from other providers don't incorrectly trigger success state for the OpenCodeGo provider.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/providers/OpenCodeGo.tsx`:
- Around line 54-59: The code sets refreshStatus to "success" whenever a
"routerModels" message is received while loading, but since this listener is on
a shared window bus, other providers' messages can incorrectly trigger success
for Opencode-Go. Add an additional condition to check that
message.values?.provider === "opencode-go" in the else if block where
message.type === "routerModels" before calling setRefreshStatus("success") to
ensure only actual Opencode-Go responses mark the refresh as successful.
---
Nitpick comments:
In `@webview-ui/src/components/settings/providers/__tests__/OpenCodeGo.spec.tsx`:
- Around line 147-154: Add a new regression test after the existing "shows
success state when routerModels arrives while loading" test that verifies
success is not shown when routerModels is dispatched with a different provider.
Create a test that calls renderComponent with opencodeGoApiKey set, clicks the
refresh-button to start loading, then dispatches a routerModels message with
values.provider set to a different provider like "openrouter" (instead of the
current provider), and assert that the success message does not appear in the
document. This ensures that routerModels messages from other providers don't
incorrectly trigger success state for the OpenCodeGo provider.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd58a22e-2cd6-4324-be0c-664df55acf7f
📒 Files selected for processing (2)
webview-ui/src/components/settings/providers/OpenCodeGo.tsxwebview-ui/src/components/settings/providers/__tests__/OpenCodeGo.spec.tsx
| } else if (message.type === "routerModels") { | ||
| if (refreshStatus === "loading") { | ||
| if (!errorJustReceived.current) { | ||
| setRefreshStatus("success") | ||
| } | ||
| } |
There was a problem hiding this comment.
Gate success state to Opencode-Go responses only.
While loading, any routerModels message sets success. Since this listener is on the shared window bus, unrelated provider responses can incorrectly mark the Opencode-Go refresh as successful. Check message.values?.provider === "opencode-go" before setting "success".
Suggested fix
- } else if (message.type === "routerModels") {
+ } else if (message.type === "routerModels") {
+ const providerName = message.values?.provider
if (refreshStatus === "loading") {
- if (!errorJustReceived.current) {
+ if (providerName === "opencode-go" && !errorJustReceived.current) {
setRefreshStatus("success")
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (message.type === "routerModels") { | |
| if (refreshStatus === "loading") { | |
| if (!errorJustReceived.current) { | |
| setRefreshStatus("success") | |
| } | |
| } | |
| } else if (message.type === "routerModels") { | |
| const providerName = message.values?.provider | |
| if (refreshStatus === "loading") { | |
| if (providerName === "opencode-go" && !errorJustReceived.current) { | |
| setRefreshStatus("success") | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/components/settings/providers/OpenCodeGo.tsx` around lines 54
- 59, The code sets refreshStatus to "success" whenever a "routerModels" message
is received while loading, but since this listener is on a shared window bus,
other providers' messages can incorrectly trigger success for Opencode-Go. Add
an additional condition to check that message.values?.provider === "opencode-go"
in the else if block where message.type === "routerModels" before calling
setRefreshStatus("success") to ensure only actual Opencode-Go responses mark the
refresh as successful.
|
Skipping minor coderabbit comments and approving |
3 participants
Bug
Reported for the Opencode Go provider (added in #172): the model list never loads — the picker is empty, a custom model name can't be entered, and the provider falls back to the hardcoded default
glm-5.1.Root cause
In
requestRouterModels(webviewMessageHandler.ts), the Opencode Go fetch was gated behindif (opencodeGoApiKey), with the comment "its /models endpoint requires auth." That premise is wrong. WhenopencodeGoApiKeyisn't inapiConfigurationat fetch time (key just typed, stored in Secret Storage and not yet hydrated, or absent), Opencode Go is never added to the fetch candidates, sorouterModels["opencode-go"]stays{}(the seeded default). TheModelPickerthen renders an empty list and falls back toopencodeGoDefaultModelId(glm-5.1); since it only offers fetched router models, no custom model can be selected either.openrouterandvercel-ai-gateway— also public endpoints — are fetched unconditionally in the same handler. Opencode Go should be too.Reproduction (deterministic)
Running the shipped
getOpencodeGoModels()against the live endpoint:The endpoint returns the full 17-model list with no Authorization header, and the existing fetcher parses it correctly. So the fetcher was never the problem — the gate was.
Fix
Fetch Opencode Go unconditionally, like the other public routers, forwarding the API key when present and still flushing the cache when a new key is supplied (Refresh Models button). One-line behavioral change in the handler + the misleading comment corrected.
Tests
requestRouterModelstests that assumed"opencode-go": {}(it's now fetched like the other public routers).apiKey: undefinedand populatesrouterModelseven when no key is configured.webviewMessageHandler.spec.ts,tsc --noEmitclean,eslint --max-warnings=0clean.Summary by CodeRabbit