perf(tools): generate serializable tool metadata artifacts - #6153
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Docs and agent skills/commands now require regenerating metadata after registry changes and add the Reviewed by Cursor Bugbot for commit 3480aee. Configure here. |
Greptile SummaryThe PR adds generated, serializable tool metadata and output artifacts plus typed accessors, synchronization tooling, tests, and a CI freshness gate. Major changes:
Confidence Score: 4/5The PR appears safe to merge, with two non-blocking hardening issues in special-key lookup behavior and the generator's bounded function scan. Existing projected data is JSON-safe and no production consumer is rewired yet, while the remaining concerns affect special unknown IDs and future schemas nested beyond the scanner's limit. Files Needing Attention: apps/sim/tools/metadata.ts, apps/sim/tools/metadata-outputs.ts, scripts/sync-tool-metadata.ts
|
| Filename | Overview |
|---|---|
| scripts/sync-tool-metadata.ts | Adds deterministic registry projection and serialization, but its function-value guard has an arbitrary depth limit despite recursively nestable schemas. |
| apps/sim/tools/metadata.ts | Adds lightweight metadata accessors; direct key lookup can return inherited Object.prototype properties for unknown special IDs. |
| apps/sim/tools/metadata-outputs.ts | Adds the isolated output-schema accessor and shares the inherited-key lookup weakness. |
| apps/sim/tools/metadata.test.ts | Covers artifact presence, representative lookups, null normalization, and top-level function exclusion. |
| .github/workflows/test-build.yml | Adds the generated metadata synchronization check to CI. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Registry[Executable tool registry] --> Generator[sync-tool-metadata.ts]
Generator --> Metadata[generated/tool-metadata.ts]
Generator --> Outputs[generated/tool-outputs.ts]
Metadata --> MetadataAPI[tools/metadata.ts]
Outputs --> OutputsAPI[tools/metadata-outputs.ts]
MetadataAPI --> FutureParams[Future params and existence consumers]
OutputsAPI --> FutureInference[Future output inference consumer]
CI[tool-metadata:check] --> Generator
Reviews (1): Last reviewed commit: "perf(tools): generate serializable tool ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 3480aee. Configure here.
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 3480aee. Configure here.
3480aee to
c6ff77a
Compare
Note on the 4/5 score aboveThat review ran at ~06:00 UTC against an earlier head; the branch is now at a later commit and Greptile has not re-run despite repeated triggers (it does not appear to re-review on force-push for these stacked bases). Flagging it so the stale score isn't read as an open concern. Its stated reason — inherited-key lookups ( The full Test and Build workflow was dispatched against the stack head and passed. Note that |
ed2ae30 to
ee8ee73
Compare
ee8ee73 to
244de24
Compare
387a2b7 to
59becf5
Compare
59becf5 to
7f543aa
Compare
Adds `scripts/sync-tool-metadata.ts`, which projects the executable tool registry down to the data half nobody needs a closure for, plus typed accessors over the result. No consumer is rewired yet — that is the next PR. `@/tools/registry` is a ~9,000-line barrel over 4,366 tools. Each `ToolConfig` mixes plain data (`params`, `outputs`, `name`) with closures (`request.headers`, `transformResponse`, `directExecution`, `postProcess`), and those closures reach every integration's SDK client and parser — which is why reaching the barrel costs ~4,700 modules. Every client-reachable caller was audited: none of them need a closure. They need `outputs`, `params`, or an existence check. Two artifacts, not one. `outputs` is ~4 MB of the ~8 MB and has a single consumer, so it is emitted separately and exposed from its own module; callers needing only params never load it. The data is a JSON string parsed at runtime rather than an imported `.json` or an object literal. That is not stylistic — with `resolveJsonModule` (enabled repo-wide) a `.json` import makes TypeScript infer a literal type for all 4,366 entries: tsc --noEmit, baseline 12.6s tsc --noEmit, with `.json` imports 8m07s (38x) tsc --noEmit, with string literals 12.0s An ambient `declare module` does not short-circuit it (measured: 8m18s), and an object literal is the same inference work. A single string literal is one cheap token for the compiler and the bundler, and `JSON.parse` beats evaluating the equivalent literal at runtime. The generator refuses to emit any function value, so shipping executable config to the client fails loudly instead of silently. `hosting` and `schemaEnrichment` are excluded on those grounds — both hold functions and are server-only. Also strips empty param entries: the registry has one (`stt_deepgram_v2`, an `undefined`) which crashes callers that read `param.type` while iterating. `JSON.stringify` drops `undefined` on its own, so the guard is there for an explicit `null` — which serializes faithfully and would reach consumers — and to warn either way. Wires `tool-metadata:check` into CI alongside the other generated-contract gates, and ignores the generated directory in biome (it exceeds the 1 MB limit and was being skipped with a notice on every commit). Adds a `tool-registry-boundary` skill covering which module to import, the three non-obvious properties of the artifacts, and how to verify an edge is actually cut — the canvas route reaches the registry through four redundant paths, so cutting one alone moves the module count by ~1.
Review found two real defects in the generated-metadata layer.
`JSON.parse` returns an object with the normal prototype, so a bare bracket
lookup resolved inherited members: `getToolMetadata('constructor')` returned a
*function* typed as `ToolMetadata`, and `getToolOutputsMetadata('toString')`
likewise — silently violating the accessors' documented "undefined if unknown"
contract. Guarded with `Object.hasOwn`, with a parameterised regression test
over `constructor`, `toString`, `valueOf`, `hasOwnProperty` and `__proto__`.
The generator's no-functions scan also gave up past ten levels of nesting. Param
and output schemas nest arbitrarily, so a deeper closure would have been dropped
silently by `JSON.stringify` while generation reported success — shipping an
incomplete schema and defeating the guarantee the scan exists to provide. The
depth cap is gone; a `WeakSet` handles the cycles that exposes.
A new tool now has a second registration step. Client code reads `params` and `outputs` from the generated artifacts rather than from the registry, so a tool added without regenerating them is registered but invisible to the UI — and CI fails on the stale artifacts. `add-tools` and `add-integration` are where someone actually adds a tool, so the step goes in both, next to the registry edit and in each checklist.
Adding a block alone needs no regeneration — it references existing tool IDs and changes no tool's shape. But a change that touches a tool alongside the block does, and this is where that is easy to miss: a block's `outputs` are authored to match its tools' outputs, and the UI now reads those from the generated metadata, so a stale artifact makes the block's declared outputs disagree with what the panel renders (and fails CI). Completes the tool-authoring surface alongside add-tools and add-integration.
The three tool-authoring skills said to regenerate after adding or changing a tool, but not after removing one. Removal is equally breaking and equally guarded: deleting a tool from `tools/registry.ts` without regenerating fails `tool-metadata:check` (verified — exit 1), so a contributor following the skill literally would have hit a CI failure the skill never warned about.
7f543aa to
380ba1a
Compare
1 participant
Adds the generator and the typed accessors. No consumer is rewired yet — that's the next PR, which is where the module-count win lands.
What and why
@/tools/registryis a ~9,000-line barrel over 4,366 tools. EachToolConfigmixes plain data (params,outputs,name) with closures (request.headers,transformResponse,directExecution,postProcess), and those closures reach every integration's SDK client and parser. That is why reaching the barrel costs ~4,700 modules.I audited every client-reachable caller: none needs a closure. They need
outputs,params, or an existence check.Two artifacts, not one
tools/generated/tool-metadata.ts{ name, description, version, params, oauth }tools/generated/tool-outputs.tsoutputsoutputsis over half the data and has a single consumer, so it gets its own module — callers needing only params never load it.The
.jsontrap (please don't "clean this up")The data is a JSON string parsed at runtime, not an imported
.jsonand not an object literal. WithresolveJsonModule— enabled repo-wide — a.jsonimport makes TypeScript infer a literal type for all 4,366 entries:tsc --noEmit.jsonimports.jsonimports + ambientdeclare moduleA generated object literal is the same inference work. A single string literal is one cheap token for both compiler and bundler, and
JSON.parsebeats evaluating the equivalent literal at runtime. The trade-off is that these files diff as one line — acceptable for a generated artifact nothing reads by eye and CI verifies wholesale. This is documented in the script and the skill.Safety properties
hostingandschemaEnrichmentare excluded on those grounds — both hold functions (hosting.enabled,pricing,enrichSchema) and are server-only.stt_deepgram_v2— which crashes callers that readparam.typewhile iterating. Worth a separate look: it's a latent data bug.tool-metadata:checkis wired into CI next to the other generated-contract gates.Test plan
tsc --noEmit12.0s (baseline 12.6s — no regression)tools/metadata.test.tscontains no null param entrieswent red; restored → green. Worth noting I first wrote that assertion in a form that couldn't fail (the real registry entry isundefined, whichJSON.stringifydrops for free), so the guard's rationale is now documented accurately.tool-metadata:checkpasses; regeneration is deterministic (sorted keys)gmail_sendparams/outputs resolveSkill
Adds
tool-registry-boundary— which module to import for which need, the three non-obvious artifact properties, and how to verify an edge is actually cut. The canvas route reaches the registry through four redundant paths, so cutting one alone moves the module count by ~1; measure the route, not the file you edited.