Run synchronous tools off the ACP connection task
A long-running command showed no spinner in Zed. The ACP connection runs the incoming reader, the outgoing writer, and spawned turn tasks as actors joined into one future, so a tool that blocks its thread stalls all of them. Every synchronous tool did exactly that — run_command polls its child in a std::thread::sleep loop for up to COMMAND_TIMEOUT — so the tool call's in_progress notification sat in the outgoing queue until the command was already finished, and the client got in_progress and completed back to back with nothing to draw in between. The synchronous tools move to the blocking pool behind one spawn_blocking, in execute_sync_tool. read_website was already wrapped for the same underlying reason (reqwest::blocking panics on a runtime thread) and now shares the one wrapper. The turn still awaits each tool in order, so nothing about the loop's sequencing changes. Measured by a new end-to-end test: a scripted `sleep 2` with the permission gate off, asserting the client sees in_progress at least a second before completed. Against the old code the two arrive 164µs apart. Fixes #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seto Elkahfi committed
Sep 2, 2026 at 22:22 UTC
64a4b4e76330d00862ae33eca8f65b523038935f
2 files changed
+129
-19
src/tools.rs
+28
-12
@@ -454,17 +454,38 @@ pub async fn execute_tool(name: &str, arguments: &str) -> String {
454
}
455
456
async fn execute_tool_impl(name: &str, arguments: &str) -> String {
457
+ match name {
458
+ TASK_TOOL_NAME => exec_task(arguments).await,
459
+ WEB_SEARCH_TOOL_NAME => exec_web_search(arguments).await,
460
+ // Tools discovered from MCP servers are namespaced `mcp__<server>__<tool>`
461
+ // and forwarded to the owning server.
462
+ _ if crate::mcp::is_mcp_tool(name) => crate::mcp::call_tool(name, arguments).await,
463
+ // Everything else is synchronous and may run for a long time: a shell
464
+ // command up to COMMAND_TIMEOUT, a glob over a big tree, an HTTP fetch
465
+ // on reqwest::blocking (which panics if polled on a runtime thread).
466
+ // The ACP connection multiplexes reading the client, writing
467
+ // notifications, and running the turn onto a single task, so blocking
468
+ // here freezes the editor's view of the session: the tool call's
469
+ // `in_progress` update — the editor's spinner — only reaches the client
470
+ // once the tool has already finished.
471
+ _ => {
472
+ let tool = name.to_owned();
473
+ let arguments = arguments.to_owned();
474
+ let running = tool.clone();
475
+ tokio::task::spawn_blocking(move || execute_sync_tool(&running, &arguments))
476
+ .await
477
+ .unwrap_or_else(|err| format!("Error: {tool} task failed: {err}"))
478
+ }
479
+ }
480
+}
481
+
482
+/// The synchronous tools, run on the blocking pool by [`execute_tool_impl`].
483
+fn execute_sync_tool(name: &str, arguments: &str) -> String {
484
match name {
485
"read_file" => exec_read_file(arguments),
486
"list_directory" => exec_list_directory(arguments),
487
"search_files" => exec_search_files(arguments),
461
- "read_website" => {
462
- // reqwest::blocking panics inside a tokio runtime, so run on the blocking pool.
463
- let args = arguments.to_owned();
464
- tokio::task::spawn_blocking(move || exec_read_website(&args))
465
- .await
466
- .unwrap_or_else(|err| format!("Error: read_website task failed: {err}"))
467
- }
488
+ "read_website" => exec_read_website(arguments),
489
"create_directory" => exec_create_directory(arguments),
490
"create_file" => exec_create_file(arguments),
491
"edit_file" => exec_edit_file(arguments),
@@ -477,11 +498,6 @@ async fn execute_tool_impl(name: &str, arguments: &str) -> String {
498
"command_output" => exec_command_output(arguments),
499
"kill_command" => exec_kill_command(arguments),
500
"skill" => crate::skills::activate_skill(arguments),
480
- TASK_TOOL_NAME => exec_task(arguments).await,
481
- WEB_SEARCH_TOOL_NAME => exec_web_search(arguments).await,
482
- // Tools discovered from MCP servers are namespaced `mcp__<server>__<tool>`
483
- // and forwarded to the owning server.
484
- _ if crate::mcp::is_mcp_tool(name) => crate::mcp::call_tool(name, arguments).await,
501
_ => format!("Unknown tool: {name}"),
502
}
503
}
tests/acp_permissions.rs
+101
-7
@@ -13,6 +13,10 @@
13
//! would reject the whole session.
14
//! 2. `selected: allow_once` — the tool must actually execute and its output
15
//! travel back to the endpoint as a tool result.
16
+//!
17
+//! A second test covers live progress: a slow command's `in_progress` tool call
18
+//! must reach the client while the command is still running, which is what the
19
+//! editor's spinner is driven from.
20
21
use std::collections::VecDeque;
22
use std::io::{BufRead, BufReader, Read, Write};
@@ -124,21 +128,34 @@ struct AgentUnderTest {
128
}
129
130
fn spawn_agent(port: u16, config_dir: &std::path::Path) -> AgentUnderTest {
127
- let mut child = Command::new(env!("CARGO_BIN_EXE_sigit"))
131
+ spawn_agent_with_permissions(port, config_dir, None)
132
+}
133
+
134
+/// `permissions` sets `SIGIT_PERMISSIONS`; `None` leaves the default `ask` mode
135
+/// in place, which is what the permission round-trip needs.
136
+fn spawn_agent_with_permissions(
137
+ port: u16,
138
+ config_dir: &std::path::Path,
139
+ permissions: Option<&str>,
140
+) -> AgentUnderTest {
141
+ let mut command = Command::new(env!("CARGO_BIN_EXE_sigit"));
142
+ command
143
.env("OPENAI_BASE_URL", format!("http://127.0.0.1:{port}"))
144
.env("OPENAI_API_KEY", "test-key")
145
.env("SIGIT_MODEL", "scripted-model")
146
.env("SIGIT_CONFIG_DIR", config_dir)
147
.env("SIGIT_MCP", "off")
133
- // A fresh config dir means the default permission mode, `ask` — make
134
- // sure the environment can't turn the gate off underneath the test.
135
- .env_remove("SIGIT_PERMISSIONS")
148
.env_remove("SIGIT_LOCAL_INFERENCE")
149
.stdin(Stdio::piped())
150
.stdout(Stdio::piped())
139
- .stderr(Stdio::null())
140
- .spawn()
141
- .expect("spawn sigit in ACP mode");
151
+ .stderr(Stdio::null());
152
+ match permissions {
153
+ // A fresh config dir means the default permission mode, `ask` — make
154
+ // sure the environment can't turn the gate off underneath the test.
155
+ None => command.env_remove("SIGIT_PERMISSIONS"),
156
+ Some(mode) => command.env("SIGIT_PERMISSIONS", mode),
157
+ };
158
+ let mut child = command.spawn().expect("spawn sigit in ACP mode");
159
160
let stdout = child.stdout.take().unwrap();
161
let (message_tx, incoming) = channel();
@@ -208,6 +225,14 @@ impl AgentUnderTest {
225
response
226
}
227
228
+ /// The first `session/update` whose payload satisfies `matches`.
229
+ fn wait_for_update(&mut self, what: &str, matches: impl Fn(&Value) -> bool) -> Value {
230
+ let message = self.wait_for(what, |message| {
231
+ message["method"] == "session/update" && matches(&message["params"]["update"])
232
+ });
233
+ message["params"]["update"].clone()
234
+ }
235
+
236
/// A request *from* the agent (has a `method` and its own id).
237
fn wait_for_agent_request(&mut self, method: &str) -> Value {
238
self.wait_for(&format!("agent request {method}"), |message| {
@@ -357,3 +382,72 @@ fn permission_round_trip_cancel_then_allow() {
382
drop(agent);
383
let _ = std::fs::remove_dir_all(&scratch);
384
}
385
+
386
+#[test]
387
+fn a_slow_commands_progress_reaches_the_client_while_it_runs() {
388
+ // `SIGIT_PERMISSIONS=allow` skips the gate, so the only thing between the
389
+ // two updates below is the command itself.
390
+ let endpoint = start_fake_endpoint(vec![
391
+ sse_tool_call("call_1", "run_command", r#"{"command":"sleep 2"}"#),
392
+ sse_text("done"),
393
+ ]);
394
+
395
+ let scratch = std::env::temp_dir().join(format!("sigit_acp_progress_{}", std::process::id()));
396
+ let config_dir = scratch.join("config");
397
+ let cwd = scratch.join("cwd");
398
+ std::fs::create_dir_all(&config_dir).unwrap();
399
+ std::fs::create_dir_all(&cwd).unwrap();
400
+
401
+ let mut agent = spawn_agent_with_permissions(endpoint.port, &config_dir, Some("allow"));
402
+
403
+ let id = agent.request(
404
+ "initialize",
405
+ json!({"protocolVersion": 1, "clientCapabilities": {}}),
406
+ );
407
+ agent.wait_for_response(id);
408
+
409
+ let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));
410
+ let session_id = agent.wait_for_response(id)["result"]["sessionId"]
411
+ .as_str()
412
+ .expect("session id")
413
+ .to_string();
414
+
415
+ let prompt_id = agent.request(
416
+ "session/prompt",
417
+ json!({
418
+ "sessionId": session_id,
419
+ "prompt": [{"type": "text", "text": "run the slow command"}],
420
+ }),
421
+ );
422
+
423
+ let started = agent.wait_for_update("the tool call starting", |update| {
424
+ update["sessionUpdate"] == "tool_call" && update["toolCallId"] == "call_1"
425
+ });
426
+ let in_progress_at = Instant::now();
427
+ assert_eq!(
428
+ started["status"], "in_progress",
429
+ "a running tool call must be announced as in_progress: {started}"
430
+ );
431
+
432
+ agent.wait_for_update("the tool call finishing", |update| {
433
+ update["sessionUpdate"] == "tool_call_update"
434
+ && update["toolCallId"] == "call_1"
435
+ && update["status"] == "completed"
436
+ });
437
+
438
+ // `sleep 2` runs between the two updates. If the turn blocked the
439
+ // connection's actors, both would only reach the client once the command
440
+ // had already finished, and the editor would never draw a spinner.
441
+ assert!(
442
+ in_progress_at.elapsed() >= Duration::from_millis(1_000),
443
+ "in_progress arrived only {:?} before completed — the client had no \
444
+ window to show progress in",
445
+ in_progress_at.elapsed()
446
+ );
447
+
448
+ let response = agent.wait_for_response(prompt_id);
449
+ assert_eq!(response["result"]["stopReason"], "end_turn");
450
+
451
+ drop(agent);
452
+ let _ = std::fs::remove_dir_all(&scratch);
453
+}