fix: address Copilot review round 12 on #2248
**The read bounds could produce a false `ok: true`.** Entries past the cap are never fetched, `fileFailed` only examines fetched rows, and `resource-limit-exceeded` is a WARNING — so a manifest whose 513th file was tampered with reported the skill verified. That trades a denial of service for a wrong answer, which is the failure this whole PR argues against. A truncated read is now recorded as `incomplete` and forces `ok: false`, reported separately so a consumer can tell "this skill is wrong" from "this skill was not fully checked". And a self entry excluded by the cap is now verified against its declared digest, not merely read for frontmatter — otherwise the skill's own SKILL.md was the one file nobody checked. **`entryKey` could crash the TUI.** It called `JSON.stringify` on the whole server-controlled entry, during render. `skillEntryKey` is decided by the same `jsonGraphError` guard that bounds the frontmatter comparison, with a coarse fallback that still separates two unrepresentable entries by URI — collapsing them would show one skill's verdict under another's name. **The tab bar wraps and `tabsHeight` was hard-coded to 1.** An OAuth-capable stdio server serving Skills needs ~107 columns, so it wraps at 132 as well as at 80, and every pane below was sized a row too tall. `tabBarRows` derives it from the same list `Tabs` renders, via a shared `visibleTabs(flags)` — the duplicated filter being how the two would drift apart again. Nine tests in a new `tabsConfig.test.ts`, including the 80-column regression. Two assert properties rather than instances: rows never decrease as the terminal narrows, and a tab wider than the row gets its own rather than looping. 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
60d5f403e6
commit
27f1b2f1bd
2 changed files with 380 additions and 33 deletions
|
|
@ -513,6 +513,30 @@ export function totalSkillBytes(resources: readonly SkillResource[]): number {
|
|||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* 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.
|
||||
|
|
|
|||
|
|
@ -91,9 +91,24 @@ export interface SkillVerifyReport {
|
|||
* `files` mirrors the manifest one-for-one (Copilot).
|
||||
*/
|
||||
files: SkillFileReport[];
|
||||
/**
|
||||
* Why the manifest could not be checked in full, or `undefined` when it was.
|
||||
*
|
||||
* Set when the read bounds truncated the manifest. It is reported separately
|
||||
* from `ok` being false so a consumer can tell "this skill is wrong" from
|
||||
* "this skill was not fully checked" — but it does make `ok` false, because
|
||||
* the alternative is worse: entries past the cap are never fetched, and
|
||||
* `resource-limit-exceeded` is only a WARNING, so a manifest whose 513th
|
||||
* file was tampered with reported `ok: true` and the CLI said the skill
|
||||
* verified (Copilot). A bound that turns a denial of service into a false
|
||||
* pass has traded down.
|
||||
*/
|
||||
incomplete?: string;
|
||||
/**
|
||||
* False when anything the SEP makes a MUST was broken: an error-severity
|
||||
* finding, a digest or size mismatch, or a file that could not be read.
|
||||
* finding, a digest or size mismatch, or a file that could not be read —
|
||||
* **or when {@link incomplete} is set**, since a verification that did not
|
||||
* finish cannot report success.
|
||||
*
|
||||
* A `warning` does **not** clear it — a `"dynamic"` manifest is legal, and a
|
||||
* report that failed CI for it would be telling server authors their
|
||||
|
|
@ -247,6 +262,10 @@ export async function verifySkills(
|
|||
// running total would CROSS the limit, so a conforming skill (≤ 16 MiB in
|
||||
// total, by definition) is never truncated.
|
||||
const manifest = boundedManifest(declared);
|
||||
const incomplete =
|
||||
manifest.length < declared.length
|
||||
? `Only ${manifest.length} of ${declared.length} manifest entries were read: the skill exceeds the ${SKILL_MAX_RESOURCE_ENTRIES}-entry / ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limits, so the rest were not fetched and cannot be reported on.`
|
||||
: undefined;
|
||||
const entryIdentity = skillUriIdentity(entry.uri);
|
||||
// Compared by NORMALIZED identity, like every other URI comparison here —
|
||||
// `checkSkillConformance` already accepts a manifest self-entry written in
|
||||
|
|
@ -327,6 +346,20 @@ export async function verifySkills(
|
|||
fail(reasonOf(err));
|
||||
}
|
||||
}
|
||||
// If the DECLARED manifest lists this file but the read bounds
|
||||
// excluded it, verify it here too. The fallback exists for the
|
||||
// frontmatter check, but reading a file and then not checking the
|
||||
// digest the manifest advertised for it would leave the entry's own
|
||||
// SKILL.md the one file nobody verified (Copilot).
|
||||
const declaredSelf = declared.find(
|
||||
(resource) => skillUriIdentity(resource.uri) === entryIdentity,
|
||||
);
|
||||
if (declaredSelf && entryBytes !== undefined) {
|
||||
files.push({
|
||||
uri: entry.uri,
|
||||
...(await verifySkillResource(declaredSelf, entryBytes)),
|
||||
});
|
||||
}
|
||||
} catch (err) {
|
||||
// An expired authorization is the one error that is not this file's
|
||||
// problem — see the note on the function.
|
||||
|
|
@ -359,7 +392,9 @@ export async function verifySkills(
|
|||
conformance,
|
||||
frontmatter,
|
||||
files,
|
||||
ok: !hasError && !fileFailed,
|
||||
...(incomplete ? { incomplete } : {}),
|
||||
// An unfinished verification is not a passing one.
|
||||
ok: !hasError && !fileFailed && incomplete === undefined,
|
||||
});
|
||||
}
|
||||
return reports;
|
||||
|
|
|
|||
Loading…
Reference in a new issue