From 02d4ab896c668decf6ccf3da0c120332c95df6c5 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 14:59:28 -0400 Subject: [PATCH] Support graph-level acp.command and acp.config defaults Workflows that run the same ACP agent on several nodes had to repeat the process attribute on every node. `acp.command` and `acp.config` were read only from the node (`Node::acp_command_attr`), with no graph-level fallback. Add `AcpDefaultsTransform`, which materializes the graph-level value onto nodes before validation. The handler's `resolve_acp_process_spec` takes only a `Node`, so reading the graph at the use site (as `retry_target` does) would mean threading a `Graph` through the handler. As a transform, neither the handler nor `backend_valid` changes, and `fabro validate` and `fabro run` stay in agreement because both go through `pipeline::transform`. The two attributes are mutually exclusive, so they inherit as a pair: a node setting either one keeps its own and inherits neither. Only nodes with `backend="acp"` inherit, keeping the attributes off `start`/`exit` and API nodes. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/core-concepts/agents.mdx | 26 +++ docs/public/reference/dot-language.mdx | 8 +- lib/apps/fabro-cli/tests/it/workflow/acp.rs | 57 ++++++ .../fabro-workflow/src/pipeline/transform.rs | 5 +- .../src/transforms/acp_defaults.rs | 186 ++++++++++++++++++ .../fabro-workflow/src/transforms/mod.rs | 2 + lib/foundation/fabro-types/src/graph.rs | 12 ++ 7 files changed, 293 insertions(+), 3 deletions(-) create mode 100644 lib/components/fabro-workflow/src/transforms/acp_defaults.rs diff --git a/docs/public/core-concepts/agents.mdx b/docs/public/core-concepts/agents.mdx index e53b12ed2e..8a1c81872b 100644 --- a/docs/public/core-concepts/agents.mdx +++ b/docs/public/core-concepts/agents.mdx @@ -50,6 +50,32 @@ implement [ ] ``` +#### Sharing one agent across nodes + +When several nodes run the same ACP agent, set `acp.command` or `acp.config` once as a graph attribute instead of repeating it on every node: + +```dot +digraph Review { + graph [ + goal="Plan, implement, and review a change" + acp.command="python3 tools/fake_acp_agent.py" + ] + + start [shape=Mdiamond] + exit [shape=Msquare] + + plan [label="Plan", backend="acp"] + implement [label="Implement", backend="acp"] + review [label="Review", backend="acp", acp.command="python3 tools/reviewer.py"] + + start -> plan -> implement -> review -> exit +} +``` + +Only nodes with `backend="acp"` inherit the graph-level value, so it never lands on `start`, `exit`, or API nodes. + +The two attributes stay mutually exclusive, and they are inherited as a pair: a node that sets *either* one keeps its own and inherits neither. In the example above, `review` uses its own command; had it set `acp.config` instead, it would use that alone rather than combining it with the graph's `acp.command`. + 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. diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 8c350d1ba5..df9b8d38f7 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -79,6 +79,8 @@ rankdir=LR | `default_max_retries` | Integer | Default retry count for all nodes (default: 0) | | `retry_target` | String | Default node ID to jump to on retry | | `fallback_retry_target` | String | Fallback retry target if primary target fails | +| `acp.command` | String | Default shell command for every `backend="acp"` node that sets neither ACP attribute itself. Mutually exclusive with `acp.config` | +| `acp.config` | String | Default JSON stdio ACP config for every `backend="acp"` node that sets neither ACP attribute itself. Mutually exclusive with `acp.command` | | `default_fidelity` | String | Default [fidelity level](/execution/context) for all nodes | | `default_thread` | String | Default thread ID for all nodes | | `max_node_visits` | Integer | Max visits per node across the run (0 = unlimited) | @@ -209,8 +211,10 @@ Start nodes can also be identified by ID (`start` or `Start`). Exit nodes can be | `output_schema` | String | Optional structured output validation. Use `routing` for Fabro's built-in routing directive schema, `@path/to/schema.json` for a JSON Schema file, or an inline JSON Schema object string. Supported on agent, prompt, and command nodes. | | `output_retries` | Integer | Corrective structured-output turns inside the same prompt conversation or agent session. Default `2`; `0` validates once and fails without repair; negative values are treated as `0`. Separate from `max_retries`. | | `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`. | +| `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. Falls back to the graph attribute of the same name. | +| `acp.config` | String | JSON stdio ACP config for nodes with `backend="acp"`. Mutually exclusive with `acp.command`. Falls back to the graph attribute of the same name. | + +Setting either attribute on a node suppresses *both* graph-level defaults, so a node can name its own process without inheriting a conflicting one. See [Agents — Sharing one agent across nodes](/core-concepts/agents#sharing-one-agent-across-nodes). #### Structured output validation diff --git a/lib/apps/fabro-cli/tests/it/workflow/acp.rs b/lib/apps/fabro-cli/tests/it/workflow/acp.rs index 9cd0845ae3..9a313a0cc9 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/acp.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/acp.rs @@ -87,6 +87,63 @@ fn acp_backend_workflow() { ); } +#[test] +fn graph_level_acp_config_is_shared_by_every_acp_node() { + let mut context = test_context!(); + context.write_home( + ".fabro/settings.toml", + "[server.auth]\nmethods = [\"dev-token\"]\n", + ); + context.isolated_server(); + let fake_agent = write_fake_acp_agent(&context); + let acp_config = fake_acp_config_attr(&fake_agent); + let workflow = context.temp_dir.join("acp_graph_default.fabro"); + context.write_temp( + "acp_graph_default.fabro", + format!( + r#"digraph ACPGraphDefault {{ + graph [goal="Exercise graph-level ACP defaults", acp.config={acp_config}] + start [shape=Mdiamond] + first [type="agent", backend="acp", prompt="write hello.txt"] + second [type="agent", backend="acp", prompt="write hello.txt"] + exit [shape=Msquare] + start -> first + first -> second + second -> exit +}}"# + ), + ); + init_git_repo(&context.temp_dir); + + context + .run_cmd() + .args(["--auto-approve", "--environment", "local"]) + .arg(&workflow) + .assert() + .success(); + + let run_dir = find_run_dir(&context); + let conclusion = read_conclusion(&run_dir); + assert_eq!(conclusion["status"].as_str(), Some("succeeded")); + + // Both nodes launched the ACP process named only at the graph level. + let events = run_events(&run_dir); + for node_id in ["first", "second"] { + let completed = events + .iter() + .find_map(|event| match &event.event.body { + EventBody::StageCompleted(props) + if event.event.node_id.as_deref() == Some(node_id) => + { + Some(props) + } + _ => None, + }) + .unwrap_or_else(|| panic!("{node_id} stage should complete")); + assert_eq!(completed.response.as_deref(), Some("hello from acp")); + } +} + #[test] fn acp_backend_does_not_inject_registered_provider_credentials() { let mut context = test_context!(); diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index b399d66373..a1608ab146 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use super::types::{Parsed, TransformOptions, Transformed}; use crate::error::Error; use crate::transforms::{ - FileInliningTransform, ImportTransform, ModelResolutionTransform, + AcpDefaultsTransform, FileInliningTransform, ImportTransform, ModelResolutionTransform, StylesheetApplicationTransform, TemplateTransform, Transform, }; @@ -63,6 +63,9 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result Result { + let mut graph = graph; + + let command = graph.acp_command_attr().map(ToString::to_string); + let config = graph.acp_config_attr().map(ToString::to_string); + if command.is_none() && config.is_none() { + return Ok(graph); + } + + for node in graph.nodes.values_mut() { + if node.agent_backend() != Some(Ok(AgentBackend::Acp)) { + continue; + } + if node.acp_command_attr().is_some() || node.acp_config_attr().is_some() { + continue; + } + + // A graph that sets both is ambiguous. Copy both so the existing + // "requires exactly one" check reports it, rather than silently + // picking one. + if let Some(command) = &command { + node.attrs.insert( + "acp.command".to_string(), + AttrValue::String(command.clone()), + ); + } + if let Some(config) = &config { + node.attrs + .insert("acp.config".to_string(), AttrValue::String(config.clone())); + } + } + + Ok(graph) + } +} + +#[cfg(test)] +mod tests { + use fabro_graphviz::graph::Node; + + use super::*; + + fn acp_node(id: &str) -> Node { + let mut node = Node::new(id); + node.attrs + .insert("backend".to_string(), AttrValue::String("acp".to_string())); + node + } + + fn apply(graph: Graph) -> Graph { + AcpDefaultsTransform.apply(graph).unwrap() + } + + fn attr<'a>(graph: &'a Graph, node_id: &str, key: &str) -> Option<&'a str> { + graph.nodes[node_id] + .attrs + .get(key) + .and_then(AttrValue::as_str) + } + + #[test] + fn graph_command_fills_in_acp_nodes_that_set_neither_attribute() { + let mut graph = Graph::new("test"); + graph.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 agent.py".to_string()), + ); + graph.nodes.insert("work".to_string(), acp_node("work")); + + let graph = apply(graph); + + assert_eq!( + attr(&graph, "work", "acp.command"), + Some("python3 agent.py") + ); + } + + #[test] + fn graph_config_fills_in_acp_nodes_that_set_neither_attribute() { + let config = r#"{"type":"stdio","command":"python3","args":["agent.py"]}"#; + let mut graph = Graph::new("test"); + graph.attrs.insert( + "acp.config".to_string(), + AttrValue::String(config.to_string()), + ); + graph.nodes.insert("work".to_string(), acp_node("work")); + + let graph = apply(graph); + + assert_eq!(attr(&graph, "work", "acp.config"), Some(config)); + } + + #[test] + fn node_command_wins_over_graph_command() { + let mut graph = Graph::new("test"); + graph.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 shared.py".to_string()), + ); + let mut node = acp_node("work"); + node.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 own.py".to_string()), + ); + graph.nodes.insert("work".to_string(), node); + + let graph = apply(graph); + + assert_eq!(attr(&graph, "work", "acp.command"), Some("python3 own.py")); + } + + #[test] + fn node_config_suppresses_the_graph_command() { + // The pair resolves per source: a node naming its own process must not + // inherit the other half from the graph and become ambiguous. + let config = r#"{"type":"stdio","command":"python3","args":["own.py"]}"#; + let mut graph = Graph::new("test"); + graph.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 shared.py".to_string()), + ); + let mut node = acp_node("work"); + node.attrs.insert( + "acp.config".to_string(), + AttrValue::String(config.to_string()), + ); + graph.nodes.insert("work".to_string(), node); + + let graph = apply(graph); + + assert_eq!(attr(&graph, "work", "acp.config"), Some(config)); + assert_eq!(attr(&graph, "work", "acp.command"), None); + } + + #[test] + fn non_acp_nodes_do_not_inherit() { + let mut graph = Graph::new("test"); + graph.attrs.insert( + "acp.command".to_string(), + AttrValue::String("python3 agent.py".to_string()), + ); + graph.nodes.insert("start".to_string(), Node::new("start")); + let mut api_node = Node::new("ask"); + api_node + .attrs + .insert("backend".to_string(), AttrValue::String("api".to_string())); + graph.nodes.insert("ask".to_string(), api_node); + + let graph = apply(graph); + + assert_eq!(attr(&graph, "start", "acp.command"), None); + assert_eq!(attr(&graph, "ask", "acp.command"), None); + } + + #[test] + fn graph_without_acp_attributes_is_unchanged() { + let mut graph = Graph::new("test"); + graph.nodes.insert("work".to_string(), acp_node("work")); + + let graph = apply(graph); + + assert_eq!(attr(&graph, "work", "acp.command"), None); + assert_eq!(attr(&graph, "work", "acp.config"), None); + } +} diff --git a/lib/components/fabro-workflow/src/transforms/mod.rs b/lib/components/fabro-workflow/src/transforms/mod.rs index 1f6879cda3..cfbee5c21d 100644 --- a/lib/components/fabro-workflow/src/transforms/mod.rs +++ b/lib/components/fabro-workflow/src/transforms/mod.rs @@ -8,6 +8,7 @@ pub trait Transform { fn apply(&self, graph: Graph) -> Result; } +mod acp_defaults; mod file_inlining; mod import; mod importable_field; @@ -17,6 +18,7 @@ pub mod stylesheet; mod stylesheet_application; pub mod variable_expansion; +pub use acp_defaults::AcpDefaultsTransform; pub use file_inlining::FileInliningTransform; pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index 7ef9bad991..8dd40f00ef 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -450,6 +450,18 @@ impl Graph { .unwrap_or(0) } + /// Graph-level `acp.command`, the default for ACP nodes that set neither + /// ACP process attribute themselves. + pub fn acp_command_attr(&self) -> Option<&str> { + self.attrs.get("acp.command").and_then(AttrValue::as_str) + } + + /// Graph-level `acp.config`, the default for ACP nodes that set neither + /// ACP process attribute themselves. + pub fn acp_config_attr(&self) -> Option<&str> { + self.attrs.get("acp.config").and_then(AttrValue::as_str) + } + /// Graph-level `retry_target`. pub fn retry_target(&self) -> Option<&str> { self.attrs.get("retry_target").and_then(AttrValue::as_str)