fix: address Copilot review round 17 on #2248
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
5a1f0efb5a
commit
9117f633aa
8 changed files with 183 additions and 29 deletions
|
|
@ -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.",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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}`;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<SkillsTab
|
||||
|
|
@ -776,7 +793,14 @@ describe("SkillsTab (#2248)", () => {
|
|||
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");
|
||||
});
|
||||
|
|
|
|||
|
|
@ -88,6 +88,18 @@ const FILE_COLOR: Record<string, string> = {
|
|||
"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<SkillVerifyReport["outcome"], string> = {
|
||||
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]"}
|
||||
</Text>
|
||||
</Box>
|
||||
|
|
|
|||
|
|
@ -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(<ControlledSkillsScreen skills={[ACME, GLOBEX]} />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
|
|
|
|||
|
|
@ -1624,7 +1624,17 @@ export function SkillsScreen({
|
|||
<EmptyState>No skills listed</EmptyState>
|
||||
) : (
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue