From a12e6da6fb8315a7611dae3ccb7e6b4450239fc1 Mon Sep 17 00:00:00 2001 From: Andrew Wheeler Date: Thu, 8 Oct 2026 19:13:48 -0600 Subject: [PATCH 1/2] Validate pending config against the pending env file --- .../app/src/admin/walled/config-tests.rs | 75 +++++++++++++++++ crates/gateway/app/src/admin/walled/config.rs | 14 +++- .../app/src/admin/walled/config_pending.rs | 11 ++- .../gateway/app/src/admin/walled/env_file.rs | 21 +---- crates/gateway/app/src/commands/apply.rs | 11 ++- crates/gateway/app/src/config_shadow-tests.rs | 84 +++++++++++++++++++ crates/gateway/app/src/config_shadow.rs | 68 +++++++++++++++ crates/gateway/config/src/config.rs | 1 + crates/gateway/config/src/config/imp.rs | 21 ++++- .../gateway/config/src/config/interpolate.rs | 37 +++++--- crates/gateway/config/src/config/tests.rs | 15 +++- crates/gateway/config/src/lib.rs | 2 +- crates/gateway/config/src/shadow-tests.rs | 43 +++++++--- crates/gateway/config/src/shadow.rs | 15 +++- 14 files changed, 355 insertions(+), 63 deletions(-) create mode 100644 crates/gateway/app/src/config_shadow-tests.rs diff --git a/crates/gateway/app/src/admin/walled/config-tests.rs b/crates/gateway/app/src/admin/walled/config-tests.rs index b9c33c86b..1ec6206ab 100644 --- a/crates/gateway/app/src/admin/walled/config-tests.rs +++ b/crates/gateway/app/src/admin/walled/config-tests.rs @@ -85,6 +85,19 @@ async fn put_config(addr: std::net::SocketAddr, body: &serde_json::Value) -> req .expect("put sends") } +/// Add a call to the Add Model endpoint to a request body +fn push_cloud_endpoint(body: &mut serde_json::Value, var: &str) { + body["endpoint"] + .as_array_mut() + .expect("endpoints are an array") + .push(serde_json::json!({ + "id": "staged", + "protocol": "openai", + "base_url": "http://127.0.0.1:9", + "api_key": format!("${{{var}}}"), + })); +} + #[tokio::test] async fn a_save_setting_active_profile_is_rejected_and_stages_nothing() { let (_temp, config, paths) = fixture(); @@ -129,3 +142,65 @@ async fn a_save_replies_with_the_config_shadow_alone() { assert!(shadow_path(&config_path).is_file()); assert!(!shadow_path(&profile_state_path(&config_path)).exists()); } + +#[tokio::test] +async fn adding_a_cloud_provider_validates_against_a_key_staged_only_in_the_env_shadow() { + // After a 'Save' operation in the UI the saved key is stored in the shadow + // env file and is not yet available in the process's environment. + // + // When a Cloud Model is added the handler should validate against the running + // environment and pending keys in the shadow env. + + let (_temp, config, paths) = fixture(); + let config_path = paths.config_path.clone(); + + std::fs::write( + shadow_path(&config_path.with_extension("env")), + "PF_TEST_STAGED_KEY=sk-staged\n", + ) + .expect("write env shadow"); + + let addr = serve_with_paths(config, paths).await; + + let mut body = save_body(addr).await; + push_cloud_endpoint(&mut body, "PF_TEST_STAGED_KEY"); + + let response = put_config(addr, &body).await; + + let expected = reqwest::StatusCode::OK; + let actual = response.status(); + assert_eq!( + actual, + expected, + "Got `{actual}`, expected `{expected}`. Response: {}", + response.text().await.unwrap_or_default() + ); + assert!(shadow_path(&config_path).is_file()); +} + +#[tokio::test] +async fn adding_a_cloud_provider_without_its_key_is_rejected_and_stages_nothing() { + let (_temp, config, paths) = fixture(); + let config_path = paths.config_path.clone(); + let addr = serve_with_paths(config, paths).await; + + let mut body = save_body(addr).await; + push_cloud_endpoint(&mut body, "PF_TEST_MISSING_KEY"); + + let response = put_config(addr, &body).await; + + assert_eq!(response.status(), reqwest::StatusCode::UNPROCESSABLE_ENTITY); + + let error: serde_json::Value = response.json().await.expect("error envelope"); + + assert_eq!(error["error"]["code"], "config_write_rejected"); + + assert!( + error["error"]["message"] + .as_str() + .is_some_and(|message| message.contains("PF_TEST_MISSING_KEY")), + "the error message does not name the missing variable: {error}" + ); + + assert!(!shadow_path(&config_path).exists()); +} diff --git a/crates/gateway/app/src/admin/walled/config.rs b/crates/gateway/app/src/admin/walled/config.rs index ee87fa22c..bb3ffd14b 100644 --- a/crates/gateway/app/src/admin/walled/config.rs +++ b/crates/gateway/app/src/admin/walled/config.rs @@ -22,6 +22,7 @@ use serde::Serialize; use crate::AppState; use crate::auth::LoopbackCaller; +use crate::config_shadow::PendingEnv; use crate::error::{GatewayError, WireJson, blocking, config_write_error}; use crate::registry::RouteInfo; @@ -82,11 +83,16 @@ async fn admin_put_config( // save validated whole - saves serialize with apply, revert, and each // other. let _guard = state.apply.lock().await; - let config = crate::admin::config_path(&state)?.to_path_buf(); + let config_path = crate::admin::config_path(&state)?.to_path_buf(); let document = toml_document(body)?; - let shadows = blocking(move || save_config_shadow(&config, document)) - .await? - .map_err(config_write_error)?; + let shadows = blocking(move || { + let pending_env = PendingEnv::new(&config_path)?; + save_config_shadow(&config_path, document, &|name: &str| { + pending_env.resolve_var(name) + }) + .map_err(config_write_error) + }) + .await??; Ok(Json(ShadowReply::staged(&shadows.config))) } diff --git a/crates/gateway/app/src/admin/walled/config_pending.rs b/crates/gateway/app/src/admin/walled/config_pending.rs index cfbad434a..b46c5ab5d 100644 --- a/crates/gateway/app/src/admin/walled/config_pending.rs +++ b/crates/gateway/app/src/admin/walled/config_pending.rs @@ -24,7 +24,7 @@ use serde::Serialize; use crate::AppState; use crate::auth::LoopbackCaller; -use crate::config_shadow::{config_root, relative_name, shadow_census}; +use crate::config_shadow::{PendingEnv, config_root, relative_name, shadow_census}; use crate::error::{GatewayError, blocking, pending_read_error}; use crate::registry::RouteInfo; @@ -107,8 +107,13 @@ pub(super) fn load_pending_for_running( config_path: &Path, running_profile: Option<&str>, ) -> Result { - load_pending_config(config_path, &ProfileSelection::new(running_profile, None)) - .map_err(|error| pending_read_error(&error)) + let pending_env = PendingEnv::new(config_path)?; + load_pending_config( + config_path, + &ProfileSelection::new(running_profile, None), + &|name: &str| pending_env.resolve_var(name), + ) + .map_err(|error| pending_read_error(&error)) } /// The profile name the real state file persists, `None` when the file is diff --git a/crates/gateway/app/src/admin/walled/env_file.rs b/crates/gateway/app/src/admin/walled/env_file.rs index 786c5dcf0..571fcf5fd 100644 --- a/crates/gateway/app/src/admin/walled/env_file.rs +++ b/crates/gateway/app/src/admin/walled/env_file.rs @@ -22,6 +22,7 @@ use serde::Serialize; use super::config::ShadowReply; use crate::AppState; use crate::auth::LoopbackCaller; +use crate::config_shadow::read_env_file; use crate::error::{GatewayError, WireJson, WireQuery, blocking, config_write_error}; use crate::registry::RouteInfo; @@ -42,7 +43,7 @@ struct EnvReply { #[derive(Debug, Serialize)] struct EnvSection { path: String, - vars: serde_json::Map, + vars: BTreeMap, } const ENV: RouteInfo = RouteInfo::walled("/admin/env", &[Method::GET, Method::PUT]); @@ -130,26 +131,10 @@ async fn admin_put_env( fn env_section(path: &Path) -> Result { Ok(EnvSection { path: path.display().to_string(), - vars: parse_env(path)?, + vars: read_env_file(path)?, }) } -/// Parses one `.env` file into a map, without touching the process -/// environment. A missing file is an empty map. -fn parse_env(path: &Path) -> Result, GatewayError> { - let mut vars = serde_json::Map::new(); - if !path.is_file() { - return Ok(vars); - } - let entries = - dotenvy::from_path_iter(path).map_err(|error| GatewayError::EnvFile(Box::new(error)))?; - for entry in entries { - let (key, value) = entry.map_err(|error| GatewayError::EnvFile(Box::new(error)))?; - vars.insert(key, serde_json::Value::from(value)); - } - Ok(vars) -} - /// Renders the variables as dotenv lines, refusing names and values the /// boot-time parser could not round-trip. fn render_env(vars: &BTreeMap) -> Result { diff --git a/crates/gateway/app/src/commands/apply.rs b/crates/gateway/app/src/commands/apply.rs index e487af64a..0490341ef 100644 --- a/crates/gateway/app/src/commands/apply.rs +++ b/crates/gateway/app/src/commands/apply.rs @@ -20,7 +20,7 @@ use tokio_util::sync::CancellationToken; use super::{APPLY_CONFIG_LABEL, Outcome}; use crate::AppState; -use crate::config_shadow::{canonical_form, config_root, relative_name, shadow_census}; +use crate::config_shadow::{PendingEnv, canonical_form, config_root, relative_name, shadow_census}; use crate::error::{GatewayError, blocking, config_write_error}; use crate::routing::Routing; @@ -132,9 +132,12 @@ pub(crate) fn capture_apply(config_path: &Path) -> Result (tempfile::TempDir, PathBuf) { + let temp = tempfile::TempDir::new().expect("temp dir"); + let path = temp.path().join("gateway.toml"); + (temp, path) +} + +#[test] +fn when_reading_the_pending_env_the_shadow_wins_over_the_real_env_file() { + let (_temp, config_path) = config_path(); + let env_path = config_path.with_extension("env"); + + let var_name = "PF_TEST_KEY"; + let value = "sk-A"; + let shadow_value = "sk-B"; + + std::fs::write(&env_path, format!("{var_name}={value}\n")).expect("error writing env file"); + std::fs::write( + shadow_path(&env_path), + format!("{var_name}={shadow_value}\n"), + ) + .expect("error writing env shadow"); + + let pending = PendingEnv::new(&config_path).expect("pending env reads"); + + let actual = pending + .resolve_var(var_name) + .expect("Could not resolve test var"); + let expected = shadow_value; + + assert_eq!(actual, expected, "Got `{actual}` but expected `{expected}`"); +} + +#[test] +fn no_shadow_env_file_is_an_empty_pending_env() { + let (_temp, config_path) = config_path(); + + let pending = PendingEnv::new(&config_path).expect("Error building pending env"); + + assert!(pending.data.is_empty()); + assert_eq!( + pending.resolve_var("PF_TEST_UNSET_EVERYWHERE"), + Err(VarError::NotPresent) + ); +} + +#[test] +fn when_reading_the_pending_env_the_shadow_file_wins_over_the_process_environment() { + // PATH is set in every test process, so it stands in for a variable + // both the file and the process define. + + let (_temp, config_path) = config_path(); + std::fs::write(config_path.with_extension("env.next"), "PATH=from-file\n") + .expect("Error writing env file"); + + let pending = PendingEnv::new(&config_path).expect("Error building pending env"); + + assert_eq!(pending.resolve_var("PATH"), Ok("from-file".to_owned())); +} + +#[test] +fn a_variable_only_in_the_process_environment_resolves_from_it() { + let (_temp, config_path) = config_path(); + + let pending = PendingEnv::new(&config_path).expect("Error building pending env"); + + assert_eq!(pending.resolve_var("PATH"), std::env::var("PATH")); +} + +#[test] +fn a_malformed_env_file_is_an_error() { + let (_temp, config_path) = config_path(); + std::fs::write( + config_path.with_extension("env.next"), + r#"="I'm only a value with no name!"#, + ) + .expect("error writing env file"); + + assert!(PendingEnv::new(&config_path).is_err()); +} diff --git a/crates/gateway/app/src/config_shadow.rs b/crates/gateway/app/src/config_shadow.rs index ed1c14815..6b17349bc 100644 --- a/crates/gateway/app/src/config_shadow.rs +++ b/crates/gateway/app/src/config_shadow.rs @@ -1,5 +1,6 @@ //! Shadow-file bookkeeping: which real config files have a pending //! `.next` shadow, and how those paths are rendered for the wire. +//! Provides a struct for reading the pending environment from the shadow files. //! //! Three readers share this. `GET /admin/config-dirty` reports the census //! as pending state, `POST /admin/config-apply` takes it under the apply @@ -8,6 +9,8 @@ //! `gateway-config`; this module only assembles the census and puts its //! paths in comparable and displayable form. +use std::collections::BTreeMap; +use std::env::VarError; use std::path::{Path, PathBuf}; use gateway_config::{pending_report, shadow_path}; @@ -85,3 +88,68 @@ pub(crate) fn relative_name(file: &Path, root: Option<&Path>) -> String { .collect(); parts.join("/") } + +/// Struct for accessing the environment variables the next boot will run with. +pub(crate) struct PendingEnv { + data: BTreeMap, +} + +impl PendingEnv { + /// Constructs `Self` from env files. + /// If a shadow env file is staged, it reads that, otherwise the real file. + pub(crate) fn new(config_path: &Path) -> Result { + let path = { + let env_path = config_path.with_extension("env"); + let shadow_path = shadow_path(&env_path); + + if shadow_path.is_file() { + shadow_path + } else { + env_path + } + }; + + Ok(Self { + data: read_env_file(&path)?, + }) + } + + /// Resolves a `${VAR}` from the pending environment. + /// If a value is not present in the pending environment, it checks the current + /// running environment. + /// + /// TODO: This function is not able to distinguish between variables set in an + /// env file and overrides set in the shell. If the pending environment defines a + /// variable `VAR=abc` but the shell environment overrides it with `VAR=def`, + /// this will return `abc` but when the gateway restarts the value will be `def` + pub(crate) fn resolve_var(&self, name: &str) -> Result { + match self.data.get(name) { + Some(value) => Ok(value.clone()), + None => std::env::var(name), + } + } +} + +/// Parses a dotenv file and returns a map without changing the process environment; a +/// missing file is an empty map. +pub(crate) fn read_env_file(path: &Path) -> Result, GatewayError> { + let mut vars = BTreeMap::new(); + + if !path.is_file() { + return Ok(vars); + } + + let entries = + dotenvy::from_path_iter(path).map_err(|error| GatewayError::EnvFile(Box::new(error)))?; + + for entry in entries { + let (key, value) = entry.map_err(|error| GatewayError::EnvFile(Box::new(error)))?; + vars.insert(key, value); + } + + Ok(vars) +} + +#[cfg(test)] +#[path = "config_shadow-tests.rs"] +mod tests; diff --git a/crates/gateway/config/src/config.rs b/crates/gateway/config/src/config.rs index 76b65abb3..64f914601 100644 --- a/crates/gateway/config/src/config.rs +++ b/crates/gateway/config/src/config.rs @@ -20,6 +20,7 @@ pub use companion::{ SpeculativeConfig, }; pub(crate) use imp::reject_profiles_directory; +pub use interpolate::VarLookupFn; use interpolate::interpolate_value; // The canonical home of the model-metadata types is `gateway-api-types`. pub use gateway_api_types::{Capabilities, ModelKind, ThinkingMode}; diff --git a/crates/gateway/config/src/config/imp.rs b/crates/gateway/config/src/config/imp.rs index 77463fca4..cdc4d15d3 100644 --- a/crates/gateway/config/src/config/imp.rs +++ b/crates/gateway/config/src/config/imp.rs @@ -11,7 +11,10 @@ use std::path::Path; use serde::Deserialize; -use super::{Config, RawConfig, RawSttPipelineConfig, Secret, WebSearchConfig, interpolate_value}; +use super::{ + Config, RawConfig, RawSttPipelineConfig, Secret, VarLookupFn, WebSearchConfig, + interpolate_value, +}; use crate::error::ConfigError; use crate::profile::{ProfileName, ProfileSelection, SelectionSource, resolve_selection}; @@ -175,6 +178,16 @@ impl Config { } pub(crate) fn parse_toml_at(raw: &str, path: Option<&Path>) -> Result { + let lookup = |name: &str| std::env::var(name); + Self::parse_toml_with_lookup(raw, path, &lookup) + } + + /// [`Config::parse_toml_at`] with `${VAR}` resolved through a custom `lookup` function + pub(crate) fn parse_toml_with_lookup( + raw: &str, + path: Option<&Path>, + lookup: VarLookupFn, + ) -> Result { // Parse first, then interpolate only string *values*. Interpolating the // raw text would expand `${VAR}` inside comments and keys, and an // interpolated value containing a quote, backslash, or newline would @@ -184,13 +197,13 @@ impl Config { source: Box::new(source), })?; reject_removed_layout(raw, path)?; - Self::from_value(document) + Self::from_value(document, lookup) } /// Interpolates string leaves, deserializes, and validates an already /// parsed TOML document. - fn from_value(mut document: toml::Value) -> Result { - interpolate_value(&mut document)?; + fn from_value(mut document: toml::Value, lookup: VarLookupFn) -> Result { + interpolate_value(&mut document, lookup)?; let raw: RawConfig = document.try_into().map_err(|source| ConfigError::Parse { path: None, source: Box::new(source), diff --git a/crates/gateway/config/src/config/interpolate.rs b/crates/gateway/config/src/config/interpolate.rs index c1861f5cc..b4bebafe4 100644 --- a/crates/gateway/config/src/config/interpolate.rs +++ b/crates/gateway/config/src/config/interpolate.rs @@ -7,12 +7,17 @@ use crate::error::ConfigError; -/// Expands `${VAR}` from the environment; `$$` is a literal `$`. +/// Looks up a concrete value for a `${VAR}` +/// +/// Uses `std::env::var` semantics, so an unknown var returns `VarError::NotPresent`. +pub type VarLookupFn<'a> = &'a dyn Fn(&str) -> Result; + +/// Expands `${VAR}` from lookup; `$$` is a literal `$`. /// /// # Errors /// Returns [`ConfigError::Interpolation`] on an unclosed `${...}` and /// [`ConfigError::UnresolvedVar`] when a referenced variable is unset. -pub(super) fn interpolate(input: &str) -> Result { +pub(super) fn interpolate(input: &str, lookup: VarLookupFn) -> Result { let mut out = String::with_capacity(input.len()); let mut chars = input.chars().peekable(); while let Some(c) = chars.next() { @@ -41,7 +46,7 @@ pub(super) fn interpolate(input: &str) -> Result { "unclosed ${...} interpolation".to_string(), )); } - let value = std::env::var(&name) + let value = lookup(&name) .map_err(|source| ConfigError::UnresolvedVar(name.clone(), source))?; out.push_str(&value); } @@ -54,19 +59,22 @@ pub(super) fn interpolate(input: &str) -> Result { /// Recursively interpolates `${VAR}` in every string leaf of a TOML value, /// leaving keys, comments (already stripped by the parser), and non-string /// scalars untouched. (CFG-007) -pub(super) fn interpolate_value(value: &mut toml::Value) -> Result<(), ConfigError> { +pub(super) fn interpolate_value( + value: &mut toml::Value, + lookup: VarLookupFn<'_>, +) -> Result<(), ConfigError> { match value { toml::Value::String(text) => { - *text = interpolate(text)?; + *text = interpolate(text, lookup)?; } toml::Value::Array(items) => { for item in items { - interpolate_value(item)?; + interpolate_value(item, lookup)?; } } toml::Value::Table(table) => { for (_, entry) in table.iter_mut() { - interpolate_value(entry)?; + interpolate_value(entry, lookup)?; } } _ => {} @@ -76,20 +84,27 @@ pub(super) fn interpolate_value(value: &mut toml::Value) -> Result<(), ConfigErr #[cfg(test)] mod tests { - use super::interpolate; + use super::*; + + fn config_lookup(name: &str) -> Result { + std::env::var(name) + } #[test] fn double_dollar_is_literal() { - assert_eq!(interpolate("cost is $$5").unwrap(), "cost is $5"); + assert_eq!( + interpolate("cost is $$5", &config_lookup).unwrap(), + "cost is $5" + ); } #[test] fn unset_variable_is_an_error() { - assert!(interpolate("${PFG_DEFINITELY_UNSET_VAR_XYZ}").is_err()); + assert!(interpolate("${PFG_DEFINITELY_UNSET_VAR_XYZ}", &config_lookup).is_err()); } #[test] fn unclosed_interpolation_is_an_error() { - assert!(interpolate("${UNCLOSED").is_err()); + assert!(interpolate("${UNCLOSED", &config_lookup).is_err()); } } diff --git a/crates/gateway/config/src/config/tests.rs b/crates/gateway/config/src/config/tests.rs index 7d273fc3b..764f4a4c9 100644 --- a/crates/gateway/config/src/config/tests.rs +++ b/crates/gateway/config/src/config/tests.rs @@ -23,6 +23,10 @@ upstream = "u1" endpoints = ["anthropic"] "#; +fn config_lookup(name: &str) -> Result { + std::env::var(name) +} + #[test] fn parses_a_valid_config() { let config = Config::from_toml_str(SAMPLE).unwrap(); @@ -276,8 +280,11 @@ mystery = true #[test] fn interpolates_and_escapes() { // SAFETY-free: reading is fine; this test sets no env vars. - assert_eq!(interpolate("a$$b").unwrap(), "a$b"); - assert_eq!(interpolate("no vars here").unwrap(), "no vars here"); + assert_eq!(interpolate("a$$b", &config_lookup).unwrap(), "a$b"); + assert_eq!( + interpolate("no vars here", &config_lookup).unwrap(), + "no vars here" + ); } #[test] @@ -314,7 +321,7 @@ endpoints = ["e"] fn unresolved_variable_is_an_error() { let missing = "${PROMPTFORGE_DEFINITELY_UNSET_VAR_XYZ}"; assert!(matches!( - interpolate(missing), + interpolate(missing, &config_lookup), Err(ConfigError::UnresolvedVar(..)) )); } @@ -322,7 +329,7 @@ fn unresolved_variable_is_an_error() { #[test] fn unclosed_interpolation_is_an_error() { assert!(matches!( - interpolate("${OPEN"), + interpolate("${OPEN", &config_lookup), Err(ConfigError::Interpolation(_)) )); } diff --git a/crates/gateway/config/src/lib.rs b/crates/gateway/config/src/lib.rs index 30cb03d2e..0330f35ad 100644 --- a/crates/gateway/config/src/lib.rs +++ b/crates/gateway/config/src/lib.rs @@ -45,7 +45,7 @@ pub use crate::config::{ MultimodalProjectorConfig, ProfileConfig, Protocol, QueuePolicy, RECOMMENDED_STT_MODELS, RecommendedSttModel, SILERO_VAD_MODEL, SearchProvider, Secret, ServerConfig, SpeculationType, SpeculativeConfig, SpeechArtifactPin, SttModelConfig, SttPipelineConfig, SttRole, ThinkingMode, - ToolDialect, ToolsConfig, WebSearchConfig, WhisperBackend, WorkshopConfig, + ToolDialect, ToolsConfig, VarLookupFn, WebSearchConfig, WhisperBackend, WorkshopConfig, }; pub use crate::profile::{ ProfileName, ProfileNameError, ProfileSelection, ProfileState, profile_state_path, diff --git a/crates/gateway/config/src/shadow-tests.rs b/crates/gateway/config/src/shadow-tests.rs index de274cf26..f53a3744a 100644 --- a/crates/gateway/config/src/shadow-tests.rs +++ b/crates/gateway/config/src/shadow-tests.rs @@ -42,6 +42,10 @@ fn state_shadow(path: &Path) -> PathBuf { shadow_path(&crate::profile_state_path(path)) } +fn process_env_lookup(name: &str) -> Result { + std::env::var(name) +} + #[test] fn write_atomic_replaces_the_real_file_and_leaves_its_shadow_alone() { let (_temp, path) = write_config(); @@ -95,7 +99,8 @@ fn active_profile_in_the_pending_document_is_refused_and_writes_no_shadow() { Value::String("work".to_owned()), ); - let error = save_config_shadow(&path, document).expect_err("active_profile is refused"); + let error = save_config_shadow(&path, document, &process_env_lookup) + .expect_err("active_profile is refused"); assert_eq!(error.kind(), crate::ConfigErrorKind::Validation); assert!( @@ -110,7 +115,8 @@ fn active_profile_in_the_pending_document_is_refused_and_writes_no_shadow() { fn pending_save_writes_only_the_config_shadow() { let (_temp, path) = write_config(); - let shadows = save_config_shadow(&path, document(CONFIG)).expect("pending save"); + let shadows = + save_config_shadow(&path, document(CONFIG), &process_env_lookup).expect("pending save"); assert_eq!( shadows, @@ -128,7 +134,7 @@ fn pending_save_restores_redacted_secrets() { let mut document = document(CONFIG); document["server"]["api_key"] = Value::String("***".to_owned()); - let shadows = save_config_shadow(&path, document).expect("pending save"); + let shadows = save_config_shadow(&path, document, &process_env_lookup).expect("pending save"); assert!( fs::read_to_string(shadows.config) @@ -144,7 +150,8 @@ fn pending_save_rejects_invalid_profiles() { let mut document = document(CONFIG); document["profile"][1]["models"] = Value::Array(vec![Value::String("ghost".to_owned())]); - let error = save_config_shadow(&path, document).expect_err("unknown member fails"); + let error = + save_config_shadow(&path, document, &process_env_lookup).expect_err("unknown member fails"); assert_eq!(error.kind(), crate::ConfigErrorKind::Validation); assert!(!shadow_path(&path).exists()); @@ -160,7 +167,8 @@ fn pending_save_accepts_a_document_that_drops_the_persisted_profile() { .expect("write state"); let candidate = CONFIG.replace("[[profile]]\nname = \"work\"\nmodels = [\"local\"]\n\n", ""); - let shadows = save_config_shadow(&path, document(&candidate)).expect("stale state degrades"); + let shadows = save_config_shadow(&path, document(&candidate), &process_env_lookup) + .expect("stale state degrades"); assert!(shadows.config.is_file()); } @@ -169,8 +177,12 @@ fn pending_save_accepts_a_document_that_drops_the_persisted_profile() { fn pending_loader_honors_a_supplied_selection() { let (_temp, path) = write_config(); - let config = load_pending_config(&path, &ProfileSelection::new(Some("travel"), None)) - .expect("pending loads"); + let config = load_pending_config( + &path, + &ProfileSelection::new(Some("travel"), None), + &process_env_lookup, + ) + .expect("pending loads"); assert_eq!(config.active_profile().expect("selected").name(), "travel"); assert!(config.local_models().is_empty()); @@ -180,7 +192,8 @@ fn pending_loader_honors_a_supplied_selection() { fn pending_loader_accepts_no_selection() { let (_temp, path) = write_config(); - let config = load_pending_config(&path, &ProfileSelection::default()).expect("pending loads"); + let config = load_pending_config(&path, &ProfileSelection::default(), &process_env_lookup) + .expect("pending loads"); assert!(config.active_profile().is_none()); assert!(config.local_models().is_empty()); @@ -201,7 +214,8 @@ fn pending_loader_reads_the_state_file_and_ignores_a_state_shadow() { ) .expect("write leftover state shadow"); - let config = load_pending_config(&path, &ProfileSelection::default()).expect("pending loads"); + let config = load_pending_config(&path, &ProfileSelection::default(), &process_env_lookup) + .expect("pending loads"); assert_eq!(config.active_profile().expect("selected").name(), "work"); assert_eq!(config.local_models().len(), 1); @@ -216,7 +230,8 @@ fn pending_loader_degrades_a_stale_state_file_like_load() { ) .expect("write state"); - let config = load_pending_config(&path, &ProfileSelection::default()).expect("pending loads"); + let config = load_pending_config(&path, &ProfileSelection::default(), &process_env_lookup) + .expect("pending loads"); assert!(config.active_profile().is_none()); assert_eq!(config.stale_state_selection(), Some("missing")); @@ -226,8 +241,12 @@ fn pending_loader_degrades_a_stale_state_file_like_load() { fn pending_loader_rejects_an_undefined_ephemeral_selection() { let (_temp, path) = write_config(); - let error = load_pending_config(&path, &ProfileSelection::new(Some("missing"), None)) - .expect_err("a typed override must name a defined profile"); + let error = load_pending_config( + &path, + &ProfileSelection::new(Some("missing"), None), + &process_env_lookup, + ) + .expect_err("a typed override must name a defined profile"); assert_eq!(error.kind(), crate::ConfigErrorKind::Validation); assert!(error.to_string().contains("missing")); diff --git a/crates/gateway/config/src/shadow.rs b/crates/gateway/config/src/shadow.rs index 608fed5ca..6d7e2154d 100644 --- a/crates/gateway/config/src/shadow.rs +++ b/crates/gateway/config/src/shadow.rs @@ -12,6 +12,7 @@ use std::sync::atomic::{AtomicU64, Ordering}; use toml::Value; use self::content::restore_secrets; +use crate::VarLookupFn; use crate::config::Config; use crate::error::ConfigError as Repr; use crate::profile::{ProfileName, ProfileSelection, ProfileState}; @@ -211,6 +212,9 @@ pub fn clear_profile_state(config_path: &Path) -> Result<(), crate::ConfigError> /// document: a state file naming a profile the document drops degrades to /// "no profile" at the next load, the same way [`Config::load`] treats it. /// +/// The `lookup` argument provides a function to resolve `${VAR}`s against the +/// pending environment. +/// /// # Errors /// Returns [`ConfigError`](crate::ConfigError) when the document is malformed /// or contains `active_profile`, a secret cannot be restored, the config is @@ -218,6 +222,7 @@ pub fn clear_profile_state(config_path: &Path) -> Result<(), crate::ConfigError> pub fn save_config_shadow( config_path: &Path, mut document: Value, + lookup: VarLookupFn, ) -> Result { crate::config::reject_profiles_directory(config_path).map_err(crate::ConfigError::from)?; let table = document.as_table().ok_or_else(|| { @@ -237,7 +242,8 @@ pub fn save_config_shadow( let rendered = toml::to_string_pretty(&document).map_err(|error| { crate::ConfigError::validation(format!("pending config does not render as TOML: {error}")) })?; - Config::parse_toml_at(&rendered, Some(config_path)).map_err(crate::ConfigError::from)?; + Config::parse_toml_with_lookup(&rendered, Some(config_path), lookup) + .map_err(crate::ConfigError::from)?; let config = write_shadow(config_path, &rendered)?; Ok(PendingShadows { config }) } @@ -250,12 +256,16 @@ pub fn save_config_shadow( /// drops degrades to no profile, reported through /// [`Config::stale_state_selection`]. /// +/// The `lookup` argument provides a function to resolve `${VAR}`s against the +/// pending environment. +/// /// # Errors /// Returns [`ConfigError`](crate::ConfigError) under the same conditions as /// [`Config::load`], or when a pending shadow cannot be read. pub fn load_pending_config( config_path: &Path, inputs: &ProfileSelection, + lookup: VarLookupFn, ) -> Result { crate::config::reject_profiles_directory(config_path).map_err(crate::ConfigError::from)?; let (source_path, raw) = read_pending_or_real(config_path) @@ -266,11 +276,12 @@ pub fn load_pending_config( source: std::io::Error::new(std::io::ErrorKind::NotFound, "config not found"), }) })?; - let config = Config::parse_toml_at( + let config = Config::parse_toml_with_lookup( &toml::to_string(&raw).map_err(|error| { crate::ConfigError::validation(format!("pending config does not render: {error}")) })?, Some(&source_path), + lookup, ) .map_err(crate::ConfigError::from)?; // The state file sits beside the real config, not the shadow. From c2baa0571c4226bed567f09bc9569799e1bb46ce Mon Sep 17 00:00:00 2001 From: Andrew Wheeler Date: Fri, 9 Oct 2026 11:58:30 -0600 Subject: [PATCH 2/2] restart-to-apply plan --- vibe/2026-10-06-1-restart-to-apply.md | 205 ++++++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 vibe/2026-10-06-1-restart-to-apply.md diff --git a/vibe/2026-10-06-1-restart-to-apply.md b/vibe/2026-10-06-1-restart-to-apply.md new file mode 100644 index 000000000..340313125 --- /dev/null +++ b/vibe/2026-10-06-1-restart-to-apply.md @@ -0,0 +1,205 @@ +--- +name: Restart to Apply and Gateway supervisor +overview: "Make config changes that need a restart land atomically with that restart, so the files, the UI, and the running gateway never disagree. The default `promptforge-gateway` launch becomes a long-lived, background supervisor process that runs the serving gateway, tray included, as a child; that child asks for a restart by exiting with `RESTART_REQUESTED`, and the supervisor relaunches it on the same port. If the new gateway fails to boot, the supervisor rolls back the new config to pending and retries. In the config UI, whenever pending changes need a restart, 'Apply' becomes 'Restart to Apply'." +todos: + - id: pending-env-validation + content: "Resolve ${VAR} at save and capture against the pending env file, then the process env, so a key staged on the Secrets tab validates the endpoint that references it" + status: completed + - id: restart-contract + content: "Child side: --supervised flag, stdin-EOF watchdog, `RESTART_REQUESTED` exit code, POST /admin/restart (promote with .prev, then shut down), restart_required on GET /admin/config-dirty" + status: pending + - id: bind-hint + content: "--port-hint for a config that binds port 0; bind through TcpSocket with SO_REUSEADDR on Unix" + status: pending + - id: supervisor-headless + content: "Supervisor mode: launch election, child spawn, exit-code loop, retries for restarts only (capped), .prev rollback, failure record; integration tests against the real binary" + status: pending + - id: config-ui + content: "Config UI: Restart to Apply button, Restart now on the profile banner, rollback failure display, unsupervised fallback; harness and tests" + status: pending + - id: docs + content: "User guide (promptforge-docs) and gateway README: supervisor model, Restart to Apply, rollback" + status: pending +isProject: false +--- + +# Restart to Apply and Gateway Supervisor + +## Problem + +Currently, 'Apply' promotes shadows (the `.next` files, such as `gateway.toml.next` and `gateway.env.next`) to the real files. When the change touches a section the process reads once at boot (`RESTART_SECTIONS` in `crates/gateway/app/src/commands-apply.rs:30`) or the env file (`commands-apply.rs:110`), 'Apply' reports `restart_required` and the gateway keeps running the old config until someone restarts it by hand. Until then the files and the UI show one config and the process runs another. Every API key edit takes this path, because the env file reaches the process only at boot (`runner.rs:1221`, `load_env_file`). + +Three further defects come from the same gap: + +- A restart cannot fail safely. `load_startup` errors are fatal (`main.rs:154-172`), promotion keeps no previous copy (`gateway/config/src/shadow.rs:164-207`, the backup is deleted on success), and the config UI is served by the process that just failed to boot. +- The restart banner (`config-ui/ui/src/main.ts:238`) can only clear when `[server]` names a fixed port. The generated default binds `127.0.0.1:0` (`boot.rs:544`), so a restarted gateway answers on a new port and the open tab never sees it. +- A new provider's API key cannot be used until the gateway has restarted with it: + 1. On the Secrets tab, add an `ANTHROPIC_API_KEY` and save. The value lands in `gateway.env.next`. + 2. On the Cloud Models page, pick an Anthropic model and click Add Model. `mergeCloudModel` (`config-ui/ui/src/services/cloud-merge.ts:41-66`) adds the provider's endpoint with `api_key = "${ANTHROPIC_API_KEY}"` and saves the whole document through `PUT /admin/config`. + 3. The save is refused with `UnresolvedVar` for `ANTHROPIC_API_KEY`, shown in the Add Model dialog, and nothing is staged. + + Applying the Secrets change first does not help: `gateway.env` then holds the key, but the running process still does not. Only a restart makes the save succeed. + The cause is save-time validation: `save_config_shadow` (`gateway/config/src/shadow.rs:325-350`) runs the document through `Config::parse_toml_at`, which validates every `${VAR}` against the process environment (`config/src/config/interpolate.rs:44`). Since the process environment holds only what the boot loaded from `gateway.env` (`runner.rs:1221`), a key entered since boot is invisible to it, whether staged or applied. Typing `${VAR}` into an endpoint's API key on the Settings page goes through the same route and validation, so it fails the same way. The documented workflow (enter the key on the Secrets tab, then add the provider) therefore takes three trips: save the key, apply and restart, then add the model. + +## Solution in brief + +When a change requires a restart, the gateway will restart itself as part of applying it (and communicate that to the user), so the files and the running gateway never disagree. + +To make that possible, starting the gateway will start two processes instead of one. A small, background supervisor starts first and runs the real gateway beneath it. When the gateway needs a restart, it shuts itself down and requests a restart from the supervisor through its exit code, and the supervisor starts a fresh gateway. Because the supervisor stays running throughout, anything that tries to start or connect to the gateway during the gap waits for the new one instead of starting a second gateway. The new gateway comes back on the same port, so an open config UI tab reconnects by itself. + +The changes, in the order they will be made: + +1. **Check new settings against new secrets.** Today, adding a model from the Cloud Models page fails if its provider's API key was entered on the Secrets tab after the gateway started, even once that key has been applied. Config edits will be checked against the keys on file, so the model can be added straight after its key. +2. **Teach the gateway to ask for a restart.** A new admin request applies the pending changes, keeps a copy of the files it replaced, and restarts the gateway. +3. **Add a way to choose the port at startup.** A new command line option asks the gateway to use a given port when its config lets the system choose one. Nothing uses it yet; the supervisor in the next change uses it to bring each restarted gateway back on the previous port, so the address stays the same. +4. **Add the supervisor.** It starts the gateway and restarts it when asked. If a restarted gateway fails to start, the supervisor puts the previous config back, returns the rejected edit to the pending list, and starts the gateway again on the old config, which then reports what went wrong. The supervisor relaunches only for restart requests. A gateway that fails on its first start, or crashes while running, ends the supervisor exactly as the gateway ends today, so systemd and the Workshop desktop app recover from crashes as they do now. +5. **Replace Apply with Restart to Apply in the config UI.** When pending changes need a restart, the Apply button becomes Restart to Apply and does both in one step. The UI waits for the gateway to return, and if the restart was rolled back it shows why, with the edit still pending. +6. **Update the documentation.** + +What stays the same: the tray icon (it belongs to the gateway, so it disappears for a moment during a restart), the systemd unit file, how the Workshop desktop app starts and supervises its gateway, and changes that already apply without a restart. + +## Target shape + +```mermaid +flowchart TD + Launch["promptforge-gateway (bare, --login, --no-tray)"] --> Sup[supervisor] + Desktop[workshop desktop supervisor] -->|"bare launch, unchanged"| Sup + Sup -->|"spawn --supervised --port-hint P, stdin pipe"| Child["serving gateway (tray unless --no-tray)"] + Child -->|"exit 0"| SupExit[supervisor exits] + Child -->|"exit RESTART_REQUESTED"| Restart[relaunch, roll back on failure] + Child -->|"any other code, or first boot fails"| Exit[supervisor exits with the child's code] + Restart --> Child +``` + +```mermaid +sequenceDiagram + participant UI as Config UI + participant C1 as child (old) + participant S as supervisor + participant C2 as child (new) + UI->>C1: POST /admin/restart + C1->>C1: validate pending, promote shadows, keep .prev + C1-->>UI: 202 + C1->>C1: graceful shutdown, exit RESTART_REQUESTED + S->>S: hold launch lock + S->>C2: spawn --supervised --port-hint P + alt C2 publishes a validated discovery file in time + S->>S: delete .prev, release launch lock + UI->>C2: GET /admin/status (new config_generation) + else C2 fails to boot + S->>S: real -> .next, .prev -> real, write failure record + S->>C2: spawn again on the restored config + UI->>C2: GET /admin/status (restart_failure, pending shadows) + end +``` + +## Decisions + +- **Restart is an exit code, not a spawn.** A child that wants a restart shuts down gracefully and exits with `RESTART_REQUESTED`, one constant in the gateway crate (value 75) that `main` and the supervisor both name. The child never launches its successor, so the lease and port races of a self-restart do not exist. The code is private to the supervisor and its child; the supervisor never exits with it, so the value carries no meaning outside the pair beyond being distinct from 0 and 1 and below the 128+N signal range. +- **No service-manager special case.** The shipped unit (`crates/gateway/app/packaging/gateway.service`) runs `promptforge-gateway --config ... --profile main` with `Restart=on-failure`; that launch becomes a supervisor like any other, and the supervisor's own exit behaves as the gateway's does today: 0 on a stop, and the child's own non-zero code at once when the first boot fails or a running gateway dies, so systemd's restart policy applies unchanged and on the same schedule. A UI restart happens inside the supervisor and systemd never sees it. `--supervised` is an internal flag between the supervisor and its child, not documented for users. The unit file does not change. +- **The supervisor is the same binary.** A bare launch, `--login`, and `--no-tray` run the supervisor; `--supervised` runs the serving gateway as today, tray included unless `--no-tray` is also given. The Windows Run key, the Linux autostart file, macOS `SMAppService`, the installer, and the workshop desktop's bare launch (`workshop/desktop/src/gateway/boot.rs:166-173`) all keep working with no change to what they launch. +- **The child keeps the instance lease; the supervisor uses the launch election.** The discovery file and the lease identify the serving process by pid and image (`runner.rs:1071`, `gateway-api-discovery/src/validated.rs:16-19`), so they stay with the child and `settle_gateway_startup` is untouched. The supervisor settles its own start with `launch_or_attach` (`gateway-api-discovery/src/lock.rs:106`): `Attach` is today's second-launch handoff (open Settings unless `--login`), `Launch(lock)` spawns the first child. Across a restart the supervisor takes `gateway.json.lock` before the old child exits and holds it until the new child publishes. The desktop supervisor, which sees the gap as `Missing` on its 5 s probe (`workshop/desktop/src/gateway/supervisor.rs:88-108`), then waits in its own `launch_or_attach` and attaches to the new child instead of launching a competitor. This reuses the election exactly as the lock's doc describes it ("a Workshop parent may hold the launch election while the spawned Gateway acquires this lease", `lock.rs:35-37`). +- **Orphan safety through a stdin pipe.** The supervisor gives the child a piped stdin; a supervised child watches it on a thread and fires `ShutdownSignal` on EOF. Killing the supervisor closes the pipe on every OS, so the child shuts down gracefully and its `llama-server` children drop through `ServerGuard::drop` (`gateway/local/src/server.rs:586-594`). No Job Object, `PR_SET_PDEATHSIG`, or new unsafe code. Orphaned `llama-server` children after a hard kill of the serving process itself are a pre-existing gap and out of scope. +- **Retries belong to restarts only.** The supervisor relaunches only after a child exits with `RESTART_REQUESTED`. A first boot that fails, or a running child that exits with any other code, ends the supervisor with that child's exit code, exactly as the gateway process ends today; the desktop supervisor and systemd keep owning crash recovery. Within a restart, failed launches retry with backoff from 250 ms doubling, at most 5 launches in all; a restart that exhausts them ends the supervisor with the last child's code. +- **Rollback is file state, not IPC.** `POST /admin/restart` writes `.prev` for every real file it promotes. The supervisor reads the presence of `.prev` files as "this restart carries a change". On success it deletes them; on failure it moves each real file back to `.next` (the rejected edit becomes pending again, visible and fixable in the UI) and each `.prev` back to the real path, then relaunches. A child that also fails on the restored config is retried within the restart's cap. +- **The failure record is a run-directory file.** On rollback the supervisor writes `restart-failure.json` beside the discovery file; the next child reads it, deletes it, and reports it once. A file carries the stderr tail without command-line length limits and needs no new channel. +- **Health is publication.** A new child counts as booted when it publishes a discovery file that `ValidatedConnection` accepts, within 30 s (the desktop's `RECOVERY_TIMEOUT`, `workshop/desktop/src/gateway/supervisor/launch.rs:14`). The boot load of local models runs after publication (`boot_load.rs`) and does not fail the boot, so it is not part of the criterion. +- **The tray stays with the serving gateway.** The supervisor is windowless and the tray code is untouched: it keeps reading in-process state through `GatewayHandle` and `AppState`. Quit stops the child, which exits 0, and the supervisor exits on 0. During a restart the icon disappears and returns once the new child starts, before its bind. When a restart exhausts its retries the supervisor exits and nothing is on screen, which matches today, where a boot failure ends the process. Moving the tray into the supervisor (a persistent icon, a Restarting phase, an Error state while no child runs) would replace every in-process tray coupling with HTTP, and is left for a follow-up if the icon gap proves a problem. +- **Restart to Apply is decided by the server.** `GET /admin/config-dirty` gains `restart_required`, computed by the same census `capture_apply` uses, so `RESTART_SECTIONS` stays the only list. + +## Repo conventions that bind this plan + +- `crates/gateway/app` has no `## Invariants` marker, so the 500-line ceiling does not bind it; split large new modules anyway. A new `supervisor` area with three or more files goes in `src/supervisor/` beside `src/supervisor.rs`; one or two files are `supervisor-