From 21b6c147286f8707ebc98e8c7469b25f42f6a6b3 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Sun, 26 Jul 2026 00:40:13 -0700 Subject: [PATCH] fix(ci): publish validated conflict resolution trees --- test/pr-merge-conflict-fixer.test.ts | 100 +++++++++++++++++----- tools/pr-merge-conflict-fixer/publish.mts | 47 ++++++---- 2 files changed, 108 insertions(+), 39 deletions(-) diff --git a/test/pr-merge-conflict-fixer.test.ts b/test/pr-merge-conflict-fixer.test.ts index 4cc712448b..064d6b2a92 100644 --- a/test/pr-merge-conflict-fixer.test.ts +++ b/test/pr-merge-conflict-fixer.test.ts @@ -99,24 +99,60 @@ function createConflictFixture(): { git(repository, ["config", "user.email", "conflict-fixer@example.test"]); git(repository, ["config", "commit.gpgsign", "false"]); write(repository, "conflict.txt", "shared\n"); - git(repository, ["add", "conflict.txt"]); + write(repository, "clean-merge.txt", "first\nkeep-1\nkeep-2\nkeep-3\nkeep-4\nkeep-5\nlast\n"); + git(repository, ["add", "conflict.txt", "clean-merge.txt"]); git(repository, ["commit", "-m", "test: add shared file"]); git(repository, ["checkout", "-b", "pull-request"]); write(repository, "conflict.txt", "pull request\n"); - git(repository, ["add", "conflict.txt"]); + write( + repository, + "clean-merge.txt", + "pull request\nkeep-1\nkeep-2\nkeep-3\nkeep-4\nkeep-5\nlast\n", + ); + git(repository, ["add", "conflict.txt", "clean-merge.txt"]); git(repository, ["commit", "-m", "test: change PR side"]); const headSha = git(repository, ["rev-parse", "HEAD"]); git(repository, ["checkout", "main"]); write(repository, "conflict.txt", "main branch\n"); + write( + repository, + "clean-merge.txt", + "first\nkeep-1\nkeep-2\nkeep-3\nkeep-4\nkeep-5\nmain branch\n", + ); write(repository, "main-only.txt", "main\n"); - git(repository, ["add", "conflict.txt", "main-only.txt"]); + git(repository, ["add", "conflict.txt", "clean-merge.txt", "main-only.txt"]); git(repository, ["commit", "-m", "test: change main side"]); const baseSha = git(repository, ["rev-parse", "HEAD"]); return { baseSha, headSha, repository }; } +function createMovedFileConflictFixture(): ReturnType { + const repository = temporaryDirectory(); + git(repository, ["init", "--initial-branch=main"]); + git(repository, ["config", "user.name", "Conflict Fixer Test"]); + git(repository, ["config", "user.email", "conflict-fixer@example.test"]); + git(repository, ["config", "commit.gpgsign", "false"]); + write(repository, "adapter.js", "shared\n"); + git(repository, ["add", "adapter.js"]); + git(repository, ["commit", "-m", "test: add shared adapter"]); + + git(repository, ["checkout", "-b", "pull-request"]); + write(repository, "adapter.js", "pull request intent\n"); + git(repository, ["add", "adapter.js"]); + git(repository, ["commit", "-m", "test: change PR adapter"]); + const headSha = git(repository, ["rev-parse", "HEAD"]); + + git(repository, ["checkout", "main"]); + fs.rmSync(path.join(repository, "adapter.js")); + write(repository, "adapter.mts", "main migration\n"); + git(repository, ["add", "-A"]); + git(repository, ["commit", "-m", "test: move main adapter"]); + const baseSha = git(repository, ["rev-parse", "HEAD"]); + return { baseSha, headSha, repository }; +} + function entryFor(fixture: ReturnType): ConflictMatrixEntry { return { base_sha: fixture.baseSha, @@ -227,7 +263,7 @@ describe("PR merge conflict fixer", () => { expect(selected.map((item) => item.pr_number)).toEqual([2]); }); - it("accepts a patch that changes only the original conflict paths (#7542)", () => { + it("accepts a patch that resolves the original conflict paths (#7542)", () => { const fixture = createConflictFixture(); const patchPath = path.join(temporaryDirectory(), "resolution.patch"); const expectedTree = createResolutionPatch(fixture, patchPath); @@ -239,27 +275,43 @@ describe("PR merge conflict fixer", () => { workDirectory: path.join(temporaryDirectory(), "publisher"), }); - expect(result.changedPaths).toEqual(["conflict.txt"]); expect(result.finalTree).toBe(expectedTree); expect(git(result.repository, ["show", `${result.finalTree}:main-only.txt`])).toBe("main"); }); - it("rejects a patch that changes a non-conflict path (#7542)", () => { - const fixture = createConflictFixture(); + it("accepts a resolution that moves PR intent to main's replacement path (#7542)", () => { + const fixture = createMovedFileConflictFixture(); const patchPath = path.join(temporaryDirectory(), "resolution.patch"); - createResolutionPatch(fixture, patchPath, (repository) => { - write(repository, "unrelated.txt", "not part of the conflict\n"); - git(repository, ["add", "unrelated.txt"]); + const repository = path.join(temporaryDirectory(), "resolver"); + const merge = required( + prepareMerge(fixture.repository, repository, fixture.headSha, fixture.baseSha), + "expected a moved-file conflict fixture", + ); + expect(merge.conflictPaths).toEqual(["adapter.js"]); + fs.rmSync(path.join(repository, "adapter.js")); + write(repository, "adapter.mts", "main migration\npull request intent\n"); + git(repository, ["add", "-A"]); + const expectedTree = writeTree(repository); + const patch = execFileSync("git", ["diff", "--binary", merge.conflictTree, expectedTree], { + cwd: repository, }); + fs.writeFileSync(patchPath, patch); - expect(() => - validateResolutionPatch({ - entry: entryFor(fixture), - patchPath, - sourceRepository: fixture.repository, - workDirectory: path.join(temporaryDirectory(), "publisher"), - }), - ).toThrow(/non-conflict paths: unrelated\.txt/u); + const result = validateResolutionPatch({ + entry: { + ...entryFor(fixture), + conflict_paths: ["adapter.js"], + }, + patchPath, + sourceRepository: fixture.repository, + workDirectory: path.join(temporaryDirectory(), "publisher"), + }); + + expect(result.finalTree).toBe(expectedTree); + expect(git(result.repository, ["show", `${result.finalTree}:adapter.mts`])).toBe( + "main migration\npull request intent", + ); + expect(() => git(result.repository, ["show", `${result.finalTree}:adapter.js`])).toThrow(); }); it("rejects changed main state without comparing the live PR head SHA (#7542)", () => { @@ -358,10 +410,16 @@ describe("PR merge conflict fixer", () => { }); expect(JSON.stringify(commitRequest?.body)).not.toMatch(/author|committer|signature/u); const blobRequests = requests.filter((item) => item.path.endsWith("/git/blobs")); - expect(blobRequests).toHaveLength(1); expect( - Buffer.from((blobRequests[0]?.body as { content: string }).content, "base64").toString(), - ).toBe("resolved intent\n"); + blobRequests + .map((item) => Buffer.from((item.body as { content: string }).content, "base64").toString()) + .sort(), + ).toEqual( + [ + "pull request\nkeep-1\nkeep-2\nkeep-3\nkeep-4\nkeep-5\nmain branch\n", + "resolved intent\n", + ].sort(), + ); expect(graphql).toHaveBeenCalledWith(expect.stringContaining("updateRefs"), { input: { clientMutationId: commitSha, diff --git a/tools/pr-merge-conflict-fixer/publish.mts b/tools/pr-merge-conflict-fixer/publish.mts index 24a932464a..c5c91fe02a 100755 --- a/tools/pr-merge-conflict-fixer/publish.mts +++ b/tools/pr-merge-conflict-fixer/publish.mts @@ -12,7 +12,6 @@ import { type ConflictMatrixEntry, parseConflictMatrixEntry } from "./discover.m import { applyResolutionPatch, ConflictFixerError, - listTreeChanges, prepareMerge, replaceWithTree, requireSha, @@ -82,7 +81,6 @@ export function validateResolutionPatch(input: { sourceRepository: string; workDirectory: string; }): { - changedPaths: string[]; finalTree: string; repository: string; } { @@ -100,15 +98,7 @@ export function validateResolutionPatch(input: { replaceWithTree(merge.repository, merge.conflictTree); applyResolutionPatch(merge.repository, input.patchPath); const finalTree = writeTree(merge.repository); - const changedPaths = listTreeChanges(merge.repository, merge.conflictTree, finalTree); - const allowedPaths = new Set(merge.conflictPaths); - const extraPaths = changedPaths.filter((file) => !allowedPaths.has(file)); - if (extraPaths.length > 0) { - throw new ConflictFixerError( - `The resolution patch changes non-conflict paths: ${extraPaths.join(", ")}`, - ); - } - return { changedPaths, finalTree, repository: merge.repository }; + return { finalTree, repository: merge.repository }; } function gitBuffer(repository: string, args: readonly string[]): Buffer { @@ -149,8 +139,13 @@ function changedPathStatuses( return results; } -function treeEntry(repository: string, tree: string, filePath: string): GitTreeEntry { +function optionalTreeEntry( + repository: string, + tree: string, + filePath: string, +): GitTreeEntry | null { const output = gitBuffer(repository, ["ls-tree", "-z", tree, "--", filePath]).toString("utf8"); + if (!output) return null; const separator = output.indexOf("\t"); if (separator < 0) throw new ConflictFixerError(`Git tree does not contain ${filePath}`); const [mode, type, sha] = output.slice(0, separator).split(" "); @@ -160,15 +155,29 @@ function treeEntry(repository: string, tree: string, filePath: string): GitTreeE return { mode, path: filePath, sha, type }; } +function treeEntry(repository: string, tree: string, filePath: string): GitTreeEntry { + const entry = optionalTreeEntry(repository, tree, filePath); + if (!entry) throw new ConflictFixerError(`Git tree does not contain ${filePath}`); + return entry; +} + +function parentContainsBlob( + repository: string, + parent: string, + entry: GitTreeEntry, +): boolean { + const parentEntry = optionalTreeEntry(repository, parent, entry.path); + return parentEntry?.type === "blob" && parentEntry.sha === entry.sha; +} + async function createGitHubTree(input: { - changedPaths: readonly string[]; + baseSha: string; finalTree: string; headSha: string; repository: string; repositoryName: string; request: GitHubRequest; }): Promise { - const changedPaths = new Set(input.changedPaths); const entries: GitTreeEntry[] = []; for (const change of changedPathStatuses(input.repository, input.headSha, input.finalTree)) { const sourceTree = change.status === "D" ? input.headSha : input.finalTree; @@ -177,7 +186,11 @@ async function createGitHubTree(input: { entries.push({ ...entry, sha: null }); continue; } - if (entry.type === "blob" && changedPaths.has(entry.path)) { + if ( + entry.type === "blob" && + !parentContainsBlob(input.repository, input.headSha, entry) && + !parentContainsBlob(input.repository, input.baseSha, entry) + ) { const content = gitBuffer(input.repository, ["cat-file", "blob", entry.sha ?? ""]); const created = (await input.request("POST", `/repos/${input.repositoryName}/git/blobs`, { content: content.toString("base64"), @@ -201,7 +214,6 @@ async function createGitHubTree(input: { } async function publishValidatedTree(input: { - changedPaths: readonly string[]; entry: ConflictMatrixEntry; finalTree: string; graphql: GraphqlRequest; @@ -211,7 +223,7 @@ async function publishValidatedTree(input: { request: GitHubRequest; }): Promise { const tree = await createGitHubTree({ - changedPaths: input.changedPaths, + baseSha: input.entry.base_sha, finalTree: input.finalTree, headSha: input.entry.head_sha, repository: input.repository, @@ -335,7 +347,6 @@ export async function publishResolution(input: { workDirectory: path.join(temporaryDirectory, "repository"), }); return await publishValidatedTree({ - changedPaths: validated.changedPaths, entry: input.entry, finalTree: validated.finalTree, graphql: input.graphql,