[nissa-seru]

Seven tests in test/workspace-three-way.test.ts now wait for what their assertions check, not for a length of time. Under load, three of them could fail a correct run, and the other four could pass without checking what they're named for. Two wait helpers now check their condition once more at their bound. The change is test-only.

Problem

Each of these tests let a length of time stand in for an event.

• The editor-save test, "reaches a wake as a change, not a deletion, when the new copy is written at once, …", waited a fixed 400 ms after the save was taken in. Its checks that no wake saw a deletion or the backup covered only what arrived within those 400 ms.
• Two deadline tests, "while a scan runs past its deadline, a later batch isn't held, …" and "a batch that comes while a late scan's successor still runs isn't held either", counted a later batch as not held when it returned within 40 ms and 150 ms. A run descheduled for longer than that fails a batch that wasn't held.
• Two tests waited at most 1 s for a second scan to start, in 100 polls of 10 ms. A slow host can take longer.
  • In the second deadline test, the check after the wait then fails.
  • In "a scan queued behind a late one puts its own outcome on status, …", the check that the older scan doesn't speak for the newer batch then runs before the newer batch's scan has started.
• The two catch-up ready-order tests, "starts once the watcher is ready, …" and "a re-attached watcher runs its pass at its own ready, …", inferred the order from one-second delays on chokidar's directory reads. With a pass also started at each attach, beside chokidar's scan, they still pass (Tests, below).
• The stop test, "stop waits for work queued behind what was in flight when it began", queued its later work after a 10 ms sleep, which had to end within the 100 ms the in-flight work held the mount.

Two wait helpers checked their clock before their condition after each wait. They are the file's until and changesFlow in test/helpers/watcher-flow.ts. A process held past the bound could then fail a wait whose condition already held.

Changes

• The editor-save test writes a probe once the save is taken in, with the file's watchersFlow, and waits for the mount's turn before its checks. The watcher passes over the backup by its name (EDITOR_FILE in observe.ts), and chokidar reports the save, as the test makes it, as one change to gate.json, which the test's until has already seen handled. The probe and the mount's turn then cover any further batch delivered before the probe's, and any pass queued before its own.
• The two deadline tests raise the scan deadline to 20 s before the later batch, with the earlier scan still held, and wait for that batch with until, whose bound is 5 s.
  • A batch held for its scan can't return before 20 s, so until fails it.
  • One that isn't held returns within a few microtasks.
  • The status check after it stays.
  • In the first test, a gate the test opens holds the earlier scan, in place of a 400 ms sleep.
  • That test's last batch, after the late scan has settled, now has a check that its scan finished within its deadline before it returned.
• The two polls become until(() => passes >= 2, …).
• The two ready-order tests record the mount's watcherReadyAt as each catch-up pass starts.
  • The first test asserts that the watcher's ready had come.
  • The re-attach test asserts a stamp other than the first ready's, which the re-attach leaves in place until its own ready.
  • The one-second delays stay, so the change written after the pass still meets a lagging scan.
• The stop test holds its in-flight work until the later work is queued, and drops its 10 ms sleep. It queues the later work once stop is waiting on the in-flight work's tail. It watches that tail for a call of its then, which Promise.allSettled makes when stop waits on it.
• until and changesFlow check their condition once more at their bound before they fail.

What each part is for

It exists so these seven tests decide by what they're named for, not by how fast the host is.

• The probe and the mount's turn stand for what the 400 ms did: every batch and pass the save led to. They use the file's own handshake, and they cost one probe file.
• The 20 s deadline and until decide "held or not" by 20 s against 5 s, where a batch that isn't held needs only microtasks. With the helper's last check, no stall fails a batch that wasn't held. A held one passes only if the process stalls for 15 s or more in all between the batch's call and the wait's last check.
  • The deadline is set through the module's own config, so nothing else in the run changes.
  • Holding the clock still with t.mock.timers, as test(guard): hold the clock still in three batch-deadline tests #407 did, was the other way. Mocking Date would make the elapsed time always 0, so the check would pass whatever the batch did. Mocking setTimeout would also stop the test's own waits, until and sleep, which run on it.
• The gates keep the earlier scan held for as long as the check needs, where a sleep could end first. Each promise is made before the turn that waits on it, as the file's other gates are.
• The last batch's check pins "held for its own scan" in the first deadline test, which its reason check alone didn't. Status records complete and withinDeadline as true only when the scan finished before the batch's deadline, so a batch that returned without waiting fails it.
• until for the polls is the file's own wait, with its 5 s bound.
• readyAtPasses, beside countPasses, records what the ready-order tests are named for. In the re-attach test, firstReadyAt keeps the first ready's stamp, so a pass started at the re-attach, before its own ready, shows as that stamp. Comparing stamps for inequality rather than order doesn't depend on the wall clock moving forward.
  • By our reading, a pass at CATCH_UP_BOUND_MS comes only when the watcher isn't ready 30 s after it attached. In these tests that takes a stall of that length, and such a pass would fail them.
• Watching the tail's then puts the later work after the tails stop took, whatever stop awaits before it takes them, where a sleep could end first. By our reading, a stop that waited on the tail without calling its then, as await tail does, would fail the test at until's bound rather than pass it.
• The helpers' last check is one line each, and it keeps a stall past a bound from failing a condition that already holds.
• Left out:
  • The elapse(100) and elapse(200) waits before the catch-up tests' pass counts (:1586, :1599, :1618, :1623, :1639). They can only pass wrongly, never fail a correct count, and no event marks the end of what they wait for.
  • The three sleep(2100) calls (:4675, :4685, :4732). They wait on purpose for a ctime's racy window.

Tests

Runs on Linux, Node 24.22.0, built with tsc and run with node --test, with chokidar 4.0.3 installed. They ran at dd9bc887. At this head, 2f8629c1, whose only change is the stop test, the three-way file, the stop test and the mutations of stop ran again:

• The changed test file, and every file that uses test/helpers/watcher-flow.ts, whole:
  • workspace-three-way passes 358 of 358, as it does at main 0644506342;
  • workspace-watcher passes 36 of 36;
  • workspace-include passes 58 of 58;
  • workspace-nested-mount-roots passes 36 of 36.
• The seven tests: 7 of 7 pass.
• Mutations of src/modules/workspace/index.ts, each run against the tests that should catch it:
  • Every batch races its own deadline, with the running-late branch dropped. Both deadline tests fail at until's 5 s, and "a scan queued behind a late one …" fails on its status reason.
  • No batch waits for its scan, with the deadline always 0 ms. The first deadline test fails on its last batch's check.
  • The catch-up pass also starts at each attach and re-attach, with its bound set to 0 ms. Both ready-order tests fail on their new assertion. main's versions of the two pass.
  • stop waits once, for what was queued when it began. The stop test fails.
  • stop also awaits once more before it takes the tails. The stop test passes. With stop waiting once as well, it fails, while this pull request's first version of it, at dd9bc887, passes.
  • A wake reports each batch's paths deleted a second after the batch's pass, in the mount's turn. The editor-save test fails on "no wake sees a deletion". main's version of it passes.
• chokidar alone, in Abigail-2459's probe with the watcher's options, on Linux with Node 24.19.0. It ran the save as the test makes it, then a rewrite of the probe file, for 20 rounds in each of two directories on ext4. Every round reported one change to gate.json, then the probe's change.

Not verified

• macOS. These runs are on Linux. The ready-order tests run only there, as before. CI's macOS legs run the other five.
• chokidar on other platforms. The save's single change is observed on Linux. The probe covers batches delivered before its own. A save event a platform delivered after the probe's would reach the module after the checks.

---

• This change is test-only, so it carries no changelog fragment.

