fix(upgrade): pin version + verify post-install to prevent silent no-ops

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==<latest>`` 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.")

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 `<sys.executable> -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 ``==<latest>`` 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")