feat: add script to auto-resolve CHANGELOG.md merge conflicts - #9994
feat: add script to auto-resolve CHANGELOG.md merge conflicts#9994Mrtenz wants to merge 10 commits into
Conversation
Every consumer-facing change requires an Unreleased changelog entry, so packages/*/CHANGELOG.md conflicts constantly when multiple PRs land around the same time, even though there's usually no real disagreement about content. This adds `yarn changelog:merge` to automatically resolve those conflicts by taking the union of entries added on each side, using @metamask/auto-changelog's parsing/stringification so category ordering and formatting stay correct. Files it can't confidently merge are left with their conflict markers intact for manual resolution.
Extract package.json reading into its own function instead of mutating a local variable, derive the merged entry count from array length deltas instead of a manual counter, replace insertion-index loops with findIndex, narrow exported types to only what's used outside the module, and use @metamask/utils#getErrorMessage instead of a hand-rolled instanceof check.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Keying merged entries by PR number alone meant a PR that legitimately adds multiple distinct changelog bullets would have all but the first treated as duplicates and silently dropped. Key on the PR number and description together instead, and add a regression test.
During a rebase, "theirs" is the commit being replayed, which can already contain an entry that also exists in "ours" (the branch being rebased onto). Since "ours" is usually the side more likely to already overlap with "theirs", using it as the merge base keeps shared entries in their existing position and only appends genuinely new entries from "theirs", instead of the reverse producing a confusing reordering.
Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cd4046c. Configure here.
| repoUrl: string; | ||
| tagPrefix: string; | ||
| }): Promise<{ content: string; mergedEntryCount: number }> { | ||
| // `ours` is used as the base to mutate and stringify. During a Git merge, |
There was a problem hiding this comment.
I find the ours/theirs versus base/ìncomingnaming differences across the file a bit confusing.
There was a problem hiding this comment.
Yeah, good point. ours/theirs is Git terminology, but base/incoming may be easier to understand. I can rename everything to the latter if you're good with that.
There was a problem hiding this comment.
Hmm, unsure about renaming everything, since "base" in Git means something different than "ours" so that could potentially be confusing too. Right now it uses "ours" and "theirs" in places where dealing with Git, and "base" and "incoming" in tool-agnostic merge logic.
There was a problem hiding this comment.
Yeah, I'm not quite sure either. But the current naming is confusing as well IMO
There was a problem hiding this comment.
I'm okay with keeping ours/theirs throughout the whole thing so there are fewer terms to keep straight (and theirs is understandable as the incoming change, to me), but up to you.
| ); | ||
| await execa('git', ['add', changelogPath], { cwd: ROOT_WORKSPACE }); | ||
|
|
||
| resolved.push({ path: changelogPath, mergedEntryCount }); |
There was a problem hiding this comment.
Should the returned paths be normalized?
There was a problem hiding this comment.
It returns the path as used by Git. Do you think it should return OS-style paths instead? That would break when using Git on Windows with these paths.
There was a problem hiding this comment.
Hmm, I was thinking these would get logged to the terminal and might not be clickable for example if they were in the wrong format. Unsure how Windows deals with that actually
There was a problem hiding this comment.
Yeah, good point. I'm not able to test it (don't have a Windows dev machine), but can just add normalisation and see if anyone runs into issues. 😅
There was a problem hiding this comment.
Looks like it works either way, probably not worth the trouble then
There was a problem hiding this comment.
Just pushed. Should I revert then?
There was a problem hiding this comment.
Now that you've done it, I guess we can just keep it 😄
Writing the merged result without running `git add` lets the user review the resolution (and the state of any other conflicted files) before deciding what to stage, rather than silently adding to the index on their behalf.
changelogPath stays forward-slash internally (git commands and path.posix calls need that form), but the path surfaced in the resolved/skipped results is what gets printed to the terminal, where a Windows-style backslash path is more likely to be recognized as a clickable link.
| * Files that can't be automatically merged are left with their conflict | ||
| * markers intact, and cause the script to exit with a non-zero code. | ||
| * | ||
| * Usage: `tsx scripts/merge-changelog-conflicts.ts` |
There was a problem hiding this comment.
Side note: Once we merge #9976 then we can start on the path of removing tsx entirely, since Node now supports reading TypeScript files directly (we will have to enable the erasableTypesOnly option, which will force us to remove enums).
| const CHANGELOG_PATH_PATTERN = /^packages\/[^/]+\/CHANGELOG\.md$/u; | ||
|
|
||
| /** | ||
| * A regular expression for breaking entries. Assumes the entry is in the |
There was a problem hiding this comment.
We don't strictly enforce this, but there are a few places where we assume this in auto-changelog, so it's probably a safe assumption here too.
| export async function resolvePackageMetadata( | ||
| changelogPath: string, | ||
| ): Promise<PackageMetadata> { | ||
| const packageJsonPath = path.posix.join( |
There was a problem hiding this comment.
I've never used path.posix.join before. I always used path.join. What's the difference?
There was a problem hiding this comment.
Oh I see is that related to this? #9994 (comment)
There was a problem hiding this comment.
Yes, and this comment too: #9994 (comment).
path by default uses OS-dependent formatting, so on Linux/macOS (POSIX) path.join('some', 'path') would be some/path, and on Windows some\\path. Git uses POSIX-style paths regardless of OS.
| // already contain entries also present in `theirs`, so preserving its | ||
| // existing order (and only appending genuinely new entries from `theirs`) | ||
| // produces more intuitive results than the reverse. | ||
| const ours = parseChangelog({ |
There was a problem hiding this comment.
Nit: Maybe ourChangelog / theirChangelog?
| * entries that were merged in. | ||
| */ | ||
| export async function mergeChangelogs({ | ||
| oursContent, |
There was a problem hiding this comment.
Nit: Maybe ourContent / theirContent?
| base[category] ??= []; | ||
|
|
||
| addedCount += mergeCategoryEntries( | ||
| base[category] as Change[], | ||
| incomingEntries, | ||
| ); |
There was a problem hiding this comment.
Would this mutate the base argument? Maybe it would be better to say:
| base[category] ??= []; | |
| addedCount += mergeCategoryEntries( | |
| base[category] as Change[], | |
| incomingEntries, | |
| ); | |
| addedCount += mergeCategoryEntries( | |
| base[category] ?? []) as Change[], | |
| incomingEntries, | |
| ); |
| base: ReleaseChanges, | ||
| incoming: ReleaseChanges, | ||
| ): number { | ||
| let addedCount = 0; |
There was a problem hiding this comment.
Nit: It might be helpful to remind ourselves what we are adding:
| let addedCount = 0; | |
| let addedEntriesCount = 0; |
| * @param incoming - The category's changes to merge from. | ||
| * @returns The number of new entries added to `base`. | ||
| */ | ||
| function mergeCategoryEntries(base: Change[], incoming: Change[]): number { |
There was a problem hiding this comment.
Nit: It might be helpful to remind ourselves that these are changes (and to emphasize that they are not release changes):
| function mergeCategoryEntries(base: Change[], incoming: Change[]): number { | |
| function mergeCategoryEntries( | |
| baseChanges: Change[], | |
| incomingChanges: Change[] | |
| ): number { |
| existingKeys.add(key); | ||
|
|
||
| if (isBreakingChange(change)) { | ||
| const firstNonBreakingIndex = base.findIndex( |
There was a problem hiding this comment.
Also to keep in mind: we currently do not enforce that breaking changes are listed first. So I'm curious if this would cause unintentional reordering of entries? I guess that wouldn't be the end of the world, so maybe it's okay if that happens. Just something to think about though.
| * @returns Whether the change is a breaking change. | ||
| */ | ||
| function isBreakingChange(change: Change): boolean { | ||
| return BREAKING_CHANGE_PATTERN.test(change.description.trim()); |
There was a problem hiding this comment.
This is fine and makes sense, but I wonder if eventually we should have auto-changelog look for this prefix and include its presence as an isBreaking property on Change, so we don't keep this knowledge in two places.
3 participants
Explanation
Every consumer-facing change requires a
packages/*/CHANGELOG.mdentry under## [Unreleased], and since many PRs land againstmainaround the same time, that section conflicts constantly — usually because two branches added different bullet points to the same category, not because of any real disagreement about content. Resolving these by hand is repetitive and error-prone (easy to accidentally drop an entry).This adds a
yarn changelog:mergescript that findspackages/*/CHANGELOG.mdfiles with unresolved Git conflicts and computes the union of entries from both sides, using@metamask/auto-changelog's parsing/stringification (parseChangelog/Changelog) rather than hand-rolling Markdown parsing, so category ordering (Added,Changed,Deprecated,Removed,Fixed,Security) and formatting stay correct.Merge behaviour:
git show :2:<path>(ours) andgit show :3:<path>(theirs) — no manual conflict-marker parsing.**BREAKING:**entries below any existing breaking entries in a category, and other new entries at the end.oxfmt, matching the convention used byscripts/update-changelog.sh.References
docs/processes/updating-changelogs.mdChecklist
Note
Low Risk
Developer-only merge helper and tests; it only rewrites conflicted package changelogs in the working tree and does not affect runtime or release automation unless someone runs it manually.
Overview
Adds
yarn changelog:merge, a maintainer tool that finds unresolved conflicts inpackages/*/CHANGELOG.mdduring a merge/rebase and writes a merged file without touching conflict markers in unmergeable cases.Conflict sides are loaded from Git stages (
:2ours,:3theirs) and merged with@metamask/auto-changelog(parseChangelog+oxfmt) so formatting matches existing changelog tooling.mergeChangelogsunions[Unreleased]and per-release entries, dedupes by PR number + description, keeps ours ordering for shared bullets, places new BREAKING lines after existing breaking entries, and inserts release sections in descending SemVer order when only one side has them. Package metadata comes from each package’spackage.json(with a fallback to the ours stage ifpackage.jsonis conflicted).The CLI logs resolved/skipped paths, suggests
yarn changelog:validate, and exits 1 when any file is skipped; extensive Jest coverage covers merge edge cases and the entrypoint.Reviewed by Cursor Bugbot for commit a4d18c6. Bugbot is set up for automated code reviews on this repo. Configure here.