Repository navigation
The Python and shell size gates duplicate ~90% of their logic, and the Python ratchet test's monkeypatching blocks the obvious fix #17377
Description
scripts/check_python_file_size.py (359 lines) and scripts/check_shell_file_size.py (307 lines, added by #17353) implement the same ratchet with the same three-way semantics. Roughly 90% of the logic is common: repo_root, normalise, count_lines, the tracked-file walk, verdict / _grandfathered_verdict, unmeasured, _vanished_entry_problem, _scan_tracked_files, audit_ceilings, run_audit, main. Everything that genuinely differs is configuration: the glob, MAX_LINES, the ceilings mapping, the excluded prefixes, the reach floor, and two repo-relative path strings.
This is the reusable-components goal case exactly — and the duplication guard only ratchets down, so the cost is real rather than aesthetic.
Why it was not done in #17353, and what the next attempt must solve first
repo_tests/python_file_size_ratchet_test.py monkeypatches the hook's MODULE ATTRIBUTES, not an object it passes in:
monkeypatch.setattr(hook, "repo_root", lambda: tmp_path)
monkeypatch.setattr(hook, "KNOWN_LARGE", {})
monkeypatch.setattr(hook, "tracked_python_files", lambda root: [unlisted])
monkeypatch.setattr(hook, "audit_ceilings", lambda: (0, []))A shared engine that takes a config object would resolve those names through itself, not through the calling module, so every one of those patches would stop taking effect. The dangerous part is the failure mode: several of these tests would keep passing while asserting nothing — a monkeypatched KNOWN_LARGE = {} that is silently ignored means the test exercises the real ceilings and still goes green.
So the extraction is not a move-the-functions job. It requires rewriting that 590-line test's patching strategy to inject through whatever seam the engine exposes, and a way to show the rewritten tests still fail when they should — otherwise the consolidation is verified by tests that no longer test anything.
Acceptance criteria
- One engine implements the ratchet; both gates are thin configuration over it.
- Neither gate's observable behaviour changes: identical verdict strings for over-ceiling, under-ceiling, compliant, unlisted-oversized and unreadable, verified case by case.
-
python_file_size_ratchet_test.pyinjects through the engine's seam rather than patching module attributes, and each rewritten test is shown to fail against a deliberately broken engine — a passing suite is not evidence here, given the failure mode above. - The duplication-guard pin is lowered by the consolidation, not left at its old figure.
- The shell gate's deliberate divergence survives:
autobot-infrastructure/stays in scope for.shand excluded for.py, with the test that pins it still passing.
Out of scope
Changing any ceiling, changing MAX_LINES, or splitting any grandfathered file. This is a structural consolidation with no behaviour change.
Filed from #17353, where the duplication was created knowingly rather than discovered.
Activity
guard(size): give .sh the size ratchet .py has had since #5060 (#17353)
guard(size): give .sh the size ratchet .py has had since #5060 (#17353)…
mrveiss commentedon Sep 24, 2026
Concrete evidence for this issue's "identical verdict strings for … unreadable, verified case by case" criterion — the two gates already disagree on that exact case today, so the consolidation has a behaviour conflict to resolve rather than a move-the-functions job.
scripts/check_shell_file_size.py:230-237reports an unreadable file throughunmeasured()and documents why skipping would be wrong:scripts/check_python_file_size.py:266-267does the opposite on the--audit-ceilingspath:No message, no
unmeasured()entry — the file leaves the population silently. (It does not inflatereached, so the reach floor still holds; the loss is that nothing reports it.)So when the engine is extracted, "neither gate's observable behaviour changes" is unsatisfiable as written for this case: one of the two must change. The shell treatment is the correct one — it is the same rule #15826 argues for, and the Python path is the outlier.
Suggest tightening the criterion to name the intended convergence explicitly, so the resolution is a recorded decision rather than whichever implementation the extraction happens to inherit.
Found while auditing guards for #17400 (reach coverage outside
repo_tests/); not filed separately, since this issue already owns the convergence. Read in the tree atbcd7c8dbea.test(gates): compare the two gates' wording by branch, not by one sen…
test(gates): drive audit_ceilings itself, and require ERROR not >=WAR…
fix(gates): the two file-size gates disagreed about how to report a f…
mrveiss commentedon Oct 3, 2026
Remainder after PR #17884, measured against merged
mainatcc2a4dce76#17884 merged as
370a34719bcarryingRefs, not a closing keyword, because it delivered the precursor and not this issue. Posting the remainder now rather than when someone next picks it up, with each criterion checked bygit show origin/main:<path>rather than against the branch.What landed: the two gates no longer disagree about how they report — findings to stderr at ERROR on both paths, a reach breach collected rather than suppressing the violations it explains, an unmeasured file reported once instead of twice or not at all, and the same
verdictwording across both. That matters here only because criterion 2 below asks for identical behaviour verified case by case, which cannot be verified between two gates that disagree about where output goes.The four criteria, and why each is still open
1 — One engine implements the ratchet; both gates are thin configuration over it. OPEN.
No shared module exists. Each gate still carries its own
_scan_tracked_files,verdict,_grandfathered_verdict,unmeasured,_vanished_entry_problem,audit_ceilings,run_auditandmain. Evidence: neither gate imports anything from the other or from a common engine —2 — The ratchet test injects through the engine's seam rather than patching module attributes. OPEN, and it is still the blocker this issue was filed to name.
repo_tests/python_file_size_ratchet_test.pycarries 12monkeypatch.setattr(hook, …)calls against module attributes, including the three the issue quotes:An engine resolving those names through itself would silently ignore every one of them, and the issue's warning stands: several of these tests would keep passing while asserting nothing. A monkeypatched
KNOWN_LARGE = {}that is ignored means the test exercises the real ceilings and still goes green.3 — The duplication-guard pin lowered by the consolidation. OPEN.
.github/workflows/duplication-guard.yml:96still readsMAX_DUP_LINES: '11707', and:118SLM_MAX_DUP_LINES: '2831'. Neither moved, which is correct — nothing was consolidated.4 — The shell gate's deliberate divergence survives. NOT YET APPLICABLE, and holding in the pre-consolidation state.
The divergence is intact today: shell
EXCLUDED_PREFIXES = (".worktrees/",)keepsautobot-infrastructure/in scope for.sh, while python excludes.worktrees/,autobot-infrastructure/andautobot-backend/code_analysis/. But the criterion asks whether it survives the consolidation, and there has been none — so this cannot be ticked, and recording it as "met" would be the kind of tick that makes a closed issue look finished. It is a precondition captured, not a criterion delivered.What #17884 added that the eventual consolidation should keep
Not criteria, but work that would be lost if the extraction starts from
origin/main's structure without reading it:_report_unmeasuredcarries the seen-versus-reached rationale in one place for both gates;_reach_breach_problemholds the reach-breach wording, identical modulo each gate's floor constant andSELF_REL; andrepo_tests/gate_reporting_parity_17377_test.pycompares the two gates' messages across all seven branches with every per-gate specific abstracted away — that file is the harness the consolidation needs, because it asserts "the two say the same thing" without pinning either text, so an engine serving both configurations passes it unchanged.Also fixed there and worth not re-breaking:
RATCHET_RELin the python gate named the test file, whoseRATCHET_BASELINEis a single re-export line, so the message told a developer to lower an entry in a file with no entries in it.fix(ci): the audit gate credited any package's fixAvailable to every …
mrveiss commentedon Oct 9, 2026
Collision warning: this branch's current state would reverse a fix on #18130
issue-17377-one-enginealready carries the consolidation — three commits, unlanded, no PR:Do not land it as-is. Measured against that branch's tip:
def configure_loggingscripts/check_python_file_size.pyscripts/check_shell_file_size.pyCommit
17012e7132on #18130 removed those two private copies (and a third) and pointed all three gates attools.lint._scan_helpers.configure_logging, which has 13 importers. Merging this branch at its current tip reintroduces two of them.It will not do so silently, which is the one piece of good news: the same commit widened
repo_tests/no_private_copy_of_a_shared_helper_test.pyto sweepscripts/*.py— it previously swept onlytools/lint/*.pyandpipeline-scripts/*.py, which is exactly why those three copies survived the #13916 consolidation. So a rebase that re-adds a local copy fails that guard by file and name rather than merging quietly.Overlapping files between this branch and #18130:
scripts/check_python_file_size.pyscripts/check_shell_file_size.pycheck_frontend_file_size.py→check_typescript_file_size.py, and refactor(forks): retire fifteen forks across eight issues, each onto one canonical home (#18121) #18130 modifies the old path.Per the one-PR-per-file rule these should not land in parallel.
Suggested order. #18130 is open with CI running and 14 commits, so it goes first; then this branch rebases onto the merged tree, and the one-engine refactor keeps the canonical import instead of re-deriving
configure_logginginside the engine. If the engine owns logging setup itself that is arguably better still — but it should then be the engine importing the canonical, not each configuration restating it.Two notes from having just worked in these files:
configure_loggingtrio was one measured instance of it: byte-identical bodies (comparingast.unparsewith docstrings stripped gives one distinct body across all three), differing only in docstring wording and one em-dash — which is why a text search for the copy missed one.typescript_file_size_ratchet_baseline.py), the pin parity check acrossrepo_tests/python_file_size_ratchet_baseline.pyandscripts/python_file_size_known_large.pyhas to keep holding — they are currently in parity at 479 entries, and I have been relying on that invariant all session.