feat: Skills extension phase 3 — CLI methods, TUI pane, resources/directory/read, and the frontmatter cross-check

[cliffhall]

Closes #2248

Phase 3 of the Skills extension (SEP-2640) — the "Reach" work #2234 named but did not gate on. Everything in the issue's Acceptance list ships here, including the frontmatter cross-check #2234 deliberately left open.

The gap a digest cannot close

SEP-2640 requires that a served SKILL.md's own YAML frontmatter match, field by field, the frontmatter its listing advertised. It is easy to assume the digest already covers this. It does not:

A digest is taken over the bytes the server served. It proves the file was not altered in transit, and says nothing about whether the listing described that file honestly.

So a server can advertise one description, serve another, recompute the digest and size to be perfectly honest about the body it sends, and pass every integrity check — while what the user approved from the catalog is not what the model receives. checkSkillFrontmatterMatch closes it, reporting one finding per differing field ("the listing says X, the served file says Y"), because "frontmatter does not match" is unactionable for the author who has to fix it. Every finding is an error: the SEP makes a discrepancy "a verification failure equivalent to a digest mismatch", and ranking it below one would contradict that.

Two comparison details that would otherwise report a conforming server as broken: values compare as canonical JSON, so a nested mapping agreeing on content compares equal despite key order (array order is significant — a YAML sequence is ordered); and the parser is pinned to the YAML 1.2 core schema, under which a date-shaped value stays the string it was on the wire rather than becoming a Date that could never equal the JSON string it came from.

The dependency

core/mcp/skillFile.ts is the one place in core/ that imports a YAML parser, and it is imported rather than approximated: a regex would report a conforming server as broken the first time a description carried a colon, a quoted string, or a block scalar, and for a tool whose entire output is "does this server conform", a checker that is itself wrong is worse than no checker.

yaml was already a repo-root dependency (reached from test-servers/src/load-config.ts), so this adds no package to any manifest. What it does add is yaml to core/'s runtime import graph, which is why it joins all three bundler external lists in the same change — per the dependency-placement rule, a root-declared dependency core/ imports must be named in each, or tsup inlines it.

resources/directory/read

#2234 shipped no schema for this on purpose — an unexercised guess in the module that is meant to be the authority on the wire format could sit wrong indefinitely. It is defined here against the normative text, alongside the call that uses it.

• The child type is the SDK's own Resource, not a shape restated here. The SEP defines the result as carrying "the same Resource objects that resources/list returns", so the schema that already decodes resources/list is the literal statement of that sentence — and one the SDK keeps current. ⚠️ It is stricter than everything else in the module (it requires name and strips unknowns, so one bad child rejects the page), and that inversion is deliberate: Resource is base-protocol and already validated exactly this strictly on the resources/list path, where listSalvage is the answer to a single bad entry. Being more permissive here would mean a URI that fails as a listed resource succeeds as a directory child.
• The modern variant requires resultType and deliberately not ttlMs / cacheScope. The asymmetry with ModernListSkillsResultSchema is the one judgement call in the module, so it is written down: for skills/list the SEP states the requirement outright; for this method it states nothing of the kind, and its one worked example carries resultType alone. Requiring more would fail a server that matched the spec's own example.
• The client refuses the call locally when the server has not declared directoryRead — the SEP's flat MUST NOT. Refusing here also keeps the Protocol tab honest: a request we were never allowed to make should not appear in the exchange log as a server-side failure.
• A directory read does not extend the manifest. The SEP calls the result "a live observation" and forbids treating it as extending the entry; to a host acting on the skill, reading an unlisted child is a verification failure exactly as a digest mismatch is. The Inspector is not a host and does not refuse the read — what it must not do is present the child as one of the skill's files, so rows are marked listed / not listed and a disagreement is explained in prose with the recovery path the SEP names (re-fetch with skills/get), rather than as a read error.

CLI

skills/list, skills/get and resources/directory/read, plus --verify: one JSON report per skill on stdout, a one-line summary on stderr so a reader who piped into jq still sees the verdict, and exit 7 on a violation.

SKILL_NONCONFORMANT is its own code rather than reusing SCHEMA_UNPORTABLE, for the reason that one exists at all: a CI job failing on an unportable tool schema and one failing on a tampered digest are different jobs, and collapsing them makes if [ $? -eq 6 ] ambiguous.

ok is false for anything the SEP makes a MUST; a warning never fails it. That matters most for resources: "dynamic", a conforming wire form — failing CI for it would tell server authors their valid skill is broken.

Reads are sequential, deliberately: a conforming manifest may declare 512 entries, and a parallel walk would open 512 resources/read calls against the server under test — the hazard the web screen bounds with a concurrency limit. Sequential also makes the report deterministic, so a CI diff of two runs shows what changed rather than what raced.

--verify's argument validation sits with --strict's, ahead of every short-circuit return in parseArgs: those returns never reach runMethod, so a check placed lower would let --verify --method servers/list be accepted and silently ignored.

The skills/list walk reuses ManagedSkillsState rather than re-implementing pagination — it carries the repeated-cursor and page-cap guards, and a second copy is how the two come to disagree. What the CLI adds is an explicit extension check: the store answers "no extension" with an empty list, which is right for a UI that must render something and wrong for a CLI, where "this server has no skills" and "this server does not serve skills" are answers a script has to tell apart.

TUI

A Skills pane, shown only when the connected server declares the extension — unlike the transport-derived Auth/Console/Network gates, this one keys off a server declaration and so is only knowable after connecting.

Each row carries its conformance verdict as a glyph as well as a colour (✓ / ! / ✗), because this pane is read over ssh, inside tmux and piped through script(1), where colour may not survive. Enter verifies the selected skill; the static checks run on every render, but digest verification needs the bytes, so it is asked for — the same split SEP-2640 makes.

One behavioural fix found by the coverage gate

verifySkills now re-throws AuthRecoveryRequiredError instead of recording it as a per-file read-error. It is not a property of the file in flight — it says the session's authorization expired, so every remaining read fails the same way. Absorbed, it produced a report of N identical read failures and swallowed the one error the TUI pane and the web commands key off to start a reauthorization, leaving the user told the files could not be read with no way offered to fix it.

Two open questions, settled

Both recorded in the code, where the next reader will look:

• skills/get caching attributes. Not era-aware, and that is settled rather than pending. SEP-2640 forecloses it in as many words: whether the result should also carry ttlMs / cacheScope "is left open." There is no requirement to enforce, and inventing one would do real harm — a server reasonably reading "left open" as "not required" would be reported non-conforming by the tool whose job is to tell it whether it conforms. looseObject means a server that does send them still parses.
• No paged mode, and not because the walk is cheap. Every consumer of this list is a whole-catalog verdict: the screen's conformance summary and the CLI's exit code are statements about the catalog. Computed over page one of three, "this server's skills conform" is not merely partial — it is wrong, with exactly the confidence of a real pass. Paging the other lists costs a reader some rows; paging this one would make the tool's own output untrue. The page count is surfaced instead (web and TUI), which serves the setting's purpose without staking a verdict on a partial read.

Fixture

Two new skills, both for checks that were otherwise undemonstrable, and directoryRead now declared:

Skill	What it exercises
lying-listing	Advertises one description and serves another. Its digest verifies — the one violation only the frontmatter check can catch. The only fixture whose SKILL.md is deliberately not derived from its listed frontmatter.
stale-manifest	Serves and directory-lists a file its resources manifest does not declare. The entry is otherwise fully conforming and verifies clean, so the disagreement between the two views is the whole defect.

The directoryRead declaration and the handler are one switch: the sub-flag's whole hazard is advertising a method nothing answers, and a config that cannot express the declaration without the handler cannot reach it. The undeclared case is still testable — against any config without "skills", where the client must refuse locally.

Conformance scenarios

The MCP conformance harness grades a client by standing up a hostile server and watching what the client does. Five of its scenarios cover the client-side obligations SEP-2640 makes observable on the wire:

Scenario	Status
no-prefetch — connect, skills/list, exit; fails if any resources/read arrives	✅ satisfied structurally — nothing is fetched until a user selects a skill or presses Verify, which is why every round trip on the screen is a button rather than an effect
verify-digest — a same-length, different-bytes body	✅ (tampered-notes)
verify-size — extra bytes the entry's size did not account for	✅ — size is cross-checked before the digest, and fails on its own
verify-frontmatter — a hostile description with digest and size recomputed to be honest	✅ new in this PR — would have failed before it
verify-unlisted — a read invited for a URI absent from resources	⚠️ surfaced, not refused — see below (stale-manifest)

⚠️ One honest caveat. verify-unlisted grades a host, whose pass is refusing the read. The Inspector has no concept of loading a skill and so has nothing to refuse; it reports the divergence instead. That is the right behaviour for a tool whose job is to show a server author what their implementation is doing, but it is a different answer from the one a host gives and should not be reported as the same.

Testing

• core/: skillFile (split + parse), skills (the frontmatter cross-check), skillsSchemas (the directory schemas, including the SEP's own worked example), skillsVerification (the fetch-and-verify walk, the auth re-throw, the per-file failure handling).
• clients/cli: dispatch for all three methods, --verify's NDJSON/summary/exit-code path, and the flag's short-circuit validation.
• clients/tui: the pane, the tab gate, the accelerator.
• clients/web: the Directory section (descend, ascend, page, empty, error, unlisted-child marking) and the frontmatter findings.
• Integration, over a real transport on both eras: the directory walk with its cursor, the -32602 for a non-directory, the stale-manifest divergence, and a whole-catalog --verify that fails exactly the three bad skills and passes dynamic-report.

npm run local:gate passes.

One pre-existing fixture bug the new check surfaced: SkillsScreen.test.tsx's "clean" skill listed a description its served SKILL.md did not carry. Every fixture's file is now derived from the frontmatter its entry advertises, which makes that class of drift impossible rather than merely fixed — the same discipline test-servers/src/skills.ts already applies.

Moves

splitSkillFile and skillFileBytes moved from clients/web/src/utils/ into core/ (skillFile.ts and skills.ts respectively) because the CLI and TUI now need them; their tests moved to src/test/core/mcp/ accordingly.

Screenshots

Captured headlessly against the built prod bundle connected to test-servers/configs/skills-http.json.

The Directory section. resources/directory/read against the skill root, paged — the fixture serves one child per page, so a client ignoring nextCursor is visibly wrong here rather than merely lucky. The Protocol panel carries both calls. It opens collapsed by default: it is the only section whose content needs a round trip nobody has made, so open it would show a button and an empty frame while taking height from the sections that have content.

A directory child the entry does not declare. stale-manifest serves and lists added-later.md while its manifest declares only SKILL.md. The row is marked NOT LISTED and the banner names the recovery path SEP-2640 specifies — re-fetch with skills/get — rather than presenting it as a read error. The entry itself is fully conforming and verifies clean, so nothing but this comparison can surface it.

A name collision. acme/reports and globex/reports are both entirely valid — the SEP requires only that the segment before /SKILL.md equal frontmatter.name, which multi-segment paths satisfy while sharing a final segment. The banner sits under the Conformance header and names the other skill; the badge reads 0 ERROR(S), 1 WARNING(S), and "No structural issues" still appears below it, because the entry itself is clean and the finding is about the pair.

The frontmatter cross-check. lying-listing advertises one description and serves another. Its digest verifies — the digest is over the bytes the server served — so this is the one violation only a real YAML parse can catch.

The TUI Skills pane. Captured from a 132×26 pseudoterminal driving the built binary against the same fixture. Every row carries its verdict as a glyph as well as a colour, because this pane is read over ssh and through script(1). The two reports skills share a name and warn; right-name fails the URI/name invariant. In the detail pane, the failed file names both digests — a verdict a reader cannot act on is not worth printing — and Listing checks: no structural issues sits above Verification FAILED without contradicting it: the static checks pass because the advertised digest is well-formed, and the bytes simply do not hash to it.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues can cause incorrect verification and inconsistent CLI, TUI, and web behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Implements Phase 3 of the Skills extension across core, CLI, TUI, web, and test servers.

Changes:

• Adds directory reads and YAML frontmatter verification.
• Adds CLI verification and a TUI Skills pane.
• Expands fixtures, tests, documentation, and bundler configuration.

Unresolved findings:

• Critical (1 vote): skillsVerification.ts verifies contents[0] instead of the URI-matching content at lines 95, 157, and 179, risking false passes or failures. Match normalized URIs and report a read error when absent.
• Moderate (1 vote): run-method.ts dispatches skills/get without checking that the server declared the Skills extension.
• Moderate (1 vote): App.tsx can retain the hidden Skills tab after switching to an unsupported server.
• Moderate (2 votes): SkillsTab.tsx keys verification only by URI, allowing stale results after the entry changes.
• Moderate (1 vote): SkillsScreen.tsx discards existing directory children and the retry cursor when “Load more” fails.
• Nit (1 vote): The integration test says five fixture skills although there are six.
• Nit (1 vote): docs/test-servers.md has stale special-case and failure counts.

File summaries

File	Description
test-servers/src/skills.ts	Adds directory handling and Skills fixtures.
test-servers/src/load-config.ts	Updates Skills configuration documentation.
test-servers/src/composable-test-server.ts	Advertises directory-read support.
docs/test-servers.md	Documents the expanded Skills fixture.
core/mcp/state/managedSkillsState.ts	Documents full-catalog pagination behavior.
core/mcp/skillsVerification.ts	Implements fetch-and-verify reporting.
core/mcp/skillsSchemas.ts	Defines directory result schemas.
core/mcp/skills.ts	Adds shared byte and frontmatter checks.
core/mcp/skillFile.ts	Parses Skill files and YAML frontmatter.
core/mcp/inspectorClientProtocol.ts	Exposes directory-read capability.
core/mcp/inspectorClient.ts	Implements directory-read requests.
clients/web/tsup.runner.config.ts	Externalizes YAML.
clients/web/src/utils/splitSkillFile.ts	Removes the migrated web-only parser.
clients/web/src/utils/skillFileBytes.ts	Removes the migrated web-only helper.
clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts	Exercises Skills over real transports.
clients/web/src/test/core/mcp/skillsVerification.test.ts	Tests verification orchestration.
clients/web/src/test/core/mcp/skillsSchemas.test.ts	Tests directory schemas.
clients/web/src/test/core/mcp/skills.test.ts	Tests frontmatter comparison.
clients/web/src/test/core/mcp/skillFileBytes.test.ts	Tests shared byte decoding.
clients/web/src/test/core/mcp/skillFile.test.ts	Tests shared frontmatter parsing.
clients/web/src/test/core/mcp/inspectorClient-skills.test.ts	Tests client directory reads.
clients/web/src/hooks/useServerCommands.tsx	Implements the directory-read command.
clients/web/src/hooks/useServerCommands.test.tsx	Tests directory command handling.
clients/web/src/components/views/InspectorView/types.ts	Extends Skills panel properties.
clients/web/src/components/views/InspectorView/InspectorView.tsx	Forwards directory callbacks.
clients/web/src/App.tsx	Gates directory browsing by capability.
clients/tui/tsup.config.ts	Externalizes YAML.
clients/tui/src/components/tabsConfig.ts	Registers the Skills tab.
clients/tui/src/components/Tabs.tsx	Adds Skills tab gating.
clients/tui/src/components/SkillsTab.tsx	Implements the Skills pane.
clients/tui/src/App.tsx	Integrates Skills state and pane rendering.
clients/tui/README.md	Documents the Skills pane.
clients/tui/__tests__/Tabs.test.tsx	Tests tab visibility and counts.
clients/tui/__tests__/SkillsTab.test.tsx	Tests Skills pane behavior.
clients/tui/__tests__/App.test.tsx	Tests Skills tab integration.
clients/cli/tsup.config.ts	Externalizes YAML.
clients/cli/src/handlers/skills-verify.ts	Formats verification summaries.
clients/cli/src/handlers/run-method.ts	Dispatches Skills CLI methods.
clients/cli/src/handlers/method-types.ts	Extends CLI method types.
clients/cli/src/handlers/consume-outcome.ts	Emits verification summaries and failures.
clients/cli/src/error-handler.ts	Adds verification failure exit code 7.
clients/cli/src/cli.ts	Adds Skills-related options.
clients/cli/README.md	Documents Skills CLI usage.
clients/cli/__tests__/skills-verify-cli.test.ts	Tests verification output and exit codes.
clients/cli/__tests__/run-method-skills.test.ts	Tests Skills method dispatch.

Review details

Suppressed comments (4)

clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts:125

• The fixture now contains six skills, as the three two-item pages below demonstrate. Saying five makes the pagination rationale inconsistent with the assertions and docs/test-servers.md.
  core/mcp/skillsVerification.ts:159
• A valid SKILL.md returned through the blob arm never gets its frontmatter checked. entryText is assigned only for text, even though the bytes immediately below are decoded and digest-verified, so CLI/TUI verification can return ok: true for a blob whose YAML disagrees with the listing. Decode the verified bytes as UTF-8 for the entry file before running the cross-check.
  core/mcp/skillsVerification.ts:190
• resources: "dynamic" can be reported as successfully verified even when its required SKILL.md cannot be read. This fallback swallows every non-auth failure, leaves files and frontmatter empty, and ok therefore remains true because the only static finding is a warning. A verification command must record this read failure (or otherwise force ok: false), since the mandatory frontmatter comparison never ran.
  docs/test-servers.md:94
• These counts are stale after adding lying-listing and stale-manifest. Five of the six fixtures are now special cases, and three are actual verification failures (tampered-notes, wrong-folder, and lying-listing); the later CLI section also says exactly three fail. Update this introduction so it does not contradict the table and expected CLI result.

• Files reviewed: 47/47 changed files
• Comments generated: 5
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Review round 1 — all five findings fixed

Mirrored here because inline replies go hidden once the fix is pushed.

Finding	Fix
Critical — skillsVerification.ts verified contents[0]	contentsFor(result, uri) selects by normalized identity, keeping the canonicalized-echo case that motivated the original code while rejecting an unrelated block; no match is now a read error
run-method.ts dispatched skills/get unguarded	Gate hoisted into assertSkillsSupported, applied to both skills/* methods
TUI could strand the user on a hidden Skills tab	Reset to info on the Auth precedent, additionally gated on connected
TUI verdict keyed by URI	Keyed on the entry, as the web screen already was
Failed “Load more” lost the listing and the cursor	Both preserved; success and failure now share one staleness-guarded commit
Nits — six-skill counts	Fixed; the docs sentence was rewritten rather than renumbered

Every fix has a test aimed at the failure named, not the happy path — the load-more test asserts the cursor survived by re-fetching successfully, not merely that the table stayed on screen.

Two notes worth recording

The critical one was a comment reasoning correctly and concluding wrongly. The code carried a justification for contents[0] — that a server may echo back a canonicalized spelling of the URI, so a string match would reject it. That premise is true; the conclusion did not follow, because normalized matching satisfies it too. Worth flagging because the failure mode is a false pass: one file's bytes hashed against another's digest and reported verified, which is an affirmative statement about a file nobody looked at.

Two of the five are the same mistake. The URI matching and the verdict key were both already solved correctly in the web client, with the reasoning written down there — and the new code did not follow it. That is one pattern, not two defects.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

Two critical verification gaps and one moderate frontmatter-validation issue remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skillFile.ts:111

• This cast admits YAML values that cannot exist in the listing's JSON object. Under the YAML 1.2 core schema, .nan and .inf become NaN/Infinity; later JSON.stringify converts each to null, so a served x: .nan incorrectly matches a listing containing x: null. Reject non-JSON-compatible parsed values before comparison.

clients/tui/src/components/SkillsTab.tsx:1

• This new source file has no file-level header; its first documentation is attached to declarations below the imports. Add the repository-required header explaining the file's purpose and why the TUI owns this pane, consistent with the other new production modules in this PR.

import React, { useCallback, useEffect, useRef, useState } from "react";

• Files reviewed: 47/47 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Added: skill-name collision reporting

SEP-2640 requires that hosts MUST NOT assume name uniqueness, and that two entries in one listing colliding on name be disambiguated "rather than silently discarding or preferring one." Nothing here reported that, and checkSkillConformance structurally cannot — it takes one entry, and a collision is a property of the pair.

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. 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 — the rule is only that the segment before /SKILL.md equal the name, which multi-segment paths satisfy while sharing a final segment. Calling it an error would tell a conforming author their catalog is invalid, so --verify still exits 0 for one.

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, and for the same reason: it changes how everything below it should be read, and stating one fact twice reads as two findings.

A bug found 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 derived once now and used for both.

The fixture gains acme/reports + globex/reports — both fully conforming, and the only fixture with a multi-segment skill path, which nothing else exercised. Eight skills over four pages; the integration test now walks the cursor to exhaustion rather than asserting a fixed page count, so a future fixture will not require editing it.

Screenshots are in the PR body. npm run local:gate passes.

[Copilot]

🟡 Changes recommended

Critical and moderate defects can incorrectly report nonconforming skills as valid.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

core/mcp/skillsVerification.ts:218

• For a "dynamic" entry this fallback is the only file read, but an ordinary rejection is swallowed. files then remains empty and warning-only conformance makes ok true, so --verify exits 0 even though the required frontmatter comparison never ran. Record this as a failing read (or a dedicated verification finding) instead of silently leaving entryText undefined.
  core/mcp/skillsVerification.ts:187
• A server may return SKILL.md as blob content, which skillFileBytes explicitly supports, but this captures frontmatter only from text. The digest can therefore verify while the fallback repeats the same blob read and skips the frontmatter check, allowing mismatched frontmatter to report ok: true. Decode the verified bytes for the entry resource as UTF-8 (while preserving text directly) before running the comparison.
  docs/test-servers.md:71
• Adding the directory handler makes the paragraph below inaccurate: readDirectoryPage intentionally emits only resultType, not ttlMs/cacheScope, yet the guide now says every result carries all three. Narrow that claim to the two skills/* results and document the directory result's deliberate exception.

• Files reviewed: 47/47 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Review round 2 — all four findings fixed

Two of these were silent holes in the verification itself, which is the failure class this PR exists to prevent, so they were the most valuable findings so far.

Finding	Fix
Critical — a blob-served SKILL.md skipped the frontmatter check	The entry's file is held as bytes; the text is derived from them via bytesToText
Critical — a dynamic skill with an unreadable SKILL.md reported ok: true	The fallback read records a read-error for all three failure shapes, so fileFailed fails the report
Moderate — a served .nan compared equal to a listed null	Non-finite numbers canonicalize to a form no JSON scalar can equal, and are named in the finding
Suppressed — SkillsTab.tsx had no file header	Added

Why the first two were invisible

They compounded. A dynamic skill has no manifest rows, so files stays empty; its only static finding is the dynamic-resources warning; and the frontmatter check was the one thing that could still fail it — but that check was being skipped, silently, whenever the read failed or the server used blob. The result was a verification that could not be performed being reported as one that passed. Neither shows up as a wrong answer in a test that uses a text-serving, reachable fixture, which is exactly what ours did.

One change beyond what was asked

The fallback read is now gated on manifestListsSelf (computed by normalized identity) rather than on "we have no text yet". Fixing the reported bug alone would have introduced a double read: a self-entry the manifest did list but whose read failed would fall through into the fallback and be reported twice. The manifest loop owns that case now; the fallback is only for a skill whose manifest never listed its own file.

On the .nan finding

I checked the premise before acting on it — yaml's 1.2 core schema does resolve .nan / .inf / -.inf to NaN / ±Infinity, and all three JSON.stringify to null, so .inf and -.inf also compared equal to each other. Tests cover all three, plus the case the fix must not break: a null the served file also writes as null still matches.

Coverage

The gate flagged every err instanceof Error ? … : … non-Error arm as uncovered. Rather than four v8 ignore comments, they are one reasonOf helper with a test that throws a bare string — one honestly-covered branch instead of four waived ones.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

Frontmatter comparison has correctness gaps, and the TUI omits actionable details for some verification failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skillFile.ts:105

• An explicit YAML null scalar (null, ~, etc.) also parses to null, so this treats a non-mapping frontmatter document as an empty mapping. Distinguish an empty/comment-only block from a non-empty scalar before returning { fields: {} }; otherwise parseSkillFrontmatter("null") contradicts the function's non-mapping contract.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1987

• This new Mantine Alert has two static styling props (color and variant) inline. The established pattern in this file is to extract such elements with .withProps() (see SkillsScreen.tsx:225-461); please define a named read-failure alert constant and leave only dynamic content at the call site.

                        <Alert color="red" variant="light" title="Read failed">

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:2000

• This new Mantine Alert also inlines two static styling props (color and variant). Match the surrounding file convention (SkillsScreen.tsx:225-461) by extracting a named .withProps() constant and passing the dynamic child count at the render site.

                        <Alert
                          color="yellow"
                          variant="light"
                          title="This directory lists files the entry does not"

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:2079

• This inline Text has two static styling props (size and c), while this file consistently extracts such Mantine elements with .withProps() (see SkillsScreen.tsx:225-461). Please use a named dimmed status-text constant here.

                                        <Text size="xs" c="dimmed">
                                          —
                                        </Text>

core/mcp/skills.ts:572

• The non-finite sentinel can collide with valid listed JSON. For example, a listing value { "#non-finite": "NaN" } and served YAML value .nan both canonicalize to the same object, so this mandatory frontmatter discrepancy is reported as a match. Detect non-finite values separately or use a type-tagged comparison encoding that no JSON value can alias.
  core/mcp/skillsVerification.ts:70
• This comment no longer matches the behavior below: a dynamic skill normally has no rows, but a failed mandatory SKILL.md fallback read adds a synthetic read-error row. Document that exception so consumers of this public report shape do not assume files is always empty for dynamic skills.

• Files reviewed: 47/47 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Review round 3 — two fixed, three already addressed

⚠️ This review ran against 4fa00d1c, the commit before the round-2 fixes landed in 033600e4. Three of its five findings are re-reports of issues already fixed. I verified each is genuinely resolved at HEAD rather than assuming it:

Re-reported finding	Where it is fixed at HEAD
blob-served SKILL.md skips the frontmatter check	entryBytes + bytesToText (skillsVerification.ts:178,225,269)
Dynamic skill's read failure swallowed → ok: true	fail() records a read-error (skillsVerification.ts:244,257,264)
.nan compares equal to null	non-finite canonicalization (skills.ts:572)

Each has tests. No action taken on these beyond confirming them.

The two live findings

Conformance: conforms printed 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, naming what the section actually covers. This also brings the TUI in line with the web screen, which hit the same problem and settled on "No structural issues" for the same reason.

The test-server guide claimed every result carries the full modern envelope — which my own directory handler made untrue. readDirectoryPage deliberately emits resultType alone: SEP-2640 states the caching attributes for a modern skills/list in as many words and says nothing of the kind for this method, and its one worked example carries resultType only. A fixture sending more than the SEP shows would make a client that wrongly required them look correct. The claim is narrowed to the two skills/* results and the exception documented as the deliberate choice it is.

Good catch on both — the first was a contradiction I had introduced and read past several times.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

The critical cyclic-frontmatter failure and moderate web verification inconsistency must be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1380

• The required frontmatter verification is coupled to previewParts, which only exists when the displayed MIME is recognized as Markdown and may come from an earlier read than the bytes hashed by “Verify all.” A SKILL.md served with a specific non-Markdown MIME therefore skips this check entirely, and a changing server can show a digest result for one snapshot with a frontmatter result from another. Parse and compare the same raw SKILL.md bytes used for verification, independently of presentation MIME.

• Files reviewed: 47/47 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 4 — all eight findings fixed

⚠️ Note on lag: each round has reviewed the commit before the previous round's fixes (r3 → 4fa00d1c, r4 → 033600e4). Round 4 raised nothing already-fixed, but it did not yet see the round-3 work either. Round 5 is requested against 79976354, which contains everything.

Two of these were visible in a terminal capture I had already taken and read past, which is worth recording — I had the evidence on screen and did not see it.

Finding	Fix
A digest mismatch rendered no digests in the TUI	failureDetail falls back to the expected/actual pair when there is no reason
A dynamic skill's read failure rendered nowhere	A Read failures block for report files the manifest does not cover
The non-finite sentinel could be aliased	Comparison is structural now; no sentinel exists to alias
parseSkillFrontmatter("null") returned { fields: {} }	An explicit null scalar is told from an empty block by the source
files doc comment claimed "empty for dynamic"	Corrected — the round-2 fix made it untrue
Three inline Mantine elements with two static props	Extracted to .withProps() constants

Why the two TUI ones hid

They are complementary halves of the same blind spot: verifySkillResource reports a size mismatch with a reason and a digest mismatch with digests-and-no-reason, and the pane rendered only reason. So the case that already worked was the one my fixtures happened to produce — my first attempt at a regression test declared the wrong size and exercised the working path, which is how a test can pass while proving nothing. The digest arm only runs when the declared size is correct.

On the sentinel

Round 3 flagged .nan matching null; my fix encoded non-finite numbers as {"#non-finite":"NaN"}. Round 4 correctly points out that a listing whose value genuinely is that object aliases the sentinel — so the fix moved the bug rather than closing it. Any encoding into the value space can be aliased by a document that contains the encoding, so there is no encoding now: jsonLikeEqual compares structurally, with Object.is giving NaN === NaN while keeping ±Infinity distinct. Serialization survives only in the message text, where aliasing cannot cause a wrong verdict. Tests cover both directions, including a listing that contains the old sentinel shape.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

A critical verification path can skip mandatory checks, and three moderate CLI/web output and state-safety issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

clients/cli/src/handlers/consume-outcome.ts:34

• A failing verification writes the plain summary here and then throws an error carrying the same summary; the top-level handleError renders that error as a second stderr line. This breaks the promised one-line stderr verdict and can confuse scripts that parse the error envelope. Let the normal error path be the sole stderr writer for failures.
  core/mcp/skills.ts:537
• The web client has not adopted this safety helper: SkillsScreen.tsx:845-848 still evaluates JSON.stringify(selected) during render. The same deeply nested server-controlled entry handled here therefore still crashes the web Skills tab before it can show the frontmatter-unparsable finding. Use skillEntryKey(selected) for the web manifest key as well.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:1081

• A preview read of SKILL.md can overwrite entryText after verification has completed, while leaving the existing digest/size verdict untouched. The frontmatter findings can therefore describe bytes from a different fetch than the displayed verification result, recreating the stale mixed-fetch verdict this state was intended to avoid. Only seed entryText from preview when no verification-derived text is present; a completed verification will still overwrite an earlier preview itself.

            }
            setVerification((prev) =>
              prev.key !== null && prev.key !== key
                ? prev
                : {
                    ...prev,
                    key,
                    files: prev.key === key ? prev.files : {},
                    entryText: text,
                  },

• Files reviewed: 51/51 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 15 — four fixed, one already resolved

The best finding of the review caught a contradiction between two of my own fixes. Round 12 was right that ok: true for a manifest whose unread 513th file may be tampered with is a false pass. Round 15 is right that forcing ok: false calls a server nonconformant for exceeding limits SEP-2640 states as SHOULD NOT — with hosts explicitly free to support more — contradicting this module's own rule that a warning never fails a report. Both are true, so a two-state answer was always going to be wrong somewhere:

outcome	Exit	Meaning
verified	0	Everything checked, everything passed
failed	7	A MUST was broken
incomplete	8	Nothing checked was wrong; the read bounds stopped the walk

A CI job that tolerates oversized catalogs can now allow 8 and still fail on 7, which is the point of not collapsing them.

manifestKey still used JSON.stringify — the same crash I fixed in the TUI in round 12, one file over. Directory children were navigable outside the skill root, with "Up" only comparing equality against it. The TUI matched report rows by raw URI, missing a row recorded under an equivalent spelling that extraReportFiles then suppressed as covered.

One thing the tests corrected

My first boundary test asserted that skill://data-analysis/../elsewhere/notes.md escapes the root. It does not — .. cannot cross the authority, so it normalizes to skill://data-analysis/elsewhere/notes.md and really is inside the skill. Rejecting it would have refused a legitimate child. That is now pinned as its own test, since the intuition is wrong in the direction that costs a false positive.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

Moderate issues remain in incomplete-verification handling, mandatory frontmatter fallback, and duplicate-name status display.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:807

• This collision map is merged into the selected detail view, but sidebar rows still compute their badge from checkSkillConformance(skill) alone. Both otherwise-conforming colliding skills therefore appear clean in the catalog until selected. Include the per-URI duplicate-name warning in the sidebar row findings too.

clients/tui/src/components/SkillsTab.tsx:536

• Use outcome for this status, not ok. An incomplete report deliberately keeps ok: true, so the current branch renders “Verified” for a clean-but-truncated verification and never reaches the INCOMPLETE label.

                    : activeReport
                      ? activeReport.ok
                        ? "[Verified — Enter to re-verify]"
                        : activeReport.incomplete
                          ? "[Verification INCOMPLETE — Enter to re-verify]"
                          : "[Verification FAILED — Enter to re-verify]"

core/mcp/skillsVerification.ts:366

• manifestListsSelf only means the self entry is in the declared-size-bounded slice; it does not mean the loop actually reached it. If received bytes exceed the runtime cap before that row, this condition skips the fallback and the mandatory frontmatter comparison never runs. Also fall back when no self-file report was produced by the loop.
  core/mcp/skillsVerification.ts:104
• This documentation still says incomplete makes ok false, but the tri-state implementation and the ok contract below intentionally keep ok: true when nothing checked failed. Update this paragraph so consumers do not implement the same incorrect branching now present in the TUI and CLI summary.

• Files reviewed: 51/51 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Review round 16 — two fixed, one already resolved, one still declined

Fixed — the byte budget could skip the mandatory frontmatter check. Another hole opened by one of my own fixes: round 13 added a break for the actual-byte budget, and manifestListsSelf described the bounded slice rather than the rows the walk reached. Breaking before a later self-entry left the flag true, suppressed the fallback, and skipped the check entirely. It is selfAttempted now, set inside the loop and marked before the read — a row the walk reached but could not read has still been attempted, and its failure belongs to the loop rather than a second fetch.

Fixed — a preview could overwrite a verification's bytes. The digest verdict on screen came from the verification's fetch; replacing only the text let the frontmatter findings describe different bytes, recreating exactly the mixed-fetch verdict this state was introduced to prevent. entryTextVerified records which read produced it; a preview no longer wins over a verification.

Already fixed — the web manifestKey JSON.stringify was resolved in round 15 (skillEntryKey, line 853). This review ran against 01a8f14c, one commit before it.

Still declined — the second stderr line on a failing --verify. Raised in round 10 and answered there; recording the position once more so it is not read as an oversight. --verify writes its human summary and then the ErrorEnvelope that every non-zero exit writes — the documented contract, and exactly what --strict does. Suppressing it would leave the human-readable verdict only inside a JSON message field and make this one command unparseable for a caller branching on .code. If the project wants envelope suppression for commands that have already reported, that is a change to --strict and --verify together; I would rather not diverge them inside this PR. The README states both lines and why.

npm run local:gate passes.

[Copilot]

🟡 Changes recommended

Incomplete verification is mislabeled as successful in CLI/TUI, and TUI capability changes can paint stale state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

clients/cli/src/handlers/skills-verify.ts:24

• Incomplete reports have ok: true, so an incomplete run currently prints “Verified … no conformance errors” even though the CLI exits 8 because it did not verify every file. Count outcome === "incomplete" separately and reserve “Verified” for reports whose outcome is actually verified.

  const failed = reports.filter((report) => !report.ok).length;

clients/tui/src/App.tsx:646

• This resets render state from capability-derived props in an effect, so after connecting to a server without Skills the old Skills pane is still rendered for one frame before the effect switches tabs. Perform this guarded state adjustment during render (or derive the effective tab) so React discards the stale render instead of painting it.

  useEffect(() => {
    if (
      activeTab === "skills" &&
      inspectorStatus === "connected" &&
      !showSkillsTab

clients/tui/src/components/SkillsTab.tsx:532

• Incomplete reports deliberately keep ok: true, so checking ok first labels a truncated verification as “Verified.” Check incomplete before ok; the current test misses this because its oversized payload also creates a size mismatch, making ok false.

                      ? activeReport.ok

• Files reviewed: 51/51 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Round 17 — 5 real, 1 stale, and a single root cause

Fixed in 9117f63; gate green before the push. Replies are on the two inline threads; this mirrors them and covers the four suppressed comments, which have no thread to reply to.

Four of the five share one cause. Round 15 replaced the two-state verdict with a tri-state outcome (verified / failed / incomplete) and kept ok meaning "nothing that was checked is wrong" — which stays true for an incomplete report. I changed the producer and did not follow it into the consumers, so three of them, plus the doc paragraph, went on branching on ok. That is exactly the class the review caught, and it is a fair hit.

#	Where	Fix
1	clients/cli/src/handlers/skills-verify.ts (inline)	summarizeSkillVerification counts off outcome. A truncated run no longer prints "no conformance errors" one line before exiting SKILL_INCOMPLETE, and a mixed failed/incomplete catalog reports both counts instead of letting the louder verdict hide the quieter one.
2	core/mcp/skillsVerification.ts (inline)	The truncation reason is set only when entries were actually left unread — a file crossing the byte budget as the final entry stopped nothing.
3	clients/tui/src/components/SkillsTab.tsx:536 (suppressed)	The status line switches on outcome through a Record over the union, so the INCOMPLETE arm is reachable — and a fourth outcome would be a type error rather than a silently missing label.
4	core/mcp/skillsVerification.ts:104 (suppressed)	The incomplete doc paragraph claimed it makes ok false. It does not, deliberately. Rewritten to say so and to name outcome as the thing to branch on, with the CLI and TUI bugs cited as what happens otherwise.
5	clients/web/.../SkillsScreen.tsx:807 (suppressed)	Sidebar rows compose in the per-URI duplicate-name warning, so two colliding skills are badged in the catalog rather than looking clean until one is selected — which is precisely when a reader needs to be told two rows share a name.
6	core/mcp/skillsVerification.ts:366 (suppressed)	Stale. This review ran against 89438d1a, one commit behind; selfAttempted landed in round 16 and is at lines 308/314/371 of HEAD. No change.

One test was passing for the wrong reason

Worth calling out rather than burying. The existing TUI "INCOMPLETE" test understated every declared size to force truncation — but an understated size is a size mismatch, so the report was failed, and the assertion passed only because the old ok-first branch reached the INCOMPLETE arm for a failed-and-truncated report. Fixing finding 3 correctly turned it red, which is the useful outcome.

It is rebuilt with honest digests and sizes, so truncation comes from the declared-size prefilter and the report is genuinely incomplete — the fixture now exercises the branch its name claims.

I also checked the new sidebar test fails without its fix (expected null not to be null) rather than assuming it did.

[Copilot]

🟡 Changes recommended

The critical byte-budget bypass and three moderate CLI and directory-navigation defects remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/cli/src/handlers/consume-outcome.ts:32

• On a failed verification this writes the human summary and then throws; the binary's top-level handleError catches that CliExitCodeError and writes its JSON error envelope to stderr too. As a result, the documented “one-line summary on stderr” becomes two stderr lines, and consumers expecting a single diagnostic line receive duplicate output. Preserve the nonzero exit status without routing an already-reported outcome through the generic error printer (or mark the exception as already reported and have handleError suppress the second write).

• Files reviewed: 51/51 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Round 18 — 3 real, 1 declined

Fixed in a3f36ce; gate green before the push. Per-comment replies are on the three inline threads; this mirrors them and answers the suppressed one, which has no thread to reply to.

Unlike rounds 15–17, these are not fallout from one shared cause — they are three independent defects in code earlier rounds had already hardened, which is a fair result for the newest parts of the diff.

#	Where	Fix
1	core/mcp/skillsVerification.ts	The 16 MiB budget was charged only after a block matched by URI and decoded. A server could answer every row with an enormous block labelled a URI nobody asked for, or an enormous invalid-base64 blob, bank zero, and have the walk issue up to 512 more. responseBytes() charges the raw response before selection and decoding; the exact decoded length is substituted where there is one.
2	clients/web/.../SkillsScreen.tsx	Containment was checked, the direct-child contract was not. A grandchild — or the directory echoing itself — passed inRoot and became navigable. Non-direct entries are now shown, labelled, and not links.
3	clients/web/.../SkillsScreen.tsx	Containment decided on the normalized URI, navigation stored the raw one. From skill://r/a/../templates, Up produced skill://r/a/.. and a second Up walked into skill://r/a. Descent passes the normalized URI; the Up arithmetic is now parentOfSkillUri, documented as normalized-only.

Declined (suppressed): two stderr lines on a failing --verify

Third appearance, declined again — but with better evidence than the previous two times, because the finding's stated premise is that this contradicts a documented contract, and as of HEAD it does not.

• The precedent is CLI-wide, not skills-specific. emit-result.ts does the same for --strict: writeSchemaLintReport writes the human report to stderr, then a CliExitCodeError is thrown whose message handleError also writes to stderr.
• The docs already say so. clients/cli/README.md — "Two stderr lines on failure, one on success, which is the same shape --strict produces and is why the envelope is not suppressed here: a caller branching on .code should not have to special-case this command."

Suppressing the envelope only for --verify would make skills the one command whose failures a caller branching on .code has to special-case. Changing the convention across the CLI is a reasonable thing to want and a different change from this one; I'd take it as a follow-up issue.

Verification of the fixes, not just their presence

• Both budget tests were mutation-checked — reverting the charge to 0 takes them from 3 reads to 20 and both go red.
• Writing the direct-child test turned up something worth recording: normalizeSkillUri("skill://data-analysis") returns undefined, because a URI with no path fails its pathname.startsWith("/") guard. A bare root echoed back is therefore refused as "outside this skill" rather than "not a direct child" — still refused, but not the label the predicate intends. The test exercises the self-echo one level down, where the URI parses, and the discrepancy is called out rather than left as an accident of the fixture.

Process note: at the maintainer's direction this is the second-to-last review round on this PR. I'll address round 19 and stop there; anything outstanding after it will be filed as follow-up issues against the verification code rather than continued here.

[Copilot]

🟡 Changes recommended

Critical unbounded catalog verification and incorrect UTF-8 byte accounting must be fixed, along with quadratic collision reporting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skills.ts:1029

• For a legal collision group of N skills, every entry filters all N URIs and embeds the other N−1 URIs in its message. That makes both processing and the generated reports O(N²), while SEP-2640 permits arbitrarily large catalogs and duplicate names are explicitly legal. Summarize collisions with a bounded sample/count (or share one group-level representation) so a server-controlled catalog cannot cause quadratic work and output.

clients/cli/src/cli.ts:755

• --verify can also exit 8 when verification is incomplete, but this help text only documents exit 7. CLI users relying on the built-in help cannot distinguish a conformance failure from a bounded/truncated run. Please document both outcomes here, matching clients/cli/README.md and EXIT_CODES.SKILL_INCOMPLETE.

      "Run the SEP-2640 conformance and digest checks over the skills returned, emit one JSON report per skill on stdout, and exit 7 if any fails. Use with --method skills/list or --method skills/get.",

• Files reviewed: 51/51 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Round 19 — 4 findings, all real. Final round.

Fixed in 62fd7d1; gate green before the push. Per-comment replies are on the two inline threads; this mirrors them and answers the two suppressed ones, which have no thread to reply to.

At the maintainer's direction this is the last review round on this PR. Everything below is either fixed here or filed as a follow-up issue — nothing is being left implicit.

#	Where	Fix
1	core/mcp/skillsVerification.ts (inline)	No catalog-level bound. The per-skill caps bound what one entry costs; nothing bounded how many entries there are, and SEP-2640 explicitly permits arbitrarily large catalogs. A single page may hold arbitrarily many entries, so the page guard bounds round trips, not entries. Adds SKILL_MAX_CATALOG_SKILLS (256) / SKILL_MAX_CATALOG_BYTES (64 MiB).
2	core/mcp/skillsVerification.ts (inline)	UTF-16 charged as bytes. My round-18 fix used text.length, undercharging non-ASCII by up to 3×. New utf8Length — allocation-free on purpose, since TextEncoder would copy a payload the server sized.
3	core/mcp/skills.ts (suppressed)	Quadratic collision reporting. For a group of N, every entry filtered all N URIs and embedded the other N−1. Duplicate names are legal and a server controls N. Now a bounded sample of three plus a count.
4	clients/cli/src/cli.ts (suppressed)	--verify help documented exit 7 but not exit 8. Both now, matching the README and EXIT_CODES.

On #2, which is the honest one to call out

This was a defect in a fix from the previous round. I had documented the charge as an approximation "never an undercount by more than a small factor" — true, and not the same as correct. 3× headroom on a 16 MiB cap is a bypass, not a rounding error. The review was right to keep pulling on it.

Entries past the run budget are reported, not dropped

The static checks cost no I/O, so a skill past the bound keeps its conformance findings and gains an incomplete reason saying nothing about its files was checked, pointing at --method skills/get --uri <skill> for a per-skill verdict. incomplete rather than pass or fail, for the reason the tri-state exists: an entry nobody read has not been cleared of anything, and failing it because we stopped would be a verdict we did not earn.

The constants say in their own doc that they are this tool's limits, not SEP-2640's. clients/cli/README.md now carries all three tiers — per skill, per skill on the wire, per run — as a table, so the run bound cannot later be misread as a conformance rule.

Follow-ups filed rather than rushed in

• Make the skills --verify catalog budget configurable #2294 — make the catalog budget configurable. The review offered this as an alternative remedy; a new CLI flag arriving in the final round would ship without a round of review on it, which is the wrong trade.
• normalizeSkillUri rejects a path-less skill URI, mislabelling a directory child #2295 — normalizeSkillUri returns undefined for a path-less skill://name, so a bare root echoed back as a directory child is labelled "outside this skill" instead of "not a direct child". Still refused, wrong label. That function is load-bearing for every URI comparison in core/mcp/skills.ts, so relaxing its guard wants its own review.

Where the 19 rounds landed

Rounds 1–8 found feature defects, several serious (blob-served files skipping mandatory checks, dynamic skills reporting ok: true, cyclic YAML crashing the tool). Rounds 9–17 increasingly found defects in my own fixes. Rounds 18–19 went back to finding real holes in the newest code — the byte-budget bypass and the unbounded catalog were both genuine, and both were introduced by me.