diff --git a/packages/outpost/db/prisma/schema.prisma b/packages/outpost/db/prisma/schema.prisma index d6842d05..5fadc25f 100644 --- a/packages/outpost/db/prisma/schema.prisma +++ b/packages/outpost/db/prisma/schema.prisma @@ -106,7 +106,7 @@ model Message { isAiGenerated Boolean @default(false) confidenceScore Float? confidenceLevel String? // "HIGH" | "MEDIUM" | "LOW" - feedback String? // "POSITIVE" | "NEGATIVE" | null (no feedback yet) + feedback String? // "POSITIVE" | "NEGATIVE" | "COMMENT_DELETED" (GitHub comment gone; reaction poll stops) | null (no feedback yet) externalCommentId String? // GitHub comment/discussion-comment ID this message was posted as, if applicable // Inbound provider message ID this row was created FROM (Postmark MessageID today). // A real column rather than a JSON key so redelivery dedup is an indexed lookup and diff --git a/packages/outpost/queue/src/handlers/__tests__/github-reaction-poll.test.ts b/packages/outpost/queue/src/handlers/__tests__/github-reaction-poll.test.ts index 3b8caf45..f9408453 100644 --- a/packages/outpost/queue/src/handlers/__tests__/github-reaction-poll.test.ts +++ b/packages/outpost/queue/src/handlers/__tests__/github-reaction-poll.test.ts @@ -1,10 +1,12 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { MockInstance } from 'vitest'; const mockMessageFindMany = vi.fn(); const mockMessageUpdate = vi.fn(); const mockCreateJob = vi.fn(); const mockListCommentReactions = vi.fn(); const mockCreateGithubClient = vi.fn().mockReturnValue({}); +const mockIsRepoVisible = vi.fn(); vi.mock('@copilotkit/outpost/db', () => ({ prisma: { @@ -18,6 +20,8 @@ vi.mock('@copilotkit/outpost/db', () => ({ vi.mock('@copilotkit/outpost/shared', () => ({ createGithubClient: (...args: unknown[]) => mockCreateGithubClient(...args), listCommentReactions: (...args: unknown[]) => mockListCommentReactions(...args), + isRepoVisible: (...args: unknown[]) => mockIsRepoVisible(...args), + isGithubNotFound: (error: unknown) => (error as { status?: unknown } | null)?.status === 404, })); vi.mock('../../create-job.js', () => ({ @@ -26,6 +30,7 @@ vi.mock('../../create-job.js', () => ({ import { handleGithubReactionPoll, + COMMENT_DELETED_FEEDBACK, GITHUB_REACTION_POLL_CONCURRENCY, } from '../github-reaction-poll.js'; import { JobType } from '../../types.js'; @@ -128,7 +133,7 @@ describe('handleGithubReactionPoll', () => { expect(mockMessageUpdate).not.toHaveBeenCalled(); expect(mockCreateJob).not.toHaveBeenCalled(); expect(result.success).toBe(true); - expect(result.data).toEqual({ checked: 1, updated: 0 }); + expect(result.data).toEqual({ checked: 1, updated: 0, deleted: 0 }); }); it('skips a message gracefully when ticket.user.externalId is null', async () => { @@ -146,7 +151,7 @@ describe('handleGithubReactionPoll', () => { expect(mockMessageUpdate).not.toHaveBeenCalled(); expect(mockCreateJob).not.toHaveBeenCalled(); expect(result.success).toBe(true); - expect(result.data).toEqual({ checked: 1, updated: 0 }); + expect(result.data).toEqual({ checked: 1, updated: 0, deleted: 0 }); }); it('catches a listCommentReactions rejection, logs, and continues without updating', async () => { @@ -249,7 +254,10 @@ describe('handleGithubReactionPoll', () => { expect(state.peak).toBeGreaterThan(1); expect(mockListCommentReactions).toHaveBeenCalledTimes(40); - expect(result).toEqual({ success: true, data: { checked: 40, updated: 0 } }); + expect(result).toEqual({ + success: true, + data: { checked: 40, updated: 0, deleted: 0 }, + }); }); it('caps the number of GitHub reads in flight', async () => { @@ -287,7 +295,105 @@ describe('handleGithubReactionPoll', () => { ticketId: 'ticket-17', reason: expect.stringContaining('GitHub reaction'), }); - expect(result).toEqual({ success: true, data: { checked: 20, updated: 2 } }); + expect(result).toEqual({ + success: true, + data: { checked: 20, updated: 2, deleted: 0 }, + }); }); }); }); + +describe('handleGithubReactionPoll: comments that 404', () => { + // Prod polled the same 12 deleted CopilotKit/CopilotKit comments on every run + // and logged a "Failed to list reactions ... Not Found" error for each, forever. + function notFound(): Error { + return Object.assign(new Error('Not Found'), { status: 404 }); + } + + function message(id: string, sourceId = 'owner/repo#42') { + return { + id, + ticketId: `ticket-${id}`, + externalCommentId: '999', + ticket: { sourceId, user: { externalId: 'reporter-login' } }, + }; + } + + let consoleErrorSpy: MockInstance; + let consoleLogSpy: MockInstance; + + beforeEach(() => { + vi.clearAllMocks(); + process.env.GITHUB_APP_ID = 'app-1'; + process.env.GITHUB_PRIVATE_KEY = 'key'; + process.env.GITHUB_INSTALLATION_ID = '123'; + consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined); + consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); + }); + + afterEach(() => { + consoleErrorSpy.mockRestore(); + consoleLogSpy.mockRestore(); + }); + + it('marks a deleted comment so it is not polled again, with one info line and no error', async () => { + mockMessageFindMany.mockResolvedValue([message('msg-1')]); + mockListCommentReactions.mockRejectedValue(notFound()); + mockIsRepoVisible.mockResolvedValue(true); + + const result = await handleGithubReactionPoll({}, context); + + expect(mockMessageUpdate).toHaveBeenCalledWith({ + where: { id: 'msg-1' }, + data: { feedback: COMMENT_DELETED_FEEDBACK }, + }); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + const deletedLines = consoleLogSpy.mock.calls.filter((args) => + String(args[0]).includes('no longer exists'), + ); + expect(deletedLines).toHaveLength(1); + expect(String(deletedLines[0][0])).toContain('msg-1'); + expect(mockCreateJob).not.toHaveBeenCalled(); + expect(result).toEqual({ success: true, data: { checked: 1, updated: 0, deleted: 1 } }); + }); + + it('does not mark anything when the repo itself is not visible, and reports the repo once', async () => { + mockMessageFindMany.mockResolvedValue([message('msg-1'), message('msg-2')]); + mockListCommentReactions.mockRejectedValue(notFound()); + mockIsRepoVisible.mockResolvedValue(false); + + const result = await handleGithubReactionPoll({}, context); + + expect(mockMessageUpdate).not.toHaveBeenCalled(); + expect(mockIsRepoVisible).toHaveBeenCalledTimes(1); + expect(mockIsRepoVisible).toHaveBeenCalledWith(expect.anything(), 'owner', 'repo'); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + expect(String(consoleErrorSpy.mock.calls[0][0])).toContain('owner/repo'); + expect(result).toEqual({ success: true, data: { checked: 2, updated: 0, deleted: 0 } }); + }); + + it('leaves the message pending and logs an error when the repo check itself fails', async () => { + mockMessageFindMany.mockResolvedValue([message('msg-1')]); + mockListCommentReactions.mockRejectedValue(notFound()); + mockIsRepoVisible.mockRejectedValue(new Error('Bad credentials')); + + const result = await handleGithubReactionPoll({}, context); + + expect(mockMessageUpdate).not.toHaveBeenCalled(); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + expect(result).toEqual({ success: true, data: { checked: 1, updated: 0, deleted: 0 } }); + }); + + it('treats a non-404 failure as transient: logs it, does not mark, does not check the repo', async () => { + mockMessageFindMany.mockResolvedValue([message('msg-1')]); + mockListCommentReactions.mockRejectedValue( + Object.assign(new Error('Server Error'), { status: 502 }), + ); + + await handleGithubReactionPoll({}, context); + + expect(mockIsRepoVisible).not.toHaveBeenCalled(); + expect(mockMessageUpdate).not.toHaveBeenCalled(); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/outpost/queue/src/handlers/github-reaction-poll.ts b/packages/outpost/queue/src/handlers/github-reaction-poll.ts index 3bd83510..3e23dbdf 100644 --- a/packages/outpost/queue/src/handlers/github-reaction-poll.ts +++ b/packages/outpost/queue/src/handlers/github-reaction-poll.ts @@ -11,13 +11,17 @@ * * Lookups run GITHUB_REACTION_POLL_CONCURRENCY at a time. One at a time, the * poll needs one GitHub round trip per pending message, and that list only - * grows: comments deleted on GitHub stay pending forever. At 256 messages - * the serial loop overran the 30s job timeout on every attempt, and the job - * was dead-lettered after each scheduled run. + * grows. At 256 messages the serial loop overran the 30s job timeout on every + * attempt, and the job was dead-lettered after each scheduled run. */ import { prisma } from '@copilotkit/outpost/db'; -import { createGithubClient, listCommentReactions } from '@copilotkit/outpost/shared'; +import { + createGithubClient, + isGithubNotFound, + isRepoVisible, + listCommentReactions, +} from '@copilotkit/outpost/shared'; import { createJob } from '../create-job.js'; import { JobType } from '../types.js'; import type { GithubReactionPollPayload, JobResult, JobHandlerContext } from '../types.js'; @@ -29,6 +33,14 @@ import type { GithubReactionPollPayload, JobResult, JobHandlerContext } from '.. */ export const GITHUB_REACTION_POLL_CONCURRENCY = 8; +/** + * Terminal `feedback` value for a message whose GitHub comment no longer exists. + * It takes the message out of the `feedback: null` set this job polls, so a + * deleted comment is logged once instead of erroring on every run. Feedback + * calibration counts only POSITIVE and NEGATIVE, so it is not affected. + */ +export const COMMENT_DELETED_FEEDBACK = 'COMMENT_DELETED'; + /** * Parse "owner/repo#number" into its components. */ @@ -83,8 +95,59 @@ export async function handleGithubReactionPoll( const client = createGithubClient({ appId, privateKey, installationId }); + // One visibility check per repo per run, shared by every 404 in that repo. + const repoVisibility = new Map>(); + const reportedHiddenRepos = new Set(); + let checked = 0; let updated = 0; + let deleted = 0; + + /** + * A 404 from the reactions endpoint means the comment is gone only if the App + * can still see the repo. If it cannot, the problem is access, not deletion: + * the message stays pending and the repo is reported once as an error. + */ + async function handleCommentNotFound( + messageId: string, + owner: string, + repo: string, + commentId: number, + ): Promise { + const key = `${owner}/${repo}`; + const check = repoVisibility.get(key) ?? isRepoVisible(client, owner, repo); + repoVisibility.set(key, check); + + let visible: boolean; + try { + visible = await check; + } catch (error) { + console.error( + `[GithubReactionPoll] Failed to list reactions for message ${messageId}: comment ${commentId} not found, and checking ${key} failed:`, + error instanceof Error ? error.message : String(error), + ); + return; + } + + if (!visible) { + if (!reportedHiddenRepos.has(key)) { + reportedHiddenRepos.add(key); + console.error( + `[GithubReactionPoll] Repo ${key} is not visible to the GitHub App (404); skipping its messages this run`, + ); + } + return; + } + + await prisma.message.update({ + where: { id: messageId }, + data: { feedback: COMMENT_DELETED_FEEDBACK }, + }); + deleted += 1; + console.log( + `[GithubReactionPoll] Comment ${commentId} for message ${messageId} no longer exists on ${key}; marked ${COMMENT_DELETED_FEEDBACK}, it will not be polled again`, + ); + } async function checkMessage(message: (typeof pendingMessages)[number]): Promise { checked += 1; @@ -107,6 +170,10 @@ export async function handleGithubReactionPoll( try { reactions = await listCommentReactions(client, parsed.owner, parsed.repo, commentId); } catch (error) { + if (isGithubNotFound(error)) { + await handleCommentNotFound(message.id, parsed.owner, parsed.repo, commentId); + return; + } console.error( `[GithubReactionPoll] Failed to list reactions for message ${message.id}:`, error instanceof Error ? error.message : String(error), @@ -155,11 +222,11 @@ export async function handleGithubReactionPoll( await context.reportProgress(100); console.log( - `[GithubReactionPoll] Checked ${checked} messages, updated ${updated} with feedback`, + `[GithubReactionPoll] Checked ${checked} messages, updated ${updated} with feedback, marked ${deleted} deleted`, ); return { success: true, - data: { checked, updated }, + data: { checked, updated, deleted }, }; } diff --git a/packages/outpost/shared/src/integrations/__tests__/github-client.test.ts b/packages/outpost/shared/src/integrations/__tests__/github-client.test.ts index 4b23e298..82244e3b 100644 --- a/packages/outpost/shared/src/integrations/__tests__/github-client.test.ts +++ b/packages/outpost/shared/src/integrations/__tests__/github-client.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from 'vitest'; -import { listCommentReactions } from '../github-client.js'; +import { isGithubNotFound, isRepoVisible, listCommentReactions } from '../github-client.js'; +import type { GithubReactionClient } from '../github-client.js'; describe('listCommentReactions', () => { it('returns each reaction with its content and reactor login', async () => { @@ -28,3 +29,46 @@ describe('listCommentReactions', () => { }); }); }); + +describe('isGithubNotFound', () => { + it('is true only for an error carrying status 404', () => { + expect(isGithubNotFound(Object.assign(new Error('Not Found'), { status: 404 }))).toBe(true); + expect(isGithubNotFound(Object.assign(new Error('Server Error'), { status: 502 }))).toBe( + false, + ); + expect(isGithubNotFound(new Error('Not Found'))).toBe(false); + expect(isGithubNotFound(null)).toBe(false); + expect(isGithubNotFound('Not Found')).toBe(false); + }); +}); + +describe('isRepoVisible', () => { + function clientWithRepoGet(get: GithubReactionClient['repos']['get']): GithubReactionClient { + return { repos: { get }, reactions: { listForIssueComment: vi.fn() } }; + } + + it('is true when the repo can be read', async () => { + const get = vi.fn().mockResolvedValue({ data: {} }); + + await expect(isRepoVisible(clientWithRepoGet(get), 'owner', 'repo')).resolves.toBe(true); + expect(get).toHaveBeenCalledWith({ owner: 'owner', repo: 'repo' }); + }); + + it('is false when the repo itself 404s', async () => { + const get = vi + .fn() + .mockRejectedValue(Object.assign(new Error('Not Found'), { status: 404 })); + + await expect(isRepoVisible(clientWithRepoGet(get), 'owner', 'repo')).resolves.toBe(false); + }); + + it('rethrows any other failure', async () => { + const get = vi + .fn() + .mockRejectedValue(Object.assign(new Error('Bad credentials'), { status: 401 })); + + await expect(isRepoVisible(clientWithRepoGet(get), 'owner', 'repo')).rejects.toThrow( + 'Bad credentials', + ); + }); +}); diff --git a/packages/outpost/shared/src/integrations/github-client.ts b/packages/outpost/shared/src/integrations/github-client.ts index 7c9011fe..14c47211 100644 --- a/packages/outpost/shared/src/integrations/github-client.ts +++ b/packages/outpost/shared/src/integrations/github-client.ts @@ -17,8 +17,11 @@ export interface GithubClientConfig { installationId: string; } -/** Minimal interface for the one Octokit call this module needs. */ +/** Minimal interface for the Octokit calls this module needs. */ export interface GithubReactionClient { + repos: { + get(params: { owner: string; repo: string }): Promise; + }; reactions: { listForIssueComment(params: { owner: string; @@ -68,3 +71,35 @@ export async function listCommentReactions( .filter((r) => r.user !== null) .map((r) => ({ content: r.content, login: r.user!.login })); } + +/** + * True when an Octokit call failed with HTTP 404. Octokit's RequestError + * carries the response status on `status`. + */ +export function isGithubNotFound(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + (error as { status?: unknown }).status === 404 + ); +} + +/** + * Whether the App can see `owner/repo`. GitHub answers 404 both for a deleted + * comment and for a repo the App cannot see; only the first means the comment + * is gone for good, so callers check this before treating a comment 404 as a + * deletion. A 404 on the repo returns false; any other failure propagates. + */ +export async function isRepoVisible( + client: GithubReactionClient, + owner: string, + repo: string, +): Promise { + try { + await client.repos.get({ owner, repo }); + return true; + } catch (error) { + if (isGithubNotFound(error)) return false; + throw error; + } +} diff --git a/packages/outpost/shared/src/integrations/index.ts b/packages/outpost/shared/src/integrations/index.ts index b6ca8e8e..b4473da9 100644 --- a/packages/outpost/shared/src/integrations/index.ts +++ b/packages/outpost/shared/src/integrations/index.ts @@ -7,5 +7,10 @@ export type { AccountStore, ExistingAccount, } from './hubspot-sync.js'; -export { createGithubClient, listCommentReactions } from './github-client.js'; +export { + createGithubClient, + listCommentReactions, + isGithubNotFound, + isRepoVisible, +} from './github-client.js'; export type { GithubClientConfig, GithubReactionClient, CommentReaction } from './github-client.js';