fix(embeddings): read managed session token from config-scoped store - #5363
Conversation
The managed/cloud embedder was constructed with a hardcoded (None state_dir, encrypt=true), so its bearer resolver read the root ~/.openhuman/auth-profiles.json via default_state_dir() instead of the user-scoped ~/.openhuman/users/<uid>/ where sign-in stores the app-session token. On a shipped desktop (OPENHUMAN_WORKSPACE unset) this made the Embeddings-tab "Test connection" report "No backend session" for an already signed-in user. Add create_embedding_provider_with_config(config, ...) that threads config.config_path.parent() + config.secrets.encrypt for managed/cloud (mirroring AuthService::from_config and the memory-tree build_cloud_embedder) and delegates every other provider unchanged. The managed RPC paths (test_connection, embed, provider_from_config) now route through it. Closes tinyhumansai#5356
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds configuration-aware embedding-provider construction. Managed and cloud providers use configured credential scope and encryption settings. Embedding RPC operations now use this factory, with tests covering session recovery, provider creation, delegation, and error handling. ChangesEmbedding provider configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EmbeddingsRPC
participant ConfigAwareFactory
participant CredentialStore
participant ManagedProvider
EmbeddingsRPC->>ConfigAwareFactory: construct provider with Config
ConfigAwareFactory->>CredentialStore: resolve configured credential scope
CredentialStore-->>ConfigAwareFactory: return app-session token
ConfigAwareFactory->>ManagedProvider: create managed or cloud provider
ManagedProvider-->>EmbeddingsRPC: execute embed or connection test
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
|
| Filename | Overview |
|---|---|
| src/openhuman/inference/embeddings/factory.rs | Adds create_embedding_provider_with_config and private managed_credential_scope; for managed/cloud providers derives credential scope from config via state_dir_from_config (mirroring AuthService::from_config); seven new tests cover the regression, round-trip, and delegation paths. |
| src/openhuman/inference/embeddings/mod.rs | Re-exports the new create_embedding_provider_with_config function; trivial change with no logic of its own. |
| src/openhuman/inference/embeddings/rpc.rs | Routes embed, test_connection, and build_embedder through create_embedding_provider_with_config instead of create_embedding_provider_with_credentials; adds three new RPC-level tests for the managed path. |
Sequence Diagram
sequenceDiagram
participant UI as Frontend
participant RPC as rpc.rs
participant Factory as factory.rs
participant Cloud as OpenHumanCloudEmbedding
participant Store as auth-profiles.json
UI->>RPC: test_connection / embed (managed)
Note over RPC,Factory: BEFORE: hardcoded (None, None, true)
RPC->>Cloud: new(None, None, true, model, dims)
Cloud->>Store: reads root ~/.openhuman (wrong path)
Store-->>Cloud: not found
Cloud-->>RPC: No backend session
Note over RPC,Factory: AFTER: create_embedding_provider_with_config
RPC->>Factory: create_embedding_provider_with_config(config, managed)
Factory->>Factory: "managed_credential_scope -> state_dir_from_config"
Factory->>Cloud: new(None, Some(config_dir), encrypt, model, dims)
Cloud->>Store: reads config-scoped dir (correct path)
Store-->>Cloud: app-session token found
Cloud-->>RPC: embed vectors
RPC-->>UI: success
Reviews (3): Last reviewed commit: "test(embeddings): bind managed factory o..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4dad0c7c21
ℹ️ 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: 1
🤖 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 `@src/openhuman/inference/embeddings/rpc.rs`:
- Around line 1495-1545: Update the managed regression tests around
test_connection_managed_without_session_reports_no_backend_session,
embed_managed_without_session_errors_with_no_backend_session, and
provider_from_config_managed_builds_cloud to isolate the default credential
scope and store an app-session only under the TempDir-backed configuration
scope. Keep the tests offline, but assert via a mock or injected bearer-token
resolver that managed provider construction reads the scoped token, so the tests
fail if config is ignored; preserve coverage for missing-session errors and
cloud-provider construction.
🪄 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: 5bd2223b-2161-498b-915f-7e808d9ae710
📒 Files selected for processing (3)
src/openhuman/inference/embeddings/factory.rssrc/openhuman/inference/embeddings/mod.rssrc/openhuman/inference/embeddings/rpc.rs
- managed_credential_scope now delegates to state_dir_from_config (the exact helper AuthService::from_config uses), so it inherits the "." fallback when config_path has no parent and the "mirrors exactly" invariant holds — and stays DRY. - Stop logging the user-scoped credential-store path (it embeds the OS username and/or users/<uid>); log only the non-identifying encrypt flag. - Strengthen the config-scope regression test with an isolation half: the app-session token stored under the config scope must NOT resolve from a default_state_dir-like scope, so a managed construction that ignores config fails the test.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Add factory_managed_provider_authenticates_with_config_scoped_token: build the managed provider through create_embedding_provider_with_config against a local mock cloud backend, and assert it authenticates with the app-session token stored under the config scope. A regression to OpenHumanCloudEmbedding::new(None, None, true, ...) would resolve no token from default_state_dir() and fail with "No backend session" before any request, so this pins the factory -> managed_credential_scope binding offline (no external network; BACKEND_URL points at the mock under the shared backend-env test lock).
2 participants
Summary
app-sessionbearer token from the same config-scoped credential store that sign-in writes to, instead of a hardcoded root path.Problem
No backend session for cloud embeddings: log in to OpenHuman, even with a valid active session (Google sign-in succeeds but connection test in Embeddings tab still shows sign-in error #5356).OpenHumanCloudEmbedding::new(None, None, true, …). Its bearer resolver therefore read~/.openhuman/auth-profiles.json(root, viadefault_state_dir()), but sign-in stores theapp-sessiontoken under the user-scoped~/.openhuman/users/<uid>/auth-profiles.json(viaAuthService::from_config). WithOPENHUMAN_WORKSPACEunset in the shipped app, the two directories never coincide, so the token was never found.test_connection, liveembed, andprovider_from_configwere all affected. The memory-tree path (build_cloud_embedder) already passed the config-derived values and was unaffected — which is why memory ingest/recall worked while the Embeddings-tab test did not.Solution
create_embedding_provider_with_config(config, …)insrc/openhuman/inference/embeddings/factory.rs. Formanaged/cloudit threadsconfig.config_path.parent()+config.secrets.encrypt(mirroringAuthService::from_configand the existing memory-treebuild_cloud_embedder); every other provider delegates tocreate_embedding_provider_with_credentialsunchanged.test_connection, liveembed, andprovider_from_config/build_embedder— through it.managed_credential_scope(config)as a pure fn so the(dir, encrypt)invariant is unit-testable without a network round-trip.isLocalSessioncorrectly reflects a remote session) and clears the banner/error once the backend returns success.Submission Checklist
test_connection/embedwith no session report the backend-session error, non-managed delegation, and unknown-provider error.cargo test --lib inference::embeddings(68/68 pass). Fulldiff-coverruns in CI-Lite.docs/TEST-COVERAGE-MATRIX.md.Closes #5356in## Related.Impact
embed) for signed-in users. Memory ingest/recall was already correct and is unchanged.(dir, encrypt)scope sign-in uses; no new secret exposure and no secrets logged (the added debug line logs only the directory + encrypt flag). Non-managed providers are byte-for-byte unchanged.Related
(None, None, true)hardcode for runtime memory embedding —memory/store/factories.rs,memory/store/client.rs(viadefault_embedding_provider), andagent/experience/ops.rs. These are a separate subsystem from the Embeddings tab and will be threaded through a config-aware constructor in a follow-up PR.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit