fix(queue): re-verify live PR state before draft-dodge and reopen-reclose closes - #2369
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-01 21:26:48 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2369 +/- ##
=======================================
Coverage 95.83% 95.83%
=======================================
Files 224 224
Lines 25006 25023 +17
Branches 9094 9100 +6
=======================================
+ Hits 23964 23981 +17
Misses 428 428
Partials 614 614
🚀 New features to boost your workflow:
|
…lose closes Both the converted_to_draft draft-dodge handler and the reopen-reclose handler (maybeRecloseDisallowedReopen) go straight from their async DB reads (getGateBlockOutcome/isGlobalAgentFrozen, or the permission/closer-history lookups) to closePullRequest with no live GET first — unlike the main gate-close path, which routes every close through executeAgentMaintenanceActions's freshness guard immediately before the mutation. In the window between webhook ingestion and the final PATCH, a maintainer could merge/close the PR themselves, or a fresh push could clear the underlying gate failure, and the close would still fire blind off the stale ingestion-time payload. A queued github-webhook job retried after a transient failure hits the same gap, since retries replay the originally captured payload rather than re-fetching. Add a fetchPullRequestFreshness call immediately before each closePullRequest, mirroring executeAgentMaintenanceActions's existing freshness step, and deny the close with a "denied" audit outcome when the PR's live state or head SHA no longer matches what triggered the handler.
…nds on, not just head/state Both live re-checks added for the draft-dodge and reopen-reclose close paths only proved the PR was still open on the same head — but a same-head, still- open PR can undergo a live transition that invalidates the SPECIFIC justification each handler is acting on: - draft-dodge: the author could convert the PR back to ready_for_review before the close fires, clearing the very draft state the close is supposed to be punishing. - reopen-reclose: the reopener could be promoted to a write/maintain/admin collaborator in the window between the initial permission read and the close, retroactively authorizing the reopen this handler is about to undo. fetchPullRequestFreshness gains an opt-in requireDraft check (used only by the draft-dodge call site; every other caller's behavior is unchanged), and the reopen-reclose handler re-runs its own hasMaintainerPermission check immediately before the mutation instead of relying solely on the initial read.
01bb95d to
6f94152
Compare
The new outcome:error regression test's /issues/42/events mock lacked a "reopened" event for the payload's actual reopener, so the #2369 live recheck #3 (reopenerSuperseded) denied the close before ever reaching the PATCH-fails path this test targets. Add the missing "reopened" event, matching the pattern already used by every other reopen-reclose test in this file.
* fix(queue): record the real outcome when a reopen-reclose fails Both the warning comment and the actual close call in maybeRecloseDisallowedReopen were wrapped in .catch(() => undefined), but the function unconditionally wrote a github_app.reopen_reclosed audit event with outcome:"completed" regardless of whether the close API call actually succeeded. A 403 from reduced permissions, a 404, or a transient 5xx was silently swallowed while the audit ledger kept recording a successful re-close — an operator trusting the audit trail would believe the one-shot close was enforced when the PR may still be open. This mirrors the same audit-fidelity gap already fixed on the draft-dodge path. Capture the close call's settled result and branch the audit outcome on it: "completed" only when closePullRequest actually resolves, "error" otherwise, with the underlying error captured in metadata. The courtesy comment's own failure still never affects this — it's independent of whether the close succeeded. * test(queue): fix reopen-reclose CI failure test's stale event timeline The new outcome:error regression test's /issues/42/events mock lacked a "reopened" event for the payload's actual reopener, so the #2369 live recheck #3 (reopenerSuperseded) denied the close before ever reaching the PATCH-fails path this test targets. Add the missing "reopened" event, matching the pattern already used by every other reopen-reclose test in this file.
What
Both the
converted_to_draftdraft-dodge handler and the reopen-reclose handler (maybeRecloseDisallowedReopen) go straight from their async DB reads (getGateBlockOutcome/isGlobalAgentFrozen, or the permission/closer-history lookups) toclosePullRequestwith no live GET first — unlike the main gate-close path, which routes every close throughexecuteAgentMaintenanceActions's freshness guard immediately before the mutation.In the window between webhook ingestion and the final PATCH, a maintainer could merge/close the PR themselves, or a fresh push could clear the underlying gate failure, and the close would still fire blind off the stale ingestion-time payload. A queued
github-webhookjob retried after a transient failure hits the same gap, since retries replay the originally captured payload rather than re-fetching (up to 3 retries, 30s delay per attempt — a real window).Fix
Add a
fetchPullRequestFreshnesscall immediately before eachclosePullRequest, mirroringexecuteAgentMaintenanceActions's existing freshness step, and deny the close with adeniedaudit outcome when the PR's live state or head SHA no longer matches what triggered the handler. Both call sites are structurally identical bypasses of the same pattern, so this applies the same fix to both.Tests
recordAuditEventfailure on the new denial path is swallowed — the handler still completes without throwing (mirrors the existing fail-safe convention for this file's other.catch()bodies).Full unsharded
test:coveragegreen (5604 passed);typecheckgreen;npm auditclean.Advances #1936. Closes #2130. Closes #2261.