[brentyates-swx]

Refs #10

Draft, claiming the work. Root causes established before writing any fix — and only one of the three groups is a timeout story.

A. The CLI trio — the harness hides its own timeout

packages/opencode/test/lib/cli-process.ts catches AppProcessError, which covers both a timeout and a spawn failure, and synthesizes:

{ exitCode: err.exitCode ?? -1, stderr: Buffer.from(...) }

So a run killed at its deadline fails as expected 0, received -1 — indistinguishable from a behavioural break unless someone reads stderr. That is precisely the cost #10 names: "every PR author has to re-derive whether their failure is real".

The three that failed in CI, all within ~400 ms of the 30-second deadline:

• unknown stream finish preserves partial output and continues
• --format json records an unknown stream finish and continuation
• --format json preserves reasoning, tool, and continuation ordering

The same file already fixed one instance of this, and its comment is the template:

Deliberately NOT a wall-clock assertion: the previous form bounded the duration by the same value as the harness kill, so a slow-but-healthy run under parallel load failed as if it had hung. Elapsed time is the harness timeout's job (a bounded backstop); the exit code is the real signal.

B. write.test.ts file permissions — umask, not flake

	umask	Bun.write yields
this box	0002	664
CI	0022	644

The test asserts exactly 0o644, and packages/opencode/src/tool/write.ts sets no mode at all. So it passes in CI by coincidence, and the test's own name — "sets file permissions when writing sensitive data" — claims an intent the code does not implement.

Forcing 0644 in the tool would be wrong, and this is the part worth flagging: it would loosen permissions for a user with a strict umask, where 0077 should yield 0600. The tool's behaviour is correct; the assertion is environment-dependent.

C. snapshot-tool-race — passes in isolation

Needs a real synchronisation point or isolation, not a longer deadline.

Deliberately out of scope, to be filed separately

The two packages/app e2e Playwright specs: different subsystem, different tooling, and e2e has now passed three consecutive runs after a single failure that took three attempts within one run to reproduce. That is a different shape from the unit timeouts and should not be lumped in with them.

🤖 Generated with Claude Code

https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU

[brentyates-swx]

Session stamp (claim):

• Session ID: c87b9b84-25b3-4804-a2ae-7e52148ee41f
• Working repo (launch cwd): /home/byates/.local/share/opencode
• Worktree for this PR: /home/byates/swx-opencode-flaky on fix/10-environment-sensitive-tests, based on origin/swxtch at 002a77ee0 (the ci: guard every inherited workflow to upstream, and enforce it in CI #22 merge)
• Identity: brent.yates@swxtch.io

Second P1 of the triage pass, picked up after #9 merged. No open PR overlaps: the three open PRs are dependabot bumps under packages/, none touching packages/opencode/test or packages/opencode/src/tool.

One crew at a time, so this is the only implementation stream in flight.

[brentyates-swx]

Progress: two of three groups fixed with established causes, one honestly unresolved

B. write.test.ts permissions — root cause proven, fixed

Not flaky at all. It asserted the umask:

umask 0022 (CI)      -> Bun.write yields 0644 -> pass
umask 0002 (dev box) -> Bun.write yields 0664 -> fail

Measured both ways on the same commit. src/tool/write.ts sets no mode, so the test's own name — "sets file permissions when writing sensitive data" — claimed an intent the code does not implement, and it passed in CI by coincidence.

Forcing a fixed mode would be the wrong fix and it is worth saying why: it would loosen permissions for anyone with a stricter umask, where 0o077 should yield 0o600. Restricting a file is the umask's job. What is worth pinning is that the tool leaves that decision alone — so the expectation is derived, 0o666 & ~process.umask(), and it now passes under umask 0002, 0022 and 0077.

Mutation-verified: adding chmod 0o600 to the tool fails it (expected 436, received 384).

A. The CLI trio — attribution first, deadlines second

#10's stated cost is "every PR author has to re-derive whether their failure is real". That is an attribution problem, not a timing one, so it is fixed first.

cli-process.ts caught AppProcessError — which covers a timeout and a spawn failure — and synthesized exitCode: -1. So a run killed at its deadline failed as expected 0, received -1: indistinguishable from a behavioural break unless someone read stderr.

RunResult now carries timedOut, using the same predicate core already uses for this distinction in packages/core/src/tool/bash.ts, and expectExit reports it. Forced a kill to verify:

opencode: the harness killed this run at its 1047ms deadline (deadlines scaled x0.02),
so the CLI never exited on its own. This is the timeout backstop, NOT a behavioural
failure: ... Raise OPENCODE_TEST_TIMEOUT_SCALE to confirm, and treat a behavioural
diagnosis as unproven until it fails with a real exit code.

Deadlines now come from one place (test/lib/deadline.ts), ×3 under CI, overridable. Keyed on CI rather than GITHUB_ACTIONS because the unit job sets GITHUB_ACTIONS=false explicitly — that would have been a silent no-op.

All 13 tests in the file pass locally.

Also fixed the last instance of the anti-pattern the file's own anomalyco#27371 comment warns about: the SIGINT test bounded durationMs by the same 30_000 as its own test budget, so the assertion could only fail by racing the thing that would have killed the test anyway.

C. snapshot-tool-race — my hypothesis did not survive measurement

I expected the 5-second poll budget to be the binding constraint. It is not. Instrumented and run three times in isolation:

MEASURE: polls=0 -> approx 0ms to first diff
MEASURE: polls=0 -> approx 0ms to first diff
MEASURE: polls=0 -> approx 0ms to first diff

The diff is present on the first poll. So raising that budget cannot be the fix for something that never waited.

What I have done is raise it anyway and make its exhaustion self-describing — the failure now distinguishes "the diff is empty" from "the diff had not been computed yet", and says which knob tells them apart. That is defensible hardening, but I am not claiming it fixes a known mechanism, and the commit message says so.

A full-package suite reproduction is running now, since #10 records this failing only in a full run and isolation demonstrably cannot reproduce it. I will report what it shows rather than closing this group on a guess.

One thing measuring it did settle: raising the poll budget alone made the test fail with this test timed out after 5000ms — the outer bun-test budget, which says nothing about what was being waited for. The poll budget has to sit inside the test budget, and both are now scaled together.

Out of scope, to be filed

The two packages/app e2e Playwright specs — different subsystem and a different failure shape (one failure needing three attempts within a single run, then three clean runs).

[brentyates-swx]

Evidence from two full-suite runs, and the e2e cause found

Following the note above: I said I would report what a full-suite reproduction showed rather than close group C on a guess. Here it is.

The attribution fix works on organic failures, not just a forced one

Two full runs of packages/opencode on this tree. Both had CLI subprocess failures, and both now report themselves:

the harness killed this run at its 30091ms deadline, so the CLI never exited on its
own. This is the timeout backstop, NOT a behavioural failure: the run was too slow,
most likely CPU contention from concurrent subprocess tests. Raise
OPENCODE_TEST_TIMEOUT_SCALE to confirm...

That is what expected 0, received -1 used to say. #10's stated cost — every author re-deriving whether a red job is real — is answered, and the proof is a real failure rather than one I induced. (Locally TIMEOUT_SCALE is 1, so the deadline stays 30 s and these still fail; a CI=1 run is in flight to show the ×3 scaling prevents them rather than merely explaining them.)

The suite is less stable than #10 says — 12 failures, then 4

Same tree, two consecutive runs:

run	failures
1	12 (incl. an unhandled decode failed error out of attention.test.ts)
2	4 — session.processor … aborted on cleanup, two CLI subprocess tests, opencode acp … stdin EOF exits cleanly

So the population is wider than the five #10 names, and two of run 2's four are in files #10 does not mention.

Group C: snapshot-tool-race did not reproduce, in either run

It passed in both full local runs, and in isolation the diff is present on the first poll. I cannot reproduce it, so I am not claiming a fix — the budget raise and the self-describing exhaustion stand as hardening that makes the next occurrence attributable. Stated plainly in the commit.

The e2e failure has a definite cause, found by a linter nothing runs

Rather than leaving this as #26, I looked. session-todo-dock-navigation.spec.ts — the exact spec that failed three attempts in one run — passes sessionStatus twice in the same mock config:

sessionStatus: { [sourceID]: { type: "busy" } },   // discarded
...
sessionStatus: () => sessionStatus,                // wins

JavaScript keeps the last key. So the first never had any effect, the intended "source session starts busy" was silently lost, and the mock read a getter over a map that started empty — leaving the test dependent entirely on a pushed SSE event arriving, with no initial-state fallback. That is a plausible mechanism for a failure that reproduces within a run and not across runs.

Confirmed by the repository's own linter, and it is the only such warning in the whole e2e directory:

session-todo-dock-navigation.spec.ts:60:5  eslint(no-dupe-keys): Duplicate key 'sessionStatus'

Fixed so both intents survive. What I am not claiming is that it is the whole cause: the spec also asserts animation by sampling opacity during fixed windows and requiring a sample to land mid-transition, which is a race between sampler and animation. That is a candidate too and is not yet measured.

The class, not just the instance

bun run lint exists and no workflow runs it — I checked. Worse, no-dupe-keys is only a warning in .oxlintrc.json, so even running bun run lint would have exited 0 on this bug.

I am testing a surgical gate: oxlint with a dozen rules promoted to errors where a hit is almost certainly a bug (no-dupe-keys, no-dupe-args, no-duplicate-case, no-self-compare, use-isnan, getter-return, no-constant-binary-expression, and similar), rather than --deny-warnings, which would fail on hundreds of pre-existing style warnings. If it is green repo-wide it goes in this PR; anything it catches gets fixed here rather than filed.

[brentyates-swx]

The lint gate is in, and a correction on my own e2e verification

Fixed rather than filed

Per the instruction to close out rather than grow the backlog, three things that would have been issues are fixed in this PR instead:

1. No workflow runs lint. Verified by grepping .github/workflows.
2. Worse: bun run lint would have exited 0 anyway. .oxlintrc.json sets almost everything to warn, including no-dupe-keys — so even wiring up the existing script would not have caught the duplicate key that produced the failing e2e spec.
3. A pre-existing TS2309 in packages/web/src/types/lang-map.d.ts (export interface alongside export = map) which would have made the new gate land red. The interface is map()'s return type and referenced nowhere else, so dropping its export resolves it and consumers still get it structurally. A gate that lands red is not a gate.

.oxlintrc.gate.json promotes thirteen rules to errors — duplicate keys, class members, else-if branches, switch cases; self-assignment and self-comparison; unsafe negation; isnan misuse; constant binary expressions; a getter or setter that does not return; assigning to an import; unreachable code. Not --deny-warnings, which would fail on hundreds of pre-existing style warnings and be switched off within a week.

Cost mattered here: the repository's type-aware oxlint config needs more than 6 GiB on this monorepo and was OOM-killed at that cap during this work. The gate runs with typeAware: false — ~2 seconds, 0.25 GiB — and every rule in it is syntactic, so nothing is lost. Invoked via a lint:correctness script so CI uses the pinned oxlint devDependency (1.60.0) rather than whatever bunx fetches that day.

Mutation-verified through the exact CI command: reintroducing the duplicate key makes bun run lint:correctness exit 1; the clean tree exits 0.

Incidentally, adding a step to an allowlisted job made #22's own digest pin demand an acknowledgement on this unrelated PR. That is it working as intended, and it is the first time it has fired outside its own PR.

Correction: my e2e verification measured nothing

I ran the fixed spec with --repeat-each=5 and got 5 of 5 failed. Before treating that as a signal I looked at the failure:

Error: browserType.launch: Executable doesn't exist at
/home/byates/.cache/ms-playwright/chromium_headless_shell-1217/chrome-linux/headless_shell

The repository pins @playwright/test 1.59.1, which wants chromium build 1217; this box has 1208 and 1223 from other projects. So all five failures were a missing browser, not the test — my change is neither vindicated nor implicated by that run.

Recording it because "5 of 5 failed" is exactly the shape of result I would have been tempted to report, and it would have been wrong in both directions: it neither proves the duplicate-key fix works nor that it broke anything. Installing the right build and re-running.

This is the third time in this session that a measurement rather than a code change was the thing at fault — the others being a mutation whose sed silently did not apply, and a reviewer reporting a bypass from cmd | tail; echo $?, which reports tail's status.

[brentyates-swx]

Measured: the scaling prevents the CLI timeouts, and my e2e hypothesis was wrong

Group A proven, not just attributed

Three full runs of packages/opencode on this tree:

profile	failures	deadline kills
local, TIMEOUT_SCALE = 1	12, then 4	2 per run
CI=1, TIMEOUT_SCALE = 3	1	0

So the ×3 scaling removes the CLI subprocess timeouts rather than merely explaining them, and the attribution message is there for the case where it is not enough. The one remaining failure under the CI profile is running task tool preserves metadata after tool-call transition — a different test, not in #10's list, and not a deadline kill. I will look at that next rather than leave it.

My e2e diagnosis was wrong, and the control is what showed it

I said the duplicate sessionStatus key was "a plausible mechanism" and explicitly did not claim it was the whole cause. It turns out it is not the cause at all.

With chromium 1217 actually installed, I ran the spec five times on each version:

version	result
base (duplicate key present)	1 failed, 4 passed
with my duplicate-key fix	5 passed

The first local reproduction of this flake. But the base failure is at line 84, and it is not the SSE path I hypothesised:

> 84 | expect((await opening).some((sample) => sample.opacity > 0.05 && sample.opacity < 0.95)).toBe(true)
      Expected: true
      Received: false

That is the sampling assertion — it requires a sample to land inside a narrow opacity band while a spring animation runs. A race between the sampler and the animation, with no bound on either.

And my 5-of-5 pass proves nothing: at the measured 1-in-5 base rate, five clean runs happen by chance about a third of the time. I am not counting that as evidence.

The actual fix

The opacity is not a CSS transition — it is a reactive style, opacity: 1 - shut(), driven by useSpring through a dockProgress prop. So there is no transitionrun or animationstart to observe, and my first instinct (Web Animations API) does not apply.

The assertions now check the ramp instead of an instant, over present-only samples:

• opening: min opacity < 0.95 (it was not already opaque) and max > 0.98 (it finished opaque)
• closing: min < 0.95 (it faded rather than vanishing in one frame)

Present-only matters: the sampler reports opacity 0 for an absent dock, and counting those is what forced the arbitrary > 0.05 floor originally. The regression's intent — animates on open, does not replay on return — is preserved; what is dropped is the requirement to catch a specific moment.

Running it 10× to compare against the measured 1-in-5 base rate.

Still standing

The duplicate-key fix stays. It is a real defect confirmed by the repository's own linter, it silently discarded the intended initial state, and the lint gate now prevents its class. It is simply not what made this spec fail.

[brentyates-swx]

My first e2e fix made it worse — 4 failures in 10, against a base of 1 in 5

Measuring it caught this, so recording it before the corrected version.

Error: opening: dock never reached full opacity
  Expected: > 0.98
  Received:   0.878927

I added max > 0.98 as a "it finished opaque" check. The spring has not settled when the 1000 ms sampling window closes — it is at 0.879 — so that is simply not observable here, and requiring it doubled the failure rate. Entirely my own invention; the original assertion never claimed it.

Direction is what is actually checkable, so the assertion is now: it started faint (min < 0.95) and rose (last > first). Same intent, nothing asserted that cannot be observed.

The 10× run also exposed a second, pre-existing race

Separate line, not mine:

> 94 | expect(openSamples.length).toBeGreaterThan(0)
       Expected: > 0
       Received:   0

The returningOpen window is 700 ms, started before switching tabs. Sometimes it closes before the dock reappears, so there are zero present samples and the assertion fails on an empty array rather than on anything about the dock. That is the same defect class as the one I fixed in the unit harness: a fixed wall-clock window tuned on a fast machine.

Raised to 2 500 ms via a named constant. Only present samples are read, so a longer window cannot weaken the assertion — it just stops the window closing before the thing it exists to observe.

What this says about the original diagnosis

Worth being explicit, since I got this wrong twice: this spec had three independent flake sources, and the duplicate sessionStatus key was none of them.

1. the opening opacity assertion requiring a sample inside a narrow band — the one CI hit
2. the returning window being too short to contain the dock reappearing
3. (mine, briefly) a settle threshold the spring does not reach in the window

The duplicate key is still a genuine defect worth the fix and the lint gate — it silently discarded the intended initial state — but I should not have offered it as a candidate cause without measuring first. The control run is what corrected me, and it only existed because I ran the unmodified spec alongside the fixed one.

Re-running 10× on the corrected version.

[brentyates-swx]

Condition-driven sampling works — and the confirmation run found a fourth cause

Measured, 20 repeats, and not zero

version	failures
base (unmodified)	2 in 10
attempt 1 — max > 0.98	4 in 10
attempt 2 — longer window	2 in 10
condition-driven	0 in 10, then 1 in 10 under load

I am not reporting the 10/10 alone. A confirmation run on a loaded box gave 9/10, and the honest figure is 1 failure in 20 against a base of 2 in 10 — roughly a 4× improvement, not elimination.

No failure at any of the three dock assertions across all 20 runs. The remaining one is somewhere else entirely:

expect(locator).toBeVisible() failed
Locator: getByRole('heading', { name: 'Todo dock animation' })
Timeout: 30000ms
at utils/waits.ts:6

That is APP_READY_TIMEOUT — the app shell not rendering within 30 s, which asserts nothing about what this spec tests. The same ten repeats took 9.6 minutes against 4.2 for identical work, so the box was about 2.3× slower.

A fourth distinct flake source in one spec, same class as the other three.

Fixed at the shared level, not in this spec

APP_READY_TIMEOUT is used across 34 e2e files, so scaling it reaches the whole suite rather than the spec that exposed it. The config's expect.timeout is scaled too — and that matters, because 10 s is exactly the bound #10 records the other two named e2e specs failing on (session-timeline-transport › reconnects after a stream error, and the historical-diff spec). Those are Playwright toBeVisible timeouts: a statement about machine speed, not about the UI.

One knob across both suites — OPENCODE_TEST_TIMEOUT_SCALE, keyed on CI since the unit job sets GITHUB_ACTIONS=false. Verified wired: ×3 gives 90 s app-ready and 30 s expect; unscaled stays 30 s and 10 s.

What the sampler does now

The opacity is a reactive style driven by useSpring, not a CSS transition, so there is no transitionrun to observe. Sampling stays, but sampleDock(page, "appears" | "opaque" | "absent") runs until the thing being waited for has happened, then a few frames past it so the caller sees the settled state — time as a bounded backstop, the same doctrine as the unit harness change in this PR.

The absence check deliberately keeps a bounded window, because absence is a claim about an interval rather than an event to wait for. The "appears" condition still applies to it, so a dock that wrongly returns ends sampling immediately and fails rather than costing the full budget.

Tally for this one spec

Four independent flake sources, and the duplicate sessionStatus key — my original hypothesis — was none of them:

1. the opening assertion requiring a sample inside a narrow opacity band (the one CI hit)
2. a 700 ms return window that could close before the dock reappeared
3. a settle threshold I invented that the spring never reaches (0.879 measured; doubled the failure rate)
4. a 30 s app-readiness wait, shared across 34 files

Each one needed its own measurement to find. Three of the four were only visible because I kept running the unmodified spec as a control alongside the fixed one.