refactor(types): apply /simplify cleanups to the demotion diff

- 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 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-06-11 10:08:57 -04:00
parent fd057d0198
commit a3a70a1517
7 changed files with 29 additions and 41 deletions

View file

@ -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

View file

@ -60,8 +60,9 @@ pub(crate) fn default_interp(path: impl AsRef<std::path::Path>) -> 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!(

View file

@ -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(),
}),
}
}

View file

@ -778,8 +778,8 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option<String> {
{
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;
}

View file

@ -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()),

View file

@ -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);

View file

@ -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")