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

chore: address Copilot review round 1 (#2226)

Browse source 
- Include `optionalDependencies` in the declared tier's candidate union. A
  peer range constrains the consumer's host; an optional range is a direct
  declaration npm attempts to install, so omitting it left an optional-only
  package free to skew unseen. `peerDependencies` stays excluded, with the
  reason stated.
- Make the end-to-end fixture's client copy UNDECLARED — only the root
  manifest names `solo`, while `clients/web` merely holds a top-level copy.
  That is #2226's defining shape, and it is what makes the candidate set a
  union across installs rather than a per-install intersection; the old
  fixture would have passed a per-install implementation. Adds the mirror
  case (declared only by a client, held by the root) so the union is pinned
  in both directions.
- Update the two docs that still described the single-tier guard:
  `.claude/skills/local-dev/SKILL.md` said "nothing gates either of those,
  and `verify:dep-lockstep` is not it" and cited the live `@types/node`
  drift in the present tense; `docs/quality-gate.md` described only the
  program tier. Both now state what each tier covers and the two limits
  that remain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-05 16:09:44 -04:00
commit 8e2c4d49fa
1 changed files with 110 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

41
scripts/verify-dep-lockstep.main.test.mjs
View file

@ -528,16 +528,20 @@ test("main: exits 1 when the root validate no longer runs the sibling guard", ()
// ---------------------------------------------------------------------------
test("main: a DECLARED package skewed across installs fails even when no program holds both copies (#2226)", () => {
// The exact shape the issue reported. `solo` is installed under the root and
// under `clients/web`, and both manifests name it — but only the shared
// `core/` imports it, so the client's program resolves the root's copy alone
// and the program tier is silent, correctly. The declared tier is what sees
// it, which is the whole reason it exists: cli's `@types/node` was hoisted in
// via `@types/express` and no program ever met the root's copy.
// The exact shape the issue reported, and the client copy is deliberately
// UNDECLARED (Copilot): only the ROOT manifest names `solo`, while
// `clients/web` merely holds a top-level copy — the way cli's `@types/node`
// arrives, hoisted via `@types/express` with no range of ours governing it.
// That is what makes the candidate set a UNION across installs rather than a
// per-install intersection, and declaring it in both manifests would let a
// per-install implementation pass this test.
//
// Only the shared `core/` imports `solo`, so the client's program resolves the
// root's copy alone and the program tier is silent — correctly. The declared
// tier is the one that sees it, which is the whole reason it exists.
withFixture(
{
rootManifestDeps: { solo: "^7.0.0" },
webManifestDeps: { solo: "^7.0.0" },
webSolo: "7.0.0",
},
(dir) => {
@ -553,10 +557,11 @@ test("main: a DECLARED package skewed across installs fails even when no program
});
test("main: a declared package held at the same version in both installs passes and is counted", () => {
// Same union shape as above — root-declared, client copy undeclared — so the
// pass and the fail differ only in the version, not in how the name is found.
withFixture(
{
rootManifestDeps: { solo: "^7.0.0" },
webManifestDeps: { solo: "^7.0.0" },
webSolo: "7.8.9",
},
(dir) => {
@ -567,6 +572,26 @@ test("main: a declared package held at the same version in both installs passes
);
});
test("main: the union runs the other way too — declared only by a CLIENT, held by the root", () => {
// The mirror of the case above, and the reason `declaredPackages` unions every
// install's manifest rather than reading the root's. `@types/react` is the
// live shape: web and tui declare it, the root does not, and the two clients
// disagreed. A root-manifest-only candidate set would report success here.
withFixture(
{
webManifestDeps: { solo: "^7.0.0" },
webSolo: "7.0.0",
},
(dir) => {
const { status, out } = runGuard(dir);
assert.equal(status, 1, out);
assert.match(out, /1 declared dependency resolves/);
assert.match(out, /7\.8\.9\s+\(\.\/node_modules\/solo\)/);
assert.match(out, /7\.0\.0\s+\(clients\/web\/node_modules\/solo\)/);
},
);
});
test("main: an installed package NO manifest declares is not a declared-tier candidate", () => {
// `outer` and `inner` sit in both installs in every fixture, but no manifest
// names them. The declared tier's boundary is what the repo declares, so its