From b9210617f58d28f0c635206cc35de43ba81a8147 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sun, 30 Aug 2026 01:10:38 +0200 Subject: [PATCH] tests: fix six review findings, four of which were guards not guarding A reviewer found six issues across the three PRs that merged today. All six are real. Four are the same shape as the defects those PRs were written to fix, which is the part worth reading. THE GATE'S SKIP GUARD WAS A MAGIC-STRING CHECK WEARING DERIVATION CLOTHES. I had replaced an --ignored-keyed guard with one that "asks the source" whether a target can emit a skip notice. It greps case-sensitively for the token SKIPPING. cli_admin.rs emitted its notice in lowercase, so the probe stayed silent for that target -- and run_expect's own downstream skip check greps case-sensitively too, so even with --nocapture that arm's skip was invisible to the guard. I swapped one magic string for another and described the result as derived. Fixed by standardizing the convention rather than widening the matcher: cli_admin.rs now emits the uppercase token, and its gate arm passes --nocapture so the newly-armed probe has something to see. Widening the matcher instead would have false-fired on that arm -- its skipping test is filtered out by the test-name selector, which the probe cannot observe -- and an over-firing guard is not the safe direction, because the next person hits a red on an arm they know is fine and deletes the guard rather than narrowing it. THE ARGUMENT SCAN RAN PAST THE SEPARATOR, so a --test after -- overwrote the target, last-one-wins, and could point the probe at a file with no skip path. Target capture now stops at --; --nocapture detection deliberately does not, because the real_daemon arm places it after the separator. THE FRAME PIN DID NOT EXERCISE THE ROUTE. It hand-built {"result": ...} and compared against itself, so renaming the wrapper in handle_read_request broke every consumer while the test stayed green. That is the defect the pin exists to catch, one level in: it pinned my belief about my own route rather than the route's output. Both now go through wrap_result, and mutating the wrapper inside the production helper turns the test red. THE NON-ACTIVE TEST BUILT ITS STATE OFF-PATH, reaching needs_reauth through the unversioned store.invalidate rather than the version-fenced call the engine makes when a refresh fails. Same state today, so it passed; if the engine's path ever also cleared stale_pending, the test would have stayed green while production diverged. The two NoHttp stubs are now one #[cfg(test)] helper. Verified it stays out of the shipped artifact: zero occurrences in the release binary against a control string that scores three. Master's 01f76b1 and 004fc06 were checked for interaction and have none -- the first adds scripts only, the second touches the manifest path. Gate: exit 0, nine real-daemon e2e arms executing. Mutation-checked: wrapper rename red, version fence red, NoHttp symbol red, guard red without --nocapture and green with it, post-separator override red, non-skipping target green. --- .../credentials-module/src/admin_surface.rs | 52 +-------- crates/credentials-module/src/main.rs | 100 +++++------------- crates/credentials-module/src/read_surface.rs | 9 +- crates/credentials-module/src/test_support.rs | 34 ++++++ crates/credentials-module/tests/cli_admin.rs | 2 +- scripts/gate.sh | 24 +++-- 6 files changed, 85 insertions(+), 136 deletions(-) create mode 100644 crates/credentials-module/src/test_support.rs diff --git a/crates/credentials-module/src/admin_surface.rs b/crates/credentials-module/src/admin_surface.rs index 37e41b1..ab61705 100644 --- a/crates/credentials-module/src/admin_surface.rs +++ b/crates/credentials-module/src/admin_surface.rs @@ -361,56 +361,6 @@ mod tests { use credentials_core::store::{mint_handle, EncryptedStore}; use credentials_core::vault_id_for; - /// A transport that cannot reach the network, used everywhere in this module. - /// - /// These rigs previously built a real `ReqwestTransport`. Nothing here ever sent a - /// request through it -- the engine is constructed with an EMPTY adapter list, so no - /// refresh can dispatch -- but that is a property of the arguments at each call site, - /// not of the type. Adding one adapter to any of these rigs would silently turn an - /// admin test into a live token exchange against a provider. - /// - /// Two sibling repos discovered exactly that on 2026-08-29: one suite was issuing 30 - /// real token-exchange requests per run with fabricated credentials, and its - /// assertions had been reading the provider's live rejection rather than their own - /// code. The defect is not the traffic, it is that a remote service was supplying a - /// test's precondition. - /// - /// This makes the guarantee structural: no adapter added later can reach outward, - /// because the transport it would be handed has no outward. `RefreshError::Transport` - /// on use also names the cause at the failure rather than producing a timeout. - struct NoHttp; - - #[async_trait::async_trait] - impl credentials_core::refresh_adapters::HttpTransport for NoHttp { - async fn post( - &self, - _url: &str, - _headers: &[(&str, &str)], - _content_type: &str, - _body: Vec, - ) -> Result< - credentials_core::refresh_adapters::HttpResponse, - credentials_core::refresh_adapters::RefreshError, - > { - Err(credentials_core::refresh_adapters::RefreshError::Transport( - "admin_surface tests do not make network calls".into(), - )) - } - - async fn get( - &self, - _url: &str, - _headers: &[(&str, &str)], - ) -> Result< - credentials_core::refresh_adapters::HttpResponse, - credentials_core::refresh_adapters::RefreshError, - > { - Err(credentials_core::refresh_adapters::RefreshError::Transport( - "admin_surface tests do not make network calls".into(), - )) - } - } - /// A test rig: the AdminSurface plus everything a caller-side signer needs /// (the same MAC key derivation the CLI would perform from the keychain key). struct Rig { @@ -446,7 +396,7 @@ mod tests { let key_id = key.key_id(); let vault_id = vault_id_for(&root).expect("vault id"); let store = Arc::new(EncryptedStore::open(store, key).expect("open vault")); - let http = Arc::new(NoHttp); + let http = Arc::new(crate::test_support::NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); Rig { admin: AdminSurface::new(engine, mac_key, vault_id, key_id), diff --git a/crates/credentials-module/src/main.rs b/crates/credentials-module/src/main.rs index 8a79c5a..e3e259d 100644 --- a/crates/credentials-module/src/main.rs +++ b/crates/credentials-module/src/main.rs @@ -23,6 +23,8 @@ mod admin_surface; mod limiter; mod read_surface; +#[cfg(test)] +mod test_support; use std::path::PathBuf; use std::sync::Arc; @@ -105,6 +107,10 @@ const OP_PUBLIC_KEY: &str = "credential.public_key"; const OP_ADMIN_CHALLENGE: &str = "admin.challenge"; const OP_ADMIN_OP: &str = "admin.op"; +pub(crate) fn wrap_result(value: T) -> serde_json::Value { + json!({ "result": value }) +} + #[tokio::main] async fn main() -> Result<(), ModuleError> { // Answered BEFORE the --subc gate, so it works on a binary that is not being @@ -932,7 +938,7 @@ async fn handle_read_request( let result = match request.method.as_str() { OP_GET => match serde_json::from_value::(request.params) { - Ok(p) => json!({ "result": surface.get(connection_id, &p).await }), + Ok(p) => wrap_result(surface.get(connection_id, &p).await), Err(e) => { return invalid_params(writer, ver, channel, epoch, corr, &e.to_string()).await } @@ -955,9 +961,9 @@ async fn handle_read_request( Ok(r) => json!({ "result": r }), // Keep the same { code, class } shape every other op uses: the class // gives retry policy and the code names the request-specific remedy. - Err(code) => json!({ - "result": { "error": read_surface::ErrorBody { code, class: code.class() } } - }), + Err(code) => wrap_result(json!({ + "error": read_surface::ErrorBody { code, class: code.class() } + })), } } Ok(_) => { @@ -982,9 +988,9 @@ async fn handle_read_request( .await { Ok(r) => json!({ "result": r }), - Err(code) => json!({ - "result": { "error": read_surface::ErrorBody { code, class: code.class() } } - }), + Err(code) => wrap_result(json!({ + "error": read_surface::ErrorBody { code, class: code.class() } + })), } } Ok(_) => { @@ -1549,61 +1555,6 @@ mod tests { use credentials_core::store::{GrantOperation, RecordState}; use read_surface::ReadSurface; - /// A transport that cannot reach the network, used by every rig in this module. - /// - /// These rigs previously built a real `ReqwestTransport`. None of them ever sent a - /// request through it: four construct the engine with an EMPTY adapter list, and the - /// fifth passes `TtlFixtureAdapter`, which ignores the transport it is handed. So the - /// suite made no outbound calls -- but that was a property of the ARGUMENTS at each - /// call site, never of the type. Adding a real adapter to any rig, or a fixture that - /// forwards, would silently turn a unit test into a live token exchange. - /// - /// Two sibling repos found exactly that on 2026-08-29. One suite had been issuing 30 - /// real token-exchange requests per run against a vendor endpoint with fabricated - /// credentials, and one of its tests passed only because the remote service rejected - /// them -- the assertion was reading the provider's live response instead of the code - /// under test. The traffic is not the defect. A remote service supplying a test's - /// precondition is. - /// - /// Verified here by running the suite inside a network namespace with egress dropped, - /// which passed -- but that proves TODAY's arguments, and has to be re-run to keep - /// meaning anything. This makes it structural instead: no adapter added later can - /// reach outward, because the transport it would be handed has no outward. Returning - /// `RefreshError::Transport` also names the cause at the point of use rather than - /// leaving someone to read a timeout. - struct NoHttp; - - #[async_trait::async_trait] - impl credentials_core::refresh_adapters::HttpTransport for NoHttp { - async fn post( - &self, - _url: &str, - _headers: &[(&str, &str)], - _content_type: &str, - _body: Vec, - ) -> Result< - credentials_core::refresh_adapters::HttpResponse, - credentials_core::refresh_adapters::RefreshError, - > { - Err(credentials_core::refresh_adapters::RefreshError::Transport( - "main tests do not make network calls".into(), - )) - } - - async fn get( - &self, - _url: &str, - _headers: &[(&str, &str)], - ) -> Result< - credentials_core::refresh_adapters::HttpResponse, - credentials_core::refresh_adapters::RefreshError, - > { - Err(credentials_core::refresh_adapters::RefreshError::Transport( - "main tests do not make network calls".into(), - )) - } - } - fn tmp_surface(seed: u8) -> Arc { tmp_surface_with_store(seed).0 } @@ -1641,7 +1592,7 @@ mod tests { .open_intent("apikey:crashed", 1, &hash) .expect("open intent"); - let http = Arc::new(NoHttp); + let http = Arc::new(crate::test_support::NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); // The daemon's own boot-gate sequence: reconcile, then record. Calls the same @@ -1667,7 +1618,7 @@ mod tests { /// known master key (seed) so tests can derive the same MAC key caller-side. fn tmp_admin(seed: u8) -> (Arc, Arc) { let (_, store, db_path) = tmp_surface_with_store(seed); - let http = Arc::new(NoHttp); + let http = Arc::new(crate::test_support::NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); let key = MasterKey::from_bytes([seed; MASTER_KEY_LEN]); let mac_key = credentials_core::admin_auth::AdminMacKey::derive(&key); @@ -1722,7 +1673,7 @@ mod tests { store.invalidate("apikey:dead").expect("invalidate"); let store = Arc::new(store); - let http = Arc::new(NoHttp); + let http = Arc::new(crate::test_support::NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); let surface = Arc::new(ReadSurface::new(engine, FetchLimiter::new(Caps::default()))); (surface, store, db_path) @@ -1780,7 +1731,7 @@ mod tests { calls: Arc::clone(&calls), fresh_ttl_ms, }; - let http = Arc::new(NoHttp); + let http = Arc::new(crate::test_support::NoHttp); let engine = Arc::new(RefreshEngine::new( Arc::clone(&store), vec![Arc::new(adapter)], @@ -2031,7 +1982,7 @@ mod tests { Arc, ) { let (surface, store, db_path) = tmp_surface_with_store(seed); - let http = Arc::new(NoHttp); + let http = Arc::new(crate::test_support::NoHttp); let engine = Arc::new(RefreshEngine::new(Arc::clone(&store), Vec::new(), http)); let key = MasterKey::from_bytes([seed; MASTER_KEY_LEN]); let mac_key = credentials_core::admin_auth::AdminMacKey::derive(&key); @@ -4673,7 +4624,7 @@ mod tests { /// with `needs_reauth` without touching the network. /// /// The state is constructed through the production paths (public `report_auth_failure` - /// sets the mark, the same `store.invalidate` the engine uses after a failed refresh + /// sets the mark, then the engine's version-fenced invalidation after a failed refresh /// flips the state), so the test is a real reading of the buggy state rather than a /// hand-staged copy of it. A pure store-level construction would pass without ever /// proving the public route is part of the path that creates it. @@ -4723,11 +4674,16 @@ mod tests { .expect("report accepted"); // Production step 2: a forced refresh then fails and the engine latches the record - // to `needs_reauth`. The store call below is exactly what the engine reaches for - // at the failure site; the column `stale_pending` is deliberately not touched by - // any of the seven state-update paths, which is the bug we are pinning here. + // to `needs_reauth`. The version-fenced store call below is exactly what the engine + // reaches for at the failure site; the column `stale_pending` is deliberately not + // touched by any of the seven state-update paths, which is the bug pinned here. store - .invalidate("oauth:needs_reauth_after_stale") + .invalidate_if_version_reported( + "oauth:needs_reauth_after_stale", + 1, + AuditCtx::vault(AuditOp::Invalidate), + None, + ) .expect("engine-style invalidate after failed refresh"); // Precondition checks: the construction actually reproduced the live shape, so a diff --git a/crates/credentials-module/src/read_surface.rs b/crates/credentials-module/src/read_surface.rs index 95ee9ec..05eb7fc 100644 --- a/crates/credentials-module/src/read_surface.rs +++ b/crates/credentials-module/src/read_surface.rs @@ -1525,10 +1525,9 @@ mod error_class_tests { /// the bytes never move under them. /// /// Serialized through the REAL producer type rather than a hand-built `json!`, then - /// wrapped with the same `result` key `handle_read_request` puts around every route - /// reply — so this pins the full on-wire frame `{"result":{"error":{...}}}`, not just - /// the inner body. A reconstruction would only pin the reconstruction — the frame - /// could drift and this would still pass. + /// wrapped through the same route-serialization helper used by `handle_read_request` + /// for `credential.get` replies — so this pins the full on-wire frame + /// `{"result":{"error":{...}}}`, not just the inner body. /// /// The literal below is the on-wire frame captured from a live daemon and handed to /// that consumer, who pinned it in their tree. Both directions now go red on drift. @@ -1543,7 +1542,7 @@ mod error_class_tests { }, }; let inner_value = serde_json::to_value(&inner).expect("serialize the error outcome"); - let got = serde_json::json!({ "result": inner_value }); + let got = crate::wrap_result(inner_value); // ORDER IS LOAD-BEARING, and this is the second version. Written with the // equality first, the specific checks below never ran: `assert_eq!` panics on any diff --git a/crates/credentials-module/src/test_support.rs b/crates/credentials-module/src/test_support.rs new file mode 100644 index 0000000..c0259eb --- /dev/null +++ b/crates/credentials-module/src/test_support.rs @@ -0,0 +1,34 @@ +/// A transport that cannot reach the network, preventing test fixtures from silently +/// acquiring live provider behavior if an adapter is added later. +pub(crate) struct NoHttp; + +#[async_trait::async_trait] +impl credentials_core::refresh_adapters::HttpTransport for NoHttp { + async fn post( + &self, + _url: &str, + _headers: &[(&str, &str)], + _content_type: &str, + _body: Vec, + ) -> Result< + credentials_core::refresh_adapters::HttpResponse, + credentials_core::refresh_adapters::RefreshError, + > { + Err(credentials_core::refresh_adapters::RefreshError::Transport( + "credentials-module tests do not make network calls".into(), + )) + } + + async fn get( + &self, + _url: &str, + _headers: &[(&str, &str)], + ) -> Result< + credentials_core::refresh_adapters::HttpResponse, + credentials_core::refresh_adapters::RefreshError, + > { + Err(credentials_core::refresh_adapters::RefreshError::Transport( + "credentials-module tests do not make network calls".into(), + )) + } +} diff --git a/crates/credentials-module/tests/cli_admin.rs b/crates/credentials-module/tests/cli_admin.rs index 5bf1398..7d77066 100644 --- a/crates/credentials-module/tests/cli_admin.rs +++ b/crates/credentials-module/tests/cli_admin.rs @@ -1582,7 +1582,7 @@ fn validation_bypass_is_absent_from_a_release_build() { fn api_key_login_flow_integration() { if std::env::var_os(CLI_BIN_ENV).is_some() { eprintln!( - "skipping api_key_login_flow_integration: {CLI_BIN_ENV} is set, and this arm \ + "SKIPPING api_key_login_flow_integration: {CLI_BIN_ENV} is set, and this arm \ needs the debug-only validation bypass that release builds omit" ); return; diff --git a/scripts/gate.sh b/scripts/gate.sh index 95de82e..6f294bc 100755 --- a/scripts/gate.sh +++ b/scripts/gate.sh @@ -148,14 +148,24 @@ run_expect() { # for arms that were never at risk, which is how a correct-sounding guard gets # deleted wholesale instead of fixed. # - # The real discriminator is whether the target's source can EMIT a skip notice, so - # ask the source. A new skip path in any test file arms this automatically; a - # removed one disarms it. Neither requires anyone to remember this function exists. - local a want_target=0 target="" has_nocapture=0 + # The source probe enforces one repository convention: a test-file skip notice uses + # the literal `SKIPPING` token. It does not discover arbitrary skip paths, and it does + # not inspect test-name filters; an arm is required to pass --nocapture when its target + # file follows that convention. The output check below enforces the same token. + local a want_target=0 target="" has_nocapture=0 before_separator=1 for a in "$@"; do [ "$a" = "--nocapture" ] && has_nocapture=1 - [ "$want_target" = "1" ] && { target="$a"; want_target=0; } - [ "$a" = "--test" ] && want_target=1 + if [ "$before_separator" = "1" ]; then + if [ "$a" = "--" ]; then + before_separator=0 + want_target=0 + elif [ "$want_target" = "1" ]; then + target="$a" + want_target=0 + elif [ "$a" = "--test" ]; then + want_target=1 + fi + fi done if [ -n "$target" ] && [ "$has_nocapture" = "0" ]; then local src @@ -280,7 +290,7 @@ run_expect 1 "migration tools" \ # because it builds one. run_expect 1 "release artifact (bypass absent)" \ cargo test --locked -p credentials-module --test cli_admin \ - validation_bypass_is_absent -- --ignored + validation_bypass_is_absent -- --ignored --nocapture # PROVE the scope claim rather than asserting it. "Every check CI runs" rots the # moment CI grows an arm, and that is exactly how it broke: CI gained an inbound