Fix OpenCode and LM Studio model refresh#317
Conversation
|
@codex is attempting to deploy a commit to the arul28's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR refines LM Studio model discovery to handle REST API payload shape variations and improves fallback to OpenAI-compatible endpoints. It marks discovered models as loaded and integrates OpenCode binary cache clearing into the provider readiness cache invalidation flow with corresponding test coverage. ChangesLocal Model Discovery and Cache Management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| discoverySource: "lmstudio-openai", | ||
| loaded: true, | ||
| }] | ||
| : [], | ||
| })); | ||
| mockState.probeOpenCodeProviderInventory.mockImplementation(async (args: any) => { | ||
| expect(args.discoveredLocalModels).toContainEqual({ provider: "lmstudio", modelId }); | ||
| return { | ||
| modelIds: [`opencode/lmstudio/${modelId}`], | ||
| providers: [{ id: "lmstudio", name: "LM Studio", connected: true, modelCount: 1 }], | ||
| error: null, |
There was a problem hiding this comment.
expect inside mock implementation can silently pass if the service catches errors
Placing an expect assertion inside mockImplementation means a failed assertion throws inside the mock, which will propagate as a rejected promise from probeOpenCodeProviderInventory. If any upstream timePhase or similar utility in the service swallows that rejection (e.g., falls back to an empty result), the inner assertion failure is invisible and the test could produce a false pass. The safer pattern is to capture the call arguments and assert on them after getStatus resolves: const spy = vi.spyOn(...) or expect(mockState.probeOpenCodeProviderInventory).toHaveBeenCalledWith(expect.objectContaining({ discoveredLocalModels: expect.arrayContaining([...]) })).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/ai/aiIntegrationService.test.ts
Line: 430-440
Comment:
**`expect` inside mock implementation can silently pass if the service catches errors**
Placing an `expect` assertion inside `mockImplementation` means a failed assertion throws inside the mock, which will propagate as a rejected promise from `probeOpenCodeProviderInventory`. If any upstream `timePhase` or similar utility in the service swallows that rejection (e.g., falls back to an empty result), the inner assertion failure is invisible and the test could produce a false pass. The safer pattern is to capture the call arguments and assert on them after `getStatus` resolves: `const spy = vi.spyOn(...)` or `expect(mockState.probeOpenCodeProviderInventory).toHaveBeenCalledWith(expect.objectContaining({ discoveredLocalModels: expect.arrayContaining([...]) }))`.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Summary
/v1/modelsresponse when/api/v1/modelsreturns an unexpected object shapeTests
npx vitest run src/main/services/ai/localModelDiscovery.test.ts src/main/services/ai/aiIntegrationService.test.ts src/main/services/ai/authDetector.test.ts src/renderer/lib/modelOptions.test.tsnpm --prefix apps/desktop run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR fixes two independent issues: a stale binary-path cache that prevented OpenCode from re-discovering its binary after a forced status refresh, and LM Studio model discovery not surfacing models found via the OpenAI-compatible
/v1/modelsendpoint (both the missing fallback when/api/v1/modelsreturns an unexpected shape, and theloaded: falseflag that hid those models from runtime model lists).aiIntegrationService.ts: addsclearOpenCodeBinaryCache()toinvalidateProviderReadinessCaches(), consistent with the existing cache-clearing pattern.localModelDiscovery.ts: tightens the REST-API guard to require a.modelsarray (falling through to the OpenAI-compat path otherwise) and changesloadedfromfalsetotruefor models discovered via the OpenAI-compat endpoint.getStatus.Confidence Score: 4/5
Safe to merge; changes are well-scoped fixes to LM Studio model discovery and OpenCode binary caching with direct test coverage for each path.
Both changes are targeted and have corresponding tests. The main area to keep an eye on is the subtle state transition in
inspectLmStudioProvider: a live LM Studio instance whose/api/v1/modelsreturns an unexpected shape while/v1/modelsis also temporarily unreachable will now report as "unreachable" rather than "reachable_no_models". The test embedding anexpectinside a mock implementation is also worth a second look before the pattern spreads.localModelDiscovery.tsdeserves a second look around the new fallback path and its effect on thereachableflag.aiIntegrationService.test.tsis worth reviewing for theexpect-inside-mock pattern.Important Files Changed
.modelsis an array, falls through to the OpenAI-compat endpoint otherwise; also fixesloaded: false→loaded: truefor OpenAI-compat discovered models. Introduces a subtle edge case where an unexpected-but-reachable/api/v1/modelsresponse plus a failing/v1/modelsflips the health from "reachable_no_models" to "unreachable".clearOpenCodeBinaryCache()toinvalidateProviderReadinessCaches()so the binary path is re-probed on every forced status refresh. Change is minimal, correct, and consistent with the pattern used for other cache-clearing calls in the same function.loaded: trueassertion on existing compat test. Coverage is good; the new "unexpected shape" test correctly mocks two sequential fetches.expectassertion insideprobeOpenCodeProviderInventory.mockImplementation, which can produce a false pass if upstream error handling swallows the rejection.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[inspectLmStudioProvider] --> B[fetch /api/v1/models] B --> C{restPayload non-null\nAND .models is array?} C -- Yes --> D[Parse models array\nfrom REST payload] D --> E{Any loaded instances?} E -- Yes --> F[Return ready\ndiscoverySource: lmstudio-rest\nloaded: true] E -- No --> G[Return reachable_no_models] C -- No\nnew fallback path --> H[fetch /v1/models] H --> I{openAiPayload non-null?} I -- No --> J[Return unreachable] I -- Yes --> K{Any model entries\nin .data?} K -- Yes --> L[Return ready\ndiscoverySource: lmstudio-openai\nloaded: true ← was false] K -- No --> M[Return reachable_no_models]Comments Outside Diff (1)
apps/desktop/src/main/services/ai/localModelDiscovery.ts, line 177-235 (link)Before this PR, any 200 OK response from
/api/v1/modelsguaranteedreachable: true. Now, if that response has an unexpected shape (no.modelsarray) and/v1/modelsalso fails or is unreachable, the returned inspection will havereachable: falseandhealth: "unreachable". A running LM Studio instance that returns an unexpected/api/v1/modelsbody during startup would briefly appear completely unreachable in the UI instead of "reachable with no loaded models".Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix LM Studio OpenAI model discovery" | Re-trigger Greptile