fix(INFRA-3081): duplicate auto-changelog entries #251
Conversation
…otency - Modified filter to require PR numbers for changelog inclusion - Added comprehensive test coverage - Prevents duplicate changelog entries on repeated workflow runs
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the filtering logic for commits to exclude commits without PR numbers from the changelog generation. The key change converts the filter condition from including commits with missing or unlogged PR numbers (!prNumber || !loggedPrNumbers.includes(prNumber)) to only including commits that have a PR number and are not already logged (prNumber && !loggedPrNumbers.includes(prNumber)).
- Changed filter logic to exclude commits without PR numbers from changelog entries
- Added comprehensive test suite covering the new filtering behavior and edge cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/get-new-changes.ts | Changed filter condition to require PR numbers and exclude direct commits |
| src/get-new-changes.test.ts | Added comprehensive test suite covering filtering, idempotency, and real-world scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The filter at line 212 guarantees that prNumber is truthy, making the subsequent if (prNumber) check redundant. This simplifies the code while maintaining the same behavior.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add explicit type annotation to fix TS2775 error in CI: 'Assertions require every name in the call target to be declared with an explicit type annotation.' This ensures TypeScript can properly validate the assert.ok call in strict mode.
Replace assert.ok with a simple if-throw pattern to avoid TypeScript TS2775 error about assertion signatures requiring explicit type annotations. This is more straightforward and eliminates the need for the assert import. The behavior is identical - throws an error if subject is empty.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add type assertion for prNumber to fix template literal type errors - Replace short variable name 'r' with 'item' in test file - Auto-fix formatting issues (import order, line breaks, trailing spaces) All linting errors are now resolved.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TypeScript can infer the string type from runCommand's return type, making the explicit annotation unnecessary. This addresses Copilot's code review feedback for cleaner code. Note: Cannot use non-null assertion (prNumber!) as suggested by Copilot because @typescript-eslint/no-non-null-assertion rule forbids it. The type assertion approach (as string) is the correct solution for this codebase's linting rules.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Qbandev
changed the title
Qbandev
changed the title
There was a problem hiding this comment.
There seem to be multiple kinds of changes in this PR. If the goal is to remove duplicate changelog entries then that's fine, but removing commits without PR numbers doesn't seem like the best solution to that problem.
Have you considered checking for the existence of the entire entry in the existing changelog instead? So if we're about to copy an entry into the changelog and it has a PR number, we don't add the entry if one already exists with the same PR number. But if the entry we're copying doesn't have a PR number, we don't add the entry if one already exists with the whole message.
Second, I don't think we should exclude commit messages without PR numbers by default. This would essentially break how we typically use auto-changelog in library repos. Just because a commit message doesn't have a PR number doesn't mean it's ineligible for inclusion in the changelog. Basically the release author needs to review this entry in the same way as every other entry. If for some reason we still really want this then can we add an option?
| plugins: | ||
| - path: .yarn/plugins/@yarnpkg/plugin-allow-scripts.cjs | ||
| spec: "https://raw.githubusercontent.com/LavaMoat/LavaMoat/main/packages/yarn-plugin-allow-scripts/bundles/@yarnpkg/plugin-allow-scripts.js" | ||
| spec: 'https://raw.githubusercontent.com/LavaMoat/LavaMoat/main/packages/yarn-plugin-allow-scripts/bundles/@yarnpkg/plugin-allow-scripts.js' |
There was a problem hiding this comment.
I agree with Copilot here, I do not see why this is necessary. Can you revert?
@mcmire Thanks for the review you’re right. I was targeting INFRA-3081, where direct commits like “Update Attributions” caused duplicate entries because idempotency relied on PR numbers. In doing so, I conflated the duplication issue with the filtering strategy and introduced a breaking change by excluding all non‑PR commits. Here’s a another path:
@gauthierpetetin can you confirm this aligns with INFRA‑3081’s expectations? Is step 2 required for your use case? I’ll rework the PR accordingly or open a new one. |
|
Hi @Qbandev @mcmire , In our use case of release changelog generation, the problem, rather than duplicate changelog entries, is the presence of commits without PRs, as they're always irrelevant (e.g. “Update Attributions”) for end users. Implementing suggestion 2., i.e. introducing a dedicated flag like --require-pr-numbers, sounds good to me! |
|
The current PR mixes concerns and already diverged; refactoring it further will bloat the diff and complicate review/rollback.
|
…iltering (#253) ## What is the current state of things and why does it need to change? Currently, `auto-changelog` includes all commits in the changelog, even direct commits without PR numbers. For projects following strict PR-based workflows (like MetaMask Extension), these direct commits should be excluded to ensure all changelog entries represent reviewed changes. ## What is the solution your changes offer and how does it work? ### 1. New `--requirePrNumbers` Flag Adds an opt-in CLI flag that filters out commits without PR numbers at generation time. Defaults to `false` for backward compatibility. **Files changed:** `cli.ts`, `update-changelog.ts`, `get-new-changes.ts` ### 2. Graceful 404 Handling for Forked Repos When running on forked repositories, the tool fetches PR details from the fork. PRs originating from the upstream repo return 404 errors. Added `try-catch` to return empty labels instead of crashing. ### 3. HTML Comment Stripping PR templates often contain HTML comments (`<!-- ... -->`) that were breaking markdown rendering in changelogs. Added `stripHtmlComments()` function to clean descriptions. ### 4. `CHANGELOG entry: null` Fix PRs marked with `CHANGELOG entry: null` were appearing as "Null (#PR)" due to: - Trailing whitespace not being trimmed - Case-sensitive comparison (only matched lowercase `null`) Fixed by adding `.trim()` and case-insensitive comparison at both check points. > **Note:** Fixes 2-4 were discovered during integration testing and are prerequisites for the `--requirePrNumbers` flag to work correctly in production environments with forked repos. ## Usage ```bash yarn auto-changelog update --requirePrNumbers --autoCategorize --useChangelogEntry --useShortPrLink --rc ``` ## Testing Tested with [consensys-test/metamask-extension-test](https://github.com/consensys-test/metamask-extension-test) on `release/1100.0.0`: | Test | Result | |------|--------| | PR Commit `feat: test commit with PR for 1100.0.0 (#40)` | ✅ **INCLUDED** | | Direct Commit `chore: direct commit without PR for 1100.0.0` | ✅ **EXCLUDED** | | `Null (#PR)` entries | ✅ **NONE** (case-insensitive fix working) | | [Generated Changelog](https://github.com/consensys-test/metamask-extension-test/blob/release/1100.0.0-Changelog/CHANGELOG.md) | ✅ Correct | ### Changelog Excerpt ```markdown ## [1100.0.0] ### Added - feat: test commit with PR for 1100.0.0 (#40) - Design upgrades for Confirmations (#38399) - Fix amount component to be only accept numeric inputs (#38235) ... ``` **Note:** `N/A` and `None` entries still appear (e.g., `N/A (#37810)`) because these are different strings from `null` - they're valid descriptions that PRs chose to use. ## References - Complements duplicate detection work: #251
Problem Statement
The auto-changelog tool in MetaMask Mobile CI was creating duplicate changelog entries when running multiple times on the same release branch. Direct commits without PR numbers (e.g., "Update Attributions", "Bump version to X.Y.Z") were being included and re-added on each workflow run.
Why this matters:
Solution
Implements a strict filter that completely excludes commits without PR numbers from changelog generation.
Key Changes
1. Filtered Commit Processing (
src/get-new-changes.ts):!prNumber || !loggedPrNumbers.includes(prNumber)prNumber && !loggedPrNumbers.includes(prNumber)2. Code Cleanup:
if (prNumber)check in map function (guaranteed by filter)as string) for TypeScript template literal requirements@typescript-eslint/no-non-null-assertionrule3. TypeScript Compatibility:
assert.ok()with explicitif (!subject) throw Error()patternassertimportCorrectness Guarantees
Test Validation ✅
Test Repository: consensys-test/metamask-extension-test
Test Branch:
release/99.0.0Test Commits:
feat: Add notification system (#4)- PR merge (has PR number)chore: Update Attributions- Direct commit (no PR number)fix: Emergency hotfix for critical bug- Direct commit (no PR number)Generated Changelog Result:
Verification:
Workflow: Using
github-tools@chore/test-auto-changelogwith this branchfix/INFRA-3081-filter-commitsRelated Issues
Fixes INFRA-3081