@sigitcodereview / sigit / commits / 124c80c

Harden ACP tool-call content rendering

Preserve whitespace-only output, avoid misleading JSON fences for malformed arguments, sanitize fence language identifiers, and add regression coverage. Co-Authored-By: siGit Code <noreply@sigit.si>

Seto Elkahfi committed Sep 10, 2026 at 14:57 UTC 124c80cb2c18a289416a36f3a3f5a0cd519890ef

1 file changed +60 -5

src/main.rs

+60 -5

@@ -370,15 +370,35 @@ fn longest_backtick_run(text: &str) -> usize {

370 /// backtick run already in `text` so it can't break out of the block.

371 fn fenced_code_block(language: &str, text: &str) -> String {

372 let fence = "`".repeat((longest_backtick_run(text) + 1).max(3));

373 + let language = if language

374 + .chars()

375 + .all(|character| character.is_ascii_alphanumeric() || matches!(character, '-' | '_' | '+'))

376 + {

377 + language

378 + } else {

379 + ""

380 + };

381 format!("{fence}{language}\n{text}\n{fence}")

382 }

383

384 +/// Displays tool arguments as JSON when they parse as JSON, otherwise as an

385 +/// unlabelled code block so malformed arguments do not get misleading syntax

386 +/// highlighting.

387 +fn tool_arguments_content(arguments_json: &str) -> ToolCallContent {

388 + let language = if serde_json::from_str::<serde_json::Value>(arguments_json).is_ok() {

389 + "json"

390 + } else {

391 + ""

392 + };

393 + fenced_code_block(language, &chat::pretty_tool_arguments(arguments_json)).into()

394 +}

395 +

396 /// Wraps a tool's output as the display content of its ACP tool-call card

397 /// (issue #78). Capped via [`chat::cap_output_preview`] — display only, the

398 /// model still gets the full output — and fenced so plain-text output (or a

399 /// stray tag, per issue #73) renders literally instead of as markdown.

400 fn tool_output_content(output: &str) -> ToolCallContent {

381 - if output.trim().is_empty() {

401 + if output.is_empty() {

402 return "(no output)".to_string().into();

403 }

404 fenced_code_block("", &chat::cap_output_preview(output)).into()

@@ -1882,8 +1902,7 @@ impl SiGitAgent {

1902 log::warn!("malformed JSON in tool arguments for '{}': {e}", tc.name);

1903 serde_json::Value::String(tc.arguments.clone())

1904 });

1885 - let invocation_content =

1886 - fenced_code_block("json", &chat::pretty_tool_arguments(&tc.arguments));

1905 + let invocation_content = tool_arguments_content(&tc.arguments);

1906 self.send_tool_call_update(

1907 cx,

1908 session_id.clone(),

@@ -1891,7 +1910,7 @@ impl SiGitAgent {

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

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

1912 .status(ToolCallStatus::InProgress)

1894 - .content(vec![invocation_content.into()])

1913 + .content(vec![invocation_content])

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

1915 .raw_input(raw_input),

1916 ),

@@ -4371,10 +4390,33 @@ mod tests {

4390

4391 #[test]

4392 fn tool_output_content_shows_a_placeholder_for_empty_output() {

4374 - let content = tool_output_content(" \n\t ");

4393 + let content = tool_output_content("");

4394 assert!(format!("{content:?}").contains("(no output)"));

4395 }

4396

4397 + #[test]

4398 + fn tool_output_content_preserves_whitespace_only_output() {

4399 + let content = tool_output_content(" \n\t ");

4400 + let rendered = format!("{content:?}");

4401 + assert!(!rendered.contains("(no output)"));

4402 + assert!(rendered.contains(" \\n\\t "));

4403 + }

4404 +

4405 + #[test]

4406 + fn tool_arguments_content_only_labels_valid_json() {

4407 + let json = format!("{:?}", tool_arguments_content(r#"{"path":"src/main.rs"}"#));

4408 + let malformed = format!("{:?}", tool_arguments_content("{not json"));

4409 + assert!(json.contains("```json"));

4410 + assert!(!malformed.contains("```json"));

4411 + assert!(malformed.contains("{not json"));

4412 + }

4413 +

4414 + #[test]

4415 + fn fenced_code_block_drops_an_unsafe_language() {

4416 + let block = fenced_code_block("json\nnot fenced", "{}");

4417 + assert_eq!(block, "```\n{}\n```");

4418 + }

4419 +

4420 #[test]

4421 fn tool_output_content_widens_the_fence_around_embedded_backticks() {

4422 let output = "here is a fence:\n```\ncode\n```";

@@ -4400,4 +4442,17 @@ mod tests {

4442 assert_eq!(longest_backtick_run("no backticks here"), 0);

4443 assert_eq!(longest_backtick_run("one ` two `` three ``` four"), 3);

4444 }

4445 +

4446 + #[test]

4447 + fn tool_call_locations_require_a_known_tool_and_string_path() {

4448 + assert!(tool_call_locations("read_file", "{not json").is_empty());

4449 + assert!(tool_call_locations("read_file", r#"{"path":42}"#).is_empty());

4450 + assert!(

4451 + tool_call_locations("future_multi_file_tool", r#"{"path":"src/main.rs"}"#).is_empty()

4452 + );

4453 +

4454 + let locations = tool_call_locations("multi_edit", r#"{"path":"src/main.rs"}"#);

4455 + assert_eq!(locations.len(), 1);

4456 + assert!(locations[0].path.ends_with("src/main.rs"));

4457 + }

4458 }