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>
The transport chain carried the SDK's own OAuth work — the discovery it
runs from inside the transport on the 401/refresh path — and that leg
was left to the SDK's incidental per-request timeout, reporting a bare
`Request timed out` naming no endpoint. On a direct (non-proxied) path
the /api/fetch backstop cannot reach it either.
`withOAuthRequestTimeout` takes an optional `isExempt` predicate; an
exempt request passes through completely untouched — no deadline, no
signal, and no body buffering, which is what a stream needs.
`exemptMcpEndpoint` exempts the MCP endpoint by origin + pathname, and
FAILS OPEN: no server URL, or either side unparseable, means exempt.
The errors are not symmetric. Bounding what should not be bounded
severs a long-running tool call withholding its headers, or an SSE
body; failing to bound leaves the SDK's timeout as the backstop it
already was.
Two test corrections, both real:
- The metadata-preservation case asserted url/redirected/type against a
synthetic Response, whose values are the rebuild's own defaults — so
it stayed green with the defineProperty loop deleted. It now seeds
non-default values first; verified by mutation that removing the loop
makes it fail.
- Two signal assertions compared against the controller rather than
`request.signal`. The Fetch standard gives a Request a dependent
signal, so those encoded non-standard identity.
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>
requestTimeout.test.ts had no file-level header, which AGENTS.md
requires of every file; it now explains what each group of cases pins
and why, including why the composition cases are written as a contrast.
The two mock fetches I added used `as unknown as typeof fetch`, which
AGENTS.md rules out without an inline justification. A contextually
typed forwarding function keeps the vitest mock inspectable and needs
no cast at all.
The PR description still described the pre-fix cancellation flow; it is
rewritten to say that the signal now reaches the backend's outbound
fetch and what the race is still for.
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>
A deadline enforced by the backend could not arrive as an
OAuthRequestTimeoutError: /api/fetch serialized it as a 500 and
createRemoteFetch turned that into a plain Error, so every instanceof
check downstream was false — including the one that lets a stalled
probe escape withRfc8414OidcCompat. That matters most where no
client-side wrapper exists to have fired first: the discovery the SDK
runs from inside the transport. The route now answers 504 with a typed
marker (OAUTH_TIMEOUT_WIRE_CODE plus url and timeoutMs) and
createRemoteFetch reconstructs the error from it, guarded by a shape
check so an upstream's own JSON cannot forge one.
The route's deadline becomes a `fetchTimeoutMs` option defaulting to
the shared constant, so the backstop can be proven to fire in
milliseconds instead of thirty seconds of wall clock — and because a
very slow authorization server is a legitimate reason to want a longer
one.
The rounding rationale was wrong: setTimeout accepts a fractional delay
and truncates it (verified on Node 26); ERR_OUT_OF_RANGE is
AbortSignal.timeout's behaviour, which this wrapper never calls. The
rounding stays, for the reason that is actually true — the budget is
interpolated into the message and exposed as timeoutMs, and a
performance.now() budget would otherwise report 1000.4000000953674ms.
Also drops a sentence in the discarded-stream test that contradicted
the qualification above 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 /api/fetch route classifies an event-stream or NDJSON response as a
stream and then answers with JSON and no body — so nobody downstream
owns the upstream stream and nothing will ever read it, while the
`finally` clears the only deadline on the way out. It is now cancelled
explicitly, best-effort, the way `releaseBody` in oidcDiscoveryCompat
already does. Measured, undici reclaims an unread body here on its own,
so this is belt and braces rather than a demonstrated leak — kept
because relying on that is an implementation detail of the fetch
beneath us, not a contract. The route test and the source comment both
say so rather than implying a stronger claim.
Forwarding the signal also made revocation.ts's rationale stale: it
still described a proxy that drops `init.signal` and a backend that
cannot cancel. Both comments there, and the matching one in
revocation.test.ts, now describe the post-#2319 path — the signal
cancels, the race guarantees, and what the race is still for.
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>
Copilot review round 5. Two of three findings taken; the third rests on a false
premise about the SDK and is declined with evidence in the PR thread.
The hint and the input description are part of this fix's user-facing contract —
an HTTPS-only instruction there puts the local-fixture workflow back out of
reach even while the validator stays permissive — and nothing asserted them.
Two cases now do, plus one asserting a loopback http URL raises no error in the
form. Each mutation (restoring either piece of old copy) fails exactly one of
them.
The docs said there was "no loopback exemption at all" before #2305, which
contradicts the sentence above it: the runtime already tolerated an `http://`
already-stored `client_id`, and that asymmetry is what the issue is about. The
gap was in the config/form validator, and the wording now says so.
Also added: a note to use the canonical spelling of the host. `127.1` and
`2130706433` are accepted here and by the SDK, since both canonicalize to
`127.0.0.1` — but a CIMD `client_id` is string-compared by the authorization
server against the URL it dereferenced, so an exotic spelling is a real footgun
independent of what either validator allows.
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>
Three findings, all real.
`arrayBuffer()` hands back the decoded entity body, so the rebuilt
response was carrying the upstream `content-encoding` and the
compressed `content-length` — a gzipped metadata document came back as
plain JSON bytes still claiming gzip and the wrong size, visible in the
Network capture. Both headers are deleted now, as `responseWithBody` in
endpointOverrides.ts already did for the same reason.
The "why a race, and not just the signal" rationale described a world
this PR had since changed: the signal does now reach the backend's
outbound fetch. The module doc and three implementation comments are
rewritten to say what the race is actually still for — a fetchFn that
ignores AbortSignal, and a backend that is itself wedged — rather than
a proxy hop that no longer drops it.
The backend cancellation path had only mock-level coverage. A
route-level test drives /api/fetch at an upstream that accepts and never
answers, aborts the browser side, and asserts the upstream observes its
connection closing. Verified by mutation: dropping `signal` from the
route's outbound fetch makes it time out rather than pass.
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>
Copilot review round 4, four findings, all correct.
The CIMD-off / CIMD-on pair the smoke guide documents is not a control without
clearing OAuth state first: tokens and the registered client persist in
oauth.json independently of the install-wide CIMD toggle, and the Inspector
reuses valid stored tokens before prompting — so a leftover grant from an
earlier CIMD-on run makes the CIMD-off phase connect and the negative control
silently proves nothing. Both the spec's pair and the walkthrough in
docs/test-servers.md now say to clear state (or isolate MCP_STORAGE_DIR) first.
The composable server announces its MCP endpoint, not the metadata document —
`server-composable.ts` prints a URL ending in `/mcp`. The guide called the
announced URL a metadata URL; it now says to take the origin from that line and
append `/client-metadata.json`.
Two general statements elsewhere in the same file still defined CIMD as always
HTTPS (the glossary row, and the Stytch section's opening), so a reader
following them would have rejected the loopback fixture the same file now
documents. Both note the exemption; the Stytch examples stay HTTPS, which they
are.
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>
Two findings, both real.
The race bounded only the caller's promise on the browser path.
`createRemoteFetch` dropped the signal, so an abort — including the
deadline's own — left the POST to /api/fetch in flight; the backend
never saw its request cancelled and its outbound fetch ran on with
neither signal nor timeout, holding a handler and an authorization-server
socket per timed-out attempt. `createRemoteFetch` now forwards the
caller's signal (init.signal when present and not undefined, else a
Request's own), and the /api/fetch route composes `c.req.raw.signal`
with a server-side deadline matching the client budget, cleared in
`finally` so a streaming response is not severed.
A probe's own timeout was swallowed by `withRfc8414OidcCompat`, which
returned the preceding 404 and discarded the endpoint name the deadline
exists to provide. A deadline the Inspector imposed is its own error,
not the server's answer, so it escapes now; every other probe failure
still falls back as documented. Nothing regresses: every candidate the
loop probes is one the SDK's own buildDiscoveryUrls emits, so a stall
there is one the SDK would have hit itself, unbounded.
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>
Copilot review round 2.
The exemption compares the WHATWG-canonicalized `URL.hostname`, so it covers
alternate spellings of three loopback *addresses* rather than three input
strings: `127.1`, `2130706433`, `0x7f.0.0.1`, `[0:0:0:0:0:0:0:1]` and a
root-anchored `127.0.0.1.` all pass. The behavior is right — the security
argument is about the address the request reaches, and all of these are the
loopback interface — but the comment described it as a literal-input match,
which it is not. It now states the contract, and six regression cases pin the
aliases alongside the existing near-miss cases.
This is also still an exact match for the SDK: `assertSecureTokenEndpoint` tests
`url.hostname` against the same three values after the same `new URL`
canonicalization, so an alias accepted here is one the SDK accepts too.
Separately, the smoke guide's fixture recipe left the shell in `clients/web`
after the build line, so the repo-root-relative paths on the next line resolved
under `clients/web` and the copy-pasted setup failed before the server started.
The build now runs in a subshell, and the block notes that the server announces
its URL on stderr and walks past a taken port.
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>
`buildEffectiveAuthFetch` took `this.fetchFn` as its base, which already
carried `withRfc8414OidcCompat` — so the deadline sat outside it. A
stalled OIDC probe was then timed on whatever budget the preceding RFC
8414 request had left, and reported under that request's URL rather
than the probe's, defeating the endpoint-level diagnostic this change
exists to add.
The auth chain is now composed separately, deadline innermost, matching
the CLI's storedAuthFetch:
withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides))
`this.fetchFn` keeps the transport chain, unbounded, so an SSE or
Streamable HTTP body is still free to stay open. Two tests pin the
ordering by contrast: outside, a stalled probe is misreported under the
preceding URL; innermost, it is bounded on its own budget and cannot be
attributed to the RFC 8414 request.
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>
`specification/v2_auth_smoke_testing.md` said `ensureCimdClientRegistration`
allows `http://127.0.0.1` metadata URLs "in tests only". That was accurate while
the exemption lived only in the runtime's already-stored-`client_id` path; after
this PR a loopback `http://` URL is a valid persisted `client.json` setting and a
valid entry in the settings form, so the sentence contradicted the workflow the
same guide asks a smoke tester to run (Copilot review round 1).
The same paragraph's recipe was stale in two other ways it is cheaper to fix
here than to leave next to a corrected claim: `cd test-servers && npm run build`
names a manifest that does not exist, and `oauth-cimd-only.json` is not a config
in this repo. Both now point at the shipped `oauth-cimd-http.json` fixture and
the real build command.
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>
Both findings are on the caller-signal handling added in round 1:
- `"signal" in init` treated a member present as `undefined` as an
explicit override. `RequestInit` is a WebIDL dictionary, where such a
member converts as absent — so `{ ...base, signal: undefined }`, which
is what a spread over an options object with no signal produces, was
clearing a Request's embedded signal instead of inheriting it. Only a
non-`undefined` value overrides now; an explicit `null` is still the
genuine no-signal case.
- An already-aborted caller signal rejected the race, but
`Promise.race` still evaluated `fetchFn(...)`, so on the
signal-dropping proxy path the OAuth request went out anyway. It now
short-circuits before anything is constructed or sent, and the
now-unreachable aborted branch inside the race is gone.
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>
`getCimdClientMetadataUrlError` rejected every non-HTTPS CIMD client metadata
URL with no loopback exemption, and the same check runs against `client.json`
on disk through `refineCimdMetadataUrl`, so it could not be worked around by
hand-editing the file either. Every test server in this repo speaks plain HTTP,
so CIMD could not be driven against any local fixture: #2242 shipped verified by
tests alone, and reproducing it by hand meant standing up a self-signed HTTPS
listener plus `NODE_TLS_REJECT_UNAUTHORIZED=0` in the test server's environment.
Accept plain `http:` on the three literals the SDK itself exempts from its
token-endpoint TLS assertion — `localhost`, `127.0.0.1` and `[::1]`. That is
option 1 in #2305 and matches what the runtime already tolerated:
`core/auth/cimd.ts` notes that an already-stored `client_id` may be an `http://`
URL used by local dev/test metadata servers. Anything outside those three,
`localhost.` and `tenant.app.localhost` included, is still rejected — matching
the SDK literal for literal keeps the two allow-lists from disagreeing about one
URL.
The exemption is a literal set rather than `isLoopbackHost` because
`core/client/config-parse.ts` is browser-safe and `core/node/hostUrl.ts` imports
`node:net`.
The field error now names the exemption instead of saying only "must use
HTTPS", and the form's hint and description say the same, so a developer
pointing the field at a local fixture is told which spellings work.
Closes#2305
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>
Three findings, all real:
- The deadline stopped at the response headers. `fetch` resolves once
headers arrive, so a server that sent headers — or half a JSON
document — and then stalled left the caller's `response.json()`
hanging with nothing watching it. The body is now buffered under the
same race and the response rebuilt around it, preserving `url`,
`redirected` and `type`, which have no constructor option, and
passing `null` for a null-body status.
- The caller's signal was only forwarded, not raced. On the browser's
`createRemoteFetch` path the signal is dropped in flight, so an abort
left the promise pending for the whole budget rather than the outer
cancellation winning as documented. It is raced now, with the
caller's own `reason` preserved, and the listener removed in
`finally` since one caller signal outlives several requests.
- A `Request` carries its signal on the input, not in `init`, so always
passing our own `signal` in `init` silently overrode it. The caller
signal is now derived from the `Request` when `init` has no `signal`
key — and an explicit `null` there still wins, per the fetch spec.
Eight new tests cover each: the stalled body, caller cancellation
against a signal-ignoring fetch, an already-aborted signal, the
Request-embedded signal, the explicit-null override, listener removal,
the rebuilt response's metadata and a null-body status.
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>
Protected-resource-metadata discovery, authorization-server metadata
discovery, dynamic client registration, the token exchange and the
refresh all went out with no AbortSignal at all. Token revocation was
the single exception, and its withDeadline race is the pattern this
generalizes.
Adds core/auth/requestTimeout.ts: withOAuthRequestTimeout wraps a fetch
so every call through it is bounded, composing any caller signal with
AbortSignal.any and racing a timer so the deadline holds even where the
signal is dropped in flight (the browser's remote fetch re-issues the
call to /api/fetch and forwards no signal). The rejection is an
OAuthRequestTimeoutError naming the URL, so a stall says which endpoint
stalled rather than riding the SDK handshake timeout and reporting
"Request timed out".
Applied inside InspectorClient.buildEffectiveAuthFetch rather than to
this.fetchFn, because the transport is handed this.fetchFn directly and
a deadline on a Streamable HTTP / SSE response would sever the stream it
is supposed to hold open. Also applied to the CLI's storedAuthFetch,
which calls SDK discovery directly and so has nothing else bounding it.
Closes#2319
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>
Validate `oauth.clientMetadataPath` the way the other two metadata paths
are validated — in `loadConfig` for the JSON route and again at server
setup for a `ServerConfig` built in code. This path is not merely
advertised: it becomes the served document's own `client_id`, so an
off-origin or query-bearing value would publish a client id this server
cannot honour.
Derive that `client_id` from `req.originalUrl` rather than from the
registered route, so a document fetched with a query string answers with
a `client_id` byte-identical to the URL it was fetched from — the
equality CIMD turns on.
Correct the docs' fixed-port warning: this fixture configures no
`issuerUrl` and derives the `client_id` from the request, which is
exactly why the integration test can drive it on a harness-chosen port.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UWGH2ytPzSPxnp4cibaV3Z
Signed-off-by: cliffhall <cliff@futurescale.com>
Moves `vitest` and `@vitest/coverage-v8` (root) and
`@vitest/browser-playwright` (web) from 4.1.10 to 4.1.11, clearing the
moderate, dev-only path-traversal advisory against `@vitest/mocker`.
The three pins alone do not resolve: `@vitest/browser-playwright`
declares an exact peer on `vitest`, web does not declare `vitest`
itself, and web's lockfile pins that peer-shadowed copy at 4.1.10 — a
knot npm refuses rather than resolves. Regenerating web's lockfile
clears it but re-resolves the whole tree, so this instead adds a
`vitest` `overrides` entry to `clients/web/package.json`, the mechanism
AGENTS.md already prescribes for a transitive that will not move.
Churn stays inside the vitest constellation: ten `@vitest/*` + `vitest`
entries to 4.1.11, plus their own transitives (`obug`, `tinyexec`,
`chai`). `clients/{cli,tui,launcher}` lockfiles are untouched, and
`npm audit` reports no advisory in any of the five installs.
AGENTS.md records the fourth edit, since the three-pin rule as written
is necessary but not sufficient.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BkS1cKEesxXwSckjBpJBt6
Signed-off-by: cliffhall <cliff@futurescale.com>
In CIMD (SEP-991) the `client_id` *is* a URL the authorization server
dereferences to learn the client's metadata. Nothing in this repo served such a
document, so a fixture could advertise `client_id_metadata_document_supported`
and still be unusable — exercising CIMD meant standing up a second host by hand.
That is why #2242 shipped verified by its tests alone, and why the v2.6.0
release smoke recorded it as the one contribution with an observable UI surface
that could not be reached.
The composable server now hosts the document itself when `oauth.clientMetadata`
is set, gated on `supportCIMD` — advertising a client the server would then
refuse to honour is a worse fixture than serving nothing. The document's
`client_id` is derived from the **request** rather than from a configured
issuer, so it stays correct when the harness picks the port, as the integration
test does.
`supportDCR: false` in the fixture is load-bearing rather than incidental. With
DCR available a CIMD failure silently succeeds via dynamic registration and the
reproduction proves nothing — during development a misconfigured run connected
happily and reported `Dynamic (DCR)` with a `test_client_…` id, which reads as
success until you check the client id. With DCR off, CIMD is the only path that
can complete, so reaching a connected state is itself the assertion; the test
pins the absent `registration_endpoint` so it stays that way.
Verified end to end against the fixture: Connection Info read
`Client registration — Client ID Metadata (CIMD)` with the client id equal to
the metadata URL. That drive needed a self-signed HTTPS listener, because the
Inspector requires the CIMD metadata URL to be HTTPS with no loopback
exemption — filed separately as #2305, since it is a validation gap rather than
a fixture one.
Closes#2306
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi
Signed-off-by: cliffhall <cliff@futurescale.com>
Ahead of the v2.6.0 milestone merge into `main`. The bump belongs on
`v2/main` so it flows into `main` with the rest of the milestone's work —
doing it on the merge branch instead leaves `v2/main` reading a stale
version and lets the bump leak into unrelated PRs (#2010).
There is one version number in the repo; the clients carry none. No tag is
created here: the release tag points at the merge commit on `main`.
Closes#2300
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi
Signed-off-by: cliffhall <cliff@futurescale.com>
`npm audit` at release time reported hono <=4.13.4 against three moderate
advisories: GHSA-gqvv-2mrq-wpjv (toSSG() writes outside the output dir),
GHSA-g6gw-c38x-mqfc (unbounded dot-notation nesting in parseBody() can
exhaust memory) and GHSA-crvj-82cr-hjcx (the query parser reads parameters
after the URL fragment, creating cache-key and proxy differentials).
hono is a runtime dependency of `core/`, so the fix raises the declared
floor rather than only moving the lockfile: a published install resolves
this range from the root manifest, and `^4.13.1` would still have let a
consumer land on a vulnerable 4.13.x.
Refs #2300
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi
Signed-off-by: cliffhall <cliff@futurescale.com>
The release skill described a v2 release as three steps and left four
things a release actually depends on implicit or absent: that it is two
pull requests against two different bases, the production smoke of the
release candidate, the ledger artifact the maintainers review, and where
a smoke finding gets fixed. It also gave only the `git tag` path, which
is the fallback rather than the norm.
Restructure it around PR 1 -> PR 2 -> Release:
- a "The shape" table contrasting the two PRs by branch, base, contents,
verification and merge condition, plus why they must not be folded
together and why PR 1 merges before PR 2 is opened;
- step 2 broken into the tree-hash check and production smoke, the
ledger's structure, and the fix-on-`v2/main` rule that keeps the merge
tree byte-identical to `origin/v2/main`;
- step 3 leading with the GitHub UI path, keeping the CLI commands as the
by-hand equivalent.
Docs only. The `npm audit fix`, `--no-git-tag-version`, tag-`origin/main`,
no-`v`-prefix and #2010 warnings are all preserved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings, all real.
- No catalog-level bound. The per-skill caps bound what ONE entry can
cost; nothing bounded how many entries there are, and SEP-2640 puts no
ceiling on a catalog — every entry costs at least one resources/read,
so a large listing made `--verify` run indefinitely and transfer
unboundedly. Adds SKILL_MAX_CATALOG_SKILLS / SKILL_MAX_CATALOG_BYTES.
Entries past the budget are still reported, with their static findings
and an `incomplete` reason, rather than dropped or failed: they were
not checked, which is neither a pass nor a verdict against the server.
- `responseBytes` charged `text.length`, which counts UTF-16 code units.
That undercharges non-ASCII by up to 3x, so a decoy block of emoji kept
the counter under 16 MiB while the wire carried far more. Adds
`utf8Length`, an allocation-free UTF-8 byte count — deliberately not
TextEncoder, which would copy a payload the server chose the size of.
- `checkSkillNameCollisions` was O(N^2) in both work and output for a
group of N: every entry filtered all N URIs and embedded the other
N-1. Duplicate names are legal and a server controls N. Now names a
bounded sample and counts the rest.
- `--verify` help documented exit 7 but not exit 8.
The three-tier bounding (per skill, per skill on the wire, per run) is
now a table in clients/cli/README.md, since the run bound is this tool's
limit rather than the spec's and should not read as a conformance rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all in paths hardened by earlier rounds.
- The 16 MiB read budget was charged only after a content block had
matched by URI *and* decoded. A server could answer every manifest row
with one enormous block labelled a different URI (so `contentsFor`
found nothing) or one enormous invalid-base64 blob (so `skillFileBytes`
threw), bank zero against the cap, and have the walk issue up to 512
more. `responseBytes` now charges the raw response before selection or
decoding; the exact decoded length is substituted when there is one, so
the common path is still measured precisely.
- The Directory section checked that a child was inside the skill root
but not that it was a child of the directory actually being read. A
grandchild, or the directory echoing itself back, passed containment
and was rendered as navigable. Non-direct entries are now shown and
labelled rather than linked — `resources/directory/read` answers with
direct children, so an entry that is not one is itself the finding.
- Containment was decided on the normalized URI while navigation sent and
stored the raw one. For `skill://r/a/../templates` the first Up produced
`skill://r/a/..` and a second walked into `skill://r/a`, a directory
nothing had validated. Directory descent passes the normalized URI, and
the Up arithmetic moved into `parentOfSkillUri`, documented as valid
only on a normalized URI.
The suppressed comment (two stderr lines on a failing `--verify`) is
declined again: `--strict` produces the same shape in `emit-result.ts`,
and clients/cli/README.md already documents it as such.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
