perf(tools): read tool metadata instead of the registry on client paths - #6155
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview A new Call sites switch off the registry: block output inference uses The metadata sync script emits Reviewed by Cursor Bugbot for commit 7e38861. Configure here. |
Greptile SummaryThis PR removes the remaining workspace-client dependencies on the executable tool registry by routing existence, parameter, and output lookups through generated metadata while preserving version resolution.
Confidence Score: 5/5The PR appears safe to merge; no concrete changed-code failure remains after checking resolver parity and the contracts of migrated consumers. The metadata APIs preserve direct-ID and latest-version resolution, generated data includes every field used by the migrated paths, and custom or MCP behavior remains unchanged.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/tool-ids.ts | Adds static ID lookup and unversioned-to-latest resolution matching the executable registry’s existing semantics. |
| scripts/sync-tool-metadata.ts | Extends metadata generation and check mode to produce and verify the generated tool-ID artifact. |
| apps/sim/tools/metadata.ts | Routes metadata and parameter lookup through version-aware ID resolution. |
| apps/sim/tools/metadata-outputs.ts | Adds version-aware output metadata lookup without importing executable tool definitions. |
| apps/sim/tools/params.ts | Replaces full registry lookups with sufficient serializable metadata for current parameter UI consumers. |
| apps/sim/serializer/index.ts | Validates user-only required parameters from generated parameter metadata while preserving existing behavior. |
| apps/sim/lib/workflows/sanitization/validation.ts | Uses the generated ID set for built-in tool existence validation while retaining custom and MCP exemptions. |
| apps/sim/lib/workflows/blocks/block-outputs.ts | Reads generated output definitions instead of loading full executable tool configurations. |
| apps/sim/tools/metadata.test.ts | Adds coverage for unknown IDs and registry-compatible version resolution across metadata artifacts. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Registry[Executable tool registry] --> Generator[sync-tool-metadata]
Generator --> IDs[Generated tool IDs]
Generator --> Metadata[Generated params / OAuth metadata]
Generator --> Outputs[Generated output metadata]
IDs --> Validation[Workflow tool validation]
IDs --> Resolver[Version-compatible ID resolver]
Resolver --> Metadata
Resolver --> Outputs
Metadata --> Serializer[Serializer field validation]
Metadata --> ParamsUI[Tool parameter UI helpers]
Outputs --> BlockOutputs[Block output discovery]
Reviews (1): Last reviewed commit: "perf(tools): read tool metadata instead ..." | Re-trigger Greptile
c926582 to
3dff815
Compare
Production bundle — the open question is now settledI flagged in the scoping that it was unknown whether prod tree-shaking already removed the registry, which would have made this a dev/CI-only win. It doesn't. Full
That is a net reduction — it already absorbs the ~8 MB of generated metadata artifacts this stack adds. The registry's executable config was genuinely shipping to browsers. So the win is three-fold, not one:
|
Behaviour-neutrality evidenceSince this swaps the data source under four call sites, "tests pass" felt insufficient. Differentially compared the old registry reads against the new metadata reads for every tool, on the real registry: Those five dimensions are exactly what the rewired call sites read: Plus:
I did attempt an in-browser click-through of the tool-input panel as well; the Chrome extension wasn't responding, so that specific check is not done. The differential above covers the same code paths at the data level, but flagging it rather than implying otherwise. |
3dff815 to
6990c7c
Compare
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7e38861. Configure here.
7e38861 to
6fc7c18
Compare
6fc7c18 to
0391495
Compare
0391495 to
d03f6cb
Compare
c79ab21 to
46fa6f2
Compare
3f4021b to
7d7d4e6
Compare
7d7d4e6 to
a4a9d34
Compare
a4a9d34 to
69852d2
Compare
Cuts the last four edges that pulled `@/tools/registry` into the workspace shell. Every workspace route drops ~4,700 modules: route before after /w (canvas) 6,592 1,908 -71% /logs 6,227 1,543 -75% /tables 5,903 1,217 -79% /files 5,996 1,310 -78% workspace layout 5,751 1,063 -82% Dev cold compile of the canvas, n=3, cache cleared between runs: before 32.3s / 31.4s / 30.1s RSS 9.0-12.5 GB after 22.4s / 22.2s / 21.6s RSS 7.8-9.2 GB That lands where the `dev:minimal` escape hatch measured (20.0s / 6.7 GB) without its downside — `dev:minimal` swaps in curated registries that drop ~250 services, whereas this keeps every tool working. Rewired: - `block-outputs` -> `getToolOutputsMetadata` (needed `outputs`) - `serializer` -> `getToolParams` (needed `params`) - `validation` -> `hasToolId` (needed existence only) - `tools/params` -> `getToolMetadata` (needed `params`, `oauth`, `name`) `tools/params.ts` was the stubborn one: `mcp-dynamic-args.tsx` imports only `formatParameterLabel` from it, so the whole registry rode in behind a string helper — the same shape as the `mergeToolParameters` edge cut earlier. Adds a third generated artifact, `tool-ids.ts` (~110 KB). Resolution needs only the key set, so `@/tools/metadata` and `@/tools/metadata-outputs` both resolve through it and stay independent of each other, and an existence check costs ~110 KB instead of ~4 MB. Behaviour preservation was the risk here: `getTool` resolves an unversioned name onto its newest version, and a plain key lookup would have silently reported 246 versioned tools as missing. `resolveToolId` is reproduced against the id set and differentially tested — 4,404 probes (every id, every stripped base name, and an unknown) comparing old vs new resolution and existence: 0 mismatches. `ToolWithParameters.toolConfig` and `SubBlocksForToolInput.toolConfig` narrow from `ToolConfig` to `ToolMetadata`. The only external reader is `tool-input.tsx`, which uses `.name`.
The skill still routed `hasToolMetadata` and `getToolIds` to `@/tools/metadata`, but this PR moved id resolution into `@/tools/tool-ids`. Left as-is it would send the next caller to the 4 MB module for an existence check that costs 110 KB — the exact mistake the skill exists to prevent. Also records the two properties a caller can silently get wrong: lookups guard with `Object.hasOwn` (a bare bracket lookup returns inherited prototype members), and they resolve unversioned names (246 tools are versioned, and a plain lookup reports them missing rather than crashing).
…est mocks Two findings from review, both real. The settings route still reached the registry: settings/[section]/page.tsx -> settings.tsx -> (dynamic import) ee/access-control/components/access-control.tsx -> group-detail.tsx -> tools/utils.ts -> tools/registry.ts It reads `getTool(id)?.name` — metadata — so it moves to `getToolMetadata`. The earlier audit missed it because it walked only from the canvas route, and the edge hides behind a dynamic `import()` that a static walk skips. Serializer tests mocked the wrong module. `Serializer` now reads params via `getToolParams` from `@/tools/metadata`, but the tests still only mocked `@/tools/utils`, so they controlled nothing and passed because the real generated artifacts happen to agree with the fixtures. Adds `toolsMetadataMock` to `@sim/testing/mocks`, backed by the same `mockToolConfigs` as `toolsUtilsMock` so a test mocking both sees one consistent tool universe, and mocks it in the three serializer suites. Verified the mock is now load-bearing: pointing it at a sentinel param makes the three user-only-required validation tests fail, and restoring it returns all 110 serializer tests to green. Before this they passed either way.
`getToolIds()` returned the module's internal array by reference, so a caller doing `getToolIds().sort()` would reorder it in place and silently corrupt every later lookup — the in-place-mutation footgun `.claude/rules/sim-react-performance.md` calls out. Frozen rather than copied: the array is consumed in loops, so copying would allocate on every call. Freezing makes the mutation throw instead of corrupt, and `[...getToolIds()].sort()` still works. Return type is now `readonly string[]`, so the mistake is a compile error rather than a runtime surprise. No caller mutates it today; this is closing the hole, not fixing a live bug.
`resolveToolId` now exists twice on purpose — `@/tools/utils` resolves against the live registry (so a tool added before regeneration still resolves at runtime), `@/tools/tool-ids` against the generated id list (so client code resolves without importing 4,300 tools). Nothing structurally kept them in step; a change to versioning logic in one would silently drift from the other. `tool-metadata:check` now asserts they agree across every id, every stripped base name, and an unknown — 4,404 probes — and only after the staleness check passes, so a missing regeneration reports as staleness rather than as drift. Verified it fails: breaking resolution for `gmail*` exits 1; restoring it passes. It cannot live in a vitest suite. `vitest.setup.ts` globally mocks `@/tools/registry` to an empty map, so `getTool` resolves nothing there — a parity test written as a spec passes or fails for the wrong reason. Both facts are recorded where the code is. Both resolvers stay exported. An earlier pass here un-exported the `@/tools/utils` one as dead; `tools/utils.server.ts` imports it through a multi-line import that a grep missed, and `tsc` caught it. Its doc now says which resolver a caller should reach for instead of leaving two identically-named functions unexplained.
69852d2 to
def01f8
Compare
This is the payoff PR. It cuts the last four edges pulling
@/tools/registryinto the workspace shell.Module graph — every workspace route, not just the canvas
/w(canvas)/logs/tables/files@/tools/registryis now unreachable from all of them.Dev cold compile (canvas, n=3, cache cleared between runs)
That lands almost exactly where the
dev:minimalescape hatch measured (20.0s / 6.7 GB) — but without its downside.dev:minimalswaps in curated registries that drop ~250 services; this keeps every tool working.What was rewired
lib/workflows/blocks/block-outputsgetToolOutputsMetadataoutputsserializer/indexgetToolParamsparamslib/workflows/sanitization/validationhasToolIdtools/paramsgetToolMetadataparams,oauth,nametools/params.tswas the stubborn one.mcp-dynamic-args.tsximports onlyformatParameterLabelfrom it — so the entire registry rode in behind a string-formatting helper, the same shape as themergeToolParametersedge in #6152.Behaviour preservation — the actual risk
getToolresolves an unversioned name onto its newest version (notion_search→notion_search_v2). A plain key lookup would have silently reported 246 versioned tools as missing — a quiet correctness bug, not a crash.So
resolveToolIdis reproduced against the id set and differentially tested: 4,404 probes (every registered id, every stripped base name, and an unknown) comparing old vs new resolution and existence — 0 mismatches.Adds a third generated artifact,
tool-ids.ts(~110 KB). Resolution needs only the key set, so@/tools/metadataand@/tools/metadata-outputsboth resolve through it and stay independent of each other — and an existence check costs ~110 KB instead of ~4 MB.ToolWithParameters.toolConfigandSubBlocksForToolInput.toolConfignarrow fromToolConfigtoToolMetadata. The only external reader istool-input.tsx, which uses.name.Test plan
vitest run tools/ serializer/ lib/workflows/ providers/ executor/ app/— 8,953 passedtsc --noEmitcleantool-metadata:checkin syncOne pre-existing failure,
cloud-review-tools.test.ts, is environmental (shells out to argbinary not installed locally); verified identical on base.