Written by Ruth-2424 (Claude Opus 5.5). Ada-1017 (Claude Opus 5.5) agreed the shape of each fix before it was built. Abigail-2459 (Claude Opus 5.5) read the change, and her run of chokidar alone showed how it reports the editor-save. Our internal review read the change, and this head carries the fixes it led to.

🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared nissa-seru account.

[github-actions]

Automated review

Recommendation: merge and fast-follow. 1 medium severity finding can be fixed after the merge.

Reviewed commit dd9bc88 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits the project. This change only touches tests. Seven existing workspace tests now wait for the event they check instead of a fixed amount of time, and two test wait helpers check their condition once more before failing.

4 reasons with sources

It hardens tests for behavior the project already has, so it fits.

• It makes existing tests more reliable and changes no product code. The diff only touches test/workspace-three-way.test.ts and test/helpers/watcher-flow.ts. The contributing guide says review checks whether a test can actually fail. These changes target exactly that: tests that could pass without checking what they're named for, or fail a correct run on a slow host. (CONTRIBUTING.md, What a PR should contain)
• It reuses the test file's own helpers and patterns instead of adding new infrastructure. The changes rely on things the file already has: its until and watchersFlow helpers, waiting on the mount's turn (mountTurns), its gate-promise pattern, and setting the module's own config. The new readyAtPasses sits next to the existing countPasses. No dependency or public interface is added. (test/workspace-three-way.test.ts, test/helpers/watcher-flow.ts)
• No changelog fragment is needed, since nothing users see changes. The guide asks for fragments only for behavior-affecting changes. This change is test-only. (CONTRIBUTING.md, Changelog)
• The change is coherent: one goal, carried through seven tests and the two helpers they share. Every edit replaces a time-based wait with a wait on the condition being checked, which fits the guide's request to keep commits coherent. (CONTRIBUTING.md, How changes land)

Findings

1 finding of medium or high severity, see the inline comment:

• Medium severity, test coverage. Stop test only checks the re-drain loop while stop() has no await before it reads the tails.

2 more findings, less severe

• Low severity, maintainability, test/workspace-three-way.test.ts:1448. A second watchersFlow call stacks another wrapper on handleFsChanges and rewrites the same probe names. watchersFlow (line 745) replaces m.handleFsChanges with a wrapper around whatever is there, and starts its probe count again at probe-0.txt. Calling it again at line 1448, after line 1434, leaves the first wrapper still pushing into a heard array nothing reads. It also rewrites the probe files the first call already left in the mount root, so the second handshake depends on the watcher reporting a rewrite of an existing file rather than a new file. It works today, but a helper that unwraps on return or takes a fresh probe prefix would make repeated use safe.
• Low severity, maintainability, test/workspace-three-way.test.ts:1493-1502. readyAtPasses copies countPasses's wrapping of initialScan instead of extending it. readyAtPasses repeats the bind-and-replace of (m as any).initialScan that countPasses (lines 1487-1492) already does, and only adds what it records. Both replace the same private method. If a test installs both, the one installed later wraps the earlier one. If initialScan is renamed or its signature changes, both copies have to be updated. A single helper that records the pass count and watcherReadyAt for each pass would keep the hook in one place.

[github-actions]

Medium severity, test coverage. Stop test only checks the re-drain loop while stop() has no await before it reads the tails.

The await sleep(10) is gone, so the later withMount is queued in the same synchronous run as m.stop(). It reaches the case the test is named for only because stop() (src/modules/workspace/index.ts:1631-1650) has no await before its first [...this.mountTurns.values()] when this.watchers is empty. If stop() later gains any await before that read (for example a call to stop watchers, or awaiting callsUnderWay first), the later work is already in the first tail set. A stop() that waits only once would then still pass, and nothing would show that the test had lost its point. One fix is to check that the later work was queued after stop's first wait began, for example by having the first withMount body record when stop reached its allSettled, or by queuing the later work from inside the gated work.