feat: Cursor/OpenCode first-class provider support - #21
Merged
Merged
Conversation
…scovery, settings, picker Wire harness model discovery into the provider registry so Cursor and OpenCode models are automatically detected via the Elixir harness RPC (provider.listModels) and pushed to the frontend via providersUpdated. Changes: - ProviderRegistry queries harness for Cursor/OpenCode model lists on startup and every 60s, merging with custom models from settings - HarnessClientAdapter exposes listProviderModels() to the registry - Settings page shows all 4 providers as first-class cards with enable/disable, model list, and custom model management - Chat picker merges custom models from settings into the model list so harness-routed providers always have selectable models - Playwright config port default aligned to dev harness (5734) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds harness-discovered model support: ProviderRegistry queries harness adapters for models (cursor, opencode), merges results with per-provider customModels from settings, polls every 60s to refresh, adds a new harness adapter method, adjusts layer wiring, and surfaces merged model options and provider status in the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Server as ProviderRegistry
participant Harness as Harness Client
participant Settings as ServerSettingsService
UI->>Server: Request providerStatuses / models
Server->>Harness: listProviderModels("cursor"/"opencode")
activate Harness
Harness-->>Server: [ { slug, name }... ]
deactivate Harness
Server->>Settings: get settings.providers[p].customModels
activate Settings
Settings-->>Server: [ custom model slugs ]
deactivate Settings
Server->>Server: Merge discovered models + customModels (dedupe by slug)
Server-->>UI: providerStatuses including merged models
Note over Server: Poll every 60s -> re-run listProviderModels and syncProviders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
size:L
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 44-50: The mapping that builds ServerProviderModel is overwriting
custom flags by hardcoding isCustom: false; change the models.map and the
similar block that flattens custom slugs to preserve the original property
(e.g., use isCustom: m.isCustom ?? false or isCustom: Boolean(m.isCustom)) so
settings-defined/custom models keep isCustom=true; ensure you reference the
ServerProviderModel construction (slug, name, isCustom, capabilities) and the
other flattening code that creates custom slugs so both places preserve
m.isCustom.
- Around line 36-43: The provider snapshot currently marks providers as
installed/ready/authenticated even when listProviderModels() fails; modify the
logic in ProviderRegistry (the code that builds the returned object with fields
provider, enabled, installed, version, status, authStatus, checkedAt) so that if
listProviderModels() rejects or returns an error you do not set installed:
true/status: "ready"/authStatus: "authenticated"; instead mark the provider as
not installed or unreachable (e.g., installed: false or status:
"unavailable"/"unreachable") and set authStatus to an appropriate failure state,
include the error context in checkedAt or an error field, and ensure the failure
path from listProviderModels() is propagated into the snapshot rather than
collapsing to an empty array.
- Around line 127-135: The polling fiber is terminated by an error because
Effect.orElseSucceed is applied outside Effect.repeat, so a failing
syncProviders() stops the repeating schedule; to keep polling alive wrap or
handle errors inside the repeated effect instead: apply error handling to
syncProviders() (e.g., syncProviders().pipe(Effect.orElseSucceed(() =>
undefined)) or syncProviders().pipe(Effect.catchAll(() =>
Effect.succeed(undefined))) before piping into Effect.delay and Effect.repeat
(keep Schedule.spaced and Effect.forkScoped unchanged) so each iteration
converts failures to a harmless success and the repeat schedule continues.
In `@apps/web/playwright.config.ts`:
- Line 12: The Playwright config's baseURL default currently uses 5734 (baseURL:
`http://localhost:${process.env.PORT ?? 5734}`) which is inconsistent with the
project's canonical port 5733 used in apps/web/vite.config.ts and
scripts/dev-runner.ts; update the fallback port in the Playwright config to 5733
(or, if 5734 is intended, update the fallback ports in vite.config.ts and
dev-runner.ts instead) so the baseURL construction in the Playwright config and
the PORT defaults across the project are consistent.
In `@apps/web/src/routes/_chat.settings.tsx`:
- Around line 524-536: The code marks enabled harness providers as "ready" even
when liveProvider is missing; change the logic in the statusKey and summary
computation so that isHarnessProvider requires a real liveProvider to be
"ready". Specifically, update the statusKey ternary (the expression computing
statusKey using providerSettings, liveProvider and providerConfig) to return a
non-ready state like "checking" or "warning" when isHarnessProvider &&
!liveProvider && providerConfig.enabled, and update the summary (the branch that
currently returns { headline: providerSettings.harnessDescription!, detail: null
} when isHarnessProvider && !liveProvider) to produce a checking/warning-style
summary instead of a success-style one (or defer to getProviderSummary only when
liveProvider exists), so ProviderStatusBanner will show the appropriate warning
until the server reports an actual status.
🪄 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
Run ID: 5f88de72-4d22-4cca-b17e-9e576e8026d8
📒 Files selected for processing (7)
apps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Services/HarnessClientAdapter.tsapps/server/src/serverLayers.tsapps/web/playwright.config.tsapps/web/src/components/ChatView.tsxapps/web/src/routes/_chat.settings.tsx
…esilience, port alignment - ProviderRegistry: propagate listProviderModels failures into snapshot (installed/status/authStatus reflect unreachable harness instead of unconditional ready/authenticated) - ProviderRegistry: preserve isCustom on custom models from settings instead of hardcoding false - ProviderRegistry: move orElseSucceed inside the repeated effect so a single syncProviders failure doesn't kill the 60s polling schedule - Playwright config: align default port to 5733 (matches vite.config.ts and dev-runner.ts) - Settings page: harness providers without liveProvider show "checking" status with pulsing dot instead of premature "ready" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ProviderRegistry: make HarnessClientAdapter an optional dependency via Effect.serviceOption so the layer composes without requiring the harness in index.ts, main.test.ts, or ProviderRegistry.test.ts - ProviderRegistry: skip harness polling fiber when adapter is absent - terminalStateStore.test: provide localStorage shim via vi.hoisted (Bun's stub lacks setItem/getItem/clear, breaking zustand persist) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/server/src/provider/Layers/ProviderRegistry.ts (1)
85-99:⚠️ Potential issue | 🟠 MajorSeparate “no models discovered” from “harness unreachable.”
reachablecurrently means “the adapter returned a non-empty array”, not “the RPC succeeded”. The currentHarnessClientManager.listProviderModels()implementation already converts transport failures into[], so a reachable provider with zero discovered models is emitted asinstalled: false/"warning"with a harness-unreachable message. Please have the adapter either throw on lookup failures or return an explicit{ models, reachable }shape, and deriveinstalled/status/authStatusfrom that signal instead ofsuccess.length > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderRegistry.ts` around lines 85 - 99, The code treats a non-empty discoveredModels.success as the signal for reachability; instead make reachability explicit by having harnessAdapter.listProviderModels either throw on transport failures or return a shape like { models, reachable } (update HarnessClientManager.listProviderModels accordingly), then replace the current reachable calculation and the installed/status/authStatus decisions (references: discoveredModels, reachable, harnessAdapter.listProviderModels, HarnessClientManager.listProviderModels, installed/status/authStatus) to derive those flags from the new reachable boolean (or thrown error) instead of discoveredModels.success.length > 0 so that “no models” and “harness unreachable” are distinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 101-107: The merge logic in ProviderRegistry that builds allModels
from models and customModels loses custom provenance when a discovered model
slug collides with a saved custom slug and also doesn't update seen for accepted
custom slugs causing duplicates; modify the merge so slugs are deduplicated by
slug while preferring the saved custom entry (preserve isCustom: true) over the
discovered one — e.g., build a map keyed by slug starting from discovered
models, then iterate customModels and overwrite or insert entries with {slug,
name: slug, isCustom: true}, and ensure the seen set (or equivalent) is updated
when a custom slug is accepted so repeated settings don't produce duplicates
(touch symbols: models, customModels, seen, allModels, ProviderRegistry).
In `@apps/web/src/terminalStateStore.test.ts`:
- Around line 6-9: The guard that decides whether to shim
globalThis.localStorage only checks for setItem but later uses
localStorage.clear(), so update the eligibility check around
globalThis.localStorage (the if block) to verify all methods your test uses—at
minimum setItem, getItem, removeItem, and clear—exist and are functions before
replacing with a shim; adjust the conditional to check typeof
globalThis.localStorage.clear === "function" (and similarly for
getItem/removeItem) so a partial implementation can't bypass the guard and cause
a runtime error when terminalStateStore.test.ts calls localStorage.clear().
---
Duplicate comments:
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 85-99: The code treats a non-empty discoveredModels.success as the
signal for reachability; instead make reachability explicit by having
harnessAdapter.listProviderModels either throw on transport failures or return a
shape like { models, reachable } (update HarnessClientManager.listProviderModels
accordingly), then replace the current reachable calculation and the
installed/status/authStatus decisions (references: discoveredModels, reachable,
harnessAdapter.listProviderModels, HarnessClientManager.listProviderModels,
installed/status/authStatus) to derive those flags from the new reachable
boolean (or thrown error) instead of discoveredModels.success.length > 0 so that
“no models” and “harness unreachable” are distinct.
🪄 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
Run ID: 6b5c64f7-aa5b-4fcb-ae5f-76b8f5ce661a
📒 Files selected for processing (2)
apps/server/src/provider/Layers/ProviderRegistry.tsapps/web/src/terminalStateStore.test.ts
Comment on lines
+101
to
+107
| const customModels = settings?.providers[provider]?.customModels ?? []; | ||
| const seen = new Set(models.map((m) => m.slug)); | ||
| const allModels = [ | ||
| ...models, | ||
| ...customModels | ||
| .filter((slug: string) => !seen.has(slug)) | ||
| .map((slug: string) => ({ slug, name: slug, isCustom: true as const })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Preserve custom provenance when a saved slug collides with discovery.
This merge only appends custom slugs that are not already in seen. If a user has a saved custom model and the harness later discovers the same slug, the merged snapshot keeps only the discovered entry with isCustom: false, which breaks custom-model management again. seen also is not updated for accepted custom slugs, so repeated values in settings still publish duplicates.
Possible fix
- const customModels = settings?.providers[provider]?.customModels ?? [];
- const seen = new Set(models.map((m) => m.slug));
- const allModels = [
- ...models,
- ...customModels
- .filter((slug: string) => !seen.has(slug))
- .map((slug: string) => ({ slug, name: slug, isCustom: true as const })),
- ];
+ const customModels = settings?.providers[provider]?.customModels ?? [];
+ const customSlugs = new Set(customModels);
+ const seen = new Set<string>();
+ const allModels = [
+ ...models.map((model) => {
+ seen.add(model.slug);
+ return {
+ ...model,
+ isCustom: customSlugs.has(model.slug),
+ };
+ }),
+ ...customModels
+ .filter((slug: string) => {
+ if (seen.has(slug)) {
+ return false;
+ }
+ seen.add(slug);
+ return true;
+ })
+ .map((slug: string) => ({ slug, name: slug, isCustom: true as const })),
+ ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderRegistry.ts` around lines 101 - 107,
The merge logic in ProviderRegistry that builds allModels from models and
customModels loses custom provenance when a discovered model slug collides with
a saved custom slug and also doesn't update seen for accepted custom slugs
causing duplicates; modify the merge so slugs are deduplicated by slug while
preferring the saved custom entry (preserve isCustom: true) over the discovered
one — e.g., build a map keyed by slug starting from discovered models, then
iterate customModels and overwrite or insert entries with {slug, name: slug,
isCustom: true}, and ensure the seen set (or equivalent) is updated when a
custom slug is accepted so repeated settings don't produce duplicates (touch
symbols: models, customModels, seen, allModels, ProviderRegistry).
Comment on lines
+6
to
+9
| if ( | ||
| typeof globalThis.localStorage === "undefined" || | ||
| typeof globalThis.localStorage.setItem !== "function" | ||
| ) { |
There was a problem hiding this comment.
Broaden the shim eligibility check to match required methods.
The guard only validates setItem, but Line 37 unconditionally calls localStorage.clear(). A partially implemented storage object can bypass this guard and still crash setup.
Suggested patch
vi.hoisted(() => {
if (
typeof globalThis.localStorage === "undefined" ||
- typeof globalThis.localStorage.setItem !== "function"
+ typeof globalThis.localStorage.getItem !== "function" ||
+ typeof globalThis.localStorage.setItem !== "function" ||
+ typeof globalThis.localStorage.removeItem !== "function" ||
+ typeof globalThis.localStorage.clear !== "function"
) {📝 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.
Suggested change
| if ( | |
| typeof globalThis.localStorage === "undefined" || | |
| typeof globalThis.localStorage.setItem !== "function" | |
| ) { | |
| if ( | |
| typeof globalThis.localStorage === "undefined" || | |
| typeof globalThis.localStorage.getItem !== "function" || | |
| typeof globalThis.localStorage.setItem !== "function" || | |
| typeof globalThis.localStorage.removeItem !== "function" || | |
| typeof globalThis.localStorage.clear !== "function" | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/terminalStateStore.test.ts` around lines 6 - 9, The guard that
decides whether to shim globalThis.localStorage only checks for setItem but
later uses localStorage.clear(), so update the eligibility check around
globalThis.localStorage (the if block) to verify all methods your test uses—at
minimum setItem, getItem, removeItem, and clear—exist and are functions before
replacing with a shim; adjust the conditional to check typeof
globalThis.localStorage.clear === "function" (and similarly for
getItem/removeItem) so a partial implementation can't bypass the guard and cause
a runtime error when terminalStateStore.test.ts calls localStorage.clear().
- ProviderCommandReactor: only enforce provider binding when there is an active session — a first turn with an explicit provider choice should be honoured, not rejected against the thread default - ProviderCommandReactor: prefer the requested provider over the thread default when no active session exists - ProviderCommandReactor test: mock startSession now respects the provider field from the caller instead of always using the harness default - CodexAdapter: add codex/event/plan_delta → turn.proposed.delta mapping (was present in codexEventMapping.ts but missing from the adapter's own event handler) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 27, 2026
This was referenced Mar 29, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
provider.listModelsRPCTest plan
pixi run dev— verify all 4 providers appear in settings page with green status and model countsbun run scripts/council.ts "test"— all 4 providers respond🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests