diff --git a/src/basic_memory/cli/auto_update.py b/src/basic_memory/cli/auto_update.py index 48456807a..4a057ff64 100644 --- a/src/basic_memory/cli/auto_update.py +++ b/src/basic_memory/cli/auto_update.py @@ -27,6 +27,10 @@ BREW_UPGRADE_TIMEOUT_SECONDS = 600 +class HomebrewCheckError(RuntimeError): + """Raised when `brew outdated` could not determine whether an update exists.""" + + class InstallSource(str, Enum): """How the running CLI appears to have been installed.""" @@ -127,19 +131,36 @@ def _version_from_pypi() -> str: def _check_homebrew_update_available(silent: bool) -> tuple[bool, str | None]: - """Check whether Homebrew reports an outdated basic-memory formula.""" - result = _run_subprocess( - ["brew", "outdated", "--quiet", PACKAGE_NAME], - timeout_seconds=BREW_OUTDATED_TIMEOUT_SECONDS, - silent=silent, - capture_output=True, - ) - # Trigger: brew outdated exits 1 when the formula IS outdated (with name on stdout). - # Why: non-zero exit here means "outdated", not "error". - # Outcome: check stdout for the package name to determine outdated status. + """Check whether Homebrew reports an outdated basic-memory formula. + + Raises: + HomebrewCheckError: brew could not answer the question (brew missing, + untrusted or stale tap, network failure, timeout). + """ + try: + result = _run_subprocess( + ["brew", "outdated", "--quiet", PACKAGE_NAME], + timeout_seconds=BREW_OUTDATED_TIMEOUT_SECONDS, + silent=silent, + capture_output=True, + ) + except (OSError, subprocess.SubprocessError) as exc: + raise HomebrewCheckError(f"could not run `brew outdated`: {exc}") from exc + + # Trigger: brew outdated exits 1 both when the formula IS outdated (name on + # stdout) and when the check failed outright (empty stdout, reason on stderr). + # Why: reading a failed check as "not outdated" reports a stale install as up + # to date, which silently pins the user to an old version. + # Outcome: trust an empty stdout only when brew exited cleanly; otherwise the + # check is unanswered and the caller must find the answer elsewhere. stdout = (result.stdout or "").strip() - is_outdated = PACKAGE_NAME in stdout - return is_outdated, None + if PACKAGE_NAME in stdout: + return True, None + if result.returncode == 0: + return False, None + + stderr = (result.stderr or "").strip() + raise HomebrewCheckError(stderr or f"`brew outdated` exited {result.returncode}") def _check_pypi_update_available() -> tuple[bool, str]: @@ -246,8 +267,21 @@ def run_auto_update( try: # --- Availability check --- latest_version: str | None = None + homebrew_check_error: str | None = None if source == InstallSource.HOMEBREW: - update_available, latest_version = _check_homebrew_update_available(silent=silent) + try: + update_available, latest_version = _check_homebrew_update_available(silent=silent) + except HomebrewCheckError as exc: + # Trigger: brew cannot answer (missing/untrusted tap, no brew, network). + # Why: an unanswered check must never be reported as up to date. PyPI is + # sound for the negative answer -- the tap can only lag PyPI, so "nothing + # newer exists" holds. It is NOT sound for installing: release.yml + # publishes to PyPI in `release`, and the homebrew formula job `needs: + # release`, so a newer PyPI version may not be installable via brew yet. + # Outcome: ask PyPI, but remember the answer came from there. + homebrew_check_error = str(exc) + logger.warning(f"Homebrew update check failed, falling back to PyPI: {exc}") + update_available, latest_version = _check_pypi_update_available() else: update_available, latest_version = _check_pypi_update_available() @@ -290,6 +324,26 @@ def run_auto_update( ), ) + if homebrew_check_error is not None: + # Trigger: availability was inferred from PyPI because brew could not answer. + # Why: the tap may not carry this version yet, and whatever hid the brew + # answer (untrusted tap, brew missing) will also block `brew upgrade`. + # Outcome: report the update and the reason instead of running a doomed + # upgrade; the user resolves the brew problem and upgrades deliberately. + return AutoUpdateResult( + status=AutoUpdateStatus.UPDATE_AVAILABLE, + source=source, + checked=True, + update_available=True, + updated=False, + latest_version=latest_version, + message=( + f"Update available (latest: {latest_version or 'unknown'}), but the " + f"Homebrew check failed: {homebrew_check_error} " + f"{_manual_update_hint(source)}" + ), + ) + # --- Automatic install --- command = ( ["uv", "tool", "upgrade", PACKAGE_NAME] diff --git a/tests/cli/test_auto_update.py b/tests/cli/test_auto_update.py index 574b7aefd..6e2a527ed 100644 --- a/tests/cli/test_auto_update.py +++ b/tests/cli/test_auto_update.py @@ -4,15 +4,18 @@ import subprocess import sys +import urllib.error from datetime import datetime, timedelta, timezone from io import StringIO from typing import Any, cast +import pytest from rich.console import Console from basic_memory.cli.auto_update import ( AutoUpdateResult, AutoUpdateStatus, + HomebrewCheckError, InstallSource, _check_homebrew_update_available, _is_interactive_session, @@ -23,6 +26,11 @@ ) from basic_memory.config import BasicMemoryConfig +UNTRUSTED_TAP_STDERR = ( + "Error: Refusing to load formula basicmachines-co/basic-memory/basic-memory " + "from untrusted tap basicmachines-co/basic-memory." +) + class StubConfigManager: """Simple in-memory ConfigManager stub for updater tests.""" @@ -138,11 +146,19 @@ def test_force_bypasses_auto_update_disabled(monkeypatch, tmp_path): def test_check_homebrew_update_available_exit_code_1_means_outdated(monkeypatch): - """brew outdated exits 1 when the formula is outdated, not on error.""" + """brew outdated exits 1 when the formula is outdated, not on error. + + Exit 1 is shared with the failure case, so stdout is the discriminator: the + tap-qualified formula name means outdated. brew also writes progress chatter + to stderr on this path, so a non-empty stderr must not be read as an error. + """ def _fake_run(command, **kwargs): return subprocess.CompletedProcess( - command, 1, stdout="basicmachines-co/basic-memory/basic-memory\n", stderr="" + command, + 1, + stdout="basicmachines-co/basic-memory/basic-memory\n", + stderr="==> Downloading Homebrew API data", ) monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _fake_run) @@ -161,6 +177,149 @@ def _fake_run(command, **kwargs): assert is_outdated is False +def test_check_homebrew_update_available_failed_check_is_not_up_to_date(monkeypatch): + """A failed `brew outdated` exits non-zero with empty stdout -- it is not an answer.""" + + def _fake_run(command, **kwargs): + return subprocess.CompletedProcess(command, 1, stdout="", stderr=UNTRUSTED_TAP_STDERR) + + monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _fake_run) + + with pytest.raises(HomebrewCheckError) as excinfo: + _check_homebrew_update_available(silent=False) + + assert "untrusted tap" in str(excinfo.value) + + +def test_check_homebrew_update_available_reports_missing_brew(monkeypatch): + """brew not on PATH must surface as an unanswered check, not as up to date.""" + + def _raise_not_found(command, **kwargs): + raise FileNotFoundError(command[0]) + + monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _raise_not_found) + + with pytest.raises(HomebrewCheckError): + _check_homebrew_update_available(silent=False) + + +def test_failed_homebrew_check_falls_back_to_pypi(monkeypatch, tmp_path): + """Regression: a failed brew check reported the install as up to date. + + The untrusted tap is one trigger; a stale tap, a missing formula, or a network + failure produce the same empty-stdout shape. + """ + config = _base_config(tmp_path) + manager = StubConfigManager(config) + + def _fake_run(command, **kwargs): + assert command[:2] == ["brew", "outdated"] + return subprocess.CompletedProcess(command, 1, stdout="", stderr=UNTRUSTED_TAP_STDERR) + + monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _fake_run) + monkeypatch.setattr( + "basic_memory.cli.auto_update._check_pypi_update_available", + lambda: (True, "9.9.9"), + ) + + result = run_auto_update( + check_only=True, + config_manager=_config_manager(manager), + executable="/opt/homebrew/Cellar/basic-memory/0.18.0/bin/python", + ) + + assert result.status == AutoUpdateStatus.UPDATE_AVAILABLE + assert result.latest_version == "9.9.9" + assert "brew upgrade basic-memory" in (result.message or "") + + +def test_failed_homebrew_check_does_not_auto_upgrade(monkeypatch, tmp_path): + """Availability inferred from PyPI must not trigger an automatic `brew upgrade`. + + release.yml publishes to PyPI in the `release` job and the Homebrew formula job + `needs: release`, so PyPI can carry a version the tap cannot install yet. On top + of that, whatever hid the brew answer (untrusted tap, missing brew) also blocks + the upgrade -- so acting on the PyPI answer runs a doomed command. + """ + config = _base_config(tmp_path) + manager = StubConfigManager(config) + calls: list[list[str]] = [] + + def _fake_run(command, **kwargs): + calls.append(command) + return subprocess.CompletedProcess(command, 1, stdout="", stderr=UNTRUSTED_TAP_STDERR) + + monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _fake_run) + monkeypatch.setattr( + "basic_memory.cli.auto_update._check_pypi_update_available", + lambda: (True, "9.9.9"), + ) + + result = run_auto_update( + config_manager=_config_manager(manager), + executable="/opt/homebrew/Cellar/basic-memory/0.18.0/bin/python", + ) + + assert result.status == AutoUpdateStatus.UPDATE_AVAILABLE + assert result.updated is False + assert result.latest_version == "9.9.9" + assert ["brew", "upgrade", "basic-memory"] not in calls + assert "untrusted tap" in (result.message or "") + assert "brew upgrade basic-memory" in (result.message or "") + + +def test_failed_homebrew_check_still_trusts_a_pypi_negative(monkeypatch, tmp_path): + """The fallback stays authoritative for "nothing newer exists". + + The tap can only lag PyPI, never lead it, so a PyPI "up to date" is sound even + when brew could not answer. Only the positive answer is unsafe to act on. + """ + config = _base_config(tmp_path) + manager = StubConfigManager(config) + + def _fake_run(command, **kwargs): + return subprocess.CompletedProcess(command, 1, stdout="", stderr=UNTRUSTED_TAP_STDERR) + + monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _fake_run) + monkeypatch.setattr( + "basic_memory.cli.auto_update._check_pypi_update_available", + lambda: (False, "0.0.0"), + ) + + result = run_auto_update( + config_manager=_config_manager(manager), + executable="/opt/homebrew/Cellar/basic-memory/0.18.0/bin/python", + ) + + assert result.status == AutoUpdateStatus.UP_TO_DATE + assert result.update_available is False + + +def test_failed_homebrew_check_reports_failure_when_pypi_is_unreachable(monkeypatch, tmp_path): + """With neither source able to answer, report the failure rather than success.""" + config = _base_config(tmp_path) + manager = StubConfigManager(config) + + def _fake_run(command, **kwargs): + return subprocess.CompletedProcess(command, 1, stdout="", stderr=UNTRUSTED_TAP_STDERR) + + def _pypi_unreachable(): + raise urllib.error.URLError("network unreachable") + + monkeypatch.setattr("basic_memory.cli.auto_update._run_subprocess", _fake_run) + monkeypatch.setattr( + "basic_memory.cli.auto_update._check_pypi_update_available", _pypi_unreachable + ) + + result = run_auto_update( + config_manager=_config_manager(manager), + executable="/opt/homebrew/Cellar/basic-memory/0.18.0/bin/python", + ) + + assert result.status == AutoUpdateStatus.FAILED + assert result.update_available is False + + def test_preload_lazy_console_modules_imports_deferred_modules(monkeypatch): # Regression: the in-place upgrade deletes the running install's files, so # any module rich/typer defers until print time must already be loaded or