refactor(lint): collapse type/lint budgets to a single per-rule limit - #31883
Conversation
The per-rule ceilings in basedpyright-code-budget.json sat at roughly 10% slack over baseline, which several in-flight PRs are already bumping into. Raise the slack on every rule to at least 50% of its baseline so there is ample headroom for a long while, while never lowering any rule that already had more generous slack (e.g. reportReturnType stays at 100). Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The three non-frontend budget files (ruff-strict, type-discipline, basedpyright-code) tracked a per-rule baseline and slack whose sum was the ceiling. Nothing consumed the split beyond that sum, so this replaces both keys with a single limit equal to the old baseline + slack; the original baselines live in git history if anyone needs them. The gate scripts and the ratchet guard now read limit directly. lint-budget-update no longer re-captures raw counts; it ratchets each rule's limit down by the number of violations this branch cleared since its branch point (the merge-base), so the granted headroom shrinks by exactly what was fixed and a limit never rises. The ratchet guard reads either schema so it still compares correctly across the migration boundary. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
…et-update make pre-commit selects which checks to run from the staged index but runs the linters over the working tree, so unstaged edits to tracked files and untracked files skew a green/red away from what a commit of only the staged changes would produce. There is no safe in-place way to lint the index, so the script now warns when unstaged or untracked changes are present, and CLAUDE.md documents that you must stage everything first for both make pre-commit and make lint-budget-update to predict CI correctly. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
|
Generated by Claude Code |
Greptile SummaryThis PR collapses the
Confidence Score: 5/5Purely tooling-internal changes with no production code paths touched; all gate scripts and the ratchet guard continue to protect CI from regressions. The change touches only lint tooling, budget JSON files, and their tests. The core ratchet arithmetic is straightforward and directly tested in all three gate suites. budget_ratchet_check.py correctly bridges the schema migration on the base side via _ceiling(). The previously noted concern about spec["limit"] lacking a legacy fallback in is_vacuous_run and ratcheted_budget is a pre-existing flag from the earlier review thread and carries no new risk after the budget files are fully migrated here. No files require special attention.
|
| Filename | Overview |
|---|---|
| scripts/type_check_gate.py | Migrated from baseline+slack to single limit; added ratcheted_budget helper; --update now lowers limits by branch fixes vs merge-base instead of re-capturing raw counts |
| scripts/ruff_strict_gate.py | Same limit schema migration and ratcheted_budget logic as type_check_gate.py; --update now passes base_ref through from args.base |
| scripts/type_discipline_gate.py | Migrated to single limit schema and gained ratcheted_budget; --update wired to base_ref; now included in lint-budget-update via Makefile |
| scripts/budget_ratchet_check.py | Dropped independent baseline tracking; added _ceiling() helper that reads both new limit and legacy baseline+slack, keeping base-side comparisons safe across the migration |
| scripts/pre_commit_lint.sh | Added unstaged/untracked detection block; warns to stderr when working tree diverges from staged index so the pre-commit result may not match CI |
| Makefile | lint-budget-update now includes lint-type-discipline-budget-update; update targets gain lint-fetch-base dependency; help text updated |
| CLAUDE.md | Added type-discipline-budget.json to the lint-budget-update guidance and documented the stage-first requirement for both pre-commit and budget update |
| tests/test_litellm/test_type_check_gate.py | Updated fixtures to new limit schema; added three new tests covering ratcheted_budget: fixes lower limits, growth holds flat, and zero-clamping |
| tests/test_litellm/test_budget_ratchet_check.py | Updated to new schema; added test confirming legacy baseline+slack format is still read correctly on the base side of a comparison |
| tests/test_litellm/test_ruff_strict_gate.py | Updated fixtures; added test for ratcheted_budget confirming fixes reduce limits and growth leaves them flat |
| tests/test_litellm/test_type_discipline_gate.py | Updated fixtures; added test for ratcheted_budget with fix-lowers/growth-holds behaviour |
Reviews (3): Last reviewed commit: "docs(lint): list type-discipline budget ..." | Re-trigger Greptile
| empty run would clear every ceiling and pass silently.""" | ||
| return not counts and any(spec["baseline"] for spec in budget.values()) | ||
| empty run would clear every limit and pass silently.""" | ||
| return not counts and any(spec["limit"] for spec in budget.values()) |
There was a problem hiding this comment.
is_vacuous_run now directly accesses spec["limit"], but budget_ratchet_check.py uses a dedicated _ceiling() helper that also handles the legacy baseline + slack schema. If cmd_check were ever called with an incompletely-migrated budget (e.g. after a merge conflict that reintroduced one old-format rule), this will KeyError instead of catching the vacuous run — the safety net silently disappears.
| return not counts and any(spec["limit"] for spec in budget.values()) | |
| return not counts and any( | |
| spec.get("limit") or (spec.get("baseline", 0) + spec.get("slack", 0)) | |
| for spec in budget.values() | |
| ) |
There was a problem hiding this comment.
Declining this one deliberately. The proposed fallback would be dead code guarding a state that no normal commit, branch, or PR reaches, and the repo guidelines rule out error handling for impossible scenarios.
The merge-conflict framing doesn't actually make it reachable in a way this change would catch. If a botched conflict resolution left one rule in the old baseline + slack form, the file is simply corrupt, and evaluate() (cap = spec["limit"]) and the expected = sum(spec["limit"] ...) line right below this one would KeyError on that same rule anyway. Patching only is_vacuous_run doesn't restore a safety net; it just moves the loud failure a few lines down while pretending the file is well-formed. A KeyError naming the missing limit key is the correct, visible failure for a corrupt budget, not something to paper over.
budget_ratchet_check.py needs _ceiling() for a real reason: it reads the base-ref copy of the JSON via git show <ref>:<path>, which genuinely can predate the migration during this transition. type_check_gate.py never reads a base-ref copy; it derives base counts by running basedpyright over the base worktree, so it can't encounter the old schema. The asymmetry is intentional, not an oversight.
Generated by Claude Code
|
Thanks for the review. On the two points:
Generated by Claude Code |
Greptile SummaryCollapses the
Confidence Score: 4/5Pure tooling refactor with no production code changes; the gate logic is mechanically equivalent to the prior schema and is well-covered by 46 unit tests. The ratchet arithmetic, schema migration, and pre-commit warning are all correct and well-tested. The only gaps are that the gate scripts access spec["limit"] directly without a legacy-schema fallback (unlike budget_ratchet_check.py), and CLAUDE.md omits type-discipline-budget.json from the budget-update guidance paragraph. Neither affects runtime correctness while the repository is in its post-migration state. CLAUDE.md (guidance paragraph names only two of the three budget files now updated by make lint-budget-update); scripts/type_check_gate.py (is_vacuous_run and ratcheted_budget lack the legacy-schema fallback present in budget_ratchet_check.py).
|
| Filename | Overview |
|---|---|
| scripts/type_check_gate.py | Migrated from baseline+slack to a single limit field; added ratcheted_budget helper; --update now lowers limits by what the branch fixed vs merge-base. is_vacuous_run uses spec["limit"] directly with no legacy-schema fallback (unlike budget_ratchet_check.py). |
| scripts/ruff_strict_gate.py | Migrated ceiling from baseline+slack to limit; ratcheted_budget helper added; cmd_update now computes branch-point delta and passes base_ref correctly. |
| scripts/type_discipline_gate.py | Migrated to single limit schema; ratcheted_budget helper added; cmd_update now takes base_ref and ratchets by branch fixes. Consistent with ruff_strict_gate.py. |
| scripts/budget_ratchet_check.py | Simplified regression detection to a single limit comparison; _ceiling() correctly handles both new limit schema and legacy baseline+slack for cross-migration diffs. Dropped the separate baseline-raise check, which is no longer needed. |
| scripts/pre_commit_lint.sh | Added unstaged/untracked file warning that lists affected files on stderr without blocking the check. Logic is correct: git diff --name-only + git ls-files --others --exclude-standard. |
| Makefile | lint-budget-update now includes type-discipline; update targets gain lint-fetch-base dependency so merge-base resolution works; new lint-type-discipline-budget-update target added and declared PHONY. |
| CLAUDE.md | Documents the stage-first requirement for pre-commit and lint-budget-update, but the budget-update guidance still only names ruff-strict and basedpyright budgets, omitting type-discipline-budget.json which is now also updated. |
| tests/test_litellm/test_type_check_gate.py | Tests updated to new limit schema; added ratcheted_budget tests covering fix-driven decrease, growth holdflat, and zero-clamp cases. Good coverage of new semantics. |
| tests/test_litellm/test_budget_ratchet_check.py | Tests updated for new schema; legacy baseline+slack compatibility test added; dropped tests for the now-removed baseline-raise check. Coverage matches the new contract. |
| type-discipline-budget.json | Collapsed baseline+slack into a single limit equal to the old sum for all eight LIT rules. Values are mechanically equivalent to the prior schema. |
Comments Outside Diff (1)
-
scripts/type_check_gate.py, line 147-154 (link)is_vacuous_runaccessesspec["limit"]without a legacy fallbackbudget_ratchet_check.pyexplicitly supports both the oldbaseline + slackschema and the newlimitschema via_ceiling()to handle the case where the base side of a diff predates the migration.is_vacuous_runhere does not have that fallback:spec["limit"]raisesKeyErroron any old-schema spec. In normal flow after this PR lands, all budget files carry the new schema, so this cannot fire. But if a future revert of the budget JSON (without a corresponding script revert) happens, the gate would crash instead of failing gracefully. The inconsistency between howbudget_ratchet_check.pyand the gate scripts handle mixed-schema data is worth a comment, even if the practical risk today is low.
Reviews (2): Last reviewed commit: "docs(lint): list type-discipline budget ..." | Re-trigger Greptile
|
Generated by Claude Code |
|
On the one remaining P2 ( The review itself grants that it cannot fire: "In normal flow after this PR lands, all budget files carry the new schema, so this cannot fire." The only scenario it constructs is a future revert of
Given the deducting item is acknowledged as unable to fire and the fix would be dead code the guidelines forbid, I'd ask that this not stand as a blocker. Generated by Claude Code |
|
bugbot run Generated by Claude Code |
1 similar comment
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 24f9877. Configure here.
e141596
into
litellm_internal_staging
…BerriAI#31883) * chore(lint): raise basedpyright per-rule slack to 50% of baseline The per-rule ceilings in basedpyright-code-budget.json sat at roughly 10% slack over baseline, which several in-flight PRs are already bumping into. Raise the slack on every rule to at least 50% of its baseline so there is ample headroom for a long while, while never lowering any rule that already had more generous slack (e.g. reportReturnType stays at 100). Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com> * refactor(lint): collapse type/lint budgets to a single per-rule limit The three non-frontend budget files (ruff-strict, type-discipline, basedpyright-code) tracked a per-rule baseline and slack whose sum was the ceiling. Nothing consumed the split beyond that sum, so this replaces both keys with a single limit equal to the old baseline + slack; the original baselines live in git history if anyone needs them. The gate scripts and the ratchet guard now read limit directly. lint-budget-update no longer re-captures raw counts; it ratchets each rule's limit down by the number of violations this branch cleared since its branch point (the merge-base), so the granted headroom shrinks by exactly what was fixed and a limit never rises. The ratchet guard reads either schema so it still compares correctly across the migration boundary. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com> * chore(lint): surface staged-vs-working parity for pre-commit and budget-update make pre-commit selects which checks to run from the staged index but runs the linters over the working tree, so unstaged edits to tracked files and untracked files skew a green/red away from what a commit of only the staged changes would produce. There is no safe in-place way to lint the index, so the script now warns when unstaged or untracked changes are present, and CLAUDE.md documents that you must stage everything first for both make pre-commit and make lint-budget-update to predict CI correctly. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com> * docs(lint): list type-discipline budget in lint-budget-update instruction --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
…dashboard Add backend-lint-metrics.json with the real reportAny (implicit Any) and reportExplicitAny (explicit Any) counts across the tree. The basedpyright budget's limit stopped tracking the real count after its limit collapse in #31883, so a trend dashboard reading that file went stale; this reports the real numbers instead. scripts/lint_metrics.py regenerates it, reusing type_check_gate's counting so the numbers match what CI measures, and a weekly workflow opens a PR when they drift. Report-only: nothing gates on the file
Relevant issues
The per-rule ceilings in the type and lint budgets sit at roughly 10% headroom over the recorded count, and multiple in-flight PRs are already tripping them even when they add only a handful of diagnostics. This first widens that headroom, then reworks how the budgets are represented and ratcheted so the mechanism is simpler to reason about
There are three steps, one per commit. First, every rule in
basedpyright-code-budget.jsongets its slack raised to at least 50% of baseline (never lowering a rule that already had more), so there is ample room for a long while. Second, the three non-frontend budget files (ruff-strict, type-discipline, basedpyright-code) drop thebaseline+slacksplit in favour of a singlelimitequal to the old sum; nothing consumed the split beyond that sum, and the original baselines live in git history if anyone wants them. Third,make pre-commitandmake lint-budget-updateget an explicit staged-vs-working-tree contractmake lint-budget-updateno longer re-captures raw counts. It lowers each rule'slimitby the number of violations this branch cleared since its branch point (the merge-base with the base branch), so the granted headroom shrinks by exactly what was fixed and a limit is never raised. CI stays red for a rule only under the same two conditions as today: the codebase total is over the limit and this change is above the branch-point count (it introduced at least one net violation), so an unrelated PR is never blamed for pre-existing driftOn parity:
make pre-commitchooses which checks to run from the staged index but runs the linters over the working tree, so unstaged edits and untracked files skew a green/red away from a commit of only the staged changes. There is no safe way to lint the index in place, so the script now warns when unstaged or untracked changes are present, and CLAUDE.md documents that you must stage everything first for bothmake pre-commitandmake lint-budget-updateto predict CI correctlyPre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
This is lint tooling, so the proof is the scripts running against the migrated budgets exactly as a developer and CI invoke them
The gates read the new
limitschema and pass:--updateratchets a limit down by exactly what the branch fixed since its branch point. Suppressing one realcast()(a genuine LIT006 fix) and re-running dropped that rule's limit by one, and by nothing else:With no branch fixes it is a no-op and never raises a limit:
The non-gating ratchet guard reads either schema, so across this migration it correctly reports the ruff and type-discipline limits as unchanged and flags only the basedpyright ceilings that genuinely rose from the 50% headroom bump (expected, and merged over by a human since the guard is advisory):
The pre-commit parity guard warns on unstaged/untracked changes and stays silent once everything is staged:
Unit tests for all three gates and the ratchet guard, including the new ratchet-by-fixes arithmetic and the legacy-schema fallback:
Type
🧹 Refactoring
🚄 Infrastructure
Changes
basedpyright-code-budget.json,ruff-strict-budget.jsonandtype-discipline-budget.jsonnow carry a singlelimitper rule.scripts/type_check_gate.py,scripts/ruff_strict_gate.pyandscripts/type_discipline_gate.pyreadlimit, and their--updateratchets it down by what the branch fixed vs its branch point (extracted into a small testedratcheted_budgethelper).scripts/budget_ratchet_check.pycompares limits and tolerates the legacybaseline+slackschema on the base side. The Makefile help and targets are reworded,lint-budget-updatenow also ratchets the type-discipline budget, and the update targets fetch the base ref since they resolve the merge-base.scripts/pre_commit_lint.shwarns on unstaged/untracked changes, and CLAUDE.md documents the stage-first requirementSlack Thread