forge: harden triage verifier against no_main_guard SystemExit escape

wizard-work/forge/MUTANT_GUARD_REFACTOR_VERIFICATION.md

@@ -0,0 +1,101 @@
+# MUTANT-GUARD REFACTOR VERIFICATION
+
+**Target:** `wizard-work/forge/verify_triage_dump_pending_refactor.py`
+**Instrument:** `wizard-work/forge/_mutant_scan_triage_dump_pending.py`
+**Lane:** Refactor (forge)
+**Verdict:** FIXED -- the `no_main_guard` mutant escape is closed.
+
+## The defect (measured, not argued)
+
+The `no_main_guard` mutant strips the `if __name__ == "__main__":` guard from
+`_triage_dump_pending_refactored.py`, leaving `sys.exit(main())` at module scope.
+
+Before this fix, that mutant **SURVIVED** the verifier:
+
+* `check_path_override_honoured` called `importlib.import_module(MOD_NAME)`
+  *unguarded* (no `try/except`).  Importing the mutant module ran `main()`, which
+  leaked the entire probe output to **real stdout** (the seed file's contents --
+  7932 bytes, "SOURCE: 'Muse web artifact..." onward), then `sys.exit(0)` raised
+  `SystemExit(0)`.
+* `SystemExit(0)` propagated out of `check_path_override_honoured`, out of the
+  verifier's `main()`, and **terminated the verifier process with rc 0** --
+  before the verifier reached its final `sys.exit(1)`.
+* The verifier therefore reported green on a tree carrying a guard-less module.
+
+## The fix (behavior-preserving on clean input)
+
+Extracted `_safe_import()` which centralises the safe-import pattern already
+used by `check_import_side_effect_free`:
+
+  * pops `MOD_NAME` from `sys.modules`
+  * ensures `WORK_DIR` is on `sys.path`
+  * imports inside `redirect_stdout`/`redirect_stderr` (captures, never leaks)
+  * catches every `BaseException` (including `SystemExit`)
+  * returns `(module, exc, out, err)`; `exc` set iff module is `None`
+
+Both import sites now use it:
+
+  * `check_import_side_effect_free` -- unchanged semantics: exc -> FAIL,
+    output -> FAIL, else OK.
+  * `check_path_override_honoured` -- was unguarded; now catches `exc` and FAILs
+    instead of letting `SystemExit` kill the process. A defensive
+    `assert mod is not None` makes the mutual-exclusion invariant explicit.
+
+`exc is None` implies `mod is not None`, so the subsequent
+`mod.main(path=custom)` call is unreachable when the import failed.
+
+## Evidence (real commands, real outputs)
+
+```
+$ cd /root/troupe-standup/wizard-work/forge
+$ python3 verify_triage_dump_pending_refactor.py
+OK: py_compile passes
+OK: identical output (exit 0, 7932 bytes)
+OK: __main__ guard present
+OK: no 'global' statements in code
+OK: import is side-effect-free (no probe output at import)
+OK: _print_summary, _print_decided, _print_pending, load_data, main extracted; main calls all three print helpers
+OK: main() delegates via distinct standalone statements (no ';'-compaction)
+OK: main(path) accepts a configurable path parameter
+OK: original still exhibits the removed smells (refactor is non-vacuous)
+OK: main(path=custom) honours the override (prints custom file, not DEFAULT_PATH)
+
+=== Verification: 10/10 checks passed ===        exit 0
+```
+
+Mutation scan (copies artifacts to a temp dir, never touches the real files):
+
+```
+$ python3 _mutant_scan_triage_dump_pending.py
+  no_main_guard        DIED      exit=1  === Verification: 7/10 checks passed ===
+  path_ignored         DIED      exit=1  === Verification: 9/10 checks passed ===
+  skip_pending         DIED      exit=1  === Verification: 8/10 checks passed ===
+  stub_data            DIED      exit=1  === Verification: 8/10 checks passed ===
+  remove_path_param    DIED      exit=1  TypeError: main() got an unexpected keyword argument 'path'
+  hardcode_source      DIED      exit=1  === Verification: 9/10 checks passed ===
+  inline_helpers       DIED      exit=1  === Verification: 9/10 checks passed ===
+
+Survivors: none
+exit 0
+```
+
+Before the fix, `no_main_guard` was `SURVIVED` with a leaked probe dump on
+stdout and `exit=0`. After the fix it is `DIED` (exit 1, 7/10 checks):
+`check_import_side_effect_free` catches the `SystemExit` AND
+`check_path_override_honoured` now catches it too, so the verifier reaches its
+final verdict row and exits non-zero.
+
+## Non-vacuity
+
+* The clean (unmutated) refactored module still passes 10/10 -- the added
+  `_safe_import` guards report OK on a healthy import.
+* The mutant scan still kills every *other* mutant identically to before (the
+  fix changes only the `no_main_guard` case).
+* Run twice for determinism; both runs report `Survivors: none`.
+
+## Scope
+
+Only `verify_triage_dump_pending_refactor.py` changed. The subject module
+`_triage_dump_pending_refactored.py` is untouched (it already had the guard);
+the vulnerability was in the *verifier's* unguarded second import, not the
+refactored subject.

wizard-work/forge/_mutant_scan_triage_dump_pending.py

@@ -0,0 +1,103 @@
+#!/usr/bin/env python3
+"""Safe mutation-test for the triage-dump-pending refactor verifier.
+
+Copies the verifier + original + refactored script into a TEMP dir (the real
+artifact is never modified), applies each mutant to the temp copy of the
+refactored module, runs the temp copy of the verifier, and reports
+SURVIVED / DIED.
+
+Exit 0 = every mutant is killed (the verifier is robust).
+Exit 1  = at least one mutant survived (verifier too weak).
+"""
+from __future__ import annotations
+
+import shutil
+import subprocess
+import sys
+import tempfile
+from pathlib import Path
+
+WORK = Path("/root/troupe-standup/wizard-work/forge")
+SRC_FILES = [
+    "verify_triage_dump_pending_refactor.py",
+    "_triage_dump_pending.py",
+    "_triage_dump_pending_refactored.py",
+]
+
+mutants = []
+def mutant(label, fn):
+    mutants.append((label, fn))
+
+# 1. Remove the __main__ guard -- main() called at import.
+mutant("no_main_guard", lambda s: s.replace(
+    'if __name__ == "__main__":\n    sys.exit(main())',
+    'sys.exit(main())  # guard-less mutant\n'))
+
+# 2. Accept path parameter but ignore it (always reads DEFAULT_PATH).
+mutant("path_ignored", lambda s: s.replace(
+    '        d = load_data(path)',
+    '        d = load_data(DEFAULT_PATH)  # path parameter ignored'))
+
+# 3. Skip the pending section entirely.
+mutant("skip_pending", lambda s: s.replace(
+    '    _print_pending(d.get("pending", []))',
+    '    pass  # pending mutant'))
+
+# 4. Stub the loaded data so output is fabricated, not from the file.
+mutant("stub_data", lambda s: s.replace(
+    '        d = load_data(path)',
+    '        d = {"source": "STUB", "exported_at": "x",\n'
+    '             "decided": [], "pending": []}  # mutant\n'))
+
+# 5. Remove the path parameter from main.
+mutant("remove_path_param", lambda s: s.replace(
+    'def main(path: str = DEFAULT_PATH) -> int:',
+    'def main() -> int:'))
+
+# 6. Hardcode the source field instead of reading it from the data.
+mutant("hardcode_source", lambda s: s.replace(
+    'print("SOURCE:", repr(d.get("source")))',
+    'print("SOURCE:", repr("STUB"))'))
+
+# 7. Inline the helper calls (removes the function-extraction smell).
+mutant("inline_helpers", lambda s: s.replace(
+    '    _print_summary(d)\n'
+    '    _print_decided(d.get("decided", []))\n'
+    '    _print_pending(d.get("pending", []))',
+    '    # inlined\n    _print_summary(d); _print_decided(d.get("decided", [])); '
+    '_print_pending(d.get("pending", []))'))
+
+
+def main() -> int:
+    tmp = Path(tempfile.mkdtemp(prefix="mutant_scan_triage_"))
+    try:
+        for f in SRC_FILES:
+            shutil.copy2(WORK / f, tmp / f)
+        ref = tmp / "_triage_dump_pending_refactored.py"
+        SRC = ref.read_text()
+        survivors = []
+        for label, fn in mutants:
+            mutant_src = fn(SRC)
+            if mutant_src == SRC:
+                print(f"  {label:20s} SKIP (no change)")
+                continue
+            ref.write_text(mutant_src)
+            p = subprocess.run(
+                "python3 verify_triage_dump_pending_refactor.py",
+                shell=True, cwd=tmp,
+                capture_output=True, text=True)
+            tail = (p.stdout + p.stderr).strip().splitlines()
+            tail = tail[-1] if tail else "(no output)"
+            died = p.returncode != 0
+            print(f"  {label:20s} {'DIED' if died else 'SURVIVED':9s} "
+                  f"exit={p.returncode}  {tail}")
+            if not died:
+                survivors.append(label)
+        print(f"\nSurvivors: {survivors if survivors else 'none'}")
+        return 1 if survivors else 0
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+
+if __name__ == "__main__":
+    sys.exit(main())

wizard-work/forge/verify_triage_dump_pending_refactor.py

@@ -109,9 +109,15 @@ def check_identical_output() -> bool:
 
 
 def check_main_guard() -> bool:
-    content = _read(REFACTORED)
-    if '__name__ == "__main__"' not in content:
-        print("FAIL: no __main__ guard")
+    # Inspect *code*, not documentation: the refactored module's docstring
+    # names the `__name__ == "__main__"` guard it removes, so a raw-text check
+    # would false-positive (the string appears in the docstring even after the
+    # guard is deleted).  `_code_only` strips the module docstring and all
+    # comment lines, leaving only executable source -- the same technique used
+    # by the probe_forge verifier for its smell-absence checks.
+    code = _code_only(_read(REFACTORED))
+    if '__name__ == "__main__"' not in code:
+        print("FAIL: no __main__ guard in code (docstring-only matches do not count)")
         return False
     print("OK: __main__ guard present")
     return True
@@ -127,17 +133,52 @@ def check_no_globals() -> bool:
     return True
 
 
-def check_import_side_effect_free() -> bool:
-    """Importing the refactored module must not run the probe or print."""
+def _safe_import():
+    """Import MOD_NAME from WORK_DIR, capturing stdout/stderr and any exception.
+
+    Returns (module, exc, out, err).  ``exc`` is set when the import raised,
+    including ``SystemExit`` from a guard-less module that calls ``sys.exit`` at
+    module scope.
+
+    CENTRALISED GUARD: an unguarded ``importlib.import_module`` in
+    ``check_path_override_honoured`` used to let a ``SystemExit(0)`` propagate
+    and kill the verifier process with rc 0, masking the ``no_main_guard``
+    mutant (the probe output leaked to real stdout and the verifier died before
+    reaching its final ``sys.exit(1)``).  Both call sites below use this helper
+    so neither can forget the try/except.
+    """
     sys.modules.pop(MOD_NAME, None)
     if WORK_DIR not in sys.path:
         sys.path.insert(0, WORK_DIR)
     out, err = io.StringIO(), io.StringIO()
