From d6b1e36e7e237a2a573f3f9a56a168358b47b6b9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 20:44:51 -0400 Subject: [PATCH] fix: address Copilot review round 7 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings. **The CLI dropped what it promised to print.** `getSkill` transforms the `skills/get` envelope to a `SkillEntry`, so every other member the loose schema accepted was gone before the CLI saw it — including the `ttlMs` / `cacheScope` SEP-2640 explicitly leaves open. The code contradicted a comment two lines above it saying a CLI whose contract is "print the result" must not reshape one. `getSkillResult` returns the envelope whole; `getSkill` is a one-line unwrap over it, which matches the callers: the UIs want the entry, the CLI wants the result. **The frontmatter verdict and the digest came from different reads.** The preview fetch and the Verify fetch are separate `resources/read` calls, so a resource changing between them could pair a verified digest with a frontmatter verdict for other bytes. `VerificationState.entryText` now holds the SKILL.md as the verification read it, written in the SAME state update as the verdict — a separate write was clobbered, because `write` returns a fresh object and drops anything not named in it. The preview stays the fallback so a reader who has not clicked Verify still gets the check. **The Conformance badge excluded the frontmatter findings** it renders, so it said `0 error(s)` above a red mismatch. Digest and size mismatches stay in their own badge: "the listing is wrong" and "the bytes are wrong" are different answers. **A repeated skill URI collided two React keys** in the TUI list, in a pane whose job is to show both entries. Index-keyed like the manifest rows. The web fixture also gains its collision pair in the served-file lookup — without it they were handed another skill's SKILL.md, which the badge fix correctly began reporting as a frontmatter error. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- .../cli/__tests__/run-method-skills.test.ts | 34 ++++++- clients/cli/src/handlers/run-method.ts | 16 ++-- clients/tui/__tests__/SkillsTab.test.tsx | 23 +++++ clients/tui/src/components/SkillsTab.tsx | 11 ++- .../SkillsScreen/SkillsScreen.test.tsx | 66 ++++++++++++- .../screens/SkillsScreen/SkillsScreen.tsx | 95 ++++++++++++++++--- core/mcp/inspectorClient.ts | 27 +++++- core/mcp/skillsSchemas.ts | 16 +++- 8 files changed, 257 insertions(+), 31 deletions(-) diff --git a/clients/cli/__tests__/run-method-skills.test.ts b/clients/cli/__tests__/run-method-skills.test.ts index dc4da07a..7074e4a0 100644 --- a/clients/cli/__tests__/run-method-skills.test.ts +++ b/clients/cli/__tests__/run-method-skills.test.ts @@ -41,6 +41,7 @@ function mockClient(overrides: Record = {}): InspectorClient { getSkillsExtension: vi.fn().mockReturnValue({ directoryRead: true }), listSkills: vi.fn().mockResolvedValue({ skills: [] }), getSkill: vi.fn(), + getSkillResult: vi.fn(), readResourceDirectory: vi.fn(), readResource: vi.fn().mockResolvedValue({ result: { contents: [{ uri: "skill://demo/SKILL.md", text: SKILL_MD }] }, @@ -85,19 +86,21 @@ describe("runMethod skills dispatch (#2248)", () => { // -32602 a declared server returns for a URI it does not serve. const client = mockClient({ getSkillsExtension: vi.fn().mockReturnValue(undefined), - getSkill: vi.fn(), + getSkillResult: vi.fn(), }); await expect( runMethod(client, { method: "skills/get", uri: "skill://x/SKILL.md" }), ).rejects.toMatchObject({ exitCode: EXIT_CODES.USAGE }); - expect(client.getSkill).not.toHaveBeenCalled(); + expect(client.getSkillResult).not.toHaveBeenCalled(); }); it("keeps the { skill } envelope on skills/get", async () => { // The client unwraps it for callers that want the entry; a CLI whose // contract is "print the result" must not quietly reshape the wire form. const entry = await cleanEntry(); - const client = mockClient({ getSkill: vi.fn().mockResolvedValue(entry) }); + const client = mockClient({ + getSkillResult: vi.fn().mockResolvedValue({ skill: entry }), + }); const outcome = await runMethod(client, { method: "skills/get", uri: entry.uri, @@ -105,6 +108,27 @@ describe("runMethod skills dispatch (#2248)", () => { expect(outcome).toMatchObject({ result: { skill: entry } }); }); + it("prints the whole skills/get envelope, not just the entry", async () => { + // SEP-2640 leaves it open whether this result carries `ttlMs`/`cacheScope`, + // so a server may send them — and unwrapping to the entry discarded exactly + // those, from a path whose contract is "print the result" (Copilot). + const entry = await cleanEntry(); + const envelope = { + skill: entry, + resultType: "complete", + ttlMs: 60, + cacheScope: "public", + }; + const client = mockClient({ + getSkillResult: vi.fn().mockResolvedValue(envelope), + }); + const outcome = await runMethod(client, { + method: "skills/get", + uri: entry.uri, + }); + expect(outcome).toMatchObject({ result: envelope }); + }); + it("requires --uri for skills/get", async () => { await expect( runMethod(mockClient(), { method: "skills/get" }), @@ -179,7 +203,9 @@ describe("runMethod skills dispatch (#2248)", () => { it("--verify works on a single skills/get", async () => { const entry = await cleanEntry(); - const client = mockClient({ getSkill: vi.fn().mockResolvedValue(entry) }); + const client = mockClient({ + getSkillResult: vi.fn().mockResolvedValue({ skill: entry }), + }); const outcome = await runMethod(client, { method: "skills/get", uri: entry.uri, diff --git a/clients/cli/src/handlers/run-method.ts b/clients/cli/src/handlers/run-method.ts index c233f9bd..67d4ae01 100644 --- a/clients/cli/src/handlers/run-method.ts +++ b/clients/cli/src/handlers/run-method.ts @@ -352,7 +352,15 @@ export async function runMethod( // not serve — "this server has no Skills support" and "no such skill" // are different answers (Copilot). assertSkillsSupported(inspectorClient, args.method); - const skill = await inspectorClient.getSkill(args.uri, args.metadata); + // The ENVELOPE, not the unwrapped entry. `getSkill` discards every other + // member the result carried — including the `ttlMs` / `cacheScope` that + // SEP-2640 explicitly leaves open — and a CLI whose contract is "print + // the result" must not drop what the server actually sent (Copilot). + const envelope = await inspectorClient.getSkillResult( + args.uri, + args.metadata, + ); + const skill = envelope.skill; if (args.verify) { const reports = await verifySkills( inspectorClient, @@ -368,11 +376,7 @@ export async function runMethod( : { exitCode: EXIT_CODES.SKILL_NONCONFORMANT }), }; } - // The `{ skill }` envelope is restored here because it is what the wire - // carries: `GetSkillResultSchema` unwraps it for callers that want the - // entry, and a CLI whose contract is "print the result" must not quietly - // reshape one. - result = { skill }; + result = envelope; } else if (args.method === "resources/directory/read") { if (!args.uri) { throw new Error( diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx index e8cf89ee..03638e32 100644 --- a/clients/tui/__tests__/SkillsTab.test.tsx +++ b/clients/tui/__tests__/SkillsTab.test.tsx @@ -605,6 +605,29 @@ describe("SkillsTab (#2248)", () => { expect(lastFrame() ?? "").toContain("urn:opaque"); }); + it("renders both rows when a listing repeats a URI", () => { + // A malformed listing can carry the same skill twice, and this pane exists + // to show BOTH — a URI-keyed row would collide them and let React drop or + // reuse one (Copilot). + const dup: SkillEntry = { + uri: "skill://twice/SKILL.md", + frontmatter: { name: "twice", description: "Listed twice" }, + resources: [], + }; + const { lastFrame } = render( + , + ); + const frame = lastFrame() ?? ""; + expect(frame).toContain("Skills (2)"); + expect(frame.match(/twice/g)?.length).toBeGreaterThanOrEqual(2); + }); + it("keys a row by its index when the entry carries no URI", () => { // A URI-less entry is a `malformed-uri` finding this pane reports, so it // must still render a addressable row rather than colliding React keys. diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index 6dc3a935..7e72574c 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -333,7 +333,16 @@ export function SkillsTab({ ? "warning" : null; return ( - + // Index-keyed like the manifest and finding rows, and for + // the same reason: a malformed listing can repeat a URI, and + // this pane exists to show BOTH entries — a URI key would + // collide them and let React drop or reuse the wrong row + // (Copilot). + {isSelected ? "▶ " : " "} {worst ? ( diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 3062735b..cd659f70 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -152,6 +152,9 @@ const ALL_SKILLS = [ MISMATCHED_SKILL, ]; +/** Everything `readFixtureFile` can serve a `SKILL.md` for. */ +const SERVED_SKILLS = [...ALL_SKILLS, ACME, GLOBEX]; + /** * A `resources/read` that serves the fixture bytes for any known URI. A skill's * own `SKILL.md` comes from {@link skillMdFor}, so it agrees with the entry's @@ -161,7 +164,10 @@ const ALL_SKILLS = [ const readFixtureFile = vi.fn(async (uri: string) => { if (uri === "skill://data-analysis/reference.md") return { text: REF_TEXT }; if (uri === "skill://tampered/notes.md") return { text: NOTES_TEXT }; - const owner = ALL_SKILLS.find((skill) => skill.uri === uri); + // Every fixture, not only the four in the default catalog — the collision + // pair is served here too, or it would be handed another skill's SKILL.md and + // report a frontmatter mismatch that the fixture never meant to demonstrate. + const owner = SERVED_SKILLS.find((skill) => skill.uri === uri); return { text: owner ? skillMdFor(owner.frontmatter as Frontmatter) : SELF_TEXT, mimeType: "text/markdown", @@ -2379,6 +2385,64 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { ); }); + 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 + // `frontmatter-mismatch` — the section contradicting its own output. + 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.getByRole("button", { name: /Conformance/ }), + ).toHaveTextContent("1 error(s), 0 warning(s)"), + ); + }); + + it("prefers the bytes verification read over the preview read", async () => { + // The two verdicts came from separate `resources/read` calls, so a resource + // that changed between them could pair a verified digest with a frontmatter + // verdict computed for different bytes (Copilot). After Verify, the + // frontmatter check reads what the verification hashed. + const user = userEvent.setup(); + let served = skillMdFor(CLEAN_FM); // agrees with the listing… + const onReadSkillFile = vi.fn(async (uri: string) => { + if (uri === "skill://data-analysis/reference.md") + return { text: REF_TEXT }; + return { text: served, mimeType: "text/markdown" }; + }); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Conformance/ })); + await waitFor(() => + expect(screen.getByText("No structural issues")).toBeInTheDocument(), + ); + + // …and then the server starts serving something else. + served = skillMdFor({ ...CLEAN_FM, description: "Changed underneath" }); + await user.click( + screen.getByRole("button", { + name: "Verify skill://data-analysis/SKILL.md", + }), + ); + await waitFor(() => + expect( + screen.getByTestId("skill-frontmatter-issues"), + ).toBeInTheDocument(), + ); + expect( + within(screen.getByTestId("skill-frontmatter-issues")).getByText( + /Changed underneath/, + ), + ).toBeInTheDocument(); + }); + it("reports nothing when the served frontmatter agrees", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index ea24a71a..8b999d6b 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -91,6 +91,18 @@ interface VerificationState { */ key: string | null; files: Record; + /** + * The text of the skill's own `SKILL.md` **as the verification read it**. + * + * Held so the frontmatter comparison and the digest describe the *same* + * fetch. They were derived from two separate `resources/read` calls — the + * on-selection preview and the Verify click — so a resource that changed + * between them could pair a verified digest with a frontmatter verdict for + * different bytes (Copilot). Set only when the verified row is the entry's + * own file; the preview remains the fallback until then, since a reader who + * has not clicked Verify should still get the check. + */ + entryText?: string; } /** @@ -869,6 +881,8 @@ export function SkillsScreen({ }); const fileStates = verification.key === manifestKey ? verification.files : {}; + const verifiedEntryText = + verification.key === manifestKey ? verification.entryText : undefined; /** * Verify one manifest ROW. Keyed by row index, not by URI: the checker @@ -893,8 +907,26 @@ export function SkillsScreen({ ); }, []); + /** + * Whether a manifest row IS the skill's own `SKILL.md`. + * + * By normalized identity, like every other URI comparison here — a manifest + * that spells its self-entry equivalently still names the same file. + */ + const isSelfResource = useCallback( + (resource: SkillResource) => + selected !== undefined && + skillUriIdentity(resource.uri) === skillUriIdentity(selected.uri), + [selected], + ); + const verifyRow = useCallback( - async (index: number, resource: SkillResource, key: string) => { + async ( + index: number, + resource: SkillResource, + key: string, + isSelfRow = false, + ) => { // NOTE: opening the Conformance section deliberately does NOT happen // here. `verifyRow` is called once per row by every "Verify all" worker // as it advances, so a batch begun on one skill keeps calling it after @@ -907,7 +939,7 @@ export function SkillsScreen({ // Claimed synchronously, so two verifications of this row are ordered // before either read starts. const attempt = (nextAttempt.current += 1); - const write = (state: FileState) => + const write = (state: FileState, entryText?: string) => setVerification((prev) => { // `null` is the un-adopted initial manifest; any other mismatch is a // continuation from a manifest that has since been invalidated. @@ -917,16 +949,31 @@ export function SkillsScreen({ // finishing last must not overwrite it. const held = files[index]; if (held !== undefined && held.attempt > attempt) return prev; - return { key, files: { ...files, [index]: state } }; + return { + key, + files: { ...files, [index]: state }, + // Carried through explicitly: this returns a FRESH state object, so + // anything not named here is dropped — which silently discarded the + // verified `SKILL.md` text the frontmatter check depends on. + ...(entryText !== undefined + ? { entryText } + : prev.key === key && prev.entryText !== undefined + ? { entryText: prev.entryText } + : {}), + }; }); write({ attempt, status: "pending" }); try { const contents = await onReadSkillFile(resource.uri); - const result = await verifySkillResource( - resource, - skillFileBytes(contents), + const bytes = skillFileBytes(contents); + const result = await verifySkillResource(resource, bytes); + // The entry's own file is captured in the SAME write as its verdict, so + // the frontmatter check reads the very bytes that were just hashed — + // see `VerificationState.entryText`. + write( + { attempt, status: "done", verification: result }, + isSelfRow ? bytesToText(bytes) : undefined, ); - write({ attempt, status: "done", verification: result }); } catch (err) { write({ attempt, @@ -957,7 +1004,7 @@ export function SkillsScreen({ const key = manifestKey; const worker = async (): Promise => { for (let i = next++; i < manifest.length; i = next++) { - await verifyRow(i, manifest[i], key); + await verifyRow(i, manifest[i], key, isSelfResource(manifest[i])); } }; const workers = Math.min(VERIFY_CONCURRENCY, manifest.length); @@ -978,7 +1025,7 @@ export function SkillsScreen({ return next; }), ); - }, [manifest, manifestKey, openConformance, verifyRow]); + }, [manifest, manifestKey, openConformance, verifyRow, isSelfResource]); /** * Put one of the skill's files in the viewer. Driven both by the effect that @@ -1397,7 +1444,13 @@ export function SkillsScreen({ * first time a file happened to be fetched. */ const frontmatterIssues = useMemo(() => { - if (!selected || !showingSkillMd || preview === undefined) return []; + 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); + } + if (!showingSkillMd || preview === undefined) return []; // Run against the **raw fetched bytes**, not against `previewParts`. // // `previewParts` is a *presentation* value: it only exists when the @@ -1418,7 +1471,7 @@ export function SkillsScreen({ return []; } return checkSkillFrontmatterMatch(selected, text); - }, [selected, showingSkillMd, preview]); + }, [selected, showingSkillMd, preview, verifiedEntryText]); /** * The findings rendered as list items — everything except the two that are @@ -1438,8 +1491,23 @@ export function SkillsScreen({ [issues], ); - const errorCount = issues.filter((i) => i.severity === "error").length; - const warningCount = issues.length - errorCount; + /** + * Everything the Conformance section reports, for the header badge. + * + * The frontmatter findings render inside this section, so counting only the + * static listing issues left the badge saying `0 error(s)` above a red + * `frontmatter-mismatch` alert — the section contradicting its own output + * (Copilot). Digest and size mismatches stay OUT: they have their own + * `mismatch(es)` badge, because "the listing is wrong" and "the bytes are + * wrong" are different answers and merging them would hide which failed. + */ + const countedIssues = useMemo( + () => [...issues, ...frontmatterIssues], + [issues, frontmatterIssues], + ); + + const errorCount = countedIssues.filter((i) => i.severity === "error").length; + const warningCount = countedIssues.length - errorCount; return ( // `data-*` readiness contract for the headless tab smoke (#2148); see @@ -1940,6 +2008,7 @@ export function SkillsScreen({ index, resource, manifestKey, + isSelfResource(resource), ); }} > diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 3eea39fb..be9bba98 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -146,13 +146,14 @@ import { import { buildClientExtensions } from "./extensions.js"; import { DirectoryReadResultSchema, - GetSkillResultSchema, + GetSkillEnvelopeSchema, ListSkillsResultSchema, ModernDirectoryReadResultSchema, ModernListSkillsResultSchema, RESOURCES_DIRECTORY_READ_METHOD, SKILLS_EXTENSION_KEY, type DirectoryReadResult, + type GetSkillEnvelope, SKILLS_GET_METHOD, SKILLS_LIST_METHOD, type SkillEntry, @@ -5609,11 +5610,28 @@ export class InspectorClient extends InspectorClientEventTarget { /** * One skill entry by URI (`skills/get`, SEP-2640). The result envelope is - * required — `GetSkillResultSchema` unwraps `{ skill }` and rejects an entry + * required — `GetSkillEnvelopeSchema` requires `{ skill }` and rejects an entry * returned inline, so a non-conforming shape fails here rather than being * silently normalized past the conformance checks. */ async getSkill(uri: string, metadata?: RequestMetadata): Promise { + return (await this.getSkillResult(uri, metadata)).skill; + } + + /** + * `skills/get` as the server sent it — the `{ skill }` envelope **and any + * other members it carried**. + * + * Separate from {@link getSkill} because the callers differ: the UIs want the + * entry, while the CLI prints the result and must not reshape it. SEP-2640 + * explicitly leaves open whether this result carries `ttlMs` / `cacheScope`, + * so a server may send them — and unwrapping to the entry discards exactly + * those (Copilot). + */ + async getSkillResult( + uri: string, + metadata?: RequestMetadata, + ): Promise { if (!this.client) { throw new Error("Client is not connected"); } @@ -5622,14 +5640,13 @@ export class InspectorClient extends InspectorClientEventTarget { uri, ...(effectiveMeta ? { _meta: effectiveMeta } : {}), }; - // `GetSkillResultSchema` unwraps the envelope, so there is nothing to - // unwrap here. + // The envelope is returned whole; `getSkill` is the one that unwraps. try { return await this.invokeMcpClient( () => this.client!.request( { method: SKILLS_GET_METHOD, params }, - GetSkillResultSchema, + GetSkillEnvelopeSchema, this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: SKILLS_GET_METHOD }, diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index e13a6fcc..4a36ec87 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -167,7 +167,21 @@ export const ModernListSkillsResultSchema = ListSkillsResultSchema.extend({ * — which is exactly the failure this extension's support exists to *report*. * A server that returns the entry inline now fails the parse, loudly. */ -const GetSkillEnvelopeSchema = z.looseObject({ skill: SkillEntrySchema }); +export const GetSkillEnvelopeSchema = z.looseObject({ + skill: SkillEntrySchema, +}); + +/** + * The `skills/get` result as the server sent it, envelope and all. + * + * Exported alongside the unwrapping schema below because the two callers want + * different things: the UIs want the entry, while the CLI's job is to print + * **the result** — and the caching attributes SEP-2640 leaves open are members + * a `looseObject` accepts and the transform then discards, so unwrapping for + * everyone silently dropped them from a contract that promised not to reshape + * anything (Copilot). + */ +export type GetSkillEnvelope = z.infer; /** * `skills/get` result, unwrapped to the entry it carries.