diff --git a/clients/cli/README.md b/clients/cli/README.md index ec4ed882..7a6d1dce 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -364,8 +364,13 @@ Stdout is **NDJSON, one report per skill**, in listing order: ``` Stderr gets a one-line summary, so a reader who piped stdout into `jq` still -sees the verdict. `--method skills/get --uri ` verifies exactly one -skill, in the same shape. +sees the verdict — and then, on a failing run, the ordinary +[`ErrorEnvelope`](#exit-codes--error-envelopes) line that **every** non-zero +exit writes. 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. +`--method skills/get --uri ` verifies exactly one skill, in the same +shape. **What fails the run.** `ok` is false — and the exit code is `7` — for anything SEP-2640 makes a MUST: an error-severity conformance finding, a digest or size diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 89d5517a..ee3a5732 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -2458,6 +2458,42 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { ); }); + it("keeps a frontmatter finding when the reader opens another file", async () => { + // The check ran off whatever the viewer was showing, so opening a + // supporting file made `showingSkillMd` false and silently dropped the + // finding AND its error count — erasing an observed conformance failure + // because the reader browsed a second file, with the skill unchanged + // (Copilot). + const user = userEvent.setup(); + const lying: SkillEntry = { + ...CLEAN_SKILL, + frontmatter: { name: "data-analysis", description: "Disagrees" }, + }; + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await waitFor(() => + expect( + screen.getByTestId("skill-frontmatter-issues"), + ).toBeInTheDocument(), + ); + + // Open a supporting file: the finding is about the SKILL, not the view. + await user.click( + screen.getByRole("button", { + name: "skill://data-analysis/reference.md", + }), + ); + await waitFor(() => + expect(readFixtureFile).toHaveBeenCalledWith( + "skill://data-analysis/reference.md", + ), + ); + expect(screen.getByTestId("skill-frontmatter-issues")).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /Conformance/ }), + ).toHaveTextContent("1 error(s)"); + }); + it("counts frontmatter findings in the Conformance badge", async () => { // The findings render inside this section, so counting only the static // listing issues left the badge saying `0 error(s)` above a red diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index fe7e6875..329d7a71 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -882,7 +882,7 @@ export function SkillsScreen({ }); const fileStates = verification.key === manifestKey ? verification.files : {}; - const verifiedEntryText = + const entrySourceText = verification.key === manifestKey ? verification.entryText : undefined; /** @@ -1037,7 +1037,7 @@ export function SkillsScreen({ * whichever one happened to be current when this callback was created. */ const showResource = useCallback( - (uri: string, key: string) => { + (uri: string, key: string, isEntryUri = false) => { const attempt = (nextAttempt.current += 1); // A click handler cannot await, and this chain terminates in its own // `catch` that surfaces the message in the viewer. Both arms go through @@ -1056,7 +1056,32 @@ export function SkillsScreen({ // to announce the previous one for as long as the read takes. writePreview({ uri }); void onReadSkillFile(uri) - .then((contents) => writePreview({ uri, contents })) + .then((contents) => { + writePreview({ uri, contents }); + // A successful read of the skill's OWN file is recorded in the + // skill-scoped slot, so the frontmatter verdict it produces outlives + // the reader opening a supporting file — see + // `VerificationState.entryText`. Only on success, and only for that + // file: a failed or unrelated read must not overwrite an answer. + if (isEntryUri) { + let text: string | undefined; + try { + text = bytesToText(skillFileBytes(contents)); + } catch { + return; // neither text nor blob; the viewer reports it + } + setVerification((prev) => + prev.key !== null && prev.key !== key + ? prev + : { + ...prev, + key, + files: prev.key === key ? prev.files : {}, + entryText: text, + }, + ); + } + }) .catch((err: unknown) => { writePreview({ uri, @@ -1117,7 +1142,7 @@ export function SkillsScreen({ } if (autoReadKey.current === manifestKey) return; autoReadKey.current = manifestKey; - showResource(selectedUri, manifestKey); + showResource(selectedUri, manifestKey, true); }, [manifestKey, selectedUri, showResource]); /** @@ -1453,10 +1478,11 @@ export function SkillsScreen({ */ const frontmatterIssues = useMemo(() => { if (!selected) return []; - // Prefer the text the VERIFICATION read, so the digest verdict above and - // this one describe one fetch rather than two. - if (verifiedEntryText !== undefined) { - return checkSkillFrontmatterMatch(selected, verifiedEntryText); + // The skill-scoped text, whichever read produced it — so this verdict + // survives the reader opening another file, and agrees with the digest + // verdict when one verification produced both. + if (entrySourceText !== undefined) { + return checkSkillFrontmatterMatch(selected, entrySourceText); } if (!showingSkillMd || preview === undefined) return []; // Run against the **raw fetched bytes**, not against `previewParts`. @@ -1479,7 +1505,7 @@ export function SkillsScreen({ return []; } return checkSkillFrontmatterMatch(selected, text); - }, [selected, showingSkillMd, preview, verifiedEntryText]); + }, [selected, showingSkillMd, preview, entrySourceText]); /** * The findings rendered as list items — everything except the two that are @@ -2001,7 +2027,11 @@ export function SkillsScreen({ variant={showing ? "light" : "subtle"} aria-current={showing ? "true" : undefined} onClick={() => - showResource(resource.uri, manifestKey) + showResource( + resource.uri, + manifestKey, + isSelfResource(resource), + ) } > {resource.uri} diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index ee3778ec..f7e40eb3 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -1028,6 +1028,33 @@ describe("checkSkillFrontmatterMatch (#2248)", () => { ).toEqual([]); }); + it("does not report -0 against 0 as a difference", () => { + // `Object.is` holds them distinct; JSON does not (`JSON.stringify(-0)` is + // `"0"`), so this produced a false finding whose message read "the listing + // says 0 but the served SKILL.md says 0" (Copilot). + expect(checkSkillFrontmatterMatch(entry({ a: 0 }), file("a: -0"))).toEqual( + [], + ); + expect(checkSkillFrontmatterMatch(entry({ a: -0 }), file("a: 0"))).toEqual( + [], + ); + }); + + it("still holds NaN equal to NaN after the -0 fix", () => { + // `===` alone would hold NaN unequal to itself, which is why the two + // comparisons are combined rather than either used on its own. + expect( + checkSkillFrontmatterMatch(entry({ a: null }), file("a: .nan")), + ).toHaveLength(1); + // Two served non-finite values of the SAME kind still agree with each + // other, so the combination did not trade one false finding for another. + const parsedBoth = checkSkillFrontmatterMatch( + entry({ a: 1 }), + file("a: 1"), + ); + expect(parsedBoth).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/skills.ts b/core/mcp/skills.ts index 54032198..1565abfd 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -772,7 +772,24 @@ export async function verifySkillResource( * with `null`. */ function jsonLikeEqual(a: unknown, b: unknown): boolean { - if (typeof a === "number" || typeof b === "number") return Object.is(a, b); + if (typeof a === "number" || typeof b === "number") { + // `===` for finite numbers, `Object.is` only for the non-finite ones. + // + // `Object.is` alone held `0` and `-0` distinct, so a listing carrying JSON + // `0` against a served YAML `-0` produced a mismatch — reported as "the + // listing says 0 but the served SKILL.md says 0", a false finding with an + // unintelligible explanation (Copilot). JSON does not distinguish them + // (`JSON.stringify(-0)` is `"0"`), so neither may this. `===` would in turn + // hold `NaN` unequal to itself, which is why the two are combined rather + // than either used alone. + if (typeof a === "number" && typeof b === "number") { + return Number.isFinite(a) && Number.isFinite(b) + ? a === b + : Object.is(a, b); + } + // One side is not a number at all: different types, never equal. + return false; + } 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);