Skip to content

fix(queue): terminalize active-review tracking in synchronize-amendment close guard - #8044

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-synchronize-amendment-terminalize-8015
Jul 22, 2026
Merged

fix(queue): terminalize active-review tracking in synchronize-amendment close guard#8044
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-synchronize-amendment-terminalize-8015

Conversation

@rsnetworkinginc

Copy link
Copy Markdown
Contributor

Summary

src/queue/review-evasion.ts has six close-enforcement guards. Four of them —
closeReviewEvasionSelfCloseIfReviewed, closeReviewEvasionDraftConversionIfReviewed,
closeRepeatedDraftCyclingIfDetected, and closeDraftPrIfPolicyEnabled — call
terminalizeActiveReviewTracking(env, repoFullName, pr.number, { onlyIfHeadSha: pr.headSha })
right after a successful enforcement close, so the PR's active-review tracking row is marked
terminal rather than left dangling.

closeSynchronizeAmendmentIfPolicyEnabled (maybeCloseSynchronizeAmendment, the
#synchronize-close-policy guard) closed the PR and recorded its audit event but never made
that cleanup call — leaving the active-review row for the PR's reviewed head dangling. This is
the guard most likely to fire while a review is still actively tracked (it fires on the
author pushing an additional commit, "regardless of what CI/review state that push
interrupts"), yet it was the one guard skipping the cleanup. The skip had no justification
anywhere in the file, unlike the separately-documented, intentional omission of the
moderation-strike call for this guard (which is left untouched).

What changed

  • src/queue/review-evasion.ts: added the identical
    await terminalizeActiveReviewTracking(env, repoFullName, pr.number, { onlyIfHeadSha: pr.headSha }).catch(() => undefined);
    call (with the same v8 ignore next best-effort annotation the four siblings use) as the
    final step of closeSynchronizeAmendmentIfPolicyEnabled, in the same position relative to
    the close/audit-event calls as its siblings. No other behavior changed; the intentional
    moderation-strike omission is unaffected.
  • test/unit/queue-lifecycle-guards.test.ts: added a test in the existing
    one-shot synchronize-amendment close policy (#synchronize-close-policy) block that starts
    active-review tracking on the PR's head, drives a synchronize-amendment enforcement close,
    and asserts the tracking row is left terminal (no dangling active row) — mirroring how the
    self-close / draft-conversion / draft-cycling / draft-PR sibling tests assert their own
    terminalizeActiveReviewTracking call.

Tests

  • Full test/unit/queue-lifecycle-guards.test.ts suite: 224 passed (includes the new test).
  • The new test asserts hasActiveReviewForHeadSha(...) is true before the close and false
    after — the row is terminalized exactly as with the four sibling guards.

Verified locally

  • npm run typecheck — clean.
  • npx turbo run build --filter=@loopover/engine — clean (fresh --force run).
  • git diff --check origin/main — clean.
  • Rebased on current origin/main; git merge-base --is-ancestor origin/main HEAD — true.
  • Coverage (codecov-style, on the changed file): the single added source line is best-effort
    cleanup annotated v8 ignore next (matching all four siblings); no added line is uncovered,
    100% patch coverage on the diff.
  • Secret-scan clean: no secret/private-key/token/credential literal in the diff.

Closes #8015

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 22, 2026
@loopover-orb

loopover-orb Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-22 15:06:22 UTC

2 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds the identical `terminalizeActiveReviewTracking` cleanup call to `closeSynchronizeAmendmentIfPolicyEnabled` that its four sibling close-enforcement guards already have, closing the gap flagged in issue #8015 where a dangling active-review row was left behind on this specific enforcement path. The change is a two-line, mechanically-correct addition placed in the identical position (after the audit event, same `onlyIfHeadSha` guard and `v8 ignore` annotation) as the four existing call sites, and the accompanying test exercises the real path: it starts tracking on the PR's headSha, drives the synchronize-amendment close through `processJob`, and asserts the row is left terminal. This is a narrow, well-targeted fix with no scope creep.

Nits — 2 non-blocking
  • The `long-file` signal on `review-evasion.ts` (~1370 lines) is a pre-existing condition unrelated to this diff, but worth flagging since the file continues to grow with each near-identical guard.
  • Given five of six guards now share this exact terminalize-call shape (comment + call + catch), consider a tiny shared helper the next time one of these guards changes, though it's not worth doing in this PR alone.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8015
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 4 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor rsnetworkinginc; Gittensor profile; 14 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds the exact terminalizeActiveReviewTracking call with the same signature and best-effort catch pattern used by the four sibling guards, placed after the close/audit-event calls as required, and includes a test verifying the active-review row is left terminal after a synchronize-amendment close.

Review context
  • Author: rsnetworkinginc
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 14 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit c2c8697 into JSONbored:main Jul 22, 2026
10 checks passed
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@75daa84). Learn more about missing BASE report.
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8044   +/-   ##
=======================================
  Coverage        ?   94.59%           
=======================================
  Files           ?        1           
  Lines           ?      259           
  Branches        ?      116           
=======================================
  Hits            ?      245           
  Misses          ?        3           
  Partials        ?       11           
Flag Coverage Δ
shard-1 3.86% <ø> (?)
shard-2 94.59% <ø> (?)
shard-3 4.24% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/review-evasion.ts 94.59% <ø> (ø)

@JSONbored JSONbored added the gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. label Jul 23, 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. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

closeSynchronizeAmendmentIfPolicyEnabled skips terminalizeActiveReviewTracking, unlike its four sibling close-enforcement guards

2 participants