-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Gate draft PRs too and rewrite the auto-close comment #3378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b3a5054
4b53405
512ac39
ccd46b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,9 @@ | |
| # In short: a PR from someone without triage rights stays open only if it links | ||
| # an open issue here that is assigned to them (or labeled `help wanted`); | ||
| # otherwise it is labeled `missing-issue-link`, gets one comment, and is closed, | ||
| # and it reopens automatically once the author is assigned. Bots and drafts are | ||
| # skipped. A triage+ user reopening the PR, removing the label, or adding | ||
| # `bypass-issue-check` overrides. | ||
| # and it reopens automatically once the author is assigned. Drafts are gated | ||
| # too; bots are skipped. A triage+ user reopening the PR, removing the label, | ||
|
Comment on lines
+7
to
+8
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit: the PR description says "CONTRIBUTING.md gets one clause noting drafts are covered" and ticks the Documentation update box, but the diff never touches CONTRIBUTING.md (the last commit, ccd46b7, deliberately reverts that change), so after merge the drafts-are-gated policy is documented only in this workflow header while the file the auto-close comment links contributors to stays silent. Either restore the one-clause CONTRIBUTING.md edit or update the PR description so it matches what merges. Extended reasoning...The PR body claims: "CONTRIBUTING.md gets one clause noting drafts are covered", and checks "I have added or updated documentation as needed". git log for the range shows commits b3a5054..ccd46b7, the last titled "Leave CONTRIBUTING.md unchanged" — grep confirms CONTRIBUTING.md contains no mention of drafts at all. The gate's closed comment (pr_intake_gate.js:162) points authors at CONTRIBUTING.md#how-pull-requests-get-in for "the full reasoning", but that section never says drafts are gated; the only place the new draft policy is written down is the header comment at .github/workflows/require-linked-issue.yml:7-8, which contributors do not read. No runtime failure — the blanket rule in CONTRIBUTING.md covers all PRs by silence — but the merged state does not match what the PR description promises, so a reviewer approving based on the description would believe the contributor-facing doc was updated when it was not. Verification: nit — every factual link in the candidate checks out. (1) The PR description claims "CONTRIBUTING.md gets one clause noting drafts are covered" and ticks "Documentation update", but the net diff |
||
| # or adding `bypass-issue-check` overrides. | ||
| # | ||
| # Operating it: | ||
| # - Live by default. To pause it without a revert, set the repository | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit: now that drafts get gate-closed, a draft with a linked-but-unassigned issue receives the linked-issues comment saying "this PR reopens on its own and there's nothing more you need to do here" — false for drafts, which after reopening are still drafts and need the author to mark them ready before anyone reviews them. Only the opener (line 140) is draft-aware; the situation text isn't. Add a draft-conditional clause to the assigned-path text (e.g. "once reopened, mark it ready for review") so draft authors aren't told they're done.
Extended reasoning...
Before this PR,
if (pr.draft) return log(...)meant a draft could never reach fail() and closedComment() was only ever rendered for non-draft PRs, so "nothing more you need to do here" was accurate. After this change, fail() at line 124 passes pr.draft into closedComment(), but only the opener at lines 139-141 varies by draft; the linkedIssues.length branch at line 147 keeps the unconditional "this PR reopens on its own and there's nothing more you need to do here". Trigger: an outsider opens a draft PR withFixes #Nwhere #N is open but unassigned — the gate closes it with this comment. When a maintainer later assigns them, reopen() at line 240 reopens the PR still in draft state and deleteGateComment removes the comment; the PR then sits as an unreviewed draft because the author was explicitly told there was nothing more to do, when in fact they must click "ready for review" (which fires the ready_for_review event and re-passes). Consequence relative to base: base never showed this comment to draft authors, so the misleading promise is newly reachable. Fix: make the assigVerification: nit — the described text mismatch is real and newly reachable, but it is only slightly misleading wording; nothing breaks. In .github/scripts/pr_intake_gate.js, fail() at line 124 now passes
pr.draftintoclosedComment(pr.draft, linkedIssues), and drafts reach fail() because the diff removed the base branch'sif (pr.draft) return log('draft — skipped until ready for review')guard (old lines