Skip to content

ci: run update-changelogs on comment for non-release PRs #10077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cryptodev-2s merged 2 commits into from
Sep 2, 2026
+16 −2
Merged

ci: run update-changelogs on comment for non-release PRs #10077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17aea1a
ci: run update-changelogs on comment for non-release PRs
cryptodev-2s Sep 2, 2026
d1d8705
Apply review suggestions
cryptodev-2s Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/update-changelogs.yml
View file
Open in desktop

@FrederikBolding FrederikBolding Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note: Should we consider running this on draft releases too? It seems a bit confusing you have to bring it out of draft to get the bot to run automatically

@cryptodev-2s cryptodev-2s Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only do this for non-draft PRs. We assume CI could still be failing, or the person may not have prepared the release correctly or fixed all the constraints (for using the correct versions), so the workflow needs to be safe to run more than once. (assuming people are correctly using the draft state)

@mcmire mcmire Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I can see both sides on this and don't have a strong opinion. That said even if more people are likely to post a comment than wait for the workflow to run then it would act similar to the "update LavaMoat policies" workflow in Extension, which is an established pattern. So maybe the way you have it is okay?

Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ jobs:
react-to-comment:
name: React to the comment
needs: is-release
if: needs.is-release.outputs.is-release == 'true' && github.event_name == 'issue_comment'
if: github.event_name == 'issue_comment'
runs-on: ubuntu-latest
environment: default-branch
permissions:
Expand Down Expand Up @@ -123,7 +123,7 @@ jobs:
update-changelogs:
name: Update changelogs
needs: is-release
if: ${{ needs.is-release.outputs.is-release == 'true' }}
if: needs.is-release.outputs.is-release == 'true' || github.event_name == 'issue_comment'

@cursor cursor Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing commenter write-access gate

High Severity

Commenting @metamaskbot update-changelogs now reaches update-changelogs, which mints a write token and pushes to the PR branch, but is-fork still accepts any commenter. On this public repository, anyone can start privileged writes on a non-fork PR.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 17aea1a. Configure here.

@cryptodev-2s cryptodev-2s Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre existing

@FrederikBolding FrederikBolding Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems correct, we would need to bail out on forks. Which we aren't doing anymore if we allow it on non-releases.

@cryptodev-2s cryptodev-2s Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is pre-existing behavior. It means that on any PR opened from within the repository, a comment from any person could trigger the workflow. Nothing changes here.

The existing fork check only verifies that the workflow is not running from a forked repository. It does not check whether the person commenting on a non-fork PR is a legitimate/authorized user.

@cryptodev-2s cryptodev-2s Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean even previously when it was scoped to release PRs only. Anyone could have triggered this by a comment.

On the bot's actual point, it's about who typed the comment, not about forks.

@mcmire mcmire Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow update-changelogs to run automatically on non-release PRs too? Let's say someone bumps a dependency in a feature branch. If we are encouraging engineers to update the changelog in advance instead of within release PRs, then it seems that we would want the dependency to be logged in the feature branch and not skipped.

@cryptodev-2s cryptodev-2s Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that too. However, doing this would cause many runs to consume compute credits unnecessarily, since the likelihood of a PR actually needing changelog updates for package bumps is relatively low.

We’re still planning to automatically plug this workflow into Dependabot PRs, for example. So I believe keeping it this way is still the better option.

@mcmire mcmire Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, alright.

runs-on: ubuntu-latest
environment: default-branch
permissions:
Expand Down
14 changes: 14 additions & 0 deletions docs/processes/updating-changelogs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,17 @@ We will offer more guidance here in the future, but in general:
- Combine like changes from multiple pull requests into a single changelog entry if necessary.
- Split disparate changes from the same pull request into multiple entries if necessary.
- Omit reverted changes from the changelog.

## Generating changelog entries for dependency bumps

Recording dependency bumps within package changelogs can be tedious. You can have these entries generated for you instead of writing them by hand:

1. Post a comment on your pull request with the text `@metamaskbot update-changelogs`.
2. The `Update Changelogs` GitHub action reacts to your comment with a 👍 and kicks off.
3. After a few minutes you will see a new comment saying either that the changelogs were updated and pushed to your branch, or that no changes were needed. If validation errors remain that the action cannot fix, the comment links to the workflow run so you can see them.

A few things to know:

- This works on any pull request, not just release pull requests. Release pull requests also get this automatically when they are opened.
- If the action pushes a commit to your branch, remember to pull it locally before you push again.
- This only works on pull requests opened from a branch in this repo. Pull requests from forks are skipped, so if you are an outside contributor you will need to update changelogs by hand.