Skip to content

Commit 996fa6e

Browse files
authored
fix(review): trim LOOPOVER_PUBLIC_STATS before the truthy-flag regex test (#10344)
isPublicStatsEnabled tested the raw, untrimmed env value against the anchored /^(1|true|yes|on)$/i regex, while every sibling flag-checker in the codebase (e.g. pr-reconciliation.ts's isPrReconciliationEnabled) trims first. A trailing newline or surrounding whitespace -- plausible from wrangler secret put reading a file, or a CI/CD-injected env var carrying a trailing newline -- made the anchored regex fail to match even though the operator clearly meant to enable the flag: the test of 'true\n' was false. Trim the value before the regex, matching the established convention. The manifestOverride early-return and every already-clean value are unchanged. Closes #10329
1 parent 5b95330 commit 996fa6e

2 files changed

Lines changed: 13 additions & 1 deletion

File tree

src/review/public-stats.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,10 @@ export function isPublicStatsEnabled(
7979
manifestOverride?: PublicStatsManifestOverride | undefined,
8080
): boolean {
8181
if (manifestOverride?.present) return manifestOverride.enabled;
82-
return /^(1|true|yes|on)$/i.test(env.LOOPOVER_PUBLIC_STATS ?? "");
82+
// #10329: trim before the anchored regex, matching every sibling flag-checker (e.g. pr-reconciliation.ts).
83+
// A trailing newline/space (plausible from `wrangler secret put` reading a file, or a CI-injected var) makes
84+
// `/^(1|true|yes|on)$/i.test("true\n")` false even though the operator clearly meant to enable the flag.
85+
return /^(1|true|yes|on)$/i.test((env.LOOPOVER_PUBLIC_STATS ?? "").trim());
8386
}
8487

8588
// Short in-isolate TTL cache for resolvePublicStatsManifestOverride, mirroring review-memory-wire.ts's

test/unit/public-stats.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,15 @@ describe("isPublicStatsEnabled", () => {
7676
expect(isPublicStatsEnabled({ LOOPOVER_PUBLIC_STATS: v })).toBe(false);
7777
});
7878

79+
it("#10329: trims the flag before the anchored regex, matching pr-reconciliation.ts", () => {
80+
// A trailing newline / surrounding whitespace (wrangler secret from a file, a CI-injected var) must not
81+
// defeat the operator's clear intent to enable it. Also confirms a genuinely unrecognised value stays off.
82+
for (const on of ["true\n", " 1 ", "\ton\t", " yes"])
83+
expect(isPublicStatsEnabled({ LOOPOVER_PUBLIC_STATS: on }), on).toBe(true);
84+
for (const off of [" false ", "\n0\n", " maybe "])
85+
expect(isPublicStatsEnabled({ LOOPOVER_PUBLIC_STATS: off }), off).toBe(false);
86+
});
87+
7988
it("a present manifest override wins outright over the env flag, in both directions (#6275)", () => {
8089
expect(isPublicStatsEnabled({ LOOPOVER_PUBLIC_STATS: "false" }, { present: true, enabled: true })).toBe(true);
8190
expect(isPublicStatsEnabled({ LOOPOVER_PUBLIC_STATS: "true" }, { present: true, enabled: false })).toBe(false);

0 commit comments

Comments
 (0)