@sigitcodereview / sigit / commits / f73e53c

fix(acp): end denied and skipped tool calls as failed

A call denied by policy or plan mode, denied by the user at the permission prompt, or skipped by the repeat guard never ran, but its card closed with the same `completed` update as a successful call. It now closes `failed`, matching the cancelled-at-the-prompt path. A call denied at the prompt also gets its own title back, since the permission request replaced it with the full arguments. Tool errors returned in band by execute_tool still end `completed`; that needs execute_tool to report success and is left for later. Fixes #139 Co-Authored-By: siGit Code v1.6.4 <noreply@sigit.si> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Seto Elkahfi committed Oct 8, 2026 at 06:51 UTC f73e53c1724fd011ae1fd503c4ca2aa6284e2a7e
3 files changed +191 -18
CHANGELOG.md
+8
@@ -28,6 +28,14 @@
28 for ids no session ever had. The entry now goes with the last request that
29 used it.
30
31 +- **A tool call that did not run no longer shows as completed.** When policy
32 + or plan mode denied a call, when the user picked Deny at the permission
33 + prompt, or when the repeat guard skipped a call the model made three times,
34 + the editor's card still ended `completed`. Those cards now end `failed`, the
35 + same way a call cancelled at the prompt already did. A call denied at the
36 + prompt also gets its own title back instead of keeping the permission
37 + request's (#139).
38 +
39 ## 1.6.4
40
41 ### Added
src/main.rs
+29 -17
@@ -3202,20 +3202,25 @@ impl SiGitAgent {
3202 }
3203
3204 // Permission gate: read-only tools pass straight through; a
3205 - // mutating tool consults policy and may ask the client.
3206 - let output = match decision {
3207 - None => format!(
3208 - "The tool `{}` was not executed again because the model repeated \
3209 - the same call three times. Continue without this tool.",
3210 - tc.name
3205 + // mutating tool consults policy and may ask the client. The
3206 + // flag says whether the tool actually ran: a call that was
3207 + // denied or skipped ends `failed`, not `completed` (#139).
3208 + let (output, executed) = match decision {
3209 + None => (
3210 + format!(
3211 + "The tool `{}` was not executed again because the model repeated \
3212 + the same call three times. Continue without this tool.",
3213 + tc.name
3214 + ),
3215 + false,
3216 ),
3217 Some(decision) => match decision {
3218 permissions::Decision::Allow => {
3214 - tools::execute_tool(&tc.name, &tc.arguments).await
3219 + (tools::execute_tool(&tc.name, &tc.arguments).await, true)
3220 }
3221 permissions::Decision::Deny(reason) => {
3222 log::info!(" ✗ {} denied by policy", tc.name);
3218 - reason
3223 + (reason, false)
3224 }
3225 permissions::Decision::Ask => {
3226 // The answer can take as long as the user likes.
@@ -3266,11 +3271,11 @@ impl SiGitAgent {
3271 )
3272 .ok();
3273 }
3269 - tools::execute_tool(&tc.name, &tc.arguments).await
3274 + (tools::execute_tool(&tc.name, &tc.arguments).await, true)
3275 }
3276 PermissionVerdict::Denied(reason) => {
3277 log::info!(" ✗ {} denied by user", tc.name);
3273 - reason
3278 + (reason, false)
3279 }
3280 PermissionVerdict::TurnCancelled => {
3281 log::info!(
@@ -3322,16 +3327,23 @@ impl SiGitAgent {
3327 log::info!(" ← {} chars", output.len());
3328
3329 if !render_as_plan {
3330 + let mut fields = ToolCallUpdateFields::new()
3331 + .status(if executed {
3332 + ToolCallStatus::Completed
3333 + } else {
3334 + ToolCallStatus::Failed
3335 + })
3336 + .content(vec![tool_output_content(&output)])
3337 + .raw_output(serde_json::Value::String(output.clone()));
3338 + // A denial at the prompt leaves the card titled with the
3339 + // permission request's full arguments; put its own back.
3340 + if !executed && announced_status == ToolCallStatus::Pending {
3341 + fields = fields.title(chat::tool_title(&tc.name, &tc.arguments));
3342 + }
3343 self.send_tool_call_update(
3344 cx,
3345 session_id.clone(),
3328 - SessionUpdate::ToolCallUpdate(ToolCallUpdate::new(
3329 - tc.id.clone(),
3330 - ToolCallUpdateFields::new()
3331 - .status(ToolCallStatus::Completed)
3332 - .content(vec![tool_output_content(&output)])
3333 - .raw_output(serde_json::Value::String(output.clone())),
3334 - )),
3346 + SessionUpdate::ToolCallUpdate(ToolCallUpdate::new(tc.id.clone(), fields)),
3347 )
3348 .ok();
3349 }
tests/acp_permissions.rs
+154 -1
@@ -551,6 +551,82 @@ fn a_permission_request_is_about_the_tool_call_it_was_announced_as() {
551 let _ = std::fs::remove_dir_all(&scratch);
552 }
553
554 +/// A call the user denies at the prompt did not run, so it ends `failed`
555 +/// rather than `completed` (issue #139), and gets its own title back from
556 +/// the permission request's.
557 +#[test]
558 +fn a_call_denied_at_the_prompt_ends_failed() {
559 + let endpoint = start_fake_endpoint(vec![
560 + sse_tool_call(
561 + "call_1",
562 + "run_command",
563 + r#"{"command":"echo sigit-denied"}"#,
564 + ),
565 + sse_text("ok, skipped"),
566 + ]);
567 +
568 + let scratch = std::env::temp_dir().join(format!("sigit_acp_perm_deny_{}", std::process::id()));
569 + let config_dir = scratch.join("config");
570 + let cwd = scratch.join("cwd");
571 + std::fs::create_dir_all(&config_dir).unwrap();
572 + std::fs::create_dir_all(&cwd).unwrap();
573 +
574 + let mut agent = spawn_agent(endpoint.port, &config_dir);
575 +
576 + let id = agent.request(
577 + "initialize",
578 + json!({"protocolVersion": 1, "clientCapabilities": {}}),
579 + );
580 + agent.wait_for_response(id);
581 +
582 + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));
583 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]
584 + .as_str()
585 + .expect("session id")
586 + .to_string();
587 +
588 + let prompt_id = agent.request(
589 + "session/prompt",
590 + json!({
591 + "sessionId": session_id,
592 + "prompt": [{"type": "text", "text": "run the command"}],
593 + }),
594 + );
595 +
596 + let call_update = |message: &Value, kind: &str| {
597 + let update = &message["params"]["update"];
598 + message["method"] == "session/update"
599 + && update["sessionUpdate"] == kind
600 + && update["toolCallId"] == "call_1"
601 + };
602 +
603 + let announced = agent.wait_for("the tool call announcement", |message| {
604 + call_update(message, "tool_call")
605 + });
606 +
607 + let permission = agent.wait_for_agent_request("session/request_permission");
608 + agent.respond(
609 + permission["id"].clone(),
610 + json!({"outcome": {"outcome": "selected", "optionId": "reject_once"}}),
611 + );
612 +
613 + let finished = agent.wait_for("the denied call closing", |message| {
614 + call_update(message, "tool_call_update")
615 + });
616 + let finished = &finished["params"]["update"];
617 + assert_eq!(finished["status"], "failed", "{finished}");
618 + assert_eq!(
619 + finished["title"], announced["params"]["update"]["title"],
620 + "the card gets its own title back after the approval dialog: {finished}"
621 + );
622 +
623 + let response = agent.wait_for_response(prompt_id);
624 + assert_eq!(response["result"]["stopReason"], "end_turn");
625 +
626 + drop(agent);
627 + let _ = std::fs::remove_dir_all(&scratch);
628 +}
629 +
630 #[test]
631 fn successful_config_option_changes_are_rendered_as_system_status_cards() {
632 let endpoint = start_fake_endpoint(vec![]);
@@ -782,9 +858,21 @@ fn plan_permission_mode_denies_mutating_tools_without_asking() {
858 "prompt": [{"type": "text", "text": "run the command"}],
859 }),
860 );
785 - let (response, _updates) = agent.wait_for_response_with_updates(prompt_id);
861 + let (response, updates) = agent.wait_for_response_with_updates(prompt_id);
862 assert_eq!(response["result"]["stopReason"], "end_turn");
863
864 + // The tool never ran, so its card must not end as a success (issue #139).
865 + let closed = updates
866 + .iter()
867 + .rev()
868 + .find(|update| {
869 + update["sessionUpdate"] == "tool_call_update"
870 + && update["toolCallId"] == "call_1"
871 + && !update["status"].is_null()
872 + })
873 + .expect("the blocked call is closed out");
874 + assert_eq!(closed["status"], "failed", "{closed}");
875 +
876 let requests = endpoint.requests.lock().unwrap();
877 assert_eq!(
878 requests.len(),
@@ -1126,6 +1214,71 @@ fn an_unparseable_tool_call_is_hidden_and_retried() {
1214 let _ = std::fs::remove_dir_all(&scratch);
1215 }
1216
1217 +/// The third identical call is skipped by the repetition guard. It never ran,
1218 +/// so its card ends `failed` while the two that did run end `completed`
1219 +/// (issue #139).
1220 +#[test]
1221 +fn a_call_skipped_by_the_repeat_guard_ends_failed() {
1222 + let repeated_arguments = json!({"path": "notes.txt"}).to_string();
1223 + let endpoint = start_fake_endpoint(vec![
1224 + sse_tool_call("call_1", "read_file", &repeated_arguments),
1225 + sse_tool_call("call_2", "read_file", &repeated_arguments),
1226 + sse_tool_call("call_3", "read_file", &repeated_arguments),
1227 + sse_text("Stopping here."),
1228 + ]);
1229 +
1230 + let scratch =
1231 + std::env::temp_dir().join(format!("sigit_acp_repeat_failed_{}", std::process::id()));
1232 + let config_dir = scratch.join("config");
1233 + let cwd = scratch.join("cwd");
1234 + std::fs::create_dir_all(&config_dir).unwrap();
1235 + std::fs::create_dir_all(&cwd).unwrap();
1236 + std::fs::write(cwd.join("notes.txt"), "hello").unwrap();
1237 +
1238 + let mut agent = spawn_agent(endpoint.port, &config_dir);
1239 +
1240 + let id = agent.request(
1241 + "initialize",
1242 + json!({"protocolVersion": 1, "clientCapabilities": {}}),
1243 + );
1244 + agent.wait_for_response(id);
1245 +
1246 + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));
1247 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]
1248 + .as_str()
1249 + .expect("session id")
1250 + .to_string();
1251 +
1252 + let prompt_id = agent.request(
1253 + "session/prompt",
1254 + json!({
1255 + "sessionId": session_id,
1256 + "prompt": [{"type": "text", "text": "read the notes"}],
1257 + }),
1258 + );
1259 + let (response, updates) = agent.wait_for_response_with_updates(prompt_id);
1260 + assert_eq!(response["result"]["stopReason"], "end_turn");
1261 +
1262 + let final_status = |call_id: &str| {
1263 + updates
1264 + .iter()
1265 + .rev()
1266 + .find(|update| {
1267 + update["sessionUpdate"] == "tool_call_update"
1268 + && update["toolCallId"] == call_id
1269 + && !update["status"].is_null()
1270 + })
1271 + .unwrap_or_else(|| panic!("{call_id} is closed out: {updates:#?}"))["status"]
1272 + .clone()
1273 + };
1274 + assert_eq!(final_status("call_1"), "completed");
1275 + assert_eq!(final_status("call_2"), "completed");
1276 + assert_eq!(final_status("call_3"), "failed");
1277 +
1278 + drop(agent);
1279 + let _ = std::fs::remove_dir_all(&scratch);
1280 +}
1281 +
1282 /// Once the repetition guard removes tools from a request, a model may still
1283 /// emit its learned tool syntax. Known calls must be removed from the visible
1284 /// response without being executed or recorded as orphaned history entries.