diff --git a/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts b/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts index 1b8ecdf08..38f59c9de 100644 --- a/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts +++ b/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts @@ -169,7 +169,7 @@ describe('preparePrReviewNotificationDelivery', () => { number: 42, threads: [ { id: 't1', resolved: true, path: null, line: null, comments: [] }, - { id: 't2', resolved: false, path: null, line: null, comments: [] }, + { id: 't2', resolved: true, path: null, line: null, comments: [] }, ], issueComments: [ { @@ -383,6 +383,263 @@ describe('triagePrReviewActivity', () => { expect(prompt).not.toContain('Current pull request state:'); }); + it.each([ + { followUpQuestion: '', followUpPrompt: '' }, + { + followUpQuestion: 'Generated question without an instruction?', + followUpPrompt: '', + }, + { + followUpQuestion: '', + followUpPrompt: 'Generated instruction without a question.', + }, + ])( + 'adds a deterministic resolve offer when unresolved threads remain and triage returns an incomplete offer', + async ({ followUpQuestion, followUpPrompt }) => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: true, + summary: 'A reviewer left feedback on the pull request.', + followUpQuestion, + followUpPrompt, + }, + }); + + await expect( + triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 1, + unresolvedThreadCount: 2, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: null, + mergeable: true, + }, + }), + ).resolves.toEqual({ + post: true, + summary: 'A reviewer left feedback on the pull request.', + followUpQuestion: 'Would you like me to resolve these issues?', + followUpPrompt: + 'Review and resolve the actionable problems on [owner/repo#42](https://github.com/owner/repo/pull/42). Revalidate each unresolved review comment against the current code and address valid issues. Update the pull request with the validated fixes.', + }); + }, + ); + + it('adds a deterministic resolve offer when a CI check is failing', async () => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: true, + summary: 'The pull request has a failing check.', + followUpQuestion: '', + followUpPrompt: '', + }, + }); + + await expect( + triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 0, + unresolvedThreadCount: 0, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: { + checks: [ + { name: 'CI / Lint', status: 'success' }, + { name: 'CI / Tests', status: 'failure' }, + ], + }, + mergeable: true, + }, + }), + ).resolves.toEqual({ + post: true, + summary: 'The pull request has a failing check.', + followUpQuestion: 'Would you like me to resolve this issue?', + followUpPrompt: + 'Review and resolve the actionable problems on [owner/repo#42](https://github.com/owner/repo/pull/42). Investigate and fix the failing CI checks: CI / Tests. Update the pull request with the validated fixes.', + }); + }); + + it('adds a deterministic resolve offer when the pull request has merge conflicts', async () => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: true, + summary: 'The pull request has merge conflicts.', + followUpQuestion: '', + followUpPrompt: '', + }, + }); + + await expect( + triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 0, + unresolvedThreadCount: 0, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: null, + mergeable: false, + }, + }), + ).resolves.toEqual({ + post: true, + summary: 'The pull request has merge conflicts.', + followUpQuestion: 'Would you like me to resolve this issue?', + followUpPrompt: + "Review and resolve the actionable problems on [owner/repo#42](https://github.com/owner/repo/pull/42). Resolve the pull request's merge conflicts without discarding either side's intended changes. Update the pull request with the validated fixes.", + }); + }); + + it('combines review feedback, failing checks, and merge conflicts in one resolve offer', async () => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: true, + summary: 'The pull request has several actionable problems.', + followUpQuestion: '', + followUpPrompt: '', + }, + }); + + await expect( + triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 0, + unresolvedThreadCount: 1, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: { + checks: [{ name: 'CI / Tests', status: 'error' }], + }, + mergeable: false, + }, + }), + ).resolves.toEqual({ + post: true, + summary: 'The pull request has several actionable problems.', + followUpQuestion: 'Would you like me to resolve these issues?', + followUpPrompt: + "Review and resolve the actionable problems on [owner/repo#42](https://github.com/owner/repo/pull/42). Revalidate each unresolved review comment against the current code and address valid issues. Investigate and fix the failing CI checks: CI / Tests. Resolve the pull request's merge conflicts without discarding either side's intended changes. Update the pull request with the validated fixes.", + }); + }); + + it('replaces a generated offer with the complete live actionable state', async () => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: true, + summary: 'A reviewer left feedback.', + followUpQuestion: 'Would you like me to resolve the review feedback?', + followUpPrompt: 'Resolve the review comments.', + }, + }); + + const result = await triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 0, + unresolvedThreadCount: 1, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: { + checks: [{ name: 'CI / Tests', status: 'failure' }], + }, + mergeable: false, + }, + }); + + expect(result).toEqual( + expect.objectContaining({ + post: true, + followUpQuestion: 'Would you like me to resolve these issues?', + followUpPrompt: expect.stringContaining( + 'Investigate and fix the failing CI checks: CI / Tests.', + ), + }), + ); + expect(result).toEqual( + expect.objectContaining({ + followUpPrompt: expect.stringContaining( + "Resolve the pull request's merge conflicts", + ), + }), + ); + }); + + it('notifies with a deterministic summary when triage rejects live actionable state', async () => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: false, + summary: '', + followUpQuestion: '', + followUpPrompt: '', + }, + }); + + await expect( + triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 0, + unresolvedThreadCount: 0, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: { + checks: [{ name: 'CI / Tests', status: 'failure' }], + }, + mergeable: true, + }, + }), + ).resolves.toEqual({ + post: true, + summary: + '[owner/repo#42](https://github.com/owner/repo/pull/42) needs attention: 1 CI check is failing.', + followUpQuestion: 'Would you like me to resolve this issue?', + followUpPrompt: + 'Review and resolve the actionable problems on [owner/repo#42](https://github.com/owner/repo/pull/42). Investigate and fix the failing CI checks: CI / Tests. Update the pull request with the validated fixes.', + }); + }); + + it('keeps a notification informational when no unresolved threads remain', async () => { + mockGenerateObject.mockResolvedValue({ + object: { + worthNotifying: true, + summary: 'The feedback has already been addressed.', + followUpQuestion: '', + followUpPrompt: '', + }, + }); + + await expect( + triagePrReviewActivity({ + ...request, + events: eventsWithoutSelfReview, + context: { + resolvedThreadCount: 1, + unresolvedThreadCount: 0, + latestReviewStatus: null, + latestReviewSummaryComment: null, + ciStatus: null, + mergeable: true, + }, + }), + ).resolves.toEqual({ + post: true, + summary: 'The feedback has already been addressed.', + followUpQuestion: null, + followUpPrompt: null, + }); + }); + it('passes the source-control provider label into the triage prompt', async () => { mockGenerateObject.mockResolvedValue({ object: { diff --git a/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts b/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts index 2030d34e0..b33260764 100644 --- a/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts +++ b/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts @@ -750,11 +750,44 @@ export async function triagePrReviewActivity({ prompt, }); - if (!object.worthNotifying && !containsSelfReviewResult) { + const unresolvedThreadCount = context?.unresolvedThreadCount ?? 0; + const failedCheckNames = + context?.ciStatus?.checks + .filter((check) => check.status === 'failure' || check.status === 'error') + .map((check) => check.name) ?? []; + const hasMergeConflicts = context?.mergeable === false; + const actionableProblemCount = + unresolvedThreadCount + + failedCheckNames.length + + (hasMergeConflicts ? 1 : 0); + const hasLiveActionableState = actionableProblemCount > 0; + + if ( + !object.worthNotifying && + !containsSelfReviewResult && + !hasLiveActionableState + ) { return { post: false, reason: 'not_worth_notifying' }; } - const summary = object.summary.trim(); + const deterministicSummary = [ + ...(unresolvedThreadCount > 0 + ? [ + `${unresolvedThreadCount} unresolved review ${unresolvedThreadCount === 1 ? 'thread remains' : 'threads remain'}`, + ] + : []), + ...(failedCheckNames.length > 0 + ? [ + `${failedCheckNames.length} CI ${failedCheckNames.length === 1 ? 'check is' : 'checks are'} failing`, + ] + : []), + ...(hasMergeConflicts ? ['the pull request has merge conflicts'] : []), + ].join(', '); + const summary = + object.summary.trim() || + (hasLiveActionableState + ? `[${repository}#${prNumber}](${prUrl}) needs attention: ${deterministicSummary}.` + : ''); if (!summary) { throw new Error( @@ -764,15 +797,44 @@ export async function triagePrReviewActivity({ const followUpQuestion = object.followUpQuestion.trim(); const followUpPrompt = object.followUpPrompt.trim(); - // The offer is only actionable when both halves exist; a question without - // an injectable instruction (or vice versa) degrades to a plain summary. - const hasFollowUp = followUpQuestion.length > 0 && followUpPrompt.length > 0; + const hasGeneratedFollowUp = + followUpQuestion.length > 0 && followUpPrompt.length > 0; + const fallbackInstructions = [ + ...(unresolvedThreadCount > 0 + ? [ + 'Revalidate each unresolved review comment against the current code and address valid issues.', + ] + : []), + ...(failedCheckNames.length > 0 + ? [ + `Investigate and fix the failing CI checks: ${failedCheckNames.join(', ')}.`, + ] + : []), + ...(hasMergeConflicts + ? [ + "Resolve the pull request's merge conflicts without discarding either side's intended changes.", + ] + : []), + ]; + const fallbackFollowUp = hasLiveActionableState + ? { + question: + actionableProblemCount === 1 + ? 'Would you like me to resolve this issue?' + : 'Would you like me to resolve these issues?', + prompt: `Review and resolve the actionable problems on [${repository}#${prNumber}](${prUrl}). ${fallbackInstructions.join(' ')} Update the pull request with the validated fixes.`, + } + : null; return { post: true, summary, - followUpQuestion: hasFollowUp ? followUpQuestion : null, - followUpPrompt: hasFollowUp ? followUpPrompt : null, + followUpQuestion: + fallbackFollowUp?.question ?? + (hasGeneratedFollowUp ? followUpQuestion : null), + followUpPrompt: + fallbackFollowUp?.prompt ?? + (hasGeneratedFollowUp ? followUpPrompt : null), }; }