scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-06 02:48:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

refactor(types): demote non-category leak fields to plain String

Browse source 
run.model.provider/name, cli.exec.model.provider/name, run.git.author.*,
and run.scm.owner/repository are identifiers/commit content — not in the
gist's command/script/headers/env/url InterpString categories — so they
become plain `String` (layer + resolved structs) with NO interpolation.

Principle: only `InterpString` fields access variables. These fields are
dropped from the variable substitute pass entirely, so `{{ vars.* }}` and
`{{ env.* }}` are both treated as literal text. This removes an incidental
behavior — run-scoped plain-`String` fields used to get `{{ vars.* }}`
substituted via the String pass (a "lucky accident"), and `env` always
leaked literally. Removing it makes variable access deliberate and typed
rather than accidental and bug-prone; if these fields should support
variables later, that will be a controlled InterpString decision.

A `tracing::warn!` fires at resolve time when a demoted run.* field still
contains `{{ ... }}`, so anyone who relied on the old incidental
substitution gets a visible notice instead of a silent change.

Consumers updated from `as_source()` to direct `String` access, and the
foundation's `#[expect(disallowed_methods, ... demotion ...)]` annotations
for these fields are removed (no longer InterpString). Behavior-reducing
second slice of the interpolation unification; stacks on the foundation
(#472).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-06-09 13:26:35 -04:00
parent 548c1574d2
commit fd057d0198
23 changed files with 154 additions and 241 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
lib/crates/fabro-checkpoint/src/author.rs
View file

@ -1,7 +1,6 @@
use std::fmt::Write;
use fabro_config::GitAuthorLayer;
use fabro_types::settings::InterpString;
use fabro_types::settings::run::GitAuthorSettings;
/// Resolved git author identity for checkpoint commits.
@ -51,30 +50,14 @@ impl GitAuthor {
}
}
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; run.git.author.* is slated for demotion to plain String in the \
interpolation unification (D2)"
)]
impl From<&GitAuthorLayer> for GitAuthor {
fn from(value: &GitAuthorLayer) -> Self {
Self::from_options(
value.name.as_ref().map(InterpString::as_source),
value.email.as_ref().map(InterpString::as_source),
)
Self::from_options(value.name.clone(), value.email.clone())
}
}
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; run.git.author.* is slated for demotion to plain String in the \
interpolation unification (D2)"
)]
impl From<&GitAuthorSettings> for GitAuthor {
fn from(value: &GitAuthorSettings) -> Self {
Self::from_options(
value.name.as_ref().map(InterpString::as_source),
value.email.as_ref().map(InterpString::as_source),
)
Self::from_options(value.name.clone(), value.email.clone())
}
}

15
lib/crates/fabro-cli/src/commands/exec.rs
View file

@ -16,7 +16,6 @@ use fabro_llm::types::{
};
use fabro_mcp::config::McpServerSettings;
use fabro_model::ProviderId;
use fabro_types::settings::InterpString;
use fabro_types::settings::cli::OutputFormat as SettingsOutputFormat;
use fabro_util::exit::{self, ErrorExt, ExitClass};
use futures::stream;
@ -276,11 +275,6 @@ impl ProviderAdapter for AuthenticatedFabroServerAdapter {
}
}
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; cli.exec.model.* is slated for demotion to plain String in the \
interpolation unification (D2)"
)]
pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResult<()> {
use fabro_agent::cli::PermissionLevel as AgentPermissionLevel;
use fabro_types::settings::run::AgentPermissions;
@ -288,13 +282,8 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu
let cli = &ctx.user_settings().cli;
#[cfg(feature = "sleep_inhibitor")]
let _sleep_guard = sleep_inhibitor::guard(cli.exec.prevent_idle_sleep);
let provider_str = cli
.exec
.model
.provider
.as_ref()
.map(InterpString::as_source);
let model_str = cli.exec.model.name.as_ref().map(InterpString::as_source);
let provider_str = cli.exec.model.provider.clone();
let model_str = cli.exec.model.name.clone();
let permissions = cli.exec.agent.permissions.map(|p| match p {
AgentPermissions::ReadOnly => AgentPermissionLevel::ReadOnly,
AgentPermissions::ReadWrite => AgentPermissionLevel::ReadWrite,

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

@ -411,8 +411,8 @@ fn create_persists_requested_overrides_into_store() {
"dry_run": resolved_run.execution.mode == fabro_types::settings::run::RunMode::DryRun,
"auto_approve": resolved_run.execution.approval == fabro_types::settings::run::ApprovalMode::Auto,
"llm": {
"model": resolved_run.model.name.as_ref().map(fabro_types::settings::InterpString::as_source),
"provider": resolved_run.model.provider.as_ref().map(fabro_types::settings::InterpString::as_source),
"model": resolved_run.model.name.clone(),
"provider": resolved_run.model.provider.clone(),
},
"environment": {
"id": resolved_run.environment.id,

29
lib/crates/fabro-config/src/builders.rs
View file

@ -651,7 +651,6 @@ fn finish_dense_result<T>(
mod tests {
use std::collections::HashMap;
use fabro_types::settings::InterpString;
use fabro_types::settings::cli::OutputVerbosity;
use fabro_types::settings::run::{ApprovalMode, EnvironmentProvider, RunMode};
@ -694,10 +693,6 @@ command = ["demo-mcp"]
);
}
#[expect(
clippy::disallowed_methods,
reason = "test asserts the raw template source"
)]
#[test]
fn workflow_builder_preserves_run_overrides_when_cli_overrides_are_added() {
let settings = WorkflowSettingsBuilder::new()
@ -705,8 +700,8 @@ command = ["demo-mcp"]
.run_overrides(RunLayer {
metadata: ReplaceMap::from(HashMap::from([("env".to_string(), "cli".to_string())])),
model: Some(RunModelLayer {
provider: Some(InterpString::parse("openai")),
name: Some(InterpString::parse("gpt-5")),
provider: Some("openai".to_string()),
name: Some("gpt-5".to_string()),
fallbacks: Vec::new(),
controls: None,
}),
@ -730,24 +725,8 @@ command = ["demo-mcp"]
settings.run.metadata.get("env").map(String::as_str),
Some("cli")
);
assert_eq!(
settings
.run
.model
.provider
.as_ref()
.map(InterpString::as_source),
Some("openai".to_string())
);
assert_eq!(
settings
.run
.model
.name
.as_ref()
.map(InterpString::as_source),
Some("gpt-5".to_string())
);
assert_eq!(settings.run.model.provider.as_deref(), Some("openai"));
assert_eq!(settings.run.model.name.as_deref(), Some("gpt-5"));
assert_eq!(settings.run.execution.mode, RunMode::DryRun);
assert_eq!(settings.run.execution.approval, ApprovalMode::Auto);
}

4
lib/crates/fabro-config/src/layers/cli.rs
View file

@ -87,11 +87,11 @@ pub struct CliExecModelLayer {
/// LLM provider for `fabro exec`.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[option(value_type = "string")]
pub provider: Option<InterpString>,
pub provider: Option<String>,
/// Model name for `fabro exec`.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[option(value_type = "string")]
pub name: Option<InterpString>,
pub name: Option<String>,
}
#[derive(

12
lib/crates/fabro-config/src/layers/run.rs
View file

@ -142,11 +142,11 @@ pub struct RunModelLayer {
/// Provider name for workflow model selection.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[option(value_type = "string")]
pub provider: Option<InterpString>,
pub provider: Option<String>,
/// Model name for workflow runs.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[option(value_type = "string")]
pub name: Option<InterpString>,
pub name: Option<String>,
/// Ordered list of fallback model references. Supports `...` splice marker
/// at layering time — see [`super::splice_array`].
#[serde(default, skip_serializing_if = "Vec::is_empty")]
@ -236,11 +236,11 @@ pub struct GitAuthorLayer {
/// Git author name for checkpoint commits.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[option(default = "\"fabro\"", value_type = "string")]
pub name: Option<InterpString>,
pub name: Option<String>,
/// Git author email for checkpoint commits.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[option(default = "\"fabro@local\"", value_type = "string")]
pub email: Option<InterpString>,
pub email: Option<String>,
}
/// `[run.prepare]` — ordered list of preparation steps. Whole list replaces
@ -537,9 +537,9 @@ pub struct RunScmLayer {
#[serde(default, skip_serializing_if = "Option::is_none")]
pub provider: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub owner: Option<InterpString>,
pub owner: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub repository: Option<InterpString>,
pub repository: Option<String>,
/// Provider-specific SCM leaves. First-pass providers.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub github: Option<ScmGitHubLayer>,

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

@ -59,6 +59,16 @@ fn resolve_target(
fn resolve_exec(exec: Option<&CliExecLayer>) -> CliExecSettings {
let exec = exec.expect("defaults.toml should provide cli.exec defaults");
let model = exec.model.as_ref();
super::warn_if_demoted_template(
"cli.exec.model.provider",
model.and_then(|model| model.provider.as_deref()),
);
super::warn_if_demoted_template(
"cli.exec.model.name",
model.and_then(|model| model.name.as_deref()),
);
CliExecSettings {
prevent_idle_sleep: exec
.prevent_idle_sleep

16
lib/crates/fabro-config/src/resolve/mod.rs
View file

@ -56,6 +56,22 @@ pub(crate) fn default_interp(path: impl AsRef<std::path::Path>) -> InterpString
InterpString::parse(&path.as_ref().to_string_lossy())
}
/// Warn when a field demoted out of the interpolation set (D2) still contains
/// claimed template tokens. These fields are plain `String` now — `{{ vars.*
/// }}` (which previously substituted via the run-scoped String pass, a
/// now-removed accident) and `{{ env.* }}` are both treated as literal text.
/// Only `InterpString` fields interpolate. Unclaimed `{{ ... }}` text (jq
/// programs, Go templates) never interpolated and does not warn.
pub(crate) fn warn_if_demoted_template(field: &str, value: Option<&str>) {
if value.is_some_and(|value| !InterpString::parse(value).is_literal()) {
tracing::warn!(
field = %field,
"this field no longer interpolates template tokens and uses the value literally; it \
was demoted to a plain string in the interpolation unification"
);
}
}
#[cfg(test)]
mod tests {
use std::collections::HashMap;

16
lib/crates/fabro-config/src/resolve/run.rs
View file

@ -108,6 +108,9 @@ fn resolve_model(model: Option<&RunModelLayer>) -> RunModelSettings {
return RunModelSettings::default();
};
super::warn_if_demoted_template("run.model.provider", model.provider.as_deref());
super::warn_if_demoted_template("run.model.name", model.name.as_deref());
RunModelSettings {
provider: model.provider.clone(),
name: model.name.clone(),
@ -133,9 +136,13 @@ fn resolve_model(model: Option<&RunModelLayer>) -> RunModelSettings {
fn resolve_git(git: Option<&RunGitLayer>) -> RunGitSettings {
RunGitSettings {
author: git.and_then(|git| {
git.author.as_ref().map(|author| GitAuthorSettings {
name: author.name.clone(),
email: author.email.clone(),
git.author.as_ref().map(|author| {
super::warn_if_demoted_template("run.git.author.name", author.name.as_deref());
super::warn_if_demoted_template("run.git.author.email", author.email.as_deref());
GitAuthorSettings {
name: author.name.clone(),
email: author.email.clone(),
}
})
}),
}
@ -492,6 +499,9 @@ fn resolve_scm(scm: Option<&RunScmLayer>) -> RunScmSettings {
return RunScmSettings::default();
};
super::warn_if_demoted_template("run.scm.owner", scm.owner.as_deref());
super::warn_if_demoted_template("run.scm.repository", scm.repository.as_deref());
RunScmSettings {
provider: scm.provider.clone(),
owner: scm.owner.clone(),

14
lib/crates/fabro-config/src/tests/resolve_cli.rs
View file

@ -128,18 +128,8 @@ level = "debug"
assert_eq!(url.as_source(), "https://config.example.com");
assert!(cli.exec.prevent_idle_sleep);
assert_eq!(
cli.exec
.model
.provider
.as_ref()
.map(InterpString::as_source),
Some("openai".to_string())
);
assert_eq!(
cli.exec.model.name.as_ref().map(InterpString::as_source),
Some("gpt-5".to_string())
);
assert_eq!(cli.exec.model.provider.as_deref(), Some("openai"));
assert_eq!(cli.exec.model.name.as_deref(), Some("gpt-5"));
assert_eq!(cli.exec.agent.permissions, Some(AgentPermissions::ReadOnly));
assert_eq!(cli.exec.agent.mcps["fs"].name, "fs");
assert_eq!(cli.output.format, OutputFormat::Json);

20
lib/crates/fabro-config/src/tests/resolve_root.rs
View file

@ -1,4 +1,3 @@
use fabro_types::settings::InterpString;
use fabro_types::settings::run::RunMode;
use crate::{ServerSettingsBuilder, SettingsLayer};
@ -118,23 +117,10 @@ name = "gpt-5"
assert_eq!(workflow_settings.workflow.graph, "graphs/workflow.dot");
assert_eq!(server.server.storage.root.as_source(), "/srv/fabro");
assert_eq!(
workflow_settings
.run
.model
.provider
.as_ref()
.map(InterpString::as_source),
Some("openai".to_string())
);
assert_eq!(
workflow_settings
.run
.model
.name
.as_ref()
.map(InterpString::as_source),
Some("gpt-5".to_string())
workflow_settings.run.model.provider.as_deref(),
Some("openai")
);
assert_eq!(workflow_settings.run.model.name.as_deref(), Some("gpt-5"));
}
#[test]

7
lib/crates/fabro-config/src/tests/resolve_run.rs
View file

@ -590,11 +590,8 @@ name = "sonnet"
settings.working_dir,
Some(InterpString::parse("{{ env.FABRO_WORKDIR }}"))
);
assert_eq!(
settings.model.provider,
Some(InterpString::parse("anthropic"))
);
assert_eq!(settings.model.name, Some(InterpString::parse("sonnet")));
assert_eq!(settings.model.provider, Some("anthropic".to_string()));
assert_eq!(settings.model.name, Some("sonnet".to_string()));
}
mod run_integrations_github_permissions {

21
lib/crates/fabro-manifest/src/lib.rs
View file

@ -74,8 +74,8 @@ pub fn build_run_overrides(input: RunOverrideInput<'_>) -> RunLayer {
.goal
.map(|goal| RunGoalLayer::Inline(InterpString::parse(goal)));
let model = (input.model.is_some() || input.provider.is_some()).then(|| RunModelLayer {
provider: input.provider.map(InterpString::parse),
name: input.model.map(InterpString::parse),
provider: input.provider.map(String::from),
name: input.model.map(String::from),
fallbacks: Vec::new(),
controls: None,
});
@ -769,11 +769,6 @@ fn build_git_context(
})
}
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; run.scm.owner/repository are slated for demotion to plain String in the \
interpolation unification (D2)"
)]
fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option<String> {
let scm = &settings.run.scm;
if !scm
@ -783,8 +778,8 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option<String> {
{
return None;
}
let owner = scm.owner.as_ref()?.as_source();
let repository = scm.repository.as_ref()?.as_source();
let owner = scm.owner.clone()?;
let repository = scm.repository.clone()?;
if owner.trim().is_empty() || repository.trim().is_empty() {
return None;
}
@ -902,10 +897,6 @@ mod tests {
)]))
}
#[expect(
clippy::disallowed_methods,
reason = "test asserts the raw template source"
)]
#[test]
fn build_run_overrides_sets_common_cli_and_mcp_layers() {
let overrides = build_run_overrides(RunOverrideInput {
@ -932,7 +923,7 @@ mod tests {
.name
.as_ref()
.unwrap()
.as_source(),
.as_str(),
"gpt-5.4-mini"
);
assert_eq!(
@ -943,7 +934,7 @@ mod tests {
.provider
.as_ref()
.unwrap()
.as_source(),
.as_str(),
"openai"
);
assert_eq!(

4
lib/crates/fabro-server/src/demo/mod.rs
View file

@ -1787,8 +1787,8 @@ mod runs {
))),
working_dir: Some(InterpString::parse("/workspace/api-server")),
model: RunModelSettings {
provider: Some(InterpString::parse("anthropic")),
name: Some(InterpString::parse("claude-opus-4-6")),
provider: Some("anthropic".to_string()),
name: Some("claude-opus-4-6".to_string()),
..RunModelSettings::default()
},
prepare: RunPrepareSettings {

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

@ -1142,31 +1142,19 @@ fn canonical_provider_id(catalog: &Catalog, provider_name: &str) -> ProviderId {
.map_or(provider_id, |provider| provider.id.clone())
}
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; run.model.name/provider are slated for demotion to plain String in the \
interpolation unification (D2)"
)]
fn resolve_model_provider(
settings: &RunNamespace,
_graph: &Graph,
configured_providers: &[ProviderId],
catalog: &Catalog,
) -> (String, Option<String>) {
let provider = settings
.model
.provider
.as_ref()
.map(InterpString::as_source);
let model = settings.model.name.as_ref().map_or_else(
|| {
catalog
.default_for_configured_ids(configured_providers)
.id
.clone()
},
InterpString::as_source,
);
let provider = settings.model.provider.clone();
let model = settings.model.name.clone().unwrap_or_else(|| {
catalog
.default_for_configured_ids(configured_providers)
.id
.clone()
});
match catalog.get(&model) {
Some(info) => (

5
lib/crates/fabro-server/src/serve.rs
View file

@ -222,7 +222,6 @@ pub struct ServeArgs {
}
fn serve_overrides(args: &ServeArgs) -> (Option<RunLayer>, Option<ServerLayer>) {
use fabro_types::settings::interp::InterpString;
let mut run = RunLayer::default();
let mut server = ServerLayer::default();
if args.web || args.no_web {
@ -231,11 +230,11 @@ fn serve_overrides(args: &ServeArgs) -> (Option<RunLayer>, Option<ServerLayer>)
}
if let Some(ref model) = args.model {
let model_layer = run.model.get_or_insert_with(RunModelLayer::default);
model_layer.name = Some(InterpString::parse(model));
model_layer.name = Some(model.clone());
}
if let Some(ref provider) = args.provider {
let model_layer = run.model.get_or_insert_with(RunModelLayer::default);
model_layer.provider = Some(InterpString::parse(provider));
model_layer.provider = Some(provider.clone());
}
if let Some(environment) = args.environment.as_ref() {
let environment_layer = run

7
lib/crates/fabro-server/src/server/tests.rs
View file

@ -13534,12 +13534,7 @@ level = "debug"
"run execution mode should inherit from server settings"
);
assert_eq!(
resolved_run
.model
.name
.as_ref()
.map(fabro_types::settings::InterpString::as_source)
.as_deref(),
resolved_run.model.name.as_deref(),
Some("claude-sonnet-4-5"),
);

4
lib/crates/fabro-types/src/settings/cli.rs
View file

@ -44,8 +44,8 @@ pub struct CliExecSettings {
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
pub struct CliExecModelSettings {
pub provider: Option<InterpString>,
pub name: Option<InterpString>,
pub provider: Option<String>,
pub name: Option<String>,
}
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]

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

@ -84,14 +84,11 @@ impl RunNamespace {
substitute_goal(&mut self.goal, &mut lookup)?;
substitute_option(&mut self.working_dir, &mut lookup)?;
substitute_string_map(&mut self.metadata, &mut lookup)?;
substitute_option(&mut self.model.provider, &mut lookup)?;
substitute_option(&mut self.model.name, &mut lookup)?;
// run.model.provider/name and run.git.author.* are plain `String`,
// demoted out of the interpolation set (D2): NO variable substitution.
// Only InterpString fields access variables — these don't anymore.
substitute_option_string(&mut self.model.controls.reasoning_effort, &mut lookup)?;
substitute_option_string(&mut self.model.controls.speed, &mut lookup)?;
if let Some(author) = &mut self.git.author {
substitute_option(&mut author.name, &mut lookup)?;
substitute_option(&mut author.email, &mut lookup)?;
}
substitute_string_vec(&mut self.checkpoint.exclude_globs, &mut lookup)?;
substitute_environment(&mut self.environment, &mut lookup)?;
substitute_map(&mut self.environment.env, &mut lookup)?;
@ -107,8 +104,8 @@ impl RunNamespace {
substitute_option(&mut slack.channel, &mut lookup)?;
}
substitute_map(&mut self.integrations.github.permissions, &mut lookup)?;
substitute_option(&mut self.scm.owner, &mut lookup)?;
substitute_option(&mut self.scm.repository, &mut lookup)?;
// run.scm.owner/repository are plain `String` (demoted, D2): no
// variable substitution.
substitute_string_vec(&mut self.prepare.commands, &mut lookup)?;
for mcp in self.agent.mcps.values_mut() {
substitute_string(&mut mcp.name, &mut lookup)?;
@ -182,9 +179,7 @@ where
if !may_reference_variable(value) {
return Ok(());
}
if InterpString::parse(value).references(Namespace::Vars) {
*value = InterpString::substitute_variables_in_str(value, lookup)?;
}
*value = InterpString::substitute_variables_in_str(value, lookup)?;
Ok(())
}
@ -412,6 +407,47 @@ mod run_namespace_variable_substitution_tests {
}
}
#[test]
fn demoted_fields_do_not_interpolate() {
// run.model.*, run.git.author.*, run.scm.owner/repository were demoted
// out of the interpolation set (D2) to plain `String`. They no longer
// access variables at all: `{{ vars.* }}` and `{{ env.* }}` are both
// left literal even when a value is available. Only InterpString fields
// interpolate — the old run-scoped String `vars` substitution on these
// fields was an incidental behavior that is now removed.
let mut run = RunNamespace {
model: super::RunModelSettings {
provider: Some("{{ vars.PROVIDER }}".to_string()),
name: Some("{{ vars.MODEL }}".to_string()),
..super::RunModelSettings::default()
},
git: super::RunGitSettings {
author: Some(super::GitAuthorSettings {
name: Some("{{ vars.AUTHOR }}".to_string()),
email: Some("{{ env.EMAIL }}".to_string()),
}),
},
scm: super::RunScmSettings {
owner: Some("{{ vars.OWNER }}".to_string()),
repository: Some("{{ env.REPO }}".to_string()),
..super::RunScmSettings::default()
},
..RunNamespace::default()
};
// Even with every variable available, demoted fields stay literal.
run.substitute_variables(|_| Some("SUBSTITUTED".to_string()))
.unwrap();
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();
assert_eq!(author.name.as_deref(), Some("{{ vars.AUTHOR }}"));
assert_eq!(author.email.as_deref(), Some("{{ env.EMAIL }}"));
assert_eq!(run.scm.owner.as_deref(), Some("{{ vars.OWNER }}"));
assert_eq!(run.scm.repository.as_deref(), Some("{{ env.REPO }}"));
}
#[test]
fn substitutes_variables_in_string_backed_settings_families() {
let mut run = RunNamespace {
@ -576,8 +612,8 @@ pub enum RunGoal {
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
pub struct RunModelSettings {
pub provider: Option<InterpString>,
pub name: Option<InterpString>,
pub provider: Option<String>,
pub name: Option<String>,
pub fallbacks: Vec<ModelRef>,
/// Run-level default values for typed model controls
/// (`reasoning_effort`, `speed`). Node and style attributes still win
@ -599,8 +635,8 @@ pub struct RunGitSettings {
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
pub struct GitAuthorSettings {
pub name: Option<InterpString>,
pub email: Option<InterpString>,
pub name: Option<String>,
pub email: Option<String>,
}
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
@ -1214,8 +1250,8 @@ impl HookDefinition {
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
pub struct RunScmSettings {
pub provider: Option<String>,
pub owner: Option<InterpString>,
pub repository: Option<InterpString>,
pub owner: Option<String>,
pub repository: Option<String>,
pub github: Option<ScmGitHubSettings>,
}

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

@ -1156,7 +1156,7 @@ mod tests {
goal: Some(RunGoalLayer::Inline(InterpString::parse("override goal"))),
metadata: ReplaceMap::from(metadata),
model: Some(RunModelLayer {
name: Some(InterpString::parse("sonnet")),
name: Some("sonnet".to_string()),
..RunModelLayer::default()
}),
pull_request: Some(RunPullRequestLayer {
@ -1207,8 +1207,6 @@ mod tests {
.run
.model
.name
.as_ref()
.map(fabro_types::settings::InterpString::as_source)
.as_deref(),
Some("claude-sonnet-4-6")
);
@ -1220,8 +1218,6 @@ mod tests {
.run
.model
.provider
.as_ref()
.map(fabro_types::settings::InterpString::as_source)
.as_deref(),
Some("anthropic")
);

21
lib/crates/fabro-workflow/src/operations/start.rs
View file

@ -21,7 +21,7 @@ use fabro_types::settings::run::{
RunModelSettings as ResolvedRunModelSettings, RunNamespace as ResolvedRunSettings,
TlsMode as ResolvedTlsMode,
};
use fabro_types::settings::{InterpString, ModelRegistry, ResolvedModelRef};
use fabro_types::settings::{ModelRegistry, ResolvedModelRef};
use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind};
use fabro_vault::Vault;
use tokio::runtime::Handle;
@ -560,25 +560,20 @@ fn resolve_docker_config(settings: &ResolvedRunSettings) -> DockerSandboxOptions
docker_config_from_environment(&settings.environment, !settings.clone.enabled)
}
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; run.model.name/provider are slated for demotion to plain String in the \
interpolation unification (D2)"
)]
fn resolve_start_llm(
catalog: &Catalog,
configured: &[ProviderId],
settings: &ResolvedRunSettings,
) -> Result<ResolvedStartLlm, Error> {
let model = settings.model.name.as_ref().map_or_else(
|| catalog.default_for_configured_ids(configured).id.clone(),
InterpString::as_source,
);
let model = settings
.model
.name
.clone()
.unwrap_or_else(|| catalog.default_for_configured_ids(configured).id.clone());
let provider = settings
.model
.provider
.as_ref()
.map(InterpString::as_source)
.clone()
.filter(|value| !value.is_empty());
let default_provider_id = catalog
@ -1302,7 +1297,7 @@ reasoning = false
.unwrap();
let catalog = Catalog::from_builtin_with_overrides(&overrides).unwrap();
let mut settings = ResolvedRunSettings::default();
settings.model.name = Some(InterpString::parse("ac"));
settings.model.name = Some("ac".to_string());
let resolved = resolve_start_llm(&catalog, &[], &settings).unwrap();

23
lib/crates/fabro-workflow/src/run_materialization.rs
View file

@ -4,29 +4,14 @@ use fabro_types::WorkflowSettings;
use fabro_types::settings::InterpString;
use fabro_types::settings::run::RunGoal;
#[expect(
clippy::disallowed_methods,
reason = "raw source is today's behavior; run.model.name/provider are slated for demotion to plain String in the \
interpolation unification (D2)"
)]
pub fn materialize_run(
mut settings: WorkflowSettings,
graph: &Graph,
catalog: &Catalog,
configured_providers: &[ProviderId],
) -> WorkflowSettings {
let configured_model = settings
.run
.model
.name
.as_ref()
.map(InterpString::as_source);
let configured_provider = settings
.run
.model
.provider
.as_ref()
.map(InterpString::as_source);
let configured_model = settings.run.model.name.clone();
let configured_provider = settings.run.model.provider.clone();
let graph_provider = graph
.attrs
.get("default_provider")
@ -57,8 +42,8 @@ pub fn materialize_run(
None => (model, provider),
};
settings.run.model.name = Some(InterpString::parse(&resolved_model));
settings.run.model.provider = resolved_provider.as_deref().map(InterpString::parse);
settings.run.model.name = Some(resolved_model);
settings.run.model.provider = resolved_provider;
let goal = graph.goal().to_string();
settings.run.goal = if goal.is_empty() {

40
lib/crates/fabro-workflow/tests/materialize_run.rs
View file

@ -10,10 +10,6 @@ fn graph(source: &str) -> Graph {
parser::parse(source).expect("graph should parse")
}
#[expect(
clippy::disallowed_methods,
reason = "test asserts the raw template source"
)]
#[test]
fn materialize_run_applies_graph_and_catalog_defaults() {
let source = r#"digraph Test {
@ -26,7 +22,7 @@ fn materialize_run_applies_graph_and_catalog_defaults() {
let settings = WorkflowSettings {
run: RunNamespace {
model: RunModelSettings {
name: Some(InterpString::parse("sonnet")),
name: Some("sonnet".to_string()),
..RunModelSettings::default()
},
pull_request: Some(PullRequestSettings {
@ -41,24 +37,8 @@ fn materialize_run_applies_graph_and_catalog_defaults() {
let materialized = materialize_run(settings, &graph(source), Catalog::builtin(), &[]);
let resolved = &materialized.run;
assert_eq!(
resolved
.model
.name
.as_ref()
.map(InterpString::as_source)
.as_deref(),
Some("claude-sonnet-4-6")
);
assert_eq!(
resolved
.model
.provider
.as_ref()
.map(InterpString::as_source)
.as_deref(),
Some("anthropic")
);
assert_eq!(resolved.model.name.as_deref(), Some("claude-sonnet-4-6"));
assert_eq!(resolved.model.provider.as_deref(), Some("anthropic"));
assert_eq!(
materialized.run.goal.as_ref(),
Some(&RunGoal::Inline(InterpString::parse("Build feature")))
@ -66,10 +46,6 @@ fn materialize_run_applies_graph_and_catalog_defaults() {
assert!(resolved.pull_request.is_none());
}
#[expect(
clippy::disallowed_methods,
reason = "test asserts the raw template source"
)]
#[test]
fn materialize_run_uses_configured_provider_defaults() {
let source = r#"digraph Test {
@ -87,13 +63,5 @@ fn materialize_run_uses_configured_provider_defaults() {
);
let resolved = &materialized.run;
assert_eq!(
resolved
.model
.provider
.as_ref()
.map(InterpString::as_source)
.as_deref(),
Some("openai")
);
assert_eq!(resolved.model.provider.as_deref(), Some("openai"));
}