From f41363e9230606a83e52b5b500b4e3bfd8cf95f5 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Sat, 8 Aug 2026 16:00:57 -0700 Subject: [PATCH 1/2] ci: make public-hygiene-lint green The public-hygiene gate flags text in customer-readable files that describes how the software was BUILT rather than what it DOES. Every hit in this repo is rewritten to state the behaviour or the invariant; no allow markers are added, so the gate keeps its teeth. --- .github/scripts/next-version.sh | 2 +- .github/workflows/release-selftest.yml | 2 +- store-postgres-plugin/tests/admin_api_e2e.rs | 5 +- store-postgres-plugin/tests/e2e.rs | 5 +- store-postgres/src/lib.rs | 2 +- store-postgres/src/tests.rs | 51 +++++++++----------- 6 files changed, 30 insertions(+), 37 deletions(-) diff --git a/.github/scripts/next-version.sh b/.github/scripts/next-version.sh index 27fce0e..a0c3758 100755 --- a/.github/scripts/next-version.sh +++ b/.github/scripts/next-version.sh @@ -2,7 +2,7 @@ # next-version.sh — compute THIS repo's next release tag for release-on-upstream.yml. # # Single source of truth for the version math, exercised in CI by release-selftest.yml so the -# release automation can't silently rot (guard #135.8). Prints "v.." to stdout. +# release automation can't silently rot. Prints "v.." to stdout. # # Inputs (env, all optional): # INPUT_VERSION explicit version to cut (leading "v" tolerated) -> used verbatim. diff --git a/.github/workflows/release-selftest.yml b/.github/workflows/release-selftest.yml index a6aafb6..9cfa502 100644 --- a/.github/workflows/release-selftest.yml +++ b/.github/workflows/release-selftest.yml @@ -1,4 +1,4 @@ -# CI self-test for the release-on-upstream version-compute logic (guard #135.8). +# CI self-test for the release-on-upstream version-compute logic. # Runs the REAL .github/scripts/next-version.sh against synthetic repos and asserts it produces a # valid next version for BOTH the has-prior-tag and no-prior-tag cases — WITHOUT publishing anything. # This is what keeps the release automation from silently rotting before the fleet fan-out is armed. diff --git a/store-postgres-plugin/tests/admin_api_e2e.rs b/store-postgres-plugin/tests/admin_api_e2e.rs index b70ea68..3b0f9a6 100644 --- a/store-postgres-plugin/tests/admin_api_e2e.rs +++ b/store-postgres-plugin/tests/admin_api_e2e.rs @@ -80,9 +80,8 @@ fn postgres_url() -> Option { /// Checks BOTH the "uplifted" `/` copy (only refreshed when `[lib]` is a ROOT /// build target of the invocation, e.g. `cargo build --all-targets`) and the raw /// `/deps/` compiler output (refreshed on every build that recompiles the lib, -/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs, and what -/// cargo-mutants runs) does NOT uplift the cdylib to the top-level profile dir, only to -/// `target/deps` — checking only `profile_dir` silently finds nothing and this test's CI hard-panic +/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs) does +/// NOT uplift the cdylib to the top-level profile dir, only to `target/deps` — checking only `profile_dir` silently finds nothing and this test's CI hard-panic /// fires even though the cdylib really was built (confirmed against `release-check.sh`'s CI run: /// "the store-postgres-plugin cdylib is not built under CI" despite the prior `cargo test /// --workspace --release` step compiling it). Same fix already applied to auth-oidc-plugin's and diff --git a/store-postgres-plugin/tests/e2e.rs b/store-postgres-plugin/tests/e2e.rs index 6c75983..3936014 100644 --- a/store-postgres-plugin/tests/e2e.rs +++ b/store-postgres-plugin/tests/e2e.rs @@ -74,9 +74,8 @@ fn postgres_url() -> Option { /// Checks BOTH the "uplifted" `/` copy (only refreshed when `[lib]` is a ROOT /// build target of the invocation, e.g. `cargo build --all-targets`) and the raw /// `/deps/` compiler output (refreshed on every build that recompiles the lib, -/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs, and what -/// cargo-mutants runs) does NOT uplift the cdylib to the top-level profile dir, only to -/// `target/deps` — checking only `profile_dir` silently finds nothing. Same fix already applied to +/// uplifted or not). A bare `cargo test --release` (what `release-check.sh`'s Phase 2 runs) does +/// NOT uplift the cdylib to the top-level profile dir, only to `target/deps` — checking only `profile_dir` silently finds nothing. Same fix already applied to /// this crate's sibling `admin_api_e2e.rs` and to auth-oidc-plugin's/webrequest-hook's equivalent /// helpers. fn plugin_path() -> Option { diff --git a/store-postgres/src/lib.rs b/store-postgres/src/lib.rs index 0584f4f..1b80267 100644 --- a/store-postgres/src/lib.rs +++ b/store-postgres/src/lib.rs @@ -61,7 +61,7 @@ fn is_undefined_table(e: &postgres::Error) -> bool { e.code() == Some(&postgres::error::SqlState::UNDEFINED_TABLE) } -/// Extract the PASSWORD from a Postgres DSN (L2). Supports both the URL form +/// Extract the PASSWORD from a Postgres DSN. Supports both the URL form /// (`postgres://user:pass@host:5432/db`) and the libpq keyword form (`... password=secret ...`), so /// a connect-error string can be scrubbed of the secret regardless of which shape the operator used. fn dsn_password(dsn: &str) -> Option { diff --git a/store-postgres/src/tests.rs b/store-postgres/src/tests.rs index 4114ba2..36deec2 100644 --- a/store-postgres/src/tests.rs +++ b/store-postgres/src/tests.rs @@ -115,8 +115,8 @@ fn connect_store_with_retry(url: &str) -> StoreResult { /// TRUE reset for test isolation -- unlike `delete_key` (a deliberate tombstone that can never fully /// reset a row by design), this raw-SQL wipe gives each test a genuinely clean slate for its id, so -/// re-running the suite (or running it twice, as red-before-green proofs do) never sees stale state -/// from a prior run leaking into the CHECK constraints (e.g. re-minting into a row still marked +/// re-running the suite (including running it twice in a row) never sees stale state from a prior +/// run leaking into the CHECK constraints (e.g. re-minting into a row still marked /// `deleted_at` from a previous run's tombstone would violate `keys_tombstone_disabled`). fn hard_reset(store: &PostgresStore, id: &str) { let mut client = store.lock(); @@ -214,10 +214,9 @@ fn delete_key_is_a_tombstone_not_a_hard_delete() { store.delete_key(id).unwrap(); - // RED-BEFORE-GREEN evidence lives in the assertions below: this test only passes if delete_key - // genuinely tombstones rather than hard-deletes. Confirmed by temporarily reverting delete_key - // to `DELETE FROM keys WHERE id=$1` during development: get_key(id) then returned None and this - // test failed at the very first assertion, exactly as expected. + // The assertions below are non-vacuous: they pass only if delete_key genuinely tombstones + // rather than hard-deletes. Were delete_key a plain `DELETE FROM keys WHERE id=$1`, get_key(id) + // would return None and the very first assertion below would fail. let after = store.get_key(id).unwrap(); assert!( after.is_some(), @@ -248,11 +247,10 @@ fn delete_key_is_a_tombstone_not_a_hard_delete() { /// that was previously deleted (deleted_at set, enabled=false by delete_key) must produce a fully /// LIVE key, not one that is enabled=true while still marked deleted_at. /// -/// RED-BEFORE-GREEN: reverting the ON CONFLICT UPDATE SET clause to omit `deleted_at=NULL` (its -/// state before this fix) makes this test fail at the `deleted_at` assertion below: `enabled` flips -/// to `true` as the caller intended, but `deleted_at` is left at whatever the tombstone set it to, -/// so the row is simultaneously "enabled" and "deleted" -- exactly the corrupt state this test -/// guards against. Confirmed by temporarily reverting the fix and re-running: this assertion failed. +/// The `deleted_at` assertion below is the load-bearing one: an ON CONFLICT UPDATE SET clause that +/// omits `deleted_at=NULL` flips `enabled` to `true` as the caller intended but leaves `deleted_at` +/// at whatever the tombstone set it to, so the row is simultaneously "enabled" and "deleted" -- +/// exactly the corrupt state this test rejects. #[test] fn put_key_with_credential_on_conflict_clears_a_stale_tombstone() { let Some(url) = live_url() else { return }; @@ -366,11 +364,10 @@ fn credential_slot_guard_rejects_clobbering_a_live_credential() { /// put_credential_tx must bind CredentialMeta::updated_at to its own column, not silently reuse /// created_at's parameter for both. /// -/// RED-BEFORE-GREEN: reverting the fix (VALUES ...,$8,$8,$9,... binding created_at's placeholder -/// twice, with `updated_at` never bound at all) makes this test fail: the round-tripped -/// `updated_at` comes back equal to `created_at` (100) instead of the distinct value (200) this -/// test mints with. Confirmed by temporarily reverting and re-running: assertion failed with -/// `left: 100, right: 200`. +/// The test is non-vacuous: were the INSERT to bind created_at's placeholder twice +/// (VALUES ...,$8,$8,$9,...) and never bind `updated_at` at all, the round-tripped `updated_at` +/// would come back equal to `created_at` (100) instead of the distinct value (200) this test mints +/// with, and the assertion below would fail with `left: 100, right: 200`. #[test] fn put_credential_binds_updated_at_to_its_own_column_not_created_at() { let Some(url) = live_url() else { return }; @@ -574,11 +571,10 @@ fn get_usage_transaction_is_actually_repeatable_read() { /// between the two steps on a SEPARATE connection, then asserts REPEATABLE READ's snapshot held: the /// second read still sees the pre-interleave state, not the concurrent writer's new model row. /// -/// RED-BEFORE-GREEN: this test is a genuine regression guard rather than a fresh finding (get_usage -/// already opens REPEATABLE READ via snapshot_consistent_tx) -- confirmed non-vacuous by temporarily -/// downgrading snapshot_consistent_tx's isolation level to READ COMMITTED and re-running: the -/// `model_count` assertion below failed (it observed the interleaved writer's new model row), then -/// passed again after restoring REPEATABLE READ. +/// This is a regression guard on an isolation level get_usage already opens (REPEATABLE READ, via +/// snapshot_consistent_tx). It is non-vacuous: at READ COMMITTED the `model_count` assertion below +/// observes the interleaved writer's new model row and fails; only REPEATABLE READ holds the +/// snapshot across both reads. #[test] fn get_usage_snapshot_does_not_observe_a_concurrent_add_usage_between_its_two_reads() { let Some(url) = live_url() else { return }; @@ -719,9 +715,9 @@ fn metering_roundtrip_new_fields() { } // --------------------------------------------------------------------------------------------- -// Mutation-testing gap fixes (cargo-mutants round 1 against this file): each test below is named -// for, and directly targets, one or more MISSED mutants -- confirmed red against the mutant before -// being folded in green here. +// Boundary and arithmetic guards for this file: each test below is named for, and directly +// targets, one specific predicate or binding whose behaviour was not otherwise pinned. Each fails +// if that predicate is perturbed in the way its name describes. // --------------------------------------------------------------------------------------------- /// `percent_decode`'s length guard and hi/lo-nibble arithmetic, pinned with cases the existing @@ -1265,10 +1261,9 @@ fn migrate_v6_does_not_rerun_the_backfill_on_an_already_migrated_database() { /// unchanged in both cases -- `migrate_locked` unconditionally runs `CREATE TABLE IF NOT EXISTS /// busbar_schema` as its very first statement, so by the time the guarded `SELECT` runs, the table /// either already exists (guard never fires) or the preceding `CREATE TABLE` itself already -/// propagated the error several lines earlier (guard never reached). This is a confirmed equivalent -/// mutant / dead branch given the current code structure, not a test-coverage gap; left unfixed -/// per policy (no test written to "kill" it, since none can, without changing the source itself -- -/// out of scope for a mutation-testing coverage pass). +/// propagated the error several lines earlier (guard never reached). Given the current statement +/// order the guarded branch is therefore unreachable, so no test can drive it without changing the +/// source itself; the test below pins the reachable half of the behaviour instead. #[test] fn migrate_propagates_a_non_undefined_table_error_and_never_silently_succeeds() { let Some(url) = live_url() else { return }; From 229e07f9cf7b436cef978e01f74ad10a2c71a598 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Sat, 8 Aug 2026 19:27:36 -0700 Subject: [PATCH 2/2] tests: supply every secret the e2e fixtures reference busbar resolves `env:` secret references during `--validate` now and exits non-zero when one cannot resolve, so the file-drop fixture -- which names `providers.mock.api_key: { env: MOCK_KEY }` and never exported MOCK_KEY -- described a gateway that could not start: busbar --validate must succeed with the file-dropped postgres plugin: stdout= stderr=[error] providers.mock.api_key: secret env:MOCK_KEY cannot resolve: environment variable 'MOCK_KEY' is unset CI had not surfaced this one yet: the admin-API test failed first for a different reason and aborted the run before this binary was reached. The variable names are derived FROM the config files busbar is about to read, rather than listed at each call site, so the next person to add a secret reference to one of these fixtures does not also have to remember to export it -- forgetting produces a failure that never names the variable. Reading the files back (not the strings the test built them from) is what keeps the two from drifting apart. `tests/common/mod.rs` holds the scan; `tests/config_secrets.rs` covers it directly. Red-before-green against a real `busbar` built from the core branch CI tests against, and a real postgres: before, `load_and_exercise_postgres_plugin_via_file_drop` FAILED with the error quoted above; after, every test in the crate passes. --- store-postgres-plugin/tests/admin_api_e2e.rs | 10 ++- store-postgres-plugin/tests/common/mod.rs | 74 +++++++++++++++++++ store-postgres-plugin/tests/config_secrets.rs | 45 +++++++++++ store-postgres-plugin/tests/e2e.rs | 10 ++- 4 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 store-postgres-plugin/tests/common/mod.rs create mode 100644 store-postgres-plugin/tests/config_secrets.rs diff --git a/store-postgres-plugin/tests/admin_api_e2e.rs b/store-postgres-plugin/tests/admin_api_e2e.rs index 71dcdf8..785d43c 100644 --- a/store-postgres-plugin/tests/admin_api_e2e.rs +++ b/store-postgres-plugin/tests/admin_api_e2e.rs @@ -40,6 +40,8 @@ //! Together: real admin-API install -> real (admin-API-driven) plugin load -> real admin-API write //! -> real, independently-verified Postgres persistence. +mod common; + use base64::Engine as _; use std::path::PathBuf; use std::process::{Child, Command, Stdio}; @@ -286,7 +288,9 @@ fn install_over_admin_api_then_mint_a_key_and_verify_postgres_directly() { let config1 = work.join("config1.yaml"); std::fs::write(&config1, &providers_and_common).unwrap(); - let child1 = Command::new(&busbar_bin) + let mut boot1 = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut boot1, &[&config1, &providers]); + let child1 = boot1 .env("BUSBAR_CONFIG", &config1) .env("BUSBAR_PROVIDERS", &providers) .env("BUSBAR_ADMIN_TOKEN", admin_token) @@ -380,7 +384,9 @@ fn install_over_admin_api_then_mint_a_key_and_verify_postgres_directly() { ) .unwrap(); - let child2 = Command::new(&busbar_bin) + let mut boot2 = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut boot2, &[&config2, &providers]); + let child2 = boot2 .env("BUSBAR_CONFIG", &config2) .env("BUSBAR_PROVIDERS", &providers) .env("BUSBAR_ADMIN_TOKEN", admin_token) diff --git a/store-postgres-plugin/tests/common/mod.rs b/store-postgres-plugin/tests/common/mod.rs new file mode 100644 index 0000000..4691343 --- /dev/null +++ b/store-postgres-plugin/tests/common/mod.rs @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright (C) 2026 Busbar Inc and contributors + +//! Support code shared by this crate's integration tests: supplying the secrets a config fixture +//! references so the real `busbar` binary can start. +//! +//! busbar resolves every `env:`/`file:` secret reference during `--validate` AND at boot, and exits +//! non-zero when one of them cannot resolve. A fixture that names a variable nobody exported is +//! therefore a gateway that cannot come up, and the test reports that as an opaque timeout or +//! early-exit a long way from the YAML that caused it. +//! +//! The variable names are derived FROM the fixture text rather than listed here on purpose: the +//! next person to add a secret reference to a config in these tests must not have to also remember +//! to export it, because forgetting produces a failure that does not name the variable. +//! +//! This lives under `tests/common/` rather than `tests/` so cargo does not compile it as a test +//! binary of its own; its own coverage lives in `tests/config_secrets.rs`. + +#![allow(dead_code)] +// Each integration test binary compiles this module separately and uses a different subset of it, +// so anything only one of them needs is genuinely dead code from the other's point of view. + +use std::path::Path; +use std::process::Command; + +/// Stands in for every secret a fixture references but the test does not otherwise set. The content +/// is irrelevant — what matters is only that resolution SUCCEEDS. +pub const SECRET_PLACEHOLDER: &str = "e2e-placeholder"; + +/// Every environment variable `config` names, whether through a `{ env: NAME }` secret reference or +/// through a legacy `*_env: NAME` field, in first-seen order and without duplicates. +pub fn referenced_env_vars(config: &str) -> Vec { + let mut names = Vec::new(); + for (idx, _) in config.match_indices("env:") { + let name: String = config[idx + "env:".len()..] + .trim_start_matches([' ', '\t']) + .chars() + .take_while(|c| c.is_ascii_alphanumeric() || *c == '_') + .collect(); + // A bare `env:` with nothing usable after it (a line break, or the key of a nested mapping) + // names no variable at all; skip it rather than exporting an empty-named one. + if !name.is_empty() && !names.contains(&name) { + names.push(name); + } + } + names +} + +/// Sets [`SECRET_PLACEHOLDER`] for every environment variable `config` references. +/// +/// Call this BEFORE any `.env()` carrying a value the test actually asserts on (an admin token, a +/// signing key): the later `.env()` wins, so the placeholder only ever fills in the secrets whose +/// value the test does not care about. +pub fn apply_placeholder_secrets(cmd: &mut Command, config: &str) { + for name in referenced_env_vars(config) { + cmd.env(name, SECRET_PLACEHOLDER); + } +} + +/// [`apply_placeholder_secrets`] over the files busbar will actually read. +/// +/// Reading the files back, rather than the strings the test built them from, is what makes this +/// impossible to get out of step: whatever landed on disk is exactly what busbar resolves. +pub fn apply_placeholder_secrets_from_files(cmd: &mut Command, paths: &[&Path]) { + for path in paths { + let text = std::fs::read_to_string(path).unwrap_or_else(|e| { + panic!( + "read config fixture {} for its secrets: {e}", + path.display() + ) + }); + apply_placeholder_secrets(cmd, &text); + } +} diff --git a/store-postgres-plugin/tests/config_secrets.rs b/store-postgres-plugin/tests/config_secrets.rs new file mode 100644 index 0000000..85a2075 --- /dev/null +++ b/store-postgres-plugin/tests/config_secrets.rs @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright (C) 2026 Busbar Inc and contributors + +//! Coverage for `tests/common`'s secret-reference scan. +//! +//! That scan is what keeps the real-binary e2e tests bootable, and when it misses a reference the +//! symptom surfaces a long way from here — as `busbar` exiting 1 before its listener comes up — +//! so it is worth testing directly rather than only through the e2e tests it supports. + +mod common; + +use common::referenced_env_vars; + +#[test] +fn finds_secret_references_and_legacy_env_fields() { + let config = "\ +providers:\n mock:\n api_key: { env: MOCK_KEY }\n\ +identity-providers:\n admin-tokens: { module: admin-tokens, token: { env: BUSBAR_ADMIN_TOKEN } }\n\ +auth:\n signing_key: { env: BUSBAR_SIGNING_KEY }\n"; + assert_eq!( + referenced_env_vars(config), + vec!["MOCK_KEY", "BUSBAR_ADMIN_TOKEN", "BUSBAR_SIGNING_KEY"], + ); + + // The flat providers catalog still names variables through the legacy `*_env:` field rather + // than a secret reference, and busbar resolves those too, so the same scan has to see them. + assert_eq!( + referenced_env_vars("mock:\n api_key_env: MOCK_KEY\n"), + vec!["MOCK_KEY"], + ); +} + +#[test] +fn deduplicates_and_ignores_references_that_name_nothing() { + assert_eq!( + referenced_env_vars("a: { env: SAME }\nb: { env: SAME }\n"), + vec!["SAME"], + "a variable referenced twice must be exported once, not twice" + ); + + // A bare `env:` opening a nested mapping, or ending a line, names no variable; exporting an + // empty-named one would be nonsense and on some platforms an outright error. + assert!(referenced_env_vars("a: { env:\n b: c }\n").is_empty()); + assert!(referenced_env_vars("listen: \"127.0.0.1:0\"\nauth:\n chain: []\n").is_empty()); +} diff --git a/store-postgres-plugin/tests/e2e.rs b/store-postgres-plugin/tests/e2e.rs index 3936014..de79ce8 100644 --- a/store-postgres-plugin/tests/e2e.rs +++ b/store-postgres-plugin/tests/e2e.rs @@ -38,6 +38,8 @@ //! install work," and converting them to a full process-boot-and-capture-stderr harness for each //! error shape is a much larger, lower-value lift than the persistence test's conversion. +mod common; + use busbar_store_postgres::PostgresStore; use std::path::PathBuf; use std::process::{Child, Command, Stdio}; @@ -244,7 +246,9 @@ fn load_and_exercise_postgres_plugin_via_file_drop() { // file-dropped plugin passes the trust/manifest gate; then a REAL BOOT (no `--validate` flag, // below) is the only thing that actually `dlopen`s the plugin and runs // `Store::connect`/migration. - let out = Command::new(&busbar_bin) + let mut validate = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut validate, &[&config, &providers]); + let out = validate .arg("--validate") .env("BUSBAR_CONFIG", &config) .env("BUSBAR_PROVIDERS", &providers) @@ -262,7 +266,9 @@ fn load_and_exercise_postgres_plugin_via_file_drop() { // PostgresStore::connect, so this check can't accidentally create the schema itself -- for the // `keys` table to appear. This is the only genuine proof that boot actually dlopened the plugin // and called Store::connect (which runs migrate()) before ever handling a request. - let child = Command::new(&busbar_bin) + let mut boot = Command::new(&busbar_bin); + common::apply_placeholder_secrets_from_files(&mut boot, &[&config, &providers]); + let child = boot .env("BUSBAR_CONFIG", &config) .env("BUSBAR_PROVIDERS", &providers) .env("BUSBAR_STATE_FILE", "") // disable the state-snapshot file; not under test here