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..f874c134a 100644 --- a/crates/builderbot-actions/src/detector.rs +++ b/crates/builderbot-actions/src/detector.rs @@ -297,67 +297,160 @@ 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. +/// +/// 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 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()); + let mut near_miss: Option = None; + + for (start, _) in response.rmatch_indices('[') { + 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: {}", - text - )) + 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)] 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"); + + assert_eq!(actions.len(), 1); + assert_eq!(actions[0].source, "package.json"); + } + + #[test] + 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"); + + 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"); - let result = extract_json_array(text); - assert!(result.is_ok()); + assert!(err.to_string().contains("lintfix")); } #[test] - fn test_extract_json_array_clean() { - let text = r#"[{"name": "Test", "command": "npm test", "actionType": "check", "autoCommit": false, "source": "package.json"}]"#; + 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"); - let result = extract_json_array(text); - assert!(result.is_ok()); + assert!(err.to_string().contains("Could not find valid JSON array")); } }