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__
This commit is contained in:
Krrish Dholakia 2026-07-15 11:58:17 -07:00
parent 851b38fc4a
commit fbef12baa9
8 changed files with 37 additions and 8 deletions

View file

@ -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))

View file

@ -229,6 +229,7 @@ __all__ = [
"SemanticMatching",
"SemanticMatchingChoice",
"build_generated_model_list",
"build_generated_proxy_config",
"chat_models",
"embedding_models",
"parse_discovered_models",

View file

@ -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:

View file

@ -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)

View file

@ -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)

View file

@ -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

View file

@ -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):

View file

@ -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