fix: preserve CIMD registration provenance across SDK issuer binding

[cliffhall]

Closes #2242

Connection Info labeled a CIMD registration Dynamic (DCR) even though no POST /oauth/register ever happened. The Client ID shown was correct (the metadata-document URL); only the provenance was wrong.

Screenshots

Same server, same flow, same Client ID — a live CIMD login against a test authorization server advertising client_id_metadata_document_supported, with a real HTTPS client-metadata document. Neither run made a single request to a registration endpoint. The after shot was recaptured against the final commit.

Before (v2/main)	After
Client registration → Dynamic (DCR)	Client registration → Client ID Metadata (CIMD)

The connection both runs produced

Cause

BaseOAuthClientProvider.saveClientInformation accepts two shapes: our own pre-registration callers pass { registrationKind }, and the SDK passes { issuer }. It treated every save without an explicit kind as DCR.

The SDK reaches that method from three places in auth(), and only one of them is a dynamic registration:

SDK call site	What it is	Old kind	Correct kind
back-stamp an existing registration with its issuer (SEP-2352)	issuer binding	dcr	whatever it already was
client_id = clientMetadataUrl when the AS advertises client_id_metadata_document_supported	the SDK's own CIMD write	dcr	cimd
registerClient(...)	a real DCR	dcr	dcr

So ensureCimdClientRegistration stored cimd correctly before authorization, and the issuer-binding write silently overwrote it moments later — which is why clearing OAuth state and reconnecting never helped.

Fix

SDK v2's saveClientInformation contract passes only { issuer }, so the mechanism cannot be handed to us. But it does not have to be inferred either.

auth() calls saveDiscoveryState before it reads or writes client information, and it takes its URL-based-client-ID branch — rather than registerClient — exactly when that metadata advertises client_id_metadata_document_supported and a clientMetadataUrl is configured. So the branch the SDK took is read back from the state the SDK itself just wrote, using the same predicate it branched on.

resolveSdkRegistrationKind first requires that CIMD is in play at all: configured for this connection, and the incoming client_id equal to that metadata-document URL. Anything else is dcr immediately. From there it is two cases, told apart by whether a registration already exists for this issuer:

Case	Answered by
A registration exists for this issuer under this client_id — the SDK is only adding the issuer to it	that registration's recorded kind (kind and credential are written and cleared together, so a stored registration always has one)
Nothing exists for this issuer — this save creates the registration	the persisted discovery state, guarded on its metadata naming this same issuer so a state left over from a previously resolved AS cannot answer for a different one

The URL comparison only decides whether CIMD is in play; the two cases decide what actually happened. That matters because RFC 7591 §3.2 leaves a dynamically issued client_id opaque, so an AS may mint the metadata URL itself.

Consequences, each of which was a defect on the way here:

• An existing DCR whose client_id happens to be the metadata URL stays dcr — it takes the back-stamp case and its recorded kind says so.
• invalid_client recovery stays cimd. auth() answers that error with invalidateCredentials("client") and an immediate retry; the clear removes the registration and its kind, so the retry takes the new-registration case and is answered from discovery state, which the clear does not touch.
• A second authorization server behind one resource gets its own answer, since discovery state describes the issuer the SDK actually resolved. One that does not advertise CIMD is dcr even when it mints the metadata URL as its client_id.
• A transient failure in our own CIMD preflight costs nothing, because what this reads is the SDK's own discovery.

Supporting changes in ensureCimdClientRegistration: it binds its pre-registration to the issuer it just discovered (SEP-2352 keys registrations per AS, and its "already registered?" check is keyed the same way — read ctx-less it resolved through the active issuer and early-returned for every later one); it reuses provider.discoveryState() before fetching; and a discovery failure now skips pre-registration rather than propagating, since this helper is an optimization over what auth() does for itself.

No UI or storage-schema change — Connection Info already renders whatever kind is stored.

Tests

The auth suites are at 627 passing, and the CIMD OAuth E2E at 34. New coverage spans both mocked cases and end-to-end ones driven against a real OAuthStorageBase and the real ensureCimdClientRegistration, since several of these behaviours are about how storage promotes and clears slots across issuers — which a mock would assert away.

