Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/loopover-engine/src/review/cla-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,15 @@
* finding into the advisory before the gate evaluates.
*/
export function evaluateClaCheck(
config: ClaCheckConfig,

Check warning on line 50 in packages/loopover-engine/src/review/cla-check.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Possible duplicate overlap

Items reference the same linked issue #5838.

Check notice on line 50 in packages/loopover-engine/src/review/cla-check.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Review queue is busy

This repo has a busy review queue in the local Gittensory cache.

Check warning on line 50 in packages/loopover-engine/src/review/cla-check.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Pull request duplicates other open work

This pull request overlaps a high-risk cluster of other open pull requests doing similar work.
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
Expand All @@ -69,7 +73,7 @@
];
}
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 [
{
Expand Down
10 changes: 7 additions & 3 deletions src/review/cla-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,15 @@
* finding into the advisory before the gate evaluates.
*/
export function evaluateClaCheck(
config: ClaCheckConfig,

Check warning on line 50 in src/review/cla-check.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Possible duplicate overlap

Items reference the same linked issue #5838.

Check notice on line 50 in src/review/cla-check.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Review queue is busy

This repo has a busy review queue in the local Gittensory cache.

Check warning on line 50 in src/review/cla-check.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Pull request duplicates other open work

This pull request overlaps a high-risk cluster of other open pull requests doing similar work.
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
Expand All @@ -69,7 +73,7 @@
];
}
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 [
{
Expand Down
24 changes: 24 additions & 0 deletions test/unit/cla-check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,31 @@
checkRunConclusion: undefined,
});
expect(out).toHaveLength(1);
expect(out[0]?.code).toBe(CLA_CHECK_UNRESOLVED_CODE);

Check warning on line 113 in test/unit/cla-check.test.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Possible duplicate overlap

Items reference the same linked issue #5838.

Check notice on line 113 in test/unit/cla-check.test.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Review queue is busy

This repo has a busy review queue in the local Gittensory cache.

Check warning on line 113 in test/unit/cla-check.test.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Pull request duplicates other open work

This pull request overlaps a high-risk cluster of other open pull requests doing similar work.
});
});

// #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);
});
});
});
6 changes: 6 additions & 0 deletions test/unit/predicted-gate-engine-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -612,9 +612,15 @@
expect(evaluateClaCheck(claConfig({ consentPhrase: "agree to the CLA" }), { body: "missing" })[0]?.code).toBe(CLA_CONSENT_MISSING_CODE);
expect(evaluateClaCheck(claConfig({ checkRunName: "CLA Assistant Lite" }), { checkRunConclusion: "success" })).toEqual([]);
expect(evaluateClaCheck(claConfig({ checkRunName: "CLA Assistant Lite" }), { checkRunConclusion: undefined })[0]?.code).toBe(CLA_CHECK_UNRESOLVED_CODE);
expect(evaluateClaCheck(claConfig({ consentPhrase: "agree", checkRunName: "CLA Assistant Lite" }), { body: "no", checkRunConclusion: "failure" })[0]?.code).toBe(

Check warning on line 615 in test/unit/predicted-gate-engine-coverage.test.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Possible duplicate overlap

Items reference the same linked issue #5838.

Check notice on line 615 in test/unit/predicted-gate-engine-coverage.test.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Review queue is busy

This repo has a busy review queue in the local Gittensory cache.

Check warning on line 615 in test/unit/predicted-gate-engine-coverage.test.ts

View check run for this annotation

Loopover ORB / LoopOver Context

Pull request duplicates other open work

This pull request overlaps a high-risk cluster of other open pull requests doing similar work.
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(
Expand Down
Loading