`ServerImportJsonModal` > "guards against a live edit made before the
debounce re-validates" opened its window on the real clock: it pasted
invalid JSON and clicked Add Server, relying on less than
VALIDATE_DEBOUNCE_MS (300ms) of wall time elapsing in between. When more
did, the debounce landed first, `canAdd` went false, and the click hit a
disabled button — a no-op that sets no submit error, so the
`findByText(/Fix the validation errors/)` timed out. The first assertion
(`onAddServer` not called) still passed, which is why the failure read as
mysterious rather than as a disabled button.
Run the test on fake timers end to end instead. The first validation is
landed explicitly with `advanceTimersByTimeAsync(VALIDATE_DEBOUNCE_MS)`
— imported from the hook, so the test cannot drift from the value it
depends on — and after the second paste the timers are simply not
advanced. The pending re-validation therefore cannot land at all, the
window stays open by construction, and the final assertion becomes a
synchronous `getByText`.
Verified with a throwaway probe holding both shapes side by side with a
deterministic 400ms stall injected between the paste and the click: the
old shape fails exactly as reported, the new one passes.
No timeout was widened and no production code changed.
Closes#2250
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQgwZ1kGzhkdkMJ81JVg42
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2244Closes#2225
All three outstanding advisories were stale lockfile resolutions, not
upward-blocked pins: every fixed version already sits inside the range its
declaring parent asks for, so refreshing the lock entry is the whole fix.
fast-uri 3.1.5 -> 3.1.7 ajv@8.18.0 asks for ^3.0.1 (root)
qs 6.15.3 -> 6.16.0 express@5.2.1 asks for ^6.14.0 (root)
browserslist 4.28.2 -> 4.28.9 via @babel/core (clients/tui)
No `overrides` entry is added, and no manifest changes. AGENTS.md's rule is
that a transitive is pinned with `overrides` rather than with `npm audit fix`
-- it does not call for a pin where none is needed. A permanent pin here would
buy nothing and would later hold a package back: `fast-uri: ^3.1.6` forbids
fast-uri 4.x for as long as it stands, including after ajv moves to it. The
monthly refresh sweep and the daily alert sweep are what catch a regression.
`npm audit` is clean in all five installs (root, web, cli, tui, launcher).
Reachability, assessed rather than assumed:
- fast-uri is the one that ships. ajv is a root runtime dependency, and Vite
pre-bundles it into the published `clients/web/dist`, so the vulnerable code
was inlined into the SPA rather than merely resolved at install time. Its
input is attacker-influenced: `schemaUtils.ts` compiles the `outputSchema`
a server under test supplies, and ajv resolves that schema's `$id`/`$ref`
through fast-uri (`ajv/dist/runtime/uri.js`). Impact is bounded well below
the advisory headlines, though: both SSRF advisories need a consumer that
fetches the parsed URI, and ajv never performs a network request. The
realistic worst case is the host-confusion pair mis-normalizing a crafted
`$id`, giving a wrong or failed validation of one tool's output.
- qs is installed in production, but nothing shipped runs it. express is a root
devDependency, but that is not its only path: `npm ls express --omit=dev`
shows it reaching a production install through
@modelcontextprotocol/server-legacy@2.0.0 (a root runtime dependency, also
via express-rate-limit) and through @modelcontextprotocol/ext-apps ->
@modelcontextprotocol/sdk@1.30.0. So qs is present in every user install.
What holds is that no shipped module ever instantiates it: nothing in
`core/`, `clients/*/src` or `clients/web/server` calls `express()`, the web
backend is Hono, and server-legacy is imported only from `test-servers/src`,
which the root `files` list does not publish. Both advisories need express to
parse an attacker-supplied query string, which requires a live express app.
- browserslist is dev tooling. It arrives in the clients/tui install through
eslint-plugin-react-hooks -> @babel/core, is reached only by lint, and is in
no published bundle. The root install already resolved a patched 4.28.8 from
the same plugin at the same version, which is what identified tui's copy as a
stale lock rather than a constrained one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YahVxMTGpigLbZBh1JGPDr
Signed-off-by: cliffhall <cliff@futurescale.com>
#2222 reported that `core/auth/revocation.ts` breaks a client secret
containing `+`, on the basis that `encodeURIComponent` leaves `+` bare.
It does not — it escapes it as `%2B`. The characters it leaves bare are
exactly `!'()*-._~` and alphanumerics, every one of which a form-
urldecoder passes through unchanged, so its output decodes identically
under both algorithms; verified over every code point up to U+2FFF.
Switching to `URLSearchParams` would be the literal algorithm §2.3.1
names and a small regression: it encodes a space as `+`, which a lenient
server decoding with `decodeURIComponent` alone reads as a literal `+`,
while `%20` is understood by both. The encoder is therefore unchanged
and the reasoning is recorded on it.
The report's second half stands and is the real defect: the fixture
decoded with `decodeURIComponent`, the encoder's own inverse, so the
round trip succeeded for every input by construction and no test could
have failed on an encoding mistake. `/oauth/revoke` now runs a genuine
RFC 6749 Appendix B decode, with unit cases for `+`, a space, `:` and
`%` (asserted on the wire and through an independent decoder), an
end-to-end client whose secret is base64 with a `+` in it, and a guard
that an unencoded credential is refused — the case that separates the
two decoders.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BhVaufrEZAfFAXcg6456hK
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot: the catalog entry is mutable and the live client is not. A card
can be edited while connected — `onConfigSubmit` writes the catalog and
does not rebuild the client — so after A connects to X and is edited to
Y, the entry reads Y while the session is still authorized against X.
Both consumers read the entry, so both got it wrong in the same way:
- the hook compared a cleared entry against A's *entry* URL, so clearing
another entry still at X missed the match and deleted and revoked the
live client's X-keyed credentials without disconnecting it — the exact
failure this PR exists to prevent;
- `runClear` locked on the entry's key, so a clear of edited-A (`url:Y`)
and a clear of entry B at X (`url:X`) took different locks while both
performed the same X-keyed operation through the same live client.
`InspectorClient.getTransportConfig()` returns the config it was
constructed with, which is what its credentials are keyed under, so both
now resolve from that and fall back to the catalog entry only when there
is no client.
The rule moves into one exported function, `resolveOAuthClearIdentity`.
Two copies of it that disagree is precisely the class of bug #2217 is,
and the two consumers each re-deriving it is how this round's finding
came to exist at all.
Both fakes gain a `getTransportConfig`, and three tests cover the split:
the resolver's own two (the client's URL wins; an active clear locks on
the client's key) and a hook case asserting an edited active entry still
disconnects when the entry sharing the client's real URL is cleared.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot, and the repo's own rule against an unjustified `as unknown as`.
The `useServers` cast was the more useful catch: removing it surfaced
that the mock omitted `reorderServers` and `importSource`, so App code
calling either would have read `undefined` with this test still passing
type-check. The full result shape is supplied instead, matching the
`mockServersWith` helper above.
The client cast becomes an intersection — the instances are already typed
`EventTarget`, so naming the test-only spy on top of that expresses what
is needed without erasing the rest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
Refreshes are concurrent — an `oauthComplete` can start a second read while
the first is still in flight — and nothing makes them settle in order. The
new catch made that visible: a slow earlier read could reject *after* a
newer one succeeded and clear the fresh result. Guard both handlers with a
monotonically increasing sequence so only the newest read writes.
The two tests were also too weak to detect their own guards, as the review
pointed out. Both started from an undefined panel, so neither could tell a
correct handler from one that merely swallowed the error:
- The rejection test now seeds a successful read first, then rejects an
`oauthComplete` refresh and asserts the already-loaded details clear.
- The post-cleanup test now reconnects with a replacement client that
populates details, then rejects the stale read, and asserts the current
details survive.
- A third test covers the ordering guard directly.
Each of the three guards has exactly one test that fails without it,
verified by mutation: dropping the sequence check, dropping the clear, and
dropping the `cancelled` check each break one test and no others.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot: `oauthClearKey`'s unit tests prove the derivation, but a
regression that dropped the `has` check, added or deleted the wrong key,
or invoked the clear twice would still have passed.
Adds an App.test.tsx case that connects entry A, holds its clear open on
a deferred `clearOAuthTokens`, then drives a clear for entry B — a
different catalog id against the same URL — and asserts the underlying
clear ran exactly once, and once more after the first settles, so the
key is proven released rather than leaking a permanently dead control.
Verified to fail against the id-keyed guard.
Two supporting changes it needs:
- an `open-settings-b` control on the mocked InspectorView, the only way
to reach a settings modal for a server other than the active one;
- `@inspector/core/mcp/remote/index.js` becomes a partial mock.
`getWebProxiedFetch` reaches for `createRemoteFetch` there, and the
bare stub threw a missing-export error before the clear under test ever
ran — the failure surfaced as the generic "Could not clear the stored
OAuth state" toast.
The case carries an explicit 20s timeout and `delay: null`: it drives
three full modal interaction sequences against the whole App tree, which
runs past the 5s default when the suite is under load.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
Clearing the retry from the mirror effect raced: an async command continuation
can open a prompt and install its retry before a previous commit's passive
effect flushes, and that effect's stale `pendingStepUp === null` snapshot would
then delete the new prompt's retry.
Install it in `trySetPendingStepUp` instead, in the same synchronous step that
opens the prompt — `null` for an ambient prompt, the operation for a
command-scoped one. Every prompt comes through there and `handleStepUpAuthorize`
is the only reader, so a slot cleared as a side effect can leave a stale closure
behind harmlessly: the next prompt overwrites it. A refused prompt still
installs nothing (#2165).
Copilot round 3 on #2237.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot: `runClear`'s double-click guard deduped by `server.id`, which is
the same identity mismatch this PR fixes, one layer up. Two catalog
entries against one URL share one credential blob, one grant and one
revocation, so an id-keyed guard let exactly the race it exists to
prevent through between them — concurrent store writes, concurrent RFC
7009 requests, and two contradictory teardown toasts.
The guard now keys on `oauthClearKey`, a pure helper that resolves the
OAuth storage key and falls back to the entry id for a config that has
none (stdio), where there is no shared state to collide over. The id
fallback keeps such entries distinct from each other rather than
collapsing them onto one key, and both forms are prefixed so a
user-supplied id can never impersonate a URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot: the stale-session guard deliberately skips `disconnect()` after a
switch, and `disconnect()` can also reject (which gets its own toast), so
claiming the active session "was disconnected too" is not always true.
What is always true is that its stored tokens went with the shared blob
and it must reconnect, so the message says that instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
The retry closure belongs to the prompt, but the two paths that clear the
step-up slot as a *consequence* of something else — the server-switch reset
and `resetOAuthRecoveryState` on disconnect — left `pendingStepUpRetryRef`
installed. The next prompt to open without a retry of its own would inherit
it, so authorizing an ambient step-up on server B ran the command server A was
left mid-way through.
Clear it in the existing mirror effect whenever the slot is null, so no arm has
to remember. A ref write in an effect, since the render-phase reset must stay
pure.
Copilot on #2237.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd
Signed-off-by: cliffhall <cliff@futurescale.com>
`useOAuthRecovery` cleared its two server-scoped pending-OAuth slots — the
deferred re-auth and the step-up prompt — in a `useEffect` keyed on
`activeServerId`. An effect runs after the commit, so a server switch painted
one frame still carrying the previous server's re-auth banner or step-up
prompt: an authorization affordance attached to the wrong server.
Reset both during render with `useValueChange` instead. `activeServerId` is a
primitive, so it satisfies the helper's `Object.is` stability requirement, and
the previous value is read from the `setState` updater argument rather than
from `sessionRef.current`, keeping the render-phase callback pure.
`react-hooks/set-state-in-effect` did not fire on the original because the
effect read the previous value through a ref rather than from a prop or state.
The test records one entry per committed render and asserts both slots are
empty in the first frame after the switch — the pre-existing `waitFor`-based
tests pass against the effect version too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd
Signed-off-by: cliffhall <cliff@futurescale.com>
The connection-details refresh effect in useOAuthRecovery voided
inspectorClient.getOAuthState() with a lone .then and no rejection
handler. In the browser that read goes through the remote OAuth store, so
it is a network round trip that can reject — a backend that is down or
restarting, a 401 on the API token, malformed stored state. void silences
no-floating-promises without terminating anything, so every such failure
became an unhandled rejection, on the initial refresh and on every
oauthComplete refresh.
Terminate the chain with a .catch that clears the panel details rather
than leaving the last successful read on screen: the panel reports the
current OAuth state, and a stale answer is indistinguishable from a fresh
one. The cancelled guard is repeated on the catch so a rejection arriving
after unmount does not write. The void stays, now with the one-line
justification AGENTS.md asks for — a synchronous useEffect body cannot
await.
Two tests cover it. Both fail against the unfixed source with unhandled
rejections, which is the failure mode itself: an unhandled rejection
fails the whole vitest run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBkPzbuxgyqiz39ytmQzMB
Signed-off-by: cliffhall <cliff@futurescale.com>
OAuth state is keyed by server URL, but `clearServerOAuthAndDisconnect`
decided "is this the active connection" from the catalog entry id. Those
are different identities and nothing keeps them in sync: `serverList`
enforces no URL uniqueness, so two entries with distinct ids against one
URL are a supported state — and a natural one, since separate entries are
how a user keeps different names, headers or per-server settings against
the same server.
Clearing the inactive entry therefore deleted the shared URL-keyed blob —
the active session's tokens, DCR client id and PKCE state — and, with
#2144 in, revoked its grant at the authorization server, while the id
check took the inactive branch: no live-client clear, no disconnect, no
session cleanup. The active session was left connected on credentials
that no longer existed, with nothing told to the user; the break then
surfaced somewhere else entirely, on the next refresh, 401 or reload.
The branch now compares the OAuth storage key of the cleared entry
against the active entry's, and treats a match as affecting the active
session — live-client clear, disconnect, session cleanup — exactly as
clearing the active entry does. The existing id and client-identity
checks stay on top: they guard the separate stale-session race from
#2144, and the snapshotted id they revalidate is now the active one
rather than the cleared entry's, which for a shared-key clear are not the
same. The toast says why the session went away.
The alternative, prohibiting duplicate URLs in `serverList`, would remove
a legitimate workflow to fix a bug in the consumer and could not repair
catalogs that already hold duplicates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4
Signed-off-by: cliffhall <cliff@futurescale.com>
Two suppressed comments, both accurate.
- Pin zod, zod/v4 and open to the root in vitest.shared.mts, and add matching
paths to the three clients/web tsconfigs. The claim they replace — that these
resolve unambiguously by walk-up — was wrong: both still sit at the top level
of clients/web/node_modules as transitive copies (zod under
eslint-plugin-react-hooks, open under Storybook), so an unpinned bare import
from a web test resolved the client copy while core/ and the SDK packages
resolved the root's. For open that is only wasteful; for zod it is a schema
built by one copy and an instanceof check made by the other across the whole
@modelcontextprotocol/* surface. zod is deduped as well, and zod/v4 gets its
own entry because first-party code imports both specifiers and pinning one
would split the package rather than collapse it.
- Update the consumer comments that #2195 falsified. clients/web/vite.config.ts
(three places) and server/start-vite-dev-server.ts all still said the shared
aliases resolve pino/hono and friends from clients/web/node_modules; they now
state the split — react and react-dom from the client install, everything
root-declared from the repo root.
- local-dev skill: replace the wrong 'resolves unambiguously' list with the
distinction that actually matters. Deleting a declaration is not the same as
the package resolving from the root, because chokidar, zod, open and react
all sit in a client install transitively; check the install, not the manifest.
ajv, commander and undici carry no pin because no client install has a copy.
npm run local:gate passes from a cold tsc build.
Signed-off-by: cliffhall <cliff@futurescale.com>
- clients/web/tsconfig.{app,node,test}.json: repoint the pino, hono,
hono/streaming, @hono/node-server, atomically, chokidar and @napi-rs/keyring
path mappings from ./node_modules to ../../node_modules. The stale mappings
were not failing the build — an unresolvable paths entry falls back to
ordinary walk-up resolution, which finds the root copy — but chokidar is the
case where the fallback never runs: Vite's transitive copy still sits under
clients/web, so tsc was type-checking against the client's copy while Vitest
and the shipped bundle both resolved the root's. Same version today, silent
skew tomorrow. Both path prefixes now carry a comment saying which ownership
each one asserts.
- local-dev skill: the consolidation rationale no longer groups commander, open
and @hono/node-server (client-imported) with the core/ set, no longer claims
#2195 moved undici (it was already root-only), and no longer implies every
root-declared package has an alias.
npm run local:gate passes with clients/web/node_modules/.tmp cleared, so the
tsc results are from a cold build rather than an incremental cache.
Signed-off-by: cliffhall <cliff@futurescale.com>
- Complete the bundler external lists against the AGENTS.md invariant rather
than against today's reachability: chokidar, hono and react to clients/cli,
chokidar and hono to clients/tui, react to the web runner. Every root-declared
package core/ imports at runtime is now named in all three, so a new import in
core/ cannot silently inline one.
- AGENTS.md: split the single over-broad bullet in two. commander, open,
@hono/node-server, vite and @vitejs/plugin-react are root dependencies reached
only from client code, so they are not part of the core/ set and need naming
only in the external list of the client that imports them. Give the
vitest.shared.mts pins their own rule, stating the react/react-dom exception
instead of implying every root-owned package is aliased to the root.
- vitest.shared.mts: the umbrella comment now says root-OWNED rather than root
runtime dependency — express is test-only and lives in the root
devDependencies — and notes that the list is deliberately not the complete
root set, since a package that resolves unambiguously needs no pin.
- local-dev skill: document why react/react-dom stay client-pinned, which the
previous text contradicted.
npm run local:gate and npm run verify:bundle-externals pass.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
@vitest/browser-playwright declares an EXACT peer on vitest ("vitest":
"4.1.10"), so that package — not the root's range — decides which vitest lands in
clients/web. Dropping web's own vitest declaration left the root free to resolve
4.1.11 while web's peer stayed at 4.1.10, so web's tests ran on one vitest while
loading a @vitest/coverage-v8 provider built against another. Both still passed,
which is the bad part.
vitest and @vitest/coverage-v8 at the root and @vitest/browser-playwright in
clients/web are now pinned exactly to 4.1.10 and move together, the same
discipline as the exact prettier pin (#1790).
The docs also claimed more than the change delivers. npm auto-installs an unmet
peer into the install that needs it, and a client install cannot see the root's
tree — so web's eslint-plugin-react-refresh / -storybook and the TUI's
eslint-plugin-react-hooks each pull a client-local eslint, and web's
Storybook/Vitest stack pulls a local typescript and vitest; @types/express
hoists an @types/node into web and cli. Those copies sit nearer than the root's
and win. AGENTS.md and the local-dev skill now say so, and state what the
consolidation actually buys: one declaration and one place to bump, with
verify:dep-lockstep as the guard that the copies agree.
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>
Declare the toolchain every client uses once, at the repo root, and delete the
per-client copies: eslint, @eslint/js, typescript-eslint, globals, prettier and
typescript (all five manifests), plus vitest, @vitest/coverage-v8 and
@types/node (four). Adds the latter three to the root manifest; the first six
were already there.
Every client reaches the root copy by walk-up — npm run puts each ancestor
node_modules/.bin on PATH, and Node and TypeScript resolve modules and @types
the same way — so nothing about the client scripts changes. clients/launcher now
declares no devDependencies at all and its validate is unchanged.
The copies had already drifted: globals sat at ^17.7.0 at the root against
^17.4.0 in all four clients, and typescript-eslint at ^8.65.0 against ^8.56.1.
Consolidating takes the root's version in both cases, and makes the exact
prettier pin (#1790) a single pin rather than five that can diverge.
Tools only one client runs stay where they are — tsup, tsx, vite, vite-node,
playwright, storybook, happy-dom, ink-testing-library, and each client's own
@types/*.
Documents the rule in AGENTS.md's Dependency placement section and the reasoning
in the local-dev skill.
Signed-off-by: cliffhall <cliff@futurescale.com>
Brings in #2192 (react-hooks lint over `core/react/`) and #2194. All the code,
`eslint.config.js` and the new root devDependency merged cleanly; three
conflicts, all in files this PR refactored.
- `.github/copilot-instructions.md` — modify/delete again. Kept deleted; the
line v2/main added there restates AGENTS.md rules that arrive in AGENTS.md.
- `AGENTS.md` — two of the three incoming hunks merged cleanly, both of them
RULES, into the sections that survived the refactor: `exhaustive-deps` is
`error` in every React scope including `core/react/**`, and
`set-state-in-effect` now covers `core/react/` with `clients/tui` called out
as the exception. The third was a `validate:core` bullet inside the pre-push
gate section this PR moved wholesale to docs/quality-gate.md, so its mechanism
went there instead — including why the plugin has to be a ROOT devDependency
(Node resolution walks up, not down, so a client-local copy is unreachable).
- `README.md` — the incoming paragraph was added to the quality-gate section
this README no longer carries. Moved verbatim to docs/quality-gate.md, beside
the type-aware-lint paragraph it belongs with.
Ran `npm install` for the new devDependency; `npm run validate` passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Seeding the settings draft during render made `useSettingsDraft` call
`resolveInitial` there, and App.tsx passes `lastPersistedSettings.resolve`.
That resolver was neither pure nor current:
- it could `recordsRef.current.delete(...)`, a shared-state mutation that a
render React replays under StrictMode or abandons would run an
unpredictable number of times;
- it read `serversRef`, which is mirrored in a passive effect and so trails
`servers` by one flush — so on exactly the render where a refreshed list
arrives, it answered from the previous commit's list. That render is the
one that seeds the draft.
`resolve` now reads `servers` directly and only reads. Eviction moves to an
effect: nothing depended on it happening at a particular moment, it only
keeps a dead record from being re-checked on every later write. Reading
`servers` also makes the supersession test fire a render earlier, which is
the right direction — the record is being checked against a list that has
genuinely been re-read.
`resolve`'s identity now changes with the list, so the test harness
delegates to the current hook result instead of handing back a snapshot from
the first render; a snapshot would have closed over the initial list and
made `setServers` look like a no-op. New test asserts `resolve` answers from
the list of the render it is called in, verified to fail against the ref
read.
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>
- `useEmaIdpLoginState` attributed a read to the wrong issuer. A bare
`EmaIdpLoginState` carries the previous issuer's answer over to a new one
until a fresh read lands — and if that read fails, indefinitely, so the
settings surface reports "logged in" for an IdP nobody authenticated
against. Round 1's `.catch()` made that worse by design, and the test
added with it asserted the wrong behaviour outright.
State is now the pair (issuer, value) and `loginState` derives from it, so
a mismatch reports "none". A monotonic read token — the device
`useInitialConfig` already uses — drops completions that were overtaken,
including a read still in flight when `logout` clears the session; issuer
keying cannot catch those, since both carry the same issuer. Both guards
are pinned by tests verified to fail without them.
Deriving the value also removed the last synchronous commit on that path,
so the `set-state-in-effect` suppression is gone — one fewer than the PR
started with.
- The draft-seeding change had no test that could tell it from the effect
it replaced: the regression is a committed frame, and RTL flushes passive
effects before any assertion on `result.current`. Both hooks now have a
commit-level trace recorded from inside the tree, asserting the previous
target's (or session's) draft is never committed for the new one. Both
were verified to fail against the effect implementation.
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>
- `useEmaIdpLoginState`: the effect-triggered `refresh()` was discarded with
`void`, but it does not own its failures — `getEmaIdpLoginState` awaits
`storage.getIdpSession`, which rejects when the backend is unreachable.
Terminate with `.catch()` and leave `loginState` untouched, as `logout`
already does. Covered by a new test that also asserts no unhandled
rejection escapes.
- Correct the history: the plugin was *client-local*, declared by both
`clients/web` and `clients/tui`. The root could reach neither, since Node
resolution walks up rather than down — that, not web-exclusivity, is why
`core/react/` went unlinted.
- Correct the enforcement claim: `clients/tui` registers the plugin but
deliberately enables only `rules-of-hooks` and `exhaustive-deps`, so
`set-state-in-effect` and `refs` hold for `clients/web` and `core/react/`
only. Says so in AGENTS.md and in the Copilot mirror, which would
otherwise have reviewers cite rules the TUI gate does not run.
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 conflicts, all in the files this PR refactored. The code from #1955 and
#2161 (core/react's useSyncExternalStore conversion) merged cleanly.
- `.github/copilot-instructions.md` — modify/delete. This PR deletes it, and
the four lines v2/main added there are a distillation of AGENTS.md rules that
arrive in AGENTS.md anyway. Kept deleted.
- `AGENTS.md` — the incoming change expanded the `core/react/` entry in the
project tree by eleven lines; this PR compacted that tree to one line per
entry on purpose. Took the compact tree and carried the substance onto its
one line. The other incoming hunk — the store-subscription rules under React
instructions — merged cleanly, which is the split working: it is a RULE, so
it lands in the section that stayed.
- `README.md` — same shape, one tree line. This README no longer enumerates
`core/`'s subdirectories at all, so the substance went where that content now
lives: `docs/architecture.md` and the `project-structure` skill's core table.
Nothing from v2/main was dropped; every incoming statement is now in exactly one
place rather than the three it used to occupy.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 6. `handleCommandScopedAuthRecovery` wrote
`pendingStepUpRetryRef` before calling `trySetPendingStepUp`, which
REFUSES a second prompt while one is already open — so the refused
command's operation replaced the open prompt's, and authorizing that
prompt ran the command the user had just been told could not start.
The retry now goes in only on a prompt that actually opened, which makes
the pairing hold in both directions: the prompt and its retry are
installed together, and the EMA branch takes them out together.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 5. `pendingStepUpRetryRef` is shared, and the EMA branch
dismisses the prompt *before* its awaits — so a newer command can open
its own step-up and install its own retry while the first authorization
is in flight. Every arm that read or cleared the ref afterwards was
acting on that newer operation: running it under the wrong
authorization and reporting its failure to the wrong source, or
deleting it outright.
The attempt now takes its retry out of the ref before any await, and no
arm touches the ref again. That covers all three sites at once and
removed the interactive-arm clear added last round — the value is
already out by then. The standard step-up path redirects and clears
synchronously, so it has no such window.
Also rewrites the `writeOAuthResumeSnapshot` docstring, which still
described the byte-comparison token it no longer returns.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
"stops listening on unmount" asserted that the rendered value stopped
changing after unmount, which proves nothing: React drops a store
notification for an unmounted component whether or not the listener is
still attached, so the test passed with the leak it existed to catch.
Spy the store's add/removeEventListener and assert the exact function
attached is the one detached. Verified it discriminates by deleting the
cleanup, watching this test fail, and restoring.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 4:
- the EMA `interactive` arm kept `pendingStepUpRetryRef`, on a comment
claiming it was "still on its way to a redirect". That is wrong twice:
`prepareOAuthRedirect` returns before the navigation is resolved, so a
pre-navigation failure leaves the operation for a later, unrelated
step-up to re-run; and the full-page navigation destroys the closure on
the success path anyway. It now clears the ref, matching the standard
step-up below.
- the two surviving `void (async () => …)()` sites now state why the
caller cannot await, as AGENTS.md requires — the comments had described
only what the catch handles.
- the `randomUUID` fallback test restored an own descriptor that never
existed (it is inherited from `Crypto.prototype`), leaving an own
`randomUUID: undefined` that would push every later test in the file
onto the fallback path. It deletes the temporary property instead, and
asserts the global is a function again afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 3, four items:
- the snapshot "attempt token" was the stored serialization, which is not
unique: the snapshot records neither the authorization URL nor its
`state`, so two concurrent redirects for one server with unchanged UI
serialize identically. `OAuthResumeSnapshot` now carries an optional
`attemptId`, stamped per write and matched on clear. Optional because
an older build's snapshot, mid-redirect across an upgrade, has none —
and treating that as a match would strand a live callback.
- the resume-failure toast promised a retry even when the session had
ended and nothing was restored. Copy is now chosen by whether anything
is left pending; `authRecoveryAbandonedMessage` covers the other case.
- a connect-scoped redirect that failed left the connection at
"connecting" with the active-server lock held, since the redirect was
what would have ended the attempt. It now disconnects first, as the
sibling arm in `useConnectionLifecycle` does — scoped to that context,
because an ambient or step-up failure does not mean the session is dead.
- both terminal EMA step-up failure arms now drop the stored retry. A
later step-up does not overwrite the ref, so a retained operation could
be re-run by an unrelated authorization. The `interactive` arm keeps
it: that one is still on its way to a redirect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
The cache was keyed on the store and its revision alone, so a changed
`read` was answered with the previous reader's output, and with a null
store — where the revision is pinned at 0 — a changed `whenAbsent` was
ignored outright. Key on `event`, `read` and `whenAbsent` too.
This makes an unstable argument defeat the cache rather than silently
serve a value derived from the previous one, so React fails it with "The
result of getSnapshot should be cached" at first render. That is the
intended failure mode and is now documented as such: a loud error beats a
wrong list nobody can account for.
Also correct the docs, which described the helper as universal. That
misread useListError as a violation: its snapshot is the stored Error
instance, already referentially stable, so it subscribes with
useSyncExternalStore directly and needs no cache. The rule is
read-during-render; the helper is for getters that return a fresh value
per read.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2, both findings valid:
- `resumePendingReauth`'s restore could resurrect a challenge whose
session ended during the awaits — a server switch or disconnect clears
the slot on purpose. It is now gated on the attempt still targeting
the live session, client identity included (reconnecting to the same
server builds a replacement client an id-only check would admit), and
uses a functional update so a newer deferral queued in the same batch
wins rather than being read past.
- `prepareOAuthRedirect` cleared whichever resume snapshot was current,
not the one it wrote. Nothing single-flights that callback, so a
redirect failing before navigation could delete a later attempt's
callback-routing state. `writeOAuthResumeSnapshot` now returns the
serialization it stored, and `clearOwnOAuthResumeSnapshot` removes it
only while it is still the current one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
Every state-subscription hook in core/react/ seeded local React state from
its store prop and re-synced it inside a useEffect. That shape paints one
frame of the previous store's data when the prop changes (switching
servers), and loses any event dispatched between the render that took the
snapshot and the effect that attached the listener.
Convert all 16 to useSyncExternalStore through a new shared
useStoreSnapshot, following useManagedListError (#1954).
useSyncExternalStore needs a referentially stable snapshot, and these
getters hand back a defensive copy, so TypedEventTarget now keeps a
per-event-type dispatch counter (getEventRevision) that the snapshot is
cached against. A contents comparison would not do: MessageLogState folds
a response into its request entry by mutating that entry in place, which a
shallow compare reports as unchanged. The bump lives on the dispatchEvent
override so no dispatch — including a hand-rolled CustomEvent — can bypass
it.
useInspectorClient is included though the issue did not list it; its nine
subscribed slices made the stale frame most visible. Its lastError stays
useState (accumulated from events, no getter to read back) but resets on
client swap during render rather than in an effect.
The settings-draft and async fetch hooks are deliberately untouched: the
former hold genuinely local editable state, the latter are I/O rather than
store subscriptions.
Also answers the issue's open question. react-hooks/set-state-in-effect
never fired here because eslint-plugin-react-hooks has never linted
core/react/ at all — it is a clients/web devDependency and the root config
loads no React plugin. Corrected the config comment that gave "no JSX" as
the reason; the gap itself is #2192, measured at 15 findings, none of them
in the hooks converted here.
Closes#1955
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all valid:
- `beginInteractiveAuthorization` navigated *before* awaiting provider
creation and flow-state recording, so a rejection could arrive after
the browser had been handed over — and the web catch clears the resume
snapshot on one. Fixed at the root: the navigation now resolves last,
after every fallible step, which makes every rejection the method can
produce a pre-navigation rejection. It is also better on its own
terms — a provider that cannot be built means the callback could not
have completed, so navigating first only bought a dead-end round trip.
- `resumePendingReauth` restored the pending slot unconditionally, which
could overwrite a newer challenge deferred while it was awaiting.
Restores only into an empty slot, so latest wins.
- the `authChallengeInteractive` catch re-read the active server rather
than the one the challenge arrived for. `serverId` is now captured
synchronously in the listener and used by both the body and the catch.
Three new tests, one per finding, each driving the interleaving rather
than the happy path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
#2153 dropped App.tsx below the React Compiler's bail-out, so the
compiler-powered rules in eslint-plugin-react-hooks v6 began reporting
nine latent findings. Each was suppressed with a directive pointing at
this issue; the later phases moved them into useConnectionLifecycle and
useMcpApps. Eight remained. Five are now fixed and three re-justified on
their own terms, so no suppression points at an open issue any more.
Fixed:
- `connectedServerId` was a useState mirrored from `connectionStatus` and
`activeServerId` by an effect. It is a pure function of both, so it is
derived. A state copy could only lag the status it copies by one
committed render, which is a frame of the wrong highlight for no gain.
- The two latest-ref writes (`sandboxUrlRef`, `listedResourcesRef`) move
from render into a layout effect, the same fix review applied to
`setupClientForServerRef` in #2153. Both refs are read by an event
handler or an async handler that runs after the commit, so neither
reader observes a different value.
- `appElicitationController` was a ref seeded during render with `??=`.
The instance is never reassigned, so `useState`'s lazy initializer
gives the same create-once semantics without a render-time ref read —
which also retires the two directives on the `useSyncExternalStore`
arguments it fed.
Kept, with the argument in place of the pointer:
- The deep-link `onToggleConnection` call. It opens a transport and
drives a handshake; the state it sets along the way is a consequence of
that I/O, not something derivable from props. An effect is the right
tool, and the one-shot `deepLinkConnectRef` guard is what bounds it.
- The two `createAppBridgeFactory` calls. `react-hooks/refs` cannot see
through the factory and warns that a ref-reading dep it is handed
(`getListedResourceMeta`) may be called during render. It is not: the
factory only stores its deps, and the sole call site is inside the
async `sandboxready` handler.
No behavior change beyond `connectedServerId` landing in the same
committed render as the status it derives from, rather than the next one.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
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>
Each of the five sites in `useOAuthRecovery` discarded a promise with
`void`, satisfying `@typescript-eslint/no-floating-promises` but not the
narrower rule AGENTS.md states — `void` only where the callee owns its
failures or the caller cannot await, with a comment saying which.
Each now picks a user-visible outcome:
- `prepareOAuthRedirect` clears the resume snapshot (so the next load
cannot read a redirect that never started as an *abandoned* one),
flags the server and raises the re-auth banner.
- the `authChallengeInteractive` handler flags and banners, where before
a rejection drew no UI response at all.
- `resumePendingReauth` restores the pending slot and toasts, so the
next trigger retries the deferred recovery instead of losing it.
- the `/oauth/callback` effect flags and banners what falls between its
existing arms — the client rebuild and the post-resume scope check.
- `handleStepUpAuthorize`'s EMA branch reports exactly as its `failed`
outcome arm does; the stored retry gets its own catch, since by then
the permissions leg succeeded and the failure is the command's.
The two surviving `void`s carry the justification AGENTS.md asks for.
Closes#2165
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
- skill-manifest: the closing `---` fence must occupy a whole line. `---oops`
was accepted as a terminator, silently truncating the frontmatter and passing
validation — the exact failure the guard exists to catch. Regression cases
added (prefix, trailing whitespace, EOF, CRLF).
- skill-eval: reject a nonzero `claude -p` exit instead of resolving it as an
empty observation. An auth error or rate limit was scoring as "no skill
invoked", which passes every negative case and reads as a trigger miss on
every positive one.
- skill-eval: extract `collectSkillInvocations` / `sampleHit` behind a `main()`
guard and cover them in `skill-eval.test.mjs` — the stream parsing and the
process-outcome handling are unreachable from an eval run's happy path.
- docs/test-servers: annotate the era on every catalogue row. Six legacy rows
carried no annotation under a "unless noted, Modern" default, so the doc sent
readers to the wrong era for the feature they were trying to see.
- docs/quality-gate: `validate` runs four guards, not three; add the
`verify:skills` row and the two new `test:scripts` suites; fix the `#publishing`
fragment left dangling by the split.
- issue-create / pr-flow: delegate the card steps to `/board-ops` rather than
restating its option IDs, which are explicitly unstable.
- issue-triage: the audit counted presence, not cardinality — an issue carrying
both `v1` and `v2` passed a check that claims "exactly one". Same for the five
type labels. Also scope the two milestone checks to board #28.
- AGENTS.md: codify the v1 milestone exception. Every milestone is a v2 release
bucket, so a v1 issue has none to take and the Incoming-milestone invariant
cannot apply to board #11.
- clients/launcher/README: its pointer promised an explanation of why the cli/tui
manifests ship, which the extracted guide does not make.
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>
