mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-28 01:32:17 +00:00
address review: wire the duplicate-key guard into CI
Greptile was right: the guard was added to check_endpoint_coverage.py, and that script is not run by any workflow - so it would never have executed. A check that never runs is decoration. It cannot simply be added to CI as-is: check_endpoint_coverage.py currently exits 1 because docs/my-website/sidebars.js no longer exists at the path it expects. That failure is unrelated to this change, and wiring the script up as-is would turn CI red for a reason this PR did not cause. So the guard moves into its own small script and gets its own CI step: - tests/code_coverage_tests/check_json_duplicate_keys.py (new, standalone) - one step in .github/workflows/test-code-quality.yml, next to check_licenses - check_endpoint_coverage.py reverted to exactly its upstream contents Verified it fails when it should: re-inserting the duplicate charity_engine entry makes the new step exit 1 and name the key; removing it again exits 0. The script also exits 1 if FILES_TO_CHECK is empty or a listed path is gone - scanning zero files must not look like success.
This commit is contained in:
parent
215ced2bdb
commit
9856bdf22f
3 changed files with 101 additions and 59 deletions
3
.github/workflows/test-code-quality.yml
vendored
3
.github/workflows/test-code-quality.yml
vendored
|
|
@ -65,6 +65,9 @@ jobs:
|
|||
- name: check_licenses
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/check_licenses.py
|
||||
|
||||
- name: check_json_duplicate_keys
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/check_json_duplicate_keys.py
|
||||
|
||||
- name: check_provider_folders_documented
|
||||
run: uv run --no-sync python ./tests/code_coverage_tests/check_provider_folders_documented.py
|
||||
|
||||
|
|
|
|||
|
|
@ -5,7 +5,6 @@ This script:
|
|||
1. Extracts all endpoint entries from the "Supported Endpoints" section of sidebars.js
|
||||
2. Validates that each endpoint has a corresponding entry in the "endpoints" object of provider_endpoints_support.json
|
||||
3. Checks that the "docs_label" field is present in each endpoint definition
|
||||
4. Checks that no object in the file declares the same key twice
|
||||
"""
|
||||
|
||||
import json
|
||||
|
|
@ -21,12 +20,6 @@ class MissingEndpointDefinitionError(Exception):
|
|||
pass
|
||||
|
||||
|
||||
class DuplicateKeyError(Exception):
|
||||
"""Raised when provider_endpoints_support.json declares the same key twice in one object."""
|
||||
|
||||
pass
|
||||
|
||||
|
||||
def get_repo_root() -> Path:
|
||||
"""Get the repository root directory."""
|
||||
# Check if litellm directory exists in current working directory
|
||||
|
|
@ -118,36 +111,6 @@ def load_provider_endpoints_file() -> Dict:
|
|||
return json.load(f)
|
||||
|
||||
|
||||
def find_duplicate_keys() -> List[Tuple[str, int]]:
|
||||
"""
|
||||
Find keys declared more than once inside the same JSON object.
|
||||
|
||||
json.load() keeps the last of a repeated key and reports no error, so an edit to
|
||||
the earlier copy is dropped without a trace: the file parses, CI passes, and the
|
||||
added fields simply are not there. That is why this check re-reads the raw file
|
||||
with object_pairs_hook instead of inspecting the parsed dict - the parsed dict
|
||||
has already lost the evidence.
|
||||
|
||||
Returns a list of (key, occurrences).
|
||||
"""
|
||||
repo_root = get_repo_root()
|
||||
file_path = repo_root / "provider_endpoints_support.json"
|
||||
|
||||
duplicates: List[Tuple[str, int]] = []
|
||||
|
||||
def collect(pairs):
|
||||
counts: Dict[str, int] = {}
|
||||
for key, _ in pairs:
|
||||
counts[key] = counts.get(key, 0) + 1
|
||||
duplicates.extend((key, n) for key, n in counts.items() if n > 1)
|
||||
return dict(pairs)
|
||||
|
||||
with open(file_path, "r") as f:
|
||||
json.loads(f.read(), object_pairs_hook=collect)
|
||||
|
||||
return duplicates
|
||||
|
||||
|
||||
def get_defined_endpoints(data: Dict) -> Dict[str, Dict]:
|
||||
"""Get all endpoint definitions from provider_endpoints_support.json."""
|
||||
return data.get("endpoints", {})
|
||||
|
|
@ -249,28 +212,6 @@ def main():
|
|||
|
||||
has_errors = False
|
||||
|
||||
# Test 0: Check for duplicate keys before anything reads the parsed file
|
||||
print("\n🔑 Test 0: Checking for duplicate keys...")
|
||||
duplicate_keys = find_duplicate_keys()
|
||||
|
||||
if duplicate_keys:
|
||||
error_msg = "\n❌ ERROR: The following keys are declared more than once in the same object:\n"
|
||||
error_msg += "=" * 70 + "\n"
|
||||
for key, occurrences in duplicate_keys:
|
||||
error_msg += f" - {key} ({occurrences} times)\n"
|
||||
error_msg += "\n" + "=" * 70 + "\n"
|
||||
error_msg += (
|
||||
"\n💡 Only the last copy survives parsing. Anything added to an earlier\n"
|
||||
)
|
||||
error_msg += " copy is silently discarded. Merge the copies into one entry.\n"
|
||||
print(error_msg)
|
||||
raise DuplicateKeyError(
|
||||
f"Duplicate key(s) in provider_endpoints_support.json: "
|
||||
f"{', '.join(key for key, _ in duplicate_keys)}"
|
||||
)
|
||||
|
||||
print("✅ No duplicate keys found!")
|
||||
|
||||
# Load provider_endpoints_support.json
|
||||
data = load_provider_endpoints_file()
|
||||
defined_endpoints = get_defined_endpoints(data)
|
||||
|
|
|
|||
98
tests/code_coverage_tests/check_json_duplicate_keys.py
Normal file
98
tests/code_coverage_tests/check_json_duplicate_keys.py
Normal file
|
|
@ -0,0 +1,98 @@
|
|||
"""
|
||||
Fail if a hand-maintained JSON config file declares the same key twice inside one object.
|
||||
|
||||
Why this needs its own check:
|
||||
|
||||
json.load() keeps the LAST of a repeated key and reports no error. So when two
|
||||
copies of the same object exist and they are not identical, the earlier copy is
|
||||
dropped without a trace - the file parses, CI passes, and whatever fields only
|
||||
the earlier copy had are simply not there. Nobody is told.
|
||||
|
||||
Every existing check reads the PARSED file, and the duplicate is already gone by
|
||||
then. This check re-reads the raw text with object_pairs_hook, which is the only
|
||||
place the evidence still exists.
|
||||
|
||||
Add a file to FILES_TO_CHECK when it is edited by hand and read by code at runtime.
|
||||
Generated files and lockfiles do not belong here.
|
||||
"""
|
||||
|
||||
import json
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from typing import Dict, List, Tuple
|
||||
|
||||
# Repo-root-relative paths of hand-maintained JSON that code reads at runtime.
|
||||
FILES_TO_CHECK = [
|
||||
"provider_endpoints_support.json",
|
||||
]
|
||||
|
||||
|
||||
def get_repo_root() -> Path:
|
||||
return Path(__file__).parent.parent.parent
|
||||
|
||||
|
||||
def find_duplicate_keys(file_path: Path) -> List[Tuple[str, int]]:
|
||||
"""Return [(key, occurrences), ...] for keys repeated inside the same object."""
|
||||
duplicates: List[Tuple[str, int]] = []
|
||||
|
||||
def collect(pairs):
|
||||
counts: Dict[str, int] = {}
|
||||
for key, _ in pairs:
|
||||
counts[key] = counts.get(key, 0) + 1
|
||||
duplicates.extend((key, n) for key, n in counts.items() if n > 1)
|
||||
return dict(pairs)
|
||||
|
||||
with open(file_path, "r") as f:
|
||||
json.loads(f.read(), object_pairs_hook=collect)
|
||||
|
||||
return duplicates
|
||||
|
||||
|
||||
def main() -> None:
|
||||
repo_root = get_repo_root()
|
||||
failures: List[str] = []
|
||||
checked = 0
|
||||
|
||||
print("🔑 Checking hand-maintained JSON config for duplicate keys...\n")
|
||||
|
||||
for rel_path in FILES_TO_CHECK:
|
||||
file_path = repo_root / rel_path
|
||||
|
||||
if not file_path.exists():
|
||||
# A path that no longer exists means this check is silently protecting
|
||||
# nothing, so say so instead of passing.
|
||||
failures.append(f"{rel_path}: file not found at {file_path}")
|
||||
continue
|
||||
|
||||
checked += 1
|
||||
duplicates = find_duplicate_keys(file_path)
|
||||
|
||||
if duplicates:
|
||||
for key, occurrences in duplicates:
|
||||
failures.append(
|
||||
f"{rel_path}: '{key}' is declared {occurrences} times inside the "
|
||||
"same object; only the last one survives json.load()"
|
||||
)
|
||||
else:
|
||||
print(f" ✅ {rel_path}")
|
||||
|
||||
if not checked:
|
||||
# Scanning zero files must never look like success.
|
||||
print("\n❌ No files were checked - FILES_TO_CHECK is empty or all paths are stale")
|
||||
sys.exit(1)
|
||||
|
||||
if failures:
|
||||
print("\n❌ Duplicate keys found:\n")
|
||||
for line in failures:
|
||||
print(f" - {line}")
|
||||
print(
|
||||
"\nMerge the duplicate objects into one, keeping every field that appears "
|
||||
"in either copy."
|
||||
)
|
||||
sys.exit(1)
|
||||
|
||||
print(f"\n✅ {checked} file(s) checked, no duplicate keys found")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
Loading…
Add table
Reference in a new issue