fix(copilot): return lock denials and stop bootstrapping a legacy secret's ACL

Two defects in the previous commits, both found by review.

assertWorkflowMutable throws, and the three orchestration entry points awaited
it outside any try/catch, so a locked workflow escaped as an exception instead
of the { success: false } result the callers consume — performChatDeploy and
the copilot deploy tools would have surfaced a generic 500 rather than a lock
denial. performRevertToVersion already converted it; the three now do too,
through a shared helper.

upsertWorkspaceEnvVars derived newKeys for createWorkspaceEnvCredentials from
the credential rows rather than the stored variables. A secret written before
credential rows existed has no ACL, so overwriting it looked like adding a new
key: it minted a credential and made the caller that secret's admin. The
environment route derives newKeys from the locked jsonb read for exactly this
reason, and now so does this.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: TheodoreSpeaks

apps/sim/lib/environment/utils.test.ts

@@ -9,12 +9,18 @@ const {
   mockGetUserEntityPermissions,
   mockGetWorkspaceEnvKeyAdminAccess,
   mockRecordAudit,
+  mockTx,
 } = vi.hoisted(() => ({
   mockCreateWorkspaceEnvCredentials: vi.fn(),
   mockEncryptSecret: vi.fn(),
   mockGetUserEntityPermissions: vi.fn(),
   mockGetWorkspaceEnvKeyAdminAccess: vi.fn(),
   mockRecordAudit: vi.fn(),
+  mockTx: {
+    execute: vi.fn(),
+    select: vi.fn(),
+    insert: vi.fn(),
+  },
 }))
 
 // vitest.setup.ts mocks this module globally; this suite tests the real one.
@@ -35,6 +41,11 @@ vi.mock('@/lib/credentials/environment', () => ({
   getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess,
   syncPersonalEnvCredentialsForUser: vi.fn(),
 }))
+vi.mock('@sim/db', () => ({
+  db: {
+    transaction: vi.fn(async (fn: (tx: unknown) => unknown) => fn(mockTx)),
+  },
+}))
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
   checkWorkspaceAccess: vi.fn(),
   getUserEntityPermissions: mockGetUserEntityPermissions,
@@ -80,12 +91,23 @@ describe('upsertWorkspaceEnvVars', () => {
     expect(mockEncryptSecret).not.toHaveBeenCalled()
   })
 
+  function stubStoredVariables(variables: Record<string, string>) {
+    mockTx.execute.mockResolvedValue(undefined)
+    mockTx.select.mockReturnValue({
+      from: () => ({ where: () => ({ limit: async () => [{ variables }] }) }),
+    })
+    mockTx.insert.mockReturnValue({
+      values: () => ({ onConflictDoUpdate: async () => undefined }),
+    })
+  }
+
   it('allows a key admin to rotate the key they administer', async () => {
     mockGetUserEntityPermissions.mockResolvedValue('write')
     mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
       adminKeys: new Set(['STRIPE_KEY']),
       knownKeys: new Set(['STRIPE_KEY']),
     })
+    stubStoredVariables({ STRIPE_KEY: 'old-cipher' })
 
     await expect(
       upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1')
@@ -103,6 +125,7 @@ describe('upsertWorkspaceEnvVars', () => {
       adminKeys: new Set<string>(),
       knownKeys: new Set(['STRIPE_KEY']),
     })
+    stubStoredVariables({ STRIPE_KEY: 'old-cipher' })
 
     await expect(
       upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1')
@@ -115,4 +138,37 @@ describe('upsertWorkspaceEnvVars', () => {
     expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
     expect(mockRecordAudit).not.toHaveBeenCalled()
   })
+
+  it('does not mint a credential for a legacy secret already in the stored map', async () => {
+    // A secret written before credential rows existed has no ACL. Treating it as
+    // new would create one and make the caller its secret-admin — the route
+    // derives newKeys from the stored variables for exactly this reason.
+    mockGetUserEntityPermissions.mockResolvedValue('admin')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set<string>(),
+      knownKeys: new Set<string>(),
+    })
+    stubStoredVariables({ LEGACY_KEY: 'old-cipher' })
+
+    await upsertWorkspaceEnvVars('ws-1', { LEGACY_KEY: 'rotated' }, 'user-1')
+
+    expect(mockCreateWorkspaceEnvCredentials).toHaveBeenCalledWith(
+      expect.objectContaining({ newKeys: [] })
+    )
+  })
+
+  it('mints a credential for a genuinely new key', async () => {
+    mockGetUserEntityPermissions.mockResolvedValue('write')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set<string>(),
+      knownKeys: new Set<string>(),
+    })
+    stubStoredVariables({})
+
+    await upsertWorkspaceEnvVars('ws-1', { BRAND_NEW: 'value' }, 'user-1')
+
+    expect(mockCreateWorkspaceEnvCredentials).toHaveBeenCalledWith(
+      expect.objectContaining({ newKeys: ['BRAND_NEW'] })
+    )
+  })
 })

apps/sim/lib/environment/utils.ts

@@ -375,7 +375,7 @@ export async function upsertWorkspaceEnvVars(
 
   // Read-modify-write on a single jsonb column, so serialize against the
   // route's identically-locked transaction or concurrent writers lose keys.
-  await db.transaction(async (tx) => {
+  const existingEncrypted = await db.transaction(async (tx) => {
     await tx.execute(
       sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)`
     )
@@ -386,10 +386,8 @@ export async function upsertWorkspaceEnvVars(
       .from(workspaceEnvironment)
       .where(eq(workspaceEnvironment.workspaceId, workspaceId))
       .limit(1)
-    const merged = {
-      ...((existingRow?.variables as Record<string, string>) || {}),
-      ...newlyEncrypted,
-    }
+    const existing = (existingRow?.variables as Record<string, string>) || {}
+    const merged = { ...existing, ...newlyEncrypted }
 
     await tx
       .insert(workspaceEnvironment)
@@ -404,10 +402,15 @@ export async function upsertWorkspaceEnvVars(
         target: [workspaceEnvironment.workspaceId],
         set: { variables: merged, updatedAt: new Date() },
       })
+
+    return existing
   })
 
   invalidateEffectiveDecryptedEnvCache({ workspaceId })
-  const newKeys = updatedKeys.filter((key) => !knownKeys.has(key))
+  // Derived from the stored variables, not from the credential rows: a legacy
+  // secret present in the jsonb map without a credential row is NOT new, and
+  // minting an ACL for it would make the caller its secret-admin.
+  const newKeys = updatedKeys.filter((key) => !(key in existingEncrypted))
   await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId })
 
   recordAudit({

apps/sim/lib/workflows/orchestration/deploy.test.ts

@@ -7,6 +7,7 @@ import {
   queueTableRows,
   resetDbChainMock,
   schemaMock,
+  workflowAuthzMockFns,
 } from '@sim/testing'
 import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 
@@ -97,9 +98,12 @@ vi.mock('@/lib/workflows/schedules', () => ({
   validateWorkflowSchedules: mockValidateWorkflowSchedules,
 }))
 
+// Resolves to the global @sim/platform-authz/workflow mock, so instanceof matches.
+import { WorkflowLockedError } from '@sim/platform-authz/workflow'
 import {
   performActivateVersion,
   performFullDeploy,
+  performFullUndeploy,
   performRevertToVersion,
 } from '@/lib/workflows/orchestration/deploy'
 
@@ -660,3 +664,38 @@ describe('performActivateVersion workspace event emission', () => {
     expect(mockEmitWorkflowDeployedEvent).not.toHaveBeenCalled()
   })
 })
+
+describe('mutation lock on the orchestration entry points', () => {
+  const mockAssertMutable = workflowAuthzMockFns.mockAssertWorkflowMutable
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+    mockAssertMutable.mockRejectedValue(new WorkflowLockedError('Workflow is locked'))
+  })
+
+  it.each([
+    ['performFullDeploy', () => performFullDeploy({ workflowId: 'wf-1', userId: 'user-1' })],
+    ['performFullUndeploy', () => performFullUndeploy({ workflowId: 'wf-1', userId: 'user-1' })],
+    [
+      'performActivateVersion',
+      () => performActivateVersion({ workflowId: 'wf-1', version: 2, userId: 'user-1' }),
+    ],
+  ])('%s returns a lock denial instead of throwing', async (_name, call) => {
+    // Callers like performChatDeploy and the copilot deploy tools consume the
+    // result object; a throw surfaces as a generic 500 instead of a denial.
+    const result = await call()
+
+    expect(result.success).toBe(false)
+    expect(result.error).toContain('locked')
+    expect(mockRecordAudit).not.toHaveBeenCalled()
+  })
+
+  it('proceeds past the gate when the workflow is mutable', async () => {
+    mockAssertMutable.mockResolvedValue(undefined)
+
+    await performFullUndeploy({ workflowId: 'wf-1', userId: 'user-1' })
+
+    expect(mockAssertMutable).toHaveBeenCalledWith('wf-1')
+  })
+})

apps/sim/lib/workflows/orchestration/deploy.ts

@@ -96,6 +96,21 @@ export interface PerformFullDeployParams {
   actorId?: string
 }
 
+/**
+ * Resolves a mutation-lock denial to a message instead of throwing, so the entry
+ * points below return their `{ success: false }` result shape rather than
+ * surfacing a 500 to callers that expect one — matching `performRevertToVersion`.
+ */
+async function workflowLockDenial(workflowId: string): Promise<string | null> {
+  try {
+    await assertWorkflowMutable(workflowId)
+    return null
+  } catch (error) {
+    if (error instanceof WorkflowLockedError) return error.message
+    throw error
+  }
+}
+
 export interface PerformFullDeployResult {
   success: boolean
   deployedAt?: Date
@@ -121,7 +136,8 @@ export async function performFullDeploy(
 
   // Backstop for every caller — routes may assert first to render their own 423,
   // but the copilot deploy tools call this directly.
-  await assertWorkflowMutable(workflowId)
+  const lockDenial = await workflowLockDenial(workflowId)
+  if (lockDenial) return { success: false, error: lockDenial, errorCode: 'validation' }
 
   const [workflowRecord] = await db
     .select()
@@ -462,7 +478,8 @@ export async function performFullUndeploy(
   const actorId = params.actorId ?? userId
   const requestId = params.requestId ?? generateRequestId()
 
-  await assertWorkflowMutable(workflowId)
+  const lockDenial = await workflowLockDenial(workflowId)
+  if (lockDenial) return { success: false, error: lockDenial }
 
   const [workflowRecord] = await db
     .select()
@@ -574,7 +591,8 @@ export async function performActivateVersion(
   const actorId = params.actorId ?? userId
   const requestId = params.requestId ?? generateRequestId()
 
-  await assertWorkflowMutable(workflowId)
+  const lockDenial = await workflowLockDenial(workflowId)
+  if (lockDenial) return { success: false, error: lockDenial, errorCode: 'validation' }
 
   const [versionRow] = await db
     .select({