improvement(tools): stop the secret guard from faking a difference between copies

Two copies pinned to identical values, where only one of them resolved an
environment variable, rendered different text — one withheld its literal — and
both were then told "other copies are pinned to different values". That is the
exact assertion the comparison exists to prevent. The duplicate check now
compares the un-withheld render, so disclosure differences no longer read as
configuration differences.

Also drops a redundant copy of the resolved-name cache, returns undefined rather
than an empty-string sentinel for an unresolved resource, unexports two
internal-only interfaces, and corrects six comments: five overstated or
referenced the module this branch renamed, and one described the wrong failure
mode for a credential entered in advanced mode.

Adds the uncovered branches the review named: canonical-id grouping (the shape
production actually sees once wire ids are aliased), a sibling that states
nothing, negative-cache reuse, both resource kinds in one pass, an omitted tool
param map, empty and oversized titles, and a non-finite number.

Author: waleedlatif1

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

@@ -1385,8 +1385,6 @@ export class AgentBlockHandler implements BlockHandler {
       usageControl: config.usageControl || 'auto',
     }
 
-    // An MCP tool has no block subblocks to describe it, so its pinned params are read straight
-    // from the configured values, keyed by the remote schema's own names.
     const { formatParameterLabel, isPasswordParameter } = await import('@/tools/params')
     registerToolPinnedFields(
       mcpTool,

apps/sim/executor/utils/tool-pinned-params.test.ts

@@ -190,6 +190,79 @@ describe('annotateToolPinnedParams', () => {
     expect(appended(second[0])).toContain('Gmail Account "Support Inbox"')
   })
 
+  it('does not claim copies differ when only their secret disclosure does', async () => {
+    const open = providerTool('gmail_read_email', [account('cred-a'), label('INBOX')])
+    const secret = providerTool('gmail_read_email', [account('cred-a'), label('INBOX')])
+
+    // Identical pins; only `secret` resolved an env variable, so its literal is withheld.
+    await annotateToolPinnedParams(ctx(), [open, secret], (tool) => tool === secret)
+
+    expect(appended(open)).toContain('Label "INBOX".')
+    expect(appended(secret)).not.toContain('INBOX')
+    expect(appended(open)).not.toContain('Other copies')
+    expect(appended(secret)).not.toContain('Other copies')
+  })
+
+  it('groups copies by canonical id once the wire ids have been aliased', async () => {
+    const first = providerTool('gmail_read_email', [label('INBOX')])
+    const second = providerTool('gmail_read_email__sim_2', [label('SENT')])
+    second.canonicalId = 'gmail_read_email'
+
+    await annotateToolPinnedParams(ctx(), [first, second])
+
+    expect(appended(first)).toContain('Other copies')
+    expect(appended(second)).toContain('Other copies')
+  })
+
+  it('does not group tools that only share a wire id shape', async () => {
+    const first = providerTool('gmail_read_email', [label('INBOX')])
+    const second = providerTool('slack_send_message', [label('SENT')])
+
+    await annotateToolPinnedParams(ctx(), [first, second])
+
+    expect(appended(first)).not.toContain('Other copies')
+    expect(appended(second)).not.toContain('Other copies')
+  })
+
+  it('gives no hint when a sibling states nothing at all', async () => {
+    const stated = providerTool('gmail_read_email', [label('INBOX')])
+    const silent = providerTool('gmail_read_email', [account('cred-deleted')])
+
+    await annotateToolPinnedParams(ctx(), [stated, silent])
+
+    expect(appended(stated)).toContain('Label "INBOX".')
+    expect(silent.description).toBe(BASE)
+    expect(appended(stated)).not.toContain('Other copies')
+  })
+
+  it('does not re-query an id that already failed to resolve', async () => {
+    const cache = new Map<string, string | null>()
+
+    await annotateToolPinnedParams(ctx(cache), [
+      providerTool('gmail_read_email', [account('cred-deleted'), label('A')]),
+      providerTool('gmail_read_email', [account('cred-deleted'), label('B')]),
+    ])
+    expect(mockFindWorkspaceCredentialLookup).toHaveBeenCalledTimes(1)
+
+    await annotateToolPinnedParams(ctx(cache), [
+      providerTool('gmail_read_email', [account('cred-deleted'), label('C')]),
+    ])
+    expect(mockFindWorkspaceCredentialLookup).toHaveBeenCalledTimes(1)
+  })
+
+  it('resolves both resource kinds in one pass', async () => {
+    mockGetKnowledgeBaseNames.mockResolvedValue(new Map([['kb-a', 'Support Docs']]))
+    const gmail = providerTool('gmail_read_email', [account('cred-a')])
+    const kb = providerTool('knowledge_search', [
+      { title: 'Knowledge Base', resource: { kind: 'knowledgeBase', id: 'kb-a' } },
+    ])
+
+    await annotateToolPinnedParams(ctx(), [gmail, kb])
+
+    expect(appended(gmail)).toContain('Gmail Account "Support Inbox"')
+    expect(appended(kb)).toContain('Knowledge Base "Support Docs"')
+  })
+
   it('does nothing without a workspace', async () => {
     const tool = providerTool('gmail_read_email', [label('INBOX')])
 

apps/sim/executor/utils/tool-pinned-params.ts

@@ -47,29 +47,49 @@ const RESOLVERS: Record<BoundResourceKind, ResourceNameResolver> = {
   knowledgeBase: (ids, workspaceId) => getKnowledgeBaseNames(ids, workspaceId),
 }
 
-function renderField(field: ToolPinnedField, resolved: ReadonlyMap<string, string>): string {
+function renderField(
+  field: ToolPinnedField,
+  resolved: ReadonlyMap<string, string | null>
+): string | undefined {
   if ('resource' in field) {
     const name = sanitizeStatedText(
       resolved.get(`${field.resource.kind}:${field.resource.id}`) ?? ''
     )
-    return name ? `${field.title} "${name}"` : ''
+    return name ? `${field.title} "${name}"` : undefined
   }
   return typeof field.value === 'string'
     ? `${field.title} "${field.value}"`
     : `${field.title} ${field.value}`
 }
 
+/** Joins what one tool states, or undefined when it has nothing to say. */
+function buildStatement(
+  fields: readonly ToolPinnedField[],
+  resolved: ReadonlyMap<string, string | null>,
+  withholdLiterals: boolean
+): string | undefined {
+  const rendered: string[] = []
+  for (const field of fields) {
+    if (rendered.length === MAX_STATED_FIELDS) break
+    if (withholdLiterals && !('resource' in field)) continue
+    const text = renderField(field, resolved)
+    if (text !== undefined) rendered.push(text)
+  }
+  return rendered.length > 0 ? rendered.join(', ') : undefined
+}
+
 /**
  * Tells the model which values a workflow pinned on a tool, and — when the agent holds several
  * copies of that tool that differ — that it must pick the right one.
  *
- * Every pinned param is stripped from the schema the model sees (`createLLMToolSchema` drops any
- * param the user filled), so without this the model cannot tell that a Gmail tool reads only
- * `INBOX`, cannot distinguish it from a sibling reading `SENT`, and may promise a caller it will
- * search a folder it can never reach.
+ * A filled param is stripped from the schema the model sees — `createLLMToolSchema` skips it for
+ * block tools, `filterSchemaForLLM` for MCP ones — so without this the model cannot tell that a
+ * Gmail tool reads only `INBOX`, cannot distinguish it from a sibling reading `SENT`, and may
+ * promise a caller it will search a folder it can never reach.
  *
  * Mutates `description` on the exact objects passed in. Provenance elsewhere is keyed on tool
- * identity, so no tool is ever replaced. Never throws: an unresolvable value is simply omitted.
+ * identity, so no tool is ever replaced. A failed name lookup never fails the block; it just
+ * leaves that field unstated. `withholdLiteralValues` is called uncaught and must not throw.
  *
  * `withholdLiteralValues` marks a tool whose configured params resolved an environment secret.
  * Its literal values are suppressed; resolved resource names still state, since a looked-up name
@@ -121,37 +141,28 @@ export async function annotateToolPinnedParams(
     })
   )
 
-  const resolvedNames = new Map<string, string>()
-  for (const [key, name] of cache) if (name) resolvedNames.set(key, name)
-
+  // Only claim the copies differ when their pinned values actually do. The comparison uses the
+  // un-withheld render on purpose: two copies pinned identically, where only one of them resolved
+  // an env secret, differ solely in what is disclosed — telling the model they are "pinned to
+  // different values" would assert a distinction it cannot act on. Comparing only the first
+  // MAX_STATED_FIELDS can still miss a difference beyond the cap, which under-warns rather than
+  // mis-warns.
   const statements = new Map<ProviderToolConfig, string>()
+  const comparableByCanonicalId = new Map<string, Set<string>>()
   for (const { tool, fields } of annotatable) {
-    const withhold = withholdLiteralValues?.(tool) ?? false
-    const rendered: string[] = []
-    for (const field of fields) {
-      if (rendered.length === MAX_STATED_FIELDS) break
-      if (withhold && !('resource' in field)) continue
-      const text = renderField(field, resolvedNames)
-      if (text) rendered.push(text)
-    }
-    if (rendered.length > 0) statements.set(tool, rendered.join(', '))
-  }
-
-  // Only claim the copies differ when their stated values actually do. Two tools bound to the same
-  // account and folder render identically, and telling the model to "call the copy the request
-  // refers to" would assert a distinction it cannot act on.
-  const statementsByCanonicalId = new Map<string, Set<string>>()
-  for (const tool of tools) {
-    const statement = statements.get(tool)
+    const statement = buildStatement(fields, cache, withholdLiteralValues?.(tool) ?? false)
     if (statement === undefined) continue
+    statements.set(tool, statement)
+
+    const comparable = buildStatement(fields, cache, false) ?? statement
     const key = tool.canonicalId ?? tool.id
-    const seen = statementsByCanonicalId.get(key)
-    if (seen) seen.add(statement)
-    else statementsByCanonicalId.set(key, new Set([statement]))
+    const seen = comparableByCanonicalId.get(key)
+    if (seen) seen.add(comparable)
+    else comparableByCanonicalId.set(key, new Set([comparable]))
   }
 
   for (const [tool, statement] of statements) {
-    const distinct = statementsByCanonicalId.get(tool.canonicalId ?? tool.id)?.size ?? 1
+    const distinct = comparableByCanonicalId.get(tool.canonicalId ?? tool.id)?.size ?? 1
     const duplicateHint =
       distinct > 1
         ? ' Other copies of this tool on this agent are pinned to different values — call the copy the request refers to.'

apps/sim/providers/tool-binding.test.ts

@@ -225,6 +225,46 @@ describe('collectToolPinnedFields', () => {
       expect(collect({ subBlocks: credentialPair, resolvedResourceParams: params })).toEqual([])
     }
   })
+
+  it('states nothing when the caller omits the tool param map', () => {
+    expect(
+      collect({
+        subBlocks: [sub({ id: 'folder', title: 'Label', type: 'folder-selector' })],
+        userProvidedParams: { folder: 'INBOX' },
+      })
+    ).toEqual([])
+  })
+
+  it('drops a field whose title sanitizes to nothing', () => {
+    expect(
+      collect({
+        subBlocks: [sub({ id: 'folder', title: '""', type: 'folder-selector' })],
+        userProvidedParams: { folder: 'INBOX' },
+        toolParams: toolParams('folder'),
+        formatParamLabel: () => '""',
+      })
+    ).toEqual([])
+  })
+
+  it('truncates an oversized title', () => {
+    const fields = collect({
+      subBlocks: [sub({ id: 'folder', title: 'T'.repeat(80), type: 'folder-selector' })],
+      userProvidedParams: { folder: 'INBOX' },
+      toolParams: toolParams('folder'),
+    })
+
+    expect(fields[0].title).toBe(`${'T'.repeat(40)}…`)
+  })
+
+  it('drops a non-finite number', () => {
+    expect(
+      collect({
+        subBlocks: [sub({ id: 'ratio', title: 'Ratio', type: 'short-input' })],
+        userProvidedParams: { ratio: Number.NaN },
+        toolParams: toolParams('ratio'),
+      })
+    ).toEqual([])
+  })
 })
 
 describe('collectPinnedFieldsFromParams', () => {
@@ -256,6 +296,12 @@ describe('sanitizeStatedText', () => {
 })
 
 describe('pinned field registration', () => {
+  it('stores nothing for an empty list, so callers see undefined', () => {
+    const tool = { id: 'gmail_read_email' }
+    registerToolPinnedFields(tool, [])
+    expect(getToolPinnedFields(tool)).toBeUndefined()
+  })
+
   it('reads back the fields registered for that exact tool object', () => {
     const tool = { id: 'gmail_read_email' }
     const field = { title: 'Label', value: 'INBOX' } as const

apps/sim/providers/tool-binding.ts

@@ -3,7 +3,7 @@ import type { SubBlockType } from '@sim/workflow-types/blocks'
 import type { SubBlockConfig } from '@/blocks/types'
 import { isNonEmpty } from '@/tools/merge-params'
 
-/** Resource kinds whose configured value is an opaque id the labeller must resolve to a name. */
+/** Resource kinds whose configured value is an opaque id that must be resolved to a name. */
 export type BoundResourceKind = 'credential' | 'knowledgeBase'
 
 /**
@@ -16,7 +16,7 @@ export type ToolPinnedField =
 
 /**
  * A workflow id is resolvable too, but its name is already fetched during tool transformation, so
- * it never reaches the labeller as an unresolved resource.
+ * it is stated as a literal and never leaves this module as an unresolved resource.
  */
 type ResolvableKind = BoundResourceKind | 'workflow'
 
@@ -114,7 +114,7 @@ function statedValue(value: unknown): string | number | boolean | undefined {
   return sanitizeStatedText(value) || undefined
 }
 
-export interface PinnedFieldSourceOptions {
+interface PinnedFieldSourceOptions {
   formatParamLabel: (paramId: string) => string
   /** `isPasswordParameter` from `@/tools/params`, injected to avoid a static registry-side edge. */
   isPasswordParam: (paramId: string) => boolean
@@ -125,8 +125,9 @@ function isSecretParamId(paramId: string, options: PinnedFieldSourceOptions): bo
 }
 
 /**
- * Pinned fields for a tool that has no block subblocks to describe it — an MCP or custom tool,
- * whose configured params are plain values keyed by the remote schema's own names.
+ * Pinned fields for a tool that has no block subblocks to describe it. Used by the MCP path, whose
+ * configured params are plain values keyed by the remote schema's own names. Custom tools take the
+ * same shape but are not wired to this yet.
  */
 export function collectPinnedFieldsFromParams(
   params: Record<string, unknown>,
@@ -182,8 +183,9 @@ export function collectToolPinnedFields(input: CollectToolPinnedFieldsInput): To
   if (!subBlocks?.length) return []
 
   // A canonical pair's advanced half is a plain `short-input`, so the kind has to come from the
-  // whole group rather than from whichever subblock is being scanned. Without this, a credential
-  // entered in advanced mode falls through to the literal path and is stated verbatim.
+  // whole group rather than from whichever subblock is being scanned. Without this a resource
+  // entered in advanced mode takes the literal path: a knowledge base id would be stated verbatim,
+  // and a credential would be dropped entirely by the secret-name check below.
   const kindByParamId = new Map<string, ResolvableKind>()
   for (const subBlock of subBlocks) {
     const kind = RESOURCE_SUBBLOCK_KINDS[subBlock.type]
@@ -194,7 +196,6 @@ export function collectToolPinnedFields(input: CollectToolPinnedFieldsInput): To
   const seenParamIds = new Set<string>()
 
   for (const subBlock of subBlocks) {
-    // A canonical pair contributes two subblocks (basic + advanced) for one logical field.
     const paramId = subBlock.canonicalParamId ?? subBlock.id
     if (seenParamIds.has(paramId)) continue
     if (selfDescribedParamId && paramId === selfDescribedParamId) continue

apps/sim/providers/utils.ts

@@ -896,7 +896,7 @@ export async function transformBlockTool(
   }
 
   // A tool that rewrote its own description from a bound param already names that resource, so the
-  // duplicate labeller must not state it twice. Keyed off the declaration rather than the rendered
+  // pinned-param annotation must not state it twice. Keyed off the declaration rather than the rendered
   // text; the inequality catches an enricher that returned the description unchanged.
   const selfDescribedParamId =
     enrichedDescription && enrichedDescription !== toolConfig.description