Skip to content

Commit d03f6cb

Browse files
committed
fix(tools): cut the settings-route registry edge and fix serializer test 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.
1 parent f8a9f98 commit d03f6cb

9 files changed

Lines changed: 66 additions & 5 deletions

File tree

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,19 @@ Three non-obvious properties, each of which was measured and is easy to undo by
4848
- **Empty param entries are stripped.** The registry contains one (`stt_deepgram_v2`), which crashes callers that read `param.type` while iterating.
4949
- **Lookups resolve versions.** `getTool` maps an unversioned name onto the newest version, and 246 tools are versioned. A plain key lookup would silently report them missing — a quiet correctness bug, not a crash. `resolveToolId` reproduces that against the id set and is differentially tested against the original.
5050

51+
## Testing code that reads tool metadata
52+
53+
Mock the module the code under test actually reads. `vi.mock('@/tools/utils', () => toolsUtilsMock)` only controls `getTool`; code that reads `params`/`outputs`/`name` goes through `@/tools/metadata`, so mocking `tools/utils` there is a **no-op that still passes** — because the real generated artifacts happen to agree with the mock fixtures. The test looks green while controlling nothing.
54+
55+
```ts
56+
import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks'
57+
58+
vi.mock('@/tools/utils', () => toolsUtilsMock) // executable lookup
59+
vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name
60+
```
61+
62+
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+
5164
## How to verify an edge actually got cut
5265

5366
Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph:

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,19 @@ Three non-obvious properties, each of which was measured and is easy to undo by
4747
- **Empty param entries are stripped.** The registry contains one (`stt_deepgram_v2`), which crashes callers that read `param.type` while iterating.
4848
- **Lookups resolve versions.** `getTool` maps an unversioned name onto the newest version, and 246 tools are versioned. A plain key lookup would silently report them missing — a quiet correctness bug, not a crash. `resolveToolId` reproduces that against the id set and is differentially tested against the original.
4949

50+
## Testing code that reads tool metadata
51+
52+
Mock the module the code under test actually reads. `vi.mock('@/tools/utils', () => toolsUtilsMock)` only controls `getTool`; code that reads `params`/`outputs`/`name` goes through `@/tools/metadata`, so mocking `tools/utils` there is a **no-op that still passes** — because the real generated artifacts happen to agree with the mock fixtures. The test looks green while controlling nothing.
53+
54+
```ts
55+
import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks'
56+
57+
vi.mock('@/tools/utils', () => toolsUtilsMock) // executable lookup
58+
vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name
59+
```
60+
61+
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+
5063
## How to verify an edge actually got cut
5164

5265
Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph:

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,19 @@ Three non-obvious properties, each of which was measured and is easy to undo by
4343
- **Empty param entries are stripped.** The registry contains one (`stt_deepgram_v2`), which crashes callers that read `param.type` while iterating.
4444
- **Lookups resolve versions.** `getTool` maps an unversioned name onto the newest version, and 246 tools are versioned. A plain key lookup would silently report them missing — a quiet correctness bug, not a crash. `resolveToolId` reproduces that against the id set and is differentially tested against the original.
4545

46+
## Testing code that reads tool metadata
47+
48+
Mock the module the code under test actually reads. `vi.mock('@/tools/utils', () => toolsUtilsMock)` only controls `getTool`; code that reads `params`/`outputs`/`name` goes through `@/tools/metadata`, so mocking `tools/utils` there is a **no-op that still passes** — because the real generated artifacts happen to agree with the mock fixtures. The test looks green while controlling nothing.
49+
50+
```ts
51+
import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks'
52+
53+
vi.mock('@/tools/utils', () => toolsUtilsMock) // executable lookup
54+
vi.mock('@/tools/metadata', () => toolsMetadataMock) // params / outputs / name
55+
```
56+
57+
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+
4659
## How to verify an edge actually got cut
4760

4861
Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph:

apps/sim/ee/access-control/components/group-detail.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ import {
7676
import type { ProviderId } from '@/providers/types'
7777
import { getAllProviderIds, getProviderFromModel } from '@/providers/utils'
7878
import type { ProviderName } from '@/stores/providers'
79-
import { getTool } from '@/tools/utils'
79+
import { getToolMetadata } from '@/tools/metadata'
8080

8181
const logger = createLogger('AccessControlGroupDetail')
8282

@@ -733,7 +733,7 @@ function BlockToolRow({
733733
const checkboxId = `block-${block.type}`
734734

735735
const toolItems = useMemo<DenylistGridItem[]>(
736-
() => (block.tools?.access ?? []).map((id) => ({ id, label: getTool(id)?.name ?? id })),
736+
() => (block.tools?.access ?? []).map((id) => ({ id, label: getToolMetadata(id)?.name ?? id })),
737737
[block.tools?.access]
738738
)
739739
const isExpandable = toolItems.length > 1

apps/sim/serializer/custom-block-lifecycle.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* - a deleted *input* on a live custom block no longer leaks its stale value into
88
* the child `inputMapping` (Bug 1).
99
*/
10-
import { toolsUtilsMock } from '@sim/testing/mocks'
10+
import { toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks'
1111
import { beforeEach, describe, expect, it, vi } from 'vitest'
1212

1313
// Build the custom-block configs INSIDE the factory (hoisted) so no top-level
@@ -33,6 +33,7 @@ vi.mock('@/blocks', async () => {
3333
return { getBlock, getAllBlocks: () => Object.values(mockBlockConfigs) }
3434
})
3535
vi.mock('@/tools/utils', () => toolsUtilsMock)
36+
vi.mock('@/tools/metadata', () => toolsMetadataMock)
3637

3738
import { extractBlockParams, Serializer } from '@/serializer/index'
3839

apps/sim/serializer/field-analysis.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,13 @@
55
* (collectBlockFieldIssues / extractBlockParams) — the single source of truth
66
* shared by the serializer's required-field validation and the copilot lint.
77
*/
8-
import { blocksMock, toolsUtilsMock } from '@sim/testing/mocks'
8+
import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks'
99
import { describe, expect, it, vi } from 'vitest'
1010

1111
const { svcConfig } = vi.hoisted(() => ({ svcConfig: { value: null as any } }))
1212

1313
vi.mock('@/tools/utils', () => toolsUtilsMock)
14+
vi.mock('@/tools/metadata', () => toolsMetadataMock)
1415
vi.mock('@/blocks', () => ({
1516
...blocksMock,
1617
getBlock: (type: string) => (type === 'svc' ? svcConfig.value : blocksMock.getBlock(type)),

apps/sim/serializer/index.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,14 @@ import {
1818
createMinimalWorkflowState,
1919
createMissingMetadataWorkflow,
2020
} from '@sim/testing/factories'
21-
import { blocksMock, toolsUtilsMock } from '@sim/testing/mocks'
21+
import { blocksMock, toolsMetadataMock, toolsUtilsMock } from '@sim/testing/mocks'
2222
import { describe, expect, it, vi } from 'vitest'
2323
import { Serializer } from '@/serializer/index'
2424
import type { SerializedWorkflow } from '@/serializer/types'
2525

2626
vi.mock('@/blocks', () => blocksMock)
2727
vi.mock('@/tools/utils', () => toolsUtilsMock)
28+
vi.mock('@/tools/metadata', () => toolsMetadataMock)
2829

2930
describe('Serializer', () => {
3031
describe('serializeWorkflow', () => {

packages/testing/src/mocks/blocks.mock.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,25 @@ export const blocksMock = {
338338

339339
/**
340340
* Pre-configured tools/utils mock for use with vi.mock('@/tools/utils', () => toolsUtilsMock).
341+
*
342+
* Only mocks the *executable* lookup. Code that reads a tool's shape now goes
343+
* through `@/tools/metadata`, so mocking this alone leaves such code reading the
344+
* real generated artifacts — see {@link toolsMetadataMock}.
341345
*/
342346
export const toolsUtilsMock = {
343347
getTool: createMockGetTool(),
344348
}
349+
350+
/**
351+
* Pre-configured metadata mock for use with
352+
* `vi.mock('@/tools/metadata', () => toolsMetadataMock)`.
353+
*
354+
* Backed by the same `mockToolConfigs` as {@link toolsUtilsMock}, so a test that
355+
* mocks both sees one consistent tool universe. Mock this wherever the code under
356+
* test reads `params`, `outputs` or `name` — mocking `@/tools/utils` there is a
357+
* no-op that only appears to work because the real artifacts happen to agree.
358+
*/
359+
export const toolsMetadataMock = {
360+
getToolMetadata: createMockGetTool(),
361+
getToolParams: (toolId: string) => createMockGetTool()(toolId)?.params,
362+
}

packages/testing/src/mocks/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ export {
3333
createMockGetTool,
3434
mockBlockConfigs,
3535
mockToolConfigs,
36+
toolsMetadataMock,
3637
toolsUtilsMock,
3738
} from './blocks.mock'
3839
// Copilot HTTP mocks (for @/lib/copilot/request/http)

0 commit comments

Comments
 (0)