cowork-bot: automated improvements (cowork/improve-devforge-cli-2)

src/devforge/cli.py

@@ -102,16 +102,21 @@ def install(
     repo_url = "https://github.com/Coding-Dev-Tools/devforge-cli.git"
     pkg = f"git+{repo_url}[{extras}]"
     console.print(f"[yellow]Installing {pkg}...[/yellow]")
+    # Only catch OS-level failures here. A bare `except Exception` would also
+    # swallow the typer.Exit raised below (typer.Exit subclasses Exception),
+    # double-printing an error line ("Error: 1") after the failure message.
     try:
-        result = subprocess.run([sys.executable, "-m", "pip", "install", pkg], capture_output=True, text=True)
-        if result.returncode == 0:
-            console.print(f"[green]Successfully installed:[/green] {', '.join(targets)}")
-        else:
-            console.print(f"[red]Installation failed:[/red] {result.stderr[:500]}")
-            raise typer.Exit(code=1)
-    except Exception as e:
-        console.print(f"[red]Error: {e}[/red]")
+        result = subprocess.run(
+            [sys.executable, "-m", "pip", "install", pkg], capture_output=True, text=True
+        )
+    except OSError as e:
+        console.print(f"[red]Error running pip:[/red] {e}")
         raise typer.Exit(code=1) from e
+    if result.returncode == 0:
+        console.print(f"[green]Successfully installed:[/green] {', '.join(targets)}")
+    else:
+        console.print(f"[red]Installation failed:[/red] {result.stderr[:500]}")
+        raise typer.Exit(code=1)
 
 
 @app.command(name="versions")
@@ -128,19 +133,35 @@ def show_versions(
     for t in targets:
         info = TOOLS[t]
         try:
-            result = subprocess.run(
-                [sys.executable, "-m", "pip", "show", info["package"]], capture_output=True, text=True
-            )
-            if result.returncode == 0:
-                for line in result.stdout.splitlines():
-                    if line.startswith("Version:"):
-                        ver = line.split(":", 1)[1].strip()
-                        console.print(f"[cyan]{t:8}[/cyan] v{ver}")
-                        break
-            else:
-                console.print(f"[dim]{t:8}[/dim] [red]not installed[/red]")
-        except Exception:
-            console.print(f"[dim]{t:8}[/dim] [red]error checking[/red]")
+            ver = _pip_version(info["package"])
+        except Exception as e:
+            console.print(f"[dim]{t:8}[/dim] [red]error checking ({e})[/red]")
+            continue
+        if ver is None:
+            console.print(f"[dim]{t:8}[/dim] [red]not installed[/red]")
+        elif ver == "":
+            # pip show succeeded but returned no Version metadata — never stay silent.
+            console.print(f"[dim]{t:8}[/dim] [yellow]installed, no version metadata[/yellow]")
+        else:
+            console.print(f"[cyan]{t:8}[/cyan] v{ver}")
+
+
+def _pip_version(package: str) -> str | None:
+    """Return the installed version of *package*, or None if not installed.
+
+    Returns "" when ``pip show`` succeeds but the output carries no
+    ``Version:`` line (broken metadata) so callers can distinguish it from a
+    clean not-installed result instead of silently printing nothing.
+    """
+    result = subprocess.run(
+        [sys.executable, "-m", "pip", "show", package], capture_output=True, text=True
+    )
+    if result.returncode != 0:
+        return None
+    for line in result.stdout.splitlines():
+        if line.startswith("Version:"):
+            return line.split(":", 1)[1].strip()
+    return ""
 
 
 def _is_tool_installed(module_name: str) -> bool:
@@ -172,11 +193,19 @@ def dispatch(ctx: typer.Context):
         # `--config file.yaml`) reach the underlying CLI instead of being
         # rejected by typer as "No such option".
         forwarded = list(ctx.args)
-        result = subprocess.run(
-            [sys.executable, "-m", module_name] + forwarded,
-            capture_output=True,
-            text=True,
-        )
+        try:
+            result = subprocess.run(
+                [sys.executable, "-m", module_name] + forwarded,
+                capture_output=True,
+                text=True,
+            )
+        except OSError as e:
+            console.print(f"[red]Error launching {tool_name}:[/red] {e}")
+            raise typer.Exit(code=1) from e
+        except KeyboardInterrupt:
+            # Forward Ctrl-C as a conventional 130 exit, not a raw traceback.
+            console.print("[yellow]Interrupted.[/yellow]")
+            sys.exit(130)
         if result.stdout:
             sys.stdout.write(result.stdout)
         if result.stderr:

tests/test_cli.py

@@ -3,7 +3,7 @@
 from __future__ import annotations
 
 from devforge import TOOLS, __version__
-from devforge.cli import _is_tool_installed, app
+from devforge.cli import _is_tool_installed, _pip_version, app
 from typer.testing import CliRunner
 from unittest import mock
 
@@ -176,3 +176,65 @@ def test_help(self):
         assert "tools" in result.stdout
         assert "versions" in result.stdout
         assert "guard" in result.stdout
+
+
+class TestPipVersionHelper:
+    @mock.patch("devforge.cli.subprocess.run")
+    def test_returns_version_line(self, mock_run):
+        """Parse Version: out of successful pip show output."""
+        mock_run.return_value = mock.MagicMock(returncode=0, stdout="Name: x\nVersion: 1.2.3\n")
+        assert _pip_version("x") == "1.2.3"
+
+    @mock.patch("devforge.cli.subprocess.run")
+    def test_not_installed_returns_none(self, mock_run):
+        mock_run.return_value = mock.MagicMock(returncode=1, stdout="", stderr="not found")
+        assert _pip_version("x") is None
+
+    @mock.patch("devforge.cli.subprocess.run")
+    def test_missing_metadata_returns_empty(self, mock_run):
+        """pip show success without a Version line must NOT look like not installed."""
+        mock_run.return_value = mock.MagicMock(returncode=0, stdout="Name: x\n")
+        assert _pip_version("x") == ""
+
+    @mock.patch("devforge.cli._pip_version", return_value="")
+    def test_versions_reports_missing_metadata(self, _mock):
+        """Silent-green regression guard: broken metadata gets an explicit line."""
+        result = runner.invoke(app, ["versions", "guard"])
+        assert result.exit_code == 0
+        assert "no version metadata" in result.stdout
+
+    @mock.patch("devforge.cli._pip_version", side_effect=OSError("boom"))
+    def test_versions_reports_error(self, _mock):
+        result = runner.invoke(app, ["versions", "guard"])
+        assert result.exit_code == 0
+        assert "error checking" in result.stdout
+
+
+class TestInstallErrorHandling:
+    @mock.patch("devforge.cli.subprocess.run")
+    def test_install_failure_no_double_error(self, mock_run):
+        """A failed pip install must print exactly one failure message.
+
+        Regression guard: the old `except Exception` also caught the
+        typer.Exit raised after 'Installation failed', printing a spurious
+        second line ('Error: 1').
+        """
+        mock_run.return_value = mock.MagicMock(returncode=1, stdout="", stderr="boom")
+        result = runner.invoke(app, ["install", "guard"])
+        assert result.exit_code == 1
+        assert "Installation failed" in result.stdout
+        assert "Error: 1" not in result.stdout
+
+    @mock.patch("devforge.cli.subprocess.run", side_effect=OSError("pip missing"))
+    def test_install_oserror_reported(self, mock_run):
+        result = runner.invoke(app, ["install", "guard"])
+        assert result.exit_code == 1
+        assert "Error running pip" in result.stdout
+
+    @mock.patch("devforge.cli._is_tool_installed", return_value=True)
+    @mock.patch("devforge.cli.subprocess.run", side_effect=OSError("python gone"))
+    def test_dispatch_oserror_reported(self, mock_run, _mock_installed):
+        """OSError from the tool subprocess gets a clear message, not a traceback."""
+        result = runner.invoke(app, ["guard"])
+        assert result.exit_code == 1
+        assert "Error launching guard" in result.stdout