Copilot review round 1.
`refreshStoredAuthToken` consumes the same fallback and made a single SDK
discovery call, so preserving the path there — without the walk the other two
consumers got — would have regressed exactly the case the walk exists to keep:
a path-hosted MCP server whose authorization metadata is published at the
domain root.
It injects its own discovery function as a test seam, so the walk is now also
exposed as `discoverAuthorizationServerMetadataFromCandidates(candidates,
discover)`, which takes that function and reports *which* candidate answered —
the CLI needs the winning URL, since it is the base the token request is made
against, not just the metadata.
Also drops the `as unknown as typeof fetch` from the new test in favour of
`vi.fn<typeof fetch>()`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Caught by CI's clients/web format:check, not by my local run — I had
formatted core/ and scripts/ but not the web client's src, which is where
the test lives.
Signed-off-by: cliffhall <cliff@futurescale.com>
When RFC 9728 protected-resource metadata names no authorization server, the
MCP server URL stands in as one. `getAuthorizationServerUrl` built that with
`new URL("/", serverUrl)`, which discards the path — exactly the part discovery
needs when the server is not hosted at the domain root. The SDK's
`buildDiscoveryUrls` derives the path-scoped well-known locations from that
pathname, so an origin-only value could only ever probe the domain root, which
404s for a server like `https://example.com/mod/minilesson/mcp.php`.
Keep the path (and strip query/fragment, which an RFC 8414 issuer never
carries). A straight swap would regress the case the origin-only value did
serve — a server that merely lives under a path while publishing its metadata
at the root — so the URL becomes the first of two candidates:
`getAuthorizationServerUrlCandidates` yields the path-scoped form then the bare
origin, and `discoverAuthorizationServerMetadataForServer` walks them, treating
a candidate's failure as non-fatal while another remains and rethrowing the
first error only when all of them fail.
`discoverScopes` and the CIMD pre-registration probe both go through the walk.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
parseSSE declared currentEvent/currentData inside the read loop, so an SSE
frame whose data: line arrived in one chunk and whose terminating blank
line arrived in the next was discarded. buffer already carried a partial
LINE across reads; nothing carried a partial FRAME.
Because this is the web client's transport, the dropped frame is a
JSON-RPC response that never settles: the request hangs forever with no
rejection, no console message, and nothing in the Network tab. The
payload does not arrive corrupted, it does not arrive at all, which is
why it presents as a hang rather than an error.
It has not bitten us because Chromium and Firefox happen to deliver whole
frames at the payload sizes this transport currently sees. That is luck,
not a guarantee: chunk boundaries depend on payload size, TCP
segmentation, and any intermediary. The exposure grows with payload size,
so a large resources/read is the most likely first victim.
The fix hoists both out of the loop and adds an end-of-stream flush — a
server that closes without a final blank line has still delivered a
complete frame, and dropping it would lose the last message of every such
stream.
parseSSE is now exported, solely so the test can reach it. That is the
only way to hit this deterministically: driving a real transport cannot
steer where a chunk boundary lands, but a test that supplies the reader
can put it exactly on the terminator.
The test keeps the two PASSING cases (whole frame, split mid-payload)
alongside the three that failed, because they are what make the diagnosis
precise. This is not "SSE parsing is broken" — mid-payload splits are
handled correctly today, and only the frame-terminator boundary loses
data. Verified against the unfixed code: 3 of 5 fail, all 5 pass after.
Signed-off-by: cliffhall <cliff@futurescale.com>
`record` read `serversRef.current` from inside the `setRecords` updater. A
functional updater must be pure -- React may defer or replay it -- so the
pairing resolved at whatever moment the updater happened to run rather than
when the record was written. A list read committing in between would then be
paired with the record as its baseline, and the very read that should have
superseded the override would instead certify it.
Read the list once, at call time, and use it for both the pairing and the
prune.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
The rollback's explanation still described the removed app-wide boolean and
claimed the override was applied only for the active server. That is now the
live client's rule alone: the override is keyed by this write's server and is
only ever displayed while that server is active, so it is recorded whatever
is active by the time the rejection arrives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
The displayed `paginatedLists` value falls back to the active `servers`
entry, and that list only advances when a `GET /api/servers` succeeds — so
once a list read fails it keeps describing disk as it was before the writes
made since. The optimistic override papered over that, but it was a single
app-wide slot cleared on every change of the *active* entry, so switching to
another server and back dropped it and the UI fell back to the stale entry.
Two things then disagreed, and the one the user could see was the wrong one:
the toggle read `off` while disk, the write tracker, and the client just
built from it all said `on`; and because the client skipped its connect-time
aggregate walk, the all-pages UI rendered an aggregate that was never
fetched, with no Load-next-page control to fill it.
Hold the override per server instead, in `usePaginatedListsOverride`, paired
with that server's entry as of when it was recorded and believed only while
the list still carries that same entry object — identity, not equality, so a
read reporting the value the override replaced supersedes it too. A server's
override then survives a switch away and back, and is still superseded by
the first successful read that rebuilds its entry.
The read cannot instead move to `lastPersistedSettings.resolve(id)`: that is
called during render, and it both reads refs and prunes superseded records.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
#2114 landed on v2/main, which is the PR this one follows up. Two
conflicts, one of them silent.
The marked conflict was cosmetic: keep v2/main's comment on the
`provider.scope` read, which now notes the getter filters (#2068).
The real one git merged cleanly and would have left dead: #2114's
if (this.oauthConfig.requestRefreshToken === false && requestedScope) {
this.pendingAuthorizationScope = requestedScope;
}
sat directly above this branch's unconditional
`recordAuthorizationRequestScope`, which overwrites the same field a few
lines later. Dropping that gate is what #2117 is for, so it is removed
rather than kept as a no-op.
The two changes compose: the SDK adds `offline_access` only when
`clientMetadata.grant_types` includes `refresh_token`, which the provider
drops while the opt-out is on -- so reading the authorize URL yields the
filtered scope there, exactly what #2114's gated branch persisted.
#2114's third case pinned the gate and inverts here, as its issue
predicted. It now asserts the request is persisted even with the grant
on; that records no new claim, since the request equals what storage
already held.
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>
Copilot review round 8 (its one suppressed finding; no new inline comments).
The `boolean-schema` suggestion for a bare `true` said the replacement
"narrows the schema deliberately". That holds in an ordinary property
position and inverts under a negating parent: `{"not": true}` rejects
every value, so swapping in `{"type": "object", …}` leaves the parent
rejecting only objects and every non-object then passes — the enclosing
contract is widened. Under `if` it redirects which branch applies rather
than tightening anything.
- Parent-aware advice under `not`: suggest `{}`, which is the exact
object form of `true` and is already exempt from `untyped-schema` in
that position, with the message saying why a concrete type would be
wrong there.
- Everywhere else, and in the `untyped-schema` suggestion too, describe
the replacement as a deliberate CHANGE of contract rather than a
narrowing. A suggestion has no business claiming an effect it cannot
guarantee across the positions it can print from.
Tests pin the `not`-specific advice and assert no bare-`true` suggestion
in any of the three positions contains "narrow". The CLI README's sample
report is re-captured from a real run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 7.
- Add `contentSchema` to the traversal. It holds the schema the decoded
string content must satisfy, so a bare `true` or a remote `$ref` under
it is a real finding all three clients were walking past. Also admit
the three content keywords to `CONSTRAINING_KEYWORDS` on the same
documented grounds as `format` — annotation-only by vocabulary, but
plainly an attempt to constrain, so reporting such a schema as
accepting anything would be noise. The comment now names that
category rather than singling `format` out.
- Make `handleError` async and await the envelope write. Round 6 made
the `--strict` report await its write and stopped one line short: the
envelope emitted for the very same throw still raced `process.exit()`,
so the machine-readable line a CI caller parses could be lost on a
pipe. The launcher's comment claiming the envelope was "well inside
the pipe buffer" was wrong reasoning and is replaced — fitting in the
buffer says nothing about whether the write was performed. Its call
site now awaits; the bin's `.catch(handleError)` needed no change.
- The CLI README called its sample block the "complete output" when the
error envelope follows it on every non-zero exit. Now scoped to "the
complete report", with the envelope named and linked.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6.
- Share one traversal between `walk` and `declaresAnyId`. The `$id` guard
added last round descended through EVERY object value, including
instance data — `examples: [{"$id": "x"}]`, a `default`, a `const` —
where `$id` is a payload key declaring no embedded resource. Reading
one there suppressed `remote-ref` for the whole document, turning last
round's false-positive fix into a blanket false negative. Both passes
now go through `forEachSubschema`, driven by the same keyword tables,
so they cannot drift about what counts as a schema position.
- Await the stderr writes. `writeSchemaLintReport` used a bare
`process.stderr.write`, but both CLI exit paths call `process.exit()`
as soon as the work returns, which discards anything still buffered on
a piped stderr — truncating or losing the multi-block `--strict`
report on exactly the redirected-output runs a CI caller uses. Added
`awaitableError` beside the existing `awaitableLog` and awaited it.
The test's fake defers its callback so a synchronous one cannot make
the assertion vacuous; two existing stderr stubs that never invoked
their callbacks were fixed in the same pass.
- Show the CLI README's sample report in full. It printed one error
block immediately followed by the summary, a shape the formatter
cannot produce — every finding gets a block. Captured from a real run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5.
- The sidebar tooltip applied `hasError` to the TOTAL, so a tool with one
error and three warnings announced as "4 schema portability errors" —
a defect the previous round introduced while adding that wording, and
worse than the vague text it replaced. Both the tooltip and the
accessible label now carry the breakdown, formatted by a new shared
`summarizeToolFindings` so the fourth place to phrase a severity count
cannot drift from the other three.
- Suppress `remote-ref` when the document declares an `$id` anywhere.
`$id` establishes a base URI, so with a root `$id` of
`https://example.com/root` the ref `https://example.com/root#/$defs/x`
resolves to THIS document and needs no fetch — the finding and its
"inline the referenced schema" remediation would both be wrong.
Classifying correctly needs full RFC 3986 resolution against nested
embedded resources, which this module does not do, so it declines
rather than guessing. A missed finding is the acceptable direction.
- Run the `--strict` validations before every short-circuit return.
`servers/list`, `servers/show`, `--list-stored-auth` and
`--print-handoff` all return from `parseArgs` before any connect, so
the checks further down let `--strict --method servers/list` succeed
while silently ignoring a tools/list-only flag.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4.
- Add `SCHEMA_UNPORTABLE` to `codeForExit`. `CliExitCodeError` lets a
caller omit the envelope, so without the case an envelope-less throw
reported the generic `error` — the same silent mislabelling the rename
was meant to prevent, one layer down. Covered via the envelope-less
construction that exposed it.
- Keep a local `type` (and the preset's `required`) alongside the remote
`$ref` in the showcase config. The TUI's `schemaToForm` dispatches on
`type`, so a `$ref`-only property fell through to a string field that
`add`'s numeric handler then rejected — making the README's "every
showcase tool is runnable" claim false there. The `remote-ref` finding
is unaffected, since that rule reads `$ref` independently.
- Restrict `type-union` to well-formed unions: non-empty, unique, and
all recognized JSON Schema type names. Its message asserts the
construct is *legal* JSON Schema, which is false for `[]`, `[3]` or
`["bananas"]`, and the `anyOf` it suggested for those would be invalid
too. Malformed schemas stay the SDK parser's business.
- Drop `if`/`then`/`else` from the presence-only allowlist. They only
constrain as a pair, so `{"if": {"const": 1}}` accepts every value and
was evading `untyped-schema`; the pairing is now checked explicitly.
- Distinguish the web sidebar severities by SHAPE, not just colour — a
circle for an error, a triangle for a warning — and name the severity
in the tooltip. Colour alone left a colour-blind reader unable to tell
them apart.
- Fix the stale "across 2 tools" count in the CLI README's hint sample.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
- Rename the `--strict` failure from "invalid" to "unportable"
(`EXIT_CODES.SCHEMA_UNPORTABLE`, envelope `schema_unportable`, and the
message text). The module argues at length that these schemas ARE
valid JSON Schema and are merely refused by some clients, so a
machine-readable code of `schema_invalid` told automated callers the
opposite. Done now, while nothing depends on it.
- Add a fixture-path integration case. Every other test built a
`ServerConfig` by hand, so `resolveConfig`'s `rawToolSchemas`
forwarding was uncovered and `unportable-schemas-http.json` was loaded
by nothing — the shipped fixture could have lost its overrides with
the suite still green. It now goes through loadConfig/resolveConfig
and asserts the exact per-tool verdict the README claims.
- Refresh the CLI README's sample output, which still showed the
pre-round-2 wording and fixture, and so dropped the contract warning
that rewording existed to add. Captured from a real run rather than
written by hand, plus a paragraph making the suggestion pattern
explicit.
- Drop the self-referential "[Try it](#unportable-tool-schemas)" link in
the root README, which pointed back at its own section.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2.
- Remove `non-object-root` entirely. It could never fire: the SDK types
`inputSchema` with `type: literal("object")`, so a tool with any other
root fails `ListToolsResultSchema` and `salvageListItems` drops it
before any client sees it. Verified against a live server advertising
`inputSchema: {type: "array"}` — `listAllTools()` returns []. A rule
that cannot fire is worse than none, because the docs then claim a
check the tool does not perform; the condition is already reported
through the malformed-items surface. Pinned by an integration test so
a change in that behavior is a signal rather than a silent gap.
- Fix the bare-`false` suggestion. `properties: {a: false}` forbids the
property; deleting the entry *permits* it with any value under the
default `additionalProperties`, so the advice inverted the schema it
replaced. Now recommends `{"not": {}}` and says why deletion differs.
- Exempt `not`-parented schemas from `untyped-schema`, so the `{"not":
{}}` above does not trip this module's own rule — under negation an
always-accepting subschema means always-reject, making "accepts any
value" the exact opposite of what it does. Scoped to that position,
not inherited by its subtree.
- Say plainly, in the `true` and untyped suggestions, that they narrow
the schema deliberately rather than implying an equivalent rewrite.
`true` has no portable object-form equivalent — `{}` is the very shape
this lint flags.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
- Scope `non-object-root` to `inputSchema`. The pinned SDK types
`inputSchema` with `type: literal("object")` but `outputSchema` as a
bare `looseObject`, so a `{"type":"string"}` output is conforming; the
rule was claiming a requirement MCP does not make, at error severity.
- Decide `untyped-schema` from an allowlist of constraining keywords
rather than a denylist of annotations. JSON Schema ignores keywords it
does not recognize, so `{"vendorHint": true}` accepts every value and a
denylist passed it as constrained. `$defs`-only schemas now flag too.
- Walk `dependentSchemas` and draft-07 `dependencies`, with a test
pinning that the property-name-array form is not a schema position.
- Suggest `anyOf` branches for an array-form `type`, never "drop it from
`required`" — accepting `null` and permitting absence are independent
contracts, so the old suggestion silently changed the schema's meaning.
- Reject `--strict` alongside `--app-info`. That path returns NDJSON from
`runMethod` and never reaches the lint, so the flag was a no-op gate
that could never fail.
- Colour the web sidebar flag from the `--inspector-*` severity tokens,
matching the detail panel and avoiding the default yellow that failed
the a11y check.
- Add a live integration test for `rawToolSchemas`. It caught that an
`outputSchema` override on a preset returning no structured content
makes every call to that tool fail client-side validation — the
showcase config did exactly that, so the flagged tool could not be
run. Moved the bare `true` onto `get_temp`, documented the caveat, and
pinned both halves.
- Drop the double cast from the lint test fixture.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
`trySilentEmaAuth` and `refreshEmaResourceTokens` saved tokens without
ever saving a scope. Both are reached from real paths -- initial EMA
authentication with a cached IdP session, refreshEnterpriseManagedTokens,
and EmaTransportOAuthProvider.tokens -- and each can be the moment a
newly configured or metadata-derived scope first takes effect, so a
silent token response left the previous stored scope standing.
Rather than a third copy of the rule, every EMA mint now ends in one
place: mintEmaResourceTokens reports the scope it requested, and
saveMintedTokens writes tokens and the RFC 6749 5.1 scope together.
The manager's EMA branch drops its own saveScope, which would now be a
redundant second write of the same value; the step-up branch keeps its
own, since it falls back to the challenge's authorizationScopes union
and runs last.
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>
Closes#1005
A tool schema can be perfectly legal JSON Schema and still be rejected by
the client the server is meant to run against — Go's `jsonschema` emits a
bare `true` for `interface{}`, which the SDK accepts and Claude Code
refuses with an opaque "Invalid input". The Inspector passed it through
silently.
Adds `core/json/schemaLint.ts`, a portability lint over a tool's
inputSchema/outputSchema, and surfaces one shared verdict everywhere:
the CLI's `--strict` report (exit 6 on an error-severity finding), the
TUI's tool list marker + detail pane, and the web Tools sidebar flag +
detail section.
Deliberately not a JSON Schema validator: a census of 617 public servers
found zero that fail the SDK's own parse, so a conformance check would
report nothing on real servers. Each rule is instead a construct that is
legal JSON Schema and known to be refused or mishandled by a shipping
client — a bare boolean in a schema position, an array-form `type`, a
remote `$ref`, a schema with no validation keyword, a non-object root.
`test-servers/configs/unportable-schemas-http.json` exercises every rule,
via a new `rawToolSchemas` override — the Zod-built presets cannot emit
any of these shapes, which is the same reason a real server only hits
this when its schemas come from another generator.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Two more paths had the same defect, both found in review.
`handleStandardAuthChallenge` returns an interactive redirect for
`unauthorized` / `token_expired` / `invalid_token` as well, and its
callback lands in the same completeOAuthFlow -- but the recording was
gated on `insufficient_scope`. Those challenges compute their own
`scopeForAuth`, which can differ from what storage holds, so a silent
token response left the stale scope standing. The read is now one
private helper both entry points call, applied to every redirect.
And `scopeForMint` is not always what the EMA mint asks for: with
neither config nor storage supplying a scope, discoverEmaResourceContext
resolves it from the protected resource metadata's `scopes_supported`,
which is what both exchanges send. `completeEmaIdpAuthorizationAndMint`
now resolves that context itself -- passing it into the mint, so no
extra discovery -- and returns it as `requestedScope` for the manager to
use as the RFC 6749 5.1 fallback.
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>
`provider.scope` is not what the authorization request asked for. The
SDK augments it with `offline_access` when the AS advertises that scope
and the client wants a refresh token, so persisting the provider's copy
after a silent token response understates the RFC 6749 5.1 implied
grant -- the same defect this change exists to fix, one notch smaller.
Read the scope off the captured authorize URL instead, falling back to
`provider.scope` when the URL carries none.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
The Server Settings modal edits a stdio server's `env` and `cwd` as
settings, while the transport reads them off the *config* — so #2091's
tracker, which resolves settings through the last write known to have
reached disk, left those two behind: `server.config` still came straight
off a `servers` entry that stops advancing once a list read fails. A save
that landed during such a window kept spawning the child process with the
pre-save environment, while the modal (re-seeded from the tracker) showed
the new value. Credentials live in `env`, so the symptom is the inspected
server failing to authorize rather than a visibly lost edit.
Adds the settings→config mapping to `core/mcp/serverList.ts` — the one
place both sides of the wire can share — and routes the `/api/servers` PUT
write-through and the web client's connect path through it, so the value
persisted and the value spawned cannot be derived differently.
Closes#2096
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
RFC 6749 5.1 defines an omitted `scope` in a token response as a grant
identical to what was requested, and `resolvePersistedScopeAfterGrant`
already encodes that rule -- but the ordinary authorization leg never
gave it a `requestedScope` to fall back to, so nothing was persisted and
the previous stored scope kept being reported as what the current token
carries.
`authenticate()` now records `provider.scope` as the pending
authorization scope once the authorization URL has been captured, so it
survives the redirect and reaches `completeOAuthFlow`. The
enterprise-managed leg had the same defect for the same reason and is
fixed one level up: it now falls back to `scopeForMint`, which is what
the mint actually asked for, rather than to the raw
`pendingAuthorizationScope` that is undefined on this leg.
A response that does carry `scope` still wins over the request, on both
legs.
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>
Copilot review round 11 on #2114. The docs claimed the persisted scope
"self-heals on the next grant"; on the ordinary authorization leg it
did not.
`authenticate()` never set `pendingAuthorizationScope` — only the
step-up paths did — so `completeOAuthFlow` called
`resolvePersistedScopeAfterGrant(tokens.scope, undefined)`. When the
token response omits `scope` (RFC 6749 §5.1: omitted means the grant
matched the request), the fallback was undefined, nothing was saved,
and the stale `offline_access` stayed in storage where
`resolveEffectiveGrantedScope` keeps reporting it as granted.
`authenticate()` now records the scope it actually requested, read once
from the filtering getter so what is recorded is what went out.
Gated on the filter being active rather than applied unconditionally.
The general version is arguably more RFC-correct, but it would change
what lands in storage for every server that never touched this setting,
inside a PR whose contract is that nothing changes unless the box is
unchecked. With the grant declared, request and storage agree and there
is nothing to heal.
Three tests; the omitted-scope one fails when the recording is removed.
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>
Copilot review round 10 on #2114 — a defect in round 8's fix, and the
precise loop that fix's extra argument was added to prevent.
`enrichChallengeWithScopes` narrows `requiredScopes` to the *missing*
subset, so a challenge requiring `offline_access tools:write` against a
stored scope that already carries `offline_access` arrives as just
`["tools:write"]`. Passing that narrowed list made the filter read the
server's explicit requirement as inherited and strip it, so
re-authorization would earn the same challenge forever.
Passes `challenge.requiredScopes` — the raw list — instead. The mixed
case is now covered, and fails against the narrowed list.
Also stops the checkbox promising more than the setting delivers: it
does not register anything for a preconfigured or CIMD client, and the
authorization server decides issuance either way. It now says it
declares the grant so the server *may* issue a refresh token.
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>
Copilot review round 9 on #2114. "Scopes above lists offline_access"
reads as a plural subject with a singular verb; naming the control
("The Scopes field above lists ...") fixes the agreement and says which
control to go edit.
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>
Unrelated to #2108's diagnostics work, folded in at the maintainer's request
rather than tracked separately.
The sticky full-bleed bar cost the whole view a band of vertical space for what
is a notification about one server, and it pushed down the monitoring sidebar
that an OAuth failure now opens. Every corner is spoken for — the toast stack
owns bottom-right, the right edge is the sidebar whose toolbar an anchored card
covers, top-left is the Servers header — so it is centered instead, which is
the one placement that reads as addressed to the window rather than attached to
the wrong panel.
Deliberately WITHOUT an overlay. This is a notification, not a decision that
must be made now, and blocking the page would force a choice between reading
the requests that explain the failure and keeping the affordance — dismissing
is not free, since "Authorize again" also clears the stale OAuth state, which a
plain reconnect does not do.
Kept as the existing declarative component rather than routed through Mantine's
`notifications`: that module is mocked in App.test.tsx, so switching would have
broken the ten tests asserting on this banner's text and its action button,
including the lost-authorization-state coverage. The message now stacks above a
right-aligned action, which a ~420px card needs.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8 on #2114.
The provider's `scope` getter is not the only request path. Mid-session
`insufficient_scope` recovery builds its union from *raw* storage in
`enrichChallengeWithScopes` and hands it to `mcpAuth` directly, so it
never reads that getter. An `offline_access` inherited from an earlier
grant therefore reappeared during step-up and the SDK re-added
`prompt=consent` with the setting off — the failure this option exists
to prevent, on the one path that bypassed it. The comment claiming the
getter was "the one seam both readers go through" was wrong, and is
corrected to name both.
`handleAuthChallenge` now applies the same filter, gated on the setting
so the default path is untouched. The forced-retry reuses that value,
so filtering once at the computation site covers it.
`scopeForDeclinedRefreshGrant` gains the scopes a challenge requires,
honored exactly like a configured one: stripping a scope the server is
asking for would loop — re-authorize, earn the same challenge, strip it
again.
Three regressions; the inherited-scope one fails when the filter is
reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
The description had grown into a paragraph that dominated the
Authorization section, and it named a specific vendor's error code
(AADSTS90094) in a control that is not vendor-specific.
Three lines now: what the grant does, when to turn it off, when it
takes effect. The Entra specifics, the automatic-scope nuance and the
does-not-revoke caveats stay in docs/mcp-server-configuration.md, which
is where someone goes for the detail. The inline offline_access warning
and the EMA note are unchanged.
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>
Copilot review round 7 on #2114 (suppressed comment). AGENTS.md
forbids inline handler logic; the new checkbox had it.
The eight OAuth controls beside it are inline in the same shape, so
consistency was an argument for leaving it. Complying anyway: this repo
holds new code to its gates even where neighbors predate them, the same
way the coverage floor works. Extracting the siblings would be an
unrelated refactor in a bug-fix PR.
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>
Copilot review round 6 on #2114.
The early return handed back the persisted scope unchanged, so an
`offline_access` the user had just typed into Scopes never reached the
request when storage predated the change — and storage routinely does,
since `createOAuthProvider` preserves a stored scope rather than
reseeding from current settings. The docs promised configured scopes
are honored and the form warned that `offline_access` "is still
requested"; both were false in exactly that state.
Honoring it means adding it, not merely declining to strip it.
Only the token itself is added, not the rest of the configured scope:
unioning unrelated configured tokens into the request is outside what
this setting governs and could push an otherwise-satisfied connection
into a step-up authorization.
Three tests, all verified to fail against the previous behavior.
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>
Copilot review round 5 on #2114 — a hole in last round's fix.
When the persisted scope was `offline_access` alone,
`scopeForDeclinedRefreshGrant` returned undefined.
`OAuthManager.createOAuthProvider` reads that as "nothing stored" and
seeds storage with the configured scope, so the request-only filter
silently overwrote the value it promised to leave alone — and
re-enabling the grant could no longer restore it.
It now falls back to the configured scope, which is both the right
thing to request and inert to that branch. The previous "storage is
never rewritten" test could not see this: it ran at the provider level,
while the seeding branch lives in the manager. The new regression runs
there, and fails when the fallback is removed.
Also corrects the EMA rationale, which was wrong even though its
conclusion held. `createOAuthProviderForTransport` does build a
`BaseOAuthClientProvider` and wrap it, and `EmaTransportOAuthProvider`
forwards `clientMetadata` — so the setting is not bypassed outright.
What it cannot change is the IdP authorization leg's fixed
`openid offline_access`, which is the leg the user signs in through and
therefore the reason the control cannot affect EMA's consent behavior.
Fixed in the docs, the code comment, the UI string and the test.
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>
Copilot review round 4 on #2114 — a real gap, not a wording one.
Declining the grant stops the SDK *adding* `offline_access`, but the
Inspector's own storage puts it back. A successful default-on
authorization persists the scope the AS granted
(`resolvePersistedScopeAfterGrant`), `offline_access` included;
`prepareForAuth` reloads it and `authenticate` hands it to the SDK as
`scope`, which sees the token in scope and appends `prompt=consent`
again. So any server that had authorized once could uncheck the box and
still hit AADSTS90094 — the users most likely to reach for the setting
were the ones it did not help.
`scopeForDeclinedRefreshGrant` filters it out of the *request* only,
applied in the provider's `scope` getter — the one seam both
`clientMetadata.scope` and `authenticate`'s `scope:` argument read
through. An `offline_access` the user configured is honored; storage is
never rewritten, so re-checking the box restores the old behavior
instead of finding the value destroyed, and scope-satisfaction checks
still see the real grant. Verified by reverting the filter: the new e2e
regression fails.
Also from that round, both previously suppressed:
The checkbox looked effective under enterprise-managed authorization,
which never builds this provider at all — it authorizes against the
enterprise IdP with a fixed `openid offline_access`
(`ema/constants.ts:16`). It now annotates itself the way the endpoint
overrides do. The existing count assertion covered two such notices and
was named for the overrides alone; it is renamed and expects three, with
a dedicated case pinning the notice to this control so the count cannot
be met by three copies elsewhere.
And "obtains no new refresh token" was another overstatement — the AS
decides issuance, and a preconfigured client is reused with whatever its
registration already permits. Replaced with what the setting actually
controls.
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>
Review round 4 raised that the callback catch leaves `activeServerId` set: the
disconnect there emits only on a status change, and a rejected token exchange
throws inside `completeOAuthFlow` before the reconnect, so the client is still
at its initial "disconnected" and the listener that would clear it never fires.
That is accurate, and clearing it is nonetheless wrong. "Authorize again" on
the re-auth banner hands `clearServerOAuthState` the live client only when the
banner's server is the active one; releasing it here passes `null` instead and
the stale tokens are never cleared from the client holding them, which is the
one thing that recovery exists to do. Confirmed by trying it — the existing
lost-authorization-state test fails on `clearOAuthTokens` never being called.
So the retention is now asserted rather than incidental, with the reason at
both the code and the test, and the InspectorView double surfaces `activeServer`
so a later cleanup of the apparent leak fails loudly instead of quietly
breaking the recovery.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The SSE branch selects a caller-supplied `eventSourceInit.fetch` as-is, so
it bypassed the challenge observer and a first-time legacy SSE
authorization lost the advertised URL the same way. Observing is passive,
so it is applied to that fetch too while the deliberate no-proxy-wrap rule
stays. Covered behaviorally against a 401-serving server.
- Dropped an `as unknown as typeof fetch` from the new integration test
(banned by AGENTS.md, and it erased the signature); the spy is typed as
`typeof fetch` directly. The `as MCPServerConfig` beside it was
unnecessary too.
- The integration test reused a per-pid OAuth storage file. SDK `auth()`
persists the advertised URL in its discovery state, so a rerun could
satisfy discovery from that cache and pass with the plumbing removed —
which would have quietly falsified the regression check. Fresh mkdtemp
per run, removed in teardown.
Adds observer unit tests (challenge reported, body untouched, 403 included,
silent on success).
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 3.
Both catches report a failure without ending the attempt, and in each the
preceding rejection is an auth-recovery error — which deliberately holds the
status at "connecting" rather than moving it to "error". So the toggle spins
and the active-server lock is never released:
- the auth-recovery arm, when `checkAuthChallengeSatisfied` is what rejected
(the retrying `connect()` does reach "error", so only this half was stuck);
- the callback leg, whose `resumeAfterOAuth` carries a reconnect that can
reject the same way. Disconnect ahead of the EMA guard there, since a stuck
session is worth clearing whichever way the error classifies.
The fetch log survives a disconnect, so the Network diagnostics this issue is
about are unaffected. Both teardowns are asserted by the tests that already
cover those arms.
Also fixes a stale comment left by round 2: the arming-hooks type carries four
hooks, not three.
Signed-off-by: cliffhall <cliff@futurescale.com>
The challenge-driven plumbing did not reach the default CLI/TUI flow. With
no stored token in the legacy era, InspectorClient deliberately builds the
transport with no authProvider (so the SDK cannot open a browser before the
callback server is listening), which also leaves challenge interception off;
the 401 then surfaces as the SDK's headerless UnauthorizedError and the
runner calls authenticate() with no challenge in hand. So a server serving
metadata only at the advertised path still failed there — including for the
legacy showcase the README instructs.
Add a passive counterpart to the challenge interceptor: an observer fetch
that reports every 401/403 without touching control flow or the body. The
OAuthManager records the advertised URL from it and uses it in
authenticate() and its CIMD pre-registration. Latest-wins, including
clearing when a later challenge advertises none.
Also: the fixture helper dropped an explicitly supplied empty
resourceMetadataPath by truthiness, bypassing validation and silently
reverting to the well-known route.
Covered by a direct-client integration test that drives connect ->
authenticate() against the real server and asserts the advertised path was
requested and the well-known one never was; verified failing without the
fix.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2114.
The unit tests asserted our own `grant_types` array and stopped there.
What actually unblocks an Entra tenant is what the SDK puts in the
authorize URL, and nothing covered that — so an SDK change that stopped
keying its augmentation off `grant_types` would leave AADSTS90094
intact with every test green.
Adds the complementary case to the existing e2e suite, against a real
AS advertising `offline_access`: with the grant declined, the authorize
URL carries neither `offline_access` nor `prompt=consent`, while the
configured scope still goes out. Verified non-vacuous by reverting the
provider conditional — it fails. The positive test alongside it now
also asserts `prompt=consent` is present, which was unasserted despite
being the literal failure in the issue.
Drops an unjustified `as unknown as ServerEntry` I added in the App
test — AGENTS.md forbids it, and it hid a real mismatch (a partial
settings object against a complete one). Typed `ServerEntry` /
`InspectorServerSettings` fixtures instead, so a field added to that
interface later is a compile error rather than a missing value at
runtime. The `constructedOAuth` helper reads through the constructor's
own parameter type and needs no cast at all now.
Scopes the two doc comments that still claimed the flag removes every
`offline_access` — `StoredMCPServer.oauth` and `OAuthProviderConfig`,
both contradicting the caveat already stated elsewhere.
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>
