forge: served_drift_check summarise() DRY collapse 4 scans -> 1, is_fatal removed
This commit is contained in:
forge committed
1 parent
481052bffa
commit
dfd06b3c27
3 files changed
+367
-11
No files matched your search
+25
-11
@@ -77,6 +77,7 @@ import sys
|
||||
import urllib.error
|
||||
import urllib.parse
|
||||
import urllib.request
|
||||
from collections import Counter
|
||||
|
||||
FATAL = ("DIFFERS", "MISSING", "HTTP_ERROR", "UNREACHABLE", "READ_FAILED")
|
||||
ADVISORY = ("SKIPPED_LARGE",)
|
||||
@@ -189,15 +190,29 @@ def grade_one(root, rev, base, path, max_bytes, timeout):
|
||||
return row
|
||||
|
||||
|
||||
def is_fatal(row):
|
||||
return row["verdict"] in FATAL
|
||||
|
||||
|
||||
def summarise(rows):
|
||||
n_fatal = sum(1 for r in rows if is_fatal(r))
|
||||
n_ok = sum(1 for r in rows if r["verdict"] == "OK")
|
||||
n_skip = sum(1 for r in rows if r["verdict"] in ADVISORY)
|
||||
return n_ok, n_fatal, n_skip
|
||||
"""One pass over `rows`: tally verdicts and the uncommitted worktree count.
|
||||
|
||||
Replaces the original's four `sum(1 for r in rows ...)` scans -- three
|
||||
verdict passes across this function and the worktree pass in report() --
|
||||
with a single iteration. Counter buckets verdicts; the dirty count is
|
||||
incremented alongside, so each row is touched once, not four times.
|
||||
|
||||
n_fatal counts every row whose verdict is one of FATAL; n_ok is the OK
|
||||
tally; n_skip the ADVISORY tally; n_dirty the rows carrying an
|
||||
uncommitted worktree. Counter returns 0 for a verdict that never occurs,
|
||||
matching `sum(1 for ...)` over an empty set.
|
||||
"""
|
||||
counts = Counter()
|
||||
n_dirty = 0
|
||||
for r in rows:
|
||||
counts[r["verdict"]] += 1
|
||||
if r.get("worktree") == "modified":
|
||||
n_dirty += 1
|
||||
n_fatal = sum(counts[v] for v in FATAL)
|
||||
n_ok = counts["OK"]
|
||||
n_skip = sum(counts[v] for v in ADVISORY)
|
||||
return n_ok, n_fatal, n_skip, n_dirty
|
||||
|
||||
|
||||
def worktree_dirty(root, rev):
|
||||
@@ -241,8 +256,7 @@ def report(rows, out=sys.stdout, rev=None):
|
||||
width = max([len(r["path"]) for r in rows] + [4])
|
||||
for r in rows:
|
||||
out.write(" %-*s %-12s %s\n" % (width, r["path"], r["verdict"], r["detail"]))
|
||||
n_ok, n_fatal, n_skip = summarise(rows)
|
||||
n_dirty = sum(1 for r in rows if r.get("worktree") == "modified")
|
||||
n_ok, n_fatal, n_skip, n_dirty = summarise(rows)
|
||||
if n_dirty:
|
||||
out.write("\nnote: %d path(s) are uncommitted against %s. The serve root is the"
|
||||
" work tree, so a DIFFERS on those rows is an edit already on the host"
|
||||
@@ -492,7 +506,7 @@ def main(argv=None):
|
||||
json.dump({"root": os.path.abspath(a.root), "rev": a.rev,
|
||||
"base_url": a.base_url, "rows": rows}, fh, indent=1)
|
||||
if a.quiet:
|
||||
n_ok, n_fatal, n_skip = summarise(rows)
|
||||
n_ok, n_fatal, n_skip, _ = summarise(rows)
|
||||
print("served: %d files, %d OK, %d drift, %d skipped"
|
||||
% (len(rows), n_ok, n_fatal, n_skip))
|
||||
return 1 if n_fatal else 0
|
||||
|
||||
@@ -0,0 +1,109 @@
|
||||
# SERVED_DRIFT_SUMMARISE_DRY_REFACTOR_VERIFICATION
|
||||
|
||||
Tool: `tools/served_drift_check.py` (forge lane: DRY, behavior-preserving)
|
||||
Date: 2026-10-20
|
||||
Original: `git show HEAD:tools/served_drift_check.py` (volt, commit c56e3be)
|
||||
Refactored: working tree (`tools/served_drift_check.py`)
|
||||
|
||||
## The smell
|
||||
|
||||
`summarise()` tallied three verdict categories by making three full `sum(1 for r in rows if ...)`
|
||||
scans, and `report()` made a fourth scan for the uncommitted-worktree count:
|
||||
|
||||
```python
|
||||
def summarise(rows):
|
||||
n_fatal = sum(1 for r in rows if is_fatal(r)) # pass 1
|
||||
n_ok = sum(1 for r in rows if r["verdict"] == "OK") # pass 2
|
||||
n_skip = sum(1 for r in rows if r["verdict"] in ADVISORY) # pass 3
|
||||
return n_ok, n_fatal, n_skip
|
||||
|
||||
# inside report():
|
||||
n_dirty = sum(1 for r in rows if r.get("worktree") == "modified") # pass 4
|
||||
```
|
||||
|
||||
Four passes over the same list; `n_dirty` recomputes a count `summarise()`'s sibling loop already could produce. `is_fatal(row)` existed solely to feed pass 1, so it was a one-line helper whose only caller just vanished. Same class of defect the forge lane already retired in
|
||||
`doc_command_check.py` (report tally loop -> Counter) and `tool_map.py` (counting loops -> `_count_by_tool`).
|
||||
|
||||
## The refactor
|
||||
|
||||
One iteration, `collections.Counter` buckets verdicts, the dirty count is incremented alongside.
|
||||
`summarise()` now returns `(n_ok, n_fatal, n_skip, n_dirty)`; both call sites were updated:
|
||||
|
||||
```python
|
||||
def summarise(rows):
|
||||
counts = Counter()
|
||||
n_dirty = 0
|
||||
for r in rows:
|
||||
counts[r["verdict"]] += 1
|
||||
if r.get("worktree") == "modified":
|
||||
n_dirty += 1
|
||||
n_fatal = sum(counts[v] for v in FATAL)
|
||||
n_ok = counts["OK"]
|
||||
n_skip = sum(counts[v] for v in ADVISORY)
|
||||
return n_ok, n_fatal, n_skip, n_dirty
|
||||
```
|
||||
|
||||
`report()` unpacks the 4-tuple and dropped the redundant `n_dirty` scan; `main()`'s `--quiet`
|
||||
path unpacks `n_ok, n_fatal, n_skip, _`. `is_fatal` was deleted (grep confirms zero references repo-wide).
|
||||
`git diff` for the change: 2,879 bytes, four hunks (import, summarise, report call site, main call site).
|
||||
|
||||
### Equivalence argument
|
||||
|
||||
The counts are the only behavioral input to `report()` and `main() --quiet`, and their printed text
|
||||
is a pure function of those counts. `Counter` returns 0 for a verdict that never occurs, exactly
|
||||
matching `sum(1 for r in rows if r["verdict"] == "X")` over an empty set; `sum(counts[v] for v in FATAL)`
|
||||
is the same cardinality as `sum(1 for r in rows if r["verdict"] in FATAL)`. The values are therefore
|
||||
identical, so every consumer renders byte-identical output.
|
||||
|
||||
## Evidence (measured)
|
||||
|
||||
The proof lives in `wizard-work/forge/verify_served_drift_summarise_dry.py`. It materializes the
|
||||
HEAD blob as `served_drift_check.py` in a temp dir (so its `__file__`-reinvoking selftest cells
|
||||
run under their real name) and compares it against the working-tree refactored copy across four
|
||||
verdicts. Run output:
|
||||
|
||||
```
|
||||
[1] original selftest rc=0 sha=647d9ce436f6407f.. PASS
|
||||
[1] refactored selftest rc=0 sha=647d9ce436f6407f.. PASS
|
||||
[2] micro: tally old=(3, 5, 1) new=(3, 5, 1) PASS
|
||||
[2] micro: n_dirty new=2 expect=2 PASS
|
||||
[3] all-OK rc=0 bytes=120 PASS
|
||||
[3] DIFFERS (stale served) rc=1 bytes=194 PASS
|
||||
[3] SKIPPED_LARGE (--max-bytes) rc=0 bytes=115 PASS
|
||||
[4] no 3-tuple summarise unpack in report PASS
|
||||
[4] no 3-tuple summarise unpack in main PASS
|
||||
[4] no orphaned is_fatal definition PASS
|
||||
|
||||
VERDICT: ALL GREEN -- summarise() DRY collapse behavior preserved
|
||||
VERIFY_RC=0
|
||||
```
|
||||
|
||||
### Verdict breakdown
|
||||
|
||||
- **[1] SELFTEST DRIFT GUARD.** The original's 25-cell battery exercises every verdict
|
||||
(OK / DIFFERS / MISSING / HTTP_ERROR / UNREACHABLE / READ_FAILED / SKIPPED_LARGE) and the
|
||||
uncommitted-worktree note path (c21-c23). Both original and refactored `--selftest` exit 0 AND
|
||||
emit sha256 `647d9ce436f6407fd5dadc1f21e485fed6095e433e4808f04441073b4aafda84` -- byte-identical
|
||||
to the pre-refactor baseline captured from the immutable HEAD blob.
|
||||
|
||||
- **[2] MICRO EQUIVALENCE.** On a synthetic census covering all verdicts plus dirty and
|
||||
worktree-less rows, the old three-scan tally and the new single pass both yield `(n_ok=3,
|
||||
n_fatal=5, n_skip=1)`, and `n_dirty=2`. Old == new[:3], dirty matches.
|
||||
|
||||
- **[3] REPORT OUTPUT DRIFT.** End-to-end CLI (`python3 served_drift_check.py ...`) on the live
|
||||
tree, byte-identical stdout AND rc between original and refactored, across three regimes:
|
||||
all-OK (rc 0), a stale served copy producing DIFFERS (rc 1 -- the FAIL path that consumes
|
||||
n_fatal), and `--max-bytes` producing SKIPPED_LARGE (rc 0 -- the ADVISORY path that consumes
|
||||
n_skip). Each was PASS: stdout+rc identical between versions.
|
||||
|
||||
- **[4] CALL-SITE SURGERY.** No stale 3-tuple unpacking remains in `report()` or `main()`; no
|
||||
orphaned `is_fatal` definition remains. Lint (ruff/via patch) reported `ok`.
|
||||
|
||||
## Verdict
|
||||
|
||||
Behavior-preserving DRY consolidation: four redundant row-list scans collapsed to one, an orphaned
|
||||
helper removed, and both consumers proven byte-identical to the original across the selftest
|
||||
battery and three live verdict regimes.
|
||||
|
||||
VERDICT: PASS -- summarise() DRY collapse is behavior-preserving; selftest 25/25
|
||||
sha256-identical, live report rc+stdout identical across OK/DIFFERS/SKIPPED_LARGE.
|
||||
@@ -0,0 +1,233 @@
|
||||
#!/usr/bin/env python3
|
||||
"""verify_served_drift_summarise_dry.py -- equivalence proof for the DRY in
|
||||
served_drift_check.py's summarise().
|
||||
|
||||
The refactor collapsed four `sum(1 for r in rows ...)` scans over the same
|
||||
row list into ONE pass:
|
||||
|
||||
- three verdict tallies inside summarise() (n_fatal, n_ok, n_skip), and
|
||||
- the worktree "dirty" tally that report() recomputed with a fourth scan.
|
||||
|
||||
It also deleted `is_fatal(row)` -- the lone caller of which was the loop just
|
||||
removed -- leaving the FATAL verdict set checked inline via `counts[v] in FATAL`.
|
||||
|
||||
That is behavior-preserving iff every consumer of the counts renders byte-
|
||||
identical output, because the ONLY behavioral input is the count values and
|
||||
they are unchanged. Four verdicts, all must be green:
|
||||
|
||||
1. SELFTEST DRIFT GUARD -- the 25-cell selftest (which exercises OK,
|
||||
DIFFERS, MISSING, HTTP_ERROR, UNREACHABLE, READ_FAILED, SKIPPED_LARGE and
|
||||
the uncommitted-worktree note/c21-c23) exits 0 AND emits the exact same
|
||||
bytes on BOTH the original (HEAD blob, run under its real name) and the
|
||||
refactored (working tree), with sha256 == BASELINE_SELFTEST_SHA.
|
||||
|
||||
2. MICRO EQUIVALENCE -- on a synthetic census covering every verdict plus
|
||||
dirty and worktree-less rows, the old three-scan tally and the new single
|
||||
pass yield identical (n_ok, n_fatal, n_skip) and identical n_dirty.
|
||||
|
||||
3. REPORT OUTPUT DRIFT -- end-to-end CLI (python3 served_drift_check.py ...)
|
||||
on the LIVE tree, byte-identical stdout+rc between original and
|
||||
refactored, across three verdict regimes: all-OK, a stale served copy
|
||||
(DIFFERS -> the rc-1 FAIL path), and --max-bytes (SKIPPED_LARGE).
|
||||
|
||||
4. CALL-SITE SURGERY -- summarise() now returns a 4-tuple and both call
|
||||
sites (report + main --quiet) unpack it; grep confirms no 3-tuple
|
||||
unpack or orphaned is_fatal remains.
|
||||
|
||||
Exit code is the verdict: 0 = all green, 1 = any mismatch.
|
||||
"""
|
||||
import hashlib
|
||||
import importlib.util
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
|
||||
REPO = "/root/troupe-standup"
|
||||
DCC = os.path.join(REPO, "tools", "served_drift_check.py")
|
||||
|
||||
# Captured from the ORIGINAL (pre-refactor) source, i.e. `git show HEAD:tools/
|
||||
# served_drift_check.py`, run under its real name `served_drift_check.py`.
|
||||
# The selftest re-invokes itself by __file__, so the temp copy MUST be named
|
||||
# served_drift_check.py or the self-referential cells fail for a harness
|
||||
# reason, not a behavior one.
|
||||
BASELINE_SELFTEST_SHA = "647d9ce436f6407fd5dadc1f21e485fed6095e433e4808f04441073b4aafda84"
|
||||
|
||||
|
||||
def materialize_original():
|
||||
"""Write the HEAD blob under its real name in a temp dir; return its path."""
|
||||
src = subprocess.run(["git", "-C", REPO, "show", "HEAD:tools/served_drift_check.py"],
|
||||
capture_output=True, text=True).stdout
|
||||
if not src:
|
||||
raise SystemExit("[1] could not read HEAD:tools/served_drift_check.py -- FAIL")
|
||||
d = tempfile.mkdtemp(prefix="sdc_orig_")
|
||||
p = os.path.join(d, "served_drift_check.py")
|
||||
with open(p, "w", encoding="utf-8") as fh:
|
||||
fh.write(src)
|
||||
os.chmod(p, 0o755)
|
||||
return p
|
||||
|
||||
|
||||
def load_module(path, name):
|
||||
spec = importlib.util.spec_from_file_location(name, path)
|
||||
mod = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(mod)
|
||||
return mod
|
||||
|
||||
|
||||
def sha_of(text):
|
||||
return hashlib.sha256(text.encode()).hexdigest()
|
||||
|
||||
|
||||
def run_cli(script, args):
|
||||
p = subprocess.run([sys.executable, script, *args],
|
||||
capture_output=True, text=True)
|
||||
return p.returncode, p.stdout, p.stderr
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# old_summarise: the three-scan tally, verbatim from the pre-refactor source.
|
||||
# Returned as the 3-tuple the original produced; n_dirty is reconstructed by
|
||||
# report() in the old code, handled separately in the micro check.
|
||||
# ---------------------------------------------------------------------------
|
||||
FATAL = ("DIFFERS", "MISSING", "HTTP_ERROR", "UNREACHABLE", "READ_FAILED")
|
||||
ADVISORY = ("SKIPPED_LARGE",)
|
||||
|
||||
|
||||
def old_summarise(rows):
|
||||
n_fatal = sum(1 for r in rows if r["verdict"] in FATAL)
|
||||
n_ok = sum(1 for r in rows if r["verdict"] == "OK")
|
||||
n_skip = sum(1 for r in rows if r["verdict"] in ADVISORY)
|
||||
return n_ok, n_fatal, n_skip
|
||||
|
||||
|
||||
def synthetic_rows():
|
||||
"""One row per verdict class, plus dirty + worktree-less rows."""
|
||||
return [
|
||||
{"path": "a", "verdict": "OK", "worktree": "clean"},
|
||||
{"path": "b", "verdict": "OK", "worktree": "modified"}, # dirty
|
||||
{"path": "c", "verdict": "DIFFERS", "worktree": "clean"}, # fatal
|
||||
{"path": "d", "verdict": "MISSING", "worktree": "clean"}, # fatal
|
||||
{"path": "e", "verdict": "HTTP_ERROR", "worktree": "modified"}, # fatal+dirty
|
||||
{"path": "f", "verdict": "UNREACHABLE", "worktree": "clean"}, # fatal
|
||||
{"path": "g", "verdict": "READ_FAILED", "worktree": "clean"}, # fatal
|
||||
{"path": "h", "verdict": "SKIPPED_LARGE", "worktree": "clean"}, # advisory
|
||||
{"path": "i", "verdict": "OK"}, # no worktree key
|
||||
]
|
||||
|
||||
|
||||
def main():
|
||||
ok = True
|
||||
orig_script = materialize_original()
|
||||
try:
|
||||
tm_orig = load_module(orig_script, "sdc_orig")
|
||||
tm_ref = load_module(DCC, "sdc_ref") # working tree = refactored
|
||||
|
||||
# [1] selftest drift guard on both versions.
|
||||
for label, script in (("original", orig_script), ("refactored", DCC)):
|
||||
rc, out, err = run_cli(script, ["--selftest"])
|
||||
sha = sha_of(out)
|
||||
good = rc == 0 and sha == BASELINE_SELFTEST_SHA
|
||||
print("[1] %-11s selftest rc=%d sha=%s.. %s"
|
||||
% (label, rc, sha[:16], "PASS" if good else "FAIL"))
|
||||
if not good:
|
||||
ok = False
|
||||
print(" expected sha=%s" % BASELINE_SELFTEST_SHA[:16])
|
||||
if err:
|
||||
print(" stderr: " + err.rstrip()[:300])
|
||||
|
||||
# [2] micro equivalence on synthetic rows.
|
||||
rows = synthetic_rows()
|
||||
old = old_summarise(rows)
|
||||
new = tm_ref.summarise(rows) # 4-tuple
|
||||
counts_ok = old == tuple(new[:3])
|
||||
n_dirty = sum(1 for r in rows if r.get("worktree") == "modified")
|
||||
dirty_ok = new[3] == n_dirty
|
||||
print("[2] micro: tally old=%r new=%r %s"
|
||||
% (old, tuple(new[:3]), "PASS" if counts_ok else "FAIL"))
|
||||
print("[2] micro: n_dirty new=%d expect=%d %s"
|
||||
% (new[3], n_dirty, "PASS" if dirty_ok else "FAIL"))
|
||||
if not (counts_ok and dirty_ok):
|
||||
ok = False
|
||||
|
||||
# [3] end-to-end CLI drift on the LIVE tree across verdict regimes.
|
||||
base = "file://" + REPO
|
||||
cases = [
|
||||
("all-OK", ["--root", REPO, "--base-url", base,
|
||||
"--prefix", "tools/doc_command_check.py"]),
|
||||
("DIFFERS (stale served)", _stale_served_args(REPO)),
|
||||
("SKIPPED_LARGE (--max-bytes)", ["--root", REPO, "--base-url", base,
|
||||
"--prefix", "tools/doc_command_check.py",
|
||||
"--max-bytes", "5"]),
|
||||
]
|
||||
for label, args in cases:
|
||||
ro_rc, ro_out, _ = run_cli(orig_script, args)
|
||||
rr_rc, rr_out, _ = run_cli(DCC, args)
|
||||
good = ro_rc == rr_rc and ro_out == rr_out
|
||||
print("[3] %-30s rc=%d bytes=%d %s"
|
||||
% (label, rr_rc, len(rr_out), "PASS" if good else "FAIL"))
|
||||
if not good:
|
||||
ok = False
|
||||
print(" orig rc=%d sha=%s" % (ro_rc, sha_of(ro_out)[:16]))
|
||||
print(" ref rc=%d sha=%s" % (rr_rc, sha_of(rr_out)[:16]))
|
||||
_show_diff(ro_out, rr_out)
|
||||
|
||||
# [4] call-site surgery: contract invariants in the working tree.
|
||||
text = open(DCC).read()
|
||||
inv = [
|
||||
("no 3-tuple summarise unpack in report",
|
||||
" n_ok, n_fatal, n_skip = summarise(rows)\n if n_dirty" not in text),
|
||||
("no 3-tuple summarise unpack in main",
|
||||
not _three_tuple_main_unpack(text)),
|
||||
("no orphaned is_fatal definition", "def is_fatal" not in text),
|
||||
]
|
||||
for label, passed in inv:
|
||||
print("[4] %-36s %s" % (label, "PASS" if passed else "FAIL"))
|
||||
if not passed:
|
||||
ok = False
|
||||
|
||||
finally:
|
||||
shutil.rmtree(os.path.dirname(orig_script), ignore_errors=True)
|
||||
|
||||
print("\nVERDICT:",
|
||||
"ALL GREEN -- summarise() DRY collapse behavior preserved"
|
||||
if ok else "MISMATCH -- behavior NOT preserved")
|
||||
sys.exit(0 if ok else 1)
|
||||
|
||||
|
||||
def _three_tuple_main_unpack(text):
|
||||
# main()'s quiet path: the OLD code had this exact 2-line shape.
|
||||
return (" n_ok, n_fatal, n_skip = summarise(rows)\n"
|
||||
" print(\"served: %d files, %d OK, %d drift, %d skipped\"" in text)
|
||||
|
||||
|
||||
def _stale_served_args(repo):
|
||||
"""Build a served dir whose tools/doc_command_check.py differs from the
|
||||
tree's copy, so the run surfaces a DIFFERS verdict (rc 1 FAIL path)."""
|
||||
served = tempfile.mkdtemp(prefix="sdc_stale_")
|
||||
sub = os.path.join(served, "tools")
|
||||
os.makedirs(sub, exist_ok=True)
|
||||
# wrong content -> byte mismatch with the git blob -> DIFFERS
|
||||
with open(os.path.join(sub, "doc_command_check.py"), "w") as fh:
|
||||
fh.write("// stale served copy\n")
|
||||
return ["--root", repo, "--base-url", "file://" + served,
|
||||
"--prefix", "tools/doc_command_check.py"]
|
||||
|
||||
|
||||
def _show_diff(a, b):
|
||||
la, lb = a.splitlines(), b.splitlines()
|
||||
for i in range(max(len(la), len(lb))):
|
||||
ga = la[i] if i < len(la) else "<end>"
|
||||
gb = lb[i] if i < len(lb) else "<end>"
|
||||
if ga != gb:
|
||||
print(" live diff at line %d: orig=%r ref=%r" % (i + 1, ga, gb))
|
||||
break
|
||||
# clean the stale served dir
|
||||
if len(sys.argv) > 1 and sys.argv[0].endswith("verify_served_drift_summarise_dry.py"):
|
||||
pass
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
Reference in new issue
Block a user