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

Author: waleedlatif1

apps/sim/ee/access-control/components/group-detail.tsx

@@ -30,7 +30,7 @@ import { formatDate } from '@sim/utils/formatting'
 import { useQueryState } from 'nuqs'
 import { saveDiscardActions } from '@/components/settings/save-discard-actions'
 import type { ShareAuthType } from '@/lib/api/contracts/public-shares'
-import { isBlockTypeAccessControlExempt } from '@/lib/permission-groups/block-access'
+import { isAccessControlAllowlistRow } from '@/lib/permission-groups/block-access'
 import { PLATFORM_CATEGORY_ORDER, PLATFORM_FEATURES } from '@/lib/permission-groups/features'
 import type { PermissionGroupConfig } from '@/lib/permission-groups/fields'
 import { UnsavedChangesModal } from '@/app/workspace/[workspaceId]/components/credential-detail'
@@ -65,6 +65,11 @@ import {
   useRemovePermissionGroupMember,
   useUpdatePermissionGroup,
 } from '@/ee/access-control/hooks/permission-groups'
+import {
+  allowlistRowsFromStored,
+  toggleAllowlistRow,
+  withAllowlistRows,
+} from '@/ee/access-control/utils/integration-allowlist-rows'
 import { SettingRow } from '@/ee/components/setting-row'
 import { useBlacklistedProviders } from '@/hooks/queries/allowed-providers'
 import { useOrganizationRoster } from '@/hooks/queries/organization'
@@ -789,9 +794,15 @@ export function GroupDetail({
    * otherwise a null→partial transition by a non-revealed admin would silently
    * drop a preview block from the stored allowlist and deny it to revealed
    * users already running it.
+   *
+   * EXCLUDES superseded blocks. They are hidden and so are never rendered, but
+   * they used to be materialized into the allowlist all the same — so an admin
+   * narrowing a previously-unrestricted allowlist by unchecking `slack_v2` wrote
+   * `slack` into it, which the runtime resolves back to `slack_v2` and allows.
+   * A decision about a retired version is made on its successor's row.
    */
   const allBlocks = useMemo(() => {
-    const blocks = getAllBlocks().filter((b) => !isBlockTypeAccessControlExempt(b.type))
+    const blocks = getAllBlocks().filter((b) => isAccessControlAllowlistRow(b.type))
     return blocks.sort((a, b) => {
       const catA = BLOCK_CATEGORY_ORDER[a.category] ?? 3
       const catB = BLOCK_CATEGORY_ORDER[b.category] ?? 3
@@ -881,17 +892,16 @@ export function GroupDetail({
 
   const guard = useSettingsUnsavedGuard({ isDirty: hasChanges })
 
+  const allBlockTypes = useMemo(() => allBlocks.map((b) => b.type), [allBlocks])
+
   /**
    * `null` means "everything allowed". Indexing the allow-lists once keeps the
    * per-row membership checks O(1) — they run for every one of the ~200 block
    * rows on each render, and again in the section-wide `every(...)` scans.
    */
   const allowedIntegrationSet = useMemo(
-    () =>
-      editingConfig.allowedIntegrations === null
-        ? null
-        : new Set(editingConfig.allowedIntegrations),
-    [editingConfig.allowedIntegrations]
+    () => allowlistRowsFromStored(allBlockTypes, editingConfig.allowedIntegrations),
+    [allBlockTypes, editingConfig.allowedIntegrations]
   )
 
   const allowedProviderSet = useMemo(
@@ -994,48 +1004,35 @@ export function GroupDetail({
   const toggleIntegration = useCallback(
     (blockType: string) => {
       setEditingConfig((prev) => {
-        const current = prev.allowedIntegrations
-        let nextAllowed: string[] | null
-        if (current === null) {
-          nextAllowed = allBlocks.map((b) => b.type).filter((t) => t !== blockType)
-        } else if (current.includes(blockType)) {
-          const updated = current.filter((t) => t !== blockType)
-          nextAllowed = updated.length === allBlocks.length ? null : updated
-        } else {
-          const updated = [...current, blockType]
-          nextAllowed = updated.length === allBlocks.length ? null : updated
-        }
+        const nextAllowed = toggleAllowlistRow(allBlockTypes, prev.allowedIntegrations, blockType)
         return {
           ...prev,
           allowedIntegrations: nextAllowed,
           deniedTools: pruneDeniedTools(nextAllowed, prev.deniedTools),
         }
       })
     },
-    [allBlocks, pruneDeniedTools]
+    [allBlockTypes, pruneDeniedTools]
   )
 
   /** Allow or deny a whole section's blocks at once, respecting the active filter. */
   const setBlocksAllowed = useCallback(
     (blocks: BlockConfig[], allowed: boolean) => {
       setEditingConfig((prev) => {
-        const allTypes = allBlocks.map((b) => b.type)
-        const current =
-          prev.allowedIntegrations === null ? new Set(allTypes) : new Set(prev.allowedIntegrations)
-        for (const block of blocks) {
-          if (allowed) current.add(block.type)
-          else current.delete(block.type)
-        }
-        const nextArr = allTypes.filter((t) => current.has(t))
-        const nextAllowed = nextArr.length === allTypes.length ? null : nextArr
+        const nextAllowed = withAllowlistRows(
+          allBlockTypes,
+          prev.allowedIntegrations,
+          blocks.map((block) => block.type),
+          allowed
+        )
         return {
           ...prev,
           allowedIntegrations: nextAllowed,
           deniedTools: pruneDeniedTools(nextAllowed, prev.deniedTools),
         }
       })
     },
-    [allBlocks, pruneDeniedTools]
+    [allBlockTypes, pruneDeniedTools]
   )
 
   const isToolAllowed = useCallback(

apps/sim/ee/access-control/utils/integration-allowlist-rows.test.ts

@@ -0,0 +1,87 @@
+/**
+ * @vitest-environment node
+ *
+ * The universe below is the editor's row set, which excludes superseded blocks
+ * (`isAccessControlAllowlistRow`). Every id is a real one, so the assertions
+ * rest on the repository's own lifecycle facts: `slack` was replaced by
+ * `slack_v2`.
+ */
+import { describe, expect, it } from 'vitest'
+import {
+  allowlistRowsFromStored,
+  toggleAllowlistRow,
+  withAllowlistRows,
+} from '@/ee/access-control/utils/integration-allowlist-rows'
+
+const UNIVERSE = ['agent', 'notion_v2', 'slack_v2'] as const
+
+describe('allowlistRowsFromStored', () => {
+  it('keeps an unrestricted allowlist unrestricted', () => {
+    expect(allowlistRowsFromStored(UNIVERSE, null)).toBeNull()
+  })
+
+  /**
+   * The runtime resolves a stored `slack` to `slack_v2` and allows it, so the
+   * row has to render checked or the editor is lying about what is permitted.
+   */
+  it('reads a stored retired id as the row it actually governs', () => {
+    const rows = allowlistRowsFromStored(UNIVERSE, ['slack'])
+
+    expect(rows?.has('slack_v2')).toBe(true)
+    expect(rows?.has('slack')).toBe(false)
+  })
+
+  it('drops an id no row corresponds to', () => {
+    expect([...(allowlistRowsFromStored(UNIVERSE, ['agent', 'retired_thing']) ?? [])]).toEqual([
+      'agent',
+    ])
+  })
+})
+
+describe('toggleAllowlistRow', () => {
+  /**
+   * The bug this closes. The editor renders only current blocks, so narrowing a
+   * previously-unrestricted allowlist used to materialize the hidden `slack`
+   * alongside the rows — and the runtime resolves `slack` back to `slack_v2`,
+   * re-allowing the integration the admin had just denied.
+   */
+  it('does not leave a superseded id behind when a row is denied', () => {
+    const next = toggleAllowlistRow(UNIVERSE, null, 'slack_v2')
+
+    expect(next).toEqual(['agent', 'notion_v2'])
+    expect(allowlistRowsFromStored(UNIVERSE, next)?.has('slack_v2')).toBe(false)
+  })
+
+  /** A stored retired id must follow its successor's fate, not outlive it. */
+  it('denies a row a stored retired id was granting', () => {
+    const next = toggleAllowlistRow(UNIVERSE, ['agent', 'slack'], 'slack_v2')
+
+    expect(next).toEqual(['agent'])
+  })
+
+  it('grants a row that was not allowed', () => {
+    expect(toggleAllowlistRow(UNIVERSE, ['agent'], 'notion_v2')).toEqual(['agent', 'notion_v2'])
+  })
+
+  /** Permitting everything is stored as "no restriction", not as a frozen list. */
+  it('collapses back to unrestricted when the last row is granted', () => {
+    expect(toggleAllowlistRow(UNIVERSE, ['agent', 'notion_v2'], 'slack_v2')).toBeNull()
+  })
+})
+
+describe('withAllowlistRows', () => {
+  it('denies a whole section at once', () => {
+    expect(withAllowlistRows(UNIVERSE, null, ['notion_v2', 'slack_v2'], false)).toEqual(['agent'])
+  })
+
+  it('emits rows in universe order however the section is ordered', () => {
+    expect(withAllowlistRows(UNIVERSE, [], ['slack_v2', 'agent'], true)).toEqual([
+      'agent',
+      'slack_v2',
+    ])
+  })
+
+  it('collapses to unrestricted when a section grant covers every row', () => {
+    expect(withAllowlistRows(UNIVERSE, ['agent'], [...UNIVERSE], true)).toBeNull()
+  })
+})

apps/sim/ee/access-control/utils/integration-allowlist-rows.ts

@@ -0,0 +1,54 @@
+import { toAccessControlAllowlist } from '@/lib/permission-groups/block-access'
+
+/**
+ * The stored `allowedIntegrations` re-expressed as editor rows: successor-
+ * resolved the way the runtime resolves it, then projected onto the universe of
+ * rows the editor actually offers. `null` stays `null` — everything allowed.
+ *
+ * A stored list can name a retired id (written before the universe excluded
+ * superseded blocks, or through the API directly). The runtime resolves it to
+ * its successor, so a stored `slack` allows `slack_v2`; reading the raw strings
+ * would render that row unchecked, and toggling it would "enable" an
+ * integration that was already permitted. Resolving first makes the checkbox
+ * tell the truth, and the projection drops the stale id on the next write.
+ */
+export function allowlistRowsFromStored(
+  universe: readonly string[],
+  stored: readonly string[] | null
+): Set<string> | null {
+  const resolved = toAccessControlAllowlist(stored)
+  return resolved === null ? null : new Set(universe.filter((type) => resolved.has(type)))
+}
+
+/**
+ * The stored allowlist after a set of rows is allowed or denied.
+ *
+ * Always emitted in universe order and collapsed back to `null` — unrestricted
+ * — when every row survives, so a group that ends up permitting everything is
+ * stored as "no restriction" rather than as a list that silently freezes out
+ * every integration added later.
+ */
+export function withAllowlistRows(
+  universe: readonly string[],
+  stored: readonly string[] | null,
+  blockTypes: readonly string[],
+  allowed: boolean
+): string[] | null {
+  const rows = allowlistRowsFromStored(universe, stored) ?? new Set(universe)
+  for (const blockType of blockTypes) {
+    if (allowed) rows.add(blockType)
+    else rows.delete(blockType)
+  }
+  const next = universe.filter((type) => rows.has(type))
+  return next.length === universe.length ? null : next
+}
+
+/** {@link withAllowlistRows} for one row, flipping whatever it is now. */
+export function toggleAllowlistRow(
+  universe: readonly string[],
+  stored: readonly string[] | null,
+  blockType: string
+): string[] | null {
+  const rows = allowlistRowsFromStored(universe, stored)
+  return withAllowlistRows(universe, stored, [blockType], !(rows === null || rows.has(blockType)))
+}

apps/sim/lib/copilot/chat/post.test.ts

@@ -61,6 +61,41 @@ const {
   releaseChatSendClaim: vi.fn(),
 }))
 
+/**
+ * The root span, captured so a test can assert what a refused turn exported.
+ * `withCopilotSpan` is a pass-through here — the nesting it provides is not
+ * under test and a real tracer would need an exporter to observe.
+ */
+const { setInputMessages, setUserMessagePreview, startCopilotOtelRoot } = vi.hoisted(() => ({
+  setInputMessages: vi.fn(),
+  setUserMessagePreview: vi.fn(),
+  startCopilotOtelRoot: vi.fn(),
+}))
+
+vi.mock('@/lib/copilot/request/otel', async () => {
+  const { ROOT_CONTEXT, trace } = await import('@opentelemetry/api')
+  const span = () => trace.getTracer('post-test').startSpan('post-test')
+  startCopilotOtelRoot.mockImplementation(() => ({
+    span: span(),
+    context: ROOT_CONTEXT,
+    requestId: 'req-1',
+    finish: vi.fn(),
+    setUserMessagePreview,
+    setInputMessages,
+    setOutputMessages: vi.fn(),
+    setRequestShape: vi.fn(),
+  }))
+  return {
+    startCopilotOtelRoot,
+    withCopilotSpan: (
+      _name: string,
+      _attrs: Record<string, unknown> | undefined,
+      fn: (child: ReturnType<typeof span>) => unknown,
+      _context?: unknown
+    ) => fn(span()),
+  }
+})
+
 const resolvePermissionGroupConfig = permissionGroupScopeMockFns.mockResolvePermissionGroupConfig
 
 const getSession = authMockFns.mockGetSession
@@ -1068,6 +1103,38 @@ describe('handleUnifiedChatPost copilot.use capability gate', () => {
     expect(createSSEStream).toHaveBeenCalledTimes(1)
   })
 
+  /**
+   * Prompt content is exported only once the turn is going to run. GenAI
+   * message capture is gated on whether capture is enabled at all, not on
+   * whether this caller may send, so capturing at span start exported the
+   * message of every turn the gate then refused.
+   */
+  it('exports no part of the prompt when the send is refused', async () => {
+    resolvePermissionGroupConfig.mockResolvedValue({
+      ...DEFAULT_PERMISSION_GROUP_CONFIG,
+      hideCopilot: true,
+    })
+
+    const response = await handleUnifiedChatPost(chatRequest({ createNewChat: true }))
+
+    expect(response.status).toBe(403)
+    expect(setInputMessages).not.toHaveBeenCalled()
+    expect(setUserMessagePreview).not.toHaveBeenCalled()
+    expect(startCopilotOtelRoot).toHaveBeenCalledWith(
+      expect.not.objectContaining({ userMessagePreview: expect.anything() })
+    )
+  })
+
+  it('captures the prompt once the send is allowed to run', async () => {
+    resolvePermissionGroupConfig.mockResolvedValue(DEFAULT_PERMISSION_GROUP_CONFIG)
+
+    const response = await handleUnifiedChatPost(chatRequest({ createNewChat: true }))
+
+    expect(response.status).toBe(200)
+    expect(setUserMessagePreview).toHaveBeenCalledWith('Hello')
+    expect(setInputMessages).toHaveBeenCalledWith({ userMessage: 'Hello' })
+  })
+
   /** A branch that lands in no workspace at all is governed by no group. */
   it('does not consult a permission group when the branch resolves no workspace', async () => {
     resolveWorkflowIdForUser.mockResolvedValue({

apps/sim/lib/copilot/chat/post.ts

@@ -1073,7 +1073,6 @@ export async function handleUnifiedChatPost(req: NextRequest) {
       executionId,
       runId,
       transport: CopilotTransport.Stream,
-      userMessagePreview: body.message,
     })
     if (otelRoot.requestId) {
       requestId = otelRoot.requestId
@@ -1088,10 +1087,6 @@ export async function handleUnifiedChatPost(req: NextRequest) {
     if (authenticatedUserEmail) {
       otelRoot.span.setAttribute(TraceAttr.UserEmail, authenticatedUserEmail)
     }
-    // `setInputMessages` is internally gated on
-    // OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT; safe to call.
-    otelRoot.setInputMessages({ userMessage: body.message })
-
     // Wrap the rest of the handler so nested spans attach to the
     // root via AsyncLocalStorage (otherwise they orphan into new traces).
     const activeOtelRoot = otelRoot
@@ -1160,6 +1155,17 @@ export async function handleUnifiedChatPost(req: NextRequest) {
         return capabilityRefusalResponse(chatCapability)
       }
 
+      /* Prompt content is captured only once the turn is going to run. Both
+         calls are internally gated on
+         OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT, but the gate is on
+         whether capture is enabled at all, not on whether this caller may send
+         — so stamping them at span start exported the message of every turn the
+         capability check above then refused. Every refusal ahead of this point
+         (a rejected branch, a withheld `copilot.use`) now records the shape of
+         the request and none of its content. */
+      activeOtelRoot.setUserMessagePreview(body.message)
+      activeOtelRoot.setInputMessages({ userMessage: body.message })
+
       let currentChat: ChatLoadResult['chat'] = null
       let conversationHistory: unknown[] = []
       let chatIsNew = false