diff --git a/blog-post-draft.md b/blog-post-draft.md new file mode 100644 index 00000000..0452cb2b --- /dev/null +++ b/blog-post-draft.md @@ -0,0 +1,206 @@ +# Craft: now challenging changesets and more + +I've [written about the secure release infrastructure before](https://vanguard.getsentry.net/p/cmihvx6b80000isk7akkwm6s8), but this post is about something different: momentum. + +After being in a somewhat dormant state for a while, Craft has been getting a lot of love lately. And I mean _a lot_. Since version 2.12.0 (just about a month ago), we've shipped a flurry of releases with some pretty exciting features. Let me walk you through what's new and why I'm excited about it. + +## Automatic Version Bumping 🎯 + +Remember the days when you had to manually decide if your release was a patch, minor, or major bump? Those days are over. Just run `craft prepare auto` and Craft analyzes your commit messages (conventional commits style) to figure out the version bump for you. If you've got a `feat:` in there, it knows it's a minor bump. A `fix:`? That's a patch. It's like having a tiny version manager living in your commits[^1]. + +But wait, there's more flexibility here. You can also use explicit bump types: + +```bash +craft prepare auto # Let Craft figure it out from commits +craft prepare patch # Force a patch bump +craft prepare minor # Force a minor bump +craft prepare major # Force a major bump +``` + +And for teams that prefer calendar-based versioning, Craft now supports `calver` natively: + +```yaml +versioning: + policy: calver + calver: + format: "%y.%-m" # e.g., 24.12 for December 2024 +``` + +The best part? When using auto-versioning, you get a preview in your PR with a "Semver Impact of This PR" section so you know exactly what _your specific change_ will do to the version. + +## Beautiful Changelogs πŸͺ· + +Craft now groups your changes both by main categories (like Bug Fixes and New Features) _and_ by scope within those categories. The result is a clean, organized changelog that's actually pleasant to read: + +```markdown +### New Features ✨ + +#### Api + +- Add user endpoint by @alice in #1 +- Add auth endpoint by @bob in #2 + +#### Core + +- Improve performance by @charlie in #3 +- General improvement by @dave in #4 +``` + +Those pesky `feat:` and `fix(scope):` prefixes? Stripped automatically. Categories appear in the order you define them. You can even override entries directly in your PR description using a `## Changelog Entry` section. + +### The `craft changelog` Command + +Want to see what the changelog will look like before you release? Now you can: + +```bash +craft changelog # Preview the upcoming changelog +craft changelog --pr 123 # See how a specific PR will appear +craft changelog --since v2.0 # Generate changelog since a specific tag +``` + +### Changelog Previews Everywhere + +This same changelog preview now shows up automatically in your PRs _and_ in publish issues. Here's what it looks like in a publish issue: + +[IMAGE: changelog-preview-publish-issue.png] + +Contributors can see exactly how their changes will appear in the final changelog, and release managers get a clear overview of what's shipping. + +### Smart Revert Handling + +And here's a fun one: **revert handling**. When you revert a commit, Craft now understands that. If both the original and the revert are in the same release, they cancel each other out - neither appears in the changelog nor affects the version bump. If the revert is standalone (the original was in a previous release), it shows up under Bug Fixes with a patch bump. It's almost like Craft understands regret[^2]. We even handle those delightful chains of `Revert "Revert "Revert "..."` correctly. Because of course, we do that. + +## GitHub Actions: All-in-One πŸ”§ + +Speaking of previews in PRs and issues - you might be wondering how that happens. We've merged the separate `action-prepare-release` repository directly into Craft. Why? Because the action was tightly coupled to Craft anyway, and maintaining them separately was creating more friction than it solved. + +This means you now get: + +1. **Reusable workflows** - Just call them from your repo, no boilerplate needed: + +```yaml +# .github/workflows/release.yml +jobs: + release: + uses: getsentry/craft/.github/workflows/release.yml@v2 + with: + version: ${{ inputs.version || 'auto' }} + secrets: inherit +``` + +```yaml +# .github/workflows/changelog-preview.yml +jobs: + preview: + uses: getsentry/craft/.github/workflows/changelog-preview.yml@v2 + secrets: inherit +``` + +2. **Proper outputs** - Get the resolved version, branch, SHA, and changelog back to use in subsequent steps +3. **Dogfooding** - Craft now uses the latest code on master to release itself. We eat our own dog food here. + +For more flexibility, you can also use the composite action directly when you need custom pre/post steps. + +## npm Workspaces Support πŸ“¦ + +For those of you maintaining monorepos with multiple packages, Craft now supports npm workspaces natively. This one's a game-changer for repos like sentry-javascript and sentry-wizard. + +Previously, sentry-javascript had to maintain a [manually curated list of 40+ npm targets](https://github.com/getsentry/sentry-javascript/blob/develop/.craft.yml) in dependency order. And that order? It was [actually incorrect](https://github.com/getsentry/sentry-javascript/pull/18429) in some places. Here's what it looked like: + +```yaml +targets: + # NPM Targets - 40+ manually curated entries in dependency order + - name: npm + id: '@sentry/core' + includeNames: /^sentry-core-\d.*\.tgz$/ + - name: npm + id: '@sentry/types' + includeNames: /^sentry-types-\d.*\.tgz$/ + - name: npm + id: '@sentry/node-core' + includeNames: /^sentry-node-core-\d.*\.tgz$/ + # ... 35+ more entries ... + - name: npm + id: '@sentry/react-router' + includeNames: /^sentry-react-router-\d.*\.tgz$/ +``` + +Now? Just this: + +```yaml +targets: + - name: npm + workspaces: true + excludeWorkspaces: /^@sentry-internal\// +``` + +That's a ~200 line reduction in config, with automatic dependency ordering. Craft reads your `package.json` workspaces config, figures out the dependency graph, and publishes in the correct order. + +We're hoping to expand this feature to other targets like [dotnet/nuget](https://github.com/getsentry/craft/issues/649) in the future. + +## Docker Multi-Registry Support 🐳 + +Pushing your Docker images to multiple registries? Craft now supports that out of the box. Google Artifact Registry, Docker Hub, GitHub Container Registry - push to all of them in a single `craft publish`. We even added support for regional Artifact Registry endpoints because, of course, Google had to make that complicated. + +## And More Quality of Life Improvements ✨ + +We've also shipped a bunch of other improvements you've asked for, like AWS Lambda layer name templating (so you don't have to manually update layer names on major version bumps), regional Artifact Registry endpoint support, and various bug fixes. + +Under the hood, we've modernized the toolchain (ESLint 9, Vitest 3, Zod for schema validation) to make contributing to Craft faster and easier[^3]. + +## New Documentation πŸ“š + +Now you might be thinking: "How am I going to remember how to use all this awesome new stuff?" Glad you asked! + +We used to ~~shove~~ put everything in the README until recently, but Craft has grown into something much larger than its humble beginnings. All these new features made the need for a dedicated docs site even more urgent. So without further ado: + +[IMAGE: craft-docs-site.png] + +**[getsentry.github.io/craft](https://getsentry.github.io/craft/)** - configuration reference, target documentation, GitHub Actions guides, and more. It's all there. + +## Why This Matters Beyond Sentry + +When we first built the release infrastructure at Sentry, it was very much _for_ Sentry. But over time, Craft has evolved into something more general. It's a release management tool that doesn't care if you're shipping npm packages, PyPI wheels, Docker images, Crates, Hex packages, or AWS Lambda layers. It just works. It also nailed the fundamental release flow from the get go: prepare and then publish. + +### How Does Craft Compare to Changesets? + +If you've used [changesets](https://github.com/changesets/action), you might wonder how Craft differs. The main philosophical difference is that Craft doesn't require you to commit changelog-related files into your repo. With changesets, you need to: + +1. Create a changeset file for each PR (an extra step to remember) +2. Manually specify the semver bump type in that file +3. Commit these temporary files to your repo +4. Remove them during release, creating churn + +With Craft, all that information lives where it naturally belongs: in your PR titles, descriptions, and labels. No extra files, no extra steps, no repo churn. The changelog is generated automatically from information that's already there. + +### Beyond Sentry + +With features like automatic versioning, intelligent changelog generation, revert handling, and proper GitHub Actions integration, Craft is becoming a solid option for anyone who wants a consistent, sound, and low-friction release process. And yes, that includes teams outside Sentry. + +Looking ahead, we want to make Craft even more accessible to the broader community. That means splitting the omnibus Docker image, and making the built-in targets more modular, so you can customize Craft for your specific needs without forking the whole thing. + +## Thank You πŸ™Œ + +None of this would have happened without the contributions and feedback from you Sentaurs. Special thanks to: + +- **Miguel** (@betegon) - for code reviews, ideas, motivation +- **Aditya** (@MathurAditya724) - for code reviews, ideas, motivation +- **Ivana** (@sentrivana) - for code reviews, issues, and being an early adopter +- **Hubert** (@hubertdeng123) - for code reviews and support +- **Andrei** (@andreiborza) - for all things AWS Lambda +- **Daniel Szoke** (@Lms24) - for code contributions, documentation, and code reviews +- **Stefan PΓΆlz** (@Flash0ver) - for dotnet fixes and code reviews + +Special thanks to **Stephanie** for organizing the brownbag session in Vienna about Craft - that session was vital in getting everyone engaged and collecting valuable feedback. And to everyone who attended and shared their thoughts: your input shaped many of these improvements. + +## Feedback Welcome! πŸ™ + +Here's the thingβ„’: I'm genuinely excited about where Craft is heading, and I'm eager to make it better. If you've got issues - old ones, new ones, feature requests, or just complaints about that one thing that's been bugging you for years - I want to hear about them. + +Drop me a message, file an issue, or just leave a comment. I'm actively working on Craft and happy to tackle whatever comes up. This is the kind of infrastructure work that quietly makes everyone's life better, and I'm here for it. + +LFG πŸš€ + +[^1]: It's not actually living there. That would be creepy. It just reads the messages. Still creepy? OK moving on. +[^2]: It doesn't. But it handles reverts correctly, which is almost the same thing in software. +[^3]: The Vitest migration alone touches ~30 test files. Don't ask me how I know this. diff --git a/src/utils/__tests__/changelog-generate.test.ts b/src/utils/__tests__/changelog-generate.test.ts index 513be041..d0558fb0 100644 --- a/src/utils/__tests__/changelog-generate.test.ts +++ b/src/utils/__tests__/changelog-generate.test.ts @@ -815,6 +815,327 @@ changelog: expect(result.changelog).toMatchSnapshot(); }); }); + + // ============================================================================ + // Revert commit handling tests + // ============================================================================ + + describe('revert handling', () => { + it('cancels out a simple revert via SHA match', async () => { + // Commit A and Revert A should cancel each other out + // Commits in newest-first order (git log order) + setup([ + { + hash: 'def456', + title: 'Revert "feat: add new feature"', + body: 'This reverts commit abc123.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'abc123', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // Both commits should cancel out, resulting in empty changelog + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + }); + + it('cancels out a simple revert via title fallback', async () => { + // When no SHA in body, fall back to title matching + // Commits in newest-first order (git log order) + setup([ + { + hash: 'def456', + title: 'Revert "feat: add new feature"', + body: '', // No SHA in body + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'abc123', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + }); + + it('keeps standalone revert when original is not in changelog', async () => { + // Revert without corresponding original commit stays as bug fix + setup([ + { + hash: 'def456', + title: 'Revert "feat: add feature from previous release"', + body: 'This reverts commit oldsha123.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // Revert should appear in Bug Fixes category with full title + expect(result.changelog).toContain('Bug Fixes'); + expect(result.changelog).toContain('Revert "feat: add feature from previous release"'); + expect(result.bumpType).toBe('patch'); + }); + + it('handles double revert correctly (A -> Revert A -> Revert Revert A)', async () => { + // A -> B (Revert A) -> C (Revert B) + // Expected: C cancels B, A remains + // Commits in newest-first order (git log order) + setup([ + { + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + { + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // B and C cancel out, A remains + expect(result.changelog).toContain('New Features'); + expect(result.changelog).toContain('Add feature'); + expect(result.changelog).not.toContain('Revert'); + expect(result.bumpType).toBe('minor'); + }); + + it('handles triple revert correctly (all cancel out)', async () => { + // A -> B (Revert A) -> C (Revert B) -> D (Revert C) + // Processing newest first: + // D cancels C, B cancels A -> nothing remains + // Commits in newest-first order (git log order) + setup([ + { + hash: 'ddd444', + title: 'Revert "Revert "Revert "feat: add feature"""', + body: 'This reverts commit ccc333.', + pr: { local: '4', remote: { number: '4', author: { login: 'dave' } } }, + }, + { + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + { + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // D cancels C, B cancels A -> empty + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + }); + + it('handles quadruple revert correctly (original remains)', async () => { + // A -> B -> C -> D -> E (each reverts previous) + // Processing newest first: + // E cancels D, C cancels B, A remains + // Commits in newest-first order (git log order) + setup([ + { + hash: 'eee555', + title: 'Revert "Revert "Revert "Revert "feat: add feature""""', + body: 'This reverts commit ddd444.', + pr: { local: '5', remote: { number: '5', author: { login: 'eve' } } }, + }, + { + hash: 'ddd444', + title: 'Revert "Revert "Revert "feat: add feature"""', + body: 'This reverts commit ccc333.', + pr: { local: '4', remote: { number: '4', author: { login: 'dave' } } }, + }, + { + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + { + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // E cancels D, C cancels B, A remains + expect(result.changelog).toContain('New Features'); + expect(result.changelog).toContain('Add feature'); + expect(result.changelog).not.toContain('Revert'); + expect(result.bumpType).toBe('minor'); + }); + + it('SHA matching takes precedence over title matching', async () => { + // Two commits with same title, revert SHA points to first one + // Only first one should be canceled, second remains + // Commits in newest-first order (git log order) + setup([ + { + hash: 'ccc333', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', // SHA points to first + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + { + hash: 'bbb222', + title: 'feat: add feature', // Same title as first + body: '', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // First feat and revert cancel, second feat remains + expect(result.changelog).toContain('Add feature'); + expect(result.changelog).not.toContain('Revert'); + expect(result.bumpType).toBe('minor'); + }); + + it('handles revert with PR number suffix in title', async () => { + // GitHub often includes PR number in title like: Revert "feat: add feature (#1)" + // Commits in newest-first order (git log order) + setup([ + { + hash: 'def456', + title: 'Revert "feat: add feature (#1)" (#2)', + body: 'This reverts commit abc123.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'abc123', + title: 'feat: add feature (#1)', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + expect(result.changelog).toBe(''); + }); + + it('uses PR title for matching when available', async () => { + // PR title differs from commit title, should use PR title for matching + // Commits in newest-first order (git log order) + setup([ + { + hash: 'def456', + title: 'Revert "feat: add feature"', // Matches PR title, not commit title + body: '', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'abc123', + title: 'wip commit message', + body: '', + pr: { + local: '1', + remote: { + number: '1', + author: { login: 'alice' }, + title: 'feat: add feature', // PR title is different + }, + }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + expect(result.changelog).toBe(''); + }); + + it('extracts SHA from body with additional explanation text', async () => { + // Revert body often contains explanation in addition to the "This reverts commit" line + // Commits in newest-first order (git log order) + setup([ + { + hash: 'def456abc123', + title: 'Revert "feat: add new feature"', + body: `This reverts commit abc123def456. + +The feature caused performance issues in production. +We need to investigate further before re-enabling. + +See incident report: https://example.com/incident/123`, + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'abc123def456', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // Both should cancel out despite the additional text in the body + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + }); + + it('detects revert from body when title does not follow standard format', async () => { + // Sometimes the title may not follow the "Revert "..."" format, + // but the body still contains "This reverts commit " + // Commits in newest-first order (git log order) + setup([ + { + hash: 'def456abc123', + title: 'fix: undo the new feature due to issues', // Non-standard revert title + body: '', + pr: { + local: '2', + remote: { + number: '2', + author: { login: 'bob' }, + body: 'This reverts commit abc123def456.\n\nThe feature caused problems.', + }, + }, + }, + { + hash: 'abc123def456', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + ], null); + const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); + // Both should cancel out because body contains the revert magic string with SHA + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + }); + }); }); describe('generateChangelogWithHighlight', () => { @@ -826,6 +1147,9 @@ describe('generateChangelogWithHighlight', () => { const mockGetChangesSince = getChangesSince as MockedFunction< typeof getChangesSince >; + const dummyGit = { + fetch: vi.fn().mockResolvedValue(undefined), + } as unknown as SimpleGit; beforeEach(() => { vi.resetAllMocks(); @@ -853,32 +1177,40 @@ describe('generateChangelogWithHighlight', () => { }); }); - function setupPRInfo( - _prNumber: number, - options: { + interface HighlightSetupOptions { + currentPR: { + number: number; title: string; body?: string; author?: string; labels?: string[]; baseRef?: string; - }, - ): void { + headSha?: string; + }; + existingCommits: TestCommit[]; + releaseConfig?: string | null; + } + + function setup(options: HighlightSetupOptions): void { + const { currentPR, existingCommits, releaseConfig } = options; + + // Mock GitHub API for current PR mockRestClient.pulls.get.mockResolvedValueOnce({ data: { - title: options.title, - body: options.body ?? '', - user: { login: options.author ?? 'test-author' }, - base: { ref: options.baseRef ?? 'main' }, + title: currentPR.title, + body: currentPR.body ?? '', + user: { login: currentPR.author ?? 'testuser' }, + base: { ref: currentPR.baseRef ?? 'main' }, + head: { sha: currentPR.headSha ?? 'pr-head-sha' }, }, }); mockRestClient.issues.listLabelsOnIssue.mockResolvedValueOnce({ - data: (options.labels ?? []).map(name => ({ name })), + data: (currentPR.labels ?? []).map(name => ({ name })), }); - } - function setupExistingCommits(commits: TestCommit[]): void { + // Mock git changes (existing commits in base branch) mockGetChangesSince.mockResolvedValueOnce( - commits.map(commit => ({ + existingCommits.map(commit => ({ hash: commit.hash, title: commit.title, body: commit.body, @@ -886,9 +1218,10 @@ describe('generateChangelogWithHighlight', () => { })), ); + // Mock GraphQL for commit info mockGraphqlClient.mockResolvedValueOnce({ repository: Object.fromEntries( - commits.map(({ hash, author, title, pr }: TestCommit) => [ + existingCommits.map(({ hash, author, title, pr }: TestCommit) => [ `C${hash}`, { author: { user: author }, @@ -913,75 +1246,314 @@ describe('generateChangelogWithHighlight', () => { ]), ), }); + + if (releaseConfig !== undefined) { + if (releaseConfig === null) { + getConfigFileDirMock.mockReturnValue(undefined); + } else { + getConfigFileDirMock.mockReturnValue('/workspace'); + readFileSyncMock.mockImplementation((path: any) => { + if ( + typeof path === 'string' && + path.includes('.github/release.yml') + ) { + return releaseConfig; + } + const error: any = new Error('ENOENT'); + error.code = 'ENOENT'; + throw error; + }); + } + } } - it('returns accurate statistics reflecting all commits processed', async () => { - const mockGit = { - fetch: vi.fn().mockResolvedValue(undefined), - } as unknown as SimpleGit; + describe('revert handling in PR preview', () => { + it('cancels out revert PR with its target commit', async () => { + setup({ + currentPR: { + number: 2, + title: 'Revert "feat: add feature"', + body: 'This reverts commit abc123.\n\nReverting due to issues.', + author: 'bob', + }, + existingCommits: [ + { + hash: 'abc123', + title: 'feat: add feature', + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 2); - // Setup PR info for the current PR (PR #100) - setupPRInfo(100, { - title: 'feat: new feature from PR', - labels: ['enhancement'], - baseRef: 'main', + // Both should cancel out + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + expect(result.prSkipped).toBe(false); }); - // Setup 3 existing commits in the changelog - setupExistingCommits([ - { - hash: 'abc123', - title: 'feat: existing feature 1', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, - { - hash: 'def456', - title: 'fix: existing bug fix', - body: '', - pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, - }, - { - hash: 'ghi789', - title: 'docs: update readme', - body: '', - pr: { - local: '3', - remote: { number: '3', author: { login: 'charlie' } }, + it('returns correct bump type when revert cancels and other commits remain', async () => { + setup({ + currentPR: { + number: 3, + title: 'Revert "feat: add feature"', + body: 'This reverts commit abc123.', + author: 'charlie', }, - }, - ]); - - const result = await generateChangelogWithHighlight(mockGit, '1.0.0', 100); - - // totalCommits should be 4 (1 current PR + 3 existing commits) - expect(result.totalCommits).toBe(4); - // matchedCommitsWithSemver should reflect all commits that matched categories with semver - // Default config has feat->minor, fix->patch, docs->patch - expect(result.matchedCommitsWithSemver).toBeGreaterThan(1); - expect(result.prSkipped).toBe(false); - // The changelog should contain entries from all commits - expect(result.changelog).toContain('New feature from PR'); - expect(result.changelog).toContain('Existing feature 1'); + existingCommits: [ + { + hash: 'abc123', + title: 'feat: add feature', + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + { + hash: 'def456', + title: 'fix: bug fix', + body: '', + pr: { + local: '2', + remote: { number: '2', author: { login: 'bob' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 3); + + // Revert and feat cancel out, only fix remains + expect(result.changelog).toContain('Bug Fixes'); + expect(result.changelog).toContain('Bug fix'); + expect(result.changelog).not.toContain('Add feature'); + expect(result.changelog).not.toContain('Revert'); + // Bump type should be patch (from fix), not minor (from cancelled feat) + expect(result.bumpType).toBe('patch'); + }); + + it('returns remaining commits bump type when revert PR cancels (PR 676 interaction)', async () => { + // This test verifies the interaction between PR 676 fix and revert handling: + // When a revert PR cancels another commit, the bump type should come from + // the REMAINING commits (not the revert PR's own bump type which would be 'patch'). + setup({ + currentPR: { + number: 3, + title: 'Revert "fix: bug fix"', // Revert's own bump type would be 'patch' + body: 'This reverts commit def456.', + author: 'charlie', + }, + existingCommits: [ + { + hash: 'def456', + title: 'fix: bug fix', // This gets cancelled + body: '', + pr: { + local: '2', + remote: { number: '2', author: { login: 'bob' } }, + }, + }, + { + hash: 'abc123', + title: 'feat: new feature', // This remains -> minor bump + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 3); + + // Revert and fix cancel out, feat remains + expect(result.changelog).toContain('New Features'); + expect(result.changelog).toContain('New feature'); + expect(result.changelog).not.toContain('Bug fix'); + expect(result.changelog).not.toContain('Revert'); + // Bump type should be 'minor' (from remaining feat), NOT 'patch' (revert's own bump type) + expect(result.bumpType).toBe('minor'); + }); + + it('handles double revert PR correctly', async () => { + // existingCommits are in git log order (newest first) + // Current PR is prepended, so final order is: [currentPR, def456, abc123] + // Processing newest first: currentPR reverts def456 -> remove both -> abc123 remains + setup({ + currentPR: { + number: 3, + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit def456.', + author: 'charlie', + }, + existingCommits: [ + // Newest first (git log order) + { + hash: 'def456', + title: 'Revert "feat: add feature"', + body: 'This reverts commit abc123.', + pr: { + local: '2', + remote: { number: '2', author: { login: 'bob' } }, + }, + }, + { + hash: 'abc123', + title: 'feat: add feature', + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 3); + + // Current PR cancels def456, abc123 remains + expect(result.changelog).toContain('New Features'); + expect(result.changelog).toContain('Add feature'); + expect(result.changelog).not.toContain('Revert'); + expect(result.bumpType).toBe('minor'); + }); + + it('keeps standalone revert PR when target not in commits', async () => { + setup({ + currentPR: { + number: 2, + title: 'Revert "feat: old feature"', + body: 'This reverts commit oldsha123.', + author: 'bob', + }, + existingCommits: [ + { + hash: 'abc123', + title: 'fix: unrelated fix', + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 2); + + // Revert stays (target not in list), fix also stays + expect(result.changelog).toContain('Bug Fixes'); + expect(result.changelog).toContain('Revert "feat: old feature"'); + expect(result.changelog).toContain('Unrelated fix'); + expect(result.bumpType).toBe('patch'); + }); + + it('highlights current PR entry in changelog', async () => { + setup({ + currentPR: { + number: 2, + title: 'feat: new feature', + body: 'Adding a great new feature.', + author: 'bob', + }, + existingCommits: [ + { + hash: 'abc123', + title: 'fix: bug fix', + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 2); + + // Current PR should be highlighted with blockquote + expect(result.changelog).toContain('> - New feature'); + expect(result.changelog).toContain('Bug fix'); + expect(result.bumpType).toBe('minor'); + }); + + it('returns PR-specific bump type, not aggregated bump from all commits (PR 676 fix)', async () => { + // This test verifies the fix from PR 676: the bump type should reflect + // THIS PR's contribution, not the aggregated bump from all commits. + setup({ + currentPR: { + number: 2, + title: 'fix: small bug fix', // patch-level change + body: 'Fixing a minor bug.', + author: 'bob', + }, + existingCommits: [ + { + hash: 'abc123', + title: 'feat: major new feature', // minor-level change in history + body: '', + pr: { + local: '1', + remote: { number: '1', author: { login: 'alice' } }, + }, + }, + ], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 2); + + // Both entries should appear in changelog + expect(result.changelog).toContain('Small bug fix'); + expect(result.changelog).toContain('Major new feature'); + // Bump type should be 'patch' (this PR's contribution), NOT 'minor' (from aggregated) + expect(result.bumpType).toBe('patch'); + }); }); - it('returns totalCommits: 1 when PR is skipped', async () => { - const mockGit = { - fetch: vi.fn().mockResolvedValue(undefined), - } as unknown as SimpleGit; + describe('skip-changelog handling', () => { + it('returns empty changelog when PR has skip label', async () => { + setup({ + currentPR: { + number: 2, + title: 'feat: skip this', + body: '', + author: 'bob', + labels: ['skip-changelog'], + }, + existingCommits: [], + }); + + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 2); - // Setup PR info with skip-changelog label - setupPRInfo(100, { - title: 'chore: internal change', - labels: ['skip-changelog'], - baseRef: 'main', + expect(result.changelog).toBe(''); + expect(result.prSkipped).toBe(true); + // Even skipped PRs contribute to version bump based on title + expect(result.bumpType).toBe('minor'); }); - const result = await generateChangelogWithHighlight(mockGit, '1.0.0', 100); + it('returns empty changelog when PR body has skip magic word', async () => { + setup({ + currentPR: { + number: 2, + title: 'fix: internal change', + body: 'This is internal.\n\n#skip-changelog', + author: 'bob', + }, + existingCommits: [], + }); - // When skipped, stats reflect only the PR itself - expect(result.totalCommits).toBe(1); - expect(result.prSkipped).toBe(true); - expect(result.changelog).toBe(''); + const result = await generateChangelogWithHighlight(dummyGit, '1.0.0', 2); + + expect(result.changelog).toBe(''); + expect(result.prSkipped).toBe(true); + expect(result.bumpType).toBe('patch'); + }); }); }); diff --git a/src/utils/__tests__/changelog-utils.test.ts b/src/utils/__tests__/changelog-utils.test.ts index ed875594..3fd20129 100644 --- a/src/utils/__tests__/changelog-utils.test.ts +++ b/src/utils/__tests__/changelog-utils.test.ts @@ -15,6 +15,10 @@ import { shouldSkipCurrentPR, getBumpTypeForPR, stripTitle, + isRevertCommit, + extractRevertedTitle, + extractRevertedSha, + processReverts, SKIP_CHANGELOG_MAGIC_WORD, BODY_IN_CHANGELOG_MAGIC_WORD, type CurrentPRInfo, @@ -87,6 +91,7 @@ describe('shouldSkipCurrentPR', () => { author: 'alice', labels: [], baseRef: 'main', + headSha: 'abc123', }; it('returns false when PR has no skip magic word', () => { @@ -109,6 +114,7 @@ describe('getBumpTypeForPR', () => { author: 'alice', labels: [], baseRef: 'main', + headSha: 'abc123', }; it('returns major for breaking changes', () => { @@ -267,3 +273,176 @@ describe('stripTitle', () => { }); }); }); + +describe('isRevertCommit', () => { + it('returns true for standard revert title format', () => { + expect(isRevertCommit('Revert "feat: add feature"')).toBe(true); + }); + + it('returns false for non-revert title', () => { + expect(isRevertCommit('feat: add feature')).toBe(false); + expect(isRevertCommit('fix: something')).toBe(false); + }); + + it('returns true when body contains revert magic string', () => { + expect(isRevertCommit('fix: undo feature', 'This reverts commit abc123def.')).toBe(true); + }); + + it('returns false when neither title nor body indicates revert', () => { + expect(isRevertCommit('fix: something', 'Just a regular fix')).toBe(false); + }); + + it('handles case-insensitive text matching in body', () => { + // The "This reverts commit" text is matched case-insensitively + // (SHAs in git are always lowercase, so we only test the text part) + expect(isRevertCommit('fix: undo', 'THIS REVERTS COMMIT abc123def.')).toBe(true); + expect(isRevertCommit('fix: undo', 'this Reverts Commit abc123def.')).toBe(true); + }); +}); + +describe('extractRevertedSha', () => { + it('extracts SHA from standard git revert message', () => { + expect(extractRevertedSha('This reverts commit abc123def456.')).toBe('abc123def456'); + }); + + it('extracts SHA without trailing period', () => { + expect(extractRevertedSha('This reverts commit abc123def456')).toBe('abc123def456'); + }); + + it('extracts SHA from body with additional text', () => { + const body = `This reverts commit abc123def456. + +The feature caused issues in production.`; + expect(extractRevertedSha(body)).toBe('abc123def456'); + }); + + it('returns null when no SHA found', () => { + expect(extractRevertedSha('Just a regular commit body')).toBeNull(); + }); + + it('handles abbreviated SHA (7 chars)', () => { + expect(extractRevertedSha('This reverts commit abc1234.')).toBe('abc1234'); + }); + + it('handles full SHA (40 chars)', () => { + const fullSha = 'abc123def456789012345678901234567890abcd'; + expect(extractRevertedSha(`This reverts commit ${fullSha}.`)).toBe(fullSha); + }); +}); + +describe('extractRevertedTitle', () => { + // The regex uses greedy matching (.+) which correctly handles nested quotes + // because the final " is anchored to the end of the string (before optional PR suffix). + // This means .+ will consume as much as possible while still allowing + // the pattern to match, effectively capturing everything between the first " + // after 'Revert ' and the last " before the end/PR suffix. + + it('extracts title from simple revert', () => { + expect(extractRevertedTitle('Revert "feat: add feature"')).toBe('feat: add feature'); + }); + + it('extracts title from revert with PR suffix', () => { + expect(extractRevertedTitle('Revert "feat: add feature" (#123)')).toBe('feat: add feature'); + }); + + it('extracts title from double revert (nested quotes)', () => { + // Revert "Revert "feat: add feature"" + // The greedy .+ matches: Revert "feat: add feature" + expect(extractRevertedTitle('Revert "Revert "feat: add feature""')).toBe( + 'Revert "feat: add feature"' + ); + }); + + it('extracts title from triple revert (deeply nested quotes)', () => { + // Revert "Revert "Revert "feat: add feature""" + expect(extractRevertedTitle('Revert "Revert "Revert "feat: add feature"""')).toBe( + 'Revert "Revert "feat: add feature""' + ); + }); + + it('extracts title from quadruple revert', () => { + // Revert "Revert "Revert "Revert "feat: add feature"""" + expect(extractRevertedTitle('Revert "Revert "Revert "Revert "feat: add feature""""')).toBe( + 'Revert "Revert "Revert "feat: add feature"""' + ); + }); + + it('extracts title with quotes in the message', () => { + // Revert "fix: handle "special" case" + expect(extractRevertedTitle('Revert "fix: handle "special" case"')).toBe( + 'fix: handle "special" case' + ); + }); + + it('extracts title from double revert with PR suffix', () => { + expect(extractRevertedTitle('Revert "Revert "feat: add feature"" (#456)')).toBe( + 'Revert "feat: add feature"' + ); + }); + + it('returns null for non-revert titles', () => { + expect(extractRevertedTitle('feat: add feature')).toBeNull(); + expect(extractRevertedTitle('Revert without quotes')).toBeNull(); + }); + + it('returns null for malformed revert titles', () => { + // Missing closing quote + expect(extractRevertedTitle('Revert "feat: add feature')).toBeNull(); + // Extra text after closing quote (without PR format) + expect(extractRevertedTitle('Revert "feat: add feature" extra')).toBeNull(); + }); +}); + +describe('processReverts', () => { + // Helper to create a minimal RawCommitInfo-like object + const commit = ( + hash: string, + title: string, + body = '', + prTitle?: string, + prBody?: string + ) => ({ + hash, + title, + body, + prTitle, + prBody, + labels: [] as string[], + }); + + it('returns empty array for empty input', () => { + expect(processReverts([])).toEqual([]); + }); + + it('cancels out revert and original via SHA', () => { + // Commits in newest-first order (git log order) + const commits = [ + commit('def456', 'Revert "feat: add feature"', 'This reverts commit abc123.'), + commit('abc123', 'feat: add feature'), + ]; + const result = processReverts(commits); + expect(result).toEqual([]); + }); + + it('cancels out revert and original via title fallback', () => { + // Commits in newest-first order (git log order) + const commits = [ + commit('def456', 'Revert "feat: add feature"'), + commit('abc123', 'feat: add feature'), + ]; + const result = processReverts(commits); + expect(result).toEqual([]); + }); + + it('keeps standalone revert when original not in list', () => { + const commits = [ + commit('def456', 'Revert "feat: old feature"', 'This reverts commit oldsha.'), + ]; + const result = processReverts(commits); + expect(result).toHaveLength(1); + expect(result[0].hash).toBe('def456'); + }); + + // Note: "current PR preview scenario" tests are covered by integration tests + // in changelog-generate.test.ts under generateChangelogWithHighlight +}); diff --git a/src/utils/changelog.ts b/src/utils/changelog.ts index 5029f219..a37953de 100644 --- a/src/utils/changelog.ts +++ b/src/utils/changelog.ts @@ -23,6 +23,8 @@ export interface CurrentPRInfo { labels: string[]; /** Base branch ref (e.g., "master") for computing merge base */ baseRef: string; + /** Head commit SHA of the PR branch */ + headSha: string; } /** @@ -55,6 +57,7 @@ async function fetchPRInfo(prNumber: number): Promise { author: pr.user?.login ?? '', labels: labels.map(l => l.name), baseRef: pr.base.ref, + headSha: pr.head.sha, }; } @@ -138,6 +141,57 @@ function escapeLeadingUnderscores(text: string): string { return text.replace(/(^| )_/, '$1\\_'); } +/** + * Checks if a commit is a revert commit. + * Revert commits are detected by: + * 1. Title starting with 'Revert "' (standard GitHub format) + * 2. Body containing 'This reverts commit ' (git revert format) + * + * @param title The commit or PR title + * @param body Optional commit or PR body to check + * @returns true if this is a revert commit + */ +export function isRevertCommit(title: string, body?: string): boolean { + // Check title for standard GitHub revert format + if (title.startsWith('Revert "')) { + return true; + } + // Check body for git revert magic string + if (body && /This reverts commit [a-f0-9]{7,40}/i.test(body)) { + return true; + } + return false; +} + +/** + * Extracts the SHA of the reverted commit from a revert commit's body. + * Git revert commits typically include "This reverts commit ." in the body. + * + * @param body The commit body text + * @returns The SHA (7-40 chars) if found, or null + */ +export function extractRevertedSha(body: string): string | null { + const match = body.match(/This reverts commit ([a-f0-9]{7,40})/i); + return match?.[1] ?? null; +} + +/** + * Extracts the original commit title from a revert commit's title. + * Handles the format: Revert "original title" with optional PR suffix (#123). + * + * @param title The revert commit title + * @returns The original title if extractable, or null + */ +export function extractRevertedTitle(title: string): string | null { + // Match: Revert "..." with optional PR suffix (#123) + // The greedy .+ correctly handles nested quotes (e.g., Revert "Revert "feat"") + // because it consumes as much as possible while the final " is anchored to the + // end of the string (before the optional PR suffix). This means for nested + // reverts, .+ captures everything between the first " and the last ". + const match = title.match(/^Revert "(.+)"(?:\s*\(#\d+\))?$/); + return match?.[1] ?? null; +} + /** * Extracts the scope from a conventional commit title. * For example: "feat(api): add endpoint" returns "api" @@ -572,6 +626,117 @@ interface RawCommitInfo { highlight?: boolean; } +/** + * Processes revert commits by canceling out revert/reverted pairs. + * + * When a revert commit and its target are both in the list, both are removed + * (they cancel each other out). Commits are expected in newest-first order + * (as returned by git log), and we process in that order to handle nested + * reverts correctly: + * - A: original commit (oldest) + * - B: Revert A + * - C: Revert B (Revert Revert A, newest) + * + * Processing newest first: C cancels B, leaving A in the changelog. + * + * Matching strategy: + * 1. Primary: Match by SHA from "This reverts commit " in body + * 2. Fallback: Match by title extracted from 'Revert "original title"' + * + * @param rawCommits Array of commits in newest-first order (git log order) + * @returns Filtered array with canceled revert pairs removed + */ +export function processReverts(rawCommits: RawCommitInfo[]): RawCommitInfo[] { + if (rawCommits.length === 0) { + return []; + } + + // Create a working set of commits (we'll remove canceled pairs) + const remaining = new Set(rawCommits); + + // Build lookup maps for efficient matching + // SHA map: hash -> commit (for SHA-based matching) + const byHash = new Map(); + // Title map: effective title -> commit (for title-based fallback) + // Use PR title if available, otherwise commit title + const byTitle = new Map(); + + for (const commit of rawCommits) { + if (commit.hash) { + byHash.set(commit.hash, commit); + } + const effectiveTitle = (commit.prTitle ?? commit.title).trim(); + // Only set if not already present (first commit with this title wins) + if (!byTitle.has(effectiveTitle)) { + byTitle.set(effectiveTitle, commit); + } + } + + // Process in original order (newest first). + // This ensures the most recent revert is processed first, correctly handling + // chains like: A -> Revert A -> Revert Revert A + for (const commit of rawCommits) { + // Skip if already removed + if (!remaining.has(commit)) { + continue; + } + + const effectiveTitle = (commit.prTitle ?? commit.title).trim(); + const effectiveBody = commit.prBody ?? commit.body; + + if (!isRevertCommit(effectiveTitle, effectiveBody)) { + continue; + } + + // Try to find the reverted commit + let revertedCommit: RawCommitInfo | undefined; + + // Strategy 1: Match by SHA from commit body + const revertedSha = extractRevertedSha(effectiveBody); + if (revertedSha) { + // Try exact match first + revertedCommit = byHash.get(revertedSha); + if (!revertedCommit || !remaining.has(revertedCommit)) { + // Try prefix match for abbreviated SHAs + revertedCommit = undefined; + for (const [hash, c] of byHash) { + if ( + remaining.has(c) && + (hash.startsWith(revertedSha) || revertedSha.startsWith(hash)) + ) { + revertedCommit = c; + break; + } + } + } + } + + // Strategy 2: Fallback to title matching + if (!revertedCommit || !remaining.has(revertedCommit)) { + const revertedTitle = extractRevertedTitle(effectiveTitle); + if (revertedTitle) { + const candidate = byTitle.get(revertedTitle); + if (candidate && remaining.has(candidate)) { + revertedCommit = candidate; + } + } + } + + // If we found the reverted commit and it's still in the remaining set, + // remove both (they cancel each other out) + if (revertedCommit && remaining.has(revertedCommit)) { + remaining.delete(commit); + remaining.delete(revertedCommit); + logger.debug( + `Revert cancellation: "${effectiveTitle}" cancels "${(revertedCommit.prTitle ?? revertedCommit.title).trim()}"` + ); + } + } + + // Return commits in original order, filtered to only remaining ones + return rawCommits.filter(commit => remaining.has(commit)); +} + /** * Valid semver bump types for auto-versioning */ @@ -628,7 +793,12 @@ export const DEFAULT_RELEASE_CONFIG: ReleaseConfig = { }, { title: 'Bug Fixes πŸ›', - commit_patterns: ['^(?fix(?:\\((?[^)]+)\\))?!?:\\s*)'], + commit_patterns: [ + '^(?fix(?:\\((?[^)]+)\\))?!?:\\s*)', + // Standalone reverts (where original isn't in changelog) are treated as bug fixes + // No capture group - we want to keep the full "Revert ..." title + '^Revert "', + ], semver: 'patch', }, { @@ -1229,7 +1399,7 @@ export async function generateChangelogWithHighlight( // Step 6: Add current PR to the list with highlight flag (at the beginning) const currentPRCommit: RawCommitInfo = { - hash: '', + hash: prInfo.headSha, title: prInfo.title.trim(), body: prInfo.body, author: prInfo.author, @@ -1239,20 +1409,33 @@ export async function generateChangelogWithHighlight( labels: prInfo.labels, highlight: true, }; + // Prepend current PR to make it newest in the list. + // Git log returns commits newest-first, so this maintains that order. const allCommits = [currentPRCommit, ...rawCommits]; - // Step 7: Run categorization on combined list (for changelog generation only) - const { data: rawData, stats } = categorizeCommits(allCommits); + // Step 7: Process reverts - cancel out revert/reverted pairs + const filteredCommits = processReverts(allCommits); + + // Step 8: Check if current PR was cancelled by revert processing + const currentPRSurvived = filteredCommits.some(c => c.highlight); - // Step 8: Serialize to markdown + // Step 9: Run categorization on filtered list + const { data: rawData, stats } = categorizeCommits(filteredCommits); + + // Step 10: Serialize to markdown const changelog = await serializeChangelog(rawData, MAX_LEFTOVERS); - // Return PR-specific bump type, not the aggregate from all commits - // But use accurate statistics from all commits processed for the changelog + // Determine bump type: + // - If current PR survived revert processing, use its specific bump type (PR 676 fix) + // This shows "what is this PR's contribution to the version bump" + // - If current PR was cancelled (it's a revert that cancelled another commit), + // show the remaining commits' aggregated bump type (the net effect on the release) + const bumpType = currentPRSurvived ? prBumpType : stats.bumpType; + return { changelog, prSkipped: false, - bumpType: prBumpType, + bumpType, totalCommits: stats.totalCommits, matchedCommitsWithSemver: stats.matchedCommitsWithSemver, }; @@ -1443,7 +1626,9 @@ async function generateRawChangelog( until?: string ): Promise { const rawCommits = await fetchRawCommitInfo(git, rev, until); - return categorizeCommits(rawCommits); + // Process reverts: cancel out revert/reverted pairs + const filteredCommits = processReverts(rawCommits); + return categorizeCommits(filteredCommits); } /**