Skip to content

fix(weekly): don't trust a re-anchored approval on a rebased dependency PR - #890

Open
tend-agent wants to merge 4 commits into
mainfrom
fix/weekly-force-push-anchor-31163236136
Open

fix(weekly): don't trust a re-anchored approval on a rebased dependency PR#890
tend-agent wants to merge 4 commits into
mainfrom
fix/weekly-force-push-anchor-31163236136

Conversation

@tend-agent

@tend-agent tend-agent commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

weekly's dependency-PR step skips approving when LAST_APPROVAL_SHA == HEAD_SHA. A force-push doesn't just move the head — GitHub re-points the prior review's commit anchor at the new head, so after a rebase that comparison reads true for a commit the bot never read. The PR is left carrying an APPROVED it never earned, and the one step that would have re-checked it declines to run.

Dependency PRs are the population tend rewrites on purpose: nightly posts @dependabot recreate and ticks renovate's rebase-check on conflicted bot PRs, both of which force-push.

Observed

PRQL/prql#6158 (dependabot, js-yaml bump). prql-bot approved at 06:09:36Z; dependabot rebased at 07:40:18Z. Running the current snippet against it now:

HEAD_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
LAST_APPROVAL_SHA=cc0207205f99c5481b8404c805a16a43c91fb3af
-> "Already approved on this commit; skipping."

cc020720 was created 90 minutes after that approval.

Fix

Take the newest head_ref_force_pushed off the timeline and drop approvals older than it, mirroring the probe #884 adds to review's three anchor sites. Switching from gh pr view --json reviews to the REST endpoint is what makes the filter expressible: submitted_at isn't in the GraphQL projection, and gh api --jq takes no --arg, so the comparison pipes to jq. Note the field rename that comes with it — REST carries .commit_id, not .commit.oid.

The equality test also gains an -n guard: with no surviving approval both sides can now be empty, and [ "" = "" ] would skip.

Fixing the approve path alone would leave the stale APPROVED standing on the two paths that don't reach it — step 2's "CI is failing, comment and skip" and "major version bump, comment and skip". Both are reachable precisely because a rebase changed something, so a rebased-into-red dependency PR would get a failure comment while still reading as bot-approved. A new item 6 dismisses any approval older than the newest rewrite on those paths, using the same reviews/$REVIEW_ID/dismissals call #884 gives review. It's idempotent — a dismissed review reports DISMISSED, so the filter stops matching it.

Verified live

PR shape before after
#6158 approved, then force-pushed skip proceed
#6157 approved, no rewrite skip skip
#6156 approved, no rewrite skip skip

The two controls confirm the redundant-approval suppression the guard exists for is intact; only the rewritten case flips.

The dismissal filter resolves against the same three: #6158 selects review 4880377370 (prql-bot, APPROVED, submitted_at 06:09:36Z, commit_id cc020720 — the post-rewrite head, re-anchored), and both controls select nothing.

Flagged in the review of #884 as a separate concern from that diff, which patches review only.

@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.

Checked the mechanism against the three PRs in the description: PRQL/prql#6158's lone approval is stamped 2026-08-07T06:09:36Z against cc020720, a commit the head_ref_force_pushed at 07:40:18Z created — the new filter drops it, and the two controls still skip. review and weekly are the only two anchor-comparison sites in the plugin, so with #884 that closes the set.

One gap the diff leaves open: only the approve path resolves a re-anchored approval. Step 2's items 4 and 5 — "If CI is failing, comment with the failure summary and skip" and "If a major version bump, comment noting it needs manual review and skip" — never reach the guard, and both are reachable precisely because a rebase changed something. So a dependency PR that was approved, then rebased into red CI, gets a failure comment while the pre-rewrite APPROVED stays the bot's latest review and the PR still reads as bot-approved — the state the title is about. Either dismiss the pre-rewrite approval on those paths (reviews/$REVIEW_ID/dismissals, as #884 does for review), or say in the body that it's deliberately a follow-up.

@tend-agent

Copy link
Copy Markdown
Collaborator Author

Good catch — dismissing, not deferring. Pushed as 6281a54.

Items 4 and 5 now carry an item 6 that pulls the newest head_ref_force_pushed off the timeline and dismisses any bot APPROVED older than it, via the same reviews/$REVIEW_ID/dismissals PUT #884 gives review. It's self-contained rather than reusing item 3's variables — items 3, 4 and 5 are alternative branches, so nothing item 3 computed is in scope on a skip path.

Resolving against the same three PRs the description uses: #6158 selects review 4880377370prql-bot, APPROVED, submitted 06:09:36Z, commit_id cc0207205f99c5481b8404c805a16a43c91fb3af, i.e. re-anchored onto the head the 07:40:18Z rewrite created — and both controls select nothing, so a PR that was never rewritten keeps its approval.

Re-running is a no-op: dismissal moves the state to DISMISSED, which the .state == "APPROVED" filter no longer matches.

Filter output
#6158  force_push=2026-08-07T07:40:18Z  stale_approval_id=4880377370
#6157  force_push=none                  stale_approval_id=none
#6156  force_push=none                  stale_approval_id=none

@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.

Item 6 closes the gap the last review flagged. Re-ran both filters live against the three PRs in the description: #6158 selects 4880377370 for dismissal and yields no surviving approval for the item 3 guard; #6157 and #6156 select nothing and keep their approvals anchored at 69d5b27c / 9b6a2e78. The issues/<n>/timeline call returns head_ref_force_pushed without a preview header, and item 3 and item 6 are exact complements (> $fp / < $fp), so no approval falls through both. #882 touches the adjacent else branch in this same block but git merge-tree merges the two cleanly.

Two things on item 6's snippet, both inline.

The selector orders select before last, so it picks the newest stale approval rather than testing whether the newest approval is stale. Given [APPROVED@06:09 (pre-rewrite), APPROVED@08:00 (post-rewrite)] and a rewrite at 07:40, it returns the 06:09 id — dismissing a review that a valid post-rewrite approval already superseded, while the 08:00 approval that determines the PR's state stays untouched. Swapping the order returns empty there and is unchanged on all three verification PRs.

The dismissal message says "re-reviewing", but items 4 and 5 are the comment-and-skip paths — nothing re-reviews. A maintainer reading the dismissal on a dependabot PR is told a review is coming that never arrives.

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

Copy link
Copy Markdown
Collaborator Author

Both fixed in f3a5bed.

Selector order. last now runs before the staleness test, so the question is whether the newest approval is stale rather than whether any stale approval exists. Reproduced your case — two bot approvals at 06:09 and 08:00 with a rewrite at 07:40: the old form returns the 06:09 id, the new form returns empty. All three verification PRs are unchanged (#6158 still selects 4880377370, controls still select nothing). A comment above the snippet records why the order matters, since the two forms look interchangeable.

Dismissal message. Now Rebased since this approval; the new head is unreviewed. — items 4 and 5 comment and stop, so the old wording promised a re-review that never comes.

Selector comparison
live:       #6158 fp=2026-08-07T07:40:18Z stale=4880377370
            #6157 fp=none                 stale=none
            #6156 fp=none                 stale=none

synthetic:  approvals @06:09 (id 1) and @08:00 (id 2), rewrite @07:40
            select-then-last -> 1     (dismisses the superseded review)
            last-then-select -> none  (the newest approval post-dates the rewrite)

@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.

Both findings from the last review are cleared in f3a5bed. I re-ran the reordered selector rather than take the claim on trust — it returns the id on a lone pre-rewrite approval, empty when a post-rewrite approval supersedes it, empty on an empty review list, and empty when $fp is unset. The . != null the commit added beyond the suggested form is what keeps that empty-list case from reaching .submitted_at.

One thing worth recording since the two snippets now look inconsistent: item 3's LAST_APPROVAL_SHA filter keeps the original select-then-last order, and that is not a second instance of the same bug. submitted_at is monotonic across the REST reviews list, so item 3's .submitted_at > $fp predicate selects a suffixlast of the suffix is last of the whole list whenever either is non-empty, making the two orders equivalent there. Item 6's < $fp selects a prefix, which is precisely why the order changed the answer there and not here.

Self-authored, so this stands as a COMMENT — no approval is coming from this reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant