From 22d47748d26adf5419b532642c42bde59d3a0b62 Mon Sep 17 00:00:00 2001 From: Rafael Gonzaga Date: Fri, 24 Jul 2026 10:38:48 -0300 Subject: [PATCH 1/3] fix(git-node): fix security release preparation Signed-off-by: RafaelGSS --- lib/cherry_pick.js | 31 +++++++---- lib/landing_session.js | 5 +- lib/prepare_release.js | 35 ++++++++++-- lib/security-release/security-release.js | 8 +++ lib/security_blog.js | 29 +++++----- test/unit/security_release.test.js | 69 ++++++++++++++++++++++++ 6 files changed, 146 insertions(+), 31 deletions(-) diff --git a/lib/cherry_pick.js b/lib/cherry_pick.js index e2eff31e..9e14ebe3 100644 --- a/lib/cherry_pick.js +++ b/lib/cherry_pick.js @@ -16,20 +16,31 @@ export default class CherryPick { lint, includeCVE, cveIds, - vulnCveMap + vulnCveMap, + promptAmend, + skipMessagePrompt } = {}) { this.prid = prid; this.cli = cli; this.dir = dir; this.upstream = upstream; this.gpgSign = gpgSign; - this.options = { owner, repo, lint, includeCVE, cveIds, vulnCveMap }; + this.skipMessagePrompt = skipMessagePrompt ?? false; + this.options = { + owner, repo, lint, includeCVE, cveIds, vulnCveMap, promptAmend + }; } get includeCVE() { return this.options.includeCVE ?? false; } + // Whether to ask before amending the PR to the proposal. Callers that already + // asked opt out of it. + get promptAmend() { + return this.options.promptAmend ?? true; + } + get cveIds() { return this.options.cveIds ?? null; } @@ -83,13 +94,15 @@ export default class CherryPick { this.expectedCommitShas = metadata.data.commits.map(({ commit }) => commit.oid); - const amend = await cli.prompt( - 'Would you like to amend this PR to the proposal?', - { default: true } - ); + if (this.promptAmend) { + const amend = await cli.prompt( + 'Would you like to amend this PR to the proposal?', + { default: true } + ); - if (!amend) { - return true; + if (!amend) { + return true; + } } try { @@ -117,7 +130,7 @@ export default class CherryPick { { captureStdout: 'lines' }); if (commitInfo.shas.length !== 1) { - const fixupAll = await cli.prompt( + const fixupAll = this.skipMessagePrompt || await cli.prompt( `${subjects.length} commits from the original PR are going to be` + 'squashed into a single commit. OK to proceed?', { defaultAnswer: true diff --git a/lib/landing_session.js b/lib/landing_session.js index 1a0c93aa..bee509a6 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -430,7 +430,10 @@ export default class LandingSession extends Session { const messageFile = this.saveMessage(rev, message); cli.separator('New Message'); cli.log(message.trim()); - const takeMessage = await cli.prompt('Use this message?'); + // `skipMessagePrompt` is only set by callers that already got a blanket + // confirmation; it is undefined on a regular landing session. + const takeMessage = this.skipMessagePrompt || + await cli.prompt('Use this message?'); if (takeMessage) { await runAsync('git', ['commit', '--amend', '-F', messageFile, ...this.gpgSign]); return true; diff --git a/lib/prepare_release.js b/lib/prepare_release.js index 7089924a..8249a72d 100644 --- a/lib/prepare_release.js +++ b/lib/prepare_release.js @@ -13,7 +13,10 @@ import { } from './release/utils.js'; import CherryPick from './cherry_pick.js'; import Session from './session.js'; -import { getAffectedVersionLines } from './security-release/security-release.js'; +import { + getAffectedVersionLines, + getDependencyUpdates +} from './security-release/security-release.js'; const isWindows = process.platform === 'win32'; @@ -130,11 +133,11 @@ export default class ReleasePreparation extends Session { } } - for (const [name, dep] of Object.entries(vulnData.dependencies ?? {})) { + for (const dep of getDependencyUpdates(vulnData.dependencies)) { const url = getPullRequestURLForLine( dep.affectedVersions, line, dep.prURL); if (url) { - targets.push({ url, cveIds: null, label: `dependency: ${name}` }); + targets.push({ url, cveIds: null, label: `dependency: ${dep.name}` }); } } @@ -169,6 +172,8 @@ export default class ReleasePreparation extends Session { cli.info(`Cherry-picking ${targets.length} PR(s) for ${line} from ` + 'vulnerabilities.json'); + let amendAll = false; + for (const target of targets) { const pr = parsePullRequestURL(target.url); if (!pr) { @@ -176,6 +181,22 @@ export default class ReleasePreparation extends Session { continue; } + if (!amendAll) { + const answer = await cli.promptSelect( + `Would you like to amend ${target.label} (${target.url}) to the proposal?`, + [ + { name: 'Yes', value: 'yes' }, + { name: 'No, skip this PR', value: 'no' }, + { + name: 'Yes to all remaining PRs (no further prompts)', + value: 'all' + } + ]); + + if (answer === 'no') continue; + amendAll = answer === 'all'; + } + if (!target.cveIds) { cli.warn(`No CVE-IDs found in vulnerabilities.json for ${target.url}`); } @@ -188,7 +209,9 @@ export default class ReleasePreparation extends Session { upstream: this.upstreamForPR(pr), lint: false, includeCVE: true, - cveIds: target.cveIds + cveIds: target.cveIds, + promptAmend: false, + skipMessagePrompt: amendAll }); const success = await cp.start(); if (!success) { @@ -862,6 +885,10 @@ export default class ReleasePreparation extends Session { return; } this.stagingBranch = currentBranch; + // On security releases `branch` resolves to `releaseBranch`, which + // tryResetBranch() needs. Derive it from the branch that is checked out: + // the new version is only calculated below, and it may bump the major. + this.releaseBranch = `v${match[1]}.x`; await this.tryResetBranch(); this.versionComponents = await this.calculateNewVersion(await this.getLastRelease(match[1])); const { major, minor, patch } = this.versionComponents; diff --git a/lib/security-release/security-release.js b/lib/security-release/security-release.js index 66d93ef2..e7425e63 100644 --- a/lib/security-release/security-release.js +++ b/lib/security-release/security-release.js @@ -245,6 +245,14 @@ export function getAffectedVersionLines(affectedVersions) { return []; } +// `dependencies` maps a dependency name to its update, or to a list of updates +// when the release lines got different ones. Flatten it into a list of updates +// that each know their name. +export function getDependencyUpdates(dependencies) { + return Object.entries(dependencies ?? {}).flatMap(([name, update]) => + (Array.isArray(update) ? update : [update]).map((entry) => ({ ...entry, name }))); +} + export function promptDependencies(cli) { return cli.prompt('Enter the link to the dependency update PR (leave empty to exit): ', { defaultAnswer: '', diff --git a/lib/security_blog.js b/lib/security_blog.js index ac700e27..ff173746 100644 --- a/lib/security_blog.js +++ b/lib/security_blog.js @@ -8,6 +8,7 @@ import { SecurityRelease, commitAndPushVulnerabilitiesJSON, getAffectedVersionLines, + getDependencyUpdates, getHighestSeverityAnnouncement, writeSecurityFile, } from './security-release/security-release.js'; @@ -293,24 +294,16 @@ export default class SecurityBlog extends SecurityRelease { } getDependencyUpdatesTemplate(dependencyUpdates) { - if (typeof dependencyUpdates !== 'object') return ''; - if (Object.keys(dependencyUpdates).length === 0) return ''; + const updates = getDependencyUpdates(dependencyUpdates); + if (updates.length === 0) return ''; let template = '\nThis security release includes the following dependency' + ' updates to address public vulnerabilities:\n'; - if (Array.isArray(dependencyUpdates)) { - for (const dependency of dependencyUpdates) { - const title = this.formatDependencyUpdateTitle(dependency); - const releaseLines = getAffectedVersionLines(dependency.affectedVersions); - template += `- ${title} on ${releaseLines.join(', ')}\n`; - } - return template; - } - - for (const [dependency, { versions, affectedVersions }] of Object.entries(dependencyUpdates)) { - const releaseLines = getAffectedVersionLines(affectedVersions); - const formattedVersions = this.formatDependencyVersions(versions); + for (const update of updates) { + const title = this.formatDependencyUpdateTitle(update) || update.name; + const releaseLines = getAffectedVersionLines(update.affectedVersions); + const formattedVersions = this.formatDependencyVersions(update.versions); const versionText = formattedVersions ? ` (${formattedVersions})` : ''; - template += `- ${dependency}${versionText} on ${releaseLines.join(', ')}\n`; + template += `- ${title}${versionText} on ${releaseLines.join(', ')}\n`; } return template; } @@ -328,9 +321,11 @@ export default class SecurityBlog extends SecurityRelease { formatDependencyVersions(versions) { if (!Array.isArray(versions)) return ''; - return versions + // The current schema carries one entry per release line, so the same + // version repeats when every line gets the same update. + return [...new Set(versions .map((version) => this.formatDependencyUpdateVersion(version)) - .filter(Boolean) + .filter(Boolean))] .join(', '); } diff --git a/test/unit/security_release.test.js b/test/unit/security_release.test.js index 3ecfceb2..cd34fede 100644 --- a/test/unit/security_release.test.js +++ b/test/unit/security_release.test.js @@ -12,6 +12,7 @@ import PrepareSecurityRelease, { import UpdateSecurityRelease from '../../lib/update_security_release.js'; import { getAffectedVersionLines, + getDependencyUpdates, getHighestSeverityAnnouncement } from '../../lib/security-release/security-release.js'; @@ -909,8 +910,76 @@ describe('security_blog: getDependencyUpdatesTemplate', () => { assert.match(template, /- undici \(8\.5\.0\) on 22\.x, 24\.x/); }); + it('does not repeat a version listed once per release line', () => { + const blog = new SecurityBlog(); + const template = blog.getDependencyUpdatesTemplate({ + llhttp: { + versions: [ + { version: '9.4.3', affectedVersions: ['main'] }, + { version: '9.4.3', affectedVersions: ['26.x'] }, + { version: '9.4.3', affectedVersions: ['24.x'] } + ], + affectedVersions: { + main: 'https://github.com/nodejs-private/node-private/pull/935', + '26.x': 'https://github.com/nodejs-private/node-private/pull/935', + '24.x': 'https://github.com/nodejs-private/node-private/pull/935' + } + } + }); + + assert.match(template, /- llhttp \(9\.4\.3\) on 26\.x, 24\.x/); + }); + + it('renders a dependency holding a list of updates', () => { + const blog = new SecurityBlog(); + const template = blog.getDependencyUpdatesTemplate({ + nghttp2: [ + { + title: 'deps: update nghttp2 to 1.68.0', + affectedVersions: ['24.x'] + }, + { + name: 'nghttp2', + versions: ['1.67.1'], + affectedVersions: ['22.x'] + } + ] + }); + + assert.match(template, /- update nghttp2 to 1\.68\.0 on 24\.x/); + assert.match(template, /- nghttp2 \(1\.67\.1\) on 22\.x/); + }); + it('returns an empty string when there are no dependency updates', () => { const blog = new SecurityBlog(); assert.strictEqual(blog.getDependencyUpdatesTemplate({}), ''); + assert.strictEqual(blog.getDependencyUpdatesTemplate(undefined), ''); + }); +}); + +describe('security_release: getDependencyUpdates', () => { + it('names the updates of a map keyed by dependency', () => { + assert.deepStrictEqual( + getDependencyUpdates({ undici: { versions: ['8.9.0'] } }), + [{ versions: ['8.9.0'], name: 'undici' }]); + }); + + it('flattens a dependency holding a list of updates', () => { + assert.deepStrictEqual( + getDependencyUpdates({ + nghttp2: [ + { affectedVersions: ['24.x'] }, + { name: 'nghttp2', affectedVersions: ['22.x'] } + ] + }), + [ + { affectedVersions: ['24.x'], name: 'nghttp2' }, + { affectedVersions: ['22.x'], name: 'nghttp2' } + ]); + }); + + it('returns an empty list when there are no dependency updates', () => { + assert.deepStrictEqual(getDependencyUpdates(undefined), []); + assert.deepStrictEqual(getDependencyUpdates({}), []); }); }); From 7c6be36214a8da6e02d17fef6b5043e2864b562a Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Sun, 26 Jul 2026 10:51:15 -0300 Subject: [PATCH 2/3] fix: adjust changelog on security releases --- lib/landing_session.js | 6 +- lib/prepare_release.js | 104 ++++++++++++++++++++--- lib/security-release/security-release.js | 2 +- test/unit/prepare_release.test.js | 70 ++++++++++++++- 4 files changed, 165 insertions(+), 17 deletions(-) diff --git a/lib/landing_session.js b/lib/landing_session.js index bee509a6..b79bf59d 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -398,14 +398,14 @@ export default class LandingSession extends Session { if (!cveID) { cveID = await cli.prompt( 'Git found no CVE-ID trailer in the original commit message. ' + - 'Please, provide the CVE-ID or leave it empty', - { questionType: 'input', defaultAnswer: 'CVE-2026-XXXXX' } + 'Please, provide the CVE-ID (e.g. CVE-2026-12345) or leave it empty', + { questionType: 'input', defaultAnswer: '' } ); } } // Some commits might not address a vulnerability, but it is necessary // for the security release to happen. - if (cveID !== '') { + if (cveID) { amended.push('CVE-ID: ' + cveID); } } diff --git a/lib/prepare_release.js b/lib/prepare_release.js index 8249a72d..7e43ee0d 100644 --- a/lib/prepare_release.js +++ b/lib/prepare_release.js @@ -15,7 +15,8 @@ import CherryPick from './cherry_pick.js'; import Session from './session.js'; import { getAffectedVersionLines, - getDependencyUpdates + getDependencyUpdates, + SEVERITY_RANKS } from './security-release/security-release.js'; const isWindows = process.platform === 'win32'; @@ -51,6 +52,34 @@ export function getPullRequestURLForLine(affectedVersions, line, legacyPrURL) { return null; } +// Format the notable changes of a security release, one entry per commit: +// `* (CVE-ID) subsystem: title (Author) – Severity`, sorted from highest to +// lowest severity, then by CVE-ID. Commits without a CVE-ID trailer +// (e.g. dependency updates) are listed last. +export function formatSecurityNotableChanges(commits, severityByCVE) { + const rank = rating => SEVERITY_RANKS.indexOf((rating || '').toUpperCase()); + const entries = commits + .filter(({ subject }) => subject) + .map(({ subject, author, cveIds = [] }) => ({ + subject, + author, + cveIds, + rating: cveIds.map(id => severityByCVE.get(id)).find(Boolean) || '' + })) + .sort((a, b) => (rank(b.rating) - rank(a.rating)) || + (b.cveIds.length - a.cveIds.length) || + (a.cveIds[0] || '').localeCompare(b.cveIds[0] || '', 'en', { numeric: true })); + + const lines = entries.map(({ subject, author, cveIds, rating }) => { + const cve = cveIds.length ? `(${cveIds.join(', ')}) ` : ''; + const severity = rating + ? ` – ${rating[0].toUpperCase()}${rating.slice(1).toLowerCase()}` + : ''; + return `* ${cve}${subject} (${author})${severity}`; + }); + return lines.length ? `${lines.join('\n')}\n` : ''; +} + export default class ReleasePreparation extends Session { constructor(argv, cli, dir) { super(cli, dir); @@ -137,7 +166,12 @@ export default class ReleasePreparation extends Session { const url = getPullRequestURLForLine( dep.affectedVersions, line, dep.prURL); if (url) { - targets.push({ url, cveIds: null, label: `dependency: ${dep.name}` }); + targets.push({ + url, + cveIds: null, + label: `dependency: ${dep.name}`, + isDependency: true + }); } } @@ -197,7 +231,7 @@ export default class ReleasePreparation extends Session { amendAll = answer === 'all'; } - if (!target.cveIds) { + if (!target.cveIds && !target.isDependency) { cli.warn(`No CVE-IDs found in vulnerabilities.json for ${target.url}`); } @@ -208,7 +242,7 @@ export default class ReleasePreparation extends Session { gpgSign: this.gpgSign, upstream: this.upstreamForPR(pr), lint: false, - includeCVE: true, + includeCVE: !target.isDependency, cveIds: target.cveIds, promptAmend: false, skipMessagePrompt: amendAll @@ -553,10 +587,12 @@ export default class ReleasePreparation extends Session { const data = await fs.readFile(majorChangelogPath, 'utf8'); const arr = data.split('\n'); const allCommits = this.getChangelog(); - const notableChanges = await this.getBranchDiff({ - onlyNotableChanges: true, - format: isSecurityRelease ? 'messageonly' : 'markdown', - }); + const notableChanges = isSecurityRelease + ? await this.getSecurityNotableChanges() + : await this.getBranchDiff({ + onlyNotableChanges: true, + format: 'markdown', + }); let releaseHeader = `## ${date}, Version ${newVersion}` + ` ${releaseInfo}, @${username}\n`; if (isSecurityRelease) { @@ -692,10 +728,12 @@ export default class ReleasePreparation extends Session { messageBody.push('This is a security release.\n\n'); } - const notableChanges = await this.getBranchDiff({ - onlyNotableChanges: true, - format: isSecurityRelease ? 'messageonly' : 'plaintext' - }); + const notableChanges = isSecurityRelease + ? await this.getSecurityNotableChanges() + : await this.getBranchDiff({ + onlyNotableChanges: true, + format: 'plaintext' + }); messageBody.push('Notable changes:\n\n'); if (isLTSTransition) { messageBody.push(`${getStartLTSBlurb(this)}\n\n`); @@ -718,6 +756,48 @@ export default class ReleasePreparation extends Session { return useMessage; } + // Build the notable changes of a security release from the commits + // cherry-picked onto the proposal branch and the severity ratings in + // vulnerabilities.json. + async getSecurityNotableChanges() { + const { upstream, versionComponents } = this; + const releaseBranch = `v${versionComponents.major}.x`; + + await forceRunAsync('git', ['remote', 'set-branches', '--add', upstream, releaseBranch], { + ignoreFailures: false + }); + await forceRunAsync('git', ['fetch', upstream, releaseBranch], { ignoreFailures: false }); + + const severityByCVE = new Map(); + const vulnPath = this.getVulnerabilitiesJSONPath(); + if (vulnPath && existsSync(vulnPath)) { + const { reports } = JSON.parse(readFileSync(vulnPath, 'utf-8')); + for (const report of reports ?? []) { + for (const cveId of report.cveIds ?? []) { + severityByCVE.set(cveId, report.severity?.rating); + } + } + } + + const log = runSync('git', [ + 'log', + '--format=%s%x1f%an%x1f%(trailers:key=CVE-ID,valueonly,separator=%x2C)%x1e', + `${upstream}/${releaseBranch}..HEAD` + ]); + const commits = log.split('\x1e').map(record => { + const [subject, author, cveIds] = record.trim().split('\x1f'); + return { + subject, + author, + // Ignore placeholder trailers such as CVE-2026-XXXXX. + cveIds: (cveIds || '').split(',') + .map(id => id.trim()) + .filter(id => /^CVE-\d{4}-\d+$/.test(id)) + }; + }); + return formatSecurityNotableChanges(commits, severityByCVE); + } + async getBranchDiff(opts) { const { cli, diff --git a/lib/security-release/security-release.js b/lib/security-release/security-release.js index e7425e63..f20d75f4 100644 --- a/lib/security-release/security-release.js +++ b/lib/security-release/security-release.js @@ -11,7 +11,7 @@ export const NEXT_SECURITY_RELEASE_REPOSITORY = { repo: 'security-release' }; -const SEVERITY_RANKS = ['LOW', 'MEDIUM', 'HIGH', 'CRITICAL']; +export const SEVERITY_RANKS = ['LOW', 'MEDIUM', 'HIGH', 'CRITICAL']; const RELEASE_LINE_RE = /^v?(\d+)(?:\.x)?$/; const SEMVER_RE = /^v?(\d+)\.\d+\.\d+/; diff --git a/test/unit/prepare_release.test.js b/test/unit/prepare_release.test.js index 594a60b7..5cf18e5e 100644 --- a/test/unit/prepare_release.test.js +++ b/test/unit/prepare_release.test.js @@ -5,7 +5,8 @@ import { readFileSync } from 'node:fs'; import * as utils from '../../lib/release/utils.js'; import { parsePullRequestURL, - getPullRequestURLForLine + getPullRequestURLForLine, + formatSecurityNotableChanges } from '../../lib/prepare_release.js'; describe('prepare_release: utils.getEOLDate', () => { @@ -136,3 +137,70 @@ describe('prepare_release: getPullRequestURLForLine', () => { assert.strictEqual(getPullRequestURLForLine(null, '22.x'), null); }); }); + +describe('prepare_release: formatSecurityNotableChanges', () => { + it('sorts by severity then CVE-ID and formats each entry', () => { + const severityByCVE = new Map([ + ['CVE-2026-48933', 'high'], + ['CVE-2026-48618', 'high'], + ['CVE-2026-48615', 'medium'], + ['CVE-2026-48617', 'low'] + ]); + const commits = [ + { + subject: 'permission: handle process.chdir on writereport', + author: 'RafaelGSS', + cveIds: ['CVE-2026-48617'] + }, + { + subject: 'lib,test: redact proxy credentials in tunnel errors', + author: 'Matteo Collina', + cveIds: ['CVE-2026-48615'] + }, + { + subject: 'crypto: guard WebCrypto cipher output length', + author: 'Filip Skokan', + cveIds: ['CVE-2026-48933'] + }, + { + subject: 'tls: normalize hostname for server identity checks', + author: 'Matteo Collina', + cveIds: ['CVE-2026-48618'] + } + ]; + + assert.strictEqual( + formatSecurityNotableChanges(commits, severityByCVE), + '* (CVE-2026-48618) tls: normalize hostname for server identity ' + + 'checks (Matteo Collina) – High\n' + + '* (CVE-2026-48933) crypto: guard WebCrypto cipher output length ' + + '(Filip Skokan) – High\n' + + '* (CVE-2026-48615) lib,test: redact proxy credentials in tunnel ' + + 'errors (Matteo Collina) – Medium\n' + + '* (CVE-2026-48617) permission: handle process.chdir on writereport ' + + '(RafaelGSS) – Low\n' + ); + }); + + it('lists commits without a CVE or severity last, without annotations', () => { + const severityByCVE = new Map([['CVE-2026-48618', 'HIGH']]); + const commits = [ + { subject: 'deps: update undici to 6.21.2', author: 'Node.js GitHub Bot', cveIds: [] }, + { subject: 'tls: some fix', author: 'A Contributor', cveIds: ['CVE-2026-1'] }, + { + subject: 'tls: normalize hostname for server identity checks', + author: 'Matteo Collina', + cveIds: ['CVE-2026-48618'] + } + ]; + + assert.strictEqual( + formatSecurityNotableChanges(commits, severityByCVE), + '* (CVE-2026-48618) tls: normalize hostname for server identity ' + + 'checks (Matteo Collina) – High\n' + + '* (CVE-2026-1) tls: some fix (A Contributor)\n' + + '* deps: update undici to 6.21.2 (Node.js GitHub Bot)\n' + ); + assert.strictEqual(formatSecurityNotableChanges([], severityByCVE), ''); + }); +}); From 9351b623823b194be8512b5cc81e2ba9b21428c9 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Sun, 26 Jul 2026 11:58:09 -0300 Subject: [PATCH 3/3] fix: cherry-pick the landedCommitSha for security releases Sometimes we need to include a commit that already landed on `main` in a security release --- lib/cherry_pick.js | 6 ++++ lib/landing_session.js | 69 +++++++++++++++++++++++++++++++++--------- lib/queries/PR.gql | 5 ++- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/lib/cherry_pick.js b/lib/cherry_pick.js index 9e14ebe3..03d1a903 100644 --- a/lib/cherry_pick.js +++ b/lib/cherry_pick.js @@ -93,6 +93,12 @@ export default class CherryPick { }, false, cli); this.expectedCommitShas = metadata.data.commits.map(({ commit }) => commit.oid); + // In security releases the fix may have already landed publicly, so the + // PR is merged and refs/pull//merge no longer exists - pick the + // commits as they landed on the base branch instead. + this.landedCommitSha = metadata.data.pr.merged + ? metadata.data.pr.mergeCommit?.oid + : undefined; if (this.promptAmend) { const amend = await cli.prompt( diff --git a/lib/landing_session.js b/lib/landing_session.js index b79bf59d..a509db8d 100644 --- a/lib/landing_session.js +++ b/lib/landing_session.js @@ -82,7 +82,9 @@ export default class LandingSession extends Session { } async downloadAndPatch() { - const { cli, upstream, prid, expectedCommitShas, crossRepoPR } = this; + const { + cli, upstream, prid, expectedCommitShas, crossRepoPR, landedCommitSha + } = this; if (crossRepoPR) { const { owner, repo } = this; @@ -107,6 +109,12 @@ export default class LandingSession extends Session { { ignoreFailure: false }); } } while (!isHeadAMergeCommit()); + } else if (landedCommitSha) { + // The PR already landed (e.g. a security fix released publicly), so + // its merge ref is gone - fetch the commits from the base branch. + cli.startSpinner(`Downloading landed commits for ${prid}`); + await forceRunAsync('git', ['fetch', upstream, landedCommitSha], + { ignoreFailure: false }); } else { cli.startSpinner(`Downloading patch for ${prid}`); await runAsync('git', [ @@ -114,26 +122,57 @@ export default class LandingSession extends Session { `refs/pull/${prid}/merge`]); } - // We fetched the commit that would result if we used `git merge`. - // ^1 and ^2 refer to the PR base and the PR head, respectively. - const [base, head, rebaseHead] = await runAsync('git', - ['rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2', 'HEAD'], - { captureStdout: 'lines' }); + let base, head, rebaseHead; + if (landedCommitSha && !crossRepoPR) { + // FETCH_HEAD is the last commit of the PR as landed on the base branch. + // Walk back while the PR-URL trailer still points to this PR to find + // the base of the landed series (landing may have squashed commits). + const prUrlRe = new RegExp(`^PR-URL: \\S+/pull/${prid}$`, 'm'); + let count = 0; + while (count < expectedCommitShas.length) { + const message = await runAsync('git', + ['log', '-1', '--format=%B', `FETCH_HEAD~${count}`], + { captureStdout: true }); + if (!prUrlRe.test(message)) break; + count++; + } + if (count === 0) { + cli.error(`Landed commit ${landedCommitSha} does not have a ` + + `PR-URL trailer pointing to pull request ${prid}`); + process.exit(1); + } + [base, head, rebaseHead] = await runAsync('git', + ['rev-parse', `FETCH_HEAD~${count}`, 'FETCH_HEAD', 'HEAD'], + { captureStdout: 'lines' }); + } else { + // We fetched the commit that would result if we used `git merge`. + // ^1 and ^2 refer to the PR base and the PR head, respectively. + [base, head, rebaseHead] = await runAsync('git', + ['rev-parse', 'FETCH_HEAD^1', 'FETCH_HEAD^2', 'HEAD'], + { captureStdout: 'lines' }); + } const commitShas = await runAsync('git', ['rev-list', `${base}..${head}`], { captureStdout: 'lines' }); cli.stopSpinner(`Fetched commits as ${shortSha(base)}..${shortSha(head)}`); cli.separator(); - const mismatchedCommits = [ - ...commitShas.filter((sha) => !expectedCommitShas.includes(sha)) - .map((sha) => `Unexpected commit ${sha}`), - ...expectedCommitShas.filter((sha) => !commitShas.includes(sha)) - .map((sha) => `Missing commit ${sha}`) - ].join('\n'); - if (mismatchedCommits.length > 0) { - cli.error(`Mismatched commits:\n${mismatchedCommits}`); - process.exit(1); + if (landedCommitSha && !crossRepoPR) { + // The landed commits have different SHAs from the ones in the PR, so + // they cannot be compared - they were validated by their PR-URL + // trailers above. + cli.ok(`Using ${commitShas.length} commit(s) as landed for PR ${prid}`); + } else { + const mismatchedCommits = [ + ...commitShas.filter((sha) => !expectedCommitShas.includes(sha)) + .map((sha) => `Unexpected commit ${sha}`), + ...expectedCommitShas.filter((sha) => !commitShas.includes(sha)) + .map((sha) => `Missing commit ${sha}`) + ].join('\n'); + if (mismatchedCommits.length > 0) { + cli.error(`Mismatched commits:\n${mismatchedCommits}`); + process.exit(1); + } } const commitInfo = { base, head, shas: commitShas, rebaseHead }; diff --git a/lib/queries/PR.gql b/lib/queries/PR.gql index 27cc1396..19c86a3a 100644 --- a/lib/queries/PR.gql +++ b/lib/queries/PR.gql @@ -31,7 +31,10 @@ query PR($prid: Int!, $owner: String!, $repo: String!) { closed, closedAt, merged, - mergedAt + mergedAt, + mergeCommit { + oid + } } } }