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

fix: address Copilot review round 6

Browse source 
The nesting rule in AGENTS.md was written backwards. "Never nest a budget inside
an equal-or-larger one" forbids the two arrangements this PR deliberately ships
(5s inside 15s, 15s inside 25s) and permits the broken one it exists to prevent.
It now says what it means: an inner budget must be strictly smaller than the
budget enclosing it, arranged by raising the outer rather than shrinking the
inner, since the inner is the bound carrying the useful diagnostic.

The retry scan is structural now rather than a regex. A character class cannot
step over a chain argument containing nested parentheses, so
`it.each([makeCase()])` and `it.skipIf(() => isWindows())` were silently
skipped — the second blind spot of the same kind in two rounds, which is what
makes the pattern the wrong tool. A bracket matcher that skips string literals
handles both, and the positional requirement that keeps fixture data and
`client.retry` out of the results is unchanged. Seven positive forms and six
false-positive shapes are pinned.

The asyncUtilTimeout check read only the first configure() call, so a file
holding the expected call followed by an overriding one passed while its
effective timeout was something else. Every call is inspected and all of them
must agree, which is stricter than checking the last and gives a message naming
the offending value.

Also corrected two PR-description bullets that still described superseded
behavior: the cli child timer stays at 15_000 with the test raised to 25_000,
and AppRenderer's waitFor keeps its 5000 rather than losing it.

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:57:30 -04:00
commit 1b7ff72e8d
2 changed files with 193 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -312,18 +312,41 @@ export function checkAsyncUtilSource(
}
const name = binding[1] ?? "configure";
const call = new RegExp(`\\b${name}\\s*\\(([\\s\\S]*?)\\)`).exec(code);
if (!call) {
// EVERY call, not the first. A setup file holding the expected call followed
// by a second one is the shape that matters: the later call wins at runtime,
// so inspecting only the first would approve a file whose effective timeout
// is something else entirely (Copilot). Requiring all of them to agree is
// stricter than checking the last and gives a clearer message than "the
// effective value is X" would.
const calls = [];
const nameRe = new RegExp(`\\b${name}\\s*\\(`, "g");
let m;
while ((m = nameRe.exec(code)) !== null) {
const open = m.index + m[0].length - 1;
const close = matchBracket(code, open);
if (close === -1) break;
calls.push(code.slice(open + 1, close));
nameRe.lastIndex = close;
}
if (calls.length === 0) {
failures.push(`does not call ${name}()`);
return failures;
}
const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(call[1]);
if (!match) {
const configured = calls
.map((args) => /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(args))
.filter(Boolean)
.map((match) => match[1]);
if (configured.length === 0) {
failures.push(`calls ${name}() without an asyncUtilTimeout`);
} else if (Number(match[1].replace(/_/g, "")) !== expected) {
failures.push(
`configures asyncUtilTimeout as ${match[1]}, expected ${expected}`,
);
return failures;
}
for (const value of configured) {
if (Number(value.replace(/_/g, "")) !== expected) {
failures.push(
`configures asyncUtilTimeout as ${value}, expected ${expected}`,
);
}
}
return failures;
}
@ -362,6 +385,76 @@ export function checkSetupFilesWiring(
];
}
/**
* Skip whitespace from `i`.
*
* @param {string} code
* @param {number} i
* @returns {number}
*/
function skipWs(code, i) {
while (i < code.length && /\s/.test(code[i])) i += 1;
return i;
}
/**
* Index just past the string or template literal starting at `i`.
*
* @param {string} code
* @param {number} i index of the opening quote
* @returns {number} index after the closing quote, or `code.length` if unclosed
*/
function skipString(code, i) {
const quote = code[i];
let j = i + 1;
while (j < code.length) {
if (code[j] === "\\") {
j += 2;
continue;
}
if (code[j] === quote) return j + 1;
j += 1;
}
return code.length;
}
/**
* Index of the bracket closing the one at `i`, honoring nesting and skipping
* string literals so a bracket inside a string cannot unbalance the count.
*
* This is what a regex cannot do, and the reason this scan is structural: a
* character class like `[^()]*` cannot consume `it.each([makeCase()])` or
* `it.skipIf(() => isWindows())`, so every such suite was a blind spot
* (Copilot).
*
* @param {string} code
* @param {number} i index of the opening bracket
* @returns {number} index of the matching close, or -1
*/
function matchBracket(code, i) {
const open = code[i];
const close = open === "(" ? ")" : "}";
let depth = 0;
let j = i;
while (j < code.length) {
const c = code[j];
if (c === '"' || c === "'" || c === "`") {
j = skipString(code, j);
continue;
}
if (c === open) depth += 1;
else if (c === close) {
depth -= 1;
if (depth === 0) return j;
}
j += 1;
}
return -1;
}
/** Test-definition heads a `retry` option can be attached to. */
const TEST_HEAD = /\b(?:it|test|describe|suite|bench)(?:\.\w+)*/g;
/**
* A `retry` declared on an individual test or suite, or passed on a command
* line — the two places a project-level check cannot see.
@ -370,24 +463,69 @@ export function checkSetupFilesWiring(
* 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.
* Structural rather than a single regex, for two reasons learned one round
* apart. The `retry` must sit in an **options object that follows the test
* name**, which is the only position Vitest reads it from — that is what keeps
* a `retry` field in fixture data, a variable named `retry`, or a mocked API's
* option out of it, and this repo has several. And the chain may carry an
* argument of its own — `it.each([makeCase()])`, `it.skipIf(() => isWin())`,
* `it.each\`table\`` — which a character-class regex cannot step over once it
* contains nested parentheses, silently skipping every such suite.
*
* @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]}`);
const found = [];
TEST_HEAD.lastIndex = 0;
let head;
while ((head = TEST_HEAD.exec(code)) !== null) {
const decl = readRetryOption(code, head.index + head[0].length);
if (decl) found.push(decl);
}
return found;
}
/**
* Read a `retry` out of the options object of the test call starting at `i`.
*
* @param {string} code comment-stripped source
* @param {number} i index just past the `it`/`describe`/… chain
* @returns {string | null}
*/
function readRetryOption(code, i) {
let j = skipWs(code, i);
// `it.each\`table\`` puts a tagged template between the chain and the call.
if (code[j] === "`") j = skipWs(code, skipString(code, j));
if (code[j] !== "(") return null;
// The first `(` is either the call itself or the chain's own argument
// (`it.each([...])`, `it.skipIf(…)`). It is the call when a string literal —
// the test name — comes first; otherwise step over it, balanced, and the real
// call is the next `(`.
let open = j;
let name = skipWs(code, open + 1);
if (!/["'`]/.test(code[name] ?? "")) {
const close = matchBracket(code, open);
if (close === -1) return null;
open = skipWs(code, close + 1);
if (code[open] !== "(") return null;
name = skipWs(code, open + 1);
if (!/["'`]/.test(code[name] ?? "")) return null;
}
let k = skipWs(code, skipString(code, name));
if (code[k] !== ",") return null;
k = skipWs(code, k + 1);
if (code[k] !== "{") return null;
const end = matchBracket(code, k);
if (end === -1) return null;
const options = code.slice(k + 1, end);
const retry = /(?:^|[\s,{])retry\s*:\s*([^,}\s]+)/.exec(options);
return retry ? `retry: ${retry[1]}` : null;
}
/**

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

@ -324,6 +324,11 @@ test("a retry on an individual test or suite is caught", () => {
`describe("x", { retry: 3 }, () => {});`,
`it.each([1])("x %i", { retry: 2 }, () => {});`,
'describe.each([[1, 2]])("x", { retry: 1 }, () => {});',
// Nested parentheses in the chain's own argument. A character class cannot
// step over either of these, so both were blind spots (Copilot).
'it.each([makeCase()])("x", { retry: 2 }, () => {});',
'it.skipIf(() => isWindows())("x", { retry: 2 }, () => {});',
'it.each`a|b`("x", { retry: 3 }, () => {});',
`it('single quoted', { timeout: 100, retry: 2 }, () => {});`,
]) {
assert.equal(findTestLevelRetries(src).length, 1, src);
@ -338,6 +343,8 @@ test("a retry that is not a test option is not a false positive", () => {
`expect(client.retry).toBe(2);`,
`it("x", async () => { await withRetries({ retry: 2 }); });`,
`vi.mock("x", () => ({ retry: 2 }));`,
`it("has retry: 2 in the name", () => {});`,
`const o = { noRetry: 2 };`,
]) {
assert.deepEqual(findTestLevelRetries(src), [], src);
}
@ -386,3 +393,21 @@ test("discovery knows every filename Vitest loads a config from", () => {
VITEST_CONFIG_FILENAMES.indexOf("vite.config.ts"),
);
});
test("every configure() call is inspected, not just the first", () => {
// The later call wins at runtime, so approving a file on its first call would
// approve one whose effective timeout is something else entirely (Copilot).
const overridden = `import { configure } from "storybook/test";
configure({ asyncUtilTimeout: 1000 });
configure({ asyncUtilTimeout: 5000 });`;
assert.deepEqual(checkAsyncUtilSource(overridden, "storybook/test"), [
"configures asyncUtilTimeout as 5000, expected 1000",
]);
});
test("a configure() call that sets something else does not hide the real one", () => {
const source = `import { configure } from "storybook/test";
configure({ testIdAttribute: "data-testid" });
configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
});