Cookbook: surface pip install failures in logs

_pip_install_fallback_chain silently discarded pip stderr via
2>/dev/null on every attempt. When pip failed (network error, venv
mismatch, disk full), the wrapper exited 0 and the Cookbook UI showed
the download as running — the silent-failure mode from #354.

Extract _pip_install_attempt() which wraps each pip invocation in a
bash -c subshell that captures output to a temp file, prints tail -5
on failure, cleans up, and exits with pip's real exit code. This
avoids the | tail pipefail masking (the first blocker on #363) while
surfacing the last 5 lines of pip output in the tmux log so users
can see what went wrong.

Both local wrapper and remote SSH runner use the same helper through
_pip_install_fallback_chain, so the fix is symmetric.

routes/cookbook_helpers.py

@@ -148,15 +148,39 @@ def _local_tooling_path_export(executable: str) -> str:
     return f'export PATH="{esc}:$PATH"'
 
 
-def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m pip", upgrade: bool = False) -> str:
-    """Build a bash pip install fallback chain.
+def _pip_install_attempt(pip_cmd: str) -> str:
+    """Wrap a single pip install command so its exit status survives the
+    fallback chain and its stderr is visible in the tmux log on failure.
 
-    Try the active interpreter/environment first. `--user` is invalid inside
-    many venvs, so only attempt the --user fallback when NOT inside a venv.
+    Without this wrapper, `pip … 2>&1 | tail -5` returns ``tail``'s exit
+    code (0), masking pip's real failure and preventing the next fallback
+    from running.  The generated snippet captures all output to a temp
+    file, prints the last 5 lines on failure (so the Cookbook log panel
+    shows useful diagnostics), cleans up, and exits with pip's original
+    status.
+    """
+    return (
+        "bash -c '"
+        f'_out=$(mktemp) && {pip_cmd} >"$_out" 2>&1; _rc=$?; '
+        'tail -5 "$_out"; rm -f "$_out"; exit $_rc'
+        "'"
+    )
+
+
+def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m pip", upgrade: bool = False) -> str:
+    """Build a bash pip install fallback chain that surfaces errors.
+
+    Try the active interpreter/environment first. ``--user`` is invalid
+    inside many venvs, so only attempt the ``--user`` fallback when NOT
+    inside a venv.
+
+    Each attempt is wrapped via :func:`_pip_install_attempt` so pip's real
+    exit code is preserved (no ``| tail`` masking) and the last 5 lines of
+    pip output appear in the Cookbook log on failure.
     """
     upgrade_flag = " -U" if upgrade else ""
-    base = f"{python_cmd} install -q{upgrade_flag} {package} 2>/dev/null"
-    user = f"{python_cmd} install --user --break-system-packages -q{upgrade_flag} {package} 2>/dev/null"
+    base = _pip_install_attempt(f"{python_cmd} install -q{upgrade_flag} {package}")
+    user = _pip_install_attempt(f"{python_cmd} install --user --break-system-packages -q{upgrade_flag} {package}")
     # Derive the python executable for the venv detection check.
     # Must use the same interpreter that pip belongs to; hardcoding
     # python3 breaks when pip lives in a venv that only has "python".
@@ -169,8 +193,13 @@ def _pip_install_fallback_chain(package: str, *, python_cmd: str = "python3 -m p
     else:
         python_exe = "python3"
     venv_check = f'{python_exe} -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"'
-    # venv_check exits 0 (true) when IN a venv; --user is only valid outside one.
-    return f"{base} || {{ {venv_check} || {user}; }}"
+    # Negated: `! venv_check` succeeds (exit 0) when NOT in a venv → `&&` tries
+    # --user.  When IN a venv `! venv_check` fails → `&&` skips --user and the
+    # group exits non-zero, propagating the base-install failure instead of
+    # masking it as success (the `|| { venv_check || … }` shape from #903
+    # swallowed the exit code because venv_check's exit-0 became the group's
+    # result).
+    return f"{base} || {{ ! {venv_check} && {user}; }}"
 
 
 def _cached_model_scan_script(model_dirs: list[str] | None = None) -> str:

tests/test_cookbook_helpers.py

@@ -10,6 +10,7 @@ from routes.cookbook_helpers import (
     _append_serve_exit_code_lines,
     _append_serve_preflight_exit_lines,
     _local_tooling_path_export,
+    _pip_install_attempt,
     _pip_install_fallback_chain,
     _ollama_bind_from_cmd,
     _safe_env_prefix,
@@ -87,17 +88,129 @@ def test_local_tooling_path_export_preserves_spaces_and_expands_path():
 
 def test_pip_install_fallback_chain_prefers_venv_safe_install():
     chain = _pip_install_fallback_chain("huggingface_hub", upgrade=True)
-    assert chain.startswith("python3 -m pip install -q -U huggingface_hub")
-    assert "|| python3 -m pip install --user --break-system-packages -q -U huggingface_hub" in chain
+    # First attempt: plain install, wrapped in status-preserving subshell
+    assert chain.startswith("bash -c '")
+    assert "python3 -m pip install -q -U huggingface_hub" in chain
+    # Second attempt: --user --break-system-packages, also wrapped
+    assert "--user --break-system-packages" in chain
+    assert "python3 -m pip install --user --break-system-packages -q -U huggingface_hub" in chain
+    # No bare `| tail` (which would mask pip's exit code)
+    assert "| tail" not in chain
+    # Negated venv check with && — so failure in a venv propagates instead of
+    # being masked as success by the venv_check's exit-0.
+    assert "! python3 -c" in chain
+    # The group uses && (not ||) between venv check and user attempt
+    assert "&&" in chain
 
 
 def test_pip_install_fallback_chain_allows_custom_python_command():
     chain = _pip_install_fallback_chain("hf_transfer", python_cmd="pip", upgrade=False)
-    assert chain == (
-        'pip install -q hf_transfer 2>/dev/null || { '
-        'python -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"'
-        ' || pip install --user --break-system-packages -q hf_transfer 2>/dev/null; }'
+    assert "pip install -q hf_transfer" in chain
+    assert "pip install --user --break-system-packages -q hf_transfer" in chain
+    # venv check uses the python executable derived from the pip command
+    assert 'python -c "import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)"' in chain
+    # Both attempts are wrapped in bash -c subshells
+    assert chain.count("bash -c '") == 2
+
+
+def test_pip_install_fallback_chain_propagates_failure_in_venv():
+    """When base install fails inside a venv, the chain must exit non-zero.
+
+    The old `{ venv_check || user }` shape from #903 masked the failure:
+    venv_check exited 0 (in venv), || short-circuited, and the group
+    reported success even though nothing was installed.  The negated
+    `{ ! venv_check && user }` shape propagates the failure correctly.
+    """
+    import shlex
+    py = shlex.quote(sys.executable)
+    # Use the venv python so venv_check detects we're in a venv.
+    # Base install fails, venv_check exits 0, negated to 1,
+    # && skips user, group exits 1.
+    script = (
+        f"{py} -c 'import sys; sys.exit(1)' || "
+        f"{{ ! {py} -c \"import sys; sys.exit(0 if sys.prefix != sys.base_prefix else 1)\" "
+        f"&& echo user_attempt; }}"
     )
+    result = subprocess.run(
+        ["bash", "-c", script],
+        capture_output=True, text=True, timeout=10,
+    )
+    assert "user_attempt" not in result.stdout
+    assert result.returncode != 0, "Chain should propagate failure when base fails in venv"
+
+
+def test_pip_install_fallback_chain_tries_user_outside_venv():
+    """When base install fails outside a venv, the chain should try --user."""
+    # Force "not in venv" by making venv_check return 1 directly.
+    script = (
+        "bash -c '"
+        "python3 -c \"import sys; sys.exit(1)\" || "
+        "{ ! python3 -c \"import sys; sys.exit(1)\" "  # venv_check=1 → negated to 0 → user runs
+        "&& echo user_attempt; }"
+        "'"
+    )
+    result = subprocess.run(
+        ["bash", "-c", script],
+        capture_output=True, text=True, timeout=10,
+    )
+    assert "user_attempt" in result.stdout, "Chain should try --user when not in venv and base fails"
+
+
+def test_pip_install_attempt_wraps_in_status_preserving_subshell():
+    """Each pip attempt must be a bash -c subshell that captures output,
+    prints tail, cleans up, and exits with pip's real status — not tail's."""
+    snippet = _pip_install_attempt("pip install -q huggingface_hub")
+    assert snippet.startswith("bash -c '")
+    assert "$(mktemp)" in snippet
+    assert "_rc=$?" in snippet
+    assert "tail -5" in snippet
+    assert "rm -f" in snippet
+    assert "exit $_rc" in snippet
+
+
+def test_pip_install_attempt_no_bare_pipe_tail():
+    """A bare `| tail` pipeline would mask pip's exit code — must not appear."""
+    snippet = _pip_install_attempt("pip install -q huggingface_hub")
+    assert "| tail" not in snippet
+
+
+def test_pip_install_attempt_failure_propagates_real_exit_code():
+    """Run the generated snippet against a deliberately broken pip install
+    to confirm the subshell exits with pip's non-zero status."""
+    snippet = _pip_install_attempt("python3 -m pip install __nonexistent_package_12345__")
+    result = subprocess.run(
+        ["bash", "-c", snippet],
+        capture_output=True,
+        text=True,
+        timeout=60,
+    )
+    assert result.returncode != 0, "pip install of a nonexistent package should fail"
+
+
+def test_pip_install_attempt_success_exits_zero():
+    """When pip succeeds, the subshell should exit 0."""
+    snippet = _pip_install_attempt("python3 -c 'pass'")
+    result = subprocess.run(
+        ["bash", "-c", snippet],
+        capture_output=True,
+        text=True,
+        timeout=15,
+    )
+    assert result.returncode == 0
+
+
+def test_pip_install_attempt_surfaces_stderr_on_failure():
+    """On failure, the last 5 lines of pip output should appear in stdout."""
+    snippet = _pip_install_attempt("python3 -m pip install __nonexistent_package_12345__")
+    result = subprocess.run(
+        ["bash", "-c", snippet],
+        capture_output=True,
+        text=True,
+        timeout=60,
+    )
+    # pip's error message should be visible in the output (not swallowed)
+    combined = result.stdout + result.stderr
+    assert "nonexistent" in combined.lower() or result.returncode != 0
 
 
 def test_serve_preflight_failure_keeps_tmux_pane_visible():