Addresses Copilot's round-4 review on #2035 — again reported as "no new
comments" while carrying three suppressed ones. All three reproduced.
1. The TUI form still named its fields from the SDK. `uriTemplateToForm`
read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields
";id" and `{id:3}` yields "id:3". The form therefore submitted
`{ ";id": "7" }` while the shared expander looked up `id`, found
nothing, and dropped the expression — the value vanished silently.
This is the finding that mattered most, because it falsified the claim
this change is built on. Moving the expander into core only makes the
clients agree if each client's FORM derives its names from the same
parser: a form submits under the names it rendered. The TUI now reads
`templateVariables`, and the SDK template is constructed only to
validate, preserving the existing malformed-template diagnostic.
Its `required` flag comes from `requiredGroups`, and only a variable
that is the sole member of a non-omittable expression is marked: ink-
form cannot express "any one of these", so marking every member of
`{a,b}` required would refuse input the expander accepts.
2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the
sub-delims !'()* bare, which RFC 6570 requires encoded for every
operator except + and #.
Fixing that settled a design question left open in the previous round.
With two encoders, the SAME value encoded differently depending on
whether its expression happened to carry a modifier, since only the
modifier pushed it onto our path. So delegation is gone entirely: one
expander, one set of rules. The SDK's `UriTemplate` is still
constructed, but only to validate a template.
3. The "Any one of: a, b" hint could contradict the disabled submit
button. A name can sit in a singleton required group AND a shared one
(`x://{a}/{a,b}`), where the singleton demands that exact field. The
hint is suppressed in that case.
Signed-off-by: cliffhall <cliff@futurescale.com>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds two optional per-server fields — `oauth.authorizationUrl` and
`oauth.tokenUrl` — that override the endpoints authorization-server
metadata discovery resolved, so a server can be pointed at a development
or staging authorization server without changing what it advertises.
SDK v2 routes both endpoints through the discovered metadata document and
neither through OAuthClientProvider, so the override is applied by
wrapping the client's base fetch and patching the metadata document in
flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather
than only the auth fetch — also covers the discovery the SDK runs from
inside the transport on the 401/refresh path.
The fields round-trip through mcp.json, the /api/servers routes, the
runner (CLI/TUI), and the Server Settings → Authorization panel, where a
value that is not an absolute http(s) URL is flagged inline; at runtime
such a value is dropped with a single warning rather than failing the
connection.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-3 review on #2035. It again reported "no new
comments" while carrying two suppressed ones; both reproduced.
1. Requiredness could not live on a variable at all. Deduplication kept the
first occurrence's group, so in `x{?a}{?b}{a,b}` both names ended up
required with singleton groups and the form demanded both -- while the
SDK expands that template with only `a` to "x?a=11". Widening the stored
group would not have been enough either: in `{a,b}{a,c}`, filling `b` and
`c` satisfies both expressions, which no per-variable flag can express.
So requiredness is now returned per expression by `requiredGroups`, and
`hasRequiredValues` asks that each group be satisfied by any one of its
names. `TemplateVariable.groupNames` is gone rather than left as a field
that quietly means something narrower than it reads; `required` remains,
documented as driving the "Optional" marker and nothing else.
2. `encodeURI` is not the allow-reserved encoder `+` and `#` call for. It
escapes `[` and `]`, which are reserved and must survive, and it escapes
`%`, so an already-encoded value is double-encoded. Measured:
"[::1]" -> "%5B::1%5D" and "%2F" -> "%252F". Both corrupt the URI rather
than merely over-escaping it -- an IPv6 literal or a pre-encoded path
reaches the server altered, which is the same class of defect #1919 is
about.
Added an RFC 6570 3.2.1 encoder that preserves reserved characters and
existing pct-triplets, splitting on `%XX` so a lone `%` is still encoded
to `%25`, and matching with the `u` flag so an astral character is
encoded whole. `+` and `#` expressions are now taken over even for a
single name, so both expansion paths agree on what those operators mean.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-2 review on #2035. It reported "no new comments"
but carried three *suppressed* ones; all three reproduced against the
pinned SDK.
1. The `;` path-parameter operator is absent from the SDK's operator list,
so `{;id}` parsed as a variable literally named ";id" and expanded to
nothing. Added the operator and its named expansion (`;a=1;b=2`).
2. An RFC 6570 prefix modifier was folded into the variable name: `{id:3}`
yielded a variable called "id:3" and expanded to nothing. Varspecs are
now parsed properly and the value truncated before encoding. Truncation
is by code point, since `String.prototype.slice` counts UTF-16 units and
would split an astral character into a lone surrogate.
For both of these the wrong URI is the lesser problem: a form has to
*name* the variables it asks the user to fill, so the panel was
rendering fields labelled `;id` and `id:3` that nobody could use.
3. Requiredness was applied per variable, but it is a property of the
*expression*: RFC 6570 drops undefined names from a multi-name
expression, so `{a,b}` with only `a` filled expands to `a`'s value --
the SDK does this too. The panel was refusing input the expander would
have accepted. `hasRequiredValues` now encodes "any one name in a
required group suffices", and such a field reads "Any one of: a, b"
rather than falsely claiming each is mandatory.
Two structural consequences, each pinned by a test:
- Takeover is now per TEMPLATE rather than per expression. Splicing
corrected fragments into a template the SDK re-expands would leave its
cross-expression `?`-to-`&` rewrite blind to the fragments already
resolved.
- Expansion is split into a strict variant that throws and a lenient one
that returns the raw template. `readResourceFromTemplate` wraps the
thrown error with the template name -- three pre-existing integration
tests assert that -- while the form must not throw on a server-supplied
template, since that would take out the panel on render. The strict
variant constructs the SDK template unconditionally, because that
construction is what validates syntax: otherwise `x://{;a}{b,c` would
take the own-expansion path and its unclosed tail would pass as literal
text with nothing objecting.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's review on #2035. All three findings reproduced against
the pinned SDK before acting.
1. `#` was misclassified as required. Measured: `x://a{#frag}` with no
`frag` expands to exactly `x://a`, a well-formed URI naming a real
resource -- unlike `{+path}` (`x://a/`) or a simple `{userId}`
(`file:///users//profile`), which leave an empty path segment. Moved `#`
into the omittable set; the required cases now assert what the URI
*becomes* when blank, so the rule is checked rather than asserted.
2. Delegating to the SDK did not actually give RFC 6570 expansion.
`UriTemplate.expandPart` takes an early `names.length > 1` branch that
raw-joins values, skipping both `encodeValue` and the operator prefix:
`x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` -- the very
unencoded-slash defect this PR is about -- and `x://a{/p,q}` to
`x://ax y,z`. Only `?`/`&` are correct, being dispatched earlier.
Fixing that in the web client alone would have left the TUI and CLI
wrong, since `readResourceFromTemplate` expands through the same class.
So parse/classify/expand now live in `core/mcp/uriTemplate.ts` and both
call sites use it. The correction is surgical: a multi-name non-query
expression is expanded here and spliced in as literal text before the
SDK sees the template (safe -- both encoders escape `{`/`}`), while
every single-name and query expression still goes through the SDK
untouched. The preview applies the same correction, so it cannot
promise a URI that submitting would not send.
3. The showcase promised a blank `{?topic}` read that did not work.
`UriTemplate.match()` compiles `{?topic}` to a *required*
`\?topic=([^&]+)`, so `match("foobar://events")` returns null and the
read 404s. A real server exposes the unfiltered collection as its own
resource; the showcase now registers `foobar://events` so the
documented step resolves. Verified end to end through the CLI.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from the Copilot review of #1965, both real.
1. The candidates were flattened to package names before the version
comparison, so a third install holding a different copy failed the guard
even when no program loads that copy beside another — and the diagnostic
named an install that never took part. `crossInstallPackages` now returns
the co-occurrences themselves (name → program → install → entry paths) and
`findSkew` compares per program, across only the installs that met there.
2. Folding a nested copy onto its outermost install lost the path that
identifies which copy was loaded, and the version was then read from the
install's top-level entry — absent or differently versioned, either way
letting a real pair pass. `classifyModulePath` now also returns the
`entryPath` npm keys the lockfile by, `topLevelLockVersions` becomes
`lockVersionsByPath`, and each holder is priced from the entry the program
resolved. A resolved copy with no lockfile entry is now a hard failure
rather than a silently dropped holder.
The failure output prints `in <program>` with each holder's install path
beside its version, so a nested copy is distinguishable from a top-level one.
Signed-off-by: cliffhall <cliff@futurescale.com>
`verify:dep-lockstep` derived its candidate set from the packages the shared
first-party sources named *directly*, so a package whose declarations reach a
program only through another package's `.d.ts` was invisible to it — it could
skew across installs and recreate the two-copy blow-up #1896 was about while
the guard stayed green. `@modelcontextprotocol/sdk` was the live example: never
written in first-party code, yet 16 of its `.d.ts` files land in clients/web's
test program.
Replace the derivation with the measured one: list every client tsconfig
project with `tsc --listFilesOnly`, map each resolved `node_modules` file to
its owning install and package, and keep the packages that reach ONE program
from TWO installs. That is exactly the set that can put two structurally
distinct copies of a type in front of one checker. The rest of the guard is
unchanged — top-level lockfile comparison, deny-by-default, `TOLERATED_SKEW` as
a name allowlist scoped to within-a-major.
The listing machinery moves to `scripts/lib/tsc-program.mjs`, shared with
`verify:typecheck-coverage`, so the two guards can't disagree about what a
program contains. It is deliberately not cached to disk between the two
processes: a stale fingerprint would make a guard measure a program that no
longer exists and pass on a real miss. The second pass costs ~14s.
`TOLERATED_SKEW` is now empty. `jose` and `@modelcontextprotocol/ext-apps` are
root-only since #1970 and cannot skew; `react` (whose types are `@types/react`,
resolved from one install) and `hono` never reach one program from two
installs. Each would be a rationale for a skew that cannot occur.
Closes#1965
Signed-off-by: cliffhall <cliff@futurescale.com>
Real deployments gate IdP selection, prompt behavior, or audience on a query
parameter the OAuth/OIDC core specs never standardized (Keycloak's
`kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
`audience`), and none of them could be driven from the Inspector.
The Inspector never builds the authorize URL — the SDK's `auth()` does, and it
exposes no hook for extra parameters — so the merge happens at the one seam
that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`.
It lives in `core/`, so web, CLI, and TUI all inherit it.
- `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`,
`code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`,
`response_type`, `scope`, `state`), the key validator the form renders
inline, and the merge itself. Reserved keys are dropped with a warning
rather than silently overriding PKCE / the state binding / RFC 8707;
blank-key rows are skipped; the caller's URL is never mutated.
- Applied to the authorization request only — nothing wires it into the token
request.
- Persisted as `oauth.authorizationParams` (a string record) on the per-server
config beside `scopes`, edited as key/value rows in Server Settings ->
Authorization ("Additional authorization parameters"), with an inline error
per offending row plus a section-level warning.
- The enterprise-managed (EMA) leg redirects to a *different* authorization
server (the enterprise IdP), so it goes through the explicit
`redirectToExternalAuthorization` entry point, which skips the merge — a
`kc_idp_hint` or `audience` meant for the MCP server's AS must never be
appended to the IdP's authorize URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2015, folded into #1928 rather than left as a follow-up.
The TUI's `schemaToForm` has the same defect the web `SchemaForm` had: its
local `JsonSchemaProperty` declares only `type?: string` — no `anyOf`, no
array-valued `type` — so a `.nullish()` argument has no top-level `type` or
`enum` for the dispatcher to read and falls to the `switch` default, losing
its select. Milder than the web bug (a plain text field, not a corrupting
JSON textarea), but the same class.
Fixing it in a second place is the argument for not having the logic in a
first place. So the collapse moves to `core/json/nullableUnion.ts` as
`normalizeNullableUnion`, generic over a minimal `NullableUnionSchema`
shape — the least a caller must expose to be *recognized* as a nullable
union — so neither client has to adopt the other's schema type. Both form
builders now share one copy and cannot drift on which schemas they render.
- `core/json/nullableUnion.ts` — the shared collapse, plus tests at
`clients/web/src/test/core/nullableUnion.test.ts` (the web suite is what
gates `core/`).
- `clients/web`: `schemaUtils.normalizeUnionType` is gone; `SchemaForm` and
`resolveRefsInMessage` call the core function directly.
- `clients/tui`: `schemaToForm` normalizes each property before dispatch,
and `JsonSchemaProperty` grows the `anyOf` / array-`type` fields the
encoding uses.
Verified by driving the built TUI under a pty against the
`nullable-fields-http.json` showcase server. Before, every argument of
`record_shipment` rendered as a plain text field; after, `direction` is a
select ("No value"), `quantity` an integer, `express` a boolean
("[Not set]").
Signed-off-by: cliffhall <cliff@futurescale.com>
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review:
- `<x.y.z>` / `<major|minor|patch>` inside a bash block is shell redirection,
so both documented commands were syntax errors if copied. Verified: `bash -n`
rejects `git tag <x.y.z> && ...` and accepts `git tag 2.3.0 && ...`. Use a
concrete 2.2.0 -> 2.3.0 example and say what to substitute.
- "agree on the version at every point" / "never disagree" overstated it. The
procedure deliberately bumps v2/main first, so between the bump and the
milestone merge the branches differ — v2/main reads the version being built,
main the one released. Describe the real invariant: they converge when the
release merge lands, and v2/main is never left behind main.
Signed-off-by: cliffhall <cliff@futurescale.com>
v2/main has been at 2.0.0 since the v2.0.0 release while main and npm `latest`
are at 2.2.0. The bump was created on the milestone-merge branch, which is cut
from main, so it only ever existed downstream of v2/main and nothing carried it
back — `chore: 2.1.0` and `chore: 2.2.0` are both on main and neither is on
v2/main.
That is not cosmetic. A branch cut from a milestone-merge branch silently
carries the bump into an unrelated PR — #2009 arrived with a 2.0.0 -> 2.2.0 diff
on a container bugfix — and anything reading the version in development
(readInspectorVersion, --version, GET /api/config) reported a version two
releases old.
Move the bump to v2/main, before the milestone merge, so it flows into main with
the rest of the milestone's work and the branches never disagree. The release
tag is applied to the resulting main commit instead. The publish workflow needs
no change: its tag-vs-package.json assertion runs at the release commit on main,
which still carries the bump.
Deliberately not a back-merge of main into v2/main: main holds the whole pre-v2
v1 history (~230 commits v2/main lacks, retained through `chore: replace main's
tree with v2`), so merging would graft all of it into the develop branch's log
permanently to deliver a two-file change.
Also sets v2/main to 2.2.0 as a one-time catch-up. Nothing in the repo asserts a
specific version value, so this is a safe two-file change.
Closes#2010
Signed-off-by: cliffhall <cliff@futurescale.com>
Two defaults that each break the Inspector inside a container.
#1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted
to the NAME `localhost`, and `server.listen(port, host)` resolves a name
through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17
the default result order is `verbatim`, so on a glibc Linux host — whose
/etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The
server bound IPv6 loopback only and refused every IPv4 client, including the
dev container port forwarder. It presents as an endless spinner rather than a
connection error because the forwarder accepts on the host side before failing
to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit
HOST=localhost is still honored as typed.
#2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox
listened on an OS-assigned port, so it changed every run and could never be
named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default
it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an
OS-assigned port (loudly) if it is taken — the collision a dynamic default
never had.
Verified in the reporter's exact image
(mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners
come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox.
Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both
defaults in the web README, the migration guide, and the root README's Docker
section.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review caught a second-order effect of the fix: making the leaked-timer net
global invalidated the rationale of a neighbouring rule.
"Do not hand-roll a bare MantineProvider — that reintroduces the leak class"
stopped being true the moment the net moved into setup.ts, which covers every
unit test regardless of how it renders. The rule itself is still right, so keep
it and replace the justification: a hand-rolled provider skips the project theme
and the helper's options and drifts from every other test. Both copies say
explicitly that the old reason no longer holds, so nobody re-derives it.
Mirrored into .github/copilot-instructions.md in the same PR, as AGENTS.md
requires for review-relevant guidance — it carried the identical stale claim.
That makes four places this one belief had propagated to. A fix can create stale
docs by making a previously-true statement false, which is easy to miss when
only the changed lines get read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Review follow-up on #1983. Copilot was right, and about a hazard this repo
had already proven and then documented away: the claim that ink's ">=19"
peer "keeps npm from hoisting it next to a React the Inspector couldn't
also use" is false. A consumer pinning React 19.0 satisfies ">=19" while
`^19.2.4` nests underneath, so ink renders through their React and the
bundle through ours — the #1952 crash, one level up, breaking TUI startup
rather than just its forms.
Bundling ink closes it (verified: with a `createRequire` banner for the
inlined CJS, since react-reconciler calls `require("react")` and
signal-exit@3 `require("assert")`), but costs ~1.4MB — react-reconciler
and yoga-layout come along. The root `react` range closes it for free
instead: `^19.0.0` is open to the whole major, so npm can satisfy our
React and a consumer's pinned React 19 with one copy, and an external ink
resolves the same copy the bundle does.
Verified against both consumer trees — the reporter's (React 18 + ink 5)
and a pinned React 19.0.0 + ink 6.8.0, which splits under `^19.2.4`.
That makes the range load-bearing while looking like an ordinary version
bump, so the tests pin it: the root range must equal ink's own declared
peer floor. They also assert each exempt package is both external and a
root dependency — external means consumers install it, the mirror image
of the inlined ones, which must not be root-declared.
The docs in all four places now say ink is exempt on cost and never on a
peer range, and name that claim as one we got wrong.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
A window.setTimeout that outlives its test file fires after happy-dom disposes
that file's window; React's dispatchSetState then throws an uncaught
ReferenceError: window is not defined. Vitest attributes it to whichever file
was running, so a single leak fails the ENTIRE run with every test passing —
landing on an innocent file under a step named "Enforce per-file coverage gate."
Seen twice in CI, both on docs-only branches.
Not a Mantine bug, and not a badly-written test. Both hooks do clear on unmount
(useTransition's clearAllTimeouts, useLockScroll's effect cleanup). It is a rAF
race: clearAllTimeouts cancels the *pending* rAF, but the transition schedules
rAF -> rAF -> setTimeout, so if the inner callback is already in flight when the
unmount lands, cancelAnimationFrame is a no-op and that callback then schedules
a timer after cleanup has run. Nothing owns it. Needs a loaded machine, which is
why it only ever appeared in CI.
Track every timer and clear whatever is still outstanding after cleanup(). The
ordering is load-bearing: legitimate unmount cleanups get their turn first, so
only true leaks are dropped. Fake timers pass through untracked, which is
correct — a fake timer cannot outlive the environment.
Also corrects the claim, in AGENTS.md and in setup.ts's own comment, that
env="test" prevents this. It does not: env is read only by Transition.mjs at its
render branch, while useTransition runs before that check (hooks cannot be
conditional) and still schedules real timers. Measured — opening a <Modal>
through renderWithMantine schedules three 200ms timers. Believing that guarantee
is why this went unexamined.
The regression test is mutation-verified: with the net disabled it fails, with
it restored it passes. It deliberately does not assert that Mantine schedules no
timers — it does, and that is normal; the contract is that none survive.
Closes#1984
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 5 was right on all three counts. Read dcoapp/app/lib/dco.js this time
rather than the README, which is where the earlier errors came from:
const authors = [commit.author.name, commit.committer.name]
const emails = [commit.author.email, commit.committer.email]
if (isMerge || ...) continue // parents.length > 1
else if (author && author.type === 'Bot') continue
So, corrected:
- Scope. Merge commits and bot-authored commits are skipped, not gated. "Every
commit" overstated it and could prompt a needless history rewrite.
- Matching. The trailer may match the author OR the committer, not the author
alone. A cherry-picker's own signoff is accepted.
- Certification. The DCO expressly covers submitting work created by others
that you have the right to submit, so signing off a cherry-pick is legitimate.
My "never sign off someone else's commit" would have told a maintainer not to
do a correct thing. The real prohibition is narrower: never fabricate a
trailer in another person's name.
This also retracts the mechanical argument in bd6c1072 for dropping the hook —
it does satisfy the check, since you are the committer. The recommendation
against it stands on honest grounds now: the trailer is a certification, a hook
makes it for every commit automatically including work merely applied on
someone else's behalf, and it provably cannot tell the difference. A judgment
about deliberateness, not a claim that it breaks.
Mirror corrected to match.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 4 was right that the hook signs with the configured identity while the
DCO check requires a trailer matching the commit *author*, and that the two
diverge on cherry-pick and `git commit -c`.
I tried the suggested author-aware guard and it does not work: inside
prepare-commit-msg, `git var GIT_AUTHOR_IDENT` returns the config identity, not
the preserved author. Verified end to end — cherry-picking Bob's commit as
Alice yields author=Bob, committer=Alice, `Signed-off-by: Alice`. That commit
both still fails the gate and certifies work Alice did not write, which is the
one thing the last bullet of this section forbids.
Three review rounds have now found three holes in this hook, each fix opening
the next. That is a wrong mechanism rather than an unfinished one, so the hook
is out. A subtly-broken hook in the file that governs how this repo is worked
on is worse than no hook.
The bullet now recommends `git commit -s` and names BOTH false automations —
`format.signOff` and the hook — recording the cherry-pick finding so the next
person doesn't rediscover it, and pointing anyone who wants automation at a
layer that actually knows the author. Mirror updated to match.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
`ink-form` and `ink-scroll-view` were externalized by the TUI bundler and
both declare a loose `react` peer (">=18"). Installed under a project that
has its own React 18, npm hoists them to that project's root — where the
peer is satisfied — while the Inspector's React 19 nests underneath. The
bundle then renders through React 19 while those two call hooks on React
18, whose dispatcher is null, so opening a tool test form (or any scroll
view) dies with "Cannot read properties of null (reading 'useState')".
Inline them instead: their `import "react"` is emitted into the bundle and
so resolves from the build directory exactly like the bundle's own, with
npm's placement no longer part of the decision. This also pins their
transitive deps to what this repo resolved — notably `ink-select-input@6`
via `overrides`, which npm ignores for a package installed as a dependency,
so consumers were getting the React-18-era v5.
`ink` stays external: it can't be bundled (its CJS `signal-exit@3` fails
ESM interop with `Dynamic require of "assert"`) and doesn't need to be —
its ">=19" peer keeps npm from hoisting it next to an unusable React.
Both packages are dropped from the root `dependencies`, since the tarball
now ships their code rather than having consumers install them.
`clients/tui/__tests__/tsupConfig.test.ts` is the durable guard: the
failure is invisible in this repo and in every smoke (a dev install has
one React) and only appears once the package is installed under another
project, so the invariant is asserted against the config instead — every
dependency declaring a `react` peer must be bundled unless exempted.
Closes#1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
Two claims were wrong, both verified against the app's own documentation
(dcoapp/app): "The DCO also creates an override button for users with write
access to the repository to create a successful check."
So "there is no per-PR override" and "there is no non-rewrite escape hatch"
were both false, and a maintainer following them would conclude that rewriting
history was the only way to unblock a PR.
Documented, but deliberately not as a repair. The override only silences the
check; it does not make the author certify anything, and the certification is
the whole point of a DCO signoff. Using it on your own unsigned commits asserts
nothing while looking like compliance — so the text says to treat it as
unavailable and sign the commits instead.
The same source confirms two things the doc already claimed: remediation
commits are disabled by default, and the trailer must match the commit author's
email (which is what the identity-aware hook in 6899e0ae exists for).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
The hook skipped insertion on any `Signed-off-by:` line, which is wrong for a
message reused from another commit — cherry-pick, `git commit -c` — since that
message already carries someone else's trailer. The DCO check requires one
matching the *author*, so the guard would be satisfied while the commit still
failed the gate: the exact silent-no-op failure mode this bullet warns about.
Match this identity's exact trailer instead. Verified: a plain commit signs
once; a message already carrying another author's signoff now gets the real
author's added alongside it; `git commit -s` is not double-signed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Both review points were factually right and my original text was wrong.
1. `git config format.signOff true` — which I had recommended as the *better*
option — only defaults the -s flag for `git format-patch`. `git commit` never
reads it, and there is no `commit.signoff` equivalent. Verified all three in a
throwaway repo: format.signOff → no trailer; commit.signoff → no effect; a
prepare-commit-msg hook → works. Shipping that advice would have been worse
than saying nothing, since it looks like a fix and silently changes nothing.
Now recommends `git commit -s`, gives the verified hook for automation, and
names format.signOff explicitly as a trap — the name invites exactly the
mistake I made, so it is worth warning about rather than omitting.
2. The DCO app's empty remediation-commit flow needs
`allowRemediationCommits.individual`. This repo ships no DCO config, so it
runs with that disabled and the original unsigned commits keep failing. The
escape hatch I offered does not exist here; rewriting is the only repair, so
the text now says to coordinate before rewriting shared history instead.
Mirror updated with both corrections.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
The DCO check fails any PR with an unsigned commit, and the rule appeared
nowhere in the repo — not AGENTS.md, CONTRIBUTING.md, README.md, or the Copilot
mirror. A hard merge gate that is invisible until it fails, whose remedy is a
history rewrite, is exactly what AGENTS.md exists to capture. It blocked #1978.
Added to the "When work is complete" list beside the other PR-hygiene gates,
weighted toward prevention over recitation: `git config format.signOff true`
once per clone beats remembering `-s`, because the failure is only discovered
after pushing. The repair path (`git rebase --signoff` + `--force-with-lease`)
carries the rebase caveat and the empty-remediation-commit alternative for when
you are not the sole author, plus a note that the signoff is a DCO assertion in
the author's own name and is never applied on someone else's behalf.
Mirrored into .github/copilot-instructions.md in the same PR, as AGENTS.md
requires for PR-hygiene rules — two lines next to `Closes #N` and the version
label, since the mirror is read on every review and is a distillation, not a
copy.
Not added to CONTRIBUTING.md: external contributors file issues rather than
PRs, so DCO never reaches them, and documenting it there would muddy that.
Closes#1979
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot was right that the previous test proved only that `removeSafe` can
delete a directory — it never drove `stop()`, so deleting the cleanup call
would have left the unit suite AND all three smokes green. That is exactly the
regression these tests exist to catch, and they did not catch it.
Extract the teardown into an exported `teardownWebServer({ child, catalogDir,
label })`, which `stop()` now delegates to, and drive it against a stand-in
child process (a real `node -e setInterval` that stays alive until signalled)
rather than a mock. Three cases:
- it removes the catalog dir it was given (asserted on the dir itself, not a
spy, so the assertion cannot drift from the behavior);
- it awaits the child's exit before removing — the #1801 race in one
assertion, since losing the await is otherwise invisible;
- an already-dead child is not an error, which is the failure path a smoke
takes when it calls fail() after the launcher has crashed.
Mutation-checked: with the `removeSafe` call deleted, 2 of the 3 now fail
(previously: 0). AGENTS.md updated to match.
Signed-off-by is now on every commit (#1978 DCO); the requirement itself is
undocumented and tracked in #1979.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses the two review comments that still applied to HEAD (the third, the
stop() race, was already fixed in a8022138).
- AGENTS.md still described the prod-web-server helper as shared by only
smoke:web and smoke:web:browser, and said nothing about catalog isolation.
It is the source of truth for these smoke mechanics, so record the invariant
there: the temp catalog per run and why, that only the catalog (not HOME) is
redirected, that teardown needs both halves of child-cleanup.mjs and is
therefore async for every caller, and where the contract is unit-tested.
- The `label` JSDoc claimed the option prefixes the temp dir. It does not —
that prefix is fixed at `smoke-web-catalog-` (deliberately, so every run's
dir is greppable); `label` only prefixes teardown warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Took v2/main's project-layout line and quality-gate table (adding
verify:dep-lockstep), keeping this branch's migration-guide mention and
prettier table alignment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
README: kept v2/main's showcase table including the new structured-output
row. AGENTS: kept v2/main's newer type-label and triage rules, rendered in
this branch's markdown formatting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review round 2, four findings, all factual errors in the docs added by
the previous commit:
- `yaml` is in the root `dependencies`, not `devDependencies`, and its only
importer is `test-servers/src/load-config.ts` — not `core/` or the root
tooling, as the `vitest.shared.mts` comment claimed.
- The rule as written told a reader to put anything reached through root-owned
code — `core/` included — in `devDependencies`. That is wrong and would break
the published package silently: the client builds externalize npm packages, so
a published install resolves them from the root manifest, where
devDependencies are absent. Placement (which manifest) and classification
(which section) are now stated as separate questions, the second following
from who consumes the package at runtime.
`yaml`'s runtime classification is left alone and called out instead: moving it
changes what ships, which is not a docs change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Per the maintenance rules, a change to the dependency layout has to land in the
docs with it. Adds the placement rule in three places, at three altitudes:
- README (Setup): the rule plus the concrete drift it prevents.
- AGENTS.md: a "Dependency placement" section — the MCP SDK packages are root
only, the v1 SDK is not ours and must not become a dependency, and anything
reached solely through root-owned code with no manifest (`test-servers/src`,
`core/`) is a root devDependency aliased to the repo root in
`vitest.shared.mts`.
- .github/copilot-instructions.md: the distilled, reviewer-citable form, so a
diff re-adding one of these to a client manifest is flagged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's ninth pass of #1962. The one-line descriptions in AGENTS.md and
the README claimed "no install-crossing dependency resolves to two different
versions", which is stronger than the gate: candidates cover only packages the
shared sources name directly, and #1965 records a live counterexample
(`@modelcontextprotocol/sdk`, skewed across two install roots while the guard
stays green).
The boundary was already documented at length on the guard and in the AGENTS.md
rule; the summaries just hadn't been narrowed to match, which is how a doc ends
up asserting a guarantee nobody implemented. Both now say "directly imported"
and point at #1965.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
All four suppressed comments from Copilot's third pass of #1962. Each was
correct.
The candidate boundary omitted `vitest.shared.mts` — root-owned, imported by
every client's vitest config, and already treated as shared non-client source
by `verify:typecheck-coverage`. It imports only Node built-ins today, which
is exactly why the omission would have gone unnoticed until a third-party
import appeared there, resolved from the root, and skewed. Shared *files* are
now a first-class list beside the shared dirs.
The allowlist was a blanket exemption by name. Each entry's rationale
establishes that a patch/minor difference is benign, which is no evidence
that a React 18-vs-19 or Hono 4-vs-5 split would be — that is a different
type surface. Being listed now tolerates skew only *within a major version*;
a cross-major skew fails like anything else. Deriving the major from the
lockfile version keeps this automatic, with no per-package range to maintain
and bump. A version that can't be parsed is treated as "cannot prove same
major" and fails rather than passes.
`verify-format-coverage.mjs`'s comment overstated the vouching graph: the
three guards do not each check the others. This one checks both siblings;
each sibling checks only this one. Described as it actually works.
The AGENTS.md rule said to bump a shared dependency in "root + all four
clients", but launcher declares no zod and `findSkew` ignores installs where
a package is absent — so the rule as written asked for spurious dependencies.
Reworded to every install that *declares* it, and mirrored in
`.github/copilot-instructions.md`.
The failure output follows: it no longer advises adding a package that is
already allowlisted, saying "MAJOR skew" instead, and it names all three
shared sources rather than the two dirs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Closes#1896
`clients/web`'s `tsc -b` exhausted the 4GB default heap at zod 4.4.x. The
cause is not a zod regression nor a pathological inference site of ours: it
is two copies of zod at *mismatched versions* inside one tsc program.
`clients/web/tsconfig.test.json` compiles `test-servers/src`, whose files
resolve zod (and the SDK's zod) from the **root** install, alongside web's
own sources, which resolve from `clients/web/node_modules`. At 4.3.6 vs
4.4.3 TypeScript must relate two structurally-distinct declarations of every
`@modelcontextprotocol/*` schema type, which is exponential over a recursive
generic surface — `TS2589 Type instantiation is excessively deep`, then OOM.
Bumping root zod to 4.4.3 and changing nothing else returns the build to
baseline: 11.4s and 2,567,882 instantiations, against 11.2s / 2,567,503 at
4.3.6. Two copies at the same version are harmless; skew is what explodes.
So drop the `~4.3.6` hold on `clients/web` and move all four manifests and
lockfiles to zod 4.4.3 together, and add `verify:dep-lockstep` to `validate`
to keep them there. The guard derives its candidate set from what `core/`
and `test-servers/src` import, compares the committed lockfiles' top-level
entries, and fails deny-by-default on any skew not in the annotated
`TOLERATED_SKEW` allowlist. It joins the existing mutual-vouch ring so
dropping it from `validate` is caught by `verify:format-coverage`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Closes#1822
Adds docs/v1-to-v2-migration.md — the map for users arriving from v1:
- CLI flag mapping (every v1 flag survives; lists what v2 adds)
- `--config` vs `--catalog` semantics with before/after examples
- Node engine bump (>=22.7.5 → >=22.19.0)
- what no longer ships (the three sub-packages; v2 is one tarball)
- the three behavior changes most likely to bite: exit code 5 on a
failing tool call, target-must-come-first under --cli, and the
reversed `--` separator
- env-var mapping (MCP_PROXY_AUTH_TOKEN → MCP_INSPECTOR_API_TOKEN,
MCP_PROXY_FULL_ADDRESS removed, SERVER_PORT repurposed), the v1 UI
config settings, web query params, and Docker
Linked from the root README (banner + docs list), the CLI README, and
docs/mcp-server-configuration.md; AGENTS.md's docs/ tree entry updated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
`smoke:web:browser` stops at first paint and never connects to a server, so
everything downstream of the connect — the Apps tab, the sandbox controller,
the UI-protocol bridge — was unexercised by any smoke. That is the code #1859
broke, and nothing would have caught a regression in it.
smoke:web:app boots the same prod --web server, spawns the mcp-app-http.json
composable server, and drives connect → open app → widget ready through one
deep-link navigate. The assertion is the documented data-app-status="ready"
contract, which the renderer only reports once the widget has loaded inside the
sandbox iframe AND completed its bridge handshake — so a single attribute
covers the proxy being served, the UI resource loading, and the handshake.
Two mechanics found by running it, both silent-failure shaped:
- server-composable.ts announces readiness on stderr, not stdout, so watching
stdout alone times out with an empty diagnostic. Both streams are scanned.
- the bound port is not the configured one — createTestServerHttp resolves via
findAvailablePort(), which walks upward when the port is taken. The smoke
parses the announced URL instead of assuming 3130.
Scope: this runs against the repo build tree like every other smoke, so it would
NOT have caught #1859 itself (a packaging failure; the file is always present
in-repo). pack:verify owns that dimension — the two are complements, and the
header comment says so. A cheap structural pre-check does assert the proxy page
sits where sandbox-controller.ts resolves it, so a move/rename fails fast.
Both failure modes verified by hand: with clients/web/static removed, the
pre-check fires; with the pre-check bypassed, the data-app-status assertion
catches it independently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
The version label says which line work belongs to; nothing said what KIND of
work it is. In practice that meant reaching for bug or enhancement and
pressing everything else into one of them — a migration guide and a
tsup->tsdown swap both landing as "enhancement", which degrades the label to
"not a bug" and makes filtering by it meaningless.
Every issue created or triaged now carries exactly one of bug, enhancement,
documentation, chore, or question, independent of its v1/v2 label. Adds a
table of what each is and is not for, and states the anti-pattern explicitly
so the two familiar labels don't absorb the other three. A PR needs no type
label — it is classified through the issue it closes, the same way it is
tracked through that issue's board card.
Wires the requirement into the three places that would otherwise drift: the
"not created until" callout, triage pass 1 (an outside reporter can set
neither label, so both are applied in the same step), and the board audit,
which gains an "open, no type label" check and a matching row in the
invariant table.
Note `chore` did not exist as a repo label and was created for this; the
existing `dependencies` label is Dependabot's and scoped to PRs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
A board card in Done asserts that the work shipped, and Done is read as the
record of what a milestone actually delivered. Nothing said so, so an issue
closed as a duplicate or not-planned was being moved to Done like any other
close — which makes that record wrong in a way no later reader can detect:
counting Done cards can no longer tell a shipped fix from a report closed as
a duplicate of one.
New rule: exactly two things earn a card a place in Done — its PR merged, or
it is a parent whose last sub-issue closed. Every other close (duplicate,
won't fix, not planned, obsolete, superseded) means nothing shipped, so the
card is deleted instead. Deleting a card touches only the board; the issue
keeps its labels and comments and stays searchable, so nothing is lost.
Also notes that the close reason is the machine-readable form of the same
distinction, and that `gh issue close --reason` cannot express `duplicate` —
it accepts only completed/not planned, so duplicate must be set through the
API or the web UI's "Mark as duplicate" (which also records a duplicate-of
link).
Adds the invariant to the board audit as a tenth check. That required the
audit to read closed issues too, so its fetch moves to --state all with
stateReason, `open(n)` becomes a state test rather than "present in the map",
and --limit must now clear the repo's total issue count, not just the open
ones. The two wrong-board label checks are scoped to open issues in the same
pass — over all history they flag four long-closed cards whose labels nobody
intends to rewrite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Follow-up on the same issue: the triage rules were clear on the happy path
but three gaps meant a bare "triage new issues" would diverge from what a
maintainer actually does.
Define triage by STATE, not authorship. An issue needs triage when it
arrives with no board card and no milestone — whoever filed it. Framing it
as "externally-filed" was wrong: a maintainer opening an issue by hand in
the GitHub UI lands in exactly the same state, because write access makes
the board reachable, not automatic. What puts an issue in Todo is somebody
performing the approval, not who owns the account. Renamed the section and
reworded every dependent passage accordingly.
Fix the discovery query, which diffed open issues against board #28 only.
Verified against the live boards: it reports #1929 as unboarded when that
issue is correctly carded on #11, so following it literally double-boards
the issue — recreating a defect a past sweep introduced. Now diffs against
the union of both boards, filtered by .content.repository (an org project
can hold other repos' issues; #11 carries one from modelcontextprotocol/
servers), and prints each hit's destination, since an unboarded issue that
already has a milestone is approved and belongs in Todo, not Incoming.
Add "The board audit" — the other drift classes a single-issue rule can't
catch (double-boarded, non-Issue items, statusless cards, the Incoming ⇔
milestone invariant in both directions, wrong board for label, missing
version label, missing Priority), as a table of invariant + fix plus one
runnable jq check that should print 0 across the board. Both snippets were
executed verbatim against the live boards.
Add "Recording the score". The rubric claimed its reasoning "survives in a
form someone can argue with later", but the board stores only the result
and is private, so nothing kept that promise. Triage now posts the axes,
the bonuses claimed, and the total as a comment.
Also scope pass 2 explicitly to a human — deciding what ships in which
release is not inferable from a rubric — and correct the stale note about
three statusless cards on #11.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
AGENTS.md had no rule linking a milestone to a board Status, and its two
existing rules contradicted each other: every new issue was to be milestoned
at create time, yet new items were to start in Incoming, with Todo reserved
for work a maintainer had approved. If every issue is milestoned at birth,
"has a milestone" carries no approval information and nothing records the
moment of sign-off.
Make assigning a milestone the approval act, giving one invariant:
Incoming <=> no milestone; everything past it <=> milestoned. External
reports and maintainer-filed issues then differ only in whether approval has
already happened -- an outside reporter cannot set a label, milestone, or
board field, so their issue arriving with none of them is normal rather than
a defect to fix on arrival.
- scope the "not created until five things" callout to maintainer-filed issues
- split the Status bullets into the two cases
- scope the milestone mandate to issues you create, stating the external
exception explicitly
- add a "Triaging externally-filed issues" section: the two passes, plus a
gh/jq snippet that diffs open issues against the board to find unboarded ones
- reword both prose descriptions of the Incoming/Todo line
- reword the rubric's "Linked to a milestone" bonus as "already approved" and
note it is the re-scoring case only; under the old rules it was a constant
offset on every issue, shifting every band boundary down by one
- update both gh recipes (#28 and #11) to default to Todo, with an Incoming
variant for triage
No mirror to .github/copilot-instructions.md: board and milestone mechanics
are on that file's deliberately-absent list.
Closes#1930
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1838)
eslint 9 pulled `minimatch@3` → `brace-expansion@1.1.16`, which carries
GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion length). It appeared in
all five lockfiles — the root plus each of the four clients, none of which is an
npm workspace, so all five had to move together.
Deferred from the pre-2.0.0 sweep because it is dev-scope, not attacker-
controlled, and a major bump across five packages was the wrong thing to attempt
immediately before an irreversible publish. This is that work.
## What moved
- `eslint` `^9.39.4/5` → `^10.8.0` and `@eslint/js` → `^10.0.1`, in all five
manifests.
- `eslint-plugin-react-hooks` `^7.0.1` → `^7.1.1` in web and tui. Required, not
cosmetic: 7.0.1's peer range stops at `^9.0.0`, so installing it alongside
eslint 10 fails `ERESOLVE`.
- Everything else already declared eslint 10 support and stayed put:
`typescript-eslint@^8.56.1` (`^8.57 || ^9 || ^10`),
`eslint-plugin-react-refresh@^0.5.2` (`^9 || ^10`),
`eslint-plugin-storybook@^10.2.19` (`>=8`).
## Config migration
None was needed. All five configs were already flat (`eslint.config.js` with
`defineConfig`/`globalIgnores` from `eslint/config`), which is the format v10
keeps and the only one it still supports. The root config's `core/` and shared-
surface blocks — including the `_`-prefix `argsIgnorePattern` /
`varsIgnorePattern` / `caughtErrorsIgnorePattern` — are unchanged and still gate
`lint:core` and `lint:shared`.
No rule was disabled, downgraded, or scoped away to make the new eslint pass.
v10 surfaced 21 real violations across three rules; all 21 are fixed in the code.
### `preserve-caught-error` (new in `eslint:recommended`) — 15 sites
A `throw new Error(msg)` inside a `catch` silently drops the original error.
Each now passes `{ cause }`, so the underlying failure survives into the chain.
Messages are unchanged, so nothing that asserts on them moves. 12 in `core/`,
2 in `clients/cli`, 1 in `test-servers/src`.
### `no-useless-assignment` (new in `eslint:recommended`) — 2 sites
`clients/cli/src/cli.ts`'s `optionArgs` and `test-servers/src/test-server-oauth`
's `valid` were initialized and then unconditionally overwritten on every path.
Both are now declared without the dead initializer; TypeScript's definite-
assignment analysis covers the remaining paths.
### `react-hooks/set-state-in-effect` — 8 sites (web)
These were false negatives the plugin fixed in 7.1.0, not a rule we newly opted
into: `set-state-in-effect` has been `error` in `flat/recommended` since 7.0.1,
and the web config has always extended it. Every one was the same anti-pattern —
resetting or re-syncing local state from a prop inside a `useEffect`, which
paints the stale value for one frame and then renders again to correct it.
All eight now use a new `useValueChange(value, onChange)` hook
(`clients/web/src/hooks/useValueChange.ts`), which is React's documented
"adjusting state during render": compare against the previous render's value
with `Object.is` and call back *during* render, so React discards the
in-progress output before it reaches the DOM.
- `MrtrConversation`, `ProtocolEntry`, `TaskCard` — mirror `isListExpanded`.
- `NetworkEntry` — same mirror, plus the "Reveal in Network" force-open. The
reveal's rAF scroll stays an effect (real external-system work); only the
`setIsExpanded(true)` moved. `isExpanded` is now seeded from
`isListExpanded || revealed`, since a render-time sync fires on change and so
cannot cover an entry that mounts already revealed.
- `PromptArgumentsForm`, `ResourceTemplatePanel` — reset on template/prompt
switch.
- `ServerConfigModal` — reset on open. Keyed on `opened ? initial : undefined`,
which collapses "opened flipped" and "initial changed while open" into one
value; `initial` is already memoized, so it cannot thrash.
`useValueChange` has its own test file, and `ServerConfigModal` gains an
open→edit→close→reopen test (the close leg was previously untested, which is
what left the new guard uncovered).
## Advisory delta
`npm audit`, per package, before → after:
| Package | before | after |
| ------------------- | -------------------------- | -------------------------- |
| root | 9 (1 low, 6 mod, 2 high) | 8 (1 low, 6 mod, 1 high) |
| `clients/web` | 19 (3 low, 6 mod, 6 high, 4 crit) | 17 (3 low, 6 mod, 4 high, 4 crit) |
| `clients/cli` | 6 (2 low, 4 high) | 4 (2 low, 2 high) |
| `clients/tui` | 4 (1 low, 3 high) | 2 (1 low, 1 high) |
| `clients/launcher` | 3 (3 high) | 1 (1 high) |
`brace-expansion` is gone from all five — `eslint@10` resolves `minimatch@10` →
`brace-expansion@5.0.9`. Dropping `@eslint/eslintrc` (removed in v10) also took
`js-yaml` with it, clearing GHSA-52cp-r559-cp3m (high) from all four clients as
a bonus. The remainder are unrelated and tracked separately.
`npm run ci` passes end to end — validate, the per-file ≥90 coverage gate, the
build gate, all five smokes (including `smoke:tui`, run locally on a real TTY),
and 462 Storybook tests.
Also documents the effect rule in AGENTS.md and mirrors it into
`.github/copilot-instructions.md`, since it is now something a reviewer cites.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: state useValueChange's referential-stability requirement (#1838)
Copilot review on #1900: the hook compares with `Object.is`, so an object or
array literal rebuilt in the component body compares unequal every render.
Because the callback is what updates state, that is not merely a wasted render —
it is an infinite loop.
Every current caller passes a primitive (`boolean`, `string`) or an already-
memoized value, so nothing changes behaviorally. But the hazard was implicit,
and the next caller is the one it would bite.
Documented on the hook itself, and one clause added to the AGENTS.md rule with
its mirror in `.github/copilot-instructions.md`. No runtime guard: the
requirement is the same one a `useEffect` dependency array carries, and here the
failure is loud and immediate rather than silent.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(cli): stop asserting the --tool-args-json catch binding is an Error (#1838)
Copilot review round 2 on #1900: the `--tool-args-json` parse handler formatted
its message with `(e as Error).message`. A `catch` binding is `unknown`, so that
cast is an assertion rather than a check — a thrown non-Error would render
`--tool-args-json is not valid JSON: undefined`, which points the user at
nothing.
`JSON.parse` does only throw `SyntaxError` today, so this is not a live bug. But
the cast is the kind AGENTS.md asks us not to leave unjustified, the same
handler was already being touched here to add `{ cause: e }`, and every other
site in this PR uses the guarded form. Now it does too.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: require useValueChange's onChange to be render-pure (#1838)
Copilot review round 3 on #1900: `onChange` is called during render, so it is
bound by render-phase purity — and the web client mounts under `StrictMode`,
which deliberately double-renders in development. A callback doing anything
external would run an unpredictable number of times, and concurrent React can
abandon an in-progress render entirely.
Every current caller already passes a callback that only calls `setState`, so
nothing changes. But "runs during render" is the whole point of the hook and the
constraint that follows from it was left implicit.
Stated on the hook, with the same clause in the AGENTS.md rule and its
`.github/copilot-instructions.md` mirror. All three now point at `NetworkEntry`
as the worked example of the split: the reveal's force-open is a state update
and lives in `useValueChange`, while its `requestAnimationFrame` scroll stays a
`useEffect`.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: replace the markdown bug template with issue forms (#1844)
Issues are the only intake channel now that external pull requests are off
(#1820), so the chooser is where the contribution policy has to be stated —
GitHub gives a would-be contributor no explanation when the "New pull request"
button isn't there.
The one existing template was a legacy **markdown** template, whose fields are
prose headings a reporter can delete or ignore. Most reports still arrive
missing the client and the transport, which are the two facts that decide where
to look. Issue forms enforce them: this replaces `bug_report.md` with
`1-bug_report.yml`, where client (web/cli/tui/core), version line (v1/v2),
Inspector version, Node version, transport, MCP server, repro, expected, and
actual are all `validations.required`. It auto-labels `bug`, and its intro says
maintainers implement the fix, so a prompt plus screenshots beats a diff —
with a field for exactly that.
Adds `2-feature_request.yml` (auto-labels `enhancement`), which puts the
problem statement first and marks the solution optional, since the problem is
what survives when a specific solution turns out not to fit.
Neither form applies a `v1`/`v2` label: GitHub can't map a form answer to a
label, so the version line is a required dropdown and a maintainer still labels
at triage. v1 takes security fixes only, so routing that answer early is the
point.
`config.yml` disables blank issues and carries the contact links. **There is
deliberately no security *template*** — a template still opens a public issue,
which is precisely what a vulnerability report must not do. The redirect is a
contact link straight to the private advisory form (`/security/advisories/new`,
verified enabled on this repo) plus one to `SECURITY.md`, which covers the v1
line too. The remaining links deflect the recurring misfiled classes: the
specification repo and the TypeScript SDK for reports that aren't about this
tool, the docs site, the contribution policy, and `#inspector-dev`.
Also fixes three links the #1884 rename left dangling — `SECURITY.md` and the
PR template still pointed at `CONTRIBUTORS.md`, which no longer exists on this
branch. A policy nobody can open is the same as no policy.
Validated against GitHub's issue-forms schema (unique ids, allowed `type`s,
`markdown` blocks carrying no `id`/`validations`, `checkboxes` marking
`required` per option rather than under `validations`).
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: address Copilot review on the issue forms (#1844)
- PR template said `v2` targets `main`; the v2 base branch is `v2/main`.
Correct on its face, and the one line in that template a reader acts on.
- Scoped the Node floor on the bug form to v2, since the form serves both
lines and v1's floor is older — reading `>= 22.19.0` as universal would
make a legitimate v1 report look out of support.
- Both the CONTRIBUTING.md paragraph and the AGENTS.md section now say the
chooser is served from the **default branch**, so neither claims a form
added on `v2/main` is live before the milestone merge.
* docs: re-wrap the CONTRIBUTING chooser paragraph (#1844)
Round-2 Copilot nit: the previous wrap left a dangling "The" at end of
line, splitting a phrase mid-sentence in the raw markdown.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: clarify the protocol-era prompt on the bug form (#1844)
Round-3 Copilot nit. "the protocol era (legacy / modern) if you selected
one" reads as referring to a field on this form, which has none — the era is
selected on the *connection*, in the Inspector's server settings. Reworded to
"the protocol era you connected with (auto / legacy / modern)", which also
picks up `auto`, the default the previous list omitted.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: stop describing the feature form as sharing the bug form's fields (#1844)
Round-4 Copilot nits, and it was right three times over — I had described
"client, version line, transport" as the required set for *both* forms in
CONTRIBUTING.md, AGENTS.md, and the config.yml header. The feature form has
neither a version-line dropdown nor a transport field, and cannot have the
first: v1 takes security fixes only, so a feature request is v2 by
construction.
Each of the three now distinguishes the two forms rather than generalizing
from the bug form.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat: declare v2 statically on the feature form (#1844)
Round-5 Copilot nit caught a real contradiction: the AGENTS.md paragraph said
no form can apply a version label, then asserted the feature form "is always
labeled v2". Both halves can't be true.
Resolved in the direction that makes the claim true rather than softer. A
form's `labels:` is static, which is only an obstacle when the value depends
on a reporter's answer — and for the feature form it doesn't: v1 takes security
fixes only, so a feature request is v2 by construction. So it now declares
`["enhancement", "v2"]` outright, which also satisfies AGENTS.md's
label-at-create-time rule without a triage step.
The bug form still can't, and shouldn't: its version line is genuinely the
reporter's to answer.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: add Incoming board status, adopt Priority field, document triage rubric (#1891)
Board #28 had no state for "filed but not yet triaged" — Todo means
maintainer-approved, so parking unreviewed issues there falsely asserted
sign-off. Adds an Incoming status ahead of Todo.
The board's Priority single-select existed but was unused (0 of 264 cards)
and undocumented, with P0/P1/P2 options matching no vocabulary in the repo.
Re-optioned to Urgent/High/Medium/Low and documented alongside Status.
Completes the priority rubric: fills in the score bands (12+/9-11/6-8/<=5),
expands both axes into scored tables, and fixes the truncated signal-indicator
line. Severity alone tops out at High by design.
Also drops the "V2 Go Live" status reference — that option is not in the
field's option list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: correct the Done option id and harden the deleted-column guidance (#1891)
The Done Status option was deleted from board #28, blanking the Status of
all 247 cards that held it. Restored from a pre-change item-list snapshot;
verified all 247 are Done again and no other card was disturbed.
A recreated option never regains its old id, so Done is now 259d6aab rather
than 248a3910 — updated in the option table and the merge step.
The hazard callout recommended the web UI as the safe path, which is what
made this look safe. It is not: deleting an option blanks its cards in the
UI exactly as it does via the API, with no undo. Corrects that, adds a
snapshot-first rule, and documents the recovery recipe used here — including
the grouping check that proves the orphaned set is exactly the deleted
option's cards before re-applying.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: add Incoming to the v1 board and document its recipes (#1891)
Board #11 had the same gap as #28 — no state for "filed but not yet
triaged" — so a v1 security-fix issue had nowhere to sit that didn't
assert maintainer approval. Adds Incoming there too.
AGENTS.md carried no v1 board recipes at all, only a link, so anyone
boarding a v1 issue had to discover the project and field ids by hand
and was liable to reach for #28's (which are rejected, but only after
the attempt). Adds a V1 board section with its ids, states that Incoming
is the default status for new items on both boards, and notes that
Priority is v2-only — #11 has no such field.
The Incoming option was added with all four existing option ids echoed
back, and a before/after item-list diff confirms all 74 cards kept their
Status. Three cards on #11 carry no Status; that predates this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: distinguish the board Priority from the org-level issue field (#1891)
An issue page shows two fields named "Priority" and nothing syncs them.
Ours is the project field on board #28. The other is a GitHub issue field
(IFSS_kgDOAdAWeg) defined at the modelcontextprotocol org and shared by
every repo in it, alongside Effort/Start date/Target date. Same name, same
four option names, unrelated storage — #1891 sat at Urgent in Fields and
High on the board simultaneously.
Adds a table naming both, states that no pass-through exists in either
direction, and says to leave the org-level one alone: it is not repo-scoped,
so deleting it would strip Priority from every other org repo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: mark the org Priority field untrusted and cap it at a +1 signal (#1891)
Both boards are private (public: false, verified), so maintainer-assigned
Status and Priority are invisible to reporters — board priority is a
working queue, not a published commitment.
The org-level Fields -> Priority is the opposite: public on the issue page
and not part of triage. A value there is a reporter's preference, not an
assessment, so it is untrusted input. It now feeds the rubric as a sixth
signal bonus — flat +1 for Urgent or High, identical for both — and never
maps to a band. Urgent needs 12, so nothing a reporter types reaches it
alone; the issue must already sit at 11 on maintainer-assessed axes.
Supersedes the earlier "it is noise" framing: the signal is worth keeping,
it just cannot be allowed to decide the outcome. Also corrects the band
arithmetic (2-15 -> 2-16) now that a sixth bonus exists, and notes that
scores recorded on 2026-08-01 cite the old /15 denominator.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: drop the /15 denominator caveat now the recorded scores are restated (#1891)
The 51 triage comments were rewritten from /15 to /16, so the note saying
they cite the old denominator no longer describes anything. No total moved:
the reporter-set signal only scores for Urgent or High, and the one scored
issue carrying an org-field value (#1826) had Low, which earns nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* docs: describe the board-option hazard per field, not Status-only (#1891)
Copilot review on #1892 flagged three places that still read as
Status-only now that the section covers Priority too:
- the version-label bullet said a v2 issue needs "a board card with a
Status"; it needs a Status and a Priority (v1 needs only a Status,
since board #11 has no Priority field)
- the updateProjectV2Field hazard said a bad edit "orphans the Status
of every card"; it orphans whichever field was edited
- the recovery recipe is genuinely Status-specific (reads .status,
writes the Status field id), so say so and give the two
substitutions for a Priority deletion
Verified `gh project item-list --format json` exposes both .status and
.priority before documenting the .priority swap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
