perf(tools): move mergeToolParameters into a registry-free leaf module - #6152
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit 3ac64f2. Configure here. |
Greptile SummaryThe PR extracts plain parameter-merging helpers into a registry-free leaf module so provider and PI execution paths no longer load the tool registry through
Confidence Score: 5/5The PR appears safe to merge, with the moved implementation, consumers, exports, and dependency boundary remaining consistent. The extracted functions are behaviorally unchanged, all repository consumers use valid module paths, and the new leaf module has no runtime path back to the tool registry.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/merge-params.ts | Introduces the registry-free leaf containing the unchanged parameter-merging implementation; its runtime dependencies do not reach the tool registry. |
| apps/sim/tools/params.ts | Removes the relocated exports and imports isNonEmpty from the new module while retaining all remaining callers and behavior. |
| apps/sim/providers/utils.ts | Redirects parameter merging to the leaf module, removing this consumer's dependency edge through registry-owning tools/params. |
| apps/sim/executor/handlers/pi/sim-tools.ts | Redirects the PI tool-spec builder to the new declaration source without changing execution behavior. |
| apps/sim/tools/params.test.ts | Updates merge-helper tests to exercise the new module path while preserving existing coverage. |
Reviews (1): Last reviewed commit: "perf(tools): move mergeToolParameters in..." | Re-trigger Greptile
3ac64f2 to
2772db5
Compare
2772db5 to
05f887c
Compare
bcd5dbb to
11f67fb
Compare
11f67fb to
bb64f82
Compare
bb64f82 to
1577e79
Compare
`providers/utils.ts` imports exactly one thing from `@/tools/params`: `mergeToolParameters`. That function performs no tool lookup at all — it merges two plain param objects. But `params.ts` imports `getTool` from `@/tools/utils`, which statically imports the 4,300-entry `@/tools/registry` barrel, so that one-symbol import was dragging the entire tool registry into every module graph that reached it. Measured with a module-graph walk from each entry: tools/params.ts 4,926 modules (registry reachable) providers/utils.ts 4,926 modules (registry reachable) -> 22 modules ✅ tools/merge-params.ts 2 modules (registry NOT reachable) `mergeToolParameters`, `deepMergeInputMapping` and `isNonEmpty` move to `@/tools/merge-params`, which is forbidden from importing `@/tools/utils`, `@/tools/registry` or `@/tools/params`. `params.ts` now imports `isNonEmpty` from there; its `isRecordLike` and `isEmptyTagValue` imports became unused and are dropped. The two consumers (`providers/utils.ts`, `executor/handlers/pi/sim-tools.ts`) import from the new module directly rather than via a re-export, per the no-re-exports rule. This is preparation, not the payoff. The canvas route still reaches the registry through three other edges (block-outputs, serializer, sanitization/validation) — all four are redundant paths and must all be cut before the route's module count moves. Those follow in the metadata-manifest PRs. Behaviour is unchanged: the moved functions are copied verbatim.
It was exported from `@/tools/params` and imported by nothing — a private helper of `mergeToolParameters` that had leaked into the public surface. Since this move created the module, the export goes with it rather than being carried forward. Verified zero consumers repo-wide before dropping it.
1577e79 to
41182a3
Compare
Why
providers/utils.tsimports exactly one symbol from@/tools/params—mergeToolParameters. That function does no tool lookup at all; it merges two plain param objects.But
params.tsimportsgetToolfrom@/tools/utils, which statically imports the 4,300-entry@/tools/registrybarrel. So that one-symbol import was dragging the entire tool registry — and every tool's HTTP closures, SDK clients and transitive deps — into every module graph that reached it.Measurement
Module-graph walk from each entry (imports + re-exports,
import typeexcluded):providers/utils.tstools/params.tsgetTool)tools/merge-params.tsWhat moved
mergeToolParameters,deepMergeInputMappingandisNonEmpty→@/tools/merge-params, copied verbatim. The new module is documented as forbidden from importing@/tools/utils,@/tools/registryor@/tools/params.params.tsnow importsisNonEmptyfrom there; itsisRecordLikeandisEmptyTagValueimports became unused and are dropped. Both consumers (providers/utils.ts,executor/handlers/pi/sim-tools.ts) import the new module directly rather than through a re-export, per the no-re-exports rule.This is preparation, not the payoff
The canvas route still reaches the registry through three other edges —
block-outputs,serializer, andsanitization/validation. All four are redundant paths, so the route's module count does not move until all of them are cut. That happens in the metadata-manifest PRs stacked on top of this one.Test plan
vitest run tools/ providers/ executor/handlers/pi/— 3,262 passedtsc --noEmitexit 0check:client-boundary,check:utils,check:boundariespassOne pre-existing failure,
executor/handlers/pi/cloud-review-tools.test.ts, is environmental — it shells out to argbinary that isn't installed on this machine. Verified it fails identically on the base commit with these changes stashed.