fix(composio): gate per-action tools on their full contract (#4853)#4995
Conversation
…nsai#4853) Per-action Composio tools handed to integrations_agent are built from the thin list_tools schema (often {"type":"object"} with no field descriptions), so the model composes calls before the action's real contract is in context and guesses argument formats — most visibly sending unquoted Gmail queries that return zero results. Add a per-turn contract gate: on the first call to an action, surface its FULL live input schema/description (from the cached live toolkit catalog) as a recoverable tool error and let the retry execute. Degrades to a normal execute whenever the contract can't be resolved, so an unconfigured/offline client never blocks the action. Wired into the per-action surface; the dispatcher, MCP, and Workflow surfaces are follow-up.
|
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 (1)
📝 WalkthroughWalkthroughThe Composio action tool now consults a per-instance contract gate before dispatch. The first call can return the live action description and input schema as a recoverable error; retries proceed using the same live configuration snapshot. Tests cover surfacing, retries, unknown actions, and independent slugs. ChangesComposio contract gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Poem
Sequence Diagram(s)sequenceDiagram
participant Model
participant ComposioActionTool
participant ContractGate
participant LiveToolkitCatalog
participant ComposioDispatch
Model->>ComposioActionTool: execute(arguments)
ComposioActionTool->>ContractGate: consult(live_config, action_slug)
ContractGate->>LiveToolkitCatalog: resolve cached live contract
LiveToolkitCatalog-->>ContractGate: contract or no match
ContractGate-->>ComposioActionTool: Surface(contract) or Proceed
ComposioActionTool-->>Model: recoverable contract error
Model->>ComposioActionTool: retry with corrected arguments
ComposioActionTool->>ContractGate: consult(live_config, action_slug)
ContractGate-->>ComposioActionTool: Proceed
ComposioActionTool->>ComposioDispatch: dispatch action
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: d80b1e3fad
ℹ️ 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".
…4853-composio-gmail-search
… feature The tinyhumansai#4912 flows gate (merged after this branch was cut) put `openhuman::tinyflows` behind `#[cfg(feature = "flows")]`, so the gates-off build could not compile contract_gate.rs's unconditional `tinyflows::caps` import (E0433 in the "Rust Feature-Gate Smoke (gates off)" lane). The contract gate's live-catalog lookup is sourced entirely from the flows/tinyflows caps layer. With flows compiled out there is no catalog source, so the gate simply has no fuller contract to surface and always proceeds (the per-action Composio tool still runs; it just loses the pre-execute contract nudge). Guard the import and the lookup/format helpers on `feature = "flows"` in lockstep, restructure `consult` to Proceed when the feature is off, and gate the flows-seeding unit tests (contract_gate_tests.rs + the action_tool retry test) on the same feature. No behavior change in the default (flows-on) build.
|
Synced onto current Root cause: the #4912 flows gate merged after this branch was cut, putting Fix: the gate's live-contract lookup is sourced entirely from the flows/tinyflows caps layer, so with flows off there is simply no catalog to surface and the gate always proceeds (the per-action tool still runs; it just loses the pre-execute contract nudge). Guarded the |
|
@coderabbitai review Gated the contract gate's tinyflows dependency on |
coderabbitai
Bot
added
bug
feature
|
✅ Action performedReview finished.
|
![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 (1)
src/openhuman/composio/contract_gate.rs (1)
91-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd diagnostics around the live-catalog lookup.
The flow logs its final branch, but not lookup entry, duration, cache/fetch outcome, or failure reason. Preserve enough detail to distinguish unknown actions, unavailable configuration, and catalog-fetch failures.
As per coding guidelines, new flows require grep-friendly diagnostics covering entry/exit, external calls, state transitions, and errors.
🤖 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/contract_gate.rs` around lines 91 - 143, Add grep-friendly diagnostics around the flows-gated lookup_contract path: log lookup entry, toolkit derivation failures, catalog fetch start/completion with duration and cache/fetch outcome, and explicit reasons for unavailable configuration, fetch failure, or unknown actions. Preserve the existing GateDecision behavior and final proceed/surface logs, using lookup_contract and fetch_live_toolkit_catalog as the primary instrumentation points.Source: Coding guidelines
🤖 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/action_tool.rs`:
- Around line 205-206: Reload the current configuration before the contract
lookup in the action execution flow, rather than passing the captured
self.config to contract_gate::consult. Store that single reloaded snapshot and
reuse it for subsequent client creation and dispatch, ensuring contract lookup
and execution use identical live configuration.
In `@src/openhuman/composio/contract_gate.rs`:
- Around line 92-101: Update the gate flow around gate.mark_seen and the
contract lookup to distinguish resolving from surfaced states, preventing
concurrent sibling calls from returning GateDecision::Proceed while the first
lookup is still pending. Have siblings wait for completion or reuse the surfaced
contract, then preserve proceeding only after the contract has been surfaced.
Add a regression test that exercises concurrent calls and verifies neither
dispatches with thin arguments before resolution.
---
Nitpick comments:
In `@src/openhuman/composio/contract_gate.rs`:
- Around line 91-143: Add grep-friendly diagnostics around the flows-gated
lookup_contract path: log lookup entry, toolkit derivation failures, catalog
fetch start/completion with duration and cache/fetch outcome, and explicit
reasons for unavailable configuration, fetch failure, or unknown actions.
Preserve the existing GateDecision behavior and final proceed/surface logs,
using lookup_contract and fetch_live_toolkit_catalog as the primary
instrumentation points.
🪄 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: 72a85496-39cd-4e6e-b05c-0ee2fd55d024
📒 Files selected for processing (4)
src/openhuman/composio/action_tool.rssrc/openhuman/composio/contract_gate.rssrc/openhuman/composio/contract_gate_tests.rssrc/openhuman/composio/mod.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Solid, well-scoped fix for the immediate Gmail-search repro — the contract gate + per-action wiring is clean, and the follow-up commit fixing the flows-off build break shows good CI discipline.
That said, I can't approve this yet.
CodeRabbit's two major findings are both real and should block merge:
action_tool.rs: the gate consultsself.config(the captured snapshot) before the function reloadslive_configa few lines later (the "[#1710 Wave 4] reload config fresh per execute" block). That reload exists specifically so a mid-sessioncomposio.mode/credential/workspace change takes effect at the very next call — the contract lookup bypasses that invariant and can resolve off a stale config while the actual dispatch uses the fresh one.contract_gate.rs:mark_seenflips the slug to "seen" before the async catalog lookup resolves. Two concurrent calls to the same action within one turn — plausible in a tool-calling agent — will race: the second seesfirst_time == falseand dispatches immediately with the thin schema, i.e. exactly the zero-result bug this PR exists to fix, just for the unlucky sibling call.
New finding — this PR claims Closes #4853 but doesn't meet the issue's own acceptance criteria.
The issue lists four criteria: repro gone, regression safety (first-call/retry + unknown-id-with-valid-list + compaction reset), >=80% diff coverage, and generalization to the MCP/Workflow dispatchers. This PR nails the first criterion and half of the second — first-call/retry is tested, but an unknown action just degrades silently instead of surfacing "not found + valid list", and there's no compaction-reset mechanism or test. Generalization to MCP/Workflows is explicitly deferred to a follow-up PR.
On top of that, the gate's own doc comments describe the seen-set as "per-turn," but it's actually scoped to the lifetime of the ComposioActionTool instance — which the PR description itself acknowledges can span multiple turns (hence the deferred "reset on compaction" TODO). So in a longer-running integrations_agent spawn, once a contract is surfaced once, it's gone for good even after the schema has dropped out of context via compaction — exactly the scenario the issue's compaction-reset criterion exists to cover.
Given that, Closes #4853 will auto-close a P1 issue on merge that's only partially resolved. Either drop the Closes keyword and reference it as partial progress (e.g. Progress on #4853), or file a tracking issue for the deferred generalization/compaction work and link it here — don't let the P1 silently close.
None of this is a knock on the overall approach — the gate design (surface-once, degrade-gracefully) is sound, and scoping the first PR to the per-action surface is reasonable. But the config-staleness and race-condition fixes are small and contained, and worth doing before merge, and the issue-closure framing needs fixing regardless.
…Rabbit) The per-action execute path consulted the contract gate with the captured spawn-time `self.config`, then separately reloaded a fresh snapshot for client creation + dispatch. After a mid-session `composio.mode` / credential / workspace change the gate's live-catalog fetch could resolve (or skip) a contract against stale routing while dispatch used fresh routing. Reload the snapshot once, up front (mirroring the existing tinyhumansai#1710 Wave-4 principle), and reuse it for the gate consult, client creation, and dispatch so all three share identical live config.
…4853-composio-gmail-search
|
@coderabbitai review Addressed all three items: (1) Codex — flows-gate independence already fixed by |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Continuation Review — New commits address first major finding
Summary
Commit 3561130 (reload live config once before the contract gate) fixes the stale-config concern from the prior review: the gate, client factory, and dispatch now consistently share a single live_config snapshot reloaded up front, exactly as requested. CodeRabbit has confirmed the fix.
Findings Update
Prior major #1 (config reload before gate) — RESOLVED
- Commit 3561130 moves the
live_configreload before the gate consult and passes the same snapshot to both gate and dispatch. This aligns with the #1710 Wave 4 convention and eliminates the stale-routing window. - CodeRabbit confirmed: "The gate, client factory, and dispatch now consistently share the single reloaded
live_configsnapshot, resolving the stale-routing concern."
Prior major #2 (TOCTOU race on concurrent calls) — ACCEPTED TRADEOFF
- The code still marks the action seen before the async lookup completes. However, CodeRabbit has reviewed this and agreed: the rare concurrent path degrades to the established thin-schema execution path (existing behavior) rather than introducing a new failure mode. Given the per-turn gate scope and sequential tool execution in the common case, this pragmatic trade-off is acceptable.
Prior major #3 (PR closes P1 #4853 with partial scope) — NEEDS ACTION
- The PR description now explicitly documents the deferred scope (dispatcher/MPC/Workflow generalisation, compaction-reset), but the PR body still uses
Closes #4853, which will auto-close the issue on merge despite only partially implementing it. - Action required: Change the
## Relatedsection to use "Addresses #4853" (or remove the "Closes") so the P1 issue remains open for the follow-up PRs. Do not auto-close a partially-addressed P1.
Gates
- CI: pending (Rust Quality check still running, but all prior checks passed).
- Conflicts: none.
- Code quality: the fix is solid and well-integrated.
@M3gA-Mind, the code fix is ready to go — just need you to adjust the issue reference in the PR body from "Closes" to "Addresses" to avoid auto-closing the P1 with incomplete scope.
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Approving, with one framing note up front: this is a fail-open schema nudge, not a security/authz gate — it intentionally proceeds whenever the contract can't be resolved, and the real execution guard (the sandbox read-only check at action_tool.rs:168) runs before it and is unchanged. So "fail-closed / bypass" doesn't apply here.
Correctness verified: the deny path short-circuits before dispatch; per-turn scoping holds on the live path (eager dynamic_tools built once per spawn at runner.rs:766, and the same tool+gate services both the first-call surface and the retry-proceed); flows-gated so the slim build degrades to Proceed. The deny path is genuinely asserted in tests, not just allow. CI + coverage green.
Non-blocking (CodeRabbit's two "Major" — valid, but both fail-open, worst case equals pre-fix behavior):
- The gate consults captured
self.configwhile dispatch reloads a fresh snapshot ataction_tool.rs:215— stale after a mid-session mode/credential toggle. Cheapest fix: reload once before the gate and pass the snapshot in. mark_seenruns before the awaited lookup, so a concurrent sibling call to the same slug can proceed before the contract surfaces. Real race, harmless while fail-open.
Forward risk worth a note on #4249-1b: LazyToolkitResolver::resolve() builds a fresh gate per call (currently dead code — let _ = &lazy_resolver). When that path is wired for dispatch, a fresh gate per resolve means the contract is surfaced on every call and the action never proceeds. Cache the resolved tool per turn.
Codex P1 (unconditional tinyflows import) is already fixed at head — no action.
Addresses maintainer review (graycyrus / oxoxDev), docs-only: - contract_gate.rs: the ContractGate's seen-set doc overclaimed "per-turn". It is actually scoped to the ContractGate instance (one per ComposioActionTool spawn) — a single turn in the common case, but a long-lived integrations_agent spawn can span turns, and the gate does not reset on context compaction (deferred). Reworded to say so accurately. - subagent_runner/ops/provider.rs: document that LazyToolkitResolver::resolve builds a fresh ComposioActionTool (and gate) per call, so once wired to dispatch the resolved tool must be cached per turn or a surfaced action can never proceed (oxoxDev forward note). No behavior change.
8714bb4
|
@graycyrus @oxoxDev — addressing the review: Major #1 (gate consults stale Major #2 (TOCTOU race on concurrent same-action calls): per your continuation review + CodeRabbit, accepted tradeoff — the rare concurrent sibling degrades to the pre-existing thin-schema path (not a new failure mode), tool execution is sequential in the common case, and the gate is an intentionally non-blocking best-effort nudge. No change. Major #3 ( oxoxDev forward note (LazyToolkitResolver::resolve builds a fresh gate per call): ✅ added a code comment at Re-requesting review — the only code change since your continuation is docs/comments + the |
…llowlist The feature-gate-smoke lane compiles AND runs the lib test binary with the domain gates off, so the contract-gate tests that name gated symbols (`resolve_composio`/`render_composio_contract`/`ToolContract` → `flows`, `resolve_mcp`/`render_mcp_contract` → `mcp`, `join_capped` → either) must carry the matching `#[cfg(feature = …)]` or the gates-off test binary fails to compile. Gate the five affected tests to match the resolvers they exercise. The lane's self-maintaining allowlist then flags the newly-gated file, so add `openhuman/tinyagents/contract_gate_tests.rs`. It also surfaced a latent gap the main lane hides (the guard is skipped on main pushes, runs only on PRs): `openhuman/composio/action_tool.rs` already carries a `#[cfg(feature = "flows")]` test from tinyhumansai#4995 but was never added — add it too so the guard is consistent. Verified gates-off: `cargo check` + `cargo test --lib` (compile + the lane's scoped run, 230 pass) clean; the guard allowlist == the grep's actual set. Default `openhuman::tinyagents::contract_gate` tests still green (33). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…n the middleware) Upstream merged tinyhumansai#4995 ("gate per-action tools on their full contract"), a per-`ComposioActionTool` in-memory gate (`composio::contract_gate`). This PR's `tinyagents::contract_gate` middleware already gates per-action Composio calls — and more robustly: its "already surfaced" state is derived from the transcript with a payload-digest, so a contract summarised or compacted OUT of context is re-delivered. tinyhumansai#4995's own docs flag that its in-memory `seen`-set does NOT reset on compaction (the model then acts on a schema it can no longer see). To avoid double-gating (the middleware short-circuits the first call before execute, so tinyhumansai#4995's tool-layer consult would fire a SECOND contract on the retry and delay execution a turn), remove the tool-layer hookup: the `gate` field, its init, the `consult` call in `execute`, and its regression test. Changes to the merged code are kept minimal — `composio::contract_gate` itself is left in place (now unused in production, tracked for removal) and its unit tests still pass; only the per-action wiring in `action_tool.rs` (+ a stale lifecycle note in `provider.rs`) is touched. The middleware is now the single per-action gate. Verified: default `clippy --lib` clean, `openhuman::tinyagents::contract_gate` (33) + `composio::action_tool` (8) + dormant `composio::contract_gate` (3) green; gates-off `cargo check` + `cargo test --lib` compile clean; the feature-gate smoke allowlist matches (action_tool.rs drops its only gated test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Sync the 22-commit lag with main. Clean auto-merge (i18n additions and the coverage matrix merged without conflict). Brings in main's CI restructure — including the rust-feature-gate-smoke allowlist guard (tinyhumansai#5022) and the composio contract gate (tinyhumansai#4995) — which the next commit reconciles with.
…after tinyhumansai#4995 Merging main surfaced two pre-existing gaps left by tinyhumansai#4995 (gate per-action tools on their full contract) that any PR merging current main hits: - `composio_credentials_state_raw_coverage_e2e` asserted a per-action tool's first `execute` returns "Fetched 1 inbox message". tinyhumansai#4995 made the first call this turn surface the action's full contract as a recoverable error and let the retry execute — tinyhumansai#4995 changed the tool but not this test. Call execute twice: assert the first surfaces the contract, the second returns the fetch result. Verified passing locally. - `composio/action_tool.rs` gained a `#[cfg(feature = "flows")]` gated test but was not added to the rust-feature-gate-smoke allowlist (tinyhumansai#5022), so the guard failed on the merged tree. Added it; the guard's ACTUAL set now equals the allowlist. Neither file is part of the custom-servers feature; they ride here only because this PR now merges a main that is internally inconsistent on them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4 participants
Summary
GMAIL_FETCH_EMAILS(and other per-action tools handed tointegrations_agent) are built from the thinlist_toolsschema — often{"type":"object"}with no field descriptions — so the model guesses argument formats and sends unquoted Gmail queries that return zero results.src/openhuman/composio/contract_gate.rsmodule + unit tests; wired into the per-action surface (ComposioActionTool).Problem
The per-action tools for
integrations_agentare built byfetch_toolkit_actions(src/openhuman/composio/connected_integrations.rs:1016) from the lightweightlist_toolsresponse, whoseparametersis oftenNone→ComposioActionToolfalls back to{"type":"object"}(src/openhuman/composio/action_tool.rs:96). The model therefore never sees the action's real schema/description (e.g. that Gmailqueryneeds quoting for multi-word phrases) and composes zero-result calls. Telling the agent to read the contract first fixes it — because only then does the full schema enter context. The full contract already exists in-tree viafetch_live_toolkit_catalog(src/openhuman/tinyflows/caps.rs:1789), just not on this surface.Solution
contract_gate::ContractGaterecords which action contracts have been surfaced this turn (interior-mutable set, one gate perComposioActionToolinstance, which lives for oneintegrations_agentspawn — so "seen" is scoped to the turn without task-local plumbing).contract_gate::consult(src/openhuman/composio/contract_gate.rs): on the first consult for a slug, resolves the full contract from the process-cachedfetch_live_toolkit_catalogand returnsGateDecision::Surface(<formatted contract>); later consults, and any consult with no resolvable contract, returnGateDecision::Proceed. No network when the catalog is cached or the client is unconfigured (fails fast).ComposioActionTool::executeconsults the gate after the sandbox check and returns the contract asToolResult::errorbefore dispatch (src/openhuman/composio/action_tool.rs).[composio][contract-gate]prefix; logs only action slugs and booleans/counts — no user data / PII.Scope note — dispatcher / MCP / Workflows / compaction-reset are follow-up
This PR gates the per-action Composio surface, the direct root cause of the Gmail repro. The issue also asks to generalise the gate to the
composio_executedispatcher, the MCP bridges (mcp_call_tool/mcp_registry_tool_call) and the Workflow dispatchers (run_workflow), and to reset the per-turn state on context compaction. The clean home for that is a singleToolMiddlewareat the turn-harness seam (assemble_turn_harness,src/openhuman/tinyagents/mod.rs), where awrap_toolhook can short-circuit any late-bound tool uniformly and per-turn state lives inRunContext::stores. That is a larger change in the agent core; it is deliberately deferred here so this fix stays contained and unit-verifiable (local cargo build/test is disabled on this machine — CI is the authority), and it builds on the reusableContractGateprimitive this PR adds.Submission Checklist
contract_gate_tests.rs(surface-then-proceed, unknown-action degrades to proceed, independent gating) +action_tool.rsexecute-level test covering both gate arms.executewiring are exercised by the new unit tests; Rust-only change, CIrust-core-coverageis the authority.## Related— N/A: no matrix rows affected.## Relatedsection — usesAddresses #4853(NOTCloses) so the partially-resolved P1 is not auto-closed on merge, per maintainer review.Impact
integrations_agentspawns (Gmail/Notion/Slack/GitHub/Linear/etc.).Related
composio_executedispatcher, the MCP bridges, and the Workflow dispatchers via a sharedToolMiddleware, and reset the per-turn seen-set on context compaction.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4853-composio-gmail-searchd80b1e3fadf8c3ab951bceb07f125f64583d5b0cValidation Run
pnpm --filter openhuman-app format:check— N/A: no frontend files changed.pnpm typecheck— N/A: no TypeScript changed.cargo test -p openhuman contract_gate— not run locally (cargo disabled on this machine per disk policy); CI is authority.rustfmton the changed files (clean);cargo check/clippydeferred to CI.Validation Blocked
command:cargo test/cargo check/cargo clippyerror:local Rust builds are disabled on this machine (disk-fill policy); CI performs full verification.impact:compile + test + clippy verification deferred to CI (rust-core-coverage,Rust Quality).Behavior Changes
integrations_agentreturn results without the user first telling the agent to read the tool contract.Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests