ci: harden PR readiness gate — consolidated comment, draft-review trigger, G.UI waiver - #1094
Merged
Merged
Conversation
…ger, and GUI waiver Consolidate the PR gate's two bot comments (enforcer + readiness) into one always-present, always-edited comment that carries the current status, the "what to do" actions, the readiness-checklist mirror, and the draft reason. Legacy two-comment PRs migrate their state into the single comment and the old comments are deleted. State fields are read for truthiness, matching the pre-consolidation gate. Trigger CodeRabbit/Codex review at the ready moment via a review-ready label (.coderabbit.yaml auto_review.labels), so a ready-but-draft PR gets reviewed without a manual @coderabbitai review. The findings claim verifies review threads primarily and supplements with CodeRabbit review-body findings that fall outside the diff range; both are head-bound and fail closed. Allow a Collaborator/Owner issue comment (e.g. "not touching gui") to waive the GUI-screenshot gate; the author cannot self-waive. Tests cover the consolidated comment, label management, migration, the outside-diff supplement, and the GUI waiver. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
|
✅ PR quality gates passed This pull request now targets The title was left unchanged. The pull request has been marked ready for review again. |
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
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 consolidates gate state and comments, validates Codex and CodeRabbit findings, adds maintainer GUI waivers, manages the ChangesPR quality gate
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequestEvent
participant EnforceWorkflow
participant GitHubReviewAPI
participant GateState
participant GateComment
PullRequestEvent->>EnforceWorkflow: trigger quality-gate evaluation
EnforceWorkflow->>GitHubReviewAPI: read review threads, reviews, comments, and labels
GitHubReviewAPI-->>EnforceWorkflow: return review and pull request data
EnforceWorkflow->>GateState: parse or migrate gate state
GateState-->>EnforceWorkflow: return claims, findings, and label state
EnforceWorkflow->>GateComment: create or update consolidated gate comment
EnforceWorkflow->>GitHubReviewAPI: add or remove review-ready label
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Wibias
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 @.coderabbit.yaml:
- Around line 17-24: Update the auto-review configuration around drafts and
labels so ready-but-draft pull requests are eligible and existing unlabeled pull
requests are not unintentionally excluded: set drafts to true for broad draft
eligibility, or use the supported opt-in configuration with enabled false when
reviews should be limited to the review-ready label.
In @.github/scripts/enforce-pr-target.test.cjs:
- Around line 51-54: Update the two trigger assertions in the “listens for
review events so bot findings after ready are caught” test to match each YAML
trigger as an independent list item, using the mapping-key form for
pull_request_review and sequence-item form for pull_request_review_comment.
Ensure the first assertion cannot pass merely because the second trigger remains
present.
In @.github/scripts/pr-quality-messages.cjs:
- Around line 12-15: Correct the doc comment above READINESS_MARKER to identify
it as the legacy pre-consolidation readiness marker used for migration and
deletion, while leaving GATE_MARKER’s consolidated PR gate comment description
unchanged.
In @.github/scripts/pr-quality-state.cjs:
- Around line 256-262: Update the comparator used to select latestForHead so
unparsable or missing submitted_at values are converted to a fixed lowest
timestamp before subtraction. Keep valid dates ordered newest first, and ensure
undated reviews sort last deterministically without changing the liveHeadSha
filtering.
- Around line 171-184: migrateLegacyGateState must preserve bot draft ownership
from either legacy record. Update the autoDraftedByBot handling so the
enforcerState and readinessState values are merged with logical OR semantics,
while retaining the existing defaults and other field mappings; add the
described regression test beside the existing merge test.
- Around line 252-273: Update coderabbitOutsideDiffFindings to filter reviews by
both liveHeadSha and review.user.login matching CODE_RABBIT_LOGIN, and attribute
results using that same constant. Add resolution handling so a later CodeRabbit
review for the same head reporting zero actionable comments, or an accepted
maintainer acknowledgement via hasGuiOverride, clears the prior count. Update
fixtures and add coverage proving human reviews with matching text are ignored
and resolved findings no longer remain counted.
In @.github/scripts/pr-quality.cjs:
- Around line 190-191: Update GUI_OVERRIDE_RE so its negation-to-gui window
cannot cross sentence or line boundaries while preserving matches for the
existing same-clause fixtures. Add regression coverage in hasGuiOverride tests
for negations separated by a sentence and by a newline/list-item boundary,
ensuring both return false.
In @.github/scripts/pr-quality.test.cjs:
- Around line 152-194: Add integration tests for collectPrQualityFailures that
pass guiOverrideComments and verify a qualifying maintainer comment suppresses
missing_ui_screenshot, while a CONTRIBUTOR comment does not. Reuse the existing
GUI fixture setup and assertions around collectPrQualityFailures; keep the
direct hasGuiOverride tests unchanged.
In @.github/workflows/enforce-pr-target.yml:
- Around line 11-15: Move pull_request_review and pull_request_review_comment
out of the pull_request_target types list and declare them as sibling top-level
workflow events in the on block. Keep the existing pull_request_target activity
types unchanged so review events trigger the findings check for ready PRs while
retaining the current trusted base-context behavior.
- Around line 205-221: Guard the label mutations in setReviewReadyLabel so
failures from addLabels or removeLabel are caught, logged as warnings, and do
not reject the workflow. Update the caller around the setReviewReadyLabel
invocation to record the resulting label state from the mutation outcome, then
continue to upsertGateComment and perform subsequent draft handling even when
label synchronization fails.
- Around line 1025-1036: Remove the unreachable fallback block guarded by
checklistRequired && checklistComplete after the ready path. The earlier
matching branch already handles this condition and returns, so delete the
associated upsertGateComment call and its comments, including the out-of-scope
readyState usage.
- Around line 583-603: Update the GraphQL query in the review-data retrieval
flow to paginate reviewThreads using pageInfo and continue fetching pages until
hasNextPage is false, collecting every thread node. Pass the complete collected
thread list to the existing mapper instead of the single-page nodes result,
while preserving the existing catch-based findingsUnverifiable fail-closed
behavior for errors on any page.
- Around line 737-747: Update the serialized-state documentation in
pr-quality-state.cjs around reviewReadyLabeled, or remove this unused persisted
field; do not present it as an input to live label calculations. In the
workflow’s checklistRequired/readyMoment logic, align maintainer PR handling
with the review-ready label trigger so maintainers cannot bypass the label while
clearing failed gates, or explicitly encode and document the intended carve-out
consistently.
In `@tests/ci-workflows.test.ts`:
- Around line 2828-2832: Remove the duplicate const createdComments declaration
in the test callback, leaving one declaration before the created lookup and
assertions. Keep repeated names in separate test callbacks unchanged.
In `@tests/helpers/enforce-pr-target-harness.ts`:
- Around line 695-726: Update the graphql method to be async so failures from
record and failGraphqlOn reject as promises rather than throwing synchronously.
Preserve the reviewThreads raw payload, and replace the fallthrough return of
recorded with a raw GraphQL-shaped payload while keeping the recording side
effect intact.
🪄 Autofix
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: 3a6cf6fd-184d-4121-9d1c-b2587fe62730
📒 Files selected for processing (11)
.coderabbit.yaml.github/scripts/enforce-pr-target.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/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.ymltests/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: 0106235a91
ℹ️ 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".
Address verified review findings on the hardened PR gate: - Move pull_request_review / pull_request_review_comment to sibling top-level events: listing them under pull_request_target.types is silently ignored by GitHub (actionlint: invalid activity type), so the review-event feature was inert. Both payloads carry pull_request, so the base.sha trusted checkout and context reads are unchanged. - migrateLegacyGateState: OR the autoDraftedByBot ownership bit across the legacy enforcer and readiness records; the readiness value overwriting a true enforcer bit dropped the restore path for bot-drafted maintainer PRs. - coderabbitOutsideDiffFindings: filter reviews to coderabbitai[bot] so a human review quoting 'Actionable comments posted' cannot untick the box; bound the immutable review-body count to runs with an unresolved bot thread so resolving threads clears the box without an empty commit; sort undated reviews deterministically instead of NaN. - GUI_OVERRIDE_RE: forbid sentence/line breaks in the negation window so 'This does not change the API. Please add a gui screenshot.' no longer waives the screenshot gate. - setReviewReadyLabel: guard label writes so a failure cannot abort the run before the gate comment or draft/ready conversion. - reviewThreads: paginate past the first 100 threads so a busy PR cannot hide unresolved bot threads (fail-open gap in a fail-closed check). - Harness: make the graphql fake async/rejecting and return a raw GraphQL payload so failures and mutation results match production. - Docs: sync pr-quality.md and the structure workflow map with the consolidated comment, findings claim, review-ready label, GUI waiver, and new triggers; document reviewReadyLabeled as non-consulted serialized state. Tests: node --test .github/scripts (420 pass), bun test tests/ci-workflows (112 pass), bun run typecheck, bun run privacy:scan, actionlint all green.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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-state.cjs (1)
299-323: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDetect standalone CodeRabbit outside-diff findings.
Line 316 only reads a positive CodeRabbit review-body count after an unrelated unresolved bot thread exists. A current-head CodeRabbit review that reports actionable outside-diff findings but creates no review thread returns a clean claim. The contributor can then complete the findings box despite unresolved CodeRabbit findings.
Track the review-body finding lifecycle independently. Count a positive current-head CodeRabbit claim even when
threadsis empty. Clear it only with a CodeRabbit-specific resolution signal, such as a later zero-actionable review for the same head.
.github/scripts/pr-quality-state.cjs#L299-L323: remove the genericunresolved > 0prerequisite and add a CodeRabbit-specific clear condition..github/scripts/pr-quality-state.test.cjs#L463-L471: replace the clean-result expectation with coverage for standalone detection and the selected clear condition.As per path instructions, readiness requires “resolved correct Codex/CodeRabbit findings.”
🤖 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-state.cjs around lines 299 - 323, Update unresolvedFindingsClaim in .github/scripts/pr-quality-state.cjs (lines 299-323) to evaluate positive current-head CodeRabbit outside-diff review findings independently of the generic unresolved thread count, and clear them only when a CodeRabbit-specific resolution signal such as a later zero-actionable review for the same head is present. Update .github/scripts/pr-quality-state.test.cjs (lines 463-471) to replace the clean-result assertion with coverage proving standalone CodeRabbit detection and the selected clear condition.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 `@docs-site/src/content/docs/contributing/pr-quality.md`:
- Around line 60-66: The gate descriptions must document that current-head
CodeRabbit review bodies are checked in addition to review threads. Update
docs-site/src/content/docs/contributing/pr-quality.md lines 60-66 to explain
that actionable outside-diff findings in those bodies block readiness, and add
the same behavior to structure/06_docs-and-release.md line 47 so both workflow
descriptions remain synchronized.
- Around line 55-56: Update the documentation wording near “if new commits are
pushed afterwards” to use the American-English form “afterward,” preserving the
surrounding sentence and meaning.
- Around line 43-45: Align the GUI-screenshot waiver policy across the
documentation and implementation: either explicitly define MEMBER as an
authorized waiver role in MAINTAINERS.md, or remove MEMBER from the allowlist in
the waiver logic near the relevant role check in pr-quality.cjs, update
pr-quality.md accordingly, and adjust the MEMBER authorization coverage in
pr-quality.test.cjs.
In `@structure/06_docs-and-release.md`:
- Line 47: Update the workflow description for enforce-target to explicitly
document the supplementary CodeRabbit review-body findings check, including that
findings outside the diff range are considered alongside review-thread
validation and can prevent readiness until addressed or waived. Keep the
existing description of the other readiness claims and review-ready labeling
behavior unchanged.
---
Outside diff comments:
In @.github/scripts/pr-quality-state.cjs:
- Around line 299-323: Update unresolvedFindingsClaim in
.github/scripts/pr-quality-state.cjs (lines 299-323) to evaluate positive
current-head CodeRabbit outside-diff review findings independently of the
generic unresolved thread count, and clear them only when a CodeRabbit-specific
resolution signal such as a later zero-actionable review for the same head is
present. Update .github/scripts/pr-quality-state.test.cjs (lines 463-471) to
replace the clean-result assertion with coverage proving standalone CodeRabbit
detection and the selected clear condition.
🪄 Autofix
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: d2ae60a6-3f72-4c4f-a396-65723d425507
📒 Files selected for processing (11)
.github/scripts/enforce-pr-target.test.cjs.github/scripts/pr-quality-messages.cjs.github/scripts/pr-quality-state.cjs.github/scripts/pr-quality-state.test.cjs.github/scripts/pr-quality.cjs.github/scripts/pr-quality.test.cjs.github/workflows/enforce-pr-target.ymldocs-site/src/content/docs/contributing/pr-quality.mdstructure/06_docs-and-release.mdtests/ci-workflows.test.tstests/helpers/enforce-pr-target-harness.ts
Address the CodeRabbit re-review of the first fix round: - Remove pull_request_review / pull_request_review_comment triggers: review events load the workflow from the PR head branch (like pull_request), so the head-controlled workflow YAML ran under a write token against base-pinned scripts, crashing the gate (parseGateState is not a function) and breaking the trusted-base model. The findings claim still runs on every pull_request_target event. - .coderabbit.yaml: drop the positive labels filter, which would restrict ALL CodeRabbit reviews to labeled PRs and starve maintainer PRs (they never carry review-ready). The label remains a status marker only. - Persist draft ownership in the consolidated comment BEFORE convertToDraft so a successful convert followed by a failed comment write still leaves the bot-created draft owned and restorable; reflect a failed conversion by rewriting the comment with autoDraftedByBot:false. - Preserve bot draft/title-prefix ownership through checklist resets (head-drift and claim-check) instead of resetting to a fresh default. - Guard the maintainer recovery branch so it never calls markReadyForReview twice on the stale draft value. - Deduplicate the failure-path comment writes via a shared draftComment helper and a single failure-status-reason builder. - Docs: sync pr-quality.md and the structure workflow map (no review-event triggers; label is a status marker; review-body supplement documented). - Tests: cover the ownership-preserving reset, update wrong-base sequences for the persist-before-convert ordering, and pin the trigger allowlist. Gates: node --test .github/scripts (420 pass), bun test tests/ci-workflows (113 pass), bun run typecheck, bun run privacy:scan, actionlint clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/enforce-pr-target.yml (1)
1027-1029: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe READY notice tells the author the label requested a review; the code says the opposite.
Line 1028 renders:
CodeRabbit/Codex review was requested via the review-ready label.The comment at Lines 775-779 states the reverse — the label is "not a CodeRabbit auto-review filter" and is "kept as a visible status marker only". The test at.github/scripts/enforce-pr-target.test.cjsLines 97-101 pins that same contract by asserting the label is never wired as a review filter.Failure mode: a contributor reads the notice, believes the label triggered the review, and waits. No review was requested by the label. The fallback sentence in the same string ("If no review appears, comment
@coderabbitai review") softens it but still frames the label as the trigger.There is a second inaccuracy on the same line.
setReviewReadyLabelat Lines 200-225 swallows anaddLabelsfailure with a warning. The notice is then printed even when the label was never applied.State what the label is.
🐛 Proposed fix: describe the label as a status marker
readyMoment - ? `CodeRabbit/Codex review was requested via the ${inlineCode(REVIEW_READY_LABEL)} label. If no review appears, comment ${inlineCode("`@coderabbitai` review")} to request one.` + ? `This PR is marked with the ${inlineCode(REVIEW_READY_LABEL)} status label. The label does not request a review; if no CodeRabbit or Codex review appears, comment ${inlineCode("`@coderabbitai` review")} to request one.` : "",🤖 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 1027 - 1029, Update the readyMoment notice to describe REVIEW_READY_LABEL as a visible status marker, not as triggering or requesting a CodeRabbit/Codex review. Keep the explicit `@coderabbitai` review fallback, and ensure the notice is only emitted when setReviewReadyLabel successfully applies the label rather than when addLabels fails.
🤖 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 97-101: Update the assertion in the test covering CodeRabbit
configuration to read .coderabbit.yaml rather than the enforce-pr-target
workflow. Validate that reviews.auto_review contains no positive labels filter,
while preserving the existing intent of preventing a review-ready label
restriction that would exclude maintainer PRs.
In `@docs-site/src/content/docs/contributing/pr-quality.md`:
- Around line 43-46: Update the documentation around the maintainer screenshot
waiver to match the behavior enforced by hasGuiOverride and the workflow: either
scope the no-self-waive statement to contributor authors, or implement the
documented restriction by excluding the PR author from waiver comments before
guiOverrideComments and hasGuiOverride consume them. Keep the documentation and
workflow behavior consistent.
In `@tests/ci-workflows.test.ts`:
- Around line 1646-1649: Remove the duplicate adjacent const titleUpdates
declaration in the test callback for “a revalidation reset preserves bot
ownership of the title prefix,” leaving one declaration before its assertions.
Also inspect the callback containing createdComments for the same same-scope
duplicate and remove any repeated declaration, while preserving valid
declarations in separate test callbacks.
---
Outside diff comments:
In @.github/workflows/enforce-pr-target.yml:
- Around line 1027-1029: Update the readyMoment notice to describe
REVIEW_READY_LABEL as a visible status marker, not as triggering or requesting a
CodeRabbit/Codex review. Keep the explicit `@coderabbitai` review fallback, and
ensure the notice is only emitted when setReviewReadyLabel successfully applies
the label rather than when addLabels fails.
🪄 Autofix
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: ab742c5f-74f1-4960-9b57-6756f62d1570
📒 Files selected for processing (5)
.github/scripts/enforce-pr-target.test.cjs.github/workflows/enforce-pr-target.ymldocs-site/src/content/docs/contributing/pr-quality.mdstructure/06_docs-and-release.mdtests/ci-workflows.test.ts
… config Address the second CodeRabbit re-review round: - tests/ci-workflows.test.ts: remove the duplicated const titleUpdates declaration (parse error) in the ownership-preserving-reset test. - enforce-pr-target.test.cjs: assert .coderabbit.yaml's auto_review has no positive labels filter directly (the workflow never writes a labels block), so a future config change that starves maintainer PRs of reviews is caught. - docs: scope the self-waive sentence to contributor authors; a maintainer who authors the PR can waive (holds push permission, not checklist-gated). Gates: node --test .github/scripts (421 pass), bun test tests/ci-workflows (113 pass), bun run typecheck, bun run privacy:scan, actionlint clean.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/contributing/pr-quality.md (1)
63-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDescribe current-head review evidence accurately.
.github/scripts/pr-quality-state.cjs:204-275treats actionable findings in the latest CodeRabbit review body forliveHeadShaas unresolved independently of review-thread state. Therefore, the current wording is incorrect:
- “every ... review thread on the PR” is broader than the head-bound check.
- “while a bot review thread is open” implies that body findings depend on an open thread.
- “resolving every bot thread clears them” is false when the current-head review body still reports actionable findings.
Update this section to state that current-head Codex and CodeRabbit threads must be resolved, and that the latest CodeRabbit review body for the current head can independently keep readiness blocked.
Proposed wording
- Before a completion is accepted, the gate verifies the checklist claims it - can check itself: the head's `ci` check must be green, the branch must be on - the latest `dev` commit or at most 10 commits behind it, and every Codex and - CodeRabbit review thread on the PR must be resolved. CodeRabbit findings - that fall outside the diff range and are reported only in a review body on - the current head are counted the same way while a bot review thread is open; - resolving every bot thread clears them. A disproved claim unticks the + Before a completion is accepted, the gate verifies the checklist claims it + can check itself: the head's `ci` check must be green, the branch must be on + the latest `dev` commit or at most 10 commits behind it, and every Codex and + CodeRabbit review thread for the current head must be resolved. CodeRabbit + findings outside the diff range that are reported in the latest review body + for the current head are checked independently of review-thread state. A + disproved claim unticks the matching box and keeps the PR a draft.As per path instructions, user-facing documentation must stay synchronized with actual workflow behavior.
🤖 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/pr-quality.md` around lines 63 - 69, Update the completion-gate description to match the behavior implemented around the current-head review evidence: require resolution of Codex and CodeRabbit threads associated with the current head, and state that actionable findings in the latest CodeRabbit review body for liveHeadSha can independently keep readiness blocked, even after bot threads are resolved. Remove wording that implies all PR threads are evaluated or that body findings depend on an open thread.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.
Outside diff comments:
In `@docs-site/src/content/docs/contributing/pr-quality.md`:
- Around line 63-69: Update the completion-gate description to match the
behavior implemented around the current-head review evidence: require resolution
of Codex and CodeRabbit threads associated with the current head, and state that
actionable findings in the latest CodeRabbit review body for liveHeadSha can
independently keep readiness blocked, even after bot threads are resolved.
Remove wording that implies all PR threads are evaluated or that body findings
depend on an open thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3354210f-b8af-4e34-becb-73dcf2cb5dff
📒 Files selected for processing (3)
.github/scripts/enforce-pr-target.test.cjsdocs-site/src/content/docs/contributing/pr-quality.mdtests/ci-workflows.test.ts
Collaborator
Author
|
[GD] Addressed feedback commit: b222002 feedbacks:
|
Collaborator
Author
[GD] Verdict: approve-commentTLDR
Full verdictSemantic propagation
Linked: none UsefulnessThe PR hardens the PR-readiness gate around the four-box checklist: one consolidated bot comment replaces the two-comment model (always-current status + "what to do" + checklist mirror + draft reason), the findings claim verifies the Codex/CodeRabbit box against head-bound review threads (fail-closed, paginated, with the outside-diff supplement), and a maintainer comment can waive the screenshot gate. These are real improvements to contributor flow, with unusually strong test infrastructure (the harness compiles and runs the actual workflow script). Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification
Gatenone Bottom lineShip it. The original PR was useful but shipped a critical trigger bug (inert review events) and several correctness defects; the review + simplify pass fixed all of them with regression coverage, kept the gate's security model intact by removing the unsafe review-event triggers, and synced the docs. Head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2220024fe
ℹ️ 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".
…mments, docs accuracy Address the Codex re-review round on the consolidated gate: - Checklist-only draft path: persist the ownership checkpoint (autoDraftedByBot true) in the consolidated comment BEFORE convertToDraft, matching the failure path, so a successful convert followed by a failed comment write still leaves the bot-created draft owned and restorable. A failed conversion rewrites the comment to release ownership. An already-draft PR still updates the comment. - Maintainer gate-comment clear: a maintainer PR that was already a draft while it failed a gate (active:true, autoDraftedByBot:false) now clears its stale DRAFT comment to READY once the failures pass, instead of leaving the old wrong-base/description actions up. - Docs (pr-quality.md): describe the findings claim accurately - bot-authored Codex/CodeRabbit threads on the current head must be resolved; the CodeRabbit review-body supplement adds to the count while a bot thread is open, and resolving every bot thread clears the box (it cannot independently block). Tests: node --test .github/scripts (421 pass), bun test tests/ci-workflows (113 pass), bun run typecheck, bun run privacy:scan, actionlint clean.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/enforce-pr-target.yml (1)
936-1003: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the repeated DRAFT comment options into one builder.
Lines 939-951, 958-970, and 990-1002 build the same
upsertGateCommentoptions object three times. Thestatus,statusReasonternary,actions: buildActions(),readiness, andchecklistRequiredfields are byte-identical across all three. Only the last entry ofnoticesdiffers.The ownership and conversion logic itself is correct. I traced all three paths:
pr.draft === true: line 936 writes once, line 988 is skipped because!pr.draftis false.!pr.draftand conversion succeeds: line 957 claims ownership, the comment persistsautoDraftedByBot: true, line 988 is skipped.!pr.draftand conversion throws: line 975 releases ownership, line 989 releases it again, and line 990 rewrites the comment with the manual-conversion notice.The risk is maintenance, not behavior. A future edit to
statusReasonoractionsmust land in three places. One miss produces a gate comment whose status line contradicts its own action list, in a file the path instructions mark as a security boundary.♻️ Proposed refactor: one options builder for the checklist-draft paths
// No quality failure; the draft is owed by the open checklist. + const checklistDraftOptions = (lastNotice) => ({ + status: "DRAFT", + statusReason: checklistRequired + ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` + : "PR is kept in draft.", + actions: buildActions(), + readiness, + checklistRequired, + notices: [...revalidationNotice, lastNotice] + }); + const STAYS_DRAFT_NOTICE = + "This PR stays in draft until every box above is ticked."; + const MANUAL_DRAFT_NOTICE = + "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked."; + if (pr.draft) { - // Already a draft: update the gate comment with the open - // checklist status and no conversion needed. - await upsertGateComment(state, { - status: "DRAFT", - statusReason: checklistRequired - ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` - : "PR is kept in draft.", - actions: buildActions(), - readiness, - checklistRequired, - notices: [ - ...revalidationNotice, - "This PR stays in draft until every box above is ticked." - ] - }); + // Already a draft: update the gate comment with the open + // checklist status and no conversion needed. + await upsertGateComment( + state, + checklistDraftOptions(STAYS_DRAFT_NOTICE) + ); } else if (!draftConverted) { // Persist the ownership checkpoint BEFORE the mutation so a // successful convert followed by a failed comment write still // leaves the bot-created draft owned and restorable. The // failure path above uses the same ordering via draftComment. state.autoDraftedByBot = true; - await upsertGateComment(state, { - status: "DRAFT", - statusReason: checklistRequired - ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` - : "PR is kept in draft.", - actions: buildActions(), - readiness, - checklistRequired, - notices: [ - ...revalidationNotice, - "This PR stays in draft until every box above is ticked." - ] - }); + await upsertGateComment( + state, + checklistDraftOptions(STAYS_DRAFT_NOTICE) + ); try { await convertToDraft(); draftConverted = true; } catch (error) { state.autoDraftedByBot = false; core.warning( `Could not convert pull request to draft: ${error.message}` ); core.setFailed( "PR quality gate failed: could not convert the pull request to draft while the review readiness checklist is open." ); } } // A failed conversion must still surface in the persisted state: // the checkpoint above claimed ownership, so a failure rewrites // it to release ownership and tell the author what to do. if (!draftConverted && !pr.draft) { state.autoDraftedByBot = false; - await upsertGateComment(state, { - status: "DRAFT", - statusReason: checklistRequired - ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` - : "PR is kept in draft.", - actions: buildActions(), - readiness, - checklistRequired, - notices: [ - ...revalidationNotice, - "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." - ] - }); + await upsertGateComment( + state, + checklistDraftOptions(MANUAL_DRAFT_NOTICE) + ); }Note that this changes no call ordering, so the mutation-contract assertions in
tests/ci-workflows.test.tsstay valid.🤖 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 936 - 1003, Extract the shared DRAFT comment fields from the three upsertGateComment calls into one local builder near the checklist-draft logic, parameterized only by the differing final notice (while preserving revalidationNotice and the existing notice text). Replace all three duplicated options objects with calls to this builder, without changing ownership updates, conversion ordering, or error handling.
🤖 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.
Outside diff comments:
In @.github/workflows/enforce-pr-target.yml:
- Around line 936-1003: Extract the shared DRAFT comment fields from the three
upsertGateComment calls into one local builder near the checklist-draft logic,
parameterized only by the differing final notice (while preserving
revalidationNotice and the existing notice text). Replace all three duplicated
options objects with calls to this builder, without changing ownership updates,
conversion ordering, or error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aef5c1ee-d64d-4fb3-a339-532182c7cd86
📒 Files selected for processing (3)
.github/workflows/enforce-pr-target.ymldocs-site/src/content/docs/contributing/pr-quality.mdtests/ci-workflows.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ecc79c521
ℹ️ 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".
Comment on lines
+421
to
+423
| // A maintainer issue comment ("not touching gui") waives the | ||
| // GUI-screenshot gate; the comments are already fetched above. | ||
| guiOverrideComments: comments |
There was a problem hiding this comment.
This workflow now changes enforce-target behavior by accepting a maintainer comment as a GUI-screenshot waiver, and the same commit adds findings-claim verification/review-ready status behavior, but AGENTS.md and MAINTAINERS.md still describe the old policy (mandatory screenshots for any gui mention and only two verified checklist claims). That leaves contributors and reviewers following contradictory branch-enforcement rules; update those policy docs along with the public guide before landing this gate change.
AGENTS.md reference: .github/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Comment on lines
+421
to
+423
| // A maintainer issue comment ("not touching gui") waives the | ||
| // GUI-screenshot gate; the comments are already fetched above. | ||
| guiOverrideComments: comments |
There was a problem hiding this comment.
This workflow now changes enforce-target behavior by accepting a maintainer comment as a GUI-screenshot waiver, and the same commit adds findings-claim verification/review-ready status behavior, but AGENTS.md and MAINTAINERS.md still describe the old policy (mandatory screenshots for any gui mention and only two verified checklist claims). That leaves contributors and reviewers following contradictory branch-enforcement rules; update those policy docs along with the public guide before landing this gate change.
Useful? React with 👍 / 👎.
Comment on lines
+192
to
+193
| const GUI_OVERRIDE_RE = | ||
| /\b(?:no|not|doesn'?t|does not|never|without)\b[^.!?\n]{0,40}?\bgui\b/i; |
There was a problem hiding this comment.
This waiver regex also matches maintainer comments that are asking for a screenshot, such as Do not merge without a gui screenshot or This is not enough; add a gui screenshot, because it only looks for any negation before gui. In those review-comment scenarios the missing-screenshot failure is cleared even though the maintainer explicitly requested evidence; restrict the match to explicit no-GUI-change wording or a deliberate waiver command.
Useful? React with 👍 / 👎.
| unresolved += 1; | ||
| } | ||
| } | ||
| if (unresolved > 0) { |
There was a problem hiding this comment.
When CodeRabbit posts only current-head outside-diff actionable findings and no inline bot thread, this guard skips coderabbitOutsideDiffFindings() because unresolved is still zero, so the contributor can tick the findings box and be marked ready without addressing those CodeRabbit findings. Fresh evidence: the supplement is now only called after an inline unresolved bot thread already exists; count current-head outside-diff review-body findings independently and let a new head or a later zero-finding review clear them.
Useful? React with 👍 / 👎.
3 tasks
This was referenced Aug 6, 2026
Closed
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enforce-pr-target.yml) around the four-box readiness checklist.review-readylabel marks the ready moment as a status marker (it is not wired as a CodeRabbit auto-review filter, which would restrict all reviews to labeled PRs and starve maintainer PRs).Verification
node --test .github/scripts/*.test.cjs— 420 passbun test tests/ci-workflows.test.ts— 113 pass (consolidated comment, label add/remove, legacy migration + OR ownership, one write per run, outside-diff supplement, screenshot waiver, ownership-preserving reset, review-threads pagination)bun run typecheck— passbun run privacy:scan— passactionlint— cleanSummary by CodeRabbit
New Features
review-readylabeling when all checks pass.Documentation