| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
09e1564672 |
fix(auth): refuse on a stuck lock, capture identity at the mkdir seam (#2082)
Copilot review round 6 — five findings (two inline, three suppressed), all valid. **A stale lock that cannot be cleared caused unlocked writes.** `acquireLock` does not only *create* directories: on finding a stale one it removes it and retries, and that removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` errors, which the probe read as "locks do not work here" and degraded on. So every Inspector on the box quietly bypassed the *same* stuck lock and raced its writes — while the release-failure message was telling the operator saves would keep failing until they cleared it. `isStuckOrHeld` now discriminates on **whether the lock directory exists** rather than on an errno taxonomy: if it is there, something holds it and we refuse; if it is not, we genuinely could not create one and degrading is the documented trade (#1848, #1905). Reading the state the decision is about avoids enumerating error codes per platform — and an enumeration is exactly what let `ENOTEMPTY` through. **Identity was captured after the acquire promise settled.** That span includes the library's own `utimes`/`stat` probe, so a waiter replacing our directory inside it would have us record *the winner's* identity as our own, after which the removal guard would accept and delete their lock. It is now recorded in the `mkdir` callback — the moment the directory becomes ours, and synchronous with respect to this process, so there is no window. **The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the guard `stat`s before removing, everything reaching the release catch is an `rmdir` that was refused — and stale takeover reclaims through that same `rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch was dead code: removed, and the advice is now unconditional. **`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A fourth externalization surface — the tsup lists configure the production bundles, not `vite dev`, whose dep scanner would otherwise walk its CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts` now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also absent from that assertion. **A test was named for something it did not exercise.** "does not treat an unlistable directory as an empty one" was driving `ENOTDIR`, which is deliberately treated *as* the fresh-install case, so the actual new behaviour was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using a `--x` directory — `readdir` denied, a known name still reachable, which is the asymmetry that makes "unlistable" different from "empty". Driven with a real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM namespace export; skipped as root and on Windows, where the mode would not bite and the test would assert nothing. Both new tests verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com> |
||
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
|
73d9b181fe |
History: MRTR conversation grouping + modern message vocabulary (#1705)
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
* History: MRTR conversation grouping + modern message vocabulary Teach the Protocol/History view the modern (2026-07-28) message vocabulary, correlating it off the frames the transport already captures — no change to how MRTR is driven (manual driving is deferred to #1704). - Correlation helpers in protocolUtils: extractResultType, extractRequestState, extractSubscriptionId, isModernFrame, and groupProtocolEntries — clustering the contiguous rounds of one logical operation (original call -> input_required -> retried call with a new id -> final result) that share a requestState token. - New MrtrConversation group renders that cluster as one expandable unit with a status derived from the final round, reusing ProtocolEntry for each round. - ProtocolEntry: resultType badge (input required / complete), a "modern" frame badge for server/discover, subscriptions/listen, and the acknowledged notification, and a copyable subscriptionId. - ProtocolListPanel folds each section through the grouping pass and shows an era badge in the header. Era is threaded from the negotiated protocolEra (App -> InspectorView -> ProtocolScreen -> panel) and never inferred from frames, respecting the transcript-classification caveat. - Shared EraBadge element (+ eraUtils); ConnectionInfoContent reuses it. - test-servers/configs/modern-mrtr-http.json serves an elicitation tool over the modern leg for eyeballing a real MRTR round-trip. - Pre-bundle @modelcontextprotocol/client in optimizeDeps.include so the Storybook vitest project doesn't re-optimize mid-run once protocolUtils pulls the client barrel into the browser graph. Closes #1627 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test-servers: real MRTR tool + live-verified History grouping While driving the modern flow in the browser, the elicitation-based config proved unable to produce MRTR: the modern (2026-07-28) SDK server rejects `server.elicitInput` — server→client requests aren't allowed on that leg; a tool must instead return `inputRequired(...)`. So the shipped `modern-mrtr-http.json` (and its README claim) were wrong. - Add a genuine MRTR tool preset (`mrtr_confirm` / `createMrtrTool`): its handler returns `inputRequired({ inputRequests: { confirm: elicit(...) } })` on the first call and the final result once the client echoes the answer back on the retry. - Teach the composable test-server to support it: the tool wrapper forwards an `InputRequiredResult` verbatim (instead of coercing it to a CallToolResult), and `toHandlerExtra` surfaces `inputResponses`/`requestState` so a handler can branch between the original call and the retry. - Point `modern-mrtr-http.json` at the new preset; correct the README to describe the real mechanism. - Add an integration test that drives the full round-trip through InspectorClient (auto-fulfilling the embedded elicitation) and asserts the final result is only reachable after the input_required round + retry — regression cover that would have caught the original config bug. Verified live in the browser: the History view renders the round-trip as one grouped "MRTR conversation" (Round 1 input_required → Round 2 complete), keyed by the real requestState, under the Modern era badge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address review notes: dedupe status badge, unique MRTR token, import alias Non-blocking review feedback on #1705: - ProtocolEntry: suppress the redundant green "OK" status badge when a `resultType` badge already conveys the outcome (a modern success shows only `complete`/`input required`). Errors/pending have no `resultType`, so their status badge still shows. - createMrtrTool: mint a unique `requestState` per original call (closure counter) so two same-action invocations don't fold into one conversation during manual eyeballing. Real SDK tokens are already unique per operation. - ProtocolEntry: align the `MessageEntry` import to the `@inspector/core` alias used by its siblings (was a deep relative path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review round 2: green `complete` badge, lighter embedded MRTR container Non-blocking observations from the second review: - ProtocolEntry: `resultType` "complete" now renders green (was gray) so a modern success keeps the at-a-glance green success affordance a legacy "OK" had, now that the redundant OK badge is suppressed. `input_required` stays yellow. - MrtrConversation: in the narrow monitoring (embedded) column, drop the outer container border and tighten padding — the per-round entries are already bordered cards, so this avoids a busy border-in-border nesting. The full-width view keeps the bordered surface that sets a conversation apart. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Review round 3: module-scope MRTR counter; revert embedded border change - test-servers: hoist the MRTR `requestState` counter to module scope. The modern (2026-07-28) leg is stateless — `createMcpHandler` rebuilds the server (and every tool closure) per request — so the previous per-closure counter reset to 0 on every request and every token ended in `:1`, defeating its purpose. A module-level counter survives the per-request factory recreation and increments monotonically across the process, so same-action invocations get distinct tokens and don't fold into one conversation. (Review finding.) - MrtrConversation: revert the embedded borderless/tighter-padding change — per maintainer feedback from manual testing, dropping the border didn't improve the narrow-column visual, so the group keeps its border in all layouts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
19a6df7ad3 |
SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624)
Migrate all three clients + core + test-servers from
@modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior
identical against legacy (2025-11-25) servers. Foundation for the SDK V2 +
New Spec stack.
Packages
- Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client,
transports, OAuth, error taxonomy, protocol types) + core (Zod schemas);
stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers ->
@modelcontextprotocol/server (+ server-legacy/sse). Deterministic import
rewrite (schemas -> core, everything else -> client); the codemod misroutes
client code to /server so it was not used for path routing.
- new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing
2026-era goes on the wire.
Runtime (core/mcp/inspectorClient.ts)
- Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode;
RequestTimeout -> SdkError/SdkErrorCode.
- Handlers take method strings; task methods (tasks/*, notifications/tasks/*)
use the 3-arg custom form (excluded from v2's spec-method set).
- Legacy tasks kept working (per project requirement): requestor tasks re-
implemented over raw client.request + explicit deprecated schemas;
experimental.tasks.callToolStream replaced by a local pollTaskToolCall
async generator (with _progressHandlers revival so task-execution progress
still reaches the caller). Receiver (server-initiated) tasks: the v2 Client
validates a sampling/elicit handler result and rejects the legacy { task }
response with -32602 — installReceiverTaskResponseBypass swaps the wrapped
entry in the private _requestHandlers map for the task-augmented branch so
{ task } reaches the wire (fixes server-initiated sampling-via-task).
- Pagination: list verbs use raw client.request({method}, List*ResultSchema)
for single-page behavior (v2's client.listX() auto-aggregate all pages).
Web / auth / apps
- Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema()
structural narrowing at the SDK-schema -> form boundary.
- ext-apps still peers on SDK v1: one compat boundary with documented casts
(remove when ext-apps#702 lands a v2 peer).
- OAuth provider saveClientInformation reconciled with v2's
OAuthClientInformationContext; InvalidClientMetadataError re-homed as a
local class (server-legacy/auth is not browser-safe).
- vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so
vi.mock intercepts the SUT's import.
Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call
rejects with -32602; cancellation is stream-abort (no guaranteed
notifications/cancelled frame); output-schema strictness restored via the
Inspector's own Ajv check on the default path.
npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5),
coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook
(410).
Closes #1624
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
Bob Dickinson
|
13dafe457b |
Guided-auth removal, OAuth smoke testing, and TUI/CLI support (#1531)
* refactor(web): update OAuth E2E tests to use new authentication method - Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration. - Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs. - Ensured that the tests continue to validate the expected authorization URL after the change. Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process. * Guided-auth removal, OAuth smoke testing, and TUI/CLI support * Added CLI OAuth runner flags tests * Added clarifying comment on port 0 binding * README notes on TUI oauth callback port, prettier fixes for CLI tests * CIMD validation uses inline like EMA IdP * Fixed TUI test failures from merge * test: bring 5 OAuth files over the 90 per-file coverage gate The guided-auth-removal work added/touched several files that fell below the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run the gate (it's local-only), so validate + test:integration passed while these gaps slipped through. Bring them all back over the line: - core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the provably-dead guards (empty host, out-of-range port, String(err) fallback, pathname "/" fallback) -- all rejected by new URL() upstream -- with justified v8 ignore comments instead of lowering the gate. - core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return. - core/react/useClientSettingsDraft.ts: test the functional-updater path, the stale-onChange-after-close prev===null guard, and the timer-fires-after-close value===null guard. - ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear. - clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration kinds, the expired IdP session, and the configured-scope/empty-scope branches of formatScopes. npm run coverage now passes (exit 0) across web, cli, tui, launcher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
037225a9fb |
feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) (#1360)
* feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) mcp.json is designed to be tool-shareable (symlinked from Claude Desktop's config, pasted into bug reports, synced via dotfiles), so storing OAuth client secrets and stdio env values in plaintext there meant any of those flows could leak them. Lift both into the OS keychain via @napi-rs/keyring (active replacement for the archived keytar). The wire shape is unchanged: the GET /api/servers handler rehydrates from the keychain so browser code sees the same JSON; the on-disk file no longer contains the secret material. Includes an idempotent migration that lifts plaintext from older mcp.json files (or hand-edited ones) into the keychain on first read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): tolerate keychain unavailability on non-set ops (#1356) CI on Linux failed because `@napi-rs/keyring` hard-errors without libsecret, and the route handlers were touching the keychain even for no-secret flows (defensive `deleteAllForServer` on POST/DELETE, sweep on PUT). The user's intent for the Linux fallback was "hard fail when a secret is actually involved" — not on every keychain touch. Make `KeyringSecretStore.get / delete / deleteAllForServer` silently degrade when the keychain is unavailable (return null / no-op); `set` remains the one operation that hard-fails with `KeychainUnavailableError`. The migration path catches the error and preserves the disk plaintext rather than losing the secret. Also inject `InMemorySecretStore` in `useServers.test.tsx` and `servers-events.test.ts` so tests never touch a developer's real keychain even when libsecret is present. Adds `KeyringSecretStore` tests (vi.mock'd native bindings) and an integration suite that simulates Linux-without-libsecret to verify the tolerance contract end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): close GET migration race; reverse keychain/disk order (#1356) Addresses code review: 1. **Concurrency**: GET /api/servers was reading + migrating outside the write lock, then writing inside it — a concurrent POST/PUT/DELETE between the read and the lock acquisition could be clobbered by the migration write. Also affected the first-launch seed write. Fix: take a fast unlocked peek; if the file is absent (seed branch) or carries plaintext to migrate (migration branch), re-read inside the write lock and decide there. Added `hasPlaintextSecrets()` as a pure predicate so the common no-migration path stays lock-free. 2. **Write ordering**: POST and PUT wrote disk first, then keychain. A keychain `set` failure mid-write left a half-configured entry on disk and trapped retries at 409. Reversed to keychain-first so a 503 leaves both stores in their pre-write state. PUT in-place now sets new fields, writes disk, then deletes obsolete fields (a failed disk write leaves orphan keychain entries — recoverable — rather than premature deletions of fields the user still expects to see). 3. **Tolerance simplification**: dropped the no-entry regex in `KeyringSecretStore.delete` — both "no entry" and "keychain unavailable" collapse to the same desired outcome (the entry isn't there anymore), so a uniform swallow is clearer. 4. **Parallelism**: `readKeychainEntriesFor` now `Promise.all`s the per-field `get` calls. macOS Keychain round-trips are 10-50ms; serializing 5+ env vars per server × N servers is a meaningful wall-clock cost on GET. 5. **Spec doc**: reflects the tolerance contract (only `set` hard-fails) and the keychain-before-disk write ordering rationale. New regression test in servers-route.test.ts covers the retry-after-503 contract directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(servers): parallel keychain writes + migration partial-state comment (#1356) Two nice-to-have observations from the re-review: - `writeKeychainEntriesFor` now `Promise.all`s its per-field sets, matching the read side. No ordering requirement among distinct (id, field) keys; on unavailability every set rejects and Promise.all surfaces the first rejection (still 503 from the route handler). - `migratePlaintextSecrets` catch block now spells out the partial-migration semantics: we return the original config so the disk plaintext stays intact, and the next successful GET retries with the keychain-wins branch absorbing already-set entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(servers): parallel keychain deletes for symmetry (#1356) Picks up the symmetry observation from the third review: `deleteKeychainFields` now uses `Promise.all` like its read and write siblings. Distinct (id, field) deletes have no ordering requirement, and `secretStore.delete` is already a silent no-op on unavailability so Promise.all has no failure-mode surprise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
5f9bfdc490 |
feat(servers): hot-reload server list on external mcp.json edits (#1345) (#1357)
* feat(servers): hot-reload server list on external mcp.json edits (#1345) Watch ~/.mcp-inspector/mcp.json from the backend and push a change event on a new /api/servers/events SSE channel. The web client subscribes via fetch + ReadableStream (so the existing bearer-auth contract is preserved — EventSource can't send custom headers) and re-fetches on each event. The watcher is lazy: it starts on the first SSE subscriber and stops when the last one disconnects, so tests that never open the channel don't spin up a real fs watcher. Self-fires from the backend's own POST/PUT/DELETE are suppressed by capturing the post-write mtime and comparing it on each watcher event. Chokidar is used for cross-platform editor-save semantics (temp-file + rename produces unlink+add sequences that bare fs.watch handles poorly on macOS/Linux); awaitWriteFinish coalesces those into a single event. createRemoteApp now returns a close() to release the watcher, which the standalone server and the vite dev plugin chain into their existing close paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review on hot-reload (#1357) - writeMcpAndTrackMtime now stat()s the file before writing; if the mtime differs from the last value we recorded, an external editor slipped in between our writes and chokidar would coalesce both into one event whose mtime matches our own write — broadcast directly so peer subscribers learn about the external edit (review finding 1, 5). - Split useServers.refresh into a public refresh() (loading toggle) and an internal refreshInternal(background) the SSE handler calls; the list now stays visible during background refreshes instead of flashing skeleton state on every external mcp.json edit (finding 2). - Coalesce multiple \n\n frames landing in one decode chunk to a single refresh call so back-to-back broadcasts don't race two concurrent GETs whose setState order is unspecified (finding 3). - Clarify createRemoteApp's close() docblock about in-flight SSE stream callbacks (finding 7). - Add integration coverage for PUT/DELETE self-suppression and for the external-edit-between-backend-writes broadcast (finding 6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address pass-2 review on hot-reload (#1357) - Clarify writeMcpAndTrackMtime comment so a future reader can't mistake the broadcast for a content-preservation guarantee: the external edit's content may already have been read-around and overwritten depending on the interleave with readMcpConfig. - Drop the no-op GET in the PUT/DELETE self-suppression tests. The pre-stat branch is short-circuited by `lastWrittenMtimeMs === null` with or without it; matches the POST test's structure. - Add explicit no-flicker regression test: tracks every `loading` value across renders during an SSE-driven refresh and asserts none flipped back to true. Catches a future regression that reaches for setLoading(true) in the background path even under React batching. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
37a4d99575 |
Port v1.5 web dev backend (vite-hono-plugin + remote-server bootstrap) (#1321)
* feat(web): port v1.5 dev backend (vite-hono-plugin + sandbox + remote-server bootstrap) Ports the v1.5 in-process dev backend into v2 so `npm run dev` serves the Hono remote-server (`/api/*`) alongside the Vite-served browser app. Without this, the browser-side `createRemoteTransport` had nothing to talk to during local development. Adds under `clients/web/server/`: - vite-hono-plugin.ts (Hono middleware on the Vite dev server; gated to `apply: 'serve'` and skipped when `process.env.VITEST` is set so it stays inert in vitest/storybook runs) - sandbox-controller.ts (standalone HTTP server for the MCP Apps sandbox) - web-server-config.ts (env parsing + initial-config payload + banner; `autoOpen` defaults off under VITEST so test runs don't shell out to `open`) - server.ts (standalone Hono prod server for the future v2 launcher, #1246) - start-vite-dev-server.ts (in-process Vite starter for the future launcher) - vite-base-config.ts (shared optimizeDeps exclusions) Wires `honoMiddlewarePlugin(buildWebServerConfigFromEnv())` into `vite.config.ts` with `apply: 'serve'`. Adds the `open` dependency and a DOM lib + `@inspector/core/*` path alias to `tsconfig.node.json` so the dev-backend files type-check alongside vite.config.ts. Coverage: web-server-config.ts (100% lines), sandbox-controller.ts (91% lines), and vite-base-config.ts (100%) all clear the per-file gate. The Vite plugin, prod server, and dev-server starter are excluded from coverage as runtime glue exercised by `npm run dev` end-to-end (callers covered separately; their logic lives in core/mcp/remote/node, which integration tests already exercise). Closes #1320. Unblocks #1244 (Wire App.tsx) — its test plan can now run end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(web/server): address PR review feedback on the dev-backend port - sandbox-controller: resolve start() with empty values on listen error (e.g. EADDRINUSE on a fixed MCP_SANDBOX_PORT) instead of leaving the promise pending forever. Without this, the Vite plugin's `await sandboxController.start()` could hang the entire dev backend on a port collision. Banner now silently omits the sandbox line; getUrl() keeps returning null. Adds a regression test exercising the EADDRINUSE path against a real socket. - vite-hono-plugin: listen for `error` and `close` on the incoming request stream (in addition to `end`) so an aborted POST/PUT (browser navigates mid-upload) doesn't leak the middleware indefinitely. - vite-hono-plugin: add a `.catch()` to the initial `pump()` kickoff so a sync throw before the first `await reader.read()` can't surface as an unhandled rejection. - sandbox_proxy.html: drop the stale CSP comment — v1.5's sandbox HTTP server never set CSP either; this PR doesn't regress anything. Replace with the actual isolation model (iframe sandbox attribute + origin allowlist) so future readers don't assume a CSP header is enforced. - Hoist `WebServerHandle` to a shared `server/types.ts` so the future launcher port (#1246) doesn't need to pick one of two duplicate declarations. - Extract `resolveAutoOpen()` helper out of the triple-nested ternary in `buildWebServerConfigFromEnv` for legibility, with the three-way logic documented in one place. - Add an inline comment explaining why `webServerConfigToInitialPayload` falls back to streamable-http for unknown discriminators (forward-compat with future SDK transports) rather than throwing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
09e1564672 | ||
|
|
a09f2dcf1d |
||
|
|
73d9b181fe |
||
|
|
19a6df7ad3 | ||
|
|
13dafe457b |
||
|
|
037225a9fb |
||
|
|
5f9bfdc490 |
||
|
|
37a4d99575 |