inspector/core/mcp/skills.ts

/**
 * Skills extension (SEP-2640) detection, conformance checking, and digest
 * verification — the part of the extension that makes the Inspector more than a
 * viewer.
 *
 * SEP-2640 puts real obligations on whoever consumes a skill: verify each
 * fetched file against the digest its manifest advertised, treat a mismatch as
 * a failure, and honor the per-skill limits. Every one of those is a check a
 * server author wants run against their implementation, which is the same
 * argument the CLI's `--strict` tool-schema lint makes. So the checks live here,
 * shared by every client, and produce a structured finding list rather than a
 * boolean — a report is useful, "invalid" is not.
 *
 * ⚠️ Skills is a **server-declared** extension, read off the connecting server's
 * `capabilities.extensions`. It deliberately does NOT belong in
 * `ADVERTISABLE_EXTENSIONS` (`core/mcp/extensions.ts`), which is the catalog of
 * extensions the *Inspector* advertises and the user toggles in Server Settings.
 * The precedent is `appElicitation.ts`, which reads the server side the same
 * way; getting it backwards would put a meaningless toggle in Server Settings.
 *
 * The Inspector is an inspector, not a host: a `resources/read` of a `SKILL.md`
 * is explicitly not a load and confers no standing, so none of the SEP's host
 * machinery (activation, per-skill consent, content-bound approval) is
 * implemented here. Surface and verify.
 *
 * **The frontmatter cross-check closes the gap the digest cannot** (#2248).
 * SEP-2640 requires that an entry's `frontmatter` match the fetched `SKILL.md`'s
 * frontmatter field by field, and no digest can establish that: a digest is
 * taken over the bytes the server served, so it proves the file was not altered
 * in transit and says nothing about whether the *listing* described that file
 * honestly. A server could advertise one description, serve another, and pass
 * every other check in this module. {@link checkSkillFrontmatterMatch} is the
 * check; it needs a real YAML parser, and `core/mcp/skillFile.ts` explains why
 * that dependency is imported rather than approximated.
 */

import type { ServerCapabilities } from "@modelcontextprotocol/client";
import {
  DYNAMIC_RESOURCES,
  SKILLS_EXTENSION_KEY,
  type SkillEntry,
  type SkillResource,
} from "./skillsSchemas.js";
import { sha256Bytes } from "./sha256.js";
import {
  jsonGraphError,
  parseSkillFrontmatter,
  splitSkillFile,
} from "./skillFile.js";

/** Maximum resource entries a single skill may declare (SEP-2640). */
export const SKILL_MAX_RESOURCE_ENTRIES = 512;

/** Maximum total size, in bytes, of a single skill's resources (16 MiB). */
export const SKILL_MAX_TOTAL_BYTES = 16 * 1024 * 1024;

/**
 * Maximum skills one verification run will actually read from, and the byte
 * ceiling across all of them.
 *
 * ⚠️ **Not SEP-2640 limits — they are this tool's own.** The SEP bounds a
 * single skill and deliberately does not bound a catalog: `skills/list` may be
 * arbitrarily long, and a page may hold arbitrarily many entries, so the
 * cursor-walk's page cap constrains nothing here. Every entry costs at least
 * one `resources/read`, so an unbounded catalog is unbounded work and
 * unbounded transfer against the tool sent to inspect it — a `--verify` in CI
 * that never returns (Copilot).
 *
 * Entries past either bound are reported as `incomplete` rather than dropped
 * or failed: they were not checked, which is neither a pass nor a verdict
 * against the server. A host wanting more is not wrong — these are safety
 * limits, not conformance ones — which is why the reason names them.
 */
export const SKILL_MAX_CATALOG_SKILLS = 256;

/** @see {@link SKILL_MAX_CATALOG_SKILLS} — 64 MiB across the whole run. */
export const SKILL_MAX_CATALOG_BYTES = 64 * 1024 * 1024;

/** The suffix every skill URI ends with; the segment before it is the name. */
export const SKILL_FILE_SUFFIX = "/SKILL.md";

/** `sha256:` followed by exactly 64 lowercase hex characters. */
const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/;

/**
 * The Agent Skills name format SEP-2640 requires of `frontmatter.name`: 1–64
 * characters of lowercase alphanumerics and hyphens, with no leading, trailing
 * or consecutive hyphen.
 *
 * Checking only that the name is non-empty let `Bad Name` reach the UI as
 * "Conforms" — and the name is not decorative here: it must equal the URI path
 * segment, so a name that cannot appear in a URI is a contradiction the entry
 * cannot satisfy.
 */
const SKILL_NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/;
const SKILL_NAME_MAX_LENGTH = 64;

/** The Agent Skills limit on `frontmatter.description`. */
const SKILL_DESCRIPTION_MAX_LENGTH = 1024;

/**
 * Length in Unicode **code points**, not UTF-16 code units.
 *
 * `String.prototype.length` counts code units, so every non-BMP character
 * (emoji, many CJK extension characters) counts twice — a perfectly valid
 * 600-character description would be measured as 1200 and reported as
 * `malformed-description`. The Agent Skills limit is in characters, and its
 * reference validator uses Python's `len()`, which counts code points. Getting
 * this wrong fails a conforming server, which is the direction this module
 * works hardest to avoid.
 */
function codePointLength(value: string): number {
  return [...value].length;
}

/**
 * What the server declared under `io.modelcontextprotocol/skills`. The only
 * sub-option SEP-2640 defines is `directoryRead`, which gates
 * `resources/directory/read`.
 */
export interface SkillsExtensionSupport {
  /** True when the server declared `directoryRead: true`. */
  directoryRead: boolean;
}

/**
 * Read the Skills extension off a server's advertised capabilities, or
 * `undefined` when the server did not declare it.
 *
 * Not era-gated, unlike `isTasksExtensionNegotiated()`: `skills/*` are not spec
 * method names in either codec, so nothing about the negotiated era changes
 * whether the extension can be served or called. A legacy-era server that
 * declares it is serving it.
 */
export function getSkillsExtension(
  capabilities: ServerCapabilities | undefined,
): SkillsExtensionSupport | undefined {
  const declared = capabilities?.extensions?.[SKILLS_EXTENSION_KEY];
  // Must be an OBJECT. SEP-2133 declares an extension as an object of
  // sub-options, so a primitive (`false`, `"skills"`) is not a declaration —
  // and treating one as support would show the Skills tab and send
  // `skills/list` to a server that never claimed to serve it. Matches how
  // `appElicitation.ts` parses the UI extension.
  if (typeof declared !== "object" || declared === null) return undefined;
  const directoryRead =
    (declared as { directoryRead?: unknown }).directoryRead === true;
  return { directoryRead };
}

/** True when the connected server declared the Skills extension. */
export function isSkillsExtensionSupported(
  capabilities: ServerCapabilities | undefined,
): boolean {
  return getSkillsExtension(capabilities) !== undefined;
}

/**
 * A skill URI in normalized form, or `undefined` when it is not one.
 *
 * Two things a raw string comparison gets wrong, and both matter:
 *
 * 1. **`..` segments.** `skill://acme/billing/refunds/../other.md` starts with
 *    the advertised root but resolves outside it. Containment has to be decided
 *    on the resolved path, so every check below goes through the parser.
 * 2. **Relative strings.** SEP-2640 requires a full resource URI, and
 *    `demo/SKILL.md` is not one — it fails to parse and is reported as
 *    `malformed-uri` rather than quietly treated as a skill path.
 *
 * An **opaque-path** URI (`skill:demo/SKILL.md`, no authority) parses but is
 * NOT normalized by the parser — its `..` segments survive verbatim — so it is
 * rejected too: containment could not be decided on it, and silently accepting
 * one would reintroduce exactly the hole this function closes.
 *
 * A **path-less authority** URI (`skill://data-analysis`) is accepted, and is
 * not the opaque case above: RFC 3986 gives a URI with an authority an empty
 * path, and the parser reports that as `pathname === ""`. The authority has to
 * be non-empty — `skill:` parses to an empty path too, with no host. It has no segments,
 * so nothing survives unnormalized. It is what a skill's root directory looks
 * like once `/SKILL.md` is removed, so rejecting it made a server echoing that
 * root back in a directory read read as "outside this skill" when it is the
 * skill itself (#2295). It names no file, so it still fails every check that
 * needs one — `skillNameFromUri` wants the `/SKILL.md` suffix, and a manifest
 * entry spelled this way does not start with `<root>/`.
 *
 * The scheme is deliberately **not** constrained. `skill://` is what SEP-2640
 * recommends and what this repo's fixture serves, but the SEP only says servers
 * SHOULD use it and explicitly allows a domain-native scheme (`github://…`), so
 * requiring `skill:` would hand a conforming server a false `malformed-uri` and
 * skip its name and root checks entirely. Containment is scheme-independent
 * anyway: it compares a resource against *this entry's own* root, so an entry
 * cannot escape its skill whatever scheme it uses.
 */
export function normalizeSkillUri(uri: string): string | undefined {
  let parsed: URL;
  try {
    parsed = new URL(uri);
  } catch {
    return undefined;
  }
  // An empty path is only the root form when an authority carries the identity:
  // `skill:` and `mailto:` also parse to `pathname === ""`, with no host.
  const pathlessRoot = parsed.pathname === "" && parsed.host !== "";
  if (!pathlessRoot && !parsed.pathname.startsWith("/")) return undefined;
  return canonicalizePercentEncoding(parsed.href);
}

/**
 * The two percent-encoding normalizations RFC 3986 §6.2.2 calls for and the
 * URL parser does **not** do: decode an escape that stands for an *unreserved*
 * character, and upper-case the hex of every escape that remains.
 *
 * `URL.href` leaves `%72eference.md` encoded, so without this a server echoing
 * an RFC-equivalent form of the URI we asked for would be rejected by
 * `onReadSkillFile` as a different resource, and an encoded skill-name segment
 * would produce a false `name-path-mismatch`. Both are the tool calling a
 * conforming server wrong, which is the failure mode this module works hardest
 * to avoid.
 */
function canonicalizePercentEncoding(value: string): string {
  return value.replace(/%[0-9a-fA-F]{2}/g, (escape) => {
    const char = String.fromCharCode(Number.parseInt(escape.slice(1), 16));
    return /[A-Za-z0-9\-._~]/.test(char) ? char : escape.toUpperCase();
  });
}

/**
 * The final `<skill-path>` segment of a skill URI — the segment *before*
 * `/SKILL.md`, not the filename. SEP-2640 requires it to equal
 * `frontmatter.name`, which is what makes a skill's name recoverable from its
 * URI alone. Returns `undefined` when the URI does not have that shape, which
 * is itself a conformance finding.
 *
 * Read off the **normalized** URI, so a traversal segment cannot produce a
 * name the resolved path does not actually carry.
 */
export function skillNameFromUri(uri: string): string | undefined {
  const normalized = normalizeSkillUri(uri);
  if (normalized === undefined || !normalized.endsWith(SKILL_FILE_SUFFIX)) {
    return undefined;
  }
  const path = normalized.slice(0, -SKILL_FILE_SUFFIX.length);
  const segment = path.slice(path.lastIndexOf("/") + 1);
  return segment.length > 0 ? segment : undefined;
}

/**
 * The comparison identity of a resource URI: its normalized form, falling back
 * to the raw string when it does not parse.
 *
 * Every URI comparison in this module and in the screen goes through this, so
 * they cannot disagree about whether two spellings name the same file. The raw
 * fallback is deliberate: two *different* unparseable URIs must stay distinct
 * rather than both collapsing to one `undefined` identity.
 */
export function skillUriIdentity(uri: string): string {
  return normalizeSkillUri(uri) ?? uri;
}

/**
 * The label a UI shows for a skill: the declared name, falling back to the URI
 * path segment, falling back to the raw URI. Never empty, so a list row is
 * always addressable even for a badly non-conforming entry.
 */
export function skillDisplayName(entry: SkillEntry): string {
  const declared = entry.frontmatter.name?.trim();
  if (declared) return declared;
  return skillNameFromUri(entry.uri) ?? entry.uri;
}

/** Machine-readable identity of a conformance finding. */
export type SkillIssueCode =
  | "dynamic-resources"
  | "missing-name"
  | "malformed-name"
  | "missing-description"
  | "malformed-description"
  | "malformed-uri"
  | "name-path-mismatch"
  | "missing-digest"
  | "malformed-digest"
  | "missing-size"
  | "malformed-size"
  | "duplicate-resource"
  | "resource-outside-skill-root"
  | "manifest-missing-self"
  | "resource-limit-exceeded"
  | "size-limit-exceeded"
  | "frontmatter-absent"
  | "frontmatter-unparsable"
  | "frontmatter-mismatch"
  | "duplicate-name";

/**
 * `error` marks a **MUST** of SEP-2640 that the server broke, so a manifest
 * reporting "0 errors" really is one the spec accepts. `warning` covers
 * everything the spec permits but a consumer still wants told about: the
 * `SHOULD NOT`-exceed interoperability limits, and — above all — `"dynamic"`
 * resources, which are legal and leave integrity unverifiable, the case most
 * worth surfacing and the one most easily buried.
 */
export type SkillIssueSeverity = "error" | "warning";

export interface SkillIssue {
  code: SkillIssueCode;
  severity: SkillIssueSeverity;
  /** Human-readable statement of what is wrong. */
  message: string;
  /** The manifest entry the finding is about, when it is a per-file finding. */
  resourceUri?: string;
}

/**
 * Run every structural check SEP-2640 states against one skill entry, returning
 * the findings in a stable order (skill-level first, then per-resource in
 * manifest order). An empty array means the entry conforms.
 *
 * This is the static half. Digest *verification* needs the file's bytes and so
 * lives in {@link verifySkillResource}, which the UI runs on demand.
 */
