diff --git a/wizard-work/forge/PROBE_FORGE_REFACTOR_VERIFICATION.md b/wizard-work/forge/PROBE_FORGE_REFACTOR_VERIFICATION.md new file mode 100644 index 0000000..2b7bd8d --- /dev/null +++ b/wizard-work/forge/PROBE_FORGE_REFACTOR_VERIFICATION.md @@ -0,0 +1,98 @@ +PROBE_FORGE_REFACTOR_VERIFICATION.md -- forge refactor verification. + +Target: /root/troupe-standup/wizard-work/forge/_probe_forge.py + -> /root/troupe-standup/wizard-work/forge/_probe_forge_refactored.py +Lane: Refactor (code review + refactors) +Date: 2026-10-25 (refactor); verifier hardened 2026-10-26 + +PURPOSE + The forge lane refactored `_probe_forge.py` -- a round-trip probe of the + forge chunker -- into `_probe_forge_refactored.py`. The refactor is + behavior-preserving: it exercises the same chunker API (chunk_text / + reconstruct) with the same inputs, emitting byte-identical output. + +SMELLS REMOVED (from the original `_probe_forge.py`) + 1. Import-time side effect: the original ran its entire probe at module + top-level (a module-level `for` loop printing immediately on import). + There was no `if __name__ == "__main__"` guard, so importing the module + executed and printed the probe output. The refactored version moves all + execution under `main()` with an explicit `__main__` guard. + + 2. Dead variable `shorter`: computed as + `shorter = min(len(r), len(text_stripped))` inside the for-else `else` + branch but never read -- every subsequent statement used + `len(r)` and `len(text_stripped)` directly. Removed; output unchanged. + + 3. Inlined first-diff search: the inline `for i in range(min(...))` loop + with its for-else side-effects is extracted into + `_find_first_diff(actual, expected) -> int | None`, a pure function + returning the mismatch index or `None`, decoupling the search from I/O so + the control flow reads as intent ("is there a diff, and if so, show it"). + + 4. Inlined chunk-structure printing: the loop that prints the first 5 chunks + with sentinels revealed is extracted into + `_print_chunk_structure(chunks, sentinel, n)`, isolating the formatting + concern and moving the data (`base`, `text`, `cs`, `r`) inside `main()` + instead of at module scope. + +VERIFIER HARDENING (the fix, applied after the refactor commit) + The verifier's import was done through an unguarded `importlib.import_module` + call. A guard-less subject that calls `sys.exit` at module scope raises + `SystemExit(0)` during import, which propagated and killed the verifier + process with rc 0 BEFORE reaching its final `assert passed == total, sys.exit(1)`. + Net effect on a defective tree: output leaked to real stdout and the harness + reported green (rc 0). This is exactly the `no_main_guard` mutant it is meant + to catch. + + Fix: replaced `_import_refactored()` with `_safe_import()`, which wraps the + import in try/except BaseException (so SystemExit is caught), captures + stdout/stderr via redirect, and returns `(module, exc, out, err)`. Both call + sites (`check_import_side_effect_free` and `check_find_first_diff_correctness`) + use it, so a guard-less subject now FAILS loudly instead of escaping. With the + real subject carrying a proper `__main__` guard, the import is clean and + every check proceeds normally. + +SAFETY: byte-identical output + The refactor does NOT change behavior. The chunker calls are identical + (`chunker.chunk_text(text, max_tokens=64, overlap_tokens=8)` and + `chunker.reconstruct(cs, 8)`), the base string is identical (adjacent string + literals concatenate to the same value), and `_find_first_diff` preserves the + original for-else semantics: returns the index of the first mismatch, or + `None` when the common prefix matches (triggering the "no char diff" branch). + +VERIFICATION (all executed, real exit codes) + 1. python3 /root/troupe-standup/wizard-work/forge/verify_probe_forge_refactor.py + -> 12/12 checks passed, exit 0 + 2. python3 _probe_forge.py (orig stdout: 790 bytes) + 3. python3 _probe_forge_refactored.py (ref stdout: 790 bytes) + 4. diff -> IDENTICAL (no output); the verifier's check_identical_output + confirms orig.stdout == ref.stdout, orig.stderr == ref.stderr, and + orig.returncode == ref.returncode (0 == 0). + 5. Round-trip assertion: probe output reports "equal: True", nchunks: 12. + 6. pytest test_chunker_refactor.py -q -> 13 passed (chunker unchanged). + +VERIFIER CHECKS (verify_probe_forge_refactor.py, 12/12) + [OK] py_compile passes + [OK] identical output (exit 0, 790 bytes) + [OK] __main__ guard present + [OK] no 'global' statements in code + [OK] dead 'shorter' variable removed + [OK] _find_first_diff, _print_chunk_structure, main extracted; + main calls helpers + chunker API + [OK] import is side-effect-free (no probe output at import) + [OK] round-trip lossless (equal: True in probe output) + [OK] original still exhibits the removed smells (refactor non-vacuous) + [OK] _find_first_diff correct on all 10 cases (mutation kill) + [OK] 'equal' is computed dynamically from actual == expected (no hardcode) + [OK] overlap values consistent (chunk_text=8, reconstruct=8) + +NON-VACUITY GUARD + The verifier's check 9 confirms the *original* `_probe_forge.py` still + exhibits every removed smell: no `__main__` guard, dead `shorter = min(...)` + variable present, and a module-level `for` loop. The refactor is therefore + non-vacuous -- it addresses real, reproduced defects, not hypotheticals. + +VERDICT: probe_forge refactor verified -- behavior-preserving (byte-identical, +equal: True, 12 chunks), 4 smells removed, and the verifier itself hardened +against the SystemExit/no_main_guard mutant-escape that previously let a +defective tree report rc 0. Final harness: 12/12 checks, exit 0. diff --git a/wizard-work/forge/verify_probe_forge_refactor.py b/wizard-work/forge/verify_probe_forge_refactor.py index 39b4e94..d3434f0 100644 --- a/wizard-work/forge/verify_probe_forge_refactor.py +++ b/wizard-work/forge/verify_probe_forge_refactor.py @@ -86,10 +86,29 @@ def _ensure_path() -> None: sys.path.insert(0, WORK_DIR) -def _import_refactored(): +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`` 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)``, + so a defective tree reported green). Both call sites below use this + helper so neither can forget the try/except. + """ sys.modules.pop(MOD_NAME, None) _ensure_path() - return importlib.import_module(MOD_NAME) + out, err = io.StringIO(), io.StringIO() + 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_py_compile() -> bool: @@ -187,17 +206,25 @@ def check_helpers_extracted() -> bool: def check_import_side_effect_free() -> bool: - """Importing the refactored module must not run the probe or print.""" - mod = _import_refactored() - out, err = io.StringIO(), io.StringIO() - # Re-import cleanly to capture any side effects. - sys.modules.pop(MOD_NAME, None) - _ensure_path() - 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: " - f"stdout={out.getvalue()!r} stderr={err.getvalue()!r}") + """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 import. 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 @@ -247,8 +274,17 @@ def check_find_first_diff_correctness() -> bool: the return-i path is never exercised by check_identical_output. By calling _find_first_diff directly with crafted strings that DO differ, we force the return path and catch the mutants. + + Uses ``_safe_import`` so a guard-less subject (calling ``sys.exit`` at module + scope) is caught here as a FAIL rather than crashing the verifier via an + unguarded ``SystemExit``. """ - mod = _import_refactored() + 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 + assert mod is not None # _safe_import: exc set iff module is None cases = [ # (actual, expected, expected_result)