feat(buzz-agent): gate LLM tool calls on session/request_permission - #5712
Open
wpfleger96 wants to merge 3 commits into
Open
feat(buzz-agent): gate LLM tool calls on session/request_permission#5712wpfleger96 wants to merge 3 commits into
wpfleger96 wants to merge 3 commits into
Conversation
Every LLM-issued MCP tool call now asks the client to authorize it before executing. buzz-agent always asks; the client applies BUZZ_ACP_PERMISSION_POLICY. The agent never reads policy, matching the layering of the other ACP harnesses. A crate-local PermissionBroker owns the full correlation lifecycle: a process-wide admission semaphore (BUZZ_AGENT_MAX_PENDING_PERMISSIONS, default 32) acquired before any correlation entry is inserted, a monotonic id allocator, an abort-safe PendingPermission lease whose Drop synchronously removes the entry and releases the slot, claim-before-wake delivery for at-most-once resolution, and a single absolute deadline (BUZZ_AGENT_PERMISSION_TIMEOUT_SECS, default 330s) shared by admission and response wait. Cancellation races inside the wait, never depending on the outer abort drain. The request builder is version-aware, keyed on the protocol version negotiated at initialize and stored for the connection lifetime: v2 nests the tool call under subject, v1 uses the legacy top-level shape. Authorization is fail-closed: execute IFF outcome is "selected" and the selected optionId equals the offered allow option; every other shape denies with a synthetic tool error and the turn continues. Argument-shape validation is hoisted ahead of the ask so a malformed call is rejected locally without prompting. load_skill and _Stop/ _PostCompact lifecycle hooks are exempt — they are not model-issued. Tests: a subprocess + fake-MCP boundary suite proves no call reaches MCP before approval, exact-allow reaches it once, and reject/cancelled/ error/malformed/unknown-outcome/wrong-option/stale-id all fail closed, plus crossed-parallel isolation and the two exemptions; broker unit tests prove timeout, abort, and multi-session admission invariants with an injectable deadline. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…laim-before-wake ordering, reject noncanonical ids Address three review findings on the permission broker's ask surface. An undeliverable ask is now terminal. wire::send_checked surfaces the mpsc send failure that occurs exactly when the writer task has exited on a closed/broken stdout; request_permission fails closed immediately on that error (dropping the lease removes the entry and releases the permit synchronously) instead of leaving a resident waiter to time out. async_main now selects on both the reader and the writer JoinHandle: writer death cancels every session and closes the reader lifecycle rather than reading input while asks wait out their deadline for a reply that can never be written. Claim-before-wake ordering is now mutation-sensitive. Production enforces it structurally — the oneshot sender is consumed by remove, so a send-before-remove mutant cannot compile without swapping the channel type. A test-only wake observer fires synchronously in the waiter's response arm and asserts the entry is already absent from pending at the wake; a faithful wake-before-claim mutant makes it observe false and the test goes red while the behavioral delivery tests stay green. parse_id now requires an exact canonical round-trip, so noncanonical aliases (perm-01, perm-+0, perm-00) that u64::parse would accept are rejected as foreign ids rather than resolving live asks. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… by the deadline Close two wire-closure terminality gaps in the permission broker's ask surface. Writer flush failure is now connection-fatal. A blocking stdout can report Ok from write_all when it only schedules the underlying write and surface the real error at flush; the writer previously discarded that flush error and kept waiting on its receiver, so a genuinely dead stdout left the writer alive, the async_main writer-death arm never fired, and an accepted ask stayed resident until its deadline. write_frames (extracted, generic over its AsyncWrite sink for tests) now returns on either write_all or flush error, dropping the receiver so the connection supervisor cancels every session. Permission enqueue is now the third phase governed by the single absolute deadline. request_permission previously bare-awaited send_checked after registering the entry; a full-but-live channel makes that await wait for capacity, racing neither cancellation nor the deadline, so a stalled writer could hold the ask and its global permit past the advertised deadline and session/cancel could not resolve it. The send now runs in the same biased select as admission and response: cancel wins Cancelled, the shared deadline wins the timeout deny, send error stays the wire-closed deny. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
buzz-agent now authorizes every LLM-issued MCP tool call through
session/request_permissionbefore it executes, instead of running tools unconditionally. The agent always asks; the client appliesBUZZ_ACP_PERMISSION_POLICYand answers. buzz-agent never reads the policy — this keeps the policy decision on the client side, matching the layering of the other ACP harnesses, and avoids duplicating policy logic that would drift.Broker
A crate-local
PermissionBroker(crates/buzz-agent/src/permission.rs), owned byAppfor the connection lifetime, owns the full request-correlation lifecycle:Semaphore(BUZZ_AGENT_MAX_PENDING_PERMISSIONS, default 32, validated>= 1) is acquired before any correlation entry is inserted. This is the security bound: the per-turn tool semaphore is constructed fresh per turn andmax_sessionsis unbounded by default, so neither bounds simultaneously-outstanding asks process-wide.PendingPermissionlease that owns the admission permit and the correlation id. ItsDropsynchronously removes the still-pending entry and releases the slot, covering task abort/panic that bypasses the normalrun_prompttail.deliverclaims (removes) the entry before waking the waiter, so each id resolves once and a later leaseDropis a no-op. Unknown/late ids are logged and dropped; only ids the broker minted (perm-<n>) are recognized.BUZZ_AGENT_PERMISSION_TIMEOUT_SECS, default 330s, validated>= 1), so a saturated call cannot outlive one timeout window even when a stalled writer blocks the enqueue. Cancellation races inside every wait — resolution never depends on the outer abort drain. A writer that dies mid-connection (stdout closed, or a blocking write that only surfaces its error at flush) is connection-fatal: it cancels all sessions, which resolves any ask waiting on a reply that can never be written.Wire
request_permission_params(crates/buzz-agent/src/wire.rs) is version-aware, keyed on the protocol version negotiated atinitializeand stored onAppfor the connection lifetime (never derived from a later mutable session field). v2 nests the tool call undersubject: {type: "tool_call", toolCall}with top-leveltitle/options; v1 uses the legacy top-leveltoolCall. No hybrid shape. Both offered options (allow_once,reject_once) carryoptionId == kind, so the client'skind-based selection and this side'soptionId-based predicate agree without a lookup table.Gate
In each spawned tool task (
crates/buzz-agent/src/agent.rs) the sequence is: acquire per-turn permit → argument-shape validation → broker admission + request + wait → cancellation recheck →emit_in_progress→mcp.call. Argument-shape validation is hoisted out ofmcp.rs::do_callintovalidate_arg_shapeso a malformed non-object argument is rejected locally without prompting for a call that could never execute.Authorization is fail-closed, stated once in
evaluate: execute IFFoutcome.outcome == "selected"and the selectedoptionIdequals the offered allow option. Every other shape (reject, cancelled, JSON-RPC error, malformed, unknown outcome, wrong/unknown option, timeout, wire-channel closure) denies with a synthetic tool error, and the turn continues.Scope
Only LLM-issued MCP calls are gated. The built-in
load_skilltool andcall_hookslifecycle calls (_Stop,_PostCompact) are exempt — they are not model-issued.readOnlyHintis never treated as a security boundary.First cut ships
allow_once/reject_onceonly; session-scoped grants are deliberately out of scope.Related issue
Part of #4938. This PR and #5106 jointly implement the feature: #5106 is the client-side policy engine and permission cards; this PR is buzz-agent's asking side (
session/request_permission). Neither closes #4938 alone.