From fbef12baa98ef6e345bbc932dc3987112786b53d Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 15 Jul 2026 11:58:17 -0700 Subject: [PATCH] fix(cli): address greptile review feedback on autoroute PR - terminate the ephemeral proxy child process when its health check fails, instead of leaking an orphaned, unrecoverable process bound to the port - replace bare assert isinstance checks (no-ops under python -O) with click.ClickException in the model-groups list and configure wizard code paths - close launch_proxy's log file handle once the child process has inherited its fd, instead of leaking it - add build_generated_proxy_config to config.py's __all__ --- .../proxy/client/cli/commands/autoroute/commands.py | 1 + .../proxy/client/cli/commands/autoroute/config.py | 1 + .../proxy/client/cli/commands/autoroute/process.py | 12 ++++++------ .../proxy/client/cli/commands/autoroute/wizard.py | 5 ++++- litellm/proxy/client/cli/commands/model_groups.py | 5 ++++- .../proxy/client/cli/autoroute/test_commands.py | 3 +++ .../proxy/client/cli/autoroute/test_wizard.py | 8 ++++++++ .../proxy/client/cli/test_model_groups_commands.py | 10 ++++++++++ 8 files changed, 37 insertions(+), 8 deletions(-) diff --git a/litellm/proxy/client/cli/commands/autoroute/commands.py b/litellm/proxy/client/cli/commands/autoroute/commands.py index b70f7bcfab5..88dcb6f554a 100644 --- a/litellm/proxy/client/cli/commands/autoroute/commands.py +++ b/litellm/proxy/client/cli/commands/autoroute/commands.py @@ -92,6 +92,7 @@ def up() -> None: try: poll_liveliness(base_url, LOG_PATH, process) except ProcessLaunchError as e: + terminate(process.pid) clear_pid_record() raise click.ClickException(str(e)) diff --git a/litellm/proxy/client/cli/commands/autoroute/config.py b/litellm/proxy/client/cli/commands/autoroute/config.py index 1d18f497324..fe2c4e467e7 100644 --- a/litellm/proxy/client/cli/commands/autoroute/config.py +++ b/litellm/proxy/client/cli/commands/autoroute/config.py @@ -229,6 +229,7 @@ __all__ = [ "SemanticMatching", "SemanticMatchingChoice", "build_generated_model_list", + "build_generated_proxy_config", "chat_models", "embedding_models", "parse_discovered_models", diff --git a/litellm/proxy/client/cli/commands/autoroute/process.py b/litellm/proxy/client/cli/commands/autoroute/process.py index c8de68307b9..000ccf49623 100644 --- a/litellm/proxy/client/cli/commands/autoroute/process.py +++ b/litellm/proxy/client/cli/commands/autoroute/process.py @@ -66,12 +66,12 @@ def allocate_free_port() -> int: def launch_proxy(config_path: Path, port: int, log_path: Path) -> "subprocess.Popen[bytes]": log_path.parent.mkdir(parents=True, exist_ok=True) - log_file = open(log_path, "w") - return subprocess.Popen( - [sys.executable, "-m", "litellm.proxy.proxy_cli", "--config", str(config_path), "--port", str(port)], - stdout=log_file, - stderr=subprocess.STDOUT, - ) + with open(log_path, "w") as log_file: + return subprocess.Popen( + [sys.executable, "-m", "litellm.proxy.proxy_cli", "--config", str(config_path), "--port", str(port)], + stdout=log_file, + stderr=subprocess.STDOUT, + ) def _tail(log_path: Path, lines: int = 40) -> str: diff --git a/litellm/proxy/client/cli/commands/autoroute/wizard.py b/litellm/proxy/client/cli/commands/autoroute/wizard.py index b9f8a5408b5..89b53d8f9fd 100644 --- a/litellm/proxy/client/cli/commands/autoroute/wizard.py +++ b/litellm/proxy/client/cli/commands/autoroute/wizard.py @@ -70,7 +70,10 @@ def run_configure_wizard(ctx: click.Context) -> Path: client = Client(base_url=base_url, api_key=api_key) raw_groups = client.model_groups.info() - assert isinstance(raw_groups, list) + if not isinstance(raw_groups, list): + raise click.ClickException( + f"Unexpected response from /model_group/info: expected a list, got {type(raw_groups).__name__}" + ) discovered = parse_discovered_models(raw_groups) chat_pool = chat_models(discovered) embedding_pool = embedding_models(discovered) diff --git a/litellm/proxy/client/cli/commands/model_groups.py b/litellm/proxy/client/cli/commands/model_groups.py index 629a0b5aaf1..7de959a9b78 100644 --- a/litellm/proxy/client/cli/commands/model_groups.py +++ b/litellm/proxy/client/cli/commands/model_groups.py @@ -29,7 +29,10 @@ def list_model_groups(ctx: click.Context, output_format: Literal["table", "json" """List model groups accessible to your key, with mode and pricing""" client = create_client(ctx) groups = client.model_groups.info() - assert isinstance(groups, list) + if not isinstance(groups, list): + raise click.ClickException( + f"Unexpected response from /model_group/info: expected a list, got {type(groups).__name__}" + ) if output_format == "json": rich.print_json(data=groups) diff --git a/tests/test_litellm/proxy/client/cli/autoroute/test_commands.py b/tests/test_litellm/proxy/client/cli/autoroute/test_commands.py index 9737e49c4ae..26c34138683 100644 --- a/tests/test_litellm/proxy/client/cli/autoroute/test_commands.py +++ b/tests/test_litellm/proxy/client/cli/autoroute/test_commands.py @@ -122,6 +122,7 @@ class TestUpCommand: claude_settings_path.write_text(json.dumps(original_settings)) fake_process = FakeProcess(pid=555) + terminate_calls = [] def _raise_launch_error(*args, **kwargs): raise ProcessLaunchError("boom: proxy never became healthy") @@ -129,12 +130,14 @@ class TestUpCommand: monkeypatch.setattr(commands_module, "launch_proxy", lambda *a, **k: fake_process) monkeypatch.setattr(commands_module, "poll_liveliness", _raise_launch_error) monkeypatch.setattr(commands_module, "allocate_free_port", lambda: 12345) + monkeypatch.setattr(commands_module, "terminate", lambda pid, **k: terminate_calls.append(pid)) monkeypatch.setattr(commands_module.secrets, "token_urlsafe", lambda n: "fixed-master-key") result = self.runner.invoke(up) assert result.exit_code != 0 assert "boom" in result.output + assert terminate_calls == [555] assert not pid_record_path.exists() assert not backup_path.exists() assert json.loads(claude_settings_path.read_text()) == original_settings diff --git a/tests/test_litellm/proxy/client/cli/autoroute/test_wizard.py b/tests/test_litellm/proxy/client/cli/autoroute/test_wizard.py index e14e93cf5c6..75431a2c588 100644 --- a/tests/test_litellm/proxy/client/cli/autoroute/test_wizard.py +++ b/tests/test_litellm/proxy/client/cli/autoroute/test_wizard.py @@ -183,6 +183,14 @@ class TestRunConfigureWizardNoChatModels: assert "no chat-capable models" in result.output.lower() assert not config_path.exists() + def test_surfaces_clean_error_when_response_is_not_a_list(self, tmp_path): + result, config_path = _run(tmp_path, {"data": CHAT_AND_EMBEDDING_GROUPS}, {}, input_str="") + + assert result.exit_code != 0 + assert result.exception is None or not isinstance(result.exception, AssertionError) + assert "Unexpected response from /model_group/info" in result.output + assert not config_path.exists() + class TestRunConfigureWizardNotInteractive: def test_fails_cleanly_when_not_a_tty(self, tmp_path): diff --git a/tests/test_litellm/proxy/client/cli/test_model_groups_commands.py b/tests/test_litellm/proxy/client/cli/test_model_groups_commands.py index d7b5b9dede4..c2809a90ba2 100644 --- a/tests/test_litellm/proxy/client/cli/test_model_groups_commands.py +++ b/tests/test_litellm/proxy/client/cli/test_model_groups_commands.py @@ -102,3 +102,13 @@ def test_list_error_handling(mock_client, cli_runner): assert result.exit_code != 0 assert "API Error" in str(result.exception) + + +def test_list_surfaces_clean_error_when_response_is_not_a_list(mock_client, cli_runner): + mock_client.return_value.model_groups.info.return_value = {"data": SAMPLE_MODEL_GROUPS} + + result = cli_runner.invoke(cli, ["model-groups", "list"]) + + assert result.exit_code != 0 + assert result.exception is None or not isinstance(result.exception, AssertionError) + assert "Unexpected response from /model_group/info" in result.output