Skip to content

build(lint): rename make pre-commit to make check with a working-tree fallback - #36277

Merged
mateo-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_make_check_fallback
Aug 8, 2026
Merged

build(lint): rename make pre-commit to make check with a working-tree fallback#36277
mateo-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_make_check_fallback

Conversation

@mateo-berri

@mateo-berri mateo-berri commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • make pre-commit with nothing staged silently checks nothing and exits 0
  • Coding agents keep hitting this and pay the multi-minute suite twice
  • With everything committed (e.g. after a merge) there was no way to predict CI lint
  • Partially staged runs skipped checks without saying which ones
  • Deleted files never counted toward the checks, so a deletion-only branch got a false green

How it solves it:

  • Renames the target to make check; make pre-commit stays as a legacy alias
  • Nothing staged now scopes checks to the working tree's diff vs the merge base
  • A tree with no changes at all prints an explicit "nothing to check"
  • Partial staging prints SKIPPED warnings naming the check and the unstaged files
  • Deletions trigger the matching checks in both modes, but deleted paths are never fed to tools that read file contents
  • Regression tests cover the fallback, the no-op, the missing-ref error, the warnings, and the deletion triggers

User Flow

Before: a contributor (or coding agent) with edited but unstaged files runs make pre-commit and gets a green exit that checked nothing

  1. They edit nine files under litellm/ and, without staging anything, run make pre-commit
  2. The run prints only a note that unstaged changes exist, runs zero checks, and exits 0 in seconds
  3. Reading that as a pass, they either push and watch the CI lint job go red on GitHub, or catch the miss, stage the nine files, and pay the multi-minute suite a second time
  4. After a merge commit, with everything committed and nothing staged, the question cannot be asked at all: the run silently checks nothing and exits 0

After: the same run with nothing staged checks the branch's actual diff, and partial staging warns instead of silently skipping

  1. They edit nine files under litellm/ and, without staging anything, run make check (typing make pre-commit still works and prints that it is a legacy alias)
  2. The run prints "nothing staged; scoping to the working tree's diff against the merge base with origin/litellm_internal_staging", lists the nine files, and runs the matching CI-equivalent checks once
  3. A green exit now means the CI lint job will pass, and a red one names the failures before anything is pushed
  4. After a merge commit the same make check validates the merged tree the same way, staging only part of the changes makes the run print which check was SKIPPED and which changed files are not staged, and a branch whose only change deletes files gets its checks run instead of a green "nothing to check"

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

Screenshots / Proof of Fix

All legs ran in a fresh bootstrapped worktree. Before legs at base e24a914, after legs at head fb7861f

Before: an unstaged litellm/constants.py edit with nothing staged runs zero checks and exits 0

$ git status --short
 M CLAUDE.md
 M Makefile
 M litellm/constants.py
 ...
$ scripts/pre_commit_lint.sh   # base version, nothing staged
pre-commit: NOTE - unstaged/untracked changes are included in these checks but
  won't be in a commit of only your staged changes, so this result may differ from
  CI. Stage everything you intend to commit (git add) for an accurate prediction:
    ...
    litellm/constants.py
exit=0

After: the same state runs the full Python lint through the fallback and passes

$ make check
check: nothing staged; scoping to the working tree's diff against the merge base with origin/litellm_internal_staging:
    CLAUDE.md
    litellm/constants.py
    Makefile
    scripts/install_git_hooks.sh
    scripts/pre_commit_lint.sh
    tests/e2e/CONTRIBUTING.md
    tests/test_litellm/test_pre_commit_lint.py
check: linting Python (make lint)
...
OK: every strict rule is within its codebase ceiling (base origin/litellm_internal_staging)
0 errors, 0 warnings, 0 notes
[from litellm import *] OK! no issues!
OK: every rule is within its basedpyright limit or no higher than base (149330 errors total)
check: ruff format --check (scoped litellm files)
1 file already formatted
check: full log: .../pre_commit_lint.log

Deletion-only branch, the case Greptile flagged: at the previous head f038be2, deleting litellm/cost_calculator.py at the staging tip with nothing staged reported a false green

$ rm litellm/cost_calculator.py && scripts/pre_commit_lint.sh   # f038be22db version
check: nothing to check (no staged files, no working-tree changes, no branch changes vs origin/litellm_internal_staging)
exit=0

