From 1eca4577458d7a254e6de2e43c5bb28f2b1761e5 Mon Sep 17 00:00:00 2001 From: Jonas Grosch Date: Thu, 1 Oct 2026 02:09:50 +0200 Subject: [PATCH 1/2] feat(config): unknown WALGIT__ overrides fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An environment override this build could not apply was dropped with a WARN: `WALGIT__SERVER__LSITEN=0.0.0.0:8080` left the server on the default 127.0.0.1:8080, a wrong-typed value likewise ran the default. The file itself already refuses unknown keys (`deny_unknown_fields`); the env path now does the same. A malformed name, an unknown section or key, or a value of the wrong type is an error at startup and in `walgit config check`, naming every such variable at once and, for an unknown key, the nearest known one: WALGIT__SERVER__LSITEN: unknown key `lsiten` in [server]; did you mean WALGIT__SERVER__LISTEN? Nothing of a refused batch is applied. The nearest key comes from serde's own `expected one of` list (Levenshtein over the siblings, no new dependency). No escape hatch. The warn-and-ignore loader existed for config/image skew (2026-08-21: a host on an older image crash-looped on a newer key), not for foreign variables sharing the prefix — nothing in the tree sets a non-config `WALGIT__*` name. That case is answered by ordering (image before the env that needs it) and by `config check --env-file` run by the binary that will serve; `--strict` and `apply_env_report` are removed, as the check is now always strict. Recorded as D50. walgit-config no longer uses tracing. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 8 ++ Cargo.lock | 1 - crates/walgit-cli/src/config_cmd.rs | 19 +-- crates/walgit-cli/src/lib.rs | 7 +- crates/walgit-cli/tests/config_env.rs | 94 +++++++++++++ crates/walgit-config/Cargo.toml | 1 - crates/walgit-config/src/lib.rs | 193 ++++++++++++++++++-------- docs/PACKFILE_MIGRATION.md | 6 +- walgit.example.toml | 3 +- 9 files changed, 247 insertions(+), 85 deletions(-) create mode 100644 crates/walgit-cli/tests/config_env.rs diff --git a/AGENTS.md b/AGENTS.md index 8fc7d20b..b2231e1d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -487,6 +487,14 @@ full cold-read/resource acceptance gates listed in `docs/spec/README.md`. and candidate external-boundary proof remain separate obligations; local loose objects and retired download membership cannot justify retirement. See the cost and remaining-evidence rows in the linked docs. +- **D50 (2026-10-01): `WALGIT__` overrides fail closed.** An environment override this build cannot + apply (malformed name, unknown section or key, wrong type) is a startup and `config check` error that + names each variable and the nearest known key; nothing of the batch is applied. This supersedes the + warn-and-ignore loader (2026-08-21: a key newer than one host's image crash-looped it): a host running + on a default it was told to change is the worse failure, and the file already refuses unknown keys. + Roll the image before the env that needs it; gate the rollout on `walgit config check --env-file` + run by the binary that will serve. + ## 5. Working rules - **No backwards compatibility (pre-1.0, banner at top):** change the shape and delete the old one in the same diff --git a/Cargo.lock b/Cargo.lock index 5d53ed26..bba44247 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5815,7 +5815,6 @@ dependencies = [ "serde", "sha2 0.10.9", "toml", - "tracing", ] [[package]] diff --git a/crates/walgit-cli/src/config_cmd.rs b/crates/walgit-cli/src/config_cmd.rs index 7b74602e..5edf51ab 100644 --- a/crates/walgit-cli/src/config_cmd.rs +++ b/crates/walgit-cli/src/config_cmd.rs @@ -9,7 +9,7 @@ use walgit_config::Config; pub async fn run(action: ConfigAction, cfg: &Arc) -> Result<()> { match action { - ConfigAction::Check { env_files, strict } => { + ConfigAction::Check { env_files } => { let mut cfg: Config = (**cfg).clone(); let mut vars: Vec<(String, String)> = Vec::new(); for f in &env_files { @@ -25,22 +25,9 @@ pub async fn run(action: ConfigAction, cfg: &Arc) -> Result<()> { } } } - let ignored = cfg.apply_env_report(vars.into_iter())?; + cfg.apply_env(vars.into_iter())?; cfg.validate()?; - for (k, why) in &ignored { - eprintln!("ignored {k}: {why}"); - } - if ignored.is_empty() { - println!("config OK"); - } else { - println!( - "config OK ({} override(s) ignored — unknown in this build)", - ignored.len() - ); - if strict { - std::process::exit(3); - } - } + println!("config OK"); Ok(()) } ConfigAction::Dump => { diff --git a/crates/walgit-cli/src/lib.rs b/crates/walgit-cli/src/lib.rs index c7b49b35..6cfc2efd 100644 --- a/crates/walgit-cli/src/lib.rs +++ b/crates/walgit-cli/src/lib.rs @@ -376,15 +376,12 @@ enum ConfigAction { /// Parse and validate the config file; print OK or the error. Check { /// Also apply `WALGIT__*` overrides from these env files (KEY=VALUE - /// lines; `#` comments) on top of the process env, report every key - /// this build ignores, and exit 3 when `--strict` and any was ignored. + /// lines; `#` comments) on top of the process env; an override this + /// build cannot apply fails the check exactly as it fails startup. /// Useful in a process supervisor's pre-start check to validate an env /// file against the binary that will actually run. #[arg(long = "env-file")] env_files: Vec, - /// Exit 3 if any override was ignored (unknown in this build). - #[arg(long)] - strict: bool, }, /// Print the effective config as TOML. Dump, diff --git a/crates/walgit-cli/tests/config_env.rs b/crates/walgit-cli/tests/config_env.rs new file mode 100644 index 00000000..42299402 --- /dev/null +++ b/crates/walgit-cli/tests/config_env.rs @@ -0,0 +1,94 @@ +// Test fixtures use panics to fail the test. +#![allow(clippy::unwrap_used)] +//! `WALGIT__` overrides fail closed through the real binaries: a typo'd +//! override stops `config check` and server startup (before anything binds), +//! naming the variable; a valid one is applied. + +use std::process::{Command, Output}; + +const TYPO: (&str, &str) = ("WALGIT__SERVER__LSITEN", "127.0.0.1:0"); +const HINT: &str = "WALGIT__SERVER__LSITEN: unknown key `lsiten` in [server]; did you mean WALGIT__SERVER__LISTEN?"; + +/// A binary with `--config /dev/null` (defaults + env, D39) and only `env`: +/// nothing from the test runner's environment leaks in. +fn run(bin: &str, args: &[&str], env: &[(&str, &str)]) -> Output { + let mut cmd = Command::new(bin); + cmd.env_clear() + .env("PATH", std::env::var_os("PATH").unwrap_or_default()) + .args(["--config", "/dev/null"]) + .args(args); + for (k, v) in env { + cmd.env(k, v); + } + cmd.output().unwrap() +} + +fn stderr(o: &Output) -> String { + String::from_utf8_lossy(&o.stderr).into_owned() +} + +#[test] +fn server_startup_refuses_a_typod_override() { + // Spawned, not `output()`: a server that ignored the typo would serve forever. + let mut child = Command::new(env!("CARGO_BIN_EXE_walgit-server")) + .env_clear() + .env("PATH", std::env::var_os("PATH").unwrap_or_default()) + .env(TYPO.0, TYPO.1) + .args(["--config", "/dev/null"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::piped()) + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_mins(1); + while child.try_wait().unwrap().is_none() { + if std::time::Instant::now() > deadline { + child.kill().unwrap(); + panic!("walgit-server started with {} set", TYPO.0); + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } + let out = child.wait_with_output().unwrap(); + assert_eq!(out.status.code(), Some(1), "{}", stderr(&out)); + assert!(stderr(&out).contains(HINT), "{}", stderr(&out)); +} + +#[test] +fn config_check_refuses_a_typod_override_in_the_env_and_in_an_env_file() { + let walgit = env!("CARGO_BIN_EXE_walgit"); + let out = run(walgit, &["config", "check"], &[TYPO]); + assert!(!out.status.success(), "process env typo accepted"); + assert!(stderr(&out).contains(HINT), "{}", stderr(&out)); + + let dir = tempfile::tempdir().unwrap(); + let env_file = dir.path().join("walgit.env"); + std::fs::write( + &env_file, + format!( + "# host overrides\nWALGIT__WAL__MAX_BATCH=7\n{}={}\n", + TYPO.0, TYPO.1 + ), + ) + .unwrap(); + let path = env_file.to_str().unwrap(); + let out = run(walgit, &["config", "check", "--env-file", path], &[]); + assert!(!out.status.success(), "env-file typo accepted"); + assert!(stderr(&out).contains(HINT), "{}", stderr(&out)); + assert!(!String::from_utf8_lossy(&out.stdout).contains("config OK")); + + std::fs::write(&env_file, "WALGIT__WAL__MAX_BATCH=7\n").unwrap(); + let out = run(walgit, &["config", "check", "--env-file", path], &[]); + assert!(out.status.success(), "{}", stderr(&out)); + assert!(String::from_utf8_lossy(&out.stdout).contains("config OK")); +} + +#[test] +fn a_valid_override_is_applied() { + let out = run( + env!("CARGO_BIN_EXE_walgit"), + &["config", "dump"], + &[("WALGIT__WAL__MAX_BATCH", "7")], + ); + assert!(out.status.success(), "{}", stderr(&out)); + let dump = String::from_utf8_lossy(&out.stdout); + assert!(dump.contains("max_batch = 7"), "{dump}"); +} diff --git a/crates/walgit-config/Cargo.toml b/crates/walgit-config/Cargo.toml index 9c10f6fe..23c2f0c7 100644 --- a/crates/walgit-config/Cargo.toml +++ b/crates/walgit-config/Cargo.toml @@ -7,7 +7,6 @@ license.workspace = true [dependencies] sha2.workspace = true hex.workspace = true -tracing.workspace = true anyhow.workspace = true serde.workspace = true toml.workspace = true diff --git a/crates/walgit-config/src/lib.rs b/crates/walgit-config/src/lib.rs index e3cdaf15..881680cd 100644 --- a/crates/walgit-config/src/lib.rs +++ b/crates/walgit-config/src/lib.rs @@ -763,6 +763,74 @@ impl Config { } } +/// Why an override at `path` does not deserialize: the error's first line, or for an unknown +/// key the section it is not in and the nearest key serde offered (`expected one of `a`, …`), +/// spelled as the variable to set instead. +fn env_override_error(path: &[String], err: &str) -> String { + let first = err + .lines() + .map(str::trim) + .find(|l| !l.is_empty()) + .unwrap_or("invalid") + .to_string(); + // `unknown field `x`, expected one of `a`, `b`` → x, then the candidates. + let Some((_, tail)) = err.split_once("unknown field `") else { + return first; + }; + let mut ticks = tail + .lines() + .next() + .unwrap_or_default() + .split('`') + .step_by(2); + let Some(field) = ticks.next() else { + return first; + }; + let Some(depth) = path.iter().position(|p| p == field) else { + return first; + }; + let section = match path.get(..depth) { + Some([]) | None => "the top level".to_string(), + Some(s) => format!("[{}]", s.join(".")), + }; + let nearest = ticks + .filter(|c| !c.is_empty()) + .map(|c| (edit_distance(field, c), c)) + .min() + .filter(|(d, _)| *d <= 2.max(field.len() / 3)); + match nearest { + Some((_, key)) => { + let fixed: Vec<&str> = path + .iter() + .take(depth) + .map(String::as_str) + .chain([key]) + .collect(); + format!( + "unknown key `{field}` in {section}; did you mean WALGIT__{}?", + fixed.join("__").to_ascii_uppercase() + ) + } + None => format!("unknown key `{field}` in {section}"), + } +} + +/// Levenshtein distance; enough to turn a typo'd override into a suggestion. +fn edit_distance(a: &str, b: &str) -> usize { + let mut prev: Vec = (0..=b.chars().count()).collect(); + for (i, ca) in a.chars().enumerate() { + let mut cur = vec![i + 1]; + for (cb, pair) in b.chars().zip(prev.windows(2)) { + let left = cur.last().copied().unwrap_or_default(); + if let [diag, up] = pair { + cur.push((diag + usize::from(ca != cb)).min(up + 1).min(left + 1)); + } + } + prev = cur; + } + prev.last().copied().unwrap_or_default() +} + /// `owner/name` matches an entry of `list` (`owner/name`, `owner/*`, `*`; `.git` tolerated). /// The `placement` table as the env overrides alone set it (None when no /// `WALGIT__PLACEMENT__*` variable was present). @@ -965,29 +1033,17 @@ impl Config { /// Apply `WALGIT__a__b=v` overrides (values parsed as TOML values, falling back to string) /// and a serverless host's `PORT`. /// - /// Config and image are released independently (the ssd-host host follows - /// the serving image's version): an override for a key **unknown to this build** - /// (or with an unparsable value) is **ignored with a WARN**, never a - /// startup failure (2026-08-21: `unknown field disk_high_watermark` - /// crash-looped a host running the previous image). The ignored keys are - /// returned by [`apply_env_report`]. + /// Fails closed, like an unknown key in the file (`deny_unknown_fields`): an override this + /// build cannot apply — a malformed name, an unknown section or key (a typo, or a key from + /// a newer build), a value of the wrong type — is an error naming every such variable (and + /// the nearest known key), and nothing is applied. A typo'd `WALGIT__SERVER__LSITEN` used + /// to be a WARN while the host ran on the default. Validate an env file against the binary + /// that will run before rolling either out: `walgit config check --env-file`. pub fn apply_env(&mut self, vars: impl Iterator) -> Result<()> { - let ignored = self.apply_env_report(vars)?; - for (k, why) in &ignored { - tracing::warn!(key = %k, reason = %why, "ignoring {k}: unknown in this build"); - } - Ok(()) - } - - /// [`apply_env`] returning the `(key, reason)` pairs it had to ignore. - pub fn apply_env_report( - &mut self, - vars: impl Iterator, - ) -> Result> { let mut vars_seen: Vec = Vec::new(); let mut doc: toml::Table = toml::Table::try_from(&*self).context("serializing config")?; let mut touched = false; - let mut ignored = Vec::new(); + let mut refused = Vec::new(); let mut port_override = None; for (k, v) in vars { if k == "PORT" { @@ -999,14 +1055,17 @@ impl Config { }; vars_seen.push(k.clone()); let path: Vec = rest.split("__").map(str::to_ascii_lowercase).collect(); - if path.is_empty() || path.iter().any(std::string::String::is_empty) { + if path.iter().any(std::string::String::is_empty) { + refused.push(format!( + "{k}: not a configuration path (WALGIT__SECTION__KEY)" + )); continue; } let value: toml::Value = v .parse::() .unwrap_or(toml::Value::String(v.clone())); - // Apply into a copy and type-check it alone: a bad/unknown key is - // dropped (WARN) instead of failing every other override with it. + // Apply into a copy and type-check it alone, so every unusable + // override is named at once, each with its own reason. let mut trial = doc.clone(); let bad = { fn set( @@ -1030,22 +1089,25 @@ impl Config { } match set(&mut trial, &path, value) { Err(why) => Some(why), - Ok(()) => trial.clone().try_into::().err().map(|e| { - e.to_string() - .lines() - .next() - .unwrap_or("invalid") - .to_string() - }), + Ok(()) => trial + .clone() + .try_into::() + .err() + .map(|e| env_override_error(&path, &e.to_string())), } }; if let Some(why) = bad { - ignored.push((k, why)); + refused.push(format!("{k}: {why}")); } else { doc = trial; touched = true; } } + anyhow::ensure!( + refused.is_empty(), + "WALGIT__ environment override(s) this build cannot apply:\n {}", + refused.join("\n ") + ); // `[placement]` is a host fact set as a GROUP: any WALGIT__PLACEMENT__* override // replaces the whole section (unset keys = the section's defaults), never // merges with the baked file's. 2026-08-21 07:00Z: the image's toml carried @@ -1073,7 +1135,7 @@ impl Config { *u = rewrite_origin_port(u, port); } } - Ok(ignored) + Ok(()) } pub fn validate(&self) -> Result<()> { @@ -1468,50 +1530,65 @@ mod tests { c.validate().unwrap(); } - /// Config and image release independently: an override for a key this - /// build does not know (or a value it cannot parse) is ignored and - /// reported, the known ones still apply, startup continues. + /// An override this build cannot apply fails closed, like an unknown key in the + /// file: every such variable is named (with the nearest key for a typo), and none + /// of the batch is applied — not even the valid ones. #[test] - fn env_override_unknown_key_is_ignored_not_fatal() { + fn env_override_this_build_cannot_apply_is_an_error() { let mut c = Config::default(); - let ignored = c - .apply_env_report( + let err = c + .apply_env( vec![ + ("WALGIT__WAL__BATCH_WINDOW".to_string(), "30ms".to_string()), ( - "WALGIT__CACHE__NOT_A_KEY_YET".to_string(), - "0.9".to_string(), + "WALGIT__SERVER__LSITEN".to_string(), + "0.0.0.0:9".to_string(), ), ( "WALGIT__WAL__MAX_BATCH".to_string(), "not-a-number".to_string(), ), ("WALGIT__NOSUCHSECTION__X".to_string(), "1".to_string()), - ("WALGIT__WAL__BATCH_WINDOW".to_string(), "30ms".to_string()), + ("WALGIT__CACHE____DIR".to_string(), "/x".to_string()), ] .into_iter(), ) - .unwrap(); + .unwrap_err() + .to_string(); + assert!( + err.contains( + "WALGIT__SERVER__LSITEN: unknown key `lsiten` in [server]; did you mean WALGIT__SERVER__LISTEN?" + ), + "{err}" + ); + assert!(err.contains("WALGIT__WAL__MAX_BATCH: "), "{err}"); + assert!( + err.contains("WALGIT__NOSUCHSECTION__X: unknown key `nosuchsection` in the top level"), + "{err}" + ); + assert!(!err.contains("did you mean WALGIT__NOSUCHSECTION"), "{err}"); + assert!( + err.contains("WALGIT__CACHE____DIR: not a configuration path"), + "{err}" + ); + assert!( + !err.contains("BATCH_WINDOW"), + "a valid override is not refused: {err}" + ); assert_eq!( c.wal.batch_window, - Duration::from_millis(30), - "known override still applied" + Config::default().wal.batch_window, + "nothing applied from a refused batch" ); - let keys: Vec<&str> = ignored.iter().map(|(k, _)| k.as_str()).collect(); - assert_eq!( - keys, - [ - "WALGIT__CACHE__NOT_A_KEY_YET", - "WALGIT__WAL__MAX_BATCH", - "WALGIT__NOSUCHSECTION__X" - ] + assert_eq!(c.server.listen, Config::default().server.listen); + // `Config::parse` (startup) reads the process env through the same path. + let mut c = Config::default(); + assert!( + c.apply_env(vec![("WALGIT__GIT__BINARI".to_string(), "g".to_string())].into_iter()) + .unwrap_err() + .to_string() + .contains("did you mean WALGIT__GIT__BINARY?") ); - assert!(ignored[0].1.contains("unknown field"), "{:?}", ignored[0]); - // Plain apply_env is the same, just warns. - let mut c2 = Config::default(); - c2.apply_env( - vec![("WALGIT__CACHE__NOT_A_KEY_YET".to_string(), "1".to_string())].into_iter(), - ) - .unwrap(); } #[test] diff --git a/docs/PACKFILE_MIGRATION.md b/docs/PACKFILE_MIGRATION.md index 65640a0f..3567bc11 100644 --- a/docs/PACKFILE_MIGRATION.md +++ b/docs/PACKFILE_MIGRATION.md @@ -24,9 +24,9 @@ this plan is not a test receipt. ## Host configuration Remove `[bundles]` and every `[[bundles.strategy]]` table, `server.roles` entries named `bundle`, and -`cache.bundle_list_entries`. The new parser rejects these removed host shapes. Environment override loaders -that warn and ignore unknown keys are not migration tools: remove corresponding `WALGIT__BUNDLES__*` and -cache overrides from the host's deployment configuration as well. +`cache.bundle_list_entries`. The new parser rejects these removed host shapes, and so does the environment +override loader: remove corresponding `WALGIT__BUNDLES__*` and cache overrides from the host's deployment +configuration as well. The lifecycle layer replaces `[compaction]` with `[packs]`; old host keys and newly submitted old settings are rejected. The ordinary `compact` role remains. LFS `serve_via = "proxy"` or `"signed_url"` is unchanged. diff --git a/walgit.example.toml b/walgit.example.toml index 79584aa9..cddb8c30 100644 --- a/walgit.example.toml +++ b/walgit.example.toml @@ -2,7 +2,8 @@ # # Start from walgit.standalone.toml for a first run; come here when you need a key. # Every key can also be set from the environment: WALGIT__SECTION__KEY=value (TOML value syntax). -# Validate with: walgit --config walgit.toml config check +# An override this build cannot apply (unknown key, wrong type) is a startup error, like an unknown key here. +# Validate with: walgit --config walgit.toml config check [--env-file host.env] [server] listen = "127.0.0.1:8080" # default; `mode = none` is refused unless this is loopback. Public bind: 0.0.0.0 with token/oidc. From ee5f5065e08989100f00a184cdb88c86b45e1f15 Mon Sep 17 00:00:00 2001 From: Jonas Grosch Date: Thu, 1 Oct 2026 02:09:50 +0200 Subject: [PATCH 2/2] docs(config): example [store.s3] shows the code defaults walgit.example.toml documents "every key with its default", but its [store.s3] block showed AWS values: `force_path_style = false` and an AWS endpoint, while `S3Config::default()` is `force_path_style = true` with `endpoint = "http://127.0.0.1:9000"`. The code is right: the flag goes straight to aws-sdk-s3, and the default endpoint is the local rustfs/MinIO rig, which needs path-style addressing (s3.rs notes it as required for rustfs; virtual-hosted would address `bucket.host`). So the example follows the code, and says when to set false (AWS, virtual-hosted addressing). `example_configurations_parse_ and_validate` now pins the example's [store.s3] to the default. Co-Authored-By: Claude Opus 5.5 --- crates/walgit-config/src/lib.rs | 7 ++++++- walgit.example.toml | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/walgit-config/src/lib.rs b/crates/walgit-config/src/lib.rs index 881680cd..706b2cf6 100644 --- a/crates/walgit-config/src/lib.rs +++ b/crates/walgit-config/src/lib.rs @@ -1418,8 +1418,13 @@ mod tests { #[test] fn example_configurations_parse_and_validate() { - Config::parse(include_str!("../../../walgit.example.toml")).unwrap(); + let example = Config::parse(include_str!("../../../walgit.example.toml")).unwrap(); Config::parse(include_str!("../../../walgit.standalone.toml")).unwrap(); + // The example documents defaults; `[store.s3]` once showed AWS values instead. + assert_eq!( + toml::to_string(&example.store.s3).unwrap(), + toml::to_string(&S3Config::default()).unwrap() + ); } #[test] diff --git a/walgit.example.toml b/walgit.example.toml index cddb8c30..2ecb2a11 100644 --- a/walgit.example.toml +++ b/walgit.example.toml @@ -73,11 +73,12 @@ multipart_threshold = "64MiB" # PUTs above this use multipart/resumable uplo multipart_part_size = "32MiB" [store.s3] -endpoint = "https://s3.us-east-1.amazonaws.com" # or http://127.0.0.1:9000 for rustfs/MinIO +endpoint = "http://127.0.0.1:9000" # the local rustfs/MinIO rig; AWS: "https://s3..amazonaws.com" region = "us-east-1" access_key_env = "AWS_ACCESS_KEY_ID" # env var names read at runtime; AWS_SESSION_TOKEN is also honored when set secret_key_env = "AWS_SECRET_ACCESS_KEY" -force_path_style = false # true for most self-hosted S3 implementations +force_path_style = true # path-style `endpoint/bucket/key`, what rustfs/MinIO/Ceph need; on AWS set false + # for virtual-hosted `bucket.endpoint/key`, AWS's addressing [store.gcs] endpoint = "https://storage.googleapis.com" # gRPC endpoint; credentials = Application Default Credentials