diff --git a/docs/internal/events.md b/docs/internal/events.md index ce4c37d3f..f548196d6 100644 --- a/docs/internal/events.md +++ b/docs/internal/events.md @@ -1952,6 +1952,8 @@ Emitted when an image or snapshot ensure step fails. ## CLI ensure events +These legacy events may appear in older run logs. Current CLI backend runs do not emit them because Fabro no longer installs or prepares provider CLIs at stage runtime. + ### `cli.ensure.started` ```json diff --git a/docs/internal/fabro-event-schema-v2-concrete-shape.md b/docs/internal/fabro-event-schema-v2-concrete-shape.md index b4032a495..feee1a2b5 100644 --- a/docs/internal/fabro-event-schema-v2-concrete-shape.md +++ b/docs/internal/fabro-event-schema-v2-concrete-shape.md @@ -401,7 +401,7 @@ V2 keeps the current durable family surface broadly intact. - `sandbox.*` - `setup.*` -- `cli.ensure.*` +- `cli.ensure.*` (legacy only) - `command.*` - `agent.cli.*` - `devcontainer.*` diff --git a/docs/public/core-concepts/agents.mdx b/docs/public/core-concepts/agents.mdx index 2bede229a..8a4331d4f 100644 --- a/docs/public/core-concepts/agents.mdx +++ b/docs/public/core-concepts/agents.mdx @@ -41,6 +41,8 @@ The CLI is selected automatically based on the node's provider: | OpenAI | `codex` | | Gemini | `gemini` | +Fabro does not install these CLIs at runtime. Install the selected CLI in the sandbox image or run setup steps before the workflow reaches a `backend="cli"` node. + Set the CLI backend on a node with `backend="cli"` or via a [model stylesheet](/workflows/stylesheets): ```dot @@ -56,26 +58,14 @@ implement [label="Implement", backend="cli"] Fabro can also run Agent Client Protocol (ACP) stdio agents with `backend="acp"`. ACP agents run inside the active Fabro sandbox, so local and Docker runs keep the same workspace isolation, secret forwarding, cancellation, and file-change tracking behavior as other agent stages. -Set ACP on a node with `backend="acp"`: - -```dot -implement [label="Implement", backend="acp", provider="openai"] -``` - -Fabro chooses a default ACP command from the provider: - -| Provider | Default ACP command | -|---|---| -| Anthropic | `npx -y @zed-industries/claude-code-acp@latest` | -| OpenAI and OpenAI-compatible providers | `npx -y @zed-industries/codex-acp@latest` | -| Gemini | `npx -y -- @google/gemini-cli@latest --experimental-acp` | - -Use `acp_command="..."` to point a node at a specific ACP stdio command: +Set ACP on a node with `backend="acp"` and an explicit `acp_command`: ```dot implement [label="Implement", backend="acp", acp_command="python3 tools/fake_acp_agent.py"] ``` +Fabro does not install ACP agents, Node.js, npm, or `npx` at runtime. The command must already be available in the sandbox image, repository, or setup steps. You can use `npx ...@latest` as an explicit `acp_command` if that is the behavior you want, but Fabro will treat it like any other user-supplied command. + ACP v1 does not have a portable model-selection request. Fabro records the selected provider and model in events and run projections, but model-specific ACP behavior must be encoded in the chosen command for now. ACP is supported with local and Docker sandboxes; Daytona does not expose bidirectional stdio yet, so ACP nodes fail there with an explicit unsupported-provider error. ### Comparison diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 290acc74c..3ba7a7259 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -207,7 +207,7 @@ Start nodes can also be identified by ID (`start` or `Start`). Exit nodes can be | `provider` | String | Explicit provider name (overrides stylesheet). Auto-inferred from the model catalog when omitted. | | `project_memory` | Boolean | When `true` (default), prompt nodes discover and include project docs (`AGENTS.md`, `CLAUDE.md`, etc.) as a system prompt. Set to `false` to disable. | | `backend` | String | Agent execution backend: `api` (default), `cli`, or `acp`. `api` runs Fabro's tool loop through provider APIs; `cli` delegates to the legacy provider CLI; `acp` runs an Agent Client Protocol stdio agent inside the active sandbox. See [Agents — Backends](/core-concepts/agents#backends). | -| `acp_command` | String | Optional ACP stdio command override for nodes with `backend="acp"`. Defaults are selected from `provider`; model selection is recorded in Fabro but not sent through stable ACP v1. | +| `acp_command` | String | Required for nodes with `backend="acp"`. The value must be a stdio ACP command available in the sandbox. Fabro records model selection but does not send it through stable ACP v1. | ### Command nodes diff --git a/lib/crates/fabro-acp/src/command.rs b/lib/crates/fabro-acp/src/command.rs index 0244654e6..670111706 100644 --- a/lib/crates/fabro-acp/src/command.rs +++ b/lib/crates/fabro-acp/src/command.rs @@ -51,6 +51,10 @@ impl std::fmt::Display for AcpCommand { pub enum AcpCommandError { #[error("acp_command must not be empty")] EmptyOverride, + #[error( + "acp_command is required for backend=\"acp\" because Fabro does not install ACP agents" + )] + MissingOverride, #[error("only stdio ACP commands are supported")] UnsupportedTransport, #[error("failed to parse acp_command")] @@ -63,41 +67,8 @@ impl From for AcpCommandError { } } -#[must_use] -pub fn default_acp_command(provider: Provider) -> AcpCommand { - match provider { - Provider::Anthropic => { - command_from_parts("npx -y @zed-industries/claude-code-acp@latest", "npx", [ - "-y", - "@zed-industries/claude-code-acp@latest", - ]) - } - Provider::Gemini => command_from_parts( - "npx -y -- @google/gemini-cli@latest --experimental-acp", - "npx", - [ - "-y", - "--", - "@google/gemini-cli@latest", - "--experimental-acp", - ], - ), - Provider::OpenAi - | Provider::Kimi - | Provider::Zai - | Provider::Minimax - | Provider::Inception - | Provider::OpenAiCompatible => { - command_from_parts("npx -y @zed-industries/codex-acp@latest", "npx", [ - "-y", - "@zed-industries/codex-acp@latest", - ]) - } - } -} - pub fn resolve_acp_command( - provider: Provider, + _provider: Provider, override_command: Option<&str>, ) -> Result { if let Some(raw) = override_command { @@ -108,7 +79,7 @@ pub fn resolve_acp_command( return parse_acp_command(trimmed); } - Ok(default_acp_command(provider)) + Err(AcpCommandError::MissingOverride) } fn parse_acp_command(raw: &str) -> Result { @@ -159,19 +130,6 @@ fn reject_non_stdio_json_transport(raw: &str) -> Result<(), AcpCommandError> { } } -fn command_from_parts( - display: impl Into, - program: impl Into, - args: [&str; N], -) -> AcpCommand { - AcpCommand { - display: display.into(), - program: program.into(), - args: args.into_iter().map(str::to_string).collect(), - env: HashMap::new(), - } -} - #[cfg(test)] mod tests { use std::path::Path; @@ -181,35 +139,11 @@ mod tests { use super::*; #[test] - fn default_command_for_anthropic_uses_zed_claude_acp() { - assert_eq!( - default_acp_command(Provider::Anthropic).to_string(), - "npx -y @zed-industries/claude-code-acp@latest" - ); - } - - #[test] - fn default_command_for_openai_compatible_family_uses_zed_codex_acp() { - for provider in [ - Provider::OpenAi, - Provider::Kimi, - Provider::Zai, - Provider::Minimax, - Provider::Inception, - Provider::OpenAiCompatible, - ] { - assert_eq!( - default_acp_command(provider).to_string(), - "npx -y @zed-industries/codex-acp@latest" - ); - } - } - - #[test] - fn default_command_for_gemini_uses_experimental_acp() { - assert_eq!( - default_acp_command(Provider::Gemini).to_string(), - "npx -y -- @google/gemini-cli@latest --experimental-acp" + fn missing_acp_command_is_rejected() { + let err = resolve_acp_command(Provider::OpenAi, None).unwrap_err(); + assert!( + err.to_string() + .contains("acp_command is required for backend=\"acp\"") ); } diff --git a/lib/crates/fabro-acp/src/lib.rs b/lib/crates/fabro-acp/src/lib.rs index 33c3abacb..ec6fab0c9 100644 --- a/lib/crates/fabro-acp/src/lib.rs +++ b/lib/crates/fabro-acp/src/lib.rs @@ -7,6 +7,6 @@ pub mod test_support; mod transport; -pub use command::{AcpCommand, AcpCommandError, default_acp_command, resolve_acp_command}; +pub use command::{AcpCommand, AcpCommandError, resolve_acp_command}; pub use error::AcpError; pub use session::{AcpRunRequest, AcpRunResult, run_acp_turn}; diff --git a/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs b/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs index a4d7d1b46..5eb37ff79 100644 --- a/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs @@ -472,6 +472,7 @@ mod tests { use fabro_agent::{AgentEvent, SandboxEvent}; use fabro_llm::types::TokenCounts; use fabro_model::{ModelRef, Provider}; + use fabro_types::run_event::CliEnsureCompletedProps; use fabro_types::{ MetadataSnapshotFailureKind, MetadataSnapshotPhase, ParallelBranchId, SandboxProvider, StageId, fixtures, @@ -527,6 +528,24 @@ mod tests { ui.handle_event(&stored); } + fn emit_body(ui: &mut ProgressUI, body: fabro_types::EventBody) { + ui.handle_event(&RunEvent { + id: "evt_legacy".to_string(), + ts: Utc::now(), + run_id: fixtures::RUN_1, + node_id: None, + node_label: None, + stage_id: None, + parallel_group_id: None, + parallel_branch_id: None, + session_id: None, + parent_session_id: None, + tool_call_id: None, + actor: None, + body, + }); + } + fn agent_event(stage: &str, event: AgentEvent) -> Event { Event::Agent { stage: stage.into(), @@ -860,13 +879,16 @@ mod tests { }); emit(&mut ui, Event::SetupStarted { command_count: 2 }); emit(&mut ui, Event::SetupCompleted { duration_ms: 8200 }); - emit(&mut ui, Event::CliEnsureCompleted { - cli_name: "gh".into(), - provider: "github".into(), - already_installed: false, - node_installed: false, - duration_ms: 600, - }); + emit_body( + &mut ui, + fabro_types::EventBody::CliEnsureCompleted(CliEnsureCompletedProps { + cli_name: "gh".into(), + provider: "github".into(), + already_installed: false, + node_installed: false, + duration_ms: 600, + }), + ); emit(&mut ui, Event::DevcontainerResolved { dockerfile_lines: 24, environment_count: 3, diff --git a/lib/crates/fabro-validate/src/rules/backend_valid.rs b/lib/crates/fabro-validate/src/rules/backend_valid.rs index 41489c762..61abba54b 100644 --- a/lib/crates/fabro-validate/src/rules/backend_valid.rs +++ b/lib/crates/fabro-validate/src/rules/backend_valid.rs @@ -1,4 +1,4 @@ -use fabro_graphviz::graph::{AttrValue, Graph}; +use fabro_graphviz::graph::{AttrValue, Graph, Node}; use fabro_types::LlmBackend; use crate::{Diagnostic, LintRule, Severity}; @@ -18,18 +18,36 @@ impl LintRule for Rule { let mut diagnostics = Vec::new(); for node in graph.nodes.values() { if let Some(backend) = node.attrs.get("backend").and_then(AttrValue::as_str) { - if backend.parse::().is_err() { - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!( - "unsupported LLM backend \"{backend}\"; expected one of: {}", - LlmBackend::EXPECTED - ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!("Use one of: {}", LlmBackend::EXPECTED)), - }); + match backend.parse::() { + Err(_) => { + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: format!( + "unsupported LLM backend \"{backend}\"; expected one of: {}", + LlmBackend::EXPECTED + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!("Use one of: {}", LlmBackend::EXPECTED)), + }); + } + Ok(LlmBackend::Acp) if acp_command_missing(node) => { + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: "backend=\"acp\" requires acp_command because Fabro does \ + not install ACP agents" + .to_string(), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Set acp_command to a stdio ACP command available in the sandbox" + .to_string(), + ), + }); + } + Ok(_) => {} } } } @@ -37,6 +55,13 @@ impl LintRule for Rule { } } +fn acp_command_missing(node: &Node) -> bool { + match node.attrs.get("acp_command").and_then(AttrValue::as_str) { + Some(command) => command.trim().is_empty(), + None => true, + } +} + #[cfg(test)] mod tests { use fabro_graphviz::graph::{AttrValue, Node}; @@ -46,8 +71,8 @@ mod tests { use crate::{LintRule, Severity}; #[test] - fn backend_valid_accepts_absent_api_cli_and_acp() { - for backend in [None, Some("api"), Some("cli"), Some("acp")] { + fn backend_valid_accepts_absent_api_and_cli() { + for backend in [None, Some("api"), Some("cli")] { let mut graph = minimal_graph(); let mut node = Node::new("work"); if let Some(backend) = backend { @@ -81,4 +106,35 @@ mod tests { .contains("unsupported LLM backend \"codex\"; expected one of: api, cli, acp") ); } + + #[test] + fn backend_valid_requires_acp_command_for_acp_backend() { + let mut graph = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + graph.nodes.insert("work".to_string(), node); + + let diagnostics = Rule.apply(&graph); + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].severity, Severity::Error); + assert!(diagnostics[0].message.contains( + "backend=\"acp\" requires acp_command because Fabro does not install ACP agents" + )); + } + + #[test] + fn backend_valid_accepts_acp_backend_with_acp_command() { + let mut graph = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + node.attrs.insert( + "acp_command".to_string(), + AttrValue::String("agent-acp".to_string()), + ); + graph.nodes.insert("work".to_string(), node); + + assert!(Rule.apply(&graph).is_empty()); + } } diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index 58998f26e..c05135c1a 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -986,38 +986,6 @@ fn event_body_from_event(event: &Event) -> EventBody { to_model: to_model.clone(), error: error.clone(), }), - Event::CliEnsureStarted { cli_name, provider } => { - EventBody::CliEnsureStarted(fabro_types::CliEnsureStartedProps { - cli_name: cli_name.clone(), - provider: provider.clone(), - }) - } - Event::CliEnsureCompleted { - cli_name, - provider, - already_installed, - node_installed, - duration_ms, - } => EventBody::CliEnsureCompleted(fabro_types::CliEnsureCompletedProps { - cli_name: cli_name.clone(), - provider: provider.clone(), - already_installed: *already_installed, - node_installed: *node_installed, - duration_ms: *duration_ms, - }), - Event::CliEnsureFailed { - cli_name, - provider, - error, - duration_ms, - exec_output_tail, - } => EventBody::CliEnsureFailed(fabro_types::CliEnsureFailedProps { - cli_name: cli_name.clone(), - provider: provider.clone(), - error: error.clone(), - duration_ms: *duration_ms, - exec_output_tail: exec_output_tail.clone(), - }), Event::CommandStarted { script, command, diff --git a/lib/crates/fabro-workflow/src/event/events.rs b/lib/crates/fabro-workflow/src/event/events.rs index eac03f337..f2067c5f4 100644 --- a/lib/crates/fabro-workflow/src/event/events.rs +++ b/lib/crates/fabro-workflow/src/event/events.rs @@ -495,25 +495,6 @@ pub enum Event { to_model: String, error: String, }, - CliEnsureStarted { - cli_name: String, - provider: String, - }, - CliEnsureCompleted { - cli_name: String, - provider: String, - already_installed: bool, - node_installed: bool, - duration_ms: u64, - }, - CliEnsureFailed { - cli_name: String, - provider: String, - error: String, - duration_ms: u64, - #[serde(default, skip_serializing_if = "Option::is_none")] - exec_output_tail: Option, - }, CommandStarted { node_id: String, script: String, @@ -1268,48 +1249,6 @@ impl Event { "LLM provider failover" ); } - Self::CliEnsureStarted { - cli_name, provider, .. - } => { - debug!(cli_name, provider, "CLI ensure started"); - } - Self::CliEnsureCompleted { - cli_name, - provider, - already_installed, - node_installed, - duration_ms, - } => { - info!( - cli_name, - provider, - already_installed, - node_installed, - duration_ms, - "CLI ensure completed" - ); - } - Self::CliEnsureFailed { - cli_name, - provider, - error, - duration_ms, - exec_output_tail, - } => { - let tail = fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); - error!( - cli_name, - provider, - error, - duration_ms, - exec_output_tail_present = tail.present, - exec_stdout_tail_bytes = tail.stdout_bytes, - exec_stderr_tail_bytes = tail.stderr_bytes, - exec_stdout_truncated = tail.stdout_truncated, - exec_stderr_truncated = tail.stderr_truncated, - "CLI ensure failed" - ); - } Self::CommandStarted { node_id, language, diff --git a/lib/crates/fabro-workflow/src/event/names.rs b/lib/crates/fabro-workflow/src/event/names.rs index 45b8aa0db..1607975f5 100644 --- a/lib/crates/fabro-workflow/src/event/names.rs +++ b/lib/crates/fabro-workflow/src/event/names.rs @@ -121,9 +121,6 @@ pub fn event_name(event: &Event) -> &'static str { Event::ArtifactCaptured { .. } => "artifact.captured", Event::SshAccessReady { .. } => "ssh.ready", Event::Failover { .. } => "agent.failover", - Event::CliEnsureStarted { .. } => "cli.ensure.started", - Event::CliEnsureCompleted { .. } => "cli.ensure.completed", - Event::CliEnsureFailed { .. } => "cli.ensure.failed", Event::CommandStarted { .. } => "command.started", Event::CommandCompleted { .. } => "command.completed", Event::AgentCliStarted { .. } => "agent.cli.started", diff --git a/lib/crates/fabro-workflow/src/handler/llm/acp.rs b/lib/crates/fabro-workflow/src/handler/llm/acp.rs index d074fd8f1..fdae41a76 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/acp.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use std::sync::Arc; use async_trait::async_trait; -use fabro_acp::{AcpError, AcpRunRequest, default_acp_command, resolve_acp_command}; +use fabro_acp::{AcpCommandError, AcpError, AcpRunRequest, resolve_acp_command}; use fabro_agent::{Sandbox, StaticEnvProvider, ToolEnvProvider}; use fabro_auth::CredentialResolver; use fabro_graphviz::graph::Node; @@ -13,9 +13,9 @@ use fabro_util::time::elapsed_ms; use tokio_util::sync::CancellationToken; use super::super::agent::{CodergenBackend, CodergenResult}; +use super::changed_files; use super::cli::AgentCli; use super::launch_env::{AgentLaunchEnvRequest, resolve_agent_launch_env}; -use super::{changed_files, node_runtime}; use crate::context::Context; use crate::error::Error; use crate::event::{Emitter, Event, StageScope}; @@ -83,19 +83,10 @@ impl AgentAcpBackend { .provider() .and_then(|value| value.parse::().ok()) .unwrap_or(self.provider); - let explicit_command = node.acp_command(); - let command = resolve_acp_command(provider, explicit_command) - .map_err(|err| Error::handler_with_source("Failed to resolve ACP command", &err))?; + let command = resolve_acp_command(provider, node.acp_command()) + .map_err(acp_command_error_to_workflow)?; - let node_runtime_env = if explicit_command.is_none() - && command.program() == default_acp_command(provider).program() - { - Some(node_runtime::ensure_node_runtime(sandbox, &cancel_token).await?) - } else { - None - }; - - let mut launch_env = resolve_agent_launch_env(AgentLaunchEnvRequest { + let launch_env = resolve_agent_launch_env(AgentLaunchEnvRequest { provider, cli: AgentCli::for_provider(provider), resolver: self.resolver.as_ref(), @@ -107,9 +98,6 @@ impl AgentAcpBackend { cancel_token: &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 @@ -260,6 +248,21 @@ fn stop_reason_to_string(stop_reason: &(impl serde::Serialize + std::fmt::Debug) .unwrap_or_else(|| format!("{stop_reason:?}")) } +fn acp_command_error_to_workflow(error: AcpCommandError) -> Error { + match error { + AcpCommandError::EmptyOverride => Error::handler("acp_command must not be empty"), + AcpCommandError::MissingOverride => Error::handler( + "acp_command is required for backend=\"acp\" because Fabro does not install ACP agents", + ), + AcpCommandError::UnsupportedTransport => { + Error::handler("only stdio ACP commands are supported") + } + AcpCommandError::Parse(source) => { + Error::handler_with_source("Failed to resolve ACP command", &source) + } + } +} + fn acp_error_to_workflow(error: AcpError) -> Error { match error { AcpError::Cancelled => Error::Cancelled, @@ -515,10 +518,8 @@ mod tests { } #[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(); + async fn acp_backend_requires_explicit_acp_command() { + let sandbox = MockSandbox::linux(); let sandbox = Arc::new(sandbox); let sandbox_dyn: Arc = sandbox.clone(); @@ -543,20 +544,20 @@ mod tests { CancellationToken::new(), ) .await; + let Err(err) = result else { + panic!("ACP without acp_command should fail"); + }; assert!( - result.is_err(), - "mock stdio transport should not complete ACP" + err.to_string() + .contains("acp_command is required for backend=\"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") + assert!( + sandbox + .captured_env_vars + .lock() + .expect("captured env lock poisoned") + .is_none(), + "ACP process should not launch when acp_command is missing" ); } diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index 681df81cb..e5bd49f85 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -40,8 +40,8 @@ fn cli_failure_detail(stdout: &str, stderr: &str, command: &str) -> String { use super::super::agent::{CodergenBackend, CodergenResult}; use super::acp::AgentAcpBackend; +use super::changed_files; use super::launch_env::{AgentLaunchEnvRequest, resolve_agent_launch_env}; -use super::{changed_files, node_runtime}; use crate::context::Context; use crate::error::Error; use crate::event::{Emitter, Event, StageScope}; @@ -76,41 +76,20 @@ impl AgentCli { Self::Gemini => "gemini", } } - - pub fn npm_package(self) -> &'static str { - match self { - Self::Claude => "@anthropic-ai/claude-code", - Self::Codex => "@openai/codex", - Self::Gemini => "@anthropic-ai/gemini-cli", - } - } } -/// Ensure the CLI tool for the given provider is installed in the sandbox. -/// -/// Checks if the CLI binary exists; if not, installs Node.js (if missing) and -/// the CLI via npm. Emits `CliEnsure*` events for observability. -async fn ensure_cli( +/// Verify the provider CLI exists in the sandbox. Fabro does not install agent +/// CLIs at runtime; sandbox images or setup steps own tool installation. +async fn verify_cli_available( cli: AgentCli, - provider: Provider, sandbox: &Arc, - emitter: &Arc, cancel_token: &CancellationToken, ) -> Result<(), Error> { - let start = std::time::Instant::now(); let cli_name = cli.name(); - let provider_str = <&'static str>::from(provider); - emitter.emit(&Event::CliEnsureStarted { - cli_name: cli_name.to_string(), - provider: provider_str.to_string(), - }); - - // Check if the CLI is already installed (include ~/.local/bin for npm-installed - // CLIs) - let version_check = sandbox + let availability_check = sandbox .exec_command( - &format!("PATH=\"$HOME/.local/bin:$PATH\" {cli_name} --version"), + &format!("PATH=\"$HOME/.local/bin:$PATH\" command -v {cli_name}"), 30_000, None, None, @@ -118,66 +97,17 @@ async fn ensure_cli( ) .await .map_err(|e| { - Error::handler_with_source(format!("Failed to check {cli_name} version"), &e) + Error::handler_with_source(format!("Failed to check {cli_name} availability"), &e) })?; - if version_check.is_success() { - let duration_ms = elapsed_ms(start); - emitter.emit(&Event::CliEnsureCompleted { - cli_name: cli_name.to_string(), - provider: provider_str.to_string(), - already_installed: true, - node_installed: false, - duration_ms, - }); + if availability_check.is_success() { return Ok(()); } - // Install Node.js (if needed) and the CLI in a single shell so PATH persists - let install_cmd = format!( - "{} && npm install -g {}", - node_runtime::ensure_node_runtime_shell(), - cli.npm_package() - ); - let install_result = sandbox - .exec_command( - &install_cmd, - 180_000, - None, - None, - Some(cancel_token.child_token()), - ) - .await - .map_err(|e| Error::handler_with_source(format!("Failed to install {cli_name}"), &e))?; - - let node_installed = true; - if !install_result.is_success() { - let duration_ms = elapsed_ms(start); - let exec_output_tail = install_result.default_redacted_output_tail(); - let error_msg = format!( - "{cli_name} install exited with code {}", - install_result.display_exit_code() - ); - emitter.emit(&Event::CliEnsureFailed { - cli_name: cli_name.to_string(), - provider: provider_str.to_string(), - error: error_msg.clone(), - duration_ms, - exec_output_tail, - }); - return Err(Error::handler(error_msg)); - } - - let duration_ms = elapsed_ms(start); - emitter.emit(&Event::CliEnsureCompleted { - cli_name: cli_name.to_string(), - provider: provider_str.to_string(), - already_installed: false, - node_installed, - duration_ms, - }); - - Ok(()) + Err(Error::handler(format!( + "CLI backend requires '{cli_name}' to be installed in the sandbox PATH. Install it in the \ + sandbox image or setup steps before running backend=\"cli\"." + ))) } /// Models that are only available through CLI tools (not via API). @@ -492,9 +422,8 @@ impl CodergenBackend for AgentCliBackend { .and_then(|s| s.parse::().ok()) .unwrap_or(self.provider); - // Ensure the CLI tool is installed in the sandbox let cli = AgentCli::for_provider(provider); - ensure_cli(cli, provider, sandbox, emitter, &cancel_token).await?; + verify_cli_available(cli, sandbox, &cancel_token).await?; let command = cli_command_for_provider(provider, model, &prompt_path); let stage_scope = StageScope::for_handler(context, &node.id); @@ -928,14 +857,7 @@ mod tests { assert_eq!(AgentCli::Gemini.name(), "gemini"); } - #[test] - fn agent_cli_npm_package() { - assert_eq!(AgentCli::Claude.npm_package(), "@anthropic-ai/claude-code"); - assert_eq!(AgentCli::Codex.npm_package(), "@openai/codex"); - assert_eq!(AgentCli::Gemini.npm_package(), "@anthropic-ai/gemini-cli"); - } - - // -- ensure_cli -- + // -- verify_cli_available -- use std::collections::VecDeque; use std::sync::Mutex; @@ -1072,113 +994,47 @@ mod tests { } #[tokio::test] - async fn ensure_cli_skips_install_when_present() { + async fn verify_cli_available_succeeds_when_present() { let commands = Arc::new(Mutex::new(Vec::new())); let sandbox: Arc = Arc::new(CliMockSandbox::new( vec![ok_result()], Arc::clone(&commands), )); - let emitter = Arc::new(Emitter::default()); - - let result = ensure_cli( - AgentCli::Claude, - Provider::Anthropic, - &sandbox, - &emitter, - &CancellationToken::new(), - ) - .await; + let result = + verify_cli_available(AgentCli::Claude, &sandbox, &CancellationToken::new()).await; assert!(result.is_ok()); let commands = commands.lock().unwrap(); assert_eq!(commands.len(), 1); - assert!(commands[0].contains("claude --version")); + assert!(commands[0].contains("command -v claude")); } #[tokio::test] - async fn ensure_cli_installs_when_missing() { + async fn verify_cli_available_fails_when_missing_without_installing() { let commands = Arc::new(Mutex::new(Vec::new())); - // version check fails, combined install succeeds let sandbox: Arc = Arc::new(CliMockSandbox::new( - vec![ - fail_result(127), // claude --version - ok_result(), // combined node + npm install - ], + vec![fail_result(127)], Arc::clone(&commands), )); - let emitter = Arc::new(Emitter::default()); - let result = ensure_cli( - AgentCli::Claude, - Provider::Anthropic, - &sandbox, - &emitter, - &CancellationToken::new(), - ) - .await; - assert!(result.is_ok()); + let result = + verify_cli_available(AgentCli::Claude, &sandbox, &CancellationToken::new()).await; + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("CLI backend requires 'claude' to be installed") + ); let commands = commands.lock().unwrap(); - assert_eq!(commands.len(), 2); - assert!(commands[1].contains("npm install -g @anthropic-ai/claude-code")); - } - - #[tokio::test] - async fn ensure_cli_fails_on_install_failure() { - let commands = Arc::new(Mutex::new(Vec::new())); - let sandbox: Arc = Arc::new(CliMockSandbox::new( - vec![ - fail_result(127), // claude --version - fail_result_with_output(1, "install stdout detail", "install stderr detail"), - ], - Arc::clone(&commands), - )); - let emitter = Arc::new(Emitter::default()); - let events = Arc::new(Mutex::new(Vec::::new())); - emitter.on_event({ - let events = Arc::clone(&events); - move |event| events.lock().unwrap().push(event.clone()) - }); - - let result = ensure_cli( - AgentCli::Claude, - Provider::Anthropic, - &sandbox, - &emitter, - &CancellationToken::new(), - ) - .await; - assert!(result.is_err()); - let error = result.unwrap_err().to_string(); - assert!(error.contains("install exited with code 1")); - assert!(!error.contains("install stdout detail")); - assert!(!error.contains("install stderr detail")); - - let events = events.lock().unwrap(); - let failed = events - .iter() - .find(|event| event.event_name() == "cli.ensure.failed") - .expect("cli ensure failed event"); - match &failed.body { - fabro_types::EventBody::CliEnsureFailed(props) => { - assert_eq!(props.error, "claude install exited with code 1"); - assert_eq!( - props - .exec_output_tail - .as_ref() - .and_then(|tail| tail.stdout.as_deref()), - Some("install stdout detail") - ); - assert_eq!( - props - .exec_output_tail - .as_ref() - .and_then(|tail| tail.stderr.as_deref()), - Some("install stderr detail") - ); - } - other => panic!("expected cli ensure failed body, got {other:?}"), - } + assert_eq!(commands.len(), 1); + assert!(commands[0].contains("command -v claude")); + assert!( + !commands + .iter() + .any(|command| command.contains("npm install")) + ); } // -- Cycle 1: cli_command_for_provider -- @@ -1637,8 +1493,8 @@ for line in sys.stdin: _cancel_token: Option, ) -> fabro_sandbox::Result { self.commands.lock().unwrap().push(command.to_string()); - // Default: success for git/version/cat/rm/ls. - if command.contains("--version") { + // Default: success for CLI availability checks and lightweight setup. + if command.contains("command -v ") { return Ok(ok_result()); } Ok(ExecResult { diff --git a/lib/crates/fabro-workflow/src/handler/llm/mod.rs b/lib/crates/fabro-workflow/src/handler/llm/mod.rs index 0669e91bd..5ab83cc61 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/mod.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/mod.rs @@ -4,7 +4,6 @@ pub mod api; pub mod changed_files; pub mod cli; pub mod launch_env; -pub mod node_runtime; pub mod preamble; pub use acp::AgentAcpBackend; diff --git a/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs b/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs deleted file mode 100644 index 8b1398051..000000000 --- a/lib/crates/fabro-workflow/src/handler/llm/node_runtime.rs +++ /dev/null @@ -1,124 +0,0 @@ -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 { - r#"export PATH="$HOME/.local/bin:$PATH" && \ -if node --version >/dev/null 2>&1 && npm --version >/dev/null 2>&1 && npx --version >/dev/null 2>&1; then \ - true; \ -else \ - os="$(uname -s)"; \ - if [ "$os" != "Linux" ]; then \ - echo "Node.js, npm, and npx are required for default ACP/CLI commands on $os" >&2; \ - exit 127; \ - fi; \ - arch="$(uname -m)"; \ - case "$arch" in \ - x86_64|amd64) node_arch="x64" ;; \ - aarch64|arm64) node_arch="arm64" ;; \ - *) echo "Unsupported Linux architecture for Node.js install: $arch" >&2; exit 127 ;; \ - esac; \ - mkdir -p "$HOME/.local" && \ - curl -fsSL "https://nodejs.org/dist/v22.14.0/node-v22.14.0-linux-${node_arch}.tar.gz" | tar -xz --strip-components=1 -C "$HOME/.local"; \ -fi"# - .to_string() -} - -pub async fn ensure_node_runtime( - sandbox: &Arc, - cancel_token: &CancellationToken, -) -> Result { - let command = format!( - "{} && printf '\\n{}%s\\n' \"$PATH\"", - ensure_node_runtime_shell(), - NODE_RUNTIME_PATH_MARKER - ); - let result = sandbox - .exec_command( - &command, - 180_000, - None, - None, - Some(cancel_token.child_token()), - ) - .await - .map_err(|err| Error::handler_with_source("Failed to ensure Node runtime", &err))?; - - if result.is_success() { - 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 {}", - result.display_exit_code() - ))) - } -} - -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") - ); - } -} diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index e13d7404a..7a451e09a 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -9692,11 +9692,10 @@ impl fabro_agent::Sandbox for CliTestEnv { }); } - // CLI version check during ensure_cli — return success so install path - // is skipped. - if command.contains("--version") { + // CLI availability check. + if command.contains("command -v ") { return Ok(fabro_agent::ExecResult { - stdout: "1.0.0\n".into(), + stdout: "/usr/local/bin/agent-cli\n".into(), stderr: String::new(), exit_code: Some(0), @@ -10005,10 +10004,10 @@ async fn cli_backend_run_fails_on_nonzero_exit() { duration_ms: 0, }); } - // CLI version check during ensure_cli — pretend already installed. - if command.contains("--version") { + // CLI availability check. + if command.contains("command -v ") { return Ok(fabro_agent::ExecResult { - stdout: "1.0.0\n".into(), + stdout: "/usr/local/bin/agent-cli\n".into(), stderr: String::new(), exit_code: Some(0),