Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions .github/scripts/pr_intake_gate.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,10 @@ module.exports = async function run({ github, context, core }) {
if (pr.merged_at) return log('merged — nothing to do');
if (pr.state === 'closed' && !gated) return log('closed by someone else — not ours');

// 1. Exempt authors: bots, anyone with triage or better, and drafts (which
// are checked again on ready_for_review).
// 1. Exempt authors: bots and anyone with triage or better. Drafts are
// gated like any other PR.
if (pr.user.type === 'Bot') return log('author is a bot — exempt');
if (await isTrusted(pr.user.login)) return pass('author has triage+ on this repo');
if (pr.draft) return log('draft — skipped until ready for review');

// 2. Overrides: a triage+ user reopening the PR or removing the label wants
// it open. Anyone else doing so just triggers a re-check.
Expand Down Expand Up @@ -122,7 +121,7 @@ module.exports = async function run({ github, context, core }) {
async function fail(linkedIssues) {
console.log(`FAIL: ${linkedIssues.length ? `not assigned to ${linkedIssues.map((n) => `#${n}`).join(', ')}` : 'no usable issue link'}`);
await addLabel(prNumber, LABEL);
await upsertGateComment(prNumber, closedComment(linkedIssues));
await upsertGateComment(prNumber, closedComment(pr.draft, linkedIssues));
if (pr.state === 'open') {
await mutate(`close PR #${prNumber}`, () => github.rest.pulls.update({ owner, repo, pull_number: prNumber, state: 'closed' }));
}
Expand All @@ -135,23 +134,37 @@ module.exports = async function run({ github, context, core }) {

// ── Comment text ─────────────────────────────────────────────────────────

function closedComment(linkedIssues) {
function closedComment(draft, linkedIssues) {
const issues = linkedIssues.map((n) => `#${n}`).join(', ');
const why = linkedIssues.length
? `you aren't currently assigned to ${issues}`
: "its description doesn't yet link an open issue in this repository (with `Fixes #123` or similar)";
const next = linkedIssues.length
? `If a maintainer would like this change as a PR from you, they'll assign you to ${issues} and this PR will reopen automatically — there's nothing more you need to do. (If you opened the issue, this PR already shows up on its timeline.)`
: `If there isn't an issue for this yet, please [open one](https://github.com/${owner}/${repo}/issues/new/choose) — a clear description of the problem is genuinely the most useful thing for us. Then add \`Fixes #<number>\` to this PR's description. If a maintainer would like the change as a PR from you, they'll assign you to the issue and this PR will reopen automatically.`;
const opener = draft
? "This PR has been closed automatically. It's still a draft, but we close those early so you don't put in more time only to have it closed the moment you mark it ready.\n\n"
: 'This PR has been closed automatically. ';
const rule = `${opener}This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue`;
const situation = linkedIssues.length
? [
`${rule}, and you aren't currently assigned to ${issues}.`,
'',
`If a maintainer assigns you to ${issues}, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take.`,

Copy link
Copy Markdown
Contributor

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 with Fixes #N where #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 assig

Verification: 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.draft into closedComment(pr.draft, linkedIssues), and drafts reach fail() because the diff removed the base branch's if (pr.draft) return log('draft — skipped until ready for review') guard (old lines

]
: [
`${rule}, and this PR doesn't link an open issue yet.`,
'',
"- **If you're already assigned to an issue for this**, add `Fixes #<n>` to the description and the PR will reopen on its own.",
`- **If there's no issue yet**, please [open one](https://github.com/${owner}/${repo}/issues/new/choose) instead: what you ran into, why it matters for your use case, and a minimal reproduction. That context is super important to us and is what we use to decide what to prioritise.`,
"- **If there's an issue but you're not assigned**, add `Fixes #<n>` anyway so they're linked, then engage on the issue itself by confirming the repro or describing the approach you'd take. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. If you are assigned, this PR reopens automatically.",
];
return [
MARKER,
`Thanks for the contribution. This repository only keeps pull requests open when they're linked to an issue that a maintainer has assigned to the author — [CONTRIBUTING.md](${contributingUrl}) explains why and how we work. This PR has been closed for now because ${why}.`,
...situation,
'',
next,
"You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way.",
'',
"There's no need to open a new PR — this one will be reopened. While it's closed, please push any updates as new commits rather than force-pushing, since GitHub can't reopen a PR whose branch has been rewritten.",
`[CONTRIBUTING.md](${contributingUrl}) has the full reasoning, but in short:`,
'',
`*Maintainers: reopening this PR, removing the \`${LABEL}\` label, or adding \`${BYPASS_LABEL}\` bypasses the check.*`,
"- We're a small team with very little capacity to review community PRs right now.",
'- Many recent PRs are AI-generated with little human review, and reviewing one carefully still costs a maintainer as much time as it ever did. A well-described issue is usually more useful to us than the code.',
'',
`*Maintainers: reopen, remove \`${LABEL}\`, or add \`${BYPASS_LABEL}\` to override.*`,
].join('\n');
}

Expand Down
13 changes: 7 additions & 6 deletions .github/scripts/pr_intake_gate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ const PEOPLE = {
// ── Scenarios ──────────────────────────────────────────────────────────────
// `prs` / `issues` describe the world before the event; `expect` describes each
// PR afterwards: state, labels, and comment ('closed' = the "this PR has been
// closed" comment, 'cannot-reopen' = the refused-reopen comment, null = none).
// closed" comment, 'closed-draft' = its draft wording, 'cannot-reopen' = the
// refused-reopen comment, null = none).
// `writes: 0` additionally asserts the gate touched nothing at all.

const scenarios = [
Expand Down Expand Up @@ -146,14 +147,13 @@ const scenarios = [
writes: 0,
},
{
name: 'draft PR is skipped until it is marked ready for review',
name: 'draft PR with no issue link is closed like any other, with a note about why drafts are closed early',
prs: [pr(3300, 'outsider', { draft: true })],
event: opened(3300, 'outsider'),
expect: { 3300: { state: 'open', labels: [], comment: null } },
writes: 0,
expect: { 3300: { state: 'closed', labels: [LABEL], comment: 'closed-draft' } },
},
{
name: 'draft marked ready for review with no link → closed',
name: 'pre-existing draft marked ready for review with no link → closed',
prs: [pr(3300, 'outsider')],
event: readyForReview(3300, 'outsider'),
expect: { 3300: { state: 'closed', labels: [LABEL], comment: 'closed' } },
Expand Down Expand Up @@ -299,7 +299,8 @@ function observe(world, expect) {
const p = world.prs.get(Number(num));
const gateComments = p.comments.filter((c) => c.user === 'github-actions[bot]' && c.body.includes('<!-- require-linked-issue -->'));
assert.ok(gateComments.length <= 1, `PR #${num} has ${gateComments.length} gate comments`);
const kind = !gateComments.length ? null : gateComments[0].body.includes("won't let it be reopened") ? 'cannot-reopen' : 'closed';
const body = gateComments[0]?.body;
const kind = !body ? null : body.includes("won't let it be reopened") ? 'cannot-reopen' : body.includes('still a draft') ? 'closed-draft' : 'closed';
out[num] = { state: p.state, labels: [...p.labels].sort(), comment: kind };
if ('foreignComments' in expect[num]) out[num].foreignComments = p.comments.length - gateComments.length;
}
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/require-linked-issue.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 git diff 57394b0..HEAD --stat touches only .github/scripts/pr_intake_gate.js, .github/scripts/pr_intake_gate.test.js, and .github/workflows/require-linked-issue.yml — CONTRIBUTING.md is net-unc

# or adding `bypass-issue-check` overrides.
#
# Operating it:
# - Live by default. To pause it without a revert, set the repository
Expand Down
Loading