fix: address Copilot review round 2 on #2248
Four findings, two of them silent holes in the verification itself. **A blob-served SKILL.md skipped the frontmatter check entirely.** The entry's own file was captured as `contents.text`, so a server returning the markdown as a base64 `blob` — a legal `resources/read` shape this module already decodes for the digest — never reached the comparison, and the report still said `ok`. A MUST that quietly did not run. The file is now held as bytes and the text derived from them, which also guarantees the digest and the frontmatter describe one snapshot. **A dynamic skill whose SKILL.md could not be read reported `ok: true`.** It has no manifest rows, so `files` stayed empty, and its only static finding is a warning — a verification that could not be performed was reported as one that passed. The fallback read now records a `read-error` for all three ways it can fail. It is gated on `manifestListsSelf` (by normalized identity) so a self-entry the manifest listed and failed to read is not read, or reported, twice. **A served `.nan` compared equal to a listed `null`.** YAML expresses non-finite numbers, JSON does not, and `JSON.stringify` turns all of them into `null` — so the canonical comparison reported a real mismatch as agreement. Verified against the parser before fixing. They now canonicalize to a form no JSON scalar can equal, and the value is named in the finding. Also: the self-entry match is by normalized identity rather than raw string, which stops a second read and stops this function disagreeing with `checkSkillConformance`; and `SkillsTab.tsx` gains the file header AGENTS.md requires. The four `err instanceof Error` ternaries are one `reasonOf` helper now — extracted because the coverage gate found every one of their non-Error arms uncovered, and one honestly-tested branch beats four ignores. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
4fa00d1cfd
commit
033600e4ca
5 changed files with 288 additions and 13 deletions
|
|
@ -1,3 +1,23 @@
|
|||
/**
|
||||
* The TUI's Skills pane — the SEP-2640 catalog in a terminal (#2248).
|
||||
*
|
||||
* **Why the TUI owns a pane rather than reusing the web screen's logic.** It
|
||||
* does reuse everything that decides an answer: `checkSkillConformance`,
|
||||
* `checkSkillNameCollisions` and `verifySkills` all live in `core/` and are
|
||||
* driven identically here, so a verdict cannot differ depending on which client
|
||||
* you asked. What is local is presentation, and the terminal's constraints are
|
||||
* genuinely different — two panes in 80 columns, no colour to rely on, and a
|
||||
* keyboard rather than a pointer.
|
||||
*
|
||||
* ⚠️ **Severity is carried by a glyph as well as a colour** (`✓` / `!` / `✗`).
|
||||
* This pane is read over ssh, inside tmux, and piped through `script(1)`, where
|
||||
* colour may not survive; a row whose only signal was `red` would then be
|
||||
* indistinguishable from a clean one.
|
||||
*
|
||||
* The pane is shown only when the connected server declares the extension —
|
||||
* that gate, and the reset that leaves the tab when it goes false, live in
|
||||
* `App.tsx` because they are navigation concerns rather than this pane's.
|
||||
*/
|
||||
import React, { useCallback, useEffect, useRef, useState } from "react";
|
||||
import { Box, Text, useInput, type Key } from "ink";
|
||||
import { ScrollView, type ScrollViewRef } from "ink-scroll-view";
|
||||
|
|
|
|||
|
|
@ -975,6 +975,43 @@ describe("checkSkillFrontmatterMatch (#2248)", () => {
|
|||
).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("does not let a YAML non-finite number match a listing's null", () => {
|
||||
// `.nan` / `.inf` are YAML values JSON cannot express, and
|
||||
// `JSON.stringify` turns every one of them into `null` — so a naive
|
||||
// canonical comparison reported a served `.nan` as EQUAL to a listed
|
||||
// `null`: a mismatch silently presented as agreement (Copilot).
|
||||
const issues = checkSkillFrontmatterMatch(
|
||||
entry({ threshold: null }),
|
||||
file("threshold: .nan"),
|
||||
);
|
||||
expect(issues).toHaveLength(1);
|
||||
expect(issues[0].code).toBe("frontmatter-mismatch");
|
||||
// The value is named in the finding rather than hidden behind `null`.
|
||||
expect(issues[0].message).toContain("NaN");
|
||||
});
|
||||
|
||||
it("distinguishes the three non-finite values from one another", () => {
|
||||
expect(
|
||||
checkSkillFrontmatterMatch(entry({ x: null }), file("x: .inf")),
|
||||
).toHaveLength(1);
|
||||
// Infinity vs -Infinity: both stringify to `null`, so they would have
|
||||
// compared equal to each other as well.
|
||||
const both = checkSkillFrontmatterMatch(
|
||||
entry({ a: 1, b: 2 }),
|
||||
file("a: .inf\nb: -.inf"),
|
||||
);
|
||||
expect(both).toHaveLength(2);
|
||||
expect(both[0].message).toContain("Infinity");
|
||||
expect(both[1].message).toContain("-Infinity");
|
||||
});
|
||||
|
||||
it("still matches a null the served file also writes as null", () => {
|
||||
// The fix must not turn a genuine agreement into a finding.
|
||||
expect(
|
||||
checkSkillFrontmatterMatch(entry({ x: null }), file("x: null")),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("reports nothing for two empty frontmatters", () => {
|
||||
expect(checkSkillFrontmatterMatch(entry({}), file(""))).toEqual([]);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -282,6 +282,150 @@ describe("verifySkills (#2248)", () => {
|
|||
reason: "expired",
|
||||
} as never);
|
||||
|
||||
it("runs the frontmatter check when the SKILL.md arrives as a blob", async () => {
|
||||
// A base64 `blob` is a legal `resources/read` shape, and this module
|
||||
// already decodes it for the digest. Reading `contents.text` skipped the
|
||||
// MANDATORY frontmatter comparison for such a server while still reporting
|
||||
// `ok` (Copilot).
|
||||
const skillMd = "---\nname: demo\ndescription: Served\n---\n\n# D\n";
|
||||
const bytes = new TextEncoder().encode(skillMd);
|
||||
const skill: SkillEntry = {
|
||||
uri: "skill://demo/SKILL.md",
|
||||
frontmatter: { name: "demo", description: "Listed" },
|
||||
resources: [
|
||||
{
|
||||
uri: "skill://demo/SKILL.md",
|
||||
digest: await sha256Digest(bytes),
|
||||
size: bytes.byteLength,
|
||||
},
|
||||
],
|
||||
};
|
||||
const readResource = vi.fn(async () => ({
|
||||
result: {
|
||||
contents: [
|
||||
{
|
||||
uri: "skill://demo/SKILL.md",
|
||||
blob: Buffer.from(skillMd, "utf8").toString("base64"),
|
||||
},
|
||||
],
|
||||
},
|
||||
}));
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
expect(report.files[0].status).toBe("verified");
|
||||
expect(report.frontmatter).toHaveLength(1);
|
||||
expect(report.ok).toBe(false);
|
||||
});
|
||||
|
||||
it("does not re-read a self-entry written in an equivalent URI form", async () => {
|
||||
// `checkSkillConformance` accepts a normalized-equivalent self-entry, so a
|
||||
// raw string comparison here would disagree with it and read the file twice.
|
||||
const skillMd = "---\nname: demo\ndescription: A demo\n---\n\n# D\n";
|
||||
const bytes = new TextEncoder().encode(skillMd);
|
||||
const skill: SkillEntry = {
|
||||
uri: "skill://demo/SKILL.md",
|
||||
frontmatter: { name: "demo", description: "A demo" },
|
||||
resources: [
|
||||
{
|
||||
uri: "skill://demo/x/../SKILL.md",
|
||||
digest: await sha256Digest(bytes),
|
||||
size: bytes.byteLength,
|
||||
},
|
||||
],
|
||||
};
|
||||
const readResource = vi.fn(async (uri: string) => ({
|
||||
result: { contents: [{ uri, text: skillMd }] },
|
||||
}));
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
expect(readResource).toHaveBeenCalledTimes(1);
|
||||
expect(report.frontmatter).toEqual([]);
|
||||
expect(report.ok).toBe(true);
|
||||
});
|
||||
|
||||
it("fails a dynamic skill whose SKILL.md cannot be read", async () => {
|
||||
// A dynamic skill has no manifest rows, so `files` stayed empty and its
|
||||
// only static finding is a warning — an unreadable SKILL.md therefore
|
||||
// reported `ok: true` for a skill whose mandatory frontmatter check never
|
||||
// ran (Copilot).
|
||||
const skill: SkillEntry = {
|
||||
uri: "skill://gen/SKILL.md",
|
||||
frontmatter: { name: "gen", description: "Generated" },
|
||||
resources: "dynamic",
|
||||
};
|
||||
const readResource = vi.fn(async () => {
|
||||
throw new Error("gone");
|
||||
});
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
expect(report.files).toEqual([
|
||||
expect.objectContaining({
|
||||
uri: "skill://gen/SKILL.md",
|
||||
status: "read-error",
|
||||
reason: "gone",
|
||||
}),
|
||||
]);
|
||||
expect(report.ok).toBe(false);
|
||||
});
|
||||
|
||||
it("fails a dynamic skill whose SKILL.md answers with no matching block", async () => {
|
||||
const skill: SkillEntry = {
|
||||
uri: "skill://gen/SKILL.md",
|
||||
frontmatter: { name: "gen", description: "Generated" },
|
||||
resources: "dynamic",
|
||||
};
|
||||
const readResource = vi.fn(async () => ({
|
||||
result: { contents: [{ uri: "skill://gen/other.md", text: "x" }] },
|
||||
}));
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
expect(report.files[0].status).toBe("read-error");
|
||||
expect(report.ok).toBe(false);
|
||||
});
|
||||
|
||||
it("does not read a failed manifest self-entry a second time", async () => {
|
||||
// Its failure is already recorded by the manifest loop; the fallback exists
|
||||
// for a skill whose manifest never listed the file at all.
|
||||
const skill = await entry();
|
||||
const readResource = vi.fn(async () => {
|
||||
throw new Error("boom");
|
||||
});
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
// Two manifest entries, two reads — no third.
|
||||
expect(readResource).toHaveBeenCalledTimes(2);
|
||||
expect(report.files).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("stringifies a non-Error rejection rather than reading .message off it", async () => {
|
||||
// A `throw "string"` anywhere in a transport reaches here; reading
|
||||
// `.message` off one would put `undefined` where the diagnosis belongs.
|
||||
const skill = await entry();
|
||||
const readResource = vi.fn(() => {
|
||||
// A non-Error rejection is the point of the test.
|
||||
throw "plainstring";
|
||||
});
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
expect(report.files[0]).toMatchObject({
|
||||
status: "read-error",
|
||||
reason: "plainstring",
|
||||
});
|
||||
});
|
||||
|
||||
it("treats a result whose contents is not an array as no content", async () => {
|
||||
// A server can return anything; `contents: "nope"` is not a block list, and
|
||||
// hashing nothing against a digest would be a confident wrong answer.
|
||||
const skill = await entry();
|
||||
const readResource = vi.fn(async () => ({
|
||||
result: { contents: "nope" },
|
||||
}));
|
||||
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.ok).toBe(false);
|
||||
});
|
||||
|
||||
it("re-throws an auth-recovery error instead of recording it per file", async () => {
|
||||
// Not a property of the file in flight: the session's authorization
|
||||
// expired, so every remaining read fails the same way. Absorbing it would
|
||||
|
|
|
|||
|
|
@ -561,6 +561,16 @@ function canonicalEntry(entry: SkillEntry): string {
|
|||
|
||||
/** Object keys sorted recursively; array ORDER is preserved throughout. */
|
||||
function canonicalize(value: unknown): unknown {
|
||||
// YAML can express `.nan` and `.inf`; JSON cannot. `JSON.stringify` turns
|
||||
// every one of them into `null`, so without this a served `x: .nan` would
|
||||
// compare EQUAL to a listing declaring `x: null` — a mismatch silently
|
||||
// reported as agreement (Copilot). The listing side arrived over JSON-RPC and
|
||||
// can never hold a non-finite number, so one appearing here is always a real
|
||||
// difference. Rendered as an object, which cannot equal any JSON scalar, and
|
||||
// which names the value in the finding rather than hiding it.
|
||||
if (typeof value === "number" && !Number.isFinite(value)) {
|
||||
return { "#non-finite": String(value) };
|
||||
}
|
||||
if (Array.isArray(value)) return value.map(canonicalize);
|
||||
if (value === null || typeof value !== "object") return value;
|
||||
return sortKeys(value as Record<string, unknown>);
|
||||
|
|
@ -639,6 +649,20 @@ export function textToBytes(text: string): Uint8Array {
|
|||
return new TextEncoder().encode(text);
|
||||
}
|
||||
|
||||
/**
|
||||
* A skill file's bytes as UTF-8 text — the inverse of {@link textToBytes}.
|
||||
*
|
||||
* Deliberately **non-fatal**: a `SKILL.md` that is not valid UTF-8 decodes with
|
||||
* replacement characters rather than throwing. That is the more useful failure,
|
||||
* because the frontmatter comparison then reports a concrete difference between
|
||||
* what the listing claimed and what the file actually holds, instead of
|
||||
* collapsing into "could not decode" and skipping the check the SEP makes
|
||||
* mandatory.
|
||||
*/
|
||||
export function bytesToText(bytes: Uint8Array): string {
|
||||
return new TextDecoder().decode(bytes);
|
||||
}
|
||||
|
||||
/**
|
||||
* Raw bytes of a `resources/read` blob content block (standard base64).
|
||||
* Uses `atob`, which Node ≥22 and every browser provide, so this stays
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ import { AuthRecoveryRequiredError } from "../auth/challenge.js";
|
|||
import type { InspectorClientProtocol } from "./inspectorClientProtocol.js";
|
||||
import type { RequestMetadata } from "./types.js";
|
||||
import {
|
||||
bytesToText,
|
||||
checkSkillConformance,
|
||||
checkSkillFrontmatterMatch,
|
||||
checkSkillNameCollisions,
|
||||
|
|
@ -79,6 +80,18 @@ export interface SkillVerifyReport {
|
|||
ok: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* The reason string for a failed read.
|
||||
*
|
||||
* One helper rather than the same ternary at each of the four call sites: a
|
||||
* rejection is not required to be an `Error` — a `throw "string"` anywhere in a
|
||||
* transport or its dependencies reaches here — and reading `.message` off one
|
||||
* would put `undefined` where the diagnosis belongs.
|
||||
*/
|
||||
function reasonOf(err: unknown): string {
|
||||
return err instanceof Error ? err.message : String(err);
|
||||
}
|
||||
|
||||
/** Result shape of one `resources/read`, narrowed to what a digest needs. */
|
||||
interface ReadContents {
|
||||
text?: string;
|
||||
|
|
@ -154,11 +167,27 @@ export async function verifySkills(
|
|||
// for the frontmatter cross-check. Reading it twice would double the load
|
||||
// on the server and, worse, could compare a digest against one snapshot
|
||||
// and frontmatter against another.
|
||||
let entryText: string | undefined;
|
||||
//
|
||||
// Held as BYTES, not text. Taking `contents.text` skipped the whole
|
||||
// frontmatter comparison whenever a server returned the markdown as a
|
||||
// base64 `blob` — which is a legal `resources/read` shape, and which this
|
||||
// module already decodes for the digest — so a mandatory check silently did
|
||||
// not run while the report still said `ok` (Copilot). Deriving the text from
|
||||
// the same verified bytes also guarantees the digest and the frontmatter
|
||||
// describe one snapshot.
|
||||
let entryBytes: Uint8Array | undefined;
|
||||
const files: SkillFileReport[] = [];
|
||||
|
||||
const manifest =
|
||||
entry.resources === DYNAMIC_RESOURCES ? [] : entry.resources;
|
||||
const entryIdentity = skillUriIdentity(entry.uri);
|
||||
// Compared by NORMALIZED identity, like every other URI comparison here —
|
||||
// `checkSkillConformance` already accepts a manifest self-entry written in
|
||||
// an equivalent form, so a raw string test would disagree with it and read
|
||||
// the same file a second time.
|
||||
const manifestListsSelf = manifest.some(
|
||||
(resource) => skillUriIdentity(resource.uri) === entryIdentity,
|
||||
);
|
||||
for (const resource of manifest) {
|
||||
let contents: ReadContents | undefined;
|
||||
try {
|
||||
|
|
@ -169,7 +198,7 @@ export async function verifySkills(
|
|||
files.push({
|
||||
uri: resource.uri,
|
||||
status: "read-error",
|
||||
reason: err instanceof Error ? err.message : String(err),
|
||||
reason: reasonOf(err),
|
||||
});
|
||||
continue;
|
||||
}
|
||||
|
|
@ -182,9 +211,6 @@ export async function verifySkills(
|
|||
});
|
||||
continue;
|
||||
}
|
||||
if (resource.uri === entry.uri && typeof contents.text === "string") {
|
||||
entryText = contents.text;
|
||||
}
|
||||
let bytes: Uint8Array;
|
||||
try {
|
||||
bytes = skillFileBytes(contents);
|
||||
|
|
@ -192,10 +218,11 @@ export async function verifySkills(
|
|||
files.push({
|
||||
uri: resource.uri,
|
||||
status: "read-error",
|
||||
reason: err instanceof Error ? err.message : String(err),
|
||||
reason: reasonOf(err),
|
||||
});
|
||||
continue;
|
||||
}
|
||||
if (skillUriIdentity(resource.uri) === entryIdentity) entryBytes = bytes;
|
||||
const verification = await verifySkillResource(resource, bytes);
|
||||
files.push({ uri: resource.uri, ...verification });
|
||||
}
|
||||
|
|
@ -203,21 +230,44 @@ export async function verifySkills(
|
|||
// A `"dynamic"` skill has no manifest, so the loop above read nothing —
|
||||
// but its SKILL.md is still served and still has to match the frontmatter
|
||||
// the listing advertised. That obligation is not waived by the file set
|
||||
// being unenumerable; only integrity is.
|
||||
if (entryText === undefined) {
|
||||
// being unenumerable; only integrity is. The same applies to a skill whose
|
||||
// manifest omits its own file.
|
||||
//
|
||||
// Gated on `manifestListsSelf` rather than on `entryBytes`, so a self-entry
|
||||
// the loop already tried and FAILED to read is not read a second time — its
|
||||
// failure is recorded there.
|
||||
if (!manifestListsSelf) {
|
||||
// Recorded as a file result, not swallowed. Because a dynamic skill has
|
||||
// no manifest rows, `files` would otherwise stay empty and its only static
|
||||
// finding is a warning — so an unreadable SKILL.md returned `ok: true`
|
||||
// for a skill whose mandatory frontmatter check never ran (Copilot).
|
||||
const fail = (reason: string) =>
|
||||
files.push({ uri: entry.uri, status: "read-error", reason });
|
||||
try {
|
||||
const invocation = await client.readResource(entry.uri, metadata);
|
||||
const contents = contentsFor(invocation.result, entry.uri);
|
||||
if (typeof contents?.text === "string") entryText = contents.text;
|
||||
if (!contents) {
|
||||
fail(
|
||||
"resources/read returned no content block for this skill's own SKILL.md, so its frontmatter cannot be checked against the listing.",
|
||||
);
|
||||
} else {
|
||||
try {
|
||||
entryBytes = skillFileBytes(contents);
|
||||
} catch (err) {
|
||||
fail(reasonOf(err));
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
// Left undefined: the frontmatter check is skipped below. When the
|
||||
// manifest listed this file the failure is already reported there, and
|
||||
// when it did not, `manifest-missing-self` is the finding that matters.
|
||||
// An expired authorization is not that case — see the note above.
|
||||
// An expired authorization is the one error that is not this file's
|
||||
// problem — see the note on the function.
|
||||
if (err instanceof AuthRecoveryRequiredError) throw err;
|
||||
fail(reasonOf(err));
|
||||
}
|
||||
}
|
||||
|
||||
const entryText =
|
||||
entryBytes === undefined ? undefined : bytesToText(entryBytes);
|
||||
|
||||
const collision = collisions.get(skillUriIdentity(entry.uri));
|
||||
const conformance = [
|
||||
...checkSkillConformance(entry),
|
||||
|
|
|
|||
Loading…
Reference in a new issue