fix: stop reading skills once the byte budget is reached, not only exceeded; review round 2 (#2294)

The byte side of the catalog budget compared with `<=`, so a run whose charged
bytes exactly equalled the limit still read one more skill. Use a strict `<`,
matching the skill count and the setting's description. The skill whose reads
cross the limit is still reported in full.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6i1HBSdSS9kL4npZ2dDSe
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/test/core/mcp/skillsVerification.test.ts

@@ -881,6 +881,50 @@ describe("verifySkills (#2248)", () => {
     expect(reports[2].incomplete).toMatch(/256 skills \/ 60 bytes/);
   });
 
+  it("stops reading once the byte budget is exactly REACHED, not only exceeded", async () => {
+    // Every name is one digit, so every SKILL.md has the same length. With the
+    // budget set to exactly one file's charge, the first skill spends it and
+    // the second must not be read (Copilot, #2294).
+    const enc = new TextEncoder();
+    const mdFor = (i: number) =>
+      `---\nname: e${i}\ndescription: A demo\n---\n\n# e${i}\n`;
+    const oneFile = enc.encode(mdFor(0)).byteLength;
+    const skills = await Promise.all(
+      Array.from({ length: 3 }, async (_, i): Promise<SkillEntry> => {
+        const bytes = enc.encode(mdFor(i));
+        return entry({
+          uri: `skill://e${i}/SKILL.md`,
+          frontmatter: { name: `e${i}`, description: "A demo" },
+          resources: [
+            {
+              uri: `skill://e${i}/SKILL.md`,
+              digest: await sha256Digest(bytes),
+              size: bytes.byteLength,
+            },
+          ],
+        });
+      }),
+    );
+    const readResource = vi.fn(async (uri: string) => ({
+      result: {
+        contents: [
+          { uri, text: mdFor(Number(/e(\d+)/.exec(uri)?.[1] ?? "0")) },
+        ],
+      },
+    }));
+    const client = {
+      readResource,
+      getServerSettings: () => ({ skillCatalogMaxBytes: oneFile }),
+    } as unknown as InspectorClientProtocol;
+    const reports = await verifySkills(client, skills);
+    expect(readResource.mock.calls.length).toBe(1);
+    expect(reports.map((r) => r.outcome)).toEqual([
+      "verified",
+      "incomplete",
+      "incomplete",
+    ]);
+  });
+
   it("charges a dynamic skill's SKILL.md read to the byte budget", async () => {
     // A `"dynamic"` skill has no manifest, so its only read is the fallback
     // SKILL.md fetch. Uncharged, `catalogBytes` stayed 0 and a byte budget of

core/mcp/skillsVerification.ts

@@ -335,8 +335,14 @@ export async function verifySkills(
     // Static checks still run for every entry — they cost no I/O, so a skill
     // past the budget is still reported on, just not read. What stops is the
     // reading.
+    //
+    // ⚠️ STRICT on bytes, like the count. A run whose charged bytes exactly
+    // equal the limit has REACHED it, and `<=` read one more skill past a
+    // budget that was already spent (Copilot, #2294). The skill whose reads
+    // cross the limit is still reported in full — that is decided by charging
+    // after the entry, below, not by this comparison.
     const withinBudget =
-      walkedSkills < budget.maxSkills && catalogBytes <= budget.maxBytes;
+      walkedSkills < budget.maxSkills && catalogBytes < budget.maxBytes;
     // The entry's own SKILL.md, read once and used twice — for its digest and
     // for the frontmatter cross-check. Reading it twice would double the load
     // on the server and, worse, could compare a digest against one snapshot