fix: address Copilot review round 15 on #2248

**Verification is a tri-state now, because two states could not be right.**
Round 12 was correct that `ok: true` for a manifest whose unread 513th file
may be tampered with is a false pass. Round 15 is correct that forcing `ok:
false` calls a server nonconformant for exceeding limits SEP-2640 states as
SHOULD NOT, with hosts free to support more — contradicting this module's own
rule that a warning never fails a report. Both hold, so:

  verified   -> exit 0   everything checked, everything passed
  failed     -> exit 7   a MUST was broken
  incomplete -> exit 8   nothing checked was wrong; the walk was cut short

`ok` keeps its narrow meaning, `allSkillsVerified` is stricter than
`every(r => r.ok)` since it selects the exit code, and `anySkillFailed`
separates 7 from 8. A job that tolerates oversized catalogs can allow 8 and
still fail on 7.

**`manifestKey` still used `JSON.stringify` on the entry** — the same crash I
fixed in the TUI in round 12, one file over.

**Directory children were navigable outside the skill root.** A server could
return `skill://other-skill/...` and clicking it left the tree, with "Up"
only comparing equality against the root. Containment is checked on the
normalized URI now, and an outside child renders as text rather than a link.
Worth recording from the tests: `..` cannot escape the authority, so
`skill://a/nested/../x.md` resolves back inside and must stay navigable.

**The TUI matched report rows by raw URI**, which missed a row recorded under
an equivalent spelling while `extraReportFiles` suppressed it as covered.
Normalized, like the membership test beside it.

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 2026-09-07 22:47:32 -04:00
commit 89438d1a4b
2 changed files with 434 additions and 141 deletions

View file

@ -201,6 +201,47 @@ describe("runMethod skills dispatch (#2248)", () => {
);
});
it("exits 8, not 7, when the walk was only truncated", async () => {
// SEP-2640 states the read limits as SHOULD NOT and lets hosts support
// more, so exiting `SKILL_NONCONFORMANT` would call a conforming server
// nonconformant — while exiting 0 would report success for entries nobody
// fetched (Copilot).
const md = "---\nname: many\ndescription: Big\n---\n\n# many\n";
const enc = new TextEncoder();
const selfDigest = await sha256Digest(enc.encode(md));
const bodyDigest = await sha256Digest(enc.encode("x"));
const entry: SkillEntry = {
uri: "skill://many/SKILL.md",
frontmatter: { name: "many", description: "Big" },
resources: Array.from({ length: 600 }, (_, i) =>
i === 0
? {
uri: "skill://many/SKILL.md",
digest: selfDigest,
size: enc.encode(md).byteLength,
}
: { uri: `skill://many/f${i}.md`, digest: bodyDigest, size: 1 },
),
};
const client = mockClient({
listSkills: vi.fn().mockResolvedValue({ skills: [entry] }),
readResource: vi.fn(async (uri: string) => ({
result: {
contents: [{ uri, text: uri.endsWith("/SKILL.md") ? md : "x" }],
},
})),
});
const outcome = await runMethod(client, {
method: "skills/list",
verify: true,
});
if (outcome.kind !== "ndjson") throw new Error("unreachable");
expect(outcome.exitCode).toBe(EXIT_CODES.SKILL_INCOMPLETE);
expect(EXIT_CODES.SKILL_INCOMPLETE).not.toBe(
EXIT_CODES.SKILL_NONCONFORMANT,
);
});
it("--verify works on a single skills/get", async () => {
const entry = await cleanEntry();
const client = mockClient({
@ -227,6 +268,7 @@ describe("summarizeSkillVerification (#2248)", () => {
frontmatter: [],
files: [{ uri: "skill://demo/SKILL.md", status: "verified" }],
ok: true,
outcome: "verified",
...over,
});
@ -247,6 +289,7 @@ describe("summarizeSkillVerification (#2248)", () => {
// so collapsing the two counts would misreport the cause.
const failed = report({
ok: false,
outcome: "failed",
files: [{ uri: "skill://demo/SKILL.md", status: "mismatch" }],
});
expect(summarizeSkillVerification([report(), failed])).toBe(
@ -255,7 +298,7 @@ describe("summarizeSkillVerification (#2248)", () => {
});
it("reports a failure with no mismatched file", () => {
const failed = report({ ok: false, files: [] });
const failed = report({ ok: false, outcome: "failed", files: [] });
expect(summarizeSkillVerification([failed])).toBe(
"1 of 1 skill failed verification (0 digest/size mismatch across 0 files).",
);

View file

@ -135,6 +135,33 @@ describe("consumeMethodOutcome NDJSON summary and exit code (#2248)", () => {
});
});
it("labels the envelope for an INCOMPLETE run, not a nonconformant one", async () => {
// The envelope's `code` follows the exit code, so a caller reading one
// never has to reconcile it against the other — and exit 8 means the
// server broke no MUST.
const streams = captureStreams();
let thrown: unknown;
try {
await consumeMethodOutcome(
{
kind: "ndjson",
lines: [{ outcome: "incomplete" }],
summary: "not fully checked",
exitCode: EXIT_CODES.SKILL_INCOMPLETE,
},
{},
);
} catch (err) {
thrown = err;
} finally {
streams.restore();
}
expect(thrown).toMatchObject({
exitCode: EXIT_CODES.SKILL_INCOMPLETE,
envelope: { code: "skills_incomplete" },
});
});
it("leaves an --app-info NDJSON outcome unchanged", async () => {
// No summary, no exit code — the field is additive and the older caller
// must behave exactly as before.