diff --git a/script/upstream/transforms/transform-package-json.test.ts b/script/upstream/transforms/transform-package-json.test.ts index 11016f0e135..a239bcc5edf 100644 --- a/script/upstream/transforms/transform-package-json.test.ts +++ b/script/upstream/transforms/transform-package-json.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test" -import { fixCatalog, fixScripts } from "./transform-package-json" +import { fixCatalog, fixScripts, mergeWithNewestVersions } from "./transform-package-json" test("fixScripts preserves Kilo-only root scripts from base", () => { const ours = { @@ -84,3 +84,33 @@ test("fixCatalog is a no-op when catalog is absent", () => { fixCatalog(pkg, "package.json", changes) expect(changes.length).toBe(0) }) + +test("mergeWithNewestVersions preserves ours' key order so kilo-only deps don't relocate", () => { + // Regression: when ours has a kilo-only dep in the middle (e.g. rotating-file-stream + // alphabetically between npm-package-arg and semver) and theirs lacks it, the merge + // result must keep that key in its original position. Previously this function + // started from theirs' keys and appended ours-only keys at the end, causing git's + // textual 3-way merge to produce a duplicate JSON key. + const ours = { + "npm-package-arg": "13.0.2", + "rotating-file-stream": "3.2.9", + semver: "^7.6.3", + zod: "catalog:", + } + const theirs = { + "npm-package-arg": "13.0.2", + semver: "^7.6.3", + zod: "catalog:", + } + const changes: string[] = [] + const result = mergeWithNewestVersions(ours, theirs, changes, "dependencies") + expect(Object.keys(result)).toEqual(["npm-package-arg", "rotating-file-stream", "semver", "zod"]) +}) + +test("mergeWithNewestVersions appends theirs-only keys at the end", () => { + const ours = { a: "1.0.0", b: "1.0.0" } + const theirs = { a: "1.0.0", c: "1.0.0" } + const changes: string[] = [] + const result = mergeWithNewestVersions(ours, theirs, changes, "dependencies") + expect(Object.keys(result)).toEqual(["a", "b", "c"]) +}) diff --git a/script/upstream/transforms/transform-package-json.ts b/script/upstream/transforms/transform-package-json.ts index 327ee30facc..45f396a18ff 100644 --- a/script/upstream/transforms/transform-package-json.ts +++ b/script/upstream/transforms/transform-package-json.ts @@ -108,8 +108,14 @@ function compareVersions(a: string, b: string): number | null { /** * Merge two dependency objects using "newest wins" strategy * For non-comparable versions (URLs, catalog:, workspace:*), upstream (theirs) wins + * + * Key order preserves ours' order first (so kilo-only deps stay in their + * original position), then appends theirs-only keys at the end. This avoids + * relocating existing keys, which would otherwise let git's textual merge + * produce duplicate JSON keys (ours keeps the line in place, theirs appears + * to "add" the same key elsewhere → both survive the merge). */ -function mergeWithNewestVersions( +export function mergeWithNewestVersions( ours: Record | undefined, theirs: Record | undefined, changes: string[], @@ -117,38 +123,38 @@ function mergeWithNewestVersions( ): Record { const result: Record = {} - // Start with all of theirs - if (theirs) { - for (const [name, version] of Object.entries(theirs)) { - result[name] = version + // Seed with ours' keys in ours' order, applying newest-wins per key. + if (ours) { + for (const [name, ourVersion] of Object.entries(ours)) { + const theirVersion = theirs?.[name] + if (theirVersion === undefined) { + result[name] = ourVersion + changes.push(`${section}: preserved ${name}@${ourVersion} (kilo-only)`) + continue + } + if (ourVersion === theirVersion) { + result[name] = theirVersion + continue + } + const cmp = compareVersions(ourVersion, theirVersion) + if (cmp === null) { + result[name] = theirVersion + changes.push(`${section}: ${name} kept upstream ${theirVersion} (special format)`) + } else if (cmp > 0) { + result[name] = ourVersion + changes.push(`${section}: ${name} ${theirVersion} -> ${ourVersion} (kilo newer)`) + } else { + result[name] = theirVersion + if (cmp < 0) changes.push(`${section}: ${name} kept upstream ${theirVersion} (upstream newer)`) + } } } - // Merge in ours, keeping newer versions - if (ours) { - for (const [name, ourVersion] of Object.entries(ours)) { - const theirVersion = result[name] - - if (!theirVersion) { - // Dependency only exists in ours - keep it - result[name] = ourVersion - changes.push(`${section}: preserved ${name}@${ourVersion} (kilo-only)`) - } else if (ourVersion !== theirVersion) { - // Both have it with different versions - compare - const comparison = compareVersions(ourVersion, theirVersion) - - if (comparison === null) { - // Can't compare (special format) - upstream wins per user preference - changes.push(`${section}: ${name} kept upstream ${theirVersion} (special format)`) - } else if (comparison > 0) { - // Ours is newer - result[name] = ourVersion - changes.push(`${section}: ${name} ${theirVersion} -> ${ourVersion} (kilo newer)`) - } else if (comparison < 0) { - // Theirs is newer - already in result - changes.push(`${section}: ${name} kept upstream ${theirVersion} (upstream newer)`) - } - // If equal, keep theirs (already in result) + // Append any theirs-only keys at the end, preserving theirs' relative order. + if (theirs) { + for (const [name, version] of Object.entries(theirs)) { + if (result[name] === undefined) { + result[name] = version } } }