From a3a70a15179b6ba76e7d17ad010c7fd31512aed5 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Thu, 11 Jun 2026 10:08:57 -0400 Subject: [PATCH] refactor(types): apply /simplify cleanups to the demotion diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Borrow instead of clone where values are only consumed as &str (exec.rs, configured_repo_origin_url, resolve_start_llm), and take() instead of clone() in materialize_run where fields are overwritten. - Hoist the resolve_git warn calls out of the combinator chain. - Correct comments that claimed "only InterpString fields interpolate" in present tense — other plain-String fields still substitute via the String pass until a later slice retires it. Co-Authored-By: Claude Fable 5 --- lib/crates/fabro-cli/src/commands/exec.rs | 12 ++++-------- lib/crates/fabro-config/src/resolve/mod.rs | 5 +++-- lib/crates/fabro-config/src/resolve/run.rs | 17 ++++++++--------- lib/crates/fabro-manifest/src/lib.rs | 4 ++-- lib/crates/fabro-types/src/settings/run.rs | 18 +++++++----------- .../fabro-workflow/src/operations/start.rs | 10 +++------- .../fabro-workflow/src/run_materialization.rs | 4 ++-- 7 files changed, 29 insertions(+), 41 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index 9de98befa..724667191 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -282,8 +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.clone(); - let model_str = cli.exec.model.name.clone(); + let provider_str = cli.exec.model.provider.as_deref(); + let model_str = cli.exec.model.name.as_deref(); let permissions = cli.exec.agent.permissions.map(|p| match p { AgentPermissions::ReadOnly => AgentPermissionLevel::ReadOnly, AgentPermissions::ReadWrite => AgentPermissionLevel::ReadWrite, @@ -293,12 +293,8 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu SettingsOutputFormat::Text => OutputFormat::Text, SettingsOutputFormat::Json => OutputFormat::Json, }); - args.agent.apply_cli_defaults( - provider_str.as_deref(), - model_str.as_deref(), - permissions, - output_format, - ); + args.agent + .apply_cli_defaults(provider_str, model_str, permissions, output_format); let server_target = user_config::exec_server_target(&args.server)?; // v2 MCPs live under `cli.exec.agent.mcps` (owner-specific) or // `run.agent.mcps`. For `fabro exec` we use the cli.exec path, falling diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index 9576aac5c..54796bbf5 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -60,8 +60,9 @@ pub(crate) fn default_interp(path: impl AsRef) -> InterpString /// 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. +/// Other plain-`String` fields still substitute `{{ vars.* }}` until the +/// String pass itself is retired in a later slice. 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!( diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index e61abf607..b554c03f9 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -134,16 +134,15 @@ fn resolve_model(model: Option<&RunModelLayer>) -> RunModelSettings { } fn resolve_git(git: Option<&RunGitLayer>) -> RunGitSettings { + let author = git.and_then(|git| git.author.as_ref()); + if let Some(author) = 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()); + } RunGitSettings { - author: git.and_then(|git| { - 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(), - } - }) + author: author.map(|author| GitAuthorSettings { + name: author.name.clone(), + email: author.email.clone(), }), } } diff --git a/lib/crates/fabro-manifest/src/lib.rs b/lib/crates/fabro-manifest/src/lib.rs index 27650e8d0..6acac3de9 100644 --- a/lib/crates/fabro-manifest/src/lib.rs +++ b/lib/crates/fabro-manifest/src/lib.rs @@ -778,8 +778,8 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { { return None; } - let owner = scm.owner.clone()?; - let repository = scm.repository.clone()?; + let owner = scm.owner.as_deref()?; + let repository = scm.repository.as_deref()?; if owner.trim().is_empty() || repository.trim().is_empty() { return None; } diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index 36ffad50b..15f453ee9 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -84,9 +84,8 @@ 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)?; - // 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. + // run.model.provider/name and run.git.author.* were demoted to plain + // `String` and removed from this pass (D2): 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)?; @@ -104,8 +103,8 @@ impl RunNamespace { substitute_option(&mut slack.channel, &mut lookup)?; } substitute_map(&mut self.integrations.github.permissions, &mut lookup)?; - // run.scm.owner/repository are plain `String` (demoted, D2): no - // variable substitution. + // run.scm.owner/repository were demoted and removed from this pass + // (D2): values stay literal. substitute_string_vec(&mut self.prepare.commands, &mut lookup)?; for mcp in self.agent.mcps.values_mut() { substitute_string(&mut mcp.name, &mut lookup)?; @@ -409,12 +408,9 @@ 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. + // 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. let mut run = RunNamespace { model: super::RunModelSettings { provider: Some("{{ vars.PROVIDER }}".to_string()), diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 752f22b51..615cbccd7 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -573,19 +573,15 @@ fn resolve_start_llm( let provider = settings .model .provider - .clone() + .as_deref() .filter(|value| !value.is_empty()); let default_provider_id = catalog .default_for_configured_ids(configured) .provider .clone(); - let provider_context = routing::resolve_provider_context( - catalog, - &default_provider_id, - &model, - provider.as_deref(), - )?; + let provider_context = + routing::resolve_provider_context(catalog, &default_provider_id, &model, provider)?; let provider_id = provider_context.provider_id; let fallback_chain = resolve_fallback_chain(catalog, &provider_id, &model, &settings.model); diff --git a/lib/crates/fabro-workflow/src/run_materialization.rs b/lib/crates/fabro-workflow/src/run_materialization.rs index d670229ab..b6f9aa873 100644 --- a/lib/crates/fabro-workflow/src/run_materialization.rs +++ b/lib/crates/fabro-workflow/src/run_materialization.rs @@ -10,8 +10,8 @@ pub fn materialize_run( catalog: &Catalog, configured_providers: &[ProviderId], ) -> WorkflowSettings { - let configured_model = settings.run.model.name.clone(); - let configured_provider = settings.run.model.provider.clone(); + let configured_model = settings.run.model.name.take(); + let configured_provider = settings.run.model.provider.take(); let graph_provider = graph .attrs .get("default_provider")