Skip to content

ci(ratchet): assert the file-size ratchet on the base and make a red base heard (#15102) - #15108

Merged
mrveiss merged 1 commit into
Dev_new_guifrom
issue-15102-base-ratchet-guard
Aug 26, 2026
Merged

mrveiss merged 1 commit into
Dev_new_guifrom
issue-15102-base-ratchet-guard

Conversation

@mrveiss

@mrveiss mrveiss commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Refs #15102. Refs #15107.

Thinking Path

The issue's hypothesis was that code-quality reported 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? 16c104be5 is the merge commit on the integration branch, not the head that merged. PR #15100's head was 5646344859f. By SHA:

SHA what it is time (UTC) the file required contexts present
43ac98f4898 merge-base of branch and base — 584 —
1eeaa912f01 branch head when the checks ran 10:46:23Z 598 all ten green (code-quality at 10:47:33Z, run 32960027518)
3b32f7b9e (#15095) lands on the base 11:19:46Z 599 —
e1bf21cf4 (#15099) lands on the base 11:20:03Z 599 —
5646344859f "Update branch" merge — the head that merged 11:20:16Z 613 none at all
merged → 16c104be5 11:20:21Z 613 code-quality on push: failure

So the green was truthful. code-quality ran the ceiling audit on 1eeaa912f01, where the file was 598 lines and the tree genuinely complied. It never ran on 5646344859f, 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 5646344859f at 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: false on the integration branch is the only setting in the protection table consistent with the merge succeeding.

required_status_checks.strict: false is 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 created 5646344859f. enforce_admins: true alone would have blocked it; strict: true alone 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-quality runs on push to the integration branch and did fail on 16c104be5 — run 32962821850, step 33, "Audit the python-file-size ceilings for drift";
  • python_file_size_ratchet_test.py::test_recorded_ceilings_match_the_files_today already runs audit_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 — a push-triggered base guard on the integration branch. It shells out to the same scripts/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:

  1. It cannot be skipped. code-quality filters both its on.push.paths and its changes job 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 no paths: filter here — git ls-files is what scopes the run.
  2. A failure becomes a tracked issue, opened or updated by title and closed again by the first clean run, and the run goes red. That is the artifact a human reads; a log line is not a report.

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 written success() && … because a custom if: replaces the implicit success check, and without it a failed checkout would leave rc unset, compare unequal to '0', and file an issue about a violation that was never measured; the audit output reaches the issue body through env: and process.env, never interpolated into the script: 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:

$ python3 scripts/check_python_file_size.py --audit-ceilings
autobot-backend/api/terminal_websocket_route_test.py: 622 lines (max 600)
rc=1

Reverted; re-ran: python-file-size ceilings: 4875 files scanned, 509 grandfathered, all live and at size. rc=0. Working tree clean (git status --porcelain shows only the two new files).

Contrast mutation B — the repo test fails when the workflow loses either new property. Replaced the audit invocation with echo and added a paths: filter to the push trigger:

FAILED test_the_guard_invokes_the_shared_audit_rather_than_reimplementing_it
  AssertionError: expected exactly one ceiling-audit invocation, found 0
FAILED test_the_guard_carries_no_paths_filter
  AssertionError: the base guard declares a path filter, so a push that touches
  no matching file skips it — and a skipped guard reports nothing
2 failed, 11 passed

Reverted; 13 passed.

Non-vacuity, both layers.

The workflow's file sweep. Forced tracked_python_files to return []:

sweep reached: 0 | problems from the sweep: []
reach check: the tree walk reached 0 files, under the 3000-file floor —
  it stopped covering most of the tree, so this run's verdict covers almost nothing.
run_audit() rc = 1

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_empty asserts the flattener returned ≥ 4 steps before anything asserts over them, and test_the_helpers_reject_a_workflow_that_lost_its_steps proves the helpers actually go empty on a workflow whose jobs have steps: [] — 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 no paths: and no dorny step, so it is outside that guard's table by construction). tools/lint/check_spdx_header.py on 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, not Closes — #15102 should close once #15107 is decided.

Model Used

Claude Opus 5 (1M context).

@github-actions

Copy link
Copy Markdown
Contributor

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

@mrveiss
mrveiss merged commit ce0edf7 into Dev_new_gui Aug 26, 2026
61 checks passed
@mrveiss
mrveiss deleted the issue-15102-base-ratchet-guard branch August 26, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant