From 69a51e65b923ddcedee695d118083d172307bcf0 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Mon, 27 Jul 2026 14:59:31 -0400 Subject: [PATCH 1/2] feat(workflow): infer command nodes from the script attribute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A node with no `shape` defaulted to `box`, which resolves to the agent handler. That made a shapeless `script` node run as an LLM call prompted with its own label, while the `script` was reported as inert — wrong behavior behind a warning. `script` is read by the command handler and by nothing else, so a shapeless node that sets it is unambiguously a command node. `shape()` now infers `parallelogram` in that case. An explicit `shape` still wins. Two rules keep the inference honest: - `script_prompt_conflict` — setting both `script` and `prompt` is an error. No handler reads both. It fires regardless of shape so that adding one cannot downgrade the error to a warning. - `command_requires_script` — a command node without a script is an error. Without this the original trap just moves: a node meant as a command that omits its script silently becomes an agent again. Also drops the `tool_command` alias in favor of `script` alone, routing the six read sites through a new `Node::script()` accessor. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/reference/dot-language.mdx | 17 ++- docs/public/workflows/stages-and-nodes.mdx | 10 +- lib/apps/fabro-cli/tests/it/cmd/validate.rs | 8 +- .../tests/it/workflow/dry_run_examples.rs | 8 +- .../src/rules/command_requires_script.rs | 131 +++++++++++++++++ .../fabro-validate/src/rules/mod.rs | 4 + .../src/rules/script_absolute_cd.rs | 28 ++-- .../src/rules/script_prompt_conflict.rs | 135 ++++++++++++++++++ .../fabro-workflow/src/handler/command.rs | 27 +--- .../src/handler/llm/preamble.rs | 33 +---- lib/foundation/fabro-types/src/graph.rs | 67 ++++++++- test/attractor/reference_template.dot | 24 ++-- test/inferred_command.fabro | 11 ++ test/legacy_tool.fabro | 11 -- 14 files changed, 419 insertions(+), 95 deletions(-) create mode 100644 lib/components/fabro-validate/src/rules/command_requires_script.rs create mode 100644 lib/components/fabro-validate/src/rules/script_prompt_conflict.rs create mode 100644 test/inferred_command.fabro delete mode 100644 test/legacy_tool.fabro diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 8c350d1ba..27b479f14 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -161,7 +161,7 @@ Each node's `shape` attribute determines its execution behavior. See [Nodes & St | `Msquare` | exit | Workflow terminal (exactly one required) | | `box` (default) | agent | Multi-turn LLM with tool access | | `tab` | prompt | Single LLM call, no tools | -| `parallelogram` | command | Execute a shell script | +| `parallelogram` (inferred from `script`) | command | Execute a shell script | | `hexagon` | human | Human-in-the-loop decision gate | | `diamond` | conditional | Route based on conditions | | `component` | parallel | Fan-out to concurrent branches | @@ -173,6 +173,19 @@ The `type` attribute can also be set explicitly to override the shape-based mapp Start nodes can also be identified by ID (`start` or `Start`). Exit nodes can be identified by ID (`exit`, `Exit`, `end`, or `End`). +### Omitting the shape + +The two most common node types don't need a `shape`. A node that sets `script` is a command node; every other shapeless node is an agent node: + +```dot +plan [prompt="Plan the work"] // agent +build [script="cargo build"] // command +``` + +An explicit `shape` always wins, so `box` with a `script` is still an agent node — and the `script` is then read by nothing. Setting both `script` and `prompt` on one node is an error: no handler reads both. + +Other node types still need their shape, because their attributes don't identify them uniquely. In particular, agent and prompt nodes take the same attributes, so a prompt node needs `shape=tab`. + ## Node attributes ### All nodes @@ -244,7 +257,7 @@ audit [ | Attribute | Type | Description | |---|---|---| -| `script` | String | Shell command to execute | +| `script` | String | Shell command to execute (required). Its presence also makes `shape=parallelogram` optional. | | `language` | String | `"shell"` (default) or `"python"` | | `output_schema` | String | Optional structured output validation. Accepts `routing`, `@path/to/schema.json`, or an inline JSON Schema object string. See [Structured output validation](#structured-output-validation). | diff --git a/docs/public/workflows/stages-and-nodes.mdx b/docs/public/workflows/stages-and-nodes.mdx index 819befe7e..c03061fa7 100644 --- a/docs/public/workflows/stages-and-nodes.mdx +++ b/docs/public/workflows/stages-and-nodes.mdx @@ -35,7 +35,7 @@ exit [shape=Msquare, label="Exit"] ### Agent -**Shape:** `box` (default) +**Shape:** `box` (default — a node with no shape and no `script` is an agent node) Runs an LLM with access to tools — bash, file editing, sub-agents — in an agentic loop. The agent works autonomously, calling tools as needed, until it decides the task is complete. @@ -94,17 +94,19 @@ Prompt nodes accept the same attributes as agent nodes (`prompt`, `reasoning_eff ### Command -**Shape:** `parallelogram` +**Shape:** `parallelogram` (optional — inferred from `script`) Runs a shell script inside the configured sandbox and captures its output. The output is available to downstream nodes as context. This ensures command nodes execute in the same environment as agent nodes. ```dot -test [label="Run Tests", shape=parallelogram, script="cargo test 2>&1 || true"] +test [label="Run Tests", script="cargo test 2>&1 || true"] ``` +`script` is what makes a node a command node, so the shape can be left off. Writing `shape=parallelogram` explicitly is still valid and does the same thing. + | Attribute | Description | |---|---| -| `script` | The shell command to execute | +| `script` | The shell command to execute (required) | | `language` | `"shell"` (default) or `"python"` | ### Human diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 190c2817b..31adc385c 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -135,17 +135,17 @@ fn styled() { } #[test] -fn legacy_tool() { +fn inferred_command() { let context = test_context!(); let mut cmd = context.validate(); - cmd.arg(fixture("legacy_tool.fabro")); + cmd.arg(fixture("inferred_command.fabro")); fabro_snapshot!(context.filters(), cmd, @" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: LegacyTool (3 nodes, 2 edges) - Graph: [FIXTURES]/legacy_tool.fabro + Workflow: InferredCommand (3 nodes, 2 edges) + Graph: [FIXTURES]/inferred_command.fabro Validation: OK "); } diff --git a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs index 658dd1fea..28ed675ec 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs @@ -148,9 +148,9 @@ fn dry_run_styled() { } #[test] -fn dry_run_legacy_tool() { +fn dry_run_inferred_command() { let context = test_context!(); - let workflow = context.install_fixture("legacy_tool.fabro"); + let workflow = context.install_fixture("inferred_command.fabro"); let mut cmd = context.run_cmd(); cmd.args(["--dry-run", "--auto-approve"]); cmd.arg(&workflow); @@ -159,9 +159,9 @@ fn dry_run_legacy_tool() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: LegacyTool (3 nodes, 2 edges) + Workflow: InferredCommand (3 nodes, 2 edges) Graph: [GRAPH_PATH] - Goal: Verify backwards compatibility with old tool naming + Goal: Verify a shapeless script node runs as a command Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] diff --git a/lib/components/fabro-validate/src/rules/command_requires_script.rs b/lib/components/fabro-validate/src/rules/command_requires_script.rs new file mode 100644 index 000000000..f6f819d8e --- /dev/null +++ b/lib/components/fabro-validate/src/rules/command_requires_script.rs @@ -0,0 +1,131 @@ +use fabro_graphviz::graph::Graph; + +use crate::{Diagnostic, LintRule, Severity}; + +pub(super) fn rule() -> Box { + Box::new(Rule) +} + +struct Rule; + +impl LintRule for Rule { + fn name(&self) -> &'static str { + "command_requires_script" + } + + fn apply(&self, graph: &Graph) -> Vec { + let mut diagnostics = Vec::new(); + for node in graph.nodes.values() { + if node.handler_type() != Some("command") { + continue; + } + if node.script().is_some_and(|s| !s.trim().is_empty()) { + continue; + } + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: format!("Command node '{}' has no 'script' to run", node.id), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Add a 'script' attribute, or remove the command shape or type if this was \ + meant to be an agent node" + .to_string(), + ), + ..Diagnostic::default() + }); + } + diagnostics + } +} + +#[cfg(test)] +mod tests { + use fabro_graphviz::graph::{AttrValue, Node}; + + use super::Rule; + use crate::rules::test_support::minimal_graph; + use crate::{LintRule, Severity}; + + fn node_with(id: &str, attrs: &[(&str, &str)]) -> Node { + let mut node = Node::new(id); + for (key, value) in attrs { + node.attrs + .insert((*key).to_string(), AttrValue::String((*value).to_string())); + } + node + } + + #[test] + fn errors_on_command_shape_without_script() { + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with("run", &[("shape", "parallelogram"), ("label", "Build")]), + ); + + let d = Rule.apply(&g); + + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert!(d[0].message.contains("no 'script'")); + assert_eq!(d[0].node_id.as_deref(), Some("run")); + } + + #[test] + fn errors_on_explicit_command_type_without_script() { + let mut g = minimal_graph(); + g.nodes + .insert("run".to_string(), node_with("run", &[("type", "command")])); + + let d = Rule.apply(&g); + + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + } + + #[test] + fn errors_on_blank_script() { + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with("run", &[("shape", "parallelogram"), ("script", " ")]), + ); + + assert_eq!(Rule.apply(&g).len(), 1); + } + + #[test] + fn accepts_command_node_with_script() { + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with("run", &[ + ("shape", "parallelogram"), + ("script", "cargo build"), + ]), + ); + g.nodes.insert( + "build".to_string(), + node_with("build", &[("script", "cargo build")]), + ); + + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn ignores_non_command_nodes() { + let mut g = minimal_graph(); + g.nodes.insert( + "plan".to_string(), + node_with("plan", &[("prompt", "do it")]), + ); + g.nodes.insert( + "gate".to_string(), + node_with("gate", &[("shape", "hexagon")]), + ); + + assert!(Rule.apply(&g).is_empty()); + } +} diff --git a/lib/components/fabro-validate/src/rules/mod.rs b/lib/components/fabro-validate/src/rules/mod.rs index cfcf26b12..07be69153 100644 --- a/lib/components/fabro-validate/src/rules/mod.rs +++ b/lib/components/fabro-validate/src/rules/mod.rs @@ -1,5 +1,6 @@ mod all_conditional_edges; mod backend_valid; +mod command_requires_script; mod condition_syntax; mod direction_valid; mod edge_target_exists; @@ -21,6 +22,7 @@ mod reachability; mod reserved_keyword_node_id; mod retry_target_exists; mod script_absolute_cd; +mod script_prompt_conflict; mod selection_valid; mod start_no_incoming; mod start_node; @@ -59,6 +61,8 @@ pub fn built_in_rules() -> Vec> { all_conditional_edges::rule(), orphan_custom_outcome::rule(), script_absolute_cd::rule(), + script_prompt_conflict::rule(), + command_requires_script::rule(), import_error::rule(), join_policy_removed::rule(), unresolved_file_ref::rule(), diff --git a/lib/components/fabro-validate/src/rules/script_absolute_cd.rs b/lib/components/fabro-validate/src/rules/script_absolute_cd.rs index 561f5960d..6b5083ad1 100644 --- a/lib/components/fabro-validate/src/rules/script_absolute_cd.rs +++ b/lib/components/fabro-validate/src/rules/script_absolute_cd.rs @@ -40,12 +40,7 @@ impl LintRule for Rule { if node.handler_type() != Some("command") { continue; } - let script = node - .attrs - .get("script") - .or_else(|| node.attrs.get("tool_command")) - .and_then(|v| v.as_str()) - .unwrap_or(""); + let script = node.script().unwrap_or(""); if contains_cd_absolute(script) { diagnostics.push(Diagnostic { rule: self.name().to_string(), @@ -114,7 +109,22 @@ mod tests { } #[test] - fn script_absolute_cd_warns_on_legacy_tool_command() { + fn script_absolute_cd_warns_on_shapeless_script_node() { + let mut g = minimal_graph(); + let mut node = Node::new("run"); + node.attrs.insert( + "script".to_string(), + AttrValue::String("cd /home/user && make".to_string()), + ); + g.nodes.insert("run".to_string(), node); + let rule = Rule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + } + + #[test] + fn script_absolute_cd_ignores_legacy_tool_command() { let mut g = minimal_graph(); let mut node = Node::new("run"); node.attrs.insert( @@ -127,9 +137,7 @@ mod tests { ); g.nodes.insert("run".to_string(), node); let rule = Rule; - let d = rule.apply(&g); - assert_eq!(d.len(), 1); - assert_eq!(d[0].severity, Severity::Warning); + assert!(rule.apply(&g).is_empty()); } #[test] diff --git a/lib/components/fabro-validate/src/rules/script_prompt_conflict.rs b/lib/components/fabro-validate/src/rules/script_prompt_conflict.rs new file mode 100644 index 000000000..39f551aa3 --- /dev/null +++ b/lib/components/fabro-validate/src/rules/script_prompt_conflict.rs @@ -0,0 +1,135 @@ +use fabro_graphviz::graph::Graph; + +use crate::{Diagnostic, LintRule, Severity}; + +pub(super) fn rule() -> Box { + Box::new(Rule) +} + +struct Rule; + +impl LintRule for Rule { + fn name(&self) -> &'static str { + "script_prompt_conflict" + } + + fn apply(&self, graph: &Graph) -> Vec { + let mut diagnostics = Vec::new(); + for node in graph.nodes.values() { + if node.script().is_none() || node.prompt().is_none() { + continue; + } + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: format!( + "Node '{}' sets both 'script' and 'prompt'. No node type reads both: \ + 'script' selects the command handler and 'prompt' selects an LLM handler", + node.id + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Remove whichever attribute is wrong, or split the node into a command node \ + and an agent node" + .to_string(), + ), + ..Diagnostic::default() + }); + } + diagnostics + } +} + +#[cfg(test)] +mod tests { + use fabro_graphviz::graph::{AttrValue, Node}; + + use super::Rule; + use crate::rules::test_support::minimal_graph; + use crate::{LintRule, Severity}; + + fn node_with(id: &str, attrs: &[(&str, &str)]) -> Node { + let mut node = Node::new(id); + for (key, value) in attrs { + node.attrs + .insert((*key).to_string(), AttrValue::String((*value).to_string())); + } + node + } + + #[test] + fn errors_when_shapeless_node_sets_both() { + let mut g = minimal_graph(); + g.nodes.insert( + "work".to_string(), + node_with("work", &[("script", "cargo build"), ("prompt", "do it")]), + ); + + let d = Rule.apply(&g); + + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert!(d[0].message.contains("'script'")); + assert!(d[0].message.contains("'prompt'")); + assert_eq!(d[0].node_id.as_deref(), Some("work")); + } + + #[test] + fn errors_even_when_shape_is_explicit() { + // An explicit shape resolves the handler, but the unread attribute is + // still a mistake, and reporting it the same way everywhere keeps + // adding a shape from turning an error into a warning. + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with("run", &[ + ("shape", "parallelogram"), + ("script", "cargo build"), + ("prompt", "do it"), + ]), + ); + g.nodes.insert( + "plan".to_string(), + node_with("plan", &[ + ("shape", "box"), + ("script", "cargo build"), + ("prompt", "do it"), + ]), + ); + + let d = Rule.apply(&g); + + assert_eq!(d.len(), 2); + assert!(d.iter().all(|d| d.severity == Severity::Error)); + } + + #[test] + fn accepts_either_attribute_alone() { + let mut g = minimal_graph(); + g.nodes.insert( + "build".to_string(), + node_with("build", &[("script", "cargo build")]), + ); + g.nodes.insert( + "plan".to_string(), + node_with("plan", &[("prompt", "do it")]), + ); + + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn ignores_legacy_tool_command_attribute() { + let mut g = minimal_graph(); + g.nodes.insert( + "work".to_string(), + node_with("work", &[ + ("tool_command", "cargo build"), + ("prompt", "do it"), + ]), + ); + + assert!(Rule.apply(&g).is_empty()); + } +} diff --git a/lib/components/fabro-workflow/src/handler/command.rs b/lib/components/fabro-workflow/src/handler/command.rs index 74a32f0ac..66542743c 100644 --- a/lib/components/fabro-workflow/src/handler/command.rs +++ b/lib/components/fabro-workflow/src/handler/command.rs @@ -31,12 +31,7 @@ impl Handler for CommandHandler { _run_dir: &Path, _services: &EngineServices, ) -> Result { - let script = node - .attrs - .get("script") - .or_else(|| node.attrs.get("tool_command")) - .and_then(|v| v.as_str()) - .unwrap_or(""); + let script = node.script().unwrap_or(""); let mut outcome = Outcome::simulated(&node.id); outcome.notes = Some(format!("[Simulated] Command skipped: {script}")); @@ -54,12 +49,7 @@ impl Handler for CommandHandler { run_dir: &Path, services: &EngineServices, ) -> Result { - let script = node - .attrs - .get("script") - .or_else(|| node.attrs.get("tool_command")) - .and_then(|v| v.as_str()) - .unwrap_or(""); + let script = node.script().unwrap_or(""); if script.is_empty() { return Ok(Outcome::fail_classify("No script specified")); @@ -1264,7 +1254,7 @@ mod tests { } #[tokio::test] - async fn tool_command_attribute_fallback() { + async fn tool_command_attribute_is_not_read() { let handler = CommandHandler; let mut node = Node::new("script_node"); node.attrs.insert( @@ -1280,13 +1270,10 @@ mod tests { .execute(&node, &context, &graph, run_dir.path(), &services) .await .unwrap(); - assert_eq!(outcome.status, StageOutcome::Succeeded); - let command_output = outcome.context_updates.get(keys::COMMAND_OUTPUT).unwrap(); - assert!( - command_text(&services, command_output) - .await - .contains("legacy") - ); + assert_eq!(outcome.status, StageOutcome::Failed { + retry_requested: false, + }); + assert!(outcome.failure_reason().unwrap().contains("No script")); } #[tokio::test] diff --git a/lib/components/fabro-workflow/src/handler/llm/preamble.rs b/lib/components/fabro-workflow/src/handler/llm/preamble.rs index 77cd9a0b4..482e186ef 100644 --- a/lib/components/fabro-workflow/src/handler/llm/preamble.rs +++ b/lib/components/fabro-workflow/src/handler/llm/preamble.rs @@ -161,15 +161,8 @@ fn render_compact_stage_details( match handler { Some("command") => { let mut lines = Vec::new(); - if let Some(n) = node { - if let Some(cmd) = n - .attrs - .get("script") - .or_else(|| n.attrs.get("tool_command")) - .and_then(|v| v.as_str()) - { - lines.push(format!(" - Script: `{cmd}`")); - } + if let Some(cmd) = node.and_then(Node::script) { + lines.push(format!(" - Script: `{cmd}`")); } if let Some(output_val) = outcome.context_updates.get(keys::COMMAND_OUTPUT) { let output = format_value(output_val); @@ -215,15 +208,8 @@ fn render_summary_high_stage_section( match handler { Some("command") => { - if let Some(n) = node { - if let Some(cmd) = n - .attrs - .get("script") - .or_else(|| n.attrs.get("tool_command")) - .and_then(|v| v.as_str()) - { - lines.push(format!("- Script: `{cmd}`")); - } + if let Some(cmd) = node.and_then(Node::script) { + lines.push(format!("- Script: `{cmd}`")); } if let Some(output_val) = outcome.context_updates.get(keys::COMMAND_OUTPUT) { if let Some(path) = artifact_path(output_val) { @@ -531,15 +517,8 @@ fn build_summary_preamble( } match handler { Some("command") => { - if let Some(n) = node { - if let Some(cmd) = n - .attrs - .get("script") - .or_else(|| n.attrs.get("tool_command")) - .and_then(|v| v.as_str()) - { - parts.push(format!(" - Script: `{cmd}`")); - } + if let Some(cmd) = node.and_then(Node::script) { + parts.push(format!(" - Script: `{cmd}`")); } } h if is_llm_handler_type(h) => { diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index 7ef9bad99..9acf1b7c0 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -153,9 +153,21 @@ impl Node { self.str_attr("label").unwrap_or(&self.id) } + /// The node's Graphviz shape, which selects its handler. + /// + /// An explicit `shape` attribute always wins. Otherwise the shape is + /// inferred from the attributes the node carries: `script` is read by the + /// command handler and by nothing else, so a shapeless node that sets it + /// is a command node. Everything else falls back to `box` (agent). #[must_use] pub fn shape(&self) -> &str { - self.str_attr("shape").unwrap_or("box") + if let Some(shape) = self.str_attr("shape") { + return shape; + } + if self.script().is_some() { + return "parallelogram"; + } + "box" } #[must_use] @@ -168,6 +180,12 @@ impl Node { self.str_attr("prompt") } + /// The shell or Python source a command node runs. + #[must_use] + pub fn script(&self) -> Option<&str> { + self.str_attr("script") + } + #[must_use] pub fn output_schema(&self) -> Option<&str> { self.str_attr("output_schema") @@ -606,6 +624,53 @@ mod tests { assert!(node.project_memory()); } + fn node_with(id: &str, attrs: &[(&str, &str)]) -> Node { + let mut node = Node::new(id); + for (key, value) in attrs { + node.attrs + .insert((*key).to_string(), AttrValue::String((*value).to_string())); + } + node + } + + #[test] + fn shapeless_script_node_infers_command() { + let node = node_with("build", &[("script", "cargo build")]); + assert_eq!(node.shape(), "parallelogram"); + assert_eq!(node.handler_type(), Some("command")); + } + + #[test] + fn shapeless_node_without_script_stays_agent() { + let node = node_with("plan", &[("prompt", "Plan the work")]); + assert_eq!(node.shape(), "box"); + assert_eq!(node.handler_type(), Some("agent")); + } + + #[test] + fn explicit_shape_wins_over_script_inference() { + let node = node_with("odd", &[("shape", "box"), ("script", "cargo build")]); + assert_eq!(node.shape(), "box"); + assert_eq!(node.handler_type(), Some("agent")); + } + + #[test] + fn explicit_type_wins_over_script_inference() { + let node = node_with("odd", &[("type", "agent"), ("script", "cargo build")]); + assert_eq!(node.shape(), "parallelogram"); + assert_eq!(node.handler_type(), Some("agent")); + } + + #[test] + fn empty_script_still_infers_command() { + // The command-requires-script lint reports this; inference only asks + // whether the attribute is present so the diagnostic lands on a + // command node rather than a silently-agent one. + let node = node_with("build", &[("script", "")]); + assert_eq!(node.shape(), "parallelogram"); + assert_eq!(node.handler_type(), Some("command")); + } + #[test] fn node_project_memory_false_overrides_default() { let mut node = Node::new("x"); diff --git a/test/attractor/reference_template.dot b/test/attractor/reference_template.dot index 35c2fac31..dfc2ba453 100644 --- a/test/attractor/reference_template.dot +++ b/test/attractor/reference_template.dot @@ -43,11 +43,11 @@ digraph reference_template { start [shape=Mdiamond, label="Start"] // Toolchain gate — fail fast before LLM stages. - // Replace tool_command with project-specific checks. + // Replace script with project-specific checks. check_toolchain [ shape=parallelogram, max_retries=0, - tool_command="echo 'Replace with project-specific toolchain checks'; exit 0" + script="echo 'Replace with project-specific toolchain checks'; exit 0" ] // PROMPT: expand_spec @@ -187,42 +187,42 @@ digraph reference_template { check_implement [shape=diamond, label="Implement OK?"] // Auto-fix formatting before verify gate. - // Replace tool_command with project-specific auto-formatter. + // Replace script with project-specific auto-formatter. fix_fmt [ shape=parallelogram, max_retries=0, - tool_command="echo 'Replace with project-specific auto-formatter'; exit 0" + script="echo 'Replace with project-specific auto-formatter'; exit 0" ] - // Replace tool_command with project-specific formatter check. + // Replace script with project-specific formatter check. verify_fmt [ shape=parallelogram, max_retries=0, - tool_command="echo 'Replace with project-specific formatter check'; exit 0" + script="echo 'Replace with project-specific formatter check'; exit 0" ] check_fmt [shape=diamond, label="Fmt OK?"] - // Replace tool_command with project-specific build command. + // Replace script with project-specific build command. verify_build [ shape=parallelogram, - tool_command="echo 'Replace with project-specific build check'; exit 0" + script="echo 'Replace with project-specific build check'; exit 0" ] check_build [shape=diamond, label="Build OK?"] - // Replace tool_command with project-specific test command. + // Replace script with project-specific test command. verify_test [ shape=parallelogram, - tool_command="echo 'Replace with project-specific test check'; exit 0" + script="echo 'Replace with project-specific test check'; exit 0" ] check_test [shape=diamond, label="Tests OK?"] - // Replace tool_command with artifact hygiene check. + // Replace script with artifact hygiene check. // Confirm deliverables meet their interface contract (exports, endpoints, // CLI behavior, observable outputs); file existence alone is insufficient. verify_artifacts [ shape=parallelogram, max_retries=0, - tool_command="echo 'Replace with artifact hygiene check'; exit 0" + script="echo 'Replace with artifact hygiene check'; exit 0" ] check_artifacts [shape=diamond, label="Artifacts OK?"] diff --git a/test/inferred_command.fabro b/test/inferred_command.fabro new file mode 100644 index 000000000..2f0e5e5cc --- /dev/null +++ b/test/inferred_command.fabro @@ -0,0 +1,11 @@ +digraph InferredCommand { + graph [goal="Verify a shapeless script node runs as a command"] + rankdir=LR + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + echo_task [label="Echo", script="echo hello"] + + start -> echo_task -> exit +} diff --git a/test/legacy_tool.fabro b/test/legacy_tool.fabro deleted file mode 100644 index 6e0b389bc..000000000 --- a/test/legacy_tool.fabro +++ /dev/null @@ -1,11 +0,0 @@ -digraph LegacyTool { - graph [goal="Verify backwards compatibility with old tool naming"] - rankdir=LR - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - - echo_task [label="Echo", shape=parallelogram, tool_command="echo hello"] - - start -> echo_task -> exit -} From d37fc0027cff38bbcf79bf28de865016e7526768 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Jul 2026 10:54:43 -0400 Subject: [PATCH 2/2] fix(workflow): align inferred command behavior --- .../playground/state/parse-fabro.test.ts | 30 ++++ .../playground/state/parse-fabro.ts | 16 +- docs/public/reference/dot-language.mdx | 12 +- docs/public/workflows/stages-and-nodes.mdx | 8 +- .../src/rules/command_requires_script.rs | 65 ++++---- .../src/rules/inert_attribute.rs | 141 +++++++++++++++--- .../fabro-validate/src/rules/mod.rs | 2 - .../src/rules/script_prompt_conflict.rs | 135 ----------------- .../fabro-validate/src/rules/test_support.rs | 9 ++ .../fabro-workflow/src/handler/command.rs | 52 +++++-- lib/foundation/fabro-types/src/graph.rs | 39 +++-- 11 files changed, 285 insertions(+), 224 deletions(-) delete mode 100644 lib/components/fabro-validate/src/rules/script_prompt_conflict.rs diff --git a/apps/fabro-web/app/components/playground/state/parse-fabro.test.ts b/apps/fabro-web/app/components/playground/state/parse-fabro.test.ts index 1a16e878d..dc3340d2c 100644 --- a/apps/fabro-web/app/components/playground/state/parse-fabro.test.ts +++ b/apps/fabro-web/app/components/playground/state/parse-fabro.test.ts @@ -174,6 +174,36 @@ describe("parseFabro", () => { expect(draft.nodes.find((n) => n.id === "plain")?.shape).toBe("box"); }); + test("shapeless script node keeps command behavior after round trip", () => { + const draft = expectOk( + parseFabro(`digraph G { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + build [script="cargo build"] + start -> build -> exit + }`), + ); + + expect(draft.nodes.find((n) => n.id === "build")?.shape).toBe( + "parallelogram", + ); + + const reparsed = expectOk(parseFabro(renderFabro(draft))); + expect(reparsed.nodes.find((n) => n.id === "build")?.shape).toBe( + "parallelogram", + ); + }); + + test("explicit type disables command shape inference", () => { + const draft = expectOk( + parseFabro(`digraph G { + work [type=agent, script="cargo build"] + }`), + ); + + expect(draft.nodes.find((n) => n.id === "work")?.shape).toBe("box"); + }); + test("unknown shape falls back to box", () => { const draft = expectOk( parseFabro(`digraph G { diff --git a/apps/fabro-web/app/components/playground/state/parse-fabro.ts b/apps/fabro-web/app/components/playground/state/parse-fabro.ts index 3478093dd..632e4f392 100644 --- a/apps/fabro-web/app/components/playground/state/parse-fabro.ts +++ b/apps/fabro-web/app/components/playground/state/parse-fabro.ts @@ -147,7 +147,7 @@ export function parseFabro(src: string): ParseResult { } function buildNode(id: string, attrs: Record): Node { - const shape = coerceShape(attrs.shape, id); + const shape = coerceShape(attrs, id); const label = typeof attrs.label === "string" ? attrs.label : id; const node: Node = { id, label, shape }; if (typeof attrs.prompt === "string") node.prompt = attrs.prompt; @@ -159,10 +159,18 @@ function buildNode(id: string, attrs: Record): Node { return node; } -function coerceShape(raw: AttrValue | undefined, nodeId: string): Shape { +function coerceShape( + attrs: Record, + nodeId: string, +): Shape { + const raw = attrs.shape; if (typeof raw !== "string") { - // Shape omitted: default to start/exit terminals if id matches, - // otherwise `box` (Fabro's agent default). + // Without an explicit shape or type, `script` selects the command + // handler. Keep the inferred shape when the draft is rendered again. + if (typeof attrs.type !== "string" && attrs.script !== undefined) { + return "parallelogram"; + } + // The playground also infers terminal shapes from reserved ids. if (nodeId === "start") return "mdiamond"; if (nodeId === "exit") return "msquare"; return "box"; diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 9319fac04..beeb36bf8 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -153,7 +153,7 @@ Node and edge defaults declared inside a subgraph are scoped — they don't leak ## Node types -Each node's `shape` attribute determines its execution behavior. See [Nodes & Stages](/workflows/stages-and-nodes) for detailed documentation of each type. +Fabro uses an explicit `type` attribute to select a node's handler. Without `type`, the `shape` attribute selects the handler. See [Nodes & Stages](/workflows/stages-and-nodes) for detailed documentation of each type. | Shape | Handler | Purpose | |---|---|---| @@ -161,7 +161,7 @@ Each node's `shape` attribute determines its execution behavior. See [Nodes & St | `Msquare` | exit | Workflow terminal (exactly one required) | | `box` (default) | agent | Multi-turn LLM with tool access | | `tab` | prompt | Single LLM call, no tools | -| `parallelogram` (inferred from `script`) | command | Execute a shell script | +| `parallelogram` (inferred from `script` when `shape` and `type` are omitted) | command | Execute a shell script | | `hexagon` | human | Human-in-the-loop decision gate | | `diamond` | conditional | Route based on conditions | | `component` | parallel | Fan-out to concurrent branches | @@ -169,20 +169,20 @@ Each node's `shape` attribute determines its execution behavior. See [Nodes & St | `insulator` | wait | Pause for a duration | | `house` | stack.manager_loop | Sub-workflow orchestration | -The `type` attribute can also be set explicitly to override the shape-based mapping. +The `type` attribute can be set explicitly to override the shape-based mapping and attribute inference. Start nodes can also be identified by ID (`start` or `Start`). Exit nodes can be identified by ID (`exit`, `Exit`, `end`, or `End`). ### Omitting the shape -The two most common node types don't need a `shape`. A node that sets `script` is a command node; every other shapeless node is an agent node: +The two most common node types don't need a `shape` or `type`. When both are omitted, a node that sets `script` is a command node. Every other node defaults to an agent node: ```dot plan [prompt="Plan the work"] // agent build [script="cargo build"] // command ``` -An explicit `shape` always wins, so `box` with a `script` is still an agent node — and the `script` is then read by nothing. Setting both `script` and `prompt` on one node is an error: no handler reads both. +An explicit `shape` or `type` always wins. For example, `shape=box` with a `script` is still an agent node, and the `script` has no effect. Setting both `script` and `prompt` on one node is an error because command handlers consume `script`, while LLM handlers consume `prompt`. Other node types still need their shape, because their attributes don't identify them uniquely. In particular, agent and prompt nodes take the same attributes, so a prompt node needs `shape=tab`. @@ -257,7 +257,7 @@ audit [ | Attribute | Type | Description | |---|---|---| -| `script` | String | Shell command to execute (required). Its presence also makes `shape=parallelogram` optional. | +| `script` | String | Shell command to execute (required). Its presence infers a command node when `shape` and `type` are omitted. | | `language` | String | `"shell"` (default) or `"python"` | | `output_schema` | String | Optional structured output validation. Accepts `routing`, `@path/to/schema.json`, or an inline JSON Schema object string. See [Structured output validation](#structured-output-validation). | diff --git a/docs/public/workflows/stages-and-nodes.mdx b/docs/public/workflows/stages-and-nodes.mdx index 2588307cd..c74de9fea 100644 --- a/docs/public/workflows/stages-and-nodes.mdx +++ b/docs/public/workflows/stages-and-nodes.mdx @@ -11,7 +11,7 @@ This distinction matters for observability and debugging: the workflow graph sho ## Node types -Every node's Graphviz `shape` attribute determines its execution behavior. If no shape is specified, the node defaults to an agent. +An explicit `type` attribute selects a node's execution behavior. Otherwise, its Graphviz `shape` selects the behavior. When both are omitted, a node with `script` is a command node and every other node defaults to an agent. ### Start @@ -35,7 +35,7 @@ exit [shape=Msquare, label="Exit"] ### Agent -**Shape:** `box` (default — a node with no shape and no `script` is an agent node) +**Shape:** `box` (default when `shape`, `type`, and `script` are omitted) Runs an LLM with access to tools — bash, file editing, sub-agents — in an agentic loop. The agent works autonomously, calling tools as needed, until it decides the task is complete. @@ -94,7 +94,7 @@ Prompt nodes accept the same attributes as agent nodes (`prompt`, `reasoning_eff ### Command -**Shape:** `parallelogram` (optional — inferred from `script`) +**Shape:** `parallelogram` (optional when `shape` and `type` are omitted — inferred from `script`) Runs a shell script inside the configured sandbox and captures its output. The output is available to downstream nodes as context. This ensures command nodes execute in the same environment as agent nodes. @@ -102,7 +102,7 @@ Runs a shell script inside the configured sandbox and captures its output. The o test [label="Run Tests", script="cargo test 2>&1 || true"] ``` -`script` is what makes a node a command node, so the shape can be left off. Writing `shape=parallelogram` explicitly is still valid and does the same thing. +When a node has no explicit `shape` or `type`, the presence of `script` makes it a command node. Writing `shape=parallelogram` explicitly is still valid and does the same thing. | Attribute | Description | |---|---| diff --git a/lib/components/fabro-validate/src/rules/command_requires_script.rs b/lib/components/fabro-validate/src/rules/command_requires_script.rs index f6f819d8e..447ac3232 100644 --- a/lib/components/fabro-validate/src/rules/command_requires_script.rs +++ b/lib/components/fabro-validate/src/rules/command_requires_script.rs @@ -45,24 +45,15 @@ mod tests { use fabro_graphviz::graph::{AttrValue, Node}; use super::Rule; - use crate::rules::test_support::minimal_graph; + use crate::rules::test_support; use crate::{LintRule, Severity}; - fn node_with(id: &str, attrs: &[(&str, &str)]) -> Node { - let mut node = Node::new(id); - for (key, value) in attrs { - node.attrs - .insert((*key).to_string(), AttrValue::String((*value).to_string())); - } - node - } - #[test] fn errors_on_command_shape_without_script() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "run".to_string(), - node_with("run", &[("shape", "parallelogram"), ("label", "Build")]), + test_support::node_with_attrs("run", &[("shape", "parallelogram"), ("label", "Build")]), ); let d = Rule.apply(&g); @@ -75,9 +66,11 @@ mod tests { #[test] fn errors_on_explicit_command_type_without_script() { - let mut g = minimal_graph(); - g.nodes - .insert("run".to_string(), node_with("run", &[("type", "command")])); + let mut g = test_support::minimal_graph(); + g.nodes.insert( + "run".to_string(), + test_support::node_with_attrs("run", &[("type", "command")]), + ); let d = Rule.apply(&g); @@ -86,29 +79,51 @@ mod tests { } #[test] - fn errors_on_blank_script() { - let mut g = minimal_graph(); + fn errors_on_legacy_tool_type_without_script() { + let mut g = test_support::minimal_graph(); g.nodes.insert( "run".to_string(), - node_with("run", &[("shape", "parallelogram"), ("script", " ")]), + test_support::node_with_attrs("run", &[("type", "tool")]), ); assert_eq!(Rule.apply(&g).len(), 1); } #[test] - fn accepts_command_node_with_script() { - let mut g = minimal_graph(); + fn errors_on_blank_script() { + let mut g = test_support::minimal_graph(); g.nodes.insert( "run".to_string(), - node_with("run", &[ + test_support::node_with_attrs("run", &[("shape", "parallelogram"), ("script", " ")]), + ); + + assert_eq!(Rule.apply(&g).len(), 1); + } + + #[test] + fn errors_on_non_string_script() { + let mut g = test_support::minimal_graph(); + let mut node = Node::new("run"); + node.attrs + .insert("script".to_string(), AttrValue::Integer(123)); + g.nodes.insert("run".to_string(), node); + + assert_eq!(Rule.apply(&g).len(), 1); + } + + #[test] + fn accepts_command_node_with_script() { + let mut g = test_support::minimal_graph(); + g.nodes.insert( + "run".to_string(), + test_support::node_with_attrs("run", &[ ("shape", "parallelogram"), ("script", "cargo build"), ]), ); g.nodes.insert( "build".to_string(), - node_with("build", &[("script", "cargo build")]), + test_support::node_with_attrs("build", &[("script", "cargo build")]), ); assert!(Rule.apply(&g).is_empty()); @@ -116,14 +131,14 @@ mod tests { #[test] fn ignores_non_command_nodes() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "plan".to_string(), - node_with("plan", &[("prompt", "do it")]), + test_support::node_with_attrs("plan", &[("prompt", "do it")]), ); g.nodes.insert( "gate".to_string(), - node_with("gate", &[("shape", "hexagon")]), + test_support::node_with_attrs("gate", &[("shape", "hexagon")]), ); assert!(Rule.apply(&g).is_empty()); diff --git a/lib/components/fabro-validate/src/rules/inert_attribute.rs b/lib/components/fabro-validate/src/rules/inert_attribute.rs index ac2629422..e2278930c 100644 --- a/lib/components/fabro-validate/src/rules/inert_attribute.rs +++ b/lib/components/fabro-validate/src/rules/inert_attribute.rs @@ -1,4 +1,4 @@ -use fabro_graphviz::graph::{self, Graph}; +use fabro_graphviz::graph::{self, Graph, Node}; use crate::{Diagnostic, LintRule, Severity}; @@ -25,8 +25,30 @@ const HANDLER_SPECIFIC_ATTRS: &[(&str, &[&str])] = &[ ("review_target", &["human"]), ]; +const SCRIPT_PROMPT_CONFLICT_RULE: &str = "script_prompt_conflict"; + struct Rule; +fn script_prompt_conflict(node: &Node) -> Diagnostic { + Diagnostic { + rule: SCRIPT_PROMPT_CONFLICT_RULE.to_string(), + severity: Severity::Error, + message: format!( + "Node '{}' sets both 'script' and 'prompt'. No built-in handler reads both: command \ + handlers consume 'script', while LLM handlers consume 'prompt'", + node.id + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( + "Remove whichever attribute is wrong, or split the node into a command node and an \ + agent node" + .to_string(), + ), + ..Diagnostic::default() + } +} + impl LintRule for Rule { fn name(&self) -> &'static str { "inert_attribute" @@ -35,6 +57,12 @@ impl LintRule for Rule { fn apply(&self, graph: &Graph) -> Vec { let mut diagnostics = Vec::new(); for node in graph.nodes.values() { + let has_script_prompt_conflict = + node.attrs.contains_key("script") && node.attrs.contains_key("prompt"); + if has_script_prompt_conflict { + diagnostics.push(script_prompt_conflict(node)); + } + // An unknown shape or type is covered by the type_known rule; a // node this rule cannot classify is skipped rather than guessed at. let Some(handler) = node.handler_type() else { @@ -47,6 +75,9 @@ impl LintRule for Rule { if !node.attrs.contains_key(*attr) { continue; } + if has_script_prompt_conflict && matches!(*attr, "script" | "prompt") { + continue; + } if consumers.contains(&handler) { continue; } @@ -74,24 +105,19 @@ impl LintRule for Rule { #[cfg(test)] mod tests { - use fabro_graphviz::graph::{AttrValue, Node}; + use fabro_graphviz::graph::{AttrValue, Edge, Node}; use super::Rule; - use crate::rules::test_support::minimal_graph; + use crate::rules::test_support; use crate::{LintRule, Severity}; fn node_with_attr(id: &str, shape: &str, attr: &str, value: &str) -> Node { - let mut node = Node::new(id); - node.attrs - .insert("shape".to_string(), AttrValue::String(shape.to_string())); - node.attrs - .insert(attr.to_string(), AttrValue::String(value.to_string())); - node + test_support::node_with_attrs(id, &[("shape", shape), (attr, value)]) } #[test] fn warns_on_script_on_agent_node() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "work".to_string(), node_with_attr("work", "box", "script", "echo hi"), @@ -104,9 +130,77 @@ mod tests { assert_eq!(d[0].node_id.as_deref(), Some("work")); } + #[test] + fn built_in_rules_report_script_prompt_conflict_once() { + let mut g = test_support::minimal_graph(); + g.nodes.insert( + "work".to_string(), + test_support::node_with_attrs("work", &[ + ("script", "cargo build"), + ("prompt", "do it"), + ]), + ); + g.edges = vec![Edge::new("start", "work"), Edge::new("work", "exit")]; + + let diagnostics = crate::validate(&g, &[]); + let work_diagnostics = diagnostics + .iter() + .filter(|diagnostic| diagnostic.node_id.as_deref() == Some("work")) + .collect::>(); + + assert_eq!(work_diagnostics.len(), 1, "diagnostics: {diagnostics:?}"); + assert_eq!(work_diagnostics[0].rule, "script_prompt_conflict"); + assert_eq!(work_diagnostics[0].severity, Severity::Error); + } + + #[test] + fn conflict_error_replaces_inert_warning_for_explicit_shapes() { + let mut g = test_support::minimal_graph(); + g.nodes.insert( + "run".to_string(), + test_support::node_with_attrs("run", &[ + ("shape", "parallelogram"), + ("script", "cargo build"), + ("prompt", "do it"), + ]), + ); + g.nodes.insert( + "plan".to_string(), + test_support::node_with_attrs("plan", &[ + ("shape", "box"), + ("script", "cargo build"), + ("prompt", "do it"), + ]), + ); + + let diagnostics = Rule.apply(&g); + + assert_eq!(diagnostics.len(), 2); + assert!( + diagnostics + .iter() + .all(|diagnostic| diagnostic.rule == "script_prompt_conflict" + && diagnostic.severity == Severity::Error) + ); + } + + #[test] + fn conflict_uses_attribute_presence() { + let mut g = test_support::minimal_graph(); + let mut node = test_support::node_with_attrs("work", &[("prompt", "do it")]); + node.attrs + .insert("script".to_string(), AttrValue::Integer(123)); + g.nodes.insert("work".to_string(), node); + + let diagnostics = Rule.apply(&g); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].rule, "script_prompt_conflict"); + } + #[test] fn warns_on_prompt_on_start_and_command_nodes() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes .get_mut("start") .expect("minimal graph has start") @@ -126,7 +220,7 @@ mod tests { #[test] fn warns_on_duration_on_command_node() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "run".to_string(), node_with_attr("run", "parallelogram", "duration", "30s"), @@ -139,7 +233,7 @@ mod tests { #[test] fn warns_on_parallel_attrs_on_agent_node() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); let mut node = Node::new("work"); node.attrs .insert("max_parallel".to_string(), AttrValue::Integer(4)); @@ -150,7 +244,7 @@ mod tests { #[test] fn warns_on_output_retries_on_command_node() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "run".to_string(), node_with_attr("run", "parallelogram", "output_retries", "2"), @@ -165,7 +259,7 @@ mod tests { #[test] fn accepts_attrs_on_their_own_handler_types() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "run".to_string(), node_with_attr("run", "parallelogram", "script", "echo hi"), @@ -202,12 +296,19 @@ mod tests { "human".to_string(), node_with_attr("human", "hexagon", "review_target", "true"), ); + g.nodes.insert( + "legacy_command".to_string(), + test_support::node_with_attrs("legacy_command", &[ + ("type", "tool"), + ("script", "echo legacy"), + ]), + ); assert!(Rule.apply(&g).is_empty()); } #[test] fn warns_on_review_target_on_non_human_node() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "work".to_string(), node_with_attr("work", "box", "review_target", "true"), @@ -222,7 +323,7 @@ mod tests { #[test] fn accepts_prompt_on_shapeless_node_defaulting_to_agent() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); let mut node = Node::new("work"); node.attrs.insert( "prompt".to_string(), @@ -234,7 +335,7 @@ mod tests { #[test] fn accepts_prompt_on_fan_in_judge() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "merge".to_string(), node_with_attr("merge", "tripleoctagon", "prompt", "pick the best"), @@ -244,7 +345,7 @@ mod tests { #[test] fn ignores_unclassifiable_node_shapes() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); g.nodes.insert( "odd".to_string(), node_with_attr("odd", "doubleoctagon", "script", "echo hi"), @@ -254,7 +355,7 @@ mod tests { #[test] fn ignores_handler_specific_attrs_on_unrecognized_explicit_types() { - let mut g = minimal_graph(); + let mut g = test_support::minimal_graph(); let mut node = Node::new("custom"); node.attrs.insert( "type".to_string(), diff --git a/lib/components/fabro-validate/src/rules/mod.rs b/lib/components/fabro-validate/src/rules/mod.rs index 87d6ee549..5360d95f1 100644 --- a/lib/components/fabro-validate/src/rules/mod.rs +++ b/lib/components/fabro-validate/src/rules/mod.rs @@ -23,7 +23,6 @@ mod reachability; mod reserved_keyword_node_id; mod retry_target_exists; mod script_absolute_cd; -mod script_prompt_conflict; mod selection_valid; mod start_no_incoming; mod start_node; @@ -63,7 +62,6 @@ pub fn built_in_rules() -> Vec> { all_conditional_edges::rule(), orphan_custom_outcome::rule(), script_absolute_cd::rule(), - script_prompt_conflict::rule(), command_requires_script::rule(), import_error::rule(), join_policy_removed::rule(), diff --git a/lib/components/fabro-validate/src/rules/script_prompt_conflict.rs b/lib/components/fabro-validate/src/rules/script_prompt_conflict.rs deleted file mode 100644 index 39f551aa3..000000000 --- a/lib/components/fabro-validate/src/rules/script_prompt_conflict.rs +++ /dev/null @@ -1,135 +0,0 @@ -use fabro_graphviz::graph::Graph; - -use crate::{Diagnostic, LintRule, Severity}; - -pub(super) fn rule() -> Box { - Box::new(Rule) -} - -struct Rule; - -impl LintRule for Rule { - fn name(&self) -> &'static str { - "script_prompt_conflict" - } - - fn apply(&self, graph: &Graph) -> Vec { - let mut diagnostics = Vec::new(); - for node in graph.nodes.values() { - if node.script().is_none() || node.prompt().is_none() { - continue; - } - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!( - "Node '{}' sets both 'script' and 'prompt'. No node type reads both: \ - 'script' selects the command handler and 'prompt' selects an LLM handler", - node.id - ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some( - "Remove whichever attribute is wrong, or split the node into a command node \ - and an agent node" - .to_string(), - ), - ..Diagnostic::default() - }); - } - diagnostics - } -} - -#[cfg(test)] -mod tests { - use fabro_graphviz::graph::{AttrValue, Node}; - - use super::Rule; - use crate::rules::test_support::minimal_graph; - use crate::{LintRule, Severity}; - - fn node_with(id: &str, attrs: &[(&str, &str)]) -> Node { - let mut node = Node::new(id); - for (key, value) in attrs { - node.attrs - .insert((*key).to_string(), AttrValue::String((*value).to_string())); - } - node - } - - #[test] - fn errors_when_shapeless_node_sets_both() { - let mut g = minimal_graph(); - g.nodes.insert( - "work".to_string(), - node_with("work", &[("script", "cargo build"), ("prompt", "do it")]), - ); - - let d = Rule.apply(&g); - - assert_eq!(d.len(), 1); - assert_eq!(d[0].severity, Severity::Error); - assert!(d[0].message.contains("'script'")); - assert!(d[0].message.contains("'prompt'")); - assert_eq!(d[0].node_id.as_deref(), Some("work")); - } - - #[test] - fn errors_even_when_shape_is_explicit() { - // An explicit shape resolves the handler, but the unread attribute is - // still a mistake, and reporting it the same way everywhere keeps - // adding a shape from turning an error into a warning. - let mut g = minimal_graph(); - g.nodes.insert( - "run".to_string(), - node_with("run", &[ - ("shape", "parallelogram"), - ("script", "cargo build"), - ("prompt", "do it"), - ]), - ); - g.nodes.insert( - "plan".to_string(), - node_with("plan", &[ - ("shape", "box"), - ("script", "cargo build"), - ("prompt", "do it"), - ]), - ); - - let d = Rule.apply(&g); - - assert_eq!(d.len(), 2); - assert!(d.iter().all(|d| d.severity == Severity::Error)); - } - - #[test] - fn accepts_either_attribute_alone() { - let mut g = minimal_graph(); - g.nodes.insert( - "build".to_string(), - node_with("build", &[("script", "cargo build")]), - ); - g.nodes.insert( - "plan".to_string(), - node_with("plan", &[("prompt", "do it")]), - ); - - assert!(Rule.apply(&g).is_empty()); - } - - #[test] - fn ignores_legacy_tool_command_attribute() { - let mut g = minimal_graph(); - g.nodes.insert( - "work".to_string(), - node_with("work", &[ - ("tool_command", "cargo build"), - ("prompt", "do it"), - ]), - ); - - assert!(Rule.apply(&g).is_empty()); - } -} diff --git a/lib/components/fabro-validate/src/rules/test_support.rs b/lib/components/fabro-validate/src/rules/test_support.rs index 18182c802..0846b7c13 100644 --- a/lib/components/fabro-validate/src/rules/test_support.rs +++ b/lib/components/fabro-validate/src/rules/test_support.rs @@ -1,5 +1,14 @@ use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; +pub(crate) fn node_with_attrs(id: &str, attrs: &[(&str, &str)]) -> Node { + let mut node = Node::new(id); + for (key, value) in attrs { + node.attrs + .insert((*key).to_string(), AttrValue::String((*value).to_string())); + } + node +} + pub(crate) fn minimal_graph() -> Graph { let mut g = Graph::new("test"); let mut start = Node::new("start"); diff --git a/lib/components/fabro-workflow/src/handler/command.rs b/lib/components/fabro-workflow/src/handler/command.rs index 66542743c..da39a70df 100644 --- a/lib/components/fabro-workflow/src/handler/command.rs +++ b/lib/components/fabro-workflow/src/handler/command.rs @@ -18,6 +18,10 @@ fn timeout_ms(node: &Node) -> Option { node.timeout().map(crate::millis_u64) } +fn non_blank_script(node: &Node) -> Option<&str> { + node.script().filter(|script| !script.trim().is_empty()) +} + /// Executes an external script configured via node attributes. pub struct CommandHandler; @@ -31,7 +35,9 @@ impl Handler for CommandHandler { _run_dir: &Path, _services: &EngineServices, ) -> Result { - let script = node.script().unwrap_or(""); + let Some(script) = non_blank_script(node) else { + return Ok(Outcome::fail_classify("No script specified")); + }; let mut outcome = Outcome::simulated(&node.id); outcome.notes = Some(format!("[Simulated] Command skipped: {script}")); @@ -49,11 +55,9 @@ impl Handler for CommandHandler { run_dir: &Path, services: &EngineServices, ) -> Result { - let script = node.script().unwrap_or(""); - - if script.is_empty() { + let Some(script) = non_blank_script(node) else { return Ok(Outcome::fail_classify("No script specified")); - } + }; let language = node .attrs @@ -392,22 +396,38 @@ mod tests { } #[tokio::test] - async fn script_handler_no_script() { + async fn missing_and_blank_scripts_fail_execution_and_simulation() { let handler = CommandHandler; - let node = Node::new("script_node"); let context = Context::new(); let graph = Graph::new("test"); let run_dir = tempfile::tempdir().unwrap(); - let services = make_services(); - let outcome = handler - .execute(&node, &context, &graph, run_dir.path(), &services) - .await - .unwrap(); - assert_eq!(outcome.status, StageOutcome::Failed { - retry_requested: false, - }); - assert_eq!(outcome.failure_reason(), Some("No script specified")); + + for script in [None, Some(" \t\n")] { + let mut node = Node::new("script_node"); + if let Some(script) = script { + node.attrs + .insert("script".to_string(), AttrValue::String(script.to_string())); + } + + let outcomes = [ + handler + .execute(&node, &context, &graph, run_dir.path(), &services) + .await + .unwrap(), + handler + .simulate(&node, &context, &graph, run_dir.path(), &services) + .await + .unwrap(), + ]; + + for outcome in outcomes { + assert_eq!(outcome.status, StageOutcome::Failed { + retry_requested: false, + }); + assert_eq!(outcome.failure_reason(), Some("No script specified")); + } + } } #[tokio::test] diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index f1f6512f2..34952a58b 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -153,18 +153,17 @@ impl Node { self.str_attr("label").unwrap_or(&self.id) } - /// The node's Graphviz shape, which selects its handler. + /// The node's Graphviz shape, which contributes to handler selection. /// - /// An explicit `shape` attribute always wins. Otherwise the shape is - /// inferred from the attributes the node carries: `script` is read by the - /// command handler and by nothing else, so a shapeless node that sets it - /// is a command node. Everything else falls back to `box` (agent). + /// An explicit `shape` or `type` attribute disables inference. Otherwise, + /// the presence of `script` infers `parallelogram`. Everything else falls + /// back to `box`. #[must_use] pub fn shape(&self) -> &str { if let Some(shape) = self.str_attr("shape") { return shape; } - if self.script().is_some() { + if self.node_type().is_none() && self.attrs.contains_key("script") { return "parallelogram"; } "box" @@ -333,8 +332,10 @@ impl Node { /// mapping. #[must_use] pub fn handler_type(&self) -> Option<&str> { - if let Some(t) = self.node_type() { - return Some(t); + match self.node_type() { + Some("tool") => return Some("command"), + Some(node_type) => return Some(node_type), + None => {} } shape_to_handler_type(self.shape()) } @@ -623,6 +624,7 @@ mod tests { assert_eq!(node.shape(), "box"); assert_eq!(node.node_type(), None); assert_eq!(node.prompt(), None); + assert_eq!(node.script(), None); assert_eq!(node.for_each(), None); assert_eq!(node.output_schema(), None); assert_eq!(node.output_retries(), 2); @@ -678,17 +680,30 @@ mod tests { #[test] fn explicit_type_wins_over_script_inference() { let node = node_with("odd", &[("type", "agent"), ("script", "cargo build")]); - assert_eq!(node.shape(), "parallelogram"); + assert_eq!(node.shape(), "box"); assert_eq!(node.handler_type(), Some("agent")); } #[test] - fn empty_script_still_infers_command() { + fn any_script_attribute_value_infers_command() { // The command-requires-script lint reports this; inference only asks // whether the attribute is present so the diagnostic lands on a // command node rather than a silently-agent one. - let node = node_with("build", &[("script", "")]); - assert_eq!(node.shape(), "parallelogram"); + let empty = node_with("empty", &[("script", "")]); + assert_eq!(empty.shape(), "parallelogram"); + assert_eq!(empty.handler_type(), Some("command")); + + let mut non_string = Node::new("non_string"); + non_string + .attrs + .insert("script".to_string(), AttrValue::Integer(123)); + assert_eq!(non_string.shape(), "parallelogram"); + assert_eq!(non_string.handler_type(), Some("command")); + } + + #[test] + fn legacy_tool_type_resolves_to_command() { + let node = node_with("build", &[("type", "tool")]); assert_eq!(node.handler_type(), Some("command")); }