tests: remove the network capability rather than relying on not using it - #22
Merged
ualtinok merged 1 commit intoAug 29, 2026
Conversation
Six test rigs across main.rs and admin_surface.rs built a real ReqwestTransport. None of them ever sent a request through it: five construct the engine with an empty adapter list, so no refresh can dispatch, and the sixth 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 one real adapter to any rig, or a fixture that forwards rather than ignores, would silently turn a unit test into a live token exchange against a provider. Nothing would have flagged it; the test would still pass, and it would pass for a reason supplied by a remote service. Two sibling repos found exactly that today. One suite had been issuing thirty real token-exchange requests per run against a vendor endpoint with fabricated credentials, and one of its tests passed only because the endpoint rejected them -- the assertion had been reading the provider's live response instead of the code under test, green for months. The traffic is not the defect. A remote service supplying a test's precondition is. I verified this suite empirically first, running it in a network namespace with egress dropped, and it passed. That proves today's arguments and expires the moment someone edits a rig. NoHttp makes it structural: 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. Verified as a capability change rather than a behaviour change: patching both NoHttp arms to panic leaves the suite green (80 passed), so nothing exercises the transport at all. The replacement asserted its five call sites were present before editing, and the production wiring at main.rs:437 is untouched. Gate: exit 0, nine real-daemon e2e arms executing.
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/credentials-module/src/admin_surface.rs">
<violation number="1" location="crates/credentials-module/src/admin_surface.rs:381">
P3: This adds a second independently maintained `NoHttp` transport implementation; future transport changes can make the module test rigs diverge. Move the stub into one shared test helper and use it from both test modules.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// 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; |
There was a problem hiding this comment.
P3: This adds a second independently maintained NoHttp transport implementation; future transport changes can make the module test rigs diverge. Move the stub into one shared test helper and use it from both test modules.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/admin_surface.rs, line 381:
<comment>This adds a second independently maintained `NoHttp` transport implementation; future transport changes can make the module test rigs diverge. Move the stub into one shared test helper and use it from both test modules.</comment>
<file context>
@@ -356,12 +356,61 @@ mod tests {
+ /// 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]
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six test rigs across
main.rsandadmin_surface.rsbuilt a realReqwestTransport.None of them ever sent a request through it. Five construct the engine with an empty adapter list, so no refresh can dispatch; the sixth passes
TtlFixtureAdapter, which ignores the transport it is handed.But that is a property of the arguments at each call site, not of the type. Adding one real adapter to any rig — or a fixture that forwards rather than ignores — would silently turn a unit test into a live token exchange against a provider. Nothing would flag it. The test would still pass, and it would pass for a reason supplied by a remote service.
Why this is worth changing when nothing is currently broken
Two sibling repos found exactly this today, independently.
One suite had been issuing 30 real token-exchange requests per run against a vendor endpoint with fabricated credentials, from every dev machine and every CI run, for months. One of its tests passed only because the endpoint rejected them — the assertion was reading the provider's live response rather than the code under test. It went red the moment a network guard was added, which is how it was found.
The traffic is not the defect. A remote service supplying a test's precondition is.
This repo is one adapter argument away from the same shape, and the argument that protects it is not visible from any test's own text.
Empirical first, then structural
I checked this suite the empirical way before changing anything — ran it inside a network namespace with egress dropped:
It passed, with loopback up (the e2e arms need it) and external egress unreachable. So the current state is genuinely clean.
That result expires the moment someone edits a rig, and re-running it requires knowing to.
NoHttpmakes it structural instead: no adapter added later can reach outward, because the transport it would be handed has no outward. ReturningRefreshError::Transportalso names the cause at the point of use rather than leaving someone to diagnose a timeout.Verified as a capability change, not a behaviour change
Patching both
NoHttparms topanic!leaves the suite green — 80 passed, nothing touches the transport at all. So this removes a capability that was never exercised rather than altering something that was.The replacement asserted its five call sites were present before editing (a substitution that silently matches nothing is indistinguishable from one that worked), and the production wiring at
main.rs:437is untouched.bash scripts/gate.shexit 0, nine real-daemon e2e arms executing.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Replaces the real
ReqwestTransportin all six test rigs with aNoHttpstub that returnsRefreshError::Transportif a request is ever attempted.The suites never actually made network calls, but that was enforced only by the arguments at each call site, not by the type. Adding one real adapter to any rig would have silently turned a unit test into a live token exchange.
NoHttpmakes the guarantee structural.NoHttptopanic!leaves all 80 tests green.main.rs:437is untouched.Written for commit dce498c. Summary will update on new commits.