fix(composio): require approval for agent writes - #5863
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughComposio tools now classify external effects from action scopes. Write and Admin actions require approval, while read actions do not. Missing or invalid dispatcher slugs remain effectful. Approval grants use normalized per-action identities. Regression tests cover both tool surfaces and middleware routing. ChangesComposio approval gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change makes Composio write and admin actions require approval while preserving unprompted read actions; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Agent
participant ApprovalSecurityMiddleware
participant ComposioExecuteTool
participant ActionScopeResolver
Agent->>ApprovalSecurityMiddleware: Invoke Composio action
ApprovalSecurityMiddleware->>ComposioExecuteTool: Check external_effect_with_args
ComposioExecuteTool->>ActionScopeResolver: Resolve action scope
ActionScopeResolver-->>ComposioExecuteTool: Return Read, Write, or Admin
ComposioExecuteTool-->>ApprovalSecurityMiddleware: Return effect status
ApprovalSecurityMiddleware->>ApprovalSecurityMiddleware: Use normalized per-action approval identity
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation All implementation and test changes support issue
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0168 · 132,350 in / 2,344 out · 16,415 cached (12%) · openrouter/openai/text-embedding-3-small, z-ai/glm-5.2, deepseek/deepseek-v4-flash · 245 embedded
critique: $0.0078 · 57,329 in / 1,036 out · 7,825 cached (14%) · z-ai/glm-5.2, deepseek/deepseek-v4-flash
security: $0.0073 · 57,224 in / 865 out · 8,590 cached (15%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
tests: $0.0012 · 12,552 in / 365 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0005 · 5,245 in / 78 out · 0 cached (0%) · deepseek/deepseek-v4-flash
How this change flows2 changed behaviours across 10 relationships. 5 surrounding behaviours are shown (60 graph nodes walked). 37 further behaviours left out to keep the diagram readable. flowchart LR
n0["...nal_effect_resolution_walks_the_tool_sets<br/>changed"]:::changed
n1["execute_tool_requires_tool_argument<br/>changed<br/>1 finding"]:::flagged
n2["openhuman"]:::impacted
n3["error_text"]:::impacted
n4["...aces_full_contract_then_proceeds_on_retry"]:::impacted
n5["fake_config_arc"]:::impacted
n6["vec"]:::impacted
n0 -->|calls| n6
n0 -->|tests| n6
n1 -->|calls| n5
n1 -->|tests| n5
n3 -->|uses| n2
n4 -->|calls| n3
n4 -->|tests| n3
n4 -->|calls| n6
n4 -->|tests| n6
n5 -->|uses| n2
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: aea203cfe1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@tinyhumansai/maintainers @senamakel — review requested for this P1 approval-gate fix. CodeRabbit and Tinysweeper approve the latest commit, all review threads are resolved, and the focused regressions plus both root Clippy lanes pass. Required CI currently stops on two failures already present in base |
Adds wire-level e2e coverage for the eight RPC controllers that `scripts/check-domain-e2e-coverage.mjs` reported as uncovered, taking agent 7/12 -> 12/12, approval 3/5 -> 5/5 and memory 34/35 -> 35/35. Every method is driven over the real JSON-RPC router rather than by calling handlers directly. That matters because the checker's notion of "covered" is the string literal "openhuman.<method>" appearing anywhere in a tests/**/*_e2e.rs file — it never verifies the method is invoked. Two of the eight (approval_get_gate_state, approval_preauthorize_flow) were false negatives: they already had handler-level coverage in tool_registry_approval_raw_coverage_e2e.rs and scored 0 only because those tests bypass the wire method name. Includes an #[ignore]d reproduction of openhuman#5862: ComposioExecuteTool and ComposioActionTool declare PermissionLevel::Write but never override external_effect*, so they inherit the trait default of false and ApprovalSecurityMiddleware — whose only gate predicate is external_effect_with_args — never parks them. Agent-initiated Composio writes therefore run with no approval card. The legacy ComposioTool classifies correctly (tools/direct_part_03.rs:66-80) but is not an agent tool, so the classification was simply not carried over when the surface was split. The test asserts the correct behaviour and so fails today; it is ignored to keep the lane green and becomes the regression guard once tinyhumansai#5863 or an equivalent fix lands. Each test was verified by mutation: the covered behaviour was broken and the test confirmed to fail naming its own assertion. Three drafts did not survive that check and were rewritten rather than kept: - an oversized-limit assertion (events.len() <= 1000) was unfalsifiable, since the queried run has no events; - a missing-run_id assertion matched .contains("run_id"), which also matches the downstream error's "run_id=" debug label — it could not tell rejection from acceptance-then-failure. It now matches the exact validate_params string; - a preauthorize idempotency assertion compared grant counts that are both empty when no gate is installed. It now pins the documented gate-absent contract instead, and says why the grant path belongs in the unit tests that own the gate lifecycle.
Adds wire-level e2e coverage for the eight RPC controllers that `scripts/check-domain-e2e-coverage.mjs` reported as uncovered, taking agent 7/12 -> 12/12, approval 3/5 -> 5/5 and memory 34/35 -> 35/35. Every method is driven over the real JSON-RPC router rather than by calling handlers directly. That matters because the checker's notion of "covered" is the string literal "openhuman.<method>" appearing anywhere in a tests/**/*_e2e.rs file — it never verifies the method is invoked. Two of the eight (approval_get_gate_state, approval_preauthorize_flow) were false negatives: they already had handler-level coverage in tool_registry_approval_raw_coverage_e2e.rs and scored 0 only because those tests bypass the wire method name. Includes an #[ignore]d reproduction of openhuman#5862: ComposioExecuteTool and ComposioActionTool declare PermissionLevel::Write but never override external_effect*, so they inherit the trait default of false and ApprovalSecurityMiddleware — whose only gate predicate is external_effect_with_args — never parks them. Agent-initiated Composio writes therefore run with no approval card. The legacy ComposioTool classifies correctly (tools/direct_part_03.rs:66-80) but is not an agent tool, so the classification was simply not carried over when the surface was split. The test asserts the correct behaviour and so fails today; it is ignored to keep the lane green and becomes the regression guard once tinyhumansai#5863 or an equivalent fix lands. Each test was verified by mutation: the covered behaviour was broken and the test confirmed to fail naming its own assertion. Three drafts did not survive that check and were rewritten rather than kept: - an oversized-limit assertion (events.len() <= 1000) was unfalsifiable, since the queried run has no events; - a missing-run_id assertion matched .contains("run_id"), which also matches the downstream error's "run_id=" debug label — it could not tell rejection from acceptance-then-failure. It now matches the exact validate_params string; - a preauthorize idempotency assertion compared grant counts that are both empty when no gate is installed. It now pins the documented gate-absent contract instead, and says why the grant path belongs in the unit tests that own the gate lifecycle.
|
@senamakel @tinyhumansai/maintainers — this verified P1 fix is ready to merge. Fresh verification at
Required CI is still blocked by repository-level failures: current |
Adds wire-level e2e coverage for the eight RPC controllers that `scripts/check-domain-e2e-coverage.mjs` reported as uncovered, taking agent 7/12 -> 12/12, approval 3/5 -> 5/5 and memory 34/35 -> 35/35. Every method is driven over the real JSON-RPC router rather than by calling handlers directly. That matters because the checker's notion of "covered" is the string literal "openhuman.<method>" appearing anywhere in a tests/**/*_e2e.rs file — it never verifies the method is invoked. Two of the eight (approval_get_gate_state, approval_preauthorize_flow) were false negatives: they already had handler-level coverage in tool_registry_approval_raw_coverage_e2e.rs and scored 0 only because those tests bypass the wire method name. Includes an #[ignore]d reproduction of openhuman#5862: ComposioExecuteTool and ComposioActionTool declare PermissionLevel::Write but never override external_effect*, so they inherit the trait default of false and ApprovalSecurityMiddleware — whose only gate predicate is external_effect_with_args — never parks them. Agent-initiated Composio writes therefore run with no approval card. The legacy ComposioTool classifies correctly (tools/direct_part_03.rs:66-80) but is not an agent tool, so the classification was simply not carried over when the surface was split. The test asserts the correct behaviour and so fails today; it is ignored to keep the lane green and becomes the regression guard once tinyhumansai#5863 or an equivalent fix lands. Each test was verified by mutation: the covered behaviour was broken and the test confirmed to fail naming its own assertion. Three drafts did not survive that check and were rewritten rather than kept: - an oversized-limit assertion (events.len() <= 1000) was unfalsifiable, since the queried run has no events; - a missing-run_id assertion matched .contains("run_id"), which also matches the downstream error's "run_id=" debug label — it could not tell rejection from acceptance-then-failure. It now matches the exact validate_params string; - a preauthorize idempotency assertion compared grant counts that are both empty when no gate is installed. It now pins the documented gate-absent contract instead, and says why the grant path belongs in the unit tests that own the gate lifecycle.
Adds wire-level e2e coverage for the eight RPC controllers that `scripts/check-domain-e2e-coverage.mjs` reported as uncovered, taking agent 7/12 -> 12/12, approval 3/5 -> 5/5 and memory 34/35 -> 35/35. Every method is driven over the real JSON-RPC router rather than by calling handlers directly. That matters because the checker's notion of "covered" is the string literal "openhuman.<method>" appearing anywhere in a tests/**/*_e2e.rs file — it never verifies the method is invoked. Two of the eight (approval_get_gate_state, approval_preauthorize_flow) were false negatives: they already had handler-level coverage in tool_registry_approval_raw_coverage_e2e.rs and scored 0 only because those tests bypass the wire method name. Includes an #[ignore]d reproduction of openhuman#5862: ComposioExecuteTool and ComposioActionTool declare PermissionLevel::Write but never override external_effect*, so they inherit the trait default of false and ApprovalSecurityMiddleware — whose only gate predicate is external_effect_with_args — never parks them. Agent-initiated Composio writes therefore run with no approval card. The legacy ComposioTool classifies correctly (tools/direct_part_03.rs:66-80) but is not an agent tool, so the classification was simply not carried over when the surface was split. The test asserts the correct behaviour and so fails today; it is ignored to keep the lane green and becomes the regression guard once tinyhumansai#5863 or an equivalent fix lands. Each test was verified by mutation: the covered behaviour was broken and the test confirmed to fail naming its own assertion. Three drafts did not survive that check and were rewritten rather than kept: - an oversized-limit assertion (events.len() <= 1000) was unfalsifiable, since the queried run has no events; - a missing-run_id assertion matched .contains("run_id"), which also matches the downstream error's "run_id=" debug label — it could not tell rejection from acceptance-then-failure. It now matches the exact validate_params string; - a preauthorize idempotency assertion compared grant counts that are both empty when no gate is installed. It now pins the documented gate-absent contract instead, and says why the grant path belongs in the unit tests that own the gate lifecycle.
Adds wire-level e2e coverage for the eight RPC controllers that `scripts/check-domain-e2e-coverage.mjs` reported as uncovered, taking agent 7/12 -> 12/12, approval 3/5 -> 5/5 and memory 34/35 -> 35/35. Every method is driven over the real JSON-RPC router rather than by calling handlers directly. That matters because the checker's notion of "covered" is the string literal "openhuman.<method>" appearing anywhere in a tests/**/*_e2e.rs file — it never verifies the method is invoked. Two of the eight (approval_get_gate_state, approval_preauthorize_flow) were false negatives: they already had handler-level coverage in tool_registry_approval_raw_coverage_e2e.rs and scored 0 only because those tests bypass the wire method name. Includes an #[ignore]d reproduction of openhuman#5862: ComposioExecuteTool and ComposioActionTool declare PermissionLevel::Write but never override external_effect*, so they inherit the trait default of false and ApprovalSecurityMiddleware — whose only gate predicate is external_effect_with_args — never parks them. Agent-initiated Composio writes therefore run with no approval card. The legacy ComposioTool classifies correctly (tools/direct_part_03.rs:66-80) but is not an agent tool, so the classification was simply not carried over when the surface was split. The test asserts the correct behaviour and so fails today; it is ignored to keep the lane green and becomes the regression guard once tinyhumansai#5863 or an equivalent fix lands. Each test was verified by mutation: the covered behaviour was broken and the test confirmed to fail naming its own assertion. Three drafts did not survive that check and were rewritten rather than kept: - an oversized-limit assertion (events.len() <= 1000) was unfalsifiable, since the queried run has no events; - a missing-run_id assertion matched .contains("run_id"), which also matches the downstream error's "run_id=" debug label — it could not tell rejection from acceptance-then-failure. It now matches the exact validate_params string; - a preauthorize idempotency assertion compared grant counts that are both empty when no gate is installed. It now pins the documented gate-absent contract instead, and says why the grant path belongs in the unit tests that own the gate lifecycle.
Adds wire-level e2e coverage for the eight RPC controllers that `scripts/check-domain-e2e-coverage.mjs` reported as uncovered, taking agent 7/12 -> 12/12, approval 3/5 -> 5/5 and memory 34/35 -> 35/35. Every method is driven over the real JSON-RPC router rather than by calling handlers directly. That matters because the checker's notion of "covered" is the string literal "openhuman.<method>" appearing anywhere in a tests/**/*_e2e.rs file — it never verifies the method is invoked. Two of the eight (approval_get_gate_state, approval_preauthorize_flow) were false negatives: they already had handler-level coverage in tool_registry_approval_raw_coverage_e2e.rs and scored 0 only because those tests bypass the wire method name. Includes an #[ignore]d reproduction of openhuman#5862: ComposioExecuteTool and ComposioActionTool declare PermissionLevel::Write but never override external_effect*, so they inherit the trait default of false and ApprovalSecurityMiddleware — whose only gate predicate is external_effect_with_args — never parks them. Agent-initiated Composio writes therefore run with no approval card. The legacy ComposioTool classifies correctly (tools/direct_part_03.rs:66-80) but is not an agent tool, so the classification was simply not carried over when the surface was split. The test asserts the correct behaviour and so fails today; it is ignored to keep the lane green and becomes the regression guard once tinyhumansai#5863 or an equivalent fix lands. Each test was verified by mutation: the covered behaviour was broken and the test confirmed to fail naming its own assertion. Three drafts did not survive that check and were rewritten rather than kept: - an oversized-limit assertion (events.len() <= 1000) was unfalsifiable, since the queried run has no events; - a missing-run_id assertion matched .contains("run_id"), which also matches the downstream error's "run_id=" debug label — it could not tell rejection from acceptance-then-failure. It now matches the exact validate_params string; - a preauthorize idempotency assertion compared grant counts that are both empty when no gate is installed. It now pins the documented gate-absent contract instead, and says why the grant path belongs in the unit tests that own the gate lifecycle.
c86b738 to
90d62c9
Compare
|
Maintainer housekeeping — rebased onto current All five red checks were inherited from the stale base (this branch forked at
No review threads were outstanding — CodeRabbit reported no actionable comments. Not approving; a maintainer reviews and merges separately. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0174 · 183,833 in / 3,078 out · 16,676 cached (9%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash, z-ai/glm-5.2 · 381 embedded
critique: $0.0083 · 82,457 in / 1,898 out · 8,461 cached (10%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
security: $0.0076 · 81,126 in / 1,032 out · 8,215 cached (10%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
tests: $0.0010 · 13,728 in / 72 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0005 · 6,522 in / 76 out · 0 cached (0%) · deepseek/deepseek-v4-flash
| assert!(!tool.external_effect_with_args(&serde_json::json!({ | ||
| "tool": "GMAIL_FETCH_EMAILS" | ||
| }))); | ||
| assert!(tool.external_effect_with_args(&serde_json::json!({}))); |
There was a problem hiding this comment.
Drop assertions for malformed inputs that should not reach the approval gate
The test asserts external_effect_with_args returns true for {} (no tool key) and for {"tool": " "} (whitespace-only value). Neither is a real Composio write request; both are inputs that the execute path should reject as missing/invalid before the approval gate is consulted. Asserting that they trigger an approval prompt would force a false positive approval card in a scenario that does not represent a valid user action, making the test a bad regression guard. Remove these two lines or replace them with assertions that external_effect_with_args returns false (or the tool rejects them earlier via its own validation).
[RULE] misleading-test-assertion ·
tinysweeper
Bot
added
priority: p2
|
Follow-up to my rebase note above — the code review I owe you, and one correction about process. Process correction, so it is on the record: a fleet rule changed after I pushed. Maintainer-side rebases are now limited to org-member PRs, and I should have left this branch alone. I am not reverting it — reverting would put the branch back on a stale base and re-break the five checks — but the force-push was mine, not yours, and the manager has been told. Your two commits are intact and unmodified ( On the change — it is well judged. Two things in particular: Reusing Failing closed everywhere it matters.
Two small notes, neither blocking:
For the record, all five red checks were inherited from the stale base — none of them referenced your diff. Details in my earlier comment. I also ran Not approving; a maintainer reviews and merges. |
2 participants
Summary
WriteandAdminactions through the existing human approval middleware.composio_executeand dynamically registeredComposioActionToolinstances.Problem
Composio tools declared
PermissionLevel::Write, but neither tool surface declared an external effect. Permission caps and interactive approval are separate contracts:ApprovalSecurityMiddlewareconsultsexternal_effect_with_args, whose inherited default wasfalse.As a result, agent-driven external writes such as
GMAIL_SEND_EMAILcould execute without showing the configured approval card.Solution
Reuse the existing static
ToolScopeclassifier as the single source of truth:WriteandAdminactions report an external effect.Readactions do not.toolargument and defaults to effectful for malformed input.This keeps the production change small and avoids a second action-classification table.
Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/ci-lite.yml. Focused branch coverage is present; the authoritative CI gate is pending.## Related.Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/5299-composio-write-approvalc86b738aad182c6d4084aba24aae0714258473bcValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckscripts/ci-cancel-aware.sh cargo test --manifest-path Cargo.toml --lib requires_approval_for_external_writes_only -- --nocapture— 2 passed.approval_identity_scopes_composio_dispatcher_grants_to_one_action— 1 passed.cargo fmt --all -- --check; contributor and product-feature root Clippy lanes both passed with-D warnings.Additional checks:
approval_external_effect_resolution_walks_the_tool_sets— passed.node scripts/ci/check-feature-forwarding.mjs— passed.Validation Blocked
cargo test --manifest-path Cargo.toml --lib 'openhuman::integrations::composio::': two existing tests fail independently of this patch (composio_list_capabilities_does_not_require_sessionexpects a curated Google Calendar capability;composio_execute_via_mock_propagates_backend_errorexpects an older error string). Neither calls the changed approval classifier; the two focused regression tests pass.pnpm --filter openhuman-app rust:clippy: currentmainpoints the Tauri manifest atvendor/tinyagents, whose refreshed pinned commit is now a virtual workspace manifest. Cargo exits before compiling this patch. No Tauri files changed.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
main.Summary by CodeRabbit
New Features
Bug Fixes
Tests