export function checkSkillConformance(entry: SkillEntry): SkillIssue[] {
  const issues: SkillIssue[] = [];
  // The RAW value is what the grammar is applied to — trimming first would let
  // `" demo "` pass, and whitespace is not in the Agent Skills name grammar.
  // The trimmed copy exists only to tell "absent" from "present but invalid".
  const rawName = entry.frontmatter.name;
  const declaredName = rawName?.trim() ? rawName : undefined;
  const uriName = skillNameFromUri(entry.uri);

  if (!declaredName) {
    issues.push({
      code: "missing-name",
      severity: "error",
      message: "frontmatter.name is required but missing or empty.",
    });
  } else if (
    codePointLength(declaredName) > SKILL_NAME_MAX_LENGTH ||
    !SKILL_NAME_PATTERN.test(declaredName)
  ) {
    // Reaches here for `" demo "` too: the grammar sees the untrimmed value.
    issues.push({
      code: "malformed-name",
      severity: "error",
      message: `frontmatter.name "${declaredName}" is not a valid Agent Skills name: 1–${SKILL_NAME_MAX_LENGTH} lowercase alphanumerics and hyphens, with no leading, trailing or consecutive hyphen.`,
    });
  }
  const rawDescription = entry.frontmatter.description;
  if (!rawDescription?.trim()) {
    // An error, not a warning: SEP-2640 requires `description` on every skill,
    // so an absent one is a format violation and must not read as "0 errors".
    issues.push({
      code: "missing-description",
      severity: "error",
      message: "frontmatter.description is required but missing or empty.",
    });
  } else if (codePointLength(rawDescription) > SKILL_DESCRIPTION_MAX_LENGTH) {
    issues.push({
      code: "malformed-description",
      severity: "error",
      message: `frontmatter.description is ${codePointLength(rawDescription)} characters, above the ${SKILL_DESCRIPTION_MAX_LENGTH}-character limit.`,
    });
  }
  if (uriName === undefined) {
    issues.push({
      code: "malformed-uri",
      severity: "error",
      message: `Skill URI must be a hierarchical URI ending with "${SKILL_FILE_SUFFIX}" and carrying a non-empty path segment before it.`,
    });
  } else if (declaredName && uriName !== declaredName) {
    // The one structural invariant the spec states outright: the segment before
    // /SKILL.md must equal frontmatter.name, so the name is recoverable from
    // the URI alone. Only checked when both halves exist — a missing name is
    // already reported above, and reporting it twice reads as two defects.
    issues.push({
      code: "name-path-mismatch",
      severity: "error",
      message: `URI path segment "${uriName}" does not match frontmatter.name "${declaredName}".`,
    });
  }

  if (entry.resources === DYNAMIC_RESOURCES) {
    issues.push({
      code: "dynamic-resources",
      severity: "warning",
      message:
        'resources is "dynamic": the file set is generated, so no digest is advertised and integrity cannot be verified.',
    });
    return issues;
  }

  // Both limits are *interoperability* bounds, not MUSTs: SEP-2640 says a
  // server SHOULD NOT exceed them and a host MAY support more. So they are
  // warnings — calling a permitted oversized skill an error would contradict
  // what `error` means here and tell a server author their skill is invalid
  // when it is merely less portable.
  if (entry.resources.length > SKILL_MAX_RESOURCE_ENTRIES) {
    issues.push({
      code: "resource-limit-exceeded",
      severity: "warning",
      message: `Skill declares ${entry.resources.length} resource entries, above the ${SKILL_MAX_RESOURCE_ENTRIES}-entry interoperability limit; a host is only required to support up to it.`,
    });
  }
  const totalBytes = totalSkillBytes(entry.resources);
  if (totalBytes > SKILL_MAX_TOTAL_BYTES) {
    issues.push({
      code: "size-limit-exceeded",
      severity: "warning",
      message: `Skill resources total ${totalBytes} bytes, above the ${SKILL_MAX_TOTAL_BYTES}-byte (16 MiB) interoperability limit; a host is only required to support up to it.`,
    });
  }

  // A manifest is the *complete* file set, and the skill's own SKILL.md is one
  // of those files. An empty list, or one that omits the entry's own URI, is
  // therefore not "a skill with no extra files" — it is a manifest that cannot
  // be checked against what the skill actually is, and reporting `Conforms`
  // for it would be a wrong answer rather than a missing one.
  // Compared on the normalized identity, like every other URI comparison here:
  // a manifest listing the RFC-equivalent `skill://demo/%53KILL.md` names the
  // same file the entry does, and is fetchable as that file, so calling it a
  // missing self-entry would be the tool disagreeing with itself.
  const entryIdentity = skillUriIdentity(entry.uri);
  if (
    !entry.resources.some(
      (resource) => skillUriIdentity(resource.uri) === entryIdentity,
    )
  ) {
    issues.push({
      code: "manifest-missing-self",
      severity: "error",
      message: `Manifest does not list the skill's own entry file (${entry.uri}); a manifest must be the complete file set.`,
    });
  }

  const seenUris = new Set<string>();
  // Relative references resolve against the skill root, so every manifest entry
  // must live under it. A URI outside that prefix is either a typo or a server
  // claiming integrity over a file that is not part of this skill. Computed
  // from the NORMALIZED entry URI, and compared against normalized resource
  // URIs, so a `..` segment cannot walk out of the root while still matching it
  // as a string. Left `undefined` for a malformed entry URI — there is no root
  // to measure against, and `malformed-uri` already reports that.
  const normalizedEntryUri = normalizeSkillUri(entry.uri);
  const root =
    normalizedEntryUri !== undefined &&
    normalizedEntryUri.endsWith(SKILL_FILE_SUFFIX)
      ? `${normalizedEntryUri.slice(0, -SKILL_FILE_SUFFIX.length)}/`
      : undefined;

  for (const resource of entry.resources) {
    // Compared on the NORMALIZED identity, because everything else here treats
    // normalized-equivalents as the same resource — containment does, and so
    // does the read that fetches the bytes. On the raw string,
    // `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md` would pass as
    // two distinct files while naming one. The raw URI is still what the
    // finding reports, so the diagnostic points at what the server actually
    // sent. Unparseable URIs fall back to the raw string: they are already
    // reported by the root check, and normalizing them all to `undefined`
    // would make two different bad URIs look like one duplicate.
    const identity = skillUriIdentity(resource.uri);
    if (seenUris.has(identity)) {
      issues.push({
        code: "duplicate-resource",
        severity: "error",
        message:
          "Manifest lists this URI more than once; entries must be unique.",
        resourceUri: resource.uri,
      });
    }
    seenUris.add(identity);
    if (root !== undefined) {
      const normalized = normalizeSkillUri(resource.uri);
      // An unparseable entry URI is outside the root by construction: nothing
      // can establish that it is inside one.
      if (normalized === undefined || !normalized.startsWith(root)) {
        issues.push({
          code: "resource-outside-skill-root",
          severity: "error",
          message: `Manifest entry does not resolve inside the skill root "${root}".`,
          resourceUri: resource.uri,
        });
      }
    }
    if (resource.digest === undefined) {
      // An error, not a warning: SEP-2640 requires `digest` on every manifest
      // entry, so an entry without one is invalid — and reporting it as a
      // warning would let such a manifest show "0 errors", which is the
      // affirmative pass this checker must never give.
      issues.push({
        code: "missing-digest",
        severity: "error",
        message:
          "Manifest entry declares no digest, which is required — and without it the file cannot be verified.",
        resourceUri: resource.uri,
      });
    } else if (!DIGEST_PATTERN.test(resource.digest)) {
      issues.push({
        code: "malformed-digest",
        severity: "error",
        message: `Digest "${resource.digest}" is not "sha256:" followed by 64 lowercase hex characters.`,
        resourceUri: resource.uri,
      });
    }
    if (resource.size === undefined) {
      // Also an error: `size` is a required field, not an integrity hint, and
      // an omitted one is what lets a server slip past the 16 MiB pre-fetch
      // limit — the entry is excluded from the total — while the UI reports no
      // conformance errors at all.
      issues.push({
        code: "missing-size",
        severity: "error",
        message:
          "Manifest entry declares no size, which is required — and without it the entry is excluded from the 16 MiB total and its length cannot be cross-checked.",
        resourceUri: resource.uri,
      });
    } else if (!isUsableSize(resource.size)) {
      issues.push({
        code: "malformed-size",
        severity: "error",
        message: `Size ${resource.size} is not a non-negative integer byte length.`,
        resourceUri: resource.uri,
      });
    }
  }

  return issues;
}

/**
 * Whether a declared `size` is a usable byte length. SEP-2640 defines it as the
 * raw byte count, so anything that is not a non-negative safe integer is
 * nonsense — and a *negative* one is worse than nonsense, because summing it
 * would pull the manifest total back under the 16 MiB limit and hide a
 * violation. Reported as `malformed-size` and excluded from the sum.
 */
function isUsableSize(size: number | undefined): size is number {
  return size !== undefined && Number.isSafeInteger(size) && size >= 0;
}

