Skip to content
Open
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
38 changes: 38 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<!--
Comment thread
maxisbey marked this conversation as resolved.
Pull requests from outside the maintainer team need to link an open issue that
a maintainer has assigned to you (or one labeled `help wanted`); others are
closed automatically until that's in place. See CONTRIBUTING.md for details.
-->

Fixes #

<!-- Provide a brief summary of your changes -->

## Motivation and Context
<!-- Why is this change needed? What problem does it solve? -->

## How Has This Been Tested?
<!-- Have you tested this in a real application? Which scenarios were tested? -->

## Breaking Changes
<!-- Will users need to update their code or configurations? -->

## Types of changes
<!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation update

## Checklist
<!-- Go over all the following points, and put an `x` in all the boxes that apply. -->
- [ ] I am assigned to the linked issue (or it is labeled `help wanted`, or I'm a maintainer)
- [ ] I have disclosed any AI assistance and can explain the change in my own words
- [ ] I have read the [MCP Documentation](https://modelcontextprotocol.io)
- [ ] My code follows the repository's style guidelines
- [ ] New and existing tests pass locally
- [ ] I have added appropriate error handling
- [ ] I have added or updated documentation as needed

## Additional context
<!-- Add any other context, implementation notes, or design decisions -->
289 changes: 289 additions & 0 deletions .github/scripts/pr_intake_gate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,289 @@
// PR intake gate. The policy lives in CONTRIBUTING.md ("How pull requests get
// in"); .github/workflows/require-linked-issue.yml wires this up to events.
//
// A pull request from someone without triage rights stays open only if its
// description links (Fixes/Closes/Resolves #N) an open issue in this repo that
// is either assigned to the PR author or labeled `help wanted`. Otherwise the
// gate labels it `missing-issue-link`, leaves one comment, and closes it. It
// re-evaluates — and reopens — the PR when the description is edited or the
// author is assigned to the issue. A triage+ user reopening the PR, removing
// the label, or adding `bypass-issue-check` overrides it, and the override
// sticks.
//
// Everything that writes goes through mutate(); when the workflow passes
// ENFORCE=false (its kill switch) the run only logs what it would have done.
'use strict';

const LABEL = 'missing-issue-link'; // marks PRs the gate has closed
const BYPASS_LABEL = 'bypass-issue-check'; // sticky maintainer override
const OPEN_LABEL = 'help wanted'; // issue label that waives assignment
const MARKER = '<!-- require-linked-issue -->';
const BOT_LOGIN = 'github-actions[bot]';
const MAX_ISSUES = 5;

module.exports = async function run({ github, context, core }) {
const { owner, repo } = context.repo;
const enforce = process.env.ENFORCE === 'true';
const contributingUrl = `https://github.com/${owner}/${repo}/blob/main/CONTRIBUTING.md#how-pull-requests-get-in`;

// ── Entry points ─────────────────────────────────────────────────────────

if (context.eventName === 'issues') {
// Someone was assigned an issue: re-evaluate their gate-closed PRs that
// reference it (they may pass now).
const issueNumber = context.payload.issue.number;
const assignee = context.payload.assignee.login;
const closed = await github.paginate(github.rest.issues.listForRepo, {
owner, repo, state: 'closed', creator: assignee, labels: LABEL, per_page: 100,
});
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);

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: 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.

return;
}

if (context.eventName === 'workflow_dispatch') {
const n = parseInt(process.env.PR_NUMBER_INPUT, 10);
if (!Number.isInteger(n) || n <= 0) throw new Error(`Bad pr_number input: ${process.env.PR_NUMBER_INPUT}`);
await evaluate(n, 'dispatch', context.payload.sender?.login);
return;
}

await evaluate(context.payload.pull_request.number, context.payload.action, context.payload.sender?.login);

// ── The rules ────────────────────────────────────────────────────────────

async function evaluate(prNumber, action, sender, hintIssue = null) {
// Always read the PR live; the event payload can be stale by the time a
// queued run starts.
const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber });
const labels = pr.labels.map((l) => l.name);
// An `unlabeled` run only fires for LABEL (see the workflow `if:`), so the
// event itself proves the label was there a moment ago.
const gated = action === 'unlabeled' || labels.includes(LABEL);
console.log(`PR #${prNumber} by ${pr.user.login} (${pr.state}${pr.draft ? ', draft' : ''}) — ${action} by ${sender ?? '-'}, enforce=${enforce}`);

// 0. Scope: open PRs, plus closed PRs the gate closed itself. Merged PRs
// and PRs someone closed for other reasons are left alone.
if (pr.merged_at) return log('merged — nothing to do');
if (pr.state === 'closed' && !gated) return log('closed by someone else — not ours');
Comment thread
maxisbey marked this conversation as resolved.

// 1. Exempt authors: bots, anyone with triage or better, and drafts (which
// 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');

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 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


// 2. Overrides: a triage+ user reopening the PR or removing the label wants
// it open. Anyone else doing so just triggers a re-check.
if ((action === 'reopened' || action === 'unlabeled') && sender && (await isTrusted(sender))) {
return pass(`${sender} ${action === 'reopened' ? 'reopened it' : 'removed the label'} — override`, { sticky: true });
}
Comment thread
maxisbey marked this conversation as resolved.
if (labels.includes(BYPASS_LABEL)) return pass(`carries ${BYPASS_LABEL}`);

// 3. The rule: the description links an open issue in this repo that is
// labeled `help wanted` or assigned to the author. Only the first few
// references are fetched; a just-assigned issue is checked first.
const author = pr.user.login.toLowerCase();
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)) {

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.

🟡 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

const issue = await getIssue(num);
if (!issue) continue; // missing, a PR, closed, or transferred away
linked.push(num);
if (issue.labels.some((l) => l.name.toLowerCase() === OPEN_LABEL)) return pass(`#${num} is labeled "${OPEN_LABEL}"`);
if (issue.assignees.some((a) => a.login.toLowerCase() === author)) return pass(`author is assigned to #${num}`);
}
return fail(linked);

// ── Outcomes ─────────────────────────────────────────────────────────

async function pass(reason, { sticky = false } = {}) {
console.log(`PASS: ${reason}`);
if (sticky) await addLabel(prNumber, BYPASS_LABEL);
if (pr.state === 'closed' && !(await reopen(pr, reason))) return;
if (gated) {
await removeLabel(prNumber, LABEL);
await deleteGateComment(prNumber);
}
}

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));
Comment thread
maxisbey marked this conversation as resolved.
if (pr.state === 'open') {
await mutate(`close PR #${prNumber}`, () => github.rest.pulls.update({ owner, repo, pull_number: prNumber, state: 'closed' }));
}
}

function log(msg) {
console.log(msg);
}
}

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

function closedComment(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.`;
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}.`,
'',
next,
'',
"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.",
'',
`*Maintainers: reopening this PR, removing the \`${LABEL}\` label, or adding \`${BYPASS_LABEL}\` bypasses the check.*`,
].join('\n');
}

function cannotReopenComment(pr, reason) {
return [
MARKER,
`This PR now passes the intake check (${reason}), but GitHub won't let it be reopened — usually because the branch was force-pushed or deleted while the PR was closed, or because another open PR uses the same branch.`,
'',
`If you have another open PR from this branch, please continue there. Otherwise, either push the branch back to \`${pr.head.sha.slice(0, 7)}\` and edit this PR's description to retry, or open a new PR with the same \`Fixes #<issue>\` line.`,
].join('\n');
}

// ── Helpers ──────────────────────────────────────────────────────────────

async function mutate(description, fn) {
if (!enforce) {
console.log(`[dry-run] would ${description}`);
return undefined;
}
return fn();
}

// Triage-or-better on this repo, from the permission endpoint's capability
// flags (role names can be custom; author_association hides private org
// members). Only a nonexistent user 404s; any other error must throw rather
// than be read as "untrusted", or a maintainer's PR could be closed.
async function isTrusted(username) {
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username });
const p = data.user?.permissions;
if (!p) throw new Error(`permission response for ${username} has no capability flags`);
const trusted = Boolean(p.triage || p.push || p.maintain || p.admin);
console.log(` ${username}: ${trusted ? 'trusted' : 'not trusted'} (role ${data.role_name || '-'})`);
return trusted;
} catch (e) {
if (e.status === 404) return false;
throw new Error(`Permission check failed for ${username} (HTTP ${e.status ?? '?'}): ${e.message}`);
}
}

// Issue numbers referenced with a closing keyword, in the forms GitHub itself
// honors: `Fixes #1`, `closes owner/repo#1`, `Resolved https://github.com/owner/repo/issues/1`.
function closingRefs(body) {
const repoRef = `${owner}/${repo}`.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const re = new RegExp(
`\\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\\s*:?\\s*(?:${repoRef}#|#|https?://github\\.com/${repoRef}/issues/)(\\d+)`,
'gi',
);
return [...new Set([...(body || '').matchAll(re)].map((m) => parseInt(m[1], 10)))];
}

// The linked issue, or null if it doesn't exist, is actually a PR, isn't
// open, or has been transferred to another repository.
async function getIssue(num) {
let issue;
try {
({ data: issue } = await github.rest.issues.get({ owner, repo, issue_number: num }));
} catch (e) {
if (e.status === 404 || e.status === 410) return null;
throw new Error(`Cannot fetch issue #${num} (HTTP ${e.status ?? '?'}): ${e.message}`);
}
if (issue.pull_request || issue.state !== 'open') return null;
if (!issue.repository_url?.endsWith(`/${owner}/${repo}`)) return null;
return issue;
}

// Reopen a gate-closed PR. GitHub refuses (422) if the branch was rewritten
// or deleted while closed, or another open PR uses it. Explain that in the
// comment and make sure the control label is (still) on, so the PR stays
// gate-managed and a later edit or override retries the reopen.
async function reopen(pr, reason) {
try {
await mutate(`reopen PR #${pr.number}`, () => github.rest.pulls.update({ owner, repo, pull_number: pr.number, state: 'open' }));
Comment thread
maxisbey marked this conversation as resolved.
return true;
} catch (e) {
if (e.status !== 422) throw e;

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: 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

core.warning(`GitHub refused to reopen PR #${pr.number}: ${e.message}`);
await addLabel(pr.number, LABEL);
await upsertGateComment(pr.number, cannotReopenComment(pr, reason));
return false;
}
}

async function addLabel(prNumber, name) {
await mutate(`add "${name}" to PR #${prNumber}`, async () => {
await ensureLabelExists(name);
await github.rest.issues.addLabels({ owner, repo, issue_number: prNumber, labels: [name] });
});
}

async function removeLabel(prNumber, name) {
await mutate(`remove "${name}" from PR #${prNumber}`, async () => {
try {
await github.rest.issues.removeLabel({ owner, repo, issue_number: prNumber, name });
} catch (e) {
if (e.status !== 404) throw e;
}
});
}

async function ensureLabelExists(name) {
const meta = {
[LABEL]: ['b76e79', 'Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md)'],
[BYPASS_LABEL]: ['0e8a16', 'Maintainer override for the linked-issue intake gate'],
}[name];
try {
await github.rest.issues.getLabel({ owner, repo, name });
} catch (e) {
if (e.status !== 404) throw e;
try {
await github.rest.issues.createLabel({ owner, repo, name, color: meta[0], description: meta[1] });
} catch (createErr) {
if (createErr.status !== 422) throw createErr; // created concurrently
}
}
}

// The gate keeps at most one comment per PR: authored by the Actions bot and
// carrying MARKER. It's created or updated on failure and deleted on pass.
async function findGateComment(prNumber) {
const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, per_page: 100 });
return comments.find((c) => c.user?.login === BOT_LOGIN && c.body?.includes(MARKER));
}

async function upsertGateComment(prNumber, body) {
const existing = await findGateComment(prNumber);
if (!existing) {
await mutate(`comment on PR #${prNumber}`, () => github.rest.issues.createComment({ owner, repo, issue_number: prNumber, body }));
} else if (existing.body !== body) {
await mutate(`update the gate comment on PR #${prNumber}`, () => github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }));
}
}

async function deleteGateComment(prNumber) {
const existing = await findGateComment(prNumber);
if (!existing) return;
await mutate(`delete the gate comment on PR #${prNumber}`, async () => {
try {
await github.rest.issues.deleteComment({ owner, repo, comment_id: existing.id });
} catch (e) {
if (e.status !== 404) throw e; // already deleted by a concurrent run
}
});
}
};
Loading
Loading