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