Skip to content

tests: remove the network capability rather than relying on not using it - #22

Merged
ualtinok merged 1 commit into
cortexkit:masterfrom
legion-works:fix/tests-cannot-reach-the-network
Aug 29, 2026
Merged

tests: remove the network capability rather than relying on not using it#22
ualtinok merged 1 commit into
cortexkit:masterfrom
legion-works:fix/tests-cannot-reach-the-network

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

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; 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:

unshare -rn sh -c 'ip link set lo up; cargo test --release --locked --offline --workspace'

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. NoHttp 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 diagnose a timeout.

Verified as a capability change, not a behaviour change

Patching both NoHttp arms to panic! 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:437 is untouched.

bash scripts/gate.sh exit 0, nine real-daemon e2e arms executing.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Replaces the real ReqwestTransport in all six test rigs with a NoHttp stub that returns RefreshError::Transport if 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. NoHttp makes the guarantee structural.

  • Verified the change is capability-only: patching NoHttp to panic! leaves all 80 tests green.
  • Production wiring at main.rs:437 is untouched.

Written for commit dce498c. Summary will update on new commits.

Review in cubic

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

@ualtinok
ualtinok merged commit a683fb9 into cortexkit:master Aug 29, 2026
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants