feat(ci): enforce PR ancestry and description quality gates - #648
Conversation
📝 WalkthroughWalkthroughThis PR adds reusable PR ancestry and description-quality gates, strengthens new bug-template validation, integrates the checks into trusted ChangesQuality validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequestTarget
participant EnforcePrTarget
participant PrQuality
participant GitHubAPI
PullRequestTarget->>EnforcePrTarget: synchronize or pull request event
EnforcePrTarget->>GitHubAPI: fetch PR, permissions, and commit comparisons
EnforcePrTarget->>PrQuality: collectPrQualityFailures(gate inputs)
PrQuality-->>EnforcePrTarget: failure codes
EnforcePrTarget->>GitHubAPI: update title, draft state, and enforcer comment
EnforcePrTarget-->>PullRequestTarget: completed or failed required check
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ci-workflows.test.ts (1)
764-776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a read-side fixture for the new marker/state pair and share the head SHA
tests/ci-workflows.test.ts:767-777and:1758-1765still feedLEGACY_MARKER+wrong-branch-enforcer-stateback into the harness. Add one restore case that usesMARKER+pr-quality-enforcer-stateso the new read path is exercised, not just the write path.tests/ci-workflows.test.ts:800hard-codes the same SHA astests/helpers/enforce-pr-target-harness.ts:125. Reuse the harness value instead so the ancestry override keys can’t drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ci-workflows.test.ts` around lines 764 - 776, Update the test fixtures around botComment and the related restore cases to include a read-side case using MARKER with pr-quality-enforcer-state, while retaining legacy coverage where needed. Replace the duplicated hard-coded head SHA in the tests with the shared value exported or exposed by enforce-pr-target-harness, so ancestry override keys use the same source.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/issue-quality.cjs:
- Around line 700-727: Update the new-form validation around the version and OS
checks in isUnusableVersion-related branches so version === null and os === null
are treated as missing required fields, while preserving existing legacy and
soft-pass behavior. Add regression tests in
.github/scripts/issue-quality.test.cjs covering otherwise-valid new bug reports
with the Version heading removed and with the Operating system heading removed;
both must fail the corresponding requirement.
In @.github/workflows/enforce-pr-target.yml:
- Around line 278-299: Wrap both compareCommitsWithBasehead calls in the
ALLOWED_BASES branch with error handling matching the collaborator permission
lookup: warn on failure and record an ancestryLookupFailed signal instead of
letting the promise escape. Pass ancestryLookupFailed into
collectPrQualityFailures and handle it like permissionLookupFailed so ancestry
validation is skipped while other gates still run; add the corresponding harness
failure option for repos.compareCommitsWithBasehead.
In `@MAINTAINERS.md`:
- Around line 26-29: Update the enforce-target policy wording in MAINTAINERS.md
to clarify that the CI check is expected but not a hard merge gate because
branch protection is not configured; remove or qualify “required” while
preserving the described failure conditions. Apply the same wording change to
the duplicated policy sentence in AGENTS.md.
In `@tests/ci-workflows.test.ts`:
- Line 716: Update the endpoint filter in the mutating-endpoint allowlist test
to remove only the two known read methods, repos.getCollaboratorPermissionLevel
and repos.compareCommitsWithBasehead, by their full method names. Remove the
broad !name.includes(".repos.") condition so other github.rest.repos mutating
methods remain covered by the exact allowlist.
In `@tests/helpers/enforce-pr-target-harness.ts`:
- Around line 456-470: Harden scopedRequire by allowing only the bare path
module before calling nodeRequire, and reject every other non-relative
specifier. In the resolved-path containment check, remove the normalized
/.github/scripts/ substring fallback and reject any path not under scriptsRoot
(or equal to it), while preserving legitimate in-boundary requires.
- Around line 496-509: Update repos.getCollaboratorPermissionLevel in the repos
stubs so failPermissionLookup uses the existing respond/failOn recording path
instead of directly returning Promise.reject. Seed
"repos.getCollaboratorPermissionLevel" in failOn alongside the other failure
options, or otherwise route the failure through respond, ensuring the call is
recorded before rejection and preserving normal permission responses when the
option is disabled.
---
Outside diff comments:
In `@tests/ci-workflows.test.ts`:
- Around line 764-776: Update the test fixtures around botComment and the
related restore cases to include a read-side case using MARKER with
pr-quality-enforcer-state, while retaining legacy coverage where needed. Replace
the duplicated hard-coded head SHA in the tests with the shared value exported
or exposed by enforce-pr-target-harness, so ancestry override keys use the same
source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b99e7233-7af8-40cd-ad4a-9be3385ffe20
📒 Files selected for processing (14)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/issue-quality.cjs.github/scripts/issue-quality.test.cjs.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.yml.github/workflows/issue-quality-tests.ymlAGENTS.mdMAINTAINERS.mddocs-site/src/content/docs/contributing.mddocs/superpowers/plans/2026-07-28-pr-quality-gates.mddocs/superpowers/specs/2026-07-28-pr-quality-gates-design.mdtests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1619420daf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (hasEscapedNewlines(body)) { | ||
| return { ok: false, reason: "escaped_newlines" }; | ||
| } | ||
| const cleaned = clean(body); |
There was a problem hiding this comment.
When a contributor opens GitHub's default PR form and leaves it untouched, .github/PULL_REQUEST_TEMPLATE.md supplies 337 characters across Summary, Verification, and Checklist sections. clean() preserves those instruction and checklist bullets, after which the structured-content check classifies the untouched template as substantive, so the new required description gate passes an effectively empty PR. Strip or explicitly recognize the repository's template boilerplate—and ignore the checklist—before measuring description content.
Useful? React with 👍 / 👎.
| const UNSTRUCTURED_MIN_BLOCKS = 2; | ||
|
|
||
| function isWrongAncestry({ behindMain, behindBase, threshold = ANCESTRY_BEHIND_THRESHOLD }) { | ||
| return behindMain === 0 && behindBase >= threshold; |
There was a problem hiding this comment.
When a legitimate contributor branch was cut from dev after the current main promotion and dev subsequently advances by at least 20 commits, behindMain is still zero because the main tip is an ancestor of every such dev commit, while behindBase reaches the threshold. This predicate therefore drafts and blocks ordinary stale dev-based branches, not just branches cut directly from main; inspect the base comparison's merge-base SHA (for example, require it to equal the main tip) rather than testing only whether main is contained in the head.
AGENTS.md reference: AGENTS.md:L64-L67
Useful? React with 👍 / 👎.
| if (willPrefixTitle) { | ||
| state.titlePrefixedByBot = true; | ||
| } else if (shouldStripTitlePrefix) { | ||
| state.titlePrefixedByBot = false; |
There was a problem hiding this comment.
When the base has been corrected but another gate still fails, shouldStripTitlePrefix clears titlePrefixedByBot here and the workflow persists that state before calling pulls.update. If that title update then fails transiently, later runs read titlePrefixedByBot: false and never remove the bot-added [WRONG BRANCH] prefix, even after every gate passes. Keep ownership set until the removal succeeds, or persist an explicit pending-removal state that subsequent runs retry.
Useful? React with 👍 / 👎.
| await convertToDraft(); | ||
| state.autoDraftedByBot = true; | ||
| await upsertComment( |
There was a problem hiding this comment.
When the initial convertToDraft mutation succeeds but the immediately following comment update fails, the only persisted checkpoint still has autoDraftedByBot: false. On every later run the PR is already a draft, so conversion is skipped, and once the gates pass the cleanup path will not mark it ready again; the workflow has permanently lost ownership of a draft state it created. Persist a pending draft-conversion state before the mutation and reconcile it on success or failure so a post-mutation API error remains recoverable.
Useful? React with 👍 / 👎.
|
@coderabbitai review @chatgpt-codex-connector review |
|
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Ready to merge. |
|
Codex findings from
CodeRabbit majors from earlier are marked addressed; CI is green on |
Capture the approved design for rejecting main-based PRs into dev/dev2-go and empty/thin/malformed PR bodies in the enforcer.
Break the approved spec into TDD tasks for pr-quality.cjs, enforcer wiring, harness coverage, and contributing docs.
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.
Reject untouched PR templates, require low ahead-of-main for ancestry, and checkpoint draft/title ownership before mutations.
312cb24 to
8a451f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/issue-quality.cjs (1)
716-726: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isUnusableVersion(os)is gated behindisNewBugForm, unlike the Version check one block up.Line 701 deliberately evaluates the unusable check on any form (
version !== null), and the comment on line 700 says so. Line 716 addsisNewBugForm &&, so a legacy body (noClient or integrationheading) that carries### Operating system→Unknown/모름falls through both branches: the unusable check is skipped by the form gate, the missing check on Line 719 is skipped by the same gate, and the removed-both check on Lines 733-742 is skipped becauseisEmpty("Unknown")is false. An unactionable OS value therefore passes on legacy bodies while the identical Version value is rejected.If the asymmetry is intentional (legacy forms never had a curated OS dropdown), a one-line comment stating that would keep the next reader from reading it as an oversight; otherwise drop the form gate to match Line 701.
🐛 Proposed fix: match the Version check's form-agnostic treatment
- if (!softPass && isNewBugForm && os !== null && isUnusableVersion(os)) { + if (!softPass && os !== null && isUnusableVersion(os)) { reasons.push("Operating system is missing or unknown.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/issue-quality.cjs around lines 716 - 726, Update the isUnusableVersion(os) branch in the OS validation logic to remove the isNewBugForm gate, matching the form-agnostic Version validation behavior. Preserve the existing isNewBugForm condition on the separate missing-OS branch and keep the current reasons and guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/pr-quality.test.cjs:
- Around line 31-36: Update the stale dev-based branch test around
isWrongAncestry to import and use ANCESTRY_AHEAD_MAIN_MAX, asserting the
boundary on both sides: aheadMain 5 is flagged and aheadMain 6 is not. Replace
the distant aheadMain: 50 assertion so the test detects changes to the
configured cap.
In @.github/workflows/enforce-pr-target.yml:
- Around line 29-43: Update the trusted checkout in the workflow step named
“Checkout trusted PR-quality scripts” to use the pull request base ref
(`github.event.pull_request.base.ref`) instead of
`github.event.repository.default_branch`, ensuring it matches the base branch
whose workflow is executing. Update the corresponding pinned expectations in
`tests/ci-workflows.test.ts` and the checkout-ref regex in
`.github/scripts/enforce-pr-target.test.cjs`.
In `@docs-site/src/content/docs/contributing.md`:
- Line 117: Update the workflow note in the contributing documentation to apply
only to the workflows using pull_request_target, specifically
enforce-pr-target.yml and pr-labeler.yml, rather than implying all workflow
changes use that trigger. Preserve the existing default-branch evaluation caveat
for those workflows.
In `@docs/superpowers/plans/2026-07-28-pr-quality-gates.md`:
- Around line 600-607: Update the “Step 1: Run required gates” checklist to
include the full `bun run test` suite alongside `bun run typecheck`, or
explicitly document an approved exemption. Preserve the existing focused CI
tests and privacy scan.
- Around line 287-315: Update assessPrDescription to call clean(body) before
hasEscapedNewlines, and pass the cleaned value to escaped-newline detection.
Preserve the existing empty, placeholder, structured-content, and
unstructured-content classification behavior while ensuring HTML comments are
normalized before checking literal escaped separators.
In `@docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md`:
- Around line 132-134: Update the Permissions sections in
docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md (lines 132-134) and
docs/superpowers/plans/2026-07-28-pr-quality-gates.md (lines 21-24) to use
contents: read while retaining pull-requests: write. Do not broaden
repository-content permissions unless documenting a specific future content
mutation.
In `@tests/ci-workflows.test.ts`:
- Around line 806-843: Add a workflow-level regression test beside the existing
ancestry scenarios that runs the real enforce-pr-target script with
compareCommitsWithBasehead configured to fail. Verify the compare lookup is
attempted, the failure warning is emitted, and no setFailed warning occurs,
proving ancestryLookupFailed is forwarded to collectPrQualityFailures and
prevents a verdict from zero-initialized comparison values.
---
Outside diff comments:
In @.github/scripts/issue-quality.cjs:
- Around line 716-726: Update the isUnusableVersion(os) branch in the OS
validation logic to remove the isNewBugForm gate, matching the form-agnostic
Version validation behavior. Preserve the existing isNewBugForm condition on the
separate missing-OS branch and keep the current reasons and guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96272d37-c8a1-47ab-91ef-7bf81bbe22e1
📒 Files selected for processing (14)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/issue-quality.cjs.github/scripts/issue-quality.test.cjs.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.yml.github/workflows/issue-quality-tests.ymlAGENTS.mdMAINTAINERS.mddocs-site/src/content/docs/contributing.mddocs/superpowers/plans/2026-07-28-pr-quality-gates.mddocs/superpowers/specs/2026-07-28-pr-quality-gates-design.mdtests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
| it("passes stale dev-based branches that are many commits ahead of main", () => { | ||
| assert.equal( | ||
| isWrongAncestry({ behindMain: 0, behindBase: 44, aheadMain: 50 }), | ||
| false, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin the ANCESTRY_AHEAD_MAIN_MAX boundary, not just a far-away value.
aheadMain: 50 is 45 commits clear of the cap, so this test still passes if someone changes the cap to 1 or to 45 — yet 5 is the number the PR description calls out as the tuned false-positive guard. Assert both sides of the edge (5 → flagged, 6 → not) the way Line 21-25 already does for ANCESTRY_BEHIND_THRESHOLD.
♻️ Proposed additional assertions
it("passes stale dev-based branches that are many commits ahead of main", () => {
assert.equal(
isWrongAncestry({ behindMain: 0, behindBase: 44, aheadMain: 50 }),
false,
);
});
+
+ it("caps ahead_by vs main at 5", () => {
+ assert.equal(ANCESTRY_AHEAD_MAIN_MAX, 5);
+ assert.equal(isWrongAncestry({ behindMain: 0, behindBase: 44, aheadMain: 5 }), true);
+ assert.equal(isWrongAncestry({ behindMain: 0, behindBase: 44, aheadMain: 6 }), false);
+ });ANCESTRY_AHEAD_MAIN_MAX also needs adding to the destructured import on Lines 5-11.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/pr-quality.test.cjs around lines 31 - 36, Update the stale
dev-based branch test around isWrongAncestry to import and use
ANCESTRY_AHEAD_MAIN_MAX, asserting the boundary on both sides: aheadMain 5 is
flagged and aheadMain 6 is not. Replace the distant aheadMain: 50 assertion so
the test detects changes to the configured cap.
| - 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"), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The trusted checkout pins default_branch (main), but the script it requires only exists on dev until a release promotion happens.
For pull_request_target, GitHub executes the workflow definition from the base branch — so once this PR merges to dev, every PR opened against dev runs this script body. Line 32 then checks out github.event.repository.default_branch, which the harness itself models as main (tests/helpers/enforce-pr-target-harness.ts Line 625: default_branch: "main"). .github/scripts/pr-quality.cjs is added in this PR and will not exist on main until a promotion. The require on Lines 41-43 then throws MODULE_NOT_FOUND before collectPrQualityFailures is ever reached: the step dies with an opaque stack trace, no comment, no core.setFailed summary — and the required enforce-target check fails for every contributor for the whole window between the dev merge and the next main release.
Two workable shapes:
- Guard the load and degrade to the wrong-base-only gate when the module is absent (fails open on the new gates, keeps the old behaviour working).
- Or pin the checkout to
github.event.pull_request.base.ref— still trusted (base-branch content, never PR head), and it is the same ref whose workflow definition is already executing, so script and workflow can never disagree.
🛡️ Option 2: check out the same trusted ref the workflow itself came from
- name: Checkout trusted PR-quality scripts
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
- ref: ${{ github.event.repository.default_branch }}
+ # The base branch, i.e. the same trusted ref this workflow definition
+ # was loaded from — never the PR head.
+ ref: ${{ github.event.pull_request.base.ref }}
persist-credentials: false
sparse-checkout: .github/scriptsNote this also touches the pinned expectations in tests/ci-workflows.test.ts Lines 561-565 and the regex in .github/scripts/enforce-pr-target.test.cjs Line 53.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 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"), | |
| ); | |
| - name: Checkout trusted PR-quality scripts | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| # The base branch, i.e. the same trusted ref this workflow definition | |
| # was loaded from — never the PR head. | |
| ref: ${{ github.event.pull_request.base.ref }} | |
| 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"), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/enforce-pr-target.yml around lines 29 - 43, Update the
trusted checkout in the workflow step named “Checkout trusted PR-quality
scripts” to use the pull request base ref (`github.event.pull_request.base.ref`)
instead of `github.event.repository.default_branch`, ensuring it matches the
base branch whose workflow is executing. Update the corresponding pinned
expectations in `tests/ci-workflows.test.ts` and the checkout-ref regex in
`.github/scripts/enforce-pr-target.test.cjs`.
Source: Linters/SAST tools
| - 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File under review ==\n'
git ls-files docs-site/src/content/docs/contributing.md
printf '\n== Workflow files ==\n'
git ls-files '.github/workflows/*' | sed 's#^`#-` #'
printf '\n== Contributing doc excerpt ==\n'
sed -n '105,125p' docs-site/src/content/docs/contributing.md
printf '\n== Search for pull_request_target in workflows ==\n'
rg -n --hidden --glob '.github/workflows/*' 'pull_request_target|pull_request' .github/workflows || trueRepository: lidge-jun/opencodex
Length of output: 3640
Scope the pull_request_target note in docs-site/src/content/docs/contributing.md:117.
The repo also has pull_request workflows such as .github/workflows/ci.yml and .github/workflows/react-doctor.yml, while only .github/workflows/enforce-pr-target.yml and .github/workflows/pr-labeler.yml use pull_request_target. Narrow this sentence to the specific workflow(s) that need default-branch evaluation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/contributing.md` at line 117, Update the workflow
note in the contributing documentation to apply only to the workflows using
pull_request_target, specifically enforce-pr-target.yml and pr-labeler.yml,
rather than implying all workflow changes use that trigger. Preserve the
existing default-branch evaluation caveat for those workflows.
| 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(/<!--[\s\S]*?-->/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" }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize HTML comments before escaped-newline detection.
The design specifies checking the cleaned body, but this plan calls hasEscapedNewlines(body) before clean(body). A bot comment containing literal \n sequences can therefore reject an otherwise valid PR. Clean first, then classify escaped separators.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-28-pr-quality-gates.md` around lines 287 -
315, Update assessPrDescription to call clean(body) before hasEscapedNewlines,
and pass the cleaned value to escaped-newline detection. Preserve the existing
empty, placeholder, structured-content, and unstructured-content classification
behavior while ensuring HTML comments are normalized before checking literal
escaped separators.
| - [ ] **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 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the full Bun test suite in final validation.
The checklist runs focused CI tests but omits bun run test, despite the repository requirement to run it before approving non-trivial changes. Add the full suite or explicitly document an approved exemption.
As per AGENTS.md, non-trivial changes require both bun run typecheck and bun run test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-28-pr-quality-gates.md` around lines 600 -
607, Update the “Step 1: Run required gates” checklist to include the full `bun
run test` suite alongside `bun run typecheck`, or explicitly document an
approved exemption. Preserve the existing focused CI tests and privacy scan.
| ### Permissions | ||
|
|
||
| Unchanged from #631: `contents: write` + `pull-requests: write` for draft GraphQL; still no untrusted checkout. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'docs/superpowers/plans/2026-07-28-pr-quality-gates.md' 'docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md'
printf '\n== Plan excerpt ==\n'
sed -n '1,120p' docs/superpowers/plans/2026-07-28-pr-quality-gates.md | cat -n
printf '\n== Design excerpt ==\n'
sed -n '110,170p' docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md | cat -nRepository: lidge-jun/opencodex
Length of output: 10891
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search permission mentions ==\n'
rg -n --hidden --glob '!**/.git/**' 'contents:\s*write|pull-requests:\s*write|pull_request_target|sparse \.github/scripts|wrong base|WRONG BRANCH' docs/superpowers -S
printf '\n== Search for content-mutating operations in these docs ==\n'
rg -n --hidden --glob '!**/.git/**' '\b(push|commit|create.*branch|delete.*branch|update.*file|write.*repo|contents write|github\.event\.repository\.default_branch)\b' docs/superpowers -SRepository: lidge-jun/opencodex
Length of output: 8005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== plan excerpt =='
sed -n '1,80p' docs/superpowers/plans/2026-07-28-pr-quality-gates.md | nl -ba | sed -n '1,80p'
echo '== design excerpt =='
sed -n '120,150p' docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md | nl -ba | sed -n '1,80p'
echo '== permission mentions =='
rg -n 'contents:\s*write|pull-requests:\s*write|pull_request_target|draft GraphQL|sparse \.github/scripts|wrong base|WRONG BRANCH' docs/superpowers -SRepository: lidge-jun/opencodex
Length of output: 216
🌐 Web query:
GitHub GraphQL draft pull request mutation permissions contents write pull-requests write
💡 Result:
To use the GitHub GraphQL API for creating and managing pull requests (including draft pull requests), your token or integration requires specific repository permissions. Permissions for Pull Request Mutations When using mutations like createPullRequest, convertPullRequestToDraft, or markPullRequestReadyForReview, you generally need the following permissions configured in your GitHub App or fine-grained personal access token (PAT): 1. pull-requests: write (Required for creating, updating, and managing the state of pull requests) [1] 2. contents: read (Required for reading repository data to support the operation) [1] Note that while contents: write is often associated with these operations, it is specifically required for actions that modify repository code, such as merging pull requests [1]. Common Issues and "Resource not accessible by integration" If you receive a "Resource not accessible by integration" error, it is frequently caused by one of the following: - Insufficient Default Permissions: In GitHub Actions, the default GITHUB_TOKEN often has read-only permissions [2]. You must explicitly grant write access in your workflow file [3][2]: permissions: contents: read pull-requests: write - Contribution from Forks: When a workflow is triggered by a pull request from a fork, the GITHUB_TOKEN may be restricted to read-only access for security reasons, even if your workflow has higher permissions defined [2]. - Server-to-Server Token Limitations: Some specific mutations may not be fully supported by server-to-server tokens (like GitHub App installations) in certain contexts, which can result in accessibility errors [4]. Draft Pull Request Specifics The createPullRequest mutation accepts a boolean draft field [5][6]. You can set draft: true when creating the pull request to initialize it as a draft [7]. Mutations for managing existing pull requests, such as convertPullRequestToDraft, also require the pull-requests: write permission [3][1].
Citations:
- 1: [SPIKE] Expand FGP per-tool permission annotations + consolidate docs github/github-mcp-server#2679
- 2: Resource not accessible by integration peter-evans/create-pull-request#3236
- 3: Change of DRAF state of Pull Request not possible over graphql actions/toolkit#1165
- 4: GraphQL API content does not appear to explain required permissions github/docs#8925
- 5: https://docs.github.com/en/graphql/reference/pulls
- 6: https://docs.github.com/en/enterprise-cloud@latest/graphql/reference/pulls
- 7: Enhance
gh pr createto support cross repo pull requests within the same organization cli/cli#10093
🌐 Web query:
site:docs.github.com draft pull request mutation permissions contents read pull-requests write
💡 Result:
In GitHub Actions, you can set the permissions of the GITHUB_TOKEN using the permissions key within your workflow file [1]. Granting contents: read and pull-requests: write provides the necessary access to read repository data and perform actions on pull requests, such as creating, updating, or managing them [2][1]. Regarding draft pull requests, they are a standard state of a pull request [3]. While the permissions you identified allow a workflow to interact with pull requests generally, there is no specific or separate permission required to interact with a draft versus a non-draft pull request [4][5]. However, operations on pull requests, such as merging, may be blocked by the repository's configuration while a pull request is in the draft state [6][7]. Key considerations for these permissions include: 1. Least Privilege: It is a recommended security practice to grant only the minimum permissions required for your workflow to function [1]. 2. Forked Repositories: If your workflow is triggered by a pull request from a forked repository, the GITHUB_TOKEN permissions are automatically restricted to read-only, regardless of the permissions specified in your workflow file, unless an administrator has enabled the setting to Send write tokens to workflows from pull requests [2][1]. 3. Implicit Deny: When you explicitly define a permissions block, any permissions not listed are set to no access (with the exception of metadata, which always receives read access) [1]. If you are attempting to automate the conversion of a pull request to/from a draft status, this is typically done using the GitHub REST API or GraphQL API, which require the pull-requests: write permission [4][7].
Citations:
- 1: https://docs.github.com/en/enterprise-server@3.13/actions/security-guides/automatic-token-authentication
- 2: https://docs.github.com/en/enterprise-server@3.2/actions/reference/authentication-in-a-workflow
- 3: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests?platform=linux
- 4: https://docs.github.com/en/rest/pulls/pulls
- 5: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2026-03-10
- 6: https://docs.github.com/en/enterprise-cloud@latest/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request
- 7: https://docs.github.com/en/graphql/reference/pulls
Reduce the workflow token to contents: read.
docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md#L132-L134 and docs/superpowers/plans/2026-07-28-pr-quality-gates.md#L21-L24 both keep contents: write, but the described pull_request_target flow only needs trusted default-branch checkout, compare/permission lookups, and PR draft/comment mutations. Keep pull-requests: write; if a future step really needs repository-content writes, call out that exact mutation instead of broadening the token here.
📍 Affects 2 files
docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md#L132-L134(this comment)docs/superpowers/plans/2026-07-28-pr-quality-gates.md#L21-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md` around lines
132 - 134, Update the Permissions sections in
docs/superpowers/specs/2026-07-28-pr-quality-gates-design.md (lines 132-134) and
docs/superpowers/plans/2026-07-28-pr-quality-gates.md (lines 21-24) to use
contents: read while retaining pull-requests: write. Do not broaden
repository-content permissions unless documenting a specific future content
mutation.
| 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); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No scenario exercises the ancestryLookupFailed branch added to the workflow this round.
.github/workflows/enforce-pr-target.yml Lines 303-308 catch compare failures, set ancestryLookupFailed, and rely on collectPrQualityFailures skipping the ancestry gate. .github/scripts/pr-quality.test.cjs Lines 225-244 pin the module-level contract, but nothing pins the workflow-level wiring: if a future edit forgets to forward ancestryLookupFailed into the call on Lines 311-321, the zero-initialised behindMain/behindBase from Lines 278-279 would still be 0/0 and the suite stays green while a compare outage silently disables the gate — or worse, a partial failure (main compare succeeds with behind_by: 0, base compare throws) leaves behindBase at 0 and produces a wrong verdict. The harness already supports failOn: ["repos.compareCommitsWithBasehead"], so the test is a few lines.
As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
💚 Proposed regression test next to the ancestry scenarios
test("a compare outage skips ancestry rather than inventing a verdict", async () => {
const { script } = await readEnforcePrTarget();
const result = await runEnforcePrTarget(script, {
pr: { base: { ref: "dev" } },
authorPermission: "read",
compareByBasehead: ANCESTRY_FAIL_COMPARES,
failOn: ["repos.compareCommitsWithBasehead"],
});
// The lookup was attempted and recorded, the gate was not enforced on
// unusable data, and the check still passes on the remaining gates.
expect(methodsOf(result)).toContain("repos.compareCommitsWithBasehead");
expect(result.warnings.some(w => w.includes("Could not compare commits"))).toBe(true);
expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ci-workflows.test.ts` around lines 806 - 843, Add a workflow-level
regression test beside the existing ancestry scenarios that runs the real
enforce-pr-target script with compareCommitsWithBasehead configured to fail.
Verify the compare lookup is attempted, the failure warning is emitted, and no
setFailed warning occurs, proving ancestryLookupFailed is forwarded to
collectPrQualityFailures and prevents a verdict from zero-initialized comparison
values.
Source: Path instructions
* 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.
feat(ci): carry PR quality gates onto dev2-go (#648)
1 participant
Summary
enforce-pr-targetto reject heads that sit on themaintip while far behind the PR base (the fix(windows): follow active Codex home for tray listener #644 failure mode: branch frommain, open intodev).\n, or thin PR descriptions (option 2: substance via two rich sections or a long multi-block body)..github/scripts+ purepr-quality.cjs; draft + comment +setFailedfor any open gate;[WRONG BRANCH]title prefix remains wrong-base only.Depends on
#631 — this branch is stacked on
fix/enforce-pr-target-draft-fallback. Merge or land #631 first; after that this PR’s diff should shrink to the quality-gate commits only.Test plan
node --test .github/scripts/pr-quality.test.cjs .github/scripts/enforce-pr-target.test.cjs(23 pass)bun test tests/ci-workflows.test.ts(64 pass)bun run typecheckbun run privacy:scancd docs-site && bun install --frozen-lockfile && bun run builddevdrafts + redenforce-target, and an empty/escaped body fails the same checkSpec / plan
docs/superpowers/specs/2026-07-28-pr-quality-gates-design.mddocs/superpowers/plans/2026-07-28-pr-quality-gates.mdSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests