diff --git a/Cargo.lock b/Cargo.lock index 14c6292d2..731a30eb3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1588,6 +1588,7 @@ dependencies = [ "fabro-devcontainer", "fabro-github", "fabro-graphviz", + "fabro-graphviz-sys", "fabro-hooks", "fabro-http", "fabro-interview", diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index edeb11cba..741f76edc 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -34,6 +34,7 @@ fabro-retro = { path = "../fabro-retro" } fabro-sandbox = { path = "../fabro-sandbox", features = ["daytona"] } fabro-checkpoint = { path = "../fabro-checkpoint" } fabro-graphviz = { path = "../fabro-graphviz" } +fabro-graphviz-sys = { path = "../fabro-graphviz-sys" } fabro-validate = { path = "../fabro-validate" } fabro-workflow = { path = "../fabro-workflow" } fabro-server = { path = "../fabro-server" } diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index a8d98959a..441e60643 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -1023,6 +1023,9 @@ pub(crate) enum Commands { /// Path to the JSON event file path: PathBuf, }, + /// Render a DOT graph to SVG (internal) + #[command(name = "__render-graph", hide = true)] + RenderGraph, /// Build a panic event and write JSON to stdout (internal testing) #[cfg(debug_assertions)] #[command(name = "__test_panic", hide = true)] @@ -1100,6 +1103,7 @@ impl Commands { }, Self::SendAnalytics { .. } => "__send_analytics", Self::SendPanic { .. } => "__send_panic", + Self::RenderGraph => "__render-graph", #[cfg(debug_assertions)] Self::TestPanic { .. } => "__test_panic", } diff --git a/lib/crates/fabro-cli/src/commands/mod.rs b/lib/crates/fabro-cli/src/commands/mod.rs index 72995e763..928e3b2fb 100644 --- a/lib/crates/fabro-cli/src/commands/mod.rs +++ b/lib/crates/fabro-cli/src/commands/mod.rs @@ -9,6 +9,7 @@ pub(crate) mod parse; pub(crate) mod pr; pub(crate) mod preflight; pub(crate) mod provider; +pub(crate) mod render_graph; pub(crate) mod repo; pub(crate) mod run; pub(crate) mod runs; diff --git a/lib/crates/fabro-cli/src/commands/render_graph.rs b/lib/crates/fabro-cli/src/commands/render_graph.rs new file mode 100644 index 000000000..ce1ec4e16 --- /dev/null +++ b/lib/crates/fabro-cli/src/commands/render_graph.rs @@ -0,0 +1,25 @@ +use std::io::{Read, Write}; + +const RENDER_ERROR_PREFIX: &str = "RENDER_ERROR:"; + +pub(crate) fn execute() -> i32 { + let mut dot_source = String::new(); + if std::io::stdin().read_to_string(&mut dot_source).is_err() { + return 1; + } + + match fabro_graphviz_sys::render_dot_to_svg(&dot_source) { + Ok(svg) => { + if std::io::stdout().write_all(&svg).is_err() { + return 1; + } + 0 + } + Err(err) => { + if write!(std::io::stdout(), "{RENDER_ERROR_PREFIX}{err}").is_err() { + return 1; + } + 0 + } + } +} diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 35481ee2c..15d3344f6 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -61,11 +61,17 @@ impl Cli { #[expect(clippy::print_stderr, reason = "fatal error reporting before exit")] #[tokio::main] async fn main() { + let raw_args: Vec = std::env::args().collect(); + let subcommand = raw_args.get(1).map(String::as_str); + let subcommand_arg = raw_args.get(2).map(String::as_str); + if subcommand == Some("__render-graph") && !matches!(subcommand_arg, Some("--help" | "-h")) { + std::process::exit(commands::render_graph::execute()); + } + tel_panic::install_panic_hook(); fabro_telemetry::init_cli(); let start = std::time::Instant::now(); - let raw_args: Vec = std::env::args().collect(); let (command_name, result) = Box::pin(main_inner()).await; let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap(); @@ -368,6 +374,7 @@ async fn main_inner() -> (String, Result<()>) { let _ = std::fs::remove_file(&path); result?; } + Commands::RenderGraph => unreachable!("__render-graph handled before CLI bootstrap"), #[cfg(debug_assertions)] Commands::TestPanic { message } => { let event = tel_panic::build_event(&message); @@ -701,6 +708,15 @@ mod tests { } } + #[test] + fn parse_render_graph_command() { + let cli = Cli::try_parse_from(["fabro", "__render-graph"]).expect("should parse"); + match *cli.command { + Commands::RenderGraph => {} + _ => panic!("unexpected command variant"), + } + } + #[test] fn parse_settings_command() { let cli = Cli::try_parse_from(["fabro", "settings"]).expect("should parse"); diff --git a/lib/crates/fabro-cli/tests/it/cmd/mod.rs b/lib/crates/fabro-cli/tests/it/cmd/mod.rs index aad21bc9a..673fcd3ae 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mod.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mod.rs @@ -29,6 +29,7 @@ mod preflight; mod provider; mod provider_login; mod ps; +mod render_graph; mod repo; mod repo_deinit; mod repo_init; diff --git a/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs new file mode 100644 index 000000000..088cec424 --- /dev/null +++ b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs @@ -0,0 +1,105 @@ +use std::io::Write; +use std::process::{Command, Stdio}; + +use fabro_test::{fabro_snapshot, test_context}; + +fn render_graph_command(context: &fabro_test::TestContext) -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_fabro")); + cmd.current_dir(&context.temp_dir); + cmd.env("NO_COLOR", "1"); + cmd.env("HOME", &context.home_dir); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled"); + cmd +} + +#[test] +fn help() { + let context = test_context!(); + let mut cmd = context.command(); + cmd.args(["__render-graph", "--help"]); + fabro_snapshot!(context.filters(), cmd, @" + success: true + exit_code: 0 + ----- stdout ----- + Render a DOT graph to SVG (internal) + + Usage: fabro __render-graph [OPTIONS] + + Options: + --json Output as JSON [env: FABRO_JSON=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); +} + +#[test] +fn render_graph_outputs_svg() { + 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(b"digraph { a -> b }") + .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(" = + LazyLock::new(|| Semaphore::new(GRAPHVIZ_RENDER_CONCURRENCY_LIMIT)); + +#[derive(Debug, thiserror::Error)] +enum RenderSubprocessError { + #[error("failed to spawn render subprocess: {0}")] + SpawnFailed(String), + #[error("render subprocess crashed: {0}")] + ChildCrashed(String), + #[error("render subprocess returned invalid output: {0}")] + ProtocolViolation(String), + #[error("{0}")] + RenderFailed(String), +} const MAX_MULTIPART_REQUEST_BYTES: u64 = 50 * 1024 * 1024; const MAX_MULTIPART_MANIFEST_BYTES: usize = 256 * 1024; @@ -6181,21 +6198,162 @@ async fn create_completion( } } -/// Render DOT source to a styled SVG image via `render_dot` on a blocking -/// thread. -pub(crate) async fn render_graph_bytes(dot_source: &str) -> Response { - use fabro_graphviz::render::render_dot; +fn render_graph_subprocess_exe( + exe_override: Option<&std::path::Path>, +) -> Result { + match exe_override { + Some(path) => Ok(path.to_path_buf()), + None => { + if let Some(path) = std::env::var_os("CARGO_BIN_EXE_fabro").map(PathBuf::from) { + return Ok(path); + } - let source = dot_source.to_owned(); - match spawn_blocking(move || render_dot(&source)).await { - Ok(Ok(bytes)) => { + let current = std::env::current_exe() + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + let current_name = current.file_stem().and_then(|name| name.to_str()); + if current_name == Some("fabro") { + return Ok(current); + } + + let candidate = current + .parent() + .and_then(|parent| parent.parent()) + .map(|parent| parent.join(if cfg!(windows) { "fabro.exe" } else { "fabro" })); + if let Some(candidate) = candidate.filter(|path| path.is_file()) { + return Ok(candidate); + } + + Ok(current) + } + } +} + +fn render_subprocess_failure( + status: &std::process::ExitStatus, + stderr: &[u8], +) -> RenderSubprocessError { + #[cfg(unix)] + { + use std::os::unix::process::ExitStatusExt; + + if let Some(signal) = status.signal() { + let stderr = String::from_utf8_lossy(stderr).trim().to_string(); + let detail = if stderr.is_empty() { + format!("terminated by signal {signal}") + } else { + format!("terminated by signal {signal}: {stderr}") + }; + return RenderSubprocessError::ChildCrashed(detail); + } + } + + let stderr = String::from_utf8_lossy(stderr).trim().to_string(); + let detail = match status.code() { + Some(code) if stderr.is_empty() => format!("exited with status {code}"), + Some(code) => format!("exited with status {code}: {stderr}"), + None if stderr.is_empty() => "child exited unsuccessfully".to_string(), + None => format!("child exited unsuccessfully: {stderr}"), + }; + RenderSubprocessError::ChildCrashed(detail) +} + +async fn render_dot_subprocess( + styled_source: &str, + exe_override: Option<&std::path::Path>, +) -> Result, RenderSubprocessError> { + let _permit = GRAPHVIZ_RENDER_SEMAPHORE + .acquire() + .await + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + let exe = render_graph_subprocess_exe(exe_override)?; + let mut cmd = Command::new(exe); + cmd.arg("__render-graph") + .env("FABRO_TELEMETRY", "off") + .env_remove("FABRO_JSON") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cmd + .spawn() + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + 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 { + drop(stdin); + let output = child + .wait_with_output() + .await + .map_err(|wait_err| RenderSubprocessError::SpawnFailed(wait_err.to_string()))?; + return Err(RenderSubprocessError::ChildCrashed(format!( + "failed writing DOT to child stdin: {err}; {}", + render_subprocess_failure(&output.status, &output.stderr) + ))); + } + drop(stdin); + + let output = child + .wait_with_output() + .await + .map_err(|err| RenderSubprocessError::SpawnFailed(err.to_string()))?; + + if !output.status.success() { + return Err(render_subprocess_failure(&output.status, &output.stderr)); + } + + if let Some(error) = output.stdout.strip_prefix(RENDER_ERROR_PREFIX) { + return Err(RenderSubprocessError::RenderFailed( + String::from_utf8_lossy(error).trim().to_string(), + )); + } + + if output.stdout.starts_with(b", +) -> Response { + use fabro_graphviz::render::{inject_dot_style_defaults, postprocess_svg}; + + let styled_source = inject_dot_style_defaults(dot_source); + match render_dot_subprocess(&styled_source, exe_override).await { + Ok(raw) => { + let bytes = postprocess_svg(raw); (StatusCode::OK, [("content-type", "image/svg+xml")], bytes).into_response() } - Ok(Err(e)) => ApiError::new(StatusCode::BAD_REQUEST, e.to_string()).into_response(), - Err(e) => ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response(), + Err(RenderSubprocessError::RenderFailed(err)) => { + ApiError::new(StatusCode::BAD_REQUEST, err).into_response() + } + Err(err) => { + ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() + } } } +pub(crate) async fn render_graph_bytes(dot_source: &str) -> Response { + render_graph_response(dot_source, None).await +} + +#[cfg(test)] +async fn render_graph_bytes_with_exe_override( + dot_source: &str, + exe_override: Option<&std::path::Path>, +) -> Response { + render_graph_response(dot_source, exe_override).await +} + async fn get_graph( _auth: AuthenticatedService, State(state): State>, @@ -6230,6 +6388,10 @@ async fn get_graph( #[cfg(test)] mod tests { + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; + #[cfg(unix)] + use std::path::{Path, PathBuf}; #[cfg(unix)] use std::process::Stdio; @@ -7834,6 +7996,70 @@ slug = "fabro" ); } + #[cfg(unix)] + #[tokio::test] + async fn render_graph_bytes_returns_bad_request_for_render_error_protocol() { + let (_dir, script_path) = write_test_executable( + "#!/bin/sh\nprintf 'RENDER_ERROR:failed to parse DOT source'\nexit 0\n", + ); + + let response = + render_graph_bytes_with_exe_override("not valid dot {{{", Some(&script_path)).await; + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + } + + #[cfg(unix)] + fn write_test_executable(script: &str) -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().expect("temp dir should exist"); + let path = dir.path().join("fake-fabro"); + std::fs::write(&path, script).expect("script should be written"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("script should be executable"); + (dir, path) + } + + #[cfg(unix)] + async fn render_graph_with_override(dot_source: &str, exe_path: &Path) -> Response { + render_graph_bytes_with_exe_override(dot_source, Some(exe_path)).await + } + + #[cfg(unix)] + #[tokio::test] + async fn render_dot_subprocess_returns_child_crashed_for_nonzero_exit() { + let (_dir, script_path) = write_test_executable("#!/bin/sh\nexit 1\n"); + + let result = render_dot_subprocess("digraph { a -> b }", Some(&script_path)).await; + + assert!(matches!( + result, + Err(RenderSubprocessError::ChildCrashed(_)) + )); + } + + #[cfg(unix)] + #[tokio::test] + async fn render_graph_bytes_returns_internal_server_error_for_child_crash() { + let (_dir, script_path) = write_test_executable("#!/bin/sh\nexit 1\n"); + + let response = render_graph_with_override("digraph { a -> b }", &script_path).await; + + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + } + + #[cfg(unix)] + #[tokio::test] + async fn render_dot_subprocess_returns_protocol_violation_for_garbage_stdout() { + let (_dir, script_path) = write_test_executable("#!/bin/sh\nprintf 'garbage'\nexit 0\n"); + + let result = render_dot_subprocess("digraph { a -> b }", Some(&script_path)).await; + + assert!(matches!( + result, + Err(RenderSubprocessError::ProtocolViolation(_)) + )); + } + #[tokio::test] async fn get_graph_not_found() { let app = test_app_with();