fix(embeddings): differentiate custom endpoint verification failures (#5017)#5064
Conversation
…inyhumansai#5017) The custom OpenAI-compatible embed verification request is well-formed (POST /v1/embeddings, user's model, Bearer key, {"input":[…],"model":…}) and a conformant endpoint verifies. The bug was that classify_embed_probe collapsed distinct causes — auth 401/403, wrong/incompatible model 400, network/DNS, dimension mismatch — into one generic "test embed failed", so a user whose model works for chat (e.g. the chat model gpt-5-mini) could not tell it is not an embeddings model. Differentiate the probe failures into dedicated, actionable codes/messages (EMBEDDINGS_MODEL_INCOMPATIBLE / _AUTH_FAILED / _ENDPOINT_UNREACHABLE / _DIMENSION_MISMATCH), tolerant of both embed-error wire shapes. Frontend now treats any non-wipe error code as a verification failure so new codes surface their message instead of being silently treated as a save. Tests: 4 differentiation cases + a mock /v1/embeddings regression proving a conformant host verifies and the request carries the user's model + key.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 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 (2)
📝 WalkthroughWalkthroughCustom embedding verification now classifies probe failures into specific error codes, validates custom endpoint requests through regression tests, and keeps the setup dialog open with fallback details for non-wipe failures. ChangesEmbeddings verification flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EmbeddingsPanel
participant update_settings
participant CustomEndpoint
User->>EmbeddingsPanel: Save custom embeddings settings
EmbeddingsPanel->>update_settings: Submit settings and verify probe
update_settings->>CustomEndpoint: Send embedding request
CustomEndpoint-->>update_settings: Return embedding or probe error
update_settings-->>EmbeddingsPanel: Return saved result or classified error
EmbeddingsPanel-->>User: Close popup or show fallback error
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: dd1929d678
ℹ️ 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: 2
🧹 Nitpick comments (1)
src/openhuman/embeddings/rpc.rs (1)
779-825: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting probe-classification helpers/tests into their own module.
This PR adds ~150 more lines (helper predicates + tests) to a file that's already well past the guideline's soft limit. Extracting
classify_embed_probe+ its helper predicates (and/or the related test module) into a dedicated file undersrc/openhuman/embeddings/would help keep individual files closer to the target size.As per coding guidelines,
**/*.{ts,tsx,rs}: "Prefer files of approximately 500 lines or fewer."🤖 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/openhuman/embeddings/rpc.rs` around lines 779 - 825, Extract classify_embed_probe and its helper predicates, including embed_error_mentions_status, is_embedding_model_incompatible, is_embedding_dimension_mismatch, and is_embedding_endpoint_unreachable, into a dedicated module under src/openhuman/embeddings/. Move their related tests with the classification code where practical, update visibility/imports and call sites, and keep behavior unchanged while reducing the size of the existing embeddings file.Source: Coding guidelines
🤖 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/EmbeddingsPanel.tsx`:
- Around line 314-326: Update the verification-result flow around the
result.error classification to add namespaced debug logs for the RPC result,
selected error classification, preserved-popup state transition, and early
return. Use only safe metadata and explicitly exclude endpoint, API key, and
backend detail values from every log; cover the external call, relevant
branches, state change, and error path without altering behavior.
In `@src/openhuman/embeddings/rpc.rs`:
- Around line 706-717: Update the remediation message in the
EMBEDDINGS_DIMENSION_MISMATCH reject branch to remove the ineffective “clear the
field to auto-detect” guidance. Direct users to set dimensions to the
endpoint/model’s native output length, preserving the existing mismatch handling
and error code.
---
Nitpick comments:
In `@src/openhuman/embeddings/rpc.rs`:
- Around line 779-825: Extract classify_embed_probe and its helper predicates,
including embed_error_mentions_status, is_embedding_model_incompatible,
is_embedding_dimension_mismatch, and is_embedding_endpoint_unreachable, into a
dedicated module under src/openhuman/embeddings/. Move their related tests with
the classification code where practical, update visibility/imports and call
sites, and keep behavior unchanged while reducing the size of the existing
embeddings file.
🪄 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: 2320eb93-514d-4ee5-bf73-ca78732238b1
📒 Files selected for processing (2)
app/src/components/settings/panels/EmbeddingsPanel.tsxsrc/openhuman/embeddings/rpc.rs
…diagnostics Address review on tinyhumansai#5064 (tinyhumansai#5017): - embed_error_mentions_status now also matches the bare-status 'Embedding API error {code}' shape (no parens), so setup-time custom endpoint verification routes 401/403 to EMBEDDINGS_AUTH_FAILED instead of the generic failure. Covered by an added case in the auth-failure test. - Drop the misleading 'or clear the field to auto-detect' remediation from the EMBEDDINGS_DIMENSION_MISMATCH message (clearing falls back to the stored value, not an auto sentinel, for text-embedding-3-* — a no-op). - Add namespaced 'app:settings:embeddings' debug diagnostics around the custom-endpoint verification-classification flow (rpc result, error code, popup-preserving state transition, early return); no endpoint/key/detail values are logged.
|
Addressed the review feedback in eb8f6c3:
Targeted validation: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…5017-custom-embed-verify-errors
|
@coderabbitai review |
✅ Action performedReview finished.
|
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Review — request changes
Thanks for the thorough investigation write-up; ruling out the reporter's five guesses with a captured request rather than prose is the right way to close #5017. The direction is correct and the policy stays a pure, unit-testable function over EmbedProbe. Four things should land before merge.
Answering the three questions this change lives or dies on:
- Are the branches exhaustive and mutually exclusive? Within
classify_embed_probe, yes — theif / else ifchain is exclusive by construction and specific-before-generic ordering is correct. But the caller undoes it:update_settingsunconditionally overrides any reject with/modelsname-mismatch guidance, so a high-confidenceEMBEDDINGS_AUTH_FAILEDcan be reported as a model-name problem. See the inline comment onrpc.rs. - Does any user-facing message leak a key or endpoint credential? No. All four new
messagestrings are static literals with no interpolation, and the newtracing::warn!(reject_code)logs only the sentinel — the security claim in the PR body holds up. (Pre-existing, not this PR: thedetailpassthrough rendered by the panel can carry the endpoint URL out of a reqwest error. Fine for the user's own screen; worth remembering ifdetailever reaches telemetry.) - Is a genuinely unknown failure still surfaced? Yes. The terminal
elsekeepsEMBEDDINGS_VERIFICATION_FAILEDwithdetail, empty vectors still reject, and the frontend shape match is strictly more conservative than the old allow-list. Nothing is swallowed into a success.classify_embed_probe_rejects_unclassified_5xx_genericallyguards exactly this.
i18n: clean. No new frontend literals, and settings.embeddings.verifyFallback is present in all 14 locale files (verified). One caveat under Minor below.
Verification note: I could not run cargo test — a fresh worktree is missing the vendored tinyagents submodule (vendor/tinyagents/Cargo.toml absent), so the build fails before compiling. The Rust review is static. This is also why finding 4 matters.
Blocking
rpc.rs— the/modelsoverride clobbers the new codes. Inline.EmbeddingsPanel.tsx— the behavior change has no test. Inline. This is the latent bug the frontend half exists to fix, and every existing failure-path test uses one of the three previously allow-listed codes, so all of them pass identically againstmain. The checklist claim that it is "covered by the existing panel behavior" is not accurate — coverage counts the line, but no test fails if someone reverts to an allow-list.rpc.rs—is_embedding_model_incompatibleduplicatesobservability::is_embedding_model_rejectedand drifts from it. Inline.rpc.rs— the classification tests are tautological against the real wire format. Inline.
Minor / non-blocking
- Folding
TimedOutintoEMBEDDINGS_ENDPOINT_UNREACHABLEloses a distinction this PR exists to create. Inline. "does not exist"misdiagnoses a typo as a chat model. Inline.- The four new backend messages are permanently untranslated.
AGENTS.mdrequires UI text throughuseT(); these bypass it as backend-emitted strings — an established pattern, and you're upfront about it. But it takes non-English users from 3 untranslated messages to 7, and sinceresult.messageis always present on these paths the translatedverifyFallbackis now effectively dead for custom-endpoint failures. The codes are stable sentinels, which is exactly what a lookup wants:
const CODE_KEYS: Record<string, string> = {
EMBEDDINGS_AUTH_FAILED: 'settings.embeddings.errAuthFailed',
EMBEDDINGS_MODEL_INCOMPATIBLE: 'settings.embeddings.errModelIncompatible',
EMBEDDINGS_ENDPOINT_UNREACHABLE: 'settings.embeddings.errUnreachable',
EMBEDDINGS_DIMENSION_MISMATCH: 'settings.embeddings.errDimensionMismatch',
};
const key = CODE_KEYS[result.error];
const baseMessage = key
? t(key)
: typeof result.message === 'string'
? result.message
: t('settings.embeddings.verifyFallback');Reasonable to defer to a follow-up — but then say so in the PR body rather than framing "no new i18n keys" as a design win.
Nitpicks
is_embedding_endpoint_unreachablemixes&&and||unparenthesized. It parses as intended ((a && b) || c || …) but reads as ambiguous — wrap the first clause. Inline comment has more.embed_error_mentions_status:contains(&format!("({code}"))has no digit boundary, so(400matches(4001. Contrived in practice, but(401)/(401as the pattern costs nothing.classify_embed_probe_distinguishes_incompatible_modeldoc comment reads "Issue #5017 — the #5017 reporter's exact case"; drop the duplicate reference.EmbeddingsPanel.tsxlogs the same error code twice ~30 lines apart on one code path; the second adds only "preserving setup popup". Consider folding into the first.
Questions
- Is the
/modelsoverride intentional for the new codes, or just not revisited? It's the difference between fully meeting the issue's acceptance criteria and meeting them except behind a public/models. - Were the six wire-shape strings verified against the current vendored
tinyagentssource, or inferred? I couldn't check (submodule absent). If verified, pinning the tinyagents commit in a comment would make finding 4 much less pressing.
Looks good
- Policy stays a pure function over a normalized
EmbedProbe, so all of it is testable without a network — consistent with how #3761/#4056 built it. is_embedding_model_incompatiblegates on 400/422 plus a model-rejection phrase rather than status alone, so a bare 400 (oversized input) still falls through. Matches the polarity contract the observability module documents.- The mock-endpoint regression test asserting the captured
authorizationheader and request body is the right way to settle "is the request correct". - No credential or endpoint content in any new log line or user-facing message.
- i18n parity intact; no new keys needed.
- Frontend allow-list to shape-match is the correct direction (fail closed on unknown codes).
| .value | ||
| .get("error") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("EMBEDDINGS_VERIFICATION_FAILED"); |
There was a problem hiding this comment.
blocker — the /models guidance override clobbers the new high-confidence codes
Just below this, on any reject, update_settings calls fetch_served_model_ids and, if the requested model isn't in the served list, return Ok(better) — replacing the classified code with EMBEDDINGS_NO_MODEL_LOADED.
That override predates this PR and was fine when everything below it was one generic code. It isn't now. An endpoint with a public /models but an authenticated /embeddings (common behind gateways/proxies) will classify a 401 as EMBEDDINGS_AUTH_FAILED, then have it overwritten with "your model isn't served" — telling the user to fix the model name when the real problem is the key. EMBEDDINGS_DIMENSION_MISMATCH has the same exposure when the served list canonicalizes ids differently.
EMBEDDINGS_ENDPOINT_UNREACHABLE is mostly self-protecting (the /models fetch fails too), so this is really about auth and dimension.
The name-mismatch heuristic is only a better diagnosis than the generic bucket. Gate it on that:
// before
let listed_endpoint = custom_endpoint
.as_deref()
.or_else(|| effective_provider.strip_prefix("custom:"));
if let Some(ep) = listed_endpoint {
// after
// The `/models` name-mismatch heuristic (issue #3761) is only a *better*
// diagnosis than the generic failure. Auth, reachability and dimension
// failures are already high-confidence and specific, so a served-list miss
// must not overwrite them (#5017).
let heuristic_may_override = matches!(
reject_code,
"EMBEDDINGS_VERIFICATION_FAILED" | "EMBEDDINGS_ENDPOINT_NO_API"
);
let listed_endpoint = custom_endpoint
.as_deref()
.or_else(|| effective_provider.strip_prefix("custom:"))
.filter(|_| heuristic_may_override);
if let Some(ep) = listed_endpoint {Worth a test asserting an EMBEDDINGS_AUTH_FAILED survives a served-list miss.
| || lower.contains("is not an embedding") | ||
| || lower.contains("does not exist") | ||
| || lower.contains("not supported for embeddings") | ||
| || lower.contains("unexpected model name format")) |
There was a problem hiding this comment.
blocker — duplicates observability::is_embedding_model_rejected, and drifts from it
src/core/observability.rs already owns a predicate for exactly these wire shapes, documented against real Sentry volume (TAURI-RUST-9SK, 4SA). This file establishes the reuse precedent one branch above — classify_embed_probe already calls crate::core::observability::is_embedding_endpoint_absent.
This copy diverges in both directions: it adds 422 and three phrases the observability one lacks, and it omits the Gemini OpenAI-compat arm (invalid_argument + batchembedcontentsrequest.model). Net effect: a Gemini-compat host emitting INVALID_ARGUMENT without the literal "unexpected model name format" is correctly demoted at runtime but falls to the generic bucket at setup — the exact class of confusion #5017 is about.
Make the existing predicate pub(crate) and delegate, extending it with what you need here:
// src/core/observability.rs
-fn is_embedding_model_rejected(lower: &str) -> bool {
- lower.contains("embedding api error")
- && lower.contains("(400")
+pub(crate) fn is_embedding_model_rejected(lower: &str) -> bool {
+ // Tolerant of all three emitted status shapes — setup-time verification
+ // also sees `openai embeddings returned HTTP <code>` (#5017).
+ let bad_request = mentions_status(lower, 400) || mentions_status(lower, 422);
+ bad_request
&& (lower.contains("does not exist")
|| lower.contains("does not support embeddings")
+ || lower.contains("not an embedding model")
+ || lower.contains("is not an embedding")
+ || lower.contains("not supported for embeddings")
|| lower.contains("unexpected model name format")
|| (lower.contains("invalid_argument")
&& lower.contains("batchembedcontentsrequest.model")))
}then delete is_embedding_model_incompatible and call it:
-} else if is_embedding_model_incompatible(&lower) {
+} else if crate::core::observability::is_embedding_model_rejected(&lower) {If you'd rather not touch the observability module in a fix PR, at minimum add the missing Gemini arm here and leave a // keep in sync with comment on both — but one predicate is the right answer.
| || lower.contains("error trying to connect") | ||
| || lower.contains("dns error") | ||
| || lower.contains("failed to lookup address") | ||
| || lower.contains("tcp connect error") |
There was a problem hiding this comment.
nitpick (x2)
Precedence: this mixes && and || unparenthesized. It parses as intended — (a && b) || c || … — but reads as ambiguous:
- lower.contains("request to") && lower.contains("failed")
+ (lower.contains("request to") && lower.contains("failed"))
|| lower.contains("connection refused")Breadth: contains("request to") && contains("failed") is loose enough to catch a 502/503 whose body says "the request to the upstream failed", classifying a reachable host as unreachable and sending the user off to check their base URL. Anchoring on the adapter's actual prefix would tighten it:
lower.contains("embeddings request to")| "That model isn't an embeddings model on this endpoint. A chat model \ | ||
| (the one that works in Chat settings) can't produce embeddings — \ | ||
| enter an embeddings model id (e.g. text-embedding-3-small, bge-m3), \ | ||
| then save again.", |
There was a problem hiding this comment.
minor — "does not exist" misdiagnoses a typo as a chat model
A 400 Model text-embedding-3-smal does not exist is a typo, not a chat model, but this tells the user "A chat model (the one that works in Chat settings) can't produce embeddings". The /models override normally rescues that case — but only when the endpoint exposes a served list, and per my other comment that override should be narrowed anyway.
Softening costs nothing and stays correct for both causes:
-"That model isn't an embeddings model on this endpoint. A chat model \
- (the one that works in Chat settings) can't produce embeddings — \
- enter an embeddings model id (e.g. text-embedding-3-small, bge-m3), \
- then save again.",
+"The endpoint rejected that model id for embeddings. Check the spelling, \
+ and note that a chat model (the one that works in Chat settings) can't \
+ produce embeddings — enter an embeddings model id (e.g. \
+ text-embedding-3-small, bge-m3), then save again.",| } | ||
| EmbedProbe::TimedOut => reject( | ||
| "EMBEDDINGS_VERIFICATION_FAILED", | ||
| "EMBEDDINGS_ENDPOINT_UNREACHABLE", |
There was a problem hiding this comment.
minor — folding TimedOut into _ENDPOINT_UNREACHABLE merges two causes
The 10s box around embedder.embed(...) is routinely blown by a reachable endpoint — LM Studio cold-loading a model into VRAM, or a rate-limited cloud host queueing the request.
The user-visible message is unchanged (it still says "timed out"), so there's no UX regression. But the code is now wrong for that case, and support/telemetry keying on the new reject_code field can no longer separate "host is down" from "host is slow". Given the whole PR is about differentiation, this is the one place it goes the other way:
EmbedProbe::TimedOut => reject(
- "EMBEDDINGS_ENDPOINT_UNREACHABLE",
+ // Distinct from `_UNREACHABLE`: the host may be perfectly reachable but
+ // slow (cold model load, queued request). Own code so telemetry and
+ // support can tell "down" from "slow" (#5017).
+ "EMBEDDINGS_VERIFICATION_TIMEOUT",Nice side effect: the frontend shape match handles a brand-new code with no change — a live demonstration that the panel fix works.
| for detail in [ | ||
| r#"openai embeddings returned HTTP 400 Bad Request: {"error":{"message":"gpt-5-mini does not support embeddings"}}"#, | ||
| r#"Embedding API error (400 Bad Request): {"error":{"message":"Model gpt-5-mini does not exist"}}"#, | ||
| r#"openai embeddings returned HTTP 400 Bad Request: {"error":"this is not an embedding model"}"#, |
There was a problem hiding this comment.
blocker — these tests are tautological against the real wire format
Every classifier keys on strings owned by the vendored tinyagents::harness::embeddings crate, and every test here asserts against a literal hand-written in this same PR. If tinyagents rephrases ("returned HTTP 401" → "HTTP status 401"), all five tests keep passing while every failure silently degrades to the generic bucket — i.e. they cannot detect the regression they exist to prevent.
I hit the sharp end of this reviewing: a fresh worktree has no vendor/tinyagents, so there is no way to confirm these six shapes are real without the submodule.
The fix is cheap because you already built the tooling — conformant_custom_endpoint_verifies_and_sends_expected_request drives a real axum mock through the real provider. Point it at failure responses too, so the assertion runs over the error string tinyagents actually produces:
/// Drives the real provider against a mock returning each failure status, so
/// the classifier is asserted over the error string tinyagents actually
/// produces rather than a literal authored by this test (#5017).
#[tokio::test]
async fn classifies_real_adapter_errors_from_a_mock_endpoint() {
for (status, body, expected) in [
(401, r#"{"error":"invalid api key"}"#, "EMBEDDINGS_AUTH_FAILED"),
(403, r#"{"error":"no access"}"#, "EMBEDDINGS_AUTH_FAILED"),
(
400,
r#"{"error":{"message":"gpt-5-mini does not support embeddings"}}"#,
"EMBEDDINGS_MODEL_INCOMPATIBLE",
),
(500, r#"{"error":"boom"}"#, "EMBEDDINGS_VERIFICATION_FAILED"),
] {
let base = spawn_failing_embeddings_mock(status, body).await;
let embedder = create_embedding_provider_with_credentials(
"custom", "gpt-5-mini", 0, "sk-x", Some(&base),
)
.expect("provider builds");
let outcome = match embedder.embed(&["connection test"]).await {
Ok(v) => EmbedProbe::Returned(v),
Err(e) => EmbedProbe::Failed(e.to_string()),
};
assert_eq!(
reject_code(outcome).as_deref(),
Some(expected),
"status {status} must classify as {expected}"
);
}
}Keep the literal-based tests as cheap documentation of known shapes — they just shouldn't be the only guard.
| typeof result.error === 'string' && | ||
| result.error !== '' && | ||
| result.error !== 'EMBEDDINGS_DIMENSION_CHANGE_REQUIRES_WIPE' | ||
| ) { |
There was a problem hiding this comment.
blocker — this behavior change has no test
This is the whole point of the frontend half ("fixes a latent unknown code silently treated as a save"), and nothing exercises it. Every failure-path test in __tests__/EmbeddingsPanel.test.tsx uses one of the three previously allow-listed codes (_ENDPOINT_NO_API, _NO_MODEL_LOADED, _VERIFICATION_FAILED), so all of them pass identically against the old allow-list. Nothing would fail if someone reverted this hunk.
Add one test whose entire value is that it fails on main:
it('surfaces a differentiated backend code with no allow-list entry (#5017)', async () => {
// Regression: the panel used to allow-list three codes, so any *new* code
// fell through to the success branch and reported a save that never happened.
const settings = makeSettings({
providers: [
makeProvider('managed', { requires_api_key: false }),
makeProvider('custom', { requires_api_key: false, requires_endpoint: true }),
],
});
vi.mocked(loadEmbeddingsSettings).mockResolvedValue(settings);
vi.mocked(updateEmbeddingsSettings).mockResolvedValue({
error: 'EMBEDDINGS_AUTH_FAILED',
message: 'The endpoint rejected the API key (401/403).',
});
renderWithProviders(<EmbeddingsPanel />);
await screen.findByText('Custom');
fireEvent.click(screen.getByRole('radio', { name: /custom/i }));
await screen.findByPlaceholderText(/https:\/\/your-endpoint/i);
fireEvent.change(screen.getByPlaceholderText(/https:\/\/your-endpoint/i), {
target: { value: 'https://api.example.com/v1' },
});
fireEvent.click(screen.getByRole('button', { name: /save.*switch/i }));
await screen.findByText(/rejected the API key/i);
// Popup stays open and reload() never ran — the config was NOT saved.
expect(screen.getByPlaceholderText(/https:\/\/your-endpoint/i)).toBeInTheDocument();
expect(vi.mocked(loadEmbeddingsSettings)).toHaveBeenCalledTimes(1);
});Worth also covering the inverse — that EMBEDDINGS_DIMENSION_CHANGE_REQUIRES_WIPE still reaches the confirm dialog rather than the new failure branch. That path exists at line 365 but the exclusion at line 339 is what protects it.
3 participants
Summary
POST /v1/embeddingswith the user's model, Bearer key, and{"input":[…],"model":…}body — a conformant endpoint verifies (proven by a new mock-endpoint test).classify_embed_probecollapsed distinct failures (auth 401/403, wrong/incompatible model 400, network/DNS, dimension mismatch) into one generic message.Problem
Issue #5017: a user enters a working OpenAI-compatible base URL + model + key under Connections → Vector encoding model → Custom. The same endpoint/key work for chat, but embedding verification fails with "Couldn't verify the embeddings endpoint — the test embed failed."
I reproduced the exact test-embed request by driving the same provider construction the save-time probe uses against a mock server. The request is correct:
Path, method, model, key, and body are all right, and a conformant
/v1/embeddingshost verifies successfully. So the reporter's guesses (a) wrong shape, (b)/models/health path, (c) model not forwarded, (d) key missing, (e) dimension validation are all ruled out for this flow.The reporter used
gpt-5-mini— a chat model — which the endpoint legitimately rejects at/v1/embeddings(HTTP 400 "does not support embeddings"). Verification should fail, but the message never said the model isn't an embeddings model, so "works for chat, fails for embed" looked like a bug. Auth (401/403), network/DNS, and dimension-mismatch failures were equally indistinguishable.Solution
src/openhuman/embeddings/rpc.rs—classify_embed_probenow maps failures to distinct codes/messages, tolerant of both embed-error wire shapes (openai embeddings returned HTTP <code>…andEmbedding API error (<code>)…):EMBEDDINGS_MODEL_INCOMPATIBLE— 400/422 + model-rejection body (the reporter's case: chat model in the embeddings field).EMBEDDINGS_AUTH_FAILED— 401/403 (embeddings key is stored separately from the chat BYOK key).EMBEDDINGS_ENDPOINT_UNREACHABLE— DNS / refused / connect / timeout.EMBEDDINGS_DIMENSION_MISMATCH— endpoint ignored thedimensionsparam and returned a different length.NO_MODEL_LOADED/ENDPOINT_NO_API/ genericVERIFICATION_FAILEDpreserved. Added a PII-safe log of the classified code (never the raw detail).app/src/components/settings/panels/EmbeddingsPanel.tsx— failure detection matches on shape (any error code other than the wipe-confirm) instead of an allow-list, so the new codes surface their backend message rather than a new code being silently treated as a successful save.Design note: verification messages are backend-emitted English strings (same pattern as the existing
EMBEDDINGS_*messages the panel already renders), so no new frontend UI literals and no new i18n keys are introduced.Submission Checklist
/v1/embeddingsregression proving a conformant host verifies and the request carries the user's model + Bearer key. Pre-existingother_failures_and_timeouttest updated to the new behavior (fail-before / pass-after).classify_embed_probebranches + helpers, each exercised by the new unit tests; the one-line frontend conditional is covered by the existing panel behavior.openhuman::embeddings::rpc::testsall pass (19/19).N/A: refines an existing feature's error handling; no feature row added/removed/renamed.## Related—N/A: no matrix feature IDs affected.axummock (already a dev-dependency); no real network.N/A: settings-panel error messaging only, no release-cut surface change.Closes #NNNin the## Relatedsection.Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— EmbeddingsPanel.tsx prettier-cleanpnpm typecheck— passesGGML_NATIVE=OFF cargo test --lib openhuman::embeddings::rpc::tests— 19 passed, 0 failedrustfmt --check src/openhuman/embeddings/rpc.rs— cleanapp/src-taurichangeValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
NO_MODEL_LOADED/ENDPOINT_NO_API/DIMENSION_CHANGE_REQUIRES_WIPEcodes, and the/modelsname-mismatch guidance path are all unchanged.EMBEDDINGS_VERIFICATION_FAILED, so no failure is ever misreported as a success.Duplicate / Superseded PR Handling
Summary by CodeRabbit