From c0a2a17f4d8f709f2c4d218691fa66f94a347595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Tue, 30 Jun 2026 18:15:58 +0200 Subject: [PATCH 01/16] fix(scripts): stop extract-tools dropping tools with long descriptions --- package.json | 2 +- scripts/extract-tools.mjs | 21 ++++++-- scripts/extract-tools.test.mjs | 89 ++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 scripts/extract-tools.test.mjs diff --git a/package.json b/package.json index aa27172ce..558934202 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "lint": "eslint . --max-warnings 0", "lint:fix": "eslint . --fix", "typecheck:scripts": "tsc --noEmit -p tsconfig.scripts.json", - "test:scripts": "node --test scripts/sync-next-dist-tag.test.mjs", + "test:scripts": "node --test scripts/sync-next-dist-tag.test.mjs scripts/extract-tools.test.mjs", "check:versions": "node scripts/check-workspace-versions.mjs" }, "devDependencies": { diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index ad8415237..65235f48b 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -41,10 +41,17 @@ function extractFromFile(filePath) { let idMatch; while ((idMatch = idPattern.exec(src)) !== null) { const id = idMatch[1]; - const afterId = src.slice(idMatch.index); - // Look for description within the next 2000 chars (handles multi-line) - const descWindow = afterId.slice(0, 2000); + // Bound the search to THIS tool's definition: from its `id:` up to the next + // `id:` declaration in the file (or end of file). No fixed-size window, so a + // long multi-line template-literal description — whose closing delimiter can + // sit thousands of chars past the `id:` (run-sequence's is ~3000) — is still + // captured in full. The old 2000-char window silently dropped such tools, so + // they were never security-scanned by the downstream `spidershield` pass. + const boundary = /\bid:\s*["'][^"']+["']/g; + boundary.lastIndex = idMatch.index + idMatch[0].length; + const next = boundary.exec(src); + const descWindow = src.slice(idMatch.index, next ? next.index : src.length); // Template literal description. // Allow escaped backticks (\`) inside the literal so inline code like @@ -68,6 +75,14 @@ function extractFromFile(filePath) { name: id, description, }); + } else { + // A tool definition has an `id` but no parseable `description`. Don't drop + // it silently (that bug hid run-sequence from the scanner) — warn on + // stderr so the failure is visible, while keeping stdout valid JSON for + // the downstream `spidershield scan --tools-json` consumer. + console.error( + `extract-tools: WARNING: tool "${id}" in ${filePath} has an id but no parseable description; skipping.` + ); } } diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs new file mode 100644 index 000000000..7892f5817 --- /dev/null +++ b/scripts/extract-tools.test.mjs @@ -0,0 +1,89 @@ +/** + * Guards scripts/extract-tools.mjs against silently dropping tool definitions + * with a long `description:` value. run-sequence's description is a multi-line + * template literal whose closing backtick sits ~3000 chars past its `id:`; the + * old extractor only scanned a fixed 2000-char window after each `id:`, so its + * `descMatch` came back null and the tool was never emitted — and therefore + * never security-scanned by the downstream `spidershield scan --tools-json`. + * + * Runs the REAL extractor against the REAL repo and asserts every discoverable + * tool definition is captured. + * + * Run: node --test scripts/extract-tools.test.mjs + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join, extname, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); +const toolsRoot = join(repoRoot, "packages", "tool-server", "src", "tools"); + +function runExtractor() { + const res = spawnSync(process.execPath, ["scripts/extract-tools.mjs"], { + cwd: repoRoot, + encoding: "utf8", + }); + assert.equal(res.status, 0, `extractor exited with ${res.status}\n${res.stderr}`); + return { tools: JSON.parse(res.stdout).tools, stderr: res.stderr ?? "" }; +} + +// Independently discover every string-literal tool id in the source tree, using +// the same `id: "..."` shape the extractor keys off but WITHOUT its (buggy) +// description step. This is the set the extractor must fully capture. Tools +// whose id is a const reference (e.g. await-ui-element's +// `id: AWAIT_UI_ELEMENT_TOOL_ID`) are not string literals and are not in scope +// for this extractor by design, so they are excluded here too. +function discoverIdLiterals() { + const ids = new Set(); + const walk = (dir) => { + for (const name of readdirSync(dir)) { + const full = join(dir, name); + if (statSync(full).isDirectory()) { + walk(full); + } else if (extname(name) === ".ts" && !name.endsWith(".d.ts")) { + const src = readFileSync(full, "utf8"); + const re = /\bid:\s*["']([^"']+)["']/g; + let m; + while ((m = re.exec(src)) !== null) ids.add(m[1]); + } + } + }; + walk(toolsRoot); + return ids; +} + +test("run-sequence (long multi-line description) is captured", () => { + const { tools } = runExtractor(); + const names = tools.map((t) => t.name); + assert.ok( + names.includes("run-sequence"), + "run-sequence was dropped — the description lookahead regressed to a fixed window" + ); +}); + +test("gesture-tap is captured (control)", () => { + const { tools } = runExtractor(); + const names = tools.map((t) => t.name); + assert.ok(names.includes("gesture-tap")); +}); + +test("every tool definition in the source is captured (no silent drops)", () => { + const { tools, stderr } = runExtractor(); + const extracted = new Set(tools.map((t) => t.name)); + const discovered = discoverIdLiterals(); + + const missing = [...discovered].filter((id) => !extracted.has(id)); + assert.deepEqual(missing, [], `tool definitions dropped by the extractor: ${missing.join(", ")}`); + assert.equal( + tools.length, + discovered.size, + "extracted tool count must equal the number of tool definitions in the source" + ); + + // A healthy tree emits no warnings; a future drop must be loud, never silent. + assert.ok(!/WARNING/.test(stderr), `extractor emitted warnings:\n${stderr}`); +}); From ea514c28296bc901a46616d1ae83e6190ab808ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Wed, 1 Jul 2026 13:04:04 +0200 Subject: [PATCH 02/16] fix(scripts): make extract-tools boundary immune to id: inside a description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The next-id: boundary could be truncated by an `id:` token appearing inside a tool's own description template literal (e.g. a structured example like `{ id: "menu-item" }`) or a nested field, dropping that tool from the security scan — the same silent-drop class the PR fixes, via a different trigger. Anchor the description search on its closing delimiter instead, so an inner id: can't cut it off, and guard against borrowing a later tool's description with an intervening-id check. Extract the parser into a pure, exported extractToolsFromSource so the boundary rules are unit-tested directly. Output on the real tree is byte-identical (72 tools, no warnings). --- scripts/extract-tools.mjs | 95 ++++++++++++++++++++++------------ scripts/extract-tools.test.mjs | 54 +++++++++++++++++++ 2 files changed, 116 insertions(+), 33 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 65235f48b..8ecd3f42c 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -10,8 +10,9 @@ import { readFileSync, readdirSync, statSync } from "node:fs"; import { join, extname } from "node:path"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { dirname } from "node:path"; +import { argv as processArgv } from "node:process"; const __filename = fileURLToPath(import.meta.url); const __dir = dirname(__filename); @@ -31,40 +32,55 @@ function walk(dir) { return entries; } -function extractFromFile(filePath) { - const src = readFileSync(filePath, "utf8"); +/** + * Extract `{ name, description }` for every string-literal `id:` tool definition + * in a single source string. Kept as a pure function (I/O separated) so the + * parsing rules can be unit-tested against crafted fixtures. + * + * @param {string} src source text + * @param {string} filePath label used in warnings + * @returns {{name: string, description: string}[]} + */ +export function extractToolsFromSource(src, filePath = "") { const tools = []; - // Match: id: "...", followed anywhere by description: `...` or "..." - // We iterate over all id: occurrences in the file. + // Iterate over every string-literal `id:` occurrence in the file. const idPattern = /\bid:\s*["']([^"']+)["']/g; let idMatch; while ((idMatch = idPattern.exec(src)) !== null) { const id = idMatch[1]; - // Bound the search to THIS tool's definition: from its `id:` up to the next - // `id:` declaration in the file (or end of file). No fixed-size window, so a + // Search forward from this `id:` for the tool's own `description:` value, + // matched together WITH its closing delimiter. No fixed-size window, so a // long multi-line template-literal description — whose closing delimiter can // sit thousands of chars past the `id:` (run-sequence's is ~3000) — is still - // captured in full. The old 2000-char window silently dropped such tools, so - // they were never security-scanned by the downstream `spidershield` pass. - const boundary = /\bid:\s*["'][^"']+["']/g; - boundary.lastIndex = idMatch.index + idMatch[0].length; - const next = boundary.exec(src); - const descWindow = src.slice(idMatch.index, next ? next.index : src.length); + // captured in full. Anchoring on the closing delimiter also means an `id:` + // token that appears INSIDE the description text (e.g. a structured example + // such as `{ id: "menu-item" }`) can't truncate the capture; a fixed window + // or a "stop at the next `id:`" bound would drop the tool and silently skip + // it from the downstream `spidershield` security scan. + const afterId = src.slice(idMatch.index + idMatch[0].length); - // Template literal description. - // Allow escaped backticks (\`) inside the literal so inline code like - // `adb pull` doesn't truncate the captured description. The class matches - // either an escape sequence (\\.) or any char that isn't ` or \. - let descMatch = descWindow.match(/\bdescription:\s*`((?:\\.|[^`\\])*)`/); + // Template literal description first. Allow escaped backticks (\`) inside the + // literal so inline code like `adb pull` doesn't truncate the captured + // description. The class matches either an escape sequence (\\.) or any char + // that isn't ` or \. Then fall back to double- and single-quoted forms. + let descMatch = afterId.match(/\bdescription:\s*`((?:\\.|[^`\\])*)`/); if (!descMatch) { // Double-quoted description - descMatch = descWindow.match(/\bdescription:\s*"((?:[^"\\]|\\.)*)"/); + descMatch = afterId.match(/\bdescription:\s*"((?:[^"\\]|\\.)*)"/); } if (!descMatch) { // Single-quoted description - descMatch = descWindow.match(/\bdescription:\s*'((?:[^'\\]|\\.)*)'/); + descMatch = afterId.match(/\bdescription:\s*'((?:[^'\\]|\\.)*)'/); + } + + // Guard against borrowing a LATER tool's description: if another tool `id:` + // declaration sits between this `id:` and the matched `description:`, this + // tool has no description of its own — a description-less tool must not + // inherit the next tool's. + if (descMatch && /\bid:\s*["'][^"']+["']/.test(afterId.slice(0, descMatch.index))) { + descMatch = null; } if (descMatch) { @@ -89,19 +105,32 @@ function extractFromFile(filePath) { return tools; } -const files = walk(toolsRoot); -const tools = []; -for (const f of files) { - tools.push(...extractFromFile(f)); +function extractFromFile(filePath) { + return extractToolsFromSource(readFileSync(filePath, "utf8"), filePath); +} + +export function extractAllTools() { + const files = walk(toolsRoot); + const tools = []; + for (const f of files) { + tools.push(...extractFromFile(f)); + } + + // Deduplicate by name (take first occurrence) + const seen = new Set(); + return tools.filter((t) => { + if (seen.has(t.name)) return false; + seen.add(t.name); + return true; + }); } -// Deduplicate by name (take first occurrence) -const seen = new Set(); -const unique = tools.filter((t) => { - if (seen.has(t.name)) return false; - seen.add(t.name); - return true; -}); +function main() { + // MCP tools/list format + console.log(JSON.stringify({ tools: extractAllTools() }, null, 2)); +} -// MCP tools/list format -console.log(JSON.stringify({ tools: unique }, null, 2)); +// Run only when invoked directly, not when imported by the test. +if (processArgv[1] && import.meta.url === pathToFileURL(processArgv[1]).href) { + main(); +} diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 7892f5817..1d9e6e65c 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -18,6 +18,7 @@ import { spawnSync } from "node:child_process"; import { readFileSync, readdirSync, statSync } from "node:fs"; import { join, extname, dirname } from "node:path"; import { fileURLToPath } from "node:url"; +import { extractToolsFromSource } from "./extract-tools.mjs"; const repoRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); const toolsRoot = join(repoRoot, "packages", "tool-server", "src", "tools"); @@ -87,3 +88,56 @@ test("every tool definition in the source is captured (no silent drops)", () => // A healthy tree emits no warnings; a future drop must be loud, never silent. assert.ok(!/WARNING/.test(stderr), `extractor emitted warnings:\n${stderr}`); }); + +// --- Boundary-parsing rules (pure, fixture-driven) ------------------------- + +test("a description containing an id: token is still captured in full", () => { + // The closing-delimiter-anchored search must not be truncated by an `id:` + // that appears INSIDE the description text (e.g. a structured example). The + // old "stop at the next id:" bound dropped such a tool — the exact class of + // silent scan-bypass this script exists to prevent. + const src = ` + export const menuTool = defineTool({ + id: "menu-item-tool", + description: \`Opens a context menu at a point. Example payload the agent can send: + { id: "sub-thing", label: "Open" } — a structured example inside the text. + Use when you need to open a nested menu item on screen.\`, + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.ok("menu-item-tool" in byName, "tool with an id: inside its description was dropped"); + assert.ok( + byName["menu-item-tool"].includes("nested menu item"), + "description was truncated at the inner id: instead of its closing backtick" + ); + // The `id: "sub-thing"` inside the description text is not a real tool. + assert.ok(!("sub-thing" in byName), "an id: inside a description was mistaken for a tool"); +}); + +test("a description-less tool does not borrow the next tool's description", () => { + // If another tool's `id:` sits between this id: and the matched description:, + // the description belongs to that later tool — don't inherit it. + const src = ` + export const bare = defineTool({ + id: "no-description-tool", + handler: async () => {}, + }); + export const next = defineTool({ + id: "has-description-tool", + description: "This description belongs to has-description-tool only.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal( + byName["has-description-tool"], + "This description belongs to has-description-tool only." + ); + assert.ok( + !("no-description-tool" in byName), + "a description-less tool borrowed the following tool's description" + ); +}); From 43a533a6b71f4cd5548e716090ab6c2eb9ea0f0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Wed, 1 Jul 2026 13:28:29 +0200 Subject: [PATCH 03/16] fix(scripts): match the nearest description by position, not delimiter form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior forward search tried the three description delimiter forms in fixed priority (template, then double, then single), each an unbounded scan. In a file with two tools using different forms — an earlier double-quoted description and a later template literal — the template regex grabbed the LATER tool's description across the whole file; the intervening-id guard then nulled it and the earlier tool was dropped from the security scan. Locate the nearest `description:` by position instead and parse whatever delimiter it actually uses, so each tool keeps its own adjacent description regardless of the forms other tools use. Real-tree output is unchanged (72 tools, no warnings). --- scripts/extract-tools.mjs | 70 ++++++++++++++++++---------------- scripts/extract-tools.test.mjs | 26 +++++++++++++ 2 files changed, 63 insertions(+), 33 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 8ecd3f42c..638f692a7 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -50,43 +50,47 @@ export function extractToolsFromSource(src, filePath = "") { while ((idMatch = idPattern.exec(src)) !== null) { const id = idMatch[1]; - // Search forward from this `id:` for the tool's own `description:` value, - // matched together WITH its closing delimiter. No fixed-size window, so a - // long multi-line template-literal description — whose closing delimiter can - // sit thousands of chars past the `id:` (run-sequence's is ~3000) — is still - // captured in full. Anchoring on the closing delimiter also means an `id:` - // token that appears INSIDE the description text (e.g. a structured example - // such as `{ id: "menu-item" }`) can't truncate the capture; a fixed window - // or a "stop at the next `id:`" bound would drop the tool and silently skip - // it from the downstream `spidershield` security scan. + // Find the tool's own `description:` by POSITION — the nearest `description:` + // after this `id:`, parsed with its actual closing delimiter. Position (not + // delimiter-form priority) matters when tools in one file use different + // description forms: a form-priority scan (template, then double, then single) + // could grab a LATER tool's template literal ahead of THIS tool's nearer + // double-quoted one, then reject it as belonging to a later id and drop this + // tool. Matching to the closing delimiter also means an `id:`/`description:` + // token appearing INSIDE the description text (e.g. a structured example + // `{ id: "menu-item" }`) can't truncate the capture — a fixed window or a + // "stop at the next `id:`" bound would silently drop the tool from the + // downstream `spidershield` security scan. const afterId = src.slice(idMatch.index + idMatch[0].length); + const descKeyword = afterId.match(/\bdescription:\s*/); - // Template literal description first. Allow escaped backticks (\`) inside the - // literal so inline code like `adb pull` doesn't truncate the captured - // description. The class matches either an escape sequence (\\.) or any char - // that isn't ` or \. Then fall back to double- and single-quoted forms. - let descMatch = afterId.match(/\bdescription:\s*`((?:\\.|[^`\\])*)`/); - if (!descMatch) { - // Double-quoted description - descMatch = afterId.match(/\bdescription:\s*"((?:[^"\\]|\\.)*)"/); - } - if (!descMatch) { - // Single-quoted description - descMatch = afterId.match(/\bdescription:\s*'((?:[^'\\]|\\.)*)'/); - } - - // Guard against borrowing a LATER tool's description: if another tool `id:` - // declaration sits between this `id:` and the matched `description:`, this - // tool has no description of its own — a description-less tool must not - // inherit the next tool's. - if (descMatch && /\bid:\s*["'][^"']+["']/.test(afterId.slice(0, descMatch.index))) { - descMatch = null; + let description = null; + if ( + descKeyword && + // No other tool `id:` between this `id:` and its `description:` ⇒ the + // description is this tool's own, not a later tool's borrowed one. + !/\bid:\s*["'][^"']+["']/.test(afterId.slice(0, descKeyword.index)) + ) { + const value = afterId.slice(descKeyword.index + descKeyword[0].length); + const delim = value[0]; + // Template literal allows escaped backticks (\`) so inline code like + // `adb pull` doesn't truncate; the quoted forms allow the standard escapes. + const valueMatch = + delim === "`" + ? value.match(/^`((?:\\.|[^`\\])*)`/) + : delim === '"' + ? value.match(/^"((?:[^"\\]|\\.)*)"/) + : delim === "'" + ? value.match(/^'((?:[^'\\]|\\.)*)'/) + : null; + if (valueMatch) { + // Unescape template-literal escapes (\` \$ \\) so the description reads + // as the rendered string, not the source form. + description = valueMatch[1].replace(/\\([`$\\])/g, "$1").trim(); + } } - if (descMatch) { - // Unescape template-literal escapes (\` \$ \\) so the description reads - // as the rendered string, not the source form. - const description = descMatch[1].replace(/\\([`$\\])/g, "$1").trim(); + if (description !== null) { tools.push({ name: id, description, diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 1d9e6e65c..40f422524 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -141,3 +141,29 @@ test("a description-less tool does not borrow the next tool's description", () = "a description-less tool borrowed the following tool's description" ); }); + +test("tools with mixed description forms in one file are both captured", () => { + // Position-based (not delimiter-form-priority) matching: an earlier tool's + // double-quoted description must not be lost to a LATER tool's template + // literal. A form-priority scan would grab the later template ahead of the + // nearer double-quoted one, then drop the earlier tool. + const src = ` + export const first = defineTool({ + id: "first-tool", + description: "First tool, plain double-quoted description.", + handler: async () => {}, + }); + export const second = defineTool({ + id: "second-tool", + description: \`Second tool, a template-literal description with some length.\`, + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal(byName["first-tool"], "First tool, plain double-quoted description."); + assert.equal( + byName["second-tool"], + "Second tool, a template-literal description with some length." + ); +}); From 4497f0264001e362d38ee8d10b6926172d38d12a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Wed, 1 Jul 2026 17:25:40 +0200 Subject: [PATCH 04/16] fix(scripts): unescape standard JS escapes, not just backtick/\$/backslash The unescape step only handled \`, \$, and \\, so a real \n/\t in a template-literal description (e.g. flow-add-step.ts's shipped description) survived as a literal two-character backslash-n instead of an actual newline in the extracted (and downstream security-scanned) text. Extend it to the standard escapes. --- scripts/extract-tools.mjs | 15 ++++++++++++--- scripts/extract-tools.test.mjs | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 638f692a7..a96d6ccfd 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -18,6 +18,11 @@ const __filename = fileURLToPath(import.meta.url); const __dir = dirname(__filename); const toolsRoot = join(__dir, "..", "packages", "tool-server", "src", "tools"); +// Standard JS string/template-literal escapes this extractor unescapes when +// rendering a captured description. Anything not listed here (e.g. \x41, +// é) is left as-is rather than guessed at. +const UNESCAPE_MAP = { n: "\n", r: "\r", t: "\t", "0": "\0" }; + function walk(dir) { const entries = []; for (const name of readdirSync(dir)) { @@ -84,9 +89,13 @@ export function extractToolsFromSource(src, filePath = "") { ? value.match(/^'((?:[^'\\]|\\.)*)'/) : null; if (valueMatch) { - // Unescape template-literal escapes (\` \$ \\) so the description reads - // as the rendered string, not the source form. - description = valueMatch[1].replace(/\\([`$\\])/g, "$1").trim(); + // Unescape the standard JS escapes so the description reads as the + // rendered string, not the source form — e.g. a template literal's + // literal "\n" must become an actual newline, not survive as a stray + // backslash-n in the extracted (and downstream security-scanned) text. + description = valueMatch[1] + .replace(/\\([`$\\'"nrt0])/g, (_m, ch) => UNESCAPE_MAP[ch] ?? ch) + .trim(); } } diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 40f422524..31c5cf017 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -167,3 +167,20 @@ test("tools with mixed description forms in one file are both captured", () => { "Second tool, a template-literal description with some length." ); }); + +test("standard escapes render as their actual characters, not literal backslash sequences", () => { + // A template-literal description with a literal "\n" must become a real + // newline in the extracted text — not survive as a stray backslash-n. Real, + // currently-shipping example: flow-add-step.ts's description uses \n between + // sentences. + const src = ` + export const withEscapes = defineTool({ + id: "with-escapes", + description: \`Line one.\\nLine two.\\tTabbed. Say \\\`hi\\\` and \\$done.\`, + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const description = tools.find((t) => t.name === "with-escapes").description; + assert.equal(description, "Line one.\nLine two.\tTabbed. Say `hi` and $done."); +}); From 3e0f5457b0e71a6f18f253149c51bf1c3889e74b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Wed, 1 Jul 2026 19:02:11 +0200 Subject: [PATCH 05/16] style: prettier formatting for extract-tools.mjs --- scripts/extract-tools.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index a96d6ccfd..de207ab67 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -21,7 +21,7 @@ const toolsRoot = join(__dir, "..", "packages", "tool-server", "src", "tools"); // Standard JS string/template-literal escapes this extractor unescapes when // rendering a captured description. Anything not listed here (e.g. \x41, // é) is left as-is rather than guessed at. -const UNESCAPE_MAP = { n: "\n", r: "\r", t: "\t", "0": "\0" }; +const UNESCAPE_MAP = { n: "\n", r: "\r", t: "\t", 0: "\0" }; function walk(dir) { const entries = []; From 8a82f9a8d9207bccc4d30792a6678d4125217607 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 2 Jul 2026 00:19:01 +0200 Subject: [PATCH 06/16] fix(scripts): harden extract-tools against nested id keys Match each tool's description: at the same object-literal brace level as its id:, ignoring braces/tokens inside string and template literals. A nested id: key (e.g. inside a defaultPayload or example object) between a tool's id: and its description: no longer drops the real tool or emits a spurious one - which would have silently removed the tool from the downstream spidershield security scan. Add matrix/nested-id guard tests. --- scripts/extract-tools.mjs | 98 +++++++++++++++++++++++++--------- scripts/extract-tools.test.mjs | 40 ++++++++++++++ 2 files changed, 113 insertions(+), 25 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index de207ab67..08d4a8106 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -37,6 +37,60 @@ function walk(dir) { return entries; } +/** + * From the source that immediately follows a tool's `id: "..."`, return the text + * of that tool's own `description:` value (starting at its opening delimiter), or + * null when the `id:` has no sibling `description:`. + * + * Scans forward tracking object-literal brace depth, ignoring braces and the + * `description:` token whenever they occur inside a string / template literal: + * - the first `description:` seen at depth 0 (a sibling of the `id:`) is the + * tool's own description; a balanced nested object between the two (e.g. + * `capability: { apple: { simulator: true } }`, which 12 real tools already + * have) does not hide it, and + * - if a `}` drops the depth below 0 first, the `id:`'s enclosing object closed + * before any sibling `description:` - the `id:` is a key nested inside another + * object (e.g. `defaultPayload: { id: "example" }`), not a tool definition. + * + * This is what stops a nested `id:` key from either being emitted as a spurious + * tool or silently dropping the real tool from the downstream `spidershield` + * security scan. + * + * @param {string} afterId source text starting just after the `id: "..."` match + * @returns {string | null} + */ +function findOwnDescriptionValue(afterId) { + let depth = 0; + let quote = null; // active string/template delimiter, or null when in code + for (let i = 0; i < afterId.length; i++) { + const ch = afterId[i]; + if (quote !== null) { + if (ch === "\\") { + i++; // skip the escaped character + } else if (ch === quote) { + quote = null; + } + continue; + } + if (ch === '"' || ch === "'" || ch === "`") { + quote = ch; + } else if (ch === "{") { + depth++; + } else if (ch === "}") { + if (--depth < 0) return null; // id's own object closed - not a tool + } else if ( + depth === 0 && + ch === "d" && + // word boundary before the token so `...Xdescription:` doesn't match + !/[A-Za-z0-9_$]/.test(afterId[i - 1] ?? "") + ) { + const kw = afterId.slice(i).match(/^description:\s*/); + if (kw) return afterId.slice(i + kw[0].length); + } + } + return null; +} + /** * Extract `{ name, description }` for every string-literal `id:` tool definition * in a single source string. Kept as a pure function (I/O separated) so the @@ -55,28 +109,19 @@ export function extractToolsFromSource(src, filePath = "") { while ((idMatch = idPattern.exec(src)) !== null) { const id = idMatch[1]; - // Find the tool's own `description:` by POSITION — the nearest `description:` - // after this `id:`, parsed with its actual closing delimiter. Position (not - // delimiter-form priority) matters when tools in one file use different - // description forms: a form-priority scan (template, then double, then single) - // could grab a LATER tool's template literal ahead of THIS tool's nearer - // double-quoted one, then reject it as belonging to a later id and drop this - // tool. Matching to the closing delimiter also means an `id:`/`description:` - // token appearing INSIDE the description text (e.g. a structured example - // `{ id: "menu-item" }`) can't truncate the capture — a fixed window or a - // "stop at the next `id:`" bound would silently drop the tool from the - // downstream `spidershield` security scan. - const afterId = src.slice(idMatch.index + idMatch[0].length); - const descKeyword = afterId.match(/\bdescription:\s*/); + // Resolve this tool's own `description:` at the SAME object level as its + // `id:` (see findOwnDescriptionValue). Matching by brace scope - rather than + // grabbing the nearest `description:` by raw position - means: + // - an `id:`/`description:` token inside a nested value or inside the + // description text (e.g. a `{ id: "menu-item" }` example) can neither + // borrow a description nor drop the real tool, and + // - the value is parsed to its actual closing delimiter, so long multi-line + // template literals are captured in full. + // A null result means this `id:` is a nested object key, not a tool. + const value = findOwnDescriptionValue(src.slice(idMatch.index + idMatch[0].length)); let description = null; - if ( - descKeyword && - // No other tool `id:` between this `id:` and its `description:` ⇒ the - // description is this tool's own, not a later tool's borrowed one. - !/\bid:\s*["'][^"']+["']/.test(afterId.slice(0, descKeyword.index)) - ) { - const value = afterId.slice(descKeyword.index + descKeyword[0].length); + if (value !== null) { const delim = value[0]; // Template literal allows escaped backticks (\`) so inline code like // `adb pull` doesn't truncate; the quoted forms allow the standard escapes. @@ -104,15 +149,18 @@ export function extractToolsFromSource(src, filePath = "") { name: id, description, }); - } else { - // A tool definition has an `id` but no parseable `description`. Don't drop - // it silently (that bug hid run-sequence from the scanner) — warn on - // stderr so the failure is visible, while keeping stdout valid JSON for - // the downstream `spidershield scan --tools-json` consumer. + } else if (value !== null) { + // A real tool: its `description:` was found at the right scope but the + // value couldn't be parsed. Don't drop it silently (that class of bug hid + // run-sequence from the scanner) - warn on stderr so the failure is + // visible, while keeping stdout valid JSON for the downstream + // `spidershield scan --tools-json` consumer. console.error( `extract-tools: WARNING: tool "${id}" in ${filePath} has an id but no parseable description; skipping.` ); } + // value === null: this `id:` is a nested object key, not a tool - skip it + // silently rather than warning about a non-tool. } return tools; diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 31c5cf017..7dcba0218 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -184,3 +184,43 @@ test("standard escapes render as their actual characters, not literal backslash const description = tools.find((t) => t.name === "with-escapes").description; assert.equal(description, "Line one.\nLine two.\tTabbed. Say `hi` and $done."); }); + +test("an id: key inside a nested object value is not mistaken for a tool", () => { + // A structured value between a tool's id: and its description: (a + // defaultPayload, example, etc.) can itself contain an `id:` key. That nested + // id: must neither (a) be emitted as a spurious tool nor (b) drop the real + // tool from the security scan. Matching the description by brace scope rather + // than raw position is what prevents both. + const src = ` + export const createThing = defineTool({ + id: "create-thing", + defaultPayload: { id: "example-id", name: "x" }, + description: "Creates a thing.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal(byName["create-thing"], "Creates a thing.", "real tool dropped by a nested id: key"); + assert.ok(!("example-id" in byName), "a nested object's id: key was emitted as a spurious tool"); + assert.equal(tools.length, 1, "exactly the real tool should be emitted"); +}); + +test("a balanced nested object (with a deep inner id:) between id: and description: keeps the tool", () => { + // Mirrors the real `capability: { apple: { ... } }` shape 12 tools already + // use, but with an inner id: key added at depth 2 - the deepest footgun form. + // The tool must be captured in full and the inner id: must not leak out. + const src = ` + export const t = defineTool({ + id: "native-thing", + capability: { apple: { simulator: true, device: true }, meta: { id: "cap-1" } }, + description: \`Does a native thing.\`, + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal(byName["native-thing"], "Does a native thing."); + assert.ok(!("cap-1" in byName), "a deeply nested id: key was emitted as a spurious tool"); + assert.equal(tools.length, 1); +}); From e35e0d93bc121ef04ed3e7a947490cc78784b217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 2 Jul 2026 00:44:28 +0200 Subject: [PATCH 07/16] fix(scripts): make extract-tools comment-aware and warn on dropped tool ids --- scripts/extract-tools.mjs | 29 +++++++++++++- scripts/extract-tools.test.mjs | 71 ++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 08d4a8106..90c812799 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -72,6 +72,21 @@ function findOwnDescriptionValue(afterId) { } continue; } + // Skip comments so an apostrophe (e.g. `// don't`), a brace, or a stray + // `description:` token inside a comment between the `id:` and the real + // `description:` can't open a fake string / shift the depth / match early. + if (ch === "/" && afterId[i + 1] === "/") { + const nl = afterId.indexOf("\n", i + 2); + if (nl === -1) return null; // line comment runs to EOF; no description follows + i = nl; // loop's i++ steps past the newline + continue; + } + if (ch === "/" && afterId[i + 1] === "*") { + const end = afterId.indexOf("*/", i + 2); + if (end === -1) return null; // unterminated block comment; nothing parseable after + i = end + 1; // land on the '/'; loop's i++ steps past it + continue; + } if (ch === '"' || ch === "'" || ch === "`") { quote = ch; } else if (ch === "{") { @@ -158,9 +173,19 @@ export function extractToolsFromSource(src, filePath = "") { console.error( `extract-tools: WARNING: tool "${id}" in ${filePath} has an id but no parseable description; skipping.` ); + } else { + // value === null: no sibling `description:` was found before this id's + // enclosing object closed. Usually the `id:` is a nested object key (e.g. + // `defaultPayload: { id: "example" }`), correctly not a tool. But a real + // top-level tool with a missing/empty `description` (type-legal: + // `description?` is optional in ToolDefinition) also lands here and would + // be silently omitted from the downstream security scan. The two aren't + // locally distinguishable, so warn on stderr (stdout stays valid JSON) to + // make a real drop loud instead of only a cryptic count mismatch in CI. + console.error( + `extract-tools: WARNING: id "${id}" in ${filePath} has no sibling description at its object scope; skipping (nested object key, or a tool missing its description).` + ); } - // value === null: this `id:` is a nested object key, not a tool - skip it - // silently rather than warning about a non-tool. } return tools; diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 7dcba0218..a7d1352b5 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -224,3 +224,74 @@ test("a balanced nested object (with a deep inner id:) between id: and descripti assert.ok(!("cap-1" in byName), "a deeply nested id: key was emitted as a spurious tool"); assert.equal(tools.length, 1); }); + +test("a line comment with an apostrophe between id: and description: keeps the tool", () => { + // A `//` comment is not code: an apostrophe inside it (e.g. `don't`) must not + // open a fake string that swallows the source up to the next quote, which + // would hide the real description: and silently drop the tool from the scan. + const src = ` + export const t = defineTool({ + id: "line-commented-tool", + // don't forget to update this description when behaviour changes + description: "The real, comment-preceded description.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal( + byName["line-commented-tool"], + "The real, comment-preceded description.", + "a line comment (with an apostrophe) between id: and description: dropped the tool" + ); + assert.equal(tools.length, 1); +}); + +test("a block comment between id: and description: keeps the tool", () => { + // A `/* ... */` block comment must be skipped whole: stray braces, an + // apostrophe, and even a fake `description:` token inside it must not shift + // the brace depth or be matched as the tool's real description. + const src = ` + export const t = defineTool({ + id: "block-commented-tool", + /* don't { do } this: description: "fake" - braces + apostrophe in a comment */ + description: "The real block-comment-preceded description.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal( + byName["block-commented-tool"], + "The real block-comment-preceded description.", + "a block comment between id: and description: dropped the tool or matched the fake description" + ); + assert.equal(tools.length, 1); +}); + +test("a top-level tool with no description is dropped loudly (stderr warning)", () => { + // `description?` is optional in ToolDefinition, so a real top-level tool can + // legally omit it. It can't be extracted, but it must not vanish from the + // security scan silently - it has to warn on stderr so the drop is diagnosed + // rather than only surfacing as a cryptic count mismatch in CI. + const src = ` + export const bare = defineTool({ + id: "no-description-real-tool", + handler: async () => {}, + }); + `; + const warnings = []; + const originalError = console.error; + console.error = (...args) => warnings.push(args.join(" ")); + let tools; + try { + tools = extractToolsFromSource(src, "fixture.ts"); + } finally { + console.error = originalError; + } + assert.equal(tools.length, 0, "a description-less tool must not be emitted"); + assert.ok( + warnings.some((w) => /WARNING/.test(w) && w.includes("no-description-real-tool")), + `expected a stderr WARNING naming the dropped id; got: ${JSON.stringify(warnings)}` + ); +}); From f11a9ed14bfaeee5e8b4feba5f16d4b2f7674f30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 2 Jul 2026 12:43:53 +0200 Subject: [PATCH 08/16] docs(scripts): correct the capability-shape tool count in extract-tools comment --- scripts/extract-tools.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 90c812799..86a33e5e5 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -46,7 +46,7 @@ function walk(dir) { * `description:` token whenever they occur inside a string / template literal: * - the first `description:` seen at depth 0 (a sibling of the `id:`) is the * tool's own description; a balanced nested object between the two (e.g. - * `capability: { apple: { simulator: true } }`, which 12 real tools already + * `capability: { apple: { simulator: true } }`, which 7 real tools already * have) does not hide it, and * - if a `}` drops the depth below 0 first, the `id:`'s enclosing object closed * before any sibling `description:` - the `id:` is a key nested inside another From efdc5fb96a89951c82bdcbb3092bfee32fa0563f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 2 Jul 2026 14:26:25 +0200 Subject: [PATCH 09/16] fix(scripts): make extract-tools id detection lexical, non-static/dup descriptions loud Address review on the extractor that feeds the spidershield security scan: - Detect `id:` only in code context, skipping strings, template literals, and comments. An `id: "..."` inside a comment - e.g. a `// see id: "screenshot"` cross-reference to another tool - was matched as a tool candidate and, via first-wins dedup, could silently overwrite the real tool's description in the scan. Lexing to code context removes that class (and the `$id:` spurious key). - Treat a non-static description value as loud, not silent: a `+` concatenation or an unescaped `${...}` template interpolation is no longer emitted as a truncated leading literal; it warns on stderr and is skipped, so a partial description never reaches the scanner unnoticed. - Warn on a duplicate tool id instead of silently dropping the later one; a same-name collision was an invisible scan-bypass. - Resolve `description:` with a sticky regex anchored at the candidate position instead of allocating a fresh substring per candidate. Real-tree output is unchanged (72 tools, no warnings). --- scripts/extract-tools.mjs | 125 +++++++++++++++++++++++++++++++------- 1 file changed, 102 insertions(+), 23 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 86a33e5e5..923662bb8 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -23,6 +23,14 @@ const toolsRoot = join(__dir, "..", "packages", "tool-server", "src", "tools"); // é) is left as-is rather than guessed at. const UNESCAPE_MAP = { n: "\n", r: "\r", t: "\t", 0: "\0" }; +// Sticky matcher for the `description:` key, anchored per candidate position so +// resolving it costs no per-character substring allocation (see findOwnDescriptionValue). +const DESCRIPTION_KEY = /description:\s*/y; + +// Sticky matcher for an `id: "..."` string literal, anchored at a known +// code-context position (see findIdLiteralsInCode). +const ID_LITERAL = /id:\s*["']([^"']+)["']/y; + function walk(dir) { const entries = []; for (const name of readdirSync(dir)) { @@ -99,13 +107,69 @@ function findOwnDescriptionValue(afterId) { // word boundary before the token so `...Xdescription:` doesn't match !/[A-Za-z0-9_$]/.test(afterId[i - 1] ?? "") ) { - const kw = afterId.slice(i).match(/^description:\s*/); - if (kw) return afterId.slice(i + kw[0].length); + DESCRIPTION_KEY.lastIndex = i; + if (DESCRIPTION_KEY.test(afterId)) return afterId.slice(DESCRIPTION_KEY.lastIndex); } } return null; } +/** + * Find every `id: "..."` string literal that occurs in CODE - never one inside a + * string, template literal, or comment. + * + * A raw global regex over the source matched `id:` tokens anywhere, so an `id:` + * appearing in a comment (e.g. a `// see id: "screenshot"` cross-reference) or in + * another tool's description text became a tool candidate. `findOwnDescriptionValue` + * then read the enclosing object's real `description:` and, via first-wins dedup, + * that spurious entry could silently overwrite a real tool's description in the + * downstream security scan. Lexing to code context first removes that whole class, + * and also skips non-id keys like `$id:` (the `$` fails the word-boundary check). + * + * @param {string} src + * @returns {{ id: string, end: number }[]} end = index just past the matched literal + */ +function findIdLiteralsInCode(src) { + const out = []; + let quote = null; // active string/template delimiter, or null when in code + for (let i = 0; i < src.length; i++) { + const ch = src[i]; + if (quote !== null) { + if (ch === "\\") + i++; // skip the escaped character + else if (ch === quote) quote = null; + continue; + } + if (ch === "/" && src[i + 1] === "/") { + const nl = src.indexOf("\n", i + 2); + if (nl === -1) break; // line comment runs to EOF + i = nl; + continue; + } + if (ch === "/" && src[i + 1] === "*") { + const end = src.indexOf("*/", i + 2); + if (end === -1) break; // unterminated block comment + i = end + 1; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") { + quote = ch; + continue; + } + // A code-context `id:` token. The word-boundary check before it means + // `grid:`, `$id:`, `androidId:`, etc. are not mistaken for a tool id. + if (ch === "i" && !/[A-Za-z0-9_$]/.test(src[i - 1] ?? "")) { + ID_LITERAL.lastIndex = i; + const m = ID_LITERAL.exec(src); + if (m) { + out.push({ id: m[1], end: ID_LITERAL.lastIndex }); + i = ID_LITERAL.lastIndex - 1; // resume just past the value (loop's i++ advances) + } + } + } + return out; +} + /** * Extract `{ name, description }` for every string-literal `id:` tool definition * in a single source string. Kept as a pure function (I/O separated) so the @@ -118,12 +182,7 @@ function findOwnDescriptionValue(afterId) { export function extractToolsFromSource(src, filePath = "") { const tools = []; - // Iterate over every string-literal `id:` occurrence in the file. - const idPattern = /\bid:\s*["']([^"']+)["']/g; - let idMatch; - while ((idMatch = idPattern.exec(src)) !== null) { - const id = idMatch[1]; - + for (const { id, end } of findIdLiteralsInCode(src)) { // Resolve this tool's own `description:` at the SAME object level as its // `id:` (see findOwnDescriptionValue). Matching by brace scope - rather than // grabbing the nearest `description:` by raw position - means: @@ -133,7 +192,7 @@ export function extractToolsFromSource(src, filePath = "") { // - the value is parsed to its actual closing delimiter, so long multi-line // template literals are captured in full. // A null result means this `id:` is a nested object key, not a tool. - const value = findOwnDescriptionValue(src.slice(idMatch.index + idMatch[0].length)); + const value = findOwnDescriptionValue(src.slice(end)); let description = null; if (value !== null) { @@ -149,13 +208,24 @@ export function extractToolsFromSource(src, filePath = "") { ? value.match(/^'((?:[^'\\]|\\.)*)'/) : null; if (valueMatch) { - // Unescape the standard JS escapes so the description reads as the - // rendered string, not the source form — e.g. a template literal's - // literal "\n" must become an actual newline, not survive as a stray - // backslash-n in the extracted (and downstream security-scanned) text. - description = valueMatch[1] - .replace(/\\([`$\\'"nrt0])/g, (_m, ch) => UNESCAPE_MAP[ch] ?? ch) - .trim(); + // The literal is only the whole description if nothing extends it. A + // trailing `+` (string concatenation) or an unescaped `${...}` in a + // template (runtime interpolation) means the rendered text isn't the + // captured literal alone - leave description null so it falls through to + // the loud warn-and-skip below rather than emitting a truncated string + // silently into the security scan. + const rest = value.slice(valueMatch[0].length).replace(/^\s+/, ""); + const isConcatenation = rest.startsWith("+"); + const hasInterpolation = delim === "`" && /\$\{/.test(valueMatch[1].replace(/\\./g, "")); + if (!isConcatenation && !hasInterpolation) { + // Unescape the standard JS escapes so the description reads as the + // rendered string, not the source form — e.g. a template literal's + // literal "\n" must become an actual newline, not survive as a stray + // backslash-n in the extracted (and downstream security-scanned) text. + description = valueMatch[1] + .replace(/\\([`$\\'"nrt0])/g, (_m, ch) => UNESCAPE_MAP[ch] ?? ch) + .trim(); + } } } @@ -166,12 +236,14 @@ export function extractToolsFromSource(src, filePath = "") { }); } else if (value !== null) { // A real tool: its `description:` was found at the right scope but the - // value couldn't be parsed. Don't drop it silently (that class of bug hid - // run-sequence from the scanner) - warn on stderr so the failure is - // visible, while keeping stdout valid JSON for the downstream - // `spidershield scan --tools-json` consumer. + // value is not a single string/template literal (a concatenation, a + // template with `${...}` interpolation, or a non-literal like a const + // reference), so its rendered text can't be captured statically. Don't + // drop it silently (that class of bug hid run-sequence from the scanner) - + // warn on stderr so the failure is visible, while keeping stdout valid + // JSON for the downstream `spidershield scan --tools-json` consumer. console.error( - `extract-tools: WARNING: tool "${id}" in ${filePath} has an id but no parseable description; skipping.` + `extract-tools: WARNING: tool "${id}" in ${filePath} has a description that is not a single string/template literal (concatenation, interpolation, or a non-literal value); skipping.` ); } else { // value === null: no sibling `description:` was found before this id's @@ -202,10 +274,17 @@ export function extractAllTools() { tools.push(...extractFromFile(f)); } - // Deduplicate by name (take first occurrence) + // Deduplicate by name (take first occurrence). A same-name collision means a + // later tool's description never reaches the security scan; warn so that drop + // is loud rather than a silent scan-bypass (tool ids are meant to be unique). const seen = new Set(); return tools.filter((t) => { - if (seen.has(t.name)) return false; + if (seen.has(t.name)) { + console.error( + `extract-tools: WARNING: duplicate tool id "${t.name}"; keeping the first occurrence and skipping the rest.` + ); + return false; + } seen.add(t.name); return true; }); From 972dff081e0ff09b25eee4708ffdf8c4d5638625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 2 Jul 2026 14:26:25 +0200 Subject: [PATCH 10/16] test(scripts): robust no-silent-drop guard and description-fullness coverage - Rewrite the whole-tree guard so a tool that legitimately uses a shape the extractor skips (a nested `id:` key, a non-static description, a description-less tool) no longer turns CI red with a misleading "dropped by the extractor" message: every discovered id must be emitted OR named in a stderr warning (skipped loudly), and no emitted name may be fabricated. It still fails on a genuine SILENT drop. - Assert run-sequence's long description is captured in full (length + final sentence), not just that its name is present; the original bug truncated it. - Add coverage for the new rules: a commented `id:` cross-reference must not corrupt a real tool, concatenation/interpolation descriptions warn and skip, and an escaped \${ stays a literal. - Correct the capability-shape tool count in a comment (7, matching the source). --- scripts/extract-tools.test.mjs | 143 +++++++++++++++++++++++++++++---- 1 file changed, 126 insertions(+), 17 deletions(-) diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index a7d1352b5..d1bf32ce2 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -32,10 +32,13 @@ function runExtractor() { return { tools: JSON.parse(res.stdout).tools, stderr: res.stderr ?? "" }; } -// Independently discover every string-literal tool id in the source tree, using -// the same `id: "..."` shape the extractor keys off but WITHOUT its (buggy) -// description step. This is the set the extractor must fully capture. Tools -// whose id is a const reference (e.g. await-ui-element's +// Independently discover every string-literal `id: "..."` in the source tree, +// using the same shape the extractor keys off but WITHOUT its description step. +// This is a SUPERSET of the tool set the extractor emits: it also matches nested +// object keys (e.g. `defaultPayload: { id: "x" }`) and id:-like tokens the +// extractor deliberately skips. The "no silent drops" test below relies on that: +// every discovered id must be either emitted or loudly warned, never dropped in +// silence. Tools whose id is a const reference (e.g. await-ui-element's // `id: AWAIT_UI_ELEMENT_TOOL_ID`) are not string literals and are not in scope // for this extractor by design, so they are excluded here too. function discoverIdLiterals() { @@ -57,13 +60,26 @@ function discoverIdLiterals() { return ids; } -test("run-sequence (long multi-line description) is captured", () => { +test("run-sequence's long description is captured IN FULL, not truncated", () => { const { tools } = runExtractor(); - const names = tools.map((t) => t.name); + const runSequence = tools.find((t) => t.name === "run-sequence"); assert.ok( - names.includes("run-sequence"), + runSequence, "run-sequence was dropped — the description lookahead regressed to a fixed window" ); + // Assert fullness, not just presence: the original bug truncated at a fixed + // 2000-char window, so a re-truncation could still emit the tool with a + // shortened description and pass a name-only check. The description is ~4285 + // chars; assert it runs well past the old window AND reaches its closing + // backtick (the final sentence), so the capture is provably complete. + assert.ok( + runSequence.description.length > 3000, + `run-sequence description truncated to ${runSequence.description.length} chars (past the old 2000-char window expected)` + ); + assert.ok( + runSequence.description.includes("returns partial results"), + "run-sequence description does not reach its final sentence — captured value is truncated" + ); }); test("gesture-tap is captured (control)", () => { @@ -72,21 +88,36 @@ test("gesture-tap is captured (control)", () => { assert.ok(names.includes("gesture-tap")); }); -test("every tool definition in the source is captured (no silent drops)", () => { +test("no tool definition is silently dropped (every id: is emitted or loudly warned)", () => { const { tools, stderr } = runExtractor(); const extracted = new Set(tools.map((t) => t.name)); const discovered = discoverIdLiterals(); - const missing = [...discovered].filter((id) => !extracted.has(id)); - assert.deepEqual(missing, [], `tool definitions dropped by the extractor: ${missing.join(", ")}`); - assert.equal( - tools.length, - discovered.size, - "extracted tool count must equal the number of tool definitions in the source" + // No fabricated tools: every emitted name is a real id: literal in the source. + const fabricated = [...extracted].filter((id) => !discovered.has(id)); + assert.deepEqual( + fabricated, + [], + `extractor emitted names with no id: literal in the source: ${fabricated.join(", ")}` ); - // A healthy tree emits no warnings; a future drop must be loud, never silent. - assert.ok(!/WARNING/.test(stderr), `extractor emitted warnings:\n${stderr}`); + // Completeness WITHOUT a false positive when a tool legitimately uses a shape + // the extractor is designed to skip: an id: literal the extractor does not + // emit — a nested object key (e.g. `defaultPayload: { id: "x" }`), a tool + // whose description isn't a static string/template literal, or a + // description-less tool — must be named in a stderr WARNING, i.e. skipped + // LOUDLY. This is the exact invariant the PR restores (the old fixed-window + // matcher dropped long-description tools SILENTLY): the check stays green when + // a future tool uses one of those shapes (the extractor warns and the count + // shifts) and fails only on a genuinely SILENT drop. + const silentlyDropped = [...discovered].filter( + (id) => !extracted.has(id) && !stderr.includes(`"${id}"`) + ); + assert.deepEqual( + silentlyDropped, + [], + `id: literals dropped by the extractor with no stderr warning: ${silentlyDropped.join(", ")}` + ); }); // --- Boundary-parsing rules (pure, fixture-driven) ------------------------- @@ -185,6 +216,54 @@ test("standard escapes render as their actual characters, not literal backslash assert.equal(description, "Line one.\nLine two.\tTabbed. Say `hi` and $done."); }); +test("a non-static description (concatenation / interpolation) is dropped loudly, not truncated silently", () => { + // The captured literal is only the whole description if nothing extends it. A + // `+` concatenation or a template `${...}` interpolation means the rendered + // text is more than the leading literal; emitting just that literal would feed + // a SILENTLY truncated string into the security scan — the same silent-partial + // class this script exists to prevent. Both must warn on stderr and be skipped. + for (const [label, src] of [ + [ + "concatenation", + `defineTool({ id: "concat-tool", description: "part A " + "part B", handler() {} });`, + ], + [ + "interpolation", + 'defineTool({ id: "interp-tool", description: `hello ${world} rest`, handler() {} });', + ], + ]) { + const warnings = []; + const originalError = console.error; + console.error = (...args) => warnings.push(args.join(" ")); + let tools; + try { + tools = extractToolsFromSource(src, "fixture.ts"); + } finally { + console.error = originalError; + } + assert.equal(tools.length, 0, `${label}: a truncated description must not be emitted`); + assert.ok( + warnings.some( + (w) => + /WARNING/.test(w) && w.includes(label === "concatenation" ? "concat-tool" : "interp-tool") + ), + `${label}: expected a stderr WARNING; got ${JSON.stringify(warnings)}` + ); + } +}); + +test("an escaped \\${ in a template is a literal, not interpolation, and is captured", () => { + // \\${...} is an escaped dollar-brace: it renders as the literal text "${...}" + // and is NOT runtime interpolation, so the tool must be captured normally. + const src = + 'defineTool({ id: "escaped-dollar", description: `price is \\${amount} today`, handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "escaped-dollar")?.description, + "price is ${amount} today" + ); +}); + test("an id: key inside a nested object value is not mistaken for a tool", () => { // A structured value between a tool's id: and its description: (a // defaultPayload, example, etc.) can itself contain an `id:` key. That nested @@ -207,7 +286,7 @@ test("an id: key inside a nested object value is not mistaken for a tool", () => }); test("a balanced nested object (with a deep inner id:) between id: and description: keeps the tool", () => { - // Mirrors the real `capability: { apple: { ... } }` shape 12 tools already + // Mirrors the real `capability: { apple: { ... } }` shape 7 tools already // use, but with an inner id: key added at depth 2 - the deepest footgun form. // The tool must be captured in full and the inner id: must not leak out. const src = ` @@ -269,6 +348,36 @@ test("a block comment between id: and description: keeps the tool", () => { assert.equal(tools.length, 1); }); +test("an id: literal inside a comment does not corrupt a real tool's description", () => { + // id detection must be lexically aware: an `id: "..."` in a COMMENT (a common + // cross-reference to another tool) must not be picked up as a tool candidate. + // If it were, it would read the enclosing object's real description and, via + // first-wins dedup, silently overwrite the referenced tool's description in the + // security scan — a silent mis-capture, the exact class this script prevents. + const src = ` + export const foo = defineTool({ + // Related to id: "screenshot"; call that first to capture the screen. + id: "foo-tool", + description: "Foo tool description.", + handler: async () => {}, + }); + export const shot = defineTool({ + id: "screenshot", + description: "Take a screenshot of the device.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal( + byName["screenshot"], + "Take a screenshot of the device.", + "a commented id: cross-reference overwrote the real tool's description" + ); + assert.equal(byName["foo-tool"], "Foo tool description."); + assert.equal(tools.length, 2, "exactly the two real tools should be emitted"); +}); + test("a top-level tool with no description is dropped loudly (stderr warning)", () => { // `description?` is optional in ToolDefinition, so a real top-level tool can // legally omit it. It can't be extracted, but it must not vanish from the From a63e99c4540496913f9ccefd127fcb5c2d37d86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 9 Jul 2026 23:59:43 +0200 Subject: [PATCH 11/16] fix(scripts): lex regex literals and reject non-literal description suffixes A regex literal between (or before) a tool's id: and description: corrupted the forward scan: a quote inside one (z.regex(/["']/)) opened a fake string state and a brace (match: /[{]/) shifted the tracked depth, so the tool was warn-skipped out of the spidershield scan; a quote-regex before the id: hid the tool with no warning at all. Both scanners now skip regex literals whole, using the standard prev-token heuristic to keep division as division. A description literal extended by a method/operator suffix (e.g. "...".slice(0, 3)) was emitted as the raw leading literal with no warning - silently wrong text in the security scan. The whole-value check now requires the property to simply end after the literal (',' or '}'), covering concatenation, method suffixes, 'as const', and any other extension. Also exports the duplicate-id dedup as a pure function so its warning path is unit-testable. --- scripts/extract-tools.mjs | 158 +++++++++++++++++++++++++++++++++----- 1 file changed, 137 insertions(+), 21 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 923662bb8..72e64d081 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -31,6 +31,48 @@ const DESCRIPTION_KEY = /description:\s*/y; // code-context position (see findIdLiteralsInCode). const ID_LITERAL = /id:\s*["']([^"']+)["']/y; +// Characters after which a `/` starts a regex literal rather than division. +// The standard prefix heuristic: a regex can only follow an operator, an opening +// bracket, a separator, or the start of the input — never an identifier, a +// number, a closing paren/bracket, or a string (where `/` means division). +// Statement-keyword positions (`return /re/`) don't occur between an object's +// properties, so the character class is sufficient here. +const REGEX_PREV_CHARS = new Set("(,=:[!&|?{};+-*%<>~^"); + +/** + * If a regex literal starts at src[i] (given the last significant code character + * before it), return the index just past its closing `/` and flags; otherwise + * return i (the `/` is division or invalid - treat it as a plain character). + * Handles `[...]` character classes (where `/` does not terminate) and `\` + * escapes. A newline before the closing `/` means it was not a regex literal. + * + * @param {string} src + * @param {number} i index of the opening `/` + * @param {string | null} prevSig last significant code char before i, or null + * @returns {number} + */ +function skipRegexLiteral(src, i, prevSig) { + if (prevSig !== null && !REGEX_PREV_CHARS.has(prevSig)) return i; + let inClass = false; + for (let j = i + 1; j < src.length; j++) { + const ch = src[j]; + if (ch === "\\") { + j++; + } else if (ch === "\n") { + return i; // regex literals are single-line; this `/` was not one + } else if (inClass) { + if (ch === "]") inClass = false; + } else if (ch === "[") { + inClass = true; + } else if (ch === "/") { + let end = j + 1; + while (end < src.length && /[a-zA-Z]/.test(src[end])) end++; // flags + return end; + } + } + return i; +} + function walk(dir) { const entries = []; for (const name of readdirSync(dir)) { @@ -70,12 +112,14 @@ function walk(dir) { function findOwnDescriptionValue(afterId) { let depth = 0; let quote = null; // active string/template delimiter, or null when in code + let prevSig = null; // last significant code char (regex-vs-division context) for (let i = 0; i < afterId.length; i++) { const ch = afterId[i]; if (quote !== null) { if (ch === "\\") { i++; // skip the escaped character } else if (ch === quote) { + prevSig = ch; // a string value just ended; a following `/` is division quote = null; } continue; @@ -95,6 +139,18 @@ function findOwnDescriptionValue(afterId) { i = end + 1; // land on the '/'; loop's i++ steps past it continue; } + // Skip regex literals whole: a quote inside one (e.g. `z.regex(/["']/)`) + // must not open a fake string, and a brace (e.g. `match: /[{]/`) must not + // shift the tracked depth - either would hide the real `description:` and + // drop the tool from the downstream security scan. + if (ch === "/") { + const end = skipRegexLiteral(afterId, i, prevSig); + if (end > i) { + i = end - 1; // loop's i++ steps past the regex + prevSig = ")"; // a value just ended; a following `/` is division + continue; + } + } if (ch === '"' || ch === "'" || ch === "`") { quote = ch; } else if (ch === "{") { @@ -110,6 +166,7 @@ function findOwnDescriptionValue(afterId) { DESCRIPTION_KEY.lastIndex = i; if (DESCRIPTION_KEY.test(afterId)) return afterId.slice(DESCRIPTION_KEY.lastIndex); } + if (!/\s/.test(ch)) prevSig = ch; } return null; } @@ -132,12 +189,16 @@ function findOwnDescriptionValue(afterId) { function findIdLiteralsInCode(src) { const out = []; let quote = null; // active string/template delimiter, or null when in code + let prevSig = null; // last significant code char (regex-vs-division context) for (let i = 0; i < src.length; i++) { const ch = src[i]; if (quote !== null) { if (ch === "\\") i++; // skip the escaped character - else if (ch === quote) quote = null; + else if (ch === quote) { + prevSig = ch; // a string value just ended; a following `/` is division + quote = null; + } continue; } if (ch === "/" && src[i + 1] === "/") { @@ -152,6 +213,17 @@ function findIdLiteralsInCode(src) { i = end + 1; continue; } + // Skip regex literals whole: a quote inside one placed BEFORE an `id:` + // (e.g. `s.replace(/"/g, "")`) would otherwise open a fake string that + // swallows the id, dropping the tool from the scan without a trace. + if (ch === "/") { + const end = skipRegexLiteral(src, i, prevSig); + if (end > i) { + i = end - 1; // loop's i++ steps past the regex + prevSig = ")"; // a value just ended; a following `/` is division + continue; + } + } if (ch === '"' || ch === "'" || ch === "`") { quote = ch; continue; @@ -164,12 +236,46 @@ function findIdLiteralsInCode(src) { if (m) { out.push({ id: m[1], end: ID_LITERAL.lastIndex }); i = ID_LITERAL.lastIndex - 1; // resume just past the value (loop's i++ advances) + prevSig = '"'; // the id literal's closing quote + continue; } } + if (!/\s/.test(ch)) prevSig = ch; } return out; } +/** + * True when nothing extends a captured description literal: after trailing + * whitespace and comments, the property must end (`,` or `}`). Any other suffix + * (`+` concatenation, a `.slice(...)`-style method call, `as const`, ...) means + * the rendered description differs from the captured literal, so the caller + * must warn-skip instead of emitting wrong text into the security scan. + * + * @param {string} rest source text starting just past the literal's closing delimiter + * @returns {boolean} + */ +function literalIsWholeValue(rest) { + for (let i = 0; i < rest.length; i++) { + const ch = rest[i]; + if (/\s/.test(ch)) continue; + if (ch === "/" && rest[i + 1] === "/") { + const nl = rest.indexOf("\n", i + 2); + if (nl === -1) return false; // line comment to EOF - property never ends + i = nl; + continue; + } + if (ch === "/" && rest[i + 1] === "*") { + const end = rest.indexOf("*/", i + 2); + if (end === -1) return false; // unterminated block comment + i = end + 1; + continue; + } + return ch === "," || ch === "}"; + } + return false; // EOF right after the literal - not a well-formed property +} + /** * Extract `{ name, description }` for every string-literal `id:` tool definition * in a single source string. Kept as a pure function (I/O separated) so the @@ -208,16 +314,17 @@ export function extractToolsFromSource(src, filePath = "") { ? value.match(/^'((?:[^'\\]|\\.)*)'/) : null; if (valueMatch) { - // The literal is only the whole description if nothing extends it. A - // trailing `+` (string concatenation) or an unescaped `${...}` in a - // template (runtime interpolation) means the rendered text isn't the - // captured literal alone - leave description null so it falls through to - // the loud warn-and-skip below rather than emitting a truncated string - // silently into the security scan. - const rest = value.slice(valueMatch[0].length).replace(/^\s+/, ""); - const isConcatenation = rest.startsWith("+"); + // The literal is only the whole description if nothing extends it: after + // it (and any whitespace/comments) the property must simply end with `,` + // or `}`. A `+` (concatenation), a `.` (method suffix like + // `".....".slice(0, 3)`), an `as const`, or any other suffix means the + // rendered text differs from the captured literal; an unescaped `${...}` + // in a template means runtime interpolation. Either way, leave + // description null so it falls through to the loud warn-and-skip below + // rather than emitting wrong text silently into the security scan. + const rest = value.slice(valueMatch[0].length); const hasInterpolation = delim === "`" && /\$\{/.test(valueMatch[1].replace(/\\./g, "")); - if (!isConcatenation && !hasInterpolation) { + if (literalIsWholeValue(rest) && !hasInterpolation) { // Unescape the standard JS escapes so the description reads as the // rendered string, not the source form — e.g. a template literal's // literal "\n" must become an actual newline, not survive as a stray @@ -243,7 +350,7 @@ export function extractToolsFromSource(src, filePath = "") { // warn on stderr so the failure is visible, while keeping stdout valid // JSON for the downstream `spidershield scan --tools-json` consumer. console.error( - `extract-tools: WARNING: tool "${id}" in ${filePath} has a description that is not a single string/template literal (concatenation, interpolation, or a non-literal value); skipping.` + `extract-tools: WARNING: tool "${id}" in ${filePath} has a description that is not a single string/template literal (a concatenation, an interpolation, a method/operator suffix, or a non-literal value); skipping.` ); } else { // value === null: no sibling `description:` was found before this id's @@ -267,16 +374,16 @@ function extractFromFile(filePath) { return extractToolsFromSource(readFileSync(filePath, "utf8"), filePath); } -export function extractAllTools() { - const files = walk(toolsRoot); - const tools = []; - for (const f of files) { - tools.push(...extractFromFile(f)); - } - - // Deduplicate by name (take first occurrence). A same-name collision means a - // later tool's description never reaches the security scan; warn so that drop - // is loud rather than a silent scan-bypass (tool ids are meant to be unique). +/** + * Deduplicate tools by name (first occurrence wins). A same-name collision means + * a later tool's description never reaches the security scan; warn so that drop + * is loud rather than a silent scan-bypass (tool ids are meant to be unique). + * Exported so the warning path is unit-testable. + * + * @param {{name: string, description: string}[]} tools + * @returns {{name: string, description: string}[]} + */ +export function dedupeToolsById(tools) { const seen = new Set(); return tools.filter((t) => { if (seen.has(t.name)) { @@ -290,6 +397,15 @@ export function extractAllTools() { }); } +export function extractAllTools() { + const files = walk(toolsRoot); + const tools = []; + for (const f of files) { + tools.push(...extractFromFile(f)); + } + return dedupeToolsById(tools); +} + function main() { // MCP tools/list format console.log(JSON.stringify({ tools: extractAllTools() }, null, 2)); From 105908fae685e0314986458feeeab7163f616111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 9 Jul 2026 23:59:43 +0200 Subject: [PATCH 12/16] test(scripts): replace the discovery-regex oracle with TypeScript-AST ground truth The old oracle regex-matched id: literals anywhere (comments included), so a benign // see id: "x" cross-reference turned CI red, while a real tool regressing to a warn-skip (regex field, concatenation) left the scan with CI green. The suite now parses every tool file with the actual TypeScript parser and requires the extractor's output to equal the AST exactly - names and descriptions byte-for-byte - plus asserts every real description stays a plain literal. typescript is test-only: the extractor itself remains dependency-free for the bare-checkout tool-description-quality workflow. Drops the run-sequence wording/length coupling (a legitimate rewrite no longer fails; a synthetic >4000-char fixture pins the fixed-window regression instead) and adds fixtures for regex-literal siblings, division, method suffixes, single-quoted forms, and the duplicate-id dedup warning. --- scripts/extract-tools.test.mjs | 422 +++++++++++++++++++++++---------- 1 file changed, 295 insertions(+), 127 deletions(-) diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index d1bf32ce2..84e2bf365 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -1,13 +1,21 @@ /** - * Guards scripts/extract-tools.mjs against silently dropping tool definitions - * with a long `description:` value. run-sequence's description is a multi-line - * template literal whose closing backtick sits ~3000 chars past its `id:`; the - * old extractor only scanned a fixed 2000-char window after each `id:`, so its - * `descMatch` came back null and the tool was never emitted — and therefore - * never security-scanned by the downstream `spidershield scan --tools-json`. + * Guards scripts/extract-tools.mjs against dropping or corrupting tool + * definitions before they reach the downstream `spidershield scan --tools-json` + * security scan. The motivating bug: the old extractor scanned a fixed + * 2000-char window after each `id:`, so run-sequence's ~4285-char multi-line + * template-literal description came back null and the tool was silently + * excluded from the scan. * - * Runs the REAL extractor against the REAL repo and asserts every discoverable - * tool definition is captured. + * Ground truth: the real-tree tests parse every tool source file with the + * actual TypeScript parser and require the extractor's output to match the AST + * exactly - names AND descriptions. Any drop (regex-literal lexing gap, window + * regression, walk change), corruption (truncation, bad unescape), or + * fabrication diverges from the AST and fails, with no reliance on stderr + * warning formats and no false red from `id:`-like text in comments or strings. + * + * The TypeScript package is a test-only dependency (unit-tests.yml runs + * `npm ci`): the extractor itself must stay dependency-free because + * tool-description-quality.yml runs it on a bare checkout without installing. * * Run: node --test scripts/extract-tools.test.mjs */ @@ -18,7 +26,8 @@ import { spawnSync } from "node:child_process"; import { readFileSync, readdirSync, statSync } from "node:fs"; import { join, extname, dirname } from "node:path"; import { fileURLToPath } from "node:url"; -import { extractToolsFromSource } from "./extract-tools.mjs"; +import ts from "typescript"; +import { extractToolsFromSource, dedupeToolsById } from "./extract-tools.mjs"; const repoRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); const toolsRoot = join(repoRoot, "packages", "tool-server", "src", "tools"); @@ -32,96 +41,158 @@ function runExtractor() { return { tools: JSON.parse(res.stdout).tools, stderr: res.stderr ?? "" }; } -// Independently discover every string-literal `id: "..."` in the source tree, -// using the same shape the extractor keys off but WITHOUT its description step. -// This is a SUPERSET of the tool set the extractor emits: it also matches nested -// object keys (e.g. `defaultPayload: { id: "x" }`) and id:-like tokens the -// extractor deliberately skips. The "no silent drops" test below relies on that: -// every discovered id must be either emitted or loudly warned, never dropped in -// silence. Tools whose id is a const reference (e.g. await-ui-element's -// `id: AWAIT_UI_ELEMENT_TOOL_ID`) are not string literals and are not in scope -// for this extractor by design, so they are excluded here too. -function discoverIdLiterals() { - const ids = new Set(); +function captureWarnings(fn) { + const warnings = []; + const originalError = console.error; + console.error = (...args) => warnings.push(args.join(" ")); + try { + return { result: fn(), warnings }; + } finally { + console.error = originalError; + } +} + +// --- TypeScript-AST oracle -------------------------------------------------- +// Computes the ground-truth tool set the extractor must emit: every object +// literal with an `id: "..."` string-literal property whose `description:` is a +// plain string / no-substitution template literal. `.text` is the COOKED value +// (escapes resolved), so it doubles as an oracle for the extractor's unescaping. +// Objects whose `description:` exists but is not a plain literal (concatenation, +// interpolation, method call, const reference) are collected as `nonStatic` - +// their rendered text cannot be extracted statically, so the scan would lose +// them; the real tree must not contain any. Ids with no description sibling +// (nested payload keys, descriptionless tools) are out of the emitted set by +// design and the extractor warn-skips them. +function oracleFromSource(src, fileName = "oracle.ts") { + const sourceFile = ts.createSourceFile(fileName, src, ts.ScriptTarget.Latest, true); + const tools = []; + const nonStatic = []; + const visit = (node) => { + if (ts.isObjectLiteralExpression(node)) { + let id = null; + let descNode; + for (const prop of node.properties) { + if (!ts.isPropertyAssignment(prop)) continue; + const name = + ts.isIdentifier(prop.name) || ts.isStringLiteral(prop.name) ? prop.name.text : null; + if (name === "id" && ts.isStringLiteral(prop.initializer)) id = prop.initializer.text; + if (name === "description") descNode = prop.initializer; + } + if (id !== null && descNode !== undefined) { + if (ts.isStringLiteral(descNode) || ts.isNoSubstitutionTemplateLiteral(descNode)) { + tools.push({ name: id, description: descNode.text.trim() }); + } else { + nonStatic.push(id); + } + } + } + ts.forEachChild(node, visit); + }; + visit(sourceFile); + return { tools, nonStatic }; +} + +function oracleFromTree() { + const tools = []; + const nonStatic = []; const walk = (dir) => { for (const name of readdirSync(dir)) { const full = join(dir, name); if (statSync(full).isDirectory()) { walk(full); } else if (extname(name) === ".ts" && !name.endsWith(".d.ts")) { - const src = readFileSync(full, "utf8"); - const re = /\bid:\s*["']([^"']+)["']/g; - let m; - while ((m = re.exec(src)) !== null) ids.add(m[1]); + const fromFile = oracleFromSource(readFileSync(full, "utf8"), full); + tools.push(...fromFile.tools); + nonStatic.push(...fromFile.nonStatic); } } }; walk(toolsRoot); - return ids; + return { tools, nonStatic }; } -test("run-sequence's long description is captured IN FULL, not truncated", () => { +const byName = (a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0); + +// --- Real-tree ground-truth tests ------------------------------------------- + +test("extractor output equals the TypeScript-AST ground truth exactly", () => { const { tools } = runExtractor(); - const runSequence = tools.find((t) => t.name === "run-sequence"); - assert.ok( - runSequence, - "run-sequence was dropped — the description lookahead regressed to a fixed window" - ); - // Assert fullness, not just presence: the original bug truncated at a fixed - // 2000-char window, so a re-truncation could still emit the tool with a - // shortened description and pass a name-only check. The description is ~4285 - // chars; assert it runs well past the old window AND reaches its closing - // backtick (the final sentence), so the capture is provably complete. - assert.ok( - runSequence.description.length > 3000, - `run-sequence description truncated to ${runSequence.description.length} chars (past the old 2000-char window expected)` - ); + const oracle = oracleFromTree(); + + // Sanity floor: an oracle bug that empties BOTH sides would make the + // equality below pass vacuously; the real tree has ~72 tools. assert.ok( - runSequence.description.includes("returns partial results"), - "run-sequence description does not reach its final sentence — captured value is truncated" + oracle.tools.length >= 60, + `oracle found only ${oracle.tools.length} tools - oracle or tree layout broke` ); -}); - -test("gesture-tap is captured (control)", () => { - const { tools } = runExtractor(); - const names = tools.map((t) => t.name); - assert.ok(names.includes("gesture-tap")); -}); -test("no tool definition is silently dropped (every id: is emitted or loudly warned)", () => { - const { tools, stderr } = runExtractor(); - const extracted = new Set(tools.map((t) => t.name)); - const discovered = discoverIdLiterals(); + // Unique ids: a same-name collision would mean one tool's description + // silently replaces another's in the scan. + const names = oracle.tools.map((t) => t.name); + const dupes = names.filter((n, i) => names.indexOf(n) !== i); + assert.deepEqual(dupes, [], `duplicate tool ids in the source tree: ${dupes.join(", ")}`); - // No fabricated tools: every emitted name is a real id: literal in the source. - const fabricated = [...extracted].filter((id) => !discovered.has(id)); + // The invariant: nothing dropped, nothing corrupted, nothing fabricated. + // Name-level diffs first for a readable failure, then the full deep equality + // (which also compares every description byte-for-byte). + const extractedNames = new Set(tools.map((t) => t.name)); + const oracleNames = new Set(names); + const dropped = [...oracleNames].filter((n) => !extractedNames.has(n)); + const fabricated = [...extractedNames].filter((n) => !oracleNames.has(n)); + assert.deepEqual( + dropped, + [], + `tools in the AST but missing from the extractor: ${dropped.join(", ")}` + ); assert.deepEqual( fabricated, [], - `extractor emitted names with no id: literal in the source: ${fabricated.join(", ")}` + `extractor emitted tools the AST does not contain: ${fabricated.join(", ")}` ); + assert.deepEqual([...tools].sort(byName), [...oracle.tools].sort(byName)); +}); - // Completeness WITHOUT a false positive when a tool legitimately uses a shape - // the extractor is designed to skip: an id: literal the extractor does not - // emit — a nested object key (e.g. `defaultPayload: { id: "x" }`), a tool - // whose description isn't a static string/template literal, or a - // description-less tool — must be named in a stderr WARNING, i.e. skipped - // LOUDLY. This is the exact invariant the PR restores (the old fixed-window - // matcher dropped long-description tools SILENTLY): the check stays green when - // a future tool uses one of those shapes (the extractor warns and the count - // shifts) and fails only on a genuinely SILENT drop. - const silentlyDropped = [...discovered].filter( - (id) => !extracted.has(id) && !stderr.includes(`"${id}"`) - ); +test("every real tool description is a plain literal (statically extractable, so it gets scanned)", () => { + const oracle = oracleFromTree(); assert.deepEqual( - silentlyDropped, + oracle.nonStatic, [], - `id: literals dropped by the extractor with no stderr warning: ${silentlyDropped.join(", ")}` + `these tools' descriptions are not a single string/template literal, so the ` + + `extractor cannot capture them and they would drop out of the spidershield ` + + `scan: ${oracle.nonStatic.join(", ")}. Make each a plain literal.` ); }); +test("run-sequence and describe (the two originally dropped tools) and gesture-tap (control) are present", () => { + // Presence-only on purpose: fullness and exact wording are covered + // byte-for-byte by the AST-equality test, so a legitimate rewrite of a + // description cannot fail this suite while capture stays complete. + const { tools } = runExtractor(); + const names = new Set(tools.map((t) => t.name)); + for (const expected of ["run-sequence", "describe", "gesture-tap"]) { + assert.ok(names.has(expected), `${expected} missing from extractor output`); + } +}); + // --- Boundary-parsing rules (pure, fixture-driven) ------------------------- +test("a >4000-char multi-line description is captured in full (the original regression)", () => { + // Pins the fixed-window bug class forever, independent of any real tool's + // current wording or length: the old extractor cut its search at 2000 chars. + const sentence = "Runs one scripted step against the target device and reports the outcome. "; + const longDescription = sentence.repeat(60).trim(); // ~4560 chars + const src = ` + export const longTool = defineTool({ + id: "long-description-tool", + description: \`${longDescription}\`, + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal(tools.length, 1, "long-description tool was dropped"); + assert.equal(tools[0].description, longDescription, "long description was truncated"); +}); + test("a description containing an id: token is still captured in full", () => { // The closing-delimiter-anchored search must not be truncated by an `id:` // that appears INSIDE the description text (e.g. a structured example). The @@ -137,14 +208,14 @@ test("a description containing an id: token is still captured in full", () => { }); `; const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); - assert.ok("menu-item-tool" in byName, "tool with an id: inside its description was dropped"); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.ok("menu-item-tool" in byId, "tool with an id: inside its description was dropped"); assert.ok( - byName["menu-item-tool"].includes("nested menu item"), + byId["menu-item-tool"].includes("nested menu item"), "description was truncated at the inner id: instead of its closing backtick" ); // The `id: "sub-thing"` inside the description text is not a real tool. - assert.ok(!("sub-thing" in byName), "an id: inside a description was mistaken for a tool"); + assert.ok(!("sub-thing" in byId), "an id: inside a description was mistaken for a tool"); }); test("a description-less tool does not borrow the next tool's description", () => { @@ -161,14 +232,14 @@ test("a description-less tool does not borrow the next tool's description", () = handler: async () => {}, }); `; - const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + const { result: tools } = captureWarnings(() => extractToolsFromSource(src, "fixture.ts")); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); assert.equal( - byName["has-description-tool"], + byId["has-description-tool"], "This description belongs to has-description-tool only." ); assert.ok( - !("no-description-tool" in byName), + !("no-description-tool" in byId), "a description-less tool borrowed the following tool's description" ); }); @@ -191,14 +262,29 @@ test("tools with mixed description forms in one file are both captured", () => { }); `; const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); - assert.equal(byName["first-tool"], "First tool, plain double-quoted description."); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal(byId["first-tool"], "First tool, plain double-quoted description."); assert.equal( - byName["second-tool"], + byId["second-tool"], "Second tool, a template-literal description with some length." ); }); +test("single-quoted id and description are captured (with escapes)", () => { + const src = ` + export const sq = defineTool({ + id: 'single-quoted-tool', + description: 'Types the user\\'s text into the focused field.', + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "single-quoted-tool")?.description, + "Types the user's text into the focused field." + ); +}); + test("standard escapes render as their actual characters, not literal backslash sequences", () => { // A template-literal description with a literal "\n" must become a real // newline in the extracted text — not survive as a stray backslash-n. Real, @@ -216,38 +302,109 @@ test("standard escapes render as their actual characters, not literal backslash assert.equal(description, "Line one.\nLine two.\tTabbed. Say `hi` and $done."); }); -test("a non-static description (concatenation / interpolation) is dropped loudly, not truncated silently", () => { - // The captured literal is only the whole description if nothing extends it. A - // `+` concatenation or a template `${...}` interpolation means the rendered - // text is more than the leading literal; emitting just that literal would feed - // a SILENTLY truncated string into the security scan — the same silent-partial - // class this script exists to prevent. Both must warn on stderr and be skipped. - for (const [label, src] of [ +test("a regex literal between id: and description: does not hide the description", () => { + // The lexer must skip regex literals whole: a brace inside a character class + // (`/[{]/`) must not shift the tracked object depth, and a quote inside a + // regex (`/["']/`) must not open a fake string. Either would make the sibling + // description: invisible and drop the tool from the security scan. + for (const [label, field] of [ + ["brace class", "match: /[{]/,"], + ["quote class", `schema: z.string().regex(/["']/),`], + ["replace with a double-quote regex", `normalize: (s) => s.replace(/"/g, ""),`], + ]) { + const src = ` + export const t = defineTool({ + id: "regex-field-tool", + ${field} + description: "Real description of regex-field-tool.", + handler: async () => {}, + }); + `; + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.equal( + tools.find((t) => t.name === "regex-field-tool")?.description, + "Real description of regex-field-tool.", + `${label}: regex literal between id: and description: dropped or corrupted the tool` + ); + assert.deepEqual(warnings, [], `${label}: unexpected warnings`); + } +}); + +test("a quote-bearing regex BEFORE the id: does not swallow the tool", () => { + // A regex containing a quote that appears before the id: (an earlier + // property, or earlier code in the file) must not open a fake string that + // hides the id: token itself — that variant produced no warning at all. + const src = ` + export const t = defineTool({ + normalize: (s) => s.replace(/"/g, ""), + id: "after-regex-tool", + description: "Description after a quote-bearing regex.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "after-regex-tool")?.description, + "Description after a quote-bearing regex." + ); +}); + +test("division is not mistaken for a regex literal", () => { + // The other side of the regex heuristic: `/` after a value (identifier, + // number, closing paren) is division and must be scanned through normally. + const src = ` + export const t = defineTool({ + id: "division-tool", + timeoutMs: 60000 / 2, + budget: total / count, + description: "Kept: the slashes above are division, not regex openers.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "division-tool")?.description, + "Kept: the slashes above are division, not regex openers." + ); +}); + +test("a non-static description (concatenation / interpolation / method suffix) is dropped loudly, not emitted wrong", () => { + // The captured literal is only the description if nothing extends it. A `+` + // concatenation, a template `${...}` interpolation, or a method/operator + // suffix (`"...".slice(0, 3)`) means the rendered text differs from the + // leading literal; emitting just that literal would feed silently wrong text + // into the security scan. All must warn on stderr and be skipped. + for (const [label, id, src] of [ [ "concatenation", + "concat-tool", `defineTool({ id: "concat-tool", description: "part A " + "part B", handler() {} });`, ], [ "interpolation", + "interp-tool", 'defineTool({ id: "interp-tool", description: `hello ${world} rest`, handler() {} });', ], + [ + "slice suffix", + "slice-tool", + `defineTool({ id: "slice-tool", description: "abcdefghij".slice(0, 3), handler() {} });`, + ], + [ + "replace suffix", + "replace-tool", + `defineTool({ id: "replace-tool", description: "hello WORLD".replace("WORLD", "there"), handler() {} });`, + ], ]) { - const warnings = []; - const originalError = console.error; - console.error = (...args) => warnings.push(args.join(" ")); - let tools; - try { - tools = extractToolsFromSource(src, "fixture.ts"); - } finally { - console.error = originalError; - } - assert.equal(tools.length, 0, `${label}: a truncated description must not be emitted`); + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.equal(tools.length, 0, `${label}: a partial/wrong description must not be emitted`); assert.ok( - warnings.some( - (w) => - /WARNING/.test(w) && w.includes(label === "concatenation" ? "concat-tool" : "interp-tool") - ), - `${label}: expected a stderr WARNING; got ${JSON.stringify(warnings)}` + warnings.some((w) => /WARNING/.test(w) && w.includes(id)), + `${label}: expected a stderr WARNING naming ${id}; got ${JSON.stringify(warnings)}` ); } }); @@ -278,10 +435,10 @@ test("an id: key inside a nested object value is not mistaken for a tool", () => handler: async () => {}, }); `; - const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); - assert.equal(byName["create-thing"], "Creates a thing.", "real tool dropped by a nested id: key"); - assert.ok(!("example-id" in byName), "a nested object's id: key was emitted as a spurious tool"); + const { result: tools } = captureWarnings(() => extractToolsFromSource(src, "fixture.ts")); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal(byId["create-thing"], "Creates a thing.", "real tool dropped by a nested id: key"); + assert.ok(!("example-id" in byId), "a nested object's id: key was emitted as a spurious tool"); assert.equal(tools.length, 1, "exactly the real tool should be emitted"); }); @@ -297,10 +454,10 @@ test("a balanced nested object (with a deep inner id:) between id: and descripti handler: async () => {}, }); `; - const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); - assert.equal(byName["native-thing"], "Does a native thing."); - assert.ok(!("cap-1" in byName), "a deeply nested id: key was emitted as a spurious tool"); + const { result: tools } = captureWarnings(() => extractToolsFromSource(src, "fixture.ts")); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); + assert.equal(byId["native-thing"], "Does a native thing."); + assert.ok(!("cap-1" in byId), "a deeply nested id: key was emitted as a spurious tool"); assert.equal(tools.length, 1); }); @@ -317,9 +474,9 @@ test("a line comment with an apostrophe between id: and description: keeps the t }); `; const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); assert.equal( - byName["line-commented-tool"], + byId["line-commented-tool"], "The real, comment-preceded description.", "a line comment (with an apostrophe) between id: and description: dropped the tool" ); @@ -339,9 +496,9 @@ test("a block comment between id: and description: keeps the tool", () => { }); `; const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); assert.equal( - byName["block-commented-tool"], + byId["block-commented-tool"], "The real block-comment-preceded description.", "a block comment between id: and description: dropped the tool or matched the fake description" ); @@ -368,13 +525,13 @@ test("an id: literal inside a comment does not corrupt a real tool's description }); `; const tools = extractToolsFromSource(src, "fixture.ts"); - const byName = Object.fromEntries(tools.map((t) => [t.name, t.description])); + const byId = Object.fromEntries(tools.map((t) => [t.name, t.description])); assert.equal( - byName["screenshot"], + byId["screenshot"], "Take a screenshot of the device.", "a commented id: cross-reference overwrote the real tool's description" ); - assert.equal(byName["foo-tool"], "Foo tool description."); + assert.equal(byId["foo-tool"], "Foo tool description."); assert.equal(tools.length, 2, "exactly the two real tools should be emitted"); }); @@ -389,18 +546,29 @@ test("a top-level tool with no description is dropped loudly (stderr warning)", handler: async () => {}, }); `; - const warnings = []; - const originalError = console.error; - console.error = (...args) => warnings.push(args.join(" ")); - let tools; - try { - tools = extractToolsFromSource(src, "fixture.ts"); - } finally { - console.error = originalError; - } + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); assert.equal(tools.length, 0, "a description-less tool must not be emitted"); assert.ok( warnings.some((w) => /WARNING/.test(w) && w.includes("no-description-real-tool")), `expected a stderr WARNING naming the dropped id; got: ${JSON.stringify(warnings)}` ); }); + +test("duplicate tool ids keep the first occurrence and warn about the rest", () => { + const duplicated = [ + { name: "dup-tool", description: "First definition - must survive." }, + { name: "unique-tool", description: "Unrelated tool." }, + { name: "dup-tool", description: "Second definition - must be skipped loudly." }, + ]; + const { result: deduped, warnings } = captureWarnings(() => dedupeToolsById(duplicated)); + assert.deepEqual(deduped, [ + { name: "dup-tool", description: "First definition - must survive." }, + { name: "unique-tool", description: "Unrelated tool." }, + ]); + assert.ok( + warnings.some((w) => /WARNING/.test(w) && w.includes("dup-tool")), + `expected a duplicate-id WARNING naming dup-tool; got: ${JSON.stringify(warnings)}` + ); +}); From 757b38998f06d7b8d9f5be34ed6f37ad8481be38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Fri, 10 Jul 2026 04:59:56 +0200 Subject: [PATCH 13/16] fix(scripts): sound lexing for keyword regexes, template interpolations, and JS escape semantics Three lexer classes could still corrupt the scan input, each proven by repro: - a regex after a keyword (function helpers like 'return /["']/' share files with tool definitions today) was read as division, so its quotes opened a fake string and every later tool in the file silently vanished; - template literals were lexed as simple quoted strings, so a nested template's opening backtick 'closed' the outer one - a following tool silently vanished, and a 'description: ...' string inside an interpolation could even be emitted as a tool's real description; - a no-substitution template-literal id (id: `x`) was invisible. The scanners now share one set of lexical skip helpers: comments, string literals, template literals with opaque interpolation skipping (recursive), and regex literals gated by the standard prev-token heuristic extended with regex-permitting keywords. Template ids count when non-interpolated; interpolated ids stay out of scope like const-reference ids. Captured text now follows full JS cooked-string semantics: \xNN, \uNNNN, \u{...}, \v/\b/\f, identity escapes (backslash drops), line continuations, and template CRLF/CR -> LF normalization. Previously anything outside a 9-escape whitelist shipped as raw source text with no warning. The interpolation probe replaces escape pairs with a placeholder instead of deleting them, so '$' + escape + '{' can no longer fabricate a fake ${ and warn-skip a valid literal. --- scripts/extract-tools.mjs | 394 +++++++++++++++++++++++++++----------- 1 file changed, 287 insertions(+), 107 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index 72e64d081..ba1886b34 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -4,6 +4,12 @@ * packages/tool-server/src/tools/**\/*.ts and outputs MCP tools/list JSON * suitable for `spidershield scan . --tools-json `. * + * Must stay dependency-free (node: builtins only): the tool-description-quality + * workflow runs it on a bare checkout without `npm ci`. The unit suite + * (extract-tools.test.mjs) independently parses the same tree with the real + * TypeScript parser and asserts this extractor's output matches it exactly, so + * any lexing gap on a shape that actually enters the tree fails CI loudly. + * * Usage: * node scripts/extract-tools.mjs > tools.json */ @@ -18,41 +24,196 @@ const __filename = fileURLToPath(import.meta.url); const __dir = dirname(__filename); const toolsRoot = join(__dir, "..", "packages", "tool-server", "src", "tools"); -// Standard JS string/template-literal escapes this extractor unescapes when -// rendering a captured description. Anything not listed here (e.g. \x41, -// é) is left as-is rather than guessed at. -const UNESCAPE_MAP = { n: "\n", r: "\r", t: "\t", 0: "\0" }; +// Single-character escapes with a non-identity meaning. Everything else +// follows full JS semantics in unescapeJsString: \xNN / \uNNNN / \u{...} decode +// to their code point, a backslash-newline is a line continuation (empty), and +// any other escaped character is an identity escape (the backslash drops). +const UNESCAPE_MAP = { n: "\n", r: "\r", t: "\t", v: "\v", b: "\b", f: "\f", 0: "\0" }; // Sticky matcher for the `description:` key, anchored per candidate position so // resolving it costs no per-character substring allocation (see findOwnDescriptionValue). const DESCRIPTION_KEY = /description:\s*/y; -// Sticky matcher for an `id: "..."` string literal, anchored at a known -// code-context position (see findIdLiteralsInCode). -const ID_LITERAL = /id:\s*["']([^"']+)["']/y; +// Sticky matcher for an `id: "..."` string/template literal with a matching +// closing delimiter, anchored at a known code-context position (see +// findIdLiteralsInCode). A template id is only static without interpolation; +// the caller rejects a captured `${`. +const ID_LITERAL = /id:\s*(["'`])([^"'`]+)\1/y; -// Characters after which a `/` starts a regex literal rather than division. -// The standard prefix heuristic: a regex can only follow an operator, an opening -// bracket, a separator, or the start of the input — never an identifier, a +// Characters after which a `/` starts a regex literal rather than division: +// an operator, an opening bracket, or a separator — never an identifier, a // number, a closing paren/bracket, or a string (where `/` means division). -// Statement-keyword positions (`return /re/`) don't occur between an object's -// properties, so the character class is sufficient here. const REGEX_PREV_CHARS = new Set("(,=:[!&|?{};+-*%<>~^"); +// Keywords a regex literal can directly follow even though they end in an +// identifier character (`return /["']/` is ordinary code in helper functions +// that share a file with tool definitions). +const REGEX_PREV_KEYWORDS = new Set([ + "return", + "typeof", + "case", + "in", + "of", + "delete", + "void", + "throw", + "new", + "do", + "else", + "yield", + "await", + "instanceof", +]); + +function walk(dir) { + const entries = []; + for (const name of readdirSync(dir)) { + const full = join(dir, name); + const stat = statSync(full); + if (stat.isDirectory()) { + entries.push(...walk(full)); + } else if (extname(name) === ".ts" && !name.endsWith(".d.ts")) { + entries.push(full); + } + } + return entries; +} + +// --- Lexical skip helpers ---------------------------------------------------- +// One shared set of rules for everything that is NOT code: comments, string and +// template literals (with `${...}` interpolations skipped opaquely), and regex +// literals. Both scanners drive these, so their notion of "code context" cannot +// drift apart. + +/** @returns {number} index just past the newline ending a `//` comment */ +function skipLineComment(src, i) { + const nl = src.indexOf("\n", i + 2); + return nl === -1 ? src.length : nl + 1; +} + +/** @returns {number} index just past the `*` + `/` ending a block comment */ +function skipBlockComment(src, i) { + const end = src.indexOf("*/", i + 2); + return end === -1 ? src.length : end + 2; +} + +/** src[i] is `'` or `"`. @returns {number} index just past the closing delimiter */ +function skipStringLiteral(src, i) { + const delim = src[i]; + for (let j = i + 1; j < src.length; j++) { + if (src[j] === "\\") j++; + else if (src[j] === delim) return j + 1; + } + return src.length; +} + /** - * If a regex literal starts at src[i] (given the last significant code character - * before it), return the index just past its closing `/` and flags; otherwise - * return i (the `/` is division or invalid - treat it as a plain character). - * Handles `[...]` character classes (where `/` does not terminate) and `\` - * escapes. A newline before the closing `/` means it was not a regex literal. + * src[i] is a backtick. Skips the whole template literal, INCLUDING `${...}` + * interpolations: the code inside an interpolation is not a tool-definition + * site, but its strings, nested templates, comments, regexes, and braces must + * be tracked so the template's real closing backtick is found. Without this, a + * nested template's opening backtick "closes" the outer one and everything + * after is mis-lexed (a fake `description:` inside an interpolation could even + * be emitted as a tool's real description). + * + * @returns {number} index just past the closing backtick + */ +function skipTemplateLiteral(src, i) { + for (let j = i + 1; j < src.length; j++) { + const ch = src[j]; + if (ch === "\\") j++; + else if (ch === "$" && src[j + 1] === "{") j = skipInterpolation(src, j + 1) - 1; + else if (ch === "`") return j + 1; + } + return src.length; +} + +/** src[i] is the `{` of a `${`. @returns {number} index just past the matching `}` */ +function skipInterpolation(src, i) { + let depth = 1; + let prevSig = null; + let prevSigIdx = -1; + for (let j = i + 1; j < src.length; j++) { + const ch = src[j]; + if (ch === "/" && src[j + 1] === "/") { + j = skipLineComment(src, j) - 1; + continue; + } + if (ch === "/" && src[j + 1] === "*") { + j = skipBlockComment(src, j) - 1; + continue; + } + if (ch === "/") { + const end = skipRegexLiteral(src, j, prevSig, prevSigIdx); + if (end > j) { + j = end - 1; + prevSig = ")"; // a value just ended; a following `/` is division + prevSigIdx = j; + continue; + } + } + if (ch === '"' || ch === "'") { + j = skipStringLiteral(src, j) - 1; + prevSig = ch; + prevSigIdx = j; + continue; + } + if (ch === "`") { + j = skipTemplateLiteral(src, j) - 1; + prevSig = "`"; + prevSigIdx = j; + continue; + } + if (ch === "{") { + depth++; + } else if (ch === "}") { + if (--depth === 0) return j + 1; + } + if (!/\s/.test(ch)) { + prevSig = ch; + prevSigIdx = j; + } + } + return src.length; +} + +/** + * True when a `/` may start a regex literal here: after an operator/separator + * character, after a regex-permitting keyword (`return`, `typeof`, ...), or at + * the start of the input. After an identifier, number, closing paren/bracket, + * or string, `/` is division. + * + * @param {string} src + * @param {string | null} prevSig last significant code char, or null + * @param {number} prevSigIdx index of prevSig in src (-1 when null) + */ +function isRegexPosition(src, prevSig, prevSigIdx) { + if (prevSig === null) return true; + if (REGEX_PREV_CHARS.has(prevSig)) return true; + if (/[A-Za-z0-9_$]/.test(prevSig)) { + let start = prevSigIdx; + while (start > 0 && /[A-Za-z0-9_$]/.test(src[start - 1])) start--; + return REGEX_PREV_KEYWORDS.has(src.slice(start, prevSigIdx + 1)); + } + return false; +} + +/** + * If a regex literal starts at src[i] (given the last significant code + * character before it), return the index just past its closing `/` and flags; + * otherwise return i (the `/` is division or invalid - treat it as a plain + * character). Handles `[...]` character classes (where `/` does not terminate) + * and `\` escapes. A newline before the closing `/` means it was not a regex + * literal. * * @param {string} src * @param {number} i index of the opening `/` * @param {string | null} prevSig last significant code char before i, or null + * @param {number} prevSigIdx index of prevSig in src (-1 when null) * @returns {number} */ -function skipRegexLiteral(src, i, prevSig) { - if (prevSig !== null && !REGEX_PREV_CHARS.has(prevSig)) return i; +function skipRegexLiteral(src, i, prevSig, prevSigIdx) { + if (!isRegexPosition(src, prevSig, prevSigIdx)) return i; let inClass = false; for (let j = i + 1; j < src.length; j++) { const ch = src[j]; @@ -73,18 +234,30 @@ function skipRegexLiteral(src, i, prevSig) { return i; } -function walk(dir) { - const entries = []; - for (const name of readdirSync(dir)) { - const full = join(dir, name); - const stat = statSync(full); - if (stat.isDirectory()) { - entries.push(...walk(full)); - } else if (extname(name) === ".ts" && !name.endsWith(".d.ts")) { - entries.push(full); +/** + * Render a captured string/template-literal body as its runtime (cooked) text, + * following JS escape semantics: named escapes (\n, \t, ...), \xNN, \uNNNN, + * \u{...}, line continuations (backslash-newline vanish), and identity escapes + * (the backslash drops). An out-of-range \u{...} is left as source text - such + * a file would not compile anyway. + * + * @param {string} raw + * @returns {string} + */ +function unescapeJsString(raw) { + return raw.replace( + /\\(?:u\{([0-9A-Fa-f]+)\}|u([0-9A-Fa-f]{4})|x([0-9A-Fa-f]{2})|(\r\n|[\s\S]))/g, + (whole, uBrace, u4, x2, single) => { + if (uBrace !== undefined) { + const cp = parseInt(uBrace, 16); + return cp <= 0x10ffff ? String.fromCodePoint(cp) : whole; + } + if (u4 !== undefined) return String.fromCharCode(parseInt(u4, 16)); + if (x2 !== undefined) return String.fromCharCode(parseInt(x2, 16)); + if (single === "\r\n" || single === "\r" || single === "\n") return ""; // line continuation + return UNESCAPE_MAP[single] ?? single; } - } - return entries; + ); } /** @@ -92,8 +265,10 @@ function walk(dir) { * of that tool's own `description:` value (starting at its opening delimiter), or * null when the `id:` has no sibling `description:`. * - * Scans forward tracking object-literal brace depth, ignoring braces and the - * `description:` token whenever they occur inside a string / template literal: + * Scans forward tracking object-literal brace depth, with comments, string and + * template literals (interpolations included), and regex literals skipped whole + * so nothing inside them can open a fake string, shift the depth, or match as a + * `description:` token: * - the first `description:` seen at depth 0 (a sibling of the `id:`) is the * tool's own description; a balanced nested object between the two (e.g. * `capability: { apple: { simulator: true } }`, which 7 real tools already @@ -111,49 +286,40 @@ function walk(dir) { */ function findOwnDescriptionValue(afterId) { let depth = 0; - let quote = null; // active string/template delimiter, or null when in code let prevSig = null; // last significant code char (regex-vs-division context) + let prevSigIdx = -1; for (let i = 0; i < afterId.length; i++) { const ch = afterId[i]; - if (quote !== null) { - if (ch === "\\") { - i++; // skip the escaped character - } else if (ch === quote) { - prevSig = ch; // a string value just ended; a following `/` is division - quote = null; - } - continue; - } - // Skip comments so an apostrophe (e.g. `// don't`), a brace, or a stray - // `description:` token inside a comment between the `id:` and the real - // `description:` can't open a fake string / shift the depth / match early. if (ch === "/" && afterId[i + 1] === "/") { - const nl = afterId.indexOf("\n", i + 2); - if (nl === -1) return null; // line comment runs to EOF; no description follows - i = nl; // loop's i++ steps past the newline + i = skipLineComment(afterId, i) - 1; continue; } if (ch === "/" && afterId[i + 1] === "*") { - const end = afterId.indexOf("*/", i + 2); - if (end === -1) return null; // unterminated block comment; nothing parseable after - i = end + 1; // land on the '/'; loop's i++ steps past it + i = skipBlockComment(afterId, i) - 1; continue; } - // Skip regex literals whole: a quote inside one (e.g. `z.regex(/["']/)`) - // must not open a fake string, and a brace (e.g. `match: /[{]/`) must not - // shift the tracked depth - either would hide the real `description:` and - // drop the tool from the downstream security scan. if (ch === "/") { - const end = skipRegexLiteral(afterId, i, prevSig); + const end = skipRegexLiteral(afterId, i, prevSig, prevSigIdx); if (end > i) { - i = end - 1; // loop's i++ steps past the regex + i = end - 1; prevSig = ")"; // a value just ended; a following `/` is division + prevSigIdx = i; continue; } } - if (ch === '"' || ch === "'" || ch === "`") { - quote = ch; - } else if (ch === "{") { + if (ch === '"' || ch === "'") { + i = skipStringLiteral(afterId, i) - 1; + prevSig = ch; + prevSigIdx = i; + continue; + } + if (ch === "`") { + i = skipTemplateLiteral(afterId, i) - 1; + prevSig = "`"; + prevSigIdx = i; + continue; + } + if (ch === "{") { depth++; } else if (ch === "}") { if (--depth < 0) return null; // id's own object closed - not a tool @@ -166,14 +332,18 @@ function findOwnDescriptionValue(afterId) { DESCRIPTION_KEY.lastIndex = i; if (DESCRIPTION_KEY.test(afterId)) return afterId.slice(DESCRIPTION_KEY.lastIndex); } - if (!/\s/.test(ch)) prevSig = ch; + if (!/\s/.test(ch)) { + prevSig = ch; + prevSigIdx = i; + } } return null; } /** - * Find every `id: "..."` string literal that occurs in CODE - never one inside a - * string, template literal, or comment. + * Find every static `id:` string/template literal that occurs in CODE - never + * one inside a string, template literal (interpolations included), comment, or + * regex literal. * * A raw global regex over the source matched `id:` tokens anywhere, so an `id:` * appearing in a comment (e.g. a `// see id: "screenshot"` cross-reference) or in @@ -183,49 +353,46 @@ function findOwnDescriptionValue(afterId) { * downstream security scan. Lexing to code context first removes that whole class, * and also skips non-id keys like `$id:` (the `$` fails the word-boundary check). * + * A template-literal id (`` id: `x` ``) counts only without `${...}` + * interpolation; an interpolated id is dynamic, like a const-reference id, and + * is out of scope for this static extractor. + * * @param {string} src * @returns {{ id: string, end: number }[]} end = index just past the matched literal */ function findIdLiteralsInCode(src) { const out = []; - let quote = null; // active string/template delimiter, or null when in code let prevSig = null; // last significant code char (regex-vs-division context) + let prevSigIdx = -1; for (let i = 0; i < src.length; i++) { const ch = src[i]; - if (quote !== null) { - if (ch === "\\") - i++; // skip the escaped character - else if (ch === quote) { - prevSig = ch; // a string value just ended; a following `/` is division - quote = null; - } - continue; - } if (ch === "/" && src[i + 1] === "/") { - const nl = src.indexOf("\n", i + 2); - if (nl === -1) break; // line comment runs to EOF - i = nl; + i = skipLineComment(src, i) - 1; continue; } if (ch === "/" && src[i + 1] === "*") { - const end = src.indexOf("*/", i + 2); - if (end === -1) break; // unterminated block comment - i = end + 1; + i = skipBlockComment(src, i) - 1; continue; } - // Skip regex literals whole: a quote inside one placed BEFORE an `id:` - // (e.g. `s.replace(/"/g, "")`) would otherwise open a fake string that - // swallows the id, dropping the tool from the scan without a trace. if (ch === "/") { - const end = skipRegexLiteral(src, i, prevSig); + const end = skipRegexLiteral(src, i, prevSig, prevSigIdx); if (end > i) { - i = end - 1; // loop's i++ steps past the regex + i = end - 1; prevSig = ")"; // a value just ended; a following `/` is division + prevSigIdx = i; continue; } } - if (ch === '"' || ch === "'" || ch === "`") { - quote = ch; + if (ch === '"' || ch === "'") { + i = skipStringLiteral(src, i) - 1; + prevSig = ch; + prevSigIdx = i; + continue; + } + if (ch === "`") { + i = skipTemplateLiteral(src, i) - 1; + prevSig = "`"; + prevSigIdx = i; continue; } // A code-context `id:` token. The word-boundary check before it means @@ -234,13 +401,21 @@ function findIdLiteralsInCode(src) { ID_LITERAL.lastIndex = i; const m = ID_LITERAL.exec(src); if (m) { - out.push({ id: m[1], end: ID_LITERAL.lastIndex }); + // A template id containing `${` is interpolated - dynamic, not a static + // tool id (same class as a const-reference id; out of scope by design). + if (!(m[1] === "`" && m[2].includes("${"))) { + out.push({ id: m[2], end: ID_LITERAL.lastIndex }); + } i = ID_LITERAL.lastIndex - 1; // resume just past the value (loop's i++ advances) - prevSig = '"'; // the id literal's closing quote + prevSig = m[1]; // the id literal's closing delimiter + prevSigIdx = i; continue; } } - if (!/\s/.test(ch)) prevSig = ch; + if (!/\s/.test(ch)) { + prevSig = ch; + prevSigIdx = i; + } } return out; } @@ -303,15 +478,16 @@ export function extractToolsFromSource(src, filePath = "") { let description = null; if (value !== null) { const delim = value[0]; - // Template literal allows escaped backticks (\`) so inline code like - // `adb pull` doesn't truncate; the quoted forms allow the standard escapes. + // Match the whole literal to its true closing delimiter. `\\[\s\S]` + // (not `\\.`) lets an escaped line terminator - a line continuation - + // stay part of one legal literal. const valueMatch = delim === "`" - ? value.match(/^`((?:\\.|[^`\\])*)`/) + ? value.match(/^`((?:\\[\s\S]|[^`\\])*)`/) : delim === '"' - ? value.match(/^"((?:[^"\\]|\\.)*)"/) + ? value.match(/^"((?:[^"\\]|\\[\s\S])*)"/) : delim === "'" - ? value.match(/^'((?:[^'\\]|\\.)*)'/) + ? value.match(/^'((?:[^'\\]|\\[\s\S])*)'/) : null; if (valueMatch) { // The literal is only the whole description if nothing extends it: after @@ -322,16 +498,18 @@ export function extractToolsFromSource(src, filePath = "") { // in a template means runtime interpolation. Either way, leave // description null so it falls through to the loud warn-and-skip below // rather than emitting wrong text silently into the security scan. + // The interpolation probe replaces escape pairs with a placeholder + // (never deletes them) so `$` + escape + `{` cannot glue into a fake + // `${`, and an escaped `\${` or `$\{` stays the literal text it renders as. const rest = value.slice(valueMatch[0].length); - const hasInterpolation = delim === "`" && /\$\{/.test(valueMatch[1].replace(/\\./g, "")); + const hasInterpolation = + delim === "`" && /\$\{/.test(valueMatch[1].replace(/\\[\s\S]/g, "")); if (literalIsWholeValue(rest) && !hasInterpolation) { - // Unescape the standard JS escapes so the description reads as the - // rendered string, not the source form — e.g. a template literal's - // literal "\n" must become an actual newline, not survive as a stray - // backslash-n in the extracted (and downstream security-scanned) text. - description = valueMatch[1] - .replace(/\\([`$\\'"nrt0])/g, (_m, ch) => UNESCAPE_MAP[ch] ?? ch) - .trim(); + // Render the runtime (cooked) text: template literals first normalize + // line terminators (`\r\n` / `\r` cook to `\n` per spec - relevant on + // a CRLF checkout), then JS escape semantics apply. + const text = delim === "`" ? valueMatch[1].replace(/\r\n?/g, "\n") : valueMatch[1]; + description = unescapeJsString(text).trim(); } } } @@ -357,12 +535,14 @@ export function extractToolsFromSource(src, filePath = "") { // enclosing object closed. Usually the `id:` is a nested object key (e.g. // `defaultPayload: { id: "example" }`), correctly not a tool. But a real // top-level tool with a missing/empty `description` (type-legal: - // `description?` is optional in ToolDefinition) also lands here and would - // be silently omitted from the downstream security scan. The two aren't - // locally distinguishable, so warn on stderr (stdout stays valid JSON) to - // make a real drop loud instead of only a cryptic count mismatch in CI. + // `description?` is optional in ToolDefinition) also lands here - as does + // one whose `description:` is written BEFORE its `id:` (this scan is + // forward-only) - and would be silently omitted from the downstream + // security scan. The cases aren't locally distinguishable, so warn on + // stderr (stdout stays valid JSON) to make a real drop loud instead of + // only a cryptic count mismatch in CI. console.error( - `extract-tools: WARNING: id "${id}" in ${filePath} has no sibling description at its object scope; skipping (nested object key, or a tool missing its description).` + `extract-tools: WARNING: id "${id}" in ${filePath} has no sibling description at its object scope; skipping (nested object key, a tool missing its description, or a description written before the id).` ); } } From bc7868566942ce3a2aa020185e7b9af0f4b74d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Fri, 10 Jul 2026 04:59:56 +0200 Subject: [PATCH 14/16] test(scripts): member-kind-aware oracle, id-form parity, cooked-text and search-scope fixtures The oracle previously looked only at plain PropertyAssignments, so a shorthand, getter, method, computed, or spread-carried 'description' left the object out of both buckets - a real tool could leave the scan with CI fully green. Those member kinds now land in the nonStatic bucket (with file names in the failure message). A plain-literal description written BEFORE its id lands in a new 'misordered' bucket with an actionable message, replacing the misleading 'missing from the extractor' failure. An 'id: "x" as const' is unwrapped (as/satisfies/parens) so a correct extraction is no longer reported as a fabrication, and template-literal ids are recognized on both sides. New fixtures pin the lexer soundness cases (keyword-position regex, nested templates, interpolation containing a fake description:, template and as-const ids), cooked-text fidelity (escape suite, CRLF cooking, $-escape no-glue, trim), and search scope (>2000 chars of siblings between id and description, nested description: key) - each verified to fail under a targeted mutation of the extractor. Declares typescript as a root devDependency: the suite imports it, and it previously resolved only through workspace hoisting. --- package-lock.json | 1 + package.json | 1 + scripts/extract-tools.test.mjs | 405 ++++++++++++++++++++++++++++++--- 3 files changed, 378 insertions(+), 29 deletions(-) diff --git a/package-lock.json b/package-lock.json index aee677c0b..332deb6bf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,6 +14,7 @@ "globals": "^17.7.0", "prettier": "^3.8.5", "semver": "^7.8.5", + "typescript": "^6.0.3", "typescript-eslint": "^8.62.0" } }, diff --git a/package.json b/package.json index 4012e1373..763750c69 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ "globals": "^17.7.0", "prettier": "^3.8.5", "semver": "^7.8.5", + "typescript": "^6.0.3", "typescript-eslint": "^8.62.0" } } diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 84e2bf365..80bcb1d70 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -13,9 +13,10 @@ * fabrication diverges from the AST and fails, with no reliance on stderr * warning formats and no false red from `id:`-like text in comments or strings. * - * The TypeScript package is a test-only dependency (unit-tests.yml runs - * `npm ci`): the extractor itself must stay dependency-free because - * tool-description-quality.yml runs it on a bare checkout without installing. + * The TypeScript package is a test-only dependency (a root devDependency, + * installed by unit-tests.yml's `npm ci`): the extractor itself must stay + * dependency-free because tool-description-quality.yml runs it on a bare + * checkout without installing. * * Run: node --test scripts/extract-tools.test.mjs */ @@ -54,47 +55,120 @@ function captureWarnings(fn) { // --- TypeScript-AST oracle -------------------------------------------------- // Computes the ground-truth tool set the extractor must emit: every object -// literal with an `id: "..."` string-literal property whose `description:` is a -// plain string / no-substitution template literal. `.text` is the COOKED value -// (escapes resolved), so it doubles as an oracle for the extractor's unescaping. -// Objects whose `description:` exists but is not a plain literal (concatenation, -// interpolation, method call, const reference) are collected as `nonStatic` - -// their rendered text cannot be extracted statically, so the scan would lose -// them; the real tree must not contain any. Ids with no description sibling -// (nested payload keys, descriptionless tools) are out of the emitted set by -// design and the extractor warn-skips them. +// literal with a static `id` (string literal or no-substitution template +// literal; `as const` / `satisfies` / parens unwrapped) whose `description` is +// a plain string / no-substitution template literal written AFTER the id. +// `.text` is the COOKED value (escapes resolved), so it doubles as an oracle +// for the extractor's unescaping. +// +// Everything else an id-bearing object can do with `description` lands in a +// LOUD bucket instead of silently escaping the safety net: +// - nonStatic: the rendered text cannot be read statically - a non-literal +// initializer (concatenation, interpolation, method call, const reference, +// `as const`), a shorthand / getter / method `description` member, a +// dynamic computed key, or a spread sibling that may carry or override the +// description at runtime. +// - misordered: a plain-literal description written BEFORE the id - the +// extractor's scan is forward-only and cannot capture it. +// The real tree must keep both buckets empty. Ids with no description-ish +// member at all (nested payload keys, descriptionless tools) stay out of the +// emitted set by design (the extractor warn-skips them), and dynamic ids +// (const references, interpolated templates) are out of scope on both sides. +function unwrapExpression(node) { + while ( + ts.isAsExpression(node) || + ts.isSatisfiesExpression(node) || + ts.isParenthesizedExpression(node) + ) { + node = node.expression; + } + return node; +} + +function isStaticText(node) { + return ts.isStringLiteral(node) || ts.isNoSubstitutionTemplateLiteral(node); +} + +// Property/member name as static text, or null when it can't be known +// statically (a dynamic computed key). `["description"]: ...` resolves to +// "description" so a computed spelling can't slip past the net. +function memberName(prop) { + if (!prop.name) return null; + if (ts.isIdentifier(prop.name) || ts.isStringLiteral(prop.name)) return prop.name.text; + if (ts.isComputedPropertyName(prop.name)) { + const expr = unwrapExpression(prop.name.expression); + return isStaticText(expr) ? expr.text : null; + } + return null; +} + function oracleFromSource(src, fileName = "oracle.ts") { const sourceFile = ts.createSourceFile(fileName, src, ts.ScriptTarget.Latest, true); const tools = []; const nonStatic = []; + const misordered = []; const visit = (node) => { if (ts.isObjectLiteralExpression(node)) { let id = null; - let descNode; - for (const prop of node.properties) { - if (!ts.isPropertyAssignment(prop)) continue; - const name = - ts.isIdentifier(prop.name) || ts.isStringLiteral(prop.name) ? prop.name.text : null; - if (name === "id" && ts.isStringLiteral(prop.initializer)) id = prop.initializer.text; - if (name === "description") descNode = prop.initializer; - } - if (id !== null && descNode !== undefined) { - if (ts.isStringLiteral(descNode) || ts.isNoSubstitutionTemplateLiteral(descNode)) { - tools.push({ name: id, description: descNode.text.trim() }); + let idIndex = -1; + let descIndex = -1; + let descValue = null; + let descIsPlain = false; + let hasDescMember = false; + let hasSpread = false; + node.properties.forEach((prop, index) => { + if (ts.isSpreadAssignment(prop)) { + hasSpread = true; + return; + } + const name = memberName(prop); + if (name === "id" && ts.isPropertyAssignment(prop)) { + const init = unwrapExpression(prop.initializer); + if (isStaticText(init)) { + id = init.text; + idIndex = index; + } + } + if (name === "description") { + hasDescMember = true; + descIndex = index; + // Only a direct PropertyAssignment to a plain literal is readable; + // deliberately NOT unwrapped - `description: "x" as const` is not a + // shape the extractor can capture, so it must stay non-static. + if (ts.isPropertyAssignment(prop) && isStaticText(prop.initializer)) { + descIsPlain = true; + descValue = prop.initializer.text.trim(); + } + } + }); + if (id !== null && hasDescMember) { + if (descIsPlain && descIndex >= idIndex) { + tools.push({ name: id, description: descValue }); + } else if (descIsPlain) { + misordered.push({ id, file: fileName }); } else { - nonStatic.push(id); + nonStatic.push({ id, file: fileName }); } + if (hasSpread && descIsPlain) { + // A spread sibling may override the plain literal at runtime. + nonStatic.push({ id, file: fileName }); + } + } else if (id !== null && hasSpread) { + // No explicit description, but the spread may carry one the extractor + // (and this oracle) can't see - force it to be written explicitly. + nonStatic.push({ id, file: fileName }); } } ts.forEachChild(node, visit); }; visit(sourceFile); - return { tools, nonStatic }; + return { tools, nonStatic, misordered }; } function oracleFromTree() { const tools = []; const nonStatic = []; + const misordered = []; const walk = (dir) => { for (const name of readdirSync(dir)) { const full = join(dir, name); @@ -104,13 +178,16 @@ function oracleFromTree() { const fromFile = oracleFromSource(readFileSync(full, "utf8"), full); tools.push(...fromFile.tools); nonStatic.push(...fromFile.nonStatic); + misordered.push(...fromFile.misordered); } } }; walk(toolsRoot); - return { tools, nonStatic }; + return { tools, nonStatic, misordered }; } +const flagList = (entries) => entries.map((e) => `${e.id} (${e.file})`).join(", "); + const byName = (a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0); // --- Real-tree ground-truth tests ------------------------------------------- @@ -157,9 +234,17 @@ test("every real tool description is a plain literal (statically extractable, so assert.deepEqual( oracle.nonStatic, [], - `these tools' descriptions are not a single string/template literal, so the ` + - `extractor cannot capture them and they would drop out of the spidershield ` + - `scan: ${oracle.nonStatic.join(", ")}. Make each a plain literal.` + `these tools' descriptions cannot be read statically (non-literal value, ` + + `shorthand/getter/computed member, or a spread that may carry/override it), ` + + `so they would drop out of the spidershield scan: ${flagList(oracle.nonStatic)}. ` + + `Write each as a plain string/template literal property.` + ); + assert.deepEqual( + oracle.misordered, + [], + `these descriptions are written BEFORE their id: - the extractor's scan is ` + + `forward-only, so they would drop out of the spidershield scan: ` + + `${flagList(oracle.misordered)}. Move each description: after its id:.` ); }); @@ -572,3 +657,265 @@ test("duplicate tool ids keep the first occurrence and warn about the rest", () `expected a duplicate-id WARNING naming dup-tool; got: ${JSON.stringify(warnings)}` ); }); + +// --- Lexer soundness: regex positions, templates, id forms ------------------ + +test("a keyword-position regex (return /[\"']/) before a tool does not swallow it", () => { + // `/` after `return` is a regex, not division; without keyword awareness the + // quotes inside it open a fake string and every later tool in the file is + // silently dropped. Helper functions with return-regexes share files with + // tool definitions in the real tree today. + const src = ` + function isQuote(c) { return /["']/.test(c); } + export const t = defineTool({ + id: "after-keyword-regex-tool", + description: "Real description after a keyword-position regex.", + handler: async () => {}, + }); + `; + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.equal( + tools.find((t) => t.name === "after-keyword-regex-tool")?.description, + "Real description after a keyword-position regex." + ); + assert.deepEqual(warnings, []); +}); + +test("a nested template literal before a tool is skipped whole (interpolation included)", () => { + // Without `\${...}` tracking, the inner template's opening backtick "closes" + // the outer one, the apostrophe in it opens a fake string, and the following + // tool silently vanishes. Nested templates are common in the real tree. + const src = ` + const msg = \`status: \${ready ? \`it's ready\` : "pending"}\`; + export const t = defineTool({ + id: "after-nested-template-tool", + description: "Real description after a nested template.", + handler: async () => {}, + }); + `; + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.equal( + tools.find((t) => t.name === "after-nested-template-tool")?.description, + "Real description after a nested template." + ); + assert.deepEqual(warnings, []); +}); + +test("a fake description: inside a template interpolation cannot be stolen as the tool's", () => { + // The interpolation's content is not code at the tool's object level; a + // `description: "..."` string inside it must not be matched ahead of the + // tool's real description. + const src = ` + export const t = defineTool({ + id: "victim-tool", + note: \`\${\`description: "stolen text",\`} tail\`, + description: "The real description of victim-tool.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "victim-tool")?.description, + "The real description of victim-tool." + ); + assert.equal(tools.length, 1); +}); + +test("a no-substitution template-literal id is captured, and the AST oracle agrees", () => { + // `id: \`x\`` is runtime-identical to `id: "x"`. Both the extractor and the + // oracle must see it - if either side skipped it, a real tool would leave + // the scan with the equality test still green (the one shape that used to + // defeat the whole safety net). + const src = + 'export const t = defineTool({ id: `template-id-tool`, description: "Real description T.", handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "template-id-tool")?.description, + "Real description T." + ); + assert.deepEqual(oracleFromSource(src).tools, [ + { name: "template-id-tool", description: "Real description T." }, + ]); +}); + +test("an interpolated template id is out of scope on both sides (dynamic, like a const-ref id)", () => { + const src = 'export const t = defineTool({ id: `dyn-${x}`, description: "D.", handler() {} });'; + const { result: tools } = captureWarnings(() => extractToolsFromSource(src, "fixture.ts")); + const oracle = oracleFromSource(src); + assert.deepEqual(tools, []); + assert.deepEqual(oracle.tools, []); + assert.deepEqual(oracle.nonStatic, []); +}); + +test('an `id: "x" as const` tool is captured, and the AST oracle agrees', () => { + // The extractor's id matcher reads through the suffix; the oracle unwraps + // as/satisfies/parens. Without the unwrap this correct extraction would be + // reported as a fabrication. + const src = + 'export const t = defineTool({ id: "as-const-tool" as const, description: "Real.", handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal(tools.find((t) => t.name === "as-const-tool")?.description, "Real."); + assert.deepEqual(oracleFromSource(src).tools, [{ name: "as-const-tool", description: "Real." }]); +}); + +// --- Cooked-text fidelity ---------------------------------------------------- + +test("full JS escape semantics: \\xNN, \\uNNNN, \\u{...}, identity escapes, line continuations", () => { + const src = ` + export const t = defineTool({ + id: "escape-suite-tool", + description: "AB: \\x41 \\u0042 \\u{1F600}|\\v|\\b|\\f| \\qidentity and a \\\ncontinuation", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "escape-suite-tool")?.description, + "AB: A B \u{1F600}|\v|\b|\f| qidentity and a continuation" + ); +}); + +test("template-literal line terminators are cooked: CRLF becomes LF", () => { + // Per spec, `\r\n` and lone `\r` inside a template literal cook to `\n`. + // Relevant on a CRLF (autocrlf) checkout, where raw bytes would otherwise + // leak \r into the scanned text. + const src = + 'defineTool({ id: "crlf-tool", description: `line one\r\nline two\rline three`, handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "crlf-tool")?.description, + "line one\nline two\nline three" + ); +}); + +test("$ + escape + { does not fabricate interpolation ($\\d{2} stays captured)", () => { + // The interpolation probe must not glue `$` to `{` across a removed escape + // pair: `$\d{2}` renders as the literal "$d{2}" and is NOT interpolation. + const src = + 'defineTool({ id: "dollar-escape-tool", description: `pattern $\\d{2} end`, handler() {} });'; + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.equal( + tools.find((t) => t.name === "dollar-escape-tool")?.description, + "pattern $d{2} end" + ); + assert.deepEqual(warnings, []); +}); + +test("surrounding whitespace in a description literal is trimmed", () => { + const src = 'defineTool({ id: "padded-tool", description: " padded text ", handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal(tools.find((t) => t.name === "padded-tool")?.description, "padded text"); +}); + +// --- Search-scope rules ------------------------------------------------------- + +test(">2000 chars of properties between id: and description: are scanned through", () => { + // The other axis of the fixed-window class: not the description LENGTH but + // the id->description search DISTANCE must be unbounded too. + const filler = Array.from({ length: 60 }, (_, k) => ` prop${k}: "${"x".repeat(40)}",`).join( + "\n" + ); + const src = ` + export const t = defineTool({ + id: "far-description-tool", +${filler} + description: "Found beyond 2000 chars of siblings.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "far-description-tool")?.description, + "Found beyond 2000 chars of siblings." + ); +}); + +test("a nested description: key between id: and the real description is not matched", () => { + // The depth anchor must hold: a `description:` inside a nested value (e.g. a + // schema fragment) is not the tool's description. + const src = ` + export const t = defineTool({ + id: "outer-tool", + schema: { description: "inner schema description, not the tool's" }, + description: "The real outer description.", + handler: async () => {}, + }); + `; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal( + tools.find((t) => t.name === "outer-tool")?.description, + "The real outer description." + ); + assert.equal(tools.length, 1); +}); + +test("a plain description written before its id: is skipped loudly and flagged by the oracle", () => { + // The extractor scans forward from the id, so it cannot capture this shape; + // it must warn, and the oracle's `misordered` bucket must name it so the + // real-tree test forces the property order to be fixed. + const src = + 'defineTool({ description: "Written above the id.", id: "desc-first-tool", handler() {} });'; + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.deepEqual(tools, []); + assert.ok( + warnings.some((w) => /WARNING/.test(w) && w.includes("desc-first-tool")), + `expected a stderr WARNING naming desc-first-tool; got: ${JSON.stringify(warnings)}` + ); + assert.deepEqual( + oracleFromSource(src).misordered.map((e) => e.id), + ["desc-first-tool"] + ); +}); + +// --- Oracle safety net for member kinds -------------------------------------- + +test("the oracle flags description members it cannot read statically (shorthand, getter, computed, spread)", () => { + // These shapes are invisible to the lexical extractor; each must land in the + // nonStatic bucket so the real-tree test turns red instead of the tool + // silently leaving the scan with CI green. + for (const [label, src] of [ + ["shorthand", 'defineTool({ id: "sh-tool", description, handler() {} });'], + ["getter", 'defineTool({ id: "get-tool", get description() { return "x"; }, handler() {} });'], + ["method", 'defineTool({ id: "m-tool", description() { return "x"; }, handler() {} });'], + [ + "computed key with non-literal value", + 'defineTool({ id: "computed-tool", ["description"]: makeDesc(), handler() {} });', + ], + [ + "spread with no explicit description", + 'defineTool({ id: "spread-tool", ...common, handler() {} });', + ], + [ + "spread beside a plain description", + 'defineTool({ id: "spread2-tool", ...common, description: "explicit", handler() {} });', + ], + ]) { + const oracle = oracleFromSource(src); + assert.ok( + oracle.nonStatic.length >= 1, + `${label}: expected the oracle to flag the shape as non-static` + ); + } +}); + +test('a computed ["description"] key with a literal value surfaces as an extractor-vs-oracle mismatch', () => { + // The oracle can read `["description"]: "x"` (it IS the runtime description) + // but the lexical extractor cannot see the `description:` token. The oracle + // must claim the tool so the real-tree equality test goes red and forces a + // plain key - never a silent green. + const src = + 'defineTool({ id: "computed-desc-tool", ["description"]: "Real text.", handler() {} });'; + const { result: tools } = captureWarnings(() => extractToolsFromSource(src, "fixture.ts")); + assert.deepEqual(tools, []); + assert.deepEqual(oracleFromSource(src).tools, [ + { name: "computed-desc-tool", description: "Real text." }, + ]); +}); From 3b5108e0f6d2a5bb735defefec0876a3e52ba1ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Fri, 10 Jul 2026 05:07:30 +0200 Subject: [PATCH 15/16] fix(scripts): use a plain-space placeholder in the interpolation probe The escape-pair placeholder was a raw NUL byte, which made grep/file treat extract-tools.mjs as binary. A space has the same no-glue property. --- scripts/extract-tools.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index ba1886b34..bb62e927b 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -503,7 +503,7 @@ export function extractToolsFromSource(src, filePath = "") { // `${`, and an escaped `\${` or `$\{` stays the literal text it renders as. const rest = value.slice(valueMatch[0].length); const hasInterpolation = - delim === "`" && /\$\{/.test(valueMatch[1].replace(/\\[\s\S]/g, "")); + delim === "`" && /\$\{/.test(valueMatch[1].replace(/\\[\s\S]/g, " ")); if (literalIsWholeValue(rest) && !hasInterpolation) { // Render the runtime (cooked) text: template literals first normalize // line terminators (`\r\n` / `\r` cook to `\n` per spec - relevant on From 160aa7455596ec28ccff7c410bfb2d8453a960c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Fri, 10 Jul 2026 05:23:33 +0200 Subject: [PATCH 16/16] fix(scripts): division after keyword-named members/postfix, cooked ids, LS/PS continuations; scope oracle policy to tool sites Extractor: - 'counts.in / 2' and 'i++ / 2' end in values; the keyword walk-back and the bare +/- in REGEX_PREV_CHARS read the following '/' as a regex opener, and with a second slash on the same line the fake regex desynchronized string lexing - a following tool could be silently dropped or have description text stolen from a string. Property-access and postfix checks close both. - ids are cooked like any literal so an escape in an id emits the runtime tool name to the scan, matching the AST oracle. - backslash + U+2028/U+2029 are line continuations per spec; they now cook to nothing like backslash-newline. Oracle: - policy buckets (nonStatic/misordered) now apply only to objects that could be tool definitions; an object reached as another object literal's property value is data, so an example payload can no longer produce a false red demanding reordering or a static description. Equality mirroring is unchanged. - a spread BEFORE an explicit description cannot override it at runtime and is no longer flagged; a spread after it, or one with no explicit description, still is. - a getter/method id beside a description was silent on every channel (a runtime-real tool bypassing the scan with green CI); it now lands in nonStatic. Fixtures cover each: member/postfix division, defaults-spread, nested data objects, getter/method ids, escaped ids, LS/PS continuations. --- scripts/extract-tools.mjs | 21 +++- scripts/extract-tools.test.mjs | 189 ++++++++++++++++++++++++++++----- 2 files changed, 183 insertions(+), 27 deletions(-) diff --git a/scripts/extract-tools.mjs b/scripts/extract-tools.mjs index bb62e927b..4d88c6aea 100644 --- a/scripts/extract-tools.mjs +++ b/scripts/extract-tools.mjs @@ -189,11 +189,21 @@ function skipInterpolation(src, i) { */ function isRegexPosition(src, prevSig, prevSigIdx) { if (prevSig === null) return true; + if (prevSig === "+" || prevSig === "-") { + // Postfix `++`/`--` ends a value, so a following `/` is division; a lone + // `+`/`-` is a binary/unary operator, after which `/` starts a regex. + return src[prevSigIdx - 1] !== prevSig; + } if (REGEX_PREV_CHARS.has(prevSig)) return true; if (/[A-Za-z0-9_$]/.test(prevSig)) { let start = prevSigIdx; while (start > 0 && /[A-Za-z0-9_$]/.test(src[start - 1])) start--; - return REGEX_PREV_KEYWORDS.has(src.slice(start, prevSigIdx + 1)); + if (!REGEX_PREV_KEYWORDS.has(src.slice(start, prevSigIdx + 1))) return false; + // A keyword-NAMED property access (`counts.in`, `obj?.new`) is a value, + // not a keyword: the `/` after it is division. + let before = start - 1; + while (before >= 0 && /\s/.test(src[before])) before--; + return src[before] !== "."; } return false; } @@ -254,7 +264,9 @@ function unescapeJsString(raw) { } if (u4 !== undefined) return String.fromCharCode(parseInt(u4, 16)); if (x2 !== undefined) return String.fromCharCode(parseInt(x2, 16)); - if (single === "\r\n" || single === "\r" || single === "\n") return ""; // line continuation + // Line continuations: backslash + any LineTerminatorSequence (LF, CRLF, + // CR, U+2028, U+2029) vanishes. + if (single === "\r\n" || /^[\n\r\u2028\u2029]$/.test(single)) return ""; return UNESCAPE_MAP[single] ?? single; } ); @@ -404,7 +416,10 @@ function findIdLiteralsInCode(src) { // A template id containing `${` is interpolated - dynamic, not a static // tool id (same class as a const-reference id; out of scope by design). if (!(m[1] === "`" && m[2].includes("${"))) { - out.push({ id: m[2], end: ID_LITERAL.lastIndex }); + // Cook the id like any literal - an id written "esc\u002Dtool" + // names the tool "esc-tool" at runtime, and the scan must see the + // runtime name. + out.push({ id: unescapeJsString(m[2]), end: ID_LITERAL.lastIndex }); } i = ID_LITERAL.lastIndex - 1; // resume just past the value (loop's i++ advances) prevSig = m[1]; // the id literal's closing delimiter diff --git a/scripts/extract-tools.test.mjs b/scripts/extract-tools.test.mjs index 80bcb1d70..4fbcdce72 100644 --- a/scripts/extract-tools.test.mjs +++ b/scripts/extract-tools.test.mjs @@ -66,14 +66,24 @@ function captureWarnings(fn) { // - nonStatic: the rendered text cannot be read statically - a non-literal // initializer (concatenation, interpolation, method call, const reference, // `as const`), a shorthand / getter / method `description` member, a -// dynamic computed key, or a spread sibling that may carry or override the -// description at runtime. +// dynamic computed key, a spread written AFTER the description (it may +// override it at runtime; one before it cannot), a spread with no explicit +// description at all (it may carry one), or a getter/method `id` beside a +// description. // - misordered: a plain-literal description written BEFORE the id - the // extractor's scan is forward-only and cannot capture it. -// The real tree must keep both buckets empty. Ids with no description-ish -// member at all (nested payload keys, descriptionless tools) stay out of the -// emitted set by design (the extractor warn-skips them), and dynamic ids -// (const references, interpolated templates) are out of scope on both sides. +// The real tree must keep both buckets empty. +// +// The policy buckets apply only to objects that could BE tool definitions: an +// object literal reached as (part of) another object literal's property value +// is data (a payload, example, or config fragment - no real tool is written +// that way), so demanding "make your description static" of it would be a +// false red. Equality mirroring still covers nested data: an id-then-plain- +// description data object is emitted by both sides alike. Ids with no +// description-ish member at all (nested payload keys, descriptionless tools) +// stay out of the emitted set by design (the extractor warn-skips them), and +// dynamic ids (const references, shorthand, interpolated templates) are out of +// scope on both sides. function unwrapExpression(node) { while ( ts.isAsExpression(node) || @@ -102,6 +112,23 @@ function memberName(prop) { return null; } +// True when the object literal is (part of) another object literal's property +// value - possibly through array elements or as/satisfies/paren wrappers. Such +// an object is data, not a tool-definition site. +function isNestedDataValue(node) { + let cur = node.parent; + while ( + cur && + (ts.isArrayLiteralExpression(cur) || + ts.isAsExpression(cur) || + ts.isSatisfiesExpression(cur) || + ts.isParenthesizedExpression(cur)) + ) { + cur = cur.parent; + } + return cur !== undefined && ts.isPropertyAssignment(cur); +} + function oracleFromSource(src, fileName = "oracle.ts") { const sourceFile = ts.createSourceFile(fileName, src, ts.ScriptTarget.Latest, true); const tools = []; @@ -111,22 +138,29 @@ function oracleFromSource(src, fileName = "oracle.ts") { if (ts.isObjectLiteralExpression(node)) { let id = null; let idIndex = -1; + let hasDynamicIdMember = false; let descIndex = -1; let descValue = null; let descIsPlain = false; let hasDescMember = false; - let hasSpread = false; + let lastSpreadIndex = -1; node.properties.forEach((prop, index) => { if (ts.isSpreadAssignment(prop)) { - hasSpread = true; + lastSpreadIndex = index; return; } const name = memberName(prop); - if (name === "id" && ts.isPropertyAssignment(prop)) { - const init = unwrapExpression(prop.initializer); - if (isStaticText(init)) { - id = init.text; - idIndex = index; + if (name === "id") { + if (ts.isPropertyAssignment(prop)) { + const init = unwrapExpression(prop.initializer); + if (isStaticText(init)) { + id = init.text; + idIndex = index; + } + } else if (ts.isGetAccessorDeclaration(prop) || ts.isMethodDeclaration(prop)) { + // A computed id the lexical extractor can never see; beside a + // description it would be a fully silent scan bypass. + hasDynamicIdMember = true; } } if (name === "description") { @@ -141,22 +175,28 @@ function oracleFromSource(src, fileName = "oracle.ts") { } } }); + const nested = isNestedDataValue(node); if (id !== null && hasDescMember) { if (descIsPlain && descIndex >= idIndex) { + // Mirrors the extractor, nested data included. tools.push({ name: id, description: descValue }); - } else if (descIsPlain) { - misordered.push({ id, file: fileName }); - } else { - nonStatic.push({ id, file: fileName }); - } - if (hasSpread && descIsPlain) { - // A spread sibling may override the plain literal at runtime. - nonStatic.push({ id, file: fileName }); + if (!nested && lastSpreadIndex > descIndex) { + // Only a spread AFTER the description can override it at runtime. + nonStatic.push({ id, file: fileName }); + } + } else if (!nested) { + if (descIsPlain) { + misordered.push({ id, file: fileName }); + } else { + nonStatic.push({ id, file: fileName }); + } } - } else if (id !== null && hasSpread) { + } else if (!nested && id !== null && lastSpreadIndex >= 0) { // No explicit description, but the spread may carry one the extractor // (and this oracle) can't see - force it to be written explicitly. nonStatic.push({ id, file: fileName }); + } else if (!nested && id === null && hasDynamicIdMember && hasDescMember) { + nonStatic.push({ id: "", file: fileName }); } } ts.forEachChild(node, visit); @@ -683,6 +723,30 @@ test("a keyword-position regex (return /[\"']/) before a tool does not swallow i assert.deepEqual(warnings, []); }); +test("a keyword-NAMED property access before a slash is division, not a regex opener", () => { + // `counts.in / 2` and `i++ / 2` end in values; without the property-access + // and postfix checks the following `/` opened a fake regex that, combined + // with a second slash on the same line, desynchronized string lexing and + // could silently steal description text. + const src = ` + export const t = defineTool({ + id: "division-after-member-tool", + ratio: counts.in / total, path: "per/sec", + tally: i++ / 2, unit: "x/y", + description: "Real description after member and postfix division.", + handler: async () => {}, + }); + `; + const { result: tools, warnings } = captureWarnings(() => + extractToolsFromSource(src, "fixture.ts") + ); + assert.equal( + tools.find((t) => t.name === "division-after-member-tool")?.description, + "Real description after member and postfix division." + ); + assert.deepEqual(warnings, []); +}); + test("a nested template literal before a tool is skipped whole (interpolation included)", () => { // Without `\${...}` tracking, the inner template's opening backtick "closes" // the outer one, the apostrophe in it opens a fake string, and the following @@ -894,8 +958,8 @@ test("the oracle flags description members it cannot read statically (shorthand, 'defineTool({ id: "spread-tool", ...common, handler() {} });', ], [ - "spread beside a plain description", - 'defineTool({ id: "spread2-tool", ...common, description: "explicit", handler() {} });', + "spread AFTER a plain description (may override it)", + 'defineTool({ id: "spread2-tool", description: "explicit", ...common, handler() {} });', ], ]) { const oracle = oracleFromSource(src); @@ -906,6 +970,83 @@ test("the oracle flags description members it cannot read statically (shorthand, } }); +test("a defaults-spread BEFORE the description is fine (it cannot override a later property)", () => { + // `{ ...commonDefaults, id, description }` is idiomatic; at runtime a + // preceding spread never overrides a later explicit property, so the tool is + // fully extractable and must NOT be flagged. + const src = + 'defineTool({ ...commonDefaults, id: "defaults-tool", description: "Explicit and final.", handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal(tools.find((t) => t.name === "defaults-tool")?.description, "Explicit and final."); + const oracle = oracleFromSource(src); + assert.deepEqual(oracle.nonStatic, [], "a preceding spread must not be flagged"); + assert.deepEqual(oracle.tools, [{ name: "defaults-tool", description: "Explicit and final." }]); +}); + +test("nested data objects never trigger the policy buckets", () => { + // An object that is another object's property value is data (a payload, + // example, defaults fragment), not a tool definition - the policy tests must + // not demand anything of it, whatever its shape. + const src = ` + export const host = defineTool({ + id: "host-tool", + examplePayload: { description: "sample item", id: "item-1" }, + defaults: { id: "default-profile", ...baseProfile }, + description: "Host tool description.", + handler: async () => {}, + }); + `; + const oracle = oracleFromSource(src); + assert.deepEqual(oracle.misordered, [], "a nested example payload must not demand reordering"); + assert.deepEqual(oracle.nonStatic, [], "a nested defaults fragment must not be flagged"); + assert.deepEqual(oracle.tools, [{ name: "host-tool", description: "Host tool description." }]); + // The extractor agrees: only the host tool is emitted. + const { result: tools } = captureWarnings(() => extractToolsFromSource(src, "fixture.ts")); + assert.deepEqual(tools, [{ name: "host-tool", description: "Host tool description." }]); +}); + +test("a getter or method id beside a description is flagged, never silently green", () => { + // The lexical extractor can never see a computed id, and such an object IS a + // runtime-real tool - without this flag it would bypass the scan with zero + // signal on any channel. + for (const [label, src] of [ + [ + "getter id", + 'defineTool({ get id() { return "getter-id-tool"; }, description: "Real.", handler() {} });', + ], + [ + "method id", + 'defineTool({ id() { return "method-id-tool"; }, description: "Real.", handler() {} });', + ], + ]) { + const oracle = oracleFromSource(src); + assert.equal(oracle.nonStatic.length, 1, `${label}: expected a nonStatic flag`); + } +}); + +test("an escaped id literal is cooked to its runtime name on both sides", () => { + // An id written with an escape (\\u002D is "-") names the tool by its COOKED + // text at runtime; the scan must see that name, and the oracle and extractor + // must agree on it. + const src = 'defineTool({ id: "esc\\u002Dtool", description: "Escaped id.", handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal(tools.find((t) => t.name === "esc-tool")?.description, "Escaped id."); + assert.deepEqual(oracleFromSource(src).tools, [{ name: "esc-tool", description: "Escaped id." }]); +}); + +test("U+2028/U+2029 line continuations cook to nothing, like backslash-newline", () => { + // Built programmatically so no raw LS/PS bytes live in this file: the + // fixture string contains backslash + U+2028 (and + U+2029), each a legal + // LineTerminatorSequence continuation that cooks to the empty string. + const src = + 'defineTool({ id: "ls-tool", description: "before \\\u2028after \\\u2029end", handler() {} });'; + const tools = extractToolsFromSource(src, "fixture.ts"); + assert.equal(tools.find((t) => t.name === "ls-tool")?.description, "before after end"); + assert.deepEqual(oracleFromSource(src).tools, [ + { name: "ls-tool", description: "before after end" }, + ]); +}); + test('a computed ["description"] key with a literal value surfaces as an extractor-vs-oracle mismatch', () => { // The oracle can read `["description"]: "x"` (it IS the runtime description) // but the lexical extractor cannot see the `description:` token. The oracle