fix(review): scope draft-cycle counting to the author's own conversions

src/queue/processors.ts

@@ -5818,16 +5818,25 @@ async function processGitHubWebhook(
           settings,
         );
       }
-      // Review-evasion protection: repeated ready<->draft cycling (#gaming-tactic-draft-cycle). Always counts
-      // the conversion (cheap, no side effects) so the count is accurate from the very first converted_to_draft
-      // event this repo ever sees, even before reviewEvasionProtection is turned on for it -- only the
-      // ENFORCEMENT below is gated on that setting. Runs after both guards above so a PR already closed by
-      // either of them fails this guard's own freshness re-check instead of being redundantly re-closed.
+      // Review-evasion protection: repeated ready<->draft cycling (#gaming-tactic-draft-cycle). Only counts a
+      // conversion PERFORMED BY THE PR'S OWN AUTHOR -- a maintainer/third-party converting the PR to draft is an
+      // unrelated action and must never contribute to (or be conflated with) the author's own cycling pattern;
+      // counting it here would let one maintainer draft-toggle plus the author's own first-ever (legitimate)
+      // conversion reach count>=2 and wrongly close that first conversion as "repeated" cycling. Always counts
+      // (cheap, no side effects) so the count is accurate from the very first converted_to_draft event this repo
+      // ever sees, even before reviewEvasionProtection is turned on for it -- only the ENFORCEMENT is gated on
+      // that setting. Runs after both guards above so a PR already closed by either of them fails this guard's
+      // own freshness re-check instead of being redundantly re-closed.
       if (payload.action === "converted_to_draft" && installationId) {
-        const draftConversionCount = await bumpPullRequestDraftConversionCount(env, repoFullName, pr.number).catch(
-          /* v8 ignore next -- fail-safe: a counter-write failure only means this ONE cycle isn't detected. */
-          () => 0,
-        );
+        const draftConverter = (payload.sender?.login ?? "").toLowerCase();
+        const draftAuthor = (pr.authorLogin ?? "").toLowerCase();
+        const isAuthorDraftConversion = draftConverter.length > 0 && draftConverter === draftAuthor;
+        const draftConversionCount = isAuthorDraftConversion
+          ? await bumpPullRequestDraftConversionCount(env, repoFullName, pr.number).catch(
+              /* v8 ignore next -- fail-safe: a counter-write failure only means this ONE cycle isn't detected. */
+              () => 0,
+            )
+          : 0;
         await maybeCloseRepeatedDraftCycling(
           env,
           deliveryId,
@@ -11890,6 +11899,12 @@ async function maybeCloseRepeatedDraftCycling(
   settings: RepositorySettings,
   draftConversionCount: number,
 ): Promise<void> {
+  // Checked BEFORE claiming the actuation lock (unlike the two sibling guards above): this guard only ever
+  // fires on the >=2nd author-driven conversion of a `reviewEvasionProtection: close` repo, which is rare -- an
+  // unconditional lock claim on every converted_to_draft webhook would add avoidable contention with the two
+  // siblings above on every repo that never enabled this feature, or on every first-time conversion.
+  if ((settings.reviewEvasionProtection ?? "off") !== "close") return;
+  if (draftConversionCount < 2) return;
   const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number);
   if (!actuationLock.acquired) {
     throw new PrActuationLockContendedError(repoFullName, pr.number, "review-evasion-draft-cycle");
@@ -11911,13 +11926,16 @@ async function closeRepeatedDraftCyclingIfDetected(
   settings: RepositorySettings,
   draftConversionCount: number,
 ): Promise<void> {
-  if ((settings.reviewEvasionProtection ?? "off") !== "close") return;
-  if (draftConversionCount < 2) return;
-  const converter = (payload.sender?.login ?? "").toLowerCase();
+  // Defense-in-depth (mirrors the two sibling guards' identical actor check): the call site already only ever
+  // increments draftConversionCount -- and therefore only ever reaches draftConversionCount >= 2 -- when THIS
+  // SAME payload's sender matches the PR's author (both non-empty), so neither `?? ""` fallback below can
+  // actually trigger and the guard below can never actually return here today. Kept anyway so a future call
+  // site added without that same pre-filter can't silently enforce against the wrong (or a blank) actor.
+  /* v8 ignore next -- unreachable given the call site's own author-only increment guarantee; see comment above. */
   const authorLogin = (pr.authorLogin ?? "").toLowerCase();
-  // Only the PR's OWN author converting their OWN PR to draft is a cycling-evasion candidate -- a third party
-  // (e.g. a maintainer converting a contributor's PR to draft) is an ordinary maintainer action, not evasion,
-  // and must never be enforced against the author who didn't do it (mirrors the two sibling guards above).
+  /* v8 ignore next -- unreachable given the call site's own author-only increment guarantee; see comment above. */
+  const converter = (payload.sender?.login ?? "").toLowerCase();
+  /* v8 ignore next -- unreachable given the call site's own author-only increment guarantee; see comment above. */
   if (!converter || !authorLogin || converter !== authorLogin) return;
   if (isProtectedAutomationAuthor(pr.authorLogin)) return;
   if (!pr.headSha) return;

test/unit/queue.test.ts

@@ -26000,6 +26000,32 @@ describe("review-evasion protection (#review-evasion-protection)", () => {
       expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false);
     });
 
+    it("REGRESSION (gate-flagged, gittensory-orb review): a maintainer's draft conversion must NOT count toward the author's own cycle -- the author's first-ever conversion is never enforced even after a prior third-party one", async () => {
+      const calls: Array<{ url: string; method: string }> = [];
+      stubEvasionFetch(calls, { collaboratorPermission: "write" });
+      const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" });
+      await setupEvasionRepo(env);
+      const maintainerConversion = draftEvasionPayload("contributor");
+      maintainerConversion.sender = { login: "a-maintainer", type: "User" };
+
+      // A maintainer converts the contributor's PR to draft first.
+      await processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-mixed-1", eventName: "pull_request", payload: maintainerConversion });
+      // Without the fix, this maintainer action would have already bumped the shared counter to 1.
+      const afterMaintainer = await env.DB.prepare("select draft_conversion_count as n from pull_requests where repo_full_name = ? and number = 42")
+        .bind("JSONbored/gittensory")
+        .first<{ n: number }>();
+      expect(afterMaintainer?.n).toBe(0); // the maintainer's own conversion never counted at all.
+
+      // The AUTHOR now converts their OWN PR to draft for the very first time -- ordinary WIP behavior.
+      await processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-mixed-2", eventName: "pull_request", payload: draftEvasionPayload("contributor") });
+
+      expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false);
+      const afterAuthor = await env.DB.prepare("select draft_conversion_count as n from pull_requests where repo_full_name = ? and number = 42")
+        .bind("JSONbored/gittensory")
+        .first<{ n: number }>();
+      expect(afterAuthor?.n).toBe(1); // the author's first conversion is counted as their first, not their second.
+    });
+
     it("does nothing for a protected automation author (e.g. dependabot[bot]), even after a repeated cycle", async () => {
       const calls: Array<{ url: string; method: string }> = [];
       stubEvasionFetch(calls);
@@ -26222,17 +26248,22 @@ describe("review-evasion protection (#review-evasion-protection)", () => {
       const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" });
       // Deliberately autonomy: {} -- draft-dodge's own outer dispatch condition (isAgentConfigured) is false, so
       // ITS lock claim never fires. The remaining sibling (review-evasion-active-review) has no settings gate at
-      // its OWN lock claim, so it claims+releases the lock normally first (mocked to succeed); THIS guard's own
-      // subsequent claim -- the second and only other .claim() call in this pass -- is the one mocked to fail,
-      // isolating its throw from the sibling's identically-shaped one.
+      // its OWN lock claim, so it claims+releases the lock normally on every converted_to_draft delivery. THIS
+      // guard now checks reviewEvasionProtection/count BEFORE claiming its own lock (#nit-lock-contention), so it
+      // never attempts a claim at all until draftConversionCount reaches 2 -- the first delivery below produces
+      // only the sibling's claim (mocked to succeed); the second produces the sibling's claim (succeeds) THEN
+      // this guard's own first-ever claim attempt, which is the one mocked to fail here.
       await setupEvasionRepo(env, { autonomy: {} });
-      const claimSpy = vi.spyOn(env.SELFHOST_TRANSIENT_CACHE!, "claim").mockResolvedValueOnce(true).mockResolvedValueOnce(false);
+      const claimSpy = vi.spyOn(env.SELFHOST_TRANSIENT_CACHE!, "claim").mockResolvedValueOnce(true).mockResolvedValueOnce(true).mockResolvedValueOnce(false);
+
+      await processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-lock-1", eventName: "pull_request", payload: draftEvasionPayload("contributor") });
+      expect(claimSpy).toHaveBeenCalledTimes(1); // count is only 1 -- this guard never attempted a claim yet.
 
       await expect(
-        processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-lock-1", eventName: "pull_request", payload: draftEvasionPayload("contributor") }),
+        processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-lock-2", eventName: "pull_request", payload: draftEvasionPayload("contributor") }),
       ).rejects.toThrow("during review-evasion-draft-cycle");
 
-      expect(claimSpy).toHaveBeenCalledTimes(2);
+      expect(claimSpy).toHaveBeenCalledTimes(3);
       expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false);
     });