From ab4c874cc05f387d10b567037684a6c3709edb5c Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 09:33:24 -0700 Subject: [PATCH 1/7] fix(warden): Prevent stale PR feedback writes Skip PR review feedback writes when a workflow run is no longer analyzing the current pull request head. Keep non-blocking unresolvable findings in Checks instead of body-only review comments, while preserving blocking request-changes reviews. Co-Authored-By: GPT-5 Codex --- README.md | 2 +- packages/docs/public/llms.txt | 6 +- packages/docs/src/content/docs/github.mdx | 11 +- .../warden/src/action/review/poster.test.ts | 89 ++++++++- packages/warden/src/action/review/poster.ts | 34 +++- .../src/action/workflow/pr-workflow.test.ts | 171 ++++++++++++++++++ .../warden/src/action/workflow/pr-workflow.ts | 152 +++++++++++++--- 7 files changed, 415 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index 1020acbc..b4d56fc1 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Your code is under new management. Agents that review your code - locally or on **Two ways to run.** CLI catches issues before you push. GitHub Action reviews every PR automatically. -**GitHub-native.** Findings appear as inline PR comments with suggested fixes. +**GitHub-native.** Eligible findings appear as inline PR comments with suggested fixes, and every finding is reported in Checks. ## Quick Start diff --git a/packages/docs/public/llms.txt b/packages/docs/public/llms.txt index 0a1fd9c0..6d8d2cdf 100644 --- a/packages/docs/public/llms.txt +++ b/packages/docs/public/llms.txt @@ -520,7 +520,7 @@ Most review skills only need `Read`, `Grep`, and `Glob` for exploring context. ## Pull Request Reviews -Warden runs automatically on pull requests via GitHub Actions, posting findings as review comments. +Warden runs automatically on pull requests via GitHub Actions and reports findings in Checks. When findings can be tied to the current pull request diff, Warden also posts inline review comments. ### Organization Setup @@ -546,10 +546,12 @@ Creates `warden.toml` and `.github/workflows/warden.yml`. 1. PR is opened or updated 2. GitHub Actions runs the Warden workflow 3. Warden analyzes changed files against configured triggers -4. Findings are posted as inline review comments +4. Findings are reported in Checks. Eligible findings are also posted as inline review comments 5. If `requestChanges` is enabled, the review requests changes when findings exceed `failOn` 6. If `failCheck` is enabled, the check run fails when findings exceed `failOn` +Warden skips PR review feedback when the workflow run is no longer analyzing the current PR head. Non-blocking findings without an inline diff location stay in Checks instead of creating timeline comments that cannot be resolved later. + ### GitHub Actions Workflow ```yaml diff --git a/packages/docs/src/content/docs/github.mdx b/packages/docs/src/content/docs/github.mdx index fc71f08d..56479b32 100644 --- a/packages/docs/src/content/docs/github.mdx +++ b/packages/docs/src/content/docs/github.mdx @@ -4,8 +4,9 @@ description: Run Warden automatically on pull requests. editUrl: false --- -Warden runs on pull requests through GitHub Actions and posts findings as -review comments. +Warden runs on pull requests through GitHub Actions and reports findings in +Checks. When findings can be tied to the current pull request diff, Warden also +posts inline review comments. This page explains the pull request behavior. Setup lives in [Repository Setup](/github/repository/), and action inputs live in @@ -16,10 +17,14 @@ This page explains the pull request behavior. Setup lives in 1. A pull request is opened or updated. 2. GitHub Actions runs the Warden workflow. 3. Warden analyzes changed files against configured triggers. -4. Findings are posted as inline review comments. +4. Findings are reported in Checks. Eligible findings are also posted as inline review comments. 5. If `requestChanges` is enabled, Warden requests changes when findings exceed `failOn`. 6. If `failCheck` is enabled, the check run fails when findings exceed `failOn`. +Warden skips PR review feedback when the workflow run is no longer analyzing +the current PR head. Non-blocking findings without an inline diff location stay +in Checks instead of creating timeline comments that cannot be resolved later. + ## What Comes From `warden.toml` Pull request behavior is still driven by `warden.toml`: diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index 4c464940..9689b1d7 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -231,11 +231,43 @@ describe('postTriggerReview', () => { pull_number: 1, commit_id: 'abc123', event: 'COMMENT', - body: 'Test review', + body: '', comments: [expect.objectContaining({ path: 'test.ts', line: 10, side: 'RIGHT', body: 'Test comment' })], }); }); + it('skips body-only non-blocking reviews', async () => { + const finding = createFinding({ location: undefined }); + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + report: { + skill: 'test-skill', + summary: 'Found 1 issue', + findings: [finding], + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'COMMENT', + body: 'Locationless finding', + comments: [], + }, + }), + reportOn: 'low', + }; + + const postResult = await postTriggerReview({ + result, + existingComments: [], + apiKey: 'test-key', + }, mockDeps); + + expect(postResult.posted).toBe(false); + expect(postResult.findingObservations).toEqual([]); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + }); + it('deduplicates findings against existing comments', async () => { const finding = createFinding(); const result: TriggerResult = { @@ -612,7 +644,7 @@ describe('postTriggerReview', () => { expect(postResult.shouldFail).toBe(false); }); - it('retries with findings in body when GitHub returns line resolution error', async () => { + it('does not leave body-only comments when GitHub returns line resolution error', async () => { const finding = createFinding(); const result: TriggerResult = { triggerName: 'test-trigger', @@ -635,10 +667,10 @@ describe('postTriggerReview', () => { vi.mocked(findingToExistingComment).mockReturnValue(createExistingComment()); - // First call fails with line resolution error, second succeeds + // First call fails with line resolution error. The fallback is body-only, + // so Warden should rely on checks instead of leaving an unresolvable review. vi.mocked(mockOctokit.pulls.createReview) - .mockRejectedValueOnce(new Error('Validation Failed: pull_request_review_thread.line does not form part of the diff')) - .mockResolvedValueOnce({} as never); + .mockRejectedValueOnce(new Error('Validation Failed: pull_request_review_thread.line does not form part of the diff')); const ctx: ReviewPostingContext = { result, @@ -648,12 +680,49 @@ describe('postTriggerReview', () => { const postResult = await postTriggerReview(ctx, mockDeps); + expect(postResult.posted).toBe(false); + expect(postResult.newComments).toHaveLength(0); + expect(postResult.findingObservations).toEqual([]); + expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); + }); + + it('still posts body-only request changes reviews', async () => { + const finding = createFinding({ severity: 'high' }); + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + report: { + skill: 'test-skill', + summary: 'Found 1 issue', + findings: [finding], + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'REQUEST_CHANGES', + body: 'Blocking finding in body', + comments: [], + }, + }), + reportOn: 'low', + requestChanges: true, + failOn: 'high', + }; + + const postResult = await postTriggerReview({ + result, + existingComments: [], + apiKey: 'test-key', + }, mockDeps); + expect(postResult.posted).toBe(true); - expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(2); - // Second call should have no inline comments and findings in body - const secondCall = vi.mocked(mockOctokit.pulls.createReview).mock.calls[1]![0]!; - expect(secondCall.comments).toEqual([]); - expect(secondCall.body).toBe('rendered findings body'); + expect(mockOctokit.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'REQUEST_CHANGES', + body: 'Blocking finding in body', + comments: [], + }) + ); }); it('does not retry on non-line-resolution errors', async () => { diff --git a/packages/warden/src/action/review/poster.ts b/packages/warden/src/action/review/poster.ts index 29d00774..86590471 100644 --- a/packages/warden/src/action/review/poster.ts +++ b/packages/warden/src/action/review/poster.ts @@ -129,15 +129,13 @@ async function postReviewToGitHub( octokit: Octokit, context: EventContext, result: RenderResult -): Promise { +): Promise { if (!context.pullRequest) { - return; + return false; } - // Only post PR reviews with inline comments - skip standalone summary comments - // as they add noise without providing actionable inline feedback if (!result.review) { - return; + return false; } const { owner, name: repo } = context.repository; @@ -155,15 +153,23 @@ async function postReviewToGitHub( start_side: c.start_line ? c.start_side ?? ('RIGHT' as const) : undefined, })); + // Non-blocking body-only reviews cannot be resolved as review threads. + // Keep those findings in Checks instead of leaving stale PR timeline entries. + if (reviewComments.length === 0 && result.review.event === 'COMMENT') { + return false; + } + await octokit.pulls.createReview({ owner, repo, pull_number: pullNumber, commit_id: commitId, event: result.review.event, - body: result.review.body, + body: result.review.event === 'COMMENT' ? '' : result.review.body, comments: reviewComments, }); + + return true; } /** @@ -399,15 +405,23 @@ export async function postTriggerReview( }); } + let posted = false; try { - await postReviewToGitHub(octokit, context, renderResultToPost); + posted = await postReviewToGitHub(octokit, context, renderResultToPost); } catch (error) { if (!isLineResolutionError(error)) { throw error; } - warnAction(`Inline comments failed for ${result.triggerName}, posting findings in review body`); - const fallback = moveCommentsToBody(renderResultToPost, postedFindings, skill); - await postReviewToGitHub(octokit, context, fallback); + if (renderResultToPost.review?.event === 'REQUEST_CHANGES') { + warnAction(`Inline comments failed for ${result.triggerName}, posting findings in review body`); + const fallback = moveCommentsToBody(renderResultToPost, postedFindings, skill); + posted = await postReviewToGitHub(octokit, context, fallback); + } else { + warnAction(`Inline comments failed for ${result.triggerName}, falling back to checks only`); + } + } + if (!posted) { + return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); } for (const finding of postedFindings) { findingObservations.push({ outcome: 'posted', finding, skill }); diff --git a/packages/warden/src/action/workflow/pr-workflow.test.ts b/packages/warden/src/action/workflow/pr-workflow.test.ts index bb7d2cf0..a6be2c21 100644 --- a/packages/warden/src/action/workflow/pr-workflow.test.ts +++ b/packages/warden/src/action/workflow/pr-workflow.test.ts @@ -134,6 +134,7 @@ const mockSetFailed = vi.mocked(setFailed); const mockWriteFindingsOutput = vi.mocked(writeFindingsOutput); // Type helper for mocking Octokit responses +type GetPullResponse = Awaited>; type ListReviewsResponse = Awaited>; // ----------------------------------------------------------------------------- @@ -150,6 +151,14 @@ interface MockOctokitOptions { }[]; } +function createGetPullResponse(headSha = PR_HEAD_SHA): GetPullResponse { + return { + data: { + head: { sha: headSha }, + }, + } as GetPullResponse; +} + function createMockOctokit(options: MockOctokitOptions = {}): Octokit { const defaultFiles = [ { @@ -167,6 +176,7 @@ function createMockOctokit(options: MockOctokitOptions = {}): Octokit { return { paginate: vi.fn(() => Promise.resolve(files)), pulls: { + get: vi.fn(() => Promise.resolve(createGetPullResponse())), listFiles: vi.fn(), listReviews: vi.fn(() => Promise.resolve({ data: [] })), createReview: vi.fn(() => Promise.resolve({ data: {} })), @@ -1140,6 +1150,99 @@ describe('runPRWorkflow', () => { ); }); + it('does not publish review feedback when the PR head has advanced', async () => { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + vi.mocked(mockOctokit.pulls.get).mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ + data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], + } as ListReviewsResponse); + + mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockOctokit.pulls.get).toHaveBeenCalledWith({ + owner: 'test-owner', + repo: 'test-repo', + pull_number: 123, + }); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + expect(mockFetchExistingComments).not.toHaveBeenCalled(); + expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); + expect(mockOctokit.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + conclusion: 'neutral', + output: expect.objectContaining({ + title: '1 issue', + }), + }) + ); + }); + + it('stops review feedback if the PR head advances before posting', async () => { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + vi.mocked(mockOctokit.pulls.get) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + + mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockOctokit.pulls.get).toHaveBeenCalledTimes(2); + expect(mockFetchExistingComments).toHaveBeenCalled(); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + }); + + it('stops stale resolution and dismissal if the PR head advances after posting', async () => { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + vi.mocked(mockOctokit.pulls.get) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValue(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ + data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], + } as ListReviewsResponse); + + mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow(mockOctokit, createDefaultInputs({ failOn: 'high' }), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); + expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + }); + + it('keeps findings in checks when inline review comments cannot resolve', async () => { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + vi.mocked(mockOctokit.pulls.createReview).mockRejectedValueOnce( + new Error('Validation Failed: pull_request_review_thread.line does not form part of the diff') + ); + + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); + expect(mockOctokit.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + output: expect.objectContaining({ + summary: expect.stringContaining('Test Finding'), + }), + }) + ); + }); + it('does not post review when no findings', async () => { mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report: createSkillReport({ findings: [] }) }); @@ -2071,6 +2174,74 @@ describe('runPRWorkflow', () => { expect(mockRunSkillTask).not.toHaveBeenCalled(); }); + it('skips cleanup when no triggers match and the PR head has advanced', async () => { + vi.mocked(mockOctokit.pulls.get).mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ + data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], + } as ListReviewsResponse); + mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); + + await runPRWorkflow( + mockOctokit, createDefaultInputs(), 'pull_request', + EVENT_PAYLOAD_PATH, NO_MATCH_FIXTURES_DIR + ); + + expect(mockFetchExistingComments).not.toHaveBeenCalled(); + expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + expect(mockRunSkillTask).not.toHaveBeenCalled(); + }); + + it('stops cleanup writes when the PR head advances after cleanup starts', async () => { + vi.mocked(mockOctokit.pulls.get) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ + data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], + } as ListReviewsResponse); + mockFetchExistingComments.mockResolvedValue([ + createExistingWardenComment({ + path: 'src/old-file.ts', + line: 5, + title: 'Bug', + description: 'Fix this', + contentHash: 'hash1', + }), + ]); + mockEvaluateFixAttempts.mockResolvedValue({ + toResolve: [{ + id: 1, + path: 'src/old-file.ts', + line: 5, + title: 'Bug', + description: 'Fix this', + contentHash: 'hash1', + isWarden: true, + isResolved: false, + threadId: 'thread-1', + }], + toReply: [], + evaluations: [], + skipped: 0, + evaluated: 1, + failedEvaluations: 0, + uniqueFindingsEvaluated: 1, + uniqueFindingsCodeChanged: 1, + uniqueFindingsResolved: 1, + usage: { inputTokens: 0, outputTokens: 0, costUSD: 0 }, + }); + + await runPRWorkflow( + mockOctokit, createDefaultInputs(), 'pull_request', + EVENT_PAYLOAD_PATH, NO_MATCH_FIXTURES_DIR + ); + + expect(mockEvaluateFixAttempts).toHaveBeenCalled(); + expect(mockOctokit.graphql).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + expect(mockRunSkillTask).not.toHaveBeenCalled(); + }); + it('normalizes empty auxiliary default before cleanup fix evaluation', async () => { mockFetchExistingComments.mockResolvedValue([ createExistingWardenComment({ diff --git a/packages/warden/src/action/workflow/pr-workflow.ts b/packages/warden/src/action/workflow/pr-workflow.ts index 7546b383..0cb93116 100644 --- a/packages/warden/src/action/workflow/pr-workflow.ts +++ b/packages/warden/src/action/workflow/pr-workflow.ts @@ -107,6 +107,7 @@ interface ReviewPhaseResult { existingComments: ExistingComment[]; activeWardenCommentIds: Set; findingObservations: FindingObservation[]; + reviewFeedbackWritable: boolean; shouldFailAction: boolean; failureReasons: string[]; } @@ -117,6 +118,8 @@ interface FixEvaluationCommentGroups { missingOriginalCommitCount: number; } +type ReviewFeedbackWriteGuard = () => Promise; + interface AuxiliaryWorkflowOptions { runtime?: RuntimeName; model?: string; @@ -520,11 +523,14 @@ async function postReviewsAndTrackFailures( auxiliaryOptions: AuxiliaryWorkflowOptions, options: { failOnPostError?: boolean } = {} ): Promise { - // Fetch existing comments for deduplication (only for PRs) + // Fetch existing comments only when this run can still mutate feedback on the + // current PR head. // Keep original list separate for stale detection (modified list includes newly posted comments) let fetchedComments: ExistingComment[] = []; let existingComments: ExistingComment[] = []; - if (context.pullRequest) { + const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); + let reviewFeedbackWritable = await canWriteReviewFeedback(); + if (reviewFeedbackWritable && context.pullRequest) { try { fetchedComments = await fetchExistingComments( octokit, @@ -558,23 +564,28 @@ async function postReviewsAndTrackFailures( reports.push(result.report); // Post review - const postResult = await postTriggerReview( - { - result, - existingComments, - apiKey: inputs.anthropicApiKey, - runtime: auxiliaryOptions.runtime, - model: auxiliaryOptions.model, - maxRetries: auxiliaryOptions.maxRetries, - failOnPostError: options.failOnPostError, - }, - { octokit, context } - ); + if (reviewFeedbackWritable) { + reviewFeedbackWritable = await canWriteReviewFeedback(); + if (reviewFeedbackWritable) { + const postResult = await postTriggerReview( + { + result, + existingComments, + apiKey: inputs.anthropicApiKey, + runtime: auxiliaryOptions.runtime, + model: auxiliaryOptions.model, + maxRetries: auxiliaryOptions.maxRetries, + failOnPostError: options.failOnPostError, + }, + { octokit, context } + ); - // Add newly posted comments to existing comments for cross-trigger deduplication - existingComments.push(...postResult.newComments); - postResult.activeWardenCommentIds.forEach((id) => activeWardenCommentIds.add(id)); - findingObservations.push(...postResult.findingObservations); + // Add newly posted comments to existing comments for cross-trigger deduplication + existingComments.push(...postResult.newComments); + postResult.activeWardenCommentIds.forEach((id) => activeWardenCommentIds.add(id)); + findingObservations.push(...postResult.findingObservations); + } + } // Check if we should fail based on this trigger's config // Filter by confidence first so low-confidence findings don't cause failure @@ -594,11 +605,41 @@ async function postReviewsAndTrackFailures( existingComments, activeWardenCommentIds, findingObservations, + reviewFeedbackWritable, shouldFailAction, failureReasons, }; } +async function canWriteReviewFeedbackForCurrentHead( + octokit: Octokit, + context: EventContext +): Promise { + if (!context.pullRequest) { + return false; + } + + try { + const { data } = await octokit.pulls.get({ + owner: context.repository.owner, + repo: context.repository.name, + pull_number: context.pullRequest.number, + }); + const currentHeadSha = data.head.sha; + if (currentHeadSha !== context.pullRequest.headSha) { + warnAction( + `Skipping PR review feedback because run head ${context.pullRequest.headSha} is no longer the PR head ${currentHeadSha}` + ); + return false; + } + return true; + } catch (error) { + Sentry.captureException(error, { tags: { operation: 'fetch_current_pr_head' } }); + warnAction(`Skipping PR review feedback because the current PR head could not be verified: ${error}`); + return false; + } +} + /** * Evaluate fix attempts on unresolved comments and resolve stale comments. * @@ -614,7 +655,10 @@ async function evaluateFixesAndResolveStale( canResolveStale: boolean, anthropicApiKey: string, auxiliaryOptions: AuxiliaryWorkflowOptions, - options: { failOnWriteError?: boolean } = {} + options: { + failOnWriteError?: boolean; + canWriteReviewFeedback?: ReviewFeedbackWriteGuard; + } = {} ): Promise<{ allResolved: boolean; autoResolvedByFixEvaluation: number; @@ -626,6 +670,13 @@ async function evaluateFixesAndResolveStale( const commentsEvaluatedByFixEval = new Set(); const commentsResolvedByStale = new Set(); const findingObservations: FindingObservation[] = []; + const canWriteReviewFeedback = options.canWriteReviewFeedback ?? (() => Promise.resolve(true)); + const blockedReviewFeedbackWriteResult = () => ({ + allResolved: false, + autoResolvedByFixEvaluation: commentsResolvedByFixEval.size, + autoResolvedByStaleCheck: commentsResolvedByStale.size, + findingObservations, + }); const commentsForFixEvaluation = wardenComments.filter( (c) => !activeWardenCommentIds.has(c.id) ); @@ -695,6 +746,11 @@ async function evaluateFixesAndResolveStale( // Resolve successful fixes if (fixEvaluation.toResolve.length > 0) { + if (!await canWriteReviewFeedback()) { + logGroupEnd(); + return blockedReviewFeedbackWriteResult(); + } + const { resolvedCount, resolvedIds } = await resolveStaleComments( octokit, fixEvaluation.toResolve, @@ -726,6 +782,10 @@ async function evaluateFixesAndResolveStale( commentsEvaluatedByFixEval.add(reply.comment.id); if (reply.comment.threadId) { try { + if (!await canWriteReviewFeedback()) { + logGroupEnd(); + return blockedReviewFeedbackWriteResult(); + } await postThreadReply(octokit, reply.comment.threadId, reply.replyBody); } catch (error) { Sentry.captureException(error, { tags: { operation: 'post_thread_reply' } }); @@ -775,6 +835,10 @@ async function evaluateFixesAndResolveStale( const staleComments = findStaleComments(commentsForStaleCheck, allFindings, scope); if (staleComments.length > 0) { + if (!await canWriteReviewFeedback()) { + return blockedReviewFeedbackWriteResult(); + } + const { resolvedCount, resolvedIds } = await resolveStaleComments( octokit, staleComments, @@ -865,6 +929,10 @@ async function dismissPreviousReviewIfResolved( !wouldRequestChanges && hasActiveFailOn ) { + if (!await canWriteReviewFeedbackForCurrentHead(octokit, context)) { + return; + } + try { await octokit.pulls.dismissReview({ owner: context.repository.owner, @@ -1533,6 +1601,7 @@ async function finalizeReportWorkflow( * Runs fix evaluation and stale resolution on existing comments so that * comments from earlier pushes get resolved even when the current push * only touches files outside all skills' paths filters. + * Skips cleanup when this run is no longer analyzing the current PR head. */ async function cleanupOrphanedComments( octokit: Octokit, @@ -1545,6 +1614,12 @@ async function cleanupOrphanedComments( return []; } + const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); + + if (!await canWriteReviewFeedback()) { + return []; + } + let existingComments: ExistingComment[]; try { existingComments = await fetchExistingComments( @@ -1573,6 +1648,7 @@ async function cleanupOrphanedComments( await evaluateFixesAndResolveStale( octokit, context, existingComments, [], new Set(), true, inputs.anthropicApiKey, auxiliaryOptions, { failOnWriteError: options.failOnWriteError, + canWriteReviewFeedback, } ); const activeSpan = Sentry.getActiveSpan(); @@ -1583,6 +1659,10 @@ async function cleanupOrphanedComments( if (allResolved) { const previousReviewInfo = await fetchPreviousReviewInfo(octokit, context); if (previousReviewInfo?.state === 'CHANGES_REQUESTED') { + if (!await canWriteReviewFeedback()) { + return findingObservations; + } + try { await octokit.pulls.dismissReview({ owner: context.repository.owner, @@ -1771,16 +1851,24 @@ async function runReportMode( canResolveStale = shouldResolveStaleComments(results); const allFindings = reviewPhase.reports.flatMap((r) => r.findings); span.setAttribute('warden.finding.count', allFindings.length); + const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); await Sentry.startSpan( { op: 'workflow.resolve', name: 'resolve stale comments' }, async (resolveSpan) => { + const canMutateReviewFeedback = + canResolveStale && + reviewPhase.reviewFeedbackWritable && + await canWriteReviewFeedback(); const resolutionResult = await evaluateFixesAndResolveStale( octokit, context, reviewPhase.fetchedComments, allFindings, reviewPhase.activeWardenCommentIds, - canResolveStale, inputs.anthropicApiKey, + canMutateReviewFeedback, inputs.anthropicApiKey, auxiliaryOptions, - { failOnWriteError: true }, + { + failOnWriteError: true, + canWriteReviewFeedback, + }, ); resolveSpan.setAttribute( 'warden.feedback.auto_resolve.fix_eval_count', @@ -1794,12 +1882,17 @@ async function runReportMode( }, ); + const canDismissReview = + canResolveStale && + reviewPhase.reviewFeedbackWritable && + await canWriteReviewFeedback(); + await finalizeReportWorkflow( octokit, context, previousReviewInfo, results, reviewPhase.reports, reviewPhase.findingObservations, reviewPhase.shouldFailAction, reviewPhase.failureReasons, - canResolveStale, + canDismissReview, triggerErrors, { failOnWriteError: true }, ); @@ -1954,6 +2047,7 @@ export async function runPRWorkflow( const canResolveStale = shouldResolveStaleComments(results); const allFindings = reviewPhase.reports.flatMap((r) => r.findings); span.setAttribute('warden.finding.count', allFindings.length); + const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); await runOrFailCore( octokit, @@ -1962,11 +2056,16 @@ export async function runPRWorkflow( () => Sentry.startSpan( { op: 'workflow.resolve', name: 'resolve stale comments' }, async (resolveSpan) => { + const canMutateReviewFeedback = + canResolveStale && + reviewPhase.reviewFeedbackWritable && + await canWriteReviewFeedback(); const resolutionResult = await evaluateFixesAndResolveStale( octokit, context, reviewPhase.fetchedComments, allFindings, reviewPhase.activeWardenCommentIds, - canResolveStale, inputs.anthropicApiKey, + canMutateReviewFeedback, inputs.anthropicApiKey, auxiliaryOptions, + { canWriteReviewFeedback }, ); resolveSpan.setAttribute( 'warden.feedback.auto_resolve.fix_eval_count', @@ -1981,12 +2080,17 @@ export async function runPRWorkflow( ), ); + const canDismissReview = + canResolveStale && + reviewPhase.reviewFeedbackWritable && + await canWriteReviewFeedback(); + await finalizeWorkflow( octokit, context, previousReviewInfo, coreCheckId, results, reviewPhase.reports, reviewPhase.findingObservations, reviewPhase.shouldFailAction, reviewPhase.failureReasons, - canResolveStale, + canDismissReview, triggerErrors, ); From a28646823da6ac2921987b2182d63525783a3295 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 17:12:50 -0700 Subject: [PATCH 2/7] fix(warden): Harden the stale PR feedback guard Centralize the head-freshness check in a ReviewFeedbackGate that retries transient GitHub errors, memoizes results briefly, and distinguishes a stale head (skip silently; the newer run owns feedback) from an unverifiable one (skip the write, and fail the run when that would suppress a blocking REQUEST_CHANGES review). Previously a single transient pulls.get failure silently disabled all review feedback for the run, dropping the merge gate for requestChanges-only configs, and every guard call was a separate uncached API request. The gate replaces the optional fail-open guard callback in evaluateFixesAndResolveStale, which now verifies the head before spending LLM tokens on fix evaluation and no longer re-checks per thread reply. Callers pass canResolveStale through unchanged, so the 'skipping due to trigger failures' log fires only for trigger failures and head staleness gets its own log line. Findings that never reach the PR now get an honest outcome: locationless findings stripped from COMMENT review bodies and inline comments GitHub rejects are recorded as skipped with the new no_inline_location reason instead of being marked posted or dropped from the observation stream. Co-Authored-By: Claude Fable 5 --- packages/docs/public/llms.txt | 2 +- packages/docs/src/content/docs/github.mdx | 6 +- .../warden/src/action/reporting/outcomes.ts | 4 +- .../warden/src/action/review/poster.test.ts | 48 +++- packages/warden/src/action/review/poster.ts | 41 +++- .../src/action/workflow/pr-workflow.test.ts | 207 +++++++++++------ .../warden/src/action/workflow/pr-workflow.ts | 208 +++++++++--------- .../workflow/review-feedback-gate.test.ts | 134 +++++++++++ .../action/workflow/review-feedback-gate.ts | 98 +++++++++ 9 files changed, 554 insertions(+), 194 deletions(-) create mode 100644 packages/warden/src/action/workflow/review-feedback-gate.test.ts create mode 100644 packages/warden/src/action/workflow/review-feedback-gate.ts diff --git a/packages/docs/public/llms.txt b/packages/docs/public/llms.txt index 6d8d2cdf..db6be7eb 100644 --- a/packages/docs/public/llms.txt +++ b/packages/docs/public/llms.txt @@ -550,7 +550,7 @@ Creates `warden.toml` and `.github/workflows/warden.yml`. 5. If `requestChanges` is enabled, the review requests changes when findings exceed `failOn` 6. If `failCheck` is enabled, the check run fails when findings exceed `failOn` -Warden skips PR review feedback when the workflow run is no longer analyzing the current PR head. Non-blocking findings without an inline diff location stay in Checks instead of creating timeline comments that cannot be resolved later. +Warden skips PR review feedback when the workflow run is no longer analyzing the current PR head. If the current head cannot be verified, Warden skips the write and fails the run rather than silently dropping a blocking review. Non-blocking findings without an inline diff location stay in Checks instead of creating timeline comments that cannot be resolved later. ### GitHub Actions Workflow diff --git a/packages/docs/src/content/docs/github.mdx b/packages/docs/src/content/docs/github.mdx index 56479b32..85c4c4b4 100644 --- a/packages/docs/src/content/docs/github.mdx +++ b/packages/docs/src/content/docs/github.mdx @@ -22,8 +22,10 @@ This page explains the pull request behavior. Setup lives in 6. If `failCheck` is enabled, the check run fails when findings exceed `failOn`. Warden skips PR review feedback when the workflow run is no longer analyzing -the current PR head. Non-blocking findings without an inline diff location stay -in Checks instead of creating timeline comments that cannot be resolved later. +the current PR head. If the current head cannot be verified, Warden skips the +write and fails the run rather than silently dropping a blocking review. +Non-blocking findings without an inline diff location stay in Checks instead +of creating timeline comments that cannot be resolved later. ## What Comes From `warden.toml` diff --git a/packages/warden/src/action/reporting/outcomes.ts b/packages/warden/src/action/reporting/outcomes.ts index 08473539..2250ac08 100644 --- a/packages/warden/src/action/reporting/outcomes.ts +++ b/packages/warden/src/action/reporting/outcomes.ts @@ -11,7 +11,7 @@ export type FindingOutcome = export type DedupeSource = 'warden' | 'external'; export type DedupeMatchType = 'hash' | 'semantic'; -export type SkippedReason = 'max_findings' | 'duplicate_in_batch'; +export type SkippedReason = 'max_findings' | 'duplicate_in_batch' | 'no_inline_location'; export type ResolvedReason = 'fix_evaluation' | 'stale_check'; export const DedupeDetailSchema = z.object({ @@ -77,7 +77,7 @@ export const FindingObservationSchema = z.discriminatedUnion('outcome', [ outcome: z.literal('skipped'), finding: FindingSchema, skill: z.string().optional(), - skippedReason: z.enum(['max_findings', 'duplicate_in_batch']), + skippedReason: z.enum(['max_findings', 'duplicate_in_batch', 'no_inline_location']), }), z.object({ outcome: z.literal('resolved'), diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index 9689b1d7..1906ec79 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -264,10 +264,52 @@ describe('postTriggerReview', () => { }, mockDeps); expect(postResult.posted).toBe(false); - expect(postResult.findingObservations).toEqual([]); + expect(postResult.findingObservations).toEqual([ + { outcome: 'skipped', finding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + ]); expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); }); + it('marks locationless findings in a mixed review as checks-only instead of posted', async () => { + const inlineFinding = createFinding(); + const bodyFinding = createFinding({ id: 'test-2', title: 'Locationless finding', location: undefined }); + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + report: { + skill: 'test-skill', + summary: 'Found 2 issues', + findings: [inlineFinding, bodyFinding], + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'COMMENT', + body: 'Locationless finding body', + comments: [{ path: 'test.ts', line: 10, body: 'Test comment' }], + }, + }), + reportOn: 'low', + }; + + vi.mocked(findingToExistingComment).mockReturnValue(createExistingComment()); + + const postResult = await postTriggerReview({ + result, + existingComments: [], + apiKey: 'test-key', + }, mockDeps); + + expect(postResult.posted).toBe(true); + expect(mockOctokit.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ event: 'COMMENT', body: '' }) + ); + expect(postResult.findingObservations).toEqual([ + { outcome: 'posted', finding: inlineFinding, skill: 'test-skill' }, + { outcome: 'skipped', finding: bodyFinding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + ]); + }); + it('deduplicates findings against existing comments', async () => { const finding = createFinding(); const result: TriggerResult = { @@ -682,7 +724,9 @@ describe('postTriggerReview', () => { expect(postResult.posted).toBe(false); expect(postResult.newComments).toHaveLength(0); - expect(postResult.findingObservations).toEqual([]); + expect(postResult.findingObservations).toEqual([ + { outcome: 'skipped', finding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + ]); expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); }); diff --git a/packages/warden/src/action/review/poster.ts b/packages/warden/src/action/review/poster.ts index 86590471..05196659 100644 --- a/packages/warden/src/action/review/poster.ts +++ b/packages/warden/src/action/review/poster.ts @@ -122,6 +122,14 @@ function recenterReportFindingIds(reportFindings: Finding[], actions: Deduplicat // GitHub Review Posting // ----------------------------------------------------------------------------- +/** + * How a review post attempt ended: + * - `posted`: the review was created on the PR + * - `checks_only`: findings could not be attached inline; they stay in Checks + * - `no_review`: nothing to post (no PR context or no rendered review) + */ +type PostReviewOutcome = 'posted' | 'checks_only' | 'no_review'; + /** * Post a PR review to GitHub. */ @@ -129,13 +137,13 @@ async function postReviewToGitHub( octokit: Octokit, context: EventContext, result: RenderResult -): Promise { +): Promise { if (!context.pullRequest) { - return false; + return 'no_review'; } if (!result.review) { - return false; + return 'no_review'; } const { owner, name: repo } = context.repository; @@ -156,7 +164,7 @@ async function postReviewToGitHub( // Non-blocking body-only reviews cannot be resolved as review threads. // Keep those findings in Checks instead of leaving stale PR timeline entries. if (reviewComments.length === 0 && result.review.event === 'COMMENT') { - return false; + return 'checks_only'; } await octokit.pulls.createReview({ @@ -169,7 +177,7 @@ async function postReviewToGitHub( comments: reviewComments, }); - return true; + return 'posted'; } /** @@ -405,9 +413,9 @@ export async function postTriggerReview( }); } - let posted = false; + let postOutcome: PostReviewOutcome = 'no_review'; try { - posted = await postReviewToGitHub(octokit, context, renderResultToPost); + postOutcome = await postReviewToGitHub(octokit, context, renderResultToPost); } catch (error) { if (!isLineResolutionError(error)) { throw error; @@ -415,15 +423,30 @@ export async function postTriggerReview( if (renderResultToPost.review?.event === 'REQUEST_CHANGES') { warnAction(`Inline comments failed for ${result.triggerName}, posting findings in review body`); const fallback = moveCommentsToBody(renderResultToPost, postedFindings, skill); - posted = await postReviewToGitHub(octokit, context, fallback); + postOutcome = await postReviewToGitHub(octokit, context, fallback); } else { warnAction(`Inline comments failed for ${result.triggerName}, falling back to checks only`); + postOutcome = 'checks_only'; } } - if (!posted) { + if (postOutcome === 'checks_only') { + for (const finding of postedFindings) { + findingObservations.push({ outcome: 'skipped', finding, skill, skippedReason: 'no_inline_location' }); + } return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); } + if (postOutcome !== 'posted') { + return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); + } + // COMMENT reviews post with an empty body, so locationless findings that + // the renderer placed in the body never reach the PR. Record them as + // checks-only instead of claiming they were posted. + const bodyStripped = renderResultToPost.review?.event === 'COMMENT'; for (const finding of postedFindings) { + if (bodyStripped && !finding.location) { + findingObservations.push({ outcome: 'skipped', finding, skill, skippedReason: 'no_inline_location' }); + continue; + } findingObservations.push({ outcome: 'posted', finding, skill }); const comment = findingToExistingComment(finding, skill); if (comment) { diff --git a/packages/warden/src/action/workflow/pr-workflow.test.ts b/packages/warden/src/action/workflow/pr-workflow.test.ts index a6be2c21..52a988ea 100644 --- a/packages/warden/src/action/workflow/pr-workflow.test.ts +++ b/packages/warden/src/action/workflow/pr-workflow.test.ts @@ -1183,43 +1183,103 @@ describe('runPRWorkflow', () => { }); it('stops review feedback if the PR head advances before posting', async () => { + // The gate memoizes head checks briefly; advance the clock past the TTL + // inside the comment fetch so the pre-post check re-verifies the head. + let now = 1_750_000_000_000; + const dateNowSpy = vi.spyOn(Date, 'now').mockImplementation(() => now); + try { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + vi.mocked(mockOctokit.pulls.get) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + + mockFetchExistingComments.mockImplementation(async () => { + now += 60_000; + return [createExistingWardenComment()]; + }); + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockOctokit.pulls.get).toHaveBeenCalledTimes(2); + expect(mockFetchExistingComments).toHaveBeenCalled(); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + } finally { + dateNowSpy.mockRestore(); + } + }); + + it('stops stale resolution and dismissal if the PR head advances after posting', async () => { + // Advance the clock past the gate TTL inside the review post so the + // resolve phase re-verifies the head and sees it advanced. + let now = 1_750_000_000_000; + const dateNowSpy = vi.spyOn(Date, 'now').mockImplementation(() => now); + try { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + vi.mocked(mockOctokit.pulls.get) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValue(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ + data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], + } as ListReviewsResponse); + vi.mocked(mockOctokit.pulls.createReview).mockImplementation(async () => { + now += 60_000; + return { data: {} } as never; + }); + + mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow(mockOctokit, createDefaultInputs({ failOn: 'high' }), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); + expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + } finally { + dateNowSpy.mockRestore(); + } + }); + + it('fails the run when a blocking review is skipped because the head cannot be verified', async () => { const finding = createFinding(); const report = createSkillReport({ findings: [finding] }); - vi.mocked(mockOctokit.pulls.get) - .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) - .mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.get).mockRejectedValue(new Error('GitHub is unavailable')); - mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); - await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + await expect( + runPRWorkflow( + mockOctokit, + createDefaultInputs({ failOn: 'high', requestChanges: true }), + 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR + ) + ).rejects.toThrow('Could not verify the PR head; blocking review was not posted'); - expect(mockOctokit.pulls.get).toHaveBeenCalledTimes(2); - expect(mockFetchExistingComments).toHaveBeenCalled(); expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); - expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); - expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); }); - it('stops stale resolution and dismissal if the PR head advances after posting', async () => { - const finding = createFinding(); + it('does not fail an unverifiable run when the blocking review would not have been posted', async () => { + // reportOn stricter than failOn: the render result is REQUEST_CHANGES but + // the poster would never post it (no reportable findings), so an + // unverifiable head must not fail the run. + const finding = createFinding({ severity: 'medium' }); const report = createSkillReport({ findings: [finding] }); - vi.mocked(mockOctokit.pulls.get) - .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) - .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) - .mockResolvedValue(createGetPullResponse('new-head-sha')); - vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ - data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], - } as ListReviewsResponse); + vi.mocked(mockOctokit.pulls.get).mockRejectedValue(new Error('GitHub is unavailable')); - mockFetchExistingComments.mockResolvedValue([createExistingWardenComment()]); mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); - await runPRWorkflow(mockOctokit, createDefaultInputs({ failOn: 'high' }), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + await runPRWorkflow( + mockOctokit, + createDefaultInputs({ failOn: 'medium', reportOn: 'high', requestChanges: true }), + 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR + ); - expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); - expect(mockEvaluateFixAttempts).not.toHaveBeenCalled(); - expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(mockSetFailed).not.toHaveBeenCalled(); }); it('keeps findings in checks when inline review comments cannot resolve', async () => { @@ -2193,53 +2253,64 @@ describe('runPRWorkflow', () => { }); it('stops cleanup writes when the PR head advances after cleanup starts', async () => { - vi.mocked(mockOctokit.pulls.get) - .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) - .mockResolvedValueOnce(createGetPullResponse('new-head-sha')); - vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ - data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], - } as ListReviewsResponse); - mockFetchExistingComments.mockResolvedValue([ - createExistingWardenComment({ - path: 'src/old-file.ts', - line: 5, - title: 'Bug', - description: 'Fix this', - contentHash: 'hash1', - }), - ]); - mockEvaluateFixAttempts.mockResolvedValue({ - toResolve: [{ - id: 1, - path: 'src/old-file.ts', - line: 5, - title: 'Bug', - description: 'Fix this', - contentHash: 'hash1', - isWarden: true, - isResolved: false, - threadId: 'thread-1', - }], - toReply: [], - evaluations: [], - skipped: 0, - evaluated: 1, - failedEvaluations: 0, - uniqueFindingsEvaluated: 1, - uniqueFindingsCodeChanged: 1, - uniqueFindingsResolved: 1, - usage: { inputTokens: 0, outputTokens: 0, costUSD: 0 }, - }); + // Advance the clock past the gate TTL inside fix evaluation so the + // pre-write check re-verifies the head and sees it advanced. + let now = 1_750_000_000_000; + const dateNowSpy = vi.spyOn(Date, 'now').mockImplementation(() => now); + try { + vi.mocked(mockOctokit.pulls.get) + .mockResolvedValueOnce(createGetPullResponse(PR_HEAD_SHA)) + .mockResolvedValueOnce(createGetPullResponse('new-head-sha')); + vi.mocked(mockOctokit.pulls.listReviews).mockResolvedValue({ + data: [{ id: 42, state: 'CHANGES_REQUESTED', user: { login: 'warden[bot]' } }], + } as ListReviewsResponse); + mockFetchExistingComments.mockResolvedValue([ + createExistingWardenComment({ + path: 'src/old-file.ts', + line: 5, + title: 'Bug', + description: 'Fix this', + contentHash: 'hash1', + }), + ]); + mockEvaluateFixAttempts.mockImplementation(async () => { + now += 60_000; + return { + toResolve: [{ + id: 1, + path: 'src/old-file.ts', + line: 5, + title: 'Bug', + description: 'Fix this', + contentHash: 'hash1', + isWarden: true, + isResolved: false, + threadId: 'thread-1', + }], + toReply: [], + evaluations: [], + skipped: 0, + evaluated: 1, + failedEvaluations: 0, + uniqueFindingsEvaluated: 1, + uniqueFindingsCodeChanged: 1, + uniqueFindingsResolved: 1, + usage: { inputTokens: 0, outputTokens: 0, costUSD: 0 }, + }; + }); - await runPRWorkflow( - mockOctokit, createDefaultInputs(), 'pull_request', - EVENT_PAYLOAD_PATH, NO_MATCH_FIXTURES_DIR - ); + await runPRWorkflow( + mockOctokit, createDefaultInputs(), 'pull_request', + EVENT_PAYLOAD_PATH, NO_MATCH_FIXTURES_DIR + ); - expect(mockEvaluateFixAttempts).toHaveBeenCalled(); - expect(mockOctokit.graphql).not.toHaveBeenCalled(); - expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); - expect(mockRunSkillTask).not.toHaveBeenCalled(); + expect(mockEvaluateFixAttempts).toHaveBeenCalled(); + expect(mockOctokit.graphql).not.toHaveBeenCalled(); + expect(mockOctokit.pulls.dismissReview).not.toHaveBeenCalled(); + expect(mockRunSkillTask).not.toHaveBeenCalled(); + } finally { + dateNowSpy.mockRestore(); + } }); it('normalizes empty auxiliary default before cleanup fix evaluation', async () => { diff --git a/packages/warden/src/action/workflow/pr-workflow.ts b/packages/warden/src/action/workflow/pr-workflow.ts index 0cb93116..17b9c646 100644 --- a/packages/warden/src/action/workflow/pr-workflow.ts +++ b/packages/warden/src/action/workflow/pr-workflow.ts @@ -43,6 +43,8 @@ import { executeTrigger } from '../triggers/executor.js'; import type { TriggerCheckReporter, TriggerResult } from '../triggers/executor.js'; import { postTriggerReview } from '../review/poster.js'; import { shouldResolveStaleComments } from '../review/coordination.js'; +import { ReviewFeedbackGate } from './review-feedback-gate.js'; +import type { ReviewFeedbackWritability } from './review-feedback-gate.js'; import type { FindingObservation } from '../reporting/outcomes.js'; import type { RuntimeName } from '../../sdk/runtimes/index.js'; import { canUseRuntimeAuth } from '../../sdk/extract.js'; @@ -107,7 +109,6 @@ interface ReviewPhaseResult { existingComments: ExistingComment[]; activeWardenCommentIds: Set; findingObservations: FindingObservation[]; - reviewFeedbackWritable: boolean; shouldFailAction: boolean; failureReasons: string[]; } @@ -118,8 +119,6 @@ interface FixEvaluationCommentGroups { missingOriginalCommitCount: number; } -type ReviewFeedbackWriteGuard = () => Promise; - interface AuxiliaryWorkflowOptions { runtime?: RuntimeName; model?: string; @@ -521,16 +520,17 @@ async function postReviewsAndTrackFailures( results: TriggerResult[], inputs: ActionInputs, auxiliaryOptions: AuxiliaryWorkflowOptions, + gate: ReviewFeedbackGate, options: { failOnPostError?: boolean } = {} ): Promise { - // Fetch existing comments only when this run can still mutate feedback on the - // current PR head. + // Skip the comment fetch only when the head has definitively advanced; on an + // unverifiable head the fetch is a harmless read and keeps later phases able + // to resolve comments once the API recovers. // Keep original list separate for stale detection (modified list includes newly posted comments) let fetchedComments: ExistingComment[] = []; let existingComments: ExistingComment[] = []; - const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); - let reviewFeedbackWritable = await canWriteReviewFeedback(); - if (reviewFeedbackWritable && context.pullRequest) { + let writability = await gate.check(); + if (writability !== 'blocked' && context.pullRequest) { try { fetchedComments = await fetchExistingComments( octokit, @@ -563,28 +563,36 @@ async function postReviewsAndTrackFailures( if (result.report) { reports.push(result.report); - // Post review - if (reviewFeedbackWritable) { - reviewFeedbackWritable = await canWriteReviewFeedback(); - if (reviewFeedbackWritable) { - const postResult = await postTriggerReview( - { - result, - existingComments, - apiKey: inputs.anthropicApiKey, - runtime: auxiliaryOptions.runtime, - model: auxiliaryOptions.model, - maxRetries: auxiliaryOptions.maxRetries, - failOnPostError: options.failOnPostError, - }, - { octokit, context } - ); + // Post review. The gate memoizes briefly, so this stays cheap between + // writes but re-verifies after slow phases (LLM dedup, consolidation). + if (writability !== 'blocked') { + writability = await gate.check(); + } + if (writability === 'writable') { + const postResult = await postTriggerReview( + { + result, + existingComments, + apiKey: inputs.anthropicApiKey, + runtime: auxiliaryOptions.runtime, + model: auxiliaryOptions.model, + maxRetries: auxiliaryOptions.maxRetries, + failOnPostError: options.failOnPostError, + }, + { octokit, context } + ); - // Add newly posted comments to existing comments for cross-trigger deduplication - existingComments.push(...postResult.newComments); - postResult.activeWardenCommentIds.forEach((id) => activeWardenCommentIds.add(id)); - findingObservations.push(...postResult.findingObservations); - } + // Add newly posted comments to existing comments for cross-trigger deduplication + existingComments.push(...postResult.newComments); + postResult.activeWardenCommentIds.forEach((id) => activeWardenCommentIds.add(id)); + findingObservations.push(...postResult.findingObservations); + } else if (writability === 'unknown' && wouldPostBlockingReview(result)) { + // A stale head skips silently (the newer run owns feedback), but an + // unverifiable head must not silently swallow a blocking review. + shouldFailAction = true; + failureReasons.push( + `${result.triggerName}: Could not verify the PR head; blocking review was not posted` + ); } // Check if we should fail based on this trigger's config @@ -605,39 +613,23 @@ async function postReviewsAndTrackFailures( existingComments, activeWardenCommentIds, findingObservations, - reviewFeedbackWritable, shouldFailAction, failureReasons, }; } -async function canWriteReviewFeedbackForCurrentHead( - octokit: Octokit, - context: EventContext -): Promise { - if (!context.pullRequest) { - return false; - } - - try { - const { data } = await octokit.pulls.get({ - owner: context.repository.owner, - repo: context.repository.name, - pull_number: context.pullRequest.number, - }); - const currentHeadSha = data.head.sha; - if (currentHeadSha !== context.pullRequest.headSha) { - warnAction( - `Skipping PR review feedback because run head ${context.pullRequest.headSha} is no longer the PR head ${currentHeadSha}` - ); - return false; - } - return true; - } catch (error) { - Sentry.captureException(error, { tags: { operation: 'fetch_current_pr_head' } }); - warnAction(`Skipping PR review feedback because the current PR head could not be verified: ${error}`); +/** + * Whether posting this trigger's review would produce a blocking + * REQUEST_CHANGES review. Mirrors the poster's posting predicate: the + * renderer can emit a REQUEST_CHANGES render result with zero reportable + * findings (reportOn stricter than failOn), which the poster never posts. + */ +function wouldPostBlockingReview(result: TriggerResult): boolean { + if (!result.report || result.renderResult?.review?.event !== 'REQUEST_CHANGES') { return false; } + const filteredFindings = filterFindings(result.report.findings, result.reportOn, result.minConfidence); + return filteredFindings.length > 0 || (result.reportOnSuccess ?? false); } /** @@ -655,10 +647,8 @@ async function evaluateFixesAndResolveStale( canResolveStale: boolean, anthropicApiKey: string, auxiliaryOptions: AuxiliaryWorkflowOptions, - options: { - failOnWriteError?: boolean; - canWriteReviewFeedback?: ReviewFeedbackWriteGuard; - } = {} + gate: ReviewFeedbackGate, + options: { failOnWriteError?: boolean } = {} ): Promise<{ allResolved: boolean; autoResolvedByFixEvaluation: number; @@ -670,7 +660,6 @@ async function evaluateFixesAndResolveStale( const commentsEvaluatedByFixEval = new Set(); const commentsResolvedByStale = new Set(); const findingObservations: FindingObservation[] = []; - const canWriteReviewFeedback = options.canWriteReviewFeedback ?? (() => Promise.resolve(true)); const blockedReviewFeedbackWriteResult = () => ({ allResolved: false, autoResolvedByFixEvaluation: commentsResolvedByFixEval.size, @@ -686,11 +675,28 @@ async function evaluateFixesAndResolveStale( runtime: fixEvaluationRuntime, }); + // Check head freshness up front so a stale or unverifiable run skips the + // LLM fix evaluation entirely, not just the writes it would produce. + let writability: ReviewFeedbackWritability = 'blocked'; + if (wardenComments.length > 0) { + if (!canResolveStale) { + logAction('Skipping stale comment resolution due to trigger failures'); + } else if (context.pullRequest) { + writability = await gate.check(); + if (writability === 'blocked') { + logAction('Skipping stale comment resolution because this run is no longer analyzing the current PR head'); + } else if (writability === 'unknown') { + logAction('Skipping stale comment resolution because the current PR head could not be verified'); + } + } + } + const canMutateFeedback = writability === 'writable'; + // Evaluate follow-up commit fix attempts if ( context.pullRequest && commentsForFixEvaluation.length > 0 && - canResolveStale && + canMutateFeedback && canUseFixEvaluationRuntime ) { try { @@ -746,7 +752,7 @@ async function evaluateFixesAndResolveStale( // Resolve successful fixes if (fixEvaluation.toResolve.length > 0) { - if (!await canWriteReviewFeedback()) { + if (!await gate.canWrite()) { logGroupEnd(); return blockedReviewFeedbackWriteResult(); } @@ -778,14 +784,14 @@ async function evaluateFixesAndResolveStale( } // Post replies for failed fixes and track them so stale pass doesn't override + if (fixEvaluation.toReply.length > 0 && !await gate.canWrite()) { + logGroupEnd(); + return blockedReviewFeedbackWriteResult(); + } for (const reply of fixEvaluation.toReply) { commentsEvaluatedByFixEval.add(reply.comment.id); if (reply.comment.threadId) { try { - if (!await canWriteReviewFeedback()) { - logGroupEnd(); - return blockedReviewFeedbackWriteResult(); - } await postThreadReply(octokit, reply.comment.threadId, reply.replyBody); } catch (error) { Sentry.captureException(error, { tags: { operation: 'post_thread_reply' } }); @@ -823,7 +829,7 @@ async function evaluateFixesAndResolveStale( // Resolve stale Warden comments (comments that no longer have matching findings) // Exclude comments already handled by fix evaluation (resolved or flagged as needing attention) - if (context.pullRequest && wardenComments.length > 0 && canResolveStale) { + if (context.pullRequest && wardenComments.length > 0 && canMutateFeedback) { try { const scope = buildAnalyzedScope(context.pullRequest.files); const commentsForStaleCheck = wardenComments.filter( @@ -835,7 +841,7 @@ async function evaluateFixesAndResolveStale( const staleComments = findStaleComments(commentsForStaleCheck, allFindings, scope); if (staleComments.length > 0) { - if (!await canWriteReviewFeedback()) { + if (!await gate.canWrite()) { return blockedReviewFeedbackWriteResult(); } @@ -883,8 +889,6 @@ async function evaluateFixesAndResolveStale( } warnAction(`Failed to resolve stale comments: ${error}`); } - } else if (!canResolveStale && wardenComments.length > 0) { - logAction('Skipping stale comment resolution due to trigger failures'); } // Determine if all unresolved Warden comments were resolved during this run @@ -911,6 +915,7 @@ async function dismissPreviousReviewIfResolved( previousReviewInfo: BotReviewInfo | null, results: TriggerResult[], canResolveStale: boolean, + gate: ReviewFeedbackGate, options: { failOnWriteError?: boolean } = {} ): Promise { // Dismiss previous CHANGES_REQUESTED if all blocking issues are resolved. @@ -929,7 +934,7 @@ async function dismissPreviousReviewIfResolved( !wouldRequestChanges && hasActiveFailOn ) { - if (!await canWriteReviewFeedbackForCurrentHead(octokit, context)) { + if (!await gate.canWrite()) { return; } @@ -966,6 +971,7 @@ async function finalizeWorkflow( shouldFailAction: boolean, failureReasons: string[], canResolveStale: boolean, + gate: ReviewFeedbackGate, triggerErrors: string[] ): Promise { await dismissPreviousReviewIfResolved( @@ -973,7 +979,8 @@ async function finalizeWorkflow( context, previousReviewInfo, results, - canResolveStale + canResolveStale, + gate ); // Set outputs @@ -1555,6 +1562,7 @@ async function finalizeReportWorkflow( shouldFailAction: boolean, failureReasons: string[], canResolveStale: boolean, + gate: ReviewFeedbackGate, triggerErrors: string[], options: { failOnWriteError?: boolean } = {} ): Promise { @@ -1564,6 +1572,7 @@ async function finalizeReportWorkflow( previousReviewInfo, results, canResolveStale, + gate, { failOnWriteError: options.failOnWriteError } ); @@ -1614,9 +1623,9 @@ async function cleanupOrphanedComments( return []; } - const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); + const gate = new ReviewFeedbackGate(octokit, context); - if (!await canWriteReviewFeedback()) { + if (!await gate.canWrite()) { return []; } @@ -1646,9 +1655,8 @@ async function cleanupOrphanedComments( const { allResolved, autoResolvedByFixEvaluation, autoResolvedByStaleCheck, findingObservations } = await evaluateFixesAndResolveStale( - octokit, context, existingComments, [], new Set(), true, inputs.anthropicApiKey, auxiliaryOptions, { + octokit, context, existingComments, [], new Set(), true, inputs.anthropicApiKey, auxiliaryOptions, gate, { failOnWriteError: options.failOnWriteError, - canWriteReviewFeedback, } ); const activeSpan = Sentry.getActiveSpan(); @@ -1659,7 +1667,7 @@ async function cleanupOrphanedComments( if (allResolved) { const previousReviewInfo = await fetchPreviousReviewInfo(octokit, context); if (previousReviewInfo?.state === 'CHANGES_REQUESTED') { - if (!await canWriteReviewFeedback()) { + if (!await gate.canWrite()) { return findingObservations; } @@ -1840,9 +1848,10 @@ async function runReportMode( logAction(`Previous Warden review state: ${previousReviewInfo.state}`); } + const gate = new ReviewFeedbackGate(octokit, context); reviewPhase = await Sentry.startSpan( { op: 'workflow.review', name: 'post reviews' }, - () => postReviewsAndTrackFailures(octokit, context, results, inputs, auxiliaryOptions, { + () => postReviewsAndTrackFailures(octokit, context, results, inputs, auxiliaryOptions, gate, { failOnPostError: true, }), ); @@ -1851,24 +1860,16 @@ async function runReportMode( canResolveStale = shouldResolveStaleComments(results); const allFindings = reviewPhase.reports.flatMap((r) => r.findings); span.setAttribute('warden.finding.count', allFindings.length); - const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); await Sentry.startSpan( { op: 'workflow.resolve', name: 'resolve stale comments' }, async (resolveSpan) => { - const canMutateReviewFeedback = - canResolveStale && - reviewPhase.reviewFeedbackWritable && - await canWriteReviewFeedback(); const resolutionResult = await evaluateFixesAndResolveStale( octokit, context, reviewPhase.fetchedComments, allFindings, reviewPhase.activeWardenCommentIds, - canMutateReviewFeedback, inputs.anthropicApiKey, - auxiliaryOptions, - { - failOnWriteError: true, - canWriteReviewFeedback, - }, + canResolveStale, inputs.anthropicApiKey, + auxiliaryOptions, gate, + { failOnWriteError: true }, ); resolveSpan.setAttribute( 'warden.feedback.auto_resolve.fix_eval_count', @@ -1882,17 +1883,13 @@ async function runReportMode( }, ); - const canDismissReview = - canResolveStale && - reviewPhase.reviewFeedbackWritable && - await canWriteReviewFeedback(); - await finalizeReportWorkflow( octokit, context, previousReviewInfo, results, reviewPhase.reports, reviewPhase.findingObservations, reviewPhase.shouldFailAction, reviewPhase.failureReasons, - canDismissReview, + canResolveStale, + gate, triggerErrors, { failOnWriteError: true }, ); @@ -2033,13 +2030,14 @@ export async function runPRWorkflow( throw error; } + const gate = new ReviewFeedbackGate(octokit, context); const reviewPhase = await runOrFailCore( octokit, context, coreCheckId, () => Sentry.startSpan( { op: 'workflow.review', name: 'post reviews' }, - () => postReviewsAndTrackFailures(octokit, context, results, inputs, auxiliaryOptions), + () => postReviewsAndTrackFailures(octokit, context, results, inputs, auxiliaryOptions, gate), ), ); @@ -2047,7 +2045,6 @@ export async function runPRWorkflow( const canResolveStale = shouldResolveStaleComments(results); const allFindings = reviewPhase.reports.flatMap((r) => r.findings); span.setAttribute('warden.finding.count', allFindings.length); - const canWriteReviewFeedback = () => canWriteReviewFeedbackForCurrentHead(octokit, context); await runOrFailCore( octokit, @@ -2056,16 +2053,11 @@ export async function runPRWorkflow( () => Sentry.startSpan( { op: 'workflow.resolve', name: 'resolve stale comments' }, async (resolveSpan) => { - const canMutateReviewFeedback = - canResolveStale && - reviewPhase.reviewFeedbackWritable && - await canWriteReviewFeedback(); const resolutionResult = await evaluateFixesAndResolveStale( octokit, context, reviewPhase.fetchedComments, allFindings, reviewPhase.activeWardenCommentIds, - canMutateReviewFeedback, inputs.anthropicApiKey, - auxiliaryOptions, - { canWriteReviewFeedback }, + canResolveStale, inputs.anthropicApiKey, + auxiliaryOptions, gate, ); resolveSpan.setAttribute( 'warden.feedback.auto_resolve.fix_eval_count', @@ -2080,17 +2072,13 @@ export async function runPRWorkflow( ), ); - const canDismissReview = - canResolveStale && - reviewPhase.reviewFeedbackWritable && - await canWriteReviewFeedback(); - await finalizeWorkflow( octokit, context, previousReviewInfo, coreCheckId, results, reviewPhase.reports, reviewPhase.findingObservations, reviewPhase.shouldFailAction, reviewPhase.failureReasons, - canDismissReview, + canResolveStale, + gate, triggerErrors, ); diff --git a/packages/warden/src/action/workflow/review-feedback-gate.test.ts b/packages/warden/src/action/workflow/review-feedback-gate.test.ts new file mode 100644 index 00000000..d2c577f6 --- /dev/null +++ b/packages/warden/src/action/workflow/review-feedback-gate.test.ts @@ -0,0 +1,134 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { Octokit } from '@octokit/rest'; +import type { EventContext } from '../../types/index.js'; +import { ReviewFeedbackGate } from './review-feedback-gate.js'; + +function createContext(overrides: Partial = {}): EventContext { + return { + eventType: 'pull_request', + action: 'opened', + repository: { owner: 'test-owner', name: 'test-repo', fullName: 'test-owner/test-repo', defaultBranch: 'main' }, + pullRequest: { + number: 123, + title: 'Test PR', + body: '', + author: 'test-user', + baseBranch: 'main', + headBranch: 'feature', + headSha: 'run-head-sha', + baseSha: 'base-sha', + files: [], + }, + repoPath: '/test/path', + ...overrides, + }; +} + +function createOctokit(getMock: ReturnType): Octokit { + return { pulls: { get: getMock } } as unknown as Octokit; +} + +function headResponse(sha: string) { + return { data: { head: { sha } } }; +} + +describe('ReviewFeedbackGate', () => { + beforeEach(() => { + vi.spyOn(console, 'log').mockImplementation(() => undefined); + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('returns blocked without API calls when there is no pull request context', async () => { + const getMock = vi.fn(); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext({ pullRequest: undefined })); + + expect(await gate.check()).toBe('blocked'); + expect(await gate.canWrite()).toBe(false); + expect(getMock).not.toHaveBeenCalled(); + }); + + it('returns writable when the PR head matches and memoizes within the TTL', async () => { + const getMock = vi.fn().mockResolvedValue(headResponse('run-head-sha')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext()); + + expect(await gate.check()).toBe('writable'); + expect(await gate.check()).toBe('writable'); + expect(await gate.canWrite()).toBe(true); + expect(getMock).toHaveBeenCalledTimes(1); + }); + + it('re-fetches after the TTL expires', async () => { + const getMock = vi.fn().mockResolvedValue(headResponse('run-head-sha')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext(), { ttlMs: 0 }); + + expect(await gate.check()).toBe('writable'); + expect(await gate.check()).toBe('writable'); + expect(getMock).toHaveBeenCalledTimes(2); + }); + + it('blocks permanently once the head has advanced', async () => { + const getMock = vi.fn().mockResolvedValue(headResponse('new-head-sha')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext(), { ttlMs: 0 }); + + expect(await gate.check()).toBe('blocked'); + expect(await gate.check()).toBe('blocked'); + expect(await gate.canWrite()).toBe(false); + expect(getMock).toHaveBeenCalledTimes(1); + }); + + it('retries transient head fetch errors before succeeding', async () => { + const getMock = vi + .fn() + .mockRejectedValueOnce(new Error('boom')) + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValueOnce(headResponse('run-head-sha')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext(), { retryDelayMs: 0 }); + + expect(await gate.check()).toBe('writable'); + expect(getMock).toHaveBeenCalledTimes(3); + }); + + it('returns unknown after exhausting retries and retries again after the TTL', async () => { + const getMock = vi.fn().mockRejectedValue(new Error('boom')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext(), { + attempts: 2, + retryDelayMs: 0, + ttlMs: 0, + }); + + expect(await gate.check()).toBe('unknown'); + expect(await gate.canWrite()).toBe(false); + expect(getMock).toHaveBeenCalledTimes(4); + }); + + it('caches unknown within the TTL so bursts of checks do not re-fetch', async () => { + const getMock = vi.fn().mockRejectedValue(new Error('boom')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext(), { + attempts: 2, + retryDelayMs: 0, + }); + + expect(await gate.check()).toBe('unknown'); + expect(await gate.check()).toBe('unknown'); + expect(getMock).toHaveBeenCalledTimes(2); + }); + + it('recovers to writable when a later fetch succeeds after unknown', async () => { + const getMock = vi + .fn() + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValue(headResponse('run-head-sha')); + const gate = new ReviewFeedbackGate(createOctokit(getMock), createContext(), { + attempts: 1, + retryDelayMs: 0, + ttlMs: 0, + }); + + expect(await gate.check()).toBe('unknown'); + expect(await gate.check()).toBe('writable'); + }); +}); diff --git a/packages/warden/src/action/workflow/review-feedback-gate.ts b/packages/warden/src/action/workflow/review-feedback-gate.ts new file mode 100644 index 00000000..f089759c --- /dev/null +++ b/packages/warden/src/action/workflow/review-feedback-gate.ts @@ -0,0 +1,98 @@ +/** + * Review Feedback Gate + * + * Single owner of the "is this run still analyzing the current PR head?" + * check that guards every PR review feedback mutation (posting reviews, + * resolving threads, replying, dismissing reviews). + */ + +import type { Octokit } from '@octokit/rest'; +import { Sentry } from '../../sentry.js'; +import { warnAction } from '../../cli/output/tty.js'; +import { sleep } from '../../sdk/retry.js'; +import type { EventContext } from '../../types/index.js'; + +export type ReviewFeedbackWritability = 'writable' | 'blocked' | 'unknown'; + +const FRESHNESS_TTL_MS = 10_000; +const HEAD_FETCH_ATTEMPTS = 3; +const HEAD_FETCH_RETRY_DELAY_MS = 500; + +/** + * Guards PR review feedback writes behind a head-freshness check. + * + * States returned by {@link ReviewFeedbackGate.check}: + * - `writable`: the PR head matched this run's head within the TTL window. + * - `blocked`: no PR context, or the head advanced past this run. Permanent + * for the run; a head that advanced never becomes current again. + * - `unknown`: the head could not be verified after retries. Writes must be + * skipped (fail closed), but the state is cached only briefly so later + * phases retry instead of disabling feedback for the whole run. Callers + * that suppress a blocking review because of `unknown` must fail the run + * instead of letting it pass silently. + * + * Results are memoized for a short TTL so bursts of writes share one + * `pulls.get` call while long LLM phases still trigger a fresh check. + */ +export class ReviewFeedbackGate { + private blocked = false; + private cached?: { status: 'writable' | 'unknown'; at: number }; + + constructor( + private readonly octokit: Octokit, + private readonly context: EventContext, + private readonly options: { ttlMs?: number; attempts?: number; retryDelayMs?: number } = {} + ) {} + + /** Report whether this run may still mutate PR review feedback. */ + async check(): Promise { + const pullRequest = this.context.pullRequest; + if (!pullRequest || this.blocked) { + return 'blocked'; + } + + const ttlMs = this.options.ttlMs ?? FRESHNESS_TTL_MS; + if (this.cached && Date.now() - this.cached.at < ttlMs) { + return this.cached.status; + } + + const attempts = this.options.attempts ?? HEAD_FETCH_ATTEMPTS; + const retryDelayMs = this.options.retryDelayMs ?? HEAD_FETCH_RETRY_DELAY_MS; + let lastError: unknown; + for (let attempt = 1; attempt <= attempts; attempt++) { + try { + const { data } = await this.octokit.pulls.get({ + owner: this.context.repository.owner, + repo: this.context.repository.name, + pull_number: pullRequest.number, + }); + if (data.head.sha !== pullRequest.headSha) { + this.blocked = true; + warnAction( + `Skipping PR review feedback because run head ${pullRequest.headSha} is no longer the PR head ${data.head.sha}` + ); + return 'blocked'; + } + this.cached = { status: 'writable', at: Date.now() }; + return 'writable'; + } catch (error) { + lastError = error; + if (attempt < attempts) { + await sleep(retryDelayMs * attempt); + } + } + } + + Sentry.captureException(lastError, { tags: { operation: 'fetch_current_pr_head' } }); + warnAction( + `Could not verify the current PR head after ${attempts} attempts; skipping review feedback writes: ${lastError}` + ); + this.cached = { status: 'unknown', at: Date.now() }; + return 'unknown'; + } + + /** True when review feedback writes are allowed right now. */ + async canWrite(): Promise { + return (await this.check()) === 'writable'; + } +} From 5b5bf2f829ba1ffd0429e3e16fed8a347404a08e Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 17:47:12 -0700 Subject: [PATCH 3/7] fix(warden): Re-verify PR head before poster GitHub writes The head-freshness check ran before postTriggerReview, but consolidation and dedup inside it are LLM phases that can run for minutes before the review write and duplicate-action comment updates. A head that advanced during that window still received stale feedback. The poster now takes the run's ReviewFeedbackGate and re-checks it after the LLM phases, before the first GitHub write. The workflow escalation for unverifiable heads moved after the post attempt so it also covers heads that become unverifiable mid-post. The gate moved to action/review since it guards review feedback, and the poster tests drive it through the octokit boundary instead of an injected callback. Co-Authored-By: Claude Fable 5 --- .../warden/src/action/review/poster.test.ts | 58 +++++++++++++++++-- packages/warden/src/action/review/poster.ts | 10 ++++ .../review-feedback-gate.test.ts | 0 .../review-feedback-gate.ts | 0 .../warden/src/action/workflow/pr-workflow.ts | 17 ++++-- 5 files changed, 75 insertions(+), 10 deletions(-) rename packages/warden/src/action/{workflow => review}/review-feedback-gate.test.ts (100%) rename packages/warden/src/action/{workflow => review}/review-feedback-gate.ts (100%) diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index 1906ec79..21817bd9 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -21,6 +21,7 @@ vi.mock('../../output/renderer.js', () => ({ import { deduplicateFindings, processDuplicateActions, findingToExistingComment, consolidateBatchFindings } from '../../output/dedup.js'; import { renderSkillReport } from '../../output/renderer.js'; +import { ReviewFeedbackGate } from './review-feedback-gate.js'; describe('postTriggerReview', () => { beforeEach(() => { @@ -35,11 +36,20 @@ describe('postTriggerReview', () => { removedCount: 0, removedFindings: [], })); + + vi.mocked(mockOctokit.pulls.createReview).mockResolvedValue({} as never); + vi.mocked(mockOctokit.pulls.get).mockResolvedValue({ data: { head: { sha: 'abc123' } } } as never); + mockDeps = { + octokit: mockOctokit, + context: mockContext, + feedbackGate: new ReviewFeedbackGate(mockOctokit, mockContext), + }; }); const mockOctokit = { pulls: { createReview: vi.fn().mockResolvedValue({}), + get: vi.fn().mockResolvedValue({ data: { head: { sha: 'abc123' } } }), }, } as unknown as Octokit; @@ -61,10 +71,8 @@ describe('postTriggerReview', () => { repoPath: '/test/path', }; - const mockDeps: ReviewPosterDeps = { - octokit: mockOctokit, - context: mockContext, - }; + // Rebuilt per test so the gate's head-freshness cache starts empty. + let mockDeps: ReviewPosterDeps; const createFinding = (overrides: Partial = {}): Finding => ({ id: 'test-1', @@ -270,6 +278,48 @@ describe('postTriggerReview', () => { expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); }); + it('skips all GitHub writes when the PR head advances during consolidation', async () => { + const findings = [createFinding(), createFinding({ id: 'test-2', title: 'Second finding' })]; + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + report: { + skill: 'test-skill', + summary: 'Found 2 issues', + findings, + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'COMMENT', + body: '', + comments: [{ path: 'test.ts', line: 10, body: 'Test comment' }], + }, + }), + reportOn: 'low', + }; + + // The head moves while the LLM consolidation runs. + vi.mocked(consolidateBatchFindings).mockImplementation(async (batch) => { + vi.mocked(mockOctokit.pulls.get).mockResolvedValue({ data: { head: { sha: 'new-head-sha' } } } as never); + return { findings: batch, removedCount: 0, removedFindings: [] }; + }); + vi.mocked(deduplicateFindings).mockResolvedValue({ + newFindings: findings, + duplicateActions: [{ finding: findings[0]!, existingComment: createExistingComment(), matchType: 'hash', originalFindingId: findings[0]!.id }], + } as never); + + const postResult = await postTriggerReview({ + result, + existingComments: [createExistingComment()], + apiKey: 'test-key', + }, mockDeps); + + expect(postResult.posted).toBe(false); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(processDuplicateActions).not.toHaveBeenCalled(); + }); + it('marks locationless findings in a mixed review as checks-only instead of posted', async () => { const inlineFinding = createFinding(); const bodyFinding = createFinding({ id: 'test-2', title: 'Locationless finding', location: undefined }); diff --git a/packages/warden/src/action/review/poster.ts b/packages/warden/src/action/review/poster.ts index 05196659..a625c061 100644 --- a/packages/warden/src/action/review/poster.ts +++ b/packages/warden/src/action/review/poster.ts @@ -24,6 +24,7 @@ import type { RuntimeName } from '../../sdk/runtimes/index.js'; import type { TriggerResult } from '../triggers/executor.js'; import { logAction, warnAction } from '../../cli/output/tty.js'; import type { FindingObservation } from '../reporting/outcomes.js'; +import type { ReviewFeedbackGate } from './review-feedback-gate.js'; // ----------------------------------------------------------------------------- // Types @@ -67,6 +68,8 @@ export interface ReviewPostResult { export interface ReviewPosterDeps { octokit: Octokit; context: EventContext; + /** Head-freshness gate shared with the rest of the workflow run. */ + feedbackGate: ReviewFeedbackGate; } function emptyReviewPostResult( @@ -343,6 +346,13 @@ export async function postTriggerReview( } } + // Consolidation and dedup above can spend minutes in LLM calls. Re-verify + // head freshness before the first GitHub write (duplicate-action comment + // updates below, then the review itself). + if (!(await deps.feedbackGate.canWrite())) { + return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); + } + // Process duplicate actions (update Warden comments, add reactions) if (dedupResult?.duplicateActions.length) { const actionCounts = await processDuplicateActions( diff --git a/packages/warden/src/action/workflow/review-feedback-gate.test.ts b/packages/warden/src/action/review/review-feedback-gate.test.ts similarity index 100% rename from packages/warden/src/action/workflow/review-feedback-gate.test.ts rename to packages/warden/src/action/review/review-feedback-gate.test.ts diff --git a/packages/warden/src/action/workflow/review-feedback-gate.ts b/packages/warden/src/action/review/review-feedback-gate.ts similarity index 100% rename from packages/warden/src/action/workflow/review-feedback-gate.ts rename to packages/warden/src/action/review/review-feedback-gate.ts diff --git a/packages/warden/src/action/workflow/pr-workflow.ts b/packages/warden/src/action/workflow/pr-workflow.ts index 17b9c646..2474f965 100644 --- a/packages/warden/src/action/workflow/pr-workflow.ts +++ b/packages/warden/src/action/workflow/pr-workflow.ts @@ -43,8 +43,8 @@ import { executeTrigger } from '../triggers/executor.js'; import type { TriggerCheckReporter, TriggerResult } from '../triggers/executor.js'; import { postTriggerReview } from '../review/poster.js'; import { shouldResolveStaleComments } from '../review/coordination.js'; -import { ReviewFeedbackGate } from './review-feedback-gate.js'; -import type { ReviewFeedbackWritability } from './review-feedback-gate.js'; +import { ReviewFeedbackGate } from '../review/review-feedback-gate.js'; +import type { ReviewFeedbackWritability } from '../review/review-feedback-gate.js'; import type { FindingObservation } from '../reporting/outcomes.js'; import type { RuntimeName } from '../../sdk/runtimes/index.js'; import { canUseRuntimeAuth } from '../../sdk/extract.js'; @@ -568,6 +568,7 @@ async function postReviewsAndTrackFailures( if (writability !== 'blocked') { writability = await gate.check(); } + let reviewPosted = false; if (writability === 'writable') { const postResult = await postTriggerReview( { @@ -579,16 +580,20 @@ async function postReviewsAndTrackFailures( maxRetries: auxiliaryOptions.maxRetries, failOnPostError: options.failOnPostError, }, - { octokit, context } + { octokit, context, feedbackGate: gate } ); // Add newly posted comments to existing comments for cross-trigger deduplication existingComments.push(...postResult.newComments); postResult.activeWardenCommentIds.forEach((id) => activeWardenCommentIds.add(id)); findingObservations.push(...postResult.findingObservations); - } else if (writability === 'unknown' && wouldPostBlockingReview(result)) { - // A stale head skips silently (the newer run owns feedback), but an - // unverifiable head must not silently swallow a blocking review. + reviewPosted = postResult.posted; + } + // A stale head skips silently (the newer run owns feedback), but an + // unverifiable head must not silently swallow a blocking review. + // Evaluated after the post attempt so a head that becomes unverifiable + // during the poster's own LLM phases is escalated too. + if (!reviewPosted && wouldPostBlockingReview(result) && (await gate.check()) === 'unknown') { shouldFailAction = true; failureReasons.push( `${result.triggerName}: Could not verify the PR head; blocking review was not posted` From fbf695e18793f88bfeaefe53cc82f43fd78ac934 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 18:04:45 -0700 Subject: [PATCH 4/7] fix(warden): Gate the review write after duplicate processing processDuplicateActions sits between the poster's head check and pulls.createReview and issues sequential GitHub writes, so with many duplicate comments it can outlive the gate's cache window. Re-verify the gate inside postReviewToGitHub immediately before the review write; in the common case this is a cache hit, and it re-fetches exactly when duplicate processing ran long. Co-Authored-By: Claude Fable 5 --- .../warden/src/action/review/poster.test.ts | 51 +++++++++++++++++++ packages/warden/src/action/review/poster.ts | 16 ++++-- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index 21817bd9..f06ad88f 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -320,6 +320,57 @@ describe('postTriggerReview', () => { expect(processDuplicateActions).not.toHaveBeenCalled(); }); + it('skips the review write when the PR head advances during duplicate processing', async () => { + // The gate verifies before duplicate processing; advance the clock past + // its cache window inside that write phase so the pre-review check + // re-fetches the head. + let now = 1_750_000_000_000; + const dateNowSpy = vi.spyOn(Date, 'now').mockImplementation(() => now); + try { + const findings = [createFinding(), createFinding({ id: 'test-2', title: 'Second finding' })]; + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + report: { + skill: 'test-skill', + summary: 'Found 2 issues', + findings, + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'COMMENT', + body: '', + comments: [{ path: 'test.ts', line: 10, body: 'Test comment' }], + }, + }), + reportOn: 'low', + }; + + vi.mocked(deduplicateFindings).mockResolvedValue({ + newFindings: [findings[0]!], + duplicateActions: [{ finding: findings[1]!, existingComment: createExistingComment(), matchType: 'hash', originalFindingId: findings[1]!.id }], + } as never); + vi.mocked(processDuplicateActions).mockImplementation(async () => { + now += 60_000; + vi.mocked(mockOctokit.pulls.get).mockResolvedValue({ data: { head: { sha: 'new-head-sha' } } } as never); + return { updated: 1, reacted: 0, skipped: 0, failed: 0 }; + }); + + const postResult = await postTriggerReview({ + result, + existingComments: [createExistingComment()], + apiKey: 'test-key', + }, mockDeps); + + expect(processDuplicateActions).toHaveBeenCalled(); + expect(postResult.posted).toBe(false); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + } finally { + dateNowSpy.mockRestore(); + } + }); + it('marks locationless findings in a mixed review as checks-only instead of posted', async () => { const inlineFinding = createFinding(); const bodyFinding = createFinding({ id: 'test-2', title: 'Locationless finding', location: undefined }); diff --git a/packages/warden/src/action/review/poster.ts b/packages/warden/src/action/review/poster.ts index a625c061..4bd0e06c 100644 --- a/packages/warden/src/action/review/poster.ts +++ b/packages/warden/src/action/review/poster.ts @@ -130,8 +130,9 @@ function recenterReportFindingIds(reportFindings: Finding[], actions: Deduplicat * - `posted`: the review was created on the PR * - `checks_only`: findings could not be attached inline; they stay in Checks * - `no_review`: nothing to post (no PR context or no rendered review) + * - `blocked`: the run may no longer write feedback for the current PR head */ -type PostReviewOutcome = 'posted' | 'checks_only' | 'no_review'; +type PostReviewOutcome = 'posted' | 'checks_only' | 'no_review' | 'blocked'; /** * Post a PR review to GitHub. @@ -139,7 +140,8 @@ type PostReviewOutcome = 'posted' | 'checks_only' | 'no_review'; async function postReviewToGitHub( octokit: Octokit, context: EventContext, - result: RenderResult + result: RenderResult, + feedbackGate: ReviewFeedbackGate ): Promise { if (!context.pullRequest) { return 'no_review'; @@ -170,6 +172,12 @@ async function postReviewToGitHub( return 'checks_only'; } + // Duplicate-action comment updates between the poster's gate check and this + // write can outlive the gate's cache window; verify once more. + if (!(await feedbackGate.canWrite())) { + return 'blocked'; + } + await octokit.pulls.createReview({ owner, repo, @@ -425,7 +433,7 @@ export async function postTriggerReview( let postOutcome: PostReviewOutcome = 'no_review'; try { - postOutcome = await postReviewToGitHub(octokit, context, renderResultToPost); + postOutcome = await postReviewToGitHub(octokit, context, renderResultToPost, deps.feedbackGate); } catch (error) { if (!isLineResolutionError(error)) { throw error; @@ -433,7 +441,7 @@ export async function postTriggerReview( if (renderResultToPost.review?.event === 'REQUEST_CHANGES') { warnAction(`Inline comments failed for ${result.triggerName}, posting findings in review body`); const fallback = moveCommentsToBody(renderResultToPost, postedFindings, skill); - postOutcome = await postReviewToGitHub(octokit, context, fallback); + postOutcome = await postReviewToGitHub(octokit, context, fallback, deps.feedbackGate); } else { warnAction(`Inline comments failed for ${result.triggerName}, falling back to checks only`); postOutcome = 'checks_only'; From b1d92631b5c7517ec1dd9ef73f1475b0fe17474e Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 18:58:24 -0700 Subject: [PATCH 5/7] test(warden): Exercise the pre-write gate check for real The duplicate-processing regression test passed through a swallowed TypeError: dedup shrank the finding set, the re-render path returned the unmocked renderSkillReport's undefined, and postReviewToGitHub threw before reaching the gate. Mock the re-render, assert the second head fetch happened, and assert no findings were marked failed so the test fails if the pre-write check is removed (verified by mutation). Co-Authored-By: Claude Fable 5 --- packages/warden/src/action/review/poster.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index f06ad88f..45ec7e5d 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -351,6 +351,14 @@ describe('postTriggerReview', () => { newFindings: [findings[0]!], duplicateActions: [{ finding: findings[1]!, existingComment: createExistingComment(), matchType: 'hash', originalFindingId: findings[1]!.id }], } as never); + // Dedup shrank the finding set, so the poster re-renders before posting. + vi.mocked(renderSkillReport).mockReturnValue(createRenderResult({ + review: { + event: 'COMMENT', + body: '', + comments: [{ path: 'test.ts', line: 10, body: 'Test comment' }], + }, + })); vi.mocked(processDuplicateActions).mockImplementation(async () => { now += 60_000; vi.mocked(mockOctokit.pulls.get).mockResolvedValue({ data: { head: { sha: 'new-head-sha' } } } as never); @@ -366,6 +374,11 @@ describe('postTriggerReview', () => { expect(processDuplicateActions).toHaveBeenCalled(); expect(postResult.posted).toBe(false); expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + // The gate verified twice: before duplicate processing and again before + // the review write, where it saw the new head. + expect(mockOctokit.pulls.get).toHaveBeenCalledTimes(2); + // No swallowed error: the findings were not marked failed. + expect(postResult.findingObservations.filter((o) => o.outcome === 'failed')).toEqual([]); } finally { dateNowSpy.mockRestore(); } From f917251b329f063271332e01b2474fd99f7d90a3 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 20:00:17 -0700 Subject: [PATCH 6/7] docs(warden): Note why wouldPostBlockingReview covers needsRequestChanges A review pass flagged the needsRequestChanges branch as uncovered by the escalation predicate; it is only reachable past the poster's reportOn early return, where the predicate is already true. State the invariant so the next reader does not have to re-derive it. Co-Authored-By: Claude Fable 5 --- packages/warden/src/action/workflow/pr-workflow.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/warden/src/action/workflow/pr-workflow.ts b/packages/warden/src/action/workflow/pr-workflow.ts index 2474f965..879668db 100644 --- a/packages/warden/src/action/workflow/pr-workflow.ts +++ b/packages/warden/src/action/workflow/pr-workflow.ts @@ -627,7 +627,10 @@ async function postReviewsAndTrackFailures( * Whether posting this trigger's review would produce a blocking * REQUEST_CHANGES review. Mirrors the poster's posting predicate: the * renderer can emit a REQUEST_CHANGES render result with zero reportable - * findings (reportOn stricter than failOn), which the poster never posts. + * findings (reportOn stricter than failOn), which the poster never posts — + * its reportOn early return runs before the needsRequestChanges branch, so + * that branch is only reachable when this predicate is already true (the + * pre-dedup filtered set was non-empty or reportOnSuccess is set). */ function wouldPostBlockingReview(result: TriggerResult): boolean { if (!result.report || result.renderResult?.review?.event !== 'REQUEST_CHANGES') { From 23aab12f88b6576925d32ead099cae16c24eff9d Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 3 Jul 2026 20:06:06 -0700 Subject: [PATCH 7/7] test(warden): Pin the reportOn early return the escalation relies on wouldPostBlockingReview assumes the poster's reportOn early return makes the needsRequestChanges branch unreachable when no findings clear reportOn. Exercise that scenario through the real poster so reordering those checks fails a test instead of silently opening an escalation gap. Co-Authored-By: Claude Fable 5 --- .../warden/src/action/review/poster.test.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index 45ec7e5d..504f9cab 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -424,6 +424,44 @@ describe('postTriggerReview', () => { ]); }); + it('does not post a blocking review when reportOn filters out all findings', async () => { + // reportOn stricter than failOn: the renderer emits a REQUEST_CHANGES + // fallback, but the reportOn early return runs before the + // needsRequestChanges branch, so nothing posts. The workflow's + // wouldPostBlockingReview escalation predicate relies on this. + const finding = createFinding({ severity: 'medium' }); + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + report: { + skill: 'test-skill', + summary: 'Found 1 issue', + findings: [finding], + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'REQUEST_CHANGES', + body: 'Findings exceed the configured threshold. See the GitHub Check for details.', + comments: [], + }, + }), + reportOn: 'high', + failOn: 'medium', + requestChanges: true, + }; + + const postResult = await postTriggerReview({ + result, + existingComments: [], + apiKey: 'test-key', + }, mockDeps); + + expect(postResult.posted).toBe(false); + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(postResult.findingObservations).toEqual([]); + }); + it('deduplicates findings against existing comments', async () => { const finding = createFinding(); const result: TriggerResult = {