Skip to content

fix(skills): report a "dynamic" skill as unverifiable, not verified - #2474

Merged
cliffhall merged 3 commits into
v2/mainfrom
v2/fix/2405-dynamic-skill-unverifiable
Sep 24, 2026
Merged

cliffhall merged 3 commits into
v2/mainfrom
v2/fix/2405-dynamic-skill-unverifiable

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #2405

Problem

--verify gave a resources: "dynamic" skill outcome: "verified", ok: true and exit 0, which is the same verdict as a skill whose every file hashed clean. For the dynamic skill nothing was hashed. To a CI job the two cases looked identical.

Change

The issue listed three possible changes and said they were not mutually exclusive. This PR does all three.

  1. A fourth outcome: unverifiable (core/mcp/skillsVerification.ts). This is SEP-2640's own word for a "dynamic" skill's SKILL.md. Precedence is failed > incomplete > unverifiable > verified:
    • A dynamic skill whose frontmatter disagrees still reports failed.
    • A dynamic skill past the catalog budget still reports incomplete, because nothing about it was read.
    • ok stays true, since "dynamic" is a conforming wire form.
    • allSkillsVerified is now false for such a skill, and a new anySkillUnverifiable helper was added.
  2. The headline says so. It reads Checked 1 skill and 0 files: no conformance errors. 1 of 1 skill advertised no digests (resources: "dynamic"), so its integrity was not checked. It never says "Verified" for such a skill.
  3. Opt-in --require-digests (with --verify). A dynamic skill then exits 9 with the envelope code skills_unverifiable. The default exit stays 0, per the issue ("not exit 7, not ok: false"). With mixed outcomes the loudest code wins: 7 > 8 > 9. Exit-code selection is now one helper, skillVerificationExitCode, instead of the same ternary copied into skills/list and skills/get. --require-digests without --verify is rejected, the same way --verify rejects a non-skills method.

The TUI Skills pane shows [UNVERIFIABLE — Enter to re-verify]. Its Record over the outcome union flagged the missing label as a type error, as #2293 intended. The CLI README's outcome table, option table and exit-code table are updated.

Verification

The issue's own repro server (srv.mjs), run against the built CLI:

server flags outcome stderr headline exit
dynamic — unverifiable Checked 1 skill and 0 files: no conformance errors. 1 of 1 skill advertised no digests (resources: "dynamic"), so its integrity was not checked. 0
dynamic --require-digests unverifiable same, plus envelope {"code":"skills_unverifiable"} 9
good — verified Verified 1 skill and 1 file: no conformance errors. 0
good --require-digests verified same 0

TUI Skills pane after pressing Enter on a dynamic skill. This is the rendered Ink frame, captured as text through the TUI's test renderer. Before this change the last line read [Verified — Enter to re-verify].

 Skills (1)                                │ weather-lookup
                                           │
 ▶ ! weather-lookup                        │ file:///skills/weather-lookup/SKILL.md
                                           │
                                           │ Looks up the weather for a city.
                                           │
                                           │ Listing checks:
                                           │   ! resources is "dynamic": the file set is generated, so no
                                           │   digest is advertised and integrity cannot be verified.
                                           │
                                           │ Manifest: "dynamic" — no files advertised
                                           │
                                           │ [UNVERIFIABLE — Enter to re-verify]

Tests added:

  • Core: unverifiable outcome, frontmatter mismatch still failed, over-budget dynamic skill still incomplete, anySkillUnverifiable. The fixture integration test now asserts dynamic-report is unverifiable.
  • CLI: default exit 0, exit 9 under the flag on both skills/list and skills/get, a precedence table for skillVerificationExitCode, the headline wording, flag validation, and the envelope code.
  • TUI: the new status label.

npm run local:gate passes.

🤖 Generated with Claude Code

…2405)

`--verify` gave a `resources: "dynamic"` skill the same verdict as one whose
every file hashed clean: `outcome: "verified"`, `ok: true`, exit 0 — though
nothing was hashed.

- core: a fourth `outcome`, `unverifiable` (SEP-2640's own word for it),
  below `failed` and `incomplete` in precedence; `ok` stays true since
  "dynamic" is a conforming wire form. New `anySkillUnverifiable` helper.
- cli: the headline no longer says "Verified" for such a skill and names how
  many advertised no digests; new opt-in `--require-digests` exits 9
  (`skills_unverifiable`) for a CI job standing in for a host that declines
  unverifiable skills. Exit-code selection moved into one helper shared by
  skills/list and skills/get. Default exit stays 0.
- tui: the Skills pane says UNVERIFIABLE instead of Verified.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation and coverage satisfy #2405, with only a minor JSDoc clarification remaining.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Fixes #2405 by distinguishing dynamic skills from fully verified skills.

Changes:

  • Adds the unverifiable outcome with defined precedence.
  • Adds CLI --require-digests, exit code 9, summaries, and documentation.
  • Updates TUI status rendering and comprehensive tests.
File Description
core/​mcp/​skillsVerification.ts Adds the outcome and helper.
clients/​web/​src/​test/​integration/​mcp/​inspectorClient-skills.test.ts Verifies real-transport behavior.
clients/​web/​src/​test/​core/​mcp/​skillsVerification.test.ts Covers precedence and helpers.
clients/​tui/​src/​components/​SkillsTab.tsx Displays unverifiable status.
clients/​tui/​__tests__/​SkillsTab.test.tsx Tests the TUI label.
clients/​cli/​src/​handlers/​skills-verify.ts Selects exit codes and summaries.
clients/​cli/​src/​handlers/​run-method.ts Applies digest policy to skill methods.
clients/​cli/​src/​handlers/​method-types.ts Adds the digest-policy argument.
clients/​cli/​src/​handlers/​consume-outcome.ts Adds the error-envelope code.
clients/​cli/​src/​error-handler.ts Defines exit code 9.
clients/​cli/​src/​cli.ts Parses and validates the new option.
clients/​cli/​README.md Documents outcomes and exit behavior.
clients/​cli/​__tests__/​skills-verify-cli.test.ts Tests validation and envelopes.
clients/​cli/​__tests__/​run-method-skills.test.ts Tests dispatch, summaries, and precedence.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread core/mcp/skillsVerification.ts
…e manifest (#2474 review)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1: 1 finding (low), fixed.

  • core/mcp/skillsVerification.ts: the anySkillUnverifiable JSDoc overstated the helper. Fixed in eab31f7: it now says the helper reads the selected outcome, so a dynamic skill that reports failed or incomplete does not count. Replied in the thread.

No suppressed comments. Requesting round 2.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation matches the linked issue and specification semantics with comprehensive coverage across affected surfaces.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review closed. Round 2 was clean: no findings, no inline comments, no suppressed comments, approval recommended, and the round-1 finding is listed as resolved. The loop stops on the first clean round.

@cliffhall
cliffhall merged commit 3436c93 into v2/main Sep 24, 2026
4 checks passed
@cliffhall
cliffhall deleted the v2/fix/2405-dynamic-skill-unverifiable branch September 24, 2026 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--verify reports outcome "verified" and exits 0 for a skill whose resources are "dynamic", with nothing hashed

2 participants