/**
 * Sum of the manifest's declared `size` fields. An entry that omits `size` — or
 * declares an unusable one — contributes nothing rather than failing the sum:
 * the limit check is about catching a server that is demonstrably over, and an
 * incomplete manifest can only ever understate the total, so this never
 * produces a false positive.
 */
export function totalSkillBytes(resources: readonly SkillResource[]): number {
  return resources.reduce(
    (sum, r) => sum + (isUsableSize(r.size) ? r.size : 0),
    0,
  );
}

/**
 * A stable key for "this exact entry", safe against a hostile listing.
 *
 * `JSON.stringify(entry)` is the obvious implementation and is the wrong one:
 * `frontmatter` is unbounded server-controlled JSON, so a deep enough object
 * throws `RangeError: Maximum call stack size exceeded` — and this is evaluated
 * during render, so one catalog entry could crash the pane that exists to
 * report on it (Copilot).
 *
 * The same guard that bounds the frontmatter comparison decides it here. When
 * the entry is representable the key is its serialization, which is exact;
 * when it is not, the key falls back to the entry's identity plus its manifest
 * length. That fallback is deliberately coarse — such an entry already carries
 * a `frontmatter-unparsable` error, so what matters is that it produces a
 * usable key rather than a precise one.
 */
export function skillEntryKey(entry: SkillEntry): string {
  if (jsonGraphError(entry) !== undefined) {
    const count = Array.isArray(entry.resources) ? entry.resources.length : -1;
    return `${skillUriIdentity(entry.uri)}#unrepresentable:${count}`;
  }
  return JSON.stringify(entry);
}

/**
 * Whether a `skills/get` entry describes the same skill as the `skills/list`
 * entry alongside it, compared **semantically** rather than byte-for-byte.
 *
 * Two things a `JSON.stringify` comparison gets wrong here, and both would
 * report a conforming server as broken: object key order is not meaningful in
 * JSON, and the resource manifest is a *set*, so a server free to enumerate it
 * in any order would look inconsistent for reordering it. Both sides are
 * canonicalized — keys sorted recursively, manifest entries sorted by URI —
 * before they are compared.
 *
 * A difference is still worth showing, but it is NOT by itself an error:
 * SEP-2640 defines `skills/get` as a fresh point-in-time snapshot, so a skill
 * that genuinely changed since the listing legitimately differs. The caller
 * presents it as "the snapshot moved" and leaves the judgement to the reader.
 */
export function skillEntriesMatch(a: SkillEntry, b: SkillEntry): boolean {
  return canonicalEntry(a) === canonicalEntry(b);
}

/**
 * Deterministic JSON for one entry: object keys sorted recursively, and **the
 * entry's own manifest** — nothing else — sorted by URI.
 *
 * The manifest sort is deliberately not recursive. `frontmatter` is verbatim
 * arbitrary JSON from the skill author, so a custom `frontmatter.metadata.
 * resources` array would be caught by a recursive rule and two genuinely
 * different frontmatters would compare equal. Only `SkillEntry.resources` is
 * a set; every other array keeps its order.
 */
function canonicalEntry(entry: SkillEntry): string {
  const { resources, uri, ...rest } = entry;
  // URIs are compared by IDENTITY, so a server that canonicalizes an escape
  // between the listing and the fetch is not reported as a changed snapshot.
  const manifest = Array.isArray(resources)
    ? [...resources]
        .map((resource) => ({
          ...resource,
          uri: skillUriIdentity(String(resource?.uri)),
        }))
        .sort((x, y) => x.uri.localeCompare(y.uri))
        .map(canonicalize)
    : resources;
  return JSON.stringify({
    ...sortKeys(rest),
    uri: skillUriIdentity(uri),
    resources: manifest,
  });
}

/** Object keys sorted recursively; array ORDER is preserved throughout. */
function canonicalize(value: unknown): unknown {
  if (Array.isArray(value)) return value.map(canonicalize);
  if (value === null || typeof value !== "object") return value;
  return sortKeys(value as Record<string, unknown>);
}

function sortKeys(value: Record<string, unknown>): Record<string, unknown> {
  return Object.fromEntries(
    Object.entries(value)
      .map(([key, member]): [string, unknown] => [key, canonicalize(member)])
      .sort(([x], [y]) => x.localeCompare(y)),
  );
}

/** Outcome of comparing a fetched file against its advertised digest. */
export type SkillVerificationStatus =
  | "verified"
  | "mismatch"
  | "unverifiable"
  | "error";

export interface SkillVerification {
  status: SkillVerificationStatus;
  /** The digest computed over the fetched bytes, when one was computed. */
  actualDigest?: string;
  /** The manifest's digest, echoed so a mismatch renders both halves. */
  expectedDigest?: string;
  /** The manifest's declared byte length, when it declared one. */
  expectedSize?: number;
  /** The fetched file's actual byte length, when it was measured. */
  actualSize?: number;
  /** Why the file could not be verified or fetched. */
  reason?: string;
}

/** Lowercase hex of a byte array — the form SEP-2640 digests are written in. */
function toHex(bytes: Uint8Array): string {
  let out = "";
  for (const byte of bytes) out += byte.toString(16).padStart(2, "0");
  return out;
}

/**
 * `sha256:<64 hex>` over the given bytes, in the exact form a manifest digest
 * takes, so a caller can compare strings rather than re-deriving the prefix.
 *
 * Uses WebCrypto (`crypto.subtle`), which both Node ≥22 and the browser provide
 * — no dependency, and per [Dependency placement] this module adds nothing to
 * any manifest. The Inspector's web client is served over localhost, a secure
 * context, so `subtle` is present there too.
 */
