From 6b837273324926540c71dc5793b186644b53e0e0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 19:34:52 -0400 Subject: [PATCH] fix: address Copilot review round 3 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- clients/tui/__tests__/SkillsTab.test.tsx | 25 +++++++++++++++++++++++- clients/tui/src/components/SkillsTab.tsx | 10 +++++++++- docs/test-servers.md | 24 ++++++++++++++++------- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx index 7d3d6408..e4b44f8c 100644 --- a/clients/tui/__tests__/SkillsTab.test.tsx +++ b/clients/tui/__tests__/SkillsTab.test.tsx @@ -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( + , + ); + 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( { 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)"); diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index 65857ba9..fb86c8eb 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -357,9 +357,17 @@ export function SkillsTab({ )} + {/* 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". */} - Conformance{issues.length === 0 ? ": conforms" : ":"} + Listing checks + {issues.length === 0 ? ": no structural issues" : ":"} {issues.map((issue, idx) => ( diff --git a/docs/test-servers.md b/docs/test-servers.md index 3fdca7d6..7acfe797 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -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