From 01a8f14c24aebfb08658eb8b056f481a6e0cefd5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 22:20:20 -0400 Subject: [PATCH] fix: address Copilot review round 14 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The TUI ignored `incomplete`.** Round 12 added that field precisely so a consumer could tell "not fully checked" from a real failure, and then the one client that reads the report did not consume it — which made the field decorative. The pane shows an Incomplete: block with the report's own reason, and the status line reads INCOMPLETE rather than FAILED. It sits ABOVE the manifest, not beside the status line: it explains the list that follows — only the first N rows were fetched, the rest stay marked `·` because nobody looked at them — and below a 512-row manifest it would be off-screen, which is the same as absent. Found by writing the test, whose first version asserted against a 600-entry fixture and could see neither string. The test triggers truncation through the BYTE budget instead, so the manifest stays three rows and the assertions measure the pane rather than the test's viewport. **The capped self-entry was reported under the wrong URI.** A manifest may write its self-entry in a normalized-equivalent form; the fallback recorded `entry.uri`, so manifest rows keyed on the declared spelling matched nothing while the normalized extra-files filter suppressed it as already covered. The verdict existed in the report and appeared nowhere on screen. Recorded under `declaredSelf.uri` now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- clients/tui/__tests__/SkillsTab.test.tsx | 44 +++++++++++++++++++ clients/tui/src/components/SkillsTab.tsx | 24 +++++++++- .../test/core/mcp/skillsVerification.test.ts | 32 ++++++++++++++ core/mcp/skillsVerification.ts | 8 +++- 4 files changed, 106 insertions(+), 2 deletions(-) diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx index 03638e32..2513d374 100644 --- a/clients/tui/__tests__/SkillsTab.test.tsx +++ b/clients/tui/__tests__/SkillsTab.test.tsx @@ -750,6 +750,50 @@ describe("SkillsTab (#2248)", () => { ); }); + it("says a verification was INCOMPLETE rather than merely failed", async () => { + // `verifySkills` sets `incomplete` so a consumer can tell "not fully + // checked" from a real failure; printing only "Verification FAILED" threw + // that distinction away, and the entries beyond the cap stayed marked `·` + // with nothing explaining why (Copilot). + // + // Truncation is triggered by the BYTE budget rather than the 512-entry one + // so the manifest stays three rows long: a 512-row pane pushes the status + // line off the frame, which would make this assert the test's viewport + // rather than the pane's behaviour. + const big = "x".repeat(6 * 1024 * 1024); + const fat: SkillEntry = { + uri: "skill://fat/SKILL.md", + frontmatter: { name: "fat", description: "Understates its sizes" }, + resources: Array.from({ length: 3 }, (_, i) => ({ + uri: i === 0 ? "skill://fat/SKILL.md" : `skill://fat/f${i}.md`, + digest: CLEAN_DIGEST, + size: 1, + })), + }; + const { lastFrame, stdin } = render( + ({ + result: { contents: [{ uri, text: big }] }, + })), + )} + width={160} + height={40} + focusedPane="list" + />, + ); + stdin.write(ENTER); + await tick(); + await tick(); + const frame = lastFrame() ?? ""; + expect(frame).toContain("Incomplete:"); + expect(frame).toContain("actually served"); + expect(frame).toContain("Verification INCOMPLETE"); + expect(frame).not.toContain("Verification FAILED"); + }); + 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 diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index b1bf292d..cdbeecff 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -402,6 +402,26 @@ export function SkillsTab({ ))} + {/* ABOVE the manifest, because it explains the list that + follows: only the first N rows were fetched, and the rest + stay marked `·` because nobody looked at them. Below a + 512-row manifest it would be off-screen, which is the same as + absent. `verifySkills` sets `incomplete` precisely so a + consumer can tell "not fully checked" from a real failure + (Copilot). */} + {activeReport?.incomplete && ( + <> + + + Incomplete: + + + + {activeReport.incomplete} + + + )} + Manifest @@ -504,7 +524,9 @@ export function SkillsTab({ : activeReport ? activeReport.ok ? "[Verified — Enter to re-verify]" - : "[Verification FAILED — Enter to re-verify]" + : activeReport.incomplete + ? "[Verification INCOMPLETE — Enter to re-verify]" + : "[Verification FAILED — Enter to re-verify]" : "[Enter to verify digests and frontmatter]"} diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index de12de11..6f3b9018 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -702,6 +702,38 @@ describe("verifySkills (#2248)", () => { expect(report.files[0].status).toBe("mismatch"); }); + it("reports the capped self-entry under the URI the MANIFEST declared", async () => { + // A manifest may write its self-entry in a normalized-equivalent form. The + // fallback recorded `entry.uri`, so a consumer matching rows against the + // manifest found nothing — while a normalized "extra files" filter + // suppressed it as already covered. The verdict existed in the report and + // appeared nowhere on screen (Copilot). + const declaredSpelling = "skill://huge/x/../SKILL.md"; + const skill: SkillEntry = { + uri: "skill://huge/SKILL.md", + frontmatter: { name: "huge", description: "Listed" }, + resources: [ + ...Array.from({ length: 600 }, (_, i) => ({ + uri: `skill://huge/f${i}.md`, + digest: `sha256:${"a".repeat(64)}`, + size: 1, + })), + { uri: declaredSpelling, digest: `sha256:${"b".repeat(64)}`, size: 1 }, + ], + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { contents: [{ uri, text: "x" }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + const self = report.files.find((f) => f.uri === declaredSpelling); + expect(self?.status).toBe("mismatch"); + // …and NOT under the entry's own spelling, which no manifest row carries. + expect(report.files.some((f) => f.uri === "skill://huge/SKILL.md")).toBe( + false, + ); + }); + it("does not truncate a conforming manifest", async () => { // A conforming skill totals at most 16 MiB by definition, so the bound // must never shorten one — otherwise it would trade a hostile-server diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 370c4bc2..e64f1955 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -376,7 +376,13 @@ export async function verifySkills( ); if (declaredSelf && entryBytes !== undefined) { files.push({ - uri: entry.uri, + // The DECLARED spelling, not the entry's. They can differ — a + // manifest may write its self-entry in a normalized-equivalent + // form — and a consumer matching rows against the manifest then + // finds nothing, while a normalized "extra files" filter suppresses + // it as already covered. The result was a verdict that existed in + // the report and appeared nowhere on screen (Copilot). + uri: declaredSelf.uri, ...(await verifySkillResource(declaredSelf, entryBytes)), }); }