mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-06 02:48:25 +00:00
refactor(types): demote cli.target.* and run.working_dir to plain String (D11)
The rule refinement: a field is InterpString iff it is in one of the five
categories (command/script/headers/env/url) AND resolved at the run
boundary — the only point where vars/secrets/inputs exist. cli.target.* is
consumed at CLI connect time, where resolving {{ vars.* }} would require
querying the server whose address is the value being resolved; its
consumers only ever leaked raw source, so no working behavior is removed.
run.working_dir is the deliberate exception called out in review: it DOES
cross the run boundary and its {{ vars.* }} substitution worked; it is
demoted on the category test alone (a path, not category content). The
removal warns at resolve time via warn_if_demoted_template, like the
other demoted fields.
Server-scope fields (storage.root, listen.unix.path, S3, github.*,
api/web.url) are deliberately NOT demoted here: their {{ env.* }}
resolution works today via fabro-server/src/interp.rs and shipped
artifacts depend on it (docker/split-web/settings.toml uses
{{ env.FABRO_WEB_URL }}); see PR discussion.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
a3a70a1517
commit
1afa06a4a0
14 changed files with 61 additions and 57 deletions
|
|
@ -222,17 +222,11 @@ fn value_at_path<'a>(document: &'a toml::Value, path: &[&str]) -> Option<&'a tom
|
|||
|
||||
/// Pull the resolved CLI target configuration out of `[cli.target]`.
|
||||
/// Returns either an http(s) URL or a unix socket path.
|
||||
#[expect(
|
||||
clippy::disallowed_methods,
|
||||
reason = "known leak: cli.target.* is a url/connection field that should resolve {{ env.* }} \
|
||||
tokens but consumes them raw today; strict resolution scheduled in the \
|
||||
interpolation unification (Phase 2 keep-rows)"
|
||||
)]
|
||||
fn cli_target_from_settings(settings: &CliNamespace) -> Option<String> {
|
||||
let target = settings.target.as_ref()?;
|
||||
match target {
|
||||
CliTargetSettings::Http { url } => Some(url.as_source()),
|
||||
CliTargetSettings::Unix { path } => Some(path.as_source()),
|
||||
CliTargetSettings::Http { url } => Some(url.clone()),
|
||||
CliTargetSettings::Unix { path } => Some(path.clone()),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,5 @@
|
|||
//! Sparse `[cli]` settings layer definitions.
|
||||
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::cli::{CliAuthStrategy, OutputFormat, OutputVerbosity};
|
||||
use fabro_types::settings::run::AgentPermissions;
|
||||
use serde::{Deserialize, Serialize};
|
||||
|
|
@ -32,11 +31,11 @@ pub struct CliLayer {
|
|||
pub enum CliTargetLayer {
|
||||
Http {
|
||||
#[serde(default)]
|
||||
url: Option<InterpString>,
|
||||
url: Option<String>,
|
||||
},
|
||||
Unix {
|
||||
#[serde(default)]
|
||||
path: Option<InterpString>,
|
||||
path: Option<String>,
|
||||
},
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ pub struct RunLayer {
|
|||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub goal: Option<RunGoalLayer>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub working_dir: Option<InterpString>,
|
||||
pub working_dir: Option<String>,
|
||||
/// Flat string-to-string map. Replaces wholesale across layers.
|
||||
#[serde(default, skip_serializing_if = "ReplaceMap::is_empty")]
|
||||
pub metadata: ReplaceMap<String>,
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@
|
|||
use std::fmt::Write;
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use fabro_types::settings::{InterpString, RunNamespace};
|
||||
use fabro_types::settings::RunNamespace;
|
||||
use serde::Serialize;
|
||||
|
||||
use crate::{Error, Result, WorkflowSettingsBuilder, run};
|
||||
|
|
@ -144,14 +144,8 @@ fn sibling_workflow_toml_for(graph: &Path) -> Option<PathBuf> {
|
|||
(toml_graph == graph).then_some(candidate)
|
||||
}
|
||||
|
||||
#[expect(
|
||||
clippy::disallowed_methods,
|
||||
reason = "known leak: run.working_dir is a path kept as InterpString (not demoted); it should \
|
||||
resolve {{ env.* }}/{{ vars.* }} tokens but consumes them raw today; strict \
|
||||
resolution scheduled in the interpolation unification (Phase 2 keep-rows)"
|
||||
)]
|
||||
pub fn resolve_working_directory_from_run(run: &RunNamespace, caller_cwd: &Path) -> PathBuf {
|
||||
let Some(work_dir) = run.working_dir.as_ref().map(InterpString::as_source) else {
|
||||
let Some(work_dir) = run.working_dir.as_deref() else {
|
||||
return caller_cwd.to_path_buf();
|
||||
};
|
||||
let path = PathBuf::from(work_dir);
|
||||
|
|
@ -557,7 +551,7 @@ directory = "../custom"
|
|||
let cwd = Path::new("/tmp/workspace");
|
||||
let resolved = resolve_working_directory_from_run(
|
||||
&RunNamespace {
|
||||
working_dir: Some(InterpString::parse("repo")),
|
||||
working_dir: Some("repo".to_string()),
|
||||
..RunNamespace::default()
|
||||
},
|
||||
cwd,
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ use fabro_types::settings::cli::{
|
|||
CliLoggingSettings, CliNamespace, CliOutputSettings, CliTargetSettings, CliUpdatesSettings,
|
||||
};
|
||||
|
||||
use super::{ResolveError, require_interp};
|
||||
use super::{ResolveError, require_string};
|
||||
use crate::{CliExecLayer, CliLayer, CliTargetLayer};
|
||||
|
||||
pub fn resolve_cli(layer: &CliLayer, errors: &mut Vec<ResolveError>) -> CliNamespace {
|
||||
|
|
@ -46,12 +46,18 @@ fn resolve_target(
|
|||
errors: &mut Vec<ResolveError>,
|
||||
) -> Option<CliTargetSettings> {
|
||||
match target {
|
||||
Some(CliTargetLayer::Http { url }) => Some(CliTargetSettings::Http {
|
||||
url: require_interp(url.as_ref(), "cli.target.url", errors),
|
||||
}),
|
||||
Some(CliTargetLayer::Unix { path }) => Some(CliTargetSettings::Unix {
|
||||
path: require_interp(path.as_ref(), "cli.target.path", errors),
|
||||
}),
|
||||
Some(CliTargetLayer::Http { url }) => {
|
||||
super::warn_if_demoted_template("cli.target.http.url", url.as_deref());
|
||||
Some(CliTargetSettings::Http {
|
||||
url: require_string(url.as_ref(), "cli.target.url", errors),
|
||||
})
|
||||
}
|
||||
Some(CliTargetLayer::Unix { path }) => {
|
||||
super::warn_if_demoted_template("cli.target.unix.path", path.as_deref());
|
||||
Some(CliTargetSettings::Unix {
|
||||
path: require_string(path.as_ref(), "cli.target.path", errors),
|
||||
})
|
||||
}
|
||||
None => None,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -29,6 +29,19 @@ pub(crate) fn require_interp(
|
|||
})
|
||||
}
|
||||
|
||||
pub(crate) fn require_string(
|
||||
value: Option<&String>,
|
||||
path: &str,
|
||||
errors: &mut Vec<ResolveError>,
|
||||
) -> String {
|
||||
value.cloned().unwrap_or_else(|| {
|
||||
errors.push(ResolveError::Missing {
|
||||
path: path.to_string(),
|
||||
});
|
||||
String::new()
|
||||
})
|
||||
}
|
||||
|
||||
#[expect(
|
||||
clippy::disallowed_methods,
|
||||
reason = "parsed_value special case: the TCP listen address parses the literal source \
|
||||
|
|
|
|||
|
|
@ -49,6 +49,8 @@ pub fn resolve_run(
|
|||
});
|
||||
}
|
||||
|
||||
super::warn_if_demoted_template("run.working_dir", layer.working_dir.as_deref());
|
||||
|
||||
RunNamespace {
|
||||
goal: resolve_goal(layer.goal.as_ref()),
|
||||
working_dir: layer.working_dir.clone(),
|
||||
|
|
|
|||
|
|
@ -3,7 +3,6 @@
|
|||
reason = "sync test fixture setup; not on a Tokio path"
|
||||
)]
|
||||
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::cli::{CliTargetSettings, OutputFormat, OutputVerbosity};
|
||||
use fabro_types::settings::run::AgentPermissions;
|
||||
use temp_env::with_var;
|
||||
|
|
@ -42,7 +41,7 @@ url = "https://config.example.com"
|
|||
assert_eq!(
|
||||
user_settings.cli.target,
|
||||
Some(CliTargetSettings::Http {
|
||||
url: InterpString::parse("https://config.example.com"),
|
||||
url: "https://config.example.com".to_string(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
|
@ -80,10 +79,6 @@ fn user_settings_resolve_returns_defaults_when_default_settings_file_is_missing(
|
|||
});
|
||||
}
|
||||
|
||||
#[expect(
|
||||
clippy::disallowed_methods,
|
||||
reason = "test asserts the raw template source"
|
||||
)]
|
||||
#[test]
|
||||
fn resolves_cli_target_exec_and_output_settings() {
|
||||
let cli = UserSettingsBuilder::from_toml(
|
||||
|
|
@ -125,7 +120,7 @@ level = "debug"
|
|||
let CliTargetSettings::Http { url } = cli.target.expect("target") else {
|
||||
panic!("expected http target");
|
||||
};
|
||||
assert_eq!(url.as_source(), "https://config.example.com");
|
||||
assert_eq!(url, "https://config.example.com");
|
||||
|
||||
assert!(cli.exec.prevent_idle_sleep);
|
||||
assert_eq!(cli.exec.model.provider.as_deref(), Some("openai"));
|
||||
|
|
|
|||
|
|
@ -586,9 +586,10 @@ name = "sonnet"
|
|||
}
|
||||
other => panic!("expected file goal, got {other:?}"),
|
||||
}
|
||||
// run.working_dir is demoted (D11): the env token stays literal text.
|
||||
assert_eq!(
|
||||
settings.working_dir,
|
||||
Some(InterpString::parse("{{ env.FABRO_WORKDIR }}"))
|
||||
settings.working_dir.as_deref(),
|
||||
Some("{{ env.FABRO_WORKDIR }}")
|
||||
);
|
||||
assert_eq!(settings.model.provider, Some("anthropic".to_string()));
|
||||
assert_eq!(settings.model.name, Some("sonnet".to_string()));
|
||||
|
|
|
|||
|
|
@ -1785,7 +1785,7 @@ mod runs {
|
|||
goal: Some(RunGoal::Inline(InterpString::parse(
|
||||
"Add rate limiting to auth endpoints",
|
||||
))),
|
||||
working_dir: Some(InterpString::parse("/workspace/api-server")),
|
||||
working_dir: Some("/workspace/api-server".to_string()),
|
||||
model: RunModelSettings {
|
||||
provider: Some("anthropic".to_string()),
|
||||
name: Some("claude-opus-4-6".to_string()),
|
||||
|
|
|
|||
|
|
@ -9,7 +9,6 @@ use std::collections::HashMap;
|
|||
|
||||
use serde::{Deserialize, Serialize};
|
||||
|
||||
use super::interp::InterpString;
|
||||
use super::run::{AgentPermissions, McpServerSettings};
|
||||
|
||||
/// A structurally resolved `[cli]` view for consumers.
|
||||
|
|
@ -26,8 +25,8 @@ pub struct CliNamespace {
|
|||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
#[serde(tag = "type", rename_all = "lowercase")]
|
||||
pub enum CliTargetSettings {
|
||||
Http { url: InterpString },
|
||||
Unix { path: InterpString },
|
||||
Http { url: String },
|
||||
Unix { path: String },
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@ use super::size::Size;
|
|||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub struct RunNamespace {
|
||||
pub goal: Option<RunGoal>,
|
||||
pub working_dir: Option<InterpString>,
|
||||
pub working_dir: Option<String>,
|
||||
pub metadata: HashMap<String, String>,
|
||||
pub inputs: HashMap<String, toml::Value>,
|
||||
pub model: RunModelSettings,
|
||||
|
|
@ -82,10 +82,10 @@ impl RunNamespace {
|
|||
F: FnMut(&str) -> Option<String>,
|
||||
{
|
||||
substitute_goal(&mut self.goal, &mut lookup)?;
|
||||
substitute_option(&mut self.working_dir, &mut lookup)?;
|
||||
substitute_string_map(&mut self.metadata, &mut lookup)?;
|
||||
// run.model.provider/name and run.git.author.* were demoted to plain
|
||||
// `String` and removed from this pass (D2): values stay literal.
|
||||
// run.working_dir, run.model.provider/name, and run.git.author.* were
|
||||
// demoted to plain `String` and removed from this pass (D2/D11):
|
||||
// values stay literal.
|
||||
substitute_option_string(&mut self.model.controls.reasoning_effort, &mut lookup)?;
|
||||
substitute_option_string(&mut self.model.controls.speed, &mut lookup)?;
|
||||
substitute_string_vec(&mut self.checkpoint.exclude_globs, &mut lookup)?;
|
||||
|
|
@ -317,7 +317,6 @@ mod run_namespace_variable_substitution_tests {
|
|||
goal: Some(RunGoal::Inline(InterpString::parse(
|
||||
"deploy {{ vars.ENV }} in {{ env.REGION }}",
|
||||
))),
|
||||
working_dir: Some(InterpString::parse("/workspace/{{ vars.ENV }}")),
|
||||
prepare: RunPrepareSettings {
|
||||
commands: vec!["echo {{ vars.ENV }} {{ env.REGION }}".to_string()],
|
||||
timeout_ms: 1_000,
|
||||
|
|
@ -376,10 +375,6 @@ mod run_namespace_variable_substitution_tests {
|
|||
goal_source,
|
||||
Some("deploy prod in {{ env.REGION }}".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
run.working_dir.as_ref().map(InterpString::as_source),
|
||||
Some("/workspace/prod".to_string())
|
||||
);
|
||||
assert_eq!(run.prepare.commands, vec![
|
||||
"echo prod {{ env.REGION }}".to_string()
|
||||
]);
|
||||
|
|
@ -408,10 +403,12 @@ mod run_namespace_variable_substitution_tests {
|
|||
|
||||
#[test]
|
||||
fn demoted_fields_do_not_interpolate() {
|
||||
// Demoted fields (run.model.*, run.git.author.*, run.scm.owner/
|
||||
// repository) were removed from the vars pass (D2): `{{ vars.* }}`
|
||||
// and `{{ env.* }}` stay literal even when a value is available.
|
||||
// Demoted fields (run.working_dir, run.model.*, run.git.author.*,
|
||||
// run.scm.owner/repository) were removed from the vars pass (D2/D11):
|
||||
// `{{ vars.* }}` and `{{ env.* }}` stay literal even when a value is
|
||||
// available.
|
||||
let mut run = RunNamespace {
|
||||
working_dir: Some("/workspace/{{ vars.ENV }}".to_string()),
|
||||
model: super::RunModelSettings {
|
||||
provider: Some("{{ vars.PROVIDER }}".to_string()),
|
||||
name: Some("{{ vars.MODEL }}".to_string()),
|
||||
|
|
@ -435,6 +432,10 @@ mod run_namespace_variable_substitution_tests {
|
|||
run.substitute_variables(|_| Some("SUBSTITUTED".to_string()))
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(
|
||||
run.working_dir.as_deref(),
|
||||
Some("/workspace/{{ vars.ENV }}")
|
||||
);
|
||||
assert_eq!(run.model.provider.as_deref(), Some("{{ vars.PROVIDER }}"));
|
||||
assert_eq!(run.model.name.as_deref(), Some("{{ vars.MODEL }}"));
|
||||
let author = run.git.author.as_ref().unwrap();
|
||||
|
|
|
|||
|
|
@ -1276,7 +1276,7 @@ mod tests {
|
|||
},
|
||||
settings: settings_from_run_layer({
|
||||
RunLayer {
|
||||
working_dir: Some(InterpString::parse("workspace")),
|
||||
working_dir: Some("workspace".to_string()),
|
||||
execution: Some(RunExecutionLayer {
|
||||
mode: Some(RunMode::DryRun),
|
||||
..RunExecutionLayer::default()
|
||||
|
|
|
|||
|
|
@ -126,11 +126,12 @@ fn resolve_goal_override(
|
|||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use fabro_types::settings::InterpString;
|
||||
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn resolve_workflow_uses_explicit_cwd_for_relative_work_dir() {
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::run::RunNamespace;
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
|
|
@ -141,7 +142,7 @@ mod tests {
|
|||
},
|
||||
settings: WorkflowSettings {
|
||||
run: RunNamespace {
|
||||
working_dir: Some(InterpString::parse("workspace")),
|
||||
working_dir: Some("workspace".to_string()),
|
||||
..RunNamespace::default()
|
||||
},
|
||||
..WorkflowSettings::default()
|
||||
|
|
@ -155,7 +156,6 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn resolve_workflow_reads_goal_override_from_dense_run_settings() {
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::run::{RunGoal, RunNamespace};
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue