fix(mothership): address review findings and CI regressions

Author: icecrasher321

.github/workflows/deploy-trigger-dev.yml

@@ -52,7 +52,7 @@ jobs:
           set -euo pipefail
           : "${TRIGGER_ACCESS_TOKEN:?TRIGGER_ACCESS_TOKEN must be configured}"
           : "${TRIGGER_PROJECT_ID:?TRIGGER_PROJECT_ID must be configured}"
-          bunx trigger.dev@4.5.12 deploy --env preview --branch dev-sim --skip-promotion
+          bunx trigger.dev@4.5.16 deploy --env preview --branch dev-sim --skip-promotion
 
       - name: Promote current dev preview
         working-directory: ./apps/sim
@@ -72,4 +72,4 @@ jobs:
             echo "::notice::Skipping superseded dev task version $VERSION"
             exit 0
           fi
-          bunx trigger.dev@4.5.12 promote "$VERSION" --env preview --branch dev-sim
+          bunx trigger.dev@4.5.16 promote "$VERSION" --env preview --branch dev-sim

apps/desktop/src/main/browser-agent/cdp.test.ts

@@ -311,7 +311,7 @@ describe('browser-agent CDP instrumentation', () => {
     }
   })
 
-  it.each(['complete', 'split', 'ambiguous'])(
+  it.each(['complete', 'split', 'worker', 'detached', 'ambiguous'])(
     'routes OOPIF evaluation through its session (%s tree)',
     async (treeKind) => {
       const contents = new WebContentsView().webContents
@@ -338,9 +338,20 @@ describe('browser-agent CDP instrumentation', () => {
         { autoAttach: true, waitForDebuggerOnStart: false, flatten: true },
         'child-session'
       )
+      if (treeKind === 'worker' || treeKind === 'detached') {
+        listener?.({}, 'Target.attachedToTarget', {
+          sessionId: 'unavailable-session',
+          targetInfo: {
+            targetId: 'unavailable',
+            type: treeKind === 'worker' ? 'worker' : 'iframe',
+          },
+        })
+      }
       vi.mocked(contents.debugger.sendCommand).mockClear()
       vi.mocked(contents.debugger.sendCommand).mockImplementation((method, _params, sessionId) => {
         if (method === 'Page.getFrameTree') {
+          if (sessionId === 'unavailable-session')
+            return Promise.reject(new Error('Target unavailable'))
           return Promise.resolve({
             frameTree:
               treeKind === 'complete'

apps/desktop/src/main/browser-agent/cdp.ts

@@ -308,7 +308,7 @@ export async function evaluateInIsolatedFrame(
   if (!protocolFrame) {
     /** Chromium omits out-of-process frames from the root target's tree. */
     const childSessions = [...(childSessionsByContents.get(contents)?.values() ?? [])]
-    const trees = await Promise.all(
+    const results = await Promise.allSettled(
       childSessions.map(async (sessionId) => {
         const result = await send<{ frameTree?: ProtocolFrameTree }>(
           contents,
@@ -319,6 +319,9 @@ export async function evaluateInIsolatedFrame(
         return result.frameTree
       })
     )
+    const trees = results.flatMap((result) =>
+      result.status === 'fulfilled' && result.value ? [result.value] : []
+    )
     const nodes = new Map<string, ProtocolFrameTree>()
     const visit = (tree: ProtocolFrameTree) => {
       nodes.set(tree.frame.id, tree)

apps/desktop/src/main/browser-agent/registry.ts

@@ -10,6 +10,7 @@ import type { Session, WebContents } from 'electron'
  * navigation time, so the post-construction registration races nothing.
  */
 const agentContents = new WeakSet<WebContents>()
+const agentSessions = new WeakSet<Session>()
 const appOrigins = new WeakMap<WebContents, string>()
 const navigations = new WeakMap<WebContents, (url: string, method: string) => boolean>()
 const permissions = new WeakMap<WebContents, BrowserPermissionHandlers>()
@@ -25,10 +26,16 @@ export function registerAgentWebContents(
   handlers?: BrowserPermissionHandlers
 ): void {
   agentContents.add(contents)
+  agentSessions.add(contents.session)
   if (appOrigin) appOrigins.set(contents, appOrigin)
   if (handlers) permissions.set(contents, handlers)
 }
 
+/** Worker requests can outlive their originating tab and omit webContents. */
+export function hasAgentSession(session: Session): boolean {
+  return agentSessions.has(session)
+}
+
 export function isAgentWebContents(contents: WebContents): boolean {
   return agentContents.has(contents)
 }

apps/desktop/src/main/telemetry-policy.test.ts

@@ -62,5 +62,13 @@ describe('attachTelemetryPolicy', () => {
     listener(request, callback)
     expect(requestPolicy.handleBrowserRequest).toHaveBeenCalledExactlyOnceWith(request, callback)
     expect(callback).not.toHaveBeenCalled()
+    requestPolicy.handleBrowserRequest.mockClear()
+    const workerRequest = { ...request, webContents: undefined, resourceType: 'other' as const }
+    listener(workerRequest, callback)
+    expect(requestPolicy.handleBrowserRequest).toHaveBeenCalledExactlyOnceWith(
+      workerRequest,
+      callback
+    )
+    expect(callback).not.toHaveBeenCalled()
   })
 })

apps/desktop/src/main/telemetry-policy.ts

@@ -1,6 +1,6 @@
 import { createLogger } from '@sim/logger'
 import type { Session } from 'electron'
-import { isAgentWebContents } from '@/main/browser-agent/registry'
+import { hasAgentSession, isAgentWebContents } from '@/main/browser-agent/registry'
 import { handleBrowserRequest } from '@/main/browser-agent/request-policy'
 import { matchesHostList } from '@/main/navigation'
 
@@ -40,7 +40,9 @@ export function attachTelemetryPolicy(session: Session, enabled: boolean): void
   session.webRequest.onBeforeRequest((details, callback) => {
     if (enabled && shouldBlockRequest(details.url)) {
       callback({ cancel: true })
-    } else if (details.webContents && isAgentWebContents(details.webContents)) {
+    } else if (
+      details.webContents ? isAgentWebContents(details.webContents) : hasAgentSession(session)
+    ) {
       handleBrowserRequest(details, callback)
     } else {
       callback({ cancel: false })

apps/sim/app/api/custom-blocks/[id]/route.ts

@@ -12,14 +12,14 @@ import {
   deleteCustomBlockSettings,
   updateCustomBlockSettings,
 } from '@/lib/workflows/custom-blocks/application/settings'
-import { customBlockError } from '@/app/api/custom-blocks/errors'
+import { customBlockResourceErrorPolicy } from '@/app/api/custom-blocks/errors'
 
 export const PATCH = defineInternalJsonRoute({
   contract: updateCustomBlockContract,
   auth: internalSessionAuth,
   operation: customBlockSettingsOperations.update,
   rateLimit: internalRateLimits.none({ reason: 'Preserve existing custom block update policy.' }),
-  errorPolicy: { project: customBlockError },
+  errorPolicy: customBlockResourceErrorPolicy,
   mapInput: ({ params, body }) => ({ id: params.id, patch: body }),
   useCase: updateCustomBlockSettings,
 })
@@ -29,7 +29,7 @@ export const DELETE = defineInternalJsonRoute({
   auth: internalSessionAuth,
   operation: customBlockSettingsOperations.delete,
   rateLimit: internalRateLimits.none({ reason: 'Preserve existing custom block deletion policy.' }),
-  errorPolicy: { project: customBlockError },
+  errorPolicy: customBlockResourceErrorPolicy,
   mapInput: ({ params }) => params,
   useCase: deleteCustomBlockSettings,
   present: () => ({ success: true as const }),

apps/sim/app/api/custom-blocks/[id]/usages/route.test.ts

@@ -17,7 +17,11 @@ const { mockHasWorkspaceAdminAccess, mockOperations } = vi.hoisted(() => ({
 vi.mock('@sim/platform-authz/workspace', async (importOriginal) => ({
   ...(await importOriginal<typeof import('@sim/platform-authz/workspace')>()),
   resolveEffectiveWorkspacePermission: async (...args: unknown[]) =>
-    (await mockHasWorkspaceAdminAccess(...args)) ? 'admin' : 'read',
+    (await mockHasWorkspaceAdminAccess(...args)) === null
+      ? null
+      : (await mockHasWorkspaceAdminAccess(...args))
+        ? 'admin'
+        : 'read',
 }))
 vi.mock('@/lib/workspaces/application/workspace-context', () => ({
   resolveActiveWorkspaceApplicationContext: async (workspaceId: string) => ({
@@ -68,6 +72,13 @@ describe('GET /api/custom-blocks/[id]/usages', () => {
     expect(response.status).toBe(404)
   })
 
+  it('conceals a block in an inaccessible workspace', async () => {
+    mockHasWorkspaceAdminAccess.mockResolvedValue(null)
+    const response = await callRoute()
+    expect(response.status).toBe(404)
+    expect(mockOperations.getCustomBlockUsageCounts).not.toHaveBeenCalled()
+  })
+
   it('returns 403 for a non-admin of the source workspace', async () => {
     mockHasWorkspaceAdminAccess.mockResolvedValue(false)
     const response = await callRoute()

apps/sim/app/api/custom-blocks/[id]/usages/route.ts

@@ -8,7 +8,7 @@ import {
   customBlockSettingsOperations,
   readCustomBlockUsages,
 } from '@/lib/workflows/custom-blocks/application/settings'
-import { customBlockError } from '@/app/api/custom-blocks/errors'
+import { customBlockResourceErrorPolicy } from '@/app/api/custom-blocks/errors'
 
 export const GET = defineInternalJsonRoute({
   contract: getCustomBlockUsageCountsContract,
@@ -17,7 +17,7 @@ export const GET = defineInternalJsonRoute({
   rateLimit: internalRateLimits.none({
     reason: 'Preserve existing custom block usage read policy.',
   }),
-  errorPolicy: { project: customBlockError },
+  errorPolicy: customBlockResourceErrorPolicy,
   mapInput: ({ params }) => params,
   useCase: readCustomBlockUsages,
 })

apps/sim/app/api/custom-blocks/errors.ts

@@ -1,4 +1,7 @@
-import { internalErrorResponse } from '@/lib/api/server/routes'
+import {
+  createInternalResourceConcealmentPolicy,
+  internalErrorResponse,
+} from '@/lib/api/server/routes'
 import { InternalUnauthenticatedError } from '@/lib/api/server/routes/internal-json-route'
 import {
   InsufficientWorkspacePermissionsError,
@@ -23,3 +26,8 @@ export function customBlockError(error: unknown, read = false) {
     return internalErrorResponse(statusForOrchestrationError(error.code), { error: error.message })
   return null
 }
+
+export const customBlockResourceErrorPolicy = createInternalResourceConcealmentPolicy({
+  base: { project: customBlockError },
+  notFoundMessage: 'Custom block not found',
+})