fix(composio): forward tags filter in direct (BYO-key) tool listing#2772
Conversation
The `tags` query param added in tinyhumansai#2714 was only wired through the backend-proxied path. In direct (BYO Composio key) mode `composio_list_tools` computed `effective_tags` and then dropped it on the direct branch, so a self-key user filtering tools by tag silently got the unfiltered set. Thread `tags` through `direct_list_tools` -> `list_tool_schemas_v3`, encoding it as repeated `tags=` params per the Composio v3 `/tools` contract (vs the backend proxy's comma-joined `tags=a,b`). The `should_forward_tags` gate is unchanged, so behaviour stays consistent with backend mode (GitHub-only today). Also add an injectable v3 base URL to `ComposioTool` (`new_with_v3_base`; prod `new` delegates with the real const) so the direct `/tools` path is HTTP-testable against a local axum mock — removing the documented "can't mock direct mode" gap in client_tests.rs. Tests: 6 query-builder unit tests + 2 axum-mock HTTP tests asserting the repeated-`tags=` wire shape and the v3 -> backend envelope reshape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR extends Composio tool schema listing to support action tag filtering in direct-mode. ComposioTool gains a configurable v3 base URL and query builder for tag-aware requests. The direct-mode client forwards tags to the v3 endpoint, and the operations layer threads effective tags through the request stack with improved error reporting. ChangesComposio tag filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/tools/impl/network/composio.rs (1)
65-70: ⚡ Quick winConstrain the injectable v3 base-URL seam to tests only.
new_with_v3_baseis crate-visible andlist_tool_schemas_v3always attachesx-api-keyto whateverbase_v3is provided. Since this seam exists for local mock tests, consider test-gating it (or routing through a private internal constructor) to prevent accidental non-HTTPS use in non-test code.💡 Suggested hardening approach
impl ComposioTool { + fn new_internal( + api_key: &str, + default_entity_id: Option<&str>, + security: Arc<SecurityPolicy>, + base_v3: String, + ) -> Self { + // existing constructor body + } + pub fn new( api_key: &str, default_entity_id: Option<&str>, security: Arc<SecurityPolicy>, ) -> Self { - Self::new_with_v3_base( + Self::new_internal( api_key, default_entity_id, security, COMPOSIO_API_BASE_V3.to_string(), ) } + #[cfg(test)] pub(crate) fn new_with_v3_base( api_key: &str, default_entity_id: Option<&str>, security: Arc<SecurityPolicy>, base_v3: String, ) -> Self { - // existing constructor body + Self::new_internal(api_key, default_entity_id, security, base_v3) } }Also applies to: 231-243
🤖 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/tools/impl/network/composio.rs` around lines 65 - 70, new_with_v3_base is currently crate-visible and allows injecting an arbitrary base_v3 that list_tool_schemas_v3 will call with an x-api-key header, creating a seam that could be used non-HTTPS in production; restrict this by making the injectable base constructor test-only: change new_with_v3_base to a private/internal constructor and expose a #[cfg(test)] pub(crate) fn new_with_v3_base (or alternatively annotate the existing fn with #[cfg(test)]) so only tests can pass a custom base_v3, and keep the public API using the normal secure constructor that enforces HTTPS; update any test call sites to use the test-only constructor and leave SecurityPolicy and list_tool_schemas_v3 unchanged.
🤖 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/tools/impl/network/composio.rs`:
- Around line 65-70: new_with_v3_base is currently crate-visible and allows
injecting an arbitrary base_v3 that list_tool_schemas_v3 will call with an
x-api-key header, creating a seam that could be used non-HTTPS in production;
restrict this by making the injectable base constructor test-only: change
new_with_v3_base to a private/internal constructor and expose a #[cfg(test)]
pub(crate) fn new_with_v3_base (or alternatively annotate the existing fn with
#[cfg(test)]) so only tests can pass a custom base_v3, and keep the public API
using the normal secure constructor that enforces HTTPS; update any test call
sites to use the test-only constructor and leave SecurityPolicy and
list_tool_schemas_v3 unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 974e5e61-57e8-4b40-9b79-5d18a6c1ecfa
📒 Files selected for processing (5)
src/openhuman/composio/client.rssrc/openhuman/composio/client_tests.rssrc/openhuman/composio/ops.rssrc/openhuman/tools/impl/network/composio.rssrc/openhuman/tools/impl/network/composio_tests.rs
Address CodeRabbit review nitpick: `new_with_v3_base` was `pub(crate)` and not test-gated, so in principle production code could inject a non-HTTPS base that `list_tool_schemas_v3` would still send `x-api-key` to. Route the shared constructor body through a private `new_internal`, keep `new` (the only production entry point) pinned to the HTTPS `COMPOSIO_API_BASE_V3` const, and `#[cfg(test)]`-gate `new_with_v3_base` so only tests can supply a custom base. Behaviour and the v3 `/tools` request shape are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — addressed the v3 base-URL seam nitpick in |
|
Tip For best results, initiate chat on the files or code changes.
(๑˃ᴗ˂)ﻭ 🐰 |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Fix is correct and well-tested overall. Two items worth addressing before merge — one is a real documentation bug, one is a minor test style nit.
| /// raw `input_parameters` JSON schema each action carries. | ||
| /// | ||
| /// Build the query-parameter pairs for the Composio v3 `GET /tools` | ||
| /// listing used by [`Self::list_tool_schemas_v3`]. |
There was a problem hiding this comment.
Doc ordering bug. The three context lines immediately above this insertion point (/// List v3 tool definitions..., /// raw \input_parameters`..., ///) are now contiguous with this new doc block, so rustdoc attaches **all of them** to build_list_tool_schemas_v3_queryinstead of tolist_tool_schemas_v3. The list_tool_schemas_v3method ends up documented only from/// Sibling of...`.
Fix: the /// List v3 tool definitions... lines belong on list_tool_schemas_v3. Either move them to just above /// Sibling of..., or drop them (the /// Sibling of... opening is sufficient). The new build_list_tool_schemas_v3_query doc should start clean with /// Build the query-parameter pairs... only.
There was a problem hiding this comment.
Good catch — fixed in 048ef3a6. Moved the List v3 tool definitions… preamble back onto list_tool_schemas_v3 (above the Sibling of… block); build_list_tool_schemas_v3_query now opens cleanly with Build the query-parameter pairs…. Verified rustdoc attaches each block to the right item.
| }), | ||
| ); | ||
| let base = start_mock_backend(app).await; | ||
| let tool = direct_tool_for_mock(base); |
There was a problem hiding this comment.
Minor nit: this test uses start_mock_backend(app).await while the sibling test added in composio_tests.rs (list_tool_schemas_v3_sends_repeated_tags_to_v3_tools_endpoint) uses inline TcpListener::bind + tokio::spawn. Both cover the same HTTP layer from different call depths. Not a blocker, but two setup styles added in the same PR gives future authors no clear model to copy — worth aligning in a follow-up.
There was a problem hiding this comment.
Aligned in 048ef3a6 rather than deferring: added a module-local start_mock_backend helper in composio_tests.rs mirroring the one in client_tests.rs, so both HTTP tests now read let base = start_mock_backend(app).await;. One consistent model to copy.
Address review feedback (M3gA-Mind): - Doc-ordering bug: inserting `build_list_tool_schemas_v3_query` had stranded `list_tool_schemas_v3`'s "List v3 tool definitions…" preamble onto the new helper. Move that preamble back above the method's "Sibling of…" block; the helper doc now opens cleanly with "Build the query-parameter pairs…". - Test style: add a module-local `start_mock_backend` helper in composio_tests.rs mirroring client_tests.rs, so both HTTP-level direct-mode tests share one mock-setup model instead of two styles. No behaviour or wire-shape change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #2772 — fix(composio): forward tags filter in direct (BYO-key) tool listing
Walkthrough
This PR closes a silent parity gap introduced by #2714: the tags filter on composio_list_tools was only wired through the backend-proxied path; in direct (BYO-key) mode effective_tags was computed but then dropped, so self-key users got the full unfiltered tool set regardless. The fix threads tags through direct_list_tools → ComposioTool::list_tool_schemas_v3, encoding it as repeated tags= query params (matching the Composio v3 /tools contract) rather than the comma-joined form the backend proxy uses. A new #[cfg(test)]-gated new_with_v3_base constructor gives list_tool_schemas_v3 an injectable base URL so the direct HTTP path — including the tags wire shape — can be asserted against a local axum mock without touching production infrastructure.
The implementation is clean, the should_forward_tags gate is left untouched, and the 8 new tests cover both the pure query-builder logic and the end-to-end HTTP round trip. The fix is core-only with no frontend or Tauri shell impact.
Changes
| File | Summary |
|---|---|
src/openhuman/composio/client.rs |
direct_list_tools gains a tags: Option<&[String]> param; converts to &[&str] refs and passes them through to list_tool_schemas_v3. Updated docs. |
src/openhuman/composio/client_tests.rs |
New axum-mock HTTP test for direct_list_tools exercising repeated tags= params and the empty-slug reshaper. New direct_tool_for_mock helper. Updated block comment. |
src/openhuman/composio/ops.rs |
One-line change in the direct branch: passes effective_tags.as_deref() (already computed) instead of nothing. Adds ?effective_tags to the pre-call debug log. |
src/openhuman/tools/impl/network/composio.rs |
ComposioTool gains base_v3: String field; new refactored into new → new_internal; #[cfg(test)] new_with_v3_base added. list_tool_schemas_v3 accepts tags, delegates param building to new pure helper build_list_tool_schemas_v3_query. |
src/openhuman/tools/impl/network/composio_tests.rs |
6 unit tests for build_list_tool_schemas_v3_query + 1 HTTP mock test for list_tool_schemas_v3. New start_mock_backend helper. |
Actionable comments (1)
💡 Refactor / suggestion
1. src/openhuman/tools/impl/network/composio.rs:248 — ensure_https not guarded in list_tool_schemas_v3
list_connected_accounts (line 522) and execute_action_v3 (line 344) both call ensure_https(&url)? before sending the x-api-key header. list_tool_schemas_v3 does not, now that url is built from the injectable self.base_v3 rather than the HTTPS const.
The production safety argument is sound — new_with_v3_base is #[cfg(test)]-gated so an HTTP base can't be supplied in a release build. But the invariant is only compiler-enforced at the constructor level, not at the call site. If a future refactor widens new_with_v3_base's visibility (e.g. during an integration test restructure), list_tool_schemas_v3 will silently transmit the API key over HTTP while list_connected_accounts and execute_action_v3 would still catch it.
Adding the guard conditionally keeps the test seam working and makes the guarantee explicit:
// Option A: guard at the call site
let url = format!("{}/tools", self.base_v3);
#[cfg(not(test))]
ensure_https(&url)?;// Option B: guard once in new_internal so no call site needs it
fn new_internal(..., base_v3: String) -> Self {
#[cfg(not(test))]
ensure_https(&base_v3).expect("base_v3 must use HTTPS in production");
// ...
}Not a merge blocker, but either option makes the constraint explicit rather than relying solely on the access-modifier comment.
Nitpicks (2)
-
src/openhuman/tools/impl/network/composio_tests.rs:8-18—start_mock_backendis duplicated verbatim fromclient_tests.rs. The modules can't share code directly given the current layout, which makes the duplication unavoidable, but the doc comment correctly calls it out. If the helpers ever diverge (e.g. a timeout knob), having two copies will be a quiet footgun. -
src/openhuman/tools/impl/network/composio.rs:36-42— Thebase_v3field doc says "Base URL for Composio v3 endpoints" butlist_actions_v3(line 136) still interpolates{COMPOSIO_API_BASE_V3}directly — it doesn't useself.base_v3. A reader maintaininglist_actions_v3tests might reasonably expectnew_with_v3_baseto intercept that path too, and be confused when it doesn't. Narrowing the doc to "Base URL used bylist_tool_schemas_v3's/toolsGET" (or a note thatlist_actions_v3is excluded) would prevent that misdirection.
Verified / looks good
effective_tags.as_deref()correctly coercesOption<Vec<String>>→Option<&[String]>at theops.rscall site;direct_list_toolsthen converts toOption<Vec<&str>>and calls.as_deref()again to yieldOption<&[&str]>— the chain is type-safe throughout.build_list_tool_schemas_v3_queryreturnsVec<(&'static str, String)>; passing this slice to reqwest's.query(¶ms)correctly serializes duplicate keys as repeated query params (tags=stars&tags=repos), not comma-joined — the HTTP mock test confirms this at the wire level.should_forward_tagsgate is unchanged; both direct and backend paths now apply the same tag-forwarding decision (GitHub only, for now).new_with_v3_baseis#[cfg(test)] pub(crate)— the production binary cannot inject a non-HTTPS base.- Empty-slug rows are dropped in
direct_list_tools(pre-existing filter), and the new HTTP test inclient_tests.rsverifies the junk row is dropped from the response. - Error paths in
direct_list_toolspropagate via?throughmap_errinto the Sentry-tagged[composio-direct]error string, symmetric with the backend branch as documented. - Debug logging added at both
direct_list_toolsentry andlist_tool_schemas_v3call site; tag count (not values) logged — no PII leaked.
| let csv = trimmed.join(","); | ||
| req = req.query(&[("toolkits", csv.as_str())]); | ||
| } | ||
| let url = format!("{}/tools", self.base_v3); |
There was a problem hiding this comment.
Unlike list_connected_accounts (line 522) and execute_action_v3 (line 344), which both call ensure_https(&url)? before transmitting x-api-key, this path skips the guard now that the URL comes from the injectable self.base_v3.
The production safety guarantee rests entirely on new_with_v3_base being #[cfg(test)]-gated — valid today, but fragile if that gate is ever widened. Consider anchoring the invariant at the constructor level instead:
// in new_internal:
fn new_internal(api_key: &str, default_entity_id: Option<&str>, security: Arc<SecurityPolicy>, base_v3: String) -> Self {
#[cfg(not(test))]
if !base_v3.starts_with("https://") {
panic!("base_v3 must use HTTPS in production; got: {base_v3}");
}
// ... rest unchanged
}Or a #[cfg(not(test))] ensure_https(&url)? at the call site. Either approach makes the constraint explicit rather than relying on the doc comment.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Clean fix with good test coverage. One non-blocking suggestion on consistency (inline); two nitpicks in the review comment. LGTM.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Clean fix with good test coverage. One non-blocking suggestion on ensure_https consistency (inline); two nitpicks in the review comment. LGTM.
2 participants
Summary
tagsfilter oncomposio_list_tools(added in feat(composio): add tags query param to GitHub tool list API #2714) was wired only through the backend-proxied path; in direct (BYO Composio key) mode it was computed and then silently dropped, so a self-key user filtering tools by tag got the unfiltered set.tagsthroughdirect_list_tools→ComposioTool::list_tool_schemas_v3, encoding it as repeatedtags=params per the Composio v3/toolscontract (vs the backend proxy's comma-joinedtags=a,b).ComposioTool(new_with_v3_base; prodnewdelegates with the real const) so the direct/toolspath is HTTP-testable against a local axum mock — closing the documented "can't mock direct mode" gap.Problem
#2714 added a
tagsquery param to the GitHub tool-list API. The wiring lived entirely on the backend client (ComposioClient::list_tools). Inops::composio_list_tools,effective_tagsis computed once at the top, but the direct branch calleddirect_list_tools(&direct, &scope)— no tags argument — andlist_tool_schemas_v3built the v3 request with onlytoolkits+limit. Result: in direct mode the tag filter was a no-op, with no error and no log. Today this only affects GitHub (theshould_forward_tagsallowlist), but it's a silent parity gap between the two modes.Solution
composio/ops.rs: the direct branch now passeseffective_tags.as_deref()(the value it was already computing). Theshould_forward_tagsgate is unchanged, so direct/backend behaviour stays consistent.composio/client.rs:direct_list_toolsgains atagsparam and forwards it.tools/impl/network/composio.rs:list_tool_schemas_v3acceptstags; request building is extracted into a pure, unit-testable helperbuild_list_tool_schemas_v3_query. Encoding is repeatedtags=params — Composio v3 documentstagsas "can be specified multiple times"; reusing the proxy's comma-join would make v3 treat"stars,repos"as a single literal tag and match nothing.ComposioToolgets an injectable v3 base URL. This mirrors how the backendComposioClientalready takes an injectable base viaIntegrationClient::newinclient_tests.rs, and removes the limitation that block-comment called out.Wire shape difference (same OR semantics):
…/agent-integrations/composio/tools?toolkits=github&tags=stars,reposbackend.composio.dev/api/v3/tools?limit=200&toolkits=github&tags=stars&tags=reposSubmission Checklist
tags=wire shape; v3→envelope reshape with empty-slug rows dropped)list_tool_schemas_v3,direct_list_tools) are covered by mock tests; theops.rsdirect-branch call line is exercised to its error path by the existingops_test.rsdirect-mode integration test. Will confirm the diff-cover number from CI and top up if the gate flags any line.## Related— N/A: no matrix rows affectedaxumis already a dev-dependency); no production deps addedCloses #NNN— N/A: no tracking issue; this brings feat(composio): add tags query param to GitHub tool list API #2714 to direct-mode parity (referenced below)Impact
x-api-key), just an additional query filter that narrows the response.ops.rsdirect-branch happy path (config → factory → connection prefetch →direct_list_tools) isn't driven end-to-end in unit tests because the factory buildsComposioToolagainst the real const base; the two new HTTP tests cover the reshaper and v3 request directly instead. Flagging in case CI's diff-cover wants the wiring line covered.Related
feat(composio): add tags query param to GitHub tool list API) to parity in direct/BYO-key modeAI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/composio-direct-tagsValidation Run
pnpm --filter openhuman-app format:check— N/A: no app/TS changespnpm typecheck— N/A: no app/TS changescargo test --lib composio→ 668 passed, 0 failed (incl. the 8 new tests)cargo fmt --checkclean; lib + tests build cleanValidation Blocked
command:git pushpre-push hook (pnpm rust:check)error:vendored tauri-cef CEF runtime + pnpm/node env not present in this sandboximpact:pushed with--no-verify; this PR touches only the Rust core lib (no Tauri shell), and CI runs the full checkBehavior Changes
composio_list_tools(..., tags)now honourstagsin direct/BYO-key mode (previously dropped)Parity Contract
should_forward_tagsallowlist unchanged; empty/blank tags still mean "no filter"Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests