Skip to content

refactor(lint): allow dict and mapping annotations in LIT001 - #41100

Open
devin-ai-integration[bot] wants to merge 12 commits into
mainfrom
litellm_remove_lit002_dict_ban
Open

devin-ai-integration[bot] wants to merge 12 commits into
mainfrom
litellm_remove_lit002_dict_ban

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Relevant issues

Pre-Submission checklist

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

  • I have Added testing in the tests/test_litellm/ directory, Adding at least 1 test is a hard requirement
  • I have added a screenshot of my new test passing locally
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem

Type

🧹 Refactoring

Changes

Main already retired LIT002 in #43971, so this PR is now the remaining half of the LIT002 cleanup: LIT001 stops flagging mapping annotations

 MUTABLE_COLLECTIONS = frozenset((
-    "dict", "list", "set", "Dict", "List", "DefaultDict", "OrderedDict", "Counter",
-    "Deque", "ChainMap", "deque", "defaultdict", "MutableMapping", "MutableSequence", "MutableSet",
+    "list", "set", "List", "Deque", "deque", "MutableSequence", "MutableSet",
 ))

So x: dict[str, int] and payload: MutableMapping[str, object] are clean, while x: dict[str, list[int]] still trips on the nested list. The LIT001 message and docstring now recommend Sequence / AbstractSet / tuple / frozenset instead of also pushing Mapping

Once dict is allowed, LIT013 flags every # mutable-ok that only covered a mapping annotation as suppressing nothing, so 945 of them across 269 files are deleted. Those are comment-only edits; the eight test files that ruff format also touched were already unformatted on main and CI's scoped format check requires them clean

AGENTS.md: the LIT001 paragraph and the no-mutation convention now say plain dict is fine (most of the ecosystem takes and returns dicts, MappingProxyType at every boundary costs more than it protects), prefer tuples over lists and frozensets over sets, prefer frozen dataclasses or ReadOnly TypedDicts for structured records, and never deep copy a value just to hand out an immutable view (the self-referential breadcrumbs deep copy that leaked memory is the motivating case)

Why: the dict ban filled files with # mutable-ok noise, pushed defensive deep copies and MappingProxyType round trips at every library boundary, and made the LIT budget ratchet turn unrelated rebases red because almost every real PR adds a dict literal. The narrower rule on sequences and sets keeps the part of the policy that cost little

Not in scope: caches that shallow copy on every mutation and per-folder scoping of the rule, both discussed as follow ups

Tests

tests/unit/test_check_type_discipline.py gains test_mapping_annotations_are_allowed (dict, Dict, DefaultDict, MutableMapping, defaultdict, OrderedDict, string forward ref all clean; dict[str, list[int]] still flagged) and the existing LIT001 cases switch their dict fixtures to list/set so they keep testing the rule

uv run pytest tests/unit/test_check_type_discipline.py -q
137 passed in 1.20s
uv run python scripts/type_discipline_gate.py
OK: no LIT rule grew past its merge-base count (base origin/main)
uv run python scripts/check_type_discipline.py litellm/ tests/ scripts/ | grep -c LIT013
0

Review state

Greptile 5/5 and Bugbot no new issues at 82ba2a2, both re-triggered on each push. Required checks green. rust-wheel is red on main too: tests/test_litellm_rust/test_traces.py fails to collect with No module named scripts, and this PR touches nothing under tests/test_litellm_rust, scripts/seed_tracing_fixtures.py, or the Makefile

Link to Devin session: https://app.devin.ai/sessions/db4d9941966544268db903dcf292d47b
Open in Devin Desktop: https://app.devin.ai/desktop/session/db4d9941966544268db903dcf292d47b?variant=devin


Note

Low Risk
Lint policy and comment cleanup only; runtime logic is unchanged aside from narrower LIT001 enforcement on sequences and sets.

Overview
LIT001 no longer treats mapping types as mutable collections: dict, Dict, MutableMapping, defaultdict, OrderedDict, and similar annotations are allowed, while nested mutable containers (e.g. dict[str, list[int]]) still fail on the inner list/set.

Because many # mutable-ok comments only existed to excuse dict-typed parameters and locals, this PR strips hundreds of those suppressions (and a few related # rebind-ok notes) across harness, integrations, LLM providers, caching, and enterprise code—comment-only in those files, with no intended behavior change.

AGENTS.md is updated to match: plain dict is acceptable at boundaries; prefer tuples/frozensets over lists/sets; use frozen dataclasses or ReadOnly TypedDicts for structured records; avoid deep copies solely to expose an immutable view.

Unit coverage adds test_mapping_annotations_are_allowed and adjusts existing LIT001 fixtures to use list/set where dict was previously the example.

Reviewed by Cursor Bugbot for commit d14859c. Bugbot is set up for automated code reviews on this repo. Configure here.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@codspeed

codspeed Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_remove_lit002_dict_ban (d14859c) with main (ee47da3)

Open in CodSpeed

@mateo-berri

Copy link
Copy Markdown
Contributor

@greptile-apps

Comment thread litellm/proxy/management_endpoints/team_endpoints.py Fixed
@greptile-apps

greptile-apps Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no new actionable issue remains

Summary

LIT001 now allows mapping annotations while keeping its checks for mutable sequences and sets

  • LIT001 allows mappings but still flags mutable sequences and sets.
  • Many # mutable-ok comments disappear as mapping annotations stop needing exceptions.

The earlier _invoke_sdk comment was dismissed by devin-ai-integration[bot] because the mutation already existed on main. The retry-loop comment was correctly disputed by devin-ai-integration[bot] and conceded by Greptile. The stale mapping examples are fixed

Reviews (14) · Last reviewed commit: "chore: merge main into litellm_remove_li..." · Reviewed by Greptile

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment •

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.

Stale Bugbot comment from a previous run.

Main already retired LIT002 (#43971). This narrows LIT001 to mutable sequences and sets, so dict, Dict,
DefaultDict, OrderedDict, Counter, ChainMap, defaultdict and MutableMapping annotations are allowed, nested
list/set inside a mapping still trips, and the 945 mutable-ok suppressions that only covered mapping
annotations are deleted (LIT013 now flags them). AGENTS.md guidance updated to match

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
devin-ai-integration Bot force-pushed the litellm_remove_lit002_dict_ban branch from 6b847ee to 22d584a Compare October 7, 2026 17:11
@devin-ai-integration devin-ai-integration Bot changed the title refactor(lint): retire LIT002 and allow dict annotations in LIT001 refactor(lint): allow dict and mapping annotations in LIT001 Oct 7, 2026
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment •

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.

Stale Bugbot comment from a previous run.

mateo-berri and others added 2 commits October 7, 2026 21:06
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…m main

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

mateo-berri and others added 2 commits October 7, 2026 21:10
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment •

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.

Stale Bugbot comment from a previous run.

@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Comments Outside Diff

These findings could not be posted inline.

  • P1 Lens service port is unresolved helm/litellm/templates/lens/service.yaml:17 ▶

    When Lens is enabled in the litellm chart, this Service targets the named port otlp, but the Lens container does not declare a port with that name. Kubernetes therefore cannot resolve the Service target port, so traffic from the Lens ingress or gateway cannot reach the worker.

@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review. The three Lens Helm findings come from main commits merged in 0be8fd5; this PR diff vs merge base touches no helm or lens files

…_dict_ban_v2

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

# Conflicts:
#	litellm/types/decisions.py
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

Comment thread litellm/harness/endpoint.py
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

@cursor cursor Bot left a comment •

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.

Stale Bugbot comment from a previous run.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment •

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.

Stale Bugbot comment from a previous run.

Comment thread litellm-proxy-extras/litellm_proxy_extras/utils.py
Comment thread scripts/check_type_discipline.py Outdated
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment •

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.

Stale Bugbot comment from a previous run.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment

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.

✅ 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 d14859c. Configure here.

@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai review

This branch is waiting to be deployed

1 waiting deployment
e2e-changed — d14859c4 Waiting Oct 9, 2026 by devin-ai-integration[bot] via oauth #4038
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