Gate external PRs on an assigned, linked issue - #3291
Conversation
Unsolicited pull requests now outnumber issues four to one and almost none are reviewable in the time we have. This adds a workflow that closes an external PR unless its description links an open issue the author is assigned to (or one labeled "help wanted"), and reopens it automatically once a maintainer assigns them. Maintainers, triage-role collaborators, bots and drafts are exempt; reopening a PR or removing the control label is a sticky maintainer override. PRs numbered below 3200 predate the gate and are only evaluated on manual dispatch. The workflow is adapted from PrefectHQ/fastmcp's require-issue-link.yml (itself from langchain), restructured into a single script that always reads PR state live, requires linked issues to be open and in this repo, finds gated PRs via the list API rather than search, and diagnoses a refused reopen instead of guessing. It ships in dry-run: set the PR_GATE_ENFORCE repository variable to "true" to enforce. CONTRIBUTING.md is rewritten around the policy (issues are the contribution; how PRs get in; who we want to hear from), AGENTS.md gains an agent-facing statement of it, and a short repo-level PR template leads with the Fixes line the gate looks for. No-Verification-Needed: workflow and docs only; exercised with a mock harness, actionlint and zizmor Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
Review feedback on the intake gate: - The rules now live in .github/scripts/pr_intake_gate.js, structured as entry points -> numbered rules -> outcomes -> helpers, and the workflow file is reduced to triggers, routing and a checkout + require. Dropped the refused-reopen diagnosis, comment minimizing and the check re-run in favour of one honest message, delete-on-pass, and not failing the check on a verdict, which also removes the actions: write permission. - CONTRIBUTING.md is reworded to be clear without being combative, is explicit that mentoring depends on maintainer capacity, and its new headings follow the file's existing title case. - AGENTS.md just points agents at CONTRIBUTING.md. - The PR template is the org template plus a short note about the gate, a Fixes line, and two checklist items. No-Verification-Needed: workflow and docs only; exercised with a mock harness, actionlint and zizmor Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
cb39e81 to
d618e7b
Compare
…ntions No-Verification-Needed: docs only Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
- .github/scripts/pr_intake_gate.test.js drives the gate through 22 end-to-end scenarios against an in-memory fake of the GitHub API and checks what it leaves behind (PR state, labels, its comment), plus a dry-run and a fail-safe case. Zero dependencies; runs under node --test and from pre-commit whenever the gate script changes. - Leave merged PRs alone even if they still carry the control label. - Mention the bypass label in the bot comment's maintainer footer, since adding it is the one override that can't lose a race with another event. - Clarify the refused-reopen comment in the code and note in the workflow header that pull_request_target runs from the default branch regardless of the PR's base, so v1.x PRs are covered. No-Verification-Needed: workflow script, its tests, and docs only Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
A pre-commit hook would make node a requirement for anyone running pre-commit --all-files locally, which CONTRIBUTING.md asks Python contributors to do. No-Verification-Needed: CI wiring only Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… managed Review follow-ups: - Adding `bypass-issue-check` is now a real override: the workflow listens for that `labeled` event (and only that label), so the bot comment's third suggestion works without an accompanying reopen. - When GitHub refuses to reopen a PR, put the control label (back) on so the PR stays gate-managed and a later edit or override retries, rather than falling out of scope after a label-removal override. - On assignment, check the just-assigned issue first so the reference cap can't hide it. - Tolerate a concurrent delete of the gate comment, as label removal already does. - Run the scenario tests on Node 24 to match the github-script runtime. Four scenarios added for the above. No-Verification-Needed: workflow script, its tests, and CI wiring only Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
| const refs = closingRefs(pr.body); | ||
| if (hintIssue && refs.includes(hintIssue)) refs.unshift(...refs.splice(refs.indexOf(hintIssue), 1)); | ||
| const linked = []; | ||
| for (const num of refs.slice(0, MAX_ISSUES)) { |
There was a problem hiding this comment.
🟡 A PR whose qualifying issue is the 6th+ closing reference is closed at opened/edited with no automatic recovery when the author was assigned before opening: the MAX_ISSUES cap (line 91) skips the qualifying ref, and since the author is already assigned, no issues: assigned event will ever fire to trigger the hint-reordering path (line 89) that exists for exactly this case. Distinct from the resolved prefilter-mismatch finding: that was fixed by the hint reorder, which only helps when assignment happens after gate closure.
Extended reasoning...
A maintainer assigns issue #106 to a contributor, who opens a PR whose description reads 'Fixes #101, fixes #102, ... fixes #106' (six closing refs, the assigned one last — e.g. a cleanup PR resolving several related reports). evaluate() fetches only refs.slice(0, 5), none qualify, and fail() closes the PR telling the author "you aren't currently assigned to #101..#105" and that it "will be reopened" once a maintainer assigns them — but the assignment already happened, so the promised issues: assigned re-evaluation never fires, and editing the description re-runs with the same first-five cap. The correctly-assigned PR stays closed with a misleading comment until someone guesses to reorder the refs or a maintainer manually overrides.
Verification: nit — the mechanics are all real in /home/claude/python-sdk/.github/scripts/pr_intake_gate.js. On opened/edited, evaluate() is called with hintIssue = null (line 52), so line 89's reorder does nothing and line 91 for (const num of refs.slice(0, MAX_ISSUES)) fetches only the first 5 closing refs (MAX_ISSUES = 5, line 22). If the author's assigned issue is the 6th ref, the assignment check a
| // are checked again on ready_for_review). | ||
| 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'); |
There was a problem hiding this comment.
🟡 nit: the draft short-circuit (line 75) runs before the trusted-sender override (line 79) and the bypass-label check (line 82), so a maintainer's reopen of a gate-closed PR is silently discarded if the PR is a draft by the time the queued run executes — no sticky bypass-issue-check is applied and the missing-issue-link label and "closed" comment remain, so the gate re-closes the PR on ready_for_review despite the explicit override.
Extended reasoning...
A maintainer reopens a gate-closed outsider PR to accept it past the gate; while the reopened run sits in the Actions queue (seconds to minutes), the author converts the now-open PR to draft to keep polishing it. The run's live read sees draft=true and returns at line 75 without reaching the sticky-override branch, leaving missing-issue-link and the stale gate comment on the open draft. When the author marks it ready for review, evaluate() finds no bypass label, the rule fails, and fail() re-closes the PR the maintainer had deliberately reopened — the maintainer must override a second time with no indication why the first override vanished.
Verification: nit — the ordering is real: line 75 (if (pr.draft) return log('draft — skipped until ready for review');) runs before the trusted-sender reopen override at lines 79-81 and the BYPASS_LABEL check at line 82, and line 59 reads the PR live ("the event payload can be stale by the time a queued run starts"), so a maintainer's reopened run on a PR the author converted to draft during the queue del
| }); | ||
| const prs = closed.filter((i) => i.pull_request && closingRefs(i.body).includes(issueNumber)); | ||
| console.log(`#${issueNumber} assigned to ${assignee}: ${prs.length} gate-closed PR(s) reference it`); | ||
| for (const pr of prs) await evaluate(pr.number, 'assigned', context.payload.sender?.login, issueNumber); |
There was a problem hiding this comment.
🟡 nit: issues-assigned handler has no per-PR error isolation and no retry: one transient API error while evaluating the first gate-closed PR aborts the loop, so the remaining PR(s) — and on any error, the assigned PR itself — never reopen despite the bot comment promising automatic reopening with "nothing more you need to do"
Extended reasoning...
A maintainer assigns issue #N to an author with two gate-closed PRs referencing it (the exact shape of the 3300/3301 test scenario). The issues: assigned run enters for (const pr of prs) await evaluate(...) at line 41; while evaluating the first PR, a transient GitHub 5xx or secondary rate limit hits getCollaboratorPermissionLevel or issues.get — both are written to throw on any non-404 status (lines 181, 204) precisely so errors aren't misread as 'untrusted'. The throw propagates out of the loop, the run fails, and the second PR is never evaluated. Because issues: only triggers on [assigned], the event is one-shot: no later event re-fires for that PR, so it stays closed with the gate comment still telling the author "they'll assign you to #N and this PR will reopen automatically — there's nothing more you need to do." Recovery requires a maintainer to notice the red Actions run and manually workflow_dispatch each PR. Wrapping each evaluate in try/catch (collecting failures and calling core.setFailed at the end) would confine a transient failure to one PR instead
Verification: nit — line 41 (for (const pr of prs) await evaluate(...)) has no try/catch and no retry, and the error paths the candidate cites really do throw on any transient failure: isTrusted line 181 (throw new Error(Permission check failed...) for any non-404) and getIssue line 204 (throws for any non-404/410), per the deliberate fail-loud comment at lines 169–170. The workflow (require-linked-issue.
1 participant
Adds an intake gate for pull requests from outside the maintainer team: a PR stays open only if it links an open issue its author is assigned to (or one labeled
help wanted). Everything else is closed by a bot with an explanation and reopens automatically once a maintainer assigns the author. CONTRIBUTING.md is rewritten around that policy.Motivation and Context
Over the last six months this repo received ~640 pull requests from outside the maintainer team — 2.7× the previous six months — of which 24 were merged. 41% of newly opened issues now attract an external PR within 48 hours (median under 11 hours), the open-PR backlog is ~80% external, and CONTRIBUTING.md's existing "issue first, no drive-by agents" rules have no mechanical backing. Reviewing a PR properly costs the same as it always did; producing one no longer does. With the maintainer time we actually have, issues are the contribution we can use, and this makes the repo say so and behave accordingly.
The workflow is adapted from PrefectHQ/fastmcp's
require-issue-link.yml(which came from langchain's); pydantic and pydantic-ai run similar gates. Within this org, inspector has already gone issues-only and typescript-sdk restricted PR creation for a month in June for the same reason.Behaviour
Fixes/Closes/Resolvesan open issue in this repo where the author is an assignee, or the issue carrieshelp wanted. Otherwise:missing-issue-linklabel, one comment, closed.editedevent; anyone triage+ reopens the PR or removes the label → stickybypass-issue-check.workflow_dispatch; from 3200 up a PR is evaluated on its next event. Once labeled, a PR is managed normally regardless of number.PASS/FAILand[dry-run] would …lines and mutates nothing until the repository variablePR_GATE_ENFORCEis set totrue.Differences from the fastmcp version (also listed in the file header): one job and one script for all three entry points (PR events, issue assignment, manual dispatch) so admission and reopening can't drift; PR state is always read live rather than from the event payload, which closes a race where a queued run could strip the label without reopening and orphan the PR; linked issues must be open and still in this repo; gated PRs are found with the list API rather than Search; reopen-before-unlabel with the refused-reopen diagnosis above; trust from capability flags rather than role-name strings; the waiver label is our existing
help wanted.Docs
ready for workmeans queued for a maintainer, not an invitation)..github/pull_request_template.md: a short repo-level template that leads with theFixes #line the gate looks for, replacing the inherited org template here.How Has This Been Tested?
help wanted; maintainer reopening their own vs a gated PR; triage-role and bot label removal; Dependabot; hand-closed PRs; closed/transferred/cross-repo/PR-number references; both refused-reopen diagnoses; issue assignment; dispatch backfill; drafts; pass→fail comment un-minimizing; a planted marker comment), in enforce and dry-run modes.actionlintandzizmor --pedanticclean (thepull_request_targettrigger carries an inline justification; the workflow never checks out or executes PR code and interpolates nothing from the PR into the script).collaborators/{user}/permissioncapability flags for admins, outsiders and app logins;issues.listForRepowithcreator+labels+state=closed;minimizeComment/unminimizeComment) were checked against the live API, and the same permission set is what fastmcp's copy has been running with since May.Breaking Changes
None for SDK users. For contributors: PRs opened without an assigned, linked issue will be closed automatically once enforcement is on; CONTRIBUTING.md describes the path.
Types of changes
Checklist
Additional context
Rollout after merge:
help wantedfrom the closed issues that still carry it, and update thehelp wanted/ready for worklabel descriptions to match CONTRIBUTING.md.PR_GATE_ENFORCE=true(Settings → Secrets and variables → Actions → Variables).gh workflow run require-linked-issue.yml -f pr_number=<n>.AI Disclaimer