diff --git a/wizard-work/forge/EDGE_SWEEP_GLOBALS_REFACTOR_VERIFICATION.md b/wizard-work/forge/EDGE_SWEEP_GLOBALS_REFACTOR_VERIFICATION.md new file mode 100644 index 0000000..25841ff --- /dev/null +++ b/wizard-work/forge/EDGE_SWEEP_GLOBALS_REFACTOR_VERIFICATION.md @@ -0,0 +1,92 @@ +# Edge-sweep globals-elimination refactor verification + +## Work +Refactored `_edge_sweep_refactored.py` -- the broad edge-case sweep that drives +600 chunker cases (cases A-D) through a lossless round-trip plus a per-chunk +token bound -- to finish what its own docstring claimed. + +## Problem (code smell) +The file's first refactor (lane: Refactor) extracted the duplicated A-D check +sequence into `check_case`, but it **left the module-global mutable counters** +(`total`, `roundtrip_fails`, `bounds_fails`) and a `global roundtrip_fails, +bounds_fails` statement inside `check_case`. That is the exact anti-pattern +`_fuzz_probe_refactored.py` already corrected with a `Stats` dataclass two +commits up the lane. The sweep also executed its case loops at **import time** +(module top level), so importing the module for a test ran 600 cases as a +side effect -- not unit-testable. + +## Refactor (behavior-preserving) +- Added a `Stats` dataclass (`@dataclass(slots=True)`, mirroring the sibling probe). +- `check_case(...)` now takes a `stats: Stats` and mutates it in place; **the + `global` statement is deleted**. +- The four case loops are folded into `_case_a` / `_case_b` / `_case_c` / `_case_d`, + each taking `(rng, stats)`, all invoked from `main()`. +- `rng = random.Random(99)` is seeded inside `main()` with the **same seed and + consumption order** as the original module global, so the 600 generated cases + reproduce the baseline sweep case-for-case. +- `if __name__ == "__main__": sys.exit(main())` guard added; importing the module + now runs zero case logic. + +Case generation, `check_case` arithmetic, the per-chunk bounds logic, and the +two summary lines are character-for-character unchanged. The variable name and +output label `bounds_fails` are preserved verbatim (the original printed +`total=600 roundtrip_fails=0 bounds_fails=0`). + +## Evidence (measured, not claimed) + +Baseline (original file before edit) captured to `edge_sweep_baseline_output.txt`: +``` +$ python3 _edge_sweep_refactored.py > edge_sweep_baseline_output.txt 2>&1; echo "EXIT=$?" +EXIT=0 +$ cat edge_sweep_baseline_output.txt + <- leading blank line (f-string "\n" prefix) +total=600 roundtrip_fails=0 bounds_fails=0 +RESULT: ALL PASS +``` + +Refactored run captured to `edge_sweep_refactored_output.txt`: +``` +$ python3 _edge_sweep_refactored.py > edge_sweep_refactored_output.txt 2>&1; echo "EXIT=$?" +EXIT=0 +``` + +Byte-for-byte equivalence: +``` +$ diff -q edge_sweep_baseline_output.txt edge_sweep_refactored_output.txt +OUTPUT IDENTICAL (diff exits 0) +``` + +py_compile: +``` +$ python3 -m py_compile _edge_sweep_refactored.py && echo PY_COMPILE_OK +PY_COMPILE_OK +``` + +Smell elimination confirmed: +``` +$ grep -nE '^[[:space:]]*global[[:space:]]+' _edge_sweep_refactored.py; echo "global-stmt grep rc=$?" +global-stmt grep rc=1 (no global statements) +$ grep -nE '^(total|roundtrip_fails|bounds_fail) += ' _edge_sweep_refactored.py; echo "module-counter grep rc=$?" +module-counter grep rc=1 (no module-level mutable counters) +``` + +## Metrics + +| metric | original (pre-pass) | refactored | +|---------------------|---------------------|------------| +| lines | 105 | 147 (structure: Stats + 4 case fns + main) | +| `def` functions | 1 | 6 | +| `global` statements | 1 (in `check_case`) | 0 | +| module-level mutable counters | 3 | 0 | +| import-time side effects (cases) | yes (top-level) | none (guarded `main`) | +| cases swept | 600 | 600 | +| roundtrip_fails | 0 | 0 | +| bounds_failures | 0 | 0 | +| exit code | 0 | 0 | +| output vs baseline | -- | byte-identical | + +## Verdict +Refactoring is behavior-preserving and measurably equivalent: 600 cases, +byte-identical summary output, exit 0, `py_compile` clean, and the `global` +mutating-globals anti-pattern is eliminated -- the probe now imports cleanly +(side-effect-free) and is unit-testable, consistent with `_fuzz_probe_refactored.py`. diff --git a/wizard-work/forge/_edge_sweep_refactored.py b/wizard-work/forge/_edge_sweep_refactored.py index 5994148..e5acccc 100644 --- a/wizard-work/forge/_edge_sweep_refactored.py +++ b/wizard-work/forge/_edge_sweep_refactored.py @@ -1,48 +1,64 @@ """Broad edge-case sweep: token-bound + lossiness for chunker fix. -Refactored: Cases A-D previously each re-implemented the identical -``chunk_text -> reconstruct -> compare -> per-chunk bounds-check`` sequence -inline, duplicating ~15 lines four times. That logic now lives in -:func:`check_case`, so each case is reduced to a data generator that feeds the -shared verifier. Behaviour and output format are unchanged (and bounds -checking is applied uniformly to every case, including C and D where it was -previously omitted as an oversight). +Refactored (forge lane: Refactor), two passes: + + Pass 1 (the first refactor this file already carried) -- Cases A-D previously + each re-implemented the identical ``chunk_text -> reconstruct -> compare -> + per-chunk bounds-check`` sequence inline, duplicating ~15 lines four times. + That logic now lives in :func:`check_case`, so each case is reduced to a data + generator that feeds the shared verifier. Behaviour and output format are + unchanged (and bounds checking is applied uniformly to every case, including + C and D where it was previously omitted as an oversight). + + Pass 2 (this pass) -- the module still mutated module-level globals + (``total``, ``roundtrip_fails``, ``bounds_fails``) via a ``global`` keyword + inside ``check_case``: the exact anti-pattern :mod:`_fuzz_probe_refactored` + already corrected with a ``Stats`` dataclass. That mutation is now funneled + through a local ``Stats`` accumulator passed explicitly into ``check_case``, + and the four case loops are collected under :func:`main` (formerly executed + at import time). No ``global`` statement remains; the module imports cleanly + and the probe is now unit-testable (no top-level side effects). The case + generators and their RNG consumption order are unchanged, so the 600-case + sweep produces byte-identical output and exit code. """ +from __future__ import annotations + import random import sys +from dataclasses import dataclass sys.path.insert(0, "/root/.hermes/wizard-outputs/forge") -import chunker +import chunker # noqa: E402 S = chunker._OVERLAP_SENTINEL -rng = random.Random(99) WORDS = ["alpha", "bravo", "charlie", "delta", "echo", "foxtrot", "golf", "hotel"] -# Counters shared across all cases. -total = 0 -roundtrip_fails = 0 -bounds_fails = 0 + +@dataclass(slots=True) +class Stats: + """Aggregate counters for the probe run (replaces module-level globals).""" + total: int = 0 + roundtrip_fails: int = 0 + bounds_fails: int = 0 -def check_case(text: str, mt: int, ot: int, label: str) -> None: +def check_case(text: str, mt: int, ot: int, label: str, stats: Stats) -> None: """Verify a single chunker input: lossless round-trip + per-chunk token bound. Mirrors the inline checks that previously existed in every case block: reconstruct must reproduce ``text.strip()`` exactly, and every chunk body (the chunk with its sentinel-delimited context head stripped) must fit within - ``max_tokens``. Updates the module-level counters and prints the first few - failures for diagnosis. + ``max_tokens``. Updates *stats* in place and prints the first few failures + for diagnosis (matching the original ``<= 3`` throttling). """ - global roundtrip_fails, bounds_fails - expected = text.strip() cs = chunker.chunk_text(text, max_tokens=mt, overlap_tokens=ot) r = chunker.reconstruct(cs, ot) if r != expected: - roundtrip_fails += 1 - if roundtrip_fails <= 3: + stats.roundtrip_fails += 1 + if stats.roundtrip_fails <= 3: print(f"RT-FAIL {label} mt={mt} ot={ot}") print(f" exp_len={len(expected)} got_len={len(r)}") @@ -50,56 +66,82 @@ def check_case(text: str, mt: int, ot: int, label: str) -> None: body = c.partition(S)[2] if S in c else c est = chunker.estimate_tokens(body) if est > mt and body.strip(): - bounds_fails += 1 - if bounds_fails <= 3: + stats.bounds_fails += 1 + if stats.bounds_fails <= 3: print(f"BOUNDS-FAIL {label} mt={mt} ot={ot} est={est} body={body[:40]!r}") -# --- Case A: sentences with multi-space and tab separators ----------------- -for trial in range(300): - n = rng.randint(2, 30) - sents = [] - for _ in range(n): - w = rng.randint(2, 40) - s = " ".join(rng.choice(WORDS) for _ in range(w)) - sep = " " * rng.randint(1, 5) + "\t" * rng.randint(0, 2) - sents.append(s + "." + sep) - text = "".join(sents) - mt = rng.choice([4, 6, 8, 12, 16, 24, 32, 48]) - ot = rng.randint(0, mt - 1) - total += 1 - check_case(text, mt, ot, f"A trial={trial}") +def _case_a(rng: random.Random, stats: Stats) -> None: + """Sentences with multi-space and tab separators.""" + for trial in range(300): + n = rng.randint(2, 30) + sents = [] + for _ in range(n): + w = rng.randint(2, 40) + s = " ".join(rng.choice(WORDS) for _ in range(w)) + sep = " " * rng.randint(1, 5) + "\t" * rng.randint(0, 2) + sents.append(s + "." + sep) + text = "".join(sents) + mt = rng.choice([4, 6, 8, 12, 16, 24, 32, 48]) + ot = rng.randint(0, mt - 1) + stats.total += 1 + check_case(text, mt, ot, f"A trial={trial}", stats) -# --- Case B: single long no-internal-space word + trailing whitespace ------- -for trial in range(100): - text = "x" * rng.randint(20, 500) + " " * rng.randint(0, 10) - mt = rng.choice([4, 8, 12, 16, 24, 32]) - ot = rng.randint(0, mt - 1) - total += 1 - check_case(text, mt, ot, f"B trial={trial}") -# --- Case C: degenerate overlap (ot = mt - 1) ------------------------------- -for trial in range(100): - n = rng.randint(3, 20) - sents = [] - for _ in range(n): - w = rng.randint(2, 15) - s = " ".join(rng.choice(WORDS) for _ in range(w)) + "." - sents.append(s) - text = " ".join(sents) - mt = rng.choice([4, 8, 12, 16, 24]) - ot = mt - 1 - total += 1 - check_case(text, mt, ot, f"C trial={trial}") +def _case_b(rng: random.Random, stats: Stats) -> None: + """Single long no-internal-space word + trailing whitespace.""" + for trial in range(100): + text = "x" * rng.randint(20, 500) + " " * rng.randint(0, 10) + mt = rng.choice([4, 8, 12, 16, 24, 32]) + ot = rng.randint(0, mt - 1) + stats.total += 1 + check_case(text, mt, ot, f"B trial={trial}", stats) -# --- Case D: no-after-space sentences (punctuation runs) -------------------- -for trial in range(100): - text = "Hello.World!How.Are.You?I.Am.Fine. " * rng.randint(1, 20) - mt = rng.choice([6, 12, 16, 24, 32]) - ot = rng.randint(0, mt - 1) - total += 1 - check_case(text, mt, ot, f"D trial={trial}") -print(f"\ntotal={total} roundtrip_fails={roundtrip_fails} bounds_fails={bounds_fails}") -print("RESULT:", "ALL PASS" if roundtrip_fails == 0 and bounds_fails == 0 else "HAS FAILURES") -sys.exit(0 if roundtrip_fails == 0 and bounds_fails == 0 else 1) +def _case_c(rng: random.Random, stats: Stats) -> None: + """Degenerate overlap (ot = mt - 1).""" + for trial in range(100): + n = rng.randint(3, 20) + sents = [] + for _ in range(n): + w = rng.randint(2, 15) + s = " ".join(rng.choice(WORDS) for _ in range(w)) + "." + sents.append(s) + text = " ".join(sents) + mt = rng.choice([4, 8, 12, 16, 24]) + ot = mt - 1 + stats.total += 1 + check_case(text, mt, ot, f"C trial={trial}", stats) + + +def _case_d(rng: random.Random, stats: Stats) -> None: + """No-after-space sentences (punctuation runs).""" + for trial in range(100): + text = "Hello.World!How.Are.You?I.Am.Fine. " * rng.randint(1, 20) + mt = rng.choice([6, 12, 16, 24, 32]) + ot = rng.randint(0, mt - 1) + stats.total += 1 + check_case(text, mt, ot, f"D trial={trial}", stats) + + +def main() -> int: + """Run all edge cases through :func:`check_case` and report the verdict. + + ``rng`` is seeded exactly as the original module-level ``random.Random(99)`` + so the 600 generated cases reproduce the baseline sweep case-for-case. + """ + stats = Stats() + rng = random.Random(99) # same seed/sequence as the original module global + _case_a(rng, stats) + _case_b(rng, stats) + _case_c(rng, stats) + _case_d(rng, stats) + print(f"\ntotal={stats.total} roundtrip_fails={stats.roundtrip_fails} " + f"bounds_fails={stats.bounds_fails}") + ok = stats.roundtrip_fails == 0 and stats.bounds_fails == 0 + print("RESULT:", "ALL PASS" if ok else "HAS FAILURES") + return 0 if ok else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/wizard-work/forge/edge_sweep_baseline_output.txt b/wizard-work/forge/edge_sweep_baseline_output.txt new file mode 100644 index 0000000..4ab09b0 --- /dev/null +++ b/wizard-work/forge/edge_sweep_baseline_output.txt @@ -0,0 +1,3 @@ + +total=600 roundtrip_fails=0 bounds_fails=0 +RESULT: ALL PASS diff --git a/wizard-work/forge/edge_sweep_refactored_output.txt b/wizard-work/forge/edge_sweep_refactored_output.txt new file mode 100644 index 0000000..4ab09b0 --- /dev/null +++ b/wizard-work/forge/edge_sweep_refactored_output.txt @@ -0,0 +1,3 @@ + +total=600 roundtrip_fails=0 bounds_fails=0 +RESULT: ALL PASS