Every guard was mutation-checked independently:

Reverted	Result
the whole dispatch back to : "dcr"	6 failed
back-stamp ignores the stored kind	2 failed — including the end-to-end existing-DCR case
new registration ignores CIMD support	2 failed
drop the issuer guard on the discovery state	1 failed
ensureCimdClientRegistration back to its ctx-less early-return	2 failed
drop the issuer from the pre-registration save	2 failed
drop the cached-discovery reuse	1 failed
make a discovery failure fatal again	1 failed

Reverting the dispatch also fails the CIMD OAuth E2E test on both transports — that failure is the reported bug itself, a completed CIMD connection reporting registrationKind: "dcr".

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA

[Copilot]

🟢 Approval recommended

All reviewed changes are covered by focused unit and end-to-end tests, with no unresolved issues.

Pull request overview

Fixes CIMD registrations being incorrectly relabeled as DCR during SDK issuer binding.

Changes:

• Preserves registration provenance using client ID and stored state.
• Adds unit coverage for provenance resolution.
• Verifies CIMD provenance across OAuth transports.

File summaries

File	Description
core/auth/providers.ts	Preserves CIMD provenance during SDK saves.
clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts	Verifies end-to-end CIMD state.
clients/web/src/test/core/auth/providers.test.ts	Covers provenance resolution and fallback behavior.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Inferring provenance from client_id can mislabel a genuine dynamic registration as CIMD.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 2 — 1 comment, addressed in c8c050b

core/auth/providers.ts — "Inferring provenance from client_id can mislabel a genuine dynamic registration as CIMD" — accepted, changed. (inline reply)

RFC 7591 §3.2 does leave a dynamically issued client_id opaque, so equality with the metadata URL is not proof on its own.

The mechanism cannot be carried from the SDK call site as suggested: SDK v2's OAuthClientProvider.saveClientInformation contract is (clientInformation, ctx?: OAuthClientInformationContext), and ctx holds only issuer — auth() passes the same infoCtx from all three of its call sites, so there is nothing to read.

What is available is a registration we recorded ourselves, so the rule is now a conjunction rather than an inference from client_id:

1. CIMD is configured for this connection right now, and
2. the incoming client_id is exactly that metadata-document URL, and
3. the registration already stored under that same client_id is recorded as cimd — written by ensureCimdClientRegistration, which reaches that line only after confirming the AS advertises client_id_metadata_document_supported.

(3) is the decisive term and is a recorded fact, not a guess about what the AS returned. Both scenarios in the comment now fall through to dcr: an AS that returns the metadata URL from POST /register fails (3) because nothing was ever recorded as cimd under that id, and an AS that reuses a prior registration's id fails (3) when that prior registration was a DCR.

I also dropped the branch that preserved a stored cimd when clientMetadataUrl was no longer configured — it widened exposure for a case not worth it.

Tests reworked to match. Five negative cases now pin each way out to dcr: id is not the metadata URL, CIMD not configured, a different metadata URL configured, no stored CIMD registration, and a stored kind of dcr.

Removed guard	Result
the whole dispatch (: "dcr" again)	providers.test.ts 1 failed / 41 passed; inspectorClient-oauth-e2e.test.ts 2 failed (SSE + Streamable HTTP) / 32 passed

Also in this round: before/after screenshots of a live CIMD login are now in the PR body — same server, same Client ID, no registration request in either run, Dynamic (DCR) → Client ID Metadata (CIMD).

npm run local:gate passes (re-running against the freshly merged v2/main).

[Copilot]

🟡 Changes recommended

The unresolved multi-issuer CIMD provenance issue can still produce incorrect DCR classification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 3 — 1 comment, addressed in f3f68c7

core/auth/providers.ts — multi-issuer CIMD provenance still classified as DCR — confirmed and fixed. (inline reply)

The trace in the comment is exactly right:

1. ensureCimdClientRegistration writes the unkeyed slot { client_id: <metadata URL>, kind: "cimd" }.
2. The first issuer-stamped save reaches updateIssuerSlot, which promotes that entry into byIssuer[A], sets activeIssuer = A, and clears the top-level fallback.
3. On a connect that resolves to issuer B, ensureCimdClientRegistration calls provider.clientInformation() with no ctx — that read resolves through activeIssuer = A, returns issuer A's client, and early-returns without pre-registering for B.
4. SDK auth() then calls clientInformation({ issuer: B }), finds no byIssuer[B] and no unkeyed fallback, takes its URL-based-client-ID branch, and saves under B — where an issuer-scoped lookup found nothing and the old code stored dcr.

The lookup is no longer scoped to the incoming issuer. It tries [issuer, undefined] in order, where undefined resolves through activeIssuer to the registration the server is actually using (and, on the first binding, to the unkeyed slot the pre-registration wrote). Condition (3) is now "a registration recorded as cimd for this server under this same client_id" — the provenance question — rather than "under this issuer", which SEP-2352 makes the wrong question.

Round 2's narrowing is unaffected: an AS that mints its own client_id still fails condition (2), so issuer B registering dynamically is still dcr.

Both cases are covered by tests driving a real OAuthStorageBase rather than mocks, since the behaviour under test is how storage promotes and clears slots across issuers. The positive test asserts the precondition mid-sequence (getClientInformation(SERVER, false, ISSUER_B) is undefined after the A binding), so it would notice if storage stopped clearing the fallback and the case became unreachable.

Mutation-checked, each guard independently:

Reverted	Result
lookupKeys back to [issuer] (round 2's behaviour)	1 failed — the issuer-B case, alone
the whole dispatch back to : "dcr"	2 failed — the single-issuer case and the issuer-B case

Unit suite 44 passed, CIMD OAuth E2E 34 passed. A full npm run local:gate is running against this commit and I'll report it here.

[Copilot]

🟡 Changes recommended

CIMD provenance must be issuer-bound to avoid misclassifying a real cross-issuer DCR.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 4 — 1 comment, addressed in 4c6a9fb

core/auth/providers.ts — the active-issuer fallback can copy CIMD provenance onto a real cross-issuer DCR — confirmed and fixed at the source. (inline reply)

I fixed it at the source instead of in the resolver. ensureCimdClientRegistration now:

• runs discovery first, and records the registration with { registrationKind: "cimd", issuer: metadata.issuer } — bound to the AS it just confirmed advertises client_id_metadata_document_supported;
• moves its "do we already have a client?" check to after discovery and keys it by that issuer. Read ctx-less, as it was, it resolved through the active issuer and early-returned for every later issuer — which is precisely what forced the cross-issuer fallback last round.

With provenance issuer-bound at the point it is established, resolveSdkRegistrationKind goes back to a single issuer-scoped lookup with no fallback. Your scenario now resolves correctly on its own: issuer B does not advertise CIMD, so nothing is recorded for B, so a registerClient result under B is dcr — even when B mints the metadata URL as its client_id.

The read stays issuer-keyed rather than issuer-only: getClientInformation falls back to the unkeyed slot when no byIssuer entry exists, which is how a pre-registration written before an issuer was known is still found on the save that first binds one.

Covered by the case you asked for, plus its complement, driven against a real OAuthStorageBase and the real ensureCimdClientRegistration (mocked only at fetch, so discovery genuinely runs):

• records dcr when a second issuer without CIMD mints the same URL as its client_id — A=CIMD then B=DCR returning the same URL. Also asserts issuer A's own provenance is untouched.
• keeps cimd when a second CIMD-supporting issuer takes over — the round-3 case, now satisfied through the pre-registration rather than a lookup fallback.
• records dcr for a second issuer that mints its own client_id.

Mutation-checked, each guard independently:

Reverted	Result
reinstate round 3's [issuer, undefined] fallback	1 failed — records dcr when a second issuer without CIMD mints the same URL, exactly this finding
ensureCimdClientRegistration back to the ctx-less early-return	2 failed — the issuer-keyed no-op case and the second-CIMD-issuer case
drop the issuer from the pre-registration save	2 failed
the whole dispatch back to : "dcr"	4 failed

One trade-off worth calling out, since it is a deliberate cost and not an oversight: moving the existing-client check after discovery means a discovery round trip on each connect attempt rather than only the first. There is no cheaper way to learn the issuer, and SDK auth() performs the same discovery immediately afterwards. It is noted in a comment at the call site.

---

Note on the previous gate run

The local:gate run I reported mid-review came back red with 11 failures across ProtocolListPanel, ServerConfigModal, ServerSettingsModal and AppsScreen. Those are load flakes from two concurrent gate runs on this machine, not a regression: this PR touches no component files (git diff origin/v2/main...HEAD --name-only is three files, all under core/auth and src/test), and all 138 tests in those four suites pass on their own. A clean gate is running against 4c6a9fb and I will report it here.

[Copilot]

🟡 Changes recommended

Two moderate issues remain around invalid-client recovery and uncached discovery availability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

core/auth/providers.ts:298

• This contract comment is now inaccurate: ensureCimdClientRegistration is an internal caller that supplies both registrationKind and the discovered issuer. Describe the issuer as optional rather than saying internal callers never provide it.

    // per-AS keying) and no kind — `resolveSdkRegistrationKind` recovers it —
    // while our callers supply the registration kind and no issuer yet.

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Fixed in 4297ef8 — removed. The helper is now declared function discoveryFetch(issuer: string, cimd: boolean): typeof fetch and returns the async function directly, so a signature drift is a compile error rather than being cast away.

Agreed on principle too: this repo's AGENTS.md treats an unjustified as unknown as as a review finding in its own right. The five remaining ones in this file are pre-existing (window stubs and the makeStorage shape) and outside this change.

[cliffhall]

Review round 5 — 3 comments + 1 suppressed, all addressed

Code in 4297ef8, comment fix in 4350731.

1. core/auth/providers.ts — invalid-client recovery loses the provenance (inline)

Confirmed and fixed in 4297ef8. I traced the path: auth() catches InvalidClient/UnauthorizedClient, calls invalidateCredentials("client") then ("tokens"), and re-enters authInternal. Our invalidateCredentials("client") calls clearClientInformation(serverUrl) with no issuer, which clears every issuer slot's clientInformation and clientRegistrationKind. The retry's URL-based client-ID save then arrives with nothing recorded, and the old code stored dcr.

The fix is the marker you suggested. IssuerBoundOAuthState gains cimdClientMetadataUrl — the CIMD client-metadata URL that issuer was confirmed to accept as a client_id:

• It is not a credential, so clearClientInformation deliberately does not touch it. An invalid_client response says the credential was rejected; it says nothing about whether the AS advertises client_id_metadata_document_supported.
• It is issuer-keyed, so round 4's cross-issuer case still resolves correctly.
• It is refreshed on every connect, and withdrawn when the AS stops advertising CIMD — ensureCimdClientRegistration now writes undefined in that branch rather than just skipping. Since discovery runs each time, the marker cannot go stale.
• Written with setActive = false, since it is recorded during discovery, before anything has been authorized against that AS — it must not promote the issuer to activeIssuer.

resolveSdkRegistrationKind now reads the marker instead of the stored kind, which also makes it simpler: CIMD is configured, the client_id is that URL, and this issuer carries that marker.

Covered by the invalid-client recovery case you asked for, at both levels:

• keeps the CIMD marker through invalid-client credential invalidation — against real storage: asserts the credential and its kind are gone after invalidateCredentials("client"), the marker is not, and the SDK's retry save is then recorded as cimd. This is the one that pins the property, so it deliberately does not mock clearClientInformation.
• keeps cimd through invalid-client recovery, which clears the credential — the same sequence at the provider level.

Mutation-checked:

Reverted	Result
resolver reads the clearable clientRegistrationKind again	2 failed — including the invalid-client case
make clearClientInformation clear the marker too	1 failed — the real-storage recovery test
ensureCimdClientRegistration stops withdrawing the marker	2 failed

Worth noting the first version of this test passed against a clearable marker, because it mocked clearClientInformation. That is why the real-storage test exists.

2. core/auth/cimd.ts — discovery became a hard network dependency (inline)

Right, and this was a regression I introduced last round rather than an inherent cost — thanks. Fixed in 4297ef8, both halves:

Reuse the cached discovery. ensureCimdClientRegistration now reads provider.discoveryState() first and uses its authorizationServerMetadata when present, so it does no discovery of its own on the path where auth() would also skip it. Both legs (RFC 9728 and RFC 8414) are behind that check.

A discovery failure is no longer fatal. The AS-metadata leg is wrapped, and on failure the helper returns instead of propagating. This is the right shape regardless of caching: pre-registration is an optimization over what SDK auth() does for itself, so failing to pre-register must never fail a connection — auth() runs its own discovery immediately afterwards and owns the error handling. It also writes no marker in that case, since nothing was learned about the AS.

Two tests:

• reuses persisted discovery state instead of re-fetching — seeds discovery state and passes a fetchFn that throws if called; asserts it is never called and the pre-registration still happens.
• skips pre-registration when discovery fails, rather than throwing — asserts it resolves, saves no client information, and invents no marker.

Mutation-checked: dropping the cached-state read fails the first; removing the catch fails the second.

3. providers.test.ts — unjustified double cast (inline)

Fixed in 4297ef8 — removed. The helper is now declared function discoveryFetch(issuer: string, cimd: boolean): typeof fetch and returns the async function directly, so a signature drift is a compile error rather than being cast away.

Agreed on principle too: this repo's AGENTS.md treats an unjustified as unknown as as a review finding in its own right. The five remaining ones in this file are pre-existing (window stubs and the makeStorage shape) and outside this change.

4. Suppressed comment — stale saveClientInformation contract comment

Correct, and a staleness my round-4 change introduced: ensureCimdClientRegistration began supplying the discovered issuer alongside the kind, so "our callers supply the registration kind and no issuer yet" stopped being true. Reworded in 4350731 to say the issuer is supplied when known, and that the unkeyed slot is only for AS metadata carrying no issuer at all.

[Copilot]

🟢 Approval recommended

The reviewed changes have comprehensive regression coverage and no unresolved issues.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Approval assessments conflict, and the PR description remains inconsistent with the implemented algorithm.

Review details

Suppressed comments (1)

core/auth/providers.ts:397

• The PR description’s stated safety rule is now stale: it says a prior registration recorded as cimd is required and that “no prior CIMD registration” falls through to dcr, but this new-registration branch intentionally returns cimd with no stored registration by consulting SDK-persisted discovery state. That distinction is central to the fix (including the transient-preflight-failure path), so please update the description’s Fix section to describe the actual two-case algorithm rather than the superseded conjunction.

    // A new registration: read back the branch the SDK took.
    const discovery = await this.storage.getDiscoveryState(this.serverUrl);
    const metadata = discovery?.authorizationServerMetadata;
    // Require the metadata to describe *this* issuer, so a state left over from
    // a previously resolved AS cannot answer for a different one.
    if (issuer !== undefined && metadata?.issuer !== issuer) return "dcr";
    return metadata?.client_id_metadata_document_supported === true
      ? "cimd"
      : "dcr";

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review round 8 — approval, 0 comments · round 9 — description staleness, fixed

Round 9's suppressed comment is correct and I've fixed it. The PR description still carried the round-2 conjunction — "a prior registration recorded as cimd is required", "no prior CIMD registration falls through to dcr" — which 6273790 superseded. That is exactly backwards for the new-registration branch, which does return cimd with nothing stored, by reading the SDK's persisted discovery state. Since that branch is what makes the transient-preflight-failure path work, describing it as the old conjunction would have made the description actively misleading rather than merely out of date.

The Fix section now describes the real algorithm: an entry condition (CIMD configured, client_id equal to the metadata URL) and then two cases, back-stamp → recorded kind, new registration → persisted discovery state guarded on issuer. The four consequences are listed as consequences of those two cases rather than as separate rules. The Tests section now carries the full eight-row mutation table rather than the two-row one from round 2.

On the conflicting approval assessments: round 8 was 🟢 Approval recommended, 0 comments against this same tree, and round 9 raised no code finding — only the description. No code changed between them, and none changed for this fix either; 6273790 is still the head.

[Copilot]

🟢 Approval recommended

The reviewed changes have comprehensive coverage and no unresolved issues.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Gate green on 6273790

npm run local:gate passes end to end against the current head — the full superset of CI, finishing on Storybook (123 files, 525 tests).

This supersedes the red run I reported mid-review: that one was 11 failures across ProtocolListPanel, ServerConfigModal, ServerSettingsModal and AppsScreen, caused by two concurrent gate runs on the same machine sharing a log file — the older run was executing the newer test tree. This PR touches no component files, and all 138 tests in those four suites passed on their own.

Screenshots recaptured against the final commit, since the mechanism changed substantially after they were first taken. Same result: Client registration → Client ID Metadata (CIMD), Client ID the metadata-document URL, and zero requests to a registration endpoint in the run.

[Copilot]

🔵 Needs a closer look

Complex OAuth storage behavior and a minor E2E coverage gap warrant final human review.

Review details

Suppressed comments (1)

clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts:380

• This E2E assertion no longer exercises resolveSdkRegistrationKind: the updated preflight saves directly into the issuer-keyed slot, OAuthStorageBase reattaches that issuer on read, and SDK 2.0 only invokes saveClientInformation for an unstamped or newly created credential. The assertion therefore remains cimd even if the new resolver always returns dcr. Add an E2E case where preflight discovery fails once but the SDK's own discovery succeeds (or seed an unstamped legacy registration), so the SDK actually calls the resolver and the reported relabeling regression is covered across the integration boundary.

        // #2242: the metadata-document URL is the client_id, and the stored
        // provenance still says CIMD after the SDK bound the registration to
        // the issuer — no `POST /register` ever happened.
        const oauthState = await client.getOAuthState();
        expect(oauthState?.client).toMatchObject({
          clientId: metadataUrl,
          registrationKind: "cimd",

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review round 11 — suppressed comment, and it was right. Fixed in be08f6d

The CIMD E2E assertion had gone vacuous, and I confirmed it empirically before fixing it: reverting the dispatch to : "dcr" left all 34 tests passing. So the "34 passed / 2 failed when reverted" evidence I cited in earlier rounds was true when I first measured it and had quietly stopped being true — the assertion stopped covering the resolver the moment the pre-registration started writing the issuer-keyed slot itself (round 4). The SDK then finds an already-stamped credential, clientInformation.issuer is defined, and it never calls saveClientInformation at all.

I took the second of your two suggestions — seeding an unstamped legacy registration — because it is not just a way onto the code path, it is the exact shape #2242 was reported against: an unkeyed CIMD registration, which is what every pre-SEP-2352 install has on disk and what the pre-registration itself wrote before it knew the issuer. The SDK back-stamps it with the issuer, and that back-stamp is the save that used to relabel it Dynamic (DCR).

New case: keeps CIMD provenance when the SDK issuer-stamps an unkeyed registration. It seeds saveClientInformation(mcpUrl, { client_id: metadataUrl }, { registrationKind: "cimd" }) with no issuer, completes a real authorization against a CIMD-advertising test AS, and asserts getOAuthState().client still reports registrationKind: "cimd".

	With the fix	Dispatch reverted to : "dcr"
inspectorClient-oauth-e2e.test.ts	36 passed	2 failed — the new case, on SSE and Streamable HTTP

That is the reported regression reproduced across the integration boundary. I also left a comment on the original assertion saying what it does and does not cover, so it is not mistaken for resolver coverage again.

A full local:gate is running against be08f6d and I will report it here.

[Copilot]

🟡 Changes recommended

The EMA wrapper must delegate issuer context and discovery-state methods, with coverage for that path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Review round 12 — 1 comment, addressed in aca522d

core/auth/providers.ts — the discovery-state fallback is unavailable through EmaTransportOAuthProvider — confirmed and fixed. (inline reply)

Confirmed and fixed in aca522d. I read the wrapper: clientInformation() and saveClientInformation() took no ctx parameter at all, and there was no discoveryState / saveDiscoveryState — while clientMetadataUrl is forwarded, so the SDK's URL-based-client-ID branch is genuinely reachable over EMA. The inner provider therefore saw issuer === undefined with nothing to read back, and the CIMD write was recorded as DCR.

Both are now delegated. Worth noting neither is only a #2242 concern — they were pre-existing SEP-2352 gaps that this fix happened to surface:

• Dropping ctx put every EMA client-information read and write on the unkeyed slot, so EMA credentials were never issuer-keyed at all.
• The missing discovery state meant the SDK re-discovered on every auth() call and could not perform its callback-leg authorization-server binding check — it warns about exactly this when a provider does not implement the pair.

Coverage, and the existing delegation test updated for the new arity:

• forwards the SDK issuer context on client-information reads and writes — asserts ctx reaches the inner provider on both.
• delegates discovery state to the inner provider — both directions.

Mutation-checked:

Reverted	Result
wrapper drops ctx again	2 failed
remove the discovery-state delegation	1 failed

Auth suites 629 passed. A full local:gate is running against aca522d and I will report it.

[Copilot]

🟢 Approval recommended

The reviewed changes have comprehensive coverage and no unresolved issues.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The reviewed changes have comprehensive coverage and no unresolved issues.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Review complete — two consecutive clean rounds, gate green on aca522d

Rounds 13 and 14 both returned 🟢 Approval recommended, 0 comments and 0 suppressed comments, against the same tree. npm run local:gate passes end to end on aca522d (finishing on Storybook, 123 files / 525 tests).

What the review changed

Six rounds raised code findings, and every one of them was real. The mechanism changed twice as a result:

Round	Finding	Outcome
2	client_id equality can mislabel a real DCR (RFC 7591 §3.2)	Narrowed to a conjunction requiring recorded provenance
3	Issuer-scoped lookup breaks the issuer-A → issuer-B CIMD flow	Bound the pre-registration to the issuer it discovered
4	The active-issuer fallback copies A's provenance onto B's real DCR	Fixed at the source; resolver back to issuer-scoped
5	invalid_client recovery clears the provenance; discovery became a hard network dependency; a double cast	Added an issuer-keyed marker; reused cached discovery; cast removed
6	The marker was written on AS support alone, relabeling an existing DCR	Made the marker earned rather than assumed
7	A transient preflight failure still recreated the bug; existing / existingKind could describe different registrations	Removed the marker. Read the SDK's own persisted discovery state instead
11	The CIMD E2E assertion had gone vacuous	Added the unkeyed-registration case, which fails on both transports when reverted
12	The EMA wrapper drops ctx and has no discovery-state methods	Delegated both; two pre-existing SEP-2352 gaps closed

Round 7 is the one worth flagging: taken with rounds 5 and 6 it said the marker was the wrong mechanism rather than a mechanism needing another patch. auth() persists the authorization-server metadata before it touches client information, and branches to its URL-based client ID on exactly the predicate that metadata carries — so the answer is read back from the SDK's own state rather than shadowed in a marker of our own. That deleted a storage field, two storage accessors, two provider methods, and three classes of finding at once.

Round 11 deserves the same note in the other direction: an assertion I had cited as evidence in earlier rounds had silently stopped covering the resolver, and I only found that out by re-running the mutation. Worth remembering that a mutation result has a shelf life.

Verification

Every guard is mutation-checked independently — twelve reverts, each failing only the tests that own it. The end-to-end reproduction is keeps CIMD provenance when the SDK issuer-stamps an unkeyed registration, which fails on SSE and Streamable HTTP without the fix.

Screenshots in the description were recaptured against the final code: Client registration → Client ID Metadata (CIMD), and no request to a registration endpoint in the run.