fix(tools): harden provider operation contracts

Author: icecrasher321

apps/sim/lib/internal/browser-use/operations/run-task.test.ts

@@ -85,6 +85,56 @@ describe('executeRunTaskOperation', () => {
     }
   })
 
+  it('uses the created profile session to fetch the live URL when task status omits it', async () => {
+    mockFetch
+      .mockResolvedValueOnce(jsonResponse({ id: 'profile-session' }))
+      .mockResolvedValueOnce(jsonResponse({ id: 'task-1' }))
+      .mockResolvedValueOnce(jsonResponse({ status: 'finished', output: 'done' }))
+      .mockResolvedValueOnce(
+        jsonResponse({
+          liveUrl: 'https://live.browser-use.com/profile-session',
+          publicShareUrl: 'https://browser-use.com/share/profile-session',
+        })
+      )
+      .mockResolvedValueOnce(new Response(null, { status: 204 }))
+
+    const result = await executeRunTaskOperation({
+      task: 'Open the page',
+      apiKey: 'api-key',
+      profile_id: 'profile-1',
+    })
+
+    expect(result.output).toMatchObject({
+      sessionId: 'profile-session',
+      liveUrl: 'https://live.browser-use.com/profile-session',
+      shareUrl: 'https://browser-use.com/share/profile-session',
+    })
+    expect(mockFetch).toHaveBeenNthCalledWith(
+      4,
+      'https://api.browser-use.com/api/v2/sessions/profile-session',
+      expect.objectContaining({ method: 'GET' })
+    )
+  })
+
+  it('returns an actionable error for a terminal failed task', async () => {
+    mockFetch
+      .mockResolvedValueOnce(jsonResponse({ id: 'task-1' }))
+      .mockResolvedValueOnce(
+        jsonResponse({ status: 'failed', output: 'Navigation could not reach the target' })
+      )
+
+    const result = await executeRunTaskOperation({ task: 'Open the page', apiKey: 'api-key' })
+
+    expect(result).toMatchObject({
+      success: false,
+      error: 'BrowserUse task failed: Navigation could not reach the target',
+      output: {
+        success: false,
+        output: 'Navigation could not reach the target',
+      },
+    })
+  })
+
   it('rejects a malformed successful create-task response', async () => {
     mockFetch.mockResolvedValueOnce(jsonResponse({ sessionId: 'session-1' }))
 
@@ -148,6 +198,7 @@ describe('executeRunTaskOperation', () => {
         method: 'PATCH',
         body: JSON.stringify({ action: 'stop' }),
         redirect: 'error',
+        signal: expect.any(AbortSignal),
       })
     )
   })
@@ -174,7 +225,12 @@ describe('executeRunTaskOperation', () => {
     expect(mockFetch).toHaveBeenNthCalledWith(
       3,
       'https://api.browser-use.com/api/v2/sessions/profile-session',
-      expect.objectContaining({ method: 'PATCH', redirect: 'error' })
+      expect.objectContaining({
+        method: 'PATCH',
+        redirect: 'error',
+        signal: expect.any(AbortSignal),
+      })
     )
+    expect(mockFetch.mock.calls[2]?.[1]?.signal).not.toBe(controller.signal)
   })
 })

apps/sim/lib/internal/browser-use/operations/run-task.ts

@@ -45,6 +45,7 @@ const taskStatusResponseSchema = z.object({
   output: z.unknown().optional(),
   steps: z.array(taskStepSchema).optional(),
 })
+const SESSION_CLEANUP_TIMEOUT_MS = 10_000
 
 const createTaskResponseSchema = z.object({
   id: z.string().min(1),
@@ -159,6 +160,7 @@ async function stopSession(sessionId: string, apiKey: string): Promise<void> {
     const response = await fetchBrowserUse(`/sessions/${encodeURIComponent(sessionId)}`, apiKey, {
       method: 'PATCH',
       body: { action: 'stop' },
+      signal: AbortSignal.timeout(SESSION_CLEANUP_TIMEOUT_MS),
     })
 
     if (response.ok) {
@@ -313,10 +315,11 @@ interface PollResult {
 async function pollForCompletion(
   taskId: string,
   apiKey: string,
+  initialSessionId: string | null,
   signal?: AbortSignal
 ): Promise<PollResult> {
   let consecutiveErrors = 0
-  let sessionId: string | null = null
+  let sessionId = initialSessionId
   let liveUrl: string | null = null
   let publicShareUrl: string | null = null
   const startTime = Date.now()
@@ -364,13 +367,20 @@ async function pollForCompletion(
     }
 
     if (['finished', 'failed', 'stopped'].includes(status)) {
+      const output = taskData.output ?? null
       return {
         success: status === 'finished',
-        output: taskData.output ?? null,
+        output,
         steps: taskData.steps ?? [],
         sessionId,
         liveUrl,
         publicShareUrl,
+        error:
+          status === 'finished'
+            ? undefined
+            : typeof output === 'string' && output.trim()
+              ? `BrowserUse task ${status}: ${output.trim()}`
+              : `BrowserUse task ${status}`,
       }
     }
 
@@ -379,13 +389,21 @@ async function pollForCompletion(
 
   const finalResult = await fetchTaskStatus(taskId, apiKey, signal)
   if (finalResult.ok && ['finished', 'failed', 'stopped'].includes(finalResult.data.status)) {
+    const status = finalResult.data.status
+    const output = finalResult.data.output ?? null
     return {
-      success: finalResult.data.status === 'finished',
-      output: finalResult.data.output ?? null,
+      success: status === 'finished',
+      output,
       steps: finalResult.data.steps ?? [],
       sessionId: finalResult.data.sessionId ?? sessionId,
       liveUrl,
       publicShareUrl,
+      error:
+        status === 'finished'
+          ? undefined
+          : typeof output === 'string' && output.trim()
+            ? `BrowserUse task ${status}: ${output.trim()}`
+            : `BrowserUse task ${status}`,
     }
   }
 
@@ -497,7 +515,7 @@ export const executeRunTaskOperation: InternalToolOperationImplementation<
     const initialSessionId = sessionId ?? data.sessionId ?? null
     logger.info(`Created BrowserUse task ${taskId}`, { sessionId: initialSessionId })
 
-    const result = await pollForCompletion(taskId, params.apiKey, signal)
+    const result = await pollForCompletion(taskId, params.apiKey, initialSessionId, signal)
 
     const finalSessionId = result.sessionId ?? initialSessionId
     const shareUrl =

apps/sim/lib/internal/github/operations.ts

@@ -29,14 +29,16 @@ interface ReviewCommentBody {
   event: 'COMMENT'
 }
 
-interface FileCommentBody {
+interface FileCommentBodyBase {
   body: string
   commit_id: string | undefined
   path: string | undefined
-  line: number | undefined
-  side: string
 }
 
+type FileCommentBody =
+  | (FileCommentBodyBase & { subject_type: 'file' })
+  | (FileCommentBodyBase & { line: number; side: string })
+
 interface GitHubCommentPayload {
   id?: number
   body?: string
@@ -112,10 +114,16 @@ function toLineNumber(value: unknown): number | undefined {
   if (typeof value === 'number') {
     parsed = value
   } else {
-    if (typeof value !== 'string' || !value.trim()) return undefined
+    if (value === undefined || value === null) return undefined
+    if (typeof value !== 'string') {
+      throw new Error('GitHub line must be a positive integer')
+    }
+    if (!value.trim()) return undefined
     parsed = Number(value.trim())
   }
-  if (!Number.isFinite(parsed)) return undefined
+  if (!Number.isFinite(parsed)) {
+    throw new Error(`GitHub line must be a valid number, but line was ${String(value)}`)
+  }
   if (!Number.isInteger(parsed)) {
     throw new Error(
       `GitHub line numbers are whole numbers, but line was ${parsed}. Set line to the integer line number in the diff.`
@@ -128,13 +136,15 @@ function fileCommentBody(
   params: CreateCommentParams,
   commitId: string | undefined
 ): FileCommentBody {
-  return {
+  const base = {
     body: params.body,
     commit_id: commitId,
     path: params.path,
-    line: toLineNumber(params.line),
-    side: params.side || 'RIGHT',
   }
+  const line = toLineNumber(params.line)
+  if (line === undefined) return { ...base, subject_type: 'file' }
+  if (line < 1) throw new Error('GitHub line numbers must be positive integers')
+  return { ...base, line, side: params.side || 'RIGHT' }
 }
 
 function commentEndpointUrl(params: CreateCommentParams): string {
@@ -169,6 +179,7 @@ function readNumber(record: Record<string, unknown>, key: string): number | unde
 
 function readCommentPayload(value: unknown): GitHubCommentPayload {
   if (!isRecordLike(value)) return {}
+  const submittedAt = readString(value, 'submitted_at')
   return {
     id: readNumber(value, 'id'),
     body: readString(value, 'body'),
@@ -179,8 +190,8 @@ function readCommentPayload(value: unknown): GitHubCommentPayload {
     position: readNumber(value, 'position'),
     side: readString(value, 'side'),
     commit_id: readString(value, 'commit_id'),
-    created_at: readString(value, 'created_at'),
-    updated_at: readString(value, 'updated_at'),
+    created_at: readString(value, 'created_at') ?? submittedAt,
+    updated_at: readString(value, 'updated_at') ?? submittedAt,
   }
 }
 

apps/sim/lib/internal/microsoft-ad/operations/add-user-app-role-assignment.test.ts

@@ -0,0 +1,48 @@
+/**
+ * @vitest-environment node
+ */
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { executeAddUserAppRoleAssignmentOperation } from '@/lib/internal/microsoft-ad/operations/add-user-app-role-assignment'
+
+const INPUT = {
+  accessToken: 'access-token',
+  userId: '11111111-1111-4111-8111-111111111111',
+  resourceId: '22222222-2222-4222-8222-222222222222',
+  appRoleId: '33333333-3333-4333-8333-333333333333',
+}
+
+describe('executeAddUserAppRoleAssignmentOperation', () => {
+  const fetchMock = vi.fn()
+
+  beforeEach(() => {
+    fetchMock.mockReset()
+    vi.stubGlobal('fetch', fetchMock)
+  })
+
+  afterEach(() => vi.unstubAllGlobals())
+
+  it('rejects malformed successful Graph JSON instead of fabricating a null assignment', async () => {
+    fetchMock.mockResolvedValueOnce(new Response('not-json'))
+
+    await expect(executeAddUserAppRoleAssignmentOperation(INPUT)).rejects.toThrow(
+      'Microsoft Graph returned malformed JSON for the app role assignment'
+    )
+    expect(fetchMock).toHaveBeenCalledTimes(1)
+  })
+
+  it('rejects a successful non-object assignment payload', async () => {
+    fetchMock.mockResolvedValueOnce(Response.json(null))
+
+    await expect(executeAddUserAppRoleAssignmentOperation(INPUT)).rejects.toThrow(
+      'Microsoft Graph returned an invalid app role assignment'
+    )
+  })
+
+  it('rejects a successful empty assignment payload', async () => {
+    fetchMock.mockResolvedValueOnce(Response.json({}))
+
+    await expect(executeAddUserAppRoleAssignmentOperation(INPUT)).rejects.toThrow(
+      'Microsoft Graph returned an invalid app role assignment'
+    )
+  })
+})

apps/sim/lib/internal/microsoft-ad/operations/add-user-app-role-assignment.ts

@@ -1,3 +1,4 @@
+import { isRecordLike } from '@sim/utils/object'
 import type { InternalToolOperationImplementation } from '@/lib/internal/tool-operations/types'
 import {
   mapAppRoleAssignment,
@@ -27,14 +28,23 @@ export const executeAddUserAppRoleAssignmentOperation: InternalToolOperationImpl
       signal,
     }
   )
-  const body = await response.json().catch(() => {
+  let body: unknown
+  try {
+    body = await response.json()
+  } catch {
     signal?.throwIfAborted()
-    return {}
-  })
+    if (response.ok) {
+      throw new Error('Microsoft Graph returned malformed JSON for the app role assignment')
+    }
+    body = {}
+  }
   signal?.throwIfAborted()
   if (!response.ok) {
     throw new Error(extractGraphErrorMessage(body, 'Failed to grant the app role to the user'))
   }
+  if (!isRecordLike(body) || typeof body.id !== 'string' || !body.id.trim()) {
+    throw new Error('Microsoft Graph returned an invalid app role assignment')
+  }
 
   return { success: true, output: { assignment: mapAppRoleAssignment(body) } }
 }

apps/sim/lib/internal/salesforce/operations/update-custom-field.test.ts

@@ -71,4 +71,16 @@ describe('salesforce update custom field operation', () => {
     )
     expect(fetchMock).toHaveBeenCalledTimes(1)
   })
+
+  it('rejects a field ID that could escape the CustomField path before provider work', async () => {
+    const fetchMock = vi.mocked(fetch)
+
+    await expect(
+      executeSalesforceUpdateCustomFieldOperation({
+        ...PARAMS,
+        fieldId: '../CustomObject',
+      } as never)
+    ).rejects.toThrow('Field ID must be a 15- or 18-character Salesforce record ID')
+    expect(fetchMock).not.toHaveBeenCalled()
+  })
 })

apps/sim/lib/internal/salesforce/operations/update-custom-field.ts

@@ -13,13 +13,17 @@ import {
 } from '@/tools/salesforce/utils'
 
 const logger = createLogger('SalesforceUpdateCustomField')
+const SALESFORCE_RECORD_ID_PATTERN = /^[A-Za-z0-9]{15}(?:[A-Za-z0-9]{3})?$/
 
 export const executeSalesforceUpdateCustomFieldOperation: InternalToolOperationImplementation<
   SalesforceUpdateCustomFieldParams
 > = async (params, signal): Promise<SalesforceUpdateCustomFieldResponse> => {
   const instanceUrl = getInstanceUrl(params.idToken, params.instanceUrl)
   const fieldId = requireId(params.fieldId, 'Field ID')
-  const url = `${instanceUrl}/services/data/v59.0/tooling/sobjects/CustomField/${fieldId}`
+  if (!SALESFORCE_RECORD_ID_PATTERN.test(fieldId)) {
+    throw new Error('Field ID must be a 15- or 18-character Salesforce record ID')
+  }
+  const url = `${instanceUrl}/services/data/v59.0/tooling/sobjects/CustomField/${encodeURIComponent(fieldId)}`
   const headers = {
     Authorization: `Bearer ${params.accessToken}`,
     'Content-Type': 'application/json',

apps/sim/lib/internal/supabase/operations/storage-update-bucket.test.ts

@@ -49,6 +49,36 @@ describe('executeStorageUpdateBucketOperation', () => {
     expect(payload.file_size_limit).toBe(4096)
   })
 
+  it('rejects a nonnumeric file limit before updating the bucket', async () => {
+    fetchMock.mockResolvedValueOnce(Response.json({ public: false, file_size_limit: 4096 }))
+
+    const result = await executeStorageUpdateBucketOperation({
+      ...INPUT,
+      fileSizeLimit: 'not-a-number' as never,
+    })
+
+    expect(result).toMatchObject({
+      success: false,
+      error: 'File size limit must be a finite number',
+    })
+    expect(fetchMock).toHaveBeenCalledTimes(1)
+  })
+
+  it.each([true, [], {}, '0x100'])('rejects a non-decimal file limit %j', async (fileSizeLimit) => {
+    fetchMock.mockResolvedValueOnce(Response.json({ public: false, file_size_limit: 4096 }))
+
+    const result = await executeStorageUpdateBucketOperation({
+      ...INPUT,
+      fileSizeLimit: fileSizeLimit as never,
+    })
+
+    expect(result).toMatchObject({
+      success: false,
+      error: 'File size limit must be a finite number',
+    })
+    expect(fetchMock).toHaveBeenCalledTimes(1)
+  })
+
   it('propagates cancellation instead of returning a failed tool envelope', async () => {
     const controller = new AbortController()
     fetchMock.mockImplementationOnce(async (_url, init) => {