Skip to content

fix(review): re-review after a force-push instead of trusting the re-anchored review SHA - #884

Open
tend-agent wants to merge 5 commits into
mainfrom
hourly/review-31160773238-prql
Open

fix(review): re-review after a force-push instead of trusting the re-anchored review SHA#884
tend-agent wants to merge 5 commits into
mainfrom
hourly/review-31160773238-prql

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

A force-push doesn't just move a PR's head — GitHub also re-points the prior review's .commit_id at the new head. tend-review's pre-flight guard compares that field against HEAD_SHA to decide whether the current commit has already been reviewed, so after a rewrite it reads LAST_REVIEW_SHA == HEAD_SHA, exits silently, and leaves a stale APPROVE standing as the active verdict on code the bot never read.

Observed

PRQL/prql#6158 (dependabot, js-yaml bump). prql-bot submitted an empty-body APPROVE at 06:09:36Z; the head at that moment was abb3853a — confirmed by the tend-mention run the review itself triggered six seconds later, which carries headSha=abb3853a. Dependabot then rebased at 07:40:18Z (head_ref_force_pushedcc020720), which re-triggered tend-review 31158576591. The agent ran the full 1 m 47 s and posted nothing.

The reviews endpoint now reports that 06:09:36Z review against a commit created at 07:40:16Z:

{"id":4880377370,"state":"APPROVED","submitted_at":"2026-08-07T06:09:36Z",
 "commit_id":"cc0207205f99c5481b8404c805a16a43c91fb3af","body":""}

The session log shows the guard firing on exactly that, and the reasoning is correct given what it was told to read:

The current HEAD has already been reviewed and approved. There are no conversation comments, no unanswered questions directed at the bot, and no unresolved bot review threads to resolve. The triggering event was synchronize, not ready_for_review, so no exception applies. Exiting silently without posting — the existing approval stands as the active verdict.

Control

An ordinary push leaves the old anchor intact, so .commit_id alone cannot distinguish the two cases. On PRQL/prql#6159, a review submitted at 06:58:45Z still reports commit_id=779b096d after 177c1997 landed on top — the guard works there, which is why this has stayed invisible.

Fix

Probe the timeline for a head_ref_force_pushed newer than the last substantive bot review. Non-zero ⇒ the reviewed commit was rewritten away, so ignore LAST_REVIEW_SHA and review the head in full. The incremental path has to be skipped too: after a rewrite LAST_REVIEW_SHA names the current head, so LAST_REVIEW_SHA..HEAD_SHA is empty and every trivial-skip heuristic keyed on it under-reports the change.

The existing LAST_REVIEW_SHA extraction is refactored to keep the whole review record rather than just .commit_id, so submitted_at comes from the same substantive-filtered pick — no second query, no risk of the two fields resolving to different reviews.

Verified live against three shapes: #6158 (force-pushed after approval) → 1; #6159 (ordinary push) → 0; #6161 (no prior bot review) → empty anchor, 0.

Gates

  • Evidence level: High, structural — no decision point. Every force-push after a bot review re-points the anchor and defeats the comparison identically.
  • Occurrences: 2 for the failure class. This one, plus #828 — the same LAST_REVIEW_SHA == HEAD_SHA guard skipping a real commit because the anchor had moved to something the bot never reviewed (fixed for the reply-container mechanism in #835; the force-push mechanism is untouched by that fix).
  • Magnitude: targeted fix — one probe plus one condition, no new section. Normal bar.
  • Wrong-outcome shape, beyond the skipped work: the review record reads "approved cc020720" for a commit the bot never fetched, and the sequence generalizes to approve-then-rewrite.

Evidence log: https://gist.github.com/192514ea2c36586f9b7f842a482d62ab

@tend-agent tend-agent added the claude-behavior Behavioral issues found by review-reviewers label Aug 7, 2026
Step 5's ALREADY_POSTED guard compares .commit_id against $HEAD_SHA the same way step 1 did, so the re-anchored review it now skips past at pre-flight was still discarding the re-review at the last step. Filter out reviews older than the newest head_ref_force_pushed, and make the step-1 probe's guard an if-block so the block does not exit 1 when there is no prior review.

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as a self-authored PR, so this is a COMMENT. I re-derived the mechanism independently rather than taking the PR body's word for it: repos/{owner}/{repo}/issues/{n}/timeline returns head_ref_force_pushed with no preview header, and running the new step-1 snippet live gives FORCE_PUSHED=1 on PRQL/prql#6158 (where LAST_REVIEW_SHA == HEAD_SHA for a commit created 90 minutes after the review), 0 on #6159, and 0 with an empty anchor on this PR. The last // {} refactor degrades correctly to an empty LAST_REVIEW_SHA when there is no prior substantive review, and max // "" handles a PR with no rewrite. The three guards are the three sites that consume the anchor; I found no fourth in this skill.

Two things, neither blocking.

The stale APPROVE is re-read but never retracted. The fix makes the bot review the rewritten head, which is the larger half. But when the re-review comes back with findings it posts a COMMENT, and the re-anchored APPROVE keeps standing on the new head — #6158 still shows prql-bot's APPROVED as its only review, active on cc020720, right now. So the "stale APPROVE standing as the active verdict" half of the reported harm survives in exactly the case that matters most: a rewrite that introduced a problem. Step 6 already owns the dismissal recipe, so it's one clause; inline suggestion below.

weekly has the same anchor comparison, unpatched. Its dependency-PR step guards with LAST_APPROVAL_SHA from [.reviews[] | select(... .state == "APPROVED")] | last | .commit.oid and skips when it equals HEAD_SHA. gh pr view --json reviews re-anchors identically to the REST field — running that exact snippet against #6158 returns cc0207205f99c5481b8404c805a16a43c91fb3af for both, i.e. "Already approved on this commit; skipping" for a commit it never approved. That population is the one tend rewrites on purpose: nightly posts @dependabot recreate and ticks renovate's rebase-check on conflicted bot PRs, both of which force-push. It's a genuinely separate concern from this diff, so it wants its own PR rather than a fold-in here — flagging it so it doesn't get lost.

Verification notes
# step-1 snippet, run live
PRQL/prql#6158: LAST_REVIEW_SHA=cc020720 AT=2026-08-07T06:09:36Z HEAD=cc020720 FORCE_PUSHED=1
PRQL/prql#6159: LAST_REVIEW_SHA=779b096d AT=2026-08-07T06:58:45Z HEAD=177c1997 FORCE_PUSHED=0
max-sixty/tend#884: LAST_REVIEW_SHA=<none>  AT=<none>            HEAD=054d6b97 FORCE_PUSHED=0

# weekly's snippet, run live against the same PR
HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."

Not findings, checked and cleared: add reduces correctly across --paginate pages at all three sites; select(.event == ... and .created_at > $at) is safe on timeline events that carry no created_at; a failed timeline fetch yields $fp == "" and falls back to the pre-PR behaviour rather than dropping a real review; no CHANGELOG entry is expected, since the release skill compiles that section at release time.

Comment thread plugins/tend-ci-runner/skills/review/SKILL.md Outdated
@tend-agent

Copy link
Copy Markdown
Collaborator Author

weekly's anchor comparison is now #890, kept out of this diff as suggested. I re-derived it rather than taking the review's word: #6158 still returns LAST_APPROVAL_SHA == HEAD_SHA == cc020720 for an approval submitted 90 minutes before that commit existed, so the guard skips the rebased PR and the unearned APPROVED stands. Same timeline probe as here; the switch from gh pr view --json reviews to REST is forced by submitted_at being absent from the GraphQL projection. Controls #6157 and #6156 (approved, never rewritten) still skip, so the redundant-approval suppression the guard exists for is intact.

The dismissal clause is in eb57838 — details in the inline reply.

…3238-prql

# Conflicts:
#	plugins/tend-ci-runner/skills/review/SKILL.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-behavior Behavioral issues found by review-reviewers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant