3,887 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
fad8480376 |
fix: only attribute a decode rejection to a Protocol entry
markResponseRejected was called for ANY refresh failure. The id it recovers is "the last response received for this method", which is the failing exchange only when a response actually arrived and was refused while decoding. A transport drop, a timeout, or an abort produces no response frame at all — the last-answered id then still points at an EARLIER successful call, so marking it stamped "Rejected by the Inspector" onto an exchange that worked. That is the same class of lie this issue exists to remove. A server-sent JSON-RPC error is excluded for a different reason: the id would be right, but the failure is the server's and its entry already renders as an error from the error frame. Gated on SdkErrorCode.InvalidResult / UnsupportedResultType — decisions the client made about a frame in hand. The failure is still recorded as list state either way; only the Protocol attribution is gated. Caught in Copilot's review of #1953. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
1e427c05f3 |
fix: mark a rejected standalone response entry as an error too
extractStatus returned none for any non-request entry, so a clientError annotation that landed on a standalone response frame — the fallback messageLogState uses when no request entry was there to fold into, e.g. a trimmed log or a reconnect boundary — rendered with no status badge at all. The clientError check now runs before the request-only lifecycle. Caught in Copilot's review of #1953. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
f201032137 |
fix: surface list-fetch failures instead of swallowing them
A list load that failed — a transport error, or a result the SDK's era codec rejects as invalid — reported nothing. The connect-time refresh is fired with no caller to await it (`void this.refresh()`), so its rejection became an unhandled rejection: the header said "Connected", the panel rendered an empty list indistinguishable from a server that has none, and the failed exchange showed in the Protocol panel as a clean success. Two surfaces now tell the truth: - ManagedListState records the failure as observable state (`getError` + an `errorChange` event) and still re-throws, so App's auth-recovery wrapper keeps working. The four `useManaged*` hooks expose it via a shared `useManagedListError`, and the Tools/Prompts/Resources sidebars render a `ListLoadError` alert with the reason and a Retry. - A response the client refused is attributed back to its Protocol entry (`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`), which now renders an Error status and a "Rejected by the Inspector" alert rather than the green resultType badge alone. The SDK gives no request id with a decode failure, so the id is recovered by correlation; see the comment on `markResponseRejected` for why that is exact. Also stops `listAllTools` from dropping a failing excluded-tools walk on the floor — it stays non-fatal but is logged. Closes #1953 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
9644ac7fe8 |
fix: prime SSE streams so Firefox resolves the events fetch
Firefox does not hand a streaming `fetch()` response to JS until the first *body* byte arrives; Chromium resolves the promise as soon as the headers do. Both SSE endpoints flushed headers immediately and then stayed silent until there was something to report, which deadlocked `/api/mcp/events`: `RemoteClientTransport.openEventStream()` awaits that fetch *before* the MCP client sends `initialize`, so no `initialize` -> no event to report -> no body byte -> the fetch never resolves. The web UI hung on "Connecting..." forever with nothing in the UI, the console, or the node logs. Write an inert `:` SSE comment the instant each stream opens. Conforming parsers ignore a comment line, so this unblocks the read without inventing a wire event and needs no client-side protocol change. `X-Content-Type-Options: nosniff` was also proposed on the issue but does not fix it -- verified against Firefox 153, the fetch still never resolves with the header set and no body byte. The delay is not MIME sniffing. The priming write is deliberately ordered *after* each handler registers its consumer (the session event consumer; the file-watch subscriber plus `ensureWatcher()`). Callers treat the arrival of the stream's first bytes as proof they are subscribed, so priming first would hand out that proof across an `await` and drop an edit made in the gap. `/api/servers/events` gets the same treatment: its fetch was equally stuck on Firefox until the first real change event, which for an unedited `mcp.json` never comes. Its client-side reader counted any `\n\n` as a change, so the inert frame would have fired a spurious background re-fetch on every connection -- it now skips frames carrying no `event:`/`data:` field. The transport's own `parseSSE` already drops comment frames correctly and needed no change. Verified end to end in real Firefox against a live streamable-HTTP test server: before, stuck on "Connecting..." past 25s; after, initialize -> notifications/initialized -> tools/list, connected. Closes #1858 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
Cliff Hall
|
097bfafdc1 |
Merge pull request #1934 from modelcontextprotocol/v2/fix/package-sandbox-proxy
fix: ship clients/web/static so the MCP Apps sandbox proxy loads |
||
|
cliffhall
|
5401e527e6 |
fix: handle spawn failure of the MCP test server in smoke:web:app
Copilot's third pass (suppressed comment) caught the remaining hole in the
child-process handling: `spawn()` reports a failure to start via an `error`
event, not `exit`. With no `error` listener Node throws it uncaught, replacing
the smoke's diagnostic with a raw stack — and because `exit` never fires in that
case, the readiness poll would otherwise spin for the full 30s before reporting
a timeout that misattributes the cause. `close` is now listened to alongside
`exit` for the same reason: it fires in cases `exit` does not.
prod-web-server.mjs already documents this exact hazard for the launcher child;
this brings the test-server child in line.
Both new branches verified by hand:
- unspawnable executable → "could not spawn the MCP test server (…): spawn
/nonexistent/node-binary ENOENT", immediately, instead of an uncaught throw
- missing module → "MCP test server exited early" with the loader stack, fast,
with no 30s hang
Full `npm run ci` green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
|
||
|
cliffhall
|
556aaca298 |
fix: fail smoke:web:app on async crashes, not just sync page errors
Copilot's second pass (suppressed comments) caught a real gap: the smoke listened only for `pageerror`, which is the *synchronous* half of the uncaught-crash class. Its async twin — an unhandled rejection, or a failed dynamic import (this app lazy-loads chunks) — is not a `pageerror`; Chromium reports it on the console channel. So an async crash during the app-open path could leave the smoke green. smoke-web-browser.mjs already solved this; this now mirrors it exactly — same FATAL_CONSOLE pattern, same split between hard failures and diagnostics, so benign noise (a font-CDN miss, a React warning) still can't flake CI. Fatal console errors are now included both in the catch-block diagnostics and in the final assertion, which is where they were missing. Confirmed working on a passing run: a benign 409 subresource error is reported as a non-fatal diagnostic rather than failing the smoke. Full `npm run ci` green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F |
||
|
cliffhall
|
3f4f68699e |
fix: give smoke:web:app its own port; note the App config's era in the table
Copilot review on #1934, both comments actionable: - smoke:web:app defaulted to 6299 — the same port smoke:web uses — while its own comment claimed the port was "distinct … so back-to-back runs can't collide". The three web smokes run sequentially in `npm run smoke`, so it passed, but a slow teardown, a TIME_WAIT socket, or a parallel run would EADDRINUSE it. Moved to 6297 (smoke:web 6299, smoke:web:browser 6298) and rewrote the comment to name the actual values rather than assert distinctness. - The showcase table tells readers to connect with Protocol Era = Modern unless noted, and mcp-app-http.json is legacy-only. That was stated in the prose below but not in the row, so a reader scanning the table would try Modern first. Marked the row. Verified: smoke:web:app passes standalone on 6297, and the full `npm run smoke` chain passes with all three web smokes on distinct ports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F |
||
|
cliffhall
|
cf0ed4ac78 |
test: add smoke:web:app — drive a real MCP App end to end in headless Chromium
`smoke:web:browser` stops at first paint and never connects to a server, so everything downstream of the connect — the Apps tab, the sandbox controller, the UI-protocol bridge — was unexercised by any smoke. That is the code #1859 broke, and nothing would have caught a regression in it. smoke:web:app boots the same prod --web server, spawns the mcp-app-http.json composable server, and drives connect → open app → widget ready through one deep-link navigate. The assertion is the documented data-app-status="ready" contract, which the renderer only reports once the widget has loaded inside the sandbox iframe AND completed its bridge handshake — so a single attribute covers the proxy being served, the UI resource loading, and the handshake. Two mechanics found by running it, both silent-failure shaped: - server-composable.ts announces readiness on stderr, not stdout, so watching stdout alone times out with an empty diagnostic. Both streams are scanned. - the bound port is not the configured one — createTestServerHttp resolves via findAvailablePort(), which walks upward when the port is taken. The smoke parses the announced URL instead of assuming 3130. Scope: this runs against the repo build tree like every other smoke, so it would NOT have caught #1859 itself (a packaging failure; the file is always present in-repo). pack:verify owns that dimension — the two are complements, and the header comment says so. A cheap structural pre-check does assert the proxy page sits where sandbox-controller.ts resolves it, so a move/rename fails fast. Both failure modes verified by hand: with clients/web/static removed, the pre-check fires; with the pre-check bypassed, the data-app-status assertion catches it independently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F |
||
|
cliffhall
|
28a21c5e08 |
test: add an MCP App showcase config to reproduce and verify #1859
The `mcp_app_demo` tool and `mcp_app_demo_widget` UI resource presets already existed in the preset registry, but no config wired them into a server — so there was no runnable way to reach a rendered MCP App, and therefore no way to exercise the sandbox proxy path that #1859 broke. `mcp-app-http.json` composes the two over plain streamable-HTTP. With it, the Apps tab renders a real widget and the packaging bug is reproducible on demand: without clients/web/static in the tarball the widget area shows "Sandbox not loaded: ENOENT … /clients/web/static/sandbox_proxy.html" instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F |
||
|
cliffhall
|
22da48c652 |
fix: ship clients/web/static so the MCP Apps sandbox proxy loads
`clients/web/static/sandbox_proxy.html` is a committed source file, not a build artifact, and it was never listed in the root package.json "files" allowlist. `sandbox-controller.ts` reads it at runtime as `<runner dir>/../static/sandbox_proxy.html`, so it resolved fine in-repo but was absent from every published tarball — the read threw and the controller served its "Sandbox not loaded" fallback page, breaking the Apps tab for anyone running `npx @modelcontextprotocol/inspector`. v1 shipped the equivalent as `server/static` in both the root and the server workspace "files" lists; that entry has no counterpart in v2. Add `clients/web/static` to the allowlist, and assert it in pack:verify twice over — once in the tarball packlist and once on disk after install, since what the runtime needs is the path *relative to* clients/web/build rather than mere presence in the tarball. No .npmignore change is needed: clients/web/.gitignore does not list `static`, so the nested-gitignore packlist hazard that hid `build/` does not apply here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F |
||
|
cliffhall
|
d0b683d4bf |
docs: require a type label (bug/enhancement/documentation/chore/question)
The version label says which line work belongs to; nothing said what KIND of work it is. In practice that meant reaching for bug or enhancement and pressing everything else into one of them — a migration guide and a tsup->tsdown swap both landing as "enhancement", which degrades the label to "not a bug" and makes filtering by it meaningless. Every issue created or triaged now carries exactly one of bug, enhancement, documentation, chore, or question, independent of its v1/v2 label. Adds a table of what each is and is not for, and states the anti-pattern explicitly so the two familiar labels don't absorb the other three. A PR needs no type label — it is classified through the issue it closes, the same way it is tracked through that issue's board card. Wires the requirement into the three places that would otherwise drift: the "not created until" callout, triage pass 1 (an outside reporter can set neither label, so both are applied in the same step), and the board audit, which gains an "open, no type label" check and a matching row in the invariant table. Note `chore` did not exist as a repo label and was created for this; the existing `dependencies` label is Dependabot's and scoped to PRs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
cliffhall
|
a9238426a6 |
docs: Done means shipped — remove, don't park, issues closed any other way
A board card in Done asserts that the work shipped, and Done is read as the record of what a milestone actually delivered. Nothing said so, so an issue closed as a duplicate or not-planned was being moved to Done like any other close — which makes that record wrong in a way no later reader can detect: counting Done cards can no longer tell a shipped fix from a report closed as a duplicate of one. New rule: exactly two things earn a card a place in Done — its PR merged, or it is a parent whose last sub-issue closed. Every other close (duplicate, won't fix, not planned, obsolete, superseded) means nothing shipped, so the card is deleted instead. Deleting a card touches only the board; the issue keeps its labels and comments and stays searchable, so nothing is lost. Also notes that the close reason is the machine-readable form of the same distinction, and that `gh issue close --reason` cannot express `duplicate` — it accepts only completed/not planned, so duplicate must be set through the API or the web UI's "Mark as duplicate" (which also records a duplicate-of link). Adds the invariant to the board audit as a tenth check. That required the audit to read closed issues too, so its fetch moves to --state all with stateReason, `open(n)` becomes a state test rather than "present in the map", and --limit must now clear the repo's total issue count, not just the open ones. The two wrong-board label checks are scoped to open issues in the same pass — over all history they flag four long-closed cards whose labels nobody intends to rewrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
cliffhall
|
04f1a56f93 |
docs: make triage runnable — state-based test, both-board sweep, board audit
Follow-up on the same issue: the triage rules were clear on the happy path but three gaps meant a bare "triage new issues" would diverge from what a maintainer actually does. Define triage by STATE, not authorship. An issue needs triage when it arrives with no board card and no milestone — whoever filed it. Framing it as "externally-filed" was wrong: a maintainer opening an issue by hand in the GitHub UI lands in exactly the same state, because write access makes the board reachable, not automatic. What puts an issue in Todo is somebody performing the approval, not who owns the account. Renamed the section and reworded every dependent passage accordingly. Fix the discovery query, which diffed open issues against board #28 only. Verified against the live boards: it reports #1929 as unboarded when that issue is correctly carded on #11, so following it literally double-boards the issue — recreating a defect a past sweep introduced. Now diffs against the union of both boards, filtered by .content.repository (an org project can hold other repos' issues; #11 carries one from modelcontextprotocol/ servers), and prints each hit's destination, since an unboarded issue that already has a milestone is approved and belongs in Todo, not Incoming. Add "The board audit" — the other drift classes a single-issue rule can't catch (double-boarded, non-Issue items, statusless cards, the Incoming ⇔ milestone invariant in both directions, wrong board for label, missing version label, missing Priority), as a table of invariant + fix plus one runnable jq check that should print 0 across the board. Both snippets were executed verbatim against the live boards. Add "Recording the score". The rubric claimed its reasoning "survives in a form someone can argue with later", but the board stores only the result and is private, so nothing kept that promise. Triage now posts the axes, the bonuses claimed, and the total as a comment. Also scope pass 2 explicitly to a human — deciding what ships in which release is not inferable from a rubric — and correct the stale note about three statusless cards on #11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
cliffhall
|
943f37a40a |
docs: make assigning a milestone the act that moves a card to Todo
AGENTS.md had no rule linking a milestone to a board Status, and its two existing rules contradicted each other: every new issue was to be milestoned at create time, yet new items were to start in Incoming, with Todo reserved for work a maintainer had approved. If every issue is milestoned at birth, "has a milestone" carries no approval information and nothing records the moment of sign-off. Make assigning a milestone the approval act, giving one invariant: Incoming <=> no milestone; everything past it <=> milestoned. External reports and maintainer-filed issues then differ only in whether approval has already happened -- an outside reporter cannot set a label, milestone, or board field, so their issue arriving with none of them is normal rather than a defect to fix on arrival. - scope the "not created until five things" callout to maintainer-filed issues - split the Status bullets into the two cases - scope the milestone mandate to issues you create, stating the external exception explicitly - add a "Triaging externally-filed issues" section: the two passes, plus a gh/jq snippet that diffs open issues against the board to find unboarded ones - reword both prose descriptions of the Incoming/Todo line - reword the rubric's "Linked to a milestone" bonus as "already approved" and note it is the re-scoring case only; under the old rules it was a constant offset on every issue, shifting every band boundary down by one - update both gh recipes (#28 and #11) to default to Todo, with an Incoming variant for triage No mirror to .github/copilot-instructions.md: board and milestone mechanics are on that file's deliberately-absent list. Closes #1930 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
Cliff Hall
|
c7bccd477d |
Merge pull request #1903 from modelcontextprotocol/v2/chore/milestone-merge-v2.1.0
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
chore: merge v2/main for the v2.1.0 milestone release |
||
|
cliffhall
|
dedee5afb5 |
chore: 2.1.0
Bumps the root package.json (and its lockfile entries) 2.0.0 -> 2.1.0 for the v2.1.0 milestone release. Only the ROOT manifest carries a version -- the four clients/* packages deliberately have none, since the Inspector ships as one tarball with a single version number that every Node client resolves through readInspectorVersion() in core/node/version.ts. So this is the only file that needs changing. Done with `npm version 2.1.0 --no-git-tag-version`: the tag is NOT created here. #1903 merges as a merge commit, so this commit lands on main intact and the release tag is pushed against it after the merge. Creating the tag now would point it at a commit that is not yet on the release branch. The publish job asserts the release tag matches this version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
Cliff Hall
|
07a2b4bdfd |
ci: sync the release workflow on v2/main with main's verbatim (#1902) (#1904)
main's copy of .github/workflows/main.yml carried three release fixes v2/main never took -- #1831 (least-privilege default GITHUB_TOKEN scope), #1834 (derive the npm dist-tag from the version) and #1836 (publish via npm OIDC trusted publishing). Diffing the file both directions shows v2/main has NOTHING main lacks: its only unique content is the superseded publish step, run: npm publish --access public --provenance env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} which is exactly what #1836 replaced. Every other line already matched. So this is a verbatim copy of main's file rather than a hunk-by-hunk forward-port -- fewer ways to get it wrong, and it leaves the two branches byte-identical (`git diff origin/main -- .github/workflows` is now empty). Not a back-merge: only this one file is taken, so none of main's pre-swap v1 lineage enters v2/main's ancestry (see #1868). Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
6233d8bb22 |
chore: merge v2/main for the v2.1.0 milestone release
Resolves the eight conflicts from the first v2/main -> main milestone merge on a branch cut from main, so v2/main's ancestry stays clean -- resolving them on the PR head would have back-merged main into v2/main, which is exactly what #1868 was closed to avoid. All eight took v2/main's side, each verifiably the newer content: - README.md stale Mcp-Param-* callout; v2/main's is corrected by #1847 (the Inspector builds the headers itself) - SECURITY.md add/add (#1843 on main, #1867 on v2/main); differ by one line, the CONTRIBUTORS.md -> CONTRIBUTING.md rename from #1884 - clients/web/package.json vitest ^4.1.0 -> ^4.1.10, the #1899 fix - the five package-lock.json files, then regenerated from a clean install rather than hand-merged main's release-workflow commits (#1831 least-privilege token, #1834 dist-tag derivation, #1836 OIDC trusted publishing) auto-merged and are preserved -- .github/workflows/main.yml is the only file where the merged tree differs from v2/main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU |
||
|
Cliff Hall
|
834fd2d77d |
chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1900)
* chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1838) eslint 9 pulled `minimatch@3` → `brace-expansion@1.1.16`, which carries GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion length). It appeared in all five lockfiles — the root plus each of the four clients, none of which is an npm workspace, so all five had to move together. Deferred from the pre-2.0.0 sweep because it is dev-scope, not attacker- controlled, and a major bump across five packages was the wrong thing to attempt immediately before an irreversible publish. This is that work. ## What moved - `eslint` `^9.39.4/5` → `^10.8.0` and `@eslint/js` → `^10.0.1`, in all five manifests. - `eslint-plugin-react-hooks` `^7.0.1` → `^7.1.1` in web and tui. Required, not cosmetic: 7.0.1's peer range stops at `^9.0.0`, so installing it alongside eslint 10 fails `ERESOLVE`. - Everything else already declared eslint 10 support and stayed put: `typescript-eslint@^8.56.1` (`^8.57 || ^9 || ^10`), `eslint-plugin-react-refresh@^0.5.2` (`^9 || ^10`), `eslint-plugin-storybook@^10.2.19` (`>=8`). ## Config migration None was needed. All five configs were already flat (`eslint.config.js` with `defineConfig`/`globalIgnores` from `eslint/config`), which is the format v10 keeps and the only one it still supports. The root config's `core/` and shared- surface blocks — including the `_`-prefix `argsIgnorePattern` / `varsIgnorePattern` / `caughtErrorsIgnorePattern` — are unchanged and still gate `lint:core` and `lint:shared`. No rule was disabled, downgraded, or scoped away to make the new eslint pass. v10 surfaced 21 real violations across three rules; all 21 are fixed in the code. ### `preserve-caught-error` (new in `eslint:recommended`) — 15 sites A `throw new Error(msg)` inside a `catch` silently drops the original error. Each now passes `{ cause }`, so the underlying failure survives into the chain. Messages are unchanged, so nothing that asserts on them moves. 12 in `core/`, 2 in `clients/cli`, 1 in `test-servers/src`. ### `no-useless-assignment` (new in `eslint:recommended`) — 2 sites `clients/cli/src/cli.ts`'s `optionArgs` and `test-servers/src/test-server-oauth` 's `valid` were initialized and then unconditionally overwritten on every path. Both are now declared without the dead initializer; TypeScript's definite- assignment analysis covers the remaining paths. ### `react-hooks/set-state-in-effect` — 8 sites (web) These were false negatives the plugin fixed in 7.1.0, not a rule we newly opted into: `set-state-in-effect` has been `error` in `flat/recommended` since 7.0.1, and the web config has always extended it. Every one was the same anti-pattern — resetting or re-syncing local state from a prop inside a `useEffect`, which paints the stale value for one frame and then renders again to correct it. All eight now use a new `useValueChange(value, onChange)` hook (`clients/web/src/hooks/useValueChange.ts`), which is React's documented "adjusting state during render": compare against the previous render's value with `Object.is` and call back *during* render, so React discards the in-progress output before it reaches the DOM. - `MrtrConversation`, `ProtocolEntry`, `TaskCard` — mirror `isListExpanded`. - `NetworkEntry` — same mirror, plus the "Reveal in Network" force-open. The reveal's rAF scroll stays an effect (real external-system work); only the `setIsExpanded(true)` moved. `isExpanded` is now seeded from `isListExpanded || revealed`, since a render-time sync fires on change and so cannot cover an entry that mounts already revealed. - `PromptArgumentsForm`, `ResourceTemplatePanel` — reset on template/prompt switch. - `ServerConfigModal` — reset on open. Keyed on `opened ? initial : undefined`, which collapses "opened flipped" and "initial changed while open" into one value; `initial` is already memoized, so it cannot thrash. `useValueChange` has its own test file, and `ServerConfigModal` gains an open→edit→close→reopen test (the close leg was previously untested, which is what left the new guard uncovered). ## Advisory delta `npm audit`, per package, before → after: | Package | before | after | | ------------------- | -------------------------- | -------------------------- | | root | 9 (1 low, 6 mod, 2 high) | 8 (1 low, 6 mod, 1 high) | | `clients/web` | 19 (3 low, 6 mod, 6 high, 4 crit) | 17 (3 low, 6 mod, 4 high, 4 crit) | | `clients/cli` | 6 (2 low, 4 high) | 4 (2 low, 2 high) | | `clients/tui` | 4 (1 low, 3 high) | 2 (1 low, 1 high) | | `clients/launcher` | 3 (3 high) | 1 (1 high) | `brace-expansion` is gone from all five — `eslint@10` resolves `minimatch@10` → `brace-expansion@5.0.9`. Dropping `@eslint/eslintrc` (removed in v10) also took `js-yaml` with it, clearing GHSA-52cp-r559-cp3m (high) from all four clients as a bonus. The remainder are unrelated and tracked separately. `npm run ci` passes end to end — validate, the per-file ≥90 coverage gate, the build gate, all five smokes (including `smoke:tui`, run locally on a real TTY), and 462 Storybook tests. Also documents the effect rule in AGENTS.md and mirrors it into `.github/copilot-instructions.md`, since it is now something a reviewer cites. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: state useValueChange's referential-stability requirement (#1838) Copilot review on #1900: the hook compares with `Object.is`, so an object or array literal rebuilt in the component body compares unequal every render. Because the callback is what updates state, that is not merely a wasted render — it is an infinite loop. Every current caller passes a primitive (`boolean`, `string`) or an already- memoized value, so nothing changes behaviorally. But the hazard was implicit, and the next caller is the one it would bite. Documented on the hook itself, and one clause added to the AGENTS.md rule with its mirror in `.github/copilot-instructions.md`. No runtime guard: the requirement is the same one a `useEffect` dependency array carries, and here the failure is loud and immediate rather than silent. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): stop asserting the --tool-args-json catch binding is an Error (#1838) Copilot review round 2 on #1900: the `--tool-args-json` parse handler formatted its message with `(e as Error).message`. A `catch` binding is `unknown`, so that cast is an assertion rather than a check — a thrown non-Error would render `--tool-args-json is not valid JSON: undefined`, which points the user at nothing. `JSON.parse` does only throw `SyntaxError` today, so this is not a live bug. But the cast is the kind AGENTS.md asks us not to leave unjustified, the same handler was already being touched here to add `{ cause: e }`, and every other site in this PR uses the guarded form. Now it does too. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: require useValueChange's onChange to be render-pure (#1838) Copilot review round 3 on #1900: `onChange` is called during render, so it is bound by render-phase purity — and the web client mounts under `StrictMode`, which deliberately double-renders in development. A callback doing anything external would run an unpredictable number of times, and concurrent React can abandon an in-progress render entirely. Every current caller already passes a callback that only calls `setState`, so nothing changes. But "runs during render" is the whole point of the hook and the constraint that follows from it was left implicit. Stated on the hook, with the same clause in the AGENTS.md rule and its `.github/copilot-instructions.md` mirror. All three now point at `NetworkEntry` as the worked example of the split: the reveal's force-open is a state update and lives in `useValueChange`, while its `requestAnimationFrame` scroll stays a `useEffect`. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
23e0c39d11 |
chore(deps): untangle the vitest/Storybook exact-pin knot to clear GHSA-p63j-vcc4-9vmv (#1839) (#1899)
GHSA-p63j-vcc4-9vmv (**critical**) — `@vitest/browser` Browser Mode provider commands accept a file path from the browser and act on it without checking the `allowWrite` permission gate. Patched in 4.1.10; `clients/web` was pinned at 4.1.0. Dev-scope only — `@vitest/*` is absent from the published `files` allowlist, and Browser Mode here runs this project's own Storybook play functions, never a third-party page. That is why #1837 deferred it rather than attempting a toolchain upgrade immediately before an irreversible publish. ## The knot, and what actually unties it `^4.1.0` already admits 4.1.10, so the declared ranges were never the problem. The family is welded by *exact* peer pins — `vitest` pins `@vitest/browser-playwright`, which pins `@vitest/browser` — so nothing moves alone. The issue predicted that an explicit coordinated `npm i -D` would fix it. It does not. With `clients/web/package-lock.json` in place, npm ERESOLVEs even when every member of the family plus the Storybook packages are named in one command and `node_modules` is deleted first: Found: @storybook/addon-a11y@10.2.19 Could not resolve dependency: dev @storybook/addon-a11y@"^10.5.5" Conflicting peer dependency: storybook@10.5.5 npm resolves against the existing lock tree and will not move the whole constellation at once. **Regenerating `clients/web/package-lock.json` is the only thing that unties it** — a clean resolve places all four at 4.1.10 with no peer complaints at all. ## Versions vitest 4.1.0 -> 4.1.10 @vitest/browser 4.1.0 -> 4.1.10 (transitive) @vitest/browser-playwright 4.1.0 -> 4.1.10 @vitest/coverage-v8 4.1.0 -> 4.1.10 storybook + @storybook/* 10.2.19 -> 10.5.5 eslint-plugin-storybook 10.2.19 -> 10.5.5 @chromatic-com/storybook 5.0.1 -> 5.2.1 `clients/{cli,tui,launcher}` were already resolving 4.1.9 (no browser pins, so they floated freely) and were never vulnerable. Their declared `^4.1.0` still *permitted* 4.1.0 on a fresh resolve, so the floor is raised to `^4.1.10` in all four manifests — same reasoning as the vite floor in #1841. Pinning only the lockfile would have left a regression path open. ## The vite 8 peer override Resolved, as a consequence rather than a workaround. `@joshwooding/vite-plugin-react-docgen-typescript@0.6.4` declared `peer vite@"^3 || ^4 || ^5 || ^6 || ^7"` against our vite 8. It is not a direct dependency — it arrives through `@storybook/react-vite`, which at 10.5.5 depends on `^0.7.0`, and 0.7.0 adds `^8.0.0` to that range. So taking Storybook 10.5.5 for the peer set drops the override too; nothing is forced. ## Two holds the regeneration forced, each with a tracked issue Regenerating the lock floats every other in-range dependency to current. Most of that is welcome — it is what takes `clients/web` from 19 advisories to 1 — but two floats broke the gate and are unrelated to this security fix: - **zod 4.3.6 -> 4.4.3** makes `clients/web`'s `tsc -b` die with `FATAL ERROR: Ineffective mark-compacts near heap limit` at the ~4GB default. Reverting zod alone makes it exit 0, so the attribution is unambiguous. Held at `~4.3.6`. See #1896. - **eslint-plugin-react-hooks 7.0.1 -> 7.1.1** enables `set-state-in-effect`, which fails `lint` on 8 pre-existing violations across 7 components. Fixing them is a real refactor with interaction-behavior consequences, not a suppression. Held at `~7.0.1`. See #1897. Both `~` pins are deliberate and temporary, and both issues say so — they are constraints to remove, not preferences. Storybook 10.5.5 also reports that `setProjectAnnotations` is applied automatically since 10.3, making `.storybook/vitest.setup.ts` redundant. Left in place: `./preview` carries the Mantine decorator and the a11y annotations drive the play-function assertions, and if automatic provisioning missed either, the 462 stories would render unthemed with a11y checks inert and very likely still pass. That silent-failure risk does not belong in a security bump. See #1898. ## Audit delta `clients/web`, where the advisory lived: before 19 (4 critical, 6 high, 6 moderate, 3 low) after 1 (1 low — esbuild-in-tsup, Windows dev server only) All four critical entries — `vitest`, `@vitest/browser`, `@vitest/browser-playwright`, `@vitest/coverage-v8` — are gone. The other 14 are transitive dev-tree advisories (hono, ws, js-yaml, flatted, brace-expansion, ...) cleared as a side effect of the clean resolve. Elsewhere: cli 6 -> 5, launcher 3 -> 2, tui 4 -> 4, root unchanged at 9 (its lock is untouched). Those remainders are pre-existing and unrelated. ## Verification `npm run ci` green end to end on the upgraded stack — the point of caring here, since this *is* the test infrastructure: - coverage gate (>=90 per file, all four dimensions) passes on all four clients; no file regressed, so no gate was lowered and no `v8 ignore` added - 4802 web tests, 304 cli, 282 tui, 5 launcher - 462 Storybook play functions across 109 files, on vitest 4.1.10 Browser Mode - verify:build-gate, and all five smokes including `smoke:tui` (real TTY here) Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
0742f78357 |
docs: replace the markdown bug template with GitHub issue forms (#1894)
* docs: replace the markdown bug template with issue forms (#1844) Issues are the only intake channel now that external pull requests are off (#1820), so the chooser is where the contribution policy has to be stated — GitHub gives a would-be contributor no explanation when the "New pull request" button isn't there. The one existing template was a legacy **markdown** template, whose fields are prose headings a reporter can delete or ignore. Most reports still arrive missing the client and the transport, which are the two facts that decide where to look. Issue forms enforce them: this replaces `bug_report.md` with `1-bug_report.yml`, where client (web/cli/tui/core), version line (v1/v2), Inspector version, Node version, transport, MCP server, repro, expected, and actual are all `validations.required`. It auto-labels `bug`, and its intro says maintainers implement the fix, so a prompt plus screenshots beats a diff — with a field for exactly that. Adds `2-feature_request.yml` (auto-labels `enhancement`), which puts the problem statement first and marks the solution optional, since the problem is what survives when a specific solution turns out not to fit. Neither form applies a `v1`/`v2` label: GitHub can't map a form answer to a label, so the version line is a required dropdown and a maintainer still labels at triage. v1 takes security fixes only, so routing that answer early is the point. `config.yml` disables blank issues and carries the contact links. **There is deliberately no security *template*** — a template still opens a public issue, which is precisely what a vulnerability report must not do. The redirect is a contact link straight to the private advisory form (`/security/advisories/new`, verified enabled on this repo) plus one to `SECURITY.md`, which covers the v1 line too. The remaining links deflect the recurring misfiled classes: the specification repo and the TypeScript SDK for reports that aren't about this tool, the docs site, the contribution policy, and `#inspector-dev`. Also fixes three links the #1884 rename left dangling — `SECURITY.md` and the PR template still pointed at `CONTRIBUTORS.md`, which no longer exists on this branch. A policy nobody can open is the same as no policy. Validated against GitHub's issue-forms schema (unique ids, allowed `type`s, `markdown` blocks carrying no `id`/`validations`, `checkboxes` marking `required` per option rather than under `validations`). Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: address Copilot review on the issue forms (#1844) - PR template said `v2` targets `main`; the v2 base branch is `v2/main`. Correct on its face, and the one line in that template a reader acts on. - Scoped the Node floor on the bug form to v2, since the form serves both lines and v1's floor is older — reading `>= 22.19.0` as universal would make a legitimate v1 report look out of support. - Both the CONTRIBUTING.md paragraph and the AGENTS.md section now say the chooser is served from the **default branch**, so neither claims a form added on `v2/main` is live before the milestone merge. * docs: re-wrap the CONTRIBUTING chooser paragraph (#1844) Round-2 Copilot nit: the previous wrap left a dangling "The" at end of line, splitting a phrase mid-sentence in the raw markdown. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: clarify the protocol-era prompt on the bug form (#1844) Round-3 Copilot nit. "the protocol era (legacy / modern) if you selected one" reads as referring to a field on this form, which has none — the era is selected on the *connection*, in the Inspector's server settings. Reworded to "the protocol era you connected with (auto / legacy / modern)", which also picks up `auto`, the default the previous list omitted. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: stop describing the feature form as sharing the bug form's fields (#1844) Round-4 Copilot nits, and it was right three times over — I had described "client, version line, transport" as the required set for *both* forms in CONTRIBUTING.md, AGENTS.md, and the config.yml header. The feature form has neither a version-line dropdown nor a transport field, and cannot have the first: v1 takes security fixes only, so a feature request is v2 by construction. Each of the three now distinguishes the two forms rather than generalizing from the bug form. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: declare v2 statically on the feature form (#1844) Round-5 Copilot nit caught a real contradiction: the AGENTS.md paragraph said no form can apply a version label, then asserted the feature form "is always labeled v2". Both halves can't be true. Resolved in the direction that makes the claim true rather than softer. A form's `labels:` is static, which is only an obstacle when the value depends on a reporter's answer — and for the feature form it doesn't: v1 takes security fixes only, so a feature request is v2 by construction. So it now declares `["enhancement", "v2"]` outright, which also satisfies AGENTS.md's label-at-create-time rule without a triage step. The bug form still can't, and shouldn't: its version line is genuinely the reporter's to answer. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
812a115424 |
docs: add Incoming to both boards, adopt the v2 Priority field, and document the triage rubric (#1892)
* docs: add Incoming board status, adopt Priority field, document triage rubric (#1891) Board #28 had no state for "filed but not yet triaged" — Todo means maintainer-approved, so parking unreviewed issues there falsely asserted sign-off. Adds an Incoming status ahead of Todo. The board's Priority single-select existed but was unused (0 of 264 cards) and undocumented, with P0/P1/P2 options matching no vocabulary in the repo. Re-optioned to Urgent/High/Medium/Low and documented alongside Status. Completes the priority rubric: fills in the score bands (12+/9-11/6-8/<=5), expands both axes into scored tables, and fixes the truncated signal-indicator line. Severity alone tops out at High by design. Also drops the "V2 Go Live" status reference — that option is not in the field's option list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: correct the Done option id and harden the deleted-column guidance (#1891) The Done Status option was deleted from board #28, blanking the Status of all 247 cards that held it. Restored from a pre-change item-list snapshot; verified all 247 are Done again and no other card was disturbed. A recreated option never regains its old id, so Done is now 259d6aab rather than 248a3910 — updated in the option table and the merge step. The hazard callout recommended the web UI as the safe path, which is what made this look safe. It is not: deleting an option blanks its cards in the UI exactly as it does via the API, with no undo. Corrects that, adds a snapshot-first rule, and documents the recovery recipe used here — including the grouping check that proves the orphaned set is exactly the deleted option's cards before re-applying. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: add Incoming to the v1 board and document its recipes (#1891) Board #11 had the same gap as #28 — no state for "filed but not yet triaged" — so a v1 security-fix issue had nowhere to sit that didn't assert maintainer approval. Adds Incoming there too. AGENTS.md carried no v1 board recipes at all, only a link, so anyone boarding a v1 issue had to discover the project and field ids by hand and was liable to reach for #28's (which are rejected, but only after the attempt). Adds a V1 board section with its ids, states that Incoming is the default status for new items on both boards, and notes that Priority is v2-only — #11 has no such field. The Incoming option was added with all four existing option ids echoed back, and a before/after item-list diff confirms all 74 cards kept their Status. Three cards on #11 carry no Status; that predates this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: distinguish the board Priority from the org-level issue field (#1891) An issue page shows two fields named "Priority" and nothing syncs them. Ours is the project field on board #28. The other is a GitHub issue field (IFSS_kgDOAdAWeg) defined at the modelcontextprotocol org and shared by every repo in it, alongside Effort/Start date/Target date. Same name, same four option names, unrelated storage — #1891 sat at Urgent in Fields and High on the board simultaneously. Adds a table naming both, states that no pass-through exists in either direction, and says to leave the org-level one alone: it is not repo-scoped, so deleting it would strip Priority from every other org repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: mark the org Priority field untrusted and cap it at a +1 signal (#1891) Both boards are private (public: false, verified), so maintainer-assigned Status and Priority are invisible to reporters — board priority is a working queue, not a published commitment. The org-level Fields -> Priority is the opposite: public on the issue page and not part of triage. A value there is a reporter's preference, not an assessment, so it is untrusted input. It now feeds the rubric as a sixth signal bonus — flat +1 for Urgent or High, identical for both — and never maps to a band. Urgent needs 12, so nothing a reporter types reaches it alone; the issue must already sit at 11 on maintainer-assessed axes. Supersedes the earlier "it is noise" framing: the signal is worth keeping, it just cannot be allowed to decide the outcome. Also corrects the band arithmetic (2-15 -> 2-16) now that a sixth bonus exists, and notes that scores recorded on 2026-08-01 cite the old /15 denominator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: drop the /15 denominator caveat now the recorded scores are restated (#1891) The 51 triage comments were rewritten from /15 to /16, so the note saying they cite the old denominator no longer describes anything. No total moved: the reporter-set signal only scores for Urgent or High, and the one scored issue carrying an org-field value (#1826) had Low, which earns nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: describe the board-option hazard per field, not Status-only (#1891) Copilot review on #1892 flagged three places that still read as Status-only now that the section covers Priority too: - the version-label bullet said a v2 issue needs "a board card with a Status"; it needs a Status and a Priority (v1 needs only a Status, since board #11 has no Priority field) - the updateProjectV2Field hazard said a bad edit "orphans the Status of every card"; it orphans whichever field was edited - the recovery recipe is genuinely Status-specific (reads .status, writes the Status field id), so say so and give the two substitutions for a Priority deletion Verified `gh project item-list --format json` exposes both .status and .priority before documenting the .priority swap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
8801d0f8bc |
fix(scripts): await the pack:verify web child's exit before removing its work dir (#1895)
* fix(scripts): await the pack:verify web child's exit before removing its work dir `pack-and-verify.mjs` had the same kill-then-remove shape that made `smoke:tui` flake with ENOTEMPTY in #1801: `verifyWeb()`'s `finally { stop() }` only *delivered* SIGTERM, then the main flow `rmSync`'d the work dir immediately, and `fail()` did the same back to back. Applies the #1814 treatment, factored into a shared `scripts/lib/child-cleanup.mjs` so the two scripts can't drift: - `stopChild()` — SIGTERM, await the child's `exit`/`close`, escalate to SIGKILL after a grace period, and ultimately proceed with a warning so a wedged child can never hang the script. `verifyWeb()`'s `stop()` now goes through it and is awaited, so the child is gone before the caller removes `work`. - `removeSafe()` — `rmSync` that warns instead of throwing. Used on both success-path removals (a leftover temp dir must never fail a run that passed) and in `fail()`, where an ENOTEMPTY could otherwise bury the real diagnostic under an rmSync stack and skip the "tarball retained at ..." hint. `fail()` stays synchronous — it is called from sync contexts and ends in `process.exit()` — so it best-effort signals the child; `removeSafe()` is what makes that safe. `smoke-tui.mjs` adopts `removeSafe()` for its `cleanup()`. Its `done()` keeps its bespoke wait (documented inline): that one must keep waiting for `close` *after* `exit` on a shorter re-armed deadline so the quoted output is complete, which `stopChild()`'s first-of-exit/close contract deliberately does not do. Adds `scripts/lib/child-cleanup.test.mjs` (10 cases, `node --test` via `npm run test:scripts`) covering the SIGTERM/SIGKILL escalation, the `close`-only spawn-failure path, the never-hang give-up, late events after giving up, and that `removeSafe` warns rather than throws. `npm run ci` passes, and `npm run pack:verify` (not in the gate — needs network) was run end to end and passes with no cleanup warnings. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scripts): normalize stopChild's graceMs and correct removeSafe's JSDoc Copilot review round 1 on #1895. - `stopChild()` trusted `graceMs` as given, but call sites build it as `Number(process.env.X ?? 5000)` — a typo yields `NaN`, which `setTimeout` treats as `0`. That would fire SIGTERM and SIGKILL in the same tick and print "did not exit within NaNms", collapsing the whole escalation silently. New exported `normalizeGraceMs()` falls back to `DEFAULT_EXIT_GRACE_MS` for anything non-finite or non-positive, with two tests: the table of bad values, and an end-to-end one asserting a `NaN` grace period still leaves only SIGTERM sent a turn later. - `removeSafe()`'s `@returns` said "true if the path was removed", which is inaccurate for a missing path (`force: true` makes it a no-op success). Reworded to say what it actually reports. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
694d013f26 |
chore(deps): bring main's rc-series bumps to v2/main (hono 2.x, vite 8.1.5) (#1881)
The 2.0.0-rc series shipped these dependency bumps to `main` only, so `v2/main` was developing against older versions and would have met them for the first time at the milestone merge. - `@hono/node-server` ^1.19.14 -> ^2.0.12 (root, clients/web) — a major. v2.0.0's only breaking changes are dropping Node 18 (the repo already requires >=22.19.0) and removing the `@hono/node-server/vercel` adapter (unused here). The used surface — `serve()`, `serveStatic`, `ServerType` — is unchanged, so no source edits were needed. - `vite` ^8.0.0 -> ^8.1.5 (root, clients/web) and ^8.0.16 -> ^8.1.5 (clients/tui) — the security bumps from #1841. `npm install` at the root (postinstall cascades into every client) regenerated the three touched lockfiles. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
13e981e9dd |
chore: restore repo files dropped by the v2 tree swap (#1871)
* chore: restore repo files dropped by the v2 tree swap
A sweep of the pre-swap tree (
|
||
|
Cliff Hall
|
ee707db52d |
ci: restore the Claude Code workflow and .mcp.json, lost in the v2 tree swap (#1869)
* ci: restore the Claude Code workflow, lost in the v2 tree swap Closes #1850. Restored byte-for-byte from |
||
|
Cliff Hall
|
ff99547309 |
docs: restore SECURITY.md on v2/main, so the next tree swap can't drop it (#1867)
Closes #1864. SECURITY.md was restored to main in #1843 but never added to v2/main. Because v2/main is the develop branch merged into main at milestones, and the two share no common git ancestor, the next milestone merge would silently remove the security policy from the default branch a second time. Copied byte-for-byte from main (sha256 bbc8aaa5a33d481f902c1b7ac260a4c9c0d41dac5bb2916607222dd205dda924, 2633 bytes). No content changes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
7cc3c1276d |
docs: unpin the branch-pinned CONTRIBUTORS.md links in the PR template (#1879)
* docs: say "security fixes only" for v1 and unpin the PR-template links The v1 maintenance policy was stated two different ways across the docs — "security fixes only" in CONTRIBUTORS.md, "bug fixes" in README.md. The #1819 PR triage told 120 contributors that v1 is security-fixes-only, so that is the wording we keep. Also make the PR template's two CONTRIBUTORS.md links relative instead of pinned to blob/main/. `main` is release-only, so those resolved against the last released tree rather than the branch the reader is on. The remaining AGENTS.md occurrences are left alone deliberately: PR #1866 rewrites those exact lines and lands first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: drop the README change; #1866 owns that line The "Repo status" callout edit here patched `(bug fixes only)` → `(security fixes only)` on the callout as it exists on v2/main. But #1866 replaces that entire callout, because the surrounding sentence is false today: it still says "The `main` branch is the legacy v1 implementation", when `main` now holds the latest released v2. So this edit both collides with #1866 on the same line and, taken on its own, would preserve the false statement while correcting only the parenthetical. #1866 already sets the v1 policy wording to "security fixes only" across AGENTS.md and README (ee020ff2), so nothing is lost by dropping it here. This PR is now exactly its intended half: the two branch-pinned `blob/main/CONTRIBUTORS.md` links in .github/pull_request_template.md, made relative. That file is untouched by #1866, so the two PRs no longer overlap on any line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
379a8a0a3a |
docs: rename CONTRIBUTORS.md to CONTRIBUTING.md so GitHub surfaces the policy (#1884)
* docs: rename CONTRIBUTORS.md to CONTRIBUTING.md (#1883) GitHub's contributing-guidelines banner — shown when someone clicks "New issue" or opens a pull request — keys on the exact filename CONTRIBUTING.md. Naming the file CONTRIBUTORS.md left that prompt dark precisely where the "open an issue, not a PR" policy needs to land. Renamed with `git mv` so history follows the file; contents are unchanged (its heading already reads "Contributing to MCP Inspector"). Updated the one in-repo reference, in AGENTS.md. The two links in .github/pull_request_template.md are deliberately untouched — PR #1879 makes them relative so they follow the rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: drop the AGENTS.md link edit; #1866 owns that line This PR edited the `See [CONTRIBUTORS.md](./CONTRIBUTORS.md)` link in AGENTS.md, which is the exact line #1866 rewrites — guaranteeing a one-line conflict when this merges after it. Handing that line to its owner instead: #1866 now writes the link as `CONTRIBUTING.md` directly, so this PR is a pure `git mv` and the two touch no common line. No conflict to resolve at merge time. The tradeoff is a short window between the two merges where AGENTS.md links to a filename that doesn't exist yet. That's deliberate and bounded: #1866 is first in the merge order and this is third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
2c24b82e8f |
docs: converge README Test servers section with main's restructure (#1880)
* docs: converge README Test servers section with main's restructure Transplants main's restructured "Test servers" section (#1836) onto v2/main — the "Serving the modern protocol era" and "Showcase configs" headings, the config table, and the per-config subsections — replacing the eight long paragraphs v2/main still carried. Folds in v2/main's #1846 correction: main's copy still claims Mcp-Param-* mirroring is skipped in the browser, which stopped being true when the Inspector took over building the mirrored headers itself. The blockquote callout keeps main's shape with the corrected wording. Scoped to that section only — the whole file is deliberately NOT copied from main, so #1866's repo-status callout and release-section edits (and every other v2/main-only line) are untouched. Shrinks the first v2/main -> main milestone merge from a whole-section conflict to a single 3-line hunk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: point the #1846 reference at the issue, not a nonexistent PR URL #1846 is the issue; the fix shipped as PR #1847. The callout's link used /pull/1846, which 404s. Per Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
3ac34e4092 |
docs: post-swap branch model, PR policy, milestone rule, and Copilot review instructions (#1866)
* docs: correct post-swap branch model, PR policy, and stale gate/tree entries Part of the go-live phase-8 docs hygiene: everything that silently inverted when `main` became v2. Branch/release model — documented canonically in AGENTS.md and corrected everywhere it was restated: - AGENTS.md: replace the one-line "Base Branches" bullet with a table giving each branch its role (v2/main = develop, main = release, v1/main = maintenance), whether PRs target it, and which npm dist-tag it publishes. - README.md: the "Repo status" callout had it backwards, claiming `main` was the legacy v1 implementation. Also note that a v2 release is cut from `main` after the milestone merge, not from v2/main. - CONTRIBUTORS.md: the version table listed v2's base branch as `main`. PR policy: - Every PR must reference an issue, from anyone, no exceptions. - PRs are opened by repo maintainers only — org write access is not authorization. Everyone else files a detailed issue. - Branch names start with the target version segment (v2/…, v1/…). - Issues and PRs carry exactly one of `v1`/`v2` at creation; default `v2`. - Screenshots proving UI/TUI changes go in the gitignored pr-screenshots/. Corrections to stale content found while in there: - The TUI coverage bullet still described the Ink/App.tsx/hooks exclusion as interim pending #1501, which closed 2026-06-29; the gate now covers all of src/** with only tui-servers.ts excluded. - The completion checklist said `npm run validate` was the gate and covered "e2e tests", contradicting the mandatory-pre-push-gate section (npm run ci). - v1 PR base was given as `main`; it is `v1/main`. - Project tree omitted core/client, docs/, and scripts/ (README: core/client, docs/). core/client is under the coverage gate and imported by both App.tsx trees. Also adds the launcher and `npm run test:scripts` to the "run the tests" list and a tiers overview, since neither was mentioned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: require a milestone on every new issue, defaulting to the current one Adds the rule alongside the version-label rule, since they fail the same way: an unmilestoned issue drops out of release planning as silently as an unlabeled one drops out of version filtering. - Set it at create time (`gh issue create --milestone <title> ...`). - If the user didn't specify one, default to the current milestone — the open milestone with the nearest due date — rather than leaving it blank pending a decision. Includes the `gh api .../milestones` one-liner that identifies it. - Milestones are release buckets, so pick by when the work ships, not by size; sub-issues normally inherit their parent's milestone. Also widens the section's opening callout: an issue is not "created" until it is labeled, milestoned, AND boarded with a Status — four distinct steps, not three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: add .github/copilot-instructions.md and require mirroring into it GitHub Copilot reads .github/copilot-instructions.md when reviewing a PR. The repo had no such file, so Copilot reviewed without any of the conventions that actually govern this codebase — the ban on `any` and on unjustified double casts, the Mantine-first styling rules, the `.withProps()` extraction rule, the lib/utils split, test placement, and the per-file >=90% four-dimension gate. The new file is a review-focused distillation of AGENTS.md: the rules a reviewer would cite against a diff, plus a short "what to prioritize" list that puts correctness and security first (this backend spawns processes and proxies outbound requests). Deliberately omitted: board recipes and IDs, milestone and branch mechanics, release procedure, and the project tree — no reviewer cites those, and copying them would double the maintenance surface. AGENTS.md's "Keep documentation files up to date" section now requires mirroring review-relevant changes into it in the same PR, states that AGENTS.md stays the source of truth, and spells out what counts as review-relevant. There is no generation step and nothing detects drift, so the rule is the only thing keeping the two in sync. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: address Copilot review — v1 policy wording, stale V2 Go Live, typo - Align the v1 maintenance policy on "security and bug fixes only" in AGENTS.md and CONTRIBUTORS.md; both said "security fixes only" while the README callout, the branch table, and the board description said "security and bug fixes". - Drop the stale "or V2 Go Live" instruction from the work-begins step — that column no longer exists on board #28 (verified via `gh project field-list 28`), so the option id it implied would be rejected. Restore the removed-column id list the same edit dropped, now including V2 Go Live, and re-date the table to today's verification. - Fix "contains the the new version" typo. - Strip a stray trailing blank line at the end of AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: restore `inspector/` as the Project Structure tree root The tree is a filesystem layout — every child is a real directory (`clients/`, `core/`, `test-servers/`). Labeling its root `v2/main/` named a branch instead, which reads as a path that does not exist in a checkout and diverged from the identical tree in README.md, which still says `inspector/`. The branch model is documented in its own table under Repository & Project Boards; the tree does not need to restate it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: settle the v1 policy wording as "security fixes only" Reverses the direction the Copilot round took in 33cc0366. That commit normalized the six v1-policy sites onto "security and bug fixes" because that was the majority wording — but majority is the wrong tiebreak here. The policy is set by what we told contributors: the #1819 backlog close note, posted to 120 closed PRs, says "v1 will receive security fixes only", and the five PRs retained for 1.0.5 are all security fixes. So the two sites that already said "security fixes only" were the correct ones, and the four that said otherwise are now aligned to them: - AGENTS.md: the `v1` label description, the branch-role table, and the v1 board line. - README.md: the "Repo status" callout. CONTRIBUTORS.md and AGENTS.md's Project Status bullet already read correctly and are unchanged. This is the substance of #1813, which is why that issue's PR is now reduced to its PR-template-link half — the wording work landed here to avoid two PRs editing the same lines in opposite directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU * docs: point the AGENTS.md policy link at CONTRIBUTING.md This PR rewrites the paragraph holding the only in-repo link to the contributing policy, so it owns that line. #1884 renames the file CONTRIBUTORS.md -> CONTRIBUTING.md; having #1884 also edit this line would guarantee a one-line conflict between the two. Writing the new filename here instead makes #1884 a pure `git mv` with no overlapping line, so neither PR conflicts with the other. Between this merging and #1884 merging, the link points at a filename that doesn't exist yet. That window is deliberate and bounded — this PR is first in the merge order (#1821) and #1884 is third. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
Sam Morrow
|
5d91d4acd0 |
fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1847)
* fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1846) A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server) rejects a tools/call whose argument carries an `x-mcp-header` annotation when the matching `Mcp-Param-*` request header is missing (-32020 "header mismatch"). The Inspector never sent that header. The SDK only mirrors inside `client.callTool()` (and skips it entirely in a browser environment), but the Inspector routes tools/call through `client.request()` for manual MRTR driving (#1704), so the SDK never mirrors for us — on any transport. On top of that, the remote (web) transport dropped per-request headers before they reached the backend's upstream fetch. - core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders` + value encoding (Base64 sentinel for non-ASCII/whitespace/empty values) and add `mcpParamHeadersForTool`. - InspectorClient.attemptToolCall: on a modern connection, build the headers from the tool's declarations + call arguments and attach them to the tools/call request options. `Protocol.request` forwards `headers` to the transport and preserves them across MRTR retry legs, so the direct StreamableHTTP transport applies them. Fixes CLI/TUI. - RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest: forward per-send headers and apply them to the backend's upstream transport.send. The browser can't set cross-origin headers, but the Node backend can — where the Inspector's real request is issued. Fixes web. Verified end-to-end against the live GitHub MCP Server (modern HTTP). Closes #1846 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: harden backend header allowlist, fix breadcrumbs, trim comments Address Copilot review on #1847: - server.ts: restrict client-supplied per-send headers to the `Mcp-Param-` prefix (string values only) via `mcpParamHeadersOnly` before applying them upstream, so the per-send channel can't inject arbitrary headers (e.g. Authorization). `settings.headers` remains the channel for configured upstream headers. Covered by new unit tests. - Fix stale `#1810` breadcrumbs → `#1846` in the two new tests. - Trim the verbose mirroring comments in inspectorClient/types/remote transport. - Drop the redundant base64-on-the-wire integration test (encoding is unit- tested; wire delivery is proven by the verbatim test), remove two unneeded test casts, and cover the boolean `true` branch. Not changed (declined with reason on the PR): buildMcpParamHeaders remains a faithful port of the SDK's helper and does not re-validate values against the declared JSON Schema type — arg types are enforced by schema validation on both sides, and matching the SDK's conforming wire output is the module's goal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd * review: mirror Mcp-Param-* on the task-augmented tools/call too Address the maintainer review on #1847: - `callToolStream` (the "Run as task" path) built its own request options and never mirrored, so a task-augmented `tools/call` on an annotated tool was still rejected by a strict modern server with -32020. It now mirrors like the plain path. - Extract the two blocks both `tools/call` entry points had duplicated — `convertStringToolArgs` (the string-arg coercion) and `applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't drift again. That duplication is why the task path was missed. - Regression test: `callToolStream` against the modern `get_weather` server asserts the POST carries `Mcp-Param-City` (verified failing without the fix). - README: the modern-network showcase claimed mirroring is skipped in the browser so the web client gets -32020, and that CLI/TUI mirror. Both are now wrong — the Inspector mirrors itself, on every client. AGENTS: note that xMcpHeader.ts now also builds the wire headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t * docs: correct the stale "browser omits Mcp-Param-*" copy Both statements predate this PR and are now the opposite of true, per the wire capture on #1847 (proxy-observed upstream headers, web leg included). - ToolDetailPanel: the mirrored-headers note claimed the browser omits `Mcp-Param-*`. It now says the web client's headers are applied by the Node backend that issues the upstream request. - createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated tool is "uncallable from the web client against a strict modern server" — the exact wrong conclusion to reach while debugging a -32020. It now records that the Inspector builds the headers itself, and that they do NOT travel this `/api/fetch` proxy path: they ride the transport's per-send `headers` through `/api/mcp/send`. Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx matches the section title only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UkhhCHryVp5H3dqEnhwZ7t --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd |
||
|
Cliff Hall
|
fb1b0cb41c |
docs: restore SECURITY.md, lost in the v2 tree swap (#1843)
`SECURITY.md` existed on the pre-swap `main` (
|
||
|
Cliff Hall
|
7aebf168e6 |
chore: 2.0.0 (#1842)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
The v2 go-live release (#1818, phase 5 of #1804). This is byte-identical to 2.0.0-rc.3 apart from dropping the prerelease suffix, which is what makes rc.3's validation meaningful: the tree that ships is the tree that was validated against the live registry. `2.0.0` has no hyphen, so the derivation added in #1834 resolves it to `latest` — this is the release that takes the tag from 1.0.1. Validated across three release candidates: - rc.1 caught two publish-path defects invisible to every local check — `NODE_AUTH_TOKEN` pointing at a non-existent secret (shadowing OIDC trusted publishing) and a missing npm CLI upgrade (trusted publishing needs >= 11.5.1; Node 22 bundles 10.x). Both fixed in #1836. - rc.2 validated the dependency sweep (#1837) that cleared every prod-scope advisory using lockfile-only changes. - rc.3 validated the vite 8.1.5 bump (#1841) closing three high-severity dev-server file-read advisories. Each RC was verified with a cold `node:22` container install driving the published tarball end to end — install, `--help`, and a real `--cli tools/list` against a live stdio server. `latest` remained on 1.0.1 throughout, proving the `--tag` derivation. Zero open Dependabot alerts at time of release. Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
73fe059072 |
fix(deps): raise vite to 8.1.5 to close three dev-server file-read advisories; 2.0.0-rc.3 (#1841)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
The #1837 sweep moved vite to 8.0.16 in the root and tui manifests but left `clients/web` pinned at **8.0.0** — `npm update` reported "up to date" and only an explicit install would move it. That left five open advisories, three high: GHSA-fx2h-pf6j-xcff high `server.fs.deny` bypass via Windows ADS GHSA-v2wj-q39q-566r high `server.fs.deny` bypassed with queries GHSA-p9ff-h696-f583 high arbitrary file read via the dev server GHSA-v6wh-96g9-6wx3 medium launch-editor NTLMv2 hash disclosure GHSA-4w7w-66w2-5vf9 medium path traversal in optimized deps These are worth fixing rather than dismissing as "dev-only". Unlike the other remaining dev-scope alerts, `server.fs.deny` bypass is *remotely triggerable*: a malicious page can read arbitrary files from anyone running `npm run dev`. Published-package users are unaffected — they get the built `dist` and never run vite — but contributors are. Also raises the declared floor to `^8.1.5` in all three manifests that declare vite. The root previously declared `^8.0.0`, which still *permitted* the vulnerable 8.0.0 on a fresh resolve, so pinning the lockfile alone would have left a regression path open. Bumped to rc.3 rather than folding this into 2.0.0: vite builds the shipped bundle, so changing it changes the artifact, and the tree that ships should be the tree that was validated against the live registry. rc.2 validated a different tree. `npm run ci` passes — full chain including the coverage gate, build gate, smokes, and 462 Storybook tests. Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
d5e35b52ee |
chore: clear all runtime dependency vulnerabilities; 2.0.0-rc.2 (#1837)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Sweeps every Dependabot alert that reaches users, then bumps to rc.2 so the swept tree gets the same live-registry validation rc.1 got. Deliberate: rc.1 validated a specific dependency tree, and changing it invalidates that proof. Result: **zero prod-scope high/critical vulnerabilities** across all five manifests (root + four clients). Achieved with lockfile changes ONLY — no package.json touched, no declared range widened, no major upgrade. Every one of these was a stale lockfile pin sitting inside an existing caret range: hono → 4.12.32 (needed 4.12.27) fast-uri → 3.1.4 (needed 3.1.3) postcss → 8.5.23 (needed 8.5.18) brace-expansion → 1.1.16 (needed 1.1.16) js-yaml → 4.3.0 (needed 4.3.0) ws → 8.21.0/1 (needed 8.21.0) flatted → 3.4.3 (needed 3.4.2) Two dev-scope clusters remain, both deliberately untouched — neither ships (the `files` allowlist is `clients/*/build`, `clients/web/dist`, and `scripts/install-clients.mjs`), and both need real work that does not belong immediately before a release: - eslint (5 alerts × 5 lockfiles): a `brace-expansion` DoS via `minimatch`, fixable only by eslint@10 — a major bump across every package, likely requiring rule migration. The glob patterns involved come from the repo's own config, never attacker-controlled. - vitest (3 critical, clients/web): a circular exact-pin knot — `@vitest/browser-playwright@4.1.0` requires `@vitest/browser@"4.1.0"` exactly, `vitest@4.1.0` requires `@vitest/browser-playwright@"4.1.0"` exactly, and `@storybook/addon-vitest` constrains the set further. `npm i` fails ERESOLVE; untangling means moving Storybook too. `npm run ci` passes on the swept tree — full chain including the coverage gate, build gate, smokes, and 462 Storybook tests. Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
954f9e83f6 |
fix(ci): publish via OIDC trusted publishing, not a non-existent NPM_TOKEN (#1836)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* fix(ci): publish via OIDC trusted publishing, not a non-existent NPM_TOKEN
The 2.0.0-rc.1 publish failed with `npm error code ENEEDAUTH`. Two causes, both
in the v2 publish job, which had never actually executed before — every prior
release was cut from the v1 workflow.
1. `NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}` pointed at a secret that does
not exist (the repo has no secrets at repo or environment level — publishing
moved to OIDC trusted publishing). `setup-node` therefore wrote an *empty*
`_authToken` into its generated `.npmrc`, and npm failed ENEEDAUTH before
OIDC was ever attempted. Removed, with a comment saying why it must not be
"restored".
2. The job never upgraded the npm CLI. Trusted publishing requires npm
>= 11.5.1 and Node 22 bundles 10.x, so this would have failed even with the
token line gone. The v1 workflow has this step; v2 was missing it.
The RC did its job: it caught both against the real registry, and consumed
nothing — the run failed before `npm publish` wrote anything, so no version was
burned and the dist-tags are untouched.
Also confirms #1834 works: the log shows `Publishing 2.0.0-rc.1 under dist-tag
'next'` before the auth failure.
README corrected — it documented `NPM_TOKEN` as part of the publish setup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
* docs: restructure the test-servers section into navigable subsections
The section had grown into eight undifferentiated paragraphs, the longest 1,965
characters, each describing one or two showcase configs. The content is good
reference material — the problem was purely that you could not find anything in
it or tell where one config's description ended and the next began.
Restructured, not rewritten. No facts removed:
- A summary table of all nine showcase configs, with what each demonstrates and
its tracking issue, so the set is visible at a glance.
- One `####` subsection per config family, so each is linkable and skimmable.
- Enumerations that were buried in prose are now tables: the six MRTR presets,
the four `trigger_*` spec-error tools and their status/code pairs.
- Step sequences (advertised extensions) and era contrasts (logging,
subscriptions, tasks) are now bullets rather than run-on sentences.
- Two caveats that were parenthetical asides — browser `Mcp-Param-*` skipping,
and `collect_elicitation` erroring on the modern leg — are now blockquotes,
since both are things you hit and then have to go hunting for.
Longest line drops 1,965 → 620 characters. Verified every previously-documented
config and all technical identifiers survive.
Noted while auditing: `demo.json`, `oauth-step-up-demo.json`,
`url-elicitation-form.json` and `xaa-ema-http.json` exist in
`test-servers/configs/` but have never been documented here. Left alone —
that is a pre-existing gap, not a regression from this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Cliff Hall
|
75e3116b10 |
chore: 2.0.0-rc.1 (#1835)
Release candidate for the 2.0.0 go-live (#1818, §6). Publishes under the `next` dist-tag via the derivation added in #1834 — verified: `2.0.0-rc.1` contains a hyphen after the patch component, so it resolves to `next`, leaving `latest` on 1.0.1 untouched. The RC exists to exercise what only exists against the live registry: the `next`-tag publish path itself (newly written in #1834), provenance/OIDC minting, the `files` allowlist as npm actually packs it, and the postinstall cascade's early-exit under a real dependency install on a clean machine. Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
145002925f |
ci: derive the npm dist-tag from the version instead of defaulting to latest (#1834)
`npm publish` defaults to `--tag latest` regardless of semver prerelease status, and the publish step passed no `--tag`. So cutting the `2.0.0-rc.1` release that #1818 calls for would have published the release candidate to `latest` — pointing every `npx @modelcontextprotocol/inspector` at an RC. That is the same class of failure #1816 fixed for v1, except triggered deliberately by following the runbook. Derives the tag from the version: a hyphen after the patch component means a prerelease (`2.0.0-rc.1` → `next`), anything else is a release (`2.0.0` → `latest`). Build metadata (`2.0.0+build.5`) is correctly not a prerelease. Implemented as a shell `case` rather than with `semver`, which is only a transitive dependency here and should not be relied on in CI. Also makes the RC step of #1818 possible at all — there was previously no way to pass a dist-tag through a release-triggered publish. Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
844567585d |
chore(deps): upgrade @hono/node-server to 2.0.12 (#1832)
* ci: set a least-privilege default GITHUB_TOKEN scope CodeQL alert #64 (`actions/missing-workflow-permissions`, medium) on `.github/workflows/main.yml`: no `permissions` block, so every job inherits the repository's default GITHUB_TOKEN scope, which is broader than anything here needs. It matters more than the severity suggests because this is the workflow that publishes to npm under OIDC — an over-permissioned token in a job that mints publish credentials is worth tightening before 2.0.0 ships (#1818). Adds a workflow-level `permissions: contents: read`. The `publish` and `publish-github-container-registry` jobs already declare their own blocks (`id-token: write` and friends) and are unaffected: job-level permissions override the workflow-level default outright rather than merging with it. The comment records that, since it means each publish job must keep listing every scope it needs — including `contents: read` — and a future edit that trims one "because it's inherited" would break the publish. Surfaced by the v2 tree swap (#1817): `main` had never scanned this workflow before. Pre-existing on `v2/main`, not introduced by that merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD * chore(deps): upgrade @hono/node-server to 2.0.12 Clears GHSA-frvp-7c67-39w9 (2 Dependabot alerts): path traversal in `serve-static` on Windows via an encoded backslash, affecting < 2.0.5. The Inspector's prod web server calls the flagged function directly (`clients/web/server/server.ts` mounts `serveStatic` at `/*`). Worth being precise about the actual exposure, since the summary line reads worse than the flaw is: `..` escapes remain blocked, so it only reaches files *inside* the configured root, by slipping past prefix-mounted middleware. Here `root` is the built SPA — everything in it is public by design — and the only prefix middleware (`/api/*`) is a separate route, not a subdirectory of that root. So there was no auth bypass and no arbitrary file read to fix; this is hygiene, not an incident. The 1.x → 2.x majors turn out to be a non-event for this codebase: v2 drops Node 18 (the repo already requires >=22.19.0) and removes the Vercel adapter (unused). `serve`, `serveStatic`, and `ServerType` — the entire import surface here, across the prod server and ~15 integration tests — are unchanged. `npm run validate` passes on all four clients. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
809b9fdf5c |
ci: set a least-privilege default GITHUB_TOKEN scope (#1831)
CodeQL alert #64 (`actions/missing-workflow-permissions`, medium) on `.github/workflows/main.yml`: no `permissions` block, so every job inherits the repository's default GITHUB_TOKEN scope, which is broader than anything here needs. It matters more than the severity suggests because this is the workflow that publishes to npm under OIDC — an over-permissioned token in a job that mints publish credentials is worth tightening before 2.0.0 ships (#1818). Adds a workflow-level `permissions: contents: read`. The `publish` and `publish-github-container-registry` jobs already declare their own blocks (`id-token: write` and friends) and are unaffected: job-level permissions override the workflow-level default outright rather than merging with it. The comment records that, since it means each publish job must keep listing every scope it needs — including `contents: read` — and a future edit that trims one "because it's inherited" would break the publish. Surfaced by the v2 tree swap (#1817): `main` had never scanned this workflow before. Pre-existing on `v2/main`, not introduced by that merge. Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
25106dcce4 |
Merge pull request #1830 from modelcontextprotocol/chore/v2-golive
chore: replace main's tree with v2 |
||
|
cliffhall
|
ec5d8e139e |
chore: replace main's tree with v2
Phase 4 of the v2 go-live runbook (#1804 §5). `main` becomes the v2 tree while keeping its own history: this is a true merge recording `v2/main` as a second parent, not a reset, so no force-push is needed and the default branch's history and PR trail survive intact. Mechanically: `-s ours` to take no tree from the merge, then the index is replaced wholesale with `v2/main`'s. `git diff HEAD origin/v2/main` is empty afterwards. Note the runbook's command needed one addition — `main` and `v2/main` have genuinely unrelated histories (different root commits, no merge base), so `--allow-unrelated-histories` is required or the merge refuses outright. All 125 open PRs previously targeting `main` were re-pointed at `v1/main` first, so none of them are stranded against a tree that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD |
||
|
Cliff Hall
|
4d30d1cd1e |
docs: restore the missing docs/ targets and split the catalog-seeding wording (#1811) (#1812)
Closes #1811 Four README links pointed at files that don't exist on `v2/main`, and the launcher README described catalog seeding as one behavior when it is two. - `docs/mcp-server-configuration.md` — written fresh for v2 (the v1.5 file predates the `--catalog`/`--config` split) - `docs/launcher-config-consolidation-plan.md` — restored from `v1.5/main`, path drift corrected, v2 notes appended - `docs/images/tui-screenshot.png` — regenerated against the current TUI - launcher README seeding wording — split into the two real behaviors (web seeds two sample servers; cli/tui seed empty) Review across three rounds also corrected several claims verified against source: the `--` separator is inverted between web and cli, a cli target placed after a flag is silently discarded, `--server` is a no-op on web and an unknown-option error on tui, the `MCP_CATALOG_PATH` ad-hoc guard is cli-only, and the conflict matrix diverges on both `--header` and `--transport stdio`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD |
||
|
Cliff Hall
|
de9d8a5024 |
fix: wait for the TUI to exit before removing its temp dir (#1801) (#1814)
* fix: wait for the TUI to exit before removing its temp dir (#1801) `done()` sent SIGTERM and then immediately `rmSync`'d the work dir, which doubles as the TUI's HOME. Nothing waited for the child, so the Ink process could still be writing into the dir while rmSync walked it — a file landing after a directory was read but before it was removed surfaces as ENOTEMPTY on macOS, failing `smoke:tui` after a successful render. Removal now happens only once the child's `exit` event fires: SIGTERM, then SIGKILL after SMOKE_TUI_EXIT_GRACE_MS (default 5s), then clean up regardless after twice that. The crash-before-render path short-circuits via a `childExited` flag set by the pre-registered exit handler. cleanup() also warns instead of throwing, so a leftover temp dir can never fail a passing smoke. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq * fix: wait on close, not exit, so spawn failure resolves instantly (review #1814) A failed spawn emits `error` + `close` and never `exit`, so the previous `child.once("exit")` wait never fired on that path: done() sent SIGTERM to a process that never existed and resolved only via the give-up timer — 10s at the default grace, with a warning blaming a SIGTERM the child never received. Waiting on `close` covers both normal termination and spawn failure, and additionally guarantees the stdio pipes are drained. The short-circuit gate moves with it, `childExited` → `childClosed`: keying it on `exit` would have skipped the drain on the crash-before-render path, which is exactly where the quoted output matters. Messages that quote that output are now thunks rendered in finish(), after the drain, instead of being sliced at call time. SIGTERM is now sent only to a child that is still running, since on both the crash and spawn-failure paths there is nothing left to signal. finish() also gets a `finished` guard so its single-entry invariant is local rather than inferred from process.exit() being synchronous. Measured, spawn failure (grace shortened to 500ms): 1.04s + spurious warning before, 0.02s and silent after. Crash-before-render now prints the child's stderr detail line that the pre-drain slice could truncate. Happy path 3x, timeout path, and full `npm run smoke` + `npm run validate` all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq * fix: bound the drain, and make the give-up warning true (review #1814 r2) Three follow-ons from the second review pass: The give-up warning still said the TUI "did not exit within Xms of SIGTERM", but after switching the wait to `close` neither clause is reliably true — the timer's most plausible trigger is now a child that has already exited, and on the crash and spawn-failure paths no SIGTERM is sent at all. That is the same defect this PR fixed one round ago (a warning naming a signal never sent), so the deadline is now re-armed with a message describing what is actually being awaited. `close` is bounded by whoever holds the stdio pipes, which can outlive the direct child: a descendant that inherited them keeps it pending. That is the one real cost of `exit` → `close`, and it is moot only because the TUI spawns nothing at boot — a reason that lives entirely outside this file. Once `exit` has fired the deadline drops to DRAIN_MS, keeping the drain guarantee without inheriting an unrelated process's lifetime. Measured against a grandchild holding the pipes for 3s: 0.54s and an accurate warning, instead of 3s. The quoted output is sliced from the tail rather than the head. The thunk added last round exists to capture bytes that arrive late, which a head slice then discards as soon as output exceeds 800 chars — and on the timeout path the last frame is what diagnoses a stuck render, not the first. Verified: a 2KB stderr crash now reports its final line. The sibling scripts keep `slice(0, 800)`; changing them is a deliberate sweep, not a drive-by here. Also corrected the cleanup() comment — the wait lives in done(), and it is for `close`, not `exit`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq * docs: correct the cleanup() and DRAIN_MS comments; tail-slice cleanly (review #1814 r3) The cleanup() comment claimed its warning "should not fire now that removal waits for close" — true only of the normal path. Both give-up branches call finish() without close, and those are precisely the states where something may still hold the dir, so the warning is expected there. Same defect class as the two warnings already fixed in this PR (a message asserting what the code no longer guarantees), relocated into a comment. The DRAIN_MS comment explained why the cap exists but not what it costs: on that path the work dir is removed while a descendant holding HOME=work may still be writing to it — #1801's race, re-entered deliberately. That is only acceptable because cleanup() warns instead of throwing, so record the two as load-bearing for each other where the constant is defined. Tail-slicing an Ink stream can begin mid-CSI-sequence, leading a diagnostic with an escape-code fragment that mangles the line after it. outputTail() drops through the first newline when the output was actually truncated, and returns short output untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
cb454990eb |
docs: refresh V2 board Status option IDs in AGENTS.md (#1823) (#1825)
* docs: refresh V2 board Status option IDs in AGENTS.md (#1823) `V2 Go Live` (`b3a6966e`) was added to the Status field via the web UI on 2026-07-27 for the go-live phases (#1804) and was not in the table; the `SDK V2 + New Spec` row it replaced in position (`1bbb6f57`) had already been removed, so following the table failed with "option Id does not belong to the field". Record the current five options, note that adding the column via the web UI left the other four IDs untouched (evidence the UI path is safe, unlike the GraphQL full-replace hazard documented below it), and list every removed column's dead ID in one place. Also add the missing recipe for moving a card that is already on the board: the existing recipes only cover `item-add` for a new item, leaving no copy-paste path for looking an existing card's item id up by issue number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq * docs: close the ID-rotation pointer over the new recipe (review #1825) The hazard note's maintenance pointer lists the hardcoded option ids that must be updated when they rotate, but the recipe added in the previous commit hardcodes a third one (159c8a02) that the pointer did not name — reproducing the staleness class this PR exists to fix. Add it. Also fold in two adjacent fixes: the "In review" casing left behind two lines from the "In progress" → "In Progress" fix (labels are matched exactly when re-fetching ids by name), and a note that --limit must stay above the board's item count, since exceeding it truncates silently and surfaces as an opaque node-resolution error from item-edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq * docs: move the --limit caveat inside the recipe fence (review #1825) The section tells the reader to copy the recipes verbatim, so guidance that lives only in the prose above the fence is lost by anyone who grabs the block — which is the whole point of the block. The two recipes above it already carry their context as in-block comments; match that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
b3be65b0b4 |
docs: add CONTRIBUTORS.md issues-only policy (closes #1517) (#1604)
* docs: add CONTRIBUTORS.md issues-only policy (#1517) Document the contribution policy: we accept issues, not pull requests. Maintainers handle design and implementation through a prompt-driven workflow. Contributors who have already built a change locally should share the prompt they used rather than a diff. Cross-reference the policy from AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: add PR template redirecting external authors to issues-only policy (#1517) GitHub auto-populates pull_request_template.md into every new PR body, so it steers would-be external PR authors to CONTRIBUTORS.md before they submit a diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: drop version-board details from CONTRIBUTORS.md (#1517) The policy takes effect after v2/main merges into main, leaving a single line of development. Tell contributors to open a well-formed issue rather than routing them to per-version project boards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: add version-board/label table to CONTRIBUTORS.md (closes #1595) Completes requirement #2 of #1517: a table mapping each active version to its base branch, project board, and version label, plus a "Label by version" note mirroring AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX * docs: drop v1.5 from CONTRIBUTORS.md, invite deeper contribution (#1517) Review feedback on PR #1604: - Remove the v1.5 row/bullet from the version table — v1.5 was an internal intermediate branch that external contributors never saw, so surfacing it in an outward-facing doc is noise. The table is now v1 / v2 only. - Add a "Want to work on the Inspector with us?" section pointing at the MCP Contributor Discord (#inspector-dev) and the community calls, framing the issues-only rule as being about unsolicited patches rather than a closed door, with supervised work as the path toward maintainership. - Link that section from "Why this policy exists", and add the same Discord pointer to the PR template. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD * docs: drop project-board links from CONTRIBUTORS.md (#1517) The org project boards aren't visible to anyone outside the organization, so linking them in an outward-facing doc points contributors at a 404. The version table is now base branch + label only, and the PR template names the version labels instead of "the appropriate version board". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD * docs: v1 takes security fixes only (#1517) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD * docs: use post-runbook branch names (v1/main, main) (#1517) This doc will be read after the branch runbook completes, when v1 lives on v1/main and v2 is the default main. Update the version table, the label-by- version line, and the PR template's label hint + CONTRIBUTORS.md links accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
7784553674 |
fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) (#1810)
* fix(auth): offer explicit re-authorize when callback state is lost (#1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(auth): detect the SDK mismatch error via isInstance (#1808) The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares that brand in a `static {}` block, so it lives on the constructor and is never reachable from an instance. The predicate always returned false, so `findIssuerBindingFailure()` never matched and the #1808 recovery banner was inert in production — the callback dead-ended exactly as before. Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand set), with a `name` fallback for a serialization boundary. Correct the module doc, which cited cross-bundle safety as the reason to avoid `instanceof` — the very thing the SDK's branded `hasInstance` provides. Every fixture now constructs a real `AuthorizationServerMismatchError` instead of a hand-rolled object with an own `mcpBrand`, which is why a green `npm run ci` hid this. Adds a test pinning the brand's placement so the feature cannot silently go inert again. Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove the duplicated banner title default, resolve the banner title/action label where the banner is raised, keep the clear-failure notification open, and bound remote-supplied issuers in user-facing copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
01df9d81bb |
fix(core): start OAuth on a modern/auto-era connect (#1805) (#1809)
* fix(core): start OAuth on a modern/auto-era connect (#1805) The SDK's `server/discover` negotiation probe runs before anything else on `protocolEra: "auto" | "modern"`, and its classifier reports whatever the transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved to `data.cause`. That buried both connect-time auth signals: the remote path's `AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the authorization URL) and a direct transport's `AuthChallengeError`. An OAuth-protected server that authorized fine on the legacy era instead produced a dead-end "Version negotiation probe failed" — no redirect, and the connection status flipped to `error` despite `isConnectAuthRecoveryError` existing to prevent exactly that. - `findNestedAuthError()` walks `cause` / `data.cause` for either typed error. Not gated on the SDK's error code or message, so it survives rewording. - `connect()` unwraps the rejection at the earliest point, ahead of `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the outer status guard, and every client's connect error handling — so web, CLI, and TUI are all fixed without a client-side change. - On the direct path, enable `interceptAuthChallenges` for the probing eras even with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely, no status and not even a cause for the walk to find. Verified against a live dual-era server that answers 401 on both `initialize` and `server/discover`: `auto` and `modern` now reject with `AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the fix is load-bearing — reverting either fails the new integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om * docs(core): mark the era-probe challenge interception as a removable workaround Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as #1807). Make its lifespan explicit so whoever picks up #1807 knows this block is the thing to delete, rather than having to re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): resolve connect() when auth recovery is satisfied silently Review findings on #1809: - connect() built its handshake promise once, outside the runConnect closure withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg re-awaited the already-rejected promise and rejected a connect() whose client was in fact connected. Short-circuit the retry when already connected, and skip the post-connect block the nested connect already ran (a duplicate `connect` event re-triggers every list-state manager's refresh). - Document the accepted 403 side effect of enabling interceptAuthChallenges with no stored tokens, and why every `{ pin }` counts as probing. - Replace the private-method double cast in the era-probe test with an observable assertion on the transport options, and split the non-auth failure case per era instead of a message alternation that passed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): key the connect retry leg off attempt, bound nested recoveries Second-pass review findings on #1809: - The satisfied-recovery short-circuit keyed off live connection status, so an onclose landing between the nested connect() and the retry leg fell through to the rejected handshake promise and reported the stale 401 as the reason the session died. withDirectAuthRecovery now passes `attempt` to its operation, so connect() identifies the retry leg structurally and reports the teardown when the recovered session is already gone. - Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a satisfied outcome recovers via a nested connect() with a fresh counter, so a server that keeps rejecting freshly refreshed tokens recursed without bound. Counted across the nesting boundary, capped at 3. - Strengthen the regression test (server info populated by the nested connect, no duplicate statusChange) and add the two missing cases: a failed reconnect still rejects, and the recursion bound terminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * test(core): cover the recovery teardown branch; fix a duplicated case Third-pass review findings on #1809, both about tests added in the previous commit: - The retry leg's teardown error had no coverage, so a regression to the old status fall-through would have stayed green. Covered deterministically: the nested connect dispatches `connect` after setting "connected", so closing the live transport from that listener flips the status before the retry leg runs. Asserts the error identity, which is the point of the change. - "still rejects when the reconnect underneath the recovery fails" used a challenging transport for every connect, so it walked the same bounded chain as the recursion-bound test rather than covering a non-auth reconnect failure, and its comment claimed an "error" status the auth path never reaches. Now fails the reconnect with a plain error and asserts both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: rinormaloku <rinormaloku37@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |