mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-11 22:53:00 +00:00
Fix: Workflow TOML config lost in detach mode (#138)
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
<details>
<summary>Ran 9 stages in 26m 29s for $9.17</summary>
| 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** |
</details>
<details>
<summary>Ran <code>ImplementPlan.fabro</code> (12 nodes and 15
edges)</summary>
```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
}
```
</details>
⚒️ Generated with [Fabro](https://fabro.sh)
---------
Co-authored-by: Fabro <noreply@fabro.sh>
Co-authored-by: Bryan Helmkamp <bryan@brynary.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
dca1e45121
commit
9e676796ea
4 changed files with 145 additions and 28 deletions
|
|
@ -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("."));
|
||||
|
|
|
|||
|
|
@ -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<EventEmitter>) -> 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<String> {
|
||||
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::<std::io::Error>()
|
||||
.is_some_and(|io| io.kind() == std::io::ErrorKind::NotFound)
|
||||
}
|
||||
|
||||
fn resolve_workflow_source(
|
||||
workflow_path: &Path,
|
||||
) -> anyhow::Result<(PathBuf, Option<WorkflowRunConfig>)> {
|
||||
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};
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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<WorkflowRunConfig> {
|
||||
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<WorkflowRunConfig> {
|
|||
|
||||
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<WorkflowRunConfig> {
|
|||
///
|
||||
/// 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)?;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue