fix(scripts): stop extract-tools dropping tools with long descriptions#436
Open
latekvo wants to merge 17 commits into
Open
fix(scripts): stop extract-tools dropping tools with long descriptions#436latekvo wants to merge 17 commits into
latekvo wants to merge 17 commits into
Conversation
…ription
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).
…r form 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).
…slash 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.
Resolve test:scripts conflict by running both new script test files (extract-tools.test.mjs and next-canary-version.test.mjs).
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.
hubgan
reviewed
Jul 2, 2026
hubgan
left a comment
hubgan
left a comment
Collaborator
There was a problem hiding this comment.
A few inline notes below.
… 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).
…overage
- 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).
hubgan
reviewed
Jul 3, 2026
hubgan
left a comment
hubgan
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the extractor rewrite end-to-end against the real tool tree, not only the suite: it does what it claims — recovers run-sequence and describe (70 to 72) with byte-exact descriptions, drops nothing, and correctly renders flow-add-step's embedded newline. Tests, typecheck, and lint all pass locally. None of the inline comments are blocking; most are latent or pre-existing.
Two items are out of scope for this PR and better handled as follow-ups: (1) tool ids defined via a const reference — e.g. await-ui-element's id: AWAIT_UI_ELEMENT_TOOL_ID — stay invisible to the scan (already acknowledged in the description); a follow-up could cover const-ref ids and add a real-tree emit-count and empty-stderr check so a future silent regression trips CI. (2) The extractor also emits two defs that are not registered as live MCP tools, paste and simulator-server — pre-existing and harmless to the scan, not introduced here.
…uffixes
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.
… 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.
…ns, 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.
…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.
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.
…s, 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
scripts/extract-tools.mjsextracts MCP tool definitions by regex to feed the downstreamspidershield scan --tools-jsonsecurity scan. For eachid:match it scanned only the next 2000 chars (const descWindow = afterId.slice(0, 2000)) for thedescription:value, and the description regexes require the string's closing delimiter to fall inside that window.Any tool whose description's closing delimiter sits past ~2000 chars is therefore matched as
nulland silently dropped - never emitted, never security-scanned. Two shipping tools hit this:run-sequence(packages/tool-server/src/tools/run-sequence/index.ts) - a ~4285-char multi-line template-literal description whose closing backtick sits ~4324 chars past itsid:.describe(packages/tool-server/src/tools/describe/index.ts) - a ~2195-char description whose closing backtick sits ~2252 chars past itsid:.Security impact:
describeis the accessibility / DOM tree tool, exactly the kind of tool aspidershieldscan should cover, and it was being silently excluded alongsiderun-sequence. A dropped tool is not a scan finding, so the gap was invisible.Repro (before)
After
Fix
description:is resolved at the same object-literal brace level as itsid:by a forward scan that tracks brace depth over real code only: comments, string literals, template literals (with${...}interpolations skipped opaquely, recursively), and regex literals (prev-token heuristic incl. regex-permitting keywords likereturn) are skipped whole, so nothing inside them can open a fake string, shift the depth, or match as adescription:/id:token. The value is then parsed to its actual closing delimiter, so a description of any length is captured in full.id:/description:token can't corrupt extraction: a description-less tool can't borrow the next tool's description, anid:appearing inside description text or a comment can't truncate or overwrite anything, and anid:key nested inside a value object (e.g.defaultPayload: { id: "example" }) is neither emitted as a spurious tool nor allowed to drop the real one.\xNN,\uNNNN,\u{...}, identity escapes, line continuations, and template\r\n/\r->\nnormalization - the emitted (and security-scanned) text is the string the runtime renders.${...}interpolation, a method/operator suffix ("...".slice(0, 3)),as const- is never emitted as a truncated/wrong literal: it warn-skips on stderr (stdout stays valid JSON for the scanner). Non-interpolated template-literal ids (id: `x`) are extracted; interpolated ids stay out of scope like const-reference ids.npm ci.CI guard
Adds
scripts/extract-tools.test.mjs(node's built-in test runner, wired intonpm run test:scripts; 36 tests). Ground truth: the real-tree tests parse every tool source file with the actual TypeScript parser (a root devDependency, test-only) and require the extractor's output to equal the AST exactly - names and descriptions byte-for-byte. Any drop, corruption, or fabrication on a shape that enters the tree fails CI loudly, with no reliance on stderr warning formats and no false red fromid:-like text in comments or strings.Two policy gates keep un-extractable shapes out of the tree instead of letting them silently leave the scan: a
descriptionthat cannot be read statically (non-literal value, shorthand/getter/method/computed member, or a spread sibling that may carry/override it) and a plain-literal description written before itsid:(the scan is forward-only) each fail with an actionable, file-naming message.Fixture tests pin the parsing rules independent of any real tool's wording - long descriptions (>4000-char value, >2000 chars of siblings between
id:anddescription:), embeddedid:/description:tokens, comments, keyword-position and sibling regex literals, division, nested templates and interpolation content, template/as constids, the full escape suite, CRLF cooking, single-quoted forms, trim, warn-skip classes, and the duplicate-id warning. Each new fixture was verified to fail under a targeted mutation of the extractor.Note:
await-ui-elementis registered but uses a const-reference id (id: AWAIT_UI_ELEMENT_TOOL_ID) rather than a static literal, so it stays out of scope for this extractor - a separate, pre-existing limitation unrelated to the description window.Verification
node scripts/extract-tools.mjs-> 72 tools,run-sequenceanddescribeboth present (total 70 -> 72), byte-identical to the TypeScript-AST parse of the tree, no stderr warningsnpm run test:scripts-> all pass (extract-tools.test.mjs: 36 tests)npm run typecheck:scripts-> cleanprettier+eslinton changed files -> 0 problemsnpm install --package-lock-only --ignore-scripts-> no diff (idempotent)