diff --git a/src/__tests__/webhook.test.ts b/src/__tests__/webhook.test.ts index f670de6ff..fbcf849de 100644 --- a/src/__tests__/webhook.test.ts +++ b/src/__tests__/webhook.test.ts @@ -108,6 +108,17 @@ describe('webhook', () => { expect(mockFetch).not.toHaveBeenCalled(); }); + it('logs a delivery that still fails on its last attempt', async () => { + const { sendWebhook } = await import('../webhook.js'); + const { log } = await import('../utils/logger.js'); + mockAutoDetectInit.mockResolvedValueOnce(singleEndpointConfig(0)); + mockFetch.mockResolvedValue({ ok: false, status: 503 }); + + await sendWebhook('push', { tool: 'claude', data: {} }); + + expect(log.warn).toHaveBeenCalledWith('Webhook to https://example.com/hook failed after 1 attempt(s): status 503'); + }); + it('should handle fetch errors gracefully', async () => { mockFetch.mockRejectedValueOnce(new Error('Network error')); @@ -147,6 +158,21 @@ describe('webhook', () => { }); }); + function singleEndpointConfig(retries: number) { + return { + localConfig: { repo: { localPath: '/tmp', remote: '' }, username: 'test', scope: 'user' }, + teamConfig: { + team: 'test', + sharing: { + webhooks: { + enabled: true, + endpoints: [{ url: 'https://example.com/hook', type: 'json', events: ['*'], timeout: 5000, retries }], + }, + }, + }, + }; + } + describe('testWebhook', () => { it('should send test event to all endpoints', async () => { const { testWebhook } = await import('../webhook.js'); @@ -156,6 +182,66 @@ describe('webhook', () => { expect(mockFetch).toHaveBeenCalledTimes(2); }); + it('reports success only when the endpoint accepts the event', async () => { + const { testWebhook } = await import('../webhook.js'); + const { log } = await import('../utils/logger.js'); + mockAutoDetectInit.mockResolvedValueOnce(singleEndpointConfig(0)); + + await testWebhook(); + + expect(log.success).toHaveBeenCalledWith('Webhook test successful: https://example.com/hook'); + expect(log.error).not.toHaveBeenCalled(); + }); + + it('reports failure when the endpoint keeps answering 5xx', async () => { + const { testWebhook } = await import('../webhook.js'); + const { log } = await import('../utils/logger.js'); + mockAutoDetectInit.mockResolvedValueOnce(singleEndpointConfig(0)); + mockFetch.mockResolvedValue({ ok: false, status: 500 }); + + await testWebhook(); + + expect(log.success).not.toHaveBeenCalled(); + expect(log.error).toHaveBeenCalledWith('Webhook test failed: https://example.com/hook'); + expect(log.warn).toHaveBeenCalledWith('Webhook to https://example.com/hook failed after 1 attempt(s): status 500'); + }); + + it('reports failure on a non-retryable 4xx', async () => { + const { testWebhook } = await import('../webhook.js'); + const { log } = await import('../utils/logger.js'); + mockAutoDetectInit.mockResolvedValueOnce(singleEndpointConfig(3)); + mockFetch.mockResolvedValue({ ok: false, status: 404 }); + + await testWebhook(); + + expect(mockFetch).toHaveBeenCalledTimes(1); + expect(log.success).not.toHaveBeenCalled(); + expect(log.error).toHaveBeenCalledWith('Webhook test failed: https://example.com/hook'); + }); + + it('retries a timed-out request with backoff and reports failure after the last attempt', async () => { + vi.useFakeTimers(); + try { + const { testWebhook } = await import('../webhook.js'); + const { log } = await import('../utils/logger.js'); + mockAutoDetectInit.mockResolvedValueOnce(singleEndpointConfig(1)); + const abort = Object.assign(new Error('aborted'), { name: 'AbortError' }); + mockFetch.mockRejectedValue(abort); + + const run = testWebhook(); + await vi.advanceTimersByTimeAsync(999); + expect(mockFetch).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(1); + await run; + + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(log.warn).toHaveBeenCalledWith('Webhook to https://example.com/hook failed after 2 attempt(s): timed out after 5000ms'); + expect(log.error).toHaveBeenCalledWith('Webhook test failed: https://example.com/hook'); + } finally { + vi.useRealTimers(); + } + }); + it('should send test event to specific endpoint', async () => { const { testWebhook } = await import('../webhook.js'); diff --git a/src/webhook.ts b/src/webhook.ts index 78df36686..6fe95e37b 100644 --- a/src/webhook.ts +++ b/src/webhook.ts @@ -43,12 +43,14 @@ export async function sendWebhook( } /** - * Send webhook to a single endpoint with retry logic. + * Send webhook to a single endpoint with retry logic. Resolves to whether the + * endpoint accepted the event; every failure is logged here and never thrown, + * so a webhook can never fail the command that fired it. */ async function sendToEndpoint( endpoint: WebhookEndpoint, payload: WebhookPayload, -): Promise { +): Promise { const { url, type, secret, timeout, retries } = endpoint; const body = formatMessage(type, payload); @@ -72,10 +74,10 @@ async function sendToEndpoint( } for (let attempt = 0; attempt <= retries; attempt++) { + let failure: string; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); try { - const controller = new AbortController(); - const timeoutId = setTimeout(() => controller.abort(), timeout); - const response = await fetch(url, { method: 'POST', headers, @@ -83,35 +85,33 @@ async function sendToEndpoint( signal: controller.signal, }); - clearTimeout(timeoutId); - if (response.ok) { log.debug(`Webhook sent successfully to ${url}`); - return; + return true; } if (response.status >= 400 && response.status < 500 && response.status !== 429) { log.warn(`Webhook to ${url} failed with status ${response.status} (not retrying)`); - return; - } - - if (attempt < retries) { - const delay = Math.pow(2, attempt) * 1000; - log.debug(`Webhook to ${url} failed, retrying in ${delay}ms...`); - await new Promise((resolve) => setTimeout(resolve, delay)); + return false; } + failure = `status ${response.status}`; } catch (error) { - if (error instanceof Error && error.name === 'AbortError') { - log.warn(`Webhook to ${url} timed out after ${timeout}ms`); - } else if (attempt < retries) { - const delay = Math.pow(2, attempt) * 1000; - log.debug(`Webhook to ${url} failed, retrying in ${delay}ms...`); - await new Promise((resolve) => setTimeout(resolve, delay)); - } else { - log.warn(`Webhook to ${url} failed after ${retries + 1} attempts: ${(error as Error).message}`); - } + failure = error instanceof Error && error.name === 'AbortError' + ? `timed out after ${timeout}ms` + : (error as Error).message; + } finally { + clearTimeout(timeoutId); + } + + if (attempt < retries) { + const delay = Math.pow(2, attempt) * 1000; + log.debug(`Webhook to ${url} failed (${failure}), retrying in ${delay}ms...`); + await new Promise((resolve) => setTimeout(resolve, delay)); + } else { + log.warn(`Webhook to ${url} failed after ${retries + 1} attempt(s): ${failure}`); } } + return false; } /** @@ -177,11 +177,10 @@ export async function testWebhook(url?: string): Promise { for (const endpoint of endpoints) { log.info(`Testing webhook to ${endpoint.url}...`); - try { - await sendToEndpoint(endpoint, testPayload); + if (await sendToEndpoint(endpoint, testPayload)) { log.success(`Webhook test successful: ${endpoint.url}`); - } catch (error) { - log.error(`Webhook test failed: ${endpoint.url} - ${(error as Error).message}`); + } else { + log.error(`Webhook test failed: ${endpoint.url}`); } } }