Skip to content

perf(tools): move mergeToolParameters into a registry-free leaf module - #6152

Merged
waleedlatif1 merged 2 commits into
perf/dev-fs-cachefrom
perf/tools-params-split
Aug 1, 2026
Merged

perf(tools): move mergeToolParameters into a registry-free leaf module#6152
waleedlatif1 merged 2 commits into
perf/dev-fs-cachefrom
perf/tools-params-split

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Stacked on #6151. Review that first; base retargets to staging once it merges.

Why

providers/utils.ts imports exactly one symbol from @/tools/paramsmergeToolParameters. That function does 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 — 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 type excluded):

entry before after
providers/utils.ts 4,926 (registry reachable) 22 (registry unreachable ✅)
tools/params.ts 4,926 4,927 (unchanged — still owns getTool)
tools/merge-params.ts 2 (registry unreachable ✅)

What moved

mergeToolParameters, deepMergeInputMapping and isNonEmpty@/tools/merge-params, copied verbatim. The new module is documented as 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. 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 edgesblock-outputs, serializer, and sanitization/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 passed
  • tsc --noEmit exit 0
  • check:client-boundary, check:utils, check:boundaries pass
  • biome clean

One pre-existing failure, executor/handlers/pi/cloud-review-tools.test.ts, is environmental — it shells out to a rg binary that isn't installed on this machine. Verified it fails identically on the base commit with these changes stashed.

@vercel

vercel Bot commented Aug 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 1, 2026 6:03pm

Request Review

@cursor

cursor Bot commented Aug 1, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Refactor-only import graph change with no intended behavior change; merge logic is copied verbatim and existing tests still cover mergeToolParameters.

Overview
Moves mergeToolParameters, deepMergeInputMapping, and isNonEmpty out of @/tools/params into a new @/tools/merge-params module (logic copied verbatim). params.ts only pulls in isNonEmpty from there and drops the unused isRecordLike / isEmptyTagValue imports.

providers/utils.ts and executor/handlers/pi/sim-tools.ts now import mergeToolParameters from merge-params instead of params, so callers that only need param merging no longer transitively load params.tsgetTool → the full @/tools/registry barrel. The new file is documented as forbidden from importing utils, registry, or params. params.test.ts imports mergeToolParameters from merge-params for the existing merge tests.

Reviewed by Cursor Bugbot for commit 3ac64f2. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The 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 tools/params.

  • Moves mergeToolParameters, deepMergeInputMapping, and isNonEmpty without behavioral changes.
  • Redirects direct consumers and tests to @/tools/merge-params.
  • Keeps tools/params functionality intact by importing isNonEmpty from the new module.

Confidence Score: 5/5

The 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.

Important Files Changed

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

`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.
@waleedlatif1
waleedlatif1 force-pushed the perf/tools-params-split branch from 1577e79 to 41182a3 Compare August 1, 2026 18:03
@waleedlatif1
waleedlatif1 merged commit 9299cee into staging Aug 1, 2026
25 checks passed
@waleedlatif1
waleedlatif1 deleted the perf/tools-params-split branch August 2, 2026 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant