Skip to content

feat(acp): allow host policy to decide permission requests - #4066

Closed
Silentpartnercoding wants to merge 1 commit into
block:mainfrom
Silentpartnercoding:feat/acp-permission-policy
Closed

feat(acp): allow host policy to decide permission requests#4066
Silentpartnercoding wants to merge 1 commit into
block:mainfrom
Silentpartnercoding:feat/acp-permission-policy

Conversation

@Silentpartnercoding

@Silentpartnercoding Silentpartnercoding commented Aug 1, 2026

Copy link
Copy Markdown

Follow-up to the permission-boundary analysis in #2931.

Buzz’s generic ACP client currently selects allow_once whenever that option is offered. This change adds an injectable asynchronous permission-decision callback so the host can choose among the exact options supplied by the agent.

The callback receives the permission request and offered options. A policy decision may select only an option that was actually offered. If the policy is unavailable, times out, is cancelled, returns an error, or selects an invalid option, the client fails closed through an offered rejection option.

The focused tests cover:

  • a deny decision emits a rejection response and never selects an allow option
  • option kinds and IDs are preserved exactly
  • string and numeric JSON-RPC request IDs
  • missing policy, timeout, cancellation, callback error, and invalid selection
  • response-write failure and cancellation cleanup

Trust boundary

This governs cooperative operations that cross session/request_permission. It does not provide general filesystem, subprocess, network, MCP, runtime, or operating-system containment. Runtimes that perform work without issuing a permission request remain outside this policy boundary.

Verification

  • cargo test -p buzz-acp
  • cargo build -p buzz-acp
  • cargo fmt --check
  • cargo clippy -p buzz-acp --all-targets -- -D warnings
  • focused deny-path JSON-RPC regression tests
  • git diff --check

@Silentpartnercoding
Silentpartnercoding requested a review from a team as a code owner August 1, 2026 05:58
@Silentpartnercoding

Copy link
Copy Markdown
Author

@BradGroux I implemented the narrow permission-decision callback experiment you outlined and kept the guarantee limited to cooperative session/request_permission traffic. The deny-path tests inspect the emitted JSON-RPC response and verify that no allow option is selected. I’d appreciate your review of the API shape, fail-closed behavior, and boundary wording.

Signed-off-by: James <james@Jamess-MBP-2.lan>
@Silentpartnercoding
Silentpartnercoding force-pushed the feat/acp-permission-policy branch from a08cf54 to e9159e4 Compare August 1, 2026 06:06
@Silentpartnercoding

Copy link
Copy Markdown
Author

Quick relevance check now that Buzz’s ACP surface has continued moving: #4066 is still mergeable against current main, and its DCO, Semgrep, and zizmor checks remain green.

Since this PR opened, #4155 and its DCO-clean replacement #4333 have added a broader Guardian proposal that also changes handle_permission_request. I reviewed the overlap rather than assuming this PR was still unique.

The distinction I see is that #4333 derives an allow/reject choice from Buzz’s fixed permission modes, while #4066 exposes a generic asynchronous host-policy callback, preserves the agent’s exact offered option IDs and kinds, and handles timeout, cancellation, policy errors, and invalid selections explicitly.

One boundary I would appreciate maintainer direction on: #4066 currently fails closed when no callback is installed, changing the historical automatic allow_once behavior. I can instead make the callback opt-in and preserve compatibility until a caller installs it, or adapt the seam to the Guardian direction if that is now preferred.

Is the generic injectable seam still useful alongside #4333, or would you prefer that this PR be revised or closed in favor of the Guardian implementation?

@Silentpartnercoding

Copy link
Copy Markdown
Author

Revisiting my own default here after watching #4609 land and then get reverted.

As written, this PR fails closed when no policy is available — so a Buzz with nothing installed denies where it previously selected allow_once. That is a behaviour change for every existing user, and it is what turns this from a code review into a product decision. That is not a decision I should be asking a single reviewer to carry, and I would rather it not be one at all.

Two shapes, whichever you prefer:

  1. Default preserved. No policy installed keeps the current allow_once selection. Fail-closed applies only when a policy is installed and is unavailable, times out, is cancelled, errors, or returns an option that was not offered. Strictness becomes opt-in rather than imposed.
  2. Seam only. Land the injectable callback with every existing path behaving exactly as it does today, and take the strictness question separately whenever you want it.

Both keep the fail-closed machinery and the focused tests as they are — the only thing that moves is which branch handles "no policy at all". Option 1 is a small change and I am happy to push it on request; say which you would rather have and I will do the work.

@Silentpartnercoding

Copy link
Copy Markdown
Author

Closing as superseded by #5712, which has now landed Buzz-native permission gating at the actual LLM tool-call boundary, including fail-closed handling for malformed or invalid permission responses. With that implementation on main, maintaining this older generic ACP callback as a conflicting parallel path no longer looks useful. The upstream solution is the better integration point. Thanks to everyone who moved the permission boundary forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant