feat(ci): verify checklist claims and reset readiness on head drift - #986
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 extracts shared readiness state, message, and maintainer helpers. The workflow binds completed checklists to the exact PR head SHA, resets stale completion after later commits, and re-drafts the PR. Tests and contributor documentation cover the new behavior. ChangesReadiness head binding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubAPI
participant EnforcePRTarget
participant StateModule
participant ChecklistModule
participant MaintainersModule
GitHubAPI->>EnforcePRTarget: deliver PR event with head SHA
EnforcePRTarget->>StateModule: parse stored readiness state
EnforcePRTarget->>StateModule: compare completion head with live head
alt Completion is stale
EnforcePRTarget->>ChecklistModule: reset readiness section
ChecklistModule-->>EnforcePRTarget: return unticked checklist
EnforcePRTarget->>StateModule: clear completion state
end
EnforcePRTarget->>MaintainersModule: parse eligible maintainers
EnforcePRTarget->>GitHubAPI: update draft status, body, and state
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 |
Wibias
added
the
chore
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/pr-quality.cjs (1)
330-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared marker-validation guard used by both readiness helpers.
Lines 331-340 duplicate the guard in
stripReviewReadinessSectionat lines 306-317 exactly: the sametypeofcheck, the sameindexOfpair, the sameend <= startrejection, and the same duplicate-occurrence counting.The failure mode is divergence. If the marker policy changes later (for example, tolerating a second marker pair, or accepting
end === start), a maintainer can update one copy and not the other.stripReviewReadinessSectionandresetReviewReadinessSectionwould then disagree about which bodies are "valid", so a body that reset refuses to touch could still be stripped, or the reverse. Both functions gate PR draft state, so the inconsistency is user-visible.Extract the bounds lookup once and let both callers use it.
♻️ Proposed refactor to share the bounds lookup
+/** + * Bounds of the single well-formed readiness section, or `null` when the body + * is not a string, has no markers, has inverted markers, or has duplicate + * marker pairs. Malformed marker sets stay untouched by both callers. + */ +function findReviewReadinessBounds(body) { + if (typeof body !== "string") return null; + const start = body.indexOf(REVIEW_READINESS_START); + const end = body.indexOf(REVIEW_READINESS_END); + if (start === -1 || end === -1 || end <= start) return null; + if ( + body.split(REVIEW_READINESS_START).length - 1 !== 1 || + body.split(REVIEW_READINESS_END).length - 1 !== 1 + ) { + return null; + } + return { start, endExclusive: end + REVIEW_READINESS_END.length }; +} + function resetReviewReadinessSection(body) { - if (typeof body !== "string") return body; - const start = body.indexOf(REVIEW_READINESS_START); - const end = body.indexOf(REVIEW_READINESS_END); - if (start === -1 || end === -1 || end <= start) return body; - if ( - body.split(REVIEW_READINESS_START).length - 1 !== 1 || - body.split(REVIEW_READINESS_END).length - 1 !== 1 - ) { - return body; - } - const section = buildReviewReadinessSection(); + const bounds = findReviewReadinessBounds(body); + if (!bounds) return body; + const section = buildReviewReadinessSection(); // Splice only the bounded section: the author's surrounding content — // including deliberate blank lines and trailing markdown — stays byte for // byte identical to what they wrote. return ( - body.slice(0, start) + + body.slice(0, bounds.start) + section + - body.slice(end + REVIEW_READINESS_END.length) + body.slice(bounds.endExclusive) ); }Apply the same substitution inside
stripReviewReadinessSectionat lines 306-317, keeping its\n{3,}collapse andtrimEnd()(that normalization is intentional there, because removing a section leaves an extra blank gap).🤖 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.cjs around lines 330 - 340, Extract the shared marker-validation logic from stripReviewReadinessSection and resetReviewReadinessSection into a helper that returns the validated start/end bounds or an invalid result. Update both functions to use this helper, preserving stripReviewReadinessSection’s existing \n{3,} collapse and trimEnd() normalization while keeping resetReviewReadinessSection’s behavior unchanged.
♻️ Duplicate comments (1)
.github/workflows/enforce-pr-target.yml (1)
631-636: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe reset still asks only for re-testing and re-ticking, not for a fresh maintainer approval of the new head.
Lines 635-636 instruct the author to re-test and re-tick. No step in the reset flow invalidates a maintainer approval that was granted against the previous head, and the repository documents that branch protection is not configured, so a stale approval survives the push. This repeats the finding raised on the previous commit at lines 625-628.
🤖 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 631 - 636, Update the headDriftNotice reset flow to explicitly invalidate the prior maintainer approval and require a fresh approval for the current head, in addition to re-testing and re-ticking all four boxes. Ensure the reset logic clears the stored approval state so stale approval cannot survive a pushed commit.Source: Path instructions
🤖 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/workflows/enforce-pr-target.yml:
- Around line 609-636: Update the head-drift logic around ticksPredateLiveHead
and headDrifted so the no-recorded-SHA case requires readiness.complete,
preventing incomplete or migrated v1 checklists from entering the reset path.
Build headDriftNotice only after freshReadiness is obtained, using the fresh
checklist state for the message and preserving maintainer notification state
when no reset occurred. Add a regression test alongside the existing stale-head
case using readinessChecklistBody(0) and maintainersPinged:true, asserting the
state is preserved and no reset notice is emitted.
In `@tests/ci-workflows.test.ts`:
- Around line 1495-1497: Replace the vacuous callsTo(result, "graphql").join("")
assertion near the rejected-completion test with an assertion over each recorded
call’s query string, using the accessor style already present nearby, so
markPullRequestReadyForReview is genuinely absent. Search this file and the
broader tests for other callsTo(...).join(...) assertions and update each
occurrence to inspect GraphQL query text rather than argument objects.
---
Outside diff comments:
In @.github/scripts/pr-quality.cjs:
- Around line 330-340: Extract the shared marker-validation logic from
stripReviewReadinessSection and resetReviewReadinessSection into a helper that
returns the validated start/end bounds or an invalid result. Update both
functions to use this helper, preserving stripReviewReadinessSection’s existing
\n{3,} collapse and trimEnd() normalization while keeping
resetReviewReadinessSection’s behavior unchanged.
---
Duplicate comments:
In @.github/workflows/enforce-pr-target.yml:
- Around line 631-636: Update the headDriftNotice reset flow to explicitly
invalidate the prior maintainer approval and require a fresh approval for the
current head, in addition to re-testing and re-ticking all four boxes. Ensure
the reset logic clears the stored approval state so stale approval cannot
survive a pushed commit.
🪄 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: 7c216074-d3a1-4acf-9961-25d75d410041
📒 Files selected for processing (4)
.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.ts
Wibias
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-maintainers.cjs:
- Around line 13-19: Update the section extraction logic in
enforce-pr-target-maintainers.cjs so a missing “## Current maintainers” heading
produces an empty recipient list rather than using the entire file; preserve the
existing subsection slicing when the heading exists. Update the corresponding
test expectation to assert [] for the absent-section fixture.
In @.github/scripts/enforce-pr-target-messages.cjs:
- Around line 37-49: Update the readiness message in the PR comment generation
flow around readinessChecklistLines so the readiness.present === false branch
only states that the checklist is not required, without claiming the PR is ready
for review. Add or update a test covering an absent checklist with a failure
section supplied through extra, and verify the output does not report the PR as
ready while preserving the failure section.
- Around line 13-15: Update inlineCode to wrap values with a Markdown code-span
delimiter longer than any contiguous backtick run in the value, rather than
attempting to escape backticks with a backslash. Extend the inlineCode tests to
cover values containing both a single embedded backtick and multiple consecutive
backticks, preserving correct rendering in each case.
In @.github/workflows/enforce-pr-target.yml:
- Around line 403-414: Prevent the completion path around completionIsStale from
attesting an unrecorded checklist during synchronize events. Require an
unrecorded completion to originate from a checklist-edit event whose event head
still matches the live head; otherwise keep the PR in draft and reset or require
a new checklist edit. Add a regression covering checklist completion on head A,
delayed persistence, and synchronize on head B.
🪄 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: b355acde-ac4c-4dc3-875a-215c192042a4
📒 Files selected for processing (8)
.github/scripts/enforce-pr-target-maintainers.cjs.github/scripts/enforce-pr-target-maintainers.test.cjs.github/scripts/enforce-pr-target-messages.cjs.github/scripts/enforce-pr-target-messages.test.cjs.github/scripts/enforce-pr-target-state.cjs.github/scripts/enforce-pr-target-state.test.cjs.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.ts
Drop the enforce-pr-target-* prefix from the split helpers so they match the existing pr-quality*.cjs naming and describe what they own.
Wibias
changed the title
Fail closed when MAINTAINERS.md lacks the current-maintainers section, render code spans with a safe delimiter, stop claiming readiness from checklist absence, and reject unrecorded complete checklists on synchronize so a push cannot inherit a still-queued attestation.
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/pr-maintainers.cjs:
- Around line 10-22: Update the section-start lookup in the maintainer parser to
match only an exact “## Current maintainers” H2 at the beginning of a line,
excluding ### subsections and prose occurrences. Preserve extraction of the
authoritative section through the existing next-heading logic, and add tests
covering both excluded cases.
🪄 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: b9bc2d18-648b-4fb5-b864-54aa0a090118
📒 Files selected for processing (9)
.github/scripts/pr-maintainers.cjs.github/scripts/pr-maintainers.test.cjs.github/scripts/pr-quality-messages.cjs.github/scripts/pr-quality-messages.test.cjs.github/scripts/pr-quality-state.cjs.github/scripts/pr-quality-state.test.cjs.github/workflows/enforce-pr-target.ymltests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
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/pr-maintainers.test.cjs:
- Around line 37-49: The tests around parseMaintainerLogins should add
regression coverage for a valid maintainer section using CRLF line endings,
trailing spaces or tabs on the heading, and a following H2 heading. Assert that
the fixture returns the expected maintainer login while preserving the existing
subsection and prose rejection cases.
🪄 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: 2309287f-e640-4ee7-87bc-aab60f5b29f8
📒 Files selected for processing (2)
.github/scripts/pr-maintainers.cjs.github/scripts/pr-maintainers.test.cjs
The gate now checks two of the four readiness boxes itself instead of trusting the author's ticks: the head's `ci` check must be green, and the branch must be on the latest dev or at most 10 commits behind it. A disproved claim unchecks the matching box, clears the completion attestation and maintainer ping, and keeps the PR a draft with an explanatory notice. Unknown CI/compare state fails closed.
Wibias
changed the title
|
✅ PR quality gates passed This pull request now targets The title was left unchanged. The pull request has been marked ready for review again. |
[GD] Verdict: approve-commentTLDR
Full verdictSemantic propagation
UsefulnessReal, user-visible gap closure: the #969 review comment asked for the checklist completion to be tied to the exact commit so "someone could get it checked and then make questionable changes afterward" without re-attestation. The gate now (1) records the attested head and re-drafts/resets on any later Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
SimplificationThe user-approved split relocated the 970-line inline script's pure logic into responsibility-named modules under GateNone. The PR is no longer a draft; Bottom lineShip. The checklist completion is bound to the attested head, the gate verifies the CI and latest-dev claims itself and unchecks/redrafts on violation, the oversized script is split by responsibility, all review threads are resolved, required CI is green on the final head, and the authoritative ship gate is |
|
Merging #986: the review-readiness checklist is now bound to the exact commit it attested, and the gate verifies the CI and latest-dev claims itself, so a completed checklist can no longer survive un-attested pushes. The oversized |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1a59df612
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| claimViolations = readinessClaimViolations({ | ||
| ciGreen, | ||
| behindBase, | ||
| behindUnknown: ancestryLookupFailed | ||
| }); |
There was a problem hiding this comment.
In the stacked-base path, behindBase is never populated because the compare calls run only when baseAllowed is true, but stacked PRs are accepted with baseAllowed=false. This feeds the initialized 0 into the latest-dev claim check here, so a contributor can target an open PR head that is far behind dev, tick all four boxes, and be marked ready without the promised freshness verification; compare DEFAULT_BASE...head separately for this checklist claim.
Useful? React with 👍 / 👎.
| let ciGreen = true; | ||
| try { | ||
| const { data: checksData } = | ||
| await github.rest.checks.listForRef({ |
There was a problem hiding this comment.
I checked this workflow's permissions block and it grants only contents: write and pull-requests: write, while GitHub documents this endpoint as requiring Checks repository read permission. In any environment where the public unauthenticated fallback is unavailable, this call throws, the catch sets ciGreen=false, and every completed contributor checklist has its CI box unticked instead of being accepted; add checks: read to the workflow permissions.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,263 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
These new PR-quality helper tests are not picked up by the existing workflows: I searched .github/workflows for pr-quality-state, pr-quality-messages, and pr-maintainers, and only enforce-pr-target.yml imports the modules, while issue-quality-tests.yml still lists and runs only the older script tests. A future PR touching only these privileged gate helpers would not trigger this test workflow or run the new unit tests, so regressions in the readiness gate can merge untested; add the new files/tests to the workflow paths and test command list, or switch that workflow to a safe glob.
Useful? React with 👍 / 👎.
1 participant
Summary
Follow-up to #969. Three things:
synchronizeevent where the head has changed, it moves the PR back to draft, unticks all four boxes in the description, clears the maintainer-notification flag, and tells the author to re-test against the latest code and complete the checklist again.enforce-pr-targetinline script is split by responsibility into small modules under.github/scripts/that follow the existingpr-quality*.cjsnaming:pr-quality-state.cjs— state markers, defaults, completion-staleness policypr-quality-messages.cjs— every user-facing comment/notice builderpr-maintainers.cjs— maintainer-list parsingEach has its own unit suite. The workflow keeps a thinner orchestrator for the GitHub reads/writes and the draft/ready transitions, so the write-allowlist and argument-shape security pins stay visible in the script.
b1a59df6): the head'scicheck must be green, and the branch must be on the latestdevcommit or at most 10 commits behind it. A disproved claim unchecks the matching box, clears the completion attestation and the maintainer ping, and keeps the PR a draft with an explanatory notice. Unknown CI or compare state fails closed. Heads with nocicheck (docs-only changes) keep the CI box; exactly 10 commits behind still passes. The same revalidation notice path now carries either the head-drift or the claim-check reset message.Why: previously a completed checklist stayed valid no matter what was pushed afterwards, so an author could tick the boxes, be marked ready and ping maintainers, then push questionable changes that nobody re-attested. The completion claim now expires automatically when the code it attested changes, and the two attestations the gate can verify itself (CI green, current dev) are checked against the live repository instead of trusted blindly.
Review rounds (
bc9147fd9,387da1005,ccc109886,dfaa8fd7f,bbb20a272,c9c63094b,cc48263c5,b1a59df6): the binding is race-proofed. A completion whose ticks predate the live head (a push raced theeditedjob) is rejected and reset instead of silently binding the newer head; a completion recorded while another quality gate keeps the draft still binds the head; a partial reset (body updated, comment update failed) is recovered on the next run; the reset preserves the author's surrounding description byte-for-byte; and a stale event on a never-completed checklist neither posts a reset notice nor wipes the bot's draft-ownership state. State written before this feature has no recorded head, so the binding starts at the next completion instead of retroactively drafting already-ready PRs; migrated states are rewritten at version 2. Module names describe ownership (pr-quality-*,pr-maintainers), not the workflow filename. CodeRabbit follow-ups (bbb20a272): maintainer parsing fails closed without the section, code spans use safe delimiters, readiness absence no longer claims ready, and synchronize events cannot inherit an unrecorded completion. (c9c63094b): the maintainer heading must be an exact## Current maintainersH2 at line start. (cc48263c5): positive CRLF/trailing-whitespace heading coverage.Test plan
bun test tests/ci-workflows.test.ts— 99/99 pass, including scenarios: completion records the head SHA; a new head after completion re-drafts, resets the boxes, and clears the ping; a same-head rerun is stable; head drift still enforces quality failures; ticks that predate the live head are rejected; completion while quality gates fail still binds the head; partial-reset state recovery; stale events on open checklists preserve ownership; redciunchecks the CI box and re-drafts; >10 commits behind unchecks the latest-dev box and re-drafts; both claims fail together; checks-lookup failure fails closed; exactly 10 behind and no-ciheads keep their boxes; pendingcicannot attest green.node --test .github/scripts/*.test.cjs— 381/381 pass, including new suites:readinessClaimViolationspartitions (green/red/behind/unknown/threshold),uncheckReviewReadinessBoxes(targeted uncheck, surrounding-content preservation, idempotence, malformed markers),buildClaimCheckNoticelines, plus the existing state/message/maintainer/checklist suites.bun run typecheck— pass.bun run lint:gui— pass.bun run privacy:scan— pass.doctor:gui:if-changed— skip (no gui change).EPERM: operation not permitted, symlink— no Developer Mode; same class documented in feat(ci): draft contributor PRs until review-readiness checklist is complete #969), all in files untouched by this diff.Review notes
resetReviewReadinessSectionanduncheckReviewReadinessBoxesin.github/scripts/pr-quality.cjssplice only the marker-bounded section; like the strip helper, they leave malformed marker sets untouched.completedAtHeadSha); parsing stays unversioned, so v1 and future states remain readable, and written states are normalized to the current version.