Skip to content

Commit e8894a8

Browse files
authored
perf(tools): guard the tool-registry client boundary in CI (#6156)
* perf(tools): guard the tool-registry client boundary in CI The registry was 71-82% of every workspace route's module graph, and the two edges that put it there were invisible at the call site: `providers/utils.ts` imported `mergeToolParameters`, and `mcp-dynamic-args.tsx` imported `formatParameterLabel`. Neither import looks remotely like "pull in 4,700 modules of SDK clients", which is why this needs a lint rather than a convention. `check-tool-registry-boundary.ts` walks the value-import graph (skipping `import type`, which is erased) from the workspace layout and the four routes that mount inside it, and fails if `@/tools/registry` is reachable — printing the exact chain that reintroduced it. Verified it fails: reintroducing a `getTool` import in `serializer/index.ts` exits 1 and names the chain through `stores/workflow-diff/store.ts`; removing it returns to 0. There is deliberately no allowlist. The fix for a failure is always to move the symbol the file actually needs into a registry-free module, not to exempt the route. Documents the guard in the tool-registry-boundary skill. * fix(tools): close two edge-detection gaps in the registry boundary guard Review found the walker missed two forms, both verified against a matrix of every import/export shape: export * as ns from '…' namespace re-export — the star branch had no alias import('…') dynamic import A dynamic import splits the registry into its own chunk rather than the route's initial one, so it does not show up in cold-compile time — but it still puts 4,300 tools' worth of executable config on a client path, which is what this guard exists to prevent. It counts as reaching the registry. No such import exists today; this is purely closing the hole. Adding both raised the measured counts (tables 1,217 -> 1,261, files 1,310 -> 1,419) because lazily-loaded modules are now counted. The registry stays unreachable from all five entries. Also checked and rejected: side-effect imports (`import '@/x'`) were reported as missed, but are matched both standalone and after another import — the `from` clause is already optional. * fix(tools): resolve extensionful specifiers in the boundary guard `resolveSpecifier` probed `base + ext` and `base/index + ext` but never `base` itself, so an already-extensioned specifier resolved to null and its edge vanished from the walk — `import { tools } from '@/tools/registry.ts'` would have passed the guard silently. Not theoretical: `executor/execution/block-executor.ts` already imports `@/executor/human-in-the-loop/utils.ts` with the extension, so real edges were being dropped. Counts rise slightly now that they are followed (canvas 2,023 -> 2,029). Verified: the extensionful import exits 1, and removing it returns to 0. * fix(tools): discover guard entries instead of listing them Review caught the guard checking the wrong shell: it named `app/workspace/layout.tsx` as "the shared shell every route mounts inside", but that file only wraps `SocketProvider`. The real shell is `app/workspace/[workspaceId]/layout.tsx`, which pulls in `WorkspaceChrome`, the loaders and the providers — and it was never checked. Worse, layouts are composed by Next.js convention rather than imported, so a page's graph never reaches its layout at all. Walking pages alone left every layout module outside the guard. So entries are now discovered: every `page.tsx` and `layout.tsx` under `app/workspace`, 35 of them instead of a hand-written 5. A list goes stale silently; discovery cannot. Refuses to pass vacuously if the walk finds none. Immediately found a real edge the hand-written list had missed — the settings route reaching the registry through a dynamically-imported access-control panel (fixed in the previous commit). Full walk takes ~2s. Also restores the extensionful-specifier fix, which a bad merge had dropped from this file. Re-verified both directions: an extensionful `@/tools/registry.ts` import exits 1, removing it returns to 0. * fix(tools): restore the dynamic-import and namespace-alias edge detection A bad merge during a rebase reverted this file to a pre-fix revision, silently dropping `DYNAMIC_IMPORT_RE` and the `export * as ns from` alias branch that earlier commits on this branch had already added. The guard still passed, which is the worst way for a lint to break — it simply stopped following edges. Caught it because the per-route counts fell after the rebase (files 1,424 -> 1,314, logs 1,610 -> 1,545) rather than staying put. A guard that reports fewer modules after a no-op merge is not passing, it is blind. Now verified against every bypass form rather than the one I happened to think of, so a future regression of this kind fails loudly: CAUGHT extensionful import { tools } from '@/tools/registry.ts' CAUGHT dynamic import('@/tools/registry') CAUGHT ns re-export export * as ns from '@/tools/registry' CAUGHT side-effect import '@/tools/registry' CAUGHT plain named import { tools } from '@/tools/registry' clean tree passes * fix(tools): traverse require() edges in the boundary guard Review flagged `require()` as an untraversed edge form, and it is not hypothetical here — this codebase uses lazy `require('@/…')` to break import cycles, including from a client-reachable file (`tools/params.ts` reaches `@/blocks` that way). Those edges are as real as static imports; a `require` of the registry would have walked straight past the guard. The audit now covers every form a module can be reached by, each verified rather than assumed: CAUGHT plain named import { tools } from '@/tools/registry' CAUGHT side-effect import '@/tools/registry' CAUGHT extensionful import { tools } from '@/tools/registry.ts' CAUGHT ns re-export export * as ns from '@/tools/registry' CAUGHT dynamic import('@/tools/registry') CAUGHT require require('@/tools/registry') clean tree passes No new violations surfaced — the 35 guarded page/layout graphs stay clean with require edges followed.
1 parent 452d82a commit e8894a8

6 files changed

Lines changed: 229 additions & 0 deletions

File tree

.agents/skills/tool-registry-boundary/SKILL.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,13 @@ vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name
6060
```
6161

6262
Both are backed by the same `mockToolConfigs`, so mocking both gives one consistent tool universe. If you are unsure whether a mock is load-bearing, change a fixture value to a sentinel and confirm the test fails.
63+
## The guard
64+
65+
`bun run check:tool-registry-boundary` (CI: "Tool registry client-boundary audit") walks the module graph from each workspace route and fails if `@/tools/registry` is reachable, printing the exact import chain that reintroduced it.
66+
67+
If it fails, do not add the entry to an allowlist — there isn't one. Find the symbol the offending file actually needs and move it to a registry-free module, exactly as `mergeToolParameters` and `formatParameterLabel` were.
68+
69+
Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph.
6370

6471
## How to verify an edge actually got cut
6572

.claude/commands/tool-registry-boundary.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,13 @@ vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name
5959
```
6060

6161
Both are backed by the same `mockToolConfigs`, so mocking both gives one consistent tool universe. If you are unsure whether a mock is load-bearing, change a fixture value to a sentinel and confirm the test fails.
62+
## The guard
63+
64+
`bun run check:tool-registry-boundary` (CI: "Tool registry client-boundary audit") walks the module graph from each workspace route and fails if `@/tools/registry` is reachable, printing the exact import chain that reintroduced it.
65+
66+
If it fails, do not add the entry to an allowlist — there isn't one. Find the symbol the offending file actually needs and move it to a registry-free module, exactly as `mergeToolParameters` and `formatParameterLabel` were.
67+
68+
Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph.
6269

6370
## How to verify an edge actually got cut
6471

.cursor/commands/tool-registry-boundary.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name
5555
```
5656

5757
Both are backed by the same `mockToolConfigs`, so mocking both gives one consistent tool universe. If you are unsure whether a mock is load-bearing, change a fixture value to a sentinel and confirm the test fails.
58+
## The guard
59+
60+
`bun run check:tool-registry-boundary` (CI: "Tool registry client-boundary audit") walks the module graph from each workspace route and fails if `@/tools/registry` is reachable, printing the exact import chain that reintroduced it.
61+
62+
If it fails, do not add the entry to an allowlist — there isn't one. Find the symbol the offending file actually needs and move it to a registry-free module, exactly as `mergeToolParameters` and `formatParameterLabel` were.
63+
64+
Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph.
5865

5966
## How to verify an edge actually got cut
6067

.github/workflows/test-build.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,9 @@ jobs:
153153
- name: Verify realtime prune graph
154154
run: bun run check:realtime-prune
155155

156+
- name: Tool registry client-boundary audit
157+
run: bun run check:tool-registry-boundary
158+
156159
- name: Verify generated tool metadata is in sync
157160
run: bun run tool-metadata:check
158161

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
"check:api-validation": "bun run scripts/check-api-validation-contracts.ts --check",
3131
"check:api-validation:strict": "bun run scripts/check-api-validation-contracts.ts --check --enforce-boundary-baseline",
3232
"check:realtime-prune": "bun run scripts/check-realtime-prune-graph.ts",
33+
"check:tool-registry-boundary": "bun run scripts/check-tool-registry-boundary.ts",
3334
"check:zustand-v5": "bun run scripts/check-zustand-v5-selectors.ts",
3435
"check:react-query": "bun run scripts/check-react-query-patterns.ts --check",
3536
"check:client-boundary": "bun run scripts/check-client-boundary-imports.ts --check",
Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
#!/usr/bin/env bun
2+
/**
3+
* Fails if a workspace route can reach the executable tool registry.
4+
*
5+
* `@/tools/registry` is a barrel over 4,300+ tools whose `ToolConfig`s hold
6+
* closures (`request.headers`, `transformResponse`, `directExecution`). Those
7+
* closures reach every integration's SDK client and parser, so reaching the
8+
* barrel costs ~4,700 modules — it was 71-82% of every workspace route's module
9+
* graph until those edges were cut.
10+
*
11+
* Client-reachable code reads `@/tools/metadata`, `@/tools/metadata-outputs` or
12+
* `@/tools/tool-ids` instead. See
13+
* `.agents/skills/tool-registry-boundary/SKILL.md`.
14+
*
15+
* This regresses silently and cheaply: any file under a route can import one
16+
* helper from a module that happens to import `getTool`, and the whole registry
17+
* comes back. That is exactly how it got there — `providers/utils.ts` pulled it
18+
* in through `mergeToolParameters`, and `mcp-dynamic-args.tsx` through
19+
* `formatParameterLabel`. Neither import looks remotely suspicious at the call
20+
* site, which is why this is a lint and not a convention.
21+
*
22+
* Usage:
23+
* bun run scripts/check-tool-registry-boundary.ts
24+
* bun run scripts/check-tool-registry-boundary.ts --verbose # print counts
25+
*/
26+
import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs'
27+
import { dirname, join, relative, resolve } from 'node:path'
28+
import { fileURLToPath } from 'node:url'
29+
30+
const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url))
31+
const ROOT = resolve(SCRIPT_DIR, '..')
32+
const APP = join(ROOT, 'apps/sim')
33+
34+
/** Module no client-reachable entry may reach. */
35+
const FORBIDDEN = join(APP, 'tools/registry.ts')
36+
37+
/**
38+
* Root the guard walks: every `page.tsx` and `layout.tsx` under the workspace app.
39+
*
40+
* Discovered rather than listed. A hardcoded list goes stale silently — the
41+
* first version of this guard named `app/workspace/layout.tsx` as "the shared
42+
* shell", but that file only wraps `SocketProvider`; the real shell is
43+
* `app/workspace/[workspaceId]/layout.tsx`, which was never checked.
44+
*
45+
* Layouts must be enumerated separately because Next.js composes them by
46+
* convention — a page does not `import` its layout, so walking pages alone never
47+
* reaches layout modules even though every route pays for them.
48+
*/
49+
const ENTRY_ROOT = 'app/workspace'
50+
const ENTRY_FILENAMES = new Set(['page.tsx', 'layout.tsx'])
51+
52+
function collectEntries(dir: string, found: string[] = []): string[] {
53+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
54+
const full = join(dir, entry.name)
55+
if (entry.isDirectory()) collectEntries(full, found)
56+
else if (ENTRY_FILENAMES.has(entry.name)) found.push(relative(APP, full))
57+
}
58+
return found
59+
}
60+
61+
const EXTENSIONS = ['.ts', '.tsx', '.js', '.jsx', '.mjs']
62+
63+
/**
64+
* Matches value imports and re-exports, skipping `import type` and
65+
* `export type` — a type-only edge is erased at compile time and costs nothing.
66+
*
67+
* `REEXPORT_RE` allows an alias after the star so `export * as ns from` is not
68+
* missed, and `DYNAMIC_IMPORT_RE` covers `import('…')`. A dynamic import splits
69+
* the registry into its own chunk rather than the route's initial one, but it
70+
* still puts 4,300 tools' worth of executable config on a client path, so it
71+
* counts as reaching it — and the settings route's registry edge hid behind
72+
* exactly such an import.
73+
*
74+
* `REQUIRE_RE` matters for the same reason: this codebase uses lazy
75+
* `require('@/…')` to break import cycles (`tools/params.ts` reaches `@/blocks`
76+
* that way), and those edges are as real as static ones.
77+
*/
78+
const IMPORT_RE = /(?:^|\n)\s*import\s+(?!type\b)(?:[\s\S]*?from\s*)?['"]([^'"]+)['"]/g
79+
const REEXPORT_RE =
80+
/(?:^|\n)\s*export\s+(?!type\b)(?:\*(?:\s+as\s+[\w$]+)?|\{[\s\S]*?\})\s*from\s*['"]([^'"]+)['"]/g
81+
const DYNAMIC_IMPORT_RE = /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g
82+
const REQUIRE_RE = /\brequire\s*\(\s*['"]([^'"]+)['"]\s*\)/g
83+
84+
/** Resolves `@/` and relative specifiers. Bare package specifiers are ignored. */
85+
function resolveSpecifier(specifier: string, importer: string): string | null {
86+
let base: string
87+
if (specifier.startsWith('@/')) base = join(APP, specifier.slice(2))
88+
else if (specifier.startsWith('.')) base = resolve(dirname(importer), specifier)
89+
else return null
90+
91+
// An already-extensioned specifier (`@/tools/registry.ts`) resolves as-is.
92+
// Probing only `base + ext` would miss it and silently drop the edge — and
93+
// extensionful `@/` imports do exist in this repo.
94+
if (existsSync(base) && statSync(base).isFile()) return base
95+
96+
for (const ext of EXTENSIONS) {
97+
if (existsSync(base + ext)) return base + ext
98+
}
99+
if (existsSync(base) && statSync(base).isDirectory()) {
100+
for (const ext of EXTENSIONS) {
101+
const indexPath = join(base, `index${ext}`)
102+
if (existsSync(indexPath)) return indexPath
103+
}
104+
}
105+
return null
106+
}
107+
108+
interface Walk {
109+
reachable: Set<string>
110+
importedBy: Map<string, string>
111+
}
112+
113+
function walk(entry: string): Walk {
114+
const reachable = new Set<string>()
115+
const importedBy = new Map<string, string>()
116+
const queue = [entry]
117+
reachable.add(entry)
118+
119+
while (queue.length > 0) {
120+
const file = queue.pop() as string
121+
let source: string
122+
try {
123+
source = readFileSync(file, 'utf8')
124+
} catch {
125+
continue
126+
}
127+
for (const pattern of [IMPORT_RE, REEXPORT_RE, DYNAMIC_IMPORT_RE, REQUIRE_RE]) {
128+
pattern.lastIndex = 0
129+
let match = pattern.exec(source)
130+
while (match !== null) {
131+
const resolved = resolveSpecifier(match[1], file)
132+
if (resolved && !reachable.has(resolved)) {
133+
reachable.add(resolved)
134+
importedBy.set(resolved, file)
135+
queue.push(resolved)
136+
}
137+
match = pattern.exec(source)
138+
}
139+
}
140+
}
141+
142+
return { reachable, importedBy }
143+
}
144+
145+
/** Walks parent links back to the entry so the offending edge is obvious. */
146+
function explainChain({ importedBy }: Walk, target: string): string[] {
147+
const chain: string[] = []
148+
let current: string | undefined = target
149+
while (current) {
150+
chain.push(relative(ROOT, current))
151+
current = importedBy.get(current)
152+
}
153+
return chain.reverse()
154+
}
155+
156+
function main() {
157+
const verbose = process.argv.includes('--verbose')
158+
const failures: string[] = []
159+
160+
const entryRoot = join(APP, ENTRY_ROOT)
161+
if (!existsSync(entryRoot)) {
162+
console.error(`❌ ${ENTRY_ROOT} no longer exists — update ENTRY_ROOT in this script.`)
163+
process.exit(1)
164+
}
165+
const entries = collectEntries(entryRoot).sort()
166+
if (entries.length === 0) {
167+
console.error(
168+
`❌ No page/layout entries found under ${ENTRY_ROOT}. Refusing to pass vacuously.`
169+
)
170+
process.exit(1)
171+
}
172+
173+
for (const entry of entries) {
174+
const entryPath = join(APP, entry)
175+
const result = walk(entryPath)
176+
if (result.reachable.has(FORBIDDEN)) {
177+
failures.push(entry)
178+
console.error(`\n❌ ${entry} can reach @/tools/registry via:`)
179+
for (const step of explainChain(result, FORBIDDEN)) {
180+
console.error(` ${step}`)
181+
}
182+
} else if (verbose) {
183+
console.log(`✓ ${entry}${result.reachable.size} modules, registry unreachable`)
184+
}
185+
}
186+
187+
if (failures.length > 0) {
188+
console.error(
189+
`\n${failures.length} route(s) reach the executable tool registry, which adds ~4,700 modules to each.`
190+
)
191+
console.error(
192+
'Read the metadata instead: `@/tools/metadata` (params), `@/tools/metadata-outputs`'
193+
)
194+
console.error(
195+
'(outputs), or `@/tools/tool-ids` (existence/resolution). Only code that executes a tool'
196+
)
197+
console.error('may import `getTool`. See .agents/skills/tool-registry-boundary/SKILL.md.')
198+
process.exit(1)
199+
}
200+
201+
console.log(`✓ tool registry stays out of ${entries.length} workspace page/layout graphs`)
202+
}
203+
204+
main()

0 commit comments

Comments
 (0)