Announcing nixpkgs-merge-bot

srid · October 17, 2023, 2:32pm

Excellent. At times I’ve had to wait 4+ weeks to get a version change to my own package merged. This will help with that.

Also TIL https://github.com/NixOS/rfcs/pull/140 (so I’ll create new packages over there going forward)

Infinisil · October 17, 2023, 2:39pm

I agree with @qyliss, I think it’s reckless to introduce this without any prior discussion with the community.

I’ve been working on migrating essentially all packages to pkgs/by-name, but this seriously makes me reconsider that, because I’d be contributing to decreasing Nixpkgs security.

RaitoBezarius · October 17, 2023, 2:40pm

Though it creates the incentive to move your package to by-name to get faster merges to be honest.
Which creates an interesting situation which I think @Infinisil should have in mind w.r.t. to their goals with RFC140. Because as I understand it, we didn’t decide it to do the treewide move to by-name and there are probably good reasons to wait before doing it too fast.

zimbatm · October 17, 2023, 2:41pm

This is just the start by the way.

As we learn from this, we’ll be able to make the bot more useful and then eventually remove commit access to the 200 people. It will make the process more democratic and self-service

I think the concerns that are brought up are reasonable, and they also apply against the existing committers. Part of the answer is to add more tooling and automate.

Another thing that I would love to see happen is the appearance of a “core” package set with different rules and guarantees.

qyliss · October 17, 2023, 2:43pm

FWIW: it’s not actually being introduced without prior discussion. That was my initial impression too, because the original post implies that the bot is already running, but since it turns out that it isn’t, this is the prior discussion.

So what we should be doing here is identifying what the concerns are, and then having a productive discussion how we address those concerns and get to the end point where maintainers do have more power over the packages they maintain. @NickCao has already made some good suggestions, as has @RaitoBezarius in #dev:nixos.org

Infinisil · October 17, 2023, 2:43pm

I think there’s a way to have a merge bot without security issues, i described this here: More granular commit access control via merge bot - #8 by Infinisil

@RaitoBezarius The migration to pkgs/by-name was in the RFC, I didn’t plan to hold off of it originally.

fgaz · October 17, 2023, 2:47pm

I think the set of affected packages/maintainers should be chosen manually instead of using by-name. As written by others, using by-name creates unwanted incentives.

Moreover, if a few maintainers (instead of packages) are chosen, gathering feedback (edit: and establishing trust) will be easier.

pbsds · October 17, 2023, 2:53pm

While this is sorely wanted by many non-comitters, I’d say caution is essential.
I took some time to write a couple of additional checks, not all of which are in my eyes required, but are worth discussing:

For clarity, a glossary:

invoker: the one posting @nixpkgs-merge-bot merge
maintainer: anyone in maintainers.nix

CI must pass
max rebuilds: 10
- higher limits could be earned on a per-invoker basis
no self merge
rate limiting: max x invocations per invoker per day
- higher limits could be earned on a per-invoker basis
invoker must be a maintainer of at least X packages / nixos modules (query packages.json)
PR must be reviewed by at least one other maintainer than the one invoking the bot
PR must target staging or staging-next, not master
the merge bot waits 24 hours before merging, and can be canceled if anyone reacts with thumb down / posts some invocation like @nixpkgs-merge-bot object
Invocations of the bot when prior checks fail result in a timeout / penalty for the invoker
bot runs nixpkgs-review right before merging, which must succeed

Perhaps even a trial run where the comitters cobble together a list of say 20-500 maintainers they think should get try out the bot.

ElvishJerricco · October 17, 2023, 3:18pm

I’ve not really been involved in any RFCs, but even if there have been too many things that should not have been RFCs, that doesn’t mean that this is one of those things. If there’s anything that really deserves an RFC, policy and automation around security critical subjects seems like it.

I would suggest that experimentation be done with a dormant bot that cannot actually perform meaningful actions, but will demonstrate what it would do. An RFC can lay the groundwork for the core functionality, and that can evolve while we experiment with the dormant bot, before the RFC is finally accepted. Then, the bot can be awakened, and we can further experiment with further dormant features. Rinse and repeat as new ideas come to bear.

Infinisil · October 17, 2023, 3:30pm

A long discussion is also taking place on Matrix

RaitoBezarius · October 17, 2023, 3:31pm

Except for Darwin tests IMHO, but that’s a detail.

Not sure why should have such a low limit, how do we decide arbitrarily that number? I feel like anything that is staging zone should just be blocked until human intervention.

This is a meaningless limitation IMHO, you can always create another GitHub account to do this type of things.

What would be the purpose or the aim?

That seems to encourage to game that metric and people who have high numbers are not necessarily always the best person to be that invoker IME (no offense).

Sounds reasonable except when such maintainer does not exist.

I don’t understand why, that seems limiting the bot to staging workflow.

I would make the delay even longer for certain set of packages. Like a systemd merge maybe should go on for 1 week or more.

Unclear what purpose does it serve, I would not introduce it without a clear idea of what does it bring to the table.

That’s kinda OfBorg responsibility, it seems excessive.

Here’s another thing I would like the mergebot to do:

choose reviewers based on the files touched, the code owners, etc.
assign 3/4 reviewers among this list, wait for their interaction with a timeout of N weeks
choose another set of reviewers and escalate it until you find like a group of people who interacts

