Skip to content

Commit 4ab45cf

Browse files
fix(custom-blocks): address review findings on agent-tool execution
1 parent 8a3644c commit 4ab45cf

7 files changed

Lines changed: 91 additions & 9 deletions

File tree

apps/sim/executor/handlers/pi/sim-tools.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type { PiToolResult, PiToolSpec } from '@/executor/handlers/pi/backend'
1616
import type { ExecutionContext } from '@/executor/types'
1717
import { transformBlockTool } from '@/providers/utils'
1818
import { executeTool } from '@/tools'
19+
import { mergeToolParameters } from '@/tools/params'
1920
import type { ToolResponse } from '@/tools/types'
2021
import { getTool } from '@/tools/utils'
2122
import { getToolAsync } from '@/tools/utils.server'
@@ -74,8 +75,11 @@ export async function buildSimToolSpecs(
7475
const result = await executeTool(
7576
toolId,
7677
{
77-
...preseededParams,
78-
...args,
78+
// Same merge the Agent block's tool calls use: user-preseeded values
79+
// win over LLM args, and `inputMapping` is deep-merged rather than
80+
// replaced — a partial mapping from the model must not drop the
81+
// user-filled fields baked onto the block.
82+
...mergeToolParameters(preseededParams, args as Record<string, unknown>),
7983
// Trusted execution context, spread last so an LLM-supplied
8084
// `_context` arg can't override it. executeTool reads this directly
8185
// for OAuth-credential resolution and internal-route identity, the

apps/sim/executor/handlers/workflow/custom-block-tool-runner.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,11 @@ vi.mock('@/executor/handlers/workflow/workflow-handler', () => ({
99
WorkflowBlockHandler: class {
1010
execute = mockExecute
1111
},
12+
aggregateChildCost: (spans: Array<{ cost?: { total?: number } }>) =>
13+
spans.reduce((sum, span) => sum + (span?.cost?.total ?? 0), 0),
1214
}))
1315

16+
import { ChildWorkflowError } from '@/executor/errors/child-workflow-error'
1417
import {
1518
buildCustomBlockExecutionContext,
1619
runCustomBlockTool,
@@ -85,6 +88,20 @@ describe('runCustomBlockTool', () => {
8588
expect(res.error).toContain('not deployed')
8689
})
8790

91+
it('rolls up already-incurred child cost when the run fails', async () => {
92+
const err: any = new Error('child blew up')
93+
err.name = 'ChildWorkflowError'
94+
err.childTraceSpans = [{ id: 's1', name: 'child', type: 'agent', cost: { total: 0.25 } }]
95+
Object.setPrototypeOf(err, ChildWorkflowError.prototype)
96+
mockExecute.mockRejectedValue(err)
97+
98+
const res = await runCustomBlockTool({ blockType: 'custom_block_abc', _context: {} })
99+
100+
expect(res.success).toBe(false)
101+
// Partial spend must not be recorded as zero-cost.
102+
expect((res.output as any).cost.total).toBeGreaterThan(0)
103+
})
104+
88105
it('rejects a missing block type without invoking the handler', async () => {
89106
const res = await runCustomBlockTool({ _context: {} })
90107
expect(res.success).toBe(false)

apps/sim/executor/handlers/workflow/custom-block-tool-runner.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@ import { createLogger } from '@sim/logger'
22
import { getErrorMessage } from '@sim/utils/errors'
33
import { generateId } from '@sim/utils/id'
44
import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution'
5-
import { WorkflowBlockHandler } from '@/executor/handlers/workflow/workflow-handler'
5+
import { ChildWorkflowError } from '@/executor/errors/child-workflow-error'
6+
import {
7+
aggregateChildCost,
8+
WorkflowBlockHandler,
9+
} from '@/executor/handlers/workflow/workflow-handler'
610
import type { ExecutionContext } from '@/executor/types'
711
import type { SerializedBlock } from '@/serializer/types'
812
import type { ToolResponse } from '@/tools/types'
@@ -109,11 +113,20 @@ export async function runCustomBlockTool(params: CustomBlockToolParams): Promise
109113
output && typeof output === 'object' && !Array.isArray(output) ? output : { result: output }
110114
return { success: true, output: normalized }
111115
} catch (error) {
112-
// The handler throws a consumer-safe `ChildWorkflowError` on failure. Partial
113-
// child cost rides its trace spans, but the provider tool loop only bills cost
114-
// from successful results, so the error message alone is surfaced here.
116+
// The handler throws a consumer-safe `ChildWorkflowError` on failure. Its trace
117+
// spans are the only carrier of spend the child already incurred before failing,
118+
// so roll that up onto the failed result too — otherwise a partially-run child is
119+
// recorded as zero-cost.
115120
const message = getErrorMessage(error, 'Custom block execution failed')
121+
const failedChildSpans = ChildWorkflowError.isChildWorkflowError(error)
122+
? error.childTraceSpans
123+
: []
124+
const childCost = aggregateChildCost(failedChildSpans)
116125
logger.info('Custom block tool execution failed', { blockType: params.blockType, message })
117-
return { success: false, output: {}, error: message }
126+
return {
127+
success: false,
128+
output: childCost > 0 ? { cost: { total: childCost } } : {},
129+
error: message,
130+
}
118131
}
119132
}

apps/sim/executor/handlers/workflow/workflow-handler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ export function remapCustomBlockInputKeys(
136136
* breakdowns), minus the base execution charge the parent applies once itself.
137137
* A naive top-level `cost.total` sum undercounts when spend sits on nested children.
138138
*/
139-
function aggregateChildCost(childTraceSpans: TraceSpan[]): number {
139+
export function aggregateChildCost(childTraceSpans: TraceSpan[]): number {
140140
if (childTraceSpans.length === 0) return 0
141141
const summary = calculateCostSummary(childTraceSpans)
142142
return Math.max(0, summary.totalCost - summary.baseExecutionCharge)

apps/sim/providers/custom-block-tool.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,34 @@ describe('transformBlockTool — custom blocks', () => {
5959
expect(mockResolve).toHaveBeenCalledWith('custom_block_test')
6060
})
6161

62+
it('does not offer the tool when a required file input has no preset value', async () => {
63+
mockResolve.mockResolvedValue({
64+
workflowId: 'wf-src',
65+
inputFields: [{ id: 'f', name: 'Files', type: 'file[]' }],
66+
requiredInputIds: ['f'],
67+
})
68+
69+
const tool = await transformBlockTool({ type: 'custom_block_test', params: {} }, options)
70+
expect(tool).toBeNull()
71+
})
72+
73+
it('still offers the tool when a required file input is pre-filled on the block', async () => {
74+
mockResolve.mockResolvedValue({
75+
workflowId: 'wf-src',
76+
inputFields: [{ id: 'f', name: 'Files', type: 'file[]' }],
77+
requiredInputIds: ['f'],
78+
})
79+
80+
const tool = await transformBlockTool(
81+
{ type: 'custom_block_test', params: { f: [{ id: 'file-1' }] } },
82+
options
83+
)
84+
expect(tool).not.toBeNull()
85+
// The preset value rides the baked mapping; the schema still omits the file field.
86+
expect(tool!.params.inputMapping).toContain('file-1')
87+
expect(Object.keys(tool!.parameters.properties.inputMapping.properties)).toEqual([])
88+
})
89+
6290
it('returns null (tool not offered) when the binding cannot be resolved', async () => {
6391
mockResolve.mockResolvedValue(null)
6492
const tool = await transformBlockTool({ type: 'custom_block_test', params: {} }, options)

apps/sim/providers/utils.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,23 @@ export async function transformBlockTool(
634634
logger.warn('custom_block_executor tool not registered')
635635
return null
636636
}
637+
const inputMapping = assembleCustomBlockInputMapping(block.params || {})
638+
// A `file[]` field is omitted from the model schema (the model can't synthesize
639+
// upload descriptors). If such a field is REQUIRED and the user hasn't
640+
// pre-filled it on the block, no invocation could ever satisfy the child's
641+
// required-input check — so don't offer an unusable tool at all.
642+
const prefilled = JSON.parse(inputMapping) as Record<string, unknown>
643+
const requiredIds = new Set(binding.requiredInputIds)
644+
const unfillableFileField = binding.inputFields.find((field) => {
645+
const key = field.id ?? field.name
646+
return isFileFieldType(field.type) && requiredIds.has(key) && !(key in prefilled)
647+
})
648+
if (unfillableFileField) {
649+
logger.warn(
650+
`Custom block ${block.type} not offered as a tool: required file input "${unfillableFileField.name}" has no preset value and cannot be supplied by the model`
651+
)
652+
return null
653+
}
637654
return {
638655
// Unique per block so two custom-block tools never collide on the wire.
639656
id: `custom_block_executor_${block.type}`,
@@ -643,7 +660,7 @@ export async function transformBlockTool(
643660
description: blockDef.description || customToolConfig.description,
644661
params: {
645662
blockType: block.type,
646-
inputMapping: assembleCustomBlockInputMapping(block.params || {}),
663+
inputMapping,
647664
},
648665
parameters: buildCustomBlockInputMappingSchema(
649666
blockDef.name,

apps/sim/tools/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1251,6 +1251,9 @@ export async function executeTool(
12511251
const endTime = new Date()
12521252
return {
12531253
...result,
1254+
// Strip internal `__`-prefixed fields the same way every other tool path does,
1255+
// so child-workflow internals never reach the agent's tool result.
1256+
output: postProcessToolOutput(normalizedToolId, result.output ?? {}),
12541257
timing: {
12551258
startTime: startTimeISO,
12561259
endTime: endTime.toISOString(),

0 commit comments

Comments
 (0)