fix: address Copilot review round 16 on #2248
**A hole my own round-13 change opened.** The byte budget added a `break`, and `manifestListsSelf` described the bounded slice rather than the rows the walk reached — so breaking before a later self-entry left the flag true, suppressed the fallback, and skipped the mandatory frontmatter check entirely. It is `selfAttempted` now, set inside the loop, and marked before the read: a row the walk reached but could not read has still been attempted, and its failure belongs to the loop rather than to a second fetch. **A preview could overwrite a verification's own bytes.** The digest verdict on screen was computed from the verification's fetch; replacing only the text let the frontmatter findings describe different bytes, recreating the mixed-fetch verdict that state exists to prevent. `entryTextVerified` marks which read produced it, and a preview no longer wins over a verification. 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:
parent
89438d1a4b
commit
5a1f0efb5a
3 changed files with 141 additions and 23 deletions
|
|
@ -2529,6 +2529,50 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("does not let a later preview overwrite a verification's own bytes", async () => {
|
||||
// The digest verdict on screen was computed from the verification's fetch;
|
||||
// replacing only the text would let the frontmatter findings describe
|
||||
// different bytes, recreating the mixed-fetch verdict this state exists to
|
||||
// prevent (Copilot).
|
||||
const user = userEvent.setup();
|
||||
let served = skillMdFor({ ...CLEAN_FM, description: "As verified" });
|
||||
const onReadSkillFile = vi.fn(async (uri: string) => {
|
||||
if (uri === "skill://data-analysis/reference.md") {
|
||||
return { text: REF_TEXT };
|
||||
}
|
||||
return { text: served, mimeType: "text/markdown" };
|
||||
});
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
await user.click(
|
||||
screen.getByRole("button", {
|
||||
name: "Verify skill://data-analysis/SKILL.md",
|
||||
}),
|
||||
);
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
within(screen.getByTestId("skill-frontmatter-issues")).getByText(
|
||||
/As verified/,
|
||||
),
|
||||
).toBeInTheDocument(),
|
||||
);
|
||||
|
||||
// The server changes, and the reader re-opens the file in the viewer. The
|
||||
// verification's text must survive, since its digest verdict still shows.
|
||||
served = skillMdFor({ ...CLEAN_FM, description: "Changed after" });
|
||||
await user.click(
|
||||
screen.getByRole("button", { name: "skill://data-analysis/SKILL.md" }),
|
||||
);
|
||||
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(3));
|
||||
expect(
|
||||
within(screen.getByTestId("skill-frontmatter-issues")).getByText(
|
||||
/As verified/,
|
||||
),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("keeps a frontmatter finding when the reader opens another file", async () => {
|
||||
// The check ran off whatever the viewer was showing, so opening a
|
||||
// supporting file made `showingSkillMd` false and silently dropped the
|
||||
|
|
|
|||
|
|
@ -105,6 +105,18 @@ interface VerificationState {
|
|||
* has not clicked Verify should still get the check.
|
||||
*/
|
||||
entryText?: string;
|
||||
/**
|
||||
* True when {@link entryText} came from a **verification** rather than the
|
||||
* preview read.
|
||||
*
|
||||
* A later preview of the same `SKILL.md` must not overwrite text a
|
||||
* verification produced: the digest verdict on screen was computed from that
|
||||
* fetch, and replacing only the text would let the frontmatter findings
|
||||
* describe different bytes — recreating the mixed-fetch verdict this state
|
||||
* exists to prevent (Copilot). A verification always wins, since it brings a
|
||||
* matching digest verdict with it.
|
||||
*/
|
||||
entryTextVerified?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -963,9 +975,12 @@ export function SkillsScreen({
|
|||
// anything not named here is dropped — which silently discarded the
|
||||
// verified `SKILL.md` text the frontmatter check depends on.
|
||||
...(entryText !== undefined
|
||||
? { entryText }
|
||||
? { entryText, entryTextVerified: true }
|
||||
: prev.key === key && prev.entryText !== undefined
|
||||
? { entryText: prev.entryText }
|
||||
? {
|
||||
entryText: prev.entryText,
|
||||
entryTextVerified: prev.entryTextVerified,
|
||||
}
|
||||
: {}),
|
||||
};
|
||||
});
|
||||
|
|
@ -1076,16 +1091,19 @@ export function SkillsScreen({
|
|||
} catch {
|
||||
return; // neither text nor blob; the viewer reports it
|
||||
}
|
||||
setVerification((prev) =>
|
||||
prev.key !== null && prev.key !== key
|
||||
? prev
|
||||
: {
|
||||
...prev,
|
||||
key,
|
||||
files: prev.key === key ? prev.files : {},
|
||||
entryText: text,
|
||||
},
|
||||
);
|
||||
setVerification((prev) => {
|
||||
if (prev.key !== null && prev.key !== key) return prev;
|
||||
const sameKey = prev.key === key;
|
||||
// Never over a verification's own text — see
|
||||
// `entryTextVerified`.
|
||||
if (sameKey && prev.entryTextVerified) return prev;
|
||||
return {
|
||||
...prev,
|
||||
key,
|
||||
files: sameKey ? prev.files : {},
|
||||
entryText: text,
|
||||
};
|
||||
});
|
||||
}
|
||||
})
|
||||
.catch((err: unknown) => {
|
||||
|
|
|
|||
|
|
@ -718,6 +718,51 @@ describe("verifySkills (#2248)", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("runs the fallback when the byte budget broke the loop before the self row", async () => {
|
||||
// `manifestListsSelf` described the bounded SLICE, not the rows reached —
|
||||
// so a break on the byte budget left it true, suppressed the fallback, and
|
||||
// skipped the mandatory frontmatter check entirely (Copilot).
|
||||
const md = "---\nname: late\ndescription: Served\n---\n\n# late\n";
|
||||
const big = "z".repeat(9 * 1024 * 1024);
|
||||
const enc = new TextEncoder();
|
||||
const skill: SkillEntry = {
|
||||
uri: "skill://late/SKILL.md",
|
||||
frontmatter: { name: "late", description: "Listed" },
|
||||
resources: [
|
||||
// Two oversized files cross the budget before the self row is reached.
|
||||
{
|
||||
uri: "skill://late/a.md",
|
||||
digest: await sha256Digest(enc.encode(big)),
|
||||
size: 1,
|
||||
},
|
||||
{
|
||||
uri: "skill://late/b.md",
|
||||
digest: await sha256Digest(enc.encode(big)),
|
||||
size: 1,
|
||||
},
|
||||
{
|
||||
uri: "skill://late/SKILL.md",
|
||||
digest: await sha256Digest(enc.encode(md)),
|
||||
size: enc.encode(md).byteLength,
|
||||
},
|
||||
],
|
||||
};
|
||||
const readResource = vi.fn(async (uri: string) => ({
|
||||
result: {
|
||||
contents: [{ uri, text: uri.endsWith("/SKILL.md") ? md : big }],
|
||||
},
|
||||
}));
|
||||
const client = { readResource } as unknown as InspectorClientProtocol;
|
||||
const [report] = await verifySkills(client, [skill]);
|
||||
// The self file was still fetched, and its frontmatter still compared.
|
||||
expect(readResource).toHaveBeenCalledWith(
|
||||
"skill://late/SKILL.md",
|
||||
undefined,
|
||||
);
|
||||
expect(report.frontmatter).toHaveLength(1);
|
||||
expect(report.frontmatter[0].code).toBe("frontmatter-mismatch");
|
||||
});
|
||||
|
||||
it("does not truncate a conforming manifest", async () => {
|
||||
// A conforming skill totals at most 16 MiB by definition, so the bound
|
||||
// must never shorten one — otherwise it would trade a hostile-server
|
||||
|
|
|
|||
Loading…
Reference in a new issue