From 56ed10451a3a52846486b22cbcbd3e8eed5362c6 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 05:29:27 -0700 Subject: [PATCH] fix(review): default reviewEvasionProtection to close, not off (#4011) A repo that never discovers and explicitly sets reviewEvasionProtection got ZERO self-close/draft-dodge/repeated-cycling protection under the old "off" default -- a real, already-exploited gaming vector (see the draft-cycling incident this session root-caused and fixed for our own 3 repos via explicit private-config overrides). The fix is NOT a migration changing the schema.ts column default, which would have zero effect: upsertRepositorySettings is the only writer and always resolves an explicit value through normalizeReviewEvasionProtection, so the raw SQLite column-level default is never reached by any live write path (the identical lesson migration 0102 already documented for linked_issue_gate_mode -- SQLite has no ALTER COLUMN SET DEFAULT, and rebuilding the table for a default nothing reads isn't worth the risk). The actually-reachable default lives in three places, all flipped together for consistency: - normalizeReviewEvasionProtection (src/db/repositories.ts): the shared read+write normalizer both getRepositorySettings and upsertRepositorySettings resolve through. Only the explicit opt-out "off" is now honored; anything else (including undefined/garbage) resolves to "close". - getRepositorySettings' own "no row exists yet" fallback object, used before a repo's first-ever settings write. - Three independent `?? "off"` fallbacks in src/queue/processors.ts (maybeCloseReviewEvasionSelfClose, maybeCloseReviewEvasionDraftConversion, the repeated-draft-cycling guard) that defended against an undefined settings value with the SAME old default, entirely independent of the DB layer -- fixing only the DB side would have left these three still silently unprotected against a genuinely undefined value. Left the schema.ts column-level DEFAULT untouched (with a comment explaining why, linking to migration 0102's precedent) rather than risk a full table-rebuild migration for a value no live path reads. --- src/db/repositories.ts | 15 +++++++++++++-- src/db/schema.ts | 14 +++++++++++--- src/queue/processors.ts | 8 ++++---- test/unit/moderation-config-db.test.ts | 14 +++++++++++--- test/unit/queue.test.ts | 26 ++++++++++++++++---------- 5 files changed, 55 insertions(+), 22 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index cfc7131b83..879427c226 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -564,7 +564,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise moderationRules: undefined, moderationWarningLabel: undefined, moderationBannedLabel: undefined, - reviewEvasionProtection: "off", + reviewEvasionProtection: "close", // #4011: default-ON -- see normalizeReviewEvasionProtection's doc comment reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL, reviewEvasionComment: true, screenshotTableGate: { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }, @@ -7055,8 +7055,19 @@ function normalizeReviewNagPolicy(value: string | null | undefined): "off" | "ho // Review-evasion protection (#review-evasion-protection): binary off|close, mirroring reviewNagPolicy's // shape minus the "hold" tier (an evasion attempt is always re-closed as the App when enabled, never merely // held -- there is no partial-enforcement mode). +// +// #4011: default-ON, the deliberate exception to every other field in this file defaulting conservatively +// (off/false/advisory). A repo that hasn't discovered and explicitly set this field got ZERO self-close/ +// draft-dodge/repeated-cycling protection under the old "off" default -- a real, already-exploited gaming +// vector (see gittensory-ai-review-repeat-spend-and-draft-gaming-fix). Any value other than the explicit +// opt-out "off" (including undefined/garbage) now resolves to "close": protected unless a repo deliberately +// turns it off, not unprotected unless a repo discovers and turns it on. This is the ONLY reachable default +// for this field -- the raw schema.ts column-level DEFAULT and the SQLite DDL default are never reached by +// any live write path (upsertRepositorySettings always resolves and supplies an explicit value through this +// exact function; see migration 0102's doc comment for the same lesson learned on a sibling field), so +// changing them would have zero effect and was deliberately left alone. function normalizeReviewEvasionProtection(value: string | null | undefined): "off" | "close" { - return value === "close" ? "close" : "off"; + return value === "off" ? "off" : "close"; } // Config-driven before/after screenshot-table gate (#2006): the row stores whenLabels/whenPaths as JSON string diff --git a/src/db/schema.ts b/src/db/schema.ts index cfd2adda9c..75ad404a79 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -151,9 +151,17 @@ export const repositorySettings = sqliteTable("repository_settings", { moderationRulesJson: text("moderation_rules_json"), moderationWarningLabel: text("moderation_warning_label"), moderationBannedLabel: text("moderation_banned_label"), - // Review-evasion protection (#review-evasion-protection): off by default. reviewEvasionLabel mirrors - // blacklistLabel/reviewNagLabel's shape -- NOT NULL with a string default; "no label" is a - // `.gittensory.yml`-only override, never persisted here. + // Review-evasion protection (#review-evasion-protection). reviewEvasionLabel mirrors blacklistLabel/ + // reviewNagLabel's shape -- NOT NULL with a string default; "no label" is a `.gittensory.yml`-only + // override, never persisted here. + // + // #4011: this raw column-level DEFAULT ('off') is intentionally left unchanged even though the actual + // resolved default is now 'close' (protection ON) -- upsertRepositorySettings is the ONLY writer and + // always resolves an explicit value through normalizeReviewEvasionProtection (its own doc comment has + // the full reasoning), so this raw default never fires through any live write path. Rebuilding it would + // need a full create-copy-drop-rename table migration for zero behavioral effect (SQLite has no ALTER + // COLUMN SET DEFAULT) -- see migration 0102's doc comment for the identical lesson already learned on + // linked_issue_gate_mode. Don't "fix" this value without re-reading that reasoning first. reviewEvasionProtection: text("review_evasion_protection").notNull().default("off"), reviewEvasionLabel: text("review_evasion_label").notNull().default("review-evasion"), reviewEvasionComment: integer("review_evasion_comment", { mode: "boolean" }).notNull().default(true), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0f378c2317..3ea7e55172 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5854,7 +5854,7 @@ async function processGitHubWebhook( // Review-evasion protection (#review-evasion-protection): a contributor closing their OWN PR while // gittensory has an ACTIVE review pass running is dodging the one-shot review, not making an ordinary // close. Runs regardless of the general draft-dodge/reopen-reclose gates above -- it is its own - // independent enforcement, config-gated on settings.reviewEvasionProtection (off by default). + // independent enforcement, config-gated on settings.reviewEvasionProtection (close by default, #4011). if (payload.action === "closed" && installationId) { await maybeCloseReviewEvasionSelfClose( env, @@ -11770,7 +11770,7 @@ async function closeReviewEvasionSelfCloseIfActive( payload: GitHubWebhookPayload, settings: RepositorySettings, ): Promise { - if ((settings.reviewEvasionProtection ?? "off") !== "close") return; + if (settings.reviewEvasionProtection === "off") return; // #4011: default-ON -- only the explicit opt-out bails const closer = (payload.sender?.login ?? "").toLowerCase(); const authorLogin = (pr.authorLogin ?? "").toLowerCase(); // Only the PR's OWN author closing their OWN PR is a self-close-evasion candidate -- a third party (e.g. a @@ -12016,7 +12016,7 @@ async function closeReviewEvasionDraftConversionIfActive( payload: GitHubWebhookPayload, settings: RepositorySettings, ): Promise { - if ((settings.reviewEvasionProtection ?? "off") !== "close") return; + if (settings.reviewEvasionProtection === "off") return; // #4011: default-ON -- only the explicit opt-out bails const converter = (payload.sender?.login ?? "").toLowerCase(); const authorLogin = (pr.authorLogin ?? "").toLowerCase(); // Only the PR's OWN author converting their OWN PR to draft is a draft-conversion-evasion candidate -- a @@ -12205,7 +12205,7 @@ async function maybeCloseRepeatedDraftCycling( // 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 (settings.reviewEvasionProtection === "off") return; // #4011: default-ON -- only the explicit opt-out bails if (draftConversionCount < 2) return; const actuationLock = await claimPrActuationLock(env, repoFullName, pr.number); if (!actuationLock.acquired) { diff --git a/test/unit/moderation-config-db.test.ts b/test/unit/moderation-config-db.test.ts index 7329f95c04..5eb6a9e18d 100644 --- a/test/unit/moderation-config-db.test.ts +++ b/test/unit/moderation-config-db.test.ts @@ -237,9 +237,9 @@ describe("per-repo moderation settings DB round-trip (#selfhost-mod-engine)", () }); describe("per-repo review-evasion protection settings DB round-trip (#review-evasion-protection)", () => { - it("defaults to off/review-evasion/true for an unconfigured repo", async () => { + it("defaults to close/review-evasion/true for an unconfigured repo (#4011: default-ON)", async () => { const settings = await getRepositorySettings(createTestEnv(), "owner/none"); - expect(settings.reviewEvasionProtection).toBe("off"); + expect(settings.reviewEvasionProtection).toBe("close"); expect(settings.reviewEvasionLabel).toBe("review-evasion"); expect(settings.reviewEvasionComment).toBe(true); }); @@ -274,11 +274,19 @@ describe("per-repo review-evasion protection settings DB round-trip (#review-eva expect(settings.reviewEvasionComment).toBe(false); }); - it("a malformed raw DB value for review_evasion_protection normalizes to 'off' on read (defensive against a direct SQL write bypassing app-level validation)", async () => { + it("a malformed raw DB value for review_evasion_protection normalizes to 'close' on read (#4011: only the explicit opt-out 'off' is honored; anything else, including a bypassed-validation bogus value, fails safe to protected)", async () => { const env = createTestEnv(); await upsertRepositorySettings(env, { repoFullName: "owner/repo" }); await env.DB.prepare("UPDATE repository_settings SET review_evasion_protection = 'bogus' WHERE repo_full_name = ?").bind("owner/repo").run(); const settings = await getRepositorySettings(env, "owner/repo"); + expect(settings.reviewEvasionProtection).toBe("close"); + }); + + it("the explicit opt-out 'off' is honored on read even after a direct SQL write (#4011)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "owner/repo" }); + await env.DB.prepare("UPDATE repository_settings SET review_evasion_protection = 'off' WHERE repo_full_name = ?").bind("owner/repo").run(); + const settings = await getRepositorySettings(env, "owner/repo"); expect(settings.reviewEvasionProtection).toBe("off"); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f0b76fb71d..0ac353c433 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -25616,7 +25616,7 @@ describe("review-evasion protection (#review-evasion-protection)", () => { expect(audit?.n).toBe(0); // no decision recorded either way -- the queue retry owns the deferred decision }); - it("does nothing when reviewEvasionProtection is off (the default)", async () => { + it("does nothing when reviewEvasionProtection is explicitly off (#4011: the only respected opt-out)", async () => { const calls: Array<{ url: string; method: string }> = []; stubEvasionFetch(calls); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); @@ -25969,10 +25969,12 @@ describe("review-evasion protection (#review-evasion-protection)", () => { expect(reopenAudit?.detail).toContain("one-shot"); }); - it("does nothing when reviewEvasionProtection is unset (undefined, not just 'off')", async () => { - // upsertRepositorySettings coalesces undefined -> "off" at write time (mirrors reviewEvasionLabel/ - // reviewEvasionComment's own write-time defaulting below), so the only way to get `undefined` past that - // coalescing and into the handler is to mock the resolved-settings layer directly. + it("STILL protects when reviewEvasionProtection is unset (undefined, not an explicit 'off') (#4011: default-ON)", async () => { + // upsertRepositorySettings coalesces undefined -> "close" at write time (mirrors reviewEvasionLabel/ + // reviewEvasionComment's own write-time defaulting below), and the consuming handler's own fallback + // (settings.reviewEvasionProtection === "off") treats anything but an explicit "off" as protected too -- + // so the only way to get `undefined` past BOTH layers and into the handler is to mock the resolved- + // settings layer directly, confirming neither layer silently reintroduces the old off-by-default gap. const calls: Array<{ url: string; method: string }> = []; stubEvasionFetch(calls); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); @@ -25983,7 +25985,8 @@ describe("review-evasion protection (#review-evasion-protection)", () => { await processJob(env, { type: "github-webhook", deliveryId: "self-close-protection-unset", eventName: "pull_request", payload: closedPayload("contributor") }); - expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const patches = calls.filter((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42")); + expect(patches.length).toBeGreaterThanOrEqual(2); // reopen then re-close, same as an explicit "close" }); it("does nothing when the webhook payload has no sender", async () => { @@ -26363,7 +26366,7 @@ describe("review-evasion protection (#review-evasion-protection)", () => { expect(strike?.n).toBe(0); }); - it("does nothing when reviewEvasionProtection is unset (undefined, not just 'off')", async () => { + it("STILL protects when reviewEvasionProtection is unset (undefined, not an explicit 'off') (#4011: default-ON)", async () => { const calls: Array<{ url: string; method: string }> = []; stubEvasionFetch(calls); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); @@ -26374,7 +26377,7 @@ describe("review-evasion protection (#review-evasion-protection)", () => { await processJob(env, { type: "github-webhook", deliveryId: "draft-evasion-protection-unset", eventName: "pull_request", payload: draftEvasionPayload("contributor") }); - expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); }); it("does nothing when the webhook payload has no sender", async () => { @@ -26567,7 +26570,7 @@ describe("review-evasion protection (#review-evasion-protection)", () => { expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); }); - it("does nothing when reviewEvasionProtection is unset (undefined, not just 'off'), even after a repeated cycle", async () => { + it("STILL enforces the repeated-cycle close when reviewEvasionProtection is unset (undefined, not an explicit 'off') (#4011: default-ON)", async () => { const calls: Array<{ url: string; method: string }> = []; stubEvasionFetch(calls); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); @@ -26576,9 +26579,12 @@ describe("review-evasion protection (#review-evasion-protection)", () => { vi.spyOn(repositorySettingsModule, "resolveRepositorySettings").mockResolvedValue({ ...baseSettings, reviewEvasionProtection: undefined }); await processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-unset-1", eventName: "pull_request", payload: draftEvasionPayload("contributor") }); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); // first conversion never closes + await processJob(env, { type: "github-webhook", deliveryId: "draft-cycle-unset-2", eventName: "pull_request", payload: draftEvasionPayload("contributor") }); - expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const patches = calls.filter((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42")); + expect(patches).toHaveLength(1); // second conversion closes, same as an explicit "close" }); it("REGRESSION (gate-flagged): does not enforce against a THIRD PARTY repeatedly converting someone else's PR to draft", async () => {