Infinisil · October 17, 2023, 3:40pm

I don’t think we need to discuss these exact requirements here. The main thing I want to see is confirmation that this bot isn’t being deployed without first going through either the RFC process or something resembling it closely.

aanderse · October 17, 2023, 3:49pm

We should split that out into its own thread as I think that is a great topic worth discussion on its own at some point.

pbsds · October 17, 2023, 4:02pm

This is rather a reduction in scope of the merge-bot. PRs that affect more should be subjected to proper review by a committer IMO.

Any automated merge bot would be susceptible to multiple accounts / a circle of friends cooperating, which should be a part of the discussion of attack vectors.

Two reasons: (1) to avoid abuse (someone could write a bot to post invocations on all PRs), and (2) to encourage reviewers to not get burned out. The incentives given to reviewers should be to produce a steady flow of reviews. A different system achieving the same effect is much appreaciated.

True, let’s not axe the committers quite yet.

Maybe my view of staging is lacking. I want to give committers a chance to revert bot-merged PRs before they hit master and get tagged in a release. A separate branch could do the trick.

Committers can still shortcut this wait time. I also central packages like systemd result in more than 10 rebuilds, making the bot stop at my second proposed check with a max rebuild limit. For simple version bump Prs and the like i think such a check is quite reasonable.

This is a crude way to incentivize invokers to be careful, and to disincentivize drive-by invokation. A better system is much appreciated

It is excessive, i believe the benefit is worth it. It could be conditioned on the ofborg build being older than x days

Great critique. These discussions really highlight the need for a RFC, and is better served on a site which supports tree-based discussion and voting

vcunat · October 17, 2023, 4:05pm

Right now:

4.8k PRs open
46 of them approved by a maintainer

I’d first work on raising this 1% higher. No merge-bot or policy is needed for that (or down-sides like security questions).

I’ll much easier merge a PR if it’s approved by a .meta.maintainer. And anyone with merge privileges can use the above link and merge, based on their (human) judgement.

Some people keep thinking that their feedback is near-useless if they don’t have merge privileges, but I strongly oppose that.

Security principles: IMO the issue is that we’ve been handing out maintainership for many years assuming that it gives no permissions whatsoever. Now it would suddenly allow self-merging without anyone getting notified?

BTW, note that the label gets applied even in PRs where the person makes themselves a maintainer. A simple illustration how easily things can go wrong.

JulienMalka · October 17, 2023, 4:13pm

Just as a precision, the merge-bot seems to check for maintainership against master:

github.com

NixOS/nixpkgs-merge-bot/blob/02120fae0a5dc0005dfc5884c5ebbf98962c3c6f/nixpkgs_merge_bot/nix.py#L43


      
                  check=True,
                  stdin=subprocess.PIPE,
                  stdout=subprocess.PIPE,
              )
              return proc.stdout
          
          

          
def get_package_maintainers(path: Path) -> list[Maintainer]:
              package_name = path.parts[3]
              # TODO maybe we want to check the merge target remote here?
              proc = nix_eval(f"github:nixos/nixpkgs/master#{package_name}.meta.maintainers")
              maintainers = json.loads(proc.decode("utf-8"))
              return [
                  Maintainer(maintainer["githubId"], maintainer["github"])
                  for maintainer in maintainers
              ]
          
          

          
def is_maintainer(github_id: int, maintainers: list[Maintainer]) -> bool:
              for m in maintainers:
                  if m.github_id == github_id:

tomberek · October 17, 2023, 4:28pm

A merge-bot does have impacts. Let’s consider them by category:

short-term: day-to-day Nixpkgs operations. The people most likely who would be either harmed or benefited should comment about this. So if this starts to go haywire, who is likely to be on hand to fix it? Who are the people whose lives are made easier and stand to gain provide their thoughts.
long-term: the security model of Nixpkgs is impacted. But it is not quite clear if it is positive or negative.
active-contributors and committers: specific kinds of normal awareness is hidden by automation. The proposal distributes this and makes it possible to have fewer committers, but more people with a limited form of commit access.
general users: Will end-users benefit? What harm can come to them? Can this harm be mitigated in other ways?

I can’t honestly determine if everything ends up with a net benefit or not, but if we can proceed with a kind of risk that can be mitigate or dealt with, (and the people who will have to deal with it are okay with it) it might end up being quite productive.

There are a few proposals for mitigating the risk, let’s try to clarify them and see if they make the risks acceptable enough to proceed with an experiment.

arianvp · October 17, 2023, 5:16pm

We only use staging to batch mass-rebuilds together. Anything that doesn’t trigger a mass-rebuild goes to master. (i.e. most leaf-packages). Once something lands on master it takes a while for it to trickle down into a release due to hydra having to build the commit. So a commit can be reverted before it hits a release.

lassulus · October 17, 2023, 6:36pm

we moved the discussion to matrix https://matrix.to/#/#nixpkgs-merge-bot:lassul.us
just come by if you want to contribute, we can add some more checks before merging and make it more strict down in the beginning until most people are happy (like checking for eval, ofborg, etc.).

vcunat · October 17, 2023, 6:47pm

You can’t rely on that, unless the package is in the channel-blocking set. Because even your fix will wait, just as the breakage, and channel update might just as well happen in between.