Stop the test suite from calling live provider endpoints - #174
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Architecture diagram
sequenceDiagram
participant Test as Test Suite
participant Guard as Preload Network Guard
participant Stub as Shared Fetch Stub (test-fetch.ts)
participant NativeFetch as Native Fetch
participant Loopback as Local Server (127.0.0.1)
participant Vendor as Vendor Endpoints
Note over Test,Guard: Test Startup - Preload setup.ts
Test->>Guard: Install guardedFetch as globalThis.fetch
Guard->>Guard: Capture nativeFetch reference
Note over Test,Guard: Test Execution - Fetch Call
Test->>Guard: globalThis.fetch(url)
Guard->>Guard: Parse URL
alt Loopback URL (127.0.0.1, ::1, localhost)
Guard->>NativeFetch: Forward to native fetch
NativeFetch->>Loopback: HTTP request
Loopback-->>NativeFetch: Response
NativeFetch-->>Guard: Response
Guard-->>Test: Return response
else Non-loopback URL (vendor endpoints)
Guard-->>Test: Throw "Blocked non-loopback fetch" error
end
Note over Test,Stub: Test Setup - Install Deterministic Stub
Test->>Stub: installDefaultFetchMock()
Stub->>Test: Replace globalThis.fetch with mock function
Note over Test,Vendor: Stubbed Fetch - OAuth Token Endpoint
Test->>Stub: fetch(TOKEN_URL)
Stub->>Stub: Match TOKEN_URL
Stub-->>Test: 400 invalid_grant response
Note over Test,Vendor: Stubbed Fetch - Profile/Quota/Messages Endpoints
Test->>Stub: fetch(PROFILE_URL | QUOTA_URL | MESSAGES_URL)
Stub->>Stub: Match endpoint pattern
Stub-->>Test: 401 unauthorized response
Note over Test,Vendor: Stubbed Fetch - Unknown URL
Test->>Stub: fetch(unknown URL)
Stub-->>Test: Reject with "Unexpected test fetch" error
Note over Test,Guard: Test Teardown - Restore Original Fetch
Test->>Guard: Restore guardedFetch as globalThis.fetch
Note over Test,Guard: Background Timer Isolation
Test->>Test: getPlugin() with disabled setInterval
Test->>Test: Override timers to prevent leaked callbacks
Test->>Test: Individual tests can provide explicit timer overrides
Note over Test,Vendor: Guard Proof Test
Test->>Guard: fetch('https://example.invalid/provider')
Guard-->>Test: Throw "Blocked non-loopback fetch" error
Test->>Test: Verify error message contains URL
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ualtinok
left a comment
ualtinok
left a comment
There was a problem hiding this comment.
Thanks for removing the live provider traffic. The shared endpoint fixtures and timer isolation are the right direction, but I found two blocking gaps and could not reproduce the PR's deterministic full-suite result.
-
The preload guard can be bypassed by redirects (
packages/opencode/src/tests/setup.ts:33). It validates only the initial URL and then calls nativefetchwith normal redirect following. I verified this with a loopbackBun.serveresponse returning302 Location: http://example.invalid/provider: the request reached external transport and failed there rather than throwingBlocked non-loopback fetch. A loopback mock redirecting to an Anthropic endpoint could therefore still make the live request. Please force manual redirect handling and reject or revalidate every destination, with a regression test covering loopback → non-loopback redirect. -
The stale-writer regression does not await the writer it intends to test (
packages/opencode/src/tests/index.test.ts:3314). AfterreleaseRefresh.resolve(), it immediately accepts sidebar state thatauth.loaderhas already published. As a mutation check, I removedreleaseRefresh.resolve()entirely, leaving the stale writer permanently blocked; the test still passed in 25 ms. Please add a completion signal after the background refresh/save path finishes, then assert the resulting disk/sidebar state.
I also ran the full OpenCode suite twice at 22b0ce3e on the configured Bun 1.3.14 runtime. Neither run was clean:
- 1316/1317:
cross-account reload cannot retime a published main observation - 1314/1317: the same test,
late fallback profile hydration cannot restore rotated credentials, andmain refresh through the plugin keeps the lineage and prime claim
The focused network/stub tests passed (97/97), as did typecheck, lint, and format. The failures are ordering/state assertions rather than network or timeout errors, so the deterministic full-gate acceptance claim is not yet established. Please harden the relevant completion conditions and rerun the full gate under contention.
|
Both blocking findings confirmed — I reproduced each before starting on them, and the first one falsifies this PR's own premise. 1. The redirect bypass is real, and it reaches the network. A loopback server returning That is a live external request the guard did not stop. Validating only the initial URL is not validating the request. Fixing it to force manual redirect handling and check every hop against the same allow-list, with both a loopback → non-loopback rejection test and a loopback → loopback still-works test. You also pointed at the right line for a second hole next to it: 2. The stale-writer test does not exercise the writer. I deleted the Passes in under 10ms with the mechanism never running. It asserts a sidebar state the boot path produces on its own, so the writer's completion was incidental to the assertion rather than a precondition of it. Adding a completion signal after the background refresh/save path and asserting the resulting state, then proving it two ways: it must fail with the writer blocked, and it must fail with the production anti-resurrection guard removed. 3. On the three ordering failures — I cannot reproduce them, and I am treating them as real anyway. What I tried: each of the three passes 5/5 in isolation, and three full-suite runs under four spinning CPU hogs surfaced only the miniflare timeout, never these. Same Bun 1.3.14. So I have no local repro to work from, and I am not going to claim your runs were noise. The likely mechanism is one this PR created. Stubbing the endpoints removed ~300ms of real network latency suite-wide, and I already found one test that went 2-in-3 flaky for exactly that reason — a background refresh that used to finish after an assertion now finishes before it. All three of yours involve background refresh, hydration, or claim ordering, and your machine's scheduling would expose that where mine does not. So rather than hunt the repro, I am auditing those three for the same defect shape as finding 2 — assertions satisfiable before the work they depend on completes — and hardening the completion conditions so the result does not depend on scheduling. If one turns out to depend on another test's state rather than its own timing, that is a worse finding and I will report it as such rather than paper it with a wait. I will post the three full-gate runs rather than one, since a single green run is exactly what you are saying does not establish this. On the red CI here: that is the miniflare timeout, and I filed the mechanism as #176. It is not the websocket exchange the test is named for — every timeout in that harness is bounded at 5s but the failure lands at 30s, so the hang is in the unbounded Thanks for running it twice and for not taking the green at face value — finding 2 is the fourth assertion-over-an-empty-set on this branch, and it was mine. |
|
Both blocking findings fixed, 1. Every hop is validated now, not just the first. The guard forces Re-ran my original probe plus the chain case that a naive fix would survive:
One detail worth flagging, since the fix needed narrow casts around a Bun/undici 2. The stale-writer test now requires the writer. It awaits the background refresh/save path, then asserts disk and sidebar state. Same mutation as before — deleting It also fails with the production anti-resurrection guard removed, so it is pinned from both directions. 3. On the three ordering failures. Still no local reproduction, and I want to be straight about what that means: I hardened them on the hypothesis rather than on evidence I could produce.
No cross-test state leakage found. Full gate, run five times — three by the implementer, two by me, all at the final tree: e2e 29/0, typecheck, format, and changed-file Biome all clean. Blocked-URL audit re-run: three hits, all the deliberate The only failure across all five is #176, which I filed with its mechanism and reproduction. I am not counting a run containing it as green, so: four of five runs carry that one known failure and none carry anything else. Thank you for the second full run. Finding 2 is the fourth assertion-over-an-empty-set on this branch and the one I am least comfortable with, because it sat inside the PR whose entire subject is tests that pass without testing anything. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Fixed in Membership is now Reverting the fix reddens the two unknown-membership tests; treating known-empty as unknown reddens the third. The stale-writer test still fails with its writer blocked (4010ms) and passes restored. One thing from this round worth surfacing, because it is a defect in a fix for a defect of the same kind. The stale-writer test I hardened earlier today still had a hole. My mutation blocked the token refresh, and it correctly reddened — but blocking the save inside So: the original test passed without the writer running; my fix made it require the writer to run; it took a third pass to make it require the writer to write. Each round the assertion moved closer to the actual mechanism, and each intermediate version looked complete. Gate at the pushed sha, run twice: Worth stating plainly: this defect was mine, introduced by the stale-writer fix in this PR, and it wiped refresh tokens on a path that is one |
|
Correction to what I told you earlier: I have now reproduced It failed on a full-suite run just now: A published feed entry for account A carried account B's observation time — exactly what the test is named to prevent. This is the one of your three I deliberately left unchanged, on the grounds that its wait already awaited the publication directly and there was no premature acceptance to fix. That reasoning was right about the test and wrong about the conclusion I drew from it: the test was fine, the production code was not, and "I cannot reproduce it" was a statement about my machine rather than about the code. The mechanism, in const persistedQuotaBelongsToRequest = Boolean(
persistedQuota && persistedQuota.accountIdentity === accountIdentity,
)
...
checkedAt: persistedQuotaBelongsToRequest
? (reloaded.quota?.mainQuotaCheckedAt ?? entry.checkedAt)
: entry.checkedAt,The guard proves that That is pre-existing — it arrived with the account-identity work, not with this PR. What this PR did was remove ~300ms of network latency suite-wide and make the interleaving reachable. Same story as the flake I already fixed here, and it means your two runs were sampling a real defect rather than noise. Being fixed now, with a deterministic test that constructs the interleaving instead of waiting for it, plus a 20-run loop on the named test — three runs cannot evidence an intermittent failure, which is the mistake I made when I reported it unreproducible. I am also having the other two you listed re-examined for the same root cause, since if they share it, the completion-condition hardening I added would be masking the defect rather than fixing it. I would rather land this fix here than separately, since you raised it here, but it is a production change in quota persistence rather than test hardening — say if you would prefer it split out. |
|
The absent-vs-empty fix you reviewed is only two thirds of the rule. A third variant survives, and it wipes credentials the same way. I fixed absent (missing file, no The last two are a populated config — the operator has accounts — and the save deletes every account's runtime state, including the refresh tokens, because the state file is the only token store. Same blast radius as the original, reached by a malformed or partially-written config rather than a missing one. My rule was "absent is not empty". The case I missed is that unparseable is not empty either: a non-empty array that produces no usable ids is a signal that membership could not be determined, not a statement that there are no members. Only a genuine Fixing it in the next round on this branch, with the probe above as the regression. Two more from the same review round, both on this PR's own guards rather than on shipped behaviour — I am recording them because they are exactly the class we have been chasing:
Both are gaps in my tests, not in the code under them; the reviewer executed red-checks for each and found the shipped logic correct. |
98d5844 to
372ee2e
Compare
|
Squashed to one commit, The unparseable-config wipe I reported above is fixed — but not where I first said, and my first fix did not close it. I probed the current tree and the wipe was still live: Membership derivation was already correct — a populated config with any unparseable entry returns "unknown". The wipe was one line further down, in what happens when membership is unknown: it fell back to pruning against Undetermined membership now prunes nothing. After: The last line matters: a config declaring a different account still prunes the orphaned state, so the anti-orphan property this whole mechanism exists for is intact. Why the test suite could not see it. The existing fixture wrote a junk config and passed a hand-built That is the same shape as the other defects on this branch: the fixture was structurally incapable of reaching the code path it was named for. Third time in this PR, so it is worth stating as a rule — a persistence test whose in-memory state did not come from the on-disk state it writes is testing a world that cannot exist. Also in this squash, from the same review round: the two "does not roll back" race tests now force the interleaving through a held lock rather than relying on scheduler luck, and restoring the naive whole-storage clear reddens both — previously it did not, which is what made them non-discriminating. |
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
372ee2e to
6005f79
Compare
|
The P1 is correct and it caught a stacking error on my part. Fixed in I found this defect earlier and fixed it in the wrong PR — the fix landed on the stacked branch (#175) while the code that has it lives here. So #174 could have merged on its own, shipping the wipe, and my report above saying it was fixed would have been true only of a branch nobody was merging yet. Probed on this branch's tree before the fix: Both halves are now here where they belong: a populated The last line is the one that matters for your original finding — a config declaring a different account still prunes the orphan, so the anti-resurrection property is intact. Worth being precise about where the wipe was, because it is not where the P1 points. Membership derivation returning an empty set for junk entries is half of it; the other half is what happens when membership is unknown, which fell back to pruning against On the three P3s:
|
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Confidence score: 5/5
- In
packages/opencode/src/tests/index.test.ts,getPlugin()bases background-interval suspension on the ambientglobalThis.setInterval, so instrumentation or test setup could make plugin behavior inconsistent and allow unintended intervals; make the suspension decision explicit or cover both global-state paths in tests.
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="packages/opencode/src/tests/index.test.ts">
<violation number="1" location="packages/opencode/src/tests/index.test.ts:372">
P3: The automatic suspension of background intervals in `getPlugin()` is decided from ambient global state: a plugin only gets no-op background intervals when `globalThis.setInterval === originalSetInterval` (module-load identity), merged with the shared module-level `pluginTimerOverrides`. Any future test that needs setInterval-driven periodic refresh (e.g. a quota/refresh interval that must actually fire) and replaces `globalThis.setInterval` must restore it to the exact module-load function or pass explicit `timerOverrides`, or it silently gets real scheduled timers (no-op wasn't applied) or no timers at all. A test that replaces `globalThis.setInterval` inline (e.g. the prime-refresh test at line 14801) relies on the describe `afterEach` restore; a test that sets it without a matching global restore in its own afterEach will leak the wrong default into later `getPlugin()` calls in the file. Consider making the interval suspension explicit per call site instead of inferring it from global timer identity so the behavior at each `getPlugin()` invocation is deterministic and self-documenting.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| timerOverrides: PluginTimerOverrides = pluginTimerOverrides, | ||
| timerOverrides: PluginTimerOverrides = {}, | ||
| ) { | ||
| const defaultTimerOverrides = |
There was a problem hiding this comment.
P3: The automatic suspension of background intervals in getPlugin() is decided from ambient global state: a plugin only gets no-op background intervals when globalThis.setInterval === originalSetInterval (module-load identity), merged with the shared module-level pluginTimerOverrides. Any future test that needs setInterval-driven periodic refresh (e.g. a quota/refresh interval that must actually fire) and replaces globalThis.setInterval must restore it to the exact module-load function or pass explicit timerOverrides, or it silently gets real scheduled timers (no-op wasn't applied) or no timers at all. A test that replaces globalThis.setInterval inline (e.g. the prime-refresh test at line 14801) relies on the describe afterEach restore; a test that sets it without a matching global restore in its own afterEach will leak the wrong default into later getPlugin() calls in the file. Consider making the interval suspension explicit per call site instead of inferring it from global timer identity so the behavior at each getPlugin() invocation is deterministic and self-documenting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/index.test.ts, line 372:
<comment>The automatic suspension of background intervals in `getPlugin()` is decided from ambient global state: a plugin only gets no-op background intervals when `globalThis.setInterval === originalSetInterval` (module-load identity), merged with the shared module-level `pluginTimerOverrides`. Any future test that needs setInterval-driven periodic refresh (e.g. a quota/refresh interval that must actually fire) and replaces `globalThis.setInterval` must restore it to the exact module-load function or pass explicit `timerOverrides`, or it silently gets real scheduled timers (no-op wasn't applied) or no timers at all. A test that replaces `globalThis.setInterval` inline (e.g. the prime-refresh test at line 14801) relies on the describe `afterEach` restore; a test that sets it without a matching global restore in its own afterEach will leak the wrong default into later `getPlugin()` calls in the file. Consider making the interval suspension explicit per call site instead of inferring it from global timer identity so the behavior at each `getPlugin()` invocation is deterministic and self-documenting.</comment>
<file context>
@@ -343,8 +367,13 @@ beforeEach(() => {
- timerOverrides: PluginTimerOverrides = pluginTimerOverrides,
+ timerOverrides: PluginTimerOverrides = {},
) {
+ const defaultTimerOverrides =
+ globalThis.setInterval === originalSetInterval &&
+ globalThis.clearInterval === originalClearInterval
</file context>
There was a problem hiding this comment.
No live defect: no current test replaces globalThis.setInterval while a getPlugin() caller relies on the module-load-identity check, so the ambient decision never misfires today. The concrete order-dependence in this class — the disabledIntervalCalls counter that only works because the suite runs serially — is being fixed to reset in beforeEach rather than relying on order. The broader ambient-identity design is left as-is intentionally: a test that genuinely needs live periodic timers passes explicit timerOverrides (the opt-in path the positive control at add-account-flows.test.ts:168 exercises), which bypasses the identity check entirely.
cf5bef3 to
f5ef5ee
Compare
|
All three P3s closed, folded into the single commit — The stub now matches exactly. Origin plus pathname, with arbitrary query preserved because the real callers send
Your point about it being unreachable today was the reason to fix rather than skip: the guard's value is that a future test cannot reach the network, not that no current test does. The If a future test does land in that position it will inherit silently, which is a fair thing to dislike — but the honest answer today is that the hazard is hypothetical, and I would rather say so than manufacture a fix and claim a defect was closed. |
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
f5ef5ee to
81200aa
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on September 19. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Confidence score: 4/5
- In
packages/opencode/src/index.ts, replacing persistedmainQuotaCheckedAtwithentry.checkedAtcan produce an incorrect quota-check timestamp when multiple supported processes harvest the same main account concurrently; preserve the persisted value and cover the concurrent-writer case.
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="packages/opencode/src/index.ts">
<violation number="1" location="packages/opencode/src/index.ts:1258">
P2: This change drops preservation of the persisted `mainQuotaCheckedAt` and always reports `entry.checkedAt`. Under a concurrent writer harvesting quota for the same main account (multi-process is supported here), the reloaded `mainQuotaCheckedAt` — which `mergeHeaderQuotaForPersistence` merges from — can be newer than this request's `entry.checkedAt`, so `publishQuotaHeaderFeed` publishes `observed_at_ms` that is older than the newest quota window data it carries. It also contradicts the PR description's stated goal of "preserve quota timestamps." Please confirm this is intentional; in the single-process case the two are identical, so the change only rewrites cross-process behavior.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| checkedAt: persistedQuotaBelongsToRequest | ||
| ? (reloaded.quota?.mainQuotaCheckedAt ?? entry.checkedAt) | ||
| : entry.checkedAt, | ||
| checkedAt: entry.checkedAt, |
There was a problem hiding this comment.
P2: This change drops preservation of the persisted mainQuotaCheckedAt and always reports entry.checkedAt. Under a concurrent writer harvesting quota for the same main account (multi-process is supported here), the reloaded mainQuotaCheckedAt — which mergeHeaderQuotaForPersistence merges from — can be newer than this request's entry.checkedAt, so publishQuotaHeaderFeed publishes observed_at_ms that is older than the newest quota window data it carries. It also contradicts the PR description's stated goal of "preserve quota timestamps." Please confirm this is intentional; in the single-process case the two are identical, so the change only rewrites cross-process behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/index.ts, line 1258:
<comment>This change drops preservation of the persisted `mainQuotaCheckedAt` and always reports `entry.checkedAt`. Under a concurrent writer harvesting quota for the same main account (multi-process is supported here), the reloaded `mainQuotaCheckedAt` — which `mergeHeaderQuotaForPersistence` merges from — can be newer than this request's `entry.checkedAt`, so `publishQuotaHeaderFeed` publishes `observed_at_ms` that is older than the newest quota window data it carries. It also contradicts the PR description's stated goal of "preserve quota timestamps." Please confirm this is intentional; in the single-process case the two are identical, so the change only rewrites cross-process behavior.</comment>
<file context>
@@ -1255,9 +1255,7 @@ const anthropicAuthPlugin = async (
- checkedAt: persistedQuotaBelongsToRequest
- ? (reloaded.quota?.mainQuotaCheckedAt ?? entry.checkedAt)
- : entry.checkedAt,
+ checkedAt: entry.checkedAt,
}
: entry
</file context>
There was a problem hiding this comment.
Half right, and the half that's right is already fixed in a different PR — but not restoring the adoption, because that is a cross-account leak.
Why it was removed. The persisted top-level mainQuotaCheckedAt carries no account binding of its own, while the quota snapshot beside it does. The old code validated identity on the snapshot and then trusted the unbound timestamp, so a published feed entry for account A could carry account B's observation time. Reproduced deterministically: quota identity account-a with top-level timestamp 2_000_000 publishes observed_at_ms=2_000_000 for a request whose own observation was 1_000_000. It surfaced first as an intermittent CI failure, which is how I found it — removing ~300ms of network latency from the suite made the interleaving reachable where it previously wasn't. The same unbound pair exists at a second consumer (getPersistedMainQuota → seedMainFromStorage), filed as #177.
So restoring the adoption to fix a timestamp-ordering nit would reintroduce cross-account data in a published feed. Not a trade I'll make.
Your inconsistency is real, though, and I'd have told you it was already handled if I hadn't checked. My first instinct was that schema v3's per-field provenance map answers it — observed_at_ms covers header-derived fields, and merged poll-owned fields carry their own checkedAt. That is true, and it is not on this branch: 81200aa is schema v2 with zero fieldSources. The provenance work is #172, open and independent of this stack.
So on this branch as it stands, a published entry can carry merged poll fields newer than the timestamp it reports, with nothing telling a reader that. Narrow, but there is a live consumer of this feed, so it isn't theoretical.
Disposition: the structural fix belongs in #172, where the per-field provenance already exists — I'm not duplicating it here. What this PR gets is a doc comment on observed_at_ms stating what it covers, so the contract is written down rather than reverse-engineered from the merge logic.
On the PR description contradiction you flagged: fair, and it's fixed. The description was written before the leak was found and still described preserving persisted quota times. It now says what the code does.
There was a problem hiding this comment.
Intentional. Reporting entry.checkedAt (the request's own observation time) rather than the reloaded persisted mainQuotaCheckedAt is deliberate: the persisted top-level timestamp has no account binding, so under a concurrent cross-account writer, adopting it re-times this request's published observed_at_ms to a different account's observation — the cross-account leak filed as #177. So the single-process identity you note is the correct behaviour, and the cross-process case must NOT inherit the persisted timestamp.
The narrow real gap is on this branch's v2 feed: a poll-owned field merged in can be newer than the header-derived observed_at_ms, with no reader-visible provenance to distinguish them. The structural fix for that is per-field provenance (each field carries its own source + checkedAt), which is PR #172's schema v3 — not present on this branch. This branch documents the v2 contract (observed_at_ms is header-observation time; merged poll fields retain their own checkedAt) and leaves the mechanism to #172.
81200aa to
353b10e
Compare
|
All four addressed in The timer leak was the real one, and it's the same defect this PR already fixed once. Constants now come from core. Good catch on the failure mode — a changed production URL would have made the stub silently stop matching and surfaced as a confusing "Unexpected test fetch" rather than anything pointing at the cause. The mutation is the nice part: changing the core endpoint constants while leaving the fixture hardcoded reddens the guard tests with Redirect bodies drain, intermediate hops only. The regression risk here is cancelling the response you return, so that's pinned in both directions: a two-hop loopback chain returns its final body intact, and an injected transport asserts intermediate cancel=1 / final cancel=0. Cancelling terminal responses reddens four tests with On So on this branch a published entry can carry merged poll fields newer than the timestamp it reports, with nothing making that visible to a reader. What I won't do is restore the persisted This PR gets a doc comment stating what Blocked-URL audit re-run after the loopback widening: exactly seven hits, all deliberate — four |
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
353b10e to
68caf2b
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
68caf2b to
9f3d4d7
Compare
381b3d5 to
88b9864
Compare
There was a problem hiding this comment.
3 issues found across 14 files (changes from recent commits).
Confidence score: 2/5
saveAccountStateinpackages/opencode/src/tests/accounts.test.tscan misclassify membership by merging an incoming runtime account intoconfiguredIds, producing incorrect persisted account state; derive membership from config-owned fields instead.saveAccountStateinpackages/opencode/src/tests/accounts.test.tscan prune runtime state for configured accountfb-1because it compares a normalized configured ID with the raw state ID, causing data loss; normalize both sides before pruning.cleanupTempConfigDirsinpackages/opencode/src/tests/index.test.tscan delete temporary config directories and clearfallbackRefresheswhile a timed-out background refresh still runs, allowing the refresh to operate against removed state; wait for or explicitly cancel the refresh before cleanup.
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="packages/opencode/src/tests/accounts.test.ts">
<violation number="1" location="packages/opencode/src/tests/accounts.test.ts:251">
P2: This test codifies data loss: config still lists account ' fb-1 ' but the scoped save pruned its runtime state to {}. The pruning in saveAccountState compares the normalized configured id ('fb-1') against the raw state key (' fb-1 '), so a still-listed account's credentials/refresh token are deleted whenever the saving snapshot omits that account. That contradicts the PR's stated 'not pruned when still listed' invariant; either the test expectation or the normalization/pruning is wrong.</violation>
<violation number="2" location="packages/opencode/src/tests/accounts.test.ts:1103">
P2: The 'does not establish membership from config-owned fields on an incoming api account' test expects state to keep both 'a' and 'b', but saveAccountState's configuredIds computation merges the incoming runtime account (apiKey/baseURL) into the config-only entry before normalizeAccount, so 'a' establishes membership and the prune loop deletes 'b'. The test's expectation and the implementation disagree — either this test fails CI or the membership/prune logic silently removes a still-configured account. Confirm which and reconcile (membership should come from the loader accepting the config entry on its own, per the PR notes).</violation>
</file>
<file name="packages/opencode/src/tests/index.test.ts">
<violation number="1" location="packages/opencode/src/tests/index.test.ts:138">
P2: When the 4s cap of `Promise.race` is exceeded, `cleanupTempConfigDirs` clears `fallbackRefreshes` and deletes the temp config dirs even though a background refresh is still running. That refresh was added to the set precisely because it may outlive the test; abandoning it after a fixed cap means a late-settling refresh can still write rotated auth/quota state into a path whose owner-directory was just removed (ENOENT) or into env points the next test now owns, reintroducing the cross-test leakage this PR is meant to eliminate. Instead of clearing the set and tearing down while a refresh is pending, await the actual refresh promises (or at least skip directory removal and `fallbackRefreshes.clear()` while any are unresolved) so the drain guarantee is honored.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const state = JSON.parse( | ||
| await readFile(getAccountStatePath(accountPath), 'utf8'), | ||
| ) | ||
| expect(state.accounts).toEqual({}) |
There was a problem hiding this comment.
P2: This test codifies data loss: config still lists account ' fb-1 ' but the scoped save pruned its runtime state to {}. The pruning in saveAccountState compares the normalized configured id ('fb-1') against the raw state key (' fb-1 '), so a still-listed account's credentials/refresh token are deleted whenever the saving snapshot omits that account. That contradicts the PR's stated 'not pruned when still listed' invariant; either the test expectation or the normalization/pruning is wrong.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/accounts.test.ts, line 251:
<comment>This test codifies data loss: config still lists account ' fb-1 ' but the scoped save pruned its runtime state to {}. The pruning in saveAccountState compares the normalized configured id ('fb-1') against the raw state key (' fb-1 '), so a still-listed account's credentials/refresh token are deleted whenever the saving snapshot omits that account. That contradicts the PR's stated 'not pruned when still listed' invariant; either the test expectation or the normalization/pruning is wrong.</comment>
<file context>
@@ -212,6 +212,78 @@ describe('main account identity', () => {
+ const state = JSON.parse(
+ await readFile(getAccountStatePath(accountPath), 'utf8'),
+ )
+ expect(state.accounts).toEqual({})
+ })
+
</file context>
There was a problem hiding this comment.
Confirmed and fixed — this was a real data-loss path, the best finding on this PR. Membership kept only trimmed ids while the loader reads state by the raw config key, so a padded-id account the loader happily serves lost its state to any scoped save. Membership now keeps BOTH key forms of each accepted entry (counted as one entry for the every-entry-parseable check), and the test at ~251 flipped to expect survival, asserting loadAccounts serves the credential before AND after the scoped save. Mutation to trimmed-only membership reddens it.
| await saveAccountState(staleStorage!, accountPath, { accounts: true }) | ||
| const state = JSON.parse(await readFile(getAccountStatePath(), 'utf8')) | ||
| expect(state.accounts).toHaveProperty('a') | ||
| expect(state.accounts).toHaveProperty('b') |
There was a problem hiding this comment.
P2: The 'does not establish membership from config-owned fields on an incoming api account' test expects state to keep both 'a' and 'b', but saveAccountState's configuredIds computation merges the incoming runtime account (apiKey/baseURL) into the config-only entry before normalizeAccount, so 'a' establishes membership and the prune loop deletes 'b'. The test's expectation and the implementation disagree — either this test fails CI or the membership/prune logic silently removes a still-configured account. Confirm which and reconcile (membership should come from the loader accepting the config entry on its own, per the PR notes).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/accounts.test.ts, line 1103:
<comment>The 'does not establish membership from config-owned fields on an incoming api account' test expects state to keep both 'a' and 'b', but saveAccountState's configuredIds computation merges the incoming runtime account (apiKey/baseURL) into the config-only entry before normalizeAccount, so 'a' establishes membership and the prune loop deletes 'b'. The test's expectation and the implementation disagree — either this test fails CI or the membership/prune logic silently removes a still-configured account. Confirm which and reconcile (membership should come from the loader accepting the config entry on its own, per the PR notes).</comment>
<file context>
@@ -992,6 +1064,274 @@ describe('account storage', () => {
+ await saveAccountState(staleStorage!, accountPath, { accounts: true })
+ const state = JSON.parse(await readFile(getAccountStatePath(), 'utf8'))
+ expect(state.accounts).toHaveProperty('a')
+ expect(state.accounts).toHaveProperty('b')
+ expect(state.accounts.a.apiKey).toBe('api-key')
+ expect(state.accounts.b.refresh).toBe('refresh-b')
</file context>
There was a problem hiding this comment.
Half-refuted with the field list: the membership path projects incoming accounts through accountRuntimeState, and for api accounts that projection is exactly {apiKey, lastUsed} — baseURL and authHeader are excluded. So the config-only entry never receives baseURL from the incoming account, normalizeAccount rejects it, membership stays unknown, and the test passes for precisely its stated reason (it is green on this head, which the 'fails CI' branch of the finding already contradicted). No change.
| }) | ||
|
|
||
| async function cleanupTempConfigDirs() { | ||
| await Promise.race([Promise.allSettled(fallbackRefreshes), Bun.sleep(4_000)]) |
There was a problem hiding this comment.
P2: When the 4s cap of Promise.race is exceeded, cleanupTempConfigDirs clears fallbackRefreshes and deletes the temp config dirs even though a background refresh is still running. That refresh was added to the set precisely because it may outlive the test; abandoning it after a fixed cap means a late-settling refresh can still write rotated auth/quota state into a path whose owner-directory was just removed (ENOENT) or into env points the next test now owns, reintroducing the cross-test leakage this PR is meant to eliminate. Instead of clearing the set and tearing down while a refresh is pending, await the actual refresh promises (or at least skip directory removal and fallbackRefreshes.clear() while any are unresolved) so the drain guarantee is honored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/index.test.ts, line 138:
<comment>When the 4s cap of `Promise.race` is exceeded, `cleanupTempConfigDirs` clears `fallbackRefreshes` and deletes the temp config dirs even though a background refresh is still running. That refresh was added to the set precisely because it may outlive the test; abandoning it after a fixed cap means a late-settling refresh can still write rotated auth/quota state into a path whose owner-directory was just removed (ENOENT) or into env points the next test now owns, reintroducing the cross-test leakage this PR is meant to eliminate. Instead of clearing the set and tearing down while a refresh is pending, await the actual refresh promises (or at least skip directory removal and `fallbackRefreshes.clear()` while any are unresolved) so the drain guarantee is honored.</comment>
<file context>
@@ -99,9 +108,119 @@ function createMockClient(
+})
+
+async function cleanupTempConfigDirs() {
+ await Promise.race([Promise.allSettled(fallbackRefreshes), Bun.sleep(4_000)])
+ fallbackRefreshes.clear()
+ await drainSidebarWrites()
</file context>
There was a problem hiding this comment.
Fixed with the bound retained — an unbounded await reintroduces the suite-wedge this PR removed elsewhere. On the timeout path the set is no longer cleared: pending refresh promises carry forward to the next cleanup (and the afterAll), which awaits them again under its own bound. Dir removal stays (the afterAll ownership sweep owns recreations). Pinned with a deliberately-slow refresh that outlives one cleanup and is awaited by the next.
88b9864 to
72bfed3
Compare
There was a problem hiding this comment.
2 existing issues remain and 1 new issue found across 14 files (changes from recent commits).
Confidence score: 3/5
packages/core/src/accounts.tscan treat an API config withoutbaseURLas valid membership, copy the field from incoming storage, and prune unrelated state; require a validbaseURLbefore accepting the entry.packages/opencode/src/tests/info-logs.test.tsandpackages/opencode/src/tests/timer-tracking.tsmake exact timer-count assertions that depend on implementation details and shared mock state, so plugin initialization changes or multiple constructions can produce misleading failures; scope/reset the counters and assert only the intended behavior.
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="packages/opencode/src/tests/timer-tracking.ts">
<violation number="1" location="packages/opencode/src/tests/timer-tracking.ts:18">
P3: `disabledIntervalCalls` is a single counter shared by every mock `disabledPluginTimerOverrides()` creates, so constructing the plugin more than once in one test accumulates the count and breaks the exact `toBe(1)` assertions that rely on a single construction per test. Scope the counter per returned overrides set (or per construction) so the count reflects one plugin's scheduling rather than the whole test file's cumulative state.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return { | ||
| // Background intervals must not outlive the test-scoped fetch mock they captured. | ||
| setInterval: mock(() => { | ||
| disabledIntervalCalls += 1 |
There was a problem hiding this comment.
P3: disabledIntervalCalls is a single counter shared by every mock disabledPluginTimerOverrides() creates, so constructing the plugin more than once in one test accumulates the count and breaks the exact toBe(1) assertions that rely on a single construction per test. Scope the counter per returned overrides set (or per construction) so the count reflects one plugin's scheduling rather than the whole test file's cumulative state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/timer-tracking.ts, line 18:
<comment>`disabledIntervalCalls` is a single counter shared by every mock `disabledPluginTimerOverrides()` creates, so constructing the plugin more than once in one test accumulates the count and breaks the exact `toBe(1)` assertions that rely on a single construction per test. Scope the counter per returned overrides set (or per construction) so the count reflects one plugin's scheduling rather than the whole test file's cumulative state.</comment>
<file context>
@@ -0,0 +1,63 @@
+ return {
+ // Background intervals must not outlive the test-scoped fetch mock they captured.
+ setInterval: mock(() => {
+ disabledIntervalCalls += 1
+ return { unref() {} } as unknown as ReturnType<typeof setInterval>
+ }) as unknown as typeof setInterval,
</file context>
There was a problem hiding this comment.
Declining — every toBe(1) assertion lives inside a test that owns exactly one construction, asserted immediately after its own getPlugin() (info-logs even re-resets mid-test before constructing), and the counter is beforeEach-reset in all three files; no hook enforces it. A future test constructing two plugins would assert its own expected count — and if it asserted 1 anyway, the loud failure at that line names the bug. Scoping the counter per overrides-set would actually break the partial-override regression: it proves the underlay supplied the disabled setInterval by observing this counter, and it can hold no handle to a counter scoped inside the set that getPlugin constructs internally.
72bfed3 to
babd759
Compare
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 14 files (changes from recent commits).
Confidence score: 3/5
- In
packages/opencode/src/tests/index.test.ts,cleanupTempConfigDirscan delete temporary config directories while the fallback refresh is still running, allowing its pendingOPENCODE_ANTHROPIC_AUTH_FIL...write to race with cleanup; ensure the refresh has settled before deleting the directories.
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
babd759 to
a41a70d
Compare
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 14 files (changes from recent commits).
Confidence score: 4/5
- In
packages/opencode/src/tests/account-command.test.ts, unconditionalmock.restore()can hide an untagged fetch mock from setup.ts’s leak detection, weakening test reliability; remove or narrow the restore so the intended leak check still runs. - In
packages/opencode/src/tests/timer-tracking.ts, the duplicateddisabledPluginTimerOverridesalso remains inindex.test.ts, creating two sources of truth; import the shared helper fromindex.test.tsand remove the duplicate.
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="packages/opencode/src/tests/account-command.test.ts">
<violation number="1" location="packages/opencode/src/tests/account-command.test.ts:121">
P3: The unconditional `mock.restore()` here defeats the leak detection the comment above it describes. `mock.restore()` restores every bun `mock()` including an untagged fetch mock, so it never reaches setup.ts's leak check (which only fires when fetch is neither `guardedFetch` nor tagged) as the comment claims it must. For these tests to preserve the intended leak signal, restore only the tagged mock and drop or guard the global `mock.restore()` so an untagged `mock()`-based fetch can't be silently cleaned up.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ), | ||
| ) | ||
| tempDirs.clear() | ||
| mock.restore() |
There was a problem hiding this comment.
P3: The unconditional mock.restore() here defeats the leak detection the comment above it describes. mock.restore() restores every bun mock() including an untagged fetch mock, so it never reaches setup.ts's leak check (which only fires when fetch is neither guardedFetch nor tagged) as the comment claims it must. For these tests to preserve the intended leak signal, restore only the tagged mock and drop or guard the global mock.restore() so an untagged mock()-based fetch can't be silently cleaned up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/account-command.test.ts, line 121:
<comment>The unconditional `mock.restore()` here defeats the leak detection the comment above it describes. `mock.restore()` restores every bun `mock()` including an untagged fetch mock, so it never reaches setup.ts's leak check (which only fires when fetch is neither `guardedFetch` nor tagged) as the comment claims it must. For these tests to preserve the intended leak signal, restore only the tagged mock and drop or guard the global `mock.restore()` so an untagged `mock()`-based fetch can't be silently cleaned up.</comment>
<file context>
@@ -68,15 +90,48 @@ const baseStorage = (): AccountStorage => ({
+ ),
+ )
+ tempDirs.clear()
+ mock.restore()
+ } finally {
+ // Assert last so a detected leak cannot abort the cleanup above.
</file context>
There was a problem hiding this comment.
Refuted by probe on this head: Bun 1.3.14's mock.restore() restores spyOn-created mocks only — a plain mock() ASSIGNED to globalThis.fetch survives it (observed: the assignment is still installed afterward), so an untagged assigned mock does reach the preload's detector; the comment's claim holds. The variant that WOULD be masked is spyOn(globalThis, 'fetch'), and no test in these files spyOns fetch (grepped). The unconditional mock.restore() stays: it is what resets call counts and non-fetch mocks between tests.
a41a70d to
68d5461
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
68d5461 to
fe78907
Compare
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Confidence score: 5/5
- In
packages/opencode/src/tests/add-account-flows.test.ts,does not retain a background interval unless the helper opts inhardcodes onesetIntervalcall and one active interval at startup, which could make the test brittle if initialization changes without affecting the intended behavior—consider asserting the opt-in/retention outcome directly.
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="packages/opencode/src/tests/add-account-flows.test.ts">
<violation number="1" location="packages/opencode/src/tests/add-account-flows.test.ts:159">
P3: The new test 'does not retain a background interval unless the helper opts in' hardcodes the plugin calling setInterval exactly once at startup (`disabledIntervalCalls` === 1, then `activeIntervals.size` === 1). This couples the test to the plugin's internal interval count: if background scheduling ever starts two intervals, both assertions fail spuriously even though the leak behavior being tested is unchanged. The test should assert the invariant it actually cares about (no interval survives teardown) rather than the exact call count.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| test('does not retain a background interval unless the helper opts in', async () => { | ||
| await getPlugin() | ||
| expect(timerTracking.disabledIntervalCalls).toBe(1) |
There was a problem hiding this comment.
P3: The new test 'does not retain a background interval unless the helper opts in' hardcodes the plugin calling setInterval exactly once at startup (disabledIntervalCalls === 1, then activeIntervals.size === 1). This couples the test to the plugin's internal interval count: if background scheduling ever starts two intervals, both assertions fail spuriously even though the leak behavior being tested is unchanged. The test should assert the invariant it actually cares about (no interval survives teardown) rather than the exact call count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/add-account-flows.test.ts, line 159:
<comment>The new test 'does not retain a background interval unless the helper opts in' hardcodes the plugin calling setInterval exactly once at startup (`disabledIntervalCalls` === 1, then `activeIntervals.size` === 1). This couples the test to the plugin's internal interval count: if background scheduling ever starts two intervals, both assertions fail spuriously even though the leak behavior being tested is unchanged. The test should assert the invariant it actually cares about (no interval survives teardown) rather than the exact call count.</comment>
<file context>
@@ -107,13 +132,54 @@ beforeEach(async () => {
+
+test('does not retain a background interval unless the helper opts in', async () => {
+ await getPlugin()
+ expect(timerTracking.disabledIntervalCalls).toBe(1)
+ expect(activeIntervals.size).toBe(0)
+
</file context>
| expect(timerTracking.disabledIntervalCalls).toBe(1) | |
| expect(timerTracking.disabledIntervalCalls).toBeGreaterThan(0) |
There was a problem hiding this comment.
Declining, consistent with the same ruling made twice on this PR (the account-command and info-logs equivalents): the exact count is a deliberate wiring pin, not incidental coupling. The invariant-only assertion (no interval survives teardown) is already present in the same test; toBe(1) additionally pins that plugin construction schedules exactly one background interval, so a second interval appearing is a loud, named signal at the wiring change — which is when we want to decide whether it was intended — rather than a silent behavioral drift the leak assertion alone would absorb.
fe78907 to
98d441d
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
98d441d to
11187c7
Compare
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 14 files (changes from recent commits).
Confidence score: 4/5
- In
packages/opencode/src/tests/network-guard-utils.ts, the!lowHextet/!highHextetchecks treat the valid string"0"as missing, so mapped loopback addresses such as[::ffff:7f00:0]can be incorrectly blocked as non-loopback; distinguish absent values from zero and add a regression test.
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
11187c7 to
b23d84b
Compare
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 14 files (changes from recent commits).
Confidence score: 5/5
packages/opencode/src/tests/accounts.test.tspreserves both padded and trimmed account keys after a scoped save, which can leave duplicated or stale credential state; update the save behavior or test fixture to remove the obsolete key.packages/opencode/src/tests/timer-tracking.tsduplicatesdisabledPluginTimerOverridesandPluginTimerOverridesfromindex.test.ts, creating maintenance and drift risk; import and reuse the existing definitions.
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="packages/opencode/src/tests/accounts.test.ts">
<violation number="1" location="packages/opencode/src/tests/accounts.test.ts:1474">
P3: This test enshrines duplicated credential state: after a scoped save, accounts[' fb-1 '] and accounts['fb-1'] both hold the same access/refresh data. The padded key is never dropped when the trimmed key is written, so a single account's credentials persist under two state keys. Reconcile the padded form (delete the raw-key entry when the trimmed key is present) and update the test to expect only the trimmed key.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const state = JSON.parse( | ||
| await readFile(getAccountStatePath(accountPath), 'utf8'), | ||
| ) as { accounts?: Record<string, { refresh?: string }> } | ||
| expect(Object.keys(state.accounts ?? {}).sort()).toEqual([ |
There was a problem hiding this comment.
P3: This test enshrines duplicated credential state: after a scoped save, accounts[' fb-1 '] and accounts['fb-1'] both hold the same access/refresh data. The padded key is never dropped when the trimmed key is written, so a single account's credentials persist under two state keys. Reconcile the padded form (delete the raw-key entry when the trimmed key is present) and update the test to expect only the trimmed key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/accounts.test.ts, line 1474:
<comment>This test enshrines duplicated credential state: after a scoped save, accounts[' fb-1 '] and accounts['fb-1'] both hold the same access/refresh data. The padded key is never dropped when the trimmed key is written, so a single account's credentials persist under two state keys. Reconcile the padded form (delete the raw-key entry when the trimmed key is present) and update the test to expect only the trimmed key.</comment>
<file context>
@@ -1039,6 +1391,280 @@ describe('account storage', () => {
+ const state = JSON.parse(
+ await readFile(getAccountStatePath(accountPath), 'utf8'),
+ ) as { accounts?: Record<string, { refresh?: string }> }
+ expect(Object.keys(state.accounts ?? {}).sort()).toEqual([
+ ' fb-1 ',
+ 'fb-1',
</file context>
There was a problem hiding this comment.
Fixed — but the naive delete-the-raw-key direction would have broken loading: the loader read state by the RAW config id, so the padded entry was load-bearing for a padded config id. Probing the real path first surfaced the deeper defect your finding brushed against: runtime saves (token rotation) write the TRIMMED key, which the old loader never read for a padded config entry — rotated credentials were persisted where loading couldn't see them (probe: second load served refresh-old). The landed fix canonicalizes the trimmed key on save (superseded padded variants are dropped), teaches the loader a raw-then-trimmed fallback for legacy state, and keeps the survival property green. Post-save state now holds exactly one key, the test expects trimmed-only, and rotation round-trips (probe now serves refresh-new). Three mutations pin it: loader fallback removed (3 red), save reconciliation removed (2 red), old raw-only loader restored (rotation probe red).
b23d84b to
f33b8f1
Compare
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 14 files (changes from recent commits).
Confidence score: 4/5
packages/opencode/src/tests/accounts.test.tsasserts that persisted accountbremains even though the implementation does not establish that outcome for the provided config and storage state; clarify the intended behavior and align the test or implementation accordingly.packages/opencode/src/tests/setup.tschecks the redirect limit only after issuing the next loopback-validated fetch, allowing one extra request before failure; move the counter check before the fetch.packages/opencode/src/tests/network-guard.test.tshas a self-referential ordering comment that can mislead future maintenance; update it to name the preceding sibling test.
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="packages/opencode/src/tests/network-guard.test.ts">
<violation number="1" location="packages/opencode/src/tests/network-guard.test.ts:499">
P3: The comment on line 499 claims this test "deliberately runs before" a test named identically to itself, which is self-referential and confusing. It should name the sibling test that actually precedes it ('restores the guard after a test leaves the default mock installed') and clarify the relationship, since the restoration is enforced by the shared afterEach hook in setup.ts rather than by ordering.</violation>
</file>
<file name="packages/opencode/src/tests/setup.ts">
<violation number="1" location="packages/opencode/src/tests/setup.ts:91">
P3: The redirect cap is checked after the next fetch is already issued, so a chain exceeding MAX_REDIRECTS sends one extra (loopback-validated) request before throwing. Check the counter before issuing the fetch at the top of the loop so the guard aborts at exactly the cap.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ).toBe(true) | ||
| }) | ||
|
|
||
| // Deliberately runs before "restores the network guard after a default fetch mock is left behind". |
There was a problem hiding this comment.
P3: The comment on line 499 claims this test "deliberately runs before" a test named identically to itself, which is self-referential and confusing. It should name the sibling test that actually precedes it ('restores the guard after a test leaves the default mock installed') and clarify the relationship, since the restoration is enforced by the shared afterEach hook in setup.ts rather than by ordering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/network-guard.test.ts, line 499:
<comment>The comment on line 499 claims this test "deliberately runs before" a test named identically to itself, which is self-referential and confusing. It should name the sibling test that actually precedes it ('restores the guard after a test leaves the default mock installed') and clarify the relationship, since the restoration is enforced by the shared afterEach hook in setup.ts rather than by ordering.</comment>
<file context>
@@ -0,0 +1,505 @@
+ ).toBe(true)
+ })
+
+ // Deliberately runs before "restores the network guard after a default fetch mock is left behind".
+ test('restores the network guard after a default fetch mock is left behind', async () => {
+ await expect(globalThis.fetch(MESSAGES_URL)).rejects.toThrow(
</file context>
| // Deliberately runs before "restores the network guard after a default fetch mock is left behind". | |
| // Deliberately runs after "restores the guard after a test leaves the default mock installed"; | |
| // the afterEach hook in setup.ts must have restored the guarded fetch in between. |
| const location = response.headers.get('location') | ||
| if (!location) return response | ||
| await response.body?.cancel() | ||
| if (redirects >= MAX_REDIRECTS) { |
There was a problem hiding this comment.
P3: The redirect cap is checked after the next fetch is already issued, so a chain exceeding MAX_REDIRECTS sends one extra (loopback-validated) request before throwing. Check the counter before issuing the fetch at the top of the loop so the guard aborts at exactly the cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/setup.ts, line 91:
<comment>The redirect cap is checked after the next fetch is already issued, so a chain exceeding MAX_REDIRECTS sends one extra (loopback-validated) request before throwing. Check the counter before issuing the fetch at the top of the loop so the guard aborts at exactly the cap.</comment>
<file context>
@@ -1,9 +1,152 @@
+ const location = response.headers.get('location')
+ if (!location) return response
+ await response.body?.cancel()
+ if (redirects >= MAX_REDIRECTS) {
+ throw new Error(`Too many redirects while fetching ${request.url}`)
+ }
</file context>
0ef96fe to
4146786
Compare
ualtinok
left a comment
ualtinok
left a comment
There was a problem hiding this comment.
Two test blockers remain on current main with the repository's configured Bun 1.3.14:
network-guard.test.tsstill performs live fetches to127.0.0.2:1and127.1.2.3:1; both hang until the test timeout on Darwin. This test only needs to verify address classification, so it should call the pureassertLoopback()predicate directly rather than opening a connection.- After rebasing current
main, the Fable 5.1 effort test (injects mid-conversation effort markers...) leavesglobalThis.fetchmocked. The network guard correctly detects that leak. Please restore the mock infinally/afterEach, or avoid mutating the global.
I merge-tested the branch onto current main: 273 focused tests produced 271 passes and these two failures. The production network guard itself remains sound.
4146786 to
e48100f
Compare
|
Both fixed at e48100f (rebased onto cb282b0, single commit).
Your focused set: 275 pass / 0 fail. Root 1421/0 twice, e2e 30/0, typecheck/format/biome clean. #175 and #182 are restacked on this head (stack gate 1524/0). |
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 14 files (changes from recent commits).
Confidence score: 3/5
- In
packages/core/src/accounts.ts, the config-account validation helper can accept an account missing requiredbaseURLorrefreshwhen runtime state supplies the field during the later merge, allowing malformed configuration to establish membership; validate required fields against the config before merging runtime state.
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Fail-loud preload network guard for the test suite: any non-loopback HTTP(S) fetch throws, including across redirects (manual redirect following, 20-hop cap, unparseable URLs fail closed). Replaces 44 live vendor requests per run with deterministic local stubs, isolates background intervals and temp dirs, and asserts no test leaves globalThis.fetch or an interval behind. Production fixes found by the guard: account-state membership treats a missing or unparseable config as UNKNOWN (no pruning) rather than empty, loader/membership agree on whitespace-normalized ids, and rotated credentials persist under the canonical key. Loopback classification tests call assertLoopback() directly instead of opening sockets (127.0.0.2:1 hangs to timeout on Darwin), and the upstream Fable 5.1 effort test restores globalThis.fetch after itself.
e48100f to
fcbd9fc
Compare
There was a problem hiding this comment.
2 issues found across 14 files (changes from recent commits).
Confidence score: 4/5
- In
packages/opencode/src/tests/setup.ts, redirect responses without alocationheader can leave the response body undrained or uncancelled, potentially leaking resources or hanging real-server tests; cancel or otherwise consume the body before returning for this case. - In
packages/opencode/src/tests/account-command.test.ts, theafterAllcleanup is ineffective becauseafterEachhas already emptiedtempDirsand removed each temporary directory, leaving only redundant cleanup logic to simplify or verify.
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="packages/opencode/src/tests/account-command.test.ts">
<violation number="1" location="packages/opencode/src/tests/account-command.test.ts:128">
P3: `afterAll` is a no-op: the per-test `afterEach` already removes `tempDir` (via the `tempDirs` set it clears) with each rm individually caught, so by the time `afterAll` runs both `tempDirs` (always empty) and `tempDir` (already deleted) contain nothing to clean. Drop the redundant `afterAll` block, or if it is meant as a safety net for a teardown that never runs, document why.</violation>
</file>
<file name="packages/opencode/src/tests/setup.ts">
<violation number="1" location="packages/opencode/src/tests/setup.ts:108">
P3: A redirect status without a `location` header returns the 3xx response without draining or cancelling its body, since `response.body?.cancel()` is only reached after the location check. In real-server tests this leaves the response stream unconsumed. Cancel/drain the body before returning in the no-location branch too.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| }) | ||
|
|
||
| afterAll(async () => { |
There was a problem hiding this comment.
P3: afterAll is a no-op: the per-test afterEach already removes tempDir (via the tempDirs set it clears) with each rm individually caught, so by the time afterAll runs both tempDirs (always empty) and tempDir (already deleted) contain nothing to clean. Drop the redundant afterAll block, or if it is meant as a safety net for a teardown that never runs, document why.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/account-command.test.ts, line 128:
<comment>`afterAll` is a no-op: the per-test `afterEach` already removes `tempDir` (via the `tempDirs` set it clears) with each rm individually caught, so by the time `afterAll` runs both `tempDirs` (always empty) and `tempDir` (already deleted) contain nothing to clean. Drop the redundant `afterAll` block, or if it is meant as a safety net for a teardown that never runs, document why.</comment>
<file context>
@@ -68,15 +90,48 @@ const baseStorage = (): AccountStorage => ({
+ }
+})
+
+afterAll(async () => {
+ await Promise.all(
+ [...tempDirs, tempDir].map((directory) =>
</file context>
| ]) | ||
| if (!REDIRECT_STATUSES.has(response.status)) return response | ||
| const location = response.headers.get('location') | ||
| if (!location) return response |
There was a problem hiding this comment.
P3: A redirect status without a location header returns the 3xx response without draining or cancelling its body, since response.body?.cancel() is only reached after the location check. In real-server tests this leaves the response stream unconsumed. Cancel/drain the body before returning in the no-location branch too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/setup.ts, line 108:
<comment>A redirect status without a `location` header returns the 3xx response without draining or cancelling its body, since `response.body?.cancel()` is only reached after the location check. In real-server tests this leaves the response stream unconsumed. Cancel/drain the body before returning in the no-location branch too.</comment>
<file context>
@@ -1,9 +1,171 @@
+ ])
+ if (!REDIRECT_STATUSES.has(response.status)) return response
+ const location = response.headers.get('location')
+ if (!location) return response
+ await response.body?.cancel()
+ if (redirects >= MAX_REDIRECTS) {
</file context>
| if (!location) return response | |
| if (!location) { | |
| await response.body?.cancel() | |
| return response | |
| } |
ualtinok
left a comment
ualtinok
left a comment
There was a problem hiding this comment.
Reverified current head: the Darwin loopback tests now exercise the pure classifier, the Fable effort block restores fetch, redirect egress guards remain closed, and CI is green. The 3xx-without-Location response is terminal and must remain readable by the caller rather than being drained.
2 participants
The test suite has been making 44 real requests to vendor endpoints on every run:
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.fetchand 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 ownglobalThis.fetchis unaffected, which is how nearly every test here already works — the wrapper is a data property specifically so reassignment and Bun'sspyOnboth keep working.Three ways out of it, all closed after review found them:
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'spreconnectopens 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.Loopback means the whole
127.0.0.0/8block, not just127.0.0.1, so a test binding127.0.0.2to 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.comstays blocked.The stubs
A shared
test-fetch.tsanswers the four endpoints deterministically. The shapes match what the code actually needs rather than a blanket 200 — the token endpoint returns400 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
200success reddensdead (400 invalid_grant) fallback → needsReauth trueand 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/countwas 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 falsewent 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 permanentinvalid_grant, and the assertion loses.That test no longer answers
TOKEN_URLat 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, thegetPluginhelpers in all four files that have one disable background intervals by default, because unownedsetIntervalcallbacks 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:
afterEachassertsglobalThis.fetchidentity 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 Optional Claustrum vault custody for fallback OAuth accounts #175 that had never restored fetch.getPluginhelper, 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./tmp/anthropic-*directories (145 MB) — nearly every file thatmkdtemps 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
accountsarray 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: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 realloadAccountspath, 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:normalizeAccountdrops entries whose shape fails (anapientry with an invalidbaseURLreturns null). So[{id: 'a', type: 'api', baseURL: 'garbage'}]established known membership{a}and pruned every other account's refresh token — whileloadAccountson 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 mintedrandomUUID()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 (
persistPushedQuotafires 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 outsideremoveAccountPersistent— 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.
persistPushedQuotavalidated that the persisted quota snapshot belonged to the request's account, then separately adopted the top-levelmainQuotaCheckedAt— a field with no account binding of its own. Check one field, trust another that can disagree. Deterministic repro: quota identityaccount-aalongside top-level timestamp2_000_000publishesobserved_at_ms=2_000_000for a request whose own observation was1_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
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.invalidsentinels 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 to127.0.0.0/8didn't overshoot.Scope
packages/coreandpackages/e2e-testsare untouched. Core has no preload of its own, and the e2e harness talks to local mock servers on127.0.0.1which would pass the guard as written. Both are worth a follow-up; neither is widened here.relay-worker-miniflare.test.tsfails intermittently under suite load and passes solo. It's an unboundedawait mf.readywaiting on worker startup, not the websocket logic the test name suggests, and it reproduces on a clean upstream tree — filed as #176.Need help on this PR? Tag
@codesmith-botwith 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
preconnecttarget, fails closed on invalid URLs, and allows127.0.0.0/8,localhost, and[::1].State safety
checkedAttimestamp.relay-worker-miniflarewebsocket failure remains a pre-existing flake.Written for commit fcbd9fc. Summary will update on new commits.
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.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
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| GuardReviews (9): Last reviewed commit: "test(opencode): stop the suite reaching ..." | Re-trigger Greptile