fix(queue): record the real outcome when a reopen-reclose fails - #2405
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 00:48:55 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 4 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 #2405 +/- ##
=======================================
Coverage 95.90% 95.91%
=======================================
Files 224 224
Lines 25217 25221 +4
Branches 9169 9172 +3
=======================================
+ Hits 24185 24190 +5
+ Misses 419 418 -1
Partials 613 613
🚀 New features to boost your workflow:
|
23c2b51 to
9794a5a
Compare
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.
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.
307a525 to
aa3fe24
Compare
What
In
maybeRecloseDisallowedReopen, both the warning comment and the actual close call were wrapped in.catch(() => undefined). Regardless of whether either GitHub API call succeeded or failed (403 from reduced permissions, 404, 5xx), the function unconditionally wrote agithub_app.reopen_reclosedaudit event withoutcome: "completed", describing the PR as re-closed. This mirrors the already-fixed draft-dodge audit-fidelity gap (#2134), on the reopen-abuse path instead.The practical impact is bounded (the next sweep still picks the PR back up and runs it through the full guard stack regardless of this path's own state), but the audit trail becomes misleading — an operator or dashboard reading
github_app.reopen_reclosed/outcome: "completed"believes the one-shot close was enforced when it may not have been.Fix
Captures the close call's settled result (
.then(() => null).catch((error) => error)) and branches the audit outcome on it:"completed"only whenclosePullRequestactually resolves,"error"otherwise (therecordAuditEventoutcome enum doesn't have a"failed"literal —"error"is the closest valid match), with the underlying error captured inmetadata. The courtesy comment's own failure is independent and still swallowed unconditionally — a failed notice doesn't change whether the close itself worked, and shouldn't be conflated with it.The handler's
return true(meaning "I own this decision, don't fall through to normal re-review") is unchanged either way — the issue's own scope is specifically the audit outcome, not the control-flow decision.Tests
outcome: "error", a detail string starting with "FAILED to re-close", and the underlying error inmetadata_json. Confirms the close was still attempted and the webhook delivery is still marked processed (the handler still owns the decision).outcome: "completed"assertion to the pre-existing success-path test (previously only checkeddetail) to lock in "a successful close is unaffected."npx tsc --noEmitclean.queue.test.ts— 203 passed.src/queue/processors.ts: fully covered.npm run test:coverage: 5631 passed, 4 skipped (pre-existing/unrelated), 0 failed.npm audit --audit-level=moderate: 0 vulnerabilities.Advances #1936. Closes #2260.