From d3bf13b090baacb705d3f7893877872b576b1b35 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 00:14:48 -0700 Subject: [PATCH] fix(review): give LOOPOVER_OPEN_PR_FILE_COLLISION a per-repo config override The open-PR file-path collision annotation (preflight/queue-health enrichment showing which open PRs touch overlapping files -- display only, never a close/gate blocker) was gated entirely by a single fleet-wide LOOPOVER_OPEN_PR_FILE_COLLISION env var, with no per-repo override -- the same shape as the other three config-as-code gaps fixed in this series. Adds openPrFileCollisionMode ("inherit" | "off" | "enabled", mirroring duplicateWinnerMode/skipAutomationBotAuthors' shape) wired through .loopover.yml settings.openPrFileCollisionMode, and a resolveOpenPrFileCollisionEnabled helper (mirrors resolveDuplicateWinnerEnabled) that the one call site in processors.ts now goes through instead of reading the env var directly. "inherit" (the default) reproduces today's exact fleet-wide behavior; "off"/"enabled" let one repo opt out of (or into) the extra GitHub API calls this enrichment costs, regardless of the global default. --- .loopover.yml.example | 9 +++++ apps/loopover-ui/public/openapi.json | 8 ++++ config/examples/loopover.full.yml | 9 +++++ .../loopover-engine/src/focus-manifest.ts | 5 +++ .../src/types/manifest-deps-types.ts | 6 +++ src/openapi/schemas.ts | 1 + src/queue/processors.ts | 3 +- src/settings/open-pr-file-collision-mode.ts | 20 ++++++++++ src/types.ts | 7 ++++ test/unit/focus-manifest.test.ts | 16 ++++++++ test/unit/open-pr-file-collision-mode.test.ts | 40 +++++++++++++++++++ 11 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 src/settings/open-pr-file-collision-mode.ts create mode 100644 test/unit/open-pr-file-collision-mode.test.ts diff --git a/.loopover.yml.example b/.loopover.yml.example index 96c1c8d3e5..299b131d80 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -704,6 +704,15 @@ settings: # inherit | off | enabled. Default: inherit. # duplicateWinnerMode: inherit + # Open-PR file-path collision annotations (#2653): enriches preflight/queue-health + # output with which open PRs touch overlapping files -- display-only, never a + # close/gate blocker. Costs an extra GitHub API round-trip per open PR, so this is + # opt-in and off by default. "inherit" defers to the self-host operator's + # LOOPOVER_OPEN_PR_FILE_COLLISION env default (itself off by default); "off"/"enabled" + # override that default in either direction for this repo. + # inherit | off | enabled. Default: inherit. + # openPrFileCollisionMode: inherit + # Hard manual-review path guardrails are config-as-code only. Safe by default (#3943): whatever you list # here is ADDED to a fixed, built-in invariant set (CI workflows/scripts, deploy config, config-as-code # files, core engine-decision paths — see DEFAULT_HARD_GUARDRAIL_GLOBS in src/review/guardrail-config.ts) diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index d6830915af..a45d231414 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -9673,6 +9673,14 @@ "updatedAt": { "type": "string", "nullable": true + }, + "openPrFileCollisionMode": { + "type": "string", + "enum": [ + "inherit", + "off", + "enabled" + ] } }, "required": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index 5a45075927..2460f4a482 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -718,6 +718,15 @@ settings: # inherit | off | enabled. Default: inherit. # duplicateWinnerMode: inherit + # Open-PR file-path collision annotations (#2653): enriches preflight/queue-health + # output with which open PRs touch overlapping files -- display-only, never a + # close/gate blocker. Costs an extra GitHub API round-trip per open PR, so this is + # opt-in and off by default. "inherit" defers to the self-host operator's + # LOOPOVER_OPEN_PR_FILE_COLLISION env default (itself off by default); "off"/"enabled" + # override that default in either direction for this repo. + # inherit | off | enabled. Default: inherit. + # openPrFileCollisionMode: inherit + # Hard manual-review path guardrails are config-as-code only. Safe by default (#3943): whatever you list # here is ADDED to a fixed, built-in invariant set (CI workflows/scripts, deploy config, config-as-code # files, core engine-decision paths — see DEFAULT_HARD_GUARDRAIL_GLOBS in src/review/guardrail-config.ts) diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index f0d880391b..fd93cfdf4f 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -401,6 +401,7 @@ export type FocusManifestSettings = Partial< | "closeOwnerAuthors" | "skipAutomationBotAuthors" | "duplicateWinnerMode" + | "openPrFileCollisionMode" | "autoLabelEnabled" | "typeLabelsEnabled" | "badgeEnabled" @@ -2243,6 +2244,10 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) // default -- "inherit" defers to it, "off"/"enabled" override in either direction for this repo. const duplicateWinnerMode = normalizeOptionalEnum(r.duplicateWinnerMode, "settings.duplicateWinnerMode", ["inherit", "off", "enabled"] as const, warnings); if (duplicateWinnerMode !== null) out.duplicateWinnerMode = duplicateWinnerMode; + // Open-PR file-path collision annotations (#2653): per-repo override of the global LOOPOVER_OPEN_PR_FILE_COLLISION + // default -- "inherit" defers to it, "off"/"enabled" override in either direction for this repo. + const openPrFileCollisionMode = normalizeOptionalEnum(r.openPrFileCollisionMode, "settings.openPrFileCollisionMode", ["inherit", "off", "enabled"] as const, warnings); + if (openPrFileCollisionMode !== null) out.openPrFileCollisionMode = openPrFileCollisionMode; // Moderation-rules engine (#selfhost-mod-engine): per-repo override of the global moderation config. const moderationGateMode = normalizeOptionalEnum(r.moderationGateMode, "settings.moderationGateMode", ["inherit", "off", "enabled"] as const, warnings); if (moderationGateMode !== null) out.moderationGateMode = moderationGateMode; diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index 511bd27ac8..3e56c34571 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -354,6 +354,12 @@ export type RepositorySettings = { * default-OFF); `"off"`/`"enabled"` fully override the global default in either direction for this repo. No * DB column -- config-as-code only, set via `.loopover.yml settings.duplicateWinnerMode`. */ duplicateWinnerMode?: "inherit" | "off" | "enabled" | undefined; + /** Open-PR file-path collision annotations (#2653): enriches preflight/queue-health output with which open + * PRs touch overlapping files -- display-only, never a close/gate blocker. `"inherit"` defers to the + * `LOOPOVER_OPEN_PR_FILE_COLLISION` global env default (itself default-OFF); `"off"`/`"enabled"` fully + * override the global default in either direction for this repo. No DB column -- config-as-code only, set + * via `.loopover.yml settings.openPrFileCollisionMode`. */ + openPrFileCollisionMode?: "inherit" | "off" | "enabled" | undefined; autoLabelEnabled: boolean; gittensorLabel: string; createMissingLabel: boolean; diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index e4a7b1db06..6b8bed917f 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -848,6 +848,7 @@ export const RepositorySettingsSchema = z moderationBannedLabel: z.string().optional(), skipAutomationBotAuthors: z.enum(["inherit", "off", "enabled"]).optional(), duplicateWinnerMode: z.enum(["inherit", "off", "enabled"]).optional(), + openPrFileCollisionMode: z.enum(["inherit", "off", "enabled"]).optional(), reviewEvasionProtection: z .enum(["off", "close"]) .optional() diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cb786fd66e..d31b268364 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -315,6 +315,7 @@ import { } from "../signals/engine"; import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner"; import { isDuplicateWinnerEnabledGlobally, resolveDuplicateWinnerEnabled } from "../settings/duplicate-winner-mode"; +import { isOpenPrFileCollisionEnabledGlobally, resolveOpenPrFileCollisionEnabled } from "../settings/open-pr-file-collision-mode"; import { buildAiReviewDiff, buildSecretScanDiff, buildUnifiedReviewDiff, totalAddedLineCount } from "../review/review-diff"; // #4013 step 4 (prep): buildAiReviewDiff/buildSecretScanDiff moved to review-diff.ts (a natural existing // home -- both already wrapped buildUnifiedReviewDiff there) rather than staying here, since keeping them @@ -8380,7 +8381,7 @@ async function maybePublishPrPublicSurface( // Scoped to collision/preflight/queue-health inputs only — every OTHER use of repoPullRequests below (e.g. the // duplicate-winner adjudication, which is same-linked-issue-based, not path-based) keeps reading the un-enriched array. const collisionPullRequests = - env.LOOPOVER_OPEN_PR_FILE_COLLISION === "true" + resolveOpenPrFileCollisionEnabled(isOpenPrFileCollisionEnabledGlobally(env), settings.openPrFileCollisionMode) ? await enrichOpenPullRequestsWithChangedFiles(env, repoFullName, repoPullRequests) : repoPullRequests; collisions = buildCollisionReport( diff --git a/src/settings/open-pr-file-collision-mode.ts b/src/settings/open-pr-file-collision-mode.ts new file mode 100644 index 0000000000..6385b4cb2f --- /dev/null +++ b/src/settings/open-pr-file-collision-mode.ts @@ -0,0 +1,20 @@ +export type OpenPrFileCollisionMode = "inherit" | "off" | "enabled"; + +/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (exact `"true"` string) -- opt-in + * and default OFF: the enrichment call this gates costs an extra GitHub API round-trip per open PR + * (enrichOpenPullRequestsWithChangedFiles), so an operator should deliberately turn it on rather than pay + * that cost by default. */ +export function isOpenPrFileCollisionEnabledGlobally(env: { LOOPOVER_OPEN_PR_FILE_COLLISION?: string | undefined }): boolean { + return env.LOOPOVER_OPEN_PR_FILE_COLLISION === "true"; +} + +/** Per-repo override resolved against the global default. Mirrors `resolveDuplicateWinnerEnabled`'s + * inherit/off/enabled shape (settings/duplicate-winner-mode.ts) -- symmetric: "off" and "enabled" both fully + * override the global default in either direction, so a repo that wants the extra file-collision annotation + * cost isn't blocked by a globally-off default, and a repo that wants to opt OUT of the extra API calls can + * do so even when the fleet default is on. */ +export function resolveOpenPrFileCollisionEnabled(globalDefault: boolean, mode: OpenPrFileCollisionMode | null | undefined): boolean { + if (mode === "off") return false; + if (mode === "enabled") return true; + return globalDefault; +} diff --git a/src/types.ts b/src/types.ts index 4caeae0189..b08c29b622 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1202,6 +1202,13 @@ export type RepositorySettings = { * default in either direction for this repo. No DB column -- config-as-code only, set via `.loopover.yml * settings.duplicateWinnerMode`. */ duplicateWinnerMode?: "inherit" | "off" | "enabled" | undefined; + /** Open-PR file-path collision annotations (#2653): enriches preflight/queue-health output with which open + * PRs touch overlapping files -- display-only, never a close/gate blocker. Costs an extra GitHub API + * round-trip per open PR to fetch changed files, so this is opt-in and default-OFF, unlike most settings + * here. `"inherit"` (the default) defers to the `LOOPOVER_OPEN_PR_FILE_COLLISION` global env default + * (itself default-OFF); `"off"`/`"enabled"` fully override the global default in either direction for this + * repo. No DB column -- config-as-code only, set via `.loopover.yml settings.openPrFileCollisionMode`. */ + openPrFileCollisionMode?: "inherit" | "off" | "enabled" | undefined; /** Review-evasion protection (#review-evasion-protection): a contributor closing or converting their OWN * PR to draft while loopover has an ACTIVE review pass running against it is dodging the one-shot * review process. The effective default is `"close"` as of #4011 (see `normalizeReviewEvasionProtection` diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 73f0f32e2a..0bcfadccd4 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -310,6 +310,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { autoProjectMilestoneMatchBackend: "autoProjectMilestoneMatchBackend:", closeOwnerAuthors: "closeOwnerAuthors:", duplicateWinnerMode: "duplicateWinnerMode:", + openPrFileCollisionMode: "openPrFileCollisionMode:", autoLabelEnabled: "autoLabelEnabled:", typeLabelsEnabled: "typeLabelsEnabled:", badgeEnabled: "badgeEnabled:", @@ -2532,6 +2533,21 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(invalid.warnings.some((w) => /settings\.duplicateWinnerMode/.test(w))).toBe(true); }); + it("parses + resolves openPrFileCollisionMode from the settings: block, overlaying the DB (#2653)", () => { + const manifest = parseFocusManifest({ settings: { openPrFileCollisionMode: "enabled" } }); + expect(manifest.settings.openPrFileCollisionMode).toBe("enabled"); + // yml overlays (replaces) the DB-configured value. + const eff = resolveEffectiveSettings({ openPrFileCollisionMode: "off" } as unknown as RepositorySettings, manifest); + expect(eff.openPrFileCollisionMode).toBe("enabled"); + // Omitted in yml ⇒ the DB-configured value survives untouched. + const noOverride = resolveEffectiveSettings({ openPrFileCollisionMode: "off" } as unknown as RepositorySettings, parseFocusManifest({})); + expect(noOverride.openPrFileCollisionMode).toBe("off"); + // An invalid enum is dropped with a warning rather than silently coerced. + const invalid = parseFocusManifest({ settings: { openPrFileCollisionMode: "sometimes" as never } }); + expect(invalid.settings.openPrFileCollisionMode).toBeUndefined(); + expect(invalid.warnings.some((w) => /settings\.openPrFileCollisionMode/.test(w))).toBe(true); + }); + it("moderationRules accepts review_evasion alongside the original three rule types (#review-evasion-protection)", () => { const manifest = parseFocusManifest({ settings: { moderationRules: ["review_evasion", "not-a-rule" as never] } }); expect(manifest.settings.moderationRules).toEqual(["review_evasion"]); diff --git a/test/unit/open-pr-file-collision-mode.test.ts b/test/unit/open-pr-file-collision-mode.test.ts new file mode 100644 index 0000000000..f7b7297e0e --- /dev/null +++ b/test/unit/open-pr-file-collision-mode.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from "vitest"; +import { isOpenPrFileCollisionEnabledGlobally, resolveOpenPrFileCollisionEnabled } from "../../src/settings/open-pr-file-collision-mode"; + +describe("isOpenPrFileCollisionEnabledGlobally", () => { + it("defaults OFF when unset", () => { + expect(isOpenPrFileCollisionEnabledGlobally({})).toBe(false); + expect(isOpenPrFileCollisionEnabledGlobally({ LOOPOVER_OPEN_PR_FILE_COLLISION: undefined })).toBe(false); + expect(isOpenPrFileCollisionEnabledGlobally({ LOOPOVER_OPEN_PR_FILE_COLLISION: "" })).toBe(false); + }); + + it("is ON only for the exact string \"true\"", () => { + expect(isOpenPrFileCollisionEnabledGlobally({ LOOPOVER_OPEN_PR_FILE_COLLISION: "true" })).toBe(true); + }); + + it("stays OFF for any other value, including truthy-looking ones", () => { + for (const value of ["1", "yes", "on", "True", "TRUE", " true "]) { + expect(isOpenPrFileCollisionEnabledGlobally({ LOOPOVER_OPEN_PR_FILE_COLLISION: value })).toBe(false); + } + }); +}); + +describe("resolveOpenPrFileCollisionEnabled", () => { + it("inherit defers to the global default in both directions", () => { + expect(resolveOpenPrFileCollisionEnabled(true, "inherit")).toBe(true); + expect(resolveOpenPrFileCollisionEnabled(false, "inherit")).toBe(false); + }); + + it("null/undefined mode behaves the same as inherit", () => { + expect(resolveOpenPrFileCollisionEnabled(true, null)).toBe(true); + expect(resolveOpenPrFileCollisionEnabled(false, undefined)).toBe(false); + }); + + it("off fully overrides a globally-ON default", () => { + expect(resolveOpenPrFileCollisionEnabled(true, "off")).toBe(false); + }); + + it("enabled fully overrides a globally-OFF default (symmetric)", () => { + expect(resolveOpenPrFileCollisionEnabled(false, "enabled")).toBe(true); + }); +});