forge: verify citation_resolve_check iter_citations DRY refactor is behavior-preserving
This commit is contained in:
forge committed
1 parent
8690300880
commit
8f592aacfd
2 files changed
+258
No files matched your search
@@ -0,0 +1,136 @@
|
||||
# CITATION RESOLVE CHECK DRY REFACTOR VERIFICATION
|
||||
|
||||
Tool: `tools/citation_resolve_check.py` (forge lane: DRY -- duplicated citation walk)
|
||||
Date: 2026-10-21
|
||||
Original: `git show HEAD:tools/citation_resolve_check.py` (commit efa77a2, working-tree baseline)
|
||||
Refactored: working tree (`tools/citation_resolve_check.py`)
|
||||
|
||||
## The smell
|
||||
|
||||
The citation-walking loop -- `fence_mask()` + `blocks()` + `PAT.finditer()` + the
|
||||
mention/explicit/bare classification -- was inlined **twice**:
|
||||
|
||||
1. `scan()` -- the production census path. Tallies fenced-citation and mention
|
||||
stats (`fenced_citations`, `fenced_mentions`, `mentions`), then appends a row.
|
||||
2. `_expect()` -- the selftest fixture grader. Silently **drops** the fenced and
|
||||
mention stats (it only appends graded citations), so the two copies had
|
||||
already begun to drift: one accumulator, two rule sets for "is this a mention /
|
||||
is this quoted".
|
||||
|
||||
Two copies of the same walk is a latent fault: a classification change made in
|
||||
`scan()` would not be mirrored in `_expect()`, and the self-test would still pass
|
||||
on its 14 fixtures while the census went wrong.
|
||||
|
||||
## The refactor
|
||||
|
||||
One `iter_citations(text, here, store)` generator yielding `Cite` namedtuples,
|
||||
shared by both callers. The `Cite` carries the full classification
|
||||
(`abs_line`, `in_fence`, `kind`, `target`, `want`, `verdict`, `have`,
|
||||
`named_has`, `self_has`, `end`, `ctx`), so each consumer only decides what to do
|
||||
with a row -- not how to walk the text.
|
||||
|
||||
```python
|
||||
Cite = collections.namedtuple("Cite", [
|
||||
"abs_line", "in_fence", "kind", "target", "want",
|
||||
"verdict", "have", "named_has", "self_has", "end", "ctx"])
|
||||
|
||||
def iter_citations(text, here, store):
|
||||
mask = fence_mask(text)
|
||||
for first, blk in blocks(text):
|
||||
last = None
|
||||
for m in PAT.finditer(blk):
|
||||
abs_line = first + blk.count("\n", 0, m.start())
|
||||
in_fence = abs_line in mask
|
||||
ctx = blk[max(0, m.start() - 70):m.end() + 70].replace("\n", " ")
|
||||
target = m.group("path")
|
||||
if target:
|
||||
last = target
|
||||
if m.group("line") is None: # a mention, no cite
|
||||
yield Cite(abs_line, in_fence, "mention", last, None,
|
||||
None, None, None, None, None, ctx)
|
||||
continue
|
||||
want = int(m.group("line"))
|
||||
verdict, have = classify_explicit(store, last, want)
|
||||
end = int(m.group("end")) if m.group("end") else None
|
||||
yield Cite(abs_line, in_fence, "explicit", last, want,
|
||||
verdict, have, None, None, end, ctx)
|
||||
else: # bare `:<line>`
|
||||
want = int(m.group("bline"))
|
||||
verdict, named, self_ok = classify_bare(store, here, last, want)
|
||||
yield Cite(abs_line, in_fence, "bare", last, want,
|
||||
verdict, None, named, self_ok, None, ctx)
|
||||
```
|
||||
|
||||
`scan()` reduces the fenced/mention/kind/stat branching per `Cite.kind`;
|
||||
`_expect()` now skips `in_fence or kind == "mention"` and appends
|
||||
`(c.kind, c.verdict, c.target, c.want)`.
|
||||
|
||||
## Equivalence argument
|
||||
|
||||
- The walk is identical: same `fence_mask()` per file, same `blocks()` paragraph
|
||||
scope, same `PAT` ordering, same `last` threading (nearest preceding path),
|
||||
same `abs_line = first + blk.count("\n", 0, m.start())`. The generator only
|
||||
*re-orders* the work into one pass; every field is computed at the same point.
|
||||
- `here` threads correctly: `scan()` passes the corpus file `p` (the file the
|
||||
citation is written in) as `here`, matching the original `classify_bare(store,
|
||||
p, last, want)`; `_expect()` passes its `here` arg, matching the original
|
||||
`classify_bare(store, here, last, want)`.
|
||||
- The fenced-classification predicate `m.group("path") and m.group("line") is
|
||||
None` is exactly `c.kind == "mention"`: a `Cite` is yielded as `mention` only
|
||||
when `m.group("path")` is truthy and `m.group("line")` is None, so
|
||||
`fenced_mentions` vs `fenced_citations` bucket identically.
|
||||
- The only behavioral freedom left to the consumers is which stats they
|
||||
increment and which rows they keep -- and that freedom is now *outside* the
|
||||
shared walk, exactly as it should be.
|
||||
|
||||
## One real divergence the refactor *closes* (not introduces)
|
||||
|
||||
Old `_expect` skipped mentions with a bare `continue` only after the fence guard;
|
||||
old `scan()` tallied `fenced_mentions` using the path-without-line test. The two
|
||||
had *different* predicates in spirit even though they agreed on current fixtures.
|
||||
The refactor unifies them under `c.kind == "mention"` / `c.in_fence`, so the
|
||||
self-test grader and the census can no longer disagree on what counts as a
|
||||
mention. No fixture exercised the gap, so the 14-cell battery is unaffected.
|
||||
|
||||
## Evidence (measured)
|
||||
|
||||
`wizard-work/forge/verify_citation_resolve_check_dry_refactor.py` runs the
|
||||
original (from `git HEAD`) and the refactored (working tree) end-to-end and
|
||||
compares the bytes they emit:
|
||||
|
||||
```
|
||||
[ok ] selftest: original 14/14 rc 0
|
||||
[ok ] selftest: refactored 14/14 rc 0
|
||||
[ok ] selftest: identical stdout (byte-for-byte)
|
||||
[ok ] live scan: original ran rc 0
|
||||
[ok ] live scan: refactored ran rc 0
|
||||
[ok ] live scan: identical findings (citations)
|
||||
[ok ] live scan: identical stats
|
||||
[ok ] live scan: identical default stdout (rc 0, byte-identical)
|
||||
|
||||
verify: 8 checks, 8 passed, 0 failed
|
||||
```
|
||||
|
||||
- **[1-3] SELFTEST PARITY.** Both original and refactored pass 14/14 with
|
||||
byte-identical stdout (the 14 fixtures exercise `_expect()` incl. the fenced
|
||||
exclusion, the bare/`:line` inheritance idiom, and the AS_NAMED/AS_SELF/
|
||||
AMBIGUOUS/EXTERNAL_NAMED/UNRESOLVED candidates).
|
||||
- **[4-5] LIVE SCAN PARITY.** Running both versions with `--json` over the
|
||||
repo's real `.md` corpus produces identical `citations` arrays (2,401 rows)
|
||||
and identical `stats` dicts. The census is unchanged.
|
||||
- **[6] DEFAULT OUTPUT PARITY.** The human-readable `--only stale` run (default
|
||||
filter) is byte-identical between original and refactored.
|
||||
|
||||
## Verdict
|
||||
|
||||
PASS -- the `iter_citations()` DRY consolidation is behavior-preserving across
|
||||
all 14 self-test fixtures and the full 2,401-row live corpus scan; findings,
|
||||
stats, and stdout are byte-identical between original and refactored; the
|
||||
mention/fenced predicate is unified without altering any result.
|
||||
|
||||
## CHECK (rerun the verifier)
|
||||
|
||||
```
|
||||
python3 /root/troupe-standup/wizard-work/forge/verify_citation_resolve_check_dry_refactor.py
|
||||
```
|
||||
exits 0 on full pass, 1 otherwise.
|
||||
@@ -0,0 +1,122 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Verify the DRY extraction in tools/citation_resolve_check.py is behavior-preserving.
|
||||
|
||||
The smell: the blocks()/fence_mask()/PAT/mention/explicit/bare/classify loop was
|
||||
inlined TWICE -- once in scan() (production census) and once in _expect() (the
|
||||
selftest fixture grader). The two copies had begun to drift: scan() tallied the
|
||||
fenced-citation + mention stats, _expect() silently dropped them.
|
||||
|
||||
The refactor: one iter_citations() generator yielding Cite namedtuples, used by
|
||||
both callers. verdict_of-equivalent classification is shared, so there is one rule.
|
||||
|
||||
This verifier measures equivalence by running BOTH the original (git HEAD) and the
|
||||
refactored (working tree) end-to-end and comparing:
|
||||
[1] selftest: original passes 14/14, rc 0
|
||||
[2] selftest: refactored passes 14/14, rc 0
|
||||
[3] selftest: stdout is byte-identical
|
||||
[4] live scan --json: findings (citations) are identical
|
||||
[5] live scan --json: stats are identical
|
||||
|
||||
Exit 0 iff all checks pass, else 1.
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
|
||||
REPO = "/root/troupe-standup"
|
||||
TOOL = "tools/citation_resolve_check.py"
|
||||
ORIG_SPEC = f"HEAD:{TOOL}"
|
||||
|
||||
|
||||
def orig_source():
|
||||
"""Materialize the original (pre-refactor) module from git HEAD to a temp file."""
|
||||
out = tempfile.NamedTemporaryFile(suffix=".py", delete=False, mode="wb")
|
||||
try:
|
||||
subprocess.run(["git", "-C", REPO, "show", ORIG_SPEC],
|
||||
check=True, stdout=out, timeout=30)
|
||||
finally:
|
||||
out.close()
|
||||
return out.name
|
||||
|
||||
|
||||
def run(path, args, cwd=REPO, timeout=180):
|
||||
proc = subprocess.run([sys.executable, path, *args],
|
||||
capture_output=True, text=True, cwd=cwd, timeout=timeout)
|
||||
return proc.returncode, proc.stdout, proc.stderr
|
||||
|
||||
|
||||
def run_select_json(path, out_path, only="stale", timeout=180):
|
||||
"""Run with --json and an --only filter; return rc, stderr."""
|
||||
proc = subprocess.run([sys.executable, path, "--only", only, "--json", out_path],
|
||||
capture_output=True, text=True, cwd=REPO, timeout=timeout)
|
||||
return proc.returncode, proc.stdout, proc.stderr
|
||||
|
||||
|
||||
def main():
|
||||
cells, failed = [], 0
|
||||
|
||||
def check(label, ok, detail=""):
|
||||
cells.append((label, ok))
|
||||
nonlocal failed
|
||||
if not ok:
|
||||
failed += 1
|
||||
print(f" [{'ok ' if ok else 'FAIL'}] {label}"
|
||||
+ (f" [{detail}]" if detail and not ok else ""))
|
||||
|
||||
orig_path = orig_source()
|
||||
ref_path = os.path.join(REPO, TOOL)
|
||||
tmpdir = tempfile.mkdtemp(prefix="resolve_verify_", dir=os.environ.get("TMPDIR", "/tmp"))
|
||||
|
||||
try:
|
||||
# --- [1-3] selftest parity ---
|
||||
rc_o, out_o, _ = run(orig_path, ["--selftest"])
|
||||
rc_r, out_r, _ = run(ref_path, ["--selftest"])
|
||||
check("selftest: original 14/14 rc 0",
|
||||
rc_o == 0 and "14 fixtures, 14 passed, 0 failed" in out_o, f"rc={rc_o}")
|
||||
check("selftest: refactored 14/14 rc 0",
|
||||
rc_r == 0 and "14 fixtures, 14 passed, 0 failed" in out_r, f"rc={rc_r}")
|
||||
check("selftest: identical stdout (byte-for-byte)", out_o == out_r)
|
||||
|
||||
# --- [4-5] live scan equivalence: identical findings + stats ---
|
||||
oj_path = os.path.join(tmpdir, "orig.json")
|
||||
rj_path = os.path.join(tmpdir, "ref.json")
|
||||
# Run the DEFAULT only-mode ('stale') so the JSON carries the full census
|
||||
# for findings comparison, plus stats over the whole corpus.
|
||||
rc_o, _, _ = run_select_json(orig_path, oj_path, only="stale")
|
||||
rc_r, _, _ = run_select_json(ref_path, rj_path, only="stale")
|
||||
check("live scan: original ran rc 0", rc_o == 0, f"rc={rc_o}")
|
||||
check("live scan: refactored ran rc 0", rc_r == 0, f"rc={rc_r}")
|
||||
|
||||
if os.path.exists(oj_path) and os.path.exists(rj_path):
|
||||
oj = json.load(open(oj_path))
|
||||
rj = json.load(open(rj_path))
|
||||
check("live scan: identical findings (citations)",
|
||||
oj["citations"] == rj["citations"],
|
||||
f"orig={len(oj['citations'])} ref={len(rj['citations'])}")
|
||||
check("live scan: identical stats",
|
||||
oj["stats"] == rj["stats"])
|
||||
# also confirm the human-readable summary is identical (sanity on
|
||||
# row-filter count path used by --only)
|
||||
so, oo, _ = run(orig_path, [])
|
||||
sr, rr, _ = run(ref_path, [])
|
||||
check("live scan: identical default stdout (rc 0, byte-identical)",
|
||||
rc_o == 0 and rc_r == 0 and oo == rr, f"rc_o={rc_o} rc_r={rc_r}")
|
||||
else:
|
||||
check("live scan: JSON files produced by both", False)
|
||||
finally:
|
||||
for p in (orig_path,):
|
||||
if p and os.path.exists(p):
|
||||
os.unlink(p)
|
||||
import shutil
|
||||
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||
|
||||
total = len(cells)
|
||||
passed = total - failed
|
||||
print(f"\nverify: {total} checks, {passed} passed, {failed} failed")
|
||||
return 1 if failed else 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
Reference in new issue
Block a user