Honor GitHub approvals carried over a clean master merge#469
Merged
Conversation
A GitHub native "Approve" review (mapped to a CR signoff in #463) carries over a clean master merge — GitHub only dismisses the review when the diff actually changes. Pulldasher dropped the CR anyway: - git-manager marked any CR/QA whose timestamp predated the head commit inactive. An approval's `submitted_at` legitimately predates a later merge commit, so the full refresh re-killed it. - the `synchronize` webhook blanket-invalidated CR/QA and never re-derived review state, and there is no periodic poll, so the approval stayed dropped until restart. Tag approval-derived CR signatures (`fromGithubApproval`) and exempt them from the commit-date staleness check — GitHub is the source of truth for whether the approval is still valid. Follow the `synchronize` invalidate with a full refresh so a carried-over approval is restored on the next push. The immediate invalidate stays for snappy UI feedback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
QA 🟢 |
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
2 participants
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.
Summary
Follow-up to #463. A GitHub native Approve review (now mapped to a CR signoff) carries over a clean master merge — GitHub only dismisses the review when the diff actually changes. Pulldasher dropped the CR anyway, so GitHub showed the PR approved while the Pulldasher CR bubble was empty.
Root cause
Two paths killed the carried-over approval:
lib/git-manager.js— the full refresh marks any CR/QA whose timestamp predates the head commit inactive. An approval'ssubmitted_atlegitimately predates a later merge commit, so every refresh re-killed it.controllers/githubHooks.js(synchronize) — a master merge fires onlysynchronize, which blanket-invalidated CR/QA and never re-derived review state. With no periodic poll (only startuprefresh.openPulls), the approval stayed dropped until restart.Fix
fromGithubApproval(models/signature.js), in-memory only — not persisted.lib/git-manager.js). GitHub is the source of truth: if the review is stillAPPROVEDat refresh time, the approval is valid; if a commit should kill it, GitHub returns a non-APPROVEDstate andparseReviewsynthesizes no CR.synchronizeinvalidate, run a fullrefresh.pullto re-derive review state (controllers/githubHooks.js). The immediate invalidate stays for snappy UI; the refresh refills only the still-valid approval.Tradeoff
One extra full refresh per push per tracked PR. The ETag cache makes these mostly
304s, so no rate-limit hit.Known edge (unchanged)
A reviewer who both approves natively and writes
CR :emoji:→ dedup keeps the emoji CR (not exempt), so it still invalidates on merge. Pre-existing dedup behavior, left as-is.Test plan
npm test— assertsfromGithubApprovaltrue for native approval, false for emoji CRnpm run lint