e2e: session-todo spec fails on all three attempts within a run, then passes cleanly on later runs

Split out of #10 during PR #25, which is fixing the unit half. The e2e specs belong in their own issue: different subsystem, different tooling, and — importantly — a different failure shape that should not be treated as the same flake.

The shape, and why it is not the unit story

#10 groups the e2e failures with the unit timeouts. They do not behave alike.

The unit failures are wall-clock deadline hits: every one lands within ~400 ms of a 30-second bound, and the count varies run to run (3 → 1 → 0 → 2 across four runs of the same branch). That is contention.

The e2e failure is not shaped like that. On run 35719872063, one spec failed on its initial attempt and both Playwright retries:

regression-session-todo-...-ying-it-across-session-tabs-chromium/test-failed-1.png
regression-session-todo-...-chromium-retry1/test-failed-1.png
regression-session-todo-...-chromium-retry2/test-failed-1.png

Three consecutive failures inside one run is not a coin-flip. Then it passed cleanly on the next three runs (bae94a2d3, 2473ff53c, a95c2dfd3), with no change to packages/app between them.

So: reproducible within a run, not reproducible across runs. That reads like state established early in a run — ordering, a shared fixture, a leaked tab or server between specs — rather than a timing bound. A longer timeout is unlikely to help and would hide it.

Also named in #10, unverified here

packages/app/e2e/regression/session-timeline-transport.spec.ts › reconnects after a stream error, and a historical-diff-summary-overflow spec, both reported as Playwright toBeVisible timeouts at 10 s. Those may genuinely be the timing class. Worth checking whether they share the retry signature above before assuming either way.

Suggested direction

Get the Playwright trace/artifact from a failing run rather than the screenshot alone — the uploaded artifact (playwright-linux-<attempt>) has it, retained 7 days, so this wants doing while a failing run is recent.
Establish whether the failure depends on spec order: run the regression/ directory with --repeat-each and with a fixed vs. shuffled order.
Only then decide between a synchronisation fix and a timing one.

What #25 does and does not cover

#25 fixes the unit half: the harness now attributes a deadline kill instead of reporting exit -1, the umask assertion is gone, and deadlines are scaled from one place. It deliberately leaves e2e alone. #10 should not be closed until this issue is resolved too.

test(e2e): fix the duplicate sessionStatus key in the todo-dock spec

ci: gate on the lint rules where a hit is almost certainly a bug

test(e2e): sample the todo dock until a condition, not for a fixed wi…

test(e2e): scale the shared app-readiness and expect timeouts

test: make the environment-sensitive tests attribute their own failur…

fix(e2e): repair the import my own sweep broke in scroll-interaction

test(e2e): scale the per-call timeouts the shared ones did not cover

fix(e2e): repair the import my own sweep broke in scroll-interaction

test(e2e): scale the per-call timeouts the shared ones did not cover

fix(e2e): repair the import my own sweep broke in scroll-interaction

test(e2e): scale the per-call timeouts the shared ones did not cover (#…

Evidence that this is not a timeout-margin problem

PR #33 (run 35766763036) failed e2e on three specs. That branch changes only .github/workflows/test.yml (a step inside the unit job) and one digest in script/check-workflow-guards.ts — nothing reachable from e2e. Its base commit b3f269135 was green on swxtch 10 minutes earlier. So this is the #26 pattern, reproduced on a branch that cannot have caused it.

The important part: that branch already contains the 3× deadline scaling from #32, and two of the three failures blew through the scaled budget without getting close.

Spec	Failure	Budget it exhausted
`review-terminal-stacked.spec.ts`	`data-total-rows` expected `2773`, received `"8"` — 14 polls, value never moved	scaled `expect.timeout` = 30 s
`review-line-comment.spec.ts:48`	`toPass` predicate never satisfied (hover → `data-hovered` → focus)	`deadline(10_000)` = 30 s
`session-timeline-transport.spec.ts:52`	`toEqual` deep-equality mismatch, `+2` received	n/a — assertion, not a wait

8 is the first dataset in that spec; the assertion expects 2773 after a switch. The tree sat on the stale value for the entire scaled window and was still sitting on it when the budget expired. That is not a render that needed more time — the state transition never happened. And the third failure is a plain value mismatch with no waiting involved at all.

Correction to my earlier note on this issue: I said the scaled expect.timeout from #32 would likely help session-timeline-transport. It did not, and the mechanism above says it could not — that spec fails an equality assertion, not a deadline. Scaling was still the right fix for the CLI deadline kills it was aimed at, but it does not touch this class, and #26 should not be considered addressed by it.

What the shape points at

playwright.config.ts sets retries: 2 in CI, so a run gives each spec three attempts. The observed pattern — all three attempts fail together, then a later run on the same code passes cleanly — means the deciding variable is per-run, not per-attempt. Retries re-run the test but do not rebuild the app or restart the shared web server, so anything poisoned at run setup persists across all three attempts and looks exactly like this.

That makes the next step a run-level differential, not more timeout tuning:

compare the built app / dev-server state between a failing and a passing run (same commit if possible),
capture whether the three specs fail together every time, which would point at one shared cause rather than three,
keep trace: "retain-on-failure" artifacts from a failing run and read the stale-tree trace directly — the 14 polls should show whether the switch event was ever dispatched.

Leaving this open with the timing hypothesis explicitly ruled out.

Root-cause evidence from a failing run's artifacts

Run 35766763036 gave the differential this issue asked for: attempt 1 failed and attempt 2 passed on the same commit, and both artifact bundles are retained. Pulled playwright-linux-1 and read the traces.

The signature belongs to a different spec than this issue names

Attempt dirs from the failing run:

spec	attempts with `test-failed-1.png`	verdict
`review-line-comment.spec.ts:48` "shows a comment button when a line number is hovered"	initial + retry1 + retry2 — all three	genuine failure
`review-terminal-stacked.spec.ts:20`	initial only; retry1 holds a bare `trace.zip`	flaky, passed on retry
`session-timeline-transport.spec.ts:52`	initial only; retry1 holds a bare `trace.zip`	flaky, passed on retry
`new-session-panel` dark background	one dir, no failure screenshot	never surfaced in CI at all

So the "fails every attempt in a run, passes in later runs" shape reproduces on review-line-comment, not the session-todo spec this issue was opened about. The other two are ordinary single-attempt flake and should not be bundled with it.

It is not a timing bound

The inner failure is the toHaveAttribute inside the toPass block, not the hover():

Locator: locator('[data-component="session-review"]').locator('[data-column-number="1"]').last()
Expected: ""
  33 × locator resolved to <div data-line-index="0,0" data-column-number="1" data-line-type="context">…</div>
     - unexpected value "null"

The element resolves and is stable across 33 polls over 30 s, hover() succeeds every time (no interception error), and data-hovered is never set. More time cannot help this. That settles the question left open in the issue: the already-scaled 30 s budget is not the constraint.

The app is broken underneath it

All three failing attempts carry an identical error toast, and the two flaky specs carry an empty notifications region:

Failed to reload ReviewLineCommentRegression
Cannot read properties of undefined (reading 'all')

data-hovered is set by @pierre/diffs' InteractionManager on pointer move, which sets it whenever no line is currently hovered — so repeated hovers would normally work. It never firing, while the diff content renders fine, means the interaction layer never came up. The toast and the dead hover are the same failure.

The network log shows the scale of it: 26 fetches each of /api/provider, /api/model, /api/model/default (and again with location[directory]) inside one test. Something is in a refetch loop.

Where the `.all` throw can come from

All provider traffic is the v2 path /api/provider, all 200 — so bootstrap.ts:232-234's legacy branch did not run here. Ruled out by the trace rather than by reading.

Three unguarded .all reads remain candidates, and each is a latent defect independent of this bug:

global-sync/utils.ts:62 — normalizeProviderList does providers.all.map(...) guarded only by !Array.isArray(providers), so undefined falls straight through.
global-sync/child-store.ts:214 — providerQuery.data?.all.size === 0 && input.global.provider.all.size > 0. The optional chain covers data but neither data.all nor input.global.provider. Note this spec's mock returns provider: { all: [], … }, so data.all.size === 0 is true and the unguarded right-hand operand is always evaluated here.
global-sync/bootstrap.ts:234 — normalizeProviderList(result.data!), a non-null assertion on a network response. Not the culprit in this run, but it asserts away precisely this failure.

Also worth noting: server-sync.tsx:385 passes global: { provider: globalStore.provider } as a plain object literal, which snapshots the value once instead of tracking it reactively.

Next step

Reproduce locally against that spec to identify which access actually throws, rather than hardening all three and calling it fixed — blanket null-guards here would convert a hard failure into a silently degraded review panel, which is how this stayed invisible in the first place.

I have not fixed this yet; this comment is the evidence so the next person does not start from the screenshot again.

Root cause found, and two of my own earlier numbers withdrawn

The cause is a gap in the e2e mock server, not a timing bound. mock-server.ts has no /api/provider, /api/model or /api/model/default route, so those requests hit the catch-all returning {}. providers.data is then undefined, normalizeProviderList throws Cannot read properties of undefined (reading 'all'), and the review panel renders its diff while its interaction layer never comes up — hover() lands, data-hovered is never set. Fix in #37.

Measured: 1251 of 1251 responses to those three endpoints were 200 {}. Constant for every protocol: "v2" spec that boots a project.

Withdrawing two things I posted earlier

"The toast distinguishes the genuinely-failing spec from the flaky ones." Wrong, and it was selection bias — error-context.md only exists for failures, so the sample I compared could only contain failures. The toast is a constant, not the discriminating variable.
"28 of 88 specs were flaky in a local full run." Withdrawn. That run, and every local flake comparison I did, happened while this machine sat at load average 90 from other sessions. One 30-test run degraded to 7 tests in 17.4 minutes and exited non-zero. Those numbers describe the box, not the suite, and I should have checked load before quoting them as a property of the code.

What is still open on this issue

#37 fixes a real defect but is not established to fix the flakiness, and I am not claiming it does. The honest position:

The spec passes in isolation (5/5 in 2 minutes) and fails inside the full 99-spec run — so a per-run variable is real, as this issue proposed.
Whether that variable is only machine load, or load plus something in the suite, is unresolved. It cannot be resolved on this box.
CI on a dedicated runner is the only place a before/after flake comparison is meaningful.

So: let #37's own e2e run, and a few subsequent runs on swxtch, be the evidence. If the affected specs stop needing retries there, this issue closes on data. If they do not, the remaining work is a run-level differential in CI — not more local runs.

Also noted while reading the traces: review-terminal-stacked.spec.ts:20 and session-timeline-transport.spec.ts:52 failed once and passed on retry with no toast, so they are a different problem from this one and should not be closed by #37.

fix(e2e): satisfy the whole catalog contract, and test it

test(e2e): move the catalog test into the e2e project and run it

fix(e2e): keep Playwright from collecting the bun catalog test

fix(e2e): make the mock catalog a payload a real server could return

ci: stop gating merges on upstream's e2e suite

Closing as not planned — this is upstream's test harness, not this fork's responsibility.

packages/app/e2e belongs to anomalyco/opencode and they maintain it. This fork exists to carry the features we need and to stay cheaply resyncable with upstream. Investing here works against the second goal: 62 of our 108 diverged files are already CI/test infrastructure against only 46 product files, and every one of those is a conflict to re-resolve on each sync. #37 would have added five more inside packages/app/e2e/**.

Instead, .github/workflows/test.yml — a file already entirely ours — no longer gates merges on this suite. It still runs and still uploads artifacts, so the signal is kept; it just cannot block feature work any more.

The diagnosis is preserved, deliberately

It cost real effort and is worth exactly as much to whoever picks it up, here or upstream:

mock-server.ts has no /api/provider, /api/model or /api/model/default route, so those requests hit the catch-all returning {}.
providers.data is therefore undefined, and normalizeProviderList throws Cannot read properties of undefined (reading 'all') — measured at 1251 of 1251 responses, so it is a constant for affected specs, not a race.
The app catches it into a "Failed to reload" toast. The visible symptom is subtler: the review panel renders its diff while its interaction layer never comes up, so hover() lands and data-hovered is never set. That is why it looked like a timeout when the element was stable across 33 polls in 30 s.
Five protocol: "v2" specs that define models are affected.

A working fix existed and was verified before being closed on scope grounds — see #37 for the shape of it, including the payload-fidelity traps (time.released must be a number, status/enabled/limit.output/package are required) that make a naive fix pass tests over an impossible server response.

Also recorded, and also not ours

CI never typechecks the e2e project. typecheck.yml runs bun typecheck → turbo → tsgo -b, and the app's tsconfig includes only ./src, so typecheck:e2e is a script nothing executes. Real gap, upstream's to close.

Reopen if this suite ever starts gating something we care about.

Navigation Menu

Repository navigation

e2e: session-todo spec fails on all three attempts within a run, then passes cleanly on later runs #26

The shape, and why it is not the unit story

Also named in #10, unverified here

Suggested direction

What #25 does and does not cover

Evidence that this is not a timeout-margin problem

What the shape points at

Root-cause evidence from a failing run's artifacts

The signature belongs to a different spec than this issue names

It is not a timing bound

The app is broken underneath it

Where the `.all` throw can come from

Next step

Root cause found, and two of my own earlier numbers withdrawn

Withdrawing two things I posted earlier

What is still open on this issue

The diagnosis is preserved, deliberately

Also recorded, and also not ours

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

e2e: session-todo spec fails on all three attempts within a run, then passes cleanly on later runs #26

Description

The shape, and why it is not the unit story

Also named in #10, unverified here

Suggested direction

What #25 does and does not cover

Activity

brentyates-swx commented on Sep 22, 2026

Evidence that this is not a timeout-margin problem

What the shape points at

brentyates-swx commented on Sep 22, 2026

Root-cause evidence from a failing run's artifacts

The signature belongs to a different spec than this issue names

It is not a timing bound

The app is broken underneath it

Where the .all throw can come from

Next step

brentyates-swx commented on Sep 22, 2026

Root cause found, and two of my own earlier numbers withdrawn

Withdrawing two things I posted earlier

What is still open on this issue

brentyates-swx commented on Sep 23, 2026

The diagnosis is preserved, deliberately

Also recorded, and also not ours

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Where the `.all` throw can come from