From ce1696706c72e09b8a3709c0eabb975dcc4f6f5d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 19:47:12 -0400 Subject: [PATCH] feat(settings): run.goal tagged union (inline | file) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--goal-file` was broken in the v2 path: `TryFrom<&RunArgs> for ConfigLayer` did `let _ = &args.goal_file;`, so clap accepted the flag listed in `--help` and then silently dropped it. Users running `fabro run demo --goal-file prompts/goal.md` ended up with no goal at all (or the DOT graph-level fallback), a regression from the legacy flat `Settings` shape. This commit adds first-class support for both inline and file-sourced goals via a tagged union on `run.goal`. Greenfield decisions: - **Single field, two variants.** `RunGoalLayer` is an untagged enum of `Inline(InterpString)` and `File { file: InterpString }`. Makes `goal XOR goal_file` un-representable in the type system and lets the v2 merge matrix treat `run.goal` as a single scalar (last-writer-wins) instead of needing a custom mutual-exclusion merge rule. Matches the existing `DaytonaDockerfileLayer` pattern. - **Relative paths are anchored at the file that declared them.** `ConfigLayer::load(path)` walks the just-parsed `SettingsFile` and rewrites any literal relative `run.goal.file` path to absolute using `path.parent()` as the base, via new `fabro_config::config::resolve_goal_file_paths`. CLI-sourced paths via `--goal-file` are anchored at CWD in `overrides::goal_layer_from_args`. Env-interpolated paths (`${env.GOALS_DIR}/goal.md`) are left unresolved until consume time and then resolved against the run's working_directory. - **New accessors, no shims.** - `run_goal_layer() -> Option<&RunGoalLayer>` — raw variant access. - `run_goal_inline_str() -> Option` — inline-only, returns `None` for file-sourced goals. - `resolve_run_goal(base_dir) -> Result>` — reads the file from disk if needed, returns text + provenance (`ResolvedGoalSource::Inline | File { path }`). - New `ResolveGoalError` enum covers env-lookup and I/O failures. - Old `run_goal() / run_goal_str()` are **deleted** outright; every call site has been updated to pick the right variant. - **CLI wiring (the actual bug fix).** `overrides::goal_layer_from_args` replaces the two `let _ = &args.goal_file;` lines with real resolution: `(Some(text), None)` → `Inline`, `(None, Some(path))` → `File { file: absolute }`. Both-set is rejected by a helper error and clap already had `conflicts_with = "goal"` as a belt-and- braces check. Applied to both `RunArgs` and `PreflightArgs`. - **Manifest builder.** `resolve_manifest_goal` now calls `args_layer.as_v2().resolve_run_goal()` and `settings.resolve_run_goal()` in precedence order, then falls through to the graph-level `@file` sugar if both are absent. The resolved goal is translated to a `ManifestGoal { text, type_, path }` by a new `resolved_goal_to_manifest` helper — inline goals get `type = Value`, file-sourced goals get `type = File` with the absolute path echoed for provenance. - **Workflow pipeline.** `fabro-workflow::operations::source:: resolve_goal_override` is rewritten to use `resolve_run_goal` against the working_directory. The orphaned helper `resolve_goal_file` (a stub from Stage 4 that was always called with `None`) is deleted. - **Server-side manifest.** `fabro-server::run_manifest:: prepare_manifest` stores the CLI-resolved goal as `RunGoalLayer::Inline`, matching the Stage 4 plan's "CLI owns goal file reads; server never touches the filesystem for goals" contract. ## Tests **Schema** (`fabro-types::settings::accessors`): - `run_goal_inline_str_returns_source_value` — literal inline variant - `run_goal_inline_str_is_none_for_file_variant` — file variant explicitly yields `None` from the inline accessor - `resolve_run_goal_reads_file_variant_from_disk` — end-to-end file read with provenance assertion - `resolve_run_goal_inline_passes_text_through` — inline passthrough **Config load** (`fabro-config::config`): - `parse_accepts_inline_goal` + `parse_accepts_file_variant` - `parse_rejects_goal_with_unknown_sibling_fields` — untagged enum correctly rejects mixed-shape TOML - `combine_replaces_file_goal_with_inline_from_higher_layer` and the reverse — confirms the tagged union merges as a single scalar with no custom rule needed - `load_rewrites_relative_goal_file_to_absolute` - `load_leaves_absolute_goal_file_untouched` - `load_leaves_env_interpolated_goal_file_untouched` **CLI overrides** (`fabro-cli::commands::run::overrides`): - `goal_and_goal_file_together_is_rejected` - `goal_file_is_anchored_at_cwd_when_relative` - `absolute_goal_file_is_preserved` - `inline_goal_builds_inline_variant` - `empty_args_produce_no_goal_layer` **CLI integration** (`fabro-cli::tests::it::cmd::run`): - `dry_run_with_goal_file_reads_contents_into_goal` — end-to-end `fabro run --dry-run --auto-approve --goal-file ` and asserts the file contents appear in the preflight summary. Explicit regression test for the silently-ignored flag. - `dry_run_rejects_goal_and_goal_file_together` — clap conflicts_with ## Callsite churn Every `run_goal() / run_goal_str()` call site updated: - `fabro-config/src/effective_settings.rs` — 2 test assertions → `run_goal_inline_str()` - `fabro-cli/tests/it/cmd/{config,create}.rs` — 3 sites → inline - `fabro-cli/src/manifest_builder.rs` — rewritten to use `resolve_run_goal` - `fabro-workflow/src/operations/create.rs` — 2 sites, test + set - `fabro-workflow/src/operations/source.rs` — rewritten - `fabro-server/src/{run_manifest,server}.rs` — set + test assertion 3,782 workspace tests pass (was 3,765, +17 new). `cargo fmt --check --all` and `cargo clippy --workspace -- -D warnings` are clean. Co-Authored-By: Claude Opus 4.6 (1M context) --- Cargo.lock | 1 + .../fabro-cli/src/commands/run/overrides.rs | 113 ++++++++- lib/crates/fabro-cli/src/manifest_builder.rs | 58 +++-- lib/crates/fabro-cli/tests/it/cmd/config.rs | 4 +- lib/crates/fabro-cli/tests/it/cmd/create.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/run.rs | 56 +++++ lib/crates/fabro-config/src/config.rs | 217 ++++++++++++++++-- .../fabro-config/src/effective_settings.rs | 10 +- lib/crates/fabro-server/src/run_manifest.rs | 9 +- lib/crates/fabro-server/src/server.rs | 2 +- lib/crates/fabro-types/Cargo.toml | 3 + .../fabro-types/src/settings/accessors.rs | 187 ++++++++++++++- lib/crates/fabro-types/src/settings/run.rs | 51 +++- .../fabro-workflow/src/operations/create.rs | 15 +- .../fabro-workflow/src/operations/source.rs | 40 +--- 15 files changed, 665 insertions(+), 103 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9d5fe180b..b023b8c87 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2061,6 +2061,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "tempfile", "toml 0.8.23", "ulid", ] diff --git a/lib/crates/fabro-cli/src/commands/run/overrides.rs b/lib/crates/fabro-cli/src/commands/run/overrides.rs index 8ad84664f..d7b344ea4 100644 --- a/lib/crates/fabro-cli/src/commands/run/overrides.rs +++ b/lib/crates/fabro-cli/src/commands/run/overrides.rs @@ -1,13 +1,15 @@ use std::collections::HashMap; +use std::path::{Path, PathBuf}; -use anyhow::Result; +use anyhow::{Result, anyhow}; use fabro_config::ConfigLayer; use fabro_sandbox::SandboxProvider; use fabro_types::settings::SettingsFile; use fabro_types::settings::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ - ApprovalMode, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunSandboxLayer, + ApprovalMode, RunExecutionLayer, RunGoalLayer, RunLayer, RunMode, RunModelLayer, + RunSandboxLayer, }; use crate::args::{PreflightArgs, RunArgs}; @@ -80,6 +82,41 @@ fn cli_layer_for_verbose(verbose: bool) -> Option { }) } +/// Build the `run.goal` override from the `--goal` / `--goal-file` args. +/// +/// The two are mutually exclusive at the clap level; this helper assumes +/// at most one is set and returns an error if that invariant is violated. +/// +/// CLI-supplied file paths are anchored at `cwd` (where the user invoked +/// the command), matching standard Unix CLI-flag conventions. +fn goal_layer_from_args( + goal: Option<&str>, + goal_file: Option<&Path>, + cwd: &Path, +) -> Result> { + match (goal, goal_file) { + (Some(_), Some(_)) => Err(anyhow!( + "--goal and --goal-file are mutually exclusive; use exactly one" + )), + (Some(text), None) => Ok(Some(RunGoalLayer::Inline(InterpString::parse(text)))), + (None, Some(path)) => { + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + }; + Ok(Some(RunGoalLayer::File { + file: InterpString::parse(&absolute.to_string_lossy()), + })) + } + (None, None) => Ok(None), + } +} + +fn current_dir_or_dot() -> PathBuf { + std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")) +} + impl TryFrom<&RunArgs> for ConfigLayer { type Error = anyhow::Error; @@ -95,8 +132,11 @@ impl TryFrom<&RunArgs> for ConfigLayer { sparse_flag(args.no_retro), ); + let cwd = current_dir_or_dot(); + let goal = goal_layer_from_args(args.goal.as_deref(), args.goal_file.as_deref(), &cwd)?; + let run = RunLayer { - goal: args.goal.as_deref().map(InterpString::parse), + goal, metadata: parse_labels(&args.label), model, sandbox, @@ -104,10 +144,6 @@ impl TryFrom<&RunArgs> for ConfigLayer { ..RunLayer::default() }; - // goal_file is not part of v2; fall through to Settings.goal_file via the bridge. - // Stage 4 consumers that still consult goal_file read it from Settings. - let _ = &args.goal_file; - Ok(Self::from(SettingsFile { run: Some(run), cli: cli_layer_for_verbose(args.verbose), @@ -126,15 +162,16 @@ impl TryFrom<&PreflightArgs> for ConfigLayer { ..RunSandboxLayer::default() }); + let cwd = current_dir_or_dot(); + let goal = goal_layer_from_args(args.goal.as_deref(), args.goal_file.as_deref(), &cwd)?; + let run = RunLayer { - goal: args.goal.as_deref().map(InterpString::parse), + goal, model, sandbox, ..RunLayer::default() }; - let _ = &args.goal_file; // Stage 4 preflight still reads goal_file via Settings bridge. - Ok(Self::from(SettingsFile { run: Some(run), cli: cli_layer_for_verbose(args.verbose), @@ -142,3 +179,59 @@ impl TryFrom<&PreflightArgs> for ConfigLayer { })) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn goal_and_goal_file_together_is_rejected() { + let err = goal_layer_from_args( + Some("inline text"), + Some(Path::new("goal.md")), + Path::new("/tmp"), + ) + .unwrap_err(); + assert!(err.to_string().contains("mutually exclusive")); + } + + #[test] + fn goal_file_is_anchored_at_cwd_when_relative() { + let layer = + goal_layer_from_args(None, Some(Path::new("prompts/goal.md")), Path::new("/cwd")) + .unwrap() + .expect("should build a goal layer"); + let RunGoalLayer::File { file } = layer else { + panic!("expected file variant"); + }; + assert_eq!(file.as_source(), "/cwd/prompts/goal.md"); + } + + #[test] + fn absolute_goal_file_is_preserved() { + let layer = goal_layer_from_args(None, Some(Path::new("/abs/goal.md")), Path::new("/cwd")) + .unwrap() + .expect("should build a goal layer"); + let RunGoalLayer::File { file } = layer else { + panic!("expected file variant"); + }; + assert_eq!(file.as_source(), "/abs/goal.md"); + } + + #[test] + fn inline_goal_builds_inline_variant() { + let layer = goal_layer_from_args(Some("inline goal"), None, Path::new("/cwd")) + .unwrap() + .expect("should build a goal layer"); + assert!(matches!(layer, RunGoalLayer::Inline(_))); + } + + #[test] + fn empty_args_produce_no_goal_layer() { + assert!( + goal_layer_from_args(None, None, Path::new("/cwd")) + .unwrap() + .is_none() + ); + } +} diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 5ea4a7f77..716e77023 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -12,7 +12,7 @@ use fabro_graphviz::parser; use fabro_sandbox::daytona::detect_repo_info; use fabro_types::RunId; use fabro_types::settings::SettingsFile; -use fabro_types::settings::run::DaytonaDockerfileLayer; +use fabro_types::settings::run::{DaytonaDockerfileLayer, ResolvedGoalSource, ResolvedRunGoal}; use fabro_workflow::git::{GitSyncStatus, head_sha, sync_status}; use crate::args::{PreflightArgs, RunArgs}; @@ -391,30 +391,30 @@ fn resolve_manifest_goal( root_dot_path: &Path, cwd: &Path, ) -> Result> { - let _working_directory = project::resolve_working_directory(settings, cwd); + let working_directory = project::resolve_working_directory(settings, cwd); - if let Some(goal) = args_layer + // Precedence 1: CLI args (`--goal` / `--goal-file`). These are already + // resolved to absolute paths by `overrides::goal_layer_from_args`. + if let Some(resolved) = args_layer .as_v2() - .run - .as_ref() - .and_then(|r| r.goal.as_ref()) + .resolve_run_goal(&working_directory) + .context("failed to resolve --goal-file contents")? { - return Ok(Some(types::ManifestGoal { - path: None, - text: goal.as_source(), - type_: types::ManifestGoalType::Value, - })); + return Ok(Some(resolved_goal_to_manifest(resolved))); } - if let Some(goal) = settings.run_goal_str() { - return Ok(Some(types::ManifestGoal { - path: None, - text: goal, - type_: types::ManifestGoalType::Value, - })); - } - // V2 does not carry a distinct `goal_file` field; file-based goals now - // come through workflow manifest layers sourced on the server side. + // Precedence 2: merged config `run.goal`. Config-sourced `goal.file` + // paths were rewritten to absolute by `ConfigLayer::load` at the + // directory of the config file that declared them. + if let Some(resolved) = settings + .resolve_run_goal(&working_directory) + .context("failed to resolve run.goal.file contents")? + { + return Ok(Some(resolved_goal_to_manifest(resolved))); + } + + // Precedence 3: graph-level `goal` attribute in the DOT, with `@file` + // sugar for workflow-colocated goal files. let graph = parser::parse(root_source) .map_err(|err| anyhow!("Failed to parse {}: {err}", root_dot_path.display()))?; let Some(goal) = graph.attrs.get("goal").and_then(AttrValue::as_str) else { @@ -441,6 +441,24 @@ fn resolve_manifest_goal( })) } +/// Translate a [`ResolvedRunGoal`] into the wire-level `ManifestGoal` +/// shape. Inline goals get `type = Value`; file-sourced goals keep their +/// absolute path as the `path` field and use `type = File`. +fn resolved_goal_to_manifest(resolved: ResolvedRunGoal) -> types::ManifestGoal { + match resolved.source { + ResolvedGoalSource::Inline => types::ManifestGoal { + path: None, + text: resolved.text, + type_: types::ManifestGoalType::Value, + }, + ResolvedGoalSource::File { path } => types::ManifestGoal { + path: Some(path.to_string_lossy().into_owned()), + text: resolved.text, + type_: types::ManifestGoalType::File, + }, + } +} + fn build_manifest_git(cwd: &Path) -> Option { let (origin_url, branch) = detect_repo_info(cwd).ok()?; let branch = branch?; diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index cba0507c7..1b7d53a21 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -294,7 +294,7 @@ fn settings_local_merges_cli_and_project_defaults() { let cfg = parse_settings(&output); assert_eq!(cfg.run_model_name_str().as_deref(), Some("project-model")); assert_eq!(cfg.run_model_provider_str().as_deref(), Some("openai")); - assert_eq!(cfg.run_goal_str().as_deref(), None); + assert_eq!(cfg.run_goal_inline_str().as_deref(), None); assert_eq!(cfg.project_directory(), Some("fabro")); // v2 R22: run.inputs replaces the inherited map wholesale rather than @@ -333,7 +333,7 @@ fn settings_local_workflow_name_applies_run_overlay_and_deep_merges() { use fabro_types::settings::run::McpEntryLayer; let cfg = parse_settings(&output); - assert_eq!(cfg.run_goal_str().as_deref(), Some("demo goal")); + assert_eq!(cfg.run_goal_inline_str().as_deref(), Some("demo goal")); assert_eq!(cfg.run_model_name_str().as_deref(), Some("run-model")); assert_eq!(cfg.run_model_provider_str().as_deref(), Some("anthropic")); diff --git a/lib/crates/fabro-cli/tests/it/cmd/create.rs b/lib/crates/fabro-cli/tests/it/cmd/create.rs index e789a7609..b4200f118 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/create.rs @@ -356,7 +356,7 @@ fn create_persists_requested_overrides_into_store() { let compact = json!({ "workflow_slug": run_record.workflow_slug, "settings": { - "goal": settings.run_goal_str(), + "goal": settings.run_goal_inline_str(), "dry_run": settings.dry_run_enabled(), "auto_approve": settings.auto_approve_enabled(), "no_retro": settings.no_retro_enabled(), diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index e1a9c8eed..64ce01c2b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -542,6 +542,62 @@ fn dry_run_simple() { "); } +#[test] +fn dry_run_with_goal_file_reads_contents_into_goal() { + // Regression test for the `--goal-file` flag that was previously + // being silently ignored in the v2 path. The file content must end + // up in the effective goal displayed in the preflight summary. + let context = test_context!(); + + let goal_dir = tempfile::tempdir().unwrap(); + let goal_path = goal_dir.path().join("goal.md"); + std::fs::write(&goal_path, "Ship the rate-limiting feature end to end.\n").unwrap(); + + let mut cmd = context.run_cmd(); + cmd.args(["--dry-run", "--auto-approve", "--goal-file"]); + cmd.arg(&goal_path); + cmd.arg(example_fixture("simple.fabro")); + + let output = cmd.output().expect("run command should execute"); + assert!( + output.status.success(), + "run should succeed:\nstderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("Ship the rate-limiting feature end to end."), + "goal file content should appear in preflight summary, got:\n{stderr}" + ); +} + +#[test] +fn dry_run_rejects_goal_and_goal_file_together() { + // clap `conflicts_with` must fire when both flags are supplied. + let context = test_context!(); + + let goal_dir = tempfile::tempdir().unwrap(); + let goal_path = goal_dir.path().join("goal.md"); + std::fs::write(&goal_path, "never read").unwrap(); + + let mut cmd = context.run_cmd(); + cmd.args(["--dry-run", "--goal", "inline override", "--goal-file"]); + cmd.arg(&goal_path); + cmd.arg(example_fixture("simple.fabro")); + let output = cmd.output().expect("run command should execute"); + assert!( + !output.status.success(), + "run should fail when --goal and --goal-file are both set" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("cannot be used with") + || stderr.contains("conflict") + || stderr.to_lowercase().contains("mutually exclusive"), + "expected conflicts_with error, got:\n{stderr}" + ); +} + #[test] fn dry_run_persists_event_history_in_store() { let context = test_context!(); diff --git a/lib/crates/fabro-config/src/config.rs b/lib/crates/fabro-config/src/config.rs index a6050fa31..15295cb43 100644 --- a/lib/crates/fabro-config/src/config.rs +++ b/lib/crates/fabro-config/src/config.rs @@ -13,6 +13,8 @@ use std::path::Path; use anyhow::Context; +use fabro_types::settings::interp::InterpString; +use fabro_types::settings::run::RunGoalLayer; use fabro_types::settings::{SettingsFile, parse_settings_file as parse_v2_settings_file}; use serde::{Deserialize, Serialize}; @@ -20,6 +22,34 @@ use crate::merge::combine_files; use crate::project::{self}; use crate::user; +/// Rewrite any relative `run.goal = { file = "..." }` path in `file` to an +/// absolute path anchored at `base_dir`. +/// +/// Called from `ConfigLayer::load` so that layers coming from different +/// config files can be merged without losing the "relative to my source +/// file" context. Paths that contain `${env.NAME}` interpolation are left +/// alone (they get resolved against the run's working directory at consume +/// time via [`SettingsFile::resolve_run_goal`]). +fn resolve_goal_file_paths(file: &mut SettingsFile, base_dir: &Path) { + let Some(run) = file.run.as_mut() else { + return; + }; + let Some(RunGoalLayer::File { file: goal_file }) = run.goal.as_mut() else { + return; + }; + if !goal_file.is_literal() { + // Env-tokenized paths stay unresolved until consume time. + return; + } + let literal = goal_file.as_source(); + let path = Path::new(&literal); + if path.is_absolute() { + return; + } + let absolute = base_dir.join(path); + *goal_file = InterpString::parse(&absolute.to_string_lossy()); +} + /// A parsed settings file layer. /// /// Thin newtype around the v2 [`SettingsFile`] parse tree. The newtype @@ -65,10 +95,17 @@ impl ConfigLayer { } /// Load a v2 TOML settings file from disk. + /// + /// Relative `run.goal = { file = "..." }` paths are resolved against + /// the directory of `path` at load time. Subsequent merging with other + /// layers can then safely treat the path as self-contained. pub fn load(path: &Path) -> anyhow::Result { let content = std::fs::read_to_string(path) .with_context(|| format!("Failed to read {}", path.display()))?; - Self::parse(&content) + let mut layer = Self::parse(&content)?; + let base_dir = path.parent().unwrap_or_else(|| Path::new(".")); + resolve_goal_file_paths(&mut layer.file, base_dir); + Ok(layer) } /// Load workflow config + project config for a workflow path. @@ -124,7 +161,7 @@ impl ConfigLayer { #[cfg(test)] mod tests { - use fabro_types::settings::InterpString; + use fabro_types::settings::run::RunGoalLayer; use super::*; @@ -139,7 +176,7 @@ mod tests { } #[test] - fn parse_accepts_minimal_v2_file() { + fn parse_accepts_inline_goal() { let layer = ConfigLayer::parse( r#" _version = 1 @@ -149,17 +186,44 @@ goal = "Do things" ) .unwrap(); assert_eq!( - layer - .file - .run - .as_ref() - .and_then(|r| r.goal.as_ref()) - .map(InterpString::as_source) - .as_deref(), + layer.file.run_goal_inline_str().as_deref(), Some("Do things") ); } + #[test] + fn parse_accepts_file_variant() { + let layer = ConfigLayer::parse( + r#" +_version = 1 +[run.goal] +file = "prompts/goal.md" +"#, + ) + .unwrap(); + let Some(RunGoalLayer::File { file }) = layer.file.run_goal_layer() else { + panic!("expected run.goal.file variant"); + }; + assert_eq!(file.as_source(), "prompts/goal.md"); + } + + #[test] + fn parse_rejects_goal_with_unknown_sibling_fields() { + // The untagged enum should reject any `{ file = ..., extra = ... }` + // shape because neither the inline nor the file variant matches. + let err = ConfigLayer::parse( + r#" +_version = 1 +[run.goal] +file = "prompts/goal.md" +extra = "boom" +"#, + ) + .unwrap_err(); + let text = format!("{err:#}"); + assert!(text.to_lowercase().contains("run.goal") || text.contains("extra")); + } + #[test] fn combine_prefers_higher_precedence_self() { let higher = ConfigLayer::parse( @@ -180,14 +244,133 @@ goal = "lower goal" .unwrap(); let merged = higher.combine(lower); assert_eq!( - merged - .file - .run - .as_ref() - .and_then(|r| r.goal.as_ref()) - .map(InterpString::as_source) - .as_deref(), + merged.file.run_goal_inline_str().as_deref(), Some("higher goal") ); } + + #[test] + fn combine_replaces_file_goal_with_inline_from_higher_layer() { + // A higher-precedence `run.goal = "inline"` must fully override a + // lower layer's `run.goal = { file = "..." }` — the scalar merge + // treats `goal` as one field regardless of which variant each + // layer picked. + let higher = ConfigLayer::parse( + r#" +_version = 1 +[run] +goal = "inline override" +"#, + ) + .unwrap(); + let lower = ConfigLayer::parse( + r#" +_version = 1 +[run.goal] +file = "/tmp/goal.md" +"#, + ) + .unwrap(); + let merged = higher.combine(lower); + assert_eq!( + merged.file.run_goal_inline_str().as_deref(), + Some("inline override") + ); + } + + #[test] + fn combine_replaces_inline_goal_with_file_from_higher_layer() { + let higher = ConfigLayer::parse( + r#" +_version = 1 +[run.goal] +file = "/tmp/goal.md" +"#, + ) + .unwrap(); + let lower = ConfigLayer::parse( + r#" +_version = 1 +[run] +goal = "inline loser" +"#, + ) + .unwrap(); + let merged = higher.combine(lower); + assert!(matches!( + merged.file.run_goal_layer(), + Some(RunGoalLayer::File { .. }) + )); + } + + #[test] + fn load_rewrites_relative_goal_file_to_absolute() { + let tmp = tempfile::tempdir().unwrap(); + let config_path = tmp.path().join("fabro.toml"); + std::fs::write( + &config_path, + r#" +_version = 1 +[run.goal] +file = "prompts/goal.md" +"#, + ) + .unwrap(); + + let layer = ConfigLayer::load(&config_path).unwrap(); + let Some(RunGoalLayer::File { file }) = layer.file.run_goal_layer() else { + panic!("expected file variant"); + }; + let resolved = file.as_source(); + let expected = tmp.path().join("prompts").join("goal.md"); + assert_eq!(resolved, expected.to_string_lossy()); + } + + #[test] + fn load_leaves_absolute_goal_file_untouched() { + let tmp = tempfile::tempdir().unwrap(); + let config_path = tmp.path().join("fabro.toml"); + let abs_goal = "/etc/fabro/goal.md"; + std::fs::write( + &config_path, + format!( + r#" +_version = 1 +[run.goal] +file = "{abs_goal}" +"# + ), + ) + .unwrap(); + + let layer = ConfigLayer::load(&config_path).unwrap(); + let Some(RunGoalLayer::File { file }) = layer.file.run_goal_layer() else { + panic!("expected file variant"); + }; + assert_eq!(file.as_source(), abs_goal); + } + + #[test] + fn load_leaves_env_interpolated_goal_file_untouched() { + // InterpString paths aren't resolved at load time because env + // lookups happen at consume time. The loader should leave them + // alone. + let tmp = tempfile::tempdir().unwrap(); + let config_path = tmp.path().join("fabro.toml"); + std::fs::write( + &config_path, + r#" +_version = 1 +[run.goal] +file = "${env.GOALS_DIR}/goal.md" +"#, + ) + .unwrap(); + + let layer = ConfigLayer::load(&config_path).unwrap(); + let Some(RunGoalLayer::File { file }) = layer.file.run_goal_layer() else { + panic!("expected file variant"); + }; + assert_eq!(file.as_source(), "${env.GOALS_DIR}/goal.md"); + } } diff --git a/lib/crates/fabro-config/src/effective_settings.rs b/lib/crates/fabro-config/src/effective_settings.rs index 840f818f5..c9f59fde6 100644 --- a/lib/crates/fabro-config/src/effective_settings.rs +++ b/lib/crates/fabro-config/src/effective_settings.rs @@ -283,7 +283,10 @@ provider = "openai" ) .unwrap(); - assert_eq!(settings.run_goal_str().as_deref(), Some("workflow goal")); + assert_eq!( + settings.run_goal_inline_str().as_deref(), + Some("workflow goal") + ); assert_eq!( settings.run_model_name_str().as_deref(), Some("workflow-model") @@ -332,7 +335,10 @@ root = "/tmp/should-be-inert" settings.server_storage_root_str().as_deref(), Some("/srv/fabro") ); - assert_eq!(settings.run_goal_str().as_deref(), Some("project goal")); + assert_eq!( + settings.run_goal_inline_str().as_deref(), + Some("project goal") + ); } #[test] diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 24673597b..d2b6aea35 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -21,8 +21,8 @@ use fabro_types::settings::SettingsFile; use fabro_types::settings::cli::{CliLayer, CliOutputLayer, OutputVerbosity}; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ - ApprovalMode, DaytonaDockerfileLayer, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, - RunSandboxLayer, + ApprovalMode, DaytonaDockerfileLayer, RunExecutionLayer, RunGoalLayer, RunLayer, RunMode, + RunModelLayer, RunSandboxLayer, }; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; use fabro_validate::Severity; @@ -91,7 +91,10 @@ pub(crate) fn prepare_manifest_with_mode( )?; if let Some(goal) = manifest.goal.as_ref() { let run = settings.run.get_or_insert_with(RunLayer::default); - run.goal = Some(InterpString::parse(&goal.text)); + // The CLI has already resolved any goal-file reads into + // `manifest.goal.text`, so the server side always stores the + // final text inline. + run.goal = Some(RunGoalLayer::Inline(InterpString::parse(&goal.text))); } Ok(PreparedManifest { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 618ece3ef..d2788dfd7 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -7420,7 +7420,7 @@ level = "debug" // Server-side `dry_run` default must not override the manifest's intent. // Verify a sampling of the persisted v2 settings. assert_eq!( - run_record.settings.run_goal_str().as_deref(), + run_record.settings.run_goal_inline_str().as_deref(), Some("Test"), "goal should be persisted from the manifest" ); diff --git a/lib/crates/fabro-types/Cargo.toml b/lib/crates/fabro-types/Cargo.toml index aed2dee7f..5684113ff 100644 --- a/lib/crates/fabro-types/Cargo.toml +++ b/lib/crates/fabro-types/Cargo.toml @@ -29,3 +29,6 @@ serde_json.workspace = true sha2.workspace = true toml.workspace = true ulid.workspace = true + +[dev-dependencies] +tempfile = "3" diff --git a/lib/crates/fabro-types/src/settings/accessors.rs b/lib/crates/fabro-types/src/settings/accessors.rs index c16671dac..347128d46 100644 --- a/lib/crates/fabro-types/src/settings/accessors.rs +++ b/lib/crates/fabro-types/src/settings/accessors.rs @@ -6,15 +6,15 @@ //! walks the real v2 structure — there is no transitional state here. use std::collections::HashMap; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use super::cli::{CliExecLayer, CliLayer, CliOutputLayer}; use super::interp::InterpString; use super::project::ProjectLayer; use super::run::{ - ApprovalMode, GitAuthorLayer, HookEntry, McpEntryLayer, RunAgentLayer, RunArtifactsLayer, - RunCheckpointLayer, RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunPrepareLayer, - RunPullRequestLayer, RunSandboxLayer, + ApprovalMode, GitAuthorLayer, HookEntry, McpEntryLayer, ResolvedGoalSource, ResolvedRunGoal, + RunAgentLayer, RunArtifactsLayer, RunCheckpointLayer, RunExecutionLayer, RunGoalLayer, + RunLayer, RunMode, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunSandboxLayer, }; use super::server::{ GithubIntegrationLayer, ServerApiLayer, ServerArtifactsLayer, ServerIntegrationsLayer, @@ -43,14 +43,67 @@ impl SettingsFile { self.run.as_ref() } + /// Raw access to the `run.goal` variant (inline or file). #[must_use] - pub fn run_goal(&self) -> Option<&InterpString> { + pub fn run_goal_layer(&self) -> Option<&RunGoalLayer> { self.run.as_ref().and_then(|r| r.goal.as_ref()) } + /// Inline goal text only. Returns `None` when `run.goal` is unset **or** + /// when it's a file-sourced goal — callers that need the file contents + /// should use [`SettingsFile::resolve_run_goal`]. #[must_use] - pub fn run_goal_str(&self) -> Option { - self.run_goal().map(InterpString::as_source) + pub fn run_goal_inline_str(&self) -> Option { + match self.run_goal_layer()? { + RunGoalLayer::Inline(s) => Some(s.as_source()), + RunGoalLayer::File { .. } => None, + } + } + + /// Resolve the `run.goal` layer to its final text, reading a file from + /// disk if necessary. + /// + /// Path resolution: + /// + /// - Absolute paths in the `file` variant are used as-is. + /// - Literal relative paths should already have been rewritten to + /// absolute at config-load time by + /// `fabro_config::resolve_goal_file_paths`. If one reaches this point + /// it will be resolved against `base_dir` as a fallback. + /// - `${env.NAME}` interpolation is resolved via `std::env::var` at + /// call time. Relative paths that survive interpolation are also + /// resolved against `base_dir`. + /// + /// Returns `Ok(None)` when `run.goal` is unset. Returns `Err` when the + /// file variant points at a path that can't be read or has an + /// unresolved env token. + pub fn resolve_run_goal( + &self, + base_dir: &Path, + ) -> Result, ResolveGoalError> { + let Some(layer) = self.run_goal_layer() else { + return Ok(None); + }; + match layer { + RunGoalLayer::Inline(s) => Ok(Some(ResolvedRunGoal { + text: s.as_source(), + source: ResolvedGoalSource::Inline, + })), + RunGoalLayer::File { file } => { + let resolved = file + .resolve(|name| std::env::var(name).ok()) + .map_err(|err| ResolveGoalError::EnvLookup { var: err.name })?; + let path = resolve_goal_file_path(&resolved.value, base_dir); + let text = std::fs::read_to_string(&path).map_err(|err| ResolveGoalError::Io { + path: path.clone(), + source: err, + })?; + Ok(Some(ResolvedRunGoal { + text, + source: ResolvedGoalSource::File { path }, + })) + } + } } #[must_use] @@ -410,21 +463,135 @@ impl SettingsFile { } } +/// Resolve a goal-file path string against `base_dir`. Absolute paths are +/// used as-is; relative paths are joined onto `base_dir`. +fn resolve_goal_file_path(path_str: &str, base_dir: &Path) -> PathBuf { + let path = Path::new(path_str); + if path.is_absolute() { + path.to_path_buf() + } else { + base_dir.join(path) + } +} + +/// Error returned by [`SettingsFile::resolve_run_goal`]. +#[derive(Debug)] +pub enum ResolveGoalError { + /// The `run.goal.file` InterpString referenced an env var that wasn't + /// set at consume time. + EnvLookup { var: String }, + /// The goal file exists in config but could not be read. + Io { + path: PathBuf, + source: std::io::Error, + }, +} + +impl std::fmt::Display for ResolveGoalError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::EnvLookup { var } => write!( + f, + "failed to resolve run.goal.file: env var {var:?} referenced by ${{env.{var}}} is not set" + ), + Self::Io { path, source } => write!( + f, + "failed to read run.goal.file at {}: {source}", + path.display() + ), + } + } +} + +impl std::error::Error for ResolveGoalError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::EnvLookup { .. } => None, + Self::Io { source, .. } => Some(source), + } + } +} + #[cfg(test)] mod tests { use super::*; use crate::settings::run::{RunLayer, RunModelLayer}; #[test] - fn run_goal_str_returns_source_value() { + fn run_goal_inline_str_returns_source_value() { let file = SettingsFile { run: Some(RunLayer { - goal: Some(InterpString::parse("Implement OAuth")), + goal: Some(RunGoalLayer::Inline(InterpString::parse("Implement OAuth"))), ..RunLayer::default() }), ..SettingsFile::default() }; - assert_eq!(file.run_goal_str().as_deref(), Some("Implement OAuth")); + assert_eq!( + file.run_goal_inline_str().as_deref(), + Some("Implement OAuth") + ); + } + + #[test] + fn run_goal_inline_str_is_none_for_file_variant() { + let file = SettingsFile { + run: Some(RunLayer { + goal: Some(RunGoalLayer::File { + file: InterpString::parse("/abs/goal.md"), + }), + ..RunLayer::default() + }), + ..SettingsFile::default() + }; + assert_eq!(file.run_goal_inline_str(), None); + assert!(matches!( + file.run_goal_layer(), + Some(RunGoalLayer::File { .. }) + )); + } + + #[test] + fn resolve_run_goal_reads_file_variant_from_disk() { + let tmp = tempfile::tempdir().unwrap(); + let goal_path = tmp.path().join("goal.md"); + std::fs::write(&goal_path, "ship the thing").unwrap(); + + let file = SettingsFile { + run: Some(RunLayer { + goal: Some(RunGoalLayer::File { + file: InterpString::parse(goal_path.to_str().unwrap()), + }), + ..RunLayer::default() + }), + ..SettingsFile::default() + }; + + let resolved = file + .resolve_run_goal(tmp.path()) + .expect("goal file should resolve") + .expect("goal should be set"); + assert_eq!(resolved.text, "ship the thing"); + assert!(matches!( + resolved.source, + ResolvedGoalSource::File { ref path } if path == &goal_path + )); + } + + #[test] + fn resolve_run_goal_inline_passes_text_through() { + let file = SettingsFile { + run: Some(RunLayer { + goal: Some(RunGoalLayer::Inline(InterpString::parse("literal goal"))), + ..RunLayer::default() + }), + ..SettingsFile::default() + }; + let resolved = file + .resolve_run_goal(std::path::Path::new("/")) + .unwrap() + .unwrap(); + assert_eq!(resolved.text, "literal goal"); + assert_eq!(resolved.source, ResolvedGoalSource::Inline); } #[test] diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index bee9b0409..04246fb30 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -19,7 +19,7 @@ use super::model_ref::ModelRef; #[serde(deny_unknown_fields)] pub struct RunLayer { #[serde(default, skip_serializing_if = "Option::is_none")] - pub goal: Option, + pub goal: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub working_dir: Option, /// Flat string-to-string map. Replaces wholesale across layers. @@ -56,6 +56,55 @@ pub struct RunLayer { pub artifacts: Option, } +/// The source of a run's goal, either inline literal text or a reference to +/// a file on disk. +/// +/// TOML surface: +/// +/// ```toml +/// # Inline form +/// [run] +/// goal = "Diagnose and fix CI build failures" +/// +/// # File form +/// [run.goal] +/// file = "prompts/fix_build.md" +/// ``` +/// +/// Relative paths inside the `file` variant are resolved against the +/// directory of the config file that declared them at load time (see +/// `fabro_config::resolve_goal_file_paths`). `${env.NAME}` interpolation is +/// supported inside the `file` path; env-tokenized relative paths stay +/// unresolved until consume time and are then resolved against the run's +/// effective working directory. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(untagged, deny_unknown_fields)] +pub enum RunGoalLayer { + Inline(InterpString), + File { file: InterpString }, +} + +/// Outcome of resolving a [`RunGoalLayer`] to its final goal text. +/// +/// Carries provenance alongside the text so downstream consumers (e.g. the +/// run manifest builder) can distinguish inline goals from file-sourced +/// goals without having to re-walk the layer. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedRunGoal { + pub text: String, + pub source: ResolvedGoalSource, +} + +/// Provenance of a [`ResolvedRunGoal`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ResolvedGoalSource { + /// Goal text came from a literal `run.goal = "..."` value. + Inline, + /// Goal text was read from a file on disk. The absolute path of that + /// file is carried for provenance / error reporting. + File { path: std::path::PathBuf }, +} + /// `[run.model]` — provider-neutral default model selection. #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index f087698cf..524bd5f58 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -3,7 +3,7 @@ use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_model::{Catalog, Provider}; use fabro_sandbox::SandboxProvider; use fabro_store::Database; -use fabro_types::settings::run::{RunLayer, RunModelLayer}; +use fabro_types::settings::run::{RunGoalLayer, RunLayer, RunModelLayer}; use fabro_types::settings::{InterpString, SettingsFile}; use fabro_types::{RunId, RunProvenance}; use std::collections::BTreeMap; @@ -416,7 +416,7 @@ pub(crate) fn resolve_run_settings(mut settings: SettingsFile, graph: &Graph) -> run.goal = if goal.is_empty() { None } else { - Some(InterpString::parse(&goal)) + Some(RunGoalLayer::Inline(InterpString::parse(&goal))) }; // Strip disabled pull_request entries so downstream consumers can // treat `Some(_)` as "PR creation is on". @@ -559,12 +559,12 @@ mod tests { start -> work -> exit }"#; let validated = validate_dot(dot, { - use fabro_types::settings::run::RunLayer; + use fabro_types::settings::run::{RunGoalLayer, RunLayer}; let mut inputs = std::collections::HashMap::new(); inputs.insert("who".to_string(), toml::Value::String("agent".to_string())); SettingsFile { run: Some(RunLayer { - goal: Some(InterpString::parse("override")), + goal: Some(RunGoalLayer::Inline(InterpString::parse("override"))), inputs: Some(inputs), ..RunLayer::default() }), @@ -766,13 +766,14 @@ mod tests { }, settings: { use fabro_types::settings::run::{ - RunExecutionLayer, RunLayer, RunMode, RunModelLayer, RunPullRequestLayer, + RunExecutionLayer, RunGoalLayer, RunLayer, RunMode, RunModelLayer, + RunPullRequestLayer, }; let mut metadata = HashMap::new(); metadata.insert("env".to_string(), "test".to_string()); SettingsFile { run: Some(RunLayer { - goal: Some(InterpString::parse("override goal")), + goal: Some(RunGoalLayer::Inline(InterpString::parse("override goal"))), metadata, model: Some(RunModelLayer { name: Some(InterpString::parse("sonnet")), @@ -831,7 +832,7 @@ mod tests { .persisted .run_record() .settings - .run_goal_str() + .run_goal_inline_str() .as_deref(), Some("override goal") ); diff --git a/lib/crates/fabro-workflow/src/operations/source.rs b/lib/crates/fabro-workflow/src/operations/source.rs index 9f8206dfc..e70f34d44 100644 --- a/lib/crates/fabro-workflow/src/operations/source.rs +++ b/lib/crates/fabro-workflow/src/operations/source.rs @@ -3,8 +3,7 @@ use std::sync::Arc; use anyhow::Context; use fabro_config::project as project_config; -use fabro_types::settings::{InterpString, SettingsFile}; -use fabro_util::path::expand_tilde; +use fabro_types::settings::SettingsFile; use crate::file_resolver::{FileResolver, FilesystemFileResolver}; use crate::workflow_bundle::BundledWorkflow; @@ -39,25 +38,6 @@ pub(crate) struct ResolvedWorkflow { pub working_directory: PathBuf, } -fn resolve_goal_file( - goal_file: Option<&Path>, - working_directory: &Path, -) -> anyhow::Result> { - let Some(goal_file) = goal_file else { - return Ok(None); - }; - let expanded = expand_tilde(goal_file); - let goal_path = if expanded.is_absolute() { - expanded - } else { - working_directory.join(expanded) - }; - let content = std::fs::read_to_string(&goal_path) - .with_context(|| format!("failed to read goal file: {}", goal_path.display()))?; - tracing::debug!(path = %goal_path.display(), "Goal loaded from file"); - Ok(Some(content)) -} - fn workflow_slug_from_path(workflow_path: &Path) -> Option { let file_name = workflow_path.file_name()?.to_string_lossy(); if workflow_path.extension().is_none() { @@ -132,7 +112,7 @@ pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result< let settings = request.settings; let working_directory = project_config::resolve_working_directory(&settings, &request.cwd); - let goal_override = settings.run_goal().map(InterpString::as_source); + let goal_override = resolve_goal_override(&settings, &working_directory)?; Ok(ResolvedWorkflow { raw_source: workflow.source.clone(), @@ -149,17 +129,18 @@ pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result< } } +/// Resolve the `run.goal` override for a direct (non-manifest) workflow +/// run. Reads the file from disk if the goal layer is the `file` variant. +/// Relative paths that survived config load (e.g. env-interpolated ones) +/// are anchored at `working_directory`. fn resolve_goal_override( settings: &SettingsFile, working_directory: &Path, ) -> anyhow::Result> { - // V2 does not yet carry a separate `goal_file` field; file-based goals - // come through the workflow manifest layer in the server-side flow. - // For direct CLI paths, the goal override comes from `run.goal`. - Ok(settings - .run_goal() - .map(InterpString::as_source) - .or(resolve_goal_file(None, working_directory)?)) + settings + .resolve_run_goal(working_directory) + .map(|opt| opt.map(|resolved| resolved.text)) + .map_err(|err| anyhow::anyhow!(err)) } #[cfg(test)] @@ -168,6 +149,7 @@ mod tests { #[test] fn resolve_workflow_uses_explicit_cwd_for_relative_work_dir() { + use fabro_types::settings::InterpString; use fabro_types::settings::run::RunLayer; let dir = tempfile::tempdir().unwrap();