Merge upstream DO-reset retry and make the portal connector recoverable - #3
Merged
Conversation
…per's OAuth
GatekeeperUserImpl inherited McpGatekeeperUserBase's reconnect(), which only
restarts this Worker's own OAuth with Cloudflare Access. That never touches
the portal's on-behalf authorization to an upstream server, so when it lapses
("Server ... requires re-authentication. Call the portal_toggle_servers tool
to re-authenticate.") the user has no way to recover: Cloudflare's MCP Server
Portals expose portal_toggle_servers as the only re-authentication path, with
no dashboard alternative, and nothing but this gatekeeper can call it.
Override reconnect() to call portal_toggle_servers itself and return the
re-authentication URL it opens. The response shape is undocumented beyond
"opens a URL-based server selection page", so the URL is recovered with the
same permissive prose scan portal_list_servers already uses
(parsePortalReconnectUrl), rather than trusting a guessed structured field.
When no URL can be recovered, this throws instead of falling back to the
inherited flow -- a silent fallback would reproduce exactly the bug being
fixed, since the user would reconnect and the upstream authorization would
still be untouched. The unparsed response is logged for later inspection but
kept out of the thrown message, since its shape isn't verified.
Adds a vitest.config.ts + cloudflare:workers stub for this package (mirroring
mcp-shared's) so GatekeeperUserImpl can be instantiated directly in tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracts the portal binding-mint validation and the configurator's server
listing out of their call sites, converts McpClient.#list from five
positional parameters to an options object, inlines handleOAuthCallback into
its only consumer, and prunes the mcp-shared exports map to the subpaths
that are actually imported.
None of it changes behaviour. The #list conversion preserves every call
site's effective arguments, including listMatchingToolIndex leaving
requireCompleteScan defaulted false, which client-pagination.test.ts pins on
both sides. handleOAuthCallback moved verbatim; it had one caller, and
http.ts already declared the OAuthCallbackAccount shape it needed, so the
inlining drops a duplicated type rather than adding one.
validatePortalScope owns the whole scope pipeline -- parse, policy check,
catalog evidence -- rather than taking an already-narrowed scope. Splitting
it the other way would have left requirePortalServerScope as a precondition
the type could not express: ToolScope & { serverId: string } proves serverId
is present, not that the portal-native tool refusal ran, so a second caller
could satisfy the signature having skipped the check that stops a grant for
portal_toggle_servers. Returning the narrowed scope is what lets the caller
keep its own narrowing, since an assertion signature is not available on an
async function.
The evidence rationale moves onto that function instead of being dropped
with the lines it annotated. Both halves are load-bearing: the first is the
only thing explaining why the listing == null branch probes for the
server-list tool rather than failing outright, and the second is the
justification for the one mode that validates a grant against an empty
catalog.
The local catalog is typed ToolIndex rather than ToolCatalog. Every branch
produces name-only evidence, and ToolCatalog advertises annotations -- the
field tools.ts classifies read-versus-approval from, and which nothing
outside tools.ts is allowed to read. It is assignable to the ToolCatalog
parameter either way.
Removing the six package-private modules from exports only bites if the
boundary is enforced, and it was not: both connectors aliased
@gadgets/mcp-shared/* to ../mcp-shared/src/*, which TypeScript consults
ahead of exports and esbuild honours too, so an import of a pruned subpath
would have type-checked and bundled regardless. The aliases are gone; the
set of subpaths imported anywhere is a subset of what remains exported, so
resolution now goes through the exports map in both tsc and wrangler.
project is declared as a function property rather than a method, since
method syntax is exempt from strictFunctionTypes' contravariance check and
this is the parameter deciding what is retained from an untrusted
tools/list response.
* Rename do-telemetry to do-retry ahead of adding retries Pure rename plus the four references, split out so the retry commit's diff shows the file as a move rather than a delete/create pair -- the content change is large enough that git's default 50% similarity threshold no longer detects it. * Retry pure user-DO reads once across a reset user_do.reset.surfaced volume (from cloudflare#133/cloudflare#162, which deliberately deferred retries) showed the one call in flight at reset time is worth recovering instead of surfacing to the browser. do-retry.ts gains retryOnDoReset(callWithFreshStub, log): exactly one retry, full jitter (a mass reset fails every in-flight call at once), and identity rethrow so the workerd flags the frontend classifier reads survive. The retry predicate is narrower than isDoResetError: durableObjectReset retries even when flagged overloaded (the incarnation is dead, so the queue that overloaded it died with it -- this is the shape production storage-timeout resets arrive in, {remote, overloaded, durableObjectReset}), while bare retryable retries only if the object isn't shedding load. The module header now spells out why those are two independent flag axes -- retryable/overloaded come from the kj exception type and describe the call, durableObjectReset is parsed from the tunneled description and describes the object -- and links the DO error-handling docs, which document the former pair but not the latter, and whose never-retry-overloaded guidance we deliberately diverge from. The replay-safety judgment lives at each call site as an explicit, greppable wrapper -- no method-name allowlist. A reset can't distinguish "never applied" from "applied, response lost", so only strictly pure reads are wrapped: the 14 read delegations in server.ts and the read sites on the fresh-stub session getters in overseer.ts (whoami, getChatContext, listModels, plus the two OverseerImpl-internal fresh-stub reads). The thunk re-evaluates the fresh-stub getter, so the second attempt reaches the new incarnation for free. Reads with hidden writes (listOutputs, listProvidedAccounts) and every write keep today's behavior; getChatContext's doc now pins that it must stay write-free. Successful retries log user_do.reset.recovered; correlated against user_do.reset.surfaced (which still fires per attempt, with operation attribution) this gives the retry win rate and the signal for wrapping more sites later.
…/upstream-bump-3 # Conflicts: # packages/gatekeeper-mcp-portal/src/portal.ts
gatekeepers.tsx only showed Reconnect when state === 'expired', which is driven by account.credentialsValid -- this gatekeeper's own OAuth with Cloudflare Access. The MCP Server Portals connector's on-behalf authorization to an upstream server (see reconnect() in gatekeeper-mcp-portal/src/portal.ts) is a separate credential this gatekeeper's credentialsValid can't observe, so it can lapse while the account still reads as connected here, leaving the user with no way to reach the portal's own re-authentication. Add alwaysOfferReconnect, keyed off the mcp_portal vendor id, to both the connector card (a plain, non-danger Reconnect button alongside the existing connected state, rather than reusing the expired styling) and the manage dialog's footer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
I have read the CLA Document and I hereby sign the CLA 0 out of 2 committers have signed the CLA. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
3 つをまとめて main に入れる。すべてデプロイ済みで本番確認済み。
1. upstream 2 コミットの取り込み
dd2b015 Retry pure user-DO reads once across a reset (#263)— capability 付与中に User DO のリセットと競合してNetwork connection lost./user_do.reset.surfacedで落ちていた問題への対処。純読み取りの DO RPC がリセットをまたいで 1 回リトライされる4288713 Simplify MCP discovery flows (#269)2.
fix/portal-reconnectのマージportal コネクタの
reconnect()をオーバーライドし、継承元（自身の Cloudflare Access OAuth をやり直すだけ）の代わりにportal_toggle_serversを呼んで Portal 自身の再認証ページの URL を返す。Portal → upstream の on-behalf OAuth が失効したとき（Server "Metabase" requires re-authentication）の唯一の回復経路で、Cloudflare はダッシュボードに代替手段を提供していない。cloudflare#269 が
portal.tsを大きく変えたため conflict したが、新構造の上でreconnectPortalの目的（URL 返却 / 取れなければレスポンスをログに出して throw / 継承実装へフォールバックしない）を維持して解消した。3. Reconnect ボタンの常時表示（portal コネクタのみ）
Reconnect ボタンは
credentialsValidが false のときだけ表示されていたが、このフラグはこの deployment 自身の Cloudflare Access OAuth の状態しか追跡しない。Portal → upstream の on-behalf 失効は workshop から観測できないため、実際に本番で「表示は正常・実態は失効・復帰手段なし」になった。MCP Server Portals コネクタのアカウントには常時 Reconnect を表示する。確認
getGatekeeperClassForは 125 秒 hang → 76ms / 72ms に改善（Add bounded progressive MCP discovery cloudflare/cloudflare-os#169/Support large MCP portal catalogs cloudflare/cloudflare-os#170 由来、前回 bump 分）@gadgets/typed-storage解決失敗で collection error（既知の環境要因、変更と無関係）🤖 Generated with Claude Code