feat: add ToolPolicy middleware for tool-loop allow/deny gate#2303
feat: add ToolPolicy middleware for tool-loop allow/deny gate#2303Liohtml wants to merge 9 commits into
Conversation
…inyhumansai#2131) Introduces a synchronous ToolPolicy trait in tools/policy.rs that the run_tool_call_loop evaluates before every tool.execute() call. Denied tools return an error ToolResult to the model without executing, preventing any side effects. The DefaultToolPolicy allows all calls, preserving backward compatibility. This complements the existing async agent::tool_policy (session layer) by covering the bus/CLI tool execution path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable tool-policy gate: ChangesTool execution policy gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/agent/harness/tool_loop.rs (1)
608-740:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPolicy gate runs too late in the call pipeline.
On Line 609 and Line 687, approval paths can execute before the policy check at Line 721. A call denied by policy can still trigger approval-side effects first. Move policy evaluation before approval handling (and before approval-gate interception) so denied calls short-circuit immediately.
Suggested reordering
- // ── Approval hook ──────────────────────────────── - if let Some(mgr) = approval { - ... - } - let tool_opt: Option<&dyn Tool> = tools_registry .iter() .chain(extra_tools.iter()) .find(|t| t.name() == call.name && is_visible(t.name())) .map(|b| b.as_ref()); + if let PolicyDecision::Deny(reason) = + tool_policy.evaluate(&call.name, &call.arguments) + { + let denied = format!("Tool '{}' denied by policy: {reason}", call.name); + emit_failed_completion(&denied).await; + individual_results.push(denied.clone()); + let _ = writeln!( + tool_results, + "<tool_result name=\"{}\">\n{denied}\n</tool_result>", + call.name + ); + continue; + } + + // ── Approval hook ──────────────────────────────── + if let Some(mgr) = approval { + ... + }🤖 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/agent/harness/tool_loop.rs` around lines 608 - 740, The PolicyDecision check currently runs after approval handling and the external-effect ApprovalGate; move the policy evaluation to run immediately after resolving tool_opt (i.e., right after the tool lookup/tracing block) so denied tools short-circuit before any approval-side effects. Concretely: relocate the block that calls tool_policy.evaluate(&call.name, &call.arguments) / matches PolicyDecision::Deny (and its emit_failed_completion/individual_results/tool_results handling) to immediately follow the `let tool_opt: Option<&dyn Tool> = ...` and tracing::debug(...) lines; keep later checks (approval manager using ApprovalRequest, mgr.prompt_cli, mgr.record_decision, ApprovalGate::try_global, and tool.external_effect_with_args) after that repositioned policy block so approvals and gate interception never run for policy-denied calls.
🤖 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/agent/harness/tool_loop.rs`:
- Around line 724-729: The diagnostic log currently uses tracing::warn! for the
expected policy-denial branch; change that call to tracing::debug! (or
tracing::trace!) so it uses a lower verbosity level per repo logging rules.
Locate the tracing::warn! invocation that logs iteration, tool =
call.name.as_str(), and reason (the policy-denied branch in tool_loop.rs) and
replace the macro with tracing::debug! while preserving the structured fields
(iteration, tool, reason) and the message string so caller semantics and fields
remain identical.
---
Outside diff comments:
In `@src/openhuman/agent/harness/tool_loop.rs`:
- Around line 608-740: The PolicyDecision check currently runs after approval
handling and the external-effect ApprovalGate; move the policy evaluation to run
immediately after resolving tool_opt (i.e., right after the tool lookup/tracing
block) so denied tools short-circuit before any approval-side effects.
Concretely: relocate the block that calls tool_policy.evaluate(&call.name,
&call.arguments) / matches PolicyDecision::Deny (and its
emit_failed_completion/individual_results/tool_results handling) to immediately
follow the `let tool_opt: Option<&dyn Tool> = ...` and tracing::debug(...)
lines; keep later checks (approval manager using ApprovalRequest,
mgr.prompt_cli, mgr.record_decision, ApprovalGate::try_global, and
tool.external_effect_with_args) after that repositioned policy block so
approvals and gate interception never run for policy-denied calls.
🪄 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: 4a525d67-79cf-4de6-bb36-3a4f42e9ed20
📒 Files selected for processing (9)
src/openhuman/agent/bus.rssrc/openhuman/agent/harness/bughunt_tests.rssrc/openhuman/agent/harness/harness_gap_tests.rssrc/openhuman/agent/harness/test_support_test.rssrc/openhuman/agent/harness/tests.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/agent/harness/tool_loop_tests.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/policy.rs
Policy evaluation now runs before both the approval hook and the external-effect approval gate, so a denied call short-circuits immediately without triggering approval side-effects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Policy denial is an expected control-flow branch, not a warning condition. Downgrade tracing::warn! to tracing::debug! per CodeRabbit review feedback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Credit to @vaddisrinivas for the well-specified issue (#2131) that made this implementation straightforward. The tool-policy middleware design and acceptance criteria in the issue guided the trait shape and test coverage directly. |
|
hey @Liohtml can u enable maintainers to make edits in this PR? and also patch the typecheck and e2e tests? |
coderabbitai
Bot
added
the
feature
|
@senamakel Done — maintainer edits enabled on both PRs (#2303 and #2306). Both branches just merged latest |
|
@senamakel Update: maintainer edits are now enabled on both PRs. The TypeScript and E2E failures are from duplicate property keys in the German i18n file ( Once the German i18n dupes are removed from |
The upstream merge introduced duplicate property keys in the German translation file de-5.ts, causing TS1117 errors in CI. Restored to the clean upstream/main version. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@senamakel Correction: the i18n duplicates in |
|
@Liohtml that's super awesome. thanks for this. just the last few tests that need to pass. if you can have a look at that all good to merge. thanks for your contrivbution! |
The upstream merge added a new test in tool_loop_tests.rs that calls run_tool_call_loop with the pre-policy 16-arg signature. Updated to pass &DefaultToolPolicy as the 17th argument. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Upstream added settings.mascot.customGif{Error,Heading,Label} to the
English locale; the German translation file was missing them, causing
i18n coverage tests to fail.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Closes #2131.
ToolPolicytrait insrc/openhuman/tools/policy.rswith aPolicyDecisionenum (Allow/Deny(String))DefaultToolPolicythat returnsAllowunconditionally (backward compatible)run_tool_call_loop(the bus/CLI tool execution path) -- denied tools return aToolResulterror to the model without executing, preventing any side effectstracing::debug!with tool name and denial reason when a call is denied (updated from warn! per code review)This complements the existing async
agent::tool_policy::ToolPolicy(session layer) by covering the lower-levelrun_tool_call_looppath used by the bus and CLI dispatchers.Scope
Test plan
cargo check -p openhuman --libpassescargo test -p openhuman "tool" --lib-- 1402 tests passcargo test -p openhuman "policy" --lib-- 153 tests pass (includes 5 new)cargo fmt --allapplied🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests