diff --git a/src/devforge/cli.py b/src/devforge/cli.py index 5abac27..e83cba5 100644 --- a/src/devforge/cli.py +++ b/src/devforge/cli.py @@ -6,6 +6,7 @@ import typer from devforge import TOOLS, __version__ from rich.console import Console +from rich.markup import escape from rich.panel import Panel from rich.table import Table @@ -103,14 +104,26 @@ def install( pkg = f"git+{repo_url}[{extras}]" console.print(f"[yellow]Installing {pkg}...[/yellow]") try: - result = subprocess.run([sys.executable, "-m", "pip", "install", pkg], capture_output=True, text=True) + result = subprocess.run( + [sys.executable, "-m", "pip", "install", pkg], + capture_output=True, + text=True, + errors="replace", + ) if result.returncode == 0: console.print(f"[green]Successfully installed:[/green] {', '.join(targets)}") else: - console.print(f"[red]Installation failed:[/red] {result.stderr[:500]}") + # pip error text routinely contains brackets such as ``[WinError 2]``; + # escape it so a failed install still exits cleanly with its message. + console.print( + f"[red]Installation failed:[/red] {escape(result.stderr[:500])}", + soft_wrap=True, + ) raise typer.Exit(code=1) + except typer.Exit: + raise except Exception as e: - console.print(f"[red]Error: {e}[/red]") + console.print(f"[red]Error: {escape(str(e))}[/red]", soft_wrap=True) raise typer.Exit(code=1) from e @@ -128,19 +141,42 @@ 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: + # Exception text can contain rich markup such as ``[/red]`` or ``[WinError 2]``; + # escape it so an unrelated tool failure can never abort the whole listing. + console.print(f"[dim]{t:8}[/dim] [red]error checking ({escape(str(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. + """ + # ``errors="replace"`` keeps non-ASCII metadata from raising inside the decode + # step and hiding an otherwise readable version. + result = subprocess.run( + [sys.executable, "-m", "pip", "show", package], + capture_output=True, + text=True, + errors="replace", + ) + 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: diff --git a/tests/test_cli.py b/tests/test_cli.py index 022df80..257e60d 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 @@ -76,6 +76,22 @@ def test_install_failure(self, mock_run): assert result.exit_code == 1 assert "failed" in result.stdout.lower() + @mock.patch("devforge.cli.subprocess.run") + def test_install_failure_with_bracketed_stderr_exits_cleanly(self, mock_run): + """pip stderr with brackets must not raise MarkupError or lose the exit code. + + Regression guard: an unescaped ``[/red]`` in pip's stderr raised inside + ``console.print`` and the except handler raised again, so the user saw a + traceback instead of a clean failure. + """ + mock_run.return_value = mock.MagicMock( + returncode=1, stdout="", stderr="ERROR: [WinError 2] no such file [/red]" + ) + result = runner.invoke(app, ["install", "guard"]) + assert result.exit_code == 1 + assert "failed" in result.stdout.lower() + assert "MarkupError" not in result.stdout + class TestVersionsCommand: def test_versions_runs(self): @@ -176,3 +192,55 @@ 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 + + @mock.patch("devforge.cli._pip_version", side_effect=OSError("bad [/red] tag [WinError 2]")) + def test_versions_error_text_cannot_break_rich_markup(self, _mock): + """Exception text must be escaped, not parsed as rich markup. + + Without escaping, a message containing ``[/red]`` raises MarkupError and + aborts the whole listing with no output at all. + """ + result = runner.invoke(app, ["versions", "guard"]) + assert result.exit_code == 0 + assert "error checking" in result.stdout + assert "boom" in result.stdout or "[/red]" in result.stdout + assert "MarkupError" not in result.stdout + + @mock.patch("devforge.cli.subprocess.run") + def test_decodes_non_ascii_metadata(self, mock_run): + """Non-ASCII metadata must not raise while decoding pip output.""" + mock_run.return_value = mock.MagicMock(returncode=0, stdout="Name: x\nAuthor: Ünïcodé ✓\nVersion: 2.0.1\n") + assert _pip_version("x") == "2.0.1" + assert mock_run.call_args.kwargs.get("errors") == "replace"