fix(models): surface discovery failure status when /v1/models returns 401 (#329)#354
fix(models): surface discovery failure status when /v1/models returns 401 (#329)#354stantheman0128 wants to merge 1 commit into
Conversation
Keep providers visible after /v1/models auth failures and expose the last discovery outcome (HTTP 401/403, network, malformed) so the Models tab can badge the group and guide manual custom-model entry (lidge-jun#329). Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughLive provider model discovery now records categorized outcomes and fallback details, exposes them through ChangesProvider discovery status
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelsPage
participant ProvidersAPI
participant ProviderModelFetcher
participant ModelCache
ModelsPage->>ProvidersAPI: load provider summaries
ProviderModelFetcher->>ModelCache: record discovery outcome
ProvidersAPI->>ModelCache: read public discovery status
ModelCache-->>ProvidersAPI: status, fallback, and detail
ProvidersAPI-->>ModelsPage: provider with discovery metadata
ModelsPage->>ModelsPage: render badge or empty-state message
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: 9f312d1f9e
ℹ️ 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".
| codexAccountMode: providerCodexAccountMode(name, p), | ||
| // Last live /models outcome (null until the first attempt). Keeps HTTP 401 / network | ||
| // failures visible on the Models page without re-fetching during cooldown (#329). | ||
| discovery: publicModelsDiscoveryStatus(name), |
There was a problem hiding this comment.
When the Models page first loads a provider with a slow or failing live /models endpoint, it requests /api/models and /api/providers in parallel; this line snapshots the in-memory status before /api/models has finished gatherRoutedModels() and called setModelsDiscoveryStatus, so the initial provider payload can contain discovery: null and the new HTTP 401/network badge is missing until the 10s poll or a manual refresh. Consider having the GUI fetch providers after the models request completes, or make this endpoint await/trigger the same catalog poll before returning discovery.
Useful? React with 👍 / 👎.
| export function publicModelsDiscoveryStatus(provider: string): ProviderDiscoveryStatus | null { | ||
| return getModelsDiscoveryStatus(provider) ?? null; |
There was a problem hiding this comment.
This public status is keyed only by provider name, but the OAuth credential-change paths (/api/oauth/logout, /api/oauth/accounts/active, and account removal/reauth) clear login/quota state without calling clearModelCache, and fetchProviderModels returns early for an OAuth provider with no token without overwriting discovery. After a recorded 401/network failure, logging out or switching/reauthing the account can therefore leave /api/providers returning the old badge until a later successful live discovery or restart; with no token it can persist indefinitely. Clear the model cache/discovery status in those OAuth paths before exposing this DTO.
Useful? React with 👍 / 👎.
| setModelsDiscoveryStatus(name, { | ||
| ok: false, | ||
| kind: "network", | ||
| fallback, | ||
| detail: liveResult.error, |
There was a problem hiding this comment.
fetchCursorUsableModels already distinguishes auth, http, timeout/transport, decode, and empty, but this new status maps every non-ok Cursor result to kind: "network". For an expired Cursor key (401/403) or a malformed RPC response, the Models page will show “Discovery failed (network)” instead of an auth/HTTP or invalid-response reason, sending users toward the wrong fix. Map Cursor auth/http/decode outcomes to the matching discovery kind/detail instead of hard-coding network.
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: 3
🤖 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 `@docs-site/src/content/docs/guides/web-dashboard.md`:
- Line 39: Update the Models guide rows in the Korean, Russian, and Simplified
Chinese web-dashboard documents to mirror the current English row, including
zero-model groups, the amber discovery-failure badge, and the retained “+”
custom-model action. Preserve each locale’s existing table and translation
formatting while replacing only the outdated Models copy.
In `@src/codex/model-cache.ts`:
- Around line 44-52: Enforce the secret-free contract for
ProviderDiscoveryStatus.detail at the setModelsDiscoveryStatus boundary before
storing or exposing it through
getModelsDiscoveryStatus/publicModelsDiscoveryStatus. Sanitize or reject
arbitrary diagnostic text, covering common credential and sensitive-response
patterns such as Bearer tokens, sk- keys, Authorization headers, raw error
details, and response snippets, while preserving the existing length limit and
public API behavior for approved diagnostics.
In `@tests/provider-discovery-status.test.ts`:
- Around line 21-31: Update the shared configuration helper in
provider-discovery-status tests to include the required OcxConfig fields port
and defaultProvider, then reuse that helper for each gatherRoutedModels setup at
the referenced test sections instead of repeating incomplete configuration
objects.
🪄 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: 16944692-c20d-4137-902c-dd70704a44e8
📒 Files selected for processing (16)
docs-site/src/content/docs/guides/web-dashboard.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/Models.tsxgui/tests/models-empty-provider.test.tsxsrc/codex/catalog/provider-fetch.tssrc/codex/model-cache.tssrc/server/management/provider-routes.tstests/models-page-groups.test.tstests/provider-discovery-status-api.test.tstests/provider-discovery-status.test.ts
| | **Codex Auth** | Add ChatGPT/Codex pool accounts, select the next-session account, refresh 5h / weekly / 30d quotas, and configure quota auto-switch and transient-failure failover. | | ||
| | **Subagents** | Feature up to five bare native or namespaced routed models in the `spawn_agent` override list. | | ||
| | **Models** | Toggle native GPT and routed models, set provider allowlists and context caps, choose v1/base/v2, and configure the v2 thread limit. Configured providers stay visible as zero-model groups when discovery is off or returns no rows. | | ||
| | **Models** | Toggle native GPT and routed models, set provider allowlists and context caps, choose v1/base/v2, and configure the v2 thread limit. Configured providers stay visible as zero-model groups when discovery is off or returns no rows. Failed live discovery (for example HTTP 401 on `/v1/models`) shows an amber status badge and keeps the provider's `+` custom-model action available. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'web-dashboard.md' docs-site/src/content/docsRepository: lidge-jun/opencodex
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/guides/web-dashboard.md \
docs-site/src/content/docs/ja/guides/web-dashboard.md \
docs-site/src/content/docs/ko/guides/web-dashboard.md \
docs-site/src/content/docs/ru/guides/web-dashboard.md \
docs-site/src/content/docs/zh-cn/guides/web-dashboard.md
do
echo "==== $f ===="
wc -l "$f"
# show the Models row area if present
rg -n -C 2 'Models|zero-model|custom-model|discovery|v1/models|amber|badge' "$f" || true
echo
doneRepository: lidge-jun/opencodex
Length of output: 4322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/guides/web-dashboard.md \
docs-site/src/content/docs/ja/guides/web-dashboard.md \
docs-site/src/content/docs/ko/guides/web-dashboard.md \
docs-site/src/content/docs/ru/guides/web-dashboard.md \
docs-site/src/content/docs/zh-cn/guides/web-dashboard.md
do
echo "==== $f ===="
wc -l "$f"
rg -n -C 3 'Models|zero-model|custom-model|discovery|v1/models|amber|badge' "$f" || true
echo
doneRepository: lidge-jun/opencodex
Length of output: 5234
🏁 Script executed:
#!/bin/bash
pwdRepository: lidge-jun/opencodex
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/ja/guides/web-dashboard.md \
docs-site/src/content/docs/ko/guides/web-dashboard.md \
docs-site/src/content/docs/ru/guides/web-dashboard.md \
docs-site/src/content/docs/zh-cn/guides/web-dashboard.md
do
echo "==== $f ===="
nl -ba "$f" | sed -n '30,45p'
echo
doneRepository: lidge-jun/opencodex
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=docs-site/src/content/docs/ja/guides/web-dashboard.md
echo "==== $f ===="
wc -l "$f"
nl -ba "$f" | sed -n '30,45p'Repository: lidge-jun/opencodex
Length of output: 319
Update the translated web-dashboard guide rows
docs-site/src/content/docs/ko/guides/web-dashboard.md:39, docs-site/src/content/docs/ru/guides/web-dashboard.md:39, and docs-site/src/content/docs/zh-cn/guides/web-dashboard.md:38 still use the older Models copy. Mirror the docs-site/src/content/docs/guides/web-dashboard.md:39 text so the locale docs also mention zero-model groups, the amber discovery-failure badge, and the retained + custom-model action.
🤖 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 `@docs-site/src/content/docs/guides/web-dashboard.md` at line 39, Update the
Models guide rows in the Korean, Russian, and Simplified Chinese web-dashboard
documents to mirror the current English row, including zero-model groups, the
amber discovery-failure badge, and the retained “+” custom-model action.
Preserve each locale’s existing table and translation formatting while replacing
only the outdated Models copy.
Source: Path instructions
| export interface ProviderDiscoveryStatus { | ||
| ok: boolean; | ||
| kind: ProviderDiscoveryKind; | ||
| at: number; | ||
| httpStatus?: number; | ||
| fallback?: ProviderDiscoveryFallback; | ||
| /** Short, secret-free diagnostic (error name or policy reason). */ | ||
| detail?: string; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
detail is serialized into a public API response with no sanitization guardrail.
ProviderDiscoveryStatus.detail is documented as "Short, secret-free diagnostic," but setModelsDiscoveryStatus (Line 69-82) only truncates it to 160 chars — it never validates/redacts content. This value flows unmodified through getModelsDiscoveryStatus/publicModelsDiscoveryStatus (Line 84-91) straight into the /api/providers GET response (discovery: publicModelsDiscoveryStatus(name) in provider-routes.ts). If any future caller passes a raw error message, response body snippet, or header value as detail, it would be exposed to every GUI client hitting that endpoint.
Consider enforcing the "secret-free" contract at the boundary rather than relying only on caller discipline, e.g. a small allowlist/regex strip for common secret shapes (Bearer , sk-, Authorization:), or restricting detail to a closed set of known diagnostic strings instead of arbitrary text.
🛡️ Example guard
+const SECRET_PATTERN = /(bearer\s+\S+|sk-[a-zA-Z0-9]{10,}|authorization\s*:\s*\S+)/i;
+
export function setModelsDiscoveryStatus(
provider: string,
status: Omit<ProviderDiscoveryStatus, "at"> & { at?: number },
): void {
const next: ProviderDiscoveryStatus = {
ok: status.ok,
kind: status.kind,
at: status.at ?? Date.now(),
};
if (typeof status.httpStatus === "number") next.httpStatus = status.httpStatus;
if (status.fallback) next.fallback = status.fallback;
- if (status.detail) next.detail = status.detail.slice(0, 160);
+ if (status.detail) next.detail = status.detail.slice(0, 160).replace(SECRET_PATTERN, "[redacted]");
discoveryStatus.set(provider, next);
}Based on path instructions for src/**: "Watch for credential handling: tokens and OAuth material must never be logged or serialized into responses."
Also applies to: 69-82, 88-91
🤖 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/model-cache.ts` around lines 44 - 52, Enforce the secret-free
contract for ProviderDiscoveryStatus.detail at the setModelsDiscoveryStatus
boundary before storing or exposing it through
getModelsDiscoveryStatus/publicModelsDiscoveryStatus. Sanitize or reject
arbitrary diagnostic text, covering common credential and sensitive-response
patterns such as Bearer tokens, sk- keys, Authorization headers, raw error
details, and response snippets, while preserving the existing length limit and
public API behavior for approved diagnostics.
Source: Path instructions
| const models = await gatherRoutedModels({ | ||
| providers: { | ||
| [provider]: { | ||
| baseUrl: "https://example.invalid/api/plan", | ||
| adapter: "anthropic", | ||
| authMode: "key", | ||
| apiKey: "plan-key", | ||
| defaultModel: "glm-5-2", | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'interface OcxConfig {
$$$
}' --lang typescript src | head -50
rg -n 'interface OcxConfig' -A 30 srcRepository: lidge-jun/opencodex
Length of output: 5498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tests/provider-discovery-status.test.ts ==\n'
cat -n tests/provider-discovery-status.test.ts | sed -n '1,260p'
printf '\n== gatherRoutedModels signature ==\n'
rg -n 'function gatherRoutedModels|const gatherRoutedModels|export .*gatherRoutedModels' src tests -A 8 -B 4
printf '\n== nearby OcxConfig construction patterns ==\n'
rg -n '\bdefaultProvider\b|\bport\b' tests src -A 2 -B 2 | sed -n '1,220p'Repository: lidge-jun/opencodex
Length of output: 25391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the OcxConfig definition and any helper/factory functions that may supply defaults.
sed -n '422,490p' src/types.ts | cat -n
printf '\n== config factories ==\n'
rg -n 'defaultProvider:|port:|providers:' src tests -A 2 -B 2 | sed -n '1,260p'Repository: lidge-jun/opencodex
Length of output: 21284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether this test file is the only place using a bare { providers } literal.
rg -n 'gatherRoutedModels\(\{\s*providers:' tests src -U -A 8 -B 2Repository: lidge-jun/opencodex
Length of output: 24266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve the exact gatherRoutedModels parameter type and confirm whether this test file is passing a bare OcxConfig literal.
rg -n 'gatherRoutedModels' src tests -A 6 -B 4Repository: lidge-jun/opencodex
Length of output: 50375
Add the missing OcxConfig fields
tests/provider-discovery-status.test.ts passes a bare { providers: { ... } } object into gatherRoutedModels, but src/types.ts:422-425 requires port and defaultProvider too. This will fail tsc in CI. Add those fields once in a shared helper and reuse it at lines 21-31, 50-60, 80-90, 111-120, 148-158, and 194-205.
🤖 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-discovery-status.test.ts` around lines 21 - 31, Update the
shared configuration helper in provider-discovery-status tests to include the
required OcxConfig fields port and defaultProvider, then reuse that helper for
each gatherRoutedModels setup at the referenced test sections instead of
repeating incomplete configuration objects.
|
Thanks @Wibias / @lidge-jun. Agree with landing #329 via #358 to avoid the double-merge conflict. Glad the discovery-failure badge / status surface is useful; happy to see the skipped + docs follow-ups folded there with credit. Closing in favor of #358 is fine on our side. |
2 participants
Summary
GET /v1/modelsfails (for example HTTP 401 on Volcengine Agent Plan keys that can still call inference), the Models page now keeps the provider group and shows an amber discovery badge with the concrete reason (Discovery failed (HTTP 401), network, blocked, or invalid response).GET /api/providersasdiscovery, so the GUI can badge without re-fetching during cooldown.kind=empty, no failure badge), and clearing status on cache reset after an auth/config change. Custom-model+remains available on zero-row groups (from fix(models): preserve empty providers and discovery controls #313).AI assistance
Drafted and verified with Cursor/Grok assistance. Human (Stan Shih / stantheman0128) reviewed the approach, Evidence, and PR text before opening.
Verification
Evidence (product path)
Started a loopback upstream that returns HTTP 401 on
/modelswhile accepting POSTs, configured an anthropic-compatibleagent-planprovider withdefaultModel: glm-5-2and no staticmodels[], then hit the same management endpoints the Models page uses:After
clearModelCache("agent-plan")(same path provider PATCH / key updates use),discoveryisnullagain so a retry is not stuck on a stale 401 badge.What was not tested
EmptyProviderHintplus grouping/badge unit tests against the same discovery DTO the page consumes.Checklist
Summary by CodeRabbit
New Features
Documentation
Tests