Stop the test suite from calling live provider endpoints

Merged
IC
2mo ago
+2543-154

The test suite has been making 44 real requests to vendor endpoints on every run:

30x  https://platform.claude.com/v1/oauth/token
10x  https://api.anthropic.com/api/oauth/profile
 2x  https://api.anthropic.com/api/oauth/usage
 1x  https://api.anthropic.com/v1/messages?beta=true

From every dev machine and every CI run. Thirty of those are live OAuth token exchanges with fabricated refresh tokens.

Nothing was billed — a token exchange isn't a billed call and the credentials are fake. The problem is what it meant for the tests: they were asserting against whatever the live endpoint returned, not against our code. At least one (stale writer does not resurrect a deleted active account) passed because of the network round-trip and fails the moment it's removed.

The guard

The test preload wraps globalThis.fetch and throws on any non-loopback request, naming the URL. It fails loud rather than returning an empty response — a swallowed request converts a visible network call into an invisible wrong result, which is worse than the status quo.

Loopback stays allowed, because the Miniflare relay-worker tests, the relay tests, and the e2e harness all bind 127.0.0.1. A test that assigns its own globalThis.fetch is unaffected, which is how nearly every test here already works — the wrapper is a data property specifically so reassignment and Bun's spyOn both keep working.

Three ways out of it, all closed after review found them:

  • Redirects. Validating only the initial URL let a loopback server 302 to an external host; verified reaching https://example.invalid/pwned. The guard now follows redirects manually and validates every hop, with a 20-hop cap. Loopback→loopback chains still work. Replayed hops follow the spec's method semantics — 301/302/303 on a non-GET/HEAD request replay as GET with the body dropped, 307/308 preserve method and body — and intermediate hop bodies are drained rather than left open.
  • preconnect. It was forwarded to the native implementation with no check, and Bun's preconnect opens a real socket. Now routed through the same assertion. Nothing currently calls it, which is why it never bit — but the guarantee is that a future test cannot reach the network, not that no current test does.
  • Unparseable input fails closed.

Loopback means the whole 127.0.0.0/8 block, not just 127.0.0.1, so a test binding 127.0.0.2 to run several local servers isn't falsely rejected. Bun canonicalises the integer, hex, and short forms (2130706433, 0x7f000001, 127.1) to dotted quads before we see them; 127.0.0.1.evil.com stays blocked.

The stubs

A shared test-fetch.ts answers the four endpoints deterministically. The shapes match what the code actually needs rather than a blanket 200 — the token endpoint returns 400 invalid_grant, which is presumably what the live endpoint was returning to a fake refresh token.

That distinction is load-bearing and I verified it by mutation: flipping the token stub to a 200 success reddens dead (400 invalid_grant) fallback → needsReauth true and nothing else. So exactly one test depends on the failure shape; the other 29 token calls were incidental machinery whose outcome nobody asserted on.

The messages endpoint matches on exact origin and pathname, with query preserved. It used to match by prefix, so /v1/messages/count was silently served a 401 as though it were an anticipated call — backwards for a mock whose job is failing loudly on anything nobody planned for.

The races the stubs exposed

Removing the network didn't create races, it revealed ones that latency had been hiding. transient (429 rate-limited) fallback → needsReauth false went intermittent at roughly 2-in-3: the stub answers instantly where the live call took ~300ms, so a background refresh that previously hadn't completed by assertion time now finishes first, marks the account dead from the permanent invalid_grant, and the assertion loses.

That test no longer answers TOKEN_URL at all — a permanent auth failure is outside what a transient rate-limit test means to exercise — and it now asserts zero token calls, so the exclusion is pinned rather than assumed. Separately, the getPlugin helpers in all four files that have one disable background intervals by default, because unowned setInterval callbacks were outliving their tests and firing into other tests' restored mocks.

Leaks the suite now catches at author time

Each hygiene fix here started life as a per-file habit, and the suite kept adding files without the habit. The durable form of each is a mechanical invariant:

  • Fetch-mock leaks. The preload's global afterEach asserts globalThis.fetch identity after every test in every file — anything other than the guard, a tagged default mock instance, or the module original throws with the offending test named. It restores the guard before throwing, so a leak fails its owner without contaminating whichever test runs next. This is detection, not blanket restore: a blanket restore would hide the next instance instead of naming it. Enforcement found real leaks immediately — including a 25-test describe in the stacked #175 that had never restored fetch.
  • Timer leaks. Background intervals are disabled by default in every getPlugin helper, tracked when explicitly enabled, and asserted drained in teardown. The leak-count bookkeeping resets per test, so the assertion holds under reordering rather than depending on which test happens to run last.
  • Temp directories. The suite had leaked 18,094 /tmp/anthropic-* directories (145 MB) — nearly every file that mkdtemps left its dirs behind. Every creator now owns its teardown. The acceptance check is end-state, not per-file: count /tmp/anthropic-* dirs before and after a full run; both read 0. One subtlety worth knowing: the plugin writes into its temp dirs asynchronously, so teardown tracks per-file ownership and sweeps late — a naive global cleanup in the preload would race parallel files and delete live dirs, which is why there isn't one.

Six production fixes that came out of the same work

None is test-only, and five of them destroy credentials.

A config we cannot parse is not a config with no accounts. Account membership is derived from the config file to decide what runtime state a save may prune. A missing file correctly meant unknown → prune nothing. But a populated accounts array whose entries yield no valid ids collapsed to an empty set, read as known-empty, and pruned everything — deleting every account's refresh token, which the state file alone stores. Probed:

CONTROL real id      survived=true
GENUINE  []          survived=false   correct
JUNK     [{}]        survived=false   wrong
JUNK     [{id: 123}] survived=false   wrong

Both halves are fixed: any unparseable entry yields unknown membership, and unknown membership prunes nothing. A config naming a different account still prunes the orphan, so the anti-resurrection property the mechanism exists for is intact.

Worth noting where the bug actually was, because it is not where it looks. Fixing only the membership derivation leaves the wipe live — the second half is that unknown membership fell back to pruning against the writer's own snapshot, and load drops the same unparseable entries, so that snapshot is empty for exactly the reason membership was unknown.

Membership ids and stored ids normalize differently, and the comparison ran across that gap. The loader trims account ids on the way in (normalizeAccountBase), so every stored id is trimmed; the config membership set compared against them was built from raw config values. An id with surrounding whitespace was therefore "absent" from its own config, and the prune deleted its refresh token. Both sides now normalize identically, and a whitespace-only id still counts as unparseable — feeding the unknown-membership rule above rather than the prune. The regression for this loads through the real loadAccounts path, because a hand-built storage fixture bypasses the loader's normalization and false-greens — the same fixture-cannot-reach-the-path trap that runs through this whole branch.

Membership must come from the loader's parse, not id syntax. The membership set counted a config entry as parseable if it was a record with a nonblank string id. The loader is stricter: normalizeAccount drops entries whose shape fails (an api entry with an invalid baseURL returns null). So [{id: 'a', type: 'api', baseURL: 'garbage'}] established known membership {a} and pruned every other account's refresh token — while loadAccounts on the same config loads zero accounts. An entry now establishes membership only if it has a valid trimmed id AND the loader accepts it — where "accepts" consults the incoming save payload first and the on-disk state as fallback, so a save can vouch for a brand-new account whose refresh token it is writing right now, while scoped saves by writers that don't carry the account keep the fail-safe unknown. The id requirement stays deliberately: an id-less entry the loader accepts gets a minted randomUUID() on load, so shape-validity alone would build a membership set that matches nothing and prunes everything.

State keys and config ids disagreed about whitespace, in both directions. The loader read runtime state by the raw config id, but post-load saves wrote state under the trimmed id. For a config entry with surrounding whitespace that meant two defects at once: any scoped save pruned the padded state key the loader was actually serving (deleting a still-listed account's refresh token), and — worse — token rotation persisted fresh credentials under the trimmed key the loader never consulted, so a reload served the pre-rotation token indefinitely (probed: second load returned refresh-old). The trimmed key is now canonical on save, superseded padded variants are dropped instead of duplicated, and the loader falls back raw-then-trimmed so legacy padded-keyed state still loads. Three mutations pin it: removing the loader fallback, removing the save-side reconciliation, and restoring the old raw-only loader each redden named tests.

Scoped saves left removed accounts orphaned. Known-membership pruning ran only on full saves, so a scoped save (persistPushedQuota fires one per served request) preserved state for an account the config no longer lists. That state would then merge onto a re-added account reusing the same id. Reachable whenever the config changes outside removeAccountPersistent — an operator editing the file, which is exactly how the defect above gets triggered too.

The two fixes pull in opposite directions, so both directions are pinned: a scoped save naming account Y must prune X when X is gone from the config, and must leave X alone when X is still in it. Making the pruning over-eager reddens five tests.

A published quota entry could carry another account's observation time. persistPushedQuota validated that the persisted quota snapshot belonged to the request's account, then separately adopted the top-level mainQuotaCheckedAt — a field with no account binding of its own. Check one field, trust another that can disagree. Deterministic repro: quota identity account-a alongside top-level timestamp 2_000_000 publishes observed_at_ms=2_000_000 for a request whose own observation was 1_000_000.

Publication now always uses the request's own observation time. Worth saying how it was found, because it says something about this branch: it first appeared as an intermittent CI failure that I could not reproduce locally and wrongly dismissed. Removing ~300ms of network latency from the suite is what made the interleaving reachable — so the guard above didn't just hide a defect, it exposed one.

The same unbound pair feeds a second consumer (getPersistedMainQuota → seedMainFromStorage). That one is filed as #177 rather than fixed here; both fix directions touch persisted-state semantics and one needs a schema-compatibility decision.

Verification

full gate         1366 pass / 0 fail
e2e               29 pass / 0 fail
typecheck         clean
format:check      clean
blocked-URL audit 0 vendor hits
temp-dir count    0 before and 0 after the full suite

The audit is the real acceptance check — I instrument the guard to log every blocked URL and run the whole suite. The seven hits are all deliberate: four example.invalid sentinels proving the guard fires, and the three near-misses (128.0.0.1, 27.0.0.1, 127.0.0.1.evil.com) that prove widening to 127.0.0.0/8 didn't overshoot.

Scope

packages/core and packages/e2e-tests are untouched. Core has no preload of its own, and the e2e harness talks to local mock servers on 127.0.0.1 which would pass the guard as written. Both are worth a follow-up; neither is widened here.

relay-worker-miniflare.test.ts fails intermittently under suite load and passes solo. It's an unbounded await mf.ready waiting on worker startup, not the websocket logic the test name suggests, and it reproduces on a clean upstream tree — filed as #176.


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

Stops OpenCode tests from making 44 live Anthropic and Claude requests by replacing provider calls with deterministic fixtures and a preload guard that blocks non-loopback network access. This makes tests faster and isolated while fixing account-state and quota-timestamp races the network latency had been hiding.

Test isolation

  • The guard validates every redirect and preconnect target, fails closed on invalid URLs, and allows 127.0.0.0/8, localhost, and [::1].
  • Shared fixtures cover token, profile, quota, and messages requests; unexpected URLs fail loudly.
  • Test helpers disable and track background intervals, detect fetch leaks, clean temporary directories, and expose initial refresh completion for synchronization.

State safety

  • Account-state pruning uses safely parsed config membership; unknown membership prunes nothing, while scoped saves remove accounts deleted by external config edits.
  • Whitespace-padded account IDs use canonical state keys and still load legacy padded or trimmed keys.
  • Published quota entries retain the request's own checkedAt timestamp.
  • The full gate and e2e suite pass; the intermittent relay-worker-miniflare websocket failure remains a pre-existing flake.

Greptile Summary

The PR prevents OpenCode tests from reaching non-loopback endpoints and replaces vendor calls with deterministic fixtures. It also makes race-sensitive tests deterministic and changes account-state pruning to use config-declared membership.

  • Adds a preload-level network guard with loopback and redirect handling.
  • Adds shared token, profile, quota, and messages fixtures.
  • Exposes completion of the initial fallback refresh for test synchronization.
  • Preserves replacement-account state during stale runtime-state writes.
  • Updates package versions and internal dependency pins to 1.21.0.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains.

No blocking failure remains.

Important Files Changed

FilenameOverview
packages/opencode/src/tests/setup.tsInstalls the test-suite fetch guard, permits loopback traffic, and manually validates redirect destinations.
packages/opencode/src/tests/test-fetch.tsProvides deterministic fixtures for the four vendor endpoints previously reached by tests.
packages/core/src/accounts.tsBases per-account state pruning on safely parsed config membership and returns the initial background-refresh promise.
packages/opencode/src/index.tsPreserves request-time quota timestamps and exposes initial fallback-refresh completion to tests.
packages/opencode/src/tests/index.test.tsAdds deterministic fetch setup, disables unowned periodic timers, and synchronizes race-sensitive assertions with observable completion.
packages/opencode/src/tests/network-guard.test.tsCovers blocked external requests, loopback access, redirect validation, fetch replacement, fixtures, and preconnect behavior.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Test[Test invokes fetch] --> Mocked{Test-installed mock?}
  Mocked -->|Yes| Fixture[Deterministic provider fixture]
  Mocked -->|No| Guard[Test preload network guard]
  Guard --> Loopback{Loopback destination?}
  Loopback -->|No| Block[Throw actionable error]
  Loopback -->|Yes| Native[Native fetch]
  Native --> Redirect{Redirect response?}
  Redirect -->|No| Result[Return response]
  Redirect -->|Yes| Guard

Reviews (9): Last reviewed commit: "test(opencode): stop the suite reaching ..." | Re-trigger Greptile