From 7ee64cfe1ac44de1d830c5fd99d184c8db9fc419 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:41:48 -0400 Subject: [PATCH 01/29] feat(ship-check): surface-diff script for comparing MCP tool lists Compares one tool-surface JSON file with the one before it: changed, added and removed tools, lines added to several tools at once, description text that repeats a schema description, and sizes. A listing mode names the tools another configuration words differently. Tests run on Node 22.18 and 24 in a new workflow, and the release archives leave test files out. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/auto_release.yml | 5 +- .github/workflows/manual_release.yml | 5 +- .github/workflows/test.yml | 32 ++ .../scripts/__tests__/surface-diff.test.mts | 514 ++++++++++++++++++ .../scripts/surface-diff.mts | 514 ++++++++++++++++++ 5 files changed, 1066 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/test.yml create mode 100644 plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts create mode 100644 plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts diff --git a/.github/workflows/auto_release.yml b/.github/workflows/auto_release.yml index e4ed7b6..80b2f79 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -99,13 +99,14 @@ jobs: 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/manual_release.yml b/.github/workflows/manual_release.yml index 373c7eb..92c0997 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -99,13 +99,14 @@ jobs: 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..a037c12 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,32 @@ +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 (Node ${{ matrix.node }}) + strategy: + matrix: + # 22.18 is the oldest Node that runs the scripts' TypeScript without a + # flag, and 24 is the current LTS. + node: ["22.18", "24"] + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: ${{ matrix.node }} + + # The scripts have no dependencies, so there is nothing to install. + - run: node --test "plugins/**/__tests__/*.test.mts" diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts new file mode 100644 index 0000000..e1fd181 --- /dev/null +++ b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts @@ -0,0 +1,514 @@ +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, +} from "../surface-diff.mts" + +const SCRIPT_PATH = fileURLToPath(new URL("../surface-diff.mts", import.meta.url)) + +const USAGE = [ + "Usage: surface-diff.mts --current [--base ] [--names]", + " surface-diff.mts --current --variants [--variants ...]", +].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" } + +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 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: "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("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("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 }, + ]) + }) +}) + +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("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("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("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("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("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\n", + }) + }) +}) diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts new file mode 100644 index 0000000..8e64254 --- /dev/null +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts @@ -0,0 +1,514 @@ +#!/usr/bin/env node +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 +} + +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[] +} + +type Candidate = { tool: string; parameter: string; text: string; length: number; preExisting: boolean } + +type ParameterText = { parameter: string; text: string } + +type SectionChange = { name: string; changed: boolean } + +export type Report = { + current: string + base: string | null + changed: string[] + added: string[] + removed: string[] + unchanged: string[] + 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+/ + +// Any run of spaces, tabs, or newlines. +const WHITESPACE_RUN = /\s+/g + +const USAGE = [ + "Usage: surface-diff.mts --current [--base ] [--names]", + " surface-diff.mts --current --variants [--variants ...]", +].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"`) + } + + 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)) +} + +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 textChange = (before: string[], after: string[]): TextChange => { + const beforeSet = new Set(before) + const afterSet = new Set(after) + + return { + added: [...afterSet].filter((text) => !beforeSet.has(text)), + removed: [...beforeSet].filter((text) => !afterSet.has(text)), + } +} + +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; the three counters are overwritten as the table is scanned. + 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) + + const overlaps = [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 }) => { + return commonSubstrings(collapseWhitespace(text), description).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 !== null && canonicalJson(base.sections[key]) !== canonicalJson(current.sections[key]), + })) +} + +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 assertSupportedRuntime = () => { + if (process.versions.bun) return + + // Node strips TypeScript types without a flag from 22.18. + const [major = 0, minor = 0] = process.versions.node.split(".").map(Number) + if (major > 22 || (major === 22 && minor >= 18)) return + + throw new InputError(`surface-diff needs Node 22.18 or later, or Bun; this is Node ${process.versions.node}`) +} + +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 }, + }, + }) + + return values + } catch (error) { + throw new InputError(`${error instanceof Error ? error.message : String(error)}\n${USAGE}`) + } +} + +const run = (argv: string[]): unknown => { + assertSupportedRuntime() + + const { current: currentPath, base: basePath, names, variants = [] } = readArguments(argv) + if (!currentPath) throw new InputError(USAGE) + + const current = loadSurface(currentPath) + + if (variants.length > 0) { + if (basePath) throw new InputError("--variants lists other configurations; it cannot be combined with --base") + + const loadedVariants = variants.map((file) => ({ file, surface: loadSurface(file) })) + return { 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 report + + const { changed, added, removed, unchanged, orderOnly, inScope } = report + return { changed, added, removed, unchanged, orderOnly, inScope } +} + +const main = () => { + try { + console.log(JSON.stringify(run(process.argv.slice(2)), null, 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() From ccdf911e542429dc0777473d0fd223aa9a9d3f3a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:47:19 -0400 Subject: [PATCH 02/29] feat(ship-check): surface-diff prints a tool's definition as readable text A surface file keeps each description on one long JSON line, which a file viewer cuts off. --show prints the named tools with the description's own line breaks, so a reviewer reads them whole. Co-Authored-By: Claude Fable 5.1 --- .../scripts/__tests__/surface-diff.test.mts | 86 +++++++++++++++++++ .../scripts/surface-diff.mts | 56 ++++++++++-- 2 files changed, 136 insertions(+), 6 deletions(-) diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts index e1fd181..4c52622 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts @@ -14,6 +14,7 @@ import { listVariants, parameterTexts, parseSurface, + showTools, } from "../surface-diff.mts" const SCRIPT_PATH = fileURLToPath(new URL("../surface-diff.mts", import.meta.url)) @@ -21,6 +22,7 @@ const SCRIPT_PATH = fileURLToPath(new URL("../surface-diff.mts", import.meta.url const USAGE = [ "Usage: surface-diff.mts --current [--base ] [--names]", " surface-diff.mts --current --variants [--variants ...]", + " surface-diff.mts --current --show [--show ...]", ].join("\n") // '{"type":"object","properties":{}}' is 33 characters and "List notes." is 11. @@ -402,6 +404,58 @@ describe("listVariants", () => { }) }) +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 })) @@ -502,6 +556,38 @@ describe("command line", () => { }) }) + 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 or --variants\n", + }) + }) + it("exits 2 when --variants is combined with --base", () => { const current = writeSurface("combined-current.json", [rawTool()]) diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts index 8e64254..435a00b 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts @@ -92,6 +92,7 @@ const WHITESPACE_RUN = /\s+/g const USAGE = [ "Usage: surface-diff.mts --current [--base ] [--names]", " surface-diff.mts --current --variants [--variants ...]", + " surface-diff.mts --current --show [--show ...]", ].join("\n") const isJsonObject = (value: unknown): value is JsonObject => { @@ -447,6 +448,38 @@ export const listVariants = (current: Surface, variants: { file: string; surface 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 "${name}"`) + + return formatTool(tool) + }) + + return shown.join("\n\n") +} + const assertSupportedRuntime = () => { if (process.versions.bun) return @@ -466,6 +499,7 @@ const readArguments = (argv: string[]) => { base: { type: "string" }, names: { type: "boolean", default: false }, variants: { type: "string", multiple: true }, + show: { type: "string", multiple: true }, }, }) @@ -475,32 +509,42 @@ const readArguments = (argv: string[]) => { } } -const run = (argv: string[]): unknown => { +const toJson = (value: unknown): string => JSON.stringify(value, null, 2) + +const run = (argv: string[]): string => { assertSupportedRuntime() - const { current: currentPath, base: basePath, names, variants = [] } = readArguments(argv) + 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 || variants.length > 0) { + throw new InputError("--show prints tools from --current; it cannot be combined with --base or --variants") + } + + return showTools(current, show, currentPath) + } + if (variants.length > 0) { if (basePath) throw new InputError("--variants lists other configurations; it cannot be combined with --base") const loadedVariants = variants.map((file) => ({ file, surface: loadSurface(file) })) - return { current: currentPath, variants: listVariants(current, loadedVariants) } + 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 report + if (!names) return toJson(report) const { changed, added, removed, unchanged, orderOnly, inScope } = report - return { changed, added, removed, unchanged, orderOnly, inScope } + return toJson({ changed, added, removed, unchanged, orderOnly, inScope }) } const main = () => { try { - console.log(JSON.stringify(run(process.argv.slice(2)), null, 2)) + console.log(run(process.argv.slice(2))) } catch (error) { if (!(error instanceof InputError)) throw error From 275c10397be82ac9dc463a4fd8c18a2d529ef8a3 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:47:19 -0400 Subject: [PATCH 03/29] feat(ship-check): tool-definition-reviewer agent and tool-definition-review skill A report-only reviewer for MCP tool definitions, dispatched on demand with the tool list a server sends saved to a file. It marks changed tools against a rubric, then compares the list with the one before the change: text changed in tools outside the stated intent, dropped facts, description text that repeats the schema, and failures the handler can return that the description does not list. Co-Authored-By: Claude Fable 5.1 --- .../agents/tool-definition-reviewer.md | 86 +++++ .../skills/tool-definition-review/SKILL.md | 313 ++++++++++++++++++ 2 files changed, 399 insertions(+) create mode 100644 plugins/ship-check/agents/tool-definition-reviewer.md create mode 100644 plugins/ship-check/skills/tool-definition-review/SKILL.md 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..82409d6 --- /dev/null +++ b/plugins/ship-check/agents/tool-definition-reviewer.md @@ -0,0 +1,86 @@ +--- +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 large change, split in two.** One dispatch carries `Pass: cold` with only the + current surface file; a second carries `Pass: diff` with everything else. Each + dispatch does its one read. +- **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 `Tools:` 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.mts` 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 `Tools:` list, and a `Pass:` line. Your preloaded tool-definition-review skill +says what each one 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. +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. + +## Output format + +Return the skill's report in its own format: the status line and verification +basis, one entry for each tool in scope, then Defects, Unintended text changes (or +"All text changes" when no intended tools were given), and Grader noise, then the +`Cleared:`, `Skipped:`, and `Not reviewed:` lines. + +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..7bfff58 --- /dev/null +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -0,0 +1,313 @@ +--- +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(node ${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts *) + - Bash(bun ${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts *) +--- + +# 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 external grader | Noise section skipped | +| `Tools:` list | no | Names that limit which tools you review | 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.mts` does the comparisons that have one right answer. Its output is +**candidates and facts, never findings**: you decide what is a defect. + +``` +node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --base --names +node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --base +node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --variants [--variants ...] +node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --show [--show ...] +``` + +- If `${CLAUDE_SKILL_DIR}` appears above as literal text, the script is at + `scripts/surface-diff.mts` beside this `SKILL.md`. Use that path. +- It needs Node 22.18 or later, or Bun (`bun` in place of `node`). +- 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 neither `node` nor `bun` exists, say so in the report's `Script:` line and + compare the two files by reading them. + +| 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 schema differs 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. 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. 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 `Tools:` line, review only those names. +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. + +## 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 | See the first correction below | +| Conciseness | 10% | Every sentence carries a fact the agent needs, stated once | See the third 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 re-reads a changed tool has usually landed +within about half a point of its earlier score, and once 0.7 below, with no change +its written reason names. + +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:** 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 count is how a reader sees the check ran. + +### Duplicated fact + +- **Action:** for each `duplicationCandidates` entry with `preExisting: false`, + and each fact you saw stated in both the description and a schema description, + 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. List candidates with `preExisting: true` without a + verdict; this change did not introduce them. + +### 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. +- **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. +- **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`. + +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. + +## 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 ; tools limit +- Script: +- Files opened: +- Tools in scope: N (M reviewed) +- 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 ; failures traced; missing entries: ; + entries with no reachable failure: ; not followed: + Candidates: "" → — + — 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: — +Not reviewed: (partial reports only) +``` + +- 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 those lines and no Marks. +- 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 report is `partial` when any tool is `not reviewed` or `not traced`. +- 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.** From 84e2d8804a8b4048d275664c99ae8c6dcb53f057 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:47:19 -0400 Subject: [PATCH 04/29] docs: list the tool-definition reviewer and the bundled-script convention Co-Authored-By: Claude Fable 5.1 --- .claude-plugin/marketplace.json | 2 +- AGENTS.md | 8 +++++ README.md | 4 +-- plugins/ship-check/.claude-plugin/plugin.json | 2 +- plugins/ship-check/README.md | 31 ++++++++++++++----- 5 files changed, 36 insertions(+), 11 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 1e00610..4931342 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 the orchestrator skill.", "version": "1.1.1", "keywords": [ "code-review", diff --git a/AGENTS.md b/AGENTS.md index 59e92d8..7efb89e 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.mts and its __tests__/ README.md plan-check/ # Pre-implementation plan review plugin .claude-plugin/ @@ -70,6 +74,10 @@ 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 dependency-free TypeScript + (`.mts`, erasable syntax only), runnable with Node 22.18+ or Bun. Their tests live in + `scripts/__tests__/` and use `node:test`; run them with + `node --test "plugins/**/__tests__/*.test.mts"`. The release archives leave `__tests__` out. ## Skill authoring diff --git a/README.md b/README.md index aef33a6..8b525e1 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 covering 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 diff --git a/plugins/ship-check/.claude-plugin/plugin.json b/plugins/ship-check/.claude-plugin/plugin.json index 75468ba..c605544 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 load project conventions and user preferences 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..56e64cf 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -1,9 +1,10 @@ # 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 @@ -14,20 +15,28 @@ deliberately loads none. | `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: 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.mts`. +It has no dependencies and needs Node 22.18 or later, or Bun. -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 +60,11 @@ 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: the JSON a client gets +from `tools/list`, or a snapshot of it the project commits. 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" }) +``` From 8f84eb048ba1722ac189ff9d6c3da4700bea7146 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:05:52 -0400 Subject: [PATCH 05/29] refactor(ship-check): surface-diff runs on Bun only The script and its tests are plain .ts files run with Bun, as the other local scripts are. The Node version check, the Node 22.18 and 24 test matrix, and the .mts extension that silenced a Node warning are gone; CI runs `bun test plugins`. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/test.yml | 13 +-- AGENTS.md | 7 +- plugins/ship-check/README.md | 4 +- .../agents/tool-definition-reviewer.md | 2 +- .../skills/tool-definition-review/SKILL.md | 23 +++--- ...ace-diff.test.mts => surface-diff.test.ts} | 16 ++-- .../{surface-diff.mts => surface-diff.ts} | 80 ++++++++++++------- 7 files changed, 82 insertions(+), 63 deletions(-) rename plugins/ship-check/skills/tool-definition-review/scripts/__tests__/{surface-diff.test.mts => surface-diff.test.ts} (97%) rename plugins/ship-check/skills/tool-definition-review/scripts/{surface-diff.mts => surface-diff.ts} (93%) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a037c12..e079dba 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,20 +13,13 @@ jobs: runs-on: ubuntu-latest # The suite runs in under a second; five minutes covers a slow runner start. timeout-minutes: 5 - name: script tests (Node ${{ matrix.node }}) - strategy: - matrix: - # 22.18 is the oldest Node that runs the scripts' TypeScript without a - # flag, and 24 is the current LTS. - node: ["22.18", "24"] + name: script tests steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 - with: - node-version: ${{ matrix.node }} + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 # The scripts have no dependencies, so there is nothing to install. - - run: node --test "plugins/**/__tests__/*.test.mts" + - run: bun test plugins diff --git a/AGENTS.md b/AGENTS.md index 7efb89e..b1bd0ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,7 +39,7 @@ plugins/ 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.mts and its __tests__/ + scripts/ # surface-diff.ts and its __tests__/ README.md plan-check/ # Pre-implementation plan review plugin .claude-plugin/ @@ -75,9 +75,8 @@ SECURITY.md # Vulnerability reporting policy - 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 dependency-free TypeScript - (`.mts`, erasable syntax only), runnable with Node 22.18+ or Bun. Their tests live in - `scripts/__tests__/` and use `node:test`; run them with - `node --test "plugins/**/__tests__/*.test.mts"`. The release archives leave `__tests__` out. + (`.ts`), run with Bun. Their tests live in `scripts/__tests__/`; run them with + `bun test plugins`. The release archives leave `__tests__` out. ## Skill authoring diff --git a/plugins/ship-check/README.md b/plugins/ship-check/README.md index 56e64cf..40b92f7 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -33,8 +33,8 @@ Each agent preloads skills via `skills:` frontmatter. The `pr-review`, 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.mts`. -It has no dependencies and needs Node 22.18 or later, or Bun. +The `tool-definition-review` skill bundles one script, `scripts/surface-diff.ts`. +It has no dependencies and runs with [Bun](https://bun.sh). The four convention-loading phase agents also use MCP tools loaded at runtime via `ToolSearch`: diff --git a/plugins/ship-check/agents/tool-definition-reviewer.md b/plugins/ship-check/agents/tool-definition-reviewer.md index 82409d6..6f7b263 100644 --- a/plugins/ship-check/agents/tool-definition-reviewer.md +++ b/plugins/ship-check/agents/tool-definition-reviewer.md @@ -42,7 +42,7 @@ from before a change. You report what you find and you fix nothing. ## What you are not -- **Not a fixer.** You have a shell to run `surface-diff.mts` and for nothing +- **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 diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index 7bfff58..f5f09a6 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -14,8 +14,7 @@ description: > the code beyond its error list (use bug-check), or README and docs prose (use code-quality). allowed-tools: - - Bash(node ${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts *) - - Bash(bun ${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts *) + - Bash(bun ${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts *) --- # Tool Definition Review @@ -48,25 +47,25 @@ JSON-RPC response whose `result` holds `tools`. It must hold the whole list. ## The script -`surface-diff.mts` does the comparisons that have one right answer. Its output is -**candidates and facts, never findings**: you decide what is a defect. +`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: ``` -node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --base --names -node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --base -node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --variants [--variants ...] -node "${CLAUDE_SKILL_DIR}/scripts/surface-diff.mts" --current --show [--show ...] +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.mts` beside this `SKILL.md`. Use that path. -- It needs Node 22.18 or later, or Bun (`bun` in place of `node`). + `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 neither `node` nor `bun` exists, say so in the report's `Script:` line and - compare the two files by reading them. +- If `bun` is not installed, say so in the report's `Script:` line and compare + the two files by reading them. | Output field | Meaning | |---|---| diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.ts similarity index 97% rename from plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts rename to plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.ts index 4c52622..19d75b8 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.mts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/__tests__/surface-diff.test.ts @@ -15,14 +15,14 @@ import { parameterTexts, parseSurface, showTools, -} from "../surface-diff.mts" +} from "../surface-diff.ts" -const SCRIPT_PATH = fileURLToPath(new URL("../surface-diff.mts", import.meta.url)) +const SCRIPT_PATH = fileURLToPath(new URL("../surface-diff.ts", import.meta.url)) const USAGE = [ - "Usage: surface-diff.mts --current [--base ] [--names]", - " surface-diff.mts --current --variants [--variants ...]", - " surface-diff.mts --current --show [--show ...]", + "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. @@ -51,7 +51,11 @@ const surfaceOf = (tools: unknown[], sections: Record = {}) => const parsedTool = (overrides: Record = {}) => { const [tool] = surfaceOf([rawTool(overrides)]).tools - if (!tool) throw new Error("surfaceOf returned no tool") + + if (!tool) { + throw new Error("surfaceOf returned no tool") + } + return tool } diff --git a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts similarity index 93% rename from plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts rename to plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts index 435a00b..975714f 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.mts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env node +#!/usr/bin/env bun import { readFileSync, realpathSync } from "node:fs" import { fileURLToPath } from "node:url" import { parseArgs } from "node:util" @@ -90,9 +90,9 @@ const SENTENCE_BOUNDARY = /(?<=[.!?])\s+/ const WHITESPACE_RUN = /\s+/g const USAGE = [ - "Usage: surface-diff.mts --current [--base ] [--names]", - " surface-diff.mts --current --variants [--variants ...]", - " surface-diff.mts --current --show [--show ...]", + "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 => { @@ -110,15 +110,21 @@ const optionalObject = (value: unknown, field: string, where: string): JsonObjec } const parseTool = (value: unknown, position: number, label: string): Tool => { - if (!isJsonObject(value)) throw new InputError(`${label}: tool ${position} is not an object`) + 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"`) } const where = `${label}: tool "${name}"` - if (!isJsonObject(inputSchema)) throw new InputError(`${where}: "inputSchema" must be an object`) + + if (!isJsonObject(inputSchema)) { + throw new InputError(`${where}: "inputSchema" must be an object`) + } return { name, @@ -153,8 +159,12 @@ export const parseSurface = (parsed: unknown, label: string): Surface => { 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}"`) + if (seenNames.has(name)) { + throw new InputError(`${label}: two tools are named "${name}"`) + } + seenNames.add(name) } @@ -183,8 +193,13 @@ const loadSurface = (path: string): Surface => parseSurface(parseJson(readText(p /** 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 + 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)])) @@ -224,8 +239,13 @@ const nonEmptyTrimmed = (texts: string[]): string[] => texts.map((text) => text. 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 [] + 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)] @@ -336,6 +356,7 @@ const longestCommonSubstring = (left: string, right: string): string => { /** 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) @@ -429,6 +450,7 @@ const listVariant = (current: Surface, file: string, variant: Surface): VariantL const differing = current.tools.flatMap((tool) => { const variantTool = variantByName.get(tool.name) + if (!variantTool) return [] const parts = changedParts(tool, variantTool) @@ -472,7 +494,10 @@ export const showTools = (surface: Surface, names: string[], label: string): str const shown = names.map((name) => { const tool = toolsByName.get(name) - if (!tool) throw new InputError(`${label}: no tool named "${name}"`) + + if (!tool) { + throw new InputError(`${label}: no tool named "${name}"`) + } return formatTool(tool) }) @@ -480,16 +505,6 @@ export const showTools = (surface: Surface, names: string[], label: string): str return shown.join("\n\n") } -const assertSupportedRuntime = () => { - if (process.versions.bun) return - - // Node strips TypeScript types without a flag from 22.18. - const [major = 0, minor = 0] = process.versions.node.split(".").map(Number) - if (major > 22 || (major === 22 && minor >= 18)) return - - throw new InputError(`surface-diff needs Node 22.18 or later, or Bun; this is Node ${process.versions.node}`) -} - const readArguments = (argv: string[]) => { try { const { values } = parseArgs({ @@ -512,10 +527,11 @@ const readArguments = (argv: string[]) => { const toJson = (value: unknown): string => JSON.stringify(value, null, 2) const run = (argv: string[]): string => { - assertSupportedRuntime() - const { current: currentPath, base: basePath, names, variants = [], show = [] } = readArguments(argv) - if (!currentPath) throw new InputError(USAGE) + + if (!currentPath) { + throw new InputError(USAGE) + } const current = loadSurface(currentPath) @@ -528,7 +544,9 @@ const run = (argv: string[]): string => { } if (variants.length > 0) { - if (basePath) throw new InputError("--variants lists other configurations; it cannot be combined with --base") + if (basePath) { + throw new InputError("--variants lists other configurations; it cannot be combined with --base") + } const loadedVariants = variants.map((file) => ({ file, surface: loadSurface(file) })) return toJson({ current: currentPath, variants: listVariants(current, loadedVariants) }) @@ -536,7 +554,10 @@ const run = (argv: string[]): string => { const base = basePath ? loadSurface(basePath) : null const report = compareSurfaces(base, current, { base: basePath ?? null, current: currentPath }) - if (!names) return toJson(report) + + if (!names) { + return toJson(report) + } const { changed, added, removed, unchanged, orderOnly, inScope } = report return toJson({ changed, added, removed, unchanged, orderOnly, inScope }) @@ -555,4 +576,7 @@ const main = () => { // 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() + +if (invokedPath && realpathSync(invokedPath) === realpathSync(fileURLToPath(import.meta.url))) { + main() +} From 131b208bd7d73cc15d09f169e2a773c0bb8a81fd Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:07:52 -0400 Subject: [PATCH 06/29] fix(ship-check): the tool-definition reviewer stops at eight tools and never reports untraced as complete A run on 29 changed tools skipped the error-entry check for nearly all of them, gave a false reason, and still reported complete. The skill now caps one dispatch at eight tools through the diff read, sets the status by counting unfinished entries, and gives concrete steps for tracing a handler. The script trims an overlap before deciding whether the base already had it, so a repetition that only gained a neighbouring space is not labelled new. Co-Authored-By: Claude Fable 5.1 --- .../agents/tool-definition-reviewer.md | 11 +++++-- .../skills/tool-definition-review/SKILL.md | 32 ++++++++++++++++--- .../scripts/__tests__/surface-diff.test.ts | 14 ++++++++ .../scripts/surface-diff.ts | 19 +++++++---- 4 files changed, 62 insertions(+), 14 deletions(-) diff --git a/plugins/ship-check/agents/tool-definition-reviewer.md b/plugins/ship-check/agents/tool-definition-reviewer.md index 6f7b263..ffbab14 100644 --- a/plugins/ship-check/agents/tool-definition-reviewer.md +++ b/plugins/ship-check/agents/tool-definition-reviewer.md @@ -31,9 +31,11 @@ from before a change. You report what you find and you fix nothing. - **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 large change, split in two.** One dispatch carries `Pass: cold` with only the - current surface file; a second carries `Pass: diff` with everything else. Each - dispatch does its one read. +- **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 `Tools:` 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. @@ -73,6 +75,9 @@ Follow your preloaded tool-definition-review skill: 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 diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index f5f09a6..cfc885d 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -103,6 +103,13 @@ them for their own review. 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.** 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 and no `Tools:` line: + do the cold read for every tool, do the diff read for the first eight names in + `inScope`, write `not reviewed` on the diff lines of the rest, and report + `partial`. The dispatcher sends the rest as `Pass: diff` with a `Tools:` line. ## Read 1: cold read @@ -205,8 +212,9 @@ dropped`). The count is how a reader sees the check ran. 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. List candidates with `preExisting: true` without a - verdict; this change did not introduce them. + 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 @@ -222,9 +230,21 @@ dropped`). The count is how a reader sees the check ran. 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. +- **How to trace one tool:** + 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. + 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. - **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.** Tracing is the check. 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 @@ -271,7 +291,8 @@ Per tool Facts: in the old text — moved (), dropped () Errors: handler ; failures traced; missing entries: ; entries with no reachable failure: ; not followed: - Candidates: "" → — + Candidates: "" → — ; + pre-existing on — not reviewed Defects @@ -298,7 +319,10 @@ Not reviewed: (partial reports only) - 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 report is `partial` when any tool is `not reviewed` or `not traced`. +- **Set the status last, by counting.** Count the entries that say `not reviewed` + or `not traced`. If the count is above zero, the status is `partial` and the + `Not reviewed:` line names those tools. `complete` means every in-scope tool went + through every check whose input was supplied. - 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. 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 index 19d75b8..0fe64f7 100644 --- 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 @@ -384,6 +384,20 @@ describe("duplication candidates", () => { }) }) +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([ 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 index 975714f..3b52457 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -380,13 +380,18 @@ const findCandidates = (tool: Tool, base: Tool | undefined): Candidate[] => { } return parameterTexts(tool.inputSchema).flatMap(({ parameter, text }) => { - return commonSubstrings(collapseWhitespace(text), description).map((overlap) => ({ - tool: tool.name, - parameter, - text: overlap, - length: overlap.length, - preExisting: baseRepeated(parameter, overlap), - })) + return commonSubstrings(collapseWhitespace(text), description).map((overlap) => { + // An overlap that only gained a neighbouring space is the same repetition the base had. + const trimmedOverlap = overlap.trim() + + return { + tool: tool.name, + parameter, + text: trimmedOverlap, + length: trimmedOverlap.length, + preExisting: baseRepeated(parameter, trimmedOverlap), + } + }) }) } From 6ca136457b1c9d5110384d9831bdff9d3bd88a8c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:23:38 -0400 Subject: [PATCH 07/29] fix(ship-check): the reviewer's scope line cannot be mistaken for the intended list One batch read its `Tools:` line as the intended-tools list, reviewed all 29 tools, traced eight, and reported complete. The scope line is now `Review only:`, every in-scope tool is traced whether or not the change meant to touch it, and the report ends with a count of unfinished entries followed by the status, so complete requires a count of zero. Co-Authored-By: Claude Fable 5.1 --- .../agents/tool-definition-reviewer.md | 14 +++--- .../skills/tool-definition-review/SKILL.md | 45 ++++++++++++------- 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/plugins/ship-check/agents/tool-definition-reviewer.md b/plugins/ship-check/agents/tool-definition-reviewer.md index ffbab14..4b3ba01 100644 --- a/plugins/ship-check/agents/tool-definition-reviewer.md +++ b/plugins/ship-check/agents/tool-definition-reviewer.md @@ -33,13 +33,13 @@ from before a change. You report what you find and you fix nothing. 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 `Tools:` 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. + `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 `Tools:` list of the names an +- **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 @@ -60,7 +60,8 @@ from before a change. You report what you find and you fix nothing. 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 `Tools:` list, and a `Pass:` line. Your preloaded tool-definition-review skill +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 one unlocks. If the dispatch names no current surface file, ask for one. Do NOT read tool definitions out of source files as a substitute. @@ -84,7 +85,8 @@ Follow your preloaded tool-definition-review skill: Return the skill's report in its own format: the status line and verification basis, one entry for each tool in scope, then Defects, Unintended text changes (or "All text changes" when no intended tools were given), and Grader noise, then the -`Cleared:`, `Skipped:`, and `Not reviewed:` lines. +`Cleared:` and `Skipped:` lines, and last the `Unfinished entries:` count and 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 diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index cfc885d..7b156a7 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -39,7 +39,7 @@ whose input is missing is **skipped and listed as skipped**, never guessed at. | 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 external grader | Noise section skipped | -| `Tools:` list | no | Names that limit which tools you review | Scope comes from the script | +| `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 @@ -98,7 +98,10 @@ them for their own review. ## Scope 1. Run the script with `--names`. `inScope` is your list. -2. If the dispatch has a `Tools:` line, review only those names. +2. If the dispatch has a `Review only:` line, those names are your whole scope. + Review each of them fully, whether or not it is in the intended-tools list, and + review no other tool. 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`, @@ -106,10 +109,11 @@ them for their own review. 4. **One dispatch takes at most eight tools through the diff read.** 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 and no `Tools:` line: - do the cold read for every tool, do the diff read for the first eight names in - `inScope`, write `not reviewed` on the diff lines of the rest, and report - `partial`. The dispatcher sends the rest as `Pass: diff` with a `Tools:` line. + nearly all of them. With more than eight tools in scope and no `Review only:` + line: do the cold read for every tool, do the diff read for the first eight + names in `inScope`, write `not reviewed` on the diff lines of the rest, and + report `partial`. The dispatcher sends the rest as `Pass: diff` with a + `Review only:` line. ## Read 1: cold read @@ -224,7 +228,9 @@ dropped`). The count is how a reader sees the check ran. 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. +- **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 @@ -260,6 +266,9 @@ message has no entry, and it is produced in a helper the change never touched. 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 the instruction files for `size`, + `cap`, `allowance`, `budget`, and `chars`, and say in the `Skipped:` line that + the search found nothing. ## Grader noise @@ -274,13 +283,13 @@ message has no entry, and it is produced in a helper the change never touched. ## Report format ``` -Tool definition review: +Tool definition review - Read: - Surfaces: current (); base -- Inputs: intended tools ; repository root ; grader results ; tools limit +- Inputs: intended tools ; repository root ; grader results ; review only - Script: - Files opened: -- Tools in scope: N (M reviewed) +- Tools in scope: N - Other configurations that differ: Per tool @@ -309,7 +318,8 @@ Grader noise Cleared: — Skipped: — -Not reviewed: (partial reports only) +Unfinished entries: — +Status: ``` - In the Marks line, P is Purpose, U is Usage, B is Behaviour, Pa is Parameters, @@ -319,10 +329,15 @@ Not reviewed: (partial reports only) - 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 — `. -- **Set the status last, by counting.** Count the entries that say `not reviewed` - or `not traced`. If the count is above zero, the status is `partial` and the - `Not reviewed:` line names those tools. `complete` means every in-scope tool went - through every check whose input was supplied. +- **The last two lines are written last, by counting.** Count the entries that say + `not reviewed` or `not traced`, and the entries that list a callee under + `not followed` without saying why that callee cannot return a failure to the + client. 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. + + 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. From cd348fab8308ffce728175dfb88a355267197be1 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:25:13 -0400 Subject: [PATCH 08/29] fix(ship-check): review findings on the surface-diff script and workflows - With no base, a snapshot's sections report changed: null, so "not compared" never reads as "unchanged". - A phrase a parameter states twice is reported as one duplication candidate. - Tests cover a line and a schema sentence removed from several tools. - The marketplace description counts the plugin's eight skills. - The release workflows pin checkout and create-github-app-token to commits, as the other workflows do. Co-Authored-By: Claude Fable 5.1 --- .claude-plugin/marketplace.json | 2 +- .github/workflows/auto_release.yml | 6 +-- .github/workflows/manual_release.yml | 4 +- .../skills/tool-definition-review/SKILL.md | 2 +- .../scripts/__tests__/surface-diff.test.ts | 47 +++++++++++++++++++ .../scripts/surface-diff.ts | 10 ++-- 6 files changed, 60 insertions(+), 11 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 4931342..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. Six agent types (pr-reviewer, code-quality-reviewer, test-auditor, bug-checker, fresh-eyes, tool-definition-reviewer) 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 80b2f79..1a72699 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -13,7 +13,7 @@ jobs: if: "!endsWith(github.actor, '[bot]')" runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Extract version from tag id: tag @@ -69,13 +69,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 }} diff --git a/.github/workflows/manual_release.yml b/.github/workflows/manual_release.yml index 92c0997..da59fc8 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -22,13 +22,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 }} diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index 7b156a7..c7f34b6 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -75,7 +75,7 @@ bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --show | `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. Empty when the file carries neither | +| `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` | 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 index 0fe64f7..191d75a 100644 --- 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 @@ -314,6 +314,47 @@ describe("compareSurfaces", () => { ]) }) + 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: [] }) @@ -343,6 +384,12 @@ describe("duplication candidates", () => { 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("finds described parameters in nested properties, items, and anyOf branches", () => { const schema = { type: "object", 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 index 3b52457..519cc89 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -43,7 +43,8 @@ type Candidate = { tool: string; parameter: string; text: string; length: number type ParameterText = { parameter: string; text: string } -type SectionChange = { name: string; changed: boolean } +// `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 @@ -363,8 +364,9 @@ export const commonSubstrings = (text: string, other: string): string[] => { const before = text.slice(0, start) const after = text.slice(start + longest.length) - const overlaps = [longest, ...commonSubstrings(before, other), ...commonSubstrings(after, other)] - return overlaps.toSorted((leftText, rightText) => rightText.length - leftText.length) + // A phrase the text states twice is one repetition of `other`, so it is reported once. + 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[] => { @@ -400,7 +402,7 @@ const compareSections = (base: Surface | null, current: Surface): SectionChange[ return present.map((key) => ({ name: key, - changed: base !== null && canonicalJson(base.sections[key]) !== canonicalJson(current.sections[key]), + changed: base ? canonicalJson(base.sections[key]) !== canonicalJson(current.sections[key]) : null, })) } From cee755368a07fe336e201ca0d79d25932e6623e4 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:43:38 -0400 Subject: [PATCH 09/29] fix(ship-check): duplication threshold applies to the trimmed overlap; validate job keeps no token A candidate that reached 40 characters only by counting a neighbouring space was reported at 39. The release workflow's validate job only reads files, so its checkout no longer persists credentials. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .github/workflows/auto_release.yml | 3 +++ .../scripts/__tests__/surface-diff.test.ts | 8 ++++++ .../scripts/surface-diff.ts | 25 ++++++++++--------- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/.github/workflows/auto_release.yml b/.github/workflows/auto_release.yml index 1a72699..480746c 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -14,6 +14,9 @@ jobs: runs-on: ubuntu-latest steps: - 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 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 index 191d75a..c13c0ba 100644 --- 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 @@ -390,6 +390,14 @@ describe("duplication candidates", () => { 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("finds described parameters in nested properties, items, and anyOf branches", () => { const schema = { type: "object", 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 index 519cc89..988a8b3 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -382,18 +382,19 @@ const findCandidates = (tool: Tool, base: Tool | undefined): Candidate[] => { } return parameterTexts(tool.inputSchema).flatMap(({ parameter, text }) => { - return commonSubstrings(collapseWhitespace(text), description).map((overlap) => { - // An overlap that only gained a neighbouring space is the same repetition the base had. - const trimmedOverlap = overlap.trim() - - return { - tool: tool.name, - parameter, - text: trimmedOverlap, - length: trimmedOverlap.length, - preExisting: baseRepeated(parameter, trimmedOverlap), - } - }) + // A trimmed overlap matches the base's repetition even when the change added a space beside it. + // The threshold is checked again because trimming can shorten an overlap below it. + const overlaps = commonSubstrings(collapseWhitespace(text), description) + .map((overlap) => overlap.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), + })) }) } From b1f6e973d694b3e712c171f9cd4b64f691dc2956 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:47:53 -0400 Subject: [PATCH 10/29] docs: root README names all eight skills, the Bun prerequisite, and how to drop the MCP servers The ship-check row listed seven coverage areas for eight skills. The adapting steps now name both vault-cortex tool prefixes the agent files carry and say what going without sequential-thinking takes. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- README.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 8b525e1..223b610 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Personal plugin marketplace for Claude Code and Claude Cowork — review agents, | Plugin | Description | |--------|-------------| -| [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 covering PR review, code quality, test audit, bug hunting, stranger reads, MCP tool-definition review, 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 @@ -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,14 +42,16 @@ 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 +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 agents use it for triage reasoning (or drop it from their `tools:` allowlists) +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 From 013a6fbf48b8d2b46b9c4ede3a79d596027ba26c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:53:41 -0400 Subject: [PATCH 11/29] fix(ship-check): the tool-definition reviewer fails when its script cannot run, and five unclear passages are reworded A reviewer whose shell refused the script compared the two files by hand, took the intended-tools list for its scope, and left 20 of 33 tools uncompared. The skill now retries once with plain paths, then reports failed. Tracing names the file tools, the eight-tool cap is stated for a diff-only dispatch, and an Errors line marked skipped with a repository root supplied counts as unfinished. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .../agents/tool-definition-reviewer.md | 27 ++++--- .../skills/tool-definition-review/SKILL.md | 77 ++++++++++++------- 2 files changed, 66 insertions(+), 38 deletions(-) diff --git a/plugins/ship-check/agents/tool-definition-reviewer.md b/plugins/ship-check/agents/tool-definition-reviewer.md index 4b3ba01..42340ec 100644 --- a/plugins/ship-check/agents/tool-definition-reviewer.md +++ b/plugins/ship-check/agents/tool-definition-reviewer.md @@ -61,15 +61,18 @@ from before a change. You report what you find and you fix nothing. 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 one unlocks. If the dispatch names no current surface file, ask for -one. Do NOT read tool definitions out of source files as a substitute. +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. +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. @@ -82,11 +85,17 @@ Follow your preloaded tool-definition-review skill: ## Output format -Return the skill's report in its own format: the status line and verification -basis, one entry for each tool in scope, then Defects, Unintended text changes (or -"All text changes" when no intended tools were given), and Grader noise, then the -`Cleared:` and `Skipped:` lines, and last the `Unfinished entries:` count and the -`Status:` line. `Status: complete` is only for a count of 0. +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 diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index c7f34b6..c2e48be 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -38,7 +38,7 @@ whose input is missing is **skipped and listed as skipped**, never guessed at. | 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 external grader | Noise section 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 | @@ -64,8 +64,13 @@ bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --show - **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 `bun` is not installed, say so in the report's `Script:` line and compare - the two files by reading them. +- **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 | |---|---| @@ -106,14 +111,19 @@ them for their own review. 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.** 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 and no `Review only:` - line: do the cold read for every tool, do the diff read for the first eight - names in `inScope`, write `not reviewed` on the diff lines of the rest, and - report `partial`. The dispatcher sends the rest as `Pass: diff` with a - `Review only:` line. +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 @@ -136,8 +146,8 @@ Mark each dimension 1 to 5. A 5 has nothing to fix. | 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 | See the first correction below | -| Conciseness | 10% | Every sentence carries a fact the agent needs, stated once | See the third correction below | +| 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: @@ -154,9 +164,9 @@ Three corrections. Apply them over the table: 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 re-reads a changed tool has usually landed -within about half a point of its earlier score, and once 0.7 below, with no change -its written reason names. +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 @@ -207,9 +217,11 @@ dropped`). The count is how a reader sees the check ran. ### Duplicated fact -- **Action:** for each `duplicationCandidates` entry with `preExisting: false`, - and each fact you saw stated in both the description and a schema description, - decide: a repetition to cut, or a constraint that belongs in both places. For a +- **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 @@ -236,7 +248,8 @@ dropped`). The count is how a reader sees the check ran. 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. -- **How to trace one tool:** +- **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. @@ -248,9 +261,10 @@ dropped`). The count is how a reader sees the check ran. - **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.** Tracing is the check. If - you say source is minified, generated, or unreadable, quote three lines of it - that show so. +- **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 @@ -329,12 +343,17 @@ Status: - 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 the entries that say - `not reviewed` or `not traced`, and the entries that list a callee under - `not followed` without saying why that callee cannot return a failure to the - client. 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. +- **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`. From 440424207c8010fea25d412dcbc6e541350c27b0 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 15:02:09 -0400 Subject: [PATCH 12/29] style: clarify comments in surface-diff, workflow path shapes, and README rubric naming surface-diff.ts: add comments on Surface.sections, Candidate.preExisting, Report.orderOnly, and wireJson; rewrite the commonSubstrings Set comment and the longestCommonSubstring DP comment for accuracy. Workflows: explain the double-dirname path shape in both release workflows and the stash/checkout/pop purpose in auto_release. ship-check README: correct TDQS expansion from "Tool Description" to "Tool Definition" in the pr-reviewer row; name the rubric (TDQS) in the tool-definition-reviewer row; add Bun failure note; clarify tools/list as the MCP method; define "surface" in the usage section. plugin.json: qualify "four phase agents" with "that load conventions" to match the README. AGENTS.md: clarify "dependency-free" as "no npm dependencies", add the *.test.ts discovery pattern, and explain what `plugins` means in `bun test plugins`. Ship-Check: code-quality Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/auto_release.yml | 2 ++ .github/workflows/manual_release.yml | 1 + AGENTS.md | 7 ++++--- plugins/ship-check/.claude-plugin/plugin.json | 2 +- plugins/ship-check/README.md | 14 ++++++++------ .../tool-definition-review/scripts/surface-diff.ts | 8 ++++++-- 6 files changed, 22 insertions(+), 12 deletions(-) diff --git a/.github/workflows/auto_release.yml b/.github/workflows/auto_release.yml index 480746c..d4a3659 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -96,6 +96,7 @@ 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") @@ -127,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 da59fc8..334d904 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -93,6 +93,7 @@ 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") diff --git a/AGENTS.md b/AGENTS.md index b1bd0ca..e6afbb4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,9 +74,10 @@ 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 dependency-free TypeScript - (`.ts`), run with Bun. Their tests live in `scripts/__tests__/`; run them with - `bun test plugins`. The release archives leave `__tests__` out. +- **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. ## Skill authoring diff --git a/plugins/ship-check/.claude-plugin/plugin.json b/plugins/ship-check/.claude-plugin/plugin.json index c605544..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, fresh-eyes deliberately loads none, and tool-definition-reviewer reviews MCP tool definitions on demand." + "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 40b92f7..2b0b4b6 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -10,12 +10,12 @@ on demand and is not a pipeline phase. | 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: 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. | +| `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 @@ -34,7 +34,8 @@ 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). +It has no dependencies and runs with [Bun](https://bun.sh). Without Bun the agent +reports the review as `failed`. The four convention-loading phase agents also use MCP tools loaded at runtime via `ToolSearch`: @@ -61,9 +62,10 @@ 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: the JSON a client gets -from `tools/list`, or a snapshot of it the project commits. Give it the file from -before the change as well, when there is one: +`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" }) 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 index 988a8b3..4c242ed 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -15,6 +15,7 @@ export type Tool = { 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" @@ -39,6 +40,7 @@ type SharedEdit = { 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 } @@ -53,6 +55,7 @@ export type Report = { added: string[] removed: string[] unchanged: string[] + // Tools whose schemas differ only in JSON key order — same meaning, different serialisation. orderOnly: string[] inScope: string[] changes: ToolChange[] @@ -211,6 +214,7 @@ 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[] => { @@ -328,7 +332,7 @@ export const parameterTexts = (schema: JsonValue | undefined, path = ""): Parame } const longestCommonSubstring = (left: string, right: string): string => { - // Dynamic programming over two rows; the three counters are overwritten as the table is scanned. + // 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) @@ -364,7 +368,7 @@ export const commonSubstrings = (text: string, other: string): string[] => { const before = text.slice(0, start) const after = text.slice(start + longest.length) - // A phrase the text states twice is one repetition of `other`, so it is reported once. + // 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) } From 4b93729ee359a12071f0f3d0714c981165da8ee1 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 15:03:12 -0400 Subject: [PATCH 13/29] docs(ship-check): the reviewer's usage example defines its prompt lines, and AGENTS.md says where tests leave the release archives Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 3 ++- plugins/ship-check/README.md | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index e6afbb4..ec4d19f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,7 +77,8 @@ SECURITY.md # Vulnerability reporting policy - **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. + 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/plugins/ship-check/README.md b/plugins/ship-check/README.md index 2b0b4b6..2d12dff 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -70,3 +70,12 @@ 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. From f6a2418af85d4df62ab009e6236abf8327f9c135 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 15:10:39 -0400 Subject: [PATCH 14/29] test(ship-check): cover every InputError path, CLI mode, and schema branch in surface-diff 14 tests added: three parseSurface rejection paths (tool not object, non-string title, non-object annotations), parameterTexts oneOf/allOf branches and non-object guard, compareSections one-side-only cases, outputSchema size accounting, findSharedEdits single-tool filtering, and five CLI integration tests (--show + --variants, unknown flag, full report with and without --base, --show with missing tool). Ship-Check: test-audit Co-Authored-By: Claude Opus 4.6 (1M context) --- .../scripts/__tests__/surface-diff.test.ts | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) 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 index c13c0ba..0dd64f3 100644 --- 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 @@ -129,6 +129,21 @@ describe("parseSurface", () => { 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()] }, @@ -364,6 +379,53 @@ describe("compareSurfaces", () => { { 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(report.sharedEdits, []) + // Verify the change was detected (the edit existed but was single-tool). + assert.deepStrictEqual(report.changed, ["read_note"]) + }) }) describe("duplication candidates", () => { @@ -419,6 +481,27 @@ describe("duplication candidates", () => { ]) }) + 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) @@ -670,4 +753,57 @@ describe("command line", () => { stderr: "--variants lists other configurations; it cannot be combined with --base\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 or --variants\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 }, + { status: 0, stderr: "", current, base, changed: ["list_notes"] }, + ) + }) + + 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 }, + { status: 0, stderr: "", current, base: null, inScope: ["list_notes"] }, + ) + }) + + 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`, + }) + }) }) From cec0249e2839d23ca9f5806ed659b3bba58cf5e7 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 15:50:55 -0400 Subject: [PATCH 15/29] fix(ship-check): the reviewer lists each traced failure as listed or MISSING, and --names is rejected with --show and --variants A test run traced six failures for one tool and reported none missing while two had no entry; a count with a verdict hid the comparison. The Errors block now carries one line for each failure. The unintended-changes section never lists an intended tool. --names was silently ignored outside the comparison report. The CLI report tests assert the full field list. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .../skills/tool-definition-review/SKILL.md | 30 +++++++--- .../scripts/__tests__/surface-diff.test.ts | 59 +++++++++++++++---- .../scripts/surface-diff.ts | 10 ++-- 3 files changed, 77 insertions(+), 22 deletions(-) diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index c2e48be..99a1875 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -76,7 +76,7 @@ bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --show |---|---| | `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 schema differs only in key order. Not a text change | +| `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 | @@ -192,9 +192,10 @@ of the report still says both reads ran, and each check you could not run gets a 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:** 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. +- **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 @@ -258,6 +259,17 @@ dropped`). The count is how a reader sees the check ran. 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` or `MISSING`. + 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`. @@ -312,8 +324,10 @@ Per tool U4: "" — B3: no entry says what an empty result looks like Facts: in the old text — moved (), dropped () - Errors: handler ; failures traced; missing entries: ; - entries with no reachable failure: ; not followed: + Errors: handler + "" () — + "" () — + entries with no reachable failure: ; not followed: Candidates: "" → — ; pre-existing on — not reviewed @@ -339,7 +353,9 @@ 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 those lines and no Marks. + `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 — `. 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 index 0dd64f3..a815dae 100644 --- 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 @@ -31,6 +31,24 @@ 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) => ({ @@ -422,9 +440,10 @@ describe("findSharedEdits", () => { [rawTool({ name: "read_note", description: "Read a note." })], ) - assert.deepStrictEqual(report.sharedEdits, []) - // Verify the change was detected (the edit existed but was single-tool). - assert.deepStrictEqual(report.changed, ["read_note"]) + assert.deepStrictEqual( + { changed: report.changed, sharedEdits: report.sharedEdits }, + { changed: ["read_note"], sharedEdits: [] }, + ) }) }) @@ -740,7 +759,7 @@ describe("command line", () => { 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 or --variants\n", + stderr: "--show prints tools from --current; it cannot be combined with --base, --names, or --variants\n", }) }) @@ -750,7 +769,7 @@ describe("command line", () => { assert.deepStrictEqual(runScript(["--current", current, "--base", current, "--variants", current]), { status: 2, stdout: "", - stderr: "--variants lists other configurations; it cannot be combined with --base\n", + stderr: "--variants lists other configurations; it cannot be combined with --base or --names\n", }) }) @@ -760,7 +779,27 @@ describe("command line", () => { 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 or --variants\n", + 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", }) }) @@ -780,8 +819,8 @@ describe("command line", () => { const output = JSON.parse(stdout) assert.deepStrictEqual( - { status, stderr, current: output.current, base: output.base, changed: output.changed }, - { status: 0, stderr: "", current, base, changed: ["list_notes"] }, + { 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 }, ) }) @@ -792,8 +831,8 @@ describe("command line", () => { const output = JSON.parse(stdout) assert.deepStrictEqual( - { status, stderr, current: output.current, base: output.base, inScope: output.inScope }, - { status: 0, stderr: "", current, base: null, inScope: ["list_notes"] }, + { 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 }, ) }) 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 index 4c242ed..171884f 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -55,7 +55,7 @@ export type Report = { added: string[] removed: string[] unchanged: string[] - // Tools whose schemas differ only in JSON key order — same meaning, different serialisation. + // Tools whose schemas or annotations differ only in JSON key order: same meaning, different serialisation. orderOnly: string[] inScope: string[] changes: ToolChange[] @@ -548,16 +548,16 @@ const run = (argv: string[]): string => { const current = loadSurface(currentPath) if (show.length > 0) { - if (basePath || variants.length > 0) { - throw new InputError("--show prints tools from --current; it cannot be combined with --base or --variants") + 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) { - throw new InputError("--variants lists other configurations; it cannot be combined with --base") + 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) })) From fdcb8ef93ba65a7a9ce8e7ee2e17333cf6fd3a94 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:04:24 -0400 Subject: [PATCH 16/29] docs(ship-check): two skill sentences a test reader could take two ways The eight-tool cap is restated beside the Review only rule, and the fact count says it covers the old description and schemas together. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .../ship-check/skills/tool-definition-review/SKILL.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index 99a1875..162b963 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -104,9 +104,10 @@ them for their own review. 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 fully, whether or not it is in the intended-tools list, and - review no other tool. The intended-tools list never changes your scope; it only - decides which changes the report calls unintended. + 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`, @@ -214,7 +215,8 @@ changed definitions afresh re-scores all of them. 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 count is how a reader sees the check ran. +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 From df188847922116ab157acb7a17515d278872b2cf Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:10:49 -0400 Subject: [PATCH 17/29] fix(ship-check): surface-diff reports a removed duplicate line and trims half characters from an overlap Losing one of two identical description lines reported no removed line, because the line comparison used sets. An overlap cut in the middle of a two-unit character kept a stray half at its edge. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .../scripts/__tests__/surface-diff.test.ts | 23 +++++++++++++ .../scripts/surface-diff.ts | 33 +++++++++++++++---- 2 files changed, 49 insertions(+), 7 deletions(-) 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 index a815dae..ba4bbd5 100644 --- 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 @@ -244,6 +244,18 @@ describe("compareSurfaces", () => { }) }) + 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" } }) @@ -479,6 +491,17 @@ describe("duplication candidates", () => { 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", 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 index 171884f..6ba4bb2 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -93,6 +93,9 @@ const SENTENCE_BOUNDARY = /(?<=[.!?])\s+/ // 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 ...]", @@ -261,13 +264,28 @@ const schemaSentences = (tool: Tool): string[] => { 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 beforeSet = new Set(before) - const afterSet = new Set(after) + const copiesBefore = countCopies(before) + const copiesAfter = countCopies(after) return { - added: [...afterSet].filter((text) => !beforeSet.has(text)), - removed: [...beforeSet].filter((text) => !afterSet.has(text)), + added: textsWithMoreCopies(copiesAfter, copiesBefore), + removed: textsWithMoreCopies(copiesBefore, copiesAfter), } } @@ -386,10 +404,11 @@ const findCandidates = (tool: Tool, base: Tool | undefined): Candidate[] => { } return parameterTexts(tool.inputSchema).flatMap(({ parameter, text }) => { - // A trimmed overlap matches the base's repetition even when the change added a space beside it. - // The threshold is checked again because trimming can shorten an overlap below it. + // 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.trim()) + .map((overlap) => overlap.replace(HALF_CHARACTER_AT_EDGE, "").trim()) .filter((overlap) => overlap.length >= MIN_OVERLAP_CHARS) return overlaps.map((overlap) => ({ From 3f7bd43a09306937c115eb63faa97fbc796be67b Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:20:12 -0400 Subject: [PATCH 18/29] fix(ship-check): surface-diff rejects a tool name that could carry shell syntax A reviewer passes tool names from the file under review to --show in a shell command. A name may now hold only letters, digits, and _ . : / -, and the skill tells the reviewer to quote each name. Ship-Check: pr-monitor Co-Authored-By: Claude Fable 5.1 --- .../skills/tool-definition-review/SKILL.md | 12 ++++++++---- .../scripts/__tests__/surface-diff.test.ts | 6 ++++++ .../tool-definition-review/scripts/surface-diff.ts | 9 +++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index 162b963..225686e 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -55,7 +55,7 @@ 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 ...] +bun "${CLAUDE_SKILL_DIR}/scripts/surface-diff.ts" --current --show '' [--show '' ...] ``` - If `${CLAUDE_SKILL_DIR}` appears above as literal text, the script is at @@ -91,9 +91,13 @@ 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. 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`. +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 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 index ba4bbd5..9279d6a 100644 --- 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 @@ -132,6 +132,12 @@ describe("parseSurface", () => { 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" }] }, 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 index 6ba4bb2..aa0a15f 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -90,6 +90,9 @@ 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 @@ -127,6 +130,12 @@ const parseTool = (value: unknown, position: number, label: string): Tool => { 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)) { From 023e243804455852ba39c62dc39b43c4296b6e4d Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:21:50 -0400 Subject: [PATCH 19/29] docs: the two READMEs and the marketplace description use plain punctuation in place of em dashes Twelve lines in three files this branch already edits. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .claude-plugin/marketplace.json | 2 +- README.md | 16 ++++++++-------- plugins/ship-check/README.md | 12 ++++++------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 3761a03..080397d 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,6 +1,6 @@ { "name": "agent-plugins", - "description": "Personal plugins for Claude Code and Claude Cowork — review agents, workflow orchestrators, and specialized skills.", + "description": "Personal plugins for Claude Code and Claude Cowork: review agents, workflow orchestrators, and specialized skills.", "owner": { "name": "aliasunder" }, diff --git a/README.md b/README.md index 223b610..f71b11f 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # agent-plugins -Personal plugin marketplace for Claude Code and Claude Cowork — review agents, workflow orchestrators, and specialized skills. +Personal plugin marketplace for Claude Code and Claude Cowork: review agents, workflow orchestrators, and specialized skills. > [!NOTE] > **This is a personal workflow repo.** The plugins here are built around my @@ -17,13 +17,13 @@ Personal plugin marketplace for Claude Code and Claude Cowork — review agents, | Plugin | Description | |--------|-------------| | [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 | +| [plan-check](plugins/plan-check/) | Pre-implementation plan review: a fresh-eyes agent (plan-reviewer) plus the plan-review skill, which covers 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, script tests (`test.yml`), and PR review (`umm_review.yml`) +- **`.claude-plugin/marketplace.json`**: marketplace manifest listing all plugins +- **`plugins/`**: the plugins themselves (agents, skills, manifests) +- **`.github/workflows/`**: release automation, script tests (`test.yml`), and PR review (`umm_review.yml`) ## Installation @@ -48,11 +48,11 @@ The ship-check tool-definition reviewer runs a bundled script, which needs [Bun] 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) +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 +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 +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/README.md b/plugins/ship-check/README.md index 2d12dff..365076c 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -11,14 +11,14 @@ on demand and is not a pipeline phase. | Agent | Phase | Color | Role | |-------|-------|-------|------| | `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. | +| `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 +Phase 6 (pr-monitor) runs inline in the orchestrator, because 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 @@ -40,8 +40,8 @@ reports the review as `failed`. 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 -- `sequentialthinking` ([sequential-thinking](https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking) MCP) — reasoning organization +- `vault_get_memory` ([vault-cortex](https://github.com/aliasunder/vault-cortex) MCP): user preferences +- `sequentialthinking` ([sequential-thinking](https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking) MCP): reasoning organization ## Usage @@ -55,7 +55,7 @@ Agent({ subagent_type: "ship-check:bug-checker", prompt: "..." }) ``` They can also be dispatched standalone for single-dimension reviews. `fresh-eyes` -runs as Phase 2 in the pipeline and can also be dispatched standalone — either way +runs as Phase 2 in the pipeline and can also be dispatched standalone. Either way it needs the file list in its prompt: ``` From 6354e56ec82d3af5f568ad687b36b46b071cdc82 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:22:56 -0400 Subject: [PATCH 20/29] fix(ship-check): the reviewer marks a schema-shadowed throw unreachable and names the files it searched for a size rule On a real 27-tool change one reviewer dispatch reported two removed error entries as defects although the input schema rejects those inputs before the handler runs, and one dispatch searched two of three instruction files and reported no size rule. Parser and library calls on file content are named as failure paths to trace. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .../skills/tool-definition-review/SKILL.md | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/plugins/ship-check/skills/tool-definition-review/SKILL.md b/plugins/ship-check/skills/tool-definition-review/SKILL.md index 225686e..3e942e1 100644 --- a/plugins/ship-check/skills/tool-definition-review/SKILL.md +++ b/plugins/ship-check/skills/tool-definition-review/SKILL.md @@ -255,19 +255,32 @@ The count is how a reader sees the check ran. 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. + 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` or `MISSING`. - Search the description `--show` printed for the message's own words. Write + 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. @@ -298,9 +311,11 @@ message has no entry, and it is produced in a helper the change never touched. 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 the instruction files for `size`, - `cap`, `allowance`, `budget`, and `chars`, and say in the `Skipped:` line that - the search found nothing. +- **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 @@ -331,8 +346,8 @@ Per tool 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 From efcc51afb6cee8c3a052e7134172dbb6d0cdb5e6 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:27:05 -0400 Subject: [PATCH 21/29] fix(ship-check): --show quotes an unknown tool name the same way the name check does The message printed a name from the command line raw, so a quote or a line break in it broke the message. Ordinary names print exactly as before. Ship-Check: triage Co-Authored-By: Claude Fable 5.1 --- .../skills/tool-definition-review/scripts/surface-diff.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index aa0a15f..155d572 100644 --- a/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts +++ b/plugins/ship-check/skills/tool-definition-review/scripts/surface-diff.ts @@ -536,7 +536,7 @@ export const showTools = (surface: Surface, names: string[], label: string): str const tool = toolsByName.get(name) if (!tool) { - throw new InputError(`${label}: no tool named "${name}"`) + throw new InputError(`${label}: no tool named ${JSON.stringify(name)}`) } return formatTool(tool) From dc1b65064978ea6e36ce9e22191a22039de72ee5 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:28:58 -0400 Subject: [PATCH 22/29] style: replace remaining em dash in plan-check marketplace description The prior commit replaced em dashes in the two READMEs and the top-level marketplace description but missed the plan-check plugin description in the same file. Ship-Check: code-quality Co-Authored-By: Claude Opus 4.6 (1M context) --- .claude-plugin/marketplace.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 080397d..88713d9 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -23,7 +23,7 @@ { "name": "plan-check", "source": "./plugins/plan-check", - "description": "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, and verification-plan safety — before any code exists.", + "description": "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, and verification-plan safety, before any code exists.", "version": "1.1.1", "keywords": [ "plan-review", From 1f1e39f37d810b74d028c64c0d1f96232407ea81 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:44:52 -0400 Subject: [PATCH 23/29] ci: one release run at a time in each release workflow Two manual dispatches started together computed the same next version and raced on the push, and two tag pushes raced on the changelog commit to main. Each workflow now queues a second run behind the first. Ship-Check: pr-monitor Co-Authored-By: Claude Fable 5.1 --- .github/workflows/auto_release.yml | 6 ++++++ .github/workflows/manual_release.yml | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/.github/workflows/auto_release.yml b/.github/workflows/auto_release.yml index d4a3659..20cfcfa 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -8,6 +8,12 @@ on: permissions: contents: write +# Two tags pushed together would both commit the changelog to main and race on the push, +# so a second run waits for the first. +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + jobs: validate: if: "!endsWith(github.actor, '[bot]')" diff --git a/.github/workflows/manual_release.yml b/.github/workflows/manual_release.yml index 334d904..91247db 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -16,6 +16,12 @@ 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 From a1fbac03deb31e39261d57e58f19d3786f5762ab Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:45:59 -0400 Subject: [PATCH 24/29] docs: the remaining public copy uses plain punctuation in place of em dashes Fifteen lines in the plan-check README and manifest, package.json, and SECURITY.md, finishing the change the two READMEs already carry. Ship-Check: pr-monitor Co-Authored-By: Claude Fable 5.1 --- SECURITY.md | 2 +- package.json | 2 +- plugins/plan-check/.claude-plugin/plugin.json | 2 +- plugins/plan-check/README.md | 24 +++++++++---------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index cded1f7..d55f0a1 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,7 +2,7 @@ ## Scope -This repository contains Claude Code plugins — agent and skill markdown files, +This repository contains Claude Code plugins: agent and skill markdown files, JSON manifests, and shell-based CI workflows. There is no runtime server or compiled application code. diff --git a/package.json b/package.json index eb02c4c..12bee72 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "agent-plugins", "version": "1.1.1", - "description": "Plugin marketplace for Claude Code and Claude Cowork — fresh-eyes code review agents, workflow orchestrators, and specialized skills", + "description": "Plugin marketplace for Claude Code and Claude Cowork: fresh-eyes code review agents, workflow orchestrators, and specialized skills", "keywords": [ "ai-agents", "ai-tools", diff --git a/plugins/plan-check/.claude-plugin/plugin.json b/plugins/plan-check/.claude-plugin/plugin.json index 3e0474c..fad4c80 100644 --- a/plugins/plan-check/.claude-plugin/plugin.json +++ b/plugins/plan-check/.claude-plugin/plugin.json @@ -1,5 +1,5 @@ { "name": "plan-check", "version": "1.1.1", - "description": "Pre-implementation plan review. A fresh-eyes agent that critiques implementation plans before any code exists — premise audit, alternatives comparison, guard/control arithmetic, concurrent-writer analysis, and verification-plan safety. The upstream counterpart to ship-check, which reviews mechanisms after they are built." + "description": "Pre-implementation plan review. A fresh-eyes agent that critiques implementation plans before any code exists, covering premise audit, alternatives comparison, guard/control arithmetic, concurrent-writer analysis, and verification-plan safety. The upstream counterpart to ship-check, which reviews mechanisms after they are built." } diff --git a/plugins/plan-check/README.md b/plugins/plan-check/README.md index fac9211..07bdf09 100644 --- a/plugins/plan-check/README.md +++ b/plugins/plan-check/README.md @@ -1,6 +1,6 @@ # plan-check -Pre-implementation plan review — the upstream counterpart to +Pre-implementation plan review, the upstream counterpart to [ship-check](../ship-check/). Ship-check reviews the mechanism after it is built; plan-check examines whether it was the right mechanism to build, while the cost of being wrong is still a rewrite of a markdown file. @@ -11,7 +11,7 @@ The founding case: a row cap on OAuth client registrations was planned, implemented, and passed all four ship-check phases (9 code-level findings, all mechanism-correct). Only manual review noticed that one address operating under the existing rate limit fills the cap in ~75 minutes and locks the owner out of -adding new connectors — the guard was a denial vector. No code review could have +adding new connectors. The guard was a denial vector. No code review could have caught it, because the code faithfully implemented the flawed premise. Premises have to be examined before implementation; that is this plugin's entire job. @@ -19,27 +19,27 @@ have to be examined before implementation; that is this plugin's entire job. | Component | Type | Role | |-----------|------|------| -| `plan-reviewer` | Agent | Fresh-eyes critique of a plan, task note, or plan-mode output. Findings only — never edits, never implements. | +| `plan-reviewer` | Agent | Fresh-eyes critique of a plan, task note, or plan-mode output. Findings only: never edits, never implements. | | `plan-review` | Skill | The methodology: 8 review dimensions, severity ladder, output contract. Usable standalone/inline on surfaces without agents. | ## Review dimensions -1. **Problem framing** — stated as what goes wrong for whom, not as the absence of +1. **Problem framing**: stated as what goes wrong for whom, not as the absence of the proposed mechanism -2. **Premise and assumption audit** — quote, classify (verified / checkable now / +2. **Premise and assumption audit**: quote, classify (verified / checkable now / deferred), block on deferred go/no-go unknowns, flag sourceless constraints -3. **Alternatives and the do-nothing baseline** — one option means no comparison; +3. **Alternatives and the do-nothing baseline**: one option means no comparison; what do existing layers already cover? -4. **Guard/control arithmetic** *(conditional)* — cheapest unauthorized trigger +4. **Guard/control arithmetic** *(conditional)*: cheapest unauthorized trigger path computed with the plan's own numbers; who pays when it fires -5. **Concurrent writers and async state** — every other writer of touched state; +5. **Concurrent writers and async state**: every other writer of touched state; proxy-for-truth conflations; what survives the reset -6. **Scope and proportionality** — post-incident overcompensation, the accepted +6. **Scope and proportionality**: post-incident overcompensation, the accepted mechanism's cost priced against the margin it buys over the next-cheapest alternative, scope creep, multi-PR delivery mechanics -7. **Verification plan quality** — runnable checks that can fail; destructive test +7. **Verification plan quality**: runnable checks that can fail; destructive test steps get their own blast-radius analysis -8. **Structure and open-question hygiene** — task-note conventions; go/no-go +8. **Structure and open-question hygiene**: task-note conventions; go/no-go unknowns separated from user-judgment questions Dimensions and severity rules are distilled from a corpus of real planning @@ -57,5 +57,5 @@ Or dispatch the agent for fresh-eyes review: > Use the plan-reviewer agent to review plans/my-feature.md Like ship-check, the agents load personal context (vault-cortex MCP, fable-mode -skill) and won't work for anyone else without adaptation — the structure and +skill) and won't work for anyone else without adaptation. The structure and review dimensions are the reusable part. From 963f400be8960c11f522b4d89b87057754a44d34 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:48:48 -0400 Subject: [PATCH 25/29] fix(ci): check out current main in manual release, not the dispatch-time commit Without `ref: main`, a queued second dispatch checks out the commit that was at the tip of main when it was dispatched, not the commit the first run left. It reads the pre-bump version, computes the same next version, and fails at `git tag` because the tag already exists. Ship-Check: bug-check Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/manual_release.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/manual_release.yml b/.github/workflows/manual_release.yml index 91247db..07ed2ac 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -36,6 +36,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + ref: main fetch-depth: 0 token: ${{ steps.app-token.outputs.token }} From 5b8f2a3df41b360b7998ef82e09a5012063cece6 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:50:08 -0400 Subject: [PATCH 26/29] fix(ci): a queued manual release checks out the branch tip, and the tag workflow keeps its original scheduling A manual release that waited behind another checked out the commit from dispatch time, computed the version the first run had already tagged, and failed at the tag step. Its checkout now takes the dispatched branch at run time. The concurrency group on the tag workflow is removed: a queue there holds one waiting run, so a third tag pushed in a burst would replace the second and leave that tag without a release. Ship-Check: pr-monitor Co-Authored-By: Claude Fable 5.1 --- .github/workflows/auto_release.yml | 6 ------ .github/workflows/manual_release.yml | 4 +++- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/.github/workflows/auto_release.yml b/.github/workflows/auto_release.yml index 20cfcfa..d4a3659 100644 --- a/.github/workflows/auto_release.yml +++ b/.github/workflows/auto_release.yml @@ -8,12 +8,6 @@ on: permissions: contents: write -# Two tags pushed together would both commit the changelog to main and race on the push, -# so a second run waits for the first. -concurrency: - group: ${{ github.workflow }} - cancel-in-progress: false - jobs: validate: if: "!endsWith(github.actor, '[bot]')" diff --git a/.github/workflows/manual_release.yml b/.github/workflows/manual_release.yml index 07ed2ac..b25e4dc 100644 --- a/.github/workflows/manual_release.yml +++ b/.github/workflows/manual_release.yml @@ -36,7 +36,9 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: main + # 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 }} From efd214fce1bfddebde8841bbd0f69c2933026b1b Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:23:40 -0400 Subject: [PATCH 27/29] docs: restore the original punctuation in the READMEs, manifests, and SECURITY.md Repo docs follow the docs standard, which has no rule against em dashes, so the rewrite of 28 lines is reverted. One change stays because that standard asks for it: the first adapting step in the root README carried two dashes and a nested parenthetical in one sentence and is now two plain sentences. Co-Authored-By: Claude Fable 5.1 --- .claude-plugin/marketplace.json | 4 ++-- README.md | 10 ++++---- SECURITY.md | 2 +- package.json | 2 +- plugins/plan-check/.claude-plugin/plugin.json | 2 +- plugins/plan-check/README.md | 24 +++++++++---------- plugins/ship-check/README.md | 12 +++++----- 7 files changed, 28 insertions(+), 28 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 88713d9..3761a03 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,6 +1,6 @@ { "name": "agent-plugins", - "description": "Personal plugins for Claude Code and Claude Cowork: review agents, workflow orchestrators, and specialized skills.", + "description": "Personal plugins for Claude Code and Claude Cowork — review agents, workflow orchestrators, and specialized skills.", "owner": { "name": "aliasunder" }, @@ -23,7 +23,7 @@ { "name": "plan-check", "source": "./plugins/plan-check", - "description": "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, and verification-plan safety, before any code exists.", + "description": "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, and verification-plan safety — before any code exists.", "version": "1.1.1", "keywords": [ "plan-review", diff --git a/README.md b/README.md index f71b11f..aa64893 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # agent-plugins -Personal plugin marketplace for Claude Code and Claude Cowork: review agents, workflow orchestrators, and specialized skills. +Personal plugin marketplace for Claude Code and Claude Cowork — review agents, workflow orchestrators, and specialized skills. > [!NOTE] > **This is a personal workflow repo.** The plugins here are built around my @@ -17,13 +17,13 @@ Personal plugin marketplace for Claude Code and Claude Cowork: review agents, wo | Plugin | Description | |--------|-------------| | [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, which covers premise audit, alternatives comparison, guard/control arithmetic, concurrent-writer analysis, mechanism-cost proportionality, and verification-plan safety before any code exists | +| [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, script tests (`test.yml`), and PR review (`umm_review.yml`) +- **`.claude-plugin/marketplace.json`** — marketplace manifest listing all plugins +- **`plugins/`** — the plugins themselves (agents, skills, manifests) +- **`.github/workflows/`** — release automation, script tests (`test.yml`), and PR review (`umm_review.yml`) ## Installation diff --git a/SECURITY.md b/SECURITY.md index d55f0a1..cded1f7 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,7 +2,7 @@ ## Scope -This repository contains Claude Code plugins: agent and skill markdown files, +This repository contains Claude Code plugins — agent and skill markdown files, JSON manifests, and shell-based CI workflows. There is no runtime server or compiled application code. diff --git a/package.json b/package.json index 12bee72..eb02c4c 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "agent-plugins", "version": "1.1.1", - "description": "Plugin marketplace for Claude Code and Claude Cowork: fresh-eyes code review agents, workflow orchestrators, and specialized skills", + "description": "Plugin marketplace for Claude Code and Claude Cowork — fresh-eyes code review agents, workflow orchestrators, and specialized skills", "keywords": [ "ai-agents", "ai-tools", diff --git a/plugins/plan-check/.claude-plugin/plugin.json b/plugins/plan-check/.claude-plugin/plugin.json index fad4c80..3e0474c 100644 --- a/plugins/plan-check/.claude-plugin/plugin.json +++ b/plugins/plan-check/.claude-plugin/plugin.json @@ -1,5 +1,5 @@ { "name": "plan-check", "version": "1.1.1", - "description": "Pre-implementation plan review. A fresh-eyes agent that critiques implementation plans before any code exists, covering premise audit, alternatives comparison, guard/control arithmetic, concurrent-writer analysis, and verification-plan safety. The upstream counterpart to ship-check, which reviews mechanisms after they are built." + "description": "Pre-implementation plan review. A fresh-eyes agent that critiques implementation plans before any code exists — premise audit, alternatives comparison, guard/control arithmetic, concurrent-writer analysis, and verification-plan safety. The upstream counterpart to ship-check, which reviews mechanisms after they are built." } diff --git a/plugins/plan-check/README.md b/plugins/plan-check/README.md index 07bdf09..fac9211 100644 --- a/plugins/plan-check/README.md +++ b/plugins/plan-check/README.md @@ -1,6 +1,6 @@ # plan-check -Pre-implementation plan review, the upstream counterpart to +Pre-implementation plan review — the upstream counterpart to [ship-check](../ship-check/). Ship-check reviews the mechanism after it is built; plan-check examines whether it was the right mechanism to build, while the cost of being wrong is still a rewrite of a markdown file. @@ -11,7 +11,7 @@ The founding case: a row cap on OAuth client registrations was planned, implemented, and passed all four ship-check phases (9 code-level findings, all mechanism-correct). Only manual review noticed that one address operating under the existing rate limit fills the cap in ~75 minutes and locks the owner out of -adding new connectors. The guard was a denial vector. No code review could have +adding new connectors — the guard was a denial vector. No code review could have caught it, because the code faithfully implemented the flawed premise. Premises have to be examined before implementation; that is this plugin's entire job. @@ -19,27 +19,27 @@ have to be examined before implementation; that is this plugin's entire job. | Component | Type | Role | |-----------|------|------| -| `plan-reviewer` | Agent | Fresh-eyes critique of a plan, task note, or plan-mode output. Findings only: never edits, never implements. | +| `plan-reviewer` | Agent | Fresh-eyes critique of a plan, task note, or plan-mode output. Findings only — never edits, never implements. | | `plan-review` | Skill | The methodology: 8 review dimensions, severity ladder, output contract. Usable standalone/inline on surfaces without agents. | ## Review dimensions -1. **Problem framing**: stated as what goes wrong for whom, not as the absence of +1. **Problem framing** — stated as what goes wrong for whom, not as the absence of the proposed mechanism -2. **Premise and assumption audit**: quote, classify (verified / checkable now / +2. **Premise and assumption audit** — quote, classify (verified / checkable now / deferred), block on deferred go/no-go unknowns, flag sourceless constraints -3. **Alternatives and the do-nothing baseline**: one option means no comparison; +3. **Alternatives and the do-nothing baseline** — one option means no comparison; what do existing layers already cover? -4. **Guard/control arithmetic** *(conditional)*: cheapest unauthorized trigger +4. **Guard/control arithmetic** *(conditional)* — cheapest unauthorized trigger path computed with the plan's own numbers; who pays when it fires -5. **Concurrent writers and async state**: every other writer of touched state; +5. **Concurrent writers and async state** — every other writer of touched state; proxy-for-truth conflations; what survives the reset -6. **Scope and proportionality**: post-incident overcompensation, the accepted +6. **Scope and proportionality** — post-incident overcompensation, the accepted mechanism's cost priced against the margin it buys over the next-cheapest alternative, scope creep, multi-PR delivery mechanics -7. **Verification plan quality**: runnable checks that can fail; destructive test +7. **Verification plan quality** — runnable checks that can fail; destructive test steps get their own blast-radius analysis -8. **Structure and open-question hygiene**: task-note conventions; go/no-go +8. **Structure and open-question hygiene** — task-note conventions; go/no-go unknowns separated from user-judgment questions Dimensions and severity rules are distilled from a corpus of real planning @@ -57,5 +57,5 @@ Or dispatch the agent for fresh-eyes review: > Use the plan-reviewer agent to review plans/my-feature.md Like ship-check, the agents load personal context (vault-cortex MCP, fable-mode -skill) and won't work for anyone else without adaptation. The structure and +skill) and won't work for anyone else without adaptation — the structure and review dimensions are the reusable part. diff --git a/plugins/ship-check/README.md b/plugins/ship-check/README.md index 365076c..2d12dff 100644 --- a/plugins/ship-check/README.md +++ b/plugins/ship-check/README.md @@ -11,14 +11,14 @@ on demand and is not a pipeline phase. | Agent | Phase | Color | Role | |-------|-------|-------|------| | `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. | +| `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, because it needs user -interaction and continuous monitoring, which agents can't do. `fresh-eyes` can also be dispatched +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 @@ -40,8 +40,8 @@ reports the review as `failed`. 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 -- `sequentialthinking` ([sequential-thinking](https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking) MCP): reasoning organization +- `vault_get_memory` ([vault-cortex](https://github.com/aliasunder/vault-cortex) MCP) — user preferences +- `sequentialthinking` ([sequential-thinking](https://github.com/modelcontextprotocol/servers/tree/main/src/sequentialthinking) MCP) — reasoning organization ## Usage @@ -55,7 +55,7 @@ Agent({ subagent_type: "ship-check:bug-checker", prompt: "..." }) ``` They can also be dispatched standalone for single-dimension reviews. `fresh-eyes` -runs as Phase 2 in the pipeline and can also be dispatched standalone. Either way +runs as Phase 2 in the pipeline and can also be dispatched standalone — either way it needs the file list in its prompt: ``` From 315985c84b16b264c12817bd43f8e4786e82bb9a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:24:16 -0400 Subject: [PATCH 28/29] docs: restore the last three README lines the punctuation rewrite changed The first adapting step and two list items return to their original text, which completes the revert of the punctuation rewrite. Co-Authored-By: Claude Fable 5.1 --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index aa64893..223b610 100644 --- a/README.md +++ b/README.md @@ -48,11 +48,11 @@ The ship-check tool-definition reviewer runs a bundled script, which needs [Bun] If you want to use these plugins as a starting point: -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. +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. 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. +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. +5. The pipeline structure, review dimensions, and procedural triggers in the skills are workflow-agnostic and should transfer as-is ## License From 3a74463113f52e5ed036c099b166e73c4d77e350 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:40:57 -0400 Subject: [PATCH 29/29] docs: the first adapting step in the root README is two plain sentences again The docs standard asks for a split when one sentence carries two dashes and a nested parenthetical. The two list items beside it end with a full stop, matching the other items in the list. Co-Authored-By: Claude Fable 5.1 --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 223b610..aa64893 100644 --- a/README.md +++ b/README.md @@ -48,11 +48,11 @@ The ship-check tool-definition reviewer runs a bundled script, which needs [Bun] 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) +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 +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 +5. The pipeline structure, review dimensions, and procedural triggers in the skills are workflow-agnostic and should transfer as-is. ## License