Repository navigation
ci(ratchet): assert the file-size ratchet on the base and make a red base heard (#15102) - #15108
Merged
Merged
Conversation
4 tasks
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #15102. Refs #15107.
Thinking Path
The issue's hypothesis was that
code-qualityreported green for a head other than the one merged. The workflow-run history says something sharper, and it changes what the fix has to be.AC1 — which head did the green belong to?
16c104be5is the merge commit on the integration branch, not the head that merged. PR #15100's head was5646344859f. By SHA:43ac98f48981eeaa912f01code-qualityat 10:47:33Z, run32960027518)3b32f7b9e(#15095)e1bf21cf4(#15099)5646344859f16c104be5code-qualityon push: failureSo the green was truthful.
code-qualityran the ceiling audit on1eeaa912f01, where the file was 598 lines and the tree genuinely complied. It never ran on5646344859f, which was created five seconds before the merge. The ten greens a human read were the ten on the previous head.The violation was born in a merge. 584 (merge-base) + 14 (the branch's fakeredis fixtures) + 15 (#15095's additions to the same file) = 613. Neither parent exceeded 600. This contradicts the issue's "the violation existed in that PR alone" — it is precisely a two-branches-combine case, and it matters, because a ratchet evaluated only against a pull request cannot see that class of violation however correctly it is gated. The violating tree did not exist until the merge produced it.
AC2 — the mechanism. No required context reported on
5646344859fat all; its only check-runs (Analyze (…),CodeQL,notify-issues,semgrep) started at or after 11:20:24Z, i.e. after the merge. With ten contexts required, a non-admin merge on that head is refused with "Expected — Waiting for status".enforce_admins: falseon the integration branch is the only setting in the protection table consistent with the merge succeeding.required_status_checks.strict: falseis a red herring for this incident: strict forces the branch to be up to date, and the branch was brought up to date — that update is what created5646344859f.enforce_admins: truealone would have blocked it;strict: truealone would not have.That remedy is a branch-protection setting only the repo owner can flip, so AC2 is recorded and evidenced here but not closed here. It is filed as #15107 with the full SHA table and the decision spelled out. This PR does not pretend to deliver it.
AC3 — and here the issue's premise is also wrong, in a way that changes the deliverable. "Nothing asserts the invariant on the base after a merge" is not true. Two things already do:
code-qualityruns onpushto the integration branch and did fail on16c104be5— run32962821850, step 33, "Audit the python-file-size ceilings for drift";python_file_size_ratchet_test.py::test_recorded_ceilings_match_the_files_todayalready runsaudit_ceilings()over the whole tree with a reach floor.The base guard existed and fired. Nobody was told. The violation was found days later by someone running the audit by hand. So adding a third copy of the audit would have fixed nothing. What this PR adds are the two properties neither existing invocation has.
What Changed
.github/workflows/ratchet-base-guard.yml— apush-triggered base guard on the integration branch. It shells out to the samescripts/check_python_file_size.py --audit-ceilings; the ratchet is not reimplemented, so there remains one source of truth invoked from three places (pre-commit,code-quality, this).It adds exactly two things:
code-qualityfilters both itson.push.pathsand itschangesjob on the backend-Python set, so a push touching no Python leaves the tree-wide invariant unexamined that day. A whole-tree invariant must not be gated on the diff that happened to arrive. There is deliberately nopaths:filter here —git ls-filesis what scopes the run.Hardening worth naming: the audit's exit code is captured rather than allowed to abort the job, so the reporting steps run on exactly the failing path; every downstream
if:is writtensuccess() && …because a customif:replaces the implicit success check, and without it a failed checkout would leavercunset, compare unequal to'0', and file an issue about a violation that was never measured; the audit output reaches the issue body throughenv:andprocess.env, never interpolated into thescript:body, so a path containing a backtick cannot break the step whose whole job is to report the failure;cancel-in-progress: false, because two merges seconds apart is the exact shape this incident had and a cancelled guard reports nothing.This workflow is not a required status check and cannot block a merge, and its header says so in those words. Required contexts are configured in branch protection and evaluated on a pull request's head; this runs on
push, after the merge has landed. It shortens the time a red base goes unheard from "until someone runs the audit locally" to one workflow run. Stopping the merge is #15107.repo_tests/ratchet_base_guard_test.py— 13 tests pinning that wiring, so it cannot quietly regress into the shape that failed. Lands in shard 9 of 12 (stable_shard.py, verified by collection), alongside the existing repo-global ratchet guards, and all twelve shards run in the matrix. Every lookup is a helper function rather than an inline expression, so the same helpers can be driven over a mutated copy of the workflow.Verification
Contrast mutation A — the guard fails when a real violation is present. Appended 140 comment lines to
autobot-backend/api/terminal_websocket_route_test.py(482 → 622) and ran the exact command the workflow runs:Reverted; re-ran:
python-file-size ceilings: 4875 files scanned, 509 grandfathered, all live and at size.rc=0. Working tree clean (git status --porcelainshows only the two new files).Contrast mutation B — the repo test fails when the workflow loses either new property. Replaced the audit invocation with
echoand added apaths:filter to the push trigger:Reverted; 13 passed.
Non-vacuity, both layers.
The workflow's file sweep. Forced
tracked_python_filesto return[]:An empty sweep fails rather than passing quietly, and the workflow leans on that floor rather than restating it.
The test's own enumeration. Every assertion in the new module is a comprehension over
steps(), and a comprehension over an empty list agrees with everything.test_the_step_enumeration_is_not_emptyasserts the flattener returned ≥ 4 steps before anything asserts over them, andtest_the_helpers_reject_a_workflow_that_lost_its_stepsproves the helpers actually go empty on a workflow whose jobs havesteps: []— so the non-emptiness is a property of the helper, not a fact about today's file.Neighbouring guards, unchanged and green.
workflow_concurrency_guard_test.py,python_file_size_ratchet_test.py,code_quality_guard_reach_test.py,composite_action_step_keys_test.py: 100 passed.pipeline-scripts/check_workflow_path_filters.py:rc=0, 6 inline path lists match the canonical set (the new workflow has nopaths:and no dorny step, so it is outside that guard's table by construction).tools/lint/check_spdx_header.pyon the new test:rc=0. New test file is 320 lines, well inside the 600-line limit it helps enforce.What I could not prove. That the merge of #15100 was an admin bypass is an inference from the protection table plus the absence of any required context on the merged head — GitHub's bypass records are not readable through the API available here. The alternative explanation (a required context reporting on a stale head) is ruled out by the evidence: the merged head carried no required context at all, stale or otherwise.
Not claimed. This PR does not make a red base block anything, and does not change branch protection. AC1, AC3 and AC4 are delivered; AC2 is settled and evidenced but its remedy is owner-only and lives in #15107.
Refs, notCloses— #15102 should close once #15107 is decided.Model Used
Claude Opus 5 (1M context).