From 2f8958ccaa7538e1554705fa2c62bcd915ef3313 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 11 May 2026 14:31:33 -0400 Subject: [PATCH] fix: pass bootstrapped node path to ACP defaults --- .../fabro-workflow/src/handler/llm/acp.rs | 60 ++++++++++++++- .../src/handler/llm/node_runtime.rs | 76 ++++++++++++++++++- 2 files changed, 129 insertions(+), 7 deletions(-) diff --git a/lib/crates/fabro-workflow/src/handler/llm/acp.rs b/lib/crates/fabro-workflow/src/handler/llm/acp.rs index f43379147..6f138aa6c 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/acp.rs @@ -86,15 +86,20 @@ impl AgentAcpBackend { let command = resolve_acp_command(provider, explicit_command) .map_err(|err| Error::handler_with_source("Failed to resolve ACP command", &err))?; - if explicit_command.is_none() + let node_runtime_env = if explicit_command.is_none() && command.program() == default_acp_command(provider).program() { - node_runtime::ensure_node_runtime(sandbox, &cancel_token).await?; - } + Some(node_runtime::ensure_node_runtime(sandbox, &cancel_token).await?) + } else { + None + }; - let launch_env = self + let mut launch_env = self .launch_env(provider, emitter, sandbox, &cancel_token) .await?; + if let Some(runtime_env) = node_runtime_env { + node_runtime::apply_node_runtime_env(&mut launch_env, runtime_env); + } let on_activity = { let emitter = Arc::clone(emitter); Arc::new(move || emitter.touch()) as Arc @@ -342,6 +347,7 @@ mod tests { use fabro_agent::{LocalSandbox, Sandbox, shell_quote}; use fabro_graphviz::graph::{AttrValue, Node}; use fabro_model::Provider; + use fabro_sandbox::test_support::MockSandbox; use fabro_types::EventBody; use tokio_util::sync::CancellationToken; @@ -570,6 +576,52 @@ mod tests { assert!(!command.contains("secret-key")); } + #[tokio::test] + async fn acp_default_command_launch_env_includes_bootstrapped_node_path() { + let mut sandbox = MockSandbox::linux(); + sandbox.exec_result.stdout = + "__FABRO_NODE_RUNTIME_PATH=/home/test/.local/bin:/usr/local/bin:/usr/bin\n".to_string(); + let sandbox = Arc::new(sandbox); + let sandbox_dyn: Arc = sandbox.clone(); + + let mut node = Node::new("work"); + node.attrs.insert( + "provider".to_string(), + AttrValue::String("openai".to_string()), + ); + node.attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + + let backend = AgentAcpBackend::new_from_env("fake-acp".to_string(), Provider::OpenAi); + let result = backend + .run( + &node, + "write hello", + &Context::new(), + None, + &Arc::new(Emitter::default()), + &sandbox_dyn, + None, + CancellationToken::new(), + ) + .await; + assert!( + result.is_err(), + "mock stdio transport should not complete ACP" + ); + + let env = sandbox + .captured_env_vars + .lock() + .expect("captured env lock poisoned") + .clone() + .expect("ACP launch env should be captured"); + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/home/test/.local/bin:/usr/local/bin:/usr/bin") + ); + } + fn fake_agent_script() -> &'static str { r#" import json diff --git a/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs b/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs index 18787773c..838917584 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs @@ -1,10 +1,18 @@ use std::sync::Arc; use fabro_agent::Sandbox; +use fabro_static::EnvVars; use tokio_util::sync::CancellationToken; use crate::error::Error; +const NODE_RUNTIME_PATH_MARKER: &str = "__FABRO_NODE_RUNTIME_PATH="; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct NodeRuntimeEnv { + pub path: String, +} + pub fn ensure_node_runtime_shell() -> String { "export PATH=\"$HOME/.local/bin:$PATH\" && \ (node --version >/dev/null 2>&1 && npm --version >/dev/null 2>&1 && npx --version >/dev/null 2>&1 || \ @@ -15,8 +23,12 @@ pub fn ensure_node_runtime_shell() -> String { pub async fn ensure_node_runtime( sandbox: &Arc, cancel_token: &CancellationToken, -) -> Result<(), Error> { - let command = ensure_node_runtime_shell(); +) -> Result { + let command = format!( + "{} && printf '\\n{}%s\\n' \"$PATH\"", + ensure_node_runtime_shell(), + NODE_RUNTIME_PATH_MARKER + ); let result = sandbox .exec_command( &command, @@ -29,7 +41,10 @@ pub async fn ensure_node_runtime( .map_err(|err| Error::handler_with_source("Failed to ensure Node runtime", &err))?; if result.is_success() { - Ok(()) + let path = parse_node_runtime_path(&result.stdout).ok_or_else(|| { + Error::handler("Node runtime install did not report the sandbox PATH".to_string()) + })?; + Ok(NodeRuntimeEnv { path }) } else { Err(Error::handler(format!( "Node runtime install exited with code {}", @@ -37,3 +52,58 @@ pub async fn ensure_node_runtime( ))) } } + +pub fn apply_node_runtime_env( + launch_env: &mut std::collections::HashMap, + runtime_env: NodeRuntimeEnv, +) { + match launch_env.get_mut(EnvVars::PATH) { + Some(existing_path) if !existing_path.is_empty() => { + *existing_path = format!("{}:{existing_path}", runtime_env.path); + } + _ => { + launch_env.insert(EnvVars::PATH.to_string(), runtime_env.path); + } + } +} + +fn parse_node_runtime_path(stdout: &str) -> Option { + stdout + .lines() + .rev() + .find_map(|line| line.strip_prefix(NODE_RUNTIME_PATH_MARKER)) + .filter(|path| !path.is_empty()) + .map(str::to_string) +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use super::{NodeRuntimeEnv, apply_node_runtime_env, parse_node_runtime_path}; + + #[test] + fn parse_node_runtime_path_uses_last_reported_marker() { + assert_eq!( + parse_node_runtime_path( + "download output\n__FABRO_NODE_RUNTIME_PATH=/old\n\ + __FABRO_NODE_RUNTIME_PATH=/home/test/.local/bin:/usr/bin\n", + ), + Some("/home/test/.local/bin:/usr/bin".to_string()) + ); + } + + #[test] + fn apply_node_runtime_env_preserves_existing_path_tail() { + let mut env = HashMap::from([("PATH".to_string(), "/custom/bin".to_string())]); + + apply_node_runtime_env(&mut env, NodeRuntimeEnv { + path: "/home/test/.local/bin:/usr/bin".to_string(), + }); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/home/test/.local/bin:/usr/bin:/custom/bin") + ); + } +}