From 29b7cc0de0b9661cd01a6aaa37752652882e76be Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Mon, 18 May 2026 10:20:56 -0700 Subject: [PATCH] feat(workflow): enforce strict api/acp backends (#307) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This PR makes agent execution a strict two-backend contract: API-backed stages use Fabro-owned model/provider auth, while ACP-backed stages launch a user-supplied stdio process that owns its own auth and tools. That removes the legacy CLI backend and prevents ACP execution from accidentally resolving or forwarding provider credentials. ## Changes - Replaces the old `api`/`cli`/`acp` backend model with `AgentBackend { api, acp }`, with `backend=\"cli\"` rejected and migrated toward explicit ACP process configuration. - Splits ACP process configuration into `acp.command` for shell command strings and `acp.config` for JSON stdio configs, while rejecting legacy `acp_command`. - Restricts ACP to `agent` nodes and rejects API-only attributes such as `model`, `provider`, `reasoning_effort`, `max_tokens`, and `speed` on ACP nodes. - Deletes the workflow CLI runtime, CLI credential resolver surface, CLI live smoke tests, and `agent.cli.*` event handling. - Updates ACP events and projections to report process identity (`command`, optional `config_name`) rather than provider/model metadata. - Updates import/stylesheet propagation, CLI workflow smoke coverage, server steering tests, and web model extraction for the new event/backend contract. ## Validation - `cargo check -p fabro-auth -p fabro-acp -p fabro-workflow -p fabro-cli --all-targets` - `cargo nextest run -p fabro-auth -p fabro-acp -p fabro-validate -p fabro-store -p fabro-workflow --lib` - `cargo nextest run -p fabro-acp` - `cargo nextest run -p fabro-cli --test it workflow::acp::acp_backend_workflow` - `cargo nextest run -p fabro-workflow --test it codergen_without_backend_simulated` - `cargo nextest run -p fabro-workflow --test it import_e2e_through_engine` - `cargo nextest run -p fabro-workflow --test it stylesheet_application` - `cargo nextest run -p fabro-server steer_with_active_acp_stage_returns_non_steerable_conflict` - `cargo nextest run -p fabro-server active_acp_stage_marker_clears_on_terminal_paths` - `cargo nextest run -p fabro-types agent_backend_accepts_only_api_and_acp` - `cd apps/fabro-web && bun test app/routes/run-stages.test.ts` - `cd apps/fabro-web && bun run typecheck` - `cargo +nightly-2026-04-14 fmt --check --all` - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` --- [![Compound Engineering](https://img.shields.io/badge/Compound_Engineering-6366f1)](https://github.com/EveryInc/compound-engineering-plugin) 🤖 Generated with GPT-5 via [Codex](https://openai.com/codex) --------- Co-authored-by: Peter Bell <4843+PeterBell@users.noreply.github.com> --- Cargo.lock | 7 +- apps/fabro-web/app/routes/run-stages.test.ts | 3 +- apps/fabro-web/app/routes/run-stages.tsx | 1 - docs/public/agents/subagents.mdx | 2 +- docs/public/agents/tools.mdx | 2 +- docs/public/core-concepts/agents.mdx | 72 +- docs/public/reference/dot-language.mdx | 5 +- docs/public/workflows/imports.mdx | 3 +- lib/crates/fabro-acp/Cargo.toml | 29 +- lib/crates/fabro-acp/src/command.rs | 276 ++- lib/crates/fabro-acp/src/lib.rs | 8 +- lib/crates/fabro-acp/src/session.rs | 4 +- lib/crates/fabro-acp/src/test_support.rs | 13 + lib/crates/fabro-acp/src/transport.rs | 6 +- lib/crates/fabro-acp/tests/session.rs | 10 +- lib/crates/fabro-auth/Cargo.toml | 1 - lib/crates/fabro-auth/src/lib.rs | 4 +- lib/crates/fabro-auth/src/resolve.rs | 289 +-- lib/crates/fabro-auth/src/vault_source.rs | 1 - lib/crates/fabro-cli/tests/it/workflow/acp.rs | 122 +- lib/crates/fabro-cli/tests/it/workflow/mod.rs | 1 - .../fabro-cli/tests/it/workflow/real_cli.rs | 71 - lib/crates/fabro-server/src/server.rs | 9 +- lib/crates/fabro-server/src/server/tests.rs | 20 +- lib/crates/fabro-store/src/run_state.rs | 176 +- lib/crates/fabro-types/src/graph.rs | 16 +- lib/crates/fabro-types/src/lib.rs | 2 +- lib/crates/fabro-types/src/llm_backend.rs | 18 +- lib/crates/fabro-types/src/run_event/misc.rs | 40 +- lib/crates/fabro-types/src/run_event/mod.rs | 16 - lib/crates/fabro-validate/Cargo.toml | 1 + .../fabro-validate/src/rules/backend_valid.rs | 391 +++- .../fabro-workflow/src/event/convert.rs | 114 +- lib/crates/fabro-workflow/src/event/events.rs | 74 +- lib/crates/fabro-workflow/src/event/names.rs | 4 - .../fabro-workflow/src/event/stored_fields.rs | 4 - .../fabro-workflow/src/handler/llm/acp.rs | 344 ++-- .../fabro-workflow/src/handler/llm/cli.rs | 1568 ----------------- .../src/handler/llm/launch_env.rs | 121 -- .../fabro-workflow/src/handler/llm/mod.rs | 5 +- .../fabro-workflow/src/handler/llm/router.rs | 148 ++ .../fabro-workflow/src/handler/llm/routing.rs | 33 +- .../fabro-workflow/src/operations/fork.rs | 11 +- .../fabro-workflow/src/pipeline/initialize.rs | 42 +- .../fabro-workflow/src/transforms/import.rs | 7 +- .../src/transforms/stylesheet.rs | 6 +- .../tests/it/daytona_integration.rs | 146 -- .../fabro-workflow/tests/it/integration.rs | 1134 +----------- 48 files changed, 1124 insertions(+), 4256 deletions(-) delete mode 100644 lib/crates/fabro-cli/tests/it/workflow/real_cli.rs delete mode 100644 lib/crates/fabro-workflow/src/handler/llm/cli.rs delete mode 100644 lib/crates/fabro-workflow/src/handler/llm/launch_env.rs create mode 100644 lib/crates/fabro-workflow/src/handler/llm/router.rs diff --git a/Cargo.lock b/Cargo.lock index dfd05fcf9..110120b0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1584,20 +1584,17 @@ version = "0.237.0-nightly.0" dependencies = [ "agent-client-protocol", "agent-client-protocol-tokio", - "bytes", - "fabro-model", "fabro-sandbox", "fabro-types", "fabro-util", "futures", - "serde", "serde_json", + "shlex", "tempfile", "thiserror 2.0.18", "tokio", "tokio-util", "tracing", - "uuid", ] [[package]] @@ -1678,7 +1675,6 @@ dependencies = [ "httpmock", "serde", "serde_json", - "shlex", "tempfile", "thiserror 2.0.18", "tokio", @@ -2508,6 +2504,7 @@ dependencies = [ name = "fabro-validate" version = "0.237.0-nightly.0" dependencies = [ + "fabro-acp", "fabro-graphviz", "fabro-model", "fabro-types", diff --git a/apps/fabro-web/app/routes/run-stages.test.ts b/apps/fabro-web/app/routes/run-stages.test.ts index 11e5db24a..57f447e90 100644 --- a/apps/fabro-web/app/routes/run-stages.test.ts +++ b/apps/fabro-web/app/routes/run-stages.test.ts @@ -368,13 +368,12 @@ describe("eventsToActivity", () => { properties: { model: "claude-opus-4-5" }, }), envelope(2, { - event: "agent.cli.started", + event: "agent.session.activated", stage_id: "agent@1", node_id: "agent", properties: { provider: "anthropic", model: "claude-sonnet-4-6", - command: "claude", }, }), ]; diff --git a/apps/fabro-web/app/routes/run-stages.tsx b/apps/fabro-web/app/routes/run-stages.tsx index e41fc5b09..33d753091 100644 --- a/apps/fabro-web/app/routes/run-stages.tsx +++ b/apps/fabro-web/app/routes/run-stages.tsx @@ -490,7 +490,6 @@ export function buildThreadDnaItems( const STAGE_MODEL_EVENT_NAMES = new Set([ "stage.prompt", "agent.session.activated", - "agent.cli.started", ]); export function extractStageModel( diff --git a/docs/public/agents/subagents.mdx b/docs/public/agents/subagents.mdx index 0427b9fa8..48e821625 100644 --- a/docs/public/agents/subagents.mdx +++ b/docs/public/agents/subagents.mdx @@ -6,7 +6,7 @@ description: "Delegate subtasks to child agent sessions" An agent can spawn **sub-agents** to delegate work to independent child sessions. Each sub-agent gets its own LLM session and tool access, runs concurrently with the parent, and returns its result when finished. -Sub-agents are only available with the [API backend](/core-concepts/agents#api-backend-default) (the default). Agents using the [CLI backend](/core-concepts/agents#cli-backend) or [ACP backend](/core-concepts/agents#acp-backend) cannot spawn Fabro sub-agents. +Sub-agents are only available with the [API backend](/core-concepts/agents#api-backend-default) (the default). Agents using the [ACP backend](/core-concepts/agents#acp-backend) cannot spawn Fabro sub-agents. ## Tools diff --git a/docs/public/agents/tools.mdx b/docs/public/agents/tools.mdx index 5185cfc18..111d8adbc 100644 --- a/docs/public/agents/tools.mdx +++ b/docs/public/agents/tools.mdx @@ -6,7 +6,7 @@ description: "Built-in tools for file I/O, shell commands, search, and web acces Every agent in Fabro has access to a set of built-in tools for interacting with the codebase and environment. Tools execute inside the agent's [sandbox](/execution/environments) — whether that's the local machine, a Docker container, or a Daytona VM — so the same tool calls work identically regardless of provider. -The tools described on this page apply to the **API backend** (the default). When using the [CLI backend](/core-concepts/agents#cli-backend) or [ACP backend](/core-concepts/agents#acp-backend), the external agent process provides its own tools — Fabro's built-in tools are not used. +The tools described on this page apply to the **API backend** (the default). When using the [ACP backend](/core-concepts/agents#acp-backend), the external agent process provides its own tools — Fabro's built-in tools are not used. ## Core tools diff --git a/docs/public/core-concepts/agents.mdx b/docs/public/core-concepts/agents.mdx index 8a4331d4f..e2387e6af 100644 --- a/docs/public/core-concepts/agents.mdx +++ b/docs/public/core-concepts/agents.mdx @@ -18,7 +18,7 @@ This loop continues until the model stops calling tools, indicating it considers ## Backends -Every agent and prompt node uses a **backend** that determines how Fabro interacts with the LLM. There are three options: +Every agent node uses a **backend** that determines how Fabro interacts with the LLM or external agent process. Prompt nodes always use the API backend. Agent nodes support two backend values: `api` and `acp`. ### API backend (default) @@ -29,66 +29,48 @@ Fabro manages the agent loop directly — it calls the LLM provider's API, execu - Provider failover - All [built-in tools](/agents/tools) and [MCP](/agents/mcp) integrations -### CLI backend - -Fabro delegates execution to a legacy external coding assistant CLI. The CLI tool manages its own tool loop internally — Fabro sends the prompt, waits for the CLI to finish, and tracks file changes via `git diff` before and after execution. - -The CLI is selected automatically based on the node's provider: - -| Provider | CLI tool | -|---|---| -| Anthropic | `claude` | -| 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 -implement [label="Implement", backend="cli"] -``` - -``` -// Stylesheet -* { backend: cli; } -``` - ### ACP backend -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. +Fabro can 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, cancellation, and file-change tracking behavior as other agent stages. -Set ACP on a node with `backend="acp"` and an explicit `acp_command`: +ACP stages do not use Fabro model/provider credentials. The ACP process owns its auth, tools, and model behavior. Configure the process with exactly one of: + +- `acp.command`: a shell command string +- `acp.config`: a JSON stdio ACP config ```dot -implement [label="Implement", backend="acp", acp_command="python3 tools/fake_acp_agent.py"] +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. +```dot +implement [ + label="Implement" + backend="acp" + acp.config="{\"type\":\"stdio\",\"name\":\"agent\",\"command\":\"python3\",\"args\":[\"tools/fake_acp_agent.py\"]}" +] +``` -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. +Fabro does not install ACP agents, Node.js, npm, or `npx` at runtime. Commands 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. + +The legacy `acp_command` attribute is rejected; use `acp.command` for shell commands or `acp.config` for JSON stdio configs. 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 -| Capability | API backend | CLI backend | ACP backend | -|---|---|---|---| -| Tools | Fabro built-in tools + MCP | CLI's own tool set | ACP agent's own tool set | -| Session caching | Supported (`fidelity` + `thread_id`) | Not supported | Agent-dependent | -| Sub-agents | Supported | Not supported | Not supported through Fabro tools | -| Provider failover | Supported | Not supported | Not supported | -| File tracking | Tool call events | `git diff` before/after | `git diff` before/after | - -### When to use the CLI backend - -- **CLI-specific tools** — leverage tool implementations built into a specific CLI (e.g. Claude Code's computer use, Codex's sandboxed execution) -- **CLI-only models** — use models that are only available through a CLI tool, not via API -- **Existing workflows** — integrate a CLI tool you already depend on without rewriting its configuration +| Capability | API backend | ACP backend | +|---|---|---| +| Tools | Fabro built-in tools + MCP | ACP agent's own tool set | +| Session caching | Supported (`fidelity` + `thread_id`) | Agent-dependent | +| Sub-agents | Supported | Not supported through Fabro tools | +| Provider failover | Supported | Not supported | +| Model/provider credentials | Resolved by Fabro | Not used by Fabro | +| File tracking | Tool call events | `git diff` before/after | ### When to use the ACP backend - **Protocol adapters** — run ACP-compatible coding agents through a stable stdio protocol - **Sandbox parity** — keep agent process execution inside Fabro's local or Docker sandbox -- **Custom agents** — use `acp_command` for a checked-in or preinstalled ACP adapter +- **Command-owned auth** — let the ACP command manage its own provider login, model selection, and credentials +- **Custom agents** — use `acp.command` or `acp.config` for a checked-in or preinstalled ACP adapter ## Tools diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 3ba7a7259..1c1656057 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -206,8 +206,9 @@ Start nodes can also be identified by ID (`start` or `Start`). Exit nodes can be | `model` | String | Explicit model ID (overrides stylesheet) | | `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 | 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. | +| `backend` | String | Agent execution backend: `api` (default) or `acp`. `api` runs Fabro's tool loop through provider APIs; `acp` runs an Agent Client Protocol stdio agent inside the active sandbox. Prompt nodes are API-only. See [Agents — Backends](/core-concepts/agents#backends). | +| `acp.command` | String | Shell command for nodes with `backend="acp"`. Mutually exclusive with `acp.config`. The value is always parsed as a command string, not JSON. | +| `acp.config` | String | JSON stdio ACP config for nodes with `backend="acp"`. Mutually exclusive with `acp.command`. | ### Command nodes diff --git a/docs/public/workflows/imports.mdx b/docs/public/workflows/imports.mdx index a3ee29906..c5eb6b0f2 100644 --- a/docs/public/workflows/imports.mdx +++ b/docs/public/workflows/imports.mdx @@ -73,7 +73,8 @@ A small set of attributes on the placeholder node propagate as **defaults** to e | `reasoning_effort` | | `speed` | | `backend` | -| `acp_command` | +| `acp.command` | +| `acp.config` | | `fidelity` | | `max_retries` | | `thread_id` | diff --git a/lib/crates/fabro-acp/Cargo.toml b/lib/crates/fabro-acp/Cargo.toml index c373c7944..3089b5c48 100644 --- a/lib/crates/fabro-acp/Cargo.toml +++ b/lib/crates/fabro-acp/Cargo.toml @@ -7,6 +7,16 @@ license.workspace = true description = "Agent Client Protocol backend support for Fabro" [features] +default = ["runtime"] +runtime = [ + "dep:fabro-sandbox", + "dep:fabro-types", + "dep:fabro-util", + "dep:futures", + "dep:tokio", + "dep:tokio-util", + "dep:tracing", +] test-support = [] [lib] @@ -18,19 +28,16 @@ workspace = true [dependencies] agent-client-protocol.workspace = true agent-client-protocol-tokio.workspace = true -fabro-model = { path = "../fabro-model" } -fabro-sandbox = { path = "../fabro-sandbox" } -fabro-types = { path = "../fabro-types" } -fabro-util = { path = "../fabro-util" } -bytes.workspace = true -serde.workspace = true +fabro-sandbox = { path = "../fabro-sandbox", optional = true } +fabro-types = { path = "../fabro-types", optional = true } +fabro-util = { path = "../fabro-util", optional = true } serde_json.workspace = true +shlex = "1" thiserror.workspace = true -tokio.workspace = true -tokio-util = { workspace = true, features = ["compat", "io"] } -futures.workspace = true -uuid.workspace = true -tracing.workspace = true +tokio = { workspace = true, optional = true } +tokio-util = { workspace = true, features = ["compat", "io"], optional = true } +futures = { workspace = true, optional = true } +tracing = { workspace = true, optional = true } [dev-dependencies] fabro-sandbox = { path = "../fabro-sandbox", features = ["test-support"] } diff --git a/lib/crates/fabro-acp/src/command.rs b/lib/crates/fabro-acp/src/command.rs index c27ecd35f..1ffae80cc 100644 --- a/lib/crates/fabro-acp/src/command.rs +++ b/lib/crates/fabro-acp/src/command.rs @@ -1,19 +1,102 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::str::FromStr; -use agent_client_protocol::schema::McpServer; +use agent_client_protocol::schema::{McpServer, McpServerStdio}; use agent_client_protocol_tokio::AcpAgent; #[derive(Debug, Clone, PartialEq, Eq)] -pub struct AcpCommand { - display: String, +pub struct AcpProcessSpec { + name: Option, program: PathBuf, args: Vec, env: HashMap, } -impl AcpCommand { +impl AcpProcessSpec { + pub fn from_attrs( + legacy_command: Option<&str>, + command: Option<&str>, + config: Option<&str>, + ) -> Result { + if legacy_command.is_some() { + return Err(AcpCommandError::LegacyCommandAttribute); + } + + match (command, config) { + (Some(command), None) => Self::from_command_attr(command), + (None, Some(config)) => Self::from_config_attr(config), + (None, None) | (Some(_), Some(_)) => Err(AcpCommandError::MissingOverride), + } + } + + pub fn from_command_attr(raw: &str) -> Result { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return Err(AcpCommandError::EmptyOverride); + } + + let parts = shlex::split(trimmed).ok_or(AcpCommandError::InvalidCommandString)?; + let agent = + AcpAgent::from_args(parts).map_err(|_| AcpCommandError::InvalidCommandString)?; + let mut spec = Self::from_server(agent.into_server())?; + spec.name = None; + Ok(spec) + } + + pub fn from_config_attr(raw: &str) -> Result { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return Err(AcpCommandError::EmptyOverride); + } + + let server = parse_config_server(trimmed)?; + Self::from_server(server) + } + + fn from_server(server: McpServer) -> Result { + match server { + McpServer::Stdio(stdio) => Self::from_stdio_config(stdio), + _ => Err(AcpCommandError::UnsupportedTransport), + } + } + + fn from_stdio_config(stdio: McpServerStdio) -> Result { + if stdio.command.as_os_str().is_empty() { + return Err(AcpCommandError::InvalidConfigShape("missing command")); + } + + let env = stdio + .env + .into_iter() + .map(|env| (env.name, env.value)) + .collect(); + Ok(Self::from_stdio_parts( + Some(stdio.name), + stdio.command, + stdio.args, + env, + )) + } + + fn from_stdio_parts( + name: Option, + program: PathBuf, + args: Vec, + env: HashMap, + ) -> Self { + Self { + name, + program, + args, + env, + } + } + + #[must_use] + pub fn name(&self) -> Option<&str> { + self.name.as_deref() + } + #[must_use] pub fn program(&self) -> &Path { &self.program @@ -29,101 +112,70 @@ impl AcpCommand { &self.env } - #[must_use] - pub fn display(&self) -> &str { - &self.display - } - #[must_use] pub fn to_shell_command(&self) -> String { render_command(&self.program, &self.args) } } -impl std::fmt::Display for AcpCommand { +impl std::fmt::Display for AcpProcessSpec { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&self.display) + f.write_str(&self.to_shell_command()) } } #[derive(Debug, thiserror::Error)] pub enum AcpCommandError { - #[error("acp_command must not be empty")] + #[error("acp_command is no longer supported; use acp.command or acp.config")] + LegacyCommandAttribute, + #[error("ACP process attribute must not be empty")] EmptyOverride, - #[error( - "acp_command is required for backend=\"acp\" because Fabro does not install ACP agents" - )] + #[error("backend=\"acp\" requires exactly one of acp.command or acp.config")] MissingOverride, #[error("only stdio ACP commands are supported")] UnsupportedTransport, - #[error("failed to parse acp_command")] - Parse(#[source] agent_client_protocol::Error), -} - -impl From for AcpCommandError { - fn from(error: agent_client_protocol::Error) -> Self { - Self::Parse(error) - } -} - -pub fn resolve_acp_command(override_command: Option<&str>) -> Result { - if let Some(raw) = override_command { - let trimmed = raw.trim(); - if trimmed.is_empty() { - return Err(AcpCommandError::EmptyOverride); - } - return parse_acp_command(trimmed); - } - - Err(AcpCommandError::MissingOverride) -} - -fn parse_acp_command(raw: &str) -> Result { - reject_non_stdio_json_transport(raw)?; - - let agent = AcpAgent::from_str(raw)?; - let McpServer::Stdio(stdio) = agent.into_server() else { - return Err(AcpCommandError::UnsupportedTransport); - }; - - let program = stdio.command; - let args = stdio.args; - let display = render_command(&program, &args); - - Ok(AcpCommand { - display, - program, - args, - env: stdio - .env - .into_iter() - .map(|env| (env.name, env.value)) - .collect(), - }) + #[error("failed to parse acp.command as a shell command")] + InvalidCommandString, + #[error("failed to parse acp.config as JSON")] + InvalidConfigJson(#[source] serde_json::Error), + #[error("invalid acp.config shape: {0}")] + InvalidConfigShape(&'static str), } fn render_command(program: &Path, args: &[String]) -> String { std::iter::once(program.to_string_lossy().into_owned()) .chain(args.iter().cloned()) - .map(|part| fabro_sandbox::shell_quote(&part)) + .map(|part| shell_quote(&part)) .collect::>() .join(" ") } -fn reject_non_stdio_json_transport(raw: &str) -> Result<(), AcpCommandError> { - let trimmed = raw.trim_start(); - if !trimmed.starts_with('{') { - return Ok(()); - } - - let Ok(value) = serde_json::from_str::(trimmed) else { - return Ok(()); - }; +fn parse_config_server(raw: &str) -> Result { + let mut value: serde_json::Value = + serde_json::from_str(raw).map_err(AcpCommandError::InvalidConfigJson)?; match value.get("type").and_then(serde_json::Value::as_str) { - Some("stdio") | None => Ok(()), - Some(_) => Err(AcpCommandError::UnsupportedTransport), + Some("stdio") | None => {} + Some(_) => return Err(AcpCommandError::UnsupportedTransport), } + + if let Some(object) = value.as_object_mut() { + object + .entry("args".to_string()) + .or_insert_with(|| serde_json::Value::Array(Vec::new())); + object + .entry("env".to_string()) + .or_insert_with(|| serde_json::Value::Array(Vec::new())); + } + + serde_json::from_value(value).map_err(AcpCommandError::InvalidConfigJson) +} + +fn shell_quote(s: &str) -> String { + shlex::try_quote(s).map_or_else( + |_| format!("'{}'", s.replace('\'', "'\\''")), + |quoted| quoted.to_string(), + ) } #[cfg(test)] @@ -133,41 +185,53 @@ mod tests { use super::*; #[test] - fn missing_acp_command_is_rejected() { - let err = resolve_acp_command(None).unwrap_err(); - assert!( - err.to_string() - .contains("acp_command is required for backend=\"acp\"") - ); - } - - #[test] - fn explicit_acp_command_overrides_provider_default() { - let command = resolve_acp_command(Some("python fake_agent.py")).unwrap(); + fn command_attr_parses_shell_command() { + let command = AcpProcessSpec::from_command_attr("python fake_agent.py").unwrap(); assert_eq!(command.to_string(), "python fake_agent.py"); + assert_eq!(command.name(), None); assert_eq!(command.program(), Path::new("python")); assert_eq!(command.args(), &["fake_agent.py".to_string()]); } #[test] - fn blank_acp_command_is_rejected() { - let err = resolve_acp_command(Some(" ")).unwrap_err(); - assert!(err.to_string().contains("acp_command must not be empty")); + fn command_attr_parses_leading_env_assignments() { + let command = AcpProcessSpec::from_command_attr( + "RUST_LOG=debug TOKEN='secret value' python fake_agent.py", + ) + .unwrap(); + + assert_eq!(command.to_string(), "python fake_agent.py"); + assert_eq!(command.program(), Path::new("python")); + assert_eq!( + command.env().get("RUST_LOG").map(String::as_str), + Some("debug") + ); + assert_eq!( + command.env().get("TOKEN").map(String::as_str), + Some("secret value") + ); } #[test] - fn json_stdio_acp_command_is_supported() { + fn blank_acp_process_attr_is_rejected() { + let err = AcpProcessSpec::from_command_attr(" ").unwrap_err(); + assert!(err.to_string().contains("must not be empty")); + } + + #[test] + fn json_stdio_acp_config_is_supported() { let raw = r#"{"type":"stdio","name":"fake","command":"python","args":["fake agent.py"],"env":[{"name":"MODE","value":"test"}]}"#; - let command = resolve_acp_command(Some(raw)).unwrap(); + let command = AcpProcessSpec::from_config_attr(raw).unwrap(); + assert_eq!(command.name(), Some("fake")); assert_eq!(command.program(), Path::new("python")); assert_eq!(command.args(), &["fake agent.py".to_string()]); assert_eq!(command.env().get("MODE").map(String::as_str), Some("test")); } #[test] - fn json_stdio_acp_command_display_omits_env_contents() { + fn json_stdio_acp_config_display_omits_env_contents() { let raw = r#"{"type":"stdio","name":"fake","command":"agent","args":["--flag","two words"],"env":[{"name":"OPENAI_API_KEY","value":"secret-key"}]}"#; - let command = resolve_acp_command(Some(raw)).unwrap(); + let command = AcpProcessSpec::from_config_attr(raw).unwrap(); assert_eq!( command.env().get("OPENAI_API_KEY").map(String::as_str), @@ -179,12 +243,40 @@ mod tests { } #[test] - fn non_stdio_acp_command_is_rejected() { + fn non_stdio_acp_config_is_rejected() { let raw = r#"{"type":"http","name":"remote","url":"https://example.test/acp"}"#; - let err = resolve_acp_command(Some(raw)).unwrap_err(); + let err = AcpProcessSpec::from_config_attr(raw).unwrap_err(); assert!( err.to_string() .contains("only stdio ACP commands are supported") ); } + + #[test] + fn command_attr_is_always_shell_command_even_when_json_shaped() { + let command = AcpProcessSpec::from_command_attr(r#"{"type":"stdio"}"#).unwrap(); + + assert_ne!(command.program(), Path::new("stdio")); + assert!(command.args().is_empty()); + } + + #[test] + fn config_attr_requires_json_stdio_config() { + let command = AcpProcessSpec::from_config_attr( + r#"{"type":"stdio","name":"fake","command":"python3","args":["agent.py"]}"#, + ) + .unwrap(); + + assert_eq!(command.name(), Some("fake")); + assert_eq!(command.program(), Path::new("python3")); + assert_eq!(command.args(), &["agent.py".to_string()]); + + assert!(AcpProcessSpec::from_config_attr("python3 agent.py").is_err()); + assert!( + AcpProcessSpec::from_config_attr( + r#"{"type":"http","name":"remote","url":"https://example.test/acp"}"# + ) + .is_err() + ); + } } diff --git a/lib/crates/fabro-acp/src/lib.rs b/lib/crates/fabro-acp/src/lib.rs index 74ba22cca..af07ab9a6 100644 --- a/lib/crates/fabro-acp/src/lib.rs +++ b/lib/crates/fabro-acp/src/lib.rs @@ -1,12 +1,18 @@ pub mod command; + +#[cfg(feature = "runtime")] pub mod error; +#[cfg(feature = "runtime")] pub mod session; #[cfg(any(test, feature = "test-support"))] pub mod test_support; +#[cfg(feature = "runtime")] mod transport; -pub use command::{AcpCommand, AcpCommandError, resolve_acp_command}; +pub use command::{AcpCommandError, AcpProcessSpec}; +#[cfg(feature = "runtime")] pub use error::{AcpError, AcpProcessExit}; +#[cfg(feature = "runtime")] pub use session::{AcpRunRequest, AcpRunResult, render_stop_reason, run_acp_turn}; diff --git a/lib/crates/fabro-acp/src/session.rs b/lib/crates/fabro-acp/src/session.rs index e0dd343b5..b52a45f28 100644 --- a/lib/crates/fabro-acp/src/session.rs +++ b/lib/crates/fabro-acp/src/session.rs @@ -14,12 +14,12 @@ use fabro_util::time::elapsed_ms; use tokio::time::{sleep, timeout}; use tokio_util::sync::CancellationToken; -use crate::command::AcpCommand; +use crate::command::AcpProcessSpec; use crate::error::AcpError; use crate::transport::{SandboxAcpTransport, TransportState}; pub struct AcpRunRequest { - pub command: AcpCommand, + pub command: AcpProcessSpec, pub prompt: String, pub cwd: String, pub timeout_ms: Option, diff --git a/lib/crates/fabro-acp/src/test_support.rs b/lib/crates/fabro-acp/src/test_support.rs index fc697731b..7ec358bc7 100644 --- a/lib/crates/fabro-acp/src/test_support.rs +++ b/lib/crates/fabro-acp/src/test_support.rs @@ -35,6 +35,19 @@ if os.environ.get("ACP_PID_RECORD"): with open(os.environ["ACP_PID_RECORD"], "w", encoding="utf-8") as record: record.write(str(os.getpid())) +if os.environ.get("ACP_ENV_RECORD"): + keys = [ + key.strip() + for key in os.environ.get( + "ACP_ENV_RECORD_KEYS", + "ANTHROPIC_API_KEY,OPENAI_API_KEY,GEMINI_API_KEY", + ).split(",") + if key.strip() + ] + snapshot = {key: os.environ[key] for key in keys if key in os.environ} + with open(os.environ["ACP_ENV_RECORD"], "w", encoding="utf-8") as record: + record.write(json.dumps(snapshot, sort_keys=True)) + def handle_sigterm(signum, frame): if os.environ.get("ACP_LINGER_TERMINATED"): with open(os.environ["ACP_LINGER_TERMINATED"], "w", encoding="utf-8") as record: diff --git a/lib/crates/fabro-acp/src/transport.rs b/lib/crates/fabro-acp/src/transport.rs index 1154f38b0..f599c5a27 100644 --- a/lib/crates/fabro-acp/src/transport.rs +++ b/lib/crates/fabro-acp/src/transport.rs @@ -20,7 +20,7 @@ use tokio::sync::Mutex as TokioMutex; use tokio::time::timeout; use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; -use crate::command::AcpCommand; +use crate::command::AcpProcessSpec; use crate::error::AcpProcessExit; const CLEAN_EXIT_PROTOCOL_GRACE: Duration = Duration::from_millis(500); @@ -89,7 +89,7 @@ impl TransportState { } pub(crate) struct SandboxAcpTransport { - command: AcpCommand, + command: AcpProcessSpec, cwd: String, env: HashMap, sandbox: Arc, @@ -98,7 +98,7 @@ pub(crate) struct SandboxAcpTransport { impl SandboxAcpTransport { pub(crate) fn new( - command: AcpCommand, + command: AcpProcessSpec, cwd: String, env: HashMap, sandbox: Arc, diff --git a/lib/crates/fabro-acp/tests/session.rs b/lib/crates/fabro-acp/tests/session.rs index e537e027b..4e987a746 100644 --- a/lib/crates/fabro-acp/tests/session.rs +++ b/lib/crates/fabro-acp/tests/session.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use std::time::Duration; use agent_client_protocol::schema::StopReason; -use fabro_acp::{AcpError, AcpRunRequest, AcpRunResult, resolve_acp_command, run_acp_turn}; +use fabro_acp::{AcpError, AcpProcessSpec, AcpRunRequest, AcpRunResult, run_acp_turn}; use fabro_sandbox::test_support::{MockSandbox, MockStdioProcess}; use fabro_sandbox::{LocalSandbox, Sandbox, shell_quote}; use fabro_util::error::collect_chain; @@ -31,7 +31,7 @@ use test_support::fake_acp_agent_script; async fn stdio_spawn_failure_returns_sandbox_error() { const SANDBOX_FAILURE: &str = "ACP backend requires bidirectional stdio; the Daytona sandbox provider does not support it yet"; - let command = resolve_acp_command(Some("fake-acp-agent")).expect("resolve ACP command"); + let command = AcpProcessSpec::from_command_attr("fake-acp-agent").expect("parse ACP command"); let mut sandbox = MockSandbox::linux(); sandbox.stdio_process_error = Some(SANDBOX_FAILURE.to_string()); let sandbox: Arc = Arc::new(sandbox); @@ -67,7 +67,7 @@ async fn clean_stdio_exit_after_final_response_completes_turn() { let sandbox = MockSandbox::linux(); sandbox.set_stdio_process(mock_acp_stdio_process("end_turn")); let sandbox: Arc = Arc::new(sandbox); - let command = resolve_acp_command(Some("mock-acp-agent")).expect("resolve ACP command"); + let command = AcpProcessSpec::from_command_attr("mock-acp-agent").expect("parse ACP command"); let result = run_acp_turn(AcpRunRequest { command, @@ -96,7 +96,7 @@ async fn session_lifecycle_initializes_sends_prompt_and_aggregates_text() { .expect("write fake ACP agent"); let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy())); - let command = resolve_acp_command(Some(&raw_command)).expect("resolve ACP command"); + let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command"); let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.path().to_path_buf())); let result = run_acp_turn(AcpRunRequest { @@ -461,7 +461,7 @@ async fn run_fake_agent_with_activity( .await .expect("write fake ACP agent"); let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy())); - let command = resolve_acp_command(Some(&raw_command)).expect("resolve ACP command"); + let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command"); let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.to_path_buf())); run_acp_turn(AcpRunRequest { diff --git a/lib/crates/fabro-auth/Cargo.toml b/lib/crates/fabro-auth/Cargo.toml index cfa60907c..992aa66a2 100644 --- a/lib/crates/fabro-auth/Cargo.toml +++ b/lib/crates/fabro-auth/Cargo.toml @@ -23,7 +23,6 @@ fabro-types = { path = "../fabro-types" } fabro-vault = { path = "../fabro-vault" } serde.workspace = true serde_json.workspace = true -shlex = "1" thiserror.workspace = true tokio.workspace = true diff --git a/lib/crates/fabro-auth/src/lib.rs b/lib/crates/fabro-auth/src/lib.rs index b04257035..c46283c4e 100644 --- a/lib/crates/fabro-auth/src/lib.rs +++ b/lib/crates/fabro-auth/src/lib.rs @@ -16,8 +16,8 @@ pub use credential_source::{CredentialSource, ResolvedCredentials}; pub use env_source::EnvCredentialSource; pub use refresh::refresh_oauth_credential; pub use resolve::{ - ApiCredential, CliAgentKind, CliCredential, CredentialResolver, CredentialUsage, EnvLookup, - ResolveError, ResolvedCredential, auth_issue_message, build_api_key_header, + ApiCredential, CredentialResolver, CredentialUsage, EnvLookup, ResolveError, + ResolvedCredential, auth_issue_message, build_api_key_header, configured_providers_from_process_env, }; pub use strategy::{ diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs index 3e08045f2..6daa97d28 100644 --- a/lib/crates/fabro-auth/src/resolve.rs +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -5,7 +5,6 @@ use fabro_model::catalog::CatalogProvider; use fabro_model::{ApiKeyHeaderPolicy, Catalog, CredentialRef, HeaderValueRef, ProviderId}; use fabro_static::EnvVars; use fabro_vault::{SecretType, Vault}; -use shlex::try_quote; use tokio::sync::RwLock as AsyncRwLock; use tokio::task::spawn_blocking; @@ -17,17 +16,9 @@ use crate::vault_ext::{VaultLookupError, vault_get_oauth, vault_get_token, vault pub type EnvLookup = Arc Option + Send + Sync>; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum CliAgentKind { - Claude, - Codex, - Gemini, -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CredentialUsage { ApiRequest, - CliAgent(CliAgentKind), } #[derive(Debug, Clone, PartialEq, Eq)] @@ -125,16 +116,9 @@ fn auth_header_for_catalog_provider( Ok(build_api_key_header(auth.header.clone(), key)) } -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct CliCredential { - pub env_vars: HashMap, - pub login_command: Option, -} - #[derive(Debug, Clone, PartialEq, Eq)] pub enum ResolvedCredential { Api(ApiCredential), - Cli(CliCredential), } #[derive(Debug, thiserror::Error)] @@ -212,14 +196,14 @@ impl CredentialResolver { pub async fn resolve( &self, provider: impl Into, - usage: CredentialUsage, + _usage: CredentialUsage, catalog: &Catalog, ) -> Result { let provider_id = provider.into(); let Some(catalog_provider) = catalog.provider(&provider_id) else { return Err(ResolveError::NotConfigured(provider_id)); }; - if usage == CredentialUsage::ApiRequest && catalog_provider.auth.is_none() { + if catalog_provider.auth.is_none() { let vault = self.vault.read().await; return self .api_credential_from_provider_auth(&vault, catalog_provider, catalog) @@ -274,14 +258,8 @@ impl CredentialResolver { }; let vault = self.vault.read().await; - match usage { - CredentialUsage::ApiRequest => self - .to_api_credential(&vault, &provider_id, &secret, catalog) - .map(ResolvedCredential::Api), - CredentialUsage::CliAgent(kind) => Ok(ResolvedCredential::Cli( - Self::to_cli_credential(&provider_id, &secret, kind, catalog), - )), - } + self.to_api_credential(&vault, &provider_id, &secret, catalog) + .map(ResolvedCredential::Api) } #[must_use] @@ -468,49 +446,6 @@ impl CredentialResolver { project_id: None, }) } - - fn to_cli_credential( - provider_id: &ProviderId, - secret: &ResolvedSecret, - kind: CliAgentKind, - catalog: &Catalog, - ) -> CliCredential { - let mut env_vars = HashMap::new(); - let is_openai = provider_id == &ProviderId::openai(); - let login_command = match (is_openai, secret, kind) { - (true, ResolvedSecret::ApiKey(key), CliAgentKind::Codex) => { - env_vars.insert(EnvVars::OPENAI_API_KEY.to_string(), key.clone()); - Some(codex_login_command(key)) - } - (true, ResolvedSecret::OAuth { credential, .. }, CliAgentKind::Codex) => { - env_vars.insert( - EnvVars::OPENAI_API_KEY.to_string(), - credential.tokens.access_token.clone(), - ); - if let Some(account_id) = &credential.account_id { - env_vars.insert(EnvVars::CHATGPT_ACCOUNT_ID.to_string(), account_id.clone()); - } - Some(codex_login_command(&credential.tokens.access_token)) - } - (_, ResolvedSecret::ApiKey(key), _) => { - if let Some(name) = primary_api_key_env_var(provider_id, catalog) { - env_vars.insert(name.to_string(), key.clone()); - } - None - } - (_, ResolvedSecret::OAuth { credential, .. }, _) => { - if let Some(name) = primary_api_key_env_var(provider_id, catalog) { - env_vars.insert(name.to_string(), credential.tokens.access_token.clone()); - } - None - } - }; - - CliCredential { - env_vars, - login_command, - } - } } fn vault_lookup_error(provider: &ProviderId, name: &str, err: VaultLookupError) -> ResolveError { @@ -545,32 +480,8 @@ pub async fn configured_providers_from_process_env( } } } -fn primary_api_key_env_var<'a>(provider: &ProviderId, catalog: &'a Catalog) -> Option<&'a str> { - catalog - .provider(provider)? - .auth - .as_ref()? - .credentials - .iter() - .find_map(|credential_ref| match credential_ref { - CredentialRef::Env(name) => Some(name.as_str()), - CredentialRef::Vault(_) => None, - }) -} - -fn codex_login_command(api_key: &str) -> String { - let quoted = - try_quote(api_key).map_or_else(|_| api_key.to_string(), std::borrow::Cow::into_owned); - format!( - "export PATH=\"$HOME/.local/bin:$PATH\" && printf '%s\\n' {quoted} | codex login --with-api-key" - ) -} - #[cfg(test)] mod tests { - #[cfg(unix)] - use std::os::unix::fs::PermissionsExt; - use chrono::{Duration, Utc}; use fabro_model::catalog::LlmCatalogSettings; use httpmock::Method::POST; @@ -628,9 +539,7 @@ mod tests { .await .unwrap(); - let ResolvedCredential::Api(api) = resolved else { - panic!("expected api credential"); - }; + let ResolvedCredential::Api(api) = resolved; assert_eq!( api.auth_header, Some(ApiKeyHeader::Bearer("env-key".to_string())) @@ -658,9 +567,7 @@ mod tests { .await .unwrap(); - let ResolvedCredential::Api(api) = resolved else { - panic!("expected api credential"); - }; + let ResolvedCredential::Api(api) = resolved; assert_eq!( api.auth_header, Some(ApiKeyHeader::Bearer("expired-access".to_string())) @@ -702,17 +609,15 @@ mod tests { let resolver = test_resolver(vault, Arc::new(|_| None)); let catalog = default_catalog(); - let ResolvedCredential::Api(api) = resolver + let resolved = resolver .resolve( ProviderId::anthropic(), CredentialUsage::ApiRequest, &catalog, ) .await - .unwrap() - else { - panic!("expected api credential"); - }; + .unwrap(); + let ResolvedCredential::Api(api) = resolved; assert_eq!( api.auth_header, @@ -764,9 +669,7 @@ reasoning = false .await .unwrap(); - let ResolvedCredential::Api(api) = resolved else { - panic!("expected api credential"); - }; + let ResolvedCredential::Api(api) = resolved; assert_eq!( api.auth_header, Some(ApiKeyHeader::Bearer("compat-key".to_string())) @@ -777,141 +680,6 @@ reasoning = false ); } - #[tokio::test] - async fn openai_codex_cli_credential_includes_login_command_and_account_id() { - let dir = tempfile::tempdir().unwrap(); - let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault_set_oauth( - &mut vault, - crate::OPENAI_CODEX_VAULT_SECRET_NAME, - &oauth_credential( - "https://auth.openai.com/oauth/token".to_string(), - Utc::now() + Duration::hours(1), - ), - ) - .unwrap(); - let resolver = test_resolver(vault, Arc::new(|_| None)); - let catalog = default_catalog(); - - let ResolvedCredential::Cli(cli) = resolver - .resolve( - ProviderId::openai(), - CredentialUsage::CliAgent(CliAgentKind::Codex), - &catalog, - ) - .await - .unwrap() - else { - panic!("expected cli credential"); - }; - - assert_eq!( - cli.env_vars.get("OPENAI_API_KEY").map(String::as_str), - Some("expired-access") - ); - assert_eq!( - cli.env_vars.get("CHATGPT_ACCOUNT_ID").map(String::as_str), - Some("acct_123") - ); - assert!( - cli.login_command - .as_deref() - .is_some_and(|command| command.contains("codex login --with-api-key")) - ); - } - - #[tokio::test] - async fn openai_api_key_cli_fallback_has_no_account_id() { - let dir = tempfile::tempdir().unwrap(); - let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault_set_token(&mut vault, "OPENAI_API_KEY", "openai-key").unwrap(); - let resolver = test_resolver(vault, Arc::new(|_| None)); - let catalog = default_catalog(); - - let ResolvedCredential::Cli(cli) = resolver - .resolve( - ProviderId::openai(), - CredentialUsage::CliAgent(CliAgentKind::Codex), - &catalog, - ) - .await - .unwrap() - else { - panic!("expected cli credential"); - }; - - assert_eq!( - cli.env_vars.get("OPENAI_API_KEY").map(String::as_str), - Some("openai-key") - ); - assert!(!cli.env_vars.contains_key("CHATGPT_ACCOUNT_ID")); - assert!(cli.login_command.is_some()); - } - - #[cfg(unix)] - #[tokio::test] - #[expect( - clippy::disallowed_methods, - reason = "integration-style test: writes and reads a fake codex script via sync std::fs to \ - verify the login_command string passes stdin correctly" - )] - async fn openai_api_key_cli_login_command_executes_codex_from_local_bin() { - let dir = tempfile::tempdir().unwrap(); - let local_bin = dir.path().join(".local/bin"); - std::fs::create_dir_all(&local_bin).unwrap(); - - let codex_path = local_bin.join("codex"); - std::fs::write( - &codex_path, - "#!/bin/sh\nprintf '%s\\n' \"$@\" > \"$HOME/codex-args.txt\"\ncat > \"$HOME/codex-stdin.txt\"\n", - ) - .unwrap(); - let mut permissions = std::fs::metadata(&codex_path).unwrap().permissions(); - permissions.set_mode(0o755); - std::fs::set_permissions(&codex_path, permissions).unwrap(); - - let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault_set_token(&mut vault, "OPENAI_API_KEY", "openai-key").unwrap(); - let resolver = test_resolver(vault, Arc::new(|_| None)); - let catalog = default_catalog(); - - let ResolvedCredential::Cli(cli) = resolver - .resolve( - ProviderId::openai(), - CredentialUsage::CliAgent(CliAgentKind::Codex), - &catalog, - ) - .await - .unwrap() - else { - panic!("expected cli credential"); - }; - - #[allow( - clippy::disallowed_methods, - reason = "This test shells through /bin/sh to verify the configured login command." - )] - let status = std::process::Command::new("/bin/sh") - .arg("-lc") - .arg(cli.login_command.unwrap()) - .env("HOME", dir.path()) - .env("PATH", "/usr/bin:/bin") - .status() - .unwrap(); - - assert!(status.success()); - assert_eq!( - std::fs::read_to_string(dir.path().join("codex-args.txt")).unwrap(), - "login\n--with-api-key\n" - ); - assert_eq!( - std::fs::read_to_string(dir.path().join("codex-stdin.txt")) - .unwrap() - .trim_end(), - "openai-key" - ); - } - #[tokio::test] async fn with_env_lookup_overrides_vault_settings() { let dir = tempfile::tempdir().unwrap(); @@ -935,13 +703,11 @@ reasoning = false ); let catalog = default_catalog(); - let ResolvedCredential::Api(api) = resolver + let resolved = resolver .resolve(ProviderId::openai(), CredentialUsage::ApiRequest, &catalog) .await - .unwrap() - else { - panic!("expected api credential"); - }; + .unwrap(); + let ResolvedCredential::Api(api) = resolved; assert_eq!(api.org_id.as_deref(), Some("env-org")); } @@ -1002,9 +768,7 @@ reasoning = false .await .unwrap(); - let ResolvedCredential::Api(api) = resolved else { - panic!("expected api credential"); - }; + let ResolvedCredential::Api(api) = resolved; assert_eq!(api.provider, ProviderId::new("acme")); assert_eq!( api.auth_header, @@ -1068,22 +832,17 @@ reasoning = false let resolver = CredentialResolver::with_env_lookup(Arc::clone(&vault), Arc::new(|_| None)); let catalog = default_catalog(); - let ResolvedCredential::Cli(cli) = resolver - .resolve( - ProviderId::openai(), - CredentialUsage::CliAgent(CliAgentKind::Codex), - &catalog, - ) + let resolved = resolver + .resolve(ProviderId::openai(), CredentialUsage::ApiRequest, &catalog) .await - .unwrap() - else { - panic!("expected cli credential"); - }; + .unwrap(); + let ResolvedCredential::Api(api) = resolved; assert_eq!( - cli.env_vars.get("OPENAI_API_KEY").map(String::as_str), - Some("new-access") + api.auth_header, + Some(ApiKeyHeader::Bearer("new-access".to_string())) ); + assert!(api.codex_mode); let stored = { let vault = vault.read().await; @@ -1116,11 +875,7 @@ reasoning = false let catalog = default_catalog(); let err = resolver - .resolve( - ProviderId::openai(), - CredentialUsage::CliAgent(CliAgentKind::Codex), - &catalog, - ) + .resolve(ProviderId::openai(), CredentialUsage::ApiRequest, &catalog) .await .unwrap_err(); diff --git a/lib/crates/fabro-auth/src/vault_source.rs b/lib/crates/fabro-auth/src/vault_source.rs index 16329ed0a..26068b5ab 100644 --- a/lib/crates/fabro-auth/src/vault_source.rs +++ b/lib/crates/fabro-auth/src/vault_source.rs @@ -52,7 +52,6 @@ impl CredentialSource for VaultCredentialSource { .await { Ok(ResolvedCredential::Api(credential)) => credentials.push(credential), - Ok(ResolvedCredential::Cli(_)) => {} Err(ResolveError::NotConfigured(_)) if provider.auth.is_some() => {} Err(err) => auth_issues.push((provider.id.clone(), err)), } diff --git a/lib/crates/fabro-cli/tests/it/workflow/acp.rs b/lib/crates/fabro-cli/tests/it/workflow/acp.rs index 706a0729b..3f34c935d 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/acp.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/acp.rs @@ -19,9 +19,8 @@ fn acp_backend_workflow() { "[server.auth]\nmethods = [\"dev-token\"]\n", ); context.isolated_server(); - seed_openai_vault(&context.storage_dir); let fake_agent = write_fake_acp_agent(&context); - let acp_command = fake_acp_command_attr(&fake_agent); + let acp_config = fake_acp_config_attr(&fake_agent); let workflow = context.temp_dir.join("acp_backend.fabro"); context.write_temp( "acp_backend.fabro", @@ -29,7 +28,7 @@ fn acp_backend_workflow() { r#"digraph ACP {{ graph [goal="Exercise ACP backend"] start [shape=Mdiamond] - work [type="agent", backend="acp", provider="openai", model="fake-acp", prompt="write hello.txt", acp_command={acp_command}] + work [type="agent", backend="acp", prompt="write hello.txt", acp.config={acp_config}] exit [shape=Msquare] start -> work work -> exit @@ -78,34 +77,37 @@ fn acp_backend_workflow() { assert!( stages.values().any(|stage| { stage["provider_used"]["mode"] == "acp" - && stage["provider_used"]["provider"] == "openai" + && stage["provider_used"]["config_name"] == "fake" + && stage["provider_used"].get("provider").is_none() }), - "run projection should include ACP provider metadata: {stages:?}" + "run projection should include ACP process metadata without provider: {stages:?}" ); } #[test] -fn acp_prompt_workflow_uses_acp_backend() { +fn acp_backend_does_not_inject_registered_provider_credentials() { 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); + seed_anthropic_vault(&context.storage_dir); + let fake_agent = write_fake_acp_agent(&context); - let acp_command = fake_acp_command_attr(&fake_agent); - let workflow = context.temp_dir.join("acp_prompt_backend.fabro"); + let env_record = context.temp_dir.join("acp-env.json"); + let acp_config = fake_acp_config_attr_recording_env(&fake_agent, &env_record); + let workflow = context.temp_dir.join("acp_provider_env.fabro"); context.write_temp( - "acp_prompt_backend.fabro", + "acp_provider_env.fabro", format!( r#"digraph ACP {{ - graph [goal="Exercise ACP prompt backend"] + graph [goal="Exercise ACP backend with stored provider credentials"] start [shape=Mdiamond] - prompt [type="prompt", backend="acp", provider="openai", model="fake-acp", project_memory=false, prompt="write hello.txt", acp_command={acp_command}] + work [type="agent", backend="acp", prompt="write hello.txt", acp.config={acp_config}] exit [shape=Msquare] - start -> prompt - prompt -> exit + start -> work + work -> exit }}"# ), ); @@ -113,6 +115,9 @@ fn acp_prompt_workflow_uses_acp_backend() { context .run_cmd() + .env_remove("ANTHROPIC_API_KEY") + .env_remove("OPENAI_API_KEY") + .env_remove("GEMINI_API_KEY") .args(["--auto-approve", "--sandbox", "local"]) .arg(&workflow) .assert() @@ -122,45 +127,12 @@ fn acp_prompt_workflow_uses_acp_backend() { 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"); - vault - .set("OPENAI_API_KEY", "test-openai-key", SecretType::Token, None) - .expect("OpenAI credential should store in test vault"); + let recorded_env: serde_json::Value = serde_json::from_str( + &std::fs::read_to_string(&env_record) + .expect("fake ACP agent should record its environment"), + ) + .expect("fake ACP environment record should be valid JSON"); + assert_eq!(recorded_env, serde_json::json!({})); } fn write_fake_acp_agent(context: &fabro_test::TestContext) -> std::path::PathBuf { @@ -168,16 +140,52 @@ fn write_fake_acp_agent(context: &fabro_test::TestContext) -> std::path::PathBuf context.temp_dir.join("fake_acp_agent.py") } -fn fake_acp_command_attr(script_path: &std::path::Path) -> String { - let command = serde_json::json!({ +fn fake_acp_config_attr(script_path: &std::path::Path) -> String { + fake_acp_config_attr_with_env(script_path, Vec::new()) +} + +fn fake_acp_config_attr_recording_env( + script_path: &std::path::Path, + env_record: &std::path::Path, +) -> String { + fake_acp_config_attr_with_env(script_path, vec![ + serde_json::json!({"name": "ACP_ENV_RECORD", "value": env_record.to_string_lossy()}), + serde_json::json!({ + "name": "ACP_ENV_RECORD_KEYS", + "value": "ANTHROPIC_API_KEY,OPENAI_API_KEY,GEMINI_API_KEY", + }), + ]) +} + +fn fake_acp_config_attr_with_env( + script_path: &std::path::Path, + extra_env: Vec, +) -> String { + let mut env = vec![serde_json::json!({"name": "ACP_MODE", "value": "write_file"})]; + env.extend(extra_env); + + let config = serde_json::json!({ "type": "stdio", "name": "fake", "command": "python3", "args": [script_path.to_string_lossy()], - "env": [{"name": "ACP_MODE", "value": "write_file"}], + "env": env, }) .to_string(); - format!("{command:?}") + format!("{config:?}") +} + +fn seed_anthropic_vault(storage_dir: &std::path::Path) { + let mut vault = + Vault::load(Storage::new(storage_dir).secrets_path()).expect("test vault should load"); + vault + .set( + "ANTHROPIC_API_KEY", + "vault-anthropic-key", + SecretType::Token, + None, + ) + .expect("Anthropic credential should store in test vault"); } fn init_git_repo(dir: &std::path::Path) { diff --git a/lib/crates/fabro-cli/tests/it/workflow/mod.rs b/lib/crates/fabro-cli/tests/it/workflow/mod.rs index 023d4f825..21cf17c21 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/mod.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/mod.rs @@ -12,7 +12,6 @@ mod dry_run_examples; mod full_stack; mod hooks; mod human_gate; -mod real_cli; use std::path::{Path, PathBuf}; use std::time::Duration; diff --git a/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs b/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs deleted file mode 100644 index ad0b51fcb..000000000 --- a/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs +++ /dev/null @@ -1,71 +0,0 @@ -use std::sync::Arc; - -use fabro_graphviz::graph::{AttrValue, Node}; -use fabro_model::ProviderId; -use fabro_workflow::context::Context; -use fabro_workflow::event::Emitter; -use fabro_workflow::handler::agent::{CodergenBackend, CodergenResult, CodergenRunRequest}; -use fabro_workflow::handler::llm::cli::AgentCliBackend; - -/// Run a real CLI tool via LocalSandbox and verify the full flow. -async fn run_real_cli_test(provider: ProviderId, model: &str) { - let workspace = tempfile::tempdir().expect("real CLI test workspace should create"); - let env: Arc = Arc::new(fabro_agent::LocalSandbox::new( - workspace.path().to_path_buf(), - )); - let backend = AgentCliBackend::new_from_env(model.to_string(), provider.clone()); - - let mut node = Node::new("real_cli_test"); - node.attrs.insert( - "prompt".to_string(), - AttrValue::String("What is 2+2? Reply with just the number.".to_string()), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - let result = backend - .run(CodergenRunRequest { - node: &node, - prompt: "What is 2+2? Reply with just the number.", - context: &context, - thread_id: None, - emitter: &emitter, - sandbox: &env, - tool_hooks: None, - cancel_token: tokio_util::sync::CancellationToken::new(), - }) - .await - .unwrap_or_else(|_| panic!("CLI backend ({provider}/{model}) should succeed")); - - match result { - CodergenResult::Text { text, usage, .. } => { - assert!( - text.contains('4'), - "{provider}/{model}: expected response to contain '4', got: {text}" - ); - let usage = usage.unwrap_or_else(|| panic!("{provider}/{model}: should have usage")); - let tokens = usage.tokens(); - assert!( - tokens.input_tokens > 0, - "{provider}/{model}: input_tokens should be > 0, got {}", - tokens.input_tokens - ); - } - CodergenResult::Full(_) => panic!("expected Text result from {provider}/{model}"), - } -} - -#[fabro_macros::e2e_test(live("ANTHROPIC_API_KEY"))] -async fn real_cli_claude() { - run_real_cli_test(ProviderId::anthropic(), "haiku").await; -} - -#[fabro_macros::e2e_test(live("OPENAI_API_KEY"))] -async fn real_cli_codex() { - run_real_cli_test(ProviderId::openai(), "").await; -} - -#[fabro_macros::e2e_test(live("GEMINI_API_KEY"))] -async fn real_cli_gemini() { - run_real_cli_test(ProviderId::gemini(), "gemini-2.5-flash").await; -} diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 2be58dfb5..f2d165d62 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2483,18 +2483,17 @@ fn update_live_run_from_event(state: &AppState, run_id: RunId, event: &RunEvent) } } } - // Track non-steerable agent stages. CLI/ACP started/completed are + // Track non-steerable agent stages. ACP started/completed are // coarser and sometimes fail to emit terminal events on error paths; // stage.completed/stage.failed below are the backstops. - EventBody::AgentCliStarted(_) | EventBody::AgentAcpStarted(_) => { + EventBody::AgentAcpStarted(_) => { if let Some(stage_id) = event.stage_id.as_ref() { managed_run .active_non_steerable_agent_stages .insert(stage_id.clone()); } } - EventBody::AgentCliCompleted(_) - | EventBody::AgentAcpCompleted(_) + EventBody::AgentAcpCompleted(_) | EventBody::AgentAcpCancelled(_) | EventBody::AgentAcpTimedOut(_) => { if let Some(stage_id) = &event.stage_id { @@ -2504,7 +2503,7 @@ fn update_live_run_from_event(state: &AppState, run_id: RunId, event: &RunEvent) } } // Stage lifecycle backstop: cover both completion and failure - // paths so a failing CLI stage doesn't strand its entry. + // paths so a failing ACP stage doesn't strand its entry. EventBody::StageCompleted(_) | EventBody::StageFailed(_) => { if let Some(stage_id) = &event.stage_id { managed_run.active_api_stages.remove(stage_id); diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 60353d4b5..e34e12800 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -8551,12 +8551,10 @@ async fn steer_with_active_acp_stage_returns_non_steerable_conflict() { ); let started = acp_event_for_stage(&run_id, &workflow_event::Event::AgentAcpStarted { - node_id: "agent".to_string(), - visit: 1, - mode: "acp".to_string(), - provider: "openai".to_string(), - model: "fake-acp".to_string(), - command: "python fake_agent.py".to_string(), + node_id: "agent".to_string(), + visit: 1, + command: "python fake_agent.py".to_string(), + config_name: None, }); update_live_run_from_event(&state, run_id, &started); @@ -8640,12 +8638,10 @@ async fn active_acp_stage_marker_clears_on_terminal_paths() { Some(RunAnswerTransport::Subprocess { control_tx }), ); let started = acp_event_for_stage(&run_id, &workflow_event::Event::AgentAcpStarted { - node_id: "agent".to_string(), - visit: 1, - mode: "acp".to_string(), - provider: "openai".to_string(), - model: "fake-acp".to_string(), - command: "python fake_agent.py".to_string(), + node_id: "agent".to_string(), + visit: 1, + command: "python fake_agent.py".to_string(), + config_name: None, }); update_live_run_from_event(&state, run_id, &started); let terminal = acp_event_for_stage(&run_id, &terminal_event); diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index c09c0c3bf..38e083641 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -3,9 +3,8 @@ use std::str::FromStr; use chrono::{DateTime, Utc}; use fabro_types::run_event::{ - AgentAcpStartedProps, AgentCliStartedProps, AgentSessionActivatedProps, - CheckpointCompletedProps, RunCompletedProps, RunFailedProps, StageCompletedProps, - StagePromptProps, + AgentAcpStartedProps, AgentSessionActivatedProps, CheckpointCompletedProps, RunCompletedProps, + RunFailedProps, StageCompletedProps, StagePromptProps, }; use fabro_types::settings::run::RunSandboxSettings; use fabro_types::{ @@ -376,13 +375,6 @@ impl RunProjectionReducer for RunProjection { }; stage.provider_used = Some(provider_used_from_agent_session_activated(props)); } - EventBody::AgentCliStarted(props) => { - let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) - else { - return Ok(()); - }; - stage.provider_used = Some(provider_used_from_agent_cli_started(props)); - } EventBody::AgentAcpStarted(props) => { let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) else { @@ -412,42 +404,6 @@ impl RunProjectionReducer for RunProjection { stage.termination = Some(props.termination); stage.script_timing = Some(script_timing); } - EventBody::AgentCliCompleted(props) => { - let Some(stage) = stage_at_current_visit(self, stored, event.seq) else { - return Ok(()); - }; - apply_agent_terminal( - "agent.cli", - stage, - props, - merge_agent_cli_output(&props.stdout, &props.stderr), - CommandTermination::Exited, - )?; - } - EventBody::AgentCliCancelled(props) => { - let Some(stage) = stage_at_current_visit(self, stored, event.seq) else { - return Ok(()); - }; - apply_agent_terminal( - "agent.cli", - stage, - props, - merge_agent_cli_output(&props.stdout, &props.stderr), - CommandTermination::Cancelled, - )?; - } - EventBody::AgentCliTimedOut(props) => { - let Some(stage) = stage_at_current_visit(self, stored, event.seq) else { - return Ok(()); - }; - apply_agent_terminal( - "agent.cli", - stage, - props, - merge_agent_cli_output(&props.stdout, &props.stderr), - CommandTermination::TimedOut, - )?; - } EventBody::AgentAcpCompleted(props) => { let Some(stage) = stage_at_current_visit(self, stored, event.seq) else { return Ok(()); @@ -456,7 +412,7 @@ impl RunProjectionReducer for RunProjection { "agent.acp", stage, props, - merge_agent_cli_output(&props.stdout, &props.stderr), + merge_agent_process_output(&props.stdout, &props.stderr), CommandTermination::Exited, )?; } @@ -468,7 +424,7 @@ impl RunProjectionReducer for RunProjection { "agent.acp", stage, props, - merge_agent_cli_output(&props.stdout, &props.stderr), + merge_agent_process_output(&props.stdout, &props.stderr), CommandTermination::Cancelled, )?; } @@ -480,7 +436,7 @@ impl RunProjectionReducer for RunProjection { "agent.acp", stage, props, - merge_agent_cli_output(&props.stdout, &props.stderr), + merge_agent_process_output(&props.stdout, &props.stderr), CommandTermination::TimedOut, )?; } @@ -893,25 +849,13 @@ fn provider_used_from_agent_session_activated(props: &AgentSessionActivatedProps Value::Object(provider_used) } -fn provider_used_from_agent_cli_started(props: &AgentCliStartedProps) -> Value { - provider_used_from_agent_process_started("cli", &props.provider, &props.model, &props.command) -} - fn provider_used_from_agent_acp_started(props: &AgentAcpStartedProps) -> Value { - provider_used_from_agent_process_started("acp", &props.provider, &props.model, &props.command) -} - -fn provider_used_from_agent_process_started( - mode: &str, - provider: &str, - model: &str, - command: &str, -) -> Value { let mut provider_used = serde_json::Map::new(); - provider_used.insert("mode".to_string(), Value::String(mode.to_string())); - provider_used.insert("provider".to_string(), Value::String(provider.to_string())); - provider_used.insert("model".to_string(), Value::String(model.to_string())); - provider_used.insert("command".to_string(), Value::String(command.to_string())); + provider_used.insert("mode".to_string(), Value::String("acp".to_string())); + provider_used.insert("command".to_string(), Value::String(props.command.clone())); + if let Some(config_name) = props.config_name.clone() { + provider_used.insert("config_name".to_string(), Value::String(config_name)); + } Value::Object(provider_used) } @@ -931,7 +875,7 @@ fn apply_agent_terminal( Ok(()) } -fn merge_agent_cli_output(stdout: &str, stderr: &str) -> String { +fn merge_agent_process_output(stdout: &str, stderr: &str) -> String { match (stdout.is_empty(), stderr.is_empty()) { (true, true) => String::new(), (false, true) => stdout.to_string(), @@ -948,8 +892,7 @@ mod tests { use fabro_types::run_event::run::RunFailedProps; use fabro_types::run_event::{ AgentAcpCancelledProps, AgentAcpCompletedProps, AgentAcpStartedProps, - AgentAcpTimedOutProps, AgentCliCancelledProps, AgentCliCompletedProps, - AgentCliTimedOutProps, AgentMessageProps, AgentSessionActivatedProps, + AgentAcpTimedOutProps, AgentMessageProps, AgentSessionActivatedProps, AgentSessionEndedProps, AgentSessionStartedProps, CheckpointCompletedProps, InterviewCompletedProps, InterviewOption, InterviewStartedProps, RunControlEffectProps, StageCompletedProps, StageFailedProps, StagePromptProps, StageRetryingProps, @@ -1367,11 +1310,9 @@ mod tests { .apply_event(&test_stage_event( 4, EventBody::AgentAcpStarted(AgentAcpStartedProps { - visit: 1, - mode: "acp".to_string(), - provider: "openai".to_string(), - model: "fake-acp".to_string(), - command: "python fake_agent.py".to_string(), + visit: 1, + command: "python fake_agent.py".to_string(), + config_name: Some("fake".to_string()), }), stage_id.clone(), )) @@ -1382,9 +1323,8 @@ mod tests { stage.provider_used.as_ref().unwrap(), &json!({ "mode": "acp", - "provider": "openai", - "model": "fake-acp", - "command": "python fake_agent.py" + "command": "python fake_agent.py", + "config_name": "fake" }) ); } @@ -1460,88 +1400,6 @@ mod tests { assert_eq!(stage.termination, Some(CommandTermination::TimedOut)); } - #[test] - fn agent_cli_completed_updates_stage_output_projection() { - let mut state = initialized_projection(); - let stage_id = StageId::new("code", 1); - start_stage(&mut state, &stage_id); - - state - .apply_event(&test_stage_event( - 4, - EventBody::AgentCliCompleted(AgentCliCompletedProps { - stdout: "done".to_string(), - stderr: "warn".to_string(), - exit_code: 0, - duration_ms: 42, - }), - stage_id.clone(), - )) - .unwrap(); - - let stage = state.stage(&stage_id).unwrap(); - assert_eq!(stage.output.as_deref(), Some("done\nwarn")); - assert_eq!(stage.termination, Some(CommandTermination::Exited)); - assert_eq!( - stage.script_timing.as_ref().unwrap()["duration_ms"], - serde_json::json!(42) - ); - } - - #[test] - fn agent_cli_cancelled_updates_stage_output_projection() { - let mut state = initialized_projection(); - let stage_id = StageId::new("code", 1); - start_stage(&mut state, &stage_id); - - state - .apply_event(&test_stage_event( - 4, - EventBody::AgentCliCancelled(AgentCliCancelledProps { - stdout: "partial".to_string(), - stderr: "cancelled".to_string(), - duration_ms: 7, - }), - stage_id.clone(), - )) - .unwrap(); - - let stage = state.stage(&stage_id).unwrap(); - assert_eq!(stage.output.as_deref(), Some("partial\ncancelled")); - assert_eq!(stage.termination, Some(CommandTermination::Cancelled)); - assert_eq!( - stage.script_timing.as_ref().unwrap()["duration_ms"], - serde_json::json!(7) - ); - } - - #[test] - fn agent_cli_timed_out_updates_stage_output_projection() { - let mut state = initialized_projection(); - let stage_id = StageId::new("code", 1); - start_stage(&mut state, &stage_id); - - state - .apply_event(&test_stage_event( - 4, - EventBody::AgentCliTimedOut(AgentCliTimedOutProps { - stdout: "partial".to_string(), - stderr: "timeout".to_string(), - duration_ms: 600, - }), - stage_id.clone(), - )) - .unwrap(); - - let stage = state.stage(&stage_id).unwrap(); - assert_eq!(stage.output.as_deref(), Some("partial\ntimeout")); - assert_eq!(stage.termination, Some(CommandTermination::TimedOut)); - assert_eq!( - stage.script_timing.as_ref().unwrap()["duration_ms"], - serde_json::json!(600) - ); - } - #[test] fn stage_completed_event_captures_duration_and_usage_per_visit() { let mut state = initialized_projection(); diff --git a/lib/crates/fabro-types/src/graph.rs b/lib/crates/fabro-types/src/graph.rs index 48ef9a2d7..27344f8c9 100644 --- a/lib/crates/fabro-types/src/graph.rs +++ b/lib/crates/fabro-types/src/graph.rs @@ -3,7 +3,7 @@ use std::time::Duration; use serde::{Deserialize, Serialize}; -use crate::LlmBackend; +use crate::AgentBackend; /// Typed attribute values for nodes, edges, and graph-level attributes. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -258,15 +258,25 @@ impl Node { } #[must_use] - pub fn llm_backend(&self) -> Option> { + pub fn agent_backend(&self) -> Option> { self.backend().map(str::parse) } #[must_use] - pub fn acp_command(&self) -> Option<&str> { + pub fn legacy_acp_command_attr(&self) -> Option<&str> { self.str_attr("acp_command") } + #[must_use] + pub fn acp_command_attr(&self) -> Option<&str> { + self.str_attr("acp.command") + } + + #[must_use] + pub fn acp_config_attr(&self) -> Option<&str> { + self.str_attr("acp.config") + } + #[must_use] pub fn selection(&self) -> &str { self.str_attr("selection").unwrap_or("deterministic") diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index e4fd7b55d..6a0138ffa 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -61,7 +61,7 @@ pub use graph::{ shape_to_handler_type, }; pub use interview::{InterviewQuestionRecord, QuestionType}; -pub use llm_backend::LlmBackend; +pub use llm_backend::AgentBackend; pub use manifest_path::{ManifestPath, ManifestPathParseError}; pub use outcome::{ FailureCategory, FailureDetail, NodeResult, Outcome, OutcomeMeta, StageOutcome, StageState, diff --git a/lib/crates/fabro-types/src/llm_backend.rs b/lib/crates/fabro-types/src/llm_backend.rs index 5a91fce2e..adacb0db1 100644 --- a/lib/crates/fabro-types/src/llm_backend.rs +++ b/lib/crates/fabro-types/src/llm_backend.rs @@ -18,15 +18,27 @@ use strum::{Display, EnumString, IntoStaticStr, VariantArray, VariantNames}; )] #[serde(rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] -pub enum LlmBackend { +pub enum AgentBackend { Api, - Cli, Acp, } -impl LlmBackend { +impl AgentBackend { #[must_use] pub fn expected_values() -> String { ::VARIANTS.join(", ") } } + +#[cfg(test)] +mod tests { + use super::AgentBackend; + + #[test] + fn agent_backend_accepts_only_api_and_acp() { + assert_eq!("api".parse::().unwrap(), AgentBackend::Api); + assert_eq!("acp".parse::().unwrap(), AgentBackend::Acp); + assert!("cli".parse::().is_err()); + assert_eq!(AgentBackend::expected_values(), "api, acp"); + } +} diff --git a/lib/crates/fabro-types/src/run_event/misc.rs b/lib/crates/fabro-types/src/run_event/misc.rs index 6345cd298..b2280c243 100644 --- a/lib/crates/fabro-types/src/run_event/misc.rs +++ b/lib/crates/fabro-types/src/run_event/misc.rs @@ -218,44 +218,12 @@ pub struct CommandCompletedProps { pub live_streaming: bool, } -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct AgentCliStartedProps { - pub visit: u32, - pub mode: String, - pub provider: String, - pub model: String, - pub command: String, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct AgentCliCompletedProps { - pub stdout: String, - pub stderr: String, - pub exit_code: i32, - pub duration_ms: u64, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct AgentCliCancelledProps { - pub stdout: String, - pub stderr: String, - pub duration_ms: u64, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct AgentCliTimedOutProps { - pub stdout: String, - pub stderr: String, - pub duration_ms: u64, -} - #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct AgentAcpStartedProps { - pub visit: u32, - pub mode: String, - pub provider: String, - pub model: String, - pub command: String, + pub visit: u32, + pub command: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub config_name: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-types/src/run_event/mod.rs b/lib/crates/fabro-types/src/run_event/mod.rs index 8ee972961..9be0c8669 100644 --- a/lib/crates/fabro-types/src/run_event/mod.rs +++ b/lib/crates/fabro-types/src/run_event/mod.rs @@ -286,14 +286,6 @@ pub enum EventBody { CommandStarted(CommandStartedProps), #[serde(rename = "command.completed")] CommandCompleted(CommandCompletedProps), - #[serde(rename = "agent.cli.started")] - AgentCliStarted(AgentCliStartedProps), - #[serde(rename = "agent.cli.completed")] - AgentCliCompleted(AgentCliCompletedProps), - #[serde(rename = "agent.cli.cancelled")] - AgentCliCancelled(AgentCliCancelledProps), - #[serde(rename = "agent.cli.timed_out")] - AgentCliTimedOut(AgentCliTimedOutProps), #[serde(rename = "agent.acp.started")] AgentAcpStarted(AgentAcpStartedProps), #[serde(rename = "agent.acp.completed")] @@ -498,10 +490,6 @@ impl EventBody { Self::CliEnsureFailed(_) => "cli.ensure.failed", Self::CommandStarted(_) => "command.started", Self::CommandCompleted(_) => "command.completed", - Self::AgentCliStarted(_) => "agent.cli.started", - Self::AgentCliCompleted(_) => "agent.cli.completed", - Self::AgentCliCancelled(_) => "agent.cli.cancelled", - Self::AgentCliTimedOut(_) => "agent.cli.timed_out", Self::AgentAcpStarted(_) => "agent.acp.started", Self::AgentAcpCompleted(_) => "agent.acp.completed", Self::AgentAcpCancelled(_) => "agent.acp.cancelled", @@ -653,10 +641,6 @@ fn is_known_event_name(event: &str) -> bool { | "cli.ensure.failed" | "command.started" | "command.completed" - | "agent.cli.started" - | "agent.cli.completed" - | "agent.cli.cancelled" - | "agent.cli.timed_out" | "agent.acp.started" | "agent.acp.completed" | "agent.acp.cancelled" diff --git a/lib/crates/fabro-validate/Cargo.toml b/lib/crates/fabro-validate/Cargo.toml index d2a1252f0..a6c3ef3f9 100644 --- a/lib/crates/fabro-validate/Cargo.toml +++ b/lib/crates/fabro-validate/Cargo.toml @@ -13,6 +13,7 @@ doctest = false workspace = true [dependencies] +fabro-acp = { path = "../fabro-acp", default-features = false } fabro-graphviz = { path = "../fabro-graphviz" } fabro-model = { path = "../fabro-model" } fabro-types = { path = "../fabro-types" } diff --git a/lib/crates/fabro-validate/src/rules/backend_valid.rs b/lib/crates/fabro-validate/src/rules/backend_valid.rs index 19014be56..b59500254 100644 --- a/lib/crates/fabro-validate/src/rules/backend_valid.rs +++ b/lib/crates/fabro-validate/src/rules/backend_valid.rs @@ -1,5 +1,6 @@ +use fabro_acp::{AcpCommandError, AcpProcessSpec}; use fabro_graphviz::graph::{Graph, Node}; -use fabro_types::LlmBackend; +use fabro_types::AgentBackend; use crate::{Diagnostic, LintRule, Severity}; @@ -18,38 +19,16 @@ impl LintRule for Rule { let mut diagnostics = Vec::new(); for node in graph.nodes.values() { if let Some(backend) = node.backend() { - match node.llm_backend() { + match node.agent_backend() { Some(Err(_)) => { - let expected = LlmBackend::expected_values(); - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!( - "unsupported LLM backend \"{backend}\"; expected one of: {expected}" - ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!("Use one of: {expected}")), - - ..Diagnostic::default() - }); + diagnostics.push(unsupported_backend_diagnostic( + self.name(), + node, + backend, + )); } - Some(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(), - ), - - ..Diagnostic::default() - }); + Some(Ok(AgentBackend::Acp)) => { + diagnostics.extend(validate_acp_node(self.name(), node)); } Some(Ok(_)) | None => {} } @@ -59,11 +38,150 @@ impl LintRule for Rule { } } -fn acp_command_missing(node: &Node) -> bool { - match node.acp_command() { - Some(command) => command.trim().is_empty(), - None => true, +fn unsupported_backend_diagnostic(rule: &str, node: &Node, backend: &str) -> Diagnostic { + if backend == "cli" { + return Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: "backend=\"cli\" is no longer supported; external agents must be launched \ + through backend=\"acp\" with acp.command or acp.config" + .to_string(), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Use backend=\"api\" for Fabro-owned provider execution, or backend=\"acp\" with \ + acp.command/acp.config for a user-supplied ACP process" + .to_string(), + ), + + ..Diagnostic::default() + }; } + + let expected = AgentBackend::expected_values(); + Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: format!("unsupported agent backend \"{backend}\"; expected one of: {expected}"), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!("Use one of: {expected}")), + + ..Diagnostic::default() + } +} + +fn validate_acp_node(rule: &str, node: &Node) -> Vec { + let mut diagnostics = Vec::new(); + + if node.handler_type() != Some("agent") { + diagnostics.push(Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: "backend=\"acp\" is only valid on agent nodes; prompt nodes are API-only" + .to_string(), + node_id: Some(node.id.clone()), + edge: None, + fix: Some("Use backend=\"api\" on prompt nodes".to_string()), + + ..Diagnostic::default() + }); + } + + if let Err(error) = AcpProcessSpec::from_attrs( + node.legacy_acp_command_attr(), + node.acp_command_attr(), + node.acp_config_attr(), + ) { + diagnostics.push(acp_process_diagnostic(rule, node, &error)); + } + + let api_only_attrs = api_only_attrs_present(node); + if !api_only_attrs.is_empty() { + diagnostics.push(Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: format!( + "backend=\"acp\" does not support API-only attributes: {}", + api_only_attrs.join(", ") + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some("Remove API model/provider/control attributes from ACP nodes".to_string()), + + ..Diagnostic::default() + }); + } + + diagnostics +} + +fn acp_process_diagnostic(rule: &str, node: &Node, error: &AcpCommandError) -> Diagnostic { + match error { + AcpCommandError::LegacyCommandAttribute => Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: "acp_command is no longer supported; use acp.command for shell commands or \ + acp.config for JSON stdio ACP configs" + .to_string(), + node_id: Some(node.id.clone()), + edge: None, + fix: Some("Rename acp_command to acp.command".to_string()), + + ..Diagnostic::default() + }, + AcpCommandError::EmptyOverride + | AcpCommandError::MissingOverride + | AcpCommandError::InvalidCommandString => Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: render_acp_process_error(error), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Set acp.command to a shell command, or acp.config to a JSON stdio ACP config" + .to_string(), + ), + + ..Diagnostic::default() + }, + AcpCommandError::InvalidConfigJson(_) + | AcpCommandError::InvalidConfigShape(_) + | AcpCommandError::UnsupportedTransport => Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message: format!( + "acp.config must be a JSON stdio ACP config: {}", + render_acp_process_error(error) + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Provide a JSON config with type=\"stdio\", command, and optional args".to_string(), + ), + + ..Diagnostic::default() + }, + } +} + +fn render_acp_process_error(error: &AcpCommandError) -> String { + error.to_string() +} + +fn api_only_attrs_present(node: &Node) -> Vec<&'static str> { + const API_ONLY_ATTRS: &[&str] = &[ + "model", + "provider", + "reasoning_effort", + "max_tokens", + "speed", + ]; + API_ONLY_ATTRS + .iter() + .copied() + .filter(|attr| node.attrs.contains_key(*attr)) + .collect() } #[cfg(test)] @@ -75,8 +193,8 @@ mod tests { use crate::{LintRule, Severity}; #[test] - fn backend_valid_accepts_absent_api_and_cli() { - for backend in [None, Some("api"), Some("cli")] { + fn backend_valid_accepts_absent_and_api() { + for backend in [None, Some("api")] { let mut graph = minimal_graph(); let mut node = Node::new("work"); if let Some(backend) = backend { @@ -107,12 +225,12 @@ mod tests { assert!( diagnostics[0] .message - .contains("unsupported LLM backend \"codex\"; expected one of: api, cli, acp") + .contains("unsupported agent backend \"codex\"; expected one of: api, acp") ); } #[test] - fn backend_valid_requires_acp_command_for_acp_backend() { + fn backend_valid_requires_acp_process_attr_for_acp_backend() { let mut graph = minimal_graph(); let mut node = Node::new("work"); node.attrs @@ -122,23 +240,208 @@ mod tests { 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" - )); + assert!( + diagnostics[0] + .message + .contains("requires exactly one of acp.command or acp.config") + ); } #[test] - fn backend_valid_accepts_acp_backend_with_acp_command() { + fn backend_valid_accepts_acp_backend_with_acp_command_attr() { 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(), + "acp.command".to_string(), AttrValue::String("agent-acp".to_string()), ); graph.nodes.insert("work".to_string(), node); assert!(Rule.apply(&graph).is_empty()); } + + #[test] + fn backend_valid_rejects_cli_backend_with_migration_guidance() { + let mut graph = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("backend".to_string(), AttrValue::String("cli".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=\"cli\" is no longer supported") + ); + assert!( + diagnostics[0] + .fix + .as_deref() + .unwrap() + .contains("backend=\"acp\"") + ); + } + + #[test] + fn backend_valid_requires_exactly_one_acp_process_attr() { + let mut missing = minimal_graph(); + let mut missing_node = Node::new("missing"); + missing_node + .attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + missing.nodes.insert("missing".to_string(), missing_node); + + let diagnostics = Rule.apply(&missing); + assert_eq!(diagnostics.len(), 1); + assert!( + diagnostics[0] + .message + .contains("requires exactly one of acp.command or acp.config") + ); + + let mut both = minimal_graph(); + let mut both_node = Node::new("both"); + both_node + .attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + both_node.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 agent.py".to_string()), + ); + both_node.attrs.insert( + "acp.config".to_string(), + AttrValue::String( + r#"{"type":"stdio","name":"agent","command":"python3","args":["agent.py"]}"# + .to_string(), + ), + ); + both.nodes.insert("both".to_string(), both_node); + + let diagnostics = Rule.apply(&both); + assert_eq!(diagnostics.len(), 1); + assert!( + diagnostics[0] + .message + .contains("requires exactly one of acp.command or acp.config") + ); + } + + #[test] + fn backend_valid_rejects_legacy_acp_command_attr() { + 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("python3 agent.py".to_string()), + ); + graph.nodes.insert("work".to_string(), node); + + let diagnostics = Rule.apply(&graph); + assert_eq!(diagnostics.len(), 1); + assert!( + diagnostics[0] + .message + .contains("acp_command is no longer supported") + ); + } + + #[test] + fn backend_valid_rejects_acp_on_prompt_nodes_and_api_only_attrs() { + let mut graph = minimal_graph(); + let mut node = Node::new("prompt"); + node.attrs + .insert("type".to_string(), AttrValue::String("prompt".to_string())); + node.attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + node.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 agent.py".to_string()), + ); + node.attrs.insert( + "model".to_string(), + AttrValue::String("gpt-5.4".to_string()), + ); + node.attrs.insert( + "reasoning_effort".to_string(), + AttrValue::String("high".to_string()), + ); + graph.nodes.insert("prompt".to_string(), node); + + let diagnostics = Rule.apply(&graph); + assert_eq!(diagnostics.len(), 2); + assert!(diagnostics.iter().any(|diagnostic| { + diagnostic + .message + .contains("backend=\"acp\" is only valid on agent nodes") + })); + assert!(diagnostics.iter().any(|diagnostic| { + diagnostic + .message + .contains("backend=\"acp\" does not support API-only attributes") + })); + } + + #[test] + fn backend_valid_rejects_invalid_acp_config_but_accepts_json_shaped_command() { + let mut command_graph = minimal_graph(); + let mut command_node = Node::new("command"); + command_node + .attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + command_node.attrs.insert( + "acp.command".to_string(), + AttrValue::String(r#"{"type":"stdio"}"#.to_string()), + ); + command_graph + .nodes + .insert("command".to_string(), command_node); + assert!(Rule.apply(&command_graph).is_empty()); + + let mut config_graph = minimal_graph(); + let mut config_node = Node::new("config"); + config_node + .attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + config_node.attrs.insert( + "acp.config".to_string(), + AttrValue::String("python3 agent.py".to_string()), + ); + config_graph.nodes.insert("config".to_string(), config_node); + + let diagnostics = Rule.apply(&config_graph); + assert_eq!(diagnostics.len(), 1); + assert!( + diagnostics[0] + .message + .contains("acp.config must be a JSON stdio ACP config") + ); + } + + #[test] + fn backend_valid_rejects_invalid_acp_command() { + let mut graph = minimal_graph(); + let mut node = Node::new("command"); + node.attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + node.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python 'unterminated".to_string()), + ); + graph.nodes.insert("command".to_string(), node); + + let diagnostics = Rule.apply(&graph); + assert_eq!(diagnostics.len(), 1); + assert!( + diagnostics[0] + .message + .contains("failed to parse acp.command as a shell command") + ); + } } diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index 0ce439079..249320ba8 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -1028,32 +1028,6 @@ fn event_body_from_event(event: &Event) -> EventBody { output_bytes: *output_bytes, live_streaming: *live_streaming, }), - Event::AgentCliStarted { - visit, - mode, - provider, - model, - command, - .. - } => EventBody::AgentCliStarted(fabro_types::AgentCliStartedProps { - visit: *visit, - mode: mode.clone(), - provider: provider.clone(), - model: model.clone(), - command: command.clone(), - }), - Event::AgentCliCompleted { - stdout, - stderr, - exit_code, - duration_ms, - .. - } => EventBody::AgentCliCompleted(fabro_types::AgentCliCompletedProps { - stdout: stdout.clone(), - stderr: stderr.clone(), - exit_code: *exit_code, - duration_ms: *duration_ms, - }), Event::AgentSessionStarted { provider, model, .. } => EventBody::AgentSessionStarted(fabro_types::AgentSessionStartedProps { @@ -1096,39 +1070,15 @@ fn event_body_from_event(event: &Event) -> EventBody { count: *count, }) } - Event::AgentCliCancelled { - stdout, - stderr, - duration_ms, - .. - } => EventBody::AgentCliCancelled(fabro_types::AgentCliCancelledProps { - stdout: stdout.clone(), - stderr: stderr.clone(), - duration_ms: *duration_ms, - }), - Event::AgentCliTimedOut { - stdout, - stderr, - duration_ms, - .. - } => EventBody::AgentCliTimedOut(fabro_types::AgentCliTimedOutProps { - stdout: stdout.clone(), - stderr: stderr.clone(), - duration_ms: *duration_ms, - }), Event::AgentAcpStarted { visit, - mode, - provider, - model, command, + config_name, .. } => EventBody::AgentAcpStarted(fabro_types::AgentAcpStartedProps { - visit: *visit, - mode: mode.clone(), - provider: provider.clone(), - model: model.clone(), - command: command.clone(), + visit: *visit, + command: command.clone(), + config_name: config_name.clone(), }), Event::AgentAcpCompleted { stdout, @@ -2077,48 +2027,6 @@ mod tests { assert_eq!(message.billing.total_usd_micros, None); } - #[test] - fn agent_cli_cancelled_maps_to_event_body_with_node_id() { - let stored = to_run_event(&fixtures::RUN_1, &Event::AgentCliCancelled { - node_id: "code".to_string(), - stdout: "out".to_string(), - stderr: "err".to_string(), - duration_ms: 42, - }); - - assert_eq!(stored.event_name(), "agent.cli.cancelled"); - assert_eq!(stored.node_id.as_deref(), Some("code")); - match &stored.body { - EventBody::AgentCliCancelled(props) => { - assert_eq!(props.stdout, "out"); - assert_eq!(props.stderr, "err"); - assert_eq!(props.duration_ms, 42); - } - other => panic!("expected AgentCliCancelled, got {other:?}"), - } - } - - #[test] - fn agent_cli_timed_out_maps_to_event_body_with_node_id() { - let stored = to_run_event(&fixtures::RUN_1, &Event::AgentCliTimedOut { - node_id: "code".to_string(), - stdout: "out".to_string(), - stderr: "err".to_string(), - duration_ms: 99, - }); - - assert_eq!(stored.event_name(), "agent.cli.timed_out"); - assert_eq!(stored.node_id.as_deref(), Some("code")); - match &stored.body { - EventBody::AgentCliTimedOut(props) => { - assert_eq!(props.stdout, "out"); - assert_eq!(props.stderr, "err"); - assert_eq!(props.duration_ms, 99); - } - other => panic!("expected AgentCliTimedOut, got {other:?}"), - } - } - #[test] fn agent_acp_events_map_to_event_bodies_with_stage_scope() { let scope = StageScope { @@ -2131,12 +2039,10 @@ mod tests { let started = to_run_event_at( &fixtures::RUN_1, &Event::AgentAcpStarted { - node_id: "code".to_string(), - visit: 2, - mode: "acp".to_string(), - provider: "openai".to_string(), - model: "fake-acp".to_string(), - command: "python fake_agent.py".to_string(), + node_id: "code".to_string(), + visit: 2, + command: "python fake_agent.py".to_string(), + config_name: Some("fake".to_string()), }, Utc::now(), Some(&scope), @@ -2149,10 +2055,8 @@ mod tests { match &started.body { EventBody::AgentAcpStarted(props) => { assert_eq!(props.visit, 2); - assert_eq!(props.mode, "acp"); - assert_eq!(props.provider, "openai"); - assert_eq!(props.model, "fake-acp"); assert_eq!(props.command, "python fake_agent.py"); + assert_eq!(props.config_name.as_deref(), Some("fake")); } other => panic!("expected AgentAcpStarted, got {other:?}"), } diff --git a/lib/crates/fabro-workflow/src/event/events.rs b/lib/crates/fabro-workflow/src/event/events.rs index fc26008e1..ca6f97f2f 100644 --- a/lib/crates/fabro-workflow/src/event/events.rs +++ b/lib/crates/fabro-workflow/src/event/events.rs @@ -536,14 +536,6 @@ pub enum Event { output_bytes: u64, live_streaming: bool, }, - AgentCliStarted { - node_id: String, - visit: u32, - mode: String, - provider: String, - model: String, - command: String, - }, /// A top-level agent session object started its lifecycle. AgentSessionStarted { session_id: String, @@ -606,32 +598,12 @@ pub enum Event { #[serde(default, skip_serializing_if = "Option::is_none")] visit: Option, }, - AgentCliCompleted { - node_id: String, - stdout: String, - stderr: String, - exit_code: i32, - duration_ms: u64, - }, - AgentCliCancelled { - node_id: String, - stdout: String, - stderr: String, - duration_ms: u64, - }, - AgentCliTimedOut { - node_id: String, - stdout: String, - stderr: String, - duration_ms: u64, - }, AgentAcpStarted { - node_id: String, - visit: u32, - mode: String, - provider: String, - model: String, - command: String, + node_id: String, + visit: u32, + command: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + config_name: Option, }, AgentAcpCompleted { node_id: String, @@ -1356,22 +1328,6 @@ impl Event { "Command completed" ); } - Self::AgentCliStarted { - node_id, - provider, - model, - .. - } => { - debug!(node_id, provider, model, "Agent CLI started"); - } - Self::AgentCliCompleted { - node_id, - exit_code, - duration_ms, - .. - } => { - debug!(node_id, exit_code, duration_ms, "Agent CLI completed"); - } Self::AgentSessionStarted { session_id, provider, @@ -1412,27 +1368,13 @@ impl Event { Self::AgentSteerDropped { reason, count, .. } => { warn!(?reason, count, "Steer dropped"); } - Self::AgentCliCancelled { - node_id, - duration_ms, - .. - } => { - debug!(node_id, duration_ms, "Agent CLI cancelled"); - } - Self::AgentCliTimedOut { - node_id, - duration_ms, - .. - } => { - debug!(node_id, duration_ms, "Agent CLI timed out"); - } Self::AgentAcpStarted { node_id, - provider, - model, + command, + config_name, .. } => { - debug!(node_id, provider, model, "Agent ACP started"); + debug!(node_id, command, ?config_name, "Agent ACP started"); } Self::AgentAcpCompleted { node_id, diff --git a/lib/crates/fabro-workflow/src/event/names.rs b/lib/crates/fabro-workflow/src/event/names.rs index c5abfbc7c..02154926c 100644 --- a/lib/crates/fabro-workflow/src/event/names.rs +++ b/lib/crates/fabro-workflow/src/event/names.rs @@ -125,8 +125,6 @@ pub fn event_name(event: &Event) -> &'static str { Event::Failover { .. } => "agent.failover", Event::CommandStarted { .. } => "command.started", Event::CommandCompleted { .. } => "command.completed", - Event::AgentCliStarted { .. } => "agent.cli.started", - Event::AgentCliCompleted { .. } => "agent.cli.completed", Event::AgentSessionStarted { .. } => "agent.session.started", Event::AgentSessionActivated { .. } => "agent.session.activated", Event::AgentSessionDeactivated { .. } => "agent.session.deactivated", @@ -134,8 +132,6 @@ pub fn event_name(event: &Event) -> &'static str { Event::AgentInterruptInjected { .. } => "agent.interrupt.injected", Event::AgentSteerBuffered { .. } => "agent.steer.buffered", Event::AgentSteerDropped { .. } => "agent.steer.dropped", - Event::AgentCliCancelled { .. } => "agent.cli.cancelled", - Event::AgentCliTimedOut { .. } => "agent.cli.timed_out", Event::AgentAcpStarted { .. } => "agent.acp.started", Event::AgentAcpCompleted { .. } => "agent.acp.completed", Event::AgentAcpCancelled { .. } => "agent.acp.cancelled", diff --git a/lib/crates/fabro-workflow/src/event/stored_fields.rs b/lib/crates/fabro-workflow/src/event/stored_fields.rs index c640f0d39..7f31cc4a8 100644 --- a/lib/crates/fabro-workflow/src/event/stored_fields.rs +++ b/lib/crates/fabro-workflow/src/event/stored_fields.rs @@ -121,10 +121,6 @@ fn stored_event_fields_for_variant(event: &Event) -> StoredEventFields { | Event::PromptCompleted { node_id, .. } | Event::CommandStarted { node_id, .. } | Event::CommandCompleted { node_id, .. } - | Event::AgentCliStarted { node_id, .. } - | Event::AgentCliCompleted { node_id, .. } - | Event::AgentCliCancelled { node_id, .. } - | Event::AgentCliTimedOut { node_id, .. } | Event::AgentAcpCompleted { node_id, .. } | Event::AgentAcpCancelled { node_id, .. } | Event::AgentAcpTimedOut { node_id, .. } => node_stored_fields(Some(node_id.clone())), diff --git a/lib/crates/fabro-workflow/src/handler/llm/acp.rs b/lib/crates/fabro-workflow/src/handler/llm/acp.rs index 08bfd011f..02a21687c 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/acp.rs @@ -4,62 +4,28 @@ use std::collections::HashMap; use std::sync::Arc; use async_trait::async_trait; -use fabro_acp::{ - AcpCommandError, AcpError, AcpRunRequest, render_stop_reason, resolve_acp_command, -}; +use fabro_acp::{AcpCommandError, AcpError, AcpProcessSpec, AcpRunRequest, render_stop_reason}; use fabro_agent::{Sandbox, StaticEnvProvider, ToolEnvProvider}; -use fabro_auth::CredentialResolver; use fabro_graphviz::graph::Node; -use fabro_model::{Catalog, ProviderId}; use fabro_util::time::elapsed_ms; use tokio_util::sync::CancellationToken; use super::super::agent::{CodergenBackend, CodergenResult, CodergenRunRequest, OneShotRequest}; -use super::cli::AgentCli; -use super::launch_env::{AgentLaunchEnvRequest, resolve_agent_launch_env}; -use super::{changed_files, routing}; +use super::changed_files; use crate::error::Error; -use crate::event::{Emitter, Event, StageScope}; +use crate::event::{Emitter, Event, RunNoticeCode, RunNoticeLevel, StageScope}; pub struct AgentAcpBackend { - model: String, - provider_id: ProviderId, - tool_env: Option>, + tool_env: Option>, github_token_refresh_managed: bool, - resolver: Option, - catalog: Arc, } impl AgentAcpBackend { #[must_use] - pub fn new( - model: String, - provider_id: impl Into, - resolver: CredentialResolver, - ) -> Self { - let provider_id = provider_id.into(); - let catalog = default_catalog(); + pub fn new() -> Self { Self { - model, - provider_id, - tool_env: None, + tool_env: None, github_token_refresh_managed: false, - resolver: Some(resolver), - catalog, - } - } - - #[must_use] - pub fn new_from_env(model: String, provider_id: impl Into) -> Self { - let provider_id = provider_id.into(); - let catalog = default_catalog(); - Self { - model, - provider_id, - tool_env: None, - github_token_refresh_managed: false, - resolver: None, - catalog, } } @@ -80,12 +46,6 @@ impl AgentAcpBackend { self } - #[must_use] - pub fn with_catalog(mut self, catalog: Arc) -> Self { - self.catalog = catalog; - self - } - async fn run_turn( &self, node: &Node, @@ -95,53 +55,29 @@ impl AgentAcpBackend { sandbox: &Arc, cancel_token: CancellationToken, ) -> Result { - let files_before = changed_files::detect_changed_files(sandbox).await; - let model = node.model().unwrap_or(&self.model); - let provider = routing::resolve_node_provider_context( - self.catalog.as_ref(), - &self.provider_id, - &self.model, - node, - )?; - let provider_id = provider.provider_id; - let profile_kind = provider.profile_kind; - let command = - resolve_acp_command(node.acp_command()).map_err(acp_command_error_to_workflow)?; - - let launch_env = resolve_agent_launch_env(AgentLaunchEnvRequest { - provider_id: provider_id.clone(), - cli: AgentCli::for_profile_kind(profile_kind), - catalog: self.catalog.as_ref(), - resolver: self.resolver.as_ref(), - tool_env: self.tool_env.as_ref(), - github_token_refresh_managed: self.github_token_refresh_managed, - stage_label: "ACP", - emitter, - sandbox, - cancel_token: &cancel_token, - }) - .await?; + let process_spec = resolve_acp_process_spec(node)?; + let config_name = process_spec.name().map(str::to_string); + let launch_env = self.resolve_launch_env(emitter).await?; let on_activity = { let emitter = Arc::clone(emitter); Arc::new(move || emitter.touch()) as Arc }; - let command_display = command.to_string(); + let command_display = process_spec.to_string(); emitter.emit_scoped( &Event::AgentAcpStarted { - node_id: node.id.clone(), - visit: stage_scope.visit, - mode: "acp".to_string(), - provider: provider_id.to_string(), - model: model.to_string(), - command: command_display, + node_id: node.id.clone(), + visit: stage_scope.visit, + command: command_display, + config_name, }, stage_scope, ); + let files_before = changed_files::detect_changed_files(sandbox).await; let launch_start = std::time::Instant::now(); let result = match fabro_acp::run_acp_turn(AcpRunRequest { - command, + command: process_spec, prompt, cwd: sandbox.working_directory().to_string(), timeout_ms: node.timeout().map(crate::millis_u64), @@ -224,10 +160,33 @@ impl AgentAcpBackend { last_file_touched, }) } + + async fn resolve_launch_env( + &self, + emitter: &Arc, + ) -> Result, Error> { + let Some(provider) = &self.tool_env else { + return Ok(HashMap::new()); + }; + if self.github_token_refresh_managed { + emitter.notice( + RunNoticeLevel::Info, + RunNoticeCode::GithubTokenRefreshLimited, + "ACP agent stages receive workflow env at process launch; stages running beyond \ + token expiry may need to be retried.", + ); + } + provider + .resolve() + .await + .map_err(|err| Error::handler_with_anyhow("Failed to resolve ACP agent env", err)) + } } -fn default_catalog() -> Arc { - Arc::new(Catalog::from_builtin().expect("default catalog should build")) +impl Default for AgentAcpBackend { + fn default() -> Self { + Self::new() + } } #[async_trait] @@ -245,38 +204,46 @@ impl CodergenBackend for AgentAcpBackend { .await } - async fn one_shot(&self, request: OneShotRequest<'_>) -> Result { - let prompt = match request.system_prompt.filter(|prompt| !prompt.is_empty()) { - Some(system_prompt) => format!("System:\n{system_prompt}\n\nUser:\n{}", request.prompt), - None => request.prompt.to_string(), - }; - self.run_turn( - request.node, - prompt, - request.emitter, - request.stage_scope, - request.sandbox, - request.cancel_token, - ) - .await + async fn one_shot(&self, _request: OneShotRequest<'_>) -> Result { + Err(Error::Validation( + "backend=\"acp\" is only valid on agent nodes; prompt nodes are API-only".to_string(), + )) } } -fn acp_command_error_to_workflow(error: AcpCommandError) -> Error { +fn acp_process_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::LegacyCommandAttribute => { + Error::handler("acp_command is no longer supported; use acp.command or acp.config") + } + AcpCommandError::EmptyOverride => Error::handler("ACP process attribute must not be empty"), + AcpCommandError::MissingOverride => { + Error::handler("backend=\"acp\" requires exactly one of acp.command or acp.config") + } AcpCommandError::UnsupportedTransport => { Error::handler("only stdio ACP commands are supported") } - AcpCommandError::Parse(source) => { - Error::handler_with_source("Failed to resolve ACP command", source) + AcpCommandError::InvalidCommandString => { + Error::handler("Failed to parse acp.command as a shell command") + } + AcpCommandError::InvalidConfigJson(source) => { + Error::handler_with_source("Failed to parse acp.config as JSON", source) + } + AcpCommandError::InvalidConfigShape(message) => { + Error::handler(format!("Invalid acp.config shape: {message}")) } } } +fn resolve_acp_process_spec(node: &Node) -> Result { + AcpProcessSpec::from_attrs( + node.legacy_acp_command_attr(), + node.acp_command_attr(), + node.acp_config_attr(), + ) + .map_err(acp_process_error_to_workflow) +} + fn acp_error_to_workflow(error: AcpError) -> Error { match error { AcpError::Cancelled => Error::Cancelled, @@ -307,17 +274,14 @@ mod tests { use fabro_acp::{AcpError, AcpProcessExit}; use fabro_agent::{LocalSandbox, Sandbox, shell_quote}; use fabro_graphviz::graph::{AttrValue, Node}; - use fabro_model::ProviderId; use fabro_sandbox::test_support::MockSandbox; use fabro_types::{CommandTermination, EventBody, ExecOutputTail}; use tokio_util::sync::CancellationToken; use super::{AgentAcpBackend, acp_error_to_workflow}; use crate::context::Context; - use crate::event::{Emitter, StageScope}; - use crate::handler::agent::{ - CodergenBackend, CodergenResult, CodergenRunRequest, OneShotRequest, - }; + use crate::event::Emitter; + use crate::handler::agent::{CodergenBackend, CodergenResult, CodergenRunRequest}; #[tokio::test] async fn acp_backend_run_sends_prompt_and_returns_text() { @@ -329,28 +293,20 @@ mod tests { .unwrap(); let mut node = Node::new("work"); - node.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); - node.attrs.insert( - "model".to_string(), - AttrValue::String("fake-acp".to_string()), - ); node.attrs .insert("backend".to_string(), AttrValue::String("acp".to_string())); node.attrs.insert( - "acp_command".to_string(), + "acp.command".to_string(), AttrValue::String(format!( "python3 {}", shell_quote(&script_path.to_string_lossy()) )), ); - let backend = - AgentAcpBackend::new_from_env("fake-acp".to_string(), ProviderId::openai()).with_env( - HashMap::from([("ACP_MODE".to_string(), "write_file".to_string())]), - ); + let backend = AgentAcpBackend::new().with_env(HashMap::from([( + "ACP_MODE".to_string(), + "write_file".to_string(), + )])); let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.path().to_path_buf())); let emitter = Arc::new(Emitter::default()); let context = Context::new(); @@ -381,63 +337,93 @@ mod tests { } #[tokio::test] - async fn acp_backend_one_shot_combines_system_prompt_and_uses_passed_sandbox() { + async fn acp_backend_accepts_acp_command_attribute_without_model_or_provider() { let tempdir = tempfile::tempdir().unwrap(); + init_git(tempdir.path()); let script_path = tempdir.path().join("fake_acp_agent.py"); - let prompt_record_path = tempdir.path().join("prompt.json"); tokio::fs::write(&script_path, fake_acp_agent_script()) .await .unwrap(); - let mut node = Node::new("prompt"); - node.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); + let mut node = Node::new("work"); node.attrs .insert("backend".to_string(), AttrValue::String("acp".to_string())); node.attrs.insert( - "acp_command".to_string(), + "acp.command".to_string(), AttrValue::String(format!( "python3 {}", shell_quote(&script_path.to_string_lossy()) )), ); - let backend = AgentAcpBackend::new_from_env("fake-acp".to_string(), ProviderId::openai()) - .with_env(HashMap::from([ - ( - "ACP_PROMPT_RECORD".to_string(), - prompt_record_path.to_string_lossy().into_owned(), - ), - ("ACP_MODE".to_string(), "write_file".to_string()), - ])); + let backend = AgentAcpBackend::new().with_env(HashMap::from([( + "ACP_MODE".to_string(), + "write_file".to_string(), + )])); let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.path().to_path_buf())); let emitter = Arc::new(Emitter::default()); let context = Context::new(); - let stage_scope = StageScope::for_handler(&context, "prompt"); let result = backend - .one_shot(OneShotRequest { - node: &node, - prompt: "User prompt", - system_prompt: Some("System prompt"), - emitter: &emitter, - stage_scope: &stage_scope, - sandbox: &sandbox, - cancel_token: CancellationToken::new(), + .run(CodergenRunRequest { + node: &node, + prompt: "write hello", + context: &context, + thread_id: None, + emitter: &emitter, + sandbox: &sandbox, + tool_hooks: None, + cancel_token: CancellationToken::new(), }) .await .unwrap(); - assert!(matches!(result, CodergenResult::Text { .. })); - let recorded = tokio::fs::read_to_string(prompt_record_path).await.unwrap(); - assert!(recorded.contains("System:\\nSystem prompt\\n\\nUser:\\nUser prompt")); - assert_eq!( - tokio::fs::read_to_string(tempdir.path().join("hello.txt")) - .await - .unwrap(), - "hello from sandbox\n" + let CodergenResult::Text { text, .. } = result else { + panic!("expected text result"); + }; + assert_eq!(text, "hello from acp"); + } + + #[tokio::test] + async fn acp_backend_does_not_forward_provider_credentials() { + let mut sandbox = MockSandbox::linux(); + sandbox.stdio_process_error = Some("stop before ACP handshake".to_string()); + let sandbox = Arc::new(sandbox); + let sandbox_dyn: Arc = sandbox.clone(); + + 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("fake-acp-agent".to_string()), ); + + let backend = AgentAcpBackend::new(); + let emitter = Arc::new(Emitter::default()); + let context = Context::new(); + let result = backend + .run(CodergenRunRequest { + node: &node, + prompt: "write hello", + context: &context, + thread_id: None, + emitter: &emitter, + sandbox: &sandbox_dyn, + tool_hooks: None, + cancel_token: CancellationToken::new(), + }) + .await; + assert!(result.is_err()); + + let captured = sandbox + .captured_env_vars + .lock() + .expect("captured env lock poisoned") + .clone() + .unwrap_or_default(); + assert!(!captured.contains_key("OPENAI_API_KEY")); + assert!(!captured.contains_key("ANTHROPIC_API_KEY")); + assert!(!captured.contains_key("GEMINI_API_KEY")); } #[tokio::test] @@ -450,21 +436,17 @@ mod tests { let mut node = Node::new("work"); node.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); - node.attrs.insert( - "acp_command".to_string(), + "acp.command".to_string(), AttrValue::String(format!( "python3 {}", shell_quote(&script_path.to_string_lossy()) )), ); - let backend = - AgentAcpBackend::new_from_env("fake-acp".to_string(), ProviderId::openai()).with_env( - HashMap::from([("ACP_STOP_REASON".to_string(), "cancelled".to_string())]), - ); + let backend = AgentAcpBackend::new().with_env(HashMap::from([( + "ACP_STOP_REASON".to_string(), + "cancelled".to_string(), + )])); let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.path().to_path_buf())); let emitter = Arc::new(Emitter::default()); let context = Context::new(); @@ -506,16 +488,12 @@ mod tests { }) .to_string(); 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())); node.attrs - .insert("acp_command".to_string(), AttrValue::String(raw_command)); + .insert("acp.config".to_string(), AttrValue::String(raw_command)); - let backend = AgentAcpBackend::new_from_env("fake-acp".to_string(), ProviderId::openai()); + let backend = AgentAcpBackend::new(); let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.path().to_path_buf())); let emitter = Arc::new(Emitter::default()); let events = Arc::new(Mutex::new(Vec::new())); @@ -554,20 +532,16 @@ mod tests { } #[tokio::test] - async fn acp_backend_requires_explicit_acp_command() { + async fn acp_backend_requires_explicit_process_attr() { let sandbox = MockSandbox::linux(); 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(), ProviderId::openai()); + let backend = AgentAcpBackend::new(); let emitter = Arc::new(Emitter::default()); let context = Context::new(); let result = backend @@ -583,11 +557,11 @@ mod tests { }) .await; let Err(err) = result else { - panic!("ACP without acp_command should fail"); + panic!("ACP without process attr should fail"); }; assert!( err.to_string() - .contains("acp_command is required for backend=\"acp\"") + .contains("requires exactly one of acp.command or acp.config") ); assert!( sandbox @@ -595,7 +569,7 @@ mod tests { .lock() .expect("captured env lock poisoned") .is_none(), - "ACP process should not launch when acp_command is missing" + "ACP process should not launch when process attr is missing" ); } @@ -609,21 +583,17 @@ mod tests { 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())); node.attrs.insert( - "acp_command".to_string(), + "acp.command".to_string(), AttrValue::String("fake-acp-agent".to_string()), ); - let backend = - AgentAcpBackend::new_from_env("fake-acp".to_string(), ProviderId::openai()).with_env( - HashMap::from([("OPENAI_API_KEY".to_string(), "test-key".to_string())]), - ); + let backend = AgentAcpBackend::new().with_env(HashMap::from([( + "WORKFLOW_ENV".to_string(), + "test-value".to_string(), + )])); let emitter = Arc::new(Emitter::default()); let context = Context::new(); let result = backend diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs deleted file mode 100644 index 5b070b18b..000000000 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ /dev/null @@ -1,1568 +0,0 @@ -//! CLI agent stages resolve workflow tool env once when launching the external -//! CLI process. Long-running CLI stages do not observe later GitHub -//! installation token refreshes until a future credential-helper integration -//! moves token lookup inside the child process. - -use std::collections::HashMap; -use std::sync::{Arc, Mutex}; - -use async_trait::async_trait; -use fabro_agent::{Sandbox, StaticEnvProvider, ToolEnvProvider, shell_quote}; -use fabro_auth::CredentialResolver; -use fabro_graphviz::graph::Node; -use fabro_llm::types::TokenCounts; -use fabro_model::{AgentProfileKind, Catalog, ModelRef, ProviderId}; -use fabro_types::settings::run::RunModelControls; -use fabro_types::{CommandOutputStream, CommandTermination, LlmBackend}; -use fabro_util::time::elapsed_ms; -use tokio_util::sync::CancellationToken; - -/// Returns up to the last `n` characters of `s`, preserving char boundaries. -fn tail_chars(s: &str, n: usize) -> String { - let total = s.chars().count(); - if total <= n { - return s.to_string(); - } - s.chars().skip(total - n).collect() -} - -/// Build a "\nstdout: " detail string for CLI failure -/// messages, falling back to the original command when both streams are empty. -fn cli_failure_detail(stdout: &str, stderr: &str, command: &str) -> String { - let stderr_tail = tail_chars(stderr, 500); - let stdout_tail = tail_chars(stdout, 500); - match (stderr_tail.is_empty(), stdout_tail.is_empty()) { - (false, false) => format!("{stderr_tail}\nstdout: {stdout_tail}"), - (false, true) => stderr_tail, - (true, false) => format!("stdout: {stdout_tail}"), - (true, true) => format!("command: {command}"), - } -} - -use super::super::agent::{CodergenBackend, CodergenResult, CodergenRunRequest, OneShotRequest}; -use super::acp::AgentAcpBackend; -use super::api::effective_request_controls; -use super::launch_env::{AgentLaunchEnvRequest, resolve_agent_launch_env}; -use super::{changed_files, routing}; -use crate::error::Error; -use crate::event::{Emitter, Event, StageScope}; -use crate::outcome::billed_model_usage_from_llm; - -/// Maps a provider to its corresponding CLI tool metadata. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum AgentCli { - Claude, - Codex, - Gemini, -} - -impl AgentCli { - pub fn for_profile_kind(profile_kind: AgentProfileKind) -> Self { - match profile_kind { - AgentProfileKind::Anthropic => Self::Claude, - AgentProfileKind::OpenAi => Self::Codex, - AgentProfileKind::Gemini => Self::Gemini, - } - } - - pub fn name(self) -> &'static str { - match self { - Self::Claude => "claude", - Self::Codex => "codex", - Self::Gemini => "gemini", - } - } -} - -/// 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, - sandbox: &Arc, - cancel_token: &CancellationToken, -) -> Result<(), Error> { - let cli_name = cli.name(); - - let availability_check = sandbox - .exec_command( - &format!("PATH=\"$HOME/.local/bin:$PATH\" command -v {cli_name}"), - 30_000, - None, - None, - Some(cancel_token.child_token()), - ) - .await - .map_err(|e| { - Error::handler_with_source(format!("Failed to check {cli_name} availability"), e) - })?; - - if availability_check.is_success() { - return 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). -const CLI_ONLY_MODELS: &[&str] = &[]; - -/// Returns true if the given model is only available through a CLI tool. -#[must_use] -pub fn is_cli_only_model(model: &str) -> bool { - CLI_ONLY_MODELS.contains(&model) -} - -/// Build the CLI command string for a given agent profile. -/// -/// The `prompt_file` is the path to a file containing the prompt text, which -/// is piped into the command's stdin via `cat`. -#[must_use] -pub fn cli_command_for_profile_kind( - profile_kind: AgentProfileKind, - model: &str, - prompt_file: &str, -) -> String { - let prompt_file = shell_quote(prompt_file); - let cli = AgentCli::for_profile_kind(profile_kind); - let model_flag = if model.is_empty() { - String::new() - } else { - let model = shell_quote(model); - match cli { - AgentCli::Codex | AgentCli::Gemini => format!(" -m {model}"), - AgentCli::Claude => format!(" --model {model}"), - } - }; - // Use `cat | command` instead of `command < file` because the background - // launch wrapper (`setsid sh -c '...' { - format!("cat {prompt_file} | codex exec --json --full-auto{model_flag}") - } - // --yolo: auto-approve all tool calls - AgentCli::Gemini => format!("cat {prompt_file} | gemini -o json --yolo{model_flag}"), - // --dangerously-skip-permissions: bypass all permission checks (required for - // non-interactive use). CLAUDECODE= unset to allow running inside a Claude Code - // session. - AgentCli::Claude => format!( - "cat {prompt_file} | CLAUDECODE= claude -p --verbose --output-format stream-json --dangerously-skip-permissions{model_flag}" - ), - } -} - -/// Parsed response from a CLI tool invocation. -#[derive(Debug)] -pub struct CliResponse { - pub text: String, - pub input_tokens: i64, - pub output_tokens: i64, -} - -/// Parse NDJSON output from Claude CLI (`--output-format stream-json`). -/// -/// Looks for the last `{"type":"result",...}` line, extracts `result` text and -/// `usage`. -fn parse_claude_ndjson(output: &str) -> Option { - let mut last_result: Option = None; - - for line in output.lines() { - let line = line.trim(); - if line.is_empty() { - continue; - } - if let Ok(value) = serde_json::from_str::(line) { - if value.get("type").and_then(|t| t.as_str()) == Some("result") { - last_result = Some(value); - } - } - } - - let result = last_result?; - let text = result - .get("result") - .and_then(|v| v.as_str()) - .unwrap_or("") - .to_string(); - let input_tokens = result - .pointer("/usage/input_tokens") - .and_then(serde_json::Value::as_i64) - .unwrap_or(0); - let output_tokens = result - .pointer("/usage/output_tokens") - .and_then(serde_json::Value::as_i64) - .unwrap_or(0); - - Some(CliResponse { - text, - input_tokens, - output_tokens, - }) -} - -/// Parse NDJSON output from Codex CLI (`codex exec --json`). -/// -/// Codex emits NDJSON lines. Text comes from `item.completed` events where -/// `item.type == "agent_message"`. TokenCounts comes from the `turn.completed` -/// event. -fn parse_codex_ndjson(output: &str) -> Option { - let mut last_message_text = String::new(); - let mut input_tokens: i64 = 0; - let mut output_tokens: i64 = 0; - let mut found_anything = false; - - for line in output.lines() { - let line = line.trim(); - if line.is_empty() { - continue; - } - let value: serde_json::Value = match serde_json::from_str(line) { - Ok(v) => v, - Err(_) => continue, - }; - - let event_type = value.get("type").and_then(|t| t.as_str()).unwrap_or(""); - - match event_type { - "item.completed" => { - let item_type = value - .pointer("/item/type") - .and_then(|t| t.as_str()) - .unwrap_or(""); - if item_type == "agent_message" { - if let Some(text) = value.pointer("/item/text").and_then(|t| t.as_str()) { - last_message_text = text.to_string(); - found_anything = true; - } - } - } - "turn.completed" => { - input_tokens = value - .pointer("/usage/input_tokens") - .and_then(serde_json::Value::as_i64) - .unwrap_or(0); - output_tokens = value - .pointer("/usage/output_tokens") - .and_then(serde_json::Value::as_i64) - .unwrap_or(0); - found_anything = true; - } - _ => {} - } - } - - if !found_anything { - return None; - } - - Some(CliResponse { - text: last_message_text, - input_tokens, - output_tokens, - }) -} - -/// Parse JSON output from Gemini CLI (`-o json`). -/// -/// Gemini outputs a single JSON object with `response` for text and -/// `stats.models..tokens` for usage. -fn parse_gemini_json(output: &str) -> Option { - let value: serde_json::Value = serde_json::from_str(output.trim()).ok()?; - let text = value - .get("response") - .and_then(|v| v.as_str()) - .unwrap_or("") - .to_string(); - - // Extract tokens from the first model in stats.models - let (input_tokens, output_tokens) = value - .pointer("/stats/models") - .and_then(|m| m.as_object()) - .and_then(|models| models.values().next()) - .map_or((0, 0), |model_stats| { - let input = model_stats - .pointer("/tokens/input") - .and_then(serde_json::Value::as_i64) - .unwrap_or(0); - let output = model_stats - .pointer("/tokens/candidates") - .and_then(serde_json::Value::as_i64) - .unwrap_or(0); - (input, output) - }); - - Some(CliResponse { - text, - input_tokens, - output_tokens, - }) -} - -/// Parse CLI output, choosing the right parser based on profile behavior. -pub fn parse_cli_response(profile_kind: AgentProfileKind, output: &str) -> Option { - match AgentCli::for_profile_kind(profile_kind) { - AgentCli::Codex => parse_codex_ndjson(output), - AgentCli::Gemini => parse_gemini_json(output), - AgentCli::Claude => parse_claude_ndjson(output), - } -} - -/// CLI backend that invokes external CLI tools (claude, codex, gemini) via -/// `exec_command()`. -pub struct AgentCliBackend { - model: String, - provider_id: ProviderId, - tool_env: Option>, - github_token_refresh_managed: bool, - resolver: Option, - run_model_controls: RunModelControls, - catalog: Arc, -} - -impl AgentCliBackend { - #[must_use] - pub fn new( - model: String, - provider_id: impl Into, - resolver: CredentialResolver, - ) -> Self { - let provider_id = provider_id.into(); - let catalog = default_catalog(); - Self { - model, - provider_id, - tool_env: None, - github_token_refresh_managed: false, - resolver: Some(resolver), - run_model_controls: RunModelControls::default(), - catalog, - } - } - - #[must_use] - pub fn new_from_env(model: String, provider_id: impl Into) -> Self { - let provider_id = provider_id.into(); - let catalog = default_catalog(); - Self { - model, - provider_id, - tool_env: None, - github_token_refresh_managed: false, - resolver: None, - run_model_controls: RunModelControls::default(), - catalog, - } - } - - #[must_use] - pub fn with_env(mut self, env: HashMap) -> Self { - self.tool_env = Some(Arc::new(StaticEnvProvider(env))); - self - } - - #[must_use] - pub fn with_tool_env_provider( - mut self, - provider: Arc, - github_token_refresh_managed: bool, - ) -> Self { - self.tool_env = Some(provider); - self.github_token_refresh_managed = github_token_refresh_managed; - self - } - - #[must_use] - pub fn with_run_model_controls(mut self, controls: RunModelControls) -> Self { - self.run_model_controls = controls; - self - } - - #[must_use] - pub fn with_catalog(mut self, catalog: Arc) -> Self { - self.catalog = catalog; - self - } -} - -fn default_catalog() -> Arc { - Arc::new(Catalog::from_builtin().expect("default catalog should build")) -} - -#[async_trait] -impl CodergenBackend for AgentCliBackend { - async fn run(&self, request: CodergenRunRequest<'_>) -> Result { - let node = request.node; - let prompt = request.prompt; - let context = request.context; - let emitter = request.emitter; - let sandbox = request.sandbox; - let cancel_token = request.cancel_token; - - // 1. Snapshot git state before the CLI run - let files_before = changed_files::detect_changed_files(sandbox).await; - - // 2. Generate unique paths for this run - let run_id = uuid::Uuid::new_v4().to_string(); - let tmp_prefix = format!("/tmp/fabro_cli_{run_id}"); - let prompt_path = format!("{tmp_prefix}_prompt.txt"); - let env_path = format!("{tmp_prefix}_env.sh"); - - sandbox - .write_file(&prompt_path, prompt) - .await - .map_err(|e| Error::handler_with_source("Failed to write prompt file", e))?; - - // 3. Build CLI command - let model = node.model().unwrap_or(&self.model); - let provider = routing::resolve_node_provider_context( - self.catalog.as_ref(), - &self.provider_id, - &self.model, - node, - )?; - let provider_id = provider.provider_id; - let profile_kind = provider.profile_kind; - let controls = effective_request_controls(&self.run_model_controls, node)?; - - let cli = AgentCli::for_profile_kind(profile_kind); - verify_cli_available(cli, sandbox, &cancel_token).await?; - - let command = cli_command_for_profile_kind(profile_kind, model, &prompt_path); - let stage_scope = StageScope::for_handler(context, &node.id); - emitter.emit_scoped( - &Event::AgentCliStarted { - node_id: node.id.clone(), - visit: stage_scope.visit, - mode: "cli".to_string(), - provider: provider_id.to_string(), - model: model.to_string(), - command: command.clone(), - }, - &stage_scope, - ); - - let launch_env = resolve_agent_launch_env(AgentLaunchEnvRequest { - provider_id: provider_id.clone(), - cli, - catalog: self.catalog.as_ref(), - resolver: self.resolver.as_ref(), - tool_env: self.tool_env.as_ref(), - github_token_refresh_managed: self.github_token_refresh_managed, - stage_label: "CLI", - emitter, - sandbox, - cancel_token: &cancel_token, - }) - .await?; - - // Write env file so the inner shell that runs the CLI command picks up - // PATH and provider env vars; we still pass `launch_env` to - // `exec_command_streaming` for parity. - let mut env_lines: Vec = vec!["export PATH=\"$HOME/.local/bin:$PATH\"".to_string()]; - env_lines.extend( - launch_env - .iter() - .map(|(k, v)| format!("export {k}={}", shell_quote(v))), - ); - sandbox - .write_file(&env_path, &env_lines.join("\n")) - .await - .map_err(|e| Error::handler_with_source("Failed to write env file", e))?; - - // Disable auto-stop so the sandbox stays alive during long CLI runs. - if let Err(e) = sandbox.set_autostop_interval(0).await { - tracing::warn!("Failed to disable sandbox auto-stop: {e}"); - } - - // Stream the CLI command directly: the previous detached `setsid &` - // launcher could not be cancelled mid-flight. By running through - // `exec_command_streaming` the run-level cancel token (and node - // timeout, when set) terminate the CLI and its descendants. - let outer_command = format!(". {} && {command}", shell_quote(&env_path)); - // Use a synchronous Mutex: each callback invocation only does a short - // `extend_from_slice` with no awaits while the lock is held, so an - // async Mutex would just add per-chunk scheduling overhead. - let stdout_buffer: Arc>> = Arc::new(Mutex::new(Vec::new())); - let stderr_buffer: Arc>> = Arc::new(Mutex::new(Vec::new())); - let stdout_buf_cb = Arc::clone(&stdout_buffer); - let stderr_buf_cb = Arc::clone(&stderr_buffer); - let emitter_for_callback = Arc::clone(emitter); - let output_callback: fabro_agent::CommandOutputCallback = Arc::new(move |stream, bytes| { - let stdout_buf = Arc::clone(&stdout_buf_cb); - let stderr_buf = Arc::clone(&stderr_buf_cb); - let emitter = Arc::clone(&emitter_for_callback); - Box::pin(async move { - // Touch the stall watchdog whenever the CLI emits output - // so long-running invocations don't trip stall timeout. - emitter.touch(); - let buf = match stream { - CommandOutputStream::Stdout => stdout_buf, - CommandOutputStream::Stderr => stderr_buf, - }; - buf.lock() - .expect("CLI output buffer mutex poisoned") - .extend_from_slice(&bytes); - Ok(()) - }) - }); - let launch_env_ref = if launch_env.is_empty() { - None - } else { - Some(&launch_env) - }; - let timeout_ms = node.timeout().map(crate::millis_u64); - let invocation_token = cancel_token.child_token(); - let launch_start = std::time::Instant::now(); - let streaming_result = sandbox - .exec_command_streaming( - &outer_command, - timeout_ms, - None, - launch_env_ref, - Some(invocation_token.clone()), - output_callback, - ) - .await; - - let cleanup_temp_files = || { - let sandbox = Arc::clone(sandbox); - let cleanup_cmd = format!("rm -f {}_*", shell_quote(&tmp_prefix)); - async move { - let _ = sandbox - .exec_command(&cleanup_cmd, 30_000, None, None, None) - .await; - } - }; - - let streaming = match streaming_result { - Ok(streaming) => streaming, - Err(err) => { - cleanup_temp_files().await; - return Err(Error::handler_with_source("Failed to run CLI command", err)); - } - }; - let result = streaming.result; - // Prefer the buffered streaming output (live chunks); fall back to the - // result struct for sandboxes that bundle output at the end. - let buffered_stdout = { - let buf = stdout_buffer - .lock() - .expect("CLI stdout buffer mutex poisoned"); - String::from_utf8_lossy(&buf).into_owned() - }; - let buffered_stderr = { - let buf = stderr_buffer - .lock() - .expect("CLI stderr buffer mutex poisoned"); - String::from_utf8_lossy(&buf).into_owned() - }; - let stdout = if buffered_stdout.is_empty() { - result.stdout.clone() - } else { - buffered_stdout - }; - let stderr = if buffered_stderr.is_empty() { - result.stderr.clone() - } else { - buffered_stderr - }; - let duration_ms = elapsed_ms(launch_start); - - match result.termination { - CommandTermination::Cancelled => { - emitter.emit_scoped( - &Event::AgentCliCancelled { - node_id: node.id.clone(), - stdout: stdout.clone(), - stderr: stderr.clone(), - duration_ms, - }, - &stage_scope, - ); - cleanup_temp_files().await; - return Err(Error::Cancelled); - } - CommandTermination::TimedOut => { - emitter.emit_scoped( - &Event::AgentCliTimedOut { - node_id: node.id.clone(), - stdout: stdout.clone(), - stderr: stderr.clone(), - duration_ms, - }, - &stage_scope, - ); - cleanup_temp_files().await; - let detail = cli_failure_detail(&stdout, &stderr, &command); - return Err(Error::handler(format!( - "CLI command timed out after {duration_ms} ms: {detail}" - ))); - } - CommandTermination::Exited => { - emitter.emit_scoped( - &Event::AgentCliCompleted { - node_id: node.id.clone(), - stdout: stdout.clone(), - stderr: stderr.clone(), - exit_code: result.exit_code.unwrap_or(-1), - duration_ms, - }, - &stage_scope, - ); - } - } - - // Cleanup temp files (Exited path). - cleanup_temp_files().await; - - let exited_success = - result.termination == CommandTermination::Exited && result.exit_code == Some(0); - if !exited_success { - let detail = cli_failure_detail(&stdout, &stderr, &command); - return Err(Error::handler(format!( - "CLI command exited with code {}: {detail}", - result - .exit_code - .map_or_else(|| "".to_string(), |c| c.to_string()), - ))); - } - - // 4. Parse the CLI output - let parsed = parse_cli_response(profile_kind, &stdout) - .ok_or_else(|| Error::handler("Failed to parse CLI output".to_string()))?; - - // 5. Detect changed files - let (files_touched, last_file_touched) = - changed_files::files_touched_since(sandbox, &files_before).await; - - let stage_usage = billed_model_usage_from_llm( - self.catalog.as_ref(), - &ModelRef { - provider: provider_id, - model_id: model.to_string(), - speed: controls.speed, - }, - &TokenCounts { - input_tokens: parsed.input_tokens, - output_tokens: parsed.output_tokens, - ..TokenCounts::default() - }, - )?; - - Ok(CodergenResult::Text { - text: parsed.text, - usage: Some(stage_usage), - files_touched, - last_file_touched, - }) - } -} - -#[expect( - clippy::disallowed_methods, - reason = "CLI agent fallback credentials intentionally read provider API-key env vars." -)] -pub(crate) fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - -/// Routes codergen invocations to API, CLI, or ACP backends based on node -/// attributes and model type. -pub struct BackendRouter { - api: Box, - cli: AgentCliBackend, - acp: AgentAcpBackend, -} - -impl BackendRouter { - #[must_use] - pub fn new( - api_backend: Box, - cli_backend: AgentCliBackend, - acp_backend: AgentAcpBackend, - ) -> Self { - Self { - api: api_backend, - cli: cli_backend, - acp: acp_backend, - } - } - - fn select_backend(node: &Node) -> Result { - routing::select_run_backend(node) - } - - fn select_one_shot_backend(node: &Node) -> Result { - routing::select_one_shot_backend(node) - } - - #[cfg(test)] - fn should_use_cli(node: &Node) -> bool { - matches!(Self::select_backend(node), Ok(LlmBackend::Cli)) - } -} - -#[async_trait] -impl CodergenBackend for BackendRouter { - async fn run(&self, request: CodergenRunRequest<'_>) -> Result { - match Self::select_backend(request.node)? { - LlmBackend::Api => self.api.run(request).await, - LlmBackend::Cli => self.cli.run(request).await, - LlmBackend::Acp => self.acp.run(request).await, - } - } - - async fn one_shot(&self, request: OneShotRequest<'_>) -> Result { - match Self::select_one_shot_backend(request.node)? { - LlmBackend::Acp => self.acp.one_shot(request).await, - LlmBackend::Api | LlmBackend::Cli => self.api.one_shot(request).await, - } - } - - async fn shutdown(&self, emitter: &Arc) { - self.api.shutdown(emitter).await; - } -} - -#[cfg(test)] -mod tests { - use std::path::Path; - - use fabro_agent::LocalSandbox; - use fabro_agent::sandbox::ExecResult; - use fabro_graphviz::graph::AttrValue; - use fabro_model::ProviderId; - - use super::*; - use crate::context::Context; - - // -- AgentCli -- - - #[test] - fn agent_cli_for_provider() { - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::Anthropic), - AgentCli::Claude - ); - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::OpenAi), - AgentCli::Codex - ); - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::Gemini), - AgentCli::Gemini - ); - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::OpenAi), - AgentCli::Codex - ); - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::OpenAi), - AgentCli::Codex - ); - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::OpenAi), - AgentCli::Codex - ); - assert_eq!( - AgentCli::for_profile_kind(AgentProfileKind::OpenAi), - AgentCli::Codex - ); - } - - #[test] - fn agent_cli_name() { - assert_eq!(AgentCli::Claude.name(), "claude"); - assert_eq!(AgentCli::Codex.name(), "codex"); - assert_eq!(AgentCli::Gemini.name(), "gemini"); - } - - // -- verify_cli_available -- - - use std::collections::VecDeque; - use std::sync::Mutex; - - use fabro_acp::test_support::fake_acp_agent_script; - use fabro_agent::sandbox::{DirEntry, GrepOptions}; - - /// Mock sandbox that returns pre-configured ExecResults in FIFO order. - struct CliMockSandbox { - results: Mutex>, - commands: Arc>>, - } - - impl CliMockSandbox { - fn new(results: Vec, commands: Arc>>) -> Self { - Self { - results: Mutex::new(results.into()), - commands, - } - } - } - - #[async_trait] - impl Sandbox for CliMockSandbox { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { - Ok(String::new()) - } - async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { - Ok(false) - } - async fn list_directory( - &self, - _path: &str, - _depth: Option, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn exec_command( - &self, - command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&std::collections::HashMap>, - _cancel_token: Option, - ) -> fabro_sandbox::Result { - self.commands.lock().unwrap().push(command.to_string()); - self.results - .lock() - .unwrap() - .pop_front() - .ok_or_else(|| fabro_sandbox::Error::message("no more mock results")) - } - async fn grep( - &self, - _pattern: &str, - _path: &str, - _options: &GrepOptions, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn glob( - &self, - _pattern: &str, - _path: Option<&str>, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn download_file_to_local( - &self, - _remote: &str, - _local: &Path, - ) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn upload_file_from_local( - &self, - _local: &Path, - _remote: &str, - ) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn initialize(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn cleanup(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - fn working_directory(&self) -> &str { - "/workspace" - } - fn platform(&self) -> &str { - "linux" - } - fn os_version(&self) -> String { - "Ubuntu 22.04".to_string() - } - async fn set_autostop_interval(&self, _minutes: i32) -> fabro_sandbox::Result<()> { - Ok(()) - } - } - - fn ok_result() -> ExecResult { - ExecResult { - exit_code: Some(0), - termination: CommandTermination::Exited, - stdout: String::new(), - stderr: String::new(), - duration_ms: 10, - } - } - - fn fail_result(code: i32) -> ExecResult { - fail_result_with_output(code, "", "error") - } - - fn fail_result_with_output(code: i32, stdout: &str, stderr: &str) -> ExecResult { - ExecResult { - exit_code: Some(code), - termination: CommandTermination::Exited, - stdout: stdout.to_string(), - stderr: stderr.to_string(), - duration_ms: 10, - } - } - - #[tokio::test] - 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 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("command -v claude")); - } - - #[tokio::test] - async fn verify_cli_available_fails_when_missing_without_installing() { - let commands = Arc::new(Mutex::new(Vec::new())); - let sandbox: Arc = Arc::new(CliMockSandbox::new( - vec![fail_result(127)], - Arc::clone(&commands), - )); - - 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(), 1); - assert!(commands[0].contains("command -v claude")); - assert!( - !commands - .iter() - .any(|command| command.contains("npm install")) - ); - } - - // -- Cycle 1: cli_command_for_profile_kind -- - - #[test] - fn cli_command_for_codex() { - let cmd = cli_command_for_profile_kind( - AgentProfileKind::OpenAi, - "gpt-5.3-codex", - "/tmp/prompt.txt", - ); - assert!(cmd.starts_with("cat /tmp/prompt.txt | codex exec --json --full-auto")); - assert!(cmd.contains("-m gpt-5.3-codex")); - } - - #[test] - fn cli_command_for_claude() { - let cmd = cli_command_for_profile_kind( - AgentProfileKind::Anthropic, - "claude-opus-4-6", - "/tmp/prompt.txt", - ); - assert!(cmd.starts_with("cat /tmp/prompt.txt |")); - assert!(cmd.contains("claude -p")); - assert!(cmd.contains("--dangerously-skip-permissions")); - assert!(cmd.contains("--output-format stream-json")); - assert!(cmd.contains("--model claude-opus-4-6")); - } - - #[test] - fn cli_command_for_gemini() { - let cmd = cli_command_for_profile_kind( - AgentProfileKind::Gemini, - "gemini-3.1-pro", - "/tmp/prompt.txt", - ); - assert!(cmd.starts_with("cat /tmp/prompt.txt | gemini -o json --yolo")); - assert!(cmd.contains("-m gemini-3.1-pro")); - } - - #[test] - fn cli_command_omits_model_when_empty() { - let cmd = cli_command_for_profile_kind(AgentProfileKind::OpenAi, "", "/tmp/prompt.txt"); - assert!(cmd.contains("codex exec --json --full-auto")); - assert!(!cmd.contains("-m ")); - let cmd = cli_command_for_profile_kind(AgentProfileKind::Anthropic, "", "/tmp/prompt.txt"); - assert!(cmd.contains("--dangerously-skip-permissions")); - assert!(!cmd.contains("--model ")); - let cmd = cli_command_for_profile_kind(AgentProfileKind::Gemini, "", "/tmp/prompt.txt"); - assert!(cmd.contains("--yolo")); - assert!(!cmd.contains("-m ")); - } - - // -- Cycle 2: is_cli_only_model -- - - #[test] - fn no_models_are_currently_cli_only() { - assert!(!is_cli_only_model("gpt-5.3-codex")); - assert!(!is_cli_only_model("claude-opus-4-6")); - assert!(!is_cli_only_model("gemini-3.1-pro-preview")); - } - - // -- Cycle 3: parse_cli_response — Claude/Gemini NDJSON -- - - #[test] - fn parse_claude_ndjson_extracts_text_and_usage() { - let output = r#"{"type":"system","message":"Claude CLI v1.0"} -{"type":"assistant","message":{"content":"thinking..."}} -{"type":"result","result":"Here is the implementation.","usage":{"input_tokens":100,"output_tokens":50}}"#; - let response = parse_cli_response(AgentProfileKind::Anthropic, output).unwrap(); - assert_eq!(response.text, "Here is the implementation."); - assert_eq!(response.input_tokens, 100); - assert_eq!(response.output_tokens, 50); - } - - #[test] - fn parse_claude_ndjson_uses_last_result() { - let output = r#"{"type":"result","result":"first","usage":{"input_tokens":10,"output_tokens":5}} -{"type":"result","result":"second","usage":{"input_tokens":20,"output_tokens":10}}"#; - let response = parse_cli_response(AgentProfileKind::Anthropic, output).unwrap(); - assert_eq!(response.text, "second"); - assert_eq!(response.input_tokens, 20); - } - - #[test] - fn parse_claude_ndjson_returns_none_for_no_result() { - let output = r#"{"type":"system","message":"hello"} -{"type":"assistant","message":{"content":"no result line"}}"#; - assert!(parse_cli_response(AgentProfileKind::Anthropic, output).is_none()); - } - - #[test] - fn parse_gemini_json_extracts_text_and_usage() { - let output = r#"{"session_id":"abc","response":"Gemini says hello","stats":{"models":{"gemini-2.5-flash":{"tokens":{"input":200,"candidates":80,"total":280}}}}}"#; - let response = parse_cli_response(AgentProfileKind::Gemini, output).unwrap(); - assert_eq!(response.text, "Gemini says hello"); - assert_eq!(response.input_tokens, 200); - assert_eq!(response.output_tokens, 80); - } - - #[test] - fn parse_gemini_json_handles_missing_stats() { - let output = r#"{"response":"hello"}"#; - let response = parse_cli_response(AgentProfileKind::Gemini, output).unwrap(); - assert_eq!(response.text, "hello"); - assert_eq!(response.input_tokens, 0); - assert_eq!(response.output_tokens, 0); - } - - #[test] - fn parse_gemini_json_returns_none_for_invalid_json() { - assert!(parse_cli_response(AgentProfileKind::Gemini, "not json").is_none()); - } - - // -- Cycle 4: parse_cli_response — Codex NDJSON -- - - #[test] - fn parse_codex_ndjson_extracts_text_and_usage() { - let output = r#"{"type":"thread.started","thread_id":"abc"} -{"type":"turn.started"} -{"type":"item.completed","item":{"id":"item_0","type":"reasoning","text":"thinking..."}} -{"type":"item.completed","item":{"id":"item_1","type":"agent_message","text":"Fixed the bug."}} -{"type":"turn.completed","usage":{"input_tokens":300,"output_tokens":150}}"#; - let response = parse_cli_response(AgentProfileKind::OpenAi, output).unwrap(); - assert_eq!(response.text, "Fixed the bug."); - assert_eq!(response.input_tokens, 300); - assert_eq!(response.output_tokens, 150); - } - - #[test] - fn parse_codex_ndjson_handles_no_message() { - let output = r#"{"type":"turn.completed","usage":{"input_tokens":10,"output_tokens":5}}"#; - let response = parse_cli_response(AgentProfileKind::OpenAi, output).unwrap(); - assert_eq!(response.text, ""); - assert_eq!(response.input_tokens, 10); - } - - #[test] - fn parse_codex_ndjson_returns_none_for_no_events() { - assert!(parse_cli_response(AgentProfileKind::OpenAi, "not json at all").is_none()); - } - - // -- Cycle 5: Node::backend() accessor (tested here since the accessor is - // simple) -- - - #[test] - fn node_backend_returns_none_by_default() { - let node = Node::new("test"); - assert_eq!(node.backend(), None); - } - - #[test] - fn node_backend_returns_cli_when_set() { - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - assert_eq!(node.backend(), Some("cli")); - } - - // -- Cycle 6: backend in stylesheet (tested in stylesheet.rs) -- - - // -- Cycle 7: BackendRouter routing logic -- - - #[test] - fn router_uses_cli_for_backend_attr() { - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - - assert!(BackendRouter::should_use_cli(&node)); - } - - #[test] - fn router_uses_api_by_default() { - let node = Node::new("test"); - - assert!(!BackendRouter::should_use_cli(&node)); - } - - #[test] - fn router_uses_api_for_non_cli_model() { - let mut node = Node::new("test"); - node.attrs.insert( - "model".to_string(), - AttrValue::String("claude-opus-4-6".to_string()), - ); - - assert!(!BackendRouter::should_use_cli(&node)); - } - - #[test] - fn router_uses_api_for_backend_api() { - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("api".to_string())); - - assert_eq!( - BackendRouter::select_backend(&node).unwrap(), - LlmBackend::Api - ); - } - - #[test] - fn router_uses_cli_for_backend_cli() { - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - - assert_eq!( - BackendRouter::select_backend(&node).unwrap(), - LlmBackend::Cli - ); - } - - #[test] - fn router_uses_acp_for_backend_acp() { - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("acp".to_string())); - - assert_eq!( - BackendRouter::select_backend(&node).unwrap(), - LlmBackend::Acp - ); - } - - #[test] - fn router_rejects_unknown_backend() { - let mut node = Node::new("test"); - node.attrs.insert( - "backend".to_string(), - AttrValue::String("codex".to_string()), - ); - - let err = BackendRouter::select_backend(&node).unwrap_err(); - assert_eq!( - err.to_string(), - "Validation error: unsupported LLM backend \"codex\"; expected one of: api, cli, acp" - ); - } - - #[tokio::test] - async fn router_routes_one_shot_to_acp_for_backend_acp() { - let tempdir = tempfile::tempdir().unwrap(); - let script_path = tempdir.path().join("fake_acp_agent.py"); - tokio::fs::write(&script_path, fake_acp_agent_script()) - .await - .unwrap(); - let sandbox: Arc = Arc::new(LocalSandbox::new(tempdir.path().to_path_buf())); - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("acp".to_string())); - node.attrs.insert( - "acp_command".to_string(), - AttrValue::String(format!( - "python3 {}", - shell_quote(&script_path.to_string_lossy()) - )), - ); - - let context = Context::new(); - let router = test_router(); - let emitter = Arc::new(Emitter::default()); - let stage_scope = StageScope::for_handler(&context, "test"); - let result = router - .one_shot(OneShotRequest { - node: &node, - prompt: "prompt", - system_prompt: None, - emitter: &emitter, - stage_scope: &stage_scope, - sandbox: &sandbox, - cancel_token: CancellationToken::new(), - }) - .await - .unwrap(); - - let CodergenResult::Text { text, .. } = result else { - panic!("expected text result"); - }; - assert_eq!(text, "hello from acp"); - } - - #[tokio::test] - async fn router_routes_one_shot_to_api_by_default() { - let node = Node::new("test"); - let sandbox: Arc = Arc::new(LocalSandbox::new( - tempfile::tempdir().unwrap().path().to_path_buf(), - )); - let context = Context::new(); - let router = test_router(); - let emitter = Arc::new(Emitter::default()); - let stage_scope = StageScope::for_handler(&context, "test"); - - let result = router - .one_shot(OneShotRequest { - node: &node, - prompt: "prompt", - system_prompt: None, - emitter: &emitter, - stage_scope: &stage_scope, - sandbox: &sandbox, - cancel_token: CancellationToken::new(), - }) - .await - .unwrap(); - - let CodergenResult::Text { text, .. } = result else { - panic!("expected text result"); - }; - assert_eq!(text, "api one-shot"); - } - - #[tokio::test] - async fn router_routes_one_shot_to_api_for_legacy_cli_backend() { - let mut node = Node::new("test"); - node.attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - let sandbox: Arc = Arc::new(LocalSandbox::new( - tempfile::tempdir().unwrap().path().to_path_buf(), - )); - let context = Context::new(); - let router = test_router(); - let emitter = Arc::new(Emitter::default()); - let stage_scope = StageScope::for_handler(&context, "test"); - - let result = router - .one_shot(OneShotRequest { - node: &node, - prompt: "prompt", - system_prompt: None, - emitter: &emitter, - stage_scope: &stage_scope, - sandbox: &sandbox, - cancel_token: CancellationToken::new(), - }) - .await - .unwrap(); - - let CodergenResult::Text { text, .. } = result else { - panic!("expected text result"); - }; - assert_eq!(text, "api one-shot"); - } - - fn test_router() -> BackendRouter { - let cli_backend = AgentCliBackend::new_from_env("model".into(), ProviderId::anthropic()); - let acp_backend = AgentAcpBackend::new_from_env("model".into(), ProviderId::anthropic()); - BackendRouter::new(Box::new(StubBackend), cli_backend, acp_backend) - } - - /// Minimal stub backend for testing routing logic. - struct StubBackend; - - #[async_trait] - impl CodergenBackend for StubBackend { - async fn run(&self, _request: CodergenRunRequest<'_>) -> Result { - Ok(CodergenResult::Text { - text: "stub".to_string(), - usage: None, - files_touched: Vec::new(), - last_file_touched: None, - }) - } - - async fn one_shot(&self, _request: OneShotRequest<'_>) -> Result { - Ok(CodergenResult::Text { - text: "api one-shot".to_string(), - usage: None, - files_touched: Vec::new(), - last_file_touched: None, - }) - } - } - - /// Sandbox stub whose `exec_command_streaming` returns a configurable - /// `CommandTermination` so we can exercise the cancel/timeout paths in - /// `AgentCliBackend::run` without spawning real processes. - struct StreamingCliMock { - commands: Arc>>, - termination: CommandTermination, - exit_code: Option, - } - - #[async_trait] - impl Sandbox for StreamingCliMock { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { - Ok(String::new()) - } - async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { - Ok(false) - } - async fn list_directory( - &self, - _path: &str, - _depth: Option, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn exec_command( - &self, - command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&std::collections::HashMap>, - _cancel_token: Option, - ) -> fabro_sandbox::Result { - self.commands.lock().unwrap().push(command.to_string()); - // Default: success for CLI availability checks and lightweight setup. - if command.contains("command -v ") { - return Ok(ok_result()); - } - Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 1, - }) - } - async fn exec_command_streaming( - &self, - command: &str, - _timeout_ms: Option, - _working_dir: Option<&str>, - _env_vars: Option<&std::collections::HashMap>, - _cancel_token: Option, - _output_callback: fabro_agent::CommandOutputCallback, - ) -> fabro_sandbox::Result { - self.commands.lock().unwrap().push(command.to_string()); - Ok(fabro_sandbox::ExecStreamingResult { - result: ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: self.exit_code, - termination: self.termination, - duration_ms: 5, - }, - streams_separated: true, - live_streaming: true, - }) - } - async fn grep( - &self, - _pattern: &str, - _path: &str, - _options: &fabro_agent::sandbox::GrepOptions, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn glob( - &self, - _pattern: &str, - _path: Option<&str>, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn download_file_to_local(&self, _: &str, _: &Path) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn upload_file_from_local(&self, _: &Path, _: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn initialize(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn cleanup(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - fn working_directory(&self) -> &str { - "/workspace" - } - fn platform(&self) -> &str { - "linux" - } - fn os_version(&self) -> String { - "Ubuntu 22.04".into() - } - async fn set_autostop_interval(&self, _minutes: i32) -> fabro_sandbox::Result<()> { - Ok(()) - } - } - - fn collect_events(emitter: &Arc) -> Arc>> { - let events = Arc::new(Mutex::new(Vec::new())); - let events_clone = Arc::clone(&events); - emitter.on_event(move |event| events_clone.lock().unwrap().push(event.clone())); - events - } - - #[tokio::test] - async fn agent_cli_backend_run_emits_cancelled_event_and_returns_cancelled() { - let commands = Arc::new(Mutex::new(Vec::new())); - let sandbox: Arc = Arc::new(StreamingCliMock { - commands: Arc::clone(&commands), - termination: CommandTermination::Cancelled, - exit_code: None, - }); - let backend = - AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let node = Node::new("step"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - let events = collect_events(&emitter); - - let result = backend - .run(CodergenRunRequest { - node: &node, - prompt: "Do something", - context: &context, - thread_id: None, - emitter: &emitter, - sandbox: &sandbox, - tool_hooks: None, - cancel_token: CancellationToken::new(), - }) - .await; - - let Err(err) = result else { - panic!("cancelled streaming should bubble Error::Cancelled"); - }; - assert!(matches!(err, Error::Cancelled)); - - let events = events.lock().unwrap(); - let names: Vec = events - .iter() - .map(|e| e.body.event_name().to_string()) - .collect(); - assert!( - names.iter().any(|n| n == "agent.cli.cancelled"), - "expected agent.cli.cancelled, got events: {names:?}" - ); - assert!( - !names.iter().any(|n| n == "agent.cli.completed"), - "should not emit agent.cli.completed on cancellation" - ); - // Cleanup `rm -f` ran. - let cmds = commands.lock().unwrap(); - assert!( - cmds.iter().any(|c| c.starts_with("rm -f /tmp/fabro_cli_")), - "expected temp cleanup, got commands: {cmds:?}" - ); - } - - #[tokio::test] - async fn agent_cli_backend_run_emits_timed_out_event_and_returns_handler_error() { - let commands = Arc::new(Mutex::new(Vec::new())); - let sandbox: Arc = Arc::new(StreamingCliMock { - commands: Arc::clone(&commands), - termination: CommandTermination::TimedOut, - exit_code: None, - }); - let backend = - AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let node = Node::new("step"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - let events = collect_events(&emitter); - - let result = backend - .run(CodergenRunRequest { - node: &node, - prompt: "Do something slow", - context: &context, - thread_id: None, - emitter: &emitter, - sandbox: &sandbox, - tool_hooks: None, - cancel_token: CancellationToken::new(), - }) - .await; - - let Err(err) = result else { - panic!("timeout streaming should produce a handler error"); - }; - assert!( - matches!(err, Error::Handler { .. }), - "expected handler error on timeout, got {err:?}" - ); - - let events = events.lock().unwrap(); - let names: Vec = events - .iter() - .map(|e| e.body.event_name().to_string()) - .collect(); - assert!( - names.iter().any(|n| n == "agent.cli.timed_out"), - "expected agent.cli.timed_out, got events: {names:?}" - ); - assert!( - !names.iter().any(|n| n == "agent.cli.completed"), - "should not emit agent.cli.completed on timeout" - ); - } -} diff --git a/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs b/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs deleted file mode 100644 index 955bcc61f..000000000 --- a/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs +++ /dev/null @@ -1,121 +0,0 @@ -use std::collections::HashMap; -use std::sync::Arc; - -use fabro_agent::{Sandbox, ToolEnvProvider}; -use fabro_auth::{CliAgentKind, CredentialResolver, CredentialUsage, ResolvedCredential}; -use fabro_model::{Catalog, CredentialRef, ProviderId}; -use tokio_util::sync::CancellationToken; - -use super::cli::{AgentCli, process_env_var}; -use crate::error::Error; -use crate::event::{Emitter, RunNoticeCode, RunNoticeLevel}; - -pub(crate) struct AgentLaunchEnvRequest<'a> { - pub provider_id: ProviderId, - pub cli: AgentCli, - pub catalog: &'a Catalog, - pub resolver: Option<&'a CredentialResolver>, - pub tool_env: Option<&'a Arc>, - pub github_token_refresh_managed: bool, - pub stage_label: &'static str, - pub emitter: &'a Arc, - pub sandbox: &'a Arc, - pub cancel_token: &'a CancellationToken, -} - -pub(crate) async fn resolve_agent_launch_env( - request: AgentLaunchEnvRequest<'_>, -) -> Result, Error> { - let cli_agent = match request.cli { - AgentCli::Claude => CliAgentKind::Claude, - AgentCli::Codex => CliAgentKind::Codex, - AgentCli::Gemini => CliAgentKind::Gemini, - }; - - let mut launch_env = if let Some(resolver) = request.resolver { - let resolved = resolver - .resolve( - request.provider_id.clone(), - CredentialUsage::CliAgent(cli_agent), - request.catalog, - ) - .await - .map_err(|err| { - Error::handler_with_source( - format!("Failed to resolve {} credential", request.stage_label), - err, - ) - })?; - let ResolvedCredential::Cli(cli_credential) = resolved else { - return Err(Error::handler("Expected CLI credential".to_string())); - }; - if let Some(login_cmd) = &cli_credential.login_command { - let login_result = request - .sandbox - .exec_command( - login_cmd, - 30_000, - None, - None, - Some(request.cancel_token.child_token()), - ) - .await - .map_err(|err| { - Error::handler_with_source( - format!("{} credential login failed", request.stage_label), - err, - ) - })?; - if !login_result.is_success() { - tracing::warn!( - exit_code = login_result.display_exit_code(), - stage = request.stage_label, - "{} credential login failed: {}", - request.stage_label, - login_result.stderr - ); - } - } - cli_credential.env_vars - } else { - let mut env = HashMap::new(); - if let Some(auth) = request - .catalog - .provider(&request.provider_id) - .and_then(|provider| provider.auth.as_ref()) - { - for credential_ref in &auth.credentials { - let CredentialRef::Env(name) = credential_ref else { - continue; - }; - if let Some(value) = process_env_var(name) { - env.insert(name.clone(), value); - } - } - } - env - }; - - if let Some(provider) = request.tool_env { - if request.github_token_refresh_managed { - request.emitter.notice( - RunNoticeLevel::Info, - RunNoticeCode::GithubTokenRefreshLimited, - format!( - "{} agent stages receive GitHub tokens at process launch; stages running \ - beyond token expiry may need to be retried.", - request.stage_label - ), - ); - } - let tool_env = provider.resolve().await.map_err(|err| { - Error::handler_with_anyhow( - format!("Failed to resolve {} agent env", request.stage_label), - err, - ) - })?; - launch_env.extend(tool_env); - } - - Ok(launch_env) -} diff --git a/lib/crates/fabro-workflow/src/handler/llm/mod.rs b/lib/crates/fabro-workflow/src/handler/llm/mod.rs index 6a19ef8b5..d028171a3 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/mod.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/mod.rs @@ -2,11 +2,10 @@ pub mod acp; pub mod activation_lease; pub mod api; pub mod changed_files; -pub mod cli; -pub mod launch_env; pub mod preamble; +pub mod router; pub mod routing; pub use acp::AgentAcpBackend; pub use api::AgentApiBackend; -pub use cli::{AgentCliBackend, BackendRouter, parse_cli_response}; +pub use router::BackendRouter; diff --git a/lib/crates/fabro-workflow/src/handler/llm/router.rs b/lib/crates/fabro-workflow/src/handler/llm/router.rs new file mode 100644 index 000000000..7b9378af1 --- /dev/null +++ b/lib/crates/fabro-workflow/src/handler/llm/router.rs @@ -0,0 +1,148 @@ +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_graphviz::graph::Node; +use fabro_types::AgentBackend; + +use super::super::agent::{CodergenBackend, CodergenResult, CodergenRunRequest, OneShotRequest}; +use super::acp::AgentAcpBackend; +use super::routing; +use crate::error::Error; +use crate::event::Emitter; + +/// Routes codergen invocations to API or ACP backends based on node attributes. +pub struct BackendRouter { + api: Box, + acp: AgentAcpBackend, +} + +impl BackendRouter { + #[must_use] + pub fn new(api_backend: Box, acp_backend: AgentAcpBackend) -> Self { + Self { + api: api_backend, + acp: acp_backend, + } + } + + fn select_backend(node: &Node) -> Result { + routing::select_run_backend(node) + } + + fn select_one_shot_backend(node: &Node) -> Result { + routing::select_one_shot_backend(node) + } +} + +#[async_trait] +impl CodergenBackend for BackendRouter { + async fn run(&self, request: CodergenRunRequest<'_>) -> Result { + match Self::select_backend(request.node)? { + AgentBackend::Api => self.api.run(request).await, + AgentBackend::Acp => self.acp.run(request).await, + } + } + + async fn one_shot(&self, request: OneShotRequest<'_>) -> Result { + match Self::select_one_shot_backend(request.node)? { + AgentBackend::Api => self.api.one_shot(request).await, + AgentBackend::Acp => { + unreachable!("ACP one-shot is rejected by select_one_shot_backend") + } + } + } + + async fn shutdown(&self, emitter: &Arc) { + self.api.shutdown(emitter).await; + } +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use async_trait::async_trait; + use fabro_agent::{LocalSandbox, Sandbox}; + use fabro_graphviz::graph::{AttrValue, Node}; + use tokio_util::sync::CancellationToken; + + use super::*; + use crate::context::Context; + use crate::event::{Emitter, StageScope}; + + #[test] + fn router_uses_api_by_default() { + let node = Node::new("test"); + + assert_eq!( + BackendRouter::select_backend(&node).unwrap(), + AgentBackend::Api + ); + } + + #[test] + fn router_rejects_cli_backend() { + let mut node = Node::new("test"); + node.attrs + .insert("backend".to_string(), AttrValue::String("cli".to_string())); + + let err = BackendRouter::select_backend(&node).unwrap_err(); + assert_eq!( + err.to_string(), + "Validation error: unsupported agent backend \"cli\"; expected one of: api, acp" + ); + } + + #[tokio::test] + async fn router_routes_one_shot_to_api_by_default() { + let node = Node::new("test"); + let sandbox: Arc = Arc::new(LocalSandbox::new( + tempfile::tempdir().unwrap().path().to_path_buf(), + )); + let context = Context::new(); + let router = BackendRouter::new(Box::new(StubBackend), AgentAcpBackend::new()); + let emitter = Arc::new(Emitter::default()); + let stage_scope = StageScope::for_handler(&context, "test"); + + let result = router + .one_shot(OneShotRequest { + node: &node, + prompt: "prompt", + system_prompt: None, + emitter: &emitter, + stage_scope: &stage_scope, + sandbox: &sandbox, + cancel_token: CancellationToken::new(), + }) + .await + .unwrap(); + + let CodergenResult::Text { text, .. } = result else { + panic!("expected text result"); + }; + assert_eq!(text, "api one-shot"); + } + + struct StubBackend; + + #[async_trait] + impl CodergenBackend for StubBackend { + async fn run(&self, _request: CodergenRunRequest<'_>) -> Result { + Ok(CodergenResult::Text { + text: "api run".to_string(), + usage: None, + files_touched: Vec::new(), + last_file_touched: None, + }) + } + + async fn one_shot(&self, _request: OneShotRequest<'_>) -> Result { + Ok(CodergenResult::Text { + text: "api one-shot".to_string(), + usage: None, + files_touched: Vec::new(), + last_file_touched: None, + }) + } + } +} diff --git a/lib/crates/fabro-workflow/src/handler/llm/routing.rs b/lib/crates/fabro-workflow/src/handler/llm/routing.rs index 14577d5d0..952c31939 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/routing.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/routing.rs @@ -1,19 +1,12 @@ use fabro_graphviz::graph::{self, Node}; use fabro_model::{AgentProfileKind, Catalog, ProviderId}; -use fabro_types::LlmBackend; +use fabro_types::AgentBackend; -use super::cli::is_cli_only_model; use crate::error::Error; -pub(crate) fn select_run_backend(node: &Node) -> Result { - match node.llm_backend() { - None => { - if node.model().is_some_and(is_cli_only_model) { - Ok(LlmBackend::Cli) - } else { - Ok(LlmBackend::Api) - } - } +pub(crate) fn select_run_backend(node: &Node) -> Result { + match node.agent_backend() { + None => Ok(AgentBackend::Api), Some(Ok(backend)) => Ok(backend), Some(Err(_)) => Err(unsupported_backend_error( node.backend().unwrap_or_default(), @@ -21,10 +14,12 @@ pub(crate) fn select_run_backend(node: &Node) -> Result { } } -pub(crate) fn select_one_shot_backend(node: &Node) -> Result { - match node.llm_backend() { - Some(Ok(LlmBackend::Acp)) => Ok(LlmBackend::Acp), - Some(Ok(LlmBackend::Api | LlmBackend::Cli)) | None => Ok(LlmBackend::Api), +pub(crate) fn select_one_shot_backend(node: &Node) -> Result { + match node.agent_backend() { + Some(Ok(AgentBackend::Acp)) => Err(Error::Validation( + "backend=\"acp\" is only valid on agent nodes; prompt nodes are API-only".to_string(), + )), + Some(Ok(AgentBackend::Api)) | None => Ok(AgentBackend::Api), Some(Err(_)) => Err(unsupported_backend_error( node.backend().unwrap_or_default(), )), @@ -37,8 +32,8 @@ pub(crate) fn node_needs_api_backend(node: &Node) -> bool { } match node.handler_type() { - Some("prompt") => !matches!(select_one_shot_backend(node), Ok(LlmBackend::Acp)), - _ => matches!(select_run_backend(node), Ok(LlmBackend::Api)), + Some("prompt") => true, + _ => matches!(select_run_backend(node), Ok(AgentBackend::Api)), } } @@ -93,7 +88,7 @@ pub(crate) fn resolve_node_provider_context( fn unsupported_backend_error(raw: &str) -> Error { Error::Validation(format!( - "unsupported LLM backend \"{raw}\"; expected one of: {}", - LlmBackend::expected_values() + "unsupported agent backend \"{raw}\"; expected one of: {}", + AgentBackend::expected_values() )) } diff --git a/lib/crates/fabro-workflow/src/operations/fork.rs b/lib/crates/fabro-workflow/src/operations/fork.rs index a42bcf5bb..6379274ed 100644 --- a/lib/crates/fabro-workflow/src/operations/fork.rs +++ b/lib/crates/fabro-workflow/src/operations/fork.rs @@ -229,9 +229,6 @@ fn replay_event_for_fork_projection(body: &EventBody) -> bool { | EventBody::InterviewTimeout(_) | EventBody::InterviewInterrupted(_) | EventBody::AgentSessionActivated(_) - | EventBody::AgentCliStarted(_) - | EventBody::AgentCliCancelled(_) - | EventBody::AgentCliTimedOut(_) | EventBody::AgentAcpStarted(_) | EventBody::AgentAcpCancelled(_) | EventBody::AgentAcpTimedOut(_) @@ -324,11 +321,9 @@ mod tests { fn fork_replay_preserves_agent_acp_projection_events() { assert!(replay_event_for_fork_projection( &EventBody::AgentAcpStarted(fabro_types::run_event::AgentAcpStartedProps { - visit: 1, - mode: "acp".to_string(), - provider: "openai".to_string(), - model: "fake-acp".to_string(), - command: "python fake_agent.py".to_string(), + visit: 1, + command: "python fake_agent.py".to_string(), + config_name: Some("fake".to_string()), }) )); assert!(replay_event_for_fork_projection( diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index f1560d5dc..3097a57ea 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -5,8 +5,7 @@ use std::time::Instant; use fabro_agent::Sandbox; use fabro_auth::{ - CredentialResolver, CredentialSource, EnvCredentialSource, VaultCredentialSource, - auth_issue_message, + CredentialSource, EnvCredentialSource, VaultCredentialSource, auth_issue_message, }; use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookRunner}; @@ -29,9 +28,7 @@ use crate::devcontainer_bridge::{devcontainer_to_snapshot_config, run_devcontain use crate::error::Error; use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::github_token_source::{AppIatMinter, GitHubTokenSource}; -use crate::handler::llm::{ - AgentAcpBackend, AgentApiBackend, AgentCliBackend, BackendRouter, routing, -}; +use crate::handler::llm::{AgentAcpBackend, AgentApiBackend, BackendRouter, routing}; use crate::handler::{HandlerRegistry, default_registry}; use crate::run_metadata::{RunMetadataRuntime, build_metadata_writer, metadata_branch_name}; use crate::run_options::{GitCheckpointOptions, RunOptions}; @@ -126,7 +123,6 @@ async fn build_registry( graph: &graph::Graph, llm_source: Arc, catalog: Arc, - cli_resolver: Option, ) -> Result<(Arc, bool), Error> { let no_backend_interviewer = Arc::clone(&interviewer); let build_no_backend = move || { @@ -172,24 +168,9 @@ async fn build_registry( .with_run_model_controls(model_controls.clone()) .with_tool_env_provider(tool_env_provider.clone()) .with_mcp_servers(mcp_servers.clone()); - let cli = cli_resolver - .clone() - .map_or_else( - || AgentCliBackend::new_from_env(model.clone(), provider_id.clone()), - |resolver| AgentCliBackend::new(model.clone(), provider_id.clone(), resolver), - ) - .with_catalog(Arc::clone(&catalog_for_api)) - .with_run_model_controls(model_controls.clone()) + let acp = AgentAcpBackend::new() .with_tool_env_provider(tool_env_provider.clone(), github_token_refresh_managed); - let acp = cli_resolver - .clone() - .map_or_else( - || AgentAcpBackend::new_from_env(model.clone(), provider_id.clone()), - |resolver| AgentAcpBackend::new(model.clone(), provider_id.clone(), resolver), - ) - .with_catalog(Arc::clone(&catalog_for_api)) - .with_tool_env_provider(tool_env_provider.clone(), github_token_refresh_managed); - Some(Box::new(BackendRouter::new(Box::new(api), cli, acp))) + Some(Box::new(BackendRouter::new(Box::new(api), acp))) })) }; @@ -350,7 +331,6 @@ pub async fn initialize( let llm_source = build_llm_source(options.vault.clone()); let catalog = Arc::clone(&options.catalog); - let cli_resolver = options.vault.clone().map(CredentialResolver::new); let sandbox_git = Arc::new(SandboxGitRuntime::new()); let metadata_runtime = Arc::new(RunMetadataRuntime::new()); @@ -518,7 +498,6 @@ pub async fn initialize( &graph, Arc::clone(&llm_source), Arc::clone(&catalog), - cli_resolver, ) .await? }; @@ -1046,7 +1025,6 @@ mod tests { &graph, Arc::new(VaultCredentialSource::new(Arc::clone(&vault))), test_catalog(), - Some(CredentialResolver::new(vault)), ) .await .unwrap(); @@ -1065,7 +1043,7 @@ mod tests { let source = format!( r#"digraph test {{ start [shape=Mdiamond]; - writer [type="agent", backend="acp", provider="openai", model="fake-acp", prompt="write hello", acp_command="python3 {}"]; + writer [type="agent", backend="acp", prompt="write hello", acp.command="python3 {}"]; exit [shape=Msquare]; start -> writer; writer -> exit; @@ -1085,20 +1063,12 @@ mod tests { writer .attrs .insert("backend".to_string(), AttrValue::String("acp".to_string())); - writer.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); - writer.attrs.insert( - "model".to_string(), - AttrValue::String("fake-acp".to_string()), - ); writer.attrs.insert( "prompt".to_string(), AttrValue::String("write hello".to_string()), ); writer.attrs.insert( - "acp_command".to_string(), + "acp.command".to_string(), AttrValue::String(format!( "python3 {}", fabro_sandbox::shell_quote(&script_path.to_string_lossy()) diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 32a48fc9f..e5e3cccb1 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -598,7 +598,8 @@ impl ImportTransform { | "reasoning_effort" | "speed" | "backend" - | "acp_command" + | "acp.command" + | "acp.config" | "fidelity" | "max_retries" | "thread_id" @@ -1005,7 +1006,7 @@ mod tests { let graph = apply_import( r#"digraph Deploy { start [shape=Mdiamond] - validate [import="./validate.fabro", model="haiku", backend="acp", acp_command="python fake_agent.py", class="fast, shared"] + validate [import="./validate.fabro", model="haiku", backend="acp", acp.command="python fake_agent.py", class="fast, shared"] exit [shape=Msquare] start -> validate -> exit }"#, @@ -1055,7 +1056,7 @@ mod tests { assert_eq!( graph.nodes["validate.test"] .attrs - .get("acp_command") + .get("acp.command") .and_then(AttrValue::as_str), Some("python fake_agent.py") ); diff --git a/lib/crates/fabro-workflow/src/transforms/stylesheet.rs b/lib/crates/fabro-workflow/src/transforms/stylesheet.rs index 2ecebe37d..b6e61f6d7 100644 --- a/lib/crates/fabro-workflow/src/transforms/stylesheet.rs +++ b/lib/crates/fabro-workflow/src/transforms/stylesheet.rs @@ -216,20 +216,20 @@ mod tests { #[test] fn apply_backend_property_via_stylesheet() { - let ss = parse_stylesheet("* { backend: cli; }").unwrap(); + let ss = parse_stylesheet("* { backend: acp; }").unwrap(); let mut graph = Graph::new("test"); graph.nodes.insert("a".into(), Node::new("a")); apply_stylesheet(&ss, &mut graph); assert_eq!( graph.nodes["a"].attrs.get("backend"), - Some(&AttrValue::String("cli".into())) + Some(&AttrValue::String("acp".into())) ); } #[test] fn backend_property_not_overridden_by_stylesheet() { - let ss = parse_stylesheet("* { backend: cli; }").unwrap(); + let ss = parse_stylesheet("* { backend: acp; }").unwrap(); let mut graph = Graph::new("test"); let mut node = Node::new("a"); node.attrs diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index e2c1e0ad6..8d6e20763 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -24,7 +24,6 @@ use std::sync::Arc; use fabro_agent::Sandbox; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; -use fabro_model::ProviderId; use fabro_sandbox::daytona::{DaytonaConfig, DaytonaSandbox, DaytonaSnapshotConfig}; use fabro_static::EnvVars; use fabro_store::{ArtifactKey, ArtifactStore, Database}; @@ -991,151 +990,6 @@ async fn daytona_parallel_git_branching_e2e() { env.cleanup().await.expect("Daytona cleanup should succeed"); } -// --------------------------------------------------------------------------- -// CLI Backend on Daytona — real CLI tools via exec_command -// --------------------------------------------------------------------------- - -use fabro_workflow::handler::agent::{CodergenBackend, CodergenResult, CodergenRunRequest}; -use fabro_workflow::handler::llm::AgentCliBackend; - -/// Helper: run a real CLI backend test on Daytona. -/// -/// Installs the CLI tool in the sandbox, then runs the AgentCliBackend against -/// it. -async fn run_daytona_cli_test(provider: ProviderId, model: &str, install_command: &str) { - let creds = load_github_app_credentials(); - let config = DaytonaConfig { - snapshot: Some(DaytonaSnapshotConfig { - name: "daytona-medium".into(), - cpu: None, - memory: None, - disk: None, - dockerfile: None, - }), - ..DaytonaConfig::default() - }; - let env = DaytonaSandbox::new(config, Some(creds), None, None, None, None) - .await - .expect("Failed to create Daytona client — is DAYTONA_API_KEY set?"); - env.initialize() - .await - .expect("Daytona sandbox should initialize"); - let env: Arc = Arc::new(env); - - // Install prerequisites (bash, curl, Node 20 via nodesource) if not available - let prereq_check = env - .exec_command( - "bash --version && curl --version && node --version && npm --version", - 10_000, - None, - None, - None, - ) - .await; - if prereq_check.as_ref().map_or(true, |r| !r.is_success()) { - let prereq = env - .exec_command( - "apt-get update -qq && apt-get install -y -qq bash curl ca-certificates gnupg >/dev/null 2>&1 \ - && curl -fsSL https://deb.nodesource.com/setup_20.x | bash - >/dev/null 2>&1 \ - && apt-get install -y -qq nodejs >/dev/null 2>&1", - 180_000, - None, - None, - None, - ) - .await - .expect("prerequisite install should not error"); - assert_eq!( - prereq.exit_code, - Some(0), - "prerequisite install failed: {}", - prereq.stderr - ); - } - - // Install the CLI tool inside the Daytona sandbox - let install_result = env - .exec_command(install_command, 120_000, None, None, None) - .await - .expect("install command should not error"); - assert_eq!( - install_result.exit_code, - Some(0), - "install command failed (exit {:?}): {}", - install_result.exit_code, - install_result.stdout - ); - - let backend = AgentCliBackend::new_from_env(model.to_string(), provider.clone()); - let node = Node::new("daytona_cli_test"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = backend - .run(CodergenRunRequest { - node: &node, - prompt: "What is 2+2? Reply with just the number.", - context: &context, - thread_id: None, - emitter: &emitter, - sandbox: &env, - tool_hooks: None, - cancel_token: CancellationToken::new(), - }) - .await; - - match result { - Ok(CodergenResult::Text { text, usage, .. }) => { - assert!( - text.contains('4'), - "{provider}/{model} on Daytona: expected '4', got: {text}" - ); - if let Some(u) = usage { - assert!( - u.tokens().input_tokens > 0, - "{provider}/{model}: input_tokens should be > 0" - ); - } - } - Ok(CodergenResult::Full(_)) => panic!("expected Text result"), - Err(e) => panic!("{provider}/{model} on Daytona failed: {e}"), - } - - env.cleanup() - .await - .expect("Daytona sandbox cleanup should succeed"); -} - -#[fabro_macros::e2e_test(live("DAYTONA_API_KEY"), live("GITHUB_APP_PRIVATE_KEY"))] -async fn daytona_cli_claude() { - run_daytona_cli_test( - ProviderId::anthropic(), - "haiku", - "curl -fsSL https://claude.ai/install.sh | bash", - ) - .await; -} - -#[fabro_macros::e2e_test(live("DAYTONA_API_KEY"), live("GITHUB_APP_PRIVATE_KEY"))] -async fn daytona_cli_codex() { - run_daytona_cli_test( - ProviderId::openai(), - "o4-mini", - "npm install -g @openai/codex", - ) - .await; -} - -#[fabro_macros::e2e_test(live("DAYTONA_API_KEY"), live("GITHUB_APP_PRIVATE_KEY"))] -async fn daytona_cli_gemini() { - run_daytona_cli_test( - ProviderId::gemini(), - "gemini-2.5-flash", - "npm install -g @google/gemini-cli", - ) - .await; -} - // --------------------------------------------------------------------------- // Daytona shadow commit E2E with sandbox-native metadata // --------------------------------------------------------------------------- diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 36fff6272..6f0af825f 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -23,7 +23,6 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; -use fabro_acp::test_support::fake_acp_agent_script; use fabro_config::RunScratch; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_graphviz::parser::parse; @@ -31,10 +30,10 @@ use fabro_interview::{ Answer, AnswerValue, AutoApproveInterviewer, CallbackInterviewer, Interviewer, QueueInterviewer, RecordingInterviewer, }; +use fabro_model::Catalog; use fabro_model::catalog::{LlmCatalogSettings, ProviderCatalogSettings}; -use fabro_model::{AgentProfileKind, Catalog, ProviderId}; use fabro_store::{ArtifactKey, ArtifactStore, Database}; -use fabro_types::{CommandTermination, RunEvent, RunId, StageId, WorkflowSettings, parse_blob_ref}; +use fabro_types::{RunEvent, RunId, StageId, WorkflowSettings, parse_blob_ref}; use fabro_validate::{Severity, validate, validate_or_raise}; use fabro_workflow::context::Context; use fabro_workflow::error::{Error, FailureSignatureExt}; @@ -46,8 +45,6 @@ use fabro_workflow::handler::command::CommandHandler; use fabro_workflow::handler::conditional::ConditionalHandler; use fabro_workflow::handler::exit::ExitHandler; use fabro_workflow::handler::human::HumanHandler; -use fabro_workflow::handler::llm::AgentAcpBackend; -use fabro_workflow::handler::llm::cli::{AgentCliBackend, BackendRouter, parse_cli_response}; use fabro_workflow::handler::manager_loop::SubWorkflowHandler; use fabro_workflow::handler::start::StartHandler; use fabro_workflow::handler::wait::WaitHandler; @@ -86,25 +83,6 @@ fn local_env() -> Arc { )) } -fn codergen_run_request<'a>( - node: &'a Node, - prompt: &'a str, - context: &'a Context, - emitter: &'a Arc, - sandbox: &'a Arc, -) -> CodergenRunRequest<'a> { - CodergenRunRequest { - node, - prompt, - context, - thread_id: None, - emitter, - sandbox, - tool_hooks: None, - cancel_token: CancellationToken::new(), - } -} - fn test_run_id(label: &str) -> RunId { let mut hasher = DefaultHasher::new(); label.hash(&mut hasher); @@ -9577,1114 +9555,6 @@ async fn node_dir_uses_visit_count_on_revisit() { ); } -// --------------------------------------------------------------------------- -// CLI Backend end-to-end tests -// --------------------------------------------------------------------------- - -/// A mock sandbox for CLI backend e2e tests. -/// Records all exec_command and write_file calls, and returns configurable -/// responses based on command content. -struct CliTestEnv { - /// All commands passed to exec_command, in order. - commands: std::sync::Mutex>, - /// All (path, content) pairs from write_file. - written_files: std::sync::Mutex>, - /// The stdout to return when the CLI command (not git) is executed. - cli_stdout: String, - /// Files returned by "git diff --name-only" AFTER the CLI runs. - /// First call returns empty (before), second returns these (after). - git_diff_call_count: std::sync::atomic::AtomicU32, - git_diff_after: String, -} - -impl CliTestEnv { - fn new(cli_stdout: &str) -> Self { - Self { - commands: std::sync::Mutex::new(Vec::new()), - written_files: std::sync::Mutex::new(Vec::new()), - cli_stdout: cli_stdout.to_string(), - git_diff_call_count: std::sync::atomic::AtomicU32::new(0), - git_diff_after: String::new(), - } - } - - fn with_git_diff_after(mut self, files: &str) -> Self { - self.git_diff_after = files.to_string(); - self - } - - fn recorded_commands(&self) -> Vec { - self.commands.lock().unwrap().clone() - } - - fn recorded_written_files(&self) -> Vec<(String, String)> { - self.written_files.lock().unwrap().clone() - } -} - -#[async_trait::async_trait] -impl fabro_agent::Sandbox for CliTestEnv { - async fn read_file( - &self, - _path: &str, - _offset: Option, - _limit: Option, - ) -> fabro_sandbox::Result { - Ok(String::new()) - } - - async fn write_file(&self, path: &str, content: &str) -> fabro_sandbox::Result<()> { - self.written_files - .lock() - .unwrap() - .push((path.to_string(), content.to_string())); - Ok(()) - } - - async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - - async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { - Ok(false) - } - - async fn list_directory( - &self, - _path: &str, - _depth: Option, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - - async fn exec_command( - &self, - command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&std::collections::HashMap>, - _cancel_token: Option, - ) -> fabro_sandbox::Result { - self.commands.lock().unwrap().push(command.to_string()); - - // Changed-file snapshot calls: first returns empty (before), second - // returns configured files (after). - if command.contains("__FABRO_CHANGED_FILES_DIFF__") - || command.starts_with("git diff") - || command.starts_with("git ls-files") - { - let call_num = self - .git_diff_call_count - .fetch_add(1, std::sync::atomic::Ordering::SeqCst); - let stdout = if command.contains("__FABRO_CHANGED_FILES_DIFF__") { - if call_num >= 1 { - format!( - "__FABRO_CHANGED_FILES_DIFF__\n{}__FABRO_CHANGED_FILES_UNTRACKED__\n", - self.git_diff_after - ) - } else { - "__FABRO_CHANGED_FILES_DIFF__\n__FABRO_CHANGED_FILES_UNTRACKED__\n".to_string() - } - } else if call_num >= 2 && command.starts_with("git diff") { - self.git_diff_after.clone() - } else { - String::new() - }; - return Ok(fabro_agent::ExecResult { - stdout, - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 5, - }); - } - - // CLI availability check. - if command.contains("command -v ") { - return Ok(fabro_agent::ExecResult { - stdout: "/usr/local/bin/agent-cli\n".into(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 1, - }); - } - - // Cleanup temp files - if command.starts_with("rm -f") { - return Ok(fabro_agent::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 1, - }); - } - - // ls -t for last_file_touched - if command.starts_with("ls -t ") { - return Ok(fabro_agent::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 1, - }); - } - - // Fallback: this is the streaming CLI invocation. The default trait - // implementation of `exec_command_streaming` delegates to this path - // and replays output through the streaming callback. - Ok(fabro_agent::ExecResult { - stdout: self.cli_stdout.clone(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 100, - }) - } - - async fn grep( - &self, - _pattern: &str, - _path: &str, - _options: &fabro_agent::GrepOptions, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - - async fn glob( - &self, - _pattern: &str, - _path: Option<&str>, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - - async fn initialize(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - - async fn cleanup(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - - async fn download_file_to_local( - &self, - _: &str, - _: &std::path::Path, - ) -> fabro_sandbox::Result<()> { - Err("not implemented".into()) - } - - async fn upload_file_from_local( - &self, - _: &std::path::Path, - _: &str, - ) -> fabro_sandbox::Result<()> { - Err("not implemented".into()) - } - - fn working_directory(&self) -> &str { - "/tmp/test" - } - - fn platform(&self) -> &str { - "darwin" - } - - fn os_version(&self) -> String { - "Darwin 24.0.0".into() - } -} - -// -- Cycle 8: AgentCliBackend::run() e2e via mock Sandbox -- - -#[tokio::test] -async fn cli_backend_run_writes_prompt_and_calls_exec() { - let claude_output = r#"{"type":"result","result":"I fixed the bug.","usage":{"input_tokens":500,"output_tokens":200}}"#; - let test_env = Arc::new(CliTestEnv::new(claude_output)); - let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - - let node = Node::new("fix_code"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = backend - .run(codergen_run_request( - &node, - "Fix the authentication bug", - &context, - &emitter, - &env, - )) - .await - .expect("CLI backend should succeed"); - - // Verify prompt was written - let written = test_env.recorded_written_files(); - let prompt_file = written - .iter() - .find(|(path, _)| path.contains("_prompt.txt")) - .expect("should write a prompt file"); - assert!( - prompt_file.0.starts_with("/tmp/fabro_cli_") && prompt_file.0.ends_with("_prompt.txt"), - "prompt path should use UUID prefix: {}", - prompt_file.0 - ); - assert_eq!(prompt_file.1, "Fix the authentication bug"); - - // Verify the CLI command was streamed (env file is sourced, then `cat - // | claude -p ...` runs as the inner shell command). - let commands = test_env.recorded_commands(); - let cli_cmd = commands - .iter() - .find(|c| c.contains("claude") && c.contains("_prompt.txt")) - .expect("should run claude CLI command"); - assert!(cli_cmd.contains("-p"), "should use pipe mode"); - assert!( - cli_cmd.contains("claude-opus-4-6"), - "should use correct model" - ); - assert!( - cli_cmd.contains(". /tmp/fabro_cli_") && cli_cmd.contains("_env.sh"), - "should source the env file before invoking the CLI: {cli_cmd}" - ); - - // Verify parsed response - match result { - CodergenResult::Text { - text, - usage, - files_touched, - .. - } => { - assert_eq!(text, "I fixed the bug."); - let usage = usage.expect("should have usage"); - assert_eq!(usage.tokens().input_tokens, 500); - assert_eq!(usage.tokens().output_tokens, 200); - assert!(files_touched.is_empty(), "no files changed before/after"); - } - CodergenResult::Full(_) => panic!("expected Text result, got Full"), - } -} - -#[tokio::test] -async fn cli_backend_run_detects_changed_files() { - let claude_output = r#"{"type":"result","result":"Created new file.","usage":{"input_tokens":100,"output_tokens":50}}"#; - let env: Arc = - Arc::new(CliTestEnv::new(claude_output).with_git_diff_after("src/main.rs\nsrc/lib.rs\n")); - let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - - let node = Node::new("implement"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = backend - .run(codergen_run_request( - &node, - "Add a new feature", - &context, - &emitter, - &env, - )) - .await - .expect("CLI backend should succeed"); - - match result { - CodergenResult::Text { files_touched, .. } => { - assert_eq!(files_touched, vec!["src/lib.rs", "src/main.rs"]); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -#[tokio::test] -async fn cli_backend_run_with_codex_provider() { - let codex_output = "{\"type\":\"item.completed\",\"item\":{\"id\":\"item_0\",\"type\":\"agent_message\",\"text\":\"Implemented the feature.\"}}\n{\"type\":\"turn.completed\",\"usage\":{\"input_tokens\":300,\"output_tokens\":150}}"; - let test_env = Arc::new(CliTestEnv::new(codex_output)); - let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new_from_env("gpt-5.3-codex".into(), ProviderId::openai()); - - let node = Node::new("implement"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = backend - .run(codergen_run_request( - &node, - "Build the API", - &context, - &emitter, - &env, - )) - .await - .expect("CLI backend should succeed"); - - // Verify codex command was streamed. - let commands = test_env.recorded_commands(); - let cli_cmd = commands - .iter() - .find(|c| c.contains("codex") && c.contains("_prompt.txt")) - .expect("should run codex CLI command"); - assert!(cli_cmd.contains("exec --json"), "should use exec mode"); - assert!( - cli_cmd.contains("gpt-5.3-codex"), - "should use correct model" - ); - - match result { - CodergenResult::Text { text, usage, .. } => { - assert_eq!(text, "Implemented the feature."); - let usage = usage.expect("should have usage"); - assert_eq!(usage.tokens().input_tokens, 300); - assert_eq!(usage.tokens().output_tokens, 150); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -#[tokio::test] -async fn cli_backend_run_fails_on_nonzero_exit() { - let env = Arc::new(CliTestEnv::new("")); - - // Override exec_command to return non-zero for the CLI call - struct FailingCliEnv; - #[async_trait::async_trait] - impl fabro_agent::Sandbox for FailingCliEnv { - async fn read_file( - &self, - _: &str, - _: Option, - _: Option, - ) -> fabro_sandbox::Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn delete_file(&self, _: &str) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> fabro_sandbox::Result { - Ok(false) - } - async fn list_directory( - &self, - _: &str, - _: Option, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn exec_command( - &self, - command: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - _: Option, - ) -> fabro_sandbox::Result { - if command.starts_with("git") { - return Ok(fabro_agent::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 0, - }); - } - // CLI availability check. - if command.contains("command -v ") { - return Ok(fabro_agent::ExecResult { - stdout: "/usr/local/bin/agent-cli\n".into(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 0, - }); - } - // The streaming CLI invocation: return non-zero exit with stderr. - if command.contains("claude") || command.contains("codex") { - return Ok(fabro_agent::ExecResult { - stdout: String::new(), - stderr: "command not found: claude".into(), - exit_code: Some(127), - termination: CommandTermination::Exited, - duration_ms: 0, - }); - } - Ok(fabro_agent::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - - termination: CommandTermination::Exited, - duration_ms: 0, - }) - } - async fn grep( - &self, - _: &str, - _: &str, - _: &fabro_agent::GrepOptions, - ) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> fabro_sandbox::Result> { - Ok(vec![]) - } - async fn initialize(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn cleanup(&self) -> fabro_sandbox::Result<()> { - Ok(()) - } - async fn download_file_to_local( - &self, - _: &str, - _: &std::path::Path, - ) -> fabro_sandbox::Result<()> { - Err("not implemented".into()) - } - async fn upload_file_from_local( - &self, - _: &std::path::Path, - _: &str, - ) -> fabro_sandbox::Result<()> { - Err("not implemented".into()) - } - fn working_directory(&self) -> &str { - "/tmp" - } - fn platform(&self) -> &str { - "darwin" - } - fn os_version(&self) -> String { - "Darwin 24.0.0".into() - } - } - - let failing_env: Arc = Arc::new(FailingCliEnv); - let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let node = Node::new("step"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let _ = env; // unused, just for the above struct - - let result = backend - .run(codergen_run_request( - &node, - "do something", - &context, - &emitter, - &failing_env, - )) - .await; - - let err = match result { - Err(e) => e, - Ok(_) => panic!("should fail on non-zero exit"), - }; - - assert!( - err.to_string().contains("exited with code 127"), - "error: {err}" - ); - assert!( - err.to_string().contains("command not found"), - "error: {err}" - ); -} - -#[tokio::test] -async fn cli_backend_run_fails_on_unparseable_output() { - let env: Arc = Arc::new(CliTestEnv::new("this is not json at all")); - let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - - let node = Node::new("step"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = backend - .run(codergen_run_request( - &node, - "do something", - &context, - &emitter, - &env, - )) - .await; - - let err = match result { - Err(e) => e, - Ok(_) => panic!("should fail on unparseable output"), - }; - - assert!( - err.to_string().contains("Failed to parse CLI output"), - "error: {err}" - ); -} - -#[tokio::test] -async fn cli_backend_run_uses_node_model_override() { - let claude_output = - r#"{"type":"result","result":"ok","usage":{"input_tokens":10,"output_tokens":5}}"#; - let test_env = Arc::new(CliTestEnv::new(claude_output)); - let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new_from_env("default-model".into(), ProviderId::anthropic()); - - let mut node = Node::new("step"); - node.attrs.insert( - "model".to_string(), - AttrValue::String("claude-sonnet-4-5".to_string()), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - backend - .run(codergen_run_request( - &node, "test", &context, &emitter, &env, - )) - .await - .expect("should succeed"); - - let commands = test_env.recorded_commands(); - let cli_cmd = commands - .iter() - .find(|c| c.contains("claude") && c.contains("_prompt.txt")) - .unwrap(); - assert!( - cli_cmd.contains("claude-sonnet-4-5"), - "should use node's model override, not default: {cli_cmd}" - ); - assert!( - !cli_cmd.contains("default-model"), - "should NOT use default model: {cli_cmd}" - ); -} - -#[tokio::test] -async fn cli_backend_run_uses_node_provider_override() { - let codex_output = "{\"type\":\"item.completed\",\"item\":{\"id\":\"item_0\",\"type\":\"agent_message\",\"text\":\"ok\"}}\n{\"type\":\"turn.completed\",\"usage\":{\"input_tokens\":10,\"output_tokens\":5}}"; - let test_env = Arc::new(CliTestEnv::new(codex_output)); - let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new_from_env("default-model".into(), ProviderId::anthropic()); - - let mut node = Node::new("step"); - node.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); - node.attrs.insert( - "model".to_string(), - AttrValue::String("gpt-5.3-codex".to_string()), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - backend - .run(codergen_run_request( - &node, "test", &context, &emitter, &env, - )) - .await - .expect("should succeed"); - - let commands = test_env.recorded_commands(); - let cli_cmd = commands - .iter() - .find(|c| c.contains("codex") && c.contains("_prompt.txt")) - .expect("should launch codex based on provider override"); - assert!(cli_cmd.contains("gpt-5.3-codex")); -} - -#[tokio::test] -async fn cli_backend_run_returns_text_and_usage() { - let claude_output = - r#"{"type":"result","result":"done","usage":{"input_tokens":10,"output_tokens":5}}"#; - let env: Arc = Arc::new(CliTestEnv::new(claude_output)); - let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - - let node = Node::new("step"); - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = backend - .run(codergen_run_request( - &node, "test", &context, &emitter, &env, - )) - .await - .expect("should succeed"); - - match result { - CodergenResult::Text { text, usage, .. } => { - assert_eq!(text, "done"); - let usage = usage.expect("CLI backend should report usage"); - assert_eq!(usage.tokens().input_tokens, 10); - assert_eq!(usage.tokens().output_tokens, 5); - assert_eq!(usage.model_id(), "claude-opus-4-6"); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -// -- BackendRouter e2e: delegates to correct backend -- - -fn test_acp_backend() -> AgentAcpBackend { - AgentAcpBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()) -} - -#[tokio::test] -async fn backend_router_delegates_to_cli_for_cli_node() { - let claude_output = r#"{"type":"result","result":"CLI response","usage":{"input_tokens":10,"output_tokens":5}}"#; - let env: Arc = Arc::new(CliTestEnv::new(claude_output)); - - let api_backend = Box::new(MockCodergenBackend); // would return "Response for ..." - let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let router = BackendRouter::new(api_backend, cli, test_acp_backend()); - - let mut node = Node::new("cli_step"); - node.attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - node.attrs.insert( - "prompt".to_string(), - AttrValue::String("Fix the bug".to_string()), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = router - .run(codergen_run_request( - &node, - "Fix the bug", - &context, - &emitter, - &env, - )) - .await - .expect("router should succeed"); - - match result { - CodergenResult::Text { text, .. } => { - assert_eq!( - text, "CLI response", - "should use CLI backend response, not mock API" - ); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -#[tokio::test] -async fn backend_router_delegates_to_api_for_normal_node() { - let env = local_env(); - - let api_backend = Box::new(MockCodergenBackend); - let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let router = BackendRouter::new(api_backend, cli, test_acp_backend()); - - let mut node = Node::new("api_step"); - node.attrs.insert( - "prompt".to_string(), - AttrValue::String("Plan the work".to_string()), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = router - .run(codergen_run_request( - &node, - "Plan the work", - &context, - &emitter, - &env, - )) - .await - .expect("router should succeed"); - - match result { - CodergenResult::Text { text, .. } => { - assert!( - text.starts_with("Response for api_step"), - "should use API mock response: {text}" - ); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -#[tokio::test] -async fn backend_router_delegates_to_cli_for_backend_attr() { - let codex_output = "{\"type\":\"item.completed\",\"item\":{\"id\":\"item_0\",\"type\":\"agent_message\",\"text\":\"Codex did it\"}}\n{\"type\":\"turn.completed\",\"usage\":{\"input_tokens\":10,\"output_tokens\":5}}"; - let env: Arc = Arc::new(CliTestEnv::new(codex_output)); - - let api_backend = Box::new(MockCodergenBackend); - let cli = AgentCliBackend::new_from_env("gpt-5.3-codex".into(), ProviderId::openai()); - let router = BackendRouter::new(api_backend, cli, test_acp_backend()); - - let mut node = Node::new("codex_step"); - node.attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - node.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = router - .run(codergen_run_request( - &node, "Build it", &context, &emitter, &env, - )) - .await - .expect("router should succeed"); - - match result { - CodergenResult::Text { text, .. } => { - assert_eq!( - text, "Codex did it", - "should route to CLI backend for backend=cli" - ); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -#[tokio::test] -async fn backend_router_delegates_to_acp_for_acp_node() { - let tempdir = tempfile::tempdir().unwrap(); - let script_path = tempdir.path().join("fake_acp_agent.py"); - tokio::fs::write(&script_path, fake_acp_agent_script()) - .await - .unwrap(); - let env: Arc = - Arc::new(fabro_agent::LocalSandbox::new(tempdir.path().to_path_buf())); - - let api_backend = Box::new(MockCodergenBackend); - let cli = AgentCliBackend::new_from_env("gpt-5.3-codex".into(), ProviderId::openai()); - let router = BackendRouter::new( - api_backend, - cli, - AgentAcpBackend::new_from_env("fake-acp".into(), ProviderId::openai()), - ); - - let mut node = Node::new("acp_step"); - node.attrs - .insert("backend".to_string(), AttrValue::String("acp".to_string())); - node.attrs.insert( - "provider".to_string(), - AttrValue::String("openai".to_string()), - ); - node.attrs.insert( - "model".to_string(), - AttrValue::String("fake-acp".to_string()), - ); - node.attrs.insert( - "acp_command".to_string(), - AttrValue::String(format!( - "python3 {}", - fabro_agent::shell_quote(&script_path.to_string_lossy()) - )), - ); - - let context = Context::new(); - let emitter = Arc::new(Emitter::default()); - - let result = router - .run(codergen_run_request( - &node, "Build it", &context, &emitter, &env, - )) - .await - .expect("router should succeed"); - - match result { - CodergenResult::Text { text, .. } => { - assert_eq!( - text, "hello from acp", - "should route to ACP backend for backend=acp" - ); - } - CodergenResult::Full(_) => panic!("expected Text result"), - } -} - -// -- Full pipeline e2e with BackendRouter -- - -#[tokio::test] -async fn full_pipeline_with_cli_backend_node() { - // Pipeline: start -> api_work -> cli_work -> exit - // api_work uses MockCodergenBackend (API), cli_work has backend="cli" - let claude_output = r#"{"type":"result","result":"CLI completed the task.","usage":{"input_tokens":100,"output_tokens":50}}"#; - let env: Arc = Arc::new(CliTestEnv::new(claude_output)); - - let mut graph = Graph::new("CliPipelineTest"); - - let mut start = Node::new("start"); - start.attrs.insert( - "shape".to_string(), - AttrValue::String("Mdiamond".to_string()), - ); - graph.nodes.insert("start".to_string(), start); - - let mut exit = Node::new("exit"); - exit.attrs.insert( - "shape".to_string(), - AttrValue::String("Msquare".to_string()), - ); - graph.nodes.insert("exit".to_string(), exit); - - let mut api_work = Node::new("api_work"); - api_work - .attrs - .insert("shape".to_string(), AttrValue::String("box".to_string())); - api_work.attrs.insert( - "prompt".to_string(), - AttrValue::String("Plan the work".to_string()), - ); - graph.nodes.insert("api_work".to_string(), api_work); - - let mut cli_work = Node::new("cli_work"); - cli_work - .attrs - .insert("shape".to_string(), AttrValue::String("box".to_string())); - cli_work.attrs.insert( - "prompt".to_string(), - AttrValue::String("Implement via CLI".to_string()), - ); - cli_work - .attrs - .insert("backend".to_string(), AttrValue::String("cli".to_string())); - graph.nodes.insert("cli_work".to_string(), cli_work); - - graph.edges.push(Edge::new("start", "api_work")); - graph.edges.push(Edge::new("api_work", "cli_work")); - graph.edges.push(Edge::new("cli_work", "exit")); - - // Build engine with BackendRouter - let api = MockCodergenBackend; - let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let router = BackendRouter::new(Box::new(api), cli, test_acp_backend()); - let codergen_handler = AgentHandler::new(Some(Box::new(router))); - - let mut registry = HandlerRegistry::new(Box::new(codergen_handler)); - registry.register("start", Box::new(StartHandler)); - registry.register("exit", Box::new(ExitHandler)); - registry.register( - "agent", - Box::new(AgentHandler::new(Some(Box::new({ - // Second BackendRouter for the "agent" handler - let api2 = MockCodergenBackend; - let cli2 = - AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - BackendRouter::new(Box::new(api2), cli2, test_acp_backend()) - })))), - ); - - let dir = tempfile::tempdir().unwrap(); - let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env); - let run_options = RunOptions { - settings: WorkflowSettings::default(), - run_dir: dir.path().to_path_buf(), - cancel_token: CancellationToken::new(), - run_id: test_run_id("test-run"), - labels: std::collections::HashMap::new(), - workflow_slug: None, - github_app: None, - base_branch: None, - display_base_sha: None, - pre_run_git: None, - fork_source_ref: None, - git: None, - }; - let (outcome, state) = engine - .run_with_state(&graph, &run_options) - .await - .expect("pipeline should succeed"); - assert_eq!(outcome.status, StageOutcome::Succeeded); - - let api_response = state - .stage(&fabro_types::StageId::new("api_work", 1)) - .and_then(|node| node.response.as_deref()) - .unwrap(); - assert!( - api_response.starts_with("Response for api_work"), - "API node should use mock: {api_response}" - ); - - let cli_response = state - .stage(&fabro_types::StageId::new("cli_work", 1)) - .and_then(|node| node.response.as_deref()) - .unwrap(); - assert_eq!( - cli_response, "CLI completed the task.", - "CLI node should use CLI backend: {cli_response}" - ); - - let provider_json = state - .stage(&fabro_types::StageId::new("cli_work", 1)) - .unwrap() - .provider_used - .as_ref() - .unwrap() - .clone(); - assert_eq!(provider_json["mode"], "cli"); -} - -// -- Stylesheet applies backend property to nodes in a full pipeline -- - -#[tokio::test] -async fn stylesheet_backend_property_routes_to_cli() { - let claude_output = r#"{"type":"result","result":"Styled CLI response.","usage":{"input_tokens":10,"output_tokens":5}}"#; - let env: Arc = Arc::new(CliTestEnv::new(claude_output)); - - let mut graph = Graph::new("StylesheetTest"); - graph.attrs.insert( - "model_stylesheet".to_string(), - AttrValue::String(".cli-node { backend: cli; }".to_string()), - ); - - let mut start = Node::new("start"); - start.attrs.insert( - "shape".to_string(), - AttrValue::String("Mdiamond".to_string()), - ); - graph.nodes.insert("start".to_string(), start); - - let mut exit = Node::new("exit"); - exit.attrs.insert( - "shape".to_string(), - AttrValue::String("Msquare".to_string()), - ); - graph.nodes.insert("exit".to_string(), exit); - - let mut work = Node::new("work"); - work.attrs - .insert("shape".to_string(), AttrValue::String("box".to_string())); - work.attrs.insert( - "prompt".to_string(), - AttrValue::String("Do work".to_string()), - ); - work.classes.push("cli-node".to_string()); - graph.nodes.insert("work".to_string(), work); - - graph.edges.push(Edge::new("start", "work")); - graph.edges.push(Edge::new("work", "exit")); - - // Apply stylesheet - let ss = parse_stylesheet(graph.model_stylesheet()).unwrap(); - apply_stylesheet(&ss, &mut graph); - - // Verify the stylesheet applied the backend property - assert_eq!( - graph.nodes["work"].backend(), - Some("cli"), - "stylesheet should set backend=cli on .cli-node" - ); - - // Run the pipeline - let api = MockCodergenBackend; - let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let router = BackendRouter::new(Box::new(api), cli, test_acp_backend()); - - let mut registry = HandlerRegistry::new(Box::new(AgentHandler::new(Some(Box::new(router))))); - registry.register("start", Box::new(StartHandler)); - registry.register("exit", Box::new(ExitHandler)); - let api2 = MockCodergenBackend; - let cli2 = AgentCliBackend::new_from_env("claude-opus-4-6".into(), ProviderId::anthropic()); - let router2 = BackendRouter::new(Box::new(api2), cli2, test_acp_backend()); - registry.register( - "agent", - Box::new(AgentHandler::new(Some(Box::new(router2)))), - ); - - let dir = tempfile::tempdir().unwrap(); - let engine = WorkflowRunner::new(registry, Arc::new(Emitter::default()), env); - let run_options = RunOptions { - settings: WorkflowSettings::default(), - run_dir: dir.path().to_path_buf(), - cancel_token: CancellationToken::new(), - run_id: test_run_id("test-run"), - labels: std::collections::HashMap::new(), - workflow_slug: None, - github_app: None, - base_branch: None, - display_base_sha: None, - pre_run_git: None, - fork_source_ref: None, - git: None, - }; - let (outcome, state) = engine - .run_with_state(&graph, &run_options) - .await - .expect("pipeline should succeed"); - assert_eq!(outcome.status, StageOutcome::Succeeded); - - let response = state - .stage(&fabro_types::StageId::new("work", 1)) - .and_then(|node| node.response.as_deref()) - .unwrap(); - assert_eq!( - response, "Styled CLI response.", - "stylesheet-driven node should use CLI backend" - ); -} - -/// Verify parse_cli_response works against real Claude CLI output captured from -/// stream-json. -#[test] -fn parse_real_claude_stream_json() { - // Real output captured from: claude -p --output-format stream-json --model - // haiku "What is 2+2?" - let output = r#"{"type":"system","subtype":"init","cwd":"/tmp","session_id":"abc"} -{"type":"assistant","message":{"content":[{"type":"text","text":"4"}]}} -{"type":"result","subtype":"success","is_error":false,"duration_ms":2000,"num_turns":1,"result":"4","usage":{"input_tokens":9,"output_tokens":5}}"#; - let response = parse_cli_response(AgentProfileKind::Anthropic, output).unwrap(); - assert_eq!(response.text, "4"); - assert_eq!(response.input_tokens, 9); - assert_eq!(response.output_tokens, 5); -} - -/// Verify parse_cli_response works against real Codex CLI output. -#[test] -fn parse_real_codex_ndjson() { - // Real output captured from: echo "What is 2+2?" | codex exec --json - let output = r#"{"type":"thread.started","thread_id":"019ca1ec-1e86-79b2-b2b2-b1d963f1aea2"} -{"type":"turn.started"} -{"type":"item.completed","item":{"id":"item_0","type":"reasoning","text":"**Confirming simple numeric reply**"}} -{"type":"item.completed","item":{"id":"item_1","type":"agent_message","text":"4"}} -{"type":"turn.completed","usage":{"input_tokens":7999,"cached_input_tokens":7040,"output_tokens":33}}"#; - let response = parse_cli_response(AgentProfileKind::OpenAi, output).unwrap(); - assert_eq!(response.text, "4"); - assert_eq!(response.input_tokens, 7999); - assert_eq!(response.output_tokens, 33); -} - -/// Verify parse_cli_response works against real Gemini CLI output. -#[test] -fn parse_real_gemini_json() { - // Real output captured from: gemini "What is 2+2?" -m gemini-2.5-flash - // --sandbox -o json - let output = r#"{"session_id":"abc","response":"4","stats":{"models":{"gemini-2.5-flash":{"api":{"totalRequests":1,"totalErrors":0,"totalLatencyMs":618},"tokens":{"input":123,"prompt":8911,"candidates":1,"total":8912,"cached":8788,"thoughts":0,"tool":0}}},"tools":{"totalCalls":0},"files":{"totalLinesAdded":0,"totalLinesRemoved":0}}}"#; - let response = parse_cli_response(AgentProfileKind::Gemini, output).unwrap(); - assert_eq!(response.text, "4"); - assert_eq!(response.input_tokens, 123); - assert_eq!(response.output_tokens, 1); -} - // --------------------------------------------------------------------------- // Git checkpoint e2e (Local) // ---------------------------------------------------------------------------