Skip to content
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.

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

tools: change inactive limit to 9 months #52459

Closed
wants to merge 1 commit into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 10, 2024

Referencing the latest TSC meeting, I'm proposing decreasing the inactive collaborator duration to 9 months.

Closes: nodejs/TSC#1524

cc @nodejs/tsc

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Apr 10, 2024
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

18 -> 12 makes sense to me

12 -> 9 can easily catch people with a new kid on a 6 month paternity leave or people going through personal issues.

Rejoining isn't too dramatic but it's a barrier to contribution, collaborators have ownership but little harm potential so I don't understand this change?

@anonrig
Copy link
Member Author

anonrig commented Apr 11, 2024

Rejoining isn't too dramatic but it's a barrier to contribution, collaborators have ownership but little harm potential so I don't understand this change?

@benjamingr request-ci single handedly is a really big security concern. any collaborator can run arbitrary code and execute it in those machines. decreasing duration doesn't solve this problem, but reduces the attack vector

@anonrig anonrig added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 11, 2024
@benjamingr
Copy link
Member

I mean, we give request-ci powers to triagers with no process other than requesting to join - there are easier ways than inactive collaborators to abuse triggering ci (which is why release machines are separate?)

@panva
Copy link
Member

panva commented Apr 12, 2024

So I assume this PR will get followed up in 2 days by yet another decrease to 6 months? /s

There's nothing new in the referenced issue that explains why the decrease to 12, which was a pushback compromise up from 6, that landed just few days ago is being revisit yet again. All I'm asking for is more elaboration and reasoning than linking to the same issue as before with no additional details.

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

I share the same opinion as @benjamingr

@benjamingr
Copy link
Member

benjamingr commented Apr 12, 2024

@anonrig you really feel strongly enough about this change to spend time on it in the TSC meeting? If it's a security concern let's kick of discussion about how to guard against the sort of attacks being a collaborator makes possible.

Collaborators can:

  • Run code on CI when it's not locked down (admittedly, triagers can do that too and that only requires self nomination)
  • Author code (admittedly being a collaborator isn't a requirement or directly related).
  • Review code (we're missing people to do that work) and join teams to get pinged about various areas in the code.

Collaborators aren't:

  • admin the CI machines (that's the build wg)
  • cut and make a release (that's the release wg)
  • get exposed to security bugs before they are public (that requires TSC membership).
  • can't access GitHub org admin stuff like the audit log and secrets (that's TSC and probbly moderation)

There are 100 collaborators, it is impractical to assume we'll never have bad actors or that the biggest threat vector is people being inactive + their GitHub being hacked. It is equally as likely someone will target a current collaborator, steal a laptop etc - though at that point target an org admin/releaser/tsc member.


I feel like this will change will lead to fewer collaborators without any security benefits. We have a hard time getting competent people to engage and keep engaging as it is. Honestly I voted in favor of the 18->12 changes because 18 did seem kind of excessive and I personally trust you to have given it some thought but it was more of a "I like Yagiz and want to see his initiatives to improve the project succeed" and less of a "this closes an attack vector".

@GeoffreyBooth
Copy link
Member

This doesn’t really close an attack vector, it just reduces its size; and with tradeoffs. I feel like if we’re going to spend effort on improving security, it should be devoted to closing attack vectors. For example, phasing out Jenkins entirely and running only in GitHub Actions, if such a thing were possible; so that “ability to run CI” isn’t by itself an attack vector. Or having Jenkins run build and test within Docker containers, so that maybe-untrusted code is sandboxed. Or I’m sure there are other ideas.

Those efforts are more work than just changing 12 to 9, but would actually concretely make the project more secure.

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

I agree with @benjamingr and @RafaelGSS as well.

IMO based on this discussion, the last TSC call and offline discussions... I strongly suggest to explore this topic as an initiative in the Security Team for 2024 (see: nodejs/security-wg#1255).

In the past the team was leading initiatives like the Threat Model, OSSF Scorecard, Best practices... with a great success. I am sure that the team can elaborate a list of potential changes in the org (policies, tooling, etc..) with the relevant SMEs to increase our resilience against this kind of attacks.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 13, 2024

Rejoining isn't too dramatic but it's a barrier to contribution, collaborators have ownership but little harm potential so I don't understand this change?

If not being a collaborator wasn't a barrier to contribution before one became a collaborator, I can't really see rejoining somehow is suddenly a barrier to contribution when someone is just making a comeback instead of being a fresh new person - you can contribute no matter you are a collaborator or not. And I think we can grant at least triage permissions to inactive collaborators so that they can start CI and land commits via the commit queue, so they already have more privileges over a fresh new contributor.

Another downside of having an exceptionally long list of collaborators that are mostly inactive is that people can get the wrong idea about the support that the project can provide - you see >100 people in the team and you thought this project must have a lot of maintenance going on and when you open an issue, you'd expect at least 1 of those >100 people are going to respond (if you check out the issue tracker now you'll find lots of issues that still haven't even got a reply after months); Or if the bug is serious enough, then at least one of those >100 people should be able to fix it soon enough so that it doesn't keep bugging everyone for 2 years until the non-regressing releases are EOL, right? (This has happened with the vm module regressions that got stuck for 2 years and got many Jest or other testing framework users stuck with the EOL 16).

The long list of inactive collaborators that are not marked as emeriti is unhealthy IMO. People would've had a more accurate expectation when they see that there are only like 30-40 active collaborators (I would say that's closer to reality). This disparity also strikes you more like the project itself is inactive (because it seems the people behind it, despite being listed as non-emeriti, aren't that active anyway), instead of that the active contributors are just not keeping up with the workload. I also think in a way this false impression would make outsiders less aware of the under-maintenance that the project has already been suffering for a few years. The closer the list is to the actual list of active collaborators, the more it helps outsiders become aware of problem and do something.

@benjamingr
Copy link
Member

If not being a collaborator wasn't a barrier to contribution before one became a collaborator, I can't really see rejoining somehow is suddenly a barrier to contribution when someone is just making a comeback instead of being a fresh new person

The thesis is that when you remove a collaborator they are more likely to feel excluded or feel their work on Node is "done".

Another downside of having an exceptionally long list of collaborators that are mostly inactive is that people can get the wrong idea about the support that the project can provide

I see the merit in this line or argument (vs. security), it may make sense to create further distinction based on much more recent activity (last 3m) of "active maintainers" or some such.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 14, 2024

The thesis is that when you remove a collaborator they are more likely to feel excluded or feel their work on Node is "done".

We are not removing them, just moving them to emeriti. I think we've always been very clear that if they want to come back, they can just ask. Or I would say better yet, we could add a rule saying they could be moved back automatically if they've sent X PRs/left X reviews in a period of Y. I found find it surprising that someone who haven't opened any PR or submitted any reviews for months would expect the project to continue listing as non-emeriti (or, active) collaborators. In fairness I think in the project, new collaborators would think of you as a stranger if you have been absent for long enough, no matter you are in a list or not. Keeping them in the list is a mere gesture and won't really change how others feel about the actual absence.

@RafaelGSS
Copy link
Member

So, my genuine feeling is that this change will only provoke non-productive discussions at a TSC meeting rather than its desired benefit. 12 months is a doable time frame considering all the advents of life mentioned by @mhdawson. If security is the concern here, I believe we should delegate such discussion to the @nodejs/security-wg as suggested by @UlisesGascon.

Even if we reduce the time frame, it won't necessarily reduce the risk of running untrusted code. If a malicious actor takes action on an "inactive" user, they could ask to rejoin the team, and they'll have access again. This change might make contributing to Node.js a bit harder, and you might feel pressured to have a commit/or a PR reviewed on Node.js core in 9 months, there are many other ways to be active (as the working groups).

Additionally, it's important to remember that having a Node.js position, such as nodejs collaborator, can be crucial for one's career, and changing this role might impact their life.

Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

I think this would be better discussed in the security-wg so we can come with a clear understanding before opening a discussion in TSC.
I personally believe it's a good thing to remove inactive collaborators but changing to 9 to 12 is not relevant.
I would personally think of other ways to fix the problem.

@naugtur
Copy link

naugtur commented Apr 17, 2024

If the project trusted someone to not run malicious code and not get their account compromised for the duration of their activity, the inactivity period doesn't change much security-wise. Getting compromised while being inactive is even less likely. And if a person develops malicious intentions, remaining active is trivial.
The security argument is not very compelling.

As someone who's been around in working groups I don't find the lack of org access detrimental to getting involved in a group and working my way in.
The perception might be different for someone experiencing anxiety or burnout tho.

@GeoffreyBooth GeoffreyBooth removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 18, 2024
@anonrig
Copy link
Member Author

anonrig commented Apr 18, 2024

@GeoffreyBooth The meeting notes from last TSC meeting refers to leaving the tsc-agenda on the pull-request:

@mhdawson will open issue in security wg repo, will tag Yagiz on issue and also leave on the agenda so we can discuss again when other TSC members are on the call.

@ovflowd
Copy link
Member

ovflowd commented Apr 24, 2024

  • This will disproportionately affect contributors who need to take breaks. And I suspect those who are more likely to take breaks will significantly overlap with those in marginalised groups in tech and open source already. A collaborator's opinion should not be less recognised because they have had 9 months off on parental leave (for example, as is standard in some countries) or hospitalised due to illness.

Hey, @BethGriggs, have you checked some of my points in my comment 🤔 I feel that a reorganization of the team structure for Node.js Collaborators would make sense here, no?

@BethGriggs
Copy link
Member

Hey, @BethGriggs, have you checked some of my points in my comment 🤔 I feel that a reorganization of the team structure for Node.js Collaborators would make sense here, no?

Yes. But, today being a collaborator is what grants you both the commit-bit and formal recognition of your approval/block on a PR. So, I think it depends on what outcomes are desired from the proposal:

  1. To restrict commit access to collaborators who need it to reduce security risk.
  2. To not formally recognise the approvals/blocks of collaborators who have been, by bot definition, inactive in the project for 9 months.

This proposal would conflate, and achieve, both. 1. could be independently achieved through reorganisation as you describe. 2. is what I feel would unduly penalise people who could still bring valuable insight.

@ovflowd
Copy link
Member

ovflowd commented Apr 24, 2024

Hey, @BethGriggs, have you checked some of my points in my comment 🤔 I feel that a reorganization of the team structure for Node.js Collaborators would make sense here, no?

Yes. But, today being a collaborator is what grants you both the commit-bit and formal recognition of your approval/block on a PR. So, I think it depends on what outcomes are desired from the proposal:

  1. To restrict commit access to collaborators who need it to reduce security risk.
  2. To not formally recognise the approvals/blocks of collaborators who have been, by bot definition, inactive in the project for 9 months.

This proposal would conflate, and achieve, both. 1. could be independently achieved through reorganisation as you describe. 2. is what I feel would unduly penalise people who could still bring valuable insight.

Yeah, I support those statements; I believe what I was thinking of was something in the line of:

  • The differentiation of Collaborators that need commit access (can merge PRs) and collaborators that don't need commit (merge) access;
    • Both can still approve/block (their approvals/requests for changes have color and impact on our Bot) PRs with GitHub's approval system, and both can request and add any label to the PRs/Issues.
  • Triagers should still be able to add/remove PR/Issue labels as triagers, but if they add sensitive labels (i.e.: trigger our CI) the GitHub Action should simply ignore/remove the label and alert that they don't have permissions.

IMO this should allow us to solve the security issues, not need to mark collaborators as emeritus and still be able to give granular access to which collaborators actually need commit access.

@aduh95
Copy link
Contributor

aduh95 commented Apr 24, 2024

  • Triagers should still be able to add/remove PR/Issue labels as triagers, but if they add sensitive labels (i.e.: trigger our CI) the GitHub Action should simply ignore/remove the label and alert that they don't have permissions.

Not related to this PR, but now that the bot will only spawn CI jobs on PRs with approvals, I'm not convinced it makes sense to restrict triaggers from using the label.

@ovflowd

This comment was marked as off-topic.

@mhdawson
Copy link
Member

I've made my points in the discussions in other issues and the TSC meetings, but just want to make it clear that I don't think we should reduce the time. This is in order to avoid catching people due to life events. 12 months seems likes the reasonable compromise.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2024

Before we move forward here I think it's important that we clarify a few things in our process.

Let's separate two things: (a) Being a Collaborator and (b) Having a currently active commit bit.

I think that once someone becomes a Collaborator they should retain that distinction indefinitely unless they either (a) voluntarily give it up or (b) have to be explicitly removed due to extenuating circumstance (like violating the Code of Conduct). Once someone becomes a Collaborator, they have the right to have a currently active commit bit.

That said, if the user becomes inactive for some period of time, it should be perfectly fine for their commit bit to be deactivated until such time as they choose to request it to be activated again.

For instance, let's take a scenario involving a legally protected classification: Someone goes on maternity leave. In Germany, legally protected maternity leave is 2 years if I'm not mistaken. Someone can take maternity leave from their job and be completely inactive for 2 years and return back to the same position later. Their status as an employee may not have changed during that period of time but it's entirely possible and likely that their account permissions to perform various actions may have been suspended while they were inactive. I think that's exactly how we should be viewing the situation here.

As a Collaborator, if I go inactive for 9 or 12 months, I should expect my commit bit to likely be suspended during that time, but not my status as a Collaborator. If I choose to return to making active contributions, it should be a simple matter of requesting that my commit bit be reactivated so I can resume the typical activities. There should be as little process and ceremony about reactivating as possible. Critically tho, I'm still listed as a Collaborator (albeit possibly with a "Currently inactive" additional label).

Suspending the commit bit automatically should occur only after a period of 9 to 12 months during which I have not performed any actions that require the commit bit. For instance, if all I do is comment on issues and PRs for 9 to 12 months, without signing off on anything, opening any PRs, landing any PRs, etc, then I should reasonably expect my commit bit to be suspended. If I suddenly want to perform any action that requires that commit bit, and I find it has been suspended, then it should be a very simple matter of having it reactivated. During this time, however, my status as a Collaborator should not have otherwise changed.

So, under this approach, was conditions would cause someone to be removed as a Collaborator? There would be only two:

  1. Voluntary resignation. To which their status changes to "Emeritus".
  2. Explicit removal for cause by vote of the TSC (e.g. for things like Code of Conduct violations). To which their status changes to "Removed".

A Collaborator with an active commit bit would have the status, "Active".
A Collaborator with an inactive commit bit would have the status, "Inactive".

Collaborators with either status otherwise should retain all the same rights and privileges.

The TSC may remove "Active" or "Inactive" Collaborators for cause at any time, but that "cause" should never be simply because their status changes from "Active" to "Inactive". The cause could be: This person has been inactive for years and has made no sign that they are ever going to return because they're off doing Rust stuff now, or something. But that decision would require a consensus/vote of the TSC and shouldn't be made solely on their recent activity levels.

If a Collaborator intends to take a break, they can request to explicitly have their status changed to "Inactive", which would cause their commit bit to be suspended.

If we can agree on these clarifications, I think we should codify them in the policies and then I'm +1 on making the "Inactive" status limit (that suspends the commit bit but does not revoke Collaborator status) set to 9 months.

@ovflowd
Copy link
Member

ovflowd commented Apr 25, 2024

Before we move forward here I think it's important that we clarify a few things in our process.

Let's separate two things: (a) Being a Collaborator and (b) Having a currently active commit bit.

I think that once someone becomes a Collaborator they should retain that distinction indefinitely unless they either (a) voluntarily give it up or (b) have to be explicitly removed due to extenuating circumstance (like violating the Code of Conduct). Once someone becomes a Collaborator, they have the right to have a currently active commit bit.

That said, if the user becomes inactive for some period of time, it should be perfectly fine for their commit bit to be deactivated until such time as they choose to request it to be activated again.

For instance, let's take a scenario involving a legally protected classification: Someone goes on maternity leave. In Germany, legally protected maternity leave is 2 years if I'm not mistaken. Someone can take maternity leave from their job and be completely inactive for 2 years and return back to the same position later. Their status as an employee may not have changed during that period of time but it's entirely possible and likely that their account permissions to perform various actions may have been suspended while they were inactive. I think that's exactly how we should be viewing the situation here.

As a Collaborator, if I go inactive for 9 or 12 months, I should expect my commit bit to likely be suspended during that time, but not my status as a Collaborator. If I choose to return to making active contributions, it should be a simple matter of requesting that my commit bit be reactivated so I can resume the typical activities. There should be as little process and ceremony about reactivating as possible. Critically tho, I'm still listed as a Collaborator (albeit possibly with a "Currently inactive" additional label).

Suspending the commit bit automatically should occur only after a period of 9 to 12 months during which I have not performed any actions that require the commit bit. For instance, if all I do is comment on issues and PRs for 9 to 12 months, without signing off on anything, opening any PRs, landing any PRs, etc, then I should reasonably expect my commit bit to be suspended. If I suddenly want to perform any action that requires that commit bit, and I find it has been suspended, then it should be a very simple matter of having it reactivated. During this time, however, my status as a Collaborator should not have otherwise changed.

This aligns very well with what I believe we should do; +1

@mcollina
Copy link
Member

There is likely a language/cultural barrier on this, because for me it seems that we should just rename what we current have as “Emeritus” as “Inactive Collaborators”, and keep “Emeritus” to folks that have left the org.

The active/inactive list can just be internal and reflect the permissions.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2024

I don't think that quite covers it. It can be jarring for someone to see that they've been removed from some team if they've been inactive for a while but intend to keep contributing. I think we do need to clarify the documentation and communication around this, and make sure it's clear what the process is for reactivating the commit bit after a hiatus.

@ShogunPanda
Copy link
Contributor

I'm +1 on James' idea.
I also agree with Matteo, if we find a good naming that's gonna be it.
I propose:

  1. Active Collaborator
  2. Associate Collaborator (no commit bit) - I don't like the term "Inactive" as it seems the person is not engaged while, as James said, it might just be that the person comments on others' PR or issues but it just happen not to be directly contributing code.
  3. Emeritus Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Apr 26, 2024

I guess we could create a @nodejs/committers team, which would be a child team of @nodejs/collaborators, and only those (and releasers) are allowed to push commits on main, then we don't need to find a new name for those who are not interested in having the commit bit?

I think it still makes sense to move folks to emeritus after a long-enough activity period, because:

  • it'd reflect better what level of maintenance to expect.
  • even without the commit bit, it would still be bad if some old account of an inactive collaborator were taken over by a malicious acto: though they couldn't push anything bad, they'd still have access to some private repos (e.g. nodejs/moderation, nodejs/collaborators).
  • if we're adding more folks without removing anyone, we're going to end up filling the number of seats allowed in our GitHub org.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 26, 2024

If people do care about the red/green mark of reviews, those without write access to the repo won't have colored reviews (I guess that'll need a feature request to GitHub?). (For landing that's irrelevant, the bot checks the README, though it may seem weird for new contributors that people coming in with grey approvals allowing their PR to land or grey change requests blocking their PR)

@aduh95
Copy link
Contributor

aduh95 commented Apr 26, 2024

@joyeecheung folks can still have write permission on the repo (and therefore green/red marks) without having permission to push on main.

@benjamingr
Copy link
Member

What James said sounds reasonable to me and I'd like to echo the sentiment of several people here that reducing the inactivity limit is by design exclusionary and goes against our traditional spirit of inclusivity (the actual kind not the performative kind)

@mhdawson
Copy link
Member

I'm not sure we need any new names/roles which will require bikeshedding, just internal teams which affect the commit bit. An implementation for what James suggested could be:

Collaborators are in the list on the README.md and stay there whether they have the commit bit or not. There can be process for moving people out of that list, but it should be at an interval where we have consensus it does not run into the issues we've been discussing related to inclusivity (or never as @jasnell suggested if that is what we feel makes sense).

We can have a shorter interval where the commit bit is removed, that would not have any effect on the README.md. I would just name the teams collaborators and collaborators_with_commit_bit. All collaborators would be in the first one, and only those with the commit bit would be in the second. It should not be a public event to join or be removed from the collaborators_with_commit_bit team and any collaborator can make the request to be added to that team if they are currently not in it.

@benjamingr
Copy link
Member

benjamingr commented Apr 26, 2024

Note that Yagiz explicitly said in the last TSC meeting it's not about security and I believe we have consensus about the security implications (this not being about security).

I believe this is explicitly in order to exclude people in order to more accurately reflect the current number of people maintaining the project as part of their daily/weekly routine.

To be fair it's a sentiment I understand - it's frustrating to have to spend my n-hours-a-week reviewing stuff instead of coding when there are ~100 people who are collaborators but few actually review/code.

Personally, being inclusive and accommodating to people is something I believe in. Especially now that we have the data thanks to Antoine to back up the fact a non-zero number (a 2 digit percent) of contributors take breaks. I also agree with Beth this negatively impacts minorities and people with small kids.

We need more maintainers and collaborators, we should spend time discussing how to make Node.js a better place for new contributors and encourage repeat contributors rather than alienate existing ones. We need a new repl, a few big refactors, a lot of initiatives to drive (I swear I'll get back to primordials!).

@lemire
Copy link
Member

lemire commented Apr 26, 2024

when there are ~100 people who are collaborators

I think that the current number is 86.

(Note that I consider that 86 is ~100.)

@lemire
Copy link
Member

lemire commented Apr 26, 2024

If you were to switch to 9 months, out of the current 86 collaborators, how many would fall off ?

@ovflowd
Copy link
Member

ovflowd commented Apr 27, 2024

We need more maintainers and collaborators, we should spend time discussing how to make Node.js a better place for new contributors and encourage repeat contributors rather than alienate existing ones. We need a new repl, a few big refactors, a lot of initiatives to drive (I swear I'll get back to primordial!).

💯 with you here. It's a tricky matter but a highly crucial one:

We need to refactor/revamp the teams under the Node.js org; there are too many teams in too many different subtrees, which is highly confusing.

  • Separate the Core Collaborators Teams to clarify who the people often reviewing and committing code are versus all other collaborators with significant participation in the Core, but not necessarily by committing.
    I believe a more precise separation makes it easier for the outer community to realize that we don't have as many people making code as they believe. It also reduces the pressure other folks have to be active with commits or code reviews to remain active core collaborators.
  • I believe that we should have an official off-GitHub community, such as Discord (Slack barrier of entry is extremely high), but I understand this is off-topic and hard to maintain
  • I wish we had better welcoming guidelines for new core collaborators (like a welcome email and a dedicated welcome page, often we need to copy-paste a bunch of URLs, and there's so much missing documentation)

@benjamingr
Copy link
Member

@lemire that's because we removed ~14 when we reduced it from 18 to 12, which means statistically there are ~2 less people who will contribute to the project in the future based on Antoine's statistics.

@targos
Copy link
Member

targos commented May 8, 2024

Removed tsc-agenda as the vote happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to reduce inactive collaborator duration