Ignacy Łątka 78d3184ba8 fix(scripts): stop extract-tools dropping tools with long descriptions (#436) ...

## Defect

`scripts/extract-tools.mjs` extracts MCP tool definitions by regex to
feed the downstream `spidershield scan --tools-json` security scan. For
each `id:` match it scanned only the next 2000 chars (`const descWindow
= afterId.slice(0, 2000)`) for the `description:` 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 `null` and **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 its `id:`.
- `describe` (`packages/tool-server/src/tools/describe/index.ts`) - a
~2195-char description whose closing backtick sits ~2252 chars past its
`id:`.

**Security impact:** `describe` is the accessibility / DOM tree tool,
exactly the kind of tool a `spidershield` scan should cover, and it was
being silently excluded alongside `run-sequence`. A dropped tool is not
a scan finding, so the gap was invisible.

### Repro (before)

```
$ node scripts/extract-tools.mjs | grep -c '"run-sequence"'
0
$ node scripts/extract-tools.mjs | grep -c '"describe"'
0
$ node scripts/extract-tools.mjs | grep -c '"gesture-tap"'
1
# total tools emitted: 70  (run-sequence AND describe both missing)
```

### After

```
$ node scripts/extract-tools.mjs | grep -c '"run-sequence"'
1
$ node scripts/extract-tools.mjs | grep -c '"describe"'
1
# total tools emitted: 72  (both captured, run-sequence's full 4285-char description intact)
```

## Fix

- Drop the fixed 2000-char lookahead. Each tool's `description:` is
resolved at the **same object-literal brace level** as its `id:` 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 like `return`) are skipped whole, so nothing
inside them can open a fake string, shift the depth, or match as a
`description:`/`id:` token. The value is then parsed to its actual
closing delimiter, so a description of any length is captured in full.
- Because matching is by brace scope rather than raw position, a stray
`id:`/`description:` token can't corrupt extraction: a description-less
tool can't borrow the next tool's description, an `id:` appearing inside
description text or a comment can't truncate or overwrite anything, and
an `id:` 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.
- Captured text follows full JS cooked-string semantics: named escapes,
`\xNN`, `\uNNNN`, `\u{...}`, identity escapes, line continuations, and
template `\r\n`/`\r` -> `\n` normalization - the emitted (and
security-scanned) text is the string the runtime renders.
- A description value that is not a single plain literal - a
concatenation, a `${...}` 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.
- Duplicate ids keep the first occurrence and warn (exported as a pure,
unit-tested function).
- The extractor stays dependency-free (node: builtins only): the
tool-description-quality workflow runs it on a bare checkout without
`npm ci`.
- Output JSON shape is unchanged.

## CI guard

Adds `scripts/extract-tools.test.mjs` (node's built-in test runner,
wired into `npm 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 from `id:`-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 `description` that 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** its `id:` (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:` and `description:`), embedded `id:`/`description:` tokens,
comments, keyword-position and sibling regex literals, division, nested
templates and interpolation content, template/`as const` ids, 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: tools registered with a const-reference / dynamic id (e.g. `id:
AWAIT_UI_ELEMENT_TOOL_ID`) rather than a static literal stay out of
scope for this extractor - a separate, pre-existing limitation unrelated
to the description window, and one this static/lexical extractor cannot
close soundly (it has no tool-scope knowledge, so a blanket warning on
every `id: <identifier>` would also fire on legitimate nested non-tool
keys like `{ id: someVar }` in a payload/example object). The full set
currently taken out of the scan this way is `app-map-crawl`,
`app-map-export`, `app-map-navigate`, `app-map-query`,
`await-screen-idle`, and `await-ui-element`. A proper fix is a follow-up
covering const-ref id resolution plus a real-tree emit-count and
empty-stderr assertion so this silent-omission class trips CI if it ever
regresses.

## Verification

- `node scripts/extract-tools.mjs` -> 72 tools, `run-sequence` and
`describe` both present (total 70 -> 72), byte-identical to the
TypeScript-AST parse of the tree, no stderr warnings
- `npm run test:scripts` -> all pass (`extract-tools.test.mjs`: 36
tests)
- mutation checks: window-bounding, depth-anchor removal, trim removal,
dedup neutralization, single-quote-branch neutralization each fail the
suite
- `npm run typecheck:scripts` -> clean
- `prettier` + `eslint` on changed files -> 0 problems
- lockfile: `npm install --package-lock-only --ignore-scripts` -> no
diff (idempotent)

2026-07-20 12:56:44 +02:00

24 KiB

Raw Permalink Blame History

View Raw