Round 2 found text the PR's own changes made stale: the fixture's
option doc, handler summary and repro step still promised -32601 on
modern; the core skills.ts header still said Skills must never be in
ADVERTISABLE_EXTENSIONS; the listSkills and getSkillResult comments
still said the codec validates none of a skills result, or that the
result arrives as the server sent it; and two tests and a docs step
still described Tasks as the only registry extension. The backfill
test also now asserts the Skills declaration is on by default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi
Signed-off-by: cliffhall <cliff@futurescale.com>
SEP-2575 gives a modern request that needs an undeclared capability its
own error, -32021 MissingRequiredClientCapability with
data.requiredCapabilities; -32601 there claims the method does not
exist. The strict fixture now throws MissingRequiredClientCapabilityError
on the modern leg and keeps -32601 on legacy, which has no such code.
The negative-control test now asserts the code and data per era, and a
new integration test resolves both checked-in strict configs through
loadConfig/resolveConfig, so a misspelled key or a dropped mapping
fails instead of leaving the manual fixtures permissive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi
Signed-off-by: cliffhall <cliff@futurescale.com>
SEP-2133 negotiates an extension from both sides, and a strict server
refuses skills/* to a client that did not declare
io.modelcontextprotocol/skills. The Inspector called those methods
without ever declaring it. Add it to ADVERTISABLE_EXTENSIONS, on by
default, which also gives it a Server Settings toggle.
Give the composable test server skillsRequireClientExtension and two
per-era strict showcase configs, so a strict server is reproducible here.
Writing the modern-era test exposed two defects the existing suite hid:
its "modern" cases set protocolEra on the transport config, which
InspectorClient does not read, so they connected on legacy; and once
they really ran modern, every skills result failed, because the modern
schemas required resultType, which the SDK codec checks and then lifts
off before a caller schema runs. Connect with versionNegotiation and
assert the era, and drop the resultType requirement (and the two modern
schema variants it was the only difference in).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot review round 2. A reverse proxy routes a public address to
one stable port, so neither MCP_SANDBOX_FULL_ADDRESS nor
MCP_APP_ORIGIN_FULL_ADDRESS is used when the listener's port is OS-assigned:
an explicit 0, a config-time collision resolved to 0, or the EADDRINUSE
fallback. Validation warnings also stop echoing the URL path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg
Signed-off-by: cliffhall <cliff@futurescale.com>
Document MCP_SANDBOX_FULL_ADDRESS and MCP_APP_ORIGIN_FULL_ADDRESS in
docs/environment-variables.md, note that ALLOWED_ORIGINS is now also the
sandbox proxy's embedder allow-list, and list public-address.ts among the
web server files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg
Signed-off-by: cliffhall <cliff@futurescale.com>
- A "dynamic" skill's only read is the fallback SKILL.md fetch, which was not
charged, so a catalog of dynamic skills never advanced catalogBytes and a
configured byte budget bounded nothing. Charge it like a manifest read.
- Add verifySkills cases for a configured byte budget and for dynamic skills.
- Wire onSkillCatalogLimitChange into the ServerSettingsForm story.
- Reword the new field descriptions, JSDoc and config table rows; rename the
route test to cover every invalid value it sends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6i1HBSdSS9kL4npZ2dDSe
Signed-off-by: cliffhall <cliff@futurescale.com>
- Scope the CLI pass-through warning to the three port variables; the
CLI normalizes HOST (wildcard -> localhost, others canonicalized).
- ALLOWED_ORIGINS default is derived from HOST: loopback origins, plus
the two wildcard origins for an all-interfaces bind, or only the
configured host's origin for a specific non-loopback host.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
An ad-hoc --server-url / command launch always negotiated the legacy era,
because protocolEra was only settable per server in mcp.json. A wrapper
that mints a short-lived bearer token therefore had to write that token to
disk to exercise a server under auto or modern.
--protocol-era <legacy|auto|modern> now feeds settings.protocolEra in all
three clients through a shared parseProtocolEra coercer:
- CLI/TUI overlay it in loadServerEntries alongside --header, onto an
ad-hoc target and onto a --catalog/--config entry for that run.
- Web stamps it on the in-memory ad-hoc entry, which the backend lifts
into settings like headers; as with --header it is rejected alongside
--catalog/--config, but unlike --header it applies to stdio too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SZ4rrvH6xaCfrst75gJh7x
Signed-off-by: cliffhall <cliff@futurescale.com>
- The CLI passes CLIENT_PORT / MCP_SANDBOX_PORT / MCP_APP_ORIGIN_PORT
into its hand-off as-is: no validation, no warning, no fallback.
- MCP_AUTO_OPEN_ENABLED=true also lets CLI interactive OAuth start with
no TTY on stdin or stderr.
- An empty MCP_INSPECTOR_SECRET_STORE counts as unset, silently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
Add MCP_SANDBOX_FULL_ADDRESS and MCP_APP_ORIGIN_FULL_ADDRESS so a reverse
proxy or ingress deployment can advertise the sandbox proxy URL and the
_meta.ui.domain app origin the browser actually reaches, instead of the
bind-derived http://localhost:<port> ones. The process still listens on
MCP_SANDBOX_PORT / MCP_APP_ORIGIN_PORT.
Both values are validated in public-address.ts and refused (warned, bind-
derived address kept) when malformed or when their origin collapses onto
the Inspector UI's (or, for the app origin, the sandbox's) — the MCP Apps
spec requires host != sandbox origin, and the single-hostname proxy layout
is exactly the one that breaks it. Plain http under an https UI is warned.
The sandbox proxy page's referrer check was a hardcoded
http://localhost|127.0.0.1 pattern no https embedder could match. It now
compares the referrer's parsed origin against an exact list injected by the
controller, derived from the same filtered allow-list as its frame-ancestors
header, with the old loopback check kept only as the no-list fallback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg
Signed-off-by: cliffhall <cliff@futurescale.com>
- Launcher README: narrow "does not interpret env vars" to server and
client configuration, so it no longer contradicts MCP_DEBUG.
- Document HOME / USERPROFILE as the base of every ~/.mcp-inspector
default, including the cwd fallback when neither is set.
- SERVER_PORT is the sandbox fallback whenever MCP_SANDBOX_PORT is not
a valid port (unset, empty or invalid), not only when unset.
- MCP_INSPECTOR_OAUTH_STATE_PATH is read by the CLI and TUI only; the
web backend always uses <MCP_STORAGE_DIR>/oauth.json.
- MCP_DEBUG / DEBUG are also read by the standalone TUI entry point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds a Skills section to Server Settings with Maximum Number of Skills and
Maximum Catalog Size fields, defaulting to SKILL_MAX_CATALOG_SKILLS (256) and
SKILL_MAX_CATALOG_BYTES (64 MiB). The values persist as skillCatalogMaxSkills /
skillCatalogMaxBytes on the server's mcp.json entry (omitted at the default),
are validated as positive integers by the /api/servers routes, and are read by
verifySkills through getServerSettings(), so the CLI's --verify and the TUI
Skills pane both honor them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6i1HBSdSS9kL4npZ2dDSe
Signed-off-by: cliffhall <cliff@futurescale.com>
- MCP_AUTO_OPEN_ENABLED: the TUI never reads it; the CLI's unset
default opens only on a stderr TTY. Migration guide row fixed too.
- Drop the blanket "empty equals unset" rule, which HOST="" and the
path variables contradict; state empty handling per row instead.
- MCP_SANDBOX_PORT / MCP_APP_ORIGIN_PORT accept 0; only CLIENT_PORT
must be fixed.
- NO_COLOR: an empty value counts as unset.
- MCP_INSPECTOR_SECRET_FILE default is ~/.mcp-inspector/secrets.json,
beside the storage directory, not inside it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
Every variable that changes runtime behavior now lives on one page,
docs/environment-variables.md: which client reads it, its default, and
its effect. It covers three variables that were undocumented
(MCP_LOG_FILE, MCP_INSPECTOR_LOG_DIR, LOG_LEVEL), the secret-store
variables previously only in the Docker guide, and the Node TLS
variables that answer #1936 (a self-signed https://localhost server).
The README docs table, the migration guide, docker.md and the four
client READMEs link to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
connectionTimeout defaulted to 0, which InspectorClient.connect() read as
"no bound at all" — so out of the box nothing the Inspector owns bounded a
connect attempt, and the only thing ending a hung handshake was the SDK's
60 s per-request timeout on initialize, reporting "Request timed out".
- Add DEFAULT_CONNECTION_TIMEOUT_MS (30000) to core/mcp/types.ts. An absent
value resolves to it on the client, on disk read, in the web form draft,
and in the CLI's header-only settings shell. An explicit 0 keeps its
documented meaning (no timeout; --connect-timeout 0) and is now persisted
so the opt-out round-trips; the default is the omit-sentinel, as for
taskTtl.
- Key the post-timeout transport teardown to the timer actually firing, not
to connectionTimeout > 0 — otherwise a non-zero default would tear down
every failed connect, including a recoverable 401 whose transport the
auth recovery holds open.
- The timeout message names the bound and where to raise it: "Connection
timed out after N ms. To accommodate a slower server, you may increase
the timeout value in Server Settings."
- The CLI's UNREACHABLE classifier now matches that message.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4h8Xa6Ed9zwtv5ojX35MT
Signed-off-by: cliffhall <cliff@futurescale.com>
- requestAppElicitation.ts, appElicitation.ts, docs/test-servers.md:
ext-apps 2.0.0 does not "predate" #733 (the PR is from July; the
release from September) — it simply does not include it. Reworded as
omission, and the version the appElicitation.ts JSDoc names is now
2.0.0 rather than 1.7.5.
- core/mcp/types.ts: the public `AppRendererClient` JSDoc described the
app-renderer proxy; it is the real SDK `Client`, handed out only while
connected, since #1745 removed the proxy.
- appCapabilities.test.ts: the two comments verified against 1.7.5 now
cite 2.0.0, which the test runs against.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Every site tagged `ext-apps#702` carried a workaround for ext-apps
peering on the v1 SDK while the Inspector runs v2. With 2.0.0 the
package's `AppBridge` extends the v2 `Protocol`, takes the v2 `Client`
and `Implementation` directly, and registers its list-changed forwarding
on that client with `setNotificationHandler("notifications/…", handler)`,
so:
- core/mcp/apps.ts: `getToolUiResourceUri(tool)` takes the v2 `Tool`
without a cast; `UiMetaShape` is replaced by ext-apps' own
`McpUiToolMeta` / `McpUiResourceMeta`, which resolve under NodeNext
since ext-apps#705. `AppInfo.csp` / `.permissions` / `.visibility`
take the named types too.
- core/mcp/inspectorClient.ts: `getAppRendererClient()` returns the SDK
client itself; the Proxy that translated the 1.x schema-first
`setNotificationHandler(Schema, handler)` into the method-string form,
its `notificationMethodFromSchema` helper and its cached field are
gone. `AppRendererClient` is now `Client` rather than `unknown`.
- createAppBridgeFactory.ts: `new AppBridge(client, HOST_INFO, …)` with
no `as unknown as`; `resolveSourceUri` passes the tool straight through.
- AppRenderer.tsx: `sendToolResult(result)` — its params ARE the v2
`CallToolResult` now.
- requestAppElicitation.ts: the double cast on the `elicitation/create`
send goes too — v2 `Protocol.request` takes the standard `Request`
shape, which `ElicitRequest` is. The helper itself stays: ext-apps#733
is not in 2.0.0.
- core/mcp/extensions.ts: `MCP_APP_MIME_TYPE` re-exports ext-apps'
`RESOURCE_MIME_TYPE` from `/app-bridge`; `UI_EXTENSION_KEY` stays
hardcoded because `EXTENSION_ID` is still `/server`-only in 2.0.0.
Tests: the Proxy-translation test is removed with the Proxy; the
`getAppRendererClient` tests assert the real `Client` instance and that
disconnect withdraws it; the `useInspectorClient` sentinel is a real
(unconnected) `Client`. Comments that cited ext-apps 1.7.5 now cite 2.0.0
where the fact still holds (#733 still unreleased).
Closes#1745
Refs #2309
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Narrow the lease's guarantees to what acquireLease and finishLease do; say
where no release ever comes; and record the concurrent-sibling arm of the
maxWorkers measurement beside the solo one.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The 45-minute wait is a total queueing budget in the skill too; the lease
promises the holder's release, not every waiter's acquisition; and what it
removes is gate-on-gate contention, for as many starts as fit the budget.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The process-group signal guarantee is POSIX-only; the 45-minute wait is a
total queueing budget, not a hung-holder detector; the fixed-port range is
6295–6299 including smoke:web:tabs.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Every stage description now matches the scripts on v2/main: the quiet-gate
baseline figures, the CI job budgets (#2333), the smoke-helper budgets that
are named constants or stated defaults, the maxWorkers decision (#2336), the
Storybook forced pre-bundle (#2340), the gate-lease test suite (#2339/#2346),
and a local:storybook row. Adds the multi-agent section at the end: what the
lease does, why a queue rather than a load wait or a worker cap, what it means
for capacity, and what it does not do.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- The holder-record removal is best-effort and can no longer throw past
release(); ERELEASED on release is the library having already dropped a
taken-over lock, so it is ignored rather than described as a lock that
"goes stale on its own".
- The signal test's stand-in now spawns a descendant of its own, so the
whole-process-group guarantee is what the test asserts: a regression to
child.kill() or a non-detached spawn leaves the descendant alive and fails.
- The three docs no longer claim a dead holder is always recovered in 30s:
a stale lock directory that cannot be removed runs the wait to its
45-minute cap and names the path.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
`npm run local:gate` now runs its stages under `scripts/gate-lease.mjs`, a
`proper-lockfile` lease under `os.tmpdir()` (or `$XDG_RUNTIME_DIR`), so a gate
started in a second worktree waits for the first instead of overlapping it.
Measured on v2/main @ aa56551b: a quiet gate is 257s green; two started
together had one fail at 279s (`smoke:web:chromium` found port 6298 held by
the other gate's smoke) and the survivor take 338s. Under the lease, three
gates started within seconds ran back to back (259s, then 262s, then the
third), each green with zero timeouts.
The lease grants rather than polls a threshold, so two waiters cannot
deadlock on each other; a holder that dies stops refreshing and is taken over
after 30s; a signal to the wrapper stops the whole process group, releases,
and exits 128+signal; the wait names the holder's pid and worktree and gives
up after 45 minutes. `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. A lock that
cannot be created runs the gate unleased with a warning.
`scriptChainRuns` looks through the wrapper (and only it) so the existing
vouches about what `local:gate` runs keep reading the real chain.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 7's one suppressed finding: "that second pass" read as the
instrumented coverage pass; the ~80s is the bare pass that ran first.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- `reachableScripts` now follows every npm spelling of `run` — the
canonical `run-script` and its `run`/`rum`/`urn` aliases — with a unit
case, so `npm run-script validate:web` appended to `local:gate` fails
the whole-gate check instead of slipping past it.
- docs/quality-gate.md `verify:skills` row, AGENTS.md skills rule 1 and the
#2085 narration now describe the actual chain (`validate:guards`, reached
by both `validate` and `local:validate`) and the qualified promise.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- Reword the CI guarantee in AGENTS.md, docs/quality-gate.md and the
pre-push-gate skill: a green gate means every check CI applies has
passed here — the strongest predictor of green CI, not a proof — and
name the residual (a test that passes only when slowed) as #1596's
class to fix rather than headroom to keep.
- docs/quality-gate.md: cli's validate DID build, through `test`'s
`pretest` hook; say that the build goes with the dropped `test` leg and
`coverage:cli` builds once, later — in both places.
- Update the two source comments that still described `local:gate` as
running `validate` (verify-skills.mjs, verify-build-gate.mjs).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
- Match each client's `cd` together with its adjacent `npm run check`, so
dropping one client's check from `local:validate` fails the test.
- Reject `npm test` / `npm t` / `npm tst` in a client's `check` commands as
well as `vitest`, since `reachableScripts` follows `npm run` only.
- docs/quality-gate.md: cli's `check` does not build — its binary is built
by `coverage:cli`, as before; say so in both places.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
`local:gate` ran every client's unit suite bare inside `validate` and
again instrumented inside `coverage` — ~80s of a ~370s run re-executing
exactly the files the coverage pass runs a few minutes later. CI pays
nothing for that duplication because its two jobs run in parallel; the
gate runs serially, so it paid all of it.
The gate's first stage is now `local:validate`: the same `validate:guards`
and `validate:core`, then each client's new `check` script (`format:check`
+ `lint` + `typecheck` + `build`) instead of its `validate`, which is now
literally `check && test`. `npm run validate` and both CI jobs are
unchanged; `local:validate` sits in the `local:` namespace so the
workflow guard keeps it out of CI by construction.
The "strict superset" claim in AGENTS.md is reworded to what is now
true — every CI *check*, not every CI *invocation* — with the reasoning:
`@vitest/coverage-v8` reads V8's precise-coverage profiler and rewrites
no source, so the instrumented pass runs byte-identical code and is only
slower, the stricter direction for the failure class this repo sees.
Measured on the same worktree: 370s -> 262s wall (EXIT=0 both), the
four removed legs summing to 78.5s in the baseline run's own profile.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 8 of the review asked for an AST. That was the right diagnosis of the
wrong problem: the question was never which parser to use, it was why a
test-timeout guard was parsing JavaScript at all.
It got there one reasonable step at a time. The guard enforces two rules no
Vitest config can report — a per-test `retry`, and Testing Library's
`asyncUtilTimeout` — so it read them out of source text, and the review found a
new valid spelling it missed in five consecutive rounds: a trailing comment,
nested parens in `it.each([makeCase()])`, a member-access prefix, the shorthand
`{ retry }`, an aliased import, a non-literal title. Each fix was correct and
each made it more parser-shaped, until it carried eight helpers doing quote
tracking and bracket balancing.
Both rules are observable at runtime, exactly:
- `vitest.setup.shared.mts` is loaded by all six projects and reads
`ctx.task.retry` — the value Vitest resolved for the test. That covers
strictly more than the scan did: a per-test option, a `describe` option
inherited by children, a project-level setting, and a `--retry` flag typed at
a command line, which the scan could never see since it only read npm scripts.
- `clients/web/src/test/asyncUtilTimeout.test.ts` asserts
`getConfig().asyncUtilTimeout`, the value this project's own waitFors read.
Neither has a spelling to anticipate, so neither can be outrun by a new one.
Both were observed firing before being trusted.
The guard keeps only what it can answer by asking Vitest: resolved budgets,
project-level retry, config discovery, and that each project still loads the
setup file — the one thing about the runtime assertion a config CAN report.
868 lines to 361, every scanner helper gone, and 33 of 48 unit tests deleted
because they pinned edge cases of a thing that no longer exists.
storybookSetup.ts goes with it. At the library's own value its configure() call
was a no-op, and the assertion is what pins the number.
AGENTS.md gains the general rule: a rule about what the code DOES is asserted at
runtime, not read out of the source — reach for a verify:* guard when the
question is about configuration, which a tool can be asked to resolve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings are the same shape as round 2's: a guard that documents an
invariant while checking one narrow expression of it.
`retry` was only rejected at the project level, so `it("x", { retry: 2 })` and a
`--retry` flag in an npm script both walked through a guard whose own header
said "no retry anywhere". Enforcement is extended rather than the claim
narrowed, because the decision is about the behavior, not about a config key —
all three places Vitest accepts one are now scanned, and the header names the
one that remains out of reach (a human typing --retry into their own shell),
rather than overclaiming.
The test-definition scan is deliberately positional: the retry must sit in an
options object following the test name, which is the only place Vitest reads it
from. A bare word match would fire on fixture data, on `client.retry`, on an
unrelated helper's options and on a mock — all of which exist here, and a guard
that cries wolf gets disabled. Both directions are tested. The first pattern
also missed `it.each([...])("name", { retry })` entirely, so every parameterized
suite was a blind spot; the table argument is now part of the pattern.
Config discovery named only the two `.ts` spellings this repo happens to use,
while Vitest loads twelve. A `clients/foo/vitest.config.mts` was therefore
invisible, and an invisible config yields no project to reject — the same hole
round 1 found in the hand-written CONFIG_ROOTS, one level down. Both discovery
and resolution now use the full set, ordered as Vitest orders it.
Both checks were observed firing before being trusted: a planted retry in a
launcher test and a planted vitest.config.mts each fail the guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The comment stripper had a real hole: the line-comment regex was anchored, so it
only removed `//` at the start of a line and a call disabled after code —
`const disabled = true; // configure({ asyncUtilTimeout: 1000 });` — survived it
and the guard passed with no effective configuration. Replaced with a scanner,
because a regex gets this wrong in both directions: it has to strip a comment
that follows code, and it must NOT treat the `//` inside a string literal as one,
which would silently truncate any line holding a URL. Quote tracking (with
escapes) is the only way to have both. Three regression tests.
Two documentation claims were also wrong. The guard header and quality-gate.md
cited #2278 and #2250 as instances of a budget nobody chose; they were not —
#2278 was a missing condition wait around a geometry read and #2250 a real race
in a test's own timing, both fixed by making the test wait for the right thing.
Presenting a race fix as evidence for a larger ceiling argues against #1596,
which this guard exists to uphold. Only the budget and poll-ceiling cases are
cited now, and both files say explicitly why those two are excluded.
And browser-timeouts.mjs overstated its own behavior twice: `bestEffort` can
spend its full budget on an otherwise passing smoke precisely because its expiry
is caught, and raising any budget does delay the report when the thing waited on
never arrives. The diagnostics improve the eventual message; they do not make
the wait shorter. Both now stated plainly, so the header is something to weigh a
change against rather than a reassurance.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
The inline finding was a real bug: main() never called the discovery check, so
the deny-by-default claim rested on test:scripts happening to run the same
comparison — a different command, which `npm run verify:test-timeouts` does not
invoke. `failures` is now seeded from it, and a probe config under clients/ was
observed to fail the guard.
Three suppressed findings, all correct. The afterEach in oauth-interactive
restated the shared hookTimeout and labelled it as a round trip, when it only
stops a server and unlinks a file; it now carries no budget. The file-lock
constants were named for the opposite of what they bound — 90s covers retries
running out against a holder that never yields, 60s covers the stale-lock
takeover that succeeds — so they are LOCK_RETRIES_EXHAUSTED_MS and
STALE_LOCK_TAKEOVER_MS. The browser-timeouts header said 15 budgets; it is 19.
Separately, and more importantly: the asyncUtilTimeout raise is reverted, on
measurement. Verifying round 2 turned up intermittent 15s timeouts across
unrelated unit files, so three arms of three full unit runs were taken in this
worktree at comparable load — unmodified v2/main 3/3 green, this change with
5000 producing 1, 4 and 0 timing-out files, and the same change with that one
line reverted 3/3 green. A wait that is MEANT to expire spends its whole budget
on the happy path, so a 5x raise is a 5x cost on exactly those tests and
saturates the worker pool, starving tests that were never slow.
Both web setup files therefore state asyncUtilTimeout at the library's own
value rather than raising it, which is what #2323's rule actually asks for: a
budget nobody chose is the defect, not a budget that is small. The tests that
assert absence by letting a wait expire are the real problem and are #2335.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Three of the six Vitest projects ran on Vitest's own testTimeout: 5000 and
five on its hookTimeout/teardownTimeout: 10000, and all 788 Testing Library
waits in the web unit project ran on asyncUtilTimeout: 1000. None of those
numbers was chosen here, and all of them were sized for an idle machine rather
than for three or four concurrent worktree sessions each running a full
local:gate on eight cores. #2292, #2278, #2250, #1942 and #1742 were each that
class, diagnosed one site at a time.
This is the complement of #1596, not a retreat from it: every fix that issue
produced stands, and no race is hidden here. What changes is that a correct,
deterministic test is no longer cut off by a budget nobody picked.
- vitest.shared.mts exports frozen TIMEOUTS / INTEGRATION_TIMEOUTS; all six
projects spread one of them, clients/launcher importing the shared module for
the first time to do so.
- Both web projects configure Testing Library's asyncUtilTimeout. The storybook
one needs its own setup file, deliberately outside .storybook/ and free of
setProjectAnnotations so the automatic preview-annotation provisioning
(#1898) is untouched.
- 28 per-suite restatements of the integration project's own budget are gone,
so raising the shared value now moves every site under it. The handful that
genuinely differ — cross-process lock contention, a full OAuth round trip —
keep their budget as a named constant that says why.
- AppRenderer's waitFor no longer asks for exactly its enclosing budget, which
it could never win (#2292's shape); cli's e2e child timer drops below the
project budget so the bound carrying the diagnostic is the one that fires.
- The transition auto-settle's slack term is named RAF_SLACK_MS and raised;
the term derived from HEADER_ANIM_MS is untouched. The two answer to
different things and must not be scaled as one number.
- The 19 Playwright budgets scattered across three web smokes become four
named constants in scripts/lib/browser-timeouts.mjs.
scripts/verify-test-timeouts.mjs resolves each project through Vitest itself
and asserts the budgets, the asyncUtilTimeout sites, and that retry stays
unset. It was observed red against the pre-change config: unit/tui/launcher at
testTimeout 5000, five projects at hookTimeout 10000, both asyncUtilTimeout
sites unmet.
Closes#2323
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
* 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>
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>
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>
`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>
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>
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>
**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>
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>
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>
Five findings, all real.
**Critical — `verifySkills` selected `contents[0]`.** These bytes are hashed
against the file's advertised digest, so accepting a block the server
labelled something else verifies one file's content against another file's
digest, and can report that as `verified`. A false pass is worse than a
missing check. `contentsFor(result, uri)` now selects by NORMALIZED identity,
which keeps the canonicalized-echo case that motivated the original code
while rejecting an unrelated block, and reports a read error when nothing
answers for the URI. This is what `onReadSkillFile` already did; the two
paths hash bytes against digests and must not disagree about which bytes.
**`skills/get` was dispatched without the extension gate** that `skills/list`
had. Hoisted into `assertSkillsSupported` so the two cannot drift — declaring
the extension commits a server to both. Without it, an undeclared server's
-32601 is indistinguishable to a script from the -32602 a declared server
returns for a URI it does not serve.
**The TUI could strand a user on a hidden Skills tab.** The tab left the bar
when the gate went false but `activeTab` did not, so the pane kept rendering
for a server that never declared the extension. Reset to `info`, following
the Auth precedent — additionally gated on `connected`, because this gate
reads a server declaration and would otherwise fire during a reconnect.
**The TUI keyed a verdict by URI**, so a refresh replacing the manifest under
the same URI left hashes computed for the previous snapshot describing the
new one. Keyed on the entry now, as the web screen already was.
**A failed "Load more" discarded the pages on screen and the retry cursor.**
Both are preserved now, and the success and failure paths share one
staleness-guarded `commit` helper so they cannot drift on which results they
may write — which is how they came to disagree.
Two nits: the six-skill counts. The docs sentence was more wrong than
reported (five awkward skills of six, three outright violations) and is
rewritten rather than renumbered.
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>
Closes#2248.
Completes SEP-2640 support across all three clients, and closes the one
obligation #2234 deliberately left open.
- `checkSkillFrontmatterMatch` compares a served SKILL.md's own YAML
frontmatter against the entry the listing advertised, field by field. No
digest can cover this: a digest is taken over the bytes the server served,
so it proves the file was not altered in transit and says nothing about
whether the listing described it honestly. Needs a real YAML parser; `yaml`
was already a root dependency, so this adds no package — but it does newly
put it on core/'s import graph, hence the three bundler `external` lists.
- `resources/directory/read`: result schemas defined against the normative
text, `InspectorClient.readResourceDirectory`, a Directory section on the
Skills screen, and `--method resources/directory/read` in the CLI. The
client refuses the call locally when the server did not declare
`directoryRead`, which is the SEP's MUST NOT. A child the directory lists
but the entry does not is marked `not listed` rather than merged into the
manifest — the SEP calls a directory result a live observation and forbids
treating it as extending the manifest.
- CLI: `skills/list`, `skills/get` and `resources/directory/read`, plus
`--verify` — one NDJSON report per skill, a summary on stderr, exit 7 on a
violation. Reads are sequential; a conforming manifest may declare 512
entries.
- TUI: a Skills pane, shown only when the server declares the extension. Each
row carries its conformance verdict as a glyph as well as a colour, since
the pane is read over ssh and through script(1). Enter verifies.
- `verifySkills` re-throws `AuthRecoveryRequiredError` rather than recording
it per file: it says the session's authorization expired, so absorbing it
would report N identical read failures and swallow the error the TUI and
the web commands key off to reauthorize.
Two open questions settled, both in code comments where they will be found:
`skills/get` carries no caching attributes because SEP-2640 leaves the
question open in as many words, so requiring them would fail a conforming
server; and there is no `PagedSkillsState` because every consumer of this
list is a whole-catalog verdict — computed over page one of three, "this
server's skills conform" is not merely partial but wrong.
The fixture grows two skills, both for checks that were otherwise
undemonstrable: `lying-listing` (listing and file disagree, digest still
verifies) and `stale-manifest` (serves a file its manifest does not declare).
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>
