Add bounded progressive MCP discovery

[dcartertwo]

An MCP grant is broader than the bounded catalog shown to the agent: a server-wide grant also covers authorized tools that fall beyond the catalog’s size limit. This adds bounded search and exact-name lookup so those tools remain discoverable and callable without loading an unbounded tools/list response.

---

[ask-bonk]

There's a test pinning MCP_BASE_TYPES to types.d.ts exactly. Both were edited identically in the diff, so this stays green. Good.

I've completed my review. Let me verify the build/types pass conceptually by checking there are no obvious type issues with the McpToolListOptions never fields and the session listTools overload return type. The implementation returns McpToolInfo[] | McpToolSummary[], matching the widest overload. This is consistent.

Review Summary

This is a large but high-quality PR. I read every line of the kernel changes (workshop-shared/gatekeeper.ts, overseer.ts) and the security-critical mcp-shared paths (trust boundary in tools.ts, at-most-once guarantees in action-store.ts, scope validation, and the new discovery bounds). The design is sound and the documentation and tests are exemplary.

Key strengths I verified:

• At-most-once integrity preserved. In facet.applyAction, revalidation throws ActionInvalidatedError inside the call wrapper before client.callTool runs, so dispatched stays false; the store marks it non-retryable and the DO-restart recovery (applying AND dispatched = 0 → retryable) correctly distinguishes pre-dispatch interruptions from possibly-landed writes.
• Portal widening guard maintained. scopedCatalog decides isPortal from scope.serverId first, so a filtered server-scoped listing that no longer contains portal_list_servers still keeps the portal_* exclusion in force — the comment explains the portal_toggle_servers self-widening attack it prevents. #freshTool re-probes for portal-native names before hydrating.
• Discovery is genuinely bounded (per-client scan bytes/tools/pages, facet-wide concurrency + queue with deadline, HydratedTools entry/byte caps with negative-result caching).
• looksLikePortal overloads correctly migrate wide-index callers to explicit cap while preserving the fail-open-toward-portal semantics.
• Kernel change is minimal and correct: ACTION_INVALIDATED_ERROR_CODE is well-scoped; the overseer converts it to a terminal rejected state, which unblocks the auto-approval drain without applying past a gate.
• base-types.ts/types.d.ts sync is preserved (pinned by base-types.test.ts; both edited identically).
• JSDoc discipline: the agent-facing .d.ts correctly avoids leaking approval-queue/internal details; discovery is documented in user terms.

I found no actionable defects. The concurrency accounting in runDiscovery, the HydratedTools eviction math, the string request-id/SSE routing fix, and the shared operation deadline all check out.

LGTM!

github run

[ask-bonk]

Documentation is thorough and accurate. The README limits table reflects the new bounds. Note the HydratedTools per-facet caps (200 entries / 1 MiB) and the search bounds (200 query chars, 20 results) aren't in the README Limits table, but that's a minor documentation completeness point, not a defect.

I've completed a comprehensive review. Let me summarize my findings. The one substantive finding is the unused public API on HydratedTools.

Review Summary

I reviewed every line of the kernel/kernel-adjacent changes (workshop-shared/gatekeeper.ts, workshop-backend/overseer.ts) and the security-critical mcp-shared paths (the tools.ts trust boundary, at-most-once integrity in action-store.ts, scope validation, the revalidation-before-dispatch in facet.ts, and the new discovery bounds in client.ts). I ran the tests (305 passing) and type checks for mcp-shared, both MCP gatekeepers, and workshop-backend — all green.

The design is sound. The things I verified specifically:

• At-most-once preserved. In facet.applyAction, revalidation throws ActionInvalidatedError inside the call wrapper before fn(client) runs, so dispatched stays false; the store closes it non-retryable and re-throws. The DO-restart recovery correctly splits applying AND dispatched IS NOT 0 (outcome-unknown) from dispatched = 0 (pre-dispatch, restage). The overseer converts the code to a terminal rejected state, and because that path resolves rather than throws, the auto-approval drain advances instead of getting stuck (auto-approval.ts:85).
• Scope validation relaxation is safe. validateToolScopeAgainstCatalog now only raises the "truncated, cannot validate" error when a named tool is genuinely absent; a tool present in a truncated catalog is positive proof and validates. The byo path fetches with listMatchingToolIndex, which fails closed (returns truncated:true) if the scan budget is hit with a tool still missing.
• Portal self-widening guard intact. scopedCatalog decides isPortal from scope.serverId first; #freshTool re-probes portal-native names with a cap:2 listing before hydrating.
• Discovery is genuinely bounded — per-client scan pages/tools/bytes, facet-wide concurrency (4) + bounded queue (32) with a shared deadline, and HydratedTools entry/byte caps with negative-result caching. The concurrency accounting conserves the active-slot count across handoff, completion, and timeout.
• base-types.ts/types.d.ts edited identically (pinned by base-types.test.ts); the agent-facing .d.ts doesn't leak approval-queue internals.

Finding (non-blocking)

Dead public API on HydratedTools (catalog.ts). HydratedTools.refresh() and the public HydratedTools.remember(tools) have no production callers — only unit tests exercise them. The policy-revalidation path (resolveToolForCall/#freshTool) was implemented directly on the facet via #resolvingCalls, bypassing HydratedTools, and searchTools calls listMatchingToolSummaries without feeding results back through remember. This is unused surface in a kernel-adjacent module rather than a correctness or security issue, but per this package's "fewer lines = easier review" bar it's worth either wiring in (e.g. searchTools populating the cache via remember, the refresh path using refresh) or dropping until a caller exists.

I did not post this inline since it spans the definition and its (absent) call sites rather than a single changed line. No other actionable issues found.

github run

[ask-bonk]

The README limits table is now comprehensive — the earlier bonk finding about missing hydrated/search bounds has been addressed (they're now in the table). Documentation is thorough.

I've completed a comprehensive review. My findings match and extend the earlier bonk reviews. Let me summarize.

Review Summary

I reviewed every line of the kernel/kernel-adjacent changes (workshop-shared/{gatekeeper,api}.ts, workshop-backend/{overseer,auto-approval}.ts) and the security-critical mcp-shared paths (the tools.ts trust boundary, at-most-once integrity in action-store.ts, scope validation, the revalidation-before-dispatch in facet.ts, discovery bounds in client.ts, and the HydratedTools cache in catalog.ts). I could not run pnpm (not installed, read-only tree), but the two prior automated runs report 305 tests passing and green type checks; I verified the test coverage of the security-critical paths by reading the diffs.

Things I verified specifically:

• At-most-once preserved. ActionStore.apply sets dispatched=false, saves, then the facet's call wrapper revalidates (connection generation, scope, policy fingerprint) and throws ActionInvalidatedError before fn(client) flips dispatched=true. The store closes the record failed / retryable=false / dispatched=false and returns {outcome:"invalidated"} idempotently (re-apply hits the guard at action-store.ts:184). DO-restart recovery correctly splits applying AND dispatched IS NOT 0 (outcome-unknown, non-retryable) from dispatched=0 (pre-dispatch, restage).
• Turn semantics are consistent. On manual-approve invalidation the overseer records the terminal invalidated state and returns without resuming (overseer.ts:7604), matching the deliberate "denial leaves the turn ended" rule for rejectAction. The sibling-resume guard now treats invalidated like rejected (overseer.ts:7720), so a sibling approval can't resume a turn that had an invalidated awaited action. The auto-approval drain stops on the first invalidated outcome and won't restart even if a concurrent drain requested a rerun (covered by a new test).
• Portal self-widening guard intact. scopedCatalog decides isPortal from scope.serverId first; #freshTool re-probes portal-native names with a cap:2 listing. The looksLikePortal boolean-overload callers (portal.ts:392,432) all list with MAX_TOOLS_PER_SERVER, matching the cap the boolean form assumes, so the migration is safe.
• Discovery is genuinely bounded. Per-McpClient scan counters (pages/tools/bytes) are per-call since withClient mints a fresh client; #list filters before the count/byte budgets so a named tool's absence is proof of absence; facet-wide concurrency (4) + bounded queue (32) with a shared deadline; HydratedTools entry/byte caps with negative-result caching and LRU eviction.
• Frontend correctness improved. Removing applyOptimisticActionState (which could only produce approved/rejected) and relying on the authoritative server broadcast is the right fix now that invalidated is a possible outcome; invalidationReason is surfaced to the user.
• Documentation is thorough — the README limits table now includes the hydrated-definition, filtered-scan, search, and bounded-operation-deadline bounds that a prior review noted were missing.

I found no actionable defects. The kernel diff is minimal and correct, and the design holds up against the self-widening, replay, and unbounded-work attack surfaces it targets.

LGTM!

github run

[ask-bonk]

I reviewed every line of the kernel/kernel-adjacent changes (workshop-shared/{gatekeeper,api}.ts, workshop-backend/{overseer,auto-approval}.ts) and the security-critical mcp-shared paths (the tools.ts trust boundary, at-most-once integrity in action-store.ts, scope validation, revalidation-before-dispatch in facet.ts, the discovery bounds in client.ts, and the HydratedTools cache in catalog.ts). I could not run pnpm (not installed, read-only tree), but two prior automated runs report 305 tests passing and green type checks; I verified security-critical coverage by reading the diffs and tests.

Things I verified specifically:

• At-most-once integrity preserved. ActionStore.apply persists dispatched=false before the call; dispatched is only flipped to true inside the wrapper when fn(client) actually runs. The facet's revalidation (connection generation, scope, policy fingerprint) throws ActionInvalidatedError before fn(client), so dispatched stays false and the record closes failed/retryable=false/dispatched=false, returning invalidated idempotently. DO-restart recovery correctly splits applying AND dispatched IS NOT 0 (outcome-unknown, non-retryable — including legacy NULL rows) from dispatched=0 (pre-dispatch, retryable restage).
• Turn/drain semantics consistent. The overseer converts the invalidated outcome to a terminal invalidated state; manual approve returns before resume, and the sibling-resume guard treats invalidated like rejected (overseer.ts:7720), so a turn with an invalidated awaited action stays ended. The auto-approval drain stops on the first invalidated and won't restart even with a pending rerun request (covered by a new test).
• Scope-validation relaxation is safe. The "truncated, cannot validate" error now fires only when a named tool is genuinely absent; a tool present in a truncated catalog is positive proof. The byo path fetches with listMatchingToolIndex (filters before budget), which returns truncated:true if the scan budget is hit with a tool still missing — fail-closed, no false-accept.
• Portal self-widening guard intact. scopedCatalog decides isPortal from scope.serverId first; #freshTool re-probes portal-native names with a cap:2 listing. The two looksLikePortal boolean-overload callers (portal.ts:392,432) both source tools from fetchTools (cap MAX_TOOLS_PER_SERVER), matching the cap the boolean form assumes.
• Discovery is genuinely bounded — per-McpClient scan pages/tools/bytes (per-call since withClient mints a fresh client), include filtering before the count/byte budgets, facet-wide concurrency (4) + bounded queue (32) with a shared deadline, and HydratedTools entry/byte caps with negative-result caching and LRU eviction. The concurrency semaphore conserves the active-slot count across handoff, completion, and timeout.
• Untrusted-text discipline — MAX_QUERY_CHARS enforced at the session layer, clampAnnotations/clampToolSummary bound server-supplied fields, and the search-query observation is plainInline-flattened.
• base-types.ts/types.d.ts edited identically (pinned by base-types.test.ts); the agent-facing .d.ts doesn't leak approval-queue internals.
• Frontend correctly drops the optimistic approved/rejected-only state in favor of the authoritative broadcast and surfaces invalidationReason.

I found no actionable defects. The kernel diff is minimal and correct, and the design holds up against the self-widening, replay, and unbounded-work surfaces it targets.

LGTM!

github run

[ask-bonk]

I reviewed every line of the kernel/kernel-adjacent changes (workshop-shared/{gatekeeper,api}.ts, workshop-backend/{overseer,auto-approval}.ts) and the security-critical mcp-shared paths (the tools.ts trust boundary, at-most-once integrity in action-store.ts, scope validation, revalidation-before-dispatch in facet.ts, the discovery bounds in client.ts, and the HydratedTools cache in catalog.ts). I diffed against the base commit 8513756 (the #168 dependency) so the review covers only this PR's changes. pnpm isn't installed on this read-only tree, but the four prior automated runs report 305 tests passing and green type checks; I verified security-critical coverage by reading the diffs and tests.

Things I verified specifically:

• At-most-once integrity preserved. ActionStore.apply persists dispatched=false before the call; dispatched flips to true only inside the wrapper when fn(client) actually runs (action-store.ts:213). The facet's revalidation — connection generation, scope, then policy fingerprint — throws ActionInvalidatedError before fn(client) (facet.ts:322), so dispatched stays false, the record closes failed/retryable=false/dispatched=false, and re-apply returns invalidated idempotently at the top guard (action-store.ts:184). DO-restart recovery correctly splits applying AND dispatched IS NOT 0 (outcome-unknown, non-retryable, including legacy NULL rows) from dispatched = 0 (pre-dispatch, retryable restage).
• Turn/drain semantics consistent. Manual approve returns before resume on invalidation (overseer.ts:7604); the sibling-resume guard treats invalidated like rejected (overseer.ts:7720), so a turn with an invalidated awaited action stays ended. The auto-approval drain returns on the first invalidated outcome and won't restart even with a pending rerun request — covered by the new does not restart after invalidation test.
• Scope-validation relaxation is safe. The "truncated, cannot validate" error now fires only when a named tool is genuinely absent (scope.ts:160); a tool present in a truncated catalog is positive proof. The byo path fetches with listMatchingToolIndex, which filters before the byte budget and returns truncated:true when the scan budget is hit with a tool still missing — fail-closed, no false-accept.
• Portal self-widening guard intact. scopedCatalog decides isPortal from scope.serverId first (catalog.ts), and #freshTool re-probes portal-native names with a cap:2 listing. The looksLikePortal boolean-overload migration preserves the fail-open-toward-portal semantics; the wide-index callers pass explicit cap.
• Discovery is genuinely bounded — per-McpClient scan pages/tools/bytes (per-call since withClient mints a fresh client), include filtering before the count/byte budgets, facet-wide concurrency (4) + bounded queue (32) sharing one 30s deadline that includes queue-wait time, and HydratedTools entry/byte caps with negative-result caching and LRU eviction. The concurrency semaphore conserves the active-slot count across handoff, completion, and timeout.
• Untrusted-text discipline — MAX_QUERY_CHARS/MAX_TOOL_NAME_CHARS enforced at the session layer, clampAnnotations/clampToolSummary bound server-supplied fields, and the search-query observation is plainInline-flattened. The string request-id prefix fixes the real SSE response-routing collision.
• base-types.ts/types.d.ts edited identically (pinned by base-types.test.ts); the agent-facing .d.ts doesn't leak approval-queue internals.
• Frontend correctly drops the optimistic approved/rejected-only state in favor of the authoritative broadcast and surfaces invalidationReason.
• Prior finding resolved — the dead HydratedTools.refresh()/public remember() from the earlier review are gone; only resolve() is public now.

I found no actionable defects. The kernel diff is minimal and correct, and the design holds up against the self-widening, replay, and unbounded-work surfaces it targets.

LGTM!

github run

[ask-bonk]

Let me check one concern about #resolvingCalls coalescing in facet.ts — whether a resolve that captured a stale policy could be served to a concurrent caller. The map is keyed by name, cleared on settle. Since resolveToolForCall is called fresh per callTool, and the result is used immediately to stage with the captured snapshot, then revalidated at apply time — coalescing only affects concurrent in-flight resolves, which is a correctness-neutral optimization (both callers get the same fresh policy). The revalidation at applyAction is the actual guard. Fine.

Let me verify the Max_TOOL_PAGES double-counting isn't a real bug: within a single #list, both the loop page counter AND #discoveryPages increment per page. Since a fresh client per call means #discoveryPages starts at 0, both start aligned. The #discoveryPages >= MAX_TOOL_PAGES check at loop top plus the for loop bound page < MAX_TOOL_PAGES are redundant but not harmful. However, #freshTool and findTool may call #list multiple times on the same client? Let me verify.

In facet.resolveToolForCall, this.call(...) wraps a single withClient → single client → the callback calls #freshTool which either calls client.listTools(2, ...) OR client.findTool(name) — one #list invocation. So #discoveryPages accumulates only within that one logical operation. Good.

The review is complete. All the security-critical invariants hold, the prior bonk finding (dead HydratedTools API) is resolved, and the documentation/tests are thorough. I found no actionable defects that the prior five reviews didn't already cover and that remain unaddressed.

LGTM!

github run

[ask-bonk]

The logic is complete and correct. dispatched flips to true inside the call callback right before client.callTool, and it's persisted via #save. The success path (state = "applied") preserves dispatched = true.

One subtle detail I want to confirm: the dispatched local variable at line 205 gates mayHaveLanded at line 225. This is the correct guard — a failure that occurs before the callback runs (e.g., connection setup failure inside withClient, or a validation throw that isn't ActionInvalidatedError) has dispatched === false, so mayHaveLanded is false and it's retryable. That matches the test at "keeps validation failures retryable when tools/call was never reached".

I've reviewed the full diff thoroughly against the base commit 47fb582. My review confirms the six prior automated reviews: the kernel change is minimal, at-most-once integrity is preserved, the portal self-widening guard holds, discovery is bounded, and untrusted-text discipline is maintained. I traced each security-critical path myself (action-store migration/recovery, facet revalidation-before-dispatch, runDiscovery concurrency accounting including the timeout/handoff races, scope validation relaxation, looksLikePortal overload cap consistency, and the frontend optimistic-state removal) and found no actionable defects.

I could not run pnpm (read-only tree, not installed), but the prior runs report 305 passing tests and green type checks, and I verified the tests cover the key invariants.

LGTM!

github run

[Maximo-Guk]

GPT Findings:

• High: Invalidated actions only stop the current drain. Later drains skip them and auto-apply successors. packages/workshop-backend/src/auto-approval.ts:61-86
• High: The new successful applyAction() invalidation result fails open during staggered deployment: an older backend ignores it and records the undispatched action as approved. Use a backward-compatible thrown sentinel. packages/mcp-shared/src/facet.ts:320-344
• Medium: Empty-string MCP cursors are treated as pagination completion, although cursors are opaque strings. packages/mcp-shared/src/client.ts:605-637
• Medium: Catalog refresh and approval-time tool revalidation bypass the new discovery semaphore/deadline, allowing stalled requests and unbounded waiters. packages/mcp-shared/src/facet.ts:193-205,331-342
• Low: Duplicate search terms can amplify matching work across the 4 MiB scan budget. Deduplicate/cap terms. packages/mcp-shared/src/tool-search.ts:39-53
• Low: Activity history labels invalidated actions but omits their reason. packages/workshop-frontend/src/Activity.tsx:611-644

[ask-bonk]

I have completed my independent review. I traced every security-critical path myself and evaluated each of the six externally-reported "GPT Findings" against the actual code rather than restating prior reviews.

Verification of security-critical invariants (all hold)

• At-most-once integrity (action-store.ts:199-239): dispatched flips to true only inside the call callback immediately before client.callTool. The facet's ActionInvalidatedError is thrown before fn(client) runs (facet.ts:328,333,338), so dispatched stays false, the record closes failed/retryable=false/dispatched=false, and re-apply returns invalidated idempotently (action-store.ts:184-190). The mayHaveLanded guard is correctly gated on the dispatched local (action-store.ts:225).
• DO-restart recovery migration (action-store.ts:99-106): correctly splits applying AND dispatched IS NOT 0 (outcome-unknown, non-retryable — legacy NULL rows fall here, the safe default) from dispatched = 0 (pre-dispatch, retryable restage).
• Turn-resume gate (overseer.ts:7884) treats invalidated like rejected, so a turn with an invalidated awaited action stays ended.
• Portal self-widening guard (facet.ts:232-239): #freshTool re-probes portal-native names with a cap:2 listing before hydrating.

Assessment of the reported findings

• "High Bump vitest from 3.2.4 to 4.1.0 #1 — later drains skip invalidated actions." This behavior is identical to how rejected already works: both are terminal states filtered out of the drain's pending list (auto-approval.ts:65), so neither persistently blocks a successor across drain invocations. This is a pre-existing property of the drain that the PR mirrors intentionally, not a new regression. Not a new defect for this PR.
• "High Bump vitest from 3.2.4 to 3.2.6 #2 — staggered-deploy fail-open." The RPC contract change (return object vs throw) means a new gatekeeper + old backend window would record an un-dispatched action as approved. But it does not cause the tool call to execute (dispatched=false); the only consequence is a misleading audit record during a rollout window on a rare policy-change race. Real, but Medium at most, not High. Worth the author's judgment whether to keep the thrown-sentinel form for rollout safety.
• Medium — empty-string cursors (client.ts:647): spec-pedantry; MCP servers signal end-of-list by omitting nextCursor, and line 616 is consistent. Negligible real-world impact.
• Medium — approval-time revalidation bypasses the discovery deadline/semaphore (facet.ts:320-345): confirmed real and the most substantive. applyAction calls this.call(...) with no deadline, and fetchOptions(env) sets no timeoutMs, so guardedFetch applies no AbortSignal.timeout (fetch.ts:155-162). The PR newly adds catalog() + #freshTool (listTools/findTool) I/O to this path, whereas every discovery path (searchTools/findTool/resolveToolForCall) carefully threads a deadline. A stalled server can now hang the revalidation indefinitely. applyAction is serialized (no amplification), so severity is modest, but the inconsistency is worth addressing by passing a deadline to the revalidation this.call.
• Low — duplicate search terms (tool-search.ts): query capped at 200 chars over a 4000-char field; negligible.
• Low — Activity omits invalidation reason (Activity.tsx:611-644): confirmed. The record carries invalidationReason and the status shows "Invalidated", but the expanded detail panel never renders the reason. Minor UX completeness gap.

Findings not posted inline (they span multiple lines / definitions rather than one changed line): the approval-time un-timed revalidation I/O (Medium) and the missing invalidationReason display (Low). I did not re-open the two "High" items as actionable defects for the reasons above. No inline review submitted.

Overall this is a high-quality, well-tested PR with a minimal and correct kernel diff. The one item I'd genuinely recommend addressing before merge is threading a deadline through the applyAction revalidation this.call so the newly-added discovery I/O in the approval path can't hang unbounded.

I'll also echo the maintainer's own review question: the second paragraph (revalidate-before-dispatch + invalidated terminal state, touching the kernel auto-approval.ts/overseer.ts and the workshop-shared RPC contract) is arguably a separable concern from progressive discovery, and splitting it would shrink the kernel-review surface per this package's "fewer kernel lines = easier review" bar.

github run

[ask-bonk]

The looksLikePortal overloads are safe. The boolean form defaults cap: MAX_TOOLS_PER_SERVER (line 74), matching what its callers actually requested. The #freshTool re-probe uses { truncated, cap: 2 } (facet.ts:244) correctly. The fail-open-toward-portal semantics (truncated → treat as portal) prevents the self-widening attack described in lines 46-52.

I've now independently verified all the security-critical invariants and evaluated every prior finding against the actual code. My conclusion:

Summary

I reviewed every line of the kernel/kernel-adjacent changes (workshop-shared/{gatekeeper,api}.ts, workshop-backend/{overseer,auto-approval}.ts) and the security-critical mcp-shared paths (the tools.ts trust boundary, at-most-once integrity in action-store.ts, scope validation in scope.ts, revalidation-before-dispatch in facet.ts, discovery bounds in client.ts, and portal self-widening in portal.ts). I diffed against the base commit (50ac3ef, the #168 dependency) so this covers only this PR's changes. pnpm isn't installed on this read-only tree, but prior runs report 305 tests passing and green type checks; I verified coverage of the security-critical paths by reading the diffs and tests.

Invariants I verified hold:

• At-most-once integrity preserved. ActionStore.apply persists dispatched=false before the call; it flips to true only inside the wrapper when client.callTool actually runs (action-store.ts:216-217). The facet's revalidation throws ActionInvalidatedError before fn(client) (facet.ts:337,341,346), so dispatched stays false and the record closes failed/retryable=false/dispatched=false, returning invalidated idempotently. DO-restart recovery correctly splits applying AND dispatched IS NOT 0 (outcome-unknown, including legacy NULL) from dispatched = 0 (pre-dispatch, retryable).
• Turn/drain semantics consistent. The drain snapshot now includes invalidated and returns early on the first one as a durable barrier (auto-approval.ts:66,71,85), so a later drain can't skip it and apply successors. Manual approve returns before resume (overseer.ts:7779); the sibling-resume guard treats invalidated like rejected (overseer.ts:7895).
• Discovery is genuinely bounded — per-client scan pages/tools/bytes, facet-wide concurrency (4) + bounded queue (32) sharing one deadline, and HydratedTools caps. The revalidation path in applyAction now runs inside runDiscovery with { deadline } (facet.ts:332-350), so the newly-added discovery I/O in the approval path is bounded.
• Scope validation relaxation is safe — a tool present in a truncated catalog validates; the "cannot validate" error fires only on genuine absence (scope.ts:172-177).
• Portal self-widening guard intact; the looksLikePortal overload migration preserves cap consistency.

On the earlier Maximo-Guk "GPT Findings": the current HEAD (e3d3e5f) has already addressed all six. Specifically: invalidated actions now persistently block later drains (High #1); applyAction uses a thrown ActionInvalidatedError (not a return value) whose message-prefix discriminator survives RPC — so an old backend sees a thrown error and leaves the action pending, never records it approved (High #2 does not apply, and the kernel JSDoc at gatekeeper.ts:839-842 documents exactly this); empty-string cursors are now valid (only undefined ends pagination); toolQueryTerms dedupes; and Activity/ChatInterface render invalidationReason.

Process note (echoing the maintainer's own review question): the second paragraph of the description — revalidate-before-dispatch plus the invalidated terminal state, which touches the kernel (auto-approval.ts, overseer.ts) and the workshop-shared RPC contract — is a separable concern from progressive discovery. Splitting it would shrink the kernel-review surface per this package's "fewer kernel lines = easier review" bar. This is a structuring suggestion for the author/maintainer, not a defect.

I found no actionable defects. The kernel diff is minimal and correct, and the design holds up against the self-widening, replay, and unbounded-work surfaces it targets.

LGTM!

github run

[dcartertwo]

Update after splitting the kernel work from this PR: #169 is now discovery-only at 06870ec9. It retains the opaque empty-cursor fix, bounded/deduplicated search, and bounded catalog discovery paths. Approval-time revalidation, rolling-deploy signaling, persistent auto-approval barriers, and invalidation UI moved to #198, stacked after #170.

Discovery-only verification: 300 MCP tests, both MCP connector suites/builds, and full pnpm lint/monorepo build.

[ask-bonk]

The tests thoroughly cover the byte-budget-after-filter semantics (line 220), page-stopping (line 186), and index-vs-catalog byte behavior (line 236). The implementation is well-tested and correct.

I've completed a thorough review of the actual code at HEAD. My analysis:

Summary

I reviewed every line of this PR's diff at HEAD (06870ec), diffing against the base 50ac3ef (the #168 dependency). The security-critical mcp-shared paths (tools.ts trust boundary, scope.ts, client.ts discovery bounds, catalog.ts hydration cache, facet.ts concurrency), the two gatekeeper connectors, schema-to-ts.ts, and all tests. I could not run pnpm (read-only tree), but the tests are extensive and I verified coverage of the key invariants by reading them.

Things I verified hold:

• Discovery is genuinely bounded. Per-McpClient scan counters (#discoveryPages/#discoveryBytes/#discoveryTools, per-call since withClient mints a fresh client), include filtering applied before the count/byte budgets (client.ts:625-627), facet-wide concurrency (4) + bounded queue (32) sharing one 30s deadline (facet.ts:146-175), and HydratedTools entry/byte caps (200 / 1 MiB) with negative-result caching and LRU eviction.
• runDiscovery concurrency accounting is sound. Slots are handed off (shift()/next()) or decremented, never both; a timed-out waiter is spliced before rejecting and never incremented a slot, so there is no leak.
• Scope-validation relaxation is safe. The "truncated, cannot validate" error now fires only when a named tool is genuinely absent (scope.ts:172-177); presence in a truncated catalog is positive proof. The byo path fetches with listMatchingToolIndex, which fails closed on scan-limit.
• Portal self-widening guard intact. scopedCatalog decides isPortal from scope.serverId first (catalog.ts:133); the looksLikePortal boolean-overload callers (portal.ts:400,440) both source tools from fetchTools at cap MAX_TOOLS_PER_SERVER, matching the boolean form's assumed cap.
• Untrusted-text discipline. MAX_QUERY_CHARS/MAX_TOOL_NAME_CHARS enforced at the session layer, toolQueryTerms dedupes and caps, clampToolSummary bounds server text, search-query observation is plainInline-flattened, and the string request-id prefix fixes the SSE routing collision.
• Empty-string cursors are treated as opaque continuation tokens (client.ts:647 only ends on undefined), bounded by MAX_TOOL_PAGES.
• Type-level routing of the new callTool/listTools overloads is verified by a full-program compile test with @ts-expect-error assertions.
• base-types.ts/types.d.ts edited identically (pinned by base-types.test.ts); RESERVED_METHOD_NAMES prevents a wire tool named listTools/callTool from shadowing the base overloads.

One process observation

This branch (06870ec, single commit on 50ac3ef) contains only the bounded progressive discovery half. The PR body's second paragraph — the revalidate-before-dispatch / terminal invalidated state touching auto-approval.ts, overseer.ts, and the workshop-shared RPC contract — and the frontend changes are not present in this diff. Those were reviewed by the earlier bot comments against a later commit (e3d3e5f) that isn't what's on this branch now.

That actually resolves the maintainer's repeated review question ("can't we split the kernel/protocol changes out and land that first?"): as it stands, this HEAD is the discovery-only PR with no kernel or workshop-shared changes at all. If the invalidation work is meant to be part of this PR, it's missing from the current head; if it was intentionally dropped to a follow-up, the PR description should be updated to match, since it still promises the second paragraph's behavior.

No actionable code defects found in the changes present at HEAD.

LGTM!

github run

[Maximo-Guk]

Couple more

1. Medium: packages/mcp-shared/src/client.ts:620-624 rejects an entire page before inspecting it when the page crosses 5,000 tools. findTool() therefore fails even when the requested tool is the first entry of a 5,001-tool page.
2. Low: Generated overloads at packages/mcp-shared/src/schema-to-ts.ts:364-367 allow a variable containing both search and name, despite runtime rejection. Add the opposite property as optional never to each specialized overload.

[ask-bonk]

Test coverage for the concurrency bounds (lines 154, 174), portal-native hydration guards (128, 145), and discovery beyond the catalog (102) is present. The review is complete.

Review Summary

I reviewed this PR at HEAD (a0d934e), diffing against the base (50ac3ef, the #168 dependency). The current HEAD is the discovery-only version — no auto-approval.ts, overseer.ts, or workshop-shared kernel/RPC changes are present, confirming the maintainer's requested split (moved to #198). I read every line of the security-critical mcp-shared paths.

Invariants I independently verified hold:

• Discovery is genuinely bounded. Per-McpClient scan counters (#discoveryPages/#discoveryBytes/#discoveryTools, per-call since withClient mints a fresh client), include filtering applied before the count/byte budgets (client.ts:625-632), facet-wide concurrency (4) + bounded queue (32) sharing one deadline (facet.ts:146-175), and HydratedTools entry/byte caps (200 / 1 MiB) with negative-result caching and LRU eviction.
• runDiscovery concurrency accounting is sound. A slot is either held by an active op or handed to a waiter, never double-counted; the timeout path splices the waiter before rejecting and never incremented a slot, so there's no leak, and clearTimeout/splice are mutually exclusive within the event loop.
• Scope-validation relaxation is safe. The "truncated, cannot validate" error fires only when a named tool is genuinely absent (scope.ts:172-176); presence in a truncated catalog is positive proof.
• Portal self-widening guard intact. scopedCatalog decides isPortal from scope.serverId first (catalog.ts:133); the looksLikePortal boolean overload defaults cap: MAX_TOOLS_PER_SERVER, matching its callers, preserving fail-open-toward-portal.
• Untrusted-text discipline. MAX_QUERY_CHARS/MAX_TOOL_NAME_CHARS enforced at the session layer, toolQueryTerms dedupes (new Set), search-query observation is plainInline-flattened, and mutual exclusion of search/name is enforced at runtime.
• base-types.ts/types.d.ts edited identically (differences are only string-literal escaping; pinned by base-types.test.ts).

On the two most-recent Maximo-Guk findings (posted 21:31), both are already addressed at this HEAD:

• Bump vitest from 3.2.4 to 4.1.0 #1 (findTool fails on first-entry of a 5,001-tool page): the #list loop inspects the bounded prefix (scanCount entries) before reporting the scan limit at client.ts:649. The test at client-pagination.test.ts:315 ("finds an exact tool before an oversized page crosses the scan limit") covers exactly this and passes.
• Bump vitest from 3.2.4 to 3.2.6 #2 (overloads allow a variable with both search and name): McpToolListOptions is a discriminated union with mutual ?: never, and the test at schema-to-ts.test.ts:145-147 proves a variable typed { search: string; name: string } is rejected (@ts-expect-error).

The earlier findings (dead HydratedTools API, empty-string cursors, duplicate search terms) are also resolved at HEAD.

LGTM!

github run

[dcartertwo]

Couple more

1. Medium: packages/mcp-shared/src/client.ts:620-624 rejects an entire page before inspecting it when the page crosses 5,000 tools. findTool() therefore fails even when the requested tool is the first entry of a 5,001-tool page.
2. Low: Generated overloads at packages/mcp-shared/src/schema-to-ts.ts:364-367 allow a variable containing both search and name, despite runtime rejection. Add the opposite property as optional never to each specialized overload.

updated

[ask-bonk]

Confirmed. The only change to gatekeeper-mcp-portal/src/portal.ts in this PR is the describe() snippet wording. The two looksLikePortal(tools, truncated) call sites (lines 400, 440) were left unchanged from the base, where looksLikePortal accepted a boolean. But this PR changed looksLikePortal's signature in mcp-shared/src/portal.ts to require an object { truncated, cap } with no boolean overload — breaking these two callers.

This is my one actionable finding. Let me verify the argument-type mismatch is unambiguous (no any leaking through this.#tools()) — #tools() returns Promise<ToolCatalog>, and ToolCatalog.truncated is boolean. So this is a hard type error.

I'll note the finding won't be posted inline since it spans the definition in one file and two call sites in another. Here is my review.

---

I reviewed the discovery-only PR at HEAD (0d7a91d), diffing against the base 50ac3ef. I read the security-critical mcp-shared paths (client.ts discovery bounds, catalog.ts hydration cache, facet.ts concurrency accounting, scope.ts, tool-search.ts, session.ts) and both connectors, and independently verified the invariants and the two most recent Maximo-Guk findings.

Finding (Medium — breaks the build)

looksLikePortal callers in gatekeeper-mcp-portal were not migrated to the new signature. This PR changes looksLikePortal in packages/mcp-shared/src/portal.ts:57 from a boolean second parameter (truncated = false) to a single required object parameter (bounds: { truncated: boolean; cap: number }), with no boolean overload. But two call sites still pass a bare boolean:

• packages/gatekeeper-mcp-portal/src/portal.ts:400 — if (!looksLikePortal(tools, truncated)) return [];
• packages/gatekeeper-mcp-portal/src/portal.ts:440 — const isPortal = looksLikePortal(tools, truncated);

truncated here is destructured from this.#tools(): Promise<ToolCatalog>, so its type is boolean. Passing it where { truncated: boolean; cap: number } is required is a TS2345 type error, so gatekeeper-mcp-portal will fail pnpm build. The gatekeeper-mcp sibling (mcp.ts:374) and catalog.ts:134 were correctly updated to the object form; only these two were missed. The diff to this file (confirmed against base) touches only the describe() snippet wording — these two lines are unchanged from the base's boolean signature.

Beyond the compile error, the intended runtime behavior is also lost: reading bounds.truncated/bounds.cap off a boolean yields undefined, collapsing looksLikePortal to only the portal_list_servers-presence check and dropping the truncation/cap fail-safe the portal self-widening guard depends on.

Fix: pass { truncated, cap: MAX_TOOLS_PER_SERVER } at both sites (matching the cap fetchTools uses, as the other callers do), and import MAX_TOOLS_PER_SERVER into that file.

The author's discovery-only verification note claimed both MCP connector suites/builds passed; this contradicts the code at HEAD, so it's worth re-running pnpm build for gatekeeper-mcp-portal specifically.

This spans the definition in one file and its two call sites in another, so I did not post it inline.

Everything else verified sound

• Discovery is genuinely bounded — per-McpClient scan counters (#discoveryPages/#discoveryBytes/#discoveryTools, per-call since withClient mints a fresh client), include filtering applied before the count/byte budgets (client.ts:625-628), facet-wide concurrency (4) + bounded queue (32) sharing one deadline (facet.ts:146-175), and HydratedTools entry/byte caps with LRU eviction and negative-result caching.
• runDiscovery concurrency accounting is sound — a slot is either held by an active op or handed to a waiter, never double-counted; the timeout path splices the waiter before rejecting and never incremented a slot, and the clearTimeout/splice are mutually exclusive.
• Both recent Maximo-Guk findings are fixed — the 5000-tool page is inspected as a bounded prefix before the scan limit is reported (client.ts:622-649, test client-pagination.test.ts:315); the listTools overloads carry name?: never/search?: never so a { search; name } variable is rejected (schema-to-ts.ts:364,366, test schema-to-ts.test.ts:145-147).
• Scope-validation relaxation is safe — the "truncated, cannot validate" error fires only on genuine absence (scope.ts:172-177).
• Untrusted-text discipline — MAX_QUERY_CHARS/MAX_TOOL_NAME_CHARS, toolQueryTerms dedupes via new Set, clampToolSummary/clampAnnotations bound server text, search-query observation is plainInline-flattened, and the string request-id prefix fixes the SSE routing collision.
• base-types.ts/types.d.ts edited identically (differences are only template-literal escaping).
• The kernel/protocol changes were correctly split out to Revalidate MCP actions before dispatch #198; this HEAD has no workshop-backend/workshop-shared changes.

One actionable finding above (not posted inline). The rest of the PR is well-designed and thoroughly tested.

github run

[ask-bonk]

The trust boundary changes are additive and safe: toolSummary reuses clampToolSummary and carries the same mode/classifiedBy classification; codeSpan and plainInline are only exported (used to flatten agent-chosen search queries in observations), with unchanged sanitization logic. The read/action classification (classifyTool) is untouched.

My review is complete. I traced all the security-critical paths and verified the prior build-breaking finding is resolved.

Review Summary

I reviewed this discovery-only PR at HEAD (d45b0ab) against the base 50ac3ef. This branch contains no workshop-backend/workshop-shared kernel changes — the revalidation/invalidation work was correctly split to #198, resolving the maintainer's review question. I read every line of the security-critical mcp-shared paths and both connectors. pnpm isn't installed on this read-only tree, but the tests are extensive and I verified coverage of the key invariants by reading them.

The one prior actionable finding is resolved

The most recent bonk review found a build-breaking type error: looksLikePortal callers in gatekeeper-mcp-portal/src/portal.ts still passed a bare boolean after the signature changed to require { truncated, cap }. At this HEAD, both call sites are fixed (portal.ts:401,441 pass { truncated, cap: MAX_TOOLS_PER_SERVER }), the import is present (portal.ts:29), and all four production callers (catalog.ts:134, mcp.ts:374, portal.ts:401,441) use the object form consistently.

Invariants I independently verified hold

• Discovery is genuinely bounded — per-McpClient scan counters (pages/tools/bytes, per-call since withClient mints a fresh client), include filtering before the count/byte budgets (client.ts:625-632), raw envelope bytes counted against the scan budget, facet-wide concurrency (4) + bounded queue (32) sharing one deadline (facet.ts:146-175), and HydratedTools entry/byte caps (200 / 1 MiB) with negative-result caching and LRU eviction.
• runDiscovery slot accounting is sound — a slot is either held by an active op or handed to a waiter, never double-counted; the timed-out waiter splices itself before rejecting and never entered the try/finally, so no slot leaks; clearTimeout/splice are mutually exclusive within the single-threaded event loop.
• 5001-tool page handled — #list inspects the bounded prefix before reporting the scan limit (client.ts:625-649), so findTool finds a target that is the first entry of an oversized page (test client-pagination.test.ts:315).
• Scope-validation relaxation is safe — the "truncated, cannot validate" error fires only when a named tool is genuinely absent (scope.ts:172-177); the byo path fetches with listMatchingToolIndex, which returns truncated:true when the scan budget is hit with a tool still missing (fail-closed).
• Portal self-widening guard intact — scopedCatalog decides isPortal from scope.serverId first (catalog.ts:133); the looksLikePortal object form preserves fail-open-toward-portal semantics.
• Untrusted-text discipline — MAX_QUERY_CHARS/MAX_TOOL_NAME_CHARS enforced at the session layer, toolQueryTerms dedupes via new Set, matchesToolQuery returns false for empty terms, clampToolSummary/clampAnnotations bound server text, and the search-query observation is plainInline-flattened.
• Overloads carry mutual never (schema-to-ts.ts:364,366) so a variable with both search and name is rejected (Maximo-Guk finding Bump vitest from 3.2.4 to 3.2.6 #2, resolved).
• base-types.ts/types.d.ts edited identically (only backtick escaping differs; pinned by base-types.test.ts); the dead HydratedTools.refresh()/public remember() are gone.

I found no actionable defects. No inline review submitted.

LGTM!

github run

[github-actions]

Preview: pr169-feature-mcp-p-e36b02f9

https://pr169-feature-mcp-p-e36b02f9-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[ndisidore]

Reviewed and looks good! Big changeset, but on the whole seems sound