fix: route embedding emitters through expected-error reporting#2216
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughBoth Ollama and OpenAI embedding providers' error reporting paths are updated to use ChangesEmbedding Error Reporting Classification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
graycyrus
left a comment
There was a problem hiding this comment.
Walkthrough
Clean, well-scoped fix that routes three embedding error-emission sites through the expected-error classifier (report_error_or_expected) instead of the unconditional report_error. This reduces Sentry noise for known Ollama/OpenAI error wire shapes while ensuring genuine parse/API-key errors still propagate. Six classifier-state tests lock the current behavior so follow-up classifier batches (post #2063 + #2188) can safely flip the expected values.
Change Summary
| File | Change type | Description |
|---|---|---|
src/openhuman/embeddings/ollama.rs |
Modified | Swap report_error → report_error_or_expected at transport-error and non-2xx paths |
src/openhuman/embeddings/ollama_tests.rs |
Modified | 5 new classifier-state tests (MA, KM, GP, GX wire shapes + parse-error sentinel) |
src/openhuman/embeddings/openai.rs |
Modified | Swap report_error → report_error_or_expected at non-2xx path |
src/openhuman/embeddings/openai_tests.rs |
Modified | 1 new classifier-state test (401 API key error sentinel) |
Per-file Analysis
ollama.rs — Mechanical swap at two call sites. The message.as_str() → &message change is correct (&String derefs to &str, both impl Display, and the generic signature <E: Display + ?Sized> is identical between old and new functions). Tag slices unchanged. LGTM.
openai.rs — Same mechanical swap, same correctness reasoning. LGTM.
ollama_tests.rs — Five well-commented tests that call expected_error_kind directly (synchronous, no network). Good use of assertion messages to explain the "why" behind each expected value. One minor note below on gp_wire_shape_classifies.
openai_tests.rs — Clean sentinel test ensuring 401 errors remain unexpected. LGTM.
Overall this is a solid, focused PR. No critical or major issues — just one minor suggestion on test precision.
| assert_eq!( | ||
| crate::core::observability::expected_error_kind(msg), | ||
| None, | ||
| "KM — matcher arm pending follow-up classifier batch" |
There was a problem hiding this comment.
[minor] is_some() doesn't pin the specific ExpectedErrorKind variant. If a future broader matcher catches this message but classifies it as a different kind, this test would pass silently.
Consider matching the exact variant for consistency with the other tests' precision:
assert_eq!(
crate::core::observability::expected_error_kind(msg),
Some(ExpectedErrorKind::LocalAiCapabilityUnavailable),
"GP — LocalAiCapabilityUnavailable matcher must catch this; closed by this PR"
);Not blocking — the other five tests all use assert_eq! with an exact expected value, so this would also bring stylistic consistency.
# Conflicts: # src/openhuman/embeddings/openai.rs
…ariant Matches the exact ExpectedErrorKind variant in gp_wire_shape_classifies instead of `is_some()`, for stylistic consistency with the other classifier tests and to avoid passing if a future broader matcher catches the message under a different variant. Addresses @graycyrus review comment on ollama_tests.rs:355.
…umansai#2216) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
…umansai#2216) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
…umansai#2216) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
Summary
report_error_or_expectedat the three direct-emit sites called out in kickoff.src/core/observability.rschanges.Problem
ollama.rsandopenai.rswere usingreport_error(...)directly, bypassing expected-error classification and creating avoidable Sentry noise.observability.rslanes.Solution
report_error(message.as_str(), ...)toreport_error_or_expected(&message, ...)at:src/openhuman/embeddings/ollama.rs(transport and non-2xx paths)src/openhuman/embeddings/openai.rs(non-2xx path)observability.rs.Submission Checklist
Closes #NNNin the## RelatedsectionImpact
Related
Closes OPENHUMAN-TAURI-MA
Closes OPENHUMAN-TAURI-KM
Closes OPENHUMAN-TAURI-GP
Closes OPENHUMAN-TAURI-GX
Classifier-arm additions for MA/KM/GX wire shapes after observability lane merges (fix(observability): demote loopback sidecar-down noise to expected (#R5 #R6) #2063 / test(observability): pin SESSION_EXPIRED wire-shape regression tests (#SG) #2188 family).
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/sentry-ollama-embed-emitter1a22b1ddc61d3bb6fdf7727ab5c4e567ab225ec5Validation Run
pnpm --filter openhuman-app format:check(Rust-only lane)pnpm typecheck(Rust-only lane)CARGO_TARGET_DIR=target_codex cargo test --lib openhuman::embeddings::ollama::tests:: -- --test-threads=1;CARGO_TARGET_DIR=target_codex cargo test --lib openhuman::embeddings::openai::tests::openai_embedding_api_error_stays_unexpected -- --exactcargo fmt;CARGO_TARGET_DIR=target_codex cargo checkValidation Blocked
command:CARGO_TARGET_DIR=target_codex cargo clippy --all-targets -- -D warningserror:Fails on pre-existing repository-wide warnings/lints outside this PR scopeimpact:No local all-target clippy clean signal for this lane; relied on focused tests + CIBehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests