file-size lint: drop per-file baselines from allowlist

[jwbron]

Summary

• Drop lines: and bytes: baselines from each entry in scripts/file-size-allowlist.yaml. Membership in the allowlist alone now exempts a file from the global cap; allowlisted files may grow freely.
• Simplify scripts/check-file-sizes.py to match. load_config tolerates legacy lines:/bytes: keys so branches that haven't rebased still load, but write_allowlist drops them on output. --update-allowlist now syncs membership (add over-cap files, drop now-under-cap ones), carrying the issue: link forward.

Why

Every PR that touches an allowlisted file had to bump that file's baseline numbers in the YAML. Every such bump conflicted with every other concurrent PR touching the same file. Six open PRs hit this same conflict at once today.

The baselines were intended to prevent further growth of grandfathered files, but the merge-conflict tax across cross-PR rebases outweighed the regression-prevention value. Decomposition for these files is tracked under #2248; size regressions on them are a known accepted state until that work lands.

Test plan

• make lint-custom passes (scripts/check-file-sizes.py exits 0, soft-cap warnings only)
• pytest tests/scripts/test_check_file_sizes.py — 31 passed (including new tests for legacy-format loading, round-trip issue preservation, and update-allowlist add/drop)
• ruff check and ruff format --check clean
• mypy clean
• yamllint clean

[egg-reviewer]

Review: file-size lint baseline removal

I reviewed the diff in full, traced the lint's call sites (make lint-custom → scripts/check-file-sizes.py, plus .github/workflows/lint.yml), and verified the YAML serialization round-trip behavior. No blocking issues. The PR is small, focused, well-tested, and correctly preserves backward compatibility. A few non-blocking observations follow.

Correctness

• load_config correctly tolerates the four practical YAML shapes for an entry: null, {}, {lines: X, bytes: Y} (legacy), {issue: "N"}, and the legacy combo {lines: X, bytes: Y, issue: "N"}. The isinstance(entry, dict) guard plus entry.get("issue") handles all of them. Verified against the new tests.
• evaluate short-circuits correctly: over-hard-cap + allowlisted → silent pass; over-hard-cap + not allowlisted → error; under-hard-cap + allowlisted → silent pass (no soft warning); under-hard-cap + not allowlisted → soft warnings still produced. Behavior matches the docstring and the YAML preamble.
• update_allowlist is correctly idempotent on no underlying change: it walks live source files, adds those over cap with the existing issue link carried forward (or None), and the YAML output sorts keys deterministically. Files that have shrunk under the cap or no longer exist are dropped — consistent with the new docstring.
• The new gate (membership-only) preserves the only thing this lint had to enforce: net-new oversize files cannot land without an explicit allowlist entry. That's intact.

Backward-compatibility risk

The PR description's claim that legacy YAML loads cleanly is correct. test_load_ignores_legacy_lines_bytes covers the case directly. Importantly, branches that haven't rebased onto this schema lose the strictness of the old check (their bumped baseline becomes irrelevant), but they don't lose any correctness — the lint still gates net-new oversize files. This is fine and exactly what the PR is trying to achieve.

Tests

The new tests exercise the production code path (no self-seeding goldens, no hand-built fixtures bypassing helpers). Coverage now includes:

• test_allowlisted_growth_is_allowed — replaces the old growth-fails tests.
• test_load_ignores_legacy_lines_bytes — protects the rebase scenario explicitly.
• test_update_allowlist_adds_new_over_cap_file and test_update_allowlist_drops_now_under_cap_file — exercise the new add/drop behavior end-to-end via a real temp filesystem.
• test_round_trip_preserves_issue — guards against silently dropping issue links on --update-allowlist.

The previous test_update_allowlist_carries_issue_forward was tightened to additionally assert that legacy lines:/bytes: keys are not re-emitted. Good.

Non-blocking suggestions

1. update_allowlist print is uninformative. It says Wrote N entries to file-size-allowlist.yaml with no signal about what changed. Since the operation now performs both add and drop, a brief diff would help operators (e.g., Added: a.py, b.py; Dropped: c.py). Two-line change.

2. Allowlist entries for files that drop under the cap stay silently listed. The new evaluate() short-circuits for allowlisted files that are under the cap, and check_all's stale-detection only fires for entries pointing to nonexistent files. So a file that was decomposed under the cap but kept in the allowlist passes silently forever, and the operator only learns the entry is unnecessary by running --update-allowlist manually. Not a regression — old code had the same gap — but the YAML preamble and docstring both instruct removal "when they drop back under the global cap," and the lint provides no nudge for that. A note: line in check_all's output (parallel to stale) would close the loop. Out of scope for this PR.

3. load_config silently ignores unknown keys. A typo like issues: (instead of issue:) on a hand-edited entry would silently lose the tracking link. Logging a warning when an entry dict has unknown keys would make this safe to hand-edit. Out of scope, but worth a follow-up if you ever see it bite.

4. Old baselines were already loose. Pre-existing observation, not this PR's problem: the previous baselines (e.g. orchestrator/routes/pipelines.py: lines: 15593) were 10x the hard cap, so the per-file regression check wasn't catching meaningful drift in practice. The PR's framing — "the merge-conflict tax outweighed the regression-prevention value" — understates how little prevention there actually was. Net: removing them is the right call, and the upside (no more cross-PR baseline conflicts) is the real win.

LGTM.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs