feat(flows): search_tool_catalog surfaces real tool output field names#4591
Conversation
…s (stop guessing)
The builder guessed downstream binding field names from a tool_call's output (e.g.
GMAIL_FETCH_EMAILS → .messages) because search_tool_catalog returned only
{slug,toolkit,scope} — no output shape. So =nodes.<tool>.item.json.<field> often
resolved null at runtime.
Composio's v3 /tools endpoint publishes output_parameters (the response schema)
alongside input_parameters; this crate previously dropped it. Now:
- composio/{types,tools/direct,client}.rs thread output_parameters through (Direct
mode; backend-proxied path forwards it if ever sent, else None=unknown).
- caps.rs: RESPONSE_FIELDS_CACHE + composio_response_fields(slug) → top-level output
field names, cached like composio_required_args (own OnceLock; never errors — None
on unknown/failure).
- builder_tools.rs: SearchToolCatalogTool annotates each match with response_fields
(best-effort; empty + a 'dry-run to verify' note when the shape is unknown).
- prompt.md: bind downstream tool outputs to real response_fields, don't guess.
cargo check/fmt clean; flows::builder_tools 33, tinyflows::caps 30, composio 363 pass.
|
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 (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesOutput schema and response-fields grounding
Workflow builder run_flow rename
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/flows/builder_tools_tests.rs (1)
202-266: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage of the cache-hit paths — but the live-fetch loop (the part with the extraction-shape risk) is never exercised.
Both new tests seed
RESPONSE_FIELDS_CACHEdirectly, short-circuiting thecomposio_list_toolsfetch +response_fields_from_schemaloop incomposio_response_fields. Consider adding a test that exercises that loop with a realisticoutput_parametersshape (including adata/errorwrapper, per the concern raised incaps.rs) to catch extraction regressions before they reachsearch_tool_catalog.🤖 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/flows/builder_tools_tests.rs` around lines 202 - 266, The new tests only cover cache-hit behavior and miss the live-fetch path inside composio_response_fields, so the extraction loop with response_fields_from_schema is still untested. Add a test that drives SearchToolCatalogTool through a realistic composio_list_tools-style response using output_parameters shaped with a data/error wrapper, and verify the extracted response_fields for a known action. Use the existing composio_response_fields and response_fields_from_schema flow in builder_tools_tests.rs so regressions in live schema extraction are caught before search_tool_catalog depends on them.src/openhuman/flows/builder_tools.rs (1)
633-660: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential
.awaitper result for response-fields lookup.Each matched result triggers a separate awaited
composio_response_fieldscall; distinct toolkits appearing in one search incur serialized network round trips (cache only helps repeated toolkits within/after the first fetch). Consider collecting unique toolkits/slugs up front and resolving them concurrently (e.g.,futures::future::join_all) before annotating results.🤖 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/flows/builder_tools.rs` around lines 633 - 660, The result annotation loop in builder_tools is doing one awaited composio_response_fields lookup per matched item, which serializes network calls when multiple distinct slugs/toolkits are returned. Refactor the logic around search_curated_catalog and the per-result enrichment to collect unique slugs up front, resolve their response fields concurrently, and then apply the cached results back onto each Value::Object entry so the loop no longer waits sequentially on each .await.
🤖 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/flows/builder_tools.rs`:
- Around line 633-660: `SearchToolCatalogTool::execute` is using a stale
captured `Arc<Config>` (`self.config`) when calling `composio_response_fields`,
so Composio client selection can miss runtime config changes. Update the execute
path to reload the live config at call time using
`config_rpc::load_config_with_timeout()` and pass that fresh config into the
`composio_response_fields`/`composio_list_tools` flow instead of relying on the
constructor-captured config from `ops.rs`. Keep the change localized to
`SearchToolCatalogTool::execute` and the Composio config lookup path so API key
or mode changes are reflected on each run.
In `@src/openhuman/tinyflows/caps.rs`:
- Around line 1371-1381: The response field lookup in `composio_response_fields`
is collapsing unknown schemas into empty lists because
`response_fields_from_schema` and the `by_slug` cache store `Vec::new()` even
when `tool.function.output_parameters` is `None`. Update the lookup path around
`composio_response_fields`, `response_fields_from_schema`, and the `by_slug`
population so `None` stays `None` and only a real empty schema becomes
`Some(vec![])`; preserve the distinction when caching and when returning the
found value.
---
Nitpick comments:
In `@src/openhuman/flows/builder_tools_tests.rs`:
- Around line 202-266: The new tests only cover cache-hit behavior and miss the
live-fetch path inside composio_response_fields, so the extraction loop with
response_fields_from_schema is still untested. Add a test that drives
SearchToolCatalogTool through a realistic composio_list_tools-style response
using output_parameters shaped with a data/error wrapper, and verify the
extracted response_fields for a known action. Use the existing
composio_response_fields and response_fields_from_schema flow in
builder_tools_tests.rs so regressions in live schema extraction are caught
before search_tool_catalog depends on them.
In `@src/openhuman/flows/builder_tools.rs`:
- Around line 633-660: The result annotation loop in builder_tools is doing one
awaited composio_response_fields lookup per matched item, which serializes
network calls when multiple distinct slugs/toolkits are returned. Refactor the
logic around search_curated_catalog and the per-result enrichment to collect
unique slugs up front, resolve their response fields concurrently, and then
apply the cached results back onto each Value::Object entry so the loop no
longer waits sequentially on each .await.
🪄 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: c1148f3c-287c-43e9-820c-facc5c4f33e2
📒 Files selected for processing (10)
src/openhuman/composio/client.rssrc/openhuman/composio/tools/direct.rssrc/openhuman/composio/tools/direct_tests.rssrc/openhuman/composio/tools_tests.rssrc/openhuman/composio/types.rssrc/openhuman/flows/agents/workflow_builder/prompt.mdsrc/openhuman/flows/builder_tools.rssrc/openhuman/flows/builder_tools_tests.rssrc/openhuman/tinyflows/caps.rssrc/openhuman/tools/ops.rs
| let mut results = search_curated_catalog(&query, toolkit, MAX_CATALOG_RESULTS); | ||
| for result in &mut results { | ||
| let Some(slug) = result | ||
| .get("slug") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string) | ||
| else { | ||
| continue; | ||
| }; | ||
| let response_fields = | ||
| crate::openhuman::tinyflows::caps::composio_response_fields(&self.config, &slug) | ||
| .await; | ||
| let Value::Object(map) = result else { | ||
| continue; | ||
| }; | ||
| match response_fields { | ||
| Some(fields) => { | ||
| map.insert("response_fields".to_string(), json!(fields)); | ||
| } | ||
| None => { | ||
| map.insert("response_fields".to_string(), json!(Vec::<String>::new())); | ||
| map.insert( | ||
| "response_fields_note".to_string(), | ||
| json!("output shape unknown — dry-run to verify the binding resolves"), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale captured Arc<Config> used for Composio client construction in execute().
self.config is captured once when SearchToolCatalogTool is constructed (at registry build time in ops.rs), then reused for every execute() call to fetch composio_response_fields, which in turn calls composio_list_tools (a Composio client-creation path). If the on-disk config changes after construction (API key rotation, mode switch), this tool keeps using the stale snapshot instead of the current settings.
Based on learnings, in src/openhuman/composio code, any execute()/client-creation path should reload the live config via config_rpc::load_config_with_timeout() at the start of the call rather than relying on a captured Arc<Config> for runtime/mode/key selection.
🔧 Suggested fix direction
async fn execute(&self, args: Value) -> anyhow::Result<ToolResult> {
+ let live_config = crate::openhuman::config_rpc::load_config_with_timeout(...)
+ .await
+ .unwrap_or_else(|_| self.config.clone());
...
- let response_fields =
- crate::openhuman::tinyflows::caps::composio_response_fields(&self.config, &slug)
- .await;
+ let response_fields =
+ crate::openhuman::tinyflows::caps::composio_response_fields(&live_config, &slug)
+ .await;🤖 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/flows/builder_tools.rs` around lines 633 - 660,
`SearchToolCatalogTool::execute` is using a stale captured `Arc<Config>`
(`self.config`) when calling `composio_response_fields`, so Composio client
selection can miss runtime config changes. Update the execute path to reload the
live config at call time using `config_rpc::load_config_with_timeout()` and pass
that fresh config into the `composio_response_fields`/`composio_list_tools` flow
instead of relying on the constructor-captured config from `ops.rs`. Keep the
change localized to `SearchToolCatalogTool::execute` and the Composio config
lookup path so API key or mode changes are reflected on each run.
Source: Learnings
There was a problem hiding this comment.
Disagreeing with the suggested fix direction here — verified against the codebase before dismissing.
SearchToolCatalogTool captures Arc<Config> at construction exactly like every other tool in this same registry (tools::ops::all_tools_with_runtime) — CronAddTool, ProposeWorkflowTool, ReviseWorkflowTool, ListFlowsTool, DryRunWorkflowTool, RunFlowTool, etc. all take config.clone() at construction and never reload live config in execute(). This is the established pattern for the whole tool belt, not something introduced by this PR.
More importantly, the registry itself is rebuilt fresh per session (agent/harness/session/builder/factory.rs:402 calls tools::all_tools_with_runtime(Arc::new(tool_config.clone()), ...) when building a session), which bounds the staleness window to a single session/turn — the same bound every other tool in the belt already lives with. Reloading live config only inside this one tool's execute() would be an inconsistent, one-off pattern deviation rather than a fix, and would need to apply to the whole registry to actually matter (a separate, much larger change out of scope for this PR).
Not applying the suggested diff. Happy to revisit as a repo-wide config-freshness pass if that's wanted, but scoping it to just search_tool_catalog doesn't hold up.
There was a problem hiding this comment.
@graycyrus Fair enough — that's a solid verification. You're right that this is the established pattern across the entire all_tools_with_runtime registry, and singling out search_tool_catalog for a config-freshness fix would be an inconsistent one-off rather than an actual improvement, given the staleness window is already bounded by the per-session registry rebuild. Withdrawing the comment.
If a repo-wide config-freshness pass is ever wanted for the tool belt, happy to help scope that out separately.
✏️ Learnings added
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 4591
File: src/openhuman/flows/builder_tools.rs:633-683
Timestamp: 2026-07-06T19:08:32.399Z
Learning: In `tinyhumansai/openhuman`, all tools registered via `tools::ops::all_tools_with_runtime` (e.g. `SearchToolCatalogTool`, `CronAddTool`, `ProposeWorkflowTool`, `ReviseWorkflowTool`, `ListFlowsTool`, `DryRunWorkflowTool`, `RunFlowTool`, etc.) capture `Arc<Config>` at construction time and never reload live config inside `execute()`. This is the established, intentional pattern for the whole tool belt, not a bug. The staleness window is bounded because the registry itself is rebuilt fresh per session — `agent/harness/session/builder/factory.rs` (around line 402) calls `tools::all_tools_with_runtime(Arc::new(tool_config.clone()), ...)` when building a session. Do not flag a single tool in this registry for reloading config via `config_rpc::load_config_with_timeout()` in `execute()`; that would be an inconsistent one-off deviation. A config-freshness fix, if desired, would need to apply repo-wide to the whole registry.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When using the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, don’t require individual execution-provider feature flags (e.g., `directml`, `coreml`, `cuda`) alongside `load-dynamic` to get EP registration code. The `ort` crate already compiles EP registration via `#[cfg(any(feature = "load-dynamic", feature = "<ep_name>"))]` guards, and adding per-EP feature flags can pull in static-linking dependencies that conflict with the dynamic loading approach. At runtime, EP availability is determined by what the dynamically loaded ONNX Runtime library (`onnxruntime.dll`/`.so`/`.dylib`) supports; ort docs indicate providers like `directml`/`xnnpack`/`coreml` are available in builds when the platform supports them.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When integrating the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, do NOT also require/enable individual provider EP Cargo features like `directml`, `coreml`, or `cuda`. In `ort` v2.x, EP registration for providers (e.g., DirectML, CoreML, CUDA, etc.) is already compiled in under source-level `#[cfg(any(feature = "load-dynamic", feature = "<provider>"))]` guards, such as in `ep/directml.rs`. Adding provider feature flags alongside `load-dynamic` can pull in static-linking dependencies that conflict with the dynamic-loading approach. Provider availability should be treated as runtime-determined by what the loaded `onnxruntime` library (`onnxruntime.dll`/`libonnxruntime.so`/`libonnxruntime.dylib`) actually supports.
Learnt from: oxoxDev
Repo: tinyhumansai/openhuman PR: 571
File: src/openhuman/local_ai/service/whisper_engine.rs:69-80
Timestamp: 2026-04-14T19:59:04.826Z
Learning: When reviewing Rust code in this repo that uses the upstream `whisper-rs` crate (v0.16.0), do not report `WhisperContextParameters::use_gpu(...)` or `WhisperContextParameters::flash_attn(...)` as missing/invalid APIs. These builder-style methods exist upstream and return `&mut Self`; they are not limited to `WhisperVadContextParams`.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 1078
File: src/openhuman/agent/agents/welcome/prompt.rs:24-24
Timestamp: 2026-05-01T13:41:00.958Z
Learning: For Rust code under `src/openhuman/**/*.rs`, use `snake_case` for local variables (not `camelCase`). If a local variable name is written in `camelCase`, treat it as a style/lint issue because it will trigger Rust’s `non_snake_case` warning (and related clippy linting, if enabled). Avoid suggesting `camelCase` for any Rust local variable names in this repository.
Learnt from: senamakel
Repo: tinyhumansai/openhuman PR: 1173
File: tests/agent_memory_loader_public.rs:88-88
Timestamp: 2026-05-04T06:50:47.877Z
Learning: In this repository, the general camelCase naming guideline should not be applied to Rust source files. For all .rs files, Rust function (and related) names should use snake_case, and snake_case Rust function names should not be flagged—even for async test functions annotated with attributes like #[tokio::test]. This is consistent with Rust’s non_snake_case lint behavior.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 3775
File: src/openhuman/inference/openai_oauth/flow_tests.rs:605-638
Timestamp: 2026-06-19T04:39:51.616Z
Learning: When reviewing path usage in the `tinyhumansai/openhuman` Rust code (especially auth/session storage), don’t flag a “path-mismatch” concern for `AuthService::from_config` just because it doesn’t use `workspace_dir`. `AuthService::from_config` resolves its state directory via `state_dir_from_config(config)`, which returns `config_path.parent()` (see `src/openhuman/credentials/core.rs:119-124`). In tests that use `test_config(&tmp)` (where `config_path = tmp.path().join("config.toml")`), creating `AuthProfilesStore::new(tmp.path(), false)` writes into the same directory that `AuthService::from_config` will read (the config parent directory).
RunFlowTool (agent-facing "test a SAVED flow" tool in the workflow-builder belt) and the unrelated legacy skills-workflow runner (agent::tools::run_workflow, RUN_WORKFLOW_TOOL_NAME) were both registered as "run_workflow" in the same default tool registry (tools::ops::all_tools_with_runtime). This tripped all_tools_default_registry_has_no_duplicate_tool_names in CI (PR tinyhumansai#4591's Rust Core Coverage lane) — pre-existing on the PR's base commit, not introduced by this PR's diff, but blocking its CI. Renames the flows-domain tool to run_flow (matches its own struct name and the get_flow/get_flow_run/list_flow_connections singular convention) and updates every reference within its own domain: workflow_builder's prompt.md, builder_prompt.rs turn-brief templates, agent.toml's tool allowlist + comments, flows/schemas.rs's field doc, and the agent_registry loader test asserting the agent's tool scope. The skills-domain run_workflow (frontend WorkflowRunnerBody, rhai_workflows bridge, turn/mod.rs dispatch) is untouched — different tool, different subsystem.
…elds cache (addresses @coderabbitai on src/openhuman/tinyflows/caps.rs:1381) The live-fetch loop in composio_response_fields always inserted a cache entry for every listed tool, even when output_parameters was None — response_fields_from_schema(None) also returns an empty Vec, so "schema genuinely unknown" and "schema present but empty" collapsed to the same cached Some(vec![]). That breaks the function's own documented contract (None == unknown, propagated so callers like search_tool_catalog can surface the "dry-run to verify" note) for any future caller that doesn't share search_tool_catalog's happens-to-mask-it prompt guidance. Only cache an entry when output_parameters is actually present, so an unknown schema correctly falls through to None on lookup. Also adds direct unit tests for the pure response_fields_from_schema extraction step (standard `properties` object, legacy top-level-keys fallback, a nested data/error envelope, and None/non-object input) — addresses a CodeRabbit nitpick that the existing builder_tools_tests.rs coverage only exercises the cache-hit path, never the live-fetch extraction loop. Testing the pure function directly is cheaper and more targeted than mocking the full composio_list_tools round trip.
(addresses @coderabbitai nitpick on src/openhuman/flows/builder_tools.rs:633-660) Each matched catalog result was awaiting its own composio_response_fields lookup in sequence, so a broad query spanning several distinct toolkits paid for their catalog round trips back-to-back (the per-toolkit cache only helps repeat lookups within/after the first fetch, not the first one). Collects the unique slugs up front and resolves them with futures::future::join_all before annotating results, so distinct-toolkit lookups happen in parallel instead of serially.
|
pr-manager status update — pushed fixes for CodeRabbit's review (27a7ba2):
Also found and fixed an unrelated, pre-existing CI blocker while running the quality suite: |
coderabbitai
Bot
added
the
rust-core
Pre-existing test drift surfaced by this PR's `agent/` test run: tinyhumansai#4591 added `get_tool_contract` to the workflow_builder propose-or-read tool belt but did not update `workflow_builder_is_registered_worker_with_narrow_propose_or_read_scope` (expected 14, actual 15). `get_tool_contract` is a read-only catalog tool that belongs in the belt; add it to the expected list. Unrelated to the Phase 5 refactor — fixed here to unblock this PR's coverage job. Claude-Session: https://claude.ai/code/session_015U8RSD1CWUeeetNUWWbTQP
2 participants
Problem
The builder guessed downstream binding field names from a
tool_call's output (e.g.GMAIL_FETCH_EMAILS→.messages) becausesearch_tool_catalogreturned only{slug, toolkit, scope}— no output shape. So=nodes.<tool>.item.json.<field>often resolved null at runtime.Fix
Composio's v3
/toolsendpoint publishesoutput_parameters(the response schema) alongsideinput_parameters; this crate was dropping it. Now:composio/{types,tools/direct,client}.rsthreadoutput_parametersthrough (Direct mode; backend-proxied path forwards it if sent, elseNone=unknown).caps.rs:RESPONSE_FIELDS_CACHE+composio_response_fields(slug)→ top-level output field names, cached likecomposio_required_args(never errors —Noneon unknown).search_tool_catalogannotates each match withresponse_fields(best-effort; empty + a "dry-run to verify" note when unknown).response_fields, don't guess.Verification
cargo check/fmtclean · flows::builder_tools 33 · tinyflows::caps 30 · composio 363 pass.Pairs with #(agent input_context) — that fixes agent inputs, this grounds tool outputs.
Summary by CodeRabbit
response_fieldsfor matched actions (when available), plus a note to use verification when they can’t be determined.run_flow(instead ofrun_workflow), aligning prompts, permissions, and error messaging.