fix(tools): address the cubic review round on direct tool execution

Two were real defects rather than polish:

The usage ledger de-duplicates on `eventKey`, and the derived key hashes actor,
workspace, source and description — identical for every call to the same tool.
`onConflictDoNothing` therefore billed the first hosted-key call and silently
dropped every one after it. A workflow run has an `executionId` to separate its
rows; a direct call has nothing, so it now mints one.

Undeclared input keys reached the executor. `impersonateUserEmail` is read
straight out of params and forwarded to credential-token resolution as an
impersonation request, and no tool declares it. The two ad-hoc denylists are
replaced by a declared-parameter allowlist — the actual boundary, and the one
`GET /api/v2/tools/{toolId}` already publishes.

Also: renaming a canonical id orphans `data.canonicalModes`, and the backfill
recovers it only when one side holds a value. `setBlockCanonicalMode` writes the
mode without clearing its sibling, so a workflow holding both would have
silently switched from the typed reference to the uploaded file.
`migrateCanonicalModeIds` carries the key across, ahead of the backfill.

Smaller: the reachability audit now mirrors execution exactly, exempting
`credentialType` only under `authoritativeParams` and refusing to treat
conditional hosting as a guarantee; the boundary guard splits on both path
separators so it still excludes the execute route on Windows; the route stops
documenting the 409 and 423 it cannot produce; and `sim tools execute` prints
the output it promised in the human formats.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: icecrasher321

apps/docs/openapi-v2-resources.json

@@ -4104,18 +4104,12 @@
           "404": {
             "$ref": "#/components/responses/NotFound"
           },
-          "409": {
-            "$ref": "#/components/responses/Conflict"
-          },
           "413": {
             "$ref": "#/components/responses/PayloadTooLarge"
           },
           "415": {
             "$ref": "#/components/responses/UnsupportedMediaType"
           },
-          "423": {
-            "$ref": "#/components/responses/Locked"
-          },
           "429": {
             "$ref": "#/components/responses/RateLimited"
           },
@@ -4383,22 +4377,6 @@
           }
         }
       },
-      "Locked": {
-        "description": "The resource is locked and cannot be modified.",
-        "content": {
-          "application/json": {
-            "schema": {
-              "$ref": "#/components/schemas/V2Error"
-            },
-            "example": {
-              "error": {
-                "code": "LOCKED",
-                "message": "Workflow is locked"
-              }
-            }
-          }
-        }
-      },
       "RateLimited": {
         "description": "The caller exceeded the request rate limit.",
         "headers": {

apps/sim/lib/api/contracts/v2/openapi/resources.ts

@@ -38,7 +38,6 @@ import {
   RATE_LIMIT_HEADERS,
   RESOURCE_CONFLICT_ERRORS,
   RESOURCE_ERRORS,
-  RESOURCE_MUTATION_ERRORS,
   V2_API_KEY_SECURITY,
   V2_API_KEY_SECURITY_SCHEMES,
   V2_COMMON_HEADERS,
@@ -1828,7 +1827,7 @@ const declaredRoutes = [
       operationId: 'executeTool',
       summary: 'Run Tool',
       description: `Run one built-in tool and return what it produced. Supply \`input\` using the parameter ids \`GET /api/v2/tools/{toolId}\` publishes; Sim resolves the credential named by \`credentialId\`, injects a hosted API key for the tools it supplies one for, and substitutes environment-variable references, so the request carries arguments rather than secrets. A parameter the tool marks \`user-only\` also accepts \`{{VAR_NAME}}\` as its whole value, resolved server-side against the workspace environment; every other value is sent verbatim, so a literal secret passes through untouched. A tool that runs and refuses is a \`200\` carrying \`status: "failed"\` and the reason — the error envelope is reserved for failures of this API, not of the third party. A tool the workspace's visible blocks do not expose answers \`404\` identically to one that does not exist; one whose integration the workspace does not permit answers \`403\` with \`error.details.code\` \`INTEGRATION_NOT_ALLOWED\`. Hosted-key spend this call incurs is billed to the workspace. ${WORKSPACE_API_KEY_DENIED}`,
-      errors: RESOURCE_MUTATION_ERRORS,
+      errors: RESOURCE_ERRORS,
       success: { description: 'The outcome of the tool call.' },
     }),
     {

apps/sim/lib/tool-execution/application/execute-tool.test.ts

@@ -283,7 +283,7 @@ describe('executeToolForCaller', () => {
   })
 
   it('resolves an unversioned name to the newest visible version', async () => {
-    await expect(run({ toolId: 'confluence_read' })).resolves.toMatchObject({
+    await expect(run({ toolId: 'confluence_read', input: {} })).resolves.toMatchObject({
       toolId: 'confluence_read_v2',
     })
   })
@@ -396,26 +396,76 @@ describe('executeToolForCaller', () => {
   })
 
   it('refuses a reserved argument rather than dropping it', async () => {
-    await expect(run({ input: { _context: { userId: 'someone-else' } } })).rejects.toMatchObject({
+    await expect(
+      run({ input: { url: 'https://a.co', _context: { userId: 'someone-else' } } })
+    ).rejects.toMatchObject({
       code: 'validation',
       message: expect.stringContaining('_context'),
     })
     expect(mocks.executeRegistryTool).not.toHaveBeenCalled()
   })
 
   it('refuses a hosted-key flag smuggled in as an argument', async () => {
-    await expect(run({ input: { __usingHostedKey: true } })).rejects.toMatchObject({
+    await expect(
+      run({ input: { url: 'https://a.co', __usingHostedKey: true } })
+    ).rejects.toMatchObject({
+      code: 'validation',
+    })
+  })
+
+  /**
+   * The executor reads this straight out of params and forwards it to
+   * credential-token resolution as an impersonation request. No tool declares
+   * it, which is why the check is a declared-parameter allowlist rather than a
+   * list of names someone remembered.
+   */
+  it('refuses an undeclared impersonation field', async () => {
+    await expect(
+      run({ input: { url: 'https://a.co', impersonateUserEmail: 'someone@example.com' } })
+    ).rejects.toMatchObject({
       code: 'validation',
+      message: expect.stringContaining('impersonateUserEmail'),
+    })
+    expect(mocks.executeRegistryTool).not.toHaveBeenCalled()
+  })
+
+  it('refuses any other undeclared input, naming it', async () => {
+    await expect(run({ input: { url: 'https://a.co', nope: 1 } })).rejects.toMatchObject({
+      code: 'validation',
+      message: expect.stringContaining('input.nope'),
     })
   })
 
   it('refuses a credential named inline instead of at the top level', async () => {
-    await expect(run({ input: { credential: 'cred-1' } })).rejects.toMatchObject({
+    await expect(
+      run({ input: { url: 'https://a.co', credential: 'cred-1' } })
+    ).rejects.toMatchObject({
       code: 'validation',
       message: expect.stringContaining('credentialId'),
     })
   })
 
+  /**
+   * The ledger de-duplicates on `eventKey`, and the derived key is a hash of
+   * actor, workspace, source and description — identical for every call to the
+   * same tool. Without a per-call id, `onConflictDoNothing` billed the first
+   * hosted-key call and silently dropped every one after it.
+   */
+  it('gives each call its own ledger event so repeat calls all bill', async () => {
+    mocks.executeRegistryTool.mockResolvedValue({
+      success: true,
+      output: { cost: { total: 0.004 } },
+    })
+
+    await run()
+    await run()
+
+    const keys = mocks.recordUsage.mock.calls.map((call) => call[0].entries[0].eventKey)
+    expect(keys).toHaveLength(2)
+    expect(keys[0]).toBeTruthy()
+    expect(keys[0]).not.toBe(keys[1])
+  })
+
   it('refuses a workspace API key: the call runs under a person or not at all', async () => {
     await expect(
       executeToolForCaller.execute({

apps/sim/lib/tool-execution/application/execute-tool.ts

@@ -1,5 +1,6 @@
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
+import { generateId } from '@sim/utils/id'
 import { resolveBillingAttribution, toBillingContext } from '@/lib/billing/core/billing-attribution'
 import { recordUsage } from '@/lib/billing/core/usage-log'
 import {
@@ -41,44 +42,46 @@ export interface ExecuteToolResult {
 }
 
 /**
- * Keys the caller may not set, because the execution path assigns them.
+ * Refuses an input key the tool does not declare.
  *
- * `_context` carries the acting identity and `enforceCredentialAccess`, and the
- * `__`-prefixed fields are the reserved transient channel `stripInternalFields`
- * documents — `__usingHostedKey` among them, which decides whether a tool bills
- * its call as hosted spend. No tool in the registry declares a parameter
- * starting with `_`, so one rule covers both and cannot collide with a real
- * argument. Refused rather than silently dropped: a caller who believed it set
- * something must not be told the call succeeded as sent.
- */
-function assertNoReservedArguments(args: Record<string, unknown>): void {
-  const reserved = Object.keys(args).find((key) => key.startsWith('_'))
-  if (reserved) {
-    throw new OrchestrationError(
-      'validation',
-      `input.${reserved} is reserved and cannot be supplied; Sim sets it from the authenticated caller`
-    )
-  }
-}
-
-/**
- * Credential fields are named once, at the top level.
+ * Strict rather than a denylist, because the denylist was already wrong twice
+ * over. `_context` carries the acting identity and `enforceCredentialAccess`;
+ * the `__`-prefixed fields are the reserved transient channel, `__usingHostedKey`
+ * among them, which decides whether a call bills as hosted spend; and
+ * `impersonateUserEmail` is read straight out of params by the executor and
+ * forwarded to credential-token resolution as an impersonation request. Naming
+ * those three is guesswork about a surface that keeps growing — a declared
+ * parameter list is the actual boundary, and it is what
+ * `GET /api/v2/tools/{toolId}` already publishes.
  *
- * The registry accepts three spellings of the same selection — `credential`,
- * `credentialId`, `oauthCredential` — which is fine inside the executor, where
- * one caller writes one of them, and wrong on a public contract, where three
- * spellings with undefined precedence is a shape no client can reason about.
+ * Also collapses the credential spellings. The executor accepts `credential`,
+ * `credentialId` and `oauthCredential` interchangeably, which is fine where one
+ * caller writes one of them and wrong on a public contract: three spellings with
+ * undefined precedence is a shape no client can reason about. The credential is
+ * named once, at the top level.
  */
-const CREDENTIAL_ARGUMENT_ALIASES = ['credential', 'credentialId', 'oauthCredential'] as const
+function assertNoUndeclaredInputs(
+  tool: ExecutableToolConfig,
+  toolId: string,
+  args: Record<string, unknown>
+): void {
+  const undeclared = Object.keys(args).filter((key) => !Object.hasOwn(tool.params ?? {}, key))
+  if (undeclared.length === 0) return
 
-function assertNoInlineCredential(args: Record<string, unknown>): void {
-  const alias = CREDENTIAL_ARGUMENT_ALIASES.find((key) => key in args)
-  if (alias) {
+  const credentialAlias = undeclared.find((key) =>
+    ['credential', 'credentialId', 'oauthCredential'].includes(key)
+  )
+  if (credentialAlias) {
     throw new OrchestrationError(
       'validation',
-      `input.${alias} is not accepted; pass the credential as the top-level credentialId field`
+      `input.${credentialAlias} is not accepted; pass the credential as the top-level credentialId field`
     )
   }
+
+  throw new OrchestrationError(
+    'validation',
+    `${toolId} does not accept ${undeclared.map((key) => `input.${key}`).join(', ')}`
+  )
 }
 
 /**
@@ -171,9 +174,6 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
     loadCatalogWorkspaceContext(input.workspaceId),
   authorizationOptions: {},
   execute: async ({ principal, input, context }): Promise<ExecuteToolResult> => {
-    assertNoReservedArguments(input.input)
-    assertNoInlineCredential(input.input)
-
     const gate = await resolveCatalogGate(principal, context)
 
     /**
@@ -205,6 +205,7 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
         `credentialId is required: ${toolId} authenticates with a ${tool.oauth.provider} credential`
       )
     }
+    assertNoUndeclaredInputs(tool, toolId, input.input)
     assertRequiredCallerInputsPresent(tool, toolId, input.input)
 
     const userId = principalUserId(principal)
@@ -234,6 +235,16 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
       },
     }
 
+    /**
+     * The ledger de-duplicates on `eventKey`, and the derived key is a hash of
+     * actor, workspace, source and description — identical for every call to the
+     * same tool. Without a per-call id `onConflictDoNothing` silently billed the
+     * first hosted-key call and nothing after it. A workflow run has an
+     * `executionId` to distinguish its rows; a direct call has nothing, so it
+     * mints one.
+     */
+    const callId = generateId()
+
     const result = await executeRegistryTool(toolId, params, {
       signal: AbortSignal.timeout((input.timeoutSeconds ?? DEFAULT_TIMEOUT_SECONDS) * 1000),
       operationContext: {
@@ -251,6 +262,7 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
     })
 
     await meterHostedKeySpend({
+      callId,
       toolId,
       userId,
       workspaceId: context.workspaceId,
@@ -282,6 +294,7 @@ export const executeToolForCaller = defineAuthorizedWorkspaceUseCase({
  * `applyHostedKeyCostToResult` makes one layer down.
  */
 async function meterHostedKeySpend(args: {
+  callId: string
   toolId: string
   userId: string
   workspaceId: string
@@ -304,6 +317,7 @@ async function meterHostedKeySpend(args: {
           source: 'api-tool',
           description: `Tool call: ${args.toolId}`,
           cost,
+          eventKey: args.callId,
         },
       ],
     })

apps/sim/lib/tool-execution/application/operations.ts

@@ -28,7 +28,7 @@ import { defineWorkspaceOperation } from '@/lib/core/application'
  * kinds ahead of a surface that uses them.
  */
 export const toolExecutionOperations = {
-  // permission-group-exempt: no static capability names running one built-in tool — the per-tool denial is the deniedTools key, applied inside @/tools against the resolved id, and the per-integration denial is the parameterized allowedIntegrations key, which the funnel cannot apply because it never sees which integration a tool id reaches. That decision is enforced from the use case by the owning-block-type check in executeToolForCaller, ahead of dispatch.
+  // permission-group-exempt: declares capability: 'none' because no static capability names running one built-in tool — the per-tool denial is the deniedTools key, applied inside @/tools against the resolved id, and the per-integration denial is the parameterized allowedIntegrations key, which the funnel cannot apply because it never sees which integration a tool id reaches. That decision is enforced from the use case by the owning-block-type check in executeToolForCaller, ahead of dispatch.
   execute: defineWorkspaceOperation({
     id: 'tools.execute',
     minimumRole: 'write',

apps/sim/lib/workflows/migrations/subblock-migrations.test.ts

@@ -11,6 +11,7 @@ import * as blocksBarrel from '@/blocks'
 import { getBlock as getRealBlock } from '@/blocks/registry'
 import {
   backfillCanonicalModes,
+  migrateCanonicalModeIds,
   migrateSubblockIds,
   SUBBLOCK_ID_MIGRATIONS,
 } from './subblock-migrations'
@@ -745,6 +746,73 @@ describe('migrateSubblockIds', () => {
   })
 })
 
+describe('migrateCanonicalModeIds', () => {
+  function mistralBlock(data: Record<string, unknown>, subBlocks: Record<string, unknown>) {
+    return makeBlock({ type: 'mistral_parse_v3', data, subBlocks } as never)
+  }
+
+  it('carries the selection across the document -> file rename', () => {
+    const { blocks, migrated } = migrateCanonicalModeIds({
+      b1: mistralBlock({ canonicalModes: { document: 'advanced' } }, {}),
+    })
+
+    expect(migrated).toBe(true)
+    const modes = blocks.b1.data?.canonicalModes as Record<string, string>
+    expect(modes).toEqual({ file: 'advanced' })
+  })
+
+  /**
+   * The case the backfill alone cannot recover. `setBlockCanonicalMode` writes
+   * the mode without clearing the sibling, so a workflow that uploaded a file,
+   * switched to advanced, then typed a reference holds both values — and
+   * `resolveCanonicalMode` prefers basic whenever the basic side is populated.
+   * Without the rename the run would silently switch to the uploaded file.
+   */
+  it('preserves advanced when both sides hold a value, which the backfill would not', () => {
+    const both = {
+      fileUpload: { id: 'fileUpload', type: 'file-upload', value: { name: 'a.pdf' } },
+      fileReference: { id: 'fileReference', type: 'short-input', value: '<block.file>' },
+    }
+
+    const { blocks } = migrateCanonicalModeIds({
+      b1: mistralBlock({ canonicalModes: { document: 'advanced' } }, both),
+    })
+    expect((blocks.b1.data?.canonicalModes as Record<string, string>).file).toBe('advanced')
+
+    // Same input through the backfill alone resolves to basic — the regression
+    // this migration exists to prevent.
+    const { blocks: backfilled } = backfillCanonicalModes({
+      b1: mistralBlock({ canonicalModes: {} }, both),
+    })
+    expect((backfilled.b1.data?.canonicalModes as Record<string, string>).file).toBe('basic')
+  })
+
+  it('leaves a block that already stores the current id alone', () => {
+    const { blocks, migrated } = migrateCanonicalModeIds({
+      b1: mistralBlock({ canonicalModes: { file: 'basic' } }, {}),
+    })
+
+    expect(migrated).toBe(false)
+    expect(blocks.b1.data?.canonicalModes).toEqual({ file: 'basic' })
+  })
+
+  it('prefers a value already written under the current id over the legacy one', () => {
+    const { blocks } = migrateCanonicalModeIds({
+      b1: mistralBlock({ canonicalModes: { document: 'advanced', file: 'basic' } }, {}),
+    })
+
+    expect(blocks.b1.data?.canonicalModes).toEqual({ file: 'basic' })
+  })
+
+  it('does not touch a block type with no canonical rename', () => {
+    const { migrated } = migrateCanonicalModeIds({
+      b1: makeBlock({ type: 'knowledge', data: { canonicalModes: { document: 'advanced' } } }),
+    })
+
+    expect(migrated).toBe(false)
+  })
+})
+
 describe('backfillCanonicalModes', () => {
   it('should add missing canonicalModes entry for knowledge block with basic value', () => {
     const input: Record<string, BlockState> = {