diff --git a/.github/scripts/next-version.sh b/.github/scripts/next-version.sh index 27fce0e..710abf7 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 cannot 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/CONTRIBUTING.md b/CONTRIBUTING.md index 40c30ed..816cf6d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -51,7 +51,8 @@ JSON `open` config into a `PostgresStore` and hands the trait object to [`busbar-plugin-sdk`](https://github.com/GetBusbar/busbar/tree/main/crates/plugin-sdk), which emits the C ABI symbols the loader resolves. All the SQL and schema logic lives in the `busbar-store-postgres` library crate this plugin wraps, in the -`busbarAI` monorepo — most substantive changes belong there, not here. +`store-postgres/` directory of THIS repository, so most substantive changes +belong there. ## Commit & PR conventions diff --git a/store-postgres-plugin/tests/admin_api_e2e.rs b/store-postgres-plugin/tests/admin_api_e2e.rs index ad44053..75d7e4b 100644 --- a/store-postgres-plugin/tests/admin_api_e2e.rs +++ b/store-postgres-plugin/tests/admin_api_e2e.rs @@ -80,8 +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 +/// uplifted or not). A bare `cargo test --release` (as CI's release checks and any direct test +/// invocation run it) 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 diff --git a/store-postgres-plugin/tests/e2e.rs b/store-postgres-plugin/tests/e2e.rs index 6c75983..00e4d17 100644 --- a/store-postgres-plugin/tests/e2e.rs +++ b/store-postgres-plugin/tests/e2e.rs @@ -74,8 +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 +/// uplifted or not). A bare `cargo test --release` (as CI's release checks and any direct test +/// invocation run it) 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. 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..a65c935 100644 --- a/store-postgres/src/tests.rs +++ b/store-postgres/src/tests.rs @@ -115,7 +115,7 @@ 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 +/// re-running the suite (or running it twice back-to-back) 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) { @@ -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 only pass if delete_key genuinely tombstones rather than hard-deletes: + // with a hard `DELETE FROM keys WHERE id=$1`, get_key(id) returns None and the very first + // assertion fails. 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. +/// Regression guard: an ON CONFLICT UPDATE SET clause that omits `deleted_at=NULL` 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. #[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 +/// Regression guard: a VALUES clause of ...,$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`. +/// test mints with. #[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 non-vacuous regression guard for behaviour the code already has (get_usage opens +/// REPEATABLE READ via snapshot_consistent_tx): downgrading snapshot_consistent_tx's isolation +/// level to READ COMMITTED makes the `model_count` assertion below fail (it observes the +/// interleaved writer's new model row); REPEATABLE READ is what makes it hold. #[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. +// Operator- and boundary-level regression guards: each test below pins a specific comparison, +// arithmetic step or early-return in this file that the broader suite does not distinguish, so a +// single-operator change (`+` vs `*`, `<` vs `<=`, a dropped clause) fails a named test here. // --------------------------------------------------------------------------------------------- /// `percent_decode`'s length guard and hi/lo-nibble arithmetic, pinned with cases the existing @@ -1265,10 +1261,10 @@ 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). This is a confirmed dead +/// branch given the current code structure, not a test-coverage gap: no test can exercise it +/// without changing the source itself, so it is documented here rather than papered over with a +/// vacuous test. #[test] fn migrate_propagates_a_non_undefined_table_error_and_never_silently_succeeds() { let Some(url) = live_url() else { return };