From c36afbb238d96e4e112aef6215e501ebb41f6dcd Mon Sep 17 00:00:00 2001
From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com>
Date: Wed, 15 Jul 2026 04:34:22 +0800
Subject: [PATCH] fix(review): treat an empty consent phrase as unconfigured,
 not auto-satisfied

Closes #5838

evaluateClaCheck (src/review/cla-check.ts) computed
`phraseSatisfied = config.consentPhrase !== null && body.includes(config.consentPhrase)`.
When consentPhrase is the empty string "" (distinct from null; claConsentPhrase is a
dashboard/API-settable `z.string().nullable()` field), `"" !== null` is true and any
`body.includes("")` is unconditionally true, so phraseSatisfied was always true -- silently
satisfying CLA consent for every PR, even one whose configured CLA check-run was failing.

Fix: normalize an empty consentPhrase to null at the top of the function, so it is treated as
"phrase detection not configured" (the same as null) -- it never satisfies consent, and never
emits a nonsensical `the PR description must contain ""` requirement. This matches the field's
documented contract ("null => phrase-match detection is not configured").

Regression tests: an empty phrase alongside a failing check-run now correctly hard-fails
(cla_consent_missing) listing only the check-run; an empty phrase with no other method is
"nothing configured" and yields no finding. cla-check.ts stays 100% branch-covered.
---
 src/review/cla-check.ts     | 10 +++++++---
 test/unit/cla-check.test.ts | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/review/cla-check.ts b/src/review/cla-check.ts
index 63902c1a23..e008a6a3a8 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());
+  // An empty-string consentPhrase is not a usable detection method: a case-insensitive `.includes("")` matches
+  // EVERY body, which would silently satisfy CLA consent for every PR. Treat it as "phrase detection not
+  // configured" (same as null) — so it never satisfies consent, and never emits a nonsensical `must contain ""`.
+  const consentPhrase = config.consentPhrase === "" ? null : config.consentPhrase;
+  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..88dfd59122 100644
--- a/test/unit/cla-check.test.ts
+++ b/test/unit/cla-check.test.ts
@@ -113,4 +113,24 @@ describe("evaluateClaCheck (#2564)", () => {
       expect(out[0]?.code).toBe(CLA_CHECK_UNRESOLVED_CODE);
     });
   });
+
+  // Regression: an empty-string consentPhrase (distinct from null) must NOT unconditionally satisfy consent —
+  // `"".includes("")`/any `body.includes("")` is always true, which previously bypassed CLA for every PR. An
+  // empty phrase is treated as "phrase detection not configured" (same as null).
+  describe("empty-string consentPhrase is treated as not configured (regression)", () => {
+    it("does NOT satisfy consent: an empty phrase alongside a failing check-run still fails, and never lists a nonsensical empty phrase", () => {
+      const out = evaluateClaCheck(config({ consentPhrase: "", checkRunName: "CLA Assistant Lite" }), {
+        body: "any body at all",
+        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('must contain ""');
+    });
+
+    it("with no other method configured, an empty phrase is 'nothing configured' → no finding (not a silent bypass, not a nonsensical failure)", () => {
+      expect(evaluateClaCheck(config({ consentPhrase: "" }), { body: "anything" })).toEqual([]);
+    });
+  });
 });