From 95a98d81b81ee5bde1f14163bbcdb7a6271f518a Mon Sep 17 00:00:00 2001 From: miguelangaranocurrents <140348970+miguelangaranocurrents@users.noreply.github.com> Date: Fri, 25 Sep 2026 10:52:59 -0500 Subject: [PATCH 1/3] ci: flag a renamed tool as withdrawn in the sync The sync's withdraws check only looked for deleted files under src/tools and skills. A rename keeps every file and changes the name clients call, so `currents-get-affected-executions` becoming `currents-get-action-executions` reached a PR with no warning. `validate` now also compares the tool names registered in server.ts before and after the sync, and adds any that disappear to the same `This withdraws:` list. The pattern that finds them moves to scripts/tool-names.mjs so the README generator and this check cannot disagree on which tools exist. Part of ENG-1540. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/sync-from-monorepo.yaml | 11 ++++++ mcp-server/scripts/sync-readme-tools.mjs | 38 ++++--------------- mcp-server/scripts/tool-names.mjs | 46 +++++++++++++++++++++++ mcp-server/scripts/withdrawn-tools.mjs | 43 +++++++++++++++++++++ 4 files changed, 108 insertions(+), 30 deletions(-) create mode 100644 mcp-server/scripts/tool-names.mjs create mode 100644 mcp-server/scripts/withdrawn-tools.mjs diff --git a/.github/workflows/sync-from-monorepo.yaml b/.github/workflows/sync-from-monorepo.yaml index 4b697d4..2c4df2f 100644 --- a/.github/workflows/sync-from-monorepo.yaml +++ b/.github/workflows/sync-from-monorepo.yaml @@ -210,6 +210,17 @@ jobs: # a fifty-file diff. REMOVED="$(git status --porcelain -- mcp-server/src/tools skills \ | awk '$1 ~ /D/ { print $2 }' | tr '\n' ' ')" + # A deleted file is not the only way to withdraw a tool: a rename + # keeps every file and changes the name clients call, which is how + # `currents-get-affected-executions` went without a warning. The + # names are compared as registered in server.ts, before and after. + # The script comes from main's checkout; sync-readme-tools.mjs above + # already ran, so a pattern that no longer matches has failed there. + git show HEAD:mcp-server/src/server.ts > /tmp/server.before.ts + WITHDRAWN="$(node mcp-server/scripts/withdrawn-tools.mjs \ + /tmp/server.before.ts mcp-server/src/server.ts | tr '\n' ' ')" + REMOVED="${REMOVED}${WITHDRAWN}" + REMOVED="${REMOVED% }" echo "removed=$REMOVED" >> "$GITHUB_OUTPUT" if [ -n "$REMOVED" ]; then echo "::warning::This sync withdraws: $REMOVED" diff --git a/mcp-server/scripts/sync-readme-tools.mjs b/mcp-server/scripts/sync-readme-tools.mjs index 2f4b3fd..eebd4e8 100644 --- a/mcp-server/scripts/sync-readme-tools.mjs +++ b/mcp-server/scripts/sync-readme-tools.mjs @@ -16,6 +16,7 @@ import { readFileSync, writeFileSync } from "node:fs"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; import { loadSkills } from "./load-skills.mjs"; +import { registeredTools as parseTools } from "./tool-names.mjs"; const root = join(dirname(fileURLToPath(import.meta.url)), ".."); const readmePath = join(root, "..", "README.md"); @@ -46,28 +47,7 @@ const origResolve = await (async () => { const serverSrc = readFileSync(join(root, "src", "server.ts"), "utf-8"); -// Quote-agnostic: the source is prettier-formatted with singleQuote, but the -// monorepo copy this file's subject is synced from has been both. -// Anchored on the tool name and its description, not on whatever function -// declares them. That call has been renamed three times — `server.registerTool`, -// then a local `registerTool` wrapper applying scope filtering, then -// `catalogTool` building a TOOL_CATALOG the server loops over — and each rename -// silently found zero tools until someone noticed. -// -// `currents-` is the stable part: `server.test.ts` asserts every registered name -// matches it, so a tool that stopped being found here would have to stop being a -// tool. The loop that registers them passes variables, so it cannot match and -// nothing is counted twice. -// -// The character class is the one `server.test.ts` allows, dots and slashes -// included, rather than the narrower `\w`. A name outside it would be skipped -// rather than reported: this only refuses to run when it finds *no* tools, so a -// partial match writes a README missing a tool and says nothing. That would be -// caught next by `host/readme.test.ts`, which fails when a registered tool is -// absent from the table — one step later, and for a reason that does not name -// the cause. -const toolRegex = - /(['"])(currents-[A-Za-z0-9_./-]+)\1\s*,\s*\{\s*description:\s*(['"])((?:\\.|(?!\3)[^\\])*)\3/g; +// What the pattern matches, and why, is documented in `tool-names.mjs`. /** * The lead sentence of a description, for a table cell. Both catalogs carry @@ -94,10 +74,10 @@ function firstSentence(description) { /** * A string literal's source text as the string it declares. * - * The regex above captures what is between the quotes, so every escape in it - * is still two characters. Decoding the quotes alone left `\\` as a pair, which - * the markdown escape below then doubled again — a description declaring - * `C:\Users` reached the README as two backslashes. + * The pattern in `tool-names.mjs` captures what is between the quotes, so + * every escape in it is still two characters. Decoding the quotes alone left + * `\\` as a pair, which the markdown escape below then doubled again — a + * description declaring `C:\Users` reached the README as two backslashes. * * One pass rather than chained replaces, so a decoded backslash is not read * again as the start of the next escape. @@ -111,10 +91,8 @@ function decodeStringLiteral(literal) { ); } -let match; -while ((match = toolRegex.exec(serverSrc)) !== null) { - const name = match[2]; - const description = decodeStringLiteral(match[4]); +for (const { name, rawDescription } of parseTools(serverSrc)) { + const description = decodeStringLiteral(rawDescription); registeredTools.push({ name, shortDesc: firstSentence(description) }); } diff --git a/mcp-server/scripts/tool-names.mjs b/mcp-server/scripts/tool-names.mjs new file mode 100644 index 0000000..c43a368 --- /dev/null +++ b/mcp-server/scripts/tool-names.mjs @@ -0,0 +1,46 @@ +/** + * The tool registrations in `server.ts`, read from its source rather than by + * importing it, so nothing the file carries is executed. + * + * Shared by `sync-readme-tools.mjs`, which builds the README table from them, + * and `withdrawn-tools.mjs`, which compares two copies of `server.ts` to find a + * tool a sync removes or renames. One pattern for both, so the check for a + * withdrawn tool cannot quietly find fewer tools than the README lists. + */ + +// Quote-agnostic: the source is prettier-formatted with singleQuote, but the +// monorepo copy this file's subject is synced from has been both. +// Anchored on the tool name and its description, not on whatever function +// declares them. That call has been renamed three times — `server.registerTool`, +// then a local `registerTool` wrapper applying scope filtering, then +// `catalogTool` building a TOOL_CATALOG the server loops over — and each rename +// silently found zero tools until someone noticed. +// +// `currents-` is the stable part: `server.test.ts` asserts every registered name +// matches it, so a tool that stopped being found here would have to stop being a +// tool. The loop that registers them passes variables, so it cannot match and +// nothing is counted twice. +// +// The character class is the one `server.test.ts` allows, dots and slashes +// included, rather than the narrower `\w`. A name outside it would be skipped +// rather than reported: this only refuses to run when it finds *no* tools, so a +// partial match writes a README missing a tool and says nothing. That would be +// caught next by `host/readme.test.ts`, which fails when a registered tool is +// absent from the table — one step later, and for a reason that does not name +// the cause. +const TOOL_PATTERN = + /(['"])(currents-[A-Za-z0-9_./-]+)\1\s*,\s*\{\s*description:\s*(['"])((?:\\.|(?!\3)[^\\])*)\3/g; + +/** + * Every tool `serverSrc` registers, in source order, with its description as + * written between the quotes (escapes still in place). + * + * @param {string} serverSrc + * @returns {{ name: string, rawDescription: string }[]} + */ +export function registeredTools(serverSrc) { + return [...serverSrc.matchAll(TOOL_PATTERN)].map((match) => ({ + name: match[2], + rawDescription: match[4], + })); +} diff --git a/mcp-server/scripts/withdrawn-tools.mjs b/mcp-server/scripts/withdrawn-tools.mjs new file mode 100644 index 0000000..9901bab --- /dev/null +++ b/mcp-server/scripts/withdrawn-tools.mjs @@ -0,0 +1,43 @@ +/** + * Prints the tools registered in one `server.ts` and absent from another, one + * per line, so a sync can tell whether it withdraws a tool. + * + * Deleted files are not enough to tell. The tools live in files under + * `src/tools`, but the name a client calls is the string `server.ts` + * registers, and a rename changes that string while leaving every file in + * place. `currents-get-affected-executions` became + * `currents-get-action-executions` that way, and the sync reported nothing + * withdrawn. + * + * Usage: node scripts/withdrawn-tools.mjs + * + * Exits 1 when either file registers no tools: a pattern that stopped + * matching would otherwise report every tool withdrawn, or none. + */ + +import { readFileSync } from "node:fs"; +import { registeredTools } from "./tool-names.mjs"; + +const [beforePath, afterPath] = process.argv.slice(2); +if (!beforePath || !afterPath) { + console.error("usage: withdrawn-tools.mjs "); + process.exit(2); +} + +/** @param {string} path */ +function namesIn(path) { + const names = new Set( + registeredTools(readFileSync(path, "utf-8")).map((tool) => tool.name), + ); + if (names.size === 0) { + console.error(`ERROR: no tools found in ${path}`); + process.exit(1); + } + return names; +} + +const before = namesIn(beforePath); +const after = namesIn(afterPath); +for (const name of before) { + if (!after.has(name)) console.log(name); +} From 0a652bdceacf608623144085461fd46137bb72b3 Mon Sep 17 00:00:00 2001 From: miguelangaranocurrents <140348970+miguelangaranocurrents@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:01:37 -0500 Subject: [PATCH 2/3] fix: skip commented-out registrations when reading tool names A registration left in a comment still matched the pattern, so a tool removed that way counted as present and its withdrawal went unreported. Comments are blanked before matching; strings and template literals are stepped over so a `//` in a description is not read as one. Co-Authored-By: Claude Opus 5.5 --- mcp-server/scripts/tool-names.mjs | 52 +++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/mcp-server/scripts/tool-names.mjs b/mcp-server/scripts/tool-names.mjs index c43a368..a450f27 100644 --- a/mcp-server/scripts/tool-names.mjs +++ b/mcp-server/scripts/tool-names.mjs @@ -31,15 +31,63 @@ const TOOL_PATTERN = /(['"])(currents-[A-Za-z0-9_./-]+)\1\s*,\s*\{\s*description:\s*(['"])((?:\\.|(?!\3)[^\\])*)\3/g; +/** + * `src` with every comment blanked to spaces, newlines kept. + * + * A registration left behind in a comment is not a tool, and counting it as + * one would hide exactly the withdrawal `withdrawn-tools.mjs` is looking for: + * the name disappears from the catalog but still matches here. + * + * Strings and template literals are stepped over, so a `//` inside one (a URL + * in a description) does not start a comment. Two things are not handled, + * because `server.ts` has neither: regex literals, and a backtick nested inside + * a template's `${}`. + * + * @param {string} src + * @returns {string} + */ +export function withoutComments(src) { + let out = ""; + let i = 0; + while (i < src.length) { + const char = src[i]; + const next = src[i + 1]; + if (char === "/" && next === "/") { + const end = src.indexOf("\n", i); + const stop = end === -1 ? src.length : end; + out += " ".repeat(stop - i); + i = stop; + } else if (char === "/" && next === "*") { + const end = src.indexOf("*/", i + 2); + const stop = end === -1 ? src.length : end + 2; + out += src.slice(i, stop).replace(/[^\n]/g, " "); + i = stop; + } else if (char === "'" || char === '"' || char === "`") { + let j = i + 1; + while (j < src.length && src[j] !== char) { + j += src[j] === "\\" ? 2 : 1; + } + out += src.slice(i, j + 1); + i = j + 1; + } else { + out += char; + i += 1; + } + } + return out; +} + /** * Every tool `serverSrc` registers, in source order, with its description as - * written between the quotes (escapes still in place). + * written between the quotes (escapes still in place). Registrations inside + * comments are not counted. * * @param {string} serverSrc * @returns {{ name: string, rawDescription: string }[]} */ export function registeredTools(serverSrc) { - return [...serverSrc.matchAll(TOOL_PATTERN)].map((match) => ({ + const code = withoutComments(serverSrc); + return [...code.matchAll(TOOL_PATTERN)].map((match) => ({ name: match[2], rawDescription: match[4], })); From 51bc76004b656f82dc027fcef62409ac6499dd6a Mon Sep 17 00:00:00 2001 From: miguelangaranocurrents <140348970+miguelangaranocurrents@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:09:56 -0500 Subject: [PATCH 3/3] fix: end a line comment at any line terminator Only LF ended a `//` comment, so a file with CR, U+2028 or U+2029 line breaks had everything after its first comment blanked, and the registrations there went unread. Co-Authored-By: Claude Opus 5.5 --- mcp-server/scripts/tool-names.mjs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/mcp-server/scripts/tool-names.mjs b/mcp-server/scripts/tool-names.mjs index a450f27..bf61c5d 100644 --- a/mcp-server/scripts/tool-names.mjs +++ b/mcp-server/scripts/tool-names.mjs @@ -31,8 +31,10 @@ const TOOL_PATTERN = /(['"])(currents-[A-Za-z0-9_./-]+)\1\s*,\s*\{\s*description:\s*(['"])((?:\\.|(?!\3)[^\\])*)\3/g; +const LINE_END = /[\n\r\u2028\u2029]/g; + /** - * `src` with every comment blanked to spaces, newlines kept. + * `src` with every comment blanked to spaces, line breaks kept. * * A registration left behind in a comment is not a tool, and counting it as * one would hide exactly the withdrawal `withdrawn-tools.mjs` is looking for: @@ -53,14 +55,16 @@ export function withoutComments(src) { const char = src[i]; const next = src[i + 1]; if (char === "/" && next === "/") { - const end = src.indexOf("\n", i); - const stop = end === -1 ? src.length : end; + // Every line terminator JavaScript recognises, not only LF: a comment + // running on past a CR would blank the registrations after it. + LINE_END.lastIndex = i; + const stop = LINE_END.exec(src)?.index ?? src.length; out += " ".repeat(stop - i); i = stop; } else if (char === "/" && next === "*") { const end = src.indexOf("*/", i + 2); const stop = end === -1 ? src.length : end + 2; - out += src.slice(i, stop).replace(/[^\n]/g, " "); + out += src.slice(i, stop).replace(/[^\n\r\u2028\u2029]/g, " "); i = stop; } else if (char === "'" || char === '"' || char === "`") { let j = i + 1;