Skip to content

tests: fix six review findings, four of which were guards not guarding #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ualtinok merged 1 commit into from
Aug 30, 2026
Merged

tests: fix six review findings, four of which were guards not guarding #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 1 addition & 51 deletions crates/credentials-module/src/admin_surface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<u8>,
) -> 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 {
Expand Down Expand Up @@ -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),
Expand Down
100 changes: 28 additions & 72 deletions crates/credentials-module/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<T: serde::Serialize>(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
Expand Down Expand Up @@ -932,7 +938,7 @@ async fn handle_read_request(

let result = match request.method.as_str() {
OP_GET => match serde_json::from_value::<GetParams>(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
}
Expand All @@ -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(_) => {
Expand All @@ -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(_) => {
Expand Down Expand Up @@ -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<u8>,
) -> 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<ReadSurface> {
tmp_surface_with_store(seed).0
}
Expand Down Expand Up @@ -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
Expand All @@ -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<admin_surface::AdminSurface>, Arc<EncryptedStore>) {
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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)],
Expand Down Expand Up @@ -2031,7 +1982,7 @@ mod tests {
Arc<EncryptedStore>,
) {
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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(

@cubic-dev-ai cubic-dev-ai Bot Aug 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This test bypasses RefreshEngine: report_auth_failure only marks the record stale, then this direct store call manufactures NeedsReauth. Drive a failing refresh through RefreshEngine (including its observation), or the test remains green if the production failure path diverges.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/main.rs, line 4681:

<comment>This test bypasses `RefreshEngine`: `report_auth_failure` only marks the record stale, then this direct store call manufactures `NeedsReauth`. Drive a failing refresh through `RefreshEngine` (including its observation), or the test remains green if the production failure path diverges.</comment>

<file context>
@@ -4723,11 +4674,16 @@ mod tests {
+        // 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,
</file context>

"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
Expand Down
9 changes: 4 additions & 5 deletions crates/credentials-module/src/read_surface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
34 changes: 34 additions & 0 deletions crates/credentials-module/src/test_support.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<u8>,
) -> 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(),
))
}
}
2 changes: 1 addition & 1 deletion crates/credentials-module/tests/cli_admin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
24 changes: 17 additions & 7 deletions scripts/gate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading