Repository navigation
Plan and safely reconcile required checks on main - #27
Conversation
zero-aska
left a comment
There was a problem hiding this comment.
Requesting changes: on the five repos with no protection, the initial-protection PUT installs a weaker rule than every protected main in the org, and that rule does not enforce MERGE-SOP's current-head approval.
Blocking:
- tools/reconcile_required_checks.py lines 109-114. When main has no protection, the planner PUTs
required_pull_request_reviews: {required_approving_review_count: 1}and sends norequired_conversation_resolution. GitHub then creates the protection withdismiss_stale_reviewsfalse and conversation resolution false. All 25 protected mains in the policy have both set to true. MERGE-SOP says "The approval is on the current head", and bar/README.md assumes a branch rule that dismisses stale approvals. This path applies now. hpack, der, path, sqlite and ssh have no protection and no ruleset, and their plans readreadywith this PUT. After an admin applies it, a re-plan reports those reposunchanged, so the tool never shows the gap again. Applying it lowers nothing that exists today, but it is the first protection these repos would get, and it is wrong for the org.
To reproduce:gh api repos/Proven-Commons/hpack/branches/main/protectionreturns 404. On this head,python3 tools/reconcile_required_checks.py --repo hpackprintsreadyand the PUT above.gh api repos/Proven-Commons/codec/branches/main/protectionshows the baseline (dismiss_stale_reviews: true,required_conversation_resolution: true).
Fix: add both to the PUT (best as policy defaults) and test them. Or defer unprotected repos ("no protection; apply the org baseline first") and plan no PUT. Then update policy/README.md.
Non-blocking:
- Still a draft. It cannot merge under MERGE-SOP until marked ready.
- Existing any-source checks come back from GitHub with
app_id: null, and the PATCH sends the null back. Normalize it to -1. No live effect today, because every required-check list is empty. - Uncaught cases: a directory at the workflow path (AttributeError), a malformed policy or saved plan (TypeError), and a policy repo that does not exist (aborts the whole run instead of deferring that one repo).
- The PR body calls the 10 skipped tests "Linux-only". They are the LeanTest cases that need BAR_LEAN=1. A full plan now covers mission plus 16 libraries, not only mission and codec.
- The root README line "Live settings remain pending" goes stale once a plan is applied.
What I checked:
- Default mode only reads. Writing needs
--apply-plan. Apply rebuilds the plan and requires an exact match with the saved one. It refuses any deferred repo, rechecks protection before each repo and the head before each write. Existing protection only gets narrow PATCHes and a POST for enforce_admins. There is no DELETE. Merging this PR changes no settings. - I ran the planner against live GitHub with a client that only allows GET. Mission gets a PATCH adding self-test, lean-self-test and template / bar (app 15368). Eleven protected libraries get a PATCH adding
bar / bar. The five unprotected repos above get the PUT. Tar is deferred. The 20 repos without.github/workflows/bar.ymlare deferred. bar / baris green on the main head of all 16 libraries that have bar.yml. Mission's ci.yml has no path filters, and its three jobs are green on main and on this head.- bar/check.py item 7 reads only required status checks. The tar deferral matches tar#7 (Set B is #4-#6). The pin guidance matches bar/README.md.
- Tests:
python3 -m unittest bar/tests/test_required_checks.pygives 19 OK.python3 -m unittest discover -s bar/tests -vruns 100, OK, with 10 skipped (BAR_LEAN). - The PR merges cleanly into current main and with #21, #23, #25, #26 and #28. #16 already conflicts with main in README.md on its own; this PR edits a different paragraph and adds no new conflict.
Generated by Claude Code
When main has no protection, the planner's PUT now dismisses stale approvals and requires conversation resolution, both policy defaults, as every protected main in the org does and as MERGE-SOP's current-head approval needs. Any-source checks that GitHub reports with app_id null are sent back as -1. A missing repository or a workflow path that is not a file is deferred instead of aborting, and malformed input exits with the usual message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QwrvNdsMA6RtgKxXn1Pmhf
26a1733 to
398f567
Compare
zero-aska
left a comment
There was a problem hiding this comment.
Approving as a maintainer who did not write this, under MERGE-SOP.
Earlier blocking finding:
- Fixed. Before, a repo whose main had no protection got a first protection weaker than the org baseline. The PUT now sets
dismiss_stale_reviews: trueandrequired_conversation_resolution: true. Both come from the policy defaults, and the planner rejects a value that is not a boolean. The test checks both settings. I reverted the fix in a scratch copy, and the test failed. policy/README.md says the same thing.
What I ran:
- Head is still 398f567. A range-diff against the reviewed 26a1733 shows the first commit unchanged. All new work is in 398f567. I read every changed file in full.
python3 -m unittest bar/tests/test_required_checks.py: 23 OK.python3 -m unittest discover -s bar/tests: 104 OK, 10 skipped (BAR_LEAN).git diff --checkis clean. CI on 398f567 is green.- A live plan of all 38 policy repos, with a client that allows only GET. Mission gets a PATCH that adds its three checks (app 15368). Twelve protected libraries get a PATCH that adds
bar / bar. Ten unprotected libraries get the new PUT: hpack, der, path, sqlite, ssh, xml, clienthello, ntp, mqtt and jws. Tar and the 14 repos without bar.yml are deferred. - GET protection on all 38. The 25 protected mains have the same settings, and the new PUT matches them in every one. The 13 unprotected mains return "Branch not protected", and no repo has a ruleset.
- Every check the plan would require exists and is green on the current main head of all 23 ready repos. All 22 library bar.yml files are the same. They run on push, pull_request and pull_request_review, with no path filters. No bar.toml overrides the
bar / barname that check item 7 looks for. - An apply simulation on mission, codec and hpack. I saved the plan as JSON and ran apply with live reads and writes intercepted. It accepts the plan and would make exactly two PATCHes and one PUT. The same plan with
dismiss_stale_reviewsremoved from the PUT is rejected with no writes. - The CLI in read-only mode:
--repo hpackexits 0 and shows the baseline PUT,--repo tarexits 2, and an unknown repo exits 1 with the usual message. - Merges: the PR merges cleanly into main. On top of main plus this PR, #23, #25, #26 and #28 each merge cleanly. Main plus this PR, #23, #25 and #28 passes the tests (121 run). #26 conflicts with #23 (bar/template/lakefile.toml) and with #28 (README.md). Neither conflict involves this PR.
The earlier non-blocking notes are also handled. app_id: null is now sent back as -1. A missing repo or a directory at the workflow path is deferred. Malformed input exits 1. The root README wording no longer goes stale. The skip reason is corrected. The PR is out of draft.
Non-blocking notes:
- The PR body is out of date. It says five repos get the PUT and the plan covers mission and 16 libraries. Today ten repos get the PUT, and the plan covers mission and 22 libraries. The admin who applies the plan should know this.
- The two baseline settings are used only when the planner creates protection. If a protected main later loses them, the plan still reads
unchanged. - The planner checks that a required check exists on main, not that it passed. All are green today.
- policy/README.md line 21, "No live protection has been changed by adding this policy", will go stale once a plan is applied.
- The two new keys are now required in any policy, even for repos that are already protected. It would help to say this in policy/README.md.
- The README says exit 2 means something was deferred. An argparse usage error, such as
--apply-planwith--repo, also exits 2.
Generated by Claude Code
Records the required-check policy, and adds a planner that reads GitHub without changing any setting:
Applying needs an explicit saved plan. Before writing, the tool rechecks the policy, the main SHA, the workflow and the current protection. Its narrow patches keep unrelated settings, existing checks, app bindings and stronger review requirements as they are.
Refs #10. Merging this changes no live setting. An administrator still has to review and apply a plan. Tar's workflow landing and a separately reviewed bar-release pin also remain.
Changes since the review of 26a1733
Rebased on current main (f4ebf8d).
dismiss_stale_reviews: trueandrequired_conversation_resolution: true. Both are policy defaults inrequired-checks.toml, and the planner requires each to be a boolean. An approval then counts only on the current head (MERGE-SOP), as on the 25 protected mains. This covers hpack, der, path, sqlite and ssh. The test for the initial protection asserts both settings, andpolicy/README.mdsays so.app_id: nullare now sent back as-1.BAR_LEAN=1, not Linux-only tests. A full plan covers mission and 16 libraries.Validation
python3 -m unittest bar/tests/test_required_checks.py: 23 OK. That is 19 before, plus four new tests: non-boolean baseline settings, nullapp_id, a directory at the workflow path, and a missing repository.python3 -m unittest discover -s bar/tests: OK, 10 skipped (BAR_LEAN).git diff --checkis clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01QwrvNdsMA6RtgKxXn1Pmhf