gate(eligibility): hold PRs against a priority issue until its window opens (#9738) - #9847
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | e487193 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 06:58 PM |
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 19:14:07 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 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.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9847 +/- ##
========================================
Coverage 91.75% 91.75%
========================================
Files 919 920 +1
Lines 112987 113098 +111
Branches 27193 27224 +31
========================================
+ Hits 103667 103777 +110
- Misses 8034 8035 +1
Partials 1286 1286
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
force-pushed
the
fix/linked-issue-rules-per-repo
branch
from
July 29, 2026 15:15
b22ca4b to
4ff24d0
Compare
Bundle ReportChanges will increase total bundle size by 93.87kB (1.2%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
JSONbored
force-pushed
the
fix/linked-issue-rules-per-repo
branch
from
July 29, 2026 15:38
4ff24d0 to
83d67e0
Compare
JSONbored
force-pushed
the
fix/linked-issue-rules-per-repo
branch
from
July 29, 2026 16:01
83d67e0 to
b2d98c5
Compare
… opens (#9738) Priority issues carry the highest payout, so assignment fairness matters most there -- and first-come pickup is only fair if everyone can SEE the issue before anyone can act on it. A PR opened moments after the label lands means the window between "issue becomes valuable" and "issue is claimed" was effectively zero for everyone else watching the repo. A PR closing a `gittensor:priority` issue is now gate-eligible only once the label has been publicly present for `gate.priorityEligibilityWindow` minutes (default 30, per repo, `0` disables it). A PR arriving inside the window is NOT rejected: it is HELD with a neutral comment naming the moment it becomes eligible, and proceeds normally once the window elapses. No penalty beyond waiting. Two decisions worth stating: - The clock is anchored to the EARLIEST labeling event, not the most recent. The spec requires that re-applying the label not reset the window for already-open PRs; an earliest anchor gives that to everyone and makes "when does this issue open for work" a single knowable instant nothing can move. - Every unknown FAILS OPEN -- an unreadable timestamp, an absent label, a GraphQL error, a missing token. Holding a contributor's PR on a fact we could not read is a penalty for our own gap. The hold reuses the existing merge-hold rail (`heldForManualReview`), so it can never close a PR. Merge holds become DATA rather than a widening list of booleans. `MERGE_HOLD_INPUTS` is the one table; the input type (`Record<MergeHoldInput, boolean>`), the `heldForManualReview` fold, and both test fixtures all derive from it. Adding a hold was three independent edits that could each be forgotten -- declaring one and not folding it into the decision compiled fine. It is now one entry, and omitting the wiring is a compile error at the single call site. `normalizeOptionalNonNegativeInteger` exists because `0` is meaningful here (it turns the rule off) and the positive-integer normalizer would have discarded it as invalid and silently applied the default -- an operator's explicit "off" becoming "on". 100% statements and branches on both changed source files.
…ig round-trip it exposed The GraphQL label-timestamp read had no tests at all: every fail-open path, the earliest-labeling anchor, and the query shape (first:, not last: -- using last: would return the most RECENT labeling, which is exactly the value re-applying the label could use to push a contributor's window back) are now pinned. Writing the manifest test surfaced a real gap: gateConfigToJson never emitted priorityEligibilityWindow, so the setting did not round-trip. resolvePriorityEligibilityHold now DEFAULTS fetchLabeledAt to the real GraphQL read rather than returning undefined when none is injected -- its doc comment already claimed that default existed. The production caller passes facts only, and a new test proves the default really reaches GitHub instead of every case passing on an injected fake. resolvePriorityTypeLabel replaces the ad-hoc `typeLabels?.priority ?? DEFAULT` each caller spelled for itself. The label-author rule (#9737) and this window act on the same label; two spellings could disagree about a repo that renamed it.
Codecov counts a /* v8 ignore */ line as unhit, so both suppressions are gone rather than carried: DEFAULT_PRIORITY_LABEL names the built-in value, so resolvePriorityTypeLabel returns it directly. PrTypeLabelSet is an open Record, which made DEFAULT_TYPE_LABELS.priority read as possibly-undefined and forced a fallback branch nothing could ever take. The .catch on resolvePriorityEligibilityHold is removed: that function catches its own GitHub read and cannot reject, so the guard was unreachable code that read as tested-and-fine while never running. The remaining branch is closed by an end-to-end test instead: a clean, approved, green PR that would MERGE is held because the priority issue it closes only became public a minute ago -- held, never closed.
JSONbored
force-pushed
the
fix/linked-issue-rules-per-repo
branch
from
July 29, 2026 18:19
cb5af26 to
0a8aacd
Compare
…INE's own suite packages/loopover-engine/src/** is credited by TWO Codecov uploads: the unflagged backend one from the root vitest run, and an "engine" flag fed by the package's own node:test suite (codecov.yml's own note: a PR touching engine source should get credit from the suite that actually behaviour-tests it). These two surfaces were only covered by the root suite, so the engine flag reported one uncovered statement per file -- which is where codecov/patch's missing 2 lines came from. A full unsharded local run (25,512 tests) confirms the backend side is already at 100%. resolvePriorityTypeLabel is exported from the engine index so its own suite can reach it, and MERGE_HOLD_INPUTS drops an export nothing outside its file used.
JSONbored
force-pushed
the
fix/linked-issue-rules-per-repo
branch
from
July 29, 2026 18:54
3eaac48 to
e487193
Compare
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
… ENGINE's own suite Same cause as the sibling fix on #9847. packages/loopover-engine/src/** is credited by two Codecov uploads, and the "engine" flag is fed by the package's own node:test suite -- so a module that only the root vitest suite exercises reads as uncovered there. author-association.ts moved INTO the engine in this PR, which makes every one of its lines new to that flag. The predicate is now behaviour-tested in the engine suite, including through buildPullRequestAdvisory, since the gate-advisory twin is one of its callers and is where the fifth duplicate copy lived.
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
… ENGINE's own suite Same cause as the sibling fix on #9847. packages/loopover-engine/src/** is credited by two Codecov uploads, and the "engine" flag is fed by the package's own node:test suite -- so a module that only the root vitest suite exercises reads as uncovered there. author-association.ts moved INTO the engine in this PR, which makes every one of its lines new to that flag. The predicate is now behaviour-tested in the engine suite, including through buildPullRequestAdvisory, since the gate-advisory twin is one of its callers and is where the fifth duplicate copy lived.
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
1 participant
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.
Closes #9738.
Why
Priority issues carry the highest payout, so assignment fairness matters most there — and first-come pickup is only fair if everyone can see the issue before anyone can act on it. A PR opened moments after the label lands means the window between "issue becomes valuable" and "issue is claimed" was effectively zero for everyone else watching the repo.
What
A PR closing a
gittensor:priorityissue is gate-eligible only once the label has been publicly present forgate.priorityEligibilityWindowminutes — default 30, per repo,0disables it.A PR arriving inside the window is not rejected. It is held, with a neutral comment naming the moment it becomes eligible, and proceeds normally once the window elapses. No penalty beyond waiting, and the contributor keeps their work. The hold rides the existing
heldForManualReviewrail, so it structurally cannot close a PR.Two decisions worth your eyes
The clock is anchored to the EARLIEST labeling event, not the most recent. The issue requires that re-applying the label not reset the clock for already-open PRs; an earliest anchor gives that to everyone and makes "when does this issue open for work" a single knowable instant that nothing later can move. The trade: a label removed and re-added months later reopens work immediately — which I think is right, since the issue was public that whole time.
Every unknown fails open — an unreadable timestamp, an absent label, a GraphQL error, a missing token. Holding a contributor's PR on a fact we could not read is a penalty for our own gap.
Merge holds are data now
Adding a hold used to be three independent edits — the input type, the
heldForManualReviewfold, and an exhaustive test fixture — each forgettable on its own. Declaring a hold and then not folding it into the decision compiled fine.MERGE_HOLD_INPUTSis now the one table. The input type (Record<MergeHoldInput, boolean>), the fold, and both test fixtures derive from it. Adding a hold is one entry, and omitting the wiring is a compile error at the single call site. I hit this the moment I added the sixth hold, which is what prompted it.normalizeOptionalNonNegativeIntegeris new because0is meaningful here: the existing positive-integer normalizer would have rejected an operator's explicit "off" as invalid and silently applied the default — turning it back on.Validation
priority-eligibility-window.ts,pr-disposition.ts), measured directly.npx vitest run --changed=origin/main: 15143 passed. The four miner CLI files that fail without a builtdist/pass once built — unrelated to this change.tscclean;docs:drift-check,ui:openapi:check,command-reference:check,manifest:drift-checkall green. The gate field is documented in both example configs, with the yml-key↔field alias declared where the drift checker expects it.Not in this PR
#9737 (author-based eligibility — strip
gittensor:priorityfrom non-maintainer-authored issues) is the epic's other half and needs a newissues.labeledwebhook path; it is its own change.