From e4879a54a138e7341aef17642fb26d78eb47e7b2 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:25:03 +0200 Subject: [PATCH] feat(ci): enforce PR ancestry and description quality gates (#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. --- .github/scripts/enforce-pr-target.test.cjs | 33 +- .github/scripts/issue-quality.cjs | 11 +- .github/scripts/issue-quality.test.cjs | 40 ++ .github/scripts/pr-quality.cjs | 172 +++++ .github/scripts/pr-quality.test.cjs | 245 +++++++ .github/workflows/enforce-pr-target.yml | 304 +++++++-- .github/workflows/issue-quality-tests.yml | 5 + AGENTS.md | 7 + MAINTAINERS.md | 6 + docs-site/src/content/docs/contributing.md | 7 + .../plans/2026-07-28-pr-quality-gates.md | 645 ++++++++++++++++++ .../2026-07-28-pr-quality-gates-design.md | 173 +++++ tests/ci-workflows.test.ts | 388 +++++++---- tests/helpers/enforce-pr-target-harness.ts | 77 ++- 14 files changed, 1902 insertions(+), 211 deletions(-) create mode 100644 .github/scripts/pr-quality.cjs create mode 100644 .github/scripts/pr-quality.test.cjs create mode 100644 docs/superpowers/plans/2026-07-28-pr-quality-gates.md create mode 100644 docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 018f8b3c3..79c3dbd26 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -13,8 +13,8 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /pull_request_target:/); assert.doesNotMatch( workflow, - /actions\/checkout@/, - "wrong-branch enforcer must not check out untrusted PR code", + /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/, + "enforcer must not check out untrusted PR head code", ); }); @@ -43,4 +43,33 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /readyConversionFailed/); assert.match(workflow, /Could not mark pull request ready for review/); }); + + it("listens for synchronize so rebase can clear ancestry failures", () => { + assert.match(workflow, /synchronize/); + }); + + it("checks out trusted default-branch scripts only (never PR head)", () => { + assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/); + assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.repository\.default_branch\s*\}\}/); + assert.match(workflow, /sparse-checkout:\s*\.github\/scripts/); + assert.match(workflow, /persist-credentials:\s*false/); + assert.doesNotMatch(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/); + }); + + it("loads pr-quality via require from the checked-out scripts", () => { + assert.match(workflow, /pr-quality\.cjs/); + assert.match(workflow, /collectPrQualityFailures/); + }); + + it("strips stale WRONG BRANCH prefix on failure when base is corrected", () => { + const failureBlock = workflow.match( + /if \(failures\.length > 0\) \{([\s\S]*?)core\.setFailed\(/, + ); + assert.ok(failureBlock, "workflow must have a failure path"); + const failurePath = failureBlock[1]; + assert.match(failurePath, /shouldStripTitlePrefix/); + assert.match(failurePath, /!hasWrongBase/); + assert.match(failurePath, /titlePrefixedByBot = false/); + assert.match(failurePath, /pr\.title\.slice\(TITLE_PREFIX\.length\)/); + }); }); diff --git a/.github/scripts/issue-quality.cjs b/.github/scripts/issue-quality.cjs index 681cdc55b..9dccb742f 100644 --- a/.github/scripts/issue-quality.cjs +++ b/.github/scripts/issue-quality.cjs @@ -704,11 +704,11 @@ function validateIssue(issue) { } else if ( !softPass && isNewBugForm && - version !== null && - (isEmpty(version) || isRawPlaceholder(version)) + (version === null || isEmpty(version) || isRawPlaceholder(version)) ) { - // New form requires Version. Legacy N/A / No response soft-pass stays - // only for bodies without Client or integration. + // New form requires Version (including when the heading was removed). + // Legacy N/A / No response soft-pass stays only for bodies without + // Client or integration. reasons.push("Version is missing."); guidance.push("Add your OpenCodex version so we can reproduce the environment."); } @@ -719,8 +719,7 @@ function validateIssue(issue) { } else if ( !softPass && isNewBugForm && - os !== null && - (isEmpty(os) || isRawPlaceholder(os)) + (os === null || isEmpty(os) || isRawPlaceholder(os)) ) { reasons.push("Operating system is missing."); guidance.push("Add your OS name and version (for example Windows 11 24H2)."); diff --git a/.github/scripts/issue-quality.test.cjs b/.github/scripts/issue-quality.test.cjs index 9c8f9bb86..c629a3d78 100644 --- a/.github/scripts/issue-quality.test.cjs +++ b/.github/scripts/issue-quality.test.cjs @@ -799,6 +799,46 @@ describe("validateIssue - bug", () => { assert.ok(result.reasons.some((r) => /Operating system/i.test(r))); }); + it("rejects a new-form bug when the Version heading was removed", () => { + const body = [ + "### Client or integration", + "Codex CLI", + "### Area", + "CLI", + "### Summary", + "Proxy returns 502 when streaming is enabled on Windows.", + "### Reproduction", + "1. ocx start", + "2. Send a streaming /v1/responses request", + "### Operating system", + "Windows 11", + ].join("\n"); + const result = validateIssue({ title: "Streaming 502", body, labels: ["bug"] }); + assert.equal(result.kind, "bug"); + assert.equal(result.valid, false); + assert.ok(result.reasons.some((r) => /Version/i.test(r) && /missing/i.test(r))); + }); + + it("rejects a new-form bug when the Operating system heading was removed", () => { + const body = [ + "### Client or integration", + "Codex CLI", + "### Area", + "CLI", + "### Summary", + "Proxy returns 502 when streaming is enabled on Windows.", + "### Reproduction", + "1. ocx start", + "2. Send a streaming /v1/responses request", + "### Version", + "2.7.42", + ].join("\n"); + const result = validateIssue({ title: "Streaming 502", body, labels: ["bug"] }); + assert.equal(result.kind, "bug"); + assert.equal(result.valid, false); + assert.ok(result.reasons.some((r) => /Operating system/i.test(r) && /missing/i.test(r))); + }); + it("rejects a new-form bug whose Reproduction is only a vague phrase", () => { const body = [ "### Client or integration", diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs new file mode 100644 index 000000000..ff286acdc --- /dev/null +++ b/.github/scripts/pr-quality.cjs @@ -0,0 +1,172 @@ +"use strict"; + +const path = require("node:path"); +const { + clean, + isPlaceholderOnlyValue, + hasSubstantialStructuredContent, +} = require(path.join(__dirname, "issue-quality.cjs")); + +const ANCESTRY_BEHIND_THRESHOLD = 20; +/** Cap on ahead_by vs main so stale `dev` forks (many commits ahead of main) are not flagged. */ +const ANCESTRY_AHEAD_MAIN_MAX = 5; +const MIN_SECTION_LEN = 40; +const MIN_RICH_SECTIONS = 2; +const UNSTRUCTURED_MIN_LEN = 120; +const UNSTRUCTURED_MIN_BLOCKS = 2; + +/** + * Exact instruction / checklist lines from `.github/PULL_REQUEST_TEMPLATE.md`. + * Untouched templates must not count as substance. + */ +const PR_TEMPLATE_BOILERPLATE_LINES = new Set([ + "explain the user-visible or maintainer-facing change.", + "list the commands or checks you ran.", + "scope stays focused and avoids unrelated cleanup.", + "docs or release notes were updated when needed.", + "security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.", +]); + +function isWrongAncestry({ + behindMain, + behindBase, + aheadMain = 0, + threshold = ANCESTRY_BEHIND_THRESHOLD, + aheadMainMax = ANCESTRY_AHEAD_MAIN_MAX, +}) { + return ( + behindMain === 0 && + behindBase >= threshold && + aheadMain <= aheadMainMax + ); +} + +function authorHasPushPermission(permission) { + return permission === "admin" || permission === "maintain" || permission === "write"; +} + +/** + * True when the body uses literal backslash-n as the dominant line break + * (agent bug seen on #644) rather than real newlines. + */ +function hasEscapedNewlines(text) { + const escaped = (text.match(/\\n/g) || []).length; + if (escaped < 2) return false; + const real = (text.match(/\n/g) || []).length; + return escaped > real; +} + +function countContentBlocks(text) { + const blocks = text + .split(/\n\s*\n/) + .map((b) => b.trim()) + .filter(Boolean); + if (blocks.length >= 2) return blocks.length; + const bullets = text + .split("\n") + .map((l) => l.trim()) + .filter((l) => /^[-*+]\s+\S/.test(l)); + return Math.max(blocks.length, bullets.length); +} + +function normalizeTemplateLine(line) { + return line + .replace(/^\s*[-*+]\s+/, "") + .replace(/^\s*\[[ xX]\]\s+/, "") + .replace(/^\s*#{1,6}\s+/, "") + .trim() + .toLowerCase(); +} + +/** Drop stock PR template headings, instructions, and checklist lines. */ +function stripPrTemplateBoilerplate(text) { + return text + .split("\n") + .filter((line) => { + const normalized = normalizeTemplateLine(line); + if (!normalized) return true; + if (PR_TEMPLATE_BOILERPLATE_LINES.has(normalized)) return false; + if (/^(summary|verification|checklist)$/.test(normalized)) return false; + return true; + }) + .join("\n"); +} + +function assessPrDescription(body) { + if (typeof body !== "string" || !body.trim()) { + return { ok: false, reason: "empty" }; + } + if (hasEscapedNewlines(body)) { + return { ok: false, reason: "escaped_newlines" }; + } + const withoutTemplate = stripPrTemplateBoilerplate(body); + const cleaned = clean(withoutTemplate); + if (!cleaned) { + const strippedComments = withoutTemplate.replace(//g, "").trim(); + if (!strippedComments) return { ok: false, reason: "empty" }; + if (isPlaceholderOnlyValue(strippedComments)) { + return { ok: false, reason: "placeholder" }; + } + return { ok: false, reason: "empty" }; + } + if (isPlaceholderOnlyValue(cleaned)) { + return { ok: false, reason: "placeholder" }; + } + if (hasSubstantialStructuredContent(cleaned, MIN_SECTION_LEN, MIN_RICH_SECTIONS)) { + return { ok: true }; + } + if ( + cleaned.length >= UNSTRUCTURED_MIN_LEN && + countContentBlocks(cleaned) >= UNSTRUCTURED_MIN_BLOCKS + ) { + return { ok: true }; + } + return { ok: false, reason: "thin" }; +} + +function collectPrQualityFailures({ + baseRef, + allowedBases, + body, + behindMain, + behindBase, + aheadMain = 0, + authorPermission, + permissionLookupFailed = false, + ancestryLookupFailed = false, +}) { + const failures = []; + const wrongBase = !allowedBases.includes(baseRef); + if (wrongBase) { + failures.push({ code: "wrong_base" }); + } else { + // Permission lookup fails closed (still evaluate ancestry). Compare API + // failures skip ancestry — zeros would falsely pass the #644 heuristic. + const skipAncestry = + ancestryLookupFailed || + (!permissionLookupFailed && authorHasPushPermission(authorPermission)); + if ( + !skipAncestry && + isWrongAncestry({ behindMain, behindBase, aheadMain }) + ) { + failures.push({ code: "wrong_ancestry" }); + } + } + + const desc = assessPrDescription(body); + if (!desc.ok) { + failures.push({ code: "bad_description", reason: desc.reason }); + } + return failures; +} + +module.exports = { + ANCESTRY_BEHIND_THRESHOLD, + ANCESTRY_AHEAD_MAIN_MAX, + isWrongAncestry, + authorHasPushPermission, + assessPrDescription, + collectPrQualityFailures, + hasEscapedNewlines, + stripPrTemplateBoilerplate, +}; diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs new file mode 100644 index 000000000..a2ac0da0b --- /dev/null +++ b/.github/scripts/pr-quality.test.cjs @@ -0,0 +1,245 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + ANCESTRY_BEHIND_THRESHOLD, + isWrongAncestry, + authorHasPushPermission, + assessPrDescription, + collectPrQualityFailures, +} = require("./pr-quality.cjs"); + +describe("isWrongAncestry", () => { + it("flags #644-shaped compares (0 behind main, far behind base, few ahead of main)", () => { + assert.equal( + isWrongAncestry({ behindMain: 0, behindBase: 44, aheadMain: 1 }), + true, + ); + }); + + it("uses threshold 20 by default", () => { + assert.equal(ANCESTRY_BEHIND_THRESHOLD, 20); + assert.equal(isWrongAncestry({ behindMain: 0, behindBase: 20, aheadMain: 1 }), true); + assert.equal(isWrongAncestry({ behindMain: 0, behindBase: 19, aheadMain: 1 }), false); + }); + + it("passes when head is behind main (not sitting on main tip)", () => { + assert.equal(isWrongAncestry({ behindMain: 1, behindBase: 44, aheadMain: 1 }), false); + }); + + it("passes stale dev-based branches that are many commits ahead of main", () => { + assert.equal( + isWrongAncestry({ behindMain: 0, behindBase: 44, aheadMain: 50 }), + false, + ); + }); +}); + +describe("authorHasPushPermission", () => { + it("accepts write/maintain/admin only", () => { + assert.equal(authorHasPushPermission("admin"), true); + assert.equal(authorHasPushPermission("maintain"), true); + assert.equal(authorHasPushPermission("write"), true); + assert.equal(authorHasPushPermission("triage"), false); + assert.equal(authorHasPushPermission("read"), false); + assert.equal(authorHasPushPermission(null), false); + }); +}); + +describe("assessPrDescription", () => { + it("rejects empty and comment-only bodies", () => { + assert.equal(assessPrDescription("").ok, false); + assert.equal(assessPrDescription(" ").ok, false); + assert.equal( + assessPrDescription("\n\n").reason, + "empty", + ); + }); + + it("rejects placeholder-only bodies", () => { + assert.equal(assessPrDescription("N/A").reason, "placeholder"); + assert.equal(assessPrDescription("TODO").reason, "placeholder"); + }); + + it("rejects literal escaped newlines like #644", () => { + const body = + "## What changed\\n- make the Windows tray launcher resolve Codex home\\n\\n## Validation\\n- git diff --check"; + assert.equal(assessPrDescription(body).reason, "escaped_newlines"); + }); + + it("rejects thin real-newline bodies", () => { + assert.equal(assessPrDescription("fix stuff").reason, "thin"); + }); + + it("rejects an untouched GitHub PR template as empty/thin", () => { + const body = [ + "## Summary", + "", + "- Explain the user-visible or maintainer-facing change.", + "", + "## Verification", + "", + "- List the commands or checks you ran.", + "", + "## Checklist", + "", + "- [ ] Scope stays focused and avoids unrelated cleanup.", + "- [ ] Docs or release notes were updated when needed.", + "- [ ] Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.", + ].join("\n"); + const result = assessPrDescription(body); + assert.equal(result.ok, false); + assert.ok(result.reason === "empty" || result.reason === "thin"); + }); + + it("accepts two rich markdown sections", () => { + const body = [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"); + assert.equal(assessPrDescription(body).ok, true); + }); + + it("accepts unstructured bodies that are long enough with multiple blocks", () => { + const p1 = + "Updates the Windows tray launcher to resolve the active Codex home through the shared helper so listener and launcher stay aligned."; + const p2 = + "Validated with git diff --check on the changed tray module; typecheck was not available in that session so CI must cover it."; + assert.equal(assessPrDescription(`${p1}\n\n${p2}`).ok, true); + }); +}); + +describe("collectPrQualityFailures", () => { + const allowed = ["dev", "dev2-go"]; + + it("reports wrong_base without requiring ancestry inputs", () => { + const failures = collectPrQualityFailures({ + baseRef: "main", + allowedBases: allowed, + body: "## Summary\n" + "x".repeat(50) + "\n\n## Test plan\n" + "y".repeat(50), + behindMain: 0, + behindBase: 0, + authorPermission: "read", + }); + assert.ok(failures.some((f) => f.code === "wrong_base")); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + }); + + it("reports wrong_base and bad_description together for main + empty body", () => { + const failures = collectPrQualityFailures({ + baseRef: "main", + allowedBases: allowed, + body: "", + behindMain: 0, + behindBase: 0, + authorPermission: "read", + }); + assert.ok(failures.some((f) => f.code === "wrong_base")); + assert.ok(failures.some((f) => f.code === "bad_description")); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + }); + + it("reports wrong_ancestry for contributor on #644-shaped compare", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"), + behindMain: 0, + behindBase: 44, + aheadMain: 1, + authorPermission: "read", + }); + assert.deepEqual( + failures.map((f) => f.code), + ["wrong_ancestry"], + ); + }); + + it("skips ancestry for push permission but still flags bad description", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: "", + behindMain: 0, + behindBase: 44, + aheadMain: 1, + authorPermission: "write", + }); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + assert.ok(failures.some((f) => f.code === "bad_description")); + }); + + it("applies ancestry when permission lookup failed (fail closed)", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"), + behindMain: 0, + behindBase: 44, + aheadMain: 1, + authorPermission: null, + permissionLookupFailed: true, + }); + assert.ok(failures.some((f) => f.code === "wrong_ancestry")); + }); + + it("does not flag stale dev-based branches that are far ahead of main", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"), + behindMain: 0, + behindBase: 44, + aheadMain: 50, + authorPermission: "read", + }); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + }); + + it("skips ancestry when compare lookup failed (cannot evaluate)", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"), + behindMain: 0, + behindBase: 0, + aheadMain: 0, + authorPermission: "read", + ancestryLookupFailed: true, + }); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + }); +}); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 6e8a52af6..8864b25da 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -7,6 +7,7 @@ on: - reopened - edited - ready_for_review + - synchronize # pull-requests:write covers title/comment updates. # contents:write is required for convertPullRequestToDraft / @@ -25,30 +26,39 @@ jobs: runs-on: ubuntu-latest steps: - - name: Require dev as PR target + - name: Checkout trusted PR-quality scripts + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Enforce PR target, ancestry, and description uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: script: | + const path = require("path"); + const { collectPrQualityFailures } = require( + path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), + ); + const ALLOWED_BASES = ["dev", "dev2-go"]; const DEFAULT_BASE = "dev"; const TITLE_PREFIX = "[WRONG BRANCH] "; - const COMMENT_MARKER = ""; + const COMMENT_MARKER = ""; + const LEGACY_COMMENT_MARKER = ""; const STATE_PATTERN = - //; + //; const { owner, repo } = context.repo; const pull_number = context.payload.pull_request.number; - // Fetch the latest PR state instead of relying on a possibly - // outdated event payload. const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number }); - // Find the workflow's existing comment. Its hidden state records - // whether the workflow changed the title or draft status. const comments = await github.paginate( github.rest.issues.listComments, { @@ -62,7 +72,8 @@ jobs: const botComment = comments.find( comment => comment.user?.login === "github-actions[bot]" && - comment.body?.includes(COMMENT_MARKER) + (comment.body?.includes(COMMENT_MARKER) || + comment.body?.includes(LEGACY_COMMENT_MARKER)) ); let botCommentId = botComment?.id ?? null; @@ -86,7 +97,7 @@ jobs: function stateMarker(state) { return ( - "" ); @@ -161,43 +172,198 @@ jobs: ); } + function descriptionFailureLines(reason) { + switch (reason) { + case "empty": + return [ + "The pull request body is empty after stripping HTML comments.", + "", + "Include a real description: a **Summary** of what changed and why, plus a **Test plan** (or equivalent substance)." + ]; + case "placeholder": + return [ + "The pull request body contains only placeholder text (for example `N/A`, `TODO`, or `No response`).", + "", + "Replace placeholders with a **Summary** and **Test plan**, or another description with at least two substantive sections or paragraphs." + ]; + case "escaped_newlines": + return [ + "The pull request body uses literal `\\n` escape sequences instead of real line breaks.", + "", + "Fix the formatting so the body uses normal markdown line breaks, then add a **Summary** and **Test plan**." + ]; + case "thin": + default: + return [ + "The pull request description is too thin to review.", + "", + "Add a **Summary** and **Test plan** (two sections with at least 40 characters each), or an unstructured body of at least 120 characters with two paragraphs or bullet groups." + ]; + } + } + + function buildFailureSections(failures) { + const sections = []; + + if (failures.some(failure => failure.code === "wrong_base")) { + sections.push( + "⚠️ **Wrong target branch**", + "", + `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${ALLOWED_BASES.map(inlineCode).join(" or ")}.`, + "", + `@${pr.user.login} Please retarget this PR to ${inlineCode(DEFAULT_BASE)}. Most contributions go to ${inlineCode(DEFAULT_BASE)} first; use ${inlineCode("dev2-go")} only for scoped Go native-port work. \`main\` receives only release promotions. See our [Contributing guide](https://lidge-jun.github.io/opencodex/contributing/) for details. Thanks! 🙏` + ); + } + + if (failures.some(failure => failure.code === "wrong_ancestry")) { + sections.push( + "⚠️ **Wrong branch ancestry**", + "", + `This pull request targets ${inlineCode(pr.base.ref)}, but its head appears to sit on the current ${inlineCode("main")} tip while being far behind ${inlineCode(pr.base.ref)}.`, + "", + `@${pr.user.login} Rebase onto the current ${inlineCode(pr.base.ref)} branch instead of opening from ${inlineCode("main")}. That keeps already-released commits out of the integration branch.` + ); + } + + const badDescription = failures.find( + failure => failure.code === "bad_description" + ); + if (badDescription) { + sections.push( + "⚠️ **Pull request description**", + "", + ...descriptionFailureLines(badDescription.reason) + ); + } + + return sections; + } + + function failureSummary(failures) { + return failures + .map(failure => { + if (failure.code === "wrong_base") { + return `wrong base (${pr.base.ref})`; + } + if (failure.code === "wrong_ancestry") { + return "wrong ancestry"; + } + if (failure.code === "bad_description") { + return `bad description (${failure.reason})`; + } + return failure.code; + }) + .join("; "); + } + const storedState = parseState(botComment?.body); - const wrongBase = !ALLOWED_BASES.includes(pr.base.ref); - - // Wrong branch: - // - add the title prefix - // - convert ready PRs to draft (best-effort) - // - post or update the explanation - // - fail the required check so merge stays blocked even if draft fails - // - remember which changes were made by this workflow - if (wrongBase) { - const willPrefixTitle = !pr.title.startsWith(TITLE_PREFIX); + + let authorPermission = null; + let permissionLookupFailed = false; + try { + const { data: permissionData } = + await github.rest.repos.getCollaboratorPermissionLevel({ + owner, + repo, + username: pr.user.login + }); + authorPermission = permissionData.permission; + } catch (error) { + permissionLookupFailed = true; + core.warning( + `Could not look up collaborator permission: ${error.message}` + ); + } + + let behindMain = 0; + let behindBase = 0; + let aheadMain = 0; + let ancestryLookupFailed = false; + const baseAllowed = ALLOWED_BASES.includes(pr.base.ref); + + if (baseAllowed) { + const headSha = pr.head.sha; + try { + const { data: mainCompare } = + await github.rest.repos.compareCommitsWithBasehead({ + owner, + repo, + basehead: `main...${headSha}` + }); + behindMain = mainCompare.behind_by; + aheadMain = mainCompare.ahead_by; + + const { data: baseCompare } = + await github.rest.repos.compareCommitsWithBasehead({ + owner, + repo, + basehead: `${pr.base.ref}...${headSha}` + }); + behindBase = baseCompare.behind_by; + } catch (error) { + ancestryLookupFailed = true; + core.warning( + `Could not compare commits for ancestry check: ${error.message}` + ); + } + } + + const failures = collectPrQualityFailures({ + baseRef: pr.base.ref, + allowedBases: ALLOWED_BASES, + body: pr.body, + behindMain, + behindBase, + aheadMain, + authorPermission, + permissionLookupFailed, + ancestryLookupFailed + }); + + if (failures.length > 0) { + const hasWrongBase = failures.some( + failure => failure.code === "wrong_base" + ); + const willPrefixTitle = + hasWrongBase && !pr.title.startsWith(TITLE_PREFIX); const state = storedState?.active ? { ...storedState } : { version: 1, active: true, autoDraftedByBot: false, - titlePrefixedByBot: false + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false }; - // Claim title ownership before mutating so a crash after the - // title write still records what the bot owns. Do not claim a - // new draft here — only after convertToDraft succeeds. + state.ancestryFailed = failures.some( + failure => failure.code === "wrong_ancestry" + ); + state.descriptionFailed = failures.some( + failure => failure.code === "bad_description" + ); + + const shouldStripTitlePrefix = + !hasWrongBase && + state.titlePrefixedByBot && + pr.title.startsWith(TITLE_PREFIX); + + // Claim title ownership before adding a prefix. Do not clear + // ownership until strip succeeds — a failed update must retry. if (willPrefixTitle) { state.titlePrefixedByBot = true; } let draftConversionFailed = false; + const failureSections = buildFailureSections(failures); await upsertComment( [ COMMENT_MARKER, stateMarker(state), "", - "⚠️ **Wrong target branch**", - "", - `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${ALLOWED_BASES.map(inlineCode).join(" or ")}.`, + ...failureSections, "", "Recording ownership state before applying title/draft changes…" ].join("\n") @@ -210,22 +376,48 @@ jobs: pull_number, title: `${TITLE_PREFIX}${pr.title}` }); + } else if (shouldStripTitlePrefix) { + await github.rest.pulls.update({ + owner, + repo, + pull_number, + title: pr.title.slice(TITLE_PREFIX.length) + }); + state.titlePrefixedByBot = false; + await upsertComment( + [ + COMMENT_MARKER, + stateMarker(state), + "", + ...failureSections, + "", + "Stale title prefix removed; continuing…" + ].join("\n") + ); } if (!pr.draft) { + // Claim draft ownership before the mutation so a successful + // convert followed by a failed comment still restores later. + state.autoDraftedByBot = true; + await upsertComment( + [ + COMMENT_MARKER, + stateMarker(state), + "", + ...failureSections, + "", + "Draft conversion pending…" + ].join("\n") + ); try { await convertToDraft(); - state.autoDraftedByBot = true; - // Checkpoint after a successful draft so ownership survives - // if the final explanation write fails. await upsertComment( [ COMMENT_MARKER, stateMarker(state), "", - "⚠️ **Wrong target branch**", - "", - `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${ALLOWED_BASES.map(inlineCode).join(" or ")}.`, + ...failureSections, "", "Draft conversion succeeded; finalising explanation…" ].join("\n") @@ -240,45 +432,43 @@ jobs: } const draftExplanation = draftConversionFailed - ? "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually, or retarget it to the correct base. The required `enforce-target` check will keep failing until the base is corrected." + ? "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required `enforce-target` check will keep failing until every issue above is resolved." : state.autoDraftedByBot - ? "This pull request is being kept as a draft automatically. Once the target branch is corrected, it will be marked ready for review again." - : "This pull request was already a draft. Its draft status will be preserved after the target branch is corrected."; + ? "This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again." + : "This pull request was already a draft. Its draft status will be preserved after every issue above is resolved."; + + const finalSections = [...failureSections]; + + if (hasWrongBase && state.titlePrefixedByBot) { + finalSections.push( + "", + `Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.` + ); + } await upsertComment( [ COMMENT_MARKER, stateMarker(state), "", - "⚠️ **Wrong target branch**", - "", - `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${ALLOWED_BASES.map(inlineCode).join(" or ")}.`, - "", - `Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.`, - "", - `@${pr.user.login} Please retarget this PR to ${inlineCode(DEFAULT_BASE)}. Most contributions go to ${inlineCode(DEFAULT_BASE)} first; use ${inlineCode("dev2-go")} only for scoped Go native-port work. \`main\` receives only release promotions. See our [Contributing guide](https://lidge-jun.github.io/opencodex/contributing/) for details. Thanks! 🙏`, + ...finalSections, "", draftExplanation ].join("\n") ); - core.setFailed( - `Pull request targets ${pr.base.ref}; allowed bases are ${ALLOWED_BASES.join(", ")}.` - ); + core.setFailed(`PR quality gate failed: ${failureSummary(failures)}`); return; } - // The target is correct and the workflow has nothing to restore. if (!storedState?.active) { core.info( - "Target branch is correct and there is no active bot state." + "All PR quality gates passed and there is no active bot state." ); return; } - // Remove only the exact prefix added by this workflow. Other title - // edits made by the contributor are preserved. if ( storedState.titlePrefixedByBot && pr.title.startsWith(TITLE_PREFIX) @@ -291,8 +481,6 @@ jobs: }); } - // Mark the PR ready only if this workflow originally converted it - // to draft. Intentionally drafted PRs remain drafts. let readyConversionFailed = false; if ( storedState.autoDraftedByBot && @@ -310,18 +498,20 @@ jobs: const completedState = readyConversionFailed ? { - // Keep ownership active so a later edited/reopened/ - // ready_for_review event retries markReadyForReview. version: 1, active: true, autoDraftedByBot: true, - titlePrefixedByBot: false + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false } : { version: 1, active: false, autoDraftedByBot: false, - titlePrefixedByBot: false + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false }; const titleResult = storedState.titlePrefixedByBot @@ -339,9 +529,9 @@ jobs: COMMENT_MARKER, stateMarker(completedState), "", - "✅ **Target branch corrected**", + "✅ **PR quality gates passed**", "", - `This pull request now targets ${inlineCode(pr.base.ref)}.`, + `This pull request now targets ${inlineCode(pr.base.ref)} with acceptable ancestry and description.`, "", `${titleResult} ${draftResult}` ].join("\n") diff --git a/.github/workflows/issue-quality-tests.yml b/.github/workflows/issue-quality-tests.yml index c6c72b66a..8c6d0da14 100644 --- a/.github/workflows/issue-quality-tests.yml +++ b/.github/workflows/issue-quality-tests.yml @@ -6,6 +6,8 @@ on: - ".github/ISSUE_TEMPLATE/**" - ".github/scripts/issue-quality.cjs" - ".github/scripts/issue-quality.test.cjs" + - ".github/scripts/pr-quality.cjs" + - ".github/scripts/pr-quality.test.cjs" - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" @@ -25,6 +27,8 @@ on: - ".github/ISSUE_TEMPLATE/**" - ".github/scripts/issue-quality.cjs" - ".github/scripts/issue-quality.test.cjs" + - ".github/scripts/pr-quality.cjs" + - ".github/scripts/pr-quality.test.cjs" - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" @@ -56,6 +60,7 @@ jobs: - name: Run validator tests run: | node --test .github/scripts/issue-quality.test.cjs + node --test .github/scripts/pr-quality.test.cjs node --test .github/scripts/pr-labeler.test.cjs node --test .github/scripts/enforce-pr-target.test.cjs node --test .github/scripts/issue-translation.test.cjs diff --git a/AGENTS.md b/AGENTS.md index 2502e1711..cc24cd4c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,6 +80,13 @@ integration line to another, or rebasing a stale branch onto the current head, is ordinary maintenance rather than noise — open it as a normal pull request and name the source commits in the description. +The **`enforce-target`** CI check rejects pull requests whose head +ancestry sits on the **`main`** tip while far behind **`dev`** or **`dev2-go`**, +and rejects empty, thin, or malformed descriptions; authors with repository +push permission skip the ancestry heuristic only. As with approval requirements +in [`MAINTAINERS.md`](./MAINTAINERS.md), this is enforced by convention until +branch protection is configured. + [`MAINTAINERS.md`](./MAINTAINERS.md) is authoritative for review and merge policy (approvals, CI requirements, security review, promotion). This file summarizes; it never overrides it. diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 2a8c08c9d..6c9a13ae8 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -44,6 +44,12 @@ through GitHub repository settings. `dev2-go` before closing out the `dev` merge, label it `needs-go-port`, and name the source commits. That label is the durable signal that a deferred port is intentional, not forgotten. +- The **`enforce-target`** CI check rejects pull requests whose head + ancestry sits on the **`main`** tip while far behind **`dev`** or **`dev2-go`**, + and rejects empty, thin, or malformed descriptions; authors with repository + push permission skip the ancestry heuristic only. As with the approval + requirement above, this is enforced by convention until branch protection is + configured (see the note under the change log). - A pull request requires approval from at least one maintainer and successful required CI checks before merge. - Authors do not approve their own pull requests. diff --git a/docs-site/src/content/docs/contributing.md b/docs-site/src/content/docs/contributing.md index 8bbfe3164..1b225c420 100644 --- a/docs-site/src/content/docs/contributing.md +++ b/docs-site/src/content/docs/contributing.md @@ -104,6 +104,13 @@ integration line to another, or rebasing a stale branch onto the current head, is normal contribution rather than noise — note the source commits in the description. +## Pull requests + +- Target **`dev`** (or **`dev2-go`** only for scoped Go native-port work). Do not open ordinary feature or fix pull requests against **`main`**. +- Branch from the current **`dev`** tip, not from **`main`**. The required **`enforce-target`** check rejects heads whose merge base sits on the **`main`** tip while the branch is far behind the pull request base (the failure mode seen in #644). +- Write a real description: a **Summary** of what changed and why, plus a **Test plan** (or equivalent substance). Empty bodies, placeholder-only text, and descriptions that use escaped `\n` instead of real line breaks fail the check. +- Workflow changes in this repository use **`pull_request_target`**. Updated enforcement logic applies only after the workflow is promoted to the repository default branch — the same operational caveat documented in #631. + ## Project maintainers The current maintainers, their responsibilities, and the review and merge policy are documented in diff --git a/docs/superpowers/plans/2026-07-28-pr-quality-gates.md b/docs/superpowers/plans/2026-07-28-pr-quality-gates.md new file mode 100644 index 000000000..fe04ac5aa --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-pr-quality-gates.md @@ -0,0 +1,645 @@ +# PR Quality Gates (Ancestry + Description) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Extend `enforce-pr-target` so PRs like #644 fail: wrong ancestry (branched from `main` while targeting `dev`/`dev2-go`) and empty/thin/malformed descriptions, with the same draft + comment + `setFailed` UX as wrong-base. + +**Architecture:** Pure validators live in `.github/scripts/pr-quality.cjs` (reusing `issue-quality` helpers for placeholders / structured sections). The workflow checks out **trusted** scripts from the repository default branch (sparse, no PR head), then the existing `github-script` step requires that module, evaluates base + ancestry + description, and drafts/`setFailed`s when any gate fails. + +**Tech Stack:** Node CommonJS (Actions scripts), `node:test` for script unit tests, Bun/`tests/ci-workflows.test.ts` + `enforce-pr-target-harness.ts` for workflow behavioral coverage, Astro docs-site for contributing copy. + +**Spec:** `docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md` + +## Global Constraints + +- `ANCESTRY_BEHIND_THRESHOLD = 20` +- Ancestry fail when `behind_main === 0 && behind_base >= 20` +- Release compare ref = `main` +- Description: min section length `40`, min rich sections `2`, unstructured min length `120`, min blocks `2` +- Skip ancestry only for authors with push/maintain/admin on the base repo; permission API failure → fail closed (apply ancestry) +- Do **not** skip description for maintainers +- `[WRONG BRANCH]` title prefix only for wrong **base** +- `pull_request_target`: never check out PR head; checkout only `github.event.repository.default_branch` + sparse `.github/scripts` +- Permissions stay `contents: write` + `pull-requests: write` +- Add trigger type `synchronize` +- No live GitHub in unit tests + +## File map + +| File | Role | +| --- | --- | +| `.github/scripts/pr-quality.cjs` | Pure ancestry + description assessment + failure collectors | +| `.github/scripts/pr-quality.test.cjs` | Node unit tests for pure rules | +| `.github/workflows/enforce-pr-target.yml` | Checkout + require + multi-gate orchestration | +| `.github/scripts/enforce-pr-target.test.cjs` | Static workflow assertions (checkout safety, synchronize, paths) | +| `tests/helpers/enforce-pr-target-harness.ts` | Allow `require` of scripts; mock compare + permission; PR `body` | +| `tests/ci-workflows.test.ts` | Structural allowlist + behavioral scenarios | +| `.github/workflows/issue-quality-tests.yml` | Path filters for new script/tests | +| `docs-site/src/content/docs/contributing.md` | User-facing branch + description rules | +| `AGENTS.md` / `MAINTAINERS.md` | One-line CI policy note | + +--- + +### Task 1: Pure `pr-quality.cjs` (ancestry + description) + +**Files:** +- Create: `.github/scripts/pr-quality.cjs` +- Create: `.github/scripts/pr-quality.test.cjs` +- Modify: `.github/workflows/issue-quality-tests.yml` (add path filters + `node --test` line) + +**Interfaces:** +- Produces: + - `ANCESTRY_BEHIND_THRESHOLD` number (`20`) + - `isWrongAncestry({ behindMain, behindBase, threshold? }) → boolean` + - `authorHasPushPermission(permission: string | null | undefined) → boolean` — true for `admin` \| `maintain` \| `write` + - `assessPrDescription(body: string | null | undefined) → { ok: true } | { ok: false, reason: "empty" | "placeholder" | "escaped_newlines" | "thin" }` + - `collectPrQualityFailures({ baseRef, allowedBases, body, behindMain, behindBase, authorPermission, permissionLookupFailed? }) → Array<{ code: "wrong_base" | "wrong_ancestry" | "bad_description", reason?: string }>` + - `wrong_base` when `!allowedBases.includes(baseRef)` + - `wrong_ancestry` only when base allowed, and (`permissionLookupFailed` or `!authorHasPushPermission(authorPermission)`), and `isWrongAncestry(...)` + - `bad_description` whenever `assessPrDescription` is not ok (even if wrong_base) + +- [ ] **Step 1: Write the failing tests** + +Create `.github/scripts/pr-quality.test.cjs`: + +```js +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + ANCESTRY_BEHIND_THRESHOLD, + isWrongAncestry, + authorHasPushPermission, + assessPrDescription, + collectPrQualityFailures, +} = require("./pr-quality.cjs"); + +describe("isWrongAncestry", () => { + it("flags #644-shaped compares (0 behind main, far behind base)", () => { + assert.equal( + isWrongAncestry({ behindMain: 0, behindBase: 44 }), + true, + ); + }); + + it("uses threshold 20 by default", () => { + assert.equal(ANCESTRY_BEHIND_THRESHOLD, 20); + assert.equal(isWrongAncestry({ behindMain: 0, behindBase: 20 }), true); + assert.equal(isWrongAncestry({ behindMain: 0, behindBase: 19 }), false); + }); + + it("passes when head is behind main (not sitting on main tip)", () => { + assert.equal(isWrongAncestry({ behindMain: 1, behindBase: 44 }), false); + }); +}); + +describe("authorHasPushPermission", () => { + it("accepts write/maintain/admin only", () => { + assert.equal(authorHasPushPermission("admin"), true); + assert.equal(authorHasPushPermission("maintain"), true); + assert.equal(authorHasPushPermission("write"), true); + assert.equal(authorHasPushPermission("triage"), false); + assert.equal(authorHasPushPermission("read"), false); + assert.equal(authorHasPushPermission(null), false); + }); +}); + +describe("assessPrDescription", () => { + it("rejects empty and comment-only bodies", () => { + assert.equal(assessPrDescription("").ok, false); + assert.equal(assessPrDescription(" ").ok, false); + assert.equal( + assessPrDescription("\n\n").reason, + "empty", + ); + }); + + it("rejects placeholder-only bodies", () => { + assert.equal(assessPrDescription("N/A").reason, "placeholder"); + assert.equal(assessPrDescription("TODO").reason, "placeholder"); + }); + + it("rejects literal escaped newlines like #644", () => { + const body = + "## What changed\\n- make the Windows tray launcher resolve Codex home\\n\\n## Validation\\n- git diff --check"; + assert.equal(assessPrDescription(body).reason, "escaped_newlines"); + }); + + it("rejects thin real-newline bodies", () => { + assert.equal(assessPrDescription("fix stuff").reason, "thin"); + }); + + it("accepts two rich markdown sections", () => { + const body = [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"); + assert.equal(assessPrDescription(body).ok, true); + }); + + it("accepts unstructured bodies that are long enough with multiple blocks", () => { + const p1 = + "Updates the Windows tray launcher to resolve the active Codex home through the shared helper so listener and launcher stay aligned."; + const p2 = + "Validated with git diff --check on the changed tray module; typecheck was not available in that session so CI must cover it."; + assert.equal(assessPrDescription(`${p1}\n\n${p2}`).ok, true); + }); +}); + +describe("collectPrQualityFailures", () => { + const allowed = ["dev", "dev2-go"]; + + it("reports wrong_base without requiring ancestry inputs", () => { + const failures = collectPrQualityFailures({ + baseRef: "main", + allowedBases: allowed, + body: "## Summary\n" + "x".repeat(50) + "\n\n## Test plan\n" + "y".repeat(50), + behindMain: 0, + behindBase: 0, + authorPermission: "read", + }); + assert.ok(failures.some((f) => f.code === "wrong_base")); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + }); + + it("reports wrong_ancestry for contributor on #644-shaped compare", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"), + behindMain: 0, + behindBase: 44, + authorPermission: "read", + }); + assert.deepEqual( + failures.map((f) => f.code), + ["wrong_ancestry"], + ); + }); + + it("skips ancestry for push permission but still flags bad description", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: "", + behindMain: 0, + behindBase: 44, + authorPermission: "write", + }); + assert.ok(!failures.some((f) => f.code === "wrong_ancestry")); + assert.ok(failures.some((f) => f.code === "bad_description")); + }); + + it("applies ancestry when permission lookup failed (fail closed)", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + body: [ + "## Summary", + "This change updates the Windows tray launcher so it resolves CODEX_HOME through the shared helper instead of a hardcoded path.", + "", + "## Test plan", + "- Launch the tray app after setting CODEX_HOME", + "- Confirm the listener and launcher use the same workspace root", + ].join("\n"), + behindMain: 0, + behindBase: 44, + authorPermission: null, + permissionLookupFailed: true, + }); + assert.ok(failures.some((f) => f.code === "wrong_ancestry")); + }); +}); +``` + +- [ ] **Step 2: Run tests — expect FAIL (module missing)** + +Run: + +```bash +node --test .github/scripts/pr-quality.test.cjs +``` + +Expected: FAIL — `Cannot find module './pr-quality.cjs'` + +- [ ] **Step 3: Implement `.github/scripts/pr-quality.cjs`** + +```js +"use strict"; + +const path = require("node:path"); +const { + clean, + isPlaceholderOnlyValue, + hasSubstantialStructuredContent, +} = require(path.join(__dirname, "issue-quality.cjs")); + +const ANCESTRY_BEHIND_THRESHOLD = 20; +const MIN_SECTION_LEN = 40; +const MIN_RICH_SECTIONS = 2; +const UNSTRUCTURED_MIN_LEN = 120; +const UNSTRUCTURED_MIN_BLOCKS = 2; + +function isWrongAncestry({ behindMain, behindBase, threshold = ANCESTRY_BEHIND_THRESHOLD }) { + return behindMain === 0 && behindBase >= threshold; +} + +function authorHasPushPermission(permission) { + return permission === "admin" || permission === "maintain" || permission === "write"; +} + +/** + * True when the body uses literal backslash-n as the dominant line break + * (agent bug seen on #644) rather than real newlines. + */ +function hasEscapedNewlines(text) { + const escaped = (text.match(/\\n/g) || []).length; + if (escaped < 2) return false; + const real = (text.match(/\n/g) || []).length; + return escaped > real; +} + +function countContentBlocks(text) { + const blocks = text + .split(/\n\s*\n/) + .map((b) => b.trim()) + .filter(Boolean); + if (blocks.length >= 2) return blocks.length; + const bullets = text + .split("\n") + .map((l) => l.trim()) + .filter((l) => /^[-*+]\s+\S/.test(l)); + return Math.max(blocks.length, bullets.length); +} + +function assessPrDescription(body) { + if (typeof body !== "string" || !body.trim()) { + return { ok: false, reason: "empty" }; + } + if (hasEscapedNewlines(body)) { + return { ok: false, reason: "escaped_newlines" }; + } + const cleaned = clean(body); + if (!cleaned) { + const strippedComments = body.replace(//g, "").trim(); + if (!strippedComments) return { ok: false, reason: "empty" }; + if (isPlaceholderOnlyValue(strippedComments)) { + return { ok: false, reason: "placeholder" }; + } + return { ok: false, reason: "empty" }; + } + if (isPlaceholderOnlyValue(cleaned)) { + return { ok: false, reason: "placeholder" }; + } + if (hasSubstantialStructuredContent(cleaned, MIN_SECTION_LEN, MIN_RICH_SECTIONS)) { + return { ok: true }; + } + if ( + cleaned.length >= UNSTRUCTURED_MIN_LEN && + countContentBlocks(cleaned) >= UNSTRUCTURED_MIN_BLOCKS + ) { + return { ok: true }; + } + return { ok: false, reason: "thin" }; +} + +function collectPrQualityFailures({ + baseRef, + allowedBases, + body, + behindMain, + behindBase, + authorPermission, + permissionLookupFailed = false, +}) { + const failures = []; + const wrongBase = !allowedBases.includes(baseRef); + if (wrongBase) { + failures.push({ code: "wrong_base" }); + } else { + const skipAncestry = + !permissionLookupFailed && authorHasPushPermission(authorPermission); + if (!skipAncestry && isWrongAncestry({ behindMain, behindBase })) { + failures.push({ code: "wrong_ancestry" }); + } + } + + const desc = assessPrDescription(body); + if (!desc.ok) { + failures.push({ code: "bad_description", reason: desc.reason }); + } + return failures; +} + +module.exports = { + ANCESTRY_BEHIND_THRESHOLD, + isWrongAncestry, + authorHasPushPermission, + assessPrDescription, + collectPrQualityFailures, + hasEscapedNewlines, +}; +``` + +- [ ] **Step 4: Run tests — expect PASS** + +```bash +node --test .github/scripts/pr-quality.test.cjs +``` + +Expected: all tests pass. + +- [ ] **Step 5: Wire path filters in `issue-quality-tests.yml`** + +In both `pull_request` and `push` `paths:` lists, add: + +```yaml + - ".github/scripts/pr-quality.cjs" + - ".github/scripts/pr-quality.test.cjs" +``` + +In the test step commands, add: + +```yaml + node --test .github/scripts/pr-quality.test.cjs +``` + +- [ ] **Step 6: Commit** + +```bash +git add .github/scripts/pr-quality.cjs .github/scripts/pr-quality.test.cjs .github/workflows/issue-quality-tests.yml +git commit -m "feat(ci): add pure PR ancestry and description quality checks" +``` + +--- + +### Task 2: Workflow orchestration (checkout + multi-gate) + +**Files:** +- Modify: `.github/workflows/enforce-pr-target.yml` +- Modify: `.github/scripts/enforce-pr-target.test.cjs` + +**Interfaces:** +- Consumes: `collectPrQualityFailures` from `pr-quality.cjs` +- Produces: workflow that on any failure drafts (soft-fail) + upserts multi-section comment + `core.setFailed`; on all-clear restores prior bot draft/title state + +- [ ] **Step 1: Extend static workflow tests (fail until yml updated)** + +Append to `.github/scripts/enforce-pr-target.test.cjs`: + +```js + it("listens for synchronize so rebase can clear ancestry failures", () => { + assert.match(workflow, /synchronize/); + }); + + it("checks out trusted default-branch scripts only (never PR head)", () => { + assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/); + assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.repository\.default_branch\s*\}\}/); + assert.match(workflow, /sparse-checkout:\s*\.github\/scripts/); + assert.match(workflow, /persist-credentials:\s*false/); + assert.doesNotMatch(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/); + }); + + it("loads pr-quality via require from the checked-out scripts", () => { + assert.match(workflow, /pr-quality\.cjs/); + assert.match(workflow, /collectPrQualityFailures/); + }); +``` + +- [ ] **Step 2: Run static test — expect FAIL** + +```bash +node --test .github/scripts/enforce-pr-target.test.cjs +``` + +Expected: FAIL on new assertions (no synchronize / checkout / pr-quality yet). + +- [ ] **Step 3: Rewrite `enforce-pr-target.yml` job steps** + +Replace the single-step job with two steps. Keep the existing GraphQL helpers, comment marker, title prefix, and soft-fail draft pattern from #631. + +1. Add `synchronize` to `on.pull_request_target.types`. +2. First step: trusted checkout + +```yaml + - name: Checkout trusted PR-quality scripts + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + sparse-checkout: .github/scripts +``` + +3. Second step: `github-script` that: + +```js +const path = require("path"); +const { collectPrQualityFailures } = require( + path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), +); +``` + +Then: + +- `pulls.get` for live PR (include `body`, `head.sha`, `user.login`, `base.ref`, `draft`, `title`, `node_id`) +- `repos.getCollaboratorPermissionLevel` — on error set `permissionLookupFailed = true` and warn +- If base allowed: `repos.compareCommitsWithBasehead` for `main...${headSha}` and `${base}...${headSha}`; read `behind_by` +- `failures = collectPrQualityFailures({...})` +- If `failures.length > 0`: + - Upsert one comment listing each failure section (`wrong_base`, `wrong_ancestry`, `bad_description`) + - Title-prefix **only** when `wrong_base` + - Soft-fail `convertToDraft` like #631; checkpoint ownership in comment state + - `core.setFailed(summary)` and return +- If no failures and `storedState?.active`: restore title/ready like #631; success comment +- If no failures and no active state: `core.info` and return + +Accept bot comments containing either `` or `` when locating state. + +Hidden state keeps `version`, `active`, `autoDraftedByBot`, `titlePrefixedByBot`; may add `ancestryFailed` / `descriptionFailed` booleans. + +- [ ] **Step 4: Run static tests — expect PASS** + +```bash +node --test .github/scripts/enforce-pr-target.test.cjs .github/scripts/pr-quality.test.cjs +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add .github/workflows/enforce-pr-target.yml .github/scripts/enforce-pr-target.test.cjs +git commit -m "feat(ci): enforce PR ancestry and description in target gate" +``` + +--- + +### Task 3: Harness + behavioral CI tests + +**Files:** +- Modify: `tests/helpers/enforce-pr-target-harness.ts` +- Modify: `tests/ci-workflows.test.ts` + +**Interfaces:** +- Consumes: workflow script that `require`s `pr-quality.cjs` and calls compare/permission APIs +- Produces: harness options for `body`, compare fixtures, permission; scoped `require` for `.github/scripts/*` only + +- [ ] **Step 1: Extend harness** + +1. Add to `RunOptions`: + - `authorPermission?: string` (default `"read"`) + - `failPermissionLookup?: boolean` + - `compareByBasehead?: Record` +2. Ensure `pr.body` and `pr.head.sha` are present on `pulls.get` payload. +3. DEFAULT_PR must include a **passing** description (two rich sections) and default compares that are **not** wrong ancestry (`main...sha` → `behind_by: 5`, `dev...sha` → `behind_by: 0`). +4. Replace `require: forbidden("require")` with a scoped loader: + +```ts +import { createRequire } from "node:module"; +import path from "node:path"; + +const nodeRequire = createRequire(path.join(process.cwd(), "package.json")); +const scriptsRoot = path.resolve(process.cwd(), ".github", "scripts"); + +function scopedRequire(id: string) { + calls.push({ method: "require", args: [id] }); + const resolved = path.isAbsolute(id) ? id : path.resolve(process.cwd(), id); + if (!resolved.startsWith(scriptsRoot + path.sep) && resolved !== scriptsRoot) { + // Also allow require of files already under scripts via absolute path from path.join + const norm = resolved.replace(/\\/g, "/"); + if (!norm.includes("/.github/scripts/")) { + throw new Error(`the script must not require ${id}`); + } + } + return nodeRequire(resolved); +} +``` + +5. Record `repos.getCollaboratorPermissionLevel` and `repos.compareCommitsWithBasehead` on the fake github client. + +- [ ] **Step 2: Update structural allowlist in `tests/ci-workflows.test.ts`** + +- Steps length **2**: checkout then github-script +- Pin checkout SHA `11bd71901bbe5b1630ceea73d27597364c9af683`, default_branch ref, `persist-credentials: false`, sparse `.github/scripts`, no PR head ref +- Types sorted: `edited`, `opened`, `ready_for_review`, `reopened`, `synchronize` +- Keep permissions object unchanged +- Keep “no `${{` in script” assertion on the github-script step only + +- [ ] **Step 3: Add behavioral scenarios** + +1. **Ancestry fail (#644):** base `dev`, permission `read`, `main...sha` behind 0 / `dev...sha` behind 44, good body → `setFailed`, draft attempted, ancestry in comment, **no** title prefix. +2. **Maintainer ancestry skip:** permission `write`, same compares, good body → no `setFailed`. +3. **Empty description:** good ancestry, `body: ""` → `setFailed` + draft. +4. **Escaped newlines:** body with literal `\\n` → `setFailed`. +5. **Clear after active:** prior bot state `active` + `autoDraftedByBot`, now good → mark ready, no `setFailed`. +6. Existing wrong-base scenarios still pass (title prefix + setFailed); give them a good body so description does not double-fail unless intended. + +- [ ] **Step 4: Run focused tests** + +```bash +node --test .github/scripts/pr-quality.test.cjs .github/scripts/enforce-pr-target.test.cjs +bun test tests/ci-workflows.test.ts +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add tests/helpers/enforce-pr-target-harness.ts tests/ci-workflows.test.ts +git commit -m "test(ci): cover PR ancestry and description enforcement paths" +``` + +--- + +### Task 4: Docs and agent policy notes + +**Files:** +- Modify: `docs-site/src/content/docs/contributing.md` +- Modify: `AGENTS.md` +- Modify: `MAINTAINERS.md` + +- [ ] **Step 1: Contributing (English)** + +Add a short **Pull requests** subsection: + +- Target `dev` (or `dev2-go` only for scoped Go work); never open ordinary PRs at `main`. +- Branch from current `dev`, not from `main`. CI rejects heads that sit on the `main` tip while far behind the PR base (the #644 failure mode). +- Include a real description (Summary + Test plan, or equivalent substance). Empty, placeholder-only, or escaped-`\n` bodies fail the required `enforce-target` check. +- Note that `pull_request_target` workflow updates apply after promotion to the repository default branch (same ops caveat as #631). + +Do not bulk-edit ja/ko/ru/zh-cn in this task. + +- [ ] **Step 2: AGENTS.md / MAINTAINERS.md** + +One sentence each: CI rejects main-based ancestry into `dev`/`dev2-go` and empty/thin/malformed PR descriptions; push-permission authors skip the ancestry heuristic only. + +- [ ] **Step 3: Commit** + +```bash +git add docs-site/src/content/docs/contributing.md AGENTS.md MAINTAINERS.md +git commit -m "docs: document PR ancestry and description quality gates" +``` + +--- + +### Task 5: Final validation + +- [ ] **Step 1: Run required gates** + +```bash +node --test .github/scripts/pr-quality.test.cjs .github/scripts/enforce-pr-target.test.cjs +bun test tests/ci-workflows.test.ts +bun run typecheck +bun run privacy:scan +``` + +Expected: all PASS. + +- [ ] **Step 2: Docs-site build** + +```bash +cd docs-site && bun install --frozen-lockfile && bun run build +``` + +Expected: build succeeds. + +- [ ] **Step 3: Diff review** + +Confirm no unrelated files; no PR-head checkout; title prefix only on wrong base; description still enforced for maintainers. + +--- + +## Spec coverage checklist + +| Spec requirement | Task | +| --- | --- | +| Wrong ancestry rule + threshold 20 | Task 1 | +| Maintainer push escape hatch; fail-closed permission | Task 1–3 | +| Description option 2 | Task 1 | +| Collect all failures; draft + setFailed | Task 2–3 | +| No `[WRONG BRANCH]` for ancestry/description | Task 2–3 | +| `synchronize` trigger | Task 2–3 | +| Trusted checkout only | Task 2–3 | +| Unit + harness + ci-workflows tests | Task 1–3, 5 | +| Contributing + AGENTS/MAINTAINERS | Task 4 | +| Default-branch promotion ops note | Task 4 | + +## Placeholder / consistency self-review + +- No TBD/TODO left in steps. +- `collectPrQualityFailures` / `assessPrDescription` names consistent across tasks. +- Checkout action SHA matches issue-quality (`11bd7190…`). +- DEFAULT_PR body/compares updated so legacy harness scenarios stay green. diff --git a/docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md b/docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md new file mode 100644 index 000000000..a5f00449b --- /dev/null +++ b/docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md @@ -0,0 +1,173 @@ +# PR quality gates (ancestry + description) — Design + +**Date:** 2026-07-28 +**Status:** Approved (brainstorm) +**Related:** #631 (wrong-base enforcer), #644 (motivating bad PR), `AGENTS.md` / `MAINTAINERS.md` branch table +**Branch base for implementation:** tip of #631 (`fix/enforce-pr-target-draft-fallback`) or `dev` after #631 merges + +## Problem + +`enforce-pr-target` only rejects wrong **base** refs (`main`, etc.). It does not catch: + +1. **Wrong ancestry** — head branched from `main` (or another release tip) while targeting `dev` / `dev2-go`, so the PR diff dumps already-released or unrelated commits into the integration branch (seen on #644: 0 behind `main`, 44 behind `dev`). +2. **Empty or low-quality descriptions** — blank bodies, comment-only bodies (e.g. only CodeRabbit release notes), placeholder-only text, literal `\n` escapes instead of real newlines, or thin bodies that lack a minimum “what/why” structure (option 2 from brainstorm). + +Wrong-base UX already drafts the PR, comments, and `setFailed`s the required check. New gates should reuse that pattern without overloading the `[WRONG BRANCH]` title prefix. + +## Goals + +- Fail the required `enforce-target` check when ancestry or description is unacceptable. +- Convert ready PRs to draft (soft-fail GraphQL, same as #631) and leave a single bot comment listing **all** open violations. +- Clear draft/comment/`setFailed` only when every gate passes (including after `synchronize` / body edits). +- Keep `pull_request_target` safe: **no checkout of PR head**; GitHub compare/API only; pure validators unit-tested offline. +- Escape hatch for maintainers with repo `push` so intentional release / promotion work is not blocked by the ancestry heuristic. + +## Non-goals + +- Rejecting local machine paths (`E:\…`), “tests not run” admissions, or forcing a fixed PR template (deferred). +- Auto-retargeting or auto-rebasing contributor branches. +- Retitling with `[WRONG BRANCH]` for ancestry/description (prefix stays wrong-**base** only). +- Blocking Dependabot / GitHub App bots beyond what the existing workflow already does (document any bot skips if added). + +## Approach + +**Extend the existing enforcer (Approach A):** extract pure checks into `.github/scripts/pr-quality.cjs`, orchestrate from `enforce-pr-target.yml` (or a thin shared runner), reuse draft/comment/`setFailed` state machine. + +Rejected alternatives: soft gate only (B — weaker); separate workflow (C — duplicated draft/comment machinery). + +## Gate composition + +Evaluate in order; collect **all** failures before mutating: + +1. **Wrong base** (existing) — `base ∉ {dev, dev2-go}` +2. **Wrong ancestry** (new) — when base is allowed +3. **Bad description** (new) — always when base is allowed (and optionally also when base is wrong, so authors fix body while retargeting; **default: run description whenever we have a PR body**, independent of ancestry) + +If any failure is active: + +- Upsert one bot comment (existing marker family, extended state) listing every open issue. +- Draft the PR if ready (soft-fail). +- `core.setFailed` with a concise summary (required check red). + +If previously active and now all clear: restore ready only if bot drafted; update comment to success; do not leave `setFailed`. + +## Wrong ancestry + +### Inputs (API only) + +For `base ∈ {dev, dev2-go}` and head SHA `H`: + +- `GET /repos/{owner}/{repo}/compare/main...{H}` → `{ ahead_by, behind_by }` +- `GET /repos/{owner}/{repo}/compare/{base}...{H}` → `{ ahead_by, behind_by }` + +No `actions/checkout` of the PR head. + +### Rule + +Flag **wrong ancestry** when: + +```text +behind_main === 0 +AND behind_base >= ANCESTRY_BEHIND_THRESHOLD # default 20 +``` + +This matches #644 (`behind_main = 0`, `behind_dev = 44`). + +Optional refinement (not required for v1): also require `ahead_main <= ahead_base` so a long-lived fork that somehow sits on `main` tip but is not dumping main-only commits is less likely to false-positive. Prefer shipping the two-clause rule first with tests against recorded compare fixtures. + +### Escape hatch + +Skip the ancestry gate when the PR author has **push** permission on the base repository (`GET /repos/{owner}/{repo}/collaborators/{login}/permission` → `admin` | `maintain` | `write`). Contributors / fork authors without push remain gated. + +Do **not** skip description quality for maintainers (empty/bad bodies remain rejected). + +### Threshold + +`ANCESTRY_BEHIND_THRESHOLD = 20` constant in the script. Document in comment why (tolerant of slightly stale `dev` forks; catches “branched from current main”). + +## Description quality (option 2) + +### Normalize body + +1. Strip HTML comments (``), including CodeRabbit release-notes blocks. +2. Trim; treat placeholder-only whole body / lines like issue-quality (`N/A`, `TODO`, `No response`, …) as empty. +3. Detect **escaped newlines**: if the cleaned body contains few or no real `\n` characters but contains the two-character sequence `\` + `n` (or `\` + `r` + `\` + `n`) as a dominant separator, classify as **malformed** (fail). #644’s API body showed literal `\n` sequences. + +### Accept when (after normalize) + +**Substantial structured content**, either: + +- **Structured path:** ≥ 2 markdown sections (h2–h4) whose cleaned text is each ≥ 40 characters, **or** +- **Unstructured path:** cleaned body length ≥ 120 characters **and** at least 2 bullets and/or paragraph breaks (real newlines separating non-empty blocks). + +### Reject when + +| Condition | Code / message key | +| --- | --- | +| Empty after strip | `empty` | +| Placeholder-only | `placeholder` | +| Escaped-newline malformed | `escaped_newlines` | +| Not substantial by either path | `thin` | + +Reuse helpers from `issue-quality.cjs` where practical (placeholder / section richness), or duplicate minimal copies to avoid coupling PR and issue workflows if import paths are awkward in Actions. Prefer shared tiny helpers over drift. + +## Workflow / UX + +### Triggers + +Extend `pull_request_target` types with **`synchronize`** so a rebase onto `dev` re-evaluates ancestry and can clear the failure. Keep: `opened`, `reopened`, `edited`, `ready_for_review`. + +### Comment shape + +Single bot comment (existing `` marker **or** rename to a neutral `` with migration: accept either marker when finding the comment). Body sections: + +- Wrong target branch (existing copy) +- Wrong branch ancestry (rebase onto current `base`; do not open from `main`) +- Pull request description (what failed + what “good enough” means) + +Hidden JSON state extended with flags such as `ancestryFailed`, `descriptionFailed`, plus existing `autoDraftedByBot` / `titlePrefixedByBot` / `active`. + +Title prefix `[WRONG BRANCH]` remains **only** for wrong base. + +### Permissions + +Unchanged from #631: `contents: write` + `pull-requests: write` for draft GraphQL; still no untrusted checkout. + +## Testing + +| Layer | Coverage | +| --- | --- | +| `.github/scripts/pr-quality.test.cjs` | Pure rules: #644-like compare fixture fails ancestry; behind_main>0 passes; maintainer skip N/A at pure layer; empty / comment-only / escaped `\n` / thin / good structured / good unstructured bodies | +| `enforce-pr-target.test.cjs` / harness | Workflow wires `synchronize`; calls quality module; `setFailed` when ancestry or description fails; soft-fail draft still; no checkout | +| `tests/ci-workflows.test.ts` | Permissions + trigger types stay in sync | + +Offline fixtures only — no live GitHub in unit tests. + +## Docs / policy + +- Short note in contributing docs (docs-site) when user-facing: PRs must target `dev`, be based on current `dev` (not `main`), and include a real description. +- `AGENTS.md` / `MAINTAINERS.md` already define branch targets; add one sentence that CI rejects main-based ancestry and empty/thin PR bodies once shipped. +- Reminder: `pull_request_target` only picks up workflow changes after promotion to the **default branch** (and typically `dev` for fork PR path consistency) — same ops note as #631. + +## Security + +- No execution of PR head code. +- Compare API uses base-repo token; head SHA is attacker-influenced only as an opaque ref for compare (GitHub-side). Do not interpolate head ref into shell. +- Collaborator permission lookup is read-only; failure of that API should **fail closed for ancestry** (treat as non-maintainer) or soft-skip with warning — choose **fail closed** (apply ancestry gate) to avoid accidental bypass. + +## Rollout + +1. Land #631 if not already merged. +2. Implement this design on a follow-up branch from #631 tip / `dev`. +3. After merge + default-branch promotion, verify on a synthetic fork PR (main-based head → `dev`, empty body) that draft + red check appear, then fix body + rebase and confirm clear. + +## Open constants (locked for v1) + +| Name | Value | +| --- | --- | +| `ANCESTRY_BEHIND_THRESHOLD` | `20` | +| Min section length | `40` | +| Min rich sections | `2` | +| Unstructured min length | `120` | +| Unstructured min blocks | `2` | +| Release compare ref | `main` | diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index ac30bb7fe..95701c84a 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -400,6 +400,46 @@ describe("GitHub Actions hardening", () => { return { workflow, jobs, steps: steps!, allSteps, script }; } + const SCRIPT_LOAD = ["require", "require"] as const; + + /** Reads every allowed-base PR performs before any enforcement writes. */ + function readsAllowedBase(tail: string[] = []): string[] { + return [ + ...SCRIPT_LOAD, + "pulls.get", + "issues.listComments", + "repos.getCollaboratorPermissionLevel", + "repos.compareCommitsWithBasehead", + "repos.compareCommitsWithBasehead", + ...tail, + ]; + } + + /** Reads for a PR whose base is outside the allow-list (no ancestry compares). */ + function readsWrongBase(tail: string[] = []): string[] { + return [ + ...SCRIPT_LOAD, + "pulls.get", + "issues.listComments", + "repos.getCollaboratorPermissionLevel", + ...tail, + ]; + } + + /** Like readsAllowedBase but with a second listComments page from paginate. */ + function readsAllowedBasePaged(tail: string[] = []): string[] { + return [ + ...SCRIPT_LOAD, + "pulls.get", + "issues.listComments", + "issues.listComments", + "repos.getCollaboratorPermissionLevel", + "repos.compareCommitsWithBasehead", + "repos.compareCommitsWithBasehead", + ...tail, + ]; + } + /** * Drop JavaScript comments so a commented-out call cannot satisfy a "calls X" * assertion. Both forms matter: an audit round removed the draft conversion @@ -522,22 +562,36 @@ describe("GitHub Actions hardening", () => { expect(Object.keys(job).sort()).toEqual(["runs-on", "steps"]); expect(job["runs-on"]).toBe("ubuntu-latest"); - // Exactly one step: name, the pinned action, and its inputs. Anything more - // is an extra privileged action nobody reviewed — the audit added a second - // `github-script` step that mutated the PR and the suite stayed green. - expect(steps).toHaveLength(1); - const [step] = steps as [WorkflowStep]; - expect(Object.keys(step).sort()).toEqual(["name", "uses", "with"]); + // Checkout trusted scripts, then run the gate. Anything more is an extra + // privileged action nobody reviewed. + expect(steps).toHaveLength(2); + const [checkout, scriptStep] = steps as [WorkflowStep, WorkflowStep]; + expect(Object.keys(checkout).sort()).toEqual(["name", "uses", "with"]); + expect(checkout.uses).toBe( + "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683", + ); + expect(Object.keys(checkout.with ?? {}).sort()).toEqual([ + "persist-credentials", + "ref", + "sparse-checkout", + ]); + expect(checkout.with).toEqual({ + ref: "${{ github.event.repository.default_branch }}", + "persist-credentials": false, + "sparse-checkout": ".github/scripts", + }); + + expect(Object.keys(scriptStep).sort()).toEqual(["name", "uses", "with"]); // `github-script` is the action, pinned to a 40-hex commit SHA: this // workflow hands a write token to whatever the ref resolves to, so a tag or // branch is a mutable dependency. - expect(step.uses).toMatch(/^actions\/github-script@[0-9a-f]{40}$/); + expect(scriptStep.uses).toMatch(/^actions\/github-script@[0-9a-f]{40}$/); // `script` is the only input. `github-token:` swaps the restricted // `GITHUB_TOKEN` for an arbitrary PAT, and every other input changes how the // action behaves with that token in hand. - expect(Object.keys(step.with ?? {})).toEqual(["script"]); + expect(Object.keys(scriptStep.with ?? {})).toEqual(["script"]); }); /** @@ -551,10 +605,9 @@ describe("GitHub Actions hardening", () => { * everything it needs from the `context` object at runtime. */ test("PR target enforcement's script interpolates nothing from the event", async () => { - const { workflow } = await readEnforcePrTarget(); - const rawScript = String( - (workflow.jobs?.["enforce-target"]?.steps?.[0]?.with?.script as string) ?? "", - ); + const { steps } = await readEnforcePrTarget(); + const scriptStep = steps.find(step => typeof step.with?.script === "string"); + const rawScript = String(scriptStep?.with?.script ?? ""); expect(rawScript).not.toContain("${{"); }); @@ -565,20 +618,24 @@ describe("GitHub Actions hardening", () => { // the draft when someone undoes it by hand. Dropping either silently makes // the gate one-shot. const types = workflow.on?.pull_request_target?.types ?? []; - expect([...types].sort()).toEqual(["edited", "opened", "ready_for_review", "reopened"]); + expect([...types].sort()).toEqual([ + "edited", + "opened", + "ready_for_review", + "reopened", + "synchronize", + ]); - // The verdict is a live base-branch comparison, not a cached one: re-read the - // PR, then derive wrongBase from it. Pinning the comparison itself stops a - // rewrite that leaves the constant in place while hard-coding the answer. + // The verdict is a live PR read plus ancestry/description checks. expect(script).toContain("github.rest.pulls.get"); + expect(script).toContain("collectPrQualityFailures"); + expect(script).toContain("github.rest.repos.getCollaboratorPermissionLevel"); + expect(script).toContain("github.rest.repos.compareCommitsWithBasehead"); // The allow-list is the gate's whole policy, so it is pinned by value and // not just by shape: a widened list is the one edit that opens every base // at once while every behavioural scenario below still passes. expect(script).toMatch(/const ALLOWED_BASES = \["dev", "dev2-go"\];/); expect(script).toMatch(/const DEFAULT_BASE = "dev";/); - expect(script).toMatch( - /const wrongBase = !ALLOWED_BASES\.includes\(pr\.base\.ref\);/, - ); // Every mutation targets the PR the event fired for. `pull_number` is the // only handle the script has, and an audit round repointed it at @@ -653,6 +710,7 @@ describe("GitHub Actions hardening", () => { expect(callArgs("github.rest.pulls.update")).toEqual([ ["owner", "pull_number", "repo", "title"], ["owner", "pull_number", "repo", "title"], + ["owner", "pull_number", "repo", "title"], ]); // Both comment writes address the PR being enforced, by its own number. @@ -672,7 +730,13 @@ describe("GitHub Actions hardening", () => { // nobody reviewed. const restWrites = [...script.matchAll(/github\.rest\.[\w.]+/g)] .map(match => match[0]) - .filter(name => !name.endsWith(".get") && !name.endsWith(".listComments")); + .filter( + name => + !name.endsWith(".get") && + !name.endsWith(".listComments") && + name !== "github.rest.repos.getCollaboratorPermissionLevel" && + name !== "github.rest.repos.compareCommitsWithBasehead", + ); expect([...new Set(restWrites)].sort()).toEqual([ "github.rest.issues.createComment", "github.rest.issues.updateComment", @@ -720,14 +784,15 @@ describe("GitHub Actions hardening", () => { } const BOT = "github-actions[bot]"; - const MARKER = ""; + const MARKER = ""; + const LEGACY_MARKER = ""; function botComment(state: Record, title = "Add a thing") { return { id: 7, user: { login: BOT }, body: [ - MARKER, + LEGACY_MARKER, ``, `about ${title}`, ].join("\n"), @@ -738,8 +803,8 @@ describe("GitHub Actions hardening", () => { const result = await run({ pr: { base: { ref: "dev" } } }); // Reads only. If a rewrite adds a write here, it appears in this list. - expect(methodsOf(result)).toEqual(["pulls.get", "issues.listComments"]); - expect(result.logs.join(" ")).toContain("Target branch is correct"); + expect(methodsOf(result)).toEqual(readsAllowedBase()); + expect(result.logs.join(" ")).toContain("All PR quality gates passed"); }); test("a PR targeting dev2-go is left completely alone, exactly like dev", async () => { @@ -749,12 +814,95 @@ describe("GitHub Actions hardening", () => { // line existed but nothing could land on it. const result = await run({ pr: { base: { ref: "dev2-go" }, title: "Port the runtime entry", draft: false } }); - expect(methodsOf(result)).toEqual(["pulls.get", "issues.listComments"]); - expect(result.logs.join(" ")).toContain("Target branch is correct"); + expect(methodsOf(result)).toEqual(readsAllowedBase()); + expect(result.logs.join(" ")).toContain("All PR quality gates passed"); // No comment either. Silence is the acceptance signal. expect(callsTo(result, "issues.createComment")).toEqual([]); }); + const HEAD_SHA = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + const ANCESTRY_FAIL_COMPARES = { + [`main...${HEAD_SHA}`]: { ahead_by: 1, behind_by: 0 }, + [`dev...${HEAD_SHA}`]: { ahead_by: 0, behind_by: 44 }, + } as const; + + test("wrong ancestry on dev fails without title prefix (#644)", async () => { + const result = await run({ + pr: { base: { ref: "dev" } }, + authorPermission: "read", + compareByBasehead: ANCESTRY_FAIL_COMPARES, + }); + + expect(callsTo(result, "pulls.update")).toEqual([]); + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "issues.createComment", + "issues.updateComment", + "graphql", + "issues.updateComment", + "issues.updateComment", + ])); + const commentBody = lastEnforcerCommentBody(result); + expect(commentBody).toContain("Wrong branch ancestry"); + expect(commentBody).not.toContain("Wrong target branch"); + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); + expect(result.warnings.some((w) => w.includes("wrong ancestry"))).toBe(true); + }); + + test("maintainers skip ancestry enforcement with the same compares", async () => { + const result = await run({ + pr: { base: { ref: "dev" } }, + authorPermission: "write", + compareByBasehead: ANCESTRY_FAIL_COMPARES, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase()); + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); + }); + + test("empty PR description fails and drafts", async () => { + const result = await run({ pr: { base: { ref: "dev" }, body: "" } }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); + expect(lastEnforcerCommentBody(result)).toContain("Pull request description"); + expect(lastEnforcerCommentBody(result)).toContain("body is empty"); + expect(callsTo(result, "graphql")).toHaveLength(1); + }); + + test("literal backslash-n in the body fails the description gate", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + body: "## Summary\\n\\nThis uses escaped newlines instead of real breaks.\\n\\n## Test plan\\n\\nAlso escaped here.", + }, + }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); + expect(lastEnforcerCommentBody(result)).toContain("literal `\\n` escape sequences"); + }); + + test("clears prior bot state when every gate passes again", async () => { + const result = await run({ + pr: { base: { ref: "dev" }, draft: true }, + comments: [botComment({ + version: 1, + active: true, + autoDraftedByBot: true, + titlePrefixedByBot: false, + ancestryFailed: true, + descriptionFailed: true, + })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "graphql", + "issues.updateComment", + ])); + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); + const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; + expect(cleared.body).toContain('"active":false'); + expect(cleared.body).toContain("PR quality gates passed"); + }); + test("every base outside the allow-list is still blocked", async () => { // Widening a list is a one-token edit, and the danger is widening it too // far. These four are the bases a contributor actually reaches for: @@ -763,15 +911,14 @@ describe("GitHub Actions hardening", () => { for (const ref of ["main", "master", "preview", "feature/x"]) { const result = await run({ pr: { base: { ref }, title: "Add a thing", draft: false } }); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); expect(lastEnforcerCommentBody(result)).toContain(`\`${ref}\``); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); } @@ -788,13 +935,11 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", - ]); + ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Port the runtime entry" }, ]); @@ -814,15 +959,14 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: false, autoDraftedByBot: false, titlePrefixedByBot: false })], }); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.updateComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Port the runtime entry" }, ]); @@ -852,18 +996,17 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "main" }, title: "Add a thing", draft: false }, }); - // Pending ownership comment first, then title/draft, then checkpoints so - // a mid-run crash still records ownership and autoDraftedByBot only after - // convertToDraft succeeds (#626 / Codex review on #631). - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + // Pending ownership first, then title prefix, then claim autoDraftedByBot and + // checkpoint before convertToDraft so a successful convert followed by a + // failed comment still restores later. + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); // The title update carries the title and nothing else. `base`, `state` // and `body` are all accepted by this endpoint; an audit round added @@ -898,13 +1041,11 @@ describe("GitHub Actions hardening", () => { // No draft conversion: it is already a draft. Pending ownership first, // then title prefix, then final explanation. State records that the bot // did not draft — which stops restore from marking it ready. - expect(methodsOf(wrong)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(wrong)).toEqual(readsWrongBase([ "issues.createComment", "pulls.update", "issues.updateComment", - ]); + ])); expect(lastEnforcerCommentBody(wrong)).toContain('"autoDraftedByBot":false'); expect(wrong.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); @@ -915,12 +1056,10 @@ describe("GitHub Actions hardening", () => { }); // The prefix comes off; the draft stays. No GraphQL at all. - expect(methodsOf(restored)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(restored)).toEqual(readsAllowedBase([ "pulls.update", "issues.updateComment", - ]); + ])); expect(callsTo(restored, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); @@ -932,13 +1071,11 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", - ]); + ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); @@ -950,7 +1087,7 @@ describe("GitHub Actions hardening", () => { const [update] = callsTo(result, "issues.updateComment") as [{ comment_id: number; body: string }]; expect(update.comment_id).toBe(7); expect(update.body).toContain('"active":false'); - expect(update.body).toContain("Target branch corrected"); + expect(update.body).toContain("PR quality gates passed"); }); test("only this workflow's own prefix is removed, not a contributor's edits", async () => { @@ -971,12 +1108,10 @@ describe("GitHub Actions hardening", () => { }); // The comment is refreshed (pending + final); title and draft are already right. - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.updateComment", "issues.updateComment", - ]); + ])); }); test("the verdict comes from the fetched PR, not the stale event payload", async () => { @@ -992,15 +1127,14 @@ describe("GitHub Actions hardening", () => { // Exact equality, not `toContain`. An audit round hung an extra // `github.request("POST /repos/attacker/other/issues", …)` off precisely // this path because it was the one scenario asserting loosely. - expect(methodsOf(wentWrong)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(wentWrong)).toEqual(readsWrongBase([ "issues.createComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); expect(callsTo(wentWrong, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, ]); @@ -1010,7 +1144,7 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "dev" } }, eventPayload: { base: { ref: "main" } }, }); - expect(methodsOf(wasFixed)).toEqual(["pulls.get", "issues.listComments"]); + expect(methodsOf(wasFixed)).toEqual(readsAllowedBase()); }); test("what the comment tells the author is the live base, not the event's", async () => { @@ -1061,14 +1195,11 @@ describe("GitHub Actions hardening", () => { // Found it: the prefix comes off, the PR is marked ready, and the // existing comment is edited rather than duplicated. - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsAllowedBasePaged([ "pulls.update", "graphql", "issues.updateComment", - ]); + ])); expect(callsTo(result, "issues.createComment")).toEqual([]); }); @@ -1088,15 +1219,14 @@ describe("GitHub Actions hardening", () => { // Enforcement still happens, and the unreadable comment is repaired in // place rather than duplicated. - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.updateComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); expect(result.warnings.join(" ")).toContain("Could not parse stored workflow state"); }); @@ -1118,12 +1248,10 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "main" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment(noRecordedChanges)], }); - expect(methodsOf(stillWrong)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(stillWrong)).toEqual(readsWrongBase([ "issues.updateComment", "issues.updateComment", - ]); + ])); // Corrected: nothing to undo, but the state must still be cleared or the // next wrong-target event resumes from a stale record. @@ -1131,14 +1259,10 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment(noRecordedChanges)], }); - expect(methodsOf(corrected)).toEqual([ - "pulls.get", - "issues.listComments", - "issues.updateComment", - ]); + expect(methodsOf(corrected)).toEqual(readsAllowedBase(["issues.updateComment"])); const [cleared] = callsTo(corrected, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); - expect(cleared.body).toContain("Target branch corrected"); + expect(cleared.body).toContain("PR quality gates passed"); }); test("a PR undrafted by hand before the retarget still gets its state cleared", async () => { @@ -1153,12 +1277,10 @@ describe("GitHub Actions hardening", () => { }); // Nothing to un-draft, the prefix comes off, and the state is cleared. - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "issues.updateComment", - ]); + ])); const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); }); @@ -1173,12 +1295,10 @@ describe("GitHub Actions hardening", () => { }); expect(callsTo(result, "pulls.update")).toEqual([]); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsAllowedBase([ "graphql", "issues.updateComment", - ]); + ])); }); test("the explanation tells the contributor what to do and where to read", async () => { @@ -1257,13 +1377,11 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment(active)], }); - expect(methodsOf(restored)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(restored)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", - ]); + ])); expect(callsTo(restored, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); @@ -1278,15 +1396,14 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "main" }, draft: false, title: "Add a thing" }, comments: [botComment(active)], }); - expect(methodsOf(wrong)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(wrong)).toEqual(readsWrongBase([ "issues.updateComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); expect(lastEnforcerCommentBody(wrong)).toContain(`"version":${version}`); expect(lastEnforcerCommentBody(wrong)).toContain('"active":true'); expect(wrong.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); @@ -1310,13 +1427,11 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment({ version: 1, active: "true", autoDraftedByBot: 1, titlePrefixedByBot: "yes" })], }); - expect(methodsOf(loose)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(loose)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", - ]); + ])); expect(callsTo(loose, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); @@ -1327,28 +1442,26 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: null, titlePrefixedByBot: 0 })], }); - expect(methodsOf(falsy)).toEqual([ - "pulls.get", - "issues.listComments", - "issues.updateComment", - ]); + expect(methodsOf(falsy)).toEqual(readsAllowedBase(["issues.updateComment"])); const [cleared] = callsTo(falsy, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); }); test("ownership comment is checkpointed before mutations and finalized after", async () => { - // Pending ownership is written before title/draft so a crash mid-mutation - // still records what the bot owns. autoDraftedByBot is only true in the - // final body after convertToDraft succeeds (#626 / #631). + // Ownership is written before title/draft. autoDraftedByBot is claimed and + // checkpointed before convertToDraft so a successful convert followed by a + // failed comment still restores later. const result = await run({ pr: { base: { ref: "main" }, draft: false } }); const methods = methodsOf(result); const pending = methods.indexOf("issues.createComment"); const title = methods.indexOf("pulls.update"); + const draftClaim = methods.indexOf("issues.updateComment"); const draft = methods.indexOf("graphql"); const finalUpdate = methods.lastIndexOf("issues.updateComment"); expect(pending).toBeGreaterThan(-1); expect(pending).toBeLessThan(title); - expect(title).toBeLessThan(draft); + expect(title).toBeLessThan(draftClaim); + expect(draftClaim).toBeLessThan(draft); expect(draft).toBeLessThan(finalUpdate); expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); }); @@ -1365,14 +1478,13 @@ describe("GitHub Actions hardening", () => { // Already prefixed, so no title write — but pending/draft/final still run. expect(callsTo(result, "pulls.update")).toEqual([]); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); }); @@ -1388,13 +1500,11 @@ describe("GitHub Actions hardening", () => { expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] " }, ]); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", "pulls.update", "issues.updateComment", - ]); + ])); }); test("an already-prefixed title is never prefixed twice", async () => { @@ -1412,12 +1522,10 @@ describe("GitHub Actions hardening", () => { // Asserting only the absent write would let an early return keyed on the // doubled prefix pass, since that skips the write too. expect(callsTo(result, "pulls.update")).toEqual([]); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", "issues.updateComment", - ]); + ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"active":true'); }); @@ -1433,14 +1541,13 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "main" }, title: "[WRONG BRANCH] [WRONG BRANCH] mine", draft: false }, }); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", + "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", - ]); + ])); // Already prefixed by the `startsWith` test, so no third prefix is added. expect(callsTo(result, "pulls.update")).toEqual([]); expect(lastEnforcerCommentBody(result)).toContain('"active":true'); @@ -1470,13 +1577,11 @@ describe("GitHub Actions hardening", () => { // The first comment's state is the one honoured: it says the bot // prefixed and drafted, so both are undone. - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", - ]); + ])); // And the first comment is the one rewritten, not the second. const [updated] = callsTo(result, "issues.updateComment") as [{ comment_id: number }]; expect(updated.comment_id).toBe(7); @@ -1592,7 +1697,7 @@ describe("GitHub Actions hardening", () => { expect(probe["core.isDebug"]).toBe(false); // And the normal path is unaffected by the probe scenario. - expect(methodsOf(result)).toEqual(["pulls.get", "issues.listComments"]); + expect(methodsOf(result)).toEqual(readsAllowedBase()); }); test("the harness runs the Node major the pinned action declares", async () => { @@ -1638,14 +1743,13 @@ describe("GitHub Actions hardening", () => { failOn: ["graphql"], failStatus: status, }); - expect(methodsOf(result)).toEqual([ - "pulls.get", - "issues.listComments", + expect(methodsOf(result)).toEqual(readsWrongBase([ "issues.createComment", "pulls.update", + "issues.updateComment", "graphql", "issues.updateComment", - ]); + ])); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain('"autoDraftedByBot":false'); expect(commentBody).toContain("Automatic draft conversion failed"); @@ -1746,11 +1850,11 @@ describe("GitHub Actions hardening", () => { // helpers and both state fields were still textually present. Presence of a // call proves nothing about whether it can be reached. expect(script).toMatch(/\n\s*if \(!storedState\?\.active\) \{\n/); - expect(script).toMatch(/\n\s*if \(wrongBase\) \{\n/); + expect(script).toMatch(/\n\s*if \(failures\.length > 0\) \{\n/); // Pending ownership is written before mutations; convertToDraft runs next; // a later upsertComment records autoDraftedByBot only after success (#631). - const branchStart = script.indexOf("if (wrongBase) {"); + const branchStart = script.indexOf("if (failures.length > 0) {"); expect(branchStart).toBeGreaterThan(-1); const branch = script.slice(branchStart); const pendingWriteIndex = branch.indexOf("await upsertComment("); diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 4fee80220..e6bdf879a 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -15,6 +15,9 @@ * either way. */ +import { createRequire } from "node:module"; +import path from "node:path"; + export type RecordedCall = { method: string; args: unknown }; export type HarnessResult = { @@ -39,6 +42,7 @@ export type PullRequestState = { number?: number; node_id?: string; title?: string; + body?: string; draft?: boolean; base?: { ref: string }; user?: { login: string }; @@ -79,6 +83,12 @@ export type RunOptions = { * exercise that branch. */ failStatus?: number; + /** Collaborator permission returned by `getCollaboratorPermissionLevel`. */ + authorPermission?: string; + /** When true, permission lookup rejects like a transient API failure. */ + failPermissionLookup?: boolean; + /** Overrides for `compareCommitsWithBasehead` keyed by `basehead`. */ + compareByBasehead?: Record; }; /** @@ -89,11 +99,23 @@ export type RunOptions = { * `context.payload.pull_request.head.sha` to tell the two apart. The extra * fields are inert to the logic and load-bearing for fidelity. */ +const DEFAULT_BODY = [ + "## Summary", + "", + "This change adds enough substantive detail for reviewers to understand the motivation and approach taken.", + "", + "## Test plan", + "", + "- [x] Run `bun test tests/ci-workflows.test.ts`", + "- [x] Confirm enforce-pr-target behaviour locally", +].join("\n"); + const DEFAULT_PR = { number: 42, node_id: "PR_kwDOnode42", id: 1122334455, title: "Add a thing", + body: DEFAULT_BODY, draft: false, state: "open", merged: false, @@ -223,6 +245,7 @@ function octokitError(method: string, status: number): Error & { status: number } function nodeLikeProcess(): Record { + const workspace = process.cwd(); return { platform: "linux", arch: "x64", @@ -259,7 +282,7 @@ function nodeLikeProcess(): Record { GITHUB_SERVER_URL: "https://github.com", GITHUB_SHA: "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", GITHUB_WORKFLOW: "Enforce PR target branch", - GITHUB_WORKSPACE: "/home/runner/work/opencodex/opencodex", + GITHUB_WORKSPACE: workspace, HOME: "/home/runner", RUNNER_ARCH: "X64", RUNNER_NAME: "GitHub Actions 1", @@ -270,7 +293,7 @@ function nodeLikeProcess(): Record { ACTIONS_RUNTIME_URL: "https://pipelines.actions.githubusercontent.com/", }, argv: ["/usr/bin/node", "/home/runner/work/_actions/actions/github-script/dist/index.js"], - cwd: () => "/home/runner/work/opencodex/opencodex", + cwd: () => workspace, exit: () => { throw new Error("the script must not call process.exit"); }, }; } @@ -375,6 +398,11 @@ export async function runEnforcePrTarget( const outputs: { name: string; value: unknown }[] = []; const states = new Map(); const failOn = new Set(options.failOn ?? []); + if (options.failPermissionLookup) { + // Route through `record` so the call appears in the recording even when it + // rejects (same semantics as `failOn`). + failOn.add("repos.getCollaboratorPermissionLevel"); + } const failStatus = options.failStatus ?? 500; const pr = { @@ -427,6 +455,37 @@ export async function runEnforcePrTarget( const respond = async (method: string, args: unknown, data?: unknown) => record(method, args, data); + const nodeRequire = createRequire(path.join(process.cwd(), "package.json")); + const scriptsRoot = path.resolve(process.cwd(), ".github", "scripts"); + /** Bare modules the workflow script may load (see enforce-pr-target.yml). */ + const ALLOWED_MODULES = new Set(["path", "node:path"]); + + function scopedRequire(id: string) { + calls.push({ method: "require", args: [id] }); + const isPathLike = id.startsWith(".") || path.isAbsolute(id); + if (!isPathLike) { + if (!ALLOWED_MODULES.has(id)) { + throw new Error(`the script must not require ${id}`); + } + return nodeRequire(id); + } + const resolved = path.isAbsolute(id) ? path.resolve(id) : path.resolve(process.cwd(), id); + if (!resolved.startsWith(scriptsRoot + path.sep) && resolved !== scriptsRoot) { + throw new Error(`the script must not require ${id}`); + } + return nodeRequire(resolved); + } + + function compareResult(basehead: string): { ahead_by: number; behind_by: number } { + const override = options.compareByBasehead?.[basehead]; + if (override) return override; + if (basehead.startsWith("main...")) { + // Default: not the #644 shape (several commits ahead of main). + return { ahead_by: 8, behind_by: 0 }; + } + return { ahead_by: 0, behind_by: 0 }; + } + const rest = { pulls: { get: (args: unknown) => respond("pulls.get", args, pr), @@ -442,6 +501,16 @@ export async function runEnforcePrTarget( createComment: (args: unknown) => respond("issues.createComment", args, { id: 99 }), updateComment: (args: unknown) => respond("issues.updateComment", args, { id: 7 }), }, + repos: { + getCollaboratorPermissionLevel: (args: unknown) => + respond("repos.getCollaboratorPermissionLevel", args, { + permission: options.authorPermission ?? "read", + }), + compareCommitsWithBasehead: (args: unknown) => { + const basehead = String((args as { basehead?: string })?.basehead ?? ""); + return respond("repos.compareCommitsWithBasehead", args, compareResult(basehead)); + }, + }, }; /** @@ -705,8 +774,8 @@ export async function runEnforcePrTarget( glob: forbidden("glob"), io: forbidden("io"), fetch: forbidden("fetch"), - require: forbidden("require"), - __original_require__: forbidden("__original_require__"), + require: scopedRequire, + __original_require__: scopedRequire, // `github-script` runs under Node. An audit round detected the harness with // `if (!process.versions.bun) return;` — a no-op in production, green here. // Shadow `process` with something that looks like the Node the workflow