From a7d867576b38c217a4558ea2e856f1fb066fe106 Mon Sep 17 00:00:00 2001 From: Lourince Daging Date: Tue, 14 Jul 2026 22:42:17 +0200 Subject: [PATCH] fix(review): treat a blank consentPhrase as unset in evaluateClaCheck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty-string consentPhrase (distinct from null/unset — reachable via the dashboard/API-settable claConsentPhrase, which has no non-empty validation) made phraseSatisfied unconditionally true, because "".includes("") is always true, silently satisfying CLA consent for every PR. Normalize a blank or whitespace-only consentPhrase to null before the phrase check, mirroring the config-as-code path's existing empty-string handling, so it behaves exactly as if unset. Applied to BOTH hand-duplicated twins (src/review/cla-check.ts and packages/loopover-engine/src/review/cla-check.ts) to keep engine-parity. Scoped to consentPhrase normalization; the escalation/severity contract and either-method-holds logic are untouched. Regression tests for empty and whitespace-only consentPhrase in both suites; full patch coverage. Closes #5838 --- .../loopover-engine/src/review/cla-check.ts | 10 +++++--- src/review/cla-check.ts | 10 +++++--- test/unit/cla-check.test.ts | 24 +++++++++++++++++++ .../predicted-gate-engine-coverage.test.ts | 6 +++++ 4 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/loopover-engine/src/review/cla-check.ts b/packages/loopover-engine/src/review/cla-check.ts index 362dd39ed2..6d5b753627 100644 --- a/packages/loopover-engine/src/review/cla-check.ts +++ b/packages/loopover-engine/src/review/cla-check.ts @@ -50,8 +50,12 @@ export function evaluateClaCheck( config: ClaCheckConfig, ctx: { body?: string | null | undefined; checkRunConclusion?: string | null | undefined }, ): AdvisoryFinding[] { - if (config.consentPhrase === null && config.checkRunName === null) return []; // nothing configured ⇒ no finding - const phraseSatisfied = config.consentPhrase !== null && (ctx.body ?? "").toLowerCase().includes(config.consentPhrase.toLowerCase()); + // A blank/whitespace-only consentPhrase is treated as unset (null), the same as the config-as-code path + // (focus-manifest normalizeOptionalString) already does — otherwise `"".includes("")` would make phrase + // detection unconditionally satisfied and silently bypass the CLA gate for every PR. + const consentPhrase = config.consentPhrase !== null && config.consentPhrase.trim() !== "" ? config.consentPhrase : null; + if (consentPhrase === null && config.checkRunName === null) return []; // nothing configured ⇒ no finding + const phraseSatisfied = consentPhrase !== null && (ctx.body ?? "").toLowerCase().includes(consentPhrase.toLowerCase()); const checkRunSatisfied = config.checkRunName !== null && (ctx.checkRunConclusion === "success" || ctx.checkRunConclusion === "neutral"); if (phraseSatisfied || checkRunSatisfied) return []; // A configured check-run whose conclusion is unresolved: cannot confirm OR deny consent via that method, so @@ -69,7 +73,7 @@ export function evaluateClaCheck( ]; } const missing: string[] = []; - if (config.consentPhrase !== null) missing.push(`the PR description must contain "${config.consentPhrase}"`); + if (consentPhrase !== null) missing.push(`the PR description must contain "${consentPhrase}"`); if (config.checkRunName !== null) missing.push(`the "${config.checkRunName}" check must pass`); return [ { diff --git a/src/review/cla-check.ts b/src/review/cla-check.ts index 63902c1a23..c1780d1f93 100644 --- a/src/review/cla-check.ts +++ b/src/review/cla-check.ts @@ -50,8 +50,12 @@ export function evaluateClaCheck( config: ClaCheckConfig, ctx: { body?: string | null | undefined; checkRunConclusion?: string | null | undefined }, ): AdvisoryFinding[] { - if (config.consentPhrase === null && config.checkRunName === null) return []; // nothing configured ⇒ no finding - const phraseSatisfied = config.consentPhrase !== null && (ctx.body ?? "").toLowerCase().includes(config.consentPhrase.toLowerCase()); + // A blank/whitespace-only consentPhrase is treated as unset (null), the same as the config-as-code path + // (focus-manifest normalizeOptionalString) already does — otherwise `"".includes("")` would make phrase + // detection unconditionally satisfied and silently bypass the CLA gate for every PR. + const consentPhrase = config.consentPhrase !== null && config.consentPhrase.trim() !== "" ? config.consentPhrase : null; + if (consentPhrase === null && config.checkRunName === null) return []; // nothing configured ⇒ no finding + const phraseSatisfied = consentPhrase !== null && (ctx.body ?? "").toLowerCase().includes(consentPhrase.toLowerCase()); const checkRunSatisfied = config.checkRunName !== null && (ctx.checkRunConclusion === "success" || ctx.checkRunConclusion === "neutral"); if (phraseSatisfied || checkRunSatisfied) return []; // A configured check-run whose conclusion is unresolved: cannot confirm OR deny consent via that method, so @@ -69,7 +73,7 @@ export function evaluateClaCheck( ]; } const missing: string[] = []; - if (config.consentPhrase !== null) missing.push(`the PR description must contain "${config.consentPhrase}"`); + if (consentPhrase !== null) missing.push(`the PR description must contain "${consentPhrase}"`); if (config.checkRunName !== null) missing.push(`the "${config.checkRunName}" check must pass`); return [ { diff --git a/test/unit/cla-check.test.ts b/test/unit/cla-check.test.ts index 92fa7fae07..019aa17748 100644 --- a/test/unit/cla-check.test.ts +++ b/test/unit/cla-check.test.ts @@ -113,4 +113,28 @@ describe("evaluateClaCheck (#2564)", () => { expect(out[0]?.code).toBe(CLA_CHECK_UNRESOLVED_CODE); }); }); + + // #5838: a blank/whitespace-only consentPhrase must be treated as unset, not as an always-matching "" that + // silently satisfies CLA consent for every PR (`"".includes("")` is unconditionally true). + describe("empty/whitespace-only consentPhrase normalization (#5838)", () => { + it("an empty-string consentPhrase does NOT unconditionally satisfy consent — it behaves as if unset", () => { + const out = evaluateClaCheck(config({ consentPhrase: "", checkRunName: "CLA Assistant Lite" }), { + body: "no consent statement here", + checkRunConclusion: "failure", + }); + expect(out).toHaveLength(1); + expect(out[0]?.code).toBe(CLA_CONSENT_MISSING_CODE); + expect(out[0]?.detail).toContain('the "CLA Assistant Lite" check must pass'); + expect(out[0]?.detail).not.toContain("PR description must contain"); + }); + + it("a whitespace-only consentPhrase with no other method configured yields no finding, exactly like null", () => { + expect(evaluateClaCheck(config({ consentPhrase: " " }), { body: "anything at all" })).toEqual([]); + }); + + it("REGRESSION: a real non-empty consentPhrase still decides consent (either-method contract unchanged)", () => { + expect(evaluateClaCheck(config({ consentPhrase: "I agree" }), { body: "... I AGREE ..." })).toEqual([]); + expect(evaluateClaCheck(config({ consentPhrase: "I agree" }), { body: "nope" })).toHaveLength(1); + }); + }); }); diff --git a/test/unit/predicted-gate-engine-coverage.test.ts b/test/unit/predicted-gate-engine-coverage.test.ts index f2aa5ad999..af873c754b 100644 --- a/test/unit/predicted-gate-engine-coverage.test.ts +++ b/test/unit/predicted-gate-engine-coverage.test.ts @@ -615,6 +615,12 @@ describe("predicted-gate engine module coverage (#2283)", () => { expect(evaluateClaCheck(claConfig({ consentPhrase: "agree", checkRunName: "CLA Assistant Lite" }), { body: "no", checkRunConclusion: "failure" })[0]?.code).toBe( CLA_CONSENT_MISSING_CODE, ); + // #5838: a blank/whitespace-only consentPhrase normalizes to unset, so it never unconditionally satisfies. + expect(evaluateClaCheck(claConfig({ consentPhrase: "", checkRunName: "CLA Assistant Lite" }), { body: "no", checkRunConclusion: "failure" })[0]?.code).toBe( + CLA_CONSENT_MISSING_CODE, + ); + expect(evaluateClaCheck(claConfig({ consentPhrase: " " }), { body: "anything" })).toEqual([]); + expect(evaluateClaCheck(claConfig({ consentPhrase: "agree" }), {})[0]?.code).toBe(CLA_CONSENT_MISSING_CODE); expect(evaluatePreMergeChecks([], { title: "t", body: "b", labels: [], changedPaths: [] })).toEqual([]); expect(