diff --git a/Cargo.lock b/Cargo.lock index 4b0fbcbdc..63d11f266 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1761,7 +1761,6 @@ dependencies = [ "fs2", "futures", "git2", - "graphviz-sys", "hkdf", "httpmock", "indicatif", diff --git a/apps/fabro-web/app/routes/run-overview.test.tsx b/apps/fabro-web/app/routes/run-overview.test.tsx new file mode 100644 index 000000000..41caeaa8b --- /dev/null +++ b/apps/fabro-web/app/routes/run-overview.test.tsx @@ -0,0 +1,88 @@ +import { afterEach, describe, expect, mock, test } from "bun:test"; +import TestRenderer, { act } from "react-test-renderer"; +import { MemoryRouter, Route, Routes } from "react-router"; + +import { ApiError } from "../lib/api-client"; + +let currentGraphData: string | null | undefined; +let currentGraphError: Error | undefined; +let currentGraphLoading = false; + +const graphMutateMock = mock(() => Promise.resolve(currentGraphData)); + +mock.module("../lib/queries", () => ({ + useRun: () => ({ data: undefined }), + useRunStages: () => ({ data: undefined }), + useRunGraph: () => ({ + data: currentGraphData, + error: currentGraphError, + isLoading: currentGraphLoading, + mutate: graphMutateMock, + }), +})); + +mock.module("../components/run-summary-panel", () => ({ + RunSummaryPanel: () =>
summary
, +})); + +mock.module("../components/stage-sidebar", () => ({ + StageSidebar: () => , +})); + +const { default: RunOverview } = await import("./run-overview"); + +const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; + +function textFromNode( + node: ReturnType, +): string { + if (!node) return ""; + if (typeof node === "string") return node; + if (Array.isArray(node)) return node.map(textFromNode).join(" "); + return (node.children ?? []).map(textFromNode).join(" "); +} + +function render(): TestRenderer.ReactTestRenderer { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + let renderer!: TestRenderer.ReactTestRenderer; + act(() => { + renderer = TestRenderer.create( + + + } /> + + , + ); + }); + mountedRenderers.push(renderer); + return renderer; +} + +afterEach(() => { + for (const renderer of mountedRenderers.splice(0)) { + act(() => renderer.unmount()); + } + currentGraphData = undefined; + currentGraphError = undefined; + currentGraphLoading = false; + graphMutateMock.mockClear(); + delete (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT; +}); + +describe("RunOverview", () => { + test("shows graph render errors instead of the empty graph state", () => { + currentGraphError = new ApiError({ + status: 400, + message: "failed to parse DOT source", + requestId: "req_123", + body: null, + }); + + const renderer = render(); + const text = textFromNode(renderer.toJSON()); + + expect(text).toContain("Couldn't render workflow graph"); + expect(text).toContain("failed to parse DOT source"); + expect(text).not.toContain("No workflow graph"); + }); +}); diff --git a/apps/fabro-web/app/routes/run-overview.tsx b/apps/fabro-web/app/routes/run-overview.tsx index 71f1de877..6a62cf50c 100644 --- a/apps/fabro-web/app/routes/run-overview.tsx +++ b/apps/fabro-web/app/routes/run-overview.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useNavigate, useParams } from "react-router"; import { graphTheme } from "../lib/graph-theme"; +import { ApiError } from "../lib/api-client"; import { useRun, useRunGraph, useRunStages } from "../lib/queries"; import { RunSummaryPanel } from "../components/run-summary-panel"; import { StageSidebar } from "../components/stage-sidebar"; @@ -9,7 +10,7 @@ import { GRAPH_ZOOM_STEPS, GraphToolbar, } from "../components/graph-toolbar"; -import { EmptyState } from "../components/state"; +import { EmptyState, ErrorState } from "../components/state"; import { ACTIVE_STAGE_STATES, SUCCEEDED_STAGE_STATES, @@ -32,6 +33,12 @@ export default function RunOverview() { [stagesQuery.data], ); const graphSvg = graphQuery.data; + const graphErrorDescription = + graphQuery.error instanceof ApiError + ? graphQuery.error.message + : graphQuery.error + ? "The graph render request failed." + : undefined; const apiStatus = runQuery.data?.lifecycle.status; const terminalOutcome: "succeeded" | "failed" | "dead" | null = apiStatus?.kind === "succeeded" || @@ -223,6 +230,12 @@ export default function RunOverview() { /> + ) : graphQuery.error ? ( + void graphQuery.mutate()} + /> ) : ( i32 { @@ -18,7 +20,8 @@ pub(crate) fn execute() -> i32 { return 1; } - match graphviz_sys::render_dot_to_svg(&dot_source) { + let dot = render::RenderableDot::from_fabro_source(&dot_source); + match render::render_raw_svg(&dot) { Ok(svg) => { if std::io::stdout().write_all(&svg).is_err() { return 1; diff --git a/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs index 22714fdb4..27644fe33 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs @@ -77,6 +77,47 @@ fn render_graph_outputs_svg() { ); } +#[test] +fn render_graph_accepts_fabro_dotted_attributes() { + let context = test_context!(); + let mut cmd = render_graph_command(&context); + cmd.args(["__render-graph"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("render-graph subprocess should spawn"); + child + .stdin + .as_mut() + .expect("stdin should be piped") + .write_all( + br#"digraph X { + start [shape=Mdiamond] + exit [shape=Msquare] + a [label="A", acp.command="codex"] + start -> a -> exit + }"#, + ) + .expect("stdin write should succeed"); + + let output = child + .wait_with_output() + .expect("render-graph subprocess should exit"); + + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).expect("stdout should be valid UTF-8"); + assert!( + stdout.contains(") -> Vec { svg.into_bytes() } -/// Render styled DOT source into SVG via the vendored Graphviz library. -pub fn render_dot(source: &str) -> anyhow::Result> { - let styled_source = inject_dot_style_defaults(source); - let raw = graphviz_sys::render_dot_to_svg(&styled_source) +/// 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>, +} + +impl<'a> RenderableDot<'a> { + /// Prepare Fabro DOT for Graphviz by applying render styling and + /// normalizing Fabro-specific syntax such as dotted attribute keys. + #[must_use] + pub fn from_fabro_source(source: &'a str) -> Self { + let styled_source = inject_dot_style_defaults(source); + let render_source = normalize_dot_for_graphviz(&styled_source).into_owned(); + Self { + source: Cow::Owned(render_source), + } + } + + /// Return the DOT source that can be handed to Graphviz. + #[must_use] + pub fn as_graphviz_source(&self) -> &str { + &self.source + } +} + +/// Render prepared DOT source into raw SVG via the vendored Graphviz library. +/// +/// This is the only `graphviz_sys` boundary in the workspace. +pub fn render_raw_svg(dot: &RenderableDot<'_>) -> anyhow::Result> { + graphviz_sys::render_dot_to_svg(dot.as_graphviz_source()) .map_err(anyhow::Error::msg) - .context("Graphviz rendering failed")?; + .context("Graphviz rendering failed") +} + +/// Render prepared DOT source into post-processed SVG. +pub fn render_svg(dot: &RenderableDot<'_>) -> anyhow::Result> { + let raw = render_raw_svg(dot)?; Ok(postprocess_svg(raw)) } +/// Render Fabro DOT source into post-processed SVG. +pub fn render_dot(source: &str) -> anyhow::Result> { + let dot = RenderableDot::from_fabro_source(source); + render_svg(&dot) +} + #[cfg(test)] mod tests { use super::*; @@ -140,4 +358,130 @@ mod tests { let result = render_dot("not valid dot {{{"); 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( + r#"digraph X { + start [shape=Mdiamond] + exit [shape=Msquare] + a [label="A", acp.command="codex"] + start -> a -> exit + }"#, + ) + .unwrap(); + + assert!(String::from_utf8(svg).unwrap().contains(" Vec { + let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../../test/dot-compatibility"); + let mut fixtures = std::fs::read_dir(&dir) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", dir.display())) + .map(|entry| { + entry + .unwrap_or_else(|err| { + panic!("failed to read entry in {}: {err}", dir.display()) + }) + .path() + }) + .filter(|path| path.extension().and_then(|ext| ext.to_str()) == Some("fabro")) + .collect::>(); + fixtures.sort(); + fixtures + } } diff --git a/lib/crates/fabro-server/src/server/handler/graph.rs b/lib/crates/fabro-server/src/server/handler/graph.rs index c65598545..96891dac9 100644 --- a/lib/crates/fabro-server/src/server/handler/graph.rs +++ b/lib/crates/fabro-server/src/server/handler/graph.rs @@ -125,7 +125,7 @@ fn render_subprocess_failure( } pub(in crate::server) async fn render_dot_subprocess( - styled_source: &str, + dot_source: &str, exe_override: Option<&std::path::Path>, ) -> Result, RenderSubprocessError> { let _permit = GRAPHVIZ_RENDER_SEMAPHORE @@ -147,7 +147,7 @@ pub(in crate::server) async fn render_dot_subprocess( let mut stdin = child.stdin.take().ok_or_else(|| { RenderSubprocessError::SpawnFailed("render subprocess stdin was not piped".to_string()) })?; - if let Err(err) = stdin.write_all(styled_source.as_bytes()).await { + if let Err(err) = stdin.write_all(dot_source.as_bytes()).await { drop(stdin); let output = child .wait_with_output() @@ -192,10 +192,9 @@ async fn render_graph_response( dot_source: &str, exe_override: Option<&std::path::Path>, ) -> Response { - use fabro_graphviz::render::{inject_dot_style_defaults, postprocess_svg}; + use fabro_graphviz::render::postprocess_svg; - let styled_source = inject_dot_style_defaults(dot_source); - match render_dot_subprocess(&styled_source, exe_override).await { + match render_dot_subprocess(dot_source, exe_override).await { Ok(raw) => { let bytes = postprocess_svg(raw); (StatusCode::OK, [("content-type", "image/svg+xml")], bytes).into_response() diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index b7d3984a5..9c77560d4 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -9059,6 +9059,56 @@ async fn render_graph_from_manifest_returns_svg() { ); } +#[tokio::test] +async fn render_graph_from_manifest_accepts_fabro_dotted_attributes() { + let app = test_app_with(); + let dot_source = r#"digraph X { + start [shape=Mdiamond] + exit [shape=Msquare] + a [label="A", acp.command="codex"] + start -> a -> exit +}"#; + + let req = Request::builder() + .method("POST") + .uri(api("/graph/render")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "manifest": { + "version": 1, + "cwd": "/tmp", + "target": { + "identifier": "workflow.fabro", + "path": "workflow.fabro", + }, + "workflows": { + "workflow.fabro": { + "source": dot_source, + "files": {}, + }, + }, + }, + "format": "svg", + })) + .unwrap(), + )) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + + let response = checked_response!(response, StatusCode::OK).await; + assert_eq!( + response + .headers() + .get("content-type") + .expect("content-type header should be present") + .to_str() + .unwrap(), + "image/svg+xml" + ); +} + #[cfg(unix)] #[tokio::test] async fn render_graph_bytes_returns_bad_request_for_render_error_protocol() { diff --git a/lib/crates/fabro-validate/src/lib.rs b/lib/crates/fabro-validate/src/lib.rs index 20b0fbeca..e5430c481 100644 --- a/lib/crates/fabro-validate/src/lib.rs +++ b/lib/crates/fabro-validate/src/lib.rs @@ -152,6 +152,7 @@ pub fn validate_with_catalog_or_raise( #[cfg(test)] mod tests { use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; + use fabro_graphviz::parser; use fabro_model::catalog::LlmCatalogSettings; use fabro_model::{Catalog, ProviderId}; @@ -368,4 +369,51 @@ reasoning = false assert_ne!(Severity::Error, Severity::Warning); assert_ne!(Severity::Warning, Severity::Info); } + + #[expect( + clippy::disallowed_methods, + reason = "unit test reads checked-in DOT compatibility fixtures synchronously" + )] + #[test] + fn dot_compatibility_corpus_validates() { + let fixtures = dot_compatibility_fixtures(); + + assert_eq!( + fixtures.len(), + 3, + "dot compatibility corpus should stay intentionally small" + ); + + for fixture in fixtures { + let source = std::fs::read_to_string(&fixture) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", fixture.display())); + let graph = parser::parse(&source) + .unwrap_or_else(|err| panic!("failed to parse {}: {err}", fixture.display())); + + validate_or_raise(&graph, &[]) + .unwrap_or_else(|err| panic!("failed to validate {}: {err}", fixture.display())); + } + } + + #[expect( + clippy::disallowed_methods, + reason = "unit test helper enumerates checked-in DOT compatibility fixtures synchronously" + )] + fn dot_compatibility_fixtures() -> Vec { + let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../../test/dot-compatibility"); + let mut fixtures = std::fs::read_dir(&dir) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", dir.display())) + .map(|entry| { + entry + .unwrap_or_else(|err| { + panic!("failed to read entry in {}: {err}", dir.display()) + }) + .path() + }) + .filter(|path| path.extension().and_then(|ext| ext.to_str()) == Some("fabro")) + .collect::>(); + fixtures.sort(); + fixtures + } } diff --git a/test/dot-compatibility/acp-agent-chain.fabro b/test/dot-compatibility/acp-agent-chain.fabro new file mode 100644 index 000000000..fa023ba91 --- /dev/null +++ b/test/dot-compatibility/acp-agent-chain.fabro @@ -0,0 +1,23 @@ +digraph AcpAgentChain { + graph [goal="Run two ACP agent stages in sequence"] + rankdir=LR + + node [shape=box, timeout="900s"] + edge [color="#666666"] + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + plan [ + label="Plan" + backend="acp" + acp.command="python3 tools/fake_acp_agent.py plan" + ] + implement [ + label="Implement" + backend="acp" + acp.command="python3 tools/fake_acp_agent.py implement" + ] + + start -> plan -> implement -> exit +} diff --git a/test/dot-compatibility/human-default-choice.fabro b/test/dot-compatibility/human-default-choice.fabro new file mode 100644 index 000000000..1c7ba8fb1 --- /dev/null +++ b/test/dot-compatibility/human-default-choice.fabro @@ -0,0 +1,17 @@ +digraph HumanDefaultChoice { + graph [goal="Ask for approval and use a default deployment choice on timeout"] + rankdir=LR + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + approve [shape=hexagon, label="Approve deploy?", human.default_choice="deploy"] + deploy [shape=parallelogram, label="Deploy", script="echo deploy"] + revise [label="Revise", prompt="Revise the change for another approval pass"] + + start -> approve + approve -> deploy [label="[D] Deploy"] + approve -> revise [label="[R] Revise"] + revise -> approve + deploy -> exit +} diff --git a/test/dot-compatibility/subworkflow-manager.fabro b/test/dot-compatibility/subworkflow-manager.fabro new file mode 100644 index 000000000..3deb58d44 --- /dev/null +++ b/test/dot-compatibility/subworkflow-manager.fabro @@ -0,0 +1,25 @@ +digraph SubworkflowManager { + graph [goal="Delegate implementation to a child workflow and review the result"] + rankdir=LR + + node [shape=box, timeout="900s"] + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + subgraph cluster_delivery { + label="Delivery" + + plan [label="Plan", prompt="Plan the child workflow handoff"] + impl [ + label="Implement & Test" + shape=house + stack.child_workflow="implement-and-test.fabro" + manager.max_cycles=50 + ] + } + + review [label="Review", prompt="Review the child workflow result"] + + start -> plan -> impl -> review -> exit +}