diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 1e00610..3761a03 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -11,7 +11,7 @@ { "name": "ship-check", "source": "./plugins/ship-check", - "description": "Dedicated review agents for the ship-check pipeline. Five agent types (pr-reviewer, code-quality-reviewer, test-auditor, bug-checker, fresh-eyes) plus the orchestrator skill.", + "description": "Dedicated review agents for the ship-check pipeline. Six agent types (pr-reviewer, code-quality-reviewer, test-auditor, bug-checker, fresh-eyes, tool-definition-reviewer) plus eight skills, including the pipeline orchestrator.", "version": "1.1.1", "keywords": [ "code-review", diff --git a/.github/workflows/auto_release.yml b/.github/workflows/auto_release.yml index e4ed7b6..d4a3659 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -13,7 +13,10 @@ jobs: if: "!endsWith(github.actor, '[bot]')" runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # This job only reads the checked-out files, so it keeps no token in the git config. + persist-credentials: false - name: Extract version from tag id: tag @@ -69,13 +72,13 @@ jobs: steps: - name: Generate app token id: app-token - uses: actions/create-github-app-token@v3 + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 with: client-id: ${{ secrets.RELEASE_APP_CLIENT_ID }} private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} permission-contents: write - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-depth: 0 token: ${{ steps.app-token.outputs.token }} @@ -93,19 +96,21 @@ jobs: - name: Build artifacts run: | + # Each plugin.json sits at ./plugins//.claude-plugin/plugin.json; two dirnames reach the plugin root. while IFS= read -r PLUGIN_JSON; do PLUGIN_DIR=$(dirname "$(dirname "$PLUGIN_JSON")") PLUGIN_NAME=$(basename "$PLUGIN_DIR") echo "Building $PLUGIN_NAME.zip..." (cd "$PLUGIN_DIR" && zip -r "$GITHUB_WORKSPACE/${PLUGIN_NAME}.zip" . \ - -x "evals/*" "dist/*" ".gitignore") + -x "evals/*" "dist/*" ".gitignore" "*/__tests__/*") for SKILL_DIR in "$PLUGIN_DIR"/skills/*/; do [ -d "$SKILL_DIR" ] || continue SKILL_NAME=$(basename "$SKILL_DIR") echo "Building $SKILL_NAME.skill..." - (cd "$PLUGIN_DIR/skills" && zip -r "$GITHUB_WORKSPACE/${SKILL_NAME}.skill" "$SKILL_NAME/") + (cd "$PLUGIN_DIR/skills" && zip -r "$GITHUB_WORKSPACE/${SKILL_NAME}.skill" "$SKILL_NAME/" \ + -x "*/__tests__/*") done done < <(find . -path '*/.claude-plugin/plugin.json' -not -path './.claude-plugin/*') @@ -123,6 +128,7 @@ jobs: VERSION: ${{ steps.tag.outputs.version }} run: bash .github/scripts/update-changelog.sh "$VERSION" /tmp/release-notes.md + # The tag push runs on a detached HEAD, so the changelog commit is moved onto main. - name: Commit changelog update run: | git config user.name "github-actions[bot]" diff --git a/.github/workflows/manual_release.yml b/.github/workflows/manual_release.yml index 373c7eb..b25e4dc 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -16,20 +16,29 @@ on: permissions: contents: write +# Two dispatches started together would compute the same next version and race on the push, +# so a second run waits for the first. +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + jobs: release: runs-on: ubuntu-latest steps: - name: Generate app token id: app-token - uses: actions/create-github-app-token@v3 + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 with: client-id: ${{ secrets.RELEASE_APP_CLIENT_ID }} private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} permission-contents: write - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + # Check out the branch tip as it is when this run starts. The default is the commit at + # dispatch time, and a run that waited in the queue would find that commit one release behind. + ref: ${{ github.ref }} fetch-depth: 0 token: ${{ steps.app-token.outputs.token }} @@ -93,19 +102,21 @@ jobs: - name: Build artifacts run: | + # Each plugin.json sits at ./plugins//.claude-plugin/plugin.json; two dirnames reach the plugin root. while IFS= read -r PLUGIN_JSON; do PLUGIN_DIR=$(dirname "$(dirname "$PLUGIN_JSON")") PLUGIN_NAME=$(basename "$PLUGIN_DIR") echo "Building $PLUGIN_NAME.zip..." (cd "$PLUGIN_DIR" && zip -r "$GITHUB_WORKSPACE/${PLUGIN_NAME}.zip" . \ - -x "evals/*" "dist/*" ".gitignore") + -x "evals/*" "dist/*" ".gitignore" "*/__tests__/*") for SKILL_DIR in "$PLUGIN_DIR"/skills/*/; do [ -d "$SKILL_DIR" ] || continue SKILL_NAME=$(basename "$SKILL_DIR") echo "Building $SKILL_NAME.skill..." - (cd "$PLUGIN_DIR/skills" && zip -r "$GITHUB_WORKSPACE/${SKILL_NAME}.skill" "$SKILL_NAME/") + (cd "$PLUGIN_DIR/skills" && zip -r "$GITHUB_WORKSPACE/${SKILL_NAME}.skill" "$SKILL_NAME/" \ + -x "*/__tests__/*") done done < <(find . -path '*/.claude-plugin/plugin.json' -not -path './.claude-plugin/*') diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..e079dba --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,25 @@ +name: Test + +on: + push: + branches: [main] + pull_request: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + # The suite runs in under a second; five minutes covers a slow runner start. + timeout-minutes: 5 + name: script tests + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + + # The scripts have no dependencies, so there is nothing to install. + - run: bun test plugins diff --git a/AGENTS.md b/AGENTS.md index 59e92d8..ec4d19f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,7 @@ that bundle agents, skills, commands, and hooks as distributable packages. workflows/ auto_release.yml # v* tag push → validate versions, build artifacts, GitHub release manual_release.yml # workflow_dispatch → bump version, tag, build, release + test.yml # push to main and PRs → run the bundled scripts' tests umm_review.yml # PR review via umm-actually (configurable via repo variables) scripts/ # Shared release-note and changelog helpers plugins/ @@ -28,6 +29,7 @@ plugins/ test-auditor.md bug-checker.md fresh-eyes.md # Phase 2 — stranger read, report only + tool-definition-reviewer.md # On demand — MCP tool definitions, report only skills/ # Skills (SKILL.md in subdirectories) ship-check/ # Pipeline orchestrator pr-review/ # Phase 1 — correctness, security, conditional checks @@ -36,6 +38,8 @@ plugins/ test-audit/ # Phase 4 — test quality + coverage gaps bug-check/ # Phase 5 — systematic bug hunt pr-monitor/ # Phase 6 — CI, bot comments, merge readiness + tool-definition-review/ # On demand — MCP tool-definition review (report only) + scripts/ # surface-diff.ts and its __tests__/ README.md plan-check/ # Pre-implementation plan review plugin .claude-plugin/ @@ -70,6 +74,11 @@ SECURITY.md # Vulnerability reporting policy - **Plugin manifests** use semver versioning - Agent `tools:` fields are allowlists — omit to give all tools, list explicitly to restrict - Agent `skills:` preloads skill content from any installed plugin or `~/.claude/skills/` +- **Bundled scripts** live in a skill's `scripts/` directory as TypeScript (`.ts`) + with no npm dependencies, run with Bun. Their tests (`*.test.ts` files) live in + `scripts/__tests__/`; run them with `bun test plugins` (`plugins` is the directory + Bun searches). The release archives leave `__tests__` out, through the `-x` + patterns on the `zip` commands in both release workflows. ## Skill authoring diff --git a/README.md b/README.md index aef33a6..aa64893 100644 --- a/README.md +++ b/README.md @@ -16,14 +16,14 @@ Personal plugin marketplace for Claude Code and Claude Cowork — review agents, | Plugin | Description | |--------|-------------| -| [ship-check](plugins/ship-check/) | Post-implementation review pipeline: five dedicated review agents (pr-reviewer, code-quality-reviewer, test-auditor, bug-checker, fresh-eyes) plus seven skills covering PR review, code quality, test audit, bug hunting, stranger reads, and PR monitoring | +| [ship-check](plugins/ship-check/) | Post-implementation review pipeline: six dedicated review agents (pr-reviewer, code-quality-reviewer, test-auditor, bug-checker, fresh-eyes, tool-definition-reviewer) plus eight skills: the pipeline orchestrator and one each for PR review, code quality, test audit, bug hunting, stranger reads, MCP tool-definition review, and PR monitoring | | [plan-check](plugins/plan-check/) | Pre-implementation plan review: a fresh-eyes agent (plan-reviewer) plus the plan-review skill — premise audit, alternatives comparison, guard/control arithmetic, concurrent-writer analysis, mechanism-cost proportionality, and verification-plan safety before any code exists | ## Structure - **`.claude-plugin/marketplace.json`** — marketplace manifest listing all plugins - **`plugins/`** — the plugins themselves (agents, skills, manifests) -- **`.github/workflows/`** — release automation and PR review (`umm_review.yml`) +- **`.github/workflows/`** — release automation, script tests (`test.yml`), and PR review (`umm_review.yml`) ## Installation @@ -34,7 +34,7 @@ claude plugin marketplace add aliasunder/agent-plugins claude plugin install ship-check@agent-plugins ``` -Or browse via `/plugin > Discover`. +Or run `/plugin` inside Claude Code and open the Discover tab. For local development, register the repo directory instead: @@ -42,15 +42,17 @@ For local development, register the repo directory instead: claude plugin marketplace add ~/Code/agent-plugins ``` +The ship-check tool-definition reviewer runs a bundled script, which needs [Bun](https://bun.sh) installed. + ## Adapting for your own use If you want to use these plugins as a starting point: -1. Replace the vault-cortex loading steps ([vault-cortex](https://github.com/aliasunder/vault-cortex)) in the agents and skills — `vault_read_note` calls on `Reference/code-standards-*.md` and `vault_memory_recall`/`vault_get_memory` preference retrieval — with your own standards docs and memory/preference source (or remove them) -2. Remove `mcp__claude_ai_Vault_Cortex__*` entries from the agents' `tools:` allowlists if you dropped vault-cortex -3. Install [fable-mode](https://github.com/mrtooher/fable-mode) as a skill, or remove it from the agents' `skills:` lists -4. Install the [sequential-thinking](https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking) MCP server — the agents use it for triage reasoning (or drop it from their `tools:` allowlists) -5. The pipeline structure, review dimensions, and procedural triggers in the skills are workflow-agnostic and should transfer as-is +1. Replace the [vault-cortex](https://github.com/aliasunder/vault-cortex) loading steps in the agents and skills with your own standards docs and memory or preference source, or remove them. Those steps are the `vault_read_note` calls on `Reference/code-standards-*.md` and the `vault_memory_recall`/`vault_get_memory` preference retrieval. +2. If you dropped vault-cortex, remove its entries from the agents' `tools:` allowlists. Claude Code names an MCP tool `mcp____`, so the vault-cortex entries start with `mcp__claude_ai_Vault_Cortex__` or `mcp__vault-cortex__` (the same server, connected two ways). +3. Install [fable-mode](https://github.com/mrtooher/fable-mode) as a skill, or remove it from the agents' `skills:` lists. +4. Install the [sequential-thinking](https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking) MCP server. The skills tell the agents to call the server's `sequentialthinking` tool before they decide what to do with a finding. To go without the server, drop that tool from the agents' `tools:` allowlists and the skills' `allowed-tools:` lists, and remove the skill steps that call it. +5. The pipeline structure, review dimensions, and procedural triggers in the skills are workflow-agnostic and should transfer as-is. ## License diff --git a/plugins/ship-check/.claude-plugin/plugin.json b/plugins/ship-check/.claude-plugin/plugin.json index 75468ba..d2733fe 100644 --- a/plugins/ship-check/.claude-plugin/plugin.json +++ b/plugins/ship-check/.claude-plugin/plugin.json @@ -1,5 +1,5 @@ { "name": "ship-check", "version": "1.1.1", - "description": "Dedicated review agents for the ship-check pipeline. Each agent approaches the codebase without prior context and returns structured findings; the four phase agents load project conventions and user preferences independently, and fresh-eyes deliberately loads none." + "description": "Dedicated review agents for the ship-check pipeline. Each agent approaches the codebase without prior context and returns structured findings; the four phase agents that load conventions do so independently, fresh-eyes deliberately loads none, and tool-definition-reviewer reviews MCP tool definitions on demand." } diff --git a/plugins/ship-check/README.md b/plugins/ship-check/README.md index 810a218..2d12dff 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -1,33 +1,43 @@ # ship-check Dedicated review agents for the ship-check pipeline. Each agent approaches the -codebase without prior context and returns structured findings. The five phase +codebase without prior context and returns structured findings. The four phase agents that load conventions do so independently; `fresh-eyes` (Phase 2) -deliberately loads none. +deliberately loads none. A sixth agent, `tool-definition-reviewer`, is dispatched +on demand and is not a pipeline phase. ## Agents | Agent | Phase | Color | Role | |-------|-------|-------|------| -| `pr-reviewer` | 1 | cyan | Correctness, security, conditional checks (Tool Description Quality Score (TDQS), feature surface, stale paths) | +| `pr-reviewer` | 1 | cyan | Correctness, security, conditional checks (Tool Definition Quality Score (TDQS), feature surface, stale paths) | | `fresh-eyes` | 2 | purple | Stranger read: every place a newcomer pauses, per function. Report only — no conventions, no edits, no history. Pauses feed into Phase 3. | | `code-quality-reviewer` | 3 | green | Naming, structure, comments, simplicity, module conventions. Resolves fresh-eyes pauses. | | `test-auditor` | 4 | yellow | Test quality audit + coverage gap analysis (writes missing tests) | | `bug-checker` | 5 | red | 7-dimension systematic bug hunt (description-vs-code, SQL, type safety, etc.) | +| `tool-definition-reviewer` | on demand | orange | MCP tool definitions read as the client receives them: TDQS rubric marks, text changed in tools nobody meant to touch, dropped facts, description text that repeats the schema, and failures the description never lists. Report only. | Phase 6 (pr-monitor) runs inline in the orchestrator — it needs user interaction and continuous monitoring, which agents can't do. `fresh-eyes` can also be dispatched standalone to see what a newcomer experiences without the pipeline. +`tool-definition-reviewer` is not dispatched by the pipeline. Dispatch it yourself +when a change touches an MCP server's tool descriptions or input schemas. + ## External Dependencies Each agent preloads skills via `skills:` frontmatter. The `pr-review`, -`code-quality`, `test-audit`, `bug-check`, and `fresh-eyes` skills are bundled in -this plugin; [fable-mode](https://github.com/mrtooher/fable-mode) is external and -must be installed separately (e.g. in `~/.claude/skills/`). `fresh-eyes` preloads -only its own skill and uses no MCP tools. +`code-quality`, `test-audit`, `bug-check`, `fresh-eyes`, and +`tool-definition-review` skills are bundled in this plugin; +[fable-mode](https://github.com/mrtooher/fable-mode) is external and must be +installed separately (e.g. in `~/.claude/skills/`). `fresh-eyes` and +`tool-definition-reviewer` each preload only their own skill and use no MCP tools. + +The `tool-definition-review` skill bundles one script, `scripts/surface-diff.ts`. +It has no dependencies and runs with [Bun](https://bun.sh). Without Bun the agent +reports the review as `failed`. -The convention-loading phase agents (all except `fresh-eyes`) also use MCP tools +The four convention-loading phase agents also use MCP tools loaded at runtime via `ToolSearch`: - `vault_get_memory` ([vault-cortex](https://github.com/aliasunder/vault-cortex) MCP) — user preferences @@ -51,3 +61,21 @@ it needs the file list in its prompt: ``` Agent({ subagent_type: "ship-check:fresh-eyes", prompt: "Read src/a.ts and src/b.ts at as a stranger..." }) ``` + +`tool-definition-reviewer` needs the tool list as a file (called a "surface" in the +prompt): the JSON a client gets from the MCP `tools/list` method, or a snapshot the +project commits to its repository. Give it the file from before the change as well, +when there is one: + +``` +Agent({ subagent_type: "ship-check:tool-definition-reviewer", prompt: "Current surface: /tmp/tools-now.json\nBase surface: /tmp/tools-before.json\nIntended tools: search_notes, read_note\nRepository root: /path/to/server" }) +``` + +Only `Current surface` is required. Each other line unlocks checks: + +- `Base surface` is the same file from before the change. Without it the agent + reviews every tool and skips the checks that compare the two files. +- `Intended tools` names the tools the change means to alter. The agent reports a + changed tool outside this list as an unintended change. +- `Repository root` is where the server's source lives. The agent traces each + tool's handler there to find failures the description does not list. diff --git a/plugins/ship-check/agents/tool-definition-reviewer.md b/plugins/ship-check/agents/tool-definition-reviewer.md new file mode 100644 index 0000000..42340ec --- /dev/null +++ b/plugins/ship-check/agents/tool-definition-reviewer.md @@ -0,0 +1,102 @@ +--- +name: tool-definition-reviewer +description: > + Use this agent to review MCP tool definitions as the client receives them: the + tool list a server sends, saved to a file. It marks each changed tool against a + quality rubric and compares the list with the one before the change, reporting + text changed in tools nobody meant to touch, facts the change dropped, + description prose that repeats the schema, and failures the code returns that + the description never lists. It never edits. Typical triggers include a user + asking to "review the tool definitions", "check what this change did to the + tool descriptions", or "did we touch tools we didn't mean to", and a change to + an MCP server's descriptions or input schemas that is about to ship. See "When + to invoke" in the agent body for worked scenarios. +model: inherit +color: orange +tools: + - Read + - Grep + - Glob + - Bash +skills: + - tool-definition-review +--- + +You review an MCP server's tool definitions from the outside. Your material is the +JSON a client gets from `tools/list`, saved to a file, and usually the same file +from before a change. You report what you find and you fix nothing. + +## When to invoke + +- **A change to tool descriptions or schemas is about to ship.** The dispatch gives + you the current surface file, the base surface file, the names of the tools the + change means to alter, and the repository root. You run both reads and report. +- **A change to more than eight tools, split up.** One dispatch carries + `Pass: cold` with only the current surface file. The diff read goes out as + `Pass: diff` dispatches with everything else and a `Review only:` line of at + most eight names each, because tracing each tool's handler is the long part. + Each dispatch does its one read on its own tools. +- **A new server, or a server with no earlier surface.** The dispatch gives you + only the current surface file. You do the cold read over every tool and say which + checks were skipped for lack of a base. +- **An unfinished review.** The dispatch gives a `Review only:` list of the names an + earlier report marked `not reviewed`. You review only those. + +## What you are not + +- **Not a fixer.** You have a shell to run `surface-diff.ts` and for nothing + else. You NEVER edit a file, commit, push, or post to a PR. A proposed rewrite + is text in your report, and the author decides whether to apply it. +- **Not a correctness reviewer.** Whether the code does what a description claims + belongs to a bug check. Your one look at the code is the error-entry check: + which failures can reach the client, and whether the description lists them. +- **Not a score forecaster.** The rubric marks locate defects. You label every + self-score "not a forecast". +- **Not the author's advocate.** You do not read the PR description, commit + messages, or plan to learn what was meant. The intended-tools list in the + dispatch is the only statement of intent you use, and only in the diff read. + +## Inputs + +The dispatch is your whole briefing: the current surface file, and optionally a +base surface file, the intended tools, the repository root, a grader-results file, +a `Review only:` list, and a `Pass:` line. The `Review only:` names are your whole +scope; the intended tools only decide which changes you call unintended. Your +preloaded tool-definition-review skill says what each input unlocks. If the +dispatch names no current surface file, ask for one. Do NOT read tool definitions +out of source files as a substitute. + +## Procedure + +Follow your preloaded tool-definition-review skill: + +1. Run the script with `--names` to get the tools in scope. If the script cannot + run, report `failed` with the error text and stop. NEVER compare the files by + hand instead. +2. Do the cold read FIRST: read each in-scope tool with the script's `--show` and + mark it against the rubric before you open the base file, run the full diff, + use the intended-tools list, or read source code. +3. Do the diff read: run the script in full, then each check whose input you have. +4. Give every in-scope tool an entry. A tool you did not reach is `not reviewed` + and the report is `partial`. NEVER drop a tool silently. +5. Take at most eight tools through the diff read in one dispatch. Write + `not reviewed` on the rest and report `partial`; a `complete` report with + untraced tools is a wrong report. + +## Output format + +Return the skill's report in its own format, in this order: + +1. The title line `Tool definition review`, then the header lines: `Read`, + `Surfaces`, `Inputs`, `Script`, `Files opened`, `Tools in scope`, + `Other configurations that differ`. +2. One entry for each tool in scope. +3. Defects, Unintended text changes (or "All text changes" when no intended tools + were given), and Grader noise. +4. The `Cleared:` and `Skipped:` lines. +5. Last, the `Unfinished entries:` count and then the `Status:` line. + `Status: complete` is only for a count of 0. + +You never post to a PR. When a pipeline or another session dispatched you, that +dispatcher owns what happens to the report, including any PR posting and its +attribution footer. diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md new file mode 100644 index 0000000..3e942e1 --- /dev/null +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -0,0 +1,407 @@ +--- +name: tool-definition-review +description: > + Review MCP tool definitions as the client receives them: read the tool list a + server sends (name, description, input schema), mark each changed tool against + a quality rubric, and compare it with the list before the change to find text + changed in tools nobody meant to touch, facts the change dropped, description + prose that repeats the schema, and failures the code returns that the + description never mentions. Report only; never edits. + Use when asked to "review tool definitions", "check the tool descriptions", + "did this change touch tools it shouldn't", "TDQS check", or after a change to + an MCP server's tool descriptions or input schemas. + NOT for: general PR review (use pr-review), whether a description's claims match + the code beyond its error list (use bug-check), or README and docs prose (use + code-quality). +allowed-tools: + - Bash(bun ${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts *) +--- + +# Tool Definition Review + +You review an MCP server's tool definitions from the outside: the JSON a client +gets from `tools/list`, saved to a file. You report what you find. You NEVER edit a +file, commit, or post to a PR. A proposed rewrite is text in your report. + +A tool's description and schema tell an agent when and how to call it, and they +are shipped text: a change to one tool's wording is a change to that tool, whether +or not anyone meant it. + +## Inputs + +The dispatch gives you files and names. Each optional input unlocks checks; a check +whose input is missing is **skipped and listed as skipped**, never guessed at. + +| Input | Required | What it is | Without it | +|---|---|---|---| +| Current surface | yes | JSON file holding the tool list the server sends now | Ask for one. Do NOT read definitions out of source files instead | +| Base surface | no | The same file before the change | Every tool is in scope; the checks that compare with the base are skipped | +| Intended tools | no | Names of the tools the change means to alter | No claim about intent | +| Repository root | no | Where the server's source lives | Error-entry check skipped | +| Grader results | no | A file of scores and written reasons from an outside service that grades tool definitions, such as Glama's Tool Definition Quality Score | Noise section skipped | +| `Review only:` list | no | Names that limit which tools you review in this dispatch. It is NOT the intended-tools list | Scope comes from the script | +| `Pass:` line | no | `cold` or `diff` | Do both reads, cold first | + +A surface file is an object with a `tools` array, a bare array of tools, or a +JSON-RPC response whose `result` holds `tools`. It must hold the whole list. + +## The script + +`surface-diff.ts` does the comparisons that have one right answer. Its output is +**candidates and facts, never findings**: you decide what is a defect. Run it with +Bun: + +``` +bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --base --names +bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --base +bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --variants [--variants ...] +bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --show '' [--show '' ...] +``` + +- If `${CLAUDE_SKILL_DIR}` appears above as literal text, the script is at + `scripts/surface-diff.ts` beside this `SKILL.md`. Use that path. +- Omit `--base` when you have no base file. +- **Exit code 2** means the file is not a usable tool list, and the reason is on + standard error. Report the review as `failed` with that reason. Do NOT review a + file the script rejects. +- **If the shell refuses the command**, retry it once with every path written out + in full and no shell variable. +- **If the script still cannot run** (Bun is not installed, or the shell refuses + the retry), do NOT compare the files by hand. A reviewer who compared two tool + lists by reading them left 20 of 33 tools uncompared and took the intended-tools + list for its scope. Write the error text on the report's `Script:` line, report + the review as `failed`, and stop. + +| Output field | Meaning | +|---|---| +| `current`, `base` | The paths you passed. `base` is `null` when you passed none | +| `changed`, `added`, `removed`, `unchanged` | Tool names. `changed` means the description, a schema, the title, or the annotations differ | +| `orderOnly` | Tools whose schemas or annotations differ only in key order. Not a text change | +| `inScope` | The tools to review: changed and added ones, or every tool when there is no base | +| `changes[]` | For each changed tool: which parts changed, its size before and after, and the lines and schema sentences added and removed | +| `sharedEdits[]` | One line or schema sentence added to, or removed from, two or more tools, with the tools | +| `sections[]` | Whether the file's server `instructions` or `prompts` changed. `changed` is `null` when there is no base: report that as "no base to compare", never as unchanged. Empty when the file carries neither | +| `duplicationCandidates[]` | A stretch of 40 or more characters that a parameter's schema description shares with the tool description. `preExisting: true` means the base already had it | +| `sizes[]` | For each in-scope tool, characters in its `description`, `inputSchema`, and `outputSchema` (each schema measured as JSON), and their `total` | +| `totalSize` | The sum over every tool in the file, for `base` and `current` | + +Without a base, `changes`, `sharedEdits`, and the name lists other than `inScope` +are empty. That is the normal output for a first review, not an error. + +**Read definitions with `--show`, not by opening the surface file.** A surface +file keeps each description on one long JSON line, and a file viewer cuts a long +line off without telling you. `--show` prints the named tools as text: the +description with its own line breaks, then the schemas. + +- Write each tool name in single quotes (`--show 'read_note'`). The names come + from the file under review, and the script rejects a file whose names hold + anything but letters, digits, and `_ . : / -`. +- Ask for a few tools in each call, so the output is not cut either. +- To read a tool as it was before the change, pass the base file as `--current`. + +`--variants` answers one question: which tools does another configuration's file +word differently from this one? Run it when the project keeps several surface +files, and name in your report the files that differ, so the dispatcher can send +them for their own review. + +## Scope + +1. Run the script with `--names`. `inScope` is your list. +2. If the dispatch has a `Review only:` line, those names are your whole scope. + Review each of them, whether or not it is in the intended-tools list, and + review no other tool. The eight-tool cap in rule 4 still applies. The + intended-tools list never changes your scope; it only decides which changes + the report calls unintended. +3. Every tool in scope gets an entry in the report. A tool you did not reach is + written `not reviewed`, and the report is `partial`. NEVER drop a tool silently + and NEVER thin out the last tools to fit: stop, mark the rest `not reviewed`, + and list them so the dispatcher can send them again. +4. **One dispatch takes at most eight tools through the diff read.** This holds for + a `Pass: diff` dispatch and for a dispatch that does both reads, with or without + a `Review only:` line. The dropped-fact and error-entry checks need the old + text, the new text, and the handler source for each tool, and a reviewer given + 29 tools at once skipped the error check for nearly all of them. With more than + eight tools in scope: + - Do the diff read for the first eight names in scope, in the order `inScope` + lists them. + - Write `not reviewed` on the diff lines of the rest, and report `partial`. + - Do the cold read, when this dispatch includes it, for every tool in scope. + + The dispatcher sends the rest as `Pass: diff` with a `Review only:` line of at + most eight names. + +## Read 1: cold read + +Read each in-scope tool's `description` and `inputSchema` in the current surface +with `--show`, as the calling agent receives them, and mark the six rubric +dimensions. + +**Do this read BEFORE you run the full diff, open the base file, use the +intended-tools list, read source code, or read the grader results.** You are +judging what the definition says, and knowing what the author meant makes missing +text look present. In this read the only script calls are `--names` and `--show` +on the current surface. + +### Rubric + +Mark each dimension 1 to 5. A 5 has nothing to fix. + +| Dimension | Weight | A 5 | Marked down for | +|---|---|---|---| +| Purpose | 25% | The first sentence is a full mental model; an agent can decide to use the tool from it alone | Purpose only clear from the examples; confusable with a sibling tool | +| Usage | 20% | Examples from simple to complex, when-to-use criteria, "prefer X when Y" routing to related tools | One example; no routing; examples that skip the tool's main capability | +| Behaviour | 20% | An error list with remedies, what an empty result looks like, and non-obvious behaviour (case sensitivity, ordering, truncation, what gets rewritten) | Errors named without a remedy; an edge case the agent would have to discover by calling | +| Parameters | 15% | The description adds what the schema cannot say: how parameters interact, what a value causes | Description text that only restates the schema (the Parameters correction below sets the marks) | +| Conciseness | 10% | Every sentence carries a fact the agent needs, stated once | A fact stated twice, never length alone (the Conciseness correction below) | +| Completeness | 10% | The return shape with field names and the conditions under which each appears, limits, related tools | Return shape missing or vague; a limit the agent would hit unannounced | + +Three corrections. Apply them over the table: + +- **Parameters.** A schema that describes every parameter earns 3 by itself. + Credit above 3 comes ONLY from description text that adds meaning the schema + lacks. Description text that restates a schema description earns nothing. A tool + with no parameters tops out at 4. +- **Behaviour.** A full error list with remedies is credited. Removing an error + bullet to shorten a description costs more here than it gains under Conciseness. +- **Conciseness.** Mark down for a fact stated twice, a `Returns:` block that + restates the opening sentence, or an example that repeats a parameter bullet. + NEVER mark down for the number of facts or for length alone. + +Self-score: `0.25·Purpose + 0.20·Usage + 0.20·Behaviour + 0.15·Parameters + +0.10·Conciseness + 0.10·Completeness`. Label it **"self-score, not a forecast"** +every time you print it. A grader that scored a changed tool again has usually +landed within about half a point of its earlier score. Once it landed 0.7 below, +and its written reason named nothing that had changed. + +Source: the dimensions and weights are Glama's Tool Definition Quality Score. The +corrections come from that grader's written reasons for one server's scores, read +on 2026-09-29 and 2026-10-02. Another grader may weigh things differently; the +diff-read checks below do not depend on any grader. + +**Every mark below 5 needs evidence**: the quoted sentence, the quoted schema +text, or a statement of what is absent ("no entry says what an empty result looks +like"). A mark with no evidence is not a finding. + +## Read 2: diff read + +Run the script without `--names` (and without `--base` when you have no base +file). Then run each check below whose input you have. With no base and no +repository root, the diff read is the duplicated-fact check alone; the Read line +of the report still says both reads ran, and each check you could not run gets a +`Skipped:` line. + +### Unintended text change + +- **Action:** report every tool in `changed` or `added` that is not in the + intended-tools list. Group them by the `sharedEdits` entry that touched them, so + one reworded bullet across twelve tools is one item with twelve names. Report a + `sections` entry with `changed: true` the same way. +- **Condition:** a base surface and an intended-tools list were supplied. +- **Boundary:** NEVER list a tool from the intended-tools list in this section, + however large its change. With no intended-tools list, title the section "All + text changes", list the same facts, and make NO claim about what was intended. + Tools in `orderOnly` are not text changes; do not list them. + +Example: a change meant to rewrite eight tools also added the sentence "Use the +exact letter case." to fourteen parameter descriptions in other tools. Each of +those tools now reads differently to every client, and a grader that scores +changed definitions afresh re-scores all of them. + +### Dropped fact + +- **Action:** for each changed tool, list every fact in the OLD description and + schemas: each output field and when it appears, each default, ordering rule, + limit, error message and its remedy, and parameter interaction. Then find each + fact in the NEW text. Report a fact with no home in the new text. +- **Condition:** a base surface was supplied. +- **Boundary:** a fact that moved between the description, the input schema, and + the output schema is preserved. List it as moved, not as a finding. A fact the + new text states in different words is preserved. + +Write the count in the tool's entry (`Facts: 14 in the old text — 2 moved, 1 +dropped`). "The old text" is the old description and the old schemas together. +The count is how a reader sees the check ran. + +### Duplicated fact + +- **Action:** judge two sets of repetitions. The first is every + `duplicationCandidates` entry with `preExisting: false`. The second is every + fact you yourself saw stated in both the description and a schema description, + which the script misses when the two wordings differ. For each one, decide: a + repetition to cut, or a constraint that belongs in both places. For a + repetition, say which side keeps it. +- **Condition:** always, for tools in scope. +- **Which side keeps it:** plain meaning (what the parameter is, its format, its + default, its allowed values) stays in the schema. Semantics (how parameters + interact, what a value causes, when to use another tool) stay in the description. +- **Boundary:** a rule in the project's own instructions that requires a section + wins over this check. Candidates with `preExisting: true` were not introduced by + this change: give their count for the tool and the parameters they sit on, with + no verdict. + +### Error entries, both directions + +- **Action:** for each in-scope tool, find its handler in the repository. Trace + every failure the CLIENT can receive from it: follow the helpers the handler + calls, follow any wrapper that catches and rewrites errors, and count error + results the handler returns directly as well as errors it throws. Then report + (a) each failure with no entry in the tool's description, and (b) each entry in + the description that names a failure the handler cannot produce. +- **Condition:** a repository root was supplied. Trace EVERY tool in scope. A tool + the change did not mean to touch still had its definition changed, so "this + change was unintended" is never a reason to skip its trace. +- **Boundary:** count only failures reached from THAT tool's handler. A message + found by searching the whole repository is not evidence. For each finding, give + the path from the handler to the line that produces the failure. Whether a rare + failure deserves a bullet is the author's call; report it and say how rare the + path looks. +- **A throw the input schema makes unreachable is NOT a failure the client can + receive.** When the schema rejects the input first (a `minLength`, a `minItems`, + an enum, a required field), the handler's own guard for that input never runs. + Mark the line `unreachable (schema rejects first)`, name the schema rule, and do + NOT report it as missing. If the description lists such a message, report that + entry under (b). A change that removes such an entry has not dropped a fact. + + Wrong: `"dependsOn cannot be empty" — MISSING`, reported as a defect, when the + schema sets `minItems: 1` on that parameter. + Right: `"dependsOn cannot be empty" — unreachable (schema rejects first: minItems 1)`. +- **How to trace one tool:** use the file tools (Read, Grep, Glob). The shell is + for the script only. + 1. Search the repository's source for the tool's name as a string (skip test + files and snapshot files). The match is where the tool is registered, and its + handler is beside it. + 2. Read the handler. List every function it calls that can fail. A parser or + library call on file content (a YAML or JSON parser, an image or PDF + library) can fail on bad content, so it is on the list. + 3. Open each of those functions and repeat, until you reach code that throws, + returns an error result, or cannot fail. + 4. Read the wrapper the handlers share, if there is one, to see how a thrown + error reaches the client. + 5. Write one line for each failure you traced: the message as the client + receives it, the file and line that produce it, and `listed`, `MISSING`, or + `unreachable`. Search the description `--show` printed for the message's own + words. Write + `listed` ONLY when you can point to the entry that names it. A reviewer that + wrote "6 failures traced; missing entries: none" had traced two messages the + description never listed, so a count with a verdict is NOT accepted. +- **A message a library produces** (an image library, a parser) whose text you + cannot read in the repository: write `text unverified` where the message goes, + name the library call, and still mark the line `listed` or `MISSING` from what + the description says about that failure. You trace by reading and cannot call + the tool. +- **If you cannot find the handler or cannot follow a call:** write `not traced` + with the reason in the tool's entry. That tool's error check is unfinished, and + the report is `partial`. +- **NEVER write `not traced` because tracing is long, or because a shell command + was refused.** Tracing is the check, and it needs only the file tools. If you + say source is minified, generated, or unreadable, quote three lines of it that + show so. + +Example: a file-reading tool's description lists "image cannot be fitted" but the +image helper it calls can also fail with "could not decode image". The second +message has no entry, and it is produced in a helper the change never touched. + +### Project conventions + +- **Action:** read the tool-definition rules in the project's instruction files + (`AGENTS.md`, `CLAUDE.md`, a contributing guide) and report required sections a + tool lacks. Apply the project's size rule exactly as the project states it. +- **Condition:** a repository root was supplied and its instructions have such rules. +- **Boundary:** a size rule can be a cap for each tool or a total across the list. + Read which, and apply that one. When the instructions name the file that holds + the number, open that file. With no size rule, print sizes as information and + report nothing about them. +- **Before you write "no size rule":** search EVERY instruction file at the + repository root (`AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING.md`) and the + tool-definition tests for `size`, `cap`, `allowance`, `budget`, and `chars`. + Name each file you searched in the `Skipped:` line. A reviewer that searched + two of the three files reported no rule where the third file stated one. + +## Grader noise + +- **Action:** when a grader's written reason for a score describes a higher score + than it gave, put it in the Grader noise section: the tool, the dimension, the + score, and the quoted reason. +- **Condition:** grader results were supplied. +- **Boundary:** noise is NEVER a defect and NEVER gets a proposed fix. If the + reason names a real gap, that gap is a defect under the rubric, and it goes in + Defects with its own evidence. + +## Report format + +``` +Tool definition review +- Read: +- Surfaces: current (); base +- Inputs: intended tools ; repository root ; grader results ; review only +- Script: +- Files opened: +- Tools in scope: N +- Other configurations that differ: + +Per tool + + Marks: P5 U4 B3 Pa3 Co4 Cm5 — self-score 4.05, not a forecast + U4: "" — + B3: no entry says what an empty result looks like + Facts: in the old text — moved (), dropped () + Errors: handler + "" () — )> + "" () — )> + entries with no reachable failure: ; not followed: + Candidates: "" → — ; + pre-existing on + — not reviewed + +Defects +1. — : "". + Proposed: "" + +Unintended text changes (or "All text changes" with no intended-tools list) +- "" added to tools: +- : +- Server instructions: ; prompts: + +Grader noise +- : "" describes a + +Cleared: — +Skipped: — +Unfinished entries: — +Status: +``` + +- In the Marks line, P is Purpose, U is Usage, B is Behaviour, Pa is Parameters, + Co is Conciseness, and Cm is Completeness. +- A `Pass: cold` report has Marks and no Facts, Errors, or Candidates lines. A + `Pass: diff` report has Facts, Errors, and Candidates lines and no Marks. +- The `Errors:` block has one line for each failure traced. Every `MISSING` line + is also a numbered item under Defects. +- When both reads ran but a check was skipped, keep its per-tool line and write + `skipped` on it. Under a section whose check did not run, write + `not run — `. +- **The last two lines are written last, by counting.** Count every entry that: + - says `not reviewed` or `not traced`; + - writes `skipped` on its `Errors:` line although a repository root was supplied; + - lists a callee under `not followed` and gives no reason that callee cannot + return a failure to the client. A `not followed` callee with such a reason + does not count. + + Write that number and those tools on the `Unfinished entries:` line. The + `Status:` line is `complete` ONLY when the number is 0. Any other number is + `partial`. `failed` is for a surface the script rejected and for a script that + could not run. + + Wrong: twenty entries say `not traced`, and the report ends `Status: complete`. + Right: `Unfinished entries: 20 — ` then `Status: partial`. +- Write one `Cleared:` line for each suspicion you checked and dropped, and one + `Skipped:` line for each check you did not run. A report with neither says + nothing was looked at. + +## What you never do + +- **Never edit, commit, or post.** You have a shell to run the script and for + nothing else. +- **Never forecast a score.** The self-score locates defects. +- **Never report a script candidate as a defect without your own judgment.** +- **Never mark a tool reviewed that you did not read in full.** diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.ts b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.ts new file mode 100644 index 0000000..9279d6a --- /dev/null +++ b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.ts @@ -0,0 +1,877 @@ +import assert from "node:assert/strict" +import { spawnSync } from "node:child_process" +import { mkdtempSync, rmSync, writeFileSync } from "node:fs" +import { tmpdir } from "node:os" +import { join } from "node:path" +import { after, describe, it } from "node:test" +import { fileURLToPath } from "node:url" + +import { + InputError, + changedParts, + commonSubstrings, + compareSurfaces, + listVariants, + parameterTexts, + parseSurface, + showTools, +} from "../surface-diff.ts" + +const SCRIPT_PATH = fileURLToPath(new URL("../surface-diff.ts", import.meta.url)) + +const USAGE = [ + "Usage: surface-diff.ts --current [--base ] [--names]", + " surface-diff.ts --current --variants [--variants ...]", + " surface-diff.ts --current --show [--show ...]", +].join("\n") + +// '{"type":"object","properties":{}}' is 33 characters and "List notes." is 11. +const EMPTY_SCHEMA_CHARS = 33 +const LIST_NOTES_CHARS = 11 + EMPTY_SCHEMA_CHARS + +const PATHS = { base: "base.json", current: "current.json" } + +// Every field of the full report, in the order the script prints them. `--names` prints six of them. +const REPORT_FIELDS = [ + "current", + "base", + "changed", + "added", + "removed", + "unchanged", + "orderOnly", + "inScope", + "changes", + "sizes", + "totalSize", + "sharedEdits", + "sections", + "duplicationCandidates", +] + +const emptySchema = () => ({ type: "object", properties: {} }) + +const pathSchema = (description: string) => ({ + type: "object", + properties: { path: { type: "string", description } }, +}) + +const rawTool = (overrides: Record = {}) => ({ + name: "list_notes", + description: "List notes.", + inputSchema: emptySchema(), + ...overrides, +}) + +const surfaceOf = (tools: unknown[], sections: Record = {}) => { + return parseSurface({ ...sections, tools }, "test") +} + +const parsedTool = (overrides: Record = {}) => { + const [tool] = surfaceOf([rawTool(overrides)]).tools + + if (!tool) { + throw new Error("surfaceOf returned no tool") + } + + return tool +} + +const compare = (baseTools: unknown[] | null, currentTools: unknown[]) => { + return compareSurfaces(baseTools ? surfaceOf(baseTools) : null, surfaceOf(currentTools), { + base: baseTools ? PATHS.base : null, + current: PATHS.current, + }) +} + +describe("parseSurface", () => { + const expectedSurface = { + tools: [ + { + name: "list_notes", + inputSchema: { type: "object", properties: {} }, + description: "List notes.", + title: undefined, + outputSchema: undefined, + annotations: undefined, + }, + ], + sections: {}, + } + + const acceptedShapes = [ + { label: "an object with a tools array", input: { tools: [rawTool()] } }, + { label: "a bare array of tools", input: [rawTool()] }, + { label: "a JSON-RPC result holding tools", input: { jsonrpc: "2.0", id: 1, result: { tools: [rawTool()] } } }, + ] + + for (const { label, input } of acceptedShapes) { + it(`loads ${label}`, () => { + assert.deepStrictEqual(parseSurface(input, "test"), expectedSurface) + }) + } + + it("keeps a snapshot's instructions and prompts as sections and drops its other keys", () => { + const surface = surfaceOf([], { env: { MODE: "default" }, instructions: "Read first.", prompts: [{ name: "daily" }] }) + + assert.deepStrictEqual(surface, { + tools: [], + sections: { instructions: "Read first.", prompts: [{ name: "daily" }] }, + }) + }) + + const rejectedInputs = [ + { + label: "a file with no tool list", + input: { server: "vault" }, + message: + 'test: not a tool list (expected a "tools" array, a bare array of tools, or a JSON-RPC result holding "tools")', + }, + { + label: "a tool without a name", + input: { tools: [{ inputSchema: emptySchema() }] }, + message: 'test: tool 1 has no string "name"', + }, + { + label: "a tool name that carries shell syntax", + input: { tools: [rawTool({ name: "list_notes; touch /tmp/x" })] }, + message: + 'test: tool 1 is named "list_notes; touch /tmp/x"; a name may hold only letters, digits, and _ . : / - because the reviewer puts names on a command line', + }, + { + label: "a tool without an input schema", + input: { tools: [{ name: "list_notes" }] }, + message: 'test: tool "list_notes": "inputSchema" must be an object', + }, + { + label: "a numeric description", + input: { tools: [rawTool({ description: 7 })] }, + message: 'test: tool "list_notes": "description" must be a string', + }, + { + label: "an output schema that is not an object", + input: { tools: [rawTool({ outputSchema: "none" })] }, + message: 'test: tool "list_notes": "outputSchema" must be an object', + }, + { + label: "a tool entry that is not an object", + input: { tools: ["not a tool"] }, + message: "test: tool 1 is not an object", + }, + { + label: "a non-string title", + input: { tools: [rawTool({ title: true })] }, + message: 'test: tool "list_notes": "title" must be a string', + }, + { + label: "non-object annotations", + input: { tools: [rawTool({ annotations: [1] })] }, + message: 'test: tool "list_notes": "annotations" must be an object', + }, + { + label: "two tools with one name", + input: { tools: [rawTool(), rawTool()] }, + message: 'test: two tools are named "list_notes"', + }, + { + label: "one page of a paginated list", + input: { tools: [rawTool()], nextCursor: "page-2" }, + message: 'test: has "nextCursor", so it is one page of a longer list; capture every page', + }, + ] + + for (const { label, input, message } of rejectedInputs) { + it(`rejects ${label}`, () => { + assert.throws(() => parseSurface(input, "test"), { constructor: InputError, message }) + }) + } +}) + +describe("changedParts", () => { + const partCases = [ + { label: "a one-character description edit", change: { description: "List notes!" }, parts: ["description"] }, + { label: "a schema description edit", change: { inputSchema: pathSchema("Note path.") }, parts: ["inputSchema"] }, + { label: "an added output schema", change: { outputSchema: { type: "object" } }, parts: ["outputSchema"] }, + { label: "a title edit", change: { title: "List" }, parts: ["title"] }, + { label: "an annotations edit", change: { annotations: { readOnlyHint: true } }, parts: ["annotations"] }, + ] + + for (const { label, change, parts } of partCases) { + it(`names the part for ${label}`, () => { + assert.deepStrictEqual(changedParts(parsedTool(), parsedTool(change)), parts) + }) + } + + it("reports nothing when a schema differs only in key order", () => { + const base = parsedTool({ inputSchema: { type: "object", properties: { path: { type: "string" } } } }) + const reordered = parsedTool({ inputSchema: { properties: { path: { type: "string" } }, type: "object" } }) + + assert.deepStrictEqual(changedParts(base, reordered), []) + }) + + it("reports a schema whose array order changed", () => { + const base = parsedTool({ inputSchema: { type: "object", required: ["path", "body"] } }) + const reordered = parsedTool({ inputSchema: { type: "object", required: ["body", "path"] } }) + + assert.deepStrictEqual(changedParts(base, reordered), ["inputSchema"]) + }) +}) + +describe("compareSurfaces", () => { + it("reports a description edit with its lines, sizes, and scope", () => { + const report = compare([rawTool()], [rawTool({ description: "List notes!" })]) + + assert.deepStrictEqual(report, { + current: "current.json", + base: "base.json", + changed: ["list_notes"], + added: [], + removed: [], + unchanged: [], + orderOnly: [], + inScope: ["list_notes"], + changes: [ + { + name: "list_notes", + parts: ["description"], + sizeBefore: LIST_NOTES_CHARS, + sizeAfter: LIST_NOTES_CHARS, + descriptionLines: { added: ["List notes!"], removed: ["List notes."] }, + schemaSentences: { added: [], removed: [] }, + }, + ], + sizes: [ + { name: "list_notes", description: 11, inputSchema: EMPTY_SCHEMA_CHARS, outputSchema: 0, total: LIST_NOTES_CHARS }, + ], + totalSize: { base: LIST_NOTES_CHARS, current: LIST_NOTES_CHARS }, + sharedEdits: [], + sections: [], + duplicationCandidates: [], + }) + }) + + it("reports a removed line when a description loses one of two identical lines", () => { + const report = compare( + [rawTool({ description: "List notes.\n- repeated\n- repeated" })], + [rawTool({ description: "List notes.\n- repeated" })], + ) + + assert.deepStrictEqual( + report.changes.map((change) => change.descriptionLines), + [{ added: [], removed: ["- repeated"] }], + ) + }) + + it("keeps a key-order-only change out of scope and marks it order-only", () => { + const base = rawTool({ inputSchema: { type: "object", properties: {} } }) + const reordered = rawTool({ inputSchema: { properties: {}, type: "object" } }) + + assert.deepStrictEqual(compare([base], [reordered]), { + current: "current.json", + base: "base.json", + changed: [], + added: [], + removed: [], + unchanged: ["list_notes"], + orderOnly: ["list_notes"], + inScope: [], + changes: [], + sizes: [], + totalSize: { base: LIST_NOTES_CHARS, current: LIST_NOTES_CHARS }, + sharedEdits: [], + sections: [], + duplicationCandidates: [], + }) + }) + + it("lists an added and a removed tool and puts only the added one in scope", () => { + const kept = rawTool({ name: "read_note", description: "" }) + const report = compare( + [rawTool({ name: "old_tool", description: "" }), kept], + [kept, rawTool({ name: "new_tool", description: "" })], + ) + + assert.deepStrictEqual(report, { + current: "current.json", + base: "base.json", + changed: [], + added: ["new_tool"], + removed: ["old_tool"], + unchanged: ["read_note"], + orderOnly: [], + inScope: ["new_tool"], + changes: [], + sizes: [ + { name: "new_tool", description: 0, inputSchema: EMPTY_SCHEMA_CHARS, outputSchema: 0, total: EMPTY_SCHEMA_CHARS }, + ], + totalSize: { base: 2 * EMPTY_SCHEMA_CHARS, current: 2 * EMPTY_SCHEMA_CHARS }, + sharedEdits: [], + sections: [], + duplicationCandidates: [], + }) + }) + + it("puts every tool in scope when there is no base", () => { + assert.deepStrictEqual(compare(null, [rawTool(), rawTool({ name: "read_note", description: "" })]), { + current: "current.json", + base: null, + changed: [], + added: [], + removed: [], + unchanged: [], + orderOnly: [], + inScope: ["list_notes", "read_note"], + changes: [], + sizes: [ + { name: "list_notes", description: 11, inputSchema: EMPTY_SCHEMA_CHARS, outputSchema: 0, total: LIST_NOTES_CHARS }, + { name: "read_note", description: 0, inputSchema: EMPTY_SCHEMA_CHARS, outputSchema: 0, total: EMPTY_SCHEMA_CHARS }, + ], + totalSize: { base: null, current: LIST_NOTES_CHARS + EMPTY_SCHEMA_CHARS }, + sharedEdits: [], + sections: [], + duplicationCandidates: [], + }) + }) + + it("groups a line added to two tools into one shared edit and leaves a one-tool line out", () => { + const sharedLine = '- "path must end in .md" — add the extension' + const report = compare( + [rawTool({ name: "read_note", description: "Read." }), rawTool({ name: "write_note", description: "Write." })], + [ + rawTool({ name: "read_note", description: `Read.\n${sharedLine}` }), + rawTool({ name: "write_note", description: `Write.\n${sharedLine}\n- only here` }), + ], + ) + + assert.deepStrictEqual(report.sharedEdits, [ + { text: sharedLine, where: "description", change: "added", tools: ["read_note", "write_note"] }, + ]) + }) + + it("groups a sentence added to two tools' schema descriptions into one shared edit", () => { + const report = compare( + [ + rawTool({ name: "read_note", inputSchema: pathSchema("Path to read.") }), + rawTool({ name: "write_note", inputSchema: pathSchema("Path to write.") }), + ], + [ + rawTool({ name: "read_note", inputSchema: pathSchema("Path to read. Use the exact letter case.") }), + rawTool({ name: "write_note", inputSchema: pathSchema("Path to write. Use the exact letter case.") }), + ], + ) + + assert.deepStrictEqual(report.sharedEdits, [ + { text: "Use the exact letter case.", where: "schema", change: "added", tools: ["read_note", "write_note"] }, + ]) + }) + + it("groups a line removed from two tools into one shared edit", () => { + const sharedLine = "- Hidden paths are not editable, matching Obsidian" + const report = compare( + [ + rawTool({ name: "read_note", description: `Read.\n${sharedLine}` }), + rawTool({ name: "write_note", description: `Write.\n${sharedLine}` }), + ], + [rawTool({ name: "read_note", description: "Read." }), rawTool({ name: "write_note", description: "Write." })], + ) + + assert.deepStrictEqual(report.sharedEdits, [ + { text: sharedLine, where: "description", change: "removed", tools: ["read_note", "write_note"] }, + ]) + }) + + it("groups a sentence removed from two tools' schema descriptions into one shared edit", () => { + const report = compare( + [ + rawTool({ name: "read_note", inputSchema: pathSchema("Path to read. Must end in md.") }), + rawTool({ name: "write_note", inputSchema: pathSchema("Path to write. Must end in md.") }), + ], + [ + rawTool({ name: "read_note", inputSchema: pathSchema("Path to read.") }), + rawTool({ name: "write_note", inputSchema: pathSchema("Path to write.") }), + ], + ) + + assert.deepStrictEqual(report.sharedEdits, [ + { text: "Must end in md.", where: "schema", change: "removed", tools: ["read_note", "write_note"] }, + ]) + }) + + it("marks a snapshot's sections as not compared when there is no base", () => { + const current = surfaceOf([rawTool()], { instructions: "Read first.", prompts: [] }) + + assert.deepStrictEqual(compareSurfaces(null, current, { base: null, current: "current.json" }).sections, [ + { name: "instructions", changed: null }, + { name: "prompts", changed: null }, + ]) + }) + + it("reports which of a snapshot's sections changed", () => { + const base = surfaceOf([rawTool()], { instructions: "Read first.", prompts: [] }) + const current = surfaceOf([rawTool()], { instructions: "Read this first.", prompts: [] }) + + assert.deepStrictEqual(compareSurfaces(base, current, PATHS).sections, [ + { name: "instructions", changed: true }, + { name: "prompts", changed: false }, + ]) + }) + + it("marks a section present only in the base as changed", () => { + const base = surfaceOf([rawTool()], { instructions: "Read first." }) + const current = surfaceOf([rawTool()]) + + assert.deepStrictEqual(compareSurfaces(base, current, PATHS).sections, [ + { name: "instructions", changed: true }, + ]) + }) + + it("marks a section present only in the current as changed", () => { + const base = surfaceOf([rawTool()]) + const current = surfaceOf([rawTool()], { prompts: [{ name: "daily" }] }) + + assert.deepStrictEqual(compareSurfaces(base, current, PATHS).sections, [ + { name: "prompts", changed: true }, + ]) + }) + + it("includes outputSchema in the size total", () => { + const outputSchema = { type: "object", properties: { count: { type: "number" } } } + const report = compare(null, [rawTool({ outputSchema })]) + const outputSchemaChars = JSON.stringify(outputSchema).length + + assert.deepStrictEqual(report.sizes, [ + { + name: "list_notes", + description: 11, + inputSchema: EMPTY_SCHEMA_CHARS, + outputSchema: outputSchemaChars, + total: 11 + EMPTY_SCHEMA_CHARS + outputSchemaChars, + }, + ]) + }) +}) + +describe("findSharedEdits", () => { + it("keeps edits that touched only one tool out of the result", () => { + const report = compare( + [rawTool({ name: "read_note", description: "Read." })], + [rawTool({ name: "read_note", description: "Read a note." })], + ) + + assert.deepStrictEqual( + { changed: report.changed, sharedEdits: report.sharedEdits }, + { changed: ["read_note"], sharedEdits: [] }, + ) + }) +}) + +describe("duplication candidates", () => { + const overlapOf = (length: number) => "s".repeat(length) + + it("ignores an overlap one character under the threshold", () => { + assert.deepStrictEqual(commonSubstrings(`1${overlapOf(39)}2`, `3${overlapOf(39)}4`), []) + }) + + it("reports an overlap at the threshold", () => { + assert.deepStrictEqual(commonSubstrings(`1${overlapOf(40)}2`, `3${overlapOf(40)}4`), [overlapOf(40)]) + }) + + it("reports every separate overlap in one text, longest first", () => { + const shorter = "a".repeat(45) + const longer = "b".repeat(50) + + assert.deepStrictEqual(commonSubstrings(`${shorter}|${longer}`, `${longer}#${shorter}`), [longer, shorter]) + }) + + it("reports an overlap once when the text states it twice", () => { + const repeated = overlapOf(40) + + assert.deepStrictEqual(commonSubstrings(`x${repeated}y${repeated}`, `${repeated}z`), [repeated]) + }) + + it("drops a candidate that reaches the threshold only by counting a neighbouring space", () => { + const report = compare(null, [ + rawTool({ description: `3 ${overlapOf(39)}4`, inputSchema: pathSchema(`1 ${overlapOf(39)}2`) }), + ]) + + assert.deepStrictEqual(report.duplicationCandidates, []) + }) + + it("leaves half of a two-unit character out of a candidate's text", () => { + // 😀 and 🨀 share their second code unit, and 😀 and 😁 share their first, so the raw overlap starts and ends mid-character. + const report = compare(null, [ + rawTool({ description: `🨀${overlapOf(40)}😀`, inputSchema: pathSchema(`😀${overlapOf(40)}😁`) }), + ]) + + assert.deepStrictEqual(report.duplicationCandidates, [ + { tool: "list_notes", parameter: "path", text: overlapOf(40), length: 40, preExisting: false }, + ]) + }) + + it("finds described parameters in nested properties, items, and anyOf branches", () => { + const schema = { + type: "object", + description: "The root is not a parameter.", + properties: { + path: { type: "string", description: "Top level." }, + filters: { + type: "object", + properties: { tags: { type: "array", items: { type: "string", description: "One tag." } } }, + }, + position: { anyOf: [{ type: "string", description: "Top or bottom." }, { type: "integer" }] }, + }, + } + + assert.deepStrictEqual(parameterTexts(schema), [ + { parameter: "path", text: "Top level." }, + { parameter: "filters.tags[]", text: "One tag." }, + { parameter: "position", text: "Top or bottom." }, + ]) + }) + + it("follows oneOf and allOf branches the same way as anyOf", () => { + const schema = { + type: "object", + properties: { + mode: { oneOf: [{ type: "string", description: "Named mode." }] }, + spec: { allOf: [{ description: "Base constraints." }, { description: "Extended constraints." }] }, + }, + } + + assert.deepStrictEqual(parameterTexts(schema), [ + { parameter: "mode", text: "Named mode." }, + { parameter: "spec", text: "Base constraints." }, + { parameter: "spec", text: "Extended constraints." }, + ]) + }) + + it("returns nothing when the schema is not an object", () => { + assert.deepStrictEqual(parameterTexts("not a schema"), []) + assert.deepStrictEqual(parameterTexts(undefined), []) + }) + + it("labels an overlap the base already had and still reports a new one in the same parameter", () => { + const oldOverlap = "o".repeat(60) + const newOverlap = "n".repeat(45) + const report = compare( + [rawTool({ description: `Old: ${oldOverlap}`, inputSchema: pathSchema(`1${oldOverlap}2`) })], + [ + rawTool({ + description: `Old: ${oldOverlap} New: ${newOverlap}`, + inputSchema: pathSchema(`1${oldOverlap}2 3${newOverlap}4`), + }), + ], + ) + + assert.deepStrictEqual(report.duplicationCandidates, [ + { tool: "list_notes", parameter: "path", text: oldOverlap, length: 60, preExisting: true }, + { tool: "list_notes", parameter: "path", text: newOverlap, length: 45, preExisting: false }, + ]) + }) +}) + +describe("duplication candidates across a change", () => { + it("keeps an overlap pre-existing when the change only adds a sentence after it", () => { + const repeated = "The note must already exist and must end in md." + const report = compare( + [rawTool({ description: `Path rules: ${repeated}`, inputSchema: pathSchema(repeated) })], + [rawTool({ description: `Path rules: ${repeated}`, inputSchema: pathSchema(`${repeated} Use the exact letter case.`) })], + ) + + assert.deepStrictEqual(report.duplicationCandidates, [ + { tool: "list_notes", parameter: "path", text: repeated, length: 47, preExisting: true }, + ]) + }) +}) + +describe("listVariants", () => { + it("names the tools another configuration words differently, and the tools only one side has", () => { + const current = surfaceOf([ + rawTool({ name: "search", description: "Hybrid search." }), + rawTool({ name: "read_note" }), + rawTool({ name: "recall" }), + ]) + const embeddingOff = surfaceOf([ + rawTool({ name: "search", description: "Full-text search." }), + rawTool({ name: "read_note" }), + rawTool({ name: "reindex" }), + ]) + + assert.deepStrictEqual(listVariants(current, [{ file: "embedding-off.json", surface: embeddingOff }]), [ + { + file: "embedding-off.json", + differing: [{ name: "search", parts: ["description"] }], + onlyInCurrent: ["recall"], + onlyInVariant: ["reindex"], + }, + ]) + }) +}) + +describe("showTools", () => { + it("prints the named tools as text, with the description's own line breaks", () => { + const surface = surfaceOf([ + rawTool({ description: "List notes.\n\nReturns: paths.", title: "List", annotations: { readOnlyHint: true } }), + rawTool({ name: "read_note", description: "Read a note.", outputSchema: { type: "object" } }), + rawTool({ name: "not_asked_for" }), + ]) + + assert.strictEqual( + showTools(surface, ["list_notes", "read_note"], "test"), + [ + "=== list_notes ===", + "title: List", + "description:", + "List notes.", + "", + "Returns: paths.", + "", + "inputSchema:", + "{", + ' "type": "object",', + ' "properties": {}', + "}", + "", + 'annotations: {"readOnlyHint":true}', + "", + "=== read_note ===", + "description:", + "Read a note.", + "", + "inputSchema:", + "{", + ' "type": "object",', + ' "properties": {}', + "}", + "", + "outputSchema:", + "{", + ' "type": "object"', + "}", + ].join("\n"), + ) + }) + + it("rejects a name the file does not hold", () => { + assert.throws(() => showTools(surfaceOf([rawTool()]), ["read_note"], "test"), { + constructor: InputError, + message: 'test: no tool named "read_note"', + }) + }) +}) + +describe("command line", () => { + const directory = mkdtempSync(join(tmpdir(), "surface-diff-test-")) + after(() => rmSync(directory, { recursive: true, force: true })) + + const writeFile = (name: string, content: string) => { + const path = join(directory, name) + writeFileSync(path, content) + return path + } + + const writeSurface = (name: string, tools: unknown[]) => writeFile(name, JSON.stringify({ tools })) + + const runScript = (args: string[]) => { + const { status, stdout, stderr } = spawnSync(process.execPath, [SCRIPT_PATH, ...args], { encoding: "utf8" }) + return { status, stdout, stderr } + } + + it("prints only names with --names", () => { + const base = writeSurface("names-base.json", [rawTool(), rawTool({ name: "read_note" })]) + const current = writeSurface("names-current.json", [rawTool({ description: "List every note." }), rawTool({ name: "read_note" })]) + + const { status, stdout, stderr } = runScript(["--current", current, "--base", base, "--names"]) + + assert.deepStrictEqual( + { status, stderr, output: JSON.parse(stdout) }, + { + status: 0, + stderr: "", + output: { + changed: ["list_notes"], + added: [], + removed: [], + unchanged: ["read_note"], + orderOnly: [], + inScope: ["list_notes"], + }, + }, + ) + }) + + it("lists another configuration's differing tools with --variants", () => { + const current = writeSurface("variants-current.json", [rawTool()]) + const variant = writeSurface("variants-readonly.json", [rawTool({ description: "List notes, read-only." })]) + + const { status, stdout, stderr } = runScript(["--current", current, "--variants", variant]) + + assert.deepStrictEqual( + { status, stderr, output: JSON.parse(stdout) }, + { + status: 0, + stderr: "", + output: { + current, + variants: [ + { + file: variant, + differing: [{ name: "list_notes", parts: ["description"] }], + onlyInCurrent: [], + onlyInVariant: [], + }, + ], + }, + }, + ) + }) + + it("exits 2 with the usage when --current is missing", () => { + assert.deepStrictEqual(runScript([]), { status: 2, stdout: "", stderr: `${USAGE}\n` }) + }) + + it("exits 2 when the file cannot be read", () => { + const missing = join(directory, "missing.json") + + assert.deepStrictEqual(runScript(["--current", missing]), { + status: 2, + stdout: "", + stderr: `${missing}: cannot be read\n`, + }) + }) + + it("exits 2 when the file is not JSON", () => { + const broken = writeFile("broken.json", "{ not json") + + assert.deepStrictEqual(runScript(["--current", broken]), { + status: 2, + stdout: "", + stderr: `${broken}: not valid JSON\n`, + }) + }) + + it("exits 2 when the file holds one page of a paginated list", () => { + const page = writeFile("page.json", JSON.stringify({ tools: [rawTool()], nextCursor: "page-2" })) + + assert.deepStrictEqual(runScript(["--current", page]), { + status: 2, + stdout: "", + stderr: `${page}: has "nextCursor", so it is one page of a longer list; capture every page\n`, + }) + }) + + it("prints a tool as text with --show", () => { + const current = writeSurface("show-current.json", [rawTool({ description: "List notes.\nSecond line." })]) + + assert.deepStrictEqual(runScript(["--current", current, "--show", "list_notes"]), { + status: 0, + stdout: [ + "=== list_notes ===", + "description:", + "List notes.", + "Second line.", + "", + "inputSchema:", + "{", + ' "type": "object",', + ' "properties": {}', + "}", + "", + ].join("\n"), + stderr: "", + }) + }) + + it("exits 2 when --show is combined with --base", () => { + const current = writeSurface("show-combined.json", [rawTool()]) + + assert.deepStrictEqual(runScript(["--current", current, "--base", current, "--show", "list_notes"]), { + status: 2, + stdout: "", + stderr: "--show prints tools from --current; it cannot be combined with --base, --names, or --variants\n", + }) + }) + + it("exits 2 when --variants is combined with --base", () => { + const current = writeSurface("combined-current.json", [rawTool()]) + + assert.deepStrictEqual(runScript(["--current", current, "--base", current, "--variants", current]), { + status: 2, + stdout: "", + stderr: "--variants lists other configurations; it cannot be combined with --base or --names\n", + }) + }) + + it("exits 2 when --show is combined with --variants", () => { + const current = writeSurface("show-variants.json", [rawTool()]) + + assert.deepStrictEqual(runScript(["--current", current, "--variants", current, "--show", "list_notes"]), { + status: 2, + stdout: "", + stderr: "--show prints tools from --current; it cannot be combined with --base, --names, or --variants\n", + }) + }) + + it("exits 2 when --names is combined with --show", () => { + const current = writeSurface("show-names.json", [rawTool()]) + + assert.deepStrictEqual(runScript(["--current", current, "--names", "--show", "list_notes"]), { + status: 2, + stdout: "", + stderr: "--show prints tools from --current; it cannot be combined with --base, --names, or --variants\n", + }) + }) + + it("exits 2 when --names is combined with --variants", () => { + const current = writeSurface("variants-names.json", [rawTool()]) + + assert.deepStrictEqual(runScript(["--current", current, "--names", "--variants", current]), { + status: 2, + stdout: "", + stderr: "--variants lists other configurations; it cannot be combined with --base or --names\n", + }) + }) + + it("exits 2 with usage when an unknown flag is passed", () => { + assert.deepStrictEqual(runScript(["--bogus"]), { + status: 2, + stdout: "", + stderr: `Unknown option '--bogus'\n${USAGE}\n`, + }) + }) + + it("prints a full report with --current and --base", () => { + const base = writeSurface("full-base.json", [rawTool()]) + const current = writeSurface("full-current.json", [rawTool({ description: "List every note." })]) + + const { status, stdout, stderr } = runScript(["--current", current, "--base", base]) + const output = JSON.parse(stdout) + + assert.deepStrictEqual( + { status, stderr, current: output.current, base: output.base, changed: output.changed, fields: Object.keys(output) }, + { status: 0, stderr: "", current, base, changed: ["list_notes"], fields: REPORT_FIELDS }, + ) + }) + + it("prints a full report with --current only", () => { + const current = writeSurface("solo-current.json", [rawTool()]) + + const { status, stdout, stderr } = runScript(["--current", current]) + const output = JSON.parse(stdout) + + assert.deepStrictEqual( + { status, stderr, current: output.current, base: output.base, inScope: output.inScope, fields: Object.keys(output) }, + { status: 0, stderr: "", current, base: null, inScope: ["list_notes"], fields: REPORT_FIELDS }, + ) + }) + + it("exits 2 when --show names a tool the file does not hold", () => { + const current = writeSurface("show-missing.json", [rawTool()]) + + assert.deepStrictEqual(runScript(["--current", current, "--show", "no_such_tool"]), { + status: 2, + stdout: "", + stderr: `${current}: no tool named "no_such_tool"\n`, + }) + }) +}) diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts new file mode 100644 index 0000000..155d572 --- /dev/null +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -0,0 +1,622 @@ +#!/usr/bin/env bun +import { readFileSync, realpathSync } from "node:fs" +import { fileURLToPath } from "node:url" +import { parseArgs } from "node:util" + +type JsonValue = string | number | boolean | null | JsonValue[] | JsonObject +type JsonObject = { [key: string]: JsonValue } + +export type Tool = { + name: string + inputSchema: JsonObject + description: string | undefined + title: string | undefined + outputSchema: JsonObject | undefined + annotations: JsonObject | undefined +} + +// `sections` holds the server's `instructions` and `prompts` entries (see SECTION_KEYS). +export type Surface = { tools: Tool[]; sections: JsonObject } + +type Part = "description" | "inputSchema" | "outputSchema" | "title" | "annotations" + +type ToolSize = { name: string; description: number; inputSchema: number; outputSchema: number; total: number } + +type TextChange = { added: string[]; removed: string[] } + +type ToolChange = { + name: string + parts: Part[] + sizeBefore: number + sizeAfter: number + descriptionLines: TextChange + schemaSentences: TextChange +} + +type SharedEdit = { + text: string + where: "description" | "schema" + change: "added" | "removed" + tools: string[] +} + +// `preExisting` is true when the base surface already had the same overlap, so the change did not introduce it. +type Candidate = { tool: string; parameter: string; text: string; length: number; preExisting: boolean } + +type ParameterText = { parameter: string; text: string } + +// `changed` is null when there is no base, so "not compared" never reads as "unchanged". +type SectionChange = { name: string; changed: boolean | null } + +export type Report = { + current: string + base: string | null + changed: string[] + added: string[] + removed: string[] + unchanged: string[] + // Tools whose schemas or annotations differ only in JSON key order: same meaning, different serialisation. + orderOnly: string[] + inScope: string[] + changes: ToolChange[] + sizes: ToolSize[] + totalSize: { base: number | null; current: number } + sharedEdits: SharedEdit[] + sections: SectionChange[] + duplicationCandidates: Candidate[] +} + +type VariantListing = { + file: string + differing: { name: string; parts: Part[] }[] + onlyInCurrent: string[] + onlyInVariant: string[] +} + +/** Thrown for anything wrong with the arguments or the input files; the CLI exits 2 on it. */ +export class InputError extends Error {} + +// Overlaps shorter than this are mostly stock phrases a description and its +// schema both need ("Vault-relative path to the note"), not a repeated fact. +export const MIN_OVERLAP_CHARS = 40 + +// Top-level keys a snapshot file can carry beside its tool list. +const SECTION_KEYS = ["instructions", "prompts"] + +const PARTS: Part[] = ["description", "inputSchema", "outputSchema", "title", "annotations"] + +const SCHEMA_BRANCH_KEYS = ["anyOf", "oneOf", "allOf"] + +// The whitespace after a sentence-ending mark; splitting on it keeps the mark with its sentence. +const SENTENCE_BOUNDARY = /(?<=[.!?])\s+/ + +// A reviewer passes tool names to `--show` in a shell command, so a name from an untrusted file must not be able to carry shell syntax. +const COMMAND_LINE_SAFE_NAME = /^[A-Za-z0-9_.:/-]+$/ + +// Any run of spaces, tabs, or newlines. +const WHITESPACE_RUN = /\s+/g + +// Overlaps are cut by UTF-16 code unit, so an edge can hold one half of a two-unit character such as an emoji. +const HALF_CHARACTER_AT_EDGE = /^[\uDC00-\uDFFF]|[\uD800-\uDBFF]$/g + +const USAGE = [ + "Usage: surface-diff.ts --current [--base ] [--names]", + " surface-diff.ts --current --variants [--variants ...]", + " surface-diff.ts --current --show [--show ...]", +].join("\n") + +const isJsonObject = (value: unknown): value is JsonObject => { + return typeof value === "object" && value !== null && !Array.isArray(value) +} + +const optionalString = (value: unknown, field: string, where: string): string | undefined => { + if (value === undefined || typeof value === "string") return value + throw new InputError(`${where}: "${field}" must be a string`) +} + +const optionalObject = (value: unknown, field: string, where: string): JsonObject | undefined => { + if (value === undefined || isJsonObject(value)) return value + throw new InputError(`${where}: "${field}" must be an object`) +} + +const parseTool = (value: unknown, position: number, label: string): Tool => { + if (!isJsonObject(value)) { + throw new InputError(`${label}: tool ${position} is not an object`) + } + + const { name, inputSchema } = value + + if (typeof name !== "string" || !name) { + throw new InputError(`${label}: tool ${position} has no string "name"`) + } + + if (!COMMAND_LINE_SAFE_NAME.test(name)) { + throw new InputError( + `${label}: tool ${position} is named ${JSON.stringify(name)}; a name may hold only letters, digits, and _ . : / - because the reviewer puts names on a command line`, + ) + } + + const where = `${label}: tool "${name}"` + + if (!isJsonObject(inputSchema)) { + throw new InputError(`${where}: "inputSchema" must be an object`) + } + + return { + name, + inputSchema, + description: optionalString(value.description, "description", where), + title: optionalString(value.title, "title", where), + outputSchema: optionalObject(value.outputSchema, "outputSchema", where), + annotations: optionalObject(value.annotations, "annotations", where), + } +} + +const findToolList = (parsed: unknown, label: string): { tools: unknown[]; container: JsonObject | null } => { + if (Array.isArray(parsed)) return { tools: parsed, container: null } + if (isJsonObject(parsed) && Array.isArray(parsed.tools)) return { tools: parsed.tools, container: parsed } + if (isJsonObject(parsed) && isJsonObject(parsed.result) && Array.isArray(parsed.result.tools)) { + return { tools: parsed.result.tools, container: parsed.result } + } + + throw new InputError( + `${label}: not a tool list (expected a "tools" array, a bare array of tools, or a JSON-RPC result holding "tools")`, + ) +} + +export const parseSurface = (parsed: unknown, label: string): Surface => { + const { tools: rawTools, container } = findToolList(parsed, label) + + // A cursor means the server had more tools to send; comparing one page would report the rest as removed. + if (container?.nextCursor) { + throw new InputError(`${label}: has "nextCursor", so it is one page of a longer list; capture every page`) + } + + const tools = rawTools.map((rawTool, index) => parseTool(rawTool, index + 1, label)) + + const seenNames = new Set() + + for (const { name } of tools) { + if (seenNames.has(name)) { + throw new InputError(`${label}: two tools are named "${name}"`) + } + + seenNames.add(name) + } + + const sectionEntries = Object.entries(container ?? {}).filter(([key]) => SECTION_KEYS.includes(key)) + + return { tools, sections: Object.fromEntries(sectionEntries) } +} + +const readText = (path: string): string => { + try { + return readFileSync(path, "utf8") + } catch { + throw new InputError(`${path}: cannot be read`) + } +} + +const parseJson = (text: string, path: string): unknown => { + try { + return JSON.parse(text) + } catch { + throw new InputError(`${path}: not valid JSON`) + } +} + +const loadSurface = (path: string): Surface => parseSurface(parseJson(readText(path), path), path) + +/** Sorts object keys at every depth, so schemas that differ only in key order serialise alike. Array order is kept, because it is part of a schema's meaning. */ +const canonicalize = (value: JsonValue): JsonValue => { + if (Array.isArray(value)) { + return value.map(canonicalize) + } + + if (!isJsonObject(value)) { + return value + } + + const sortedEntries = Object.entries(value).toSorted(([leftKey], [rightKey]) => (leftKey < rightKey ? -1 : 1)) + return Object.fromEntries(sortedEntries.map(([key, child]) => [key, canonicalize(child)])) +} + +// An absent part serialises as the empty string, which no present value does, so absent and present never compare equal. +const canonicalJson = (value: JsonValue | undefined): string => { + return value === undefined ? "" : JSON.stringify(canonicalize(value)) +} + +// Same as `canonicalJson` but without sorting keys, so two tools that differ only in key order produce different strings. +const wireJson = (value: JsonValue | undefined): string => (value === undefined ? "" : JSON.stringify(value)) + +export const changedParts = (base: Tool, current: Tool): Part[] => { + return PARTS.filter((part) => canonicalJson(base[part]) !== canonicalJson(current[part])) +} + +const differsOnlyInKeyOrder = (base: Tool, current: Tool): boolean => { + const sameMeaning = changedParts(base, current).length === 0 + return sameMeaning && PARTS.some((part) => wireJson(base[part]) !== wireJson(current[part])) +} + +// A tool may ship no description; it is compared and measured as empty text. +const descriptionOf = (tool: Tool | undefined): string => tool?.description ?? "" + +const toolSize = (tool: Tool): ToolSize => { + const description = descriptionOf(tool).length + const inputSchema = JSON.stringify(tool.inputSchema).length + const outputSchema = tool.outputSchema ? JSON.stringify(tool.outputSchema).length : 0 + + return { name: tool.name, description, inputSchema, outputSchema, total: description + inputSchema + outputSchema } +} + +const sumSizes = (tools: Tool[]): number => tools.reduce((sum, tool) => sum + toolSize(tool).total, 0) + +const nonEmptyTrimmed = (texts: string[]): string[] => texts.map((text) => text.trim()).filter(Boolean) + +const descriptionLines = (tool: Tool): string[] => nonEmptyTrimmed(descriptionOf(tool).split("\n")) + +const collectDescriptions = (schema: JsonValue | undefined): string[] => { + if (Array.isArray(schema)) { + return schema.flatMap(collectDescriptions) + } + + if (!isJsonObject(schema)) { + return [] + } + + const own = typeof schema.description === "string" ? [schema.description] : [] + return [...own, ...Object.values(schema).flatMap(collectDescriptions)] +} + +const schemaSentences = (tool: Tool): string[] => { + const descriptions = [...collectDescriptions(tool.inputSchema), ...collectDescriptions(tool.outputSchema)] + return descriptions.flatMap((description) => nonEmptyTrimmed(description.split(SENTENCE_BOUNDARY))) +} + +const countCopies = (texts: string[]): Map => { + const copies = new Map() + + for (const text of texts) { + copies.set(text, (copies.get(text) ?? 0) + 1) + } + + return copies +} + +const textsWithMoreCopies = (copies: Map, thanIn: Map): string[] => { + return [...copies.keys()].filter((text) => (copies.get(text) ?? 0) > (thanIn.get(text) ?? 0)) +} + +/** Compares how many copies of each text there are, so losing one of two identical lines is still a removal. Each text is listed once. */ +const textChange = (before: string[], after: string[]): TextChange => { + const copiesBefore = countCopies(before) + const copiesAfter = countCopies(after) + + return { + added: textsWithMoreCopies(copiesAfter, copiesBefore), + removed: textsWithMoreCopies(copiesBefore, copiesAfter), + } +} + +const describeChange = (base: Tool, current: Tool): ToolChange => { + return { + name: current.name, + parts: changedParts(base, current), + sizeBefore: toolSize(base).total, + sizeAfter: toolSize(current).total, + descriptionLines: textChange(descriptionLines(base), descriptionLines(current)), + schemaSentences: textChange(schemaSentences(base), schemaSentences(current)), + } +} + +const editsOf = ( + { name, descriptionLines: lines, schemaSentences: sentences }: ToolChange, +): SharedEdit[] => { + return [ + ...lines.added.map((text) => ({ text, where: "description" as const, change: "added" as const, tools: [name] })), + ...lines.removed.map((text) => ({ text, where: "description" as const, change: "removed" as const, tools: [name] })), + ...sentences.added.map((text) => ({ text, where: "schema" as const, change: "added" as const, tools: [name] })), + ...sentences.removed.map((text) => ({ text, where: "schema" as const, change: "removed" as const, tools: [name] })), + ] +} + +/** Groups identical added or removed text across tools. Text that touched one tool stays on that tool's own change record. */ +export const findSharedEdits = (changes: ToolChange[]): SharedEdit[] => { + const editsByKey = new Map() + + for (const edit of changes.flatMap(editsOf)) { + const key = JSON.stringify([edit.where, edit.change, edit.text]) + const toolsSoFar = editsByKey.get(key)?.tools ?? [] + editsByKey.set(key, { ...edit, tools: [...toolsSoFar, ...edit.tools] }) + } + + return [...editsByKey.values()].filter((edit) => edit.tools.length > 1) +} + +const collapseWhitespace = (text: string): string => text.replace(WHITESPACE_RUN, " ").trim() + +const childPath = (path: string, name: string): string => (path ? `${path}.${name}` : name) + +const branchTexts = (schema: JsonObject, path: string): ParameterText[] => { + return SCHEMA_BRANCH_KEYS.flatMap((key) => { + const options = schema[key] + return Array.isArray(options) ? options.flatMap((option) => parameterTexts(option, path)) : [] + }) +} + +/** Every described parameter in a schema, with a dotted path. Branches of anyOf, oneOf, and allOf describe the same parameter, so they keep its path. */ +export const parameterTexts = (schema: JsonValue | undefined, path = ""): ParameterText[] => { + if (!isJsonObject(schema)) return [] + + // The root schema's own description belongs to no parameter. + const describesParameter = path !== "" && typeof schema.description === "string" + const own = describesParameter ? [{ parameter: path, text: String(schema.description) }] : [] + + const properties = isJsonObject(schema.properties) ? Object.entries(schema.properties) : [] + const nested = properties.flatMap(([name, child]) => parameterTexts(child, childPath(path, name))) + + return [...own, ...nested, ...parameterTexts(schema.items, `${path}[]`), ...branchTexts(schema, path)] +} + +const longestCommonSubstring = (left: string, right: string): string => { + // Dynamic programming over two rows; bestLength/bestEnd track the winner so far. + let bestLength = 0 + let bestEnd = 0 + let previousRow = new Uint32Array(right.length + 1) + + for (let leftIndex = 1; leftIndex <= left.length; leftIndex += 1) { + const row = new Uint32Array(right.length + 1) + + for (let rightIndex = 1; rightIndex <= right.length; rightIndex += 1) { + if (left[leftIndex - 1] !== right[rightIndex - 1]) continue + + const runLength = (previousRow[rightIndex - 1] ?? 0) + 1 + row[rightIndex] = runLength + + if (runLength > bestLength) { + bestLength = runLength + bestEnd = leftIndex + } + } + + previousRow = row + } + + return left.slice(bestEnd - bestLength, bestEnd) +} + +/** Every non-overlapping stretch of `text`, at least MIN_OVERLAP_CHARS long, that also appears in `other`. Longest first. */ +export const commonSubstrings = (text: string, other: string): string[] => { + const longest = longestCommonSubstring(text, other) + + if (longest.length < MIN_OVERLAP_CHARS) return [] + + const start = text.indexOf(longest) + const before = text.slice(0, start) + const after = text.slice(start + longest.length) + + // When the same substring appears twice in `text`, both halves produce it, so the Set keeps one copy. + const overlaps = new Set([longest, ...commonSubstrings(before, other), ...commonSubstrings(after, other)]) + return [...overlaps].toSorted((leftText, rightText) => rightText.length - leftText.length) +} + +const findCandidates = (tool: Tool, base: Tool | undefined): Candidate[] => { + const description = collapseWhitespace(descriptionOf(tool)) + const baseDescription = collapseWhitespace(descriptionOf(base)) + const baseParameters = parameterTexts(base?.inputSchema) + + const baseRepeated = (parameter: string, overlap: string): boolean => { + const sameParameter = baseParameters.filter((baseParameter) => baseParameter.parameter === parameter) + const inBaseSchema = sameParameter.some((baseParameter) => collapseWhitespace(baseParameter.text).includes(overlap)) + + return inBaseSchema && baseDescription.includes(overlap) + } + + return parameterTexts(tool.inputSchema).flatMap(({ parameter, text }) => { + // An overlap loses a half character and the spaces at its edges, so it matches the base's + // repetition even when the change added a space beside it. The threshold is checked again + // because that trimming can shorten an overlap below it. + const overlaps = commonSubstrings(collapseWhitespace(text), description) + .map((overlap) => overlap.replace(HALF_CHARACTER_AT_EDGE, "").trim()) + .filter((overlap) => overlap.length >= MIN_OVERLAP_CHARS) + + return overlaps.map((overlap) => ({ + tool: tool.name, + parameter, + text: overlap, + length: overlap.length, + preExisting: baseRepeated(parameter, overlap), + })) + }) +} + +const compareSections = (base: Surface | null, current: Surface): SectionChange[] => { + const present = SECTION_KEYS.filter((key) => key in current.sections || (base !== null && key in base.sections)) + + return present.map((key) => ({ + name: key, + changed: base ? canonicalJson(base.sections[key]) !== canonicalJson(current.sections[key]) : null, + })) +} + +export const compareSurfaces = ( + base: Surface | null, + current: Surface, + paths: { base: string | null; current: string }, +): Report => { + const baseTools = base ? base.tools : [] + const baseByName = new Map(baseTools.map((tool) => [tool.name, tool])) + const currentNames = new Set(current.tools.map((tool) => tool.name)) + + const pairs = current.tools.flatMap((tool) => { + const baseTool = baseByName.get(tool.name) + return baseTool ? [{ baseTool, tool }] : [] + }) + + const changedPairs = pairs.filter(({ baseTool, tool }) => changedParts(baseTool, tool).length > 0) + const changes = changedPairs.map(({ baseTool, tool }) => describeChange(baseTool, tool)) + const changed = changes.map((change) => change.name) + + const added = base ? current.tools.filter((tool) => !baseByName.has(tool.name)).map((tool) => tool.name) : [] + const removed = baseTools.filter((tool) => !currentNames.has(tool.name)).map((tool) => tool.name) + const unchanged = pairs.map(({ tool }) => tool.name).filter((name) => !changed.includes(name)) + const orderOnlyPairs = pairs.filter(({ baseTool, tool }) => differsOnlyInKeyOrder(baseTool, tool)) + + // Without a base nothing is known to be untouched, so every tool is reviewed. + const inScopeNames = new Set(base ? [...changed, ...added] : currentNames) + const inScopeTools = current.tools.filter((tool) => inScopeNames.has(tool.name)) + + return { + current: paths.current, + base: paths.base, + changed, + added, + removed, + unchanged, + orderOnly: orderOnlyPairs.map(({ tool }) => tool.name), + inScope: inScopeTools.map((tool) => tool.name), + changes, + sizes: inScopeTools.map(toolSize), + totalSize: { base: base ? sumSizes(base.tools) : null, current: sumSizes(current.tools) }, + sharedEdits: findSharedEdits(changes), + sections: compareSections(base, current), + duplicationCandidates: inScopeTools.flatMap((tool) => findCandidates(tool, baseByName.get(tool.name))), + } +} + +const listVariant = (current: Surface, file: string, variant: Surface): VariantListing => { + const variantByName = new Map(variant.tools.map((tool) => [tool.name, tool])) + const currentNames = new Set(current.tools.map((tool) => tool.name)) + + const differing = current.tools.flatMap((tool) => { + const variantTool = variantByName.get(tool.name) + + if (!variantTool) return [] + + const parts = changedParts(tool, variantTool) + return parts.length > 0 ? [{ name: tool.name, parts }] : [] + }) + + return { + file, + differing, + onlyInCurrent: current.tools.filter((tool) => !variantByName.has(tool.name)).map((tool) => tool.name), + onlyInVariant: variant.tools.filter((tool) => !currentNames.has(tool.name)).map((tool) => tool.name), + } +} + +/** For each other configuration's file, the tools it words differently from `current`. */ +export const listVariants = (current: Surface, variants: { file: string; surface: Surface }[]): VariantListing[] => { + return variants.map(({ file, surface }) => listVariant(current, file, surface)) +} + +const formatTool = (tool: Tool): string => { + const title = tool.title ? [`title: ${tool.title}`] : [] + const outputSchema = tool.outputSchema ? ["", "outputSchema:", JSON.stringify(tool.outputSchema, null, 2)] : [] + const annotations = tool.annotations ? ["", `annotations: ${JSON.stringify(tool.annotations)}`] : [] + + return [ + `=== ${tool.name} ===`, + ...title, + "description:", + descriptionOf(tool), + "", + "inputSchema:", + JSON.stringify(tool.inputSchema, null, 2), + ...outputSchema, + ...annotations, + ].join("\n") +} + +/** The named tools as readable text. A surface file keeps each description on one long JSON line, which file viewers cut off. */ +export const showTools = (surface: Surface, names: string[], label: string): string => { + const toolsByName = new Map(surface.tools.map((tool) => [tool.name, tool])) + + const shown = names.map((name) => { + const tool = toolsByName.get(name) + + if (!tool) { + throw new InputError(`${label}: no tool named ${JSON.stringify(name)}`) + } + + return formatTool(tool) + }) + + return shown.join("\n\n") +} + +const readArguments = (argv: string[]) => { + try { + const { values } = parseArgs({ + args: argv, + options: { + current: { type: "string" }, + base: { type: "string" }, + names: { type: "boolean", default: false }, + variants: { type: "string", multiple: true }, + show: { type: "string", multiple: true }, + }, + }) + + return values + } catch (error) { + throw new InputError(`${error instanceof Error ? error.message : String(error)}\n${USAGE}`) + } +} + +const toJson = (value: unknown): string => JSON.stringify(value, null, 2) + +const run = (argv: string[]): string => { + const { current: currentPath, base: basePath, names, variants = [], show = [] } = readArguments(argv) + + if (!currentPath) { + throw new InputError(USAGE) + } + + const current = loadSurface(currentPath) + + if (show.length > 0) { + if (basePath || names || variants.length > 0) { + throw new InputError("--show prints tools from --current; it cannot be combined with --base, --names, or --variants") + } + + return showTools(current, show, currentPath) + } + + if (variants.length > 0) { + if (basePath || names) { + throw new InputError("--variants lists other configurations; it cannot be combined with --base or --names") + } + + const loadedVariants = variants.map((file) => ({ file, surface: loadSurface(file) })) + return toJson({ current: currentPath, variants: listVariants(current, loadedVariants) }) + } + + const base = basePath ? loadSurface(basePath) : null + const report = compareSurfaces(base, current, { base: basePath ?? null, current: currentPath }) + + if (!names) { + return toJson(report) + } + + const { changed, added, removed, unchanged, orderOnly, inScope } = report + return toJson({ changed, added, removed, unchanged, orderOnly, inScope }) +} + +const main = () => { + try { + console.log(run(process.argv.slice(2))) + } catch (error) { + if (!(error instanceof InputError)) throw error + + console.error(error.message) + process.exitCode = 2 + } +} + +// The plugin cache reaches this file through a symlink, so the two paths are compared after resolving links. +const invokedPath = process.argv[1] + +if (invokedPath && realpathSync(invokedPath) === realpathSync(fileURLToPath(import.meta.url))) { + main() +}