From 2bd04c7935e1a5c5b849a67e7c9daf56f92cbb28 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Thu, 18 Jun 2026 10:03:14 -0400 Subject: [PATCH] Demote control-plane config to plain String; native FABRO_WEB_URL read (#510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third reducing PR of the interpolation unification (**D11 resolution (c)**): the control plane never interpolates. `InterpString` is now strictly the user-facing workflow config language; server identity, storage, listen, object-store, and GitHub App identifiers are plain `String`, consumed where needed with no resolution point. ## Demoted to `String` (was `InterpString`) Both the layer and resolved types: - `server.listen.unix.path`, `server.api.url`, `server.web.url` - `server.storage.root`, `server.artifacts.prefix`, `server.slatedb.prefix` - object store: `Local.root`, S3 `bucket` / `region` / `endpoint` (shared by artifacts + slatedb) - `github.app_id` / `client_id` / `slug` **Kept `InterpString`:** `slack.default_channel` (run-time consumption — the one server-defined survivor). `server.listen.tcp.address` stays the `SocketAddr` `parsed_value` special case. ## Native `FABRO_WEB_URL` read Deployment-time late binding now goes through a native env read instead of a `{{ env.* }}` token: `FABRO_WEB_URL` overrides `server.web.url` (**env override > settings literal > default**), applied in `canonical_origin` and reused by the JWT issuer, cookie-secure check, and system-info. `docker/split-web` no longer ferries the value through a settings token (compose still sets the env var). `canonical_origin`'s error message now advertises a knob that is actually true for everyone. ## Behavior change (release notes) - `{{ env.* }}` / `{{ vars.* }}` tokens in the demoted server fields are now **literal text**, not interpolated. The resolve layer emits `warn_if_demoted_template` for every demoted field, so operators with tokens still in server config **fail loud** rather than silently treating the token as a literal. - Operators who relied on env-based storage location should use the existing native `FABRO_STORAGE_DIR` (`--storage-dir`) override. `FABRO_STORAGE_ROOT` promotion is intentionally deferred (not a proven need). ## Cleanup `fabro-server`'s `crate::interp` shrinks to just the process-env lookup facade; `resolve_interp` / `_path` / `_with` and the `AppState::resolve_interp` seam are deleted (nothing resolves server-scope `InterpString` anymore). ## Verification - `cargo build --workspace` ✅ - `cargo +nightly clippy --workspace --all-targets -- -D warnings` ✅ (incl. the `as_source` gate) - `cargo +nightly fmt --check --all` ✅ - `cargo nextest run --workspace`: 6305 passed; added two tests covering the `FABRO_WEB_URL` override precedence (env-wins and settings-literal fallback). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- docker/split-web/settings.toml | 4 +- .../fabro-cli/src/commands/run/runner.rs | 14 +- lib/crates/fabro-cli/src/local_server.rs | 50 +----- lib/crates/fabro-cli/src/user_config.rs | 63 +++----- lib/crates/fabro-config/src/layers/server.rs | 26 +-- lib/crates/fabro-config/src/resolve/mod.rs | 4 +- lib/crates/fabro-config/src/resolve/server.rs | 148 +++++++++++------- lib/crates/fabro-config/src/tests/combine.rs | 5 +- .../fabro-config/src/tests/resolve_root.rs | 6 +- .../fabro-config/src/tests/resolve_server.rs | 48 +++--- lib/crates/fabro-server/src/auth/cli_flow.rs | 10 +- lib/crates/fabro-server/src/canonical_host.rs | 2 +- .../fabro-server/src/canonical_origin.rs | 36 +++-- lib/crates/fabro-server/src/diagnostics.rs | 23 +-- lib/crates/fabro-server/src/install.rs | 12 +- lib/crates/fabro-server/src/interp.rs | 53 ++----- lib/crates/fabro-server/src/jwt_auth.rs | 21 +-- lib/crates/fabro-server/src/serve.rs | 106 +++++-------- lib/crates/fabro-server/src/server.rs | 47 +++--- .../fabro-server/src/server/handler/runs.rs | 13 +- .../fabro-server/src/server/handler/system.rs | 40 ++--- lib/crates/fabro-server/src/server/tests.rs | 53 +++++-- lib/crates/fabro-server/src/web_auth.rs | 31 +--- .../fabro-server/tests/it/api/system.rs | 10 +- lib/crates/fabro-types/src/dense.rs | 18 +-- lib/crates/fabro-types/src/settings/server.rs | 64 +++----- 26 files changed, 364 insertions(+), 543 deletions(-) diff --git a/docker/split-web/settings.toml b/docker/split-web/settings.toml index f9159278e..1f370f998 100644 --- a/docker/split-web/settings.toml +++ b/docker/split-web/settings.toml @@ -1,8 +1,10 @@ _version = 1 +# server.web.url is provided at runtime by the FABRO_WEB_URL environment +# variable (set in docker-compose.split-web.yaml), read natively by the server. +# It overrides the default below, so no value is needed here. [server.web] enabled = true -url = "{{ env.FABRO_WEB_URL }}" [server.auth] methods = ["dev-token"] diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 12e550b03..9eba9a3d5 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -19,7 +19,6 @@ use fabro_model::Catalog; use fabro_server::run_tool_manifest; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_tool::fabro_client::ClientBackend; -use fabro_types::settings::InterpString; use fabro_types::settings::run::{RunMode, RunNamespace}; use fabro_types::{ ArtifactUpload, EventBody, FailureReason, Principal, RunBlobId, RunEvent, RunId, @@ -1108,11 +1107,6 @@ fn stamp_system_worker(mut event: RunEvent) -> 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>, @@ -1123,12 +1117,8 @@ fn maybe_build_github_credentials( let strategy = server_ns .map(|server| server.integrations.github.strategy) .unwrap_or_default(); - let app_id = server_ns - .and_then(|server| server.integrations.github.app_id.as_ref()) - .map(InterpString::as_source); - let app_slug = server_ns - .and_then(|server| server.integrations.github.slug.as_ref()) - .map(InterpString::as_source); + let app_id = server_ns.and_then(|server| server.integrations.github.app_id.clone()); + let app_slug = server_ns.and_then(|server| server.integrations.github.slug.clone()); if requires_github_credentials(resolved_run) { return build_github_credentials(strategy, app_id.as_deref(), app_slug.as_deref(), vault); diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 7797ff37a..366d39104 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -4,11 +4,10 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use fabro_config::bind::BindRequest; -use fabro_config::user::default_storage_dir; use fabro_server::serve::resolve_bind_request_from_server_settings; use fabro_types::ServerSettings; +use fabro_types::settings::ServerAuthMethod; use fabro_types::settings::server::LogDestination; -use fabro_types::settings::{InterpString, ServerAuthMethod}; use fabro_util::error::SharedError; use crate::user_config; @@ -75,42 +74,8 @@ impl LocalServerConfig { } pub(crate) fn storage_dir_from_toml(source: &str) -> Result { - storage_dir_from_toml_with_lookup(source, &process_env_var) -} - -#[expect( - clippy::disallowed_methods, - reason = "Local server config interpolation owns a process-env lookup facade for {{ env.* }} values." -)] -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, -) -> Result { let document: toml::Value = toml::from_str(source).context("failed to parse settings file")?; - let storage_root = string_at_path(&document, &["server", "storage", "root"]).map_or_else( - || InterpString::parse(&default_storage_dir().to_string_lossy()), - |root| InterpString::parse(&root), - ); - let resolved_root = storage_root - .resolve(lookup) - .with_context(|| format!("failed to resolve {}", storage_root.as_source()))?; - Ok(PathBuf::from(resolved_root.value)) -} - -fn string_at_path(document: &toml::Value, path: &[&str]) -> Option { - let mut current = document; - for segment in path { - current = current.get(*segment)?; - } - current.as_str().map(str::to_owned) + Ok(user_config::storage_dir_from_document(&document, None)) } #[cfg(test)] @@ -119,7 +84,7 @@ mod tests { use fabro_config::user::default_storage_dir; - use super::{storage_dir_from_toml, storage_dir_from_toml_with_lookup}; + use super::storage_dir_from_toml; #[test] fn storage_dir_from_toml_reads_explicit_root_without_full_server_resolution() { @@ -144,18 +109,17 @@ root = "/srv/fabro" } #[test] - fn storage_dir_from_toml_resolves_env_interpolation() { - let path = storage_dir_from_toml_with_lookup( + fn storage_dir_from_toml_keeps_template_token_literal() { + let path = storage_dir_from_toml( r#" _version = 1 [server.storage] root = "{{ env.FABRO_STORAGE_ROOT }}" "#, - &|name| (name == "FABRO_STORAGE_ROOT").then_some("/srv/fabro".to_string()), ) - .expect("storage root should resolve"); + .expect("storage root should parse"); - assert_eq!(path, PathBuf::from("/srv/fabro")); + assert_eq!(path, PathBuf::from("{{ env.FABRO_STORAGE_ROOT }}")); } } diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 9999da69b..f93227ca9 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -9,9 +9,9 @@ use fabro_config::{ CliLayer, LogFilter, ParseError, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder, }; use fabro_static::EnvVars; +use fabro_types::settings::RunNamespace; use fabro_types::settings::cli::CliTargetSettings; use fabro_types::settings::server::LogDestination; -use fabro_types::settings::{InterpString, RunNamespace}; use fabro_types::{ServerSettings, UserSettings}; use fabro_util::error::SharedError; use fabro_util::version::FABRO_VERSION; @@ -36,7 +36,7 @@ pub(crate) fn load_resolved_settings( ) -> anyhow::Result { let document = load_settings_document(config_path)?; let storage_override = storage_dir.map(Path::to_path_buf); - let storage_dir = storage_dir_from_document(&document, storage_dir)?; + let storage_dir = storage_dir_from_document(&document, storage_dir); let pre_tracing_config = pre_tracing_config_from_document(&document)?; let run_settings = load_run_settings(config_path).map_err(SharedError::new); let server_settings = load_server_settings(config_path) @@ -168,19 +168,20 @@ fn log_destination_at_path( .map(Some) } -fn storage_dir_from_document( +pub(crate) fn storage_dir_from_document( document: &toml::Value, storage_dir: Option<&Path>, -) -> anyhow::Result { - storage_dir_from_document_with_lookup(document, storage_dir, &process_env_var) -} +) -> PathBuf { + if let Some(dir) = storage_dir { + return dir.to_path_buf(); + } -#[expect( - clippy::disallowed_methods, - reason = "CLI settings loading owns the process-env facade for interpolation." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() + // `server.storage.root` is plain control-plane config and does not + // interpolate; the FABRO_STORAGE_DIR-backed `storage_dir` argument above is + // the deployment-time override. + let storage_root = string_at_path(document, &["server", "storage", "root"]) + .unwrap_or_else(|| default_storage_dir().to_string_lossy().into_owned()); + PathBuf::from(storage_root) } #[expect( @@ -191,23 +192,6 @@ fn process_env_var_os(name: &str) -> Option { std::env::var_os(name) } -fn storage_dir_from_document_with_lookup( - document: &toml::Value, - storage_dir: Option<&Path>, - lookup: &dyn Fn(&str) -> Option, -) -> anyhow::Result { - if let Some(dir) = storage_dir { - return Ok(dir.to_path_buf()); - } - - let storage_root = string_at_path(document, &["server", "storage", "root"]).map_or_else( - || InterpString::parse(&default_storage_dir().to_string_lossy()), - |root| InterpString::parse(&root), - ); - let resolved_root = storage_root.resolve(lookup)?; - Ok(PathBuf::from(resolved_root.value)) -} - fn string_at_path(document: &toml::Value, path: &[&str]) -> Option { value_at_path(document, path).and_then(|value| value.as_str().map(str::to_owned)) } @@ -350,7 +334,7 @@ pub(crate) fn load_resolved_settings_from_toml( ) -> anyhow::Result { let document: toml::Value = toml::from_str(source).context("failed to parse settings file")?; let storage_override = storage_dir.map(Path::to_path_buf); - let storage_dir = storage_dir_from_document(&document, storage_dir)?; + let storage_dir = storage_dir_from_document(&document, storage_dir); let pre_tracing_config = pre_tracing_config_from_document(&document)?; let run_settings = RunSettingsBuilder::from_toml_with_catalog( source, @@ -560,7 +544,7 @@ url = "https://configured.example.com" let document = toml::Value::Table(toml::Table::new()); assert_eq!( - storage_dir_from_document(&document, None).unwrap(), + storage_dir_from_document(&document, None), default_storage_dir() ); } @@ -578,13 +562,16 @@ root = "/srv/fabro" .expect("fixture should parse"); assert_eq!( - storage_dir_from_document(&document, None).unwrap(), + storage_dir_from_document(&document, None), PathBuf::from("/srv/fabro") ); } #[test] - fn storage_dir_resolves_env_interpolated_root() { + fn storage_dir_keeps_template_token_literal() { + // server.storage.root is plain control-plane config now: a `{{ env.* }}` + // token is used verbatim, never resolved. Deployment-time overrides go + // through the FABRO_STORAGE_DIR-backed `storage_dir` argument instead. let document: toml::Value = toml::from_str( r#" _version = 1 @@ -594,14 +581,10 @@ root = "{{ env.FABRO_STORAGE_ROOT }}" "#, ) .expect("fixture should parse"); - let temp = tempfile::tempdir().unwrap(); assert_eq!( - storage_dir_from_document_with_lookup(&document, None, &|name| { - (name == "FABRO_STORAGE_ROOT").then(|| temp.path().display().to_string()) - }) - .unwrap(), - temp.path() + storage_dir_from_document(&document, None), + PathBuf::from("{{ env.FABRO_STORAGE_ROOT }}") ); } @@ -630,7 +613,7 @@ root = "/srv/fabro" .expect("settings document should load"); assert_eq!( - storage_dir_from_document(&document, None).unwrap(), + storage_dir_from_document(&document, None), PathBuf::from("/srv/fabro") ); } diff --git a/lib/crates/fabro-config/src/layers/server.rs b/lib/crates/fabro-config/src/layers/server.rs index b68277d9e..fb5cde79f 100644 --- a/lib/crates/fabro-config/src/layers/server.rs +++ b/lib/crates/fabro-config/src/layers/server.rs @@ -46,7 +46,7 @@ pub enum ServerListenLayer { }, Unix { #[serde(default)] - path: Option, + path: Option, }, } @@ -57,7 +57,7 @@ pub enum ServerListenLayer { #[serde(deny_unknown_fields)] pub struct ServerApiLayer { #[serde(default, skip_serializing_if = "Option::is_none")] - pub url: Option, + pub url: Option, } /// `[server.web]` — web surface settings. @@ -67,7 +67,7 @@ pub struct ServerWebLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub enabled: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub url: Option, + pub url: Option, } /// `[server.auth]` — cohesive server auth surface. @@ -122,7 +122,7 @@ pub struct ServerSandboxProviderLayer { #[serde(deny_unknown_fields)] pub struct ServerStorageLayer { #[serde(default, skip_serializing_if = "Option::is_none")] - pub root: Option, + pub root: Option, } /// `[server.artifacts]` — object-store-backed artifact storage. @@ -132,7 +132,7 @@ pub struct ServerArtifactsLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub provider: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub prefix: Option, + pub prefix: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub local: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -146,7 +146,7 @@ pub struct ServerSlateDbLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub provider: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub prefix: Option, + pub prefix: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub flush_interval: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -163,18 +163,18 @@ pub struct ObjectStoreLocalLayer { /// Overrides the default root, which otherwise falls back to /// `{server.storage.root}/objects/{domain}`. #[serde(default, skip_serializing_if = "Option::is_none")] - pub root: Option, + pub root: Option, } #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct ObjectStoreS3Layer { #[serde(default, skip_serializing_if = "Option::is_none")] - pub bucket: Option, + pub bucket: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub region: Option, + pub region: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub endpoint: Option, + pub endpoint: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub path_style: Option, } @@ -218,11 +218,11 @@ pub struct GithubIntegrationLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub strategy: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub app_id: Option, + pub app_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub client_id: Option, + pub client_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub slug: Option, + pub slug: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub webhooks: Option, } diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index 0432ca766..17e7b4b15 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -69,8 +69,8 @@ pub(crate) fn parse_socket_addr( } } -pub(crate) fn default_interp(path: impl AsRef) -> InterpString { - InterpString::parse(&path.as_ref().to_string_lossy()) +pub(crate) fn default_string(path: impl AsRef) -> String { + path.as_ref().to_string_lossy().into_owned() } /// Warn when a field demoted out of the interpolation set (D2) still contains diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index 17ab3ff0b..cfeffc415 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -1,4 +1,5 @@ -use fabro_types::settings::InterpString; +use std::path::Path; + use fabro_types::settings::server::{ GithubIntegrationSettings, GithubIntegrationStrategy, IntegrationWebhooksSettings, ObjectStoreProvider, ObjectStoreSettings, ServerApiSettings, ServerArtifactsSettings, @@ -10,7 +11,10 @@ use fabro_types::settings::server::{ }; use fabro_util::Home; -use super::{ResolveError, default_interp, parse_socket_addr, require_interp}; +use super::{ + ResolveError, default_string, parse_socket_addr, require_interp, require_string, + warn_if_demoted_template, +}; use crate::user::default_storage_dir; use crate::{ IntegrationWebhooksLayer, ObjectStoreLocalLayer, ObjectStoreS3Layer, ServerApiLayer, @@ -27,11 +31,12 @@ pub fn resolve_server(layer: &ServerLayer, errors: &mut Vec) -> Se let integrations = resolve_integrations(layer.integrations.as_ref()); validate_github_webhook_strategy(&integrations, layer.api.as_ref(), errors); + let api_url = layer.api.as_ref().and_then(|api| api.url.clone()); + warn_if_demoted_template("server.api.url", api_url.as_deref()); + ServerNamespace { listen, - api: ServerApiSettings { - url: layer.api.as_ref().and_then(|api| api.url.clone()), - }, + api: ServerApiSettings { url: api_url }, web, auth, sandbox: resolve_sandbox(layer.sandbox.as_ref()), @@ -87,10 +92,10 @@ fn resolve_sandbox_provider( } fn resolve_storage(layer: Option<&ServerStorageLayer>) -> ServerStorageSettings { + let root = layer.and_then(|storage| storage.root.as_deref()); + warn_if_demoted_template("server.storage.root", root); ServerStorageSettings { - root: layer - .and_then(|storage| storage.root.clone()) - .unwrap_or_else(|| default_interp(default_storage_dir())), + root: root.map_or_else(|| default_string(default_storage_dir()), str::to_owned), } } @@ -100,13 +105,16 @@ fn resolve_listen( ) -> ServerListenSettings { match layer { None => ServerListenSettings::Unix { - path: default_interp(Home::from_env().socket_path()), - }, - Some(ServerListenLayer::Unix { path }) => ServerListenSettings::Unix { - path: path - .clone() - .unwrap_or_else(|| default_interp(Home::from_env().socket_path())), + path: default_string(Home::from_env().socket_path()), }, + Some(ServerListenLayer::Unix { path }) => { + warn_if_demoted_template("server.listen.path", path.as_deref()); + ServerListenSettings::Unix { + path: path + .clone() + .unwrap_or_else(|| default_string(Home::from_env().socket_path())), + } + } Some(ServerListenLayer::Tcp { address }) => { let address = parse_socket_addr( &require_interp(address.as_ref(), "server.listen.address", errors), @@ -121,14 +129,17 @@ fn resolve_listen( fn resolve_web(layer: Option<&ServerWebLayer>) -> ServerWebSettings { let layer = layer.expect("defaults.toml should provide server.web defaults"); + let url = layer + .url + .clone() + .expect("defaults.toml should provide server.web.url"); + warn_if_demoted_template("server.web.url", Some(url.as_str())); + ServerWebSettings { enabled: layer .enabled .expect("defaults.toml should provide server.web.enabled"), - url: layer - .url - .clone() - .expect("defaults.toml should provide server.web.url"), + url, } } @@ -207,18 +218,21 @@ fn validate_github_webhook_strategy( fn resolve_artifacts( layer: Option<&ServerArtifactsLayer>, - storage_root: &InterpString, + storage_root: &str, errors: &mut Vec, ) -> ServerArtifactsSettings { let provider = layer .and_then(|artifacts| artifacts.provider) .expect("defaults.toml should provide server.artifacts.provider"); + let prefix = layer + .and_then(|artifacts| artifacts.prefix.clone()) + .expect("defaults.toml should provide server.artifacts.prefix"); + warn_if_demoted_template("server.artifacts.prefix", Some(prefix.as_str())); + ServerArtifactsSettings { - prefix: layer - .and_then(|artifacts| artifacts.prefix.clone()) - .expect("defaults.toml should provide server.artifacts.prefix"), - store: resolve_object_store( + prefix, + store: resolve_object_store( provider, layer.and_then(|artifacts| artifacts.local.as_ref()), layer.and_then(|artifacts| artifacts.s3.as_ref()), @@ -231,7 +245,7 @@ fn resolve_artifacts( fn resolve_slatedb( layer: Option<&ServerSlateDbLayer>, - storage_root: &InterpString, + storage_root: &str, errors: &mut Vec, ) -> ServerSlateDbSettings { let provider = layer @@ -250,10 +264,13 @@ fn resolve_slatedb( ); } + let prefix = layer + .and_then(|slatedb| slatedb.prefix.clone()) + .expect("defaults.toml should provide server.slatedb.prefix"); + warn_if_demoted_template("server.slatedb.prefix", Some(prefix.as_str())); + ServerSlateDbSettings { - prefix: layer - .and_then(|slatedb| slatedb.prefix.clone()) - .expect("defaults.toml should provide server.slatedb.prefix"), + prefix, store: resolve_object_store( provider, layer.and_then(|slatedb| slatedb.local.as_ref()), @@ -274,59 +291,70 @@ fn resolve_object_store( provider: ObjectStoreProvider, local: Option<&ObjectStoreLocalLayer>, s3: Option<&ObjectStoreS3Layer>, - storage_root: &InterpString, + storage_root: &str, path_prefix: &str, errors: &mut Vec, ) -> ObjectStoreSettings { match provider { - ObjectStoreProvider::Local => ObjectStoreSettings::Local { - root: local - .and_then(|local| local.root.clone()) - .unwrap_or_else(|| storage_root.clone()), - }, + ObjectStoreProvider::Local => { + let root = local.and_then(|local| local.root.as_deref()); + warn_if_demoted_template(&format!("{path_prefix}.local.root"), root); + ObjectStoreSettings::Local { + root: root.map_or_else(|| storage_root.to_owned(), str::to_owned), + } + } ObjectStoreProvider::S3 => { - let bucket = require_interp( - s3.and_then(|s3| s3.bucket.as_ref()), - &format!("{path_prefix}.s3.bucket"), - errors, - ); - let region = require_interp( - s3.and_then(|s3| s3.region.as_ref()), - &format!("{path_prefix}.s3.region"), - errors, - ); + let bucket_field = format!("{path_prefix}.s3.bucket"); + let region_field = format!("{path_prefix}.s3.region"); + let endpoint_field = format!("{path_prefix}.s3.endpoint"); + let bucket = + require_string(s3.and_then(|s3| s3.bucket.as_ref()), &bucket_field, errors); + let region = + require_string(s3.and_then(|s3| s3.region.as_ref()), ®ion_field, errors); + let endpoint = s3.and_then(|s3| s3.endpoint.clone()); + warn_if_demoted_template(&bucket_field, Some(bucket.as_str())); + warn_if_demoted_template(®ion_field, Some(region.as_str())); + warn_if_demoted_template(&endpoint_field, endpoint.as_deref()); ObjectStoreSettings::S3 { bucket, region, - endpoint: s3.and_then(|s3| s3.endpoint.clone()), + endpoint, path_style: s3.and_then(|s3| s3.path_style).unwrap_or(false), } } } } -#[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('/'); - InterpString::parse(&format!("{root}/objects/{domain}")) +fn object_store_default_root(storage_root: &str, domain: &str) -> String { + Path::new(storage_root) + .join("objects") + .join(domain) + .to_string_lossy() + .into_owned() } fn resolve_integrations(layer: Option<&ServerIntegrationsLayer>) -> ServerIntegrationsSettings { ServerIntegrationsSettings { github: layer .and_then(|integrations| integrations.github.as_ref()) - .map(|github| GithubIntegrationSettings { - enabled: github.enabled.unwrap_or(true), - strategy: github.strategy.unwrap_or_default(), - app_id: github.app_id.clone(), - client_id: github.client_id.clone(), - slug: github.slug.clone(), - webhooks: github.webhooks.as_ref().map(resolve_github_webhooks), + .map(|github| { + warn_if_demoted_template( + "server.integrations.github.app_id", + github.app_id.as_deref(), + ); + warn_if_demoted_template( + "server.integrations.github.client_id", + github.client_id.as_deref(), + ); + warn_if_demoted_template("server.integrations.github.slug", github.slug.as_deref()); + GithubIntegrationSettings { + enabled: github.enabled.unwrap_or(true), + strategy: github.strategy.unwrap_or_default(), + app_id: github.app_id.clone(), + client_id: github.client_id.clone(), + slug: github.slug.clone(), + webhooks: github.webhooks.as_ref().map(resolve_github_webhooks), + } }) .unwrap_or_default(), slack: layer diff --git a/lib/crates/fabro-config/src/tests/combine.rs b/lib/crates/fabro-config/src/tests/combine.rs index fb0db5848..ff95a8aec 100644 --- a/lib/crates/fabro-config/src/tests/combine.rs +++ b/lib/crates/fabro-config/src/tests/combine.rs @@ -315,9 +315,6 @@ bucket = "higher-bucket" let merged = higher.combine(lower); let s3 = merged.server.unwrap().artifacts.unwrap().s3.unwrap(); - assert_eq!( - s3.bucket.map(|bucket| bucket.as_source()), - Some("higher-bucket".to_string()) - ); + assert_eq!(s3.bucket, Some("higher-bucket".to_string())); assert_eq!(s3.region, None); } diff --git a/lib/crates/fabro-config/src/tests/resolve_root.rs b/lib/crates/fabro-config/src/tests/resolve_root.rs index ae5c6df81..64bac4895 100644 --- a/lib/crates/fabro-config/src/tests/resolve_root.rs +++ b/lib/crates/fabro-config/src/tests/resolve_root.rs @@ -79,10 +79,6 @@ 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#" @@ -115,7 +111,7 @@ name = "gpt-5" "resolved project settings should not expose deprecated directory" ); assert_eq!(workflow_settings.workflow.graph, "graphs/workflow.dot"); - assert_eq!(server.server.storage.root.as_source(), "/srv/fabro"); + assert_eq!(server.server.storage.root, "/srv/fabro"); assert_eq!( workflow_settings.run.model.provider.as_deref(), Some("openai") diff --git a/lib/crates/fabro-config/src/tests/resolve_server.rs b/lib/crates/fabro-config/src/tests/resolve_server.rs index 051aed809..a8619f3be 100644 --- a/lib/crates/fabro-config/src/tests/resolve_server.rs +++ b/lib/crates/fabro-config/src/tests/resolve_server.rs @@ -3,7 +3,6 @@ reason = "sync test fixture setup and raw template source assertions; not on a Tokio path" )] -use fabro_types::settings::InterpString; use fabro_types::settings::server::{ GithubIntegrationStrategy, LogDestination, ObjectStoreSettings, ServerAuthMethod, ServerListenSettings, ServerNamespace, @@ -61,20 +60,17 @@ fn resolves_server_defaults_from_empty_settings() { let settings = resolve_server(&empty_settings_with_auth_methods()); assert_eq!( - settings.storage.root.as_source(), + settings.storage.root, default_storage_dir().to_string_lossy() ); assert!(settings.web.enabled); - assert_eq!(settings.web.url.as_source(), "http://localhost:3000"); + assert_eq!(settings.web.url, "http://localhost:3000"); assert_eq!(settings.scheduler.max_concurrent_runs, 5); assert_eq!(settings.logging.destination, LogDestination::File); match settings.listen { ServerListenSettings::Unix { path } => { - assert_eq!( - path.as_source(), - Home::from_env().socket_path().to_string_lossy() - ); + assert_eq!(path, Home::from_env().socket_path().to_string_lossy()); } ServerListenSettings::Tcp { .. } => panic!("expected default listen transport to be unix"), } @@ -82,7 +78,7 @@ fn resolves_server_defaults_from_empty_settings() { match settings.artifacts.store { ObjectStoreSettings::Local { root } => { assert_eq!( - root.as_source(), + root, default_storage_dir() .join("objects") .join("artifacts") @@ -91,12 +87,12 @@ fn resolves_server_defaults_from_empty_settings() { } ObjectStoreSettings::S3 { .. } => panic!("expected local artifact store by default"), } - assert_eq!(settings.artifacts.prefix.as_source(), ""); + assert_eq!(settings.artifacts.prefix, ""); match settings.slatedb.store { ObjectStoreSettings::Local { root } => { assert_eq!( - root.as_source(), + root, default_storage_dir() .join("objects") .join("slatedb") @@ -278,7 +274,7 @@ root = "/srv/fabro" let context = fabro_config::ServerSettingsBuilder::from_layer(&settings) .expect("settings should resolve"); - assert_eq!(context.server.storage.root.as_source(), "/srv/fabro"); + assert_eq!(context.server.storage.root, "/srv/fabro"); } #[test] @@ -301,7 +297,7 @@ root = "/srv/from-home" with_var("FABRO_HOME", Some(home.path()), || { let settings = fabro_config::ServerSettingsBuilder::load_default().expect("settings should resolve"); - assert_eq!(settings.server.storage.root.as_source(), "/srv/from-home"); + assert_eq!(settings.server.storage.root, "/srv/from-home"); }); } @@ -367,22 +363,22 @@ slug = "fabro-app" match settings.listen { ServerListenSettings::Unix { path } => { - assert_eq!(path, InterpString::parse("{{ env.FABRO_SOCKET }}")); + assert_eq!(path, "{{ env.FABRO_SOCKET }}"); } ServerListenSettings::Tcp { .. } => panic!("expected unix listen transport"), } assert_eq!( - settings.integrations.github.app_id, - Some(InterpString::parse("{{ env.GITHUB_APP_ID }}")) + settings.integrations.github.app_id.as_deref(), + Some("{{ env.GITHUB_APP_ID }}") ); assert_eq!( - settings.integrations.github.client_id, - Some(InterpString::parse("{{ env.GITHUB_CLIENT_ID }}")) + settings.integrations.github.client_id.as_deref(), + Some("{{ env.GITHUB_CLIENT_ID }}") ); assert_eq!( - settings.integrations.github.slug, - Some(InterpString::parse("fabro-app")) + settings.integrations.github.slug.as_deref(), + Some("fabro-app") ); } @@ -522,7 +518,7 @@ fn resolve_storage_root_defaults_with_minimal_server_auth_methods() { let settings = ServerSettingsBuilder::from_layer(&empty_settings_with_auth_methods()) .expect("default server settings should resolve"); assert_eq!( - settings.server.storage.root.as_source(), + settings.server.storage.root, default_storage_dir().to_string_lossy() ); } @@ -540,11 +536,14 @@ root = "/srv/fabro" let settings = ServerSettingsBuilder::from_layer(&file).expect("server settings should resolve"); - assert_eq!(settings.server.storage.root.as_source(), "/srv/fabro"); + assert_eq!(settings.server.storage.root, "/srv/fabro"); } #[test] -fn resolve_storage_root_preserves_env_interpolation() { +fn resolve_storage_root_keeps_template_token_literal() { + // `server.storage.root` is plain control-plane config now: a `{{ env.* }}` + // token is stored verbatim and never interpolated (deployment-time + // overrides go through FABRO_STORAGE_DIR, not a settings token). let file = parse( r#" _version = 1 @@ -556,10 +555,7 @@ root = "{{ env.FABRO_STORAGE_ROOT }}" let settings = ServerSettingsBuilder::from_layer(&file).expect("server settings should resolve"); - assert_eq!( - settings.server.storage.root, - InterpString::parse("{{ env.FABRO_STORAGE_ROOT }}") - ); + assert_eq!(settings.server.storage.root, "{{ env.FABRO_STORAGE_ROOT }}"); } #[test] diff --git a/lib/crates/fabro-server/src/auth/cli_flow.rs b/lib/crates/fabro-server/src/auth/cli_flow.rs index 236e126d6..142dd7b66 100644 --- a/lib/crates/fabro-server/src/auth/cli_flow.rs +++ b/lib/crates/fabro-server/src/auth/cli_flow.rs @@ -770,12 +770,10 @@ fn github_auth_not_configured() -> Response { ) } -fn resolved_web_url(state: &AppState) -> Option { - state.canonical_origin().ok() -} - fn session_cookie_secure(state: &AppState) -> bool { - resolved_web_url(state).is_some_and(|web_url| web_url.starts_with("https://")) + state + .canonical_origin() + .is_ok_and(|web_url| web_url.starts_with("https://")) } fn eligible_session(session: Option<&SessionCookie>) -> Option<&SessionCookie> { @@ -810,7 +808,7 @@ fn confirm_resume_origin_is_valid(headers: &HeaderMap, state: &AppState) -> bool let Ok(origin) = origin.to_str() else { return false; }; - let Some(web_url) = resolved_web_url(state) else { + let Ok(web_url) = state.canonical_origin() else { return false; }; let Ok(origin_url) = Url::parse(origin) else { diff --git a/lib/crates/fabro-server/src/canonical_host.rs b/lib/crates/fabro-server/src/canonical_host.rs index cc44f1629..f3adcd45b 100644 --- a/lib/crates/fabro-server/src/canonical_host.rs +++ b/lib/crates/fabro-server/src/canonical_host.rs @@ -257,7 +257,7 @@ url = "{web_url}" fn invalid_canonical_origin_state() -> Arc { crate::test_support::test_app_state_with_env_lookup( - settings_with_web_url("{{ env.FABRO_WEB_URL }}"), + settings_with_web_url("http://valid.example.com"), RunLayer::default(), 5, |_| Some("/relative".to_string()), diff --git a/lib/crates/fabro-server/src/canonical_origin.rs b/lib/crates/fabro-server/src/canonical_origin.rs index 4231fd9fa..6f9996790 100644 --- a/lib/crates/fabro-server/src/canonical_origin.rs +++ b/lib/crates/fabro-server/src/canonical_origin.rs @@ -3,26 +3,40 @@ reason = "Canonical origin validation handles the public server origin; it is not credential-bearing log output." )] +use fabro_static::EnvVars; 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, ) -> Result { - let value = resolved - .web - .url - .resolve(|name| env_lookup(name)) - .map_err(|_| canonical_origin_error(&resolved.web.url.as_source()))? - .value; + let value = effective_web_url(resolved, |name| env_lookup(name)); + canonical_origin_from_effective_web_url(&value) +} - validate_public_url(&value).map_err(|_| canonical_origin_error(&value)) +pub(crate) fn canonical_origin_from_effective_web_url(value: &str) -> Result { + validate_public_url(value).map_err(|_| canonical_origin_error(value)) +} + +/// The effective `server.web.url`. +/// +/// `web.url` is plain control-plane config that never interpolates `{{ env.* +/// }}` tokens. Deployment-time late binding instead goes through the native +/// `FABRO_WEB_URL` process-env read: the env override wins when set (and +/// non-empty), otherwise the literal settings value is used. +/// +/// Generic over the lookup so both the `EnvLookup`-backed callers and +/// `resolve_jwt_issuer` (which threads a bare `Fn`) share this one definition +/// of the override precedence. +pub(crate) fn effective_web_url( + resolved: &ServerNamespace, + lookup: impl Fn(&str) -> Option, +) -> String { + lookup(EnvVars::FABRO_WEB_URL) + .filter(|value| !value.trim().is_empty()) + .unwrap_or_else(|| resolved.web.url.clone()) } fn canonical_origin_error(value: &str) -> String { diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 4ce1c9837..ff4a64711 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -10,8 +10,8 @@ use fabro_model::{Catalog, ProviderId}; use fabro_redact::redact_string; use fabro_sandbox::daytona; use fabro_static::EnvVars; +use fabro_types::settings::ServerAuthMethod; use fabro_types::settings::server::GithubIntegrationStrategy; -use fabro_types::settings::{InterpString, ServerAuthMethod}; use fabro_util::check_report::{CheckDetail, CheckResult, CheckSection, CheckStatus}; use fabro_util::dev_token::validate_dev_token_format; use fabro_util::session_secret; @@ -327,11 +327,6 @@ 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 { @@ -446,20 +441,8 @@ async fn check_github_app(state: &AppState) -> CheckResult { }; } - let app_id = settings - .server - .integrations - .github - .app_id - .as_ref() - .map(InterpString::as_source); - let slug = settings - .server - .integrations - .github - .slug - .as_ref() - .map(InterpString::as_source); + let app_id = settings.server.integrations.github.app_id.clone(); + let slug = settings.server.integrations.github.slug.clone(); let private_key_raw = state.vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY); let client_id = settings.server.integrations.github.client_id.is_some(); let client_secret = state diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index f01788458..91ed5bd6e 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -31,7 +31,6 @@ use fabro_sandbox::daytona; use fabro_static::EnvVars; use fabro_store::ArtifactStore; use fabro_types::ServerSettings; -use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::EnvironmentProvider; use fabro_types::settings::server::ObjectStoreSettings; use fabro_types::settings::{is_wildcard_host, validate_public_url_with_label}; @@ -1188,8 +1187,8 @@ fn object_store_validation_settings( match selection { InstallObjectStoreState::Local { .. } => None, InstallObjectStoreState::S3 { bucket, region, .. } => Some(ObjectStoreSettings::S3 { - bucket: InterpString::parse(bucket), - region: InterpString::parse(region), + bucket: bucket.clone(), + region: region.clone(), endpoint: None, path_style: false, }), @@ -2277,10 +2276,8 @@ async fn write_artifact_store_metadata( settings: &ServerSettings, storage_dir: &Path, ) -> anyhow::Result<()> { - use fabro_types::settings::interp::InterpString; - let mut settings = settings.clone(); - settings.server.storage.root = InterpString::parse(&storage_dir.display().to_string()); + settings.server.storage.root = storage_dir.display().to_string(); let (object_store, prefix) = serve::build_artifact_object_store(&settings.server)?; let artifact_store = ArtifactStore::new(object_store, prefix); artifact_store.write_metadata(FABRO_VERSION).await?; @@ -2581,8 +2578,7 @@ methods = ["dev-token"] .unwrap(); let mut overridden = settings.clone(); - overridden.server.storage.root = - fabro_types::settings::interp::InterpString::parse(&dir.path().display().to_string()); + overridden.server.storage.root = dir.path().display().to_string(); let (object_store, prefix) = crate::serve::build_artifact_object_store(&overridden.server).unwrap(); let marker = if prefix.is_empty() { diff --git a/lib/crates/fabro-server/src/interp.rs b/lib/crates/fabro-server/src/interp.rs index 599421e1d..4b0de8eeb 100644 --- a/lib/crates/fabro-server/src/interp.rs +++ b/lib/crates/fabro-server/src/interp.rs @@ -1,50 +1,17 @@ -//! Shared process-env interpolation helpers for server-scope settings. +//! The server-owned process-environment lookup facade. //! -//! 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. +//! Server control-plane settings (storage root, listen path, web/api URL, +//! object-store coordinates, GitHub App identifiers) are plain `String` and do +//! not interpolate; deployment-time late binding goes through native env reads +//! (e.g. `FABRO_WEB_URL`, read in `canonical_origin`). This module owns the +//! single process-env lookup facade those reads — and server secret reads — go +//! through; 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. +/// The server-owned process-env lookup facade for native env reads and server +/// configuration/secret reads. #[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" + reason = "server configuration and secret reads 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 cd442edef..18984267e 100644 --- a/lib/crates/fabro-server/src/jwt_auth.rs +++ b/lib/crates/fabro-server/src/jwt_auth.rs @@ -14,6 +14,7 @@ use tracing::info; #[cfg(test)] use crate::auth::REFRESH_TOKEN_PREFIX; use crate::auth::{self, AuthErrorCode, JwtError, JwtSigningKey, KeyDeriveError}; +use crate::canonical_origin::effective_web_url; use crate::error::ApiError; use crate::interp::process_env_var; @@ -138,22 +139,16 @@ fn resolve_jwt_issuer(settings: &ServerNamespace, lookup: &F) -> String where F: Fn(&str) -> Option, { + let web_url = effective_web_url(settings, lookup); + if !web_url.is_empty() { + return web_url; + } + settings - .web + .api .url - .resolve(|name| lookup(name)) - .ok() - .map(|resolved| resolved.value) + .clone() .filter(|value| !value.is_empty()) - .or_else(|| { - settings - .api - .url - .as_ref() - .and_then(|url| url.resolve(|name| lookup(name)).ok()) - .map(|resolved| resolved.value) - .filter(|value| !value.is_empty()) - }) .unwrap_or_else(|| "fabro-server".to_string()) } diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 87a726398..070a0a088 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -32,7 +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::interp::process_env_var; use crate::server::{ AppState, AppStateConfig, ResolvedAppStateSettings, RouterOptions, build_app_state, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, @@ -260,28 +260,22 @@ fn resolve_webhook_preconditions( github: &GithubIntegrationSettings, state: &Arc, webhook_secret_present: bool, -) -> anyhow::Result { +) -> WebhookPreconditions { if github.strategy != GithubIntegrationStrategy::App { - return Ok(WebhookPreconditions::Skip( - "GitHub integration auth is not set to app".to_string(), - )); + return WebhookPreconditions::Skip("GitHub integration auth is not set to app".to_string()); } if !webhook_secret_present { - return Ok(WebhookPreconditions::Skip(format!( - "{WEBHOOK_SECRET_ENV} is not set" - ))); + return WebhookPreconditions::Skip(format!("{WEBHOOK_SECRET_ENV} is not set")); } - let Some(app_id) = github.app_id.as_ref().map(resolve_interp).transpose()? else { - return Ok(WebhookPreconditions::Skip( + let Some(app_id) = github.app_id.clone() else { + return WebhookPreconditions::Skip( "server.integrations.github.app_id is not set".to_string(), - )); + ); }; let github_app = match state.github_credentials(github) { Ok(creds) => creds, Err(err) => { - return Ok(WebhookPreconditions::Skip(format!( - "GitHub credentials are invalid: {err}" - ))); + return WebhookPreconditions::Skip(format!("GitHub credentials are invalid: {err}")); } }; let github_app = match github_app { @@ -290,20 +284,20 @@ fn resolve_webhook_preconditions( fabro_github::GitHubCredentials::Pat(_) | fabro_github::GitHubCredentials::Installation(_), ) => { - return Ok(WebhookPreconditions::Skip( + return WebhookPreconditions::Skip( "GitHub webhooks require GitHub App credentials".to_string(), - )); + ); } None => { - return Ok(WebhookPreconditions::Skip( + return WebhookPreconditions::Skip( "GITHUB_APP_PRIVATE_KEY is not available".to_string(), - )); + ); } }; - Ok(WebhookPreconditions::Ready { + WebhookPreconditions::Ready { app_id, private_key_pem: github_app.private_key_pem, - }) + } } async fn start_webhook_strategy( @@ -318,7 +312,7 @@ async fn start_webhook_strategy( }; let (app_id, private_key_pem) = - match resolve_webhook_preconditions(github, state, webhook_secret_present)? { + match resolve_webhook_preconditions(github, state, webhook_secret_present) { WebhookPreconditions::Ready { app_id, private_key_pem, @@ -355,9 +349,7 @@ async fn start_webhook_strategy( let server_api_url = resolved_server_settings .api .url - .as_ref() - .map(resolve_interp) - .transpose()? + .clone() .ok_or_else(|| { anyhow::anyhow!( "server.api.url must be set when webhook strategy = \"server_url\" (resolver invariant)" @@ -495,7 +487,7 @@ where let build_options = build_options.cloned().unwrap_or_default(); match settings { ObjectStoreSettings::Local { root } => { - build_local_object_store_with_preference(&resolve_interp_path(root)?, false) + build_local_object_store_with_preference(&PathBuf::from(root), false) } ObjectStoreSettings::S3 { bucket, @@ -505,11 +497,11 @@ where } => { let mut builder = AmazonS3Builder::new() .with_http_connector(NoProxyReqwestConnector) - .with_bucket_name(resolve_interp(bucket)?) - .with_region(resolve_interp(region)?) + .with_bucket_name(bucket.clone()) + .with_region(region.clone()) .with_virtual_hosted_style_request(!*path_style); if let Some(endpoint) = endpoint.as_ref() { - builder = builder.with_endpoint(resolve_interp(endpoint)?); + builder = builder.with_endpoint(endpoint.clone()); } builder = configure_s3_builder_from_env_lookup(builder, env_lookup, &build_options)?; Ok(Arc::new(builder.build()?)) @@ -543,16 +535,14 @@ pub fn resolve_bind_request_from_server_settings( ) -> anyhow::Result { match explicit_bind.map(bind::parse_bind).transpose()? { Some(bind) => Ok(bind), - None => resolved_bind_request(&settings.server), + None => Ok(resolved_bind_request(&settings.server)), } } -fn resolved_bind_request( - resolved_server_settings: &ServerNamespace, -) -> anyhow::Result { +fn resolved_bind_request(resolved_server_settings: &ServerNamespace) -> BindRequest { match &resolved_server_settings.listen { - ServerListenSettings::Unix { path } => Ok(BindRequest::Unix(resolve_interp_path(path)?)), - ServerListenSettings::Tcp { address, .. } => Ok(BindRequest::Tcp(*address)), + ServerListenSettings::Unix { path } => BindRequest::Unix(PathBuf::from(path)), + ServerListenSettings::Tcp { address, .. } => BindRequest::Tcp(*address), } } @@ -567,7 +557,7 @@ fn absolute_path(path: PathBuf) -> anyhow::Result { } fn load_server_secrets_for_settings(settings: &ServerNamespace) -> anyhow::Result { - let storage_root = resolve_interp_path(&settings.storage.root)?; + let storage_root = PathBuf::from(&settings.storage.root); let server_env_path = Storage::new(&storage_root).runtime_directory().env_path(); ServerSecrets::load(server_env_path, process_env_snapshot()).map_err(anyhow::Error::from) } @@ -576,7 +566,7 @@ pub(crate) fn build_artifact_object_store_with_server_secrets( settings: &ServerNamespace, server_secrets: &ServerSecrets, ) -> anyhow::Result<(Arc, String)> { - let prefix = resolve_interp(&settings.artifacts.prefix)?; + let prefix = settings.artifacts.prefix.clone(); let object_store = build_object_store_from_settings_with_lookup( &settings.artifacts.store, &|name| server_secrets.get(name), @@ -596,7 +586,7 @@ fn build_slatedb_store_with_server_secrets( settings: &ServerNamespace, server_secrets: &ServerSecrets, ) -> anyhow::Result<(Arc, String, Duration, bool)> { - let prefix = resolve_interp(&settings.slatedb.prefix)?; + let prefix = settings.slatedb.prefix.clone(); let object_store = build_object_store_from_settings_with_lookup( &settings.slatedb.store, &|name| server_secrets.get(name), @@ -654,7 +644,7 @@ where let disk_server_settings = runtime_settings.server_settings.server.clone(); let data_dir = match storage_dir_override { Some(path) => path, - None => resolve_interp_path(&disk_server_settings.storage.root)?, + None => PathBuf::from(&disk_server_settings.storage.root), }; let storage = Storage::new(&data_dir); let vault_path = storage.secrets_path(); @@ -1148,10 +1138,6 @@ 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; @@ -1163,7 +1149,6 @@ mod tests { use fabro_config::ServerSettingsBuilder; use fabro_config::bind::{Bind, BindRequest}; use fabro_types::ServerSettings; - use fabro_types::settings::interp::InterpString; use fabro_types::settings::server::{LogDestination, ObjectStoreSettings}; use fabro_util::Home; use tokio::time::sleep; @@ -1173,9 +1158,8 @@ mod tests { SHUTDOWN_GRACE_PERIOD, ServeArgs, ServerTitlePhase, apply_effective_log_destination, bind_tcp_host_with_fallback, build_local_object_store_with_preference, build_object_store_from_settings_with_lookup, build_slatedb_store, - force_exit_after_shutdown, resolve_bind_request_from_server_settings, resolve_interp, - serve_overrides, serve_until_shutdown, server_bind_title, server_title, - spawn_shutdown_orchestrator_inner, + force_exit_after_shutdown, resolve_bind_request_from_server_settings, serve_overrides, + serve_until_shutdown, server_bind_title, server_title, spawn_shutdown_orchestrator_inner, }; use crate::server::ResolvedAppStateSettings; @@ -1210,16 +1194,6 @@ mod tests { .expect("settings should resolve") } - #[test] - fn server_settings_interpolation_rejects_variables() { - let err = resolve_interp(&InterpString::parse("{{ vars.STORAGE_ROOT }}")).unwrap_err(); - - let rendered = format!("{err:#}"); - assert!(rendered.contains("failed to resolve {{ vars.STORAGE_ROOT }}")); - assert!(rendered.contains("variable \"STORAGE_ROOT\"")); - assert!(rendered.contains("not supported in this interpolation context")); - } - fn resolved_runtime_settings(source: &str) -> ResolvedAppStateSettings { ResolvedAppStateSettings { manifest_run_defaults: manifest_run_defaults(source), @@ -1351,7 +1325,7 @@ mod tests { .with_storage_override(&PathBuf::from("/srv/fabro-storage")); assert_eq!( - resolved.server_settings.server.storage.root.as_source(), + resolved.server_settings.server.storage.root, "/srv/fabro-storage" ); let fabro_types::settings::ObjectStoreSettings::Local { root } = @@ -1359,13 +1333,13 @@ mod tests { else { panic!("artifacts store should stay local"); }; - assert_eq!(root.as_source(), "/srv/fabro-storage/objects/artifacts"); + assert_eq!(root, "/srv/fabro-storage/objects/artifacts"); let fabro_types::settings::ObjectStoreSettings::Local { root } = &resolved.server_settings.server.slatedb.store else { panic!("slatedb store should stay local"); }; - assert_eq!(root.as_source(), "/srv/fabro-storage/objects/slatedb"); + assert_eq!(root, "/srv/fabro-storage/objects/slatedb"); } #[test] @@ -1383,7 +1357,7 @@ root = "/srv/from-disk" .with_storage_override(&PathBuf::from("/srv/from-runtime")); assert_eq!( - resolved.server_settings.server.storage.root.as_source(), + resolved.server_settings.server.storage.root, "/srv/from-runtime" ); assert_eq!( @@ -1623,8 +1597,8 @@ disk_cache = true #[test] fn build_object_store_from_settings_uses_injected_static_credentials() { let settings = ObjectStoreSettings::S3 { - bucket: InterpString::parse("fabro-data"), - region: InterpString::parse("us-east-1"), + bucket: "fabro-data".to_string(), + region: "us-east-1".to_string(), endpoint: None, path_style: false, }; @@ -1645,8 +1619,8 @@ disk_cache = true #[test] fn build_object_store_from_settings_rejects_partial_static_credentials() { let settings = ObjectStoreSettings::S3 { - bucket: InterpString::parse("fabro-data"), - region: InterpString::parse("us-east-1"), + bucket: "fabro-data".to_string(), + region: "us-east-1".to_string(), endpoint: None, path_style: false, }; @@ -1670,8 +1644,8 @@ disk_cache = true #[test] fn build_object_store_from_settings_ignores_endpoint_override_env_vars() { let settings = ObjectStoreSettings::S3 { - bucket: InterpString::parse("fabro-data"), - region: InterpString::parse("us-east-1"), + bucket: "fabro-data".to_string(), + region: "us-east-1".to_string(), endpoint: None, path_style: false, }; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 469bfabe1..2bc7cb946 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -87,11 +87,11 @@ use fabro_store::{ }; #[cfg(test)] use fabro_types::BlockedReason; +use fabro_types::settings::RunNamespace; use fabro_types::settings::run::{NotificationRouteSettings, RunMode}; use fabro_types::settings::server::{ GithubIntegrationSettings, GithubIntegrationStrategy, LogDestination, }; -use fabro_types::settings::{InterpString, RunNamespace}; use fabro_types::{ AgentBackend, AskFabro, AskFabroUnavailableReason, EventBody, InterviewQuestionRecord, PairId, PairMessageId, PairTarget, PendingReason, Principal, PullRequestLink, QuestionType, RunBlobId, @@ -141,12 +141,12 @@ use crate::automation_materializer::{ AutomationRunMaterializeError, AutomationRunMaterializeInput, AutomationRunMaterialized, AutomationRunMaterializer, GitRepoCache, ProductionAutomationRunMaterializer, }; -use crate::canonical_origin::resolve_canonical_origin; +use crate::canonical_origin::{canonical_origin_from_effective_web_url, effective_web_url}; 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::interp::process_env_var; use crate::jwt_auth::{self, AuthMode}; use crate::principal_middleware::{ AuthContextSlot, RequestAuth, RequestAuthContext, RequireRunBlob, RequireRunManagementTarget, @@ -1090,6 +1090,7 @@ pub struct AppState { manifest_run_defaults: RwLock>, manifest_run_settings: RwLock>, pub(crate) server_settings: RwLock>, + effective_web_url: RwLock, catalog: RwLock>, pub(crate) env_lookup: EnvLookup, pub(crate) github_api_base_url: String, @@ -1349,10 +1350,7 @@ impl AppState { } pub(crate) fn server_storage_dir(&self) -> PathBuf { - PathBuf::from( - resolve_interp(&self.server_settings().server.storage.root) - .expect("server storage root should be resolved at startup"), - ) + PathBuf::from(&self.server_settings().server.storage.root) } /// Scratch directory used by the automation materializer when staging @@ -1487,12 +1485,15 @@ impl AppState { daemon.bind.to_target().parse() } - pub(crate) fn resolve_interp(&self, value: &InterpString) -> anyhow::Result { - resolve_interp_with(value, |name| (self.env_lookup)(name)) + pub(crate) fn effective_web_url(&self) -> String { + self.effective_web_url + .read() + .expect("effective web url lock poisoned") + .clone() } pub(crate) fn canonical_origin(&self) -> Result { - resolve_canonical_origin(&self.server_settings().server, &self.env_lookup) + canonical_origin_from_effective_web_url(&self.effective_web_url()) } pub(crate) fn session_key(&self) -> Option { @@ -1500,18 +1501,13 @@ 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, ) -> Result, String> { match settings.strategy { GithubIntegrationStrategy::App => { - let Some(app_id) = settings.app_id.as_ref().map(InterpString::as_source) else { + let Some(app_id) = settings.app_id.clone() else { return Ok(None); }; let raw = self.vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY); @@ -1523,7 +1519,7 @@ impl AppState { fabro_github::GitHubAppCredentials { app_id, private_key_pem, - slug: settings.slug.as_ref().map(InterpString::as_source), + slug: settings.slug.clone(), }, ))) } @@ -1574,6 +1570,8 @@ impl AppState { } = resolved_settings; let server_settings = Arc::new(server_settings); let manifest_run_defaults = Arc::new(manifest_run_defaults); + let effective_web_url = + effective_web_url(&server_settings.server, |name| (self.env_lookup)(name)); let manifest_run_settings = resolve_manifest_run_settings_with_catalog( manifest_run_defaults.as_ref(), &self.environment_store, @@ -1582,8 +1580,7 @@ impl AppState { Catalog::from_builtin_with_overrides(&llm_catalog_settings) .context("building LLM model catalog")?, ); - resolve_canonical_origin(&server_settings.server, &self.env_lookup) - .map_err(anyhow::Error::msg)?; + canonical_origin_from_effective_web_url(&effective_web_url).map_err(anyhow::Error::msg)?; *self .manifest_run_defaults @@ -1597,6 +1594,10 @@ impl AppState { .server_settings .write() .expect("server settings lock poisoned") = server_settings; + *self + .effective_web_url + .write() + .expect("effective web url lock poisoned") = effective_web_url; *self.catalog.write().expect("catalog lock poisoned") = catalog; Ok(()) } @@ -2334,6 +2335,8 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result anyhow::Result anyhow::Result PathBuf::from(path), - Err(err) => { - return ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - format!("Failed to resolve server storage root: {err}"), - ) - .into_response(); - } - }; + let storage_root = state.server_storage_dir(); let created = match Box::pin(operations::create( state.store.as_ref(), create_input, diff --git a/lib/crates/fabro-server/src/server/handler/system.rs b/lib/crates/fabro-server/src/server/handler/system.rs index 4eae3837e..862230bbc 100644 --- a/lib/crates/fabro-server/src/server/handler/system.rs +++ b/lib/crates/fabro-server/src/server/handler/system.rs @@ -20,7 +20,6 @@ use super::super::{ counts_toward_scheduler_capacity, delete_run_internal, diagnostics, get, post, resource_sampler, spawn_blocking, system_sandbox_provider, to_i64, }; -use crate::interp::resolve_interp; pub(super) fn routes() -> Router> { Router::new() @@ -52,14 +51,8 @@ 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(); let (total_runs, active_runs, scheduler_slots_used) = { let runs = state.runs.lock().expect("runs lock poisoned"); let active = runs @@ -85,7 +78,7 @@ async fn get_system_info(_auth: RequiredUser, State(state): State> let response = SystemInfoResponse { version: Some(FABRO_VERSION.to_string()), - server_url: Some(server_settings.server.web.url.as_source()), + server_url: Some(state.effective_web_url()), git_sha: option_env!("FABRO_GIT_SHA").map(str::to_string), build_date: option_env!("FABRO_BUILD_DATE").map(str::to_string), profile: option_env!("FABRO_BUILD_PROFILE").map(str::to_string), @@ -132,10 +125,10 @@ fn github_integration_status( .to_string(), ); if let Some(slug) = settings.slug.as_ref() { - metadata.insert("slug".to_string(), display_interp(state, slug)); + metadata.insert("slug".to_string(), slug.clone()); } if let Some(app_id) = settings.app_id.as_ref() { - metadata.insert("app_id".to_string(), display_interp(state, app_id)); + metadata.insert("app_id".to_string(), app_id.clone()); } if !settings.enabled { @@ -188,6 +181,10 @@ fn github_integration_status( ) } +fn display_interp(state: &AppState, value: &InterpString) -> String { + value.resolve_or_source(|name| (state.env_lookup)(name)) +} + fn slack_integration_status(state: &AppState) -> SystemIntegrationStatus { let settings = &state.server_settings().server.integrations.slack; let mut metadata = BTreeMap::new(); @@ -281,10 +278,6 @@ fn missing_vault_secret(state: &AppState, name: &str) -> bool { .is_none_or(str::is_empty) } -fn display_interp(state: &AppState, value: &InterpString) -> String { - value.resolve_or_source(|name| (state.env_lookup)(name)) -} - async fn get_system_resources(_auth: RequiredUser, State(state): State>) -> Response { match resource_sampler::sample_system_resources(&state).await { Ok(response) => (StatusCode::OK, Json(response)).into_response(), @@ -475,16 +468,12 @@ async fn get_github_repo( let base_url = fabro_github::github_api_base_url(); let (token, client) = match github_settings.strategy { GithubIntegrationStrategy::App => { - let Some(app_id) = github_settings.app_id.as_ref() else { + if github_settings.app_id.is_none() { return ApiError::new( StatusCode::SERVICE_UNAVAILABLE, "server.integrations.github.app_id is not configured", ) .into_response(); - }; - if let Err(err) = resolve_interp(app_id) { - return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) - .into_response(); } let creds = match state.github_credentials(github_settings) { Ok(Some(fabro_github::GitHubCredentials::App(creds))) => creds, @@ -509,16 +498,9 @@ async fn get_github_repo( .into_response(); } }; - let install_url = match github_settings.slug.as_ref() { - 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()) - .into_response(); - } - }, - None => format!("https://github.com/organizations/{owner}/settings/installations"), - }; + let install_url = creds.installation_url(&owner).unwrap_or_else(|| { + format!("https://github.com/organizations/{owner}/settings/installations") + }); let client = match state.http_client() { Ok(http) => http, diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 33f97da1b..b6333b317 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -1056,18 +1056,18 @@ fn replace_settings_rejects_invalid_canonical_origin_and_keeps_previous_settings "ftp://fabro.example.com", "http://0.0.0.0:32276", ] { + // No FABRO_WEB_URL override: web.url is plain config now, so the + // invalid value is rejected from the settings literal and the kept + // previous settings stay valid. let state = test_app_state_with_env_lookup( canonical_origin_settings("http://valid.example.com"), RunLayer::default(), 5, - { - let invalid = invalid.to_string(); - move |name| (name == "FABRO_WEB_URL").then(|| invalid.clone()) - }, + |_| None, ); let err = state - .replace_runtime_settings(resolved_runtime_settings_from_toml( + .replace_runtime_settings(resolved_runtime_settings_from_toml(&format!( r#" _version = 1 @@ -1075,9 +1075,9 @@ _version = 1 methods = ["dev-token"] [server.web] -url = "{{ env.FABRO_WEB_URL }}" +url = "{invalid}" "#, - )) + ))) .expect_err("invalid canonical origin should be rejected"); assert!( err.to_string() @@ -1091,10 +1091,36 @@ url = "{{ env.FABRO_WEB_URL }}" } } -#[expect( - clippy::disallowed_methods, - reason = "test asserts the raw template source" -)] +#[test] +fn canonical_origin_prefers_fabro_web_url_env_override() { + // FABRO_WEB_URL is the native control-plane override; it wins over the + // plain `server.web.url` settings literal. + let state = test_app_state_with_env_lookup( + canonical_origin_settings("http://settings.example.com"), + RunLayer::default(), + 5, + |name| (name == "FABRO_WEB_URL").then(|| "http://env.example.com".to_string()), + ); + + assert_eq!(state.canonical_origin().unwrap(), "http://env.example.com"); +} + +#[test] +fn canonical_origin_uses_settings_literal_without_env_override() { + // Without FABRO_WEB_URL set, the plain `server.web.url` literal is used. + let state = test_app_state_with_env_lookup( + canonical_origin_settings("http://settings.example.com"), + RunLayer::default(), + 5, + |_| None, + ); + + assert_eq!( + state.canonical_origin().unwrap(), + "http://settings.example.com" + ); +} + #[test] fn replace_settings_updates_layer_and_typed_server_settings() { let state = test_app_state_with_options( @@ -1150,10 +1176,7 @@ root = "/srv/new" .expect("valid settings should replace current state"); assert_eq!(state.canonical_origin().unwrap(), "http://new.example.com"); - assert_eq!( - state.server_settings().server.storage.root.as_source(), - "/srv/new" - ); + assert_eq!(state.server_settings().server.storage.root, "/srv/new"); assert_eq!( state .manifest_run_settings() diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index c8b617278..44edece47 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -21,7 +21,6 @@ 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, @@ -324,12 +323,8 @@ fn session_provider(auth_method: AuthMethod) -> &'static str { fn session_cookie_secure(state: &AppState) -> bool { state - .server_settings() - .server - .web - .url - .resolve(process_env_var) - .is_ok_and(|resolved| resolved.value.starts_with("https://")) + .canonical_origin() + .is_ok_and(|web_url| web_url.starts_with("https://")) } fn redacted_url_for_log(url: &str) -> String { @@ -443,16 +438,7 @@ async fn login_github( json!({"error": "GitHub App client_id is not configured"}), ); }; - let client_id = match state.resolve_interp(client_id) { - Ok(client_id) => client_id, - Err(err) => { - warn!(error = %err, "OAuth login failed: client_id could not be resolved"); - return json_response( - StatusCode::CONFLICT, - json!({"error": format!("GitHub App client_id could not be resolved: {err}")}), - ); - } - }; + let client_id = client_id.clone(); let web_url = match state.canonical_origin() { Ok(web_url) => web_url, Err(err) => { @@ -590,16 +576,7 @@ async fn callback_github( json!({"error": "GitHub App client_id is not configured"}), ); }; - let client_id = match state.resolve_interp(client_id) { - Ok(client_id) => client_id, - Err(err) => { - error!(error = %err, "OAuth callback failed: client_id could not be resolved"); - return json_response( - StatusCode::CONFLICT, - json!({"error": format!("GitHub App client_id could not be resolved: {err}")}), - ); - } - }; + let client_id = client_id.clone(); let Some(client_secret) = state.vault_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) else { error!("OAuth callback failed: GITHUB_APP_CLIENT_SECRET not configured"); return json_response( diff --git a/lib/crates/fabro-server/tests/it/api/system.rs b/lib/crates/fabro-server/tests/it/api/system.rs index 34005cde1..d8ee7e63c 100644 --- a/lib/crates/fabro-server/tests/it/api/system.rs +++ b/lib/crates/fabro-server/tests/it/api/system.rs @@ -9,7 +9,6 @@ use axum::body::Body; use axum::http::{Request, StatusCode}; use fabro_config::Storage; use fabro_types::RunId; -use fabro_types::settings::interp::InterpString; use tempfile::tempdir; use tower::ServiceExt; @@ -40,8 +39,7 @@ fn temp_storage_settings() -> (tempfile::TempDir, TestAppSettings, PathBuf) { let temp = tempdir().expect("tempdir should create"); let mut settings = test_settings(); let storage_dir = temp.path().join("storage"); - settings.server_settings.server.storage.root = - InterpString::parse(&storage_dir.to_string_lossy()); + settings.server_settings.server.storage.root = storage_dir.to_string_lossy().into_owned(); (temp, settings, storage_dir) } @@ -114,14 +112,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(); - let configured_server_url = settings.server_settings.server.web.url.as_source(); + let configured_server_url = settings.server_settings.server.web.url.clone(); let app = fabro_server::test_support::build_test_router(test_app_state_with_options(settings, 5)); diff --git a/lib/crates/fabro-types/src/dense.rs b/lib/crates/fabro-types/src/dense.rs index 6ffacc92a..cc70c2171 100644 --- a/lib/crates/fabro-types/src/dense.rs +++ b/lib/crates/fabro-types/src/dense.rs @@ -4,8 +4,8 @@ use std::path::Path; use serde::{Deserialize, Serialize}; use crate::settings::{ - CliNamespace, InterpString, ObjectStoreSettings, ProjectNamespace, RunNamespace, - ServerNamespace, WorkflowNamespace, + CliNamespace, ObjectStoreSettings, ProjectNamespace, RunNamespace, ServerNamespace, + WorkflowNamespace, }; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -16,7 +16,7 @@ pub struct ServerSettings { impl ServerSettings { #[must_use] pub fn with_storage_override(mut self, path: &Path) -> Self { - self.server.storage.root = InterpString::parse(&path.display().to_string()); + self.server.storage.root = path.display().to_string(); override_local_object_store_root(&mut self.server.artifacts.store, path, "artifacts"); override_local_object_store_root(&mut self.server.slatedb.store, path, "slatedb"); self @@ -31,13 +31,11 @@ fn override_local_object_store_root( let ObjectStoreSettings::Local { root } = store else { return; }; - *root = InterpString::parse( - &storage_root - .join("objects") - .join(domain) - .display() - .to_string(), - ); + *root = storage_root + .join("objects") + .join(domain) + .display() + .to_string(); } #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index 806788d37..59055027c 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -70,36 +70,27 @@ pub enum ServerListenSettings { address: SocketAddr, }, Unix { - path: InterpString, + path: String, }, } impl Default for ServerListenSettings { fn default() -> Self { Self::Unix { - path: InterpString::parse(""), + path: String::new(), } } } #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct ServerApiSettings { - pub url: Option, + pub url: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct ServerWebSettings { pub enabled: bool, - pub url: InterpString, -} - -impl Default for ServerWebSettings { - fn default() -> Self { - Self { - enabled: false, - url: InterpString::parse(""), - } - } + pub url: String, } #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] @@ -160,37 +151,20 @@ impl Default for ServerSandboxProviderSettings { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct ServerStorageSettings { - pub root: InterpString, + pub root: String, } -impl Default for ServerStorageSettings { - fn default() -> Self { - Self { - root: InterpString::parse(""), - } - } -} - -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct ServerArtifactsSettings { - pub prefix: InterpString, + pub prefix: String, pub store: ObjectStoreSettings, } -impl Default for ServerArtifactsSettings { - fn default() -> Self { - Self { - prefix: InterpString::parse(""), - store: ObjectStoreSettings::default(), - } - } -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ServerSlateDbSettings { - pub prefix: InterpString, + pub prefix: String, pub store: ObjectStoreSettings, #[serde( serialize_with = "serialize_std_duration", @@ -203,7 +177,7 @@ pub struct ServerSlateDbSettings { impl Default for ServerSlateDbSettings { fn default() -> Self { Self { - prefix: InterpString::parse(""), + prefix: String::new(), store: ObjectStoreSettings::default(), flush_interval: StdDuration::ZERO, disk_cache: false, @@ -215,12 +189,12 @@ impl Default for ServerSlateDbSettings { #[serde(tag = "type", rename_all = "snake_case")] pub enum ObjectStoreSettings { Local { - root: InterpString, + root: String, }, S3 { - bucket: InterpString, - region: InterpString, - endpoint: Option, + bucket: String, + region: String, + endpoint: Option, path_style: bool, }, } @@ -228,7 +202,7 @@ pub enum ObjectStoreSettings { impl Default for ObjectStoreSettings { fn default() -> Self { Self::Local { - root: InterpString::parse(""), + root: String::new(), } } } @@ -275,9 +249,9 @@ pub struct ServerIntegrationsSettings { pub struct GithubIntegrationSettings { pub enabled: bool, pub strategy: GithubIntegrationStrategy, - pub app_id: Option, - pub client_id: Option, - pub slug: Option, + pub app_id: Option, + pub client_id: Option, + pub slug: Option, pub webhooks: Option, }