Skip to content

Commit a83200b

Browse files
committed
fix(acp): preserve permission decision routing
Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com>
1 parent aeb8f71 commit a83200b

2 files changed

Lines changed: 154 additions & 19 deletions

File tree

crates/buzz-acp/src/acp.rs

Lines changed: 150 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,9 @@ pub struct PermissionDecision {
169169
pub option_id: String,
170170
}
171171

172+
type SharedPermissionDecisionReceiver =
173+
std::sync::Arc<tokio::sync::Mutex<tokio::sync::mpsc::Receiver<PermissionDecision>>>;
174+
172175
/// Lifecycle state of a single `session/request_permission` request under
173176
/// the `ask` policy.
174177
#[derive(Debug, Clone)]
@@ -252,8 +255,11 @@ pub struct AcpClient {
252255
owner_pubkey_known: bool,
253256
/// Channel for delivering `permission_decision` control frames from the
254257
/// observer dispatch loop into the read loop's decision arm.
255-
/// Installed by `install_permission_decision_rx`; consumed by the read loop.
256-
permission_decision_rx: Option<tokio::sync::mpsc::Receiver<PermissionDecision>>,
258+
/// Installed by `install_permission_decision_rx` and shared across every
259+
/// prompt read loop in the task. Keeping the receiver behind an `Arc`
260+
/// prevents an initial-message prompt or cancellation from consuming the
261+
/// only decision route before the main prompt runs.
262+
permission_decision_rx: Option<SharedPermissionDecisionReceiver>,
257263
/// The JSON-RPC id of the most recently sent `session/prompt` request.
258264
/// Used by [`cancel_with_cleanup`] to drain the correct response.
259265
/// Set in [`session_prompt_with_idle_timeout`]; consumed in [`cancel_with_cleanup`].
@@ -696,7 +702,7 @@ impl AcpClient {
696702
&mut self,
697703
rx: tokio::sync::mpsc::Receiver<PermissionDecision>,
698704
) {
699-
self.permission_decision_rx = Some(rx);
705+
self.permission_decision_rx = Some(std::sync::Arc::new(tokio::sync::Mutex::new(rx)));
700706
}
701707

702708
/// Update metadata that will be attached to subsequent raw wire events.
@@ -1750,10 +1756,10 @@ impl AcpClient {
17501756
// so the ack_tx oneshot is never leaked silently).
17511757
let mut steer_rx = self.steer_rx.take();
17521758

1753-
// Take the per-session permission decision receiver into a local for
1754-
// the same reason: `self.reader` and `decision_rx` cannot both be
1755-
// borrowed inside `select!` via `self`.
1756-
let mut decision_rx = self.permission_decision_rx.take();
1759+
// Clone the per-task decision receiver handle into the read loop. The
1760+
// receiver itself stays owned by the client, so sequential prompts
1761+
// (notably initial_message followed by the real turn) share one route.
1762+
let decision_rx = self.permission_decision_rx.clone();
17571763

17581764
// Tracks the in-flight steer write: `(request_id, transport, ack_tx)`.
17591765
// While `Some`, the steer arm is gated off so we don't stack writes,
@@ -1918,8 +1924,8 @@ impl AcpClient {
19181924
// owner decisions are not starved by a continuously-ready stdout.
19191925
// Cancel-safe: `mpsc::Receiver::recv` does not lose messages on drop.
19201926
Some(decision) = async {
1921-
match decision_rx.as_mut() {
1922-
Some(rx) => rx.recv().await,
1927+
match decision_rx.as_ref() {
1928+
Some(rx) => rx.lock().await.recv().await,
19231929
None => None,
19241930
}
19251931
} => {
@@ -2691,15 +2697,21 @@ impl AcpClient {
26912697
}
26922698
PermissionPolicy::Ask => {
26932699
// Availability gate (spec §10): `ask` requires both an active observer
2694-
// and a known owner. Without either, downgrade to `reject` with a loud
2695-
// warning — never sideways to `allow`.
2700+
// and a known owner plus a live route for the owner's decision.
2701+
// Without all three, downgrade to `reject` with a loud warning —
2702+
// never sideways to `allow`.
26962703
let observer_active = self.observer.is_some();
2697-
if !observer_active || !self.owner_pubkey_known {
2704+
let decision_route_active = match self.permission_decision_rx.as_ref() {
2705+
Some(rx) => !rx.lock().await.is_closed(),
2706+
None => false,
2707+
};
2708+
if !observer_active || !self.owner_pubkey_known || !decision_route_active {
26982709
tracing::warn!(
26992710
target: "acp::permission",
2700-
"ask policy unavailable (observer={}, owner_known={}) — downgrading to reject for id={id}",
2711+
"ask policy unavailable (observer={}, owner_known={}, decision_route={}) — downgrading to reject for id={id}",
27012712
observer_active,
2702-
self.owner_pubkey_known
2713+
self.owner_pubkey_known,
2714+
decision_route_active
27032715
);
27042716
// Fall through to the Reject arm's logic.
27052717
self.pending_permission_id = Some(id.clone());
@@ -2710,7 +2722,9 @@ impl AcpClient {
27102722
msg,
27112723
&nonce,
27122724
false,
2713-
Some("policy=ask unavailable (no observer/owner); downgraded to reject"),
2725+
Some(
2726+
"policy=ask unavailable (no observer/owner/decision route); downgraded to reject",
2727+
),
27142728
);
27152729
let response = permission_denial_response(&id, &options)?;
27162730
self.finish_permission_sync(&id, &nonce, "rejected", response)
@@ -6221,6 +6235,9 @@ mod tests {
62216235
let config = ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap();
62226236
client.set_permission_config(config);
62236237
client.set_owner_pubkey_known(false); // explicitly unknown
6238+
client.set_observer(Some(crate::observer::ObserverHandle::in_process()), 0);
6239+
let (_tx, rx) = tokio::sync::mpsc::channel::<PermissionDecision>(1);
6240+
client.install_permission_decision_rx(rx);
62246241

62256242
let msg = perm_request(2, default_opts());
62266243
let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30);
@@ -6229,6 +6246,40 @@ mod tests {
62296246
assert!(client.pending_permissions.is_empty());
62306247
}
62316248

6249+
#[tokio::test]
6250+
async fn ask_without_live_decision_route_is_non_actionable_and_rejected() {
6251+
let mut client = spawn_inert_client().await;
6252+
client.set_permission_config(
6253+
ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap(),
6254+
);
6255+
client.set_owner_pubkey_known(true);
6256+
let observer = crate::observer::ObserverHandle::in_process();
6257+
client.set_observer(Some(observer.clone()), 0);
6258+
let (decision_tx, decision_rx) = tokio::sync::mpsc::channel::<PermissionDecision>(1);
6259+
client.install_permission_decision_rx(decision_rx);
6260+
drop(decision_tx);
6261+
6262+
let msg = perm_request(3, default_opts());
6263+
let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30);
6264+
client
6265+
.handle_permission_request(&msg, hard_deadline)
6266+
.await
6267+
.expect("missing route must fail closed without breaking the transport");
6268+
6269+
assert!(client.pending_permissions.is_empty());
6270+
let event = observer
6271+
.snapshot()
6272+
.into_iter()
6273+
.find(|event| event.kind == "acp_read" && event.authorization.is_some())
6274+
.expect("permission request must remain observable");
6275+
let authorization = event.authorization.expect("checked above");
6276+
assert!(!authorization.actionable);
6277+
assert!(authorization
6278+
.reason
6279+
.as_deref()
6280+
.is_some_and(|reason| reason.contains("decision route")));
6281+
}
6282+
62326283
// ── Production-path tests: real loop emits request, captures nonce ──────
62336284

62346285
/// Full end-to-end production path test for the `ask` decision flow:
@@ -6380,6 +6431,90 @@ mod tests {
63806431
);
63816432
}
63826433

6434+
#[tokio::test]
6435+
async fn ask_decision_route_survives_sequential_prompt_read_loops() {
6436+
let capture_file = std::env::temp_dir().join(format!(
6437+
"buzz-acp-sequential-{}.ndjson",
6438+
uuid::Uuid::new_v4()
6439+
));
6440+
let request_one = r#"{"jsonrpc":"2.0","id":41,"method":"session/request_permission","params":{"sessionId":"sess","options":[{"optionId":"opt-allow","kind":"allow_once","name":"Allow"},{"optionId":"opt-deny","kind":"reject_once","name":"Deny"}]}}"#;
6441+
let request_two = r#"{"jsonrpc":"2.0","id":42,"method":"session/request_permission","params":{"sessionId":"sess","options":[{"optionId":"opt-allow","kind":"allow_once","name":"Allow"},{"optionId":"opt-deny","kind":"reject_once","name":"Deny"}]}}"#;
6442+
let terminal_one = r#"{"jsonrpc":"2.0","id":1001,"result":{"stopReason":"end_turn"}}"#;
6443+
let terminal_two = r#"{"jsonrpc":"2.0","id":1002,"result":{"stopReason":"end_turn"}}"#;
6444+
let script = format!(
6445+
r#"printf '{request_one}\n'; read -r response; printf '%s\n' "$response" >> {capture}; printf '{terminal_one}\n'; printf '{request_two}\n'; read -r response; printf '%s\n' "$response" >> {capture}; printf '{terminal_two}\n'"#,
6446+
capture = capture_file.display(),
6447+
);
6448+
6449+
let mut client = spawn_script(&script).await;
6450+
client.set_permission_config(
6451+
ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap(),
6452+
);
6453+
client.set_owner_pubkey_known(true);
6454+
let observer = crate::observer::ObserverHandle::in_process();
6455+
let mut observer_rx = observer.subscribe();
6456+
client.set_observer(Some(observer), 0);
6457+
let (decision_tx, decision_rx) =
6458+
tokio::sync::mpsc::channel::<PermissionDecision>(PERMISSION_MAP_CAP);
6459+
client.install_permission_decision_rx(decision_rx);
6460+
6461+
let decision_task = tokio::spawn(async move {
6462+
let mut delivered = 0;
6463+
while delivered < 2 {
6464+
let event =
6465+
tokio::time::timeout(std::time::Duration::from_secs(5), observer_rx.recv())
6466+
.await
6467+
.expect("permission event timed out")
6468+
.expect("observer channel closed");
6469+
let Some(authorization) = event.authorization else {
6470+
continue;
6471+
};
6472+
if !authorization.actionable {
6473+
continue;
6474+
}
6475+
decision_tx
6476+
.send(PermissionDecision {
6477+
request_nonce: authorization.request_nonce,
6478+
option_id: "opt-allow".to_string(),
6479+
})
6480+
.await
6481+
.expect("decision route must remain open");
6482+
delivered += 1;
6483+
}
6484+
});
6485+
6486+
let idle_timeout = std::time::Duration::from_secs(5);
6487+
let max_duration = std::time::Duration::from_secs(15);
6488+
for expected_id in [1001, 1002] {
6489+
let result = client
6490+
.read_until_response_with_idle_timeout(
6491+
"sess",
6492+
expected_id,
6493+
idle_timeout,
6494+
tokio::time::Instant::now() + max_duration,
6495+
max_duration,
6496+
)
6497+
.await
6498+
.expect("both sequential prompts must complete");
6499+
assert_eq!(result["stopReason"], "end_turn");
6500+
}
6501+
decision_task.await.expect("decision task failed");
6502+
6503+
let capture = std::fs::read_to_string(&capture_file).expect("read wire capture");
6504+
let _ = std::fs::remove_file(&capture_file);
6505+
let responses: Vec<serde_json::Value> = capture
6506+
.lines()
6507+
.map(|line| serde_json::from_str(line).expect("valid response JSON"))
6508+
.collect();
6509+
assert_eq!(responses.len(), 2);
6510+
assert_eq!(responses[0]["id"], 41);
6511+
assert_eq!(responses[1]["id"], 42);
6512+
assert!(responses.iter().all(|response| {
6513+
response["result"]["outcome"]["outcome"] == "selected"
6514+
&& response["result"]["outcome"]["optionId"] == "opt-allow"
6515+
}));
6516+
}
6517+
63836518
/// Cancel test: asserts exactly one JSON-RPC response per pending id, no
63846519
/// replay on subsequent cancel. Proves behavior at the wire level by
63856520
/// capturing the raw NDJSON lines written to the agent's stdin.

crates/buzz-acp/src/lib.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3434,10 +3434,10 @@ fn dispatch_pending(
34343434
let steer_tx = Some(tx);
34353435

34363436
// Permission decision channel: delivers `permission_decision` control
3437-
// frames into the read loop's decision arm (spec §4). Installed
3438-
// per-session (the receiver is taken by the read loop and dropped
3439-
// when the turn ends; the next turn installs a fresh pair). Capacity
3440-
// matches PERMISSION_MAP_CAP so each pending entry gets a slot.
3437+
// frames into the read loop's decision arm (spec §4). Sequential read
3438+
// loops within this task share the receiver; the next dispatched task
3439+
// replaces it with a fresh pair. Capacity matches PERMISSION_MAP_CAP so
3440+
// each pending entry gets a slot.
34413441
let (perm_tx, perm_rx) = tokio::sync::mpsc::channel::<crate::acp::PermissionDecision>(
34423442
crate::acp::PERMISSION_MAP_CAP,
34433443
);

0 commit comments

Comments
 (0)