diff --git a/src/github/client.ts b/src/github/client.ts index 8e03e09a..eeef4bd5 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -226,6 +226,15 @@ export const githubClient = { }; }, + async deletePRComment(owner: string, repo: string, commentId: number): Promise { + logger.debug('Deleting PR comment', { owner, repo, commentId }); + await getClient().issues.deleteComment({ + owner, + repo, + comment_id: commentId, + }); + }, + async getPRReviews(owner: string, repo: string, prNumber: number): Promise { logger.debug('Fetching PR reviews', { owner, repo, prNumber }); const { data } = await getClient().pulls.listReviews({ diff --git a/src/triggers/github/webhook-handler.ts b/src/triggers/github/webhook-handler.ts index f1de81ae..3c960c2d 100644 --- a/src/triggers/github/webhook-handler.ts +++ b/src/triggers/github/webhook-handler.ts @@ -7,7 +7,12 @@ import { getPersonaToken, resolvePersonaIdentities } from '../../github/personas import { withPMCredentials } from '../../pm/context.js'; import { createPMProvider, pmRegistry, withPMProvider } from '../../pm/index.js'; import { extractGitHubContext, generateAckMessage } from '../../router/ackMessageGenerator.js'; -import type { CascadeConfig, ProjectConfig, TriggerContext } from '../../types/index.js'; +import type { + AgentResult, + CascadeConfig, + ProjectConfig, + TriggerContext, +} from '../../types/index.js'; import { enqueueWebhook, getQueueLength, @@ -25,6 +30,35 @@ import { runAgentExecutionPipeline } from '../shared/agent-execution.js'; import { processNextQueuedWebhook } from '../shared/webhook-queue.js'; import type { TriggerResult } from '../types.js'; +async function deleteProgressCommentOnSuccess( + result: TriggerResult, + _agentResult: AgentResult, +): Promise { + // Only delete the progress comment for non-implementation agents. + // The implementation agent's success is handled via lifecycle (handleSuccess), + // which manages the PR comment separately. + if (result.agentType === 'implementation') return; + + const input = result.agentInput as { repoFullName?: string }; + if (!input.repoFullName || !result.prNumber) return; + + let owner: string; + let repo: string; + try { + ({ owner, repo } = parseRepoFullName(input.repoFullName)); + } catch { + return; + } + + const { initialCommentId } = getSessionState(); + if (!initialCommentId) return; + + await safeOperation(() => githubClient.deletePRComment(owner, repo, initialCommentId), { + action: 'delete progress comment after agent success', + prNumber: result.prNumber, + }); +} + async function updateInitialCommentWithError( result: TriggerResult, agentResult: { success: boolean; error?: string }, @@ -105,6 +139,7 @@ async function executeGitHubAgent( skipPrepareForAgent: true, skipHandleFailure: true, handleSuccessOnlyForAgentType: 'implementation', + onSuccess: deleteProgressCommentOnSuccess, onFailure: updateInitialCommentWithError, logLabel: 'GitHub agent', }; diff --git a/src/triggers/shared/agent-execution.ts b/src/triggers/shared/agent-execution.ts index 35afd458..a4f24f62 100644 --- a/src/triggers/shared/agent-execution.ts +++ b/src/triggers/shared/agent-execution.ts @@ -33,6 +33,12 @@ export interface AgentExecutionConfig { */ handleSuccessOnlyForAgentType?: string; + /** + * Optional callback invoked when the agent succeeds (after pipeline completes). + * Used by GitHub to delete the progress comment for non-implementation agents. + */ + onSuccess?: (result: TriggerResult, agentResult: AgentResult) => Promise; + /** * Optional callback invoked when the agent fails (after pipeline completes). * Used by GitHub to update the PR comment with an error message. @@ -178,7 +184,7 @@ export async function runAgentExecutionPipeline( return; } - const { skipPrepareForAgent = false, onFailure, logLabel = 'Agent' } = executionConfig; + const { skipPrepareForAgent = false, onSuccess, onFailure, logLabel = 'Agent' } = executionConfig; const workItemId = result.workItemId; @@ -218,6 +224,10 @@ export async function runAgentExecutionPipeline( runId: agentResult.runId, }); + if (onSuccess && agentResult.success) { + await onSuccess(result, agentResult); + } + if (onFailure && !agentResult.success) { await onFailure(result, agentResult); } diff --git a/tests/unit/github/client.test.ts b/tests/unit/github/client.test.ts index 9de950f8..7432a9a2 100644 --- a/tests/unit/github/client.test.ts +++ b/tests/unit/github/client.test.ts @@ -15,6 +15,7 @@ const mockPulls = { const mockIssues = { createComment: vi.fn(), updateComment: vi.fn(), + deleteComment: vi.fn(), listComments: vi.fn(), }; @@ -303,6 +304,20 @@ describe('githubClient', () => { }); }); + describe('deletePRComment', () => { + it('calls deleteComment with correct params', async () => { + mockIssues.deleteComment.mockResolvedValue({}); + + await withGitHubToken('test-token', () => githubClient.deletePRComment('owner', 'repo', 200)); + + expect(mockIssues.deleteComment).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + comment_id: 200, + }); + }); + }); + describe('getPRReviews', () => { it('returns mapped reviews', async () => { mockPulls.listReviews.mockResolvedValue({ diff --git a/tests/unit/triggers/agent-execution.test.ts b/tests/unit/triggers/agent-execution.test.ts index 3003cee4..7fbae5d6 100644 --- a/tests/unit/triggers/agent-execution.test.ts +++ b/tests/unit/triggers/agent-execution.test.ts @@ -394,6 +394,36 @@ describe('runAgentExecutionPipeline', () => { }); }); + describe('onSuccess callback', () => { + it('calls onSuccess when agent succeeds', async () => { + const agentResult: AgentResult = { + success: true, + runId: 'run-1', + output: '', + }; + vi.mocked(runAgent).mockResolvedValue(agentResult); + const onSuccess = vi.fn().mockResolvedValue(undefined); + + await runAgentExecutionPipeline(mockTriggerResult, mockProject, mockConfig, { onSuccess }); + + expect(onSuccess).toHaveBeenCalledWith(mockTriggerResult, agentResult); + }); + + it('does not call onSuccess when agent fails', async () => { + vi.mocked(runAgent).mockResolvedValue({ + success: false, + error: 'Agent error', + runId: 'run-1', + output: '', + }); + const onSuccess = vi.fn().mockResolvedValue(undefined); + + await runAgentExecutionPipeline(mockTriggerResult, mockProject, mockConfig, { onSuccess }); + + expect(onSuccess).not.toHaveBeenCalled(); + }); + }); + describe('onFailure callback', () => { it('calls onFailure when agent fails', async () => { const agentResult: AgentResult = {