github-featured/mvanhorn--last30days-skill
2
0
Fork 0
Code Issues Pull Requests Deployments Models Agent Notes Packages Projects Releases Wiki Activity

fix(setup-wizard): resolve npx via shutil.which on Windows (#904) (#911)

Browse Source 
Co-authored-by: SomSamantray <>
Co-authored-by: Trevin Chow <trevin@trevinchow.com>
This commit is contained in:
Som Samantray
2026-07-30 22:52:43 +05:30
committed by GitHub
parent ef6c279ceb
commit f310b82c14
4 changed files with 188 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
changelog.d/904.fixed.md
+1
View File
@@ -0,0 +1 @@
On Windows, the setup wizard's npx-based installs (Digg, arXiv, Techmeme) always failed silently because `shutil.which("npx")` resolves `PATHEXT` but `subprocess.run` given the bare string `"npx"` does not. Windows users also got macOS-only Homebrew guidance when yt-dlp was missing. Both are fixed: the resolved npx path is now passed through, and Windows gets `pip install yt-dlp` guidance instead.
skills/last30days/scripts/lib/setup_wizard.py
+66 -37
View File
@@ -112,11 +112,17 @@ def run_auto_setup(config: Dict[str, Any], *, allow_browser_cookies: bool = Fals
cookies_found[source_name] = result[1]
break # Found cookies for this service, stop trying browsers
# Check yt-dlp availability and install via Homebrew if missing
# Check yt-dlp availability and install via Homebrew if missing. Windows
# has no Homebrew, and its working install path is `pip install yt-dlp`
# (see #904), so it gets its own no-op-install guidance branch instead of
# falling into the Homebrew-oriented no_homebrew outcome.
ytdlp_action: str
if shutil.which("yt-dlp") is not None:
ytdlp_installed = True
ytdlp_action = "already_installed"
elif os.name == "nt":
ytdlp_installed = False
ytdlp_action = "no_pip_windows"
elif shutil.which("brew") is not None:
brew_stderr = ""
try:
@@ -226,6 +232,46 @@ def _digg_bin_dir_hint(digg_path: str) -> str:
return parent
def _run_npx_install(slug: str) -> Tuple[str, str]:
"""Resolve ``npx`` and run the Printing Press catalog install for ``slug``.
Shared by ``_install_digg_cli`` and ``_install_pp_cli`` -- this is only the
"resolve npx, run the install, interpret no_npx/exception/nonzero-rc"
slice; each caller keeps its own on-path/off-path re-verification
(``_digg_bin_candidate_paths`` vs ``_pp_bin_candidate_paths`` already use
different candidate-directory sources, so merging them here would change
off-path detection behavior beyond this fix's scope).
Fixes the Windows PATHEXT mismatch: ``shutil.which("npx")`` resolves
``npx.CMD`` via PATHEXT, but ``subprocess.run`` given the bare string
``"npx"`` as argv[0] does not do that resolution and fails with
``WinError 2``. Passing the resolved path is a no-op on macOS/Linux, where
``shutil.which`` already returns the exact path ``CreateProcess``/``execve``
would resolve.
Returns ``(action, stderr)``: ``action`` is ``"no_npx"``,
``"install_failed"``, or ``""`` when the subprocess ran and returned
``rc=0`` (in which case ``stderr`` carries any non-fatal stderr output for
the caller's own off-path logging).
"""
npx = shutil.which("npx")
if npx is None:
return "no_npx", ""
try:
proc = subprocess.run(
[npx, "-y", PRINTING_PRESS_NPM, "install", slug, "--cli-only"],
capture_output=True, text=True, timeout=DIGG_INSTALL_TIMEOUT,
)
except Exception as exc:
logger.warning("npx install %s exception: %s", slug, exc)
return "install_failed", str(exc)
if proc.returncode != 0:
stderr = proc.stderr or f"npx install {slug} exited {proc.returncode}"
logger.warning("npx install %s failed (rc=%s): %s", slug, proc.returncode, stderr)
return "install_failed", stderr
return "", (proc.stderr or "")
def _install_digg_cli() -> Tuple[bool, str, str, str]:
"""Best-effort install of the digg-pp-cli binary.
@@ -247,32 +293,21 @@ def _install_digg_cli() -> Tuple[bool, str, str, str]:
off_path = _digg_off_path_binary()
if off_path:
return False, "installed_off_path", "", off_path
if shutil.which("npx") is None:
return False, "no_npx", "", ""
try:
proc = subprocess.run(
["npx", "-y", PRINTING_PRESS_NPM, "install", "digg", "--cli-only"],
capture_output=True, text=True, timeout=DIGG_INSTALL_TIMEOUT,
)
except Exception as exc:
logger.warning("npx install digg exception: %s", exc)
return False, "install_failed", str(exc), ""
if proc.returncode != 0:
stderr = proc.stderr or f"npx install digg exited {proc.returncode}"
logger.warning("npx install digg failed (rc=%s): %s", proc.returncode, stderr)
return False, "install_failed", stderr, ""
action, stderr = _run_npx_install("digg")
if action:
return False, action, stderr, ""
on_path = _digg_on_path()
if on_path:
return True, "installed", "", ""
off_path = _digg_off_path_binary()
if off_path:
combined = (proc.stderr or "").strip()
combined = stderr.strip()
if combined:
logger.warning("digg-pp-cli installed off PATH: %s", combined)
return False, "installed_off_path", combined, off_path
stderr = proc.stderr or "install completed but digg-pp-cli was not found"
logger.warning("npx install digg failed verification: %s", stderr)
return False, "install_failed", stderr, ""
stderr_msg = stderr or "install completed but digg-pp-cli was not found"
logger.warning("npx install digg failed verification: %s", stderr_msg)
return False, "install_failed", stderr_msg, ""
# Additional default-on Printing Press sources installed the same way as Digg:
@@ -328,32 +363,21 @@ def _install_pp_cli(slug: str, bin_name: str) -> Tuple[bool, str, str, str]:
off_path = _pp_off_path_binary(bin_name)
if off_path:
return False, "installed_off_path", "", off_path
if shutil.which("npx") is None:
return False, "no_npx", "", ""
try:
proc = subprocess.run(
["npx", "-y", PRINTING_PRESS_NPM, "install", slug, "--cli-only"],
capture_output=True, text=True, timeout=DIGG_INSTALL_TIMEOUT,
)
except Exception as exc:
logger.warning("npx install %s exception: %s", slug, exc)
return False, "install_failed", str(exc), ""
if proc.returncode != 0:
stderr = proc.stderr or f"npx install {slug} exited {proc.returncode}"
logger.warning("npx install %s failed (rc=%s): %s", slug, proc.returncode, stderr)
return False, "install_failed", stderr, ""
action, stderr = _run_npx_install(slug)
if action:
return False, action, stderr, ""
on_path = shutil.which(bin_name)
if on_path:
return True, "installed", "", ""
off_path = _pp_off_path_binary(bin_name)
if off_path:
combined = (proc.stderr or "").strip()
combined = stderr.strip()
if combined:
logger.warning("%s installed off PATH: %s", bin_name, combined)
return False, "installed_off_path", combined, off_path
stderr = proc.stderr or f"install completed but {bin_name} was not found"
logger.warning("npx install %s failed verification: %s", slug, stderr)
return False, "install_failed", stderr, ""
stderr_msg = stderr or f"install completed but {bin_name} was not found"
logger.warning("npx install %s failed verification: %s", slug, stderr_msg)
return False, "install_failed", stderr_msg, ""
def install_default_pp_sources() -> Dict[str, Dict[str, Any]]:
@@ -555,6 +579,11 @@ def get_setup_status_text(results: Dict[str, Any]) -> str:
lines.append(" - yt-dlp install failed \u2014 run `brew install yt-dlp` manually")
elif ytdlp_action == "no_homebrew":
lines.append(" - yt-dlp not found. Install Homebrew first, then: brew install yt-dlp")
elif ytdlp_action == "no_pip_windows":
lines.append(
" - yt-dlp not found. Install with: pip install yt-dlp "
"(it may install to a Scripts directory not on PATH -- add it to PATH if YouTube search stays inactive)"
)
elif ytdlp_action == "already_installed":
lines.append(" - yt-dlp already installed")
elif results.get("ytdlp_installed", False):
tests/test_setup_wizard.py
+92 -9
View File
@@ -1,5 +1,6 @@
"""Tests for the first-run setup wizard module."""
import os
import subprocess
import tempfile
from pathlib import Path
@@ -10,6 +11,27 @@ import pytest
from lib import setup_wizard
class _NtOs:
"""Delegates to the real os module but reports name == 'nt'.
Patched only in setup_wizard's own namespace (mirrors
tests/test_health_probe_taxonomy.py's ``_NtOs``) so pathlib and the rest
of the test process are unaffected.
"""
name = "nt"
def __getattr__(self, attr):
return getattr(os, attr)
class _PosixOs:
"""Delegates to the real os module but reports name == 'posix'."""
name = "posix"
def __getattr__(self, attr):
return getattr(os, attr)
class TestIsFirstRun:
"""Tests for is_first_run()."""
@@ -60,8 +82,9 @@ class TestRunAutoSetup:
@patch("lib.cookie_extract.extract_cookies_with_source")
@patch("shutil.which")
def test_no_cookies_found(self, mock_which, mock_extract):
def test_no_cookies_found(self, mock_which, mock_extract, monkeypatch):
"""When no cookies found, results dict has empty cookies_found."""
monkeypatch.setattr(setup_wizard, "os", _PosixOs())
mock_extract.return_value = None
mock_which.return_value = None
@@ -139,8 +162,9 @@ class TestYtdlpAutoInstall:
@patch("lib.cookie_extract.extract_cookies_with_source", return_value=None)
@patch("subprocess.run")
@patch("shutil.which")
def test_ytdlp_missing_brew_available_installs(self, mock_which, mock_subproc, mock_extract):
"""yt-dlp missing + brew available -> installs via brew."""
def test_ytdlp_missing_brew_available_installs(self, mock_which, mock_subproc, mock_extract, monkeypatch):
"""yt-dlp missing + brew available (non-Windows) -> installs via brew."""
monkeypatch.setattr(setup_wizard, "os", _PosixOs())
def which_side_effect(cmd):
if cmd == "yt-dlp":
return None
@@ -161,8 +185,9 @@ class TestYtdlpAutoInstall:
@patch("lib.cookie_extract.extract_cookies_with_source", return_value=None)
@patch("shutil.which")
def test_ytdlp_missing_brew_missing(self, mock_which, mock_extract):
"""yt-dlp missing + brew missing -> no_homebrew."""
def test_ytdlp_missing_brew_missing(self, mock_which, mock_extract, monkeypatch):
"""yt-dlp missing + brew missing (non-Windows) -> no_homebrew."""
monkeypatch.setattr(setup_wizard, "os", _PosixOs())
mock_which.return_value = None
results = setup_wizard.run_auto_setup({})
@@ -170,6 +195,26 @@ class TestYtdlpAutoInstall:
assert results["ytdlp_installed"] is False
assert results["ytdlp_action"] == "no_homebrew"
@patch("lib.cookie_extract.extract_cookies_with_source", return_value=None)
@patch("shutil.which")
def test_ytdlp_missing_on_windows(self, mock_which, mock_extract, monkeypatch):
"""Regression for #904: yt-dlp missing on Windows -> pip guidance, no
Homebrew attempt (Windows has no Homebrew and pip is the working path)."""
monkeypatch.setattr(setup_wizard, "os", _NtOs())
mock_which.return_value = None
with patch("subprocess.run") as mock_subproc:
results = setup_wizard.run_auto_setup({})
mock_subproc.assert_not_called()
assert results["ytdlp_installed"] is False
assert results["ytdlp_action"] == "no_pip_windows"
text = setup_wizard.get_setup_status_text(results)
assert "pip install yt-dlp" in text
assert "Homebrew" not in text
assert "Scripts" in text
@patch("lib.cookie_extract.extract_cookies_with_source", return_value=None)
@patch("shutil.which")
def test_ytdlp_already_installed(self, mock_which, mock_extract):
@@ -184,8 +229,9 @@ class TestYtdlpAutoInstall:
@patch("lib.cookie_extract.extract_cookies_with_source", return_value=None)
@patch("subprocess.run")
@patch("shutil.which")
def test_brew_install_fails(self, mock_which, mock_subproc, mock_extract):
"""brew install yt-dlp fails -> install_failed with stderr."""
def test_brew_install_fails(self, mock_which, mock_subproc, mock_extract, monkeypatch):
"""brew install yt-dlp fails (non-Windows) -> install_failed with stderr."""
monkeypatch.setattr(setup_wizard, "os", _PosixOs())
def which_side_effect(cmd):
if cmd == "yt-dlp":
return None
@@ -267,9 +313,46 @@ class TestDiggAutoInstall:
# The wizard now also best-effort-installs the additional default-on
# Printing Press sources (arxiv/techmeme/trustpilot), so digg is one of
# several install calls rather than the only one.
# several install calls rather than the only one. Argv[0] must be the
# *resolved* npx path (mirroring shutil.which's return value), not the
# bare "npx" string -- passing the bare name breaks Windows, where
# shutil.which resolves PATHEXT (npx.CMD) but subprocess.run does not.
mock_subproc.assert_any_call(
["npx", "-y", setup_wizard.PRINTING_PRESS_NPM, "install", "digg", "--cli-only"],
["/opt/homebrew/bin/npx", "-y", setup_wizard.PRINTING_PRESS_NPM, "install", "digg", "--cli-only"],
capture_output=True, text=True, timeout=setup_wizard.DIGG_INSTALL_TIMEOUT,
)
assert results["digg_installed"] is True
assert results["digg_action"] == "installed"
@patch("lib.cookie_extract.extract_cookies_with_source", return_value=None)
@patch("subprocess.run")
@patch("shutil.which")
def test_digg_install_uses_resolved_windows_npx_path(
self, mock_which, mock_subproc, mock_extract, tmp_path, monkeypatch
):
"""Regression for #904: a Windows-style resolved npx path (PATHEXT
resolution, e.g. npx.CMD) must be passed to subprocess.run verbatim --
not the bare string "npx", which fails with WinError 2 on Windows
because CreateProcess does not do PATHEXT resolution the way
shutil.which does."""
self._empty_home(tmp_path, monkeypatch)
calls = {"digg": 0}
windows_npx = r"C:\Program Files\nodejs\npx.CMD"
def which_side_effect(cmd):
if cmd == "digg-pp-cli":
calls["digg"] += 1
return None if calls["digg"] == 1 else r"C:\Users\me\.local\bin\digg-pp-cli"
if cmd == "npx":
return windows_npx
return None
mock_which.side_effect = which_side_effect
mock_subproc.return_value = MagicMock(returncode=0, stderr="")
results = setup_wizard.run_auto_setup({})
mock_subproc.assert_any_call(
[windows_npx, "-y", setup_wizard.PRINTING_PRESS_NPM, "install", "digg", "--cli-only"],
capture_output=True, text=True, timeout=setup_wizard.DIGG_INSTALL_TIMEOUT,
)
assert results["digg_installed"] is True
tests/test_setup_wizard_pp_sources.py
+29
View File
@@ -58,6 +58,35 @@ def test_install_success(monkeypatch, no_off_path):
assert action == "installed"
def test_install_uses_resolved_windows_npx_path(monkeypatch, no_off_path):
"""Regression for #904: subprocess.run must receive the resolved npx
path (e.g. a Windows PATHEXT-resolved npx.CMD), not the bare "npx"
string -- bare "npx" fails with WinError 2 on Windows."""
calls = {"n": 0}
windows_npx = r"C:\Program Files\nodejs\npx.CMD"
def fake_which(name):
if name == "npx":
return windows_npx
if name == "techmeme-pp-cli":
calls["n"] += 1
return None if calls["n"] == 1 else r"C:\Users\me\.local\bin\techmeme-pp-cli"
return None
run_calls = []
def fake_run(cmd, **kwargs):
run_calls.append(cmd)
return type("P", (), {"returncode": 0, "stdout": "", "stderr": ""})()
monkeypatch.setattr(sw.shutil, "which", fake_which)
monkeypatch.setattr(sw.subprocess, "run", fake_run)
installed, action, stderr, off = sw._install_pp_cli("techmeme", "techmeme-pp-cli")
assert installed is True
assert action == "installed"
assert run_calls[0][0] == windows_npx
def test_install_failed_nonzero_rc(monkeypatch, no_off_path):
def fake_which(name):
return "/usr/bin/npx" if name == "npx" else None