fix(composio): gate write actions through approval + reshape agent-path results - #5259
fix(composio): gate write actions through approval + reshape agent-path results#5259yh928 wants to merge 2 commits into
Conversation
…ults Two gaps on the agent's Composio execution surface, both diagnosed live. **Approval (P1).** The human-in-the-loop approval card is raised only for tools whose `external_effect_with_args` is true, but neither `composio_execute` nor the per-action `ComposioActionTool` declared it — so a Composio mail send (`GMAIL_SEND_EMAIL`) fired with no approval prompt at all, even when the user had "ask before sending" configured. The contract gate (schema-presence) and `permission_level = Write` (channel caps) do not raise that card. Both surfaces now report external-effect for a write/admin-scoped action and stay false for a pure read, so a write routes through the `ApprovalGate` while a fetch/list flows through unprompted. Scope is classified synchronously (`resolve_action_scope`'s body has no `await`, so it is reused via `resolve_action_scope_sync`). **Reshape (P4, tinyhumansai#2585).** When the agent calls a Composio action directly, a verbose provider envelope — Gmail's full MIME tree under `payload.parts[]` — landed in context on the raw-JSON fallback body. The provider response reshape that slims it (the same one the sync path runs) was only wired into sync; it now runs inline on the agent execute + per-action paths, so `resp.data` is slimmed before it can become the tool body. A backend-rendered `markdown_formatted` body is already clean and unaffected. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
📝 WalkthroughWalkthroughComposio tools now classify external effects for gating and apply toolkit-provider response post-processing before serializing action results. Tests cover write, admin, read, and missing-tool cases. ChangesComposio action flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ToolCaller
participant ComposioExecuteTool
participant ActionScopeResolver
participant Dispatcher
participant ToolkitProvider
ToolCaller->>ComposioExecuteTool: provide tool slug and arguments
ComposioExecuteTool->>ActionScopeResolver: resolve action scope
ActionScopeResolver-->>ComposioExecuteTool: effect classification
ComposioExecuteTool->>Dispatcher: dispatch action
Dispatcher-->>ComposioExecuteTool: response data
ComposioExecuteTool->>ToolkitProvider: post-process response data
ToolkitProvider-->>ComposioExecuteTool: reshaped data
ComposioExecuteTool-->>ToolCaller: serialized tool result
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
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 (1)
src/openhuman/composio/tools.rs (1)
1477-1549: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd regression tests for response reshaping on both execution paths.
The new behavior is untested: verify shaped
resp.datain the raw-JSON fallback, preservation of backendmarkdown_formatted, and forwarding ofraw_htmlarguments.
src/openhuman/composio/tools.rs#L1477-L1549: add deterministic dispatcher-path coverage insrc/openhuman/composio/tools_tests.rs.src/openhuman/composio/action_tool.rs#L304-L341: add equivalent per-action coverage in this module’s tests.As per coding guidelines, “Untested code is incomplete; add tests for new or changed behavior.”
🤖 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/tools.rs` around lines 1477 - 1549, Add regression tests for response reshaping in src/openhuman/composio/tools.rs lines 1477-1549 by adding deterministic dispatcher-path coverage in src/openhuman/composio/tools_tests.rs, verifying shaped resp.data for the raw-JSON fallback, preservation of backend markdown_formatted, and forwarding of raw_html arguments. Add equivalent per-action coverage for src/openhuman/composio/action_tool.rs lines 304-341 in that module’s tests, covering the same behaviors.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.
Outside diff comments:
In `@src/openhuman/composio/tools.rs`:
- Around line 1477-1549: Add regression tests for response reshaping in
src/openhuman/composio/tools.rs lines 1477-1549 by adding deterministic
dispatcher-path coverage in src/openhuman/composio/tools_tests.rs, verifying
shaped resp.data for the raw-JSON fallback, preservation of backend
markdown_formatted, and forwarding of raw_html arguments. Add equivalent
per-action coverage for src/openhuman/composio/action_tool.rs lines 304-341 in
that module’s tests, covering the same behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82754407-63cc-4cb4-8159-8cdceb516340
📒 Files selected for processing (3)
src/openhuman/composio/action_tool.rssrc/openhuman/composio/tools.rssrc/openhuman/composio/tools_tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f13e2bdad
ℹ️ 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".
|
| Filename | Overview |
|---|---|
| src/openhuman/composio/tools.rs | Adds resolve_action_scope_sync, action_mutates_external_state, and external_effect_with_args on ComposioExecuteTool; inserts inline post_process_action_result reshape on the agent path. Logic is correct and conservative (empty/non-string slug defaults to gated). |
| src/openhuman/composio/action_tool.rs | Adds external_effect_with_args on ComposioActionTool (keyed off the fixed action_name), inserts inline reshape before event publish matching tools.rs, and adds reshape_args clone before the dispatch consumes args. Event ordering is now consistent between both surfaces. |
| src/openhuman/composio/tools_tests.rs | Adds execute_tool_gates_writes_but_not_reads_via_external_effect covering send, delete, fetch, and absent-slug cases for ComposioExecuteTool. Clean and well-scoped. |
Sequence Diagram
sequenceDiagram
participant Agent as Agent Loop
participant Gate as ApprovalGate
participant EWT as external_effect_with_args
participant Scope as resolve_action_scope_sync
participant Exec as execute()
participant Dispatch as execute_dispatch
participant Reshape as post_process_action_result
Agent->>Gate: invoke composio_execute / ComposioActionTool
Gate->>EWT: external_effect_with_args(args)
EWT->>Scope: action_mutates_external_state(slug)
Scope-->>EWT: Write/Admin → true, Read → false
EWT-->>Gate: bool
alt write/admin action
Gate->>Agent: show approval card (blocks)
Agent->>Gate: user approves
end
Gate->>Exec: execute(args)
Exec->>Dispatch: execute_composio_action_kind[_with_connection]
Dispatch-->>Exec: ComposioResponse
Exec->>Reshape: post_process_action_result(slug, reshape_args, resp.data)
Reshape-->>Exec: slimmed resp.data
Exec->>Agent: ToolResult (markdown_formatted or slimmed JSON)
Reviews (2): Last reviewed commit: "fix(composio): publish the action event ..." | Re-trigger Greptile
…patcher does `ComposioExecuteTool` reshapes then publishes; the per-action tool published then reshaped. The payload names no reshaped field today, so the order is not observable — but two surfaces describing the same action must not disagree about which snapshot the event saw, or the first field added to it diverges silently between them. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
|
Pushed Event-bus publish / reshape ordering (greptile) — fixed. Agreed there is no functional difference today, and that is exactly why it was worth closing: the two surfaces describing the same action must not disagree about which snapshot the event saw, or the first field added to the payload diverges silently between them. Approval prompt for a contract-only probe (codex P2) — real, but the safe direction, and the fix belongs upstream of this PR. The behaviour you describe is right: on a first-time write slug the card is raised, The alternative is worse. To skip the prompt this PR would have to predict that On the denial case: a denied The ordering fix you are pointing at is real and is being done where it belongs — moving the gate to tool admission ( |
Summary
Two gaps on the agent's Composio execution surface, both diagnosed against a live instance.
Approval gate (the reported bug)
The human-in-the-loop approval card is raised only for a tool whose
external_effect_with_argsistrue, but neithercomposio_executenor the per-actionComposioActionTooldeclared it — so a Composio mail send (GMAIL_SEND_EMAIL) via theintegrations_agentfired with no approval prompt at all, even when the user had configured "ask before sending". Neither the contract gate (which only ensures the action schema is in context) norpermission_level = Write(channel caps: allow/block, not a prompt) raises that card.Both surfaces now report external-effect for a write/admin-scoped action and stay
falsefor a pure read, so a write routes through theApprovalGate(parking for the card under a WebChat turn, which the inline sub-agent inherits) while a fetch/list flows through unprompted — matching theexternal_effectcontract. Scope is classified synchronously:resolve_action_scope's body has noawait, so it is reused via aresolve_action_scope_synccore.Agent-path envelope reshape (#2585)
When the agent calls a Composio action directly, a verbose provider envelope — Gmail's full MIME tree under
payload.parts[], dozens ofReceived:headers — landed in context on the raw-JSON fallback body. The response reshape that slims it into one clean record per message was wired only into the sync path; it now runs inline on the agentcomposio_executeand per-action paths, soresp.datais slimmed before it can become the tool body. A backend-renderedmarkdown_formattedbody is already clean and unaffected.Tests
cargo test --lib composio— new coverage:execute_tool_gates_writes_but_not_reads_via_external_effectandper_action_tool_gates_writes_but_not_readsassert the gate/no-gate split for send/delete vs fetch. (Thecomposio::opsdelete-connection tests are flaky only under the parallel chunk-DB cold-open race, fixed separately in tinycortex; they pass--test-threads=1.)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Closes #5299