From 0deafec1b1e3016197c2cab69deada3e67d45126 Mon Sep 17 00:00:00 2001 From: Thomas Willson Date: Tue, 6 Oct 2026 14:54:35 -0700 Subject: [PATCH] AssignReviewers: find the PR below by branch, not the head filter In a repo that is a fork of another repo in the same org, the list endpoint's head: owner:branch filter finds nothing, so mid-stack PRs never inherited their assignees and got a fresh pick instead. goldengate-bms2000-string is a fork of stafl-bms2000-template; its #126 got different assignees from #125 below it. Match head branches across the repo's open PRs instead, leaving out PRs from forks. Co-Authored-By: Claude Opus 5.5 --- AssignReviewers/assign.js | 12 ++++++++-- AssignReviewers/assign.test.js | 40 ++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/AssignReviewers/assign.js b/AssignReviewers/assign.js index bfabb85..36dacaf 100644 --- a/AssignReviewers/assign.js +++ b/AssignReviewers/assign.js @@ -28,6 +28,10 @@ function humanLogins(users) { // // When a whole stack is submitted at once, every PR's run starts together. So if a ready PR below // has no assignees yet, wait for its own run to assign them before looking further down. +// +// The PR below is found by matching head branches across the repo's open PRs rather than with the +// list endpoint's `head: owner:branch` filter. In a repo that is a fork of another repo in the same +// org (goldengate-bms2000-string is a fork of stafl-bms2000-template), that filter finds nothing. async function inheritedAssignees(github, owner, repo, pr, author, { maxDepth, parentWaitMs, pollMs, sleep }) { let baseRef = pr.base.ref; const seen = new Set([pr.number]); @@ -35,9 +39,13 @@ async function inheritedAssignees(github, owner, repo, pr, author, { maxDepth, p const { data } = await github.rest.pulls.get({ owner, repo, pull_number: number }); return humanLogins(data.assignees).filter((login) => login !== author && login !== data.user.login); }; + // PRs from other repos (forks) can share a branch name with this repo's stack, so they're left out. + const thisRepo = `${owner}/${repo}`.toLowerCase(); + const open = (await github.paginate(github.rest.pulls.list, { owner, repo, state: 'open', per_page: 100 })).filter( + (p) => p.head.repo?.full_name.toLowerCase() === thisRepo, + ); for (let depth = 0; depth < maxDepth; depth++) { - const parents = await github.rest.pulls.list({ owner, repo, state: 'open', head: `${owner}:${baseRef}` }); - const parent = parents.data.find((p) => !seen.has(p.number)); + const parent = open.find((p) => p.head.ref === baseRef && !seen.has(p.number)); if (!parent) return null; seen.add(parent.number); let assignees = await assigneesOf(parent.number); diff --git a/AssignReviewers/assign.test.js b/AssignReviewers/assign.test.js index 345cdca..63553f9 100644 --- a/AssignReviewers/assign.test.js +++ b/AssignReviewers/assign.test.js @@ -7,8 +7,10 @@ const STAFF = ['staffA', 'staffB', 'staffC']; const EVERYONE = [...STAFF, 'devD', 'devE', 'devF']; // A fake GitHub: open PRs keyed by number, team membership, per-user load, and when each person -// was last assigned (`assigned`, login to ISO time). -function fakeGithub({ prs, load = {}, assigned = {} }) { +// was last assigned (`assigned`, login to ISO time). A PR's `headRepo` is the repo its branch is in +// (this one by default). `fork` makes the list endpoint's head filter find nothing, as GitHub's +// does in a repo that is a fork of another repo in the same org. +function fakeGithub({ prs, load = {}, assigned = {}, fork = false }) { const calls = { assign: [], unassign: [], review: [], graphql: 0 }; const byNumber = new Map(prs.map((p) => [p.number, { assignees: [], requested: [], teams: [], reviews: [], ...p }])); const teams = { embeddedreviewersstaff: STAFF, embeddedreviewers: EVERYONE }; @@ -66,8 +68,13 @@ function fakeGithub({ prs, load = {}, assigned = {} }) { }, list: async ({ head }) => ({ data: [...byNumber.values()] - .filter((p) => `StaflSystems:${p.head}` === head) - .map((p) => ({ number: p.number, draft: Boolean(p.draft), base: { ref: p.base } })), + .filter((p) => !head || (!fork && !p.headRepo && `StaflSystems:${p.head}` === head)) + .map((p) => ({ + number: p.number, + draft: Boolean(p.draft), + base: { ref: p.base }, + head: { ref: p.head, repo: { full_name: p.headRepo ?? 'StaflSystems/StaflLib' } }, + })), }), listReviews: async ({ pull_number }) => ({ data: byNumber.get(pull_number).reviews.map(([login, state]) => ({ user: user(login), state })), @@ -194,6 +201,31 @@ test('skips a draft parent with no assignees and inherits from further down', as assert.deepStrictEqual(fake.pr(32).assignees.sort(), ['devF', 'staffB']); }); +test('mid-stack in a repo that is a fork: still finds the PR below', async () => { + const fake = fakeGithub({ + fork: true, + prs: [ + { number: 25, author: 'devE', head: 'a', base: 'main', assignees: ['staffC', 'devD'] }, + { number: 26, author: 'devE', head: 'b', base: 'a' }, + ], + }); + await run(fake, 26); + assert.deepStrictEqual(fake.pr(26).assignees.sort(), ['devD', 'staffC']); + assert.strictEqual(fake.calls.graphql, 0); +}); + +test('a PR from a fork with the same branch name is not the PR below', async () => { + const fake = fakeGithub({ + prs: [ + { number: 27, author: 'devF', head: 'a', headRepo: 'someone/StaflLib', base: 'main', assignees: ['staffA', 'devD'] }, + { number: 28, author: 'devE', head: 'b', base: 'a' }, + ], + load: { staffA: 3, staffB: 0, staffC: 3, devD: 3, devF: 0 }, + }); + await run(fake, 28); + assert.deepStrictEqual(fake.pr(28).assignees, ['staffB', 'devF']); +}); + test('waits for a ready parent submitted in the same stack to get its assignees', async () => { const fake = fakeGithub({ prs: [