fix(memory_tree): diagnose embeddings from the real resolver ladder - #5346
fix(memory_tree): diagnose embeddings from the real resolver ladder#5346Mustaqeem66 wants to merge 8 commits into
Conversation
`resolve_embedder_choice` is the single source of truth for which embeddings provider both factories select, but it is private and returns a value that owns a constructed embedder, so nothing else can ask "what would you pick?" without building one. Add `resolved_embedder()` + `ResolvedEmbedder`, a side-effect-free view of the same ladder that reports the provider label (or the deliberate `none` opt-out, or genuinely unconfigured) without constructing an embedder or touching the network. This lets the memory-tree health doctor diagnose the embeddings stage from the real ladder instead of re-deriving an approximation of it (tinyhumansai#5330), and keeps that diagnosis correct as the ladder evolves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit accidentally reflowed the rule characters on an existing comment header. Restore it verbatim so the diff contains only the new diagnostic API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leaves the diff containing only the new diagnostic API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The doctor's embeddings stage re-derived provider resolution instead of asking the factory. It checked `memory_tree.embedding_endpoint` (without the model that the factory also requires) and then fell back to the top-level `embeddings_provider` workload field, which is `None` by default — a one-step approximation of a six-step ladder. So every install that resolves further down the ladder (the unified `ollama:<model>` workload setting, a user-configured OpenAI-compatible endpoint, or the logged-in managed cloud that most installs land on) was reported `embeddings_unconfigured` while embedding demonstrably worked. Because embeddings sit near the front of the ordered stage list, that bogus failure became `first_blocking_cause` and masked the genuine one, sending users to "fix" a setting that was already correct. Route the stage through `resolved_embedder()` so the diagnosis is the factory's own answer, and keep the deliberate `none` opt-out reported as an intentional choice rather than a fault. Tests: make `test_config()` hermetic (the ladder probes the filesystem for `auth-profiles.json`, so the suite would otherwise depend on whether the developer's machine is logged in), and add regression coverage for the terminal cloud rung and a mid-ladder local model. Fixes tinyhumansai#5330 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe embedder factory now exposes provider-resolution diagnostics without constructing an embedder. ChangesEmbedder health resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HealthDoctor
participant EmbedderFactory
participant ResolutionLadder
HealthDoctor->>EmbedderFactory: Call resolved_embedder(config)
EmbedderFactory->>ResolutionLadder: Resolve configured provider
ResolutionLadder-->>EmbedderFactory: Return provider, opt-out, or unconfigured
EmbedderFactory-->>HealthDoctor: Return ResolvedEmbedder
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/openhuman/memory/tree/score/embed/factory.rs | Adds ResolvedEmbedder enum and resolved_embedder() public diagnostic function that wraps the existing private resolve_embedder_choice ladder; adds 5 well-structured tests including a parity guard (now covering all three Provider variants: ollama, openai, cloud) and the logged-in cloud regression case. |
| src/openhuman/memory/tree/health/doctor.rs | Replaces the hand-rolled 2-step embeddings approximation in run_doctor with a call to resolved_embedder; makes test_config() hermetic by planting config_path inside the TempDir; adds 2 regression tests including a precondition-checked cloud-provider test. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_doctor / resolved_embedder] --> B{Rung 1:\nembedding_endpoint\n+ embedding_model set?}
B -->|Yes| C[Provider: ollama override]
B -->|No| D{Rung 2:\nembeddings_provider\n= 'none'?}
D -->|Yes| E[OptOut]
D -->|No| F{Rung 3:\nworkload_local_model\n'embeddings'?}
F -->|Yes| G[Provider: ollama local workload]
F -->|No| H{Rung 4:\nOpenAiCompatEmbedder\n::try_from_config?}
H -->|Ok Some| I[Provider: openai / custom]
H -->|Ok None| J{Rung 5:\ncloud_session_available\nauth-profiles.json?}
J -->|Yes| K[Provider: cloud]
J -->|No| L[Unconfigured]
H -->|Err| L
subgraph Doctor embeddings stage
M{ResolvedEmbedder?}
E --> M
C --> M
G --> M
I --> M
K --> M
L --> M
M -->|Provider p| N[StageHealth::ok\nprovider configured: p]
M -->|OptOut| O[StageHealth::ok\ndisabled by you]
M -->|Unconfigured| P[StageHealth::bad\nEmbeddingsUnconfigured]
end
Reviews (3): Last reviewed commit: "chore(memory_tree): relocate resolved_em..." | Re-trigger Greptile
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/memory/tree/score/embed/factory.rs (1)
711-746: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend the parity test to cover "cloud" and "custom", the branches most central to this PR.
The comment above the test states this guard checks parity Parity guard: for every provider the ladder can select, the label the diagnostic reports must equal the name the read factory's embedder reports. The
casesvector only covers"ollama"and"openai". It omits"cloud"(the managed-cloud rung whose false-unconfiguredbug motivated this whole PR) and"custom"(the lmstudio-style local OpenAI-compatible endpoint). Add these two cases so the parity guard actually protects the scenario this PR was written to fix.♻️ Proposed additional cases
let cases: Vec<(&str, Box<dyn Fn(&mut Config)>)> = vec![ ( "ollama", Box::new(|c: &mut Config| { c.memory_tree.embedding_endpoint = Some("http://localhost:11434".into()); c.memory_tree.embedding_model = Some("bge-m3".into()); }), ), ( "openai", Box::new(|c: &mut Config| { c.memory.embedding_provider = "openai".to_string(); c.memory.embedding_model = "text-embedding-3-large".to_string(); }), ), + ( + "custom", + Box::new(|c: &mut Config| { + c.memory.embedding_provider = "lmstudio".to_string(); + c.memory.embedding_model = "bge-m3".to_string(); + c.cloud_providers = vec![ + crate::openhuman::config::schema::cloud_providers::CloudProviderCreds { + id: "p_lmstudio".to_string(), + slug: "lmstudio".to_string(), + endpoint: "http://localhost:1234/v1".to_string(), + ..Default::default() + }, + ]; + }), + ), ]; for (expected, setup) in cases { let (_tmp, mut cfg) = test_config(); cfg.memory_tree.embedding_endpoint = None; cfg.memory_tree.embedding_model = None; + // "cloud" needs a session; touch it unconditionally, it's a no-op + // for the other cases. + touch_auth_profile(&cfg); setup(&mut cfg);Note:
"cloud"also needs a case added similarly, driven bytouch_auth_profilealone with no other config set.🤖 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/memory/tree/score/embed/factory.rs` around lines 711 - 746, Extend the cases in resolved_embedder_label_agrees_with_the_built_embedder to cover the "cloud" and "custom" provider branches. Configure the cloud case using only touch_auth_profile, with no other embedding settings, and configure custom with its lmstudio-style local OpenAI-compatible endpoint settings; assert both continue through the existing resolved_embedder and built.name parity checks.
🤖 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.
Nitpick comments:
In `@src/openhuman/memory/tree/score/embed/factory.rs`:
- Around line 711-746: Extend the cases in
resolved_embedder_label_agrees_with_the_built_embedder to cover the "cloud" and
"custom" provider branches. Configure the cloud case using only
touch_auth_profile, with no other embedding settings, and configure custom with
its lmstudio-style local OpenAI-compatible endpoint settings; assert both
continue through the existing resolved_embedder and built.name parity checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77692f28-bcb4-4237-a4da-75beeeccec4b
📒 Files selected for processing (2)
src/openhuman/memory/tree/health/doctor.rssrc/openhuman/memory/tree/score/embed/factory.rs
…he doc claim Review feedback from @greptile-apps on tinyhumansai#5346: - The parity guard said "for every provider the ladder can select" but only covered `ollama` and `openai`, so a rename of the `cloud` label would have slipped through — and cloud is the rung most installs actually land on. Add it using the existing `touch_auth_profile` helper. - `resolved_embedder`'s doc claimed "no embedder is constructed", which is narrowly untrue on the OpenAI-compatible rung: `try_from_config` does build an `OpenAiCompatEmbedder`, which is dropped immediately after its label is read. Say what's actually guaranteed instead — no network request on any rung, and no embedder escapes to the caller. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @greptile-apps — both points were fair and are addressed in e7333d7. 1. Parity guard omitted the (
// The terminal rung, and the one most installs actually land
// on — so a rename of the `cloud` label must break this guard
// too. (greptile on the parity guard)
"cloud",
Box::new(|c: &mut Config| {
touch_auth_profile(c);
}),
),The loop body already clears 2. "no embedder is constructed" was inaccurate on the OpenAI-compatible rung. Correct, and I'd rather the doc under-promise than mislead someone reasoning about cost.
The two properties that matter for calling this from the doctor — no network I/O and no degraded-flag mutation (only the factories' terminals touch those) — both still hold on every rung, so the change is to the wording, not the contract. For the record on process: I have no Rust toolchain or network locally, so CI is doing the verifying here — fmt, clippy, the feature-gate build and the coverage gate were all green on the previous commit, and I'll watch this one. Flagged plainly in the PR's Validation Blocked section rather than quietly ticking boxes. |
|
Both fixes land cleanly in e7333d7. Parity guard — cloud case: The Doc comment: The revised wording is accurate and states exactly what matters for callers: no network I/O on any rung, no embedder escapes. That's the right level of precision. Both concerns are resolved. LGTM. |
…onflict main gained `redact_ladder_error` + `effective_embedder_slug` and nine new tests appended at the end of `mod tests` (tinyhumansai#5402). This branch also appends its five `resolved_embedder` tests at the same anchor, so the two insertions collide and the PR reports a conflict. Move the `resolved_embedder` test block from the end of `mod tests` to just after `none_provider_returns_inert`, well clear of main's insertion point. Test order is irrelevant in Rust and no test body, name or assertion is changed - the file's set of lines is identical, only their position moves. Verified by 3-way merge against merge-base 210cd6c: 0 conflict regions, 0 lines dropped from either side.
Conflicts resolved (
|
Summary
embeddingsstage re-derived provider resolution instead of asking the embedder factory, so it reportedembeddings_unconfiguredon installs where embedding demonstrably works.resolved_embedder()+ResolvedEmbeddertoscore/embed/factory.rs: a diagnostic view of the factory's existing resolution ladder that reports which provider would be selected without handing back an embedder or touching the network.embeddingsstage through it, so the diagnosis is the factory's own answer rather than a parallel approximation.first_blocking_causeand masked the genuine one — this restores the doctor's core promise of "one actionable answer".test_config()hermetic.Problem
run_doctorcomputed the embeddings stage like this:That is a one-step approximation of a six-step ladder.
resolve_embedder_choiceinscore/embed/factory.rs— the single source of truth walked by bothbuild_embedder_from_config(read) andbuild_write_embedder(write) — resolves in this order:memory_tree.embedding_endpointandembedding_model, both non-empty)embeddings_provider == "none"→ deliberate opt-outconfig.workload_local_model("embeddings")→ local OllamaOpenAiCompatEmbedder::try_from_config→ user OpenAI / custom endpointcloud_session_available→ managed cloudThree concrete defects:
config.embeddings_provider, which is workload routing and defaults toNone.So the common case (logged in, no explicit embeddings config) embeds fine but is reported
embeddings_unconfigured. In the reporter's case that maskedsummarizer_unavailable, and sent them to "fix" a setting that was already correct.Solution
Rather than teach the doctor to read one more config field — which would re-break the moment the ladder changes — expose the ladder's answer:
The doctor then just matches on it. Design notes:
resolve_embedder_choice/EmbedderChoicepublic.EmbedderChoice::OpenAiCompatowns a constructedOpenAiCompatEmbedder; publishing it would leak embedder internals and hand callers a built embedder they didn't ask for.ResolvedEmbedderisCopyand carries only a label.Path::exists(); on the OpenAI-compatible rung it constructs anOpenAiCompatEmbedderthat is dropped immediately after its label is read (thanks @greptile-apps for catching that my first doc comment overclaimed here). It does not set the process-global degraded flags — only the factories' terminals do. The doctor already performs synchronous SQLite reads, so this is comfortably the cheapest thing it does.Err(_)is folded intoUnconfigured. A ladder error means the configured OpenAI-compatible provider could not be constructed — from the user's perspective embeddings are unusable, which is whatUnconfiguredsays. The write path still fails loudly with the underlying error.noneopt-out staysok. It's a deliberate choice (likescheduler_gate = off), reported with an honest note that can't read as a working provider — preserving the behaviour a previous CodeRabbit review asked for.Tests
factory.rs(5): the logged-in cloud default, thenoneopt-out staying distinct from unconfigured, the genuinely-unconfigured case, the OpenAI-compatible backend, and a parity guard asserting the diagnostic's label equalsbuild_embedder_from_config(...).name()forollama,openaiandcloud— so adding a ladder rung, or renaming a label, and updating only one match arm fails CI.doctor.rs(2):working_cloud_provider_is_not_reported_unconfigured(the #5330 regression, with a precondition assert that the ladder really resolves to cloud, so a failure can only be the doctor's fault) andworkload_local_model_is_reported_as_configured(a mid-ladder rung).doctor.rs'stest_config()now plantsconfig_pathinside theTempDir. The ladder's last rung probes the filesystem forauth-profiles.jsonnext to the config file, so without this the doctor suite would read the developer's real session and pass or fail depending on whether the machine happens to be logged in. This mirrors the harnessfactory.rsalready uses.Submission Checklist
resolved_embedder_reports_unconfigured_when_nothing_resolves, theErr(_) → Unconfiguredarm, and the preservedmisconfigured_workspace_reports_embeddings_as_first_blocking_cause.N/A: behaviour-only fix; no feature rows added, removed or renamed. The Coverage Matrix Sync check passed.## Related—N/A: no matrix rows are affected by this change.resolved_embedderissues no network request on any rung; the new tests use aTempDirand a stub file only. No new crates.N/A: no release-cut surface changes.The doctor report's shape is unchanged; only the value of one stage is corrected.Closes #NNNin the## Relatedsection.Impact
memory_tree doctoragent tool / CLI / RPC output and the status panel'sfirst_blocking_cause.embeddings_unconfiguredwill now show the resolved provider, andfirst_blocking_causewill surface the real problem (orhealthy). Installs with genuinely no provider are unchanged.run_doctorcall — allocations plus a singlePath::exists(), alongside the several SQLite queries the doctor already runs.resolved_embedderis additive; no existing signature changed.Related
status/pipeline_statussurface derives its own embeddings view; worth auditing whether it shares this bug, but it's out of scope here and I didn't want to widen the diff.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
N/A— external contributor, no Linear access.N/ACommit & Branch
Mustaqeem66:fix/5330-doctor-embeddings-ladderValidation Run
pnpm --filter openhuman-app format:check—N/A: no frontend files changed.CI's Frontend Checks correctly skipped for this diff.pnpm typecheck—N/A: no TypeScript changed.This PR touches two.rsfiles only.cargo test -p openhuman memory::tree::health::doctorand... memory::tree::score::embed::factory.N/A: no Tauri code touched.Validation Blocked
command:cargo test/cargo fmt --check/cargo clippy/pnpm typecheckerror:no Rust toolchain (cargo/rustcabsent) and no network access to install one or reach the crate registry in the environment this change was authored in.impact:I could not compile or execute anything before pushing. I compensated by hand-verification: both files were reconstructed byte-for-byte against their upstream blobs before editing (confirmed by git blob SHA) so the diff contains only intended changes; I traced all six ladder rungs against every pre-existing doctor test to confirm none flip ("none"→OptOut→ stillok;"ollama:bge-m3"→ rung 3 →Provider("ollama")→ stillok; the misconfigured-workspace test → rung 6 → stillUnconfigured, which the hermeticconfig_pathchange guarantees). CI has since confirmed this: fmt, clippy, the feature-gate smoke build and the coverage gate all pass on the branch. I'll keep treating CI as the authority and fix anything it flags promptly.Disclosing plainly per the repo's AI-authorship requirement: this PR was written with AI assistance. The analysis, the design decision, and every claim above are ones I stand behind and can defend in review, and the validation I could not perform is stated rather than checked off silently.
Behavior Changes
embeddingsstage reports the provider the embedder factory would actually select, instead of a locally re-derived approximation.openhuman doctor/ the memory-tree status panel stop claiming "no embeddings provider configured" on working installs, andfirst_blocking_causenow names the real blocker instead of being masked by the false embeddings failure.Parity Contract
embeddings), theFailureCode::EmbeddingsUnconfiguredcode, and all three note strings are unchanged verbatim. A genuinely unconfigured install produces a byte-identical stage to before. Thenoneopt-out keeps itsok-with-honest-note treatment.resolved_embedderand the two factories share oneresolve_embedder_choicecall, so they cannot diverge;resolved_embedder_label_agrees_with_the_built_embedderasserts the diagnostic's label equalsEmbedder::name()forollama,openaiandcloud; and the doctor test asserts a precondition on the ladder before asserting on the stage.Duplicate / Superseded PR Handling
doctor,embeddings,resolved_embedderand memory_tree doctor reports embeddings_unconfigured while embeddings are configured and working #5330 references before starting.N/ASummary by CodeRabbit