Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ Don't hesitate to use values in .env to get needed API keys and other secrets, a

Python max line length is 120, not 88

When you fix violations gated by `ruff-strict-budget.json`, `type-discipline-budget.json`, or `basedpyright-code-budget.json`, run `make lint-budget-update` and commit the lowered limits so the ceilings ratchet down instead of leaving stale headroom. It measures the working tree, so it must contain exactly the fixes you're committing
Never edit or commit `ruff-strict-budget.json`, `type-discipline-budget.json`, `basedpyright-code-budget.json`, or `test-quality-budget.json` on a PR branch, and don't run `make lint-budget-update` there. A scheduled Devin automation lowers the limits on `litellm_internal_staging` in its own PR by exactly what landed since the last ratchet, so concurrent PRs don't fight over the same `"limit"` lines. If your branch already carries a budget edit, drop it before opening the PR

`make check` (f.k.a. `make pre-commit`, which still works identically as an alias) saves its complete output to a log file in .git (overwriting previous logs) and prints that path as its first and last output lines. To inspect a run, read or grep that log instead of re-running the multi-minute checks just to see a different slice

Expand Down
59 changes: 8 additions & 51 deletions scripts/test_quality_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,12 @@

Every rule is seeded at exactly its count on the day the gate landed, so the
suite's existing debt is grandfathered and any net-new violation trips the gate
immediately. ``--update`` ratchets a limit down by the violations this branch
fixed relative to its branch point (the merge-base), so the ceilings only ever
fall. Base counts are measured with the *current* checker, so a rule introduced
on this branch is counted at the base too and ratchets like every other one.

Only ever falling is not the same as always falling, so the gate enforces the
second half: a branch that clears violations and leaves the ceiling above its
new count fails, naming the rules and telling the author to run
``make lint-budget-update``. Without that, a removed violation could come back
later under a ceiling nobody lowered. Drift already in the base is never
blamed, so this fires only on the branch that did the clearing.
immediately. ``--update`` ratchets a limit down by the violations fixed relative
to ``--base``, so the ceilings only ever fall. Base counts are measured with the
*current* checker, so a rule introduced on this branch is counted at the base too
and ratchets like every other one. The ratchet runs as a scheduled automation
against litellm_internal_staging, not on PR branches, so concurrent PRs never
race to edit the same limit.

The deliberate difference from its sibling: this gate has no headroom anywhere.
Type discipline seeded LIT010/LIT011 at 1.5x to leave room for an in-flight
Expand Down Expand Up @@ -144,21 +139,6 @@ def over_ceiling(head: Mapping[str, int], budget: Mapping[str, Mapping[str, int]
)


def unratcheted(
head: Mapping[str, int],
base: Mapping[str, int],
budget: Mapping[str, Mapping[str, int]],
) -> tuple[Breach, ...]:
"""Rules this branch cleared without lowering the ceiling behind them. Requires
both `head < base`, so drift already in the base is never blamed on this change,
and `head < limit`, so a ceiling already at the count is left alone."""
return tuple(sorted(
Breach(rule, head.get(rule, 0), spec["limit"], head.get(rule, 0) - base.get(rule, 0))
for rule, spec in budget.items()
if head.get(rule, 0) < base.get(rule, 0) and head.get(rule, 0) < spec["limit"]
))


def evaluate(
head: Mapping[str, int],
base: Mapping[str, int],
Expand Down Expand Up @@ -198,38 +178,15 @@ def introduced(
return tuple(v for v in violations if v.line in changed.get(v.file, frozenset()))


def touches_measured_tree(base_point: str) -> bool:
"""Whether this branch changed anything that can move a count. A branch that
touches neither the test tree nor the checker cannot have cleared a violation,
so the base scan is skipped and the gate stays cheap on the common change."""
changed: Final = _run(
["git", "diff", "--name-only", base_point, "--", TARGET, str(CHECKER.relative_to(REPO_ROOT))]
)
return bool(changed.strip())


def cmd_check(base: str) -> None:
budget: Final = json.loads(BUDGET_PATH.read_text())
head: Final = head_violations()
head_counts: Final = count_by_rule(head)
base_point: Final = resolve_base_point(base)
if not over_ceiling(head_counts, budget) and not touches_measured_tree(base_point):
if not over_ceiling(head_counts, budget):
print(f"OK: every TQ rule is within its test-suite ceiling (base {base})")
return
base_point: Final = resolve_base_point(base)
base_at_point: Final = base_counts(base_point)
stale: Final = unratcheted(head_counts, base_at_point, budget)
if stale:
print(f"FAIL: TQ-rule limits were left above the count this branch reached (base {base}):")
for breach in stale:
print(
f" {breach.rule}: this branch cleared {-breach.added} down to {breach.total}, "
f"but the limit is still {breach.cap}"
)
print(
"Run `make lint-budget-update` and commit the lowered limits, so the "
"violations you cleared cannot come back under a ceiling nobody moved."
)
raise SystemExit(1)
breaches: Final = evaluate(head_counts, base_at_point, budget)
if not breaches:
print(f"OK: every TQ rule is within its test-suite ceiling (base {base})")
Expand Down
33 changes: 4 additions & 29 deletions tests/test_litellm/test_test_quality_gate.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
"""Tests for scripts/test_quality_gate.py.

The gate's whole value is that it blames a change only for what it adds, that a limit
can never rise, and that a limit cannot stay above a count the branch pushed below it.
All three live in pure functions, so they are tested directly: `evaluate` for the blame
rule, `ratcheted_budget` for the one-way ratchet, `unratcheted` for the ceiling a branch
left behind, and `parse_changed_lines` for the diff scan that turns a breach into
file:line.
The gate's whole value is that it blames a change only for what it adds and that a
limit can never rise. Both live in pure functions, so they are tested directly:
`evaluate` for the blame rule, `ratcheted_budget` for the one-way ratchet, and
`parse_changed_lines` for the diff scan that turns a breach into file:line.
"""

import importlib.util
Expand Down Expand Up @@ -73,29 +71,6 @@ def test_ratchet_lowers_a_rule_introduced_on_this_branch_like_any_other():
assert updated["TQ001"]["limit"] == 4


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Ratchet coverage was removed

These changes remove the previous ratchet-enforcement tests without adding coverage for the workflow that now owns this behavior. No test models queued push ranges or verifies that each fix is applied exactly once. This violates the repository directive that modifications to existing tests must not weaken regression coverage, so replacement coverage is required before merging.

Rule Used: What: Flag any modifications to existing tests and... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

def test_a_branch_that_cleared_violations_must_lower_the_ceiling():
stale = gate.unratcheted({"TQ001": 6}, {"TQ001": 10}, _BUDGET)
assert [(b.rule, b.total, b.cap, b.added) for b in stale] == [("TQ001", 6, 10, -4)]


def test_headroom_already_in_the_base_is_not_blamed_on_this_branch():
assert gate.unratcheted({"TQ001": 6}, {"TQ001": 6}, _BUDGET) == ()


def test_a_branch_that_cleared_down_to_the_ceiling_exactly_is_clean():
assert gate.unratcheted({"TQ001": 10}, {"TQ001": 12}, _BUDGET) == ()


def test_a_branch_that_added_violations_is_not_a_ratchet_finding():
assert gate.unratcheted({"TQ001": 14}, {"TQ001": 10}, _BUDGET) == ()


def test_the_ratchet_finding_survives_the_update_that_answers_it():
cleared = {"TQ001": 6}
updated = gate.ratcheted_budget(_BUDGET, cleared, {"TQ001": 10})
assert gate.unratcheted(cleared, {"TQ001": 10}, updated) == ()


def test_parse_changed_lines_groups_hunks_under_their_own_file():
diff = (
"diff --git a/tests/a.py b/tests/a.py\n"
Expand Down
Loading