ci: gate repository root against unbounded file growth - #32529
ci: gate repository root against unbounded file growth#32529devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Co-Authored-By: Ishaan Jaffer <155045088+ishaan-berri@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThis PR introduces a lightweight CI gate that prevents unbounded growth of files in the repository root by counting tracked root-level files with
Confidence Score: 4/5Safe to merge — changes are confined to a new CI workflow, a standalone utility script, and its tests; no production code is touched. The script and workflow are clean and well-thought-out. Two small robustness gaps exist: Both findings are in scripts/check_root_file_count.py and tests/test_litellm/test_check_root_file_count.py and are confined to argument validation and test import robustness.
|
| Filename | Overview |
|---|---|
| .github/workflows/check-root-file-count.yml | New workflow gating PRs on repo-root file count; correctly pins action SHAs, sets minimal permissions, and uses a sensible concurrency group. |
| scripts/check_root_file_count.py | Clean, well-structured script; minor robustness gap in main() — missing argument-count and type validation before int(argv[1]). |
| tests/test_litellm/test_check_root_file_count.py | Good coverage of the three key behaviors; module loading at file scope can produce a confusing import error if the path changes. |
Reviews (1): Last reviewed commit: "ci: gate repository root against unbound..." | Re-trigger Greptile
| def main(argv: tuple[str, ...]) -> int: | ||
| max_root_files = int(argv[1]) |
There was a problem hiding this comment.
The
main() function indexes argv[1] without first verifying that a second element exists, and passes the raw string directly to int() without catching ValueError. Invoking the script with no argument or a non-integer value produces a bare Python traceback rather than an actionable message — something a developer debugging the workflow locally would find confusing.
| def main(argv: tuple[str, ...]) -> int: | |
| max_root_files = int(argv[1]) | |
| def main(argv: tuple[str, ...]) -> int: | |
| if len(argv) < 2: | |
| print("Usage: check_root_file_count.py <max_root_files>", file=sys.stderr) | |
| return 2 | |
| try: | |
| max_root_files = int(argv[1]) | |
| except ValueError: | |
| print(f"Error: expected an integer argument, got {argv[1]!r}", file=sys.stderr) | |
| return 2 |
| _MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "check_root_file_count.py" | ||
| _spec = importlib.util.spec_from_file_location("check_root_file_count", _MODULE_PATH) | ||
| checker = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(checker) |
There was a problem hiding this comment.
The module is loaded at module scope, so a
None return from spec_from_file_location (e.g. if the script path is wrong after a rename) causes module_from_spec(None) to raise TypeError at collection time, making the entire test file fail to import with a confusing traceback. Adding a guard before proceeding keeps the failure message clear.
| _MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "check_root_file_count.py" | |
| _spec = importlib.util.spec_from_file_location("check_root_file_count", _MODULE_PATH) | |
| checker = importlib.util.module_from_spec(_spec) | |
| _spec.loader.exec_module(checker) | |
| _MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "check_root_file_count.py" | |
| _spec = importlib.util.spec_from_file_location("check_root_file_count", _MODULE_PATH) | |
| if _spec is None or _spec.loader is None: | |
| raise ImportError(f"Could not load module from {_MODULE_PATH}") | |
| checker = importlib.util.module_from_spec(_spec) | |
| _spec.loader.exec_module(checker) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…imit to 43 Co-Authored-By: Ishaan Jaffer <155045088+ishaan-berri@users.noreply.github.com>
Merging this PR will not alter performance
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
Relevant issues
Follows the #pr-review discussion about random files landing in the repo root (the
qa_sticky_session.shexample from #31688 is removed here)Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Captured at commit
cd1fe15f93. After removingqa_sticky_session.shthe tree has 43 tracked root files, so with the limit at 43 the gate passes; dropping the limit by one simulates a PR adding a 44th root file and it fails with an actionable message plus the full listingType
🚄 Infrastructure
Changes
I looked at how other projects keep their root tidy before building this. There is no widely adopted off-the-shelf GitHub Action for "cap files in the repo root"; projects that care either maintain a hand-rolled allowlist script or, more commonly, enforce a simple count in CI (the kubernetes/kubernetes
verify-*shell hacks are the canonical example of the latter). Per Ishaan's steer in the thread I went with the count approach since it is the least fussy and directly answers Yuneng's "how do we explicitly allow a new file" questionscripts/check_root_file_count.pylists the tracked files withgit ls-files, keeps only those with no/in their path (repo root), and exits non-zero when the count exceeds the limit passed on argv. The new.github/workflows/check-root-file-count.ymlruns it on PRs into thelitellm_*branches (and the merge queue) withMAX_ROOT_FILESset to the current countThis PR also removes
qa_sticky_session.sh, the stray root-level QA script from #31688 that kicked off the thread; it is a standalone manual curl script that nothing imports, so deleting it drops the count to 43 and letsMAX_ROOT_FILESstart at 43 rather than 44Pinning the limit at exactly today's count is deliberate: the tree passes as-is, no further cleanup is forced, and any PR that adds a net-new root file goes red. To land a genuinely needed root file you bump
MAX_ROOT_FILESin the same PR, which turns "allow a new root file" into a one-line, reviewable diff rather than an invisible drift. A reviewer seeing that bump can push back or ask for the file to move into a subdirectory insteadA count-only gate does not pin which files are allowed, so swapping one root file for another keeps the tree green; that is an accepted trade for the simplicity Ishaan asked for. If we later want per-file control the same script can grow an allowlist without changing the workflow contract
tests/test_litellm/test_check_root_file_count.pypins the behavior that matters: that only top-level paths are counted (nested paths are ignored), that count-equal-to-limit passes while over-limit fails, and that the failure message is actionable and lists offenders in sorted orderLink to Devin session: https://app.devin.ai/sessions/ca9e2a7219ce49faa2c7bdcbff0b36db
Requested by: @ishaan-berri