Skip to content

fix(review): evaluateClaCheck treats an empty-string consentPhrase as unconditionally satisfied #5838

Description

@JSONbored

Context

evaluateClaCheck in src/review/cla-check.ts evaluates the .loopover.yml gate.claMode + gate.cla configuration against a PR. Consent is satisfied when either a configured PR-body phrase match or a configured CLA-bot check-run conclusion holds:

const phraseSatisfied = config.consentPhrase !== null && (ctx.body ?? "").toLowerCase().includes(config.consentPhrase.toLowerCase());

(line 54)

If config.consentPhrase is the empty string "" (distinct from null/unset), "" !== null is true, and "".toLowerCase().includes("") (or any body .includes("")) is unconditionally true — so phraseSatisfied is always true regardless of the PR body's actual content, silently satisfying CLA consent for every PR.

This is reachable in practice: claConsentPhrase is a dashboard/API-settable field defined in src/openapi/schemas.ts as z.string().nullable().optional() (line 686) with no non-empty-string validation, and it flows straight into evaluateClaCheck's config unmodified — src/queue/processors.ts:8554 passes { consentPhrase: settings.claConsentPhrase ?? null, ... } (the ?? null only substitutes for undefined/null, not ""). A maintainer who fat-fingers a blank save on this field (clearing the input without also flipping claMode off) silently disables the entire CLA gate for every subsequent PR, with no warning anywhere.

Note this is not an issue for .loopover.yml-sourced config-as-code: the equivalent manifest field goes through packages/loopover-engine/src/focus-manifest.ts's normalizeOptionalString/parsePublicSafeText, which already treats an empty/whitespace-only string as null (with a warning). The gap is specific to the DB-backed, dashboard-settable claConsentPhrase path.

test/unit/cla-check.test.ts has no test case for an empty-string consentPhrase.

Requirements

  • evaluateClaCheck must treat a blank or whitespace-only consentPhrase the same as null (not configured) — mirroring the .trim()-before-check idiom already used by sibling gate-config evaluators in the same directory (e.g. the pattern isGroundingEnabled/isFixHandoffEnabled use for their own string-config fields).
  • Fix must be scoped strictly to input validation/normalization of consentPhrase inside evaluateClaCheck (or at its call site in processors.ts, whichever is the more natural single point of truth — prefer fixing inside evaluateClaCheck itself since it's the shared, testable, side-effect-free evaluator) — do not touch the escalation/severity contract (CLA_CONSENT_MISSING_CODE/CLA_CHECK_UNRESOLVED_CODE, the "either method holds" logic, or isConfiguredGateBlocker's mode handling).
  • If consentPhrase normalizes to blank and checkRunName is also unset/blank, the function must return [] exactly as it does today for config.consentPhrase === null && config.checkRunName === null (no finding — nothing configured).
  • If consentPhrase normalizes to blank but checkRunName IS configured, the check-run-only path must behave exactly as if consentPhrase had been null from the start.

Deliverables

  • evaluateClaCheck (or its call site) normalizes an empty/whitespace-only consentPhrase to null before the phraseSatisfied check.
  • A regression test in test/unit/cla-check.test.ts asserting that consentPhrase: "" (and consentPhrase: " ") does NOT unconditionally satisfy consent — the check-run path (if configured) or the cla_consent_missing/no-op behavior takes over exactly as if consentPhrase were null.
  • A test asserting the existing non-empty consentPhrase behavior is unchanged (no regression to the documented "either method holds" contract).

Test Coverage Requirements

Aim for 99%+ Codecov patch coverage (100% including the new branch/invariant) on the touched lines in src/review/cla-check.ts. This is a fix for a real gate-bypass condition, so a dedicated regression test for the empty-string case is required, not just incidental line coverage.

Expected Outcome

Setting claConsentPhrase to an empty string via the dashboard/API (whether by a fat-finger blank save or an API client sending "") no longer silently satisfies CLA consent for every PR — it now behaves identically to leaving the field unset, matching the config-as-code (.loopover.yml) path's existing empty-string handling.

Links & Resources

  • src/review/cla-check.ts (evaluateClaCheck, phraseSatisfied at line 54, missing array at line 72)
  • src/openapi/schemas.ts (claConsentPhrase: z.string().nullable().optional(), line 686 — no non-empty validation)
  • src/queue/processors.ts (line 8554 — consentPhrase: settings.claConsentPhrase ?? null)
  • src/signals/focus-manifest.ts (line 501, gate.claConsentPhrase) and packages/loopover-engine/src/focus-manifest.ts (normalizeOptionalString, already treats blank as null for the config-as-code path — the precedent this fix should mirror)
  • test/unit/cla-check.test.ts (existing test suite)
  • Related: #2564 (original CLA / license-compatibility gate dimension)

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions