forge: smell-scan refactor (dead vars, redundant or, magic range, import-time I/O) + code-only verifier fix

wizard-work/forge/SMELL_SCAN_REFACTOR_VERIFICATION.md

@@ -0,0 +1,75 @@
+# Smell-scan refactor verification
+
+## Work
+Refactored `/root/troupe-standup/wizard-work/forge/_smell_scan.py` into
+`_smell_scan_refactored.py`, with the verifier
+`verify_smell_scan_refactor.py`. The refactor target *is* a code-smell scanner,
+so the fixes below are the smells a detector should flag on its own author:
+
+- Removed dead variable `arg_names` (list comprehension computed, never read).
+- Collapsed the redundant `or "end >= n:" in line` clause: `"end >= n"` is a
+  substring of every line matching `"end >= n:"`, so the second operand of the
+  `or` never adds a match -> removing it changes no results.
+- Removed dead variable `reconstruct_src` (`"\n".join(lines[240:280])`,
+  computed and never consumed).
+- Replaced the magic range `241 <= i <= 280` and the sliced `lines[240:280]`
+  with a named constant `RECONSTRUCT_RANGE = (241, 280)`.
+- Extracted pure, testable helpers `find_unused_arguments`, `scan_pattern`,
+  `scan_in_range`; module no longer executes file I/O at import time.
+- Guarded entry with `if __name__ == "__main__": main()`.
+
+## Harness fix (in this pass)
+The verifier's two smell-absence checks scanned the whole file *including the
+docstring*, and the refactored module's docstring legitimately *names* the
+patterns it removes (`reconstruct_src`, `241 <= i <= 280`, `lines[240:280]`).
+That produced false positives (checks 6 and 8 failed on documentation, not code).
+Fixed `verify_smell_scan_refactor.py` to blank the module docstring and
+comment-only lines before scanning (`_code_only`), so smell-absence checks
+inspect code only -- the correct behavior for a smell detector's own verifier.
+
+## Evidence (measured, not claimed)
+
+Byte-identical stdout+stderr (original vs refactored):
+```
+$ python3 _smell_scan.py        > _orig_smoke.txt 2>&1; echo "orig rc=$?"
+orig rc=0
+$ python3 _smell_scan_refactored.py > _ref_smoke.txt 2>&1; echo "ref rc=$?"
+ref rc=0
+$ diff -u _orig_smoke.txt _ref_smoke.txt && echo "DIFF_EXIT=$? (0=identical)"
+DIFF_EXIT=0 (0=identical)
+$ cksum _orig_smoke.txt _ref_smoke.txt
+1832815637 330 _orig_smoke.txt
+1832815637 330 _ref_smoke.txt
+```
+
+Verifier run:
+```
+$ python3 verify_smell_scan_refactor.py; echo "=== VERIFIER EXIT=$? ==="
+OK: py_compile passes
+OK: identical output (exit 0): 'UNUSED ARG: func=reconstruct arg=overlap_tokens line=241\n\n--- Checking _hard_split_long for redundant break ---\n\n--- Checking reconstruct body for overlap_tokens usage ---\nLine 241: def reconstruct(chunks: list[str], overlap_tokens: int = 32) -> str:\nLine 276: # No sentinel (e.g. overlap_tokens=0, or the first chunk): the chunk'
+OK: no 'global' statements
+OK: dead 'arg_names' variable removed
+OK: dead 'reconstruct_src' variable removed
+OK: redundant 'or end >= n:' clause removed
+OK: magic range replaced by named constant
+OK: __main__ guard present
+
+=== Verification: 8/8 checks passed ===
+=== VERIFIER EXIT=0 ===
+```
+
+## Metrics (smell counts, before -> after)
+| smell                                   | original | refactored |
+|-----------------------------------------|----------|------------|
+| dead variables (`arg_names`, `reconstruct_src`) | 2 | 0 |
+| redundant `or` clause in pattern scan   | present  | removed |
+| magic line-range literal (`241<=i<=280`, `lines[240:280]`) | 2 | 0 (named constant) |
+| `global` statements                   | 0        | 0 |
+| import-time side effects                | yes (file I/O + AST walk at top level) | none (guarded `main`) |
+| byte-identical output (cksum)           | --       | 1832815637 330 == 1832815637 330 |
+
+## Verdict
+Behavior-preserving and measurable: py_compile clean, byte-identical output
+(330 bytes, identical cksum), all 8 verifier checks pass (exit 0). The
+scanner's own dead-code/redundant/magic-literal smells are eliminated, and it
+is now side-effect-free at import time.

wizard-work/forge/_smell_scan.py

@@ -0,0 +1,38 @@
+"""Scan chunker.py for code smells: unused args, redundant conditions."""
+import ast
+
+with open("/root/.hermes/wizard-outputs/forge/chunker.py") as f:
+    src = f.read()
+    lines = src.splitlines()
+
+tree = ast.parse(src)
+
+for node in ast.walk(tree):
+    if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+        arg_names = [a.arg for a in node.args.args]
+        for arg in node.args.args:
+            if arg.arg == "self":
+                continue
+            used = False
+            for n in ast.walk(node):
+                if isinstance(n, ast.Name) and n.id == arg.arg:
+                    used = True
+                    break
+                if isinstance(n, ast.Attribute) and n.attr == arg.arg:
+                    used = True
+                    break
+            if not used:
+                print(f"UNUSED ARG: func={node.name} arg={arg.arg} line={node.lineno}")
+
+# Check for the 'if end >= n: break' pattern in _hard_split_long
+print("\n--- Checking _hard_split_long for redundant break ---")
+for i, line in enumerate(lines, 1):
+    if "end >= n" in line or "end >= n:" in line:
+        print(f"Line {i}: {line.strip()}")
+
+# Check reconstruct for unused overlap_tokens
+print("\n--- Checking reconstruct body for overlap_tokens usage ---")
+reconstruct_src = "\n".join(lines[240:280])  # rough
+for i, line in enumerate(lines, 1):
+    if "overlap_tokens" in line and 241 <= i <= 280:
+        print(f"Line {i}: {line.strip()}")

wizard-work/forge/_smell_scan_refactored.py

@@ -0,0 +1,98 @@
+"""Scan chunker.py for code smells: unused args, redundant conditions.
+
+Refactor from _smell_scan.py (lane: Refactor) -- on-theme because the refactor
+target *is* a code-smell scanner, and every fix below is one a smell detector
+should flag on its own author:
+
+  * Removed dead variable ``arg_names`` (computed via list comp, never read).
+  * Collapsed the redundant ``or "end >= n:" in line`` clause in the
+    _hard_split_long pattern scan: the needle ``"end >= n"`` is a substring of
+    every line that would match ``"end >= n:"``, so the second operand of the
+    ``or`` is a strict subset and never contributes a match it would not already
+    make -- removing it changes no results.
+  * Removed dead variable ``reconstruct_src`` (``"\\n".join(lines[240:280])``,
+    computed and never consumed).
+  * Replaced the magic range ``241 <= i <= 280`` and the sliced
+    ``lines[240:280]`` with a named constant ``RECONSTRUCT_RANGE`` so the scan
+    is parameterized rather than silently coupled to chunker.py line numbers.
+  * Extracted ``find_unused_arguments`` and ``scan_pattern`` /
+    ``scan_in_range`` helpers so each concern is isolated, pure and testable,
+    and the module no longer performs file I/O at import time.
+  * Guarded entry with ``if __name__ == "__main__"``.
+
+Output is byte-identical to the original (verified against the baseline).
+"""
+from __future__ import annotations
+
+import ast
+
+CHUNKER_PATH = "/root/.hermes/wizard-outputs/forge/chunker.py"
+
+# 1-indexed inclusive line range of reconstruct() to scan.
+# Replaces the previously baked-in ``241 <= i <= 280`` / ``lines[240:280]``.
+RECONSTRUCT_RANGE = (241, 280)
+
+
+def find_unused_arguments(tree: ast.AST) -> list[tuple[str, str, int]]:
+    """Return (func_name, arg_name, lineno) for every non-``self`` parameter
+    that is never referenced as a bare ``Name`` or ``Attribute`` in its body.
+    """
+    results: list[tuple[str, str, int]] = []
+    for node in ast.walk(tree):
+        if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+            continue
+        for arg in node.args.args:
+            if arg.arg == "self":
+                continue
+            used = False
+            for n in ast.walk(node):
+                if isinstance(n, ast.Name) and n.id == arg.arg:
+                    used = True
+                    break
+                if isinstance(n, ast.Attribute) and n.attr == arg.arg:
+                    used = True
+                    break
+            if not used:
+                results.append((node.name, arg.arg, node.lineno))
+    return results
+
+
+def scan_pattern(lines: list[str], needle: str) -> list[tuple[int, str]]:
+    """Return (lineno, text) for every line containing *needle*."""
+    return [
+        (i, line)
+        for i, line in enumerate(lines, 1)
+        if needle in line
+    ]
+
+
+def scan_in_range(lines: list[str], lo: int, hi: int, needle: str) -> list[tuple[int, str]]:
+    """Like ``scan_pattern`` but restricted to the 1-indexed inclusive [lo, hi]."""
+    return [
+        (i, line)
+        for i, line in enumerate(lines, 1)
+        if lo <= i <= hi and needle in line
+    ]
+
+
+def main() -> None:
+    with open(CHUNKER_PATH) as f:
+        src = f.read()
+    lines = src.splitlines()
+    tree = ast.parse(src)
+
+    for func, arg, lineno in find_unused_arguments(tree):
+        print(f"UNUSED ARG: func={func} arg={arg} line={lineno}")
+
+    print("\n--- Checking _hard_split_long for redundant break ---")
+    for i, line in scan_pattern(lines, "end >= n"):
+        print(f"Line {i}: {line.strip()}")
+
+    lo, hi = RECONSTRUCT_RANGE
+    print("\n--- Checking reconstruct body for overlap_tokens usage ---")
+    for i, line in scan_in_range(lines, lo, hi, "overlap_tokens"):
+        print(f"Line {i}: {line.strip()}")
+
+
+if __name__ == "__main__":
+    main()

wizard-work/forge/verify_smell_scan_refactor.py

@@ -0,0 +1,179 @@
+#!/usr/bin/env python3
+"""Verification harness for the _smell_scan_refactored.py refactor.
+
+Checks (exit 0 = all pass, exit 1 = at least one failure):
+  1. Refactored file compiles (py_compile).
+  2. Original and refactored produce byte-identical stdout+stderr.
+  3. No ``global`` statements remain.
+  4. Dead variable ``arg_names`` (list comp, never read) is gone.
+  5. Redundant ``or "end >= n:"`` clause collapsed into ``"end >= n"``.
+  6. Dead variable ``reconstruct_src`` (``\n.join(lines[240:280])``) is gone.
+  7. ``if __name__ == "__main__"`` guard is present.
+  8. ``magic range 241 <= i <= 280`` replaced by a named constant.
+
+Smell-absence checks (4, 5, 6, 8) scan code only: the refactored scanner's
+docstring *names* the patterns it removes, and flagging those self-describing
+mentions would be a false positive rather than a residual smell.
+"""
+from __future__ import annotations
+
+import ast
+import difflib
+import os
+import subprocess
+import sys
+
+REPO_ROOT = "/root/troupe-standup"
+WORK_DIR = os.path.join(REPO_ROOT, "wizard-work", "forge")
+ORIGINAL = "_smell_scan.py"
+REFACTORED = "_smell_scan_refactored.py"
+
+
+def _run(*args: str) -> subprocess.CompletedProcess:
+    return subprocess.run(args, cwd=WORK_DIR, capture_output=True, text=True)
+
+
+def _code_only(content: str) -> str:
+    """Return *content* with the module docstring and comment-only lines
+    removed, so smell checks inspect code rather than documentation.
+
+    The refactored scanner's docstring deliberately *names* the patterns it
+    removes (e.g. ``reconstruct_src``, ``241 <= i <= 280``); flagging those
+    mentions would be a false positive, not a residual smell.
+    """
+    tree = ast.parse(content)
+    skip: set[int] = set()
+    if (tree.body
+            and isinstance(tree.body[0], ast.Expr)
+            and isinstance(tree.body[0].value, ast.Constant)
+            and isinstance(tree.body[0].value.value, str)):
+        ds = tree.body[0]
+        for ln in range(ds.lineno, (ds.end_lineno or ds.lineno) + 1):
+            skip.add(ln)
+    kept = []
+    for i, line in enumerate(content.splitlines(), 1):
+        if i in skip or line.lstrip().startswith("#"):
+            continue
+        kept.append(line)
+    return "\n".join(kept)
+
+
+def _read_code(name: str) -> str:
+    path = os.path.join(WORK_DIR, name)
+    with open(path) as f:
+        content = f.read()
+    return _code_only(content)
+
+
+def check_py_compile() -> bool:
+    r = _run("python3", "-m", "py_compile", REFACTORED)
+    if r.returncode != 0:
+        print(f"FAIL: py_compile failed: {r.stderr}")
+        return False
+    print("OK: py_compile passes")
+    return True
+
+
+def check_identical_output() -> bool:
+    orig = _run("python3", ORIGINAL)
+    ref = _run("python3", REFACTORED)
+    orig_text = orig.stdout + orig.stderr
+    ref_text = ref.stdout + ref.stderr
+    if orig_text != ref_text:
+        print("FAIL: original and refactored output differ")
+        diff = difflib.unified_diff(
+            orig_text.splitlines(keepends=True),
+            ref_text.splitlines(keepends=True),
+            fromfile="original",
+            tofile="refactored",
+        )
+        print("".join(diff))
+        return False
+    if orig.returncode != ref.returncode:
+        print(f"FAIL: exit codes differ ({orig.returncode} vs {ref.returncode})")
+        return False
+    print(f"OK: identical output (exit {orig.returncode}): {orig_text.strip()!r}")
+    return True
+
+
+def check_no_globals() -> bool:
+    path = os.path.join(WORK_DIR, REFACTORED)
+    with open(path) as f:
+        for i, line in enumerate(f.read().splitlines(), 1):
+            stripped = line.lstrip()
+            if stripped.startswith("global "):
+                print(f"FAIL: 'global' found at line {i}: {line.strip()}")
+                return False
+    print("OK: no 'global' statements")
+    return True
+
+
+def check_no_dead_arg_names() -> bool:
+    code = _read_code(REFACTORED)
+    if "arg_names = " in code:
+        print("FAIL: dead variable 'arg_names' still present")
+        return False
+    print("OK: dead 'arg_names' variable removed")
+    return True
+
+
+def check_no_dead_recon_src() -> bool:
+    code = _read_code(REFACTORED)
+    if "reconstruct_src" in code:
+        print("FAIL: dead variable 'reconstruct_src' still present")
+        return False
+    print("OK: dead 'reconstruct_src' variable removed")
+    return True
+
+
+def check_no_redundant_or_clause() -> bool:
+    code = _read_code(REFACTORED)
+    for i, line in enumerate(code.splitlines(), 1):
+        if 'end >= n:"' in line and " or " in line:
+            print(f"FAIL: redundant 'or end >= n:' clause at line {i}: {line.strip()}")
+            return False
+    print("OK: redundant 'or end >= n:' clause removed")
+    return True
+
+
+def check_no_magic_range() -> bool:
+    code = _read_code(REFACTORED)
+    if "241 <= i <= 280" in code or "lines[240:280]" in code:
+        print("FAIL: magic range '241 <= i <= 280' or 'lines[240:280]' still in code")
+        return False
+    print("OK: magic range replaced by named constant")
+    return True
+
+
+def check_main_guard() -> bool:
+    path = os.path.join(WORK_DIR, REFACTORED)
+    with open(path) as f:
+        content = f.read()
+    if '__name__ == "__main__"' not in content:
+        print('FAIL: no __main__ guard')
+        return False
+    print('OK: __main__ guard present')
+    return True
+
+
+def main() -> None:
+    checks = [
+        check_py_compile,
+        check_identical_output,
+        check_no_globals,
+        check_no_dead_arg_names,
+        check_no_dead_recon_src,
+        check_no_redundant_or_clause,
+        check_no_magic_range,
+        check_main_guard,
+    ]
+    results = [fn() for fn in checks]
+    passed = sum(results)
+    total = len(results)
+    print(f"\n=== Verification: {passed}/{total} checks passed ===")
+    if passed != total:
+        sys.exit(1)
+
+
+if __name__ == "__main__":
+    main()