diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 305e1f5625..3bc8dc42a4 100644 --- a/src/queue/processors.ts +++ b/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 { + // 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 { - 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; diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 4d29df6257..b10a3ff189 100644 --- a/test/unit/queue.test.ts +++ b/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); });