Thread tool call context through policy#2334
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 (2)
📝 WalkthroughWalkthroughA new ChangesTool call context refactoring
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent::execute_tool_call
participant Policy as ToolPolicy
participant Executor as ToolExecutor
Agent->>Policy: ToolPolicyRequest::new(tool_name, args, ToolCallContext::session(...))
Policy->>Agent: allow / deny
Agent->>Executor: execute tool call (if allowed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/agent/tool_policy.rs (1)
53-63: ⚡ Quick winMake
ToolPolicyRequestderive its mirror fields fromcontext.These fields are documented as mirrors, but direct struct literals can still let them drift from
context, which means policy results can differ based on which copy gets read. A small constructor keeps that invariant in one place for production code and tests.♻️ Suggested shape
pub struct ToolPolicyRequest { pub tool_name: String, pub arguments: serde_json::Value, pub context: ToolCallContext, /// Backward-compatible mirror of `context.session_id`. pub session_id: String, /// Backward-compatible mirror of `context.channel`. pub channel: String, /// Backward-compatible mirror of `context.agent_definition_id`. pub agent_definition_id: String, } + +impl ToolPolicyRequest { + pub fn new( + tool_name: impl Into<String>, + arguments: serde_json::Value, + context: ToolCallContext, + ) -> Self { + Self { + tool_name: tool_name.into(), + arguments, + session_id: context.session_id.clone(), + channel: context.channel.clone(), + agent_definition_id: context.agent_definition_id.clone(), + context, + } + } +}🤖 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 53 - 63, ToolPolicyRequest currently exposes mirror fields (session_id, channel, agent_definition_id) that can drift from context; add a single constructor (e.g., impl ToolPolicyRequest { pub fn new(tool_name: String, arguments: serde_json::Value, context: ToolCallContext) -> Self }) that builds the struct and sets session_id = context.session_id.clone(), channel = context.channel.clone(), and agent_definition_id = context.agent_definition_id.clone(); switch existing call sites to use this constructor and consider making the mirror fields non-pub (or documenting they must be created via new) so the invariant is enforced in one place (refer to ToolPolicyRequest, context, session_id, channel, agent_definition_id).
🤖 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/tool_policy.rs`:
- Around line 53-63: ToolPolicyRequest currently exposes mirror fields
(session_id, channel, agent_definition_id) that can drift from context; add a
single constructor (e.g., impl ToolPolicyRequest { pub fn new(tool_name: String,
arguments: serde_json::Value, context: ToolCallContext) -> Self }) that builds
the struct and sets session_id = context.session_id.clone(), channel =
context.channel.clone(), and agent_definition_id =
context.agent_definition_id.clone(); switch existing call sites to use this
constructor and consider making the mirror fields non-pub (or documenting they
must be created via new) so the invariant is enforced in one place (refer to
ToolPolicyRequest, context, session_id, channel, agent_definition_id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b17eb976-0799-43f9-90b6-c3137adcaef1
📒 Files selected for processing (3)
src/openhuman/agent/harness/session/turn.rssrc/openhuman/agent/harness/session/turn_tests.rssrc/openhuman/agent/tool_policy.rs
vaddisrinivas
changed the title
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — Thread tool call context through policy
Clean, well-structured PR that introduces ToolCallContext and threads it through the session→policy path. The backward-compat approach (mirror fields + ::new() constructor) is solid. CI is all green and the existing test coverage was updated properly.
| Area | Files | Verdict |
|---|---|---|
| Rust core (tool policy) | tool_policy.rs |
1 major, 2 minor |
| Rust core (session turn) | turn.rs |
Clean |
| Tests | turn_tests.rs |
Clean |
Nice work on keeping the change surface small and backward-compatible. The major item below is about acceptance criteria alignment — the rest is polish.
|
@vaddisrinivas unresolved review feedback from graycyrus — please address before we review. |
|
Thanks for the nudge. I addressed the graycyrus review threads inline in 9200fd2 and marked the fixed conversations resolved. Re-review welcome. |
… codex/oh-2132-tool-call-context # Conflicts: # src/openhuman/agent/harness/session/turn.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review — all prior findings addressed.
The three items from the previous review are resolved:
| Finding | Status |
|---|---|
[major] Debug leaks session_id/channel verbatim |
Fixed — custom Debug impls with redact_for_debug(), verified by new debug_redacts_sensitive_context_fields test |
[minor] Unused ToolCallSource variants trigger dead_code |
Fixed — #[allow(dead_code)] with doc comment noting follow-up ingress paths |
[minor] Backward-compat fields lack #[deprecated] |
Fixed — #[deprecated(note = "use context.*")] on all three mirror fields |
Issue #2132 acceptance criteria look fully covered now: backward compat preserved, session path tested with context assertions, policy receives context before execution, sensitive fields redacted from debug output.
Clean from my side — no new findings.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
3 participants
Refs #2132
Summary
ToolCallContextto tool policy requests.Codex PR Checklist
codex/oh-2132-tool-call-context2c87a4f7bdb975f6f9e3bce7bbaf4567b18b52dfsrc/openhuman/agent/tool_policy.rsadds the context shape;src/openhuman/agent/harness/session/turn.rspopulates it for session tool calls;src/openhuman/agent/harness/session/turn_tests.rsasserts policy receives it.cargo fmt --manifest-path Cargo.toml --all --check;cargo test --manifest-path Cargo.toml tool_policy;node scripts/check-pr-checklist.mjs /tmp/pr-body-2132.md.pnpm pr:checklist /tmp/pr-body-2132.mdblocked withzsh:1: command not found: pnpm; used the same checklist checker vianode scripts/check-pr-checklist.mjs /tmp/pr-body-2132.md.vaddisrinivas:codex/oh-2132-tool-call-contextor issue2132before opening this draft PR.Summary by CodeRabbit
Refactor
Tests