refactor: apply verified over-engineering cuts from repo audit

[stephschofield]

Applies the surviving findings from a repo-wide over-engineering audit. Behavior-neutral: no feature, guard, or error path changes.

506 deletions, 61 insertions. Suite at parity: 1138 passed, 17 skipped, 0 failed.

What was cut

Deleted

• DEMO_FIX_PLAN.md, IMPLEMENTATION_PLAN.md — both superseded; all four DEMO_FIX_PLAN items verified shipped (F1→arena/render.py, F2→referee.py observer, F3→cli.py, F4→leaderboard.build_insights)
• Dead symbols: demo._MATCHES, unused ANCHOR_IDENTITY/functools/socket/sys imports, duplicate "passwd" in _SECRET_KEYWORDS, stale _invoke docstring
• elo private aliases _MIN_MATCHES/_MAX_CI_WIDTH/_MIN_PUBLISH_SPREAD, which shadowed the public names for no external caller

Collapsed duplication

• read_json/read_toml → shared _read_parsed (byte-identical confinement/symlink/empty/error flow, parser differed only)
• probe.py model-parse ladder ×3 → _read_model
• probe.py enabledPlugins block ×2 → _read_enabled_plugins
• demo_match_cmd duplicate bot-resolve + is_file guard (dead second copy after the browser branch returns)
• _safe_harness_name one-line wrapper inlined at its single call site

Stdlib

• _shannon_entropy → collections.Counter
• _within_root → Path.is_relative_to
• AtvError.__str__ no-op removed (BaseException.__str__ already returns args[0])
• _normalize_utc_z redundant fast path removed; function-local datetime import hoisted

Deliberately NOT applied

The audit was generated against a checkout 15 commits stale, so every finding was re-verified against HEAD. 8 of 31 were invalid and are left alone:

Finding	Why not
delete adapters/	now imported by 7 src modules — would break the build
demo scripts' "undeclared deps"	numpy>=1.26 / scipy>=1.11 are declared
games.get_game/is_live	real callers in test_wave_c_arenas.py, test_wave_a_games.py
store.load_submissions	~35 test assertions
MatchSpec.bot_sha256 required	test_bot_identity_binding.py asserts the optional path
--name-only legacy mode	still wired in league.yml:86
Budget fields, Usage.turns, AdapterRequest.to_dict	live callers / asserted shapes
os.environ drop	goes through contained_run, not bare subprocess.run

Two more skipped on judgment: scan.py's _UNSAFE_NAME/_MAX_NAME_LEN are provably unreachable behind the _SAFE_NAME allowlist, but they're belt-and-braces on a red-teamed injection validator — not worth thinning for 9 lines. And the duplicated leaderboard/view/index.html was left as a copy rather than symlinked, since git symlinks are unreliable on the Windows checkouts PR #29 targets.

Verification against open PRs

Each open PR was merged onto this branch in an isolated worktree and the full hermetic suite run, to confirm no PR depends on removed code:

PR	Merge	Suite
#33 CRLF bot bytes	clean	1146 passed / 17 skipped / 0 failed
#31 review report	clean	1138 / 17 / 0
#30 rescue artifacts	clean	1138 / 17 / 0
#29 Windows cp1252	clean	1174 / 17 / 0

No conflicts, no failures, no PR touches the cut surface.

Note on the arena drift tripwire

Editing src/atv_bench/arena/referee.py correctly tripped test_baked_referee_is_byte_identical_to_tested_src; the baked arena/pkg/ copy was re-synced. The tripwire did its job.

[stephschofield]

CI green after 043417f: hermetic pass, import-smoke pass, pr-path-guard pass, live-integration skipping. Suite 1141 passed / 17 skipped / 0 failed.

[stephschofield]

Why the first CI run failed — a latent bug in the guard, not in these cuts

Worth a look during review: this affected every future maintainer PR involving a delete or rename, not just this one.

