diff --git a/AGENTS.md b/AGENTS.md index 4ca4fbeb5..9129a7be4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,12 +110,13 @@ Before merging changes that add or move shared test helpers, verify: ## Architecture -Fabro is an AI-powered workflow orchestration platform. Workflows are defined as Graphviz graphs, where each node is a stage (agent, prompt, command, conditional, human, parallel, etc.). Petri, the workflow engine, admits a workflow at create and executes every run; `fabro-petri` is the only crate that imports it. +Fabro is an AI-powered workflow orchestration platform. Workflows are defined as Graphviz graphs, where each node is a stage (agent, prompt, command, conditional, human, parallel, etc.). Petri, the workflow engine, admits a workflow at create and executes every run. Two crates import it: `fabro-petri` (the engine adapters) and `fabro-dot` (Petri's DOT parser, for reading a graph's shape and file references). ### Rust crates (`lib/apps/`, `lib/components/`, and `lib/foundation/`) - **fabro-cli** — CLI entry point. Commands: `run`, `exec`, `serve`, `validate`, `parse`, `cp`, `model`, `doctor`, `install`, `ps`, `system prune` - **fabro-workflow** — Fabro's platform half of a run: creates a run around Petri's admission (the run's display graph is read off the admitted graph), archives, forks and retries runs, and holds the run tools and the pull request pipeline. Compilation and execution are Petri's, through `fabro-petri` -- **fabro-graphviz** — Graphviz DOT parser, the typed graph model, and SVG rendering +- **fabro-dot** — The workflow graph as written, read through Petri's DOT parser: its name, goal, node and edge counts, and the files it references (`import`, `stack.child_workflow`, `@file` prompts, the goal). The bundler and the workflow-version store walk references through it; `fabro-graphviz` re-emits Fabro DOT for Graphviz through it +- **fabro-graphviz** — SVG rendering of workflow graphs through the vendored Graphviz (`graphviz-sys`) - **fabro-sandbox** — Local, Docker, and Daytona sandbox providers. `RunSandbox` is also the `Environment` pebble's coding agent runs its tools through; agent stages, Ask Fabro, hook evaluators, and `fabro exec` all run on the `pebble-coding-agent` crate (pinned by rev in the workspace `Cargo.toml`). `RunSandbox` is also the `Environment` pebble's coding agent runs its tools through; agent stages, Ask Fabro, hook evaluators, and `fabro exec` all run on the `pebble-coding-agent` crate (pinned by rev in the workspace `Cargo.toml`). Docker is the default runtime provider and creates clone-based `/workspace` containers through the operator's Docker daemon; Daytona uses the same GitHub-only clone-source contract. Docker daemon access is host-root-equivalent and assumes trusted callers/payloads. - **fabro-petri** — Fabro's adapters over Petri, the workflow engine: the one crate that imports the Petri packages (pinned by rev in the workspace `Cargo.toml`), holding the run store over SQLite and the platform adapters - **fabro-server** — Axum HTTP server. Routes for runs, sessions, models, completions, usage. SSE event streaming. Demo mode via header @@ -221,12 +222,13 @@ Before merging changes that add or move shared test helpers, verify: ## Architecture -Fabro is an AI-powered workflow orchestration platform. Workflows are defined as Graphviz graphs, where each node is a stage (agent, prompt, command, conditional, human, parallel, etc.). Petri, the workflow engine, admits a workflow at create and executes every run; `fabro-petri` is the only crate that imports it. +Fabro is an AI-powered workflow orchestration platform. Workflows are defined as Graphviz graphs, where each node is a stage (agent, prompt, command, conditional, human, parallel, etc.). Petri, the workflow engine, admits a workflow at create and executes every run. Two crates import it: `fabro-petri` (the engine adapters) and `fabro-dot` (Petri's DOT parser, for reading a graph's shape and file references). ### Rust crates (`lib/apps/`, `lib/components/`, and `lib/foundation/`) - **fabro-cli** — CLI entry point. Commands: `run`, `exec`, `serve`, `validate`, `parse`, `cp`, `model`, `doctor`, `install`, `ps`, `system prune` - **fabro-workflow** — Fabro's platform half of a run: creates a run around Petri's admission (the run's display graph is read off the admitted graph), archives, forks and retries runs, and holds the run tools and the pull request pipeline. Compilation and execution are Petri's, through `fabro-petri` -- **fabro-graphviz** — Graphviz DOT parser, the typed graph model, and SVG rendering +- **fabro-dot** — The workflow graph as written, read through Petri's DOT parser: its name, goal, node and edge counts, and the files it references (`import`, `stack.child_workflow`, `@file` prompts, the goal). The bundler and the workflow-version store walk references through it; `fabro-graphviz` re-emits Fabro DOT for Graphviz through it +- **fabro-graphviz** — SVG rendering of workflow graphs through the vendored Graphviz (`graphviz-sys`) - **fabro-sandbox** — Local, Docker, and Daytona sandbox providers. `RunSandbox` is also the `Environment` pebble's coding agent runs its tools through; agent stages, Ask Fabro, hook evaluators, and `fabro exec` all run on the `pebble-coding-agent` crate (pinned by rev in the workspace `Cargo.toml`). `RunSandbox` is also the `Environment` pebble's coding agent runs its tools through; agent stages, Ask Fabro, hook evaluators, and `fabro exec` all run on the `pebble-coding-agent` crate (pinned by rev in the workspace `Cargo.toml`). Docker is the default runtime provider and creates clone-based `/workspace` containers through the operator's Docker daemon; Daytona uses the same GitHub-only clone-source contract. Docker daemon access is host-root-equivalent and assumes trusted callers/payloads. - **fabro-petri** — Fabro's adapters over Petri, the workflow engine: the one crate that imports the Petri packages (pinned by rev in the workspace `Cargo.toml`), holding the run store over SQLite and the platform adapters - **fabro-server** — Axum HTTP server. Routes for runs, sessions, models, completions, usage. SSE event streaming. Demo mode via header diff --git a/Cargo.lock b/Cargo.lock index 187d2e5d1..390e6aa99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2257,6 +2257,18 @@ dependencies = [ "walkdir", ] +[[package]] +name = "fabro-dot" +version = "0.361.0-nightly.0" +dependencies = [ + "fabro-template", + "fabro-types", + "insta", + "petri-frontend-attractor", + "thiserror 2.0.18", + "walkdir", +] + [[package]] name = "fabro-dump" version = "0.361.0-nightly.0" @@ -2323,13 +2335,9 @@ name = "fabro-graphviz" version = "0.361.0-nightly.0" dependencies = [ "anyhow", - "fabro-types", + "fabro-dot", "graphviz-sys", - "nom", "regex", - "serde", - "serde_json", - "thiserror 2.0.18", ] [[package]] @@ -2415,8 +2423,8 @@ dependencies = [ "async-trait", "fabro-api", "fabro-config", + "fabro-dot", "fabro-github", - "fabro-graphviz", "fabro-template", "fabro-test", "fabro-tool", @@ -2631,6 +2639,7 @@ dependencies = [ "fabro-client", "fabro-config", "fabro-db", + "fabro-dot", "fabro-environment", "fabro-github", "fabro-graphviz", @@ -2965,7 +2974,6 @@ dependencies = [ "fabro-dump", "fabro-environment", "fabro-github", - "fabro-graphviz", "fabro-http", "fabro-llm", "fabro-macros", @@ -3001,7 +3009,7 @@ name = "fabro-workflow-version" version = "0.361.0-nightly.0" dependencies = [ "fabro-config", - "fabro-graphviz", + "fabro-dot", "fabro-store", "fabro-template", "fabro-types", @@ -4496,12 +4504,6 @@ dependencies = [ "once_cell", ] -[[package]] -name = "minimal-lexical" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a" - [[package]] name = "miniz_oxide" version = "0.8.9" @@ -4578,16 +4580,6 @@ dependencies = [ "libc", ] -[[package]] -name = "nom" -version = "7.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d273983c5a657a70a3e8f2a01329822f3b8c8172b73826411a55751e404a0a4a" -dependencies = [ - "memchr", - "minimal-lexical", -] - [[package]] name = "normalize-line-endings" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index 3d4d0d3c9..03a1548a4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -112,8 +112,9 @@ pebble-cli-core = { git = "https://github.com/lithoscomputer/pebble", rev = "67c # petri: the workflow engine Fabro runs its workflows on. Pinned by rev, the # same way pebble and sandbox-driver are. Petri pins the same pebble, # lithos-llm and sandbox-driver revisions as this file, so the workspace links -# one copy of each. Only `fabro-petri` may depend on these packages; the keys -# carry the `petri_` prefix so the crate names say where they come from. +# one copy of each. Only `fabro-petri` and `fabro-dot` (the DOT parser alone) +# may depend on these packages; the keys carry the `petri_` prefix so the crate +# names say where they come from. petri_runtime = { git = "https://github.com/lithoscomputer/petri.git", rev = "13e1044e9796101f2a97b4dd2f2ee81e96941939", package = "petri-runtime" } petri_execution = { git = "https://github.com/lithoscomputer/petri.git", rev = "13e1044e9796101f2a97b4dd2f2ee81e96941939", package = "petri-execution" } petri_store = { git = "https://github.com/lithoscomputer/petri.git", rev = "13e1044e9796101f2a97b4dd2f2ee81e96941939", package = "petri-store" } diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 31f08b74b..f841559c5 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -20,7 +20,7 @@ use chrono::{DateTime, Duration as ChronoDuration, Utc}; use fabro_client::{AuthEntry, AuthStore, DevTokenEntry, OAuthEntry, StoredSubject}; use fabro_test::{fabro_json_snapshot, fabro_snapshot, test_context}; use fabro_types::settings::run::{McpServerSettings, McpTransport}; -use fabro_types::{Graph, RunId, WorkflowSettings, test_support}; +use fabro_types::{RunGraph, RunId, WorkflowSettings, test_support}; use httpmock::Method::{GET, POST}; use httpmock::MockServer; @@ -2208,7 +2208,7 @@ async fn mcp_events_decodes_run_created_with_model_keyed_fallbacks() { "kind": "run.created", "spec": { "settings": settings, - "graph": Graph::new("Remote Workflow"), + "graph": RunGraph::new("Remote Workflow"), "labels": {}, "source_directory": "/srv/repo", "provenance": test_support::test_run_provenance() diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 8bf2f2700..64de2b6f4 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -352,7 +352,7 @@ fn edge_only_node() { exit_code: 1 ----- stdout ----- ----- stderr ----- - Workflow: EdgeOnlyNode (2 nodes, 2 edges) + Workflow: EdgeOnlyNode (3 nodes, 2 edges) Graph: [FIXTURES]/edge_only_node.fabro error: [FIXTURES]/edge_only_node.fabro:8:14: `misspelled_node` is named by an edge but never declared (attractor.undeclared_node) × Validation failed diff --git a/lib/apps/fabro-server/Cargo.toml b/lib/apps/fabro-server/Cargo.toml index ab7294453..2dee109a4 100644 --- a/lib/apps/fabro-server/Cargo.toml +++ b/lib/apps/fabro-server/Cargo.toml @@ -27,6 +27,7 @@ fabro-install = { path = "../../components/fabro-install" } fabro-spa = { path = "../fabro-spa" } fabro-config = { path = "../../foundation/fabro-config" } fabro-environment.workspace = true +fabro-dot = { path = "../../components/fabro-dot" } fabro-graphviz = { path = "../../components/fabro-graphviz" } fabro-interview = { path = "../../components/fabro-interview" } fabro-slack = { path = "../../components/fabro-slack" } diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 9dbcb490a..67f17aa35 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -113,7 +113,13 @@ pub fn validate_collected_workflow( }; let working_directory = project::resolve_working_directory_from_run(&settings.run, Path::new("/workspace")); - let shape = workflow_shape_of(&check, &workflow.source, &settings, &working_directory); + let shape = workflow_shape_of( + &check, + &lowered.entrypoint, + &workflow.source, + &settings, + &working_directory, + ); Ok(types::ValidateResponse { ok: !check.has_errors(), workflow: run_manifest::workflow_summary(&check, &shape, lowered.entrypoint.as_path()), diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index fefe049b6..887d659e5 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -12,9 +12,8 @@ use fabro_config::{ CliLayer, CliOutputLayer, EnvironmentLayer, MergeMap, RunLayer, SettingsLayer, WorkflowSettingsBuilder, parse_input_overrides, parse_labels, project, }; +use fabro_dot::WorkflowGraph; use fabro_github::token_source::{InstallationTokenSource, ResolvedToken, TokenSnapshot}; -use fabro_graphviz::graph::AttrValue; -use fabro_graphviz::parser; use fabro_graphviz::render::apply_direction; use fabro_petri::check::Launch; use fabro_petri::run_graph; @@ -1321,6 +1320,7 @@ pub(crate) struct WorkflowShape { pub(crate) fn workflow_shape(check: &ManifestCheck, prepared: &PreparedManifest) -> WorkflowShape { workflow_shape_of( check, + &prepared.target_path, &prepared.root_source, &prepared.settings, &prepared.source_directory, @@ -1329,13 +1329,14 @@ pub(crate) fn workflow_shape(check: &ManifestCheck, prepared: &PreparedManifest) pub(crate) fn workflow_shape_of( check: &ManifestCheck, + graph_path: &ManifestPath, root_source: &str, settings: &WorkflowSettings, working_directory: &Path, ) -> WorkflowShape { let mut shape = check.graph.as_ref().map_or_else( || { - parser::parse(root_source).map_or_else( + WorkflowGraph::parse(&graph_path.to_string(), root_source).map_or_else( |_| WorkflowShape { name: String::new(), nodes: 0, @@ -1343,15 +1344,10 @@ pub(crate) fn workflow_shape_of( goal: String::new(), }, |graph| WorkflowShape { - goal: graph - .attrs - .get("goal") - .and_then(AttrValue::as_str) - .unwrap_or_default() - .to_string(), - nodes: graph.nodes.len(), - edges: graph.edges.len(), - name: graph.name, + goal: graph.goal().unwrap_or_default().to_string(), + nodes: graph.node_count(), + edges: graph.edge_count(), + name: graph.name().to_string(), }, ) }, diff --git a/lib/components/fabro-dot/Cargo.toml b/lib/components/fabro-dot/Cargo.toml new file mode 100644 index 000000000..d19e6b81b --- /dev/null +++ b/lib/components/fabro-dot/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "fabro-dot" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "The workflow graph as written, read through Petri's DOT parser: its shape and the files it references" + +[lib] +doctest = false + +[lints] +workspace = true + +[dependencies] +fabro-template = { path = "../../foundation/fabro-template" } +fabro-types = { path = "../../foundation/fabro-types" } +petri_frontend_attractor.workspace = true +thiserror.workspace = true + +[dev-dependencies] +insta.workspace = true +walkdir.workspace = true diff --git a/lib/components/fabro-dot/src/graphviz.rs b/lib/components/fabro-dot/src/graphviz.rs new file mode 100644 index 000000000..f16236747 --- /dev/null +++ b/lib/components/fabro-dot/src/graphviz.rs @@ -0,0 +1,253 @@ +//! Fabro DOT re-emitted as DOT Graphviz accepts. +//! +//! Graphviz rejects unquoted dotted attribute keys such as `acp.command`, +//! which Fabro's language allows. The render paths parse the source with +//! Petri's parser and print it back with every id quoted that needs quoting. + +use std::borrow::Cow; + +use petri_frontend_attractor::dot::{ + self, AstValue, Attr, AttrBlock, DotGraph, EdgeStmt, NodeStmt, Statement, SubgraphStmt, +}; + +/// Convert Fabro DOT into DOT Graphviz accepts. +/// +/// If the source is outside the subset Petri parses, it is returned unchanged +/// so Graphviz can judge it itself: it may be valid Graphviz that is not a +/// workflow. +#[must_use] +pub fn normalize_for_graphviz(source: &str) -> Cow<'_, str> { + match dot::parse("workflow.fabro", source) { + Ok(graph) => Cow::Owned(emit_graph(&graph)), + Err(_) => Cow::Borrowed(source), + } +} + +fn emit_graph(graph: &DotGraph) -> String { + let mut out = String::new(); + out.push_str("digraph"); + if !graph.name.name.is_empty() { + out.push(' '); + out.push_str(&dot_id(&graph.name.name)); + } + out.push_str(" {\n"); + emit_statements(&mut out, &graph.statements, 1); + out.push_str("}\n"); + out +} + +fn emit_statements(out: &mut String, statements: &[Statement], indent: usize) { + for statement in statements { + emit_statement(out, statement, indent); + } +} + +fn emit_statement(out: &mut String, statement: &Statement, indent: usize) { + match statement { + Statement::GraphAttrs(attrs) => emit_defaults(out, "graph", attrs, indent), + Statement::NodeDefaults(attrs) => emit_defaults(out, "node", attrs, indent), + Statement::EdgeDefaults(attrs) => emit_defaults(out, "edge", attrs, indent), + Statement::Subgraph(subgraph) => emit_subgraph(out, subgraph, indent), + Statement::Node(node) => emit_node(out, node, indent), + Statement::Edge(edge) => emit_edge(out, edge, indent), + Statement::GraphAttr(attr) => { + push_indent(out, indent); + emit_attr(out, attr); + out.push_str(";\n"); + } + } +} + +fn emit_defaults(out: &mut String, keyword: &str, attrs: &AttrBlock, indent: usize) { + push_indent(out, indent); + out.push_str(keyword); + out.push(' '); + emit_attr_block(out, attrs); + out.push_str(";\n"); +} + +fn emit_subgraph(out: &mut String, subgraph: &SubgraphStmt, indent: usize) { + push_indent(out, indent); + out.push_str("subgraph"); + if let Some(name) = &subgraph.name { + out.push(' '); + out.push_str(&dot_id(&name.name)); + } + out.push_str(" {\n"); + emit_statements(out, &subgraph.statements, indent + 1); + push_indent(out, indent); + out.push_str("}\n"); +} + +fn emit_node(out: &mut String, node: &NodeStmt, indent: usize) { + push_indent(out, indent); + out.push_str(&dot_id(&node.id.name)); + if let Some(attrs) = &node.attrs { + out.push(' '); + emit_attr_block(out, attrs); + } + out.push_str(";\n"); +} + +fn emit_edge(out: &mut String, edge: &EdgeStmt, indent: usize) { + push_indent(out, indent); + let mut nodes = edge.nodes.iter(); + if let Some(first) = nodes.next() { + out.push_str(&dot_id(&first.name)); + for node in nodes { + out.push_str(" -> "); + out.push_str(&dot_id(&node.name)); + } + } + if let Some(attrs) = &edge.attrs { + out.push(' '); + emit_attr_block(out, attrs); + } + out.push_str(";\n"); +} + +fn emit_attr_block(out: &mut String, attrs: &AttrBlock) { + out.push('['); + for (index, attr) in attrs.iter().enumerate() { + if index > 0 { + out.push_str(", "); + } + emit_attr(out, attr); + } + out.push(']'); +} + +fn emit_attr(out: &mut String, attr: &Attr) { + out.push_str(&dot_id(&attr.key)); + out.push('='); + out.push_str(&dot_value(&attr.value)); +} + +fn dot_value(value: &AstValue) -> String { + match value { + AstValue::Str(value) => quoted_dot_string(value), + AstValue::Int(value) => value.to_string(), + AstValue::Float(value) => value.to_string(), + AstValue::Bool(value) => value.to_string(), + AstValue::Ident(value) => dot_id(value), + } +} + +fn dot_id(value: &str) -> String { + if is_plain_dot_id(value) && !is_dot_keyword(value) { + value.to_string() + } else { + quoted_dot_string(value) + } +} + +fn quoted_dot_string(value: &str) -> String { + let mut out = String::with_capacity(value.len() + 2); + out.push('"'); + for ch in value.chars() { + match ch { + '\\' => out.push_str("\\\\"), + '"' => out.push_str("\\\""), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + _ => out.push(ch), + } + } + out.push('"'); + out +} + +fn is_plain_dot_id(value: &str) -> bool { + let mut chars = value.chars(); + let Some(first) = chars.next() else { + return false; + }; + if !(first.is_ascii_alphabetic() || first == '_') { + return false; + } + chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_') +} + +fn is_dot_keyword(value: &str) -> bool { + matches!( + value.to_ascii_lowercase().as_str(), + "digraph" | "edge" | "graph" | "node" | "strict" | "subgraph" + ) +} + +fn push_indent(out: &mut String, indent: usize) { + for _ in 0..indent { + out.push_str(" "); + } +} + +#[cfg(test)] +mod tests { + use super::normalize_for_graphviz; + + #[test] + fn quotes_dotted_attribute_keys() { + let source = r#"digraph X { + a [label="A", acp.command="codex"] + }"#; + + let normalized = normalize_for_graphviz(source); + + assert!(normalized.contains(r#""acp.command"="codex""#)); + } + + #[test] + fn quotes_known_fabro_dotted_attribute_keys() { + let source = r#"digraph X { + approve [human.default_choice="deploy"] + child [stack.child_workflow="child.fabro", manager.max_cycles=50] + approve -> child + }"#; + + let normalized = normalize_for_graphviz(source); + + assert!(normalized.contains(r#""human.default_choice"="deploy""#)); + assert!(normalized.contains(r#""stack.child_workflow"="child.fabro""#)); + assert!(normalized.contains(r#""manager.max_cycles"=50"#)); + } + + #[test] + fn preserves_subgraphs_defaults_and_bare_graph_attributes() { + let source = r##"digraph X { + rankdir=LR + node [color="#357f9e"] + subgraph cluster_loop { + label="Loop" + a [acp.command="codex"] + } + }"##; + + let normalized = normalize_for_graphviz(source); + + assert!(normalized.contains("rankdir=LR;")); + assert!(normalized.contains("node [")); + assert!(normalized.contains("subgraph cluster_loop")); + assert!(normalized.contains(r#""acp.command"="codex""#)); + } + + #[test] + fn quotes_ids_that_collide_with_keywords_or_need_escaping() { + let source = r#"digraph X { + "my node" [label="say \"hi\""] + "my node" -> Node + }"#; + + let normalized = normalize_for_graphviz(source); + + assert!(normalized.contains(r#""my node" [label="say \"hi\""]"#)); + assert!(normalized.contains(r#""my node" -> "Node""#)); + } + + #[test] + fn source_outside_the_subset_passes_through_unchanged() { + let source = "graph G { a -- b }"; + + assert_eq!(normalize_for_graphviz(source), source); + } +} diff --git a/lib/components/fabro-dot/src/lib.rs b/lib/components/fabro-dot/src/lib.rs new file mode 100644 index 000000000..f5e9aea31 --- /dev/null +++ b/lib/components/fabro-dot/src/lib.rs @@ -0,0 +1,290 @@ +//! The workflow graph as written, read through Petri's DOT parser. +//! +//! Petri admits and runs every workflow. Fabro's platform reads the DOT only +//! to learn a workflow's shape (its name, goal, node and edge counts) and the +//! files it names through a fixed attribute vocabulary: `import`, +//! `stack.child_workflow`, and `@`-prefixed `prompt`, `output_schema` and +//! `goal` values. The bundler also scans the inline templates for includes: a +//! non-`@` goal or prompt, and the entrypoint's `model_stylesheet`. +//! +//! File references are static. They may not contain template syntax, because +//! they resolve before any template renders. [`WorkflowGraph::references`] is +//! the one walker over that vocabulary: the manifest bundler and the +//! workflow-version store both consume it, so a new reference-bearing +//! attribute is added here once. +//! +//! This crate and `fabro-petri` are the two places Fabro imports Petri. It +//! stays small so the bundler and the version store read a graph without +//! pulling the engine in, and so `fabro-graphviz` can re-emit Fabro DOT for +//! Graphviz without a parser of its own. + +mod graphviz; + +use std::fmt; + +use fabro_template::{StaticReferenceError, validate_static_reference}; +use fabro_types::ReferenceKind; +pub use graphviz::normalize_for_graphviz; +use petri_frontend_attractor::dot; +use petri_frontend_attractor::model::{self, AttrValue, NodeDecl, Workflow}; + +/// A DOT text Petri's parser refused: the first problem it found, with the +/// position Petri reported. +#[derive(Clone, Debug, PartialEq, Eq, thiserror::Error)] +pub struct ParseError { + /// Petri's diagnostic code, such as `dot.syntax` or + /// `unsupported.dot.html_string`. + pub code: String, + pub message: String, + /// What to write instead, when Petri offers one. + pub hint: Option, + /// The file the text was parsed as. + pub file: String, + /// One-based; zero when the problem has no position. + pub line: u32, + /// One-based; zero when the problem has no position. + pub column: u32, +} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.file)?; + if self.line > 0 { + write!(f, ":{}:{}", self.line, self.column)?; + } + write!(f, ": {}", self.message)?; + if let Some(hint) = &self.hint { + write!(f, " ({hint})")?; + } + Ok(()) + } +} + +/// Whether the walked graph is the workflow's entrypoint or was reached +/// through an `import` or `stack.child_workflow` reference. +/// +/// Position-dependent reference semantics (today: `model_stylesheet` is a +/// template root only on the entrypoint, because an imported stylesheet is +/// ignored at run time) live in the walker, so every consumer applies the +/// same rule. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum GraphPosition { + Entrypoint, + Imported, +} + +/// What one reference in a workflow graph points at. +/// +/// `@` prefixes are already stripped from file references. Inline variants +/// carry template content the consumer feeds to template-dependency +/// discovery. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum GraphReferenceKind<'graph> { + /// `graph [goal="@"]`. + GoalFile { reference: &'graph str }, + /// A non-`@` graph `goal`: inline template content. + GoalInline { content: &'graph str }, + /// The entrypoint graph's inline `model_stylesheet` template content. + ModelStylesheetInline { content: &'graph str }, + /// `node [import=""]`: another graph file to walk. + Import { reference: &'graph str }, + /// `node [stack.child_workflow=""]`. + ChildWorkflow { reference: &'graph str }, + /// `node [="@"]` for the file-inlined attributes + /// `prompt` and `output_schema`. + FileInline { + key: &'graph str, + reference: &'graph str, + }, + /// A non-`@` node prompt: inline template content. + InlinePrompt { content: &'graph str }, +} + +impl<'graph> GraphReferenceKind<'graph> { + /// The file this reference names and the kind it is validated as; + /// `None` for inline template content. + #[must_use] + pub fn file_reference(&self) -> Option<(&'graph str, ReferenceKind)> { + match *self { + Self::GoalFile { reference } => Some((reference, ReferenceKind::GraphGoalFile)), + Self::Import { reference } => Some((reference, ReferenceKind::Import)), + Self::ChildWorkflow { reference } => Some((reference, ReferenceKind::ChildWorkflow)), + Self::FileInline { reference, .. } => Some((reference, ReferenceKind::FileInline)), + Self::GoalInline { .. } + | Self::ModelStylesheetInline { .. } + | Self::InlinePrompt { .. } => None, + } + } +} + +/// One file reference or inline template found in a workflow graph, with +/// where it was written. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct GraphReference<'graph> { + pub kind: GraphReferenceKind<'graph>, + /// The node the reference sits on; `None` for a graph attribute. + pub node: Option<&'graph str>, + /// The position of the attribute key, one-based. + pub line: u32, + pub column: u32, +} + +/// A parsed workflow graph: Petri's semantic model of the DOT, with node and +/// edge defaults applied, subgraphs flattened and edge chains expanded. +#[derive(Clone, Debug, PartialEq)] +pub struct WorkflowGraph { + workflow: Workflow, +} + +impl WorkflowGraph { + /// Parse `text` as the workflow file `file`, which positions and the + /// error name. + pub fn parse(file: &str, text: &str) -> Result { + let dot = dot::parse(file, text).map_err(|diagnostic| ParseError { + code: diagnostic.code.to_string(), + message: diagnostic.message, + hint: diagnostic.hint, + file: diagnostic.span.file.to_string(), + line: diagnostic.span.line, + column: diagnostic.span.column, + })?; + Ok(Self { + workflow: model::build(&dot), + }) + } + + /// The `digraph` name; empty when the graph has none. + #[must_use] + pub fn name(&self) -> &str { + &self.workflow.name + } + + /// The graph `goal` attribute as written, `@` prefix included, when it + /// is a string. + #[must_use] + pub fn goal(&self) -> Option<&str> { + self.graph_str("goal") + } + + /// The graph `model_stylesheet` attribute, when it is a string. + #[must_use] + pub fn model_stylesheet(&self) -> Option<&str> { + self.graph_str("model_stylesheet") + } + + /// Every node, declared or named only by an edge. + #[must_use] + pub fn node_count(&self) -> usize { + self.workflow.nodes.len() + } + + /// Every edge, with chains expanded. + #[must_use] + pub fn edge_count(&self) -> usize { + self.workflow.edges.len() + } + + fn graph_str(&self, key: &str) -> Option<&str> { + self.workflow + .attrs + .get(key) + .and_then(|attr| attr.value.as_str()) + } + + /// Every static file reference and inline template in this graph, in + /// declaration order, each file reference checked to be template-free. + /// + /// The walk covers this graph alone; recursing into `Import` targets and + /// resolving references against a file source are the consumer's job. + /// `position` says whether this graph is the workflow entrypoint, which + /// gates the position-dependent references. + pub fn references( + &self, + position: GraphPosition, + ) -> Result>, StaticReferenceError> { + let mut found = Vec::new(); + + if let Some(goal) = self.workflow.attrs.get("goal") { + if let Some(goal_text) = goal.value.as_str().filter(|goal| !goal.is_empty()) { + let kind = if let Some(reference) = goal_text.strip_prefix('@') { + validate_static_reference(reference, ReferenceKind::GraphGoalFile)?; + GraphReferenceKind::GoalFile { reference } + } else { + GraphReferenceKind::GoalInline { content: goal_text } + }; + found.push(GraphReference { + kind, + node: None, + line: goal.span.line, + column: goal.span.column, + }); + } + } + + if position == GraphPosition::Entrypoint { + if let Some(stylesheet) = self.workflow.attrs.get("model_stylesheet") { + if let Some(content) = stylesheet.value.as_str().filter(|css| !css.is_empty()) { + found.push(GraphReference { + kind: GraphReferenceKind::ModelStylesheetInline { content }, + node: None, + line: stylesheet.span.line, + column: stylesheet.span.column, + }); + } + } + } + + for node in &self.workflow.nodes { + node_references(node, &mut found)?; + } + Ok(found) + } +} + +fn node_references<'graph>( + node: &'graph NodeDecl, + found: &mut Vec>, +) -> Result<(), StaticReferenceError> { + for (key, attr) in node.attrs.iter() { + let AttrValue::Str(value) = &attr.value else { + continue; + }; + let kind = match key { + "import" => GraphReferenceKind::Import { reference: value }, + "stack.child_workflow" => GraphReferenceKind::ChildWorkflow { reference: value }, + "prompt" | "output_schema" => match value.strip_prefix('@') { + Some(reference) => GraphReferenceKind::FileInline { key, reference }, + None => continue, + }, + _ => continue, + }; + if let Some((reference, reference_kind)) = kind.file_reference() { + validate_static_reference(reference, reference_kind)?; + } + found.push(GraphReference { + kind, + node: Some(&node.id), + line: attr.span.line, + column: attr.span.column, + }); + } + + if let Some(prompt) = node.attrs.get("prompt") { + if let Some(content) = prompt + .value + .as_str() + .filter(|prompt| !prompt.starts_with('@')) + { + found.push(GraphReference { + kind: GraphReferenceKind::InlinePrompt { content }, + node: Some(&node.id), + line: prompt.span.line, + column: prompt.span.column, + }); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests; diff --git a/lib/components/fabro-dot/src/snapshots/fabro_dot__tests__checked_in_workflows_keep_their_shape_and_references.snap b/lib/components/fabro-dot/src/snapshots/fabro_dot__tests__checked_in_workflows_keep_their_shape_and_references.snap new file mode 100644 index 000000000..34947fd28 --- /dev/null +++ b/lib/components/fabro-dot/src/snapshots/fabro_dot__tests__checked_in_workflows_keep_their_shape_and_references.snap @@ -0,0 +1,132 @@ +--- +source: lib/components/fabro-dot/src/tests.rs +expression: report +--- +.fabro/workflows/card-game/workflow.fabro: CardGame nodes=20 edges=31 + goal-inline + inline@expand_spec + inline@impl_setup + inline@verify_setup + inline@impl_data + inline@verify_data + inline@impl_logic + inline@verify_logic + inline@impl_ui + inline@verify_ui + inline@impl_integration + inline@verify_integration + inline@review +.fabro/workflows/card-game-fast/workflow.fabro: CardGameFast nodes=6 edges=7 + goal-inline + inline@plan_app + inline@implement_app + inline@verify_app + inline@fix_app +.fabro/workflows/code-review/code-review.fabro: CodeReview nodes=26 edges=33 + goal-inline + stylesheet-inline + file:output_schema:schemas/file-groups.schema.json@grouping + file:prompt:prompts/group-files.md.j2@grouping + file:output_schema:schemas/findings.schema.json@finder + file:prompt:prompts/finder.md.j2@finder + file:output_schema:schemas/verdict.schema.json@verifier + file:prompt:prompts/verify.md.j2@verifier + file:output_schema:schemas/findings.schema.json@sweeper + file:prompt:prompts/sweep.md.j2@sweeper + file:output_schema:schemas/verdict.schema.json@sweep_verifier + file:prompt:prompts/verify.md.j2@sweep_verifier +.fabro/workflows/context-demo/workflow.fabro: ContextDemo nodes=3 edges=2 + goal-inline + inline@emit +.fabro/workflows/daytona-medium/workflow.fabro: DaytonaMedium nodes=3 edges=2 + goal-inline +.fabro/workflows/gh-list/workflow.fabro: GhList nodes=4 edges=3 + goal-inline +.fabro/workflows/gh-triage/workflow.fabro: GhTriage nodes=3 edges=2 + goal-inline + inline@triage +.fabro/workflows/goal/workflow.fabro: Goal nodes=6 edges=8 + goal-inline + file:prompt:prompts/continue.md@work + file:prompt:prompts/audit.md@audit + inline@fixup +.fabro/workflows/hello/workflow.fabro: Hello nodes=3 edges=2 + goal-inline + inline@greet +.fabro/workflows/implement-issue/workflow.fabro: ImplementIssue nodes=4 edges=3 + goal-inline + stylesheet-inline + inline@plan + child:fabro/workflows/implement-plan/workflow.fabro@implement +.fabro/workflows/implement-plan/workflow.fabro: ImplementPlan nodes=11 edges=14 + goal-inline + inline@fix_lints + inline@implement + file:prompt:prompts/simplify.md@simplify_opus + file:prompt:prompts/simplify.md@simplify_sol + inline@fixup +.fabro/workflows/interview/workflow.fabro: Interview nodes=8 edges=15 + goal-inline + inline@summarize +.fabro/workflows/patch-cves/workflow.fabro: PatchCves nodes=3 edges=2 + goal-inline + stylesheet-inline + file:prompt:prompts/patch-cves.md@patch +.fabro/workflows/pr-simplify/workflow.fabro: PrSimplify nodes=3 edges=2 + goal-inline + file:prompt:prompts/simplify.md@simplify +.fabro/workflows/sleeper/workflow.fabro: Sleeper nodes=3 edges=2 + goal-inline + inline@nap +.fabro/workflows/smoke/workflow.fabro: Smoke nodes=8 edges=7 + goal-inline +.fabro/workflows/solitaire/workflow.fabro: Solitaire nodes=20 edges=31 + goal-inline + inline@expand_spec + inline@impl_setup + inline@verify_setup + inline@impl_data + inline@verify_data + inline@impl_logic + inline@verify_logic + inline@impl_ui + inline@verify_ui + inline@impl_integration + inline@verify_integration + inline@review +.fabro/workflows/solitaire-fast/workflow.fabro: SolitaireFast nodes=6 edges=7 + goal-inline + inline@plan_app + inline@implement_app + inline@verify_app + inline@fix_app +test/dot-compatibility/acp-agent-chain.fabro: AcpAgentChain nodes=4 edges=3 + goal-inline +test/dot-compatibility/human-default-choice.fabro: HumanDefaultChoice nodes=5 edges=5 + goal-inline + inline@revise +test/dot-compatibility/subworkflow-manager.fabro: SubworkflowManager nodes=5 edges=4 + goal-inline + inline@plan + child:implement-and-test.fabro@impl + inline@review +lib/apps/fabro-cli/tests/it/workflow/fixtures/agent_linear.fabro: AgentLinear nodes=3 edges=2 + goal-inline + inline@work +lib/apps/fabro-cli/tests/it/workflow/fixtures/command_agent_mixed.fabro: CommandAgentMixed nodes=5 edges=4 + goal-inline + inline@work +lib/apps/fabro-cli/tests/it/workflow/fixtures/command_pipeline.fabro: CommandPipeline nodes=4 edges=3 + goal-inline +lib/apps/fabro-cli/tests/it/workflow/fixtures/command_routing.fabro: CommandRouting nodes=6 edges=6 + goal-inline +lib/apps/fabro-cli/tests/it/workflow/fixtures/conditional_branching.fabro: ConditionalBranching nodes=6 edges=6 + goal-inline +lib/apps/fabro-cli/tests/it/workflow/fixtures/full_stack.fabro: FullStack nodes=7 edges=7 + goal-inline + inline@plan + inline@impl +lib/apps/fabro-cli/tests/it/workflow/fixtures/human_gate.fabro: HumanGate nodes=6 edges=6 + goal-inline + inline@draft + inline@revise diff --git a/lib/components/fabro-dot/src/tests.rs b/lib/components/fabro-dot/src/tests.rs new file mode 100644 index 000000000..caa3ffb37 --- /dev/null +++ b/lib/components/fabro-dot/src/tests.rs @@ -0,0 +1,257 @@ +use std::fmt::Write as _; +use std::path::{Path, PathBuf}; + +use super::{GraphPosition, GraphReference, GraphReferenceKind, WorkflowGraph}; + +fn parse(text: &str) -> WorkflowGraph { + WorkflowGraph::parse("workflow.fabro", text).unwrap_or_else(|error| panic!("{error}")) +} + +fn describe(reference: &GraphReference<'_>) -> String { + let where_ = reference + .node + .map_or_else(String::new, |node| format!("@{node}")); + let what = match reference.kind { + GraphReferenceKind::GoalFile { reference } => format!("goal-file:{reference}"), + GraphReferenceKind::GoalInline { content } => format!("goal-inline:{content}"), + GraphReferenceKind::ModelStylesheetInline { content } => { + format!("stylesheet-inline:{content}") + } + GraphReferenceKind::Import { reference } => format!("import:{reference}"), + GraphReferenceKind::ChildWorkflow { reference } => format!("child:{reference}"), + GraphReferenceKind::FileInline { key, reference } => format!("file:{key}:{reference}"), + GraphReferenceKind::InlinePrompt { content } => format!("inline:{content}"), + }; + format!("{what}{where_}") +} + +fn describe_all(graph: &WorkflowGraph, position: GraphPosition) -> Vec { + graph + .references(position) + .unwrap_or_else(|error| panic!("{error}")) + .iter() + .map(describe) + .collect() +} + +#[test] +fn reads_the_shape_with_defaults_applied_and_chains_expanded() { + let graph = parse( + r#"digraph Branch { + graph [goal="Implement and validate a feature"] + rankdir=LR + node [shape=box, timeout="900s"] + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + plan [label="Plan", prompt="Plan the implementation"] + implement [label="Implement", prompt="Implement the plan"] + validate [label="Validate", prompt="Run tests"] + gate [shape=diamond, label="Tests passing?"] + + start -> plan -> implement -> validate -> gate + gate -> exit [label="Yes", condition="outcome=succeeded"] + gate -> implement [label="No", condition="outcome!=succeeded"] + }"#, + ); + + assert_eq!(graph.name(), "Branch"); + assert_eq!(graph.goal(), Some("Implement and validate a feature")); + assert_eq!(graph.model_stylesheet(), None); + assert_eq!(graph.node_count(), 6); + assert_eq!(graph.edge_count(), 6); +} + +#[test] +fn parse_errors_carry_petri_code_and_position() { + let error = WorkflowGraph::parse("flows/bad.fabro", "digraph A { } extra stuff").unwrap_err(); + + assert_eq!(error.code, "dot.syntax"); + assert_eq!(error.file, "flows/bad.fabro"); + assert_eq!((error.line, error.column), (1, 15)); + assert_eq!( + error.to_string(), + "flows/bad.fabro:1:15: unexpected `extra` after the graph" + ); + + let error = WorkflowGraph::parse("w.fabro", "not a graph").unwrap_err(); + assert_eq!(error.code, "dot.syntax"); +} + +#[test] +fn visits_every_reference_kind_once_in_declaration_order() { + let graph = parse( + r#"digraph Refs { + graph [goal="@goal.md", model_stylesheet="{% include 'styles.partial' %}"] + imported [import="graphs/child.fabro"] + child [stack.child_workflow="children/check.fabro"] + file_prompt [prompt="@prompts/task.md", output_schema="@schemas/out.json"] + inline [prompt="Do the {{ thing }}"] + keyword [output_schema="routing"] + }"#, + ); + + assert_eq!(describe_all(&graph, GraphPosition::Entrypoint), [ + "goal-file:goal.md", + "stylesheet-inline:{% include 'styles.partial' %}", + "import:graphs/child.fabro@imported", + "child:children/check.fabro@child", + "file:output_schema:schemas/out.json@file_prompt", + "file:prompt:prompts/task.md@file_prompt", + "inline:Do the {{ thing }}@inline", + ]); +} + +#[test] +fn references_carry_the_attribute_position() { + let graph = parse("digraph P {\n a [label=\"A\",\n prompt=\"@task.md\"]\n}"); + + let references = graph.references(GraphPosition::Entrypoint).unwrap(); + assert_eq!(references.len(), 1); + assert_eq!((references[0].line, references[0].column), (3, 6)); + assert_eq!(references[0].node, Some("a")); +} + +#[test] +fn node_defaults_reach_every_node_declared_under_them() { + let graph = parse( + r#"digraph Defaults { + node [prompt="@shared.md"] + a + b [prompt="own prompt"] + subgraph cluster_x { + node [output_schema="@x.json"] + c + } + d + }"#, + ); + + assert_eq!(describe_all(&graph, GraphPosition::Entrypoint), [ + "file:prompt:shared.md@a", + "inline:own prompt@b", + "file:output_schema:x.json@c", + "file:prompt:shared.md@c", + "file:prompt:shared.md@d", + ]); +} + +#[test] +fn imported_graphs_do_not_emit_model_stylesheet() { + let graph = parse( + r#"digraph Imported { + graph [model_stylesheet="* { reasoning_effort: low; }"] + }"#, + ); + + assert!(describe_all(&graph, GraphPosition::Imported).is_empty()); + assert_eq!(describe_all(&graph, GraphPosition::Entrypoint), [ + "stylesheet-inline:* { reasoning_effort: low; }" + ]); +} + +#[test] +fn empty_goal_and_non_string_attributes_are_not_references() { + let graph = parse( + r#"digraph Quiet { + graph [goal=""] + a [prompt=5, import=true] + }"#, + ); + + assert!(describe_all(&graph, GraphPosition::Entrypoint).is_empty()); +} + +#[test] +fn rejects_template_syntax_in_references_before_visiting() { + for (source, kind) in [ + ( + r#"digraph T { imported [import="graphs/{{ name }}.fabro"] }"#, + "import reference", + ), + ( + r#"digraph T { child [stack.child_workflow="{{ inputs.child }}"] }"#, + "child workflow reference", + ), + ( + r#"digraph T { work [prompt="@prompts/{{ lang }}.md"] }"#, + "file inline reference", + ), + ( + r#"digraph T { graph [goal="@{{ goal_file }}"] }"#, + "graph goal file reference", + ), + ] { + let error = parse(source) + .references(GraphPosition::Entrypoint) + .expect_err("template syntax in a file reference must be refused"); + assert_eq!(error.kind().to_string(), kind, "source: {source}"); + } +} + +fn repository_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../..") + .canonicalize() + .expect("the repository root should resolve") +} + +fn checked_in_workflows(root: &Path) -> Vec { + let mut files = Vec::new(); + for directory in [ + ".fabro/workflows", + "test/dot-compatibility", + "lib/apps/fabro-cli/tests/it/workflow/fixtures", + ] { + for entry in walkdir::WalkDir::new(root.join(directory)).sort_by_file_name() { + let entry = entry.expect("workflow directory entries should be readable"); + if entry.path().extension().and_then(|ext| ext.to_str()) == Some("fabro") { + files.push(entry.into_path()); + } + } + } + files +} + +/// The walker's output over every checked-in workflow bundle. A change here +/// means the bundler will see a different version closure for one of Fabro's +/// own workflows: read the diff as that. +#[expect( + clippy::disallowed_methods, + reason = "unit test reads the checked-in workflow bundles synchronously" +)] +#[test] +fn checked_in_workflows_keep_their_shape_and_references() { + let root = repository_root(); + let mut report = String::new(); + for path in checked_in_workflows(&root) { + let relative = path + .strip_prefix(&root) + .expect("workflow paths sit under the repository root"); + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|error| panic!("failed to read {}: {error}", path.display())); + let graph = WorkflowGraph::parse(&relative.display().to_string(), &text) + .unwrap_or_else(|error| panic!("{error}")); + writeln!( + report, + "{}: {} nodes={} edges={}", + relative.display(), + graph.name(), + graph.node_count(), + graph.edge_count() + ) + .expect("writing to a String cannot fail"); + for reference in graph.references(GraphPosition::Entrypoint).unwrap() { + let description = match reference.kind { + GraphReferenceKind::GoalInline { .. } => "goal-inline".to_string(), + GraphReferenceKind::ModelStylesheetInline { .. } => "stylesheet-inline".to_string(), + GraphReferenceKind::InlinePrompt { .. } => { + format!("inline@{}", reference.node.unwrap_or_default()) + } + _ => describe(&reference), + }; + writeln!(report, " {description}").expect("writing to a String cannot fail"); + } + } + insta::assert_snapshot!(report); +} diff --git a/lib/components/fabro-graphviz/Cargo.toml b/lib/components/fabro-graphviz/Cargo.toml index 83faa3c05..f3f053b4c 100644 --- a/lib/components/fabro-graphviz/Cargo.toml +++ b/lib/components/fabro-graphviz/Cargo.toml @@ -4,7 +4,7 @@ edition.workspace = true version.workspace = true publish = false license.workspace = true -description = "Graphviz DOT parser and typed graph data model" +description = "SVG rendering of workflow graphs through the vendored Graphviz" [lib] doctest = false @@ -14,12 +14,6 @@ workspace = true [dependencies] anyhow.workspace = true +fabro-dot = { path = "../fabro-dot" } graphviz-sys.workspace = true -fabro-types = { path = "../../foundation/fabro-types" } -nom = "7" regex = { workspace = true } -serde = { workspace = true } -thiserror = { workspace = true } - -[dev-dependencies] -serde_json = { workspace = true } diff --git a/lib/components/fabro-graphviz/src/error.rs b/lib/components/fabro-graphviz/src/error.rs deleted file mode 100644 index 9950b7738..000000000 --- a/lib/components/fabro-graphviz/src/error.rs +++ /dev/null @@ -1,9 +0,0 @@ -use thiserror::Error as ThisError; - -#[derive(Debug, ThisError)] -pub enum Error { - #[error("Parse error: {0}")] - Parse(String), -} - -pub type Result = std::result::Result; diff --git a/lib/components/fabro-graphviz/src/graph/mod.rs b/lib/components/fabro-graphviz/src/graph/mod.rs deleted file mode 100644 index 26535e727..000000000 --- a/lib/components/fabro-graphviz/src/graph/mod.rs +++ /dev/null @@ -1,3 +0,0 @@ -pub mod types; - -pub use types::*; diff --git a/lib/components/fabro-graphviz/src/graph/types.rs b/lib/components/fabro-graphviz/src/graph/types.rs deleted file mode 100644 index 46adafde8..000000000 --- a/lib/components/fabro-graphviz/src/graph/types.rs +++ /dev/null @@ -1 +0,0 @@ -pub use fabro_types::graph::*; diff --git a/lib/components/fabro-graphviz/src/lib.rs b/lib/components/fabro-graphviz/src/lib.rs index e9b997618..cf25e6cd6 100644 --- a/lib/components/fabro-graphviz/src/lib.rs +++ b/lib/components/fabro-graphviz/src/lib.rs @@ -1,6 +1 @@ -pub mod error; -pub mod graph; -pub mod parser; pub mod render; - -pub use error::{Error, Result}; diff --git a/lib/components/fabro-graphviz/src/parser/ast.rs b/lib/components/fabro-graphviz/src/parser/ast.rs deleted file mode 100644 index 8aa50e449..000000000 --- a/lib/components/fabro-graphviz/src/parser/ast.rs +++ /dev/null @@ -1,122 +0,0 @@ -use serde::{Deserialize, Serialize}; - -/// A parsed DOT value before semantic interpretation. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub enum AstValue { - Str(String), - Int(i64), - Float(f64), - Bool(bool), - /// A bare identifier used as a value (e.g., shape names, direction - /// keywords). - Ident(String), -} - -/// A list of key-value attribute pairs from an attribute block `[k=v, ...]`. -pub type AttrBlock = Vec<(String, AstValue)>; - -/// A node statement: `id [attrs]?`. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct NodeStmt { - pub id: String, - pub attrs: Option, -} - -/// An edge statement: `A -> B -> C [attrs]?`. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct EdgeStmt { - /// Chain of node IDs (at least 2). - pub nodes: Vec, - pub attrs: Option, -} - -/// A subgraph statement: `subgraph name? { stmts }`. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct SubgraphStmt { - pub name: Option, - pub statements: Vec, -} - -/// A single statement in a DOT graph body. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub enum Statement { - /// `graph [attrs]` - GraphAttr(AttrBlock), - /// `node [attrs]` - NodeDefaults(AttrBlock), - /// `edge [attrs]` - EdgeDefaults(AttrBlock), - /// `subgraph name? { ... }` - Subgraph(SubgraphStmt), - /// `id [attrs]?` - Node(NodeStmt), - /// `A -> B -> C [attrs]?` - Edge(EdgeStmt), - /// Top-level `key = value` - GraphAttrDecl(String, AstValue), -} - -/// The top-level parsed DOT graph. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct DotGraph { - pub name: String, - pub statements: Vec, -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn ast_value_variants() { - let s = AstValue::Str("hello".into()); - let i = AstValue::Int(42); - let f = AstValue::Float(3.15); - let b = AstValue::Bool(true); - let id = AstValue::Ident("LR".into()); - - assert_eq!(s, AstValue::Str("hello".into())); - assert_eq!(i, AstValue::Int(42)); - assert_eq!(f, AstValue::Float(3.15)); - assert_eq!(b, AstValue::Bool(true)); - assert_eq!(id, AstValue::Ident("LR".into())); - } - - #[test] - fn dot_graph_construction() { - let graph = DotGraph { - name: "test".into(), - statements: vec![ - Statement::GraphAttrDecl("rankdir".into(), AstValue::Ident("LR".into())), - Statement::Node(NodeStmt { - id: "start".into(), - attrs: Some(vec![("shape".into(), AstValue::Ident("Mdiamond".into()))]), - }), - ], - }; - assert_eq!(graph.name, "test"); - assert_eq!(graph.statements.len(), 2); - } - - #[test] - fn edge_stmt_chained() { - let edge = EdgeStmt { - nodes: vec!["A".into(), "B".into(), "C".into()], - attrs: Some(vec![("label".into(), AstValue::Str("next".into()))]), - }; - assert_eq!(edge.nodes.len(), 3); - } - - #[test] - fn subgraph_stmt() { - let sub = SubgraphStmt { - name: Some("cluster_loop".into()), - statements: vec![Statement::NodeDefaults(vec![( - "timeout".into(), - AstValue::Str("900s".into()), - )])], - }; - assert_eq!(sub.name.as_deref(), Some("cluster_loop")); - assert_eq!(sub.statements.len(), 1); - } -} diff --git a/lib/components/fabro-graphviz/src/parser/grammar.rs b/lib/components/fabro-graphviz/src/parser/grammar.rs deleted file mode 100644 index d205bc6c2..000000000 --- a/lib/components/fabro-graphviz/src/parser/grammar.rs +++ /dev/null @@ -1,445 +0,0 @@ -use nom::IResult; -use nom::branch::alt; -use nom::bytes::complete::tag; -use nom::character::complete::{char, multispace0, one_of}; -use nom::combinator::opt; -use nom::error::{Error, ParseError}; -use nom::multi::many0; -use nom::sequence::{delimited, preceded, terminated, tuple}; - -use crate::parser::ast::{ - AstValue, AttrBlock, DotGraph, EdgeStmt, NodeStmt, Statement, SubgraphStmt, -}; -use crate::parser::lexer::combinators::{identifier, key, value, ws, ws_tag}; - -/// Parse a single attribute: `key = value`. -fn attr(input: &str) -> IResult<&str, (String, AstValue)> { - let (rest, (k, _, _, v)) = - tuple((preceded(ws, key), ws, char('='), preceded(ws, value)))(input)?; - Ok((rest, (k, v))) -} - -/// Parse an attribute block: `[ attr (sep? attr)* ]` where `sep` is `,` or `;`. -/// -/// Per the DOT spec, the separator between attributes is optional — whitespace -/// (including newlines) alone is enough. This accepts comma-separated, -/// semicolon-separated, and newline-separated attribute lists interchangeably. -fn attr_block(input: &str) -> IResult<&str, AttrBlock> { - delimited( - preceded(ws, char('[')), - many0(terminated(attr, opt(preceded(ws, one_of(",;"))))), - preceded(ws, char(']')), - )(input) -} - -/// Parse optional semicolon. -fn opt_semi(input: &str) -> IResult<&str, Option> { - preceded(ws, opt(char(';')))(input) -} - -/// Parse a graph attr statement: `graph [attrs] ;?` -fn graph_attr_stmt(input: &str) -> IResult<&str, Statement> { - let (rest, (_, attrs, _)) = tuple((ws_tag("graph"), attr_block, opt_semi))(input)?; - Ok((rest, Statement::GraphAttr(attrs))) -} - -/// Parse node defaults: `node [attrs] ;?` -fn node_defaults(input: &str) -> IResult<&str, Statement> { - let (rest, (_, attrs, _)) = tuple((ws_tag("node"), attr_block, opt_semi))(input)?; - Ok((rest, Statement::NodeDefaults(attrs))) -} - -/// Parse edge defaults: `edge [attrs] ;?` -fn edge_defaults(input: &str) -> IResult<&str, Statement> { - let (rest, (_, attrs, _)) = tuple((ws_tag("edge"), attr_block, opt_semi))(input)?; - Ok((rest, Statement::EdgeDefaults(attrs))) -} - -/// Parse a graph attr declaration: `identifier = value ;?` -fn graph_attr_decl(input: &str) -> IResult<&str, Statement> { - let (rest, (k, _, _, v, _)) = tuple(( - preceded(ws, identifier), - ws, - char('='), - preceded(ws, value), - opt_semi, - ))(input)?; - Ok((rest, Statement::GraphAttrDecl(k.to_string(), v))) -} - -/// Parse a subgraph: `subgraph name? { statement* }` -fn subgraph_stmt(input: &str) -> IResult<&str, Statement> { - let (rest, _) = ws_tag("subgraph")(input)?; - let (rest, name) = opt(preceded(ws, identifier))(rest)?; - let (rest, _) = preceded(ws, char('{'))(rest)?; - let (rest, stmts) = many0(statement)(rest)?; - let (rest, _) = preceded(ws, char('}'))(rest)?; - Ok(( - rest, - Statement::Subgraph(SubgraphStmt { - name: name.map(String::from), - statements: stmts, - }), - )) -} - -/// Parse an edge or node statement. -/// If an identifier is followed by `->`, parse as edge; otherwise as node. -fn node_or_edge_stmt(input: &str) -> IResult<&str, Statement> { - let (rest, first_id) = preceded(ws, identifier)(input)?; - - // Try to parse as edge: first_id (-> id)+ [attrs]? ;? - if let Ok((rest2, _)) = arrow::>(rest) { - let (rest2, second_id) = preceded(ws, identifier)(rest2)?; - let mut nodes = vec![first_id.to_string(), second_id.to_string()]; - let mut remaining = rest2; - while let Ok((r, _)) = arrow::>(remaining) { - let (r, next_id) = preceded(ws, identifier)(r)?; - nodes.push(next_id.to_string()); - remaining = r; - } - let (remaining, attrs) = opt(attr_block)(remaining)?; - let (remaining, _) = opt_semi(remaining)?; - return Ok((remaining, Statement::Edge(EdgeStmt { nodes, attrs }))); - } - - // Parse as node: first_id [attrs]? ;? - let (rest, attrs) = opt(attr_block)(rest)?; - let (rest, _) = opt_semi(rest)?; - Ok(( - rest, - Statement::Node(NodeStmt { - id: first_id.to_string(), - attrs, - }), - )) -} - -/// Parse a single statement. -fn statement(input: &str) -> IResult<&str, Statement> { - preceded( - ws, - alt(( - graph_attr_stmt, - node_defaults, - edge_defaults, - subgraph_stmt, - // graph_attr_decl must be tried before node_or_edge because both start with an - // identifier. graph_attr_decl is `id = value` while node is `id [attrs]?` - // We try graph_attr_decl first; if it fails (no `=` after id) we fall through to - // node_or_edge. - graph_attr_decl, - node_or_edge_stmt, - )), - )(input) -} - -/// Parse a complete DOT graph: `digraph name { statement* }`. -/// -/// # Errors -/// -/// Returns a nom error if the input does not match the DOT grammar. -pub fn parse_dot_graph(input: &str) -> IResult<&str, DotGraph> { - let (rest, _) = ws_tag("digraph")(input)?; - let (rest, name) = preceded(ws, identifier)(rest)?; - let (rest, _) = preceded(ws, char('{'))(rest)?; - let (rest, stmts) = many0(statement)(rest)?; - let (rest, _) = preceded(ws, char('}'))(rest)?; - Ok((rest, DotGraph { - name: name.to_string(), - statements: stmts, - })) -} - -// We need arrow to work with explicit error types -fn arrow<'a, E: ParseError<&'a str>>(input: &'a str) -> IResult<&'a str, &'a str, E> { - preceded(multispace0, tag("->"))(input) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::parser::ast::AstValue; - - #[test] - fn parse_single_attr() { - let (rest, (k, v)) = attr(" label = \"Hello\"").unwrap(); - assert_eq!(k, "label"); - assert_eq!(v, AstValue::Str("Hello".into())); - assert_eq!(rest, ""); - } - - #[test] - fn parse_attr_block_empty() { - let (rest, attrs) = attr_block("[]").unwrap(); - assert!(attrs.is_empty()); - assert_eq!(rest, ""); - } - - #[test] - fn parse_attr_block_single() { - let (rest, attrs) = attr_block("[label=\"Hello\"]").unwrap(); - assert_eq!(attrs.len(), 1); - assert_eq!(attrs[0].0, "label"); - assert_eq!(rest, ""); - } - - #[test] - fn parse_attr_block_multiple() { - let (rest, attrs) = attr_block("[shape=Mdiamond, label=\"Start\"]").unwrap(); - assert_eq!(attrs.len(), 2); - assert_eq!(attrs[0].0, "shape"); - assert_eq!(attrs[0].1, AstValue::Ident("Mdiamond".into())); - assert_eq!(attrs[1].0, "label"); - assert_eq!(attrs[1].1, AstValue::Str("Start".into())); - assert_eq!(rest, ""); - } - - // Regression test for https://github.com/fabro-sh/fabro/issues/179. - // Standard DOT allows newline (or any whitespace) as an attribute separator - // inside `[ ... ]`, with commas optional. The multi-line, comma-less form is - // what most DOT editors and formatters produce for long attribute lists. - #[test] - fn parse_attr_block_multiline_without_commas() { - let input = "[\n label=\"Inspect Code\"\n shape=tab\n \ - prompt=\"@prompts/inspect.md\"\n class=\"heavy\"\n \ - reasoning_effort=\"high\"\n]"; - let (rest, attrs) = attr_block(input).unwrap(); - assert_eq!(attrs.len(), 5); - assert_eq!(attrs[0].0, "label"); - assert_eq!(attrs[0].1, AstValue::Str("Inspect Code".into())); - assert_eq!(attrs[1].0, "shape"); - assert_eq!(attrs[1].1, AstValue::Ident("tab".into())); - assert_eq!(attrs[2].0, "prompt"); - assert_eq!(attrs[2].1, AstValue::Str("@prompts/inspect.md".into())); - assert_eq!(attrs[3].0, "class"); - assert_eq!(attrs[3].1, AstValue::Str("heavy".into())); - assert_eq!(attrs[4].0, "reasoning_effort"); - assert_eq!(attrs[4].1, AstValue::Str("high".into())); - assert_eq!(rest, ""); - } - - #[test] - fn parse_graph_attr_stmt() { - let (_, stmt) = graph_attr_stmt("graph [goal=\"Run tests\"]").unwrap(); - match stmt { - Statement::GraphAttr(attrs) => { - assert_eq!(attrs.len(), 1); - assert_eq!(attrs[0].0, "goal"); - } - _ => panic!("expected GraphAttr"), - } - } - - #[test] - fn parse_node_defaults_stmt() { - let (_, stmt) = node_defaults("node [shape=box, timeout=\"900s\"]").unwrap(); - assert!(matches!(stmt, Statement::NodeDefaults(_))); - } - - #[test] - fn parse_edge_defaults_stmt() { - let (_, stmt) = edge_defaults("edge [weight=0]").unwrap(); - assert!(matches!(stmt, Statement::EdgeDefaults(_))); - } - - #[test] - fn parse_graph_attr_decl_stmt() { - let (_, stmt) = graph_attr_decl("rankdir=LR").unwrap(); - match stmt { - Statement::GraphAttrDecl(k, v) => { - assert_eq!(k, "rankdir"); - assert_eq!(v, AstValue::Ident("LR".into())); - } - _ => panic!("expected GraphAttrDecl"), - } - } - - #[test] - fn parse_node_stmt_simple() { - let (_, stmt) = node_or_edge_stmt("start [shape=Mdiamond, label=\"Start\"]").unwrap(); - match stmt { - Statement::Node(n) => { - assert_eq!(n.id, "start"); - assert!(n.attrs.is_some()); - } - _ => panic!("expected Node"), - } - } - - #[test] - fn parse_node_stmt_no_attrs() { - let (_, stmt) = node_or_edge_stmt("run_tests ;").unwrap(); - match stmt { - Statement::Node(n) => { - assert_eq!(n.id, "run_tests"); - assert!(n.attrs.is_none()); - } - _ => panic!("expected Node"), - } - } - - #[test] - fn parse_node_stmt_empty_attrs() { - let (_, stmt) = node_or_edge_stmt("consolidate_dod []").unwrap(); - match stmt { - Statement::Node(n) => { - assert_eq!(n.id, "consolidate_dod"); - assert_eq!(n.attrs.as_ref().unwrap().len(), 0); - } - _ => panic!("expected Node"), - } - } - - #[test] - fn parse_edge_stmt_simple() { - let (_, stmt) = node_or_edge_stmt("start -> run_tests").unwrap(); - match stmt { - Statement::Edge(e) => { - assert_eq!(e.nodes, vec!["start", "run_tests"]); - assert!(e.attrs.is_none()); - } - _ => panic!("expected Edge"), - } - } - - #[test] - fn parse_edge_stmt_chained() { - let (_, stmt) = node_or_edge_stmt("start -> run_tests -> report -> exit").unwrap(); - match stmt { - Statement::Edge(e) => { - assert_eq!(e.nodes, vec!["start", "run_tests", "report", "exit"]); - } - _ => panic!("expected Edge"), - } - } - - #[test] - fn parse_edge_stmt_with_attrs() { - let (_, stmt) = - node_or_edge_stmt("gate -> exit [label=\"Yes\", condition=\"outcome=succeeded\"]") - .unwrap(); - match stmt { - Statement::Edge(e) => { - assert_eq!(e.nodes, vec!["gate", "exit"]); - let attrs = e.attrs.unwrap(); - assert_eq!(attrs.len(), 2); - } - _ => panic!("expected Edge"), - } - } - - #[test] - fn parse_subgraph() { - let input = r#"subgraph cluster_loop { - label = "Loop A" - node [thread_id="loop-a"] - Plan [label="Plan next step"] - }"#; - let (_, stmt) = subgraph_stmt(input).unwrap(); - match stmt { - Statement::Subgraph(s) => { - assert_eq!(s.name.as_deref(), Some("cluster_loop")); - assert_eq!(s.statements.len(), 3); - } - _ => panic!("expected Subgraph"), - } - } - - #[test] - fn parse_full_simple_graph() { - let input = r#"digraph Simple { - graph [goal="Run tests and report"] - rankdir=LR - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - - run_tests [label="Run Tests", prompt="Run the test suite and report results"] - report [label="Report", prompt="Summarize the test results"] - - start -> run_tests -> report -> exit - }"#; - let (_, graph) = parse_dot_graph(input).unwrap(); - assert_eq!(graph.name, "Simple"); - assert_eq!(graph.statements.len(), 7); - } - - #[test] - fn parse_full_branching_graph() { - let input = r#"digraph Branch { - graph [goal="Implement and validate a feature"] - rankdir=LR - node [shape=box, timeout="900s"] - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - plan [label="Plan", prompt="Plan the implementation"] - implement [label="Implement", prompt="Implement the plan"] - validate [label="Validate", prompt="Run tests"] - gate [shape=diamond, label="Tests passing?"] - - start -> plan -> implement -> validate -> gate - gate -> exit [label="Yes", condition="outcome=succeeded"] - gate -> implement [label="No", condition="outcome!=succeeded"] - }"#; - let (_, graph) = parse_dot_graph(input).unwrap(); - assert_eq!(graph.name, "Branch"); - // graph [goal=...], rankdir=LR, node [defaults], 6 nodes, 1 chain + 2 edges = - // 12 - assert!(graph.statements.len() >= 11); - } - - #[test] - fn parse_human_gate_graph() { - let input = r#"digraph Review { - rankdir=LR - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - - review_gate [ - shape=hexagon, - label="Review Changes", - type="human" - ] - - start -> review_gate - review_gate -> ship_it [label="[A] Approve"] - review_gate -> fixes [label="[F] Fix"] - ship_it -> exit - fixes -> review_gate - }"#; - let (_, graph) = parse_dot_graph(input).unwrap(); - assert_eq!(graph.name, "Review"); - } - - #[test] - fn parse_qualified_key_attr() { - let (rest, (k, v)) = attr(" tool_hooks.pre = \"echo hello\"").unwrap(); - assert_eq!(k, "tool_hooks.pre"); - assert_eq!(v, AstValue::Str("echo hello".into())); - assert_eq!(rest, ""); - } - - #[test] - fn parse_duration_attr() { - let (_, (k, v)) = attr(" timeout = 900s").unwrap(); - assert_eq!(k, "timeout"); - assert_eq!(v, AstValue::Str("900s".into())); - } - - #[test] - fn parse_boolean_attr() { - let (_, (k, v)) = attr(" goal_gate = true").unwrap(); - assert_eq!(k, "goal_gate"); - assert_eq!(v, AstValue::Bool(true)); - } - - #[test] - fn parse_integer_attr() { - let (_, (k, v)) = attr(" max_retries = 3").unwrap(); - assert_eq!(k, "max_retries"); - assert_eq!(v, AstValue::Int(3)); - } -} diff --git a/lib/components/fabro-graphviz/src/parser/lexer.rs b/lib/components/fabro-graphviz/src/parser/lexer.rs deleted file mode 100644 index b121e8eab..000000000 --- a/lib/components/fabro-graphviz/src/parser/lexer.rs +++ /dev/null @@ -1,427 +0,0 @@ -/// Strip `//` line comments and `/* */` block comments from DOT source. -#[must_use] -pub fn strip_comments(input: &str) -> String { - let mut result = String::with_capacity(input.len()); - let chars: Vec = input.chars().collect(); - let len = chars.len(); - let mut i = 0; - - while i < len { - if i + 1 < len && chars[i] == '/' && chars[i + 1] == '/' { - // Line comment: skip to end of line - i += 2; - while i < len && chars[i] != '\n' { - i += 1; - } - } else if i + 1 < len && chars[i] == '/' && chars[i + 1] == '*' { - // Block comment: skip to closing */ - i += 2; - while i + 1 < len && !(chars[i] == '*' && chars[i + 1] == '/') { - if chars[i] == '\n' { - result.push('\n'); - } - i += 1; - } - if i + 1 < len { - i += 2; // skip */ - } - } else if chars[i] == '"' { - // Quoted string: pass through without stripping - result.push(chars[i]); - i += 1; - while i < len && chars[i] != '"' { - result.push(chars[i]); - if chars[i] == '\\' && i + 1 < len { - i += 1; - result.push(chars[i]); - } - i += 1; - } - if i < len { - result.push(chars[i]); // closing quote - i += 1; - } - } else { - result.push(chars[i]); - i += 1; - } - } - - result -} - -/// nom combinators for whitespace and common tokens. -pub mod combinators { - use nom::branch::alt; - use nom::bytes::complete::{tag, take_while, take_while1}; - use nom::character::complete::{char, multispace0}; - use nom::combinator::{map, opt, recognize}; - use nom::error::{Error, ErrorKind}; - use nom::sequence::{delimited, pair, preceded}; - use nom::{Err, IResult}; - - use crate::parser::ast::AstValue; - - /// Parse optional whitespace (including newlines). - pub fn ws(input: &str) -> IResult<&str, &str> { - multispace0(input) - } - - /// Parse a token surrounded by optional whitespace. - pub fn ws_tag<'a>(t: &'a str) -> impl Fn(&'a str) -> IResult<&'a str, &'a str> { - move |input| delimited(ws, tag(t), ws)(input) - } - - /// Parse an identifier: `[A-Za-z_][A-Za-z0-9_]*`. - pub fn identifier(input: &str) -> IResult<&str, &str> { - recognize(pair( - take_while1(|c: char| c.is_ascii_alphabetic() || c == '_'), - take_while(|c: char| c.is_ascii_alphanumeric() || c == '_'), - ))(input) - } - - /// Parse a qualified ID: `identifier(.identifier)+`. - pub fn qualified_id(input: &str) -> IResult<&str, String> { - let (rest, first) = identifier(input)?; - let mut result = first.to_string(); - let mut remaining = rest; - let mut found_dot = false; - while let Ok((r, _)) = char::<&str, Error<&str>>('.')(remaining) { - if let Ok((r2, segment)) = identifier(r) { - result.push('.'); - result.push_str(segment); - remaining = r2; - found_dot = true; - } else { - break; - } - } - if found_dot { - Ok((remaining, result)) - } else { - Err(Err::Error(Error::new(input, ErrorKind::Tag))) - } - } - - /// Parse a key: either a qualified ID or a simple identifier. - pub fn key(input: &str) -> IResult<&str, String> { - alt((qualified_id, map(identifier, String::from)))(input) - } - - /// Parse a double-quoted string with escape handling. - pub fn quoted_string(input: &str) -> IResult<&str, String> { - let (input, _) = char('"')(input)?; - let mut result = String::new(); - let mut chars = input.chars(); - let mut consumed = 0; - - loop { - match chars.next() { - Some('"') => { - consumed += 1; - return Ok((&input[consumed..], result)); - } - Some('\\') => { - consumed += 1; - match chars.next() { - Some('"') => { - result.push('"'); - consumed += 1; - } - Some('n') => { - result.push('\n'); - consumed += 1; - } - Some('t') => { - result.push('\t'); - consumed += 1; - } - Some('\\') => { - result.push('\\'); - consumed += 1; - } - Some(c) => { - result.push('\\'); - result.push(c); - consumed += c.len_utf8(); - } - None => { - return Err(Err::Error(Error::new(input, ErrorKind::Char))); - } - } - } - Some(c) => { - result.push(c); - consumed += c.len_utf8(); - } - None => { - return Err(Err::Error(Error::new(input, ErrorKind::Char))); - } - } - } - } - - /// Parse a boolean: `true` or `false`. - pub fn boolean(input: &str) -> IResult<&str, bool> { - let (rest, word) = identifier(input)?; - match word { - "true" => Ok((rest, true)), - "false" => Ok((rest, false)), - _ => Err(Err::Error(Error::new(input, ErrorKind::Tag))), - } - } - - /// Parse a float: optional sign, optional integer part, `.`, fractional - /// digits. - pub fn float_value(input: &str) -> IResult<&str, f64> { - let (rest, raw) = recognize(pair( - pair(opt(char('-')), take_while(|c: char| c.is_ascii_digit())), - pair(char('.'), take_while1(|c: char| c.is_ascii_digit())), - ))(input)?; - let val: f64 = raw - .parse() - .map_err(|_| Err::Error(Error::new(input, ErrorKind::Float)))?; - Ok((rest, val)) - } - - /// Parse an integer: optional sign, digits. Not followed by `.` (that's a - /// float). - pub fn integer_value(input: &str) -> IResult<&str, i64> { - let (rest, raw) = recognize(pair( - opt(char('-')), - take_while1(|c: char| c.is_ascii_digit()), - ))(input)?; - if rest.starts_with('.') { - return Err(Err::Error(Error::new(input, ErrorKind::Digit))); - } - let val: i64 = raw - .parse() - .map_err(|_| Err::Error(Error::new(input, ErrorKind::Digit)))?; - Ok((rest, val)) - } - - /// Parse a duration: integer followed by unit suffix (ms, s, m, h, d). - pub fn duration_value(input: &str) -> IResult<&str, AstValue> { - let (rest, num) = recognize(pair( - opt(char('-')), - take_while1(|c: char| c.is_ascii_digit()), - ))(input)?; - let (rest, unit) = alt((tag("ms"), tag("s"), tag("m"), tag("h"), tag("d")))(rest)?; - if rest - .chars() - .next() - .is_some_and(|c| c.is_ascii_alphanumeric()) - { - return Err(Err::Error(Error::new(input, ErrorKind::Tag))); - } - Ok((rest, AstValue::Str(format!("{num}{unit}")))) - } - - /// Parse a bare string value containing hyphens and dots (e.g., - /// `gpt-5.2-codex`). - /// - /// Must start with an alpha/underscore character, then may continue with - /// alphanumeric, underscore, hyphen, or dot characters. Must contain at - /// least one hyphen or dot (otherwise `identifier` handles it). - pub fn bare_string(input: &str) -> IResult<&str, String> { - let (rest, raw) = recognize(pair( - take_while1(|c: char| c.is_ascii_alphabetic() || c == '_'), - take_while(|c: char| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.'), - ))(input)?; - if !raw.contains('-') && !raw.contains('.') { - return Err(Err::Error(Error::new(input, ErrorKind::Verify))); - } - Ok((rest, raw.to_string())) - } - - /// Parse an AST value: duration, float, integer, boolean, quoted string, - /// bare identifier, or bare string (e.g., `gpt-5.2-codex`). - pub fn value(input: &str) -> IResult<&str, AstValue> { - let input = input.trim_start(); - alt(( - map(quoted_string, AstValue::Str), - duration_value, - map(float_value, AstValue::Float), - map(integer_value, AstValue::Int), - map(boolean, AstValue::Bool), - map(bare_string, AstValue::Str), - map(identifier, |s: &str| AstValue::Ident(s.to_string())), - ))(input) - } - - /// Parse the arrow operator `->` surrounded by optional whitespace. - pub fn arrow(input: &str) -> IResult<&str, &str> { - preceded(ws, tag("->"))(input) - } -} - -#[cfg(test)] -mod tests { - use super::combinators::*; - use super::*; - use crate::parser::ast::AstValue; - - #[test] - fn strip_line_comments() { - let input = "hello // this is a comment\nworld"; - assert_eq!(strip_comments(input), "hello \nworld"); - } - - #[test] - fn strip_block_comments() { - let input = "before /* inside */ after"; - assert_eq!(strip_comments(input), "before after"); - } - - #[test] - fn strip_block_comments_multiline() { - let input = "a /* line1\nline2 */ b"; - let result = strip_comments(input); - assert_eq!(result, "a \n b"); - } - - #[test] - fn strip_preserves_strings() { - let input = r#""hello // not a comment" rest"#; - assert_eq!(strip_comments(input), r#""hello // not a comment" rest"#); - } - - #[test] - fn strip_string_with_escapes() { - let input = r#""escaped \" quote" rest"#; - assert_eq!(strip_comments(input), r#""escaped \" quote" rest"#); - } - - #[test] - fn parse_identifier() { - assert_eq!(identifier("hello_world123 "), Ok((" ", "hello_world123"))); - assert_eq!(identifier("_private rest"), Ok((" rest", "_private"))); - assert!(identifier("123abc").is_err()); - } - - #[test] - fn parse_qualified_id() { - assert_eq!( - qualified_id("tool_hooks.pre rest"), - Ok((" rest", "tool_hooks.pre".into())) - ); - assert_eq!(qualified_id("a.b.c rest"), Ok((" rest", "a.b.c".into()))); - assert!(qualified_id("simple rest").is_err()); - } - - #[test] - fn parse_key_simple_and_qualified() { - assert_eq!(key("label rest"), Ok((" rest", "label".into()))); - assert_eq!( - key("tool_hooks.pre rest"), - Ok((" rest", "tool_hooks.pre".into())) - ); - } - - #[test] - fn parse_quoted_string() { - assert_eq!(quoted_string(r#""hello""#), Ok(("", "hello".into()))); - assert_eq!( - quoted_string(r#""line1\nline2""#), - Ok(("", "line1\nline2".into())) - ); - assert_eq!( - quoted_string(r#""tab\there""#), - Ok(("", "tab\there".into())) - ); - assert_eq!( - quoted_string(r#""escaped \" quote""#), - Ok(("", "escaped \" quote".into())) - ); - assert_eq!( - quoted_string(r#""back\\slash""#), - Ok(("", "back\\slash".into())) - ); - } - - #[test] - fn parse_boolean() { - assert_eq!(boolean("true rest"), Ok((" rest", true))); - assert_eq!(boolean("false rest"), Ok((" rest", false))); - assert!(boolean("yes").is_err()); - } - - #[test] - fn parse_integer() { - assert_eq!(integer_value("42 rest"), Ok((" rest", 42))); - assert_eq!(integer_value("-1 rest"), Ok((" rest", -1))); - assert_eq!(integer_value("0 rest"), Ok((" rest", 0))); - assert!(integer_value("42.5").is_err()); - } - - #[test] - fn parse_float() { - assert_eq!(float_value("3.15 rest"), Ok((" rest", 3.15))); - assert_eq!(float_value("0.5 rest"), Ok((" rest", 0.5))); - assert_eq!(float_value("-3.15 rest"), Ok((" rest", -3.15))); - assert_eq!(float_value(".5 rest"), Ok((" rest", 0.5))); - } - - #[test] - fn parse_duration() { - assert_eq!( - duration_value("250ms rest"), - Ok((" rest", AstValue::Str("250ms".into()))) - ); - assert_eq!( - duration_value("900s rest"), - Ok((" rest", AstValue::Str("900s".into()))) - ); - assert_eq!( - duration_value("15m rest"), - Ok((" rest", AstValue::Str("15m".into()))) - ); - assert_eq!( - duration_value("2h rest"), - Ok((" rest", AstValue::Str("2h".into()))) - ); - assert_eq!( - duration_value("1d rest"), - Ok((" rest", AstValue::Str("1d".into()))) - ); - } - - #[test] - fn parse_value_all_types() { - assert_eq!(value(r#""hello""#), Ok(("", AstValue::Str("hello".into())))); - assert_eq!(value("250ms"), Ok(("", AstValue::Str("250ms".into())))); - assert_eq!(value("3.15"), Ok(("", AstValue::Float(3.15)))); - assert_eq!(value("42"), Ok(("", AstValue::Int(42)))); - assert_eq!(value("true"), Ok(("", AstValue::Bool(true)))); - assert_eq!(value("LR"), Ok(("", AstValue::Ident("LR".into())))); - } - - #[test] - fn parse_bare_string_with_hyphens_and_dots() { - assert_eq!(bare_string("gpt-5.2 rest"), Ok((" rest", "gpt-5.2".into()))); - assert_eq!( - bare_string("gpt-5.2-codex"), - Ok(("", "gpt-5.2-codex".into())) - ); - assert_eq!( - bare_string("gpt-5.3-codex-spark"), - Ok(("", "gpt-5.3-codex-spark".into())) - ); - assert_eq!( - bare_string("gemini-3-flash-preview"), - Ok(("", "gemini-3-flash-preview".into())) - ); - // Plain identifier without hyphens/dots should fail (identifier handles it) - assert!(bare_string("LR").is_err()); - assert!(bare_string("openai").is_err()); - } - - #[test] - fn parse_value_bare_string() { - assert_eq!(value("gpt-5.2"), Ok(("", AstValue::Str("gpt-5.2".into())))); - assert_eq!( - value("gpt-5.2-codex"), - Ok(("", AstValue::Str("gpt-5.2-codex".into()))) - ); - } -} diff --git a/lib/components/fabro-graphviz/src/parser/mod.rs b/lib/components/fabro-graphviz/src/parser/mod.rs deleted file mode 100644 index 02bcc8df5..000000000 --- a/lib/components/fabro-graphviz/src/parser/mod.rs +++ /dev/null @@ -1,199 +0,0 @@ -pub mod ast; -pub mod grammar; -pub mod lexer; -pub mod semantic; - -use self::ast::DotGraph; -use crate::error::Error; -use crate::graph::types::Graph; - -/// Parse a DOT source string into a raw `DotGraph` AST. -/// -/// Strips comments, parses the grammar, and validates there is no -/// trailing content. Does NOT perform semantic transformation. -/// -/// # Errors -/// -/// Returns an error if the input is not valid DOT syntax or contains -/// trailing content after the graph definition. -pub fn parse_ast(input: &str) -> Result { - let stripped = lexer::strip_comments(input); - let (rest, dot_graph) = grammar::parse_dot_graph(&stripped) - .map_err(|e| Error::Parse(format!("grammar error: {e}")))?; - - let remaining = rest.trim(); - if !remaining.is_empty() { - return Err(Error::Parse(format!( - "unexpected trailing content: {:?}", - &remaining[..remaining.len().min(50)] - ))); - } - - Ok(dot_graph) -} - -/// Parse a DOT source string into a semantic `Graph`. -/// -/// Strips comments, parses the grammar, and performs semantic transformation -/// (expanding chained edges, applying defaults, flattening subgraphs). -/// -/// # Errors -/// -/// Returns an error if the input is not valid DOT syntax or contains -/// trailing content after the graph definition. -pub fn parse(input: &str) -> Result { - let dot_graph = parse_ast(input)?; - semantic::ast_to_graph(&dot_graph) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn parse_simple_linear() { - let input = r#"digraph Simple { - graph [goal="Run tests and report"] - rankdir=LR - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - - run_tests [label="Run Tests", prompt="Run the test suite and report results"] - report [label="Report", prompt="Summarize the test results"] - - start -> run_tests -> report -> exit - }"#; - let graph = parse(input).unwrap(); - assert_eq!(graph.name, "Simple"); - assert_eq!(graph.goal(), "Run tests and report"); - assert_eq!(graph.nodes.len(), 4); - // start->run_tests, run_tests->report, report->exit - assert_eq!(graph.edges.len(), 3); - assert!(graph.nodes.contains_key("start")); - assert!(graph.nodes.contains_key("exit")); - } - - #[test] - fn parse_branching_with_conditions() { - let input = r#"digraph Branch { - graph [goal="Implement and validate a feature"] - rankdir=LR - node [shape=box, timeout="900s"] - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - plan [label="Plan", prompt="Plan the implementation"] - implement [label="Implement", prompt="Implement the plan"] - validate [label="Validate", prompt="Run tests"] - gate [shape=diamond, label="Tests passing?"] - - start -> plan -> implement -> validate -> gate - gate -> exit [label="Yes", condition="outcome=succeeded"] - gate -> implement [label="No", condition="outcome!=succeeded"] - }"#; - let graph = parse(input).unwrap(); - assert_eq!(graph.name, "Branch"); - assert_eq!(graph.nodes.len(), 6); - // chain: 4 edges + 2 conditional = 6 - assert_eq!(graph.edges.len(), 6); - - // Check condition on gate -> exit edge - let gate_exit = graph - .edges - .iter() - .find(|e| e.from == "gate" && e.to == "exit") - .unwrap(); - assert_eq!(gate_exit.condition(), Some("outcome=succeeded")); - } - - #[test] - fn parse_human_gate() { - let input = r#"digraph Review { - rankdir=LR - - start [shape=Mdiamond, label="Start"] - exit [shape=Msquare, label="Exit"] - - review_gate [ - shape=hexagon, - label="Review Changes", - type="human" - ] - - start -> review_gate - review_gate -> ship_it [label="[A] Approve"] - review_gate -> fixes [label="[F] Fix"] - ship_it -> exit - fixes -> review_gate - }"#; - let graph = parse(input).unwrap(); - assert_eq!(graph.name, "Review"); - let gate = &graph.nodes["review_gate"]; - assert_eq!(gate.node_type(), Some("human")); - assert_eq!(gate.shape(), "hexagon"); - } - - #[test] - fn parse_with_comments() { - let input = r"// This is a comment - digraph Test { - /* block comment */ - start [shape=Mdiamond] // inline comment - exit [shape=Msquare] - start -> exit - }"; - let graph = parse(input).unwrap(); - assert_eq!(graph.nodes.len(), 2); - } - - #[test] - fn parse_error_on_invalid_input() { - let result = parse("not a graph"); - assert!(result.is_err()); - } - - #[test] - fn parse_error_on_trailing_content() { - let input = "digraph A { } extra stuff"; - let result = parse(input); - assert!(result.is_err()); - } - - #[test] - fn parse_subgraph_derives_class_on_contained_nodes() { - let input = r#"digraph SubgraphClassTest { - start [shape=Mdiamond] - exit [shape=Msquare] - - subgraph cluster_loop { - label = "Loop A" - plan [label="Plan"] - implement [label="Implement"] - plan -> implement - } - - start -> plan - implement -> exit - }"#; - let graph = parse(input).unwrap(); - assert!(graph.nodes["plan"].classes.contains(&"loop-a".to_string())); - assert!( - graph.nodes["implement"] - .classes - .contains(&"loop-a".to_string()) - ); - } - - #[test] - fn parse_prompt_handler_type_attribute() { - let input = r#"digraph Prompt { - start [shape=Mdiamond] - exit [shape=Msquare] - classify [type="prompt", prompt="Classify this"] - start -> classify -> exit - }"#; - let graph = parse(input).unwrap(); - assert_eq!(graph.nodes["classify"].handler_type(), Some("prompt")); - } -} diff --git a/lib/components/fabro-graphviz/src/parser/semantic.rs b/lib/components/fabro-graphviz/src/parser/semantic.rs deleted file mode 100644 index fca86dafd..000000000 --- a/lib/components/fabro-graphviz/src/parser/semantic.rs +++ /dev/null @@ -1,631 +0,0 @@ -use std::collections::{HashMap, HashSet}; -use std::time::Duration; - -use crate::error::Error; -use crate::graph::types::{AttrValue, Edge, Graph, Node}; -use crate::parser::ast::{AstValue, AttrBlock, DotGraph, EdgeStmt, NodeStmt, Statement}; - -/// Convert an AST `AstValue` to a semantic `AttrValue`. -fn convert_value(ast_val: &AstValue) -> AttrValue { - match ast_val { - AstValue::Str(s) | AstValue::Ident(s) => { - if let Some(dur) = parse_duration_str(s) { - return AttrValue::Duration(dur); - } - AttrValue::String(s.clone()) - } - AstValue::Int(n) => AttrValue::Integer(*n), - AstValue::Float(f) => AttrValue::Float(*f), - AstValue::Bool(b) => AttrValue::Boolean(*b), - } -} - -fn parse_duration_str(s: &str) -> Option { - if s.ends_with("ms") { - let num = s.strip_suffix("ms")?.parse::().ok()?; - return Some(Duration::from_millis(num)); - } - let (num_str, multiplier) = if let Some(n) = s.strip_suffix('s') { - (n, 1_000u64) - } else if let Some(n) = s.strip_suffix('m') { - (n, 60_000u64) - } else if let Some(n) = s.strip_suffix('h') { - (n, 3_600_000u64) - } else if let Some(n) = s.strip_suffix('d') { - (n, 86_400_000u64) - } else { - return None; - }; - let num: u64 = num_str.parse().ok()?; - Some(Duration::from_millis(num * multiplier)) -} - -fn convert_attrs(block: &AttrBlock) -> HashMap { - block - .iter() - .map(|(k, v)| (k.clone(), convert_value(v))) - .collect() -} - -/// Split a `class` attribute value into individual class names. -/// -/// Classes are separated by whitespace. Commas are also accepted, because they -/// were the only separator Fabro used to recognize. Splitting on commas first -/// and then on whitespace drops empty entries without extra trimming. -fn split_class_attr(class_attr: &str) -> impl Iterator { - class_attr.split(',').flat_map(str::split_whitespace) -} - -/// Derive a CSS class name from a subgraph label. -fn derive_class_from_label(label: &str) -> String { - label - .to_lowercase() - .chars() - .map(|c| if c == ' ' { '-' } else { c }) - .filter(|c| c.is_ascii_alphanumeric() || *c == '-') - .collect() -} - -fn collect_declared_node_ids(statements: &[Statement], node_ids: &mut HashSet) { - for statement in statements { - match statement { - Statement::Node(node) => { - node_ids.insert(node.id.clone()); - } - Statement::Subgraph(subgraph) => { - collect_declared_node_ids(&subgraph.statements, node_ids); - } - _ => {} - } - } -} - -struct SemanticState { - graph: Graph, - declared_node_ids: HashSet, - node_defaults: HashMap, - edge_defaults: HashMap, -} - -impl SemanticState { - fn new(name: String, declared_node_ids: HashSet) -> Self { - Self { - graph: Graph::new(name), - declared_node_ids, - node_defaults: HashMap::new(), - edge_defaults: HashMap::new(), - } - } - - fn ensure_node(&mut self, id: &str) -> &mut Node { - let node_defaults = &self.node_defaults; - self.graph.nodes.entry(id.to_string()).or_insert_with(|| { - let mut node = Node::new(id); - node.attrs.clone_from(node_defaults); - node - }) - } - - fn process_node(&mut self, node_stmt: &NodeStmt, subgraph_class: Option<&str>) { - let node = self.ensure_node(&node_stmt.id); - if let Some(attrs) = &node_stmt.attrs { - for (k, v) in attrs { - node.attrs.insert(k.clone(), convert_value(v)); - } - } - if let Some(cls) = subgraph_class { - node.add_class(cls); - } - // Node defaults can also set `class`, so read the merged attrs. The - // clone releases the borrow on `node.attrs` before appending. - let class_attr = node - .attrs - .get("class") - .and_then(AttrValue::as_str) - .map(String::from); - if let Some(class_attr) = class_attr { - for cls in split_class_attr(&class_attr) { - node.add_class(cls); - } - } - } - - fn process_edge(&mut self, edge_stmt: &EdgeStmt, subgraph_class: Option<&str>) { - for id in &edge_stmt.nodes { - if !self.declared_node_ids.contains(id) { - continue; - } - let node = self.ensure_node(id); - if let Some(cls) = subgraph_class { - node.add_class(cls); - } - } - let edge_attrs = edge_stmt - .attrs - .as_ref() - .map_or_else(HashMap::new, convert_attrs); - for pair in edge_stmt.nodes.windows(2) { - let mut edge = Edge::new(&pair[0], &pair[1]); - for (k, v) in &self.edge_defaults { - edge.attrs.insert(k.clone(), v.clone()); - } - for (k, v) in &edge_attrs { - edge.attrs.insert(k.clone(), v.clone()); - } - self.graph.edges.push(edge); - } - } - - fn process_statements( - &mut self, - statements: &[Statement], - subgraph_class: Option<&str>, - scoped_node_defaults: &HashMap, - scoped_edge_defaults: &HashMap, - ) { - let saved_node_defaults = self.node_defaults.clone(); - let saved_edge_defaults = self.edge_defaults.clone(); - for (k, v) in scoped_node_defaults { - self.node_defaults.insert(k.clone(), v.clone()); - } - for (k, v) in scoped_edge_defaults { - self.edge_defaults.insert(k.clone(), v.clone()); - } - - for stmt in statements { - match stmt { - Statement::GraphAttr(attrs) => { - for (k, v) in attrs { - self.graph.attrs.insert(k.clone(), convert_value(v)); - } - } - Statement::NodeDefaults(attrs) => { - for (k, v) in convert_attrs(attrs) { - self.node_defaults.insert(k, v); - } - } - Statement::EdgeDefaults(attrs) => { - for (k, v) in convert_attrs(attrs) { - self.edge_defaults.insert(k, v); - } - } - Statement::GraphAttrDecl(key, val) => { - self.graph.attrs.insert(key.clone(), convert_value(val)); - } - Statement::Node(node_stmt) => { - self.process_node(node_stmt, subgraph_class); - } - Statement::Edge(edge_stmt) => { - self.process_edge(edge_stmt, subgraph_class); - } - Statement::Subgraph(sub) => { - let sub_class = sub.statements.iter().find_map(|s| match s { - Statement::GraphAttrDecl(k, AstValue::Str(s) | AstValue::Ident(s)) - if k == "label" => - { - Some(derive_class_from_label(s)) - } - Statement::GraphAttr(attrs) => attrs.iter().find_map(|(k, v)| { - if k == "label" { - match v { - AstValue::Str(s) | AstValue::Ident(s) => { - Some(derive_class_from_label(s)) - } - _ => None, - } - } else { - None - } - }), - _ => None, - }); - - let mut sub_node_defaults = HashMap::new(); - let mut sub_edge_defaults = HashMap::new(); - for s in &sub.statements { - match s { - Statement::NodeDefaults(attrs) => { - sub_node_defaults.extend(convert_attrs(attrs)); - } - Statement::EdgeDefaults(attrs) => { - sub_edge_defaults.extend(convert_attrs(attrs)); - } - _ => {} - } - } - - self.process_statements( - &sub.statements, - sub_class.as_deref(), - &sub_node_defaults, - &sub_edge_defaults, - ); - } - } - } - - self.node_defaults = saved_node_defaults; - self.edge_defaults = saved_edge_defaults; - } -} - -/// Convert a parsed `DotGraph` AST into a semantic `Graph`. -/// -/// # Errors -/// -/// Returns an error if the AST cannot be converted to a valid graph. -pub fn ast_to_graph(dot: &DotGraph) -> Result { - let mut declared_node_ids = HashSet::new(); - collect_declared_node_ids(&dot.statements, &mut declared_node_ids); - let mut state = SemanticState::new(dot.name.clone(), declared_node_ids); - let empty = HashMap::new(); - state.process_statements(&dot.statements, None, &empty, &empty); - Ok(state.graph) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::parser::ast::SubgraphStmt; - - #[test] - fn convert_ast_str_to_string() { - assert_eq!( - convert_value(&AstValue::Str("hello".into())), - AttrValue::String("hello".into()) - ); - } - - #[test] - fn convert_ast_duration_str() { - assert_eq!( - convert_value(&AstValue::Str("900s".into())), - AttrValue::Duration(Duration::from_mins(15)) - ); - assert_eq!( - convert_value(&AstValue::Str("250ms".into())), - AttrValue::Duration(Duration::from_millis(250)) - ); - assert_eq!( - convert_value(&AstValue::Str("15m".into())), - AttrValue::Duration(Duration::from_mins(15)) - ); - assert_eq!( - convert_value(&AstValue::Str("2h".into())), - AttrValue::Duration(Duration::from_hours(2)) - ); - assert_eq!( - convert_value(&AstValue::Str("1d".into())), - AttrValue::Duration(Duration::from_hours(24)) - ); - } - - #[test] - fn convert_ast_int() { - assert_eq!(convert_value(&AstValue::Int(42)), AttrValue::Integer(42)); - } - - #[test] - fn convert_ast_bool() { - assert_eq!( - convert_value(&AstValue::Bool(true)), - AttrValue::Boolean(true) - ); - } - - #[test] - fn convert_ast_float() { - assert_eq!( - convert_value(&AstValue::Float(3.15)), - AttrValue::Float(3.15) - ); - } - - #[test] - fn convert_ast_ident() { - assert_eq!( - convert_value(&AstValue::Ident("LR".into())), - AttrValue::String("LR".into()) - ); - } - - #[test] - fn derive_class_simple() { - assert_eq!(derive_class_from_label("Loop A"), "loop-a"); - assert_eq!(derive_class_from_label("Code Review"), "code-review"); - assert_eq!(derive_class_from_label("Hello World!!!"), "hello-world"); - } - - #[test] - fn ast_to_graph_simple_linear() { - let dot = DotGraph { - name: "Simple".into(), - statements: vec![ - Statement::GraphAttr(vec![("goal".into(), AstValue::Str("Run tests".into()))]), - Statement::GraphAttrDecl("rankdir".into(), AstValue::Ident("LR".into())), - Statement::Node(NodeStmt { - id: "start".into(), - attrs: Some(vec![ - ("shape".into(), AstValue::Ident("Mdiamond".into())), - ("label".into(), AstValue::Str("Start".into())), - ]), - }), - Statement::Node(NodeStmt { - id: "exit".into(), - attrs: Some(vec![ - ("shape".into(), AstValue::Ident("Msquare".into())), - ("label".into(), AstValue::Str("Exit".into())), - ]), - }), - Statement::Node(NodeStmt { - id: "run_tests".into(), - attrs: Some(vec![("label".into(), AstValue::Str("Run Tests".into()))]), - }), - Statement::Edge(EdgeStmt { - nodes: vec!["start".into(), "run_tests".into(), "exit".into()], - attrs: None, - }), - ], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert_eq!(graph.name, "Simple"); - assert_eq!(graph.goal(), "Run tests"); - assert_eq!(graph.nodes.len(), 3); - assert_eq!(graph.edges.len(), 2); - assert_eq!(graph.edges[0].from, "start"); - assert_eq!(graph.edges[0].to, "run_tests"); - assert_eq!(graph.edges[1].from, "run_tests"); - assert_eq!(graph.edges[1].to, "exit"); - } - - #[test] - fn ast_to_graph_node_defaults_applied() { - let dot = DotGraph { - name: "Defaults".into(), - statements: vec![ - Statement::NodeDefaults(vec![ - ("shape".into(), AstValue::Ident("box".into())), - ("timeout".into(), AstValue::Str("900s".into())), - ]), - Statement::Node(NodeStmt { - id: "plan".into(), - attrs: Some(vec![("label".into(), AstValue::Str("Plan".into()))]), - }), - Statement::Node(NodeStmt { - id: "implement".into(), - attrs: Some(vec![ - ("label".into(), AstValue::Str("Implement".into())), - ("timeout".into(), AstValue::Str("1800s".into())), - ]), - }), - ], - }; - - let graph = ast_to_graph(&dot).unwrap(); - let plan = &graph.nodes["plan"]; - assert_eq!( - plan.attrs.get("shape").and_then(AttrValue::as_str), - Some("box") - ); - assert_eq!( - plan.attrs.get("timeout").and_then(AttrValue::as_duration), - Some(Duration::from_mins(15)) - ); - - let implement = &graph.nodes["implement"]; - assert_eq!( - implement - .attrs - .get("timeout") - .and_then(AttrValue::as_duration), - Some(Duration::from_mins(30)) - ); - } - - #[test] - fn ast_to_graph_subgraph_class_derivation() { - let dot = DotGraph { - name: "SubgraphTest".into(), - statements: vec![Statement::Subgraph(SubgraphStmt { - name: Some("cluster_loop".into()), - statements: vec![ - Statement::GraphAttrDecl("label".into(), AstValue::Str("Loop A".into())), - Statement::Node(NodeStmt { - id: "plan".into(), - attrs: None, - }), - ], - })], - }; - - let graph = ast_to_graph(&dot).unwrap(); - let plan = &graph.nodes["plan"]; - assert!(plan.classes.contains(&"loop-a".to_string())); - } - - #[test] - fn ast_to_graph_subgraph_class_from_graph_attr_block() { - let dot = DotGraph { - name: "SubgraphAttrBlock".into(), - statements: vec![Statement::Subgraph(SubgraphStmt { - name: Some("cluster_review".into()), - statements: vec![ - Statement::GraphAttr(vec![( - "label".into(), - AstValue::Str("Code Review".into()), - )]), - Statement::Node(NodeStmt { - id: "reviewer".into(), - attrs: None, - }), - ], - })], - }; - - let graph = ast_to_graph(&dot).unwrap(); - let reviewer = &graph.nodes["reviewer"]; - assert!(reviewer.classes.contains(&"code-review".to_string())); - } - - #[test] - fn ast_to_graph_edge_defaults_applied() { - let dot = DotGraph { - name: "EdgeDefaults".into(), - statements: vec![ - Statement::EdgeDefaults(vec![("weight".into(), AstValue::Int(5))]), - Statement::Edge(EdgeStmt { - nodes: vec!["a".into(), "b".into()], - attrs: None, - }), - ], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert_eq!( - graph.edges[0] - .attrs - .get("weight") - .and_then(AttrValue::as_i64), - Some(5) - ); - } - - #[test] - fn ast_to_graph_chained_edges_with_attrs() { - let dot = DotGraph { - name: "Chained".into(), - statements: vec![Statement::Edge(EdgeStmt { - nodes: vec!["a".into(), "b".into(), "c".into()], - attrs: Some(vec![("label".into(), AstValue::Str("next".into()))]), - })], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert_eq!(graph.edges.len(), 2); - assert_eq!(graph.edges[0].label(), Some("next")); - assert_eq!(graph.edges[1].label(), Some("next")); - } - - fn classes_from_attr(class_attr: &str) -> Vec { - let dot = DotGraph { - name: "ClassTest".into(), - statements: vec![Statement::Node(NodeStmt { - id: "work".into(), - attrs: Some(vec![("class".into(), AstValue::Str(class_attr.into()))]), - })], - }; - - ast_to_graph(&dot).unwrap().nodes["work"].classes.clone() - } - - #[test] - fn ast_to_graph_class_attr_splits_on_whitespace_and_commas() { - let expected = vec!["coding", "critical"]; - for class_attr in [ - "coding critical", - "coding,critical", - "coding, critical", - "coding critical", - " coding\tcritical\n", - "coding,,critical", - "coding critical coding", - ] { - assert_eq!( - classes_from_attr(class_attr), - expected, - "class attr {class_attr:?}" - ); - } - } - - #[test] - fn ast_to_graph_keeps_undeclared_edge_endpoints_out_of_nodes() { - let dot = DotGraph { - name: "Implicit".into(), - statements: vec![Statement::Edge(EdgeStmt { - nodes: vec!["a".into(), "b".into()], - attrs: None, - })], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert!(graph.nodes.is_empty()); - assert_eq!(graph.edges, vec![Edge::new("a", "b")]); - } - - #[test] - fn ast_to_graph_includes_only_declared_edge_endpoints() { - let dot = DotGraph { - name: "Declared".into(), - statements: vec![ - Statement::Node(NodeStmt { - id: "a".into(), - attrs: None, - }), - Statement::Edge(EdgeStmt { - nodes: vec!["a".into(), "b".into()], - attrs: None, - }), - ], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert!(graph.nodes.contains_key("a")); - assert!(!graph.nodes.contains_key("b")); - } - - #[test] - fn ast_to_graph_declaration_after_edge_still_counts() { - let dot = DotGraph { - name: "DeclaredLater".into(), - statements: vec![ - Statement::NodeDefaults(vec![("model".into(), AstValue::Str("first".into()))]), - Statement::Edge(EdgeStmt { - nodes: vec!["a".into(), "b".into()], - attrs: None, - }), - Statement::NodeDefaults(vec![("model".into(), AstValue::Str("second".into()))]), - Statement::Node(NodeStmt { - id: "b".into(), - attrs: Some(vec![("prompt".into(), AstValue::Str("Do it".into()))]), - }), - ], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert!(graph.nodes.contains_key("b")); - assert!(!graph.nodes.contains_key("a")); - assert_eq!( - graph.nodes["b"] - .attrs - .get("model") - .and_then(AttrValue::as_str), - Some("first") - ); - } - - #[test] - fn ast_to_graph_subgraph_declaration_counts() { - let dot = DotGraph { - name: "SubgraphDeclared".into(), - statements: vec![ - Statement::Edge(EdgeStmt { - nodes: vec!["start".into(), "plan".into()], - attrs: None, - }), - Statement::Subgraph(SubgraphStmt { - name: Some("cluster_loop".into()), - statements: vec![Statement::Node(NodeStmt { - id: "plan".into(), - attrs: None, - })], - }), - ], - }; - - let graph = ast_to_graph(&dot).unwrap(); - assert!(graph.nodes.contains_key("plan")); - assert!(!graph.nodes.contains_key("start")); - } -} diff --git a/lib/components/fabro-graphviz/src/render.rs b/lib/components/fabro-graphviz/src/render.rs index c10ca7989..e9df1c12f 100644 --- a/lib/components/fabro-graphviz/src/render.rs +++ b/lib/components/fabro-graphviz/src/render.rs @@ -2,11 +2,7 @@ use std::borrow::Cow; use std::sync::LazyLock; use anyhow::Context as _; - -use crate::parser; -use crate::parser::ast::{ - AstValue, AttrBlock, DotGraph, EdgeStmt, NodeStmt, Statement, SubgraphStmt, -}; +use fabro_dot::normalize_for_graphviz; /// Dark mode CSS injected into SVG output (leading newline included for /// insertion). @@ -45,10 +41,11 @@ pub fn apply_direction<'a>(source: &'a str, direction: &str) -> std::borrow::Cow RANKDIR_RE.replace(source, replacement.as_str()) } -/// Inject DOT graph-level style defaults. +/// Inject DOT graph-level style defaults, after normalizing Fabro DOT into +/// DOT Graphviz accepts. #[must_use] pub fn inject_dot_style_defaults(source: &str) -> String { - let source = normalize_dot_for_graphviz(source); + let source = normalize_for_graphviz(source); inject_dot_style_defaults_raw(&source) } @@ -78,182 +75,6 @@ pub fn postprocess_svg(raw: Vec) -> Vec { svg.into_bytes() } -/// Convert Fabro DOT accepted by our parser into DOT accepted by Graphviz. -/// -/// Graphviz rejects unquoted dotted attribute keys such as `acp.command`. -/// Fabro's parser accepts those keys, so render paths normalize parsed Fabro -/// DOT before handing it to Graphviz. If the source is valid Graphviz but -/// outside the subset parsed by Fabro, return it unchanged and let Graphviz -/// handle it. -#[must_use] -pub fn normalize_dot_for_graphviz(source: &str) -> std::borrow::Cow<'_, str> { - let Ok(dot) = parser::parse_ast(source) else { - return Cow::Borrowed(source); - }; - Cow::Owned(emit_dot_graph(&dot)) -} - -fn emit_dot_graph(dot: &DotGraph) -> String { - let mut out = String::new(); - out.push_str("digraph "); - out.push_str(&dot_id(&dot.name)); - out.push_str(" {\n"); - emit_statements(&mut out, &dot.statements, 1); - out.push_str("}\n"); - out -} - -fn emit_statements(out: &mut String, statements: &[Statement], indent: usize) { - for statement in statements { - emit_statement(out, statement, indent); - } -} - -fn emit_statement(out: &mut String, statement: &Statement, indent: usize) { - match statement { - Statement::GraphAttr(attrs) => { - push_indent(out, indent); - out.push_str("graph "); - emit_attr_block(out, attrs); - out.push_str(";\n"); - } - Statement::NodeDefaults(attrs) => { - push_indent(out, indent); - out.push_str("node "); - emit_attr_block(out, attrs); - out.push_str(";\n"); - } - Statement::EdgeDefaults(attrs) => { - push_indent(out, indent); - out.push_str("edge "); - emit_attr_block(out, attrs); - out.push_str(";\n"); - } - Statement::Subgraph(subgraph) => emit_subgraph(out, subgraph, indent), - Statement::Node(node) => emit_node(out, node, indent), - Statement::Edge(edge) => emit_edge(out, edge, indent), - Statement::GraphAttrDecl(key, value) => { - push_indent(out, indent); - out.push_str(&dot_id(key)); - out.push('='); - out.push_str(&dot_value(value)); - out.push_str(";\n"); - } - } -} - -fn emit_subgraph(out: &mut String, subgraph: &SubgraphStmt, indent: usize) { - push_indent(out, indent); - out.push_str("subgraph"); - if let Some(name) = &subgraph.name { - out.push(' '); - out.push_str(&dot_id(name)); - } - out.push_str(" {\n"); - emit_statements(out, &subgraph.statements, indent + 1); - push_indent(out, indent); - out.push_str("}\n"); -} - -fn emit_node(out: &mut String, node: &NodeStmt, indent: usize) { - push_indent(out, indent); - out.push_str(&dot_id(&node.id)); - if let Some(attrs) = &node.attrs { - out.push(' '); - emit_attr_block(out, attrs); - } - out.push_str(";\n"); -} - -fn emit_edge(out: &mut String, edge: &EdgeStmt, indent: usize) { - push_indent(out, indent); - let mut nodes = edge.nodes.iter(); - if let Some(first) = nodes.next() { - out.push_str(&dot_id(first)); - for node in nodes { - out.push_str(" -> "); - out.push_str(&dot_id(node)); - } - } - if let Some(attrs) = &edge.attrs { - out.push(' '); - emit_attr_block(out, attrs); - } - out.push_str(";\n"); -} - -fn emit_attr_block(out: &mut String, attrs: &AttrBlock) { - out.push('['); - for (index, (key, value)) in attrs.iter().enumerate() { - if index > 0 { - out.push_str(", "); - } - out.push_str(&dot_id(key)); - out.push('='); - out.push_str(&dot_value(value)); - } - out.push(']'); -} - -fn dot_value(value: &AstValue) -> String { - match value { - AstValue::Str(value) => quoted_dot_string(value), - AstValue::Int(value) => value.to_string(), - AstValue::Float(value) => value.to_string(), - AstValue::Bool(value) => value.to_string(), - AstValue::Ident(value) => dot_id(value), - } -} - -fn dot_id(value: &str) -> String { - if is_plain_dot_id(value) && !is_dot_keyword(value) { - value.to_string() - } else { - quoted_dot_string(value) - } -} - -fn quoted_dot_string(value: &str) -> String { - let mut out = String::with_capacity(value.len() + 2); - out.push('"'); - for ch in value.chars() { - match ch { - '\\' => out.push_str("\\\\"), - '"' => out.push_str("\\\""), - '\n' => out.push_str("\\n"), - '\r' => out.push_str("\\r"), - '\t' => out.push_str("\\t"), - _ => out.push(ch), - } - } - out.push('"'); - out -} - -fn is_plain_dot_id(value: &str) -> bool { - let mut chars = value.chars(); - let Some(first) = chars.next() else { - return false; - }; - if !(first.is_ascii_alphabetic() || first == '_') { - return false; - } - chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_') -} - -fn is_dot_keyword(value: &str) -> bool { - matches!( - value.to_ascii_lowercase().as_str(), - "digraph" | "edge" | "graph" | "node" | "strict" | "subgraph" - ) -} - -fn push_indent(out: &mut String, indent: usize) { - for _ in 0..indent { - out.push_str(" "); - } -} - /// DOT source prepared for Graphviz rendering. pub struct RenderableDot<'a> { source: Cow<'a, str>, @@ -378,49 +199,6 @@ digraph G { assert!(result.is_err()); } - #[test] - fn normalize_dot_quotes_dotted_attribute_keys() { - let source = r#"digraph X { - a [label="A", acp.command="codex"] - }"#; - - let normalized = normalize_dot_for_graphviz(source); - - assert!(normalized.contains(r#""acp.command"="codex""#)); - } - - #[test] - fn normalize_dot_quotes_known_fabro_dotted_attribute_keys() { - let source = r#"digraph X { - approve [human.default_choice="deploy"] - child [stack.child_workflow="child.fabro", manager.max_cycles=50] - approve -> child - }"#; - - let normalized = normalize_dot_for_graphviz(source); - - assert!(normalized.contains(r#""human.default_choice"="deploy""#)); - assert!(normalized.contains(r#""stack.child_workflow"="child.fabro""#)); - assert!(normalized.contains(r#""manager.max_cycles"=50"#)); - } - - #[test] - fn normalize_dot_preserves_subgraphs_and_defaults() { - let source = r##"digraph X { - node [color="#357f9e"] - subgraph cluster_loop { - label="Loop" - a [acp.command="codex"] - } - }"##; - - let normalized = normalize_dot_for_graphviz(source); - - assert!(normalized.contains("node [")); - assert!(normalized.contains("subgraph cluster_loop")); - assert!(normalized.contains(r#""acp.command"="codex""#)); - } - #[test] fn render_dot_accepts_fabro_dotted_attribute_keys() { let svg = render_dot( diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index edc222c0f..6457caa1d 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -18,7 +18,7 @@ async-trait.workspace = true fabro-api = { path = "../../foundation/fabro-api" } fabro-config = { path = "../../foundation/fabro-config" } fabro-github = { path = "../fabro-github" } -fabro-graphviz = { path = "../fabro-graphviz" } +fabro-dot = { path = "../fabro-dot" } fabro-template = { path = "../../foundation/fabro-template" } fabro-tool = { path = "../fabro-tool" } fabro-types = { path = "../../foundation/fabro-types" } diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 43a028dd1..343d87275 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -26,15 +26,13 @@ use fabro_config::{ RunEnvironmentLayer, RunExecutionLayer, RunGoalLayer, RunLayer, RunModelLayer, RunScmLayer, WorkflowSettingsBuilder, }; -use fabro_graphviz::graph::AttrValue; -use fabro_graphviz::parser; +use fabro_dot::WorkflowGraph; use fabro_template::validate_static_reference; -use fabro_types::graph::ReferenceKind; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ApprovalMode, ResolvedGoalSource, ResolvedRunGoal, RunMode}; use fabro_types::{ - DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, RunTarget, - SandboxProviderKind, WorkflowSettings, + DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, ReferenceKind, + RunTarget, SandboxProviderKind, WorkflowSettings, }; use fabro_workflow::git::{self, GitSyncStatus}; pub use fabro_workflow_version::CollectedWorkflowClosure; @@ -274,9 +272,9 @@ fn resolve_manifest_goal( // Precedence 3: graph-level `goal` attribute in the DOT, with `@file` // sugar for workflow-colocated goal files. - let graph = parser::parse(root_source) + let graph = WorkflowGraph::parse(&root_dot_path.display().to_string(), root_source) .with_context(|| format!("Failed to parse {}", root_dot_path.display()))?; - let Some(goal) = graph.attrs.get("goal").and_then(AttrValue::as_str) else { + let Some(goal) = graph.goal() else { return Ok(None); }; if let Some(reference) = goal.strip_prefix('@') { diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index ad6f420f1..ed34c01dd 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -8,14 +8,12 @@ use fabro_config::project::WorkflowLocation; use fabro_config::{ EnvironmentDockerfileLayer, EnvironmentImageLayer, RunGoalLayer, SettingsLayer, }; -use fabro_graphviz::parser; +use fabro_dot::{GraphPosition, GraphReferenceKind, WorkflowGraph}; use fabro_template::{ - BundleTemplateStore, FilesystemTemplateStore, GraphPosition, GraphReference, - GraphReferenceError, RecordingTemplateStore, TemplateContext, TemplateDependencyClosure, - TemplateRenderMode, TemplateSource, validate_static_reference, visit_graph_references, + BundleTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, TemplateContext, + TemplateDependencyClosure, TemplateRenderMode, TemplateSource, validate_static_reference, }; -use fabro_types::ManifestPath; -use fabro_types::graph::ReferenceKind; +use fabro_types::{ManifestPath, ReferenceKind}; use crate::{ WorkflowVersionCollectError, manifest_path_from_absolute, normalize_absolute_path, @@ -222,7 +220,7 @@ impl<'a> WorkflowBundler<'a> { &workflow.dot_path.to_string(), )?; } - let graph = parser::parse(&workflow.source) + let graph = WorkflowGraph::parse(&workflow.dot_path.to_string(), &workflow.source) .with_context(|| format!("Failed to parse {}", workflow.absolute_dot_path.display()))?; let workflow_base_dir = workflow .absolute_dot_path @@ -234,14 +232,14 @@ impl<'a> WorkflowBundler<'a> { manifest_parent_or_dot(&workflow.dot_path)? }; - // Imports and child workflows require a mutable borrow of self, so - // collect them during the walk and recurse after the visitor returns. + // Imports and child workflows recurse, so collect them during the + // walk and follow them after every reference of this graph is read. let mut imports = Vec::new(); let mut children = Vec::new(); - visit_graph_references(&graph, position, |reference| -> Result<()> { - match reference { - GraphReference::GoalFile { reference } => { + for reference in graph.references(position)? { + match reference.kind { + GraphReferenceKind::GoalFile { reference } => { let bundled = self.collect_bundled_file( files, workflow_base_dir, @@ -250,12 +248,16 @@ impl<'a> WorkflowBundler<'a> { ReferenceKind::GraphGoalFile, Some(workflow.dot_path.clone()), )?; - self.collect_bundled_template_includes(files, &bundled, &workflow_template_root) + self.collect_bundled_template_includes( + files, + &bundled, + &workflow_template_root, + )?; } - GraphReference::GoalInline { content } - | GraphReference::InlinePrompt { content } - | GraphReference::ModelStylesheetInline { content } => self - .collect_template_include_files( + GraphReferenceKind::GoalInline { content } + | GraphReferenceKind::InlinePrompt { content } + | GraphReferenceKind::ModelStylesheetInline { content } => { + self.collect_template_include_files( files, TemplateSource::new( workflow.dot_path.clone(), @@ -263,8 +265,9 @@ impl<'a> WorkflowBundler<'a> { content.to_owned(), ), Some(&workflow.dot_path), - ), - GraphReference::FileInline { key, reference } => { + )?; + } + GraphReferenceKind::FileInline { key, reference } => { let bundled = self.collect_bundled_file( files, workflow_base_dir, @@ -280,9 +283,8 @@ impl<'a> WorkflowBundler<'a> { &workflow_template_root, )?; } - Ok(()) } - GraphReference::Import { reference } => { + GraphReferenceKind::Import { reference } => { let imported = self.collect_bundled_file( files, workflow_base_dir, @@ -292,18 +294,10 @@ impl<'a> WorkflowBundler<'a> { Some(workflow.dot_path.clone()), )?; imports.push(imported); - Ok(()) - } - GraphReference::ChildWorkflow { reference } => { - children.push(reference); - Ok(()) } + GraphReferenceKind::ChildWorkflow { reference } => children.push(reference), } - }) - .map_err(|error| match error { - GraphReferenceError::StaticReference(source) => anyhow::Error::new(source), - GraphReferenceError::Visit(error) => error, - })?; + } for imported in imports { if visited_imports.insert(imported.path.to_string()) { @@ -756,19 +750,19 @@ mod tests { } #[test] - fn parse_errors_keep_the_graphviz_error_in_the_source_chain() { + fn parse_errors_keep_petris_error_in_the_source_chain() { let temp = tempfile::tempdir().expect("temp directory should be created"); let graph = temp.path().join("workflow.fabro"); write_file(&graph, "not a graph"); let error = bundle_graph(temp.path(), &graph).expect_err("invalid graph should fail"); - assert!( - error - .chain() - .any(|cause| cause.downcast_ref::().is_some()), - "unexpected error chain: {error:#}" - ); + let parse_error = error + .chain() + .find_map(|cause| cause.downcast_ref::()) + .unwrap_or_else(|| panic!("unexpected error chain: {error:#}")); + assert_eq!(parse_error.file, "workflow.fabro"); + assert_eq!(parse_error.code, "dot.syntax"); } #[test] diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index 566161a9a..a9037ab07 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -275,10 +275,8 @@ mod tests { .with_writer(move || writer.clone()) .finish(); let inputs = [ - source("workflow", &[( - "workflow", - "PRIVATE_CONTENT invalid source", - )]), + // Petri's parse error quotes the token it stopped at. + source("workflow", &[("workflow", "digraph W {} PRIVATE_CONTENT")]), source("workflow.toml", &[( "workflow.toml", "_version = 1\nPRIVATE_CONTENT = [unterminated", diff --git a/lib/components/fabro-workflow-version/Cargo.toml b/lib/components/fabro-workflow-version/Cargo.toml index f3eb60317..6e3a97440 100644 --- a/lib/components/fabro-workflow-version/Cargo.toml +++ b/lib/components/fabro-workflow-version/Cargo.toml @@ -14,7 +14,7 @@ workspace = true [dependencies] fabro-config = { path = "../../foundation/fabro-config" } -fabro-graphviz = { path = "../fabro-graphviz" } +fabro-dot = { path = "../fabro-dot" } fabro-store = { path = "../fabro-store" } fabro-template = { path = "../../foundation/fabro-template" } fabro-types = { path = "../../foundation/fabro-types" } diff --git a/lib/components/fabro-workflow-version/src/lib.rs b/lib/components/fabro-workflow-version/src/lib.rs index b9f8eb6ed..945c34e7f 100644 --- a/lib/components/fabro-workflow-version/src/lib.rs +++ b/lib/components/fabro-workflow-version/src/lib.rs @@ -12,15 +12,15 @@ use fabro_config::parse::{SettingsSource, validate_settings_source}; use fabro_config::{ EnvironmentDockerfileLayer, EnvironmentImageLayer, RunGoalLayer, SettingsLayer, }; -use fabro_graphviz::parser; +use fabro_dot::{GraphPosition, GraphReferenceKind, WorkflowGraph}; use fabro_template::{ - BundleTemplateStore, GraphPosition, GraphReference, GraphReferenceError, StaticReferenceError, - TemplateDiscoveryError, TemplateSource, discover_static_dependency_closure, - validate_static_reference, visit_graph_references, + BundleTemplateStore, StaticReferenceError, TemplateDiscoveryError, TemplateSource, + discover_static_dependency_closure, validate_static_reference, }; -use fabro_types::graph::ReferenceKind; use fabro_types::settings::InterpString; -use fabro_types::{ManifestPath, WorkflowPath, WorkflowPathParseError, WorkflowVersion}; +use fabro_types::{ + ManifestPath, ReferenceKind, WorkflowPath, WorkflowPathParseError, WorkflowVersion, +}; use thiserror::Error; mod closure; @@ -34,7 +34,7 @@ pub enum WorkflowVersionError { GraphParse { path: WorkflowPath, #[source] - source: fabro_graphviz::Error, + source: Box, }, #[error("invalid {kind} in `{path}`: `{reference}`")] InvalidReference { @@ -273,58 +273,57 @@ fn validate_graph_closure( kind: ReferenceKind::Import, target: path.clone(), })?; - let graph = parser::parse(source).map_err(|source| WorkflowVersionError::GraphParse { - path: path.clone(), - source, + let graph = WorkflowGraph::parse(path.as_str(), source).map_err(|source| { + WorkflowVersionError::GraphParse { + path: path.clone(), + source: Box::new(source), + } })?; let position = if &path == version.entrypoint() { GraphPosition::Entrypoint } else { GraphPosition::Imported }; + let references = + graph + .references(position) + .map_err(|source| WorkflowVersionError::StaticReference { + path: path.clone(), + source, + })?; - visit_graph_references(&graph, position, |reference| match reference { - GraphReference::GoalFile { reference } => { - let target = resolve_reference(&path, ReferenceKind::GraphGoalFile, reference)?; - let content = - require_file(version, &path, ReferenceKind::GraphGoalFile, target.clone())?; - template_roots.push(&target, content); - Ok(()) - } - GraphReference::GoalInline { content } - | GraphReference::InlinePrompt { content } - | GraphReference::ModelStylesheetInline { content } => { - template_roots.push(&path, content); - Ok(()) - } - GraphReference::Import { reference } => { - let target = resolve_reference(&path, ReferenceKind::Import, reference)?; - require_file(version, &path, ReferenceKind::Import, target.clone())?; - queue.push_back(target); - Ok(()) - } - GraphReference::ChildWorkflow { reference } => { - let target = resolve_reference(&path, ReferenceKind::ChildWorkflow, reference)?; - child_workflows.insert(target); - Ok(()) - } - GraphReference::FileInline { key, reference } => { - let target = resolve_reference(&path, ReferenceKind::FileInline, reference)?; - let content = - require_file(version, &path, ReferenceKind::FileInline, target.clone())?; - if key == "prompt" { + for reference in references { + match reference.kind { + GraphReferenceKind::GoalFile { reference } => { + let target = resolve_reference(&path, ReferenceKind::GraphGoalFile, reference)?; + let content = + require_file(version, &path, ReferenceKind::GraphGoalFile, target.clone())?; template_roots.push(&target, content); } - Ok(()) + GraphReferenceKind::GoalInline { content } + | GraphReferenceKind::InlinePrompt { content } + | GraphReferenceKind::ModelStylesheetInline { content } => { + template_roots.push(&path, content); + } + GraphReferenceKind::Import { reference } => { + let target = resolve_reference(&path, ReferenceKind::Import, reference)?; + require_file(version, &path, ReferenceKind::Import, target.clone())?; + queue.push_back(target); + } + GraphReferenceKind::ChildWorkflow { reference } => { + let target = resolve_reference(&path, ReferenceKind::ChildWorkflow, reference)?; + child_workflows.insert(target); + } + GraphReferenceKind::FileInline { key, reference } => { + let target = resolve_reference(&path, ReferenceKind::FileInline, reference)?; + let content = + require_file(version, &path, ReferenceKind::FileInline, target.clone())?; + if key == "prompt" { + template_roots.push(&target, content); + } + } } - }) - .map_err(|error| match error { - GraphReferenceError::StaticReference(source) => WorkflowVersionError::StaticReference { - path: path.clone(), - source, - }, - GraphReferenceError::Visit(error) => error, - })?; + } } let configured = version @@ -410,8 +409,7 @@ mod tests { use std::collections::BTreeMap; use fabro_template::{TemplateDiscoveryError, TemplateLoadError}; - use fabro_types::graph::ReferenceKind; - use fabro_types::{BlobHash, WorkflowPath, WorkflowVersion, WorkflowVersionId}; + use fabro_types::{BlobHash, ReferenceKind, WorkflowPath, WorkflowVersion, WorkflowVersionId}; use super::{ValidatedWorkflowVersion, WorkflowVersionError}; diff --git a/lib/components/fabro-workflow/Cargo.toml b/lib/components/fabro-workflow/Cargo.toml index 376415cb0..e13a63064 100644 --- a/lib/components/fabro-workflow/Cargo.toml +++ b/lib/components/fabro-workflow/Cargo.toml @@ -23,7 +23,6 @@ workspace = true anyhow.workspace = true fabro-auth = { path = "../../foundation/fabro-auth" } fabro-config = { path = "../../foundation/fabro-config" } -fabro-graphviz = { path = "../fabro-graphviz" } fabro-sandbox = { path = "../fabro-sandbox" } sandbox-driver.workspace = true pebble-coding-agent.workspace = true diff --git a/lib/components/fabro-workflow/README.md b/lib/components/fabro-workflow/README.md index fa3698ea5..795eb25ea 100644 --- a/lib/components/fabro-workflow/README.md +++ b/lib/components/fabro-workflow/README.md @@ -2,8 +2,9 @@ Fabro's platform half of a workflow run: what Fabro does around the engine. -Petri compiles and executes every run. `fabro-petri` is the one crate that -talks to it, and this crate keeps what Fabro itself owns: +Petri compiles and executes every run. `fabro-petri` is the crate that talks +to the engine (`fabro-dot` reads a graph's shape and file references through +Petri's parser), and this crate keeps what Fabro itself owns: - **`operations`** — creating a run around Petri's admission (`materialize_admitted_run`, `persist_create_run`), and the other run diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index c38ca8ce5..28608920e 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -1,4 +1,3 @@ -use fabro_graphviz::Error as GraphvizError; use fabro_types::diagnostic::Diagnostic; use fabro_util::error::{SharedError, collect_chain, render_with_causes}; use thiserror::Error as ThisError; @@ -80,14 +79,6 @@ impl From for Error { } } -impl From for Error { - fn from(e: GraphvizError) -> Self { - match e { - GraphvizError::Parse(msg) => Self::Parse(msg), - } - } -} - pub type Result = std::result::Result; #[cfg(test)] diff --git a/lib/components/fabro-workflow/src/pull_request.rs b/lib/components/fabro-workflow/src/pull_request.rs index 8ff79ea28..85b81474e 100644 --- a/lib/components/fabro-workflow/src/pull_request.rs +++ b/lib/components/fabro-workflow/src/pull_request.rs @@ -3,7 +3,6 @@ use std::sync::{Arc, LazyLock}; use std::time::Duration; use fabro_github::{self as github_app, ssh_url_to_https}; -use fabro_graphviz::parser; use fabro_llm::credentials::CredentialProvider; use fabro_llm::lithos_catalog::Catalog; use fabro_llm::{Client, ClientOptions, Request, selection}; @@ -202,7 +201,8 @@ fn format_arc_details_section( parts.push(String::new()); parts.push("".to_string()); - // Workflow graph summary — prefer RunSpec's graph, fall back to DOT parsing + // Workflow graph summary, from the run's display graph. The DOT source + // travels with the spec, so it is only shown under the spec's summary. if let Some(record) = run_spec { let workflow_name = if record.graph.name.is_empty() { "unnamed" @@ -227,40 +227,11 @@ fn format_arc_details_section( } parts.push(String::new()); parts.push("".to_string()); - } else if let Some(dot) = dot_source { - parts.push(String::new()); - - // Extract graph name and count nodes/edges for the summary - let (graph_name, node_count, edge_count) = parse_dot_summary(dot); - - parts.push(format!( - "
\nRan {graph_name} ({node_count} {} and {edge_count} {})", - if node_count == 1 { "node" } else { "nodes" }, - if edge_count == 1 { "edge" } else { "edges" } - )); - parts.push(String::new()); - parts.push("```dot".to_string()); - parts.push(dot.to_string()); - parts.push("```".to_string()); - parts.push(String::new()); - parts.push("
".to_string()); } parts.join("\n") } -/// Parse a DOT source string to extract graph name, node count, and edge count. -fn parse_dot_summary(dot: &str) -> (String, usize, usize) { - match parser::parse(dot) { - Ok(graph) => ( - format!("{}.fabro", graph.name), - graph.nodes.len(), - graph.edges.len(), - ), - Err(_) => ("workflow.fabro".to_string(), 0, 0), - } -} - /// Read plan text from the first `plan*` node response in run state. /// /// Nodes are sorted alphabetically so `plan` is preferred over `planning`. @@ -671,8 +642,8 @@ mod tests { use fabro_llm::lithos_catalog::AdapterId; use fabro_llm::{Response, ResponseStream}; use fabro_types::{ - PetriAdmission, RunGraph, RunProjection, RunSpec, StageSummary, WorkflowSettings, - first_event_seq, fixtures, test_support, + PetriAdmission, RunGraph, RunGraphEdge, RunGraphNode, RunProjection, RunSpec, StageHandler, + StageSummary, WorkflowSettings, first_event_seq, fixtures, test_support, }; use fabro_vault::{SecretType, Vault}; use httpmock::Method::{GET, POST}; @@ -913,7 +884,23 @@ capabilities = { text = true, tools = true, response_format = { json_object = tr fn format_arc_details_with_dot_graph() { let conclusion = make_test_conclusion(); let dot = "digraph implement {\n plan [type=\"agent\"]\n code [type=\"agent\"]\n plan -> code\n}\n"; - let section = format_arc_details_section(&conclusion, None, Some(dot)); + let mut graph = RunGraph::new("implement"); + for node in ["plan", "code"] { + graph.nodes.insert(node.to_string(), RunGraphNode { + label: node.to_string(), + kind: StageHandler::Agent, + }); + } + graph.edges.push(RunGraphEdge { + from: "plan".to_string(), + to: "code".to_string(), + }); + let spec = RunSpec { + graph, + graph_source: Some(dot.to_string()), + ..test_support::test_run_spec() + }; + let section = format_arc_details_section(&conclusion, Some(&spec), Some(dot)); assert!(section.contains("implement.fabro")); assert!(section.contains("2 nodes and 1 edge")); @@ -921,6 +908,15 @@ capabilities = { text = true, tools = true, response_format = { json_object = tr assert!(section.contains("digraph implement")); } + #[test] + fn format_arc_details_without_a_spec_has_no_graph_summary() { + let conclusion = make_test_conclusion(); + let section = format_arc_details_section(&conclusion, None, Some("digraph x {}")); + + assert!(!section.contains("```dot")); + assert!(!section.contains("Ran ")); + } + // ── read_plan_text tests ──────────────────────────────────────────── #[test] @@ -1128,29 +1124,6 @@ capabilities = { text = true, tools = true, response_format = { json_object = tr response_mock.assert_async().await; } - // ── parse_dot_summary tests ───────────────────────────────────────── - - #[test] - fn parse_dot_summary_basic() { - let dot = r#"digraph my_workflow { - plan [type="agent"] - code [type="agent"] - plan -> code -}"#; - let (name, nodes, edges) = parse_dot_summary(dot); - assert_eq!(name, "my_workflow.fabro"); - assert_eq!(nodes, 2); - assert_eq!(edges, 1); - } - - #[test] - fn parse_dot_summary_empty() { - let (name, nodes, edges) = parse_dot_summary(""); - assert_eq!(name, "workflow.fabro"); - assert_eq!(nodes, 0); - assert_eq!(edges, 0); - } - // ── format_duration_ms tests ──────────────────────────────────────── #[test] diff --git a/lib/foundation/fabro-template/src/lib.rs b/lib/foundation/fabro-template/src/lib.rs index 0c3c4ba78..610da405b 100644 --- a/lib/foundation/fabro-template/src/lib.rs +++ b/lib/foundation/fabro-template/src/lib.rs @@ -16,10 +16,7 @@ pub use dependency::{ TemplateDependencyKind, TemplateDiscoveryError, discover_static_dependency_closure, extract_template_dependencies, }; -pub use static_reference::{ - GraphPosition, GraphReference, GraphReferenceError, StaticReferenceError, - validate_static_reference, visit_graph_references, -}; +pub use static_reference::{StaticReferenceError, validate_static_reference}; pub use store::{ BundleTemplateStore, CachedTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, TemplateIncludeResolver, TemplateLoadError, TemplateSource, TemplateSourceOrigin, diff --git a/lib/foundation/fabro-template/src/static_reference.rs b/lib/foundation/fabro-template/src/static_reference.rs index 6dc2a310d..ceec839c3 100644 --- a/lib/foundation/fabro-template/src/static_reference.rs +++ b/lib/foundation/fabro-template/src/static_reference.rs @@ -1,19 +1,14 @@ -//! Static file references in workflow graphs. +//! Static file references in workflow packages. //! -//! Workflow graphs name other files through a fixed attribute vocabulary -//! (`import`, `stack.child_workflow`, `@`-prefixed `prompt`/`output_schema` -//! values, the graph `goal`, and inline root template fields such as -//! `model_stylesheet`). File references are *static*: they may not contain -//! template syntax, because they are resolved before template rendering. -//! -//! [`visit_graph_references`] is the one walker over that vocabulary. The -//! manifest bundler and workflow-version validation both consume it, so a new -//! reference-bearing attribute is added here once instead of drifting between -//! per-crate walkers. +//! A workflow names other files from its graph (`import`, +//! `stack.child_workflow`, `@`-prefixed `prompt`, `output_schema` and `goal` +//! values) and from its `workflow.toml` (a Dockerfile, a run goal file). +//! These references are *static*: they may not contain template syntax, +//! because they are resolved before template rendering. The graph walker that +//! finds them lives in `fabro-dot`; this module owns the one rule every +//! consumer applies to a reference before resolving it. -use fabro_types::graph::{ - AttributeScope, Graph, GraphReferenceKind, ReferenceKind, reference_kind_for_attribute, -}; +use fabro_types::ReferenceKind; use crate::contains_template_syntax; @@ -57,131 +52,11 @@ pub fn validate_static_reference( Ok(()) } -/// One file reference or inline template discovered in a workflow graph. -/// -/// `@` prefixes are already stripped from file references; inline variants -/// carry template content that the consumer should feed to template-dependency -/// discovery. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum GraphReference<'graph> { - /// `graph [goal="@"]`. - GoalFile { reference: &'graph str }, - /// A non-`@` graph `goal`: inline template content. - GoalInline { content: &'graph str }, - /// The entrypoint graph's inline `model_stylesheet` template content. - /// - /// Emitted only when the walked graph is [`GraphPosition::Entrypoint`]; - /// imported stylesheets are ignored at runtime, so they are never - /// template roots. - ModelStylesheetInline { content: &'graph str }, - /// `node [import=""]` — another graph file to walk. - Import { reference: &'graph str }, - /// `node [stack.child_workflow=""]`. - ChildWorkflow { reference: &'graph str }, - /// `node [="@"]` for file-inlined attributes - /// (`prompt`, `output_schema`). - FileInline { - key: &'graph str, - reference: &'graph str, - }, - /// A non-`@` node prompt: inline template content. - InlinePrompt { content: &'graph str }, -} - -/// Error from [`visit_graph_references`]. -#[derive(Debug, thiserror::Error)] -pub enum GraphReferenceError { - #[error(transparent)] - StaticReference(StaticReferenceError), - #[error(transparent)] - Visit(E), -} - -/// Whether the walked graph is the workflow's entrypoint or was reached -/// through an `import`/`stack.child_workflow` reference. -/// -/// Position-dependent reference semantics (today: `model_stylesheet` is a -/// template root only on the entrypoint) live in the walker, so every -/// consumer applies the same rule. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum GraphPosition { - Entrypoint, - Imported, -} - -/// Walk every static file reference and inline template in one parsed graph, -/// validating that file references are template-free before emitting them. -/// -/// The walker covers a single graph; recursion into `Import` targets and -/// resolution of references against a file source are the consumer's job. -/// `position` tells the walker whether this graph is the workflow entrypoint, -/// which gates position-dependent references such as `model_stylesheet`. -pub fn visit_graph_references<'graph, E>( - graph: &'graph Graph, - position: GraphPosition, - mut visit: impl FnMut(GraphReference<'graph>) -> Result<(), E>, -) -> Result<(), GraphReferenceError> { - let goal = graph.goal(); - if !goal.is_empty() { - if let Some(reference) = goal.strip_prefix('@') { - validate_static_reference(reference, ReferenceKind::GraphGoalFile) - .map_err(GraphReferenceError::StaticReference)?; - visit(GraphReference::GoalFile { reference }).map_err(GraphReferenceError::Visit)?; - } else { - visit(GraphReference::GoalInline { content: goal }) - .map_err(GraphReferenceError::Visit)?; - } - } - - let model_stylesheet = graph.model_stylesheet(); - if position == GraphPosition::Entrypoint && !model_stylesheet.is_empty() { - visit(GraphReference::ModelStylesheetInline { - content: model_stylesheet, - }) - .map_err(GraphReferenceError::Visit)?; - } - - for node in graph.nodes.values() { - for (key, value) in &node.attrs { - let Some(value) = value.as_str() else { - continue; - }; - let Some(kind) = reference_kind_for_attribute(AttributeScope::Node, key, value) else { - continue; - }; - let reference = match kind { - GraphReferenceKind::Import | GraphReferenceKind::ChildWorkflow => value, - // Classification only yields these kinds for `@` values. - GraphReferenceKind::FileInline | GraphReferenceKind::GraphGoalFile => value - .strip_prefix('@') - .expect("file reference classification requires a leading '@'"), - }; - validate_static_reference(reference, kind.into()) - .map_err(GraphReferenceError::StaticReference)?; - let event = match kind { - GraphReferenceKind::Import => GraphReference::Import { reference }, - GraphReferenceKind::ChildWorkflow => GraphReference::ChildWorkflow { reference }, - GraphReferenceKind::FileInline => GraphReference::FileInline { key, reference }, - GraphReferenceKind::GraphGoalFile => GraphReference::GoalFile { reference }, - }; - visit(event).map_err(GraphReferenceError::Visit)?; - } - - if let Some(prompt) = node.prompt().filter(|prompt| !prompt.starts_with('@')) { - visit(GraphReference::InlinePrompt { content: prompt }) - .map_err(GraphReferenceError::Visit)?; - } - } - Ok(()) -} - #[cfg(test)] mod tests { - use std::collections::BTreeSet; + use fabro_types::ReferenceKind; - use fabro_types::graph::{AttrValue, Graph, Node, ReferenceKind}; - - use super::{GraphPosition, GraphReference, GraphReferenceError, validate_static_reference}; + use super::validate_static_reference; #[test] fn static_reference_rejects_template_syntax() { @@ -203,106 +78,4 @@ mod tests { validate_static_reference("@schemas/result.json", ReferenceKind::FileInline).is_ok() ); } - - 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 visits_every_reference_kind_once() { - let mut graph = Graph::new("test"); - graph.attrs.insert( - "goal".to_string(), - AttrValue::String("@goal.md".to_string()), - ); - graph.attrs.insert( - "model_stylesheet".to_string(), - AttrValue::String("{% include 'styles.partial' %}".to_string()), - ); - for node in [ - node_with("imported", &[("import", "graphs/child.fabro")]), - node_with("child", &[("stack.child_workflow", "children/check.fabro")]), - node_with("file_prompt", &[("prompt", "@prompts/task.md")]), - node_with("inline", &[("prompt", "Do the {{ thing }}")]), - ] { - graph.nodes.insert(node.id.clone(), node); - } - - let mut seen = BTreeSet::new(); - super::visit_graph_references( - &graph, - GraphPosition::Entrypoint, - |reference| -> Result<(), std::convert::Infallible> { - seen.insert(match reference { - GraphReference::GoalFile { reference } => format!("goal-file:{reference}"), - GraphReference::GoalInline { content } => format!("goal-inline:{content}"), - GraphReference::ModelStylesheetInline { content } => { - format!("stylesheet-inline:{content}") - } - GraphReference::Import { reference } => format!("import:{reference}"), - GraphReference::ChildWorkflow { reference } => format!("child:{reference}"), - GraphReference::FileInline { key, reference } => { - format!("file:{key}:{reference}") - } - GraphReference::InlinePrompt { content } => format!("inline:{content}"), - }); - Ok(()) - }, - ) - .unwrap(); - - assert_eq!( - seen, - BTreeSet::from([ - "goal-file:goal.md".to_string(), - "import:graphs/child.fabro".to_string(), - "child:children/check.fabro".to_string(), - "file:prompt:prompts/task.md".to_string(), - "inline:Do the {{ thing }}".to_string(), - "stylesheet-inline:{% include 'styles.partial' %}".to_string(), - ]) - ); - } - - #[test] - fn imported_graphs_do_not_emit_model_stylesheet() { - let mut graph = Graph::new("test"); - graph.attrs.insert( - "model_stylesheet".to_string(), - AttrValue::String("* { reasoning_effort: low; }".to_string()), - ); - - super::visit_graph_references( - &graph, - GraphPosition::Imported, - |reference| -> Result<(), std::convert::Infallible> { - panic!("imported graph emitted {reference:?}") - }, - ) - .unwrap(); - } - - #[test] - fn rejects_template_syntax_in_references_before_visiting() { - let mut graph = Graph::new("test"); - graph.nodes.insert( - "imported".to_string(), - node_with("imported", &[("import", "graphs/{{ name }}.fabro")]), - ); - - let error = super::visit_graph_references( - &graph, - GraphPosition::Entrypoint, - |_| -> Result<(), std::convert::Infallible> { - panic!("references with template syntax must not be visited") - }, - ) - .unwrap_err(); - assert!(matches!(error, GraphReferenceError::StaticReference(_))); - } } diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs deleted file mode 100644 index fe894802c..000000000 --- a/lib/foundation/fabro-types/src/graph.rs +++ /dev/null @@ -1,497 +0,0 @@ -//! The workflow graph as written: the typed model `fabro_graphviz::parser` -//! produces from a DOT file. -//! -//! This is the graph the bundler and workflow version registration walk to -//! find what a workflow references (`import`, `stack.child_workflow`, -//! `@file` prompts, the goal, the model stylesheet). Petri compiles and -//! admits the workflow; the graph a run displays is [`crate::RunGraph`], -//! read off Petri's admitted graph, not this model. - -use std::collections::HashMap; -use std::time::Duration; - -use serde::{Deserialize, Serialize}; - -/// Typed attribute values for nodes, edges, and graph-level attributes. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub enum AttrValue { - String(String), - Integer(i64), - Float(f64), - Boolean(bool), - Duration(Duration), -} - -impl AttrValue { - #[must_use] - pub fn as_str(&self) -> Option<&str> { - match self { - Self::String(s) => Some(s), - _ => None, - } - } - - #[must_use] - pub const fn as_i64(&self) -> Option { - match self { - Self::Integer(n) => Some(*n), - _ => None, - } - } - - #[must_use] - pub const fn as_f64(&self) -> Option { - match self { - Self::Float(n) => Some(*n), - _ => None, - } - } - - #[must_use] - pub const fn as_bool(&self) -> Option { - match self { - Self::Boolean(b) => Some(*b), - _ => None, - } - } - - #[must_use] - pub const fn as_duration(&self) -> Option { - match self { - Self::Duration(d) => Some(*d), - _ => None, - } - } -} - -/// Maps Graphviz shapes to handler type strings (Section 2.8). -#[must_use] -pub fn shape_to_handler_type(shape: &str) -> Option<&'static str> { - match shape { - "Mdiamond" => Some("start"), - "Msquare" => Some("exit"), - "box" => Some("agent"), - "tab" => Some("prompt"), - "hexagon" => Some("human"), - "diamond" => Some("conditional"), - "component" => Some("parallel"), - "tripleoctagon" => Some("parallel.fan_in"), - "parallelogram" => Some("command"), - "house" => Some("stack.manager_loop"), - "insulator" => Some("wait"), - _ => None, - } -} - -/// A node in the workflow graph. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct Node { - pub id: String, - pub attrs: HashMap, - /// CSS-like classes for model stylesheet targeting (from `class` attr and - /// subgraph derivation). - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub classes: Vec, -} - -impl Node { - pub fn new(id: impl Into) -> Self { - Self { - id: id.into(), - attrs: HashMap::new(), - classes: Vec::new(), - } - } - - /// Appends a class, ignoring blank names and ones already present. - /// - /// Classes accumulate from several sources — the `class` attribute and - /// enclosing subgraphs — so every caller needs the same de-duplicating - /// append. The name is trimmed, and a name that is empty or only - /// whitespace is dropped. Order is preserved. - pub fn add_class(&mut self, class: &str) { - let class = class.trim(); - if !class.is_empty() && !self.classes.iter().any(|existing| existing == class) { - self.classes.push(class.to_string()); - } - } - - fn str_attr(&self, key: &str) -> Option<&str> { - self.attrs.get(key).and_then(AttrValue::as_str) - } - - #[must_use] - pub fn label(&self) -> &str { - self.str_attr("label").unwrap_or(&self.id) - } - - /// The node's Graphviz shape, which contributes to handler selection. - /// - /// 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.node_type().is_none() && self.attrs.contains_key("script") { - return "parallelogram"; - } - "box" - } - - #[must_use] - pub fn node_type(&self) -> Option<&str> { - self.str_attr("type") - } - - #[must_use] - pub fn prompt(&self) -> Option<&str> { - self.str_attr("prompt") - } - - /// Resolve the handler type for this node using explicit type or shape - /// mapping. - #[must_use] - pub fn handler_type(&self) -> Option<&str> { - match self.node_type() { - Some("tool") => return Some("command"), - Some(node_type) => return Some(node_type), - None => {} - } - shape_to_handler_type(self.shape()) - } -} - -/// An edge connecting two nodes in the workflow graph. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct Edge { - pub from: String, - pub to: String, - pub attrs: HashMap, -} - -impl Edge { - pub fn new(from: impl Into, to: impl Into) -> Self { - Self { - from: from.into(), - to: to.into(), - attrs: HashMap::new(), - } - } - - fn str_attr(&self, key: &str) -> Option<&str> { - self.attrs.get(key).and_then(AttrValue::as_str) - } - - #[must_use] - pub fn label(&self) -> Option<&str> { - self.str_attr("label") - } - - #[must_use] - pub fn condition(&self) -> Option<&str> { - self.str_attr("condition") - } -} - -/// The parsed workflow graph containing nodes, edges, and graph-level -/// attributes. -#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] -pub struct Graph { - pub name: String, - pub nodes: HashMap, - pub edges: Vec, - pub attrs: HashMap, -} - -impl Graph { - pub fn new(name: impl Into) -> Self { - Self { - name: name.into(), - nodes: HashMap::new(), - edges: Vec::new(), - attrs: HashMap::new(), - } - } - - /// Graph-level goal attribute. - pub fn goal(&self) -> &str { - self.attrs - .get("goal") - .and_then(AttrValue::as_str) - .unwrap_or("") - } - - /// Graph-level model stylesheet attribute. - pub fn model_stylesheet(&self) -> &str { - self.attrs - .get("model_stylesheet") - .and_then(AttrValue::as_str) - .unwrap_or("") - } -} - -/// Where an attribute appears in a workflow graph. -#[derive(Clone, Copy, Debug, Eq, PartialEq, strum::Display, strum::IntoStaticStr)] -#[strum(serialize_all = "snake_case")] -pub enum AttributeScope { - Graph, - Node, - Edge, -} - -/// Kinds of static (non-templated) workflow-owned file references. -#[derive(Clone, Copy, Debug, Eq, PartialEq, strum::Display)] -pub enum ReferenceKind { - #[strum(to_string = "file inline reference")] - FileInline, - #[strum(to_string = "import reference")] - Import, - #[strum(to_string = "child workflow reference")] - ChildWorkflow, - #[strum(to_string = "Dockerfile reference")] - Dockerfile, - #[strum(to_string = "graph goal file reference")] - GraphGoalFile, - #[strum(to_string = "run goal file reference")] - RunGoalFile, -} - -/// Kinds of static file references that graph attributes can carry: the -/// subset of [`ReferenceKind`] that [`reference_kind_for_attribute`] can -/// classify. Config-sourced kinds (Dockerfiles, run goal files) are -/// unrepresentable here by construction. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum GraphReferenceKind { - FileInline, - Import, - ChildWorkflow, - GraphGoalFile, -} - -impl From for ReferenceKind { - fn from(kind: GraphReferenceKind) -> Self { - match kind { - GraphReferenceKind::FileInline => Self::FileInline, - GraphReferenceKind::Import => Self::Import, - GraphReferenceKind::ChildWorkflow => Self::ChildWorkflow, - GraphReferenceKind::GraphGoalFile => Self::GraphGoalFile, - } - } -} - -/// Classify a graph attribute as a static file reference, if it is one. -#[must_use] -pub fn reference_kind_for_attribute( - scope: AttributeScope, - key: &str, - value: &str, -) -> Option { - match key { - "import" if matches!(scope, AttributeScope::Node) => Some(GraphReferenceKind::Import), - "stack.child_workflow" if matches!(scope, AttributeScope::Node) => { - Some(GraphReferenceKind::ChildWorkflow) - } - "goal" if matches!(scope, AttributeScope::Graph) && value.starts_with('@') => { - Some(GraphReferenceKind::GraphGoalFile) - } - "prompt" | "output_schema" - if matches!(scope, AttributeScope::Node) && value.starts_with('@') => - { - Some(GraphReferenceKind::FileInline) - } - _ => None, - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn attr_value_accessors_match_their_variant() { - assert_eq!( - AttrValue::String("hello".to_string()).as_str(), - Some("hello") - ); - assert_eq!(AttrValue::Integer(1).as_str(), None); - assert_eq!(AttrValue::Integer(42).as_i64(), Some(42)); - assert_eq!(AttrValue::String("x".to_string()).as_i64(), None); - assert_eq!(AttrValue::Float(3.15).as_f64(), Some(3.15)); - assert_eq!(AttrValue::Integer(1).as_f64(), None); - assert_eq!(AttrValue::Boolean(true).as_bool(), Some(true)); - assert_eq!(AttrValue::String("true".to_string()).as_bool(), None); - let ten = Duration::from_secs(10); - assert_eq!(AttrValue::Duration(ten).as_duration(), Some(ten)); - assert_eq!(AttrValue::Integer(10).as_duration(), None); - } - - #[test] - fn shape_to_handler_type_mappings() { - assert_eq!(shape_to_handler_type("Mdiamond"), Some("start")); - assert_eq!(shape_to_handler_type("Msquare"), Some("exit")); - assert_eq!(shape_to_handler_type("box"), Some("agent")); - assert_eq!(shape_to_handler_type("tab"), Some("prompt")); - assert_eq!(shape_to_handler_type("hexagon"), Some("human")); - assert_eq!(shape_to_handler_type("diamond"), Some("conditional")); - assert_eq!(shape_to_handler_type("component"), Some("parallel")); - assert_eq!( - shape_to_handler_type("tripleoctagon"), - Some("parallel.fan_in") - ); - assert_eq!(shape_to_handler_type("parallelogram"), Some("command")); - assert_eq!(shape_to_handler_type("house"), Some("stack.manager_loop")); - assert_eq!(shape_to_handler_type("insulator"), Some("wait")); - assert_eq!(shape_to_handler_type("unknown"), None); - } - - #[test] - fn node_defaults() { - let node = Node::new("test"); - assert_eq!(node.id, "test"); - assert_eq!(node.label(), "test"); - assert_eq!(node.shape(), "box"); - assert_eq!(node.node_type(), None); - assert_eq!(node.prompt(), None); - assert!(node.classes.is_empty()); - assert_eq!(node.handler_type(), Some("agent")); - } - - #[test] - fn add_class_trims_names_and_drops_blanks_and_duplicates() { - let mut node = Node::new("work"); - node.add_class("coding"); - node.add_class(" coding "); - node.add_class(""); - node.add_class(" "); - node.add_class("\tcritical\n"); - - assert_eq!(node.classes, ["coding", "critical"]); - } - - 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")); - assert_eq!(node.prompt(), Some("Plan the work")); - } - - #[test] - fn explicit_shape_or_type_wins_over_script_inference() { - let shaped = node_with("odd", &[("shape", "box"), ("script", "cargo build")]); - assert_eq!(shaped.shape(), "box"); - assert_eq!(shaped.handler_type(), Some("agent")); - - let typed = node_with("odd", &[("type", "agent"), ("script", "cargo build")]); - assert_eq!(typed.shape(), "box"); - assert_eq!(typed.handler_type(), Some("agent")); - } - - #[test] - fn any_script_attribute_value_infers_command() { - 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 explicit_types_and_shapes_resolve_handler_types() { - assert_eq!( - node_with("build", &[("type", "tool")]).handler_type(), - Some("command") - ); - assert_eq!( - node_with("gate", &[("type", "human")]).handler_type(), - Some("human") - ); - assert_eq!( - node_with("entry", &[("shape", "Mdiamond")]).handler_type(), - Some("start") - ); - assert_eq!(node_with("odd", &[("shape", "star")]).handler_type(), None); - } - - #[test] - fn edge_attributes_are_read_as_written() { - let bare = Edge::new("a", "b"); - assert_eq!(bare.from, "a"); - assert_eq!(bare.to, "b"); - assert_eq!(bare.label(), None); - assert_eq!(bare.condition(), None); - - let mut edge = Edge::new("a", "b"); - edge.attrs - .insert("label".to_string(), AttrValue::String("next".to_string())); - edge.attrs.insert( - "condition".to_string(), - AttrValue::String("outcome=succeeded".to_string()), - ); - assert_eq!(edge.label(), Some("next")); - assert_eq!(edge.condition(), Some("outcome=succeeded")); - } - - #[test] - fn graph_goal_and_stylesheet_default_to_empty() { - let mut graph = Graph::new("test"); - assert_eq!(graph.name, "test"); - assert_eq!(graph.goal(), ""); - assert_eq!(graph.model_stylesheet(), ""); - - graph.attrs.insert( - "goal".to_string(), - AttrValue::String("Run tests".to_string()), - ); - graph.attrs.insert( - "model_stylesheet".to_string(), - AttrValue::String("* { model: gpt-5.4; }".to_string()), - ); - assert_eq!(graph.goal(), "Run tests"); - assert_eq!(graph.model_stylesheet(), "* { model: gpt-5.4; }"); - } - - #[test] - fn output_schema_at_value_is_file_inline_reference() { - assert_eq!( - reference_kind_for_attribute( - AttributeScope::Node, - "output_schema", - "@schemas/result.schema.json", - ), - Some(GraphReferenceKind::FileInline), - ); - } - - #[test] - fn output_schema_builtin_keyword_is_not_file_inline_reference() { - assert_eq!( - reference_kind_for_attribute(AttributeScope::Node, "output_schema", "routing"), - None, - ); - } -} diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 2563e0c33..c10532063 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -14,7 +14,6 @@ pub mod diff; pub mod engine; pub mod failure_signature; pub mod git_identity; -pub mod graph; mod id; mod input_scalar; pub mod interview; @@ -28,6 +27,7 @@ pub mod pair; pub mod parallel; pub mod principal; pub mod pull_request; +pub mod reference; pub mod repository; pub mod run; pub mod run_failure; @@ -84,7 +84,6 @@ pub use diff::{DiffStats, DiffSummary, RunDiff}; pub use engine::{PetriAdmission, PetriGraphRef}; pub use failure_signature::FailureSignature; pub use git_identity::{GitIdentity, GitIdentitySource}; -pub use graph::{AttrValue, AttributeScope, Edge, Graph, Node, shape_to_handler_type}; pub use input_scalar::{ JsonScalarToTomlError, TomlScalarToJsonError, json_scalar_to_toml_value, toml_scalar_to_json_value, @@ -129,6 +128,7 @@ pub use pull_request::{ PullRequestDetailsUnavailableReason, PullRequestGithubDetail, PullRequestLink, PullRequestMeta, PullRequestRef, PullRequestResponse, PullRequestTimestamps, PullRequestUser, }; +pub use reference::ReferenceKind; pub use repository::{ GitHubRepositorySlug, GitHubRepositorySlugError, RepositoryProvider, RepositoryRef, is_valid_git_branch_name, is_valid_git_tag_name, normalize_git_commit_sha, diff --git a/lib/foundation/fabro-types/src/reference.rs b/lib/foundation/fabro-types/src/reference.rs new file mode 100644 index 000000000..45c9b69ba --- /dev/null +++ b/lib/foundation/fabro-types/src/reference.rs @@ -0,0 +1,25 @@ +//! The kinds of static file reference a workflow package carries. +//! +//! A workflow names other files from its graph (`import`, +//! `stack.child_workflow`, `@`-prefixed `prompt`, `output_schema` and `goal` +//! values) and from its `workflow.toml` (a Dockerfile, a run goal file). +//! Every such reference is static: it is resolved before any template +//! renders, so it may not contain template syntax. The kind names which rule +//! a reference was read under, for error messages. + +/// Kinds of static (non-templated) workflow-owned file references. +#[derive(Clone, Copy, Debug, Eq, PartialEq, strum::Display)] +pub enum ReferenceKind { + #[strum(to_string = "file inline reference")] + FileInline, + #[strum(to_string = "import reference")] + Import, + #[strum(to_string = "child workflow reference")] + ChildWorkflow, + #[strum(to_string = "Dockerfile reference")] + Dockerfile, + #[strum(to_string = "graph goal file reference")] + GraphGoalFile, + #[strum(to_string = "run goal file reference")] + RunGoalFile, +}