scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-12 23:02:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

feat(settings): run.goal tagged union (inline | file)

Browse source 
`--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<String>` — inline-only, returns
    `None` for file-sourced goals.
  - `resolve_run_goal(base_dir) -> Result<Option<ResolvedRunGoal>>` —
    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:🇮🇹:cmd::run`):
- `dry_run_with_goal_file_reads_contents_into_goal` — end-to-end
  `fabro run --dry-run --auto-approve --goal-file <path>` 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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-09 19:47:12 -04:00
parent fac0b10244
commit ce1696706c
No known key found for this signature in database
15 changed files with 665 additions and 103 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -2061,6 +2061,7 @@ dependencies = [
"serde",
"serde_json",
"sha2",
"tempfile",
"toml 0.8.23",
"ulid",
]

113
lib/crates/fabro-cli/src/commands/run/overrides.rs
View file

@ -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<CliLayer> {
})
}
/// 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<Option<RunGoalLayer>> {
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()
);
}
}

58
lib/crates/fabro-cli/src/manifest_builder.rs
View file

@ -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<Option<types::ManifestGoal>> {
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<types::ManifestGit> {
let (origin_url, branch) = detect_repo_info(cwd).ok()?;
let branch = branch?;

4
lib/crates/fabro-cli/tests/it/cmd/config.rs
View file

@ -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"));

2
lib/crates/fabro-cli/tests/it/cmd/create.rs
View file

@ -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(),

56
lib/crates/fabro-cli/tests/it/cmd/run.rs
View file

@ -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!();

217
lib/crates/fabro-config/src/config.rs
View file

@ -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<Self> {
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");
}
}

10
lib/crates/fabro-config/src/effective_settings.rs
View file

@ -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]

9
lib/crates/fabro-server/src/run_manifest.rs
View file

@ -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 {

2
lib/crates/fabro-server/src/server.rs
View file

@ -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"
);

3
lib/crates/fabro-types/Cargo.toml
View file

@ -29,3 +29,6 @@ serde_json.workspace = true
sha2.workspace = true
toml.workspace = true
ulid.workspace = true
[dev-dependencies]
tempfile = "3"

187
lib/crates/fabro-types/src/settings/accessors.rs
View file

@ -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<String> {
self.run_goal().map(InterpString::as_source)
pub fn run_goal_inline_str(&self) -> Option<String> {
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<Option<ResolvedRunGoal>, 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]

51
lib/crates/fabro-types/src/settings/run.rs
View file

@ -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<InterpString>,
pub goal: Option<RunGoalLayer>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub working_dir: Option<InterpString>,
/// Flat string-to-string map. Replaces wholesale across layers.
@ -56,6 +56,55 @@ pub struct RunLayer {
pub artifacts: Option<RunArtifactsLayer>,
}
/// 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)]

15
lib/crates/fabro-workflow/src/operations/create.rs
View file

@ -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")
);

40
lib/crates/fabro-workflow/src/operations/source.rs
View file

@ -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<Option<String>> {
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<String> {
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<Option<String>> {
// 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();