mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
fix(config): route project/workflow loaders through ConfigLayer::load
The Stage 6 audit caught that `load_project_config` and `load_run_config`
bypassed `ConfigLayer::load` and called `parse_project_config` /
`ConfigLayer::parse` directly. As a result, `resolve_goal_file_paths` —
which rewrites relative `[run.goal] file = "..."` paths to absolute
against the declaring file's directory — only fired for
`~/.fabro/settings.toml`, never for `fabro.toml` or `workflow.toml`.
That meant a project author writing
[run.goal]
file = "prompts/goal.md"
would have the relative path survive all the way to consume time and
get resolved against the run's `working_directory` instead of the
config-file directory, contradicting the agreed "config-file rooted"
rule and breaking the most common case.
Both loaders now delegate to `ConfigLayer::load(path)`, which performs
the load-time rewrite. The user-settings path was already correct.
## Tests
- `load_project_config_rewrites_relative_goal_file_path`
- `load_run_config_rewrites_relative_goal_file_path`
- `load_run_config_leaves_absolute_goal_file_untouched`
- `build_manifest_resolves_relative_goal_file_in_project_config` —
end-to-end via `build_run_manifest`, asserting the absolute path lands
in `manifest.goal.path` and the file contents land in
`manifest.goal.text`.
- `build_manifest_resolves_relative_goal_file_in_workflow_config` — same
shape but exercising `workflow.toml`-declared goal files, which
resolve relative to the much deeper workflow directory rather than
the project root.
3,787 workspace tests pass (was 3,782, +5 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:
parent
2b2f158c16
commit
fcf3152daf
3 changed files with 210 additions and 7 deletions
|
|
@ -660,4 +660,132 @@ mod tests {
|
|||
.contains_key("fabro/workflows/child/workflow.fabro")
|
||||
);
|
||||
}
|
||||
|
||||
/// A relative `[run.goal] file = "..."` declared in `fabro.toml` must
|
||||
/// resolve against the directory of `fabro.toml`, not against the
|
||||
/// invocation cwd. We exercise this by invoking from a subdirectory
|
||||
/// below the project root.
|
||||
#[test]
|
||||
#[allow(unsafe_code, clippy::allow_attributes)]
|
||||
fn build_manifest_resolves_relative_goal_file_in_project_config() {
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
let project = temp.path();
|
||||
let workflow_dir = project.join("fabro/workflows/demo");
|
||||
std::fs::create_dir_all(&workflow_dir).unwrap();
|
||||
std::fs::create_dir_all(project.join("prompts")).unwrap();
|
||||
|
||||
std::fs::write(
|
||||
project.join("fabro.toml"),
|
||||
r#"_version = 1
|
||||
|
||||
[run.goal]
|
||||
file = "prompts/goal.md"
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
std::fs::write(project.join("prompts/goal.md"), "ship from project root").unwrap();
|
||||
|
||||
std::fs::write(
|
||||
workflow_dir.join("workflow.toml"),
|
||||
"_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n",
|
||||
)
|
||||
.unwrap();
|
||||
std::fs::write(
|
||||
workflow_dir.join("workflow.fabro"),
|
||||
r"digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let sandboxed_settings = temp.path().join("empty-settings.toml");
|
||||
std::fs::write(&sandboxed_settings, "_version = 1\n").unwrap();
|
||||
// SAFETY: single-threaded unit test body.
|
||||
unsafe {
|
||||
std::env::set_var("FABRO_CONFIG", &sandboxed_settings);
|
||||
}
|
||||
|
||||
let built = build_run_manifest(ManifestBuildInput {
|
||||
workflow: PathBuf::from("fabro/workflows/demo/workflow.toml"),
|
||||
cwd: project.to_path_buf(),
|
||||
args_layer: ConfigLayer::default(),
|
||||
args: None,
|
||||
run_id: None,
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
// SAFETY: single-threaded unit test body.
|
||||
unsafe {
|
||||
std::env::remove_var("FABRO_CONFIG");
|
||||
}
|
||||
|
||||
let goal = built.manifest.goal.expect("manifest goal should be set");
|
||||
assert_eq!(goal.text, "ship from project root");
|
||||
assert_eq!(goal.type_, types::ManifestGoalType::File);
|
||||
let resolved = goal.path.expect("file goal must carry a path");
|
||||
let expected = project.join("prompts").join("goal.md");
|
||||
assert_eq!(PathBuf::from(resolved), expected);
|
||||
}
|
||||
|
||||
/// A relative `[run.goal] file = "..."` declared in `workflow.toml`
|
||||
/// must resolve against the directory of `workflow.toml`, not against
|
||||
/// the invocation cwd or project root.
|
||||
#[test]
|
||||
#[allow(unsafe_code, clippy::allow_attributes)]
|
||||
fn build_manifest_resolves_relative_goal_file_in_workflow_config() {
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
let project = temp.path();
|
||||
let workflow_dir = project.join("fabro/workflows/demo");
|
||||
std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap();
|
||||
|
||||
std::fs::write(project.join("fabro.toml"), "_version = 1\n").unwrap();
|
||||
std::fs::write(
|
||||
workflow_dir.join("workflow.toml"),
|
||||
r#"_version = 1
|
||||
|
||||
[workflow]
|
||||
graph = "workflow.fabro"
|
||||
|
||||
[run.goal]
|
||||
file = "prompts/goal.md"
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
std::fs::write(
|
||||
workflow_dir.join("prompts/goal.md"),
|
||||
"ship from workflow dir",
|
||||
)
|
||||
.unwrap();
|
||||
std::fs::write(
|
||||
workflow_dir.join("workflow.fabro"),
|
||||
r"digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let sandboxed_settings = temp.path().join("empty-settings.toml");
|
||||
std::fs::write(&sandboxed_settings, "_version = 1\n").unwrap();
|
||||
// SAFETY: single-threaded unit test body.
|
||||
unsafe {
|
||||
std::env::set_var("FABRO_CONFIG", &sandboxed_settings);
|
||||
}
|
||||
|
||||
let built = build_run_manifest(ManifestBuildInput {
|
||||
workflow: PathBuf::from("fabro/workflows/demo/workflow.toml"),
|
||||
cwd: project.to_path_buf(),
|
||||
args_layer: ConfigLayer::default(),
|
||||
args: None,
|
||||
run_id: None,
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
// SAFETY: single-threaded unit test body.
|
||||
unsafe {
|
||||
std::env::remove_var("FABRO_CONFIG");
|
||||
}
|
||||
|
||||
let goal = built.manifest.goal.expect("manifest goal should be set");
|
||||
assert_eq!(goal.text, "ship from workflow dir");
|
||||
assert_eq!(goal.type_, types::ManifestGoalType::File);
|
||||
let resolved = goal.path.expect("file goal must carry a path");
|
||||
let expected = workflow_dir.join("prompts").join("goal.md");
|
||||
assert_eq!(PathBuf::from(resolved), expected);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -33,10 +33,11 @@ pub fn parse_project_config(content: &str) -> anyhow::Result<ConfigLayer> {
|
|||
}
|
||||
|
||||
/// Load a project config from a file path.
|
||||
///
|
||||
/// Goes through [`ConfigLayer::load`] so that relative `run.goal.file`
|
||||
/// paths are anchored at the directory of `path` at load time.
|
||||
pub fn load_project_config(path: &Path) -> anyhow::Result<ConfigLayer> {
|
||||
let content = std::fs::read_to_string(path)
|
||||
.with_context(|| format!("Failed to read {}", path.display()))?;
|
||||
let config = parse_project_config(&content)?;
|
||||
let config = ConfigLayer::load(path).context("Failed to parse project config")?;
|
||||
let root = config
|
||||
.as_v2()
|
||||
.project
|
||||
|
|
@ -477,4 +478,28 @@ retros = true
|
|||
assert_eq!(found_path, tmp.path().join("fabro.toml"));
|
||||
assert_eq!(config.as_v2().version, Some(1));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_project_config_rewrites_relative_goal_file_path() {
|
||||
use fabro_types::settings::run::RunGoalLayer;
|
||||
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let path = tmp.path().join("fabro.toml");
|
||||
fs::write(
|
||||
&path,
|
||||
r#"_version = 1
|
||||
|
||||
[run.goal]
|
||||
file = "prompts/goal.md"
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let config = load_project_config(&path).unwrap();
|
||||
let Some(RunGoalLayer::File { file }) = config.as_v2().run_goal_layer() else {
|
||||
panic!("expected file variant");
|
||||
};
|
||||
let expected = tmp.path().join("prompts").join("goal.md");
|
||||
assert_eq!(file.as_source(), expected.to_string_lossy());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -18,11 +18,10 @@ pub fn parse_run_config(contents: &str) -> anyhow::Result<ConfigLayer> {
|
|||
|
||||
/// Load and parse a run config from a TOML file.
|
||||
///
|
||||
/// Returns the v2-backed `ConfigLayer`.
|
||||
/// Goes through [`ConfigLayer::load`] so that relative `run.goal.file`
|
||||
/// paths are anchored at the directory of `path` at load time.
|
||||
pub fn load_run_config(path: &Path) -> anyhow::Result<ConfigLayer> {
|
||||
let content = std::fs::read_to_string(path)
|
||||
.with_context(|| format!("Failed to read {}", path.display()))?;
|
||||
ConfigLayer::parse(&content)
|
||||
ConfigLayer::load(path)
|
||||
.with_context(|| format!("Failed to parse workflow config at {}", path.display()))
|
||||
}
|
||||
|
||||
|
|
@ -34,3 +33,54 @@ pub fn resolve_graph_path(workflow_toml: &Path, graph_relative: &str) -> PathBuf
|
|||
.unwrap_or_else(|| Path::new("."))
|
||||
.join(graph_relative)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use fabro_types::settings::run::RunGoalLayer;
|
||||
|
||||
#[test]
|
||||
fn load_run_config_rewrites_relative_goal_file_path() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let workflow_dir = tmp.path().join("fabro").join("workflows").join("demo");
|
||||
std::fs::create_dir_all(&workflow_dir).unwrap();
|
||||
let workflow_toml = workflow_dir.join("workflow.toml");
|
||||
std::fs::write(
|
||||
&workflow_toml,
|
||||
r#"_version = 1
|
||||
|
||||
[run.goal]
|
||||
file = "prompts/goal.md"
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let config = load_run_config(&workflow_toml).unwrap();
|
||||
let Some(RunGoalLayer::File { file }) = config.as_v2().run_goal_layer() else {
|
||||
panic!("expected file variant");
|
||||
};
|
||||
let expected = workflow_dir.join("prompts").join("goal.md");
|
||||
assert_eq!(file.as_source(), expected.to_string_lossy());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_run_config_leaves_absolute_goal_file_untouched() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let workflow_toml = tmp.path().join("workflow.toml");
|
||||
std::fs::write(
|
||||
&workflow_toml,
|
||||
r#"_version = 1
|
||||
|
||||
[run.goal]
|
||||
file = "/etc/fabro/goal.md"
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let config = load_run_config(&workflow_toml).unwrap();
|
||||
let Some(RunGoalLayer::File { file }) = config.as_v2().run_goal_layer() else {
|
||||
panic!("expected file variant");
|
||||
};
|
||||
assert_eq!(file.as_source(), "/etc/fabro/goal.md");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue