fix(warden): Prevent stale PR feedback writes#424
Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 <noreply@anthropic.com>
dcramer
force-pushed
the
fix/warden-stale-pr-feedback
branch
from
93c3bdf to
ab4c874
Compare
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5b5bf2f. Configure here.
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 <noreply@anthropic.com>
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
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 <noreply@anthropic.com>
…nges 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warden verifies that a workflow run is still analyzing the current PR head before mutating PR review feedback: posting reviews, resolving threads, replying, dismissing blocking reviews, and no-trigger cleanup. An older run completes its Checks without touching current PR review state.
The check lives in a single `ReviewFeedbackGate` that every write path shares. It retries transient GitHub errors, memoizes results for a short TTL so bursts of writes share one `pulls.get` call, and distinguishes two failure modes:
The gate also runs before fix evaluation so a stale run skips the LLM spend, not just the writes it would produce, and the "skipping due to trigger failures" log now fires only for trigger failures.
Non-blocking findings that cannot be attached as inline review comments stay in Checks instead of becoming body-only reviews that Warden cannot resolve later, and they are recorded with a new `no_inline_location` skipped reason in the findings output — previously they were either marked `posted` without reaching the PR or dropped from the observation stream entirely. Blocking REQUEST_CHANGES reviews keep the body fallback so the existing review lifecycle can still dismiss them.
The GitHub docs, README, and public llms.txt now describe Warden as Checks-first with eligible inline PR comments.