From fd057d0198c0e1d3b51c0a339d9234bcd4cc1e81 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 9 Jun 2026 13:26:35 -0400 Subject: [PATCH] refactor(types): demote non-category leak fields to plain String MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- lib/crates/fabro-checkpoint/src/author.rs | 21 +----- lib/crates/fabro-cli/src/commands/exec.rs | 15 +--- lib/crates/fabro-cli/tests/it/cmd/create.rs | 4 +- lib/crates/fabro-config/src/builders.rs | 29 ++------ lib/crates/fabro-config/src/layers/cli.rs | 4 +- lib/crates/fabro-config/src/layers/run.rs | 12 ++-- lib/crates/fabro-config/src/resolve/cli.rs | 10 +++ lib/crates/fabro-config/src/resolve/mod.rs | 16 +++++ lib/crates/fabro-config/src/resolve/run.rs | 16 ++++- .../fabro-config/src/tests/resolve_cli.rs | 14 +--- .../fabro-config/src/tests/resolve_root.rs | 20 +----- .../fabro-config/src/tests/resolve_run.rs | 7 +- lib/crates/fabro-manifest/src/lib.rs | 21 ++---- lib/crates/fabro-server/src/demo/mod.rs | 4 +- lib/crates/fabro-server/src/run_manifest.rs | 26 ++----- lib/crates/fabro-server/src/serve.rs | 5 +- lib/crates/fabro-server/src/server/tests.rs | 7 +- lib/crates/fabro-types/src/settings/cli.rs | 4 +- lib/crates/fabro-types/src/settings/run.rs | 70 ++++++++++++++----- .../fabro-workflow/src/operations/create.rs | 6 +- .../fabro-workflow/src/operations/start.rs | 21 +++--- .../fabro-workflow/src/run_materialization.rs | 23 ++---- .../fabro-workflow/tests/materialize_run.rs | 40 ++--------- 23 files changed, 154 insertions(+), 241 deletions(-) diff --git a/lib/crates/fabro-checkpoint/src/author.rs b/lib/crates/fabro-checkpoint/src/author.rs index f6a366930..9546962b6 100644 --- a/lib/crates/fabro-checkpoint/src/author.rs +++ b/lib/crates/fabro-checkpoint/src/author.rs @@ -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()) } } diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index 94a784c43..9de98befa 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -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, diff --git a/lib/crates/fabro-cli/tests/it/cmd/create.rs b/lib/crates/fabro-cli/tests/it/cmd/create.rs index dd942910f..5a9a4402f 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/create.rs @@ -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, diff --git a/lib/crates/fabro-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index 63672fc6f..aa281d449 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -651,7 +651,6 @@ fn finish_dense_result( 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); } diff --git a/lib/crates/fabro-config/src/layers/cli.rs b/lib/crates/fabro-config/src/layers/cli.rs index 00a998190..472d58412 100644 --- a/lib/crates/fabro-config/src/layers/cli.rs +++ b/lib/crates/fabro-config/src/layers/cli.rs @@ -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, + pub provider: Option, /// Model name for `fabro exec`. #[serde(default, skip_serializing_if = "Option::is_none")] #[option(value_type = "string")] - pub name: Option, + pub name: Option, } #[derive( diff --git a/lib/crates/fabro-config/src/layers/run.rs b/lib/crates/fabro-config/src/layers/run.rs index 3cd9bdca9..505385a3b 100644 --- a/lib/crates/fabro-config/src/layers/run.rs +++ b/lib/crates/fabro-config/src/layers/run.rs @@ -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, + pub provider: Option, /// Model name for workflow runs. #[serde(default, skip_serializing_if = "Option::is_none")] #[option(value_type = "string")] - pub name: Option, + pub name: Option, /// 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, + pub name: Option, /// Git author email for checkpoint commits. #[serde(default, skip_serializing_if = "Option::is_none")] #[option(default = "\"fabro@local\"", value_type = "string")] - pub email: Option, + pub email: Option, } /// `[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, #[serde(default, skip_serializing_if = "Option::is_none")] - pub owner: Option, + pub owner: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub repository: Option, + pub repository: Option, /// Provider-specific SCM leaves. First-pass providers. #[serde(default, skip_serializing_if = "Option::is_none")] pub github: Option, diff --git a/lib/crates/fabro-config/src/resolve/cli.rs b/lib/crates/fabro-config/src/resolve/cli.rs index 9a58520e4..a9400b251 100644 --- a/lib/crates/fabro-config/src/resolve/cli.rs +++ b/lib/crates/fabro-config/src/resolve/cli.rs @@ -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 diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index 03ec3bdad..9576aac5c 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -56,6 +56,22 @@ pub(crate) fn default_interp(path: impl AsRef) -> 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; diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 83c11fd63..e61abf607 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -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(), diff --git a/lib/crates/fabro-config/src/tests/resolve_cli.rs b/lib/crates/fabro-config/src/tests/resolve_cli.rs index 9c7c6c942..3216f8800 100644 --- a/lib/crates/fabro-config/src/tests/resolve_cli.rs +++ b/lib/crates/fabro-config/src/tests/resolve_cli.rs @@ -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); diff --git a/lib/crates/fabro-config/src/tests/resolve_root.rs b/lib/crates/fabro-config/src/tests/resolve_root.rs index c3c24c7c0..ae5c6df81 100644 --- a/lib/crates/fabro-config/src/tests/resolve_root.rs +++ b/lib/crates/fabro-config/src/tests/resolve_root.rs @@ -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] diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index e45cb2af5..877e7ebd0 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -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 { diff --git a/lib/crates/fabro-manifest/src/lib.rs b/lib/crates/fabro-manifest/src/lib.rs index 4f50d06f5..27650e8d0 100644 --- a/lib/crates/fabro-manifest/src/lib.rs +++ b/lib/crates/fabro-manifest/src/lib.rs @@ -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 { let scm = &settings.run.scm; if !scm @@ -783,8 +778,8 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { { 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!( diff --git a/lib/crates/fabro-server/src/demo/mod.rs b/lib/crates/fabro-server/src/demo/mod.rs index d189b2a21..010d7ab6a 100644 --- a/lib/crates/fabro-server/src/demo/mod.rs +++ b/lib/crates/fabro-server/src/demo/mod.rs @@ -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 { diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 15a8a810e..9c9786706 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -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) { - 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) => ( diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 57170129a..87a726398 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -222,7 +222,6 @@ pub struct ServeArgs { } fn serve_overrides(args: &ServeArgs) -> (Option, Option) { - 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, Option) } 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 diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index a32a89c40..a07f1d927 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -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"), ); diff --git a/lib/crates/fabro-types/src/settings/cli.rs b/lib/crates/fabro-types/src/settings/cli.rs index 19aa30d76..a429c266a 100644 --- a/lib/crates/fabro-types/src/settings/cli.rs +++ b/lib/crates/fabro-types/src/settings/cli.rs @@ -44,8 +44,8 @@ pub struct CliExecSettings { #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct CliExecModelSettings { - pub provider: Option, - pub name: Option, + pub provider: Option, + pub name: Option, } #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index 9aabd80f8..36ffad50b 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -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, - pub name: Option, + pub provider: Option, + pub name: Option, pub fallbacks: Vec, /// 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, - pub email: Option, + pub name: Option, + pub email: Option, } #[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, - pub owner: Option, - pub repository: Option, + pub owner: Option, + pub repository: Option, pub github: Option, } diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index 6590a4f17..3c3ed5a72 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -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") ); diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 0a561d9eb..752f22b51 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -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 { - 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(); diff --git a/lib/crates/fabro-workflow/src/run_materialization.rs b/lib/crates/fabro-workflow/src/run_materialization.rs index 8b1caf3ba..d670229ab 100644 --- a/lib/crates/fabro-workflow/src/run_materialization.rs +++ b/lib/crates/fabro-workflow/src/run_materialization.rs @@ -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() { diff --git a/lib/crates/fabro-workflow/tests/materialize_run.rs b/lib/crates/fabro-workflow/tests/materialize_run.rs index 2c3679809..77fdf6dd0 100644 --- a/lib/crates/fabro-workflow/tests/materialize_run.rs +++ b/lib/crates/fabro-workflow/tests/materialize_run.rs @@ -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")); }