diff --git a/src/devforge/cli.py b/src/devforge/cli.py index 5abac27..f325e98 100644 --- a/src/devforge/cli.py +++ b/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: diff --git a/tests/test_cli.py b/tests/test_cli.py index 022df80..8f57718 100644 --- a/tests/test_cli.py +++ b/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