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(