diff --git a/.github/DEVELOPING_V3.md b/.github/DEVELOPING_V3.md index 34a72190329..62045fc745f 100644 --- a/.github/DEVELOPING_V3.md +++ b/.github/DEVELOPING_V3.md @@ -177,6 +177,14 @@ If the PR says the lockfile was **deferred** (a `package.json` / `pnpm-workspace conflicted too), resolve the manifests first, then regenerate it with `pnpm install --lockfile-only` and include the result in your fix commit. +Watch for the **"Deleted on one side, changed on the other"** section. Git leaves no markers +for a delete/modify, so the branch looks clean where it is not: the merge keeps `3.x`'s side +(its deletion, or its file when `master` deleted it). Confirm that is right, and check +whether `master`'s change has to be carried over by hand — when a breaking commit re-recorded +or renamed a file, `master`'s edit to the old one usually belongs on the replacement, and no +automation can find that for you. The PR names the `master` commit behind each conflicted +path so you can see what the change was. + Then **merge the PR with the normal merge button.** `master`'s commits arrive as-is and your fix stays its own commit. **Never close a conflict PR unmerged** — closing resolves nothing, the same conflict reopens on the next sync, and the new PR will call out the abandoned one. @@ -194,9 +202,10 @@ is squashed, and the tree guard proves the result is exactly the merge of `3.x` `master`. **Who gets pinged.** The conflict is attributed to the authors of the `3.x` commits behind the -conflicted files (`.github/scripts/sync-conflict-owners.mjs`, mapped to GitHub accounts). -Those authors are **requested as reviewers** on the conflict PR and listed in the -`#alerts-v3-sync` message. +conflicted files, plus the `master` commits that touched the same files +(`.github/scripts/sync-conflict-owners.mjs`, mapped to GitHub accounts). Both sides are named +in the PR body and the `#alerts-v3-sync` message. **Nobody is requested as a reviewer** — the +resolver picks the PR up themselves. ## Trialing v3 diff --git a/.github/WORKFLOWS.md b/.github/WORKFLOWS.md index c826c1d5a27..7a1a11b51a8 100644 --- a/.github/WORKFLOWS.md +++ b/.github/WORKFLOWS.md @@ -415,9 +415,11 @@ to mechanical, tool-generated files (the pnpm lockfile, bot-maintained data file `MECHANICAL_PATHS` in `sync-master-to-3x.mjs`) are auto-resolved during the replay; the tree check then applies to every path except those files. On a real code conflict `3.x` is left untouched and a draft PR carrying the conflict markers (labeled `automation:v3-sync`, with -mechanical files pre-resolved) is opened on `sync/master-to-3x`, requesting the -breaking-commit authors as reviewers via `sync-conflict-owners.mjs`, posting to -`#alerts-v3-sync` and pausing further syncs until it is resolved and merged normally. +mechanical files pre-resolved) is opened on `sync/master-to-3x`, naming both ends of the +conflict — the breaking-commit authors and the `master` commits that touched the same files +— via `sync-conflict-owners.mjs`, posting to `#alerts-v3-sync` and pausing further syncs +until it is resolved and merged normally. Delete/modify conflicts have no markers to carry, +so they are resolved toward `3.x` and listed as an explicit decision in the PR body. `build-v3-nightly.yml` publishes `n8nio/n8n:v3-nightly[-]` images from `3.x` by calling `docker-build-push.yml` with `ref: 3.x` + `date_tag`. On Mondays it also retags that run's n8n + runners manifests as a release candidate (by digest on GHCR, so diff --git a/.github/scripts/sync-conflict-owners.mjs b/.github/scripts/sync-conflict-owners.mjs index 656e756c94a..ba5d6a74053 100644 --- a/.github/scripts/sync-conflict-owners.mjs +++ b/.github/scripts/sync-conflict-owners.mjs @@ -1,6 +1,7 @@ #!/usr/bin/env node /** - * Attributes a master→3.x sync conflict to the authors of the breaking commits. + * Attributes a master→3.x sync conflict to BOTH sides: the authors of the breaking commits + * that diverged 3.x, and the master commits that touched the same files. * * Since 3.x = master + breaking commits, the commits that diverged 3.x are exactly * `..` (where is the fetched master SHA and the pre-rebase 3.x @@ -8,12 +9,18 @@ * conflict; their GitHub authors are who to nudge. `conflictedFiles` must run while the * rebase is stopped, i.e. with unmerged paths present in the index. * + * The master half is the other end of the same conflict, and knowing which master commit + * touched the file is usually what makes it resolvable (a fixture re-recorded by a + * dependency bump reads as an unexplained clash without it). Nobody is requested as a + * reviewer: the PR body and the Slack post name both sides, and the resolver picks + * themselves. + * * git log gives only the author name/email, and ~2/3 of n8n authors commit with a - * non-noreply email that carries no GitHub username. So the conflicted files → breaking - * SHAs analysis is done locally, and a SINGLE GraphQL call maps those few SHAs to GitHub + * non-noreply email that carries no GitHub username. So the conflicted files → commit + * analysis is done locally, and a SINGLE GraphQL call maps those few SHAs to GitHub * logins. Bot- and unlinked-account commits resolve to a null user and are skipped. * - * Emits a JSON object to stdout: { ownersCsv, slack, body }. + * Emits a JSON object to stdout: { slack, body }. * * Usage: * node .github/scripts/sync-conflict-owners.mjs --base --sync-branch @@ -47,10 +54,39 @@ export function breakingShas(base, files, git = runGit, tip = 'HEAD') { return [...shas]; } -// Resolve commit SHAs to GitHub logins in one GraphQL call. Commits whose author has -// no linked account (unverified email, bots) resolve to a null user and are dropped. -export async function resolveLogins(repo, shas, token, fetchFn = fetch) { - if (shas.length === 0) return []; +// Master commits that touched the given files since the branches diverged — the other end +// of the conflict. Capped per file: the point is naming the change, not a full log. +export function masterCommitsByFile(base, files, git = runGit, tip = 'FETCH_HEAD', limit = 3) { + const byFile = new Map(); + for (const file of files) { + const out = git([ + 'log', + `${base}..${tip}`, + `--max-count=${limit}`, + '--format=%H %h %s', + '--', + file, + ]); + const commits = out + .split('\n') + .filter(Boolean) + .map((line) => { + const [sha, short, ...rest] = line.split(' '); + return { sha, short, subject: rest.join(' ') }; + }); + byFile.set(file, commits); + } + return byFile; +} + +/** + * Resolve commit SHAs to GitHub logins in one GraphQL call. Commits whose author has no + * linked account (unverified email, bots) resolve to a null user and are left out. + * + * @returns {Promise>} SHA → login, for the SHAs that resolved. + */ +export async function resolveCommitAuthors(repo, shas, token, fetchFn = fetch) { + if (shas.length === 0) return new Map(); const [owner, name] = repo.split('/'); const aliases = shas .map((sha, i) => `c${i}: object(oid: "${sha}") { ... on Commit { author { user { login } } } }`) @@ -71,35 +107,117 @@ export async function resolveLogins(repo, shas, token, fetchFn = fetch) { if (json.errors) throw new Error(`GitHub GraphQL error: ${JSON.stringify(json.errors)}`); const repository = json.data?.repository ?? {}; - const logins = new Set(); - for (const node of Object.values(repository)) { - const login = node?.author?.user?.login; - if (login) logins.add(login); - } - return [...logins].sort(); + const authors = new Map(); + shas.forEach((sha, i) => { + const login = repository[`c${i}`]?.author?.user?.login; + if (login) authors.set(sha, login); + }); + return authors; } -// Build the conflict-PR body, reviewer CSV, and Slack owner line. +/** + * Both ends of a conflict, in one GraphQL call: the logins behind the target-side + * (breaking) commits, and the master commits that touched each conflicted path with their + * authors. Degrades to empty attribution — a transient API failure must still let the + * conflict PR open. + * + * `base` is the fetched master SHA (= the target branch's base, since 3.x = master + N), + * `tip` the pre-merge target tip, and the master half is read from where the two diverged. + */ +export async function gatherAttribution({ + repo, + token, + files, + base, + tip = 'HEAD', + git = runGit, + fetchFn = fetch, + log = console.error, +}) { + const targetShas = breakingShas(base, files, git, tip); + + let byFile = new Map(); + try { + byFile = masterCommitsByFile(git(['merge-base', tip, base]), files, git, base); + } catch (error) { + log(`warning: could not read the master side of the conflict: ${error.message}`); + } + + const masterShas = [...byFile.values()].flat().map((c) => c.sha); + let authors = new Map(); + try { + authors = await resolveCommitAuthors(repo, [...targetShas, ...masterShas], token, fetchFn); + } catch (error) { + log(`warning: could not resolve owners: ${error.message}`); + } + + return { + owners: [...new Set(targetShas.map((sha) => authors.get(sha)).filter(Boolean))].sort(), + masterCommits: new Map( + [...byFile].map(([file, commits]) => [ + file, + commits.map((c) => ({ ...c, login: authors.get(c.sha) })), + ]), + ), + }; +} + +/** + * Build the conflict-PR body and the Slack owner line. + * + * `files` are the marker-carrying conflicts. `deleteConflicts` are the ones git left + * WITHOUT markers ({ path, deletedBy: 'target' | 'master' }) — they need their own section + * or a resolver reads the branch as "nothing to fix here". `masterCommits` maps a + * conflicted path to the master commits that touched it, so each entry names both ends of + * the clash. Nobody is requested as a reviewer. + */ export function buildOutputs({ syncBranch, targetBranch = '3.x', files, owners, + deleteConflicts = [], + masterCommits = new Map(), preResolved = [], lockfileDeferred = false, abandoned = [], }) { - const filesMd = files.map((f) => `- \`${f}\``).join('\n') || '_none detected_'; + // `- \`path\`` plus a nested line per master commit that touched it. + const fileMd = (path, suffix = '') => { + const commits = masterCommits.get(path) ?? []; + return [ + `- \`${path}\`${suffix}`, + ...commits.map( + ({ short, subject, login }) => + ` - master: \`${short}\` ${subject}${login ? ` — @${login}` : ''}`, + ), + ].join('\n'); + }; + const filesMd = files.map((f) => fileMd(f)).join('\n') || '_none detected_'; + const deleteMd = deleteConflicts + .map(({ path, deletedBy }) => + fileMd( + path, + deletedBy === 'master' + ? ` — deleted on master, changed on \`${targetBranch}\`; the merge kept \`${targetBranch}\`'s file` + : ` — deleted on \`${targetBranch}\`, changed on master; the merge kept \`${targetBranch}\`'s deletion`, + ), + ) + .join('\n'); const ownersMd = owners.length ? owners.map((o) => `- @${o}`).join('\n') : '_Could not auto-attribute — review the conflicted files manually._'; const abandonedWarning = abandoned.length ? `A previous PR for this recurring conflict (${abandoned.map((pr) => `#${pr.number}`).join(', ')}) was closed without being merged. Closing resolves nothing — the conflict comes back on the next sync. **Merge, don't close.**` : ''; + const masterOwners = [ + ...new Set([...masterCommits.values()].flat().flatMap((c) => (c.login ? [c.login] : []))), + ].sort(); const slack = [ owners.length ? `Likely owners (GitHub): ${owners.map((o) => `@${o}`).join(' ')}` : 'Could not auto-attribute owners.', + masterOwners.length ? `· master side: ${masterOwners.map((o) => `@${o}`).join(' ')}` : '', abandoned.length ? `⚠️ ${abandoned.map((pr) => `<${pr.url}|#${pr.number}>`).join(', ')} was closed without merging and the conflict is back — merge this one, don't close it.` : '', @@ -110,18 +228,28 @@ export function buildOutputs({ `Automated \`master\`→\`${targetBranch}\` sync hit a conflict.`, ...(abandonedWarning ? ['', '> [!WARNING]', `> ${abandonedWarning}`] : []), '', - `**\`${targetBranch}\` was not touched.** This branch is \`master\` merged into \`${targetBranch}\` with the **conflict markers committed**, so you can see exactly what clashed. The required checks stay red until they are resolved, so this PR cannot be merged half-done.`, + files.length + ? `**\`${targetBranch}\` was not touched.** This branch is \`master\` merged into \`${targetBranch}\` with the conflicts committed exactly as git left them — **conflict markers included** — so you can see what clashed. The required checks stay red until they are resolved, so this PR cannot be merged half-done.` + : `**\`${targetBranch}\` was not touched.** This branch is \`master\` merged into \`${targetBranch}\` exactly as git left it. Nothing here carries conflict markers, so the checks can go green on a merge that was resolved by default — read the decision below before merging.`, '', '### How to resolve', `1. \`git fetch origin ${syncBranch} && git switch ${syncBranch}\``, - '2. Fix the conflict markers and commit them **in one commit of your own** (rather than amending the merge).', + '2. Resolve everything listed below and commit it **in one commit of your own** (rather than amending the merge).', `3. \`git push origin ${syncBranch}\``, `4. **Merge this PR with the normal merge button.** \`master\`'s commits come in as-is and your fix stays its own commit — nothing is squashed. Never close this PR unmerged: closing resolves nothing and the same conflict reopens on the next sync.`, '', `**Daily syncs are paused until this PR is merged.** The next sync then replays \`${targetBranch}\` onto master linearly again — which also drops this merge commit and its markers out of \`${targetBranch}\`'s history — and verifies the result is exactly what a merge would produce.`, - '', - '### Conflicted files', - filesMd, + // An empty file list is only worth printing when there is no other section either: + // "_none detected_" is then the signal that detection itself came up short. + ...(files.length || !deleteConflicts.length ? ['', '### Conflicted files', filesMd] : []), + ...(deleteConflicts.length + ? [ + '', + '### Deleted on one side, changed on the other', + `Git leaves **no conflict markers** for these, so the branch looks clean where it is not — the merge kept \`${targetBranch}\`'s side. Decide whether that is right, and whether the other side's change has to be carried over (onto a re-recorded or renamed replacement file, typically):`, + deleteMd, + ] + : []), ...(preResolved.length ? [ '', @@ -139,10 +267,10 @@ export function buildOutputs({ : []), '', '### Likely owners', - `Authors of the ${targetBranch} commits behind the conflicted files, requested as reviewers:`, + `Authors of the ${targetBranch} commits behind the conflicted files — nobody is requested as a reviewer, so pick this up between yourselves:`, ownersMd, ].join('\n'); - return { ownersCsv: owners.join(','), slack, body }; + return { slack, body }; } async function main() { @@ -164,16 +292,7 @@ async function main() { if (!token) throw new Error('GH_TOKEN / GITHUB_TOKEN env var is required'); const files = conflictedFiles(); - const shas = breakingShas(base, files); - - // Degrade gracefully: a transient API failure should still open the PR - // (unattributed) rather than fail the whole sync. - let owners = []; - try { - owners = await resolveLogins(repo, shas, token); - } catch (error) { - console.error(`warning: could not resolve owners: ${error.message}`); - } + const { owners, masterCommits } = await gatherAttribution({ repo, token, files, base }); process.stdout.write( JSON.stringify( @@ -182,6 +301,7 @@ async function main() { targetBranch: values['target-branch'], files, owners, + masterCommits, }), ), ); diff --git a/.github/scripts/sync-conflict-owners.test.mjs b/.github/scripts/sync-conflict-owners.test.mjs index 9552e00ccba..c45d2e8a8d8 100644 --- a/.github/scripts/sync-conflict-owners.test.mjs +++ b/.github/scripts/sync-conflict-owners.test.mjs @@ -1,7 +1,13 @@ import assert from 'node:assert/strict'; import { test } from 'node:test'; -import { breakingShas, resolveLogins, buildOutputs } from './sync-conflict-owners.mjs'; +import { + breakingShas, + masterCommitsByFile, + resolveCommitAuthors, + gatherAttribution, + buildOutputs, +} from './sync-conflict-owners.mjs'; test('breakingShas collects unique SHAs across the conflicted files only', () => { const calls = []; @@ -29,7 +35,97 @@ test('breakingShas can be scoped to an explicit tip (the pre-rebase 3.x tip)', ( assert.deepEqual(calls[0], ['log', 'BASE..PREHEAD', '--format=%H', '--', 'a.ts']); }); -test('resolveLogins maps SHAs to logins in one call, dropping unlinked/bot authors', async () => { +test('masterCommitsByFile reads the master side of each conflicted file, capped', () => { + const calls = []; + const git = (args) => { + calls.push(args); + return args.at(-1) === 'a.ts' + ? 'full1 short1 fix(core): a thing (#1)\nfull2 short2 chore: another' + : ''; + }; + + const byFile = masterCommitsByFile('BASE', ['a.ts', 'b.ts'], git, 'MASTER', 3); + + assert.deepEqual(byFile.get('a.ts'), [ + { sha: 'full1', short: 'short1', subject: 'fix(core): a thing (#1)' }, + { sha: 'full2', short: 'short2', subject: 'chore: another' }, + ]); + assert.deepEqual(byFile.get('b.ts'), []); + assert.deepEqual(calls[0], [ + 'log', + 'BASE..MASTER', + '--max-count=3', + '--format=%H %h %s', + '--', + 'a.ts', + ]); +}); + +test('gatherAttribution resolves both sides of the conflict in a single call', async () => { + const git = (args) => { + if (args[0] === 'merge-base') return 'DIVERGED'; + if (args.includes('--format=%H')) return 'breaking-sha'; + if (args.includes('--format=%H %h %s')) return 'master-sha msha build(core): bump deps (#2)'; + return ''; + }; + let queried = []; + const fetchFn = async (_url, opts) => { + queried.push(JSON.parse(opts.body).query); + return { + ok: true, + json: async () => ({ + data: { + repository: { + c0: { author: { user: { login: 'alice' } } }, + c1: { author: { user: { login: 'bob' } } }, + }, + }, + }), + }; + }; + + const { owners, masterCommits } = await gatherAttribution({ + repo: 'n8n-io/n8n', + token: 't', + files: ['a.ts'], + base: 'MASTER', + tip: 'PREHEAD', + git, + fetchFn, + }); + + assert.equal(queried.length, 1); + assert.deepEqual(owners, ['alice']); + assert.deepEqual(masterCommits.get('a.ts'), [ + { sha: 'master-sha', short: 'msha', subject: 'build(core): bump deps (#2)', login: 'bob' }, + ]); +}); + +test('gatherAttribution still attributes the 3.x side when the master side cannot be read', async () => { + const git = (args) => { + if (args[0] === 'merge-base') throw new Error('no merge base'); + return 'breaking-sha'; + }; + const fetchFn = async () => ({ + ok: true, + json: async () => ({ data: { repository: { c0: { author: { user: { login: 'alice' } } } } } }), + }); + + const { owners, masterCommits } = await gatherAttribution({ + repo: 'r', + token: 't', + files: ['a.ts'], + base: 'MASTER', + git, + fetchFn, + log: () => {}, + }); + + assert.deepEqual(owners, ['alice']); + assert.equal(masterCommits.size, 0); +}); + +test('resolveCommitAuthors maps SHAs to logins in one call, dropping unlinked/bot authors', async () => { let calls = 0; const fetchFn = async (url, opts) => { calls++; @@ -50,35 +146,40 @@ test('resolveLogins maps SHAs to logins in one call, dropping unlinked/bot autho }), }; }; - const owners = await resolveLogins('n8n-io/n8n', ['sha1', 'sha2', 'sha3'], 't', fetchFn); + const authors = await resolveCommitAuthors('n8n-io/n8n', ['sha1', 'sha2', 'sha3'], 't', fetchFn); assert.equal(calls, 1); // single batched request - assert.deepEqual(owners, ['alice', 'bob']); // sorted, deduped, null dropped + assert.deepEqual( + [...authors], + [ + ['sha1', 'bob'], + ['sha2', 'alice'], + ], + ); // sha3's null author is dropped }); -test('resolveLogins makes no request when there are no SHAs', async () => { +test('resolveCommitAuthors makes no request when there are no SHAs', async () => { let calls = 0; const fetchFn = async () => { calls++; return { ok: true, json: async () => ({ data: { repository: {} } }) }; }; - assert.deepEqual(await resolveLogins('r', [], 't', fetchFn), []); + assert.equal((await resolveCommitAuthors('r', [], 't', fetchFn)).size, 0); assert.equal(calls, 0); }); -test('resolveLogins throws on API/GraphQL errors (caller degrades gracefully)', async () => { +test('resolveCommitAuthors throws on API/GraphQL errors (caller degrades gracefully)', async () => { const httpError = async () => ({ ok: false, status: 502, json: async () => ({}) }); const gqlError = async () => ({ ok: true, json: async () => ({ errors: [{ message: 'bad' }] }) }); - await assert.rejects(resolveLogins('r', ['s'], 't', httpError), /502/); - await assert.rejects(resolveLogins('r', ['s'], 't', gqlError), /GraphQL error/); + await assert.rejects(resolveCommitAuthors('r', ['s'], 't', httpError), /502/); + await assert.rejects(resolveCommitAuthors('r', ['s'], 't', gqlError), /GraphQL error/); }); -test('buildOutputs formats reviewers, slack line, and PR body with owners', () => { +test('buildOutputs formats the slack line and PR body with owners', () => { const out = buildOutputs({ syncBranch: 'sync/master-to-3x', files: ['packages/cli/x.ts'], owners: ['alice', 'bob'], }); - assert.equal(out.ownersCsv, 'alice,bob'); assert.equal(out.slack, 'Likely owners (GitHub): @alice @bob'); assert.match(out.body, /### Conflicted files/); assert.match(out.body, /- `packages\/cli\/x\.ts`/); @@ -86,7 +187,8 @@ test('buildOutputs formats reviewers, slack line, and PR body with owners', () = assert.match(out.body, /- @bob/); assert.match(out.body, /Daily syncs are paused until this PR is merged/); // Markers are the review surface; the fix goes in the resolver's own commit. - assert.match(out.body, /conflict markers committed/); + assert.match(out.body, /conflict markers included/); + assert.match(out.body, /nobody is requested as a reviewer/); assert.match(out.body, /in one commit of your own/); assert.match(out.body, /Merge this PR with the normal merge button/); assert.match(out.body, /`3\.x` was not touched/); @@ -99,7 +201,6 @@ test('buildOutputs degrades gracefully when nothing could be attributed', () => files: ['x.ts'], owners: [], }); - assert.equal(out.ownersCsv, ''); assert.equal(out.slack, 'Could not auto-attribute owners.'); assert.match(out.body, /Could not auto-attribute/); }); @@ -141,3 +242,52 @@ test('buildOutputs warns about conflict PRs that were closed without merging', ( assert.match(out.slack, //); assert.match(out.slack, /merge this one, don't close it/); }); + +test('buildOutputs names the master commit behind each conflicted file', () => { + const out = buildOutputs({ + syncBranch: 'sync/master-to-3x', + files: ['packages/cli/x.ts'], + owners: ['alice'], + masterCommits: new Map([ + [ + 'packages/cli/x.ts', + [{ short: 'abc1234', subject: 'build(core): bump deps (#2)', login: 'bob' }], + ], + ]), + }); + assert.match( + out.body, + /- `packages\/cli\/x\.ts`\n {2}- master: `abc1234` build\(core\): bump deps \(#2\) — @bob/, + ); + assert.equal(out.slack, 'Likely owners (GitHub): @alice · master side: @bob'); +}); + +test('buildOutputs gives marker-less delete/modify conflicts their own section', () => { + const out = buildOutputs({ + syncBranch: 'sync/master-to-3x', + files: [], + owners: ['alice'], + deleteConflicts: [ + { path: 'fixtures/a.json', deletedBy: 'target' }, + { path: 'src/b.ts', deletedBy: 'master' }, + ], + masterCommits: new Map([ + ['fixtures/a.json', [{ short: 'abc1234', subject: 'build: re-record (#2)' }]], + ]), + }); + assert.match(out.body, /### Deleted on one side, changed on the other/); + // Nothing keeps the checks red here, so the body must not promise that it does. + assert.match(out.body, /the checks can go green on a merge that was resolved by default/); + assert.match(out.body, /no conflict markers/); + assert.match( + out.body, + /- `fixtures\/a\.json` — deleted on `3\.x`, changed on master; the merge kept `3\.x`'s deletion/, + ); + assert.match( + out.body, + /- `src\/b\.ts` — deleted on master, changed on `3\.x`; the merge kept `3\.x`'s file/, + ); + assert.match(out.body, / {2}- master: `abc1234` build: re-record \(#2\)/); + // Nothing carries markers, so the misleading "Conflicted files" list is dropped. + assert.equal(/### Conflicted files/.test(out.body), false); +}); diff --git a/.github/scripts/sync-master-to-3x.mjs b/.github/scripts/sync-master-to-3x.mjs index 9513a56284e..e1282ab67dc 100644 --- a/.github/scripts/sync-master-to-3x.mjs +++ b/.github/scripts/sync-master-to-3x.mjs @@ -34,16 +34,19 @@ * 4. The content does NOT reconcile on a real code path → a genuinely new conflict. 3.x * is left UNTOUCHED and a draft PR is opened on the sync branch carrying the conflict * markers — with the mechanical files pre-resolved, so the resolver only deals with - * real code — attributed to the authors of the breaking commits behind the conflicted - * files. Syncs pause until it is merged. + * real code — naming both ends of the clash: the authors of the breaking commits and + * the master commits that touched the same files. Delete/modify conflicts leave no + * markers, so they are resolved toward 3.x and reported as an explicit decision + * instead. Syncs pause until it is merged. * * The conflict branch carries the conflict markers, so the resolver sees exactly what clashed - * and the required checks stay red until they fix it in a commit of their own. That PR is - * merged with the normal GitHub merge button — master's commits arrive as-is and the fix stays - * its own commit. NEVER close a conflict PR unmerged: closing resolves nothing and the same - * conflict reopens on the next sync. 3.x itself never has markers at its tip (nightly images - * build from it), and the merge commit holding them is dropped from its history by the next - * replay. + * and the required checks stay red until they fix it in a commit of their own. A conflict git + * left without markers (delete/modify) has no such gate: the PR body carries the decision that + * was made by default and says as much. That PR is merged with the normal GitHub merge button + * — master's commits arrive as-is and the fix stays its own commit. NEVER close a conflict PR + * unmerged: closing resolves nothing and the same conflict reopens on the next sync. 3.x + * itself never has markers at its tip (nightly images build from it), and the merge commit + * holding them is dropped from its history by the next replay. * * Runs from a checkout of the target branch (fetch-depth 0). Assumes credentials are NOT * persisted by checkout — pushes go through an explicit token URL. @@ -71,12 +74,7 @@ import { mergeTree, runGit, } from './branch-replay.mjs'; -import { - conflictedFiles, - breakingShas, - resolveLogins, - buildOutputs, -} from './sync-conflict-owners.mjs'; +import { conflictedFiles, gatherAttribution, buildOutputs } from './sync-conflict-owners.mjs'; // Re-exported so this module stays the single entry point for the master→3.x flow, tests // included. The implementations are branch-pair-agnostic and shared with the bundle replay. @@ -184,6 +182,34 @@ export function resolveQueueSidePath({ git, path, log = console.log }) { } } +/** + * The unmerged paths git left as delete/modify: one side removed the file, the other + * changed it. These need naming separately, because a real merge leaves NO conflict + * markers for them — the working tree simply holds the surviving side, so committing the + * merge as-is silently takes that side with nothing for a resolver to look at. + * + * `ls-files -u` lines are ` \t`: stage 1 base, 2 ours (the target + * branch), 3 theirs (master). A base with one side missing is the delete; no base at all is + * an add/add, which is a normal content conflict and keeps its markers. + * + * @returns {Array<{ path: string, deletedBy: 'target' | 'master' }>} + */ +export function deleteModifyConflicts(git, paths) { + const out = []; + for (const path of paths) { + const stages = new Set( + git(['ls-files', '-u', '--', path]) + .split('\n') + .map((line) => /^\S+ \S+ (\d)\t/.exec(line)?.[1]) + .filter(Boolean), + ); + if (!stages.has('1')) continue; + if (!stages.has('2')) out.push({ path, deletedBy: 'target' }); + else if (!stages.has('3')) out.push({ path, deletedBy: 'master' }); + } + return out; +} + /** * Replay the 3.x-only commits onto master, resolving stalls that involve ONLY mechanical * paths in place — folded into the stalled commit exactly as a human's `rebase --continue` @@ -305,6 +331,11 @@ export function writeGithubOutput(obj, env = process.env) { * cannot be merged half-resolved (an auto-resolved branch would be green with master's * change silently dropped). * + * Delete/modify conflicts have no markers to leave, so they are resolved toward 3.x's side + * — the same side the replay favours — and reported separately. Left to `add -A` they would + * commit master's surviving blob instead, re-adding a file 3.x deleted on purpose with + * nothing in the diff to suggest a decision was made. + * * The lockfile is left with its markers when a manifest is among the code conflicts * (regenerating is meaningless until the manifests are resolved) or when the regen fails * transiently — flagged via `lockfileDeferred` so the PR body carries the instruction. @@ -312,11 +343,18 @@ export function writeGithubOutput(obj, env = process.env) { * 3.x never carries the markers at its tip, and not for long in its history either: this * merge commit is dropped by the next replay, which takes the queue's commits only. * - * @returns {{ files: string[], preResolved: string[], lockfileDeferred: boolean }} - * `files` is the list the PR reports and attributes owners for: the code conflicts, - * or every conflict when none are code (a fallback after a failed auto-resolution). + * @returns {{ files: string[], deleteConflicts: Array<{path: string, deletedBy: string}>, + * preResolved: string[], lockfileDeferred: boolean }} + * `files` is the marker-carrying list the PR reports, or every conflict when none are + * code (a fallback after a failed auto-resolution); owners are attributed for both lists. */ -export function buildConflictBranch({ git, pnpm, masterSha, log = console.log }) { +export function buildConflictBranch({ + git, + pnpm, + masterSha, + target = TARGET_BRANCH, + log = console.log, +}) { const merge = attempt(git, ['merge', '--no-edit', masterSha]); if (merge.ok) { // merge-tree said these conflict; if a real merge disagrees, don't guess. @@ -348,9 +386,28 @@ export function buildConflictBranch({ git, pnpm, masterSha, log = console.log }) } } + // No markers to leave behind for these — resolve toward 3.x and report them instead. + const deleteConflicts = deleteModifyConflicts(git, code); + for (const { path, deletedBy } of deleteConflicts) { + if (deletedBy === 'target') { + log(`Keeping ${target}'s deletion of ${path} (master modified it)...`); + git(['rm', '--force', '--', path]); + } else { + log(`Keeping ${target}'s ${path} (master deleted it)...`); + git(['checkout', '--ours', '--', path]); + git(['add', '--', path]); + } + } + const deleted = new Set(deleteConflicts.map((c) => c.path)); + git(['add', '-A']); git(['commit', '--no-edit', '--no-verify']); - return { files: code.length > 0 ? code : all, preResolved, lockfileDeferred }; + return { + files: code.length > 0 ? code.filter((p) => !deleted.has(p)) : all, + deleteConflicts, + preResolved, + lockfileDeferred, + }; } // Conflict PRs that were recently closed WITHOUT being merged — closing resolves nothing, @@ -377,8 +434,10 @@ export function recentAbandonedConflictPrs( } /** - * Push the marker-carrying conflict branch and open a draft PR, attributing it to the - * authors of the breaking commits behind the conflicted files. 3.x is left untouched. + * Push the marker-carrying conflict branch and open a draft PR naming both ends of the + * conflict: the authors of the breaking commits behind the conflicted files, and the master + * commits that touched the same files. Nobody is requested as a reviewer — the PR body and + * the Slack post are the ping. 3.x is left untouched. * * @returns {Promise<{ prUrl: string, ownersSlack: string }>} */ @@ -392,22 +451,23 @@ export async function openConflictPr({ pushUrl, target = TARGET_BRANCH, files = [], + deleteConflicts = [], preResolved = [], lockfileDeferred = false, fetchFn = fetch, log = console.log, }) { // Attribute against the pre-merge tip: HEAD is the merge commit by now. - const shas = breakingShas(masterSha, files, git, preHead); - - // Degrade gracefully: a transient API failure should still open the PR - // (unattributed) rather than fail the whole sync. - let owners = []; - try { - owners = await resolveLogins(repo, shas, token, fetchFn); - } catch (error) { - log(`warning: could not resolve owners: ${error.message}`); - } + const { owners, masterCommits } = await gatherAttribution({ + repo, + token, + files: [...files, ...deleteConflicts.map((c) => c.path)], + base: masterSha, + tip: preHead, + git, + fetchFn, + log, + }); let abandoned = []; try { @@ -416,11 +476,13 @@ export async function openConflictPr({ log(`warning: could not check for abandoned conflict PRs: ${error.message}`); } - const { ownersCsv, slack, body } = buildOutputs({ + const { slack, body } = buildOutputs({ syncBranch: SYNC_BRANCH, targetBranch: target, files, owners, + deleteConflicts, + masterCommits, preResolved, lockfileDeferred, abandoned, @@ -455,16 +517,6 @@ export async function openConflictPr({ body, ]); - // Request owners as reviewers (best-effort: the API rejects the PR author - // and non-collaborators, so a failure here must not fail the sync). - if (ownersCsv) { - try { - gh(['pr', 'edit', prUrl, '--add-reviewer', ownersCsv]); - } catch { - log(`::warning::could not request some reviewers: ${ownersCsv}`); - } - } - return { prUrl, ownersSlack: slack }; } @@ -576,10 +628,11 @@ export async function sync({ ); } - const { files, preResolved, lockfileDeferred } = buildConflictBranch({ + const { files, deleteConflicts, preResolved, lockfileDeferred } = buildConflictBranch({ git, pnpm, masterSha, + target, log, }); const { prUrl, ownersSlack } = await openConflictPr({ @@ -592,6 +645,7 @@ export async function sync({ pushUrl, target, files, + deleteConflicts, preResolved, lockfileDeferred, fetchFn, diff --git a/.github/scripts/sync-master-to-3x.test.mjs b/.github/scripts/sync-master-to-3x.test.mjs index f520ec66ab1..0a0f7fa184f 100644 --- a/.github/scripts/sync-master-to-3x.test.mjs +++ b/.github/scripts/sync-master-to-3x.test.mjs @@ -8,6 +8,7 @@ import { blocksLockfileRegen, resolveMechanicalPath, resolveQueueSidePath, + deleteModifyConflicts, rebaseResolvingMechanical, reconcileWithMergeTreeAtTip, reconcileLockfileAtTip, @@ -749,6 +750,11 @@ test('buildConflictBranch commits the conflicted state, markers and all', () => assert.deepEqual(preResolved, []); assert.equal(lockfileDeferred, false); assert.deepEqual(git.calls[0], ['merge', '--no-edit', MASTER]); + assert.equal( + git.calls.some((a) => a[0] === 'rm'), + false, + 'a content conflict keeps its markers', + ); assert.ok(git.calls.some((a) => a[0] === 'add' && a[1] === '-A')); assert.ok(git.calls.some((a) => a[0] === 'commit' && a.includes('--no-edit'))); // The markers ARE the review surface here, so nothing may auto-resolve them. @@ -762,6 +768,53 @@ test('buildConflictBranch commits the conflicted state, markers and all', () => ); }); +test('deleteModifyConflicts tells the deleting side apart, and ignores add/add', () => { + const stages = { + 'gone-on-3x.json': '100644 oid 1\tgone-on-3x.json\n100644 oid 3\tgone-on-3x.json', + 'gone-on-master.ts': '100644 oid 1\tgone-on-master.ts\n100644 oid 2\tgone-on-master.ts', + 'both-added.ts': '100644 oid 2\tboth-added.ts\n100644 oid 3\tboth-added.ts', + 'content.ts': '100644 oid 1\tcontent.ts\n100644 oid 2\tcontent.ts\n100644 oid 3\tcontent.ts', + }; + const git = (args) => stages[args.at(-1)] ?? ''; + + assert.deepEqual(deleteModifyConflicts(git, Object.keys(stages)), [ + { path: 'gone-on-3x.json', deletedBy: 'target' }, + { path: 'gone-on-master.ts', deletedBy: 'master' }, + ]); +}); + +test('buildConflictBranch resolves marker-less delete/modify conflicts toward 3.x', () => { + const git = makeStub([ + [(a) => a[0] === 'merge', fail('CONFLICT (modify/delete): fixtures/a.json')], + [isConflictedFiles, 'fixtures/a.json\nsrc/b.ts'], + [ + (a) => a[0] === 'ls-files', + (a) => + a.at(-1) === 'fixtures/a.json' + ? '100644 oid 1\tfixtures/a.json\n100644 oid 3\tfixtures/a.json' + : '100644 oid 1\tsrc/b.ts\n100644 oid 2\tsrc/b.ts', + ], + ]); + + const { files, deleteConflicts } = buildConflictBranch({ + git, + pnpm: makeStub(), + masterSha: MASTER, + log: () => {}, + }); + + // Both are marker-less, so neither belongs in the "conflicted files" list. + assert.deepEqual(files, []); + assert.deepEqual(deleteConflicts, [ + { path: 'fixtures/a.json', deletedBy: 'target' }, + { path: 'src/b.ts', deletedBy: 'master' }, + ]); + // 3.x deleted the fixture: keep the deletion rather than master's re-added blob. + assert.ok(git.calls.some((a) => a[0] === 'rm' && a.at(-1) === 'fixtures/a.json')); + // master deleted the source file: keep 3.x's. + assert.ok(git.calls.some((a) => a[0] === 'checkout' && a.includes('--ours'))); +}); + test('buildConflictBranch pre-resolves mechanical files so only code conflicts remain', () => { const git = makeStub([ [(a) => a[0] === 'merge', fail('CONFLICT')], @@ -864,11 +917,14 @@ test('sync opens a draft conflict PR and leaves 3.x untouched on a real conflict const body = create[create.indexOf('--body') + 1]; assert.match(body, /Merge this PR with the normal merge button/); assert.match(body, /nothing is squashed/); - assert.match(body, /conflict markers committed/); + assert.match(body, /conflict markers included/); + assert.match(body, /- @alice/); - // Owner requested as reviewer. - const edit = gh.calls.find((a) => a[0] === 'pr' && a[1] === 'edit'); - assert.equal(edit[edit.indexOf('--add-reviewer') + 1], 'alice'); + // Reviewers are never requested — the body and the Slack post are the ping. + assert.equal( + gh.calls.some((a) => a[0] === 'pr' && a[1] === 'edit'), + false, + ); // Only the sync branch is pushed — 3.x must not move. const pushes = git.calls.filter((a) => a[0] === 'push'); @@ -937,11 +993,6 @@ test('openConflictPr degrades gracefully when owner resolution fails', async () assert.equal(prUrl, 'https://github.com/n8n-io/n8n/pull/1'); assert.equal(ownersSlack, 'Could not auto-attribute owners.'); - // No reviewer request when there are no owners. - assert.equal( - gh.calls.some((a) => a[0] === 'pr' && a[1] === 'edit'), - false, - ); }); test('openConflictPr calls out a recently abandoned conflict PR', async () => { @@ -976,3 +1027,40 @@ test('openConflictPr calls out a recently abandoned conflict PR', async () => { assert.match(body, /Merge, don't close/); assert.match(ownersSlack, //); }); + +test('sync reports a marker-less delete/modify conflict as its own decision, with the master commit', async () => { + const git = makeStub([ + ...baseGitRoutes.filter((r) => !r[0](['merge-tree']) && !r[0](['merge-base'])), + [(a) => a[0] === 'merge-base' && a[1] === '--is-ancestor', fail()], + [(a) => a[0] === 'merge-base', 'DIVERGED'], + [(a) => a[0] === 'merge-tree', conflictedMergeTree('fixtures/a.json')], + [(a) => a[0] === 'merge', fail('CONFLICT (modify/delete): fixtures/a.json')], + [isConflictedFiles, 'fixtures/a.json'], + [(a) => a[0] === 'ls-files', '100644 oid 1\tfixtures/a.json\n100644 oid 3\tfixtures/a.json'], + [(a) => a[0] === 'log' && a.includes('--format=%H'), 'breaking-sha'], + [(a) => a[0] === 'log' && a.includes('--format=%H %h %s'), 'master-sha msha build: bump (#2)'], + ]); + const gh = makeStub([ + ...noOpenPr, + [(a) => a[0] === 'pr' && a[1] === 'create', 'https://github.com/n8n-io/n8n/pull/99'], + ]); + + await sync({ + git, + gh, + pnpm: makeStub(), + env, + fetchFn: okFetch(['alice', 'bob']), + log: () => {}, + }); + + const create = gh.calls.find((a) => a[0] === 'pr' && a[1] === 'create'); + const body = create[create.indexOf('--body') + 1]; + assert.match(body, /### Deleted on one side, changed on the other/); + assert.match(body, /- `fixtures\/a\.json` — deleted on `3\.x`, changed on master/); + assert.match(body, / {2}- master: `msha` build: bump \(#2\) — @bob/); + // 3.x is still untouched — the decision is the resolver's, only the PR moves. + const pushes = git.calls.filter((a) => a[0] === 'push'); + assert.equal(pushes.length, 1); + assert.equal(pushes[0].at(-1), `HEAD:refs/heads/${SYNC_BRANCH}`); +}); diff --git a/.github/workflows/util-sync-master-to-3x.yml b/.github/workflows/util-sync-master-to-3x.yml index ce8b44f5d59..5b8a0451bd1 100644 --- a/.github/workflows/util-sync-master-to-3x.yml +++ b/.github/workflows/util-sync-master-to-3x.yml @@ -13,11 +13,13 @@ # data files) are resolved in place during the replay — no PR, no commit, no human. When # master genuinely conflicts with 3.x on real code, 3.x is left untouched: a draft conflict # PR carrying the conflict markers (mechanical files pre-resolved) is opened on the sync -# branch and #alerts-v3-sync is notified. The resolver fixes the markers in a commit of their -# own and merges that PR with the normal merge button — never closes it, since closing -# resolves nothing and the conflict returns. No further syncs run until it is merged. 3.x -# itself never has markers at its tip (nightly images build from it) and the merge commit -# holding them drops out of its history at the next replay. +# branch and #alerts-v3-sync is notified. Delete/modify conflicts, which git cannot express +# as markers, are resolved toward 3.x's side and listed in that PR as an explicit decision. +# The resolver fixes the markers in a commit of their own and merges that PR with the normal +# merge button — never closes it, since closing resolves nothing and the conflict returns. +# No further syncs run until it is merged. 3.x itself never has markers at its tip (nightly +# images build from it) and the merge commit holding them drops out of its history at the +# next replay. # # See .github/DEVELOPING_V3.md for the full v3 development model.