doc: harden policy around objections

[mmarchini]

Harden policy around objections to avoid misunderstanding and to
encourage collaboration between pull request authors and objectors.

Fixes: #34564

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mmarchini]

This ended up quite verbose. Happy to hear suggestions on how to shrink it down :)

[aduh95]

Would it be worth to add the case when:

1. a collaborator request an explicit change (E.G.: using GitHub suggestion)
2. the contributor commits the suggested change
3. the collaborator is unresponsive to dismiss their objection

I feel that in such scenario, it would be fair to say that if the collaborator has not commented the PR for seven days after the commit time, their objection may be dismissed.

In practice though, collaborators will often "approve" a PR in GitHub UI even when they suggest changes (there are examples in this very PR). The documentation is unclear if such request may be dismissed or the collaborator's approval is actually granted only if those changes are committed. I always assume it's the latter but the current documentation suggests it's the former.

[mmarchini]

@aduh95 that's what this change is trying to clarify. There's a huge difference between a small suggestion which improves code quality a bit and an explicit objection. The first one doesn't prevent a PR from landing, the second one does. If the new text doesn't give that message, I'm happy to hear suggestions.

As for the dismissal part: unresponsiveness has always been grounds to dismiss an objection, it just wasn't defined how long until someone could dismiss someone else's objection. This PR clarifies that, now an objection may be dismissed seven days after a collaborator asks something to the objector and the objector is unresponsive. Again, I'm open to suggestions to improve the text if that's not clear.

[guybedford]

Thanks for putting this together. Hope the comments aren't too much, but it seems important to think through this stuff a little.

Another thing I was wondering is if we should allow a grace period for PR reversion when collaborators miss something? For example, something like:

If a Collaborator is unaware of a PR landing that they would otherwise have blocked, a 48 hour grace period is provided in which previously unstated objections may be retroactively made allowing for the PR to be reverted. After that time period the process of reverting requires full consensus to land.

[guybedford]

Both of these two sentences seem to be restatements of the first statement. I appreciate making sure it is clear, but perhaps just a single restatement would be fine between these?

To add another variation to the mix, another restatement could be a comprehensive statement of the positive criteria for landing a PR.

A PR may land only when both the wait time has passed and there are no explicit objections as marked through the GitHub "request changes" feature.

[mmarchini]

I'll try to rephrase it so it's less repetitive.

[mmarchini]

Sorry, forgot to address these before landing. Sent another PR that should reduce repetitiveness: #34702

[guybedford]

Again it might be nice to state this in the positive:

Any PR objection must include a clear reason for that objection as well as responsiveness to further discussion towards consensus. If reaching consensus is not possible...

[mmarchini]

The suggestion drops the recommendation to provide actionable steps, but I see your point. Let me try to rephrase this too.

[guybedford]

7 days feels a little arbitrary here to me. Eg what if someone is on holiday? Also I'm not sure the criteria for unresponsive are necessarily easy to define without considering what it means to request a response? Does it perhaps make sense to delegate this process to the TSC entirely?

[mmarchini]

7 days feels a little arbitrary here to me

So is 48h to land a PR or 7 days before we can land a PR with only one approval :D. Honestly I don't have any preference as to how long to wait until an objection can be dismissed, so I'm open to suggestions.

Also I'm not sure the criteria for unresponsive are necessarily easy to define without considering what it means to request a response

Can you provide an example of an interaction where "requesting a response" would be ambiguous? Should we require that the objector be mentioned (@) for the time to start counting?

Does it perhaps make sense to delegate this process to the TSC entirely?

I'm against this as it just adds more bureaucracy. Collaborators who don't feel comfortable dismissing an objection can always ping TSC if they want to.

[guybedford]

I guess I'm also just trying to err on the side of encouraging collaboration over process, but agreed these rules are for when collaboration breaks down and having a clear non-process-based clause for moving forward does seem like it could help in such dire circumstances! That said, it seems like it could be healthier to remove any collaborator as a collaborator entirely if things got to such a point as that is simply not collaboration or a healthy project at all. That is, I don't think a functional open source process exists under this clause being triggered.

[mmarchini]

Sorry, missed this comment before.

I guess I'm also just trying to err on the side of encouraging collaboration over process

Always involving TSC to dismiss an objection wouldn't be encouraging collaboration over process though.

but agreed these rules are for when collaboration breaks down and having a clear non-process-based clause for moving forward does seem like it could help in such dire circumstances

We assume all collaborators are acting in good faith in our governance, if we remove that assumption we'd need to rewrite all of our governance documentation. Personally I don't see "the objector being unresponsive" in this case as a "dire circumstance", they might've just missed the notification or might be on a holiday*. "Unresponsive" is not the objector responding with "I still disagree with this change", but instead no interactions with the issue for seven days.

Again, collaboration over process, if an objector goes on holiday and indicates so on their GitHub profile or commenting on the PR, I'd expect the author to wait until the objector comes back before dismissing the objection.

That said, it seems like it could be healthier to remove any collaborator as a collaborator entirely if things got to such a point as that is simply not collaboration or a healthy project at all

In extreme cases where collaboration breaks down and becomes hostility between collaborators, the TSC and Moderation team should be notified and will step in to mediate the discussion. Removing a collaborator from the org is one of the most extreme actions we can take, and AFAIK we don't have any clear policies around it besides inactive collaborators (TSC is allowed to move inactive collaborators to Emeritus) and breach of privacy of Moderation reports. With that being said, the TSC is responsible for "maintaining the list of Collaborators", which could be interpreted as having the power/responsibility to remove someone if needed. Again, this is such an extreme case that if happened would probably damage the project healthiness for a while.

That is, I don't think a functional open source process exists under this clause being triggered

Not sure which clause you're referring to, can you elaborate?

---

With all that being said, I think we went too much in the weeds of an issue that rarely happens because of an isolated case that happened recently (which would've been prevented by the changes on this PR). Therefore I'll go ahead and land this, but feel free to reply if you want to continue the discussion.

[guybedford]

100% of course it can be used in a collaborative way to dismiss an objection without needing full ceremony. That said there is a side of this which could be used in bad faith or as a last resort when communication breaks down but let's hope it never comes to that and if it does that things can be mediated properly through the TSC.

[mmarchini]

Another thing I was wondering is if we should allow a grace period for PR reversion when collaborators miss something?

We already have review times, unless/until we remove those I don't think that's necessary (definitely something to think about if we ever land #33879). We don't have rules against reverting commits, so any collaborator can open a PR asking to revert a change by providing reasoning. Blocking such a PR follows the same rules as objecting to any other PRs, so appropriate reason should be provided (I wouldn't consider "we landed this already" an appropriate reason for example, but using the same reasoning as to why the original PR was opened would be valid). If a revert is requested because process was not followed, it can always be fast-tracked (and likely will be, like the recent incident).

[mmarchini]

I'll try to rephrase it so it's less repetitive.

[mmarchini]

The suggestion drops the recommendation to provide actionable steps, but I see your point. Let me try to rephrase this too.

[mmarchini]

7 days feels a little arbitrary here to me

So is 48h to land a PR or 7 days before we can land a PR with only one approval :D. Honestly I don't have any preference as to how long to wait until an objection can be dismissed, so I'm open to suggestions.

Also I'm not sure the criteria for unresponsive are necessarily easy to define without considering what it means to request a response

Can you provide an example of an interaction where "requesting a response" would be ambiguous? Should we require that the objector be mentioned (@) for the time to start counting?

Does it perhaps make sense to delegate this process to the TSC entirely?

I'm against this as it just adds more bureaucracy. Collaborators who don't feel comfortable dismissing an objection can always ping TSC if they want to.

[guybedford]

We already have review times, unless/until we remove those I don't think that's necessary

Consider the same example of what happened recently, where say @devsnek had not seen the PR at all and was not aware of it (so I'm saying hypothetically if he was not a blocker on the original PR), then only became aware of it after it landed. My concern is that his ability to revert would then have been impeded entirely if we don't allow some grace period. That is we force collaborators to be quick to block and those who aren't watching the stream can easily miss things.

Perhaps a grace period doesn't help collaborators missing things though too, I was just trying to think about that problem from the other perspective here.

[devsnek]

our review period is already what accounts for making sure everyone sees the pr. given our incredibly low revert rate, I would say it works pretty well.

[mmarchini]

My concern is that their ability to revert would then have been impeded entirely if we don't allow some grace period

It woudln't have been impeded because:

We don't have rules against reverting commits, so any collaborator can open a PR asking to revert a change by providing reasoning. Blocking such a PR follows the same rules as objecting to any other PRs, so appropriate reason should be provided (I wouldn't consider "we landed this already" an appropriate reason for example, but using the same reasoning as to why the original PR was opened would be valid).

Happy to hear others thoughts on this though.

[guybedford]

The issue is that the revert might not be able to land as it might not have consensus since the criteria to land have flipped around.

I guess it's about considering if there is some degree of baking in place or if merged is considered merged. And, in spite of my previous position on this, having some degree of baking does seem worthwhile to me as otherwise bad stuff can creep in and be impossible to remove via the existing consensus process being one-way. It also encourages stronger blocking knowing a merge is final whereas some grace period might make landing seem less serious.

It is likely getting off-topic to go down this path now, but wanted to mention the concept at least while we were thinking about cases of these things.

[mmarchini]

The issue is that the revert might not be able to land as it might not have consensus since the criteria to land have flipped around.

If there are objections in the revert PR and the reverter and objectors can't reach a consensus, the issue would be escalated to TSC and the PR would either land or be closed, just like the original PR. Unless it's more likely for TSC to rule one way or the other depending if it's a PR or a revert, which I don't think would be the case for most PRs (unless the original PR was included in a release). Again, "we already landed this PR" is not, in my opinion, a valid reason to object to a revert PR.

I see your point on the grace period, but then again, there's no reasonable wait time we can add to prevent someone from eventually missing a PR. I think a "grace period" can be discussed in another issue/PR since the changes in this PR don't make the grace period more needed than it was before it.

[mmarchini]

As a side note, a grace period also means PRs will need to wait X days before being included on Current, which is not required today, and such policy change would need to be approved by the Release team (since the Release plan falls under their charter).

[mmarchini]

Landed in 71aeea1