diff --git a/clippy.toml b/clippy.toml index 9a1759ec3..066d42b88 100644 --- a/clippy.toml +++ b/clippy.toml @@ -31,6 +31,7 @@ disallowed-methods = [ { path = "reqwest::blocking::Client::new", reason = "Use fabro_http::blocking_http_client() or fabro_http::blocking_test_http_client()", allow-invalid = true }, { path = "reqwest::blocking::Client::builder", reason = "Use fabro_http::BlockingHttpClientBuilder::new()", allow-invalid = true }, { path = "reqwest::get", reason = "Build a fabro_http client and send the request explicitly", allow-invalid = true }, + { path = "fabro_types::settings::interp::InterpString::as_source", reason = "Returns the unresolved template source, which leaks {{ ... }} tokens as literal text downstream. Resolve via resolve()/resolve_with() or substitute via substitute_with() instead; document intentional raw-source access (serialization, error messages, deliberate source preservation) with #[expect(clippy::disallowed_methods, reason = \"...\")]", allow-invalid = true }, ] disallowed-types = [ { path = "std::io::Read", reason = "Blocking trait; prefer tokio::io::AsyncReadExt on Tokio paths. Document intentional sync I/O with #[expect(clippy::disallowed_types, reason = \"...\")]" }, diff --git a/lib/crates/fabro-checkpoint/src/author.rs b/lib/crates/fabro-checkpoint/src/author.rs index 0b1714615..f6a366930 100644 --- a/lib/crates/fabro-checkpoint/src/author.rs +++ b/lib/crates/fabro-checkpoint/src/author.rs @@ -51,6 +51,11 @@ 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( @@ -60,6 +65,11 @@ impl From<&GitAuthorLayer> for 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<&GitAuthorSettings> for GitAuthor { fn from(value: &GitAuthorSettings) -> Self { Self::from_options( diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index 0e118d2a0..94a784c43 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -276,6 +276,11 @@ 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; diff --git a/lib/crates/fabro-cli/src/commands/run/overrides.rs b/lib/crates/fabro-cli/src/commands/run/overrides.rs index 693e59a7c..40fa46f6a 100644 --- a/lib/crates/fabro-cli/src/commands/run/overrides.rs +++ b/lib/crates/fabro-cli/src/commands/run/overrides.rs @@ -114,6 +114,10 @@ pub(crate) fn preflight_args_overrides(args: &PreflightArgs) -> Result RunEvent { event } +#[expect( + clippy::disallowed_methods, + reason = "known leak: GitHub App id/slug passes unresolved; strict resolution scheduled in the \ + interpolation unification (Phase 2)" +)] fn maybe_build_github_credentials( settings: &WorkflowSettings, vault: Option<&fabro_vault::Vault>, diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 58e0ea063..7797ff37a 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -86,6 +86,10 @@ fn process_env_var(name: &str) -> Option { std::env::var(name).ok() } +#[expect( + clippy::disallowed_methods, + reason = "raw source shown in the error message when resolution fails" +)] fn storage_dir_from_toml_with_lookup( source: &str, lookup: &dyn Fn(&str) -> Option, diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 3ec462f8c..e561d7480 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -222,6 +222,12 @@ 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 { let target = settings.target.as_ref()?; match target { diff --git a/lib/crates/fabro-cli/tests/it/cmd/create.rs b/lib/crates/fabro-cli/tests/it/cmd/create.rs index f3eb32f87..dd942910f 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/create.rs @@ -347,6 +347,10 @@ digraph FooWorkflow { ); } +#[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" +)] #[test] fn create_persists_requested_overrides_into_store() { let context = test_context!(); diff --git a/lib/crates/fabro-config/migrations/2026050101_legacy_sandbox_to_environments.rs b/lib/crates/fabro-config/migrations/2026050101_legacy_sandbox_to_environments.rs index 860504d65..ff152928a 100644 --- a/lib/crates/fabro-config/migrations/2026050101_legacy_sandbox_to_environments.rs +++ b/lib/crates/fabro-config/migrations/2026050101_legacy_sandbox_to_environments.rs @@ -417,6 +417,10 @@ provider = "daytona" assert!(migrated.is_none()); } + #[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" + )] #[test] fn daytona_snapshot_labels_lifecycle_and_volumes_migrate() { let migrated = migrate( diff --git a/lib/crates/fabro-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index b5a76d58d..38c022725 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -693,6 +693,10 @@ 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() diff --git a/lib/crates/fabro-config/src/load.rs b/lib/crates/fabro-config/src/load.rs index b4d378b69..a1182b7a5 100644 --- a/lib/crates/fabro-config/src/load.rs +++ b/lib/crates/fabro-config/src/load.rs @@ -38,6 +38,10 @@ pub(crate) fn load_settings_path(path: &Path, source: SettingsSource) -> Result< Ok(layer) } +#[expect( + clippy::disallowed_methods, + reason = "guarded by is_literal() above; the raw source is the literal value" +)] pub(crate) fn resolve_goal_file_paths(file: &mut SettingsLayer, base_dir: &Path) { let Some(run) = file.run.as_mut() else { return; diff --git a/lib/crates/fabro-config/src/project.rs b/lib/crates/fabro-config/src/project.rs index eccdf684d..bf48db569 100644 --- a/lib/crates/fabro-config/src/project.rs +++ b/lib/crates/fabro-config/src/project.rs @@ -144,6 +144,12 @@ fn sibling_workflow_toml_for(graph: &Path) -> Option { (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 { return caller_cwd.to_path_buf(); diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index 418aed302..03ec3bdad 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -29,6 +29,11 @@ pub(crate) fn require_interp( }) } +#[expect( + clippy::disallowed_methods, + reason = "parsed_value special case: the TCP listen address parses the literal source \ + (templates intentionally unsupported)" +)] pub(crate) fn parse_socket_addr( value: &InterpString, path: &str, diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 52eb838f6..83c11fd63 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -141,6 +141,11 @@ fn resolve_git(git: Option<&RunGitLayer>) -> RunGitSettings { } } +#[expect( + clippy::disallowed_methods, + reason = "known leak: prepare step templates collapse to raw source unresolved; strict \ + resolution scheduled in the interpolation unification (Phase 2)" +)] fn resolve_prepare( prepare: Option<&RunPrepareLayer>, errors: &mut Vec, @@ -281,6 +286,11 @@ fn resolve_agent(agent: Option<&RunAgentLayer>) -> RunAgentSettings { } } +#[expect( + clippy::disallowed_methods, + reason = "known leak: MCP transport templates collapse to raw source unresolved; strict \ + resolution scheduled in the interpolation unification (Phase 2, supersedes PR #370)" +)] pub(crate) fn resolve_mcp_entry(name: &str, entry: &McpEntryLayer) -> McpServerSettings { let transport = match entry { McpEntryLayer::Stdio { @@ -357,6 +367,11 @@ pub(crate) fn resolve_mcp_entry(name: &str, entry: &McpEntryLayer) -> McpServerS } } +#[expect( + clippy::disallowed_methods, + reason = "known leak: MCP transport templates collapse to raw source unresolved; strict \ + resolution scheduled in the interpolation unification (Phase 2, supersedes PR #370)" +)] fn resolve_mcp_command( script: Option<&InterpString>, command: Option<&Vec>, @@ -369,6 +384,11 @@ fn resolve_mcp_command( .unwrap_or_default() } +#[expect( + clippy::disallowed_methods, + reason = "intentional source preservation: the hook executor re-resolves {{ env.* }} \ + tokens at hook fire time" +)] fn resolve_hook(hook: &HookEntry, index: usize, errors: &mut Vec) -> HookDefinition { let variants = [ hook.script.is_some() || hook.command.is_some(), @@ -414,6 +434,11 @@ fn resolve_hook(hook: &HookEntry, index: usize, errors: &mut Vec) } } +#[expect( + clippy::disallowed_methods, + reason = "intentional source preservation: the hook executor re-resolves {{ env.* }} \ + tokens at hook fire time" +)] fn resolve_hook_type(hook: &HookEntry) -> Option { if hook.script.is_some() || hook.command.is_some() { return None; diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index 26daf7ba7..17ab3ff0b 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -305,6 +305,11 @@ fn resolve_object_store( } } +#[expect( + clippy::disallowed_methods, + reason = "derives sibling default paths in source form; the result is re-parsed as an \ + InterpString and resolves at consumption" +)] fn object_store_default_root(storage_root: &InterpString, domain: &str) -> InterpString { let root = storage_root.as_source(); let root = root.trim_end_matches('/'); diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index 0dbdabbca..691200293 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -140,6 +140,11 @@ fn process_env_var(name: &str) -> Option { std::env::var(name).ok() } +#[expect( + clippy::disallowed_methods, + reason = "goal text intentionally passes through in source form; goals become importable \ + templates in the interpolation unification (Phase 3)" +)] fn resolve_layer_goal( goal: &RunGoalLayer, base_dir: &Path, @@ -153,6 +158,11 @@ fn resolve_layer_goal( } } +#[expect( + clippy::disallowed_methods, + reason = "goal text intentionally passes through in source form; goals become importable \ + templates in the interpolation unification (Phase 3)" +)] fn resolve_goal( goal: &RunGoal, base_dir: &Path, @@ -167,6 +177,10 @@ fn resolve_goal( } #[cfg(test)] +#[expect( + clippy::disallowed_methods, + reason = "tests assert the raw template source" +)] mod tests { use fabro_types::settings::run::RunGoal; diff --git a/lib/crates/fabro-config/src/tests/combine.rs b/lib/crates/fabro-config/src/tests/combine.rs index 557c4316c..fb0db5848 100644 --- a/lib/crates/fabro-config/src/tests/combine.rs +++ b/lib/crates/fabro-config/src/tests/combine.rs @@ -1,3 +1,8 @@ +#![expect( + clippy::disallowed_methods, + reason = "tests assert the raw template source" +)] + use fabro_types::settings::InterpString; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::settings::server::LogDestination; diff --git a/lib/crates/fabro-config/src/tests/resolve_cli.rs b/lib/crates/fabro-config/src/tests/resolve_cli.rs index 626da3cb5..9c7c6c942 100644 --- a/lib/crates/fabro-config/src/tests/resolve_cli.rs +++ b/lib/crates/fabro-config/src/tests/resolve_cli.rs @@ -80,6 +80,10 @@ 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( diff --git a/lib/crates/fabro-config/src/tests/resolve_root.rs b/lib/crates/fabro-config/src/tests/resolve_root.rs index b8f907811..c3c24c7c0 100644 --- a/lib/crates/fabro-config/src/tests/resolve_root.rs +++ b/lib/crates/fabro-config/src/tests/resolve_root.rs @@ -80,6 +80,10 @@ provider = "not-a-provider" assert!(rendered.contains("run.environment.provider")); } +#[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" +)] #[test] fn namespace_resolvers_cover_root_level_settings_shape() { let source = r#" diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index e89edfc05..e45cb2af5 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -94,6 +94,10 @@ fn resolves_run_defaults_from_empty_settings() { assert!(settings.pull_request.is_none()); } +#[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" +)] #[test] fn resolves_named_daytona_environment_from_injected_catalog() { let settings = workflow_settings_from_toml_with_catalog( @@ -748,6 +752,10 @@ issues = "read" ); } + #[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" + )] #[test] fn resolver_preserves_interp_string_in_permissions() { let resolved = super::workflow_settings_from_toml( diff --git a/lib/crates/fabro-config/src/tests/resolve_server.rs b/lib/crates/fabro-config/src/tests/resolve_server.rs index d45f1e040..051aed809 100644 --- a/lib/crates/fabro-config/src/tests/resolve_server.rs +++ b/lib/crates/fabro-config/src/tests/resolve_server.rs @@ -1,6 +1,6 @@ #![expect( clippy::disallowed_methods, - reason = "sync test fixture setup; not on a Tokio path" + reason = "sync test fixture setup and raw template source assertions; not on a Tokio path" )] use fabro_types::settings::InterpString; diff --git a/lib/crates/fabro-environment/src/model.rs b/lib/crates/fabro-environment/src/model.rs index 2943ac538..e07cb052e 100644 --- a/lib/crates/fabro-environment/src/model.rs +++ b/lib/crates/fabro-environment/src/model.rs @@ -310,6 +310,11 @@ fn append_string_map(root: &mut Table, name: &str, map: &StickyMap) { } } +#[expect( + clippy::disallowed_methods, + reason = "serializing the InterpString map back to its TOML source form; tokens must \ + round-trip unresolved (resolution happens at consumption, not serialization)" +)] fn append_interp_map(root: &mut Table, name: &str, map: &StickyMap) { if map.is_empty() { return; diff --git a/lib/crates/fabro-manifest/src/lib.rs b/lib/crates/fabro-manifest/src/lib.rs index 5a21cdd6d..4f50d06f5 100644 --- a/lib/crates/fabro-manifest/src/lib.rs +++ b/lib/crates/fabro-manifest/src/lib.rs @@ -769,6 +769,11 @@ 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 @@ -897,6 +902,10 @@ 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 { diff --git a/lib/crates/fabro-server/src/canonical_origin.rs b/lib/crates/fabro-server/src/canonical_origin.rs index 2a9ab1170..4231fd9fa 100644 --- a/lib/crates/fabro-server/src/canonical_origin.rs +++ b/lib/crates/fabro-server/src/canonical_origin.rs @@ -7,6 +7,10 @@ use fabro_types::settings::{ServerNamespace, validate_public_url}; use crate::server::EnvLookup; +#[expect( + clippy::disallowed_methods, + reason = "raw source shown in the error message when resolution fails" +)] pub(crate) fn resolve_canonical_origin( resolved: &ServerNamespace, env_lookup: &EnvLookup, diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 6b8d1f1ab..4ce1c9837 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -327,6 +327,11 @@ fn short_error_line(rendered: &str) -> String { } } +#[expect( + clippy::disallowed_methods, + reason = "known leak: GitHub App id/slug passes unresolved; strict resolution scheduled in the \ + interpolation unification (Phase 2)" +)] async fn check_github_app(state: &AppState) -> CheckResult { let settings = state.server_settings(); if settings.server.integrations.github.strategy == GithubIntegrationStrategy::Token { diff --git a/lib/crates/fabro-server/src/interp.rs b/lib/crates/fabro-server/src/interp.rs new file mode 100644 index 000000000..599421e1d --- /dev/null +++ b/lib/crates/fabro-server/src/interp.rs @@ -0,0 +1,51 @@ +//! Shared process-env interpolation helpers for server-scope settings. +//! +//! Server-scope `InterpString` fields resolve `{{ env.* }}` tokens against +//! the server's own process environment. This module owns the single +//! process-env lookup facade and the canonical resolve helpers; do not add +//! per-module copies. + +use std::path::PathBuf; + +use anyhow::Context; +use fabro_types::settings::InterpString; + +/// Resolve a server-scope `InterpString` with a caller-provided env lookup. +/// +/// This is the single resolve core; [`resolve_interp`] (process env) and +/// `AppState::resolve_interp` (injectable `env_lookup` seam) both delegate +/// here. +pub(crate) fn resolve_interp_with( + value: &InterpString, + lookup: impl FnMut(&str) -> Option, +) -> anyhow::Result { + value + .resolve(lookup) + .map(|resolved| resolved.value) + .map_err(anyhow::Error::from) +} + +/// Resolve a server-scope `InterpString` against the process environment. +#[expect( + clippy::disallowed_methods, + reason = "raw source shown in the error message when resolution fails" +)] +pub(crate) fn resolve_interp(value: &InterpString) -> anyhow::Result { + resolve_interp_with(value, process_env_var) + .with_context(|| format!("failed to resolve {}", value.as_source())) +} + +/// [`resolve_interp`], parsed into a filesystem path. +pub(crate) fn resolve_interp_path(value: &InterpString) -> anyhow::Result { + Ok(PathBuf::from(resolve_interp(value)?)) +} + +/// The server-owned process-env lookup facade for `{{ env.* }}` +/// interpolation and server configuration/secret reads. +#[expect( + clippy::disallowed_methods, + reason = "server-scope interpolation and configuration own this process-env lookup facade" +)] +pub(crate) fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} diff --git a/lib/crates/fabro-server/src/jwt_auth.rs b/lib/crates/fabro-server/src/jwt_auth.rs index 3c956ac79..cd442edef 100644 --- a/lib/crates/fabro-server/src/jwt_auth.rs +++ b/lib/crates/fabro-server/src/jwt_auth.rs @@ -15,6 +15,7 @@ use tracing::info; use crate::auth::REFRESH_TOKEN_PREFIX; use crate::auth::{self, AuthErrorCode, JwtError, JwtSigningKey, KeyDeriveError}; use crate::error::ApiError; +use crate::interp::process_env_var; type HmacSha256 = Hmac; const DEV_TOKEN_COMPARE_KEY: &[u8] = b"fabro-dev-token-compare-key"; @@ -58,14 +59,6 @@ pub fn resolve_auth_mode(settings: &ServerNamespace) -> Result { resolve_auth_mode_with_lookup(settings, process_env_var) } -#[expect( - clippy::disallowed_methods, - reason = "Server auth startup validation intentionally reads process env for server secrets." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - pub fn resolve_auth_mode_with_lookup(settings: &ServerNamespace, lookup: F) -> Result where F: Fn(&str) -> Option, diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 53d5c5451..187f42ab0 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -27,6 +27,7 @@ pub mod diagnostics; pub mod error; pub mod github_webhooks; pub mod install; +mod interp; pub mod jwt_auth; pub mod manifest_validation; mod migrations; diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 4feb22133..15a8a810e 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -11,7 +11,7 @@ use fabro_config::parse::{self, SettingsSource}; use fabro_config::{ CliLayer, CliOutputLayer, EnvironmentDockerfileLayer, EnvironmentImageLayer, EnvironmentLayer, MergeMap, RunLayer, SettingsLayer, WorkflowSettingsBuilder, parse_input_overrides, - parse_labels, + parse_labels, project, }; use fabro_graphviz::graph::{Graph, is_llm_handler_type}; use fabro_graphviz::render::apply_direction; @@ -40,6 +40,7 @@ use tokio::process::Command; use tokio::time; use tracing::warn; +use crate::interp::process_env_var; use crate::server::AppState; use crate::server_secrets::LlmClientResult; @@ -173,7 +174,7 @@ pub(crate) fn prepare_manifest_with_environment_defaults( target_path, workflow_bundle, workflow_input, - source_directory: resolve_working_directory(&settings, &cwd), + source_directory: project::resolve_working_directory_from_run(&settings.run, &cwd), }) } @@ -374,31 +375,6 @@ fn manifest_args_overrides( }) } -fn resolve_working_directory(settings: &WorkflowSettings, caller_cwd: &Path) -> PathBuf { - let Some(work_dir) = settings - .run - .working_dir - .as_ref() - .map(InterpString::as_source) - else { - return caller_cwd.to_path_buf(); - }; - let path = PathBuf::from(&work_dir); - if path.is_absolute() { - path - } else { - caller_cwd.join(path) - } -} - -#[expect( - clippy::disallowed_methods, - reason = "Manifest preflight interpolation owns a process-env lookup facade for {{ env.* }} values." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - fn resolve_manifest_dockerfiles( layer: &mut SettingsLayer, config_path: &ManifestPath, @@ -1166,6 +1142,11 @@ 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, diff --git a/lib/crates/fabro-server/src/run_tool_manifest.rs b/lib/crates/fabro-server/src/run_tool_manifest.rs index d17f5ccd0..8e38e8317 100644 --- a/lib/crates/fabro-server/src/run_tool_manifest.rs +++ b/lib/crates/fabro-server/src/run_tool_manifest.rs @@ -131,6 +131,10 @@ mod tests { assert_eq!(args.input, vec![r"count=3", r#"decision="approve""#]); } + #[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" + )] #[test] fn run_overrides_preserve_goal_file_as_file_goal() { let spec = ValidatedCreateRunSpec::try_from(CreateRunSpec { diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 1178b009b..57170129a 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -16,8 +16,7 @@ use fabro_static::EnvVars; use fabro_types::ServerSettings; use fabro_types::settings::server::{GithubIntegrationStrategy, LogDestination, WebhookStrategy}; use fabro_types::settings::{ - GithubIntegrationSettings, InterpString, ObjectStoreSettings, ServerListenSettings, - ServerNamespace, + GithubIntegrationSettings, ObjectStoreSettings, ServerListenSettings, ServerNamespace, }; use fabro_util::terminal::Styles; use object_store::aws::{AmazonS3Builder, AmazonS3ConfigKey}; @@ -33,6 +32,7 @@ use tracing::{error, info, warn}; use crate::canonical_origin::resolve_canonical_origin; use crate::github_webhooks::{TailscaleFunnelManager, WEBHOOK_ROUTE, WEBHOOK_SECRET_ENV}; +use crate::interp::{process_env_var, resolve_interp, resolve_interp_path}; use crate::server::{ AppState, AppStateConfig, ResolvedAppStateSettings, RouterOptions, build_app_state, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, @@ -557,25 +557,6 @@ fn resolved_bind_request( } } -fn resolve_interp(value: &InterpString) -> anyhow::Result { - value - .resolve(process_env_var) - .map(|resolved| resolved.value) - .with_context(|| format!("failed to resolve {}", value.as_source())) -} - -#[expect( - clippy::disallowed_methods, - reason = "Server settings interpolation owns a process-env lookup facade for {{ env.* }} values." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - -fn resolve_interp_path(value: &InterpString) -> anyhow::Result { - Ok(PathBuf::from(resolve_interp(value)?)) -} - fn absolute_path(path: PathBuf) -> anyhow::Result { if path.is_absolute() { Ok(path) @@ -1168,6 +1149,10 @@ fn server_bind_title(bind: &Bind) -> String { reason = "tests reserve/probe ports via sync std::net::TcpListener; the async server under \ test uses tokio::net::TcpListener separately" )] +#[expect( + clippy::disallowed_methods, + reason = "tests assert the raw template source" +)] mod tests { use std::io; use std::path::PathBuf; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 8413a4579..158eb751e 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -146,6 +146,7 @@ use crate::error::ApiError; use crate::github_webhooks::{ WEBHOOK_ROUTE, WEBHOOK_SECRET_ENV, parse_event_metadata, verify_signature, }; +use crate::interp::{process_env_var, resolve_interp, resolve_interp_with}; use crate::jwt_auth::{self, AuthMode}; use crate::principal_middleware::{ AuthContextSlot, RequestAuth, RequestAuthContext, RequireRunBlob, RequireRunManagementTarget, @@ -1349,7 +1350,7 @@ impl AppState { pub(crate) fn server_storage_dir(&self) -> PathBuf { PathBuf::from( - resolve_interp_string(&self.server_settings().server.storage.root) + resolve_interp(&self.server_settings().server.storage.root) .expect("server storage root should be resolved at startup"), ) } @@ -1487,10 +1488,7 @@ impl AppState { } pub(crate) fn resolve_interp(&self, value: &InterpString) -> anyhow::Result { - value - .resolve(|name| (self.env_lookup)(name)) - .map(|resolved| resolved.value) - .map_err(anyhow::Error::from) + resolve_interp_with(value, |name| (self.env_lookup)(name)) } pub(crate) fn canonical_origin(&self) -> Result { @@ -1502,6 +1500,11 @@ impl AppState { .and_then(|value| auth::derive_cookie_key(value.as_bytes()).ok()) } + #[expect( + clippy::disallowed_methods, + reason = "known leak: GitHub App id/slug passes unresolved; strict resolution scheduled in the \ + interpolation unification (Phase 2)" + )] pub(crate) fn github_credentials( &self, settings: &GithubIntegrationSettings, @@ -1627,21 +1630,6 @@ fn decode_secret_pem(name: &str, raw: &str) -> Result { .map_err(|err| format!("{name} base64 decoded to invalid UTF-8: {err}")) } -fn resolve_interp_string(value: &InterpString) -> anyhow::Result { - value - .resolve(process_env_var) - .map(|resolved| resolved.value) - .map_err(anyhow::Error::from) -} - -#[expect( - clippy::disallowed_methods, - reason = "Server state owns process-env lookup facades for interpolation and non-secret configuration." -)] -pub(crate) fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - fn start_optional_slack_service(state: &Arc) { let Some(service) = state.slack_service.clone() else { return; @@ -2399,7 +2387,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result PathBuf::from(path), Err(err) => { return ApiError::new( @@ -969,7 +969,7 @@ async fn validate_run_manifest( async fn substitute_run_variables( state: &AppState, settings: &mut WorkflowSettings, -) -> Result<(), ResolveEnvError> { +) -> Result<(), ResolveError> { let variables = state.variables.read().await; settings .run diff --git a/lib/crates/fabro-server/src/server/handler/system.rs b/lib/crates/fabro-server/src/server/handler/system.rs index f611d2fc2..4eae3837e 100644 --- a/lib/crates/fabro-server/src/server/handler/system.rs +++ b/lib/crates/fabro-server/src/server/handler/system.rs @@ -18,8 +18,9 @@ use super::super::{ SystemInfoResponse, SystemIntegrationStatus, SystemIntegrationsResponse, SystemRepairRunIssue, SystemRepairRunsResponse, SystemRunCounts, build_disk_usage_response, build_prune_plan, counts_toward_scheduler_capacity, delete_run_internal, diagnostics, get, post, - resolve_interp_string, resource_sampler, spawn_blocking, system_sandbox_provider, to_i64, + resource_sampler, spawn_blocking, system_sandbox_provider, to_i64, }; +use crate::interp::resolve_interp; pub(super) fn routes() -> Router> { Router::new() @@ -51,6 +52,11 @@ async fn get_server_settings(_auth: RequiredUser, State(state): State>) -> Response { let manifest_run_settings = state.manifest_run_settings(); let server_settings = state.server_settings(); @@ -276,9 +282,7 @@ fn missing_vault_secret(state: &AppState, name: &str) -> bool { } fn display_interp(state: &AppState, value: &InterpString) -> String { - state - .resolve_interp(value) - .unwrap_or_else(|_| value.as_source()) + value.resolve_or_source(|name| (state.env_lookup)(name)) } async fn get_system_resources(_auth: RequiredUser, State(state): State>) -> Response { @@ -478,7 +482,7 @@ async fn get_github_repo( ) .into_response(); }; - if let Err(err) = resolve_interp_string(app_id) { + if let Err(err) = resolve_interp(app_id) { return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) .into_response(); } @@ -506,7 +510,7 @@ async fn get_github_repo( } }; let install_url = match github_settings.slug.as_ref() { - Some(slug) => match resolve_interp_string(slug) { + Some(slug) => match resolve_interp(slug) { Ok(slug) => format!("https://github.com/apps/{slug}/installations/new"), Err(err) => { return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 363884738..a32a89c40 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -1090,6 +1090,10 @@ url = "{{ env.FABRO_WEB_URL }}" } } +#[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" +)] #[test] fn replace_settings_updates_layer_and_typed_server_settings() { let state = test_app_state_with_options( @@ -13433,6 +13437,10 @@ async fn post_runs_returns_submitted_status() { assert_eq!(run_json_status(&body)["kind"], "submitted"); } +#[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" +)] #[tokio::test] async fn start_run_persists_full_settings_snapshot() { let source = r#" diff --git a/lib/crates/fabro-server/src/test_support.rs b/lib/crates/fabro-server/src/test_support.rs index 7e2731809..8d3f94cf7 100644 --- a/lib/crates/fabro-server/src/test_support.rs +++ b/lib/crates/fabro-server/src/test_support.rs @@ -30,12 +30,13 @@ use ulid::Ulid; use crate::auth; use crate::automation_materializer::AutomationRunMaterializer; pub use crate::automation_materializer::TestAutomationRunMaterializer; +use crate::interp::process_env_var; use crate::jwt_auth::{AuthMode, ConfiguredAuth}; #[cfg(test)] use crate::principal_middleware::{AuthContextSlot, RequestAuthContext}; use crate::server::{ self, AppState, AppStateConfig, EnvLookup, RegistryFactoryOverride, ResolvedAppStateSettings, - RouterOptions, build_app_state, process_env_var, + RouterOptions, build_app_state, }; use crate::server_secrets::ServerSecrets; #[cfg(test)] diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index 033e46db6..8bbdb81a5 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -21,6 +21,7 @@ use tracing::{debug, error, info, warn}; use crate::auth::{GithubEndpoints, browser_shell}; use crate::error::ApiError; +use crate::interp::process_env_var; use crate::jwt_auth::{AuthMode, auth_method_name, dev_token_matches}; use crate::principal_middleware::{ RequestAuth, RequestAuthContext, UserProfile, require_authenticated_user, @@ -345,14 +346,6 @@ fn session_timestamp(timestamp: i64) -> Result, ApiError> { }) } -#[expect( - clippy::disallowed_methods, - reason = "Web auth resolves configured {{ env.* }} URLs through this process-env facade." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - async fn login_dev_token( State(state): State>, Extension(auth_mode): Extension, diff --git a/lib/crates/fabro-server/tests/it/api/system.rs b/lib/crates/fabro-server/tests/it/api/system.rs index 1604d49cd..34005cde1 100644 --- a/lib/crates/fabro-server/tests/it/api/system.rs +++ b/lib/crates/fabro-server/tests/it/api/system.rs @@ -114,6 +114,10 @@ async fn load_questions(app: &axum::Router, run_id: &str) -> serde_json::Value { .await } +#[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" +)] #[tokio::test] async fn get_system_info_returns_runtime_fields() { let (_temp, settings, expected_storage_dir) = temp_storage_settings(); diff --git a/lib/crates/fabro-types/src/settings/interp.rs b/lib/crates/fabro-types/src/settings/interp.rs index 2eec30a5a..664ec583c 100644 --- a/lib/crates/fabro-types/src/settings/interp.rs +++ b/lib/crates/fabro-types/src/settings/interp.rs @@ -1,10 +1,19 @@ -//! Env var and run variable interpolation for config strings. +//! Interpolation for config strings. //! -//! Any string field may use `{{ env.NAME }}` tokens, either as a whole value or -//! as one or more substrings inside a larger string. Run-scoped settings may -//! additionally use non-sensitive `{{ vars.NAME }}` tokens. Resolution happens -//! only when the field is consumed, and provenance tracking lets outward-facing -//! renderers redact env-sourced values uniformly. +//! An [`InterpString`] field may contain narrow `{{ .NAME }}` +//! tokens — no template logic — drawn from the four [`Namespace`]s: `env`, +//! `vars`, `secrets`, and `inputs`. Which namespaces actually resolve is +//! scope-determined by the caller through [`ResolveCtx`]: server-scope +//! settings provide `env` (and eventually `secrets`), run-scope settings +//! additionally provide `vars` and `inputs`. A token whose namespace is not +//! available in the resolution context fails loudly instead of passing +//! through as literal text. +//! +//! Resolution timing is split: `vars`/`inputs` substitute early (server-side, +//! at run creation) via [`InterpString::substitute_with`], while +//! `env`/`secrets` resolve late, at consumption time in the process that owns +//! the value, via [`InterpString::resolve_with`]. Provenance tracking lets +//! outward-facing renderers redact env- and secret-sourced values uniformly. use std::fmt; @@ -13,8 +22,8 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use crate::variable::is_env_style_name; -/// A config string that may contain `{{ env.NAME }}` or `{{ vars.NAME }}` -/// tokens. +/// A config string that may contain `{{ env.NAME }}`, `{{ vars.NAME }}`, +/// `{{ secrets.NAME }}`, or `{{ inputs.NAME }}` tokens. #[derive(Debug, Clone, PartialEq, Eq)] pub struct InterpString { segments: Vec, @@ -23,8 +32,130 @@ pub struct InterpString { #[derive(Debug, Clone, PartialEq, Eq)] enum Segment { Literal(String), - EnvVar(String), - Variable(String), + Token { + namespace: Namespace, + name: String, + }, +} + +/// The interpolation namespaces recognized inside `{{ ... }}` tokens. +#[derive( + Debug, Clone, Copy, PartialEq, Eq, Hash, strum::Display, strum::EnumString, strum::IntoStaticStr, +)] +#[strum(serialize_all = "lowercase")] +pub enum Namespace { + /// `{{ env.NAME }}` — process environment, resolved at consumption time. + Env, + /// `{{ vars.NAME }}` — non-sensitive run variables, substituted early. + Vars, + /// `{{ secrets.NAME }}` — vault secrets, resolved at consumption time. + Secrets, + /// `{{ inputs.NAME }}` — workflow run inputs, substituted early. + Inputs, +} + +impl Namespace { + /// The noun used for this namespace in error messages. + fn noun(self) -> &'static str { + match self { + Self::Env => "environment variable", + Self::Vars => "variable", + Self::Secrets => "secret", + Self::Inputs => "input", + } + } + + /// Parse a trimmed `{{ ... }}` token body into a namespace + name, or + /// `None` when the body is not a recognized token (it then stays literal). + fn parse_token(token: &str) -> Option<(Self, String)> { + let trimmed = token.trim(); + let (prefix, name) = trimmed.split_once('.')?; + let namespace = prefix.parse::().ok()?; + namespace + .is_valid_name(name) + .then(|| (namespace, name.to_owned())) + } + + fn is_valid_name(self, name: &str) -> bool { + match self { + // Preserves the original env token grammar: any non-empty run of + // ASCII alphanumerics/underscores (leading digits allowed). + Self::Env => { + !name.is_empty() + && name + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') + } + Self::Vars | Self::Secrets => is_env_style_name(name), + // Input keys are TOML bare keys; additionally allow interior + // hyphens. + Self::Inputs => { + let mut chars = name.chars(); + match chars.next() { + Some(first) if first.is_ascii_alphanumeric() || first == '_' => {} + _ => return false, + } + chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '-') + } + } + } +} + +/// The namespace lookups available when resolving or substituting an +/// [`InterpString`]. +/// +/// Which namespaces are populated is scope-determined by the caller: a token +/// in a namespace with no lookup is a [`ResolveErrorKind::Unavailable`] error +/// under [`InterpString::resolve_with`], and passes through unchanged under +/// [`InterpString::substitute_with`]. +#[derive(Default)] +pub struct ResolveCtx<'a> { + env: Option>, + vars: Option>, + secrets: Option>, + inputs: Option>, +} + +type LookupFn<'a> = Box Option + 'a>; + +impl<'a> ResolveCtx<'a> { + #[must_use] + pub fn new() -> Self { + Self::default() + } + + #[must_use] + pub fn with_env(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { + self.env = Some(Box::new(lookup)); + self + } + + #[must_use] + pub fn with_vars(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { + self.vars = Some(Box::new(lookup)); + self + } + + #[must_use] + pub fn with_secrets(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { + self.secrets = Some(Box::new(lookup)); + self + } + + #[must_use] + pub fn with_inputs(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { + self.inputs = Some(Box::new(lookup)); + self + } + + fn lookup_for(&mut self, namespace: Namespace) -> Option<&mut LookupFn<'a>> { + match namespace { + Namespace::Env => self.env.as_mut(), + Namespace::Vars => self.vars.as_mut(), + Namespace::Secrets => self.secrets.as_mut(), + Namespace::Inputs => self.inputs.as_mut(), + } + } } impl InterpString { @@ -35,41 +166,21 @@ impl InterpString { match segments.last_mut() { Some(Segment::Literal(existing)) => existing.push_str(text), - Some(Segment::EnvVar(_) | Segment::Variable(_)) | None => { + Some(Segment::Token { .. }) | None => { segments.push(Segment::Literal(text.to_owned())); } } } - fn parse_env_token(token: &str) -> Option { - let trimmed = token.trim(); - let name = trimmed.strip_prefix("env.")?; - if name.is_empty() - || !name - .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') - { - return None; - } - Some(name.to_owned()) - } - - fn parse_vars_token(token: &str) -> Option { - let trimmed = token.trim(); - let name = trimmed.strip_prefix("vars.")?; - if is_env_style_name(name) { - Some(name.to_owned()) - } else { - None - } - } - - /// Parse a raw string into its literal/env-var segments. + /// Parse a raw string into its literal/token segments. /// /// The [`From`] and [`From<&str>`] impls delegate here. /// - /// Parsing is infallible: the token grammar is intentionally permissive so - /// that validation happens at consumption time along with env lookup. + /// Parsing is infallible and intentionally permissive: only + /// `{{ .NAME }}` shaped tokens are claimed; any other + /// `{{ ... }}` text (jq programs, Go templates, unterminated braces) + /// stays literal. This is a documented known limitation — validation of + /// claimed tokens happens at substitution/resolution time. #[must_use] pub fn parse(input: &str) -> Self { let mut segments: Vec = Vec::new(); @@ -81,10 +192,8 @@ impl InterpString { let after_open = &rest[start + 2..]; if let Some(close) = after_open.find("}}") { let token = &after_open[..close]; - if let Some(name) = Self::parse_env_token(token) { - segments.push(Segment::EnvVar(name)); - } else if let Some(name) = Self::parse_vars_token(token) { - segments.push(Segment::Variable(name)); + if let Some((namespace, name)) = Namespace::parse_token(token) { + segments.push(Segment::Token { namespace, name }); } else { Self::push_literal(&mut segments, &rest[start..start + 2 + close + 2]); } @@ -116,60 +225,48 @@ impl InterpString { .all(|seg| matches!(seg, Segment::Literal(_))) } - /// True when this string contains at least one env var token. + /// True when this string contains at least one token in `namespace`. #[must_use] - pub fn references_env(&self) -> bool { + pub fn references(&self, namespace: Namespace) -> bool { self.segments .iter() - .any(|seg| matches!(seg, Segment::EnvVar(_))) + .any(|seg| matches!(seg, Segment::Token { namespace: ns, .. } if *ns == namespace)) } - /// True when this string contains at least one run variable token. + /// The names referenced in `namespace` by this string, in source order. #[must_use] - pub fn references_vars(&self) -> bool { - self.segments - .iter() - .any(|seg| matches!(seg, Segment::Variable(_))) - } - - /// The env var names referenced by this string, in source order. - #[must_use] - pub fn env_var_names(&self) -> Vec<&str> { + pub fn names(&self, namespace: Namespace) -> Vec<&str> { self.segments .iter() .filter_map(|seg| match seg { - Segment::EnvVar(name) => Some(name.as_str()), - Segment::Literal(_) | Segment::Variable(_) => None, + Segment::Token { + namespace: ns, + name, + } if *ns == namespace => Some(name.as_str()), + Segment::Literal(_) | Segment::Token { .. } => None, }) .collect() } - /// The run variable names referenced by this string, in source order. - #[must_use] - pub fn var_names(&self) -> Vec<&str> { - self.segments - .iter() - .filter_map(|seg| match seg { - Segment::Variable(name) => Some(name.as_str()), - Segment::Literal(_) | Segment::EnvVar(_) => None, - }) - .collect() - } - - /// The raw source string. + /// The raw, unresolved template source. + /// + /// This is a footgun for consumers: passing the raw source downstream + /// leaks `{{ ... }}` tokens as literal text. Resolve via + /// [`InterpString::resolve`] / [`InterpString::resolve_with`] (or + /// substitute via [`InterpString::substitute_with`]) instead. Intentional + /// uses — serialization, error messages, deliberate source preservation — + /// must document themselves with + /// `#[expect(clippy::disallowed_methods, reason = "...")]`. #[must_use] pub fn as_source(&self) -> String { let mut out = String::new(); for seg in &self.segments { match seg { Segment::Literal(text) => out.push_str(text), - Segment::EnvVar(name) => { - out.push_str("{{ env."); - out.push_str(name); - out.push_str(" }}"); - } - Segment::Variable(name) => { - out.push_str("{{ vars."); + Segment::Token { namespace, name } => { + out.push_str("{{ "); + out.push_str(namespace.into()); + out.push('.'); out.push_str(name); out.push_str(" }}"); } @@ -178,101 +275,65 @@ impl InterpString { out } - /// Resolve this string using `lookup`, which should return the current - /// value for a given env var name (or `None` if unset). + /// Fully resolve every token using the lookups in `ctx`. /// - /// On success the caller gets the final string plus provenance describing - /// whether any env var contributed to the value. On failure the caller - /// learns which env var was unresolved. - pub fn resolve(&self, mut lookup: F) -> Result - where - F: FnMut(&str) -> Option, - { + /// Tokens in a namespace `ctx` has no lookup for fail with + /// [`ResolveErrorKind::Unavailable`]: namespace availability is + /// scope-determined, and a token outside its scope must fail loudly + /// rather than pass through as literal text. A lookup miss fails with + /// [`ResolveErrorKind::Missing`] — there is no fallback to the raw + /// source. + pub fn resolve_with(&self, ctx: &mut ResolveCtx<'_>) -> Result { let mut value = String::new(); - let mut used = Vec::new(); + let mut env_names = Vec::new(); + let mut secret_names = Vec::new(); for seg in &self.segments { match seg { Segment::Literal(text) => value.push_str(text), - Segment::EnvVar(name) => { + Segment::Token { namespace, name } => { + let Some(lookup) = ctx.lookup_for(*namespace) else { + return Err(ResolveError::unavailable(*namespace, name)); + }; let Some(resolved) = lookup(name) else { - return Err(ResolveEnvError::missing_env(name)); + return Err(ResolveError::missing(*namespace, name)); }; value.push_str(&resolved); - used.push(name.clone()); - } - Segment::Variable(name) => { - return Err(ResolveEnvError::unsupported_variable(name)); + match namespace { + Namespace::Env => env_names.push(name.clone()), + Namespace::Secrets => secret_names.push(name.clone()), + Namespace::Vars | Namespace::Inputs => {} + } } } } - let provenance = if used.is_empty() { - Provenance::Literal - } else { - Provenance::EnvSourced { names: used } - }; - Ok(Resolved { value, provenance }) + Ok(Resolved { + value, + provenance: Provenance::from_names(env_names, secret_names), + }) } - /// Resolve env and run variable tokens with separate lookup functions. + /// Substitute tokens for the namespaces `ctx` provides, preserving tokens + /// for the namespaces it does not — their resolution happens later, + /// possibly in a different process. /// - /// Variables are non-sensitive, so variable-only interpolation does not - /// mark the value as env-sourced for redaction. - pub fn resolve_with_variables( - &self, - mut env_lookup: F, - mut variable_lookup: G, - ) -> Result - where - F: FnMut(&str) -> Option, - G: FnMut(&str) -> Option, - { - let mut value = String::new(); - let mut used_env = Vec::new(); - for seg in &self.segments { - match seg { - Segment::Literal(text) => value.push_str(text), - Segment::EnvVar(name) => { - let Some(resolved) = env_lookup(name) else { - return Err(ResolveEnvError::missing_env(name)); - }; - value.push_str(&resolved); - used_env.push(name.clone()); - } - Segment::Variable(name) => { - let Some(resolved) = variable_lookup(name) else { - return Err(ResolveEnvError::missing_variable(name)); - }; - value.push_str(&resolved); - } - } - } - - let provenance = if used_env.is_empty() { - Provenance::Literal - } else { - Provenance::EnvSourced { names: used_env } - }; - Ok(Resolved { value, provenance }) - } - - /// Substitute only `{{ vars.* }}` tokens while preserving `{{ env.* }}` - /// tokens for their existing consumption-time env lookup. - pub fn substitute_variables(&self, mut lookup: F) -> Result - where - F: FnMut(&str) -> Option, - { + /// This is the early, server-side pass (`vars`/`inputs`); late-bound + /// namespaces (`env`/`secrets`) survive in token form for their + /// consumption-time [`InterpString::resolve_with`]. + pub fn substitute_with(&self, ctx: &mut ResolveCtx<'_>) -> Result { let mut segments = Vec::new(); for seg in &self.segments { match seg { Segment::Literal(text) => Self::push_literal(&mut segments, text), - Segment::EnvVar(name) => segments.push(Segment::EnvVar(name.clone())), - Segment::Variable(name) => { - let Some(resolved) = lookup(name) else { - return Err(ResolveEnvError::missing_variable(name)); - }; - Self::push_literal(&mut segments, &resolved); - } + Segment::Token { namespace, name } => match ctx.lookup_for(*namespace) { + Some(lookup) => { + let Some(resolved) = lookup(name) else { + return Err(ResolveError::missing(*namespace, name)); + }; + Self::push_literal(&mut segments, &resolved); + } + None => segments.push(seg.clone()), + }, } } if segments.is_empty() { @@ -280,6 +341,67 @@ impl InterpString { } Ok(Self { segments }) } + + /// Resolve in an env-only context, e.g. server-scope settings. + /// + /// `lookup` should return the current value for a given env var name (or + /// `None` if unset). Tokens in any other namespace fail with + /// [`ResolveErrorKind::Unavailable`]. + pub fn resolve(&self, lookup: F) -> Result + where + F: FnMut(&str) -> Option, + { + self.resolve_with(&mut ResolveCtx::new().with_env(lookup)) + } + + /// Resolve in an env-only context, falling back to the raw template + /// source when resolution fails so a missing env var surfaces as a + /// recognizable diagnostic instead of a silently dropped value. + #[expect( + clippy::disallowed_methods, + reason = "intentional raw-source fallback so a missing env var surfaces as a \ + recognizable diagnostic; slated for hard-error semantics in the \ + interpolation unification (D3)" + )] + #[must_use] + pub fn resolve_or_source(&self, lookup: F) -> String + where + F: FnMut(&str) -> Option, + { + self.resolve(lookup) + .map_or_else(|_| self.as_source(), |resolved| resolved.value) + } + + /// Substitute only `{{ vars.* }}` tokens while preserving all other + /// namespaces for their consumption-time resolution. + pub fn substitute_variables(&self, lookup: F) -> Result + where + F: FnMut(&str) -> Option, + { + self.substitute_with(&mut ResolveCtx::new().with_vars(lookup)) + } + + /// Substitute `{{ vars.* }}` tokens inside a plain string, returning the + /// result in source form with all other tokens preserved. + /// + /// This is the string-typed counterpart of + /// [`InterpString::substitute_variables`] for settings fields stored as + /// `String`; it keeps the raw-source round-trip in one audited place. + pub fn substitute_variables_in_str(value: &str, lookup: F) -> Result + where + F: FnMut(&str) -> Option, + { + let parsed = Self::parse(value); + if !parsed.references(Namespace::Vars) { + return Ok(value.to_owned()); + } + #[expect( + clippy::disallowed_methods, + reason = "canonical raw-source round-trip for String-typed settings fields whose \ + remaining tokens resolve downstream" + )] + Ok(parsed.substitute_variables(lookup)?.as_source()) + } } impl From for InterpString { @@ -294,7 +416,7 @@ impl From<&str> for InterpString { } } -/// The outcome of a successful env interpolation resolution. +/// The outcome of a successful interpolation resolution. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Resolved { pub value: String, @@ -304,66 +426,78 @@ pub struct Resolved { /// Provenance metadata for resolved config values. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Provenance { - /// No env var contributed to this value. + /// No env var or secret contributed to this value. Literal, - /// One or more env vars contributed to this value. Used by outward-facing - /// renderers to redact env-sourced values uniformly. - EnvSourced { names: Vec }, + /// One or more env vars and/or secrets contributed to this value. Used by + /// outward-facing renderers to redact sensitive-sourced values uniformly. + /// `vars`/`inputs` are non-sensitive and do not mark a value as sourced. + Sourced { + env_names: Vec, + secret_names: Vec, + }, } -/// An error returned when an env var referenced in a config string is not set. +impl Provenance { + fn from_names(env_names: Vec, secret_names: Vec) -> Self { + if env_names.is_empty() && secret_names.is_empty() { + Self::Literal + } else { + Self::Sourced { + env_names, + secret_names, + } + } + } +} + +/// An error from resolving or substituting interpolation tokens. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct ResolveEnvError { - pub name: String, - pub kind: ResolveEnvErrorKind, +pub struct ResolveError { + pub namespace: Namespace, + pub name: String, + pub kind: ResolveErrorKind, } -impl ResolveEnvError { - fn missing_env(name: &str) -> Self { +impl ResolveError { + fn missing(namespace: Namespace, name: &str) -> Self { Self { + namespace, name: name.to_string(), - kind: ResolveEnvErrorKind::MissingEnv, + kind: ResolveErrorKind::Missing, } } - fn missing_variable(name: &str) -> Self { + fn unavailable(namespace: Namespace, name: &str) -> Self { Self { + namespace, name: name.to_string(), - kind: ResolveEnvErrorKind::MissingVariable, - } - } - - fn unsupported_variable(name: &str) -> Self { - Self { - name: name.to_string(), - kind: ResolveEnvErrorKind::UnsupportedVariable, + kind: ResolveErrorKind::Unavailable, } } } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ResolveEnvErrorKind { - MissingEnv, - MissingVariable, - UnsupportedVariable, +pub enum ResolveErrorKind { + /// The namespace is available in this context but has no value for the + /// referenced name. + Missing, + /// The namespace is not available in this resolution context. + Unavailable, } -impl fmt::Display for ResolveEnvError { +impl fmt::Display for ResolveError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let noun = self.namespace.noun(); + let namespace = self.namespace; match self.kind { - ResolveEnvErrorKind::MissingEnv => write!( + ResolveErrorKind::Missing => write!( f, - "environment variable {:?} referenced by {{{{ env.{} }}}} is not set", + "{noun} {:?} referenced by {{{{ {namespace}.{} }}}} is not set", self.name, self.name ), - ResolveEnvErrorKind::MissingVariable => write!( + ResolveErrorKind::Unavailable => write!( f, - "variable {:?} referenced by {{{{ vars.{} }}}} is not set", - self.name, self.name - ), - ResolveEnvErrorKind::UnsupportedVariable => write!( - f, - "variable {:?} referenced by {{{{ vars.{} }}}} is not supported in this \ + "{noun} {:?} referenced by {{{{ {namespace}.{} }}}} is not supported in this \ interpolation context", self.name, self.name ), @@ -371,10 +505,14 @@ impl fmt::Display for ResolveEnvError { } } -impl std::error::Error for ResolveEnvError {} +impl std::error::Error for ResolveError {} impl Serialize for InterpString { fn serialize(&self, serializer: S) -> Result { + #[expect( + clippy::disallowed_methods, + reason = "serialization round-trips the unresolved template source by design" + )] serializer.serialize_str(&self.as_source()) } } @@ -387,7 +525,10 @@ impl<'de> Deserialize<'de> for InterpString { type Value = InterpString; fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("a string, optionally containing {{ env.NAME }} interpolation tokens") + f.write_str( + "a string, optionally containing {{ env.NAME }}, {{ vars.NAME }}, \ + {{ secrets.NAME }}, or {{ inputs.NAME }} interpolation tokens", + ) } fn visit_str(self, value: &str) -> Result { @@ -404,6 +545,10 @@ impl<'de> Deserialize<'de> for InterpString { } #[cfg(test)] +#[expect( + clippy::disallowed_methods, + reason = "tests assert raw template source round-trips" +)] mod tests { use std::collections::HashMap; @@ -418,31 +563,31 @@ mod tests { } #[test] - fn literal_string_has_no_env_refs() { + fn literal_string_has_no_refs() { let s = InterpString::parse("hello world"); assert!(s.is_literal()); - assert!(!s.references_env()); - assert_eq!(s.env_var_names(), Vec::<&str>::new()); + assert!(!s.references(Namespace::Env)); + assert_eq!(s.names(Namespace::Env), Vec::<&str>::new()); } #[test] fn whole_value_env_reference() { let s = InterpString::parse("{{ env.API_KEY }}"); assert!(!s.is_literal()); - assert_eq!(s.env_var_names(), vec!["API_KEY"]); + assert_eq!(s.names(Namespace::Env), vec!["API_KEY"]); assert_eq!(s.as_source(), "{{ env.API_KEY }}"); } #[test] fn substring_env_reference() { let s = InterpString::parse("Bearer {{ env.TOKEN }}"); - assert_eq!(s.env_var_names(), vec!["TOKEN"]); + assert_eq!(s.names(Namespace::Env), vec!["TOKEN"]); } #[test] fn multi_token_env_reference() { let s = InterpString::parse("{{ env.USER }}@{{ env.HOST }}:{{env.PORT}}"); - assert_eq!(s.env_var_names(), vec!["USER", "HOST", "PORT"]); + assert_eq!(s.names(Namespace::Env), vec!["USER", "HOST", "PORT"]); } #[test] @@ -460,8 +605,9 @@ mod tests { .resolve(lookup_from(&[("API_KEY", "secret-123")])) .unwrap(); assert_eq!(resolved.value, "secret-123"); - assert_eq!(resolved.provenance, Provenance::EnvSourced { - names: vec!["API_KEY".into()], + assert_eq!(resolved.provenance, Provenance::Sourced { + env_names: vec!["API_KEY".into()], + secret_names: vec![], }); } @@ -479,8 +625,9 @@ mod tests { .resolve(lookup_from(&[("USER", "root"), ("HOST", "example.com")])) .unwrap(); assert_eq!(resolved.value, "root@example.com"); - assert_eq!(resolved.provenance, Provenance::EnvSourced { - names: vec!["USER".into(), "HOST".into()], + assert_eq!(resolved.provenance, Provenance::Sourced { + env_names: vec!["USER".into(), "HOST".into()], + secret_names: vec![], }); } @@ -489,6 +636,12 @@ mod tests { let s = InterpString::parse("{{ env.MISSING }}"); let err = s.resolve(lookup_from(&[])).unwrap_err(); assert_eq!(err.name, "MISSING"); + assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.kind, ResolveErrorKind::Missing); + assert_eq!( + err.to_string(), + "environment variable \"MISSING\" referenced by {{ env.MISSING }} is not set" + ); } #[test] @@ -499,6 +652,23 @@ mod tests { assert_eq!(resolved.provenance, Provenance::Literal); } + #[test] + fn unknown_namespace_token_stays_literal() { + for raw in [ + "{{ unknown.NAME }}", + "{{ .leading }}", + "{{ env. }}", + "{{ no_dot }}", + "{{ secrets.bad-name }}", + "{{ if .Values.foo }}", + ] { + let s = InterpString::parse(raw); + assert!(s.is_literal(), "{raw} should stay literal"); + let resolved = s.resolve(lookup_from(&[])).unwrap(); + assert_eq!(resolved.value, raw); + } + } + #[test] fn serde_round_trip_preserves_token_form() { #[derive(Debug, serde::Deserialize, serde::Serialize, PartialEq)] @@ -513,41 +683,65 @@ mod tests { assert_eq!(rendered, input); } + #[test] + fn serde_round_trip_preserves_all_namespaces() { + #[derive(Debug, serde::Deserialize, serde::Serialize, PartialEq)] + struct Wrap { + s: InterpString, + } + + let input = r#"{"s":"{{ env.A }}/{{ vars.B }}/{{ secrets.C }}/{{ inputs.d-key }}"}"#; + let parsed: Wrap = serde_json::from_str(input).unwrap(); + let rendered = serde_json::to_string(&parsed).unwrap(); + assert_eq!(rendered, input); + } + #[test] fn vars_reference_round_trips_source() { let s = InterpString::parse("{{ vars.RUNTIME_TOKEN }}"); - assert_eq!(s.var_names(), vec!["RUNTIME_TOKEN"]); + assert_eq!(s.names(Namespace::Vars), vec!["RUNTIME_TOKEN"]); assert_eq!(s.as_source(), "{{ vars.RUNTIME_TOKEN }}"); } #[test] - fn resolve_with_variables_substitutes_env_and_var_tokens() { + fn resolve_with_substitutes_env_and_var_tokens() { let s = InterpString::parse("https://{{ env.REGION }}.{{ vars.DOMAIN }}"); let resolved = s - .resolve_with_variables( - lookup_from(&[("REGION", "us-east-1")]), - lookup_from(&[("DOMAIN", "example.com")]), + .resolve_with( + &mut ResolveCtx::new() + .with_env(lookup_from(&[("REGION", "us-east-1")])) + .with_vars(lookup_from(&[("DOMAIN", "example.com")])), ) .unwrap(); assert_eq!(resolved.value, "https://us-east-1.example.com"); - assert_eq!(resolved.provenance, Provenance::EnvSourced { - names: vec!["REGION".into()], + assert_eq!(resolved.provenance, Provenance::Sourced { + env_names: vec!["REGION".into()], + secret_names: vec![], }); } #[test] - fn resolve_with_variables_reports_missing_variable() { + fn resolve_with_reports_missing_variable() { let s = InterpString::parse("{{ vars.MISSING }}"); let err = s - .resolve_with_variables(lookup_from(&[]), lookup_from(&[])) + .resolve_with( + &mut ResolveCtx::new() + .with_env(lookup_from(&[])) + .with_vars(lookup_from(&[])), + ) .unwrap_err(); assert_eq!(err.name, "MISSING"); - assert_eq!(err.kind, ResolveEnvErrorKind::MissingVariable); + assert_eq!(err.namespace, Namespace::Vars); + assert_eq!(err.kind, ResolveErrorKind::Missing); + assert_eq!( + err.to_string(), + "variable \"MISSING\" referenced by {{ vars.MISSING }} is not set" + ); } #[test] @@ -557,6 +751,114 @@ mod tests { let err = s.resolve(lookup_from(&[])).unwrap_err(); assert_eq!(err.name, "RUNTIME_TOKEN"); - assert_eq!(err.kind, ResolveEnvErrorKind::UnsupportedVariable); + assert_eq!(err.namespace, Namespace::Vars); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + assert_eq!( + err.to_string(), + "variable \"RUNTIME_TOKEN\" referenced by {{ vars.RUNTIME_TOKEN }} is not supported \ + in this interpolation context" + ); + } + + #[test] + fn env_only_resolution_rejects_secrets_reference() { + let s = InterpString::parse("{{ secrets.API_KEY }}"); + + let err = s.resolve(lookup_from(&[])).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Secrets); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + assert_eq!( + err.to_string(), + "secret \"API_KEY\" referenced by {{ secrets.API_KEY }} is not supported in this \ + interpolation context" + ); + } + + #[test] + fn resolve_with_secrets_tracks_provenance() { + let s = InterpString::parse("Bearer {{ secrets.API_KEY }} via {{ env.PROXY }}"); + + let resolved = s + .resolve_with( + &mut ResolveCtx::new() + .with_env(lookup_from(&[("PROXY", "proxy.internal")])) + .with_secrets(lookup_from(&[("API_KEY", "vault-value")])), + ) + .unwrap(); + + assert_eq!(resolved.value, "Bearer vault-value via proxy.internal"); + assert_eq!(resolved.provenance, Provenance::Sourced { + env_names: vec!["PROXY".into()], + secret_names: vec!["API_KEY".into()], + }); + } + + #[test] + fn resolve_with_inputs_substitutes_without_provenance() { + let s = InterpString::parse("run-{{ inputs.ticket-id }}"); + + let resolved = s + .resolve_with(&mut ResolveCtx::new().with_inputs(lookup_from(&[("ticket-id", "1234")]))) + .unwrap(); + + assert_eq!(resolved.value, "run-1234"); + assert_eq!(resolved.provenance, Provenance::Literal); + } + + #[test] + fn substitute_variables_preserves_late_bound_tokens() { + let s = + InterpString::parse("{{ vars.NAME }}:{{ env.HOME }}:{{ secrets.KEY }}:{{ inputs.id }}"); + + let substituted = s + .substitute_variables(lookup_from(&[("NAME", "fabro")])) + .unwrap(); + + assert_eq!( + substituted.as_source(), + "fabro:{{ env.HOME }}:{{ secrets.KEY }}:{{ inputs.id }}" + ); + } + + #[test] + fn substitute_variables_reports_missing_variable() { + let s = InterpString::parse("{{ vars.MISSING }}"); + + let err = s.substitute_variables(lookup_from(&[])).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Vars); + assert_eq!(err.kind, ResolveErrorKind::Missing); + } + + #[test] + fn substitute_with_merges_adjacent_literals() { + let s = InterpString::parse("a{{ vars.B }}c"); + + let substituted = s + .substitute_with(&mut ResolveCtx::new().with_vars(lookup_from(&[("B", "b")]))) + .unwrap(); + + assert!(substituted.is_literal()); + assert_eq!(substituted.as_source(), "abc"); + } + + #[test] + fn substitute_variables_in_str_round_trips_source() { + let out = InterpString::substitute_variables_in_str( + "{{ vars.NAME }} at {{ env.HOME }}", + lookup_from(&[("NAME", "fabro")]), + ) + .unwrap(); + + assert_eq!(out, "fabro at {{ env.HOME }}"); + } + + #[test] + fn namespace_displays_lowercase() { + assert_eq!(Namespace::Env.to_string(), "env"); + assert_eq!(Namespace::Vars.to_string(), "vars"); + assert_eq!(Namespace::Secrets.to_string(), "secrets"); + assert_eq!(Namespace::Inputs.to_string(), "inputs"); } } diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index 2e087844a..336d36a9e 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -25,7 +25,7 @@ pub use cli::{ CliLoggingSettings, CliNamespace, CliOutputSettings, CliTargetSettings, CliUpdatesSettings, }; pub use duration::{Duration, ParseDurationError}; -pub use interp::{InterpString, Provenance, ResolveEnvError, Resolved}; +pub use interp::{InterpString, Provenance, ResolveCtx, ResolveError, ResolveErrorKind, Resolved}; pub use model_ref::{ AmbiguousModelRef, ModelRef, ModelRegistry, ParseModelRefError, ResolvedModelRef, }; diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index f138157a0..9aabd80f8 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -14,7 +14,7 @@ use serde::ser::SerializeStruct; use serde::{Deserialize, Serialize}; use super::duration::Duration; -use super::interp::{InterpString, ResolveEnvError}; +use super::interp::{InterpString, Namespace, ResolveError}; use super::model_ref::ModelRef; use super::size::Size; @@ -77,7 +77,7 @@ impl Default for RunNamespace { } impl RunNamespace { - pub fn substitute_variables(&mut self, mut lookup: F) -> Result<(), ResolveEnvError> + pub fn substitute_variables(&mut self, mut lookup: F) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -128,7 +128,7 @@ impl RunNamespace { } } -fn substitute_goal(goal: &mut Option, lookup: &mut F) -> Result<(), ResolveEnvError> +fn substitute_goal(goal: &mut Option, lookup: &mut F) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -141,7 +141,7 @@ where fn substitute_option( value: &mut Option, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -154,7 +154,7 @@ where fn substitute_map( values: &mut HashMap, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -164,27 +164,26 @@ where Ok(()) } -fn substitute(value: &mut InterpString, lookup: &mut F) -> Result<(), ResolveEnvError> +fn substitute(value: &mut InterpString, lookup: &mut F) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { - if !value.references_vars() { + if !value.references(Namespace::Vars) { return Ok(()); } *value = value.substitute_variables(lookup)?; Ok(()) } -fn substitute_string(value: &mut String, lookup: &mut F) -> Result<(), ResolveEnvError> +fn substitute_string(value: &mut String, lookup: &mut F) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { if !may_reference_variable(value) { return Ok(()); } - let parsed = InterpString::parse(value); - if parsed.references_vars() { - *value = parsed.substitute_variables(lookup)?.as_source(); + if InterpString::parse(value).references(Namespace::Vars) { + *value = InterpString::substitute_variables_in_str(value, lookup)?; } Ok(()) } @@ -196,7 +195,7 @@ fn may_reference_variable(value: &str) -> bool { fn substitute_option_string( value: &mut Option, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -206,7 +205,7 @@ where } } -fn substitute_string_vec(values: &mut [String], lookup: &mut F) -> Result<(), ResolveEnvError> +fn substitute_string_vec(values: &mut [String], lookup: &mut F) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -219,7 +218,7 @@ where fn substitute_string_map( values: &mut HashMap, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -232,7 +231,7 @@ where fn substitute_mcp_transport( transport: &mut McpTransport, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -251,7 +250,7 @@ where fn substitute_environment( environment: &mut RunEnvironmentSettings, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -271,7 +270,7 @@ where fn substitute_dockerfile_source( source: &mut Option, lookup: &mut F, -) -> Result<(), ResolveEnvError> +) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -283,7 +282,7 @@ where } } -fn substitute_hook_type(hook_type: &mut HookType, lookup: &mut F) -> Result<(), ResolveEnvError> +fn substitute_hook_type(hook_type: &mut HookType, lookup: &mut F) -> Result<(), ResolveError> where F: FnMut(&str) -> Option, { @@ -314,6 +313,10 @@ mod run_namespace_variable_substitution_tests { RunEnvironmentSettings, RunGoal, RunNamespace, RunPrepareSettings, }; + #[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" + )] #[test] fn substitutes_variables_in_interp_and_late_bound_run_strings() { let mut run = RunNamespace { @@ -500,9 +503,9 @@ impl RunIntegrationsGithubSettings { } /// Resolve every `permissions` value's `{{ env.* }}` tokens via - /// `lookup`, falling back to `InterpString::as_source()` when - /// resolution fails so callers see a recognizable diagnostic instead - /// of a silently dropped key. The `lookup` seam keeps tests free of + /// `lookup`, falling back to the raw template source when resolution + /// fails so callers see a recognizable diagnostic instead of a + /// silently dropped key. The `lookup` seam keeps tests free of /// process-env coupling; production callers pass a thin wrapper over /// `std::env::var`. pub fn resolve_permissions(&self, mut lookup: F) -> HashMap @@ -511,12 +514,7 @@ impl RunIntegrationsGithubSettings { { self.permissions .iter() - .map(|(name, value)| { - let resolved = value - .resolve(&mut lookup) - .map_or_else(|_| value.as_source(), |resolved| resolved.value); - (name.clone(), resolved) - }) + .map(|(name, value)| (name.clone(), value.resolve_or_source(&mut lookup))) .collect() } } @@ -875,12 +873,7 @@ impl RunEnvironmentSettings { { self.env .iter() - .map(|(name, value)| { - let resolved = value - .resolve(&mut lookup) - .map_or_else(|_| value.as_source(), |resolved| resolved.value); - (name.clone(), resolved) - }) + .map(|(name, value)| (name.clone(), value.resolve_or_source(&mut lookup))) .collect() } } diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index 8bf16d115..6590a4f17 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -1133,6 +1133,10 @@ mod tests { } } + #[expect( + clippy::disallowed_methods, + reason = "test asserts the raw template source" + )] #[tokio::test] async fn create_persists_normalized_config_and_initial_state() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 8083350be..0a561d9eb 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -560,6 +560,11 @@ 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], diff --git a/lib/crates/fabro-workflow/src/run_materialization.rs b/lib/crates/fabro-workflow/src/run_materialization.rs index f8c7356f5..8b1caf3ba 100644 --- a/lib/crates/fabro-workflow/src/run_materialization.rs +++ b/lib/crates/fabro-workflow/src/run_materialization.rs @@ -4,6 +4,11 @@ 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, diff --git a/lib/crates/fabro-workflow/tests/materialize_run.rs b/lib/crates/fabro-workflow/tests/materialize_run.rs index b2054474c..2c3679809 100644 --- a/lib/crates/fabro-workflow/tests/materialize_run.rs +++ b/lib/crates/fabro-workflow/tests/materialize_run.rs @@ -10,6 +10,10 @@ 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 { @@ -62,6 +66,10 @@ 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 {