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>
Step 5 of the phase-2 App.tsx decomposition (#2129, under #2126): the two
sandbox bridge factories, the renderer handle, and the app-rendered
elicitation controller move out of App.tsx into a hook of their own.
Nearly a leaf — it reads the active client and the resource listing and
calls nothing back — so `getListedResourceMeta`, `publishDocument` and
`listedResourcesRef` become internal to the hook rather than App.tsx
locals. The hook is called where `resources` is derived, which is what
lets it own the latest-listing ref write too.
`sandboxUrl` and `initialConfigSettledRef` deliberately stay in App.tsx:
whether the sandbox exists decides whether the client advertises the
nested MCP Apps `elicitation` capability, and that is a
connection-lifecycle question rather than an Apps one.
Inert by construction: no behavior change, and the extraction is covered
100% on all four dimensions plus the existing `smoke:web:app` and
`smoke:web:elicit`, in both Chromium and Firefox.
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>
Three findings, all valid.
**The read/get stages asserted an RPC status, not a render.** #2148 asks
for the resource content and the prompt messages to *render*, and
`data-read-status` / `data-get-status` flip the moment the call resolves
— so the smoke stayed green with the panel deleted. `ResourcePreviewPanel`
and `PromptMessagesDisplay` now carry `data-testid="resource-preview"` /
`"prompt-messages"`, and each stage waits on the status **and** the
rendered panel. Verified by breaking each testid: the smoke fails with
`reading `events` did not render a preview — last observed:
data-read-status="ok"`, which is the finding stated back exactly.
**`smoke:web:tabs` followed ambient `SMOKE_BROWSER`**, so
`SMOKE_BROWSER=firefox npm run ci` would have run the supposedly
Chromium-only smoke under Firefox — contradicting both this PR's stated
scope and the gated-tier invariant in `run-engine-smokes.test.mjs`. The
npm script now pins `chromium` on **both** halves (install and launch)
and the smoke resolves through `resolveRequestedBrowser(argv, env)`, where
an explicit argument wins over the environment; a new case in that same
test file pins it, including that the smoke stays out of `ENGINE_SMOKES`.
**The AGENTS rules were not mirrored into `.github/copilot-instructions.md`.**
Correct, and worse than an omission: that file's engine bullet asserts the
headless web smokes are "engine-parameterized, not Chromium-only", which
this PR made false. It now names `smoke:web:tabs` as the deliberate
exception, and carries the genuinely review-citable new rule — assert
`data-*`, never copy, and never stop at an RPC status where the ask was
that something render.
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>
- Publish `setupClientForServerRef` from a layout effect rather than during
render. React runs every layout effect before any passive effect of the same
commit, and the /oauth/callback effect is passive, so the seam is still
published before its first read — without the render-phase ref mutation the
compiler rule rejects. This was the one `react-hooks/refs` finding the PR
introduced, so its suppression is gone rather than deferred to #2161 under a
justification that did not describe it.
- Narrow the hook's `fetchLogRef` option to `FetchRequestSource`, the
`getFetchRequests` slice it actually consumes, mirroring
`InspectorClientEventSource` in useProgressToasts. That removes the test
harness's `as unknown as` cast, and the narrowed contract is real: it
immediately rejected the loosely-typed fixture, which is now a properly typed
`FetchRequestEntry`.
- Make the test harness hold both refs across renders and publish them from a
layout effect, as App.tsx does. The deferred-publication test previously
passed because `Probe` built a fresh ref each render and the rerender also
changed `servers`; it now asserts what the guard is actually for — a run that
cannot exchange must not spend the single-use code — and re-triggers on the
`servers` change that carries it in production.
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 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>
Phase 2 step 2 of the App.tsx decomposition (#2129, under #2126). App.tsx goes
from 4,267 to 3,178 lines; no behaviour change.
The new hook owns the step-up slot and its retry operation, the re-auth banner,
the deferred background-tab recovery, the redirect plumbing (webOAuthStorage,
sessionStorageAdapter, onBeforeOAuthRedirect, prepareOAuthRedirect), the
command-scoped recovery wrappers, the four OAuth effects, the abandoned-redirect
check, the /oauth/callback completion, the Connection Info OAuth details, and
the clear/finalize paths. All five once-only latches move with it unchanged.
Three seams could not be a straight move:
- useSessionRef no longer takes pendingStepUp/pendingReauth. They are this
hook's own state, so App.tsx would have had to hold them purely to hand them
back; instead the owner mirrors them into the ref in an effect of its own,
following the shape activeServerName already had.
- setupClientForServer is injected through a ref, because it reads
onBeforeOAuthRedirect and sessionStorageAdapter out of this hook and so
cannot be declared before it. The callback effect reads the ref before
burning its one-shot latch.
- The two clear-OAuth modal adapters and onReauthenticateFromBanner stay in
App.tsx: they bind App-owned state, and the latter drives a reconnect, which
step 3 owns.
Dropping ~1,100 lines took App.tsx under the React Compiler's bail-out, so the
compiler-backed react-hooks rules now analyse it and report nine pre-existing
findings that were previously invisible. Each is suppressed with a narrow
justified disable naming #2161, which is filed to resolve them; the one new
finding (exhaustive-deps on setReAuthBanner) is fixed rather than suppressed.
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 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>
`build` ran `npm run validate` and `npm run coverage` back to back, and on a
recent green run those two steps were 84% of a 17m9s wall clock (7m41 + 6m50).
They are serial for no reason: `coverage` consumes nothing `validate` produces.
Every client's `test:coverage` is self-sufficient — web and cli build the test
servers themselves (cli its own bin too), tui and launcher run from source — and
the only consumers of `clients/*/build` are the smokes, which stay in `build`
alongside the `validate` that produces them.
So split `coverage` into a sibling job. Both publish jobs become
`needs: [build, coverage]`: that is the safety property, not a side effect —
left as `needs: build` a release could publish with the coverage gate red.
Deliberately NOT backgrounding the two inside one job: that puts two vitest
fleets on one 4-core runner, which already times tests out at the 5s default.
Wall clock ~17m -> ~10m, for one extra runner and a duplicated ~24s install.
Splitting `coverage` per client is the next lever and stays out of scope.
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 5 — four findings, all in the suppressed block.
1+4. The regression stubs did `console.log(MARKER); process.exit(1)`.
Writing to a PIPE is asynchronous and `process.exit()` tears the
process down without draining, so the marker can be lost outright —
and then the stub that pins the paint-then-die path fails as "exited
before rendering", the neighbouring branch and the wrong diagnostic
for the thing it guards.
Worth recording that I met this mechanism last round and did not join
the dots: probing the drain race, a 4MB write followed by
`process.exit()` reached the parent with the marker missing
entirely. That was noted as a reason the repro failed, while the same
construct stayed in the stubs. Both now set `process.exitCode` and
let the loop drain — still exiting immediately after the write — with
a comment, since `process.exit()` is the more obvious spelling and
will read as a simplification to the next person. Same fix for the
`console.error("boom")` stub, whose assertion had the same exposure.
2. DEFAULTS claimed "every one is a hard floor as well as a default",
which contradicts `normalizeMs` (any finite positive value passes)
and the suite two files over, which passes 400ms against a 2000ms
default. It rejects the shape a bad env var produces, not smallness.
3. The raw-mode forbidden-output check was unreachable from any test:
the real PTY happy path only exercises the pattern NOT matching, so
a regression breaking it would stay green forever — an
acceptance-criterion guard with nothing guarding it, which is the
recursive form of this PR's own complaint.
Now a `runRenderSmoke` option (`forbidOutput`), so it runs inside the
tested state machine and against the fully drained output.
`smoke:tui` passes its pattern in and drops the post-hoc check. It
can only turn a pass into a failure, never rewrite an existing
failure's message: a crash reason is more useful than "and it also
printed X", which is usually a symptom of that crash. Three tests —
matching, not matching, and that asymmetry.
Verified: test:scripts 213/213; `smoke:tui OK — rendered "MCP Servers" at
360ms and was still running 2000ms later`; and the no-PTY shim still
fails through the refactored path.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
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>
Copilot review round 3.
`done()` is first-caller-wins, and the render deadline stayed armed after
first paint. So a marker landing near `timeoutMs` let that timer fire
*inside* the survival window and settle the run as
child did not render "MCP Servers" within 15000ms
followed by an output tail that visibly contains "MCP Servers". A TUI that
is merely slow fails intermittently, with a diagnostic naming the one
thing that demonstrably did happen — the same wrong-attribution class this
PR exists to remove, and one that reads as flake.
First paint has happened; its deadline no longer has a question to ask, so
`onData` clears it at the moment the marker is observed.
Also hoists `renderTimer`'s declaration beside `surviveTimer`'s. It was a
`const` below both closures that clear it, safe only because `onData` and
`done` run from events rather than the synchronous body. That already held
for `done`; this fix adds a second reader, so the ordering is now stated
rather than relied upon.
The test paints at 250ms with timeoutMs 400 and surviveMs 400, so the
render deadline expires while the survival window is open — the exact
shape. Verified it fails against the module with the clearTimeout removed
and passes with it: a wider timeoutMs would have been green either way,
which is why the numbers are tight.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings, both of which quietly undo the PR.
1. `SMOKE_TUI_SURVIVE_MS` was unvalidated. `??` guards only nullish, so
an env var set to the empty string reaches `Number("")` -> 0 and a typo
reaches NaN; `setTimeout` fires immediately on both. The survival check
would then resolve before the child could possibly have died, printing
"still running 0ms later" and asserting nothing — the exact first-paint
false green this PR removes, restored by an empty environment variable,
on a green run.
Fixed at the module rather than only at the env-parse site, so every
caller is covered: `normalizeMs` (same shape as child-cleanup.mjs's
`normalizeGraceMs`, matching an existing policy rather than inventing a
second one) applied to all four durations before anything reads them,
which also stops the messages interpolating "NaNms". Defaults moved to
an exported DEFAULTS so the smoke and the module cannot drift.
`surviveMs: 0` is deliberately NOT honored as "skip the wait": no
caller wants that, and it is indistinguishable from the typo.
2. (suppressed comment) `spawnSync` REPORTS ENOENT in `result.error` — it
does not throw — so `probeScriptVersion`'s try/catch read a missing
binary as "ran, printed nothing". On linux that empty output fell
through to the util-linux guess, and the smoke then hard-failed
spawning a binary that does not exist instead of taking the documented
skip.
`probeScriptVersion` now returns `{ available, output }`, and
`resolvePtyWrapper` returns `{ ok: true, flavor, wrap }` or
`{ ok: false, reason }` — the two unavailability causes deserve
different skip messages, since "no script(1) on PATH" and "nobody has
verified this platform's invocation" send the reader elsewhere. A
non-zero exit is explicitly not unavailability: BSD `script` has no
`--version` and answers with its usage, which is the evidence the
flavor fallback reads.
Tests for both, since each finding lives entirely in a branch no happy
path reaches: the normalizeMs table (empty string, typo, undefined, -1,
Infinity, a numeric string, 0), an end-to-end run proving a
`surviveMs: Number("")` paint-then-die stub still fails, the
`result.error` ENOENT and timeout shapes, and both `resolvePtyWrapper`
failure reasons.
Verified: `SMOKE_TUI_SURVIVE_MS="" npm run smoke:tui` reports "still
running 2000ms later"; with PATH blanked, resolvePtyWrapper returns
`{ok: false, reason: "no `script(1)` on PATH"}` rather than guessing.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
- pty.mjs: narrow the BSD argv set to {darwin, freebsd}. Grouping every
BSD together was wrong — NetBSD's `script` takes the command through
`-c`, so the argv form fails at `script` startup, which this smoke would
then report as a TUI crash. Rather than add an unverified `-c` branch
for netbsd/openbsd, they fall through to `null` and the smoke skips with
the reason: a guessed invocation turns a clean skip into a false failure
while looking like support.
- pty.test.mjs: "supported unless Windows" would fail `test:scripts` on
aix/sunos/netbsd/openbsd, where a null wrapper is correct and the smoke
skips by design. State the policy once (SUPPORTED_PLATFORMS) and branch
on it, keeping the real-probe teeth that catch a `script` missing from
PATH. Pin netbsd/openbsd/aix/sunos to null in scriptFlavorFor's tests
so "unsupported" reads as the default, not a win32 special case.
- render-smoke.mjs: the @returns contract claimed a close guarantee the
code does not give — both drain give-up branches resolve after only a
warning, so output can be truncated and the child may still hold the
work dir. Document the bounded fallback and name the required cleanup
shape (removeSafe, not rmSync), since a caller "tightening" cleanup on
the old wording would reintroduce #1801 exactly where it doesn't hold.
- AGENTS.md, clients/launcher/README.md: drop the pre-#2111 claim that the
smokes build test-servers "on demand if it is missing". ensureTestServers
rebuilds unconditionally, once per process — presence is not freshness.
- .github/copilot-instructions.md: mirror the survival rule, per the
same-PR mirroring requirement. One bullet, generalized past this fix:
a smoke must assert the thing survives, not that it started, and the
started-then-died branch belongs in a scripts/lib test against a stub.
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>
Addresses Copilot's review on #2152. The client stand-in was
`({ tag }) as unknown as InspectorClient` — a double cast with no
justification, which AGENTS.md rejects.
`Object.create(InspectorClient.prototype)` gives a real prototype chain
without running the constructor, so it needs only a single cast and still
drags in no transport or environment. The tag went with it: every
assertion compares identity, and `Object.create` already returns a
distinct object per call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MZdpUwhHf943FoprcLVzEm
Signed-off-by: cliffhall <cliff@futurescale.com>
Step 1 of phase 2 of the App.tsx decomposition, landed first and alone
because it is the seam the remaining clusters are extracted onto.
App.tsx kept six "latest value" refs so long-lived callbacks could read a
current value without listing it as a dependency and being re-created on
every change: `activeServerNameRef`, `activeServerIdRef`, `serversRef`,
`inspectorClientRef`, `pendingStepUpRef` and `pendingReauthRef`, plus the
three passive effects that synced them. `useSessionRef` replaces all nine
with one hook returning one stable ref onto a `SessionSnapshot`.
This is not a store: nothing renders off it, nothing subscribes to it, and
mutating it never schedules a render. It is not the prop-to-state sync
AGENTS.md forbids either — that rule is about `setState` in an effect, and
a ref write produces no render at all. It is exactly the pattern the file
already used, written once instead of six times.
What it buys the later steps is a *stable* dependency: a hook taking
`sessionRef` does not drag the other clusters into its argument list, so
`useOAuthRecovery`, `useConnectionLifecycle` and `useServerCommands` can
be extracted one PR at a time.
Behavior is unchanged:
- The snapshot is seeded from the first render's values, matching what
each `useRef(value)` seeded.
- `activeServerName` keeps its one non-uniform rule — it holds the last
server that *was* active and is never cleared, because a transport
crash clears `activeServerId` before the failure toast is raised. It is
derived from `servers`/`activeServerId` rather than taking the
`activeServer` memo, which is the same value by definition and is what
lets the hook be called ahead of that memo.
- The two pending-* syncs already ran ahead of the only effect that reads
them, so merging them changes no effect's view. The other four synced
below every effect that reads them, so hoisting that sync changes none
either.
- The sync effect is deliberately un-gated rather than carrying a
dependency list: re-assigning unchanged values costs nothing and
removes a place for a newly-added field to be forgotten.
`sessionRef` is declared ahead of its first reader because a hook return
is not provably stable to `react-hooks/exhaustive-deps` — consumers must
list it, and a later declaration would put every one of them in the
temporal dead zone. Listing a ref that never changes identity re-runs
nothing.
The inline `pendingStepUp` state shape is named `PendingStepUp` and moved
beside `StepUpSource` in `utils/stepUp.ts`, so the snapshot can name it
without importing from the component that owns the state.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MZdpUwhHf943FoprcLVzEm
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot's second pass accepted every code fix and re-raised one item:
useProgressToasts is the only extracted hook that returns nothing, which
contradicts #2128's "each hook returns one object" criterion, and the
disagreement was recorded nowhere a reader would find it.
Its ask was either a result object or an explicit exception. Taking the
second: the hook subscribes, shows toasts and unsubscribes, and the only
state it owns is toast-id bookkeeping the caller must not see, so `{}`
would advertise an API that is not there. The criterion exists to stop a
hook handing back a long list of loose values — useTabUiState's
twenty-two is the case it was written against, and that one does return
a bundled object.
Noted in the hook's own doc block (so it reads as a decision rather than
an oversight) and on #2128 itself, so the acceptance criteria and the
implementation agree.
Signed-off-by: cliffhall <cliff@futurescale.com>
Seven of the eight comments, plus the three suppressed ones (the same
double-cast class):
- Narrow the toast hooks to `InspectorClientEventSource`, the client's
typed event-listener surface. A bare `EventTarget` cannot satisfy it
(its listener takes an `Event`, the typed one a `TypedEventGeneric`
with `detail`), so both tests now construct a real
`InspectorClientEventTarget` and dispatch through `dispatchTypedEvent`
rather than casting a stand-in. That failed to compile immediately and
caught two bad fixtures: `task()` was a `Pick<Task, "status">` where
the event map wants a full `Task`, and `toolCallTaskUpdated` was
dispatched with no `task` at all.
- Narrow `useExportActions`'s three store parameters to the single
clearing method each handler calls, so the test doubles are
structurally checked instead of cast away.
- Build real `MessageEntry` fixtures (`timestamp` is a `Date`, not `0`),
removing four more double casts.
- Fix a tautological assertion in useTabUiState.test: it compared a
callback to itself, so it held however the identity changed. Every
callback is now captured before the update and compared to those
references.
- Reword `resetTabUiState`'s doc: `exhaustive-deps` cannot infer
stability across a hook boundary, so the dependency IS named — the
stability buys no churn, not omission.
- Justify the one remaining double cast in place, per the type-safety
rule.
`useProgressToasts` still returns `void` rather than an object,
deliberately: it has nothing to return, and `{}` would imply an API that
is not there. Noted on the PR as the one deviation from #2128's
"returns one object" criterion.
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase 1 of #2126. Six hooks move out of `clients/web/src/App.tsx`, taking
its component body from 4,891 to 4,267 lines with no behavior change:
- useThemeToggle — the light/dark toggle
- useTabUiState — the nine per-screen `ui` objects, the active tab,
the pinned-Protocol ids, and the session reset
- useInspectorStores — the twelve state managers as one create/destroy
lifecycle, their `core/react` hooks, and the
pagination-mode selection
- useExportActions — the eleven Clear / Export / Replay handlers
- useProgressToasts — `notifications/progress` toasts
- useTaskToasts — task-status toasts, the cancel confirmation, the
taskId -> progress map, and the in-flight
tool-call taskId ref
Holding the stores in one slot collapses `setupClientForServer`'s and
`onConfirmRemove`'s dependency arrays from twelve entries to two stable
callbacks; `destroyStores` reads the live set through a ref mirror rather
than the state it also writes.
All six hooks are at 100% on all four coverage dimensions, no `v8 ignore`
needed. Also refreshes the now-stale "~4.5k-line" description of App.tsx
in AGENTS.md and the coverage `include` comment.
Signed-off-by: cliffhall <cliff@futurescale.com>
- Move REPLAYABLE_PROTOCOL_METHODS / isReplayableProtocolMethod out of
components/groups/protocolUtils.ts into utils/replayableProtocolMethods.ts.
It is a pure predicate over a string set, so by the project's own lib-vs-utils
rule it never belonged under components/ — and keeping it there was what made
lib/protocolReplay.ts import 'up' into the component graph. Both consumers now
import from utils/; the predicate keeps its own test.
- Give replayProtocolRequest an exported ReplayClient = Pick<InspectorClient, ...>
naming the nine methods it calls, so a caller (and the test) can satisfy the
real contract instead of casting a stub into a whole client.
- Rebuild the protocolReplay fixtures as plain typed MessageEntry values. The
double cast had been hiding a real defect: MessageEntry.timestamp is a Date
and the fixture passed a number, so the assertion confirmed a shape
messagesToLogEntries cannot produce.
- Replace the window casts in authToken.test.ts with Reflect.set /
Reflect.deleteProperty. No 'as unknown as' remains in either test.
- Drop the stale 'floats top-right' sentence on ReAuthBannerBar, which moved
verbatim and arrived contradicting the paragraph below it.
Declined: re-homing ReAuthBannerBar's transform/zIndex as a theme variant.
'styles' is the Styles API rather than an inline style, Mantine exposes neither
property as a prop, and a styling change is out of scope for a phase whose
safety argument is that nothing was edited.
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase 0 of #2126. Lines 239-668 of clients/web/src/App.tsx were already at
module scope — helpers, formatters, constants, and the toast subcomponents,
none of which close over component state — so the whole block moves verbatim
into files. App.tsx drops from 5,303 to 4,892 lines with no behavior change.
Placement follows the lib-vs-utils rule in AGENTS.md: pure computation to
utils/, anything touching the environment to lib/.
EMPTY_SETTINGS and the two loose toast ids deliberately do NOT land in a new
top-level src/constants.ts, which the issue suggested: the web coverage
include is a whitelist naming components/hooks/theme/lib/utils/server, so a
module there would have fallen out of the >=90 gate silently. They go under
utils/ instead.
ReAuthBannerBar is in the block but not in the issue's table — it was added
after the issue was written (#2108). It moves to components/groups/ReAuthBanner/,
beside the banner it wraps.
Every new module is at 100% on all four coverage dimensions. Two v8 ignores,
each justified in place: getAuthToken's typeof-window guard (a happy-dom
inherent path) and replayProtocolRequest's provably-dead default case.
Signed-off-by: cliffhall <cliff@futurescale.com>
Making every pinned field read-only left an optional one unreachable: the
schema is equally happy with or without it, but the form displayed a value the
user could neither send nor edit. An optional `const` is a yes/no rather than
a fixed answer, so it renders as its single choice, clearable — opting in sends
the schema's own typed value, opting out leaves the property absent. A required
one is still displayed read-only, since there is nothing to decide.
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>
A schema may require a property it never declares: `{ type: "object",
required: ["token"] }` is legal and the tool plainly takes an argument.
declaresAnyFields counted only `properties`, so an App tool shaped that way —
including nested under a composition — was reported input-free and
auto-invoked with `{}`.
Signed-off-by: cliffhall <cliff@futurescale.com>
- branchesKey is built from the resolved alternatives themselves, not from
labels and field names: two branches can share both while pinning different
discriminators or typing a field differently, and a reorder of those went
unnoticed — the form then displayed one branch and submitted another.
- InspectorView gains a deep-link test with root-union appArgs: they name the
second branch, and the assertions pin the picker, the read-only
discriminator, THAT branch's default (not the first branch's), and the
absence of the other branch's field. The old shallow spread fails it.
Signed-off-by: cliffhall <cliff@futurescale.com>
The cross-branch agreement check ignored `const`, so two branches pinned to
`1` and `"1"` both stated no `type`, both matched the text `1`, and
the first one's typed constant was sent. Textually indistinguishable constants
of different types are ambiguous, and the raw string the user typed is the
honest answer.
Signed-off-by: cliffhall <cliff@futurescale.com>
