Skip to content

Commit 354ec48

Browse files
yogthosYogthos
andauthored
fix(F1): route ACP permission asks to auto-deny instead of dropping (#76)
Track F-CRITICAL #1 from ROADMAP.md. ## Problem `extras/acp/mod.rs::build_acp_permission` constructed `(ask_tx, _ask_rx)` and immediately dropped `ask_rx`. When a tool needed `Ask` confirmation in ACP mode (Zed / editor client), the permission check called `ask_tx.send()`, awaited the oneshot reply, and waited 30 seconds before timing out. The user saw a generic tool failure with no context. Tools requiring permission were effectively unrunnable from ACP clients unless the user configured `--yolo` or explicit allow rules in `permission` config. ## Fix New `spawn_acp_ask_drain` task takes ownership of the receiver and responds to every `AskRequest` with `UserDecision::Deny` immediately. Fail-fast with a clear deny beats a 30s timeout: - The LLM sees the denial in the tool result and can re-plan. - The error message is "Permission denied by user", not a generic timeout. - Explicit allow rules (set via config or `/allow add` in interactive mode beforehand) still work — they short-circuit before the ask channel. Routing the ask through the ACP protocol as a real `requestPermission` notification (so the editor surfaces a dialog) is a larger Phase C5-ish feature; F1 is the minimum-viable non-hang fix. ## Tests Two new tests in `extras::acp::tests`: - `acp_ask_drain_responds_with_deny`: send one `AskRequest`, assert the reply arrives within 200ms with `Deny`. - `acp_ask_drain_handles_multiple_concurrent_asks`: send 5 asks in a loop, assert all 5 replies arrive promptly with Deny — guards against a single-shot drain bug. ## Test plan - [x] `cargo test --features acp` — 2 new tests pass. - [x] `cargo test --features plugin` — 649 pass (full suite). - [x] `cargo build --all-features` — compiles, no warnings. Co-authored-by: Yogthos <yogthos@gmail.com>
1 parent 3defb69 commit 354ec48

1 file changed

Lines changed: 97 additions & 2 deletions

File tree

src/extras/acp/mod.rs

Lines changed: 97 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,11 +284,106 @@ fn build_acp_permission(state: &AcpState) -> (Option<PermCheck>, Option<AskSende
284284
let checker = PermissionChecker::new(&perm_config, mode, None);
285285
let perm: PermCheck = Arc::new(Mutex::new(checker));
286286

287-
let (ask_tx, _ask_rx) = tokio::sync::mpsc::channel(64);
288-
287+
let (ask_tx, ask_rx) = tokio::sync::mpsc::channel(64);
288+
spawn_acp_ask_drain(ask_rx);
289289
(Some(perm), Some(ask_tx))
290290
}
291291

292+
/// Drain `ask_rx` by responding to every permission ask with
293+
/// `Deny`. ACP runs are non-interactive — there's no human at a
294+
/// keyboard to confirm prompts. Previously the receiver was simply
295+
/// dropped (`_ask_rx`), so any tool needing `Ask` confirmation
296+
/// hit the 30s permission timeout and surfaced as a generic
297+
/// failure to the editor client. Fail-fast with a clear deny is
298+
/// strictly better: the LLM sees the denial immediately and can
299+
/// re-plan, or the user can configure explicit allow rules.
300+
///
301+
/// **Future work**: route the ask through the ACP protocol as a
302+
/// `requestPermission` notification so the editor client can
303+
/// surface a real dialog. Out of scope for the F1 fix; that's a
304+
/// Phase C5-ish feature requiring ACP protocol wiring.
305+
fn spawn_acp_ask_drain(
306+
mut ask_rx: tokio::sync::mpsc::Receiver<crate::permission::ask::AskRequest>,
307+
) {
308+
tokio::spawn(async move {
309+
while let Some(req) = ask_rx.recv().await {
310+
// The tool's caller is awaiting on `req.reply`. Dropping
311+
// it without sending would also surface as a tool error
312+
// ("Permission system unavailable"), but Deny is a
313+
// clearer signal that the call was *refused* rather
314+
// than the system being broken.
315+
let _ = req.reply.send(crate::permission::ask::UserDecision::Deny);
316+
}
317+
});
318+
}
319+
320+
#[cfg(test)]
321+
mod tests {
322+
use super::*;
323+
324+
/// Regression for F1: any `AskRequest` sent through the ACP
325+
/// ask channel must be promptly responded to with `Deny`,
326+
/// rather than hanging. Without `spawn_acp_ask_drain`, the
327+
/// `reply` oneshot is dropped on receiver drop → tool sees
328+
/// `Permission system unavailable` (technically OK, but slower
329+
/// and worse signal). With the drain, the tool sees `Deny`
330+
/// within a tick.
331+
#[tokio::test]
332+
async fn acp_ask_drain_responds_with_deny() {
333+
let (ask_tx, ask_rx) = tokio::sync::mpsc::channel::<crate::permission::ask::AskRequest>(8);
334+
spawn_acp_ask_drain(ask_rx);
335+
336+
let (reply_tx, reply_rx) = tokio::sync::oneshot::channel();
337+
ask_tx
338+
.send(crate::permission::ask::AskRequest {
339+
tool: "bash".to_string(),
340+
input: "rm -rf /".to_string(),
341+
reply: reply_tx,
342+
})
343+
.await
344+
.expect("send must succeed");
345+
346+
let resp = tokio::time::timeout(std::time::Duration::from_millis(200), reply_rx)
347+
.await
348+
.expect("must reply within 200ms — F1 regression")
349+
.expect("reply channel must not be dropped");
350+
assert!(
351+
matches!(resp, crate::permission::ask::UserDecision::Deny),
352+
"ACP ask must auto-deny; got {:?}",
353+
resp,
354+
);
355+
}
356+
357+
/// Multiple concurrent asks all get responded to.
358+
#[tokio::test]
359+
async fn acp_ask_drain_handles_multiple_concurrent_asks() {
360+
let (ask_tx, ask_rx) = tokio::sync::mpsc::channel::<crate::permission::ask::AskRequest>(8);
361+
spawn_acp_ask_drain(ask_rx);
362+
363+
let mut replies = Vec::new();
364+
for i in 0..5 {
365+
let (reply_tx, reply_rx) = tokio::sync::oneshot::channel();
366+
ask_tx
367+
.send(crate::permission::ask::AskRequest {
368+
tool: format!("bash-{i}"),
369+
input: format!("cmd-{i}"),
370+
reply: reply_tx,
371+
})
372+
.await
373+
.unwrap();
374+
replies.push(reply_rx);
375+
}
376+
377+
for reply_rx in replies {
378+
let resp = tokio::time::timeout(std::time::Duration::from_millis(500), reply_rx)
379+
.await
380+
.expect("each reply must arrive promptly")
381+
.expect("reply channel dropped");
382+
assert!(matches!(resp, crate::permission::ask::UserDecision::Deny));
383+
}
384+
}
385+
}
386+
292387
fn resolve_acp_mode(cli: &Cli, cfg: &Config) -> SecurityMode {
293388
if cli.yolo || cfg.yolo.unwrap_or(false) {
294389
SecurityMode::Yolo

0 commit comments

Comments
 (0)