fix: address Copilot review round 4
The comment stripper had a real hole: the line-comment regex was anchored, so it
only removed `//` at the start of a line and a call disabled after code —
`const disabled = true; // configure({ asyncUtilTimeout: 1000 });` — survived it
and the guard passed with no effective configuration. Replaced with a scanner,
because a regex gets this wrong in both directions: it has to strip a comment
that follows code, and it must NOT treat the `//` inside a string literal as one,
which would silently truncate any line holding a URL. Quote tracking (with
escapes) is the only way to have both. Three regression tests.
Two documentation claims were also wrong. The guard header and quality-gate.md
cited #2278 and #2250 as instances of a budget nobody chose; they were not —
#2278 was a missing condition wait around a geometry read and #2250 a real race
in a test's own timing, both fixed by making the test wait for the right thing.
Presenting a race fix as evidence for a larger ceiling argues against #1596,
which this guard exists to uphold. Only the budget and poll-ceiling cases are
cited now, and both files say explicitly why those two are excluded.
And browser-timeouts.mjs overstated its own behavior twice: `bestEffort` can
spend its full budget on an otherwise passing smoke precisely because its expiry
is caught, and raising any budget does delay the report when the thing waited on
never arrives. The diagnostics improve the eventual message; they do not make
the wait shorter. Both now stated plainly, so the header is something to weigh a
change against rather than a reassurance.
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
parent
1cc1b48c30
commit
7b794de0b0
4 changed files with 104 additions and 19 deletions
|
|
@ -40,7 +40,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor
|
|||
| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Five more arrive with the skills (#2163). `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. `claude-cli.test.mjs` covers the one place that decides how the CLI is spawned — the Windows `.cmd` shim needs a shell, and its arguments then need quoting — with the platform **injected**, since a win32-only bug that no runner here executes is exactly the kind that ships. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. |
|
||||
| `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. |
|
||||
| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`.
|
||||
| `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #2278, #2250, #1942 and #1742 were each an instance, found one site at a time. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. Three properties beyond the numbers: a **seventh project** with no row is an error rather than a silent skip, since an unstated budget is precisely the drift being prevented; **`retry` must stay unset**, because a retry turns a load-induced red into a silent green on the only pre-push gate here and would re-open #1596; and both web projects must `configure({ asyncUtilTimeout })`, the Testing Library half that no Vitest config can see — it governs 788 `waitFor`/`findBy*` sites and is the binding bound on any async assertion, being tighter than any per-test budget here. What is enforced there is that the value is **stated**, not that it is large: raising it to 5000 was tried and measured worse (three arms of three full unit runs — unmodified `v2/main` 3/3 green, the raise 1/4/0 files timing out, the raise reverted 3/3 green), because a wait that is *meant* to expire spends its whole budget on the happy path, so a 5x raise is a 5x cost on exactly those tests and starves the worker pool. The reasoning is recorded at the call site. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its pure decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`.
|
||||
| `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #1942 and #1742 were each an instance, found one site at a time. Deliberately **not** cited here: #2278 (a missing condition wait around a geometry read) and #2250 (a real race in a test's own timing) were fixed by making the test wait for the right thing, and presenting a race fix as evidence for a larger ceiling would argue against #1596. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. Three properties beyond the numbers: a **seventh project** with no row is an error rather than a silent skip, since an unstated budget is precisely the drift being prevented; **`retry` must stay unset**, because a retry turns a load-induced red into a silent green on the only pre-push gate here and would re-open #1596; and both web projects must `configure({ asyncUtilTimeout })`, the Testing Library half that no Vitest config can see — it governs 788 `waitFor`/`findBy*` sites and is the binding bound on any async assertion, being tighter than any per-test budget here. What is enforced there is that the value is **stated**, not that it is large: raising it to 5000 was tried and measured worse (three arms of three full unit runs — unmodified `v2/main` 3/3 green, the raise 1/4/0 files timing out, the raise reverted 3/3 green), because a wait that is *meant* to expire spends its whole budget on the happy path, so a 5x raise is a 5x cost on exactly those tests and starves the worker pool. The reasoning is recorded at the call site. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its pure decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`.
|
||||
| `npm run local:gate` | **Mandatory pre-push command.** `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. |
|
||||
| `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). |
|
||||
|
||||
|
|
|
|||
|
|
@ -10,16 +10,23 @@
|
|||
* has, and for the same reason: a budget nobody can find is a budget nobody
|
||||
* revisits.
|
||||
*
|
||||
* These are generous on purpose and cost a passing run nothing. Playwright
|
||||
* budgets are ceilings on a poll, not sleeps: every one of them returns the
|
||||
* instant its locator condition holds. What they have to absorb is a smoke
|
||||
* running against a cold `dist/` build on a machine already carrying three
|
||||
* other agent sessions' gates.
|
||||
* Three of the four are ceilings on a poll rather than sleeps — `ui`,
|
||||
* `roundTrip` and `nested` each return the instant their locator condition
|
||||
* holds, so a passing smoke pays only the time the app actually took. What they
|
||||
* absorb is a smoke running against a cold `dist/` build on a machine already
|
||||
* carrying other agent sessions' gates.
|
||||
*
|
||||
* ⚠️ Raising one of these does NOT make a smoke wait longer for a server that
|
||||
* is never coming: the launcher-death race in `smoke-web-browser.mjs` and the
|
||||
* `waitForStage` diagnostics in the other two are what report that, and they
|
||||
* report it with a cause rather than a timeout.
|
||||
* ⚠️ `bestEffort` is the exception and is not free: its expiry is caught and
|
||||
* ignored, so a smoke that never reaches network idle spends the full budget on
|
||||
* an otherwise **passing** run. That is why it is the smallest of the four, and
|
||||
* why it should stay that way.
|
||||
*
|
||||
* ⚠️ Raising any of these does delay the report when the thing waited on never
|
||||
* arrives — a larger budget is a longer wait before the failure. What the
|
||||
* launcher-death race in `smoke-web-browser.mjs` and the `waitForStage`
|
||||
* diagnostics in the other two buy is a better *eventual* message, naming a
|
||||
* cause instead of a locator; they do not make the wait shorter. Weigh both
|
||||
* when changing a value here.
|
||||
*/
|
||||
|
||||
export const BROWSER_TIMEOUTS = Object.freeze({
|
||||
|
|
|
|||
|
|
@ -10,8 +10,13 @@
|
|||
* eight logical cores). A correct, deterministic test cut off mid-flight by
|
||||
* such a budget fails a gate its diff did not break, which trains people to
|
||||
* re-run rather than read — the same argument AGENTS.md's "Lint has no warning
|
||||
* tier" makes from the other direction. #2292, #2278, #2250, #1942 and #1742
|
||||
* were each that class, found and fixed one site at a time.
|
||||
* tier" makes from the other direction. #2292, #1942 and #1742 were each that
|
||||
* class — a budget or poll ceiling nobody chose — found and fixed one site at a
|
||||
* time. ⚠️ Not every timing failure in this repo's history belongs here: #2278
|
||||
* was a missing condition wait around a geometry read and #2250 a genuine race
|
||||
* in a test's own timing, both fixed by making the test wait for the right
|
||||
* thing. Citing those as evidence for a larger ceiling would argue against
|
||||
* #1596, which this guard is meant to uphold rather than erode (Copilot).
|
||||
*
|
||||
* Three properties, which is what makes this worth a guard rather than a
|
||||
* comment:
|
||||
|
|
@ -172,22 +177,67 @@ export function checkProject(name, config, expected = EXPECTED_PROJECTS) {
|
|||
}
|
||||
|
||||
/**
|
||||
* Strip `//` and block comments so a commented-out call cannot satisfy a check.
|
||||
* Strip comments so a disabled call cannot satisfy a check.
|
||||
*
|
||||
* Comment-aware rather than exact, deliberately: a naive scan of the raw source
|
||||
* would accept a `// configure({ asyncUtilTimeout: 1000 })` left behind by
|
||||
* someone disabling it, which is the most likely way this stops being
|
||||
* configured (Copilot). String literals are not parsed out — a `configure(` in
|
||||
* a string would still be accepted — but these two files are fifteen lines each
|
||||
* and the false-positive that matters is the commented one.
|
||||
* configured (Copilot).
|
||||
*
|
||||
* A scanner rather than a regex, for two reasons that a regex gets wrong in
|
||||
* opposite directions. It must strip a comment that **follows code** on a line
|
||||
* — `const disabled = true; // configure({ … })` — which an anchored
|
||||
* line-comment pattern misses entirely (Copilot); and it must NOT treat the
|
||||
* `//` inside a string literal as the start of one, which an unanchored pattern
|
||||
* would, silently truncating any line holding a URL. Tracking quotes is the
|
||||
* only way to have both.
|
||||
*
|
||||
* @param {string} source
|
||||
* @returns {string}
|
||||
*/
|
||||
export function stripComments(source) {
|
||||
return source
|
||||
.replace(/\/\*[\s\S]*?\*\//g, "")
|
||||
.replace(/(^|\n)\s*\/\/[^\n]*/g, "$1");
|
||||
let out = "";
|
||||
let quote = null;
|
||||
for (let i = 0; i < source.length; i += 1) {
|
||||
const c = source[i];
|
||||
const next = source[i + 1];
|
||||
if (quote) {
|
||||
// A backslash escapes the next character, so a `\"` cannot close a `"`.
|
||||
if (c === "\\") {
|
||||
out += c + (next ?? "");
|
||||
i += 1;
|
||||
continue;
|
||||
}
|
||||
if (c === quote) quote = null;
|
||||
out += c;
|
||||
continue;
|
||||
}
|
||||
if (c === '"' || c === "'" || c === "`") {
|
||||
quote = c;
|
||||
out += c;
|
||||
continue;
|
||||
}
|
||||
if (c === "/" && next === "/") {
|
||||
while (i < source.length && source[i] !== "\n") i += 1;
|
||||
// Keep the newline, so line structure (and any anchored match a caller
|
||||
// makes) survives the strip.
|
||||
out += "\n";
|
||||
continue;
|
||||
}
|
||||
if (c === "/" && next === "*") {
|
||||
i += 2;
|
||||
while (
|
||||
i < source.length &&
|
||||
!(source[i] === "*" && source[i + 1] === "/")
|
||||
) {
|
||||
i += 1;
|
||||
}
|
||||
i += 1;
|
||||
continue;
|
||||
}
|
||||
out += c;
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -222,6 +222,34 @@ configure({ asyncUtilTimeout: 1_000 });`;
|
|||
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
|
||||
});
|
||||
|
||||
test("a call disabled in a TRAILING line comment does not satisfy the check", () => {
|
||||
// The anchored regex this replaced only stripped `//` at the start of a line,
|
||||
// so a call commented out after code survived and the guard passed with no
|
||||
// effective configuration (Copilot).
|
||||
const source = `import { configure } from "storybook/test";
|
||||
const disabled = true; // configure({ asyncUtilTimeout: 1000 });`;
|
||||
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
|
||||
"does not call configure()",
|
||||
]);
|
||||
});
|
||||
|
||||
test("a // inside a string literal is not treated as a comment", () => {
|
||||
// The failure mode in the other direction: an unanchored strip would truncate
|
||||
// any line holding a URL, which could remove the real call.
|
||||
const source = `import { configure } from "storybook/test";
|
||||
const docs = "https://testing-library.com/docs";
|
||||
configure({ asyncUtilTimeout: 1000 });`;
|
||||
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
|
||||
assert.match(stripComments(source), /https:\/\/testing-library/);
|
||||
});
|
||||
|
||||
test("stripComments handles escapes and mid-line block comments", () => {
|
||||
assert.equal(stripComments("a /* x */ b"), "a b");
|
||||
// A backslash-escaped quote must not close the string early, or everything
|
||||
// after it would be scanned as code.
|
||||
assert.match(stripComments('const s = "a\\" // b";\nkeep'), /keep/);
|
||||
});
|
||||
|
||||
test("stripComments leaves executable code alone", () => {
|
||||
const source = `/** header */
|
||||
import { configure } from "storybook/test"; // trailing
|
||||
|
|
|
|||
Loading…
Reference in a new issue