workflows/labels: manage approval labels

[wolfgangwalther]

This enables the labels workflow to manage the 12.approvals: ? and 12.approved-by: package-maintainer labels. Two challenges had to be solved for that:

• The pull_request_review events run without permissions. We noticed this in workflows/dismissed-review: drop #413256 already and I summarized it in Automate approval badges update through GitHub action #307917 (comment) as well.
• We need eval results (package maintainers) to match them with the approved reviews.

The missing permissions are solved by running the labels workflow on workflow_run - which triggers on completion of a no-op pull_request_review workflow. This also allows us to bring back #413256, so did that in the first commit, verifying the approach.

To get the eval results, the label workflow is now run after Eval as a reusable workflow. This means labeling will happen with a delay, compared to the status-quo, but I don't see this as a problem. The labels workflow is triggered after Eval and after any approval or dismissal of reviews.

Currently, the approach to count approvals is based on pure number of approved reviews, without taking "push after review" as invalidation into account. I think the status-quo, automated (?) by @wegank, is different though. I assume we'll want that behavior and we should implement it as a branch protection rule, which reads:

Dismiss stale pull request approvals when new commits are pushed
New, reviewable commits pushed will dismiss previous pull request review approvals.

This will also give us better feedback in GitHub's UI than right now, where we remove the approvals label, but still see the approvals in the reviewers list. The branch protection rule should dismiss them entirely.

Also to consider: How is 12.approved-by: package-maintainer to be defined? We defined 11.by: package-maintainer as "the PR author must be maintainer of all touched packages". I don't think this definition makes sense for the approval label (and doesn't seem to be used like this in practice right now). So I went with "at least one reviewer needs to maintain at least one of the touched packages" to add this label.

Resolves #307917.

Things done

It's hard to test it in my own fork, because I can't approve my own PRs. I did some basic testing with approvals created by CI, though - this should work. I did not test the different scenarios of "approving while eval is still running", "approval after eval ran", "dismissal" etc. - but they all just trigger the same workflow without any special conditions. So I'm fairly confident it should work.

• Basic testing in my fork.
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[MattSturgeon]

I think the status-quo, automated (?) by @wegank, is different though.

I believe the current automation has been inconsistent with whether or not it counts stale reviews.

I don't think we need to get too hung up on maintaining the exact status quo of an unofficial labeling bot 🙂

I assume we'll want that behavior and we should implement it as a branch protection rule

Note there are actually two very similar protection/ruleset rules:

• Dismiss stale pull request approvals when new commits are pushed
  New, reviewable commits pushed will dismiss previous pull request review approvals.

• Require approval of the most recent reviewable push
  Whether the most recent reviewable push must be approved by someone other than the person who pushed it.

These are related but subtly different; one relates to whether "old" approvals still count, the other relates to "who" can sign off the latest code.

---

Personally, I find the label more useful when it includes all approvals, even if they didn't approve the very latest code.

I'd be in favour of this PR counting all approvals; outdated or not, and by committers or not. IIUC that is how the PR behaves currently?

I think follow-up PRs/issues could discuss whether we want to add another label for "up-to-date approvals" and/or formalise what exactly counts as an "approval" for the existing label. Whether and what branch protection/rulesets we want should also be a separate discussion.

How is 12.approved-by: package-maintainer to be defined? [...] I went with "at least one reviewer needs to maintain at least one of the touched packages" to add this label.

SGTM

[wolfgangwalther]

I don't think we need to get too hung up on maintaining the exact status quo of an unofficial labeling bot 🙂

Not necessarily, no - but it was the only data point I found, so far, about which behavior to prefer.

These are related but subtly different; one relates to whether "old" approvals still count, the other relates to "who" can sign off the latest code.

Yeah, I'm not suggesting to require approvals before merge, so the second option doesn't apply. This would be another different discussion.

I'd be in favour of this PR counting all approvals; outdated or not, and by committers or not. IIUC that is how the PR behaves currently?

Correct.

I think follow-up PRs/issues could discuss whether we want to add another label for "up-to-date approvals" and/or formalise what exactly counts as an "approval" for the existing label. Whether and what branch protection/rulesets we want should also be a separate discussion.

Fine by me, we'd just make sure that this automation doesn't conflict with other automation approaches, flipping labels back and forth. So those would need to be disabled.

[philiptaron]

It's hard to test it in my own fork, because I can't approve my own PRs.

You mean that https://github.com/wolfgangbot/ isn't yet under your control?! 🫢

[philiptaron]

A few curious questions.

[wolfgangwalther]

Also added the default shell bash introduced in #415680 to the new workflows.

[wolfgangwalther]

Makes me smile on this PR :)

But a question related to that, @wegank: Was this automated and if yes, what's the rule you're looking at? This would not be labeled approved-by: package-maintainer with this PR's automation, because there are no package rebuilds.

Thus - are you using codeowners as a reference, too?

[wegank]

Thus - are you using codeowners as a reference, too?

Yeah, actually I was abusing the label, as it was added when someone pinged by nix-owners gave their approval...

[wolfgangwalther]

Ah, interesting logic.

I see multiple options here:

• Ignore codeowners for approval labels
• Do the same as previously, and add the by-package-maintainer approval label when any codeowner approves, too
• Add a new 12.approved-by: codeowner label

Any preference, anyone?

[MattSturgeon]

• Add a new 12.approved-by: codeowner label

This is my preference, but I'm happy for it do be done in another PR if that unblocks this one and/or makes it easier to review

[wolfgangwalther]

Yeah, to be able to handle codeowners, the codeowner workflow would first have to be integrated with eval/reviewers/labelling. I have some preliminary work in #411409, but that has stalled a bit with no clear path forward.

[wolfgangwalther]

I think this is as good as it gets for now - we will get the best feedback by just running it. Will merge later today, when I have the time to monitor this in action.

[nixpkgs-ci]

Successfully created backport PR for release-24.11:

• [Backport release-24.11] workflows/labels: manage approval labels #416409

[nixpkgs-ci]

Successfully created backport PR for release-25.05:

• [Backport release-25.05] workflows/labels: manage approval labels #416410

[wolfgangwalther]

So one thing that happens: External or first-time contributors (only saw it for the latter, not sure, yet) can't run those pull_request_review workflows unless someone approves the workflow to run. Example: https://github.com/NixOS/nixpkgs/actions/runs/15632302375.

This only affects the labeling, and only after a review submission. On creation of the PR and force push, the labels (including approvals) will still be added, but not for reviews - until those workflows are approved. This affects all contributors, even committers, posting a review on those PRs - whether this needs approval or not is about the PR creator.

I'm not sure, yet, whether a one-time approval is enough or whether that needs to be repeated after a force push.

Whose PRs are affected depends on this setting in Actions / General:

@infinisil could you clarify what this is set to for nixpkgs?

[wolfgangwalther]

I'm not sure, yet, whether a one-time approval is enough or whether that needs to be repeated after a force push.

This needs to be repeated after each force-push, just tested.

This is annoying, because it means that approved PRs of first-time contributors will regularly not have approval labels set :/

[MattSturgeon]

So one thing that happens: External or first-time contributors (only saw it for the latter, not sure, yet) can't run those pull_request_review workflows unless someone approves the workflow to run. Example: NixOS/nixpkgs/actions/runs/15632302375.

This is annoying, I expect "require approval" is enabled because some workflow events are insecure when run from untrusted forks; the same reason pull_request_target is preferred over pull_request.

I assume the scenario GitHub is protecting us from is a user modifying a pull_request_review workflow on their fork, so that when run it does something unexpected that our non-fork workflow wouldn't?

I believe users could also add entirely new workflows to their fork and run them on PRs to the main repo, if not for these security settings?

Although from a security perspective there's little difference between a fork having additional workflows and a fork having modified workflows.

The details are documented under pull_request_review. They actually explicitly mention:

When a first-time contributor submits a pull request to a public repository, a maintainer with write access may need to approve running workflows on the pull request. For more information, see Approving workflow runs from public forks.

Based on that wording, it sounds like pull_request_target would also not trigger on events in forks, but IIUC that's not correct because the workflow is read from the non-fork repo for _target events?

[wolfgangwalther]

I assume the scenario GitHub is protecting us from is a user modifying a pull_request_review workflow on their fork, so that when run it does something unexpected that our non-fork workflow wouldn't?

I believe users could also add entirely new workflows to their fork and run them on PRs to the main repo, if not for these security settings?

Both correct.

Although from a security perspective there's little difference between a fork having additional workflows and a fork having modified workflows.

I think the difference is in terms of whose resources are used. Anybody could create pull_request workflows in a PR and do bad things like mining crypto or whatever.

Based on that wording, it sounds like pull_request_target would also not trigger on events in forks, but IIUC that's not correct because the workflow is read from the non-fork repo for _target events?

Correct. pull_request is the same as pull_request_review - it would require approval. pull_request_target does not, because it runs "in the context of the base branch".

So yeah, the very same reason why we need to make this whole dance with workflow_run to get a workflow with secrets/permissions to run causes this "require approval" thing.

I see two ways to deal with that here:

• We can either say we're confident with the first option in the screenshot "first-time contributor and new on GitHub" - and then for those that still need their workflows approved manually, we live with that.
• Or we can switch to a periodic workflow approach, where we don't trigger on the review event, but just scan those PRs every now and then. We probably don't need to scan too many PRs every time, because we can sort them by updated, and then paginate through the result until we get to PRs that were updated before we last ran the action.

The latter would also be required if we want to manage the "merge conflict" label via actions as well.

Edit: Of course, a hybrid approach would also work. Still keep the review event, so we get quick labeling for known contributors - but then come back periodically and update PRs for first-time contributors as well. Not sure whether we could get rid of the "Some workflows need approval" button in the PRs, though.

[MattSturgeon]

• Or we can switch to a periodic workflow approach, where we don't trigger on the review event, but just scan those PRs every now and then. We probably don't need to scan too many PRs every time, because we can sort them by updated, and then paginate through the result until we get to PRs that were updated before we last ran the action.

The latter would also be required if we want to manage the "merge conflict" label via actions as well.

This sounds reasonable. It's frustrating that we can't use events to trigger adding labels without running into permission/security issues, but using on: schedule would certainly simplify the implementation. In practice, we could limit the result to all PRs updated since the last scheduled run and maybe do a pass each hour?

Edit: Of course, a hybrid approach would also work. Still keep the review event, so we get quick labeling for known contributors - but then come back periodically and update PRs for first-time contributors as well. Not sure whether we could get rid of the "Some workflows need approval" button in the PRs, though.

This could also work. Maybe start of with a simple on: schedule workflow and consider adding back the pull_request_review triggers once it is working well?

[wolfgangwalther]

using on: schedule would certainly simplify the implementation. In practice, we could limit the result to all PRs updated since the last scheduled run and maybe do a pass each hour?

Yes, I was thinking about "1 hour" as well. This workflow could also implement something like a "one hour self-merge cooldown" mentioned in NixOS/org#122 (comment), which also only works with a scheduled workflow. Plenty of things we can make use of a scheduled workflow for!

This could also work. Maybe start of with a simple on: schedule workflow and consider adding back the pull_request_review triggers once it is working well?

Well, we already have the pull_request_review stuff running, so I'd just keep it that way for now. No need to back it out again, imho, at least not before we got feedback on whether "first-time contribution and new to github" would be an acceptable setting.

I'd still look into setting up a scheduled workflow at the same time.

[philiptaron]

I’m really excited about the scheduled workflow for efficiency reasons as well. Being able to batch these and similar low priority jobs together is absolutely a win for efficiency which lets them do substantially more work across a substantially greater set of PRS then they would otherwise. The fact that it might be required for correctness or availability reasons is just another motivating factor.

[MattSturgeon]

Being able to batch these and similar low priority jobs together is absolutely a win for efficiency which lets them do substantially more work across a substantially greater set of PRS then they would otherwise.

To really benefit from the efficiency gain we'd have to drop the pull_request_review triggers and rely exclusively on schedule.

This may be safer from a security perspective too, since we won't be running workflows from the fork's context.

I don't feel strongly on if we should have a hybrid approach or exclusively use schedule, but the latter does sound simpler to reason about.

[wolfgangwalther]

but the latter does sound simpler to reason about.

That's true and tips the scale for me - the workflow_run stuff is really a bit odd.

[wolfgangwalther]

Also, the workflow_run stuff doesn't seem to be that reliable:

It should have added the label after the approval already, but only did after the force push.