fp-stability: address PR review — atomic prebuilt install, verify VERROU_HOME tree, more tests

verrou.sh try_prebuilt() runs as an if-condition (set -e suppressed), so a failed extract could fall through and the source build would install over a half-written tree; now extract+verify in a staging dir and swap into PREFIX atomically with explicit error checks. _find_verrou now verifies the $VERROU_HOME tree actually runs the verrou tool (with VALGRIND_LIB for a relocated prebuilt) so a broken/stale tree reads as absent and gets reinstalled, not used until it fails per-run. Fix comment rot (fp-stability now auto-installs). Add unit tests for _verrou_env (incl. preserve-user-VALGRIND_LIB), _dd_env PYTHONPATH composition, _install_verrou hard-fail guards, _has_verrou_tool OSError, and the broken-VERROU_HOME case.

Author: sbryngelson

toolchain/bootstrap/verrou.sh

@@ -4,7 +4,8 @@
 # `./mfc.sh fp-stability`). Verrou is NOT a Python/pip package — it is a fork of
 # Valgrind. By default this downloads a prebuilt, hash-verified artifact (seconds);
 # if none is available for this tag/arch it falls back to a source build (~20 min).
-# Either way it's a deliberate, explicit step, not something fp-stability does silently.
+# fp-stability auto-runs this on first use when Verrou is absent (printing what it
+# does); it is also safe to run by hand. A failed install aborts, never a silent skip.
 #
 #   bash toolchain/bootstrap/verrou.sh            # install into $HOME/.local/verrou
 #   VERROU_HOME=/path bash toolchain/bootstrap/verrou.sh
@@ -80,20 +81,34 @@ try_prebuilt() {
         rm -rf "$dl"; return 1
     fi
 
-    mkdir -p "$PREFIX"
+    # Extract + verify in a staging dir, then swap into $PREFIX atomically. set -e
+    # is suppressed inside a function used as an `if` condition, so check each step
+    # explicitly — otherwise a failed extract would fall through and the source
+    # build would install on top of a half-written tree (or a stale one on --force).
+    local stage="$dl/stage"
+    mkdir -p "$stage"
     if tar --zstd --help >/dev/null 2>&1; then
-        tar -C "$PREFIX" --zstd -xf "$dl/$asset"
+        tar -C "$stage" --zstd -xf "$dl/$asset" || { echo "WARNING: prebuilt extract failed — building from source instead." >&2; rm -rf "$dl"; return 1; }
     else
-        zstd -dc "$dl/$asset" | tar -C "$PREFIX" -xf -
+        zstd -dc "$dl/$asset" | tar -C "$stage" -xf - || { echo "WARNING: prebuilt extract failed — building from source instead." >&2; rm -rf "$dl"; return 1; }
     fi
-    rm -rf "$dl"
 
     # Valgrind bakes its build prefix into the binary; the artifact's env.sh sets
-    # VALGRIND_LIB relative to the extracted tree so the relocated install works.
-    if ! ( . "${PREFIX}/env.sh" && "${PREFIX}/bin/valgrind" --tool=verrou --version >/dev/null 2>&1 ); then
+    # VALGRIND_LIB relative to the tree so the relocated install works. Verify the
+    # staged tree runs before committing it.
+    if ! ( . "${stage}/env.sh" && "${stage}/bin/valgrind" --tool=verrou --version >/dev/null 2>&1 ); then
         echo "WARNING: prebuilt did not run — building from source instead." >&2
-        return 1
+        rm -rf "$dl"; return 1
+    fi
+
+    # Commit only now: replace any existing $PREFIX atomically.
+    mkdir -p "$(dirname "$PREFIX")"
+    rm -rf "$PREFIX"
+    if ! mv "$stage" "$PREFIX"; then
+        echo "WARNING: could not install prebuilt to ${PREFIX} — building from source instead." >&2
+        rm -rf "$dl"; return 1
     fi
+    rm -rf "$dl"
     return 0
 }
 

toolchain/mfc/fp_stability_runners.py

@@ -32,19 +32,24 @@
 from .printer import cons
 
 
-def _has_verrou_tool(valgrind_bin: str) -> bool:
+def _has_verrou_tool(valgrind_bin: str, env: dict = None) -> bool:
     """True if this valgrind actually provides the 'verrou' tool. A plain system
-    valgrind does not — accepting one would only fail later at run time."""
+    valgrind does not — accepting one would only fail later at run time. Pass env
+    (with VALGRIND_LIB) to verify a relocated prebuilt tree, which cannot load its
+    tool without it."""
     try:
-        return subprocess.run([valgrind_bin, "--tool=verrou", "--version"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False).returncode == 0
+        return subprocess.run([valgrind_bin, "--tool=verrou", "--version"], env=env, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False).returncode == 0
     except OSError:
         return False
 
 
 def _find_verrou() -> str:
     verrou_home = os.environ.get("VERROU_HOME", os.path.join(os.path.expanduser("~"), ".local", "verrou"))
     candidate = os.path.join(verrou_home, "bin", "valgrind")
-    if os.path.isfile(candidate) and os.access(candidate, os.X_OK):
+    # Require the $VERROU_HOME tree to actually run the verrou tool (with VALGRIND_LIB
+    # for a relocated prebuilt). A broken/stale/non-Verrou tree there must read as
+    # "absent" so it gets reinstalled, not used until it fails on every run.
+    if os.path.isfile(candidate) and os.access(candidate, os.X_OK) and _has_verrou_tool(candidate, _verrou_env(candidate)):
         return candidate
     # Fall back to a valgrind on PATH only if it is Verrou-enabled; a bare system
     # valgrind must read as "Verrou absent" so it gets installed, not misused.

toolchain/mfc/test_fp_stability.py

@@ -392,9 +392,27 @@ def test_find_verrou_prefers_verrou_home_candidate(tmp_path, monkeypatch):
     vbin.write_text("#!/bin/sh\n")
     vbin.chmod(0o755)
     monkeypatch.setenv("VERROU_HOME", str(tmp_path))
+    # The candidate must also verify as Verrou-enabled; stub that so the test
+    # exercises precedence, not a real valgrind invocation.
+    monkeypatch.setattr(runners, "_has_verrou_tool", lambda _bin, _env=None: True)
     assert runners._find_verrou() == str(vbin)
 
 
+def test_find_verrou_rejects_broken_verrou_home_tree(tmp_path, monkeypatch):
+    from mfc import fp_stability_runners as runners
+
+    # A valgrind exists at $VERROU_HOME but does not actually run the verrou tool
+    # (broken/stale/non-Verrou): it must read as absent, not be returned.
+    vbin = tmp_path / "bin" / "valgrind"
+    vbin.parent.mkdir(parents=True)
+    vbin.write_text("#!/bin/sh\n")
+    vbin.chmod(0o755)
+    monkeypatch.setenv("VERROU_HOME", str(tmp_path))
+    monkeypatch.setattr(runners, "_has_verrou_tool", lambda _bin, _env=None: False)
+    monkeypatch.setattr(runners.shutil, "which", lambda _name: None)
+    assert runners._find_verrou() == ""
+
+
 def test_find_verrou_rejects_non_verrou_path_valgrind(tmp_path, monkeypatch):
     from mfc import fp_stability_runners as runners
 
@@ -425,3 +443,83 @@ def __init__(self, rc):
     assert runners._has_verrou_tool("/any/valgrind") is True
     monkeypatch.setattr(runners.subprocess, "run", lambda *a, **k: _R(1))
     assert runners._has_verrou_tool("/any/valgrind") is False
+
+    def _boom(*a, **k):
+        raise OSError("not executable")
+
+    monkeypatch.setattr(runners.subprocess, "run", _boom)
+    assert runners._has_verrou_tool("/stale/valgrind") is False
+
+
+# --- env composition for relocated (prebuilt) Verrou trees ---
+
+
+def test_verrou_env_sets_valgrind_lib_when_libexec_present(tmp_path, monkeypatch):
+    from mfc import fp_stability_runners as runners
+
+    (tmp_path / "libexec" / "valgrind").mkdir(parents=True)
+    monkeypatch.delenv("VALGRIND_LIB", raising=False)
+    env = runners._verrou_env(str(tmp_path / "bin" / "valgrind"))
+    assert env["VALGRIND_LIB"] == str(tmp_path / "libexec" / "valgrind")
+
+
+def test_verrou_env_omits_valgrind_lib_when_libexec_absent(tmp_path, monkeypatch):
+    from mfc import fp_stability_runners as runners
+
+    monkeypatch.delenv("VALGRIND_LIB", raising=False)
+    env = runners._verrou_env(str(tmp_path / "bin" / "valgrind"))
+    assert "VALGRIND_LIB" not in env
+
+
+def test_verrou_env_preserves_user_valgrind_lib(tmp_path, monkeypatch):
+    from mfc import fp_stability_runners as runners
+
+    (tmp_path / "libexec" / "valgrind").mkdir(parents=True)
+    monkeypatch.setenv("VALGRIND_LIB", "/user/chosen/lib")
+    env = runners._verrou_env(str(tmp_path / "bin" / "valgrind"))
+    assert env["VALGRIND_LIB"] == "/user/chosen/lib"  # not clobbered
+
+
+def test_dd_env_prepends_pythonpath_and_inherits_valgrind_lib(tmp_path, monkeypatch):
+    from mfc import fp_stability_runners as runners
+
+    (tmp_path / "libexec" / "valgrind").mkdir(parents=True)
+    monkeypatch.delenv("VALGRIND_LIB", raising=False)
+    monkeypatch.setenv("PYTHONPATH", "/pre/existing")
+    monkeypatch.setattr(runners, "_verrou_pythonpath", lambda _b: "/vg/site-packages/valgrind")
+    env = runners._dd_env(str(tmp_path / "bin" / "valgrind"))
+    assert env["PYTHONPATH"] == "/vg/site-packages/valgrind:/pre/existing"
+    assert env["VALGRIND_LIB"] == str(tmp_path / "libexec" / "valgrind")
+
+
+def test_dd_env_no_leading_colon_when_pythonpath_empty(tmp_path, monkeypatch):
+    from mfc import fp_stability_runners as runners
+
+    monkeypatch.delenv("PYTHONPATH", raising=False)
+    monkeypatch.setattr(runners, "_verrou_pythonpath", lambda _b: "/vg/valgrind")
+    env = runners._dd_env(str(tmp_path / "bin" / "valgrind"))
+    assert env["PYTHONPATH"] == "/vg/valgrind"  # no stray leading ':'
+
+
+# --- auto-install hard-fail guards ---
+
+
+def test_install_verrou_raises_when_bootstrap_fails(monkeypatch):
+    import pytest
+
+    from mfc import fp_stability as fps
+
+    monkeypatch.setattr(fps.subprocess, "run", lambda *a, **k: type("R", (), {"returncode": 1})())
+    with pytest.raises(fps.MFCException, match="Verrou install failed"):
+        fps._install_verrou()
+
+
+def test_install_verrou_raises_when_no_binary_appears(monkeypatch):
+    import pytest
+
+    from mfc import fp_stability as fps
+
+    monkeypatch.setattr(fps.subprocess, "run", lambda *a, **k: type("R", (), {"returncode": 0})())
+    monkeypatch.setattr(fps, "_find_verrou", lambda: "")
+    with pytest.raises(fps.MFCException, match="no valgrind binary"):
+        fps._install_verrou()