fix(agent-harness): structured policy-block messages with workaround (#4094)

[M3gA-Mind]

Closes #4094

Problem

When a tool policy / permission boundary blocks an action, the session tool executor returned a bare denial line (e.g. Tool 'X' blocked by tool policy: requires Execute, channel 'web' allows ReadOnly). The agent could dead-end on it — no clear next step, no way to unblock — leaving the user stuck.

Enforcement approach

New session/policy_denial.rs renders every policy/permission denial as a structured message: Blocked: <what>. Reason: <why>. Workaround: <how-to-enable / permitted alternative>. Relay this to the user…. The relay instruction is baked in so the agent surfaces an explanation + next step rather than silently halting.

agent_tool_exec::run_agent_tool_call now routes all four block branches through it:

• Session policy denial & per-call permission too low → names the required vs. granted permission and tells the agent how to raise the channel's agent-access tier (Settings → Agent access / config.update_autonomy_settings / [autonomy]), or to fall back to a lower-permission tool.
• Pluggable ToolPolicy Deny → carries the policy's reason plus "use a permitted alternative / ask the user to adjust the policy" (matches the issue's run_script → sandbox restriction example).
• RequireApproval → tells the agent to ask the user to approve, then retry.

The message is returned as the failed tool result, so it flows back into the turn the same way the unknown-tool corrective error is surfaced to the model (sibling PR #4360).

Acceptance

• ✅ Policy denials return a structured reason + suggested alternative to the agent.
• ✅ The agent is instructed to relay a user-facing explanation and next step rather than silently halting.

Tests

• Unit tests in policy_denial.rs for each denial variant's rendered message.
• Integration tests in agent_tool_exec.rs: a session-policy-denied Execute tool and a pluggable-policy-denied call each yield the structured message (reason + workaround + relay instruction) with success == false and a completion emitted through progress (not a silent drop).

Caveat: local test run was blocked by the pre-existing whisper-rs-sys Metal linker error on this macOS SDK (_OBJC_CLASS_$_MTLResidencySetDescriptor, documented in CLAUDE.md), unrelated to this change — the crate compiles; CI runs the tests. This area overlaps PR #4360 (session/agent_tool_exec.rs); will rebase onto it once it lands.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 13df7c95-cb34-4d37-930b-9af95edad6b1

📥 Commits

Reviewing files that changed from the base of the PR and between 4c98a31 and 6450b94.

📒 Files selected for processing (4)

• src/openhuman/agent/harness/session/agent_tool_exec.rs
• src/openhuman/agent/harness/session/mod.rs
• src/openhuman/agent/harness/session/policy_denial.rs
• src/openhuman/tinyagents/middleware.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80e75c6aa0

ℹ️ 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".

[M3gA-Mind]

Review follow-up (commit 6450b94)

Addressed both Codex findings:

• P1 — structured denials never reached real turns. The production turn path (turn() → run_turn_via_tinyagents_session) runs through the tinyagents ToolPolicyMiddleware, which had its own copy of the denial branches emitting the old bare strings. PolicyDenial is now pub(crate) and rendered in the middleware too — channel_permission_block (session-deny + per-call ceiling) and the pluggable-policy (Deny / RequireApproval) block. Added a middleware regression test (channel_permission_block_returns_structured_denial) asserting the structured reason + workaround + relay instruction on the turn path.
• P2 — wrong workaround target. Permission-ceiling denials are capped by [agent].channel_permissions (from build_session), not autonomy. The workaround now points at that config with valid level tokens (read_only/write/execute/dangerous) instead of config.update_autonomy_settings / [autonomy] (which only sets readonly|supervised|full and never accepts an "Execute" tier). Pluggable-policy workarounds are unchanged (not channel-ceiling based).

cargo fmt --check clean; cargo check --lib --tests passes locally (full test link is blocked by the pre-existing whisper-rs-sys Metal SDK issue, so CI runs the suite).

[M3gA-Mind]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sanil-23]

Review — #4390 · structured policy-block messages with workaround (#4094)

Verdict: LGTM with one coverage gap worth closing. The change is well-scoped and the follow-up commit (6450b94) correctly fixed both Codex findings — the structured message now reaches the real turn path (the tinyagents ToolPolicyMiddleware), and the workaround points at [agent].channel_permissions with valid tokens rather than the wrong [autonomy] knob. I traced both execution paths and the security/policy layer; the core logic is sound.

Walkthrough

PolicyDenial (new session/policy_denial.rs) renders four denial variants (SessionForbidden, PermissionTooLow, PolicyDenied, ApprovalRequired) into a Blocked / Reason / Workaround / relay-instruction message. It's now shared (pub(crate)) between the two mutually-exclusive tool-exec paths — the legacy agent_tool_exec::run_agent_tool_call (via Agent::turn) and the tinyagents ToolPolicyMiddleware (production turn path). Good de-dup of the rendering; the branch structure is still mirrored across the two files, which is inherent to the #4249 migration living in both worlds.

What I verified

• No message duplication / double-check. The two paths are exclusive: tinyagents tools are registered as adapters and gated by the middleware (which short-circuits before next.run), so agent_tool_exec's denial branches never fire on the turn path. denials::record / policy.check run once per call. ✅
• P2 token correctness. permission_config_token emits read_only/write/execute/dangerous; parse_permission_level strips -/_ and lowercases, so every suggested token round-trips. ✅
• No fragile downstream consumers. Grepped src/ + app/src/ — nothing pattern-matches the old wording ("blocked by tool policy: requires", "action requires ... permission"), so changing the string breaks no parser. ✅
• Surfacing. Denials return as error: Some(message) ⇒ success=false, flowing into the timeline as a failed call — consistent with the unknown-tool corrective pattern. ✅

Findings (prioritized)

1. (major — coverage) The middleware pluggable-policy branch is untested on the turn path.
middleware.rs:858-873 (the Deny/RequireApproval → PolicyDenial render inside call()) has no test. The new middleware test channel_permission_block_returns_structured_denial calls channel_permission_block() directly and never exercises the full call() → self.policy.check() deny branch. The equivalent branch in agent_tool_exec.rs is covered (pluggable_policy_denial_returns_structured_message_with_alternative), but that's the other path. Net: the production turn path's pluggable Deny/RequireApproval rendering is unverified, and those changed lines may also miss the ≥80% diff-cover gate. Suggest a middleware test that drives the whole call() with a DenyingToolPolicy (and one RequireApproval) so both the message and the error=Some(..) surfacing are asserted end-to-end.

2. (minor — conflict) Merge-order coordination with sibling #4389. #4389 (change strategy on repeated unproductive tool results) also edits tinyagents/middleware.rs and tinyagents/mod.rs. Different regions, so likely auto-mergeable, but flag the rebase order between the two. (#4386/#4387 touch turn/* and don't overlap; #4419 touches only observability.rs.)

3. (nitpick) Odd phrasing when allowed == PermissionLevel::None. The shared workaround tail reads "…a tool that needs only None access." Only reachable if a channel is capped at None; cosmetic. Could special-case None to "a read-only tool" or drop the tail.

Question for the author

• #4419 tool-timeline interaction: the denial is now a multi-sentence paragraph carried in both content and error. Does the timeline render (or truncate) that cleanly for a blocked call, and is duplicating the string across content+error still the intended shape post-#4419? (This mirrors the pre-existing pattern, so not a regression — just confirming.)

Looks good

• Correct variant selection (RequireApproval vs Deny) in both files; reason borrow lifetime is fine.
• Strong unit coverage of all four PolicyDenial variants, plus session + pluggable integration tests on the agent_tool_exec path.
• Doc comments accurately explain why the workaround targets channel_permissions, not autonomy — this is exactly the kind of "why" comment the repo asks for.

[sanil-23]

Requesting changes on one point (details in my review comment above).

Blocker to resolve before merge — untested pluggable-policy render on the production turn path. middleware.rs:858-873 renders the Deny/RequireApproval denial inside call(), but no test exercises it: the new channel_permission_block_returns_structured_denial test calls channel_permission_block() directly and never reaches self.policy.check(). The equivalent branch is covered only on the other (agent_tool_exec) path. Net effect:

1. The production turn path's pluggable Deny/RequireApproval message + error=Some(..) surfacing is unverified.
2. Those changed lines likely miss the ≥80% diff-cover gate (.github/workflows/pr-ci.yml), which will block merge anyway.

Please add a ToolPolicyMiddleware test that drives the full call() with a denying policy (and one RequireApproval) asserting the structured message reaches the TaToolResult. Everything else — the P1/P2 fixes, token correctness, the four PolicyDenial variants — looks good; this is the only thing holding it up.