feat(ci): draft contributor PRs until review-readiness checklist is complete - #969
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a four-item contributor review-readiness checklist. The workflow manages checklist state, draft status, readiness comments, ready-for-review transitions, and maintainer notifications. Tests and documentation cover the new behavior. ChangesContributor review-readiness gate
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Contributor
participant enforce_pr_target
participant GitHub
participant MAINTAINERS
Contributor->>GitHub: Update PR checklist
enforce_pr_target->>GitHub: Read PR body and permissions
enforce_pr_target->>MAINTAINERS: Read maintainer logins
enforce_pr_target->>GitHub: Update checklist, comments, and draft state
GitHub-->>MAINTAINERS: Send completion notification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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: 08923a5fba
ℹ️ 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 (!storedState?.active) { | ||
| if (!storedState?.active && !checklistRequired) { |
There was a problem hiding this comment.
When getCollaboratorPermissionLevel transiently fails for a clean maintainer PR, the fail-closed contributor path injects the checklist and converts the PR to draft; on a later successful run, checklistRequired becomes false and this early return ignores storedReadinessState.autoDraftedByBot, leaving the maintainer PR permanently drafted by the bot. Include the readiness ownership state in the restoration decision before returning so authors with push permission retain the documented failure-only behavior.
AGENTS.md reference: AGENTS.md:L181-L186
Useful? React with 👍 / 👎.
| screenshotFailed: false | ||
| }; | ||
|
|
||
| const titleResult = storedState.titlePrefixedByBot |
There was a problem hiding this comment.
When a contributor completes the checklist while an existing bot comment has missing or malformed JSON state, parseState returns null but botComment still makes this block run, so dereferencing storedState.titlePrefixedByBot throws and the workflow never marks the PR ready or notifies maintainers. Use optional access or synthesize a cleared state before reading these fields, as the failure path already does.
AGENTS.md reference: AGENTS.md:L181-L185
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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.cjs:
- Around line 243-246: Update appendReviewReadinessSection’s marker validation
so any existing but malformed REVIEW_READINESS_START/REVIEW_READINESS_END
markers are reported as present and incomplete, preventing another checklist
from being appended; only bodies with exactly one correctly ordered pair should
be treated as absent or complete. Add regression coverage for inverted and
duplicate markers, asserting appendReviewReadinessSection returns each body
unchanged.
In @.github/workflows/enforce-pr-target.yml:
- Around line 859-897: Resolve the maintainer list with readMaintainerLogins()
before updating readinessState in the checklist-complete flow. Set
maintainersPinged and notified to true only when maintainers.length is greater
than zero, so an empty or failed lookup leaves the state unclaimed and allows a
later run to notify maintainers.
- Around line 806-855: Guard the `storedState` property reads in `titleResult`
and `draftResult` within the `botComment` completion block, matching the
existing optional-chaining usage so missing or unparsable state preserves the
fallback messages without throwing. Add a regression case in
`tests/ci-workflows.test.ts` covering a contributor PR with a complete checklist
and a marker-only or corrupted bot comment, asserting the workflow completes
successfully.
- Around line 183-207: Update extractReviewReadiness to return per-item checked
state, then change readinessChecklistLines to use each item’s corresponding
state rather than comparing index with readiness.checked. Preserve the existing
aggregate checked/total values and the fixed 4/4 completion text in
upsertReadinessComment.
- Around line 161-181: Update readMaintainerLogins to extract only the text
between the “## Current maintainers” heading and the next “##” heading, then
apply matchAll to that section instead of the full file. Preserve the existing
login deduplication and error-handling behavior.
In `@docs-site/src/content/docs/contributing/pr-quality.md`:
- Around line 43-50: Update the maintainer-notification statements in
docs-site/src/content/docs/contributing/pr-quality.md lines 43-50, AGENTS.md
lines 181-185, and MAINTAINERS.md lines 28-33 to specify that notifications go
to maintainers listed in MAINTAINERS.md, excluding the pull request author.
In `@tests/ci-workflows.test.ts`:
- Around line 12-36: Update lastEnforcerCommentBody and lastReadinessCommentBody
to explicitly detect when no matching comment exists before accessing the final
entry. Throw a descriptive error that identifies the missing enforcer or
readiness comment family, while preserving the existing behavior of returning
the latest matching update or creation.
In `@tests/helpers/enforce-pr-target-harness.ts`:
- Line 529: The harness must stop exposing the real filesystem and use a
recorded read-only stub instead. In tests/helpers/enforce-pr-target-harness.ts
at lines 529-529, remove node:fs from ALLOWED_MODULES and update scopedRequire
to allow only MAINTAINERS.md under process.cwd(), honoring
options.maintainersFile; in tests/ci-workflows.test.ts at lines 1196-1199,
provide a fixture roster through that override or derive expected logins from
MAINTAINERS.md, removing the hardcoded roster assertions at both referenced
locations.
🪄 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: 2cdb4d2f-b53a-4022-b167-7097f1bc2b12
📒 Files selected for processing (8)
.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.ymlAGENTS.mdMAINTAINERS.mddocs-site/src/content/docs/contributing/pr-quality.mdtests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/enforce-pr-target.test.cjs:
- Around line 79-81: Update the failure-block regex assertion in the test to
anchor on the quality-failure branch, using `if (failures.length > 0) {` or
otherwise validating both `core.setFailed(` call sites, so call reordering
cannot select the checklist conversion path. Rename the `"workflow must have a
draft path"` assertion message to identify the quality-failure path.
In `@tests/ci-workflows.test.ts`:
- Around line 1196-1204: Update the failure test around lastReadinessCommentBody
and result.warnings to filter setFailed: warnings, assert exactly one matching
entry, and verify that entry contains the fail-closed message. Add a
complete-checklist case using a bot comment with LEGACY_COMMENT_MARKER and no
parseable state, then guard the enforce-pr-target workflow path after parseState
returns null so storedState is not dereferenced.
🪄 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: 7bec0bae-14b4-422b-941e-7d7ac8f8b583
📒 Files selected for processing (3)
.github/scripts/enforce-pr-target.test.cjs.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.ts
|
[GD] Addressed feedback feedbacks:
commit: 40b99b7 |
[GD] Verdict: gatedPR: Semantic propagation
Linked: none UsefulnessReal gap closed: contributor PRs could previously be marked ready for review with unresolved Codex/CodeRabbit findings or no proof of local CI, while the repo's own contract defines ready as a claim that the change is complete, understood, and tested. The gate makes that claim explicit and mechanical for non-maintainer authors. Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (applied — explicitly requested)
Gate
Bottom lineThe change is complete and reviewed: all axes pass, CI is green on the final head, and every bot finding is fixed or explicitly declined. The verdict is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40b99b73e7
ℹ️ 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".
| // cannot churn the body on every run. | ||
| let readiness = extractReviewReadiness(pr.body); | ||
| if (checklistRequired && !readiness.present) { | ||
| const injectedBody = appendReviewReadinessSection(pr.body ?? ""); |
There was a problem hiding this comment.
When a contributor opens a PR with an empty, placeholder, escaped-newline, or thin body, the first run injects this roughly 400-character checklist and triggers an edited run; on that run, assessPrDescription treats the bot-authored heading, instructions, and checklist items as substantive content, so bad_description clears without the author adding a Summary or Test plan. Strip the bounded readiness section before calling collectPrQualityFailures (or classify its lines as boilerplate) so contributor PRs still enforce the documented description gate.
AGENTS.md reference: AGENTS.md:L177-L180
Useful? React with 👍 / 👎.
| await upsertReadinessComment( | ||
| { ...storedReadinessState, autoDraftedByBot: false }, |
There was a problem hiding this comment.
When a maintainer was auto-drafted during a failed permission lookup and markReadyForReview then fails transiently after permission recovery, this unconditional update clears autoDraftedByBot and claims the PR is ready even though it remains a draft; later runs therefore never retry the conversion. Fresh evidence in this revision is that the new recovery catch continues into this state-clearing update; preserve ownership and report the failure unless the GraphQL mutation succeeds.
AGENTS.md reference: AGENTS.md:L181-L186
Useful? React with 👍 / 👎.
| await convertToDraft(); | ||
| draftConverted = true; | ||
| readinessState.autoDraftedByBot = true; |
There was a problem hiding this comment.
In the checklist-only path, ownership is recorded only after convertToDraft succeeds. If that mutation succeeds but the following readiness-comment update fails or the job is interrupted, the durable comment still says autoDraftedByBot:false; when a failed permission lookup later recovers—or the author gains push permission—the maintainer recovery path leaves the bot-created draft in place permanently. Persist a pending ownership claim before the mutation, as the quality-failure path already does, then clear it only when conversion fails.
AGENTS.md reference: AGENTS.md:L181-L186
Useful? React with 👍 / 👎.
| readiness.present | ||
| ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there." | ||
| : "This PR is ready for review; the review readiness checklist is not required for this author.", |
There was a problem hiding this comment.
After a transient permission lookup failure, the bot has already injected the checklist into the maintainer's PR body, so the recovery run still has readiness.present === true; this condition therefore renders “kept in draft until every requirement” and the four unchecked items even after markReadyForReview succeeds and the same comment says the PR is ready. Base applicability on checklistRequired rather than marker presence, and remove or retire the bot-managed body section when the author is confirmed to have push permission.
AGENTS.md reference: AGENTS.md:L181-L186
Useful? React with 👍 / 👎.
| extractReviewReadiness, | ||
| appendReviewReadinessSection, | ||
| REVIEW_READINESS_ITEMS |
There was a problem hiding this comment.
After this change lands on dev, pull_request_target loads this workflow from the PR's dev base, but the checkout still explicitly loads .github/scripts from the repository default branch (main). Until the next maintainer promotion, main's pr-quality.cjs lacks the newly destructured readiness exports, so every subsequent PR run reaches extractReviewReadiness(...) as undefined and aborts before enforcing anything. Load the helpers from the trusted base revision or keep the workflow compatible with the currently promoted default-branch script.
AGENTS.md reference: AGENTS.md:L159-L161
Useful? React with 👍 / 👎.
|
[GD] Addressed feedback feedbacks:
commit: a62bc19 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/ci-workflows.test.ts (1)
22-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReject created comments that lack an enforcer marker.
Line 22 falls back to every
issues.createCommentcall when no enforcer comment exists. A readiness-only comment can then be returned bylastEnforcerCommentBody, so later assertions inspect the wrong comment instead of reporting the missing enforcer comment.Return only
enforcerCreates. Throw"scenario recorded no enforcer comment"when that list is empty.🤖 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 22 - 26, Update lastEnforcerCommentBody to use only enforcerCreates, removing the fallback to creates; throw "scenario recorded no enforcer comment" when enforcerCreates is empty, then return its last comment body.
🤖 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.cjs:
- Around line 250-275: Update the review-readiness parsing logic around
REVIEW_READINESS_START and REVIEW_READINESS_END to require exactly one
occurrence of each marker before parsing checklist boxes. Return present: true
and complete: false for duplicate, partial, or inverted markers, and ensure
stripReviewReadinessSection leaves malformed duplicate sections unchanged.
Extend the related tests to cover duplicate sections being incomplete and
preserved during stripping.
In @.github/workflows/enforce-pr-target.yml:
- Around line 32-36: Pin the workflow checkout in
.github/workflows/enforce-pr-target.yml to github.event.pull_request.base.sha
instead of base.ref, ensuring trusted policy files match the event base commit.
Update the corresponding assertions in
.github/scripts/enforce-pr-target.test.cjs lines 51-53 and
tests/ci-workflows.test.ts lines 843-846, then run bun run typecheck and bun run
test.
---
Duplicate comments:
In `@tests/ci-workflows.test.ts`:
- Around line 22-26: Update lastEnforcerCommentBody to use only enforcerCreates,
removing the fallback to creates; throw "scenario recorded no enforcer comment"
when enforcerCreates is empty, then return its last comment body.
🪄 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: a5253fa3-a31c-40f5-b86a-4d1241187a2e
📒 Files selected for processing (9)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.ymlAGENTS.mdMAINTAINERS.mddocs-site/src/content/docs/contributing/pr-quality.mdtests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
…list markers as malformed
|
[GD] Addressed feedback feedbacks:
commit: a924576 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/enforce-pr-target.test.cjs:
- Line 53: Update the workflow test around the base-SHA assertion to target the
“Checkout trusted PR-quality scripts” step specifically, parsing or capturing
that step and validating its with.ref value equals
github.event.pull_request.base.sha. Keep the existing sparse-checkout assertions
bound to that same checkout step, rather than searching the entire workflow
text.
🪄 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: baf0fcd9-3824-46f4-8816-254f8ec1a34e
📒 Files selected for processing (5)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.ts
|
[GD] Addressed feedback feedbacks:
commit: 1fc7ab9 |
…ayer `assessPrDescription` already refuses the injected section, and the unit tests cover that directly. What was not pinned is the sequence that exploits it, which no single-function test can express: the attack needs two runs and a body the bot itself wrote in between. Run one opens a contributor PR with an empty description; the gate fails `bad_description` and injects the checklist. Run two ticks the four boxes the bot just added. If the injected section ever counts as substance again, the description gate passes on text the author never wrote and the PR is marked ready for review — an empty PR, reviewable, with the maintainers pinged. The test takes the injected body from the recorded `pulls.update` call rather than a hand-built fixture, so it exercises the text the gate actually writes, and asserts `graphql` is never reached — that is where `markPullRequestReadyForReview` goes out. The screenshot axis gets the same treatment: the injected section adds renderable structure but no image, so a gui-cued PR with a complete checklist and no screenshot must still fail and stay drafted. Driven red to prove it is not vacuous: reverting the `stripReviewReadinessSection` call in `assessPrDescription` fails the laundering test and nothing else. Verified: bun test tests/ci-workflows.test.ts 83 pass; node --test on .github/scripts 58 pass; typecheck and privacy:scan pass.
2 participants
Summary
devcommit, all correct Codex and CodeRabbit findings fixed, and the ready-for-review confirmation.enforce-targetmarks the PR ready for review and notifies the maintainers listed inMAINTAINERS.md(excluding the author). Wrong-base PRs are drafted as before; retargeting todevupdates the bot message and is remembered, but the draft stays until the checklist is complete.enforce-targetcheck itself stays green while only the checklist is pending.Test plan
bun test tests/ci-workflows.test.ts— 81/81 pass (harness-executed scenarios: draft-on-open, retarget-remember, checklist completion, maintainer ping, wrong-base + complete checklist, permission-recovery restore and failure retention, corrupted-state guard, checkpointed draft ownership, and the existing enforcer audit cases).node --test .github/scripts/enforce-pr-target.test.cjs .github/scripts/pr-quality.test.cjs— 58/58 pass (checklist extraction/injection/stripping, inverted/duplicate marker, per-item state, description-gate isolation).bun run typecheck— pass;bun run lint:gui— pass;bun run privacy:scan— pass;doctor:gui:if-changed— skip (no gui change).node --checkon the extracted workflow script — pass; the script contains no${{ }}interpolation and keeps the exactpull_request_targettrigger/permission allowlist.tests/config.test.tsfailures reproduce on an untouched baseline). One unrelated scan test timed out once under load and passes in isolation.gates, validator tests, keyring, and CodeRabbit green on the previous head; the macOS lane hit a Bun 1.3.14 runtime segfault once and passed on rerun (same crash class reproduced locally on Windows; recentdevruns on the identical base tip are green).Review notes
pull_request_targetruns the workflow from the PR's base branch, so the trusted script checkout now usesgithub.event.pull_request.base.ref— after this lands ondev, the gate loads the matchingdevscripts instead of the pre-promotionmainones.bc66a878/40b99b73; round 2 (Codex) ina62bc19— description-gate isolation, recovery ownership retention, checkpointed draft ownership, maintainer checklist retirement, base-revision script checkout.Limitations
Summary by CodeRabbit
New Features
Documentation