Gate external PRs on an assigned, linked issue - #3291
Conversation
cb39e81 to
d618e7b
Compare
![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}
| 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.
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. Anyone with triage or better, bots and drafts are exempt; reopening a PR, removing the control label, or adding the bypass label is a sticky maintainer override. PRs numbered below 3200 predate the gate and are only evaluated on manual dispatch. It is live from merge; setting the PR_GATE_ENFORCE repository variable to "false" turns it log-only. The rules live in .github/scripts/pr_intake_gate.js with scenario tests beside it (run in the checks job); the workflow file is triggers, routing and a checkout + require. Adapted from PrefectHQ/fastmcp's require-issue-link.yml (itself from langchain). CONTRIBUTING.md is rewritten around the policy (issues are the contribution; how PRs get in; who we'd love to hear from), AGENTS.md points agents at it, and the repo-level PR template is the org template plus a short note about the gate. No-Verification-Needed: workflow, its script and tests, and docs only Signed-off-by: Max Isbey <224885523+maxisbey@users.noreply.github.com>
8f0d415 to
1e7b8d9
Compare
| contains(github.event.pull_request.labels.*.name, 'missing-issue-link') || | ||
| github.event.action == 'unlabeled' | ||
| ) && | ||
| (github.event.action != 'unlabeled' || github.event.label.name == 'missing-issue-link') && |
There was a problem hiding this comment.
🟡 nit: the bypass override has no revoke path: routing drops unlabeled events for bypass-issue-check (line 58 only lets missing-issue-link removals through), so a maintainer removing the bypass label to put a PR back under the gate triggers no re-evaluation; and the script's unlabeled-override branch (pr_intake_gate.js lines 79-81) never reads payload.label — its comment even hardcodes the assumption that unlabeled runs are only for LABEL — so simply widening the routing would make a trusted maintainer's bypass removal be read as a sticky override that re-adds bypass-issue-check. Distinct from the pending labeled/missing-issue-link routing finding: that is the labeled-side clause on line 59; this is the unlabeled-side clause plus the script-side label assumption, and neither mini
Extended reasoning...
A maintainer reopens an outsider's PR to discuss it (or adds bypass-issue-check by hand); the sticky override applies the bypass label. They later decide the PR should go through the gate after all and remove bypass-issue-check — the natural undo of the override the gate's own comment advertises. The workflow job if: at line 58 evaluates (action != 'unlabeled' || label.name == 'missing-issue-link') to false for that event, so no run starts and the PR silently stays open and permanently exempt: nothing re-evaluates it until the author happens to edit the description or mark it ready for review, which for an abandoned or spam PR may be never. The maintainer's only working recourse is to close the PR manually. Fixing routing alone is not enough: with the event routed, evaluate() at pr_intake_gate.js:79-81 sees action === 'unlabeled' and a trusted sender and returns pass(..., { sticky: true }), immediately re-adding the bypass label the maintainer just removed — the script must also check which label the event removed.
Verification: nit — the described gap is real but self-heals lazily. Workflow line 58 (github.event.action != 'unlabeled' || github.event.label.name == 'missing-issue-link') drops unlabeled events for bypass-issue-check, so removing the bypass label starts no run and the PR is not immediately re-gated. And the fix is not a one-line routing widen: pr_intake_gate.js lines 79-81 treat any trusted-sender `unl
| await mutate(`reopen PR #${pr.number}`, () => github.rest.pulls.update({ owner, repo, pull_number: pr.number, state: 'open' })); | ||
| return true; | ||
| } catch (e) { | ||
| if (e.status !== 422) throw e; |
There was a problem hiding this comment.
🟡 nit: reopen() only self-heals the 422 case — a non-422 error (5xx/rate-limit) from pulls.update in the sticky trusted-unlabel path rethrows after the control label is already gone, leaving the PR closed with only bypass-issue-check, a state step 0 (line 69) then disowns as 'closed by someone else' on every later event.
Extended reasoning...
A triage+ user removes missing-issue-link from a gate-closed PR. evaluate() takes the sticky override: pass() adds bypass-issue-check (line 104), then calls reopen() (line 105). GitHub returns a transient 502/500 or secondary-rate-limit error from pulls.update; reopen()'s catch rethrows because status !== 422 (line 220), so the label re-add + cannot-reopen comment in the 422 branch (lines 222-223) never run and the workflow run fails. The PR is now closed, carries only BYPASS_LABEL, and lacks LABEL. On every subsequent event — the author editing the description, a maintainer adding the bypass label again, or a manual workflow_dispatch — evaluate() computes gated=false (no LABEL, action not 'unlabeled') and returns at line 69 ('closed by someone else — not ours') before the bypass check at line 82, so none of the recovery paths the gate comment and CONTRIBUTING.md advertise ever reopen it. The still-present gate comment falsely promises 'this one will be reopened'; only a maintainer manually clicking Reopen (or re-running the failed run) recovers it. Fix at the root: re-add LABEL
Verification: nit — .github/scripts/pr_intake_gate.js line 220: if (e.status !== 422) throw e; — in the sticky trusted-unlabel path (line 79-80 → pass() adds BYPASS_LABEL at line 104 → reopen() at line 105), a non-422 error from pulls.update rethrows before the recovery branch (lines 222-223) re-adds the control label, so the PR ends closed with only bypass-issue-check. Line 63 then computes gated=false on ev
| maintainer: { type: 'User', perms: { admin: true, maintain: true, push: true, triage: true, pull: true } }, | ||
| triager: { type: 'User', perms: { triage: true, pull: true } }, // e.g. a trusted-contributors team | ||
| outsider: { type: 'User', perms: { pull: true } }, | ||
| another: { type: 'User', perms: { pull: true } }, |
There was a problem hiding this comment.
🟡 [quality] dead test fixture: the another entry in PEOPLE is never referenced by any scenario or helper — leftover dead code
Extended reasoning...
Concrete cost: dead code left behind. another: { type: 'User', perms: { pull: true } } at .github/scripts/pr_intake_gate.test.js:24 is not used anywhere in the file (no scenario uses 'another' as a PR author, issue assignee, or event sender; PEOPLE is only consulted for PR authors and permission lookups). It duplicates outsider exactly, suggesting to a reader that some scenario distinguishes two untrusted users when none does, and it will silently rot as scenarios evolve. Delete the entry, or add the missing scenario it was presumably meant for (e.g. an issue assigned to a different untrusted user than the PR author does not reopen the PR).
Verification: nit — /home/claude/python-sdk/.github/scripts/pr_intake_gate.test.js:24 defines another: { type: 'User', perms: { pull: true } } in the PEOPLE fixture, and a whole-file grep for another finds only that one line — no scenario, event helper, or assertion ever uses it as a PR author, issue assignee, or event sender. It is byte-for-byte identical in capability flags to outsider on line 23, so it
| ( | ||
| github.event_name == 'pull_request_target' && | ||
| ( | ||
| github.event.pull_request.number >= 3200 || |
There was a problem hiding this comment.
🔴 Grandfathering floor 3200 is stale: the repo's issue/PR counter already passed 3315 before this commit (merges #3315/#3296/#3223 predate it in history), so open PRs numbered 3200–3315+ that predate the gate are routed into it and retroactively closed, contradicting the header's own claim (line 15) that pre-gate PRs are ignored.
Extended reasoning...
An outsider opened PR #3280 on 2026-08-13, before the gate merged, under the old CONTRIBUTING rules that did not require being assigned to a linked issue. After this change lands, the author edits the PR description (or marks the draft ready for review). The routing if: admits the event because 3280 >= 3200, evaluate() finds no qualifying assigned/help-wanted issue link, and the gate labels the pre-gate PR missing-issue-link, posts the closure comment, and closes it — retroactive enforcement the floor was explicitly written to prevent ('PRs below the number in the job if: predate the gate and are ignored'). Fix is to bump the floor to the counter value current at merge time (>3320).
Verification: normal — the grandfathering floor is demonstrably stale and nothing else guards the window. Line 54 of .github/workflows/require-linked-issue.yml admits github.event.pull_request.number >= 3200, and the header (line 14: "PRs below the number in the job if: predate the gate and are ignored"; lines 45-46: "grandfathering floor") documents 3200 as the pre-gate boundary. But git history shows
| github.event.action == 'unlabeled' | ||
| ) && | ||
| (github.event.action != 'unlabeled' || github.event.label.name == 'missing-issue-link') && | ||
| (github.event.action != 'labeled' || github.event.label.name == 'bypass-issue-check') |
There was a problem hiding this comment.
🟡 nit: routing drops labeled events for missing-issue-link (line 59 only admits bypass-issue-check), so a maintainer hand-applying the gate's own label does nothing — the PR stays open while wearing a label whose gate-created description says "Auto-closed: PR needs a linked issue assigned to its author"
Extended reasoning...
A triager sees an outsider PR with no linked issue (e.g. a pre-floor PR below 3200, which per the header is otherwise only reachable via manual gh workflow run) and applies the missing-issue-link label, reasonably expecting the automation to comment and close it — the label's description, set by ensureLabelExists() in pr_intake_gate.js (line 247), advertises exactly that. The labeled event is filtered out by line 59 (github.event.action != 'labeled' || github.event.label.name == 'bypass-issue-check'), so no run happens: the PR remains open indefinitely, mislabeled as auto-closed, with no explanatory comment for the author. The script would handle the event correctly if routed — pr_intake_gate.js line 63 treats a labeled PR as gate-managed and fail() would comment and close it — and it later does exactly that on the next edited event (line 55 lets the now-labeled PR through), so the PR is abruptly closed at a later, unrelated edit instead of when the maintainer acted. Fix is one clause in the routing: also admit labeled events for missing-issue-link.
Verification: nit — the mechanics check out. /home/claude/python-sdk/.github/workflows/require-linked-issue.yml line 59 reads (github.event.action != 'labeled' || github.event.label.name == 'bypass-issue-check'), so a labeled event whose label is missing-issue-link fails the job if: and no run starts; no other workflow reacts to that label either. A maintainer hand-applying missing-issue-link therefor
| getLabel: async () => ({ data: {} }), | ||
| createLabel: async ({ name }) => { write(`create label ${name}`); }, |
There was a problem hiding this comment.
🟡 [quality] nit: the fake's getLabel unconditionally succeeds, so ensureLabelExists's 404 -> createLabel -> 422-tolerance branch (pr_intake_gate.js lines 250-259) is unreachable in every scenario, and the createLabel stub with its write() tracking at line 351 is dead code
Extended reasoning...
Concrete cost: the label-creation path is the one path guaranteed to run on the gate's first real enforced failure in production (creating missing-issue-link/bypass-issue-check with the intended color and description, plus the concurrent-create 422 swallow), yet no test can ever execute it — while the fake's createLabel stub, which records a 'create label' write no scenario asserts, gives readers false confidence it is covered. Making the fake's getLabel 404 for labels the world doesn't declare (mirroring the real API) would exercise creation in the first fail-path scenario for free; as written, a regression in ensureLabelExists (e.g. the meta[0]/meta[1] lookup or the 422 swallow) would pass the suite and only surface as a failed workflow run on the first live gate closure.
Verification: nit — In the fake client, .github/scripts/pr_intake_gate.test.js line 350 is getLabel: async () => ({ data: {} }), which succeeds unconditionally, so the 404 catch in ensureLabelExists (.github/scripts/pr_intake_gate.js lines 250-259) — the createLabel call with the intended color/description and the 422 concurrent-create swallow — is unreachable in every scenario. Consequently the `createLa
| } | ||
| function dispatch(number, sender) { | ||
| return { eventName: 'workflow_dispatch', prNumber: number, payload: { inputs: { pr_number: String(number) }, sender: { login: sender } } }; | ||
| } |
There was a problem hiding this comment.
🟡 [quality] nit: dispatch() helper builds payload.inputs.pr_number, but the gate never reads payload.inputs — it reads the PR_NUMBER_INPUT env var that run() sets from event.prNumber, so the payload field is dead fixture data
Extended reasoning...
Readers of the test are misled into thinking the workflow_dispatch path consumes context.payload.inputs; if the script's input handling ever changes, this dead field keeps the test green while documenting a wire format the gate does not use. Dropping the inputs object from the fake payload removes the misleading dead code.
Verification: nit — pr_intake_gate.test.js:267 builds payload: { inputs: { pr_number: String(number) } }, but the gate reads the number only from process.env.PR_NUMBER_INPUT (pr_intake_gate.js:46), which run() sets from event.prNumber (test line 272). No code in the gate or test harness references payload.inputs; grep of pr_intake_gate.js shows the workflow_dispatch branch uses only the env var plus `pa
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, removes the label, or addsbypass-issue-check→ sticky override.workflow_dispatch; from 3200 up a PR is evaluated on its next event. Once labeled, a PR is managed normally regardless of number.PR_GATE_ENFORCEtofalseis a kill switch: runs then logPASS/FAILand[dry-run] would …lines and mutate nothing.Differences from the fastmcp version (also listed in the file header): the rules live in
.github/scripts/pr_intake_gate.js(with scenario tests beside it) and the workflow is just triggers and routing; one script serves all 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; linked issues must be open and still in this repo; gated PRs are found with the list API rather than Search; reopen happens before the label is removed and a refused reopen is explained rather than guessed at; trust from capability flags rather than role-name strings; the waiver label is our existinghelp 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?
.github/scripts/pr_intake_gate.test.js: 26 named scenarios (no link; link tohelp wanted; maintainer reopening their own vs a gated PR; triage-role and bot label removal; the bypass label; Dependabot; hand-closed and merged PRs; closed/transferred/cross-repo/PR-number references; refused reopen and the retry after it; issue assignment incl. past the reference cap; dispatch backfill; drafts; a planted marker comment) plus kill-switch and fail-safe cases, run in CI on Node 24.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:
PR_GATE_ENFORCE=falsepauses it.help wanted/ready for worklabel descriptions to match CONTRIBUTING.md.gh workflow run require-linked-issue.yml -f pr_number=<n>.AI Disclaimer