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

fix: address Copilot review round 5

Browse source 
Both findings are the same shape as round 2's: a guard that documents an
invariant while checking one narrow expression of it.

`retry` was only rejected at the project level, so `it("x", { retry: 2 })` and a
`--retry` flag in an npm script both walked through a guard whose own header
said "no retry anywhere". Enforcement is extended rather than the claim
narrowed, because the decision is about the behavior, not about a config key —
all three places Vitest accepts one are now scanned, and the header names the
one that remains out of reach (a human typing --retry into their own shell),
rather than overclaiming.

The test-definition scan is deliberately positional: the retry must sit in an
options object following the test name, which is the only place Vitest reads it
from. A bare word match would fire on fixture data, on `client.retry`, on an
unrelated helper's options and on a mock — all of which exist here, and a guard
that cries wolf gets disabled. Both directions are tested. The first pattern
also missed `it.each([...])("name", { retry })` entirely, so every parameterized
suite was a blind spot; the table argument is now part of the pattern.

Config discovery named only the two `.ts` spellings this repo happens to use,
while Vitest loads twelve. A `clients/foo/vitest.config.mts` was therefore
invisible, and an invisible config yields no project to reject — the same hole
round 1 found in the hand-written CONFIG_ROOTS, one level down. Both discovery
and resolution now use the full set, ordered as Vitest orders it.

Both checks were observed firing before being trusted: a planted retry in a
launcher test and a planted vitest.config.mts each fail the guard.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-12 14:41:29 -04:00
commit f4d5b6a292
2 changed files with 187 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

119
scripts/verify-test-timeouts.mjs
View file

@ -31,7 +31,12 @@
* 3. **`retry` must stay unset.** A retry converts a load-induced red into a
* silent green on the only pre-push gate this repo has, and would re-open
* #1596 by hiding a real race behind a second attempt. That was a deliberate
* "do not raise" decision, so it is enforced rather than remembered.
* "do not raise" decision, so it is enforced rather than remembered — and
* enforced in all three places Vitest accepts one, since the decision is
* about the behavior rather than about a config key: the resolved project,
* an individual `it`/`describe` options object, and a `--retry` flag in an
* npm script (Copilot). What stays outside its reach is a human typing
* `--retry` into their own shell, which no committed check can see.
*
* It also asserts the Testing Library half, which no Vitest config can see:
* `asyncUtilTimeout` governs every `waitFor` / `findBy*` in the web projects
@ -46,6 +51,7 @@
* both `asyncUtilTimeout` assertions unmet.
*/
import { execFileSync } from "node:child_process";
import { existsSync, readdirSync, readFileSync } from "node:fs";
import { dirname, join, relative, resolve } from "node:path";
import { fileURLToPath } from "node:url";
@ -139,6 +145,22 @@ export const EXPECTED_ASYNC_UTIL_TIMEOUT = 1_000;
*/
export const CLIENTS_DIR = "clients";
/**
* Every filename Vitest will load a config from, most specific first.
*
* The full set, not the two spellings this repo happens to use: discovery is
* only deny-by-default if it sees every config Vitest would (Copilot). A
* `clients/foo/vitest.config.mts` that this list did not name would be
* invisible, and an invisible config yields no project to reject — the same
* hole as a hand-written `CONFIG_ROOTS`, one level down. Order matters at
* resolution time, where the first match wins, and mirrors Vitest's own
* preference for a `vitest.config.*` over a `vite.config.*`.
*/
export const VITEST_CONFIG_FILENAMES = Object.freeze([
...["ts", "mts", "cts", "js", "mjs", "cjs"].map((e) => `vitest.config.${e}`),
...["ts", "mts", "cts", "js", "mjs", "cjs"].map((e) => `vite.config.${e}`),
]);
/**
* Compare one resolved project against its row.
*
@ -340,6 +362,48 @@ export function checkSetupFilesWiring(
];
}
/**
* A `retry` declared on an individual test or suite, or passed on a command
* line — the two places a project-level check cannot see.
*
* The "no retry" decision is about the behavior, not about one config key, so a
* guard that only reads `project.config.retry` leaves `it("…", { retry: 2 })`
* and `--retry=2` in an npm script wide open (Copilot). Both are scanned here.
*
* The test-definition pattern deliberately requires the `retry` to sit in an
* **options object that follows the test name**, which is the only position
* Vitest reads it from. That keeps a `retry` field in fixture data, a variable
* named `retry`, or a mocked API's option out of it — this repo has several, and
* a bare `/\bretry\b/` scan would fail on all of them.
*
* @param {string} source
* @returns {string[]} the matched declarations, empty when there are none
*/
export function findTestLevelRetries(source) {
const code = stripComments(source);
// The optional group between the chain and the name is `it.each`'s table —
// `it.each([...])("name", …)` or its tagged-template form — which otherwise
// pushes the name out of the position this pattern looks for, so every
// parameterized suite would be a blind spot.
const re =
/\b(?:it|test|describe|suite|bench)(?:\.\w+)*(?:\s*(?:\([^()]*\)|`[^`]*`))?\s*\(\s*(?:"[^"]*"|'[^']*'|`[^`]*`)\s*,\s*\{[^}]*?\bretry\s*:\s*([^,}\s]+)/g;
return [...code.matchAll(re)].map((m) => `retry: ${m[1]}`);
}
/**
* A `retry` flag passed to vitest from an npm script.
*
* @param {Record<string, unknown>} scripts
* @returns {string[]} `"<name>: <script>"` for each offender
*/
export function findScriptRetries(scripts) {
return Object.entries(scripts ?? {})
.filter(
([, cmd]) => typeof cmd === "string" && /(^|\s)--retry(=|\s|$)/.test(cmd),
)
.map(([name, cmd]) => `${name}: ${String(cmd)}`);
}
/**
* Every Vitest config under `clients/`, as repo-relative directories.
*
@ -360,9 +424,7 @@ export function discoverConfigRoots(root = repoRoot) {
.filter((e) => e.isDirectory())
.map((e) => join(clients, e.name))
.filter((dir) =>
["vitest.config.ts", "vite.config.ts"].some((f) =>
existsSync(join(dir, f)),
),
VITEST_CONFIG_FILENAMES.some((f) => existsSync(join(dir, f))),
)
.map((dir) => relative(root, dir).split("\\").join("/"))
.sort();
@ -400,14 +462,14 @@ export function checkConfigRootCoverage(discovered, configured = CONFIG_ROOTS) {
/**
* Resolve every project of one config, via Vitest's own resolver.
*
* @param {string} root absolute directory holding the `vitest.config.ts` or `vite.config.ts`
* @param {string} root absolute directory holding one of `VITEST_CONFIG_FILENAMES`
* @returns {Promise<{name: string, config: Record<string, unknown>}[]>}
*/
async function resolveProjects(root) {
const { createVitest } = await import("vitest/node");
const config = ["vitest.config.ts", "vite.config.ts"]
.map((f) => join(root, f))
.find((f) => existsSync(f));
const config = VITEST_CONFIG_FILENAMES.map((f) => join(root, f)).find((f) =>
existsSync(f),
);
if (!config) {
throw new Error(`no vitest/vite config under ${root}`);
}
@ -482,6 +544,43 @@ async function main() {
}
}
// `retry`, in the two places a resolved project cannot show it. Tracked files
// only — an untracked scratch test is not something this repo ships.
const testFiles = execFileSync(
"git",
["ls-files", "*.test.ts", "*.test.tsx", "*.test.mts", "*.stories.tsx"],
{ cwd: repoRoot, encoding: "utf-8" },
)
.split("\n")
.filter(Boolean);
for (const file of testFiles) {
const found = findTestLevelRetries(
readFileSync(resolve(repoRoot, file), "utf-8"),
);
for (const decl of found) {
failures.push(
`${file} declares \`${decl}\` on a test or suite — a retry turns a ` +
`load-induced red into a silent green on the only pre-push gate here (#1596)`,
);
}
}
const manifests = execFileSync(
"git",
["ls-files", "package.json", "*/package.json", "*/*/package.json"],
{ cwd: repoRoot, encoding: "utf-8" },
)
.split("\n")
.filter(Boolean);
for (const file of manifests) {
const { scripts } = JSON.parse(
readFileSync(resolve(repoRoot, file), "utf-8"),
);
for (const offender of findScriptRetries(scripts)) {
failures.push(`${file} passes --retry from a script — ${offender}`);
}
}
for (const { file, from, project } of ASYNC_UTIL_SITES) {
const abs = resolve(repoRoot, file);
if (!existsSync(abs)) {
@ -513,7 +612,9 @@ async function main() {
console.log(
`verify:test-timeouts OK — ${checked} Vitest projects on stated budgets, ` +
`no retry, ${ASYNC_UTIL_SITES.length} asyncUtilTimeout sites configured.`,
`no retry in any project, test or script (${testFiles.length} test files, ` +
`${manifests.length} manifests), ` +
`${ASYNC_UTIL_SITES.length} asyncUtilTimeout sites configured.`,
);
}

75
scripts/verify-test-timeouts.test.mjs
View file

@ -27,8 +27,11 @@ import {
checkProject,
checkSetupFilesWiring,
discoverConfigRoots,
findScriptRetries,
findTestLevelRetries,
identifyProject,
stripComments,
VITEST_CONFIG_FILENAMES,
} from "./verify-test-timeouts.mjs";
const ok = { ...EXPECTED_TIMEOUTS };
@ -311,3 +314,75 @@ test("discovery finds exactly the configs this guard is set up to check", () =>
);
assert.deepEqual(checkConfigRootCoverage(discoverConfigRoots()), []);
});
test("a retry on an individual test or suite is caught", () => {
// The project-level check cannot see these, and the decision is about the
// behavior rather than about one config key (Copilot).
for (const src of [
`it("x", { retry: 2 }, async () => {});`,
`test("x", { retry: 1 }, () => {});`,
`describe("x", { retry: 3 }, () => {});`,
`it.each([1])("x %i", { retry: 2 }, () => {});`,
'describe.each([[1, 2]])("x", { retry: 1 }, () => {});',
`it('single quoted', { timeout: 100, retry: 2 }, () => {});`,
]) {
assert.equal(findTestLevelRetries(src).length, 1, src);
}
});
test("a retry that is not a test option is not a false positive", () => {
// All four shapes exist in this repo. A bare /\bretry\b/ scan would fail on
// every one of them, and a guard that cries wolf gets disabled.
for (const src of [
`const fixture = { retry: 2 };`,
`expect(client.retry).toBe(2);`,
`it("x", async () => { await withRetries({ retry: 2 }); });`,
`vi.mock("x", () => ({ retry: 2 }));`,
]) {
assert.deepEqual(findTestLevelRetries(src), [], src);
}
});
test("a retry hidden in a comment is not reported", () => {
assert.deepEqual(
findTestLevelRetries(`// it("x", { retry: 2 }, () => {});`),
[],
);
});
test("a --retry flag in an npm script is caught", () => {
assert.deepEqual(findScriptRetries({ test: "vitest run --retry=2" }), [
"test: vitest run --retry=2",
]);
assert.equal(findScriptRetries({ test: "vitest run --retry 2" }).length, 1);
});
test("a script that merely contains the word retry is not a false positive", () => {
assert.deepEqual(
findScriptRetries({ test: "vitest run retry-helper.ts" }),
[],
);
assert.deepEqual(findScriptRetries({ test: "vitest run --no-retry-x" }), []);
assert.deepEqual(findScriptRetries({}), []);
assert.deepEqual(findScriptRetries(undefined), []);
});
test("discovery knows every filename Vitest loads a config from", () => {
// Deny-by-default only holds if discovery sees every config Vitest would; a
// `vitest.config.mts` this list did not name would be invisible (Copilot).
for (const ext of ["ts", "mts", "cts", "js", "mjs", "cjs"]) {
assert.ok(
VITEST_CONFIG_FILENAMES.includes(`vitest.config.${ext}`),
`vitest.config.${ext}`,
);
assert.ok(
VITEST_CONFIG_FILENAMES.includes(`vite.config.${ext}`),
`vite.config.${ext}`,
);
}
// vitest.config.* is preferred over vite.config.*, as Vitest itself does.
assert.ok(
VITEST_CONFIG_FILENAMES.indexOf("vitest.config.cjs") <
VITEST_CONFIG_FILENAMES.indexOf("vite.config.ts"),
);
});