3,715 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
2f2d545751 |
docs: narrow the dep-lockstep summaries to the invariant it actually proves
From Copilot's ninth pass of #1962. The one-line descriptions in AGENTS.md and the README claimed "no install-crossing dependency resolves to two different versions", which is stronger than the gate: candidates cover only packages the shared sources name directly, and #1965 records a live counterexample (`@modelcontextprotocol/sdk`, skewed across two install roots while the guard stays green). The boundary was already documented at length on the guard and in the AGENTS.md rule; the summaries just hadn't been narrowed to match, which is how a doc ends up asserting a guarantee nobody implemented. Both now say "directly imported" and point at #1965. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
48d74ba423 |
chore(deps): check each shared source contributed, not the aggregate count
From Copilot's eighth pass of #1962. The no-op guard tested only `files.length === 0`, which stopped meaning anything once `SHARED_SOURCE_FILES` was added: `vitest.shared.mts` alone keeps the total nonzero, so a moved or renamed `core/` would leave the guard deriving candidates from a near-empty set and passing on skew it exists to catch. `sourcesWithNoFiles` now requires every configured dir and every named file to have contributed, and names the ones that didn't. Directory matching is on a path separator, so a prefix sibling (`core-internal/`) can't vouch for a renamed `core/`. Also drops two test comments left stale by the move to `ts.preProcessFile` — they still described the deleted TRIVIA regex. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
3638185e0f |
chore(deps): fail with a cause when typescript can't be resolved
The guard resolves typescript from `clients/web`. The realistic way for that to fail is a root install run with INSPECTOR_SKIP_CLIENT_INSTALL=1, which leaves `clients/web/node_modules` empty — and a bare MODULE_NOT_FOUND stack gives no hint that the fix is `npm install` at the root. Throw with that guidance and the original error as `cause`. Deliberately not a silent skip: an unrun guard guards nothing, which is the failure mode the sibling-vouch ring exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
fcd45037c9 |
chore(deps): extract specifiers with ts.preProcessFile; document the transitive boundary
From Copilot's sixth pass of #1962. Take the TypeScript-parser suggestion, which was raised across three rounds and was right. The regex scan got both directions wrong and every widening traded one failure for the other: it *missed* valid syntax (`import x = require(…)`, an import-attributes argument, a comment between tokens — each a silent miss, so the package never entered the candidate set and its skew passed), and it *invented* names from prose, since `// adapted from "react"` is indistinguishable from an import to a pattern that cannot tell code from a comment. The earlier prose test only passed because `cwd omitted` isn't a valid package name. `ts.preProcessFile` is the cheap answer I'd missed while arguing against the full AST: TypeScript's lightweight pre-parse scanner, no parse tree and no type checking, purpose-built to return module specifiers. It handles every import form, trivia, strings, and regex literals, and never reads a comment as code. typescript resolves from `clients/web`, which already carries it, via the `createRequire` base `smoke-web-browser.mjs` established — a bare import would resolve relative to `scripts/`, not the cwd. The derived set drops 18 → 17 real names: every genuine package is retained and the one prose artifact is gone. Regression tests now use *real* package names in comments and strings, the case the old test couldn't cover. Also records the KNOWN BOUNDARY the review's other finding identified. Candidates cover what the shared sources name directly, so a package reaching the program only through another package's `.d.ts` is invisible — `@modelcontextprotocol/sdk` is the live example, skewed 1.29.0 vs 1.30.0 and present in web's program from both installs, never written in first-party code. Closing it properly changes what the guard measures, so it is tracked as #1965, with both candidate derivations measured there: a lockfile closure is unusable (155 packages, 25 skewed, nearly all irrelevant tooling, and it misses the SDK anyway), while `tsc --listFilesOnly` is correct and small (15 for clients/web). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
2f1d97e7bb |
chore(deps): treat line comments as trivia too in the import scan
From Copilot's fifth pass of #1962. `TRIVIA` covered block comments but not `//`, despite the contract right above it saying a comment is legal wherever whitespace is — so `import(// lazy\n"pkg")`, `require(// lazy\n"pkg")`, and `from // reason\n"pkg"` were stepped over, and such a package would never enter the candidate set. Same silent-miss direction as the block-comment gap. Both comment syntaxes are now covered. The line-comment branch runs to end-of-line only; the newline itself is matched by TRIVIA's `\s` branch. Regression test added alongside the block-comment one. The derived set is unchanged at 18 real names — widening trivia does not admit prose, since a specifier must still follow immediately and `packageNameOf` rejects anything not shaped like one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
2dcf1450b2 |
chore(deps): see imports through comment trivia; name vitest.shared.mts in the README
Both suppressed comments from Copilot's fourth pass of #1962. TypeScript allows a comment anywhere whitespace is legal, so `import(/* webpackIgnore: true */ "pkg")` and `from /* why */ "pkg"` are valid imports the patterns stepped over. That is the dangerous direction: a missed specifier never enters the candidate set, so the package's skew passes the guard silently, against the over-approximation the module claims. A `TRIVIA` fragment now stands in for `\s*` at every inter-token position, with regression cases for the commented static, dynamic, side-effect, and `require` forms. I did not move to the TypeScript parser. This is a root `scripts/` `.mjs` tool with no bundler and no TS dependency; pulling in a compiler API to read import specifiers is a lot of machinery for a scan that only needs to over-approximate, and each missed form is a one-line pattern with a test beside it. The derived set is unchanged at 18 real names, so nothing here widens what is checked today. The README's command row also still described the candidate scan as `core/` and `test-servers/src` only, omitting the `vitest.shared.mts` the guard now includes; it also never mentioned that an allowlisted package is tolerated only within a major. Both stated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
35317fd9ed |
chore(deps): scope the allowlist to a major, cover vitest.shared.mts, fix the vouch comment
All four suppressed comments from Copilot's third pass of #1962. Each was correct. The candidate boundary omitted `vitest.shared.mts` — root-owned, imported by every client's vitest config, and already treated as shared non-client source by `verify:typecheck-coverage`. It imports only Node built-ins today, which is exactly why the omission would have gone unnoticed until a third-party import appeared there, resolved from the root, and skewed. Shared *files* are now a first-class list beside the shared dirs. The allowlist was a blanket exemption by name. Each entry's rationale establishes that a patch/minor difference is benign, which is no evidence that a React 18-vs-19 or Hono 4-vs-5 split would be — that is a different type surface. Being listed now tolerates skew only *within a major version*; a cross-major skew fails like anything else. Deriving the major from the lockfile version keeps this automatic, with no per-package range to maintain and bump. A version that can't be parsed is treated as "cannot prove same major" and fails rather than passes. `verify-format-coverage.mjs`'s comment overstated the vouching graph: the three guards do not each check the others. This one checks both siblings; each sibling checks only this one. Described as it actually works. The AGENTS.md rule said to bump a shared dependency in "root + all four clients", but launcher declares no zod and `findSkew` ignores installs where a package is absent — so the rule as written asked for spurious dependencies. Reworded to every install that *declares* it, and mirrored in `.github/copilot-instructions.md`. The failure output follows: it no longer advises adding a package that is already allowlisted, saying "MAJOR skew" instead, and it names all three shared sources rather than the two dirs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
4fe02bca45 |
docs: keep the Docker recipes loopback-only; note the legacy token fallback
- The v1 recipe published on 127.0.0.1 and the v2 one silently dropped that. Docker's -p binds host interfaces independently of the container's HOST, and a no-Origin request skips the allow-list (core/mcp/remote/node/server.ts:241), so the token is the only guard on a process-spawning backend. Restore the loopback prefix on both recipes and say why. - MCP_PROXY_AUTH_TOKEN is not removed: web-server-config.ts:329 still falls back to it. Call it a deprecated fallback rather than a rename, and distinguish it from the query param, which has no fallback (App.tsx getAuthToken reads only the new name). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a |
||
|
cliffhall
|
ddfce0f7ac |
fix: reject a non-string protocolVersion, and drop the test double casts
Copilot review follow-ups: - `applyProtocolVersion` took the value straight from an unvalidated JSON body and went to `RegExp.test`, which coerces — `123` and `true` both matched the token pattern and reached `Transport.setProtocolVersion` with the wrong type. It now takes `unknown` and checks `typeof === "string"` first, with cases for the coercible values. - Replace `as unknown as Transport` in the new session fixtures with a `makeTransport()` helper returning a structurally valid `Transport`, so an interface change stays visible to the tests. - Drop `as unknown as typeof fetch` in the new transport test — `vi.fn<typeof fetch>()` is already the right type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
8c9cf40211 |
docs: qualify the ad-hoc rule and fix the contradicting -- guidance
- The "nothing combines with an ad-hoc target" rule has a web exception: run-web.ts:130 exempts `--transport stdio`, so web ignores it rather than rejecting. Only cli/tui reject every `--transport`. - mcp-server-configuration.md still said a leading-dash argument cannot reach a stdio server under --cli, the opposite of what the migration guide now says. The parser puts every pre-`--` token in targetArgs, so the guide is right; corrected the older section to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a |
||
|
cliffhall
|
908284d046 |
docs: address Copilot's suppressed comments on the migration guide
- `--` under --cli: everything before the separator IS forwarded to the target, flags included (cli.ts:536-541), so "no way to pass a leading-dash argument" was wrong. Show the correct placement instead of sending users to a catalog file. - Stdout: v2's emitResult appends "\n" where v1's awaitableLog wrote the JSON with no terminator, so the output is not byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a |
||
|
cliffhall
|
47e0ee059a |
chore(deps): match CommonJS and template-literal imports in the shared-source scan
From the suppressed comment on Copilot's second pass of #1962. Under- approximating is the dangerous direction here: a package the scan misses never enters the candidate set, so its skew passes the guard silently. Three forms went unmatched. `import x = require("pkg")` and a bare `require("pkg")` are the ordinary import syntax in `.cts` — which the previous commit just admitted to the scan, so leaving them out would have been an inconsistency introduced by that widening. A dynamic import carrying import attributes (`import("pkg", { with: … })`) was missed because the pattern required the closing paren. A static template literal (`import(\`pkg\`)`) was missed for want of a backtick in the quote class. Backticks are accepted only in the *call* forms. A `from` clause requires a string literal, so allowing them there would buy nothing while reopening the prose hazard the anchored specifier pattern closes: inline code in comments is written with backticks throughout this codebase, and admitting it inflated the derived set from 18 names to 41 with prose fragments (`tools`, `connect()`, `messages`). Inert today, since a name absent from every lockfile contributes nothing — but a prose word colliding with a real package name would silently widen the set. Pinned with a regression test. The real set is unchanged at 18, so these forms add no package today; they close the syntax against the one that arrives later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
7a74b5eba1 |
fix: forward the negotiated protocol version to the remote backend transport
The web client's SDK Client owns the `initialize` handshake, so the `setProtocolVersion()` call it makes after initialize lands on RemoteClientTransport — a pass-through to the Node backend, not the real HTTP transport. The backend's StreamableHTTPClientTransport therefore never learned the negotiated version and stamped no `Mcp-Protocol-Version` on any request after initialize, starting with `notifications/initialized`. A stateful server that requires the header rejects that notification with HTTP 400. RemoteClientTransport now records the version and rides it on every `/api/mcp/send`; the backend applies it to its transport before the send, so that request and everything the transport issues afterwards (the standalone SSE GET, the session DELETE) carry the header. The version is validated against a token pattern on the backend so a client can't push arbitrary bytes into an upstream header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 |
||
|
cliffhall
|
20527dc177 |
docs: address Copilot review on the v1 → v2 migration guide
- Sandbox port: the Apps tab needs a second, dynamic listener, so "no second port" was wrong for Docker/remote deployments. Qualify the architecture section, add a pinned-port Docker recipe, and add a troubleshooting entry. - Node floor: npm only warns EBADENGINE unless engine-strict is set, so npx does not refuse to run on an older Node. - v1 default list: the web UI persisted its list in localStorage; only the on-disk default catalog is new. - The v1→v2 --server example needs --cli (and a --method) to select an entry; note there is no one-entry web equivalent. - New behavior change #4: an ambiguous URL path (neither /mcp nor /sse) now errors instead of falling back to SSE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a |
||
|
cliffhall
|
e480fdc49d |
chore(deps): cover .mts/.cts in the shared-source scan, fix the README validate row
Both from Copilot's review of #1962. The candidate-set enumeration matched only `.ts`/`.tsx`, while `verify:format-coverage` and `verify:typecheck-coverage` both gate all four TypeScript extensions. No `.mts`/`.cts` exists under `core/` or `test-servers/src` today, which is exactly why the omission would have gone unnoticed until a shared dependency arrived through one and skewed — the failure this guard exists to prevent. Extract the rule as the exported `isSharedSourceFile` predicate, cover all four extensions, and pin it with regression tests (including path-boundary anchoring, so a `core-internal/` sibling isn't swept in). The README's `validate` row also omitted `test:scripts` and claimed typecheck was cli/tui only; launcher runs one too, and web typechecks via `tsc -b` inside its `build`. Restate the row to match the actual script chain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
e328da43b5 |
test: cover the duplicateToolNames server option over a live connection
The ToolControls regressions build their `Tool[]` by hand, so they would still pass if the new server option broke — config loading, resolveConfig, the ordering, the title marker, or its composition with pagination. Copilot flagged exactly that gap. Adds an integration test that connects a real InspectorClient to a real streamable-HTTP test server and asserts the wire shape: repeats appended (not adjacent — the ordering the defect depends on), only the appended copy marked "(duplicate)", an unregistered name ignored, an empty list a no-op, and duplication running before the page slice so a pair straddles a boundary. The last case loads `duplicate-tool-names-http.json` through loadConfig + resolveConfig, so the JSON plumbing the manual repro uses is covered too. Verified non-vacuous: mutating withDuplicates to place repeats adjacent fails four of the five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
6c33da38c9 |
chore(deps): align zod across installs and guard against version skew
Closes #1896 `clients/web`'s `tsc -b` exhausted the 4GB default heap at zod 4.4.x. The cause is not a zod regression nor a pathological inference site of ours: it is two copies of zod at *mismatched versions* inside one tsc program. `clients/web/tsconfig.test.json` compiles `test-servers/src`, whose files resolve zod (and the SDK's zod) from the **root** install, alongside web's own sources, which resolve from `clients/web/node_modules`. At 4.3.6 vs 4.4.3 TypeScript must relate two structurally-distinct declarations of every `@modelcontextprotocol/*` schema type, which is exponential over a recursive generic surface — `TS2589 Type instantiation is excessively deep`, then OOM. Bumping root zod to 4.4.3 and changing nothing else returns the build to baseline: 11.4s and 2,567,882 instantiations, against 11.2s / 2,567,503 at 4.3.6. Two copies at the same version are harmless; skew is what explodes. So drop the `~4.3.6` hold on `clients/web` and move all four manifests and lockfiles to zod 4.4.3 together, and add `verify:dep-lockstep` to `validate` to keep them there. The guard derives its candidate set from what `core/` and `test-servers/src` import, compares the committed lockfiles' top-level entries, and fails deny-by-default on any skew not in the annotated `TOLERATED_SKEW` allowlist. It joins the existing mutual-vouch ring so dropping it from `validate` is caught by `verify:format-coverage`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
4340283f70 |
test: add a duplicate-tool-names test server for the #1957 repro
Nothing in the repo could serve the shape this fix is about: every preset registers a unique name, and the SDK's `registerTool` rejects a repeat. So the only reproduction was a unit test — there was no way to smoke the real app against a server that returns duplicates. Adds `ServerConfig.duplicateToolNames` (a list of registered tool names) to the composable test server, plumbed through the JSON config, and a showcase config `duplicate-tool-names-http.json`. The named tools are emitted a second time in `tools/list` with the same `name` and a "(duplicate)" title. The repeats are appended after the whole list rather than placed beside their twin. That is both the realistic shape (two tool sources concatenated) and what makes the defect observable: React matches a leading run of same-key children first, so a head-adjacent duplicate happens to line up and the bug hides. Only a separated pair collides in the keyed map and orphans a row — verified both ways against the prod bundle. The `tools/list` override is shared with the existing pagination handler (which already hand-builds the list); with no `maxPageSize` it returns a single unbounded page. Duplication happens before pagination, so a duplicated pair can straddle a page boundary the way a real server's would. Also adds a ToolControls test for this appended ordering, distinct from the reporter's interleaved fixture, and documents the config in the README showcase table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
6f535356b4 |
fix: key tool rows by source position so duplicate names filter correctly
`ToolControls` keyed both the main list and the SEP-2243 excluded list by `tool.name`. A server may legitimately return the same tool name more than once, and colliding React keys let a filtered-out row survive reconciliation: searching narrows the array, React matches the surviving rows against the stale keys, and an unrelated row stays mounted (plus a "two children with the same key" console warning). Stamp each row's position in the unfiltered list before filtering and use `<index>:<name>` as the key, on both lists. The position is captured before the filter, so the key stays stable as the search narrows. No deduplication, no change to what `tools/list` returns, and the name/title search behavior is unchanged. Adds regression tests for both lists using the reporter's fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
2e89443739 |
docs: add the v1 → v2 migration guide
Closes #1822 Adds docs/v1-to-v2-migration.md — the map for users arriving from v1: - CLI flag mapping (every v1 flag survives; lists what v2 adds) - `--config` vs `--catalog` semantics with before/after examples - Node engine bump (>=22.7.5 → >=22.19.0) - what no longer ships (the three sub-packages; v2 is one tarball) - the three behavior changes most likely to bite: exit code 5 on a failing tool call, target-must-come-first under --cli, and the reversed `--` separator - env-var mapping (MCP_PROXY_AUTH_TOKEN → MCP_INSPECTOR_API_TOKEN, MCP_PROXY_FULL_ADDRESS removed, SERVER_PORT repurposed), the v1 UI config settings, web query params, and Docker Linked from the root README (banner + docs list), the CLI README, and docs/mcp-server-configuration.md; AGENTS.md's docs/ tree entry updated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
cliffhall
|
347173fab7 |
fix: absorb only the teardown's own close, not any ConnectionClosed
Review feedback (round 3). The predicate absorbed *any* CONNECTION_CLOSED, while the doc comment claimed it absorbed only the teardown's — so a transport dropping on its own after the progress notifications but before `disconnect()` would have been swallowed. These tests assert only on the notifications, so that regression would have passed green. Gate the absorption on a teardown flag owned by the helper, which now returns an `InFlightCall` exposing `disconnectAndSettle()`. Keeping the flag inside means it can't be raised too early (which would silently reopen the hole) and the settle can't be forgotten; call sites also collapse from two statements to one. Verified: a pre-teardown ConnectionClosed now fails the test and stays observed (no run-level error), while a teardown-time one is still absorbed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
c60563b7d6 |
fix: observe the derived promise so an unexpected rejection can't go unhandled
Review feedback (round 2). Narrowing the handler in the previous commit fixed the silent-pass but reintroduced #1947's own failure class on the unexpected path: `then` returns a *derived* promise, and the re-throw rejects that one rather than `call`. The caller doesn't await it until after `disconnect()`, so a rejection arriving while the test is still waiting on progress notifications sits unobserved for seconds and Node reports it as an unhandled rejection. Attach an observer to the derived promise the moment it exists. Nothing is swallowed — it stays rejected, so the caller's `await` still fails the test. Verified both directions with a probe rejecting with a non-ConnectionClosed error and awaiting after a 1.5s gap: without the observer the run reports "Unhandled Rejection" + "Errors 1 error"; with it, the test fails and the run stays clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
0b9aee11a3 |
fix: only absorb the teardown's own rejection in settleInFlight
Review feedback: the blanket rejection handler also converted a genuine `callTool` failure into a pass. That matters here because these tests assert on the progress notifications, which arrive before the result — so a call that emitted all three progress events and then failed would have satisfied every assertion and been swallowed. Narrow the handler to absorb only `SdkError` / `CONNECTION_CLOSED` and re-throw anything else. The handler is still attached at call time, so an unexpected rejection stays *handled* and surfaces as an ordinary test failure at the `await inFlight` after `disconnect()` rather than as another unhandled rejection. Fulfillment is still accepted: whether the call beats the teardown is a race, so asserting either outcome would reintroduce the original bug as a flake. Verified the narrowed predicate is actually exercised rather than dead code — both call sites reject with SdkError/CONNECTION_CLOSED on every run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
|
cliffhall
|
1881e5c73f |
fix: settle the deliberately in-flight tool calls in the progress tests
Two progress tests fire `client.callTool()` without holding the promise so they can assert on the notifications the call streams while it is still in flight. `disconnect()` then closes the SDK client, which rejects every pending request with "Connection closed" — with nothing holding the promise that lands as an unhandled rejection, which vitest counts as a run error and fails `npm run ci` at `coverage` even though all tests pass. Add a `settleInFlight()` helper that attaches the handler at call time (not after the assertions, which would leave a window the rejection can escape through) and returns a promise the test awaits after `disconnect()` so teardown stays ordered. Neither outcome is asserted: the call may legitimately complete before the teardown or reject with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 |
||
Cliff Hall
|
a09f2dcf1d |
fix: load @napi-rs/keyring lazily so an unsupported platform degrades instead of crashing (#1905) (#1948)
* fix: load @napi-rs/keyring lazily so an unsupported platform degrades `@napi-rs/keyring` ships one prebuilt binary per platform triple and throws on import where it has none. On Android / Termux there is no `@napi-rs/keyring-android-arm64`, so the static top-level import in `core/auth/node/secret-store.ts` threw during module evaluation and the Inspector exited at startup with "Cannot find native binding" — before any of the keychain-unavailable handling in that same file could run. Replaces the static import with a cached dynamic one. The outcome is cached, not just the module, so a box without a binary doesn't re-attempt (and re-throw) resolution on every secret operation — `expectedSecretFields` means that would otherwise be once per server per `GET /api/servers` — and so `set` can name the underlying cause in its `KeychainUnavailableError`. An unloadable package now folds into the same degradation contract as an unreachable keychain: `get` returns null, `delete` and `deleteAllForServer` no-op, and `set` throws `KeychainUnavailableError` (not double-wrapped — the load failure is already typed by the time the catch sees it). The class doc comment now enumerates all three ways "unavailable" can arrive, since each escaped the contract at some point. The shared `vi.mock` stub can't express this — it models a keyring that loads — so the new tests build on `vi.resetModules()` + `vi.doMock` with a throwing factory and re-import the module, covering the load failure, each method under it, and the load-once caching. Verified the dynamic import survives bundling in all three clients (cli, tui, and the web runner) rather than being inlined back to a static one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F * fix: steer a missing native binding to a reinstall hint `KeychainUnavailableError` gave the Linux libsecret / gnome-keyring advice for every cause, including the one where the keychain is fine and the @napi-rs/keyring platform package is what's missing — an unsupported triple (#1905) or npm's optional-deps bug dropping it on a supported one (npm/cli#4828, the Windows report on #1852). Detect the loader's "Cannot find native binding" phrasing and point those users at a reinstall / npx-cache clear instead. Ported from the superseded #1943, which fixed this alongside the same lazy-load change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: name a bad keyring namespace instead of "is not a constructor" The lazy import accepted whatever the resolution produced. The named exports arrive through CJS interop, so a resolution that stopped yielding them — a default-only export upstream, a bundler changing interop, a platform where named-export detection fails — would hand us `undefined`, and `new mod.AsyncEntry(...)` would throw a TypeError inside the very try that implements graceful degradation. Shape-check the resolved module and treat a bad shape as unavailable. Scope, because it is narrower than it first appears: this does NOT prevent a bad shape from emptying the secret list. `get` returns null either way — that is its read-tolerance contract, and the TypeError lands in the same catch a dead keychain does. What changes is the diagnosis. Without the check the only signal is `set` reporting "keyring.mod.AsyncEntry is not a constructor", which reads like an Inspector bug; with it, `set` names the real problem once, at the load boundary, in the same actionable 503 as every other unavailability. Catching the silent-empty-list case itself would need a round-trip against the unmocked package, which nothing in the suite does today. The member access sits inside the try because reading a missing export is not always a harmless undefined — a Proxy-backed namespace can throw, as vitest's module mocks do — and letting that escape would reject the cached promise, which is otherwise guaranteed to always resolve. Of the four new tests only the cause-message one fails without the guard; the rest pin the surrounding contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * docs: correct the test header's count of unavailability entry points The shape check added a fourth way "unavailable" can arrive, and the enumeration in secret-store.ts was updated to match, but the test file's header still said three — so it read as an exhaustive list that silently omitted the case a whole describe below it covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: give the wrong-shape cause its own remediation hint The shape check added a cause that fell through to the libsecret advice — so a packaging mismatch told the user to install a keyring daemon, which cannot help, and on Windows or macOS points somewhere that does not exist. That is the same wrong-advice bug the native-binding branch was added to fix, reintroduced by the guard itself. Hint selection moves into `hintFor`, where libsecret is the explicit fallback rather than the default, and the shape failure becomes a distinct `KeyringModuleShapeError` matched by type — it is our own error, so there is no reason to re-parse text we wrote (the native-binding branch matches a string only because that message comes from napi-rs). Also retitles the wrong-shape test: it claimed the guard avoids "returning null secrets", but `get` returns null either way by the read-tolerance contract. The title now says what actually differs — `set` hard-fails — matching the correction already made in the comments. Both raised by Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 * fix: give both shape-check failure paths the packaging hint The shape check can fail two ways — the members are absent, or reading them throws — and only the first carried KeyringModuleShapeError. The catch path returned the raw error, so a Proxy-backed namespace throwing on access fell through to the libsecret advice: the same wrong-remedy bug as the previous two commits, one branch further along. Both paths now carry the type. KeyringModuleShapeError takes a detail string plus a standard `cause`, so the throwing path reads "its exports could not be read: <original>" and keeps the underlying failure legible while still earning the packaging hint. Three instances of one mistake in a row (a new cause added without deciding which remediation it inherits) says the shape was wrong, not just the branch: making libsecret an explicit fallback in `hintFor` did not stop me adding a second exit that skipped it. The tests now pin the invariant directly — one asserts both construction paths produce the same hint — so the next cause that forgets is a failing test rather than a reviewer's catch. Raised by Copilot review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
bb219dc001 |
fix: construct AsyncEntry inside the try so keychain degradation engages (#1848) (#1945)
* fix: construct AsyncEntry inside the try so keychain degradation engages `KeyringSecretStore` built its `AsyncEntry` outside the `try` in `get`, `set`, and `delete`. `AsyncEntry::new` performs the platform-store setup (on Linux, the Secret Service connect with a keyutils fallback) and throws when no backend is reachable, so the documented degradation contract never engaged for a construction-time failure: the raw keyring error escaped, 500ing every `GET /api/servers` on a box without a Secret Service (the published container, which has no D-Bus session). `expectedSecretFields` always includes the OAuth slot, so `rehydrateConfig` constructs an entry for every server — the default seeded catalog was enough to 500 the first list load, before any server was added. And because the escaping error wasn't a `KeychainUnavailableError`, it bypassed both the routes' 503 translation and the `migratePlaintextSecrets` skip branch, yielding a generic 500 instead of the actionable message. Moving construction inside the existing `try` restores the contract for that failure mode: `get` returns null, `delete` no-ops, and `set` is the only operation that throws — as `KeychainUnavailableError`, which is what the 503 translation and the migration skip both match on. The test stub's constructor could not fail, which is why the coverage gate never saw the gap. Adds a `constructorThrows` hook alongside the existing method-level `failures` flags and covers all three methods plus `deleteAllForServer` under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F * test: pin why the OAuth secret field is unconditional `expectedSecretFields` always includes the OAuth slot, and #1848's report frames that as why the 500 fired so broadly — which invites a follow-up that skips the keychain read for servers with "no OAuth config". That change would lose data. `extractSecretsFromStored` deletes the `oauth` block outright when `clientSecret` was its only property, so such a server carries NO marker on disk that a secret exists — the keychain is the sole record, and the unconditional slot is what finds it again. Gating the read on a disk-visible `oauth` block would silently stop rehydrating exactly that shape. The existing tests do fail if the slot is made conditional (verified by applying the change: 4 red), but only under names that explain nothing about the consequence — `always lists the OAuth slot first` reads like a tautology worth updating rather than a trap. Add a round-trip case that states the consequence, so the next reader sees the field is load-bearing rather than defensive. 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
|
8b3d2bce32 |
Merge pull request #1954 from modelcontextprotocol/v2/fix/surface-list-fetch-errors
fix: surface list-fetch failures instead of swallowing them |
||
|
cliffhall
|
a6f233eb6a |
fix: read the list error via useSyncExternalStore; clear correlation maps
Two fixes from Copilot's review of #1953. useManagedListError re-synced React state from the `state` prop inside a useEffect, which renders one frame carrying the PREVIOUS store's error after `state` changes (switching servers) before the effect corrects it — the pattern AGENTS.md forbids. useSyncExternalStore reads the snapshot during render, so a store swap lands in the same frame, and it also closes the window where an error recorded between render and subscribe was missed. The snapshot returns the stored Error instance (or null), so it is referentially stable as the API requires. The `markResponseRejected` correlation maps were never cleared. Ids of requests that never got a response — a timeout, a dropped connection — are the only entries trackResponse can't remove, so they accumulated across reconnects. Cleared on the start-clean connect path, per the convention documented on resetSessionState (one route out, `onerror` with no `onclose`, tears down nothing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
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> |