modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

chore(deps): extract specifiers with ts.preProcessFile; document the transitive boundary

Browse source 
From Copilot's sixth pass of #1962.

Take the TypeScript-parser suggestion, which was raised across three rounds
and was right. The regex scan got both directions wrong and every widening
traded one failure for the other: it *missed* valid syntax (`import x =
require(…)`, an import-attributes argument, a comment between tokens — each a
silent miss, so the package never entered the candidate set and its skew
passed), and it *invented* names from prose, since `// adapted from "react"`
is indistinguishable from an import to a pattern that cannot tell code from a
comment. The earlier prose test only passed because `cwd omitted` isn't a
valid package name.

`ts.preProcessFile` is the cheap answer I'd missed while arguing against the
full AST: TypeScript's lightweight pre-parse scanner, no parse tree and no
type checking, purpose-built to return module specifiers. It handles every
import form, trivia, strings, and regex literals, and never reads a comment as
code. typescript resolves from `clients/web`, which already carries it, via
the `createRequire` base `smoke-web-browser.mjs` established — a bare import
would resolve relative to `scripts/`, not the cwd.

The derived set drops 18 → 17 real names: every genuine package is retained
and the one prose artifact is gone. Regression tests now use *real* package
names in comments and strings, the case the old test couldn't cover.

Also records the KNOWN BOUNDARY the review's other finding identified.
Candidates cover what the shared sources name directly, so a package reaching
the program only through another package's `.d.ts` is invisible —
`@modelcontextprotocol/sdk` is the live example, skewed 1.29.0 vs 1.30.0 and
present in web's program from both installs, never written in first-party
code. Closing it properly changes what the guard measures, so it is tracked
as #1965, with both candidate derivations measured there: a lockfile closure
is unusable (155 packages, 25 skewed, nearly all irrelevant tooling, and it
misses the SDK anyway), while `tsc --listFilesOnly` is correct and small
(15 for clients/web).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
This commit is contained in:
cliffhall 2026-08-10 21:24:55 -04:00
commit fcd45037c9
2 changed files with 66 additions and 59 deletions
Download patch file Download diff file Expand all files Collapse all files

99
scripts/verify-dep-lockstep.mjs
View file

@ -27,10 +27,25 @@
// allowlist of packages verified to tolerate it (below). A dependency that // allowlist of packages verified to tolerate it (below). A dependency that
// starts skewing therefore fails `validate` and forces a decision, rather than // starts skewing therefore fails `validate` and forces a decision, rather than
// surfacing months later as an unexplained OOM. // surfacing months later as an unexplained OOM.
//
// KNOWN BOUNDARY (#1965): the candidate set covers packages the shared sources
// name *directly*. A package whose declarations reach the program only through
// another package's `.d.ts` is invisible here — `@modelcontextprotocol/sdk` is
// the live example, skewed root 1.29.0 vs `clients/web` 1.30.0 and present in
// web's program from both installs, yet never written in first-party code
// (the shared sources import the split `@modelcontextprotocol/client|core|…`).
// Two derivations were measured for closing this. A lockfile dependency
// closure is unusable — 155 packages, 25 of them skewed, nearly all irrelevant
// tooling (`chai`, `qs`, `iconv-lite`) — and it misses the SDK anyway. Reading
// what actually lands in each program (`tsc --listFilesOnly`, keeping packages
// present under two install roots) is both correct and small: 15 for
// `clients/web`, ~10 once nested duplicates are dropped. That is the right
// derivation and is tracked separately, since it changes what the guard
// measures and surfaces skews needing their own decisions.
import { readFileSync, existsSync, readdirSync } from "node:fs"; import { readFileSync, existsSync, readdirSync } from "node:fs";
import { execFileSync } from "node:child_process"; import { execFileSync } from "node:child_process";
import { builtinModules } from "node:module"; import { builtinModules, createRequire } from "node:module";
import path from "node:path"; import path from "node:path";
import { fileURLToPath, pathToFileURL } from "node:url"; import { fileURLToPath, pathToFileURL } from "node:url";
import { rootReachesScript } from "./lib/npm-scripts.mjs"; import { rootReachesScript } from "./lib/npm-scripts.mjs";
@ -116,59 +131,51 @@ export function packageNameOf(specifier) {
return name; return name;
} }
// Every form that can introduce a dependency's *types* (Copilot, #1962). // Specifiers are extracted with TypeScript's own `preProcessFile` rather than
// `import x = require("pkg")` and a bare `require("pkg")` matter because the // by regex (Copilot, #1962 — raised across three review rounds, and correctly).
// shared trees may hold `.cts` sources, where that is the ordinary import // A regex scan gets both directions wrong: it *misses* valid syntax (an
// syntax; and neither call form requires the closing paren, so an import // `import x = require(…)` in a `.cts`, an import-attributes argument, a comment
// attributes argument (`import("pkg", { with: … })`) can't hide the specifier. // between tokens — each a silent miss, so the package never enters the
// candidate set and its skew passes), and it *invents* names from prose, since
// `// adapted from "react"` is indistinguishable from an import to a pattern
// that can't tell code from a comment. Every widening of the regex traded one
// of those failures for the other.
// //
// Backticks are accepted ONLY in the call forms, where a static template // `preProcessFile` is TypeScript's lightweight pre-parse scanner — not a full
// literal is legal. A `from` clause requires a string literal, so allowing // parse and no type checking — and it is exactly built for this: it returns
// backticks there would buy nothing while reopening the prose hazard the // every module specifier, handling all import forms, trivia, strings, and
// anchored specifier pattern exists to close — inline code in a comment is // regex literals correctly, and it never sees a comment as code.
// written with backticks throughout this codebase, and "…derived from
// `tools`…" would otherwise enter the candidate set as a package named
// `tools`. Inert today, but a prose word that collides with a real package
// name would silently widen the set.
// //
// Whitespace between tokens is really *trivia*: TypeScript allows a comment // typescript is resolved from `clients/web`, which already carries it; the root
// anywhere whitespace is legal, so `import(/* webpackIgnore: true */ "pkg")`, // has no TS dependency of its own. The `createRequire` base is load-bearing —
// `from /* why */ "pkg"`, and the line-comment forms (`import(// lazy\n"pkg")`) // a bare `import("typescript")` would resolve relative to `scripts/`, not the
// are all valid and must still be seen (Copilot, #1962). `TRIVIA` stands in for // cwd (the same reason `smoke-web-browser.mjs` resolves playwright this way).
// `\s*` at every such position and covers both comment syntaxes. A line comment let tsCache;
// runs to end-of-line only — the newline itself is matched by the `\s` branch. function typescript() {
const TRIVIA = String.raw`(?:\s|/\*[\s\S]*?\*/|//[^\n]*)*`; if (!tsCache) {
const SPECIFIER_FORMS = [ const require_ = createRequire(
// import … from "x" / export … from "x" path.join(repoRoot, "clients", "web", "package.json"),
new RegExp(String.raw`\bfrom${TRIVIA}["']([^"']+)["']`, "g"), );
// side-effect import "x" tsCache = require_("typescript");
new RegExp(String.raw`\bimport${TRIVIA}["']([^"']+)["']`, "g"), }
// dynamic import("x"[, opts]) return tsCache;
new RegExp( }
String.raw`\bimport${TRIVIA}\(${TRIVIA}["'\`]([^"'\`]+)["'\`]`,
"g",
),
// import x = require("x"), require("x")
new RegExp(
String.raw`\brequire${TRIVIA}\(${TRIVIA}["'\`]([^"'\`]+)["'\`]`,
"g",
),
];
/** /**
* Every third-party package name imported by a blob of TypeScript source. * Every third-party package name imported by a blob of TypeScript source.
* Deliberately a regex scan rather than a parse: it only needs to over- rather * Over-approximating is safe (a name absent from every lockfile contributes
* than under-approximate, since a name absent from every lockfile contributes * nothing downstream — `@inspector/core` is a build-time alias, not a package,
* nothing downstream (`@inspector/core` is a build-time alias, not a package, * and drops out that way); under-approximating is not, since a missed package
* and drops out that way). * never enters the candidate set and its skew would pass silently.
*/ */
export function importedPackageNames(source) { export function importedPackageNames(source) {
const names = new Set(); const names = new Set();
for (const re of SPECIFIER_FORMS) { // (source, readImportFiles, detectJavaScriptImports) — the latter two make it
for (const m of source.matchAll(re)) { // report `require(…)` and dynamic imports as well as static ones.
const name = packageNameOf(m[1]); const { importedFiles } = typescript().preProcessFile(source, true, true);
if (name) names.add(name); for (const { fileName } of importedFiles) {
} const name = packageNameOf(fileName);
if (name) names.add(name);
} }
return names; return names;
} }

26
scripts/verify-dep-lockstep.test.mjs
View file

@ -155,26 +155,26 @@ test("importedPackageNames: the three specifier forms that introduce types", ()
]); ]);
}); });
test("importedPackageNames: prose after `from` is not an import", () => { test("importedPackageNames: prose in comments never becomes a package (Copilot, #1962)", () => {
// The scan over-approximates on purpose, but must not invent package names // The regex scan this replaced could not tell code from a comment, so
// out of comment text — a bogus name absent from every lockfile is inert, // `// adapted from "react"` added `react` to the candidate set — and if that
// yet a *plausible* one would silently widen the candidate set. // installed package were skewed, an unrelated comment would fail `validate`.
// These use REAL package names, which is the case the old prose test missed:
// it only passed because `cwd omitted` isn't a valid package name.
const source = ` const source = `
// Resolved relative to the runner dir, not from "cwd omitted" by the caller. // adapted from "react"
/** Reads the manifest from "file:" URLs. */ /** Mirrors the behavior of "express", see require("yaml") below. */
/** The excluded set derived from \\\`hono\\\`-style paths. */
// const disabled = await import("undici");
import { z } from "zod"; import { z } from "zod";
`; `;
assert.deepEqual([...importedPackageNames(source)], ["zod"]); assert.deepEqual([...importedPackageNames(source)], ["zod"]);
}); });
test("importedPackageNames: a backticked `from` in prose is not an import", () => { test("importedPackageNames: a specifier inside a string literal is not an import", () => {
// Backticks are legal in the *call* forms (a static template literal) but
// never after `from`, which requires a string literal. Accepting them there
// would sweep in inline code from comments — this codebase writes it with
// backticks throughout — and a prose word colliding with a real package name
// would silently widen the candidate set.
const source = ` const source = `
/** The excluded set derived from \\\`tools\\\`, keyed off \\\`express\\\`-style paths. */ const msg = 'run require("chokidar") to load it';
const re = /"jose"/;
import { z } from "zod"; import { z } from "zod";
`; `;
assert.deepEqual([...importedPackageNames(source)], ["zod"]); assert.deepEqual([...importedPackageNames(source)], ["zod"]);