collectSchemaDefaults assigned by name, so a legal field called `__proto__`
went to the legacy prototype setter instead of the result. A required one
pinned by `const` would then render read-only and be seeded with nothing,
leaving submit permanently disabled. Seeded through defineProperty now, the
way the other schema-record paths already build their maps.
Signed-off-by: cliffhall <cliff@futurescale.com>
applySchemaConstants only corrected top-level pinned fields, but the deep
link's overlay replaces a nested object wholesale rather than merging into it —
so `{ config: { kind: "sms" } }` slipped past a nested read-only
`kind: { const: "email" }` the form was displaying. It now recurses into
object-valued properties, resolving each one's own root union the same way.
Signed-off-by: cliffhall <cliff@futurescale.com>
- selectBranchIndex compares a discriminator structurally: a `const` may be
an object or array, and deep-link arguments arrive as freshly parsed
instances that could never be reference-equal to the schema's own.
- New applySchemaConstants, applied by InspectorView AFTER the deep link's
`appArgs` are spread over the seeded defaults. The form renders a pinned
field read-only, so a link disagreeing with one would otherwise auto-open an
App with a hidden value contradicting the shape on screen.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: schema values are compared through a key-sorted rendering, so two
discriminator constants written as {a,b} and {b,a} are recognized as the same
value rather than as mutually exclusive alternatives.
- selectBranchIndex: after the const and required passes, a name only ONE
alternative declares identifies that alternative; a name several declare is
ambiguous and says nothing.
- additionalProperties counts as restrictive only when its schema states an
assertion — an annotation-only `{ title: ... }` is the equivalent of
`true`, like `{}`.
- Drop the unjustified double casts from the ToolTestModal fixtures; the
schemas are assignable to Partial<Tool> as written.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: a `oneOf` demands that EXACTLY one alternative match, which
flattening cannot preserve, so it is offered only when a discriminator makes
the alternatives mutually exclusive; `anyOf` makes no such claim and is
unaffected. An empty `additionalProperties` schema is the equivalent of
`true` and no longer declines a union.
- selectBranchIndex: falls back to the branch whose own required fields the
values supply, so an undiscriminated union's values open the picker on the
shape they describe rather than on the first branch.
- admitsNull: honors a sibling `nullable: true` beside `const: null`.
- SchemaForm: a value is carried only where the incoming branch leaves the
root's declaration as it found it; anything it declares itself is reset,
since it may type the name differently from where the value was typed.
`hasOwn`/`fromEntries` throughout, so `constructor` is not read as a
supplied value and `__proto__` is not dropped into the prototype setter.
- TUI: the same prototype-safe construction on the way out of the form, and
the chosen branch's `required` list is checked at submit — rendering a
branch's fields optional makes the FORM satisfiable, not the call, so the
missing names are reported rather than a known-invalid call being sent.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: decline a union that adds fields under a restrictive root
`additionalProperties`. The keyword constrains what its SIBLING
`properties` does not name, so the original schema admits none of the
branch fields; flattening moves them beside the keyword, where they read as
allowed and the form would submit what the schema forbids.
- `default` joins the annotation set: it constrains nothing, so two
declarations suggesting different initial values no longer conflict.
- Property records are read with `Object.hasOwn` and built with
`fromEntries`/`defineProperty`. `properties` is a JSON record, so
`constructor` is a legal argument name that `in` reported as a collision
the root never made, and `__proto__` is one that assignment would drop
into the legacy prototype setter instead of keeping as a field.
- SchemaForm: a value survives a branch switch unless BOTH branches declare
the name. A root argument the outgoing branch merely specialized is still a
root argument in a branch that inherits it, so dropping it erased a valid
value.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: union members get the same flattenability test as allOf members,
so a branch whose nested `allOf: [false]` makes it unsatisfiable is no
longer offered as a callable alternative with that constraint erased.
- selectBranchIndex: weigh only the pinned constants the caller SUPPLIED. One
it did not supply is a value this identification exists to seed, so
requiring it meant a deep link naming `kind` alone matched no branch
whenever the branches also pinned, say, a `version`. A supplied constant
that disagrees is still evidence against.
- admitsNull: `const: null` is conjunctive with its siblings, not an
override — `{ type: "string", const: null }` and
`{ const: null, anyOf: [...] }` reject every value, so the claim is made
only where the type and a sibling union leave null on the table.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: an `allOf` member is foldable only when it states nothing beyond
what the merge applies (`type`/`properties`/`required` plus annotations),
so a nested `anyOf`, a `not`, or an `additionalProperties` can no longer
be erased along with the keyword; members are checked against the
ACCUMULATED merge, so two of them contradicting each other is caught even
when neither contradicts the root.
- admitsNull: `const: null` admits null, so a required field pinned to it is
no longer seeded with the one value it accepts and then reported missing.
- collectSchemaDefaults takes the values a caller is about to overlay, and
seeds the branch THEY identify — the App deep link passes its `appArgs`, so
branch 0's defaults can no longer sit invisibly under branch 1's arguments.
- The branch inference moves into core as `selectBranchIndex`, shared by the
seeding and the form's picker so the two cannot disagree.
Signed-off-by: cliffhall <cliff@futurescale.com>
The previous commit's coverage gate failed on `core/json/rootUnion.ts`
branches (89.81%). A property declared with JSON Schema's boolean form has no
keywords to merge, so the two sides agree only by being the same declaration —
now tested both ways, and the dead `baseProperty !== undefined` guard is gone
(`conflicts` is only reached for a name the base declares).
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: a branch restating a constraint the root states differently is
declined (keywords at one level are conjunctive and no intersection is
computed here), while annotations may disagree freely. An `allOf` with a
member it cannot fold in — JSON Schema's boolean form, or a $ref whose
constraints are unknown rather than absent — is left intact instead of
merged-and-stripped, which would turn an unsatisfiable schema into a
fillable form.
- rootUnion: `declaresAnyFields` counts the raw composition members, so
`hasInputFields` reports fields for a union the resolver declines and an
App tool is never auto-invoked with empty arguments.
- SchemaForm: the picker opens on the branch the supplied values identify
(an App deep link overlays its args, so they can name another branch), and
a branch switch no longer carries a value the outgoing branch declared —
the branches may type the same name differently.
- TUI: a base property some branch redeclares is offered in EVERY branch's
section under that branch's inherited or specialized declaration, so a
branch that does not redeclare it can still supply a required root
argument.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: a name both the root and a branch declare is now MERGED rather
than replaced, so a root `minimum` survives a branch `maximum`; a branch
that contradicts the root on a property's `type` describes an unsatisfiable
value and is declined outright. Branch offerability also requires that
`properties` really is an object (a malformed `properties: null` threw)
and that the member's own `type` admits objects.
- rootUnion: `ownFields` becomes `declaredFields` — a branch commonly
specializes a root property, and consumers must render the specialization.
- TUI: the generated select and branch prefixes step aside from any name the
schema itself declares (JSON has no reserved namespace), decode filters them
by exact name, a branch's specialization renders in its own section, and a
`const` is rendered as a one-option select whose typed value is restored
from the schema on submit.
- convertToolParameters: identify the branch from the discriminator constants
the call supplies; when none identifies one, coerce only names every
declaring branch types the same way rather than picking an arbitrary branch.
Signed-off-by: cliffhall <cliff@futurescale.com>
Every string field in a tool or elicitation form carries an enlarge
button in its right section, so tabbing through a form stopped on one
between each pair of fields. Give it tabIndex={-1}, as #1487 did for the
clear button.
That button was deliberately left in the tab order by #2042, on the
grounds that clearing has a keyboard equivalent and enlarging has none.
So give it one: Enter on the focused single-line field enlarges it and
enters the newline. Nothing was listening for that key there — no
consumer renders SchemaForm inside a <form> — and it is the key a user
presses trying to type the newline the input swallows, which is the
failure #2042 exists to fix.
Enter enters the newline rather than only reshaping the field, since
consuming the keystroke leaves the next word running on from the last. A
field already at its maxLength is enlarged without one. Shift+Enter
enlarges too; the Ctrl/Cmd/Alt chords are left free for a consumer to
bind to running the tool. Clicking the button still only enlarges.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: decline a schema carrying BOTH oneOf and anyOf rather than
reading one and silently dropping the other's constraints.
- SchemaForm: handle `const` before every type dispatch and for every
constant type, so a numeric/boolean constant, or a schema carrying both
`const` and `enum`, can no longer offer a forbidden value.
- collectSchemaDefaults / TUI initialValue: `const` outranks `default`,
which is an annotation a schema may set to something its const rejects.
- convertToolParameters: resolve from the first branch's merged schema, so a
branch's specialization of a root-declared property is what types the arg.
- TUI: ink-form keys values by field name across the whole form, so two
branches' `kind` were one field and the later section's initial value
decided what the earlier submitted. Branch fields now render under prefixed
names behind a Variant select, and decodeFormValues translates back on
submit, dropping every branch but the chosen one.
Signed-off-by: cliffhall <cliff@futurescale.com>
The three headers still said the non-Chromium engines were on demand and
that 'nothing runs it for you'. Both became false when Firefox moved into
npm run ci; I updated the README and AGENTS.md for that change and did not
carry it into the files themselves.
Each header now names the three tiers separately rather than collapsing
them into a CI/not-CI binary: GitHub CI is Chromium only, the local
pre-push gate is Chromium and Firefox, and WebKit is on demand.
That distinction is the whole point of the arrangement, so a header that
blurs it is worse than one that is merely out of date.
Addresses Copilot review 5026731741 on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
A tool's `inputSchema` may carry composition keywords at its root — the
2026-07-28 revision makes this explicit — but both form builders enumerate
the root's `properties` and nothing else. A schema keeping its fields on
`allOf`/`oneOf`/`anyOf` branches therefore rendered no controls at all: no
branch picker, no fields, not even the raw-JSON fallback a union-typed
*property* falls back to, so the tool could only be called with empty
arguments.
Adds `core/json/rootUnion.ts`, one flattening shared by all three clients so
they cannot drift on which schemas they can render:
- `allOf` is merged unconditionally (conjunctive — there is no choice to
present), a root `oneOf`/`anyOf` is returned as branches.
- Web `SchemaForm` renders a Variant picker above the fields and prunes the
outgoing branch's values on a switch, since they are no longer visible and
would describe a shape the call is not making.
- TUI `schemaToForm` gives each branch its own section with optional fields —
ink-form is static, and only one alternative applies to a call.
- `convertToolParameters` looks through the branches, so a CLI `--tool-arg`
is typed by the schema that declares it rather than sent as a string.
Required-field gating asks whether *any* branch is satisfied, which is
selection-independent and never blocks arguments the schema accepts. A
`const` is seeded like a `default` and rendered read-only: it admits exactly
one value, so anything else the user could type produces a rejected call.
Adds a `root-union-schemas-http.json` showcase server.
Closes#2123
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
`refreshStoredAuthToken` consumes the same fallback and made a single SDK
discovery call, so preserving the path there — without the walk the other two
consumers got — would have regressed exactly the case the walk exists to keep:
a path-hosted MCP server whose authorization metadata is published at the
domain root.
It injects its own discovery function as a test seam, so the walk is now also
exposed as `discoverAuthorizationServerMetadataFromCandidates(candidates,
discover)`, which takes that function and reports *which* candidate answered —
the CLI needs the winning URL, since it is the base the token request is made
against, not just the metadata.
Also drops the `as unknown as typeof fetch` from the new test in favour of
`vi.fn<typeof fetch>()`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Caught by CI's clients/web format:check, not by my local run — I had
formatted core/ and scripts/ but not the web client's src, which is where
the test lives.
Signed-off-by: cliffhall <cliff@futurescale.com>
Dropping the CI job left cross-engine checking as something you had to
remember to run, which in practice means nobody runs it. This moves it
into npm run ci, so it happens once per push, in front of a human who can
still reason about the change -- rather than once per CI run, on every
branch, to re-derive an answer we already had.
scripts/run-engine-smokes.mjs is what makes that possible cross-platform:
SMOKE_BROWSER has to reach three child processes, and a POSIX VAR=x npm
run prefix does not work under Windows cmd.exe, which npm uses there.
Setting it in the child env from Node does. smoke:web:firefox passes the
engine as an ARGUMENT rather than relying on the ambient variable, so a
stray SMOKE_BROWSER cannot silently redirect the gate to another engine
-- verified: an explicit argument beats it.
It also gives the smoke list one home. It was previously an && chain in
package.json beside a second chain in the workflow, so adding a fourth
smoke meant remembering both. ENGINE_SMOKES is now the list, with a test
pinning its contents and its order -- a smoke going missing is invisible
at runtime, because a shorter list is still a passing run.
Firefox only. WebKit fails the App smokes for unidentified reasons and
must not be in a gate.
This makes npm run ci a strict superset of GitHub CI rather than a mirror.
That was already true of smoke:tui, which self-skips on CI; Firefox is the
second such step. The direction that matters still holds: passing
npm run ci locally means CI's gates will pass.
Signed-off-by: cliffhall <cliff@futurescale.com>
When RFC 9728 protected-resource metadata names no authorization server, the
MCP server URL stands in as one. `getAuthorizationServerUrl` built that with
`new URL("/", serverUrl)`, which discards the path — exactly the part discovery
needs when the server is not hosted at the domain root. The SDK's
`buildDiscoveryUrls` derives the path-scoped well-known locations from that
pathname, so an origin-only value could only ever probe the domain root, which
404s for a server like `https://example.com/mod/minilesson/mcp.php`.
Keep the path (and strip query/fragment, which an RFC 8414 issuer never
carries). A straight swap would regress the case the origin-only value did
serve — a server that merely lives under a path while publishing its metadata
at the root — so the URL becomes the first of two candidates:
`getAuthorizationServerUrlCandidates` yields the path-scoped form then the bare
origin, and `discoverAuthorizationServerMetadataForServer` walks them, treating
a candidate's failure as non-fatal while another remains and rethrowing the
first error only when all of them fail.
`discoverScopes` and the CIMD pre-registration probe both go through the walk.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
parseSSE declared currentEvent/currentData inside the read loop, so an SSE
frame whose data: line arrived in one chunk and whose terminating blank
line arrived in the next was discarded. buffer already carried a partial
LINE across reads; nothing carried a partial FRAME.
Because this is the web client's transport, the dropped frame is a
JSON-RPC response that never settles: the request hangs forever with no
rejection, no console message, and nothing in the Network tab. The
payload does not arrive corrupted, it does not arrive at all, which is
why it presents as a hang rather than an error.
It has not bitten us because Chromium and Firefox happen to deliver whole
frames at the payload sizes this transport currently sees. That is luck,
not a guarantee: chunk boundaries depend on payload size, TCP
segmentation, and any intermediary. The exposure grows with payload size,
so a large resources/read is the most likely first victim.
The fix hoists both out of the loop and adds an end-of-stream flush — a
server that closes without a final blank line has still delivered a
complete frame, and dropping it would lose the last message of every such
stream.
parseSSE is now exported, solely so the test can reach it. That is the
only way to hit this deterministically: driving a real transport cannot
steer where a chunk boundary lands, but a test that supplies the reader
can put it exactly on the terminator.
The test keeps the two PASSING cases (whole frame, split mid-payload)
alongside the three that failed, because they are what make the diagnosis
precise. This is not "SSE parsing is broken" — mid-payload splits are
handled correctly today, and only the frame-terminator boundary loses
data. Verified against the unfixed code: 3 of 5 fail, all 5 pass after.
Signed-off-by: cliffhall <cliff@futurescale.com>
The job was trialled and did not earn its place. It was genuinely cheap —
~2 minutes, running in parallel with the 15-minute build job, so zero
added wall-clock — but across a dozen runs Firefox never once disagreed
with Chromium. It was paying runner time and carrying a real flake
surface (playwright install --with-deps runs apt-get update, which fails
whenever a third-party repo in the runner image breaks; that is what the
one red run was) to re-confirm a result already in hand.
Everything that made cross-engine checking possible stays: SMOKE_BROWSER,
the shared headless-browser helper, the installer, and the tests. So
"SMOKE_BROWSER=firefox npm run smoke:web:engine" is there for the moment
it is worth running — before touching the sandbox, the CSP builder or the
proxy page — without taxing every push to re-derive the same answer.
The workflow is now byte-identical to v2/main again; the publish jobs go
back to needing only build.
Docs say what is true rather than what was intended, and AGENTS.md
records the reasoning so the job is not re-added on the general argument
that cross-engine coverage is good — that argument was already accepted,
and the on-demand path is what serves it. Re-add it on evidence: a real
cross-engine regression a gate would have caught.
Also swept the now-stale "matrix" language out of the smokes and the
resolver, including the empty-SMOKE_BROWSER rationale, which no longer
leans on a matrix key that does not exist.
Signed-off-by: cliffhall <cliff@futurescale.com>
#2132 was closed without the investigation going anywhere, so every
'pending #2132' pointer in the tree was about to send a reader to a
closed issue whose own diagnosis had already been retracted.
The docs now say what is actually known, and nothing more: the two App
smokes fail under Playwright's WebKit for unidentified reasons; it does
not reproduce in real Safari; an isolated repro of the suspected
mechanism did not reproduce it under Playwright's WebKit either; and
chasing it further was judged not worth the effort. A reader hitting it
locally is told to treat it as unexplained rather than as a defect.
The matrix decision is unchanged and never depended on the cause — the
engine fails, so it is not gated.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
An isolated repro of the mechanism I described — a server writing three
SSE frames then going silent with the connection held open, read via
fetch + getReader — delivers every frame in all three engines, WebKit
included. So 'the SSE stream's last message is not delivered' is not a
supported description of the failure.
The keep-alive experiment that appeared to confirm it was one
observation, and the failure is not even stable in shape: two consecutive
webkit runs failed at different stages (ready wait vs connect wait),
which a deterministic buffering bug would not do.
What is unchanged: the App smokes really do fail under Playwright's
WebKit, which is why the engine stays out of the matrix; that reason
never depended on the mechanism. The parseSSE cross-chunk defect also
stands, being read off the code rather than inferred from an experiment.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from Copilot's suppressed block.
1. resolveBrowserName treated a present-but-empty SMOKE_BROWSER as unset
and returned chromium. The test that covered it even documented WHY
that is dangerous — GitHub Actions renders an undefined matrix key as
"" rather than omitting the variable — and then asserted the unsafe
behavior. A misspelled matrix key would run chromium inside a job named
for another engine and report green: the same false-coverage outcome
the unrecognized-value branch exists to prevent, reached by a typo
instead of a bad value.
Only an absent variable now selects the default. Empty throws, naming
the likely cause and the actual remedy ('unset it', which is not the
same as setting it to nothing). The workflow always renders a real
value, so nothing in CI is affected.
2. install-smoke-browser.mjs had no tests. Its I/O half runs on every
smoke and fails loudly; its argument half does not — every caller
passes nothing or the literal 'chromium', so the precedence rule is
asserted by nothing at runtime. It matters: pack:verify passes chromium
BECAUSE it launches chromium, and if the environment won,
SMOKE_BROWSER=webkit npm run pack:verify would install one engine and
launch another, surfacing as a misleading "Executable doesn't exist".
resolveRequestedBrowser is now exported and pure; execution moved
behind main() with the entrypoint guard verify-typecheck-coverage.mjs
uses, so importing the module for tests cannot trigger an install.
Both mutation-verified: restoring either behavior fails the tests that
claim to cover it.
Addresses the suppressed comments on Copilot's fifth review of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from Copilot's suppressed block.
1. The launch-failure message is #2086's acceptance criterion — a missing
browser must name THAT engine and its own install command — and it had
no test. The smokes cannot supply one: install-smoke-browser fetches
the binary first, so a passing smoke says nothing about what a failing
one prints, and that message is the deliverable. loadBrowser now takes
an injectable loadPlaywright purely to open that seam.
The test asserts per engine that the message leads with that engine,
names --with-deps <that engine> and no other, and preserves the
underlying cause. Verified by mutation: hard-coding 'chromium' in the
message fails it. It also pins the two branches apart — an unresolvable
package must say
> @modelcontextprotocol/inspector@2.3.0 postinstall
> node scripts/install-clients.mjs
[install-clients] Installing dependencies for clients/web...
up to date, audited 583 packages in 614ms
198 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[install-clients] Installing dependencies for clients/cli...
up to date, audited 237 packages in 496ms
69 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[install-clients] Installing dependencies for clients/tui...
up to date, audited 314 packages in 518ms
102 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[install-clients] Installing dependencies for clients/launcher...
up to date, audited 160 packages in 449ms
50 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
up to date, audited 281 packages in 3s
108 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities and must NOT say ,
which fetches binaries and cannot fix a missing package.
The unsupported-engine test now also asserts loadPlaywright was never
called, pinning the ORDER rather than only the message: validating
first is what makes the error name the real mistake.
2. prod-web-server.mjs's header still described the prefix this PR removed. Corrected, keeping why
deriving paths from import.meta.url is still right, and adding the
elicitation smoke to the list of callers it had also outgrown.
Addresses the suppressed comments on Copilot's review of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
#2132 was filed asserting that a Safari user opening an MCP App gets a
permanently 'loading' widget. That was never tested. A manual check in
real Safari renders the App normally, so the claim is unsupported and is
removed from all six places it reached.
The failure under Playwright's WebKit is real and still keeps that engine
out of the CI matrix. What is not established is that any shipping
browser behaves this way — and Playwright ships its own WebKit build
whose networking stack is not Safari's, which is exactly the layer the
failure lives in.
headless-browser.mjs already carried the caveat that a green WebKit run
is not a Safari guarantee. The same sentence forbids the inverse
inference, which is the one that was made; it now says so explicitly, and
AGENTS.md carries the rule: reproduce in the real browser before claiming
user impact in one.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 1 fixed the three smoke headers and AGENTS.md but left the same
overstatement in the two files Copilot flagged as previously-missed:
- README said the matrix runs 'the remaining engines', which reads as
Firefox AND WebKit and contradicts the very next paragraph. It now
names the current matrix (Firefox alone) and points at that paragraph.
- The workflow's fail-fast comment cited a WebKit-vs-Firefox scenario the
one-entry matrix cannot produce. It now justifies keeping the flag for
when a second engine returns, matching the AGENTS.md wording.
Swept the whole tree for the remaining variants of the claim; none left.
Addresses the suppressed comments on Copilot's second review of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
The three smoke headers and one AGENTS.md bullet still said CI runs all
three engines. That was written before pointing the smokes at WebKit
found #2132; the matrix was then narrowed to Firefox and these were not
brought along, so they claimed a gate that does not exist. Each now says
what CI actually enforces — Chromium in the build job's `npm run smoke`,
Firefox in the Sandbox smokes matrix — and where WebKit stands.
smoke-web-browser.mjs additionally notes that IT passes in WebKit; the
two App smokes are the ones that do not, and a reader of this file should
not infer otherwise from a bare '#2132' reference.
The fail-fast: false rationale no longer cites a WebKit-vs-Firefox
example it cannot currently produce, and says why the flag is still right
for a one-entry matrix.
Addresses Copilot review on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Every browser-driven check in this repo launched Chromium and nothing else.
That is fine for most of the web client, whose behavior is React and Mantine.
It is not fine for the MCP Apps sandbox, which is built out of the primitives
that genuinely diverge between engines: a CSP <meta> injected as the first
<head> child of a srcdoc document, a nested sandboxed iframe, a
Permissions-Policy allow attribute, and postMessage origin discipline across
those two frames. Nothing else covers that surface — sandbox-csp.test.ts
asserts which policy STRING is built (environment-independent by
construction), and no Storybook story reaches the sandbox at all, since all
three App stories use a data: placeholder iframe and a mock bridge.
The engine is now a parameter:
SMOKE_BROWSER=webkit npm run smoke:web:app
SMOKE_BROWSER=firefox npm run smoke:web:engine # all three smokes
Unset means chromium, so `npm run ci` and `npm run smoke` are unchanged. An
unrecognized value is an error rather than a fallback — falling back would
report a green Chromium run under a job labelled "webkit", claiming coverage
that never ran — and a launch failure names the engine that failed and its own
`playwright install` invocation.
Selection, launching, and the fatal-vs-benign page-diagnostics split now live
in one shared scripts/lib/headless-browser.mjs. The three smokes had each
hand-rolled the diagnostics split and two had their own copy of loadChromium;
pack:verify goes through the same helper, pinned to Chromium explicitly since
it is a packaging check.
The npm scripts drop their `cd clients/web && npx playwright install chromium`
prefix for scripts/install-smoke-browser.mjs. The engine is a variable now, and
a ${VAR:-default} expansion does not expand under Windows' cmd.exe; npx is also
a .cmd shim there that a shell-free spawn cannot start, so this resolves the
Playwright CLI through resolveNodeBin — the #1939 problem solved the #1939 way.
CI gains a `Sandbox smokes (<engine>)` matrix job rather than matrixing `build`,
which also runs validate, the coverage gate, two verify gates and Storybook —
all engine-independent. Both publish jobs now need it.
WebKit is supported by the tooling but deliberately NOT in the CI matrix yet:
pointing the smokes at it immediately found #2132, a pre-existing Safari
incompatibility in the web client's SSE transport that strands the last message
on /api/mcp/events, so an MCP App never leaves "loading". That is a product bug
affecting real Safari users, with its own diagnosis and a verified fix, and it
is not a matrix change. Adding `webkit` here is a one-word diff once it lands.
An engine is either green in CI or absent from it; a continue-on-error job would
report coverage nobody is held to.
Signed-off-by: cliffhall <cliff@futurescale.com>
`record` read `serversRef.current` from inside the `setRecords` updater. A
functional updater must be pure -- React may defer or replay it -- so the
pairing resolved at whatever moment the updater happened to run rather than
when the record was written. A list read committing in between would then be
paired with the record as its baseline, and the very read that should have
superseded the override would instead certify it.
Read the list once, at call time, and use it for both the pairing and the
prune.
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>
The rollback's explanation still described the removed app-wide boolean and
claimed the override was applied only for the active server. That is now the
live client's rule alone: the override is keyed by this write's server and is
only ever displayed while that server is active, so it is recorded whatever
is active by the time the rejection arrives.
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>
The displayed `paginatedLists` value falls back to the active `servers`
entry, and that list only advances when a `GET /api/servers` succeeds — so
once a list read fails it keeps describing disk as it was before the writes
made since. The optimistic override papered over that, but it was a single
app-wide slot cleared on every change of the *active* entry, so switching to
another server and back dropped it and the UI fell back to the stale entry.
Two things then disagreed, and the one the user could see was the wrong one:
the toggle read `off` while disk, the write tracker, and the client just
built from it all said `on`; and because the client skipped its connect-time
aggregate walk, the all-pages UI rendered an aggregate that was never
fetched, with no Load-next-page control to fill it.
Hold the override per server instead, in `usePaginatedListsOverride`, paired
with that server's entry as of when it was recorded and believed only while
the list still carries that same entry object — identity, not equality, so a
read reporting the value the override replaced supersedes it too. A server's
override then survives a switch away and back, and is still superseded by
the first successful read that rebuilds its entry.
The read cannot instead move to `lastPersistedSettings.resolve(id)`: that is
called during render, and it both reads refs and prunes superseded records.
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>
