Support large MCP portal catalogs

[dcartertwo]

Depends on #169.

An MCP Portal flattens many upstream servers into one tool catalog, which may be too large to retain safely. The configurator trusts the portal’s bounded server list first, falls back to a compact global index only when that list is incomplete, and fetches one bounded summary list for the selected server. Every grant names exactly one upstream server, optionally narrowed to specific tools.

Portal-native management tools are never grantable. Truncated or malformed discovery fails closed, local scope errors are rejected before remote scans, and all catalog work is bounded.

The endpoint must expose direct tools. Deployments where Code Mode is default-on can opt this client out with ?codemode=off; enforced Code Mode is unsupported.

[ask-bonk]

Signature matches. The classification-from-index design correctly resolves the security defect the tests describe (labels derived from annotations, which the index carries, not from whether the detailed fetch reached the tool).

I've completed a thorough review. The change is well-designed and the kernel-adjacent security boundaries hold. Let me summarize findings. I found no actionable defects — the concerns I investigated (truncation fail-safe in the mint path, bounded requestedTools.size, classification-source correctness, dangling imports, auth-transition handling) all check out.

LGTM!

Summary of what I verified:

• Mint path (getGatekeeperClassFor) — the security-critical resource minting in portal.ts:373-406. Named-tool grants are validated with listMatchingToolIndex(requestedTools.size, ...); requestedTools.size is bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard, and any truncation (byte budget or scan limit) causes validateToolScopeAgainstCatalog to throw rather than silently mint an under-validated grant. Server-wide grants for unreported servers require one prefixed-tool proof, and an empty/truncated result fails closed. The listedServers === null branch still proves portal capability via findTool before minting.
• Classification correctness — toolGrantOptions derives read-only/needs approval labels from the bounded index's annotations, never from whether the detailed catalog reached the tool. This is the defect the new tests pin down, and it's handled correctly (including vetted/byo parity — the form never claims auto-approval).
• Auth handling — portalAuthRequiresReconnect's new asymmetry (oauth-configured may prove public; explicit none stays strict) matches the updated tests. #configurationRevision/invalidateConnectionState correctly detect token rotation and clear captured credentials/session state. The allowsOAuthCallback gate correctly drops pending auth state when config no longer permits it.
• No dangling references — removed helpers (fetchPortalServers, scopeAllows, classifyTool, requireCompleteCatalogForToolSelection) survive only in prose comments; all newly-referenced shared exports exist with matching signatures.
• Configurator UX — the new "empty" endpoint state and ?codemode=off guidance are consistent between .d.ts, the UI, and the updated tests.

I could not run pnpm build/types:check because dependencies aren't installed in this read-only environment, but static review of all cross-package references was consistent.

github run

[ask-bonk]

All signatures match. I've completed a thorough review. Let me consolidate my findings.

Assessment

The change is well-designed and the kernel-adjacent security boundaries hold. I verified:

Mint path (getGatekeeperClassFor, portal.ts:392-430) — the security-critical resource minting:

• Named-tool grants: requestedTools.size is bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard; listMatchingToolIndex(size, filter) collects only matching entries and validateToolScopeAgainstCatalog throws on any absent tool (and specifically on truncated when a name is missing), so truncation fails closed rather than silently minting.
• Reported server-wide grants use { tools: [], truncated: false } and resolve via reportedServers.
• Unreported server-wide grants require one prefixed-tool proof, and an empty result throws "absent from catalog" — fail-closed.
• The listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL) before minting.

Classification correctness — toolGrantOptions derives read-only/needs approval from the bounded index's annotations via classifyAnnotations, never from whether the detailed catalog reached the tool. This is exactly the defect the new tests pin down (with vetted/byo parity — the form never claims auto-approval). Signatures match (IndexedTool.annotations, classifyAnnotations(annotations, trust)).

Auth transitions (account.ts) — the new allowsOAuthFallback/allowsOAuthCallback hooks default open in the base, and the portal narrows them (auth !== "none"; oauth-config + same-endpoint). Both new throw/return paths call restoreSelection/clear pending state consistently with surrounding code. portalAuthRequiresReconnect's new asymmetry (oauth-configured may prove public; explicit none stays strict) matches the updated tests. getConnection's revision check advances generation before super.getConnection re-reads it — consistent.

No dangling references — all newly-referenced shared exports (listMatchingToolIndex, fetchToolIndex, IndexedTool, ToolIndex, isValidToolName, toolBelongsToServer, hexEncode, classifyAnnotations, MAX_TOOLS_PER_SERVER) exist with matching signatures. Removed helpers survive only in prose.

Configurator UX — the new "empty" endpoint state and ?codemode=off guidance are consistent across .d.ts, the UI, and tests. The "up to 200 shown" copy matches the MAX_TOOLS_PER_SERVER slice in toolGrantOptions.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static review of all cross-package references and the test expectations was consistent. I found no actionable defects.

LGTM!

github run

[ask-bonk]

The change from scope.serverId! to scope.serverId (line 459) is now sound because requirePortalServerScope(scope) was upgraded to an assertion function (asserts scope is ToolScope & { serverId: string }), narrowing scope.serverId to string. This is a nice cleanup that removes a non-null assertion by making the guard's contract explicit in the type system. Good.