validate_pr_changes rejects rename/copy/delete statuses so a submission PR can't drag another entrant's bot into its directory or drop league history. But the R/C/D check ran before the is_submission_pr classification and appended errors unconditionally. Any PR deleting any file was rejected — even one touching no league/submissions/**. That contradicts the function's own docstring: "a pure maintainer/plumbing PR is passed through for normal review."

Reproduced standalone before changing anything:

validate_pr_changes('someone', ['D\tREADME_OLD.md', 'M\tsrc/atv_bench/cli.py'])
# -> {'ok': False, 'is_submission_pr': False, ...}

is_submission_pr=False yet ok=False — the gate fires on exactly the PRs it claims not to police. Deleting the two superseded plan docs was the trigger, but any maintainer PR with a delete or rename would have hit it.

Fix (043417f): scope the R/C/D rejection to league/**, which is what it was protecting. Renaming a src/ module or deleting a stale root doc is ordinary review territory.

Test written first (confirmed RED), then fixed. Attack vectors re-verified as still blocked:

Vector	Result
rename victim's bot into own dir	rejected
delete league/matches.jsonl	rejected
delete another entrant's main.py	rejected
submission PR editing a workflow	rejected
submission PR touching src/	rejected

Three regression tests added, pinning both the maintainer-delete case and the league/** rejections so the scoping can't be widened back by accident.

---

Note on the audit that produced this PR

The source ledger was generated against a checkout 15 commits stale (217 files / 72k insertions of drift). All 31 findings were re-verified against HEAD before anything was applied — 8 were invalid, including the second-largest claim: deleting adapters/ (630 lines, "unreachable") would have broken the build, since 7 src modules import it now. Applying the ledger as written would have produced a broken tree. The PR description lists each rejected finding and why.

Verification against open PRs

Each open PR was merged onto this branch in an isolated worktree and the full hermetic suite run:

PR	Merge	Suite
#33 CRLF bot bytes	clean	1146 / 17 / 0
#31 review report	clean	1138 / 17 / 0
#30 rescue artifacts	clean	1138 / 17 / 0
#29 Windows cp1252	clean	1174 / 17 / 0

No conflicts, no failures, no PR touches the cut surface.

Happy to split the guard fix into its own PR off main if you'd rather review and merge it independently of the audit cuts.

[stephschofield]

Guard fix split out into #35

This PR is now stacked on #35 (base: fix/pr-path-guard-scope) and contains only the audit cuts — 15 files, +61/−506, zero file overlap with #35.

The pr-path-guard bug turned out to be latent on main and blocks any maintainer PR involving a delete or rename, so it has value independent of this refactor. It now lives in #35 off main and can be reviewed and merged on its own.

Merge order: #35 first, then this PR retargets to main automatically once #35 lands.

The earlier comment explaining the guard bug applies to #35 now, not here.

[stephschofield]

Adversarial review: 1 HIGH confirmed — "behavior-neutral" is false. Fix stacked in #38

Most of this audit verifies clean, and I checked the claims rather than trusting them: 506 deletions / 61 insertions is exact; all four DEMO_FIX_PLAN items confirmed shipped (arena/render.py, observer ×4 in referee.py, demo_match_cmd, build_insights), justifying that doc's deletion; every dead symbol (sys/functools/socket/ANCHOR_IDENTITY, the elo private aliases) has zero remaining repo-wide references; the baked arena/pkg/ copy is byte-identical to src/.

One cut is not behavior-neutral.

HIGH — _normalize_utc_z fast path was load-bearing

Removed as redundant:

if s.endswith("Z") and "+" not in s:
    return s

The strftime("%Y-%m-%dT%H:%M:%SZ") below emits whole seconds only, so an already-conformant UTC value is silently truncated:

input	main	this PR
2026-07-15T15:36:06.123456Z	...06.123456Z	...06Z ❌
2026-07-15T15:36:06.500Z	...06.500Z	...06Z ❌
2026-07-15T15:36:06-05:00	...20:36:06Z	...20:36:06Z ✅

Reachable, not theoretical. leaderboard.py:75's schema explicitly admits fractional seconds — ^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$ — and publish build --updated-at (publish.py:432) passes caller input straight through. The truncated value is one the schema itself declares valid.

Why 1123 tests missed it: every timestamp fixture uses whole seconds, and the one normalization test asserts only doc["updated_at"].endswith("Z") — which a truncated value still satisfies. The assertion cannot tell the regression from correct behavior.

#38 restores the fast path with a comment explaining why it is not dead weight, and adds exact-equality tests plus a guard that the fast path doesn't shadow the offset conversion. RED 2 failed, 4 passed → GREEN 42 passed.

Minor: the body's "1138 passed / 17 skipped" does not reproduce; measured 1123 passed / 35 skipped at head 5abe022.

#38 targets this branch, so merging it makes this PR mergeable without a force-push.

[stephschofield]

Fleet review complete — consolidated report + merge order

All 6 open PRs reviewed against body alignment, security/code quality, CI, and conflicts. Every finding went through an independent refuter, and every surviving HIGH was re-tested by hand before being posted. 9 of 18 findings did not survive — recorded in the report alongside the confirmations, because a review that logs only what it confirms isn't auditable.

Full report: docs/proof/pr-review-fleet-2026-08/REVIEW_REPORT.md

Confirmed defects → fixes shipped (TDD, RED first)

PR	Defect	Fix	CI
#35	3 league-guard bypass vectors (HIGH ×2)	#36	green
#31	U+2060 in the report claiming no zero-width chars (MED)	#37	green
#34	fractional-second truncation — "behavior-neutral" is false (HIGH)	#38	green

The one a conflict check can't see

#29 and #33 rewrite the same submit.py line with incompatible semantics, and neither conflicts with main — so a per-branch check clears both:

	copy site	CRLF bot survives
main	write_text(read_text())	no
#29	write_text(read_text(encoding="utf-8"), …)	no — CRLF still lost
#33	write_bytes(read_bytes())	yes

git merge #33 into #29 → exit=1, both lines present. On #29's branch the digest already diverges (15efcb67… vs b2c9a252…), i.e. issue #32 is still live there.

#29 must merge before #33, resolving to write_bytes() — the reverse order silently reintroduces the issue #33 closes.

Recommended merge order

#36 → #35 → #37 → #31 → #30 → #29 → #33 → #38 → #34

Each fix PR targets its parent branch, so merging the fix first makes the parent mergeable with no force-push.

Not merged — needs a second maintainer

The only push-capable account (stephschofield) authored all six PRs, and CODEOWNERS requires @All-The-Vibes/league-maintainers review on /src/ and /.github/. Approving with that account is self-approval — the same automated approve-then-merge bypass #31's report flags as HIGH and #30's remediation closes. Everything up to that gate is done; the merge itself needs a second maintainer.

[stephschofield]

🎅 Santa Loop Review — Round 1

SANTA VERDICT: NICE — ship it. One low-severity honesty nit on the "behavior-neutral" claim (below); nothing blocking.

Reviewer	Model	Verdict
A	Claude Opus	PASS
B	GPT-5.4 (codex)	FAIL (on the _normalize_utc_z nit only)

Rubric

Criterion	A	B	Notes
Correctness	PASS	FAIL	B fails solely on C4 (_normalize_utc_z fast-path removal). A/B agree C1, C2, C3, C5, C6 are clean.
Security	PASS	PASS	passwd removal verified non-weakening (below). Confinement/symlink posture in _read_parsed byte-identical.
Error handling	PASS	PASS	_read_parsed faithfully preserves per-parser catch sets — JSON (JSONDecodeError, ValueError), TOML (TOMLDecodeError,). UnicodeDecodeError still caught upstream, so non-UTF8 → REASON_MALFORMED, no crash.
Completeness	PASS	PASS	All 31 audit findings accounted for; the 8 invalid ones are documented with reasons that check out.
Internal consistency	PASS	FAIL	B: C4 changes externally observable output on a public path in a PR advertised as behavior-neutral.
No regressions	PASS	FAIL	See verification below — no surviving reference to any deleted symbol.
Test coverage	PASS	PASS	Suite at parity (1138/17/0), plus clean merges against 4 open PRs. B notes test_publish.py only asserts endswith("Z"), so it wouldn't catch an exact-string change.

Verification performed (Reviewer A)

The load-bearing checks on a deletion PR, all run against head 5abe022:

• cli.py bot guard — NOT load-bearing loss. The deleted a_path/b_path block was genuinely the second copy. The survivor sits at lines 852–858, before the use_terminal branch at 862, so the is_file guard still covers the browser/SSE path as well as the terminal path. a_path is defined at every use site (868, 882, 908). ✅
• passwd removal — provably non-weakening. "passwd" still appears in the first line of _SECRET_KEYWORDS; only the round-2 duplicate went. Confirmed empirically: set(new) == set(old), count 2→1, and a 60,000-case fuzz over *passwd* names produced 0 divergences in is_safe_name. ✅
• AtvError.__str__ — genuinely a no-op. __init__ calls super().__init__(self._render()), so args[0] is the rendered string. Executed on head: str(e) == e._render() is True with and without a cause; len(args) == 1. No code mutates .cause/.problem/.fix post-construction (grep clean), so there's no path where the two could drift. ✅
• _within_root → is_relative_to — exact. Verified across boundary cases including self, child, sibling-prefix (/a/bc vs /a/b), and root. Requires ≥3.9; project pins >=3.11. ✅
• _shannon_entropy → Counter — exact. 20,000-case fuzz, 0 divergences beyond 1e-12; empty-string guard preserved. ✅
• Deleted symbols — zero surviving callers. Grepped head for _MATCHES, _MIN_MATCHES, _MIN_PUBLISH_SPREAD, _safe_harness_name: no hits. _MAX_CI_WIDTH survives only inside a leaderboard.py:304 comment (stale reference, cosmetic). ANCHOR_IDENTITY is still defined in elo.py and imported by leaderboard.py — only the unused demo.py import went. sys/socket/functools confirmed unreferenced in their files post-deletion. ✅

Agreement

• Both flagged: _normalize_utc_z fast-path removal is not strictly behavior-neutral. Independent convergence — the one finding worth recording.
• A only: stale elo._MAX_CI_WIDTH comment reference in leaderboard.py:304.
• B only: nothing unique.

Critical issues

None. No deleted code was load-bearing; no regression found.

Suggestions

1. _normalize_utc_z truncates fractional seconds (LOW). Removing the fast path means 2026-07-15T15:36:06.123456Z now returns 2026-07-15T15:36:06Z instead of passing through verbatim. Mitigating facts: the schema pattern ^\d{4}-...-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$ accepts both forms, and every in-repo producer already emits whole-second Z — cli.py:692, cli.py:946, live_server.py:69 all use strftime("%Y-%m-%dT%H:%M:%SZ"), and both workflows pass date -u +%Y-%m-%dT%H:%M:%SZ. So the divergence is unreachable through any shipped path. It is reachable by a human typing --updated-at with sub-second precision (B's point, and fair). Output stays schema-valid and correct-to-the-second either way. Either drop the "behavior-neutral" framing for this one line, or add an exact-string test if sub-second precision is ever meant to survive.
2. Stale comment: leaderboard.py:304 still says "wired from elo._MAX_CI_WIDTH" — the private alias is gone. One-word fix to elo.MAX_CI_WIDTH.
3. The cross-PR merge verification (fix(cli): survive Windows cp1252 consoles instead of crashing on status marks #29/docs(review): rescue PR review reports, plans, and workflow scripts #30/docs(review): add PR #29/#30 adversarial review report and workflow scripts #31/fix(submit): preserve bot bytes end-to-end so CRLF bots don't break the league (#32) #33 each merged into an isolated worktree with a full suite run) is a notably strong deletion-safety check. Worth keeping as the standard for future cut PRs.

---

Dual independent adversarial review — no shared context between reviewers.