Skip to content

fix(memory_tree): diagnose embeddings from the real resolver ladder - #5346

Open
Mustaqeem66 wants to merge 8 commits into
tinyhumansai:mainfrom
Mustaqeem66:fix/5330-doctor-embeddings-ladder
Open

fix(memory_tree): diagnose embeddings from the real resolver ladder#5346
Mustaqeem66 wants to merge 8 commits into
tinyhumansai:mainfrom
Mustaqeem66:fix/5330-doctor-embeddings-ladder

Conversation

@Mustaqeem66

@Mustaqeem66 Mustaqeem66 commented Aug 3, 2026

Copy link
Copy Markdown

Summary

  • The memory-tree health doctor's embeddings stage re-derived provider resolution instead of asking the embedder factory, so it reported embeddings_unconfigured on installs where embedding demonstrably works.
  • Add resolved_embedder() + ResolvedEmbedder to score/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.
  • Route the doctor's embeddings stage through it, so the diagnosis is the factory's own answer rather than a parallel approximation.
  • Because the embeddings stage sits near the front of the ordered stage list, the bogus failure became first_blocking_cause and masked the genuine one — this restores the doctor's core promise of "one actionable answer".
  • Adds 7 tests (5 factory, 2 doctor) and makes the doctor's test_config() hermetic.

Note on the issue's file paths: #5330 cites src/openhuman/memory_tree/health/doctor.rs and src/openhuman/memory_tree/score/embed/factory.rs. Those paths don't exist; the real ones are under memory/tree/. The described behaviour is otherwise exactly as reported.

Problem

run_doctor computed the embeddings stage like this:

let embeddings_provider = config
    .memory_tree
    .embedding_endpoint
    .as_deref()
    .filter(|s| !s.trim().is_empty())
    .map(|_| "ollama-override".to_string())
    .or_else(|| config.embeddings_provider.clone())
    .filter(|s| !s.trim().is_empty());

That is a one-step approximation of a six-step ladder. resolve_embedder_choice in score/embed/factory.rs — the single source of truth walked by both build_embedder_from_config (read) and build_write_embedder (write) — resolves in this order:

  1. explicit Ollama override (memory_tree.embedding_endpoint and embedding_model, both non-empty)
  2. embeddings_provider == "none" → deliberate opt-out
  3. config.workload_local_model("embeddings") → local Ollama
  4. OpenAiCompatEmbedder::try_from_config → user OpenAI / custom endpoint
  5. cloud_session_available → managed cloud
  6. nothing usable

Three concrete defects:

  1. Step 1 requires endpoint and model; the doctor checked only the endpoint (so it could also report a provider the factory would reject).
  2. It fell back to the top-level config.embeddings_provider, which is workload routing and defaults to None.
  3. It never saw rungs 3–5 at all — including rung 5, the path a default logged-in install lands on.

So the common case (logged in, no explicit embeddings config) embeds fine but is reported embeddings_unconfigured. In the reporter's case that masked summarizer_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:

pub enum ResolvedEmbedder {
    Provider(&'static str), // same label Embedder::name() reports
    OptOut,                 // `none` — off by choice, not a fault
    Unconfigured,
}

pub fn resolved_embedder(config: &Config) -> ResolvedEmbedder;

The doctor then just matches on it. Design notes:

  • New public API instead of making resolve_embedder_choice / EmbedderChoice public. EmbedderChoice::OpenAiCompat owns a constructed OpenAiCompatEmbedder; publishing it would leak embedder internals and hand callers a built embedder they didn't ask for. ResolvedEmbedder is Copy and carries only a label.
  • No network, nothing escapes. The ladder allocates and does one Path::exists(); on the OpenAI-compatible rung it constructs an OpenAiCompatEmbedder that 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 into Unconfigured. A ladder error means the configured OpenAI-compatible provider could not be constructed — from the user's perspective embeddings are unusable, which is what Unconfigured says. The write path still fails loudly with the underlying error.
  • The none opt-out stays ok. It's a deliberate choice (like scheduler_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, the none opt-out staying distinct from unconfigured, the genuinely-unconfigured case, the OpenAI-compatible backend, and a parity guard asserting the diagnostic's label equals build_embedder_from_config(...).name() for ollama, openai and cloud — 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) and workload_local_model_is_reported_as_configured (a mid-ladder rung).

doctor.rs's test_config() now plants config_path inside the TempDir. The ladder's last rung probes the filesystem for auth-profiles.json next 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 harness factory.rs already uses.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) — 7 new tests; failure/edge cases covered by resolved_embedder_reports_unconfigured_when_nothing_resolves, the Err(_) → Unconfigured arm, and the preserved misconfigured_workspace_reports_embeddings_as_first_blocking_cause.
  • Diff coverage ≥ 80% — verified by CI, not locally: Rust Core Coverage (cargo-llvm-cov) and the PR CI Gate both passed on this branch. I have no Rust toolchain locally (see Validation Blocked), so CI is the evidence here.
  • Coverage matrix updated — N/A: behaviour-only fix; no feature rows added, removed or renamed. The Coverage Matrix Sync check passed.
  • All affected feature IDs from the matrix are listed in the PR description under ## RelatedN/A: no matrix rows are affected by this change.
  • No new external network dependencies introduced — resolved_embedder issues no network request on any rung; the new tests use a TempDir and a stub file only. No new crates.
  • Manual smoke checklist updated if this touches release-cut surfaces — N/A: no release-cut surface changes. The doctor report's shape is unchanged; only the value of one stage is corrected.
  • Linked issue closed via Closes #NNN in the ## Related section.

Impact

  • Runtime/platform: all platforms, diagnostic-only. Affects the memory_tree doctor agent tool / CLI / RPC output and the status panel's first_blocking_cause.
  • Behaviour: installs previously mislabelled embeddings_unconfigured will now show the resolved provider, and first_blocking_cause will surface the real problem (or healthy). Installs with genuinely no provider are unchanged.
  • Performance: one extra ladder walk per run_doctor call — allocations plus a single Path::exists(), alongside the several SQLite queries the doctor already runs.
  • Security / migration / compatibility: none. No serialized shape changed, no config migration, no new deps. resolved_embedder is additive; no existing signature changed.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A — external contributor, no Linear access.
  • URL: N/A

Commit & Branch

  • Branch: Mustaqeem66:fix/5330-doctor-embeddings-ladder
  • Commit SHA: see the branch head (latest commit addresses the Greptile review)

Validation Run

I have no Rust/pnpm toolchain and no network in the environment this was authored in, so nothing below was run locally. Each box is ticked against the equivalent CI job on this branch, which is stronger evidence than a local run anyway. Full detail in Validation Blocked.

  • pnpm --filter openhuman-app format:checkN/A: no frontend files changed. CI's Frontend Checks correctly skipped for this diff.
  • pnpm typecheckN/A: no TypeScript changed. This PR touches two .rs files only.
  • Focused tests: NOT RUN locally — covered by CI's Rust Core Coverage (cargo-llvm-cov) ✅, which builds and runs the crate's test suite including the 7 new tests. Intended local commands were cargo test -p openhuman memory::tree::health::doctor and ... memory::tree::score::embed::factory.
  • Rust fmt/check (if changed): NOT RUN locally — covered by CI's Rust Quality (fmt, clippy) ✅ and Rust Feature-Gate Smoke (gates off) ✅.
  • Tauri fmt/check (if changed): N/A: no Tauri code touched.

Validation Blocked

  • command: cargo test / cargo fmt --check / cargo clippy / pnpm typecheck
  • error: no Rust toolchain (cargo/rustc absent) 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 → still ok; "ollama:bge-m3" → rung 3 → Provider("ollama") → still ok; the misconfigured-workspace test → rung 6 → still Unconfigured, which the hermetic config_path change 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

  • Intended behavior change: the doctor's embeddings stage reports the provider the embedder factory would actually select, instead of a locally re-derived approximation.
  • User-visible effect: openhuman doctor / the memory-tree status panel stop claiming "no embeddings provider configured" on working installs, and first_blocking_cause now names the real blocker instead of being masked by the false embeddings failure.

Parity Contract

  • Legacy behavior preserved: the stage id (embeddings), the FailureCode::EmbeddingsUnconfigured code, and all three note strings are unchanged verbatim. A genuinely unconfigured install produces a byte-identical stage to before. The none opt-out keeps its ok-with-honest-note treatment.
  • Guard/fallback/dispatch parity checks: resolved_embedder and the two factories share one resolve_embedder_choice call, so they cannot diverge; resolved_embedder_label_agrees_with_the_built_embedder asserts the diagnostic's label equals Embedder::name() for ollama, openai and cloud; and the doctor test asserts a precondition on the ladder before asserting on the stage.

Duplicate / Superseded PR Handling

Summary by CodeRabbit

  • Bug Fixes
    • Improved embedding health checks to accurately reflect the configured provider and resolution settings.
    • Added clearer status reporting for enabled providers, intentional opt-out, and unavailable configurations.
    • Improved support for managed cloud, local-model, and OpenAI-compatible embedding setups.

Mustaqeem66 and others added 6 commits August 3, 2026 22:57
`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>
@Mustaqeem66
Mustaqeem66 requested a review from a team August 3, 2026 19:23
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 75d9d143-d407-4458-af1f-23a4c3deae7e

📥 Commits

Reviewing files that changed from the base of the PR and between e7333d7 and d3abfc8.

📒 Files selected for processing (1)
  • src/openhuman/memory/tree/score/embed/factory.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/openhuman/memory/tree/score/embed/factory.rs

📝 Walkthrough

Walkthrough

The embedder factory now exposes provider-resolution diagnostics without constructing an embedder. run_doctor uses these diagnostics to report managed-cloud, local-model, opt-out, and unconfigured embedding states.

Changes

Embedder health resolution

Layer / File(s) Summary
Add embedder resolution diagnostics
src/openhuman/memory/tree/score/embed/factory.rs
Added ResolvedEmbedder and resolved_embedder. Tests cover provider selection, opt-out, unconfigured states, OpenAI-compatible routing, and factory parity.
Integrate diagnostics into the health doctor
src/openhuman/memory/tree/health/doctor.rs
Updated run_doctor to use factory resolution. Tests isolate authentication profiles and cover managed-cloud and local Ollama 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
Loading

Possibly related PRs

  • tinyhumansai/openhuman#5402 — Both changes update factory.rs to derive embedding-provider diagnostics from the shared resolution ladder.

Suggested labels: rust-core, memory, bug

Poem

I hop through the factory, neat and bright,
Cloud or Ollama, both resolve right.
Opt-out rests beneath the tree,
Unconfigured states are clear to see.
The doctor smiles: “Healthy!” 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: embedding diagnosis now uses the real resolver ladder.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot added bug memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. labels Aug 3, 2026
@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a false-positive embeddings_unconfigured report in the memory-tree health doctor by routing the embeddings stage through the factory's own resolve_embedder_choice ladder instead of a hand-rolled two-step approximation that missed rungs 3–5 (local workload model, OpenAI-compatible, and the logged-in managed cloud that most installs land on).

  • Adds ResolvedEmbedder (a Copy diagnostic enum) and resolved_embedder() to factory.rs as a thin public wrapper over the existing private resolve_embedder_choice, carrying only a label — not a live embedder or any network call.
  • Replaces the doctor's 15-line local approximation with a single match resolved_embedder(config) call, eliminating the drift surface permanently.
  • Makes test_config() in doctor.rs hermetic by planting config_path inside the TempDir, so the cloud-rung filesystem probe no longer reads the developer's real session; adds 7 new tests including a precondition-checked regression test for the masked-first_blocking_cause scenario.

Confidence Score: 5/5

Diagnostic-only change with no serialized shape, config, or schema modifications; all existing tests pass and 7 new tests lock in the corrected behaviour.

The change is structurally sound: both factories and the new diagnostic share one resolve_embedder_choice call, making divergence a compile-time error if EmbedderChoice gains a new variant. The previous two Greptile review comments (missing cloud case in the parity guard; doc overclaiming no embedder constructed) are both addressed in this revision. No runtime paths outside the health doctor are modified.

Files Needing Attention: No files require special attention.

Important Files Changed

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
Loading

Reviews (3): Last reviewed commit: "chore(memory_tree): relocate resolved_em..." | Re-trigger Greptile

Comment thread src/openhuman/memory/tree/score/embed/factory.rs
Comment thread src/openhuman/memory/tree/score/embed/factory.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/openhuman/memory/tree/score/embed/factory.rs (1)

711-746: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Extend 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 cases vector only covers "ollama" and "openai". It omits "cloud" (the managed-cloud rung whose false-unconfigured bug 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 by touch_auth_profile alone 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5efb24a and 9f3e674.

📒 Files selected for processing (2)
  • src/openhuman/memory/tree/health/doctor.rs
  • src/openhuman/memory/tree/score/embed/factory.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 3, 2026
…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>
@Mustaqeem66

Copy link
Copy Markdown
Author

Thanks @greptile-apps — both points were fair and are addressed in e7333d7.

1. Parity guard omitted the cloud case. You're right, and it was the worst one to omit: cloud is the rung most installs actually land on, and it's the rung this whole PR exists because of. The guard's docstring promised "every provider the ladder can select" while covering two of three, so a rename of the "cloud" literal in either build_cloud_embedder or the Ok(EmbedderChoice::Cloud) arm would have sailed through — exactly the drift the test is supposed to prevent. Added, reusing touch_auth_profile:

(
    // 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 embedding_endpoint/embedding_model before running each setup, and test_config() plants config_path in a TempDir, so the cloud case resolves through rungs 1–4 without matching and terminates at rung 5 — no cross-case leakage. (Block body rather than Box::new(touch_auth_profile) to keep the &Config/&mut Config coercion explicit and avoid a redundant_closure argument.)

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. try_from_config does construct an OpenAiCompatEmbedder; it's dropped right after .name() is read. Reworded to state what's actually guaranteed:

Cheap and side-effect-free: it allocates, probes one path, and (only on the OpenAI-compatible rung) constructs an OpenAiCompatEmbedder that is dropped immediately after its label is read. No network request is issued on any rung, and no embedder escapes.

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.

@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown

Both fixes land cleanly in e7333d7.

Parity guard — cloud case: The touch_auth_profile setup is correct; with embedding_endpoint/embedding_model cleared at the top of each iteration the cloud config falls straight through rungs 1–4 and hits rung 5 as intended. The comment calling out why cloud is the most important rung to guard is a good addition — future maintainers will understand why it's covered explicitly.

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.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 4, 2026
…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.
@Mustaqeem66

Copy link
Copy Markdown
Author

Conflicts resolved (d3abfc8)

Rebasing wasn't available in the environment I authored this in, so I resolved the conflict by relocating the colliding block within the file. Details below so a reviewer can verify it cheaply.

What was actually conflicting

Only one region, and only in src/openhuman/memory/tree/score/embed/factory.rs.

Since this branch was cut (merge-base 210cd6c), main is 22 commits ahead and added to that file:

  • redact_ladder_error() + effective_embedder_slug(), inserted after build_write_embedder
  • nine new tests appended at the end of mod tests

This branch adds:

  • ResolvedEmbedder + resolved_embedder(), inserted before build_write_embedder's doc comment
  • five new tests appended at the end of mod tests

The two production-code insertions are ~60 lines apart and never collided. The conflict was entirely the two independent "append at the end of mod tests" edits landing on the same anchor — the closing } of the test module.

src/openhuman/memory/tree/health/doctor.rs did not conflict: its blob is identical at the merge-base and at main (fd29ed6), so it merges clean and I did not touch it.

How it was resolved

Moved the five resolved_embedder tests from the end of mod tests to just after none_provider_returns_inert() — roughly 100 unchanged lines clear of main's insertion point.

  • No test body, name, assertion or production line was changed. The file's multiset of lines is byte-identical to the previous branch head; only the position of one block moved.
  • Test order is irrelevant in Rust, and the helpers those tests use (test_config(), touch_auth_profile(), build_embedder_from_config()) are module-scoped and order-independent.
  • The diff stat is unchanged at +270 −15 across 2 files, which is itself evidence nothing was gained or dropped.

Verified by reconstructing all three merge sides byte-for-byte (confirmed via git blob SHA against the upstream blobs) and running a 3-way merge against merge-base 210cd6c: 0 conflict regions, 0 lines dropped from main, 0 lines dropped from this branch, braces balanced. GitHub now reports the PR as no longer conflicting.

One thing worth a maintainer's call

main's new effective_embedder_slug() walks the same resolve_embedder_choice ladder as this PR's resolved_embedder(). They coexist and compile fine, but they aren't equivalent:

  • resolved_embedder() distinguishes OptOut from Unconfigured, and returns openai.name() on the OpenAI-compatible rung
  • effective_embedder_slug() flattens OpenAI-compatible down to "custom"

I deliberately did not unify them here — that's a behaviour decision, not a conflict resolution, and folding it in would widen a diff that is currently a pure move. Happy to follow up in a separate PR if you'd like them consolidated, and equally happy to do it here if you'd rather it not ship as two overlapping accessors.

As before: I still have no Rust toolchain locally, so nothing was compiled or executed — the validation above is structural/textual only. CI on this branch remains the authority, and I'll fix anything it flags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure.

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

memory_tree doctor reports embeddings_unconfigured while embeddings are configured and working

1 participant