From b293175382fd7a9335147ac03b65f16d2b22e1f1 Mon Sep 17 00:00:00 2001 From: forge Date: Sat, 24 Oct 2026 21:59:24 +0000 Subject: [PATCH] forge: smell-scan refactor (dead vars, redundant or, magic range, import-time I/O) + code-only verifier fix --- .../forge/SMELL_SCAN_REFACTOR_VERIFICATION.md | 75 ++++++++ wizard-work/forge/_smell_scan.py | 38 ++++ wizard-work/forge/_smell_scan_refactored.py | 98 ++++++++++ .../forge/verify_smell_scan_refactor.py | 179 ++++++++++++++++++ 4 files changed, 390 insertions(+) create mode 100644 wizard-work/forge/SMELL_SCAN_REFACTOR_VERIFICATION.md create mode 100644 wizard-work/forge/_smell_scan.py create mode 100644 wizard-work/forge/_smell_scan_refactored.py create mode 100644 wizard-work/forge/verify_smell_scan_refactor.py diff --git a/wizard-work/forge/SMELL_SCAN_REFACTOR_VERIFICATION.md b/wizard-work/forge/SMELL_SCAN_REFACTOR_VERIFICATION.md new file mode 100644 index 0000000..abb9d9e --- /dev/null +++ b/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. diff --git a/wizard-work/forge/_smell_scan.py b/wizard-work/forge/_smell_scan.py new file mode 100644 index 0000000..542027d --- /dev/null +++ b/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()}") diff --git a/wizard-work/forge/_smell_scan_refactored.py b/wizard-work/forge/_smell_scan_refactored.py new file mode 100644 index 0000000..e45e0d4 --- /dev/null +++ b/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() diff --git a/wizard-work/forge/verify_smell_scan_refactor.py b/wizard-work/forge/verify_smell_scan_refactor.py new file mode 100644 index 0000000..499c8e0 --- /dev/null +++ b/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()