diff --git a/dailybot_cli/commands/upgrade.py b/dailybot_cli/commands/upgrade.py index 981ecc7..8bb1950 100644 --- a/dailybot_cli/commands/upgrade.py +++ b/dailybot_cli/commands/upgrade.py @@ -38,11 +38,21 @@ ) -def _build_upgrade_argv(method: str) -> list[str] | None: +def _build_upgrade_argv(method: str, latest: str | None = None) -> list[str] | None: """Return argv for `subprocess.run` to perform the upgrade, or None. - `None` means "we don't auto-run for this method — print the command - instead". `homebrew`, `binary`, `editable`, and `unknown` fall in + *latest* is the version string we resolved against PyPI's JSON API. When + known and the method is ``pip``, we pin ``dailybot-cli==`` so pip + can't silently no-op. This guards against a real-world failure mode + observed after a fresh release: the JSON API returned ``1.7.0`` but pip's + package index cache (or the user-fallback resolver) still saw ``1.6.1`` as + the latest available, so ``pip install --upgrade dailybot-cli`` exited 0 + with "Requirement already satisfied" and we falsely reported success. + Pinning the exact version forces pip to fetch from PyPI regardless of any + cached resolution. + + ``None`` means "we don't auto-run for this method — print the command + instead". ``homebrew``, ``binary``, ``editable``, and ``unknown`` fall in that category for safety reasons documented above. """ if method == "pipx": @@ -60,11 +70,42 @@ def _build_upgrade_argv(method: str) -> list[str] | None: if method == "pip": # Use the same Python that's running the CLI. Reliable across venvs. - return [sys.executable, "-m", "pip", "install", "--upgrade", _PACKAGE] + target: str = f"{_PACKAGE}=={latest}" if latest else _PACKAGE + return [sys.executable, "-m", "pip", "install", "--upgrade", target] return None # homebrew, binary, editable, unknown +def _query_installed_version() -> str | None: + """Return the currently installed ``dailybot-cli`` version as a string. + + Spawns a fresh Python subprocess so the answer reflects the on-disk state + *after* pip finished, not whatever the running interpreter cached at + startup. Returns ``None`` when the package can't be located (e.g. it was + actually uninstalled, or the subprocess failed for an unrelated reason). + """ + try: + result: subprocess.CompletedProcess[str] = subprocess.run( + [ + sys.executable, + "-c", + "from importlib.metadata import version, PackageNotFoundError\n" + "try:\n" + " print(version('dailybot-cli'))\n" + "except PackageNotFoundError:\n" + " pass", + ], + capture_output=True, + text=True, + check=False, + timeout=10, + ) + except (OSError, subprocess.TimeoutExpired): + return None + out: str = result.stdout.strip() + return out or None + + def _manual_command_hint(method: str) -> str: """Human-readable upgrade command for methods we don't auto-run.""" if method == "homebrew": @@ -142,7 +183,7 @@ def upgrade(dry_run: bool, force: bool) -> None: click.echo(f"\n {_manual_command_hint(method)}\n") return - argv: list[str] | None = _build_upgrade_argv(method) + argv: list[str] | None = _build_upgrade_argv(method, latest=latest) if argv is None: # Methods we don't auto-run: homebrew, binary, uv-tool-not-found, unknown. @@ -176,4 +217,19 @@ def upgrade(dry_run: bool, force: bool) -> None: raise SystemExit(1) click.echo() + + # Post-upgrade verification — pip can exit 0 without doing anything (e.g. + # stale index cache + user-fallback edge case). The subprocess returncode + # is not enough; check the actual on-disk version. + installed_after: str | None = _query_installed_version() + if installed_after and installed_after == __version__ and latest and latest != __version__: + print_warning( + f"The package manager exited successfully but the installed " + f"version is still {installed_after}. This sometimes happens " + f"with pip after a fresh release (cached index) or when system " + f"site-packages is read-only. Try the install script as a " + f"reliable fallback:\n {_manual_command_hint('binary')}" + ) + return + print_success("Upgrade complete. Run 'dailybot --version' to confirm.") diff --git a/tests/commands_test.py b/tests/commands_test.py index f288933..3cf96be 100644 --- a/tests/commands_test.py +++ b/tests/commands_test.py @@ -146,6 +146,10 @@ def test_upgrade_pipx_runs_pipx(self, runner: CliRunner) -> None: "dailybot_cli.commands.upgrade.shutil.which", return_value="/usr/local/bin/pipx", ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value="999.0.0", + ), patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, ): mock_run.return_value = MagicMock(returncode=0) @@ -170,6 +174,10 @@ def test_upgrade_uv_tool(self, runner: CliRunner) -> None: "dailybot_cli.commands.upgrade.shutil.which", return_value="/usr/local/bin/uv", ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value="999.0.0", + ), patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, ): mock_run.return_value = MagicMock(returncode=0) @@ -178,8 +186,9 @@ def test_upgrade_uv_tool(self, runner: CliRunner) -> None: argv = mock_run.call_args[0][0] assert argv == ["uv", "tool", "upgrade", "dailybot-cli"] - def test_upgrade_pip_runs_python_pip(self, runner: CliRunner) -> None: - """A generic pip install runs ` -m pip install --upgrade ...`.""" + def test_upgrade_pip_pins_version_from_pypi(self, runner: CliRunner) -> None: + """A pip install pins the exact version (==X.Y.Z) so pip can't no-op + when its index cache is stale or system-site is read-only.""" with ( patch( "dailybot_cli.commands.upgrade._fetch_latest_pypi_version", @@ -189,6 +198,36 @@ def test_upgrade_pip_runs_python_pip(self, runner: CliRunner) -> None: "dailybot_cli.commands.upgrade._detect_install_method", return_value="pip", ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value="999.0.0", + ), + patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, + ): + mock_run.return_value = MagicMock(returncode=0) + result = runner.invoke(cli, ["upgrade"]) + assert result.exit_code == 0 + argv = mock_run.call_args[0][0] + assert argv[1:] == ["-m", "pip", "install", "--upgrade", "dailybot-cli==999.0.0"] + + def test_upgrade_pip_falls_back_to_unpinned_when_pypi_unreachable( + self, runner: CliRunner + ) -> None: + """If the JSON API didn't return a version, we still try `pip install --upgrade` + without a pin — at least gives the user a chance instead of bailing.""" + with ( + patch( + "dailybot_cli.commands.upgrade._fetch_latest_pypi_version", + return_value=None, + ), + patch( + "dailybot_cli.commands.upgrade._detect_install_method", + return_value="pip", + ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value="999.0.0", + ), patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, ): mock_run.return_value = MagicMock(returncode=0) @@ -309,6 +348,10 @@ def test_upgrade_force_runs_even_when_latest(self, runner: CliRunner) -> None: "dailybot_cli.commands.upgrade._detect_install_method", return_value="pip", ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value=__version__, + ), patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, ): mock_run.return_value = MagicMock(returncode=0) @@ -339,6 +382,66 @@ def test_upgrade_subprocess_failure_propagates_exit_code(self, runner: CliRunner assert result.exit_code == 42 assert "failed with exit code 42" in result.output + def test_upgrade_warns_when_pip_silently_noops(self, runner: CliRunner) -> None: + """Reproduces the real-world failure: pip exits 0 but the on-disk + version didn't change. This happened after the v1.7.0 release with + a stale pip index cache + read-only system site-packages. + + The fix is twofold: pin ``==`` so pip can't claim "already + satisfied" (covered by other tests) AND verify the post-install + version (covered here).""" + from dailybot_cli import __version__ + + with ( + patch( + "dailybot_cli.commands.upgrade._fetch_latest_pypi_version", + return_value="999.0.0", + ), + patch( + "dailybot_cli.commands.upgrade._detect_install_method", + return_value="pip", + ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value=__version__, # pip exited 0 but didn't actually upgrade + ), + patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, + ): + mock_run.return_value = MagicMock(returncode=0) + result = runner.invoke(cli, ["upgrade"]) + assert result.exit_code == 0 + # Should NOT claim success + assert "Upgrade complete" not in result.output + # Should warn the user clearly and point at the install.sh fallback. + # Rich wraps long warnings at terminal width, so collapse whitespace + # before searching for the phrase. + collapsed: str = " ".join(result.output.split()) + assert "exited successfully but the installed version is still" in collapsed + assert "install.sh" in collapsed + + def test_upgrade_succeeds_when_version_actually_changed(self, runner: CliRunner) -> None: + """Sanity: when the post-install version differs from current, we + report success.""" + with ( + patch( + "dailybot_cli.commands.upgrade._fetch_latest_pypi_version", + return_value="999.0.0", + ), + patch( + "dailybot_cli.commands.upgrade._detect_install_method", + return_value="pip", + ), + patch( + "dailybot_cli.commands.upgrade._query_installed_version", + return_value="999.0.0", + ), + patch("dailybot_cli.commands.upgrade.subprocess.run") as mock_run, + ): + mock_run.return_value = MagicMock(returncode=0) + result = runner.invoke(cli, ["upgrade"]) + assert result.exit_code == 0 + assert "Upgrade complete" in result.output + @patch("dailybot_cli.main.set_api_url_override") @patch("dailybot_cli.commands.update.get_token") @patch("dailybot_cli.commands.update.DailyBotClient")