Skip to content

fix(queue): reclose ambiguous padded reopens - #2470

Merged
JSONbored merged 2 commits into
mainfrom
codex/fix-reopener-ambiguity-in-pr-closure
Jul 2, 2026
Merged

fix(queue): reclose ambiguous padded reopens#2470
JSONbored merged 2 commits into
mainfrom
codex/fix-reopener-ambiguity-in-pr-closure

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent a timing/window-evasion bypass where a contributor can pad the issue-events timeline so their disallowed reopen falls outside the bounded scan and survives the one-shot reopen-prevention policy.
  • Preserve the intended protection that a visible different reopener or a fully-covered timeline should block an automatic re-close, while avoiding the introduced fail-open ambiguity when no reopen is observed inside the bounded window.

Description

  • Change the latest-reopener guard in recloseDisallowedReopenIfNeeded so an uncovered bounded timeline with no visible reopened events no longer counts as proof the original reopen was superseded; only a fully-covered timeline with a different reopener or an observed different latest reopener denies the re-close (src/queue/processors.ts).
  • Simplify the audit detail for the superseded-reopener path to always report the current reopener when present and keep the unknown fallback when the login is null.
  • Add a regression unit test that simulates a padded multi-page events timeline where the contributor reopen is hidden beyond the scanned window and verifies the bot still comments, attempts to close, and records a completed/error audit outcome as appropriate (test/unit/queue.test.ts).
  • Keep the previously-added live re-checks and fail-safe behavior for permission reads and timeline errors intact while addressing the window-evasion case.

Testing

  • Ran focused unit tests with Vitest for the "one-shot reopen prevention" suite and the new/patched cases, and the test selections passed (npx vitest run test/unit/queue.test.ts -t "one-shot reopen prevention" and targeted -t patterns).
  • Ran npm run typecheck and it completed with no TypeScript errors.
  • Attempted the full local gate npm run test:ci but it could not finish in this environment due to external network/actionlint setup issues; npm audit --audit-level=moderate also failed to run because the registry audit endpoint returned 403 from this environment.
  • The added/modified unit tests exercise the regression scenarios and passed in the local focused runs.

Codex Task

@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 06:40:24 UTC

4 files · 1 AI reviewer · no blockers · readiness 68/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change correctly separates timeline read failures from bounded-but-successful scans, so the re-close guard no longer treats a padded window with no visible reopen as proof of supersession while still denying on API errors or a visible different current reopener. The `errored` bit is threaded through the shared issue-event lookup and the focused tests cover the new padded-window path plus the existing error path. I do not see a reachable break in the visible diff.

Nits — 6 non-blocking
  • nit: `src/github/pr-actions.ts:200` still describes `LastCloserResult` primarily as a closer/reopen-guard shape even though `LastTimelineActorResult` now carries error semantics for both closed and reopened event lookups; tightening the wording would make the shared contract less misleading.
  • nit: `test/unit/queue.test.ts:11551` uses a hard-coded fake token string in the fetch stub, which is harmless but likely to keep triggering secret-scanner noise in review tooling.
  • In `src/github/pr-actions.ts:200`, split the JSDoc into a generic timeline-result contract and smaller closer/reopener notes so future callers do not copy the wrong fail-open/fail-closed interpretation.
  • In `test/unit/queue.test.ts:11551`, use an obviously synthetic placeholder such as `test-installation-token` or centralize the fake access-token response helper used by nearby webhook tests.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 548 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 548 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 65 PR(s), 548 issue(s).
  • Related work: Titles/paths share 6 meaningful terms. (PR #2473)
  • Related work: Titles/paths share 7 meaningful terms. (PR #2477, PR #2478)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Triage stale or unlinked PRs.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

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

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.93%. Comparing base (118d537) to head (5277b93).
⚠️ Report is 34 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2470   +/-   ##
=======================================
  Coverage   95.93%   95.93%           
=======================================
  Files         225      225           
  Lines       25338    25340    +2     
  Branches     9218     9219    +1     
=======================================
+ Hits        24308    24310    +2     
  Misses        417      417           
  Partials      613      613           
Files with missing lines Coverage Δ
src/github/pr-actions.ts 100.00% <100.00%> (ø)
src/queue/processors.ts 91.74% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The reopener-supersede check collapsed two different outcomes of
getLastReopenerLogin/getLastCloserLogin into the same
{ login: null, coveredAllPages: false } shape: a bounded scan that
ran to completion and simply found no matching event in its window
(the padded-timeline case this PR fixes), and a caught read error
(network/auth/rate-limit) that proves nothing at all. Collapsing them
meant a transient GitHub API error during the reopener re-check now
re-closed a PR instead of failing closed.

getLastActorForEvent (src/github/pr-actions.ts) now returns a third
field, `errored`, set only in the catch block. The reopener-supersede
check in recloseDisallowedReopenIfNeeded (src/queue/processors.ts)
treats an errored read as superseded (deny the re-close, fail
closed) before falling through to the existing covered/bounded-window
logic, restoring the original fail-closed guarantee for genuine
errors while keeping the padded-window fix intact.

Updates every existing getLastCloserLogin/getLastReopenerLogin
assertion in test/unit/github-pr-actions.test.ts to the new three-field
shape, and reverts the reopener-timeline-read-error regression test in
test/unit/queue.test.ts to assert the safe (fail-closed) outcome
instead of locking in the unsafe re-close.

Caught by gate review on #2470.
@dosubot dosubot Bot added size:M and removed size:S labels Jul 2, 2026
@JSONbored
JSONbored merged commit 3564f67 into main Jul 2, 2026
12 checks passed
@JSONbored
JSONbored deleted the codex/fix-reopener-ambiguity-in-pr-closure branch July 2, 2026 07:01
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant