feat: rune review v0.2 — conflict + dead-rule detection with TUI

[codex-devlab]

Summary

Adds rune review — a third top-level verb that detects rule conflicts and dead rules across CLAUDE.md/AGENTS.md/etc., presents them in a Textual TUI, and applies fixes with a SHA-256-safe backup pipeline. Plus rune patch apply/verify for reproducible site-packages deployment.

26 commits across 5 phases. Designed via RALPLAN consensus (Architect SOUND + Critic APPROVE in 2 iterations); see spec at ~/ToolSet/rune/spec-2026-06-11.md and plan at ~/ToolSet/rune/plan-2026-06-11.md.

What's new

Subsystem	Module(s)	Notes
Patch journal	rune/patches/manifest.py, rune/cli/patch.py	TOML manifest w/ SHA-256 pre/post hashes; verify/apply subcommands; apply handles new-file case (empty pre_sha256); ≤500ms verify on 20 entries
L1 lexical conflict	rune/review/conflict_lexical.py	modal+verb+object triple extraction; P=1.00 ∧ R=1.00 on 20+20 labeled fixture (≥0.90/≥0.70 required)
Stage A static dead-rule	rune/review/dead_static.py	trigger keyword → repo file-extension match w/ rglob fallback
L2 NLI conflict	rune/review/conflict_nli.py	cross-encoder/nli-deberta-v3-base default (MIT) or nli-distilroberta-base (Apache-2.0); fail-closed (--allow-model-download required); P≥0.85 ∧ R≥0.75 budget
Stage B event dead-rule	rune/review/dead_events.py	events.jsonl parse; --events-window-days flag (stub for v0.3 windowing)
Textual TUI	rune/review/tui.py	14 keybinds (j/k/space/a/p/d/c/o/r//// /?/g,g/G/q); multi-select state; mid-session mtime staleness banner; cold start ≤800ms budget
Apply pipeline	rune/review/applier.py	per-chunk SHA-256 staleness refusal; .rune/backups/<ISO>/ timestamped snapshots; --apply --yes / --restore / --keep-backups 10 / --no-prune; reverse-line-order edits
Deployment helper	scripts/build_manifest.py	generates manifest from dev clone for any target-root
Schema + docs	rune/review/{schema.json, SCHEMA_POLICY.md, L1_LIMITS.md, BENCHMARKS.md}	JSON Schema draft-07 v1.0/1.1 (additive-only); L1 failure-mode matrix; reference hardware

Lazy import boundary

rune review --json (L1 path) does NOT import textual, torch, transformers, or sentence_transformers — verified via sys.modules snapshot tests. analyze/optimize startup unaffected.

Test plan

• All 36 new tests + 82 baseline tests pass: 118 passing, 2 skipped
• Skipped tests are NLI integration (P/R + latency), gated behind RUNE_RUN_NLI=1 + cached model — opt-in
• L1 P/R achieved 1.00/1.00 on hand-curated fixture (20 positives covering 9+ verbs × polarity, 20 negatives including the 5 L1_LIMITS modes)
• rune patch apply end-to-end verified against temp target dir: 18 entries, post-apply verify reports all OK
• rune review --apply --yes round-trip: apply → restore → byte-identical to pre-apply
• 11 consecutive apply sessions → 10 snapshots remain (retention enforcement)
• Site-packages NOT touched in this branch — deployment is opt-in via scripts/build_manifest.py --target-root <dest> + rune patch apply
• NLI integration tests (run locally with huggingface-cli download cross-encoder/nli-deberta-v3-base && RUNE_RUN_NLI=1 pytest tests/test_l2_*.py)
• Manual TUI smoke (rune review /path/with/CLAUDE.md — should launch Textual app, q to exit cleanly)

Open questions deferred to v0.3+

• Widen L2 candidate filter beyond keyword overlap (currently re-uses L1 conflict pairs as candidates — honest naming "NLI verification of lexically-adjacent rule pairs", not broad semantic conflict)
• Concurrent --apply lockfile
• rune review --watch mode
• events_window_days actual windowing (currently accepted, not yet applied to event filtering)

🤖 Generated with Claude Code

[codex-devlab]

Code Review — feat/review-v0.2

Assessment: NEEDS_CHANGES — substantive correctness issues in the apply pipeline (loader/applier SHA mismatch, backup path flattening, headless heuristic deletes content) plus a TUI promise that isn't actually rendered. Important tier is mostly cleanup but a few items (payload SHA verify, manifest TARGETS drift) should land before this PR goes near a real site-packages target.

26 commits / 81 files / +1553 / −0. Spec at ~/ToolSet/rune/spec-2026-06-11.md, plan at ~/ToolSet/rune/plan-2026-06-11.md (consensus-approved via RALPLAN).

---

Strengths

1. Clean lazy-loading discipline — rune/review/cli.py:48,54,79,95 defers heavy imports (dead_events, conflict_nli, applier, tui) inside their conditional branches. tests/test_import_hygiene.py enforces this via sys.modules snapshots after --json invoke. Right pattern, well tested.

2. Fail-closed L2 design — rune/review/conflict_nli.py:23-27 refuses to run without cached model + --allow-model-download, with actionable stderr pointing at huggingface-cli download. tests/test_l2_fail_closed.py exercises with HF_HUB_OFFLINE=1.

3. Per-chunk SHA staleness gate — rune/review/applier.py:43-47 verifies all ops first before any mutation. Combined with reverse-order application this is the right shape for a multi-op editor (modulo C1).

4. Manifest hardening — rune/patches/manifest.py:21-31 wraps tomllib.load in try/except and validates required fields. tests/test_patch_manifest.py covers malformed-TOML + missing-field.

5. Additive-only schema policy — rune/review/SCHEMA_POLICY.md plus real JSON Schema at rune/review/schema.json with tests/test_review_json_schema.py validating live output. Real contract.

---

Critical (must fix before merge)

C1. Loader SHA and applier SHA disagree — every real apply will raise StaleChunkError

rune/review/loader.py:12 vs rune/review/applier.py:44-45

• Loader: sha = hashlib.sha256(c.text.encode("utf-8")).hexdigest() — hashes Chunk.text as inventory produces it (trailing \n preserved).
• Applier: current = _read_chunk(...).rstrip("\n"); current_sha = hashlib.sha256(current.encode("utf-8")).hexdigest() — strips trailing \n before hashing.

These hash different byte sequences whenever the chunk ends in \n (common case). Result: in normal use, --apply --yes on a freshly-scanned unmutated repo raises StaleChunkError.

tests/test_apply_staleness.py only passes because it constructs ChunkRef.text = "Always use TS." (no trailing newline) by hand — it does NOT route through load_chunk_refs. No end-to-end test loads + applies. tests/test_restore_roundtrip.py does subprocess rune review --apply --yes but passes only if the inventory adapter happens to strip newlines from chunk text.

Fix: centralize as _sha_chunk(text) in a shared helper; pick one canonical form (recommend text.rstrip("\n")). Add an integration test: load_chunk_refs → apply_operations(kind="keep") on a real CLAUDE.md, assert no StaleChunkError.

C2. NLI label-index assumption is brittle across models

rune/review/conflict_nli.py:32-35

scores = model.predict([(ref_a.text, ref_b.text), (ref_b.text, ref_a.text)])
contradiction_score = max(scores[0][0], scores[1][0])

For cross-encoder/nli-deberta-v3-base (default), label order is [contradiction, entailment, neutral], so [0] IS contradiction. Correct today.

For --nli-small → cross-encoder/nli-distilroberta-base, the label order is not enforced across checkpoint revisions. The inline comment literally admits "MNLI label order is typically [contradiction, entailment, neutral]". The code blindly indexes [0] for both.

Fix: read the mapping from the model:

id2label = model.config.id2label
contradiction_idx = next(i for i, lbl in id2label.items() if "contradict" in lbl.lower())
contradiction_score = max(scores[0][contradiction_idx], scores[1][contradiction_idx])

Add a unit test (NLI-gated) that asserts the label resolution works for both models.

C3. --restore flattens paths — silent data loss on multi-file repos

rune/review/applier.py:24-29 and rune/review/cli.py:29-40

Backup: _backup_file does rel = file.name; dst = backup_root / rel — flattens to basename. Two CLAUDE.md files at different paths in the same apply session overwrite each other in the snapshot.

Restore: target = path / f.name for f in snap.iterdir() — restores by basename to repo root. A subdir/CLAUDE.md is silently moved to repo_root/CLAUDE.md on restore.

Fix: backup by relative path. Either pass repo_root into apply_operations (store at snapshot_root / file.relative_to(repo_root)) OR iterate operation_log.json on restore and use the recorded op["file"] path.

C4. --apply --yes deletes content via headless heuristic with no spec coverage

rune/review/cli.py:80-86

ops: list = []
for c in conflicts:
    ops.append(Operation(kind="delete", ref=c.b))
for d in dead:
    ops.append(Operation(kind="delete", ref=d.chunk))

The "delete b-side of every conflict + delete every dead-static candidate" heuristic is not in the spec — the spec/plan has a TUI for the user to interactively select. b is just j > i from find_lexical_conflicts (file order — arbitrary). For a = "Always use TS" / b = "Never use TS", this silently deletes "Never". Combined with find_dead_rules_static's weak signal (substring match on every file path), Go-targeted rules in a Python repo would be deleted with no user input.

The test tests/test_restore_roundtrip.py documents this with the comment "Apply (headless heuristic: delete the b-side of every conflict)" — the test author admits the heuristic is hacky.

Fix options: (a) require --select <ids> or --from-report <path.json> for non-interactive apply; (b) rename to --apply-heuristic distinct from --apply and document loudly; (c) at minimum print deletion plan to stderr before applying.

C5. TUI staleness banner is never rendered; r (rescan) raises

rune/review/tui.py:37-49

• compose() never yields a banner widget — stale_banner_visible is read only by the test, never displayed. The "≤1s mid-session staleness banner" promise (commit 4871683) is satisfied for tests, not for users.
• Interval is not cancelled in action_quit / on_unmount — race risk on shutdown.
• Once stale_banner_visible = True, never resets. The r binding (line 19) has no action_rescan method → pressing r raises in Textual.

Fix: render a real banner (reactive Static), implement action_rescan (clear selected, reset _initial_mtimes), cancel interval on exit. Add TUI test asserting the banner widget is visible after mtime change (not just the bool).

C7. No atomic write — partial multi-file apply on crash

rune/review/applier.py:62

path.write_text("".join(lines)) is sequential per file. If process dies after file #1 is written but before file #2, repo is half-applied with no rollback marker.

Fix: write to path.with_suffix(path.suffix + ".rune-tmp") then os.replace. Even better, write ALL temp files first, then atomically rename them all (or rollback if any rename fails). The backup mechanism allows manual recovery but only if the user knows to invoke --restore.

(C6 was a misread on first pass — withdrawn.)

---

Important (should fix soon)

• I1 L1 P/R test uses per-file evaluation (tests/test_l1_precision_recall.py:28-31) — degenerate setup that doesn't measure realistic corpus-level precision. Recommend cross-file evaluation OR explicit documentation that the metric is synthetic.
• I2 find_dead_rules_static rglobs the whole repo per chunk (rune/review/dead_static.py:24-28,43) — O(n*m). Walk once, build sets, then look up.
• I3 No conflict-pair dedup (rune/review/conflict_lexical.py:62-71) — duplicate triples in same chunk emit duplicate pairs.
• I4 _read_chunk re-reads file per op (rune/review/applier.py:19-21,44,51) — read once per file.
• I5 No bounds check on start_line/end_line (rune/review/applier.py:52-57) — comment branch raises IndexError if end_line > len(lines); delete is silent no-op.
• I6 --apply + --restore: restore wins silently (rune/review/cli.py:29-40) — should be explicit mutual-exclusion error.
• I7 --apply + --json: --json silently ignored (rune/review/cli.py:75-92) — either print apply log as JSON or refuse the combo.
• I8 verify_entry "OK if matches pre OR post" collapses two states (rune/patches/manifest.py:46-48) — replace with PENDING / APPLIED / DRIFT.
• I9 apply_cmd new-file branch doesn't verify payload SHA matches post_sha256 (rune/cli/patch.py:34-42) — corrupted payload silently creates wrong file. Apply to existing-file branch too.
• I10 --events-window-days accepted but silently ignored (rune/review/dead_events.py:13) — at minimum warn when non-default.
• I11 Undeclared deps (scripts/build_manifest.py:15 vs pyproject.toml) — tomli_w, jsonschema, pytest-asyncio all used but not in dev extras.
• I12 Source.trigger discarded then re-parsed via regex (rune/review/loader.py:20 → rune/review/dead_static.py:18) — pick one source of truth.

---

Minor (polish)

• M1 L1 detector is O(n²) — bucket by (verb, object) for v0.3.
• M2 "skip if NEG within 10 chars" heuristic (rune/review/conflict_lexical.py:34) — replace with anchored lookahead.
• M3 L2 candidate generation reuses L1 output — narrow recall ceiling. By definition can't catch the 5 L1_LIMITS modes.
• M4 TARGETS in scripts/build_manifest.py:18-28 hardcoded — new modules silently skipped.
• M5 Generated rune/patches/manifest.toml + payloads/ untracked but not gitignored — add to .gitignore.
• M6 TUI _label shows "finding N" with no path/kind — users can't tell what they're selecting.
• M7 _detail widget yielded but never updated — pressing j/k navigates but pane stays empty.
• M8 tests/test_tui_cold_start.py:14-21 measures the test's own sleep, not cold-start time.
• M9 Unused pytest imports (minor flake8 lint).
• M10 ReviewReport.to_dict() is one-way; no from_dict for future --replay.

---

Follow-up issues will be opened separately and cross-referenced from this PR.

[codex-devlab]

Follow-up tracking — review issues + next phases

New tracking issues from this review (10)

Critical (must-fix; some are PR-blocking, some are scoped-out follow-ups)

• Centralize SHA computation for chunk identity (loader ↔ applier) #22 Centralize SHA computation (C1, I12) — loader/applier hash disagreement. This is the most urgent.
• Two-phase atomic multi-file apply (no half-applied state on crash) #17 Two-phase atomic apply (C7) — no half-applied state on crash.
• TUI: render staleness banner, implement rescan/preview/apply handlers #21 TUI render + action handlers (C5, M6, M7) — banner, rescan, preview, apply, ~10 declared keybinds currently have no handler.

Important

• Corpus-level L1 precision/recall harness (replace per-file synthetic eval) #20 Corpus-level L1 P/R harness (I1) — replace per-file synthetic eval.
• L2 candidate generation: replace L1-echo with embedding shingling #18 L2 candidate generation (M3) — replace L1-echo with embedding shingling so L2 actually catches L1_LIMITS modes.
• Verify payload SHA matches post_sha256 in rune patch apply #24 Payload SHA verify in rune patch apply (I9).
• rune patch verify: distinguish PENDING vs APPLIED vs DRIFT #23 rune patch verify distinguish PENDING/APPLIED/DRIFT (I8).
• Implement --events-window-days for Stage B dead-rule detection #26 Implement --events-window-days (I10) — currently silent no-op.

Minor

• build_manifest.py: auto-discover TARGETS + CI guard against drift #19 build_manifest.py TARGETS auto-discovery + missing-deps fix (M4, I11) — also pulls in tomli_w/jsonschema/pytest-asyncio declaration.
• TUI cold-start: measure actual mount time, not test sleep #25 TUI cold-start: real mount-time measurement (M8).

PR-blocking review items NOT separately ticketed (must fix in this PR)

These should land as additional commits on feat/review-v0.2 before merge:

• C2 — NLI label-index brittleness: read model.config.id2label instead of indexing [0].
• C3 — --restore path flattening: backup by relative path, restore via operation_log.json.
• C4 — --apply --yes headless heuristic: gate behind --select <ids> / --from-report or rename to --apply-heuristic.

Important review items, small enough to bundle into this PR

• I2 Walk repo once in find_dead_rules_static, cache extension/path sets.
• I3 Conflict pair dedup (seen: set[tuple]).
• I4 Single-read per file in apply_operations.
• I5 Bounds check start_line/end_line in delete/comment branches.
• I6 --apply + --restore mutual-exclusion error.
• I7 --apply + --json interaction (print apply log as JSON, OR refuse).

Minor review items

• M1 O(n²) L1 — TODO comment pointing at bucketing strategy.
• M2 Replace "skip if NEG within 10 chars" with anchored lookahead.
• M5 Add rune/patches/manifest.toml, rune/patches/payloads/ to .gitignore.
• M9/M10 Lint sweep + from_dict deferred to v0.3.

Next phases (v0.3+)

Independent from this PR's must-fixes:

1. Real semantic L2 — once L2 candidate generation: replace L1-echo with embedding shingling #18 lands, retire the "lexically-adjacent" disclaimer and aim at recall on the 5 L1_LIMITS modes.
2. rune review --watch mode — continuous re-scan + event push to TUI. Mentioned in spec §7 as deferred.
3. Concurrent --apply lockfile — single-user/single-session assumption is fine for v0.2 but breaks on shared CI / editor integration. Defer until concrete user report.
4. Schema 1.1 stabilization — current --l2 bumps schema_version to 1.1. Once additive contract is exercised in v0.3, re-confirm the SCHEMA_POLICY.md "additive" definition is precise.
5. PyPI packaging + README — overlaps with the open Task 15: README + PyPI packaging #15.

Question on existing open issues (phase-1 tasks)

Issues #5–#12 (Task 5–10, 11, 13) are all phase-1 labels representing v0.1.0 work that already shipped in main. None are affected by this PR. Recommend closing them in a sweep separate from this PR — owner discretion, not gated on v0.2 merge.

#13 (E2E Benchmarks), #14 (init/scaffold), #15 (README + PyPI) remain genuinely incomplete and should stay open.

---

Suggested merge sequence:

1. Address C1/C2/C3/C4 + the bundled Importants on this branch → re-review.
2. Once green, merge feat/review-v0.2 → main.
3. v0.3 milestone planning kicks off the 10 follow-up issues above.

[codex-devlab]

Review fixes landed — 5 thematic commits

Per RALPLAN consensus plan (~/ToolSet/rune/pr16-fix-plan-2026-06-11.md, also at .omc/plans/pr16-review-v02-fixes.md), the merge-blocking review findings are addressed in 5 thematic commits ordered E → A → C → B → D.

Test count: 118 → 128 passed + 4 skipped (NLI-gated; no regressions; +10 new test cases).

Commit-to-finding map

Commit	SHA	Findings addressed
E — pre-flight test fix	8419d18	Unmasks the C1 false-positive in tests/test_apply_staleness.py (test passed by coincidence with no-trailing-newline text). New test intentionally FAILS at this commit, proving C1 was a real bug.
A — applier.py canonical-form bundle	0e5a846	C1 (canonical SHA: .rstrip("\n") → .strip() to mirror producer at tokenizer.py:26), C3 (rename misleading rel = file.name → relative_path = file.relative_to(base_root); propagate base_root through apply_operations signature; backup preserves nested paths), I4 (single read per file via cached lines), I5 (bounds check raises ValueError on out-of-range chunk). E's failing test now PASSES.
C — cli.py apply gate + mutex + JSON identity	5bc3b41	C4 (--apply --yes requires --confirm-delete-heuristics OR --from-report OR --select OR RUNE_ALLOW_BLIND_APPLY=1; bare invocation exits 2 with stderr guidance), I6 (--apply + --restore mutex exits 2), I7 (stdout JSON byte-identical to operation_log.json when --apply --json). tests/test_restore_roundtrip.py and test_backup_retention.py updated with --confirm-delete-heuristics (one-line additions).
B — NLI label resolution	dfce92a	C2 (dynamic model.config.id2label lookup; raises RuntimeError on incompatible checkpoints; tested for both default cross-encoder/nli-deberta-v3-base and --nli-small cross-encoder/nli-distilroberta-base — gated behind RUNE_RUN_NLI=1). Removed brittle hardcoded [0] index.
D — perf cleanup	fff5b83	I2 (find_dead_rules_static: O(n·m) → O(n+m) via single pre-walk into exts_present set + path_strs_lower list; removed _repo_has_extension helper), I3 (conflict pair dedup via seen: set[tuple[int,int,str,str]]).

Caller audit (per plan §4 Commit C)

rg -n "rune apply" .github/ scripts/ docs/ → zero hits. No external automation breaks on C4's gating.

Not addressed in this PR (deferred per consensus plan)

These remain as separate follow-up issues:

Issue	Title	Why deferred
#21	TUI render + 10 missing action handlers (C5, M6, M7)	Large refactor; separate PR
#17	Two-phase atomic multi-file apply (C7)	Medium; separate PR
#20	Corpus-level L1 P/R (I1)	Test improvement, non-blocking
#18	L2 candidate generation (M3)	Design work for v0.3
#19	build_manifest TARGETS auto-discovery + missing deps	Release process
#23	rune patch verify PENDING/APPLIED/DRIFT (I8)	UX polish
#24	Payload SHA verify in apply (I9)	Hardening
#25	TUI cold-start real measurement (M8)	Test improvement
#26	--events-window-days implementation (I10)	Stub → real feature
#22	(Closed by Commit A)	Centralize chunk SHA ✓

Recommend closing #22 as resolved by Commit A.

Next step

CI gate via gh pr checks 16 --watch. On green, recommend squash-merge (per consensus): all 31 commits → one semantic commit on main. Per-commit visibility preserved during review; clean linear history on main.

🤖 Generated with Claude Code