Skip to content

feat(ship-check): tool-definition reviewer agent, skill, and surface-diff script - #33

Open
aliasunder wants to merge 29 commits into
mainfrom
feat/tool-definition-reviewer
Open

aliasunder wants to merge 29 commits into
mainfrom
feat/tool-definition-reviewer

Conversation

@aliasunder

@aliasunder aliasunder commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

What this adds

A report-only reviewer for MCP tool definitions, dispatched on demand. It reads the tool list a server sends (tools/list, saved to a file) the way a client receives it, and compares it with the list from before a change.

  • plugins/ship-check/agents/tool-definition-reviewer.md: a sixth ship-check agent. Tools are Read, Grep, Glob, and Bash (Bash only to run the script). It never edits, commits, or posts.
  • plugins/ship-check/skills/tool-definition-review/SKILL.md: the rubric and the checks.
    • A cold read marks each in-scope tool on six rubric dimensions.
    • A diff read reports text changed in tools outside the stated intent, facts the change dropped, description text that repeats a schema description, and failures the handler can return that the description does not list (and listed failures nothing produces).
    • The error check writes one line for each failure it traced, marked listed or MISSING, so the comparison with the description is visible in the report.
    • One dispatch does both reads on a small change. A Pass: cold or Pass: diff line and a Review only: list split a larger one, and one dispatch takes at most eight tools through the diff read.
    • A report is partial, never complete, while any tool is unreviewed or untraced, and it names those tools. If the script cannot run, the report is failed; the reviewer does not compare the files by hand.
  • scripts/surface-diff.ts in that skill, run with Bun: the comparisons that have one right answer, printed as JSON for the agent to judge.
    • Changed, added, removed, and unchanged tools, with key-order-only changes kept apart.
    • A line or schema sentence added to (or removed from) two or more tools, grouped as one shared edit.
    • Description text that repeats a parameter's schema description (40 characters or more), labelled when the base already had it.
    • Sizes, and whether a snapshot's server instructions or prompts changed.
    • --variants lists the tools another configuration's file words differently; --show prints a tool's definition as readable text, because a surface file keeps each description on one long JSON line.
    • It rejects input it cannot trust: an unrecognised shape, a tool without a name or input schema, duplicate names, a tool name that could carry shell syntax (the reviewer passes names on a command line), one page of a paginated list, or flags that do not combine.

What this does not change

The ship-check pipeline does not dispatch the new agent, and pr-review's tool-definition dimension is unchanged. Whether the reviewer becomes part of the pipeline is a separate decision that waits on a comparison against the existing reviewer on a real change.

Supporting changes

  • .github/workflows/test.yml: runs bun test plugins on pushes to main and on pull requests.
  • Release workflows: the plugin and skill archives leave __tests__ out, every action is pinned to a commit, and the job that only validates versions keeps no token in its checkout.
  • Docs: AGENTS.md (structure tree and a convention for bundled scripts), the root README, the plugin README, and the two manifest descriptions now list six agents and eight skills in ship-check. The root README's install and adapting steps name the Bun prerequisite and both vault-cortex tool prefixes.

How it was checked

  • bun test plugins passes; the script tests check runs it on every push.
  • Two mutation checks: removing the key sort fails the two key-order tests, and raising the overlap threshold by one fails the boundary test.
  • Run on two real snapshots of a 33-tool server, the script reports 29 changed and 4 unchanged tools and groups one sentence added to 14 tools as a single shared edit.
  • The agent was dispatched against a tool list with five planted changes (an unintended edit, a deleted fact, a moved fact, a removed error entry, an invented error entry) and reported all five correctly, and against another server's tool list with no base.
  • A first run on a 29-tool change skipped the error-entry check for most tools and still reported complete. The eight-tool limit, the status rule, and the tracing steps in the skill come from that run. Re-run in batches of eight, the same change produced the three error entries known to be missing.
  • A reviewer whose shell refused the script compared the files by hand and left 20 of 33 tools uncompared, which is why a script that cannot run now ends the review as failed.
  • A later run with the script working stopped at eight tools and reported partial as the skill requires, but for one tool it traced six failures and reported none missing while two had no entry. The one-line-per-failure format comes from that run.
  • The release zip exclude pattern was run locally and leaves no __tests__ entry in the archive.

The script has no dependencies and is not type-checked in CI; the tests run it under Bun.

🤖 Generated with Claude Code

aliasunder and others added 4 commits October 3, 2026 13:41
Compares one tool-surface JSON file with the one before it: changed,
added and removed tools, lines added to several tools at once,
description text that repeats a schema description, and sizes. A
listing mode names the tools another configuration words differently.

Tests run on Node 22.18 and 24 in a new workflow, and the release
archives leave test files out.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… text

A surface file keeps each description on one long JSON line, which a
file viewer cuts off. --show prints the named tools with the
description's own line breaks, so a reviewer reads them whole.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…review skill

A report-only reviewer for MCP tool definitions, dispatched on demand
with the tool list a server sends saved to a file. It marks changed
tools against a rubric, then compares the list with the one before the
change: text changed in tools outside the stated intent, dropped
facts, description text that repeats the schema, and failures the
handler can return that the description does not list.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tion

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts Outdated
Comment thread .claude-plugin/marketplace.json Outdated
@umm-actually

umm-actually Bot commented Oct 3, 2026

Copy link
Copy Markdown

Pin create-github-app-token to a commit SHA in release workflows
Low severity · ci · high confidence

.github/workflows/auto_release.yml:72 — beyond the diff's line ranges, in code the changes touch or depend on.

auto_release.yml and manual_release.yml use actions/create-github-app-token@v3, a mutable tag for a third-party action that issues release credentials, while the new test.yml pins every action to a full commit SHA with a version comment. The repo's own pr-review skill requires third-party actions to be SHA-pinned, and the release pipeline is the most privileged workflow.

Failure scenario: The action maintainer force-pushes a new v3 tag (or the action repo is compromised); the next tag push runs attacker-controlled code in the release job, which holds contents: write and an app token with write permission.

Suggested fix
Pin to the full commit SHA with a version comment as test.yml does — uses: actions/create-github-app-token@<full-sha> # v3.x.x — and pin the remaining floating actions/checkout@v7 lines in both release workflows for consistency.

umm-actually · deepseek/deepseek-v4-flash-0731

@umm-actually

umm-actually Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

umm-actually re-reviewed at 3a74463

No new findings (11 tracked finding(s) across all runs).


umm-actually · minimax/minimax-m3

aliasunder and others added 2 commits October 3, 2026 14:05
The script and its tests are plain .ts files run with Bun, as the
other local scripts are. The Node version check, the Node 22.18 and 24
test matrix, and the .mts extension that silenced a Node warning are
gone; CI runs `bun test plugins`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d never reports untraced as complete

A run on 29 changed tools skipped the error-entry check for nearly all
of them, gave a false reason, and still reported complete. The skill
now caps one dispatch at eight tools through the diff read, sets the
status by counting unfinished entries, and gives concrete steps for
tracing a handler. The script trims an overlap before deciding whether
the base already had it, so a repetition that only gained a
neighbouring space is not labelled new.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread .github/workflows/test.yml
Comment thread plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts Outdated
aliasunder and others added 2 commits October 3, 2026 14:23
… intended list

One batch read its `Tools:` line as the intended-tools list, reviewed
all 29 tools, traced eight, and reported complete. The scope line is
now `Review only:`, every in-scope tool is traced whether or not the
change meant to touch it, and the report ends with a count of
unfinished entries followed by the status, so complete requires a
count of zero.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lows

- With no base, a snapshot's sections report changed: null, so "not
  compared" never reads as "unchanged".
- A phrase a parameter states twice is reported as one duplication
  candidate.
- Tests cover a line and a schema sentence removed from several tools.
- The marketplace description counts the plugin's eight skills.
- The release workflows pin checkout and create-github-app-token to
  commits, as the other workflows do.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copy link
Copy Markdown
Owner Author

On the unpinned actions in the release workflows: fixed in cd348fa. auto_release.yml and manual_release.yml now pin actions/checkout to 3d3c42e (v7.0.1) and actions/create-github-app-token to bcd2ba4 (v3.2.0), the same commits umm_review.yml and test.yml use. That was five floating references across the two files.


Generated by Claude Code

aliasunder and others added 5 commits October 3, 2026 14:43
…; validate job keeps no token

A candidate that reached 40 characters only by counting a neighbouring space
was reported at 39. The release workflow's validate job only reads files, so
its checkout no longer persists credentials.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ow to drop the MCP servers

The ship-check row listed seven coverage areas for eight skills. The adapting
steps now name both vault-cortex tool prefixes the agent files carry and say
what going without sequential-thinking takes.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…annot run, and five unclear passages are reworded

A reviewer whose shell refused the script compared the two files by hand, took
the intended-tools list for its scope, and left 20 of 33 tools uncompared. The
skill now retries once with plain paths, then reports failed. Tracing names the
file tools, the eight-tool cap is stated for a diff-only dispatch, and an Errors
line marked skipped with a repository root supplied counts as unfinished.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ADME rubric naming

surface-diff.ts: add comments on Surface.sections, Candidate.preExisting,
Report.orderOnly, and wireJson; rewrite the commonSubstrings Set comment and
the longestCommonSubstring DP comment for accuracy.

Workflows: explain the double-dirname path shape in both release workflows
and the stash/checkout/pop purpose in auto_release.

ship-check README: correct TDQS expansion from "Tool Description" to "Tool
Definition" in the pr-reviewer row; name the rubric (TDQS) in the
tool-definition-reviewer row; add Bun failure note; clarify tools/list as
the MCP method; define "surface" in the usage section.

plugin.json: qualify "four phase agents" with "that load conventions" to
match the README.

AGENTS.md: clarify "dependency-free" as "no npm dependencies", add the
*.test.ts discovery pattern, and explain what `plugins` means in
`bun test plugins`.

Ship-Check: code-quality

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…es, and AGENTS.md says where tests leave the release archives

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
aliasunder and others added 2 commits October 3, 2026 15:10
…ranch in surface-diff

14 tests added: three parseSurface rejection paths (tool not object,
non-string title, non-object annotations), parameterTexts oneOf/allOf
branches and non-object guard, compareSections one-side-only cases,
outputSchema size accounting, findSharedEdits single-tool filtering,
and five CLI integration tests (--show + --variants, unknown flag,
full report with and without --base, --show with missing tool).

Ship-Check: test-audit

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…MISSING, and --names is rejected with --show and --variants

A test run traced six failures for one tool and reported none missing while
two had no entry; a count with a verdict hid the comparison. The Errors block
now carries one line for each failure. The unintended-changes section never
lists an intended tool. --names was silently ignored outside the comparison
report. The CLI report tests assert the full field list.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread plugins/ship-check/skills/tool-definition-review/SKILL.md Outdated
aliasunder and others added 7 commits October 3, 2026 16:04
The eight-tool cap is restated beside the Review only rule, and the fact
count says it covers the old description and schemas together.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ims half characters from an overlap

Losing one of two identical description lines reported no removed line,
because the line comparison used sets. An overlap cut in the middle of a
two-unit character kept a stray half at its edge.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ell syntax

A reviewer passes tool names from the file under review to --show in a shell
command. A name may now hold only letters, digits, and _ . : / -, and the
skill tells the reviewer to quote each name.

Ship-Check: pr-monitor
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…uation in place of em dashes

Twelve lines in three files this branch already edits.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…le and names the files it searched for a size rule

On a real 27-tool change one reviewer dispatch reported two removed error
entries as defects although the input schema rejects those inputs before the
handler runs, and one dispatch searched two of three instruction files and
reported no size rule. Parser and library calls on file content are named as
failure paths to trace.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…name check does

The message printed a name from the command line raw, so a quote or a line
break in it broke the message. Ordinary names print exactly as before.

Ship-Check: triage
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The prior commit replaced em dashes in the two READMEs and the top-level
marketplace description but missed the plan-check plugin description in
the same file.

Ship-Check: code-quality

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread .github/workflows/manual_release.yml
aliasunder and others added 7 commits October 3, 2026 16:44
Two manual dispatches started together computed the same next version and
raced on the push, and two tag pushes raced on the changelog commit to main.
Each workflow now queues a second run behind the first.

Ship-Check: pr-monitor
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… dashes

Fifteen lines in the plan-check README and manifest, package.json, and
SECURITY.md, finishing the change the two READMEs already carry.

Ship-Check: pr-monitor
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ime commit

Without `ref: main`, a queued second dispatch checks out the commit that was
at the tip of main when it was dispatched, not the commit the first run left.
It reads the pre-bump version, computes the same next version, and fails at
`git tag` because the tag already exists.

Ship-Check: bug-check

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ag workflow keeps its original scheduling

A manual release that waited behind another checked out the commit from
dispatch time, computed the version the first run had already tagged, and
failed at the tag step. Its checkout now takes the dispatched branch at run
time. The concurrency group on the tag workflow is removed: a queue there
holds one waiting run, so a third tag pushed in a burst would replace the
second and leave that tag without a release.

Ship-Check: pr-monitor
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… SECURITY.md

Repo docs follow the docs standard, which has no rule against em dashes, so
the rewrite of 28 lines is reverted. One change stays because that standard
asks for it: the first adapting step in the root README carried two dashes
and a nested parenthetical in one sentence and is now two plain sentences.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nged

The first adapting step and two list items return to their original text,
which completes the revert of the punctuation rewrite.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…es again

The docs standard asks for a split when one sentence carries two dashes and
a nested parenthetical. The two list items beside it end with a full stop,
matching the other items in the list.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant