diff --git a/.agents/skills/add-tools/SKILL.md b/.agents/skills/add-tools/SKILL.md index d54af50dd0f..ec57396720b 100644 --- a/.agents/skills/add-tools/SKILL.md +++ b/.agents/skills/add-tools/SKILL.md @@ -190,6 +190,8 @@ fallback, or caller-controlled `_context` authority. A required `'hidden'` param needs an `oauth` declaration or `hosting.apiKeyParam` to supply it (`bun run check:tool-param-reachability`). +A declared `timeout` param is an ordinary tool input — put it in the request body or URL yourself if the provider expects it; it becomes Sim's millisecond request deadline only when the tool sets `timeoutParamIsDeadline: true` (`http_request`). A `method` param on a tool with a fixed `request.method` would be sent as the HTTP verb, so the same audit rejects it. + ### Parameter Types - `'string'` - Text values - `'number'` - Numeric values diff --git a/apps/sim/blocks/blocks/redis.ts b/apps/sim/blocks/blocks/redis.ts index 662822e0a96..736c29b3310 100644 --- a/apps/sim/blocks/blocks/redis.ts +++ b/apps/sim/blocks/blocks/redis.ts @@ -255,23 +255,25 @@ export const RedisBlock: BlockConfig = { 'redis_setnx', ], config: { - tool: (params) => { + tool: (params) => `redis_${params.operation}`, + params: (params) => { + const coerced: Record = {} if (params.ex) { - params.ex = Number(params.ex) + coerced.ex = Number(params.ex) } if (params.seconds !== undefined) { - params.seconds = Number(params.seconds) + coerced.seconds = Number(params.seconds) } if (params.start !== undefined) { - params.start = Number(params.start) + coerced.start = Number(params.start) } if (params.stop !== undefined) { - params.stop = Number(params.stop) + coerced.stop = Number(params.stop) } if (params.increment !== undefined) { - params.increment = Number(params.increment) + coerced.increment = Number(params.increment) } - return `redis_${params.operation}` + return coerced }, }, }, diff --git a/apps/sim/blocks/blocks/upstash.ts b/apps/sim/blocks/blocks/upstash.ts index 94280ebe787..9200af4e568 100644 --- a/apps/sim/blocks/blocks/upstash.ts +++ b/apps/sim/blocks/blocks/upstash.ts @@ -255,21 +255,6 @@ export const UpstashBlock: BlockConfig = { ], config: { tool: (params) => { - if (params.ex) { - params.ex = Number(params.ex) - } - if (params.seconds !== undefined) { - params.seconds = Number(params.seconds) - } - if (params.start !== undefined) { - params.start = Number(params.start) - } - if (params.stop !== undefined) { - params.stop = Number(params.stop) - } - if (params.increment !== undefined) { - params.increment = Number(params.increment) - } switch (params.operation) { case 'get': return 'upstash_redis_get' @@ -307,6 +292,25 @@ export const UpstashBlock: BlockConfig = { throw new Error(`Unknown operation: ${params.operation}`) } }, + params: (params) => { + const coerced: Record = {} + if (params.ex) { + coerced.ex = Number(params.ex) + } + if (params.seconds !== undefined) { + coerced.seconds = Number(params.seconds) + } + if (params.start !== undefined) { + coerced.start = Number(params.start) + } + if (params.stop !== undefined) { + coerced.stop = Number(params.stop) + } + if (params.increment !== undefined) { + coerced.increment = Number(params.increment) + } + return coerced + }, }, }, inputs: { diff --git a/apps/sim/scripts/check-block-registry.ts b/apps/sim/scripts/check-block-registry.ts index 8c453be49ca..5e5bd885dcf 100644 --- a/apps/sim/scripts/check-block-registry.ts +++ b/apps/sim/scripts/check-block-registry.ts @@ -3,7 +3,7 @@ /** * CI check: enforces block-registry invariants that protect the runtime. * - * Two checks run in sequence: + * Checks run in sequence: * * 1. **Subblock ID stability** — diffs the current registry against a base ref * and fails if any subblock ID was removed without a corresponding entry in @@ -17,6 +17,18 @@ * resolve values via direct lookup; mismatches false-flag fields as missing * at submit time. * + * 3. **Selector coercions** — a block's inline `tools.config.tool` may not + * assign a coerced value (`Number(...)`, `parseInt(...)`, `JSON.parse(...)`, + * `!!x`, unary `+`, a template literal, a comparison, …) back onto its params, + * nor write to a param with a compound assignment (`+=`, `??=`, …) or `++`/`--`. + * `selectToolId` runs the selector during serialization on the object that + * becomes the serialized block's `params`, before `` references + * resolve, so `params.x = Number(params.x)` turns a reference into `NaN`. + * Coercions belong in `tools.config.params`, which the executor runs on + * resolved inputs. + * + * Integration BlockMeta coverage and sunset `replacedBy` targets are checked too. + * * Usage: * bun run apps/sim/scripts/check-block-registry.ts [base-ref] * @@ -25,6 +37,9 @@ */ import { execFileSync } from 'node:child_process' +import { readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import ts from '@typescript/typescript6' import { SUBBLOCK_ID_MIGRATIONS } from '@/lib/workflows/migrations/subblock-migrations' import { getAllBlocks, getBlock, getBlockMeta, getBlockRegistry } from '@/blocks/registry' import { readBlockRegistryAtRef } from '@/scripts/block-registry-snapshot' @@ -231,6 +246,132 @@ function checkSunsetReplacedBy(): CheckResult { return { kind: 'fail', errors } } +const COERCION_CALLEES = new Set([ + 'BigInt', + 'Boolean', + 'JSON.parse', + 'Number', + 'Number.parseFloat', + 'Number.parseInt', + 'String', + 'parseFloat', + 'parseInt', +]) + +const COERCING_UNARY = new Set([ + ts.SyntaxKind.PlusToken, + ts.SyntaxKind.MinusToken, + ts.SyntaxKind.ExclamationToken, + ts.SyntaxKind.TildeToken, +]) + +const COMPARISON_OPERATORS = new Set([ + ts.SyntaxKind.EqualsEqualsToken, + ts.SyntaxKind.EqualsEqualsEqualsToken, + ts.SyntaxKind.ExclamationEqualsToken, + ts.SyntaxKind.ExclamationEqualsEqualsToken, + ts.SyntaxKind.LessThanToken, + ts.SyntaxKind.LessThanEqualsToken, + ts.SyntaxKind.GreaterThanToken, + ts.SyntaxKind.GreaterThanEqualsToken, +]) + +/** + * Whether `expression` derives a new value anywhere in it — a coercing call, a unary `+ - ! ~`, + * a template literal with substitutions, or a comparison — including under `??`, `||`, or a + * conditional. Any of these turns an unresolved `` reference into a fixed value. + */ +function isCoercion(expression: ts.Node): boolean { + if (ts.isPrefixUnaryExpression(expression) && COERCING_UNARY.has(expression.operator)) return true + if (ts.isCallExpression(expression) && COERCION_CALLEES.has(expression.expression.getText())) { + return true + } + if (ts.isTemplateExpression(expression)) return true + if ( + ts.isBinaryExpression(expression) && + COMPARISON_OPERATORS.has(expression.operatorToken.kind) + ) { + return true + } + return ts.forEachChild(expression, (child) => (isCoercion(child) ? true : undefined)) ?? false +} + +function findSelectorCoercions(file: string, source: ts.SourceFile): string[] { + const errors: string[] = [] + const visit = (node: ts.Node) => { + const selector = + ts.isPropertyAssignment(node) || ts.isMethodDeclaration(node) ? node : undefined + const config = selector?.parent.parent + if ( + selector && + ts.isIdentifier(selector.name) && + selector.name.text === 'tool' && + config && + ts.isPropertyAssignment(config) && + ts.isIdentifier(config.name) && + config.name.text === 'config' + ) { + const fn = ts.isMethodDeclaration(selector) ? selector : selector.initializer + const paramsName = + (ts.isArrowFunction(fn) || ts.isFunctionExpression(fn) || ts.isMethodDeclaration(fn)) && + fn.parameters[0] && + ts.isIdentifier(fn.parameters[0].name) + ? fn.parameters[0].name.text + : undefined + const isParamsMember = (target: ts.Expression) => + (ts.isPropertyAccessExpression(target) || ts.isElementAccessExpression(target)) && + ts.isIdentifier(target.expression) && + target.expression.text === paramsName + const findAssignments = (inner: ts.Node) => { + const operator = ts.isBinaryExpression(inner) ? inner.operatorToken.kind : undefined + const derivedWrite = + (ts.isBinaryExpression(inner) && + isParamsMember(inner.left) && + ((operator === ts.SyntaxKind.EqualsToken && isCoercion(inner.right)) || + (operator !== undefined && + operator >= ts.SyntaxKind.FirstCompoundAssignment && + operator <= ts.SyntaxKind.LastCompoundAssignment))) || + ((ts.isPrefixUnaryExpression(inner) || ts.isPostfixUnaryExpression(inner)) && + (inner.operator === ts.SyntaxKind.PlusPlusToken || + inner.operator === ts.SyntaxKind.MinusMinusToken) && + isParamsMember(inner.operand)) + if (derivedWrite) { + const line = source.getLineAndCharacterOfPosition(inner.getStart()).line + 1 + errors.push( + `${file}:${line}: \`${inner.getText()}\` coerces a param inside tools.config.tool.\n` + + ' → Return the coerced value from tools.config.params instead.' + ) + } + ts.forEachChild(inner, findAssignments) + } + if (paramsName) findAssignments(fn) + } + ts.forEachChild(node, visit) + } + visit(source) + return errors +} + +function checkSelectorCoercions(): CheckResult { + const blocksDir = join(gitRoot, 'apps/sim/blocks/blocks') + const errors: string[] = [] + for (const name of readdirSync(blocksDir)) { + if (!name.endsWith('.ts') || name.endsWith('.test.ts')) continue + const path = join(blocksDir, name) + const source = ts.createSourceFile( + path, + readFileSync(path, 'utf-8'), + ts.ScriptTarget.Latest, + true + ) + errors.push(...findSelectorCoercions(`apps/sim/blocks/blocks/${name}`, source)) + } + if (errors.length === 0) { + return { kind: 'pass', message: 'Selector coercion check passed' } + } + return { kind: 'fail', errors } +} + function reportResult(label: string, failureHeader: string, result: CheckResult): boolean { if (result.kind === 'pass') { console.log(`✓ ${result.message}`) @@ -252,6 +393,7 @@ const stabilityResult = checkSubblockIdStability() const canonicalResult = checkCanonicalIdContract() const metaCoverageResult = checkIntegrationMetaCoverage() const sunsetResult = checkSunsetReplacedBy() +const selectorCoercionResult = checkSelectorCoercions() const stabilityOk = reportResult( 'Subblock ID stability check', @@ -277,4 +419,10 @@ const sunsetOk = reportResult( sunsetResult ) -process.exit(stabilityOk && canonicalOk && metaCoverageOk && sunsetOk ? 0 : 1) +const selectorCoercionOk = reportResult( + 'Selector coercion check', + 'tools.config.tool runs at serialization, before references resolve, so a coerced reference becomes NaN.', + selectorCoercionResult +) + +process.exit(stabilityOk && canonicalOk && metaCoverageOk && sunsetOk && selectorCoercionOk ? 0 : 1) diff --git a/apps/sim/tools/firecrawl/map.ts b/apps/sim/tools/firecrawl/map.ts index 1fd8a6a0376..efb47a21e81 100644 --- a/apps/sim/tools/firecrawl/map.ts +++ b/apps/sim/tools/firecrawl/map.ts @@ -69,6 +69,8 @@ export const mapTool: ToolConfig = { hosting: firecrawlHosting(), + timeoutParamIsDeadline: true, + request: { method: 'POST', url: 'https://api.firecrawl.dev/v2/map', diff --git a/apps/sim/tools/firecrawl/parse.ts b/apps/sim/tools/firecrawl/parse.ts index c896dba2a78..d450a0ff5b0 100644 --- a/apps/sim/tools/firecrawl/parse.ts +++ b/apps/sim/tools/firecrawl/parse.ts @@ -94,6 +94,8 @@ export const parseTool: InternalToolConfig = { hosting: firecrawlHosting(), + timeoutParamIsDeadline: true, + operation: { modelInput: { mode: 'project', diff --git a/apps/sim/tools/http/request.ts b/apps/sim/tools/http/request.ts index a1a49055b8f..91b9735e9dd 100644 --- a/apps/sim/tools/http/request.ts +++ b/apps/sim/tools/http/request.ts @@ -96,6 +96,8 @@ export const requestTool: ToolConfig = { }, }, + timeoutParamIsDeadline: true, + request: { allowSameOrigin: true, url: (params: RequestParams) => { diff --git a/apps/sim/tools/index.ts b/apps/sim/tools/index.ts index bbe9a19177e..51976ca76bc 100644 --- a/apps/sim/tools/index.ts +++ b/apps/sim/tools/index.ts @@ -105,6 +105,7 @@ import { getOwnEnumerableDataEntries, prepareToolRequest, projectToolModelInputParams, + readRequestedDeadline, } from '@/tools/request-transport' import type { BYOKProviderId, @@ -2735,7 +2736,7 @@ async function executeDeclaredInternalOperation({ } else { const handler = await getInternalToolOperationHandler(toolId) if (!handler) throw new Error(`No internal operation registered for ${toolId}`) - const requestedTimeout = Number(params.timeout) + const requestedTimeout = Number(readRequestedDeadline(tool, params)) const operationTimeout = Number.isFinite(requestedTimeout) && requestedTimeout > 0 ? Math.min(requestedTimeout, getMaxExecutionTimeout()) diff --git a/apps/sim/tools/request-transport.ts b/apps/sim/tools/request-transport.ts index 0dfb6543654..4fb6127d68a 100644 --- a/apps/sim/tools/request-transport.ts +++ b/apps/sim/tools/request-transport.ts @@ -155,6 +155,18 @@ function collectProvenanceSensitiveHeaders( return [...sensitiveHeaders] } +/** + * Reads the caller's millisecond deadline from `params.timeout`, unless the tool declares a + * `timeout` param of its own without {@link ToolConfig.timeoutParamIsDeadline}. + */ +export function readRequestedDeadline( + tool: ExecutableToolConfig, + params: Record +): unknown { + if (tool.params?.timeout && !tool.timeoutParamIsDeadline) return undefined + return params.timeout +} + function formatToolRequest( tool: ToolConfig, params: Record, @@ -189,7 +201,7 @@ function formatToolRequest( } } - const rawTimeout = params.timeout + const rawTimeout = readRequestedDeadline(tool, params) const timeout = rawTimeout != null ? Number(rawTimeout) : undefined const validTimeout = timeout != null && Number.isFinite(timeout) && timeout > 0 diff --git a/apps/sim/tools/types.ts b/apps/sim/tools/types.ts index 9edd5843a5b..75da1ace616 100644 --- a/apps/sim/tools/types.ts +++ b/apps/sim/tools/types.ts @@ -321,6 +321,16 @@ export interface ToolConfig

{ * Usage is billed according to the pricing config. */ hosting?: ToolHostingConfig

+ + /** + * Makes this tool's declared `timeout` param the execution deadline, in milliseconds. + * + * A caller may pass `params.timeout` to bound any tool that does not declare one. A tool that + * declares its own `timeout` param owns that value instead — usually a provider field in + * seconds or a duration string, which as a millisecond deadline would abort the request almost + * immediately — so the executor treats it as the deadline only when this is set. + */ + timeoutParamIsDeadline?: true } export interface TableRow { diff --git a/scripts/check-tool-param-reachability.ts b/scripts/check-tool-param-reachability.ts index 15cf02d4110..11b8dbb73b2 100644 --- a/scripts/check-tool-param-reachability.ts +++ b/scripts/check-tool-param-reachability.ts @@ -47,11 +47,15 @@ * reachable, so a parameter needing an exemption is one no caller can supply — * exactly what this audit exists to reject. * + * Two registry rules ride along: a hosted tool may not report its own `cost` + * output, and a fixed-verb external tool may not declare a `method` param that + * the transport would send as the HTTP verb. + * * Usage: * bun run scripts/check-tool-param-reachability.ts */ import { tools } from '../apps/sim/tools/registry' -import type { ToolConfig } from '../apps/sim/tools/types' +import { isInternalToolConfig, type ToolConfig } from '../apps/sim/tools/types' /** * The one parameter credential resolution assigns unconditionally. @@ -119,6 +123,26 @@ function findHostedToolsReportingCost(): string[] { .sort() } +/** + * An external tool with a fixed HTTP verb must not declare a `method` param. + * + * `formatToolRequest` (`tools/request-transport.ts`) sends `params.method || request.method` + * whenever `request.method` is not a function, so a provider field named `method` (an S3 + * presign operation, a payment method) silently replaces the verb. A tool whose verb really + * comes from that param reads it in a `request.method` function, which this rule allows. + */ +function findVerbOverridingParams(): string[] { + return Object.entries(tools) + .filter( + ([, config]) => + !isInternalToolConfig(config) && + typeof config.request.method !== 'function' && + Object.hasOwn(config.params ?? {}, 'method') + ) + .map(([toolId]) => toolId) + .sort() +} + function findUnreachableParams(): Finding[] { const findings: Finding[] = [] @@ -177,6 +201,17 @@ function main(): void { process.exit(1) } + const verbOverriders = findVerbOverridingParams() + if (verbOverriders.length > 0) { + console.error('Tool parameter reachability audit failed:\n') + for (const toolId of verbOverriders) { + console.error( + ` ${toolId} — declares a 'method' param on a fixed-verb request; the transport sends params.method as the HTTP verb. Rename the param, or read it in a request.method function` + ) + } + process.exit(1) + } + if (findings.length === 0) { console.log( `✓ tool parameter reachability: all ${toolCount} tools supply every required hidden parameter through oauth, hosting, or a published shape`