Repository navigation
tech-debt(ci): 3 of 4 commit-blocking pre-commit checkers have no tests + 2 live violations they already detect #13200
Description
Discovered while adding the no-open-without-encoding gate (#13151 / PR #13153).
1. Three commit-blocking gates have no tests
.pre-commit-config.yaml wires four local Python checkers. Only one has a test suite:
| Checker | Lines | Tests |
|---|---|---|
pipeline-scripts/check_env_var_registry.py |
217 | none |
pipeline-scripts/check_i18n_plural_third_arg.py |
217 | none |
pipeline-scripts/check_no_literal_ttl_seconds.py |
157 | none |
pipeline-scripts/check_open_encoding.py |
105 | check_open_encoding_test.py (18 cases) |
These block every commit that touches a matching file. A false positive in any of them stops all work repo-wide until someone edits the checker, and there is no test to catch one before it ships.
This is not hypothetical. While building the fourth checker, two false-positive classes only surfaced from running it against real code:
tarfile.open(path, "r:gz")— the mode contains nob, so a naive mode check passes it straight through. Eight call sites inbackup_manager.pywould have been flagged.PILImage.open(...),os.open(...),zf.open(...)— matching any attribute call named.openpulled in image decoding, file descriptors and zip members. This inflated the reported violation count from 3 to 21 before it was caught.
Both are now pinned by negative test cases. The other three checkers have never had that exercise.
2. Two live violations those gates already detect but nobody sees
pre-commit only inspects changed files, so a violation in a file nobody touches stays invisible. Running the existing checkers across the tree surfaces two real ones:
autobot_shared/rate_limiter.py:252 — literal TTL:
# Expire the whole key after 2 hours so stale keys auto-clean
pipe.expire(hour_key, 7200)CLAUDE.md requires TTLs to come from a named constant. autobot_shared/ssot_constants.py exists but currently defines no 7200/2-hour value, so the constant needs adding as part of the fix.
autobot-backend/code_embedding_generator.py:35 — unregistered env var:
OPENVINO_CACHE_DIR = os.getenv("AUTOBOT_OPENVINO_CACHE_DIR", "data/openvino_cache")AUTOBOT_OPENVINO_CACHE_DIR does not appear in autobot_shared/env_registry.py (0 matches).
Same drift shape as #3391 → #13151 and #4945 → #13149: a correct gate, plus legacy sites it never inspects.
3. Consolidation opportunity
The four checkers duplicate the same scaffolding: walk files from argv, parse, report path:line: message, return 1 on any finding. Three share the exact main(argv: list[str]) -> int + sys.exit(main(sys.argv[1:])) contract; the fourth adds argparse on top — a small inconsistency introduced by #13153 that should be reconciled either way.
Shell hooks already have lib/_common.sh (#7203). The Python checkers have no equivalent.
A small shared harness — file iteration, AST parse with graceful skip on syntax errors, finding accumulation, consistent output and exit code — would remove that duplication and, more usefully, give one place to test the scaffolding so each checker only has to test its own predicate.
Suggested decomposition
- Task 1: fix the two live violations (add the TTL constant + register the env var)
- Task 2: shared harness in
pipeline-scripts/+ migrate the four checkers onto it, reconciling the CLI contract - Task 3: test suites for the three untested checkers, each covering its known false-positive shapes
- Task 4: decide whether the advisory
pre-commit run --all-filesstep inenforce-precommit.ymlshould become blocking once the tree is clean — today it endsexit 0, so legacy violations never surface
Acceptance
- Every local checker in
.pre-commit-config.yamlhas a test suite with negative cases. - The two violations above are fixed.
- Running all checkers across the tree reports zero.
Activity
mrveiss commentedon Aug 25, 2026
Pickup — PR #15048, base SHA
dc39243680Worked with #13543, #13286 and #10691 as one defect class. Re-measuring first, because parts
of this issue have been overtaken since it was filed.
Already fixed in the tree (verified, not assumed)
autobot_shared/rate_limiter.py:253now readspipe.expire(hour_key, TTL_2_HOURS).AUTOBOT_OPENVINO_CACHE_DIRis present inautobot_shared/env_registry.py:1119.enforce-precommit.ymlno longer endsexit 0:Fail when a hook could not run at all (#14181)andFail when a behavioural hook reported a finding (#14878)bothgate.
check_no_literal_ttl_seconds.pyandcheck_env_var_registry.pyboth gained suites(
check_no_literal_ttl_seconds_test.py,check_env_var_registry_helpers_test.py— thelatter drives
main()despite its name).Third acceptance criterion — verified now
All four checkers run across the whole tree, not just changed files, report zero:
check_i18n_plural_third_arg.py.ts/.vueoutsidesrc/i18n/rc=0check_env_var_registry.py.pyrc=0check_no_literal_ttl_seconds.py.pyrc=0check_open_encoding.py.pyrc=0What PR #15048 adds
pipeline-scripts/check_i18n_plural_third_arg_test.py— 20 cases driving the realchecker, asserting the non-zero exit (including through
subprocess, the way pre-commitinvokes it), plus the false-positive shapes this class of checker only learns from real code:
a comma nested inside the second argument, a nested call, a
)inside a string literal, a keythat only looks plural.
It also closed a fail-open found while writing them:
_load_plural_keys()returnedset()onany error and
main()turned that intoreturn 0. An unreadableen.jsonmade the hookpass every file it was handed while still reporting success — the emptied-allowlist shape.
It now raises
PluralKeysUnavailableand the hook blocks, the same directioncheck-jsoninthe same config already takes.
TestTheKeySetIsNotSilentlyEmptyis the oracle: it asserts therepository really does yield plural keys, so the other 19 cases cannot pass vacuously.
Mutation-proved:
_has_third_argforced to always report a third arg → 7 cases fail; thefail-open restored → 2 fail; every plural call reported regardless → 3 false-positive cases
fail. All restored; none pushed.
Remaining — this issue is not closed by that PR
.pre-commit-config.yamlnaming a Python entry point, 25 are now named by a test suite. The last one is
tools/lint/check_spdx_header.py, which runs with--fixand therefore rewrites sourcefiles during a commit with no test at all — filed as test(precommit): check_spdx_header.py rewrites source on every commit and has no test suite #15049.
pipeline-scripts/checkers, reconciling theargparse-vs-bare-mainCLI split) is untouched.ci(precommit): exempt the checker's own stderr report from the no-pri…
ci(gates): make four gates that could not fail actually fail (#13543, #…
fix(lint): floor every full-repo sweep and gate git ls-files (#14896, #…
mrveiss commentedon Sep 20, 2026
Branch-freeze re-verification against current
origin/main(2026-09-20). Tracked branchissue-13543-gates-that-cannot-failis already gone from origin.All 3 named acceptance items have real progress; the third is only fully confirmed for one of three checkers:
pipeline-scripts/check_env_var_registry_test.py,check_i18n_plural_third_arg_test.py,check_no_literal_ttl_seconds_test.pyall exist alongside the pre-existingcheck_open_encoding_test.py.autobot_shared/rate_limiter.py:253now uses a namedTTL_2_HOURSconstant instead of the literal7200;AUTOBOT_OPENVINO_CACHE_DIRis registered — not inenv_registry.pydirectly (that file appears to have been split since this issue was filed), but inautobot_shared/env_registry_ai.py, confirmed present.check_env_var_registry.pyruns directly and blockingly inside the requiredcode-qualityworkflow (code-quality.yml:517,exit 1on finding) — that's a real, continuous tree-wide assertion, andcode-qualityhas been passing on recent runs, so this one is confirmed. I did not find an equivalent tree-wide CI invocation forcheck_i18n_plural_third_arg.pyorcheck_no_literal_ttl_seconds.py— pre-commit only ever inspects changed files (the exact limitation the issue itself named), so a legacy violation in an untouched file could still be invisible for those two specifically.Not checked (the issue's "Suggested decomposition" tasks, explicitly labeled suggested rather than required): the shared harness (Task 2) doesn't exist as a file;
enforce-precommit.yml'spre-commit run --all-filesstep (Task 4) still routes to|| true/ a warning rather than blocking, unchanged.2 of 3 core items fully done, the third partially confirmed. Leaving open for the two untested-tree-wide checkers.