fix: address Copilot review round 1 on #2248

Five findings, all real.

**Critical — `verifySkills` selected `contents[0]`.** These bytes are hashed
against the file's advertised digest, so accepting a block the server
labelled something else verifies one file's content against another file's
digest, and can report that as `verified`. A false pass is worse than a
missing check. `contentsFor(result, uri)` now selects by NORMALIZED identity,
which keeps the canonicalized-echo case that motivated the original code
while rejecting an unrelated block, and reports a read error when nothing
answers for the URI. This is what `onReadSkillFile` already did; the two
paths hash bytes against digests and must not disagree about which bytes.

**`skills/get` was dispatched without the extension gate** that `skills/list`
had. Hoisted into `assertSkillsSupported` so the two cannot drift — declaring
the extension commits a server to both. Without it, an undeclared server's
-32601 is indistinguishable to a script from the -32602 a declared server
returns for a URI it does not serve.

**The TUI could strand a user on a hidden Skills tab.** The tab left the bar
when the gate went false but `activeTab` did not, so the pane kept rendering
for a server that never declared the extension. Reset to `info`, following
the Auth precedent — additionally gated on `connected`, because this gate
reads a server declaration and would otherwise fire during a reconnect.

**The TUI keyed a verdict by URI**, so a refresh replacing the manifest under
the same URI left hashes computed for the previous snapshot describing the
new one. Keyed on the entry now, as the web screen already was.

**A failed "Load more" discarded the pages on screen and the retry cursor.**
Both are preserved now, and the success and failure paths share one
staleness-guarded `commit` helper so they cannot drift on which results they
may write — which is how they came to disagree.

Two nits: the six-skill counts. The docs sentence was more wrong than
reported (five awkward skills of six, three outright violations) and is
rewritten rather than renumbered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-07 18:43:58 -04:00
commit 775107776d
2 changed files with 452 additions and 56 deletions

View file

@ -137,13 +137,87 @@ describe("verifySkills (#2248)", () => {
expect(report.ok).toBe(false);
});
it("refuses a block for a DIFFERENT uri rather than verifying it", async () => {
// The dangerous shape, and the reason positional selection is wrong: these
// bytes are about to be hashed against THIS file's advertised digest, so
// accepting a block the server labelled something else verifies one file's
// content against another file's digest — and can report that as
// `verified`. A false pass is worse than a missing check.
const skill = await entry();
const readResource = vi.fn(async () => ({
result: {
contents: [{ uri: "skill://demo/unrelated.md", text: "other bytes" }],
},
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
expect(report.files.every((f) => f.status === "read-error")).toBe(true);
expect(report.files[0].reason).toMatch(/no content block for this URI/);
expect(report.ok).toBe(false);
});
it("finds the matching block when it is not the first one", async () => {
// A server may answer with more than one block, in any order; taking
// `contents[0]` would hash the wrong file's bytes.
const bytes = new TextEncoder().encode(REF);
const skill: SkillEntry = {
uri: "skill://demo/SKILL.md",
frontmatter: { name: "demo", description: "A demo" },
resources: [
{
uri: "skill://demo/ref.md",
digest: await sha256Digest(bytes),
size: bytes.byteLength,
},
],
};
const readResource = vi.fn(async () => ({
result: {
contents: [
{ uri: "skill://demo/decoy.md", text: "decoy" },
{ uri: "skill://demo/ref.md", text: REF },
],
},
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
expect(report.files[0].status).toBe("verified");
});
it("ignores a malformed block while still finding the real one", async () => {
const bytes = new TextEncoder().encode(REF);
const skill: SkillEntry = {
uri: "skill://demo/SKILL.md",
frontmatter: { name: "demo", description: "A demo" },
resources: [
{
uri: "skill://demo/ref.md",
digest: await sha256Digest(bytes),
size: bytes.byteLength,
},
],
};
const readResource = vi.fn(async () => ({
result: {
contents: [
null,
{ uri: 42 },
{ uri: "skill://demo/ref.md", text: REF },
],
},
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
expect(report.files[0].status).toBe("verified");
});
it("reports a response with no content blocks as a read failure", async () => {
const skill = await entry();
const readResource = vi.fn(async () => ({ result: { contents: [] } }));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
expect(report.files[0]).toMatchObject({ status: "read-error" });
expect(report.files[0].reason).toMatch(/no content blocks/);
expect(report.files[0].reason).toMatch(/no content block for this URI/);
});
it("reports a block carrying neither text nor blob as a read failure", async () => {

View file

@ -121,8 +121,8 @@ describe("Skills extension over a real transport (#2234)", () => {
// below — so a modern page missing the envelope surfaces here as a
// rejection rather than as a missing property.
//
// The fixture pages at two over five skills, so a client that stops
// here sees less than half.
// The fixture pages at two over six skills, so a client that stops
// here sees a third of the catalog.
expect(first.skills).toHaveLength(2);
expect(first.nextCursor).toBeDefined();