Merge branch 'pgx/t2' into feat/permission-groups-coverage

Author: waleedlatif1

.agents/skills/validate-permission-group-item/SKILL.md

@@ -48,8 +48,8 @@ Confirm the assertions at the bottom of `fields.ts` still name a field of this k
 ## Step 3: Admin UI (`ee/access-control/components/group-detail.tsx`)
 
 - **Boolean:** appears automatically via `PLATFORM_FEATURES`. Confirm its `category` is in `PLATFORM_CATEGORY_ORDER`; an unlisted one renders after every ordered section.
-- **Allowlist / denylist:** renders **nothing** unless it is in the `featureExtras` map — keyed by the *parent boolean's feature id*, not the config key. No picker and no bespoke section means no admin can ever set it. Report it.
-- For a picker, check both behaviors: refuses an empty selection (`if (values.length === 0) return`) and collapses a full one back to `null` (otherwise the allowlist freezes at today's members).
+- **Nested allowlist / denylist** (one that qualifies a platform-feature boolean): renders **nothing** unless it is in the `featureExtras` map — keyed by the *parent boolean's feature id*, not the config key. No picker there means no admin can ever set it. Report it. Top-level lists — `allowedIntegrations`, `allowedModelProviders`, `deniedModels`, `deniedTools` — are not in `featureExtras` and must not be reported for it; they render from the dedicated Providers and Blocks sections, so check them there.
+- For an **allowlist** picker, check both behaviors: refuses an empty selection (`if (values.length === 0) return`) and collapses a full one back to `null` (otherwise the allowlist freezes at today's members). A **denylist** picker must do neither: clearing every entry is how an admin denies nothing, and a full selection is a real state that denies everything.
 - Check the parent is the right one (`allowedKnowledgeConnectors` under `hide-knowledge-base`, not `disable-knowledge-base-creation`).
 
 ## Step 4: Capability rule

apps/sim/app/api/v1/middleware.test.ts

@@ -29,13 +29,15 @@ const {
   mockGetRateLimit,
   mockGetUserEntityPermissions,
   mockGetWorkspaceBillingSettings,
+  mockGetWorkspaceBilledAccountUserId,
 } = vi.hoisted(() => ({
   mockAuthenticateV1Request: vi.fn(),
   mockGetSubscription: vi.fn(),
   mockCheckRateLimit: vi.fn(),
   mockGetRateLimit: vi.fn(),
   mockGetUserEntityPermissions: vi.fn(),
   mockGetWorkspaceBillingSettings: vi.fn(),
+  mockGetWorkspaceBilledAccountUserId: vi.fn(),
 }))
 
 vi.mock('@/app/api/v1/auth', () => ({
@@ -61,7 +63,7 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
 
 vi.mock('@/lib/workspaces/utils', () => ({
   getWorkspaceBillingSettings: mockGetWorkspaceBillingSettings,
-  getWorkspaceBilledAccountUserId: vi.fn(async () => 'billed-user'),
+  getWorkspaceBilledAccountUserId: mockGetWorkspaceBilledAccountUserId,
 }))
 
 import { DEFAULT_PERMISSION_GROUP_CONFIG } from '@/lib/permission-groups/fields'
@@ -70,6 +72,7 @@ import {
   checkRateLimit,
   checkWorkspaceScope,
   createRateLimitResponse,
+  requireWorkspaceRequestActor,
   v1ValidationErrorResponse,
 } from '@/app/api/v1/middleware'
 
@@ -421,3 +424,47 @@ describe('checkWorkspaceScope', () => {
     expect(response?.status).toBe(403)
   })
 })
+
+describe('requireWorkspaceRequestActor', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockGetWorkspaceBilledAccountUserId.mockResolvedValue('billed-user')
+  })
+
+  it('substitutes the billed account as the system actor for a workspace key', async () => {
+    const actor = await requireWorkspaceRequestActor(
+      { allowed: true, keyType: 'workspace', userId: 'key-creator' } as never,
+      'workspace-1'
+    )
+
+    expect(actor).toEqual({ ok: true, actorUserId: 'billed-user' })
+  })
+
+  it('keeps the owner for a personal key', async () => {
+    const actor = await requireWorkspaceRequestActor(
+      { allowed: true, keyType: 'personal', userId: 'user-1' } as never,
+      'workspace-1'
+    )
+
+    expect(actor).toEqual({ ok: true, actorUserId: 'user-1' })
+  })
+
+  /**
+   * An archived or deleted workspace has no billed account to stand in. That is
+   * a reachable request about an unreachable workspace, not a server fault: the
+   * call sites used to throw, and the routes' catch-all reported it as a 500.
+   */
+  it('projects an unresolvable actor onto a 400 rather than throwing', async () => {
+    mockGetWorkspaceBilledAccountUserId.mockResolvedValue(null)
+
+    const actor = await requireWorkspaceRequestActor(
+      { allowed: true, keyType: 'workspace', userId: 'key-creator' } as never,
+      'workspace-gone'
+    )
+
+    expect(actor.ok).toBe(false)
+    if (actor.ok) throw new Error('expected a refusal')
+    expect(actor.response.status).toBe(400)
+    await expect(actor.response.json()).resolves.toEqual({ error: 'Invalid workspace ID' })
+  })
+})

apps/sim/app/api/v1/middleware.ts

@@ -532,6 +532,30 @@ export async function resolveWorkspaceRequestActor(
   return rateLimit.userId ?? null
 }
 
+/**
+ * {@link resolveWorkspaceRequestActor} as a route-ready result.
+ *
+ * The resolver answers `null` for a real, reachable request: an authenticated
+ * workspace key whose workspace has since been archived or deleted has no
+ * billed account to stand in as its system actor. Every call site used to
+ * `throw` on that, which the route's catch-all turned into a generic 500 — an
+ * unreachable workspace reported as a server fault. It is the same condition
+ * the routes already report as a 400 `Invalid workspace ID` when the addressed
+ * table belongs to another workspace, so it is reported the same way, from one
+ * place, rather than five copies of a throw.
+ */
+export async function requireWorkspaceRequestActor(
+  rateLimit: RateLimitResult,
+  workspaceId: string
+): Promise<{ ok: true; actorUserId: string } | { ok: false; response: NextResponse }> {
+  const actorUserId = await resolveWorkspaceRequestActor(rateLimit, workspaceId)
+  if (actorUserId) return { ok: true, actorUserId }
+  return {
+    ok: false,
+    response: NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }),
+  }
+}
+
 /**
  * v1 wrapper: renders {@link resolveWorkspaceAccess} as the v1 `{ error }` body.
  * Returns null on success, NextResponse on failure.

apps/sim/app/api/v1/tables/[tableId]/route.test.ts

@@ -46,9 +46,20 @@ vi.mock('@/app/api/v1/middleware', () => ({
   /**
    * Mirrors the real resolver: a workspace key names no human, so the billed
    * account stands in as the explicit system actor; anything else keeps its
-   * owner.
+   * owner. The route reads it through `requireWorkspaceRequestActor`, which
+   * projects an unresolvable actor onto a 400 instead of throwing, so the mock
+   * reproduces that projection rather than only the raw resolver.
    */
   resolveWorkspaceRequestActor: mockResolveWorkspaceRequestActor,
+  requireWorkspaceRequestActor: async (rateLimit: unknown, workspaceId: string) => {
+    const actorUserId = await mockResolveWorkspaceRequestActor(rateLimit, workspaceId)
+    return actorUserId
+      ? { ok: true, actorUserId }
+      : {
+          ok: false,
+          response: NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }),
+        }
+  },
 }))
 
 vi.mock('@/lib/table', () => ({
@@ -109,6 +120,23 @@ describe('DELETE /api/v1/tables/[tableId] — orchestration failure projection',
     mockGetUserEntityPermissions.mockResolvedValue('admin')
   })
 
+  /**
+   * A workspace key whose workspace has since been archived resolves no billed
+   * account, so there is no system actor to attribute the deletion to. That is
+   * a reachable request about an unreachable workspace, not a server fault: it
+   * used to `throw`, and the catch-all reported it as a 500.
+   */
+  it('reports an unresolvable workspace actor as a 400, not a 500', async () => {
+    mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'user-1', keyType: 'workspace' })
+    mockResolveWorkspaceRequestActor.mockResolvedValue(null)
+
+    const response = await DELETE(makeRequest(), makeContext())
+
+    expect(response.status).toBe(400)
+    expect(await response.json()).toEqual({ error: 'Invalid workspace ID' })
+    expect(mockPerformDeleteTable).not.toHaveBeenCalled()
+  })
+
   it('renders an unclassified internal failure as a fixed generic message', async () => {
     mockPerformDeleteTable.mockResolvedValue({
       success: false,

apps/sim/app/api/v1/tables/[tableId]/route.ts

@@ -17,7 +17,7 @@ import {
   checkRateLimit,
   checkWorkspaceScope,
   createRateLimitResponse,
-  resolveWorkspaceRequestActor,
+  requireWorkspaceRequestActor,
   tableAccessPrincipal,
 } from '@/app/api/v1/middleware'
 
@@ -137,12 +137,14 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
      * A workspace key names no human, so its creator must not be attributed the
      * deletion in audit and analytics. The shared resolver substitutes the
      * explicit system actor for a workspace key and keeps the owner for a
-     * personal one, exactly as the row routes on this table already do.
+     * personal one, exactly as the row routes on this table already do. An
+     * archived or deleted workspace has no billed account to stand in, which is
+     * a controlled 400 rather than an uncaught throw the catch-all would report
+     * as a 500.
      */
-    const actorUserId = await resolveWorkspaceRequestActor(rateLimit, workspaceId)
-    if (!actorUserId) {
-      throw new Error(`Unable to resolve system actor for workspace ${workspaceId}`)
-    }
+    const actor = await requireWorkspaceRequestActor(rateLimit, workspaceId)
+    if (!actor.ok) return actor.response
+    const actorUserId = actor.actorUserId
 
     const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
     if (!result.ok) return accessError(result, requestId, tableId)

apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts

@@ -30,7 +30,7 @@ import {
   checkRateLimit,
   checkWorkspaceScope,
   createRateLimitResponse,
-  resolveWorkspaceRequestActor,
+  requireWorkspaceRequestActor,
   tableAccessPrincipal,
   v1ValidationErrorResponse,
   v1ValidationErrorResponseFromError,
@@ -135,10 +135,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: RowR
 
     const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
     if (scopeError) return scopeError
-    const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
-    if (!actorUserId) {
-      throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
-    }
+    const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
+    if (!actor.ok) return actor.response
+    const actorUserId = actor.actorUserId
 
     const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
     if (!result.ok) return accessError(result, requestId, tableId)

apps/sim/app/api/v1/tables/[tableId]/rows/route.ts

@@ -44,7 +44,7 @@ import {
   checkRateLimit,
   checkWorkspaceScope,
   createRateLimitResponse,
-  resolveWorkspaceRequestActor,
+  requireWorkspaceRequestActor,
   tableAccessPrincipal,
   v1ValidationErrorResponse,
   v1ValidationErrorResponseFromError,
@@ -243,15 +243,9 @@ export const POST = withRouteHandler(
         const batchValidated = parsed.data.body
         const scopeError = await checkWorkspaceScope(rateLimit, batchValidated.workspaceId, 'write')
         if (scopeError) return scopeError
-        const actorUserId = await resolveWorkspaceRequestActor(
-          rateLimit,
-          batchValidated.workspaceId
-        )
-        if (!actorUserId) {
-          throw new Error(
-            `Unable to resolve system actor for workspace ${batchValidated.workspaceId}`
-          )
-        }
+        const batchActor = await requireWorkspaceRequestActor(rateLimit, batchValidated.workspaceId)
+        if (!batchActor.ok) return batchActor.response
+        const actorUserId = batchActor.actorUserId
         return handleBatchInsert(
           requestId,
           tableId,
@@ -266,10 +260,9 @@ export const POST = withRouteHandler(
 
       const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
       if (scopeError) return scopeError
-      const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
-      if (!actorUserId) {
-        throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
-      }
+      const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
+      if (!actor.ok) return actor.response
+      const actorUserId = actor.actorUserId
 
       const accessResult = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
       if (!accessResult.ok) return accessError(accessResult, requestId, tableId)
@@ -350,10 +343,9 @@ export const PUT = withRouteHandler(async (request: NextRequest, context: TableR
 
     const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
     if (scopeError) return scopeError
-    const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
-    if (!actorUserId) {
-      throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
-    }
+    const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
+    if (!actor.ok) return actor.response
+    const actorUserId = actor.actorUserId
 
     const accessResult = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
     if (!accessResult.ok) return accessError(accessResult, requestId, tableId)

apps/sim/app/api/v1/tables/[tableId]/rows/upsert/route.ts

@@ -21,7 +21,7 @@ import {
   checkRateLimit,
   checkWorkspaceScope,
   createRateLimitResponse,
-  resolveWorkspaceRequestActor,
+  requireWorkspaceRequestActor,
   tableAccessPrincipal,
   v1ValidationErrorResponse,
   v1ValidationErrorResponseFromError,
@@ -55,10 +55,9 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Upser
 
     const scopeError = await checkWorkspaceScope(rateLimit, validated.workspaceId, 'write')
     if (scopeError) return scopeError
-    const actorUserId = await resolveWorkspaceRequestActor(rateLimit, validated.workspaceId)
-    if (!actorUserId) {
-      throw new Error(`Unable to resolve system actor for workspace ${validated.workspaceId}`)
-    }
+    const actor = await requireWorkspaceRequestActor(rateLimit, validated.workspaceId)
+    if (!actor.ok) return actor.response
+    const actorUserId = actor.actorUserId
 
     const result = await checkAccess(tableId, tableAccessPrincipal(rateLimit), 'write')
     if (!result.ok) return accessError(result, requestId, tableId)