From e1d5b925e4f7e07346043d5c4bdff0e998196587 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 11 May 2026 13:22:41 -0400 Subject: [PATCH] fix: cover ACP prompt workflows and exit diagnostics --- lib/crates/fabro-acp/src/transport.rs | 6 +- lib/crates/fabro-acp/tests/session.rs | 13 +++- lib/crates/fabro-agent/src/sandbox.rs | 4 +- lib/crates/fabro-cli/tests/it/workflow/acp.rs | 71 +++++++++++++++++++ lib/crates/fabro-sandbox/src/docker.rs | 14 ++-- lib/crates/fabro-sandbox/src/lib.rs | 5 +- lib/crates/fabro-sandbox/src/local.rs | 15 ++-- lib/crates/fabro-sandbox/src/sandbox.rs | 28 +++++++- lib/crates/fabro-sandbox/src/test_support.rs | 5 +- 9 files changed, 138 insertions(+), 23 deletions(-) diff --git a/lib/crates/fabro-acp/src/transport.rs b/lib/crates/fabro-acp/src/transport.rs index a5ad283fa..431382147 100644 --- a/lib/crates/fabro-acp/src/transport.rs +++ b/lib/crates/fabro-acp/src/transport.rs @@ -129,8 +129,12 @@ impl ConnectTo for SandboxAcpTransport { termination = handle.wait() => { let termination = termination.map_err(ProtocolError::into_internal_error)?; let stderr = stderr.tail_string().await; + let exit_code = termination + .exit_code + .map_or_else(|| "unknown".to_string(), |code| code.to_string()); Err(internal_error(format!( - "ACP process exited before protocol completed: termination={termination:?}, stderr={stderr}" + "ACP process exited before protocol completed: termination={}, exit_code={exit_code}, stderr={stderr}", + termination.termination, ))) } } diff --git a/lib/crates/fabro-acp/tests/session.rs b/lib/crates/fabro-acp/tests/session.rs index fd7e09ff2..3f57046c1 100644 --- a/lib/crates/fabro-acp/tests/session.rs +++ b/lib/crates/fabro-acp/tests/session.rs @@ -309,7 +309,18 @@ async fn early_exit_returns_protocol_error_with_stderr() { .await .expect_err("early exit should error"); - assert!(matches!(err, AcpError::Protocol(_))); + let AcpError::Protocol(error) = err else { + panic!("expected protocol error"); + }; + let message = error.to_string(); + assert!( + message.contains("exit_code=2"), + "early exit should include exit code in diagnostic: {message}" + ); + assert!( + message.contains("early boom"), + "early exit should include stderr tail in diagnostic: {message}" + ); } async fn run_fake_agent( diff --git a/lib/crates/fabro-agent/src/sandbox.rs b/lib/crates/fabro-agent/src/sandbox.rs index 60ad25581..6256967e0 100644 --- a/lib/crates/fabro-agent/src/sandbox.rs +++ b/lib/crates/fabro-agent/src/sandbox.rs @@ -4,6 +4,6 @@ pub use fabro_sandbox::{ CommandOutputCallback, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, StdioProcess, StdioProcessHandle, - WorktreeEvent, WorktreeEventCallback, WorktreeOptions, WorktreeSandbox, delegate_sandbox, - format_lines_numbered, shell_quote, + StdioProcessTermination, WorktreeEvent, WorktreeEventCallback, WorktreeOptions, + WorktreeSandbox, delegate_sandbox, format_lines_numbered, shell_quote, }; diff --git a/lib/crates/fabro-cli/tests/it/workflow/acp.rs b/lib/crates/fabro-cli/tests/it/workflow/acp.rs index 780e9f89c..60c8de35a 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/acp.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/acp.rs @@ -85,6 +85,77 @@ fn acp_backend_workflow() { ); } +#[test] +fn acp_prompt_workflow_uses_acp_backend() { + let mut context = test_context!(); + context.write_home( + ".fabro/settings.toml", + "[server.auth]\nmethods = [\"dev-token\"]\n", + ); + context.isolated_server(); + seed_openai_vault(&context.storage_dir); + let fake_agent = fixture("fake_acp_agent.py"); + let workflow = context.temp_dir.join("acp_prompt_backend.fabro"); + context.write_temp( + "acp_prompt_backend.fabro", + format!( + r#"digraph ACP {{ + graph [goal="Exercise ACP prompt backend"] + start [shape=Mdiamond] + prompt [type="prompt", backend="acp", provider="openai", model="fake-acp", project_memory=false, prompt="write hello.txt", acp_command="python3 {}"] + exit [shape=Msquare] + start -> prompt + prompt -> exit +}}"#, + fake_agent.display() + ), + ); + init_git_repo(&context.temp_dir); + + context + .run_cmd() + .args(["--auto-approve", "--sandbox", "local"]) + .arg(&workflow) + .assert() + .success(); + + let run_dir = find_run_dir(&context); + let conclusion = read_conclusion(&run_dir); + assert_eq!(conclusion["status"].as_str(), Some("succeeded")); + + let events = run_events(&run_dir); + assert!(has_event(&run_dir, "agent.acp.started")); + assert!(has_event(&run_dir, "agent.acp.completed")); + assert!( + !has_event(&run_dir, "agent.session.activated"), + "ACP prompt should not activate an API-mode agent session" + ); + let completed = events + .iter() + .find_map(|event| match &event.event.body { + EventBody::StageCompleted(props) + if event.event.node_id.as_deref() == Some("prompt") => + { + Some(props) + } + _ => None, + }) + .expect("prompt stage should complete"); + assert_eq!(completed.response.as_deref(), Some("hello from acp")); + + let state = serde_json::to_value(run_state(&run_dir)).expect("run state should serialize"); + let stages = state["stages"] + .as_object() + .expect("run state should contain stages"); + assert!( + stages.values().any(|stage| { + stage["provider_used"]["mode"] == "acp" + && stage["provider_used"]["provider"] == "openai" + }), + "run projection should include ACP provider metadata: {stages:?}" + ); +} + fn seed_openai_vault(storage_dir: &std::path::Path) { let mut vault = Vault::load(Storage::new(storage_dir).secrets_path()).expect("test vault should load"); diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index a541fc416..9c914c2ab 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -30,7 +30,7 @@ use crate::sandbox::{StdioProcessControl, optional_timeout, resolve_path}; use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, - StdioProcess, StdioProcessHandle, format_lines_numbered, shell_quote, + StdioProcess, StdioProcessHandle, StdioProcessTermination, format_lines_numbered, shell_quote, }; const WORKING_DIRECTORY: &str = "/workspace"; @@ -882,7 +882,7 @@ struct DockerStdioProcessControl { container_id: String, exec_id: String, stop_file: String, - termination: TokioMutex>, + termination: TokioMutex>, } #[async_trait] @@ -892,11 +892,11 @@ impl StdioProcessControl for DockerStdioProcessControl { return Ok(()); } request_docker_exec_stop_with(&self.docker, &self.container_id, &self.stop_file).await?; - *self.termination.lock().await = Some(CommandTermination::Cancelled); + *self.termination.lock().await = Some(StdioProcessTermination::cancelled()); Ok(()) } - async fn wait(&self) -> crate::Result { + async fn wait(&self) -> crate::Result { if let Some(termination) = *self.termination.lock().await { return Ok(termination); } @@ -908,8 +908,10 @@ impl StdioProcessControl for DockerStdioProcessControl { .await .map_err(|e| crate::Error::context("Failed to inspect Docker stdio exec", e))?; if inspect.running != Some(true) { - *self.termination.lock().await = Some(CommandTermination::Exited); - return Ok(CommandTermination::Exited); + let exit_code = inspect.exit_code.and_then(|code| i32::try_from(code).ok()); + let termination = StdioProcessTermination::exited(exit_code); + *self.termination.lock().await = Some(termination); + return Ok(termination); } time::sleep(std::time::Duration::from_millis(50)).await; } diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index fb2e98753..04ca2fb86 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -41,8 +41,9 @@ pub use reconnect::{reconnect, reconnect_for_run, reconnect_for_run_with_callbac pub use sandbox::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GitRunInfo, GitSetupIntent, GrepOptions, Sandbox, SandboxEvent, - SandboxEventCallback, StderrCollector, StdioProcess, StdioProcessHandle, format_lines_numbered, - git_push_via_exec, redacted_output_tail, setup_git_via_exec, shell_quote, + SandboxEventCallback, StderrCollector, StdioProcess, StdioProcessHandle, + StdioProcessTermination, format_lines_numbered, git_push_via_exec, redacted_output_tail, + setup_git_via_exec, shell_quote, }; pub use sandbox_spec::SandboxSpec; pub use terminal::{TerminalSession, TerminalSize, open_terminal_for_run}; diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index c29156bcf..5418fb567 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -16,7 +16,7 @@ use crate::sandbox::{StdioProcessControl, optional_timeout}; use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, - StdioProcess, StdioProcessHandle, format_lines_numbered, + StdioProcess, StdioProcessHandle, StdioProcessTermination, format_lines_numbered, }; pub struct LocalSandbox { @@ -145,7 +145,7 @@ where struct LocalStdioProcessControl { child: TokioMutex, - termination: TokioMutex>, + termination: TokioMutex>, } #[async_trait] @@ -157,22 +157,23 @@ impl StdioProcessControl for LocalStdioProcessControl { let mut child = self.child.lock().await; sigterm_then_kill(&mut child).await; - *self.termination.lock().await = Some(CommandTermination::Cancelled); + *self.termination.lock().await = Some(StdioProcessTermination::cancelled()); Ok(()) } - async fn wait(&self) -> crate::Result { + async fn wait(&self) -> crate::Result { if let Some(termination) = *self.termination.lock().await { return Ok(termination); } let mut child = self.child.lock().await; - child + let status = child .wait() .await .map_err(|e| crate::Error::context("Failed to wait for stdio process", e))?; - *self.termination.lock().await = Some(CommandTermination::Exited); - Ok(CommandTermination::Exited) + let termination = StdioProcessTermination::exited(status.code()); + *self.termination.lock().await = Some(termination); + Ok(termination) } } diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index 90655d48e..4679f972a 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -754,15 +754,39 @@ impl StdioProcessHandle { self.control.terminate().await } - pub async fn wait(&self) -> crate::Result { + pub async fn wait(&self) -> crate::Result { self.control.wait().await } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct StdioProcessTermination { + pub termination: CommandTermination, + pub exit_code: Option, +} + +impl StdioProcessTermination { + #[must_use] + pub fn exited(exit_code: Option) -> Self { + Self { + termination: CommandTermination::Exited, + exit_code, + } + } + + #[must_use] + pub fn cancelled() -> Self { + Self { + termination: CommandTermination::Cancelled, + exit_code: None, + } + } +} + #[async_trait] pub(crate) trait StdioProcessControl: Send + Sync { async fn terminate(&self) -> crate::Result<()>; - async fn wait(&self) -> crate::Result; + async fn wait(&self) -> crate::Result; } #[derive(Debug, Clone)] diff --git a/lib/crates/fabro-sandbox/src/test_support.rs b/lib/crates/fabro-sandbox/src/test_support.rs index bd2bcf013..5ab0ec71b 100644 --- a/lib/crates/fabro-sandbox/src/test_support.rs +++ b/lib/crates/fabro-sandbox/src/test_support.rs @@ -11,6 +11,7 @@ use crate::sandbox::StdioProcessControl; use crate::{ DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, StdioProcess, StdioProcessHandle, + StdioProcessTermination, }; // --- MockSandbox --- @@ -117,8 +118,8 @@ impl StdioProcessControl for MockStdioProcessControl { Ok(()) } - async fn wait(&self) -> crate::Result { - Ok(CommandTermination::Exited) + async fn wait(&self) -> crate::Result { + Ok(StdioProcessTermination::exited(Some(0))) } }