Author	SHA1	Message	Date
cliffhall	decc673a5d	fix(auth): address Copilot review round 5, and land the impl its tests need ... Repairs origin as well as answering the review. 2d2c787e committed the `absorbFileSecretsIntoKeyring` tests without the implementation they import, so the branch has been failing `tsc` since; this commit brings the source across. Round 5's four findings (two inline, two suppressed): - **The stale-lock takeover was still racy.** Round 4 made *stealing* liveness-aware but left the takeover as remove-then-recreate, which is not an election: two waiters can both judge the same lock stale, the first removes it and takes a fresh one, and the second then removes *that* — both inside the section, which is the lost update the takeover exists to prevent. Claiming is now an atomic `rename` to a token-unique name, so exactly one waiter can take a given stale lock and the losers get ENOENT. A failed claim also falls through to the deadline instead of `continue`ing, so a takeover that can never succeed times out rather than spinning forever on a swallowed error. - **Every `stat` failure was read as "no file yet".** An existing file we cannot inspect (EACCES on the directory, a mode-less filesystem) then produced no warning at all, and the footer went on stating owner-only permissions having verified nothing. There is now a third state, `unknown`, carried end to end: `permissionsUnknown` on the descriptor, a caveat that says the permissions could not be checked, a `warn` tone, and its own branch in the footer sentence — which otherwise would have claimed "Owner-only permissions." by omission, the same bug one layer up. - **mountinfo decoded only `\040`.** The kernel escapes tab, newline and backslash too (`\011`, `\012`, `\134`), so a mounted path containing any of them was compared in its escaped form, matched nothing, and read as the container's writable layer — `memory` selected and persistence silently lost on a box that had arranged for it. Decoded as octal in general now, since the rule is octal-in-general and a future addition would otherwise reintroduce exactly this. - **File-backed secrets vanished once a keychain appeared.** Save secrets without libsecret, install libsecret, and the next run selects the keychain and stops seeing them: still on disk, read by nothing, and nothing looks broken. `absorbFileSecretsIntoKeyring` hands them over on the run that finds a keychain, under the rules the two existing plaintext migrations already use — keychain wins on conflict, the file is removed only on complete success, and a file that cannot be read is never deleted, because that is precisely the case where the values would be lost. Tests: a two-waiter race over one stale lock asserting a single holder throughout; `unknown` from `readSecretFilePermissions` through the descriptor to the footer sentence and tone; the hand-off's happy path, keychain-wins, write-failure, undecryptable-file, empty-file, malformed-key, and explicitly-configured-keyring cases; and six more rejected descriptor shapes, including each file-only field on a non-file kind. Full `npm run ci` green — 5,732 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 14:15:35 -04:00
cliffhall	2d2c787e22	style(web): tighten the in-memory caveat and punctuate the footer ... Reads "Memory (this session only): Secrets are not written anywhere and are lost on exit." — shorter tail than "lost when the Inspector exits", and a colon separating the store from what follows it. The colon is added at the footer's composition, not inside `secretStorageLabel`. The label is also consumed alone by `secretStorageSummary`, which appends " at <path>" for a file-backed store, so a colon baked into the label would print "File (encrypted): at /path" in the startup banner. It renders only when a detail actually follows. The caveat string is shared with the banner's warning line, so the terminal picks up the shorter wording too. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 13:56:13 -04:00
cliffhall	0cb59cf3fd	fix(auth): stop heartbeating a lock that is no longer ours ... Found on a re-read of the round-4 liveness work, not by review. The heartbeat refreshed the lock directory's mtime unconditionally. After a steal — possible when this process looked dead, or across hosts where the pid check cannot apply — the directory belongs to someone else, and touching it keeps *their* lock looking alive. If they then die, the stale timestamp nobody is maintaining is being maintained by us, and a third writer can never break in: a stale lock turned unbreakable, which is the permanent outage the stealing exists to avoid. The heartbeat now re-reads the owner stamp and clears itself when the token no longer matches, so it only ever refreshes a lock we still hold. The release path already checked the token; this closes the same hole on the other side. Tested by stamping a different owner onto a held lock and aging it: the mtime stays old across several heartbeat intervals, and release leaves the thief's lock intact. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 13:50:40 -04:00
cliffhall	050556e1a7	fix(auth): address Copilot review round 4 on the secret-store PR ... Four findings (two inline, two suppressed), all taken. Three are the lock, which is where the remaining sharp edges were. - **A lock could be created and then never stamped.** If `mkdir` succeeded but writing the owner token failed, the catch treated it as "cannot lock here", returned the no-op release, and left the directory behind. The mutation then ran unlocked *and* every later writer queued behind an orphan nobody could deliberately release, until it aged out. The acquisition now tracks whether it created the directory, removes it on a failed stamp, and refuses rather than entering the section unlocked. - **An old mtime was being treated as proof the owner was dead.** It is not: a holder that is SIGSTOPed, starved by a blocked event loop, or on a machine that suspended is alive and not running, and expiring it on elapsed time alone takes the lock out from under a live critical section — the lost update the lock exists to prevent, arrived at through the machinery meant to prevent it. The heartbeat narrowed the window; it could not close it, because the holder that most needs protecting is precisely the one not running its timers. The owner stamp now carries `pid` and `host`, and staleness only *opens* the question — on the same host a waiter asks the OS whether that pid still exists (`kill(pid, 0)`, which sends no signal; `EPERM` counts as alive, so a holder running as another user is never stolen from). Only a genuinely absent process is stolen from. Where the stamp cannot be trusted — another machine over a shared filesystem, or a missing/corrupt owner file — the mtime remains the answer, because the alternative is a lock nothing can ever break, turning one dead process into a permanent outage. - **Podman was classified as a host.** `/.dockerenv` is Docker's marker and rootless Podman has no equivalent of it, while its cgroup paths say `libpod-<id>.scope` rather than the literal "podman" the pattern looked for. Such a box fell back to a file in the ephemeral writable layer and reported it as durable — the wrong way round to be wrong. `/run/.containerenv` and `libpod` are both recognized now. - **Two tests floated a second `acquireLock`**, so the test could finish and `afterEach` could remove the temp directory while the acquisition was still running. Both hold and settle it now. Separately, a defect of my own found while re-reading round 3: `describeFileStore` called `tightenSecretFilePermissions`, so the function whose job is to *describe* the store issued a `chmod` on every `GET /api/config` — once per page load, on a file another process may be mid-write on. Split out a read-only `readSecretFilePermissions`; the repair stays at startup, where it happens once and its outcome is announced. Tests: a stale lock whose owner is alive is not stolen (and survives the attempt), one whose pid is gone is, one stamped by another host is; a failed ownership stamp reports and leaves nothing behind; Podman via both signals; and the read-only permission check leaves a 0644 file at 0644. Full `npm run ci` green — 5,711 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 13:40:32 -04:00
cliffhall	b143dd6cbd	fix(auth): address Copilot review round 3 on the secret-store PR ... Nine findings (six inline, three suppressed), all taken. Two were serious. **The memory fallback made the migrations destructive.** Both migrations — mcp.json in the remote server, client.json in `node-persistence` — lift a plaintext secret off disk, write it to the secret store, and then delete it from the file. That trade is safe only while the store is at least as durable as the disk, which held by construction while `InMemorySecretStore` was a test double. Making it a production fallback broke it: on an unmounted container a plain `GET /api/servers` would have moved a user's existing secrets into RAM and lost them at exit — destroying data on a read. `SecretStore` gains an optional `isDurable()`, absent meaning durable, and both migrations withhold the disk delete when it answers false (the value is still loaded, so the session behaves normally). The production fallback is now a named `SessionSecretStore` rather than the bare `InMemorySecretStore`, so the suite's many doubles keep standing in for a *working keychain* — which is what they are there to be — and only code that deliberately chooses RAM in production says so. **Round 2's per-call descriptor never reached anyone.** Both web servers resolved it once at startup and baked it into `initialConfig`, which `/api/config` serves unchanged — so the freshness fix was inert for its only consumer. The route now takes a `secretStorageResolver` and calls it per request. The lock needed three more fixes, and they compound: - **Release removed whatever directory was at the path.** A holder whose work outran the stale threshold would delete the *next* owner's lock, letting a third writer into a section two processes believed they held. Ownership is now stamped with a random token and release only removes a lock still carrying it. - **The timeout fell through to an unlocked write** — under contention, the one moment another writer is provably active. It now throws `SecretFileLockTimeoutError`, which `set` is allowed to do. - **The first write of every process bypassed the lock**, because the secrets directory does not exist yet and `mkdir` failed ENOENT. The parent is created first. First writes are exactly when two processes are most likely to start together. Fixing the timeout exposed a fourth: with stale (10s) below timeout, a waiter stole from a holder that was merely *slow* rather than dead — reinstating the lost update. A holder now heartbeats its lock, so "stale" means the holder is gone, and the timeout is what a live-but-slow holder produces. Lock timings are injectable so the tests can exercise contention, staleness, and timeout without spending the real intervals. Also: the loose-permission state is carried into `SecretStorageInfo` as `looseMode` and rendered as the caveat (it outranks the encryption caveat — a file others can read is a live exposure, while "unencrypted" is a property of a file only its owner can open), rather than living in a startup log the browser cannot see; and `usableSecretStorage` validates `pendingEncryption` and `looseMode` as the optional booleans/numbers they are (`pendingEncryption: "false"` is truthy and would have printed advice that is the opposite of the truth) and rejects file-only fields on a keychain or memory descriptor. Tests: both migrations against a session store with a durable control alongside, so the guard is visibly the difference; `/api/config` serving a changing resolver and falling back without one; lock ownership after a steal, the timeout under a live holder, no-steal from a slow holder, and a locked first write; `looseMode` through the descriptor, the caveat ordering, and the tone; and five more rejected payload shapes. Full `npm run ci` green — 5,704 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 13:19:56 -04:00
cliffhall	de7795ca54	fix(auth): address Copilot review round 2 on the secret-store PR ... Seven findings, all taken. - **The descriptor went stale after the upgrading write.** Round 1 made it read the file instead of the policy, but it was still computed once and cached beside the selection — so the first `set` under a newly-set passphrase encrypted the file while `/api/config` kept serving "still unencrypted" for the rest of the session, describing bytes this very process had just changed. The *selection* stays cached (the banner, the payload, and the store doing the writing must agree on which store it is); the file descriptor is re-derived per call, which costs one read of a small file per config fetch. - **The mount check asked the wrong question.** Comparing a directory's `st_dev` against its parent's detects a mount *at* that path, but a volume mounted at `/home/node` leaves `/home/node/.mcp-inspector` an ordinary subdirectory sharing its parent's device — so a durable container setup was judged unmounted and demoted to session-only `memory`. It now reads `/proc/self/mountinfo` and asks whether the directory is *under* any mount other than `/`, which also catches the same-device bind mount no `st_dev` test can see. Where mountinfo is unreadable (macOS, Windows, a `/proc`-less container) it falls back to the device comparison, now against `/` rather than the parent. - **A decrypted payload was cast, not checked.** GCM proves the bytes are authentic, not that they parse to the right shape. An array passes `typeof === "object"`, takes the named assignment, and then `JSON.stringify` drops every named property of an array — so `set` would resolve having written a file without the secret it was handed. Silent loss on the one operation whose entire contract is that it does not lose things. Both branches now validate a non-array object of string values, and a shape refusal is deliberately not rethrown as a key mismatch, which would send the user to fix a working passphrase. - **The read-modify-write cycle was only serialized in-process.** A durable file is shared state and a second Inspector on the same box (a CLI run beside a web session) could read the same map and overwrite the first one's save, both reporting success. Mutations now take a `mkdir`-based lock. A lock left by a killed process is stolen after 10s — one `docker kill` must not brick secret writes permanently, which would be a worse failure than the race. If the lock cannot be created at all, the write proceeds unlocked rather than denying a real user a real secret to prevent a race with a process that may not exist. Reads do not lock: `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **The permission repair was fire-and-forget while the docs promised 0600.** A file owned by another user, or on a read-only mount, stayed exposed while the footer described it as protected. It now verifies the resulting mode and warns, naming the actual mode, when it could not be tightened. - **`Group`'s theme carried a domain-named `secretStorageFooter` variant**, which AGENTS.md reserves for app-wide primitive styling. Renamed to the structural `stickyModalFooter`; the secret-storage meaning stays in the element that owns it. - The PR description still claimed the salt made a short passphrase safe, contradicting the corrected comment and README. Updated. Tests: both mount layouts that motivated the change (mount-at and mount-above) plus the sibling-prefix, space-escaped, first-run, and no-mountinfo cases; the array and non-string refusals on both the plaintext and the authentic-ciphertext paths (the latter through a hand-built fixture, since the store's own writer cannot produce that state); lock exclusion, stale-lock stealing, the un-lockable fallback, and two independent store instances racing; the permission report in all three states; and the descriptor no longer going stale within one process. Full `npm run ci` green — 5,688 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 12:51:14 -04:00
cliffhall	3452a63101	fix(auth): address Copilot review on the secret-store PR ... Six findings, all taken. - The descriptor reported the *write policy*, not the file. Adding `MCP_INSPECTOR_SECRET_KEY` to an install that already has a plaintext file flipped `store.encrypted` immediately, so the banner and footer claimed "File (encrypted)" while the existing secrets sat readable until the next `set`. `FileSecretStore.readOnDiskEncryption()` now reads the envelope (never the payload, so it needs no passphrase) and the descriptor is built from that. The transitional state gets its own `pendingEncryption` flag, which changes the advice without softening the verdict — "set a passphrase" is the wrong thing to tell someone who already has, so the caveat names the pending write instead. - The secrets file ignored the configured storage directory. A container mounting only that directory was judged unmounted, fell back to `memory`, and lost secrets the user had arranged to keep. `defaultSecretFilePath()` now follows `MCP_STORAGE_DIR` — the variable that already relocates OAuth tokens and `client.json` — between the explicit `MCP_INSPECTOR_SECRET_FILE` and the `~/.mcp-inspector` default. (The issue comment called it `MCP_INSPECTOR_STORAGE_DIR`; no such variable exists.) - `version` was never checked before the plaintext branch, so a `{ version: 2, encryption: "none" }` file was read and silently rewritten as version 1, discarding whatever the newer format added — the same destroy-to-satisfy-an-additive-request case the encrypted branch already refuses. Checked first now, so the answer no longer depends on which encryption mode the newer writer chose. - The doc comment claimed a per-file salt made "a short, memorable value safe to use". It does not: the salt defeats precomputation, not guessing, and the scrypt cost is deliberately a few milliseconds because it runs on every read and write. Both the comment and the README now say to use a high-entropy value, generated rather than chosen. - `usableSecretStorage` validated only `kind`, so a partial `{ kind: "file" }` was cast through and — because a missing `plaintext` is falsy — rendered as the quiet, neutral *encrypted* file. That is the confident half-answer the guard exists to reject, produced by the absence of information rather than by any claim the backend made. `reason`, `durable`, and each kind's own required fields are checked. - The footer's inline `Tooltip` carried four static props; extracted to a `CaveatTooltip` constant per the `.withProps()` rule. Tests cover each: the policy/file disagreement in both directions and the no-file-yet fallback, `MCP_STORAGE_DIR` and its precedence against `MCP_INSPECTOR_SECRET_FILE`, the version refusal (asserting the bytes survive it) split from the unrecognized-cipher refusal it used to be conflated with, `readOnDiskEncryption` including the keyless and unparseable cases, five new rejected descriptor shapes, and the changed caveat. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 12:26:33 -04:00
cliffhall	bc408f5869	feat(auth): file-backed and in-memory SecretStore for hosts with no keychain ... Closes #1950 Before this, a box with no reachable OS keychain — the published container (no D-Bus session, #1848), Android/Termux (no platform binary, #1905), a minimal Linux install without libsecret — could not persist an OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore` degraded into a store that silently holds nothing: `get` returned null, `delete` no-opped, and `set` hard-failed with a 503. That is the correct failure behavior, but it left the documented, supported way to run the Inspector with no secret persistence whatsoever. Adds the two missing implementations and, more importantly, the policy that picks one and the surfaces that say which was picked. - `FileSecretStore` — one `0600` JSON document at `~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever `MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The whole map is encrypted as a unit rather than value-by-value so the account names — `${serverId}:${field}` — are hidden too; a per-value scheme would leave a readable index of which servers you hold a client secret for. A file that can no longer be decrypted reads as empty (matching the interface's read tolerance) but *refuses to be written*, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - `secret-store-selection.ts` — the policy. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright; otherwise probe the keychain and, if unreachable, fall back loudly. The fallback is `memory` in a container whose secrets directory is not on a mount, and `file` otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `--rm` will discard. Cached per process so the banner, `/api/config`, and the store doing the writing cannot disagree. - Automatic, but never silent. The downgrade prints at startup, rides `GET /api/config`, and is named in a permanent 32px footer at the bottom of both dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is the thing a user dismisses before doing the work it describes. The plaintext and memory cases render their caveat inline in a warning tone — a warning you must hover to discover is not one. - `SecretStoreUnavailableError` is now the base every store's `set` throws, so the routes translate one condition into a 503 instead of matching `KeychainUnavailableError` by name. Docs: the container section of the root README gains a table of which store you get and how to make secrets survive, plus the `MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules. Tests: the existing interface contract block is extracted to `secretStoreContract.ts` and run against all three implementations, so none can drift on the availability contract. `FileSecretStore` is tested against a real temp filesystem — mode, cross-instance round-trip, that the ciphertext does not contain the plaintext, the read-modify-write race, and every refusal path. Full `npm run ci` passes, per-file gate included. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-22 12:06:31 -04:00
cliffhall	8d6803953c	perf(auth): gate the override wrapper on the discovery request, and guard disk values ... Two suppressed review findings. With an override configured, the wrapper cloned and parsed every successful application/json response — every tools/call result and every resource payload on the direct CLI/TUI transports — only to conclude it was not a metadata document. It now tests the request URL for the RFC 8414 / OIDC well-known paths first, so ordinary traffic costs one string test and is returned untouched. The read path also trusted StoredMCPServer's `string` type for the two override fields, which a hand-edited mcp.json does not honor: the CLI and TUI read the file directly and then `.trim()` these, so a number would crash the server load. Guarded with cleanEndpointOverride, mirroring cleanAuthorizationParams. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-16 22:11:16 -04:00
cliffhall	e5c20608bf	fix(auth): never read a stream, and say what the issuer override does not do ... Two review findings on the fetch wrapper: The content-type test was a substring match for "json", which also matches `application/x-ndjson` — how the streamable-HTTP transport can serve its long-lived server-push channel (`isLongLivedStreamResponse`). Awaiting `.json()` on a clone of an unbounded stream never resolves, so the MCP connection would hang for as long as an override was configured. It now matches the media type exactly (`application/json` or a `+json` suffix), with a regression test driven by a stream that never closes. The overrides also leave `issuer` as discovery returned it, so pointing one at an authorization server that advertises a different issuer is rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That check is deliberately not weakened; the limit is now stated on the module, in the field description, and in the configuration guide. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-16 21:52:09 -04:00
cliffhall	ac8a08e42f	fix(auth): only rebuild the response when an override is actually applied ... Two review findings on the fetch wrapper: Reading the body with `response.text()` meant every successful JSON response was rebuilt while overrides were configured, even when it was not a metadata document. That dropped native properties a synthesized `Response` cannot carry (`url`, `redirected`, `type`) from responses the wrapper has no business altering, and threw outright on a body-less 204/205, whose status Fetch forbids a body on — so an unrelated transport response could fail. It now parses a clone and returns the caller's own response unless a document is actually patched. `new URL` also accepts embedded credentials while Fetch rejects a request URL carrying them, so `https://user:pass@host/token` passed both the form and the runtime and then failed deep in the OAuth exchange. `oauthEndpointUrlError` now rejects a username or password. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-16 21:30:32 -04:00
cliffhall	05698647f8	fix(auth): never apply endpoint overrides on the enterprise-managed leg ... The overrides are per-server configuration for the *resource* server's authorization server, but the fetch wrapper that applies them is shared with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs through the same fetch and satisfies the metadata predicate. An authorization override could therefore redirect the IdP login to the resource AS, and a token override could send the IdP code or refresh token there. OAuthManager.getEndpointOverrides() now returns nothing when the server is enterprise-managed — the same boundary redirectToExternalAuthorization draws for the custom authorization parameters (#2018) — with a regression test, the note surfaced on both form fields, and the rule documented. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-16 21:13:51 -04:00
cliffhall	364d8c9dbc	feat(auth): per-server authorization and token URL overrides ... Adds two optional per-server fields — `oauth.authorizationUrl` and `oauth.tokenUrl` — that override the endpoints authorization-server metadata discovery resolved, so a server can be pointed at a development or staging authorization server without changing what it advertises. SDK v2 routes both endpoints through the discovered metadata document and neither through OAuthClientProvider, so the override is applied by wrapping the client's base fetch and patching the metadata document in flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather than only the auth fetch — also covers the discovery the SDK runs from inside the transport on the 401/refresh path. The fields round-trip through mcp.json, the /api/servers routes, the runner (CLI/TUI), and the Server Settings → Authorization panel, where a value that is not an absolute http(s) URL is flagged inline; at runtime such a value is dropped with a single warning rather than failing the connection. Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-16 20:36:24 -04:00
cliffhall	b25ece8ded	feat: per-server custom OAuth authorization-request parameters (#2018) ... Real deployments gate IdP selection, prompt behavior, or audience on a query parameter the OAuth/OIDC core specs never standardized (Keycloak's `kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's `audience`), and none of them could be driven from the Inspector. The Inspector never builds the authorize URL — the SDK's `auth()` does, and it exposes no hook for extra parameters — so the merge happens at the one seam that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`. It lives in `core/`, so web, CLI, and TUI all inherit it. - `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`, `code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`, `response_type`, `scope`, `state`), the key validator the form renders inline, and the merge itself. Reserved keys are dropped with a warning rather than silently overriding PKCE / the state binding / RFC 8707; blank-key rows are skipped; the caller's URL is never mutated. - Applied to the authorization request only — nothing wires it into the token request. - Persisted as `oauth.authorizationParams` (a string record) on the per-server config beside `scopes`, edited as key/value rows in Server Settings -> Authorization ("Additional authorization parameters"), with an inline error per offending row plus a section-level warning. - The enterprise-managed (EMA) leg redirects to a *different* authorization server (the enterprise IdP), so it goes through the explicit `redirectToExternalAuthorization` entry point, which skips the merge — a `kc_idp_hint` or `audience` meant for the MCP server's AS must never be appended to the IdP's authorize URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com>	2026-08-15 19:31:41 -04:00
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>	2026-08-08 18:15:20 -07:00
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>	2026-08-08 17:30:16 -07:00
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>	2026-08-01 20:13:30 -04:00
Cliff Hall	7784553674	fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) (#1810) ... * fix(auth): offer explicit re-authorize when callback state is lost (#1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(auth): detect the SDK mismatch error via isInstance (#1808) The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares that brand in a `static {}` block, so it lives on the constructor and is never reachable from an instance. The predicate always returned false, so `findIssuerBindingFailure()` never matched and the #1808 recovery banner was inert in production — the callback dead-ended exactly as before. Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand set), with a `name` fallback for a serialization boundary. Correct the module doc, which cited cross-bundle safety as the reason to avoid `instanceof` — the very thing the SDK's branded `hasInstance` provides. Every fixture now constructs a real `AuthorizationServerMismatchError` instead of a hand-rolled object with an own `mcpBrand`, which is why a green `npm run ci` hid this. Adds a test pinning the brand's placement so the feature cannot silently go inert again. Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove the duplicated banner title default, resolve the banner title/action label where the banner is raised, keep the clear-failure notification open, and bound remote-supplied issuers in user-facing copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 18:08:31 -04:00
Cliff Hall	01df9d81bb	fix(core): start OAuth on a modern/auto-era connect (#1805) (#1809) ... * fix(core): start OAuth on a modern/auto-era connect (#1805) The SDK's `server/discover` negotiation probe runs before anything else on `protocolEra: "auto" | "modern"`, and its classifier reports whatever the transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved to `data.cause`. That buried both connect-time auth signals: the remote path's `AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the authorization URL) and a direct transport's `AuthChallengeError`. An OAuth-protected server that authorized fine on the legacy era instead produced a dead-end "Version negotiation probe failed" — no redirect, and the connection status flipped to `error` despite `isConnectAuthRecoveryError` existing to prevent exactly that. - `findNestedAuthError()` walks `cause` / `data.cause` for either typed error. Not gated on the SDK's error code or message, so it survives rewording. - `connect()` unwraps the rejection at the earliest point, ahead of `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the outer status guard, and every client's connect error handling — so web, CLI, and TUI are all fixed without a client-side change. - On the direct path, enable `interceptAuthChallenges` for the probing eras even with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely, no status and not even a cause for the walk to find. Verified against a live dual-era server that answers 401 on both `initialize` and `server/discover`: `auto` and `modern` now reject with `AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the fix is load-bearing — reverting either fails the new integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om * docs(core): mark the era-probe challenge interception as a removable workaround Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as #1807). Make its lifespan explicit so whoever picks up #1807 knows this block is the thing to delete, rather than having to re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): resolve connect() when auth recovery is satisfied silently Review findings on #1809: - connect() built its handshake promise once, outside the runConnect closure withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg re-awaited the already-rejected promise and rejected a connect() whose client was in fact connected. Short-circuit the retry when already connected, and skip the post-connect block the nested connect already ran (a duplicate `connect` event re-triggers every list-state manager's refresh). - Document the accepted 403 side effect of enabling interceptAuthChallenges with no stored tokens, and why every `{ pin }` counts as probing. - Replace the private-method double cast in the era-probe test with an observable assertion on the transport options, and split the non-auth failure case per era instead of a message alternation that passed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(core): key the connect retry leg off attempt, bound nested recoveries Second-pass review findings on #1809: - The satisfied-recovery short-circuit keyed off live connection status, so an onclose landing between the nested connect() and the retry leg fell through to the rejected handshake promise and reported the stale 401 as the reason the session died. withDirectAuthRecovery now passes `attempt` to its operation, so connect() identifies the retry leg structurally and reports the teardown when the recovered session is already gone. - Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a satisfied outcome recovers via a nested connect() with a fresh counter, so a server that keeps rejecting freshly refreshed tokens recursed without bound. Counted across the nesting boundary, capped at 3. - Strengthen the regression test (server info populated by the nested connect, no duplicate statusChange) and add the two missing cases: a failed reconnect still rejects, and the recursion bound terminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * test(core): cover the recovery teardown branch; fix a duplicated case Third-pass review findings on #1809, both about tests added in the previous commit: - The retry leg's teardown error had no coverage, so a regression to the old status fall-through would have stayed green. Covered deterministically: the nested connect dispatches `connect` after setting "connected", so closing the live transport from that listener flips the status before the retry leg runs. Asserts the error identity, which is the point of the change. - "still rejects when the reconnect underneath the recovery fails" used a challenging transport for every connect, so it walked the same bounded chain as the recursion-bound test rather than covering a non-auth reconnect failure, and its comment claimed an "error" status the auth path never reaches. Now fails the reconnect with a plain error and asserts both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: rinormaloku <rinormaloku37@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 15:06:01 -04:00
Cliff Hall	cea22d86b8	feat(web): enforce localhost-only binding; expand loopback origin allow-list (#1796) ... Closes #1795. Enforce localhost-only binding in code (the "never 0.0.0.0" rule was only a spec note): refuse an all-interfaces HOST — across every spelling the OS binds as the wildcard (inet_aton decimal/octal/hex, IPv6, IPv4-mapped, zone-scoped, IDNA/Unicode) — unless DANGEROUSLY_BIND_ALL_INTERFACES=true (Docker's sanctioned opt-in). Fixes the reported stdio-connect 403 by expanding the default loopback origin allow-list to all three interchangeable forms. Hardening surfaced across an extended review: closes an IDNA/fullwidth wildcard bypass, a blank-ALLOWED_ORIGINS fail-open, and the CLI/TUI OAuth-callback wildcard bind (loopback-only now, RFC 8252); canonicalizes the origin allow-list the way a browser does; derives frame-ancestors from the real embedder list (IPv6-literal exclusion verified in headless Chromium); strictPort on all three bind points; and preserves the CLI exit-code map + JSON envelope through the mcp-inspector --cli launcher path. Shared host normalization lives in core/node/hostUrl.ts. Docs + tests + smokes throughout. 🤖 Generated with [Claude Code](https://claude.com/claude-code)	2026-07-26 22:15:10 -04:00
Bob Dickinson	33e88eac9f	CLI: catalog listing, auth flags, and OAuth browser UX (#1782) ... * feat(cli): catalog listing, auth flags, and OAuth browser UX Add servers/list and servers/show, --relogin / --stored-auth-only, browser OAuth navigation with OSC 8 links, and extract method handlers under clients/cli/src/handlers. Tracks #1781. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): address #1782 review — relogin clear, stored-auth-only, docs Clear both raw and normalized OAuth keys on --relogin; check the shared store before bailing under --stored-auth-only mid-session; restore the tests README link target; document servers/show redaction limits; keep mcpi groundwork with TODO(#1432) markers. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): gate OAuth browser open and harden #1782 review follow-ups Arm auto-open only for CLI-owned interactive OAuth, honor MCP_AUTO_OPEN_ENABLED, move --relogin clear after validation, skip keychain on servers/list, and cover secret redaction after rehydrate. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): silence disarmed OAuth URLs and round-3 review nits Suppress authorize URL print when navigation is disarmed or --stored-auth-only; document MCP_AUTO_OPEN_ENABLED; redact Cookie/auth headers; keep servers/show unit tests off the OS keychain. Co-authored-by: Cursor <cursoragent@cursor.com> * test(cli): cover withArmedAutoOpen for the ≥90 branch gate CI failed on cliOAuth.ts branch coverage after the auto-open arming helper landed; exercise the armed path through runCliInteractiveOAuth. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): mid-RPC unauthorized recovery and clear round-4 nits Mirror connect's UnauthorizedError path in withCliAuthRecoveryRetry, let MCP_AUTO_OPEN_ENABLED=true force open off-TTY, mock open-url in the in-process runner, reject --relogin on catalog methods, restore the __tests__ README inventory, and narrow OAuth helpers to structural client types. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): reconnect after mid-RPC OAuth and finish round-5 nits Reconnect before retrying after unauthorized recovery, guard non-OAuth configs, decouple forceAutoOpen, move open-url mock to vitest setupFiles, and reject --relogin with stored-auth short-circuits. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): fail fast on non-TTY interactive OAuth and round-6 nits Skip interactive OAuth without a TTY unless MCP_AUTO_OPEN_ENABLED=true; document --stored-auth-only for CI; redact settings.metadata; reject --relogin for stdio; clarify AuthRecoveryRequired reconnect comments. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): admit interactive OAuth when stdin is a TTY (round-7) Gate on stdin||stderr so `2>&1 | tee` still works; fold confirm/auto-open/ TTY into CliOAuthConnectOptions; rename authRequiredFailure; document MCP_AUTO_OPEN_ENABLED=true as non-TTY admit. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): fail fast on step-up without stdin (round-8) Decline [y/N] on readline EOF/close, require a stdin TTY for the default step-up confirmer (force-open admit is not enough), document stderr-only browser auto-open vs stdin||stderr flow admit, and drop the alias wrapper. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): allow piped step-up answers; bound silent pipes (round-9) Drop the hard stdin-TTY gate that broke `echo y | …`. Keep EOF decline and add a short non-TTY timeout so an open pipe that never writes cannot hang. Clarify isTTY as admit-only; extract the readline mock factory. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): keep partial piped step-up answers (round-10) Capture readline `line` before close so `printf y` without a newline is not treated as decline; timeout is auth_required with a distinct message; document the 5s bound and post-confirm callback wait; pin the TTY no-timeout arm. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): tighten step-up timeout messaging and TTY test (round-11) Make the TTY no-timeout assertion use a settled flag (the Promise.race check was vacuous), say "timed out" instead of "declined" on the pipe bound, and document that piped answers need a newline or stdin close. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>	2026-07-26 16:55:25 -04:00
Cliff Hall	373f346e5f	chore: close quality-gate coverage gaps (#1689) (#1766) ... Closes #1689. Typecheck cli/tui src, add a core/ format+lint gate, and gate core/json + core/client coverage. See PR #1766 for the full review history.	2026-07-24 17:20:12 -04:00
Cliff Hall	99ef731746	Refactor / justify codebase-wide 'as unknown as' casts (#1690) (#1756) ... * core: refactor serverList.ts object↔Record casts into typed helper (#1690) Remove the four pre-existing `as unknown as Record<string, unknown>` / `as unknown as MCPServerConfig` casts in core/mcp/serverList.ts: - stripInspectorFields: rewrite as clone + `delete` over INSPECTOR_FIELD_KEYS. Every Inspector-extension key is optional on StoredMCPServer, so the result is still (a subtype of) MCPServerConfig with no cast. - oauth-strip in extractSecretsFromStored: `oauth` is optional, so a plain `delete stripped.oauth` is legal — drop the cast. - mcpConfigToServerEntries: route the one genuinely-needed widening through a single documented `toRecord(value: object)` helper. Taking `object` makes the single `as Record<string, unknown>` legal (no TS2352), so the double cast disappears from the call site. Net: zero `as unknown as` in serverList.ts; the one remaining structural cast lives inside the audited toRecord helper. Behavior-neutral — existing coverage (79 serverList tests) unchanged; full `npm run ci` passes. Closes #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * core+web+test-servers: eliminate/justify remaining `as unknown as` casts (#1690) Codebase-wide follow-on to the serverList refactor: audit every `as unknown as` double cast outside tests and either remove it or ensure it carries a justifying comment (the review bar is "no *unjustified* double cast"). Removed (widened via a shared helper or narrowed to a single cast): - Promote the serverList `toRecord(value: object)` widening helper to core/json/jsonUtils.ts and reuse it at core/mcp/node/config.ts (normalize loop), core/mcp/remote/node/server.ts (pino level dispatch), and clients/web/src/App.tsx (window global lookup). - inspectorClient.rawWireRequest: type the frame as `JSONRPCRequest` and narrow only `params` with a single structural cast (the SDK's `_meta`-typed params is what blocked direct assignment). - toolOutputValidation: SDK `outputSchema` → `JsonSchemaType` is a single cast. - test-server-fixtures: the optional `task` handle is reachable with a single cast (target field is optional). Justified in place (genuinely unavoidable — private SDK internals, partial interface stubs, third-party/nominal bridges): the inspectorClient private-map accesses, resourceContext + tokenAuthProvider partial `OAuthClientProvider` stubs, the ext-apps peer bridge, modern/legacy `_requestHandlers` bypasses, and the happy-dom `matchMedia` test mock. Behavior-neutral. Full `npm run ci` passes (validate, ≥90 coverage gate, smoke, Storybook). Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * address @claude review nits: consistent cast justifications + drop reflow noise (#1690) - test-servers/src/modern-tasks.ts: add an inline justification above the `_requestHandlers` double cast, mirroring the sibling in composable-test-server.ts (was leaning on the function JSDoc only). - Annotate the partial-`AppBridge` mock double casts in the story fixtures (AppsScreen ×2, AppRenderer, InspectorView) so no non-test-file double cast is unjustified. - inspectorClient.ts: revert unrelated Prettier reflow hunks (protocolVersion, task-content array, timeout-reject wrap, reconnect guard) so the diff is the single rawWireRequest cast change only. Behavior-neutral (comment/format-only). `npm run ci` green apart from a known flaky redaction test whose assertion collided with a random request id. Refs #1690 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * actually revert the reflow noise this time (#1690) The reflow reverts in cc187ea were silently undone by a `prettier --write` I ran on the same file right after, so the four Prettier-only hunks from 6a3afa4a were still in the diff (caught in re-review). Revert them for real now — and without re-running Prettier on the file: - core/mcp/inspectorClient.ts: restore the protocolVersion line, task-content array, timeout-reject call, and reconnect guard to their origin/v2/main form, so the file's diff is only the single rawWireRequest cast change. - test-servers/src/modern-tasks.ts: restore the ModernTaskStatus union to its one-line form (the leading-`|` reflow was also Prettier churn), leaving only the new `_requestHandlers` justification comment. Formatting-only; `npm run validate` green. Refs #1690 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>	2026-07-24 09:34:32 -04:00
Bob Dickinson	a71e6d2012	Auth hardening CI gaps + SDK consolidation: EMA leg 2 / scopes (#1527) (#1725) ... Close #1527 acceptance gaps with automated tests (SEP-2468 iss, SEP-2207 offline_access, SEP-2351 discovery shapes, SEP-2352 AS-migration, EraNegotiationFailed→401 unwrap); lean EMA leg 2 and scope helpers on the v2 SDK; drop deprecated TS baseUrl. Reviewed LGTM.	2026-07-19 22:36:21 -04:00
Cliff Hall	7fecebed5e	OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1694) ... * OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1625) Re-key the persisted OAuth store to (server, issuer) and add the SEP-837/2350/2352 authorization-hardening UX. The SDK v2 auth() drives the SEP-2352 flow off the provider's ctx.issuer; the Inspector's job is to honor it and round-trip the issuer stamp. SEP-2352 — issuer-bound credentials - ServerOAuthState nests issuer-bound credentials under byIssuer[issuer] = { clientInformation, clientRegistrationKind, tokens }, with an activeIssuer pointer answering the transport's ctx-less per-request bearer read. - Fix a latent bug: OAuthStorageBase re-parsed tokens/client-info through OAuthTokensSchema/OAuthClientInformationSchema, which strip the non-RFC `issuer` field and silently defeat the SDK's discardIfIssuerMismatch. The store re-attaches the stamp from the byIssuer key. - BaseOAuthClientProvider threads ctx.issuer and implements discoveryState()/saveDiscoveryState() (activates the SDK's callback-leg AuthorizationServerMismatchError binding check) and invalidateCredentials(). - Lazy, migration-free upgrade: a legacy pre-1625 blob's top-level clientInformation/tokens are returned as an unkeyed fallback until the first issuer-stamped save promotes them into byIssuer. SEP-837 — application_type - DCR declares application_type: "native" (localhost Inspector). - ClientSettingsForm renders RegistrationRejectedError detail and reflects DCR's deprecated-in-favor-of-CIMD (SEP-991) status. SEP-2350 — step-up scope union - Per-server onInsufficientScope (reauthorize | throw) setting wired ServerSettingsForm -> StoredMCPServer.oauth -> StreamableHTTPClientTransport. - StepUpAuthModal visualizes the prior-union-challenged scope set, tagging each scope `new` vs `already granted`. mcpAuth simplification - Delete the hand-rolled authorizeWithoutRefresh (mishandled issuer stamping); forward forceReauthorization/skipIssuerMetadataValidation to native SDK auth() so both the initial and step-up paths get SEP-2352 stamping. Vocabulary + docs - OAuthStep gains cimd_fetch / issuer_comparison / scope_step_up (recorded on a step-up redirect); the Network tab labels auth requests by OAuth phase. - v2_auth_hardening.md gains an as-built status section. Deferred to the era-aware card: the SdkError(EraNegotiationFailed) -> UnauthorizedError unwrap and finishAuth(URLSearchParams) form (only fire under non-legacy negotiation). Closes #1625 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address @claude review: issuer-stamp source gating + clear/activeIssuer + phase over-match - SEP-2352: stamp the issuer only when the credential actually came from the byIssuer slot (`fromSlot`), not merely when a slot exists. A partially-populated slot alongside a surviving legacy field would otherwise stamp a legacy (possibly cross-AS) credential with the current issuer, defeating discardIfIssuerMismatch. - Issuer-scoped clears no longer promote the cleared issuer to `activeIssuer` (updateIssuerSlot gains a `setActive` flag; save paths keep the promotion). - oauthNetworkPhase matches register/token/authorize as the final path segment (trailing slash tolerated) so `/token/refresh` or `/api/register/foo` no longer over-match. - Tests for all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-16 16:52:53 -04:00
cliffhall	f4fb4b50c9	Forward RFC 9207 iss to the v2 SDK (fixes OAuth + EMA leg 1) ... SDK v2 added RFC 9207 §2.4 authorization-response issuer validation (`validateAuthorizationResponseIssuer` / `IssuerMismatchError`) as a mix-up attack defense. It is strict: when AS metadata advertises `authorization_response_iss_parameter_supported: true` and no `iss` reaches `exchangeAuthorization`/`auth()`, the exchange throws. v1.29.0 had no `iss` parameter and no RFC 9207 validation at all, so dropping the callback `iss` was harmless. The migration swapped in the stricter SDK without feeding it the new input, breaking the authorization-code exchange against any RFC 9207 authorization server (Okta/xaa.dev advertise it): Issuer mismatch in authorization response (RFC 9207): expected "https://idp.xaa.dev", received undefined Found by smoke-testing EMA against live xaa.dev staging; not caught by CI because the mock IdP never advertised the parameter, so the SDK took its lenient path (a missing `iss` is only fatal when advertised). `iss` was dropped in three places, with the plumbing half-built — `oauthManager`/`inspectorClient` already accepted `iss`, but nothing supplied or forwarded it: - `parseOAuthCallbackParams` never extracted `iss` from the callback query - `mcpAuth` never forwarded `options.iss` to SDK `auth()` — its own comment ("forwarded on v2 upgrade; ignored by v1 SDK auth()") predicted this exact gap, so standard OAuth was broken identically, not just EMA - EMA leg 1 (`completeIdpOidcAuthorization`) never passed `iss` to `exchangeAuthorization` Thread `iss` end to end: callback query -> CallbackParams -> resumeAfterOAuth -> completeOAuthFlow -> {mcpAuth | EMA leg 1} -> SDK. The Node callback server and runner carry it for CLI/TUI. Tests: `ema-mock-servers.ts` now advertises `authorization_response_iss_parameter_supported: true` so the mock is at least as strict as real IdPs — with it absent the SDK silently takes the lenient path and a dropped `iss` goes unnoticed. Flipping it made three existing tests reproduce the failure. Adds regression coverage for forwarding and for rejecting a mismatched `iss` before any network call. Verified end to end against live xaa.dev staging: EMA legs 1-3 (IdP SSO -> ID-JAG mint -> resource token) now complete and the MCP session initializes. web 3099, cli 212, tui 274 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjMemXZohSR2EndBQYif2b	2026-07-15 20:16:43 -04:00
cliffhall	f9f79fff54	Address @claude review: format core/, typed output-validation error, document progress heuristic ... - Run Prettier on the 60 core/ files this PR touched — the deterministic import rewrite had collapsed multi-line imports and left throws unwrapped; core/ is in no format gate so it slipped past CI. Now Prettier-clean. - Default-path output-schema violation now throws ProtocolError(ProtocolErrorCode.InvalidParams, …) instead of a bare Error, matching the prior v1 contract (SDK threw a typed McpError with a code) so downstream `instanceof`/`.code` branching keeps working. - Document the "assumes serial construction" invariant on pollTaskToolCall's progress-subscription revival (the `_progressHandlers` new-key heuristic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-15 11:08:54 -04:00
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>	2026-07-15 10:44:24 -04:00
Cliff Hall	a029e42a19	cli: Wave 3 — programmatic review path (rollup of #1573–#1575) (#1655) ... * feat(cli): programmatic ergonomics — initialize, tool-args-json, connect-timeout, env-var fixes Adds the #1573 programmatic-ergonomics surface to the CLI, re-implemented informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts: - `--method initialize` — connect-only probe emitting `{serverInfo, protocolVersion, capabilities, instructions}`. - `--tool-args-json '{"k":"v"}'` — tool args passed verbatim as JSON (no key=value coercion); mutually exclusive with `--tool-arg`, with clear errors for combined use, malformed JSON, and non-object values. - `--connect-timeout <ms>` — default 15000 for ad-hoc `--server-url`/target runs, file-level timeout otherwise; `0` disables. Backed by the exported `withConnectTimeout()` helper. - MCP_CATALOG_PATH is ignored when an ad-hoc target is given, so a shell that exports it can still run one-off ad-hoc invocations. - MCP_STORAGE_DIR is honored by core `getStateFilePath` (`<dir>/oauth.json`), below the per-file MCP_INSPECTOR_OAUTH_STATE_PATH override. Tests: new programmatic-ergonomics.test.ts (CLI, in-process runCli + direct withConnectTimeout unit tests) and getStateFilePath resolution cases in the web storage integration suite. Per-file coverage gate stays ≥90. README: documents the new flags and env-var semantics. Closes #1573 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * feat(cli): --app-info probe + --format json envelope Adds the #1574 app-probing and machine-readable output surface, re-implemented informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts. - `--method tools/call --tool-name <t> --app-info` — prints one JSON line describing the tool's app posture (hasApp, resourceUri, csp, permissions, domain, …) WITHOUT invoking the tool; exit 0 when it has an app, 2 (no_app) when it doesn't. A missing tool exits 5 (tool_not_found) — distinct from the no-app exit so a typo isn't mistaken for "no app". - `--method tools/list --app-info` — one NDJSON line per tool over a single connection (`| jq -c 'select(.hasApp)'`). - `--format json` — a single stdout envelope (`{result}` plus `{appInfo}` as a sibling key for App tools), no banner, so any method pipes into jq. - A tools/call returning isError:true now exits 5 (tool_is_error) after printing its payload, so && chains stop on a failed call. Implementation: `collectAppInfo()` (core `extractAppInfo` + a tolerated resource read that reports `resourceError`), an NDJSON loop in tools/list, and a single `emitResult()` that owns format + exit-code handling. `runMethod` now returns a `MethodOutcome` discriminated union so the NDJSON path can signal "already emitted". Tests: app-info.test.ts (exit-0/2/5 + NDJSON + no-invoke), format-json.test.ts (envelope shapes + bad --format), emit-result.test.ts (direct unit coverage of the isError→exit-5 and resourceError branches via exported emitResult/ collectAppInfo). Updated the tools.test "nonexistent tool" case to the new exit-5 tool_not_found contract. Per-file coverage ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). README documents the flags, NDJSON shape, and exit semantics. Closes #1574 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * feat(cli): stored-auth suite — --use-stored-auth, --wait-for-auth, --list-stored-auth, --print-handoff Adds the #1575 web→CLI OAuth handoff surface, re-implemented informed by PR #1510 and adapted to v2/main. - `--use-stored-auth` — read the shared oauth.json and inject `Authorization: Bearer <token>` for --server-url; exit 3 (no_stored_token), listing stored keys, when none matches. - `--wait-for-auth <sec>` — poll oauth.json (500ms) until a token for --server-url appears, then proceed; timeout → exit 3 (auth_wait_timeout). - `--list-stored-auth` — print `{oauthStatePath, storedServerUrls}` and exit (short-circuit, no connect). - `--print-handoff` — emit `{serverUrl, deepLink, portForwardCmd, oauthStatePath, apiToken, note}` for --server-url and exit. Adaptations vs. the reference: - State-path resolution reuses core `getStateFilePath()` (the MCP_INSPECTOR_OAUTH_STATE_PATH → <MCP_STORAGE_DIR>/oauth.json → default precedence added in #1573) instead of a CLI-local resolver. - The state file is read through the shared isomorphic `parseOAuthPersistBlob`, so both the plain `{servers,idpSessions}` layout the web backend writes and the legacy `{state,version}` envelope are accepted. - `parseArgs` now returns a ParseResult union so the short-circuit modes return without a server connection. Blind injection: the token is passed as-is; a stale one surfaces as HTTP 401 → exit 3. Documented in the README along with all four flags, the state-path precedence, and a follow-up note that --print-handoff's deepLink shape is interim pending #1576's canonical handoff format. Tests: stored-auth.test.ts drives injection, --header coexistence, missing --server-url, no-token exit-3, URL-normalisation matching, MCP_STORAGE_DIR resolution, listing, handoff (+note), wait-for-auth polling/timeout, and normalizeServerUrl units — all against a real in-process HTTP test server. Per-file coverage ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). Closes #1575 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * docs(core): scope MCP_STORAGE_DIR comment in getStateFilePath to the OAuth backend Address review of PR #1651: the doc comment claimed MCP_STORAGE_DIR was honoured "across the app, so setting it once points every persist backend at the same directory", but the branch only relocates oauth.json. Narrow the comment to state it is OAuth-backend-only (client.json/mcp.json still resolve from getDefaultStorageDir → HOME) and record why the resolution lives here rather than in getDefaultStorageDir (the default-path constant is evaluated at module load, so a runtime-set MCP_STORAGE_DIR — as the CLI stored-auth / --wait-for-auth flows set it — must be read at call time). Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review(cli): per-tool-tolerant app-info listing + drop dead helper (PR #1653) Address review of PR #1653: - collectAppInfo now never throws: a malformed `_meta.ui.resourceUri` (extractAppInfo throws) folds into `{hasApp:false, resourceError}` just like a resources/read failure. This makes `tools/list --app-info` per-tool tolerant — one bad tool no longer aborts the whole NDJSON listing with a generic exit-1. Guarding inside collectAppInfo (vs. a loop-local try/catch) keeps the branch unit-testable; added an emit-result.test.ts case for the malformed-URI path. - Documented that `tools/list --app-info` always emits NDJSON regardless of `--format` (the per-tool list shape is fixed; --format json only reshapes the single-result paths) — in both the code comment and the README. - Removed the now-unused `expectJsonError` helper (its only caller moved to the exit-5 tool_not_found contract). Coverage stays ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review(cli): stored-auth follow-ups — TODO(#1576), wait-for-auth guard test, docs (PR #1655) Address review of PR #1655: - Added an explicit TODO(#1576) on buildHandoff's deepLink: `transport:"http"` is hardcoded even for SSE, and `autoConnect=<token>` is not one of the three token sources the web app reads (per CLAUDE.md) — both to be reconciled with #1576's canonical handoff format. - Added a test for `--wait-for-auth` without `--server-url`, covering the previously-untested `--wait-for-auth` arm of the shared missing-server-url guard's message ternary. - Documented that `--list-stored-auth` / `--print-handoff` are mutually exclusive short-circuit modes (list takes precedence) and that a stored refresh_token is not yet used (noted as a follow-up in code + README). No behavior change; coverage stays ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-11 22:14:23 -04:00
cliffhall	32a3610737	Merge remote-tracking branch 'origin/v2/main' into 1579-wave-1 ... # Conflicts: # clients/cli/package-lock.json # clients/cli/package.json # clients/web/src/App.tsx # clients/web/src/lib/environmentFactory.test.ts # clients/web/src/lib/environmentFactory.ts # clients/web/src/lib/remoteOAuthStorage.test.ts # clients/web/src/lib/remoteOAuthStorage.ts # clients/web/src/test/core/auth/connection-state.test.ts # clients/web/src/test/core/auth/providers.test.ts # clients/web/src/test/core/auth/storage-browser.test.ts # clients/web/src/test/core/auth/storage-remote.test.ts # clients/web/src/test/core/mcp/oauthManager.test.ts # clients/web/src/test/integration/auth/node/storage.test.ts # clients/web/src/test/integration/storage/adapters.test.ts # clients/web/src/utils/clearServerOAuthState.test.ts # clients/web/src/utils/clearServerOAuthState.ts # core/auth/connection-state.ts # core/auth/oauth-storage.ts # core/auth/providers.ts # core/auth/storage.ts # core/auth/store.ts # core/storage/adapters/remote-storage.ts	2026-07-09 09:41:08 -04:00
cliffhall	1e6a2825d4	fix(auth): keep oauth-persist browser-safe (move Node file backend out) ... The refactor routed the browser's RemoteOAuthStorage through `oauth-persist.ts`, which value-imported the Node-only `store-io.js` (`atomically` -> `stubborn-fs` -> `node:process.getuid`, plus `node:fs/promises`). That dragged Node built-ins into the browser bundle and the web app crashed to a blank page at runtime — a regression from v2/main, where browser OAuth storage used a Zustand JSON adapter with no Node imports. Unit/integration tests run in node/happy-dom and `smoke:web` only checks the served HTML, so none of them execute the app in a real browser and the break slipped through. Fix, following the existing `store-id.ts` precedent: - Split the pure JSON (de)serializers into Node-free `core/storage/store-serialize.ts`; `store-io.ts` re-exports them so existing importers are unaffected. - Move the Node-only `createFileOAuthPersistBackend` into `core/auth/node/oauth-persist-file.ts`; update `storage-node.ts` and the storage adapters test to import from there. - `oauth-persist.ts` now imports only the Node-free serializers, so the browser bundle no longer pulls in `node:fs`/`atomically`. Tests: - oauth-persist-browser-safe.test.ts: source-level guard that `oauth-persist.ts` never imports store-io/node:/atomically. - Direct unit tests for the isomorphic remote + session backends (restoring oauth-persist.ts coverage after the file backend moved out). - store.test.ts: OAuthMemoryStore unit coverage incl. replace() fallbacks. - storage.test.ts: clearAllOAuthClientState null-snapshot (no file) path. Verified end-to-end in a real browser (Playwright): DCR OAuth flow completes, tokens persist via the remote store, and after a full page reload the server reconnects with no re-authentication. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-08 16:20:31 -04:00
cliffhall	54d5b18b1e	fix(auth): drop redundant empty-object guard in remote OAuth read() ... parseOAuthPersistBlob already returns null for {} (the server's missing-file response) and for a literal null body, so the Object.keys(store).length === 0 guard was redundant — and would throw a TypeError on a null body before parseOAuthPersistBlob could handle it. Pass the parsed res.json() straight through. Addresses Claude review item #1 on #1612. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-08 13:54:14 -04:00
cliffhall	edc7dbb1c1	Merge remote-tracking branch 'origin/v2/main' into v2/1549-oauth-storage-no-zustand ... # Conflicts: # core/auth/store.ts	2026-07-08 13:44:12 -04:00
cliffhall	ba48e06ffd	fix(auth): guard OAuth-storage async paths against unhandled rejections ... Review follow-ups on the RemoteOAuthStorage web migration: - useEmaIdpLoginState.logout(): add a .catch to the now-async clearEmaIdpSession call so a storage failure no longer surfaces as an unhandled rejection; leave loginState unchanged on failure (the session was not actually cleared). Add a test asserting no unhandled rejection. - createOAuthStore: attach a no-op catch to the eager persistLoadPromise so a hydration read that fails before any load() awaits it does not become an unhandled rejection; real awaiters still observe the rejection via the shared reference. - storage-remote.test: replace `as unknown as typeof fetch` double-casts with the typed vi.fn<typeof fetch>() pattern the same PR established. - remoteOAuthStorage: correct the doc comment to describe the cache-of-one accurately instead of implying a keyed map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-08 10:59:40 -04:00
Bob Dickinson	af174666d6	fix(auth): address Claude review on OAuth persist serialization ... Serialize persist() in OAuthStorageBase so overlapping remote POSTs cannot reorder and clobber state. Regenerate launcher package-lock.json to drop stale zustand entries. Let parseOAuthPersistBlob accept parsed objects from res.json() without a redundant JSON round-trip.	2026-07-05 18:38:13 -07:00
Bob Dickinson	3c65bdb4c6	Removed Zustand storage, cleanup up auth storage IO interfaces, implementations, and usage.	2026-07-04 21:56:45 -07:00
Bob Dickinson	512f45d357	fix(auth): reject load() when persist read fails	2026-07-03 14:50:04 -07:00
cliffhall	cfbdfdf2bd	Merge remote-tracking branch 'origin/v2/main' into 1579-wave-1 ... # Conflicts: # AGENTS.md # clients/cli/README.md # clients/web/src/App.test.tsx # clients/web/src/App.tsx # clients/web/src/test/core/auth/utils.test.ts # core/mcp/node/transport.ts # test-servers/src/composable-test-server.ts	2026-07-03 17:39:07 -04:00
Bob Dickinson	a24091484c	fix(auth): await OAuth storage load() to fix remote OAuth races	2026-07-03 14:18:00 -07:00
cliffhall	f4b6095719	test: close ≥90% per-file coverage gaps for mid-session auth ... The mid-session auth work passed `validate`/CI but failed the local-only `npm run test:coverage` per-file gate (≥90% lines/statements/functions/ branches) on 12 files. Add targeted unit/integration tests so every changed file clears the gate; annotate provably-dead defensive branches with justified `/* v8 ignore */` comments rather than relaxing the gate. Files brought to ≥90 on all four dimensions: - core/auth: mcpAuth, oauthUx, utils, challenge, node/runner-interactive-oauth - core/mcp: oauthManager, inspectorClient, node/authChallengeFetch, remote/remoteClientTransport, remote/node/{remote-session,server} - clients/web/src/utils: oauthResume, browserTabVisibility `npm run test:coverage` now passes (3575 tests, 0 threshold failures); `npm run validate` green across all four clients. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZyeWManYTvcXxjcrhxSQc	2026-07-03 12:54:37 -04:00
Bob Dickinson	a066873e37	Fixes for remaining issues	2026-07-02 18:36:44 -07:00
Bob Dickinson	52023ff1c3	fix(auth): clear OAuth callback timer and reorder EMA scope persist ... Clear the loopback callback timeout in runner-interactive-oauth finally so already_authorized and error paths do not hang the CLI/TUI. Check EMA step-up satisfaction against minted token scope before persisting the requested union, matching the standard OAuth path. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-07-02 17:55:34 -07:00
Bob Dickinson	562bdd00f1	All of the mid-session and step-up auth work	2026-07-02 16:35:31 -07:00
cliffhall	588fc3f8ea	docs(auth): note getClientRegistrationKind's hydration invariant like getScope ... Round-3 @claude review of #1592 (optional nit): getClientRegistrationKind is the other synchronous getter that, like getScope, is safe only because its sole caller (buildOAuthConnectionState) awaits getClientInformation/ getServerMetadata first, flushing hydration before this runs on the post-redirect callback path. Add a docstring mirroring getScope's so a future refactor doesn't read it without a preceding awaited storage read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX	2026-07-01 19:31:10 -04:00
cliffhall	4e149719bd	fix(auth): route clearClientInformation through clearAfterHydration too ... Round-2 @claude review of #1592 caught a seventh member of the clear family that the first pass missed: clearClientInformation still wrote to the store synchronously, carrying the same pre-hydration clobber risk the clearAfterHydration helper was written for (a clear before hydration lands persists a near-empty blob, overwriting the whole on-disk oauth.json). Wrap its mutation the same way and add a clear-before-hydration test proving a sibling token (and thus every other server's blob) survives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX	2026-07-01 19:26:25 -04:00
cliffhall	8c10b8eb8e	fix(auth): make clear* hydration-safe, migrate raw-key blobs, doc keepalive/memo caveats ... Address round-1 @claude review of #1592: - OAuthStorageBase.clear* no longer mutate the pre-hydration (empty) store — a clear issued before hydration is deferred until it lands, so it merges onto the real persisted state instead of (a) being resurrected by the late rehydrate merge or (b) persisting a near-empty blob that clobbers every other server's on-disk credential. Already-hydrated clears stay synchronous. - store.setServerState migrates a pre-normalization raw-key blob onto the canonical key on first partial write (was: shadowed it with a fresh canonical entry, orphaning the raw blob's other fields). clearServerState now also drops the raw-key orphan. - Document the keepalive 64KB combined-body ceiling on the remote-storage write, the getRemoteOAuthStorage memo key intentionally omitting fetchFn, and the real ordering invariant behind the sync getScope. - Tests for the clear-before-hydration and raw-key-migration paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX	2026-07-01 19:20:49 -04:00
cliffhall	c299b6e9d8	feat(web): migrate OAuth store to shared /store API (RemoteOAuthStorage parity) (closes #1548) ... Migrate the web client's OAuth storage from BrowserOAuthStorage (sessionStorage) to RemoteOAuthStorage, which POSTs through the backend's /api/storage/oauth route to ~/.mcp-inspector/storage/oauth.json (mode 0600). A credential obtained in the browser is now the same blob the TUI/CLI read on the same host, giving web ⇄ TUI ⇄ CLI parity (Wave 1 of #1579). Core (shared with TUI/CLI): - OAuthStorageBase now drives a single async hydration (`ready()`, `getHydrationError()`); every post-redirect read (getClientInformation, getTokens, getCodeVerifier, getServerMetadata, getIdpSession) and every save awaits it so a late hydration merge cannot clobber a fresh write. getCodeVerifier/getServerMetadata become async; the store is created with skipHydration:true so there is no auto-hydration to race the explicit one. - store.ts adds normalizeServerUrl so a token saved under https://Example.com/mcp is found when the CLI asks for https://example.com/mcp/; getServerState falls back to the raw key. - remote-storage adapter POSTs with keepalive so the write survives the OAuth authorize redirect, surfaces swallowed persist failures, and gives richer read/write error messages. - generateOAuthState throws instead of silently degrading to Math.random. - NodeOAuthStorage honors MCP_INSPECTOR_OAUTH_STATE_PATH for isolated fixtures. Provider codeVerifier()/getServerMetadata() + the EMA idpOidc and connection-state readers await the now-async storage. Web: - environmentFactory + a new shared lib/remoteOAuthStorage accessor (memoized per {baseUrl, authToken}) wire RemoteOAuthStorage into the connection path, the EMA IdP hook, and the per-server "clear OAuth" action so all three share one in-memory view of oauth.json. Tests: async-hydration + normalizeServerUrl suite, keepalive/error-path coverage, and the sync→async migration across existing storage/provider/EMA tests. Web validate + coverage gate + integration suite green; CLI/TUI/ launcher validate green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX	2026-07-01 18:39:17 -04:00
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>	2026-06-29 15:35:51 -04:00
Cliff Hall	c96db0402e	test: raise per-file coverage gate to 90 across all clients (closes #1271) (#1532) ... * test(web): audit branch-coverage outliers, raise per-file gate to 70 (#1271) The web per-file coverage gate had drifted red: the recently-landed EMA auth feature (core/auth/ema/*) and ClientSettings components shipped undertested, and the prior 50% branch floor let business-logic branches go untested. CI does not run the coverage gate, so this accumulated. Add tests to bring every failing file above the gate and push branch coverage well past 70 across the board, then raise the per-file branches threshold 50 -> 70 in clients/web/vite.config.ts. - EMA core (emaFlow, resourceContext, jwt, wire): 50-88% -> 86-100% branch - EMA OIDC/transport (idpOidc, tokenEndpoint, transportProvider): -> 97-100% - oauthManager / discovery: 73-83% -> 88-92% - ClientSettingsModal/Form, ConnectionInfoContent: 0-88% -> 96-100% - useClientSettingsDraft / useEmaIdpLoginState hooks: 20% -> 100% - TaskStatusBadge / SortableServerCard outliers: 50% -> 100% Coverable branches reached by mocking useComputedColorScheme (dark theme), dnd-kit useSortable (isDragging), and fetch/OIDC paths. Remaining sub-100 branches are genuinely-unreachable defensive guards or happy-dom-inherent (portal / useMediaQuery / typeof window) and documented in test comments. No source files changed. Global web branch coverage 88.73% -> 90.63%. AGENTS.md updated; cli/tui/launcher stay at 50 (out of scope). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: raise per-file coverage gate to 90/90/90/90 across all clients Follows the #1271 web branch-coverage audit (50 -> 70) by raising the whole per-file gate to >=90 on all four dimensions — lines, statements, functions, branches — for web, cli, tui, and launcher (core/ is measured under the web suite). A codebase-wide audit added real tests for every outlier and brought global coverage up across the board. Config: all four vitest/vite coverage configs set to 90/90/90/90. Two pure-type modules (core/mcp/import/types.ts, clients/web/server/ types.ts) added to the web coverage exclude list, matching the existing type-only exclusions. Tests: ~50 test files extended/created across core (auth, ema, mcp, remote, state, storage, react), web components/screens/server, and the cli/tui clients. Highlights: full EMA OAuth flow + transport coverage, inspectorClient/remote server backfill, every Mantine control's handler branches, and the cli unsupported-method / argv-fallback paths. Genuinely-unreachable branches are annotated at the source with justified `/* v8 ignore -- <reason> */` comments rather than lowering the gate: happy-dom-inherent paths (Mantine portal, useMediaQuery, typeof window SSR guards), React StrictMode effect replay, and provably-dead defensive guards (?? fallbacks the types make impossible, Select.onChange values outside the allowed list). No runtime logic changed; the only structural edits are two behavior-preserving refactors (ResourceTemplatePanel hoists the field value to a local so the ignore applies in JSX; ToolsScreen swaps a ternary for an if) and prettier line-wrapping. Also fixed a flaky real-timer debounce assertion in PromptArgumentsForm (asserted an exact sibling-typing interleaving that raced the 300ms debounce; now asserts the load-bearing fact — a non-empty fire-time sibling capture). AGENTS.md updated to describe the uniform 90 gate and the v8-ignore policy. All four client coverage gates, root validate, storybook (370), and smoke pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(web): make connect-crash SSE reader stop on the terminal event `readSseEvents` looped until the server closed the stream (`done`). The server closes the events stream immediately after writing the terminal `transport_error`, but under the heavy parallel scheduler pressure of a full `coverage` run (v8 instrumentation across all workers) that close can lag far enough behind the event to trip the 30s test timeout — a hang, not a wrong result. This made the test intermittently fail under `npm run coverage` (it passed in isolation and under CI's lighter `test:integration`). Stop reading as soon as the terminal `transport_error` event arrives (cancelling the reader) instead of waiting only for `done`. That event is always the last one the session queue emits, so nothing is lost, and the test no longer depends on stream-close timing. The `done` break is kept for any stream that closes first. Verified green across two consecutive full `npm run test:coverage` runs (3094 tests) where it previously hung, plus isolation (~0.5s). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-27 15:26:03 -04:00
Bob Dickinson	5de4e911e3	feat(web,core): enterprise-managed authorization (EMA) (#1509) (#1512) ... Adds enterprise-managed authorization (EMA): browser-side IdP login + RFC 8693 token-exchange and JWT-bearer resource-token mint, per-server EMA opt-in in Server Settings, IdP config in Client Settings, and IdP clientSecret keychain storage. Includes the v2/main merge and review follow-ups (400 on malformed client config; normalizeIdpIssuer import consolidation).	2026-06-24 22:15:12 -04:00