From 9856bdf22f1ffe74de513506333a4d5bfa1c4ab2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 22:04:23 +0000 Subject: [PATCH] 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. --- .github/workflows/test-code-quality.yml | 3 + .../check_endpoint_coverage.py | 59 ----------- .../check_json_duplicate_keys.py | 98 +++++++++++++++++++ 3 files changed, 101 insertions(+), 59 deletions(-) create mode 100644 tests/code_coverage_tests/check_json_duplicate_keys.py diff --git a/.github/workflows/test-code-quality.yml b/.github/workflows/test-code-quality.yml index d02f5878396..a963ee75257 100644 --- a/.github/workflows/test-code-quality.yml +++ b/.github/workflows/test-code-quality.yml @@ -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 diff --git a/tests/code_coverage_tests/check_endpoint_coverage.py b/tests/code_coverage_tests/check_endpoint_coverage.py index b0e7c9df3a2..2d46d1ab469 100644 --- a/tests/code_coverage_tests/check_endpoint_coverage.py +++ b/tests/code_coverage_tests/check_endpoint_coverage.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) diff --git a/tests/code_coverage_tests/check_json_duplicate_keys.py b/tests/code_coverage_tests/check_json_duplicate_keys.py new file mode 100644 index 00000000000..96969c5e1ab --- /dev/null +++ b/tests/code_coverage_tests/check_json_duplicate_keys.py @@ -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()