| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cliffhall
|
7db596b5e6 |
fix(web): disable the enlarge button along with its field
Copilot review round 2 on #2109. A disabled form (a tool call in flight) left the enlarge button live, and activating it swapped in a text area that mounts disabled — so the focus hand-off targeted an unfocusable element and keyboard focus was dropped to the document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
5340283fb4 |
fix(web): let tool string arguments be typed as multiline (#2042)
A string argument renders as a single-line `<input>`, which swallows Enter — so a value containing newlines could not be typed at all. Each string field now carries an enlarge button (ImEnlarge2) to the left of its clear button. Clicking it swaps the input for an autosizing text area that keeps the field's value, label, and length constraints. It is deliberately one-way: shrinking back would have to either discard newlines the user typed or keep a value the field can no longer show, and both are worse than staying enlarged. Enlarging is per-field and resets with `resetKey`, so it cannot carry across to another tool's same-named field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
80f76e365e |
test(web): raise the theme-flip waitFor budget
Pre-existing flake, not introduced here — the file is untouched by this branch and the test already carries a comment about failing this way once. The default 1000ms was still too tight: it failed twice consecutively under `npm run ci` (~1021ms, 0 calls) while passing every time the file runs alone, which is a budget signature rather than a broken observer. A generous `waitFor` costs nothing on the passing path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fafc8367b8 |
fix(web): move the app origin off 6276, bound the publish route
Copilot review round 2 on #2100, plus a collision the review did not find. The default app-origin port was 6276 — which is already RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback. That port is fixed precisely so an OAuth app can pre-register http://127.0.0.1:6276/oauth/callback, which makes it the one listener here that cannot move when something takes it first. A running --web would hold it and break a later --cli/--tui OAuth flow at its registered redirect URI, and docs/mcp-app-review.md has those two running side by side. This listener has a documented dynamic fallback, so this listener yields: the default is now 6278 (6277 skipped — v1's retired proxy port, which the migration guide tells people to stop forwarding). A test asserts the two defaults stay distinct. Review comments: - POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The 8MiB check ran after c.req.json() had buffered and parsed the whole request, and bounded only `html` — so bulk in `csp`, or in keys the route never reads, was fully materialized before the 413. - `csp` is validated as an HTTP header value and length-bounded. The controller hands it to res.writeHead() verbatim, where Node throws ERR_INVALID_CHAR synchronously — inside the handler for a later, unrelated request, outside this route's error handling. - The app-origin port now propagates everywhere the other two do: the CLI --print-handoff port-forward command (and its test), the Dockerfile's ENV and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md. Without it an app declaring _meta.ui.domain is unreachable through every documented remote workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
ec40cf5531 |
feat(web): honor _meta.ui.domain with a dedicated app origin
An MCP App is rendered by handing its HTML to the sandbox proxy as `srcdoc`, in a frame sandboxed without `allow-same-origin` — the #1565 isolation model. That gives the app document an opaque origin, so every request it makes carries `Origin: null` and no CORS / OAuth-callback / API-key allowlist can admit it. `_meta.ui.domain` is the spec field a server uses to ask its host for a stable, dedicated origin, and it was being dropped on the floor. The field is explicitly host-dependent and the Inspector owns no domain infrastructure, so it treats any non-empty value as a request rather than an address, and answers with a real loopback origin on its own listener (MCP_APP_ORIGIN_PORT, default 6276). One shared origin, path-keyed per document: that delivers what the field is for without minting a port or DNS name per app, and is therefore not a per-app isolation boundary. The inner frame is granted `allow-same-origin` on this path only, which is what makes the origin real. It is not a #1565 regression: the listener is on its own port, so the app stays cross-origin to both the proxy and the Inspector. The proxy refuses the grant if the URL's origin equals its own, and the grant is never reachable from the server-supplied `sandbox` string, which is still stripped. The per-app CSP is delivered as a real response header there rather than only as a <meta>. Every failure path falls back to the default srcdoc render with a console warning naming the field, rather than blanking the app. Also fixes a pre-existing `tsc -b` break on v2/main (useServers.test.tsx passed `metadata: []` where `RequestMetadata` is expected), which has had CI red since #2087 and blocks the pre-push gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
5eba105c67 |
Merge branch 'v2/main' into v2/fix/2055-ui-resource-meta-ui | ||
|
cliffhall
|
53943c2ef8 |
fix(web): let a malformed _meta.ui stop the carrier lookup
uiMeta returned undefined for both "this bag declares no ui" and "its ui
is unusable", so a content item declaring `ui: null` fell through and was
granted whatever a BROADER carrier asked for — wrong precedence, wrong
direction to fail in. It now distinguishes the two: no `ui` key means
keep looking, a declared-but-malformed one decides for "no hints".
Also pin the full three-way precedence with a case carrying all three
carriers at once (no prior case would have caught the first two operands
being swapped), and state why a permission's object value need not be
empty: `{}` is the spec's extension point, so rejecting a populated one
would drop grants from servers written against a newer spec.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
e2a88d49dd |
fix(web): restore the read result envelope as a metadata carrier
`McpUiReadResourceResult` — the return type of a registerAppResource read callback — types `_meta.ui` at the result level, so the envelope is a supported carrier after all and dropping it lost the hints for servers using that shape. Consult all three, most-specific first: content item, result envelope, listing entry; first found wins outright. Also make the App wiring test wait for BOTH factories by their advertiseElicitation flag. Waiting on a count passed as soon as either was built, so stripping the wiring from one call site left the other's deps in the array and the assertion loop still passed. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
45785a0aa6 |
fix(web): screen app-supplied sandbox permissions before granting them
The sandbox proxy's buildAllowAttribute() tests each permission key for
truthiness, so an untrusted `_meta.ui.permissions` bag could stand a
non-boolean in for a grant — `{ camera: "false" }`, a plausible way for a
server to mean "off", reads as on. Nothing screened it, because before
the `_meta.ui` fix no permissions reached the proxy at all.
Add approveSandboxPermissions alongside approveCspSources: keep only the
four keys the proxy knows, and only where the value is the spec's empty-
object marker or `true`; drop anything else (warning on a malformed
value, silently on an explicit `false`) and return undefined when nothing
survives. Both the notification and the hostCapabilities echo now carry
the approved set, so the app sees what was granted.
Also document what the listing lookup can and cannot see under
`paginatedLists`.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
0c989a0c34 |
fix(web): use the resources/list entry as the UI metadata fallback
The read RESULT's own `_meta` is not a documented carrier for the sandbox hints. ext-apps documents the `resources/list` entry's `_meta.ui` as the static default a host reviews at connection time, with a read content item's own `_meta.ui` taking precedence (McpUiAppResourceConfig). Swap the fallback accordingly and thread the listing through a new optional `getListedResourceMeta` dep, wired in App.tsx from the resources list. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
06c7985d5c |
fix(web): read MCP App UI resource metadata from _meta.ui
McpUiResourceMeta describes the value of a `_meta` bag's `ui` key, not `_meta` itself, so casting `content._meta` to it left `csp` and `permissions` undefined for every spec-conforming app. The generated sandbox CSP then fell back to `connect-src 'none'` and the app's declared connectDomains never took effect. Unwrap `_meta.ui` (with a result-level fallback, which the spec allows) through a small type-guarded helper, mirroring the tool side of this file where `getToolUiResourceUri` already reads `_meta.ui.resourceUri`. The test fixture now builds the real wire shape, and cases cover the result-level fallback plus the unnested bag being ignored. Closes #2055 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
616dd9c196 |
fix: exclude undefined from _meta values, and cover the JSON worker
- `JsonValue` admits `undefined`, so `RequestMetadata` accepted values that do not survive serialization: `JSON.stringify` drops an `undefined` object member and rewrites an `undefined` array element to `null`. The type contradicted the one promise this payload makes. Added `StrictJsonValue` / `StrictJsonObject` and used them for the metadata path. Deliberately not a change to the repo-wide `JsonValue`: for tool *arguments* `undefined` legitimately means "not supplied". The narrowing is sound wherever it lands here, since every one of these values comes from `JSON.parse`, which cannot produce `undefined`. The CLI's `parseKeyValuePair` needed the same tightening — caught by `clients/cli`'s typecheck, which the web `tsc -b` does not cover. - Nothing asserted the JSON worker's annotations, and the wrapper's own "Not valid JSON" message comes from our parser rather than from Ace — so a broken worker URL would have left every test passing while the gutter diagnostics silently disappeared. Added a Storybook play function that waits for a real `session.getAnnotations()` error entry, and verified it fails when the worker module URL is pointed at a bad path. - `aria-describedby` referenced only the error, so a supplied `description` was never announced and would have been dropped entirely once the draft went invalid. Both ids are referenced now, with the description surviving the error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
167b9b4dab |
fix: validate every reserved _meta member against the SDK's own schema
`progressToken` was not the only reserved key. The pinned SDK's
`RequestMetaSchema` also constrains `io.modelcontextprotocol/related-task` to
`{ taskId: string }`, and a conforming server rejects the *whole request* over
one bad reserved member rather than ignoring that key. Before #1910 every value
was a string, so only `progressToken` could be wrong; the widening made all of
them reachable.
Replace both hand-rolled checks with the SDK's schemas — `RequestMetaSchema`
for the payload, `ProgressTokenSchema` for the single value. The schema is
loose, so unknown keys pass through untouched, and zod reports the offending
top-level key as `issue.path[0]`, so only that member is dropped and its
siblings survive. A member the SDK constrains in a later release is then
covered without anyone remembering to add it — which is the drift that produced
the `isInteger`/`isSafeInteger` bug one round ago.
`name` was hard-coded, and react-ace uses it as the editor container's DOM id,
so two instances emitted duplicate ids. Derived from the per-instance id, and
deliberately distinct from the id given to the textarea — that one is what the
wrapper's `<label for>` targets, so reusing it would just move the collision.
The docs table, the docs prose and the form hint all claimed "any JSON" without
qualification, presenting a configuration as valid that the client then drops.
All three now name the reserved keys and say what happens to a bad one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
7f10fa971c |
fix: address the Copilot review of the Ace editor
- `isProgressToken` used `Number.isInteger`, but zod 4's `.int()` rejects anything past `MAX_SAFE_INTEGER` — verified against the pinned zod — so an unsafe integer passed the guard and was left for the server to reject. Now `isSafeInteger`, with the boundary covered in both directions. - `echoed` held only what this component last emitted, so it went stale the moment an external value was applied: A → B → A stranded the editor on B, because the second A matched the entry left from before B arrived and was dismissed as our own echo. It now advances to whatever is on screen. - `Input.Wrapper` associates a *Mantine* input through context, and Ace is not one — it renders its own DOM. The wrapper's label and error node therefore pointed at nothing: the editor was never marked `aria-invalid`, never described by the error text, and its visible label did not focus it. Wired explicitly through the wrapper's id in an effect. - Removed two leftover `[dbg]` logs an earlier regex cleanup missed. - Documented the editor integration and the shared `cssVariablesResolver` in the web README, per the rule about new dependencies and patterns. Both footguns are recorded: the `?url` worker import, and the gutter override keying off Ace's cssClass rather than the theme module name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
63f81151f5 |
test(web): stop the AppRenderer theme-flip test racing its MutationObserver
Unrelated to this PR's subject, and pre-existing — it arrived with #1854. The test set `data-mantine-color-scheme` and asserted the bridge push after a single `await Promise.resolve()`, but how many ticks happy-dom takes to deliver a MutationObserver callback is load-dependent. Observed failing once in a full `npm run ci`, with 0 calls; passes standalone and on re-runs. Adding a dependency the size of `ace-builds` shifts worker timing enough to expose it, so fixing it here rather than leaving a known-intermittent failure in the gate for whoever hits it next. `waitFor` retries the same assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
ed799a0c0b |
feat(web): use the Ace editor for the request-metadata JSON
A textarea is a poor place to hand-write a nested `_meta` payload. Ace closes a
brace when you open one, folds nested objects, and — through its JSON worker —
marks the offending line in the gutter instead of reporting one message for the
whole document.
Three things the swap turned up:
- Ace fires *two* change events for a replace (a remove, then an insert), so
select-all-and-retype briefly leaves an empty document. Acting on the first
emitted `{}` and wiped the user's saved metadata the instant they selected
it. `handleChange` now coalesces the pair in a microtask, and the component
tracks what it last emitted so the parent echoing that value back is not
mistaken for an external reset.
- Ace's own `github` themes fail WCAG AA on the gutter — `#AAA` on `#e8e8e8` is
1.89:1, and the dark theme is 4.13:1. Folding needs the gutter, so the colors
are overridden with palette tokens (5.82:1 and 4.88:1) rather than hiding it.
- `textInputAriaLabel` alone never reaches the DOM: Ace composes the label as
"<label>, Cursor at row N" and only recomputes it when the cursor moves, so
it is forced once on load. Until then the control's only accessible name is a
position readout.
Testing is split because Ace's input path does not work under happy-dom at all
— `userEvent.type` reaches the textarea and produces no edit, so a test written
that way asserts nothing. Unit tests drive the editor instance through a shared
`src/test/aceEditor.ts` helper; the real keyboard behaviour (`{` then Enter
giving `{\n \n}`) is asserted by Storybook play functions in Chromium.
`react-ace`/`ace-builds` are declared in `clients/web` rather than the root, per
the rule for a dependency that renders React components.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
3a64ce7f4e |
refactor: move the JSON-object draft parser into utils
`react-refresh/only-export-components` rejects a non-component export from a component module, so `parseObjectDraft` could not live beside `JsonObjectInput`. It is a pure transform, which puts it in `utils` under the `lib`/`utils` rule anyway. Also render the metadata hint's inline literals through `<Code>` rather than markdown backticks, which a Mantine `Text` shows verbatim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
b33532c024 |
feat: allow complex JSON values in request _meta
Closes #1910 `_meta` was typed `Record<string, string>` end to end in the Inspector — on disk as a `{ key, value }[]` pair array, in the Server Settings form as key/value rows, and in every request verb's per-call metadata parameter. Nothing in the MCP spec restricts a `_meta` value to a string, and the SDK models the field as a `z.looseObject`, so the restriction was ours alone. Widen the whole path to `RequestMetadata` (`Record<string, JsonValue>`): - Per-server default metadata is a JSON **object** on disk and in memory. The pre-#1910 pair array is still read (`normalizeStoredMetadata`) so an existing `mcp.json` keeps working; it is never written back, so a save migrates the entry. - The Server Settings → Request Metadata section is a JSON editor (`JsonObjectInput`) instead of key/value rows — rows cannot express an object value. It keeps the typed text as the display source and emits only parsed JSON, the same draft split `SchemaJsonField` uses, so the box stays typeable; invalid text is flagged inline and not applied. - The CLI stops flattening `--metadata` / `--tool-metadata` through `metaValueToString`. Those values were already JSON-parsed, so `trace={"id":"abc"}` used to reach the wire as the *string* `{"id":"abc"}` and now reaches it as an object. - `servers/show` redaction replaces a sensitive key's whole value rather than a string, so a structured value cannot leak through its members. - `progressTokenOf` narrows the reserved `progressToken` key back to `string | number` before it reaches the SDK; any other JSON still travels inside `_meta` but wires up no progress callback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
a859093bd3 |
Merge remote-tracking branch 'origin/v2/main' into v2/feat/1950-file-secret-store
Signed-off-by: cliffhall <cliff@futurescale.com> # Conflicts: # README.md # clients/web/src/App.tsx |
||
|
cliffhall
|
beaaad4982 |
fix(apps): make a covered elicitation dialog inert
Only the top modal owned the focus trap, but every lower `Modal.Content` remained an exposed `aria-modal` dialog with reachable controls and a live iframe — so browse navigation could land in a dialog that is visually covered and that itself claims the rest of the page is inert. Non-top contents are now `inert`: out of the accessibility tree and out of focus order, without unmounting, so each app keeps its bridge and its handshake and is live the moment it becomes top. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
0f3ee265fc |
fix(apps): bind app elicitations to the connection that made them
Two of the three findings; the third is answered in the PR thread. **A replaced client could still enqueue.** The one-shot sweep only rejected what was already queued, but the outgoing `InspectorClient` disconnects asynchronously and can enqueue during its own teardown — and that late entry would be rendered by a factory bound to the REPLACEMENT client, reading its resource and answering through a different server. Requests now belong to a session: each constructed client gets one, closing it rejects that session's entries AND refuses anything it queues afterwards, and another session's entries are untouched. **The advertisement no longer has to guess.** Whether a sandbox exists is only known once `/api/config` resolves, and the answer is baked into the client at construction — guessing "available" over-claims, guessing "unavailable" strands the whole session on the native form despite having a sandbox. The connect path now awaits that fetch (already in flight since mount, so no human ever waits; it only orders a deep-link auto-connect racing the same page load), after which an absent `sandboxUrl` means confirmed-absent. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
8f14d371ad |
fix(auth): land the round-20 UI wiring that never applied, and nonce the staging name
Three of round 21's four findings say the same thing: **the UI half of round 20 was never applied, and I reported it as done.** The edit script aborted partway on a regex error, after writing one file and before writing the other two. So `ServerConfigModal` gained the footer prop but `App.tsx` never passed it — the footer was `undefined` in production and rendered nothing, visible only to the unit test that passes a descriptor directly — the extra refresh call sites never landed, and the footer's doc comment still claimed two secret-entry dialogs while the code had three. `npm run ci` passed throughout, because every test I added exercises the piece that *did* land. This is the same failure as round 17's uncommitted files with a different cause: I verified intent (green gate, clean typecheck) instead of the artifact. Each edit now asserts it changed the file, and the result is grepped afterwards — the check costs a second and is the only one that describes what is actually on disk. Landed now, and verified in the files rather than inferred: - `App.tsx` passes `secretStorage` to `ServerConfigModal`, so the disclosure the previous commit claimed to add actually renders. - Every secret-bearing write refreshes the descriptor — config submit and both import flows, not just the two settings saves. Any of them can perform the pending plaintext-to-encrypted upgrade, after which the footer was reporting "Plaintext file" until reload. - The footer's doc names all three dialogs. The wrong count there was not a description of the bug, it *was* the bug: the omitted dialog was the one taking secrets with no disclosure. Also, the fourth finding: **the staging name was reusable across restarts.** `recoverOrphanedSnapshots` deliberately leaves an orphan when a live `secrets.json` also exists, and a pid-only name repeats (pid 1 on every container start) — so the next claim would `rename` straight over that orphan and permanently discard secrets it may uniquely hold. A per-attempt nonce makes a staging path unrepeatable. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
d90c51a04c |
fix(apps): close the three gaps the review flagged as previously missed
**The task-augmented inbound branch bypassed the routing.** A server→client `elicitation/create` carrying `params.task` answers immediately with a `CreateTaskResult` and settles the TASK when the user answers, so it cannot ride `enqueuePendingElicitation` — and it was therefore never offered to an app, even with a valid `_meta.ui.resourceUri`. It now makes the same attempt, completing the task with the app's result and falling back to the native queue otherwise; an abort fails the task rather than reopening it natively. The two task-settling closures are extracted so both answer routes settle it identically. **The web client advertised the capability without a sandbox.** `useInitialConfig` leaves `sandboxUrl` undefined while `/api/config` is in flight and when the sandbox controller could not start, but the renderer was supplied unconditionally — so the client claimed it could host an app in exactly the window where it could not. The option is now gated on a resolved sandbox URL: such a connection behaves like the CLI/TUI, native queue and no claim made. **Concurrent modals each owned the keyboard.** Every open `Modal.Root` installed its own focus trap, Escape handler and overlay, so the traps fought and one Escape could dismiss more than one pending request. Only the topmost owns them now; the rest stay mounted, keeping their bridges and handshakes, but inert to the keyboard. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
98d1fe752d |
fix(apps): gate the capability observer on the bridge's own initialize schema
The hand-rolled accept check was shallower than the bridge's, so frames it
rejects could still set `elicitation` here — `appInfo: {}` passed, though
`McpUiInitializeRequestSchema` requires an implementation name and version.
Any approximation drifts; the schema itself does not, and ext-apps exports it.
Capabilities are still read from the ORIGINAL frame rather than the parse
output, since that schema is precisely what strips `elicitation` — validating
with it and reading through it would defeat the module. The one check the
schema cannot make is that the frame is a request at all (a notification
carries the same method and params), so the id check stays.
Also stops treating a truthy non-object as an advertisement: the draft declares
`elicitation` as an object, so `elicitation: true` is a value it does not
define and the bridge's schema will reject it the moment it carries the key.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
b86db5c7a2 |
fix(apps): track the latest accepted app ui/initialize, not the first
The previous round froze the recorded capabilities at the first handshake, on the belief that the bridge ignores re-initialization. It does not: ext-apps 1.7.5's `_oninitialize` warns about the double-mount and then assigns `_appCapabilities` and `_appInfo` from the new frame — "the latest appInfo/ appCapabilities replace the previous values", in its own words. Freezing left this gate reporting capabilities the bridge no longer held, in both directions: a handshake advertising `elicitation` followed by one without it still read as advertised. Every frame that passes the accept gate now replaces the recorded value, and `appCapabilities` joins `protocolVersion` and `appInfo` in that gate since the bridge's own schema requires all three. A frame the bridge would reject still records nothing AND leaves the previous value alone — it is a route to changing the gate in neither direction. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
cc0770babd |
fix(apps): fail closed on a malformed app ui/initialize
The capability observer recorded `params.appCapabilities` from ANY frame whose method was `ui/initialize`, before the bridge validated it. A view could therefore send a second, malformed initialize — one the bridge rejects, keeping the capabilities it had already accepted — and flip `elicitation` on in this gate, after which the host would forward an elicitation the bridge never negotiated. Now only a well-formed initialize REQUEST is recorded (a JSON-RPC id, plus the handshake's required `protocolVersion` and `appInfo`), and only the first one: the bridge keeps what it accepted and ignores re-initialization, so this gate agrees rather than offering a second, laxer path to the same flag. A malformed first frame records nothing. This is a gate, not a second copy of the bridge's schema — the bridge stays the authority on the rest of the frame. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
7c77c2b15f |
fix(apps): address review of app-rendered elicitations
Eight findings from the Copilot review, all accepted.
Ownership across connections was the substantive one, in three parts. A
mid-session transport close reaches teardown only through
`clearAndAnnouncePendingPeerRequests`, whose emptiness check looked at the
native queues alone — so a lone app elicitation survived a dropped connection
with its modal still open; the check now counts the active set too. The web
bridge factory resolves its client at call time, so an entry queued by a
replaced `InspectorClient` could rebuild against the *next* one and answer
through a different server; the host now drops every entry synchronously on
the swap (`AppElicitationController.failAll`) rather than racing core's
awaited teardown. And the request id was only monotonic within one client, so
each replacement client's first request was `app-elicitation-1` — settling
that id could resolve the wrong server's request. It now carries a
per-instance prefix.
Result validation now parses the whole value with `ElicitResultSchema` before
the requested-schema check, instead of inspecting `action` by hand: a
`{ action: "decline", content: { x: {} } }` used to pass, though the standard
result permits only primitives and string arrays in `content`.
The dialog's accessible name now includes the request id and the prompt — two
concurrent elicitations can be for the same app URI, and the previous name
made those indistinguishable, which is what the comment beside it claimed to
prevent.
`observeAppCapabilities` takes a minimal structural `onmessage` contract
(generic over the message type, since a handler typed for a narrower message
is not assignable to one typed for `unknown`), removing the double cast at the
call site.
`smoke:web:elicit` now fails on an uncaught page error after a successful
drive, and captures the async half of that class off the console channel, as
its two sibling smokes do — it could otherwise print OK over a broken bundle.
Adds the tests each fix needs, including the two the review asked for
directly: that `advertiseElicitation` reaches the `AppBridge` constructor, and
that it is absent by default.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
bc4b774d80 |
fix(auth): address Copilot review round 12
Three inline findings and one of the two suppressed. The other suppressed one — that the hand-off's delete is still a TOCTOU — is the cross-process guarantee, decided against for this PR and tracked as #2082. **My convergence tests did not converge anything.** The reviewer caught that the "two stores racing" tests cannot race: `serialize` keys its queue on the resolved path *process-wide*, which I made it do in the previous commit precisely so two instances would take turns. So the tests passed by serialization while their names and comments claimed they were exercising the optimistic retry, and the retry's successful path had no coverage at all. A fix I was pleased with silently disabled the tests written to prove it worked. Split in two now, both honest about what they drive: - `in-process mutations are serialized per path` keeps the multi-instance cases, which pin a real property — it is what makes the in-process case correct *without* relying on the retry. - `cross-process convergence` injects one external write between the store's write and its verifying read, via raw `fs` so it bypasses the queue the way another process would. The surviving entry can only be explained by the re-apply, so these pin the mechanism rather than the outcome. Covers a set, a delete, and the encrypted path where every attempt re-derives. **A well-formed envelope is not an openable one.** `readOnDiskEncryption` classified by structure alone, so a wrong or unset `MCP_INSPECTOR_SECRET_KEY` produced the quiet, healthy "Encrypted file" footer for a file whose every `get` returns `null` and every `set` is refused — the exact state the unreadable footer promises to warn about. It now authenticates: decrypt and discard, costing one scrypt derivation (~23ms) per descriptor build, once per `/api/config`. A test asserted the old behavior and has been rewritten; the comment on it explains why "describable" has to mean *accurately* describable. **The copy button's `aria-label` was eating the warning.** `aria-label` replaces descendant text, so a screen-reader user heard "Copy secrets file path …" and never heard "Plaintext file" — the one sentence that should change their mind about typing a secret. The status now leads the accessible name and the path follows, which also keeps the name a superset of the visible text (WCAG 2.5.3). **The spec still described the pre-#1950 invariant** in its migration bullets: every plaintext secret lifted into the keychain and removed from `mcp.json`, with only `KeychainUnavailableError` named. Both bullets now describe the selected store, the durability gate that withholds the disk delete for a session-scoped store, the strict read behind the store-wins lookup, and `SecretStoreUnavailableError` as the family the handler catches. Full `npm run ci` green, including the per-file ≥90 gate — the new validator and retry branches needed six more cases to clear it. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
08fc20a22d |
feat(apps): negotiated app-rendered form elicitations
Let a server hand a form `elicitation/create` to an MCP App and return the app's ordinary `ElicitResult`, with the native elicitation form as the fallback on every failure. No second extension, no custom method, and no custom result shape: the only new wire surface is a nested `elicitation` flag on the existing `io.modelcontextprotocol/ui` extension on each side, plus `_meta.ui.resourceUri` on the request. App rendering is selected only when all four gates hold — client `elicitation.form`, client MCP Apps MIME type, the nested `elicitation` setting on BOTH peers, and a valid absolute `ui://` URI on the request. Only the web client advertises the nested client-side setting, and only because it has a sandbox renderer: supplying `InspectorClientOptions.appElicitation` is what opts a client in, so CLI and TUI keep advertising the MIME type without ever claiming they can resolve an elicitation through an app. Routing lives in the one funnel both entry points already use (`enqueuePendingElicitation`), so the inbound handler and the MRTR driver cannot diverge. Ownership is request-scoped — a per-request id keys the renderer, iframe and bridge — so two concurrent elicitations can never resolve through each other's bridges. An explicit `decline`/`cancel` is a completed elicitation and goes back to the server; everything else (absent or malformed metadata, resource/sandbox/bridge failure, an app with no elicitation capability, a timeout, an invalid result or one that fails the requested schema) falls back. The Inspector speaks the ext-apps#733 / SEP-3118 wire protocol but cannot yet consume its helpers — the released `@modelcontextprotocol/ext-apps` (1.7.5) predates that PR. `core/mcp/appElicitation.ts` and `AppRenderer/requestAppElicitation.ts` mirror it exactly and are marked for deletion once a release containing it ships. One consequence needed its own seam: ext-apps 1.7.5 parses the view's `ui/initialize` through a schema that strips `elicitation`, so an app that correctly advertises it would look like one that did not, silently turning every negotiated elicitation into a fallback. `AppRenderer/appCapabilities.ts` records the raw frame instead, and prefers the bridge's own value once it carries the key. `AppRenderer` now takes an `AppRenderSource` union rather than a `Tool`, so the same renderer and bridge factory serve an App tool and an elicitation without either faking the other's shape. Adds the public fixture (`app_choose_option` + a self-contained `ui://demo/choose-option.html` app covering accept/decline/cancel), its two showcase configs, and `smoke:web:elicit`, which drives the negotiated path end to end in headless Chromium and then the same tool against a server that never advertised the capability, asserting the native form takes it. Closes #1854 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
219765efff |
fix(auth): address Copilot review round 10, and document the concurrency design
The headline is a correction to my own claim rather than a new defect. **The optimistic verify is weaker than the comment said it was.** It described the residual as a crash between the write and the verifying read. The reviewer pointed out that no crash is needed: order two writers as write-A, verify-A, write-B, verify-B and both report success while A's entry is gone — A's verify simply ran before there was anything to see, and B did nothing wrong. A crash is one instance of that shape, not the whole of it. The trace in the old comment quietly assumed A verifies *after* B writes, which nothing guarantees. Corrected in the doc comment, the README and the spec, and the honest summary is now stated rather than implied: this converges in every interleaving where the clobber lands before the verify, it is not mutual exclusion, and a deployment that needs the guarantee wants an OS-backed lock from a dedicated library — not another hand-rolled election. It remains better than what it replaced, which lost updates across a wider set of interleavings and could not be closed at all with what Node exposes. The other findings: - **An envelope naming the cipher still is not an openable one.** Round 9 added a `kdf`/`data` presence check; three dot-separated strings satisfied it while an empty tag, a 4-byte IV or `N: 3` still made every decrypt throw — so the quiet "Encrypted file" footer went to a file whose next save was guaranteed to fail. Now validates part sizes, a non-empty ciphertext, and the scrypt cost parameters (node rejects a non-power-of-two `N` synchronously, out of the derivation rather than the decrypt). A positive control asserts a file we actually wrote still reads as encrypted. - **`encryptionUnknown` + `looseMode` claimed an exposure it had not established.** With both set, `plaintext` is absent, so a `!== false` test landed on "anyone who can read it can read the secrets in it" — about a file that may well hold ciphertext we simply could not classify. Three-way now, in `secretStorageCaveat` and mirrored in the footer's tooltip, with a test on each. - **The README described a lock that no longer exists**, which is the worst kind of stale doc: it stated a durability guarantee the code had stopped providing. Rewritten to describe the actual design, including the residual. - **The private-method access in the concurrency test** now carries the justification the repo's TypeScript rules require: the asserted shape is `writeMap`'s real signature, so a rename breaks the line instead of silently detaching the stub, and a production injection seam would let the real write path drift from the tested one. Also updates `specification/v2_servers_file.md`, which still described #1356's world: one keychain-only store, and unavailability as a hard error. It now covers store selection and the probe, the file format and lazy encryption upgrade, the concurrency design and its residual, the `getStrict`/`getMany` seams and the two traps `getStrict` hit, the durability gate on migration, the keychain hand-off and its conditional delete, and how the active store is surfaced. Full `npm run ci` green. (One unrelated `AppRenderer` theme-flip test failed once under full-suite parallelism and passes in isolation with and without these changes — a flake, not a regression.) Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
6efc65d2af |
fix(auth): address Copilot review round 7 (five of six findings)
Five taken; one rejected with evidence, below. - **The keychain hand-off ran outside the file lock.** Read → copy → delete has to be one transaction: another Inspector completing a `set` between the read and the delete had its brand-new secret removed without ever being copied — a write that reported success and then vanished, which is the loss the lock exists to prevent, reached through the migration meant to preserve things. The whole sequence now runs under the same cross-process lock every mutation takes. - **An unmigratable entry was skipped and the file deleted anyway.** A key that is not `serverId:field` cannot be addressed through the store API, so it cannot be copied — which makes the hand-off incomplete by definition, and the complete-success rule then forbids removing the source. It was discarding that value permanently in order to tidy up a migration that had failed. The copyable entries still go across; the file stays, and says why. - **`readOnDiskEncryption` accepted any format version.** A version-2 file was reported as a usable plaintext or encrypted store even though `readMap` refuses it and every save fails. Checked before the mode now, matching `readMap`. - **The loose-mode warning at selection was encryption-blind**, claiming a reader could read the secrets of a file that is ciphertext, and it duplicated the caveat printed moments later by `warnAboutSecretStorage`. Selection now only repairs; the descriptor path does the reporting, because it is the one that knows whether the file is encrypted. - **The footer's tooltip offered to copy the path without showing it.** The band stopped printing it and the accessible name carries it for screen-reader users, which left a sighted user unable to see where a custom destination points without pasting it somewhere else. **Not taken: "`writeStoreFile` is not atomic."** It is. The finding reads `store-io.ts` line 84 as a direct write, but `writeFile` there is imported from the `atomically` package, not `node:fs`. Verified rather than argued: writing over an existing file changes its inode (so a rename happened) and leaves mode `0600`. I had started implementing the suggested temp+rename before checking the import, which would have replaced a tested library with a hand-rolled duplicate. That check corrected something of mine, though: `tightenSecretFilePermissions` justified itself with "a rename over a 0644 file keeps the loose mode forever", which is false — the temp carries `mode: 0o600` and the rename replaces the inode, so every *write* re-establishes it. The function is still needed for the case it actually addresses: a file that exists and is never written. Comment corrected to say so. Full `npm run ci` green — 5,753 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
fdeffa1f00 |
fix(auth): address Copilot review round 6 on the secret-store PR
Four findings, all suppressed, all real. Three are the same mistake in different places: a state that was never established being reported as a confident answer. - **`readOnDiskEncryption` conflated "no file" with "unreadable file".** Both answered `null`, and the descriptor falls back to the configured write policy on `null` — so a passphrase-configured install reported "File (encrypted)" about a file whose envelope had never parsed, and which `set` was in fact about to refuse rather than overwrite. It now returns a four-state result, and only `absent` falls back to the policy (honestly: the first write really will create it that way). `unreadable` produces `encryptionUnknown` on the descriptor, `plaintext` is omitted entirely so nothing can read a default out of it, and the label, caveat, tone and footer all carry "unreadable" rather than guessing a mode. - **The loose-mode warning overstated exposure for an encrypted file.** "Anyone who can read it can read the secrets in it" is false when the file is ciphertext — a reader gets the bytes and still needs the passphrase. Both the caveat and the footer tooltip now branch on encryption: the passphrase becomes the only protection, which is the real consequence. Overstating it is not harmless, because a warning that cries wolf is discounted on the occasion it is literally true. - **The keychain hand-off could invert its own keychain-wins rule.** `SecretStore.get` answers `null` both for a missing entry and for a keychain it could not read, and the migration *writes* on `null` — so a transient read failure would overwrite a newer keychain value with the older file copy. Added an optional `getStrict` (throws instead of swallowing) implemented by `KeyringSecretStore`, with `secretStoreGetStrict` falling back to `get` for stores whose reads cannot fail. A throw now aborts the hand-off with the file left in place, so the next run retries. Writing the tests turned up one more, in code the review had passed: `parseOwner` recorded a garbled pid as `-1`, and `process.kill(-1, 0)` is a *wildcard* asking about every process this user can signal — it answers "alive" almost always, which would have made a lock with a corrupt stamp permanently unstealable. Only a positive pid is usable now; anything else is treated as no pid at all and the mtime decides. Tests: the four envelope states kept distinct; the descriptor refusing to guess, with the genuine no-file case as the control; strict-read abort without a write and with the file preserved, plus a store whose `get` and `getStrict` disagree so the test proves which one the migration consults; encryption-aware wording asserted in both directions in the caveat and the band; three unusable owner stamps and a wildcard pid; and the descriptor shapes that must now be rejected — neither half of the encryption question answered, or both. Full `npm run ci` green — 5,750 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
cliffhall
|
1aa1f5a0ca |
docs(web): attach the footer doc blocks to the functions they describe
No behavior change.
Adding `filePermissionsSentence` in
|
||
|
cliffhall
|
decc673a5d |
fix(auth): address Copilot review round 5, and land the impl its tests need
Repairs origin as well as answering the review.
|
||
|
cliffhall
|
71cb7f3eae |
feat(web): state the file store as a sentence, and copy its path on click
The footer's file states now read "Secrets: Plaintext file. Owner-only
permissions." and "Secrets: Encrypted file. Owner-only permissions.",
and the band itself became the copy affordance for the path.
Three things worth reviewing:
- The path is no longer printed on the band. It goes to the clipboard
instead, which means the only place it stays *readable* is the
button's accessible name — so that carries it in full
("Copy secrets file path: /home/node/.mcp-inspector/secrets.json")
rather than a generic "Copy". A clipboard is no use to a screen-reader
user who cannot inspect it.
- "Owner-only permissions." is a claim of fact, so it is made only when
the mode was verified as 0600. `looseMode` is set exactly when it is
something else and could not be tightened, and the message says
"Mode 0644 — not owner-only." there. A footer that asserted the
opposite of the truth about a secrets file is the one failure this
band exists to prevent, so it has its own test.
- The footer phrases its own copy rather than reusing
`secretStorageLabel` for the file case: that label is parenthetical
("File (unencrypted)") because the startup banner appends
" at <path>" to it. The tone and the underlying facts still come from
the shared helpers, so the two surfaces cannot disagree about
anything that matters. Memory's line is still derived from the shared
caveat.
The click target is an `UnstyledButton` filling the band edge to edge —
a real control, reachable by keyboard and announced as a button, rather
than a click handler on a div. Padding moved from the band to the inner
row so the button has no dead margins.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
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> |
||
|
cliffhall
|
6460d1804a |
style(web): bold the "Secrets:" prefix in the settings-modal footers
Weights the label so the band reads as a labelled fact rather than one run of prose — the eye lands on what the line is about before reading which store it names. Rendered as a `span` inside the existing `Text` rather than as a sibling in the `Group`: `Text` renders a `<p>`, so a nested one would be a `<p>` inside a `<p>` (invalid, and the browser closes the outer paragraph early), and a sibling would take the Group's `gap` and sit too far from the value it labels. `inherit` keeps it at the label's own size. Splitting the text is why the assertions move off `getByText`: it matches a string against an element's *direct* text nodes, so "Secrets: OS keychain" now reads as two fragments even though it renders as one line. The unit tests, both modal tests, and the four stories assert `toHaveTextContent` on the band instead, which reads the full subtree. Verified in the running dev server: prefix 700, store name 500, both at the same 12px. Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
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> |
||
|
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>
|
||
|
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> |
||
|
cliffhall
|
519ecf49ee |
chore(lint): enable no-floating-promises in all five ESLint scopes
A floated promise is invisible at review time — the call reads like an awaited one minus four characters — and the unhandled rejection it produces surfaces in a different test, in a different file, as a stack pointing at SDK internals. In #1947 two un-held `callTool` calls made `npm run ci` unpassable and took a full investigation to attribute; the fix was two lines. `@typescript-eslint/no-floating-promises` catches the class outright, so enable it at `error` everywhere. The rule is type-aware and `tseslint.configs.recommended` is not, so each scope's config gains a parser project. The four clients name their own tsconfig projects (both of them — `src` and `__tests__`/`server` live in different ones). The root scope has no tsconfig at all, so add `tsconfig.lint.json`: a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and gating nothing. It sets `moduleResolution: bundler` deliberately — under NodeNext `core/`'s extensionless imports fail to resolve, every import degrades to `any`, and the rule silently stops seeing promises at all. Measured surface, and the fixes: - core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono invokes subscribers with a bare `subscriber()`, so a promise returned from the listener is floated one layer up; `closeAbortedStream` owns the rejection where it can actually be owned. - tui (5) — `handleDisconnect` had no catch of its own and its only caller is a key handler that cannot await, so a failing disconnect was a live unhandled rejection; it now surfaces the message the way `handleConnect` does. The rest are `void` at callers that genuinely cannot await, over callees that already own their failures. - web (92) — 83 are Storybook `expect(...)`, which is instrumented and returns a promise, now awaited (`scrollAreaStoryAssertions` is async for the same reason). Two tests were asserting against a provider call they never awaited. `open(url)` in both web servers now warns instead of escaping a synchronous callback. - cli, launcher — already clean. Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s to ~19s, inside `validate`. Narrowing the loaded projects is the lever if that needs reducing later; dropping the rule is not. Closes #1959 Signed-off-by: cliffhall <cliff@futurescale.com> |
||
|
Cliff Hall
|
0f31de83af |
Merge branch 'v2/main' into v2/fix/1909-salvage-malformed-list-items | ||
|
cliffhall
|
3a22d4eedc |
fix: disambiguate the two token rows' controls for screen readers (#2019)
Copilot review round 1. With an ID Token row alongside the Access Token
row, both can render a "Decode JWT" toggle and a "Copy" control, and the
four buttons shared two accessible names — button-by-button screen-reader
navigation could not tell which token a control acted on. That collision
is introduced by this PR: before it there was only ever one token row.
- `CopyButton` takes an optional `label` naming what is being copied,
used for the accessible name only ("Copy ID Token"). The visible
tooltip stays "Copy", and the name keeps "Copy"/"Copied" as its first
word so it still contains the visible text (WCAG 2.5.3 Label in Name).
- `OAuthTokenField` qualifies its decode toggle the same way ("Decode JWT
for Access Token") and passes its row label down to `CopyButton`.
The visible UI is unchanged — this is accessible-name only.
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
fe5b622fd1 |
fix: salvage a list result's valid entries when one is malformed (#1909)
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
|
||
|
cliffhall
|
f201032137 |
fix: surface list-fetch failures instead of swallowing them
A list load that failed — a transport error, or a result the SDK's era codec rejects as invalid — reported nothing. The connect-time refresh is fired with no caller to await it (`void this.refresh()`), so its rejection became an unhandled rejection: the header said "Connected", the panel rendered an empty list indistinguishable from a server that has none, and the failed exchange showed in the Protocol panel as a clean success. Two surfaces now tell the truth: - ManagedListState records the failure as observable state (`getError` + an `errorChange` event) and still re-throws, so App's auth-recovery wrapper keeps working. The four `useManaged*` hooks expose it via a shared `useManagedListError`, and the Tools/Prompts/Resources sidebars render a `ListLoadError` alert with the reason and a Retry. - A response the client refused is attributed back to its Protocol entry (`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`), which now renders an Error status and a "Rejected by the Inspector" alert rather than the green resultType badge alone. The SDK gives no request id with a decode failure, so the id is recovered by correlation; see the comment on `markResponseRejected` for why that is exact. Also stops `listAllTools` from dropping a failing excluded-tools walk on the floor — it stays non-fatal but is logged. Closes #1953 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt |
||
|
Cliff Hall
|
a82a70f4f5 |
refactor(web): sharpen the src/lib vs src/utils rule and move misfiled modules (#1784)
* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776) Codify the empirical rule the web client already followed loosely — utils = pure functions that compute; lib = things that instantiate, adapt, or touch the environment — in AGENTS.md and clients/web/README.md, and move the modules that sat on the wrong side of it. Moves (source + co-located test): - lib/sandbox-csp -> utils (pure CSP-string construction; type-only import) - utils/browserTabVisibility -> lib (registers visibilitychange listeners) - utils/oauthResume -> lib (reads/writes sessionStorage) - utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage) Borderline audits (kept in place): pendingReauth (pure type declarations), deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow / oauthNetworkPhase (pure transforms / type-only core imports) stay in utils; downloadFile stays whole in lib (dominant side is DOM side effects). Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its source at src/lib/downloadFile.test.ts, and document the test-placement rule (side-by-side by default; src/test/ only for core/ tests, the integration project, and shared scaffolding). Pure move — no behavior change; no build/coverage config keys off the boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776) Responds to the @claude review on PR #1784: 1. Stale path references (fixed): update the four in-repo pointers left at pre-move paths — specification/v2_auth_mid_session.md (oauthResume, browserTabVisibility → lib/), and the two load-bearing sandbox security comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp). 2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} → utils/ (no external importers; only its own test) and removed the undocumented lib/types/ subdir. Documented that utils/ is the home for pure shared domain types + their constructors — no separate types/ bucket. 3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth and re-export it from lib/oauthResume, so the only cross-directory arrow now points lib → utils. Codified that invariant in AGENTS.md. Wording nit: AGENTS.md + README now state diagnostic logging (console.warn/error) doesn't count as a side effect for the utils rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 2 — stale customHeaders refs, rule wording (#1776) Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed): 1. Stale paths (same class as round-1 item 1): the customHeaders move left three references to the deleted lib/types/customHeaders.ts path in specification/ (v2_ux_interfaces_phase_0_4_inventory.md, v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts. 2. customHeaders anchor: note in AGENTS.md that its shape is staged for ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer state reads as intentional rather than rot; kept as the no-types/-bucket illustration and led the anchor list with load-bearing modules. 3. Type-only-import clause: split the conflated clause — a type-only import (pendingReauth) and re-exporting core's pure functions/constants (oauthUx/oauthFlow) are distinct reasons to stay in utils; what makes a module lib is wrapping core's stateful runtime. Fixed in both AGENTS.md and the web README. Cosmetic: re-padded the v2_auth_mid_session.md table row to column width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 3 — scope the "no types/ bucket" claim (#1776) Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed): 1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist (navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is for ambient .d.ts stubs, not domain types. 2. README example list drifted from AGENTS.md's round-2 load-bearing anchors — realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer customHeaders from the headline list). 3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's destination, not as the v1.5 source location. Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776) Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed): The round-3 scope fix overshot: it asserted src/types/ holds "not domain types at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave opposite guidance about the same directory. - AGENTS.md + web README: softened to "not the place for new domain types" and added a parenthetical that the one lingering domain type (dead navigation.ts) is tracked for removal in #1785; until then a pure domain type belongs in utils/, not src/types/. - v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts). Self-heals on merge of #1785. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776) Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed): 1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will remove it" — a participle with no subject. Reworded to "the dead navigation.ts InspectorTab, which #1785 will remove". 2. Readability (item 2): the src/utils/ bullet had accreted into a single ~1,400-char blob across five review rounds. Promoted the carve-outs to scannable sub-bullets (Domain types / Diagnostic logging / Importing from core) and lifted the src/types/ note out to its own line, since it's a statement about a sibling directory, not about utils. AGENTS.md only; the README's parallel sentence was already clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs+config: address review round 6 — correct the coverage-glob claim (#1776) Responds to the round-6 @claude review on PR #1784: 1. "no build/coverage glob" was factually wrong (should-fix): the web coverage include in clients/web/vite.config.ts DOES enumerate src/lib/** and src/utils/**. The conclusion (a move between them is coverage-neutral) survives, but the reason was wrong. Reworded both AGENTS.md and the web README to say nothing *enforces* the boundary — no path alias, and the include lists both dirs so a move is coverage-neutral — plus the important whitelist caveat: a module outside components/lib/utils/server falls out of the ≥90 gate entirely. 2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx} appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three lines from the config the doc now describes. Nits: gave the README bullet the same round-5 sub-bullet restructure as AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't autolink in rendered repo markdown). npm run ci green (config change re-verified through coverage + smokes + Storybook). 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> |
||
|
Cliff Hall
|
da37692485 |
refactor(web): sweep AGENTS.md TypeScript + Mantine rule violations (#1771)
Codebase-wide sweep bringing the web client into strict compliance with the AGENTS.md TypeScript and Mantine/React conventions: ~170 inline 2+-static-prop elements extracted to named .withProps() constants across ~77 files, the AGENTS.md rule wording tightened to match (static-prop boundary + Accordion/Transition/data-*/Box exemptions), a real ScreenStage Box->Flex layout regression fixed (direction: column) with a Storybook regression guard, and an App.tsx double-cast tightened. Closes #1762. |
||
|
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> |
||
|
Cliff Hall
|
2259b1a3e1 |
Tasks: io.modelcontextprotocol/tasks extension on modern servers (era fork) (#1731)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* Tasks: modern io.modelcontextprotocol/tasks extension (era fork core) Core + test-server implementation of the SEP-2663 tasks extension for modern (2026-07-28) connections, forked from the legacy 2025-11-25 tasks path. SDK v2 removed all tasks support AND era-gates the tasks/* spec methods out of the modern era on both the client (outbound) and server (inbound) sides, and its codec rejects a resultType:"task" result outright. So the Inspector drives the extension itself: task creation rides tools/call (a modern method) with the resultType:"task" frame rewritten at the transport to a CallToolResult carrying the handle in _meta; tasks/get/update/cancel go over a raw-wire request channel that bypasses the era gate (full modern envelope, string ids, consumed by the transport). input_required tasks reuse the existing pending-request modal via fulfilInputRequests, answered with tasks/update. No tasks/list, no tasks/result. Tab + run-as-task gate on the negotiated extension; the managed task store's refresh re-polls known handles on modern (no server list). Test server serves the extension via an Express interceptor (SDK can't) + a shared poll-driven runtime. Full integration coverage of both eras. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: tests + coverage (raw-wire, schemas, era integration) Adds unit coverage for modernTaskSchemas and the raw-wire request channel (transport-null guard, send failure, timeout, error response, teardown), plus end-to-end era integration tests (create/poll/input-required/update/cancel, synchronous passthrough, unknown-id error, modern store re-poll). Fixes the App and constructor-capabilities tests for the always-on tasks extension. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: docs + share test servers per era to de-flake coverage Document the tasks era-fork test configs in README and the new core/test-server files in AGENTS.md. Share one modern + one legacy test server across the era integration block (started lazily, stopped in afterAll) — starting a fresh Express server per test made connection negotiation flaky under the heavy concurrent coverage run. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Restore pr-screenshots deleted inadvertently The logging/subscriptions PR screenshots + README were staged-deleted in the working tree before this branch; restore them from v2/main so this PR doesn't remove other PRs' artifacts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: offer Run-as-task for any tool + PR screenshots On modern (SEP-2663) task creation is server-directed — the client declares the extension once and any tool may return a task — so the Tools screen now offers 'Run as task' for every tool on a modern connection (threaded via a modernTasks prop through InspectorView → ToolsScreen → ToolDetailPanel), rather than gating on the legacy per-tool taskSupport. Verified end-to-end in the web UI (remote proxy): create → poll tasks/get → input_required elicitation → tasks/update → inline result, plus the unchanged legacy tasks/list/result flow. Adds proof screenshots. Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: address two follow-ups (task-input note + ordinary-path auto-poll) 1. The pending-request note now distinguishes a modern task round from an MRTR round: a new 'task-input-required' PendingRequestOrigin threads through fulfilInputRequests → the pending-request modal → MrtrOriginNote, which shows 'submitted via a tasks/update request (SEP-2663), not a retry' instead of the MRTR retry copy. 2. The ordinary (non-run-as-task) modern callTool path now auto-polls an unsolicited task handle: attemptToolCall detects the rewritten handle in the result _meta and drives it to termination (get → input_required/update → inline result), dispatching task events so the Tasks tab + toasts track it and the tool result resolves to the task's final result. The poll loop is shared with the streaming path via extracted helpers (submitModernTaskInput / modernTaskTerminalOutcome / modernTaskPollInterval). Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: address review (negotiated envelope version, input-round cap, clarifying comment) + fresh smoke screenshots Review of #1731: 1. withModernTaskEnvelope now stamps the NEGOTIATED protocol version (getProtocolVersion() ?? MODERN_PROTOCOL_VERSION) so the envelope matches the MCP-Protocol-Version header instead of a hardcoded constant. 2. A never-completing input_required task is now bounded by MRTR_MAX_ROUNDS in the shared submitModernTaskInput (threaded through both poll loops), so a non-conformant server can't re-prompt the user forever. New modern_loop_task test-server tool + integration test cover the cap. 3. ToolDetailPanel: comment clarifying that per-tool taskSupport:"forbidden" is deliberately ignored on modern (task creation is server-directed under SEP-2663). (The abort-during-poll observation is addressed in the PR reply: the cancel path uses tasks/cancel and disconnect rejects the in-flight raw request, so the poll stops; matches legacy.) Fresh web-UI smoke screenshots (modern connected, task completed, input_required with the corrected tasks/update note, input completed). Refs #1631 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Tasks modern era: fix uncancelable input-required loop + sticky cancel Two bug fixes reported during manual testing of the tasks era-fork (#1631): 1. A modern task paused at `input_required` (e.g. `modern_loop_task`, which never advances) could not be cancelled: the poll loop blocked on the pending-request modal, so `cancelRequestorTask` sent `tasks/cancel` but the modal kept re-prompting. Now each modern poll loop registers a per-task AbortController (`registerTaskInputAbort`), `cancelRequestorTask` aborts it to reject the pending elicitation and unblock the poll, and the embedded input requests are tagged with their owning task id (`RELATED_TASK_META_KEY`) so the pending-request UI's Cancel action cancels the TASK rather than sending a "cancel" answer the server would re-prompt on. 2. On the legacy server, cancelling a task briefly showed CANCELLED then flipped to COMPLETED when the in-flight tool call resolved. Cancellation is a deliberate terminal decision, so `ManagedRequestorTasksState` now remembers cancelled task ids and ignores any later non-cancelled status update. Also: move the compact task id onto its own line in the embedded monitoring sidebar so the status badge / Cancel Task control don't wrap; DRY the duplicated task poll-interval logic into a shared `taskPollInterval` helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Refresh tasks PR screenshots + capture both bug-fix scenarios Recapture the five tasks flow screenshots against the current build and add two demonstrating the post-review fixes: modern_loop_task cancelled from the pending-request modal (uncancelable-loop fix) and a legacy task staying CANCELLED after the in-flight call resolves (sticky-cancel fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * Address re-review: sticky cancel on legacy refresh + App cancel-routing test Re-review of the cancelable-loop / sticky-cancel fix raised two non-blocking hardening items: 1. The legacy tasks/list refresh() rebuilt the list from the server, honoring only dismissedTaskIds — so a cooperative-cancel server that re-lists a user-cancelled task as completed could un-stick the cancel on a manual Refresh. The refresh path now pins a cancelledTaskIds task to "cancelled" (keeping it visible-as-cancelled, matching the live-merge handlers) rather than dropping it. Covered by a new state test. 2. Added an App.test.tsx case locking in the user-facing half of the fix: Cancel on a task-input-required elicitation calls cancelRequestorTask (not respond). 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> |
||
|
Cliff Hall
|
320daf2f67 |
Resources: subscriptions/listen model on modern servers (era fork) (#1630) (#1730)
Fork resource subscriptions by protocol era: legacy keeps resources/subscribe; modern (2026-07-28) models subscriptions as a filter over one long-lived subscriptions/listen stream with reconnect-by-re-listen (capped backoff), an optimistic Connecting→Listening header badge, and a fix so the browser proxy no longer blocks listen() ~60s. Closes #1630. |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
7db596b5e6 | ||
|
|
5340283fb4 | ||
|
|
80f76e365e |
||
|
|
fafc8367b8 |
||
|
|
ec40cf5531 |
||
|
|
5eba105c67 |
||
|
|
53943c2ef8 | ||
|
|
e2a88d49dd | ||
|
|
45785a0aa6 | ||
|
|
0c989a0c34 | ||
|
|
06c7985d5c | ||
|
|
616dd9c196 | ||
|
|
167b9b4dab | ||
|
|
7f10fa971c | ||
|
|
63f81151f5 | ||
|
|
ed799a0c0b | ||
|
|
3a64ce7f4e | ||
|
|
b33532c024 | ||
|
|
a859093bd3 | ||
|
|
beaaad4982 | ||
|
|
0f3ee265fc | ||
|
|
8f14d371ad | ||
|
|
d90c51a04c | ||
|
|
98d1fe752d | ||
|
|
b86db5c7a2 | ||
|
|
cc0770babd | ||
|
|
7c77c2b15f | ||
|
|
bc4b774d80 | ||
|
|
08fc20a22d | ||
|
|
219765efff | ||
|
|
6efc65d2af | ||
|
|
fdeffa1f00 | ||
|
|
1aa1f5a0ca | ||
|
|
decc673a5d | ||
|
|
71cb7f3eae | ||
|
|
2d2c787e22 | ||
|
|
6460d1804a | ||
|
|
de7795ca54 | ||
|
|
3452a63101 | ||
|
|
bc408f5869 | ||
|
|
519ecf49ee | ||
|
|
0f31de83af |
||
|
|
3a22d4eedc | ||
|
|
fe5b622fd1 | ||
|
|
f201032137 | ||
|
|
a82a70f4f5 |
||
|
|
da37692485 |
||
|
|
99ef731746 |
||
|
|
2259b1a3e1 |
||
|
|
320daf2f67 |