export async function sha256Digest(bytes: Uint8Array): Promise<string> {
  // `crypto.subtle` is exposed only in a SECURE CONTEXT, and this app is
  // documented as servable over plain HTTP on a LAN IP
  // (`clients/web/README.md#hosting-on-a-network`). There, `crypto` exists but
  // `crypto.subtle` does not — so without this fallback every verification
  // would throw and the UI would report a read failure for a file it fetched
  // perfectly well. `sha256Bytes` is checked against the published FIPS 180-4
  // vectors and differentially against WebCrypto, so the two paths agree.
  const subtle = globalThis.crypto?.subtle;
  if (!subtle) return `sha256:${toHex(sha256Bytes(bytes))}`;
  // Copy the VIEW into a fresh typed array rather than slicing its backing
  // store. Two things depend on that: a `Uint8Array` can be a window into a
  // larger buffer, so hashing the buffer would digest neighbouring bytes; and
  // `SharedArrayBuffer.prototype.slice()` returns another `SharedArrayBuffer`,
  // which `crypto.subtle.digest` rejects — so slicing-and-casting would have
  // failed at runtime for the exact input a cast claimed to handle.
  // `new Uint8Array(view)` always allocates a plain `ArrayBuffer`, which is
  // also why no cast is needed here.
  const copy = new Uint8Array(bytes);
  const hash = await subtle.digest("SHA-256", copy.buffer);
  return `sha256:${toHex(new Uint8Array(hash))}`;
}

/** UTF-8 bytes of a `resources/read` text content block. */
export function textToBytes(text: string): Uint8Array {
  return new TextEncoder().encode(text);
}

/**
 * A skill file's bytes as UTF-8 text — the inverse of {@link textToBytes}.
 *
 * Deliberately **non-fatal**: a `SKILL.md` that is not valid UTF-8 decodes with
 * replacement characters rather than throwing. That is the more useful failure,
 * because the frontmatter comparison then reports a concrete difference between
 * what the listing claimed and what the file actually holds, instead of
 * collapsing into "could not decode" and skipping the check the SEP makes
 * mandatory.
 */
export function bytesToText(bytes: Uint8Array): string {
  return new TextDecoder().decode(bytes);
}

/**
 * Raw bytes of a `resources/read` blob content block (standard base64).
 * Uses `atob`, which Node ≥22 and every browser provide, so this stays
 * dependency-free and works unchanged in both.
 */
export function base64ToBytes(blob: string): Uint8Array {
  const binary = atob(blob);
  const bytes = new Uint8Array(binary.length);
  for (let i = 0; i < binary.length; i += 1) bytes[i] = binary.charCodeAt(i);
  return bytes;
}

/**
 * The content a `resources/read` returned for one skill file. Either `text` (a
 * `TextResourceContents`) or `blob` (base64, a `BlobResourceContents`).
 */
export interface SkillFileContents {
  text?: string;
  blob?: string;
  mimeType?: string;
}

/**
 * The raw bytes of a skill file, as fetched — the bytes its digest was taken
 * over.
 *
 * Throws for a result carrying neither `text` nor `blob`. That is a server bug,
 * and it must not be quietly treated as empty content: an empty `Uint8Array`
 * has a perfectly good SHA-256, so a silent fallback would report a *digest
 * mismatch* — a confident, wrong diagnosis — instead of "this response carried
 * no content at all". Callers surface the throw as a per-file read failure.
 */
export function skillFileBytes(contents: SkillFileContents): Uint8Array {
  if (typeof contents.text === "string") return textToBytes(contents.text);
  if (typeof contents.blob === "string") return base64ToBytes(contents.blob);
  throw new Error("resources/read returned neither text nor blob content.");
}

/**
 * Verify one fetched skill file against its manifest entry.
 *
 * A mismatch is reported as `"mismatch"` with both digests attached rather than
 * thrown — the whole value proposition is showing a digest mismatch loudly, and
 * a thrown error would collapse into whatever the caller's generic failure UI
 * says. `"unverifiable"` means the manifest advertised no digest (or advertised
 * a malformed one, already reported by {@link checkSkillConformance}); nothing
 * about the file itself is wrong, we simply have nothing to compare against.
 *
 * The declared `size` is cross-checked **before** the digest and fails
 * verification on its own. A length that disagrees with the manifest is a real
 * inconsistency even when the digest matches — the digest is taken over the
 * bytes the server served, so agreeing with it says nothing about whether the
 * manifest describes those bytes — and it is the cheaper check, so a
 * 16 MiB file that was never going to verify is not hashed first.
 */
export async function verifySkillResource(
  resource: SkillResource,
  bytes: Uint8Array,
): Promise<SkillVerification> {
  const expectedSize = resource.size;
  if (expectedSize !== undefined && expectedSize !== bytes.byteLength) {
    return {
      status: "mismatch",
      expectedSize,
      actualSize: bytes.byteLength,
      ...(resource.digest !== undefined
        ? { expectedDigest: resource.digest }
        : {}),
      reason: `Manifest declares ${expectedSize} bytes but the fetched file is ${bytes.byteLength}.`,
    };
  }
  const expectedDigest = resource.digest;
  if (expectedDigest === undefined) {
    return {
      status: "unverifiable",
      reason: "The manifest entry advertises no digest.",
    };
  }
  if (!DIGEST_PATTERN.test(expectedDigest)) {
    return {
      status: "unverifiable",
      expectedDigest,
      reason:
        'The advertised digest is not "sha256:" followed by 64 lowercase hex characters.',
    };
  }
  const actualDigest = await sha256Digest(bytes);
  return {
    status: actualDigest === expectedDigest ? "verified" : "mismatch",
    actualDigest,
    expectedDigest,
    ...(expectedSize !== undefined
      ? { expectedSize, actualSize: bytes.byteLength }
      : {}),
  };
}

/**
 * Structural equality for JSON-like values, with YAML's extra scalars handled.
 *
 * ⚠️ **Comparison is structural rather than serialized, and that is the point.**
 * `JSON.stringify` is not injective over what a YAML parser produces: `.nan`,
 * `.inf` and `-.inf` all serialize to `null`, so a served `x: .nan` compared
 * EQUAL to a listing declaring `x: null` — and to each other. An earlier fix
 * encoded non-finite numbers as a sentinel object, which merely moved the
 * problem: a listing whose value genuinely *was* that object aliased the
 * sentinel and matched a served `.nan` (Copilot). Any encoding into the value
 * space can be aliased by a document containing the encoding, so there is no
 * sentinel here at all.
 *
 * `Object.is` on the number path is what makes it work: it holds `NaN` equal to
 * `NaN`, keeps `Infinity` and `-Infinity` distinct, and never equates either
 * with `null`.
 */
function jsonLikeEqual(a: unknown, b: unknown): boolean {
  if (typeof a === "number" || typeof b === "number") {
    // `===` for finite numbers, `Object.is` only for the non-finite ones.
    //
    // `Object.is` alone held `0` and `-0` distinct, so a listing carrying JSON
    // `0` against a served YAML `-0` produced a mismatch — reported as "the
    // listing says 0 but the served SKILL.md says 0", a false finding with an
    // unintelligible explanation (Copilot). JSON does not distinguish them
    // (`JSON.stringify(-0)` is `"0"`), so neither may this. `===` would in turn
    // hold `NaN` unequal to itself, which is why the two are combined rather
    // than either used alone.
    if (typeof a === "number" && typeof b === "number") {
      return Number.isFinite(a) && Number.isFinite(b)
        ? a === b
        : Object.is(a, b);
    }
    // One side is not a number at all: different types, never equal.
    return false;
  }
  if (a === null || b === null) return a === b;
  if (typeof a !== "object" || typeof b !== "object") return Object.is(a, b);
  const aArray = Array.isArray(a);
  if (aArray !== Array.isArray(b)) return false;
  if (aArray) {
    const x = a as unknown[];
    const y = b as unknown[];
    // Array ORDER is significant — a YAML sequence is ordered.
    return x.length === y.length && x.every((v, i) => jsonLikeEqual(v, y[i]));
  }
  const x = a as Record<string, unknown>;
  const y = b as Record<string, unknown>;
  const keys = Object.keys(x);
  // Key order is not meaningful in either JSON or YAML, so only the key SET and
  // the values matter.
  return (
    keys.length === Object.keys(y).length &&
    keys.every((k) => Object.hasOwn(y, k) && jsonLikeEqual(x[k], y[k]))
  );
}

/**
 * A frontmatter value as it should READ in a finding.
 *
 * `JSON.stringify` renders every non-finite number as `null`, which would print
 * "the listing says null but the served file says null" for a real difference.
 * Only the display is special-cased; the comparison above never goes through a
 * string, so this cannot reintroduce an aliasing bug.
 */
function displayValue(value: unknown): string {
  if (typeof value === "number" && !Number.isFinite(value)) {
    return String(value);
  }
  return JSON.stringify(canonicalize(value), (_key, member: unknown) =>
    typeof member === "number" && !Number.isFinite(member)
      ? `<${String(member)}>`
      : member,
  );
}

/**
 * Compare the fetched `SKILL.md`'s own frontmatter against the frontmatter the
 * entry advertised, field by field — the SEP-2640 obligation a digest cannot
 * discharge (#2248).
 *
 * SEP-2640: *"hosts MUST parse its YAML frontmatter and compare it
 * field-by-field against the entry's `frontmatter`. Any discrepancy MUST be
 * treated as a verification failure equivalent to a digest mismatch"*. So every
 * finding here is an `error`, matching what a digest mismatch reports — the
 * spec makes them equivalent and the report must not rank one below the other.
 *
 * **One finding per differing field, not one per file.** "Frontmatter does not
 * match" is unactionable for the server author who has to fix it; "listing says
 * `description: A`, file says `description: B`" is the whole diagnosis. The
 * union of both sides' keys is walked, so a field present on only one side is
 * reported as such rather than silently skipped.
 *
 * ⚠️ **Only call this with the bytes of the entry's own `SKILL.md`.** The check
 * is meaningless against a supporting file, which has no frontmatter to match,
 * and would report every one of them as `frontmatter-absent`. Callers select
 * the file; this function cannot tell which one it was handed.
 *
 * Values are compared as **canonical JSON**, so a frontmatter field holding a
 * nested mapping compares equal when the two sides agree on content and differ
 * only in key order — which is not a discrepancy in either JSON or YAML. Array
 * order *is* significant and is preserved, because a YAML sequence is ordered.
 */
