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..db6be7eb 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. 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 ```yaml diff --git a/packages/docs/src/content/docs/github.mdx b/packages/docs/src/content/docs/github.mdx index fc71f08d..85c4c4b4 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,16 @@ 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. 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` Pull request behavior is still driven by `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 4c464940..504f9cab 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', @@ -231,11 +239,229 @@ 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([ + { outcome: 'skipped', finding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + ]); + 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('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); + // 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); + 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(); + // 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(); + } + }); + + 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('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 = { @@ -612,7 +838,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 +861,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 +874,51 @@ describe('postTriggerReview', () => { const postResult = await postTriggerReview(ctx, mockDeps); + expect(postResult.posted).toBe(false); + expect(postResult.newComments).toHaveLength(0); + expect(postResult.findingObservations).toEqual([ + { outcome: 'skipped', finding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + ]); + 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..4bd0e06c 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( @@ -122,22 +125,30 @@ 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) + * - `blocked`: the run may no longer write feedback for the current PR head + */ +type PostReviewOutcome = 'posted' | 'checks_only' | 'no_review' | 'blocked'; + /** * Post a PR review to GitHub. */ async function postReviewToGitHub( octokit: Octokit, context: EventContext, - result: RenderResult -): Promise { + result: RenderResult, + feedbackGate: ReviewFeedbackGate +): Promise { if (!context.pullRequest) { - return; + return 'no_review'; } - // 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 'no_review'; } const { owner, name: repo } = context.repository; @@ -155,15 +166,29 @@ 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 '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, 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 'posted'; } /** @@ -329,6 +354,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( @@ -399,17 +431,40 @@ export async function postTriggerReview( }); } + let postOutcome: PostReviewOutcome = 'no_review'; try { - await postReviewToGitHub(octokit, context, renderResultToPost); + postOutcome = await postReviewToGitHub(octokit, context, renderResultToPost, deps.feedbackGate); } 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); + postOutcome = await postReviewToGitHub(octokit, context, fallback, deps.feedbackGate); + } else { + warnAction(`Inline comments failed for ${result.triggerName}, falling back to checks only`); + postOutcome = 'checks_only'; + } } + 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/review/review-feedback-gate.test.ts b/packages/warden/src/action/review/review-feedback-gate.test.ts new file mode 100644 index 00000000..d2c577f6 --- /dev/null +++ b/packages/warden/src/action/review/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/review/review-feedback-gate.ts b/packages/warden/src/action/review/review-feedback-gate.ts new file mode 100644 index 00000000..f089759c --- /dev/null +++ b/packages/warden/src/action/review/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'; + } +} diff --git a/packages/warden/src/action/workflow/pr-workflow.test.ts b/packages/warden/src/action/workflow/pr-workflow.test.ts index bb7d2cf0..52a988ea 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,159 @@ 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 () => { + // 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).mockRejectedValue(new Error('GitHub is unavailable')); + + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + 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.createReview).not.toHaveBeenCalled(); + }); + + 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).mockRejectedValue(new Error('GitHub is unavailable')); + + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report }); + + await runPRWorkflow( + mockOctokit, + createDefaultInputs({ failOn: 'medium', reportOn: 'high', requestChanges: true }), + 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR + ); + + expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); + expect(mockSetFailed).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 +2234,85 @@ 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 () => { + // 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 + ); + + 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 () => { 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..879668db 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/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'; @@ -518,13 +520,17 @@ async function postReviewsAndTrackFailures( results: TriggerResult[], inputs: ActionInputs, auxiliaryOptions: AuxiliaryWorkflowOptions, + gate: ReviewFeedbackGate, options: { failOnPostError?: boolean } = {} ): Promise { - // Fetch existing comments for deduplication (only for PRs) + // 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[] = []; - if (context.pullRequest) { + let writability = await gate.check(); + if (writability !== 'blocked' && context.pullRequest) { try { fetchedComments = await fetchExistingComments( octokit, @@ -557,24 +563,42 @@ async function postReviewsAndTrackFailures( if (result.report) { 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 } - ); + // 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(); + } + let reviewPosted = false; + 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, 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); + // 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); + 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` + ); + } // Check if we should fail based on this trigger's config // Filter by confidence first so low-confidence findings don't cause failure @@ -599,6 +623,23 @@ 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 — + * 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') { + return false; + } + const filteredFindings = filterFindings(result.report.findings, result.reportOn, result.minConfidence); + return filteredFindings.length > 0 || (result.reportOnSuccess ?? false); +} + /** * Evaluate fix attempts on unresolved comments and resolve stale comments. * @@ -614,6 +655,7 @@ async function evaluateFixesAndResolveStale( canResolveStale: boolean, anthropicApiKey: string, auxiliaryOptions: AuxiliaryWorkflowOptions, + gate: ReviewFeedbackGate, options: { failOnWriteError?: boolean } = {} ): Promise<{ allResolved: boolean; @@ -626,6 +668,12 @@ async function evaluateFixesAndResolveStale( const commentsEvaluatedByFixEval = new Set(); const commentsResolvedByStale = new Set(); const findingObservations: FindingObservation[] = []; + const blockedReviewFeedbackWriteResult = () => ({ + allResolved: false, + autoResolvedByFixEvaluation: commentsResolvedByFixEval.size, + autoResolvedByStaleCheck: commentsResolvedByStale.size, + findingObservations, + }); const commentsForFixEvaluation = wardenComments.filter( (c) => !activeWardenCommentIds.has(c.id) ); @@ -635,11 +683,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 { @@ -695,6 +760,11 @@ async function evaluateFixesAndResolveStale( // Resolve successful fixes if (fixEvaluation.toResolve.length > 0) { + if (!await gate.canWrite()) { + logGroupEnd(); + return blockedReviewFeedbackWriteResult(); + } + const { resolvedCount, resolvedIds } = await resolveStaleComments( octokit, fixEvaluation.toResolve, @@ -722,6 +792,10 @@ 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) { @@ -763,7 +837,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( @@ -775,6 +849,10 @@ async function evaluateFixesAndResolveStale( const staleComments = findStaleComments(commentsForStaleCheck, allFindings, scope); if (staleComments.length > 0) { + if (!await gate.canWrite()) { + return blockedReviewFeedbackWriteResult(); + } + const { resolvedCount, resolvedIds } = await resolveStaleComments( octokit, staleComments, @@ -819,8 +897,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 @@ -847,6 +923,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. @@ -865,6 +942,10 @@ async function dismissPreviousReviewIfResolved( !wouldRequestChanges && hasActiveFailOn ) { + if (!await gate.canWrite()) { + return; + } + try { await octokit.pulls.dismissReview({ owner: context.repository.owner, @@ -898,6 +979,7 @@ async function finalizeWorkflow( shouldFailAction: boolean, failureReasons: string[], canResolveStale: boolean, + gate: ReviewFeedbackGate, triggerErrors: string[] ): Promise { await dismissPreviousReviewIfResolved( @@ -905,7 +987,8 @@ async function finalizeWorkflow( context, previousReviewInfo, results, - canResolveStale + canResolveStale, + gate ); // Set outputs @@ -1487,6 +1570,7 @@ async function finalizeReportWorkflow( shouldFailAction: boolean, failureReasons: string[], canResolveStale: boolean, + gate: ReviewFeedbackGate, triggerErrors: string[], options: { failOnWriteError?: boolean } = {} ): Promise { @@ -1496,6 +1580,7 @@ async function finalizeReportWorkflow( previousReviewInfo, results, canResolveStale, + gate, { failOnWriteError: options.failOnWriteError } ); @@ -1533,6 +1618,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 +1631,12 @@ async function cleanupOrphanedComments( return []; } + const gate = new ReviewFeedbackGate(octokit, context); + + if (!await gate.canWrite()) { + return []; + } + let existingComments: ExistingComment[]; try { existingComments = await fetchExistingComments( @@ -1571,7 +1663,7 @@ 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, } ); @@ -1583,6 +1675,10 @@ async function cleanupOrphanedComments( if (allResolved) { const previousReviewInfo = await fetchPreviousReviewInfo(octokit, context); if (previousReviewInfo?.state === 'CHANGES_REQUESTED') { + if (!await gate.canWrite()) { + return findingObservations; + } + try { await octokit.pulls.dismissReview({ owner: context.repository.owner, @@ -1760,9 +1856,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, }), ); @@ -1779,7 +1876,7 @@ async function runReportMode( octokit, context, reviewPhase.fetchedComments, allFindings, reviewPhase.activeWardenCommentIds, canResolveStale, inputs.anthropicApiKey, - auxiliaryOptions, + auxiliaryOptions, gate, { failOnWriteError: true }, ); resolveSpan.setAttribute( @@ -1800,6 +1897,7 @@ async function runReportMode( reviewPhase.findingObservations, reviewPhase.shouldFailAction, reviewPhase.failureReasons, canResolveStale, + gate, triggerErrors, { failOnWriteError: true }, ); @@ -1940,13 +2038,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), ), ); @@ -1966,7 +2065,7 @@ export async function runPRWorkflow( octokit, context, reviewPhase.fetchedComments, allFindings, reviewPhase.activeWardenCommentIds, canResolveStale, inputs.anthropicApiKey, - auxiliaryOptions, + auxiliaryOptions, gate, ); resolveSpan.setAttribute( 'warden.feedback.auto_resolve.fix_eval_count', @@ -1987,6 +2086,7 @@ export async function runPRWorkflow( reviewPhase.findingObservations, reviewPhase.shouldFailAction, reviewPhase.failureReasons, canResolveStale, + gate, triggerErrors, );