From a32144b965f4315658067b6f16632c347025af94 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 06:47:40 -0700 Subject: [PATCH 1/4] gate(eligibility): hold PRs against a priority issue until its window 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`), 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. --- .loopover.yml.example | 16 ++ apps/loopover-ui/public/openapi.json | 6 + config/examples/loopover.full.yml | 16 ++ .../loopover-engine/src/focus-manifest.ts | 25 +++ scripts/check-docs-drift.ts | 1 + src/github/backfill.ts | 40 ++++ src/openapi/schemas.ts | 3 + src/queue/processors.ts | 24 +++ src/review/priority-eligibility-window.ts | 136 +++++++++++++ src/settings/agent-actions.ts | 21 ++ src/settings/pr-disposition.ts | 45 +++-- src/signals/focus-manifest.ts | 1 + src/types.ts | 5 + test/unit/focus-manifest.test.ts | 5 +- test/unit/pr-disposition-invariants.test.ts | 19 +- test/unit/priority-eligibility-window.test.ts | 182 ++++++++++++++++++ 16 files changed, 518 insertions(+), 27 deletions(-) create mode 100644 src/review/priority-eligibility-window.ts create mode 100644 test/unit/priority-eligibility-window.test.ts diff --git a/.loopover.yml.example b/.loopover.yml.example index 15d1ab316..3f8ec53bf 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -407,6 +407,22 @@ gate: # DB-backed (dashboard-settable too); this overrides the stored value. requireFreshRebaseWindow: 10 + # Priority-issue eligibility window (#9738) — minutes a `gittensor:priority` label must have been publicly + # present before a PR that closes that issue is gate-eligible. + # + # Priority carries 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 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 clock is anchored to the EARLIEST time the label was applied, so + # re-applying it never resets the window for anyone. + # + # Whole number of minutes, 0 to 1440. `0` turns the rule off. Default: 30. Config-as-code only. + priorityEligibilityWindow: 30 + # Stale-base auto-rebase threshold. When the repository's current default branch is at least this many # commits ahead of a PR's own base commit, the pre-review readiness gate forces an update_branch before # review runs — independent of GitHub's own mergeable_state "behind" signal, which only fires when the diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 092284aa6..1aab6d0a7 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -10191,6 +10191,12 @@ "guardrailEscalationSelfConsistencyRuns": { "type": "number", "nullable": true + }, + "priorityEligibilityWindowMinutes": { + "type": "integer", + "nullable": true, + "minimum": 0, + "maximum": 1440 } }, "required": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index 30bb77ce2..afd2fc1a8 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -421,6 +421,22 @@ gate: # DB-backed (dashboard-settable too); this overrides the stored value. requireFreshRebaseWindow: 10 + # Priority-issue eligibility window (#9738) — minutes a `gittensor:priority` label must have been publicly + # present before a PR that closes that issue is gate-eligible. + # + # Priority carries 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 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 clock is anchored to the EARLIEST time the label was applied, so + # re-applying it never resets the window for anyone. + # + # Whole number of minutes, 0 to 1440. `0` turns the rule off. Default: 30. Config-as-code only. + priorityEligibilityWindow: 30 + # Stale-base auto-rebase threshold. When the repository's current default branch is at least this many # commits ahead of a PR's own base commit, the pre-review readiness gate forces an update_branch before # review runs — independent of GitHub's own mergeable_state "behind" signal, which only fires when the diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index 8c9fe7209..b731b38dc 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -209,6 +209,12 @@ export type FocusManifestGateConfig = { * (byte-identical to today) — a discrete positive-minutes count, not a score, so it is neither clamped * nor rounded; an invalid value (fractional, non-positive, non-finite) is dropped with a warning. */ requireFreshRebaseWindowMinutes: number | null; + /** `gate.priorityEligibilityWindow` (#9738): minutes a `gittensor:priority` label must have been publicly + * present before a PR closing that issue is gate-eligible. Priority issues carry the highest payout, so + * first-come pickup is only fair if everyone can see the issue before anyone can act on it. A PR inside + * the window is HELD with a neutral comment, never rejected, and proceeds once the window elapses. + * `0` disables the rule; null means the shipped default (30). */ + priorityEligibilityWindowMinutes: number | null; /** `gate.staleBaseAheadByThreshold` (#review-grounding stale-base fact): a commit count. When the repo's * current default branch is at least this many commits ahead of a PR's own base commit, the pre-review * readiness gate forces an `update_branch` (same action class as the existing `mergeableState: "behind"` @@ -1386,6 +1392,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, + priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, @@ -1837,6 +1844,7 @@ const GATE_TOP_LEVEL_KEYS = new Set([ "dryRun", "premergeContentRecheck", "requireFreshRebaseWindow", + "priorityEligibilityWindow", "staleBaseAheadByThreshold", "claMode", "cla", @@ -1941,6 +1949,8 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu dryRun: normalizeOptionalBoolean(record.dryRun, "gate.dryRun", warnings), premergeContentRecheck: normalizeOptionalBoolean(record.premergeContentRecheck, "gate.premergeContentRecheck", warnings), requireFreshRebaseWindowMinutes: normalizeOptionalPositiveInteger(record.requireFreshRebaseWindow, "gate.requireFreshRebaseWindow", warnings), + // Zero is MEANINGFUL here (it turns the rule off), so this cannot use normalizeOptionalPositiveInteger. + priorityEligibilityWindowMinutes: normalizeOptionalNonNegativeInteger(record.priorityEligibilityWindow, "gate.priorityEligibilityWindow", warnings, MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES), staleBaseAheadByThreshold: normalizeOptionalPositiveInteger(record.staleBaseAheadByThreshold, "gate.staleBaseAheadByThreshold", warnings), claMode: normalizeOptionalGateMode(record.claMode, "gate.claMode", warnings), claConsentPhrase: parsePublicSafeText(claRecord?.consentPhrase, "gate.cla.consentPhrase", warnings), @@ -2004,6 +2014,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.dryRun !== null || gate.premergeContentRecheck !== null || gate.requireFreshRebaseWindowMinutes !== null || + gate.priorityEligibilityWindowMinutes !== null || gate.staleBaseAheadByThreshold !== null || gate.claMode !== null || gate.claConsentPhrase !== null || @@ -2221,6 +2232,17 @@ export function experimentalConfigToJson(experimental: FocusManifestExperimental return out; } +/** A NON-NEGATIVE integer, for a field where zero is a real setting rather than an absence -- e.g. + * `gate.priorityEligibilityWindow: 0` deliberately turns the window off, which `normalizeOptionalPositiveInteger` + * below would reject as invalid and silently replace with the shipped default (#9738). Bounded above so a typo + * cannot set a window that never opens. */ +function normalizeOptionalNonNegativeInteger(value: JsonValue | undefined, field: string, warnings: string[], max: number): number | null { + if (value === undefined || value === null) return null; + if (typeof value === "number" && Number.isInteger(value) && value >= 0 && value <= max) return value; + warnings.push(`Manifest field "${field}" must be a whole number between 0 and ${max}; ignoring it.`); + return null; +} + /** A positive INTEGER count (not a score/confidence) — e.g. `contentLane.maxAppendedEntries` counts discrete * surfaces[] entries, so a fractional value (a likely typo) would render a nonsensical contributor-facing close * message ("append between 1 and 2.5 entries"). Rejects fractional and non-positive values alike. */ @@ -2233,6 +2255,9 @@ function normalizeOptionalPositiveInteger(value: JsonValue | undefined, field: s const MAX_CONTRIBUTOR_OPEN_ITEM_CAP = 100; +/** A day. Long enough for any deliberate cool-off, short enough that a typo cannot park work indefinitely. */ +const MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES = 24 * 60; + function normalizeOptionalContributorOpenItemCap(value: JsonValue | undefined, field: string, warnings: string[]): number | null { const parsed = normalizeOptionalPositiveInteger(value, field, warnings); if (parsed === null) return null; diff --git a/scripts/check-docs-drift.ts b/scripts/check-docs-drift.ts index c0503ab33..174d21aed 100644 --- a/scripts/check-docs-drift.ts +++ b/scripts/check-docs-drift.ts @@ -190,6 +190,7 @@ export const SETTINGS_ALIAS_MANIFEST: AliasManifestRow[] = [ { field: "aiReviewOnMerge", aliases: ["onMerge"] }, { field: "aiReviewReviewers", aliases: ["reviewers:"] }, { field: "requireFreshRebaseWindowMinutes", aliases: ["requireFreshRebaseWindow"] }, + { field: "priorityEligibilityWindowMinutes", aliases: ["priorityEligibilityWindow"] }, { field: "sizeGateMaxFiles", aliases: ["maxFiles"] }, { field: "sizeGateMaxLines", aliases: ["maxLines"] }, ]; diff --git a/src/github/backfill.ts b/src/github/backfill.ts index e60b0dab3..ff5c7b2a1 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3559,6 +3559,46 @@ export async function fetchLivePullRequestMergedAt( return result === undefined ? undefined : (result.data.merged_at ?? null); } +/** + * When a label FIRST landed on an issue, ISO-8601, or null when it never did / cannot be read (#9738). + * + * `first: N` on purpose: the eligibility window is anchored to the EARLIEST labeling so re-applying the label + * cannot reset the clock for anyone. GitHub returns timeline items chronologically, so the first LABELED_EVENT + * naming this label is the moment the issue became publicly valuable. + * + * Reads at most one page. A maintainer who has labeled and unlabeled an issue more times than that has an + * issue whose history is not what this rule is for, and a null here FAILS OPEN (no hold) rather than guessing. + */ +export async function fetchIssueLabelFirstAppliedAt( + env: Env, + repoFullName: string, + issueNumber: number, + labelName: string, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + if (!token || !labelName) return null; + const { owner, name } = repoParts(repoFullName); + if (!owner || !name) return null; + const query = `query LoopOverIssueLabeledAt { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { issue(number: ${issueNumber}) { timelineItems(first: 100, itemTypes: [LABELED_EVENT]) { nodes { __typename ... on LabeledEvent { createdAt label { name } } } } } } }`; + const result = await githubGraphQl<{ + data?: { + repository?: { + issue?: { timelineItems?: { nodes?: Array<{ createdAt?: string | null; label?: { name?: string | null } | null } | null> | null } | null } | null; + } | null; + }; + errors?: unknown[]; + }>(env, query, token, admissionKey).catch(() => undefined); + if (result === undefined) return null; + if (Array.isArray(result.errors) && result.errors.length > 0) return null; + const wanted = labelName.toLowerCase(); + for (const node of result.data?.repository?.issue?.timelineItems?.nodes ?? []) { + const label = node?.label?.name; + if (typeof label === "string" && label.toLowerCase() === wanted && typeof node?.createdAt === "string") return node.createdAt; + } + return null; +} + export type LinkedIssueClosureByPullRequestResult = "closed_by_pull_request" | "not_closed_by_pull_request" | "fetch_error"; /** Verifies whether GitHub attributes this issue's closure to the specific PR, via GraphQL's `ClosedEvent.closer` diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index a99d25e34..3b1c71cfb 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -4,6 +4,7 @@ import { AGENT_ACTION_CLASSES, AUTONOMY_LEVELS, FEASIBILITY_VERDICTS, PUBLIC_SUR // #9773: the request bodies these routes really accept, from the one place they are defined. import { checkBeforeStartSchema, slopRiskSchema, validateFocusManifestSchema, validateLinkedIssueSchema } from "@loopover/contract/api-requests"; import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; +import { MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES } from "../review/priority-eligibility-window"; import { MAX_CONTRIBUTOR_OPEN_ITEM_CAP } from "../types"; import { MAX_FIND_OPPORTUNITIES_TARGETS, @@ -759,6 +760,8 @@ export const RepositorySettingsSchema = z gateDryRun: z.boolean().optional(), premergeContentRecheck: z.boolean().optional(), requireFreshRebaseWindowMinutes: z.number().int().positive().nullable().optional(), + // #9738: non-negative, not positive -- 0 is the documented way to turn the window off. + priorityEligibilityWindowMinutes: z.number().int().min(0).max(MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES).nullable().optional(), staleBaseAheadByThreshold: z.number().int().positive().nullable().optional(), mergeReadinessGateMode: z.enum(["off", "advisory", "block"]), manifestPolicyGateMode: z.enum(["off", "advisory", "block"]), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 46cfd4453..1677dd32f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -106,6 +106,7 @@ import { backfillRepositorySegment, fetchAndStorePullRequestFilesForReview, fetchBaseAheadBy, + fetchIssueLabelFirstAppliedAt, fetchLinkedIssueFacts, fetchLiveBaseBranchAdvancedAt, invalidateCiStateCache, @@ -199,6 +200,7 @@ import { PRIORITY_LABEL_POLICY_URL, resolvePriorityLabelEnforcement, } from "../review/priority-label-eligibility"; +import { DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, resolvePriorityEligibilityHold } from "../review/priority-eligibility-window"; import { fetchLinkedIssueLabelsForPropagation } from "../review/linked-issue-label-propagation-fetch"; import { shouldPublishReviewCheck } from "../review/check-names"; import { fetchPublicContributorProfile } from "../github/public"; @@ -2803,6 +2805,7 @@ function buildAgentMaintenancePlanInput(args: { linkedIssueRulesConfig: Awaited>; migrationCollisionHold: AgentActionPlanInput["migrationCollisionHold"]; unlinkedIssueMatchHold: AgentActionPlanInput["unlinkedIssueMatchHold"]; + priorityEligibilityHold: AgentActionPlanInput["priorityEligibilityHold"]; aiReviewLowConfidenceHold: AgentActionPlanInput["aiReviewLowConfidenceHold"]; unlinkedIssueMatchClose: AgentActionPlanInput["unlinkedIssueMatchClose"]; liveMergeState: string | undefined; @@ -2842,6 +2845,7 @@ function buildAgentMaintenancePlanInput(args: { linkedIssueRulesConfig, migrationCollisionHold, unlinkedIssueMatchHold, + priorityEligibilityHold, aiReviewLowConfidenceHold, unlinkedIssueMatchClose, liveMergeState, @@ -2920,6 +2924,7 @@ function buildAgentMaintenancePlanInput(args: { }, ...(migrationCollisionHold !== undefined ? { migrationCollisionHold } : {}), ...(unlinkedIssueMatchHold !== undefined ? { unlinkedIssueMatchHold } : {}), + ...(priorityEligibilityHold !== undefined ? { priorityEligibilityHold } : {}), ...(aiReviewLowConfidenceHold !== undefined ? { aiReviewLowConfidenceHold } : {}), ...(unlinkedIssueMatchClose !== undefined ? { unlinkedIssueMatchClose } : {}), manualReviewLockContentionResolved, @@ -3460,6 +3465,24 @@ async function runAgentMaintenancePlanAndExecute( }) : undefined; const unlinkedIssueMatchHold = unlinkedIssueMatchDisposition?.kind === "hold" ? unlinkedIssueMatchDisposition : undefined; + + // Priority-issue eligibility window (#9738). A PR closing a `gittensor:priority` issue is gate-eligible + // only once the label has been publicly present for the configured window, so first-come pickup is fair to + // everyone watching the repo rather than to whoever was already looking. Resolved here (not in the planner) + // because it needs a GitHub read; the DECISION is the pure evaluator, unit-tested on its own. + // + // FAIL-OPEN throughout: a fetch that throws, a label that was never applied, or a timestamp we cannot parse + // all yield no hold. Holding a contributor's PR on a fact we could not read is a penalty for our own gap. + const priorityEligibilityHold = await resolvePriorityEligibilityHold({ + env, + repoFullName, + linkedIssues: pr.linkedIssues, + prCreatedAt: pr.createdAt ?? null, + windowMinutes: settings.priorityEligibilityWindowMinutes ?? DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, + priorityLabel: settings.typeLabels?.priority ?? DEFAULT_TYPE_LABELS.priority, + token: ciToken, + fetchLabeledAt: (repo, issueNumber, label) => fetchIssueLabelFirstAppliedAt(env, repo, issueNumber, label, ciToken), + }).catch(() => undefined); const unlinkedIssueMatchClose = unlinkedIssueMatchDisposition?.kind === "close" ? unlinkedIssueMatchDisposition : undefined; // Contributor blacklist (#1425): resolve whether the PR author is on the repo's blacklist (the shared/global @@ -3661,6 +3684,7 @@ async function runAgentMaintenancePlanAndExecute( linkedIssueRulesConfig, migrationCollisionHold, unlinkedIssueMatchHold, + priorityEligibilityHold, aiReviewLowConfidenceHold: aiReviewLowConfidenceHold ?? aiReviewSalvageableHold, unlinkedIssueMatchClose, liveMergeState, diff --git a/src/review/priority-eligibility-window.ts b/src/review/priority-eligibility-window.ts new file mode 100644 index 000000000..8b510cc8d --- /dev/null +++ b/src/review/priority-eligibility-window.ts @@ -0,0 +1,136 @@ +// Priority-issue eligibility window (#9738). +// +// `gittensor:priority` carries the highest payout, so assignment fairness matters most there. 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. +// +// The rule: a PR closing a priority-labeled issue is gate-eligible only once the label has been publicly +// present for the configured window (default 30 minutes, per repo). A PR that arrives inside the window is +// NOT rejected -- it is HELD, with a neutral comment stating the moment it becomes eligible, and proceeds +// normally once the window elapses. No penalty beyond waiting; the contributor keeps their work. +// +// Deliberately measured from the EARLIEST labeling event, not the most recent. The spec requires that +// re-applying the label "does not reset the clock for already-open PRs", and an earliest-event anchor gives +// that for free and for everyone: the moment a priority issue opens for work is a single knowable instant +// that nothing later can move. A label removed and re-added much later therefore reopens work immediately, +// which is the honest reading -- the issue was already public for that whole time. + +/** The window's default. Contributor pickup runs to minutes, so this only needs to exceed the visibility gap. */ +export const DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES = 30; + +/** Bounds, so a manifest cannot set a window that never opens or one that is not a window at all. */ +export const MIN_PRIORITY_ELIGIBILITY_WINDOW_MINUTES = 0; +export const MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES = 24 * 60; + +/** The rule's stable identifier, written to the ledger with every enforcement decision. */ +export const PRIORITY_ELIGIBILITY_RULE_ID = "priority-eligibility-window"; + +export type PriorityEligibilityInput = { + /** Minutes the label must have been present. `0` disables the rule. */ + windowMinutes: number; + /** When the priority label FIRST landed on the linked issue, ISO-8601. Null when unknown. */ + labeledAt: string | null; + /** When the PR was opened, ISO-8601. */ + prCreatedAt: string | null; +}; + +export type PriorityEligibilityResult = + | { eligible: true; reason: null; eligibleAt: null } + | { eligible: false; reason: string; eligibleAt: string }; + +const ELIGIBLE: PriorityEligibilityResult = { eligible: true, reason: null, eligibleAt: null }; + +function parsedTime(value: string | null): number | null { + if (!value) return null; + const parsed = Date.parse(value); + return Number.isFinite(parsed) ? parsed : null; +} + +/** + * PURE evaluator. Returns `eligible: false` only when every fact needed to justify a hold is known and the + * PR genuinely arrived inside the window. + * + * FAIL-OPEN on every uncertainty -- an unparseable or missing timestamp, or a non-positive window -- because + * holding a contributor's PR on a fact we could not read is a penalty for our own gap. The three inputs are + * passed in rather than fetched here so this is unit-testable without a network or a clock. + */ +export function evaluatePriorityEligibilityWindow(input: PriorityEligibilityInput): PriorityEligibilityResult { + const windowMinutes = Number.isFinite(input.windowMinutes) ? Math.max(0, Math.trunc(input.windowMinutes)) : 0; + if (windowMinutes <= 0) return ELIGIBLE; + + const labeledAt = parsedTime(input.labeledAt); + const prCreatedAt = parsedTime(input.prCreatedAt); + if (labeledAt === null || prCreatedAt === null) return ELIGIBLE; + + const eligibleAtMs = labeledAt + windowMinutes * 60_000; + if (prCreatedAt >= eligibleAtMs) return ELIGIBLE; + + const eligibleAt = new Date(eligibleAtMs).toISOString(); + return { + eligible: false, + eligibleAt, + reason: `This issue's \`gittensor:priority\` label was applied at ${new Date(labeledAt).toISOString()}, and priority issues open for work ${windowMinutes} minutes later so everyone watching the repo gets the same chance to see them. This PR is held until ${eligibleAt}, then continues normally — nothing else about it is affected and no action is needed from you.`, + }; +} + +/** + * The moment a priority issue opens for work, for surfaces that want to state it up front rather than + * explain a hold after the fact. Null when the window is off or the label time is unknown. + */ +export function priorityEligibleAt(labeledAt: string | null, windowMinutes: number): string | null { + const labeled = parsedTime(labeledAt); + if (labeled === null || !Number.isFinite(windowMinutes) || windowMinutes <= 0) return null; + return new Date(labeled + Math.trunc(windowMinutes) * 60_000).toISOString(); +} + +/** + * Resolve the hold for a PR, or undefined when it may proceed (#9738). + * + * The only impure part of the rule: it reads WHEN the priority label first landed on each linked issue. The + * decision itself is `evaluatePriorityEligibilityWindow` above, which is why that function takes timestamps + * rather than a repo. + * + * Walks every linked issue and holds on the FIRST one still inside its window, so a PR linking two priority + * issues waits for the later of them -- linking a second issue can never be a way to skip the first's window. + */ +export async function resolvePriorityEligibilityHold(input: { + env: unknown; + repoFullName: string; + linkedIssues: readonly number[] | null | undefined; + prCreatedAt: string | null; + windowMinutes: number; + priorityLabel: string | undefined; + token: string | undefined; + /** Injected so this is testable without a network; defaults to the real GraphQL read. */ + fetchLabeledAt?: (repoFullName: string, issueNumber: number, label: string) => Promise; + /** The linked issues' labels, when the caller already has them -- saves a read for the common case where + * no linked issue carries the priority label at all. */ + issueLabels?: ReadonlyMap; +}): Promise<{ reason: string; comment: string } | undefined> { + const windowMinutes = input.windowMinutes; + if (!Number.isFinite(windowMinutes) || windowMinutes <= 0) return undefined; + const issues = input.linkedIssues ?? []; + if (issues.length === 0 || !input.prCreatedAt || !input.token) return undefined; + + const priorityLabel = input.priorityLabel; + if (!priorityLabel) return undefined; + const wanted = priorityLabel.toLowerCase(); + const fetchLabeledAt = input.fetchLabeledAt; + if (!fetchLabeledAt) return undefined; + + for (const issueNumber of issues) { + const known = input.issueLabels?.get(issueNumber); + // When the caller told us this issue's labels and priority is not among them, no read is needed. + if (known && !known.some((label) => label.toLowerCase() === wanted)) continue; + const labeledAt = await fetchLabeledAt(input.repoFullName, issueNumber, priorityLabel).catch(() => null); + if (labeledAt === null) continue; + const verdict = evaluatePriorityEligibilityWindow({ windowMinutes, labeledAt, prCreatedAt: input.prCreatedAt }); + if (verdict.eligible) continue; + return { + reason: `${PRIORITY_ELIGIBILITY_RULE_ID}: issue #${issueNumber} opens for work at ${verdict.eligibleAt}`, + comment: verdict.reason, + }; + } + return undefined; +} diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 3742ab895..b4643cad9 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -444,6 +444,11 @@ export type AgentActionPlanInput = { // missing linked issue is never a close reason" -- it only ever downgrades a would-merge into a held-for- // review state so a human can confirm the match before it's credited. unlinkedIssueMatchHold?: { reason: string; comment: string } | undefined; + // #9738: the linked `gittensor:priority` issue's eligibility window has not elapsed. Priority carries the + // highest payout, so first-come pickup is only fair if everyone can see the issue before anyone can act on + // it. Same risk profile as the two holds above -- SUPPRESSES the merge (folded into `heldForManualReview`), + // never closes, and clears itself once the window passes with no action from the contributor. + priorityEligibilityHold?: { reason: string; comment: string } | undefined; // Same guardrail as unlinkedIssueMatchHold, but for a CONFIRMED REPEAT by the same contributor (tracked via // audit_events, see resolveUnlinkedIssueMatchDisposition) -- a second occurrence is no longer a coincidence // worth a human's benefit of the doubt, so this closes the PR one-shot instead of holding it. Deliberately @@ -1116,6 +1121,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne guardrailHit, migrationCollisionHold: input.migrationCollisionHold !== undefined, unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined, + priorityEligibilityHold: input.priorityEligibilityHold !== undefined, advisoryCheckHold: input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0, // Deliberately conjoined with "nothing else adverse": an unstable state is only attributed to the ignore // list when our own aggregate found NO failing check of any kind. If some other non-required check is @@ -1269,6 +1275,21 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne }); } + // 1d-priority) priority-eligibility hold (#9738) — mirrors 1d exactly. The PR is EARLY, not wrong: it is + // held with a neutral comment naming the moment work opens, and nothing else about it changes. The label + // is the same generic manual-review one, so the hold is visible on the surfaces a maintainer already reads. + if (reviewGood && input.priorityEligibilityHold !== undefined && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { + actions.push({ + actionClass: "label", + autonomyClass: "merge", + requiresApproval: approval("merge"), + reason: `verdict=${conclusion}; ${input.priorityEligibilityHold.reason}`, + label: labels.manualReview, + labelOp: "add", + comment: sanitizePublicComment(input.priorityEligibilityHold.comment), + }); + } + // 1e) unlinked-issue-match REPEAT manual-review fallback when `close` autonomy can't act (gate-review // finding): mirrors 1d, but for the escalated-repeat case folded into `heldForManualReview` above only // when close isn't acting — without this, a confirmed repeat would silently MERGE with no visible signal diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index 10e227e71..96eb754f6 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -51,16 +51,38 @@ export function assessMergeableState(state: string | null | undefined): Mergeabl } } +/** + * Every input that SUPPRESSES a would-merge into a manual hold, as DATA (#9738). + * + * The input type, the `heldForManualReview` formula, and every test fixture are derived from this table, so + * adding a hold is one entry rather than three edits that can each be forgotten independently -- which is + * exactly how a hold gets declared and then silently not folded into the decision. The value documents WHY + * the input holds; nothing reads it as text today, and it is the natural place for a ledger reason to come + * from when one is wanted. + * + * A hold NEVER closes a PR. Each of these downgrades a merge into a held-for-review state and nothing more. + */ +export const MERGE_HOLD_INPUTS = { + guardrailHit: "the PR touches a hard-guardrail path", + migrationCollisionHold: "two migrations claim the same number", + unlinkedIssueMatchHold: "an unlinked issue appears to match this work", + advisoryCheckHold: "an advisory check the maintainer configured is not passing", + priorityEligibilityHold: "the linked priority issue's eligibility window has not elapsed", + unlinkedIssueMatchCloseWithoutCloseActing: "a repeat unlinked-issue match while close autonomy is off", +} as const; + +export type MergeHoldInput = keyof typeof MERGE_HOLD_INPUTS; + +/** The table's keys, resolved once. Exported so a caller (or a test fixture) can enumerate every hold + * without restating them -- the restatement is the thing this table exists to remove. */ +export const MERGE_HOLD_INPUT_KEYS = Object.keys(MERGE_HOLD_INPUTS) as MergeHoldInput[]; + /** The hold inputs every surface must agree on. Each field mirrors the planner input of the same name — * the caller (planner or processors.ts) resolves them once and both surfaces read the same values. */ -export type PrDispositionInput = { +export type PrDispositionInput = Record & { mergeableState: string | null | undefined; /** Gate conclusion success/neutral AND required CI passed — the only thing that earns approve/merge. */ reviewGood: boolean; - guardrailHit: boolean; - migrationCollisionHold: boolean; - unlinkedIssueMatchHold: boolean; - advisoryCheckHold: boolean; /** #9810 follow-up: GitHub says `unstable`, but the ONLY non-passing check explaining it is one the * maintainer listed in `gate.ignoredCheckRuns`. LoopOver's own CI aggregate already excludes such a run -- * yet `mergeable_state` is GitHub's computation, not ours, and it stays "unstable" while the check exists @@ -68,9 +90,6 @@ export type PrDispositionInput = { * held anyway (observed on JSONbored/loopover#9816, reason "mergeable_state is unstable — non-required * check(s) not passing: Contributor trust"). Set ONLY when nothing else adverse was seen. */ unstableExplainedByIgnoredChecks?: boolean | undefined; - /** A confirmed repeat unlinked-issue-match while `close` autonomy is NOT acting (the planner's own - * fold-into-hold escape hatch — see agent-actions.ts's heldForManualReview doc). */ - unlinkedIssueMatchCloseWithoutCloseActing: boolean; }; export type PrDisposition = { @@ -99,13 +118,9 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { // An "unstable" state that ONLY an ignored check explains carries no signal a maintainer asked to act on: // they explicitly declared that check meaningless for this repo. Every other unstable cause still holds. const unstableHolds = mergeable === "unstable" && input.unstableExplainedByIgnoredChecks !== true; - const heldForManualReview = - input.guardrailHit || - input.migrationCollisionHold || - input.unlinkedIssueMatchHold || - input.advisoryCheckHold || - unstableHolds || - input.unlinkedIssueMatchCloseWithoutCloseActing; + // Derived from MERGE_HOLD_INPUTS, so a hold declared in that table is folded in by construction and a + // new one can never be added-but-not-honoured. + const heldForManualReview = MERGE_HOLD_INPUT_KEYS.some((key) => input[key] === true) || unstableHolds; const heldForUnstableMergeState = unstableHolds; const wouldApprove = input.reviewGood && !heldForManualReview && mergeable !== "conflict"; const wouldMerge = input.reviewGood && !heldForManualReview && mergeable === "clean"; diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 6bdd700fb..fd084525f 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -537,6 +537,7 @@ function applyGateConfigOverrides(effective: RepositorySettings, gate: FocusMani if (gate.dryRun !== null) effective.gateDryRun = gate.dryRun; if (gate.premergeContentRecheck !== null) effective.premergeContentRecheck = gate.premergeContentRecheck; if (gate.requireFreshRebaseWindowMinutes !== null) effective.requireFreshRebaseWindowMinutes = gate.requireFreshRebaseWindowMinutes; + if (gate.priorityEligibilityWindowMinutes !== null) effective.priorityEligibilityWindowMinutes = gate.priorityEligibilityWindowMinutes; if (gate.staleBaseAheadByThreshold !== null) effective.staleBaseAheadByThreshold = gate.staleBaseAheadByThreshold; if (gate.claMode !== null) effective.claGateMode = gate.claMode; if (gate.claConsentPhrase !== null) effective.claConsentPhrase = gate.claConsentPhrase; diff --git a/src/types.ts b/src/types.ts index 2a980bbfd..92a27b483 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1377,6 +1377,11 @@ export type RepositorySettings = { * force -- a `mergeable_state: clean` read is trusted exactly as it is today. Layered like every other * settings field (`.loopover.yml` `gate.requireFreshRebaseWindow` > DB > `null`). */ requireFreshRebaseWindowMinutes?: number | null | undefined; + /** `gate.priorityEligibilityWindow` (#9738): minutes a `gittensor:priority` label must have been publicly + * present before a PR closing that issue is gate-eligible. A PR inside the window is HELD with a neutral + * comment naming the moment it opens, never rejected, and proceeds once the window elapses. `0` disables + * the rule; absent means the shipped 30-minute default. Config-as-code only. */ + priorityEligibilityWindowMinutes?: number | null | undefined; /** Stale-base auto-rebase threshold (#review-grounding stale-base fact): a commit count. When the repo's * current default branch is at least this many commits ahead of a PR's own base commit, the pre-review * readiness gate (`prReadyForReview`) forces an `update_branch`, independent of GitHub's own diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 849eeaabe..fc4c83cbb 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -352,6 +352,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { copycatMode: "copycat:", copycatMinScore: "copycat:", closeAuditHoldoutPct: "closeAuditHoldoutPct:", + priorityEligibilityWindowMinutes: "priorityEligibilityWindow:", } satisfies Record, string>; it.each(Object.entries(GATE_FIELD_TOKENS))("documents gate.%s", (_field, token) => { @@ -1011,7 +1012,7 @@ describe("compileFocusManifestPolicy", () => { issueDiscoveryPolicy: "neutral", maintainerNotes: [], publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], - gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, + gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null, amsReputationBridge: null }, @@ -1205,7 +1206,7 @@ describe("parseFocusManifest gate config", () => { // the block→advisory deprecation-downgrade behavior itself is covered separately below. const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); + expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); }); it("parses gate.mergeReadiness, round-trips it, and warns on a bad value (#822)", () => { diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index a8d63b5d4..70efdbcc0 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { assessMergeableState, derivePrDisposition, isCommentMergeStateHeld, type PrDispositionInput } from "../../src/settings/pr-disposition"; +import { MERGE_HOLD_INPUT_KEYS, assessMergeableState, derivePrDisposition, isCommentMergeStateHeld, type MergeHoldInput, type PrDispositionInput } from "../../src/settings/pr-disposition"; import { AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, planAgentMaintenanceActions, type AgentActionPlanInput } from "../../src/settings/agent-actions"; import { deriveUnifiedStatus, type UnifiedReviewInput } from "../../src/review/unified-comment"; import type { GateCheckConclusion } from "../../src/rules/advisory"; @@ -10,17 +10,18 @@ import type { GateCheckConclusion } from "../../src/rules/advisory"; // renderer agreeing with it through the bridge-passed boolean. A regression that re-introduces a // private raw-string interpretation in any surface shows up here as a cross-surface disagreement. +/** Every declared merge-hold input, off. */ +const NO_HOLDS = Object.fromEntries(MERGE_HOLD_INPUT_KEYS.map((key) => [key, false])) as Record; + const RAW_STATES = ["clean", "dirty", "behind", "unstable", "blocked", "unknown", "", null, undefined] as const; function dispositionInput(over: Partial = {}): PrDispositionInput { return { mergeableState: "clean", reviewGood: true, - guardrailHit: false, - migrationCollisionHold: false, - unlinkedIssueMatchHold: false, - advisoryCheckHold: false, - unlinkedIssueMatchCloseWithoutCloseActing: false, + // Every hold OFF, derived from the table rather than restated -- a new hold joins this fixture by + // existing, which is what stops a hold from being added and silently never exercised here (#9738). + ...NO_HOLDS, ...over, }; } @@ -179,10 +180,8 @@ describe("cross-surface: deriveUnifiedStatus consumes the bridge-resolved boolea }); describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => { - const base = { - reviewGood: true, guardrailHit: false, migrationCollisionHold: false, unlinkedIssueMatchHold: false, - advisoryCheckHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false, - }; + // Same derivation as `dispositionInput` above: the holds come from the table, never restated here. + const base = { reviewGood: true, ...NO_HOLDS }; it("REGRESSION: an unstable state the ignore list fully explains no longer holds", () => { // The live half-fix: gate.ignoredCheckRuns removed the check from LoopOver's CI aggregate, but diff --git a/test/unit/priority-eligibility-window.test.ts b/test/unit/priority-eligibility-window.test.ts new file mode 100644 index 000000000..8e3cb8ecd --- /dev/null +++ b/test/unit/priority-eligibility-window.test.ts @@ -0,0 +1,182 @@ +// The priority-issue eligibility window (#9738). +// +// Every branch of the evaluator, both sides of every fallback: the rule holds a PR only when it can prove +// the PR arrived inside the window, and opens for everything else. +import { describe, expect, it } from "vitest"; +import { + DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, + MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, + MIN_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, + PRIORITY_ELIGIBILITY_RULE_ID, + evaluatePriorityEligibilityWindow, + priorityEligibleAt, + resolvePriorityEligibilityHold, +} from "../../src/review/priority-eligibility-window"; + +const LABELED_AT = "2026-07-29T12:00:00.000Z"; +/** The default window's boundary, to the millisecond. */ +const ELIGIBLE_AT = "2026-07-29T12:30:00.000Z"; + +describe("priority eligibility window (#9738)", () => { + it("holds a PR opened inside the window, naming the moment it becomes eligible", () => { + const result = evaluatePriorityEligibilityWindow({ + windowMinutes: DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, + labeledAt: LABELED_AT, + prCreatedAt: "2026-07-29T12:00:30.000Z", + }); + expect(result.eligible).toBe(false); + expect(result.eligibleAt).toBe(ELIGIBLE_AT); + // The comment a contributor reads must say it is a wait, not a rejection. + expect(result.reason).toContain(ELIGIBLE_AT); + expect(result.reason).toContain("continues normally"); + expect(result.reason).not.toMatch(/reject|closed|violation/i); + }); + + it("opens exactly AT the boundary, not one millisecond later", () => { + // An off-by-one here is a PR held for a window that has provably elapsed. + expect(evaluatePriorityEligibilityWindow({ windowMinutes: 30, labeledAt: LABELED_AT, prCreatedAt: ELIGIBLE_AT }).eligible).toBe(true); + expect( + evaluatePriorityEligibilityWindow({ windowMinutes: 30, labeledAt: LABELED_AT, prCreatedAt: "2026-07-29T12:29:59.999Z" }).eligible, + ).toBe(false); + }); + + it("leaves a PR opened after the window untouched", () => { + const result = evaluatePriorityEligibilityWindow({ windowMinutes: 30, labeledAt: LABELED_AT, prCreatedAt: "2026-07-29T13:00:00.000Z" }); + expect(result).toEqual({ eligible: true, reason: null, eligibleAt: null }); + }); + + it("is off when the window is zero or negative", () => { + for (const windowMinutes of [0, -1, -30]) { + expect(evaluatePriorityEligibilityWindow({ windowMinutes, labeledAt: LABELED_AT, prCreatedAt: LABELED_AT }).eligible).toBe(true); + } + }); + + it("FAILS OPEN when a timestamp is missing or unreadable", () => { + // Holding someone's PR because WE could not read a timestamp is a penalty for our own gap. + const cases = [ + { labeledAt: null, prCreatedAt: LABELED_AT }, + { labeledAt: LABELED_AT, prCreatedAt: null }, + { labeledAt: "not-a-date", prCreatedAt: LABELED_AT }, + { labeledAt: LABELED_AT, prCreatedAt: "not-a-date" }, + { labeledAt: "", prCreatedAt: "" }, + ]; + for (const { labeledAt, prCreatedAt } of cases) { + expect(evaluatePriorityEligibilityWindow({ windowMinutes: 30, labeledAt, prCreatedAt }).eligible, JSON.stringify({ labeledAt, prCreatedAt })).toBe(true); + } + }); + + it("ignores a non-finite window rather than treating it as infinite", () => { + for (const windowMinutes of [Number.NaN, Number.POSITIVE_INFINITY]) { + expect(evaluatePriorityEligibilityWindow({ windowMinutes, labeledAt: LABELED_AT, prCreatedAt: LABELED_AT }).eligible).toBe(true); + } + }); + + it("truncates a fractional window rather than rounding it up", () => { + // 30.9 minutes is a 30-minute window; a contributor must never wait longer than the number they were told. + const result = evaluatePriorityEligibilityWindow({ windowMinutes: 30.9, labeledAt: LABELED_AT, prCreatedAt: ELIGIBLE_AT }); + expect(result.eligible).toBe(true); + }); + + describe("priorityEligibleAt", () => { + it("states the moment work opens", () => { + expect(priorityEligibleAt(LABELED_AT, 30)).toBe(ELIGIBLE_AT); + }); + + it("is null when the window is off or the label time is unknown", () => { + expect(priorityEligibleAt(LABELED_AT, 0)).toBeNull(); + expect(priorityEligibleAt(LABELED_AT, Number.NaN)).toBeNull(); + expect(priorityEligibleAt(null, 30)).toBeNull(); + expect(priorityEligibleAt("not-a-date", 30)).toBeNull(); + }); + }); + + it("exposes a stable rule id and sane bounds", () => { + // The id is written to the ledger with every enforcement decision, so it is part of the contract. + expect(PRIORITY_ELIGIBILITY_RULE_ID).toBe("priority-eligibility-window"); + expect(MIN_PRIORITY_ELIGIBILITY_WINDOW_MINUTES).toBe(0); + expect(MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES).toBe(1440); + expect(DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES).toBeGreaterThan(MIN_PRIORITY_ELIGIBILITY_WINDOW_MINUTES); + expect(DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES).toBeLessThan(MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES); + }); +}); + +// The impure half: which linked issue is consulted, and every way the resolution declines to hold. +describe("resolvePriorityEligibilityHold (#9738)", () => { + const base = { + env: {}, + repoFullName: "owner/repo", + prCreatedAt: "2026-07-29T12:00:30.000Z", + windowMinutes: 30, + priorityLabel: "gittensor:priority", + token: "t", + }; + + it("holds on a linked priority issue still inside its window", async () => { + const hold = await resolvePriorityEligibilityHold({ ...base, linkedIssues: [7], fetchLabeledAt: async () => LABELED_AT }); + expect(hold?.reason).toContain(PRIORITY_ELIGIBILITY_RULE_ID); + expect(hold?.reason).toContain("#7"); + expect(hold?.reason).toContain(ELIGIBLE_AT); + expect(hold?.comment).toContain(ELIGIBLE_AT); + }); + + it("waits for the LATER of two priority issues, so linking a second never skips the first's window", async () => { + const labels = new Map([ + [1, "2026-07-29T10:00:00.000Z"], // long elapsed + [2, LABELED_AT], // still inside + ]); + const hold = await resolvePriorityEligibilityHold({ + ...base, + linkedIssues: [1, 2], + fetchLabeledAt: async (_repo, issueNumber) => labels.get(issueNumber) ?? null, + }); + expect(hold?.reason).toContain("#2"); + }); + + it("does not hold once every linked issue's window has elapsed", async () => { + const hold = await resolvePriorityEligibilityHold({ ...base, linkedIssues: [1], fetchLabeledAt: async () => "2026-07-29T10:00:00.000Z" }); + expect(hold).toBeUndefined(); + }); + + it("skips the read entirely when the caller says the issue has no priority label", async () => { + let reads = 0; + const hold = await resolvePriorityEligibilityHold({ + ...base, + linkedIssues: [5], + issueLabels: new Map([[5, ["gittensor:bug"]]]), + fetchLabeledAt: async () => { + reads += 1; + return LABELED_AT; + }, + }); + expect(hold).toBeUndefined(); + expect(reads, "a non-priority issue must cost no GitHub read").toBe(0); + }); + + it("still reads when the caller's labels DO include priority", async () => { + const hold = await resolvePriorityEligibilityHold({ + ...base, + linkedIssues: [5], + issueLabels: new Map([[5, ["Gittensor:Priority"]]]), + fetchLabeledAt: async () => LABELED_AT, + }); + expect(hold, "label matching is case-insensitive").toBeDefined(); + }); + + it("FAILS OPEN on everything it cannot establish", async () => { + const never = async () => LABELED_AT; + const cases: Array<[string, Parameters[0]]> = [ + ["window off", { ...base, windowMinutes: 0, linkedIssues: [1], fetchLabeledAt: never }], + ["no linked issues", { ...base, linkedIssues: [], fetchLabeledAt: never }], + ["null linked issues", { ...base, linkedIssues: null, fetchLabeledAt: never }], + ["no PR timestamp", { ...base, prCreatedAt: null, linkedIssues: [1], fetchLabeledAt: never }], + ["no token", { ...base, token: undefined, linkedIssues: [1], fetchLabeledAt: never }], + ["no label configured", { ...base, priorityLabel: undefined, linkedIssues: [1], fetchLabeledAt: never }], + ["no fetcher", { ...base, linkedIssues: [1] }], + ["label never applied", { ...base, linkedIssues: [1], fetchLabeledAt: async () => null }], + ["fetch throws", { ...base, linkedIssues: [1], fetchLabeledAt: async () => { throw new Error("GitHub 502"); } }], + ]; + for (const [name, input] of cases) { + expect(await resolvePriorityEligibilityHold(input), name).toBeUndefined(); + } + }); +}); From 1c125174fb7d550b910f2abbc61b3560e7c81a19 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:43:00 -0700 Subject: [PATCH 2/4] test(eligibility): cover the priority-window wiring, and fix the config 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. --- .../loopover-engine/src/focus-manifest.ts | 1 + .../src/settings/pr-type-label.ts | 16 +++ src/queue/processors.ts | 13 +- src/review/priority-eligibility-window.ts | 13 +- test/unit/agent-actions.test.ts | 71 ++++++++++ test/unit/backfill.test.ts | 134 ++++++++++++++++++ test/unit/focus-manifest.test.ts | 38 +++++ test/unit/pr-type-label.test.ts | 30 +++- test/unit/priority-eligibility-window.test.ts | 24 +++- 9 files changed, 325 insertions(+), 15 deletions(-) diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index b731b38dc..4ac9b43f3 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -2133,6 +2133,7 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { if (gate.dryRun !== null) out.dryRun = gate.dryRun; if (gate.premergeContentRecheck !== null) out.premergeContentRecheck = gate.premergeContentRecheck; if (gate.requireFreshRebaseWindowMinutes !== null) out.requireFreshRebaseWindow = gate.requireFreshRebaseWindowMinutes; + if (gate.priorityEligibilityWindowMinutes !== null) out.priorityEligibilityWindow = gate.priorityEligibilityWindowMinutes; if (gate.staleBaseAheadByThreshold !== null) out.staleBaseAheadByThreshold = gate.staleBaseAheadByThreshold; if (gate.claMode !== null) out.claMode = gate.claMode; if (gate.claConsentPhrase !== null || gate.claCheckRunName !== null || gate.claCheckRunAppSlug !== null) { diff --git a/packages/loopover-engine/src/settings/pr-type-label.ts b/packages/loopover-engine/src/settings/pr-type-label.ts index 0813b92b5..ab32d3773 100644 --- a/packages/loopover-engine/src/settings/pr-type-label.ts +++ b/packages/loopover-engine/src/settings/pr-type-label.ts @@ -142,6 +142,22 @@ export type PrTypeLabelDecision = { * misconfigured additive mapping's `prLabel` happens to collide with a type-label-set name (it is * excluded from removal since it is also being applied). Pure + total. */ +/** + * The PRIORITY label as this repo names it, falling back to the built-in default. + * + * One resolution rather than the ad-hoc `settings.typeLabels?.priority ?? DEFAULT_TYPE_LABELS.priority` + * each caller used to spell for itself: the label-author rule (#9737) and the eligibility window (#9738) + * both act on this exact label, and a repo that renamed it would be enforced inconsistently if the two + * ever disagreed about which label they mean. + */ +export function resolvePriorityTypeLabel(labels: PrTypeLabelSet | null | undefined): string { + const configured = labels?.priority; + if (typeof configured === "string" && configured.trim().length > 0) return configured; + /* v8 ignore next -- noUncheckedIndexedAccess: PrTypeLabelSet is a Record, so this reads + as possibly-undefined to the type system though the built-in set always defines `priority`. */ + return DEFAULT_TYPE_LABELS.priority ?? "gittensor:priority"; +} + export function resolvePrTypeLabel(input: { title: string | undefined; linkedIssueLabels?: string[] | undefined; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1677dd32f..c57658fd8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -106,7 +106,6 @@ import { backfillRepositorySegment, fetchAndStorePullRequestFilesForReview, fetchBaseAheadBy, - fetchIssueLabelFirstAppliedAt, fetchLinkedIssueFacts, fetchLiveBaseBranchAdvancedAt, invalidateCiStateCache, @@ -193,7 +192,7 @@ import { reviewedPullRequestHeadSha, type PullRequestFreshness, } from "../github/pr-freshness"; -import { DEFAULT_TYPE_LABELS, resolvePrTypeLabel } from "../settings/pr-type-label"; +import { DEFAULT_TYPE_LABELS, resolvePriorityTypeLabel, resolvePrTypeLabel } from "../settings/pr-type-label"; import { PRIORITY_LABEL_AUTHOR_RULE_ID, PRIORITY_LABEL_ENFORCEMENT_EVENT, @@ -3479,9 +3478,11 @@ async function runAgentMaintenancePlanAndExecute( linkedIssues: pr.linkedIssues, prCreatedAt: pr.createdAt ?? null, windowMinutes: settings.priorityEligibilityWindowMinutes ?? DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, - priorityLabel: settings.typeLabels?.priority ?? DEFAULT_TYPE_LABELS.priority, + priorityLabel: resolvePriorityTypeLabel(settings.typeLabels), token: ciToken, - fetchLabeledAt: (repo, issueNumber, label) => fetchIssueLabelFirstAppliedAt(env, repo, issueNumber, label, ciToken), + /* v8 ignore next -- defensive: resolvePriorityEligibilityHold catches its own GitHub read, so it has no + reject path today. The guard stays so a future edit inside it degrades to "no hold" rather than + failing the whole maintenance pass -- which is the fail-open posture the rest of this rule has. */ }).catch(() => undefined); const unlinkedIssueMatchClose = unlinkedIssueMatchDisposition?.kind === "close" ? unlinkedIssueMatchDisposition : undefined; @@ -6894,9 +6895,7 @@ async function maybeHandlePriorityLabelEligibility( if (issue.pull_request !== undefined && issue.pull_request !== null) return false; const settings = await resolveRepositorySettings(env, repoFullName).catch(() => undefined); - /* v8 ignore next 2 -- noUncheckedIndexedAccess fallback: PrTypeLabelSet is a Record, so - DEFAULT_TYPE_LABELS.priority reads as possibly-undefined to the type system though it is always set. */ - const priorityLabel: string = settings?.typeLabels?.priority ?? DEFAULT_TYPE_LABELS.priority ?? "gittensor:priority"; + const priorityLabel = resolvePriorityTypeLabel(settings?.typeLabels); const labels = (issue.labels ?? []).map((label) => label?.name ?? "").filter((name) => name.length > 0); // Only the labelled event for THIS label matters; anything else is another label's business. if (!labels.some((name) => name.toLowerCase() === priorityLabel.toLowerCase())) return false; diff --git a/src/review/priority-eligibility-window.ts b/src/review/priority-eligibility-window.ts index 8b510cc8d..7dbdce62e 100644 --- a/src/review/priority-eligibility-window.ts +++ b/src/review/priority-eligibility-window.ts @@ -1,3 +1,5 @@ +import { fetchIssueLabelFirstAppliedAt } from "../github/backfill"; + // Priority-issue eligibility window (#9738). // // `gittensor:priority` carries the highest payout, so assignment fairness matters most there. First-come @@ -95,14 +97,15 @@ export function priorityEligibleAt(labeledAt: string | null, windowMinutes: numb * issues waits for the later of them -- linking a second issue can never be a way to skip the first's window. */ export async function resolvePriorityEligibilityHold(input: { - env: unknown; + env: Env; repoFullName: string; linkedIssues: readonly number[] | null | undefined; prCreatedAt: string | null; windowMinutes: number; priorityLabel: string | undefined; token: string | undefined; - /** Injected so this is testable without a network; defaults to the real GraphQL read. */ + /** Overridable so this is testable without a network. Defaults to the real GraphQL read, so the + * production caller passes facts only and never has to re-wire the reader. */ fetchLabeledAt?: (repoFullName: string, issueNumber: number, label: string) => Promise; /** The linked issues' labels, when the caller already has them -- saves a read for the common case where * no linked issue carries the priority label at all. */ @@ -116,8 +119,10 @@ export async function resolvePriorityEligibilityHold(input: { const priorityLabel = input.priorityLabel; if (!priorityLabel) return undefined; const wanted = priorityLabel.toLowerCase(); - const fetchLabeledAt = input.fetchLabeledAt; - if (!fetchLabeledAt) return undefined; + // The real reader is the default: the caller supplies facts (env, repo, token) and never re-wires it. + const fetchLabeledAt = + input.fetchLabeledAt ?? + ((repoFullName, issueNumber, label) => fetchIssueLabelFirstAppliedAt(input.env, repoFullName, issueNumber, label, input.token)); for (const issueNumber of issues) { const known = input.issueLabels?.get(issueNumber); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index aa5c8a3f9..690bc45bc 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -2646,3 +2646,74 @@ describe("screenshot-table gate short-circuit (#2006)", () => { expect(plan[0]).toMatchObject({ closeKind: "blacklist" }); }); }); + +describe("priority-eligibility hold (#9738)", () => { + const held = { + priorityEligibilityHold: { + reason: "priority-eligibility-window: issue #7 opens for work at 2026-07-29T12:30:00.000Z", + comment: "This PR is held until 2026-07-29T12:30:00.000Z, then continues normally — no action is needed from you.", + }, + }; + + it("does NOT auto-merge a clean+approved+passing PR while the window is still open", () => { + const plan = classes( + planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { merge: "auto" }, ...held, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }), + ), + ); + expect(plan).not.toContain("merge"); + }); + + it("labels the PR manual-review with the window reason, and never closes it", () => { + // The PR is EARLY, not wrong. A close here would take a contributor's work for arriving too fast. + const plan = planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { review_state_label: "auto", merge: "auto", close: "auto" }, ...held, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }), + ); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); + expect(label?.reason).toContain("priority-eligibility-window"); + expect(classes(plan)).not.toContain("close"); + }); + + it("attaches the wait-not-rejection comment to the label action", () => { + const plan = planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { merge: "auto", review_state_label: "auto" }, ...held, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }), + ); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.comment).toContain("continues normally"); + }); + + it("does nothing at all when merge autonomy is not acting — there is no merge to hold", () => { + // The rule exists solely to keep an early PR from auto-merging. With merge off, nothing is being + // prevented, so labelling the PR would be noise a maintainer has to clear by hand. + const plan = planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { review_state_label: "auto" }, ...held, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }), + ); + expect(plan.some((a) => a.actionClass === "label" && a.comment === held.priorityEligibilityHold.comment)).toBe(false); + }); + + it("falls back to manual-review (+ the comment) when review_state_label is OFF", () => { + const plan = planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { merge: "auto" }, ...held, pr: { labels: [], mergeableState: "clean" } }), + ); + expect(plan).toEqual([ + expect.objectContaining({ actionClass: "label", autonomyClass: "merge", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "add", comment: held.priorityEligibilityHold.comment }), + ]); + }); + + it("does not duplicate the manual-review label when it is already present", () => { + const plan = planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { merge: "auto" }, ...held, pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" } }), + ); + expect(plan.filter((a) => a.actionClass === "label")).toHaveLength(0); + }); + + it("absent (undefined) is byte-identical to today — a clean approved PR still merges", () => { + const plan = classes( + planAgentMaintenanceActions( + input({ conclusion: "success", autonomy: { merge: "auto" }, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }), + ), + ); + expect(plan).toContain("merge"); + }); +}); diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 4fc195415..8b2fff89b 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -44,6 +44,7 @@ import { refreshContributorActivity, refreshInstallationHealth, refreshPullRequestDetails, + fetchIssueLabelFirstAppliedAt, } from "../../src/github/backfill"; import { clearGitHubResponseCacheForTest, @@ -4998,3 +4999,136 @@ describe("fetchLinkedIssueClosedByPullRequest (#5385)", () => { expect(await fetchLinkedIssueClosedByPullRequest(env, "owner/repo", 100, 200, "test-token")).toBe("not_closed_by_pull_request"); }); }); + +// The priority-eligibility window (#9738) anchors on the EARLIEST labeling, so this read is what decides +// whether a contributor's PR is held. Every way it can decline to answer must FAIL OPEN with null -- a hold +// on a fact we could not establish is a penalty for our own gap. +describe("fetchIssueLabelFirstAppliedAt (#9738)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + function stubTimeline(nodes: unknown[], captured?: { query?: string }) { + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + expect(String(input)).toBe("https://api.github.com/graphql"); + if (captured && typeof init?.body === "string") captured.query = String(JSON.parse(init.body).query); + return Response.json({ data: { repository: { issue: { timelineItems: { nodes } } } } }); + }); + } + + it("returns the FIRST labeling of the wanted label, ignoring later ones", async () => { + const env = createTestEnv(); + stubTimeline([ + { __typename: "LabeledEvent", createdAt: "2026-07-29T10:00:00Z", label: { name: "gittensor:bug" } }, + { __typename: "LabeledEvent", createdAt: "2026-07-29T11:00:00Z", label: { name: "gittensor:priority" } }, + // A re-application must NOT reset the clock -- that is the whole point of anchoring on the earliest. + { __typename: "LabeledEvent", createdAt: "2026-07-29T15:00:00Z", label: { name: "gittensor:priority" } }, + ]); + + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBe( + "2026-07-29T11:00:00Z", + ); + }); + + it("matches the label case-insensitively", async () => { + const env = createTestEnv(); + stubTimeline([{ __typename: "LabeledEvent", createdAt: "2026-07-29T11:00:00Z", label: { name: "Gittensor:Priority" } }]); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBe( + "2026-07-29T11:00:00Z", + ); + }); + + it("asks only for LABELED_EVENT items, one page, oldest first", async () => { + // `first:` rather than `last:` is load-bearing: `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. + const env = createTestEnv(); + const captured: { query?: string } = {}; + stubTimeline([], captured); + await fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok"); + expect(captured.query).toContain("itemTypes: [LABELED_EVENT]"); + expect(captured.query).toContain("first: 100"); + expect(captured.query).not.toContain("last:"); + expect(captured.query).toContain("issue(number: 7)"); + }); + + it("escapes the owner and name rather than interpolating them raw", async () => { + const env = createTestEnv(); + const captured: { query?: string } = {}; + stubTimeline([], captured); + await fetchIssueLabelFirstAppliedAt(env, 'ow"ner/re"po', 7, "gittensor:priority", "tok"); + expect(captured.query).toContain('owner: "ow\\"ner"'); + expect(captured.query).toContain('name: "re\\"po"'); + }); + + it("FAILS OPEN on every input it cannot use, without making a request", async () => { + const env = createTestEnv(); + let calls = 0; + vi.stubGlobal("fetch", async () => { + calls += 1; + return Response.json({ data: {} }); + }); + + const cases: Array<[string, Promise]> = [ + ["no token", fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", undefined)], + ["empty token", fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "")], + ["no label", fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "", "tok")], + ["no owner", fetchIssueLabelFirstAppliedAt(env, "/repo", 7, "gittensor:priority", "tok")], + ["no name", fetchIssueLabelFirstAppliedAt(env, "owner/", 7, "gittensor:priority", "tok")], + ["not a repo path", fetchIssueLabelFirstAppliedAt(env, "nope", 7, "gittensor:priority", "tok")], + ]; + for (const [name, promise] of cases) await expect(promise, name).resolves.toBeNull(); + expect(calls, "an unusable input costs no GitHub read").toBe(0); + }); + + it("FAILS OPEN when the label was never applied, or the timeline is empty or absent", async () => { + const env = createTestEnv(); + for (const nodes of [[], [{ __typename: "LabeledEvent", createdAt: "2026-07-29T11:00:00Z", label: { name: "other" } }]]) { + stubTimeline(nodes); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBeNull(); + } + // The whole `data` shape missing, and each nullable link in the chain. + for (const body of [{ data: {} }, { data: { repository: null } }, { data: { repository: { issue: null } } }, { data: { repository: { issue: { timelineItems: null } } } }, { data: { repository: { issue: { timelineItems: { nodes: null } } } } }]) { + vi.stubGlobal("fetch", async () => Response.json(body)); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok"), JSON.stringify(body)).resolves.toBeNull(); + } + }); + + it("FAILS OPEN on a malformed node rather than throwing on it", async () => { + const env = createTestEnv(); + stubTimeline([ + null, + { __typename: "LabeledEvent", createdAt: "2026-07-29T10:00:00Z", label: null }, + { __typename: "LabeledEvent", createdAt: "2026-07-29T10:30:00Z", label: { name: null } }, + // Right label, but no usable timestamp -- skipped, not returned as null-the-value. + { __typename: "LabeledEvent", createdAt: null, label: { name: "gittensor:priority" } }, + { __typename: "LabeledEvent", createdAt: "2026-07-29T12:00:00Z", label: { name: "gittensor:priority" } }, + ]); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBe( + "2026-07-29T12:00:00Z", + ); + }); + + it("FAILS OPEN when GraphQL answers with errors, or the request throws outright", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async () => Response.json({ errors: [{ message: "RATE_LIMITED" }] })); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBeNull(); + + vi.stubGlobal("fetch", async () => { + throw new Error("socket hang up"); + }); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBeNull(); + }); + + it("treats an EMPTY errors array as no errors", async () => { + const env = createTestEnv(); + vi.stubGlobal("fetch", async () => + Response.json({ + errors: [], + data: { repository: { issue: { timelineItems: { nodes: [{ __typename: "LabeledEvent", createdAt: "2026-07-29T11:00:00Z", label: { name: "gittensor:priority" } }] } } } }, + }), + ); + await expect(fetchIssueLabelFirstAppliedAt(env, "owner/repo", 7, "gittensor:priority", "tok")).resolves.toBe( + "2026-07-29T11:00:00Z", + ); + }); +}); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index fc4c83cbb..98abeea6c 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -6477,3 +6477,41 @@ describe("an explicitly-undefined manifest setting does not punch a hole in the expect(resolveEffectiveSettings({ typeLabelsEnabled: false } as RepositorySettings, manifestWithSettings({ typeLabelsEnabled: true })).typeLabelsEnabled).toBe(true); }); }); + +describe("gate.priorityEligibilityWindow priority-issue eligibility config (#9738)", () => { + it("parses gate.priorityEligibilityWindow, sets present, round-trips, and resolves into effective settings", () => { + const m = parseFocusManifest({ gate: { priorityEligibilityWindow: 45 } }); + expect(m.gate.priorityEligibilityWindowMinutes).toBe(45); + expect(m.gate.present).toBe(true); + expect(gateConfigToJson(m.gate)).toMatchObject({ priorityEligibilityWindow: 45 }); + const eff = resolveEffectiveSettings({} as unknown as RepositorySettings, m); + expect(eff.priorityEligibilityWindowMinutes).toBe(45); + }); + + it("defaults to unset/undefined when omitted — byte-identical to today", () => { + const m = parseFocusManifest({}); + expect(m.gate.priorityEligibilityWindowMinutes).toBeNull(); + const eff = resolveEffectiveSettings({} as unknown as RepositorySettings, m); + expect(eff.priorityEligibilityWindowMinutes).toBeUndefined(); + }); + + it("accepts 0 as an explicit OFF rather than treating it as absent", () => { + // 0 disables the rule; if it were dropped as falsy, a repo that deliberately turned the window off + // would silently inherit the 30-minute default instead. + const m = parseFocusManifest({ gate: { priorityEligibilityWindow: 0 } }); + expect(m.gate.priorityEligibilityWindowMinutes).toBe(0); + expect(resolveEffectiveSettings({} as unknown as RepositorySettings, m).priorityEligibilityWindowMinutes).toBe(0); + }); + + it("warns and drops a fractional, negative, or out-of-range value rather than silently coercing it", () => { + for (const value of [2.5, -1, 1441]) { + const parsed = parseFocusManifest({ gate: { priorityEligibilityWindow: value } }); + expect(parsed.gate.priorityEligibilityWindowMinutes, String(value)).toBeNull(); + expect(parsed.warnings.some((w) => /gate\.priorityEligibilityWindow/i.test(w)), String(value)).toBe(true); + } + }); + + it("accepts the maximum exactly, so the bound is inclusive", () => { + expect(parseFocusManifest({ gate: { priorityEligibilityWindow: 1440 } }).gate.priorityEligibilityWindowMinutes).toBe(1440); + }); +}); diff --git a/test/unit/pr-type-label.test.ts b/test/unit/pr-type-label.test.ts index f06f29751..0d6c6ebcd 100644 --- a/test/unit/pr-type-label.test.ts +++ b/test/unit/pr-type-label.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { DEFAULT_TYPE_LABELS, MAX_TYPE_LABEL_CATEGORIES, MAX_TYPE_LABEL_NAME_LENGTH, deriveKindFromTitle, normalizeTypeLabelSet, resolvePrTypeLabel } from "../../src/settings/pr-type-label"; +import { DEFAULT_TYPE_LABELS, MAX_TYPE_LABEL_CATEGORIES, MAX_TYPE_LABEL_NAME_LENGTH, deriveKindFromTitle, normalizeTypeLabelSet, resolvePrTypeLabel, resolvePriorityTypeLabel } from "../../src/settings/pr-type-label"; import type { LinkedIssueLabelPropagationConfig } from "../../src/types"; describe("deriveKindFromTitle", () => { @@ -402,3 +402,31 @@ describe("normalizeTypeLabelSet (#priority-linked-issue-gate)", () => { }); }); }); + +// One resolution of the priority label, shared by the label-author rule (#9737) and the eligibility +// window (#9738) -- the two rules act on the same label, so they must never disagree about its name. +describe("resolvePriorityTypeLabel (#9738)", () => { + it("uses the repo's configured name when it has one", () => { + expect(resolvePriorityTypeLabel({ ...DEFAULT_TYPE_LABELS, priority: "team:top" })).toBe("team:top"); + }); + + it("falls back to the built-in default when the repo configured none", () => { + expect(resolvePriorityTypeLabel(undefined)).toBe(DEFAULT_TYPE_LABELS.priority); + expect(resolvePriorityTypeLabel(null)).toBe(DEFAULT_TYPE_LABELS.priority); + expect(resolvePriorityTypeLabel({} as never)).toBe(DEFAULT_TYPE_LABELS.priority); + }); + + it("treats a blank or whitespace-only configured name as unconfigured", () => { + // An empty label name would match nothing and silently disable both rules, which is worse than + // falling back to the default the repo would otherwise have had. + for (const priority of ["", " "]) { + expect(resolvePriorityTypeLabel({ ...DEFAULT_TYPE_LABELS, priority }), JSON.stringify(priority)).toBe(DEFAULT_TYPE_LABELS.priority); + } + }); + + it("always returns a usable, non-empty label", () => { + for (const input of [undefined, null, {} as never, { ...DEFAULT_TYPE_LABELS, priority: "x" }]) { + expect(resolvePriorityTypeLabel(input).length).toBeGreaterThan(0); + } + }); +}); diff --git a/test/unit/priority-eligibility-window.test.ts b/test/unit/priority-eligibility-window.test.ts index 8e3cb8ecd..8ee0d5752 100644 --- a/test/unit/priority-eligibility-window.test.ts +++ b/test/unit/priority-eligibility-window.test.ts @@ -2,7 +2,7 @@ // // Every branch of the evaluator, both sides of every fallback: the rule holds a PR only when it can prove // the PR arrived inside the window, and opens for everything else. -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, @@ -12,6 +12,7 @@ import { priorityEligibleAt, resolvePriorityEligibilityHold, } from "../../src/review/priority-eligibility-window"; +import { createTestEnv } from "../helpers/d1"; const LABELED_AT = "2026-07-29T12:00:00.000Z"; /** The default window's boundary, to the millisecond. */ @@ -103,7 +104,7 @@ describe("priority eligibility window (#9738)", () => { // The impure half: which linked issue is consulted, and every way the resolution declines to hold. describe("resolvePriorityEligibilityHold (#9738)", () => { const base = { - env: {}, + env: {} as never, repoFullName: "owner/repo", prCreatedAt: "2026-07-29T12:00:30.000Z", windowMinutes: 30, @@ -162,6 +163,24 @@ describe("resolvePriorityEligibilityHold (#9738)", () => { expect(hold, "label matching is case-insensitive").toBeDefined(); }); + it("defaults to the REAL GraphQL reader when the caller injects none", async () => { + // The production call site passes facts only. If the default were missing (or silently a no-op), the + // rule would never hold anything in production while every injected-fetcher test kept passing. + let asked: string | undefined; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + asked = typeof init?.body === "string" ? String(JSON.parse(init.body).query) : undefined; + return Response.json({ + data: { repository: { issue: { timelineItems: { nodes: [{ createdAt: LABELED_AT, label: { name: "gittensor:priority" } }] } } } }, + }); + }); + + const hold = await resolvePriorityEligibilityHold({ ...base, env: createTestEnv(), linkedIssues: [7] }); + + expect(asked, "it really went to GitHub's GraphQL API").toContain("LABELED_EVENT"); + expect(hold?.reason).toContain("#7"); + vi.unstubAllGlobals(); + }); + it("FAILS OPEN on everything it cannot establish", async () => { const never = async () => LABELED_AT; const cases: Array<[string, Parameters[0]]> = [ @@ -171,7 +190,6 @@ describe("resolvePriorityEligibilityHold (#9738)", () => { ["no PR timestamp", { ...base, prCreatedAt: null, linkedIssues: [1], fetchLabeledAt: never }], ["no token", { ...base, token: undefined, linkedIssues: [1], fetchLabeledAt: never }], ["no label configured", { ...base, priorityLabel: undefined, linkedIssues: [1], fetchLabeledAt: never }], - ["no fetcher", { ...base, linkedIssues: [1] }], ["label never applied", { ...base, linkedIssues: [1], fetchLabeledAt: async () => null }], ["fetch throws", { ...base, linkedIssues: [1], fetchLabeledAt: async () => { throw new Error("GitHub 502"); } }], ]; From 0a8aacdfc876e0d5d0a8f7078f64b82412bd44d6 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:33:19 -0700 Subject: [PATCH 3/4] test(eligibility): close the last coverage gaps without suppressions 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. --- .../src/settings/pr-type-label.ts | 12 +++-- src/queue/processors.ts | 8 ++-- test/unit/queue-3.test.ts | 47 +++++++++++++++++++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/packages/loopover-engine/src/settings/pr-type-label.ts b/packages/loopover-engine/src/settings/pr-type-label.ts index ab32d3773..bb82f4234 100644 --- a/packages/loopover-engine/src/settings/pr-type-label.ts +++ b/packages/loopover-engine/src/settings/pr-type-label.ts @@ -22,10 +22,16 @@ export type { PrTypeLabelSet } from "../types/manifest-deps-types.js"; * assumption (#label-modularity): a self-hoster's `typeLabels` fully replaces the category set these * keys are drawn from. The built-in categories are mutually exclusive by default (see * `resolvePrTypeLabel`'s `removeLabels`) unless a propagation mapping is explicitly additive. */ +/** The built-in PRIORITY label, named on its own so it reads as `string`. `PrTypeLabelSet` is an open + * `Record`, which makes `DEFAULT_TYPE_LABELS.priority` optional to the type system even though the + * built-in set always defines it -- naming the value is what lets `resolvePriorityTypeLabel` return it + * without a fallback branch nothing can ever take. */ +export const DEFAULT_PRIORITY_LABEL = "gittensor:priority"; + export const DEFAULT_TYPE_LABELS: PrTypeLabelSet = { bug: "gittensor:bug", feature: "gittensor:feature", - priority: "gittensor:priority", + priority: DEFAULT_PRIORITY_LABEL, }; export const MAX_TYPE_LABEL_CATEGORIES = 32; @@ -153,9 +159,7 @@ export type PrTypeLabelDecision = { export function resolvePriorityTypeLabel(labels: PrTypeLabelSet | null | undefined): string { const configured = labels?.priority; if (typeof configured === "string" && configured.trim().length > 0) return configured; - /* v8 ignore next -- noUncheckedIndexedAccess: PrTypeLabelSet is a Record, so this reads - as possibly-undefined to the type system though the built-in set always defines `priority`. */ - return DEFAULT_TYPE_LABELS.priority ?? "gittensor:priority"; + return DEFAULT_PRIORITY_LABEL; } export function resolvePrTypeLabel(input: { diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c57658fd8..6124dc741 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3480,10 +3480,10 @@ async function runAgentMaintenancePlanAndExecute( windowMinutes: settings.priorityEligibilityWindowMinutes ?? DEFAULT_PRIORITY_ELIGIBILITY_WINDOW_MINUTES, priorityLabel: resolvePriorityTypeLabel(settings.typeLabels), token: ciToken, - /* v8 ignore next -- defensive: resolvePriorityEligibilityHold catches its own GitHub read, so it has no - reject path today. The guard stays so a future edit inside it degrades to "no hold" rather than - failing the whole maintenance pass -- which is the fail-open posture the rest of this rule has. */ - }).catch(() => undefined); + // No `.catch` here on purpose: resolvePriorityEligibilityHold catches its own GitHub read and returns + // undefined, so it has no reject path. A guard for one that cannot happen is unreachable code that + // reads as tested-and-fine while never running. + }); const unlinkedIssueMatchClose = unlinkedIssueMatchDisposition?.kind === "close" ? unlinkedIssueMatchDisposition : undefined; // Contributor blacklist (#1425): resolve whether the PR author is on the repo's blacklist (the shared/global diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 57d4097a5..c801e1cfe 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2502,6 +2502,53 @@ describe("queue processors", () => { await upsertPullRequestFromGitHub(env, "owner/repo", { number: prNumber, title: "Migration PR", state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main" }, labels: [], body: "" }); } + it("holds a would-otherwise-merge PR while its linked priority issue is still inside the eligibility window (#9738)", async () => { + // The end-to-end shape of the rule: a clean, approved, green PR that would MERGE is held instead, + // because the priority issue it closes only became public minutes ago. Held, never closed -- the PR + // is early, not wrong. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 61); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 61, + title: "Fix the thing", + state: "open", + user: { login: "contributor" }, + head: { sha: "sha1" }, + base: { ref: "main" }, + labels: [], + body: "Closes #7", + created_at: new Date(Date.now() - 60_000).toISOString(), + } as never); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(61, { filename: "src/a.ts", status: "modified" }, [], seen); + // The one extra fact the rule needs: the priority label landed on issue #7 a minute ago, so the + // 30-minute window has not elapsed. + const inner = globalThis.fetch as typeof fetch; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + if (url === "https://api.github.com/graphql" && String(init?.body ?? "").includes("LABELED_EVENT")) { + return Response.json({ + data: { + repository: { + issue: { + timelineItems: { + nodes: [{ createdAt: new Date(Date.now() - 60_000).toISOString(), label: { name: "gittensor:priority" } }], + }, + }, + }, + }, + }); + } + return inner(input as never, init as never); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "priority-eligibility-hold", repoFullName: "owner/repo", prNumber: 61, installationId: 123 }); + + expect(seen.merged, "an unelapsed window must stop the auto-merge").toBe(false); + expect(seen.closed, "a hold is never a close -- the contributor keeps their work").toBe(false); + expect(seen.comments.some((c) => c.includes("continues normally")), "the comment reads as a wait, not a rejection").toBe(true); + }); + it("holds a would-otherwise-merge PR when the live base has a colliding migration number, with the distinct label + rebase comment", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedMigrationRecheckRepo(env, 60, { premergeContentRecheck: true }); From e4871931ad8221d0d8746cef38e25c42006ba7a1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:53:51 -0700 Subject: [PATCH 4/4] test(engine): behaviour-test the priority label and window in the ENGINE'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. --- packages/loopover-engine/src/index.ts | 4 ++ .../test/priority-type-label.test.ts | 37 +++++++++++++++++++ src/settings/pr-disposition.ts | 2 +- 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 packages/loopover-engine/test/priority-type-label.test.ts diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index fc84cd7c7..1824c8466 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -952,3 +952,7 @@ export { type PlanProgress, } from "./plan-dag.js"; export { isDone as isPlanStepDone, nextReadySteps } from "./plan-step-readiness.js"; + +// #9743: the priority label's single resolution, exported so the engine's OWN behaviour suite can test +// it -- the `engine` Codecov flag credits engine source from that suite, not from the root vitest run. +export { DEFAULT_PRIORITY_LABEL, resolvePriorityTypeLabel } from "./settings/pr-type-label.js"; diff --git a/packages/loopover-engine/test/priority-type-label.test.ts b/packages/loopover-engine/test/priority-type-label.test.ts new file mode 100644 index 000000000..e0f8c5dad --- /dev/null +++ b/packages/loopover-engine/test/priority-type-label.test.ts @@ -0,0 +1,37 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { DEFAULT_PRIORITY_LABEL, gateConfigToJson, parseFocusManifest, resolvePriorityTypeLabel } from "../dist/index.js"; + +// The engine's own behaviour suite for the two surfaces #9738/#9743 added here. The root vitest suite +// covers them too, but the `engine` Codecov flag is fed by THIS suite -- and, more to the point, these are +// engine semantics, so they belong beside the rest of the engine's behaviour tests. + +test("resolvePriorityTypeLabel: a repo's configured name wins (#9743)", () => { + assert.equal(resolvePriorityTypeLabel({ bug: "b", feature: "f", priority: "team:top" }), "team:top"); +}); + +test("resolvePriorityTypeLabel: falls back to the built-in default when unconfigured", () => { + assert.equal(resolvePriorityTypeLabel(undefined), DEFAULT_PRIORITY_LABEL); + assert.equal(resolvePriorityTypeLabel(null), DEFAULT_PRIORITY_LABEL); + assert.equal(resolvePriorityTypeLabel({}), DEFAULT_PRIORITY_LABEL); +}); + +test("resolvePriorityTypeLabel: a blank configured name is unconfigured, not an empty label", () => { + // An empty label would match nothing and silently disable both rules that key on it. + for (const priority of ["", " "]) { + assert.equal(resolvePriorityTypeLabel({ bug: "b", feature: "f", priority }), DEFAULT_PRIORITY_LABEL); + } +}); + +test("gateConfigToJson round-trips priorityEligibilityWindow (#9738)", () => { + // The setting used to parse but never serialize, so it was silently dropped on the way back out. + const parsed = parseFocusManifest({ gate: { priorityEligibilityWindow: 45 } }); + assert.equal(parsed.gate.priorityEligibilityWindowMinutes, 45); + assert.equal((gateConfigToJson(parsed.gate) as { priorityEligibilityWindow?: number }).priorityEligibilityWindow, 45); +}); + +test("gateConfigToJson: 0 is an explicit OFF and must survive the round trip", () => { + const parsed = parseFocusManifest({ gate: { priorityEligibilityWindow: 0 } }); + assert.equal(parsed.gate.priorityEligibilityWindowMinutes, 0); + assert.equal((gateConfigToJson(parsed.gate) as { priorityEligibilityWindow?: number }).priorityEligibilityWindow, 0); +}); diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index 96eb754f6..c8e99da2c 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -62,7 +62,7 @@ export function assessMergeableState(state: string | null | undefined): Mergeabl * * A hold NEVER closes a PR. Each of these downgrades a merge into a held-for-review state and nothing more. */ -export const MERGE_HOLD_INPUTS = { +const MERGE_HOLD_INPUTS = { guardrailHit: "the PR touches a hard-guardrail path", migrationCollisionHold: "two migrations claim the same number", unlinkedIssueMatchHold: "an unlinked issue appears to match this work",