Branch protection is available again. What do we switch on?

[ARSH871-bot]

Answering this one here rather than on #245, as the first real use of the rule
in the announcement.

The fact that changed

The repository is public again (private: false, checked against the API
today). Branch protection needs GitHub Pro or a public repo, so it is
available to us at no cost right now.

GET /repos/ARSH871-bot/Netwise/branches/main/protection
-> 404 "Branch not protected"

That is available and switched off, not unavailable. It has been both this
month: on 31 August the same call returned 403 "Upgrade to GitHub Pro". So
whatever we decide, the decision only holds while the repo stays public, and
that has now changed three times.

What is worth reading before answering

We measured this on #245 rather than guessing. Across 45 pull requests merged
since 20 August, requiring a review would have blocked none of them — we
already review everything. So "require a review" costs nothing and buys nothing
today; it buys something the first time somebody is in a hurry.

dismiss_stale_reviews is the interesting one, and it contradicts M-2 rule
2. M-2 says a merge-only push does not void an approval, and the reviewer
decides. Auto-dismissal would void it automatically. M-2 is now ratified 4 of 4
as of this morning, so switching that on would put a setting and a signed
agreement in direct conflict.

My proposal, to accept or argue with

1. Require a pull request before merging — yes. Free, matches what we do.
2. Require one approving review — yes. Same reason.
3. Require status checks to pass (pytest 3.12, pytest 3.13) — yes. This
   is the one with real value: CI is currently a signal, not a gate, and we
   have had two red mains this month.
4. dismiss_stale_reviews — no, because M-2 says the reviewer decides
   and M-2 has four signatures.
5. Include administrators — no, not before the 10th. If protection
   misfires during submission week we need to be able to move.

Please reply with agree / disagree per number. I will mark whichever answer we
converge on, and then it is decided — not "decided in a chat somebody
remembers".

[ARSH871-bot]

Correction: the premise of this thread has already moved

I opened this a few hours ago saying branch protection was free because the repository was public. It is private again, and the same API call now refuses:

2026-09-03  early      private: false   404 "Branch not protected"      -> available, switched off
2026-09-03  now        private: true    403 "Upgrade to GitHub Pro ..."  -> unavailable

So none of the five proposals can be actioned today. I am not editing the post above, because what happened to it is more useful than the post was.

This is the third visibility flip this month (public on 27 August, private on 31 August, public and then private again today). CLAUDE.md already carries the lesson in writing: a fact which can change underneath a document should not be stated in the present tense in one. I wrote a present-tense claim into the first line of a decision thread and it was false within hours.

What still holds, and it is most of the thread:

• The measurement stands. Across 45 pull requests merged since 20 August, requiring a review would have blocked none of them.
• dismiss_stale_reviews still contradicts M-2 rule 2, and M-2 is now ratified 4 of 4. That argument does not depend on visibility at all.

What I am changing about how we decide this. Rather than deciding what to switch on, let us decide the rule and write it down, so it can be applied the next time the repo is public without re-arguing:

When branch protection is available, we enable: require a pull request, require one approving review, and require both pytest checks. We do not enable dismiss_stale_reviews, because M-2 gives that decision to the reviewer. We do not include administrators while a submission deadline is inside two weeks.

Agree or argue with that instead. A decision phrased that way survives the setting being unavailable, which the one above did not.

[patelankeet2]

Agree with the rule as re-posted, all of it.

Require a PR, require one approving review, require both pytest checks -- yes to all three, and the measurement already does the arguing for the first two: if zero of the last 45 PRs would have been blocked, it costs nothing today and buys something the day someone is in a hurry. The pytest gate is the one that actually changes behaviour rather than just formalising what we already do -- CI has been a signal we could ignore, and we have had two red mains this month to show for it.

No to dismiss_stale_reviews, and not as a judgement call -- M-2 rule 2 already says the reviewer decides whether a merge-only push voids their approval, and M-2 is signed 4 of 4. Turning that setting on would put a switch and a signed agreement in direct conflict, and the agreement should win.

No to including administrators before the 10th, agreed. Not the week to risk protection misfiring during submission crunch.

[SamikaPerera]

Reposting here -- my original vote landed on #245 rather than on this discussion, since I answered before seeing that the decision had moved. Same position, in the place it actually counts.

Agreed with your position exactly, on all three. YES to requiring status checks -- a red cross being merely advisory is the gap, not CI itself; this just makes the existing signal actually block. YES to requiring one approval -- this is M-1 rule 1 written into the platform rather than left to discipline. Correctly still lets an author merge once someone else has approved, matching the amended rule. NO to dismissing stale reviews -- you're right this is the one that would actively contradict M-2. A merge-only push from main doesn't need re-reading the diff under our own rule, and this setting can't tell that apart from a real content change. The thing that looks like it enforces M-2 is the one that would break it.

[SamikaPerera]

A live data point worth adding to the measurement.

Just pushed a genuine content commit to #284 after Arsh's and Ankeet's approvals -- under M-2 rule 1 that voids both, and I said so on the thread. But GitHub still shows reviewDecision: APPROVED, because dismiss_stale_reviews is off, so nothing on the platform actually stops this merging on approvals that predate the new commit. The only thing holding it right now is the rule being followed voluntarily, in writing, by the person who just broke the approval state.

Not arguing to reverse my vote -- the reasoning against dismiss_stale_reviews still holds, since it can't distinguish a genuine new commit from a merge-only push, and punishing the second case is a real cost. But the "0 of 45" measurement was about what already happened; this is the first time in practice the gap it can't see was actually live, not just theoretical. Worth having on record before the vote closes.

[SamikaPerera]

Following up on my earlier data point -- it's a pattern now, not a one-off. Three instances in the last few days: #284 (my own commit voided two approvals), and now #304 has both Shubham's and Ankeet's approvals predating the two most recent commits. Same underlying gap each time -- reviewDecision doesn't reflect it, only the rule being followed manually does.

Timestamps for the record, checked rather than remembered:

#284   my commit a084c4c            2026-09-05 15:21:35Z
       ARSH871-bot   APPROVED       2026-09-04 05:43:54Z   -- 34h before
       patelankeet2  APPROVED       2026-09-05 07:18:04Z   --  8h before

#304   e8cc4c8 fixture repair       2026-09-08 01:55:06Z
       41cf430 A-4 table            2026-09-08 02:09:52Z
       shubhamkataria2005 APPROVED  2026-09-06 22:31:40Z   -- before both
       patelankeet2       APPROVED  2026-09-06 02:53:06Z   -- before both

The two shapes are not equally concerning, and I think that matters for the vote

Worth naming them separately, because lumping them together overstates the case.

Ankeet's #304 review is the benign shape. It was accurate when given and describes a tree that has since changed. He verified the fixtures end to end against real Batfish and reported the four CVE outcomes -- all true at the time, and still true, because the bug Arsh later found was genuinely latent rather than missed. It only surfaced when Arsh moved a line to see what it was doing. Nothing about that review was wrong; the tree moved under it.

That is close to the merge-only-push case the argument against dismiss_stale_reviews is specifically about, and it is exactly the case where auto-dismissal would cost us a real review for no gain.

Shubham's is the sharper one. He requested changes, approved conditional on adding something himself, did not add it, and the gap he had flagged sat open under a green approval for two days until I wrote it. That is not the tree moving under a review -- it is an approval issued against a condition that was never met, and nothing anywhere recorded that.

dismiss_stale_reviews would not have caught that either, incidentally. There was no push to dismiss on -- the gap was the absence of one. So it is evidence about the manual rule's load-bearing-ness rather than evidence for the setting.

Where that leaves my vote

Unchanged. Still yes to requiring status checks, yes to requiring one approval, no to dismiss_stale_reviews -- the false-positive cost on merge-only pushes is real and Ankeet's case above is a live example of it.

But the "0 of 45" figure was about what had already happened, and three cases in four days is a meaningfully different picture from zero. Not different enough to flip the third setting; different enough that "the rule is followed by hand and that has been sufficient" is now a claim with counterexamples attached rather than an unqualified one.

Two things I would rather see than auto-dismissal, and neither needs a setting:

1. State the condition in the review, not the comment. Shubham's case would have been visible if "approving on the condition that X lands" were part of the review body rather than an offer in prose beneath it.
2. Check the timestamps before merging, not after. All three of these were found by comparing review times to commit times, which takes about ten seconds and is the thing dismiss_stale_reviews would automate badly.

One correction to my own earlier comment here

I said on 6 September that this was "the first time in practice the gap it can't see was actually live." Two of the three cases are mine -- #284 was my commit, and #304's is my PR -- so I should be clear that I am reporting a pattern I am half of, not one I only observed.

Also worth noting, since it changes what is actionable: the repository is private again as of today, and /branches/main/protection returns 404 with nothing configured. So none of the three settings can be switched on right now regardless of what we decide. That is the third visibility flip; @ARSH871-bot's reframing of this discussion as a rule to apply when protection is available rather than a list to switch on today looks better with every one of them.