Skip to content

Commit 75bd272

Browse files
authored
feat(ci): enforce PR ancestry and description quality gates (lidge-jun#648)
* docs: spec PR quality gates for ancestry and descriptions Capture the approved design for rejecting main-based PRs into dev/dev2-go and empty/thin/malformed PR bodies in the enforcer. * docs: plan PR ancestry and description quality gates Break the approved spec into TDD tasks for pr-quality.cjs, enforcer wiring, harness coverage, and contributing docs. * feat(ci): add pure PR ancestry and description quality checks * test(ci): cover wrong_base plus bad_description together * feat(ci): enforce PR ancestry and description in target gate * fix(ci): strip WRONG BRANCH prefix when base is corrected * test(ci): cover PR ancestry and description enforcement paths * docs: document PR ancestry and description quality gates * fix(ci): address CodeRabbit findings on PR quality gates Treat removed new-form Version/OS headings as missing, soft-fail ancestry compares, tighten harness require/write allowlists, and clarify enforce-target is convention until branch protection. * fix(ci): address Codex findings on PR quality gates Reject untouched PR templates, require low ahead-of-main for ancestry, and checkpoint draft/title ownership before mutations.
1 parent f19a73c commit 75bd272

14 files changed

Lines changed: 1902 additions & 211 deletions

.github/scripts/enforce-pr-target.test.cjs

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ describe("enforce-pr-target workflow", () => {
1313
assert.match(workflow, /pull_request_target:/);
1414
assert.doesNotMatch(
1515
workflow,
16-
/actions\/checkout@/,
17-
"wrong-branch enforcer must not check out untrusted PR code",
16+
/ref:\s*\$\{\{\s*github\.event\.pull_request\.head/,
17+
"enforcer must not check out untrusted PR head code",
1818
);
1919
});
2020

@@ -43,4 +43,33 @@ describe("enforce-pr-target workflow", () => {
4343
assert.match(workflow, /readyConversionFailed/);
4444
assert.match(workflow, /Could not mark pull request ready for review/);
4545
});
46+
47+
it("listens for synchronize so rebase can clear ancestry failures", () => {
48+
assert.match(workflow, /synchronize/);
49+
});
50+
51+
it("checks out trusted default-branch scripts only (never PR head)", () => {
52+
assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/);
53+
assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.repository\.default_branch\s*\}\}/);
54+
assert.match(workflow, /sparse-checkout:\s*\.github\/scripts/);
55+
assert.match(workflow, /persist-credentials:\s*false/);
56+
assert.doesNotMatch(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/);
57+
});
58+
59+
it("loads pr-quality via require from the checked-out scripts", () => {
60+
assert.match(workflow, /pr-quality\.cjs/);
61+
assert.match(workflow, /collectPrQualityFailures/);
62+
});
63+
64+
it("strips stale WRONG BRANCH prefix on failure when base is corrected", () => {
65+
const failureBlock = workflow.match(
66+
/if \(failures\.length > 0\) \{([\s\S]*?)core\.setFailed\(/,
67+
);
68+
assert.ok(failureBlock, "workflow must have a failure path");
69+
const failurePath = failureBlock[1];
70+
assert.match(failurePath, /shouldStripTitlePrefix/);
71+
assert.match(failurePath, /!hasWrongBase/);
72+
assert.match(failurePath, /titlePrefixedByBot = false/);
73+
assert.match(failurePath, /pr\.title\.slice\(TITLE_PREFIX\.length\)/);
74+
});
4675
});

.github/scripts/issue-quality.cjs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -704,11 +704,11 @@ function validateIssue(issue) {
704704
} else if (
705705
!softPass &&
706706
isNewBugForm &&
707-
version !== null &&
708-
(isEmpty(version) || isRawPlaceholder(version))
707+
(version === null || isEmpty(version) || isRawPlaceholder(version))
709708
) {
710-
// New form requires Version. Legacy N/A / No response soft-pass stays
711-
// only for bodies without Client or integration.
709+
// New form requires Version (including when the heading was removed).
710+
// Legacy N/A / No response soft-pass stays only for bodies without
711+
// Client or integration.
712712
reasons.push("Version is missing.");
713713
guidance.push("Add your OpenCodex version so we can reproduce the environment.");
714714
}
@@ -719,8 +719,7 @@ function validateIssue(issue) {
719719
} else if (
720720
!softPass &&
721721
isNewBugForm &&
722-
os !== null &&
723-
(isEmpty(os) || isRawPlaceholder(os))
722+
(os === null || isEmpty(os) || isRawPlaceholder(os))
724723
) {
725724
reasons.push("Operating system is missing.");
726725
guidance.push("Add your OS name and version (for example Windows 11 24H2).");

.github/scripts/issue-quality.test.cjs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,46 @@ describe("validateIssue - bug", () => {
799799
assert.ok(result.reasons.some((r) => /Operating system/i.test(r)));
800800
});
801801

802+
it("rejects a new-form bug when the Version heading was removed", () => {
803+
const body = [
804+
"### Client or integration",
805+
"Codex CLI",
806+
"### Area",
807+
"CLI",
808+
"### Summary",
809+
"Proxy returns 502 when streaming is enabled on Windows.",
810+
"### Reproduction",
811+
"1. ocx start",
812+
"2. Send a streaming /v1/responses request",
813+
"### Operating system",
814+
"Windows 11",
815+
].join("\n");
816+
const result = validateIssue({ title: "Streaming 502", body, labels: ["bug"] });
817+
assert.equal(result.kind, "bug");
818+
assert.equal(result.valid, false);
819+
assert.ok(result.reasons.some((r) => /Version/i.test(r) && /missing/i.test(r)));
820+
});
821+
822+
it("rejects a new-form bug when the Operating system heading was removed", () => {
823+
const body = [
824+
"### Client or integration",
825+
"Codex CLI",
826+
"### Area",
827+
"CLI",
828+
"### Summary",
829+
"Proxy returns 502 when streaming is enabled on Windows.",
830+
"### Reproduction",
831+
"1. ocx start",
832+
"2. Send a streaming /v1/responses request",
833+
"### Version",
834+
"2.7.42",
835+
].join("\n");
836+
const result = validateIssue({ title: "Streaming 502", body, labels: ["bug"] });
837+
assert.equal(result.kind, "bug");
838+
assert.equal(result.valid, false);
839+
assert.ok(result.reasons.some((r) => /Operating system/i.test(r) && /missing/i.test(r)));
840+
});
841+
802842
it("rejects a new-form bug whose Reproduction is only a vague phrase", () => {
803843
const body = [
804844
"### Client or integration",

.github/scripts/pr-quality.cjs

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
"use strict";
2+
3+
const path = require("node:path");
4+
const {
5+
clean,
6+
isPlaceholderOnlyValue,
7+
hasSubstantialStructuredContent,
8+
} = require(path.join(__dirname, "issue-quality.cjs"));
9+
10+
const ANCESTRY_BEHIND_THRESHOLD = 20;
11+
/** Cap on ahead_by vs main so stale `dev` forks (many commits ahead of main) are not flagged. */
12+
const ANCESTRY_AHEAD_MAIN_MAX = 5;
13+
const MIN_SECTION_LEN = 40;
14+
const MIN_RICH_SECTIONS = 2;
15+
const UNSTRUCTURED_MIN_LEN = 120;
16+
const UNSTRUCTURED_MIN_BLOCKS = 2;
17+
18+
/**
19+
* Exact instruction / checklist lines from `.github/PULL_REQUEST_TEMPLATE.md`.
20+
* Untouched templates must not count as substance.
21+
*/
22+
const PR_TEMPLATE_BOILERPLATE_LINES = new Set([
23+
"explain the user-visible or maintainer-facing change.",
24+
"list the commands or checks you ran.",
25+
"scope stays focused and avoids unrelated cleanup.",
26+
"docs or release notes were updated when needed.",
27+
"security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.",
28+
]);
29+
30+
function isWrongAncestry({
31+
behindMain,
32+
behindBase,
33+
aheadMain = 0,
34+
threshold = ANCESTRY_BEHIND_THRESHOLD,
35+
aheadMainMax = ANCESTRY_AHEAD_MAIN_MAX,
36+
}) {
37+
return (
38+
behindMain === 0 &&
39+
behindBase >= threshold &&
40+
aheadMain <= aheadMainMax
41+
);
42+
}
43+
44+
function authorHasPushPermission(permission) {
45+
return permission === "admin" || permission === "maintain" || permission === "write";
46+
}
47+
48+
/**
49+
* True when the body uses literal backslash-n as the dominant line break
50+
* (agent bug seen on #644) rather than real newlines.
51+
*/
52+
function hasEscapedNewlines(text) {
53+
const escaped = (text.match(/\\n/g) || []).length;
54+
if (escaped < 2) return false;
55+
const real = (text.match(/\n/g) || []).length;
56+
return escaped > real;
57+
}
58+
59+
function countContentBlocks(text) {
60+
const blocks = text
61+
.split(/\n\s*\n/)
62+
.map((b) => b.trim())
63+
.filter(Boolean);
64+
if (blocks.length >= 2) return blocks.length;
65+
const bullets = text
66+
.split("\n")
67+
.map((l) => l.trim())
68+
.filter((l) => /^[-*+]\s+\S/.test(l));
69+
return Math.max(blocks.length, bullets.length);
70+
}
71+
72+
function normalizeTemplateLine(line) {
73+
return line
74+
.replace(/^\s*[-*+]\s+/, "")
75+
.replace(/^\s*\[[ xX]\]\s+/, "")
76+
.replace(/^\s*#{1,6}\s+/, "")
77+
.trim()
78+
.toLowerCase();
79+
}
80+
81+
/** Drop stock PR template headings, instructions, and checklist lines. */
82+
function stripPrTemplateBoilerplate(text) {
83+
return text
84+
.split("\n")
85+
.filter((line) => {
86+
const normalized = normalizeTemplateLine(line);
87+
if (!normalized) return true;
88+
if (PR_TEMPLATE_BOILERPLATE_LINES.has(normalized)) return false;
89+
if (/^(summary|verification|checklist)$/.test(normalized)) return false;
90+
return true;
91+
})
92+
.join("\n");
93+
}
94+
95+
function assessPrDescription(body) {
96+
if (typeof body !== "string" || !body.trim()) {
97+
return { ok: false, reason: "empty" };
98+
}
99+
if (hasEscapedNewlines(body)) {
100+
return { ok: false, reason: "escaped_newlines" };
101+
}
102+
const withoutTemplate = stripPrTemplateBoilerplate(body);
103+
const cleaned = clean(withoutTemplate);
104+
if (!cleaned) {
105+
const strippedComments = withoutTemplate.replace(/<!--[\s\S]*?-->/g, "").trim();
106+
if (!strippedComments) return { ok: false, reason: "empty" };
107+
if (isPlaceholderOnlyValue(strippedComments)) {
108+
return { ok: false, reason: "placeholder" };
109+
}
110+
return { ok: false, reason: "empty" };
111+
}
112+
if (isPlaceholderOnlyValue(cleaned)) {
113+
return { ok: false, reason: "placeholder" };
114+
}
115+
if (hasSubstantialStructuredContent(cleaned, MIN_SECTION_LEN, MIN_RICH_SECTIONS)) {
116+
return { ok: true };
117+
}
118+
if (
119+
cleaned.length >= UNSTRUCTURED_MIN_LEN &&
120+
countContentBlocks(cleaned) >= UNSTRUCTURED_MIN_BLOCKS
121+
) {
122+
return { ok: true };
123+
}
124+
return { ok: false, reason: "thin" };
125+
}
126+
127+
function collectPrQualityFailures({
128+
baseRef,
129+
allowedBases,
130+
body,
131+
behindMain,
132+
behindBase,
133+
aheadMain = 0,
134+
authorPermission,
135+
permissionLookupFailed = false,
136+
ancestryLookupFailed = false,
137+
}) {
138+
const failures = [];
139+
const wrongBase = !allowedBases.includes(baseRef);
140+
if (wrongBase) {
141+
failures.push({ code: "wrong_base" });
142+
} else {
143+
// Permission lookup fails closed (still evaluate ancestry). Compare API
144+
// failures skip ancestry — zeros would falsely pass the #644 heuristic.
145+
const skipAncestry =
146+
ancestryLookupFailed ||
147+
(!permissionLookupFailed && authorHasPushPermission(authorPermission));
148+
if (
149+
!skipAncestry &&
150+
isWrongAncestry({ behindMain, behindBase, aheadMain })
151+
) {
152+
failures.push({ code: "wrong_ancestry" });
153+
}
154+
}
155+
156+
const desc = assessPrDescription(body);
157+
if (!desc.ok) {
158+
failures.push({ code: "bad_description", reason: desc.reason });
159+
}
160+
return failures;
161+
}
162+
163+
module.exports = {
164+
ANCESTRY_BEHIND_THRESHOLD,
165+
ANCESTRY_AHEAD_MAIN_MAX,
166+
isWrongAncestry,
167+
authorHasPushPermission,
168+
assessPrDescription,
169+
collectPrQualityFailures,
170+
hasEscapedNewlines,
171+
stripPrTemplateBoilerplate,
172+
};

0 commit comments

Comments
 (0)