feat: consolidate provider discovery, workspace, and smart routing upgrades#434
feat: consolidate provider discovery, workspace, and smart routing upgrades#434HaydernCenterpoint wants to merge 11 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds a provider directory and metadata pipeline, safe model discovery with race protection, a redesigned provider catalog and connection UI, provider icon fallback resolution, usage summaries, localized copy, and smart-routing combo generation with API and Models-page controls. ChangesProvider platform updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProviderModal
participant DiscoveryAPI
participant UpstreamModels
participant ModalState
User->>ProviderModal: Select preset, enter API key
ProviderModal->>DiscoveryAPI: POST /api/provider-presets/discover
DiscoveryAPI->>UpstreamModels: Fetch live /models (if live-discovery enabled)
UpstreamModels-->>DiscoveryAPI: Return model list or error
DiscoveryAPI-->>ProviderModal: Normalized models + source
ProviderModal->>ModalState: Update selectedModels, defaultModel
User->>ProviderModal: (Optional) change endpoint → abort stale discover
ModalState-->>User: Show discovered models or reference note
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: b8928910df
ℹ️ 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 selectedSubset = selectedModels.length > 0 && selectedModels.length < discoveredModels.length | ||
| ? selectedModels | ||
| : undefined; |
There was a problem hiding this comment.
When the OpenRouter “free models only” discovery path succeeds, every displayed model is already the filtered free set, so leaving them all checked makes selectedModels.length === discoveredModels.length and selectedSubset becomes undefined. The POST then saves the provider without a selectedModels allowlist, and the normal live OpenRouter catalog will expose paid models later despite the user choosing the free-only setup; persist the discovered IDs whenever openRouterFreeOnly is true.
Useful? React with 👍 / 👎.
| const rawId = typeof item.id === "string" ? item.id | ||
| : typeof item.name === "string" ? item.name | ||
| : typeof item.model === "string" ? item.model | ||
| : ""; |
There was a problem hiding this comment.
When this parser is used for Google's { models: [...] } response, it now converts every row with a name into a catalog model without checking supportedGenerationMethods. Google's list also contains embedding-only models such as rows that only support embedContent, but the Google adapter routes chat via generateContent/streamGenerateContent, so those entries become selectable Codex chat models and then fail upstream; skip Google rows that do not advertise a generation method.
Useful? React with 👍 / 👎.
| const id = `auto-${mode}`; | ||
| config.combos = { ...(config.combos ?? {}), [id]: combo }; | ||
| saveConfig(config); |
There was a problem hiding this comment.
If a user already has a provider named auto-balance, auto-cost, or auto-intelligence, this endpoint still writes a combo with the same id. The normal combo validator rejects combo ids that collide with provider names, so saving here can leave config.json invalid on the next load; run the same combo validation or choose a non-colliding id before persisting the smart route.
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: 20
🤖 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 `@gui/public/provider-icons/LICENSE.simple-icons`:
- Line 5: Replace the corrupted CC0 license text in LICENSE.simple-icons with
the official CC0 1.0 source, preserving the complete notice and correcting all
replacement characters at the referenced lines. Save the file explicitly as
valid UTF-8 so the original quotation marks and apostrophes render correctly.
In `@gui/src/components/AddProviderModal.tsx`:
- Around line 254-258: Update the default-model assignment in the discovery flow
around setSelectedModels and setForm so form.defaultModel remains within the
freshly discovered model IDs. Preserve an existing default only when its ID
appears in models; otherwise select models[0].id as the fallback, including when
freeOnly is enabled, while retaining the current behavior when no models are
discovered.
In `@gui/src/components/provider-catalog/ProviderCatalog.tsx`:
- Around line 230-234: Update the provider-catalog detail rendering in
ProviderCatalog so aria-live="polite" applies only to the empty-state or
asynchronous status text, not the detail subtree containing the interactive
provider form. Keep the accounts and select-provider messages announced while
rendering detail outside the live region.
In `@gui/src/components/providers/OAuthPanel.tsx`:
- Around line 55-72: Update the OAuthPanel key-provider row rendering to derive
its dot color and status text from provider.hasApiKey, showing a not-configured
state when false instead of always using prov.hasApiKey. Restore a kind ===
"key" remediation action using the existing prov.keyAdd translation and the
appropriate API/setup handler. Add the prov.noApiKey translation to all
supported locale resources, preserving account-provider behavior.
In `@gui/src/components/providers/ProviderCardList.tsx`:
- Around line 162-172: Update the outer resource-block guard in ProviderCardList
so it uses the existing renderable-content predicate from the component’s
earlier logic, rather than quota truthiness alone. Ensure the block renders only
when QuotaBars has rows or the total-token fallback is available, while
preserving both child render paths unchanged.
- Around line 165-169: Update ProviderCardList’s token-count fallback to use the
active i18n locale instead of formatTokenCount’s default "en" locale. Confirm
the useI18n export shape and obtain the locale from the i18n context, then pass
it to formatTokenCount(totalTokens, locale) while preserving the existing
rendering conditions and translation usage.
In `@gui/src/i18n/de.ts`:
- Around line 201-202: Update the German locale export in de.ts to import TKey
from en.ts and declare the locale object as Record<TKey, string>, matching the
type-checking pattern used by ja, ko, ru, and zh. Remove the local keyof typeof
de TKey declaration while preserving all existing German translations.
In `@gui/src/pages/Models.tsx`:
- Around line 143-165: Update applySmartRouting to parse both response.json()
and comboResponse.json() with the file’s established catch-to-null pattern.
Treat an unparsable mutation response as a generic smart-routing failure, and
ensure an unparsable combos response does not expose a raw SyntaxError or
incorrectly report the completed mutation as failed; preserve the existing
status and combo-update behavior for valid responses.
- Around line 47-48: Add the mode-specific locale entries
models.smartRouting_intelligenceHint, models.smartRouting_balanceHint, and
models.smartRouting_costHint to de.ts, en.ts, ja.ts, ko.ts, ru.ts, and zh.ts,
matching the existing smart-routing terminology and the lookup built from
SmartRoutingMode in Models.tsx. Preserve the current mode-specific lookup
behavior.
In `@gui/src/styles.css`:
- Around line 1062-1066: Reorder the responsive CSS so the max-width: 760px
mobile rules have final precedence over the max-width: 980px rules. Move the
block containing .provider-connection-groups and
.provider-connection-group--custom above the 760px block, or restrict it to
761px–980px, while preserving the mobile single-column layout and custom-group
styling.
In `@gui/src/styles/provider-catalog.css`:
- Around line 95-98: Synchronize the `.provider-detail-actions` bleed values
with the `18px` padding introduced by the `@media (max-width: 1050px)` rule on
`.provider-catalog-detail`. In that breakpoint, override the bar’s bottom and
horizontal/vertical negative margins and matching horizontal padding so it
bleeds exactly 18px, preserving the existing sticky layout and the separate
`max-width: 760px` behavior.
- Around line 12-19: Remove the fixed header-height subtraction from
.provider-catalog, including the mobile calc(100% - 56px) override, and let
.provider-loader-card lay out its header and catalog content using its existing
flex structure. Preserve the mobile .provider-loader-card > .modal-head padding
adjustment so the catalog and sticky action bar remain fully reachable when the
header grows.
In `@gui/tests/provider-discovery-race.test.tsx`:
- Around line 79-93: Move request-body validation out of the fetch mock so
assertion failures are not swallowed by AddProviderModal’s discovery error
handling. In the fetch stub, record each parsed body in a discoveryBodies
collector; after each Discover models click, assert the corresponding body
contains presetId "alpha" and the expected regional provider.baseUrl, including
the updated URL after switching endpoints.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 418-421: Update allowsModelsEnvelope in provider-fetch.ts to
determine Google’s mode using the shared effectiveGoogleMode helper, including
the registry fallback used by buildModelsRequest, rather than defaulting
directly from prov.googleMode. Preserve the existing /api/tags detection and
envelope handling, and apply the same consistency fix to the identical logic in
provider-routes.ts.
In `@src/combos/smart-routing.ts`:
- Line 1: Use the canonical SmartRoutingMode and SMART_ROUTING_MODES from
src/combos/smart-routing.ts as the GUI source of truth: expose that mode list
through the existing runtime config/bootstrap payload, consume it in
gui/src/pages/Models.tsx, and remove the local literal union and hardcoded mode
array. Ensure the mode-driven buttons and i18n key lookups iterate over the
shared runtime values so server-side additions or removals are reflected and
type-checked by the GUI.
- Around line 132-142: Update buildSmartRoutingCombo to enforce each provider’s
selectedModels allow-list while constructing eligible models, using the existing
model/provider matching semantics and treating an undefined or empty restriction
consistently with shared configuration filtering. Keep the existing
disabled-provider, disabled-model, and context-window checks unchanged so raw
catalog inputs cannot bypass routing configuration.
In `@src/providers/free-directory.ts`:
- Around line 128-168: Add the missing provider label mappings to LABELS for
openrouter, siliconflow, opencode, monsterapi, deepinfra, deepseek, stepfun, and
ai21, using the required brand casing: OpenRouter, SiliconFlow, OpenCode,
MonsterAPI, DeepInfra, DeepSeek, StepFun, and AI21. Leave the fallback label
generation and FREE_PROVIDER_DIRECTORY mapping unchanged.
In `@src/server/management/combo-routes.ts`:
- Around line 102-108: Update the smart-routing handler’s response construction
to derive the model field with comboPublicModelId(id, combo) instead of
interpolating combo/${id}. Keep the remaining response fields unchanged and
ensure this endpoint uses the same public model identifier as the sibling combo
handlers.
In `@src/server/management/provider-routes.ts`:
- Around line 489-490: Update the allowsModelsEnvelope calculation to use the
already-resolved effectiveGoogleMode from buildModelsRequest instead of directly
evaluating entry.googleMode, while preserving the existing Google adapter and
/api/tags conditions.
In `@tests/combo-management-api.test.ts`:
- Around line 156-207: Add a regression test alongside the existing
smart-routing tests that applies the “intelligence” mode, changes the
enabled-model set, then applies the same mode again. Assert the existing
“auto-intelligence” combo is refreshed with the new targets, remains a single
key without stale or duplicate entries, and configuration persistence continues
to work.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: a8b7fbdf-2825-47b7-810f-0c87868674b9
⛔ Files ignored due to path filters (54)
gui/public/provider-icons/agentrouter.pngis excluded by!**/*.pnggui/public/provider-icons/agnesai.svgis excluded by!**/*.svggui/public/provider-icons/ai21-color.svgis excluded by!**/*.svggui/public/provider-icons/aionlabs-color.svgis excluded by!**/*.svggui/public/provider-icons/api-airforce.svgis excluded by!**/*.svggui/public/provider-icons/arcee-color.svgis excluded by!**/*.svggui/public/provider-icons/baichuan-color.svgis excluded by!**/*.svggui/public/provider-icons/bazaarlink.svgis excluded by!**/*.svggui/public/provider-icons/blackbox.pngis excluded by!**/*.pnggui/public/provider-icons/bluesminds.svgis excluded by!**/*.svggui/public/provider-icons/cerebras-color.svgis excluded by!**/*.svggui/public/provider-icons/cohere-color.svgis excluded by!**/*.svggui/public/provider-icons/coze.svgis excluded by!**/*.svggui/public/provider-icons/deepinfra-color.svgis excluded by!**/*.svggui/public/provider-icons/duckduckgo.svgis excluded by!**/*.svggui/public/provider-icons/freemodel-dev.svgis excluded by!**/*.svggui/public/provider-icons/friendli.svgis excluded by!**/*.svggui/public/provider-icons/hackclub.svgis excluded by!**/*.svggui/public/provider-icons/hyperbolic-color.svgis excluded by!**/*.svggui/public/provider-icons/iflytek-color.svgis excluded by!**/*.svggui/public/provider-icons/inference.svgis excluded by!**/*.svggui/public/provider-icons/kilocode.svgis excluded by!**/*.svggui/public/provider-icons/liquid.svgis excluded by!**/*.svggui/public/provider-icons/litellm.pngis excluded by!**/*.pnggui/public/provider-icons/llm7.svgis excluded by!**/*.svggui/public/provider-icons/longcat-color.svgis excluded by!**/*.svggui/public/provider-icons/monsterapi.svgis excluded by!**/*.svggui/public/provider-icons/morph-color.svgis excluded by!**/*.svggui/public/provider-icons/nebius.svgis excluded by!**/*.svggui/public/provider-icons/nlpcloud.svgis excluded by!**/*.svggui/public/provider-icons/nousresearch.svgis excluded by!**/*.svggui/public/provider-icons/novita-color.svgis excluded by!**/*.svggui/public/provider-icons/nscale.pngis excluded by!**/*.pnggui/public/provider-icons/ovh.svgis excluded by!**/*.svggui/public/provider-icons/pollinations.svgis excluded by!**/*.svggui/public/provider-icons/predibase.pngis excluded by!**/*.pnggui/public/provider-icons/provider-generic.svgis excluded by!**/*.svggui/public/provider-icons/publicai.svgis excluded by!**/*.svggui/public/provider-icons/puter.svgis excluded by!**/*.svggui/public/provider-icons/qoder-color.svgis excluded by!**/*.svggui/public/provider-icons/reka.pngis excluded by!**/*.pnggui/public/provider-icons/requesty.svgis excluded by!**/*.svggui/public/provider-icons/sambanova-color.svgis excluded by!**/*.svggui/public/provider-icons/scaleway.svgis excluded by!**/*.svggui/public/provider-icons/sensenova-color.svgis excluded by!**/*.svggui/public/provider-icons/siliconcloud-color.svgis excluded by!**/*.svggui/public/provider-icons/spark-color.svgis excluded by!**/*.svggui/public/provider-icons/stepfun-color.svgis excluded by!**/*.svggui/public/provider-icons/t3-web.svgis excluded by!**/*.svggui/public/provider-icons/tencent-color.svgis excluded by!**/*.svggui/public/provider-icons/together-color.svgis excluded by!**/*.svggui/public/provider-icons/uncloseai.svgis excluded by!**/*.svggui/public/provider-icons/volcengine-color.svgis excluded by!**/*.svggui/public/provider-icons/zhipu-color.svgis excluded by!**/*.svg
📒 Files selected for processing (42)
gui/public/provider-icons/LICENSE.litellmgui/public/provider-icons/LICENSE.lobe-iconsgui/public/provider-icons/LICENSE.omniroutegui/public/provider-icons/LICENSE.simple-iconsgui/src/App.tsxgui/src/components/AddProviderModal.tsxgui/src/components/QuotaBars.tsxgui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-catalog/provider-presets.tsgui/src/components/providers/OAuthPanel.tsxgui/src/components/providers/ProviderCardList.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Models.tsxgui/src/pages/Providers.tsxgui/src/provider-icons.tsgui/src/styles.cssgui/src/styles/provider-catalog.cssgui/tests/provider-catalog.test.tsgui/tests/provider-discovery-race.test.tsxsrc/codex/catalog/provider-fetch.tssrc/combos/index.tssrc/combos/smart-routing.tssrc/oauth/index.tssrc/providers/derive.tssrc/providers/free-directory.tssrc/providers/registry.tssrc/server/management/combo-routes.tssrc/server/management/provider-routes.tstests/combo-management-api.test.tstests/google-models-listing.test.tstests/provider-connection-test.test.tstests/provider-directory-routing.test.tstests/provider-free-directory.test.tstests/provider-model-envelope.test.tstests/provider-registry-parity.test.tstests/provider-workspace-data.test.tstests/smart-routing.test.ts
|
|
||
| ## Statement of Purpose | ||
|
|
||
| The laws of most jurisdictions throughout the world automatically confer exclusive Copyright and Related Rights (defined below) upon the creator and subsequent owner(s) (each and all, an �owner�) of an original work of authorship and/or a database (each, a �Work�). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mojibake in the CC0 license text — replacement characters instead of quotes/apostrophes.
Lines 5, 9, 14, 20, 22, and 27 contain the Unicode replacement character � where the original CC0 1.0 text uses curly quotes/apostrophes (e.g. "an owner) of an original work of authorship" — note the corrupted glyph before "owner"). This indicates an encoding issue when the file was authored/committed, not an intentional plain-text substitution. Since this ships as a legal attribution notice, please re-save it from the official CC0 1.0 source text with correct UTF-8 encoding to avoid distributing a garbled license.
Also applies to: 9-9, 14-14, 20-20, 22-22, 27-27
🤖 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 `@gui/public/provider-icons/LICENSE.simple-icons` at line 5, Replace the
corrupted CC0 license text in LICENSE.simple-icons with the official CC0 1.0
source, preserving the complete notice and correcting all replacement characters
at the referenced lines. Save the file explicitly as valid UTF-8 so the original
quotation marks and apostrophes render correctly.
| <section className="provider-catalog-detail" aria-live="polite"> | ||
| {tier === "accounts" | ||
| ? <div className="provider-catalog-detail-empty"><strong>{t("modal.tab.accounts")}</strong><span>{t("modal.accountsHint")}</span></div> | ||
| : detail ?? <div className="provider-catalog-detail-empty"><strong>{t("modal.selectProvider")}</strong><span>{t("modal.selectProviderHint")}</span></div>} | ||
| </section> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
aria-live="polite" now wraps the whole interactive detail pane, not just a status message.
At Line 230 the live region encloses detail, which in AddProviderModal is the full provider form (name/base-URL/API-key fields, discovery list, action bar — AddProviderModal.tsx Lines 445-730). Any re-render inside that subtree — every keystroke that changes form, every checkbox toggle in provider-model-list — becomes a polite announcement, so screen-reader users get the entire pane re-read while typing. Live regions should wrap only the status text that changes asynchronously.
♿ Proposed fix: scope the live region to the empty-state/status text
- <section className="provider-catalog-detail" aria-live="polite">
+ <section className="provider-catalog-detail">
{tier === "accounts"
- ? <div className="provider-catalog-detail-empty"><strong>{t("modal.tab.accounts")}</strong><span>{t("modal.accountsHint")}</span></div>
- : detail ?? <div className="provider-catalog-detail-empty"><strong>{t("modal.selectProvider")}</strong><span>{t("modal.selectProviderHint")}</span></div>}
+ ? <div className="provider-catalog-detail-empty" aria-live="polite"><strong>{t("modal.tab.accounts")}</strong><span>{t("modal.accountsHint")}</span></div>
+ : detail ?? <div className="provider-catalog-detail-empty" aria-live="polite"><strong>{t("modal.selectProvider")}</strong><span>{t("modal.selectProviderHint")}</span></div>}
</section>📝 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.
| <section className="provider-catalog-detail" aria-live="polite"> | |
| {tier === "accounts" | |
| ? <div className="provider-catalog-detail-empty"><strong>{t("modal.tab.accounts")}</strong><span>{t("modal.accountsHint")}</span></div> | |
| : detail ?? <div className="provider-catalog-detail-empty"><strong>{t("modal.selectProvider")}</strong><span>{t("modal.selectProviderHint")}</span></div>} | |
| </section> | |
| <section className="provider-catalog-detail"> | |
| {tier === "accounts" | |
| ? <div className="provider-catalog-detail-empty" aria-live="polite"><strong>{t("modal.tab.accounts")}</strong><span>{t("modal.accountsHint")}</span></div> | |
| : detail ?? <div className="provider-catalog-detail-empty" aria-live="polite"><strong>{t("modal.selectProvider")}</strong><span>{t("modal.selectProviderHint")}</span></div>} | |
| </section> |
🤖 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 `@gui/src/components/provider-catalog/ProviderCatalog.tsx` around lines 230 -
234, Update the provider-catalog detail rendering in ProviderCatalog so
aria-live="polite" applies only to the empty-state or asynchronous status text,
not the detail subtree containing the interactive provider form. Keep the
accounts and select-provider messages announced while rendering detail outside
the live region.
| const keylessFree = provider?.keyOptional === true && !provider?.hasApiKey; | ||
| return ( | ||
| <div key={name} className="oauth-row"> | ||
| <span className="oauth-name" title={kind === "account" ? oauthLabel(name) : name}> | ||
| <span className="provider-icon provider-icon-sm">{icon && <img src={icon} alt="" aria-hidden="true" />}</span> | ||
| <span className="oauth-name-text">{name}</span> | ||
| </span> | ||
| <span className="oauth-status"> | ||
| <span className={`dot ${isBusy ? "dot-amber" : "dot-green"}`} /> | ||
| <span className="oauth-email muted"> | ||
| {kind === "account" ? (isBusy ? t("prov.waitingBrowser") : st.email ?? t("prov.loggedIn")) : keylessFree ? t("modal.badge.free") : t("prov.hasApiKey")} | ||
| </span> | ||
| </span> | ||
| <span className="oauth-actions"> | ||
| {kind === "account" && isForward && <a className="btn btn-ghost btn-sm" href="#codex-auth">{t("prov.manageCodexAccounts")}</a>} | ||
| {kind === "account" && !isForward && !isBusy && <button className="btn btn-ghost btn-sm" onClick={() => logoutOAuth(name)}>{t("prov.logout")}</button>} | ||
| {kind === "account" && isBusy && <button className="btn btn-ghost btn-sm" onClick={() => { void cancelLoginOAuth(name); }}>{t("common.cancel")}</button>} | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Key rows report a green "has API key" status even when the management API says hasApiKey is false — and the old "add the missing key" action is gone.
keylessFree at Line 55 is only true for keyOptional === true && !hasApiKey. Every other key provider falls into the t("prov.hasApiKey") branch at Line 65, paired with the green dot at Line 63, because provider.hasApiKey is never consulted for the non-keyless case. So a configured-but-unauthenticated provider (config.providers[name] = { keyOptional: false, hasApiKey: false }, exactly what /api/config returns for a provider added without a key) renders as connected-with-a-key. Line 68-72 also emits no action for kind === "key", so the previous per-row "set up API key" affordance is gone and the user has no in-place way to fix it — the section-level add button at Line 129 starts a brand-new provider instead.
As per path instructions, "GUI state changes stay consistent with the management API responses."
🐛 Proposed fix: derive the dot/status from `hasApiKey` and offer a remediation action
const isForward = provider?.authMode === "forward";
- const keylessFree = provider?.keyOptional === true && !provider?.hasApiKey;
+ const hasKey = provider?.hasApiKey === true;
+ const keylessFree = provider?.keyOptional === true && !hasKey;
+ const missingKey = kind === "key" && !hasKey && !keylessFree;
return (
<div key={name} className="oauth-row">
@@
<span className="oauth-status">
- <span className={`dot ${isBusy ? "dot-amber" : "dot-green"}`} />
+ <span className={`dot ${isBusy || missingKey ? "dot-amber" : "dot-green"}`} />
<span className="oauth-email muted">
- {kind === "account" ? (isBusy ? t("prov.waitingBrowser") : st.email ?? t("prov.loggedIn")) : keylessFree ? t("modal.badge.free") : t("prov.hasApiKey")}
+ {kind === "account"
+ ? (isBusy ? t("prov.waitingBrowser") : st.email ?? t("prov.loggedIn"))
+ : keylessFree ? t("modal.badge.free")
+ : missingKey ? t("prov.noApiKey")
+ : t("prov.hasApiKey")}
</span>
</span>
<span className="oauth-actions">
+ {missingKey && <button className="btn btn-primary btn-sm" type="button" onClick={() => onAddProvider({ tier: "paid" })}>{t("prov.keyAdd")}</button>}
{kind === "account" && isForward && <a className="btn btn-ghost btn-sm" href="`#codex-auth`">{t("prov.manageCodexAccounts")}</a>}t("prov.keyAdd") already exists (used in ProviderCardList.tsx Line 269); prov.noApiKey needs adding to all six locale files under gui/src/i18n/.
🤖 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 `@gui/src/components/providers/OAuthPanel.tsx` around lines 55 - 72, Update the
OAuthPanel key-provider row rendering to derive its dot color and status text
from provider.hasApiKey, showing a not-configured state when false instead of
always using prov.hasApiKey. Restore a kind === "key" remediation action using
the existing prov.keyAdd translation and the appropriate API/setup handler. Add
the prov.noApiKey translation to all supported locale resources, preserving
account-provider behavior.
Source: Path instructions
| const LABELS: Record<string, string> = { | ||
| agy: "AGY", aihorde: "AI Horde", "api-airforce": "API Airforce", "arcee-ai": "Arcee AI", | ||
| bazaarlink: "BazaarLink", blackbox: "Blackbox", bluesminds: "Bluesminds", "cloudflare-ai": "Cloudflare AI", | ||
| coze: "Coze", "duckduckgo-web": "DuckDuckGo Web", "felo-web": "Felo Web", friendliai: "FriendliAI", | ||
| gemini: "Google Gemini", "github-models": "GitHub Models", huggingchat: "HuggingChat", huggingface: "Hugging Face", | ||
| iflytek: "iFlytek", "inference-net": "Inference.net", llm7: "LLM7", "muse-spark-web": "Muse Spark Web", | ||
| nlpcloud: "NLP Cloud", "ollama-cloud": "Ollama Cloud", ovhcloud: "OVHcloud", "qwen-web": "Qwen Web", | ||
| "t3-web": "T3 Web", uncloseai: "UncloseAI", ainative: "AI Native", baidu: "Baidu Qianfan", | ||
| glm: "Z.AI GLM", "glm-cn": "BigModel GLM (CN)", "kilo-gateway": "Kilo Gateway", "opencode-zen": "OpenCode Zen", | ||
| sealion: "SEA-LION", bytez: "Bytez", "nous-research": "Nous Research", agentrouter: "AgentRouter", | ||
| ai21: "AI21", baichuan: "Baichuan", deepinfra: "DeepInfra", deepseek: "DeepSeek", doubao: "Doubao", | ||
| "freemodel-dev": "FreeModel.dev", nebius: "Nebius", novita: "Novita", nscale: "Nscale", nvidia: "NVIDIA NIM", | ||
| publicai: "PublicAI", qoder: "Qoder", sensenova: "SenseNova", stepfun: "StepFun", vertex: "Google Vertex AI", | ||
| }; | ||
|
|
||
| const referenceNote = "Reference entry only: no safe documented API integration is enabled. Configure it manually only with provider documentation; consumer-web cookies and anti-bot bypasses are intentionally unsupported."; | ||
|
|
||
| export const FREE_PROVIDER_DIRECTORY: readonly FreeDirectoryProvider[] = Object.values(FREE_PROVIDER_ACCESS_GROUPS) | ||
| .flat() | ||
| .filter((id, index, ids) => ids.indexOf(id) === index) | ||
| .map(id => { | ||
| const accessGroups = (Object.entries(FREE_PROVIDER_ACCESS_GROUPS) as [ProviderAccessGroup, readonly string[]][]) | ||
| .filter(([, ids]) => ids.includes(id)) | ||
| .map(([group]) => group); | ||
| const override = CONNECTABLE[id]; | ||
| return { | ||
| id, | ||
| label: LABELS[id] ?? id.split("-").map(word => word.charAt(0).toUpperCase() + word.slice(1)).join(" "), | ||
| adapter: "openai-chat", | ||
| baseUrl: "", | ||
| authKind: "key", | ||
| accessGroups, | ||
| supportLevel: "reference", | ||
| verification: "unverified", | ||
| discovery: "unsupported", | ||
| lastVerified: LAST_VERIFIED, | ||
| liveModels: false, | ||
| ...(!override ? { note: referenceNote } : {}), | ||
| ...override, | ||
| } satisfies FreeDirectoryProvider; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing LABELS entries produce mis-cased brand names for several prominent providers.
LABELS (lines 128-141) omits openrouter, siliconflow, opencode, monsterapi, deepinfra, deepseek, stepfun, and ai21. None of the corresponding CONNECTABLE overrides set a label (the openAi() helper never sets one), so these fall through to the auto-generated fallback at line 155, which only capitalizes the first letter of each hyphen-separated segment. The result: "Openrouter", "Siliconflow", "Opencode", "Monsterapi", "Deepinfra", "Deepseek", "Stepfun", "Ai21" — instead of the correct brand casing OpenRouter, SiliconFlow, OpenCode, MonsterAPI, DeepInfra, DeepSeek, StepFun, AI21. Note opencode-zen already has the correct "OpenCode Zen" label, making the mismatch with bare opencode particularly visible in the catalog.
🏷️ Proposed fix: add the missing entries to LABELS
const LABELS: Record<string, string> = {
- agy: "AGY", aihorde: "AI Horde", "api-airforce": "API Airforce", "arcee-ai": "Arcee AI",
+ agy: "AGY", aihorde: "AI Horde", "api-airforce": "API Airforce", "arcee-ai": "Arcee AI",
+ openrouter: "OpenRouter", siliconflow: "SiliconFlow", opencode: "OpenCode",
+ monsterapi: "MonsterAPI", deepinfra: "DeepInfra", deepseek: "DeepSeek",
+ stepfun: "StepFun", ai21: "AI21",
bazaarlink: "BazaarLink", blackbox: "Blackbox", bluesminds: "Bluesminds", "cloudflare-ai": "Cloudflare AI",📝 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.
| const LABELS: Record<string, string> = { | |
| agy: "AGY", aihorde: "AI Horde", "api-airforce": "API Airforce", "arcee-ai": "Arcee AI", | |
| bazaarlink: "BazaarLink", blackbox: "Blackbox", bluesminds: "Bluesminds", "cloudflare-ai": "Cloudflare AI", | |
| coze: "Coze", "duckduckgo-web": "DuckDuckGo Web", "felo-web": "Felo Web", friendliai: "FriendliAI", | |
| gemini: "Google Gemini", "github-models": "GitHub Models", huggingchat: "HuggingChat", huggingface: "Hugging Face", | |
| iflytek: "iFlytek", "inference-net": "Inference.net", llm7: "LLM7", "muse-spark-web": "Muse Spark Web", | |
| nlpcloud: "NLP Cloud", "ollama-cloud": "Ollama Cloud", ovhcloud: "OVHcloud", "qwen-web": "Qwen Web", | |
| "t3-web": "T3 Web", uncloseai: "UncloseAI", ainative: "AI Native", baidu: "Baidu Qianfan", | |
| glm: "Z.AI GLM", "glm-cn": "BigModel GLM (CN)", "kilo-gateway": "Kilo Gateway", "opencode-zen": "OpenCode Zen", | |
| sealion: "SEA-LION", bytez: "Bytez", "nous-research": "Nous Research", agentrouter: "AgentRouter", | |
| ai21: "AI21", baichuan: "Baichuan", deepinfra: "DeepInfra", deepseek: "DeepSeek", doubao: "Doubao", | |
| "freemodel-dev": "FreeModel.dev", nebius: "Nebius", novita: "Novita", nscale: "Nscale", nvidia: "NVIDIA NIM", | |
| publicai: "PublicAI", qoder: "Qoder", sensenova: "SenseNova", stepfun: "StepFun", vertex: "Google Vertex AI", | |
| }; | |
| const referenceNote = "Reference entry only: no safe documented API integration is enabled. Configure it manually only with provider documentation; consumer-web cookies and anti-bot bypasses are intentionally unsupported."; | |
| export const FREE_PROVIDER_DIRECTORY: readonly FreeDirectoryProvider[] = Object.values(FREE_PROVIDER_ACCESS_GROUPS) | |
| .flat() | |
| .filter((id, index, ids) => ids.indexOf(id) === index) | |
| .map(id => { | |
| const accessGroups = (Object.entries(FREE_PROVIDER_ACCESS_GROUPS) as [ProviderAccessGroup, readonly string[]][]) | |
| .filter(([, ids]) => ids.includes(id)) | |
| .map(([group]) => group); | |
| const override = CONNECTABLE[id]; | |
| return { | |
| id, | |
| label: LABELS[id] ?? id.split("-").map(word => word.charAt(0).toUpperCase() + word.slice(1)).join(" "), | |
| adapter: "openai-chat", | |
| baseUrl: "", | |
| authKind: "key", | |
| accessGroups, | |
| supportLevel: "reference", | |
| verification: "unverified", | |
| discovery: "unsupported", | |
| lastVerified: LAST_VERIFIED, | |
| liveModels: false, | |
| ...(!override ? { note: referenceNote } : {}), | |
| ...override, | |
| } satisfies FreeDirectoryProvider; | |
| }); | |
| const LABELS: Record<string, string> = { | |
| agy: "AGY", aihorde: "AI Horde", "api-airforce": "API Airforce", "arcee-ai": "Arcee AI", | |
| openrouter: "OpenRouter", siliconflow: "SiliconFlow", opencode: "OpenCode", | |
| monsterapi: "MonsterAPI", deepinfra: "DeepInfra", deepseek: "DeepSeek", | |
| stepfun: "StepFun", ai21: "AI21", | |
| bazaarlink: "BazaarLink", blackbox: "Blackbox", bluesminds: "Bluesminds", "cloudflare-ai": "Cloudflare AI", | |
| coze: "Coze", "duckduckgo-web": "DuckDuckGo Web", "felo-web": "Felo Web", friendliai: "FriendliAI", | |
| gemini: "Google Gemini", "github-models": "GitHub Models", huggingchat: "HuggingChat", huggingface: "Hugging Face", | |
| iflytek: "iFlytek", "inference-net": "Inference.net", llm7: "LLM7", "muse-spark-web": "Muse Spark Web", | |
| nlpcloud: "NLP Cloud", "ollama-cloud": "Ollama Cloud", ovhcloud: "OVHcloud", "qwen-web": "Qwen Web", | |
| "t3-web": "T3 Web", uncloseai: "UncloseAI", ainative: "AI Native", baidu: "Baidu Qianfan", | |
| glm: "Z.AI GLM", "glm-cn": "BigModel GLM (CN)", "kilo-gateway": "Kilo Gateway", "opencode-zen": "OpenCode Zen", | |
| sealion: "SEA-LION", bytez: "Bytez", "nous-research": "Nous Research", agentrouter: "AgentRouter", | |
| ai21: "AI21", baichuan: "Baichuan", deepinfra: "DeepInfra", deepseek: "DeepSeek", doubao: "Doubao", | |
| "freemodel-dev": "FreeModel.dev", nebius: "Nebius", novita: "Novita", nscale: "Nscale", nvidia: "NVIDIA NIM", | |
| publicai: "PublicAI", qoder: "Qoder", sensenova: "SenseNova", stepfun: "StepFun", vertex: "Google Vertex AI", | |
| }; | |
| const referenceNote = "Reference entry only: no safe documented API integration is enabled. Configure it manually only with provider documentation; consumer-web cookies and anti-bot bypasses are intentionally unsupported."; | |
| export const FREE_PROVIDER_DIRECTORY: readonly FreeDirectoryProvider[] = Object.values(FREE_PROVIDER_ACCESS_GROUPS) | |
| .flat() | |
| .filter((id, index, ids) => ids.indexOf(id) === index) | |
| .map(id => { | |
| const accessGroups = (Object.entries(FREE_PROVIDER_ACCESS_GROUPS) as [ProviderAccessGroup, readonly string[]][]) | |
| .filter(([, ids]) => ids.includes(id)) | |
| .map(([group]) => group); | |
| const override = CONNECTABLE[id]; | |
| return { | |
| id, | |
| label: LABELS[id] ?? id.split("-").map(word => word.charAt(0).toUpperCase() + word.slice(1)).join(" "), | |
| adapter: "openai-chat", | |
| baseUrl: "", | |
| authKind: "key", | |
| accessGroups, | |
| supportLevel: "reference", | |
| verification: "unverified", | |
| discovery: "unsupported", | |
| lastVerified: LAST_VERIFIED, | |
| liveModels: false, | |
| ...(!override ? { note: referenceNote } : {}), | |
| ...override, | |
| } satisfies FreeDirectoryProvider; | |
| }); |
🤖 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/providers/free-directory.ts` around lines 128 - 168, Add the missing
provider label mappings to LABELS for openrouter, siliconflow, opencode,
monsterapi, deepinfra, deepseek, stepfun, and ai21, using the required brand
casing: OpenRouter, SiliconFlow, OpenCode, MonsterAPI, DeepInfra, DeepSeek,
StepFun, and AI21. Leave the fallback label generation and
FREE_PROVIDER_DIRECTORY mapping unchanged.
| return jsonResponse({ | ||
| success: true, | ||
| mode, | ||
| id, | ||
| model: `combo/${id}`, | ||
| targets: combo.targets, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Use comboPublicModelId(id, combo) instead of hand-building combo/${id} for the response's model field.
Both sibling handlers in this file derive the public model string via the shared helper — GET /api/combos uses comboPublicModelId(id, combo) (Line 72) and PUT /api/combos uses newPublicModel = comboPublicModelId(id, normalized) (Line 159/224). This new smart-routing handler instead hardcodes model: \combo/${id}`. Today the two happen to produce the same value because buildSmartRoutingCombonever sets analias, but that's an implicit coincidence, not an enforced invariant — if comboPublicModelId's logic changes (alias support, versioned ids, etc.), this endpoint's modelfield would silently diverge from whatGET /api/combos` reports for the exact same combo, breaking a public API contract for any client relying on it.
♻️ Proposed fix
- const id = `auto-${mode}`;
- config.combos = { ...(config.combos ?? {}), [id]: combo };
- saveConfig(config);
- clearComboSelectionState(id);
- clearComboTargetCooldowns(id);
- await refreshCodexCatalogBestEffort();
- return jsonResponse({
- success: true,
- mode,
- id,
- model: `combo/${id}`,
- targets: combo.targets,
- });
+ const { comboPublicModelId } = await import("../../combos");
+ const id = `auto-${mode}`;
+ config.combos = { ...(config.combos ?? {}), [id]: combo };
+ saveConfig(config);
+ clearComboSelectionState(id);
+ clearComboTargetCooldowns(id);
+ await refreshCodexCatalogBestEffort();
+ return jsonResponse({
+ success: true,
+ mode,
+ id,
+ model: comboPublicModelId(id, combo),
+ targets: combo.targets,
+ });📝 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.
| return jsonResponse({ | |
| success: true, | |
| mode, | |
| id, | |
| model: `combo/${id}`, | |
| targets: combo.targets, | |
| }); | |
| const { comboPublicModelId } = await import("../../combos"); | |
| const id = `auto-${mode}`; | |
| config.combos = { ...(config.combos ?? {}), [id]: combo }; | |
| saveConfig(config); | |
| clearComboSelectionState(id); | |
| clearComboTargetCooldowns(id); | |
| await refreshCodexCatalogBestEffort(); | |
| return jsonResponse({ | |
| success: true, | |
| mode, | |
| id, | |
| model: comboPublicModelId(id, combo), | |
| targets: combo.targets, | |
| }); |
🤖 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/server/management/combo-routes.ts` around lines 102 - 108, Update the
smart-routing handler’s response construction to derive the model field with
comboPublicModelId(id, combo) instead of interpolating combo/${id}. Keep the
remaining response fields unchanged and ensure this endpoint uses the same
public model identifier as the sibling combo handlers.
| const allowsModelsEnvelope = (entry.adapter === "google" && (entry.googleMode ?? "ai-studio") === "ai-studio") | ||
| || new URL(request.url).pathname.replace(/\/+$/, "").endsWith("/api/tags"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Same registry-fallback gap as provider-fetch.ts (Line 418-419): allowsModelsEnvelope should use effectiveGoogleMode.
(entry.adapter === "google" && (entry.googleMode ?? "ai-studio") === "ai-studio") bypasses the registry-default fallback that buildModelsRequest (called two lines above via effectiveGoogleMode) already applies. See the consolidated comment for the shared fix.
🤖 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/server/management/provider-routes.ts` around lines 489 - 490, Update the
allowsModelsEnvelope calculation to use the already-resolved effectiveGoogleMode
from buildModelsRequest instead of directly evaluating entry.googleMode, while
preserving the existing Google adapter and /api/tags conditions.
| test("smart routing builds the selected mode even when cost pricing is unavailable", async () => { | ||
| await withTempHome(async () => { | ||
| const config = baseConfig({ | ||
| providers: { | ||
| a: { | ||
| adapter: "openai-chat", | ||
| baseUrl: "https://a.example/v1", | ||
| apiKey: "key", | ||
| liveModels: false, | ||
| models: ["unknown-reasoning-model"], | ||
| }, | ||
| b: { | ||
| adapter: "openai-chat", | ||
| baseUrl: "https://b.example/v1", | ||
| apiKey: "key", | ||
| liveModels: false, | ||
| models: ["unknown-agent-model"], | ||
| }, | ||
| }, | ||
| defaultProvider: "a", | ||
| combos: { manual: VALID_COMBO, "auto-balance": VALID_COMBO }, | ||
| }); | ||
| saveConfig(config); | ||
| let refreshes = 0; | ||
|
|
||
| const response = await comboApi(config, "POST", "/api/smart-routing", { mode: "intelligence" }, async () => { refreshes += 1; }); | ||
| const body = await responseJson(response) as { id?: string }; | ||
|
|
||
| expect(response?.status).toBe(200); | ||
| expect(body.id).toBe("auto-intelligence"); | ||
| expect(Object.keys(config.combos ?? {}).sort()).toEqual(["auto-balance", "auto-intelligence", "manual"]); | ||
| expect(config.combos?.["auto-intelligence"]?.strategy).toBe("round-robin"); | ||
| expect(config.combos?.["auto-balance"]).toEqual(VALID_COMBO); | ||
| expect(refreshes).toBe(1); | ||
| expect(JSON.parse(readFileSync(getConfigPath(), "utf8")).combos["auto-intelligence"]).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| test("smart routing rejects an invalid mode without changing config", async () => { | ||
| await withTempHome(async () => { | ||
| const config = baseConfig(); | ||
| saveConfig(config); | ||
| const before = readFileSync(getConfigPath(), "utf8"); | ||
|
|
||
| const response = await comboApi(config, "POST", "/api/smart-routing", { mode: "fastest" }); | ||
| const body = await responseJson(response); | ||
|
|
||
| expect(response?.status).toBe(400); | ||
| expect(body.error).toBe("mode must be one of: intelligence, balance, cost"); | ||
| expect(readFileSync(getConfigPath(), "utf8")).toBe(before); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression test for re-applying the same smart-routing mode (the "refresh" half of the feature).
The PR objective explicitly describes Smart Routing as creating "or refresh[ing]" automatic combos, but these tests only cover the "create" path (Lines 156-192) and the invalid-mode rejection (194-207). Neither test calls /api/smart-routing twice with the same mode to confirm the existing auto-<mode> entry gets replaced in place (no duplicate key, targets actually updated, saveConfig still called correctly) rather than left stale or duplicated.
Suggest adding a test that: (1) applies intelligence mode once, (2) mutates the enabled-models set, (3) re-applies intelligence mode, and (4) asserts config.combos["auto-intelligence"] reflects the new targets and Object.keys(config.combos) still has no duplicate/stale entry.
As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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 `@tests/combo-management-api.test.ts` around lines 156 - 207, Add a regression
test alongside the existing smart-routing tests that applies the “intelligence”
mode, changes the enabled-model set, then applies the same mode again. Assert
the existing “auto-intelligence” combo is refreshed with the new targets,
remains a single key without stale or duplicate entries, and configuration
persistence continues to work.
Source: Path instructions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/pages/Providers.tsx (1)
68-68: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the stale
toggleWorkspacereference.After moving view-mode ownership to
App.tsx,gui/src/pages/Providers.tsxno longer definestoggleWorkspace, but the workspace header still calls it. This produces a TypeScript error (Cannot find name 'toggleWorkspace') and blocks the GUI build.Remove this obsolete header toggle, or pass
toggleGlobalWorkspacefromApp.tsxand use that callback consistently.Also applies to: 574-575
🤖 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 `@gui/src/pages/Providers.tsx` at line 68, Remove the obsolete toggleWorkspace usage from the workspace header in Providers, or replace it by consistently wiring and invoking the toggleGlobalWorkspace callback supplied by App. Ensure Providers no longer references the undefined toggleWorkspace symbol so the TypeScript build succeeds.
🤖 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.
Outside diff comments:
In `@gui/src/pages/Providers.tsx`:
- Line 68: Remove the obsolete toggleWorkspace usage from the workspace header
in Providers, or replace it by consistently wiring and invoking the
toggleGlobalWorkspace callback supplied by App. Ensure Providers no longer
references the undefined toggleWorkspace symbol so the TypeScript build
succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc771d00-a120-4a6b-8e3d-0404dbc4a449
📒 Files selected for processing (9)
gui/src/App.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Providers.tsxgui/src/styles.css
e9c31f5 to
7daaee4
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/management/provider-routes.ts (1)
295-308: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConnectivity test (
/api/providers/test) doesn't use the newparseProviderModelsApiItems, so it can misreport a working provider as failing.
list = json && typeof json === "object" && !Array.isArray(json) ? (...) : undefinedexplicitly excludes top-level array responses (!Array.isArray(json)), so a provider whose/modelsendpoint returns a bare array (e.g.[{ "id": "..." }, ...]) always falls into!Array.isArray(list)→"upstream /models returned an unexpected shape", even though this exact shape is now correctly normalized byparseProviderModelsApiItems(used two lines below at 491, and explicitly tested viaparseProviderModelsApiItems([{ id: "accounts/fireworks/models/llama" }])intests/provider-model-envelope.test.ts). Users would see a false "connection failed" for a provider that actually works.♻️ Suggested fix: reuse the shared parser here too
- const json = await res.json().catch(() => null) as { data?: unknown; models?: unknown } | null; - // OpenAI-style lists use { data: [...] }; Google's /v1beta/models (the other shape - // buildModelsRequest can produce) returns { models: [...] }. - const list = json && typeof json === "object" && !Array.isArray(json) - ? (Array.isArray(json.data) ? json.data : Array.isArray(json.models) ? json.models : undefined) - : undefined; - if (!Array.isArray(list)) { + const json = await res.json().catch(() => null) as unknown; + const allowsModelsEnvelope = effectiveGoogleMode(name, prov) === "ai-studio" + || new URL(modelsUrl).pathname.replace(/\/+$/, "").endsWith("/api/tags"); + const list = parseProviderModelsApiItems(json, allowsModelsEnvelope); + if (!list) { return jsonResponse({ ok: false, latencyMs, error: "upstream /models returned an unexpected shape" }); }(requires importing
effectiveGoogleModefrom../../providers/registry, already imported for other purposes in this file's peers)🤖 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/server/management/provider-routes.ts` around lines 295 - 308, Update the `/api/providers/test` handler to reuse `parseProviderModelsApiItems` for normalizing the `/models` response, including top-level array responses, instead of manually extracting only `data` or `models` in the local `list` logic. Add the required `effectiveGoogleMode` import and pass the appropriate provider context so the connectivity test matches the parsing behavior used by the existing model request flow.
♻️ Duplicate comments (7)
gui/src/pages/Models.tsx (1)
143-165: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
response.json()/comboResponse.json()are still unguarded, unlike every other handler in this file.
addCustomModel(Line 513),updateCustomModel(Line 538),setMultiAgentMode(Line 407) andputV2Threads(Line 442) all use.catch(() => null). Here a non-JSON body — including on a successful 200 — throws aSyntaxErrorinto the outer catch, so Line 160 shows the user a raw parser message and reports failure even though the server-side mutation already committed.🐛 Proposed fix
- const payload = await response.json() as { error?: string }; - if (!response.ok) throw new Error(payload.error || String(response.status)); - const comboResponse = await fetch(`${apiBase}/api/combos`); - if (!comboResponse.ok) throw new Error(String(comboResponse.status)); - setCombos(parseComboList(await comboResponse.json())); + const payload = await response.json().catch(() => null) as { error?: string } | null; + if (!response.ok) throw new Error(payload?.error || String(response.status)); + const comboResponse = await fetch(`${apiBase}/api/combos`); + if (!comboResponse.ok) throw new Error(String(comboResponse.status)); + setCombos(parseComboList(await comboResponse.json().catch(() => null)));🤖 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 `@gui/src/pages/Models.tsx` around lines 143 - 165, Update applySmartRouting to safely parse both response.json() and comboResponse.json(), matching the guarded parsing used by addCustomModel, updateCustomModel, setMultiAgentMode, and putV2Threads. Treat non-JSON response bodies as null and preserve the successful mutation flow without exposing raw JSON parser errors or falsely reporting failure.src/server/management/combo-routes.ts (1)
102-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDerive
modelviacomboPublicModelId(id, combo)instead of hardcodingcombo/${id}.Both sibling handlers use the shared helper (Line 72 for GET, Line 159 for PUT). The values coincide today only because
buildSmartRoutingCombonever setsalias; if alias/id formatting logic evolves, this endpoint'smodelwould silently diverge from whatGET /api/combosreports for the same combo.🤖 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/server/management/combo-routes.ts` around lines 102 - 108, Update the response construction in the affected combo handler to set model using the shared comboPublicModelId(id, combo) helper instead of hardcoding combo/${id}. Keep the remaining response fields unchanged and align this handler with the sibling GET and PUT handlers.tests/combo-management-api.test.ts (1)
156-192: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThe "refresh an existing combo" half of Smart Routing is still untested.
Both new tests exercise creation (Lines 156-192) and invalid-mode rejection (Lines 194-207); neither POSTs the same
modetwice. Add a test that appliesintelligence, changes the enabled-model set, re-appliesintelligence, then assertsconfig.combos["auto-intelligence"]holds the new targets andObject.keys(config.combos)gained no stale/duplicate entry.As per path instructions for
tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."🤖 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 `@tests/combo-management-api.test.ts` around lines 156 - 192, Add a focused test beside the existing smart-routing tests that POSTs mode “intelligence” once, changes the enabled model set, then POSTs the same mode again. Assert that config.combos["auto-intelligence"] contains the updated targets and that the combo keys contain no stale or duplicate entry, while preserving the existing creation and invalid-mode coverage.Source: Path instructions
gui/src/components/AddProviderModal.tsx (1)
254-258: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
form.defaultModelcan still be submitted outside theselectedModelssubset.Line 255 selects all discovered ids, but Line 258 only rewrites
defaultModelwhenfreeOnlyis set or the field is empty. A default seeded inchoosePreset(Line 184) that is absent from the freshly discovered list survives, and Lines 291-300 then persistselectedModelswithout it — the provider's default route points at a model the proxy was told not to expose. Reconcile the default against the discovered ids, not just emptiness.🤖 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 `@gui/src/components/AddProviderModal.tsx` around lines 254 - 258, Update the defaultModel assignment in the discovery success flow near setSelectedModels so it is retained only when it exists in the freshly discovered models; otherwise replace it with an appropriate discovered model id (or the existing empty/default behavior). Ensure the submitted defaultModel always belongs to the selectedModels set, including defaults seeded by choosePreset.src/combos/smart-routing.ts (1)
137-142: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEligibility filtering still ignores each provider's
selectedModelsallow-list.Lines 137-141 check
disabled,disabledModels, and context window, but neverconfig.providers[...].selectedModels. The invariant only holds because the single caller (src/server/management/combo-routes.tsLine 88) pre-filters viafilterCatalogVisibleModels. Either document that precondition here or re-check the allow-list inside the function so a future call site cannot bypass a user's explicit model restriction.As per path instructions for
src/**: "Flag Node-only APIs that break under Bun, provider/adapter contract drift, and changes that bypass the shared routing/config layers."🤖 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/combos/smart-routing.ts` around lines 137 - 142, Update the eligibility filtering in the smart-routing function around the eligible model pipeline to enforce each provider’s selectedModels allow-list using the existing shared model-visibility/config logic, alongside the disabled and disabledModels checks. Ensure models excluded by config.providers[model.provider].selectedModels cannot be routed even when callers do not pre-filter through filterCatalogVisibleModels.Source: Path instructions
gui/tests/provider-discovery-race.test.tsx (1)
79-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe request-body assertion still lives inside the
fetchstub (Lines 82-87), where its diagnostics are lost.
discoverModelsingui/src/components/AddProviderModal.tsx(Lines 237-261) wraps thefetchcall intry/catch, so a thrownexpecthere is first converted intosetDiscoveryError(cause.message). The test then fails later and opaquely at Line 137 (discoveryResolvers[0]!isundefined, because the throw at Line 82 happens before Line 88 pushes the resolver) instead of reporting the actual body mismatch. Record the parsed bodies in a collector and assert them from the test body after each click, as previously suggested.🤖 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 `@gui/tests/provider-discovery-race.test.tsx` around lines 79 - 93, Move the request-body validation out of the fetch stub: collect each parsed discovery request body in a dedicated array, let the stub push the body before registering its deferred resolver, and assert the expected body from the test flow after each click. Preserve the existing requestIndex-dependent baseUrl expectations while ensuring body mismatches are reported directly by the test rather than through discoverModels’ error handling.src/providers/free-directory.ts (1)
128-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
LABELSfix is only partial — several brand names still mis-cased.The previously flagged gap is fixed for
ai21,deepinfra,deepseek, andstepfun, butopenrouter,siliconflow,opencode, andmonsterapiare still missing fromLABELS. None of theirCONNECTABLEoverrides (all built viaopenAi()) set an explicitlabel, so they still fall through to the auto-capitalize fallback at line 155, producing"Openrouter","Siliconflow","Opencode","Monsterapi"instead ofOpenRouter,SiliconFlow,OpenCode,MonsterAPI.Additionally, two more ids with no hyphen in their id can't be fixed by the fallback splitting logic at all, since it only capitalizes after
-:hackclub→"Hackclub"(should be "Hack Club") andsambanova→"Sambanova"(should be "SambaNova").🏷️ Proposed fix: complete the LABELS map
const LABELS: Record<string, string> = { agy: "AGY", aihorde: "AI Horde", "api-airforce": "API Airforce", "arcee-ai": "Arcee AI", + openrouter: "OpenRouter", siliconflow: "SiliconFlow", opencode: "OpenCode", monsterapi: "MonsterAPI", + hackclub: "Hack Club", sambanova: "SambaNova", bazaarlink: "BazaarLink", blackbox: "Blackbox", bluesminds: "Bluesminds", "cloudflare-ai": "Cloudflare AI",🤖 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/providers/free-directory.ts` around lines 128 - 141, Update the LABELS map to add explicit display names for openrouter, siliconflow, opencode, monsterapi, hackclub, and sambanova: OpenRouter, SiliconFlow, OpenCode, MonsterAPI, Hack Club, and SambaNova. Leave the existing CONNECTABLE definitions and fallback label logic unchanged.
🤖 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 `@gui/src/components/AddProviderModal.tsx`:
- Around line 694-705: Update the model-selection onChange handler to stop
assigning model.id to defaultModel when checking an additional model. Preserve
the existing defaultModel while adding selections, and only repair it when the
removed model was the current default or when the existing default is empty,
using the next selected model as the fallback.
In `@gui/src/components/providers/OAuthPanel.tsx`:
- Around line 115-142: Add explicit aria-label values to the add-provider
buttons in OAuthPanel, including the accounts, API-keys, and custom-provider
buttons. Build each label using the existing prov.add translation (or a
dedicated action-specific translation) together with the relevant provider type,
while leaving the visible button text unchanged.
In `@gui/src/components/QuotaBars.tsx`:
- Around line 160-173: Update the `title` assignment on the `quota-summary-item`
span in the `rows.map` renderer so warning rows also receive a textual hint,
reusing the existing `t("quota.limitReached")` translation as suggested while
preserving the exhausted-state title behavior.
In `@gui/src/styles/provider-catalog.css`:
- Around line 88-92: Add the -webkit-backdrop-filter declaration alongside
backdrop-filter in .provider-detail-actions, preserving the existing blur(12px)
value and keeping the sticky action bar’s other styles unchanged.
In `@gui/tests/provider-catalog.test.ts`:
- Around line 25-35: Replace hard-coded catalog counts and test-title totals in
the provider catalog tests with invariants: curated entries must have access
groups, IDs must be unique, and the summed group counts must cover curated
entries while explicitly preserving multi-group membership for glm-cn. Replace
source-file reads and regex/string assertions with behavioral tests using
rendered components and DOM queries, following the ProviderCardList/container
pattern in provider-discovery-race.test.tsx; verify visible fallback and layout
behavior without depending on formatting or expression spelling.
- Around line 87-111: Scope both media-query assertions in “provider cards keep
content and resource rows aligned” and “classic provider workspace exposes
connection intents and a compact provider grid” to the matched `@media` block
body, preventing the regex from crossing a closing brace or matching a later
media block. Preserve the existing selectors and expected mobile layout while
anchoring the match to the 760px rule.
In `@src/combos/smart-routing.ts`:
- Around line 11-14: Replace the GUI-side SmartRoutingMode declaration and
hardcoded mode list in Models.tsx with the runtime mode list sourced from the
existing config/bootstrap payload. Expose SMART_ROUTING_MODES through that
payload, update the GUI to derive its types and rendering/validation from the
received list, and preserve the current behavior for the existing modes.
In `@src/providers/registry.ts`:
- Around line 1027-1051: The shared-provider merge in PROVIDER_REGISTRY needs an
explicit base-wins invariant for transport fields. Add a small development-time
equality check for adapter, baseUrl, and authKind between matching
BASE_PROVIDER_REGISTRY and FREE_PROVIDER_DIRECTORY entries, or document the
invariant directly beside the merge if that is the project convention; keep
metadata overlay behavior unchanged.
In `@tests/provider-workspace-data.test.ts`:
- Around line 483-487: Update the “every free provider catalog entry has a local
icon” test to collect provider IDs whose providerIconSrc result fails the
expected path pattern, then assert the collected offenders are empty so a
failure reports all missing-icon provider IDs instead of stopping at the first
iteration.
---
Outside diff comments:
In `@src/server/management/provider-routes.ts`:
- Around line 295-308: Update the `/api/providers/test` handler to reuse
`parseProviderModelsApiItems` for normalizing the `/models` response, including
top-level array responses, instead of manually extracting only `data` or
`models` in the local `list` logic. Add the required `effectiveGoogleMode`
import and pass the appropriate provider context so the connectivity test
matches the parsing behavior used by the existing model request flow.
---
Duplicate comments:
In `@gui/src/components/AddProviderModal.tsx`:
- Around line 254-258: Update the defaultModel assignment in the discovery
success flow near setSelectedModels so it is retained only when it exists in the
freshly discovered models; otherwise replace it with an appropriate discovered
model id (or the existing empty/default behavior). Ensure the submitted
defaultModel always belongs to the selectedModels set, including defaults seeded
by choosePreset.
In `@gui/src/pages/Models.tsx`:
- Around line 143-165: Update applySmartRouting to safely parse both
response.json() and comboResponse.json(), matching the guarded parsing used by
addCustomModel, updateCustomModel, setMultiAgentMode, and putV2Threads. Treat
non-JSON response bodies as null and preserve the successful mutation flow
without exposing raw JSON parser errors or falsely reporting failure.
In `@gui/tests/provider-discovery-race.test.tsx`:
- Around line 79-93: Move the request-body validation out of the fetch stub:
collect each parsed discovery request body in a dedicated array, let the stub
push the body before registering its deferred resolver, and assert the expected
body from the test flow after each click. Preserve the existing
requestIndex-dependent baseUrl expectations while ensuring body mismatches are
reported directly by the test rather than through discoverModels’ error
handling.
In `@src/combos/smart-routing.ts`:
- Around line 137-142: Update the eligibility filtering in the smart-routing
function around the eligible model pipeline to enforce each provider’s
selectedModels allow-list using the existing shared model-visibility/config
logic, alongside the disabled and disabledModels checks. Ensure models excluded
by config.providers[model.provider].selectedModels cannot be routed even when
callers do not pre-filter through filterCatalogVisibleModels.
In `@src/providers/free-directory.ts`:
- Around line 128-141: Update the LABELS map to add explicit display names for
openrouter, siliconflow, opencode, monsterapi, hackclub, and sambanova:
OpenRouter, SiliconFlow, OpenCode, MonsterAPI, Hack Club, and SambaNova. Leave
the existing CONNECTABLE definitions and fallback label logic unchanged.
In `@src/server/management/combo-routes.ts`:
- Around line 102-108: Update the response construction in the affected combo
handler to set model using the shared comboPublicModelId(id, combo) helper
instead of hardcoding combo/${id}. Keep the remaining response fields unchanged
and align this handler with the sibling GET and PUT handlers.
In `@tests/combo-management-api.test.ts`:
- Around line 156-192: Add a focused test beside the existing smart-routing
tests that POSTs mode “intelligence” once, changes the enabled model set, then
POSTs the same mode again. Assert that config.combos["auto-intelligence"]
contains the updated targets and that the combo keys contain no stale or
duplicate entry, while preserving the existing creation and invalid-mode
coverage.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 321b4d58-1873-4664-beb8-176952c497b3
⛔ Files ignored due to path filters (54)
gui/public/provider-icons/agentrouter.pngis excluded by!**/*.pnggui/public/provider-icons/agnesai.svgis excluded by!**/*.svggui/public/provider-icons/ai21-color.svgis excluded by!**/*.svggui/public/provider-icons/aionlabs-color.svgis excluded by!**/*.svggui/public/provider-icons/api-airforce.svgis excluded by!**/*.svggui/public/provider-icons/arcee-color.svgis excluded by!**/*.svggui/public/provider-icons/baichuan-color.svgis excluded by!**/*.svggui/public/provider-icons/bazaarlink.svgis excluded by!**/*.svggui/public/provider-icons/blackbox.pngis excluded by!**/*.pnggui/public/provider-icons/bluesminds.svgis excluded by!**/*.svggui/public/provider-icons/cerebras-color.svgis excluded by!**/*.svggui/public/provider-icons/cohere-color.svgis excluded by!**/*.svggui/public/provider-icons/coze.svgis excluded by!**/*.svggui/public/provider-icons/deepinfra-color.svgis excluded by!**/*.svggui/public/provider-icons/duckduckgo.svgis excluded by!**/*.svggui/public/provider-icons/freemodel-dev.svgis excluded by!**/*.svggui/public/provider-icons/friendli.svgis excluded by!**/*.svggui/public/provider-icons/hackclub.svgis excluded by!**/*.svggui/public/provider-icons/hyperbolic-color.svgis excluded by!**/*.svggui/public/provider-icons/iflytek-color.svgis excluded by!**/*.svggui/public/provider-icons/inference.svgis excluded by!**/*.svggui/public/provider-icons/kilocode.svgis excluded by!**/*.svggui/public/provider-icons/liquid.svgis excluded by!**/*.svggui/public/provider-icons/litellm.pngis excluded by!**/*.pnggui/public/provider-icons/llm7.svgis excluded by!**/*.svggui/public/provider-icons/longcat-color.svgis excluded by!**/*.svggui/public/provider-icons/monsterapi.svgis excluded by!**/*.svggui/public/provider-icons/morph-color.svgis excluded by!**/*.svggui/public/provider-icons/nebius.svgis excluded by!**/*.svggui/public/provider-icons/nlpcloud.svgis excluded by!**/*.svggui/public/provider-icons/nousresearch.svgis excluded by!**/*.svggui/public/provider-icons/novita-color.svgis excluded by!**/*.svggui/public/provider-icons/nscale.pngis excluded by!**/*.pnggui/public/provider-icons/ovh.svgis excluded by!**/*.svggui/public/provider-icons/pollinations.svgis excluded by!**/*.svggui/public/provider-icons/predibase.pngis excluded by!**/*.pnggui/public/provider-icons/provider-generic.svgis excluded by!**/*.svggui/public/provider-icons/publicai.svgis excluded by!**/*.svggui/public/provider-icons/puter.svgis excluded by!**/*.svggui/public/provider-icons/qoder-color.svgis excluded by!**/*.svggui/public/provider-icons/reka.pngis excluded by!**/*.pnggui/public/provider-icons/requesty.svgis excluded by!**/*.svggui/public/provider-icons/sambanova-color.svgis excluded by!**/*.svggui/public/provider-icons/scaleway.svgis excluded by!**/*.svggui/public/provider-icons/sensenova-color.svgis excluded by!**/*.svggui/public/provider-icons/siliconcloud-color.svgis excluded by!**/*.svggui/public/provider-icons/spark-color.svgis excluded by!**/*.svggui/public/provider-icons/stepfun-color.svgis excluded by!**/*.svggui/public/provider-icons/t3-web.svgis excluded by!**/*.svggui/public/provider-icons/tencent-color.svgis excluded by!**/*.svggui/public/provider-icons/together-color.svgis excluded by!**/*.svggui/public/provider-icons/uncloseai.svgis excluded by!**/*.svggui/public/provider-icons/volcengine-color.svgis excluded by!**/*.svggui/public/provider-icons/zhipu-color.svgis excluded by!**/*.svg
📒 Files selected for processing (42)
gui/public/provider-icons/LICENSE.litellmgui/public/provider-icons/LICENSE.lobe-iconsgui/public/provider-icons/LICENSE.omniroutegui/public/provider-icons/LICENSE.simple-iconsgui/src/App.tsxgui/src/components/AddProviderModal.tsxgui/src/components/QuotaBars.tsxgui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-catalog/provider-presets.tsgui/src/components/providers/OAuthPanel.tsxgui/src/components/providers/ProviderCardList.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Models.tsxgui/src/pages/Providers.tsxgui/src/provider-icons.tsgui/src/styles.cssgui/src/styles/provider-catalog.cssgui/tests/provider-catalog.test.tsgui/tests/provider-discovery-race.test.tsxsrc/codex/catalog/provider-fetch.tssrc/combos/index.tssrc/combos/smart-routing.tssrc/oauth/index.tssrc/providers/derive.tssrc/providers/free-directory.tssrc/providers/registry.tssrc/server/management/combo-routes.tssrc/server/management/provider-routes.tstests/combo-management-api.test.tstests/google-models-listing.test.tstests/provider-connection-test.test.tstests/provider-directory-routing.test.tstests/provider-free-directory.test.tstests/provider-model-envelope.test.tstests/provider-registry-parity.test.tstests/provider-workspace-data.test.tstests/smart-routing.test.ts
| onChange={() => { | ||
| const nextSelected = checked | ||
| ? selectedModels.filter(id => id !== model.id) | ||
| : [...selectedModels, model.id]; | ||
| setSelectedModels(nextSelected); | ||
| setForm(current => current ? { | ||
| ...current, | ||
| defaultModel: checked && current.defaultModel === model.id | ||
| ? (nextSelected[0] ?? "") | ||
| : !checked ? model.id : current.defaultModel, | ||
| } : current); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Checking any extra model silently overwrites the user's defaultModel.
In the onChange at Lines 694-705, the !checked ? model.id branch means every additional model the user ticks becomes the new form.defaultModel, clobbering whatever they typed in the Default model field (Line 658) or what discovery selected. Only the un-check path needs to repair the default; ticking a model should leave an existing default alone.
🐛 Proposed fix: only repair the default when it is removed or empty
setForm(current => current ? {
...current,
- defaultModel: checked && current.defaultModel === model.id
- ? (nextSelected[0] ?? "")
- : !checked ? model.id : current.defaultModel,
+ defaultModel: checked && current.defaultModel === model.id
+ ? (nextSelected[0] ?? "")
+ : current.defaultModel || model.id,
} : current);📝 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.
| onChange={() => { | |
| const nextSelected = checked | |
| ? selectedModels.filter(id => id !== model.id) | |
| : [...selectedModels, model.id]; | |
| setSelectedModels(nextSelected); | |
| setForm(current => current ? { | |
| ...current, | |
| defaultModel: checked && current.defaultModel === model.id | |
| ? (nextSelected[0] ?? "") | |
| : !checked ? model.id : current.defaultModel, | |
| } : current); | |
| }} | |
| onChange={() => { | |
| const nextSelected = checked | |
| ? selectedModels.filter(id => id !== model.id) | |
| : [...selectedModels, model.id]; | |
| setSelectedModels(nextSelected); | |
| setForm(current => current ? { | |
| ...current, | |
| defaultModel: checked && current.defaultModel === model.id | |
| ? (nextSelected[0] ?? "") | |
| : current.defaultModel || model.id, | |
| } : current); | |
| }} |
🤖 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 `@gui/src/components/AddProviderModal.tsx` around lines 694 - 705, Update the
model-selection onChange handler to stop assigning model.id to defaultModel when
checking an additional model. Preserve the existing defaultModel while adding
selections, and only repair it when the removed model was the current default or
when the existing default is empty, using the next selected model as the
fallback.
| <section className="provider-connection-group"> | ||
| <header> | ||
| <span>{t("pws.tab.accounts")}</span> | ||
| <button className="btn btn-ghost btn-sm provider-connection-add" type="button" onClick={() => onAddProvider({ tier: "accounts" })}> | ||
| <IconPlus />{t("pws.tab.accounts")} | ||
| </button> | ||
| </header> | ||
| {connectedAccounts.length > 0 ? ( | ||
| <div className="oauth-grid">{connectedAccounts.map(name => renderProviderRow(name, "account"))}</div> | ||
| ) : <span className="muted text-label">{t("prov.noConnections")}</span>} | ||
| </section> | ||
| <section className="provider-connection-group"> | ||
| <header> | ||
| <span>{t("pws.apiKeys")}</span> | ||
| <button className="btn btn-ghost btn-sm provider-connection-add" type="button" onClick={() => onAddProvider({ tier: "paid" })}> | ||
| <IconPlus />{t("pws.apiKeys")} | ||
| </button> | ||
| </header> | ||
| {keyProviders.length > 0 ? ( | ||
| <div className="oauth-grid">{keyProviders.map(name => renderProviderRow(name, "key"))}</div> | ||
| ) : <span className="muted text-label">{t("prov.noConnections")}</span>} | ||
| </section> | ||
| <section className="provider-connection-group provider-connection-group--custom"> | ||
| <header><span>{t("modal.customProvider")}</span></header> | ||
| <button className="btn btn-ghost provider-custom-add" type="button" onClick={() => onAddProvider({ custom: true })}> | ||
| <IconPlus />{t("modal.customProvider")} | ||
| </button> | ||
| </section> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The three add buttons reuse their section-heading keys, so their accessible name is just the section title.
At Lines 118-120 the header text and the button text are both t("pws.tab.accounts"), giving the visual pattern "Accounts … + Accounts"; same for t("pws.apiKeys") (Lines 128-131) and t("modal.customProvider") (Lines 138-141). IconPlus is a bare <svg> with no title (gui/src/icons.tsx Line 23), so a screen reader announces these buttons as "Accounts", "API keys", "Custom provider" — indistinguishable from the headings and with no hint that they open the add-provider flow. Add an aria-label built from the existing t("prov.add") string (or a dedicated key) so the action is unambiguous.
♿ Proposed fix: give each add button an explicit action label
- <button className="btn btn-ghost btn-sm provider-connection-add" type="button" onClick={() => onAddProvider({ tier: "accounts" })}>
+ <button className="btn btn-ghost btn-sm provider-connection-add" type="button"
+ aria-label={`${t("prov.add")}: ${t("pws.tab.accounts")}`}
+ onClick={() => onAddProvider({ tier: "accounts" })}>
<IconPlus />{t("pws.tab.accounts")}
</button>Apply the same treatment to the API-keys button (Line 129) and the custom-provider button (Line 139).
🤖 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 `@gui/src/components/providers/OAuthPanel.tsx` around lines 115 - 142, Add
explicit aria-label values to the add-provider buttons in OAuthPanel, including
the accounts, API-keys, and custom-provider buttons. Build each label using the
existing prov.add translation (or a dedicated action-specific translation)
together with the relevant provider type, while leaving the visible button text
unchanged.
| {rows.map((row, index) => { | ||
| const warn = isQuotaWarn(row.percent, threshold); | ||
| const exhausted = isQuotaExhausted(row.percent); | ||
| return ( | ||
| <span | ||
| key={`${row.label}-${index}`} | ||
| className={`quota-summary-item${warn ? " quota-summary-item--warn" : ""}`} | ||
| title={exhausted ? t("quota.limitReached") : undefined} | ||
| > | ||
| <span>{row.label}</span> | ||
| <strong>{Math.round(row.percent)}%</strong> | ||
| {row.resetAt && <span className="quota-summary-reset">{formatResetFuture(row.resetAt, t, locale)}</span>} | ||
| {exhausted && <span className="quota-summary-state">{t("quota.limitReached")}</span>} | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The warn state is communicated by color alone in this layout.
Line 166 adds quota-summary-item--warn, whose only effect is color: var(--amber) on the nested <strong> (gui/src/styles.css Line 1035). Unlike the compact layout — where the bar fill also encodes the level — and unlike exhausted (Line 172 renders actual text), a user who cannot distinguish the amber percentage gets no signal that the quota is near its threshold (WCAG 1.4.1, use of color). The cheapest fix is to reuse the title you already compute for the exhausted case.
♿ Proposed fix: attach a textual hint for the warn state too
<span
key={`${row.label}-${index}`}
className={`quota-summary-item${warn ? " quota-summary-item--warn" : ""}`}
- title={exhausted ? t("quota.limitReached") : undefined}
+ title={exhausted ? t("quota.limitReached") : warn ? row.limitLabel : undefined}
>If a dedicated "near limit" string is preferable, add one locale key and use it here across all six files in gui/src/i18n/.
📝 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.
| {rows.map((row, index) => { | |
| const warn = isQuotaWarn(row.percent, threshold); | |
| const exhausted = isQuotaExhausted(row.percent); | |
| return ( | |
| <span | |
| key={`${row.label}-${index}`} | |
| className={`quota-summary-item${warn ? " quota-summary-item--warn" : ""}`} | |
| title={exhausted ? t("quota.limitReached") : undefined} | |
| > | |
| <span>{row.label}</span> | |
| <strong>{Math.round(row.percent)}%</strong> | |
| {row.resetAt && <span className="quota-summary-reset">{formatResetFuture(row.resetAt, t, locale)}</span>} | |
| {exhausted && <span className="quota-summary-state">{t("quota.limitReached")}</span>} | |
| </span> | |
| {rows.map((row, index) => { | |
| const warn = isQuotaWarn(row.percent, threshold); | |
| const exhausted = isQuotaExhausted(row.percent); | |
| return ( | |
| <span | |
| key={`${row.label}-${index}`} | |
| className={`quota-summary-item${warn ? " quota-summary-item--warn" : ""}`} | |
| title={exhausted ? t("quota.limitReached") : warn ? row.limitLabel : undefined} | |
| > | |
| <span>{row.label}</span> | |
| <strong>{Math.round(row.percent)}%</strong> | |
| {row.resetAt && <span className="quota-summary-reset">{formatResetFuture(row.resetAt, t, locale)}</span>} | |
| {exhausted && <span className="quota-summary-state">{t("quota.limitReached")}</span>} | |
| </span> |
🤖 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 `@gui/src/components/QuotaBars.tsx` around lines 160 - 173, Update the `title`
assignment on the `quota-summary-item` span in the `rows.map` renderer so
warning rows also receive a textual hint, reusing the existing
`t("quota.limitReached")` translation as suggested while preserving the
exhausted-state title behavior.
| .provider-detail-actions { | ||
| position: sticky; bottom: -24px; z-index: 2; display: flex; align-items: center; gap: 8px; | ||
| margin: 2px -24px -24px; padding: 14px 24px; border-top: 1px solid var(--border); | ||
| background: color-mix(in oklab, var(--surface) 94%, transparent); backdrop-filter: blur(12px); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add the -webkit- prefix for backdrop-filter on the sticky action bar.
Line 91 uses only the unprefixed backdrop-filter, while the rest of the codebase pairs it (see gui/src/styles.css Lines 1054-1055, which sets both backdrop-filter and -webkit-backdrop-filter). Without the prefix, Safari ≤ 17 renders this bar fully transparent over the scrolling model list, so the Add/Cancel buttons sit on top of unblurred content.
🎨 Proposed fix
- background: color-mix(in oklab, var(--surface) 94%, transparent); backdrop-filter: blur(12px);
+ background: color-mix(in oklab, var(--surface) 94%, transparent);
+ -webkit-backdrop-filter: blur(12px); backdrop-filter: blur(12px);📝 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.
| .provider-detail-actions { | |
| position: sticky; bottom: -24px; z-index: 2; display: flex; align-items: center; gap: 8px; | |
| margin: 2px -24px -24px; padding: 14px 24px; border-top: 1px solid var(--border); | |
| background: color-mix(in oklab, var(--surface) 94%, transparent); backdrop-filter: blur(12px); | |
| } | |
| .provider-detail-actions { | |
| position: sticky; bottom: -24px; z-index: 2; display: flex; align-items: center; gap: 8px; | |
| margin: 2px -24px -24px; padding: 14px 24px; border-top: 1px solid var(--border); | |
| background: color-mix(in oklab, var(--surface) 94%, transparent); | |
| -webkit-backdrop-filter: blur(12px); backdrop-filter: blur(12px); | |
| } |
🤖 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 `@gui/src/styles/provider-catalog.css` around lines 88 - 92, Add the
-webkit-backdrop-filter declaration alongside backdrop-filter in
.provider-detail-actions, preserving the existing blur(12px) value and keeping
the sticky action bar’s other styles unchanged.
| test("curated directory keeps 81 unique providers and counts glm-cn in both groups", () => { | ||
| expect(curatedPresets(rows)).toHaveLength(81); | ||
| expect(accessGroupCounts(rows)).toEqual({ | ||
| "recurring-or-keyless": 43, | ||
| "recurring-uncapped": 13, | ||
| "recurring-credit": 2, | ||
| "signup-credit": 24, | ||
| }); | ||
| expect(filterByAccessGroup(rows, "recurring-uncapped").some(row => row.id === "glm-cn")).toBe(true); | ||
| expect(filterByAccessGroup(rows, "signup-credit").some(row => row.id === "glm-cn")).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
These tests assert source text and hard-coded catalog counts, so they will fail on harmless edits and pass on real regressions.
Two distinct brittleness sources:
- Lines 25-35 bake the directory size into both the assertions and the test title (
keeps 81 unique providers), plus four per-group magic numbers. Adding one directory entry breaks five assertions and forces a title rename, with nothing explaining which number is the contract. Prefer invariants: every curated entry has ≥1 access group, ids are unique, andsum(accessGroupCounts) >= curatedPresets().length(strict>only for multi-group entries likeglm-cn, which the existing Lines 33-34 already pin explicitly). - Lines 62-64, 68-75, 79-84, 88-93, 97-110 and 114-118
readFileSyncthe component/CSS sources and regex the raw text.expect(styles).toContain(".main-inner.main-inner--providers { max-width: 1440px; }")(Line 118) breaks on a single reformat,expect(panel).toContain("oauthStatus[provider]?.loggedIn || busy === provider")(Line 70) pins an expression's exact spelling, andexpect(cards).not.toContain('layout="summary"')(Line 82) asserts the absence of a string rather than any behavior.gui/tests/provider-discovery-race.test.tsxalready renders into a container and queries the DOM — that pattern would let these same intents be verified behaviorally (e.g. renderProviderCardListwith a quota-less provider and assert the token fallback appears).
Also applies to: 61-65, 113-119
🤖 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 `@gui/tests/provider-catalog.test.ts` around lines 25 - 35, Replace hard-coded
catalog counts and test-title totals in the provider catalog tests with
invariants: curated entries must have access groups, IDs must be unique, and the
summed group counts must cover curated entries while explicitly preserving
multi-group membership for glm-cn. Replace source-file reads and regex/string
assertions with behavioral tests using rendered components and DOM queries,
following the ProviderCardList/container pattern in
provider-discovery-race.test.tsx; verify visible fallback and layout behavior
without depending on formatting or expression spelling.
| test("provider cards keep content and resource rows aligned", () => { | ||
| const cards = readFileSync(new URL("../src/components/providers/ProviderCardList.tsx", import.meta.url), "utf8"); | ||
| const css = readFileSync(new URL("../src/styles.css", import.meta.url), "utf8"); | ||
| expect(cards).toContain('className="prov-card-resource"'); | ||
| expect(css).toMatch(/\.prov-card\s*\{[^}]*grid-template-rows:\s*minmax\(0, 1fr\) auto minmax\(27px, auto\);/); | ||
| expect(css).toMatch(/\.provider-card-grid\s*\{[^}]*align-items:\s*stretch;/); | ||
| expect(css).toMatch(/@media \(max-width: 760px\)[\s\S]*\.prov-card-main\s*\{[^}]*grid-template-columns:\s*1fr;/); | ||
| }); | ||
|
|
||
| test("classic provider workspace exposes connection intents and a compact provider grid", () => { | ||
| const panel = readFileSync(new URL("../src/components/providers/OAuthPanel.tsx", import.meta.url), "utf8"); | ||
| const cards = readFileSync(new URL("../src/components/providers/ProviderCardList.tsx", import.meta.url), "utf8"); | ||
| const page = readFileSync(new URL("../src/pages/Providers.tsx", import.meta.url), "utf8"); | ||
| const css = readFileSync(new URL("../src/styles.css", import.meta.url), "utf8"); | ||
| expect(panel).toContain('onAddProvider({ tier: "accounts" })'); | ||
| expect(panel).toContain('onAddProvider({ tier: "paid" })'); | ||
| expect(panel).toContain('onAddProvider({ custom: true })'); | ||
| expect(cards).toContain('className="provider-card-grid"'); | ||
| expect(cards).toContain('t("dash.tokens30d")'); | ||
| expect(page).toContain('fetch(`${apiBase}/api/usage?range=30d`)'); | ||
| expect(page).toContain('initialTier={addIntent?.tier}'); | ||
| expect(page).toContain('initialCustom={addIntent?.custom}'); | ||
| expect(css).toMatch(/\.provider-card-grid\s*\{[^}]*grid-template-columns:\s*repeat\(2,/); | ||
| expect(css).toMatch(/@media \(max-width: 760px\)[\s\S]*\.provider-card-grid\s*\{[^}]*grid-template-columns:\s*1fr;/); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The [\s\S]* media-query assertions are vacuous — they pass even when the rule lives in a different media block.
Line 93 and Line 110 both use @media \(max-width: 760px\)[\s\S]*<selector>, and [\s\S]* happily spans the remainder of the file, including the closing } of the 760px block. So Line 110 only proves that .provider-card-grid { grid-template-columns: 1fr; } appears somewhere after the 760px header — which is exactly true of the copy in the trailing @media (max-width: 980px) block (gui/src/styles.css Lines 1110-1114). The assertion therefore cannot distinguish the intended mobile rule from the later block that currently overrides it. Anchor the match to the block body by forbidding an intervening }-at-line-start, or better, assert the computed layout by rendering.
🐛 Proposed fix: scope the match to the media block's own body
- expect(css).toMatch(/@media \(max-width: 760px\)[\s\S]*\.prov-card-main\s*\{[^}]*grid-template-columns:\s*1fr;/);
+ // [^@]* keeps the match inside the 760px block — no later `@media` rule can satisfy it.
+ expect(css).toMatch(/@media \(max-width: 760px\)\s*\{[^@]*\.prov-card-main\s*\{[^}]*grid-template-columns:\s*1fr;/);Apply the same anchoring to the .provider-card-grid assertion at Line 110.
🤖 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 `@gui/tests/provider-catalog.test.ts` around lines 87 - 111, Scope both
media-query assertions in “provider cards keep content and resource rows
aligned” and “classic provider workspace exposes connection intents and a
compact provider grid” to the matched `@media` block body, preventing the regex
from crossing a closing brace or matching a later media block. Preserve the
existing selectors and expected mobile layout while anchoring the match to the
760px rule.
| export type SmartRoutingMode = "intelligence" | "balance" | "cost"; | ||
|
|
||
| export const SMART_ROUTING_MODES: readonly SmartRoutingMode[] = ["intelligence", "balance", "cost"]; | ||
| export const SMART_ROUTING_PROFILE_VERIFIED_AT = "2026-07-24"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The mode enum is still duplicated between server and GUI with no shared source of truth.
SmartRoutingMode/SMART_ROUTING_MODES here remain independently re-declared in gui/src/pages/Models.tsx (Line 47 and the hardcoded ["intelligence", "balance", "cost"] at Line 725). Adding or renaming a mode server-side silently no-ops in the UI. Expose this list to the GUI at runtime (e.g. through the existing config/bootstrap payload) instead.
🤖 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/combos/smart-routing.ts` around lines 11 - 14, Replace the GUI-side
SmartRoutingMode declaration and hardcoded mode list in Models.tsx with the
runtime mode list sourced from the existing config/bootstrap payload. Expose
SMART_ROUTING_MODES through that payload, update the GUI to derive its types and
rendering/validation from the received list, and preserve the current behavior
for the existing modes.
| test("every free provider catalog entry has a local icon", () => { | ||
| for (const provider of FREE_PROVIDER_DIRECTORY) { | ||
| expect(providerIconSrc(provider.id, { models: provider.models })).toMatch(/^\/provider-icons\//); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Surface the failing provider id in the loop assertion.
With ~81 iterations, a failure at Line 485 reports only expected undefined to match /^\/provider-icons\//, leaving you to bisect the directory to find which entry lost its icon. Collect the offenders instead so one run names them all.
💚 Proposed fix
test("every free provider catalog entry has a local icon", () => {
- for (const provider of FREE_PROVIDER_DIRECTORY) {
- expect(providerIconSrc(provider.id, { models: provider.models })).toMatch(/^\/provider-icons\//);
- }
+ const missing = FREE_PROVIDER_DIRECTORY
+ .filter(provider => !providerIconSrc(provider.id, { models: provider.models })?.startsWith("/provider-icons/"))
+ .map(provider => provider.id);
+ expect(missing).toEqual([]);
});📝 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.
| test("every free provider catalog entry has a local icon", () => { | |
| for (const provider of FREE_PROVIDER_DIRECTORY) { | |
| expect(providerIconSrc(provider.id, { models: provider.models })).toMatch(/^\/provider-icons\//); | |
| } | |
| }); | |
| test("every free provider catalog entry has a local icon", () => { | |
| const missing = FREE_PROVIDER_DIRECTORY | |
| .filter(provider => !providerIconSrc(provider.id, { models: provider.models })?.startsWith("/provider-icons/")) | |
| .map(provider => provider.id); | |
| expect(missing).toEqual([]); | |
| }); |
🤖 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 `@tests/provider-workspace-data.test.ts` around lines 483 - 487, Update the
“every free provider catalog entry has a local icon” test to collect provider
IDs whose providerIconSrc result fails the expected path pattern, then assert
the collected offenders are empty so a failure reports all missing-icon provider
IDs instead of stopping at the first iteration.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
7daaee4 to
9b05269
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b05269eb7
ℹ️ 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".
| .filter(model => model.provider !== "combo" && config.providers[model.provider]?.disabled !== true) | ||
| .filter(model => config.providers[model.provider] !== undefined) | ||
| .filter(model => !(config.disabledModels ?? []).some(disabled => slugEquals(disabled, model.provider, model.id))) |
There was a problem hiding this comment.
When a key-auth provider is saved without an apiKey (the add-provider path allows this, and static/configured models are still returned), this filter still treats its models as eligible and persists them into an auto-* combo. Selecting that smart route then fails at request time because adapters such as openai-chat throw on authMode: "key" without a credential, so the automatic combo can be created successfully but be unusable; check provider auth readiness before adding targets.
Useful? React with 👍 / 👎.
| matchesResolvedTemplate = candidateUrl.origin === templateUrl.origin | ||
| && !candidateUrl.search | ||
| && !candidateUrl.hash | ||
| && pathPattern.test(candidateUrl.pathname); |
There was a problem hiding this comment.
When a preset template puts the placeholder in the hostname, such as Azure OpenAI's https://{resource}.openai.azure.com/openai, this origin equality can never pass after the user replaces {resource} with their real resource name. The discover endpoint then rejects the resolved URL with provider baseUrl does not match the trusted preset before it ever fetches models, even though the same resolved URL can be saved; match the hostname against the template as well as the path.
Useful? React with 👍 / 👎.
| const invalidateDiscoveryRequest = () => { | ||
| discoveryRequestRef.current += 1; | ||
| discoveryAbortRef.current?.abort(); | ||
| discoveryAbortRef.current = null; | ||
| setDiscoveryBusy(false); | ||
| }; |
There was a problem hiding this comment.
When a discovery-affecting field changes, this only aborts in-flight requests and clears the busy flag, leaving the previous discoveredModels/selectedModels in state. If a user discovers models for one endpoint or key, selects a subset, then edits the base URL/API key/endpoint before adding the provider, submit still persists that stale allowlist against the new provider; clear the discovery state (and any discovery-derived default) on invalidation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/catalog/provider-fetch.ts (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBoth
allowsModelsEnvelopechecks re-derive Google's ai-studio mode inline instead of reusingeffectiveGoogleMode, and theprovider-fetch.tscopy has a real registry-fallback gap.
buildModelsRequestdecides the ai-studio branch viaeffectiveGoogleMode(providerName, effectiveProvider), which checksprov.googleMode ?? registryEntry.googleMode ?? "ai-studio". Both discovery call sites instead inlineentry/prov.googleMode ?? "ai-studio", skipping the registry-fallback step entirely.
src/codex/catalog/provider-fetch.ts#L418-421:provis the user's runtimeOcxProviderConfig, which can genuinely havegoogleModeunset while the registry's default is"vertex"/"cloud-code-assist"— in that casebuildModelsRequestbuilds a non-ai-studio request, but this envelope check still assumes ai-studio, mis-validating the response shape. Fix:const allowsModelsEnvelope = effectiveGoogleMode(name, prov) === "ai-studio" || ...(importeffectiveGoogleModefrom../../providers/registryif not already imported).src/server/management/provider-routes.ts#L489-491:entryis the raw registry entry with no user-override channel in this endpoint, so the current behavior happens to matcheffectiveGoogleMode's result today — but it should still calleffectiveGoogleMode(presetId, entry) === "ai-studio"to stay correct if that assumption ever changes (e.g. a future override field is merged intoproviderbefore this check).♻️ Proposed fix (both sites)
- const allowsModelsEnvelope = (prov.adapter === "google" && (prov.googleMode ?? "ai-studio") === "ai-studio") + const allowsModelsEnvelope = effectiveGoogleMode(name, prov) === "ai-studio" || new URL(url).pathname.replace(/\/+$/, "").endsWith("/api/tags");- const allowsModelsEnvelope = (entry.adapter === "google" && (entry.googleMode ?? "ai-studio") === "ai-studio") + const allowsModelsEnvelope = effectiveGoogleMode(presetId, entry) === "ai-studio" || new URL(request.url).pathname.replace(/\/+$/, "").endsWith("/api/tags");🤖 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/codex/catalog/provider-fetch.ts` at line 1, Update both allowsModelsEnvelope checks in provider-fetch.ts and provider-routes.ts to use effectiveGoogleMode with the relevant provider name and configuration instead of re-deriving googleMode inline. Import effectiveGoogleMode from the registry module where needed, passing name/prov in provider-fetch.ts and presetId/entry in provider-routes.ts, while preserving the existing ai-studio comparison and envelope logic.
🤖 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 `@gui/src/components/provider-catalog/ProviderCatalog.tsx`:
- Around line 203-210: Update
gui/src/components/provider-catalog/ProviderCatalog.tsx lines 203-210 to derive
needsReauth from accountStatus[row.id], display the re-authentication label when
set, and route the action column at lines 215-221 to login instead of logout;
update gui/src/components/providers/OAuthPanel.tsx lines 62-67 to use the amber
status dot and re-auth label when st.needsReauth is true, matching
ProviderCardList behavior.
In `@src/combos/smart-routing.ts`:
- Around line 80-89: Update modelCost to preserve explicit zero-cost pricing
from findExpectedPriceOverlay or metadata as a valid result, returning 0 for
free models while reserving undefined only for missing pricing data. Remove the
positive-value checks that discard {input: 0, output: 0}, while retaining the
existing provider and pricing-source selection behavior.
In `@src/server/management/provider-routes.ts`:
- Around line 467-478: Propagate the caller’s allowPrivateNetwork opt-in into
the provider object used by the models-URL check in the discovery flow. Update
the object passed to providerDestinationResolvedError for request.url to include
allowPrivateNetwork from the resolved provider, preserving the existing
destination validation behavior.
- Around line 427-442: Update matchesResolvedTemplate to resolve placeholders in
the templateUrl hostname/origin as well as its pathname, so host-templated
presets such as Azure OpenAI match concrete supplied URLs. Replace the literal
origin comparison with equivalent hostname/host pattern validation while
preserving the existing search, hash, and path constraints.
---
Outside diff comments:
In `@src/codex/catalog/provider-fetch.ts`:
- Line 1: Update both allowsModelsEnvelope checks in provider-fetch.ts and
provider-routes.ts to use effectiveGoogleMode with the relevant provider name
and configuration instead of re-deriving googleMode inline. Import
effectiveGoogleMode from the registry module where needed, passing name/prov in
provider-fetch.ts and presetId/entry in provider-routes.ts, while preserving the
existing ai-studio comparison and envelope logic.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 85ca47a2-1bb3-4940-bfbd-aae492492436
⛔ Files ignored due to path filters (54)
gui/public/provider-icons/agentrouter.pngis excluded by!**/*.pnggui/public/provider-icons/agnesai.svgis excluded by!**/*.svggui/public/provider-icons/ai21-color.svgis excluded by!**/*.svggui/public/provider-icons/aionlabs-color.svgis excluded by!**/*.svggui/public/provider-icons/api-airforce.svgis excluded by!**/*.svggui/public/provider-icons/arcee-color.svgis excluded by!**/*.svggui/public/provider-icons/baichuan-color.svgis excluded by!**/*.svggui/public/provider-icons/bazaarlink.svgis excluded by!**/*.svggui/public/provider-icons/blackbox.pngis excluded by!**/*.pnggui/public/provider-icons/bluesminds.svgis excluded by!**/*.svggui/public/provider-icons/cerebras-color.svgis excluded by!**/*.svggui/public/provider-icons/cohere-color.svgis excluded by!**/*.svggui/public/provider-icons/coze.svgis excluded by!**/*.svggui/public/provider-icons/deepinfra-color.svgis excluded by!**/*.svggui/public/provider-icons/duckduckgo.svgis excluded by!**/*.svggui/public/provider-icons/freemodel-dev.svgis excluded by!**/*.svggui/public/provider-icons/friendli.svgis excluded by!**/*.svggui/public/provider-icons/hackclub.svgis excluded by!**/*.svggui/public/provider-icons/hyperbolic-color.svgis excluded by!**/*.svggui/public/provider-icons/iflytek-color.svgis excluded by!**/*.svggui/public/provider-icons/inference.svgis excluded by!**/*.svggui/public/provider-icons/kilocode.svgis excluded by!**/*.svggui/public/provider-icons/liquid.svgis excluded by!**/*.svggui/public/provider-icons/litellm.pngis excluded by!**/*.pnggui/public/provider-icons/llm7.svgis excluded by!**/*.svggui/public/provider-icons/longcat-color.svgis excluded by!**/*.svggui/public/provider-icons/monsterapi.svgis excluded by!**/*.svggui/public/provider-icons/morph-color.svgis excluded by!**/*.svggui/public/provider-icons/nebius.svgis excluded by!**/*.svggui/public/provider-icons/nlpcloud.svgis excluded by!**/*.svggui/public/provider-icons/nousresearch.svgis excluded by!**/*.svggui/public/provider-icons/novita-color.svgis excluded by!**/*.svggui/public/provider-icons/nscale.pngis excluded by!**/*.pnggui/public/provider-icons/ovh.svgis excluded by!**/*.svggui/public/provider-icons/pollinations.svgis excluded by!**/*.svggui/public/provider-icons/predibase.pngis excluded by!**/*.pnggui/public/provider-icons/provider-generic.svgis excluded by!**/*.svggui/public/provider-icons/publicai.svgis excluded by!**/*.svggui/public/provider-icons/puter.svgis excluded by!**/*.svggui/public/provider-icons/qoder-color.svgis excluded by!**/*.svggui/public/provider-icons/reka.pngis excluded by!**/*.pnggui/public/provider-icons/requesty.svgis excluded by!**/*.svggui/public/provider-icons/sambanova-color.svgis excluded by!**/*.svggui/public/provider-icons/scaleway.svgis excluded by!**/*.svggui/public/provider-icons/sensenova-color.svgis excluded by!**/*.svggui/public/provider-icons/siliconcloud-color.svgis excluded by!**/*.svggui/public/provider-icons/spark-color.svgis excluded by!**/*.svggui/public/provider-icons/stepfun-color.svgis excluded by!**/*.svggui/public/provider-icons/t3-web.svgis excluded by!**/*.svggui/public/provider-icons/tencent-color.svgis excluded by!**/*.svggui/public/provider-icons/together-color.svgis excluded by!**/*.svggui/public/provider-icons/uncloseai.svgis excluded by!**/*.svggui/public/provider-icons/volcengine-color.svgis excluded by!**/*.svggui/public/provider-icons/zhipu-color.svgis excluded by!**/*.svg
📒 Files selected for processing (42)
gui/public/provider-icons/LICENSE.litellmgui/public/provider-icons/LICENSE.lobe-iconsgui/public/provider-icons/LICENSE.omniroutegui/public/provider-icons/LICENSE.simple-iconsgui/src/App.tsxgui/src/components/AddProviderModal.tsxgui/src/components/QuotaBars.tsxgui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-catalog/provider-presets.tsgui/src/components/providers/OAuthPanel.tsxgui/src/components/providers/ProviderCardList.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Models.tsxgui/src/pages/Providers.tsxgui/src/provider-icons.tsgui/src/styles.cssgui/src/styles/provider-catalog.cssgui/tests/provider-catalog.test.tsgui/tests/provider-discovery-race.test.tsxsrc/codex/catalog/provider-fetch.tssrc/combos/index.tssrc/combos/smart-routing.tssrc/oauth/index.tssrc/providers/derive.tssrc/providers/free-directory.tssrc/providers/registry.tssrc/server/management/combo-routes.tssrc/server/management/provider-routes.tstests/combo-management-api.test.tstests/google-models-listing.test.tstests/provider-connection-test.test.tstests/provider-directory-routing.test.tstests/provider-free-directory.test.tstests/provider-model-envelope.test.tstests/provider-registry-parity.test.tstests/provider-workspace-data.test.tstests/smart-routing.test.ts
| {tier === "accounts" && filteredAccountRows.map(row => { | ||
| const status = accountStatus[row.id]; | ||
| const busy = busyProvider === row.id; | ||
| const loggedIn = !!status?.loggedIn; | ||
| const statusText = loggedIn ? (status.email ?? row.statusLabel ?? t("modal.accountLoggedIn")) : (status?.error ?? row.statusLabel ?? t("modal.accountLoggedOut")); | ||
| return ( | ||
| <div key={row.id} className="provider-catalog-row provider-catalog-account-row"> | ||
| <span className="provider-catalog-row-copy"><span className="title">{row.label}</span><span className="sub">{statusText}</span></span> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
needsReauth is plumbed through the status objects but never rendered on either connected-provider surface. gui/src/pages/Providers.tsx Line 137 sets needsReauth: true on the Codex/forward status that feeds both components, and AccountLoginStatus was extended for it in gui/src/components/provider-catalog/ProviderCatalog.tsx Line 16 — yet both row renderers branch only on loggedIn, so an account the management API says must re-authenticate is presented as healthy and offers only a logout action.
gui/src/components/provider-catalog/ProviderCatalog.tsx#L203-L210: deriveneedsReauthfromaccountStatus[row.id], use a re-auth label forstatusText, and route the action column at Lines 215-221 to the login button instead of logout in that state.gui/src/components/providers/OAuthPanel.tsx#L62-L67: make the dot amber and the label a re-auth string whenst.needsReauthis true, mirroring thepws.reauthtreatment already used ingui/src/components/providers/ProviderCardList.tsxLine 94.
As per path instructions, "GUI state changes stay consistent with the management API responses."
📍 Affects 2 files
gui/src/components/provider-catalog/ProviderCatalog.tsx#L203-L210(this comment)gui/src/components/providers/OAuthPanel.tsx#L62-L67
🤖 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 `@gui/src/components/provider-catalog/ProviderCatalog.tsx` around lines 203 -
210, Update gui/src/components/provider-catalog/ProviderCatalog.tsx lines
203-210 to derive needsReauth from accountStatus[row.id], display the
re-authentication label when set, and route the action column at lines 215-221
to login instead of logout; update gui/src/components/providers/OAuthPanel.tsx
lines 62-67 to use the amber status dot and re-auth label when st.needsReauth is
true, matching ProviderCardList behavior.
Source: Path instructions
| function modelCost(config: OcxConfig, model: CatalogModel): number | undefined { | ||
| const provider = config.providers[model.provider]; | ||
| if (provider?.authMode === "local") return 0; | ||
| const metaCost = metadata(model.provider, model.id)?.cost; | ||
| const overlay = findExpectedPriceOverlay(model.provider, model.id)?.cost4; | ||
| const cost = overlay | ||
| ?? (metaCost && Object.values(metaCost).some(value => value > 0) ? metaCost : undefined); | ||
| if (!cost || !Object.values(cost).some(value => value > 0)) return undefined; | ||
| return cost.input + cost.output * 2; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Not applicable — please confirm internally: does findExpectedPriceOverlay's cost4 (or the generated jawcode model metadata cost field) ever legitimately report {input:0, output:0} for a verified free-tier model, as opposed to using it as a sentinel for "pricing unknown"?
💡 Result:
Yes, the {input: 0, output: 0} structure is used legitimately to represent free-tier models, not merely as a sentinel for unknown pricing [1][2][3]. In systems utilizing the cost4 metadata structure or jawcode metadata, a value of 0 for input and output costs is explicitly assigned to models designated as free (such as those with a :free suffix or specifically registered free-tier models like NVIDIA Nemotron variants) to bypass payment challenges and budget throttling [1][2][3]. Key distinctions include: 1. Legitimate Zero-Cost: Models verified as free-tier are explicitly assigned {input: 0, output: 0, cacheRead: 0, cacheWrite: 0} [2][3]. This allows the gateway to identify the request as cost-free and skip payment processes (e.g., 402 challenges) [1]. 2. Sentinel vs. Explicit: While some systems may have issues where unpriced or "null" catalog entries are incorrectly treated as $0—often leading to them being erroneously selected by cost-optimized routers [4]—verified free models utilize the explicit 0-cost assignment as a functional requirement for free-tier operation [1][2]. 3. Implementation: In codebases like those referencing resolveMatchedPrice or getPricingSync, an explicit check for free-tier eligibility (or the presence of a known free-model identifier) ensures that the zero values are treated as a deliberate pricing policy rather than a data omission [5][2].
Citations:
- 1: https://docs.solvela.ai/docs/concepts/free-tier
- 2: BYK/loreai@629b92f
- 3: BYK/loreai@e02e178
- 4: cost-optimized routing treats null-priced models as $0 (always picked as cheapest) ferro-labs/ai-gateway#126
- 5: https://cephalochromoscope.net/9ce1cc9a-41ca-4ef5-85af-3fdd76949f2a
Treat explicit zero-cost overlays as free, not unknown
src/combos/smart-routing.ts:80-89 turns any remote model with overlay.cost4 or metadata cost {input: 0, output: 0} into undefined via the Object.values(cost).some(value => value > 0) check. That makes legitimate free-tier models invisible to "cost" mode in buildSmartRoutingCombo, because eligibility depends on candidate.cost !== undefined. Keep 0 as a valid cost and reserve undefined for missing pricing data.
🤖 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/combos/smart-routing.ts` around lines 80 - 89, Update modelCost to
preserve explicit zero-cost pricing from findExpectedPriceOverlay or metadata as
a valid result, returning 0 for free models while reserving undefined only for
missing pricing data. Remove the positive-value checks that discard {input: 0,
output: 0}, while retaining the existing provider and pricing-source selection
behavior.
| let matchesResolvedTemplate = false; | ||
| if (suppliedBaseUrl && entry.baseUrl.includes("{")) { | ||
| try { | ||
| const marker = "opencodex-template-value"; | ||
| const templateUrl = new URL(entry.baseUrl.replace(/\{[^}]+\}/g, marker)); | ||
| const candidateUrl = new URL(suppliedBaseUrl); | ||
| const escapedPath = templateUrl.pathname.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const pathPattern = new RegExp(`^${escapedPath.replaceAll(marker, "[A-Za-z0-9_-]+")}/?$`); | ||
| matchesResolvedTemplate = candidateUrl.origin === templateUrl.origin | ||
| && !candidateUrl.search | ||
| && !candidateUrl.hash | ||
| && pathPattern.test(candidateUrl.pathname); | ||
| } catch { | ||
| matchesResolvedTemplate = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for registry baseUrl templates with a placeholder in the hostname (before the first '/').
fd registry.ts src/providers --exec rg -n 'baseUrl:\s*"[^"]*\{[^"/]*\}[^"]*"' {}Repository: lidge-jun/opencodex
Length of output: 715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the provider route logic around matchesResolvedTemplate.
sed -n '400,470p' src/server/management/provider-routes.ts
# Inspect the registry entries that use templated baseUrl values.
sed -n '650,710p' src/providers/registry.ts
sed -n '970,1005p' src/providers/registry.tsRepository: lidge-jun/opencodex
Length of output: 13037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether these templated presets have explicit accepted URL choices or overrides.
rg -n 'id:\s*"(azure-openai|cloudflare-ai-gateway|cloudflare-workers-ai)"|allowBaseUrlOverride|baseUrlChoices' src/providers/registry.ts src/server/management/provider-routes.ts
# Inspect any tests or call sites covering preset discovery/connection for templated URLs.
rg -n 'provider-presets/discover|matchesResolvedTemplate|trusted preset|baseUrlChoices' src testRepository: lidge-jun/opencodex
Length of output: 4282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '670,700p' src/providers/registry.ts
sed -n '980,1015p' src/providers/registry.tsRepository: lidge-jun/opencodex
Length of output: 5829
Handle host placeholders in matchesResolvedTemplate src/server/management/provider-routes.ts:427-447 only turns the templated pathname into a regex; the origin check stays literal after replacing {...} with "opencodex-template-value". That breaks host-templated presets like src/providers/registry.ts:676 (azure-openai, https://{resource}.openai.azure.com/openai): a real URL such as https://my-resource.openai.azure.com/openai will never match and gets rejected as “does not match the trusted preset.” Compare the resolved hostname too, or build the regex against host/origin so Azure OpenAI-style templates can pass without a manual override.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 433-433: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapedPath.replaceAll(marker, "[A-Za-z0-9_-]+")}/?$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 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/server/management/provider-routes.ts` around lines 427 - 442, Update
matchesResolvedTemplate to resolve placeholders in the templateUrl
hostname/origin as well as its pathname, so host-templated presets such as Azure
OpenAI match concrete supplied URLs. Replace the literal origin comparison with
equivalent hostname/host pattern validation while preserving the existing
search, hash, and path constraints.
| const destinationError = await providerDestinationResolvedError(presetId, provider); | ||
| if (destinationError) return jsonResponse({ error: destinationError }, 400); | ||
| if (entry.discovery === "static" || entry.discovery === "unsupported" || entry.liveModels === false) { | ||
| return jsonResponse(fallbackModels.length | ||
| ? { ok: true, models: fallbackModels, source: "static", latencyMs: 0 } | ||
| : { ok: false, models: [], source: "static", latencyMs: 0, error: "No static model catalog is available for this preset" }); | ||
| } | ||
| const { resolveModelsAuthToken, buildModelsRequest } = await import("../../oauth"); | ||
| const apiKey = await resolveModelsAuthToken(presetId, provider); | ||
| const request = buildModelsRequest(provider, apiKey, presetId); | ||
| const modelsDestinationError = await providerDestinationResolvedError(presetId, { baseUrl: request.url }); | ||
| if (modelsDestinationError) return jsonResponse({ error: modelsDestinationError }, 400); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Models-discovery destination check silently drops the caller's allowPrivateNetwork opt-in, breaking discovery for legitimately-allowed private-network providers.
providerDestinationResolvedError(presetId, provider) at Line 467 is checked against the full provider object (which may carry allowPrivateNetwork: true from supplied.allowPrivateNetwork === true at Line 456). But the second check at Line 477, against the resolved models URL, builds a bare { baseUrl: request.url } — omitting allowPrivateNetwork. Per providerDestinationResolvedError's own escape hatch (registryAllowsPrivateNetwork(name) || provider.allowPrivateNetwork === true), any private/self-hosted host that just passed the first check (typically the same host, since request.url is usually ${provider.baseUrl}/models) will now fail the second check purely because the flag wasn't propagated — even though the user explicitly authorized it moments earlier.
🐛 Proposed fix: propagate allowPrivateNetwork to the models-URL destination check
- const modelsDestinationError = await providerDestinationResolvedError(presetId, { baseUrl: request.url });
+ const modelsDestinationError = await providerDestinationResolvedError(presetId, {
+ baseUrl: request.url,
+ allowPrivateNetwork: provider.allowPrivateNetwork,
+ });📝 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.
| const destinationError = await providerDestinationResolvedError(presetId, provider); | |
| if (destinationError) return jsonResponse({ error: destinationError }, 400); | |
| if (entry.discovery === "static" || entry.discovery === "unsupported" || entry.liveModels === false) { | |
| return jsonResponse(fallbackModels.length | |
| ? { ok: true, models: fallbackModels, source: "static", latencyMs: 0 } | |
| : { ok: false, models: [], source: "static", latencyMs: 0, error: "No static model catalog is available for this preset" }); | |
| } | |
| const { resolveModelsAuthToken, buildModelsRequest } = await import("../../oauth"); | |
| const apiKey = await resolveModelsAuthToken(presetId, provider); | |
| const request = buildModelsRequest(provider, apiKey, presetId); | |
| const modelsDestinationError = await providerDestinationResolvedError(presetId, { baseUrl: request.url }); | |
| if (modelsDestinationError) return jsonResponse({ error: modelsDestinationError }, 400); | |
| const destinationError = await providerDestinationResolvedError(presetId, provider); | |
| if (destinationError) return jsonResponse({ error: destinationError }, 400); | |
| if (entry.discovery === "static" || entry.discovery === "unsupported" || entry.liveModels === false) { | |
| return jsonResponse(fallbackModels.length | |
| ? { ok: true, models: fallbackModels, source: "static", latencyMs: 0 } | |
| : { ok: false, models: [], source: "static", latencyMs: 0, error: "No static model catalog is available for this preset" }); | |
| } | |
| const { resolveModelsAuthToken, buildModelsRequest } = await import("../../oauth"); | |
| const apiKey = await resolveModelsAuthToken(presetId, provider); | |
| const request = buildModelsRequest(provider, apiKey, presetId); | |
| const modelsDestinationError = await providerDestinationResolvedError(presetId, { | |
| baseUrl: request.url, | |
| allowPrivateNetwork: provider.allowPrivateNetwork, | |
| }); | |
| if (modelsDestinationError) return jsonResponse({ error: modelsDestinationError }, 400); |
🤖 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/server/management/provider-routes.ts` around lines 467 - 478, Propagate
the caller’s allowPrivateNetwork opt-in into the provider object used by the
models-URL check in the discovery flow. Update the object passed to
providerDestinationResolvedError for request.url to include allowPrivateNetwork
from the resolved provider, preserving the existing destination validation
behavior.
🔒 Under maintainer review — detailed feedback incoming@lidge-jun (maintainer) has this PR in an active review pass. Please do not merge or close This PR is not part of the current integration batch. It is held back deliberately — Heads-up on rebasing: No action needed from you until then. Thanks for the contribution and for your patience. Review tracker: |
|
Thank you for the large amount of provider-workspace work in this PR. At 96 files (
Each split should target current
The current diff also does not apply to current I appreciate the breadth of the contribution. Splitting it along the boundaries above will let us review trust metadata, discovery behavior, workspace UX, and routing correctness independently. Please re-cut each split from current Reviewed as part of a maintainer integration pass. |
Provider discovery and safety
/modelscatalogs with cache, cooldown, static fallback, malformed-response rejection, and support for OpenAI, Google, Ollama, and Fireworks response envelopes.Provider setup and workspace
Routing and OpenRouter
Validation
bun run test— 4,112 tests passedbun run typecheckbun run lint:guibun run build:guibun run privacy:scanSummary by CodeRabbit
Summary
New Features
Bug Fixes
Documentation
Tests