From 9117f633aa5e94482112af51ab759714e44094eb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 23:12:28 -0400 Subject: [PATCH] fix: address Copilot review round 17 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the five findings share one root cause: the tri-state `outcome` added in round 15 was not propagated to consumers, which kept branching on `ok` — true for an `incomplete` report, since nothing that was checked was wrong. - CLI `summarizeSkillVerification` counts off `outcome`, not `ok`, so a truncated walk no longer prints "no conformance errors" one line before exiting SKILL_INCOMPLETE. A mixed failed/incomplete catalog reports both counts rather than letting the louder verdict hide the quieter. - `verifySkills` sets the truncation reason only when entries were actually left unread. A file crossing the byte budget as the FINAL entry stopped nothing, and "Stopped after 3 of 3" both read as a contradiction and demoted a fully-read skill out of `verified`. - The TUI status line switches on `outcome` through a `Record` over the union, so the INCOMPLETE arm is reachable and a fourth outcome would be a type error rather than a silently missing label. - The `incomplete` doc paragraph said it makes `ok` false. It does not, deliberately — corrected, and it now names `outcome` as the thing a consumer must branch on. - The web sidebar composes in the per-URI `duplicate-name` warning, so two colliding skills are badged in the catalog instead of looking clean until one is selected. The suppressed `selfAttempted` finding is stale — fixed in round 16. The existing TUI INCOMPLETE test was passing for the wrong reason: its fixture understated every size, which is itself a mismatch, so the report was `failed` and only the old `ok`-first branch printed INCOMPLETE. It is rebuilt with honest digests and sizes, so truncation comes from the declared-size prefilter and the report is genuinely incomplete. 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 | 29 +++++++++++++ clients/cli/src/handlers/skills-verify.ts | 25 +++++++++-- clients/tui/__tests__/SkillsTab.test.tsx | 42 +++++++++++++++---- clients/tui/src/components/SkillsTab.tsx | 23 +++++++--- .../SkillsScreen/SkillsScreen.test.tsx | 21 ++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 12 +++++- .../test/core/mcp/skillsVerification.test.ts | 29 +++++++++++++ core/mcp/skillsVerification.ts | 31 +++++++++----- 8 files changed, 183 insertions(+), 29 deletions(-) diff --git a/clients/cli/__tests__/run-method-skills.test.ts b/clients/cli/__tests__/run-method-skills.test.ts index e750f97c..ae791c48 100644 --- a/clients/cli/__tests__/run-method-skills.test.ts +++ b/clients/cli/__tests__/run-method-skills.test.ts @@ -303,4 +303,33 @@ describe("summarizeSkillVerification (#2248)", () => { "1 of 1 skill failed verification (0 digest/size mismatch across 0 files).", ); }); + + it("does not claim a truncated walk verified", () => { + // An `incomplete` report keeps `ok: true` — nothing checked was wrong — + // so a summary branching on `ok` printed "no conformance errors" one line + // before the run exited SKILL_INCOMPLETE (Copilot). + const cut = report({ + outcome: "incomplete", + incomplete: "Stopped after 2 of 9 manifest entries.", + }); + expect(summarizeSkillVerification([cut])).toBe( + "Checked 1 skill and 1 file: no conformance errors in what was read." + + " 1 of 1 skill could not be fully checked: the read bounds stopped the walk.", + ); + }); + + it("reports a mixed catalog on both counts", () => { + // The louder verdict must not hide the quieter one: a caller told only + // about the failure would think the rest of the catalog was cleared. + const failed = report({ + ok: false, + outcome: "failed", + files: [{ uri: "skill://demo/SKILL.md", status: "mismatch" }], + }); + const cut = report({ outcome: "incomplete", incomplete: "Stopped." }); + expect(summarizeSkillVerification([report(), failed, cut])).toBe( + "1 of 3 skills failed verification (1 digest/size mismatch across 3 files)." + + " 1 of 3 skills could not be fully checked: the read bounds stopped the walk.", + ); + }); }); diff --git a/clients/cli/src/handlers/skills-verify.ts b/clients/cli/src/handlers/skills-verify.ts index 9ef59cd8..7d1910f1 100644 --- a/clients/cli/src/handlers/skills-verify.ts +++ b/clients/cli/src/handlers/skills-verify.ts @@ -21,7 +21,14 @@ import type { SkillVerifyReport } from "@inspector/core/mcp/skillsVerification.j export function summarizeSkillVerification( reports: readonly SkillVerifyReport[], ): string { - const failed = reports.filter((report) => !report.ok).length; + // ⚠️ Counted off `outcome`, never off `ok`. `ok` means "nothing that was + // checked is wrong", which an `incomplete` report satisfies while the walk + // was cut short — so branching on `ok` printed "no conformance errors" one + // line before exiting SKILL_INCOMPLETE (Copilot). + const failed = reports.filter((report) => report.outcome === "failed").length; + const incomplete = reports.filter( + (report) => report.outcome === "incomplete", + ).length; const files = reports.reduce((sum, report) => sum + report.files.length, 0); const mismatched = reports.reduce( (sum, report) => @@ -30,7 +37,17 @@ export function summarizeSkillVerification( ); const skillWord = reports.length === 1 ? "skill" : "skills"; const fileWord = files === 1 ? "file" : "files"; - return failed === 0 - ? `Verified ${reports.length} ${skillWord} and ${files} ${fileWord}: no conformance errors.` - : `${failed} of ${reports.length} ${skillWord} failed verification (${mismatched} digest/size mismatch across ${files} ${fileWord}).`; + // A catalog can be both: some skills broken, others merely cut short. Say so + // rather than letting the louder verdict hide the quieter one. + const incompleteClause = + incomplete === 0 + ? "" + : ` ${incomplete} of ${reports.length} ${skillWord} could not be fully checked: the read bounds stopped the walk.`; + const headline = + failed === 0 + ? incomplete === 0 + ? `Verified ${reports.length} ${skillWord} and ${files} ${fileWord}: no conformance errors.` + : `Checked ${reports.length} ${skillWord} and ${files} ${fileWord}: no conformance errors in what was read.` + : `${failed} of ${reports.length} ${skillWord} failed verification (${mismatched} digest/size mismatch across ${files} ${fileWord}).`; + return `${headline}${incompleteClause}`; } diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx index 2513d374..e59c563c 100644 --- a/clients/tui/__tests__/SkillsTab.test.tsx +++ b/clients/tui/__tests__/SkillsTab.test.tsx @@ -757,18 +757,35 @@ describe("SkillsTab (#2248)", () => { // 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 + // so the manifest stays four 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. + // + // ⚠️ Every digest and size here is HONEST, so the only thing wrong with + // this skill is the unfinished walk. An earlier version understated the + // sizes, which is itself a size mismatch — the report was `failed` and the + // test passed only because the status line branched on `ok` before + // `incomplete`, the very bug this pins (Copilot). const big = "x".repeat(6 * 1024 * 1024); + const bigDigest = await sha256Digest(textToBytes(big)); + const fatMd = "---\nname: fat\ndescription: Four big files\n---\n\n# F\n"; 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, - })), + frontmatter: { name: "fat", description: "Four big files" }, + resources: [ + { + uri: "skill://fat/SKILL.md", + digest: await sha256Digest(textToBytes(fatMd)), + size: textToBytes(fatMd).byteLength, + }, + // Three 6 MiB files: the third crosses the 16 MiB budget, so the + // manifest is cut before it and one entry is never fetched. + ...Array.from({ length: 3 }, (_, i) => ({ + uri: `skill://fat/f${i + 1}.md`, + digest: bigDigest, + size: textToBytes(big).byteLength, + })), + ], }; const { lastFrame, stdin } = render( { pageCount={1} inspectorClient={mockClient( vi.fn().mockImplementation(async (uri: string) => ({ - result: { contents: [{ uri, text: big }] }, + result: { + contents: [ + { + uri, + text: uri === "skill://fat/SKILL.md" ? fatMd : big, + }, + ], + }, })), )} width={160} @@ -789,7 +813,7 @@ describe("SkillsTab (#2248)", () => { await tick(); const frame = lastFrame() ?? ""; expect(frame).toContain("Incomplete:"); - expect(frame).toContain("actually served"); + expect(frame).toContain("interoperability limits"); expect(frame).toContain("Verification INCOMPLETE"); expect(frame).not.toContain("Verification FAILED"); }); diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index c76ec46e..c09e24c5 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -88,6 +88,18 @@ const FILE_COLOR: Record = { "read-error": "red", }; +/** + * The status line for each of the three verification outcomes. + * + * A `Record` over the union rather than a chain of ternaries, so adding a + * fourth outcome is a type error here instead of a silently missing label. + */ +const VERIFY_STATUS: Record = { + verified: "[Verified — Enter to re-verify]", + incomplete: "[Verification INCOMPLETE — Enter to re-verify]", + failed: "[Verification FAILED — Enter to re-verify]", +}; + /** * The explanation printed under a failed file row. * @@ -529,11 +541,12 @@ export function SkillsTab({ {verifying ? "[Verifying…]" : activeReport - ? activeReport.ok - ? "[Verified — Enter to re-verify]" - : activeReport.incomplete - ? "[Verification INCOMPLETE — Enter to re-verify]" - : "[Verification FAILED — Enter to re-verify]" + ? // ⚠️ Switched on `outcome`, not on `ok`. `ok` stays + // true for an `incomplete` report — nothing checked + // was wrong — so an `ok`-first branch printed + // "Verified" for a walk the read bounds cut short and + // the INCOMPLETE arm was unreachable (Copilot). + VERIFY_STATUS[activeReport.outcome] : "[Enter to verify digests and frontmatter]"} diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index ee4b74d3..4b6e524c 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -2401,6 +2401,27 @@ describe("SkillsScreen name collisions (#2248)", () => { expect(screen.getByTestId("skill-name-collision")).toBeInTheDocument(); }); + it("badges the collision in the sidebar, before either is selected", async () => { + // The collision is a property of the LISTING, so `checkSkillConformance` + // on one entry cannot see it — and a sidebar computed from that alone + // showed both colliding rows as clean until one was clicked, which is + // exactly when a reader most needs to be told two rows share a name + // (Copilot). Nothing is selected here on purpose. + renderWithMantine(); + const rows = [ACME, GLOBEX].map((skill) => + screen.getByText(skill.uri).closest(".mantine-NavLink-root"), + ); + for (const row of rows) { + expect(row).not.toBeNull(); + // One finding, badged — a warning, so yellow rather than the red that + // would call a conforming server broken. + const badge = row?.querySelector(".mantine-Badge-root"); + expect(badge).not.toBeNull(); + expect(badge).toHaveTextContent("1"); + expect(badge?.getAttribute("style") ?? "").toContain("yellow"); + } + }); + it("says nothing when the names are distinct", 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 922514df..0d697044 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -1624,7 +1624,17 @@ export function SkillsScreen({ No skills listed ) : ( filtered.map((skill) => { - const skillIssues = checkSkillConformance(skill); + // ⚠️ The collision is a property of the LISTING, not of the + // entry, so `checkSkillConformance` alone cannot see it — and + // a sidebar computed from that alone showed both colliding + // skills as clean until one was selected, which is exactly + // when a reader most needs to be told two rows are the same + // name (Copilot). Same composition as `conformance` above. + const collision = collisions.get(skillUriIdentity(skill.uri)); + const skillIssues = [ + ...checkSkillConformance(skill), + ...(collision ? [collision] : []), + ]; const errors = skillIssues.filter( (i) => i.severity === "error", ).length; diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 3b46d24f..0bbdc822 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -656,6 +656,35 @@ describe("verifySkills (#2248)", () => { expect(readResource.mock.calls.length).toBeLessThan(10); expect(report.incomplete).toMatch(/actually served/); }); + + it("is not incomplete when the budget is crossed by the LAST entry", async () => { + // Crossing the line on the final row stopped nothing: every manifest entry + // was fetched and checked. Reporting "Stopped after 4 of 4" there both + // reads as a contradiction and demotes a fully-read skill out of + // `verified` (Copilot). + // + // The sizes are understated for the same reason as the test above — with + // honest ones the *declared* prefilter stops first and the received-bytes + // guard is never reached at all. That understatement is itself a size + // mismatch, so this fixture is `failed`; what it pins is that the walk is + // not ALSO reported as cut short. + const { skill, client } = await truncatable({ + name: "edge", + count: 4, + body: "z".repeat(6 * 1024 * 1024), + }); + for (const r of skill.resources as { size?: number }[]) r.size = 1; + const readResource = vi.spyOn( + client as unknown as { readResource: (u: string) => unknown }, + "readResource", + ); + const [report] = await verifySkills(client, [skill]); + // All four read — the fourth is what crosses the 16 MiB budget. + expect(readResource.mock.calls.length).toBe(4); + expect(report.files).toHaveLength(4); + expect(report.incomplete).toBeUndefined(); + expect(report.outcome).toBe("failed"); + }); it("still reports the file that crossed the byte budget", async () => { // The crossing file is verified before the walk stops, so its verdict is // not fetched and then thrown away. diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index efd7520a..fa65057a 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -94,14 +94,17 @@ export interface SkillVerifyReport { /** * Why the manifest could not be checked in full, or `undefined` when it was. * - * Set when the read bounds truncated the manifest. It is reported separately - * from `ok` being false so a consumer can tell "this skill is wrong" from - * "this skill was not fully checked" — but it does make `ok` false, because - * the alternative is worse: entries past the cap are never fetched, and - * `resource-limit-exceeded` is only a WARNING, so a manifest whose 513th - * file was tampered with reported `ok: true` and the CLI said the skill - * verified (Copilot). A bound that turns a denial of service into a false - * pass has traded down. + * Set when the read bounds truncated the manifest, and **only** when entries + * were actually left unread — a file that crosses the byte budget as the + * last entry checked nothing short, so it is not incomplete. + * + * ⚠️ It does **not** make {@link ok} false. `ok` keeps the narrow meaning of + * "nothing that was checked is wrong", and a truncated walk checked nothing + * that was wrong. The signal a consumer must branch on is {@link outcome} + * being `"incomplete"`, never `!ok` — a manifest whose unread 513th file was + * tampered with reports `ok: true`, and a consumer that prints "verified" on + * `ok` alone turns a denial of service into a false pass (Copilot). Both the + * CLI summary and the TUI status line had exactly that bug. */ incomplete?: string; /** @@ -306,7 +309,7 @@ export async function verifySkills( // an equivalent form, so a raw string test would disagree with it and read // the same file a second time. let selfAttempted = false; - for (const resource of manifest) { + for (const [index, resource] of manifest.entries()) { if (skillUriIdentity(resource.uri) === entryIdentity) { // Marked before the read, not after: a row the walk reached but could // not read has still been attempted, and its failure is recorded here @@ -357,7 +360,15 @@ export async function verifySkills( // prevent and is not something this API exposes. receivedBytes += bytes.byteLength; if (receivedBytes > SKILL_MAX_TOTAL_BYTES) { - incomplete = `Stopped after ${files.length} of ${declared.length} manifest entries: the files actually served exceed the ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limit, whatever sizes the manifest declared.`; + // ⚠️ Only *incomplete* when the budget actually cost a read. Crossing + // the line on the final entry stopped nothing — every manifest row was + // fetched and checked — and reporting "Stopped after 3 of 3" there + // both reads as a contradiction and demotes a fully-checked skill out + // of `verified` (Copilot). The prefilter's own reason, if it dropped + // entries before the walk, is already set and is not overwritten. + if (index < manifest.length - 1) { + incomplete = `Stopped after ${index + 1} of ${declared.length} manifest entries: the files actually served exceed the ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limit, whatever sizes the manifest declared.`; + } break; } }