I've completed a thorough review of this PR.

Assessment

This is a well-designed change. I reviewed the security-critical mint path, the shared-library trust boundary, the concurrency guards, and cross-package consistency. The prior ask-bonk reviews were accurate; my independent verification confirms their findings and adds a few points.

Mint path (getGatekeeperClassFor, portal.ts:416-460) — fails closed in every branch:

• Named-tool grants: requestedTools.size is bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard. listMatchingToolIndex(size, filter) collects matching entries with stopWhenFull=true; a present tool set returns truncated: true but with all names, so validateToolScopeAgainstCatalog never trips the truncation check. A missing name yields either "truncated → cannot validate" or "absent" — never a silent mint.
• Reported server-wide grants resolve via reportedServers with { tools: [], truncated: false }.
• Unreported server-wide grants require one prefixed-tool proof; an empty/truncated result throws "absent from the current portal catalog."
• The listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL) before minting.

Type soundness — passing a ToolIndex where ToolCatalog is expected type-checks because IndexedTool ({ name; annotations? }) is structurally assignable to McpTool (only name required). requirePortalServerScope was correctly upgraded to an asserts function, which is what legitimizes dropping the scope.serverId! non-null assertion at portal.ts:459.

Classification correctness — toolGrantOptions derives read/approval labels from the bounded index's annotations via classifyAnnotations, never from whether the detailed catalog reached the tool. The index preserves exactly the four booleans (clampAnnotations) that the runtime classifyTool path also consumes, so the picker label and the enforced classification agree. This is precisely the defect the new tests pin, including vetted/byo parity.

Auth transitions — allowsOAuthFallback/allowsOAuthCallback default open in the base and are narrowed by the portal; both new paths clear pending state consistently. The portalAuthRequiresReconnect asymmetry matches the updated tests across all relevant cases. The #connectingRevisions guard is in-memory per-DO-instance state, which is correct for DO single-threaded request interleaving; the comment accurately scopes its purpose.

No dangling references — helpers dropped from the connector (fetchPortalServers, requireCompleteCatalogForToolSelection, classifyTool, scopeAllows) remain legitimately used in mcp-shared (facet/catalog/scope) and in gatekeeper-mcp; they are not dead code. All newly imported shared exports exist with matching signatures.

Configurator UX — the new "empty" vs "unavailable" states, ?codemode=off guidance, and "up to 200 shown" copy are consistent across the .d.ts, UI, README, and tests.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of all cross-package references, type assignability, and the mint-path fail-closed logic was consistent. I found no actionable defects.

LGTM!

github run

[ask-bonk]

MCP_PORTAL_TOKEN matches. The #configurationRevision correctly incorporates the token only when config.auth === "token", which is why a token rotation changes the digest and forces session invalidation via getConnection.

My independent review is complete. I verified the security-critical mint path, the classification-source invariant, the auth-transition logic, the concurrency guards, and cross-package consistency. All boundaries hold and every branch fails closed.

I could not run pnpm build/types:check/pnpm test (read-only environment with no installed dependencies), but static verification of type assignability (IndexedTool structurally satisfies McpTool's name-only requirement where a ToolCatalog/ToolIndex is passed to validateToolScopeAgainstCatalog), the asserts upgrade to requirePortalServerScope legitimizing the dropped scope.serverId!, and all newly-imported shared exports (listMatchingToolIndex, fetchToolIndex, IndexedTool, ToolIndex, isValidToolName, toolBelongsToServer, hexEncode, classifyAnnotations, MAX_TOOLS_PER_SERVER, toolGrantOptions, PortalConfig, PortalServerListing, McpConnection) was consistent.

Key findings from my verification:

• Mint path fails closed in every branch. Named-tool grants use listMatchingToolIndex(requestedTools.size, filter) (bounded ≤200 by parseToolScope); a requested name absent from the endpoint either trips the "truncated → cannot validate" or the "absent from catalog" throw — never a silent mint. Reported server-wide grants resolve via reportedServers with {tools: [], truncated: false}; unreported ones require one prefixed-tool proof; the listedServers === null path still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).
• Classification correctness (the defect the tests pin). toolGrantOptions derives read-only/needs approval from the bounded index annotations via classifyAnnotations, never from whether described reached the tool. Runtime enforcement uses classifyTool, which delegates to the same classifyAnnotations; both preserve the identical four clampAnnotations booleans, so the picker label and enforced behavior agree.
• Auth transitions. portalAuthRequiresReconnect's new asymmetry (oauth-configured may prove public; explicit none stays strict) matches the updated tests. #configurationRevision folds in MCP_PORTAL_TOKEN only for token auth, so a rotation forces invalidateConnectionState in getConnection; the #connectingRevisions in-memory guard correctly avoids invalidating a concurrent same-revision reconnect.
• No dangling references. Removed connector helpers (fetchPortalServers, requireCompleteCatalogForToolSelection, classifyTool, scopeAllows) remain legitimately used in mcp-shared/gatekeeper-mcp, not dead code.
• Configurator UX. The new "empty" vs "unavailable" states, ?codemode=off guidance, and "up to 200 shown" copy are consistent across the .d.ts, UI, README, and tests.

LGTM!

github run

[ask-bonk]

isValidToolName just bounds a string to non-empty and ≤ max chars. Used here as a general bounded-string guard for serverId (which is also a tool-name-prefix). It's a reasonable reuse (a server id is a prefix of a valid tool name), primarily to reject undefined/empty and cap length before it's used as a map key. Acceptable, though a serverId-specific name would read better. Not a defect.

I've completed a thorough review. Let me summarize my assessment. The prior ask-bonk reviews are accurate and this is my independent verification.

The change is well-designed. I verified the security-critical paths:

Mint path (getGatekeeperClassFor, portal.ts:417-461) — fails closed in every branch:

• Named-tool grants use listMatchingToolIndex(requestedTools.size, filter), bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard. When all names are present, #list returns truncated: true but with every name, so validateToolScopeAgainstCatalog (scope.ts:155-166) never trips the truncation check; a missing name either hits "truncated → cannot validate" or "absent" — never a silent mint. Cross-server names are additionally rejected by toolBelongsToServer (scope.ts:157).
• Reported server-wide grants resolve via reportedServers with {tools:[], truncated:false}; unreported ones require one prefixed-tool proof; the listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).
• The scope.tools = [] (pinned-empty) case mints a zero-tool grant on a reported server and is refused on an unreported one — fail-closed either way.

Classification correctness — toolGrantOptions (config.ts) derives read-only/needs approval from the bounded index's annotations via classifyAnnotations, never from whether the detailed catalog reached the tool. The index now carries exactly the four clampAnnotations booleans that runtime classifyTool also consumes, so picker label and enforced behavior agree. This is precisely the defect the new tests pin, with vetted/byo parity.

Auth transitions — allowsOAuthFallback/allowsOAuthCallback default open in the base and are narrowed by the portal; both new paths clear pending state consistently. portalAuthRequiresReconnect's new asymmetry matches the updated tests. #configurationRevision folds MCP_PORTAL_TOKEN in only for token auth. The #connectingRevisions in-memory per-DO guard correctly avoids invalidating a concurrent same-revision reconnect.

Configurator bounds — #toolIndex is capped at MAX_PORTAL_TOOL_INDEX; #tools bounds both concurrent loads (MAX_CONFIGURATOR_TOOL_CATALOGS) and retained catalogs; listToolOptions only allocates a fetch for a server id the bounded survey observed, preventing an arbitrary RPC argument from triggering a full paginated scan. isReady (!== "portal") and the "empty"/"unavailable" split are fail-closed and consistent across .d.ts, UI, and tests.

No dangling references — removed helpers (fetchPortalServers, requireCompleteCatalogForToolSelection, scopeAllows, classifyTool) survive only in prose; all newly-imported shared exports exist with matching signatures.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of type assignability, the asserts upgrade to requirePortalServerScope, and all cross-package references was consistent. I found no actionable defects.

LGTM!

github run

[ask-bonk]

The README is thorough and consistent with the implementation. I've completed my review.

I performed an independent verification of the security-critical paths rather than relying on the prior ask-bonk reviews:

Mint path (getGatekeeperClassFor, portal.ts:427-462) — fails closed in every branch:

• Named-tool grants call listMatchingToolIndex(requestedTools.size, filter) with stopWhenFull=true; requestedTools.size is bounded ≤200 by parseToolScope. When all requested names are present the survey returns truncated: true with every name, so validateToolScopeAgainstCatalog (scope.ts:167-178) only reaches the truncation throw when a name is actually missing — never a silent mint. A missing name yields either "truncated → cannot validate" or "absent". Verified against #list at client.ts:641-654.
• Reported server-wide grants resolve via reportedServers with {tools:[], truncated:false}; the pinned-empty (scope.tools=[], size-0) case still hits the survey branch and validates the server, fail-closed on an unreported server.
• The listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

Classification invariant — toolGrantOptions (config.ts) derives labels via classifyAnnotations(entry.annotations, trust) from the bounded index, and runtime classifyTool (tools.ts:101) delegates to the same classifyAnnotations. Both consume the identical four clampAnnotations booleans, so picker label and enforced behavior agree. This is exactly the defect the new config tests pin, including vetted/byo parity.

Type soundness — requirePortalServerScope upgraded to an asserts function legitimizes dropping scope.serverId!. IndexedTool ({name; annotations?}) is structurally assignable where ToolCatalog/McpTool is expected.

Auth transitions — portalAuthRequiresReconnect's new asymmetry matches the updated tests; #configurationRevision folds MCP_PORTAL_TOKEN in only for token auth; the #connectingRevisions in-memory guard correctly avoids invalidating a concurrent same-revision reconnect; allowsOAuthFallback/allowsOAuthCallback default open in the base and are narrowed by the portal, both clearing pending state consistently.

Configurator bounds — #toolIndex capped at MAX_PORTAL_TOOL_INDEX; #tools bounds concurrent loads and retained catalogs (MAX_CONFIGURATOR_TOOL_CATALOGS); listToolOptions only allocates a fetch for a server id the bounded survey observed, so an arbitrary RPC argument cannot trigger a full paginated scan.

No dangling references — removed helpers survive only in prose; all newly-imported shared exports (hexEncode, MAX_TOOLS_PER_SERVER, classifyAnnotations, toolBelongsToServer, listMatchingToolIndex, fetchToolIndex, IndexedTool, ToolIndex, isValidToolName) exist with matching signatures.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability, and all cross-package references was consistent. I found no actionable defects.

LGTM!

github run

[ask-bonk]

The README is thorough and consistent with the implementation.

I've completed a thorough independent review. Let me consolidate. I examined the security-critical mint path, the shared-library changes, the concurrency guards, and cross-package consistency, verifying against the actual source rather than relying on the prior reviews.

Mint path (getGatekeeperClassFor, portal.ts:427-461) — fails closed in every branch, confirmed against #list (client.ts:605-661) and validateToolScopeAgainstCatalog (scope.ts:150-179):

• Named-tool grants: requestedTools.size is bounded ≤200 by parseToolScope's guard (scope.ts:95). listMatchingToolIndex(size, filter, stopWhenFull=true) returns all present names with truncated:true; since names contains them all, the truncation branch is never reached, so all-present is a valid mint and any missing name hits "truncated → cannot validate" or "absent". The pinned-empty (scope.tools=[], size 0) case validates the server via reportedServers/prefix and mints zero tools, fail-closed on an unreported server.
• Reported server-wide grants resolve via reportedServers with {tools:[], truncated:false}; unreported ones require one prefixed-tool proof; the listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

Classification invariant — toolGrantOptions (config.ts:129) derives labels from classifyAnnotations(entry.annotations, ...) on the bounded index, never from described membership. indexTool now preserves the four clampAnnotations booleans, so picker label and runtime classifyTool agree. This is exactly the defect the new config tests pin, with vetted/byo parity.

Type soundness — requirePortalServerScope upgraded to an asserts function legitimizes dropping scope.serverId! at portal.ts:466. IndexedTool is structurally assignable where ToolCatalog is expected.

Fail-closed parsing — parsePortalServers now returns {servers, complete}; complete is false for absent/unrecognized/partly-malformed responses, and listServerOptions throws when a truncated index pairs with an incomplete listing. The empty-structuredContent-array → {complete:true, servers:[]} distinction is handled correctly.

Auth/concurrency — allowsOAuthFallback/allowsOAuthCallback default open in the base and are narrowed by the portal, clearing pending state consistently. portalAuthRequiresReconnect's new asymmetry matches the updated tests. #configurationRevision folds MCP_PORTAL_TOKEN in only for token auth; the #connectingRevisions in-memory per-DO guard (cleaned up in finally) correctly avoids invalidating a concurrent same-revision reconnect.

Configurator bounds — #toolIndex capped at MAX_PORTAL_TOOL_INDEX; #tools bounds concurrent loads and retained catalogs; listToolOptions only allocates a fetch for a server id the survey observed, and rejects invalid ids first.

No dangling references — removed helpers survive only in prose; all newly-imported shared exports exist with matching signatures. README and .d.ts state changes ("empty" vs "unavailable", ?codemode=off, "up to 200 shown") are consistent.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability, and all cross-package references was consistent. I found no actionable defects.

LGTM!

github run

[ask-bonk]

All exports resolve. listMatchingToolIndex/fetchToolIndex are methods/functions verified earlier. No dangling references in the connector.

Let me do one final verification: the getConnection override uses this.#connectingRevisions.has(revision). There is one subtle concern about whether getConnection could be called for a non-portal-matching endpoint before the revision check. Looking at the override: it throws early if !config || !server || !sameEndpoint || portalAuthRequiresReconnect. This mirrors the base's own protective checks. Fine.

I've thoroughly reviewed the security-critical mint path, the classification-source invariant, the fail-closed parsing, the auth transitions, the concurrency guard, the configurator bounds, and cross-package consistency. Let me confirm my findings against the actual behavior once more on the one area I flagged (OAuth revision recording) — it is benign since the oauth digest excludes the token and the legacy-account branch only invalidates for token auth.

I found no actionable defects. My independent verification confirms the prior review findings.

LGTM!

I performed an independent review of this PR (base feature/mcp-progressive-discovery) rather than relying on the prior ask-bonk comments, verifying against the actual source in mcp-shared and gatekeeper-mcp-portal.

Mint path (getGatekeeperClassFor, portal.ts) — fails closed in every branch, confirmed against #list (client.ts:600-660) and validateToolScopeAgainstCatalog (scope.ts):

• Named-tool grants call listMatchingToolIndex(requestedTools.size, filter) with stopWhenFull=true; requestedTools.size is bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard. When all names are present the survey returns truncated:true with every name, so validateToolScopeAgainstCatalog reaches its truncation throw only when a name is actually missing (or the byte budget cut it short) — never a silent mint. Cross-server names are additionally rejected by toolBelongsToServer.
• Reported server-wide grants resolve via reportedServers with {tools:[],truncated:false}; the pinned-empty (scope.tools=[], size-0) case validates the server and mints zero tools, fail-closed on an unreported server.
• Unreported server-wide grants require one prefixed-tool proof; the listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

Classification invariant — toolGrantOptions (config.ts) derives read/approval labels from the bounded index annotations via classifyAnnotations, never from described membership. indexTool now preserves exactly the four clampAnnotations booleans that runtime classifyTool also consumes, so picker label and enforced behavior agree. This is precisely the defect the new config tests pin, including vetted/byo parity.

Fail-closed parsing — parsePortalServers now returns {servers, complete}; complete is false for absent/unrecognized/partly-malformed replies (verified the - malformed bullet and structuredContent:[] cases against parseServerLine/parseStructured). listServerOptions throws when a truncated index pairs with an incomplete listing.

Type soundness — requirePortalServerScope upgraded to an asserts function legitimizes dropping scope.serverId!. IndexedTool is structurally assignable where ToolCatalog is expected.

Auth/concurrency — allowsOAuthFallback/allowsOAuthCallback default open in the base and are narrowed by the portal, both clearing pending state consistently. portalAuthRequiresReconnect's new asymmetry matches the updated tests. #configurationRevision folds MCP_PORTAL_TOKEN in only for token auth; the OAuth path never records a revision in beginConnect (it completes via acceptAuthCode), but this is benign because the oauth digest excludes the token and the legacy-account branch invalidates only for token auth. The #connectingRevisions in-memory per-DO guard is cleaned up in finally and correctly avoids invalidating a concurrent same-revision reconnect. The new authChanged clause in beginConnect correctly clears stale credentials on an auth-mode repoint.

Configurator bounds — #toolIndex capped at MAX_PORTAL_TOOL_INDEX; #tools bounds concurrent loads and retained catalogs (MAX_CONFIGURATOR_TOOL_CATALOGS); listToolOptions only allocates a fetch for a server id the bounded survey observed, so an arbitrary RPC argument cannot trigger a full paginated scan.

No dangling references — removed connector helpers (fetchPortalServers, requireCompleteCatalogForToolSelection, scopeAllows, classifyTool) survive only in prose; all newly-imported shared exports (isValidToolName, hexEncode, classifyAnnotations, MAX_TOOLS_PER_SERVER, toolBelongsToServer, listMatchingToolIndex, fetchToolIndex, IndexedTool, ToolIndex, McpConnection, PortalServerListing, PortalConfig) exist with matching signatures. README and .d.ts state changes ("empty" vs "unavailable", ?codemode=off, "up to 200 shown") are consistent.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability, and all cross-package references was consistent.

LGTM!

github run

[ask-bonk]

The tests thoroughly pin down the security-critical behavior, including the annotation-clamping test that verifies clampAnnotations drops non-boolean and unknown claims — an important defensive test since a server could otherwise inject a huge string into readOnlyHint.

I've completed a thorough independent review of this PR. Let me consolidate my findings.

I verified the change against the actual source rather than relying solely on the prior ask-bonk comments:

Mint path (getGatekeeperClassFor, portal.ts:417-461) — fails closed in every branch, confirmed against #list (client.ts:635-666) and validateToolScopeAgainstCatalog (scope.ts:150-179):

• Named-tool grants call listMatchingToolIndex(requestedTools.size, filter) with stopWhenFull=true; requestedTools.size is bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard (scope.ts:95). When all requested names are present the survey returns truncated:true with every name, so the truncation throw is reached only when a name is genuinely missing (or the byte budget cut it short) — never a silent mint.
• The pinned-empty case (params.has("tools") → scope.tools=[], requestedTools.size 0): listMatchingToolIndex(0, …) returns {tools:[], truncated:true}, and validation mints only if the server is reported or a prefixed tool proves it — fail-closed on an unreported server.
• Reported server-wide grants short-circuit to {tools:[], truncated:false}; unreported ones require one prefixed-tool proof; the listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

Classification invariant (the defect the tests pin). toolGrantOptions (config.ts) derives read/approval labels from the bounded index annotations via classifyAnnotations, never from described membership. indexTool now retains clampAnnotations(tool.annotations), and runtime classifyTool delegates to the same classifyAnnotations, so picker label and enforced behavior agree on the identical four booleans. The client-pagination test confirms clampAnnotations drops non-boolean/unknown claims (defends against an oversized readOnlyHint payload), and the config test covers vetted/byo parity.

Fail-closed parsing. parsePortalServers now returns {servers, complete}; complete is false for absent/unrecognized/partly-malformed replies. listServerOptions throws when a truncated index pairs with an incomplete listing.

Type soundness. requirePortalServerScope upgraded to an asserts function legitimizes dropping scope.serverId!. IndexedTool is structurally assignable where ToolCatalog is expected.

Auth/concurrency. portalAuthRequiresReconnect's new asymmetry matches the tests. #configurationRevision folds MCP_PORTAL_TOKEN in only for token auth (so OAuth digests are token-independent and the previous === undefined baseline never spuriously invalidates an OAuth account, even though the OAuth completion path in beginConnect returns a redirect rather than recording a revision — benign). The #connectingRevisions in-memory per-DO guard is cleaned up in finally and avoids invalidating a concurrent same-revision reconnect.

Configurator bounds. #toolIndex capped at MAX_PORTAL_TOOL_INDEX; #tools bounds concurrent loads and retained catalogs (MAX_CONFIGURATOR_TOOL_CATALOGS); listToolOptions allocates a fetch only for a server id the bounded survey observed, so an arbitrary RPC argument cannot trigger a full paginated scan. The "empty"/"unavailable" split and ?codemode=off guidance are consistent across the .d.ts, UI, and tests.

No dangling references. Removed connector helpers (fetchPortalServers, requireCompleteCatalogForToolSelection, scopeAllows, classifyTool) remain legitimately used in mcp-shared; all newly-imported shared exports exist with matching signatures.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability, and all cross-package references was consistent. I found no actionable defects.

LGTM!

github run

[Maximo-Guk]

GPT Findings:

1. Medium: Failed reconnect can leave a previously public account persisted as OAuth and unusable. packages/mcp-shared/src/account.ts:329-338
2. Medium: OAuth reconnect can be invalidated while awaiting its callback when an existing facet advances the connection generation. packages/gatekeeper-mcp-portal/src/portal.ts:298-352
3. Medium: Standards-shaped structuredContent suppresses valid textual fallback, breaking large-catalog discovery. packages/mcp-shared/src/portal.ts:194-239
4. Medium: Index surveys throw at the 50-page limit instead of returning { truncated: true }. packages/mcp-shared/src/client.ts:623-664
5. Medium: Invalid server IDs and cross-server or malformed tool names trigger full catalog scans before local validation. packages/gatekeeper-mcp-portal/src/portal.ts:425-462

[ask-bonk]

@dcartertwo Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[ask-bonk]

Confirmed the mint path fails closed: a named tool absent from the (possibly truncated) index throws. The security-critical invariant holds.

I have completed a thorough independent review. My assessment of the four GPT findings and the overall change:

• Findings Bump vitest from 3.2.4 to 4.1.0 #1 and Bump vite from 7.3.2 to 7.3.5 #3 describe behavior that is pre-existing in the base branch (06870ec, PR Add bounded progressive MCP discovery #169), not introduced by this PR. In finding Bump vitest from 3.2.4 to 4.1.0 #1's specific case, this PR's new allowsOAuthFallback (returning server.auth !== "none" for the portal) actually closes the "public account flipped to OAuth" path rather than opening it. Finding Bump vite from 7.3.2 to 7.3.5 #3's #list loop is byte-identical to the base; and where its throw is reachable, the new callers fail closed (no mint / configurator error).
• Finding Sync GitLab main to GitHub #4 is incorrect. getGatekeeperClassFor short-circuits requestedTools?.size === 0 to { tools: [], truncated: false } (portal.ts:443-444), so an empty tool scope mints with no catalog scan. The finding misreads the code.
• Finding Bump vitest from 3.2.4 to 3.2.6 #2 describes a genuine behavior change introduced here (if (structured) return structured now suppresses the text fallback whenever any structuredContent is present). But it is a deliberate, documented, test-pinned design decision that fails closed (degrades to prefix-derived names, or the configurator throws "Could not retrieve the portal's complete server list"), never mints an over-broad grant, and only bites a hypothetical future portal that pairs an unrecognized-shape structuredContent with a valid text list. Under the review standard (report discrete actionable defects; ignore speculative, fail-closed concerns), this doesn't rise to a blocking finding.

I independently verified the security-critical mint path fails closed in every branch (validateToolScopeAgainstCatalog throws on truncation or absence; requestedTools.size is bounded ≤200 by parseToolScope), the classification-source invariant (labels derived from bounded index annotations via classifyAnnotations, matching runtime classifyTool), the requirePortalServerScope asserts upgrade legitimizing the dropped scope.serverId!, the concurrency guard (#connectingRevisions), and cross-package references. I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification was consistent.

No actionable defects introduced by this PR.

LGTM!

github run

[ask-bonk]

UnknownError: "Internal server error"

github run

[ask-bonk]

No dangling references to the removed helpers in the connector. The imports at the top of portal.ts (fetchToolIndex, listMatchingToolIndex, toolBelongsToServer, isValidToolName, hexEncode, classifyAnnotations via config, etc.) all resolve to existing exports I've confirmed.

My independent review is complete. I have no inline findings to submit.

Review complete

I performed an independent review of this PR against the base branch feature/mcp-progressive-discovery (a0d934e), reading the actual source rather than relying on the prior reviews, and independently triaging the five GPT findings from the Maximo-Guk comment.

Mint path (getGatekeeperClassFor, portal.ts:385-473) — fails closed in every branch. Named-tool grants call listMatchingToolIndex(requestedTools.size, filter) bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard; validateToolScopeAgainstCatalog throws on truncation-with-missing-name, cross-server name (toolBelongsToServer), or absent name. Reported server-wide grants resolve via reportedServers with {tools:[], truncated:false}; the pinned-empty (size-0) case validates the server and mints zero tools; the listedServers === null path still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL). All branches throw rather than mint an under-validated grant.

Classification invariant — toolGrantOptions (config.ts:118) derives read-only/needs approval from the bounded index annotations via classifyAnnotations, and runtime classifyTool (tools.ts:101) delegates to the same function on the same clampAnnotations booleans, so the picker label and enforced behavior agree. This is exactly the defect the new config tests pin, with the annotation-clamping test defending against oversized readOnlyHint payloads.

Auth transitions — allowsOAuthFallback (portal.ts:280) narrows the base to server.auth !== "none", which closes the public→OAuth-persistence path for the portal; #configurationRevision folds MCP_PORTAL_TOKEN in only for token auth; the #connectingRevisions guard is per-DO in-memory and cleaned up in finally.

On the five GPT findings: I confirmed #1 and #4 describe pre-existing base behavior (byte-identical #list and beginConnect OAuth-persist paths) not introduced here; #1 is actually reduced by this PR. #2 is benign (OAuth digest excludes the token; auth transitions throw before the revision block). #3 is a real, deliberate behavior change (if (structured) return structured now suppresses the text fallback for a malformed non-array structuredContent) but fails closed and only affects a hypothetical future portal shape. #5 is efficiency/hardening only, strictly less scanning than the base, bounded and fail-closed. None rise to a discrete, actionable defect under the review standard.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability, and cross-package references was consistent.

LGTM!

github run

[ask-bonk]

ClassifiedTool = ToolClassification & { tool: McpTool } retains the same shape as before (tool plus the classification fields), so all existing consumers (catalog, schema-to-ts, session, facet, gatekeeper-mcp) keep type-checking. The refactor is backward-compatible.

I've now completed a thorough independent review of the actual delta (base 0d7a91d → 972434f), reading source rather than relying on prior comments, and I independently triaged the five GPT findings.

Review summary

I verified the security-critical paths against the actual source in mcp-shared and gatekeeper-mcp-portal.

Mint path (getGatekeeperClassFor, portal.ts:299-387) — fails closed in every branch:

• Named-tool grants call listMatchingToolIndex(requestedTools.size, filter); requestedTools.size is bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard (scope.ts:95). validateToolScopeAgainstCatalog (scope.ts:150-179) throws on any missing name — either "truncated → cannot validate" or "absent". When all names are present, truncation is irrelevant since the check only fires on absence. A forever-paginating endpoint now returns {tools:[], truncated:true} (client.ts:667) rather than throwing, which still fails closed here.
• The size-0 pinned-empty case and reported server-wide grants resolve via reportedServers; unreported server-wide grants require one prefixed-tool proof; the listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

scanLimit() throw→return change (client.ts:667) is safe. findTool and listMatchingToolSummaries pass failOnScanLimit=true and still throw; only the index/list surveys degrade to truncated:true, and every mint-path consumer of that treats truncation as fail-closed. Each withClient builds a fresh McpClient, so discovery counters are per-operation.

Classification invariant (the defect the tests pin). toolGrantOptions (config.ts:132) derives read/approval labels from classifyAnnotations on the bounded index annotations, and runtime classifyTool (tools.ts:94) delegates to the same classifyAnnotations over the same clampAnnotations booleans, so picker label and enforced behavior agree. ClassifiedTool = ToolClassification & { tool } keeps the prior shape, so all existing consumers stay type-sound.

Type soundness. requirePortalServerScope upgraded to an asserts function legitimizes dropping scope.serverId!. IndexedTool ({name; annotations?}) is structurally assignable where validateToolScopeAgainstCatalog expects ToolCatalog (reads only name/truncated).

Fail-closed parsing. parsePortalServers now returns {servers, complete}; complete is false for absent/unrecognized/partly-malformed replies. structuredContent:[] correctly yields {complete:true, servers:[]} (authoritative empty), and a non-array object falls through to text.

On the five GPT findings (Maximo-Guk): #1 and #4 describe pre-existing base behavior (the #list page loop and account persistence paths), and #4 misreads the size-0 short-circuit which avoids a scan; #2 is benign given the token-excluding OAuth digest; #3 is the deliberate, test-pinned structuredContent-authoritative decision that fails closed; #5 is strictly-less-scanning hardening. None is a discrete, actionable defect introduced here.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability, and cross-package references was consistent. I found no actionable defects.

LGTM!

github run

[ask-bonk]

scopeAllows still exists and is used in mcp-shared (catalog, facet, scope) and gatekeeper-mcp. It's only removed from the portal connector's own imports. The doc-comment reference in config.ts:87 accurately describes the runtime-enforcement behavior (the facet still uses scopeAllows). So the comment is accurate. No dead reference.

I have completed my review. Let me summarize my independent assessment.

Review complete

I reviewed the true delta of this PR (single commit b4a5027 on base d45b0ab, feature/mcp-progressive-discovery), reading the source directly and independently triaging the five GPT findings from the Maximo-Guk comment.

Mint path (getGatekeeperClassFor, portal.ts:339-376) — fails closed in every branch. requirePortalServerScope now runs first as an asserts function and rejects invalid server ids and cross-server/malformed tool names before any remote scan. Named-tool grants call listMatchingToolIndex(requestedTools.size, filter) (bounded ≤200 by parseToolScope); validateToolScopeAgainstCatalog (scope.ts:150-179) throws on truncation-with-missing-name, cross-server name, or absence. The size-0 pinned-empty case and reported server-wide grants resolve via reportedServers; unreported server-wide grants require one prefixed-tool proof; the listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

scanLimit() throw→return change (client.ts:667) is safe. findTool/listMatchingToolSummaries still pass failOnScanLimit=true and throw; only the index/list surveys degrade to {truncated:true}, which every mint-path consumer treats as fail-closed. The plain-endpoint listTools/fetchTools path now returns a truncated catalog for a forever-paginating server rather than throwing — a graceful degradation consistent with the existing byte/scan-budget truncation, and enforcement still validates against truncated.

Classification-source invariant (the pinned defect). toolGrantOptions (config.ts) derives read-only/needs approval from the bounded index's annotations via classifyAnnotations, and runtime classifyTool (tools.ts) delegates to the same classifyAnnotations over the identical clampAnnotations booleans, so picker label and enforced behavior agree. Tests pin this, including vetted/byo parity and dropping of non-boolean/unknown claims.

Fail-closed parsing. parsePortalServers now returns {servers, complete}. A non-array structuredContent returns null and does fall through to text (test-confirmed), so GPT finding #3's "non-array object suppresses text" is incorrect; only an actual array is authoritative, and incomplete parses fail closed at the configurator/mint boundary.

On the five GPT findings: #1 and #2 reference account.ts, which this PR does not touch (byte-identical to base — out of scope). #3 is addressed by the deliberate, test-pinned array-authoritative design that fails closed. #4 misreads the size-0 short-circuit and the fail-closed truncation semantics. #5 is resolved: local validation now precedes remote scans. None is an actionable defect introduced here.

Consistency. No dangling imports; removed helpers survive only in accurate prose comments. The "empty"/"unavailable" state split, ?codemode=off guidance, and "up to 200 shown" copy are consistent across .d.ts, UI, README, and tests.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability (IndexedTool structurally satisfies McpTool/ToolCatalog), the asserts upgrade legitimizing the dropped scope.serverId!, and all cross-package references was consistent.

LGTM!

github run

[ask-bonk]

Everything checks out. clampAnnotations preserves exactly the four booleans (dropping non-boolean/unknown), and both indexTool and clampToolDefinition use it, so the picker label (from index annotations) and runtime classifyTool (from full definition annotations) will agree.

I've now independently verified the security-critical paths. Let me consolidate my findings.

Review complete

I reviewed the true delta of this PR (single commit b4a5027 on base d45b0ab, feature/mcp-progressive-discovery), reading the source directly rather than relying on the prior ask-bonk reviews, and independently triaging the five GPT findings from the Maximo-Guk comment.

Mint path (getGatekeeperClassFor, portal.ts:339–386) — fails closed in every branch. requirePortalServerScope now runs first as an asserts function (config.ts:96) and rejects invalid server ids and cross-server/malformed tool names locally, before any remote scan. Named-tool grants call listMatchingToolIndex(requestedTools.size, filter) (bounded ≤200 by parseToolScope's MAX_TOOLS_PER_SERVER guard, scope.ts:95); validateToolScopeAgainstCatalog (scope.ts:167–178) throws on truncation-with-missing-name, cross-server name, or absence — never a silent mint. The scope.tools = [] (size-0) case, reported server-wide grants, and the unreported single-prefixed-tool proof all resolve fail-closed. The listedServers === null branch still proves portal capability via findTool(PORTAL_LIST_SERVERS_TOOL).

scanLimit() throw→return change (client.ts:667) is safe. findTool/listMatchingToolSummaries still pass failOnScanLimit=true and throw; only the plain index/list surveys degrade to { truncated: true }, which every mint-path consumer treats as fail-closed. Test-pinned by "returns a truncated catalog/index for a server that paginates forever."

Classification-source invariant (the pinned defect). toolGrantOptions (config.ts:132) derives read-only/needs approval from the bounded index annotations via classifyAnnotations, and runtime classifyTool (tools.ts) delegates to the same classifyAnnotations over the identical four clampAnnotations booleans. clampAnnotations (client.ts:325) drops non-boolean/unknown claims, so a server cannot inject an oversized readOnlyHint payload — pinned by the new tests, including vetted/byo parity.

Fail-closed parsing. parsePortalServers now returns { servers, complete }; complete is false for absent/unrecognized/partly-malformed replies. structuredContent: [] correctly yields complete-empty, a non-array structured object falls through to text (test-confirmed), and joining all text blocks marks the listing incomplete if any bullet is malformed. listServerOptions throws when a truncated index pairs with an incomplete listing.

On the five GPT findings: #1 and #2 reference packages/mcp-shared/src/account.ts, which this PR does not touch (byte-identical to base — out of scope for this diff). #3 (structuredContent suppresses text fallback) is addressed: a non-array structured payload returns null and does fall through to text. #4 misreads the size-0 short-circuit and the fail-closed truncation semantics. #5 is resolved: local scope validation now precedes remote scans. None is an actionable defect introduced by this PR.

Consistency. No dangling imports; removed connector helpers (fetchPortalServers, requireCompleteCatalogForToolSelection, scopeAllows, classifyTool, groupToolsByServer) survive only in accurate prose or remain used elsewhere in mcp-shared. The "empty"/"unavailable" state split, ?codemode=off guidance, and "up to 200 shown" copy are consistent across the .d.ts, UI, README, and tests.

I could not run pnpm build/types:check/pnpm test (read-only environment, dependencies not installed), but static verification of the mint-path fail-closed logic, classification-source correctness, type assignability (IndexedTool structurally satisfies ToolCatalog's name-only requirement), the asserts upgrade legitimizing the dropped scope.serverId!, and all cross-package references was consistent.

LGTM!

github run