fix(composio): enrich connection picker with cached account identity (#3356)#3400
Conversation
Closes tinyhumansai#3356 (remaining part) After the initial fix that hid raw connection IDs, the picker still showed generic "Account 1 / Account 2 / Account 3" labels when multiple accounts existed for the same provider (Gmail, Slack, GitHub, etc.). ## What changed - `ComposioConnection` (`types.rs`) gains three new optional fields: `account_email`, `workspace`, `username` — matching the camelCase wire names the TypeScript frontend type already declared. - `enrich_connections_with_identity()` (`ops.rs`) — a zero-latency enrichment pass called after every `composio_list_connections` fetch (both backend-proxied and direct-mode paths). It reads the persisted provider profile cache (`load_connected_identities()`) and joins each connection row by `(toolkit, connection_id)`, populating: · `account_email` ← identity.email (Gmail, Google Sheets, …) · `workspace` ← identity.display_name (Slack user / team name) · `username` ← identity.handle (GitHub, Twitter, …) No live API calls — profile rows are already written by `persist_provider_profile` on each successful sync. - `client.rs` struct initialiser updated with the new `None` fields. - Unit tests (`ops_tests.rs`) cover: no-cache passthrough, email enrichment, handle enrichment, pre-populated identity preservation, multi-account same-toolkit, and unmatched-connection fallback. ## Result Users with multiple connected accounts for the same provider now see readable labels (e.g. "Gmail · alice@example.com") in the picker instead of "Account 1 / Account 2 / Account 3".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughComposioConnection adds optional identity/display fields (account_email, workspace, username). Direct responses initialize them as None; both Direct and Backend listing paths call a new helper that best-effort enriches those fields from cached provider profiles before returning. ChangesIdentity Field Enrichment
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant RPC as composio_list_connections
participant Cache as ProviderProfileCache
participant Enricher as enrich_connections_with_identity
participant Response as API Response
Client->>RPC: list connections request
RPC->>Cache: fetch cached provider profiles
RPC->>Enricher: pass connections + cached profiles
Enricher->>Enricher: build (source, identifier) lookup
Enricher->>Enricher: populate account_email / workspace / username where missing
Enricher->>RPC: return enriched connections
RPC->>Response: return enriched ComposioConnectionsResponse
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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/composio/ops_tests.rs`:
- Around line 2270-2273: init_memory_client currently calls
crate::openhuman::memory::global::init(ws) but drops the resulting guard
immediately, allowing other tests to rebind global memory concurrently; change
init_memory_client to return the guard type produced by
crate::openhuman::memory::global::init (propagate its concrete type or use impl
Drop/Box<...> if needed) and update tests to assign the return value to a local
binding (e.g., let _guard = init_memory_client(tmp.path());) so the guard lives
for the duration of the test and prevents parallel rebinds.
🪄 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: 642abed9-b4b3-4732-ad4f-571e6d4d2880
📒 Files selected for processing (4)
src/openhuman/composio/client.rssrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_tests.rssrc/openhuman/composio/types.rs
CI (Rust Core Coverage) caught 5 struct initializers in tests that were missing the new account_email/workspace/username fields added in the previous commit: - src/openhuman/composio/types.rs (4 test helpers) - src/openhuman/heartbeat/planner/collectors.rs (1 test helper)
… test tests/composio_raw_coverage_e2e.rs:1301 was missing account_email, workspace, and username fields added in the identity enrichment change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/composio_raw_coverage_e2e.rs (1)
1310-1313: 💤 Low valueOptional: Consider verifying identity fields' serialization behavior.
The test validates that
createdAtis absent from the serialized JSON whenNone. For consistency and completeness, consider adding similar assertions for the new identity fields (accountEmail,workspace,username) to verify they also skip serialization whenNone.✨ Optional test enhancement
assert!(serde_json::to_value(default_connection) .unwrap() .get("createdAt") .is_none()); + let serialized = serde_json::to_value(default_connection).unwrap(); + assert!(serialized.get("createdAt").is_none()); + assert!(serialized.get("accountEmail").is_none()); + assert!(serialized.get("workspace").is_none()); + assert!(serialized.get("username").is_none());🤖 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 `@tests/composio_raw_coverage_e2e.rs` around lines 1310 - 1313, Add assertions in the same test that verify the identity fields skip serialization when None: after the existing createdAt check, assert that serde_json::to_value(default_connection).unwrap().get("accountEmail").is_none(), and likewise for "workspace" and "username". Locate the test using the default_connection variable (in tests/composio_raw_coverage_e2e.rs) and add these three assertions using the same pattern to ensure consistency.
🤖 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 `@tests/composio_raw_coverage_e2e.rs`:
- Around line 1310-1313: Add assertions in the same test that verify the
identity fields skip serialization when None: after the existing createdAt
check, assert that
serde_json::to_value(default_connection).unwrap().get("accountEmail").is_none(),
and likewise for "workspace" and "username". Locate the test using the
default_connection variable (in tests/composio_raw_coverage_e2e.rs) and add
these three assertions using the same pattern to ensure consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31baff25-c3ca-4bec-a5a8-7e5167ce6674
📒 Files selected for processing (1)
tests/composio_raw_coverage_e2e.rs
init_memory_client now returns a MutexGuard<'static, ()> that callers hold for the duration of the test, preventing concurrent tests from rebinding the global memory client and cross-contaminating assertions. Addresses CodeRabbit review comment on unsynchronized global rebind.
memory::global::init spawns async tasks internally, which requires a Tokio runtime context. Tests using plain #[test] panicked with "there is no reactor running". All 6 enrichment tests now use #[tokio::test] + async fn, matching the pattern in memory::global.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed the cached-identity enrichment. Solid, additive change with good test coverage and correct wire alignment to the existing TS ComposioConnection type (accountEmail/workspace/username) and the picker's email → workspace → username fallback. Three notes inline — only the first is worth fixing before merge.
| if conn.account_email.is_some() || conn.workspace.is_some() || conn.username.is_some() { | ||
| continue; | ||
| } | ||
| let toolkit_key = conn.toolkit.trim().to_ascii_lowercase(); |
There was a problem hiding this comment.
Latent bug — asymmetric key normalization. The lookup is keyed by what persist_provider_profile stores, which is normalize_token() on both segments (profile.rs:142,146). Here the toolkit side uses to_ascii_lowercase() while the conn-id side (next line) uses normalize_connection_identifier (= normalize_token). These differ: normalize_token also maps any non-[a-z0-9_-] char → _ and trims. For today's alphanumeric Composio slugs (gmail, github, slack, notion) they coincide, so it works now — but it silently breaks the join for any future slug containing a ./space/+.
Use the same function on both sides for symmetry:
let toolkit_key = normalize_connection_identifier(&conn.toolkit);One-line change that removes the footgun.
There was a problem hiding this comment.
Fixed in d1e8f4f — changed to normalize_connection_identifier(&conn.toolkit) so both sides of the lookup key use the same normalization as persist_provider_profile. Good catch on the future-slug footgun.
| /// persisted provider profile cache so the UI picker can show | ||
| /// "Gmail · user@example.com" instead of a generic "Account N" label. | ||
| /// | ||
| /// This is best-effort and zero-latency — no live API calls are made. |
There was a problem hiding this comment.
Nit on wording: "zero-latency" is true for the network (no live API calls — good), but each poll now does one extra SQLite read via load_connected_identities() (profile_facets_by_type). Since composio_list_connections is a polling RPC, the precise claim is "no live API calls" rather than "zero-latency." Negligible cost, just tightening the doc.
There was a problem hiding this comment.
Fixed in d1e8f4f — updated to "no live API calls (one SQLite read per poll)".
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn enrich_does_nothing_when_no_cached_identities() { |
There was a problem hiding this comment.
Test-isolation nit: this test relies on the global memory client being un-initialized, but the sibling #[tokio::test]s init the process-global singleton to a temp workspace and run in parallel — without holding ENRICH_IDENTITY_TEST_LOCK here, the "no cached identities" premise isn't actually guaranteed. The assertion still passes (id "c1" won't match any persisted row), so it's not flaky in practice, but the test name overstates what it proves. Either hold the lock, or rename to reflect that it asserts the unmatched-id fallback.
There was a problem hiding this comment.
Fixed in d1e8f4f — the test now calls let _guard = init_memory_client(tmp.path()); so it holds ENRICH_IDENTITY_TEST_LOCK for its full duration. The fresh temp workspace has no profiles, so load_connected_identities returns empty and the assertion holds without relying on un-initialized state.
…test isolation - ops.rs: use normalize_connection_identifier(&conn.toolkit) instead of to_ascii_lowercase() so both sides of the lookup key use the same normalization as persist_provider_profile (fixes asymmetry flagged by M3gA-Mind that would silently break slugs containing '.'/space/'+') - ops.rs: fix doc comment — 'zero-latency' → 'no live API calls' - ops_tests.rs: hold ENRICH_IDENTITY_TEST_LOCK in enrich_does_nothing_when_no_cached_identities so it can't race with sibling tests that rebind the global memory client
2 participants
Summary
Closes #3356 (remaining part — account identity clarity after the initial raw-ID fix).
After the initial fix (#3356) hid raw connection IDs, the picker still showed generic "Account 1 / Account 2 / Account 3" labels when multiple accounts existed for the same provider. This PR surfaces the readable identity info that is already cached from provider syncs.
Changes
ComposioConnection(types.rs) — adds three optional fields matching the camelCase wire names the TypeScript frontend already declared:accountEmail,workspace,username.enrich_connections_with_identity()(ops.rs) — a zero-latency enrichment pass called after everycomposio_list_connectionsfetch (both backend-proxied and direct-mode paths). Reads the persisted provider profile cache (load_connected_identities()) and joins each connection row by(toolkit, connection_id):account_email← cached email (Gmail, Google Sheets, Google Calendar, …)workspace← cached display_name (Slack user/team name, Notion, …)username← cached handle (GitHub, Twitter, …)persist_provider_profileon each successful sync.client.rs— struct initializer updated with the newNonefields.ops_tests.rs— unit tests: no-cache passthrough, email enrichment, handle enrichment, pre-populated identity preservation, multi-account same-toolkit disambiguation, unmatched-connection fallback.Result
Summary by CodeRabbit
New Features
Tests