Feat: port ToolboxToolset from adk-python (MCP Toolbox for Databases) - #583
Open
AmaadMartin wants to merge 7 commits into
Open
Feat: port ToolboxToolset from adk-python (MCP Toolbox for Databases)#583AmaadMartin wants to merge 7 commits into
AmaadMartin wants to merge 7 commits into
Conversation
added 7 commits
August 3, 2026 10:54
The MCP Toolbox JavaScript SDK is declared as an optional peer plus a devDependency rather than a regular dependency: it depends on @google/adk itself, so a regular dependency would make the published graph cyclic. The devDependency is what makes types and test mocks resolve in-repo, mirroring Python's opt-in `google-adk[toolbox]` extra.
Ports adk-python's ToolboxToolset. Like the Python class, this is a thin adapter over the separately published toolbox SDK rather than a hand-rolled protocol client: the SDK is loaded through a dynamic import so the package stays optional, and a missing install surfaces as an actionable error. Loaded tools are re-wrapped in a module-private BaseTool adapter instead of being returned directly, so the tools carry this package's BaseTool brand even when the SDK resolved a different @google/adk copy.
Mocks @toolbox-sdk/adk end to end: the real package imports @google/adk at module scope, which resolves to the built core/dist and is absent on an unbuilt checkout. Covers both selectors and their union, identity-forwarding of auth token getters and bound params (asserting adk-js never resolves them itself), client laziness and memoisation, declaration passthrough, unwrapped propagation of server errors, and the missing-optional-peer message.
…rror The constant duplicated the import specifier, which must stay a literal for static analysis, so the two could drift apart.
The union guards read toolNames?.length, so [] behaves like omission. No other test distinguishes that from an === undefined check, which would silently return no tools at all.
`@toolbox-sdk/adk`'s declarations import `@google/adk`, which the workspace symlink resolves to core/dist/types. tsc then sees that directory as both an input and an output of the same run and refuses to emit, so `npm run build` failed with 151 TS5055 errors on every build after the first. CI never saw it because a fresh checkout has no dist. Mapping the specifier to the sources fixes it for core's own compilation. The mapping cannot live in the root tsconfig: dev/ genuinely depends on the built core, and redirecting it there fails with TS6059 rootDir violations.
…apping With the resolution bug fixed, the SDK's ToolboxTool is the same BaseTool this package declares, so the local adapter was pure indirection: the SDK already passes the core tool's name and description to super(), its runAsync is the same delegation, and its _getDeclaration does the zod conversion we wanted. The brand is Symbol.for(), a global registry, so isBaseTool() holds across duplicate package copies, and nothing on the getTools() consumption path uses instanceof (llm_agent.ts uses the symbol guard). Test fixtures now extend BaseTool, as the real SDK tools do; all 15 cases are kept. Also drops ToolboxBoundValue, which collapsed to exactly `unknown` (a union containing the top type absorbs its other arms), and moves the export off common.ts -- the browser barrel -- onto the node entrypoint beside MCPToolset, since the SDK is node-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: #issue_number
Related: #issue_number
Problem: adk-python ships
ToolboxToolset(src/google/adk/tools/toolbox_toolset.py), which lets an ADK agent consume the tools served by a running MCP Toolbox for Databases server. adk-js has no equivalent —Toolboxreturns zero hits acrosscore/src,dev/srcandintegrations/src. TypeScript users have no way to reach a Toolbox server short of hand-rolling a client.Solution: Add
ToolboxToolset, aBaseToolsetexported from@google/adk.The most important design fact is that the Python class is not a protocol implementation — it is a ~110-line delegate around the separately published
toolbox-adkPyPI package. This port keeps that shape: a thin adapter over the first-party JavaScript SDK (@toolbox-sdk/adk, published by Google fromgoogleapis/mcp-toolbox-sdk-js), not a hand-rolled Toolbox/MCP client.Design decisions worth a reviewer's attention:
The SDK is an optional peer + dev dependency, never a regular dependency.
@toolbox-sdk/adkitself depends on@google/adk, so making it a regular dependency ofcorecreates a package cycle. It is loaded through a dynamicimport()inside atry/catch, and its type imports are allimport typein private positions — the emittedcore/dist/types/tools/toolbox_toolset.d.tscontains noimportof the SDK, so consumers who never install it see no dangling type reference.peerDependenciesMeta.optionalis a new key incore/package.json; without it npm 7+ would auto-install the peer and drag the cycle into every consumer's tree. This mirrors Python's opt-inpip install google-adk[toolbox]extra.We depend on
@toolbox-sdk/adk, not@toolbox-sdk/core.@toolbox-sdk/coredeclareszod@^3.24.4as a peer, which conflicts with this repo'szod@^4.2.1and makesnpm installfail withERESOLVE.@toolbox-sdk/adkcarrieszod@3as a regular dependency, so npm nests it (verified:node_modules/@toolbox-sdk/adk/node_modules/{zod@3.25.76,@toolbox-sdk/core}alongside the repo'szod@4.4.3).Root-cause fix:
@google/adkno longer self-resolves to core's own build output.@toolbox-sdk/adk's declarations import@google/adk, which the workspace symlink resolves tocore/dist/types. tsc then treats that directory as both an input and an output of the same run and refuses to emit —npm run buildfailed with 151TS5055errors on every build after the first, while CI stayed green because a fresh checkout has nodist.core/tsconfig.jsonnow maps the specifier to the sources.Measured:
mainbuilds twice cleanly, this branch (before the fix) did not, and now does — three consecutivenpm run buildruns, 0 TS5055. The mapping deliberately lives incore/tsconfig.jsonrather than the root:dev/genuinely depends on the built core, and redirecting the specifier repo-wide failsdev's build withTS6059rootDir violations (measured, so rejected).getTools()returns the SDK's tools unwrapped. An earlier revision wrapped each tool in a localBaseTooladapter, because theunique symbolbrand onBaseToolmade the SDK's tools non-assignable (TS2322). That was a symptom of the resolution bug above; with the mapping in place it disappears, and the adapter was pure indirection — the SDK'sToolboxToolalready extendsBaseTool, already passes the core tool's name and description tosuper(), itsrunAsyncis the same delegation, and its_getDeclaration()performs the zod conversion. It is now deleted.Runtime safety was checked independently rather than assumed: the brand is
Symbol.for('google.adk.baseTool'), a global registry, soisBaseTool()holds even across duplicate package copies; and nothing on thegetTools()consumption path usesinstanceof(llm_agent.ts:322uses the symbol guard, andskill_toolset.ts'sinstanceofapplies to user-declared tools, not toolset results).The declaration and the execution both stay inside the SDK. adk-js never converts the tool's schema — it is a zod v3 object and this repo's zod-v4 helpers would mishandle it — and never invokes the core callable itself. Returning the tools unwrapped is what guarantees that.
Auth getters and bound params are forwarded by identity, never resolved here. That is what makes Python's documented "resolved per call, not captured once" semantics hold; the tests assert both that the objects are passed by reference and that adk-js never invokes the callables.
Exported from the node entrypoint, not the browser barrel.
core/src/index_web.tsis onlyexport * from './common.js', so anything incommon.tsis on the browser surface.@toolbox-sdk/adkdeclaresengines: {node: ">=20"}and pullsaxiosand@modelcontextprotocol/sdk, so the export lives incore/src/index.tsbesideMCPToolset, matching how every other node-only export in this package is placed.Cross-language parity notes (which rule was applied where the two conflict):
server_urlpositional;toolset_name/tool_names/… keyword argsserverUrlpositional + aToolboxToolsetOptionsbagget_tools(readonly_context)ignores the context (selection is server-side)getTools(_context)accepts and ignoresclose()forwards to the delegateToolboxClientexposes onlyloadTool/loadToolset— there is noclose/disposeand nothing to release. Precedent:AgentRegistrySingleMCPToolset.close()is likewise empty.ImportError("ToolboxToolset requires the 'toolbox-adk' package. …")Error("ToolboxToolset requires the '@toolbox-sdk/adk' package. …", {cause})causepreserves the underlying import failure.credentials: Optional[CredentialConfig]ToolboxClientconstructor is(url, session?, clientHeaders?, protocol?)— there is no credentials parameter and noCredentialConfigtype in its public typings. Rather than invent a stub option with no reader, it is left out.**kwargspassthroughCollision check (run before starting):
gh pr list --repo AmaadMartin/adk-js --state all --limit 1000returned zero PRs — open or closed — whose title or branch mentionstoolbox. The adjacent toolset PRs that do exist (#528 GCS, #529 ApiRegistry, #533/#534 Eventarc, #464/#465 APIHub, #462/#463 Google API Discovery, #578/#579 Application Integration, #104–#106 BigQuery/Bigtable/PubSub, and the MCP PRs #112/#141/#147/#580) each add their own files; none touchescore/src/tools/toolbox_toolset.ts. Branched frommain, not stacked.Known limitation, disclosed rather than hidden. The repo-level
npm run ts:check(roottsconfig.json, not part of the CI workflow) reports 281 pre-existing errors onmain, all of one class:@google/adkresolves tocore/dist/types, so theunique symbolbrand onBaseTool/BaseAgentmismatches the one incore/src. This PR makes it 282 — one further instance of that same pre-existing defect, atreturn sdkTools. Fixing it properly means putting the path mapping at the root, which breaksdev's build as described above, so it is out of scope here.npm run build,npm run lint,npm run format:checkandnpm run docs:check— the checks CI actually runs — are all clean.No unrequested extras: no
toolFilter/prefixoption (Python has neither —super([])), no@experimentaldecorator (MCPToolsetdoes not use it), and no docs pages or samples.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
core/test/tools/toolbox_toolset_test.ts— 15 tests,@toolbox-sdk/adkfully mocked. Mocking is mandatory rather than convenient: the real package imports@google/adkat module scope, which inside the workspace resolves tocore/dist/esmand is absent on an unbuilt checkout. The fixtures extendBaseTool, exactly as the real SDK's tools do.Coverage of the new file is 100% statements / 100% branches / 100% functions / 100% lines (
--coverage.include='core/src/tools/toolbox_toolset.ts'). No coverage suppression was added and no structure was weakened to reach it.Proof that the tests can fail (mutation testing). Coverage is a floor, not proof, so every test was run against deliberately broken source. 17 mutations were applied one at a time to
core/src/tools/toolbox_toolset.ts; every one was killed, and each of the 15 tests is killed by at least one:|| !toolNames?.lengtharm of the selector guardloadToolset(toolsetName, …)→loadToolset(undefined, …)if (true)toolNames === undefined(treat[]as an explicit selector)if (toolNames?.length)→if (false)authTokenGetters→{...authTokenGetters}(copy, not identity)boundParams→{...boundParams}undefinedinstead ofadditionalHeadersif (!this.client)→if (true)(drop memoisation)close()throwsgetToolsreturns[]when_contextis set.catch(() => {throw new Error('re-tagged')})aroundloadToolset{cause}from the missing-peer errortry/catchso the raw import error escapesR16 and R17 exist specifically because deleting the adapter changed what two of the tests pin: they now guard the passthrough invariant (adk-js must not wrap, rename, or intercept the SDK's tools) rather than adapter behaviour, and they are renamed accordingly. Without those two mutations they would only have been killed by the coarse R1. The source was diffed against a pristine copy afterwards to confirm no mutation was left behind.
Failure paths are exercised, not just the happy path: the missing-optional-peer error (message and the preserved
cause), and unwrapped propagation of a server/transport rejection.Manual End-to-End (E2E) Tests:
No automated e2e test is added, and this is deliberate rather than an omission: an e2e test would import the real
@toolbox-sdk/adk, which requires both a builtcore/distand a live MCP Toolbox server — neither is available to CI. Nothing was added undertests/integration/**ortests/e2e/**.Instead the feature was verified locally against the real SDK with no mocks anywhere: a throwaway stateless-MCP Toolbox server (plain
node:http, JSON-RPCtools/list+tools/callon/mcp/[toolset]) was run on loopback, the freshly built@google/adkand the real@toolbox-sdk/adkwere installed into a scratch package, andToolboxToolsetwas driven against it over real HTTP. All ten checks passed — before and after the adapter was removed, which is the evidence that removing it is behaviour-preserving:Notably this confirms the two behaviours the mocked tests can only assert by delegation: a bound parameter really is pre-filled on the wire and hidden from the
FunctionDeclaration, and an auth-token getter really is invoked once per call (id-token-1, thenid-token-2) and arrives as themy-google-auth_tokenheader.For a maintainer who wants to reproduce against a real server: run a local MCP Toolbox server on
127.0.0.1:5000,npm install @toolbox-sdk/adk, and confirmawait new ToolboxToolset('http://127.0.0.1:5000').getTools()returns the server's tools.Full local validation on the pushed commit:
Type-leak check on the emitted declarations:
grep -rE "from '@toolbox-sdk|import\(.@toolbox-sdk" core/dist/types/returns no files. Export-placement check:core/dist/types/index_web.d.tsandcore/dist/web/index_web.jscontain zeroToolboxreferences, whilecore/dist/types/index.d.tscarries the export.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.