fix: address Copilot review round 4 on #2251

- SkillsScreen: actually call `skills/get`. The client method and its
  tests existed but no production caller invoked it, so #2234's acceptance
  criterion ("skills/get retrieves a single entry") was unmet and a server
  author's required handler could not be exercised. The detail pane now
  fetches the selected URI on demand and reports whether the fetched entry
  AGREES with the one skills/list advertised — both describe the same
  skill, so a disagreement is a server bug only a side-by-side fetch shows.
- core/mcp/sha256.ts: a dependency-free SHA-256, used when `crypto.subtle`
  is absent. `SubtleCrypto` needs a secure context, and this app is
  documented as servable over plain HTTP on a LAN IP — where every skill
  verification threw and the UI reported a read failure for files it had
  fetched fine. Checked against the FIPS 180-4 vectors and differentially
  against WebCrypto.
- skills.ts: require the `skill:` scheme in `normalizeSkillUri`. Checking
  only that a URI was hierarchical let `https://demo/SKILL.md` pass the
  name and root checks — a manifest pointing anywhere on the web, reported
  as conforming.
- skills.ts: `missing-digest` and `missing-size` are errors. Both are
  required fields, and an omitted `size` is what lets a server slip past
  the 16 MiB pre-fetch limit while the UI reports zero errors. `warning`
  is now reserved for what is legal yet unverifiable — `"dynamic"`.
- SkillsScreen: key the "Verify all" batch guard to the manifest. A global
  flag left a newly selected skill's button disabled until the previous
  skill's reads settled — forever, if one hung.

Not changed, deliberately: whether a modern-era `skills/list` result MUST
carry the SEP-2549 caching attributes. #2234's analysis records it as open
and the review asserts the opposite; neither reading was checked against
the normative text. Leaving the schema permissive accepts a server that
omits them, while tightening on a wrong reading would reject conforming
responses — the more expensive direction. `skillsSchemas.ts` states this
and #2248 settles it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-04 22:38:03 -04:00
commit 1cb05ca573
11 changed files with 527 additions and 23 deletions

View file

@ -835,6 +835,7 @@ function App() {
onRefreshResources,
onRefreshSkills,
onReadSkillFile,
onGetSkill,
onRefreshTasks,
onTogglePaginatedLists,
onLoadMoreTools,
@ -1832,6 +1833,7 @@ function App() {
onSkillsUiChange: setUi.setSkillsUi,
onRefreshSkills,
onReadSkillFile,
onGetSkill,
};
const tasksPanelProps: TasksPanelProps = {

View file

@ -107,6 +107,13 @@ const baseProps: SkillsScreenProps = {
onUiChange: vi.fn(),
onRefreshList: vi.fn(),
onReadSkillFile: readFixtureFile,
// Echoes back the very entry `skills/list` advertised, so the default is the
// agreeing case; tests that care about a disagreement override it.
onGetSkill: vi.fn(async (uri: string) => {
const found = ALL_SKILLS.find((skill) => skill.uri === uri);
if (!found) throw new Error(`Unknown skill uri: ${uri}`);
return found;
}),
};
// SkillsScreen is controlled: the selection and the sidebar search live in the
@ -430,6 +437,97 @@ describe("SkillsScreen", () => {
expect(within(issues).getAllByText("duplicate-resource")).toHaveLength(2);
});
it("fetches the selected entry through skills/get and reports agreement", async () => {
// The acceptance criterion this exists for: `skills/get` is one of the two
// methods the extension requires, and a server author's handler is only
// exercisable if something actually calls it.
const user = userEvent.setup();
const onGetSkill = vi.fn().mockResolvedValue(CLEAN_SKILL);
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
expect(onGetSkill).toHaveBeenCalledWith(CLEAN_SKILL.uri);
expect(
await screen.findByText("skills/get agrees with skills/list"),
).toBeInTheDocument();
});
it("reports a skills/get entry that disagrees with the listing", async () => {
// Both describe the same skill, so a disagreement is a server bug that
// only a side-by-side fetch can surface.
const user = userEvent.setup();
const onGetSkill = vi.fn().mockResolvedValue({
...CLEAN_SKILL,
frontmatter: { ...CLEAN_SKILL.frontmatter, description: "different" },
});
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
expect(
await screen.findByText("skills/get disagrees with skills/list"),
).toBeInTheDocument();
// The fetched entry is rendered beside the verdict so the difference is
// inspectable rather than merely asserted. (Its JSON goes through
// `ContentViewer`'s highlighter, which splits tokens across elements, so
// the presence of the block is what is pinned here — the copy above is
// what states the finding.)
expect(screen.getByTestId("skills-get-result")).toBeInTheDocument();
});
it("reports a failed skills/get", async () => {
const user = userEvent.setup();
const onGetSkill = vi.fn().mockRejectedValue(new Error("-32602"));
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
expect(await screen.findByText("skills/get failed")).toBeInTheDocument();
expect(screen.getByText("-32602")).toBeInTheDocument();
});
it("discards a skills/get that resolves after the selection moved on", async () => {
const user = userEvent.setup();
let release: ((value: SkillEntry) => void) | undefined;
const onGetSkill = vi.fn(
() =>
new Promise<SkillEntry>((resolve) => {
release = resolve;
}),
);
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
await user.click(screen.getByText("tampered"));
release?.(CLEAN_SKILL);
expect(screen.queryByTestId("skills-get-result")).not.toBeInTheDocument();
});
it("frees Verify all for a newly selected skill while the old batch is hung", async () => {
// A global flag would leave the new skill's button disabled until the
// previous skill's reads settled — forever, if one of them hangs.
const user = userEvent.setup();
const onReadSkillFile = vi.fn(
() => new Promise<{ text: string }>(() => {}),
);
renderWithMantine(
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Verify all/ }));
expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled();
await user.click(screen.getByText("tampered"));
expect(
screen.getByRole("button", { name: /Verify all/ }),
).not.toBeDisabled();
});
it("shows the SKILL.md preview on demand", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);

View file

@ -80,6 +80,18 @@ interface PreviewState {
message?: string;
}
/**
* The result of the on-demand `skills/get`, plus the manifest it belongs to.
* `agrees` records whether the fetched entry matched the one `skills/list`
* returned — the reason for making the call at all.
*/
interface FetchedEntryState {
key: string | null;
entry?: SkillEntry;
agrees?: boolean;
message?: string;
}
export interface SkillsScreenProps {
skills: SkillEntry[];
/** Pages the last `skills/list` walk took; shown so pagination is visible. */
@ -91,6 +103,13 @@ export interface SkillsScreenProps {
onRefreshList: () => void;
/** Fetch one skill file's contents via `resources/read`, on demand. */
onReadSkillFile: (uri: string) => Promise<SkillFileContents>;
/**
* Re-fetch the selected entry through `skills/get` (SEP-2640). Distinct from
* the entry `skills/list` already returned, and the point of exercising it is
* that the two must agree: a server whose `skills/get` disagrees with its own
* listing is broken in a way only a side-by-side fetch can show.
*/
onGetSkill: (uri: string) => Promise<SkillEntry>;
}
/**
@ -149,6 +168,15 @@ const ControlsRow = Group.withProps({
gap: "sm",
});
// The Resources header carries three action buttons beside its count badge, so
// it wraps rather than truncating the badge on a narrow detail pane — unlike
// the sidebar row above, where the search field is meant to absorb the space.
const SectionControlsRow = Group.withProps({
justify: "space-between",
wrap: "wrap",
gap: "sm",
});
const SearchInput = TextInput.withProps({
size: "xs",
flex: 1,
@ -269,6 +297,7 @@ export function SkillsScreen({
onUiChange,
onRefreshList,
onReadSkillFile,
onGetSkill,
}: SkillsScreenProps) {
const { selectedSkillUri, search } = ui;
// Both slices carry the manifest key they belong to, and every async
@ -284,9 +313,14 @@ export function SkillsScreen({
files: {},
});
const [previewState, setPreviewState] = useState<PreviewState>({ key: null });
// True while a "Verify all" batch is in flight; disables the button so a
// second click cannot stack another pool of workers on top.
const [batchRunning, setBatchRunning] = useState(false);
const [fetchedEntry, setFetchedEntry] = useState<FetchedEntryState>({
key: null,
});
// The manifest whose "Verify all" batch is in flight, or `null`. Keyed rather
// than a bare boolean: a global flag would leave a NEWLY selected skill's
// button disabled until the previous skill's reads settled — indefinitely, if
// one of them hangs.
const [batchKey, setBatchKey] = useState<string | null>(null);
// Monotonic per-row attempt token. A ref because it is claimed inside an
// event handler, never during render.
const nextAttempt = useRef(0);
@ -342,6 +376,7 @@ export function SkillsScreen({
useValueChange(manifestKey, (next) => {
setVerification({ key: next, files: {} });
setPreviewState({ key: next });
setFetchedEntry({ key: next });
});
const fileStates = verification.key === manifestKey ? verification.files : {};
@ -407,12 +442,14 @@ export function SkillsScreen({
}
};
const workers = Math.min(VERIFY_CONCURRENCY, manifest.length);
setBatchRunning(true);
setBatchKey(key);
// The concurrency cap is per invocation, so without the button being
// disabled below, a second click would start a second pool of four and a
// third would make it twelve — the flood the cap exists to prevent.
void Promise.all(Array.from({ length: workers }, () => worker())).finally(
() => setBatchRunning(false),
// Clears only ITS OWN batch: a stale finalizer must not free a button the
// user has since re-armed on another skill.
() => setBatchKey((prev) => (prev === key ? null : prev)),
);
}, [manifest, manifestKey, verifyRow]);
@ -438,6 +475,33 @@ export function SkillsScreen({
});
}, [manifestKey, onReadSkillFile, selected]);
const fetchEntry = useCallback(() => {
if (!selected) return;
const key = manifestKey;
// Same shape as the SKILL.md read: a click handler cannot await, the chain
// ends in its own `catch`, and both arms compare the manifest key they
// started under so a late answer cannot land under another skill.
void onGetSkill(selected.uri)
.then((entry) => {
// Compared field-by-field against what `skills/list` advertised. The
// two describe the same skill, so a disagreement is a server bug that
// only shows up when both are fetched.
const agrees = JSON.stringify(entry) === JSON.stringify(selected);
setFetchedEntry((prev) =>
prev.key !== null && prev.key !== key ? prev : { key, entry, agrees },
);
})
.catch((err: unknown) => {
const message = err instanceof Error ? err.message : String(err);
setFetchedEntry((prev) =>
prev.key !== null && prev.key !== key ? prev : { key, message },
);
});
}, [manifestKey, onGetSkill, selected]);
const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined;
const batchRunning = batchKey === manifestKey;
const preview =
previewState.key === manifestKey ? previewState.contents : undefined;
const previewError =
@ -561,7 +625,7 @@ export function SkillsScreen({
</Stack>
<Stack gap="xs">
<ControlsRow>
<SectionControlsRow>
<InlineRow>
<SectionHeading>Resources</SectionHeading>
<CountBadge>
@ -570,6 +634,9 @@ export function SkillsScreen({
</CountBadge>
</InlineRow>
<InlineRow>
<VerifyButton onClick={fetchEntry}>
Fetch with skills/get
</VerifyButton>
<VerifyButton onClick={showSkillMd}>
View SKILL.md
</VerifyButton>
@ -581,7 +648,7 @@ export function SkillsScreen({
Verify all
</VerifyButton>
</InlineRow>
</ControlsRow>
</SectionControlsRow>
{selected.resources === DYNAMIC_RESOURCES ? (
<Alert color="yellow" title="Dynamic resources">
This skill declares{" "}
@ -692,6 +759,41 @@ export function SkillsScreen({
})}
</Stack>
{fetched?.message !== undefined && (
<Alert color="red" title="skills/get failed">
{fetched.message}
</Alert>
)}
{fetched?.entry !== undefined && (
<Alert
data-testid="skills-get-result"
color={fetched.agrees ? "green" : "red"}
title={
fetched.agrees
? "skills/get agrees with skills/list"
: "skills/get disagrees with skills/list"
}
>
<Stack gap={2}>
<Text size="sm">
{fetched.agrees
? "The entry this server returns for this URI is identical to the one it listed."
: "The entry this server returns for this URI differs from the one it listed; both describe the same skill, so one of them is wrong."}
</Text>
{!fetched.agrees && (
<ContentViewer
block={{
type: "text",
text: JSON.stringify(fetched.entry, null, 2),
}}
mimeType="application/json"
copyable
/>
)}
</Stack>
</Alert>
)}
{previewError && (
<Alert color="red" title="Could not read SKILL.md">
{previewError}

View file

@ -433,6 +433,11 @@ const skillsArgs: SkillsPanelProps = {
onSkillsUiChange: fn(),
onRefreshSkills: fn(),
onReadSkillFile: fn(async () => ({ text: "" })),
onGetSkill: fn(async (uri: string) => ({
uri,
frontmatter: {},
resources: [],
})),
};
const tasksArgs: TasksPanelProps = {

View file

@ -178,6 +178,7 @@ function makeProps(...overrides: PropOverrides[]): InspectorViewProps {
onSkillsUiChange: vi.fn(),
onRefreshSkills: vi.fn(),
onReadSkillFile: vi.fn().mockResolvedValue({ text: "" }),
onGetSkill: vi.fn(),
...mergeBundle("skills", overrides),
},
tasks: {

View file

@ -463,6 +463,7 @@ export function InspectorView({
onSkillsUiChange,
onRefreshSkills,
onReadSkillFile,
onGetSkill,
} = skillsPanel;
const {
tasks,
@ -1061,6 +1062,7 @@ export function InspectorView({
onUiChange: onSkillsUiChange,
onRefreshList: onRefreshSkills,
onReadSkillFile,
onGetSkill,
};
const tasksScreenProps = {
tasks,

View file

@ -322,6 +322,8 @@ export interface SkillsPanelProps {
onRefreshSkills: () => void;
/** Read one skill file (`resources/read`) so its digest can be checked. */
onReadSkillFile: (uri: string) => Promise<SkillFileContents>;
/** Re-fetch the selected entry through `skills/get`. */
onGetSkill: (uri: string) => Promise<SkillEntry>;
}
/** The Tasks monitor: the task list, its progress map, and actions. */

View file

@ -916,6 +916,28 @@ describe("onReadSkillFile (#2234)", () => {
});
});
describe("onGetSkill (#2234)", () => {
it("routes the uri through the client's skills/get", async () => {
const getSkill = vi.fn().mockResolvedValue({
uri: "skill://demo/SKILL.md",
frontmatter: {},
resources: [],
});
const h = harness({ client: client({ getSkill }) });
await expect(
h.api().onGetSkill("skill://demo/SKILL.md"),
).resolves.toMatchObject({ uri: "skill://demo/SKILL.md" });
expect(getSkill).toHaveBeenCalledWith("skill://demo/SKILL.md");
});
it("throws when there is no client", async () => {
const h = harness();
await expect(h.api().onGetSkill("skill://demo/SKILL.md")).rejects.toThrow(
"Client is not connected",
);
});
});
describe("onRefreshSkills (#2234)", () => {
it("drives the store refresh in the background", () => {
const h = harness();

View file

@ -35,6 +35,7 @@ import type {
import type { GetPromptState } from "../components/screens/PromptsScreen/PromptsScreen";
import type { ReadResourceState } from "../components/screens/ResourcesScreen/ResourcesScreen";
import type { SkillFileContents } from "../utils/skillFileBytes";
import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js";
import { UrlElicitationErrorToastMessage } from "../components/elements/Toasts/UrlElicitationErrorToastMessage";
import {
errorCodeOf,
@ -219,6 +220,8 @@ export interface ServerCommands {
* the digest is taken over.
*/
onReadSkillFile: (uri: string) => Promise<SkillFileContents>;
/** Re-fetch one skill entry through `skills/get` (SEP-2640). */
onGetSkill: (uri: string) => Promise<SkillEntry>;
onSubscribeResource: (uri: string) => void;
onUnsubscribeResource: (uri: string) => void;
onCompleteArgument: (
@ -973,6 +976,17 @@ export function useServerCommands({
[onReadResourceContents],
);
// `skills/get` is the extension's second required method, and the Skills tab
// calls it on demand so a server author can see their own handler answer —
// and see whether it agrees with what their `skills/list` advertised.
const onGetSkill = useCallback(
async (uri: string): Promise<SkillEntry> => {
if (!inspectorClient) throw new Error("Client is not connected");
return inspectorClient.getSkill(uri);
},
[inspectorClient],
);
const onRefreshSkills = useCallback(() => {
runCommandInBackground(
() => refreshSkills(),
@ -997,6 +1011,7 @@ export function useServerCommands({
onReadResource,
onReadResourceContents,
onReadSkillFile,
onGetSkill,
onSubscribeResource,
onUnsubscribeResource,
onCompleteArgument,

View file

@ -0,0 +1,85 @@
import { describe, it, expect } from "vitest";
import { sha256Bytes } from "@inspector/core/mcp/sha256";
import { sha256Digest, textToBytes } from "@inspector/core/mcp/skills";
const hex = (bytes: Uint8Array) =>
Array.from(bytes)
.map((b) => b.toString(16).padStart(2, "0"))
.join("");
/**
* The fallback exists because `crypto.subtle` is absent in a non-secure
* context, and the web client is documented as servable over plain HTTP on a
* LAN IP. So it is checked two ways: against the published FIPS 180-4 vectors,
* and differentially against WebCrypto — "it agrees with the real thing" is the
* property that matters, and it is asserted rather than assumed.
*/
describe("sha256Bytes (the non-secure-context fallback)", () => {
it("matches the FIPS 180-4 vector for the empty message", () => {
expect(hex(sha256Bytes(new Uint8Array()))).toBe(
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
);
});
it("matches the FIPS 180-4 vector for 'abc'", () => {
expect(hex(sha256Bytes(textToBytes("abc")))).toBe(
"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad",
);
});
it("matches the FIPS 180-4 two-block vector", () => {
expect(
hex(
sha256Bytes(
textToBytes(
"abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq",
),
),
),
).toBe("248d6a61d20638b8e5c026930c3e6039a33ce45964ff2167f6ecedd419db06c1");
});
it("agrees with WebCrypto across the block boundaries", async () => {
// 55/56/63/64/65 bracket the padding cases: the last block that still fits
// its length field, the one that forces an extra block, and the exact
// multiple of 64.
for (const length of [0, 1, 55, 56, 63, 64, 65, 200, 1000]) {
const bytes = new Uint8Array(length);
for (let i = 0; i < length; i += 1) bytes[i] = (i * 7 + 13) % 256;
const reference = new Uint8Array(
await crypto.subtle.digest("SHA-256", bytes),
);
expect(hex(sha256Bytes(bytes))).toBe(hex(reference));
}
});
it("hashes only the view, not its whole backing buffer", () => {
const backing = new Uint8Array([0xff, ...textToBytes("abc"), 0xff]);
expect(hex(sha256Bytes(backing.subarray(1, 4)))).toBe(
"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad",
);
});
});
describe("sha256Digest without crypto.subtle", () => {
it("falls back rather than throwing, and returns the same digest", async () => {
// Exactly the shape a plain-HTTP LAN page presents: `crypto` exists,
// `crypto.subtle` does not. Before the fallback this threw for every file
// and the UI reported a read failure for a file it had fetched fine.
const withSubtle = await sha256Digest(textToBytes("abc"));
const real = globalThis.crypto;
try {
Object.defineProperty(globalThis, "crypto", {
configurable: true,
value: { getRandomValues: real.getRandomValues.bind(real) },
});
expect(globalThis.crypto.subtle).toBeUndefined();
await expect(sha256Digest(textToBytes("abc"))).resolves.toBe(withSubtle);
} finally {
Object.defineProperty(globalThis, "crypto", {
configurable: true,
value: real,
});
}
});
});

View file

@ -113,6 +113,10 @@ describe("skillNameFromUri", () => {
expect(skillNameFromUri("SKILL.md")).toBeUndefined();
});
it("returns undefined for a non-skill scheme", () => {
expect(skillNameFromUri("https://demo/SKILL.md")).toBeUndefined();
});
it("returns undefined for a relative string", () => {
// SEP-2640 requires a full resource URI; treating `demo/SKILL.md` as one
// would let a non-conforming entry report a name and pass the path check.
@ -141,6 +145,14 @@ describe("normalizeSkillUri", () => {
expect(normalizeSkillUri("demo/SKILL.md")).toBeUndefined();
});
it("rejects a non-skill scheme", () => {
// Checking only that a URI is hierarchical would let this through and then
// pass the name and root checks — a manifest pointing anywhere on the web,
// reported as conforming.
expect(normalizeSkillUri("https://demo/SKILL.md")).toBeUndefined();
expect(normalizeSkillUri("file:///demo/SKILL.md")).toBeUndefined();
});
it("rejects an opaque-path URI, which the parser does not normalize", () => {
// `skill:demo/../x.md` parses but keeps its `..` verbatim, so containment
// could not be decided on it — accepting it would reopen the hole.
@ -229,7 +241,7 @@ describe("checkSkillConformance", () => {
expect(issues[0].severity).toBe("warning");
});
it("reports a manifest entry with no digest as unverifiable", () => {
it("reports a manifest entry with no digest as an error", () => {
const issues = checkSkillConformance(
entry({
resources: [
@ -239,6 +251,7 @@ describe("checkSkillConformance", () => {
}),
);
expect(issues.map((i) => i.code)).toEqual(["missing-digest"]);
expect(issues[0].severity).toBe("error");
expect(issues[0].resourceUri).toBe("skill://demo/ref.md");
});
@ -331,7 +344,7 @@ describe("checkSkillConformance", () => {
expect(issues.map((i) => i.code)).toContain("malformed-uri");
});
it("reports a manifest entry with no size as a warning", () => {
it("reports a manifest entry with no size as an error", () => {
const issues = checkSkillConformance(
entry({
resources: [
@ -341,7 +354,7 @@ describe("checkSkillConformance", () => {
}),
);
expect(issues.map((i) => i.code)).toEqual(["missing-size"]);
expect(issues[0].severity).toBe("warning");
expect(issues[0].severity).toBe("error");
});
it("reports a digest that is not sha256 + 64 lowercase hex", () => {