Repository navigation
chore(pack:verify): assert skills/mcpdo ships and drive an mcpdo tool call #2679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| /** | ||
| * Which bare specifiers a built bundle imports, and whether each one resolves | ||
| * from a given directory under **ESM** rules (#2650). | ||
| * | ||
| * `pack:verify` uses this to prove that every package the installed `mcpdo` | ||
| * bundle reaches for is actually resolvable from the consumer install — the | ||
| * failure a missing root `dependencies` entry or a dropped `external` produces, | ||
| * and one that only shows at the moment the import runs. Driving a couple of | ||
| * commands cannot see a lazy `import()` on a path those commands never take. | ||
| * | ||
| * Resolution is ESM on purpose. `@modelcontextprotocol/ext-tasks` exports only | ||
| * an `import` condition, so a CommonJS `require.resolve` reports it missing even | ||
| * though the bundle — which is ESM — loads it fine. The 2.10.0 hand check hit | ||
| * exactly that false failure. So the probe runs `import.meta.resolve` in a | ||
| * module placed **inside** the directory being checked, which is the resolver | ||
| * and the starting point the bundle itself gets. | ||
| * | ||
| * Extraction is a statement-anchored scan rather than a parser: esbuild emits | ||
| * every static import at column 0, so anchoring at the line start keeps prose | ||
| * in the bundle's comments (`… apart from "a digest was wrong" …`) from reading | ||
| * as a `from "<specifier>"` clause. | ||
| */ | ||
|
|
||
| import { spawnSync } from "node:child_process"; | ||
| import { rmSync, writeFileSync } from "node:fs"; | ||
| import { isBuiltin } from "node:module"; | ||
| import { join, resolve } from "node:path"; | ||
|
|
||
| const STATIC_FROM = /^(?:import|export)\b[^;]*?\bfrom\s*["']([^"']+)["']/gm; | ||
| const SIDE_EFFECT = /^import\s*["']([^"']+)["']/gm; | ||
| const DYNAMIC = /\bimport\(\s*["']([^"']+)["']\s*\)/g; | ||
|
|
||
| /** True for a specifier npm has to supply: not relative, absolute or a builtin. */ | ||
| export function isBareSpecifier(specifier) { | ||
| return ( | ||
| !specifier.startsWith(".") && | ||
| !specifier.startsWith("/") && | ||
| !specifier.includes(":") && | ||
| !isBuiltin(specifier) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} source one bundle file's text | ||
| * @returns {string[]} the bare specifiers it imports, sorted and de-duplicated | ||
| */ | ||
| export function collectBareImports(source) { | ||
| const found = new Set(); | ||
| for (const pattern of [STATIC_FROM, SIDE_EFFECT, DYNAMIC]) { | ||
| for (const match of source.matchAll(pattern)) { | ||
| if (isBareSpecifier(match[1])) found.add(match[1]); | ||
| } | ||
| } | ||
| return [...found].sort(); | ||
| } | ||
|
|
||
| const PROBE_NAME = "__pack_verify_resolve_probe.mjs"; | ||
| const PROBE = `const failed = []; | ||
| for (const s of JSON.parse(process.argv[2])) { | ||
| try { import.meta.resolve(s); } catch (e) { failed.push({ specifier: s, error: e.message }); } | ||
| } | ||
| process.stdout.write(JSON.stringify(failed)); | ||
| `; | ||
|
|
||
| /** | ||
| * ESM-resolve each specifier from inside `dir`. The probe module is written | ||
| * into `dir` and removed again, so it resolves exactly as a module there does. | ||
| * | ||
| * @param {{ dir: string, specifiers: string[] }} options | ||
| * @returns {{ specifier: string, error: string }[]} the ones that did not resolve | ||
| */ | ||
| export function unresolvedFrom({ dir, specifiers }) { | ||
| const probe = resolve(dir, PROBE_NAME); | ||
| writeFileSync(probe, PROBE); | ||
| try { | ||
| const r = spawnSync(process.execPath, [probe, JSON.stringify(specifiers)], { | ||
| cwd: dir, | ||
| encoding: "utf8", | ||
| }); | ||
| if (r.status !== 0) { | ||
| throw new Error(`resolve probe exited ${r.status}: ${r.stderr}`); | ||
| } | ||
| return JSON.parse(r.stdout); | ||
| } finally { | ||
| rmSync(probe, { force: true }); | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // Tests for `bundle-bare-imports.mjs` (#2650): the bare-specifier scan over a | ||
| // bundle, and ESM resolution from inside a directory — including the | ||
| // import-only `exports` shape a CommonJS `require.resolve` misreports. | ||
|
|
||
| import { test } from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import { | ||
| mkdirSync, | ||
| mkdtempSync, | ||
| readdirSync, | ||
| rmSync, | ||
| writeFileSync, | ||
| } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { | ||
| collectBareImports, | ||
| isBareSpecifier, | ||
| unresolvedFrom, | ||
| } from "./bundle-bare-imports.mjs"; | ||
|
|
||
| test("isBareSpecifier excludes relative, absolute, URL and builtin specifiers", () => { | ||
| assert.equal(isBareSpecifier("zod"), true); | ||
| assert.equal(isBareSpecifier("@modelcontextprotocol/client/stdio"), true); | ||
| for (const s of [ | ||
| "./chunk.js", | ||
| "../x.js", | ||
| "/abs.js", | ||
| "node:fs", | ||
| "fs", | ||
| "fs/promises", | ||
| "readline/promises", | ||
| ]) { | ||
| assert.equal(isBareSpecifier(s), false, s); | ||
| } | ||
| }); | ||
|
|
||
| test("collectBareImports finds static, multi-line, re-export, side-effect and dynamic imports", () => { | ||
| const source = [ | ||
| 'import { a } from "zod";', | ||
| "import {", | ||
| " b,", | ||
| " c", | ||
| '} from "@modelcontextprotocol/client/stdio";', | ||
| 'export * from "yaml";', | ||
| 'import "side-effect-pkg";', | ||
| 'import { readFileSync } from "fs";', | ||
| 'import { x } from "./chunk-ABC.js";', | ||
| 'const m = await import("undici");', | ||
| 'const n = await import("@napi-rs/keyring");', | ||
| ].join("\n"); | ||
| assert.deepEqual(collectBareImports(source), [ | ||
| "@modelcontextprotocol/client/stdio", | ||
| "@napi-rs/keyring", | ||
| "side-effect-pkg", | ||
| "undici", | ||
| "yaml", | ||
| "zod", | ||
| ]); | ||
| }); | ||
|
|
||
| test('collectBareImports ignores `from "..."` prose inside a comment', () => { | ||
| const source = [ | ||
| "/**", | ||
| ' * digests to check" apart from "a digest was wrong" and "the walk', | ||
| ' * event since I last looked" from "something has", without', | ||
| " */", | ||
| 'import { z } from "zod";', | ||
| ].join("\n"); | ||
| assert.deepEqual(collectBareImports(source), ["zod"]); | ||
| }); | ||
|
|
||
| test("unresolvedFrom ESM-resolves an import-only package and reports a missing one", () => { | ||
| const root = mkdtempSync(join(tmpdir(), "bare-imports-")); | ||
| try { | ||
| const pkg = join(root, "node_modules", "import-only"); | ||
| mkdirSync(pkg, { recursive: true }); | ||
| writeFileSync( | ||
| join(pkg, "package.json"), | ||
| JSON.stringify({ | ||
| name: "import-only", | ||
| type: "module", | ||
| exports: { "./sub": { import: "./sub.js" } }, | ||
| }), | ||
| ); | ||
| writeFileSync(join(pkg, "sub.js"), "export {};\n"); | ||
| const dir = join(root, "consumer"); | ||
| mkdirSync(dir); | ||
|
|
||
| const failed = unresolvedFrom({ | ||
| dir, | ||
| specifiers: ["import-only/sub", "not-installed"], | ||
| }); | ||
| assert.deepEqual( | ||
| failed.map((f) => f.specifier), | ||
| ["not-installed"], | ||
| ); | ||
| // The probe leaves nothing behind in the checked directory. | ||
| assert.deepEqual(readdirSync(dir), []); | ||
| } finally { | ||
| rmSync(root, { recursive: true, force: true }); | ||
| } | ||
| }); |
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.