From 80dd709c59c74a0ff09eeeb7bb0a42cc4fe5ea56 Mon Sep 17 00:00:00 2001 From: Carlos Eduardo Arango Gutierrez Date: Fri, 30 Oct 2026 23:52:14 +0100 Subject: [PATCH 1/4] test(workflows): cover conflict-check base filter, null retries and comment recovery #640 merged with three gaps its review threads found: open PRs that target another branch are checked against that branch but told to update from main; a PR whose mergeable is null is never read again in that run; and a comment that fails after the label went on is never retried, because the label alone stops the next run from commenting. These tests pin the fixed behaviour. pulls.list must ask for base main. A null mergeable is read up to three times, waiting 5 s then 10 s through a fake setTimeout, so no test sleeps. The needs-rebase comment carries a hidden marker, and a labeled, conflicting PR with no marker comment since the label went on gets the comment on the next run, once. The fake now keeps state in the fixtures so one test can run the script twice. Signed-off-by: Carlos Eduardo Arango Gutierrez --- .github/tests/conflict-check.test.mjs | 257 ++++++++++++++++++++++---- 1 file changed, 226 insertions(+), 31 deletions(-) diff --git a/.github/tests/conflict-check.test.mjs b/.github/tests/conflict-check.test.mjs index 6688db37..65254544 100644 --- a/.github/tests/conflict-check.test.mjs +++ b/.github/tests/conflict-check.test.mjs @@ -13,10 +13,13 @@ // See the License for the specific language governing permissions and // limitations under the License. + // Tests for the needs-rebase script in .github/workflows/conflict-check.yaml. // Each test runs the workflow's own script against a recorded fake of the // GitHub API. As on GitHub, pulls.list does not return `mergeable`, so the -// script has to read it from pulls.get. +// script has to read it from pulls.get. The fake keeps its state in the PR +// fixtures, so a second run against the same fixtures sees what the first +// one wrote. import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -26,38 +29,110 @@ import { GITHUB_DIR, extractStep, fakeCore, loadWorkflow, recordingGithub, runGi const WORKFLOW = join(GITHUB_DIR, 'workflows', 'conflict-check.yaml'); const REPO = { owner: 'dsx-ai-factory', repo: 'topograph' }; +// The hidden marker the needs-rebase comment carries. +const MARKER = ''; + +// GitHub's clock for what the fake writes: one second per write, later than +// every timestamp a fixture sets by hand. +let seconds = 0; +function stamp() { + seconds += 1; + return new Date(Date.UTC(2026, 9, 30, 12, 0, seconds)).toISOString(); +} + +// One open pull request. `mergeable` is what pulls.get returns; an array is +// what successive reads return, the last value repeating. `comments` and +// `events` are what issues.listComments and issues.listEvents return. +function pr( + number, + { mergeable, draft = false, labels = [], login = `author${number}`, base = 'main', comments = [], events = [] }, +) { + return { + number, + draft, + mergeable, + base: { ref: base }, + labels: labels.map((name) => ({ name })), + user: { login }, + comments, + events, + }; +} -// One open pull request as pulls.get returns it. -function pr(number, { mergeable, draft = false, labels = [], login = `author${number}` }) { - return { number, draft, mergeable, labels: labels.map((name) => ({ name })), user: { login } }; +function page(items, { page: number = 1, per_page: perPage = 30 }) { + return items.slice((number - 1) * perPage, number * perPage); } -// A fake GitHub holding `prs`. pulls.list serves them a page at a time -// without `mergeable`; pulls.get serves each one in full. `routes` replaces -// individual handlers. +// A fake GitHub holding `prs`. pulls.list serves the open PRs whose base is +// `base` a page at a time, without `mergeable`; pulls.get serves each one in +// full. Label and comment writes change the fixtures and record an event. +// `routes` replaces individual handlers. function fakeGithub(prs, routes = {}) { + const reads = new Map(); + const find = (number) => prs.find((p) => p.number === number); return recordingGithub({ - 'rest.pulls.list': ({ page = 1, per_page: perPage = 30 }) => ({ - data: prs.slice((page - 1) * perPage, page * perPage).map(({ mergeable, ...listed }) => listed), + 'rest.pulls.list': (params) => ({ + data: page( + prs.filter((p) => params.base === undefined || p.base.ref === params.base), + params, + ).map(({ number, draft, base, labels, user }) => ({ number, draft, base, labels, user })), }), - 'rest.pulls.get': ({ pull_number: number }) => ({ data: prs.find((p) => p.number === number) }), - 'rest.issues.addLabels': ({ labels }) => ({ data: labels.map((name) => ({ name })) }), - 'rest.issues.removeLabel': () => ({ data: [] }), - 'rest.issues.createComment': () => ({ data: { id: 1 } }), + 'rest.pulls.get': ({ pull_number: number }) => { + const p = find(number); + const read = reads.get(number) ?? 0; + reads.set(number, read + 1); + const values = Array.isArray(p.mergeable) ? p.mergeable : [p.mergeable]; + const { comments, events, ...full } = p; + return { data: { ...full, labels: [...p.labels], mergeable: values[Math.min(read, values.length - 1)] } }; + }, + 'rest.issues.addLabels': ({ issue_number: number, labels }) => { + const p = find(number); + for (const name of labels) { + if (!p.labels.some((label) => label.name === name)) p.labels.push({ name }); + p.events.push({ event: 'labeled', label: { name }, created_at: stamp() }); + } + return { data: p.labels }; + }, + 'rest.issues.removeLabel': ({ issue_number: number, name }) => { + const p = find(number); + p.labels = p.labels.filter((label) => label.name !== name); + p.events.push({ event: 'unlabeled', label: { name }, created_at: stamp() }); + return { data: p.labels }; + }, + 'rest.issues.createComment': ({ issue_number: number, body }) => { + const p = find(number); + p.comments.push({ body, created_at: stamp(), user: { login: 'github-actions[bot]' } }); + return { data: { id: p.comments.length } }; + }, + 'rest.issues.listComments': (params) => ({ data: page(find(params.issue_number).comments, params) }), + 'rest.issues.listEvents': (params) => ({ data: page(find(params.issue_number).events, params) }), ...routes, }); } +// Runs the script with a fake setTimeout that records each delay and fires +// at once, so no test waits for real. async function run(github) { const { script } = extractStep(WORKFLOW, 'conflicts', 'check'); const { core, log } = fakeCore(); - await runGithubScript(script, { github, context: { repo: REPO }, core }); - return log; + const delays = []; + const realSetTimeout = globalThis.setTimeout; + globalThis.setTimeout = (callback, ms, ...args) => { + delays.push(ms); + queueMicrotask(() => callback(...args)); + return 0; + }; + try { + await runGithubScript(script, { github, context: { repo: REPO }, core }); + } finally { + globalThis.setTimeout = realSetTimeout; + } + return { log, delays }; } // The calls that change a pull request: labels and comments. function writes(calls) { - return calls.filter(({ route }) => route.startsWith('rest.issues.')); + return calls.filter(({ route }) => route.startsWith('rest.issues.') && !route.startsWith('rest.issues.list')); } // The pull request numbers the script read with pulls.get, in order. @@ -65,18 +140,33 @@ function fetched(calls) { return calls.filter(({ route }) => route === 'rest.pulls.get').map(({ params }) => params.pull_number); } +function failure(status, message) { + return () => { + throw Object.assign(new Error(message), { status }); + }; +} + test('conflicted PR gets label and one comment', async () => { const { github, calls } = fakeGithub([pr(7, { mergeable: false, login: 'octocat' })]); - await run(github); + const { delays } = await run(github); + // A PR that was not labeled needs no comment lookup, and a known + // mergeable value needs no wait. + assert.deepEqual(calls.map(({ route }) => route), [ + 'rest.pulls.list', + 'rest.pulls.get', + 'rest.issues.addLabels', + 'rest.issues.createComment', + ]); + assert.deepEqual(delays, []); const changed = writes(calls); - assert.deepEqual(changed.map(({ route }) => route), ['rest.issues.addLabels', 'rest.issues.createComment']); assert.deepEqual(changed[0].params, { ...REPO, issue_number: 7, labels: ['needs-rebase'] }); const { body, ...target } = changed[1].params; assert.deepEqual(target, { ...REPO, issue_number: 7 }); assert.ok(body.startsWith('@octocat '), `comment starts ${JSON.stringify(body.slice(0, 20))}`); assert.ok(body.includes('`main`'), 'comment does not name `main`'); + assert.ok(body.endsWith(`\n\n${MARKER}`), `comment ends ${JSON.stringify(body.slice(-50))}`); assert.deepEqual( calls.filter(({ route }) => route === 'rest.pulls.get').map(({ params }) => params), [{ ...REPO, pull_number: 7 }], @@ -93,7 +183,7 @@ test('clean PR with label loses it', async () => { ]); }); -test('unknown mergeable does nothing', async () => { +test('unknown mergeable is read three times, then left alone', async () => { // One unlabeled and one labeled: treating null as a conflict would label // #9, and treating it as clean would strip the label from #10. const { github, calls } = fakeGithub([ @@ -101,18 +191,63 @@ test('unknown mergeable does nothing', async () => { pr(10, { mergeable: null, labels: ['needs-rebase'] }), ]); - const log = await run(github); + const { log, delays } = await run(github); assert.deepEqual(writes(calls), []); - assert.deepEqual(fetched(calls), [9, 10]); + assert.deepEqual(fetched(calls), [9, 10, 9, 10, 9, 10]); + assert.deepEqual(delays, [5000, 10000]); assert.deepEqual(log.info, [ - '#9: GitHub has not computed mergeability yet, skipped', - '#10: GitHub has not computed mergeability yet, skipped', + '#9: mergeability still unknown after 3 reads, left for the next run', + '#10: mergeability still unknown after 3 reads, left for the next run', ]); }); +test('mergeability computed on a later read is acted on', async () => { + const { github, calls } = fakeGithub([ + pr(17, { mergeable: [null, false], login: 'octocat' }), + pr(18, { mergeable: [null, null, true], labels: ['needs-rebase'] }), + ]); + + const { delays } = await run(github); + + assert.deepEqual(fetched(calls), [17, 18, 17, 18, 18]); + assert.deepEqual(delays, [5000, 10000]); + assert.deepEqual( + writes(calls).map(({ route, params }) => [route, params.issue_number]), + [ + ['rest.issues.addLabels', 17], + ['rest.issues.createComment', 17], + ['rest.issues.removeLabel', 18], + ], + ); +}); + +test('no further wait once every mergeability is known', async () => { + const { github, calls } = fakeGithub([pr(19, { mergeable: [null, true], labels: ['needs-rebase'] })]); + + const { delays } = await run(github); + + assert.deepEqual(fetched(calls), [19, 19]); + assert.deepEqual(delays, [5000]); + assert.deepEqual(writes(calls).map(({ route }) => route), ['rest.issues.removeLabel']); +}); + test('already labeled conflicted PR gets no second comment', async () => { - const { github, calls } = fakeGithub([pr(11, { mergeable: false, labels: ['needs-rebase'] })]); + // The marker comment came after needs-rebase was added. The later `bug` + // label is not a new conflict. + const { github, calls } = fakeGithub([ + pr(11, { + mergeable: false, + labels: ['needs-rebase', 'bug'], + events: [ + { event: 'labeled', label: { name: 'needs-rebase' }, created_at: '2026-10-20T00:00:00Z' }, + { event: 'labeled', label: { name: 'bug' }, created_at: '2026-10-22T00:00:00Z' }, + ], + comments: [ + { body: `@author11 please rebase.\n\n${MARKER}`, created_at: '2026-10-20T00:00:01Z', user: { login: 'github-actions[bot]' } }, + ], + }), + ]); await run(github); @@ -120,6 +255,50 @@ test('already labeled conflicted PR gets no second comment', async () => { assert.deepEqual(fetched(calls), [11]); }); +test('a failed comment is posted by the next run, and only once', async () => { + const prs = [pr(15, { mergeable: false, login: 'octocat' })]; + + const first = fakeGithub(prs, { 'rest.issues.createComment': failure(502, 'Server Error') }); + await assert.rejects(run(first.github), { status: 502, message: 'Server Error' }); + assert.deepEqual(writes(first.calls).map(({ route }) => route), ['rest.issues.addLabels', 'rest.issues.createComment']); + + // The PR now has needs-rebase and no comment. + const second = fakeGithub(prs); + await run(second.github); + const posted = writes(second.calls); + assert.deepEqual(posted.map(({ route, params }) => [route, params.issue_number]), [['rest.issues.createComment', 15]]); + assert.ok(posted[0].params.body.startsWith('@octocat '), 'retried comment does not mention the author'); + assert.ok(posted[0].params.body.endsWith(MARKER), 'retried comment has no marker'); + + const third = fakeGithub(prs); + await run(third.github); + assert.deepEqual(writes(third.calls), []); +}); + +test('a comment from an earlier conflict does not count for the current one', async () => { + // #16 conflicted, was told, merged cleanly, and conflicts again; the + // comment for the second conflict failed after the label went on. + const { github, calls } = fakeGithub([ + pr(16, { + mergeable: false, + labels: ['needs-rebase'], + events: [ + { event: 'labeled', label: { name: 'needs-rebase' }, created_at: '2026-10-01T00:00:00Z' }, + { event: 'unlabeled', label: { name: 'needs-rebase' }, created_at: '2026-10-05T00:00:00Z' }, + { event: 'labeled', label: { name: 'needs-rebase' }, created_at: '2026-10-20T00:00:00Z' }, + ], + comments: [ + { body: `@author16 please rebase.\n\n${MARKER}`, created_at: '2026-10-01T00:00:01Z', user: { login: 'github-actions[bot]' } }, + { body: 'Rebased, thanks.', created_at: '2026-10-21T00:00:00Z', user: { login: 'author16' } }, + ], + }), + ]); + + await run(github); + + assert.deepEqual(writes(calls).map(({ route, params }) => [route, params.issue_number]), [['rest.issues.createComment', 16]]); +}); + test('drafts are skipped', async () => { // The draft conflicts, so reading it would label it. const { github, calls } = fakeGithub([ @@ -133,6 +312,26 @@ test('drafts are skipped', async () => { assert.deepEqual(writes(calls), []); }); +test('PRs that target another branch are not checked', async () => { + // #20 conflicts with its own base, not with main; reading it would tell + // its author to update from main. + const { github, calls } = fakeGithub([ + pr(20, { mergeable: false, base: 'release-1.1' }), + pr(21, { mergeable: false }), + ]); + + await run(github); + + assert.deepEqual(fetched(calls), [21]); + assert.deepEqual( + writes(calls).map(({ route, params }) => [route, params.issue_number]), + [ + ['rest.issues.addLabels', 21], + ['rest.issues.createComment', 21], + ], + ); +}); + test('second page of open PRs is processed', async () => { const prs = Array.from({ length: 150 }, (_, i) => pr(i + 1, { mergeable: i + 1 !== 150 })); const { github, calls } = fakeGithub(prs); @@ -142,8 +341,8 @@ test('second page of open PRs is processed', async () => { assert.deepEqual( calls.filter(({ route }) => route === 'rest.pulls.list').map(({ params }) => params), [ - { ...REPO, state: 'open', per_page: 100, page: 1 }, - { ...REPO, state: 'open', per_page: 100, page: 2 }, + { ...REPO, state: 'open', base: 'main', per_page: 100, page: 1 }, + { ...REPO, state: 'open', base: 'main', per_page: 100, page: 2 }, ], ); assert.equal(fetched(calls).length, 150); @@ -160,11 +359,7 @@ test('missing label surfaces as job failure', async () => { // A token holding only pull-requests: write cannot create a label, so // GitHub answers 403 when needs-rebase does not exist. const { github, calls } = fakeGithub([pr(14, { mergeable: false })], { - 'rest.issues.addLabels': () => { - throw Object.assign(new Error('You do not have permission to create labels on this repository.'), { - status: 403, - }); - }, + 'rest.issues.addLabels': failure(403, 'You do not have permission to create labels on this repository.'), }); await assert.rejects(run(github), { From cdea04b61ffe5b104a86069a71bd3ac94947cb62 Mon Sep 17 00:00:00 2001 From: Carlos Eduardo Arango Gutierrez Date: Tue, 3 Nov 2026 12:00:07 +0100 Subject: [PATCH 2/4] fix(workflows): check only main PRs, retry null mergeability, retry failed comments Three findings from #640's review threads, which merged unanswered: - pulls.list now asks for base main. A PR targeting another branch was checked against that branch, then labeled and told to update from main. - A null mergeable is read again after 5 s and 10 s. Before, the run that saw null was the only one until main moved again, so a newly conflicting PR could stay unlabeled. The waits are shared by every PR still unknown, so a run waits at most 15 s in total, well inside timeout-minutes. - The comment carries a hidden marker, and a conflicting PR that already has the label gets the comment unless a marker comment was created at or after needs-rebase last went on. A failed createComment used to leave the label as false proof that the author was told. The label still goes on first, so a missing label fails the job before any comment; comparing against the latest labeled event keeps a comment from an earlier conflict from counting for a new one. Signed-off-by: Carlos Eduardo Arango Gutierrez --- .github/workflows/conflict-check.yaml | 158 ++++++++++++++++++-------- 1 file changed, 109 insertions(+), 49 deletions(-) diff --git a/.github/workflows/conflict-check.yaml b/.github/workflows/conflict-check.yaml index 7c09abb6..8b0a1a0c 100644 --- a/.github/workflows/conflict-check.yaml +++ b/.github/workflows/conflict-check.yaml @@ -13,39 +13,52 @@ # See the License for the specific language governing permissions and # limitations under the License. -# Labels open pull requests that no longer merge cleanly into main, so authors -# and reviewers see a conflict without opening each PR: -# mergeable false add needs-rebase and comment once, mentioning the author +# Labels open, non-draft pull requests that target main and no longer merge +# cleanly into it, so authors and reviewers see a conflict without opening +# each PR: +# mergeable false add needs-rebase and comment once per conflict, +# mentioning the author # mergeable true remove needs-rebase if the PR has it -# mergeable null log it and change nothing -# Draft pull requests are skipped. Open PRs are read page by page, so the check -# covers more than the first 100. +# mergeable null read it again after 5 s and after 10 s more; still null +# after three reads, log it and change nothing +# Only PRs whose base is main are listed: one that targets another branch +# would be checked against that branch but told to update from main. Open PRs +# are read page by page, so the check covers more than the first 100. # # Why push to main: a PR starts conflicting when its base moves, and GitHub # recomputes `mergeable` against the new main after each push to it. The check # does not run when an author pushes a fix to the PR branch, so needs-rebase # comes off on the next push to main, or earlier through workflow_dispatch. # -# Why null is skipped: pulls.get returns `mergeable: null` while GitHub is +# Why null is read again: pulls.get returns `mergeable: null` while GitHub is # still computing it in the background, which is common right after main -# moves. Reading null as a conflict would label PRs that merge cleanly, and -# reading it as clean would strip the label from PRs that still conflict. The -# next run sees the computed value. +# moves, and no other run would read that PR until main moves again. Reading +# null as a conflict would label PRs that merge cleanly, and reading it as +# clean would strip the label from PRs that still conflict. Every PR is read +# once before the first wait, and the waits are shared by all PRs still +# unknown, so a run waits at most 15 s however many PRs are pending. +# +# Why the comment carries a marker: the label alone does not prove the author +# was told. If createComment fails after addLabels succeeded, the PR keeps the +# label with no comment. So for a conflicting PR that already has the label, +# the run lists its issue events and comments, and posts the comment unless +# one carrying the hidden marker was created at or after needs-rebase last went +# on; a comment from an earlier conflict does not count. The label still goes +# on first, so a missing label fails the job before any comment is posted. # # Why no cancel: runs share one concurrency group and a running one is never # cancelled, because a cancelled run would skip part of the check for the -# merge that started it. Stopped between adding the label and posting the -# comment, it leaves a labeled PR whose author is never asked to rebase: the -# next run sees the label and does not comment again. GitHub still replaces a -# pending run with a newer one, which loses nothing, since every run reads all -# open PRs as they stand when it starts. +# merge that started it. GitHub still replaces a pending run with a newer one, +# which loses nothing, since every run reads all open PRs as they stand when +# it starts. # -# Permissions: listing and reading PRs needs pull-requests read. Labels and -# comments go through the issues endpoints, which accept pull-requests write -# on a pull request (GitHub's "Permissions required for GitHub Apps", Pull -# requests section), so the job does not hold issues: write. That token cannot -# create a label, so needs-rebase must exist (it is declared in -# .github/labels.yml); if it does not, addLabels fails and so does the job. +# Permissions: listing and reading PRs needs pull-requests read. Labels, +# comments and issue events go through the issues endpoints, which accept +# pull-requests read or write on a pull request (GitHub's "Permissions +# required for GitHub Apps", Pull requests section), so the job does not hold +# issues: write. That token cannot create a label, so needs-rebase must exist +# (it is declared in .github/labels.yml); if it does not, addLabels fails and +# so does the job. name: Merge Conflict Check @@ -71,7 +84,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 10 permissions: - pull-requests: write # read open PRs, add or remove needs-rebase, comment on them + pull-requests: write # read open PRs and their events and comments, add or remove needs-rebase, comment on them steps: - name: Check open pull requests for conflicts id: check @@ -79,35 +92,47 @@ jobs: with: script: | const { owner, repo } = context.repo; - const pulls = await github.paginate(github.rest.pulls.list, { - owner, - repo, - state: 'open', - per_page: 100, - }); + // Hidden when GitHub renders the comment; a later run finds the + // comment by it. + const MARKER = ''; + // The wait before each read of a PR whose mergeable is still null. + const WAITS_MS = [0, 5000, 10000]; - for (const listed of pulls) { - const number = listed.number; - if (listed.draft) { - core.info(`#${number}: draft, skipped`); - continue; - } + // Whether a marker comment was created at or after the label last + // went on, so it was posted for the current conflict. + async function notified(number) { + const events = await github.paginate(github.rest.issues.listEvents, { + owner, + repo, + issue_number: number, + per_page: 100, + }); + const labeledAt = events + .filter((event) => event.event === 'labeled' && event.label?.name === 'needs-rebase') + .reduce((latest, event) => Math.max(latest, Date.parse(event.created_at)), -Infinity); + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: number, + per_page: 100, + }); + return comments.some( + (comment) => (comment.body ?? '').includes(MARKER) && Date.parse(comment.created_at) >= labeledAt, + ); + } - // pulls.list does not return mergeable; pulls.get does, and - // starts GitHub's background computation when it is unknown. - const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); + async function reconcile(pr) { + const number = pr.number; const labeled = pr.labels.some((label) => label.name === 'needs-rebase'); - - if (pr.mergeable === null) { - core.info(`#${number}: GitHub has not computed mergeability yet, skipped`); - } else if (pr.mergeable === false) { - if (labeled) { - core.info(`#${number}: conflicts with main, already labeled`); - continue; + if (pr.mergeable === false) { + if (!labeled) { + // Not caught: a missing label must fail the job, and the + // comment must not go out without the label. + await github.rest.issues.addLabels({ owner, repo, issue_number: number, labels: ['needs-rebase'] }); + } else if (await notified(number)) { + core.info(`#${number}: conflicts with main, already labeled and told`); + return; } - // Not caught: a missing label must fail the job, and the - // comment must not go out without the label. - await github.rest.issues.addLabels({ owner, repo, issue_number: number, labels: ['needs-rebase'] }); await github.rest.issues.createComment({ owner, repo, @@ -117,12 +142,47 @@ jobs: 'Please rebase it onto the latest `main`, or merge `main` into it, resolve the conflicts, and push.', 'The `needs-rebase` label comes off the next time this check finds that it merges cleanly;', 'the check runs after every push to `main`.', - ].join(' '), + ].join(' ') + `\n\n${MARKER}`, }); - core.info(`#${number}: conflicts with main, labeled needs-rebase`); + core.info(`#${number}: conflicts with main, labeled needs-rebase and commented`); } else if (labeled) { // mergeable is true here. await github.rest.issues.removeLabel({ owner, repo, issue_number: number, name: 'needs-rebase' }); core.info(`#${number}: merges cleanly, removed needs-rebase`); } } + + const pulls = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: 'open', + base: 'main', + per_page: 100, + }); + + let pending = []; + for (const listed of pulls) { + if (listed.draft) { + core.info(`#${listed.number}: draft, skipped`); + continue; + } + pending.push(listed.number); + } + + for (const wait of WAITS_MS) { + if (pending.length === 0) break; + if (wait > 0) await new Promise((resolve) => setTimeout(resolve, wait)); + const unknown = []; + for (const number of pending) { + // pulls.list does not return mergeable; pulls.get does, and + // starts GitHub's background computation when it is unknown. + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); + if (pr.mergeable === null) unknown.push(number); + else await reconcile(pr); + } + pending = unknown; + } + + for (const number of pending) { + core.info(`#${number}: mergeability still unknown after ${WAITS_MS.length} reads, left for the next run`); + } From da0452d032fac8f8aea62e2af8b184194ee4bc02 Mon Sep 17 00:00:00 2001 From: Carlos Eduardo Arango Gutierrez Date: Tue, 3 Nov 2026 12:00:46 +0100 Subject: [PATCH 3/4] docs(workflows): record the conflict-check fixes The #640 CHANGELOG bullet sits under [Unreleased], so no release shipped the behaviour it describes. It said the check covers all open pull requests, but it skips drafts and now also PRs that target another branch. Amend that bullet to say so and to name the retried null mergeability and the comment recovery, rather than adding a Fixed bullet for a bug no release had. CONTRIBUTING.md tells authors that PRs targeting another branch are not checked. Signed-off-by: Carlos Eduardo Arango Gutierrez --- CHANGELOG.md | 2 +- CONTRIBUTING.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccf42eac..190c3d94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - License headers: `make license-headers` adds NVIDIA's standard SPDX and Apache 2.0 header to files that have none, and `make license-header-check`, run by `license-scan.yml`, fails on any file without a copyright or license header, or with an NVIDIA header in one of the two older forms. Every NVIDIA header now uses the standard form, keeping each file's original year; files that had no header now carry one. - Nautobot provider (`nautobot`) discovers switch fabric topology through the GraphQL API, with required `endpoint`, compute `deviceRole`, and `switchRoles` parameters, optional `location` and `platform` filters, status filters, and required `creds.token` authentication. Switch queries paginate using the configured `pageSize` (default `100`). Discovery starts at selected switches, collects compute endpoints from cable peers, follows connected switches outside the initial location and platform filters to infer fabric tiers, and groups redundant switches into a scheduler tree. Attached compute endpoints can also be in other locations or use other platforms. Duplicate device IDs and switch names fail with HTTP 502 identifying the conflicts; switch-name errors include both device UUIDs. Devices missing an ID or name are skipped with a warning. Oversized pages log a warning and advance the offset by the actual number of returned devices, including skipped records. Transport failures return HTTP 502 with the underlying error. - `workflow-tests.yaml` runs unit tests for the scripts inside GitHub workflows. -- `conflict-check.yaml` adds the `needs-rebase` label to open pull requests that no longer merge cleanly into `main`, comments once to ask the author to resolve the conflicts, and removes the label once the pull request merges cleanly again. It runs after every push to `main`. +- `conflict-check.yaml` adds the `needs-rebase` label to open, non-draft pull requests that target `main` and no longer merge cleanly into it, comments once to ask the author to resolve the conflicts, and removes the label once the pull request merges cleanly again. It runs after every push to `main`. A pull request whose mergeability GitHub has not computed yet is read again after 5 and 10 seconds before being left for the next run, and a pull request that got `needs-rebase` but not the comment, because posting it failed, gets the comment on the next run. ### Changed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 51b26522..0fc0123d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -351,7 +351,8 @@ workflow (`conflict-check.yaml`) adds the `needs-rebase` label and comments once to ask you to resolve the conflicts. Rebase onto the latest `main`, or merge `main` into your branch, and push. The check runs after every push to `main`, so the label comes off on the first run that finds the PR merges -cleanly, not when you push the fix. Draft PRs are not checked. +cleanly, not when you push the fix. Draft PRs and PRs that target a branch +other than `main` are not checked. Keep the PR in draft while you're still reshaping it — draft PRs don't page reviewers, so that's the cheapest phase to rework history or force-push in. From cea696f0cbdf28cd5c1c00be0f6c2ae90a63a79d Mon Sep 17 00:00:00 2001 From: Carlos Eduardo Arango Gutierrez Date: Tue, 3 Nov 2026 13:41:07 +0100 Subject: [PATCH 4/4] test(workflows): pin the conflict-check cutoff, the no-event case and comment paging Three parts of the comment check had no test that fails when they break. The marker comment counts when it was created in the same second as the needs-rebase label, which is the usual case given GitHub's 1-second timestamps; turning >= into > would post a second comment on most conflicts. With no needs-rebase labeled event, any marker comment counts. And comments are read past the first page, so a PR with more than 100 comments still finds its marker. Each case fails under the matching mutant of conflict-check.yaml and passes on it as it is. Signed-off-by: Carlos Eduardo Arango Gutierrez --- .github/tests/conflict-check.test.mjs | 66 +++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/.github/tests/conflict-check.test.mjs b/.github/tests/conflict-check.test.mjs index 65254544..99ccb1fd 100644 --- a/.github/tests/conflict-check.test.mjs +++ b/.github/tests/conflict-check.test.mjs @@ -299,6 +299,72 @@ test('a comment from an earlier conflict does not count for the current one', as assert.deepEqual(writes(calls).map(({ route, params }) => [route, params.issue_number]), [['rest.issues.createComment', 16]]); }); +test('a comment made in the same second as the label counts', async () => { + // GitHub timestamps have 1-second resolution, and the label and the + // comment usually go on in the same second. + const { github, calls } = fakeGithub([ + pr(22, { + mergeable: false, + labels: ['needs-rebase'], + events: [{ event: 'labeled', label: { name: 'needs-rebase' }, created_at: '2026-10-20T00:00:00Z' }], + comments: [ + { body: `@author22 please rebase.\n\n${MARKER}`, created_at: '2026-10-20T00:00:00Z', user: { login: 'github-actions[bot]' } }, + ], + }), + ]); + + await run(github); + + assert.deepEqual(writes(calls), []); +}); + +test('with no labeled event, any marker comment counts', async () => { + // #23 has needs-rebase, but listEvents shows no needs-rebase labeled + // event, so there is no cutoff; the check stays quiet rather than + // risk a second comment. + const { github, calls } = fakeGithub([ + pr(23, { + mergeable: false, + labels: ['needs-rebase'], + events: [{ event: 'labeled', label: { name: 'bug' }, created_at: '2026-10-22T00:00:00Z' }], + comments: [ + { body: `@author23 please rebase.\n\n${MARKER}`, created_at: '2026-10-01T00:00:00Z', user: { login: 'github-actions[bot]' } }, + ], + }), + ]); + + await run(github); + + assert.deepEqual(writes(calls), []); +}); + +test('a marker comment on the second page of comments counts', async () => { + // 100 comments fill the first page at per_page: 100; the marker comment + // is the 101st. + const comments = Array.from({ length: 100 }, (_, i) => ({ + body: `Comment ${i + 1}.`, + created_at: new Date(Date.UTC(2026, 9, 21, 0, 0, i)).toISOString(), + user: { login: 'author24' }, + })); + comments.push({ body: `@author24 please rebase.\n\n${MARKER}`, created_at: '2026-10-22T00:00:00Z', user: { login: 'github-actions[bot]' } }); + const { github, calls } = fakeGithub([ + pr(24, { + mergeable: false, + labels: ['needs-rebase'], + events: [{ event: 'labeled', label: { name: 'needs-rebase' }, created_at: '2026-10-20T00:00:00Z' }], + comments, + }), + ]); + + await run(github); + + assert.deepEqual(writes(calls), []); + assert.deepEqual( + calls.filter(({ route }) => route === 'rest.issues.listComments').map(({ params }) => params.page), + [1, 2], + ); +}); + test('drafts are skipped', async () => { // The draft conflicts, so reading it would label it. const { github, calls } = fakeGithub([