Providers settings overhaul: runtime cards + fully-managed OpenCode subscriptions, Kimi, custom providers#852
Conversation
…, Kimi, custom providers Restructure Settings → Providers into Coding Agent cards plus a single "OpenCode — Universal Model Access" group. Seed subscription OAuth (GitHub Copilot, ChatGPT, GitLab, Poe, DigitalOcean) into ADE's isolated managed OpenCode dir via the server auth API with a device-code modal; persist provider inventory stale-while-revalidate so the long tail renders on first open; add first-class Moonshot AI key + Kimi for Coding membership entries; inject custom providers and model slugs into the managed OpenCode config; re-apply models.dev enrichment on refresh with last-synced visibility; type OpenCode launch failures (incl. Gatekeeper quarantine fix); dim configuration-gated models in the picker with a settings deep-link; wire web-client adapter + TUI Kimi parity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace semantics for ai.customProviders/customModelSlugs (union made removals impossible); surface both lists on the AI status payload and seed the settings form from config so saves are authoritative full-list writes; fall back to persisted OpenCode inventory when a forced probe fails; cancel an active OAuth flow on modal unmount; dedupe the web adapter's OpenCode types against shared/types (fixing a latent array-shape divergence) and the modelsDev refresh duplication; drop a dead inventory parameter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pin the quality-gate fixes as named regressions (mergeAiConfig replace semantics; stale persisted-inventory fallback on failed forced probes), fold the inventory-persistence test fragment into the module suite, and bring internal docs (settings/config-schema/chat/architecture) in line with the shipped providers overhaul. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesOpenCode access and provider configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
arul28
changed the title
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e in openCodeAuthService), address Greptile #3606770990 (ai.getStatus drops customProviders/customModelSlugs) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e07297d739
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!id || slugs.length === 0) { | ||
| setError("A custom provider needs an id and at least one model slug."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When adding a custom provider from Advanced, an entry with an id and model slugs but an empty Base URL passes this validation and then gets a success notice. That provider is unusable because addCustomProviders() skips custom providers with missing baseURL, so the saved entry never appears in the inventory/model picker and the user gets no actionable error. Include draft.baseUrl.trim() in the required fields before calling updateConfig.
Useful? React with 👍 / 👎.
…e saving custom providers) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c077777400
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _event: Electron.IpcRendererEvent, | ||
| payload: OpenCodeOAuthStatusEvent, | ||
| ) => cb(payload); | ||
| ipcRenderer.on(IPC.aiOpencodeOAuthStatus, listener); |
There was a problem hiding this comment.
When the active project is a remote runtime, opencodeOAuthStart is routed through callProjectRuntimeActionOr, and the remote service relays status changes into the runtime event buffer (registry.ts pushes kind: "opencodeOAuthStatus"). This subscriber only listens to the local Electron IPC channel, so in desktop sessions backed by a remote project the connect modal never sees connected, timeout, or failed and remains stuck in the waiting state even though the remote flow completed.
Useful? React with 👍 / 👎.
| const result = await window.ade.ai.opencodeOAuthStart({ | ||
| providerId, | ||
| methodIndex, | ||
| inputs: Object.keys(filteredInputs).length ? filteredInputs : undefined, | ||
| }); |
There was a problem hiding this comment.
If the user clicks Cancel while the modal is still in the "Opening…" state, this awaited start call may not have registered an active backend flow yet, so opencodeOAuthCancel is a no-op; once the await later resolves, the component is already closing but the backend poller/browser flow remains active until it connects or times out. This affects slow OpenCode server launch or authorize calls and leaves a cancelled sign-in running in the background.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/services/opencode/openCodeInventory.ts (1)
240-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not route aliases to model IDs OpenCode did not advertise.
Without the removed available-ID set, an advertised
claude-sonnet-4-6entry becomes a descriptor routed toclaude-sonnet-5, even when that target is absent fromprovider.models. Restore the availability check or preservemidas the OpenCode routing ID while canonicalizing only ADE's display identity.Also applies to: 436-436
🤖 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 `@apps/desktop/src/main/services/opencode/openCodeInventory.ts` around lines 240 - 243, Update normalizeOpenCodeProviderModel to prevent aliases from routing to unavailable OpenCode model IDs: restore the advertised model-ID availability check, or retain mid as the routing ID while only canonicalizing ADE’s display identity. Ensure entries such as claude-sonnet-4-6 continue routing to the provider model actually advertised in provider.models.apps/desktop/src/renderer/components/settings/ProvidersSection.tsx (2)
719-733: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDelete the OpenCode credential as well as the ADE-stored key.
Keys saved through this screen are also persisted through
setOpencodeProviderKey, but deletion only callsdeleteApiKey. The credential can therefore remain active in OpenCode after the user selects Delete.Add a service/IPC/preload operation that clears the OpenCode provider credential and invoke it before reporting removal.
As per coding guidelines, “Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes.”
As per path instructions, prefer fixing the underlying service or shared type rather than adding a renderer-only workaround.
🤖 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 `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx` around lines 719 - 733, Extend the existing service/IPC/preload contract used by setOpencodeProviderKey with an operation to clear the provider credential, keeping shared types and preload exposure synchronized. Update deleteApiKey to invoke this OpenCode credential-clearing operation before setNotice reports removal, while preserving the existing ADE key deletion and refresh flow.Sources: Coding guidelines, Path instructions
697-713: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle OpenCode key-registration failures before reporting success.
setOpencodeProviderKeyreturns{ ok, error }, but Line 700 ignores the result. The web adapter’s offline fallback returnsok: false, so the UI currently reports that the key was saved even when OpenCode rejected it.Proposed fix
if (options?.alsoOpenCode) { - try { - await window.ade.ai.setOpencodeProviderKey({ providerId: provider, key: trimmed }); - } catch { - // OpenCode registration is best-effort; the stored key still applies. - } + const result = await window.ade.ai.setOpencodeProviderKey({ + providerId: provider, + key: trimmed, + }); + if (!result.ok) { + throw new Error(result.error || "OpenCode rejected the provider key."); + } }🤖 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 `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx` around lines 697 - 713, Update the OpenCode registration flow in the key-save handler around setOpencodeProviderKey to inspect its { ok, error } result, including the offline fallback failure. If registration fails, surface the returned error and stop before invalidating caches, clearing verification state, showing the success notice, canceling editing, or refreshing status; only execute those success actions when registration succeeds.
🧹 Nitpick comments (1)
configuration/ai-providers.mdx (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapitalize "ID".
For correctness in technical writing, "ID" is typically capitalized.
💡 Proposed change
<Step title="More providers"> - Beyond the first-class rows, OpenCode reaches **160+ more providers**. Search the chip cloud, then paste a key against any provider by its id. The catalog is cached between sessions, so it renders the moment Settings opens. + Beyond the first-class rows, OpenCode reaches **160+ more providers**. Search the chip cloud, then paste a key against any provider by its ID. The catalog is cached between sessions, so it renders the moment Settings opens. </Step>🤖 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 `@configuration/ai-providers.mdx` around lines 54 - 56, Update the “More providers” Step text in the provider documentation to capitalize “id” as “ID,” preserving the surrounding wording and formatting.
🤖 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 `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 2052-2067: Update ensureOpenCodeOAuthStatusRelayBridge to retain
the unsubscribe function returned by addOpenCodeOAuthStatusListener and register
it with runtime.dispose. Ensure teardown removes the listener and prevents
further eventBuffer pushes into the disposed runtime, while preserving the
existing WeakSet guard and error handling.
In `@apps/desktop/src/main/services/opencode/openCodeAuthService.ts`:
- Around line 273-278: Update the key-storage flow around hooks.storeApiKey so
durable persistence is required for success: validate providerId and key before
storing, and when the mirror fails, do not return { ok: true }; instead roll
back the accepted server key where supported or return an explicit
failure/partial-failure result while preserving the existing warning log.
- Around line 71-85: Replace the OAuth polling setInterval flow with a
serialized loop that awaits each provider.list request before delaying for the
next two-second poll. Add a per-request AbortController/deadline, pass its
signal through the client request, and abort timed-out requests; retain the
existing five-minute overall polling limit and cleanup behavior. Update
listConnectedProviders and the polling logic together so no overlapping or
indefinitely pending requests remain.
- Around line 87-97: Validate the URL in openExternal before calling Electron’s
shell.openExternal, allowing only https URLs and explicitly permitted loopback
http URLs (localhost or loopback IPs). Reject file, custom-protocol, and other
schemes without attempting to open them, while preserving the existing
runtime-optional Electron handling and caller-visible fallback behavior.
In `@apps/desktop/src/main/services/opencode/openCodeInventory.ts`:
- Around line 85-94: Validate the parsed value in readPersistedInventoryFile
before memoizing or returning it. Check each inventory entry against the
expected persisted shape, including that its providers value is an array, and
discard incompatible entries or data so loadPersistedOpenCodeInventory always
receives valid inventory.
In `@apps/desktop/src/main/services/opencode/openCodeServerManager.ts`:
- Around line 539-557: Update openCodeServerManager.ts lines 539-557 so
bad-signature is returned only when explicit code-signature or
developer-verification evidence exists and probeOpenCodeBinaryQuarantine returns
a conclusive result; classify generic kills and unknown probe results as
unknown. Update openCodeBinaryManager.ts lines 188-203 so a successful
quarantine-attribute read returns quarantined, confirmed attribute absence
returns clean, and all other probe failures return unknown.
In `@apps/desktop/src/renderer/components/settings/OAuthConnectModal.tsx`:
- Around line 62-73: Update the OAuth method selection flow around methodIndex
and the inputs state so prompt defaults are rebuilt whenever the selected method
changes. When method changes, initialize each select prompt to its first option
value and other prompts to an empty string, ensuring submission uses the same
defaults displayed by the UI; preserve any intended existing input behavior
where applicable.
- Around line 139-146: In the OAuth start flow around setStartResult, call
openUrlInAdeBrowser with result.url immediately after receiving the response so
the client renderer opens the returned OAuth URL. Do not await this helper,
since it returns void; preserve the existing flowActiveRef and waiting-phase
updates.
In
`@apps/desktop/src/renderer/components/shared/ModelPicker/ModelPickerContent.tsx`:
- Around line 113-120: The auth-only filtering path must exclude models that
require configuration, even when their provider family is otherwise ready.
Update filterAvailable() to reject modelRequiresConfiguration(m) when authOnly
is enabled, and add a regression case covering this configured-state mismatch
while preserving existing non-auth-only behavior.
In `@apps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.ts`:
- Around line 919-928: Update the test fixtures in the OpenCode command-results
setup to match the shared response contracts: make authMethods.methods use the
expected array shape, and set refreshModelsDev.lastFetchedAt to a numeric
timestamp or null rather than an ISO string. Preserve the existing test behavior
while sourcing values consistent with the shared OpenCode types.
---
Outside diff comments:
In `@apps/desktop/src/main/services/opencode/openCodeInventory.ts`:
- Around line 240-243: Update normalizeOpenCodeProviderModel to prevent aliases
from routing to unavailable OpenCode model IDs: restore the advertised model-ID
availability check, or retain mid as the routing ID while only canonicalizing
ADE’s display identity. Ensure entries such as claude-sonnet-4-6 continue
routing to the provider model actually advertised in provider.models.
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx`:
- Around line 719-733: Extend the existing service/IPC/preload contract used by
setOpencodeProviderKey with an operation to clear the provider credential,
keeping shared types and preload exposure synchronized. Update deleteApiKey to
invoke this OpenCode credential-clearing operation before setNotice reports
removal, while preserving the existing ADE key deletion and refresh flow.
- Around line 697-713: Update the OpenCode registration flow in the key-save
handler around setOpencodeProviderKey to inspect its { ok, error } result,
including the offline fallback failure. If registration fails, surface the
returned error and stop before invalidating caches, clearing verification state,
showing the success notice, canceling editing, or refreshing status; only
execute those success actions when registration succeeds.
---
Nitpick comments:
In `@configuration/ai-providers.mdx`:
- Around line 54-56: Update the “More providers” Step text in the provider
documentation to capitalize “id” as “ID,” preserving the surrounding wording and
formatting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 604a8189-5f80-45fa-990a-ae9a08219ce8
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**
📒 Files selected for processing (37)
apps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsxapps/ade-cli/src/tuiClient/providerMetadata.test.tsapps/ade-cli/src/tuiClient/providerMetadata.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/aiSettingsStatus.tsapps/desktop/src/main/services/ai/apiKeyStore.tsapps/desktop/src/main/services/ai/authDetector.tsapps/desktop/src/main/services/ai/modelsDevService.test.tsapps/desktop/src/main/services/ai/modelsDevService.tsapps/desktop/src/main/services/config/projectConfigService.test.tsapps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/opencode/openCodeAuthService.test.tsapps/desktop/src/main/services/opencode/openCodeAuthService.tsapps/desktop/src/main/services/opencode/openCodeBinaryManager.tsapps/desktop/src/main/services/opencode/openCodeInventory.test.tsapps/desktop/src/main/services/opencode/openCodeInventory.tsapps/desktop/src/main/services/opencode/openCodeRuntime.test.tsapps/desktop/src/main/services/opencode/openCodeRuntime.tsapps/desktop/src/main/services/opencode/openCodeServerManager.test.tsapps/desktop/src/main/services/opencode/openCodeServerManager.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/settings/OAuthConnectModal.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.test.tsxapps/desktop/src/renderer/components/shared/ModelPicker/ModelPickerContent.tsxapps/desktop/src/renderer/components/shared/ProviderLogos.tsxapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/misc.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/config.tsconfiguration/ai-providers.mdx
| if ( | ||
| process.platform === "darwin" | ||
| && ( | ||
| code === "EPERM" | ||
| || lower.includes("operation not permitted") | ||
| || lower.includes("killed") | ||
| || lower.includes("code signature") | ||
| || lower.includes("developer cannot be verified") | ||
| ) | ||
| ) { | ||
| const quarantine = probeOpenCodeBinaryQuarantine(binaryPath); | ||
| if (quarantine === "quarantined") { | ||
| return { | ||
| kind: "quarantined", | ||
| binaryPath, | ||
| fixCommand: `xattr -d com.apple.quarantine "${binaryPath}"`, | ||
| }; | ||
| } | ||
| return { kind: "bad-signature", binaryPath }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep inconclusive macOS launch evidence classified as unknown.
The quarantine probe and launch classifier currently collapse uncertain outcomes into a definitive bad-signature diagnosis.
apps/desktop/src/main/services/opencode/openCodeServerManager.ts#L539-L557: require explicit signature evidence and a conclusive probe before returningbad-signature; classify generic kills or unknown probes asunknown.apps/desktop/src/main/services/opencode/openCodeBinaryManager.ts#L188-L203: returnquarantinedon a successful attribute read,cleanonly for confirmed absence, andunknownfor other failures.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 2 files
apps/desktop/src/main/services/opencode/openCodeServerManager.ts#L539-L557(this comment)apps/desktop/src/main/services/opencode/openCodeBinaryManager.ts#L188-L203
🤖 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 `@apps/desktop/src/main/services/opencode/openCodeServerManager.ts` around
lines 539 - 557, Update openCodeServerManager.ts lines 539-557 so bad-signature
is returned only when explicit code-signature or developer-verification evidence
exists and probeOpenCodeBinaryQuarantine returns a conclusive result; classify
generic kills and unknown probe results as unknown. Update
openCodeBinaryManager.ts lines 188-203 so a successful quarantine-attribute read
returns quarantined, confirmed attribute absence returns clean, and all other
probe failures return unknown.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 720e412b5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void window.ade.ai.opencodeOAuthCancel({ providerId }).catch(() => undefined); | ||
| return; | ||
| } | ||
| openUrlInAdeBrowser(result.url); |
There was a problem hiding this comment.
When OpenCode returns a URL, startOAuth already tries to open it through isAllowedOAuthExternalUrl, which rejects non-HTTPS/non-local HTTP URLs; however this renderer call opens the returned result.url again through the generic ADE browser path. In the case where a provider/OpenCode bug returns something like http://example.com/... or file://..., the main-process allow-list is bypassed and the renderer still opens it, so the OAuth URL should be validated with the same policy here or opened only by the backend.
Useful? React with 👍 / 👎.
| if (!res.ok) { | ||
| return { ok: false, error: `OpenCode DELETE /auth failed (${res.status}).` }; | ||
| } | ||
| return { ok: true }; |
There was a problem hiding this comment.
setProviderKey mirrors accepted provider keys into ADE's API key store, and buildProviderConfig() re-injects those stored keys into every new managed OpenCode server. This clear path only DELETEs the credential from the currently leased server and returns success, so any caller of the new ai.clearOpencodeProviderKey action outside the desktop two-step menu will see the key come back on the next server launch/status refresh unless the durable store is deleted here as well.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e65e74a10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const trimmed = editValue.trim(); | ||
| if (!trimmed) return; | ||
|
|
||
| setError(null); | ||
| setNotice(null); | ||
| try { | ||
| await window.ade.ai.storeApiKey(provider, trimmed); | ||
| if (options?.alsoOpenCode) { | ||
| const result = await window.ade.ai.setOpencodeProviderKey({ providerId: provider, key: trimmed }); |
There was a problem hiding this comment.
When alsoOpenCode is true, the key has already been written to ADE's durable key store before this OpenCode registration call runs. If this call returns ok: false or throws, the catch path leaves the editor open and shows an error, but the key remains stored and can be injected into the next managed OpenCode launch anyway. Since setOpencodeProviderKey already mirrors the key after a successful PUT /auth/{id}, call it instead of the earlier store write for OpenCode-backed saves, or roll the store write back on failure.
Useful? React with 👍 / 👎.
| await window.ade.ai.opencodeOAuthCancel({ providerId }).catch(() => undefined); | ||
| throw new Error("OpenCode returned an unsafe OAuth URL."); | ||
| } | ||
| openUrlInAdeBrowser(result.url); |
There was a problem hiding this comment.
For desktop OAuth flows that return a URL, openCodeAuthService.startOAuth already opens the same URL via Electron shell.openExternal before resolving. Calling openUrlInAdeBrowser here then opens a second auth page in ADE's browser, so a normal sign-in produces two browser surfaces for the same flow. Choose either the main-process auto-open or the renderer browser open, but not both.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdcc350a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| const connected = await hooks.listConnectedProviders(lease.url, deps.projectRoot, controller.signal); | ||
| if (activeFlows.get(providerId) !== flow) return; | ||
| if (connected.includes(providerId)) { |
There was a problem hiding this comment.
When an OAuth method connects a provider under a different id than the one used to start the flow, this poll never observes success because it only checks connected.includes(providerId). OpenCode auth methods can do this for enterprise variants such as GitHub Copilot Enterprise, where the completed credential is saved under a separate provider id; the user finishes login, but ADE keeps waiting until timeout and never refreshes inventory. Please detect the connected provider id produced by the flow, or compare against the connected set delta from before the flow started, instead of only the original id.
Useful? React with 👍 / 👎.
| opencodeProviders: opencodeInventory.providers, | ||
| opencodeProvidersStale: opencodeInventory.stale, | ||
| modelsDevLastFetchedAt: getModelsDevLastFetchedAt(), | ||
| customProviders: effectiveConfig?.ai?.customProviders, |
There was a problem hiding this comment.
When .ade/local.json contains ai.customProviders, this effective value is the local list because config merging gives local custom providers replacement precedence. The Settings save path uses this status field as the full-list write source for ai.updateConfig, which writes shared config, so adding a provider can disappear on the next refresh while also copying local-only provider definitions into .ade/config.json. Please expose the shared list/scope separately or save back to local when local owns this field.
Useful? React with 👍 / 👎.
| callProjectRuntimeActionOr("ai", "opencodeOAuthStart", { args }, () => | ||
| ipcRenderer.invoke(IPC.aiOpencodeOAuthStart, args), |
There was a problem hiding this comment.
The runtime action bridge already passes this object as toolArgs.args, and buildAiDomainService.opencodeOAuthStart reads args.providerId directly. Sending { args } makes remote/local runtime-bound settings calls arrive as { args: { providerId, ... } }, so requireNonEmptyString(args?.providerId, "providerId") throws and OpenCode OAuth cannot start outside the direct IPC fallback. Pass args directly here and in the adjacent OpenCode key/cancel calls.
Artifacts
Repro: focused Vitest harness for nested versus direct OpenCode OAuth args
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/preload/preload.ts
Line: 3930-3931
Comment:
**Unwrap runtime action args**
The runtime action bridge already passes this object as `toolArgs.args`, and `buildAiDomainService.opencodeOAuthStart` reads `args.providerId` directly. Sending `{ args }` makes remote/local runtime-bound settings calls arrive as `{ args: { providerId, ... } }`, so `requireNonEmptyString(args?.providerId, "providerId")` throws and OpenCode OAuth cannot start outside the direct IPC fallback. Pass `args` directly here and in the adjacent OpenCode key/cancel calls.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
/provider/auth,/oauth/authorize,PUT /auth/{id}) — never the user's personal opencode state. Claude Pro/Max via OpenCode intentionally excluded (Anthropic ToS).MOONSHOT_API_KEY, idmoonshotai) + Kimi for Coding membership entry (kimi-for-coding).ai.customProviders/ai.customModelSlugsconfig injected into the managed OpenCode config; replace-semantics merge so removals persist.xattrfix / bad signature / port conflict / timeout).personalChats.streamEvents), TUI Kimi aliases, adeActions registry (web/remote reachability); iOS verified no-change.Test plan
/qualitydual-review (no Blockers; all safe fixes applied + re-review ALL-CLEAR) and/testpasses run pre-ship.moonshotai/kimi-for-codingpresent in provider catalog.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR overhauls provider settings and OpenCode-powered model access. The main changes are:
Confidence Score: 4/5
One contained bug affects runtime-bound OpenCode auth flows.
Most service and UI changes are coherent, but the preload bridge passes OpenCode auth arguments in a shape the ADE action service does not consume.
apps/desktop/src/preload/preload.ts
What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Settings as Providers Settings UI participant Preload as Electron Preload / Web Adapter participant Actions as ADE AI Actions participant Auth as OpenCode Auth Service participant Server as Managed OpenCode Server participant Store as ADE API Key Store Settings->>Preload: opencodeOAuthStart / setProviderKey Preload->>Actions: "ai.opencode* runtime action" Actions->>Auth: startOAuth / setProviderKey Auth->>Server: "/provider/auth, /oauth/authorize, /auth/{id}" Auth-->>Settings: OAuth status events Auth->>Store: mirror or delete durable API key Auth->>Server: provider.list polling / inventory refresh%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Settings as Providers Settings UI participant Preload as Electron Preload / Web Adapter participant Actions as ADE AI Actions participant Auth as OpenCode Auth Service participant Server as Managed OpenCode Server participant Store as ADE API Key Store Settings->>Preload: opencodeOAuthStart / setProviderKey Preload->>Actions: "ai.opencode* runtime action" Actions->>Auth: startOAuth / setProviderKey Auth->>Server: "/provider/auth, /oauth/authorize, /auth/{id}" Auth-->>Settings: OAuth status events Auth->>Store: mirror or delete durable API key Auth->>Server: provider.list polling / inventory refreshComments Outside Diff (1)
apps/desktop/src/main/services/opencode/openCodeInventory.ts, line 147-151 (link)fingerprintOpenCodeConfigomitsai.customProvidersandai.customModelSlugs, butbuildOpenCodeMergedConfiginjects both into the OpenCode provider map. A non-forced inventory read within the TTL can reuse a cache result built before the custom provider edit, so newly added or removed custom models stay stale until the TTL expires or a forced refresh happens.Artifacts
Repro: focused Vitest test for stale custom provider inventory cache reuse
Repro: verbose Vitest output showing stale second result and provider.list called once
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "ship: iteration 5 — fix key persistence,..." | Re-trigger Greptile