Add runtime policy, revocation, and audit correlation for generated tools#2547
Conversation
|
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds generated-tool runtime context to policy requests, a RequireApproval decision and blocking_reason(), GeneratedToolRuntimePolicy implementation and tests, a Tool hook to supply context, executor threading and denial handling, and audit correlation for generated-tool events. ChangesGenerated Tool Policy and Audit
Sequence DiagramsequenceDiagram
participant Agent as Agent::execute_tool_call
participant Policy as GeneratedToolRuntimePolicy::check
participant Decision as ToolPolicyDecision
participant Audit as AuditLogger::log_generated_tool_event
Agent->>Policy: check(request + generated_tool context)
Policy->>Decision: Allow / RequireApproval / Deny (with reason)
Decision->>Agent: decision (optional blocking reason)
alt blocked or requires approval
Agent->>Audit: log_generated_tool_event(provider_id, capability_id, policy_decision, approval_id)
else allowed
Agent->>Agent: proceed with tool execution
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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
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: 1
🧹 Nitpick comments (1)
src/openhuman/agent/tool_policy.rs (1)
236-310: ⚡ Quick winAdd branch-level debug tracing for runtime policy decisions.
At Lines 236-310, generated-tool policy evaluation has multiple enforcement branches (revoked provider/capability, provider/capability/risk mappings, default allow) but emits no local debug/trace diagnostics, which makes runtime policy outcomes hard to audit during incidents.
♻️ Suggested patch
impl GeneratedToolRuntimePolicy { @@ fn action_for( &self, tool_name: &str, context: &GeneratedToolRuntimeContext, ) -> (RuntimeToolPolicyAction, String) { @@ - if let Some(action) = self.config.risk_actions.get(&context.risk) { + if let Some(action) = self.config.risk_actions.get(&context.risk) { return ( *action, format!("risk `{:?}` matched runtime policy", context.risk), ); } ( RuntimeToolPolicyAction::Allow, format!("tool `{tool_name}` allowed"), ) } } @@ async fn check(&self, request: &ToolPolicyRequest) -> ToolPolicyDecision { if !self.config.enabled { + tracing::trace!( + policy = self.name(), + tool = request.tool_name.as_str(), + "[rpc] generated tool runtime policy disabled; allowing" + ); return ToolPolicyDecision::Allow; } let Some(context) = request.generated_tool.as_ref() else { + tracing::trace!( + policy = self.name(), + tool = request.tool_name.as_str(), + "[rpc] generated tool context missing; allowing" + ); return ToolPolicyDecision::Allow; }; let (action, reason) = self.action_for(&request.tool_name, context); + tracing::debug!( + policy = self.name(), + tool = request.tool_name.as_str(), + provider_id = context.provider_id.as_str(), + capability_id = context.capability_id.as_str(), + risk = ?context.risk, + action = ?action, + reason = reason.as_str(), + "[rpc] generated tool runtime policy decision" + ); match action { RuntimeToolPolicyAction::Allow => ToolPolicyDecision::Allow, RuntimeToolPolicyAction::RequireApproval => { ToolPolicyDecision::require_approval(reason) } RuntimeToolPolicyAction::Deny => ToolPolicyDecision::deny(reason), } } }As per coding guidelines: "Use
log/tracingatdebugortracelevel on ... state transitions, and any branch that is hard to infer from tests alone."🤖 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/tool_policy.rs` around lines 236 - 310, The action_for/check flow in GeneratedToolRuntimePolicy lacks branch-level debug tracing; add debug or trace logs inside GeneratedToolRuntimePolicy::action_for for each branch (revoked provider, revoked capability, capability_actions hit, provider_actions hit, risk_actions hit, and default allow) emitting the chosen RuntimeToolPolicyAction, the formatted reason, tool_name, context.provider_id, context.capability_id, and context.risk as appropriate, and also log the final decision in GeneratedToolRuntimePolicy::check right before returning the ToolPolicyDecision (use ToolPolicyDecision::require_approval / deny / Allow paths); reference the action_for function, the RuntimeToolPolicyAction enum, and the check method to locate insertion points.
🤖 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/session/turn.rs`:
- Around line 1202-1205: The policy check is using ToolPolicyRequest::new which
leaves generated_tool as None so generated-tool runtime policy rules never run;
before calling self.tool_policy.check(&policy_request).await, populate the
request's generated_tool context (e.g., via the request builder/constructor that
accepts generated tool metadata or by setting the generated_tool field on the
ToolPolicyRequest you created) so the generated-tool runtime policy (revoked
provider/capability/risk action mappings) is available to the check; update the
code path around ToolPolicyRequest::new and the call site of
self.tool_policy.check to pass the generated tool context into the request.
---
Nitpick comments:
In `@src/openhuman/agent/tool_policy.rs`:
- Around line 236-310: The action_for/check flow in GeneratedToolRuntimePolicy
lacks branch-level debug tracing; add debug or trace logs inside
GeneratedToolRuntimePolicy::action_for for each branch (revoked provider,
revoked capability, capability_actions hit, provider_actions hit, risk_actions
hit, and default allow) emitting the chosen RuntimeToolPolicyAction, the
formatted reason, tool_name, context.provider_id, context.capability_id, and
context.risk as appropriate, and also log the final decision in
GeneratedToolRuntimePolicy::check right before returning the ToolPolicyDecision
(use ToolPolicyDecision::require_approval / deny / Allow paths); reference the
action_for function, the RuntimeToolPolicyAction enum, and the check method to
locate insertion 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: e9224089-8318-4a32-ba09-0fbac4e981b7
📒 Files selected for processing (3)
src/openhuman/agent/harness/session/turn.rssrc/openhuman/agent/tool_policy.rssrc/openhuman/security/audit.rs
|
Security/review note: the new |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/openhuman/agent/tool_policy.rs (1)
455-507: ⚡ Quick winAdd a regression test for the
enabled + missing generated contextallow path.That branch is the backward-compatibility escape hatch, but the new tests only cover
enabled=falseandenabled=truewith generated context. A future change here could start blocking built-in tools without a test catching it.Proposed test shape
+ #[tokio::test] + async fn generated_runtime_policy_allows_when_context_missing() { + let policy = GeneratedToolRuntimePolicy::new(GeneratedToolRuntimePolicyConfig { + enabled: true, + ..Default::default() + }); + + let request = ToolPolicyRequest::new( + "echo", + serde_json::json!({ "value": 1 }), + ToolCallContext::session("session", "chat", "orchestrator", "call-1", 1), + ); + + assert_eq!(policy.check(&request).await, ToolPolicyDecision::Allow); + }🤖 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/tool_policy.rs` around lines 455 - 507, Add a regression test to ensure the Enabled=true but missing generated context path returns Allow: create a new #[tokio::test] (e.g., generated_runtime_policy_allows_when_enabled_but_missing_context) that constructs GeneratedToolRuntimePolicy::new(GeneratedToolRuntimePolicyConfig { enabled: true, ..Default::default() }), call policy.check(&generated_request_missing_context()).await (or reuse generated_request() but remove/omit the generated context fields), and assert the result equals ToolPolicyDecision::Allow; reference GeneratedToolRuntimePolicy, GeneratedToolRuntimePolicyConfig, policy.check, and generated_request to locate where to add the test so built-in tools remain allowed when generated context is absent.src/openhuman/agent/harness/session/turn.rs (1)
1204-1230: ⚡ Quick winExtract this policy gate into a small helper.
execute_tool_callis already carrying a lot of hot-path responsibility, and this new generated-context enrichment + block-message formatting adds another policy-specific branch inline. Pulling it into a helper would make future policy changes easier and stop growing an already oversized module.As per coding guidelines
**/*.{ts,tsx,rs}: File size should not exceed approximately 500 lines. When a module grows beyond this threshold, split it into smaller, more focused modules with clear responsibilities.🤖 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/session/turn.rs` around lines 1204 - 1230, The policy-check block inside execute_tool_call (creating ToolPolicyRequest, enriching it with tool.generated_runtime_context(&call.arguments), calling self.tool_policy.check(&policy_request).await, inspecting policy_decision.blocking_reason(), and formatting the blocked message) should be extracted into a small helper function (e.g. compute_tool_policy_outcome or check_tool_policy) that takes the tool name, call.arguments, and context and returns a (message, allowed: bool) or a custom enum; move the ToolPolicyRequest creation and the with_generated_tool_context call, the self.tool_policy.check invocation, and the ToolPolicyDecision match/formatting into that helper, and replace the inline block in execute_tool_call with a single call to the new helper so future policy changes and generated_runtime_context enrichment live in their own unit.src/openhuman/agent/harness/session/turn_tests.rs (1)
655-695: 🏗️ Heavy liftMove the generated-tool policy cases into their own test module.
This file is already very large and now mixes turn lifecycle, transcript, memory, prompt, and generated-tool policy coverage. Splitting these new cases into a dedicated sibling test module would make failures easier to localize and keep this suite navigable.
As per coding guidelines
**/*.{ts,tsx,rs}: File size should not exceed approximately 500 lines. When a module grows beyond this threshold, split it into smaller, more focused modules with clear responsibilities.🤖 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/session/turn_tests.rs` around lines 655 - 695, Split the generated-tool policy tests (e.g., the test function execute_tool_call_threads_generated_tool_context_into_policy and any related helpers/types like GeneratedContextTool, RequireGeneratedContextPolicy, ParsedToolCall) out of the large turn_tests.rs into a new sibling test module file (e.g., generated_tool_policy_tests.rs) and update module imports; move the test functions and any small helper structs/fixtures they rely on, add the necessary use/imports (tokio::test, tempfile, Arc, AtomicUsize, Ordering, Agent::builder, XmlToolDispatcher, Memory creation helpers) at the top of the new file, keep the #[tokio::test] attributes and assertions unchanged, and ensure the new file is included in the test build (via mod declaration or Cargo test discovery) so the tests compile and run as before.
🤖 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.
Nitpick comments:
In `@src/openhuman/agent/harness/session/turn_tests.rs`:
- Around line 655-695: Split the generated-tool policy tests (e.g., the test
function execute_tool_call_threads_generated_tool_context_into_policy and any
related helpers/types like GeneratedContextTool, RequireGeneratedContextPolicy,
ParsedToolCall) out of the large turn_tests.rs into a new sibling test module
file (e.g., generated_tool_policy_tests.rs) and update module imports; move the
test functions and any small helper structs/fixtures they rely on, add the
necessary use/imports (tokio::test, tempfile, Arc, AtomicUsize, Ordering,
Agent::builder, XmlToolDispatcher, Memory creation helpers) at the top of the
new file, keep the #[tokio::test] attributes and assertions unchanged, and
ensure the new file is included in the test build (via mod declaration or Cargo
test discovery) so the tests compile and run as before.
In `@src/openhuman/agent/harness/session/turn.rs`:
- Around line 1204-1230: The policy-check block inside execute_tool_call
(creating ToolPolicyRequest, enriching it with
tool.generated_runtime_context(&call.arguments), calling
self.tool_policy.check(&policy_request).await, inspecting
policy_decision.blocking_reason(), and formatting the blocked message) should be
extracted into a small helper function (e.g. compute_tool_policy_outcome or
check_tool_policy) that takes the tool name, call.arguments, and context and
returns a (message, allowed: bool) or a custom enum; move the ToolPolicyRequest
creation and the with_generated_tool_context call, the self.tool_policy.check
invocation, and the ToolPolicyDecision match/formatting into that helper, and
replace the inline block in execute_tool_call with a single call to the new
helper so future policy changes and generated_runtime_context enrichment live in
their own unit.
In `@src/openhuman/agent/tool_policy.rs`:
- Around line 455-507: Add a regression test to ensure the Enabled=true but
missing generated context path returns Allow: create a new #[tokio::test] (e.g.,
generated_runtime_policy_allows_when_enabled_but_missing_context) that
constructs GeneratedToolRuntimePolicy::new(GeneratedToolRuntimePolicyConfig {
enabled: true, ..Default::default() }), call
policy.check(&generated_request_missing_context()).await (or reuse
generated_request() but remove/omit the generated context fields), and assert
the result equals ToolPolicyDecision::Allow; reference
GeneratedToolRuntimePolicy, GeneratedToolRuntimePolicyConfig, policy.check, and
generated_request to locate where to add the test so built-in tools remain
allowed when generated context is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3d42823-8558-4ccc-97a2-47b2157b3ff9
📒 Files selected for processing (3)
src/openhuman/agent/harness/session/turn.rssrc/openhuman/agent/harness/session/turn_tests.rssrc/openhuman/agent/tool_policy.rs
Fix compile error in turn_tests where `create_memory` was imported from `crate::openhuman::memory` instead of `crate::openhuman::memory_store`. Add regression test for the enabled-but-missing-generated-context allow path (addresses @coderabbitai nitpick on tool_policy.rs).
|
@YOMXXX Good observation re:
This is intentionally a safe v1 primitive — fail-closed by default, with the variant preserved so a future approval-prompt integration can branch on |
3 participants
Summary
Problem
Solution
ToolPolicyRequestwith optionalGeneratedToolRuntimeContext.ToolPolicyDecision::RequireApprovaland a sharedblocking_reasonhelper so the existing session runner blocks non-allow decisions.GeneratedToolRuntimePolicyand structured generated-tool audit helpers while preserving the default allow-all policy.Submission Checklist
## Related: N/A, no coverage-matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md): N/A, no release-cut UI/runtime smoke surface changed.Closes #NNNin the## Relatedsection.Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/oh-2543-generated-tool-runtime-policy7445c7dfc996f1503508cd628f44ed670400de87Validation Run
pnpm --filter openhuman-app format:check: N/A, Rust-only backend policy/audit change.pnpm typecheck: N/A, Rust-only backend policy/audit change.cargo test --manifest-path Cargo.toml generated_runtime_policy --lib;cargo test --manifest-path Cargo.toml audit_log_generated_tool --lib;cargo test --manifest-path Cargo.toml threads_generated_tool_context --libcargo fmt --manifest-path Cargo.toml --all --check;git diff --checknode scripts/check-pr-checklist.mjs /tmp/openhuman-pr-body-2543.mdValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
vaddisrinivas:codex/oh-2543-generated-tool-runtime-policy.Summary by CodeRabbit
New Features
Tests