After: the same deletion-only tree triggers the Python lint, which goes red on the import CI would have caught, and the deleted path is never handed to ruff format

$ rm litellm/cost_calculator.py && scripts/pre_commit_lint.sh   # fb7861fbfd version
check: nothing staged; scoping to the working tree's diff against the merge base with origin/litellm_internal_staging:
    litellm/cost_calculator.py
check: linting Python (make lint)
...
    from litellm.cost_calculator import (
ModuleNotFoundError: No module named 'litellm.cost_calculator'
make[1]: *** [check-import-safety] Error 1
exit=1
$ grep -c "ruff format --check" pre_commit_lint.log
0

After: partial staging (only CLAUDE.md staged, litellm/constants.py left unstaged) via the legacy alias

$ git add CLAUDE.md && make pre-commit
make pre-commit is a legacy alias; use make check
check: NOTE - unstaged/untracked changes are included in these checks but
  ...
check: SKIPPED Python lint (make lint) because these changed files are not staged:
    litellm/constants.py
exit=0

After: a clean tree at the base tip is an explicit no-op

$ scripts/pre_commit_lint.sh   # clean checkout on the staging tip
check: nothing to check (no staged files, no working-tree changes, no branch changes vs origin/litellm_internal_staging)
exit=0

Test suite, 10 pre-existing tests plus 9 new ones:

$ uv run --no-sync pytest tests/test_litellm/test_pre_commit_lint.py -q
19 passed in 13.39s

Type

🚄 Infrastructure
✅ Test

Changes

  • Makefile: check target replaces pre-commit, which becomes an alias that prints a notice and delegates
  • scripts/pre_commit_lint.sh: scope selection is now staged files when anything is staged, else the working tree's diff (untracked included) vs the merge base with origin/litellm_internal_staging; staged mode warns which checks were skipped over unstaged files; an unresolvable base ref fails with a fetch hint instead of silently checking nothing
  • scripts/pre_commit_lint.sh: deletions count toward the check triggers in both modes (a deletion alone can break CI), while the file lists fed to ruff format, prettier, and eslint are filtered to paths that still exist
  • CLAUDE.md, tests/e2e/CONTRIBUTING.md, scripts/install_git_hooks.sh: document make check and the new scoping rules
  • tests/test_litellm/test_pre_commit_lint.py: regression tests for the fallback (unstaged, committed, untracked, deletion-only), the explicit no-op, the missing-ref failure, the SKIPPED warnings, and staged deletions

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

@greptile-apps

greptile-apps Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR renames the primary local lint target to make check and makes its scope reflect staged changes or, when nothing is staged, the working-tree and branch diff against the merge base.

  • Retains make pre-commit as a legacy alias.
  • Includes deletions when selecting CI-equivalent checks while excluding missing paths from file-reading tools.
  • Warns when partial staging causes checks to be skipped.
  • Adds regression coverage for fallback scoping, deletion-only changes, missing base refs, and explicit no-op behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported deletion-only false green is fixed for staged, unstaged, and committed deletions without passing deleted paths to file-reading tools.

Important Files Changed

Filename Overview
scripts/pre_commit_lint.sh Implements staged-or-merge-base scope selection, deletion-aware triggers, existing-file filtering, and partial-staging diagnostics; the previously reported deletion-only false green is resolved.
tests/test_litellm/test_pre_commit_lint.py Adds focused regression tests for unstaged, committed, untracked, deletion-only, no-op, missing-ref, and partial-staging cases.
Makefile Introduces check as the primary target and preserves pre-commit as a delegating compatibility alias.
CLAUDE.md Updates contributor guidance to describe the renamed command and its staged-versus-merge-base scoping behavior.

Reviews (2): Last reviewed commit: "build(lint): count deleted files toward ..." | Re-trigger Greptile

Comment thread scripts/pre_commit_lint.sh Outdated
Comment thread scripts/pre_commit_lint.sh
@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@mateo-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

@mateo-berri
mateo-berri merged commit 8b16ee1 into litellm_internal_staging Aug 8, 2026
79 of 80 checks passed
@mateo-berri
mateo-berri deleted the litellm_make_check_fallback branch August 8, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants