From f7fc41c94ce9d2156b58df750d7a1fce516144db Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 15 Jul 2026 18:23:28 -0700 Subject: [PATCH] fix(cli): surface clean errors instead of raw tracebacks in lite up/down load_json_or_empty and read_backup both delegate to pydantic's validate_json, which raises ValidationError on invalid JSON or a non-object root -- neither up() nor down() caught it, so a corrupt settings or backup file surfaced an unformatted Python traceback instead of a clean CLI error. Both now convert to UpError, and down() (previously uncaught entirely) and up()'s teardown path now handle it. restore_claude_settings also gained a parent.mkdir guard before rewriting CLAUDE_SETTINGS_PATH: if ~/.claude/ was removed while `lite up` was running, the restore would crash before deleting the backup file, permanently stranding it and breaking every future `lite down`. --- litellm/proxy/client/cli/commands/up.py | 28 ++++++++-- .../proxy/client/cli/test_up_commands.py | 51 +++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/litellm/proxy/client/cli/commands/up.py b/litellm/proxy/client/cli/commands/up.py index 61b649b7415..dc9157d7ca4 100644 --- a/litellm/proxy/client/cli/commands/up.py +++ b/litellm/proxy/client/cli/commands/up.py @@ -13,7 +13,7 @@ from types import FrameType from typing import IO, Iterator, Mapping import click -from pydantic import JsonValue, TypeAdapter +from pydantic import JsonValue, TypeAdapter, ValidationError from litellm.litellm_core_utils.cli_token_utils import is_cli_token_fresh @@ -52,7 +52,10 @@ def load_json_or_empty(path: Path) -> dict[str, JsonValue]: content = f.read() if not content.strip(): return {} - return _SETTINGS_ADAPTER.validate_json(content) + try: + return _SETTINGS_ADAPTER.validate_json(content) + except ValidationError: + raise UpError(f"{path} contains invalid JSON (or its root is not an object); cannot proceed safely.") def merge_claude_settings( @@ -105,7 +108,11 @@ def read_backup(backup_path: Path | None = None) -> BackupRecord | None: if not path.exists(): return None with open(path, "r") as f: - return _BACKUP_RECORD_ADAPTER.validate_json(f.read()) + content = f.read() + try: + return _BACKUP_RECORD_ADAPTER.validate_json(content) + except ValidationError: + raise UpError(f"{path} contains invalid or unexpected JSON; cannot restore from it safely.") def restore_claude_settings(settings_path: Path | None = None, backup_path: Path | None = None) -> BackupRecord | None: @@ -119,6 +126,7 @@ def restore_claude_settings(settings_path: Path | None = None, backup_path: Path if record is None: return None if record.existed and record.content is not None: + resolved_settings_path.parent.mkdir(parents=True, exist_ok=True) with open(resolved_settings_path, "w") as f: json.dump(record.content, f, indent=2) elif resolved_settings_path.exists(): @@ -228,8 +236,15 @@ def up(ctx: click.Context) -> None: stop_event.set() def _restore_once() -> None: - if restored.acquire(blocking=False): + if not restored.acquire(blocking=False): + return + try: _restore_and_report() + except UpError as e: + # Runs from atexit/a signal handler, outside Click's own exception + # handling -- raising here would only produce an unhandled-exception + # warning on stderr, not a clean message. + click.echo(str(e), err=True) signal.signal(signal.SIGINT, _handle_signal) signal.signal(signal.SIGTERM, _handle_signal) @@ -246,7 +261,10 @@ def down() -> None: Use this after a `lite up` process was killed uncleanly (e.g. `kill -9`) instead of stopped with Ctrl-C. """ - _restore_and_report() + try: + _restore_and_report() + except UpError as e: + raise click.ClickException(str(e)) __all__ = [ diff --git a/tests/test_litellm/proxy/client/cli/test_up_commands.py b/tests/test_litellm/proxy/client/cli/test_up_commands.py index a2986027b98..1b182553644 100644 --- a/tests/test_litellm/proxy/client/cli/test_up_commands.py +++ b/tests/test_litellm/proxy/client/cli/test_up_commands.py @@ -89,6 +89,18 @@ class TestLoadJsonOrEmpty: path.write_text(json.dumps({"theme": "dark"})) assert load_json_or_empty(path) == {"theme": "dark"} + def test_raises_clean_error_on_invalid_json(self, tmp_path): + path = tmp_path / "settings.json" + path.write_text("not json at all {{{") + with pytest.raises(UpError, match="invalid JSON"): + load_json_or_empty(path) + + def test_raises_clean_error_on_non_object_root(self, tmp_path): + path = tmp_path / "settings.json" + path.write_text(json.dumps([1, 2, 3])) + with pytest.raises(UpError, match="invalid JSON"): + load_json_or_empty(path) + class TestBackupRoundTrip: def test_restores_original_content_when_file_existed(self, monkeypatch, tmp_path): @@ -123,6 +135,26 @@ class TestBackupRoundTrip: assert restore_claude_settings() is None assert not settings_path.exists() + def test_recreates_claude_dir_if_it_was_deleted_while_up_was_running(self, monkeypatch, tmp_path): + """If ~/.claude/ is removed while `lite up` holds it open, restoring must recreate the + directory rather than crash with FileNotFoundError and strand the backup file, which + would otherwise permanently break every future `lite down`.""" + claude_dir = tmp_path / "claude_dir" + settings_path = claude_dir / "settings.json" + backup_path = tmp_path / "backup.json" + monkeypatch.setattr(up_module, "CLAUDE_SETTINGS_PATH", settings_path) + monkeypatch.setattr(up_module, "BACKUP_PATH", backup_path) + original = {"theme": "dark"} + claude_dir.mkdir(parents=True) + write_backup(BackupRecord(existed=True, content=original)) + shutil.rmtree(claude_dir) + + restored = restore_claude_settings() + + assert restored is not None + assert json.loads(settings_path.read_text()) == original + assert not backup_path.exists() + def test_read_backup_round_trips_write_backup(self, monkeypatch, tmp_path): _patch_paths(monkeypatch, tmp_path) write_backup(BackupRecord(existed=True, content={"a": 1})) @@ -132,6 +164,14 @@ class TestBackupRoundTrip: _patch_paths(monkeypatch, tmp_path) assert read_backup() is None + def test_read_backup_raises_clean_error_on_corrupt_content(self, monkeypatch, tmp_path): + _settings_path, backup_path = _patch_paths(monkeypatch, tmp_path) + backup_path.parent.mkdir(parents=True, exist_ok=True) + backup_path.write_text("not json at all {{{") + + with pytest.raises(UpError, match="invalid or unexpected JSON"): + read_backup() + def test_write_backup_restricts_permissions_for_a_new_file(self, monkeypatch, tmp_path): _settings_path, backup_path = _patch_paths(monkeypatch, tmp_path) write_backup(BackupRecord(existed=True, content={"a": 1})) @@ -345,3 +385,14 @@ class TestDownCommand: assert result.exit_code == 0, result.output assert "Nothing to restore." in result.output + + def test_surfaces_clean_error_on_a_corrupt_backup_file(self, monkeypatch, tmp_path): + _settings_path, backup_path = _patch_paths(monkeypatch, tmp_path) + backup_path.parent.mkdir(parents=True, exist_ok=True) + backup_path.write_text("not json at all {{{") + + result = self.runner.invoke(down) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "invalid or unexpected JSON" in result.output