Repository navigation
guard(size): give .sh the size ratchet .py has had since #5060 (#17353) - #17378
Merged
Merged
Conversation
Shell had no size gate of any kind -- no checker, no baseline, no ratchet test. Python has had one since #5060 and a ratchet since #14236, so a .py file cannot grow past a recorded ceiling. Meanwhile scripts/lib/hardcoded-value-rules.sh -- the rule set BOTH the repo-wide scan and the pre-commit hook read -- went 787 to 842 lines in one PR with nothing to say about it. The gate that can say no shapes the change; the one that does not exist does not. Measured first: 216 tracked .sh files, 10 over the 600-line ceiling. Not the 509 the Python gate had to grandfather, which is why the ceilings sit inline rather than in a sibling data module. SCOPE IS A DELIBERATE DIVERGENCE, and it is the decision in this change. check_python_file_size.py excludes autobot-infrastructure/. Inheriting that here would drop 6 of the 10 oversized files -- including four of the five that motivated the issue -- and the gate would ship green having checked almost nothing, which is the vacuous-gate failure MEASUREMENT_DISCIPLINE.md names. Python can afford that exclusion because little Python lives there. Shell cannot: infrastructure is WHERE SHELL LIVES, and deployment scripts are the code with the least type checking and the most operational consequence. So this gate excludes .worktrees/ only, and a test pins that divergence so a later "align the two gates" change fails and reads why. Same three-way ratchet as the Python gate, including the branch that looks wrong: a file UNDER its ceiling fails until the ceiling is lowered, because an unlowered ceiling re-licenses the lines just cut. Ceilings are kept in two copies that a test holds equal (#14498) -- the gap between a lowered hook and an unlowered mirror is exactly those lines, and is spendable. An unreadable file is a violation, not a skip: None from count_lines means "never measured", which is a different claim from "within the limit", and only the second is what exit 0 may report. NOT extracted into a shared engine with the Python gate, deliberately, though ~90% of the logic is common. repo_tests/python_file_size_ratchet_test.py monkeypatches hook.KNOWN_LARGE, hook.repo_root and hook.tracked_python_files as MODULE ATTRIBUTES; an engine taking a config object would stop honouring those patches and the tests would keep passing while asserting nothing. Consolidating means rewriting that 590-line test's patching strategy, which is a bigger and riskier change than this issue asks for. Filed as #17377. Wired into code-quality.yml beside the Python audit, so an entry whose file shrank, moved or was deleted is caught repo-wide rather than waiting for somebody to stage it.
Contributor
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…files it pins (#17353) Review found the gate shipping green while not running in the case it exists for -- the vacuous-gate failure this PR is supposed to prevent, in the PR that adds it. `--audit-ceilings` lives in the `code-quality` job, which is gated on `needs.changes.outputs.backend`. That filter carries a generic `'**/*.py'` and had NO generic `.sh` entry -- only three specific shell prefixes. Measured against the ten grandfathered paths, four fell outside every pattern: install.sh (1265) scripts/pr-preflight.sh (818) autobot-slm-backend/ansible/deploy.sh (718) autobot-infrastructure/autobot-slm-backend/scripts/bootstrap-slm.sh (652) So a PR editing ONLY one of those skipped `code-quality` entirely and the audit never ran. The well-behaved case hid it: lowering a ceiling touches `repo_tests/**`, which IS in the filter, so a correct change self-triggers the job. The misbehaving case -- shrink the file, forget the ceiling -- is the one that skipped. This PR's own run hid it too, for the same reason. Fixed by adding `'**/*.sh'` beside `'**/*.py'`, in the canonical filter and in code-quality.yml's `on.push.paths` copy, which `check_workflow_path_filters.py` polices for exactly this drift. Re-measured: 0 of 10 uncovered. Two more from the same review: * the exclude-mirror test asserted CONTAINMENT, not equality, so widening the yaml to `^(\.worktrees/|autobot-infrastructure/)` would still have passed -- '.worktrees/' is a substring of it. That widening is the "align the two gates" mistake the neighbouring test exists to refuse, and it would have silently stopped pre-commit gating infrastructure shell files. Now set equality, with the alternation parsed rather than string-matched. * the tree walk `continue`d past an unreadable file, contradicting this module's own docstring that an unmeasured file is a violation and not a skip. It now reports one, and still does not count it toward `reached`, so it cannot prop up the floor check without having been ruled on. Plus a boundary test pinning that an unlisted file at exactly MAX_LINES passes and 601 fails: a gate that trips AT the limit and one that trips past it read identically in review.
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
1 participant
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.
Thinking Path
Shell had no size gate of any kind — no checker, no baseline, no ratchet
test. Python has had one since #5060 and a ratchet since #14236, so a
.pyfile cannot grow past a recorded ceiling.
The asymmetry is not theoretical.
scripts/lib/hardcoded-value-rules.sh— therule set both the repo-wide scan and the pre-commit hook read — went 787 →
842 lines in a single PR with nothing to say about it. The gate that can say no
shapes the change; the one that does not exist does not.
Measured before building, because the size of the grandfathering problem
determines the design:
Ten entries, not 509 — which is why the ceilings sit inline rather than in a
sibling data module.
python_file_size_known_large.pyexists only because 512entries would have pushed that hook over its own limit.
The decision in this change: scope
check_python_file_size.pyexcludesautobot-infrastructure/. Inheritingthat exclusion would drop 6 of the 10 oversized shell files, including four of
the five the issue opens with. The gate would ship, pass, and gate almost
nothing — the vacuous-gate failure
MEASUREMENT_DISCIPLINE.mdnames.That exclusion is defensible for Python (little Python lives there). It is
indefensible for shell: infrastructure is where shell lives, and deployment
scripts are the code with the least type checking and the most operational
consequence. So this gate excludes
.worktrees/only.The divergence is pinned by a test (
test_the_infrastructure_tree_is_in_scope),so a future "align the two gates" change fails and reads why rather than
silently halving the gate's reach.
What Changed
scripts/check_shell_file_size.py--audit-ceilingstree walkrepo_tests/shell_file_size_ratchet_baseline.pyrepo_tests/shell_file_size_ratchet_test.py.pre-commit-config.yaml.github/workflows/code-quality.ymlSemantics mirror the Python gate, including the branch that looks wrong: a
file under its ceiling fails until the ceiling is lowered, because an
unlowered ceiling re-licenses the lines just cut. Two copies of the ceilings are
held equal by a test (#14498) — the gap between a lowered hook and an unlowered
mirror is exactly those lines, and it is spendable.
An unreadable file is a violation, not a skip:
Nonefromcount_linesmeans never measured, which is a different claim from within the limit, and
only the second is what exit 0 is entitled to report.
Deliberately not done: the shared engine
~90% of the logic is common with the Python gate, and consolidating is the
repo's stated goal. I did not do it here, and the reason is a trap worth
recording —
repo_tests/python_file_size_ratchet_test.pypatches moduleattributes:
An engine taking a config object resolves those names through itself, so every
patch would stop taking effect — and the failure mode is the dangerous one:
tests keep passing while asserting nothing (a patched
KNOWN_LARGE = {}that isignored means the test silently exercises the real ceilings). Consolidating
means rewriting that 590-line test's injection strategy and proving the
rewritten tests still fail when they should.
Filed as #17377 with that hazard written down, rather than done badly here.
Verification
Pre-push hook — hook-registered runs, not ad-hoc local ones. The first attempt
failed and blocked the push, which is the evidence that the selection is
real:
The test derived the repo root from
__file__instead ofrepo_tests._paths.repo_root()(#15925), which answers wrongly under a nestedcheckout. Fixed, then re-pushed.
CI is the authority on the gate's own behaviour: it runs
--audit-ceilingsrepo-wide, which is the first time this walk executes anywhere.
Second commit: the gate did not run for 4 of the 10 files it pins
A
code-reviewerpass found this PR doing the thing it exists to prevent.--audit-ceilingslives in thecode-qualityjob, gated onneeds.changes.outputs.backend. That filter carries a generic'**/*.py'andhad no generic
.shentry — only three specific shell prefixes. Measuredagainst the ten grandfathered paths, four fell outside every pattern:
install.shscripts/pr-preflight.shautobot-slm-backend/ansible/deploy.shautobot-infrastructure/autobot-slm-backend/scripts/bootstrap-slm.shA PR editing only one of those skipped
code-qualityentirely, so the auditnever ran.
The well-behaved case hid it. Lowering a ceiling touches
repo_tests/**,which is in the filter, so a correct change self-triggers the job. The
misbehaving case — shrink the file, forget to lower the ceiling — is the one
that skipped. This PR's own CI hid it for the same reason: its diff touches
repo_tests/**and.pre-commit-config.yaml, socode-qualityran here andproved nothing about the scenario above.
Fixed by adding
'**/*.sh'beside'**/*.py'in the canonical filter and incode-quality.yml'son.push.pathscopy, whichpipeline-scripts/check_workflow_path_filters.pypolices for precisely thisdrift. Re-measured against all ten: 0 uncovered.
Three more from the same review
^(\.worktrees/|autobot-infrastructure/)still passed, because.worktrees/is a substring of it — the exact "align the two gates" mistake the neighbouring test exists to refuse, and it would have silently stopped pre-commit gating infrastructure shell filescontinued past an unreadable filereached, so it cannot prop up the floor check without being ruled onMAX_LINESAcceptance criteria
#17353 asks three things, conditional on a decision. The owner decided shell
is gated, so all three apply:
.shsize checker with the same ceiling semanticsscripts/check_shell_file_size.py,MAX_LINES = 600, same three-wayverdict()KNOWN_LARGE, 10 entries at their measured sizes; no file is asked to shrinkrepo_tests/shell_file_size_ratchet_test.py, incl. the under-ceiling branchThe issue's alternative — "if shell should not be gated, say so where a reader
will find it" — does not apply, since the decision was to gate.
Closes #17353
Model Used
Claude Opus 5 (1M context).