Copilot, all three accepted.
1. Connecting still is not the condition. A few integration tests
deliberately hand-roll a JSON-RPC server because the composable
fixture *cannot* produce what they assert on —
`inspectorClient-malformed-list.test.ts` says so in its own header:
"the SDK's own server cannot emit these shapes: they are exactly what
its types forbid". Same for `listSalvage-era.test.ts`. Real
transport, real client, zero `test-servers/` dependency.
Round 4 already stated the right condition at the top — "does this
test depend on a fixture from `test-servers/`" — and then three
downstream sites went on saying "connects". Those are now scoped to
fixture use, and the hand-rolled exception is stated where the
trigger is defined rather than left to be discovered, with both files
named. Connecting is kept as a hint, since it usually does imply a
fixture, with the instruction to check whether one can express the
case first.
2. "description-strength measurement" was the wrong surface, and this
PR is what made it wrong: a chained prompt is deliberately written so
the target's *description* cannot fire on it, so the number measures
the pointer in the first skill's body and nothing else. Left as-is it
would send the next person tuning a red hand-off at the description.
3. "an 0.8 bar" -> "a 0.8 bar".
Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, RUNS=5.
Staged by explicit path, not `git add -A` — see ec6895a5.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot caught this. `scripts/verify-build-gate.mjs` appends a
`node:fs` import to `clients/web/src/main.tsx`, asserts the
browser-externalized-builtin gate rejects it, then restores the file.
Round 6 was committed with `git add -A` while `local:gate` was mid-run,
so the staged snapshot caught the probe in place and shipped it.
The consequence is exactly what the gate exists to prevent: a Node
built-in in the normal browser graph, which makes the gate fail every
Vite build from here on. CI could not have passed.
`main.tsx` is restored byte-for-byte to its v2/main content; no other
file was affected.
The transferable lesson, and the reason this is its own commit rather
than a squash: **never stage with `git add -A` while the gate is
running.** `verify:build-gate` mutates a tracked source file for the
length of one build, and that window is wide enough to catch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, accepted — and the sharpest catch of the six, because it is
the rationale contradicting the operator two paragraphs below it.
Round 1 rewrote the CHAIN_THRESHOLD comment to say a well-shaped
pointer "does clear 0.8" and cited 100%/80% as the evidence. This
threshold is compared with `>`, so 80% would FAIL an 0.8 bar; the
example refuted the sentence it was offered in support of. It was also
stale: 100%/80% was an intermediate build, and the shipped state
measures 100%/100%.
Both copies now cite 100%/100% as the clearing evidence and keep
100%/80% only as the residual-noise figure, saying explicitly that it
is not an example of clearing the bar.
While checking the claim I found the docs copy needed the same care for
a different reason: `passesThreshold` is strict for the chain and
INCLUSIVE for the first move (`rate > t` vs `rate >= t`, skill-eval.mjs
:558), and 0.8 is the constant for the inclusive one. So "0.8 is
compared strictly" is false as literally read. The docs now say a
*chain* bar of 0.8 would be the strict one, name the operator for both,
and spell out the consequence at RUNS=5 — only 5/5 passes, 4/5 does not.
No functional change; CHAIN_THRESHOLD is still 0.5, still strict.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, accepted. Round 4 split the trigger into two branches and
fixed the closing section, but left the paragraph that ends the opening
section speaking for both — asserting a real transport and a protocol
era for the build-only case that lines 28-31 had just established has
neither.
Scoped explicitly: "In the connecting case …" keeps the transport and
protocol-era guidance, "In the build-only case …" says that guidance
does not apply and names what is actually wanted from `test-servers`
there — how to build the fixture, and why a stale build keeps serving
old code.
Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, full suite
at RUNS=5.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, both accepted.
1. "Anything that puts a built fixture on disk" matches every web test,
not the case it was written for: `clients/web`'s `pretest` runs
`test-servers:build` ahead of every unit run, so the fixture is on
disk for the pure parser and storage tests the very next paragraph
excludes. The trigger contradicted its own exclusion list.
Re-keyed to depending on the artefact — the test imports, spawns, or
points a config at it — with the pretest fact stated as its own ⚠️ so
the distinction is legible rather than implied.
2. The closing section still said the fixture-consuming case needs "a
real server over a real transport" and a protocol era, which is
exactly false for the build-only consumer round 3 added. `smoke:tui`
drives no transport, so config choice and protocol era do not apply
to it — only the build and staleness half does.
The condition is now stated once, with two named sub-cases that say
which part of `test-servers` each needs: connecting takes the whole
procedure, naming-the-artefact takes the build half. The closing
section mirrors that split instead of collapsing them.
Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, full suite
at RUNS=5. Unchanged across all four rounds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
Two factual corrections, both suppressed comments.
- `.claude/skills/local-dev/SKILL.md`: the react/react-dom exception cited
"19.2.8 today, against the root's 19.2.7" — a skew this PR removes. Dropped
the version pair; the point being made is about which install resolves the
pair, and it stands without a number that goes stale on the next bump.
- `verify-dep-lockstep.mjs`: the `devDependencies` bullet claimed all four
surfaced skews were a devDependency somewhere. `react` is a root runtime
dependency whose client copies are peer-installed, so it is three of four —
which is a better illustration of why the union spans fields as well as
installs, and the bullet now says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, both nits accepted — the first is a correctness hole, not a nit.
1. `smoke:tui` never asserts a round trip, so round 2 filed it under
"boot-only, needs nothing". It calls
`ensureTestServers({ requires: ["stdio"] })` and hands the built
fixture to the TUI as its catalog's stdio command
(scripts/smoke-tui.mjs:105-124) — which means the staleness hazard,
the single most expensive thing in `test-servers`, lands on it in
full. Connecting was the wrong sole condition.
The trigger is now "does this test **connect to, or build and
consume**, an MCP fixture?", with `smoke:tui` named at all three
decision points and the fixture-consumption case given its own
paragraph in the closing section.
2. The "29 of 74" figure was brittle and I could not reproduce it
against a different pattern: 29 by my grep, 32 by a direct
`@modelcontextprotocol/inspector-test-server` import, 39 by a wider
union. A number whose value is a function of the grep does not belong
in a skill body that nobody re-runs. Dropped; the concrete
counter-examples stay, which is what actually carries the point —
`storage/store-id.test.ts` validates a string and `mcp/import/*`
parses config files, both sitting beside live-connection tests.
Re-measured after broadening the condition: 100% / 100% hand-off,
63/63 first-move at 100%, full suite at RUNS=5. Unchanged across all
three rounds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, both findings accepted — and the first is the same defect one
level down from round 1.
1. Round 1 replaced "a smoke" with a transport condition but left "an
integration test" unqualified, and left the tier note treating web
integration as wholly fixture-dependent. It is not: **29 of the 74
files in `src/test/integration/` reference a server at all.**
`storage/store-id.test.ts` validates a string; `mcp/import/*` parses
config files. They sit there for the node env and the 30s timeout —
placement is the project manifest, which is exactly why it cannot
also be the fixture trigger.
All four sites now say "an integration test **that connects**" and
name the counter-examples, including the folder-is-not-the-trigger
point at the `integration` bullet itself, and the tier ⚠️ now states
that the boundary cuts *across* the tiers rather than along them.
2. Replacing the worked example orphaned the warning after it: "the two
runs above straddle a 67-point swing … with no change to the pointer"
had nothing left to refer to, since the 33%/33% and 100%/33% pair it
names was gone and the runs on show differed by 20 points across a
pointer rewrite. The weak-pointer pair is restored as its own
paragraph, the warning names both figures explicitly, and the 20-point
intermediate is now framed as RUNS=5 residual noise — contrasted with
the 67, rather than standing in for it.
Re-measured, since "that connects" narrows the phrasing the two chain
prompts have to match: 100% / 100% hand-off, 63/63 first-move at 100%,
full suite at RUNS=5. Unchanged from round 1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
All four suppressed comments, none with an inline thread to answer.
- `docs/quality-gate.md`: the second-tier explanation was a paragraph
*inside* the script table, which breaks the table — the `local:gate` and
`pack:verify` rows below it would have rendered as pipe-delimited text.
Folded back into the `verify:dep-lockstep` cell.
- `verify-dep-lockstep.mjs`: the module header and `declaredPackages`'
summary line both still named two fields after the implementation grew a
third. Both now name all three, and the header says which field is
excluded and points at the reason.
- `AGENTS.md`: "Nothing keeps the surviving copies aligned, and nothing
gates them" contradicted the sentence added later in the same paragraph.
Split the two claims — nothing aligns them *automatically*, and the guard
now rejects the drift; the bump is still by hand.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, both findings accepted.
1. The trigger was over-broad. "a smoke" and "everything from web
integration rightwards" routed work to `test-servers` that needs no
fixture at all: `smoke:launcher` only checks `--help`, `smoke:web`
asserts the SPA is served, `smoke:web:browser` stops at first paint,
and Storybook play functions render from fixture props. The condition
is now "does this exercise MCP behaviour over a transport?", stated
as such, with both sides named — the smokes that connect
(`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`)
and `pack:verify` on one side, the boot-only ones and Storybook on
the other. Depth in the tier list is explicitly not the boundary.
2. The threshold rationale contradicted the new result. It said an 0.8
bar would mark a hand-off red "regardless of how strongly the first
skill points at the second" — which the 100% measurement refutes.
Both copies (docs and `skill-eval.mjs`) now say what 0.5 actually
buys: a floor on useful reliability for a noisier second-hop load, so
the column still separates an adequate pointer from a broken one. The
value is unchanged.
Re-measured after the narrowing, since a tighter trigger could have
weakened the pointer: 100% / 100% on the hand-off and 63/63 first-move
at 100%, full suite at RUNS=5. Recorded figure updated from the
intermediate 100% / 80%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
- Include `optionalDependencies` in the declared tier's candidate union. A
peer range constrains the consumer's host; an optional range is a direct
declaration npm attempts to install, so omitting it left an optional-only
package free to skew unseen. `peerDependencies` stays excluded, with the
reason stated.
- Make the end-to-end fixture's client copy UNDECLARED — only the root
manifest names `solo`, while `clients/web` merely holds a top-level copy.
That is #2226's defining shape, and it is what makes the candidate set a
union across installs rather than a per-install intersection; the old
fixture would have passed a per-install implementation. Adds the mirror
case (declared only by a client, held by the root) so the union is pinned
in both directions.
- Update the two docs that still described the single-tier guard:
`.claude/skills/local-dev/SKILL.md` said "nothing gates either of those,
and `verify:dep-lockstep` is not it" and cited the live `@types/node`
drift in the present tense; `docs/quality-gate.md` described only the
program tier. Both now state what each tier covers and the two limits
that remain.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
- `newAttemptId` calls `globalThis.crypto.getRandomValues(...)` directly
rather than through a bound alias. CodeQL's `js/insecure-randomness`
browser model recognizes a secure RNG by that literal method call, so
the indirection would have left the `Math.random` last resort
classified as an unmitigated source.
- The no-crypto test arm now removes `globalThis.crypto` entirely
instead of hiding its two methods, matching the crypto-absence test in
`src/test/core/auth/utils.test.ts`. Hiding only the methods left the
global truthy, so the arm could not have caught a regression that read
`globalThis.crypto` without the optional guard. `withoutCryptoMember`
narrows to `randomUUID`, its only remaining caller.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>
The hand-off measured 33% / 33% at RUNS=3 (#2204's two committed chain
cases). `testing` opened with a one-sentence ⚠️ *classifying* which work
belongs to `test-servers` — a fact, which the model skims — and said it
nowhere else except a bare cross-reference in the last section.
Rewrite it as an action with a trigger ("load the `test-servers` skill
now — that is step one"), enumerate the situations that qualify, and
repeat the pointer at the two later points where the run actually
decides it is writing an integration test: the `integration` project
bullet under "Where the test file goes", and the tier list. The top of a
body is read before the model knows it needs the second skill, so a
pointer that lives only there has already been scrolled past by the time
it matters.
Measured at RUNS=5 on the full suite: the two hand-off cases go to
100% / 80% (100% / 100% on a focused `-- test-servers` run), and all
63 first-move cases stay at 100% — no displacement, as expected, since
neither description changed.
Also update the two places that recorded the old number as the standing
state: the worked example in `docs/skill-authoring.md`, and the
`CHAIN_THRESHOLD` rationale in `scripts/skill-eval.mjs`, which cites the
33% measurement as the reason the bar is 0.5 rather than 0.8. The bar is
unchanged — a reshaped pointer raises the ceiling those two cases reach,
not the floor a new hand-off case should be judged against.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW
Signed-off-by: cliffhall <cliff@futurescale.com>
`Record<string, unknown>` does not overlap `Crypto` (TS2352 under `tsc -b`).
`Partial<Pick<Crypto, …>>` says the same thing in one legal cast and keeps
`delete` operating on a known-optional property.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>
The `publish-github-container-registry` job's attestation step emitted two
warning annotations on every release — the only annotations on an otherwise
green release run:
Failed to create storage record: Error: Failed to persist storage record:
no artifacts found
Please check that the "artifact-metadata:write" permission has been included
The attestation itself was never affected: it signed, uploaded to Rekor, to
the repository and to the registry, and `gh attestation verify` passes against
the published 2.5.0 image. What was not landing is the separate artifact
metadata *storage record* — GitHub's org-level index of where a published
artifact lives.
Both of the issue's open questions are now answered:
1. `artifact-metadata` is a documented `permissions:` key (read/write/none),
so the workflow parses. Pushing this branch runs CI and validates that.
2. The `no artifacts found` text does NOT mean this job uploaded no *workflow*
artifact. It is the generic 404 body of the org artifact-metadata REST API
that `@actions/attest` POSTs to (`createStorageRecord`); the sibling read
endpoint returns the identical message for 2.5.0's real digest and for an
all-zeros digest that cannot exist. The documented precondition actually
unmet is this permission: the action emits a storage record when
`push-to-registry` is true AND the workflow carries `artifact-metadata:
write`, and the repo is org-owned as the API requires.
Confirmation that the record lands can only come from a release run, so the
step also carries a comment recording the evidence and the exact post-release
check — and what to do instead (`create-storage-record: false` plus a note) if
the warning survives. Either branch satisfies the issue's acceptance criterion
rather than leaving an unexplained warning in the release channel.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EgCNAyMgKX1Fm3uhrAyeP9
Signed-off-by: cliffhall <cliff@futurescale.com>
`clients/cli` resolved `@types/node` 24.13.1 against the root's 24.13.3.
Neither client declares it — #2196 consolidated it to the root — so the cli
copy arrived transitively via `@types/express` and was constrained by no
range of ours. `verify:dep-lockstep` reported OK and was right to: it
compares only packages that reach one `tsc` **program** from two installs,
and no one program sees both copies.
Fixes the instance and the class, which the issue notes are not exclusive.
**The class.** The guard grows a second tier alongside the program-derived
one, asking the weaker but broader question `AGENTS.md`'s "one version per
install-crossing dependency" rule actually states: does a package this repo
declares anywhere resolve to two versions across our installs at all? The
candidate set is every name in any install's `dependencies`/`devDependencies`
that more than one install holds a **top-level** copy of — 17 packages today.
Nested copies are excluded: one exists because some dependency asked for a
different version, so it is that dependency's range, not ours.
Neither tier subsumes the other. The program tier sees a copy no manifest
names (the `@modelcontextprotocol/sdk` case, arriving through another
package's `.d.ts`). The declared tier sees a transitive copy no program
loads (this issue), two *clients* disagreeing with no root copy involved
(`@types/react`, web against tui), and the peer shadows `AGENTS.md` calls out
as ungated — `eslint`, `typescript`, `vitest`.
Deny-by-default, with its own empty `TOLERATED_DECLARED_SKEW` and the same
within-a-major rule, now stated once in a shared `toleratesSkew` so the two
tiers cannot drift.
**The instance, and three others the new tier surfaced.** All four were
patch-level:
- `@types/node` — cli 24.13.1 vs root 24.13.3. Transitive, so it takes an
`overrides` entry in `clients/cli` (the repo's sanctioned lever for a
transitive pin), at the root's own `^24.12.4` rather than a frozen number.
- `@types/react` — web 19.2.18 vs tui 19.2.17.
- `react` — web 19.2.8 vs root/tui 19.2.7.
- `@vitejs/plugin-react` — web 6.0.5 vs root 6.0.2.
The last three are `npm update` in the stale install: it moves the lockfile
within the declared range without widening a range, which matters for the
root `react ^19.0.0` that `ink`'s externalized-bundle exemption depends on.
No declaration or range changed.
Tests: 4 end-to-end cases (the fixture gains manifest deps and a client-side
`solo` install, so the skew is real and no program holds both copies) and 13
helper cases.
Closes#2226
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings from the v2.5.0 milestone-merge review (#2215).
1. `progressToastId` collided across distinct progress streams.
`ProgressToken` is `string | number`, so `String(token)` mapped the
numeric token 7 and the string token "7" onto one id — and because
notifications keyed by the same id are *replaced* rather than
stacked, two concurrent streams overwrote each other's toast. The
absent case was worse: it hardcoded the sentinel "default", which a
server is free to send as a genuine string token. The id now carries
the token's type (`progress-n:7` / `progress-s:7`) and gives the
no-token case a prefix of its own (`progress-none`) that no token
value can produce.
2. `newAttemptId`'s fallback now prefers `crypto.getRandomValues`.
`randomUUID` needs a secure context; `getRandomValues` does not and
exists in every browser that has `crypto` at all — so the exact
situation the fallback exists for (a `file://` page, a plain-HTTP
non-loopback host) still has a CSPRNG on hand. `Math.random` stays
as the last resort for a `crypto`-less global, and the "never a
security token" comment stays too. Retires CodeQL alert 72
(`js/insecure-randomness`) honestly rather than by dismissing it.
Tests: collision cases for 7 vs "7" and the absent token vs "default" /
"none", a distinctness sweep over the whole id space; and the attempt-id
fallback test split into a getRandomValues arm (asserting the CSPRNG is
used and `Math.random` is not) and a no-crypto-at-all arm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 16 reported no inline comments and three suppressed findings, all
valid:
- `getSkillsExtension` treated a non-object extension value (`false`,
`"skills"`) as a declaration. SEP-2133 declares an extension as an
object of sub-options, so a primitive is not one — now rejected,
matching `appElicitation.ts`.
- The `onGetSkill` prop doc claimed the two responses "must agree" and
that a difference means a broken server, contradicting the
implementation, which reports a difference as a finding rather than a
fault. Reworded to what the code does.
- No real-transport test exercised the fixture's `skills/*` handlers.
Added `src/test/integration/mcp/inspectorClient-skills.test.ts`,
parameterized over both protocol eras: capability advertisement, the
paged `skills/list` walk (direct and through `ManagedSkillsState`),
`skills/get`, the `-32602` for an unknown URI, `resources/read` of a
skill file, and delegation of an ordinary resource through the
fixture's `resources/read` wrapper.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 15 generated no new inline comments; all five suppressed findings
were real and unaddressed.
- SkillsScreen: claim the attempt BEFORE issuing the request, for both the
SKILL.md preview and `skills/get`. Recording it only on settle left a
window where an older request that happened to resolve first was still
considered current and published while a newer one was in flight —
`verifyRow` already claimed its row up front, these two did not. Tests
answer the OLDER request first and assert nothing is published.
- SkillsScreen: "No skills listed", not "No skills". SEP-2640 lets a server
return an empty or partial catalog and says an empty result is not proof
it has none — an unlisted skill is still fetchable by URI — so the old
copy had the tool asserting something the protocol explicitly does not.
- SkillsScreen: each row's Verify button gets an `aria-label` carrying its
URI. Every row's visible text is "Verify" and the URI cell is not
programmatically associated with the button, so a screen-reader user had
no way to tell which file each control checked.
- inspectorClient: attribute a rejected `skills/get` envelope with
`markResponseRejected`, so the Protocol tab stops rendering it as a clean
success while the screen shows an error. In the client rather than a
store because `skills/get` has none. Gated on `isClientDecodeRejection`,
with a test that a transport failure is NOT attributed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
The review finally quoted the normative text, which is what I asked for
across five rounds before declining: SEP-2640's `skills/list` section says
"In protocol versions 2026-07-28 and later, the result also carries …
`ttlMs` and `cacheScope`." So the era-aware validation is in.
- skillsSchemas: `ModernListSkillsResultSchema` = the page plus the base
list envelope, with field shapes mirroring `ModernResultEnvelopeSchema`
in `listSalvage.ts` so this repo's two statements of a modern envelope
cannot drift. `InspectorClient.listSkills` picks it from the negotiated
era; the legacy shape stays permissive, because those are 2026-era
attributes a legacy server has no business sending.
Nothing else catches this: `skills/*` is consumer-owned, so it is absent
from the SDK's cacheable-method registry and a modern server answering
`{ skills: [] }` was reaching the conformance UI as a clean list.
- skills.ts: count `frontmatter.description` (and `name`) length in Unicode
CODE POINTS. `String.length` counts UTF-16 code units, so 600 non-BMP
characters measured as 1200 and a perfectly valid description was
reported `malformed-description` — a conforming server failed by an
off-by-encoding, which is the direction this module works hardest to
avoid. The Agent Skills reference validator uses Python `len()`.
- #2248 narrowed: the `skills/list` half is settled here; what remains is
whether `skills/get` carries the same attributes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 3 on #2256: both detectors were one-sided. Each measured
the sub-floor remainder against the real clock, so a preempted worker
overshoots the deadline, the remainder goes negative, and the unfixed
`remainingMs <= 0` bound takes the same branch and prints the same
message — the test passes on an implementation with no floor. It could
never fail wrongly, but it could silently stop testing anything, which on
a flake fix is the wrong half of that guarantee to keep.
Both now stub `performance.now()`: the web case advances it inside the
fetch so the second grant's remainder is exactly
MIN_REVOCATION_REQUEST_BUDGET_MS - 1, and the CLI case freezes it so the
remainder at the check is the budget itself. Positive and under the floor
on every machine, which is the only state that separates the two bounds.
Verified as detectors with the clock stubbed: reverting the core floor
gives 1 failed / 57 passed, reverting the CLI floor 1 failed / 14 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 1 on #2256: the `budgetMs: 0` case passes under the old
`remainingMs <= 0` bound too, so nothing in the CLI suite detected the
floor added to `sendPlans`.
The new case gives it a budget that is positive and below the floor, and
asserts on *this loop's* exhaustion message — the one naming the server
URL. That distinction is the test: with the floor removed the plan is
handed a 4ms budget and core's identical floor declines it one level
down, so a bare "budget was exhausted" match passes either way. Verified
as a detector — 1 failed / 14 passed with the CLI floor reverted, 15
passed with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99
Signed-off-by: cliffhall <cliff@futurescale.com>
- SkillsScreen: put the SESSION in the invalidation key. This screen stays
mounted across a disconnect, so content alone cannot tell server A's entry
from an identical-looking one on server B — A's in-flight verification
could land afterwards and report `verified` for a file never read from B,
and a retained batch entry could leave B's Verify all disabled.
`useInspectorStores` now exposes a `sessionNonce`, bumped on both create
and destroy so it never repeats across a reconnect, and App keys on
`${activeServerId}:${sessionNonce}` — the server id alone would repeat,
which is one of the crossings this exists to prevent.
- SkillsScreen: match the selection by `skillUriIdentity`, in both the
lookup and the NavLink active check. A refresh that canonicalizes
`skill://demo/%53KILL.md` names the same skill, and the detail pane was
emptying out because the server changed its spelling — the last raw-string
URI comparison left after round 12.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
The shared teardown deadline was decided on `remainingMs > 0` against
`Date.now()`. Both halves of that are noise-sensitive: the deadline is
enforced by a `setTimeout`, and a wall clock at millisecond resolution
can still read a hair short of it when the loop comes round, so the next
grant inherits a fractional budget and spends it on a request that cannot
possibly complete. The same run then issues one request or two depending
on scheduling — which is what makes the "shares one deadline across
grants" test intermittent on CI (#2252).
Measure the deadline with `performance.now()` (monotonic, so an NTP step
or a suspend cannot expire or extend the budget, and sub-millisecond, so
none is lost to rounding), and skip any grant reaching the loop with less
than MIN_REVOCATION_REQUEST_BUDGET_MS left. The skipped grant is already
reported as `failed`, so nothing is silently dropped — the needless call
to the authorization server is.
The CLI's outer per-plan budget in `sendPlans` shares both the shape and
the defect, so it gets the same treatment.
One consequence worth naming: a monotonic clock hands `revokeToken` a
fractional budget, and Node's `AbortSignal.timeout` throws
`ERR_OUT_OF_RANGE` on a non-integer delay — before the fetch, so the
request would never be sent and the revocation would report that as its
failure. `revokeToken` now rounds its own budget to whole milliseconds,
which also covers any other caller passing a fractional one.
Tests: the epsilon skip is pinned by a grant that lands inside the floor
but above zero (removing the floor fails it, verified); the fractional
budget by a `revokeToken` call at 19.996ms.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99
Signed-off-by: cliffhall <cliff@futurescale.com>
`ServerImportJsonModal` > "guards against a live edit made before the
debounce re-validates" opened its window on the real clock: it pasted
invalid JSON and clicked Add Server, relying on less than
VALIDATE_DEBOUNCE_MS (300ms) of wall time elapsing in between. When more
did, the debounce landed first, `canAdd` went false, and the click hit a
disabled button — a no-op that sets no submit error, so the
`findByText(/Fix the validation errors/)` timed out. The first assertion
(`onAddServer` not called) still passed, which is why the failure read as
mysterious rather than as a disabled button.
Run the test on fake timers end to end instead. The first validation is
landed explicitly with `advanceTimersByTimeAsync(VALIDATE_DEBOUNCE_MS)`
— imported from the hook, so the test cannot drift from the value it
depends on — and after the second paste the timers are simply not
advanced. The pending re-validation therefore cannot land at all, the
window stays open by construction, and the final assertion becomes a
synchronous `getByText`.
Verified with a throwaway probe holding both shapes side by side with a
deterministic 400ms stall injected between the paste and the click: the
old shape fails exactly as reported, the new one passes.
No timeout was widened and no production code changed.
Closes#2250
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQgwZ1kGzhkdkMJ81JVg42
Signed-off-by: cliffhall <cliff@futurescale.com>
All three findings were the same underlying inconsistency: URI comparisons
on raw strings, in a flow that elsewhere deliberately treats normalized
equivalents as the same resource. One `skillUriIdentity` helper now backs
every comparison, so they cannot disagree.
- skills.ts: `manifest-missing-self` compares identities, so a manifest
listing the RFC-equivalent `skill://demo/%53KILL.md` is recognized as the
entry's own file — it is fetchable as that file, so reporting it missing
was the tool disagreeing with itself.
- skills.ts: `skillEntriesMatch` normalizes the entry URI and every manifest
URI before comparing, so a server that canonicalizes an escape between the
listing and the fetch is not reported as a changed snapshot.
- SkillsScreen: the `wrongUri` check uses the same identity, so a
canonicalizing server is not accused of answering for a different skill —
the read path already accepts exactly that equivalence.
- skills.ts: apply the Agent Skills name grammar to the RAW value. Trimming
first let `" demo "` pass, and whitespace is not in the grammar — so an
entry could report "Conforms" with a name that can never equal its URI
path segment. The trimmed copy now only distinguishes absent from invalid.
- skills.ts: add `malformed-description` for a description above the
1024-character Agent Skills limit, with boundary coverage.
- inspectorClient-skills.test: justify the `as unknown as` per AGENTS.md —
both fields are `private` with no public setter, the asserted shape is
exactly what the class declares, and the alternative is a live connection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
- skills.ts: apply the two RFC 3986 §6.2.2 percent-encoding normalizations
the URL parser does not — decode an escape standing for an unreserved
character, and upper-case the hex of every escape that remains. `URL.href`
leaves `%72eference.md` encoded, so a server echoing an RFC-equivalent
form of the URI we asked for was rejected by `onReadSkillFile` as a
different resource, and an encoded name segment produced a false
`name-path-mismatch`. Both are the tool calling a conforming server wrong.
- oauthResume.test: assert the Skills restore path. It was wired into the
setters but never checked, and the comprehensive present-value and
undefined-fallback cases omitted Skills entirely. Adds it to both, plus a
case restoring a REAL saved selection rather than the EMPTY value — a bug
that always wrote the default would have passed the others.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
- skills.ts: validate `frontmatter.name` against the Agent Skills format
(1–64 lowercase alphanumerics and hyphens, no leading, trailing or
consecutive hyphen). Checking only for non-emptiness let `Bad Name` read
as "Conforms" — and the name is not decorative: it must equal the URI
path segment, so a name that cannot appear in a URI is a contradiction
the entry cannot satisfy. Suppressed when the name is absent entirely,
which `missing-name` already reports.
- skills.ts: detect duplicates on the NORMALIZED identity. Containment and
the read that fetches the bytes both treat normalized-equivalents as one
resource, so `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md`
were passing as two distinct files while naming one. The finding still
reports the raw URI, and two different unparseable URIs stay distinct.
- SkillsScreen: run `checkSkillConformance` on the fetched `skills/get`
entry and treat an error — or an entry answering for a different URI —
as invalid (red), not as "a different snapshot" (yellow). A fresh
point-in-time read excuses a CHANGE; it does not excuse a violation, and
answering with another skill is never a refresh of the one requested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
- SkillsScreen: track "Verify all" batches in a MAP keyed by manifest, not
one slot. A slot remembers only the most recent batch, so A running →
start B → return to A left A's button enabled and a second worker pool
could be started on top of A's first, doubling the concurrency cap. The
round-7 test switched to B but never STARTED B's batch, which is exactly
why it missed this; it now does, and fails against the old single slot.
- test-servers/skills.ts: pass `result` schemas to `setRequestHandler` for
both custom methods, and type the three builders from them, so a shape
change in `toEntry` / `listSkillsPage` fails `tsc` rather than shipping a
fixture that claims to conform. Declared locally rather than imported
from `core/mcp/skillsSchemas.ts`: a fixture validated against the
client's own schema could never catch the client being wrong.
One correction to the review's premise on that second point: the SDK does
NOT require `{ params, result }`, and supplying `result` does not validate
the response. Its own doc on `RequestHandlerSchemas` says `result` is
optional and "no runtime validation is performed on the result" — it types
the handler's return value. The change is worth making for that
compile-time check; it does not do what was claimed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
- useServerCommands: require the returned URI to match the requested one
in `onReadSkillFile`. The "a single-block response must be the block we
asked for" fallback is right for a viewer and wrong here: these bytes
are hashed against the requested URI's advertised digest, so accepting
a block the server labelled `b.md` would verify one file's content
against another file's digest — and could report it `verified`. A
normalized equivalent is still accepted (a server may echo a resolved
`..`), but two unparseable URIs do not compare equal, and the error
names what came back instead.
- test-servers/skills.ts: build each `SKILL.md` FROM its frontmatter
object. Three fixtures listed one description and served another, which
is an undocumented extra violation — SEP-2640 requires the listed and
served frontmatter to be identical — and would have made phase 3's
frontmatter check report a finding these fixtures were not built to
demonstrate. Deriving one from the other makes that drift impossible
rather than merely fixed.
The PR description's severity table also still described the round-1
contract (limits as errors, digest/size/description as warnings) while
the code says the reverse. Rewritten to match, with the MUST/SHOULD split
stated rather than implied.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
- skills.ts: stop sorting `resources` arrays recursively. Only the entry's
OWN manifest is a set; `frontmatter` is verbatim arbitrary JSON, so a
custom `frontmatter.metadata.resources` array was being sorted too and
two genuinely different frontmatters compared equal. The manifest sort is
now done once, at the top level, and every other array keeps its order.
- SkillsScreen: invalidate on the whole selected entry, not just its
manifest. The `skills/get` comparison covers `frontmatter`, so a refresh
that changed only a description left a stale "matches skills/list"
verdict on screen — computed against an entry that no longer existed.
Re-verifying after a metadata-only refresh is the cheap direction to be
wrong in; showing a match against a different entry is not.
- SkillsScreen: give each "Verify all" invocation a token, so a finalizer
can only ever clear its own batch rather than any batch with a matching
manifest key.
- SkillsScreen.stories: supply `onGetSkill`. Every story rendered the
"Fetch with skills/get" button, and clicking it threw.
On the batch token: the A → B → A sequence the review describes is not
actually reachable, because the button is disabled whenever the in-flight
batch's key matches the selection — returning to A finds it disabled, not
clickable. The token is defence in depth for the same property, and the
test asserts what is really guaranteed rather than the unreachable path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
Most of this round re-reported code the previous commit already changed
(it generated 0 new comments). Two findings were genuinely open.
- SkillsScreen: give "View SKILL.md" and "Fetch with skills/get" the same
attempt ordering `verifyRow` got in round 3. Both were still
last-completion-wins within one manifest: a double click whose older
request settled last replaced the newer result. One shared `isStale`
helper now covers all three actions — a result is dropped when its
manifest was invalidated OR a later click already wrote.
- test-servers/skills.ts + docs: stop calling all three edge cases
"non-conforming". `resources: "dynamic"` is a LEGAL wire form for
generated content — it is unverifiable, not invalid, which is exactly
why the checker reports it as a warning. The prose contradicted the
code it was describing. Now: one legal-but-unverifiable case and two
actual violations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
Two of these reverse round 4, which asked for the opposite. Where the
readings conflict I took the permissive one: a wrong tightening rejects
conforming servers, a wrong loosening only fails to report something.
- skills.ts: stop requiring the `skill:` scheme. SEP-2640 says a server
SHOULD use `skill://` and explicitly allows a domain-native scheme, so
round 4's requirement handed a conforming `github://…` skill a false
`malformed-uri` and skipped its name and root checks. Containment is
scheme-independent — it compares against THIS entry's own root — so the
traversal fix that requirement came packaged with is unaffected.
- skills.ts: the 512-entry and 16 MiB limits are warnings. Both are
SHOULD NOTs for a server and MAYs for a host, so calling them errors
contradicted the "error = a MUST was broken" contract round 4
established and told authors a permitted skill was invalid.
- skills.ts: `skillEntriesMatch` compares canonically — keys sorted,
manifest sorted by URI. `JSON.stringify` treated key order and manifest
order as differences, though neither carries meaning.
- SkillsScreen: a differing `skills/get` is "a different snapshot"
(yellow), not a disagreement (red). The SEP defines it as a fresh
point-in-time read, so a skill that changed since the listing
legitimately differs.
- useServerCommands: route `onGetSkill` through the command-scoped OAuth
recovery like every other server command. An expired authorization
rendered an error and stopped there — no reauthorization, no retry.
- test-servers/skills.ts: stamp the modern base result envelope
(`resultType` / `ttlMs` / `cacheScope`) on every skills result. The SDK
stamps it only for methods in its own codec, and `skills/*` are
consumer-owned, so a 2026-era connection was receiving results without
it. Unconditional rather than per era: the modern leg builds a server
per request, so there is no era to branch on at registration time, and
on legacy they are three members no codec inspects.
- composable-test-server: remove the `skills.directoryRead` option. It was
publicly settable and advertised a method nothing serves, producing the
exact false capability the fixture exists to help catch. `skills` is now
a plain boolean; the flag returns in phase 3 (#2248) with its handler.
Still not changed: whether a modern `skills/list` RESULT SCHEMA must
require that envelope. Stamping it server-side is free; requiring it
client-side would reject servers on a reading nobody has checked against
the normative text. #2248 owns settling it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
Three things the Skills work got wrong in that modal, all found by review
of the PR screenshots.
- The "Skills Extension" section repeated `io.modelcontextprotocol/skills`
as its value — the identifier the "Server Extensions" section two rows
above already lists, so the section added nothing. What a flat key list
*cannot* show is the extension's sub-options, which is the fact a server
author opens this modal to check. Renamed "Skills Extension Options" and
rendered as a ✓/✗ row for `directoryRead`, in the same vocabulary as the
capability columns, so it reads as the same kind of claim.
- The extension sections' contents were bold (`ValueText`, the value half
of a label/value pair) while sitting directly beneath the capability
checklists, which are plain. They are lists of items, not values, so
they now use the same `Text` the checklist rows do.
- Those lists were comma-joined into one line, which wraps mid-identifier
in a half-width column. One row per identifier.
The `skills-directory-read` tests now assert `data-supported` rather than
the copy: "Not supported" contains "Supported", so a text assertion passed
for either answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
- SkillsScreen: actually call `skills/get`. The client method and its
tests existed but no production caller invoked it, so #2234's acceptance
criterion ("skills/get retrieves a single entry") was unmet and a server
author's required handler could not be exercised. The detail pane now
fetches the selected URI on demand and reports whether the fetched entry
AGREES with the one skills/list advertised — both describe the same
skill, so a disagreement is a server bug only a side-by-side fetch shows.
- core/mcp/sha256.ts: a dependency-free SHA-256, used when `crypto.subtle`
is absent. `SubtleCrypto` needs a secure context, and this app is
documented as servable over plain HTTP on a LAN IP — where every skill
verification threw and the UI reported a read failure for files it had
fetched fine. Checked against the FIPS 180-4 vectors and differentially
against WebCrypto.
- skills.ts: require the `skill:` scheme in `normalizeSkillUri`. Checking
only that a URI was hierarchical let `https://demo/SKILL.md` pass the
name and root checks — a manifest pointing anywhere on the web, reported
as conforming.
- skills.ts: `missing-digest` and `missing-size` are errors. Both are
required fields, and an omitted `size` is what lets a server slip past
the 16 MiB pre-fetch limit while the UI reports zero errors. `warning`
is now reserved for what is legal yet unverifiable — `"dynamic"`.
- SkillsScreen: key the "Verify all" batch guard to the manifest. A global
flag left a newly selected skill's button disabled until the previous
skill's reads settled — forever, if one hung.
Not changed, deliberately: whether a modern-era `skills/list` result MUST
carry the SEP-2549 caching attributes. #2234's analysis records it as open
and the review asserts the opposite; neither reading was checked against
the normative text. Leaving the schema permissive accepts a server that
omits them, while tightening on a wrong reading would reject conforming
responses — the more expensive direction. `skillsSchemas.ts` states this
and #2248 settles it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
