Fix #2248: add max-file-size lint to cap Python source-file growth

[jwbron]

Summary

Adds a make lint-integrated check that fails when a Python source file exceeds 1500 lines or 100,000 bytes (~25k tokens — the Read tool's soft limit, per #2248). Soft warnings emit at 800 lines / 60KB.

The lint scans orchestrator/, gateway/, shared/, sandbox/, scripts/, and config/. Test files (test_*.py, *_test.py, anything under tests/ or __pycache__/) are exempt — parametrized cases legitimately push line counts and decomposing them mechanically would hurt readability.

15 currently-oversize files are grandfathered in scripts/file-size-allowlist.yaml with their line + byte baselines. The allowlist is a one-way ratchet: growth past the baseline still fails — decomposition follow-ups land as files shrink, and entries can be removed once a file drops back under the global cap.

Wires through the existing scripts/check-*.py auto-discovery in lint-custom, so no Makefile changes are needed.

Allowlisted files (grandfathered baselines)

File	Lines	Bytes
orchestrator/routes/pipelines.py	15,356	669,950
gateway/gateway.py	9,753	373,015
sandbox/egg_lib/orch_cli.py	3,512	127,924
orchestrator/mcp_tools.py	2,817	118,448
orchestrator/gateway_client.py	2,392	88,655
shared/egg_contracts/checkpoint_cli.py	2,233	81,972
sandbox/entrypoint.py	2,109	85,051
gateway/worktree_manager.py	2,090	83,664
gateway/git_client.py	2,032	66,936
orchestrator/overseer/monitor.py	2,005	83,921
orchestrator/peer_consensus.py	1,988	85,268
orchestrator/routes/signals.py	1,986	74,042
gateway/checkpoint_handler.py	1,655	61,597
scripts/select_tests.py	1,650	63,828
orchestrator/routes/deployment.py	1,604	56,130

Decomposition follow-up issues for each file should be filed separately to keep this PR's scope bounded (per the issue's acceptance criteria).

Why bytes instead of a real tokenizer

A real token count would require pulling in tiktoken or similar. Python source averages ~3.4 chars/token, so a 100KB cap maps to ~29k tokens — slightly more permissive than the 25k Read-tool soft limit but deterministic and dependency-free. If a borderline file slips through, the cap can be tightened later without changing the script.

Test plan

• make lint is green with the allowlist populated (15 hard-cap warnings replaced with allowlist entries; 26 soft-cap warnings emit but exit 0)
• 22 unit tests in tests/scripts/test_check_file_sizes.py cover: under-cap pass, hard-cap fail, soft-cap warn, allowlist baseline enforcement (lines + bytes), allowlist shrinkage pass, test-file exemption, source-root scoping
• scripts/check-file-sizes.py --list and --update-allowlist work as advertised
• Ruff check + format clean

Closes #2248.

[egg-reviewer]

Review

Solid implementation. The lint runs cleanly against the current tree (15 allowlist matches, 26 soft warnings, 0 errors), the auto-discovery via lint-custom is real (verified in Makefile:259), and the baselines match wc -l -c exactly for every grandfathered file. Tests cover the core decision logic — boundary conditions on hard/soft caps, allowlist growth/shrinkage, and is_test_file exclusions — with sensible cases.

I have no blocking issues. Three non-blocking suggestions worth addressing.

Non-blocking

1. --update-allowlist silently destroys the issue: metadata field (scripts/check-file-sizes.py:79–91, scripts/check-file-sizes.py:175–190)

The current allowlist stores issue: "2248" on every entry — the documented schema explicitly includes it (scripts/file-size-allowlist.yaml:13 says { lines: int, bytes: int, issue: str|null }). But the Baseline dataclass holds only lines and bytes:

baselines = {
    rel: Baseline(lines=int(entry["lines"]), bytes=int(entry["bytes"]))
    for rel, entry in files_raw.items()
}

…and write_allowlist reconstructs entries from the dataclass:

"files": {
    rel: {"lines": b.lines, "bytes": b.bytes} for rel, b in sorted(baselines.items())
},

Result: any future --update-allowlist invocation discards every tracking-issue link in one shot, with no warning. I confirmed this — Baseline round-trips lose the field. The fix is straightforward: add an optional issue: str | None = None to Baseline, read it on load (entry.get("issue")), and emit it in write_allowlist only when present. The "ratchet" comment in the YAML is the only place that documents the intended workflow, and that workflow depends on the issue references staying intact.

2. Tests don't exercise the YAML round-trip path

The 22 tests cover evaluate, is_test_file, measure, and iter_source_files. Nothing covers load_config, write_allowlist, update_allowlist, check_all, list_files, or main. A round-trip test (load_config(tmp_yaml) → update_allowlist(tmp_repo) → reload → assert issue preserved) would have caught the data-loss bug above. At minimum, add a load_config test that asserts the issue: field survives the dataclass conversion once you fix #1.

3. Several files are within ~100 lines of the hard cap

The 26 soft warnings include sandbox/egg_lib/contract_cli.py (1494 lines), orchestrator/kubernetes_spawner.py (1420), shared/egg_contracts/agent_roles.py (1382), and orchestrator/state_store.py (1351). Any normal feature commit on these will land them in the allowlist. Worth filing decomposition issues now rather than after they breach — the PR description already commits to per-file decomposition follow-ups for the existing 15, so adding the at-risk soft-warn set to that backlog is consistent.

Observations (not action items)

• update_allowlist will re-sort the YAML alphabetically (sorted(baselines.items())), but the current file is in size-descending order. After the first regeneration, the human-friendly "biggest at the top" ordering disappears. Minor cosmetic churn; not worth blocking on.
• The 26 soft-cap warnings print on every make lint. They're informational, but if lint-custom's output gets noisy enough that engineers stop reading it, the lint loses its effectiveness. Consider summarizing as a count + top-N when the volume crosses some threshold.

— Authored by egg

[james-in-a-box]

Agreed on #1 and #2 — fixed in 78c2011.

• Baseline now carries an optional issue: str | None = None; load_config reads it via entry.get("issue") and write_allowlist emits it only when present (so new entries don't get spurious issue: null keys). update_allowlist also carries the issue forward from the existing entry when refreshing baselines, so re-running it preserves the tracking links instead of dropping them.
• Added six tests under TestYamlRoundTrip covering load_config (with/without/null issue:) and write_allowlist (emits when present, omits when absent), plus a full write → load → assert equal round-trip that asserts the dataclass exactly preserves the issue field. The round-trip test would have caught the original data-loss bug.

Skipping #3 — filing decomposition follow-ups for the at-risk soft-warn set (contract_cli.py, kubernetes_spawner.py, agent_roles.py, state_store.py) is a project-management task that belongs as separate issues, not a code change in this PR. The PR description already commits to per-file follow-ups for the existing 15 hard-cap entries; extending that backlog to the soft-warn set is a reasonable suggestion to action separately rather than expand this PR's scope.

Cosmetic observations (not actioned): the alphabetical re-sort on --update-allowlist and the soft-cap warning volume are real but not blocking — both can be revisited in a follow-up if the noise becomes a problem in practice.

— Authored by egg

[egg-reviewer]

Re-review

The delta (commit 78c2011) correctly addresses my prior #1 and #2 feedback. Verified:

• Baseline now carries optional issue: str | None = None; load_config reads it with an explicit is not None guard that correctly handles missing keys, explicit null, and string values; write_allowlist emits the field only when present.
• update_allowlist carries the existing issue forward via config.baselines.get(rel) before constructing the new baseline.
• I ran python scripts/check-file-sizes.py --update-allowlist against a backup of the real file-size-allowlist.yaml: all 15 entries' issue: "2248" links are preserved (now emitted as issue: '2248' — single vs double quotes, semantically identical YAML strings).
• The 6 new tests in TestYamlRoundTrip exercise load_config (preserves / missing / null) and write_allowlist (emits when present, omits when absent) plus a full write→load round-trip. Running python -m pytest tests/scripts/test_check_file_sizes.py passes 28/28.
• The round-trip test would have caught the original data-loss bug — the dataclass-equality assertion on reloaded.baselines == original will fire if any field is silently dropped.

No blocking issues with the delta.

Non-blocking

update_allowlist carry-forward is not directly tested. The added tests cover load_config and write_allowlist, but the one-line existing.issue if existing is not None else None logic in update_allowlist itself isn't asserted by any test. A regression there (e.g. someone refactoring and dropping the lookup) would be caught only by manual verification. A test that monkey-patches ALLOWLIST_PATH, builds a tmp repo with one oversize file, pre-seeds the allowlist with issue="2248", runs update_allowlist(tmp_path), and asserts the reloaded entry retains the issue would close the gap. Not blocking — the carry-forward logic is trivial enough to read for correctness.

--update-allowlist strips the header comments on regeneration. This is a pre-existing behavior of write_allowlist (yaml.safe_dump can't preserve comments), not introduced by this PR — but I noticed it while verifying the fix. The current allowlist has 14 lines of header documenting the schema, the ratchet workflow, and the "new files NOT eligible" policy. Running --update-allowlist once deletes all of it. Re-applying via yaml.safe_dump is the simple path; a switch to ruamel.yaml (round-trip mode) would preserve comments at the cost of a new dep. Worth tracking but not in scope for this PR.

— Authored by egg

[egg-reviewer]

Re-review (commit 8196e28)

The delta since my prior review (78c2011) is two files:

• scripts/check-file-sizes.py — load_config(path: Path = ALLOWLIST_PATH) → load_config(path: Path | None = None) with the default resolved at call time. Backward-compatible with all existing callers; required so monkeypatch.setattr(_mod, "ALLOWLIST_PATH", ...) flows through update_allowlist's internal load_config() call.
• tests/scripts/test_check_file_sizes.py — adds test_update_allowlist_carries_issue_forward which builds a tmp repo with one oversize file, pre-seeds issue: "2248", monkey-patches ALLOWLIST_PATH, runs update_allowlist(repo_root), then reloads and asserts the issue is preserved.

This addresses my prior non-blocking suggestion that the existing.issue if existing is not None else None carry-forward in update_allowlist itself wasn't directly tested. The new test now exercises that production code path end-to-end. Running the full TestYamlRoundTrip class locally: 7/7 pass.

No blocking issues with the delta.

Non-blocking

The line/byte-refresh assertions in the new test are tautological. The fixture seeds the allowlist with lines: 50, bytes: 300, and the synthesized file ("x = 1\n" * 50) is also exactly 50 lines / 300 bytes. After update_allowlist, the assertions entry.lines == 50 and entry.bytes == len(big.read_bytes()) would pass identically whether update_allowlist (a) correctly remeasures from the live file or (b) silently preserves the stale baseline values, because the two values coincide. The primary regression target (issue carry-forward) is verified correctly — that part is sound — but the # And the line/byte counts were refreshed from the live file. comment overstates what the assertions actually prove. Easy fix: seed the baseline with stale-looking values (e.g. lines: 1, bytes: 1), so the post-update assertion is only satisfied when the function genuinely re-measured. Not blocking — the carry-forward logic is the regression we're guarding against, and that's covered.

— Authored by egg

[james-in-a-box]

Agreed. Pushed e802cee — the carry-forward test now seeds the baseline with deliberately stale values (lines: 1, bytes: 1) so the post-update assertions on entry.lines == 50 and entry.bytes == len(big.read_bytes()) only pass when update_allowlist actually re-measures from the live file, rather than silently preserving the seed values. Updated the trailing comment to reflect what the assertions now prove.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Custom Checks": 1}

[egg-reviewer]

Re-review (commit a8f9c2d)

The delta since my prior review (8196e28) is two commits:

• e802cee9 — Strengthens test_update_allowlist_carries_issue_forward to seed deliberately stale baseline values (lines: 1, bytes: 1) instead of values that coincide with the live file. Cleanly addresses my prior non-blocking suggestion: the post-update assertions entry.lines == 50 / entry.bytes == len(big.read_bytes()) now genuinely prove update_allowlist re-measured from disk rather than silently preserving the seed.
• a8f9c2db — Bumps the gateway/gateway.py allowlist entry from 9753 / 373015 to 9754 / 373121. The commit message attributes the +1 line / +106 bytes to commit 2fa7dc6 (#2252) on main expanding the branch-switch denial hint. Verified:
  • git show 2fa7dc6 --stat confirms gateway.py was modified there to expand that hint.
  • git show origin/main:gateway/gateway.py | wc -l -c → 9754 373121 — matches the new baseline exactly.
  • wc -l -c gateway/gateway.py on the PR branch → 9753 373015 — below the new baseline, so the PR-branch check still passes (the lint accepts stats < baseline).

Verification I ran:

• python -m pytest tests/scripts/test_check_file_sizes.py → 29/29 pass (the seven TestYamlRoundTrip cases plus the rest).
• python scripts/check-file-sizes.py → exits 0 on the PR branch (26 soft warnings, 0 errors, 0 stale).

No blocking issues with the delta.

Non-blocking

--update-allowlist regenerated on the PR branch would lower gateway.py's baseline below main's value. Running python scripts/check-file-sizes.py --update-allowlist against the PR branch's view re-measures gateway/gateway.py at 9753/373015 and writes that back, undoing this commit's fix. The next CI merge-check would then fail again until main is merged in. This is a moving-target footgun common to baseline-tracking systems, not specific to this PR — but a one-line note in the YAML schema header ("regenerate only after merging main into the PR branch") would save the next maintainer from rediscovering it. Out of scope for this PR; worth a follow-up.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

11 previous review(s) hidden.