export function checkSkillFrontmatterMatch(
  entry: SkillEntry,
  skillFileText: string,
): SkillIssue[] {
  const { frontmatter } = splitSkillFile(skillFileText);
  if (frontmatter === undefined) {
    return [
      {
        code: "frontmatter-absent",
        severity: "error",
        message:
          "The served SKILL.md carries no YAML frontmatter block, so the listing's frontmatter cannot be the file's.",
        resourceUri: entry.uri,
      },
    ];
  }
  // The LISTING side is bounded too, before anything recurses over it. It
  // arrives over JSON-RPC so it cannot be cyclic, but it is just as unbounded
  // in depth — and both `jsonLikeEqual` and `displayValue` walk it, so a
  // server advertising an absurdly nested value crashed the tool exactly as a
  // cyclic served one did (Copilot). Checked before the file is parsed: there
  // is no point reading one if the thing to compare it against is unusable.
  const listedError = jsonGraphError(entry.frontmatter);
  if (listedError) {
    return [
      {
        code: "frontmatter-unparsable",
        severity: "error",
        message: `The listing's own frontmatter cannot be compared: ${listedError}`,
        resourceUri: entry.uri,
      },
    ];
  }
  const parsed = parseSkillFrontmatter(frontmatter);
  if ("error" in parsed) {
    return [
      {
        code: "frontmatter-unparsable",
        severity: "error",
        message: `The served SKILL.md's frontmatter is not valid YAML: ${parsed.error}`,
        resourceUri: entry.uri,
      },
    ];
  }
  const issues: SkillIssue[] = [];
  // Sorted so the report is stable across runs — `Object.keys` order follows
  // insertion, which is the wire order on one side and the file order on the
  // other, and those need not agree even when the content does.
  const fields = [
    ...new Set([
      ...Object.keys(entry.frontmatter),
      ...Object.keys(parsed.fields),
    ]),
  ].sort();
  for (const field of fields) {
    const listed = entry.frontmatter[field];
    const served = parsed.fields[field];
    // `undefined` is the only way "absent" reaches here: JSON has no undefined
    // value, and a YAML key written with an empty value parses to `null`, which
    // is a present field holding null and compares as one.
    if (listed === undefined) {
      issues.push({
        code: "frontmatter-mismatch",
        severity: "error",
        message: `The served SKILL.md declares "${field}" but the listing's frontmatter omits it.`,
        resourceUri: entry.uri,
      });
      continue;
    }
    if (served === undefined) {
      issues.push({
        code: "frontmatter-mismatch",
        severity: "error",
        message: `The listing declares "${field}" but the served SKILL.md omits it.`,
        resourceUri: entry.uri,
      });
      continue;
    }
    if (!jsonLikeEqual(listed, served)) {
      issues.push({
        code: "frontmatter-mismatch",
        severity: "error",
        message: `Field "${field}" differs: the listing says ${displayValue(listed)} but the served SKILL.md says ${displayValue(served)}.`,
        resourceUri: entry.uri,
      });
    }
  }
  return issues;
}

/**
 * How many colliding URIs a `duplicate-name` message names before it counts the
 * rest. Three is enough to show the shape of the collision; the count carries
 * the scale.
 */
const COLLISION_SAMPLE = 3;

/**
 * Findings that can only be computed over the **whole listing**, keyed by the
 * entry they belong to (its normalized URI identity).
 *
 * Today that is exactly one: two entries in a single `skills/list` colliding on
 * `frontmatter.name`. {@link checkSkillConformance} structurally cannot report
 * it — it sees one entry at a time, and a collision is a property of the pair.
 *
 * SEP-2640: *"Hosts MUST NOT assume name uniqueness"*, and *"When two entries
 * in one listing collide on `name`, hosts MUST disambiguate them — for example
 * by their distinguishing path segments — rather than silently discarding or
 * preferring one."*
 *
 * ⚠️ **A collision is a `warning`, not an `error`, and the distinction is the
 * whole point of the severity split.** The obligation here is on the *host*,
 * not the server: a server may legitimately publish two skills with the same
 * name under different paths, and the SEP's own example
 * (`acme/billing/refunds`) is exactly that. Reporting it as an error would tell
 * a conforming server author their catalog is invalid. What the warning says is
 * that a consumer must not collapse the two — which is why the Inspector shows
 * each skill's URI beside its name, and now says so rather than leaving the
 * reader to notice.
 *
 * Names are compared **raw**, not trimmed or case-folded. The Agent Skills
 * grammar is lowercase already, and a checker that normalized more than the
 * grammar does would report a collision between two names the spec considers
 * distinct.
 */
export function checkSkillNameCollisions(
  entries: readonly SkillEntry[],
): Map<string, SkillIssue> {
  const byName = new Map<string, SkillEntry[]>();
  for (const entry of entries) {
    const name = entry.frontmatter.name;
    // An absent name is `missing-name`, reported per entry. Two entries that
    // both omit one are not "colliding on a name" — there is no name — and
    // saying so would bury the real finding under a derived one.
    if (typeof name !== "string" || name.trim() === "") continue;
    const group = byName.get(name);
    if (group) group.push(entry);
    else byName.set(name, [entry]);
  }

  const issues = new Map<string, SkillIssue>();
  for (const [name, group] of byName) {
    // Deduplicated by URI identity first: the SAME skill appearing twice in a
    // listing is a repeated entry, not two skills sharing a name, and
    // `skills/list` returning it twice is a different defect from the one this
    // function reports.
    const identities = new Set(group.map((e) => skillUriIdentity(e.uri)));
    if (identities.size < 2) continue;
    const uris = [...identities].sort();
    for (const entry of group) {
      const self = skillUriIdentity(entry.uri);
      // ⚠️ A bounded SAMPLE, taken with an early exit — not
      // `uris.filter(...)` and not the whole list in the message. Duplicate
      // names are legal and SEP-2640 puts no ceiling on a catalog, so a group
      // of N made both the work and the generated text O(N²): every one of N
      // entries scanned all N URIs and embedded the other N−1 (Copilot). A
      // server controls N, which turns a legal listing into a denial of
      // service against the tool meant to inspect it.
      const sample: string[] = [];
      for (const uri of uris) {
        if (uri === self) continue;
        sample.push(uri);
        if (sample.length === COLLISION_SAMPLE) break;
      }
      const unshown = identities.size - 1 - sample.length;
      // Naming a few and counting the rest keeps the finding actionable — a
      // reader needs to see that it IS a collision and where to look, not a
      // transcript of the catalog.
      const others =
        unshown > 0
          ? `${sample.join(", ")}, and ${unshown} more`
          : sample.join(", ");
      const subject =
        identities.size === 2
          ? "Another skill in this listing also declares"
          : `${identities.size - 1} other skills in this listing also declare`;
      issues.set(self, {
        code: "duplicate-name",
        severity: "warning",
        message: `${subject} the name "${name}" (${others}). This is legal — a consumer must tell them apart by their URIs rather than collapsing or preferring one.`,
      });
    }
  }
  return issues;
}