From 4da1f59695addb6f2f20761f17053e30fc4ca67b Mon Sep 17 00:00:00 2001 From: "brynary-fabro[bot]" <265161896+brynary-fabro[bot]@users.noreply.github.com> Date: Sat, 21 Mar 2026 14:15:13 -0400 Subject: [PATCH] Fix: Workflow TOML config lost in detach mode (#138) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes a bug where workflow TOML configuration (including `[pull_request]` settings) was silently dropped when running in detached mode (`fabro run -d`). The root cause was a three-part failure: `create.rs` checked the raw CLI argument string for a `.toml` extension instead of the resolved path, so `run.toml` was never written to the run directory; `RunEngine` always fell back to `graph.fabro` (a DOT file), causing `prepare_workflow` to return `run_cfg = None` and lose all TOML-level configuration; and `pull_request`/`asset_globs` fields in `RunConfig` had no fallback to `run_defaults` when `run_cfg` was absent. The fix replaces the naive file-copy approach with a proper serialization pipeline. Rather than copying the raw TOML (which would contain a `graph` field pointing to a nonexistent file in the run directory), `create.rs` now calls `write_run_config_snapshot`, which serializes the already-merged `WorkflowRunConfig` and rewrites the `graph` field to `"graph.fabro"` — the canonical cached name. This makes the run directory fully self-contained with all defaults merged, environment variables resolved, and the graph path correct. `RunEngine` in `main.rs` now unconditionally points at `run.toml`; a new `resolve_workflow_source` helper handles the `.toml` path by loading the config and resolving the graph path, with a backward-compatible fallback to `graph.fabro` for older detached runs created before this change. As defense-in-depth, fallbacks to `run_defaults` are added throughout `run.rs` for `pull_request`, `asset_globs`, `devcontainer`, and `sandbox.env` — ensuring bare `.fabro` files passed directly still pick up project-level defaults. Two new unit tests verify the serialization round-trip (confirming `graph` is rewritten and `pull_request` config is preserved) and the missing-`run.toml` fallback behavior. ### Fabro Details
Ran 9 stages in 26m 29s for $9.17 | Stage | Duration | Cost | Retries | |---|---|---|---| | start | 0s | – | 0 | | toolchain | 0s | – | 0 | | preflight_compile | 1m 14s | – | 0 | | preflight_lint | 13s | – | 0 | | implement | 4m 54s | $0.71 | 0 | | simplify_opus | 8m 41s | $1.77 | 0 | | simplify_gpt | 10m 41s | $6.69 | 0 | | verify | 18s | – | 0 | | fmt | 1s | – | 0 | | **Total** | **26m 29s** | **$9.17** | **0** |
Ran ImplementPlan.fabro (12 nodes and 15 edges) ```dot digraph ImplementPlan { graph [ goal="Implement and simplify", model_stylesheet=" * { model: claude-opus-4-6; } " ] rankdir=LR start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] toolchain [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0] preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check -q --workspace 2>&1", max_retries=0] preflight_lint [label="Preflight Lint", shape=parallelogram, script="cargo clippy -q --workspace -- -D warnings 2>&1", max_retries=0] fix_lints [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3] implement [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD."] simplify_opus [label="Simplify (Opus)", prompt="@prompts/simplify.md"] simplify_gpt [label="Simplify (GPT-54)", prompt="@prompts/simplify.md", model="gpt-54"] verify [label="Verify", shape=parallelogram, script="cargo clippy -q --workspace -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1", goal_gate=true, retry_target="fixup"] fixup [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures.", max_visits=3] fmt [label="Format", shape=parallelogram, script="cargo fmt --all 2>&1", max_retries=0] start -> toolchain toolchain -> preflight_compile [condition="outcome=success"] toolchain -> exit preflight_compile -> preflight_lint [condition="outcome=success"] preflight_compile -> exit preflight_lint -> implement [condition="outcome=success"] preflight_lint -> fix_lints fix_lints -> preflight_lint implement -> simplify_opus -> simplify_gpt -> verify verify -> fmt [condition="outcome=success"] verify -> fixup fixup -> verify fmt -> exit } ```
⚒️ Generated with [Fabro](https://fabro.sh) --------- Co-authored-by: Fabro Co-authored-by: Bryan Helmkamp Co-authored-by: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/create.rs | 14 +- lib/crates/fabro-cli/src/commands/run.rs | 142 ++++++++++++++++++-- lib/crates/fabro-cli/src/main.rs | 10 +- lib/crates/fabro-config/src/run.rs | 7 +- 4 files changed, 145 insertions(+), 28 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/create.rs b/lib/crates/fabro-cli/src/commands/create.rs index d9611dd91..f64872786 100644 --- a/lib/crates/fabro-cli/src/commands/create.rs +++ b/lib/crates/fabro-cli/src/commands/create.rs @@ -4,7 +4,7 @@ use chrono::Local; use fabro_config::run::RunDefaults; use fabro_workflows::run_spec::RunSpec; -use super::run::{prepare_workflow, RunArgs}; +use super::run::{cached_graph_path, prepare_workflow, write_run_config_snapshot, RunArgs}; use fabro_util::terminal::Styles; /// Create a workflow run: allocate run directory, persist spec, return (run_id, run_dir). @@ -21,7 +21,7 @@ pub async fn create_run( .as_ref() .ok_or_else(|| anyhow::anyhow!("--workflow is required"))?; - let prep = prepare_workflow(args, run_defaults, styles, quiet)?; + let mut prep = prepare_workflow(args, run_defaults, styles, quiet)?; let goal = prep.graph.goal(); @@ -41,7 +41,7 @@ pub async fn create_run( tokio::fs::create_dir_all(&run_dir).await?; // Write essential files - tokio::fs::write(run_dir.join("graph.fabro"), &prep.source).await?; + tokio::fs::write(cached_graph_path(&run_dir), &prep.source).await?; tokio::fs::write(run_dir.join("id.txt"), &run_id).await?; std::fs::File::create(run_dir.join("progress.jsonl"))?; fabro_workflows::run_status::write_run_status( @@ -50,12 +50,8 @@ pub async fn create_run( None, ); - // Save TOML config alongside the run if present - if workflow_path.extension().is_some_and(|ext| ext == "toml") { - if let Ok(toml_contents) = tokio::fs::read(workflow_path).await { - tokio::fs::write(run_dir.join("run.toml"), toml_contents).await?; - } - } + // Serialize the merged run config so the run dir is self-contained. + write_run_config_snapshot(&run_dir, prep.run_cfg.as_mut()).await?; // Build and save RunSpec let working_directory = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); diff --git a/lib/crates/fabro-cli/src/commands/run.rs b/lib/crates/fabro-cli/src/commands/run.rs index 7faa494bf..aa7ce210c 100644 --- a/lib/crates/fabro-cli/src/commands/run.rs +++ b/lib/crates/fabro-cli/src/commands/run.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::io::IsTerminal; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -444,6 +444,62 @@ fn local_sandbox_with_callback(cwd: PathBuf, emitter: Arc) -> Arc< Arc::new(env) } +pub(crate) const RUN_GRAPH_FILE: &str = "graph.fabro"; +pub(crate) const RUN_CONFIG_FILE: &str = "run.toml"; + +pub(crate) fn cached_graph_path(run_dir: &Path) -> PathBuf { + run_dir.join(RUN_GRAPH_FILE) +} + +pub(crate) fn cached_run_config_path(run_dir: &Path) -> PathBuf { + run_dir.join(RUN_CONFIG_FILE) +} + +fn serialize_run_config_snapshot(run_cfg: &mut WorkflowRunConfig) -> anyhow::Result { + run_cfg.graph = RUN_GRAPH_FILE.to_string(); + toml::to_string_pretty(run_cfg).context("Failed to serialize run config") +} + +pub(crate) async fn write_run_config_snapshot( + run_dir: &Path, + run_cfg: Option<&mut WorkflowRunConfig>, +) -> anyhow::Result<()> { + if let Some(cfg) = run_cfg { + let toml_str = serialize_run_config_snapshot(cfg)?; + tokio::fs::write(cached_run_config_path(run_dir), toml_str).await?; + } + Ok(()) +} + +fn is_missing_cached_run_config(path: &Path, error: &anyhow::Error) -> bool { + path.starts_with(fabro_workflows::run_lookup::default_runs_base()) + && error + .root_cause() + .downcast_ref::() + .is_some_and(|io| io.kind() == std::io::ErrorKind::NotFound) +} + +fn resolve_workflow_source( + workflow_path: &Path, +) -> anyhow::Result<(PathBuf, Option)> { + let path = project_config::resolve_workflow_arg(workflow_path)?; + if path.extension().is_some_and(|ext| ext == "toml") { + match run_config::load_run_config(&path) { + Ok(cfg) => { + let dot = run_config::resolve_graph_path(&path, &cfg.graph); + Ok((dot, Some(cfg))) + } + // Backward compatibility for detached runs created before run.toml existed. + Err(err) if is_missing_cached_run_config(&path, &err) => { + Ok((path.with_file_name(RUN_GRAPH_FILE), None)) + } + Err(err) => Err(err), + } + } else { + Ok((path, None)) + } +} + /// Result of workflow preparation (shared between `create` and `run` commands). pub(crate) struct PreparedWorkflow { pub source: String, @@ -480,7 +536,7 @@ pub(crate) fn prepare_workflow( // Resolve workflow arg, load run config if TOML, apply defaults let (dot_path, run_cfg) = { - let (dot, cfg) = project_config::resolve_workflow(workflow_path)?; + let (dot, cfg) = resolve_workflow_source(workflow_path)?; match cfg { Some(mut cfg) => { cfg.apply_defaults(&run_defaults); @@ -610,7 +666,7 @@ pub async fn run_command( let PreparedWorkflow { source, graph, - run_cfg, + mut run_cfg, sandbox_provider, model, provider, @@ -680,7 +736,7 @@ pub async fn run_command( tokio::fs::create_dir_all(&run_dir).await?; fabro_util::run_log::activate(&run_dir.join("cli.log")) .context("Failed to activate per-run log")?; - tokio::fs::write(run_dir.join("graph.fabro"), &source).await?; + tokio::fs::write(cached_graph_path(&run_dir), &source).await?; tokio::fs::write(run_dir.join("run.pid"), std::process::id().to_string()).await?; fabro_workflows::run_status::write_run_status( &run_dir, @@ -698,10 +754,14 @@ pub async fn run_command( ); }); - if workflow_path.extension().is_some_and(|ext| ext == "toml") { - if let Ok(toml_contents) = tokio::fs::read(workflow_path).await { - tokio::fs::write(run_dir.join("run.toml"), toml_contents).await?; - } + // Serialize the merged run config so the run dir is self-contained. + // env refs (${env.VARNAME}) are still unresolved at this point, so + // plaintext secrets are never written to disk. + write_run_config_snapshot(&run_dir, run_cfg.as_mut()).await?; + + // Now resolve ${env.VARNAME} references for runtime use. + if let Some(ref mut cfg) = run_cfg { + run_config::resolve_sandbox_env(cfg)?; } // Create progress UI (used for both normal and verbose modes) @@ -969,6 +1029,7 @@ pub async fn run_command( let devcontainer_config = if run_cfg .as_ref() .and_then(|c| c.sandbox.as_ref()) + .or(run_defaults.sandbox.as_ref()) .and_then(|s| s.devcontainer) .unwrap_or(false) { @@ -1300,6 +1361,7 @@ pub async fn run_command( if let Some(toml_env) = run_cfg .as_ref() .and_then(|c| c.sandbox.as_ref()) + .or(run_defaults.sandbox.as_ref()) .and_then(|s| s.env.clone()) { env.extend(toml_env); @@ -1424,11 +1486,13 @@ pub async fn run_command( pull_request: run_cfg .as_ref() .and_then(|c| c.pull_request.as_ref()) + .or(run_defaults.pull_request.as_ref()) .filter(|p| p.enabled) .cloned(), asset_globs: run_cfg .as_ref() .and_then(|c| c.assets.as_ref()) + .or(run_defaults.assets.as_ref()) .map(|a| a.include.clone()) .unwrap_or_default(), workflow_slug: workflow_slug.clone(), @@ -1841,7 +1905,10 @@ async fn run_from_branch( // Read graph DOT from metadata branch let source = fabro_workflows::git::MetadataStore::read_graph_dot(&original_cwd, &run_id)? .ok_or_else(|| { - anyhow::anyhow!("no graph.fabro found on metadata branch for run {run_id}") + anyhow::anyhow!( + "no {} found on metadata branch for run {run_id}", + RUN_GRAPH_FILE + ) })?; // If --pipeline was also provided, use it instead (allows overriding) @@ -1885,7 +1952,7 @@ async fn run_from_branch( tokio::fs::create_dir_all(&run_dir).await?; fabro_util::run_log::activate(&run_dir.join("cli.log")) .context("Failed to activate per-run log")?; - tokio::fs::write(run_dir.join("graph.fabro"), &source).await?; + tokio::fs::write(cached_graph_path(&run_dir), &source).await?; let base_sha = fabro_workflows::git::MetadataStore::read_manifest(&original_cwd, &run_id)? .and_then(|m| m.base_sha); @@ -2784,6 +2851,61 @@ fn build_event_envelope( mod tests { use super::*; + #[test] + fn serialize_run_config_snapshot_rewrites_graph_path() { + let cfg = run_config::WorkflowRunConfig { + version: 1, + goal: Some("test".to_string()), + graph: "workflow.fabro".to_string(), + work_dir: None, + llm: None, + setup: None, + sandbox: None, + vars: None, + hooks: Vec::new(), + checkpoint: Default::default(), + pull_request: Some(run_config::PullRequestConfig { + enabled: true, + ..Default::default() + }), + assets: None, + mcp_servers: Default::default(), + github: None, + }; + + let pr = cfg.pull_request.clone(); + let mut cfg = cfg; + let serialized = serialize_run_config_snapshot(&mut cfg).unwrap(); + let reparsed = run_config::parse_run_config(&serialized).unwrap(); + + assert_eq!(reparsed.graph, RUN_GRAPH_FILE); + assert_eq!(reparsed.pull_request, pr); + } + + #[test] + fn resolve_workflow_source_falls_back_to_graph_for_missing_cached_run_config() { + // Place the test dir inside the runs base so the fallback is allowed. + let runs_base = fabro_workflows::run_lookup::default_runs_base(); + std::fs::create_dir_all(&runs_base).unwrap(); + let dir = tempfile::tempdir_in(&runs_base).unwrap(); + std::fs::write(dir.path().join(RUN_GRAPH_FILE), "digraph test {}").unwrap(); + + let (dot_path, run_cfg) = + resolve_workflow_source(&dir.path().join(RUN_CONFIG_FILE)).unwrap(); + + assert_eq!(dot_path, dir.path().join(RUN_GRAPH_FILE)); + assert!(run_cfg.is_none()); + } + + #[test] + fn resolve_workflow_source_errors_for_missing_run_toml_outside_runs_dir() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(RUN_GRAPH_FILE), "digraph test {}").unwrap(); + + let result = resolve_workflow_source(&dir.path().join(RUN_CONFIG_FILE)); + assert!(result.is_err()); + } + #[test] fn apply_goal_override_cli_wins_over_toml() { use fabro_graphviz::graph::{AttrValue, Graph}; diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index a1f13478c..da8f5b864 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -724,13 +724,9 @@ async fn main_inner() -> (String, Result<()>) { ) })?; - // Use the cached graph snapshot instead of the original file - let cached_graph = run_dir.join("graph.fabro"); - let workflow_path = if cached_graph.exists() { - cached_graph - } else { - spec.workflow_path - }; + // Prefer the cached run.toml. prepare_workflow() falls back to the + // sibling graph snapshot for older detached runs that predate run.toml. + let workflow_path = commands::run::cached_run_config_path(&run_dir); let run_args = commands::run::RunArgs { workflow: Some(workflow_path), diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index 3b4b33543..04bc39ee0 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -299,6 +299,10 @@ impl RunDefaults { /// The `graph` path in the returned config is resolved relative to the /// TOML file's parent directory. Any `dockerfile = { path = "..." }` is /// resolved to inline content. +/// +/// `${env.VARNAME}` references in `[sandbox.env]` are NOT resolved here — +/// call [`resolve_sandbox_env`] separately after snapshotting, so that +/// plaintext secrets are never written to disk. pub fn load_run_config(path: &Path) -> anyhow::Result { let contents = std::fs::read_to_string(path) .with_context(|| format!("Failed to read {}", path.display()))?; @@ -306,7 +310,6 @@ pub fn load_run_config(path: &Path) -> anyhow::Result { let config_dir = path.parent().unwrap_or(Path::new(".")); resolve_dockerfile(&mut config, config_dir)?; - resolve_sandbox_env(&mut config)?; Ok(config) } @@ -315,7 +318,7 @@ pub fn load_run_config(path: &Path) -> anyhow::Result { /// /// Only whole-value references are supported (no partial interpolation). /// Missing host env vars produce a hard error. -fn resolve_sandbox_env(config: &mut WorkflowRunConfig) -> anyhow::Result<()> { +pub fn resolve_sandbox_env(config: &mut WorkflowRunConfig) -> anyhow::Result<()> { if let Some(env) = config.sandbox.as_mut().and_then(|s| s.env.as_mut()) { resolve_env_refs(env)?; }