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 5 on the SDK watch

Browse source 
Four findings. The first ends a pattern rather than patching it again.

1. The tool grants did not enforce the boundary they claimed, for two
   reasons. `--allowedTools` only PRE-APPROVES; `--tools` is what
   restricts availability — a distinction this repo had already learned
   and written down in `scripts/skill-eval.mjs`, and which I failed to
   apply here. And a `Bash(...)` rule can match a COMPOUND command
   (`gh release view … && curl …`), so the denials could be walked past.

   That is the fourth grant in this review whose real surface was wider
   than it looked, after `gh api`, `gh issue comment --body-file` and
   `npm view --registry`. Each fix scoped the command and left something
   else open, so this one removes the capability instead: THE MODEL NOW
   HAS NO BASH AT ALL.

   Upstream release notes are prefetched by a deterministic `run:` step
   into `upstream-release-notes.md` (the 30 most recent releases via
   `gh api`), and the prompt points the model at that file and tells it
   to say so plainly if the range it needs is absent. Verified against
   the real ext-apps releases API. The model's tools are now
   `--tools "Read,Grep,Glob"` for availability, the same three in
   `--allowedTools` so no headless run stalls on an unanswerable prompt,
   and `--disallowedTools` naming Bash and the write/network tools as a
   third layer — the same three-part shape `skill-eval.mjs` uses.

2. The security documentation asserted something false. `sweep` inherits
   `issues: write` because filing issues is its purpose, so "the ONLY job
   holding a token that can write" was never true. Both AGENTS.md and the
   workflow comment now state the actual invariant: NO MODEL RUNS IN A
   WRITE-CAPABLE JOB. Two of three jobs write; neither runs a model.

   The test changed with it, and this is the substantive half: it was
   asserting `analyze.permissions === {contents: read}`, which is one
   job's slice and would have passed happily if someone added a model
   step to `post`. It now iterates EVERY job and, for each that runs
   `claude-code-action`, asserts no write scope and no posting step —
   falling back to the inherited top-level permissions, so an omitted
   `permissions:` block fails rather than defaulting silently.

3. (Suppressed.) The generated checklist told maintainers to edit
   `package.json` unconditionally, which is wrong for the one case this
   script models separately: `ext-apps` is `^1.7.4`, which already admits
   a 1.8.0 target, so only the lockfile moves. `needsManifestEdit` now
   selects the checklist items by asking whether each behind row's
   declared range admits the target — exact pins need the edit, satisfied
   ranges do not, and an unparseable declaration errs toward asking for a
   look.

4. (Suppressed.) The `post` job comment carried the same false claim as
   finding 2; corrected in the same change.

Mutation-checked, each failing a different set:

  needsManifestEdit -> true  -> its unit test + the ext-apps body test
  --tools line deleted       -> the tool-availability test

61 tests in this file; `npm run local:gate` green end to end.

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

53
scripts/sdk-watch.mjs
View file

@ -346,6 +346,56 @@ export function buildIssueTitle(state) {
const cell = (value) => String(value).replace(/\|/g, "\\|");
/**
* Does adopting `target` require editing the root manifest, or only the lockfile?
*
* ⚠️ Not every bump is a manifest edit, and saying so unconditionally was wrong
* for the very case this script exists to handle separately (Copilot). The four
* `typescript-sdk` packages are pinned **exactly**, so any new version needs the
* manifest changed. `ext-apps` is a **range** (`^1.7.4`), so a 1.8.0 target is
* already satisfied by what `package.json` says and only `npm install` is needed
* — telling a maintainer to edit the manifest there sends them to change a line
* that is already correct.
*
* A row whose declared value is not a parseable range (an unparsed dependency,
* or the `(undeclared)` placeholder) counts as needing the edit: that is the
* conservative direction, since it asks for a look rather than asserting none is
* required.
*
* @param {Array<{declared: string, behind: boolean}>} rows
* @param {string} target
* @returns {boolean}
*/
export function needsManifestEdit(rows, target) {
return rows
.filter((r) => r.behind)
.some(
(r) =>
!semver.validRange(r.declared) || !semver.satisfies(target, r.declared),
);
}
/**
* The first checklist items, which differ by the answer above.
*
* @param {Array<{declared: string, behind: boolean}>} rows
* @param {string} target
* @returns {string[]}
*/
export function manifestChecklist(rows, target) {
const placement =
"every runtime dependency `core/` imports is declared in the **repo-root** `package.json` and nowhere else ([Dependency placement](https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement))";
return needsManifestEdit(rows, target)
? [
`- [ ] Bump the version(s) in the repo-root \`package.json\` — ${placement}. The four \`typescript-sdk\` packages are pinned **exactly**, so they move together.`,
"- [ ] `npm install` at the root, and commit the refreshed lockfile.",
]
: [
`- [ ] **No manifest edit needed** — the declared range already admits ${target}, so this is a lockfile refresh. (${placement}, so if that ever stops being true the bump belongs there.)`,
"- [ ] `npm install` at the root, and commit the refreshed lockfile.",
];
}
/**
* @param {NonNullable<ReturnType<typeof groupState>>} state
* @returns {string}
@ -383,8 +433,7 @@ export function buildIssueBody(state) {
"",
"### Upgrade checklist",
"",
"- [ ] Bump the version(s) in the **repo-root** `package.json` — every runtime dependency `core/` imports is declared there and nowhere else ([Dependency placement](https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement)). The four `typescript-sdk` packages are pinned **exactly**, so they move together.",
"- [ ] `npm install` at the root, and commit the refreshed lockfile.",
...manifestChecklist(rows, target),
"- [ ] Re-check the bundler `external` lists (`clients/{cli,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`) if the release adds or renames an entry point; `npm run verify:bundle-externals` enforces this against the built output.",
"- [ ] `npm run format`, then `npm run local:gate`.",
"",

199
scripts/sdk-watch.test.mjs
View file

@ -30,6 +30,7 @@ import {
installedVersion,
isSweepAuthored,
main,
needsManifestEdit,
parseMarker,
parseSupersededMarker,
pickMilestone,
@ -173,60 +174,122 @@ test("isSweepAuthored requires both the automation author and the sweep's labels
assert.equal(isSweepAuthored({}), false);
});
test("the job the model runs in holds no write permission", () => {
/** The parsed workflow, read from disk so the tests assert the shipped file. */
function workflowDoc() {
return YAML.parse(
readFileSync(
new URL("../.github/workflows/sdk-watch.yml", import.meta.url),
"utf8",
),
);
}
const analyzeJob = () => workflowDoc().jobs.analyze;
const analyzeClaudeArgs = () =>
analyzeJob().steps.find((s) =>
String(s.uses ?? "").startsWith("anthropics/claude-code-action"),
).with.claude_args;
test("no model runs in a job that can write", () => {
// ⚠️ GitHub scopes permissions per JOB, not per step. While the model action
// and the `gh issue comment` shared a job, the `issues: write` the posting
// needed was on the token handed to the model — however carefully the step was
// written (Copilot). The separation is the control; this test is what keeps a
// later edit from quietly undoing it by merging the jobs or widening a scope.
const workflow = YAML.parse(
readFileSync(
new URL("../.github/workflows/sdk-watch.yml", import.meta.url),
"utf8",
),
);
const analyze = workflow.jobs.analyze;
const runsModel = (analyze.steps ?? []).some((s) =>
String(s.uses ?? "").startsWith("anthropics/claude-code-action"),
);
assert.ok(runsModel, "the analyze job is the one that runs the model");
assert.deepEqual(
analyze.permissions,
{ contents: "read" },
"the model's job must hold contents: read and nothing else",
);
assert.equal(
(analyze.steps ?? []).some((s) => /gh issue comment/.test(s.run ?? "")),
false,
"posting belongs in the separately-permissioned job",
);
// written (Copilot, round 4). The separation is the control; this is what keeps
// a later edit from undoing it by merging the jobs or widening a scope.
//
// ⚠️ Stated over EVERY job, not just `analyze`. `sweep` is write-capable too —
// filing issues is its purpose — so "only one job can write" was never the
// invariant, and asserting it would have been asserting something false
// (Copilot, round 5). What must hold is that no write-capable job runs a model.
const workflow = workflowDoc();
const inherited = workflow.permissions ?? {};
let modelJobs = 0;
for (const [name, job] of Object.entries(workflow.jobs)) {
const runsModel = (job.steps ?? []).some((s) =>
String(s.uses ?? "").startsWith("anthropics/claude-code-action"),
);
if (!runsModel) continue;
modelJobs += 1;
// A job with no `permissions:` block inherits the top-level one, which here
// includes `issues: write` — so an omitted block is a failure, not a default.
const permissions = job.permissions ?? inherited;
const writes = Object.entries(permissions).filter(
([, level]) => level === "write",
);
assert.deepEqual(
writes,
[],
`job "${name}" runs a model and holds write scope: ${JSON.stringify(permissions)}`,
);
assert.equal(
(job.steps ?? []).some((s) => /gh issue comment/.test(s.run ?? "")),
false,
`job "${name}" runs a model and also posts`,
);
}
assert.equal(modelJobs, 1, "expected exactly one job to run the model");
});
test("the model is granted no command that can reach an arbitrary host", () => {
// `npm view` accepts `--registry=<URL>` and a Bash grant matches only a
// PREFIX, so granting it was an outbound channel no output scan can see
// (Copilot). The general rule — check a command's flag surface before granting
// it — is in AGENTS.md; this pins the specific instance.
const workflow = YAML.parse(
readFileSync(
new URL("../.github/workflows/sdk-watch.yml", import.meta.url),
"utf8",
),
test("the model's tool availability is restricted to reading, with no shell", () => {
// ⚠️ Three grants in a row turned out to have a wider flag surface than they
// looked (`gh api`, `gh issue comment --body-file`, `npm view --registry`), and
// a `Bash(...)` rule can match a COMPOUND command besides. The resolution was
// to remove the capability rather than narrow it a fourth time (Copilot).
//
// ⚠️ `--tools` is what RESTRICTS availability; `--allowedTools` only
// pre-approves, so a tool another settings file permits stays reachable if
// only the latter names it. This repo learned that in `skill-eval.mjs` first.
const args = analyzeClaudeArgs();
const tools = /--tools\s+"([^"]*)"/.exec(args)?.[1] ?? "";
assert.deepEqual(
tools
.split(",")
.map((t) => t.trim())
.sort(),
["Glob", "Grep", "Read"],
"--tools must enumerate exactly the three read-only tools",
);
const args = workflow.jobs.analyze.steps.find((s) =>
String(s.uses ?? "").startsWith("anthropics/claude-code-action"),
).with.claude_args;
const allowed = /--allowedTools\s+"([^"]*)"/.exec(args)?.[1] ?? "";
assert.ok(allowed.length > 0, "expected an --allowedTools whitelist");
for (const forbidden of ["npm", "curl", "wget", "WebFetch", "WebSearch"]) {
assert.ok(allowed.length > 0, "expected --allowedTools to pre-approve them");
for (const forbidden of [
"Bash",
"npm",
"curl",
"wget",
"WebFetch",
"WebSearch",
]) {
assert.equal(
allowed.includes(forbidden),
false,
`--allowedTools must not grant ${forbidden}`,
);
assert.equal(
tools.includes(forbidden),
false,
`--tools must not make ${forbidden} available`,
);
}
});
test("the release notes are fetched by a step the model does not run in", () => {
// The model has no shell, so the notes must arrive some other way — if this
// step is ever dropped, the analysis silently degrades to guesswork.
const analyze = analyzeJob();
const fetch = (analyze.steps ?? []).find((s) =>
/upstream-release-notes\.md/.test(s.run ?? ""),
);
assert.ok(fetch, "expected a deterministic release-notes fetch step");
assert.equal(
fetch.uses,
undefined,
"it must be a plain run step, not a model",
);
});
test("the workflow posts the exact marker the sweep looks for", () => {
// ⚠️ The marker is duplicated across the script and the workflow because the
// posting step is shell, not JS. If the two ever drift, every issue reads as
@ -448,6 +511,64 @@ test("buildIssueBody tables every package in the group and marks which are behin
assert.ok(body.includes(`https://github.com/${SDK.repo}/releases`));
});
test("needsManifestEdit is true for an exact pin and false for a satisfied range", () => {
// The two cases this sweep actually watches. The SDK packages are pinned
// exactly, so any new version needs the manifest changed; `ext-apps` is a
// caret range that already admits the target, so only the lockfile moves.
assert.equal(
needsManifestEdit([{ declared: "2.0.0", behind: true }], "2.1.0"),
true,
);
assert.equal(
needsManifestEdit([{ declared: "^1.7.4", behind: true }], "1.8.0"),
false,
);
// A range that does NOT admit the target still needs the edit.
assert.equal(
needsManifestEdit([{ declared: "^1.7.4", behind: true }], "2.0.0"),
true,
);
// Unparseable declarations count as needing a look, not as needing none.
assert.equal(
needsManifestEdit([{ declared: "(undeclared)", behind: true }], "1.0.0"),
true,
);
// A package that is not behind does not drag the whole group into an edit.
assert.equal(
needsManifestEdit(
[
{ declared: "^1.0.0", behind: true },
{ declared: "2.0.0", behind: false },
],
"1.5.0",
),
false,
);
});
test("buildIssueBody tells ext-apps it needs no manifest edit", () => {
// ⚠️ The generated checklist used to say "bump the version in package.json"
// unconditionally, which is wrong for the one case this script goes out of its
// way to model separately: `^1.7.4` already admits 1.8.0, so it sends the
// maintainer to change a line that is already correct (Copilot).
const extBody = buildIssueBody(
groupState(EXT, {
"@modelcontextprotocol/ext-apps": {
declared: "^1.7.4",
installed: "1.7.5",
latest: "1.8.0",
},
}),
);
assert.ok(extBody.includes("No manifest edit needed"));
assert.equal(extBody.includes("Bump the version(s)"), false);
// ...and the exactly-pinned SDK still gets told to edit it.
const sdkBody = buildIssueBody(behindState());
assert.ok(sdkBody.includes("Bump the version(s)"));
assert.equal(sdkBody.includes("No manifest edit needed"), false);
});
test("buildIssueBody escapes a pipe so one value cannot break the table apart", () => {
const state = groupState(EXT, {
"@modelcontextprotocol/ext-apps": {