fix(copilot): update secret descriptions without values

Author: Sg312

apps/sim/lib/copilot/generated/tool-catalog-v1.ts

@@ -4761,12 +4761,16 @@ export const SetEnvironmentVariables: ToolCatalogEntry = {
             description: {
               type: 'string',
               description:
-                'What the variable is for, in one short phrase — aim for under 80 characters, like "Stripe live key for the billing workflow". Not a sentence, and never a restatement of the name. Workspace scope only; sending it with scope personal is rejected. Omit it on an existing variable to leave its current description untouched; send an empty string to clear one.',
+                'What the variable is for, in one short phrase — aim for under 80 characters, like "Stripe live key for the billing workflow". Not a sentence, and never a restatement of the name. Workspace scope only; sending it with scope personal is rejected. Omit it on an existing variable to leave its current description untouched; send an empty string to clear one. You may send it alone, without a value, to describe a secret that already exists.',
             },
             name: { type: 'string', description: 'Variable name' },
-            value: { type: 'string', description: 'Variable value' },
+            value: {
+              type: 'string',
+              description:
+                "Variable value. Omit it to leave an existing variable's value untouched and change only its description — never invent or guess a value you were not given, which would overwrite the real secret.",
+            },
           },
-          required: ['name', 'value'],
+          required: ['name'],
         },
       },
     },

apps/sim/lib/copilot/generated/tool-schemas-v1.ts

@@ -4602,18 +4602,19 @@ export const TOOL_RUNTIME_SCHEMAS: Record<string, ToolRuntimeSchemaEntry> = {
               description: {
                 type: 'string',
                 description:
-                  'What the variable is for, in one short phrase — aim for under 80 characters, like "Stripe live key for the billing workflow". Not a sentence, and never a restatement of the name. Workspace scope only; sending it with scope personal is rejected. Omit it on an existing variable to leave its current description untouched; send an empty string to clear one.',
+                  'What the variable is for, in one short phrase — aim for under 80 characters, like "Stripe live key for the billing workflow". Not a sentence, and never a restatement of the name. Workspace scope only; sending it with scope personal is rejected. Omit it on an existing variable to leave its current description untouched; send an empty string to clear one. You may send it alone, without a value, to describe a secret that already exists.',
               },
               name: {
                 type: 'string',
                 description: 'Variable name',
               },
               value: {
                 type: 'string',
-                description: 'Variable value',
+                description:
+                  "Variable value. Omit it to leave an existing variable's value untouched and change only its description — never invent or guess a value you were not given, which would overwrite the real secret.",
               },
             },
-            required: ['name', 'value'],
+            required: ['name'],
           },
         },
       },

apps/sim/lib/copilot/tools/server/user/set-environment-variables.test.ts

@@ -16,16 +16,22 @@ const {
   ensureWorkflowAccessMock,
   ensureWorkspaceAccessMock,
   getDefaultWorkspaceIdMock,
-  setWorkspaceSecretMock,
+  listCredentialsMock,
+  performUpdateCredentialMock,
 } = vi.hoisted(() => ({
   ensureWorkflowAccessMock: vi.fn(),
   ensureWorkspaceAccessMock: vi.fn(),
   getDefaultWorkspaceIdMock: vi.fn(),
-  setWorkspaceSecretMock: vi.fn(),
+  listCredentialsMock: vi.fn(),
+  performUpdateCredentialMock: vi.fn(),
 }))
 
-vi.mock('@/lib/credentials/secret-values', () => ({
-  setWorkspaceSecret: setWorkspaceSecretMock,
+vi.mock('@/lib/credentials/queries', () => ({
+  listVisibleWorkspaceCredentials: listCredentialsMock,
+}))
+
+vi.mock('@/lib/credentials/orchestration', () => ({
+  performUpdateCredential: performUpdateCredentialMock,
 }))
 
 vi.mock('@/lib/copilot/tools/handlers/access', () => ({
@@ -46,7 +52,13 @@ describe('setEnvironmentVariablesServerTool', () => {
     getDefaultWorkspaceIdMock.mockResolvedValue('ws-default')
     upsertPersonalEnvVarsMock.mockResolvedValue({ added: ['API_KEY'], updated: [] })
     upsertWorkspaceEnvVarsMock.mockResolvedValue(['API_KEY'])
-    setWorkspaceSecretMock.mockResolvedValue({ created: false, updatedAt: new Date() })
+    listCredentialsMock.mockResolvedValue({
+      data: [
+        { id: 'cred-api', envKey: 'API_KEY' },
+        { id: 'cred-other', envKey: 'OTHER_KEY' },
+      ],
+    })
+    performUpdateCredentialMock.mockResolvedValue({ success: true })
   })
 
   it('defaults to workspace scope and uses the current workspace context', async () => {
@@ -103,7 +115,7 @@ describe('setEnvironmentVariablesServerTool', () => {
     )
   })
 
-  it('describes a workspace secret through the same writer the Set Secret API uses', async () => {
+  it('describes a workspace secret through the credential update handler, never rewriting its value', async () => {
     await setEnvironmentVariablesServerTool.execute(
       {
         variables: [
@@ -114,19 +126,58 @@ describe('setEnvironmentVariablesServerTool', () => {
       { userId: 'user-1', workspaceId: 'ws-1' }
     )
 
-    expect(setWorkspaceSecretMock).toHaveBeenCalledTimes(1)
-    expect(setWorkspaceSecretMock).toHaveBeenCalledWith({
-      workspaceId: 'ws-1',
-      name: 'API_KEY',
-      value: 'secret',
+    expect(performUpdateCredentialMock).toHaveBeenCalledTimes(1)
+    expect(performUpdateCredentialMock).toHaveBeenCalledWith({
+      credentialId: 'cred-api',
       userId: 'user-1',
       description: 'Stripe live key',
+      allowedTypes: ['env_workspace'],
     })
-    // The access-checked bulk write still runs first: it is what authorizes the
-    // caller and mints the credential row a new key's description needs.
+    // The access-checked value write runs first: it authorizes the caller and
+    // mints the credential row a new key's description hangs on.
     expect(upsertWorkspaceEnvVarsMock.mock.invocationCallOrder[0]).toBeLessThan(
-      setWorkspaceSecretMock.mock.invocationCallOrder[0]
+      performUpdateCredentialMock.mock.invocationCallOrder[0]
+    )
+  })
+
+  it('describes a secret that already exists without touching its value', async () => {
+    const result = await setEnvironmentVariablesServerTool.execute(
+      { variables: [{ name: 'API_KEY', description: 'Stripe live key' }] },
+      { userId: 'user-1', workspaceId: 'ws-1' }
+    )
+
+    // Nothing is written to the secret itself: coercing the absent value to ''
+    // would blank the very secret the model is annotating.
+    expect(upsertWorkspaceEnvVarsMock).toHaveBeenCalledWith('ws-1', {}, 'user-1')
+    expect(performUpdateCredentialMock).toHaveBeenCalledWith(
+      expect.objectContaining({ credentialId: 'cred-api', description: 'Stripe live key' })
+    )
+    expect(result.describedVariables).toEqual(['API_KEY'])
+  })
+
+  it('keeps a stored value reported when its description fails', async () => {
+    performUpdateCredentialMock.mockResolvedValue({ success: false, error: 'Forbidden' })
+
+    const result = await setEnvironmentVariablesServerTool.execute(
+      { variables: [{ name: 'API_KEY', value: 'secret', description: 'Stripe live key' }] },
+      { userId: 'user-1', workspaceId: 'ws-1' }
     )
+
+    expect(result.workspaceUpdatedVariables).toEqual(['API_KEY'])
+    expect(result.describedVariables).toEqual([])
+    expect(result.message).toContain('API_KEY: Forbidden')
+  })
+
+  it('fails a describe-only call that saved nothing', async () => {
+    performUpdateCredentialMock.mockResolvedValue({ success: false, error: 'Forbidden' })
+    upsertWorkspaceEnvVarsMock.mockResolvedValue([])
+
+    await expect(
+      setEnvironmentVariablesServerTool.execute(
+        { variables: [{ name: 'API_KEY', description: 'Stripe live key' }] },
+        { userId: 'user-1', workspaceId: 'ws-1' }
+      )
+    ).rejects.toThrow('Could not describe: API_KEY: Forbidden')
   })
 
   it('clears a description sent blank and leaves an omitted one alone', async () => {
@@ -140,9 +191,9 @@ describe('setEnvironmentVariablesServerTool', () => {
       { userId: 'user-1', workspaceId: 'ws-1' }
     )
 
-    expect(setWorkspaceSecretMock).toHaveBeenCalledTimes(1)
-    expect(setWorkspaceSecretMock).toHaveBeenCalledWith(
-      expect.objectContaining({ name: 'API_KEY', description: null })
+    expect(performUpdateCredentialMock).toHaveBeenCalledTimes(1)
+    expect(performUpdateCredentialMock).toHaveBeenCalledWith(
+      expect.objectContaining({ credentialId: 'cred-api', description: null })
     )
   })
 

apps/sim/lib/copilot/tools/server/user/set-environment-variables.ts

@@ -7,7 +7,8 @@ import {
   getDefaultWorkspaceId,
 } from '@/lib/copilot/tools/handlers/access'
 import type { BaseServerTool, ServerToolContext } from '@/lib/copilot/tools/server/base-tool'
-import { setWorkspaceSecret } from '@/lib/credentials/secret-values'
+import { performUpdateCredential } from '@/lib/credentials/orchestration'
+import { listVisibleWorkspaceCredentials } from '@/lib/credentials/queries'
 import { upsertPersonalEnvVars, upsertWorkspaceEnvVars } from '@/lib/environment/utils'
 
 type EnvironmentVariableInputValue = string | number | boolean | null | undefined
@@ -37,10 +38,20 @@ interface SetEnvironmentVariablesResult {
   addedVariables: string[]
   updatedVariables: string[]
   workspaceUpdatedVariables: string[]
+  describedVariables: string[]
 }
 
 const EnvVarSchema = z.object({ variables: z.record(z.string(), z.string()) })
 
+/** A row that only annotates a secret that already exists — it sends no value. */
+function isDescriptionOnly(item: EnvironmentVariableInput): boolean {
+  return (
+    (item.value === undefined || item.value === null) &&
+    typeof item.description === 'string' &&
+    item.description.trim().length > 0
+  )
+}
+
 /**
  * Collects the descriptions the model actually sent. A variable that omits the
  * field is absent from the result, so an existing description survives a value
@@ -65,13 +76,18 @@ function normalizeDescriptions(
   return descriptions
 }
 
+/**
+ * Values to write. An array item that carries a description but no value is a
+ * description-only edit and is deliberately absent here: coercing its missing
+ * value to `''` would blank the very secret the model is trying to annotate.
+ */
 function normalizeVariables(
   input: Record<string, EnvironmentVariableInputValue> | EnvironmentVariableInput[]
 ): Record<string, string> {
   if (Array.isArray(input)) {
     return input.reduce(
       (acc, item) => {
-        if (item && typeof item.name === 'string') {
+        if (item && typeof item.name === 'string' && !isDescriptionOnly(item)) {
           acc[item.name] = String(item.value ?? '')
         }
         return acc
@@ -84,6 +100,57 @@ function normalizeVariables(
   ) as Record<string, string>
 }
 
+/**
+ * Writes descriptions onto secrets, never their values. Resolves each key to its
+ * credential row and hands it to `performUpdateCredential` — the same handler the
+ * secrets settings page calls — so credential-admin access, the `env_personal`
+ * refusal, and the audit record are all decided in one place.
+ *
+ * Only the `description` column is touched. Re-sending the value to attach a note
+ * would clobber a rotation that landed between the two writes, and the note is
+ * never worth losing someone else's secret over.
+ */
+async function describeSecrets(params: {
+  workspaceId: string
+  userId: string
+  descriptions: Record<string, string | null>
+}): Promise<{ described: string[]; failures: string[] }> {
+  const names = Object.keys(params.descriptions)
+  if (names.length === 0) return { described: [], failures: [] }
+
+  const { data: credentials } = await listVisibleWorkspaceCredentials({
+    workspaceId: params.workspaceId,
+    userId: params.userId,
+    workspaceAccess: { canAdmin: false },
+    types: ['env_workspace'],
+  })
+  const idByEnvKey = new Map(
+    credentials.flatMap((row) => (row.envKey ? [[row.envKey, row.id] as const] : []))
+  )
+
+  const described: string[] = []
+  const failures: string[] = []
+  for (const name of names) {
+    const credentialId = idByEnvKey.get(name)
+    if (!credentialId) {
+      failures.push(`no workspace secret named ${name}`)
+      continue
+    }
+    const result = await performUpdateCredential({
+      credentialId,
+      userId: params.userId,
+      description: params.descriptions[name],
+      allowedTypes: ['env_workspace'],
+    })
+    if (result.success) {
+      described.push(name)
+    } else {
+      failures.push(`${name}: ${result.error ?? 'could not be described'}`)
+    }
+  }
+  return { described, failures }
+}
+
 async function resolveWorkspaceId(
   params: SetEnvironmentVariablesParams,
   context: ServerToolContext | undefined,
@@ -142,6 +209,8 @@ export const setEnvironmentVariablesServerTool: BaseServerTool<
     const added: string[] = []
     const updated: string[] = []
     let workspaceUpdated: string[] = []
+    let described: string[] = []
+    let descriptionFailures: string[] = []
 
     let resolvedWorkspaceId: string | undefined
     if (scope === 'workspace') {
@@ -151,20 +220,15 @@ export const setEnvironmentVariablesServerTool: BaseServerTool<
         validatedVariables,
         authenticatedUserId
       )
-      // Same writer the Set Secret use case calls, so the description lands with
-      // the semantics the secrets UI and API already define. It runs second
-      // because a brand-new key has no credential row to describe until the
-      // upsert above has minted one — and only for described keys, so a plain
-      // rotation stays a single write.
-      for (const [name, description] of Object.entries(descriptions)) {
-        await setWorkspaceSecret({
-          workspaceId: resolvedWorkspaceId,
-          name,
-          value: validatedVariables[name],
-          userId: authenticatedUserId,
-          description,
-        })
-      }
+      // Runs after the value write, which is what mints the credential row a
+      // brand-new key's description hangs on.
+      const outcome = await describeSecrets({
+        workspaceId: resolvedWorkspaceId,
+        userId: authenticatedUserId,
+        descriptions,
+      })
+      described = outcome.described
+      descriptionFailures = outcome.failures
     } else {
       const result = await upsertPersonalEnvVars(authenticatedUserId, validatedVariables)
       added.push(...result.added)
@@ -182,11 +246,20 @@ export const setEnvironmentVariablesServerTool: BaseServerTool<
       workspaceId: resolvedWorkspaceId,
     })
 
+    // A failed description never fails a stored value — but a describe-only call
+    // has nothing else to report, so its failure is the result.
+    if (descriptionFailures.length > 0 && workspaceUpdated.length === 0) {
+      throw new Error(`Could not describe: ${descriptionFailures.join('; ')}`)
+    }
+
     const parts: string[] = []
     if (added.length > 0) parts.push(`${added.length} personal secret(s) added`)
     if (updated.length > 0) parts.push(`${updated.length} personal secret(s) updated`)
     if (workspaceUpdated.length > 0)
       parts.push(`${workspaceUpdated.length} workspace secret(s) updated`)
+    if (described.length > 0) parts.push(`${described.length} description(s) saved`)
+    if (descriptionFailures.length > 0)
+      parts.push(`descriptions not saved (${descriptionFailures.join('; ')})`)
 
     return {
       message: `Successfully processed ${totalProcessed} secret(s): ${parts.join(', ')}`,
@@ -197,6 +270,7 @@ export const setEnvironmentVariablesServerTool: BaseServerTool<
       addedVariables: added,
       updatedVariables: updated,
       workspaceUpdatedVariables: workspaceUpdated,
+      describedVariables: described,
     }
   },
 }