Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 73 additions & 15 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<IssueRecord> {
Expand Down Expand Up @@ -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<number> {
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<number> {
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)
Expand Down Expand Up @@ -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))];
}
15 changes: 10 additions & 5 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
)
Expand Down
50 changes: 33 additions & 17 deletions src/review/linked-issue-hard-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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" };
}
Expand All @@ -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<LinkedIssueHardRuleResult | undefined> {
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,
Expand Down
28 changes: 27 additions & 1 deletion test/unit/data-spine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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" });
Expand Down
9 changes: 7 additions & 2 deletions test/unit/db-parsers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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"));
Expand Down
Loading