fix(selfhost): stop delayed webhooks from resurrecting closed PRs' review tracking

.loopover.yml.example

@@ -1428,6 +1428,14 @@ settings:
 # prReconciliation:
 #   enabled: true   # Bool. Default: false (the env var decides instead).
 
+# Fleet-wide active_review_tracking reconciliation sweep (#webhook-reorder-clobber): config-as-code override
+# for the LOOPOVER_ACTIVE_REVIEW_RECONCILIATION flag. A delayed webhook job can leave a PR's review-tracking
+# row stuck "active" after the PR has actually already closed on GitHub (queue backpressure widens the race).
+# This sweep re-checks stale "active" rows against LIVE GitHub state and terminalizes the ones confirmed
+# closed. Same shape and precedence as `prReconciliation:` above.
+# activeReviewReconciliation:
+#   enabled: true   # Bool. Default: false (the env var decides instead).
+
 # Opt-in federated fleet intelligence export (#1970): packages this instance's OWN calibration signals into a
 # signed, anonymized bundle an operator can choose to hand to a peer (or to a collector they run). Exports
 # AGGREGATE figures only -- gate precision, reversal/slop/copycat rates over a window -- never source code,

config/examples/loopover.full.yml

@@ -1442,6 +1442,14 @@ settings:
 # prReconciliation:
 #   enabled: true   # Bool. Default: false (the env var decides instead).
 
+# Fleet-wide active_review_tracking reconciliation sweep (#webhook-reorder-clobber): config-as-code override
+# for the LOOPOVER_ACTIVE_REVIEW_RECONCILIATION flag. A delayed webhook job can leave a PR's review-tracking
+# row stuck "active" after the PR has actually already closed on GitHub (queue backpressure widens the race).
+# This sweep re-checks stale "active" rows against LIVE GitHub state and terminalizes the ones confirmed
+# closed. Same shape and precedence as `prReconciliation:` above.
+# activeReviewReconciliation:
+#   enabled: true   # Bool. Default: false (the env var decides instead).
+
 # Opt-in federated fleet intelligence export (#1970): packages this instance's OWN calibration signals into a
 # signed, anonymized bundle an operator can choose to hand to a peer (or to a collector they run). Exports
 # AGGREGATE figures only -- gate precision, reversal/slop/copycat rates over a window -- never source code,

migrations/0172_pull_requests_github_updated_at.sql

@@ -0,0 +1,8 @@
+-- Out-of-order webhook guard (#webhook-reorder-clobber): a webhook queued behind a slow/congested job can be
+-- processed AFTER a later event for the same PR already landed and applied a newer state -- its embedded PR
+-- snapshot is then stale and must not regress lifecycle-identity fields (state/headSha/mergedAt) GitHub has
+-- already moved past. This column stores GitHub's OWN `updated_at` for the PR (distinct from `updated_at`,
+-- which is app bookkeeping) so upsertPullRequestFromGitHub can compare incoming vs. stored before applying a
+-- write. NULL for every existing row -- the guard fails open (applies the write) whenever it has nothing to
+-- compare against, so this backfills itself the next time each PR is synced.
+ALTER TABLE pull_requests ADD COLUMN github_updated_at TEXT;

packages/loopover-engine/src/config-lint.ts

@@ -25,6 +25,7 @@ const TOP_LEVEL_FIELDS = [
   "upstreamDriftIssues",
   "sweepWatchdog",
   "prReconciliation",
+  "activeReviewReconciliation",
   "federatedIntelligence",
 ] as const;
 

packages/loopover-engine/src/focus-manifest-validation.ts

@@ -13,6 +13,7 @@ import {
   upstreamDriftIssuesConfigToJson,
   sweepWatchdogConfigToJson,
   prReconciliationConfigToJson,
+  activeReviewReconciliationConfigToJson,
   federatedIntelligenceConfigToJson,
   settingsOverrideToJson,
   type FocusManifest,
@@ -95,6 +96,8 @@ function focusManifestToNormalizedJson(manifest: FocusManifest): Record<string,
   if (sweepWatchdog !== null) normalized.sweepWatchdog = sweepWatchdog;
   const prReconciliation = prReconciliationConfigToJson(manifest.prReconciliation);
   if (prReconciliation !== null) normalized.prReconciliation = prReconciliation;
+  const activeReviewReconciliation = activeReviewReconciliationConfigToJson(manifest.activeReviewReconciliation);
+  if (activeReviewReconciliation !== null) normalized.activeReviewReconciliation = activeReviewReconciliation;
   const federatedIntelligence = federatedIntelligenceConfigToJson(manifest.federatedIntelligence);
   if (federatedIntelligence !== null) normalized.federatedIntelligence = federatedIntelligence;
 

packages/loopover-engine/src/focus-manifest.ts

@@ -494,6 +494,18 @@ export type FocusManifestPrReconciliationConfig = {
   enabled: boolean;
 };
 
+/**
+ * Config-as-code override for the active_review_tracking reconciliation sweep
+ * (LOOPOVER_ACTIVE_REVIEW_RECONCILIATION), declared under top-level `activeReviewReconciliation:`
+ * (#webhook-reorder-clobber). Same shape and precedence as `prReconciliation:` above -- the sweep re-checks
+ * `active_review_tracking` rows a delayed webhook job left stuck "active" for a PR that already closed.
+ * Not present ⇒ the caller falls back to the LOOPOVER_ACTIVE_REVIEW_RECONCILIATION env var.
+ */
+export type FocusManifestActiveReviewReconciliationConfig = {
+  present: boolean;
+  enabled: boolean;
+};
+
 /**
  * Config-as-code opt-in for the federated fleet intelligence export (#1970), declared under
  * `federatedIntelligence:`. Gates buildFederatedBundle (src/orb/federated-bundle.ts), which packages this
@@ -1194,6 +1206,7 @@ export type FocusManifest = {
   upstreamDriftIssues: FocusManifestUpstreamDriftIssuesConfig;
   sweepWatchdog: FocusManifestSweepWatchdogConfig;
   prReconciliation: FocusManifestPrReconciliationConfig;
+  activeReviewReconciliation: FocusManifestActiveReviewReconciliationConfig;
   federatedIntelligence: FocusManifestFederatedIntelligenceConfig;
   warnings: string[];
 };
@@ -1374,6 +1387,11 @@ const EMPTY_PR_RECONCILIATION_CONFIG: FocusManifestPrReconciliationConfig = {
   enabled: false,
 };
 
+const EMPTY_ACTIVE_REVIEW_RECONCILIATION_CONFIG: FocusManifestActiveReviewReconciliationConfig = {
+  present: false,
+  enabled: false,
+};
+
 const EMPTY_FEDERATED_INTELLIGENCE_CONFIG: FocusManifestFederatedIntelligenceConfig = {
   present: false,
   enabled: false,
@@ -1408,6 +1426,7 @@ const EMPTY_MANIFEST: FocusManifest = {
   upstreamDriftIssues: { ...EMPTY_UPSTREAM_DRIFT_ISSUES_CONFIG },
   sweepWatchdog: { ...EMPTY_SWEEP_WATCHDOG_CONFIG },
   prReconciliation: { ...EMPTY_PR_RECONCILIATION_CONFIG },
+  activeReviewReconciliation: { ...EMPTY_ACTIVE_REVIEW_RECONCILIATION_CONFIG },
   federatedIntelligence: { ...EMPTY_FEDERATED_INTELLIGENCE_CONFIG },
   warnings: [],
 };
@@ -1448,6 +1467,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo
     upstreamDriftIssues: { ...EMPTY_UPSTREAM_DRIFT_ISSUES_CONFIG },
     sweepWatchdog: { ...EMPTY_SWEEP_WATCHDOG_CONFIG },
     prReconciliation: { ...EMPTY_PR_RECONCILIATION_CONFIG },
+    activeReviewReconciliation: { ...EMPTY_ACTIVE_REVIEW_RECONCILIATION_CONFIG },
     federatedIntelligence: { ...EMPTY_FEDERATED_INTELLIGENCE_CONFIG },
   };
 }
@@ -2328,6 +2348,29 @@ export function prReconciliationConfigToJson(config: FocusManifestPrReconciliati
   return { enabled: config.enabled };
 }
 
+/**
+ * Parse the optional top-level `activeReviewReconciliation:` mapping (#webhook-reorder-clobber). Mirrors
+ * {@link parsePrReconciliationConfig} exactly — `enabled` is the only field.
+ */
+function parseActiveReviewReconciliationConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestActiveReviewReconciliationConfig {
+  if (value === undefined || value === null) return { ...EMPTY_ACTIVE_REVIEW_RECONCILIATION_CONFIG };
+  if (typeof value !== "object" || Array.isArray(value)) {
+    warnings.push('Manifest field "activeReviewReconciliation" must be a mapping; ignoring it.');
+    return { ...EMPTY_ACTIVE_REVIEW_RECONCILIATION_CONFIG };
+  }
+  const record = value as Record<string, JsonValue>;
+  const enabled = normalizeOptionalBoolean(record.enabled, "activeReviewReconciliation.enabled", warnings) ?? false;
+  return { present: true, enabled };
+}
+
+/** Serialize an activeReviewReconciliation config back into the parse-compatible shape so a cached snapshot
+ *  round-trips through {@link parseActiveReviewReconciliationConfig} unchanged. Returns null when nothing is
+ *  configured. */
+export function activeReviewReconciliationConfigToJson(config: FocusManifestActiveReviewReconciliationConfig): JsonValue {
+  if (!config.present) return null;
+  return { enabled: config.enabled };
+}
+
 /**
  * Parse the optional `federatedIntelligence:` mapping (#1970). Mirrors {@link parseUpstreamDriftIssuesConfig}
  * exactly -- `enabled` is the only field, defaulting to false, so the parsed value IS the effective value and
@@ -3884,6 +3927,7 @@ export function parseFocusManifest(raw: unknown, source?: FocusManifestSource):
     upstreamDriftIssues: parseUpstreamDriftIssuesConfig(record.upstreamDriftIssues, warnings),
     sweepWatchdog: parseSweepWatchdogConfig(record.sweepWatchdog, warnings),
     prReconciliation: parsePrReconciliationConfig(record.prReconciliation, warnings),
+    activeReviewReconciliation: parseActiveReviewReconciliationConfig(record.activeReviewReconciliation, warnings),
     federatedIntelligence: parseFederatedIntelligenceConfig(record.federatedIntelligence, warnings),
     warnings,
   };
@@ -3910,6 +3954,7 @@ export function parseFocusManifest(raw: unknown, source?: FocusManifestSource):
     !manifest.upstreamDriftIssues.present &&
     !manifest.sweepWatchdog.present &&
     !manifest.prReconciliation.present &&
+    !manifest.activeReviewReconciliation.present &&
     !manifest.federatedIntelligence.present
   ) {
     warnings.push("Manifest contained no recognized focus fields; falling back to deterministic signals.");

packages/loopover-engine/src/index.ts

@@ -789,6 +789,7 @@ export {
   upstreamDriftIssuesConfigToJson,
   sweepWatchdogConfigToJson,
   prReconciliationConfigToJson,
+  activeReviewReconciliationConfigToJson,
   federatedIntelligenceConfigToJson,
   FEDERATED_COLLECTOR_MODES,
   settingsOverrideToJson,

src/db/repositories.ts

@@ -1,5 +1,5 @@
 import { parsePullRequestTargetKey } from "@loopover/engine";
-import { and, asc, desc, eq, gte, inArray, isNotNull, not, or, sql, type SQL } from "drizzle-orm";
+import { and, asc, desc, eq, gte, inArray, isNotNull, lt, not, or, sql, type SQL } from "drizzle-orm";
 import { getDb } from "./client";
 import {
   activeReviewTracking,
@@ -350,7 +350,6 @@ export async function upsertPullRequestFromGitHub(
   const record = toPullRequestRecord(repoFullName, pr);
   const db = getDb(env.DB);
   const syncedAt = nowIso();
-  const lastSeenOpenAt = pr.state === "open" ? (options.seenOpenAt ?? syncedAt) : null;
   const existingClaimRows = await db
     .select({
       linkedIssuesJson: pullRequests.linkedIssuesJson,
@@ -359,6 +358,9 @@ export async function upsertPullRequestFromGitHub(
       bodyObservedAt: pullRequests.bodyObservedAt,
       headSha: pullRequests.headSha,
       headShaObservedAt: pullRequests.headShaObservedAt,
+      state: pullRequests.state,
+      mergedAt: pullRequests.mergedAt,
+      githubUpdatedAt: pullRequests.githubUpdatedAt,
     })
     .from(pullRequests)
     .where(and(eq(pullRequests.repoFullName, repoFullName), eq(pullRequests.number, pr.number)))
@@ -371,6 +373,33 @@ export async function upsertPullRequestFromGitHub(
   // `linkedIssues.length === 0` branch) on any such upsert. Fall back to whatever is already stored in that
   // case; only a genuinely observed (possibly empty) body updates the claim. (#linked-issue-sparse-payload-preserve)
   const existingClaimRow = existingClaimRows[0];
+  // Out-of-order webhook guard (#webhook-reorder-clobber): a webhook for an OLDER event (e.g. `review_requested`)
+  // can be dequeued AFTER a NEWER event for the same PR (e.g. `closed`) already landed, if the older job was
+  // stuck behind queue backpressure -- its embedded `pull_request` snapshot is then stale and must not regress
+  // state/headSha/mergedAt back to what GitHub reported minutes ago (this is exactly how an already-closed PR's
+  // active_review_tracking row got resurrected: a delayed job re-saw `state: "open"` and restarted a review
+  // pass nothing ever terminalized). Compares GitHub's own `updated_at` against what this row last observed;
+  // `isStalePayload` is only ever true when BOTH sides have a real timestamp to compare AND the incoming one is
+  // strictly older -- a sparse payload (no `updated_at`) or a pre-migration/first-ever row (`githubUpdatedAt`
+  // absent) fails OPEN, applying the write exactly as before this guard existed. Decided in JS (not SQL), up
+  // front, so EVERY downstream computation below (lastSeenOpenAt, isReadyForReview, the headShaObservedAt clock,
+  // and this call's own RETURNED record) reasons from the same resolved values instead of the raw payload --
+  // otherwise a rejected-as-stale write could still corrupt those derived fields even though state/headSha/
+  // mergedAt themselves were protected. Deliberately NOT extended to `draft`/`isDraft` (isReadyForReview's other
+  // input): no reported failure mode implicates draft-status staleness, and doing so would need its own resolved
+  // field for no demonstrated benefit.
+  const incomingGithubUpdatedAt = pr.updated_at ?? null;
+  const isStalePayload =
+    incomingGithubUpdatedAt !== null &&
+    existingClaimRow?.githubUpdatedAt != null &&
+    incomingGithubUpdatedAt < existingClaimRow.githubUpdatedAt;
+  const resolvedState = isStalePayload ? existingClaimRow!.state : pr.state;
+  const resolvedHeadSha = isStalePayload ? (existingClaimRow!.headSha ?? undefined) : pr.head?.sha;
+  const resolvedMergedAt = isStalePayload ? (existingClaimRow!.mergedAt ?? undefined) : (pr.merged_at ?? undefined);
+  // No `?? undefined` fallback on the stale branch (unlike headSha/mergedAt above): isStalePayload's own
+  // definition already requires existingClaimRow.githubUpdatedAt to be non-null, so that branch is unreachable.
+  const resolvedGithubUpdatedAt = isStalePayload ? existingClaimRow!.githubUpdatedAt : (incomingGithubUpdatedAt ?? undefined);
+  const lastSeenOpenAt = resolvedState === "open" ? (options.seenOpenAt ?? syncedAt) : null;
   const preserveSparseBody = pr.body === undefined && existingClaimRow !== undefined;
   const existingPayload = preserveSparseBody ? parseJson<{ body?: string | null }>(existingClaimRow.payloadJson, {}) : undefined;
   const existingBody = existingPayload?.body ?? null;
@@ -396,11 +425,12 @@ export async function upsertPullRequestFromGitHub(
   // headSha change (including the PR's first-ever sync) always restarts the clock; an unchanged headSha keeps
   // whatever was already stored (including null, which self-heals the instant the PR leaves draft or gets a
   // fresh commit -- no backfill migration needed, mirrors bodyObservedAt's own non-backfill philosophy).
-  const isReadyForReview = pr.state === "open" && !(pr.draft ?? pr.isDraft ?? false);
-  const incomingHeadSha = pr.head?.sha;
-  const headShaChanged = incomingHeadSha !== undefined && incomingHeadSha !== existingClaimRow?.headSha;
+  // Reads resolvedState/resolvedHeadSha (not pr.state/pr.head?.sha directly) so a stale, rejected payload can't
+  // still reset this clock out from under the out-of-order webhook guard above (#webhook-reorder-clobber).
+  const isReadyForReview = resolvedState === "open" && !(pr.draft ?? pr.isDraft ?? false);
+  const headShaChanged = resolvedHeadSha !== undefined && resolvedHeadSha !== existingClaimRow?.headSha;
   const headShaObservedAt =
-    !isReadyForReview || incomingHeadSha === undefined
+    !isReadyForReview || resolvedHeadSha === undefined
       ? (existingClaimRow?.headShaObservedAt ?? null)
       : headShaChanged || !existingClaimRow?.headShaObservedAt
         ? syncedAt
@@ -412,13 +442,13 @@ export async function upsertPullRequestFromGitHub(
       repoFullName,
       number: pr.number,
       title: pr.title,
-      state: pr.state,
+      state: resolvedState,
       authorLogin: pr.user?.login,
       authorAssociation: pr.author_association,
-      headSha: pr.head?.sha,
+      headSha: resolvedHeadSha,
       headRef: pr.head?.ref,
       baseRef: pr.base?.ref,
-      mergedAt: pr.merged_at ?? undefined,
+      mergedAt: resolvedMergedAt,
       htmlUrl: pr.html_url,
       labelsJson: jsonString(record.labels),
       linkedIssuesJson,
@@ -427,6 +457,7 @@ export async function upsertPullRequestFromGitHub(
       headShaObservedAt,
       lastSeenOpenAt,
       payloadJson: jsonString(payload),
+      githubUpdatedAt: resolvedGithubUpdatedAt,
       // GitHub's own PR creation time (see PullRequestRecord.createdAt's doc comment, src/types.ts) --
       // set ONLY here, on first insert, and deliberately absent from onConflictDoUpdate's `set` below so
       // a resync never overwrites it. `?? undefined` falls through to the column's own $defaultFn when a
@@ -438,13 +469,13 @@ export async function upsertPullRequestFromGitHub(
       target: [pullRequests.repoFullName, pullRequests.number],
       set: {
         title: pr.title,
-        state: pr.state,
+        state: resolvedState,
         authorLogin: pr.user?.login,
         authorAssociation: pr.author_association,
-        headSha: pr.head?.sha,
+        headSha: resolvedHeadSha,
         headRef: pr.head?.ref,
         baseRef: pr.base?.ref,
-        mergedAt: pr.merged_at ?? undefined,
+        mergedAt: resolvedMergedAt,
         htmlUrl: pr.html_url,
         labelsJson: jsonString(record.labels),
         linkedIssuesJson,
@@ -453,10 +484,11 @@ export async function upsertPullRequestFromGitHub(
         headShaObservedAt,
         lastSeenOpenAt,
         payloadJson: jsonString(payload),
+        githubUpdatedAt: resolvedGithubUpdatedAt,
         updatedAt: syncedAt,
       },
     });
-  return { ...record, body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt };
+  return { ...record, state: resolvedState, headSha: resolvedHeadSha, mergedAt: resolvedMergedAt ?? null, body, linkedIssues, linkedIssueClaimedAt, bodyObservedAt, headShaObservedAt };
 }
 
 function resolveLinkedIssueClaimedAt(
@@ -5841,6 +5873,25 @@ export async function terminalizeActiveReviewTracking(
   return Number(result.meta.changes ?? 0) > 0;
 }
 
+/** Rows still `active` and older than `olderThanIso` -- candidates for runActiveReviewReconciliation
+ *  (src/review/active-review-reconciliation.ts) to verify against LIVE GitHub state before terminalizing. Age
+ *  alone is never sufficient to terminalize (a genuinely slow review must not be force-closed); the caller
+ *  confirms each row's PR is actually closed before acting. No index on (status, startedAt) -- a full scan is
+ *  fine at this table's current scale (low thousands of rows); worth an index if that changes materially. */
+export async function listStaleActiveReviewTracking(
+  env: Env,
+  olderThanIso: string,
+): Promise<Array<{ repoFullName: string; pullNumber: number; startedAt: string }>> {
+  return getDb(env.DB)
+    .select({
+      repoFullName: activeReviewTracking.repoFullName,
+      pullNumber: activeReviewTracking.pullNumber,
+      startedAt: activeReviewTracking.startedAt,
+    })
+    .from(activeReviewTracking)
+    .where(and(eq(activeReviewTracking.status, "active"), lt(activeReviewTracking.startedAt, olderThanIso)));
+}
+
 // Review memory (#2178, data-model slice of #1964). Hard per-repo cap on stored suppression signals — mirrors
 // rag.ts's MAX_CHUNKS_PER_REPO discipline (bound a repo-controlled, unboundedly-growable store). A repo that
 // keeps dismissing NEW finding shapes evicts its OLDEST suppression first rather than growing forever.