feat(gui): provider discovery-failure badge + honest Claude helper fallback copy (#329, #331)#358
Conversation
📝 WalkthroughWalkthroughThe change records structured provider model-discovery outcomes, exposes them through provider APIs, and displays localized failure details in the Models UI. It also clarifies Claude Code’s unset helper-model behavior with UI warnings, tests, and multilingual documentation. ChangesProvider discovery failure reporting
Claude helper-model transparency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderDiscovery
participant ModelCache
participant ProviderAPI
participant ModelsPage
ProviderDiscovery->>ModelCache: record ok or failure reason
ProviderAPI->>ModelCache: read discovery status
ModelCache-->>ProviderAPI: return discovery summary
ModelsPage->>ProviderAPI: fetch provider summaries
ProviderAPI-->>ModelsPage: return discovery metadata
ModelsPage-->>ModelsPage: render badge and localized reason
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: b620df88bc
ℹ️ 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".
| <div style={{ padding: "6px 12px" }}> | ||
| {isNative && <p className="muted text-label" style={{ margin: "2px 0 6px" }}>{t("models.nativeHint")}</p>} | ||
| {rows.length === 0 && <EmptyProviderHint liveModels={liveModels} />} | ||
| {rows.length === 0 && <EmptyProviderHint liveModels={liveModels} discovery={discovery} />} |
There was a problem hiding this comment.
When a failed live fetch falls back to configured, default, or stale models, fetchProviderModels records the failure but returns nonempty rows; this condition then skips EmptyProviderHint, so the new badge remains invisible and the discovery failure is still log-only. Render the failure status independently of the empty-provider hint so providers using fallback rows also expose the failed discovery.
Useful? React with 👍 / 👎.
| {value === "" && ( | ||
| <p className="notice-warn" role="note" style={{ marginTop: 8 }}> | ||
| {t("claude.smallFastModelNativeWarning")} | ||
| </p> |
There was a problem hiding this comment.
For configurations with tierModels.haiku set but smallFastModel empty, this warning is false: effectiveModelEnv uses tierModels.haiku ?? smallFastModel and sets both helper variables, so Claude Code does not fall back to native Sonnet. Base the warning on the effective Haiku/helper value (available via effectiveModelEnv) rather than smallFastModel alone.
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: 1
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/Models.tsx (1)
755-800: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDiscovery-failed badge only ever renders for fully empty providers — misses the exact scenario issue
#329calls out.
EmptyProviderHint(which owns the "Discovery failed" badge/reason/settings-link, lines 1136-1157) is only invoked at Line 800 whenrows.length === 0. If a provider has even one manually configured/static model and live discovery is currently failing (e.g. HTTP 401 on/v1/models), the group renders normally with zero indication that discovery is broken — the header at lines 760-762 shows only the provider name, native badge, and active count, with no failure signal.This directly undercuts the PR's own objective (
#329): "the provider should remain visible, discovery failures should be surfaced clearly, and manually configured models should remain usable and selectable" — those two conditions (usable static models + failing discovery) are exactly the case this gating misses. It's also untested:gui/tests/models-empty-provider.test.tsx's "destination-blocked" and "invalid JSON" tests configure amodels: ["static-fallback"]provider and assert the discovery state directly, but only exerciseEmptyProviderHintin isolation — never the actualrows.length > 0header path.Move the badge to the always-rendered group header so it survives regardless of row count:
🛠️ Proposed fix: surface the badge in the group header, not just the empty state
<span className="text-body font-semibold">{provider}</span> {isNative && <span className="muted mono text-caption" style={{ padding: "1px 6px", border: "1px solid var(--border)", borderRadius: "var(--radius-pill)" }}>{t("models.nativeGroupLabel")}</span>} + {liveModels && discovery?.status === "failed" && ( + <span className="badge badge-amber" title={discoveryFailureReason(t, discovery)}> + {t("models.discoveryFailedBadge")} + </span> + )} <span className="muted mono text-label">{t("models.active", { active: activeCount, total: rows.length })}</span>The detailed hint text inside
EmptyProviderHintcan stay gated onrows.length === 0(it's the right place for the full explanation + settings link when there's nothing else to show), but the header badge should be unconditional on row count.🤖 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 755 - 800, Move the discovery-failure badge from the rows.length === 0 rendering path into the always-rendered provider group header around the provider name and active count, using the existing discovery failure state and badge component from EmptyProviderHint. Keep EmptyProviderHint’s detailed explanation and settings link gated to empty providers, while ensuring the header badge appears for providers with manually configured models as well.
🤖 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/tests/claude-code-background-helper.test.tsx`:
- Around line 6-20: Restore the original navigator.language property descriptor
rather than only its value. In the beforeEach/afterEach hooks, snapshot
Object.getOwnPropertyDescriptor(globalThis.navigator, "language"), then restore
that descriptor verbatim after each test; if no own descriptor existed, delete
the temporary property.
---
Outside diff comments:
In `@gui/src/pages/Models.tsx`:
- Around line 755-800: Move the discovery-failure badge from the rows.length ===
0 rendering path into the always-rendered provider group header around the
provider name and active count, using the existing discovery failure state and
badge component from EmptyProviderHint. Keep EmptyProviderHint’s detailed
explanation and settings link gated to empty providers, while ensuring the
header badge appears for providers with manually configured models as well.
🪄 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: 02b5cded-d446-451d-a9ac-233d3fa00927
📒 Files selected for processing (23)
docs-site/src/content/docs/guides/claude-code.mddocs-site/src/content/docs/ja/guides/claude-code.mddocs-site/src/content/docs/ko/guides/claude-code.mddocs-site/src/content/docs/ru/guides/claude-code.mddocs-site/src/content/docs/zh-cn/guides/claude-code.mdgui/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/models-groups.tsgui/src/pages/ClaudeCode.tsxgui/src/pages/Models.tsxgui/tests/claude-code-background-helper.test.tsxgui/tests/models-empty-provider.test.tsxsrc/codex/catalog/provider-fetch.tssrc/codex/model-cache.tssrc/server/management/provider-routes.tstests/codex-catalog.test.tstests/cursor-hardening.test.tstests/management-provider-validation.test.tstests/models-page-groups.test.ts
|
#354 also fixes #329 with a few extras worth folding in before merge:
Happy to take those as a tiny follow-up after #358 if you prefer to merge as-is. Either way we should close #354 with credit to @stantheman0128. |
|
Thanks for the ping @Wibias. Agree those three follow-ups (skipped when liveModels === false, optional empty/fallback status fields, Models doc line) are worth folding into #358 before merge. Happy to review or take a tiny follow-up after #358 if you prefer to land the main path first. Appreciate the credit. |
Dual review (security + bugs/edge cases)Bot review threads are not cleared yet (3 unresolved: 2 Codex P2 + 1 CodeRabbit test hygiene). CI is green. Below is an independent second look (security + functional), including overlap notes with #354. Security categories checkedNo evidence of malware, backdoor, hidden C2, local admission bypass, classic XSS/HTML sinks, or credential logging. Discovery status is a small discriminated union ( Privacy posture for this change looks good. Blockers
Should-fix before merge
Non-blocking / already solid
Aligns with open bot threads + #354Findings 1–2 match the open Codex P2s. Finding 3 matches the Wibias/#354 extras already discussed on this PR (skipped when Suggested fix order
|
|
@Wibias Thanks for the dual review. Agree with the fix order, especially showing the discovery-failure badge even when fallback rows keep the group nonempty, and folding the #354 extras (skipped when liveModels === false, clearer status fields) into this PR rather than merging both. #354 can stay closed with credit as discussed. Happy to help on a tiny follow-up after #358 if you prefer to keep this PR lean; otherwise taking those into #358 is ideal. Stan Shih (stantheman0128) |
b620df8 to
631fa1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/tests/models-empty-provider.test.tsx (1)
19-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
navigator.languagerestoration only preserves thevalue, not the full property descriptor.
afterEachat lines 33-36 does:Object.defineProperty(globalThis.navigator, "language", { configurable: true, value: previousLanguage, });This only captured
.language's value in the earlierbeforeEach(line 23:previousLanguage = (globalThis.navigator as ...)?.language;), notObject.getOwnPropertyDescriptor(...). If the originallanguageproperty was an accessor (getter) or had differentwritable/enumerableattributes (e.g. happy-dom/Bun's own navigator implementation, or a prior test's monkey-patch), thisdefinePropertycall permanently replaces it with a plain writable/enumerable data property, silently changing behavior for any later test in the same process that readsnavigator.language.This is the exact same class of bug called out in the PR notes as a required pre-merge fix ("Restore the complete
navigator.languageproperty descriptor in the helper test"), and the correct pattern already exists in this very file at lines 73-75 / 133-137 (Object.getOwnPropertyDescriptor+ full restore viaObject.defineProperty(globalThis, key, descriptor)ordelete) fordocument/window/localStorage/IS_REACT_ACT_ENVIRONMENT.navigator.languageshould follow the same pattern for consistency and safety.🔧 Suggested fix
-let previousLanguage: unknown; +let previousLanguageDescriptor: PropertyDescriptor | undefined; const originalFetch = globalThis.fetch; beforeEach(() => { - previousLanguage = (globalThis.navigator as { language?: unknown } | undefined)?.language; + previousLanguageDescriptor = Object.getOwnPropertyDescriptor(globalThis.navigator, "language"); ... }); afterEach(() => { globalThis.fetch = originalFetch; clearModelCache(); - Object.defineProperty(globalThis.navigator, "language", { - configurable: true, - value: previousLanguage, - }); + if (previousLanguageDescriptor) { + Object.defineProperty(globalThis.navigator, "language", previousLanguageDescriptor); + } });🤖 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/models-empty-provider.test.tsx` around lines 19 - 37, Update the navigator.language setup in beforeEach/afterEach to capture and restore the complete property descriptor using Object.getOwnPropertyDescriptor and the existing descriptor-restoration pattern in this file, deleting the property when no original descriptor exists. Preserve the test’s en-US override while ensuring accessor, writable, configurable, and enumerable attributes are restored exactly.
🤖 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/i18n/ko.ts`:
- Around line 748-750: Update the Korean translation value for
claude.smallFastModelNativeWarning to translate “helper-model overrides”
directly, removing the reference to an environment variable while preserving the
warning about native Sonnet usage and provider charges.
In `@src/codex/catalog/provider-fetch.ts`:
- Line 254: Clear the provider discovery status before returning configured from
both credential-skip paths: the Cursor and OAuth branches that return when
!apiKey. Reuse clearProviderDiscoveryStatus(name), matching the existing
liveModels === false handling, so stale discovery results are removed whenever
live discovery is skipped due to missing credentials.
---
Outside diff comments:
In `@gui/tests/models-empty-provider.test.tsx`:
- Around line 19-37: Update the navigator.language setup in beforeEach/afterEach
to capture and restore the complete property descriptor using
Object.getOwnPropertyDescriptor and the existing descriptor-restoration pattern
in this file, deleting the property when no original descriptor exists. Preserve
the test’s en-US override while ensuring accessor, writable, configurable, and
enumerable attributes are restored exactly.
🪄 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: 9270e9a4-6f74-4f04-b8ee-0c1b5a622818
📒 Files selected for processing (23)
docs-site/src/content/docs/guides/claude-code.mddocs-site/src/content/docs/ja/guides/claude-code.mddocs-site/src/content/docs/ko/guides/claude-code.mddocs-site/src/content/docs/ru/guides/claude-code.mddocs-site/src/content/docs/zh-cn/guides/claude-code.mdgui/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/models-groups.tsgui/src/pages/ClaudeCode.tsxgui/src/pages/Models.tsxgui/tests/claude-code-background-helper.test.tsxgui/tests/models-empty-provider.test.tsxsrc/codex/catalog/provider-fetch.tssrc/codex/model-cache.tssrc/server/management/provider-routes.tstests/codex-catalog.test.tstests/cursor-hardening.test.tstests/management-provider-validation.test.tstests/models-page-groups.test.ts
| "claude.smallFastModelAccurateHint": "Claude Code가 대화 요약, 주제 감지 같은 백그라운드 작업에 쓰는 모델입니다. 서브에이전트의 haiku 별칭도 이 모델을 사용합니다.", | ||
| "claude.smallFastModelUnsetOption": "Claude Code가 선택(네이티브 모델)", | ||
| "claude.smallFastModelNativeWarning": "비워 두면 OpenCodex가 보조 모델 환경 변수를 설정하지 않습니다. Claude Code가 네이티브 Sonnet 모델을 사용할 수 있으며, 네이티브 프로바이더 요금이 발생할 수 있습니다.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mistranslation: "환경 변수" (environment variable) isn't in the English source and misrepresents the mechanism.
Line 750: "claude.smallFastModelNativeWarning": "비워 두면 OpenCodex가 보조 모델 환경 변수를 설정하지 않습니다. ..." — this says OpenCodex "does not set the helper-model environment variable". The English source (en.ts line 1035) says only "When unset, OpenCodex leaves the helper-model overrides unset." — no mention of an environment variable. de.ts ("setzt OpenCodex keine Hilfsmodell-Overrides") and ja.ts ("ヘルパーモデルの上書きを設定しません") both correctly translate "overrides", not "environment variable". A Korean-reading user could conclude they need to set some env var manually to fix this, which isn't how the GUI setting works.
💬 Suggested fix
- "claude.smallFastModelNativeWarning": "비워 두면 OpenCodex가 보조 모델 환경 변수를 설정하지 않습니다. Claude Code가 네이티브 Sonnet 모델을 사용할 수 있으며, 네이티브 프로바이더 요금이 발생할 수 있습니다.",
+ "claude.smallFastModelNativeWarning": "비워 두면 OpenCodex가 보조 모델 오버라이드를 설정하지 않습니다. Claude Code가 네이티브 Sonnet 모델을 사용할 수 있으며, 네이티브 프로바이더 요금이 발생할 수 있습니다.",📝 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.
| "claude.smallFastModelAccurateHint": "Claude Code가 대화 요약, 주제 감지 같은 백그라운드 작업에 쓰는 모델입니다. 서브에이전트의 haiku 별칭도 이 모델을 사용합니다.", | |
| "claude.smallFastModelUnsetOption": "Claude Code가 선택(네이티브 모델)", | |
| "claude.smallFastModelNativeWarning": "비워 두면 OpenCodex가 보조 모델 환경 변수를 설정하지 않습니다. Claude Code가 네이티브 Sonnet 모델을 사용할 수 있으며, 네이티브 프로바이더 요금이 발생할 수 있습니다.", | |
| "claude.smallFastModelAccurateHint": "Claude Code가 대화 요약, 주제 감지 같은 백그라운드 작업에 쓰는 모델입니다. 서브에이전트의 haiku 별칭도 이 모델을 사용합니다.", | |
| "claude.smallFastModelUnsetOption": "Claude Code가 선택(네이티브 모델)", | |
| "claude.smallFastModelNativeWarning": "비워 두면 OpenCodex가 보조 모델 오버라이드를 설정하지 않습니다. Claude Code가 네이티브 Sonnet 모델을 사용할 수 있으며, 네이티브 프로바이더 요금이 발생할 수 있습니다.", |
🤖 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/i18n/ko.ts` around lines 748 - 750, Update the Korean translation
value for claude.smallFastModelNativeWarning to translate “helper-model
overrides” directly, removing the reference to an environment variable while
preserving the warning about native Sonnet usage and provider charges.
| clearProviderDiscoveryStatus(name); | ||
| return configured; | ||
| } | ||
| if (!apiKey) return configured; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale discovery status leaks through the !apiKey skip paths.
Both the Cursor !apiKey early-return (line 254) and the OAuth !apiKey early-return (lines 281-286) skip the live discovery attempt entirely but never touch discoveryStatus. Contrast this with the liveModels === false branches at lines 250-253 and 288, which correctly call clearProviderDiscoveryStatus(name) before returning configured.
Failure mode: if a provider previously discovered successfully ({status:"ok"}) or failed with some HTTP/network reason, and the credential later goes missing (OAuth logout, Cursor token revoked), GET /api/providers keeps reporting that stale status indefinitely — even though no live attempt is being made this round. The Models page would then show either a misleadingly "healthy" badge for a provider that isn't actually discovering, or a stale/irrelevant failure reason (e.g. an old "HTTP 500" from months ago) instead of reflecting the current "not logged in" state. This directly conflicts with the PR's goal of surfacing discovery status accurately.
🩹 Proposed fix — clear stale status on the credential-skip paths too
- if (!apiKey) return configured;
+ if (!apiKey) {
+ clearProviderDiscoveryStatus(name);
+ return configured;
+ } if (prov.authMode === "oauth" && !apiKey) {
// No usable token (logged out, or account marked needsReauth). Still surface the
// configured static catalog so the GUI Models tab / rail counts are not empty —
// matching Cursor's !apiKey → configured degradation and fetch-failure fallback.
+ clearProviderDiscoveryStatus(name);
return configured;
}Also applies to: 281-286
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 254, Clear the provider
discovery status before returning configured from both credential-skip paths:
the Cursor and OAuth branches that return when !apiKey. Reuse
clearProviderDiscoveryStatus(name), matching the existing liveModels === false
handling, so stale discovery results are removed whenever live discovery is
skipped due to missing credentials.
3 participants
Summary
Closes the remaining UX gaps from #329 and #331:
ok/failedwith reasonhttp+status,blocked,invalid_response,network,provider), exposed additively onGET /api/providers. The Models page renders aDiscovery failedbadge with a localized reason (e.g.Model discovery failed (HTTP 401)) and a settings link next to the existing empty-discovery guidance. Successful discovery clears the status.Design notes
reason: "http"carries a requiredhttpStatus; other reasons cannot carry one (invalid states unrepresentable)./api/providersis read after/api/modelsto avoid a badge flash race.models.discoveryFailed*/claude.smallFastModel*locale keys added across the six dictionaries; nocodexAuth.*keys, nostyles.csschanges (existing badge/warning classes reused).Tests + rendered QA
Named activation tests for all five failure reasons + status-reset + additive/no-attempt path + helper copy (server: +229 lines, gui: +322 lines). Rendered QA at 1280×720 against a live isolated proxy: badge/reason/settings-link visible,
/api/models→/api/providersorder confirmed, zero console errors.Docs:
claude-code.mdguide updated in five locales.Design/audit trail:
devlog/_plan/260724_bugfix_train/040_ux_discovery_badge.md.Closes #329. Closes #331.
Summary by CodeRabbit