@sigitcodereview / sigit / commits / 9b205b4

Answer the review on the blocking-pool switch

Three points from the automated review: The JoinHandle unwrap_or_else was flagged as a panic risk. It is the opposite: unwrap_or_else on a Result cannot panic, and turning a JoinError into a tool result is what keeps a panicking tool from unwinding through the turn — which is what would have happened when these tools ran inline. Said so in a comment rather than changing the code. execute_sync_tool now documents that it runs on a blocking thread with no reactor, so a future tool that needs to await belongs with task and web_search in execute_tool_impl. The timing assertion gets padding: four seconds of command against a one-second floor. A loaded runner only widens the gap being measured, so the one real flake vector is this thread being descheduled between reading the two updates, and three seconds of slack covers that without weakening what the assertion separates — four seconds from the 164µs the blocking version produced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Seto Elkahfi committed Sep 3, 2026 at 07:30 UTC 9b205b48c0066e17cb65745c615256607fc64752

2 files changed +23 -3

src/tools.rs

+12

@@ -474,12 +474,24 @@ async fn execute_tool_impl(name: &str, arguments: &str) -> String {

474 let running = tool.clone();

475 tokio::task::spawn_blocking(move || execute_sync_tool(&running, &arguments))

476 .await

477 + // A `JoinError` means the tool panicked (or the runtime is

478 + // shutting down). Report it as the tool's result rather than

479 + // propagating: a panicking tool should cost the model one bad

480 + // tool result, not the whole turn. Note this is strictly safer

481 + // than running the tool inline, where the panic would unwind

482 + // through the turn and take the ACP connection with it.

483 .unwrap_or_else(|err| format!("Error: {tool} task failed: {err}"))

484 }

485 }

486 }

487

488 /// The synchronous tools, run on the blocking pool by [`execute_tool_impl`].

489 +///

490 +/// Everything here runs on a blocking thread with no reactor, so nothing in

491 +/// this match may await or drive a tokio resource. A new tool that needs the

492 +/// async context belongs in [`execute_tool_impl`] alongside `task` and

493 +/// `web_search` instead — and then owes its own answer to the question this

494 +/// dispatch exists for: not blocking the caller's task for its whole run.

495 fn execute_sync_tool(name: &str, arguments: &str) -> String {

496 match name {

497 "read_file" => exec_read_file(arguments),

tests/acp_permissions.rs

+11 -3

@@ -399,15 +399,23 @@ fn a_slow_commands_progress_reaches_the_client_while_it_runs() {

399 std::fs::create_dir_all(&config_dir).unwrap();

400 std::fs::create_dir_all(&cwd).unwrap();

401

402 - // The command has to take a couple of seconds through `sh -c` / `cmd /C`.

402 + // The command has to run for a few seconds through `sh -c` / `cmd /C`.

403 // `sleep` is neither a cmd builtin nor on a Windows runner's PATH (Git's

404 // usr\bin, which carries the MSYS coreutils, is deliberately left off it),

405 // and the Windows job hung on it rather than failing fast. `ping` ships in

406 // System32, waits a second between echoes, and never reads stdin.

407 + //

408 + // Four seconds against the one-second assertion below is deliberate

409 + // padding. A busy runner only ever widens the gap being measured — it

410 + // cannot make the command finish sooner — so the one way a correct agent

411 + // could fail here is this thread being descheduled for seconds between

412 + // reading the two updates off the channel. Three seconds of slack covers

413 + // that without weakening the assertion, which is separating four seconds

414 + // from the 164µs the blocking version produced.

415 #[cfg(unix)]

408 - let command = "sleep 2";

416 + let command = "sleep 4";

417 #[cfg(windows)]

410 - let command = "ping -n 3 127.0.0.1";

418 + let command = "ping -n 5 127.0.0.1";

419

420 // `SIGIT_PERMISSIONS=allow` skips the gate, so the only thing between the

421 // two updates below is the command itself.