[codex] Fix Composio connected integration matching#1229
Conversation
📝 WalkthroughWalkthroughThis PR standardizes Composio connection status checking and toolkit name handling by introducing ChangesComposio Connection Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/openhuman/composio/periodic.rs (1)
174-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCache key mismatch:
duelookup uses normalized toolkit, butrecord_sync_successis called with the rawconn.toolkit.Line 174 builds the dedupe key from the normalized toolkit:
let key = (toolkit.clone(), conn.id.clone()); // normalizedBut on success at line 209 the timestamp is stored under the raw value:
record_sync_success(&conn.toolkit, &conn.id); // raw, e.g. " Slack "For any backend payload where
conn.toolkitdiffers from its normalized form (whitespace, mixed case), the next tick'smap.get(&key)lookup will miss the just-written entry and the scheduler will re-fire on every tick — defeating the per-providersync_interval_secsbudget that this whole module exists to enforce.The same risk exists for the bus path (
src/openhuman/composio/bus.rsline 407 callsrecord_sync_success(&toolkit, …)with the toolkit from the raw event payload).Proposed fix — record under the normalized key and align logging fields
tracing::debug!( - toolkit = %conn.toolkit, + toolkit = %toolkit, + raw_toolkit = %conn.toolkit, connection_id = %conn.id, interval_secs, "[composio:periodic] firing sync" ); match provider.sync(&ctx, SyncReason::Periodic).await { Ok(outcome) => { tracing::debug!( - toolkit = %conn.toolkit, + toolkit = %toolkit, connection_id = %conn.id, items = outcome.items_ingested, elapsed_ms = outcome.elapsed_ms(), "[composio:periodic] sync ok" ); - record_sync_success(&conn.toolkit, &conn.id); + record_sync_success(&toolkit, &conn.id); fired += 1; } Err(e) => { tracing::warn!( - toolkit = %conn.toolkit, + toolkit = %toolkit, connection_id = %conn.id, error = %e, "[composio:periodic] sync failed (will retry next tick)" );And in
bus.rsconsider normalizing before recording, or routing the success record through the same normalized key path used by the scheduler.🤖 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/composio/periodic.rs` around lines 174 - 222, The dedupe key uses the normalized variable toolkit but on success you call record_sync_success(&conn.toolkit, &conn.id) which writes under the raw toolkit string and breaks lookups; change the success path to call record_sync_success(&toolkit, &conn.id) (and update the tracing fields to log the normalized toolkit where appropriate) and apply the same normalization-before-recording fix in the bus path (replace calls that pass the raw toolkit with the normalized toolkit or route through the same normalization helper) so writes and reads use the identical key.src/openhuman/composio/ops.rs (1)
398-413:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
resolve_toolkit_for_connectionreturns the raw toolkit while callers feed it straight intoget_provider.Now that the rest of this module standardizes on
normalized_toolkit()(lines 613, 803, 833), the helper at line 412 is the last raw-toolkit hold-out. Its callers (composio_get_user_profileline 424–426 andcomposio_syncline 475–477) pass the result directly intoget_provider(&toolkit), and the registry is populated with canonical lowercase keys. If the backend ever returns" Slack "for a connection'stoolkit,get_providerwill miss and the user gets"no native provider registered for toolkit ' Slack '"even though the rest of the system treats that connection as a valid Slack one.The
composio_delete_connectionpath also uses this value (line 137) to scope identity-facet deletion and the emittedComposioConnectionDeletedevent — also better off normalized.Proposed fix
- Ok(conn.toolkit) + Ok(conn.normalized_toolkit())🤖 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/composio/ops.rs` around lines 398 - 413, The helper resolve_toolkit_for_connection returns the raw toolkit string; change it to return the normalized form by passing conn.toolkit through normalized_toolkit() before returning so callers (composio_get_user_profile, composio_sync and places that emit ComposioConnectionDeleted) receive the canonical lowercase/trimmed toolkit key; locate resolve_toolkit_for_connection, call normalized_toolkit(conn.toolkit) (or the module's equivalent) and return that result instead of raw conn.toolkit to ensure registry lookups via get_provider(&toolkit) succeed.src/openhuman/memory/slack_ingestion/rpc.rs (1)
78-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign toolkit normalization with
periodic.rs.To maintain consistency across similar code paths, normalize
conn.toolkitbefore passing toProviderContext.src/openhuman/composio/periodic.rs(line 161 onward) extracts and passes the normalized toolkit to the context; the manual trigger path inslack_ingestion/rpc.rscurrently passes the raw value. Even thoughSlackProvider::synchardcodes state operations with the string"slack", normalizing here prevents potential issues if future code relies on consistent context values.Proposed fix
for conn in candidates { + let toolkit = conn.normalized_toolkit(); let ctx = ProviderContext { config: Arc::clone(&config_arc), client: client.clone(), - toolkit: conn.toolkit.clone(), + toolkit, connection_id: Some(conn.id.clone()), };🤖 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/slack_ingestion/rpc.rs` around lines 78 - 94, The manual sync loop constructs ProviderContext with conn.toolkit unnormalized; align it with periodic.rs by normalizing the toolkit first (use the same helper used in periodic.rs—e.g., normalize_toolkit(...) or Toolkit::normalize(...) to produce toolkit_normalized) and pass toolkit: toolkit_normalized into ProviderContext instead of conn.toolkit so the context receives the normalized value.
🧹 Nitpick comments (2)
src/openhuman/composio/ops.rs (1)
799-810: 💤 Low valueOptional: reuse the helper for symmetry with the connection path.
The connection-side derivation at line 833 uses
c.normalized_toolkit(), while the toolkits-allowlist branch here re-implementstrim().to_ascii_lowercase()inline against plainStringvalues. Behaviorally equivalent; just calling out that if you later change the canonicalization rule (e.g. NFC unicode normalization) you'd have two places to update. A small free function intypes.rs(fn normalize_toolkit(s: &str) -> String) used by bothComposioConnection::normalized_toolkitand this loop would keep them in lockstep — fine to defer.🤖 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/composio/ops.rs` around lines 799 - 810, Replace the inline canonicalization in the allowlisted_toolkits creation (the map using trim().to_ascii_lowercase()) with a shared helper so both sides stay consistent: add a fn normalize_toolkit(s: &str) -> String in types.rs, change ComposioConnection::normalized_toolkit to call that helper, and update the client.list_toolkits().await mapping (the allowlisted_toolkits variable) to call normalize_toolkit(&toolkit) instead of re-implementing trim/to_ascii_lowercase.src/openhuman/composio/ops_tests.rs (1)
444-513: 💤 Low valueGood regression coverage; consider an explicit negative assertion for the admin tool.
The
slack.tools.len() == 1+ name check implicitly verifies thatSLACK_DELETE_CHANNELwas filtered out, which is the second half of the PR's claim ("still filtering an admin Slack tool"). To make the intent self-documenting (and avoid a future contributor "fixing" the test by relaxing it when the default scope changes), an explicit assertion that the admin tool is absent would help:Suggested addition
assert!(slack.connected); assert_eq!(slack.tools.len(), 1); assert_eq!(slack.tools[0].name, "SLACK_FETCH_CONVERSATION_HISTORY"); + assert!( + slack.tools.iter().all(|t| t.name != "SLACK_DELETE_CHANNEL"), + "admin-scoped Slack tool must be filtered out under default user scope" + );Also note the test's correctness depends on the default
UserScopePrefblocking admin actions — worth a one-line comment so the dependency is visible.🤖 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/composio/ops_tests.rs` around lines 444 - 513, The test fetch_connected_integrations_treats_slack_and_telegram_status_like_ui should explicitly assert that the admin Slack tool is filtered out: after locating slack (variable slack) add an assertion that none of slack.tools have name "SLACK_DELETE_CHANNEL" (e.g., assert!(!slack.tools.iter().any(|t| t.name == "SLACK_DELETE_CHANNEL"))), and add a one-line comment near the test noting it depends on the default UserScopePref blocking admin actions so the intent is clear.
🤖 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.
Outside diff comments:
In `@src/openhuman/composio/ops.rs`:
- Around line 398-413: The helper resolve_toolkit_for_connection returns the raw
toolkit string; change it to return the normalized form by passing conn.toolkit
through normalized_toolkit() before returning so callers
(composio_get_user_profile, composio_sync and places that emit
ComposioConnectionDeleted) receive the canonical lowercase/trimmed toolkit key;
locate resolve_toolkit_for_connection, call normalized_toolkit(conn.toolkit) (or
the module's equivalent) and return that result instead of raw conn.toolkit to
ensure registry lookups via get_provider(&toolkit) succeed.
In `@src/openhuman/composio/periodic.rs`:
- Around line 174-222: The dedupe key uses the normalized variable toolkit but
on success you call record_sync_success(&conn.toolkit, &conn.id) which writes
under the raw toolkit string and breaks lookups; change the success path to call
record_sync_success(&toolkit, &conn.id) (and update the tracing fields to log
the normalized toolkit where appropriate) and apply the same
normalization-before-recording fix in the bus path (replace calls that pass the
raw toolkit with the normalized toolkit or route through the same normalization
helper) so writes and reads use the identical key.
In `@src/openhuman/memory/slack_ingestion/rpc.rs`:
- Around line 78-94: The manual sync loop constructs ProviderContext with
conn.toolkit unnormalized; align it with periodic.rs by normalizing the toolkit
first (use the same helper used in periodic.rs—e.g., normalize_toolkit(...) or
Toolkit::normalize(...) to produce toolkit_normalized) and pass toolkit:
toolkit_normalized into ProviderContext instead of conn.toolkit so the context
receives the normalized value.
---
Nitpick comments:
In `@src/openhuman/composio/ops_tests.rs`:
- Around line 444-513: The test
fetch_connected_integrations_treats_slack_and_telegram_status_like_ui should
explicitly assert that the admin Slack tool is filtered out: after locating
slack (variable slack) add an assertion that none of slack.tools have name
"SLACK_DELETE_CHANNEL" (e.g., assert!(!slack.tools.iter().any(|t| t.name ==
"SLACK_DELETE_CHANNEL"))), and add a one-line comment near the test noting it
depends on the default UserScopePref blocking admin actions so the intent is
clear.
In `@src/openhuman/composio/ops.rs`:
- Around line 799-810: Replace the inline canonicalization in the
allowlisted_toolkits creation (the map using trim().to_ascii_lowercase()) with a
shared helper so both sides stay consistent: add a fn normalize_toolkit(s: &str)
-> String in types.rs, change ComposioConnection::normalized_toolkit to call
that helper, and update the client.list_toolkits().await mapping (the
allowlisted_toolkits variable) to call normalize_toolkit(&toolkit) instead of
re-implementing trim/to_ascii_lowercase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 903beb7c-488c-4393-8af4-3b317078949a
📒 Files selected for processing (7)
src/openhuman/composio/bus.rssrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_tests.rssrc/openhuman/composio/periodic.rssrc/openhuman/composio/tools.rssrc/openhuman/composio/types.rssrc/openhuman/memory/slack_ingestion/rpc.rs
Co-authored-by: Jwalin Shah <jshah1331@gmail.com>
3 participants
Summary
Problem
Solution
ComposioConnection::is_active()andComposioConnection::normalized_toolkit()as the boundary helpers for external Composio strings.Submission Checklist
docs/TESTING-STRATEGY.md## Related— no matrix feature ID identified for this narrow bug fix.docs/TESTING-STRATEGY.md)Closes #NNNin the## RelatedsectionImpact
pnpm debug rust composio -- --nocaptureandcargo check --manifest-path Cargo.tomlpassed earlier on this branch; no further Rust validation was run after machine-memory constraints were identified.Related
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor