Skip to content

Commit f94197d

Browse files
committed
fix(queue): fast-track a PR whose merge/close got denied by a live staleness recheck
A duplicate-cluster winner merging, a base conflict clearing, or GitHub finishing its async mergeable_state computation all carry no webhook to the affected PR, so a review-good PR left in this state waited out the ordinary sweep cadence instead of getting a prompt re-look. Extends the existing outage-repair priority set (surfaceRepairPriorityPullNumbers) to also fast-track these cases, reusing its existing per-head-SHA attempt cap so a genuinely stuck PR still falls back to ordinary cadence.
1 parent 1454d22 commit f94197d

4 files changed

Lines changed: 347 additions & 2 deletions

File tree

‎src/db/repositories.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2942,6 +2942,57 @@ export async function countRecentAuditEventsForActorInRepoWithTargetSuffix(
29422942
return row.count;
29432943
}
29442944

2945+
// #orb-stale-recheck-priority: the THREE self-resolving "denied" detail strings the executor's live staleness
2946+
// rechecks can produce (agent-action-executor.ts's "8) Live ... re-verification" block -- duplicateStaleReason /
2947+
// mergeableStaleReason / threadStaleReason, each suffixed with " — action not executed" by that block's shared
2948+
// `audit("denied", ...)` call). Deliberately NOT imported from agent-action-executor.ts: that module imports
2949+
// FROM db/repositories.ts (installation tokens, PR records, ...), so importing back would create a real
2950+
// module-load cycle -- same hazard agent-actions.ts's CONCRETE_EVIDENCE_BLOCKER_CODES comment documents for the
2951+
// identical reason. A source-text parity test guards these three literals against producer-side drift instead.
2952+
// Deliberately EXCLUDES a CI-staleness denial (ciStaleReason): CI flipping already re-triggers a fresh
2953+
// evaluation via the check-run/status webhook that changed it, so it doesn't share the other three's "no
2954+
// webhook ever reaches this PR" gap.
2955+
const STALE_RECHECK_DENIAL_DETAIL_PATTERN =
2956+
/^(duplicate-cluster winner #\d+ is no longer open|the base-branch conflict that justified this close has since cleared|the review thread\(s\) that justified this close are now all resolved) — action not executed$/;
2957+
2958+
/**
2959+
* PR numbers within `repoFullName` whose most recent `agent.action.close`/`agent.action.merge` attempt was
2960+
* DENIED by one of the executor's live staleness rechecks (duplicate-cluster winner / base-conflict / review-
2961+
* thread) within `sinceIso`, rather than by a durable, externally-actioned reason (a manual-review label, a
2962+
* merge-train wait, contributor-cap contention, ...). Those rechecks exist precisely because the fact that
2963+
* justified the close/merge can flip WITHOUT a webhook ever notifying THIS pr -- a duplicate-cluster sibling
2964+
* merging fires a webhook about the SIBLING, not this PR, so nothing naturally re-triggers a look here. Callers
2965+
* (surfaceRepairPriorityPullNumbers) fold this into the SAME priority set the outage-repair path already uses,
2966+
* so a matching PR gets a fast, prioritized re-look instead of waiting out the ordinary sweep cadence -- and
2967+
* inherits that path's existing per-head-SHA attempt cap for free, so a genuinely stuck PR still falls back to
2968+
* ordinary cadence rather than being re-selected forever. Reuses the same literal-prefix `LIKE ... ESCAPE`
2969+
* scoping as {@link countRecentAuditEventsForActorInRepo} so a repo name containing a SQL wildcard is matched
2970+
* literally.
2971+
*/
2972+
export async function recentStaleRecheckDeniedPullNumbers(env: Env, repoFullName: string, sinceIso: string): Promise<number[]> {
2973+
const db = getDb(env.DB);
2974+
const targetPrefixPattern = `${escapeSqlLikePattern(repoFullName)}#%`;
2975+
const rows = await db
2976+
.select({ targetKey: auditEvents.targetKey, detail: auditEvents.detail })
2977+
.from(auditEvents)
2978+
.where(
2979+
and(
2980+
eq(auditEvents.actor, "loopover"),
2981+
inArray(auditEvents.eventType, ["agent.action.close", "agent.action.merge"]),
2982+
eq(auditEvents.outcome, "denied"),
2983+
sql`${auditEvents.targetKey} LIKE ${targetPrefixPattern} ESCAPE '\\'`,
2984+
gte(auditEvents.createdAt, sinceIso),
2985+
),
2986+
);
2987+
const pullNumbers = new Set<number>();
2988+
for (const row of rows) {
2989+
if (!row.detail || !STALE_RECHECK_DENIAL_DETAIL_PATTERN.test(row.detail)) continue;
2990+
const target = parsePullRequestTargetKey(row.targetKey);
2991+
if (target) pullNumbers.add(target.pullNumber);
2992+
}
2993+
return [...pullNumbers];
2994+
}
2995+
29452996
/** #orb-ci-stuck-repeat / #orb-retry-storm ops-alerts signal: the single PR within `repoFullName` that published
29462997
* the most review surfaces in the last `sinceIso`-bounded window, and how many. `github_app.pr_public_surface_
29472998
* published` is a genuine INSERT-only event (never upserted) recorded once per successful publish pass

‎src/queue/processors.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ import {
6565
countRecentAuditEventsForActorAndTarget,
6666
countRecentAuditEventsForActorInRepo,
6767
countRecentAuditEventsForActorInRepoWithTargetSuffix,
68+
recentStaleRecheckDeniedPullNumbers,
6869
hasAuditEventForDelivery,
6970
hasAuditEventForHeadSha,
7071
recordGateBlockOutcome,
@@ -1123,7 +1124,7 @@ async function isRegateRepairExhausted(env: Env, repoFullName: string, pr: Pick<
11231124
return true;
11241125
}
11251126

1126-
async function surfaceRepairPriorityPullNumbers(
1127+
export async function surfaceRepairPriorityPullNumbers(
11271128
env: Env,
11281129
repoFullName: string,
11291130
pulls: readonly PullRequestRecord[],
@@ -1133,7 +1134,23 @@ async function surfaceRepairPriorityPullNumbers(
11331134
for (const pr of pulls) {
11341135
if (pr.headSha && pr.lastPublishedSurfaceSha !== pr.headSha)
11351136
priorityPullNumbers.add(pr.number);
1137+
// #orb-stale-recheck-priority: the bot already approved THIS exact commit (approvedHeadSha === headSha,
1138+
// set only once agentHoldAuditDetail's own gate-passed/CI-green/approvals-satisfied checks all cleared),
1139+
// yet the PR is still open with mergeable_state neither "clean" (would already have merged) nor "dirty"
1140+
// (a real conflict, which needs a human, not a fast recheck) -- i.e. GitHub is still computing
1141+
// mergeability. GitHub sends NO webhook when that computation finishes, so without this the PR waits out
1142+
// whatever the ordinary sweep cadence happens to be instead of the few seconds this actually takes
1143+
// (observed: green+approved PRs stuck OPEN, see fetchLivePullRequestMergeState's own doc comment).
1144+
if (pr.headSha && pr.approvedHeadSha === pr.headSha && pr.mergeableState && pr.mergeableState !== "clean" && pr.mergeableState !== "dirty")
1145+
priorityPullNumbers.add(pr.number);
11361146
}
1147+
// Scoped to `pulls` (not added unconditionally): a denial recorded against a PR that has SINCE closed/merged
1148+
// (so no longer appears in this open-PR list) must not resurrect a priority entry for it here — and doing so
1149+
// would violate the "every priorityPullNumbers entry has a pulls match with a truthy headSha" invariant the
1150+
// exhaustion-check loop below relies on.
1151+
const openPullNumbers = new Set(pulls.filter((pr) => pr.headSha).map((pr) => pr.number));
1152+
for (const prNumber of await recentStaleRecheckDeniedPullNumbers(env, repoFullName, new Date(Date.now() - REGATE_REPAIR_ATTEMPT_LOOKBACK_MS).toISOString()))
1153+
if (openPullNumbers.has(prNumber)) priorityPullNumbers.add(prNumber);
11371154
if (gateCheckEnabled) {
11381155
await Promise.all(
11391156
pulls.map(async (pr) => {
@@ -1154,7 +1171,7 @@ async function surfaceRepairPriorityPullNumbers(
11541171
await Promise.all(
11551172
[...priorityPullNumbers].map(async (prNumber) => {
11561173
const pr = pulls.find((candidate) => candidate.number === prNumber);
1157-
/* v8 ignore next -- priorityPullNumbers is only ever populated (both loops above) from a `pr` in `pulls` that already had a truthy headSha, so this lookup always succeeds with one; the guard only satisfies Array#find's `| undefined` return type. */
1174+
/* v8 ignore next -- priorityPullNumbers is only ever populated (every loop above, including the openPullNumbers-scoped stale-recheck one) from a `pr` in `pulls` that already had a truthy headSha, so this lookup always succeeds with one; the guard only satisfies Array#find's `| undefined` return type. */
11581175
if (!pr?.headSha) return;
11591176
if (await isRegateRepairExhausted(env, repoFullName, pr)) priorityPullNumbers.delete(prNumber);
11601177
}),

‎test/unit/db-parsers.test.ts‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { readFileSync } from "node:fs";
12
import { afterEach, describe, expect, it, vi } from "vitest";
23
import {
34
claimMaintainerRecapPeriod,
@@ -8,6 +9,7 @@ import {
89
countRecentAuditEventsForActorAndTarget,
910
countRecentAuditEventsForActorInRepo,
1011
countRecentAuditEventsForActorInRepoWithTargetSuffix,
12+
recentStaleRecheckDeniedPullNumbers,
1113
findHottestInconclusiveReviewTargetForRepo,
1214
findHottestReviewTargetForRepo,
1315
hasAuditEventForDelivery,
@@ -946,6 +948,124 @@ describe("database row parser hardening", () => {
946948
expect(await countRecentAuditEventsForActorInRepoWithTargetSuffix(env, "chatty", "github_app.monitored_mention_ping", "owner/foo_bar", "mention:some_login", "2026-06-24T09:00:00.000Z")).toBe(1);
947949
});
948950

951+
describe("recentStaleRecheckDeniedPullNumbers (#orb-stale-recheck-priority)", () => {
952+
it("returns the PR number for a duplicate-cluster-winner staleness denial", async () => {
953+
const env = createTestEnv();
954+
await recordAuditEvent(env, {
955+
eventType: "agent.action.close",
956+
actor: "loopover",
957+
targetKey: "owner/repo#5",
958+
outcome: "denied",
959+
detail: "duplicate-cluster winner #7437 is no longer open — action not executed",
960+
createdAt: "2026-06-24T10:00:00.000Z",
961+
});
962+
963+
expect(await recentStaleRecheckDeniedPullNumbers(env, "owner/repo", "2026-06-24T09:00:00.000Z")).toEqual([5]);
964+
});
965+
966+
it("returns the PR number for a base-conflict or a review-thread staleness denial, and dedupes repeats for the same PR", async () => {
967+
const env = createTestEnv();
968+
await recordAuditEvent(env, {
969+
eventType: "agent.action.close",
970+
actor: "loopover",
971+
targetKey: "owner/repo#6",
972+
outcome: "denied",
973+
detail: "the base-branch conflict that justified this close has since cleared — action not executed",
974+
createdAt: "2026-06-24T10:00:00.000Z",
975+
});
976+
// A second denial for the SAME pr later in the window must not produce a duplicate entry.
977+
await recordAuditEvent(env, {
978+
eventType: "agent.action.merge",
979+
actor: "loopover",
980+
targetKey: "owner/repo#6",
981+
outcome: "denied",
982+
detail: "the review thread(s) that justified this close are now all resolved — action not executed",
983+
createdAt: "2026-06-24T10:05:00.000Z",
984+
});
985+
986+
expect(await recentStaleRecheckDeniedPullNumbers(env, "owner/repo", "2026-06-24T09:00:00.000Z")).toEqual([6]);
987+
});
988+
989+
it("ignores a denial reason that is NOT one of the three self-resolving staleness reasons", async () => {
990+
const env = createTestEnv();
991+
// A manual-review-label denial is durable (needs a human to remove the label) -- retrying fast wastes a
992+
// cycle for zero output, so it must not be surfaced here.
993+
await recordAuditEvent(env, {
994+
eventType: "agent.action.merge",
995+
actor: "loopover",
996+
targetKey: "owner/repo#7",
997+
outcome: "denied",
998+
detail: 'manual-review label "manual-review" is present on the live PR — merge not executed',
999+
createdAt: "2026-06-24T10:00:00.000Z",
1000+
});
1001+
// A live-CI staleness denial already gets a fresh look from the check-run/status webhook that changed
1002+
// CI in the first place, so it's deliberately excluded too.
1003+
await recordAuditEvent(env, {
1004+
eventType: "agent.action.merge",
1005+
actor: "loopover",
1006+
targetKey: "owner/repo#8",
1007+
outcome: "denied",
1008+
detail: "live CI is no longer passing (now: pending) — action not executed",
1009+
createdAt: "2026-06-24T10:00:00.000Z",
1010+
});
1011+
1012+
expect(await recentStaleRecheckDeniedPullNumbers(env, "owner/repo", "2026-06-24T09:00:00.000Z")).toEqual([]);
1013+
});
1014+
1015+
it("ignores a matching denial outside the actor/eventType/outcome/repo/time scope", async () => {
1016+
const env = createTestEnv();
1017+
const matchingDetail = "duplicate-cluster winner #1 is no longer open — action not executed";
1018+
// Wrong actor.
1019+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "someone-else", targetKey: "owner/repo#10", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:00:00.000Z" });
1020+
// Wrong event type (a "completed" close whose OWN detail happens to reuse the phrase, e.g. a comment quoting it).
1021+
await recordAuditEvent(env, { eventType: "agent.action.hold", actor: "loopover", targetKey: "owner/repo#11", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:00:00.000Z" });
1022+
// Wrong outcome (the SAME reason text, but the action actually went through).
1023+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/repo#12", outcome: "completed", detail: matchingDetail, createdAt: "2026-06-24T10:00:00.000Z" });
1024+
// Wrong repo.
1025+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/other-repo#13", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:00:00.000Z" });
1026+
// Before the cutoff.
1027+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/repo#14", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T07:00:00.000Z" });
1028+
1029+
expect(await recentStaleRecheckDeniedPullNumbers(env, "owner/repo", "2026-06-24T09:00:00.000Z")).toEqual([]);
1030+
});
1031+
1032+
it("treats the repo name as a literal LIKE prefix (regression mirroring countRecentAuditEventsForActorInRepo's escaping fix)", async () => {
1033+
const env = createTestEnv();
1034+
const matchingDetail = "duplicate-cluster winner #1 is no longer open — action not executed";
1035+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/foo_bar#20", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:00:00.000Z" });
1036+
// owner/fooXbar is a DIFFERENT repo that would spuriously match "owner/foo_bar#%" if `_` were left as a
1037+
// SQL wildcard instead of being escaped.
1038+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/fooXbar#21", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:01:00.000Z" });
1039+
1040+
expect(await recentStaleRecheckDeniedPullNumbers(env, "owner/foo_bar", "2026-06-24T09:00:00.000Z")).toEqual([20]);
1041+
});
1042+
1043+
it("skips a matching row whose targetKey fails to parse into a pull number (matches the LIKE prefix but has a non-numeric suffix)", async () => {
1044+
const env = createTestEnv();
1045+
const matchingDetail = "duplicate-cluster winner #1 is no longer open — action not executed";
1046+
// Satisfies the "owner/repo#%" SQL prefix but parsePullRequestTargetKey rejects the non-integer suffix.
1047+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/repo#abc", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:00:00.000Z" });
1048+
await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/repo#22", outcome: "denied", detail: matchingDetail, createdAt: "2026-06-24T10:01:00.000Z" });
1049+
1050+
expect(await recentStaleRecheckDeniedPullNumbers(env, "owner/repo", "2026-06-24T09:00:00.000Z")).toEqual([22]);
1051+
});
1052+
1053+
// #orb-stale-recheck-priority parity guard: STALE_RECHECK_DENIAL_DETAIL_PATTERN hand-types the three
1054+
// "denied" reason strings the executor's live staleness rechecks can produce, rather than importing them
1055+
// from agent-action-executor.ts -- that module imports FROM db/repositories.ts, so the reverse import would
1056+
// create a real module-load cycle (same hazard CONCRETE_EVIDENCE_BLOCKER_CODES's own parity guard,
1057+
// agent-actions.test.ts, documents). This reads the real producer source text instead, so a future rewording
1058+
// at the producer fails this test immediately rather than silently making the hand-typed pattern permanently
1059+
// unmatchable.
1060+
it.each(["duplicate-cluster winner #${action.duplicateWinnerPrNumber} is no longer open", "the base-branch conflict that justified this close has since cleared", "the review thread(s) that justified this close are now all resolved"])(
1061+
"%s is still produced (not merely mentioned) in its producer (src/services/agent-action-executor.ts)",
1062+
(reason) => {
1063+
const source = readFileSync("src/services/agent-action-executor.ts", "utf8");
1064+
expect(source).toContain(reason);
1065+
},
1066+
);
1067+
});
1068+
9491069
it("findHottestReviewTargetForRepo returns the PR with the most published surfaces in the window, scoped to ONE repo (#orb-ci-stuck-repeat)", async () => {
9501070
const env = createTestEnv();
9511071
const publish = (targetKey: string, createdAt: string) =>

0 commit comments

Comments
 (0)