From a13d030e7da29f7d974c5417367e324d8477578c Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:45:54 +0900 Subject: [PATCH] fix(settings): read the two remaining LOOPOVER_* flags via the codebase truthy convention isDuplicateWinnerEnabledGlobally and isOpenPrFileCollisionEnabledGlobally compared their env flag against the exact string "true", while every other LOOPOVER_* flag reader uses the trimmed, case-insensitive /^(1|true|yes|on)$/i convention (e.g. selfTuneFlagOn). Both functions' JSDoc even claimed the exact-"true" form WAS that convention. So LOOPOVER_DUPLICATE_WINNER=1 -- the value the regex accepts first and env.d.ts publishes for a sibling flag -- read as OFF, as did a TRUE spelling or a .env value with trailing whitespace, with no warning: the flag simply behaved as unset. Route both through /^(1|true|yes|on)$/i.test((env.X ?? "").trim()), byte-identical in shape to repository-settings.ts's selfTuneFlagOn. Correct both JSDoc claims and the third restatement in advisory.ts to name the resolver instead of the raw comparison. This only widens the accepted truthy set; "true" still enables, unset/empty still disables, and resolveDuplicateWinnerEnabled / resolveOpenPrFileCollisionEnabled keep their exact inherit/off/enabled behaviour. Closes #10054 --- src/rules/advisory.ts | 3 ++- src/settings/duplicate-winner-mode.ts | 12 ++++++----- src/settings/open-pr-file-collision-mode.ts | 8 ++++--- test/unit/duplicate-winner-mode.test.ts | 21 ++++++++++++++----- test/unit/open-pr-file-collision-mode.test.ts | 14 ++++++++----- 5 files changed, 39 insertions(+), 19 deletions(-) diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index eb3049988e..9a165e5746 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -376,7 +376,8 @@ export function buildPullRequestAdvisory( /** Duplicate-winner adjudication (#dup-winner). When true AND this PR is the cluster winner (the lowest * open sibling number), the `duplicate_pr_risk` finding is suppressed so the winner is not gate-blocked / * closed as a duplicate. Default/false ⇒ every duplicate sibling keeps the finding (byte-identical). The - * caller sets this to `env.LOOPOVER_DUPLICATE_WINNER === "true"`. */ + * caller resolves this via `isDuplicateWinnerEnabledGlobally` (settings/duplicate-winner-mode.ts), which + * applies the codebase truthy convention rather than a raw string comparison. */ duplicateWinnerEnabled?: boolean; /** Author logins of the linked issues (one entry per resolved issue, may be null when unknown). Used to * surface a `self_authored_linked_issue` finding when the PR author also opened the linked issue. Absent diff --git a/src/settings/duplicate-winner-mode.ts b/src/settings/duplicate-winner-mode.ts index 164edf862a..a46e4f4712 100644 --- a/src/settings/duplicate-winner-mode.ts +++ b/src/settings/duplicate-winner-mode.ts @@ -1,11 +1,13 @@ export type DuplicateWinnerMode = "inherit" | "off" | "enabled"; -/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (exact `"true"` string, e.g. the - * raw checks this replaces) -- unlike `isSkipAutomationBotPullRequestsEnabledGlobally` (default ON, inverted - * truthy match), this flag is opt-in and default OFF: sparing a duplicate cluster's earliest claimant is a - * real behavior change to the close disposition, not a low-risk waste-elimination default. */ +/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (`/^(1|true|yes|on)$/i`, + * trimmed + case-insensitive, e.g. `selfTuneFlagOn`/`isReputationEnabled`) -- so `1`, `on`, `TRUE`, and a + * `.env` value carrying trailing whitespace all read as truthy, not silently as OFF. Unlike + * `isSkipAutomationBotPullRequestsEnabledGlobally` (default ON, inverted truthy match), this flag is opt-in + * and default OFF: sparing a duplicate cluster's earliest claimant is a real behavior change to the close + * disposition, not a low-risk waste-elimination default. */ export function isDuplicateWinnerEnabledGlobally(env: { LOOPOVER_DUPLICATE_WINNER?: string | undefined }): boolean { - return env.LOOPOVER_DUPLICATE_WINNER === "true"; + return /^(1|true|yes|on)$/i.test((env.LOOPOVER_DUPLICATE_WINNER ?? "").trim()); } /** Per-repo override resolved against the global default. Mirrors `resolveSkipAutomationBotPullRequests`'s diff --git a/src/settings/open-pr-file-collision-mode.ts b/src/settings/open-pr-file-collision-mode.ts index 6385b4cb2f..73fdcaa21c 100644 --- a/src/settings/open-pr-file-collision-mode.ts +++ b/src/settings/open-pr-file-collision-mode.ts @@ -1,11 +1,13 @@ 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 +/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (`/^(1|true|yes|on)$/i`, trimmed + * + case-insensitive, e.g. `selfTuneFlagOn`/`isReputationEnabled`) -- so `1`, `on`, `TRUE`, and a `.env` + * value carrying trailing whitespace all read as truthy, not silently as OFF. 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"; + return /^(1|true|yes|on)$/i.test((env.LOOPOVER_OPEN_PR_FILE_COLLISION ?? "").trim()); } /** Per-repo override resolved against the global default. Mirrors `resolveDuplicateWinnerEnabled`'s diff --git a/test/unit/duplicate-winner-mode.test.ts b/test/unit/duplicate-winner-mode.test.ts index 7bbdf65cb2..e9979f1fd2 100644 --- a/test/unit/duplicate-winner-mode.test.ts +++ b/test/unit/duplicate-winner-mode.test.ts @@ -8,15 +8,26 @@ describe("isDuplicateWinnerEnabledGlobally", () => { expect(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: "" })).toBe(false); }); - it("is ON only for the exact string \"true\"", () => { - expect(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: "true" })).toBe(true); + it("is ON for every value the codebase truthy convention accepts (#10054)", () => { + // Was `=== "true"` only, which silently read `1` / `on` / `TRUE` / a whitespace-padded `.env` value as + // OFF. It now mirrors selfTuneFlagOn's `/^(1|true|yes|on)$/i.test((X ?? "").trim())`, trimmed + i-flag. + for (const value of ["1", "true", "TRUE", "yes", "on", " true "]) { + expect(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: value }), value).toBe(true); + } }); - it("stays OFF for any other value, including truthy-looking ones", () => { - for (const value of ["1", "yes", "on", "True", "TRUE", " true "]) { - expect(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: value })).toBe(false); + it("stays OFF for a falsy or unrecognised value", () => { + for (const value of ["0", "false", "off", "no", "maybe"]) { + expect(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: value }), value).toBe(false); } }); + + it("the =1 form an operator naturally writes actually enables the feature through the resolver (#10054)", () => { + // The end-to-end regression: `1` is the value the convention regex accepts first and env.d.ts publishes + // for a sibling flag, but under `=== "true"` it read OFF -- so a repo on `inherit` stayed off even after + // the operator set it. The fix must carry all the way through resolveDuplicateWinnerEnabled. + expect(resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: "1" }), "inherit")).toBe(true); + }); }); describe("resolveDuplicateWinnerEnabled", () => { diff --git a/test/unit/open-pr-file-collision-mode.test.ts b/test/unit/open-pr-file-collision-mode.test.ts index f7b7297e0e..7b4463dd1e 100644 --- a/test/unit/open-pr-file-collision-mode.test.ts +++ b/test/unit/open-pr-file-collision-mode.test.ts @@ -8,13 +8,17 @@ describe("isOpenPrFileCollisionEnabledGlobally", () => { 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("is ON for every value the codebase truthy convention accepts (#10054)", () => { + // Was `=== "true"` only, which silently read `1` / `on` / `TRUE` / a whitespace-padded `.env` value as + // OFF. It now mirrors selfTuneFlagOn's `/^(1|true|yes|on)$/i.test((X ?? "").trim())`, trimmed + i-flag. + for (const value of ["1", "true", "TRUE", "yes", "on", " true "]) { + expect(isOpenPrFileCollisionEnabledGlobally({ LOOPOVER_OPEN_PR_FILE_COLLISION: value }), value).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); + it("stays OFF for a falsy or unrecognised value", () => { + for (const value of ["0", "false", "off", "no", "maybe"]) { + expect(isOpenPrFileCollisionEnabledGlobally({ LOOPOVER_OPEN_PR_FILE_COLLISION: value }), value).toBe(false); } }); });