Every variable that changes runtime behavior now lives on one page,
docs/environment-variables.md: which client reads it, its default, and
its effect. It covers three variables that were undocumented
(MCP_LOG_FILE, MCP_INSPECTOR_LOG_DIR, LOG_LEVEL), the secret-store
variables previously only in the Docker guide, and the Node TLS
variables that answer #1936 (a self-signed https://localhost server).
The README docs table, the migration guide, docker.md and the four
client READMEs link to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRJ64MnDTg1TpzCTEV9Xva
Signed-off-by: cliffhall <cliff@futurescale.com>
`local:gate` ran every client's unit suite bare inside `validate` and
again instrumented inside `coverage` — ~80s of a ~370s run re-executing
exactly the files the coverage pass runs a few minutes later. CI pays
nothing for that duplication because its two jobs run in parallel; the
gate runs serially, so it paid all of it.
The gate's first stage is now `local:validate`: the same `validate:guards`
and `validate:core`, then each client's new `check` script (`format:check`
+ `lint` + `typecheck` + `build`) instead of its `validate`, which is now
literally `check && test`. `npm run validate` and both CI jobs are
unchanged; `local:validate` sits in the `local:` namespace so the
workflow guard keeps it out of CI by construction.
The "strict superset" claim in AGENTS.md is reworded to what is now
true — every CI *check*, not every CI *invocation* — with the reasoning:
`@vitest/coverage-v8` reads V8's precise-coverage profiler and rewrites
no source, so the instrumented pass runs byte-identical code and is only
slower, the stricter direction for the failure class this repo sees.
Measured on the same worktree: 370s -> 262s wall (EXIT=0 both), the
four removed legs summing to 78.5s in the baseline run's own profile.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds `docs/cli-smoke-testing.md` — a copyable connect → list → call →
assert workflow for testing an arbitrary MCP server from a shell or a CI
job, plus index rows in the root README and a pointer from the CLI
README.
The gap this fills is a user-facing one. `clients/cli/README.md` is a
flag reference and `scripts/smoke-cli.mjs` is an internal end-to-end
check of the launcher → CLI path; neither tells someone how to compose
those flags into a check they run on every commit against their own
server.
Covers, in order: connecting over stdio/HTTP/SSE (including the `--`
separator, which splits the opposite way under `--cli`); `--format json`
and its `{result[, appInfo]}` envelope; asserting a tool exists with
`jq -e`; one representative `tools/call` and the `--tool-arg` vs
`--tool-args-json` coercion difference; the exit-code map and how to
branch on it; keeping OAuth non-interactive with `--stored-auth-only`
and an isolated `MCP_STORAGE_DIR`; and negative assertions — refusals,
a secret-shape scan of captured output, and `--strict` schema
portability. Ends with a complete script and a GitHub Actions job.
Every command and output shape in the guide was verified against the
bundled composable test servers over both stdio and HTTP rather than
transcribed from the reference, which corrected three claims that would
otherwise have shipped wrong:
- `--tool-arg zip=012` does not send `12`. `012` is not valid JSON, so
it is sent as the literal string. The real trap is `zip=10001`, which
does coerce to a number and is then rejected by a string schema.
- `if ! cmd; then status=$?` yields `0`, because `!` inverts the
pipeline's status — every failure class would look identical. The
guide uses `|| status=$?` and says why.
- The sample script does not report a failure class on its own; that is
the opt-in `case` block from §5.
The §8 script was run end to end against a live HTTP test server, and
exits 0/3/4/5/6 were each reproduced against a real server.
Closes#1886
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B
Signed-off-by: cliffhall <cliff@futurescale.com>
Staying abreast of SDK releases — OAuth especially — was a habit rather
than a mechanism. This adds the third of this repo's issue-filing sweeps,
after the monthly dependency refresh (#2229) and the daily Dependabot
alert sweep (#2233), following the same shape as both:
npm registry -> sweep -> issue -> Opus analysis comment -> maintainer PR
scripts/sdk-watch.mjs compares the @modelcontextprotocol/* packages
installed on v2/main against the registry and files one tracking issue
per upstream that is behind — labeled v2 + chore + dependencies, and
milestoned to the nearest dated open bucket.
Four design points, each verified against this repo first:
* Two upstreams, two issues. client/core/server/server-legacy ship
from typescript-sdk in lockstep; ext-apps ships from its own repo.
* The comparison is against the INSTALLED version, not the declared
range. ext-apps is `^1.7.4` with 1.7.5 already in the lockfile, so
comparing the declared string would file an issue for a bump
`npm install` has already taken.
* A fifth @modelcontextprotocol/* package added to the root manifest
and not added to SDK_GROUPS fails the sweep loudly, rather than
going unwatched behind a green run.
* No board write and no issue closing. No PROJECT_TOKEN exists in
this org, so the issue arrives labeled and milestoned for
/issue-triage to board — as dependency-refresh.mjs already does. A
further release files its own issue and leaves a supersession
comment on the older one; closing stays a maintainer act.
The analysis half runs Claude, not Copilot. #1063 sketched "a copilot
agent running Opus", and neither half of that is reachable from a
workflow: assigning copilot-swe-agent produces a pull request — the
artifact #2229/#2233/#2235 removed from this repo — and its model cannot
be selected programmatically, since replaceActorsForAssignable takes no
model parameter. So the analyze job uses anthropics/claude-code-action
with --model claude-opus-5, told to post one comment and denied every
file-writing tool, running on the ANTHROPIC_API_KEY org secret. It runs
only over issues the sweep just created, which holds it to one analysis
per release rather than a near-identical comment every night.
Upstream release notes are untrusted input to that job, so its system
prompt says so and its --allowedTools list is a whitelist with no Edit,
Write or git; the job's token carries contents: read only.
36 tests cover the pure halves directly and main() through an injected
spawn, with the silent-failure cases first: a registry error must not
report a clean sweep, and an already-filed target must not refile.
Closes#1063
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019JWUZxLDnPgLQzjBrtk1x9
Signed-off-by: cliffhall <cliff@futurescale.com>
- Filter to the npm ecosystem. Dependabot alerts are not npm-only and
this repo has a Dockerfile, so an alert against it would have been
parsed as a lockfile, thrown, and aborted the whole daily sweep
before any npm group ran. A non-npm alert is now reported loudly with
its GHSAs so a human can file it, and readManifest no longer lets one
unparseable manifest take the run down.
- Distinguish a superseded bump from a closed one. GitHub can revise
first_patched_version, moving an advisory to a different key while it
stays open; reporting that as "fixed or dismissed" would stand down a
live exposure. The reason now reads from the open GHSA set.
- Only claim security-update PRs are off when the run actually read the
setting. The guard degrades to UNVERIFIED, and an issue asserting
what the run could not confirm is worse than one that stays quiet.
- README.md and AGENTS.md: scripts/ now holds repo automation run from
CI, not only build/verify tooling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
`skills:eval` was at 38/46 with several skills measuring 0-33%. The low
scores were mostly bad eval cases rather than bad descriptions, for two
reasons that were nowhere written down:
- The harness runs each case with `--max-turns 1`, so the skill has to be
the model's FIRST tool call. A prompt asking "how does X work here", or
naming a concrete file, sends the model to Read/Grep first — a better
answer to the question, scored as a miss. Every 0% case was one of these.
- `AGENTS.md` is resident on every turn, so a prompt it already answers is
answered without any skill and the case can never pass.
Replace the cases that measured the wrong thing, expand every model-invoked
skill from 3 to 5 positives (negatives kept) so one flake cannot cross the
threshold, and reshape `local-dev`'s description to the action-first form.
Add `docs/skill-authoring.md` with the case shapes that fire, the ones that
cannot, and the probe-then-measure loop.
RUNS=5 npm run skills:eval: 63/63 at or above threshold, 60 at 100%,
all 18 negatives clean.
Closes#2202
Signed-off-by: cliffhall <cliff@futurescale.com>
Every runtime dependency core/ imports is now declared once, in the repo-root
package.json, and nowhere else. Deleting the client-side copies does not merely
align them: a package installs only into an install root that declares it, so
with no second declaration a second copy cannot be created at all — the failure
class #1896 was opened for and #1965/#2029 built verify:dep-lockstep to detect.
- clients/{web,cli,tui,launcher}: dropped ajv, atomically, chokidar, commander,
open, pino, zod, @napi-rs/keyring, hono, @hono/node-server and react, as each
applied. clients/cli and clients/launcher now declare no runtime dependencies.
- Root hono floor raised ^4.12.18 -> ^4.13.1, keeping the higher of the two
ranges that were in the tree rather than silently lowering web's.
- tsup external lists: added ajv and zod to all three bundles, plus atomically
and open (cli) and atomically (tui). tsup externalizes only what the nearest
manifest declares, so a package that moves to the root is inlined unless it is
named explicitly.
- vitest.shared.mts: repointed the pino, hono, @hono/node-server, atomically,
chokidar and @napi-rs/keyring pins from <client>/node_modules to the repo
root, joining express, yaml and proper-lockfile. Left behind they would
resolve to a directory that no longer exists, or to a transitive copy an
unrelated dependency dragged in. react and react-dom stay client-pinned so
the pair stays version-matched within an install.
- Docs: AGENTS.md dependency placement, the root README and the local-dev skill
describe the consolidated layout.
verify:dep-lockstep candidate set: 4 -> 2 (hono and pino fell out; only
@standard-schema/spec and zod still reach one tsc program from two installs).
npm run local:gate and npm run pack:verify both pass.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The CI wiring vouch accepted any syntactically-executable `run:`, so a step
moved into a release-only job or put behind `if: false` still reported as
wired while PR CI validated nothing. It now requires an UNCONDITIONAL step:
any `if:` on the job or the step disqualifies it, since this guard cannot
evaluate an Actions expression and "runs sometimes" is not the property being
asserted. Seven predicate cases, including the two conditional shapes and a
multi-line `run:` that must still count.
- The `local:gate` stage list now has one owner, `docs/quality-gate.md`. It had
three copies, and they drifted apart across rounds 5, 6 and 8 of this review —
which is the argument the PR itself is making, applied to itself.
Signed-off-by: cliffhall <cliff@futurescale.com>
- verify:skills:cli — the authoritative validator is now a GUARANTEED step in
`local:gate` and CI, not a best-effort hand-off. It resolves the CLI itself
(an installed one at 2.1.233+, else a pinned `npx -y`), so both tiers run the
same script and cannot drift. `verify:skills` keeps skipping when the CLI is
absent, which is right for `validate`: fast, offline, no Claude Code required.
- skill-manifest: the 1,536-char listing-entry cap covers `description` +
`when_to_use` together; checking the description alone passed an over-cap
entry. `when_to_use` is now parsed, validated and counted, with boundary tests
at 1,536/1,537.
- verify-skills: report the budget as *this repo's contribution*. The real
listing budget is shared with bundled skills and the contributor's own
~/.claude/skills, none of which is visible from here, so the number was
claiming more than it measured.
- skill-eval: a negative case now means "no skill OF THIS REPO'S fired". It
failed on any Skill invocation, so a contributor with an unrelated global
skill got a false failure about their environment rather than these skills.
- docs/architecture + local-dev: the Node backend runner gets its
`@inspector/core` alias from `clients/web/tsup.runner.config.ts`, not from
Vite. The old text sent runner work to the wrong config.
- local-dev: the tarball does not ship "only each client's build/" — the
allowlist also carries `clients/web/dist`, `clients/web/static` and the
install helper. States the invariant that actually matters instead.
- issue-triage: the test for needing triage is "no board card". Adding "and no
milestone" would skip the approved-but-unboarded case the same section then
handles. Also corrected the `$M` note: the lookup is `--state all`, so closed
issues DO have entries — `isopen` is about the invariants, not the lookup.
- issue-create: replaced "all five" with a per-version table. Steps 3 and 6
(milestone, Priority) do not exist for v1, so the old list opened with a rule
the same skill later tells you to violate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2163
`AGENTS.md` was one 1,223-line contract loaded in full on every turn, whether
the task was a one-line CSS fix or a release cut. This splits it: rules stay
resident, procedures move to `.claude/skills/`.
- Ten skills under `.claude/skills/`, seven `disable-model-invocation: true`
(invoked by name only, so they cost no listing context and have no trigger
question) and three model-invoked with committed eval cases.
- `AGENTS.md` 1,223 → 380 lines: rules, a skills index, and a project tree
reduced to one line per entry. The removed recipes are not duplicated.
- `README.md` 811 → 105 lines, split into `docs/{architecture,quality-gate,
test-servers,publishing,docker}.md`, and dropped from `CLAUDE.md`'s auto-load.
- `.github/copilot-instructions.md` and its mirroring rule removed — Copilot
reads `AGENTS.md`.
- `npm run verify:skills` (in `validate`, so in `local:gate` and CI) parses each
SKILL.md the way Claude Code does, requires an explicit invocation mode and
eval cases for model-invoked skills, and budgets the listing.
- `npm run skills:eval` runs the committed trigger cases headless.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 14. Two code fixes and a documentation sweep.
- `buildRevocationRequest` ran outside `revokeToken`'s best-effort try.
`encodeURIComponent` throws on a lone UTF-16 surrogate — valid JSON, so it
can reach there from a persisted client id or secret — and every caller has
already cleared its local state by then, so a rejection instead of a `failed`
outcome would break the documented guarantee. Construction is inside the try
now, and the catch reports `params.endpoint` rather than a `url` that may not
exist on that path.
- The fixture's Basic decode turned a malformed percent escape into an Express
500, reporting a bad credential as a server fault rather than as the
`invalid_client` 401 the endpoint means. Decoding failure is failed
authentication.
- Ten places still said revocation happens *before* the clear, which the
reorder in the previous commit made false: the settings-form description, the
`--no-revoke` help text, both READMEs, `docs/mcp-server-configuration.md`,
three JSDoc blocks, and the PR body. Swept, and the ordering is stated where
it is load-bearing rather than asserted in passing. Also merged the two
adjacent JSDoc blocks on `InspectorClient.clearOAuthTokens`.
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>
Two more review findings, both real.
The shim wrapped `effectiveAuthFetch` only, but the transport is constructed
with the base `this.fetchFn`, and the SDK runs its own discovery from inside
the transport on the 401/refresh leg — so a reconnect with an existing auth
provider could still hit the upstream failure. It now wraps the base fetch,
the same seam `withOAuthEndpointOverrides` uses and the only one that reaches
both paths.
That trades away the previous claim that a captured Network entry shows the
real 404: below the trackers, what they record is the substitution. Rather
than leave that silently misleading, a substituted response now carries
`x-inspector-oauth-metadata-source` naming the URL its body was actually
fetched from — the transport builds its tracker internally, so there is no
seam above it to install into instead. An integration test captures the fetch
the transport is handed and drives it against a real server, since nothing
else proves that wiring.
The wrapper also keyed on URL shape alone, but nothing stops an authorization
or token endpoint from living under `/.well-known/oauth-authorization-server/`;
a POST there returning an ordinary OAuth 4xx would have had its error replaced
by a metadata document. Metadata discovery is a GET, so anything else is now
left alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
- `slow_task` sleeps through a helper that removes both listeners on
whichever outcome wins and returns early on an already-aborted signal.
`{ once: true }` detaches on the event firing, not on the waiter losing
interest, so the old loop left one listener per second attached and a
60s run tripped MaxListenersExceededWarning.
- Report the outcome on stderr. A successful cancel closes the stream the
result would travel on, so the tool's return value is undeliverable by
construction — the README now points at the server's terminal rather
than at a result panel that cannot show it.
- Drop a double cast the file's own next hunk shows is unnecessary, and a
floating observer promise whose assertion was false anyway: `postSend`
rethrows before awaiting the SSE wait, so the send rejects on abort
whether or not that wait was cancelled.
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>
The Cancel button POSTed `notifications/cancelled` on every transport. On
a 2026-07-28 Streamable HTTP connection that is the wrong signal — the
spec makes closing the request's SSE response stream the cancellation
signal there and says the notification is neither required nor expected —
so a spec-compliant server acknowledged it 202 and dropped it while the
tool ran to completion.
The SDK already implements the fork, off `transport.hasPerRequestStream`.
Every Inspector connection is wrapped in `MessageTrackingTransport`, which
did not forward that flag, so the SDK read `undefined` and took the stdio
branch on every client — the CLI and TUI included, whose real
`StreamableHTTPClientTransport` advertises it correctly.
The web client needed three more links: its browser transport answers for
the upstream transport that lives on the backend, so it advertises the
flag for `streamable-http` and applies the SDK's `requestSignal` to its
`POST /api/mcp/send`; and `/api/mcp/send`, held open for the whole call,
forwards the browser's disconnect to the upstream `transport.send`.
The test fixture's `toWebRequest` built a Web Request with no `signal`, so
no fixture server could observe a disconnect at all — without that, a test
asserting the cancel reaches the server passes vacuously.
stdio and SSE keep `notifications/cancelled`: they multiplex every request
over one shared channel and have no per-request stream to close. So does
the legacy era, which predates the mechanism.
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>
Clearing the Inspector's OAuth state deleted the local copy of the tokens
and stopped there. The access token — and the refresh token, which is
long-lived by design — stayed valid at the authorization server until they
expired on their own, so a day of connect/disconnect iteration left it
holding grants for sessions that ended hours ago. RFC 7009 §1 describes
exactly that case, and nothing in the repo constructed a revocation
request, so a server author implementing `revocation_endpoint` had no way
to exercise it with the tool that exists to exercise this.
core/auth/revocation.ts builds and sends the request; the three clear paths
call it, so the behavior is written once — web's "Clear OAuth state and
disconnect" (both the active and stored-only paths), the TUI's Auth tab,
and the CLI's --relogin. It runs BEFORE the local clear, because the token,
the client credentials and the discovered `revocation_endpoint` all live in
the store the clear is about to empty. The request names the refresh token
when there is one: §2.1 asks the AS to also invalidate the access tokens
issued under the same grant, so one request covers both halves.
Best-effort by construction. Every path returns a TokenRevocationOutcome
rather than throwing: no advertised endpoint means nothing is sent and that
authorization server behaves exactly as before, and a network error, a
non-2xx, a 5s timeout or an unreadable store is reported and nothing more.
Forgetting the tokens is what the user asked for. `lost_authorization_state`
recovery deliberately skips it — it clears a half-finished flow to retry it,
and an authorization that never completed has no grant to revoke.
Client authentication resolves the preregistered slot first, mirroring
BaseOAuthClientProvider.clientInformation; reading only the dynamic slot
would send no authentication for a server configured with `oauth.clientId`.
The Basic credential is byte-identical to the SDK's applyBasicAuth so this
request and the token request cannot present the same secret differently.
Opt out per server with `oauth.revokeOnClear` (default on, only `false`
written to disk) or per CLI run with --no-revoke. Read at clear time rather
than connect time, so toggling needs no reconnect. Turning it off is a
testing affordance: a client that walks away still holding live tokens is a
case a server author may want to reproduce.
test-servers now advertises `revocation_endpoint` and serves POST
/oauth/revoke, faithful on §2.2 (an unknown token is a success) and §2.1
(revoking a refresh token invalidates the access tokens under the same
grant), with oauth-revocation-http.json / oauth-no-revocation-http.json as
showcase configs.
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 SDK's `discoverAuthorizationServerMetadata` picks its validation schema
from the well-known filename that resolved rather than from the document that
came back, so anything found at `/.well-known/openid-configuration` is
validated as an OpenID provider document — requiring `jwks_uri`,
`subject_types_supported` and `id_token_signing_alg_values_supported`, three
fields RFC 8414 does not define. RFC 8414 §5 permits that filename for general
OAuth metadata, so a conforming plain OAuth 2.0 authorization server is
rejected; and because the parse throws instead of continuing the candidate
loop, discovery aborts and the connection fails outright.
Filed upstream as modelcontextprotocol/typescript-sdk#2733.
`core/auth/oidcDiscoveryCompat.ts` works around it without fabricating a
field: on a failed RFC 8414 candidate it probes the OIDC candidates the SDK
would try next and, when one returns a document that satisfies
`OAuthMetadataSchema` but fails the OIDC schema, serves that body as the RFC
8414 response so the SDK picks the schema that describes it. A genuine OpenID
provider document is left to the SDK's own OIDC leg. Issuer validation runs
unchanged, since the substituted document is the one the server published.
It wraps `effectiveAuthFetch` — above the fetch tracker, the opposite of
`withOAuthEndpointOverrides` — so the Network tab still records the real 404
and the real probe rather than the substitution.
The candidate derivation mirrors the SDK's `buildDiscoveryUrls`, which a fetch
wrapper cannot call, and a test pins it against that export so the two cannot
drift.
Adds `oauth.asMetadataPath` to the composable test server (mirroring
`oauth.resourceMetadataPath`) and the `oauth-rfc8414-at-oidc-path-http.json`
showcase config that reproduces the failure.
Closes#2172
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review on #2176. All three findings were the same gap seen from
three angles: `"after-first"` counts *resource-subscription* listens, not
listens, and both the README and the option's own doc comment said
"the first listen".
That distinction is load-bearing rather than incidental — the Inspector
opens a listen at connect time whenever a list-change opt-in is live
(#1920), so counting listens would spend the allowance there and refuse
the user's very first Subscribe, leaving the documented repro describing
a trace nobody sees. It is also what a reader of the option would
otherwise get wrong.
- README: say "the first listen that subscribes to a resource" in both
places, and name the connect-time listen as the reason.
- `ServerConfig.modern.neverAcknowledgeSubscriptions`: document the
list-change exemption and contrast it with `true`.
- Integration test for the mode itself, with every list-change opt-in on
so the connect-time listen really is opened: it must be acknowledged
without consuming the allowance, the first resource subscription
acknowledged, the next refused with the stream left `active`.
Closes#2097
Signed-off-by: cliffhall <cliff@futurescale.com>
A modern-era `subscriptions/listen` is a long-lived request: the first
message on its stream must be `notifications/subscriptions/acknowledged`,
and the JSON-RPC `result` for the listen id is reserved as the
graceful-closure marker. A server that answers the listen request with a
bare `result` is saying "acknowledged and closed in the same breath" — the
SDK settles the subscription `graceful` and rejects the pending `listen()`.
The Inspector read that rejection as an ordinary drop: eight re-lists over
roughly a minute, the badge flickering `Reconnecting…`, and a final bare
`Stream ended` that said nothing about why. Accepting the message was
never wrong — it is a well-formed graceful-closure response. The silence
was.
- `core/mcp/subscriptionAck.ts` owns the predicate. The SDK gives this
close no code of its own — it is the same `SdkError(ConnectionClosed)`
any pre-ack close carries — so the predicate reads the message, since
the alternative is refusing to retry a genuinely transient drop.
- The condition is deterministic, so the stream ends on the first
occurrence instead of spending the whole backoff run. Both routes are
covered: the one a subscribe/unsubscribe reaches, and the one a
reconnect reaches when an acknowledged stream drops and the server has
since started refusing.
- A `"never-acknowledged"` status of its own, not `"ended"` — `ended`
means an expected close. It surfaces as the thrown error's message, an
orange "Not acknowledged" badge, and a notice in the panel.
- `subscriptions-never-acknowledged-http.json` reproduces it. It
acknowledges the first listen that subscribes to a resource and refuses
every one after, which is what makes the badge reachable by hand.
Closes#2097
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, ninth and tenth reviews:
- A custom `shell:` is a COMMAND TEMPLATE that Actions runs around every
script (`{0}` is the script path), so `shell: npm run local:gate && bash {0}`
invoked the gate while the `run:` value beside it looked harmless. Step
`shell`, and `defaults.run.shell` at both workflow and job level, are
command regions now. An ordinary `bash`/`pwsh`/`cmd` matches no rule, so
naming them costs nothing.
- GitHub's multiline environment-file syntax (`SMOKE_BROWSER<<EOF`, value,
delimiter) bypassed the assignment patterns. It is reported as an override
whose value the guard cannot READ rather than parsed: the value can arrive
through any number of shapes — a brace group redirected to $GITHUB_ENV, a
heredoc nested in another — and a parser handling some of them would be a
coverage claim that holds only for the shapes someone thought of. Nobody
spells `chromium` that way, and if they did, `SMOKE_BROWSER=chromium` is the
fix.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, seventh review:
- The unreadable-name rules only knew GitHub expression syntax, so
`ENGINE: firefox` plus `npm run smoke:web:$ENGINE` ran a forbidden pass with
the guard silent — the shell resolves the name, which makes it exactly as
unreadable as a ${{ }} expression and forbidden for the same reason. The
pattern keys off a leading $ or %, covering $VAR, ${VAR}, $env:VAR and cmd's
%VAR% in one, for both the engine and `local:` families.
- The env-key comparison was case-sensitive, but Windows environment variables
are not: `env: { smoke_browser: firefox }` on a windows-latest job sets the
very variable Node reads as SMOKE_BROWSER. Compared case-insensitively now,
as is the POSIX assignment pattern. On a POSIX runner that spelling would be
a different variable and harmless, so this is stricter there than it needs to
be — the right direction for a guard, and the alternative is a verdict that
depends on a `runs-on` which may itself be an expression.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, sixth review: the run-script rule matched POSIX `NAME=value` only.
PowerShell is the DEFAULT shell on a Windows runner, so
`$env:SMOKE_BROWSER = "firefox"; npm run smoke` redirected the suite with the
guard silent — on exactly the workflow least likely to be read closely.
Three patterns now, one per dialect: POSIX (which also covers cmd's
`set NAME=…` and the GitHub-native `>> $GITHUB_ENV` append, since both carry
the same substring), PowerShell's `$env:NAME =` and `${env:NAME} =`, and the
.NET SetEnvironmentVariable call reachable from PowerShell. Regression cases
for all five spellings, plus a negative for a PowerShell assignment of
chromium.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, fifth review:
- The SMOKE_BROWSER rule was a blanket text match across every region, so a
script that merely PRINTS the variable — `echo "SMOKE_BROWSER: firefox"` —
or a `with:` input mentioning it was reported as an override. It is
kind-aware now: an `env:` entry counts only when SMOKE_BROWSER is literally
its key (recognized structurally, not by regex), a `run:` script only on a
shell assignment, and a `with:` value not at all — whatever it names is
already covered by the script rules. A shell assignment inside an `echo` is
indistinguishable without parsing the shell, and inside `run:` erring toward
the finding is the safe direction; that is stated in the header.
- `jobs.<id>.container.env` is job-wide too — every step in the container sees
it — so an override there redirected `npm run smoke` with the guard silent.
Now traversed, and a `container:` given as a bare image string is ignored
rather than tripping over.
- Nothing asserted the FIRST acceptance criterion: reintroducing a `ci` or
`ci:*` root script left test:scripts green. Asserted directly over the
manifest's keys now, and checked to actually bite by reviving `ci` and
watching the suite fail.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, fourth review — four findings, all real:
- Matching pairs by KEY NAME anywhere in the tree scanned things that execute
nothing. Workflow input ids are user-defined, so an input legitimately named
`env` put its own description and default in front of the rules — failing
the gate on documentation OF the gate. The walk is structural now: workflow
`env`, job `env`/`with`, step `run`/`env`/`with`, and nothing else.
- YAML aliases were read as scalars, which have no `.value` on an alias node —
so `run: *command`, with the command anchored in a metadata scalar the walk
never visits, invoked anything at all while the guard saw an empty string.
Aliases are resolved, and reported at the line where they are USED.
- `file` was accepted for diagnostics and then not forwarded, so a malformed
real workflow was reported as `<workflow>`.
- One negative fixture was a YAML comment, which the parser strips — so it
asserted nothing. Inside a `run:` block the comment text really does survive
into the scalar, so the "a comment invokes nothing" rule is now applied
there, where it actually has to be, and the fixture is a real block scalar.
The table's fixtures are schema-valid workflows now rather than bare `- run:`
fragments, which a structural walk correctly cannot see.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, third review — and the second finding is really a verdict on the
approach, so the fix is the approach:
- `SMOKE_BROWSER:` with no value is valid YAML that sets the variable to the
empty string, which `resolveBrowserName` rejects as present-but-empty rather
than treating as unset. The value pattern required \S+, so the guard said
nothing. It is \S* now, with its own message.
- The hand-rolled opener could not see `run: | # explanation` or the flow
mapping `env: { SMOKE_BROWSER: firefox }`. That is the third spelling it has
missed in three rounds, each one a forbidden invocation that would have
passed while test:scripts stayed green — worse than no guard, because it
reports a coverage that is not there. YAML has too many spellings to
recognize by indentation, so extractExecutableRegions now parses with the
`yaml` package. It is already a root dependency, and this file is not in the
published files allowlist, so nothing about the tarball changes.
A parse error now throws rather than yielding no regions: a workflow the guard
cannot read must not pass as a workflow with nothing in it.
Verified by injecting a flow-mapping SMOKE_BROWSER override into the real
main.yml — the suite fails, and goes green again when removed.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, second review — both findings were real:
- A `${{ … }}` body can itself contain braces, so the `[^}]*\}\}` arm added in
the previous round stopped at the first `}` and missed
`smoke:web:${{ format('{0}', matrix.browser) }}` — the very case it was
added for. It now keys off the `${{` prefix, which nesting cannot defeat.
- Scanning every non-comment line made ordinary metadata a finding:
`name: Explain why smoke:web:firefox stays local` executes nothing and was
reported as an engine invocation, contradicting the header's "what it
forbids, and nothing more". The scan now walks executable positions only —
`run:` scalars (inline and block) and `env:`/`with:` values — via an
exported `extractExecutableRegions`, hand-rolled because `test:scripts` is
dependency-free by design. The narrowing is a stated limit, not a hidden
one: an execution vector outside those three positions is not seen.
Verified by injecting a nested-brace SMOKE_BROWSER expression into the real
main.yml — the suite fails, and goes green again when removed.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, first review:
- The guard read only literal names, so `npm run smoke:web:${{ matrix.browser }}`
and `npm run local:${{ matrix.task }}` walked past it — the one form in which
a forbidden engine reaches CI without anyone typing its name. It already
refused an expression-valued SMOKE_BROWSER for that exact reason; this applies
the reasoning consistently. Its limit is now stated: a fully opaque
`npm run ${{ matrix.script }}` names no family and is not detectable.
- The module header claimed the gate adds "the non-Chromium engine passes";
it adds Firefox only, and WebKit belongs to neither tier.
- eslint.config.js:33 still said `npm run ci` — the last stale reference.
- AGENTS.md read "everything from `smoke` rightward is `local:gate` only",
which is two claims wrong: smoke and Storybook run in GitHub CI too, and
`pack:verify` is in neither tier.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
The root `ci` script was never what runs in GitHub CI, and the name worked
against us three ways: it collides with the built-in `npm ci` (which does
something else entirely and is one keystroke away), it invites the
cross-engine passes into a workflow by looking like the thing a workflow
should call, and it blurs "GitHub CI" and "the local gate" in every doc
that names it.
- Rename `ci` → `local:gate` and `ci:storybook` → `local:storybook`, with
no back-compat alias: `npm run ci` now fails with npm's missing-script
error, which prints the available scripts.
- Add `scripts/lib/workflow-gate.mjs` + its `test:scripts` suite, which
runs over the repo's real `.github/workflows/**` and fails on an
invocation of a `local:*` script, of a non-Chromium engine pass (derived
from SUPPORTED_BROWSERS), or of `smoke:web:engine`, or on `SMOKE_BROWSER`
set to anything but a literal `chromium`. It does not forbid
`npm run smoke`, `smoke:web:chromium` or `smoke:tui` — those belong in CI
and are there today. Comments are not scanned, so the workflow can still
explain itself. This is the durable half; the rest is prose.
- Document the two-tier model once, in the root README ("Two tiers: GitHub
CI and the local gate"), and point the other docs at it rather than
restating the split.
- Sweep the ~40 stale references across README, AGENTS.md,
.github/copilot-instructions.md, the client READMEs, the roadmap doc and
the script headers. AGENTS.md and copilot-instructions.md change
together, per the mirror rule.
No gate changes what it actually runs.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
The web client had exactly one connected-flow smoke and it was the Apps
tab. `smoke:web:browser` proves the bundle boots and paints but never
connects; `smoke:web:app` / `smoke:web:elicit` connect but only drive the
Apps surface. So the three highest-traffic tabs were unexercised through
the prod bundle end to end — everything covering them (unit tests,
Storybook, the integration project) mocks or bypasses the prod server, so
a regression living only in the built bundle or in the prod Hono
backend's wiring of a tab passed all of them.
`scripts/smoke-web-tabs.mjs` drives all three against one server on one
browser launch (~5s added wall-clock): Tools runs `list_items` and
asserts its `structuredContent` section rendered (#1908), Resources reads
`events` with the template list asserted separately — `resources/list`
and `resources/templates/list` are different calls and either can fail
alone — and Prompts fetches the argument-less `simple_prompt`.
Every assertion is on a `data-*` attribute, never on copy, mirroring the
`data-app-status` precedent: each screen now carries a readiness contract
(`data-tool-count` / `data-call-status` and its Resources / Prompts
equivalents), tabulated in the web README and pinned by screen unit tests
so a rename fails loudly there rather than as an opaque timeout here.
The deep-link connect step is extracted to `lib/deep-link-connect.mjs`
rather than copied a third time. That matters because `parseDeepLink`
ignores a link it cannot validate rather than reporting one, so a drifted
copy fails as an opaque connect timeout in whichever smoke was not
updated. `smoke:web:elicit`'s hand-rolled copy is retired onto it.
Chromium-only and deliberately not in `ENGINE_SMOKES`: the engine tiers
exist for the MCP Apps sandbox's genuinely divergent primitives, and
these tabs are ordinary React and Mantine. Network, Protocol, Logs and
Tasks remain uncovered — a stated gap, not an oversight.
Closes#2148
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
The old name — "Run install, format, lint, build, and test on every push" —
enumerated steps rather than naming the workflow, and was wrong in two
directions: it never mentioned the coverage gate, and "on every push" reads
past the two release-gated publish jobs. `CI` renders as `CI / build` and
`CI / coverage` in the checks list, where the job name already carries the
detail.
Display-only: nothing references the name (no badge, no `workflow_run`, no
sibling workflow), and branch protection keys off JOB names, which are unchanged.
Also: README described the publish jobs as `needs: build`, which the previous
commit falsified.
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>
Copilot review round 4.
`exit` can be observed before the child's last output drains — `done()`
already keeps reading until `close` for exactly that reason — so `onData`
can run after the verdict is settled. Recording `markerAt` there is still
right: the message is a thunk rendered at `finish()`, and a marker still
sitting in the pipe when `exit` fired was painted all the same. Arming a
survival timer is not. It could not change the result (`done` is a no-op
once settled), but it holds the event loop open for the whole window,
which under `node --test` is a suite lingering seconds for nothing.
That implies something the comment did not say, and it had already bitten
this PR: if `markerAt` can be set after the exit was observed, any wording
derived from observation ORDER is unstable. The old message read
"rendered at Nms and then exited Mms later", where M was
`since() - markerAt` evaluated at finish — not the paint-to-exit gap at
all, and incoherent whenever the marker surfaced during the drain. The
test asserted on /survival window/, which the losing side of that race
could drop. It could have flaked, and the flake would have looked like
the bug this PR fixes.
So the message now makes no ordering claim and reports both times as the
observations they are, with `exitAt` captured in the exit handler rather
than recomputed later. Assertions moved to the race-stable phrase, plus a
test that runs the paint-then-die stub 12x asserting verdict AND wording
are identical every time — one pass proves nothing about a race.
The ordering could not be forced on darwin, so the guard is reasoned
rather than reproduced, and the reply says so. Two attempts and what they
actually showed: a 4MB payload with the marker last never gets there,
because `process.exit()` discards pending stdout writes (the "before
rendering" branch is correctly taken); marker on stderr with a graceful
exitCode was observed first in all 30 runs, none lingering near the
window. `done()` exists precisely because exit and close are separate
events, so treating their order as guaranteed was worth removing anyway.
Also (suppressed comment): the README's "Two more do the same" clause now
lists four suites, not two, after this PR added render-smoke and pty.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
`smoke:tui` spawned the launcher with `stdio: ["ignore", …]`, waited for
"MCP Servers" in the output, printed OK, and SIGTERMed. It never looked at
the child again.
The child was already dead. Ink mounts `useInput`, `useInput` needs raw
mode, and raw mode is a property of the file descriptor — with stdin on
/dev/null the TUI painted one frame and exited 1 about 40ms later with
"Raw mode is not supported on the current process.stdin". Measured here:
first frame at 396ms, exit 1 at 433ms. The smoke won that race and
reported success, so what it asserted was *first paint*, not survival —
contradicting its own header ("boots and renders without crashing").
This is not environment-specific: `stdio: ["ignore", …]` makes the child's
stdin /dev/null whether or not the developer's terminal is a TTY, so this
smoke has never verified a running TUI on any machine. And the race cut
both ways — an `exit` processed first failed the run, which reads as flake
rather than as the standing defect.
Two pieces:
- `scripts/lib/pty.mjs` gives the child a real terminal via `script(1)`,
no dependency. The three flavors are not interchangeable (BSD takes
argv; util-linux takes one shell string plus `-e`; busybox that string
without `-e`), so the invocation is built and unit-tested rather than
written inline. A platform with no `script(1)` skips loudly instead of
running without one, which would be a guaranteed failure rather than a
weaker check.
- `scripts/lib/render-smoke.mjs` is the fix: the child must still be
running `surviveMs` (default 2s) after first paint. Without that, a
future harness change reintroduces the same false green and nothing
notices — which is why it lives in a module `test:scripts` drives
against a stub that paints the marker and immediately exits. Today's
harness passes that stub; this one fails it, with a diagnostic that
distinguishes "died after painting" from "died before painting".
The smoke additionally fails if the raw-mode error appears in a passing
run, so a future Ink that degrades to a warning instead of throwing can't
ship a TUI with no keyboard input past this gate.
Verified end to end: with a `script` shim that allocates no pty, the smoke
now fails with "it painted one frame, it did not run"; with the real one
the TUI is still running 2s after its first frame and never logs the
raw-mode error.
The `process.env.CI` skip stays, but its comment is corrected: the PTY
removes the technical blocker it used to cite, so the smoke is local-only
by decision now, not by capability. Whether it joins GitHub CI is out of
scope here (#2146 is keeping the other local-only smokes out on purpose).
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
- `const` constrains a property that is PRESENT; it neither requires the
property nor acts as a default. It is now supplied automatically only where
the schema also requires the field — in `collectSchemaDefaults`,
`applySchemaConstants` and the TUI's initial values alike — so an optional
`dryRun: { const: true }` no longer turns a valid `{}` call into one that
asks the server to do something. A value that IS present is still corrected
to the constant, whatever supplied it, and the field stays read-only.
- Offerability is judged on the base+member merge rather than the raw member,
so `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` over
root-declared properties is offered. It was declined, which left the gate
checking the base alone and accepting `{}` — which that schema rejects.
- The TUI's branch sections keep the schema's own `required` for the seeding
decision while still rendering every branch field non-required, which is what
keeps a static form submittable.
Signed-off-by: cliffhall <cliff@futurescale.com>
The README claimed both showcase tools rendered nothing but the Execute Tool
button, which the PR's own screenshots contradict: `echo` carries a root
`message` and rendered that one field, so it was callable with half its
arguments; only `get_weather`, whose fields live entirely on its `oneOf`,
rendered no controls at all. The two now show the two halves deliberately.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all from Copilot's suppressed block.
1. The tests never checked that loadBrowser launches the engine it was
asked for. The failure-path stand-in rejects identically for every
engine, so loadBrowser(root, "firefox") could have called
chromium.launch() and still produced a correctly Firefox-labelled
error. The smokes could not catch it either: the Firefox gate runs
after Chromium is already installed, so a mis-dispatched launch would
SUCCEED and report Firefox coverage that never happened.
Added a success-path test with distinct per-engine spies asserting the
requested engine is invoked, that nothing else is, and that its handle
is what comes back. Mutation-verified: hard-coding playwright.chromium
fails it.
2. The launch-failure message told the reader to run
`npx playwright install --with-deps <engine>`, which is wrong now that
the npm scripts run from the repo root rather than clients/web -- I
introduced that when I removed the `cd`. Playwright is pinned in
clients/web, so a bare root-level npx can fetch a different version and
install a browser revision the pinned one still cannot launch. It now
points at `npm run smoke:web:<engine>` first, and scopes the raw
command to clients/web with the reason.
That message named smoke:web:webkit, which did not exist -- only the
chromium and firefox scripts did. Added it, and a test asserting every
supported engine has one, since a remedy nobody can run is worse than
no remedy. Mutation-verified.
3. AGENTS.md said npm run ci "mirrors" the workflow and then, two
sentences later, that it is "a superset, not a mirror". It now leads
with strict superset, which is the topology this PR is documenting.
The README and AGENTS.md remediation wording is corrected the same way as
the runtime message, so a reader gets the same advice wherever they find
it.
Addresses Copilot review 5030668906 on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
A distinct `const` does not make `oneOf` alternatives mutually exclusive
unless the property carrying it is required: two branches pinning an OPTIONAL
`kind` both match `{}`, so arguments omitting it satisfy more than one
alternative — exactly what `oneOf` forbids. The property must now be required
by the root or by every branch.
Also makes `propertiesOf` total (an unreadable `properties` reads as empty,
with a separate predicate for the one caller that must tell them apart), which
removes a row of unreachable `?? {}` fallbacks, and covers the value/type
comparisons the const checks rest on.
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: 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>
- 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>
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>
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>
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>
