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>
Four of the five findings share one root cause: the tri-state `outcome`
added in round 15 was not propagated to consumers, which kept branching
on `ok` — true for an `incomplete` report, since nothing that was checked
was wrong.
- CLI `summarizeSkillVerification` counts off `outcome`, not `ok`, so a
truncated walk no longer prints "no conformance errors" one line before
exiting SKILL_INCOMPLETE. A mixed failed/incomplete catalog reports
both counts rather than letting the louder verdict hide the quieter.
- `verifySkills` sets the truncation reason only when entries were
actually left unread. A file crossing the byte budget as the FINAL
entry stopped nothing, and "Stopped after 3 of 3" both read as a
contradiction and demoted a fully-read skill out of `verified`.
- The TUI status line switches on `outcome` through a `Record` over the
union, so the INCOMPLETE arm is reachable and a fourth outcome would
be a type error rather than a silently missing label.
- The `incomplete` doc paragraph said it makes `ok` false. It does not,
deliberately — corrected, and it now names `outcome` as the thing a
consumer must branch on.
- The web sidebar composes in the per-URI `duplicate-name` warning, so
two colliding skills are badged in the catalog instead of looking
clean until one is selected.
The suppressed `selfAttempted` finding is stale — fixed in round 16.
The existing TUI INCOMPLETE test was passing for the wrong reason: its
fixture understated every size, which is itself a mismatch, so the report
was `failed` and only the old `ok`-first branch printed INCOMPLETE. It is
rebuilt with honest digests and sizes, so truncation comes from the
declared-size prefilter and the report is genuinely incomplete.
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>
**A hole my own round-13 change opened.** The byte budget added a `break`,
and `manifestListsSelf` described the bounded slice rather than the rows the
walk reached — so breaking before a later self-entry left the flag true,
suppressed the fallback, and skipped the mandatory frontmatter check
entirely. It is `selfAttempted` now, set inside the loop, and marked before
the read: a row the walk reached but could not read has still been attempted,
and its failure belongs to the loop rather than to a second fetch.
**A preview could overwrite a verification's own bytes.** The digest verdict
on screen was computed from the verification's fetch; replacing only the text
let the frontmatter findings describe different bytes, recreating the
mixed-fetch verdict that state exists to prevent. `entryTextVerified` marks
which read produced it, and a preview no longer wins over a verification.
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>
**Verification is a tri-state now, because two states could not be right.**
Round 12 was correct that `ok: true` for a manifest whose unread 513th file
may be tampered with is a false pass. Round 15 is correct that forcing `ok:
false` calls a server nonconformant for exceeding limits SEP-2640 states as
SHOULD NOT, with hosts free to support more — contradicting this module's own
rule that a warning never fails a report. Both hold, so:
verified -> exit 0 everything checked, everything passed
failed -> exit 7 a MUST was broken
incomplete -> exit 8 nothing checked was wrong; the walk was cut short
`ok` keeps its narrow meaning, `allSkillsVerified` is stricter than
`every(r => r.ok)` since it selects the exit code, and `anySkillFailed`
separates 7 from 8. A job that tolerates oversized catalogs can allow 8 and
still fail on 7.
**`manifestKey` still used `JSON.stringify` on the entry** — the same crash I
fixed in the TUI in round 12, one file over.
**Directory children were navigable outside the skill root.** A server could
return `skill://other-skill/...` and clicking it left the tree, with "Up"
only comparing equality against the root. Containment is checked on the
normalized URI now, and an outside child renders as text rather than a link.
Worth recording from the tests: `..` cannot escape the authority, so
`skill://a/nested/../x.md` resolves back inside and must stay navigable.
**The TUI matched report rows by raw URI**, which missed a row recorded under
an equivalent spelling while `extraReportFiles` suppressed it as covered.
Normalized, like the membership test beside it.
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>
**The TUI ignored `incomplete`.** Round 12 added that field precisely so a
consumer could tell "not fully checked" from a real failure, and then the one
client that reads the report did not consume it — which made the field
decorative. The pane shows an Incomplete: block with the report's own reason,
and the status line reads INCOMPLETE rather than FAILED.
It sits ABOVE the manifest, not beside the status line: it explains the list
that follows — only the first N rows were fetched, the rest stay marked `·`
because nobody looked at them — and below a 512-row manifest it would be
off-screen, which is the same as absent. Found by writing the test, whose
first version asserted against a 600-entry fixture and could see neither
string. The test triggers truncation through the BYTE budget instead, so the
manifest stays three rows and the assertions measure the pane rather than the
test's viewport.
**The capped self-entry was reported under the wrong URI.** A manifest may
write its self-entry in a normalized-equivalent form; the fallback recorded
`entry.uri`, so manifest rows keyed on the declared spelling matched nothing
while the normalized extra-files filter suppressed it as already covered. The
verdict existed in the report and appeared nowhere on screen. Recorded under
`declaredSelf.uri` now.
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>
**The byte budget trusted server-declared sizes.** A manifest advertising
`size: 1` — or, since the wire schema deliberately accepts it for reporting,
no size at all — sailed through the declared budget and then served
arbitrarily large bodies. The 16 MiB safeguard protected against an honest
server only, which is no safeguard.
The walk now tracks bytes ACTUALLY received and stops issuing reads once the
real total crosses the cap, marking the report `incomplete` and so `ok:
false`. The declared prefilter is kept rather than replaced: it refuses to
schedule an obviously oversized set, and the received-byte counter stops one
that lied. The count happens after verifying the crossing file, so that file
is still reported rather than fetched and discarded.
⚠️ This bounds the total across responses, not any single one: a first
response larger than the cap is already in memory before it can be measured,
which would need a streaming read the client API does not expose. Stated in
the code rather than left to look closed.
**A modern `skills/get` did not require `resultType`.** The "left open"
question SEP-2640 states covers `ttlMs` / `cacheScope` and only those;
`resultType` is base-protocol per SEP-2322 and appears in the SEP's own
`skills/get` example. Requiring it of `resources/directory/read` but not here
was an inconsistency in the module rather than a distinction the spec draws.
Era-selected now, with the caching attributes still optional.
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>
**The read bounds could produce a false `ok: true`.** Entries past the cap are
never fetched, `fileFailed` only examines fetched rows, and
`resource-limit-exceeded` is a WARNING — so a manifest whose 513th file was
tampered with reported the skill verified. That trades a denial of service
for a wrong answer, which is the failure this whole PR argues against. A
truncated read is now recorded as `incomplete` and forces `ok: false`,
reported separately so a consumer can tell "this skill is wrong" from "this
skill was not fully checked". And a self entry excluded by the cap is now
verified against its declared digest, not merely read for frontmatter —
otherwise the skill's own SKILL.md was the one file nobody checked.
**`entryKey` could crash the TUI.** It called `JSON.stringify` on the whole
server-controlled entry, during render. `skillEntryKey` is decided by the
same `jsonGraphError` guard that bounds the frontmatter comparison, with a
coarse fallback that still separates two unrepresentable entries by URI —
collapsing them would show one skill's verdict under another's name.
**The tab bar wraps and `tabsHeight` was hard-coded to 1.** An OAuth-capable
stdio server serving Skills needs ~107 columns, so it wraps at 132 as well as
at 80, and every pane below was sized a row too tall. `tabBarRows` derives it
from the same list `Tabs` renders, via a shared `visibleTabs(flags)` — the
duplicated filter being how the two would drift apart again.
Nine tests in a new `tabsConfig.test.ts`, including the 80-column regression.
Two assert properties rather than instances: rows never decrease as the
terminal narrows, and a tab wider than the row gets its own rather than
looping.
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>
**Reads were bounded by entry count but not by total bytes.** A manifest can
sit at exactly 512 entries and declare a gigabyte each, so round 9's cap
still let a server dictate unbounded bandwidth and time after
`size-limit-exceeded` had already been reported. `boundedManifest` caps on
both interoperability limits now.
Sizes are summed exactly as `totalSkillBytes` sums them, so the bound and the
finding that reports the overage cannot disagree about the total. A read is
skipped only when the running total would CROSS the limit, so a conforming
skill — at most 16 MiB by definition — is never truncated; there is a test
for that, because a bound that protected against a hostile server by giving a
wrong answer about a good one would be a poor trade. The self-file fallback
still applies, so the mandatory frontmatter check survives either cap.
**The `vitest.shared.mts` pin rationale had gone stale.** It said `yaml` is
reached only through `test-servers/src`, which this PR made untrue by
importing it from `core/mcp/skillFile.ts`. That comment is what a future
dependency-placement change would read before removing or reclassifying the
pin, so it now names the `core/` path and the bundler `external` lists that
follow from it.
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>
Round 10 raised no new inline comments; three suppressed findings, two fixed
and one declined with a reason.
**`Object.is` held `0` and `-0` distinct**, so a listing carrying JSON `0`
against a served YAML `-0` produced a mismatch — reported as "the listing
says 0 but the served SKILL.md says 0", a false finding with an
unintelligible explanation. JSON does not distinguish them, so neither may
this: `===` for finite numbers, `Object.is` kept only for the non-finite
cases it was introduced for.
**Browsing a supporting file erased an observed failure.** The frontmatter
check ran off whatever the viewer was showing, so opening a second file made
`showingSkillMd` false and dropped the finding and its error count, with the
skill unchanged. The entry's text is skill-scoped now, written by whichever
read produces it first — the on-selection preview or a verification — and
invalidated only with `manifestKey`. That also subsumes the earlier
same-fetch requirement rather than sitting beside it.
**Declined: the second stderr line on exit 7.** `--verify` writes its summary
and then the ordinary `ErrorEnvelope` that EVERY non-zero exit writes, which
is the documented contract and exactly what `--strict` does. Suppressing it
would make this one command's failure output unparseable by a caller
branching on `.code`. What was wrong is the README, which implied stderr
carries only the summary; it now states both lines and why.
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>
**Verification reads were unbounded.** The 512-entry limit is checked by
`checkSkillConformance` — as a warning, since the SEP makes it an
interoperability bound rather than a MUST — but checking it constrains
nothing, so a hostile server advertising a million entries had the tool
perform a million sequential reads after the report already knew the manifest
was over. Capped at `SKILL_MAX_RESOURCE_ENTRIES`, with the finding still
reported so bounding the work does not silence the reason it was
unnecessary. `manifestListsSelf` is computed over the READ slice, so a
self-entry pushed past the cap still reaches the fallback: the frontmatter
comparison is mandatory and must not be lost to a limit that bounds other
files.
**A malformed skill URI still produced a directory root.** `skillUriIdentity`
falls back to the raw string, so `not a uri/SKILL.md` yielded the root `not a
uri` and enabled the Directory section — letting the UI build a request from
a URI the conformance checks had already rejected. The comment above it
claimed otherwise. `normalizeSkillUri` now, with the rule written down:
identity is for COMPARING two spellings, not for deciding a URI is
well-formed enough to build a request from.
The existing regression test passed against a weaker input that failed for
lacking the suffix and never reached the fallback; it is a table of three
shapes now.
**The test-server guide overstated its own coverage,** claiming four of the
five conformance scenarios map onto fixtures. Three do: there is no
size-mismatch fixture, because the fixture can override an advertised digest
and nothing else, so that path is covered by unit tests. Corrected, with what
adding one would take.
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>
**The depth guard covered only the YAML side.** The listing arrives over
JSON-RPC so it cannot be cyclic, but it is just as unbounded in depth — and
both `jsonLikeEqual` and its message formatter walk it, so a server
advertising a deeply nested value crashed the tool exactly as a cyclic served
one did. Reproduced first: 60,000 levels against a shallow served value gives
`RangeError: Maximum call stack size exceeded`. `jsonGraphError` is shared
now and runs over `entry.frontmatter` before the file is parsed — there is no
point reading a SKILL.md if the thing to compare it against is unusable.
**A frontmatter mismatch stayed behind a collapsed section.** Round 7 made
the badge count these findings; the alerts explaining a mandatory
verification failure were still one click away on a section that opens
collapsed for a structurally clean entry. Conformance now reveals itself when
they appear, keyed on the entry so it fires once rather than fighting a user
who collapses it again.
That reveal exposed three fixtures serving one skill's SKILL.md for every URI
— a genuine frontmatter mismatch the check was right to report. The stale-
batch invariant test and the Storybook fixtures now derive each file from the
frontmatter its own entry advertises, with digests computed from those bytes
rather than hard-coded, so the class of drift is closed rather than the
instances patched. `SAMPLE_FM` is the single source for both halves.
One story subtlety worth recording: only a skill's OWN SKILL.md is derived.
Serving it for `notes.md` too made the tampered fixture fail its SIZE check
first, which is a different finding from the digest mismatch that story
exists to demonstrate.
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>
Four findings.
**The CLI dropped what it promised to print.** `getSkill` transforms the
`skills/get` envelope to a `SkillEntry`, so every other member the loose
schema accepted was gone before the CLI saw it — including the `ttlMs` /
`cacheScope` SEP-2640 explicitly leaves open. The code contradicted a comment
two lines above it saying a CLI whose contract is "print the result" must not
reshape one. `getSkillResult` returns the envelope whole; `getSkill` is a
one-line unwrap over it, which matches the callers: the UIs want the entry,
the CLI wants the result.
**The frontmatter verdict and the digest came from different reads.** The
preview fetch and the Verify fetch are separate `resources/read` calls, so a
resource changing between them could pair a verified digest with a
frontmatter verdict for other bytes. `VerificationState.entryText` now holds
the SKILL.md as the verification read it, written in the SAME state update as
the verdict — a separate write was clobbered, because `write` returns a fresh
object and drops anything not named in it. The preview stays the fallback so
a reader who has not clicked Verify still gets the check.
**The Conformance badge excluded the frontmatter findings** it renders, so it
said `0 error(s)` above a red mismatch. Digest and size mismatches stay in
their own badge: "the listing is wrong" and "the bytes are wrong" are
different answers.
**A repeated skill URI collided two React keys** in the TUI list, in a pane
whose job is to show both entries. Index-keyed like the manifest rows.
The web fixture also gains its collision pair in the served-file lookup —
without it they were handed another skill's SKILL.md, which the badge fix
correctly began reporting as a frontmatter error.
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>
`&a [1, *a]` puts the self-reference after a plain value, so a guard that
only inspected the head of a sequence would miss it. Already caught; this
records that it is.
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>
**A cyclic YAML alias crashed the tool.** `meta: &m [*m]` parses cleanly into
a self-referential array, and the frontmatter comparison is a recursive walk,
so a hostile server could take down `--verify` or the TUI with a stack
overflow instead of receiving a finding. Verified before fixing: it really
did raise `RangeError: Maximum call stack size exceeded`.
Rejected at the parse, so no consumer has to be cycle-safe on its own, and it
becomes an ordinary `frontmatter-unparsable` finding. Detection is per-PATH
rather than per-graph — `seen` unwinds on the way back up — so an alias reused
across siblings, which is ordinary YAML and represents fine in JSON, is not
mistaken for a cycle. A depth bound closes the same hole by its other door: a
legal, acyclic but absurdly nested document exhausts the stack just as well,
and a cycle check alone passes it.
**The web frontmatter check was gated on the presentation MIME.** It ran off
`previewParts`, which exists only when the displayed type is recognized as
markdown — so a `SKILL.md` a server typed `text/plain`, or served as a base64
blob, skipped a mandatory comparison while the pane still read as clean. It
now decodes the same fetched bytes the digest is taken over, which also stops
the two answers describing different derivations of the payload.
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>
Eight findings. Two were visible in a terminal capture I had already taken
and read past.
**A digest mismatch rendered with no digests.** `verifySkillResource` sets
`reason` for a SIZE mismatch — which short-circuits before hashing — while a
digest mismatch carries `expectedDigest` / `actualDigest` and no `reason`, so
the TUI printed a bare `✗ notes.md` with nothing to act on. `failureDetail`
now falls back to the expected/actual pair, truncated to fit the pane.
**A dynamic skill's read failure rendered nowhere.** Its synthetic
`read-error` row lives in the report, not the manifest, and the manifest is
empty for such a skill by definition — so the pane said only "Verification
FAILED". There is a Read failures block for any report file the manifest does
not cover, matched on normalized identity.
**The non-finite sentinel could be aliased.** A listing whose value genuinely
was `{"#non-finite":"NaN"}` canonicalized identically to a served `.nan`, so
the round-3 fix moved the bug rather than closing it. Any encoding into the
value space can be aliased by a document containing the encoding, so the
comparison is structural now (`jsonLikeEqual`) and there is no sentinel at
all; `Object.is` gives NaN === NaN while keeping ±Infinity distinct.
Serialization is used only for the message.
**`parseSkillFrontmatter("null")` returned `{ fields: {} }`,** contradicting
its own non-mapping contract: an explicit null scalar parses to exactly what
an empty block does. They are told apart by the source, since the parsed
value cannot.
Also: the `files` doc comment still claimed it is empty for a dynamic skill,
which the round-2 fix made untrue; and three inline Mantine elements with two
static styling props each are extracted to `.withProps()` constants, per the
convention the rest of the file follows.
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>
Two live findings. The review ran against 4fa00d1c — the commit before the
round-2 fixes — so its other three were re-reports of already-fixed issues,
verified present at HEAD rather than re-fixed.
**The TUI printed "Conformance: conforms" directly above "Verification
FAILED".** Self-contradictory in the one place a reader looks for a verdict:
the static checks pass on a skill whose advertised digest is well-formed,
while its bytes do not hash to it. The heading is now "Listing checks", which
names what the section covers — the checks against the entry the listing
returned, which say nothing about the bytes served. Same problem, and the
same resolution, as the web screen's "No structural issues".
**The test-server guide claimed every result carries the full modern
envelope**, which the new directory handler made untrue: `readDirectoryPage`
deliberately emits `resultType` alone, because SEP-2640 states the caching
attributes for a modern `skills/list` and says nothing of the kind here, and
its one worked example carries `resultType` only. The claim is narrowed to
the two `skills/*` results and the exception is documented as the deliberate
choice it is.
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>
Four findings, two of them silent holes in the verification itself.
**A blob-served SKILL.md skipped the frontmatter check entirely.** The entry's
own file was captured as `contents.text`, so a server returning the markdown
as a base64 `blob` — a legal `resources/read` shape this module already
decodes for the digest — never reached the comparison, and the report still
said `ok`. A MUST that quietly did not run. The file is now held as bytes and
the text derived from them, which also guarantees the digest and the
frontmatter describe one snapshot.
**A dynamic skill whose SKILL.md could not be read reported `ok: true`.** It
has no manifest rows, so `files` stayed empty, and its only static finding is
a warning — a verification that could not be performed was reported as one
that passed. The fallback read now records a `read-error` for all three ways
it can fail. It is gated on `manifestListsSelf` (by normalized identity) so a
self-entry the manifest listed and failed to read is not read, or reported,
twice.
**A served `.nan` compared equal to a listed `null`.** YAML expresses
non-finite numbers, JSON does not, and `JSON.stringify` turns all of them
into `null` — so the canonical comparison reported a real mismatch as
agreement. Verified against the parser before fixing. They now canonicalize
to a form no JSON scalar can equal, and the value is named in the finding.
Also: the self-entry match is by normalized identity rather than raw string,
which stops a second read and stops this function disagreeing with
`checkSkillConformance`; and `SkillsTab.tsx` gains the file header AGENTS.md
requires.
The four `err instanceof Error` ternaries are one `reasonOf` helper now —
extracted because the coverage gate found every one of their non-Error arms
uncovered, and one honestly-tested branch beats four ignores.
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>
SEP-2640: hosts MUST NOT assume name uniqueness, and when two entries in one
listing collide on `name` a host MUST disambiguate them rather than silently
discarding or preferring one.
`checkSkillConformance` structurally cannot see this — it takes one entry and
a collision is a property of the pair — so `checkSkillNameCollisions` walks
the listing and returns a finding per colliding entry, each naming the
others. All three clients merge it into the entry's own findings, so it
carries through the header badge, the CLI report and the TUI row marks with
no new surface.
**It is a `warning`, not an `error`,** and that is the severity split doing
its job. The obligation is on the *consumer*: a server may legitimately
publish two skills with the same name under different paths, and the SEP's
own `acme/billing/refunds` example is exactly that shape. Calling it an error
would tell a conforming author their catalog is invalid. `--verify` therefore
still exits 0 for a collision.
In the web screen it renders as a banner directly under the Conformance
header and is filtered out of the findings list, the same treatment
`dynamic-resources` gets: it changes how everything below it should be read,
and stating one fact twice reads as two findings.
Fixed while adding it: an entry whose only finding was a banner one rendered
an EMPTY findings container instead of "no structural issues", because the
list's presence was decided on the unfiltered set while its contents were
filtered. `listedIssues` is now derived once and used for both.
The fixture gains `acme/reports` + `globex/reports`, both fully conforming
and sharing the name `reports` — also the only fixture with a multi-segment
skill path, which nothing else exercised. Eight skills over four pages now,
and the integration test walks the cursor to exhaustion rather than asserting
a fixed page count, so a future fixture does not require editing it.
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>
