diff --git a/apps/sim/lib/webhooks/providers/ashby.test.ts b/apps/sim/lib/webhooks/providers/ashby.test.ts index f51eb1f8d9a..9c6a81aadf0 100644 --- a/apps/sim/lib/webhooks/providers/ashby.test.ts +++ b/apps/sim/lib/webhooks/providers/ashby.test.ts @@ -3,7 +3,7 @@ */ import crypto from 'crypto' import { createMockRequest } from '@sim/testing' -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { ashbyHandler } from '@/lib/webhooks/providers/ashby' describe('ashbyHandler', () => { @@ -136,6 +136,135 @@ describe('ashbyHandler', () => { }) }) + describe('createSubscription error reporting', () => { + const realFetch = globalThis.fetch + afterEach(() => { + globalThis.fetch = realFetch + }) + + const ctx = { + requestId: 'req-1', + webhook: { + id: 'wh-1', + path: '/api/webhooks/trigger/abc', + providerConfig: { apiKey: 'k', triggerId: 'ashby_job_create' }, + }, + } as never + + const respondWith = (body: unknown, status = 200) => { + globalThis.fetch = vi.fn().mockResolvedValue( + new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }) + ) as never + } + + it('surfaces the object-shaped errors array Ashby documents', async () => { + // Reading only errorInfo.message misses this form, which is what a + // missing-permission failure arrives in - the user would see + // 'Unknown Ashby API error' instead of the actual cause. + respondWith({ success: false, errors: [{ message: 'missing_endpoint_permission' }] }) + await expect(ashbyHandler.createSubscription?.(ctx)).rejects.toThrow( + /missing_endpoint_permission/ + ) + }) + + it('surfaces the plain-string errors array Ashby also returns', async () => { + respondWith({ success: false, errors: ['webhook_not_found'] }) + await expect(ashbyHandler.createSubscription?.(ctx)).rejects.toThrow(/webhook_not_found/) + }) + + it('still prefers errorInfo.message when Ashby sends both shapes at once', async () => { + respondWith({ + success: false, + errors: ['webhook_not_found'], + errorInfo: { code: 'webhook_not_found', message: 'Webhook not found' }, + }) + await expect(ashbyHandler.createSubscription?.(ctx)).rejects.toThrow(/Webhook not found/) + }) + + it('keeps the actionable duplicate-webhook guidance reachable', async () => { + // The duplicate branch only fires when the message was extracted, so an + // unparsed error costs the user the instructions for fixing it. + respondWith({ success: false, errors: [{ message: 'duplicate webhook for this url' }] }) + await expect(ashbyHandler.createSubscription?.(ctx)).rejects.toThrow( + /Ashby Settings > API\/Webhooks/ + ) + }) + }) + + describe('deleteSubscription', () => { + const realFetch = globalThis.fetch + afterEach(() => { + globalThis.fetch = realFetch + }) + + const ctx = (strict: boolean) => + ({ + requestId: 'req-1', + strict, + webhook: { + id: 'wh-1', + providerConfig: { apiKey: 'k', externalId: 'ext-1' }, + }, + }) as never + + const respondWith = (body: unknown, status = 200) => { + globalThis.fetch = vi.fn().mockResolvedValue( + new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }) + ) as never + } + + it('treats a 200 carrying success:false as a failed delete', async () => { + // Ashby returns what would be a 4XX as HTTP 200. Branching on + // response.ok alone reported the leak as a successful cleanup. + respondWith({ success: false, errors: [{ message: 'missing_endpoint_permission' }] }) + await expect(ashbyHandler.deleteSubscription?.(ctx(true))).rejects.toThrow( + /missing_endpoint_permission/ + ) + }) + + it('stays non-fatal for a failed delete when not strict', async () => { + respondWith({ success: false, errors: [{ message: 'missing_endpoint_permission' }] }) + await expect(ashbyHandler.deleteSubscription?.(ctx(false))).resolves.toBeUndefined() + }) + + it('treats an already-removed webhook as done even in strict mode', async () => { + respondWith({ success: false, errors: ['webhook_not_found'] }) + await expect(ashbyHandler.deleteSubscription?.(ctx(true))).resolves.toBeUndefined() + }) + + it('recognizes the not-found envelope Ashby actually sends for a repeat delete', async () => { + // errorInfo.message wins over the code array in the extractor, so it reads + // 'Webhook not found' - matching that against `webhook_not_found` would + // turn idempotent cleanup into a strict-mode throw. + respondWith({ + success: false, + errors: ['webhook_not_found'], + errorInfo: { + code: 'webhook_not_found', + message: 'Webhook not found', + requestId: '01JSJ8FEK5ZN4XQBZP7DBKK7ZC', + }, + }) + await expect(ashbyHandler.deleteSubscription?.(ctx(true))).resolves.toBeUndefined() + }) + + it('recognizes a not-found reported only as prose', async () => { + respondWith({ success: false, errorInfo: { message: 'Webhook not found' } }) + await expect(ashbyHandler.deleteSubscription?.(ctx(true))).resolves.toBeUndefined() + }) + + it('accepts a successful delete', async () => { + respondWith({ success: true, results: { webhookId: 'ext-1' } }) + await expect(ashbyHandler.deleteSubscription?.(ctx(true))).resolves.toBeUndefined() + }) + }) + describe('extractIdempotencyId', () => { it('derives a stable key from application id + updatedAt', () => { const body = { diff --git a/apps/sim/lib/webhooks/providers/ashby.ts b/apps/sim/lib/webhooks/providers/ashby.ts index 781fef57c66..93925a7d6b9 100644 --- a/apps/sim/lib/webhooks/providers/ashby.ts +++ b/apps/sim/lib/webhooks/providers/ashby.ts @@ -17,6 +17,80 @@ import type { } from '@/lib/webhooks/providers/types' import { buildFallbackDeliveryFingerprint } from '@/lib/webhooks/providers/utils' +/** + * Kept local rather than imported from `@/tools/ashby/utils`, which has the same + * logic. The webhook providers are reachable from workspace page graphs, and the + * knowledge page graph currently sits exactly at the ceiling + * `check:tool-registry-boundary` allows - so neither an import edge into + * `@/tools/**` nor an extra module in this directory fits. Both copies derive + * from the same three documented Ashby error shapes and are covered + * independently by `tools/ashby/utils.test.ts` and `ashby.test.ts` here. + */ +/** + * Extract a human-readable error message from an Ashby error response. Ashby + * documents two shapes and uses three in practice: + * + * - `errorInfo: { code, message, requestId }` + * - `errors: ['webhook_not_found']` - plain strings + * - `errors: [{ message, parameter }]` - objects, which is the form a 403 for a + * missing module permission arrives in, and which stringifies to + * `[object Object]` unless the message is read explicitly + * + * A single response can carry more than one of these at once. + */ +function ashbyErrorMessage(data: unknown, fallback: string): string { + if (!data || typeof data !== 'object') return fallback + const d = data as Record + const info = d.errorInfo as Record | undefined + if (info && typeof info.message === 'string' && info.message) return info.message + if (Array.isArray(d.errors) && d.errors.length > 0) { + const messages = d.errors + .map((e) => { + if (typeof e === 'string') return e + if (e && typeof e === 'object') { + const entry = e as Record + const message = typeof entry.message === 'string' ? entry.message : '' + const parameter = typeof entry.parameter === 'string' ? entry.parameter : '' + if (message && parameter) return `${message} (${parameter})` + if (message) return message + } + return '' + }) + .filter(Boolean) + if (messages.length > 0) return messages.join('; ') + } + return fallback +} + +/** + * Whether an Ashby error response means the webhook id no longer exists. + * + * Ashby signals this as the machine code `webhook_not_found`, carried on + * `errorInfo.code` and/or as an `errors` entry — but the same envelope's + * `errorInfo.message` reads `Webhook not found`, and that is what + * `ashbyErrorMessage` returns, since message wins over the deprecated code + * array. Matching the extracted message against the code therefore misses the + * envelope Ashby actually sends for a repeat delete, and idempotent cleanup + * would be reported as a real failure. Read the codes directly, and keep a + * prose fallback for the message-only form. + */ +function isAshbyWebhookNotFound(data: Record, message: string): boolean { + const info = data.errorInfo as Record | undefined + if (typeof info?.code === 'string' && /webhook_not_found/i.test(info.code)) return true + + if (Array.isArray(data.errors)) { + for (const entry of data.errors) { + if (typeof entry === 'string' && /webhook_not_found/i.test(entry)) return true + if (entry && typeof entry === 'object') { + const entryMessage = (entry as Record).message + if (typeof entryMessage === 'string' && /webhook_not_found/i.test(entryMessage)) return true + } + } + } + + return /webhook[\s_]not[\s_]found/i.test(message) +} + const logger = createLogger('WebhookProvider:Ashby') function validateAshbySignature(secretToken: string, signature: string, body: string): boolean { @@ -212,9 +286,15 @@ export const ashbyHandler: WebhookProviderHandler = { const responseBody = (await ashbyResponse.json().catch(() => ({}))) as Record if (!ashbyResponse.ok || !responseBody.success) { - const errorInfo = responseBody.errorInfo as Record | undefined - const errorMessage = - errorInfo?.message || (responseBody.message as string) || 'Unknown Ashby API error' + // Ashby documents two error shapes and uses both. Reading only + // `errorInfo.message` misses the `errors: [{ message, parameter }]` form, + // which is what a missing-permission failure arrives in - and the + // duplicate-webhook branch below only fires when the message was + // extracted, so losing it costs the user the actionable guidance. + const errorMessage = ashbyErrorMessage( + responseBody, + (responseBody.message as string) || 'Unknown Ashby API error' + ) let userFriendlyMessage = 'Failed to create webhook subscription in Ashby' if (ashbyResponse.status === 401) { @@ -289,23 +369,43 @@ export const ashbyHandler: WebhookProviderHandler = { body: JSON.stringify({ webhookId: externalId }), }) - if (ashbyResponse.ok) { - await ashbyResponse.body?.cancel() + const responseBody = (await ashbyResponse.json().catch(() => ({}))) as Record + + /** + * Ashby returns what would be a 4XX elsewhere as HTTP 200 with + * `success: false`, so the status alone cannot separate a completed + * delete from a rejected one. Branching on `ashbyResponse.ok` reported + * every rejection as a successful cleanup while Sim dropped its own row + * — and with no `webhook.list` endpoint, an orphan left behind that way + * cannot be enumerated afterwards. + * + * Unlike `createSubscription`, an absent `success` field is treated as + * success rather than failure: teardown runs on the undeploy path, and + * failing closed on an unparseable body would wedge cleanup on a + * response shape Ashby does not document. + */ + const rejected = !ashbyResponse.ok || responseBody.success === false + const errorMessage = ashbyErrorMessage(responseBody, `HTTP ${ashbyResponse.status}`) + + if (!rejected) { logger.info( `[${ctx.requestId}] Successfully deleted Ashby webhook subscription ${externalId}` ) - } else if (ashbyResponse.status === 404) { - await ashbyResponse.body?.cancel() + } else if ( + ashbyResponse.status === 404 || + isAshbyWebhookNotFound(responseBody, errorMessage) + ) { logger.info( `[${ctx.requestId}] Ashby webhook ${externalId} not found during deletion (already removed)` ) } else { - const responseBody = await ashbyResponse.json().catch(() => ({})) logger.warn( - `[${ctx.requestId}] Failed to delete Ashby webhook (non-fatal): ${ashbyResponse.status}`, - { response: responseBody } + `[${ctx.requestId}] Failed to delete Ashby webhook (non-fatal): ${errorMessage}`, + { status: ashbyResponse.status, response: responseBody } ) - if (ctx.strict) throw new Error(`Failed to delete Ashby webhook: ${ashbyResponse.status}`) + if (ctx.strict) { + throw new Error(`Failed to delete Ashby webhook: ${errorMessage}`) + } } } catch (error) { logger.warn(`[${ctx.requestId}] Error deleting Ashby webhook (non-fatal)`, error) diff --git a/apps/sim/triggers/ashby/utils.ts b/apps/sim/triggers/ashby/utils.ts index f503f940164..ac50d4bf201 100644 --- a/apps/sim/triggers/ashby/utils.ts +++ b/apps/sim/triggers/ashby/utils.ts @@ -43,9 +43,9 @@ export function isAshbyEventMatch(triggerId: string, action: string): boolean { */ export function ashbySetupInstructions(eventType: string): string { const instructions = [ - 'Enter your Ashby API Key above. You can find your API key in Ashby at Settings > API Keys.', - `The webhook for ${eventType} events will be automatically created in Ashby when you save the trigger.`, - 'The webhook will be automatically deleted if you remove this trigger.', + 'Enter your Ashby API Key above. You can find your API key in Ashby at Settings > API Keys. It needs the apiKeysWrite permission.', + `The webhook for ${eventType} events is created in Ashby when you deploy the workflow, not when you save the trigger.`, + 'The webhook is deleted from Ashby when you remove this trigger and redeploy.', ] return instructions @@ -275,7 +275,7 @@ export function buildJobCreateOutputs(): Record { status: { type: 'string', description: 'Job status (Open, Closed, Draft, Archived)' }, employmentType: { type: 'string', - description: 'Employment type (FullTime, PartTime, Intern, Contract)', + description: 'Employment type (FullTime, PartTime, Intern, Contract, Temporary)', }, }, } as Record