fix: pre-landing review fixes

Specialist review of the deploy-permission change found six issues:

- deployment-permission-matrix.test.ts was never staged. Mutation-proven: 12
  of the 13 role changes had no guard without it.
- performCreateWorkspaceApiKey's new 'forbidden' code was wired to 403 on the
  REST route but not on the copilot use case, so an authorization denial
  surfaced as an opaque 500. Confirmed independently by three reviewers.
- The version-param coercion fix widened the union's z.input to unknown under
  Zod 4, letting a client pass anything at compile time — the exact drift the
  fix existed to prevent. Replaced with an explicit transform so the input
  stays number | string | 'active'.
- Both chat authorization gates and the promote/undeploy tool routes had zero
  coverage of their required level; reverting them to admin passed the suite.
  Added assertions, each verified to fail under mutation.
- Narrowed authorizeDeploymentWorkflow's action union, which retained an
  'admin' arm with no callers.

TODOS.md records three follow-ups, the notable one being that the v1
deployment surface does not honor the registry's workspaceApiKey: 'deny' —
pre-existing, but this branch lowers the bar it sits behind.

Author: mzxchandra

TODOS.md

@@ -0,0 +1,46 @@
+# TODOS
+
+## API / Permissions
+
+### Align the v1 deployment surface with the operation registry
+**Priority:** P1
+
+`workflowOperations.deploy/undeploy/activateVersion/revertVersion` all declare
+`workspaceApiKey: 'deny'`, but `resolveV1DeploymentWorkflow`
+(`apps/sim/app/api/v1/workflows/utils.ts`) predates the registry and runs its own
+`validateWorkspaceAccess`, which resolves a workspace API key to its creator's
+permission. A workspace key therefore deploys through v1 regardless of the
+declared deny.
+
+Pre-existing, not introduced by the deploy-requires-write branch — but that
+branch lowered the bar from `admin` to `write`, so a key created by an admin who
+was later demoted to editor now keeps working where it previously stopped.
+
+Fixing it is a breaking change for anyone deploying via v1 with a workspace key,
+so it needs its own release note and deprecation window rather than riding along
+in a permissions PR.
+
+Noted in: `apps/sim/lib/core/application/deployment-permission-matrix.test.ts`
+(scope note on the `workspaceApiKey` describe block).
+
+### Route the deployment tool routes through the operation registry
+**Priority:** P3
+
+`apps/sim/app/api/tools/deployments/{deploy,promote,undeploy}/route.ts` call
+`authorizeDeploymentWorkflow(..., 'write')` with a hardcoded role literal instead
+of consuming `workflowOperations.deploy.minimumRole`. A future change to the
+central `minimumRole` leaves these three routes at a stale value. They also sit
+outside the operation's `principalKinds` policy.
+
+### Add `mship-tools:check` to CI
+**Priority:** P2
+
+`apps/sim/lib/copilot/generated/tool-catalog-v1.ts` is generated from the
+mothership contract and enforced at runtime by
+`apps/sim/lib/copilot/tool-executor/executor.ts`. Nothing in CI verifies it is in
+sync, so a regeneration from a stale sibling checkout can silently revert tool
+permissions. The sim-side catalog currently carries schema bounds that
+mothership's committed contract does not, so regeneration is not byte-stable in
+either direction until the mothership side lands.
+
+## Completed

apps/sim/app/api/chat/utils.permissions.test.ts

@@ -0,0 +1,69 @@
+/**
+ * @vitest-environment node
+ */
+
+import { workflowAuthzMockFns } from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { checkWorkflowAccessForChatCreation } from '@/app/api/chat/utils'
+
+/**
+ * Chat deployment dropped from `admin` to `write` alongside the rest of the
+ * deployment lifecycle. These helpers call `authorizeWorkflowByWorkspacePermission`
+ * directly rather than going through the operation registry, so the central
+ * permission-matrix test does not cover them — every consumer suite mocks
+ * `checkChatAccess` wholesale, which leaves the required level unobserved.
+ * These assert the level itself, so reverting it to `admin` fails here.
+ */
+describe('chat deployment permission level', () => {
+  const WORKFLOW = { id: 'wf-1', workspaceId: 'ws-1', name: 'Test Workflow' }
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('creating a chat deployment requires write, not admin', async () => {
+    workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
+      allowed: true,
+      status: 200,
+      workflow: WORKFLOW,
+      workspacePermission: 'write',
+    })
+
+    const result = await checkWorkflowAccessForChatCreation('wf-1', 'user-1')
+
+    expect(result.hasAccess).toBe(true)
+    expect(workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission).toHaveBeenCalledWith({
+      workflowId: 'wf-1',
+      userId: 'user-1',
+      action: 'write',
+    })
+  })
+
+  it('denies a member the authorizer rejects', async () => {
+    workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
+      allowed: false,
+      status: 403,
+      message: 'Insufficient permissions',
+      workflow: WORKFLOW,
+      workspacePermission: 'read',
+    })
+
+    const result = await checkWorkflowAccessForChatCreation('wf-1', 'user-1')
+
+    expect(result.hasAccess).toBe(false)
+    expect(result.workflow).toBeUndefined()
+  })
+
+  it('denies when the workflow does not resolve', async () => {
+    workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
+      allowed: false,
+      status: 404,
+      message: 'Workflow not found',
+      workflow: null,
+    })
+
+    const result = await checkWorkflowAccessForChatCreation('wf-1', 'user-1')
+
+    expect(result.hasAccess).toBe(false)
+  })
+})

apps/sim/app/api/tools/deployments/routes.test.ts

@@ -221,6 +221,28 @@ describe('POST /api/tools/deployments/undeploy', () => {
     expect(mockPerformFullUndeploy).not.toHaveBeenCalled()
   })
 
+  it('requires write permission on the workflow workspace', async () => {
+    workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
+      allowed: false,
+      status: 403,
+      message: 'Insufficient permissions',
+      workflow: WORKFLOW_RECORD,
+      workspacePermission: 'read',
+    })
+
+    const response = await undeployPost(
+      makePost('undeploy', { workflowId: WORKFLOW_ID, workspaceId: 'ws-1' })
+    )
+
+    expect(response.status).toBe(403)
+    expect(workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission).toHaveBeenCalledWith({
+      workflowId: WORKFLOW_ID,
+      userId: 'user-1',
+      action: 'write',
+    })
+    expect(mockPerformFullUndeploy).not.toHaveBeenCalled()
+  })
+
   it('undeploys a deployed workflow', async () => {
     const response = await undeployPost(
       makePost('undeploy', { workflowId: WORKFLOW_ID, workspaceId: 'ws-1' })
@@ -254,6 +276,28 @@ describe('POST /api/tools/deployments/promote', () => {
     })
   })
 
+  it('requires write permission on the workflow workspace', async () => {
+    workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
+      allowed: false,
+      status: 403,
+      message: 'Insufficient permissions',
+      workflow: WORKFLOW_RECORD,
+      workspacePermission: 'read',
+    })
+
+    const response = await promotePost(
+      makePost('promote', { workflowId: WORKFLOW_ID, workspaceId: 'ws-1', version: 3 })
+    )
+
+    expect(response.status).toBe(403)
+    expect(workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission).toHaveBeenCalledWith({
+      workflowId: WORKFLOW_ID,
+      userId: 'user-1',
+      action: 'write',
+    })
+    expect(mockPerformActivateVersion).not.toHaveBeenCalled()
+  })
+
   it('promotes the given version to live', async () => {
     const response = await promotePost(
       makePost('promote', { workflowId: WORKFLOW_ID, workspaceId: 'ws-1', version: 3 })

apps/sim/app/api/tools/deployments/utils.ts

@@ -53,7 +53,7 @@ export async function authorizeDeploymentWorkflow(
   userId: string,
   workflowId: string,
   workspaceId: string,
-  action: 'read' | 'write' | 'admin'
+  action: 'read' | 'write'
 ): Promise<
   { ok: true; workflow: AuthorizedDeploymentWorkflow } | { ok: false; response: NextResponse }
 > {

apps/sim/lib/api-key/application/create-api-key.ts

@@ -40,6 +40,12 @@ export const createCopilotWorkspaceApiKey = defineAuthorizedWorkspaceUseCase({
       if (result.errorCode === 'conflict') {
         throw new OrchestrationError('conflict', result.error ?? 'API key name already exists')
       }
+      if (result.errorCode === 'forbidden') {
+        throw new OrchestrationError(
+          'forbidden',
+          result.error ?? 'Admin permission is required to create a workspace API key'
+        )
+      }
       throw new Error('Failed to create workspace API key')
     }
     return { key: result.key, workspaceId: context.workspaceId }

apps/sim/lib/api/contracts/deployments.ts

@@ -31,10 +31,21 @@ export const deploymentVersionParamsSchema = z.object({
  * `z.number()` here makes every numeric version unreachable and leaves
  * `active` as the only satisfiable value. Coercion of `'active'` yields NaN,
  * fails `.int()`, and falls through to the literal branch, so ordering is safe.
+ *
+ * The numeric branch is `number | string` piped into the coercion rather than a
+ * bare `z.coerce.number()`: a coerced member widens the union's `z.input` to
+ * `unknown`, which would let a client pass anything at compile time and fail
+ * only at runtime — the exact drift this schema exists to prevent.
  */
 export const deploymentVersionOrActiveParamsSchema = z.object({
   id: z.string().min(1, 'Invalid workflow ID'),
-  version: z.union([z.coerce.number().int().positive(), z.literal('active')]),
+  version: z.union([
+    z
+      .union([z.number(), z.string()])
+      .transform((value) => (typeof value === 'number' ? value : Number(value)))
+      .pipe(z.number().int().positive()),
+    z.literal('active'),
+  ]),
 })
 
 export const deploymentVersionRouteParamsSchema = z.object({

apps/sim/lib/core/application/deployment-permission-matrix.test.ts

@@ -0,0 +1,82 @@
+/**
+ * @vitest-environment node
+ */
+import { describe, expect, it } from 'vitest'
+import { apiKeyOperations } from '@/lib/api-key/application/operations'
+import { mcpServerOperations } from '@/lib/mcp/application/operations'
+import { workflowOperations } from '@/lib/workflows/application/operations'
+
+/**
+ * The deployment permission matrix, asserted in one place.
+ *
+ * `minimumRole` is declarative data, so a single-character edit silently widens
+ * or narrows access with no failing test — the surrounding suites assert which
+ * operation a handler invoked, not what role that operation demands. This pins
+ * the boundary that matters: deploying a workflow needs `write`, while minting a
+ * workspace API key (a credential that can invoke every deployed workflow in the
+ * workspace) and installing a custom block (code that runs in every workflow)
+ * stay `admin`.
+ */
+describe('deployment permission matrix', () => {
+  describe('deployment lifecycle requires write', () => {
+    it.each([
+      ['deploy', workflowOperations.deploy],
+      ['undeploy', workflowOperations.undeploy],
+      ['deployChat', workflowOperations.deployChat],
+      ['undeployChat', workflowOperations.undeployChat],
+      ['updatePublicApi', workflowOperations.updatePublicApi],
+      ['activateVersion', workflowOperations.activateVersion],
+      ['revertVersion', workflowOperations.revertVersion],
+      ['updateVersion', workflowOperations.updateVersion],
+    ])('workflowOperations.%s', (_name, operation) => {
+      expect(operation.minimumRole).toBe('write')
+    })
+
+    it.each([
+      ['createWorkflowServer', mcpServerOperations.createWorkflowDeploymentServer],
+      ['updateWorkflowServer', mcpServerOperations.updateWorkflowDeploymentServer],
+      ['deleteWorkflowServer', mcpServerOperations.deleteWorkflowDeploymentServer],
+      ['deployTool', mcpServerOperations.deployWorkflowTool],
+      ['undeployTool', mcpServerOperations.undeployWorkflowTool],
+    ])('mcpServerOperations.%s', (_name, operation) => {
+      expect(operation.minimumRole).toBe('write')
+    })
+  })
+
+  describe('credential and code-injection surfaces stay admin', () => {
+    it('minting a workspace API key requires admin', () => {
+      expect(apiKeyOperations.createFromCopilot.minimumRole).toBe('admin')
+    })
+
+    it('workflow policy (lock) requires admin', () => {
+      expect(workflowOperations.updatePolicy.minimumRole).toBe('admin')
+    })
+  })
+
+  /**
+   * Scope note: this asserts the operation *declarations*, which govern every
+   * surface routed through the operation registry. The v1 REST API
+   * (`resolveV1DeploymentWorkflow`) predates the registry and runs its own
+   * `validateWorkspaceAccess`, resolving a workspace key to its creator — so a
+   * workspace key can still deploy there. That gap is pre-existing and tracked
+   * separately; do not read these assertions as covering v1.
+   */
+  describe('deployment operations reject workspace API keys', () => {
+    it.each([
+      ['deploy', workflowOperations.deploy],
+      ['undeploy', workflowOperations.undeploy],
+      ['updatePublicApi', workflowOperations.updatePublicApi],
+      ['activateVersion', workflowOperations.activateVersion],
+      ['revertVersion', workflowOperations.revertVersion],
+    ])(
+      'workflowOperations.%s denies workspace_api_key so a long-lived key cannot deploy',
+      (_name, operation) => {
+        expect(operation.workspaceApiKey).toBe('deny')
+      }
+    )
+
+    it('creating an API key from copilot denies workspace API keys', () => {
+      expect(apiKeyOperations.createFromCopilot.workspaceApiKey).toBe('deny')
+    })
+  })
+})