diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx
index e4b44f8c..e8cf89ee 100644
--- a/clients/tui/__tests__/SkillsTab.test.tsx
+++ b/clients/tui/__tests__/SkillsTab.test.tsx
@@ -670,6 +670,88 @@ describe("SkillsTab (#2248)", () => {
expect(lastFrame() ?? "").toContain("skill://acme/reports/SKILL.md");
});
+ it("shows the digests for a mismatch, not just the failed mark", async () => {
+ // `verifySkillResource` sets `reason` for a SIZE mismatch but not a digest
+ // one, so a pane rendering only `reason` left a bare `✗` with no diagnosis
+ // — a failed verification the reader cannot act on (Copilot).
+ // The declared size must be RIGHT, or the cheaper size cross-check
+ // short-circuits before hashing and reports its own `reason` instead —
+ // which is the path that already rendered.
+ const digestOnly: SkillEntry = {
+ ...clean,
+ resources: [
+ {
+ uri: "skill://clean/SKILL.md",
+ digest: CLEAN_DIGEST,
+ size: textToBytes(SKILL_MD).byteLength,
+ },
+ ],
+ };
+ const { lastFrame, stdin } = render(
+ ,
+ );
+ stdin.write(ENTER);
+ await tick();
+ const frame = lastFrame() ?? "";
+ expect(frame).toContain("Verification FAILED");
+ // Truncated to keep the line inside a narrow pane; the CLI report carries
+ // the digests in full.
+ expect(frame).toMatch(/expected sha256:0+…/);
+ expect(frame).toMatch(/got sha256:[0-9a-f]+…/);
+ });
+
+ it("shows the reason for a size mismatch, which carries no digest", async () => {
+ // The other arm: a length disagreement fails before the hash, so there is
+ // no actual digest to print and the reason is the whole diagnosis.
+ const { lastFrame, stdin } = render(
+ ,
+ );
+ stdin.write(ENTER);
+ await tick();
+ expect(lastFrame() ?? "").toContain(
+ "Manifest declares 51 bytes but the fetched file is 52.",
+ );
+ });
+
+ it("shows a read failure the manifest does not cover", async () => {
+ // A dynamic skill has no manifest rows, so the synthetic read-error row
+ // `verifySkills` records for its own SKILL.md was rendered nowhere and the
+ // pane said only "Verification FAILED".
+ const { lastFrame, stdin } = render(
+ ,
+ );
+ stdin.write(ENTER);
+ await tick();
+ const frame = lastFrame() ?? "";
+ expect(frame).toContain("Read failures:");
+ expect(frame).toContain("SKILL.md");
+ expect(frame).toContain("upstream gone");
+ expect(frame).toContain("Verification FAILED");
+ });
+
it("shows the details footer only when the details pane is focused", () => {
const unfocused = render(
(d ? `${d.slice(0, 23)}…` : "—");
+ return `expected ${short(file.expectedDigest)}, got ${short(file.actualDigest)}`;
+}
+
/** The file name a manifest URI ends in, for a list that must fit 40 columns. */
function fileNameOf(uri: string): string {
const cut = uri.lastIndexOf("/");
@@ -254,6 +272,14 @@ export function SkillsTab({
selectedSkill && selectedSkill.resources !== DYNAMIC_RESOURCES
? selectedSkill.resources
: [];
+ // Compared on normalized identity, like every other URI comparison here, so a
+ // manifest entry written in an equivalent form is not reported twice.
+ const manifestIdentities = new Set(
+ manifest.map((resource) => skillUriIdentity(resource.uri)),
+ );
+ const extraReportFiles = (activeReport?.files ?? []).filter(
+ (file) => !manifestIdentities.has(skillUriIdentity(file.uri)),
+ );
return (
@@ -410,15 +436,48 @@ export function SkillsTab({
({resource.size} B)
) : null}
- {fileReport?.reason && (
+ {fileReport && failureDetail(fileReport) && (
- {fileReport.reason}
+ {failureDetail(fileReport)}
)}
);
})}
+ {/* A report can carry a file the MANIFEST does not — a dynamic
+ skill has no rows at all, yet a failed read of its own
+ SKILL.md is recorded so the failure is visible. Rendering only
+ manifest rows left "Verification FAILED" with no diagnosis
+ anywhere on screen (Copilot). */}
+ {extraReportFiles.length > 0 && (
+ <>
+
+ Read failures:
+
+ {extraReportFiles.map((file, idx) => (
+
+
+
+ {FILE_MARK[file.status] ?? "?"}{" "}
+
+ {fileNameOf(file.uri)}
+
+ {failureDetail(file) && (
+
+ {failureDetail(file)}
+
+ )}
+
+ ))}
+
+ )}
+
{activeReport && activeReport.frontmatter.length > 0 && (
<>
diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
index 2431c2aa..39eae21a 100644
--- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
+++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
@@ -380,6 +380,26 @@ const ResourceNameCaption = Text.withProps({
maw: "50%",
});
+/** A read that failed, in the Directory section. */
+const ReadFailureAlert = Alert.withProps({
+ color: "red",
+ variant: "light",
+ title: "Read failed",
+});
+
+/**
+ * The directory-vs-manifest divergence banner. Yellow: the server is not
+ * necessarily wrong — its listing may simply be newer than the held entry.
+ */
+const UnlistedChildrenAlert = Alert.withProps({
+ color: "yellow",
+ variant: "light",
+ title: "This directory lists files the entry does not",
+});
+
+/** The em dash standing in for a verdict that does not apply to a row. */
+const NoVerdictText = Text.withProps({ size: "xs", c: "dimmed" });
+
const IssueStack = Stack.withProps({
gap: "xs",
});
@@ -1984,9 +2004,7 @@ export function SkillsScreen({
{directoryError !== undefined && (
-
- {directoryError}
-
+ {directoryError}
)}
{/* Stated in prose the first time the two views
disagree, because the per-row chip alone does not say
@@ -1994,12 +2012,7 @@ export function SkillsScreen({
re-approval" is what SEP-2640 asks a host to present
here, rather than a read error. */}
{unlistedChildren.length > 0 && (
-
+
The server is serving {unlistedChildren.length} file
{unlistedChildren.length === 1 ? "" : "s"} here that
the held skills/list entry does not
@@ -2009,7 +2022,7 @@ export function SkillsScreen({
verification failure equivalent to a digest mismatch.
Re-fetch the entry with skills/get to see
whether the skill has changed.
-
+
)}
{directoryChildren !== undefined &&
(directoryChildren.length === 0 ? (
@@ -2074,9 +2087,7 @@ export function SkillsScreen({
{child.mimeType ?? "—"}
{isDir || isDynamic ? (
-
- —
-
+ —
) : (
{
expect(parseSkillFrontmatter("# just a comment")).toEqual({ fields: {} });
});
+ it("reports an explicit null scalar as an error, not as no fields", () => {
+ // `null` and `~` parse to the same value an EMPTY block does, but only the
+ // empty one is a degenerate mapping — returning `{ fields: {} }` for an
+ // explicit null scalar contradicts this function's own contract (Copilot).
+ for (const src of ["null", "~", " null ", "# lead\nnull"]) {
+ expect(parseSkillFrontmatter(src)).toEqual({
+ error: expect.stringContaining("mapping"),
+ });
+ }
+ });
+
+ it("still reads a comment-only block as a mapping of no fields", () => {
+ // The distinction is made on the SOURCE, so this must not regress.
+ expect(parseSkillFrontmatter("# just a comment\n\n # another")).toEqual({
+ fields: {},
+ });
+ });
+
+ it("does not mistake a leading # inside a value for a comment", () => {
+ // Comments are stripped only at the start of a line; a `#` inside a value
+ // is part of it, and treating it as a comment would call a block with real
+ // content empty.
+ expect(parseSkillFrontmatter('name: "#hashtag"')).toEqual({
+ fields: { name: "#hashtag" },
+ });
+ });
+
it("reports a scalar block as an error rather than as no fields", () => {
// `just a string` parses successfully as a scalar. Reporting it as an
// empty mapping would present a malformed file as one that merely omitted
diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts
index 551f86b3..67132a11 100644
--- a/clients/web/src/test/core/mcp/skills.test.ts
+++ b/clients/web/src/test/core/mcp/skills.test.ts
@@ -1005,6 +1005,29 @@ describe("checkSkillFrontmatterMatch (#2248)", () => {
expect(both[1].message).toContain("-Infinity");
});
+ it("cannot be fooled by a listing that looks like an encoding", () => {
+ // The regression this guards: encoding non-finite numbers as a sentinel
+ // object let a listing whose value genuinely WAS that object alias it and
+ // match a served `.nan` (Copilot). The comparison is structural now, so
+ // there is no encoding to alias.
+ const issues = checkSkillFrontmatterMatch(
+ entry({ x: { "#non-finite": "NaN" } }),
+ file("x: .nan"),
+ );
+ expect(issues).toHaveLength(1);
+ expect(issues[0].code).toBe("frontmatter-mismatch");
+ });
+
+ it("still matches a listing object that equals the served mapping", () => {
+ // …and the guard must not make a genuine agreement look like a difference.
+ expect(
+ checkSkillFrontmatterMatch(
+ entry({ x: { "#non-finite": "NaN" } }),
+ file('x:\n "#non-finite": NaN'),
+ ),
+ ).toEqual([]);
+ });
+
it("still matches a null the served file also writes as null", () => {
// The fix must not turn a genuine agreement into a finding.
expect(
diff --git a/core/mcp/skillFile.ts b/core/mcp/skillFile.ts
index c49b7fe0..297591b2 100644
--- a/core/mcp/skillFile.ts
+++ b/core/mcp/skillFile.ts
@@ -93,6 +93,21 @@ export type ParsedFrontmatter =
* everything. An *empty* block (`fields: {}`) is a different fact and is
* reported as a successful parse of nothing.
*/
+/**
+ * Whether a frontmatter block holds anything but whitespace and comments.
+ *
+ * Comments are stripped only from the start of a line: a `#` inside a value is
+ * part of that value, and treating it as a comment would call a block with real
+ * content empty. That is the safe direction — mistaking content for emptiness
+ * here would turn a malformed document back into "no fields", which is the bug
+ * this exists to prevent.
+ */
+function hasContent(yamlText: string): boolean {
+ return yamlText
+ .split("\n")
+ .some((line) => line.trim() !== "" && !line.trimStart().startsWith("#"));
+}
+
export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter {
let parsed: unknown;
try {
@@ -100,9 +115,16 @@ export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter {
} catch (err) {
return { error: err instanceof Error ? err.message : String(err) };
}
- // `null` is what an empty (or comment-only) block parses to — a real, if
- // degenerate, mapping of no fields rather than a malformed one.
- if (parsed === null || parsed === undefined) return { fields: {} };
+ // An empty (or comment-only) block parses to `null`, and so does an explicit
+ // `null` / `~` scalar — but only the first is a degenerate mapping of no
+ // fields. The second is a non-mapping document, and returning `{ fields: {} }`
+ // for it contradicts this function's own contract (Copilot). They are told
+ // apart by the SOURCE, since the parsed value cannot distinguish them.
+ if (parsed === null || parsed === undefined) {
+ return hasContent(yamlText)
+ ? { error: "Frontmatter is not a YAML mapping of fields." }
+ : { fields: {} };
+ }
if (typeof parsed !== "object" || Array.isArray(parsed)) {
return {
error: "Frontmatter is not a YAML mapping of fields.",
diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts
index be8d2748..d33847b9 100644
--- a/core/mcp/skills.ts
+++ b/core/mcp/skills.ts
@@ -561,16 +561,6 @@ 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);
@@ -760,6 +750,65 @@ export async function verifySkillResource(
};
}
+/**
+ * Structural equality for JSON-like values, with YAML's extra scalars handled.
+ *
+ * ⚠️ **Comparison is structural rather than serialized, and that is the point.**
+ * `JSON.stringify` is not injective over what a YAML parser produces: `.nan`,
+ * `.inf` and `-.inf` all serialize to `null`, so a served `x: .nan` compared
+ * EQUAL to a listing declaring `x: null` — and to each other. An earlier fix
+ * encoded non-finite numbers as a sentinel object, which merely moved the
+ * problem: a listing whose value genuinely *was* that object aliased the
+ * sentinel and matched a served `.nan` (Copilot). Any encoding into the value
+ * space can be aliased by a document containing the encoding, so there is no
+ * sentinel here at all.
+ *
+ * `Object.is` on the number path is what makes it work: it holds `NaN` equal to
+ * `NaN`, keeps `Infinity` and `-Infinity` distinct, and never equates either
+ * with `null`.
+ */
+function jsonLikeEqual(a: unknown, b: unknown): boolean {
+ if (typeof a === "number" || typeof b === "number") return Object.is(a, b);
+ if (a === null || b === null) return a === b;
+ if (typeof a !== "object" || typeof b !== "object") return Object.is(a, b);
+ const aArray = Array.isArray(a);
+ if (aArray !== Array.isArray(b)) return false;
+ if (aArray) {
+ const x = a as unknown[];
+ const y = b as unknown[];
+ // Array ORDER is significant — a YAML sequence is ordered.
+ return x.length === y.length && x.every((v, i) => jsonLikeEqual(v, y[i]));
+ }
+ const x = a as Record;
+ const y = b as Record;
+ const keys = Object.keys(x);
+ // Key order is not meaningful in either JSON or YAML, so only the key SET and
+ // the values matter.
+ return (
+ keys.length === Object.keys(y).length &&
+ keys.every((k) => Object.hasOwn(y, k) && jsonLikeEqual(x[k], y[k]))
+ );
+}
+
+/**
+ * A frontmatter value as it should READ in a finding.
+ *
+ * `JSON.stringify` renders every non-finite number as `null`, which would print
+ * "the listing says null but the served file says null" for a real difference.
+ * Only the display is special-cased; the comparison above never goes through a
+ * string, so this cannot reintroduce an aliasing bug.
+ */
+function displayValue(value: unknown): string {
+ if (typeof value === "number" && !Number.isFinite(value)) {
+ return String(value);
+ }
+ return JSON.stringify(canonicalize(value), (_key, member: unknown) =>
+ typeof member === "number" && !Number.isFinite(member)
+ ? `<${String(member)}>`
+ : member,
+ );
+}
+
/**
* Compare the fetched `SKILL.md`'s own frontmatter against the frontmatter the
* entry advertised, field by field — the SEP-2640 obligation a digest cannot
@@ -848,13 +897,11 @@ export function checkSkillFrontmatterMatch(
});
continue;
}
- const listedJson = JSON.stringify(canonicalize(listed));
- const servedJson = JSON.stringify(canonicalize(served));
- if (listedJson !== servedJson) {
+ if (!jsonLikeEqual(listed, served)) {
issues.push({
code: "frontmatter-mismatch",
severity: "error",
- message: `Field "${field}" differs: the listing says ${listedJson} but the served SKILL.md says ${servedJson}.`,
+ message: `Field "${field}" differs: the listing says ${displayValue(listed)} but the served SKILL.md says ${displayValue(served)}.`,
resourceUri: entry.uri,
});
}
diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts
index 84e0fdce..6bb2f96d 100644
--- a/core/mcp/skillsVerification.ts
+++ b/core/mcp/skillsVerification.ts
@@ -67,7 +67,16 @@ export interface SkillVerifyReport {
* frontmatter discrepancy.
*/
frontmatter: SkillIssue[];
- /** One entry per manifest file, in manifest order. Empty for `"dynamic"`. */
+ /**
+ * One entry per manifest file, in manifest order.
+ *
+ * ⚠️ **Not necessarily empty for a `"dynamic"` skill.** Such a skill has no
+ * manifest rows, but a failed read of its own `SKILL.md` — the file the
+ * mandatory frontmatter comparison needs — is recorded here as a synthetic
+ * `read-error` row against the entry's URI, so the failure is visible and
+ * fails the report rather than passing silently. A consumer must not assume
+ * `files` mirrors the manifest one-for-one (Copilot).
+ */
files: SkillFileReport[];
/**
* False when anything the SEP makes a MUST was broken: an error-severity