fix(review): treat a blank consentPhrase as unset in evaluateClaCheck (#5838)

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 [
     {

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 [
     {

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" }), {

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";