fix: address Copilot review round 3 on #2248

Two live findings. The review ran against 4fa00d1c — the commit before the
round-2 fixes — so its other three were re-reports of already-fixed issues,
verified present at HEAD rather than re-fixed.

**The TUI printed "Conformance: conforms" directly above "Verification
FAILED".** Self-contradictory in the one place a reader looks for a verdict:
the static checks pass on a skill whose advertised digest is well-formed,
while its bytes do not hash to it. The heading is now "Listing checks", which
names what the section covers — the checks against the entry the listing
returned, which say nothing about the bytes served. Same problem, and the
same resolution, as the web screen's "No structural issues".

**The test-server guide claimed every result carries the full modern
envelope**, which the new directory handler made untrue: `readDirectoryPage`
deliberately emits `resultType` alone, because SEP-2640 states the caching
attributes for a modern `skills/list` and says nothing of the kind here, and
its one worked example carries `resultType` only. The claim is narrowed to
the two `skills/*` results and the exception is documented as the deliberate
choice it is.

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 19:34:52 -04:00
commit 6b83727332
3 changed files with 50 additions and 9 deletions

View file

@ -141,6 +141,28 @@ describe("SkillsTab (#2248)", () => {
expect(frame).toContain("! gen");
});
it("does not claim the listing conforms while verification is failing", async () => {
// The two verdicts sat in one pane and contradicted each other: the static
// checks pass on `clean` (its advertised digest is well-formed), while the
// bytes do not hash to it. The heading now names what it actually covers.
const { lastFrame, stdin } = render(
<SkillsTab
skills={[clean]}
pageCount={1}
inspectorClient={mockClient()}
width={140}
height={30}
focusedPane="list"
/>,
);
stdin.write(ENTER);
await tick();
const frame = lastFrame() ?? "";
expect(frame).toContain("Verification FAILED");
expect(frame).toContain("Listing checks: no structural issues");
expect(frame).not.toContain("conforms");
});
it("shows the selected skill's URI, description, findings and manifest", () => {
const { lastFrame } = render(
<SkillsTab
@ -154,7 +176,8 @@ describe("SkillsTab (#2248)", () => {
const frame = lastFrame() ?? "";
expect(frame).toContain("skill://clean/SKILL.md");
expect(frame).toContain("A clean skill");
expect(frame).toContain("Conformance: conforms");
// Named for what it covers: the static checks against the listing.
expect(frame).toContain("Listing checks: no structural issues");
expect(frame).toContain("Manifest (1)");
expect(frame).toContain("SKILL.md");
expect(frame).toContain("(51 B)");

View file

@ -357,9 +357,17 @@ export function SkillsTab({
</Box>
)}
{/* Named for the checks it actually covers. An unqualified
"conforms" sat directly above "Verification FAILED" in the
same pane and flatly contradicted it — these are the static
checks against the LISTING, and passing them says nothing
about the bytes the server serves (Copilot). Same wording
problem, and the same fix, as the web screen's "No structural
issues". */}
<Box marginTop={1} flexShrink={0}>
<Text bold>
Conformance{issues.length === 0 ? ": conforms" : ":"}
Listing checks
{issues.length === 0 ? ": no structural issues" : ":"}
</Text>
</Box>
{issues.map((issue, idx) => (

View file

@ -76,13 +76,23 @@ handler cannot reach it. To exercise the *undeclared* case, connect to any
config **without** `"skills"`, where the Inspector must refuse to send the call
locally rather than letting the server answer it.
Every result carries the modern base envelope (`resultType` / `ttlMs` /
`cacheScope`). `skills/*` are consumer-owned, so the SDK stamps nothing for
them; without it a 2026-era connection would receive a result missing the
envelope. It is stamped unconditionally rather than per era — the modern leg
builds a fresh server per request, so there is no era to branch on when the
handlers are registered, and on the legacy leg they are three extra members no
codec inspects.
**Both `skills/*` results carry the full modern base envelope** (`resultType` /
`ttlMs` / `cacheScope`). They are consumer-owned methods, so the SDK stamps
nothing for them; without it a 2026-era connection would receive a result
missing the envelope. It is stamped unconditionally rather than per era — the
modern leg builds a fresh server per request, so there is no era to branch on
when the handlers are registered, and on the legacy leg they are three extra
members no codec inspects.
⚠️ **`resources/directory/read` deliberately carries `resultType` alone.**
SEP-2640 states the caching attributes for a modern `skills/list` in as many
words and says nothing of the kind for this method, whose one worked example
carries `resultType` and nothing else. A fixture sending more than the SEP shows
would make a client that wrongly *required* them look correct, which is the
opposite of what a conformance fixture is for — so `readDirectoryPage` stops
where the spec does, and `ModernDirectoryReadResultSchema` requires exactly as
much.
It works on **either era**: `skills/list`, `skills/get` and
`resources/directory/read` are consumer-owned extension methods that neither
era codec defines, so the SDK's era gate skips them entirely — which is why