@sigitcodereview / sigit / commits / da45d00

fix(acp): ask permission for the tool call that was announced

A tool call that needed approval reached the client as two unrelated calls. handle_prompt announced it under the model's id as in_progress before the permission check ran, and request_tool_permission then sent session/request_permission under a fresh perm-<uuid> id with status pending. A client drew an approval card beside a call that already claimed to be running. The permission decision is now taken before the announcement, since it sets the status the call is announced with. A call that will ask is announced as pending, the permission request names it by its own id, and an in_progress update follows the approval. A call allowed by policy still goes straight to in_progress. The permission request sets the title to the tool name and its full arguments so the user can see what they approve. Now that it is the same call, that would replace the card's title for good, so the in_progress update puts the original one back. Fixes #138 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Seto Elkahfi committed Oct 5, 2026 at 10:28 UTC da45d004bd1b9381ef59642aad83885f20cde580

4 files changed +176 -36

.agents/AGENTS.md

+5 -1

@@ -248,7 +248,11 @@ feeds results back. Neither the loop nor ACP/TUI surfaces depend on a concrete b

248 overrides and the default mode from `[permissions]` in `settings.toml` (`allow`/`ask`/`deny`,

249 default `ask`; `SIGIT_PERMISSIONS` env overrides the default). On `ask`, the ACP path sends

250 `session/request_permission` (allow once / allow for session / deny) and the TUI pauses the

251 - inference task on a y/a/n prompt. Note: ACP turn-affecting handlers run in `cx.spawn`ed tasks

251 + inference task on a y/a/n prompt. On the ACP path the decision is taken *before* the call is

252 + announced, because it sets the announced status: a call that will ask starts `pending`, the

253 + permission request carries that call's own id, and an `in_progress` update follows approval.

254 + That update also puts the card's title back, since the permission request overwrites it

255 + with the full arguments. Note: ACP turn-affecting handlers run in `cx.spawn`ed tasks

256 serialized by `SiGitAgent::turn_lock` so the dispatch loop can route the client's permission

257 answer mid-turn — don't move them back inline, and don't await client requests from inline

258 handlers (deadlock).

CHANGELOG.md

@@ -17,6 +17,13 @@

17 (`content_filter`) with `refusal`. A refused turn is also taken out of the

18 conversation, which is what the protocol tells the client to expect (#140).

20 +- **A tool call that needs approval is one card, not two.** The permission

21 + request carried an id of its own, so an ACP client showed an approval card

22 + beside a call that already claimed to be running. The call is now announced

23 + as `pending`, the permission request names that same call, and it moves to

24 + `in_progress` once approved. Calls allowed by policy still start out

25 + `in_progress` (#138).

26 +

27 - **A headless run no longer ends silently with no final message.** When the

28 last round came back empty, which is what happens when a run is cut off at

29 the tool-round cap mid-task, stdout was empty and the exit status was 0, so

src/main.rs

+71 -35

@@ -2494,6 +2494,39 @@ impl SiGitAgent {

2494 tc.arguments.chars().take(120).collect::<String>()

2495 );

2496

2497 + let signature = format!("{}\n{}", tc.name, tc.arguments);

2498 + let repeat_count = repeated_tool_calls

2499 + .entry(signature)

2500 + .and_modify(|count| *count += 1)

2501 + .or_insert(1);

2502 + // Repeated status checks are expected for a live background

2503 + // command. command_output long-polls and the overall round cap

2504 + // still bounds a confused model without cutting off a real

2505 + // build or release while it is in progress.

2506 + let repeated = *repeat_count >= 3 && tc.name != "command_output";

2507 + if repeated {

2508 + force_text = true;

2509 + stopped_repeating = Some(tc.name.clone());

2510 + log::warn!(

2511 + "prompt({}) stopping repeated tool call `{}` after {} attempts",

2512 + session_id,

2513 + tc.name,

2514 + repeat_count

2515 + );

2516 + }

2517 +

2518 + // Decided before the call is announced, because the answer sets

2519 + // the status it is announced with: a call that has to wait for

2520 + // the user starts out `pending`, everything else is already

2521 + // running. A repeated call is never run, so never asked about.

2522 + let decision = (!repeated).then(|| {

2523 + permissions::decision_for(&session_id.to_string(), &tc.name, &tc.arguments)

2524 + });

2525 + let announced_status = match decision {

2526 + Some(permissions::Decision::Ask) => ToolCallStatus::Pending,

2527 + _ => ToolCallStatus::InProgress,

2528 + };

2529 +

2530 // Only treat the call as a plan once its arguments have actually

2531 // produced one. Keying off the tool name alone means a

2532 // `write_todos` the converter rejects sends no plan *and* skips

@@ -2525,7 +2558,7 @@ impl SiGitAgent {

2558 SessionUpdate::ToolCall(

2559 ToolCall::new(tc.id.clone(), chat::tool_title(&tc.name, &tc.arguments))

2560 .kind(tool_kind_for(&tc.name))

2528 - .status(ToolCallStatus::InProgress)

2561 + .status(announced_status)

2562 .content(vec![invocation_content])

2563 .locations(tool_call_locations(&tc.name, &tc.arguments))

2564 .raw_input(raw_input),

@@ -2534,41 +2567,15 @@ impl SiGitAgent {

2567 .ok();

2568 }

2569

2537 - let signature = format!("{}\n{}", tc.name, tc.arguments);

2538 - let repeat_count = repeated_tool_calls

2539 - .entry(signature)

2540 - .and_modify(|count| *count += 1)

2541 - .or_insert(1);

2542 - // Repeated status checks are expected for a live background

2543 - // command. command_output long-polls and the overall round cap

2544 - // still bounds a confused model without cutting off a real

2545 - // build or release while it is in progress.

2546 - let repeated = *repeat_count >= 3 && tc.name != "command_output";

2547 - if repeated {

2548 - force_text = true;

2549 - stopped_repeating = Some(tc.name.clone());

2550 - log::warn!(

2551 - "prompt({}) stopping repeated tool call `{}` after {} attempts",

2552 - session_id,

2553 - tc.name,

2554 - repeat_count

2555 - );

2556 - }

2557 -

2570 // Permission gate: read-only tools pass straight through; a

2571 // mutating tool consults policy and may ask the client.

2560 - let output = if repeated {

2561 - format!(

2572 + let output = match decision {

2573 + None => format!(

2574 "The tool `{}` was not executed again because the model repeated \

2575 the same call three times. Continue without this tool.",

2576 tc.name

2565 - )

2566 - } else {

2567 - match permissions::decision_for(

2568 - &session_id.to_string(),

2569 - &tc.name,

2570 - &tc.arguments,

2571 - ) {

2577 + ),

2578 + Some(decision) => match decision {

2579 permissions::Decision::Allow => {

2580 tools::execute_tool(&tc.name, &tc.arguments).await

2581 }

@@ -2578,10 +2585,36 @@ impl SiGitAgent {

2585 }

2586 permissions::Decision::Ask => {

2587 match self

2581 - .request_tool_permission(cx, &session_id, &tc.name, &tc.arguments)

2588 + .request_tool_permission(

2589 + cx,

2590 + &session_id,

2591 + &tc.id,

2592 + &tc.name,

2593 + &tc.arguments,

2594 + )

2595 .await

2596 {

2597 PermissionVerdict::Approved => {

2598 + // The call the user just approved leaves

2599 + // `pending` here. The title goes back to

2600 + // the card's own: the permission request

2601 + // replaced it with the full arguments.

2602 + if !render_as_plan {

2603 + self.send_tool_call_update(

2604 + cx,

2605 + session_id.clone(),

2606 + SessionUpdate::ToolCallUpdate(ToolCallUpdate::new(

2607 + tc.id.clone(),

2608 + ToolCallUpdateFields::new()

2609 + .status(ToolCallStatus::InProgress)

2610 + .title(chat::tool_title(

2611 + &tc.name,

2612 + &tc.arguments,

2613 + )),

2614 + )),

2615 + )

2616 + .ok();

2617 + }

2618 tools::execute_tool(&tc.name, &tc.arguments).await

2619 }

2620 PermissionVerdict::Denied(reason) => {

@@ -2632,7 +2665,7 @@ impl SiGitAgent {

2665 }

2666 }

2667 }

2635 - }

2668 + },

2669 };

2670

2671 log::info!(" ← {} chars", output.len());

@@ -2798,7 +2831,9 @@ impl SiGitAgent {

2831 Ok(PromptResponse::new(stop_reason))

2832 }

2833

2801 - /// Ask the ACP client for permission to run one tool call. Presents

2834 + /// Ask the ACP client for permission to run one tool call. The request

2835 + /// names the call by `tool_call_id`, the id it was announced under, so the

2836 + /// client updates that one card instead of drawing a second. Presents

2837 /// allow-once / allow-for-session / deny; an "always allow" choice is

2838 /// recorded via [`permissions::grant_for_session`]. Only safe to call from

2839 /// a spawned task (see the handler registration in `run_acp_server`): the

@@ -2807,6 +2842,7 @@ impl SiGitAgent {

2842 &self,

2843 cx: &ConnectionTo<Client>,

2844 session_id: &SessionId,

2845 + tool_call_id: &str,

2846 tool_name: &str,

2847 arguments: &str,

2848 ) -> PermissionVerdict {

@@ -2828,7 +2864,7 @@ impl SiGitAgent {

2864 let request = RequestPermissionRequest::new(

2865 session_id.clone(),

2866 ToolCallUpdate::new(

2831 - format!("perm-{}", uuid::Uuid::new_v4()),

2867 + tool_call_id.to_string(),

2868 ToolCallUpdateFields::new()

2869 .title(title)

2870 .kind(tool_kind_for(tool_name))

tests/acp_permissions.rs

+93

@@ -458,6 +458,99 @@ fn permission_round_trip_cancel_then_allow() {

458 let _ = std::fs::remove_dir_all(&scratch);

459 }

460

461 +/// A call that needs approval is one tool call from start to finish (issue

462 +/// #138). The permission request used to carry an id of its own, so a client

463 +/// drew an approval card next to a call that already claimed to be running.

464 +#[test]

465 +fn a_permission_request_is_about_the_tool_call_it_was_announced_as() {

466 + let endpoint = start_fake_endpoint(vec![

467 + sse_tool_call(

468 + "call_1",

469 + "run_command",

470 + r#"{"command":"echo sigit-one-card"}"#,

471 + ),

472 + sse_text("done"),

473 + ]);

474 +

475 + let scratch = std::env::temp_dir().join(format!("sigit_acp_perm_id_{}", std::process::id()));

476 + let config_dir = scratch.join("config");

477 + let cwd = scratch.join("cwd");

478 + std::fs::create_dir_all(&config_dir).unwrap();

479 + std::fs::create_dir_all(&cwd).unwrap();

480 +

481 + let mut agent = spawn_agent(endpoint.port, &config_dir);

482 +

483 + let id = agent.request(

484 + "initialize",

485 + json!({"protocolVersion": 1, "clientCapabilities": {}}),

486 + );

487 + agent.wait_for_response(id);

488 +

489 + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));

490 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]

491 + .as_str()

492 + .expect("session id")

493 + .to_string();

494 +

495 + let prompt_id = agent.request(

496 + "session/prompt",

497 + json!({

498 + "sessionId": session_id,

499 + "prompt": [{"type": "text", "text": "run the command"}],

500 + }),

501 + );

502 +

503 + // Messages arrive in order and `wait_for` drops what it skips, so each

504 + // step below also checks that the one before it came first.

505 + let call_update = |message: &Value, kind: &str| {

506 + let update = &message["params"]["update"];

507 + message["method"] == "session/update"

508 + && update["sessionUpdate"] == kind

509 + && update["toolCallId"] == "call_1"

510 + };

511 +

512 + let announced = agent.wait_for("the tool call announcement", |message| {

513 + call_update(message, "tool_call")

514 + });

515 + // `pending` is the schema's default status, so it is left off the wire.

516 + let status = &announced["params"]["update"]["status"];

517 + assert!(

518 + status.is_null() || status == "pending",

519 + "a call waiting for approval has not started: {announced}"

520 + );

521 +

522 + let permission = agent.wait_for_agent_request("session/request_permission");

523 + assert_eq!(

524 + permission["params"]["toolCall"]["toolCallId"], "call_1",

525 + "the request must name the announced call: {permission}"

526 + );

527 + agent.respond(

528 + permission["id"].clone(),

529 + json!({"outcome": {"outcome": "selected", "optionId": "allow_once"}}),

530 + );

531 +

532 + let started = agent.wait_for("the approved call starting", |message| {

533 + call_update(message, "tool_call_update")

534 + });

535 + let started = &started["params"]["update"];

536 + assert_eq!(started["status"], "in_progress", "{started}");

537 + assert_eq!(

538 + started["title"], announced["params"]["update"]["title"],

539 + "the card gets its own title back after the approval dialog: {started}"

540 + );

541 +

542 + let finished = agent.wait_for("the call finishing", |message| {

543 + call_update(message, "tool_call_update")

544 + });

545 + assert_eq!(finished["params"]["update"]["status"], "completed");

546 +

547 + let response = agent.wait_for_response(prompt_id);

548 + assert_eq!(response["result"]["stopReason"], "end_turn");

549 +

550 + drop(agent);

551 + let _ = std::fs::remove_dir_all(&scratch);

552 +}

553 +

554 #[test]

555 fn successful_config_option_changes_are_rendered_as_system_status_cards() {

556 let endpoint = start_fake_endpoint(vec![]);