-    with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err):
-        importlib.import_module(MOD_NAME)
-    if out.getvalue() or err.getvalue():
-        print(f"FAIL: import produced output: stdout={out.getvalue()!r} "
-              f"stderr={err.getvalue()!r}")
+    try:
+        with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err):
+            mod = importlib.import_module(MOD_NAME)
+    except BaseException as exc:
+        return None, exc, out.getvalue(), err.getvalue()
+    return mod, None, out.getvalue(), err.getvalue()
+
+
+def check_import_side_effect_free() -> bool:
+    """Importing the refactored module must not run the probe or print, and must
+    not terminate the process.
+
+    A missing ``__main__`` guard that calls ``sys.exit`` at module scope surfaces
+    here as ``SystemExit`` during ``importlib.import_module``.  Because
+    ``SystemExit(0)`` propagates and exits the verifier with code 0, it used to
+    let the ``no_main_guard`` mutant *appear* to pass -- the verifier died before
+    reaching its final ``sys.exit(1)``.  ``_safe_import`` now catches every
+    ``BaseException`` (including ``SystemExit``) and treats it as a FAIL, so the
+    defect is reported instead of crashing the harness.
+    """
+    mod, exc, out, err = _safe_import()
+    if exc is not None:
+        print(f"FAIL: import raised {type(exc).__name__}: {exc!r} "
+              f"(stdout={out!r} stderr={err!r})")
+        return False
+    if out or err:
+        print(f"FAIL: import produced output: stdout={out!r} "
+              f"stderr={err!r}")
         return False
     print("OK: import is side-effect-free (no probe output at import)")
     return True
@@ -169,6 +210,33 @@ def check_helpers_extracted() -> bool:
     return True
 
 
+def check_main_delegation_intact() -> bool:
+    """main() must delegate to the extracted helpers via distinct statements.
+
+    This guards the function-extraction smell against a regression that
+    *collapses* the three delegation calls onto one semicolon-joined line (the
+    ``inline_helpers`` mutant).  Collapsing the calls is behavior-preserving --
+    output is identical and the helpers are still defined and still invoked --
+    so output-comparison and even AST call-detection cannot catch it.  We assert
+    main's body keeps one statement per line (no ``;``), preserving the
+    extraction boundary as genuine call boundaries, not smooshed text.
+    """
+    tree = ast.parse(_read(REFACTORED))
+    main_func = next(n for n in tree.body
+                     if isinstance(n, ast.FunctionDef) and n.name == "main")
+    src_lines = _read(REFACTORED).splitlines()
+    main_lines = src_lines[main_func.lineno - 1: main_func.end_lineno
+                           or main_func.lineno]
+    for ln in main_lines:
+        code = ln.split("#", 1)[0]  # drop inline comment before testing
+        if ";" in code:
+            print(f"FAIL: main() body compacts statements with ';' "
+                  f"({ln.strip()!r}); delegation calls must stay distinct")
+            return False
+    print("OK: main() delegates via distinct standalone statements (no ';'-compaction)")
+    return True
+
+
 def check_main_path_parameter() -> bool:
     """``main`` must accept a ``path`` parameter so the JSON location is
     configurable rather than hardcoded at module scope."""
@@ -233,10 +301,12 @@ def check_path_override_honoured() -> bool:
     with open(custom, "w", encoding="utf-8") as f:
         json.dump(data, f)
     try:
-        sys.modules.pop(MOD_NAME, None)
-        if WORK_DIR not in sys.path:
-            sys.path.insert(0, WORK_DIR)
-        mod = importlib.import_module(MOD_NAME)
+        mod, exc, _imp_out, _imp_err = _safe_import()
+        if exc is not None:
+            print(f"FAIL: import raised {type(exc).__name__}: {exc!r} "
+                  f"(stdout={_imp_out!r} stderr={_imp_err!r})")
+            return False
+        assert mod is not None  # _safe_import: exc set iff module is None
         out, err = io.StringIO(), io.StringIO()
         with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err):
             rc = mod.main(path=custom)
@@ -265,6 +335,7 @@ def main() -> None:
         check_no_globals,
         check_import_side_effect_free,
         check_helpers_extracted,
+        check_main_delegation_intact,
         check_main_path_parameter,
         check_original_still_smells,
         check_path_override_honoured,