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 3 (four suppressed findings)

Browse source 
No inline comments this round; all four came from the suppressed block.

The AppRenderer one is a regression I introduced by combining two changes that
were individually right. #2323 deleted that `waitFor`'s `{ timeout: 5000 }` as a
budget nested inside an equal one, which it was — and then round 2 pinned
asyncUtilTimeout at 1000, which is the exact default the site's own comment
records failing twice. The raise is restored: the enclosing budget is 15000 now,
so it is observable rather than unwinnable, and the value it beats is a measured
failure rather than a library default.

The guard could approve a configure() imported from the wrong Testing Library
copy — `cleanup` from @testing-library/react and `configure` from
@testing-library/dom satisfied two independent regexes while configuring a copy
no test calls. It now resolves the binding `configure` has in the import from
the required module, follows an alias, and reads asyncUtilTimeout out of that
call rather than out of the file. Three regression tests, including the split
import and a decoy constant elsewhere in the module.

secret-store-selection's 60s exception is hoisted to
LOCK_RETRIES_EXHAUSTED_MS, matching file-lock.test.ts, which bounds the same
wait — the new rule asks for named as well as justified.

And the testing skill still prescribed `HEADER_ANIM_MS + 200`, so a test written
by following it would have bypassed the shared slack constant this change
introduced. It now prescribes `HEADER_ANIM_MS + RAF_SLACK_MS` and says why the
two terms are separate.

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 12:02:57 -04:00
commit 1cc1b48c30
2 changed files with 140 additions and 53 deletions
Download patch file Download diff file Expand all files Collapse all files

29
clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx
View file

@ -557,17 +557,28 @@ describe("AppRenderer", () => {
// budget that is too tight rather than of a broken observer. Raised
// rather than retried: a `waitFor` that is generous costs nothing on the
// passing path, since it returns as soon as the assertion holds.
await waitFor(() =>
expect(bridge.sendHostContextChange).toHaveBeenCalledWith(
expect.objectContaining({
theme: "dark",
styles: expect.objectContaining({
variables: expect.objectContaining({
"--color-background-primary": "#101113",
//
// #2323 briefly deleted this argument as one of its "a budget inside an
// equal budget can never be observed" cases, which it was: 5000 inside a
// 5000ms test could never win. That is fixed from the other end now — the
// enclosing budget is 15000 — so the raise is observable again and has to
// stay, because the value it beats is a measured failure rather than a
// default. It does NOT ride on `asyncUtilTimeout`, which is pinned at
// 1000 precisely because raising it globally was measured worse
// (Copilot).
await waitFor(
() =>
expect(bridge.sendHostContextChange).toHaveBeenCalledWith(
expect.objectContaining({
theme: "dark",
styles: expect.objectContaining({
variables: expect.objectContaining({
"--color-background-primary": "#101113",
}),
}),
}),
}),
),
),
{ timeout: 5000 },
);
} finally {
getComputedStyleSpy.mockRestore();

75
clients/web/src/test/integration/auth/node/secret-store-selection.test.ts
View file

@ -82,6 +82,15 @@ async function loadWithProbe(available: boolean, detail = "no D-Bus session") {
return import("@inspector/core/auth/node/secret-store-selection.js");
}
/**
* Past the integration project's own 30s deliberately, and one of the few sites
* that should be (#2323): the test using this waits on a second process holding
* the secrets-file lock until `proper-lockfile`'s retries run out, which is real
* elapsed contention rather than slack for a loaded machine. Named to match
* `file-lock.test.ts`, which bounds the same wait.
*/
const LOCK_RETRIES_EXHAUSTED_MS = 60_000;
describe("parseSecretStoreEnv", () => {
it("accepts each kind, case- and whitespace-insensitively", () => {
expect(parseSecretStoreEnv("keyring")).toBe("keyring");
@ -684,42 +693,42 @@ describe("absorbFileSecretsIntoKeyring", () => {
return filePath;
}
it("does not throw when another process holds the lock (#2082)", async () => {
// `withSecretFileLock` throws on a lock held past its retry budget, which
// is right for a `set` — the user is waiting on that value — and wrong
// here. This function is awaited directly by both `resolveSecretStore`
// branches, so an escaping error fails store resolution and with it the
// whole session: a stuck writer elsewhere on the box would stop the
// Inspector from starting. Leaving the file for the next run is the only
// acceptable outcome.
const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(true);
it(
"does not throw when another process holds the lock (#2082)",
async () => {
// `withSecretFileLock` throws on a lock held past its retry budget, which
// is right for a `set` — the user is waiting on that value — and wrong
// here. This function is awaited directly by both `resolveSecretStore`
// branches, so an escaping error fails store resolution and with it the
// whole session: a stuck writer elsewhere on the box would stop the
// Inspector from starting. Leaving the file for the next run is the only
// acceptable outcome.
const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(true);
const properLockfile = (await import("proper-lockfile")).default;
const release = await properLockfile.lock(filePath, {
realpath: false,
stale: 10_000,
});
const properLockfile = (await import("proper-lockfile")).default;
const release = await properLockfile.lock(filePath, {
realpath: false,
stale: 10_000,
});
await expect(
mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()),
).resolves.toBeUndefined();
await release();
await expect(
mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()),
).resolves.toBeUndefined();
await release();
// Left exactly as it was, and said so — asserting the *lock* message
// specifically, since the pre-existing claim-failure warning also ends in
// "left in place" and would let this pass without the lock ever being hit.
expect(existsSync(filePath)).toBe(true);
expect(warn).toHaveBeenCalledWith(
expect.stringContaining("Could not lock the secrets file"),
);
// Past the integration project's own 30s deliberately, and one of the few
// sites that should be (#2323): this test waits on a second process holding
// the secrets-file lock until `proper-lockfile` gives up on it, which is
// real elapsed contention rather than slack for a loaded machine.
}, 60_000);
// Left exactly as it was, and said so — asserting the *lock* message
// specifically, since the pre-existing claim-failure warning also ends in
// "left in place" and would let this pass without the lock ever being hit.
expect(existsSync(filePath)).toBe(true);
expect(warn).toHaveBeenCalledWith(
expect.stringContaining("Could not lock the secrets file"),
);
},
LOCK_RETRIES_EXHAUSTED_MS,
);
it("takes no lock when there is nothing to migrate (#2082)", async () => {
// The overwhelmingly common startup: a keychain is available and no file