fix(tools): require every input the caller is the only source for

The previous commit gated the check on `user-only`, which reads the visibility
taxonomy as if it constrained who may send a value. It does not. `visibility`
describes editor roles — a human filling a block field, the agent block's model
choosing an argument, either, or neither — and a direct call has no editor and
no agent block, so those roles collapse to one caller.
`createUserToolSchema`, which this endpoint and Copilot's
`call_integration_tool` both publish, already says so by omitting `hidden` and
nothing else.

So the rule is not about roles: Sim supplies it, or the caller must. Skipping
`hidden` stays safe because `check-tool-param-reachability` fails any required
hidden parameter without a declared filler.

Concretely this closes `thinking_tool.thought`, the one required `llm-only`
parameter in the registry, which the narrower check let through as `undefined`.
It also moves required `user-or-llm` inputs to a pre-dispatch failure naming
every missing field at once, rather than the merge validator's first-failure
mid-execution.

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

Author: icecrasher321

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

@@ -111,6 +111,11 @@ const TOOL_METADATA: Record<string, Record<string, unknown>> = {
     },
     hosting: { apiKeyParam: 'apiKey' },
   },
+  thinking_tool: {
+    id: 'thinking_tool',
+    name: 'Thinking',
+    params: { thought: { type: 'string', required: true, visibility: 'llm-only' } },
+  },
   zendesk_get_ticket: {
     id: 'zendesk_get_ticket',
     name: 'Zendesk Get Ticket',
@@ -167,6 +172,7 @@ const previewBlock = block({
   tools: { access: ['preview_call'] },
 })
 const zendeskBlock = block({ type: 'zendesk', tools: { access: ['zendesk_get_ticket'] } })
+const thinkingBlock = block({ type: 'thinking', tools: { access: ['thinking_tool'] } })
 const confluenceBlock = block({
   type: 'confluence_v2',
   tools: { access: ['confluence_read_v2'] },
@@ -203,6 +209,7 @@ describe('executeToolForCaller', () => {
       previewBlock,
       confluenceBlock,
       zendeskBlock,
+      thinkingBlock,
     ])
     mocks.executeRegistryTool.mockResolvedValue({ success: true, output: { markdown: '# Hi' } })
     mocks.resolveBillingAttribution.mockResolvedValue({ workspaceId: WORKSPACE_ID })
@@ -342,6 +349,27 @@ describe('executeToolForCaller', () => {
     })
   })
 
+  /**
+   * `visibility` describes editor roles, and a direct call has no editor: the
+   * caller is the only source, so an `llm-only` parameter is as much theirs to
+   * send as a `user-only` one. Gating the check on `user-only` alone left
+   * `thinking_tool.thought` dispatching as `undefined`.
+   */
+  it('refuses a missing llm-only input too — the caller is the only source here', async () => {
+    await expect(run({ toolId: 'thinking_tool', input: {} })).rejects.toMatchObject({
+      code: 'validation',
+      message: expect.stringContaining('input.thought'),
+    })
+    expect(mocks.executeRegistryTool).not.toHaveBeenCalled()
+  })
+
+  it('refuses a missing user-or-llm input before dispatch rather than mid-execution', async () => {
+    await expect(run({ input: {} })).rejects.toMatchObject({
+      code: 'validation',
+      message: expect.stringContaining('input.url'),
+    })
+  })
+
   it('accepts a {{VAR}} reference as a present value, leaving resolution to the executor', async () => {
     await run({
       toolId: 'zendesk_get_ticket',

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

@@ -82,35 +82,50 @@ function assertNoInlineCredential(args: Record<string, unknown>): void {
 }
 
 /**
- * Refuses a call missing a required parameter only its caller can supply.
+ * Refuses a call missing a required parameter the caller was supposed to send.
  *
- * `validateRequiredParametersAfterMerge` deliberately checks `user-or-llm`
- * alone, because on the workflow path a `user-only` parameter was already
- * validated during serialization against the block field that holds it. This
- * path has no serialization step, so nothing had checked them — a caller
- * omitting `zendesk_get_ticket`'s `subdomain` reached Zendesk as `undefined`
- * and came back a provider authentication failure, which is the same
- * undiagnosable shape this PR set out to remove.
+ * `visibility` is an editor-role concept: it says whether a value comes from a
+ * human filling a block field (`user-only`), the agent block's model choosing an
+ * argument (`llm-only`), either (`user-or-llm`), or neither (`hidden`). A direct
+ * call has no editor and no agent block, so those roles collapse — the caller is
+ * the only source there is. `createUserToolSchema`, which is what both this
+ * endpoint and Copilot's `call_integration_tool` publish, already says as much
+ * by omitting `hidden` and nothing else.
  *
- * A parameter Sim itself supplies is not missing: `hosting` fills its
- * `apiKeyParam` on a key-hosting deployment, so `firecrawl_scrape` must stay
- * callable with no `apiKey`. The condition mirrors `injectHostedKeyIfNeeded`
- * exactly — same three tests, same order — so the two cannot disagree about
- * whether a value is coming.
+ * So the rule here is not about roles: **Sim supplies it, or the caller must.**
+ * `hidden` is skipped because Sim fills it from a resolved credential or a
+ * hosted key — and `check-tool-param-reachability` is what makes that safe to
+ * assume, since it fails any required `hidden` parameter without a declared
+ * filler.
+ *
+ * Nothing else had checked these. `validateRequiredParametersAfterMerge` covers
+ * `user-or-llm` alone, because on the workflow path the rest were validated
+ * during serialization against the block fields holding them, and this path has
+ * no serialization step. Omitting `zendesk_get_ticket`'s `subdomain` reached
+ * Zendesk as `undefined` and came back a provider authentication failure — the
+ * same undiagnosable shape this branch set out to remove.
  */
 function assertRequiredCallerInputsPresent(
   tool: ExecutableToolConfig,
   toolId: string,
   params: Record<string, unknown>
 ): void {
+  /**
+   * Mirrors `injectHostedKeyIfNeeded`'s three tests, in its order, so the two
+   * cannot disagree about whether a value is coming. Rejecting the omission
+   * would make every hosted-key tool uncallable without a key the caller does
+   * not need to hold; accepting it on a deployment that hosts none would
+   * promise a key nothing supplies.
+   */
   const hostedKeyParam =
     isHosted && tool.hosting && (!tool.hosting.enabled || tool.hosting.enabled(params))
       ? tool.hosting.apiKeyParam
       : undefined
 
   const missing = Object.entries(tool.params ?? {})
     .filter(([name, declaration]) => {
-      if (!declaration?.required || declaration.visibility !== 'user-only') return false
+      if (!declaration?.required) return false
+      if (declaration.visibility === 'hidden') return false
       if (name === hostedKeyParam) return false
       const value = params[name]
       return value === undefined || value === null || value === ''