Copilot review round 1: "every project inherits availableParallelism() - 1"
overstated the scope. The five forks-pool projects do; web's storybook
project runs the browser pool, whose default is min(12, availableParallelism()
- 1) — the same 7 on this machine, but a different expression, capped on
larger machines and 1 unless headless — and none of the arms measured it. A
maxWorkers set on that project would still apply (getThreadsCount reads it
first), so it is not exempt from the decision, only unmeasured; both files
now say so.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
No Vitest project sets `maxWorkers`, so every one inherits Vitest's
`availableParallelism() - 1` (7 on the team's M3). #2336 asked whether that
number should be stated or capped. Measured at the leased baseline (#2339),
interleaved default / --maxWorkers=4 / default, three rounds each: a cap of 4
costs +14% wall on the web unit suite and +39% on coverage:web, saves 18-22%
CPU, and prevents zero failures (0 of 18 runs either way). Against a sibling
session's concurrent unit run it is a small politeness trade with no
throughput gain; the only failures seen were one test's 5s inner waitFor at
14 workers, which is a site to fix rather than a reason to cap.
So the default is kept, as a chosen value: the reasoning and the numbers sit
beside TIMEOUTS in vitest.shared.mts, and AGENTS.md carries the rule so a
reviewer can cite it against a future per-project cap. CI is unaffected.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Handle SIGQUIT (Ctrl-\) on the same tree-shutdown path as SIGINT, SIGTERM
and SIGHUP. The gate runs in its own process group, so a signal the wrapper
does not handle ends the wrapper alone and orphans the gate behind a lease
that then goes stale under it. The signal test is parametrized over SIGTERM
(143) and SIGQUIT (131).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- The spawn itself — spawnSpec and the synchronous argument validation in
child_process.spawn — now sits inside the try whose finally releases the
lease, so a command that fails before it starts cannot strand the lock
until stale takeover. Tested with a non-array args value.
- A genuine release failure no longer falls through to the "released
after" line; the lock may still be blocking the queue, and the message
that says so is the last word.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- The holder-record removal is best-effort and can no longer throw past
release(); ERELEASED on release is the library having already dropped a
taken-over lock, so it is ignored rather than described as a lock that
"goes stale on its own".
- The signal test's stand-in now spawns a descendant of its own, so the
whole-process-group guarantee is what the test asserts: a regression to
child.kill() or a non-detached spawn leaves the descendant alive and fails.
- The three docs no longer claim a dead holder is always recovered in 30s:
a stale lock directory that cannot be removed runs the wait to its
45-minute cap and names the path.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- A non-ELOCKED acquire error with the lock directory still present (a
stale lock the library could not remove: ENOTEMPTY, EACCES, EROFS) now
waits like a held lock instead of degrading to an unleased run; only a
lock that cannot be created at all degrades.
- Spawn with a shell on Windows (npm is npm.cmd there) and quote the
arguments for cmd.exe via the existing winShellArgs; exported as
spawnSpec so the branch is unit-tested from POSIX.
- Release the lease on every way out, including a spawn failure, via a
finally around the child wait.
- Guard every lock removal (release and the library's exit handler) on the
directory's identity, mirroring core/auth/node/file-lock.ts, so a holder
whose lock was taken over never removes the winner's.
- The signal test stops the wrapper in a finally, so a failed setup
assertion cannot leak its idle grandchild.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
`npm run local:gate` now runs its stages under `scripts/gate-lease.mjs`, a
`proper-lockfile` lease under `os.tmpdir()` (or `$XDG_RUNTIME_DIR`), so a gate
started in a second worktree waits for the first instead of overlapping it.
Measured on v2/main @ aa56551b: a quiet gate is 257s green; two started
together had one fail at 279s (`smoke:web:chromium` found port 6298 held by
the other gate's smoke) and the survivor take 338s. Under the lease, three
gates started within seconds ran back to back (259s, then 262s, then the
third), each green with zero timeouts.
The lease grants rather than polls a threshold, so two waiters cannot
deadlock on each other; a holder that dies stops refreshing and is taken over
after 30s; a signal to the wrapper stops the whole process group, releases,
and exits 128+signal; the wait names the holder's pid and worktree and gives
up after 45 minutes. `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. A lock that
cannot be created runs the gate unleased with a warning.
`scriptChainRuns` looks through the wrapper (and only it) so the existing
vouches about what `local:gate` runs keep reading the real chain.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 7's one suppressed finding: "that second pass" read as the
instrumented coverage pass; the ~80s is the bare pass that ran first.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- `reachableScripts` now follows every npm spelling of `run` — the
canonical `run-script` and its `run`/`rum`/`urn` aliases — with a unit
case, so `npm run-script validate:web` appended to `local:gate` fails
the whole-gate check instead of slipping past it.
- docs/quality-gate.md `verify:skills` row, AGENTS.md skills rule 1 and the
#2085 narration now describe the actual chain (`validate:guards`, reached
by both `validate` and `local:validate`) and the qualified promise.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- Reword the CI guarantee in AGENTS.md, docs/quality-gate.md and the
pre-push-gate skill: a green gate means every check CI applies has
passed here — the strongest predictor of green CI, not a proof — and
name the residual (a test that passes only when slowed) as #1596's
class to fix rather than headroom to keep.
- docs/quality-gate.md: cli's validate DID build, through `test`'s
`pretest` hook; say that the build goes with the dropped `test` leg and
`coverage:cli` builds once, later — in both places.
- Update the two source comments that still described `local:gate` as
running `validate` (verify-skills.mjs, verify-build-gate.mjs).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Trace reachability from `local:gate` itself, not only its first stage, when
asserting that no `validate` / `validate:<client>` is reached — a later
stage could reintroduce the bare suite just as easily as the first.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- Match each client's `cd` together with its adjacent `npm run check`, so
dropping one client's check from `local:validate` fails the test.
- Reject `npm test` / `npm t` / `npm tst` in a client's `check` commands as
well as `vitest`, since `reachableScripts` follows `npm run` only.
- docs/quality-gate.md: cli's `check` does not build — its binary is built
by `coverage:cli`, as before; say so in both places.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
`local:gate` ran every client's unit suite bare inside `validate` and
again instrumented inside `coverage` — ~80s of a ~370s run re-executing
exactly the files the coverage pass runs a few minutes later. CI pays
nothing for that duplication because its two jobs run in parallel; the
gate runs serially, so it paid all of it.
The gate's first stage is now `local:validate`: the same `validate:guards`
and `validate:core`, then each client's new `check` script (`format:check`
+ `lint` + `typecheck` + `build`) instead of its `validate`, which is now
literally `check && test`. `npm run validate` and both CI jobs are
unchanged; `local:validate` sits in the `local:` namespace so the
workflow guard keeps it out of CI by construction.
The "strict superset" claim in AGENTS.md is reworded to what is now
true — every CI *check*, not every CI *invocation* — with the reasoning:
`@vitest/coverage-v8` reads V8's precise-coverage profiler and rewrites
no source, so the instrumented pass runs byte-identical code and is only
slower, the stricter direction for the failure class this repo sees.
Measured on the same worktree: 370s -> 262s wall (EXIT=0 both), the
four removed legs summing to 78.5s in the baseline run's own profile.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The storybook project's optimizeDeps now comes from
`getStorybookOptimizeDeps()` in `vite-base-config.ts`, beside the dev-server
helper it mirrors, so the `force` flag and the base include/exclude it carries
are asserted by `vite-base-config.test.ts` rather than living only in an
inline object a config refactor could drop.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Vite keys the Storybook project's dep cache on the lockfile and the config,
never on what the story graph imports. After a story or component gains an
import of an already-installed package, the cached bundle is still "valid",
Vite skips the scan, and the first request for the new package re-runs the
optimizer mid-run: the browser hash is bumped, a full-reload is sent that the
Vitest tester iframes ignore, and the `@storybook/react` renderer already
loaded in a live iframe keeps the old `?v=` for its lazy
`import("@storybook/react-dom-shim")`, so every story that iframe renders from
then on fails with "Failed to fetch dynamically imported module". The next run
is green because the cache has caught up.
`optimizeDeps.force: true` on the storybook project discards the cache up
front so the scanner crawls every story entry before the browser opens — the
cold path CI already takes on every PR. Measured: stale-cache 6/6 red on the
baseline, 6/6 green with the fix, 3/3 red reverted; ~0.8s per run.
Closes#2340
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
#2323 left five smoke and CI budgets as "assess"; they are now #2333, so the
rule says where they went rather than reading as if they were covered.
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>
Copilot review round 1 on #2329. Four findings, all correct.
The whole-modal claim was false for the three rows whose value is a Badge:
`ThemeBadge` defaults to `fw: 600`, so Transport, Era and Status still rendered
bold-label/bold-value. A `ValueBadge` constant carries the normal weight for
Transport and Status, and `EraBadge` takes an optional `fw` so Connection Info
can ask for it without changing the standalone chip in ProtocolListPanel. The
chip still reads as a chip — its emphasis is the outline and colour, not the
font weight.
Three test weaknesses, each of which let a regression through:
- The weight assertion used `queryByText(...)?.style.fontWeight ?? ""`, which
passes vacuously when the row is absent entirely. Now `getByText`, and
deleting a value row fails three tests instead of none.
- The full-width test asserted only the transparent background, so swapping
`FullWidthField` back for the two-column `SimpleGrid` would still have passed.
It now asserts the structure: label and value are siblings, and the value is
in no SimpleGrid.
- `OAuthTokenField`'s bold caption was asserted nowhere — the weight test
renders no token. A case now covers Access Token and ID Token through it.
Note for anyone asserting a Badge's weight later: `getByText` lands on the
inner label span and `fw` is applied to the root, so the assertion has to walk
up or it reads an empty string and passes against anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB
Signed-off-by: cliffhall <cliff@futurescale.com>
Client ID sat third in OAuth Details, between Status and Client registration,
so its label-over-value block interrupted the run of inline two-column rows and
broke the scan down the label column for everything after it. Access Token and
ID Token already used that layout, at the bottom.
Moved Client ID and Auth URL down to sit with them: the inline rows now run
Protocol, Status, Client registration, IdP session, Scopes, and the full-width
fields follow as one group — Auth URL, Client ID, Access Token, ID Token — so
Client ID is directly above the only other field a reader sees laid out the
same way.
A test asserts the order off the DOM rather than each field's presence, since
order is the property; putting Client ID back in its old slot fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB
Signed-off-by: cliffhall <cliff@futurescale.com>
Connection Info rendered every label/value row as a plain label against a bold
value. That is backwards for a reference panel: the label is the fixed
scaffolding a reader scans down, the value is what differs per connection, and
bolding every value means nothing stands out. Reversed across the whole modal —
Server Implementation, Discovery and OAuth Details, plus the token captions —
rather than in one section, since two halves disagreeing is worse than either
convention.
Client ID and Auth URL were a Code block in the right half of a two-column grid,
so a long value got roughly half the modal width. A CIMD client id IS a URL, so
it is long by construction, and it broke mid-token —
`…/client-metadata.` / `json` — which reads as a rendering fault rather than as
one value. Both now put their label on its own line with the value full width
beneath, the way OAuthTokenField already lays out a token, and neither carries
an inset surface any more: the grey framed a value that is simply the answer to
"Client ID", not a quoted blob needing separation from prose.
The transparent treatment is a new `wrapping-plain` theme variant rather than a
change to `wrapping`. That one is shared with ElicitationUrlPanel,
ListLoadError, MalformedItemsWarning and the CSV fallback, where the inset is
doing real work; a test asserts `wrapping` keeps its surface so those consumers
cannot be changed by accident.
Closes#2328
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB
Signed-off-by: cliffhall <cliff@futurescale.com>
Third and last site of the same class. `ReadableStream.cancel()` adopts
the underlying source's cancel promise, which is permitted never to
settle, so awaiting it kept the handler pending forever — and on an
unbounded request there is no timer to release it either. The cancel is
started and the handler returns, so the JSON response and the `finally`
complete regardless.
The `void` carries the justification for which of AGENTS.md's permitted
cases it is: the callee owns its failures through its own `catch`, and
the caller cannot await.
Test: a `/stream-hostile` upstream that sends event-stream headers and
one frame and then never closes, asserting the route still answers 200
with no body.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
`ReadableStream.cancel()` adopts the underlying source's cancel promise,
which is permitted never to settle — so awaiting `releaseBody` while
propagating a cancellation can hang the very thing it was meant to end.
The five exceptional exits in the compat probe loop now start the
release without waiting on it: the release is a courtesy to the
connection pool, and the caller's abort or timeout must reach it
regardless. The two non-exceptional awaits are unchanged.
Same class as the tee teardown in round 24, one file over.
Also moves the `redactUrlQuery` import above
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS's JSDoc, which it had split from the
constant — so hover and API tooling see the doc again.
Verified by mutation: awaiting the release makes the new test time out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
* chore: land the #2247 round-9 review findings on v2/main
Copilot filed a ninth review round on #2247 after it had merged, so none of
it reached v2/main. All five findings are addressed here.
1. The testing -> test-servers hand-off delivered the wrong half of the
procedure. test-servers documented only the two-process manual path
(server-composable.js --config <name>.json plus a second Inspector
process), but an integration test builds the server in-process from the
@modelcontextprotocol/inspector-test-server API and owns its lifecycle.
Adds a "Two ways to use a fixture — pick the right one first" section
with the in-process recipe, the four mechanics that bite (one barrel,
start/stop in afterEach, read started.url, era is a constructor option),
and a pointer to the reference test; renames "Run one" to "Run one by
hand (two processes)" so the manual path names itself. testing now says
in the connecting bullet that an automated test does NOT spawn.
2. testing's heading asked "does the test need a real server?", which its
own hand-rolled-server exception contradicts. It now asks whether the
test uses a test-servers/ fixture.
3. "imports" was too broad a dependency test — test-server-scope.test.ts
imports the package to unit-test scope middleware as a pure function and
never starts a server. The condition is now "starts, spawns, or
configures", with that test named as the counterexample.
4. docs/skill-authoring.md attributed the 33% -> 100% rise to imperative
wording and placement independently. The #2247 edit changed both at
once, so only the combination is established; the split into two levers
is now labelled the plausible mechanism, not a measured result.
5. The CHAIN_THRESHOLD comment generalised "a well-shaped pointer clears
0.8" from one two-case run of that same multi-variable edit. It now
reads as an existence proof about the ceiling, not a property of
pointers in general.
Also records the headline lesson where it can be found again: a green
hand-off number proves skill B loaded, never that B answered the question —
in AGENTS.md's chain-case rules and in the skill-authoring checklist.
Docs, skill bodies and one comment only; no runtime code changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>
* fix: address Copilot review round 1 on #2264
The new section claimed two shapes and there are three — the automated
stdio path was missing, and it is the repo's established pattern
(getTestMcpServerCommand() into a stdio transport, used by five web
integration files and the whole CLI suite). Telling callers an automated
test "does not spawn" told them not to use it.
- test-servers: "Two ways" -> "Three ways", with a table and a subsection
for the spawned-stdio shape — the transport owns the child (disconnect,
not stop()), no config can be selected because that entry point runs the
stdio default, and it is still the build. Names a reference test for it.
- test-servers: the build is no longer scoped to the manual path. All three
shapes resolve test-servers/build/ — two through the barrel alias, one by
running the emitted .js — so the staleness hazard is called out as
applying to every shape, and `Build first` no longer opens by claiming
the only reason is subprocess spawning.
- testing: the "does not spawn" instruction is replaced by the two
automated shapes and when to pick each. What they share, and what the
hand-off actually needed to say, is that neither picks a showcase config.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>
* fix: address Copilot review round 2 on #2264
Round 1 fixed the missing stdio shape but kept the wrong axis. Cutting
the taxonomy by "automated vs by hand" excluded the config-driven smokes
— smoke-web-elicitation.mjs spawns server-composable.js --config
directly, and lib/mcp-app-flow.mjs does it for smoke:web:app,
smoke:web:tabs and pack:verify. Those are automated AND need every bit
of the config and protocol-era guidance a person at two terminals does,
so the skill was routing their authors away from it.
- test-servers: the cut is now how the SERVER IS STOOD UP, stated as
such. Third shape is "spawned composable HTTP", whose consumers are
the smokes, pack:verify, and a person by hand — the config/era
guidance belongs to the shape, whoever starts it. Table gains a
Consumers column and loses the Who column that caused this.
- test-servers: the stale "an automated test builds the server
in-process instead" line under `Run one by hand` is gone (suppressed
comment); that heading is now the same shape driven by a person.
- testing: routes by what is driving — integration/CLI test to
in-process HTTP, stdio-is-the-point to spawned stdio, and a smoke to
the composable-config path with the era guidance it actually needs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>
* fix: address Copilot review round 3 on #2264
Round 2 replaced one wrong axis with a narrower one: it routed "a smoke"
wholesale to the config-driven composable path, and smoke:cli does not
use it. smoke-cli.mjs starts an in-process createTestServerHttp for the
header round-trip (so it can read back what the CLI sent) and writes the
built stdio entry into a --catalog for the connect checks; smoke:tui uses
only the latter. Neither picks a config.
- test-servers: consumers filled in per row — in-process HTTP gains CLI
tests and smoke:cli, spawned stdio gains smoke:cli and smoke:tui, and
the composable row is narrowed to the WEB smokes. A warning states the
point directly: "a smoke" is not a shape, pick by the row.
- test-servers: the stdio shape now covers naming the built entry in a
catalog without connecting, which is what those two smokes do.
- testing: same narrowing, plus the reason spelled out so the word
"smoke" stops being a routing key.
- testing: the dependency criterion said "starts, spawns, or configures",
which excluded the build-only smoke:tui case documented directly above
it — the skill both required and rejected the hand-off for it
(suppressed comment). It now reads "starts, spawns, configures, or
hands a built entry to the subject under test".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zwYWanPhvgu6wiKoeGg4N
Signed-off-by: cliffhall <cliff@futurescale.com>
---------
Signed-off-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Losing the race rejected the caller but did not stop the read. Because
clone() tees the body, a drain still running kept pulling bytes AND
kept buffering them for the unread original branch — unbounded memory
on exactly the signal-ignoring body the race exists to contain.
The clone is now drained through an explicit reader rather than
clone().arrayBuffer(), precisely so the losing side has something it
can cancel: arrayBuffer() locks the stream to an internal reader
nothing else can reach. On any throw both branches are torn down, which
is safe because that path never hands the response to a caller.
The cancels are deliberately not awaited: cancelling one branch of a
tee can itself wait on the other, which would hold the timeout open for
as long as the stall it is meant to end. Each carries its own catch, so
a stream already cancelled, errored or locked elsewhere does not mask
the timeout the caller actually needs — covered by a test whose stream
throws from cancel().
Verified by mutation: removing the teardown makes the new test fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
The timeout message interpolated the raw request URL, and
createFetchTracker records that message verbatim into the Network log
and the persisted session. An OAuth endpoint's query string can carry a
`code`, an `access_token` or a `client_secret` — exactly the fields
fetchTracking already redacts everywhere else it records a URL — so a
stalled signed endpoint would have written its secret to both.
Redacted with the existing `redactUrlQuery`, in the
OAuthRequestTimeoutError constructor rather than at each display site,
so both the message and the public `url` field are covered by
construction and no future consumer can reintroduce it. The route's own
proxied-timeout message and the URL in its 504 marker get the same
treatment, since both cross back to the browser and are recorded there.
The path and every non-sensitive parameter survive, so the endpoint
stays identifiable — which was the point of naming it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
The timeout and cancellation exits added over the last few rounds drop
the original failed discovery response, which nobody will ever receive
— and an unread body holds its connection open on Node/undici. The
successful-substitution path already released it for that reason; the
five exceptional exits now do too (pre-abort, timeout, fetch-abort,
post-release abort, body-read abort). Matters most for a standalone
user of the exported wrapper, where no timeout wrapper underneath has
already drained it.
Also drops the last descriptions of the superseded rebuild: the test
file's header, an inline comment asserting the response is "not the
same object" when the file later asserts identity, and the PR
description's account of copied metadata and deleted encoding headers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
Rebuilding a Response around the buffered bytes could never be
faithful. `url`, `redirected` and `type` are internal slots, so
shadowing them as own properties satisfied a direct read and was lost
the moment a caller called clone() — which returns a fresh native
Response. The headers guard differed too, and the decoded bytes made
the inherited content-encoding and content-length wrong, so those had
to be stripped: four observable differences from the response fetch
actually produced.
The wrapper now drains `response.clone()` under the deadline and
returns the original. Cloning tees the body, so draining one branch
buffers the other: the bound is unchanged, the caller reads without a
second trip, and every native slot stays intact. rebuildResponse and
its null-body handling are gone, and with them the whole class.
Also: getServerUrl() throws for a non-HTTP configuration, and this
wrapped fetch can still be handed to a transport or a custom factory
that calls it — so the documented fail-open was reachable only until
that getter threw, at which point a configuration error propagated out
of a fetch. Caught, for the same reason readNegotiatesEndpoint catches.
Tests: response identity, the body still readable after the drain, a
re-clone keeping its metadata (the case a rebuild could not satisfy),
content-encoding preserved because nothing invalidates it now, a
null-body status passed through, and the throwing getter failing open
while still not slipping past the form-encoded check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
The exemption was URL-only, so an authorization server configured at
the MCP endpoint's own URL — `oauthTokenUrl` takes any absolute URL —
made the transport-internal refresh take the exempt branch and run with
no signal, no body buffering and no deadline. That is the one request
this change exists to bound.
`isExempt` now receives the request's headers as well as its URL, and
`exemptMcpEndpoint` refuses to exempt anything sent as
`application/x-www-form-urlencoded`, before any URL comparison. That is
a discriminator rather than a heuristic: the token exchange, the
refresh and revocation are all form-encoded per RFC 6749 §4.1.3 and RFC
7009 §2.1, and MCP is JSON-RPC over application/json and never is.
Also records, on the no-deadline route test, what it does not catch: at
a 400ms wait an unconditional timer restored at the production 30s
budget would still leave the request pending and the test would pass.
Catching that needs either a ~31s wait on every CI run or a route-level
deadline knob existing only for the test, and a second source of truth
for the deadline is the defect round 13 removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 17 widened exemptMcpEndpoint to the origin for everything, which
exempted same-origin OAuth work as well as MCP traffic — on the very
401/discovery path #2319 exists to bound.
The rule is now as narrow as each transport allows. Streamable HTTP
sends every MCP request to the configured URL, so origin + pathname is
exact there and same-origin OAuth stays bounded. Legacy SSE negotiates
its message endpoint inside the stream, where we never see it, so the
origin rule is the narrowest available — and it is exact rather than
approximate, since the SDK enforces that the message endpoint shares
the stream URL's origin.
The residual gap is therefore confined to legacy SSE against a server
hosting its own OAuth endpoints, and it is stated in the docblock and
pinned by a test rather than left implicit. Neither alternative was
reachable: the SDK takes a single fetch for all network requests, so
there is no separate auth seam to route through, and the negotiated SSE
endpoint is parsed inside the SDK's own stream handler.
Fails open on every uncertainty, including not knowing the transport —
a path rule applied to what turns out to be SSE severs real MCP
traffic, so an absent or throwing hint falls back to the origin rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
exemptMcpEndpoint compared origin + pathname, which misses legacy SSE
entirely: SSEClientTransport opens the configured URL and is handed a
SEPARATE message endpoint by the server, then POSTs every JSON-RPC
message to that second pathname. Those were bounded and body-buffered
as OAuth traffic, so a slow tool call over SSE would be severed at 30s
and reported as an OAuthRequestTimeoutError.
It compares origin alone now. That is exact rather than approximate:
the SDK enforces that the SSE message endpoint shares the stream URL's
origin and rejects one that does not, so every MCP request this chain
carries is same-origin with the configured URL by construction.
The cost, stated in the docblock and pinned by a test: an OAuth
endpoint on the MCP server's own origin is exempt here and so unbounded
on this chain. The OAuth chain still bounds the Inspector's own
requests to it, so only the SDK's transport-internal OAuth against a
same-origin server falls back to where it was before #2319.
Caller cancellation was also still swallowed in two later places in the
compat probe loop — `probe.text()` has its own catch, and `fetch`
resolves on headers, so an abort mid-body landed there and was answered
with the preceding 404; and an abort during releaseBody's await would
have carried on probing. Both recheck the signal now, and all three
sites throw the caller's own `reason` rather than whatever shape the
layer beneath rejected with.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
Two real bugs and two stale rationales.
The OIDC fallback probes were issued with `{ headers }` and no signal,
so a caller that aborted after the initial 404 got a timeout reported
in place of its own cancellation, while the probe ran on to its own
deadline. The probes now carry the caller's signal — via the shared
`callerSignalOf`, so the precedence rule has one definition — a
pre-aborted caller is not probed at all, and an abort is rethrown
rather than folded back into the preceding response.
`Math.max(0, Math.round(timeoutMs))` let NaN and Infinity through.
`setTimeout(fn, NaN)` fires immediately, and a delay above 2**31-1
overflows a 32-bit signed int and falls back to 1ms — so a caller
asking for a very large budget, or one whose arithmetic produced NaN,
got an immediate OAuth timeout. `normalizeBudget` falls back to the
default on non-finite and clamps to the timer's range; the route's
envelope-supplied deadline gets the same clamp, but ignores a
non-finite value outright since a malformed deadline reads safest as
no deadline.
The wire-marker docblock and the matching route test still said a
backend deadline is the only one on the transport-internal discovery
path, which round 11 changed. Both now give the real reason the marker
matters: both ends run the same budget, and it is the backend winning
the race — a backgrounded tab throttling setTimeout — that must not
decide whether the error names its endpoint.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
Three comments still said transport-internal discovery has no
client-side wrapper, or that the wrapper must never be applied to the
transport fetch. Round 11 made both false, and the second actively
tells a caller to do the opposite of what InspectorClient does — while
hiding that isExempt is the safety-critical part.
- createRemoteFetch's marker comment now gives the real reason the
reconstruction matters: both ends run the same budget, so this path
is the backend winning the race, most plausibly a backgrounded tab
throttling setTimeout while the server's fires on schedule. Which
deadline fired must not decide whether the error names its endpoint.
- The body-buffering comment now says buffering is the second reason
isExempt is not optional on a mixed chain, since an exempt request
skips it and an SSE body stays open rather than being drained.
- The matching test comment is reframed as testing the proxy boundary
independently of which deadline wins.
The PR description is rewritten around what is actually shipped: both
chains bounded and differing only in the exemption, the per-request
envelope that replaced the fetchTimeoutMs option, and the three
mutation-verified tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
The server-side timer added in round 9 applied to every /api/fetch
request. But that route carries MCP traffic as well as OAuth work, so a
Streamable HTTP tool call that legitimately withholds its response
headers past the budget was aborted there — reintroducing on the
backend exactly the regression exemptMcpEndpoint prevents on the
client, and reporting it as an OAuth timeout besides.
The caller now decides. `withOAuthRequestTimeout` stamps its budget on
the RequestInit it hands down, `createRemoteFetch` reads it back and
puts it in the /api/fetch envelope, and the route applies a timer only
when one is present — so an exempt MCP request, which never gets an
init of its own, is bounded by nothing.
The carrier is a WeakMap keyed on the init rather than a header:
serializeRequest copies request headers verbatim into the payload and
the route re-sends them to the authorization server, so a marker header
would leak to a third party. It needs no cast onto RequestInit and is
collected with the object.
The `fetchTimeoutMs` option from round 9 is gone — the deadline now
lives in exactly one place, the client wrapper, which is also what
makes the 504 marker honest: only a request the OAuth wrapper bounded
can produce one.
Ten tests: the stamping and its rounding, nothing stamped on an exempt
call or an init that never went through the wrapper, the envelope
carrying the budget (and not a header), its omission on both exempt and
direct calls, and at the route the 504 marker, no deadline without one,
and a non-numeric deadline ignored. The no-deadline case is verified by
mutation: an unconditional timer makes it fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
Copying url/redirected/type onto the rebuilt response as enumerable own
properties changed what a consumer sees: `Object.keys`, object spread
and `JSON.stringify` are all affected. Which answer is correct turns
out to be the host's business — under the Fetch standard these are
prototype getters with no own enumerable key, while happy-dom defines
them as own enumerable data properties — so the descriptor is copied
from the freshly built Response rather than either answer being
hardcoded. The test compares against a baseline built in the same
runtime for the same reason.
Three comments still described the transport fetch as unbounded, which
round 11 changed:
- the wrapper's own docblock said it must never wrap the transport,
obscuring that isExempt is what makes doing so safe;
- the auth-chain comment in inspectorClient repeated it;
- buildEffectiveAuthFetch's docblock said transport-internal discovery
is not covered by a deadline.
All three now state the real distinction — both chains are bounded, and
they differ only in that the transport chain exempts the MCP endpoint —
and the docblock says why reusing the transport chain here would be
wrong rather than merely different.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
