fix(embeddings): scope the memory client's default embedder to config credentials - #5515
Conversation
… credentials The namespace-document ingest tier builds its inline embedder through the keyless `default_embedding_provider()`, which hardcodes `(openhuman_dir=None, secrets_encrypt=true)`. That resolves the managed session from `default_state_dir()` with encryption forced on — a scope that diverges from where sign-in actually wrote the `app-session` token whenever the config disables secret encryption or roots the workspace/user elsewhere than the process default. "Test connection" builds through the config-aware `create_embedding_provider_with_config` (managed_credential_scope) and passes, while every ingest embed then fails with "No backend session" and the document persists vector-less — so a signed-in user's ingested memory is silently unsearchable. tinyhumansai#5427 fixed the config-aware path but left this keyless one. Add `default_embedding_provider_with_config(config)` — routes managed construction through `managed_credential_scope`, the same (state_dir, encrypt) scope Test connection uses — and call it from the two config-holding callers: the memory-client embedding-host seam (`OpenHumanEmbeddingHost::default_embedding_provider`) and the agent-experience store. The keyless variant stays for genuinely config-less callers, doc-warned as a best-effort fallback. Adds an e2e binding test (axum mock captures the bearer) proving the config-scoped default embedder authenticates with the app-session token sign-in stored.
|
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)
📝 WalkthroughWalkthroughEmbedding provider creation now accepts ChangesConfig-scoped embedding providers
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR routes config-holding embedding callers through the configured credential scope while retaining the keyless fallback; no actionable merge-blocking risk remains at the current head, so it is merge-ready after normal checks. Sequence Diagram(s)sequenceDiagram
participant Config
participant EmbeddingFactory
participant CredentialStore
participant EmbeddingServer
Config->>EmbeddingFactory: provide credential scope and model settings
EmbeddingFactory->>CredentialStore: read config-scoped app-session token
CredentialStore-->>EmbeddingFactory: return session token
EmbeddingFactory->>EmbeddingServer: send embedding request with bearer token
EmbeddingServer-->>EmbeddingFactory: return default-sized vectors
Possibly related PRs
Suggested labels: Suggested reviewers: 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 |
What this change touches5 files, +152 -4 across 3 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src/openhuman/inference/embeddings<br/>3 files +139 -2"]:::changed
n1["src/openhuman/memory<br/>1 file +9 -1"]:::changed
n2["src/openhuman/agent/experience<br/>1 file +4 -1"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0372 · 32,342 in / 9,807 out · 17,111 cached (53%) · z-ai/glm-5.2
critique: $0.0183 · 12,286 in / 5,191 out · 5,521 cached (45%) · z-ai/glm-5.2
security: $0.0068 · 9,394 in / 1,685 out · 6,959 cached (74%) · z-ai/glm-5.2
tests: $0.0070 · 4,377 in / 2,420 out · 3,351 cached (77%) · z-ai/glm-5.2
description: $0.0052 · 6,285 in / 511 out · 1,280 cached (20%) · z-ai/glm-5.2
…ngs-batch-skip-5501
…ory v1.0.1 Same pre-existing broken-main as folded into tinyhumansai#5528: the inference coverage lane runs the whole openhuman::inference namespace, and tinymemory v1.0.1 changed the effective-embedder ladder (local Ollama now resolves from an explicit memory_tree.embedding_endpoint override or the unified workload_local_model setting, not the embeddings_provider string alone). The submodule bump landed on main without updating this test. Drive the deterministic explicit-endpoint rung so the test keeps its tinyhumansai#5402 assertion. Not caused by this PR; fails identically on plain main.
Summary
default_embedding_provider_with_config(config)and route the two config-holding callers (the memory-client embedding-host seam and the agent-experience store) through it, so the ingest embedder resolves the same credential scope "Test connection" uses.default_embedding_provider()is retained for genuinely config-less callers, doc-warned as a best-effort fallback.Problem
After #5427 fixed the managed sign-in flow, "Test connection" passes and raw ingestion completes — but the embedding pass produces no vectors and memory search returns nothing, with no error shown to the user (#5501).
Root cause: the namespace-document ingest tier builds its inline embedder via the keyless
default_embedding_provider(), hardcodedOpenHumanCloudEmbedding::new(openhuman_dir = None, secrets_encrypt = true, …).openhuman_dir = Noneresolves the managed session fromdefault_state_dir()with encryption forced on — a scope that diverges from where sign-in actually wrote theapp-sessiontoken whenever the config disables secret encryption or roots the workspace/user elsewhere than the process default. "Test connection" instead builds throughcreate_embedding_provider_with_config→managed_credential_scope(config), so the two paths disagree on where the token lives. The memory client caches its embedder process-globally, so the wrong scope sticks for the process lifetime. #5427 fixed the config-aware path but left this keyless one.Solution
default_embedding_provider_with_config(config)builds the managed cloud embedder throughmanaged_credential_scope(config)— the exact(state_dir, encrypt)pairAuthService::from_configwrites theapp-sessiontoken to at sign-in.OpenHumanEmbeddingHost::default_embedding_provider(the process-global-cached memory-client seam,MemoryClient::from_workspace_dir) andagent::experience::ops::open_store_in_subdirnow use the config-aware constructor via&self.config/config.cloud_embedding_providerseam that the memory-tree tier uses, so both tiers now resolve managed credentials identically.Submission Checklist
default_provider_with_config_authenticates_with_config_scoped_token(local axum mock captures the bearer, asserts the config-scopedapp-sessiontoken is used); its failure mode is exactly the divergent-scope regression this PR fixes. Existingmanaged_scope_resolves_signin_stored_app_session_tokenround-trip retained.Rust Core Coverage/coverage-gatejobs confirm the merged-lcov threshold on changed lines.## Related— N/A: no matrix rows change.Closes #NNNin the## Relatedsection.Impact
encryptflag is logged, never the path. No new network dependency. No migration.Related
tinymemory-coredocument ingest path (stores chunks vector-less and returnsOkon embed failure) and the "log visibly / do not mark fully processed on failure" acceptance criteria belong in a separatetinymemorychange + submodule gitlink bump (not editable from this repo).CI note — pre-existing failures inherited from
mainThe red checks on this PR are not produced by this diff, which touches only four
src/openhuman/**files (noCargo.toml/Cargo.lock, noconfig/**):cannot update the lock file app/src-tauri/Cargo.lock because --locked. This branch'sapp/src-tauri/Cargo.lockis byte-identical tomain(untouched here) and the error reproduces locally on a clean checkout — it is a lock-drift onmain.config::migration_helpers::opstests, code this PR does not touch.Both jobs fail identically on
main's own latest CI Lite run (base commit #5492), so they are pre-existing breakage, surfaced here rather than worked around. They should be fixed onmain/ in dedicated PRs, not papered over in this one.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/managed-embeddings-batch-skip-5501934cbefe3Validation Run
pnpm --filter openhuman-app format:check: noapp/frontend changes.pnpm typecheck: no TypeScript changed.cargo test --lib inference::embeddings::factory(6/6),agent::experience(29/29),memory::seam_integration(8/8).cargo fmt --checkclean;cargo clippyclean on the touched files.app/src-taurichange.Validation Blocked
command:fulldiff-cover+ fullcargo test --liberror:not run locally (long); thememory::driver::embeddedsubtree fails in isolation with "no EmbeddingHost installed" because those tests rely on a process-global seam that only another test installs in the full suite — verified identical pass/fail counts with this change stashed, so it is a pre-existing test-ordering artifact unaffected by this PR.impact:CI runs the full suite + diff-cover gate.Behavior Changes
Parity Contract
default_embedding_provider()is unchanged for config-less callers; non-managed providers (Ollama/custom/voyage/none) are untouched.cloud_embedding_providerseam already used by the memory-tree tier.Duplicate / Superseded PR Handling
Summary by CodeRabbit