diff --git a/packages/loopover-engine/src/review/cla-check.ts b/packages/loopover-engine/src/review/cla-check.ts index 362dd39ed2..c6013fdc78 100644 --- a/packages/loopover-engine/src/review/cla-check.ts +++ b/packages/loopover-engine/src/review/cla-check.ts @@ -50,8 +50,13 @@ 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), mirroring the config-as-code path's + // normalizeOptionalString (packages/loopover-engine/src/focus-manifest.ts): otherwise `"".includes("")` (or + // any body `.includes("")`) is unconditionally true, silently satisfying consent for every PR — the DB-backed + // dashboard `claConsentPhrase` field has no non-empty validation and reaches here via `?? null` unchanged (#5838). + const consentPhrase = config.consentPhrase !== null && config.consentPhrase.trim().length > 0 ? 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 +74,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..bdc8a66ae8 100644 --- a/src/review/cla-check.ts +++ b/src/review/cla-check.ts @@ -50,8 +50,13 @@ 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), mirroring the config-as-code path's + // normalizeOptionalString (packages/loopover-engine/src/focus-manifest.ts): otherwise `"".includes("")` (or + // any body `.includes("")`) is unconditionally true, silently satisfying consent for every PR — the DB-backed + // dashboard `claConsentPhrase` field has no non-empty validation and reaches here via `?? null` unchanged (#5838). + const consentPhrase = config.consentPhrase !== null && config.consentPhrase.trim().length > 0 ? 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 +74,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..6888550667 100644 --- a/test/unit/cla-check.test.ts +++ b/test/unit/cla-check.test.ts @@ -73,6 +73,33 @@ describe("evaluateClaCheck (#2564)", () => { }); }); + // #5838: a blank/whitespace-only consentPhrase must normalize to null (unset), matching the config-as-code + // path's normalizeOptionalString — otherwise `body.includes("")` is unconditionally true, silently satisfying + // CLA consent for every PR (a gate bypass reachable via a fat-fingered blank save of the dashboard field). + describe("blank consentPhrase normalizes to unset (regression for #5838)", () => { + it("an empty-string consentPhrase as the ONLY configured method → nothing configured, no finding (not auto-satisfied)", () => { + expect(evaluateClaCheck(config({ consentPhrase: "" }), { body: "no consent statement here" })).toEqual([]); + }); + + it("a whitespace-only consentPhrase as the ONLY configured method → nothing configured, no finding", () => { + expect(evaluateClaCheck(config({ consentPhrase: " " }), { body: "no consent statement here" })).toEqual([]); + }); + + it("an empty-string consentPhrase with a configured check-run behaves as if the phrase were null (unresolved → HOLD)", () => { + const out = evaluateClaCheck(config({ consentPhrase: "", checkRunName: "CLA Assistant Lite" }), { body: "no phrase here", checkRunConclusion: undefined }); + expect(out).toHaveLength(1); + expect(out[0]?.code).toBe(CLA_CHECK_UNRESOLVED_CODE); + }); + + it("an empty-string consentPhrase with a resolved-failing check-run → cla_consent_missing lists only the check, never the blank phrase", () => { + const out = evaluateClaCheck(config({ consentPhrase: "", checkRunName: "CLA Assistant Lite" }), { body: "no phrase 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("the PR description must contain"); + }); + }); + describe("either-method contract (both configured)", () => { it("phrase satisfied, check-run unresolved → satisfied (phrase alone decides; no hold)", () => { const out = evaluateClaCheck(config({ consentPhrase: "agree to the CLA", checkRunName: "CLA Assistant Lite" }), { diff --git a/test/unit/predicted-gate-engine-branch-coverage.test.ts b/test/unit/predicted-gate-engine-branch-coverage.test.ts index b6a860ed18..aadca603b7 100644 --- a/test/unit/predicted-gate-engine-branch-coverage.test.ts +++ b/test/unit/predicted-gate-engine-branch-coverage.test.ts @@ -118,6 +118,12 @@ describe("predicted-gate engine branch coverage (#2283)", () => { expect(evaluateClaCheck({ consentPhrase: "I agree", checkRunName: "CLA Bot" }, { body: "nope", checkRunConclusion: undefined })[0]?.code).toBe( CLA_CHECK_UNRESOLVED_CODE, ); + // #5838: a blank/whitespace-only consentPhrase normalizes to null (unset), so it never auto-satisfies consent. + expect(evaluateClaCheck({ consentPhrase: "", checkRunName: null }, { body: "no consent here" })).toEqual([]); + expect(evaluateClaCheck({ consentPhrase: " ", checkRunName: null }, { body: "no consent here" })).toEqual([]); + expect(evaluateClaCheck({ consentPhrase: "", checkRunName: "CLA Bot" }, { body: "nope", checkRunConclusion: undefined })[0]?.code).toBe( + CLA_CHECK_UNRESOLVED_CODE, + ); expect(guardrailPathMatches(["src/a.ts"], ["src/a.ts"])).toEqual([{ path: "src/a.ts", glob: "src/a.ts" }]); expect(guardrailPathMatches(["other.ts"], ["src/a.ts"])).toEqual([]); const pathological = "src/*-*-*-final.ts";