From f82f294f0d2b507d6681c9cd6a42dceb64b94567 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sat, 27 Dec 2025 21:13:59 +0300 Subject: [PATCH 1/2] fix(changelog): Handle revert commits properly in changelog and version inference When generating changelogs or inferring version bumps: - Revert commits that cancel out their target (both in current changelog) are removed - Standalone reverts (target not in changelog) appear as bug fixes with patch bump - Nested reverts (Revert Revert...) are handled by processing in reverse chronological order - SHA-based matching is preferred, with title matching as fallback --- .../__tests__/changelog-generate.test.ts | 312 +++++++++++++++++- src/utils/__tests__/changelog-utils.test.ts | 244 ++++++++++++++ src/utils/changelog.ts | 173 +++++++++- 3 files changed, 723 insertions(+), 6 deletions(-) diff --git a/src/utils/__tests__/changelog-generate.test.ts b/src/utils/__tests__/changelog-generate.test.ts index a7dce6a9..1a3c7b51 100644 --- a/src/utils/__tests__/changelog-generate.test.ts +++ b/src/utils/__tests__/changelog-generate.test.ts @@ -606,5 +606,315 @@ 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 + setup([ + { + hash: 'abc123', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'def456', + title: 'Revert "feat: add new feature"', + body: 'This reverts commit abc123.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + ], 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 + setup([ + { + hash: 'abc123', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'def456', + title: 'Revert "feat: add new feature"', + body: '', // No SHA in body + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + ], 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 + setup([ + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + ], 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 + setup([ + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + { + hash: 'ddd444', + title: 'Revert "Revert "Revert "feat: add feature"""', + body: 'This reverts commit ccc333.', + pr: { local: '4', remote: { number: '4', author: { login: 'dave' } } }, + }, + ], 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 + setup([ + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + }, + { + hash: 'ddd444', + title: 'Revert "Revert "Revert "feat: add feature"""', + body: 'This reverts commit ccc333.', + pr: { local: '4', remote: { number: '4', author: { login: 'dave' } } }, + }, + { + hash: 'eee555', + title: 'Revert "Revert "Revert "Revert "feat: add feature""""', + body: 'This reverts commit ddd444.', + pr: { local: '5', remote: { number: '5', author: { login: 'eve' } } }, + }, + ], 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 + setup([ + { + hash: 'aaa111', + title: 'feat: add feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'bbb222', + title: 'feat: add feature', // Same title as first + body: '', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + { + 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' } } }, + }, + ], 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)" + setup([ + { + hash: 'abc123', + title: 'feat: add feature (#1)', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + hash: 'def456', + title: 'Revert "feat: add feature (#1)" (#2)', + body: 'This reverts commit abc123.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + ], 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 + setup([ + { + hash: 'abc123', + title: 'wip commit message', + body: '', + pr: { + local: '1', + remote: { + number: '1', + author: { login: 'alice' }, + title: 'feat: add feature', // PR title is different + }, + }, + }, + { + hash: 'def456', + title: 'Revert "feat: add feature"', // Matches PR title, not commit title + body: '', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + }, + ], 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 + setup([ + { + hash: 'abc123def456', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + 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' } } }, + }, + ], 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 " + setup([ + { + hash: 'abc123def456', + title: 'feat: add new feature', + body: '', + pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + }, + { + 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.', + }, + }, + }, + ], 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(); + }); + }); +}); diff --git a/src/utils/__tests__/changelog-utils.test.ts b/src/utils/__tests__/changelog-utils.test.ts index 461dc69a..7d7fcd6f 100644 --- a/src/utils/__tests__/changelog-utils.test.ts +++ b/src/utils/__tests__/changelog-utils.test.ts @@ -14,6 +14,10 @@ import { shouldSkipCurrentPR, getBumpTypeForPR, stripTitle, + isRevertCommit, + extractRevertedTitle, + extractRevertedSha, + processReverts, SKIP_CHANGELOG_MAGIC_WORD, BODY_IN_CHANGELOG_MAGIC_WORD, type CurrentPRInfo, @@ -266,3 +270,243 @@ 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', () => { + const commits = [ + commit('abc123', 'feat: add feature'), + commit('def456', 'Revert "feat: add feature"', 'This reverts commit abc123.'), + ]; + const result = processReverts(commits); + expect(result).toEqual([]); + }); + + it('cancels out revert and original via title fallback', () => { + const commits = [ + commit('abc123', 'feat: add feature'), + commit('def456', 'Revert "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'); + }); + + it('handles current PR preview scenario - revert PR cancels existing commit', () => { + // Simulates generateChangelogWithHighlight where current PR has empty hash + // and is a revert of a commit in the existing list. + // Commits are in chronological order (oldest first) matching test conventions. + const commits = [ + // Existing commit in base branch (oldest) + commit('abc123', 'feat: add feature'), + // Current PR (unmerged, no hash) - is a revert (newest) + commit( + '', // empty hash for unmerged PR + 'Revert "feat: add feature"', + 'This reverts commit abc123.', + 'Revert "feat: add feature"', + 'This reverts commit abc123.\n\nReverting due to issues.' + ), + ]; + const result = processReverts(commits); + // Both should cancel out + expect(result).toEqual([]); + }); + + it('handles current PR preview scenario - revert PR uses title matching', () => { + // When PR body doesn't contain SHA, falls back to title matching + const commits = [ + commit('abc123', 'feat: add feature'), + commit( + '', + 'Revert "feat: add feature"', + '', + 'Revert "feat: add feature"', + 'Reverting this PR due to issues.' // No SHA in body + ), + ]; + const result = processReverts(commits); + expect(result).toEqual([]); + }); + + it('handles current PR preview scenario - non-revert PR unaffected', () => { + const commits = [ + commit('abc123', 'fix: bug fix'), + commit( + '', + 'feat: new feature', + '', + 'feat: new feature', + 'Adding a new feature' + ), + ]; + const result = processReverts(commits); + expect(result).toHaveLength(2); + }); + + it('handles double revert in preview scenario', () => { + // Current PR is Revert Revert, should cancel with existing Revert + // Commits in chronological order: A (oldest) -> B (Revert A) -> Current PR (Revert B, newest) + const commits = [ + commit('abc123', 'feat: add feature'), + commit('def456', 'Revert "feat: add feature"', 'This reverts commit abc123.'), + commit( + '', + 'Revert "Revert "feat: add feature""', + 'This reverts commit def456.', + 'Revert "Revert "feat: add feature""', + 'This reverts commit def456.' + ), + ]; + const result = processReverts(commits); + // Current PR cancels def456, leaving abc123 + expect(result).toHaveLength(1); + expect(result[0].hash).toBe('abc123'); + }); +}); diff --git a/src/utils/changelog.ts b/src/utils/changelog.ts index 360319bb..2f68acbf 100644 --- a/src/utils/changelog.ts +++ b/src/utils/changelog.ts @@ -138,6 +138,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 +623,108 @@ 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). Reverts are processed in reverse chronological + * order (newest first) to correctly handle nested reverts like: + * - A: original commit + * - B: Revert A + * - C: Revert B (Revert Revert A) + * + * Processing C 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 chronological order (oldest first) + * @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) { + 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 reverts in reverse chronological order (newest first) + // This ensures nested reverts are handled correctly + const reversedCommits = rawCommits.toReversed(); + + for (const commit of reversedCommits) { + // 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, then prefix match for abbreviated SHAs + revertedCommit = byHash.get(revertedSha); + if (!revertedCommit) { + // Try prefix match for abbreviated SHAs + for (const [hash, c] of byHash) { + if (hash.startsWith(revertedSha) || revertedSha.startsWith(hash)) { + revertedCommit = c; + break; + } + } + } + } + + // Strategy 2: Fallback to title matching + if (!revertedCommit) { + const revertedTitle = extractRevertedTitle(effectiveTitle); + if (revertedTitle) { + revertedCommit = byTitle.get(revertedTitle); + } + } + + // 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 +781,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', }, { @@ -1241,10 +1399,13 @@ export async function generateChangelogWithHighlight( }; const allCommits = [currentPRCommit, ...rawCommits]; - // Step 6: Run categorization on combined list - const { data: rawData, stats } = categorizeCommits(allCommits); + // Step 6: Process reverts - cancel out revert/reverted pairs + const filteredCommits = processReverts(allCommits); + + // Step 7: Run categorization on filtered list + const { data: rawData, stats } = categorizeCommits(filteredCommits); - // Step 7: Serialize to markdown + // Step 8: Serialize to markdown const changelog = await serializeChangelog(rawData, MAX_LEFTOVERS); return { @@ -1439,7 +1600,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); } /** From 30a58aa753135484ba2153b5fa32eaa42b031657 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Sun, 28 Dec 2025 00:53:48 +0300 Subject: [PATCH 2/2] logic fixes --- .../__tests__/changelog-generate.test.ts | 489 +++++++++++++++--- src/utils/__tests__/changelog-utils.test.ts | 46 +- src/utils/changelog.ts | 50 +- 3 files changed, 465 insertions(+), 120 deletions(-) diff --git a/src/utils/__tests__/changelog-generate.test.ts b/src/utils/__tests__/changelog-generate.test.ts index 1a3c7b51..4f2aa77f 100644 --- a/src/utils/__tests__/changelog-generate.test.ts +++ b/src/utils/__tests__/changelog-generate.test.ts @@ -22,7 +22,7 @@ import * as config from '../../config'; import { readFileSync } from 'fs'; import type { SimpleGit } from 'simple-git'; -import { generateChangesetFromGit, clearChangesetCache } from '../changelog'; +import { generateChangesetFromGit, generateChangelogWithHighlight, clearChangesetCache } from '../changelog'; import { type TestCommit } from './fixtures/changelog'; const getConfigFileDirMock = config.getConfigFileDir as jest.MockedFunction; @@ -614,19 +614,20 @@ changelog: 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: 'abc123', - title: 'feat: add new feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, { 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 @@ -636,19 +637,20 @@ changelog: 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: 'abc123', - title: 'feat: add new feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, { 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(''); @@ -675,12 +677,13 @@ changelog: 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: 'aaa111', - title: 'feat: add feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + hash: 'ccc333', + title: 'Revert "Revert "feat: add feature""', + body: 'This reverts commit bbb222.', + pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, }, { hash: 'bbb222', @@ -689,10 +692,10 @@ changelog: pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, }, { - hash: 'ccc333', - title: 'Revert "Revert "feat: add feature""', - body: 'This reverts commit bbb222.', - pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, + 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); @@ -707,18 +710,13 @@ changelog: // 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: 'aaa111', - title: 'feat: add feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, - { - hash: 'bbb222', - title: 'Revert "feat: add feature"', - body: 'This reverts commit aaa111.', - pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + 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', @@ -727,10 +725,16 @@ changelog: pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, }, { - hash: 'ddd444', - title: 'Revert "Revert "Revert "feat: add feature"""', - body: 'This reverts commit ccc333.', - pr: { local: '4', remote: { number: '4', author: { login: 'dave' } } }, + 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); @@ -743,18 +747,19 @@ changelog: // 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: 'aaa111', - title: 'feat: add feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + 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: 'bbb222', - title: 'Revert "feat: add feature"', - body: 'This reverts commit aaa111.', - pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, + 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', @@ -763,16 +768,16 @@ changelog: pr: { local: '3', remote: { number: '3', author: { login: 'charlie' } } }, }, { - hash: 'ddd444', - title: 'Revert "Revert "Revert "feat: add feature"""', - body: 'This reverts commit ccc333.', - pr: { local: '4', remote: { number: '4', author: { login: 'dave' } } }, + hash: 'bbb222', + title: 'Revert "feat: add feature"', + body: 'This reverts commit aaa111.', + pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, }, { - 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: '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); @@ -786,12 +791,13 @@ changelog: 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: 'aaa111', - title: 'feat: add feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, + 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', @@ -800,10 +806,10 @@ changelog: pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, }, { - 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: '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); @@ -815,19 +821,20 @@ changelog: 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: 'abc123', - title: 'feat: add feature (#1)', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, { 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(''); @@ -835,7 +842,14 @@ changelog: 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', @@ -849,12 +863,6 @@ changelog: }, }, }, - { - hash: 'def456', - title: 'Revert "feat: add feature"', // Matches PR title, not commit title - body: '', - pr: { local: '2', remote: { number: '2', author: { login: 'bob' } } }, - }, ], null); const result = await generateChangesetFromGit(dummyGit, '1.0.0', 10); expect(result.changelog).toBe(''); @@ -862,13 +870,8 @@ changelog: 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: 'abc123def456', - title: 'feat: add new feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, { hash: 'def456abc123', title: 'Revert "feat: add new feature"', @@ -880,6 +883,12 @@ 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 @@ -890,13 +899,8 @@ See incident report: https://example.com/incident/123`, 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: 'abc123def456', - title: 'feat: add new feature', - body: '', - pr: { local: '1', remote: { number: '1', author: { login: 'alice' } } }, - }, { hash: 'def456abc123', title: 'fix: undo the new feature due to issues', // Non-standard revert title @@ -910,6 +914,12 @@ See incident report: https://example.com/incident/123`, }, }, }, + { + 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 @@ -918,3 +928,316 @@ See incident report: https://example.com/incident/123`, }); }); }); + +describe('generateChangelogWithHighlight', () => { + let mockClient: jest.Mock; + let mockPullsGet: jest.Mock; + let mockListLabels: jest.Mock; + const mockGetChangesSince = getChangesSince as jest.MockedFunction; + const dummyGit = { + fetch: jest.fn().mockResolvedValue(undefined), + } as unknown as SimpleGit; + + beforeEach(() => { + jest.resetAllMocks(); + clearChangesetCache(); + mockClient = jest.fn(); + mockPullsGet = jest.fn(); + mockListLabels = jest.fn(); + (getGitHubClient as jest.MockedFunction).mockReturnValue({ + graphql: mockClient, + pulls: { get: mockPullsGet }, + issues: { listLabelsOnIssue: mockListLabels }, + } as any); + getConfigFileDirMock.mockReturnValue(undefined); + getGlobalGitHubConfigMock.mockResolvedValue({ + repo: 'test-repo', + owner: 'test-owner', + }); + readFileSyncMock.mockImplementation(() => { + const error: any = new Error('ENOENT'); + error.code = 'ENOENT'; + throw error; + }); + }); + + interface HighlightSetupOptions { + currentPR: { + number: number; + title: string; + body?: string; + author?: string; + labels?: string[]; + baseRef?: string; + headSha?: string; + }; + existingCommits: TestCommit[]; + releaseConfig?: string | null; + } + + function setup(options: HighlightSetupOptions): void { + const { currentPR, existingCommits, releaseConfig } = options; + + // Mock GitHub API for current PR + mockPullsGet.mockResolvedValueOnce({ + data: { + title: currentPR.title, + body: currentPR.body ?? '', + user: { login: currentPR.author ?? 'testuser' }, + base: { ref: currentPR.baseRef ?? 'main' }, + head: { sha: currentPR.headSha ?? 'pr-head-sha' }, + }, + }); + mockListLabels.mockResolvedValueOnce({ + data: (currentPR.labels ?? []).map(name => ({ name })), + }); + + // Mock git changes (existing commits in base branch) + mockGetChangesSince.mockResolvedValueOnce( + existingCommits.map(commit => ({ + hash: commit.hash, + title: commit.title, + body: commit.body, + pr: commit.pr?.local || null, + })) + ); + + // Mock GraphQL for commit info + mockClient.mockResolvedValueOnce({ + repository: Object.fromEntries( + existingCommits.map(({ hash, author, title, pr }: TestCommit) => [ + `C${hash}`, + { + author: { user: author }, + associatedPullRequests: { + nodes: pr?.remote + ? [ + { + author: pr.remote.author, + number: pr.remote.number, + title: pr.remote.title ?? title, + body: pr.remote.body || '', + labels: { + nodes: (pr.remote.labels || []).map(label => ({ + name: label, + })), + }, + }, + ] + : [], + }, + }, + ]) + ), + }); + + 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; + }); + } + } + } + + 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); + + // Both should cancel out + expect(result.changelog).toBe(''); + expect(result.bumpType).toBeNull(); + expect(result.prSkipped).toBe(false); + }); + + 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', + }, + 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('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'); + }); + }); + + 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); + + expect(result.changelog).toBe(''); + expect(result.prSkipped).toBe(true); + // Even skipped PRs contribute to version bump based on title + expect(result.bumpType).toBe('minor'); + }); + + 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: [], + }); + + 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 7d7fcd6f..514b1bce 100644 --- a/src/utils/__tests__/changelog-utils.test.ts +++ b/src/utils/__tests__/changelog-utils.test.ts @@ -90,6 +90,7 @@ describe('shouldSkipCurrentPR', () => { author: 'alice', labels: [], baseRef: 'main', + headSha: 'abc123', }; it('returns false when PR has no skip magic word', () => { @@ -112,6 +113,7 @@ describe('getBumpTypeForPR', () => { author: 'alice', labels: [], baseRef: 'main', + headSha: 'abc123', }; it('returns major for breaking changes', () => { @@ -412,18 +414,20 @@ describe('processReverts', () => { }); it('cancels out revert and original via SHA', () => { + // Commits in newest-first order (git log order) const commits = [ - commit('abc123', 'feat: add feature'), 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('abc123', 'feat: add feature'), commit('def456', 'Revert "feat: add feature"'), + commit('abc123', 'feat: add feature'), ]; const result = processReverts(commits); expect(result).toEqual([]); @@ -439,20 +443,19 @@ describe('processReverts', () => { }); it('handles current PR preview scenario - revert PR cancels existing commit', () => { - // Simulates generateChangelogWithHighlight where current PR has empty hash - // and is a revert of a commit in the existing list. - // Commits are in chronological order (oldest first) matching test conventions. + // Simulates generateChangelogWithHighlight where current PR is prepended (newest). + // Commits in newest-first order (git log order, current PR first). const commits = [ - // Existing commit in base branch (oldest) - commit('abc123', 'feat: add feature'), - // Current PR (unmerged, no hash) - is a revert (newest) + // Current PR (newest, using headSha) commit( - '', // empty hash for unmerged PR + 'pr-head-sha', 'Revert "feat: add feature"', 'This reverts commit abc123.', 'Revert "feat: add feature"', 'This reverts commit abc123.\n\nReverting due to issues.' ), + // Existing commit in base branch (older) + commit('abc123', 'feat: add feature'), ]; const result = processReverts(commits); // Both should cancel out @@ -460,49 +463,54 @@ describe('processReverts', () => { }); it('handles current PR preview scenario - revert PR uses title matching', () => { - // When PR body doesn't contain SHA, falls back to title matching + // When PR body doesn't contain SHA, falls back to title matching. + // Commits in newest-first order. const commits = [ - commit('abc123', 'feat: add feature'), commit( - '', + 'pr-head-sha', 'Revert "feat: add feature"', '', 'Revert "feat: add feature"', 'Reverting this PR due to issues.' // No SHA in body ), + commit('abc123', 'feat: add feature'), ]; const result = processReverts(commits); expect(result).toEqual([]); }); it('handles current PR preview scenario - non-revert PR unaffected', () => { + // Commits in newest-first order const commits = [ - commit('abc123', 'fix: bug fix'), commit( - '', + 'pr-head-sha', 'feat: new feature', '', 'feat: new feature', 'Adding a new feature' ), + commit('abc123', 'fix: bug fix'), ]; const result = processReverts(commits); expect(result).toHaveLength(2); }); it('handles double revert in preview scenario', () => { - // Current PR is Revert Revert, should cancel with existing Revert - // Commits in chronological order: A (oldest) -> B (Revert A) -> Current PR (Revert B, newest) + // Current PR is Revert Revert, should cancel with existing Revert. + // Commits in newest-first order: Current PR (newest) -> B (Revert A) -> A (original, oldest) const commits = [ - commit('abc123', 'feat: add feature'), - commit('def456', 'Revert "feat: add feature"', 'This reverts commit abc123.'), + // Current PR - Revert Revert (newest) commit( - '', + 'pr-head-sha', 'Revert "Revert "feat: add feature""', 'This reverts commit def456.', 'Revert "Revert "feat: add feature""', 'This reverts commit def456.' ), + // Revert A + commit('def456', 'Revert "feat: add feature"', 'This reverts commit abc123.'), + // Original commit A (oldest) + commit('abc123', 'feat: add feature'), ]; const result = processReverts(commits); // Current PR cancels def456, leaving abc123 diff --git a/src/utils/changelog.ts b/src/utils/changelog.ts index 2f68acbf..7f50cdfb 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, }; } @@ -627,19 +630,20 @@ interface RawCommitInfo { * 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). Reverts are processed in reverse chronological - * order (newest first) to correctly handle nested reverts like: - * - A: original commit + * (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) + * - C: Revert B (Revert Revert A, newest) * - * Processing C first: C cancels B, leaving A in the changelog. + * 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 chronological order (oldest first) + * @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[] { @@ -658,7 +662,9 @@ export function processReverts(rawCommits: RawCommitInfo[]): RawCommitInfo[] { const byTitle = new Map(); for (const commit of rawCommits) { - byHash.set(commit.hash, commit); + 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)) { @@ -666,11 +672,10 @@ export function processReverts(rawCommits: RawCommitInfo[]): RawCommitInfo[] { } } - // Process reverts in reverse chronological order (newest first) - // This ensures nested reverts are handled correctly - const reversedCommits = rawCommits.toReversed(); - - for (const commit of reversedCommits) { + // 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; @@ -689,12 +694,16 @@ export function processReverts(rawCommits: RawCommitInfo[]): RawCommitInfo[] { // Strategy 1: Match by SHA from commit body const revertedSha = extractRevertedSha(effectiveBody); if (revertedSha) { - // Try exact match first, then prefix match for abbreviated SHAs + // Try exact match first revertedCommit = byHash.get(revertedSha); - if (!revertedCommit) { + if (!revertedCommit || !remaining.has(revertedCommit)) { // Try prefix match for abbreviated SHAs + revertedCommit = undefined; for (const [hash, c] of byHash) { - if (hash.startsWith(revertedSha) || revertedSha.startsWith(hash)) { + if ( + remaining.has(c) && + (hash.startsWith(revertedSha) || revertedSha.startsWith(hash)) + ) { revertedCommit = c; break; } @@ -703,10 +712,13 @@ export function processReverts(rawCommits: RawCommitInfo[]): RawCommitInfo[] { } // Strategy 2: Fallback to title matching - if (!revertedCommit) { + if (!revertedCommit || !remaining.has(revertedCommit)) { const revertedTitle = extractRevertedTitle(effectiveTitle); if (revertedTitle) { - revertedCommit = byTitle.get(revertedTitle); + const candidate = byTitle.get(revertedTitle); + if (candidate && remaining.has(candidate)) { + revertedCommit = candidate; + } } } @@ -1387,7 +1399,7 @@ export async function generateChangelogWithHighlight( // Step 5: 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, @@ -1397,6 +1409,8 @@ 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 6: Process reverts - cancel out revert/reverted pairs