Definitelytyped: Allow "code owners" to merge PRs for individual typings

[SupernaviX]

Github recently added support for a CODEOWNERS file, which allows individual reviewers to be in charge of specific subdirectories in a project. To reduce the management overhead of this giant monorepo and decrease turnaround time on PRs to existing typings, it would be nice if the people in the "definitions by" list for a set of typings could be made "code owners" for them, and be allowed to approve/merge pull requests for those typings themselves.

[isiahmeadows]

This could also be really helpful in keeping me out of having to deal with reviewing PRs for definitions I am only somewhat involved with (make sure it reflects reality), but another person is primarily responsible for.

[RyanCavanaugh]

GitHub doesn't allow CODEOWNERs to merge, unfortunately, and there's no way to change this. https://twitter.com/GitHubHelp/status/884434724713881600

@typescript-bot has the capability to merge so we could potentially allow it to recognize a "Please merge" command from a listed owner (which is data we already have from the headers).

[isiahmeadows]

I'd be game for that (provided it's documented somewhere).

[SupernaviX]

Yeah, that sounds fine to me.

[RyanCavanaugh]

@andy-ms will be working on this area soon (maybe already?). Main focus will be generating the CODEOWNERs file from the header information + augmenting @typescript-bot to accept merge commands from those owners.

We'll still need to iron out the policy a little. Open questions I'd like feedback on:

• Is it acceptable for a PR to merge (assuming CI is ✔️ ) as soon as an owner approves? Or is there a minimum time we should enforce before merging (so that possibly other owners can weigh in)?
• What should be done with changes spanning multiple definitions? Presumably get approval + merge command from at least one author per definition before merging?
• Is the "honor system" for acquiring ownership still OK in a world where owners can cause a merge and subsequent publish to @types? I am concerned about the ease with which someone could insert a malicious definition - I'm not aware of any RCE exploits ever occurring in TypeScript but if someone did find one, this would be an enormous risk, and certainly DT running on a workflow where no "trusted" user would have to review a PR first would increase the incentive for someone to go looking for one. Maybe we only accept merge commands from people who have passed some trust boundary (multiple PRs submitted over some course of months or something)?

/cc @vvakame @Igorbek @nycdotnet @Bartvds @basarat @blakeembrey @Steve-Fenton for input

[Igorbek]

@RyanCavanaugh my thoughts

Is it acceptable for a PR to merge (assuming CI is ✔️ ) as soon as an owner approves?
I would prefer rules where

• approving by 1 owner does not allow merging
  
  
  
  • approved PRs can be merged after 1 (maybe 2?) days after approval
  
  
• approving by 2 or more owners
  
  
  
  • allows to merge after 1 (or 2) hours
  
  
• the team still can merge right away :)

What should be done with changes spanning multiple definitions? Presumably get approval + merge command from at least one author per definition before merging?

At least 1 owner per definition need to approve which falls to "2 or more owners" case

Is the "honor system" for acquiring ownership still OK in a world where owners can cause a merge and subsequent publish to @types?

I think it's been working pretty well so far, but the concern is fair. I'd rather wait till we really have an accident :) than make it too restrictive and slow.

[nycdotnet]

Sorry for the delayed reply - I've been out of town.

I like the idea of updating typescript-bot to support this.

My comments below all assume a reasonably high level of trust of code owners - see Honor System comment below for how I define a valid owner.

• Regarding merging upon ✔️ CI and single valid owner approval: IMO fine to merge right away if single valid code owner and not key package (see below). If multiple valid owners, would like to see minimum 4 days to auto-merge after first valid owner approval (allows for long weekends and 24 hour response time), 10 days feels right to me, counting from the time of the first valid owner approval.
• Regarding merging upon ✔️ CI and multiple valid owner approval: merge right away.
• Regarding merging upon ✔️ CI and any MS team approval: merge right away.
• Regarding multiple-definition and ✔️ CI: I agree with the idea to get a valid owner from each definition, likely using the rules above.

Honor system: This is a very interesting question. I think that acquiring ownership has to be something that people eyeball. Might be worth updating typescript-bot to be able to reply to PRs that update CODEOWNERS to provide a quick summary of: contributions of the submitter to the definition in question, contributions of the submitter to other definitions, contributions or ownership of the library in question (if possible to determine), age of submitter GitHub account, two factor enabled on submitter GitHub account. I do think that any code owner must have two-factor turned on. I think that when any of these checks run, the account should be checked for two-factor enabled, and owners without two-factor enabled when the PR is created should be ignored for that PR. Owners with two-factor enabled will be considered valid.

For some very widely-used definitions - "key packages" - think "Node", for example, it might make sense to hardcode requiring a MS team member as a +1 - in this case, the CODEOWNERS data alone may not be sufficient.

If typescript-bot is going to handle this work, it might even be useful to support the ability to reply to an issue with the action to take similar to how the rust-lang repo uses Bors. This would mean that an MS team member's approval does not necessarily immediately cause a merge unless they also reply and @ the typescript-bot with merge.

I've written the above fairly quickly and I'm not 100% certain all the ideas hang together, but I hope these thoughts are helpful. Thank you for thinking of me for input!

[RyanCavanaugh]

The ownership / honor system question is very tricky. It seems like it would create a very awkward situation if someone submits a PR with good changes and adds their name to the list, and we push back and say "You're not [x] enough to be an author yet" and require them to change their branch (talk about a way to discourage new contributors!). I also want people to feel like they can add their name to this to get clued in on future changes to the file should they care.

I agree with "key packages" requiring a higher bar for merges. I'd probably draw the line at 'underscore' given the data here https://gist.github.com/RyanCavanaugh/71529f9e14d82cec9e3cab2f79fbac58

I've been waffling on the "trusted author" concept. A good security system (if we're calling it 'security') validates the transaction, not the user. If someone doesn't modify tsconfig.json or package.json there's very little undoable damage they can do; for small diffs affecting only .d.ts and test files we can hold a lower bar, but require sign-off from MS or a core DT member for other changes.

I'm definitely all-in on requiring auto-merges to be initiated by an explicit command. The bot can prompt 'trusted' authors only with a @ mention to ask them if they'd like to cause a merge to occur. This makes it less weird when we have "untrusted authors" involved.

We also need to apply slightly different rules depending on what's being edited - things that touch tsconfig.json, package.json, etc, need to be reviewed more carefully.

[isiahmeadows]

@RyanCavanaugh

The ownership / honor system question is very tricky. It seems like it would create a very awkward situation if someone submits a PR with good changes and adds their name to the list, and we push back and say "You're not [x] enough to be an author yet" and require them to change their branch (talk about a way to discourage new contributors!). I also want people to feel like they can add their name to this to get clued in on future changes to the file should they care.

This could be addressed by requiring an okay by at least two owners for that file (or one if the file only has one owner), or MS/a core DT member only in cases where no owner is responsive, disagreements crop up, etc.

I've been waffling on the "trusted author" concept. A good security system (if we're calling it 'security') validates the transaction, not the user. If someone doesn't modify tsconfig.json or package.json there's very little undoable damage they can do; for small diffs affecting only .d.ts and test files we can hold a lower bar, but require sign-off from MS or a core DT member for other changes.

Agreed, but we should still have one or more authors sufficiently trusted to at least catch innocent mistakes. They might not be required to sign off on every PR, but they should definitely be cc'd on any proposed changes, and if a trusted author request changes in a review, the bot should allow and honor that by waiting a few days and not auto-merging. There are other complications, though (subscription of untrusted authors?).

[RyanCavanaugh]

This is live now but undocumented - authors should be able to comment with "mergeplz" to cause the bot to automatically merge the PR. We'd like to try this on a few PRs to work out any remaining kinks and to get a feel for how it works

[Bartvds]

Is there is way for 'code owners' to unsubscribe from the system and bots and forfeit ownership?

I used to be very active in TypeScript so I'm in a ton off definitions and git history but I've not touched any TypeScript for a few years now so changes should not depend on my approval and I don't really need the notifications or merge rights (I don't mind but it seems a bit crufty).

Also there was this https://github.com/Chaosthebot/Chaos project a while ago that had some cool experiments in open repos/merge management by automation. They went through a few approaches with votes and privileged members, owners and such.

[SupernaviX]

Couldn't you make a PR to remove yourself from the list of authors in the headers?

[Bartvds]

Sure, but is the header about attribution or ownership?