Support local overrides for editable Cline providers#412
Conversation
arafatkatze
force-pushed
the
arafatkatze/fix-openai-provider-overrides
branch
from
2cbda32 to
b1bcfc1
Compare
Greptile SummaryThis PR fixes editing built-in OpenAI-compatible Cline providers (e.g., LiteLLM) by proactively seeding a local Confidence Score: 4/5Safe to merge with minor follow-up polish; no correctness or data-loss issues found. No P0 or P1 issues found. All findings are P2 efficiency and UX consistency concerns (duplicate async calls during seeding, per-call disk I/O in listSdkProviderCatalog, and the UI edit-gate not explicitly mirroring the backend MANAGED_OAUTH_PROVIDER_IDS set). The core seeding-with-rollback logic is sound and the previously flagged modelsSourceUrl regression is addressed in this revision. src/cline-sdk/sdk-provider-boundary.ts — seeding helper functions have redundant async calls; web-ui/src/components/shared/cline-setup-section.tsx — edit-button guard may diverge from backend for oca/openai-codex.
|
| Filename | Overview |
|---|---|
| src/cline-sdk/sdk-provider-boundary.ts | Core seeding logic: proactively creates local registry entries for overrideable built-in providers before calling SDK update, with rollback on failure; adds getOverrideableBuiltInProvider called twice per seeding and readRegisteredProviderModels called twice, and listSdkProviderCatalog now reads models.json on every invocation. |
| src/core/api-contract.ts | Extends runtimeClineProviderCatalogItemSchema with custom, client, capabilities, modelsSourceUrl, headers, and timeoutMs fields needed for the edit modal to preload saved values. |
| src/cline-sdk/cline-provider-service.ts | Threads new catalog fields (client, capabilities, modelsSourceUrl, headers, timeoutMs, custom) from the SDK catalog into the runtime provider catalog item; straightforward and low-risk. |
| web-ui/src/components/shared/cline-setup-section.tsx | Adds edit-button visibility logic for openai-compatible built-in providers and preloads saved fields into the edit modal; the canEditSelectedProvider guard doesn't explicitly exclude backend-blocked managed OAuth providers (oca, openai-codex). |
| web-ui/src/components/shared/cline-add-provider-dialog.tsx | Fixes capability initialization to preserve explicit empty arrays rather than replacing them with add-mode defaults when initialValues.capabilities is []. |
| web-ui/src/components/shared/cline-setup-section.test.tsx | New focused test file covering edit-button visibility for openai-compatible, non-openai-compatible, managed OAuth, and custom providers, plus preload of advanced settings and capability overrides in the edit dialog. |
| web-ui/src/components/shared/cline-add-provider-dialog.test.tsx | Adds a test asserting that an explicit empty capability array in initialValues is not overridden by the default ["streaming","tools"]. |
Sequence Diagram
sequenceDiagram
participant UI as Edit Modal (UI)
participant Svc as cline-provider-service
participant Bdry as sdk-provider-boundary
participant Registry as models.json
participant SDK as ClineCore SDK
UI->>Svc: updateCustomProvider(input)
Svc->>Bdry: updateSdkCustomProvider(input)
Bdry->>Registry: readModelsRegistry()
Registry-->>Bdry: previousModelsState
Bdry->>SDK: getOverrideableBuiltInProvider(providerId)
SDK-->>Bdry: provider (openai-compatible) or null
alt No local entry and provider is overrideable
Bdry->>SDK: getModelsForProvider(providerId) [x2]
Bdry->>Registry: writeModelsRegistry(seeded entry)
Bdry->>SDK: updateLocalProvider(providerManager, input)
alt SDK update succeeds
Bdry-->>UI: ok
else SDK update fails
Bdry->>Registry: writeModelsRegistry(previousModelsState)
Bdry->>SDK: unregisterProvider(providerId)
Bdry-->>UI: error
end
else Local entry already exists
Bdry->>SDK: updateLocalProvider(providerManager, input)
Bdry-->>UI: ok
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/cline-sdk/sdk-provider-boundary.ts
Line: 544
Comment:
**`getOverrideableBuiltInProvider` called twice per seeding operation**
`getOverrideableBuiltInProvider(providerId)` is awaited on line 544 to determine `shouldSeedLocalOverride`, then called again inside `seedLocalProviderOverride` (which is only reached when the first result was truthy). Unless the SDK catalog can change between the two awaits, this doubles the catalog lookup cost every time a built-in provider is edited for the first time.
Consider passing the already-resolved provider into the seed function, or extract a shared variable:
```ts
const overrideableBuiltInProvider = await getOverrideableBuiltInProvider(providerId);
const shouldSeedLocalOverride = !hasLocalProvider && Boolean(overrideableBuiltInProvider);
// ...
if (shouldSeedLocalOverride) {
await seedLocalProviderOverride(input, overrideableBuiltInProvider!);
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cline-sdk/sdk-provider-boundary.ts
Line: 449-488
Comment:
**`readRegisteredProviderModels` called twice during a single seed**
`resolveSeedModelIds` (line 449) calls `readRegisteredProviderModels` to discover existing model IDs, and `seedLocalProviderOverride` calls it again on line 486 to retrieve model names for the registry entry. Both calls hit the same SDK endpoint for the same `providerId`. The result from the first call should be threaded through, or both pieces of work consolidated into a single async call.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cline-sdk/sdk-provider-boundary.ts
Line: 342-360
Comment:
**`listSdkProviderCatalog` now reads `models.json` on every catalog fetch**
The previous implementation was a single SDK call; the new one adds a `readFile` + `JSON.parse` round-trip for every invocation of `listSdkProviderCatalog`. If the catalog is polled frequently (e.g., on a timer or after each save), this adds repeated disk I/O on every fetch. Consider caching the registry read with a short TTL or invalidating the cache only when writes occur (the write path is already centralized in `writeModelsRegistry`).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web-ui/src/components/shared/cline-setup-section.tsx
Line: 177-180
Comment:
**UI edit-gate doesn't mirror backend `MANAGED_OAUTH_PROVIDER_IDS`**
The backend excludes `"cline"`, `"oca"`, and `"openai-codex"` from seeding via `MANAGED_OAUTH_PROVIDER_IDS`. The UI guard only excludes providers when `isOauthProviderSelected` is true. If `oca` or `openai-codex` have `client === "openai-compatible"` in the catalog and their controller state doesn't set `isOauthProviderSelected: true`, the Edit button will appear for them. Clicking Update would surface a "provider does not exist" error from the backend rather than a clear "not editable" message.
Consider adding an explicit exclusion set in the UI to keep the two sides in sync:
```ts
const MANAGED_OAUTH_PROVIDER_IDS = new Set(["cline", "oca", "openai-codex"]);
const canEditSelectedProvider =
controller.providerId.trim().length > 0 &&
!controller.isOauthProviderSelected &&
!MANAGED_OAUTH_PROVIDER_IDS.has(controller.normalizedProviderId) &&
(selectedProvider?.custom === true || selectedProvider?.client === "openai-compatible");
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "Wrap Cline setup tests with tooltip prov..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function normalizeProviderCapabilities( | ||
| values: readonly string[] | undefined, | ||
| ): RuntimeClineProviderCapability[] | undefined { | ||
| if (values === undefined) { | ||
| return undefined; | ||
| } | ||
| const capabilities = values?.filter((value): value is RuntimeClineProviderCapability => | ||
| EDITABLE_PROVIDER_CAPABILITIES.has(value as RuntimeClineProviderCapability), | ||
| ); | ||
| return capabilities ?? []; | ||
| } |
There was a problem hiding this comment.
?? [] fallback
After the if (values === undefined) return undefined guard, values is narrowed to readonly string[]. Array.prototype.filter always returns an array — it never returns null or undefined — so capabilities ?? [] is dead code and the ?? [] branch is never reached.
| function normalizeProviderCapabilities( | |
| values: readonly string[] | undefined, | |
| ): RuntimeClineProviderCapability[] | undefined { | |
| if (values === undefined) { | |
| return undefined; | |
| } | |
| const capabilities = values?.filter((value): value is RuntimeClineProviderCapability => | |
| EDITABLE_PROVIDER_CAPABILITIES.has(value as RuntimeClineProviderCapability), | |
| ); | |
| return capabilities ?? []; | |
| } | |
| const capabilities = values.filter((value): value is RuntimeClineProviderCapability => | |
| EDITABLE_PROVIDER_CAPABILITIES.has(value as RuntimeClineProviderCapability), | |
| ); | |
| return capabilities; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web-ui/src/components/shared/cline-setup-section.tsx
Line: 63-73
Comment:
**Unreachable `?? []` fallback**
After the `if (values === undefined) return undefined` guard, `values` is narrowed to `readonly string[]`. `Array.prototype.filter` always returns an array — it never returns `null` or `undefined` — so `capabilities ?? []` is dead code and the `?? []` branch is never reached.
```suggestion
const capabilities = values.filter((value): value is RuntimeClineProviderCapability =>
EDITABLE_PROVIDER_CAPABILITIES.has(value as RuntimeClineProviderCapability),
);
return capabilities;
```
How can I resolve this? If you propose a fix, please make it concise.![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: 29fdfc0ae5
ℹ️ 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".
| await writeModelsRegistry(previousModelsState); | ||
| providerManager.write(previousSettingsState); | ||
| ClineCore.Llms.unregisterProvider(providerId); | ||
| throw updateError; |
There was a problem hiding this comment.
If the second updateLocalProvider call fails after seeding a local override, the rollback path always calls Llms.unregisterProvider(providerId). In this flow providerId is an existing built-in provider (the whole branch is for built-ins missing a local entry), so a failed edit can remove that built-in from the in-memory catalog for the rest of the process, making the provider disappear until restart. Rollback should restore prior files/settings without unregistering the original built-in provider unconditionally.
Useful? React with 👍 / 👎.
| const canEditSelectedProvider = | ||
| controller.providerId.trim().length > 0 && | ||
| !controller.isOauthProviderSelected && | ||
| (selectedProvider?.custom === true || selectedProvider?.client === "openai-compatible"); |
There was a problem hiding this comment.
The new edit-gating logic requires the selected catalog entry to be marked custom or openai-compatible. But when catalog loading fails, getProviderCatalog() injects a fallback provider row for the currently configured provider without those fields, so the Edit button is hidden even though a provider is selected. In that degraded state users can no longer open the edit dialog to repair provider settings, which regresses the previous recovery path.
Useful? React with 👍 / 👎.
|
@greptileai review the pr again |
1 participant
Summary
Fixes editing built-in OpenAI-compatible Cline providers such as LiteLLM from the provider edit modal.
The modal previously routed every edit through
updateCustomProvider, but SDKupdateLocalProvideronly updates providers that already exist in the localmodels.jsonregistry. Built-in providers likelitellmexist in the SDK catalog, not in that local registry, so saving the edit surfacedprovider "litellm" does not exist.This PR makes first edit of an eligible built-in provider create a local override for the same provider ID before calling the SDK update path, so the SDK still owns model source fetching, settings persistence, and provider registration without depending on SDK error message text. It also makes the modal reload saved local override fields, including capabilities, instead of falling back to SDK catalog defaults.
Changes
client, capabilities, local/custom status,modelsSourceUrl, timeout, and custom headers through the runtime provider catalog.modelsSourceUrlvalues when no local override exists, while allowing local overrides to clear or replace that field.models.json.Validation
npm --prefix web-ui test -- cline-add-provider-dialog.test.tsx cline-setup-section.test.tsxnpm --prefix web-ui run typechecknpm --prefix web-ui run buildgit diff --checkManual UI test:
CLINE_DATA_DIR.provider "litellm" does not existorENOENTerrors.vision, clicked Update provider, reopened Edit, and confirmedvisionstayed selected.visionback off, clicked Update provider, reopened Edit, and confirmed all capability toggles stayed off.Note: root
npm run typecheckis currently blocked in this local dependency setup by existing@clinebot/coreexport mismatches unrelated to this PR.