Don't discard a valid cloud key when the add-time model probe fails transiently - #5341
Conversation
Adding a cloud key provider wrote the API key first, then probed `/models`; on any probe failure it rolled the key back and threw "Could not reach ...". A momentary probe hiccup (timeout, unreachable host) therefore discarded a perfectly valid key and reported the save as failed. The built-in key dialog has no "add anyway" escape hatch, so the provider was effectively unsaveable — and because the rollback's key-clear was best-effort with a swallowed error, a failed clear left the key orphaned on disk, reading as "not saved" across restarts. Classify the probe failure instead of treating every failure the same: - Auth failure (401 / invalid key): the key itself is wrong, so keep the existing behavior — roll both stores back and reject so the user fixes it. - Non-auth failure (timeout / unreachable / unknown) for a cloud key provider: the key is plausibly valid, so keep it, persist the provider, and surface a non-fatal amber "the key was saved, but the provider was unreachable" advisory rather than blocking the save. - Local runtimes keep the original reject-on-probe behavior (an unreachable runtime is a real setup error and holds no key). Both rollback catches now log instead of swallowing, so a failed key-clear can no longer orphan a key silently.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughProvider verification now distinguishes authentication failures from other failures. Cloud keys and providers persist after non-authentication probe failures with a dismissible advisory. Authentication and local-runtime failures still roll back configuration and credentials. ChangesProvider verification handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsUI
participant AIPanel
participant ProviderAPI
participant SettingsStorage
SettingsUI->>AIPanel: submit provider credentials
AIPanel->>ProviderAPI: probe /models
ProviderAPI-->>AIPanel: verification result
alt non-authentication failure
AIPanel->>SettingsStorage: save provider and key
AIPanel-->>SettingsUI: show saved-but-unreachable advisory
else authentication or local-runtime failure
AIPanel->>SettingsStorage: restore configuration and clear credentials
AIPanel-->>SettingsUI: report setup failure
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 011123c451
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 `@app/src/components/settings/panels/AIPanel.tsx`:
- Around line 3676-3691: Add regression tests for the rollback handling around
flushCloudProviders and clearCloudProviderKey: mock the initial flush as
successful, make rollback flushing and credential cleanup fail, and assert both
failures are warned. Verify the probe still rejects and the warning path does
not persist the provider.
- Around line 3054-3072: Update
app/src/components/settings/panels/AIPanel.tsx:3054-3072 around
classifyProviderVerificationFailure so HTTP 403 and forbidden responses classify
as auth before the non-fatal credential-retention branch. Update
app/src/components/settings/panels/__tests__/AIPanel.test.tsx:62-66 to map 403
and forbidden mock responses to auth, and add a bare-403 regression case at
996-1010 asserting key cleanup and no settings save.
- Around line 3073-3087: Update the failure branch in
openhumanUpdateLocalAiSettings to snapshot and restore the affected local_ai
settings, including endpoint, provider, enablement flags, and OMLX API key, when
listProviderModels fails. Preserve the existing cloud_providers and key rollback
behavior, and add a regression test verifying rejected local probes leave
local_ai unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14c6a848-c103-4e0b-86b8-34bb41f49e0c
📒 Files selected for processing (2)
app/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/__tests__/AIPanel.test.tsx
|
| Filename | Overview |
|---|---|
| app/src/services/api/aiSettingsApi.ts | Adds proxy/gateway early-exit guard and 403-credential heuristic to classifyProviderVerificationFailure; ordering and word-boundary regexes are correct |
| app/src/components/settings/panels/AIPanel.tsx | Introduces slug-keyed providerSaveNotice state, non-fatal probe branch, amber advisory UI, and logged (non-swallowed) rollback handlers; all clear-points are scoped to the correct slug |
| app/src/components/settings/panels/tests/AIPanel.test.tsx | Migrates mock to importOriginal to reuse real classifier/describer, adds five new tests covering non-auth keep, auth reject, 403 Forbidden, rollback-logging (builtin and custom paths) |
| app/src/services/api/tests/aiSettingsApi.test.ts | Adds targeted classification test cases for 403-credential, proxy-WAF, bare 407, 502, and request-ID false-positive scenarios |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[connectProvider called] --> B[setCloudProviderKey]
B --> C[flushCloudProviders with new provider]
C --> D{isCodexOAuth or isCliLogin?}
D -- Yes --> I[persist + close dialog]
D -- No --> E[listProviderModels probe]
E -- success --> I
E -- failure --> F[classifyProviderVerificationFailure]
F --> G{isKeyProvider AND reason != auth?}
G -- Yes non-auth cloud key --> H[setProviderSaveNotice with slug+message]
H --> I
G -- No auth failure or local runtime --> J[flushCloudProviders rollback - logs on error]
J --> K{isKeyProvider?}
K -- Yes --> L[clearCloudProviderKey - logs on error]
K -- No --> M[throw Could not reach ...]
L --> M
Reviews (3): Last reviewed commit: "Harden probe classification and key the ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…logs Review follow-ups on the add-time provider fix: - Classify HTTP 403 / Forbidden as `auth` in `classifyProviderVerificationFailure`. A revoked or permission-denied key surfaces as a bare 403 with no "401"/"unauthorized" text; without this it fell into the non-fatal branch and was kept behind a "key was saved" advisory instead of being rejected. Added classifier assertions and a bare-403 connect regression (key cleared, no save, no advisory). - Remove the provider-toggle-off advisory clears. `setProviderSaveNotice(null)` holds a single unscoped string, so toggling off provider B wiped an advisory that belonged to provider A. The advisory is already dismissible and is reset on the next connect attempt, so the unscoped clear did more harm than good. - Add regression tests for the rollback-failure logging on both the built-in connect path and the custom-provider editor path: with the initial flush succeeding and both rollback legs failing, each failure is warned and the provider is not persisted. - Make the test's `describeProviderVerificationFailure` stub reason-dependent so it reflects the actual code path (auth "rejected it" vs the non-auth "a test call failed" copy) instead of always returning the auth string.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Review
The direction is right. Making the add-time probe non-fatal for cloud key providers, keeping local runtimes reject-on-probe, and un-swallowing the two rollback .catch(() => {}) are all correct. The rejected loadAISettings reconcile is well-reasoned too — toggle-OFF really does leave the identical on-disk shape, so the reconcile could not have distinguished a disconnect from a failed-add orphan.
My concern is that the change makes error classification load-bearing, and the classifier is not precise enough to carry that weight.
1. Major — the new 403 / forbidden rule reopens the reported bug for proxied users
classifyProviderVerificationFailure (app/src/services/api/aiSettingsApi.ts:509) matches bare substrings against the whole error string. Two very common network-side rejections now land in auth and therefore delete a valid key:
407 Proxy Authentication Required→ matches the pre-existingauthenticationterm.- A corporate proxy / Cloudflare / WAF
403 Forbiddensitting in front of the provider → matches the two terms this PR adds.
Neither says anything about the key itself. The PR description lists the triggers it is fixing as "timeout, unreachable host, proxy" — so the classifier now routes one of the named triggers straight into the destructive branch.
Before this PR, misclassification was harmless: every failure rolled back identically. Now reason === 'auth' is the difference between keeping the key and destroying it, so the loose matching has real consequences. Note also that haystack.includes('403') matches 1403, a request id that happens to contain 403, or 4032 bytes.
Codex and CodeRabbit both pushed for 403 → auth, and in isolation that is defensible for a revoked key. The gap is that the false-positive direction was not weighed — and that direction is the bug being fixed.
Suggested narrowing, before the auth branch:
// Gateway/proxy rejections are network-side — the key is untouched.
if (/\b407\b|proxy|cloudflare|bad gateway|gateway timeout/.test(haystack)) return 'unknown';…and require 403 to co-occur with credential wording rather than matching anywhere in the string.
2. Medium — the "local runtimes hold no API key" premise is false for OMLX
The comment at app/src/components/settings/panels/AIPanel.tsx:3057 justifies rejecting local runtimes on the grounds that they hold no key to preserve. OMLX is credentialMode: 'endpoint_key' → isLocalRuntime === true, and it does write a key:
await openhumanUpdateLocalAiSettings({
base_url: endpoint, api_key: trimmed, provider: 'omlx', runtime_enabled: true, opt_in_confirmed: true,
});The else branch rolls back flushCloudProviders only — local_ai is never reverted. So an OMLX add whose probe fails leaves local_ai.api_key plus runtime_enabled: true on disk with no provider entry: exactly the orphan class this PR exists to eliminate, left in place and justified by an incorrect premise.
The behaviour is unchanged from before, so it is not a regression from this PR — but the comment should be corrected either way, and it deserves a follow-up issue.
3. Minor — providerSaveNotice is a single un-keyed string
It is cleared only at the top of connectProvider and by Dismiss. Consequences:
- Toggling the referenced provider OFF (
AIPanel.tsx:3252) removes it without clearing the advisory — that path never callsconnectProvider, so a stale notice keeps naming a provider that no longer exists. Same foronClearKeyat:3709. - Connecting an unrelated provider clears an advisory about a different one.
Storing { slug, message } and rendering/clearing per slug fixes both. (The CodeRabbit release-note bullet "Notices clear when … a provider is removed" is not backed by the code.)
4. Nit — the test mock is a third copy of the auth rule
AIPanel.test.tsx:58 hand-reimplements the classifier regex across two mocks. The new 403 rule already had to be added in two places to keep them in sync. vi.importActual for these two pure functions would remove the drift risk entirely.
Happy to re-review once (1) is addressed — the rest are non-blocking.
Address maintainer review — the change made error classification decide whether a key is kept or deleted, so the classifier has to be precise: - Network/gateway/proxy rejections no longer delete a valid key. A guard ahead of the auth branch routes 407, `proxy`, `cloudflare`, `bad gateway`, and `gateway timeout` to `unknown` (non-destructive). A bare 403 only counts as a rejected credential when it co-occurs with credential wording, and 401/403 now match on word boundaries so a request id like `1403` can't trip them. This closes the reopened data-loss path for corporate-proxy / WAF users (the "proxy" trigger the fix is meant to preserve keys through). - Key the "key saved, but unreachable" advisory by slug. It was a single unscoped string, so connecting one provider or toggling off another wiped an unrelated provider's notice. It is now cleared only for the provider it belongs to, and provider removal / key-clear drops its own advisory. - Correct the local-runtime comment: OMLX (`endpoint_key`) is a local runtime that does write a key (into `local_ai.api_key`), so the "holds no key" justification was wrong. Its `local_ai` write is not rolled back on a failed probe — pre-existing behaviour, noted for follow-up. - Drop the hand-copied classifier from the AIPanel test; it now uses the real `classify`/`describe` via `importOriginal`, removing the drift the 403 change had to update in three places.
|
Thanks — the false-positive direction is the right thing to weigh here, and it's addressed in 1 (Major) — proxy/gateway false positives. Fixed. 2 (Medium) — OMLX premise. Corrected the comment: OMLX ( 3 (Minor) — un-keyed advisory. Now 4 (Nit) — third copy of the classifier. Removed. The AIPanel test now pulls the real All checks green locally (typecheck, lint, 100% diff coverage). Ready for another look. |
3 participants
Summary
/modelsprobe fails for a transient reason.authfailure (401 / invalid key) still rejects and rolls back; a non-auth failure (timeout / unreachable / unknown) keeps the key, persists the provider, and shows a non-fatal advisory..catch(() => {})swallows now log, so a failed key-clear can no longer orphan a key on disk silently.classifyProviderVerificationFailure/describeProviderVerificationFailurecopy and thesettings.ai.providerTest.*i18n family — no new i18n keys.Problem
Closes #5339. When a user pasted a valid DeepSeek key and hit Save,
connectProviderwrote the key (setCloudProviderKey), flushed the provider config, then probedopenhuman.inference_list_models. On any probe failure it rolled the key back and threwCould not reach …, so:/modelshiccup (timeout, unreachable host, proxy) discarded a valid key and reported the save as failed. The built-in key dialog has no "add anyway" escape hatch (the custom-provider editor got one in [Bug] Azure Foundry: "Model not found" when deployment name differs from model name — allow free-text deployment name input #5213), so the provider was effectively unsaveable.clearCloudProviderKey(slug).catch(() => {})swallowed its error. If the clear failed (most likely under the very transient conditions that failed the probe), the key stayed inauth-profiles.jsonwith no config entry — an orphan that reads as "saved but not saved" and survives restarts.Solution
In
connectProvider's probecatch, classify the error with the existingclassifyProviderVerificationFailure:auth(401 / invalid key), or any local runtime: unchanged — roll both stores back and throw so the dialog shows the error and stays open.!isLocalRuntime && slug !== 'openhuman'): do not roll back or throw. Keep the key, fall through topersist, and set a dismissible amber advisory (providerSaveNotice) rendered next to the provider list — "The key was saved, but '' …". The provider is usable once reachable; the user picks a model to route to it.Both rollback
.catchhandlers nowconsole.warninstead of swallowing.Design notes / tradeoffs:
loadAISettingsreconcile that re-materialised an orphaned builtin key on reload. It was dropped: the builtin chip toggle-OFF removes the config entry without clearing the key, so an intentional disconnect leaves the exact same on-disk state as a failed-add orphan — the reconcile could not distinguish them and would silently revert disconnects. Preventing the orphan at the source (this PR) is the correct fix; a pre-existing orphan now self-heals on re-add.Submission Checklist
connectProvidertests inAIPanel.test.tsx: a non-auth probe failure keeps the key + persists + shows the advisory (and is dismissible); anauthprobe failure clears the key + rejects.appVitest suite green (9114 passed).pnpm test:rust—N/A: no Rust changed.N/A: behaviour-only change to an existing feature (provider connect); no feature row added/removed/renamed.## Related—N/A: no matrix feature row affected.N/A: no new network calls; the model-listing probe already existed.N/A: no change to release-cut smoke steps; the provider-connect happy path is unchanged, only the transient-failure branch differs.Closes #NNN— see## Related.Impact
Related
config.cloud_providerswhole-array replace on write (src/openhuman/config/ops/model.rs) is a load-modify-save with no lock; a concurrent eager flush + persist can clobber an entry. Out of scope here (Rust); worth a dedicated fix.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/5339-deepseek-key-save-mismatchValidation Run
pnpm --filter openhuman-app format:check— passpnpm typecheck— passpnpm test src/components/settings/panels/__tests__/AIPanel.test.tsx— 68 pass; fullappunit suite — 9114 pass / 0 failN/A: no Rust changedN/A: no Rust changedValidation Blocked
command:localpre-pushhook (pnpm rust:clippy)error:cargo clippyfails to resolvetinychannelson this host due to pre-existing vendored-submodule pointer drift in the working tree (unrelated to this PR); it also needsGGML_NATIVE=OFFfor whisper-rs/llama.cpp on Apple Silicon. Bypassed with--no-verifyas unrelated pre-existing breakage. Remote CI runs clippy against clean submodules.impact:none on this change — it touches only two TypeScript files (zero Rust);format:check,lint(0 errors),typecheck, andlint:commands-tokensall pass locally.Behavior Changes
Parity Contract
auth(401 / invalid key) failures and all local-runtime probe failures still roll back and reject exactly as before;codex_oauth/cli_loginstill skip the probe.isKeyProvider = !isLocalRuntime && slug !== 'openhuman'mirrors the originalclearCloudProviderKeyguard; theelsebranch reproduces the original rollback + throw.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes