From 9f14b6c64f8e654b4f579cca47c1da3d21670098 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 14:24:58 +1000 Subject: [PATCH 1/2] fix(actions): recover the action array from one-shot agent responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detect Actions has returned zero actions for every repo added since 2026-07-07. The agent explores the project and answers correctly, but the response never survives parsing. Two things had to line up: - `BasicMessageWriter` — the writer behind `run_acp_prompt` — appended `[Tool: …]` / `[Result: …]` markers to the same buffer it collects agent text in, so `get_text()` returned a transcript rather than an answer. Every caller of that buffer (action detection, run-phase autodetect, badge short names) parses it as JSON. - `extract_json_array` sliced from the first `[` to the last `]`. Once the transcript led with `[Tool: Terminal]`, that slice covered the whole marker-riddled transcript and never parsed. Stop recording tool activity in `BasicMessageWriter`, and replace the first-bracket-to-last-bracket slice with a backward scan: candidate `[` positions are tried from the end of the response, each prefix-parsed into `Vec`. That takes the agent's final array, ignores trailing text such as a closing code fence, and skips arrays of some other shape. Verified against the 8 KB response logged for `block/berd`, which the old extractor rejected and the new one parses into all 32 actions. Detection results are not backfilled: contexts already marked detected need the manual Detect Actions button pressed once. Signed-off-by: Matt Toohey --- crates/acp-client/README.md | 2 +- crates/acp-client/src/driver.rs | 30 ++++-- crates/acp-client/src/simple.rs | 4 +- crates/builderbot-actions/src/detector.rs | 123 ++++++++++++++-------- 4 files changed, 108 insertions(+), 51 deletions(-) diff --git a/crates/acp-client/README.md b/crates/acp-client/README.md index 8768c6c60..9d8c54177 100644 --- a/crates/acp-client/README.md +++ b/crates/acp-client/README.md @@ -81,7 +81,7 @@ if let Some(agent) = find_acp_agent_by_id("goose") { ### Prompt Functions -- `run_acp_prompt(agent, working_dir, prompt) -> anyhow::Result` - Send a one-shot prompt +- `run_acp_prompt(agent, working_dir, prompt) -> anyhow::Result` - Send a one-shot prompt; returns the agent's text only (tool calls and results are omitted) ### Types diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index ef8c8bb58..a251f059d 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -2595,6 +2595,11 @@ fn extract_content_preview(content: &[ToolCallContent]) -> Option { // ============================================================================= /// Simple in-memory message writer for basic usage. +/// +/// Only agent *text* is recorded. Tool calls and tool results are deliberately +/// dropped: this writer backs one-shot prompting ([`crate::run_acp_prompt`]), +/// whose callers parse the accumulated text as machine-readable output (JSON). +/// Interleaving tool markers would corrupt that payload. pub struct BasicMessageWriter { text: Mutex, last_flush_at: Mutex, @@ -2634,11 +2639,11 @@ impl MessageWriter for BasicMessageWriter { async fn record_tool_call( &self, _tool_call_id: &str, - title: &str, + _title: &str, _raw_input: Option<&serde_json::Value>, ) { - let mut current = self.text.lock().await; - current.push_str(&format!("\n[Tool: {}]\n", title)); + // Intentionally ignored: see the type-level docs. Tool activity must not + // land in the buffer that one-shot callers parse. } async fn update_tool_call_title( @@ -2650,9 +2655,8 @@ impl MessageWriter for BasicMessageWriter { // Nothing to do for basic implementation } - async fn record_tool_result(&self, _tool_call_id: &str, content: &str) { - let mut current = self.text.lock().await; - current.push_str(&format!("\n[Result: {}]\n", content)); + async fn record_tool_result(&self, _tool_call_id: &str, _content: &str) { + // Intentionally ignored: see the type-level docs. } } @@ -3523,6 +3527,20 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE assert_eq!(buffer.match_cursor, 2); } + #[tokio::test] + async fn basic_writer_records_only_agent_text() { + let writer = BasicMessageWriter::new(); + + writer.append_text("[").await; + writer.record_tool_call("call-1", "Terminal", None).await; + writer.record_tool_result("call-1", "total 42").await; + writer.append_text("]").await; + + // One-shot callers parse this buffer as JSON, so tool activity must not + // leak into it. + assert_eq!(writer.get_text().await, "[]"); + } + #[tokio::test] async fn replay_handler_treats_user_only_boundaries_as_complete() { let writer: Arc = Arc::new(BasicMessageWriter::new()); diff --git a/crates/acp-client/src/simple.rs b/crates/acp-client/src/simple.rs index fa7b64143..901a02e99 100644 --- a/crates/acp-client/src/simple.rs +++ b/crates/acp-client/src/simple.rs @@ -96,7 +96,9 @@ impl AgentDriver for SimpleDriverWrapper { /// /// # Returns /// -/// The agent's text response +/// The agent's text response. Tool calls and tool results the agent made along +/// the way are not included, so the result stays parseable when the prompt asks +/// for structured output. pub async fn run_acp_prompt(agent: &AcpAgent, working_dir: &Path, prompt: &str) -> Result { run_acp_prompt_with_options(agent, working_dir, prompt, None).await } diff --git a/crates/builderbot-actions/src/detector.rs b/crates/builderbot-actions/src/detector.rs index cd1f52156..56e454de3 100644 --- a/crates/builderbot-actions/src/detector.rs +++ b/crates/builderbot-actions/src/detector.rs @@ -297,43 +297,27 @@ fn has_git_hooks_path_override(working_dir: &Path) -> bool { .unwrap_or(false) } -/// Parse the AI response and extract suggested actions +/// Parse the AI response and extract suggested actions. +/// +/// The response is not guaranteed to be bare JSON: agents wrap the array in a +/// markdown code fence, prefix it with prose, and may emit brackets of their own +/// (`[1]`, glob patterns, transcript markers) before the answer. So candidate +/// `[` positions are scanned from the **end** of the response — the agent's +/// final array wins — and each candidate is prefix-parsed, which lets trailing +/// text such as a closing fence be ignored. A candidate only counts if it +/// deserializes into `Vec`, so unrelated arrays are skipped. fn parse_ai_response(response: &str) -> Result> { - let json_str = extract_json_array(response)?; - - let actions: Vec = serde_json::from_str(&json_str).map_err(|e| { - anyhow::anyhow!( - "Failed to parse AI response as JSON: {}. Response was: {}", - e, - json_str - ) - })?; - - Ok(actions) -} - -/// Extract JSON array from AI response that might contain extra text -fn extract_json_array(text: &str) -> Result { - // First try to parse the entire response as JSON - if text.trim().starts_with('[') && serde_json::from_str::(text).is_ok() { - return Ok(text.to_string()); - } - - // Look for JSON array in the text - if let Some(start) = text.find('[') { - if let Some(end) = text.rfind(']') { - if end > start { - let json_str = &text[start..=end]; - if serde_json::from_str::(json_str).is_ok() { - return Ok(json_str.to_string()); - } - } + for (start, _) in response.rmatch_indices('[') { + let mut stream = serde_json::Deserializer::from_str(&response[start..]) + .into_iter::>(); + if let Some(Ok(actions)) = stream.next() { + return Ok(actions); } } Err(anyhow::anyhow!( "Could not find valid JSON array in AI response. Response was: {}", - text + response )) } @@ -341,23 +325,76 @@ fn extract_json_array(text: &str) -> Result { mod tests { use super::*; + const TEST_ACTION: &str = r#"{"name": "Test", "command": "npm test", "actionType": "check", "autoCommit": false, "source": "package.json"}"#; + #[test] - fn test_extract_json_array() { - let text = r#"Here are some actions: -[ - {"name": "Test", "command": "npm test", "actionType": "check", "autoCommit": false, "source": "package.json"} -] -That's all!"#; + fn parses_array_surrounded_by_prose() { + let text = format!("Here are some actions:\n[\n {TEST_ACTION}\n]\nThat's all!"); + + let actions = parse_ai_response(&text).expect("array between prose should parse"); + + assert_eq!(actions.len(), 1); + assert_eq!(actions[0].name, "Test"); + } + + #[test] + fn parses_bare_array() { + let text = format!("[{TEST_ACTION}]"); + + let actions = parse_ai_response(&text).expect("bare array should parse"); + + assert_eq!(actions.len(), 1); + } + + #[test] + fn parses_fenced_array_after_tool_transcript() { + // Regression: one-shot transcripts used to interleave `[Tool: …]` / + // `[Result: …]` markers, whose leading `[` swallowed the real array. + let text = format!( + "I'll explore the project structure.\n\ + [Tool: Terminal]\n\ + [Result: List top-level project files]\n\ + ```json\n\ + [{TEST_ACTION}]\n\ + ```\n" + ); + + let actions = parse_ai_response(&text).expect("fenced array after markers should parse"); + + assert_eq!(actions.len(), 1); + assert_eq!(actions[0].command, "npm test"); + } + + #[test] + fn prefers_the_final_array_over_earlier_ones() { + let text = format!( + "Example of the shape I will return:\n\ + [{{\"name\": \"Example\", \"command\": \"echo hi\", \"actionType\": \"run\", \"autoCommit\": false, \"source\": \"README.md\"}}]\n\ + Here is the real answer:\n\ + [{TEST_ACTION}]" + ); + + let actions = parse_ai_response(&text).expect("last valid array should win"); + + assert_eq!(actions.len(), 1); + assert_eq!(actions[0].name, "Test"); + } + + #[test] + fn skips_arrays_that_are_not_actions() { + let text = format!("Candidates: [\"justfile\", \"package.json\"]\n[{TEST_ACTION}]\n[1, 2]"); + + let actions = parse_ai_response(&text).expect("non-action arrays should be skipped"); - let result = extract_json_array(text); - assert!(result.is_ok()); + assert_eq!(actions.len(), 1); + assert_eq!(actions[0].source, "package.json"); } #[test] - fn test_extract_json_array_clean() { - let text = r#"[{"name": "Test", "command": "npm test", "actionType": "check", "autoCommit": false, "source": "package.json"}]"#; + fn errors_when_no_action_array_is_present() { + let err = parse_ai_response("I could not find any build files. [Tool: Terminal]") + .expect_err("responses without an action array should fail"); - let result = extract_json_array(text); - assert!(result.is_ok()); + assert!(err.to_string().contains("Could not find valid JSON array")); } } From 1da25ef4033df29b0c8cf7353516ebb82b160393 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 14:54:12 +1000 Subject: [PATCH 2/2] fix(actions): surface the shape error when the action array almost parses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review dae123fa flagged a diagnosability regression in the backward-scan extractor: a near-miss array — valid JSON, but with e.g. one malformed actionType among 32 otherwise-valid actions — was silently skipped, and the caller only saw the generic "could not find valid JSON array" error. The old first-to-last-bracket slice at least surfaced the serde error. Now, when no candidate deserializes into Vec, the scan remembers the typed serde error from the closest candidate that parses as a JSON array of objects, and reports that instead of the generic message. Arrays of non-objects (file lists, exit codes) still don't count, so their unhelpful type errors can't mask a real near-miss earlier in the response. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- crates/builderbot-actions/src/detector.rs | 72 ++++++++++++++++++++--- 1 file changed, 64 insertions(+), 8 deletions(-) diff --git a/crates/builderbot-actions/src/detector.rs b/crates/builderbot-actions/src/detector.rs index 56e454de3..f874c134a 100644 --- a/crates/builderbot-actions/src/detector.rs +++ b/crates/builderbot-actions/src/detector.rs @@ -306,19 +306,44 @@ fn has_git_hooks_path_override(working_dir: &Path) -> bool { /// final array wins — and each candidate is prefix-parsed, which lets trailing /// text such as a closing fence be ignored. A candidate only counts if it /// deserializes into `Vec`, so unrelated arrays are skipped. +/// +/// When nothing parses, a candidate that is a valid JSON array of objects but +/// fails the `SuggestedAction` shape is almost certainly the agent's answer +/// with a drifted field, so the typed error for the closest such near-miss is +/// surfaced instead of the generic not-found message. fn parse_ai_response(response: &str) -> Result> { + let mut near_miss: Option = None; + for (start, _) in response.rmatch_indices('[') { - let mut stream = serde_json::Deserializer::from_str(&response[start..]) - .into_iter::>(); - if let Some(Ok(actions)) = stream.next() { - return Ok(actions); + let candidate = &response[start..]; + let mut stream = + serde_json::Deserializer::from_str(candidate).into_iter::>(); + match stream.next() { + Some(Ok(actions)) => return Ok(actions), + Some(Err(shape_err)) if near_miss.is_none() => { + let mut untyped = serde_json::Deserializer::from_str(candidate) + .into_iter::>(); + if let Some(Ok(values)) = untyped.next() { + if values.iter().all(|v| v.is_object()) { + near_miss = Some(shape_err); + } + } + } + _ => {} } } - Err(anyhow::anyhow!( - "Could not find valid JSON array in AI response. Response was: {}", - response - )) + match near_miss { + Some(shape_err) => Err(anyhow::anyhow!( + "Found a JSON array of objects in the AI response, but it does not match the expected action shape ({}). Response was: {}", + shape_err, + response + )), + None => Err(anyhow::anyhow!( + "Could not find valid JSON array in AI response. Response was: {}", + response + )), + } } #[cfg(test)] @@ -397,4 +422,35 @@ mod tests { assert!(err.to_string().contains("Could not find valid JSON array")); } + + #[test] + fn surfaces_the_shape_error_for_near_miss_arrays() { + // One malformed actionType in an otherwise valid array: the serde + // error should reach the caller instead of the generic message. + let text = r#"[{"name": "Lint", "command": "just lint", "actionType": "lintfix", "autoCommit": false, "source": "justfile"}]"#; + + let err = parse_ai_response(text).expect_err("near-miss arrays should fail"); + + let message = err.to_string(); + assert!(message.contains("does not match the expected action shape")); + assert!(message.contains("lintfix")); + } + + #[test] + fn near_miss_error_is_not_masked_by_trailing_junk_arrays() { + let text = r#"[{"name": "Lint", "command": "just lint", "actionType": "lintfix", "autoCommit": false, "source": "justfile"}] +Exit codes seen: [1, 2]"#; + + let err = parse_ai_response(text).expect_err("near-miss arrays should fail"); + + assert!(err.to_string().contains("lintfix")); + } + + #[test] + fn non_object_arrays_do_not_count_as_near_misses() { + let err = parse_ai_response(r#"Files found: ["justfile", "package.json"]"#) + .expect_err("responses without an action array should fail"); + + assert!(err.to_string().contains("Could not find valid JSON array")); + } }