test(cli): yield the event loop between base-tree tests - #12573

Independent Critical-only review — head 021e9222

Test-infrastructure only: nine added lines in packages/cli/src/commands/review/base-tree.test.ts, no production code and no assertion changed.

The change does what it claims

The suite drives real git through spawnSync/execFileSync, which blocks synchronously, so the worker's event loop never turns for the duration of the file. Vitest's worker→main onTaskUpdate RPC then exceeds its timeout and the run exits non-zero with every test green — a false CI failure, which is the right thing to fix.

The remedy yields one loop turn per test:

const realSetImmediate = setImmediate;
beforeEach(() => new Promise<void>((resolve) => realSetImmediate(resolve)));

Three properties make this correct rather than merely plausible:

• The primitive is captured at module load. const realSetImmediate = setImmediate executes at import time, before any test body can install fake timers, so the yield cannot be swallowed by a mocked timer queue. This file does not currently call vi.useFakeTimers, so the capture is defensive rather than load-bearing today — but it is what keeps the hook working if a later test in this file fakes timers, and it costs nothing.
• beforeEach is genuinely in scope. It is imported from vitest at line 18 alongside describe, it, expect, afterEach and vi, so the file-level hook cannot fail with a ReferenceError that would take the whole suite down.
• Hook order is right. The file-level hook at :65 is registered outside any describe, so vitest runs it before the suite's own beforeEach at :201 that creates the base tree and acquires the worktree lease. The yield therefore drains the RPC queued by the previous test before the next one starts blocking on git, which is exactly the "bounds each stall to one test" property the comment claims.

Vitest awaits a promise returned from a hook, so returning new Promise<void> does make the yield real rather than fire-and-forget.

Consistent with an existing repo-wide precedent

The comment cites scripts/tests/test-setup.ts, and that precedent is real and already merged at main (:22-32), describing the same failure mode — "a worker's event loop blocked for the whole file (>60s)" — and applying the same capture-at-load technique:

const realSetTimeout = setTimeout;
beforeEach(() => new Promise((resolve) => realSetTimeout(resolve, 0)));

The only difference is the primitive: setImmediate here versus setTimeout(…, 0) there. Both are captured before tests run and both yield a full loop turn; setImmediate fires in the check phase after I/O polling and is not subject to timer clamping, which is a reasonable choice for a suite whose stalls are caused by child-process I/O. Two independent sites in the repo now describe and treat the same vitest behaviour, which corroborates the diagnosis rather than resting on it.

Isolation impact

Adding a yield between tests does open a window in which a promise left pending by the previous test can settle. That is inherent to the fix and is bounded here: the prior test has already completed its assertions, and the suite's lease is released in afterEach (:382) before the next test's yield runs, so nothing that a passed assertion depended on can be retroactively disturbed. No existing assertion, expectation, fixture or timeout was modified — the diff is purely additive.

Scope and CI

One file, nine lines, no historical review and no review thread on this PR, so there were no prior blocking issues to re-verify. I did not audit the rest of base-tree.test.ts beyond the hook interactions above, since the diff cannot reach it.

Test (ubuntu-latest, Node 22.x), review-pr and triage were all still in progress at the time of review. I did not wait for them; per Critical-only scope a pending check is not a gate. Worth noting for the author that the check which actually demonstrates the fix is the one still running, so the empirical confirmation lands with CI rather than with this review.

Verdict: APPROVE — No Critical found. The hook is correctly scoped and ordered, the primitive is captured before fake timers could intercept it, and the technique matches an already-merged repo-wide precedent for the same vitest event-loop starvation.

Independent Critical-only review — head 1014ad02

Re-derived at this head. My earlier approval was recorded at 021e9222 and was auto-dismissed when the branch moved.

What moved

021e9222..1014ad02 is one commit, test(cli): pin the base-tree event-loop yield with a witness pair, adding 16 lines to packages/cli/src/commands/review/base-tree.test.ts and nothing else. No production file changed, so the yield hook I verified at the previous head is untouched — I re-confirmed it is still present and unmodified at :65 rather than assuming so from the commit message.

No historical blocker

The only finding ever filed on this PR is R1-1, a Suggestion at floor:"o", reporting that nothing in the repository pinned the invariant the yield establishes. There is one review thread and it is resolved. No Critical has been recorded in any round, and there is no CHANGES_REQUESTED against this PR.

The new commit is the answer to that Suggestion, so I checked whether it actually pins the invariant or merely looks like it does.

The witness is genuine, not vacuous

let yieldObserved = false;

it('arms a flag from a real macrotask callback', () => {
  realSetImmediate(() => { yieldObserved = true; });
});

it('observes the event loop turned between tests', () => {
  expect(yieldObserved).toBe(true);
});

Three properties make this a real mutation-killer:

• setImmediate is the right oracle. A setImmediate callback fires only in the check phase of a genuine loop turn. Microtask draining between tests — promise continuations in the runner, which is all that would happen without the hook — cannot reach the check phase, so the flag stays false. The witness therefore fails if the yield is removed, which is exactly the property R1-1 asked to have pinned.
• It cannot be vacuous in this file specifically. The defect being fixed exists because the loop does not turn on its own here: the suite drives real git through spawnSync/execFileSync and blocks the worker for the whole file. That is the strongest available evidence that nothing else in the runner supplies the turn the witness depends on.
• Ordering is correct and the scope is clean. Both tests sit at file top level, after the }); that closes describe('runBaseTree') at :3995, so the suite's own beforeEach at :201 — which creates the base tree and acquires the worktree lease through blocking git — does not apply to them. I confirmed the yield at :65 is the only file-level hook in the file: there is no file-level afterEach, beforeAll or afterAll that could turn the loop independently. Immediates run FIFO, so the callback armed during the first test fires before the resolve the second test's beforeEach queues, and the assertion observes true for the right reason.

The tests reuse the same load-captured realSetImmediate the yield itself uses, so the witness stays correct even if a future test in this file installs fake timers — the property that made the original fix robust is preserved in the thing that now pins it.

One consequence worth stating rather than gating on: the pair is order-dependent by construction, so it assumes vitest runs tests in file order. That holds under the default configuration and is inherent to any witness for an inter-test property; a shuffled sequence would be the only thing to break it, and it would break loudly rather than silently passing.

CI

review-pr was queued at this head; no failure attributable to this diff, and CI state is not a gate. The check that empirically demonstrates the original fix — Test (ubuntu-latest, Node 22.x) — was not among the outstanding checks at the time of review.

Verdict: APPROVE — No Critical found. The delta is test-only, no production code moved since the yield was verified, and the added witness genuinely pins the invariant by depending on a check-phase turn that only the yield supplies.

LGTM at 1014ad02. Test-only change, CI green at this head, no open threads, and qqqys has already approved this head.