fix(review): close the contributor open-PR cap's ordering + TOCTOU race

src/queue/transient-locks.ts

@@ -130,3 +130,34 @@ export class PrActuationLockContendedError extends RetryableJobError {
     this.name = "PrActuationLockContendedError";
   }
 }
+
+// Per-(repo, author) contributor open-item-cap mutex (#7284-fix, TOCTOU race): every existing lock above
+// scopes to ONE PR; the cap-membership decision is inherently about the AUTHOR's whole open-PR set on this
+// repo, so a burst of sibling PRs from the same author needs ONE shared lock namespace keyed by author, not
+// per-PR — two siblings' cap-checks (or a cap-check racing a merge) must never both proceed against a stale
+// view of "how many of this author's PRs are currently open" at the same time. Same short-TTL, best-effort,
+// per-holder-token shape as claimPrActuationLock above; see this module's own header comment for why a
+// missing claim()/releaseIfValue() primitive fails OPEN rather than fake exclusivity.
+const CONTRIBUTOR_CAP_LOCK_TTL_SECONDS = 30;
+function contributorCapLockKey(repoFullName: string, authorLogin: string): string {
+  return `contributor-cap-lock:${repoFullName.toLowerCase()}:${authorLogin.toLowerCase()}`;
+}
+export async function claimContributorCapLock(
+  env: Env,
+  repoFullName: string,
+  authorLogin: string,
+): Promise<TransientLockClaim> {
+  return claimTransientLock(
+    env,
+    contributorCapLockKey(repoFullName, authorLogin),
+    CONTRIBUTOR_CAP_LOCK_TTL_SECONDS,
+  );
+}
+export async function releaseContributorCapLock(
+  env: Env,
+  repoFullName: string,
+  authorLogin: string,
+  ownerToken: string | null,
+): Promise<void> {
+  await releaseTransientLockIfOwner(env, contributorCapLockKey(repoFullName, authorLogin), ownerToken);
+}

src/services/agent-action-executor.ts

@@ -41,6 +41,7 @@ import {
 import { incr } from "../selfhost/metrics";
 import { shouldWaitForOlderSiblings } from "../review/merge-train";
 import { captureError } from "../selfhost/sentry";
+import { claimContributorCapLock, releaseContributorCapLock } from "../queue/transient-locks";
 
 // The agent actor name on every audit record — the App acts on the maintainer's behalf per their configured
 // autonomy (the config IS the authorization; there is no human commenter to authorize, unlike #824).
@@ -166,6 +167,17 @@ export type AgentActionExecutionContext = {
   // ?? the CONTRIBUTOR_CAP_CANCEL_CI_DEFAULT env var) before building the context — the executor itself has no
   // settings access, only whatever ctx carries, mirroring how agentPaused/agentDryRun are already threaded in.
   contributorCapCancelCi?: boolean | undefined;
+  // Pre-merge contributor-cap re-check (#7284-fix, TOCTOU race): a caller-supplied closure (closed over the
+  // repo's settings/token, resolved before this ctx was built — same "the executor has no settings access"
+  // shape as every other field here) the executor calls, under the per-(repo, author) mutex
+  // (claimContributorCapLock), immediately before actually executing a merge. Returns true when still safe to
+  // merge, false when a fresh check confirms the author is NOW over cap (a sibling opened/closed since this
+  // PR's own cap check earlier in its pipeline pass) — the executor records a "denied" outcome and skips the
+  // merge (same idiom as every other pre-condition denial in this loop) rather than duplicating close-planning
+  // logic inline; the next natural re-evaluation (webhook/sweep) plans a close off the current, accurate cap
+  // state. Absent when the caller has no cap configured for this repo — merge proceeds exactly as before this
+  // field existed, zero added cost.
+  contributorCapMergeRecheck?: (() => Promise<boolean>) | undefined;
   // Moderation-rules engine (#selfhost-mod-engine): the repo's PER-REPO override fields, resolved by the
   // CALLER from RepositorySettings before building the context (same "the executor has no settings access"
   // shape as contributorCapCancelCi above). Absent/undefined ⇒ inherit the global config's own defaults. The
@@ -532,6 +544,32 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
         }).catch(() => undefined);
       }
     }
+    // 8c) pre-merge contributor-cap re-check (#7284-fix, TOCTOU race): confirmed live -- a PR whose OWN earlier
+    // cap check passed can still merge after a sibling's later cap-close made the author over cap, because each
+    // PR's cap check runs independently with no shared lock. Re-verify right before the irreversible write,
+    // under the SAME per-author mutex a concurrent sibling's cap-close/wake also acquires, so the two can never
+    // both act on a stale view of the author's open-PR count. `ctx.contributorCapMergeRecheck` is only ever set
+    // by the caller when a cap is actually configured for this repo (the common case leaves it undefined) —
+    // zero added cost, byte-identical to before this field existed.
+    if (action.actionClass === "merge" && ctx.contributorCapMergeRecheck) {
+      const authorLogin = ctx.authorLogin ?? "";
+      const { acquired, ownerToken } = await claimContributorCapLock(env, ctx.repoFullName, authorLogin);
+      if (!acquired) {
+        // "denied", not a thrown error: the next natural re-evaluation (webhook/sweep) picks this PR back up
+        // with fresh state once the concurrent holder finishes deciding for this same author.
+        await audit("denied", `contributor cap lock contended for ${ctx.repoFullName} author ${authorLogin} — action not executed`);
+        continue;
+      }
+      try {
+        const stillUnderCap = await ctx.contributorCapMergeRecheck();
+        if (!stillUnderCap) {
+          await audit("denied", `contributor cap re-check confirmed ${authorLogin} is now over cap on ${ctx.repoFullName} — action not executed`);
+          continue;
+        }
+      } finally {
+        await releaseContributorCapLock(env, ctx.repoFullName, authorLogin, ownerToken);
+      }
+    }
     // 9) live — perform the real mutation, recording success or the error.
     try {
       const detailOverride = await performAction(env, ctx, action);

src/settings/agent-actions.ts

@@ -603,6 +603,46 @@ function contributorCapCloseMessage(authorLogin: string, openCount: number, cap:
   return `LoopOver closed this because @${authorLogin} has ${openCount} open ${itemNoun}, above ${scopeDescription} of ${cap}. Close or merge an existing one to open a new one. This is an automated maintenance action.`;
 }
 
+/**
+ * Plan JUST the per-contributor open-item cap short-circuit (#7284-fix, resource-waste ordering) — factored
+ * out of planAgentMaintenanceActions so a cheap, CI-independent caller (the PR-open webhook path in
+ * processors.ts) can plan the SAME close+label actions the full disposition plan would eventually produce,
+ * without needing gate/ciAggregate/blacklistEntry/etc — none of which this short-circuit ever reads. Pure,
+ * deterministic, zero-hallucination, identical output to what planAgentMaintenanceActions itself produces for
+ * the same contributorCapMatch input (that function calls this as its own first check, below).
+ */
+export function planContributorCapClose(input: {
+  autonomy: AutonomyPolicy | null | undefined;
+  authorIsOwner: boolean;
+  authorIsAdmin: boolean;
+  authorIsAutomationBot: boolean;
+  contributorCapMatch: AgentActionPlanInput["contributorCapMatch"];
+  contributorCapLabel: AgentActionPlanInput["contributorCapLabel"];
+  pr: { headSha?: string | null | undefined };
+}): PlannedAgentAction[] | null {
+  const capContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot;
+  if (input.contributorCapMatch?.matched !== true || !capContributor) return null;
+  const level = (actionClass: AgentActionClass) => resolveAutonomy(input.autonomy, actionClass);
+  const acting = (actionClass: AgentActionClass) => isActingAutonomyLevel(level(actionClass));
+  const approval = (actionClass: AgentActionClass) => autonomyRequiresApproval(level(actionClass));
+  const { authorLogin, openCount, cap, itemKind, scope } = input.contributorCapMatch;
+  const label = resolveNullableLabel(input.contributorCapLabel, DEFAULT_CONTRIBUTOR_CAP_LABEL);
+  const actions: PlannedAgentAction[] = [];
+  if (acting("close")) {
+    actions.push({
+      actionClass: "close",
+      requiresApproval: approval("close"),
+      reason: "over the per-contributor open-item cap",
+      closeReasons: ["over the per-contributor open-item cap"],
+      closeComment: sanitizePublicComment(contributorCapCloseMessage(authorLogin, openCount, cap, itemKind, scope)),
+      closeKind: "contributor_cap",
+      ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}),
+    });
+  }
+  if (acting("close") && label !== null) actions.push({ actionClass: "label", autonomyClass: "close", closeKind: "contributor_cap", requiresApproval: approval("close"), reason: "over the per-contributor open-item cap", label, labelOp: "add" });
+  return actions;
+}
+
 // The close comment for review-nag cooldown (#2463). DOES interpolate authorLogin/pingCount/maxPings — none of
 // that is private (the author's own login and their own public @loopover ping count are already public/
 // derivable from the PR thread itself), mirroring the contributor-cap close message's same reasoning.
@@ -735,31 +775,20 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   }
 
   // Per-contributor open-item cap (#2270): same zero-hallucination short-circuit shape as the blacklist above —
-  // fires ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only. Re-checks the owner/admin/bot exemption
-  // independently of the caller (defense-in-depth, matching the blacklist block's own redundant check above).
-  const capContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot;
-  if (input.contributorCapMatch?.matched === true && capContributor) {
-    const { authorLogin, openCount, cap, itemKind, scope } = input.contributorCapMatch;
-    // #label-scoping: same close-autonomy-gated, null-clearable shape as the blacklist label above.
-    const label = resolveNullableLabel(input.contributorCapLabel, DEFAULT_CONTRIBUTOR_CAP_LABEL);
-    // Close is pushed BEFORE its coupled label (#label-close-split-brain) — see the closeKind doc comment above.
-    if (acting("close")) {
-      actions.push({
-        actionClass: "close",
-        requiresApproval: approval("close"),
-        reason: "over the per-contributor open-item cap",
-        closeReasons: ["over the per-contributor open-item cap"],
-        closeComment: sanitizePublicComment(contributorCapCloseMessage(authorLogin, openCount, cap, itemKind, scope)),
-        closeKind: "contributor_cap",
-        // Pin like blacklist/review_nag/copycat/screenshot_table above (#2452): without this an auto_with_approval
-        // stage persists params.expectedHeadSha as undefined, so decidePendingAgentAction's isUnpinnedRatifyingAction
-        // guard unconditionally rejects the maintainer's accept before the close ever executes.
-        ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}),
-      });
-    }
-    if (acting("close") && label !== null) actions.push({ actionClass: "label", autonomyClass: "close", closeKind: "contributor_cap", requiresApproval: approval("close"), reason: "over the per-contributor open-item cap", label, labelOp: "add" });
-    return actions;
-  }
+  // fires ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only. Delegates to planContributorCapClose
+  // (#7284-fix) so the SAME short-circuit is also directly callable from a CI-independent caller (the PR-open
+  // webhook path in processors.ts) without needing this function's other inputs (gate/ciAggregate/etc, none of
+  // which this short-circuit reads).
+  const capClose = planContributorCapClose({
+    autonomy: input.autonomy,
+    authorIsOwner: input.authorIsOwner,
+    authorIsAdmin: input.authorIsAdmin,
+    authorIsAutomationBot: input.authorIsAutomationBot,
+    contributorCapMatch: input.contributorCapMatch,
+    contributorCapLabel: input.contributorCapLabel,
+    pr: input.pr,
+  });
+  if (capClose !== null) return capClose;
 
   // Review-nag cooldown (#2463): same zero-hallucination short-circuit shape as the blacklist above — fires
   // ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only. The webhook trigger has already decided "this

test/unit/agent-action-executor.test.ts

@@ -2081,3 +2081,78 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train
     expect(mergePullRequest).not.toHaveBeenCalled();
   });
 });
+
+describe("pre-merge contributor-cap re-check (#7284-fix, TOCTOU race)", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    vi.mocked(fetchPullRequestFreshness).mockImplementation(async (_env, args) => ({
+      status: "current",
+      liveHeadSha: args.expectedHeadSha ?? null,
+      liveState: "open",
+      liveLabels: [],
+    }));
+  });
+
+  it("absent recheck (no cap configured for this repo): merge proceeds exactly as before this field existed", async () => {
+    const env = createTestEnv({});
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99" }), [merge]);
+    expect(outcomes[0]?.outcome).toBe("completed");
+    expect(mergePullRequest).toHaveBeenCalled();
+  });
+
+  it("recheck confirms still under cap: merge proceeds", async () => {
+    const env = createTestEnv({});
+    const recheck = vi.fn(async () => true);
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]);
+    expect(recheck).toHaveBeenCalledTimes(1);
+    expect(outcomes[0]?.outcome).toBe("completed");
+    expect(mergePullRequest).toHaveBeenCalled();
+  });
+
+  it("REGRESSION (#7284-fix, reproduces the #7284-#7289 incident): recheck confirms the author is NOW over cap -- the merge is denied, not executed", async () => {
+    const env = createTestEnv({});
+    const recheck = vi.fn(async () => false);
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]);
+    expect(recheck).toHaveBeenCalledTimes(1);
+    expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "denied" });
+    expect(outcomes[0]?.detail).toMatch(/now over cap/);
+    expect(mergePullRequest).not.toHaveBeenCalled();
+  });
+
+  it("lock contended (another pass already holds the per-author claim): the merge is denied without ever calling the recheck", async () => {
+    const env = createTestEnv({});
+    // Pre-claim the exact same lock key a concurrent sibling's cap-close/wake would hold.
+    await env.SELFHOST_TRANSIENT_CACHE?.claim?.("contributor-cap-lock:owner/repo:farmer99", "someone-else-token", 30);
+    const recheck = vi.fn(async () => true);
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]);
+    expect(recheck).not.toHaveBeenCalled();
+    expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "denied" });
+    expect(outcomes[0]?.detail).toMatch(/lock contended/);
+    expect(mergePullRequest).not.toHaveBeenCalled();
+  });
+
+  it("releases the lock after a successful recheck, so a later merge for the SAME author can still acquire it", async () => {
+    const env = createTestEnv({});
+    const recheck = vi.fn(async () => true);
+    await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]);
+    // If the first call's lock leaked (never released), this second claim attempt would fail.
+    const secondClaim = await env.SELFHOST_TRANSIENT_CACHE?.claim?.("contributor-cap-lock:owner/repo:farmer99", "probe-token", 30);
+    expect(secondClaim).toBe(true);
+  });
+
+  it("releases the lock after a denied (now-over-cap) recheck too, not just the success path", async () => {
+    const env = createTestEnv({});
+    const recheck = vi.fn(async () => false);
+    await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]);
+    const secondClaim = await env.SELFHOST_TRANSIENT_CACHE?.claim?.("contributor-cap-lock:owner/repo:farmer99", "probe-token", 30);
+    expect(secondClaim).toBe(true);
+  });
+
+  it("no authorLogin on ctx: the lock key degrades to an empty-string author rather than throwing", async () => {
+    const env = createTestEnv({});
+    const recheck = vi.fn(async () => true);
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: undefined, contributorCapMergeRecheck: recheck }), [merge]);
+    expect(outcomes[0]?.outcome).toBe("completed");
+    expect(mergePullRequest).toHaveBeenCalled();
+  });
+});