Round 8 of the review asked for an AST. That was the right diagnosis of the
wrong problem: the question was never which parser to use, it was why a
test-timeout guard was parsing JavaScript at all.
It got there one reasonable step at a time. The guard enforces two rules no
Vitest config can report — a per-test `retry`, and Testing Library's
`asyncUtilTimeout` — so it read them out of source text, and the review found a
new valid spelling it missed in five consecutive rounds: a trailing comment,
nested parens in `it.each([makeCase()])`, a member-access prefix, the shorthand
`{ retry }`, an aliased import, a non-literal title. Each fix was correct and
each made it more parser-shaped, until it carried eight helpers doing quote
tracking and bracket balancing.
Both rules are observable at runtime, exactly:
- `vitest.setup.shared.mts` is loaded by all six projects and reads
`ctx.task.retry` — the value Vitest resolved for the test. That covers
strictly more than the scan did: a per-test option, a `describe` option
inherited by children, a project-level setting, and a `--retry` flag typed at
a command line, which the scan could never see since it only read npm scripts.
- `clients/web/src/test/asyncUtilTimeout.test.ts` asserts
`getConfig().asyncUtilTimeout`, the value this project's own waitFors read.
Neither has a spelling to anticipate, so neither can be outrun by a new one.
Both were observed firing before being trusted.
The guard keeps only what it can answer by asking Vitest: resolved budgets,
project-level retry, config discovery, and that each project still loads the
setup file — the one thing about the runtime assertion a config CAN report.
868 lines to 361, every scanner helper gone, and 33 of 48 unit tests deleted
because they pinned edge cases of a thing that no longer exists.
storybookSetup.ts goes with it. At the library's own value its configure() call
was a no-op, and the assertion is what pins the number.
AGENTS.md gains the general rule: a rule about what the code DOES is asserted at
runtime, not read out of the source — reach for a verify:* guard when the
question is about configuration, which a tool can be asked to resolve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Two scanner bypasses and a wrong claim in the testing skill.
The call scan used `\b` before the binding name, and a word boundary succeeds
straight after a dot — so `other.configure({ asyncUtilTimeout: 1000 })` satisfied
the check while the imported binding was never called at all. A negative
lookbehind excludes a member access and any identifier the name is merely the
tail of; both shapes are now tested.
The retry scan required a colon, so JavaScript's shorthand property form
`it("x", { retry }, …)` — the same declaration with the value bound above — went
through. Both forms are recognized now, and `{ retryCount }` still does not
match.
The testing skill told developers the auto-settle "no-ops under fake timers".
It throws, deliberately, so a deadlock reports immediately instead of hanging
until the project's hookTimeout. Documenting it as a silent no-op is the more
dangerous direction to be wrong in: someone combining the two would expect
degraded cleanup and get a failed test. The skill now says which it is and why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The nesting rule in AGENTS.md was written backwards. "Never nest a budget inside
an equal-or-larger one" forbids the two arrangements this PR deliberately ships
(5s inside 15s, 15s inside 25s) and permits the broken one it exists to prevent.
It now says what it means: an inner budget must be strictly smaller than the
budget enclosing it, arranged by raising the outer rather than shrinking the
inner, since the inner is the bound carrying the useful diagnostic.
The retry scan is structural now rather than a regex. A character class cannot
step over a chain argument containing nested parentheses, so
`it.each([makeCase()])` and `it.skipIf(() => isWindows())` were silently
skipped — the second blind spot of the same kind in two rounds, which is what
makes the pattern the wrong tool. A bracket matcher that skips string literals
handles both, and the positional requirement that keeps fixture data and
`client.retry` out of the results is unchanged. Seven positive forms and six
false-positive shapes are pinned.
The asyncUtilTimeout check read only the first configure() call, so a file
holding the expected call followed by an overriding one passed while its
effective timeout was something else. Every call is inspected and all of them
must agree, which is stricter than checking the last and gives a message naming
the offending value.
Also corrected two PR-description bullets that still described superseded
behavior: the cli child timer stays at 15_000 with the test raised to 25_000,
and AppRenderer's waitFor keeps its 5000 rather than losing it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings are the same shape as round 2's: a guard that documents an
invariant while checking one narrow expression of it.
`retry` was only rejected at the project level, so `it("x", { retry: 2 })` and a
`--retry` flag in an npm script both walked through a guard whose own header
said "no retry anywhere". Enforcement is extended rather than the claim
narrowed, because the decision is about the behavior, not about a config key —
all three places Vitest accepts one are now scanned, and the header names the
one that remains out of reach (a human typing --retry into their own shell),
rather than overclaiming.
The test-definition scan is deliberately positional: the retry must sit in an
options object following the test name, which is the only place Vitest reads it
from. A bare word match would fire on fixture data, on `client.retry`, on an
unrelated helper's options and on a mock — all of which exist here, and a guard
that cries wolf gets disabled. Both directions are tested. The first pattern
also missed `it.each([...])("name", { retry })` entirely, so every parameterized
suite was a blind spot; the table argument is now part of the pattern.
Config discovery named only the two `.ts` spellings this repo happens to use,
while Vitest loads twelve. A `clients/foo/vitest.config.mts` was therefore
invisible, and an invisible config yields no project to reject — the same hole
round 1 found in the hand-written CONFIG_ROOTS, one level down. Both discovery
and resolution now use the full set, ordered as Vitest orders it.
Both checks were observed firing before being trusted: a planted retry in a
launcher test and a planted vitest.config.mts each fail the guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The comment stripper had a real hole: the line-comment regex was anchored, so it
only removed `//` at the start of a line and a call disabled after code —
`const disabled = true; // configure({ asyncUtilTimeout: 1000 });` — survived it
and the guard passed with no effective configuration. Replaced with a scanner,
because a regex gets this wrong in both directions: it has to strip a comment
that follows code, and it must NOT treat the `//` inside a string literal as one,
which would silently truncate any line holding a URL. Quote tracking (with
escapes) is the only way to have both. Three regression tests.
Two documentation claims were also wrong. The guard header and quality-gate.md
cited #2278 and #2250 as instances of a budget nobody chose; they were not —
#2278 was a missing condition wait around a geometry read and #2250 a real race
in a test's own timing, both fixed by making the test wait for the right thing.
Presenting a race fix as evidence for a larger ceiling argues against #1596,
which this guard exists to uphold. Only the budget and poll-ceiling cases are
cited now, and both files say explicitly why those two are excluded.
And browser-timeouts.mjs overstated its own behavior twice: `bestEffort` can
spend its full budget on an otherwise passing smoke precisely because its expiry
is caught, and raising any budget does delay the report when the thing waited on
never arrives. The diagnostics improve the eventual message; they do not make
the wait shorter. Both now stated plainly, so the header is something to weigh a
change against rather than a reassurance.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
No inline comments this round; all four came from the suppressed block.
The AppRenderer one is a regression I introduced by combining two changes that
were individually right. #2323 deleted that `waitFor`'s `{ timeout: 5000 }` as a
budget nested inside an equal one, which it was — and then round 2 pinned
asyncUtilTimeout at 1000, which is the exact default the site's own comment
records failing twice. The raise is restored: the enclosing budget is 15000 now,
so it is observable rather than unwinnable, and the value it beats is a measured
failure rather than a library default.
The guard could approve a configure() imported from the wrong Testing Library
copy — `cleanup` from @testing-library/react and `configure` from
@testing-library/dom satisfied two independent regexes while configuring a copy
no test calls. It now resolves the binding `configure` has in the import from
the required module, follows an alias, and reads asyncUtilTimeout out of that
call rather than out of the file. Three regression tests, including the split
import and a decoy constant elsewhere in the module.
secret-store-selection's 60s exception is hoisted to
LOCK_RETRIES_EXHAUSTED_MS, matching file-lock.test.ts, which bounds the same
wait — the new rule asks for named as well as justified.
And the testing skill still prescribed `HEADER_ANIM_MS + 200`, so a test written
by following it would have bypassed the shared slack constant this change
introduced. It now prescribes `HEADER_ANIM_MS + RAF_SLACK_MS` and says why the
two terms are separate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The inline finding was a real bug: main() never called the discovery check, so
the deny-by-default claim rested on test:scripts happening to run the same
comparison — a different command, which `npm run verify:test-timeouts` does not
invoke. `failures` is now seeded from it, and a probe config under clients/ was
observed to fail the guard.
Three suppressed findings, all correct. The afterEach in oauth-interactive
restated the shared hookTimeout and labelled it as a round trip, when it only
stops a server and unlinks a file; it now carries no budget. The file-lock
constants were named for the opposite of what they bound — 90s covers retries
running out against a holder that never yields, 60s covers the stale-lock
takeover that succeeds — so they are LOCK_RETRIES_EXHAUSTED_MS and
STALE_LOCK_TAKEOVER_MS. The browser-timeouts header said 15 budgets; it is 19.
Separately, and more importantly: the asyncUtilTimeout raise is reverted, on
measurement. Verifying round 2 turned up intermittent 15s timeouts across
unrelated unit files, so three arms of three full unit runs were taken in this
worktree at comparable load — unmodified v2/main 3/3 green, this change with
5000 producing 1, 4 and 0 timing-out files, and the same change with that one
line reverted 3/3 green. A wait that is MEANT to expire spends its whole budget
on the happy path, so a 5x raise is a 5x cost on exactly those tests and
saturates the worker pool, starving tests that were never slow.
Both web setup files therefore state asyncUtilTimeout at the library's own
value rather than raising it, which is what #2323's rule actually asks for: a
budget nobody chose is the defect, not a budget that is small. The tests that
assert absence by letting a wait expire are the real problem and are #2335.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
All nine findings taken.
Two budgets I had lowered are restored as named exceptions rather than folded
into the shared value. cli's e2e child timer goes back to 15s and the two tests
that spawn it get E2E_SPAWN_MS = 25s, so the bound carrying the diagnostic still
wins without taking load tolerance off the one suite that spawns a real process.
App.test.tsx's three-modal sequence keeps THREE_MODAL_SEQUENCE_MS = 20s — a
value chosen against an observed failure, which the shared ceiling would have
replaced with a general one.
Three real false-positive paths in the guard are closed. It now discovers every
Vitest config under clients/ and fails on one it is not configured to check —
without which "a project with no row is an error" was vacuous, since an
undiscovered config yields no project to reject. It strips comments before
looking for configure() and requires the exact expected value, so neither a
commented-out call nor asyncUtilTimeout: 1 passes. And it compares each
resolved project's setupFiles against the site it claims to configure, so
deleting a setupFiles entry can no longer leave both projects reported as
configured while Testing Library silently reverts to 1000ms. Each has a test,
and the setupFiles check was observed to fire against a real deletion.
Three stale comments the change had invalidated: main.yml still counted four
durable guards, PreviewAnnotations.stories.tsx still said the storybook project
had no setupFiles, and the new AGENTS.md rule read as though no Group F row had
been settled when this PR settles F6. browser-timeouts.test.mjs gains the
purpose header this repo's files carry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Three of the six Vitest projects ran on Vitest's own testTimeout: 5000 and
five on its hookTimeout/teardownTimeout: 10000, and all 788 Testing Library
waits in the web unit project ran on asyncUtilTimeout: 1000. None of those
numbers was chosen here, and all of them were sized for an idle machine rather
than for three or four concurrent worktree sessions each running a full
local:gate on eight cores. #2292, #2278, #2250, #1942 and #1742 were each that
class, diagnosed one site at a time.
This is the complement of #1596, not a retreat from it: every fix that issue
produced stands, and no race is hidden here. What changes is that a correct,
deterministic test is no longer cut off by a budget nobody picked.
- vitest.shared.mts exports frozen TIMEOUTS / INTEGRATION_TIMEOUTS; all six
projects spread one of them, clients/launcher importing the shared module for
the first time to do so.
- Both web projects configure Testing Library's asyncUtilTimeout. The storybook
one needs its own setup file, deliberately outside .storybook/ and free of
setProjectAnnotations so the automatic preview-annotation provisioning
(#1898) is untouched.
- 28 per-suite restatements of the integration project's own budget are gone,
so raising the shared value now moves every site under it. The handful that
genuinely differ — cross-process lock contention, a full OAuth round trip —
keep their budget as a named constant that says why.
- AppRenderer's waitFor no longer asks for exactly its enclosing budget, which
it could never win (#2292's shape); cli's e2e child timer drops below the
project budget so the bound carrying the diagnostic is the one that fires.
- The transition auto-settle's slack term is named RAF_SLACK_MS and raised;
the term derived from HEADER_ANIM_MS is untouched. The two answer to
different things and must not be scaled as one number.
- The 19 Playwright budgets scattered across three web smokes become four
named constants in scripts/lib/browser-timeouts.mjs.
scripts/verify-test-timeouts.mjs resolves each project through Vitest itself
and asserts the budgets, the asyncUtilTimeout sites, and that retry stays
unset. It was observed red against the pre-change config: unit/tui/launcher at
testTimeout 5000, five projects at hookTimeout 10000, both asyncUtilTimeout
sites unmet.
Closes#2323
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall