diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 4715da2fbb..fd6f9568a0 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -475,9 +475,17 @@ function resolveLinkedIssueClaimedAt( ): string | null { if (linkedIssues.length === 0) return null; if (!existing) return observedLinkedIssueClaimedAt; + // Duplicate-winner priority bug (#linked-issue-claim-overlap-preserve): this used to reset the claim + // whenever the linked-issue SET differed AT ALL from the prior sync, including a pure ADDITION (e.g. "Fixes + // #1" -> "Fixes #1, Fixes #2"). Because linkedIssueClaimedAt is a single PR-level timestamp (not stored + // per-issue), that reset threw away issue #1's original, legitimately-earliest claim time just because the + // author later also referenced an unrelated #2 -- letting a LATER PR that also claims #1 leapfrog into + // duplicate-cluster winner via isDuplicateClusterWinnerByClaim. The correct rule: only start a FRESH clock + // when the new set shares NO issue with the old one (a genuine swap to unrelated work); any overlap means at + // least one issue's claim is a continuation, not a new claim, so the earliest timestamp must survive. if ( existing.linkedIssuesJson === linkedIssuesJson || - sameLinkedIssueSet(parseLinkedIssuesJson(existing.linkedIssuesJson), linkedIssues) + linkedIssueSetsOverlap(parseLinkedIssuesJson(existing.linkedIssuesJson), linkedIssues) ) return existing.linkedIssueClaimedAt ?? observedLinkedIssueClaimedAt; return observedLinkedIssueClaimedAt; @@ -488,12 +496,13 @@ function parseLinkedIssuesJson(value: string): number[] { return Array.isArray(parsed) ? (parsed as number[]) : []; } -function sameLinkedIssueSet(left: number[], right: number[]): boolean { - return normalizedLinkedIssueSet(left) === normalizedLinkedIssueSet(right); -} - -function normalizedLinkedIssueSet(numbers: number[]): string { - return jsonString([...new Set(numbers)].sort((left, right) => left - right)); +// Whether `left` and `right` share at least one linked-issue number -- used to decide whether a linked-issue +// SET change is a continuation of an existing claim (overlap) or a genuine swap to unrelated issues (no +// overlap), see resolveLinkedIssueClaimedAt above. +function linkedIssueSetsOverlap(left: number[], right: number[]): boolean { + if (left.length === 0 || right.length === 0) return false; + const leftSet = new Set(left); + return right.some((value) => leftSet.has(value)); } export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issue: GitHubIssuePayload, options: { seenOpenAt?: string } = {}): Promise { @@ -3908,15 +3917,56 @@ export async function bumpPullRequestMergeAttempt(env: Env, fullName: string, nu // Review-evasion: repeated ready<->draft cycling (#gaming-tactic-draft-cycle). +// Idempotency-marker eventType for bumpPullRequestDraftConversionCount below (#draft-conversion-retry-double- +// count). Deliberately NOT one of the MODERATION_VIOLATION_EVENT_TYPE values -- it must never feed +// countModerationViolationsForActor's ban-threshold tally, it exists purely to make ONE counter bump +// idempotent per webhook delivery. +const DRAFT_CONVERSION_BUMP_EVENT_TYPE = "review_evasion.draft_conversion_bump"; + /** Increment the ready<->draft conversion counter for a PR and return the new total. Deliberately NOT scoped * to headSha (unlike bumpPullRequestMergeAttempt) -- a contributor pushing a new commit between draft cycles - * is still doing the same repeated-evasion shape, so a fresh head must not reset the count back to zero. */ -export async function bumpPullRequestDraftConversionCount(env: Env, fullName: string, number: number): Promise { - const db = getDb(env.DB); - await db - .update(pullRequests) - .set({ draftConversionCount: sql`${pullRequests.draftConversionCount} + 1`, updatedAt: nowIso() }) - .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number))); + * is still doing the same repeated-evasion shape, so a fresh head must not reset the count back to zero. + * + * Delivery-idempotent (#draft-conversion-retry-double-count): processGitHubWebhook's own webhook-processing + * pass explicitly re-throws on a rate-limited/retryable error partway through (e.g. a live GitHub CI/mergeable + * fetch later in the same pass), which the queue consumer turns into a `message.retry()` redelivery of the + * SAME message body (same deliveryId). Without a guard, that redelivery re-runs this bump for the SAME + * physical draft conversion, poisoning the count toward a false "2nd offense" and wrongly auto-closing (plus + * moderation-striking) a contributor who converted to draft exactly once. Mirrors + * recordModerationViolation/hasModerationViolationForTarget's own idempotent-per-(actor, eventType, targetKey) + * `audit_events` check-then-act pattern in this same file, with `deliveryId` standing in for `actor` (it, not + * a GitHub login, is the thing that must be unique per real invocation) and deliberately no time window -- + * unlike hasAuditEventForDelivery's short redelivery-window check, a queue retry can legitimately land long + * after the original attempt (backoff), so the marker must be PERMANENT, matching recordModerationViolation's + * own "a later replay must not re-count just because time has passed" reasoning. + * + * Ordering note: the counter increment happens BEFORE the marker write (not after) so that if the marker + * write itself fails, the counter has still genuinely advanced (worst case: a rare future retry could + * double-bump once more, no worse than before this fix) rather than the marker silently blocking a real + * future bump forever while the counter itself never advanced. + */ +export async function bumpPullRequestDraftConversionCount(env: Env, fullName: string, number: number, deliveryId: string): Promise { + const db = getDb(env.DB); + const targetKey = `${fullName}#${number}`; + const alreadyBumpedForThisDelivery = await hasModerationViolationForTarget(env, deliveryId, DRAFT_CONVERSION_BUMP_EVENT_TYPE, targetKey); + if (!alreadyBumpedForThisDelivery) { + await db + .update(pullRequests) + .set({ draftConversionCount: sql`${pullRequests.draftConversionCount} + 1`, updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number))); + await recordAuditEvent(env, { + eventType: DRAFT_CONVERSION_BUMP_EVENT_TYPE, + actor: deliveryId, + targetKey, + outcome: "completed", + detail: "ready<->draft conversion counter bumped", + metadata: { repoFullName: fullName, pullNumber: number }, + }).catch( + /* v8 ignore next -- best-effort: an audit write failure only means a LATER retry of this exact delivery + * could double-bump once more; the counter increment above already succeeded and is not rolled back. */ + () => undefined, + ); + } const [row] = await db .select({ count: pullRequests.draftConversionCount }) .from(pullRequests) @@ -8077,7 +8127,15 @@ export function extractLinkedIssueNumbers(text: string, repoFullName: string, li return extractLinkedIssueNumbersWithOverflow(text, repoFullName, limit).numbers; } +// Requires the SAME GitHub closing-keyword adjacency extractLinkedIssueNumbersWithOverflow's regex enforces +// (#issue-body-pr-mention-pollution) -- without it, ANY bare "PR #N"/"pull request #N" mention in an issue's +// body (e.g. "similar to what we saw in PR #501, unrelated feature") was counted as a real link, even though +// no closing verb tied it to this issue. That falsely populated IssueRecord.linkedPrs, which +// buildContributorOpportunities uses to exclude the issue from the available-issues pool and which +// buildIssueQualityReport uses to force the issue's status to "do_not_use" -- silently hiding a fully +// available, unclaimed issue from contributor recommendations purely because its body happened to mention any +// other real PR number in the repo. function extractLinkedPrNumbers(text: string): number[] { - const matches = [...text.matchAll(/\b(?:PR|pull request)\s+#(\d+)\b/gi)]; + const matches = [...text.matchAll(/\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+(?:PR|pull request)\s+#(\d+)\b/gi)]; return [...new Set(matches.map((match) => Number(match[1])).filter((value) => Number.isInteger(value) && value > 0))]; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 63d516fff9..5a38e561a8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -592,6 +592,7 @@ import { resolveHardGuardrailGlobs } from "../review/guardrail-config"; import { guardrailPathMatches, isGuardrailHit } from "../signals/change-guardrail"; import { createIssueComment } from "../github/pr-actions"; import { + anyLinkedIssueHardRuleOn, loadLinkedIssueHardRules, mergeLinkedIssueHardRuleWithPersistedViolation, resolveLinkedIssueHardRule, @@ -2696,10 +2697,14 @@ async function runAgentMaintenancePlanAndExecute( if (liveLinkedIssueHardRule?.violated === true) { await markPullRequestLinkedIssueHardRuleViolated(env, repoFullName, pr.number, liveLinkedIssueHardRule.reason ?? "the linked issue is not eligible for a community PR").catch(() => undefined); } - const linkedIssueHardRule = mergeLinkedIssueHardRuleWithPersistedViolation(liveLinkedIssueHardRule, { - violatedAt: pr.linkedIssueHardRuleViolatedAt, - reason: pr.linkedIssueHardRuleViolationReason, - }); + const linkedIssueHardRule = mergeLinkedIssueHardRuleWithPersistedViolation( + liveLinkedIssueHardRule, + { + violatedAt: pr.linkedIssueHardRuleViolatedAt, + reason: pr.linkedIssueHardRuleViolationReason, + }, + anyLinkedIssueHardRuleOn(linkedIssueRulesConfig), + ); // Unlinked-issue guardrail (#unlinked-issue-guardrail, credibility-gate-farming defense): when this PR // links NO issue and the repo opted in (settings.unlinkedIssueGuardrail.mode === "hold"), check whether the @@ -5848,7 +5853,7 @@ async function handlePullRequestWebhookEvent( const draftAuthor = (pr.authorLogin ?? "").toLowerCase(); const isAuthorDraftConversion = draftConverter.length > 0 && draftConverter === draftAuthor; const draftConversionCount = isAuthorDraftConversion - ? await bumpPullRequestDraftConversionCount(env, repoFullName, pr.number).catch( + ? await bumpPullRequestDraftConversionCount(env, repoFullName, pr.number, deliveryId).catch( /* v8 ignore next -- fail-safe: a counter-write failure only means this ONE cycle isn't detected. */ () => 0, ) diff --git a/src/review/linked-issue-hard-rules.ts b/src/review/linked-issue-hard-rules.ts index 7a959ec364..7e4f294368 100644 --- a/src/review/linked-issue-hard-rules.ts +++ b/src/review/linked-issue-hard-rules.ts @@ -49,6 +49,20 @@ export type LinkedIssueHardRuleResult = { const NO_VIOLATION: LinkedIssueHardRuleResult = { violated: false, reason: null }; +/** Whether the repo's config currently has ANY linked-issue hard rule set to `"block"`. Shared by + * {@link evaluateLinkedIssueHardRules}, {@link resolveLinkedIssueHardRule} (both use it to skip evaluation + * entirely when nothing is enforced), and the call site's `anyRuleOn` argument to + * {@link mergeLinkedIssueHardRuleWithPersistedViolation} (#linked-issue-hard-rule-persistence) -- one shared + * definition so those three checks can never drift out of sync with each other. */ +export function anyLinkedIssueHardRuleOn(config: LinkedIssueHardRulesConfig): boolean { + return ( + config.ownerAssignedClose === "block" || + config.assignedIssueClose === "block" || + config.missingPointLabelClose === "block" || + config.maintainerOnlyLabelClose === "block" + ); +} + function findMatchingLabel(labels: string[], candidates: string[]): string | null { const wanted = new Set(candidates.map((c) => c.toLowerCase())); return labels.find((label) => wanted.has(label.toLowerCase())) ?? null; @@ -77,12 +91,7 @@ export function evaluateLinkedIssueHardRules(input: { }): LinkedIssueHardRuleResult { const { config, repoOwner } = input; const ownerLower = repoOwner.toLowerCase(); - const anyRuleOn = - config.ownerAssignedClose === "block" || - config.assignedIssueClose === "block" || - config.missingPointLabelClose === "block" || - config.maintainerOnlyLabelClose === "block"; - if (!anyRuleOn) return NO_VIOLATION; + if (!anyLinkedIssueHardRuleOn(config)) return NO_VIOLATION; for (const issue of input.issues) { if (issue.state !== "open") continue; @@ -143,17 +152,29 @@ export function evaluateLinkedIssueHardRules(input: { * pending-closure label as if the violation never happened. * * `violatedAt` is the PR's persisted first-violation marker (`pullRequests.linkedIssueHardRuleViolatedAt`) — - * present (non-null) once ANY pass has ever confirmed a violation for this PR, and NEVER cleared. When present, - * the merged result is forced to `violated: true` regardless of what the live pass found THIS time, falling - * back to the persisted `reason` only when the live pass didn't also (re-)confirm one this pass. A live - * violation always wins for the `reason` text (freshest, most specific), so a persisted memory never masks new - * information — it only ever ADDS enforcement the live-only path would have missed. + * present (non-null) once ANY pass has ever confirmed a violation for this PR, and NEVER cleared. When present + * AND at least one rule is still `"block"` (`anyRuleOn`), the merged result is forced to `violated: true` + * regardless of what the live pass found THIS time, falling back to the persisted `reason` only when the live + * pass didn't also (re-)confirm one this pass. A live violation always wins for the `reason` text (freshest, + * most specific), so a persisted memory never masks new information — it only ever ADDS enforcement the + * live-only path would have missed. + * + * `anyRuleOn` (#linked-issue-hard-rule-persistence-disable-rescue) exists because `live === undefined` is + * AMBIGUOUS on its own: resolveLinkedIssueHardRule returns `undefined` both when a rule is still active but + * THIS pass's body has zero linked issues (the dodge-1 case above, which the persisted marker must still + * catch) AND when NO rule is configured "block" at all anymore (the operator disabled every rule). Those two + * cases must NOT be treated the same: a maintainer who deliberately turns every rule off must be able to + * rescue a PR a NOW-DISABLED rule flagged in the past, or that PR stays condemned to a one-shot close forever + * even though the config that flagged it no longer exists. `anyRuleOn` disambiguates them — pass + * `anyLinkedIssueHardRuleOn(config)` from the same config the live evaluation was just run against. */ export function mergeLinkedIssueHardRuleWithPersistedViolation( live: LinkedIssueHardRuleResult | undefined, persisted: { violatedAt: string | null | undefined; reason: string | null | undefined }, + anyRuleOn: boolean, ): LinkedIssueHardRuleResult | undefined { if (live?.violated === true) return live; + if (!anyRuleOn) return live; if (persisted.violatedAt == null) return live; return { violated: true, reason: persisted.reason ?? "the linked issue is not eligible for a community PR" }; } @@ -180,12 +201,7 @@ export async function resolveLinkedIssueHardRule(args: { // (not "unknown") and the key can never be passed out of sync with the token it belongs to. installationId?: number | null | undefined; }): Promise { - const anyRuleOn = - args.config.ownerAssignedClose === "block" || - args.config.assignedIssueClose === "block" || - args.config.missingPointLabelClose === "block" || - args.config.maintainerOnlyLabelClose === "block"; - if (!anyRuleOn) return undefined; + if (!anyLinkedIssueHardRuleOn(args.config)) return undefined; if (extractLinkedIssueNumbersWithOverflow(args.body ?? "", args.repoFullName).overflow) { return { violated: true, diff --git a/test/unit/data-spine.test.ts b/test/unit/data-spine.test.ts index 7a66f8c0fe..5f7d813ee7 100644 --- a/test/unit/data-spine.test.ts +++ b/test/unit/data-spine.test.ts @@ -454,7 +454,13 @@ describe("data spine repositories", () => { state: "open", user: { login: "JSONbored" }, labels: [{}, { name: "bug" }], - body: "Related PR #1 and pull request #2.", + // #issue-body-pr-mention-pollution regression: a bare "Related PR #1" mention (no closing keyword) used + // to be enough to count as a real link -- that was the audited bug (any "PR #N"/"pull request #N" text + // mention anywhere in an issue body silently populated linkedPrs, hiding available issues from + // contributor recommendations). extractLinkedPrNumbers now requires the same closing-keyword adjacency + // extractLinkedIssueNumbersWithOverflow already enforces, so the body must actually say "closes"/ + // "fixes"/"resolves" immediately before the PR reference for it to count. + body: "Closes PR #1 and fixes pull request #2.", }); await upsertIssueFromGitHub(env, "owner/repo", { number: 11, @@ -479,6 +485,26 @@ describe("data spine repositories", () => { expect(await listContributorIssues(env, "JSONBORED")).toEqual(expect.arrayContaining([expect.objectContaining({ repoFullName: "owner/repo", number: 10 }), expect.objectContaining({ repoFullName: "owner/repo", number: 11 })])); }); + // REGRESSION (#issue-body-pr-mention-pollution): a bare "PR #N"/"pull request #N" text mention with no + // GitHub closing keyword nearby must NOT count as a real linked PR -- a very common way to reference other + // PRs in discussion ("see PR #N", "regressed after PR #N", "blocked on PR #N") that has nothing to do with + // solving THIS issue. Before the fix, any such mention silently populated linkedPrs, which + // buildContributorOpportunities uses to exclude an issue from the available pool and buildIssueQualityReport + // uses to force status to "do_not_use" -- hiding a fully open, unclaimed issue from recommendations. + it("does not treat a bare PR mention with no closing keyword as a real linked PR", async () => { + const env = createTestEnv(); + await upsertIssueFromGitHub(env, "owner/repo", { + number: 55, + title: "Discussion mentions an unrelated PR", + state: "open", + user: { login: "JSONbored" }, + labels: [], + body: "...similar to what we saw in PR #501, unrelated feature. Also see pull request #502 for context.", + }); + + expect(await getIssue(env, "owner/repo", 55)).toMatchObject({ linkedPrs: [] }); + }); + it("persists a per-PR slop assessment, round-trips it via the cached record, and keeps latest-wins (PR2)", async () => { const env = createTestEnv(); await upsertPullRequestFromGitHub(env, "owner/sloppr", { number: 5, title: "Churn", state: "open", user: { login: "alice" }, labels: [], body: "x" }); diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index f5eb4453b3..7f31ad763f 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -161,7 +161,7 @@ describe("database row parser hardening", () => { ); }); - it("REGRESSION: adding another linked issue resets the PR-level claim time", async () => { + it("REGRESSION: adding an OVERLAPPING linked issue does NOT reset the PR-level claim time; only a fully disjoint set does", async () => { const env = createTestEnv(); vi.useFakeTimers(); @@ -202,10 +202,15 @@ describe("database row parser hardening", () => { }); const expanded = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 11); + // #linked-issue-claim-overlap-preserve regression: this used to assert linkedIssueClaimedAt was reset to + // the fresh "2026-06-29T10:05:00.000Z" timestamp -- that was the AUDITED BUG itself, not the intended + // design. Adding #2 alongside the already-claimed #1 shares an overlap with the prior set ({1} ∩ {1,2} = + // {1}), so #1's ORIGINAL claim time must survive; resetting it let a later PR that also claims #1 leapfrog + // ahead in duplicate-cluster winner priority purely because this PR later mentioned an unrelated issue. expect(expanded).toMatchObject({ title: "Expanded claim", linkedIssues: [1, 2], - linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z", + linkedIssueClaimedAt: first?.linkedIssueClaimedAt, }); vi.setSystemTime(new Date("2026-06-29T10:07:00.000Z")); diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index fc673ea7a4..319b4b9874 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -3,6 +3,7 @@ import { createTestEnv } from "../helpers/d1"; import * as backfillModule from "../../src/github/backfill"; import { MAX_LINKED_ISSUE_NUMBERS } from "../../src/db/repositories"; import { + anyLinkedIssueHardRuleOn, DEFAULT_LINKED_ISSUE_HARD_RULES, evaluateLinkedIssueHardRules, hasVerifiableOpenLinkedIssueReference, @@ -596,31 +597,36 @@ describe("mergeLinkedIssueHardRuleWithPersistedViolation (#linked-issue-hard-rul it("returns the live result unchanged when it is ALREADY a violation (persisted memory adds nothing new)", () => { const live = { violated: true, reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs unless assigned by a maintainer." }; - expect(mergeLinkedIssueHardRuleWithPersistedViolation(live, notPersisted)).toBe(live); + expect(mergeLinkedIssueHardRuleWithPersistedViolation(live, notPersisted, true)).toBe(live); // A live violation's reason wins even when a DIFFERENT persisted reason also exists — freshest evidence. expect( - mergeLinkedIssueHardRuleWithPersistedViolation(live, { violatedAt: "2026-06-01T00:00:00Z", reason: "a stale, different reason" }), + mergeLinkedIssueHardRuleWithPersistedViolation(live, { violatedAt: "2026-06-01T00:00:00Z", reason: "a stale, different reason" }, true), ).toBe(live); }); it("passes through undefined (no rule applies) when nothing is persisted", () => { - expect(mergeLinkedIssueHardRuleWithPersistedViolation(undefined, notPersisted)).toBeUndefined(); + expect(mergeLinkedIssueHardRuleWithPersistedViolation(undefined, notPersisted, true)).toBeUndefined(); }); it("passes through a clean { violated: false } result unchanged when nothing is persisted", () => { const clean = { violated: false, reason: null }; - expect(mergeLinkedIssueHardRuleWithPersistedViolation(clean, notPersisted)).toBe(clean); + expect(mergeLinkedIssueHardRuleWithPersistedViolation(clean, notPersisted, true)).toBe(clean); }); // REGRESSION (dodge 1): a contributor edits the PR body during the flag-then-close grace window to strip the // "Closes #N" reference. The next pass's live re-parse then sees zero linked issues, so resolveLinkedIssueHardRule // returns `undefined` -- exactly like this "live" input. Without the persisted memory, clearLinkedIssueFlag - // would remove the pending-closure label as if the violation never happened. - it("REGRESSION (body-edit-during-grace-window): a persisted violation is enforced even when the live re-parse now finds NO linked issues at all (undefined)", () => { - const merged = mergeLinkedIssueHardRuleWithPersistedViolation(undefined, { - violatedAt: "2026-06-01T12:00:00Z", - reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs unless assigned by a maintainer.", - }); + // would remove the pending-closure label as if the violation never happened. `anyRuleOn: true` here because the + // rule that originally flagged this PR is STILL active -- only the body changed, not the config. + it("REGRESSION (body-edit-during-grace-window): a persisted violation is enforced even when the live re-parse now finds NO linked issues at all (undefined), as long as some rule is still on", () => { + const merged = mergeLinkedIssueHardRuleWithPersistedViolation( + undefined, + { + violatedAt: "2026-06-01T12:00:00Z", + reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs unless assigned by a maintainer.", + }, + true, + ); expect(merged).toEqual({ violated: true, reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs unless assigned by a maintainer.", @@ -635,6 +641,7 @@ describe("mergeLinkedIssueHardRuleWithPersistedViolation (#linked-issue-hard-rul const merged = mergeLinkedIssueHardRuleWithPersistedViolation( { violated: false, reason: null }, { violatedAt: "2026-06-01T12:00:00Z", reason: "Linked issue #9 is already assigned to @claimed-dev — only the assignee or a maintainer can submit that work." }, + true, ); expect(merged).toEqual({ violated: true, @@ -643,11 +650,51 @@ describe("mergeLinkedIssueHardRuleWithPersistedViolation (#linked-issue-hard-rul }); it("falls back to the generic reason when a persisted violation carries a null/missing reason", () => { - expect(mergeLinkedIssueHardRuleWithPersistedViolation(undefined, { violatedAt: "2026-06-01T00:00:00Z", reason: null })).toEqual({ + expect(mergeLinkedIssueHardRuleWithPersistedViolation(undefined, { violatedAt: "2026-06-01T00:00:00Z", reason: null }, true)).toEqual({ violated: true, reason: "the linked issue is not eligible for a community PR", }); }); + + // REGRESSION (#linked-issue-hard-rule-persistence-disable-rescue): a maintainer enables a rule, it flags a + // PR (persisting a violation marker), then the maintainer decides the rule is too aggressive and turns EVERY + // linkedIssueHardRule off. On the next pass, live is `undefined` because resolveLinkedIssueHardRule's own + // anyRuleOn guard short-circuits (no rule is "block" at all anymore) -- NOT because a rule is still active + // but this pass's body/issue-state dodged detection (that's the `anyRuleOn: true` cases above). The persisted + // marker must NOT resurrect a violation from a rule that no longer exists, or the PR stays condemned to a + // one-shot close forever despite the maintainer's own deliberate config change. + it("REGRESSION (all rules disabled): a persisted violation is NOT resurrected once every linkedIssueHardRule is off", () => { + const merged = mergeLinkedIssueHardRuleWithPersistedViolation( + undefined, + { + violatedAt: "2026-06-01T12:00:00Z", + reason: "Linked issue #9 is assigned to the maintainer (@acme) — that work is reserved for the maintainer, so this PR cannot be auto-accepted.", + }, + false, + ); + expect(merged).toBeUndefined(); + }); + + // Same rescue case, but live evaluated to a clean result THIS pass rather than undefined (e.g. a stale + // in-flight computation) -- anyRuleOn: false must win regardless of what live looked like. + it("REGRESSION (all rules disabled): a persisted violation is NOT resurrected even if live is a clean result", () => { + const clean = { violated: false, reason: null }; + const merged = mergeLinkedIssueHardRuleWithPersistedViolation(clean, { violatedAt: "2026-06-01T12:00:00Z", reason: "stale reason" }, false); + expect(merged).toBe(clean); + }); +}); + +describe("anyLinkedIssueHardRuleOn (#linked-issue-hard-rule-persistence-disable-rescue)", () => { + it("is false when every rule is off", () => { + expect(anyLinkedIssueHardRuleOn(config())).toBe(false); + }); + + it("is true when any single rule is block", () => { + expect(anyLinkedIssueHardRuleOn(config({ ownerAssignedClose: "block" }))).toBe(true); + expect(anyLinkedIssueHardRuleOn(config({ assignedIssueClose: "block" }))).toBe(true); + expect(anyLinkedIssueHardRuleOn(config({ missingPointLabelClose: "block" }))).toBe(true); + expect(anyLinkedIssueHardRuleOn(config({ maintainerOnlyLabelClose: "block" }))).toBe(true); + }); }); describe("hasVerifiableOpenLinkedIssueReference (#unlinked-issue-guardrail-followup — pure evaluator)", () => { diff --git a/test/unit/queue-lifecycle-guards.test.ts b/test/unit/queue-lifecycle-guards.test.ts index 57868219c1..76b3649dc7 100644 --- a/test/unit/queue-lifecycle-guards.test.ts +++ b/test/unit/queue-lifecycle-guards.test.ts @@ -3448,8 +3448,8 @@ describe("review-evasion protection (#review-evasion-protection)", () => { updated_at: "2026-05-27T00:00:00Z", } as never); - expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 77)).toBe(1); - expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 77)).toBe(2); + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 77, "delivery-1")).toBe(1); + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 77, "delivery-2")).toBe(2); // A fresh push (new head SHA) between cycles must NOT reset the counter -- unlike mergeAttemptCount. await repositoriesModule.upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { id: 4242, @@ -3464,12 +3464,46 @@ describe("review-evasion protection (#review-evasion-protection)", () => { created_at: "2026-05-27T00:00:00Z", updated_at: "2026-05-27T00:00:00Z", } as never); - expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 77)).toBe(3); + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 77, "delivery-3")).toBe(3); }); it("returns 0 for a PR that does not exist (no row to increment)", async () => { const env = createTestEnv({}); - expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 999999)).toBe(0); + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 999999, "delivery-x")).toBe(0); + }); + + // REGRESSION (#draft-conversion-retry-double-count): a queue retry of the SAME webhook message (a GitHub + // 5xx, rate-limit, or transient D1 write failure later in the SAME processGitHubWebhook pass) redelivers + // the identical deliveryId. Before this fix, bumpPullRequestDraftConversionCount had zero idempotency + // protection, so that retry re-bumped the counter for ONE real, first-ever, entirely legitimate draft + // conversion -- pushing the count from 1 to 2 and firing maybeCloseRepeatedDraftCycling's false "2nd + // offense" close + moderation strike against an innocent contributor. + it("REGRESSION (#draft-conversion-retry-double-count): a retry redelivery of the SAME deliveryId does not double-count a single physical conversion", async () => { + const env = createTestEnv({}); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await repositoriesModule.upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + id: 5252, + number: 88, + state: "open", + title: "First-ever draft conversion", + user: { login: "contributor" }, + head: { sha: "sha-retry-1", ref: "fix", repo: { full_name: "contributor/gittensory", owner: { login: "contributor" } } }, + base: { sha: "base123", ref: "main", repo: { full_name: "JSONbored/gittensory", owner: { login: "JSONbored" } } }, + draft: false, + merged: false, + created_at: "2026-05-27T00:00:00Z", + updated_at: "2026-05-27T00:00:00Z", + } as never); + + // The original webhook processing attempt bumps the counter... + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 88, "delivery-retry-abc")).toBe(1); + // ...then a downstream step throws a retryable error, the queue consumer calls message.retry(), and the + // SAME message body (SAME deliveryId) is redelivered and reprocessed from the top. Must stay at 1, not 2. + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 88, "delivery-retry-abc")).toBe(1); + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 88, "delivery-retry-abc")).toBe(1); + + // A genuinely DIFFERENT, later delivery (a real second draft conversion) still counts as a new offense. + expect(await repositoriesModule.bumpPullRequestDraftConversionCount(env, "JSONbored/gittensory", 88, "delivery-real-second")).toBe(2); }); }); });