# Code Health Review — 2026-07-06
# HUMAN PLAYTESTING
* n/p should always say "There are changes in the other panel, would you like me to move you to the
other panel" when you try to go beyond the last diff in the current panel and the other panel
actually has changes. If the files are identical, a popup "Files are identical" is good.
Scope: duplicate code, code length, refactoring/generalization opportunities, structure,
readability. Ordered by expected payoff within each section. Line numbers are as of commit
f5e78e0.
## Status (2026-07-07)
Implemented in the working tree, verified by the full test suite (266 passed; only the two
pre-existing failures remain: `rust_algorithm_change` / `rust_turbopack_module_rule`
`matches_human_solution`) and by `benchmark_optimal_solutions` (178 mismatches / 13 unsolved —
byte-identical to a clean HEAD baseline run):
- **1.1 + 1.2 + 1.3** — `code::metadata::metadata_of` (Cow) replaced all 18 clone stanzas; the
twin passes are now thin wrappers over `diff::hash_tree_matching::solve` (spec = hash accessor +
classifier + reasons); the O(n) `mapping.iter().any` scans became O(1) `before_node_map`
lookups; the identical pass got the structural pass's unclaimed-duplicate fix, and the pinning
test now asserts one-to-one duplicate pairing.
- **1.4** — `NodeCache::build` folded into one `cache_for` helper; the `unsafe` transmute now has
a single home.
- **1.5 + 1.6 + 1.9(blob)** — `stats::filesystem::find_git_repositories` (sorted; `commit_stats`
is now deterministic), generic `stats::sampling::Reservoir<T>`, `stats::git::blob_bytes`.
- **§4 lints** — the four `spf_a` dead-store initializers removed; rustc's definite-assignment
check proves the values were never read. Build is warning-free.
- **§4 typos** — `memo`, `unmatched`, forest/theoretically/mapping/maps/languages/its, plus
`Therefore`/`something`/`fields` in code.rs/metadata.rs.
Not yet done (deliberately): the module splits of `human_solver.rs` / `apted/common.rs` (§2) and
the before/after side-parameterization (1.7/1.8) — the next candidates, per §5's order of attack.
---
## 1. Duplicate code
### 1.1 Metadata-clone boilerplate in every solver pass (also a real perf cost)
Every pass in the pipeline opens with the same 8-line stanza, once per side:
```rust
});
```
Sites (9 files, 18 clones): `solve_identical_trees.rs:37`, `solve_structurally_identical_trees.rs:42`,
`solve_semantically_structural_nodes.rs:115` **and** `:258` (twice in one file),
`solve_similar_flow_control.rs:56`, `solve_identical_diagnostic_statements.rs:62`,
`solve_moved_subtrees.rs:66`, `apted/common.rs:2293`, `test/helper/optimal_iud.rs:160`.
This is not just repetition: `ASTMetadata` holds several whole-tree `HashMap`s
(`node_info`, `node_to_full_hash`, `full_hash_to_node`, structural-hash maps, …), so a single
`Diff::from_code` run deep-copies both sides' metadata ~7 times each. The comment at each site
("We clone to avoid lifetime issues") concedes the clone is incidental.
**Suggestion:** compute/ensure metadata once in `Diff::from_code` and pass `&ASTMetadata` for each
side into every pass (they already all share the `solve(before, after, &node_cache, &mut ast_diff)`
signature — extend it to take the two metadata refs). If a standalone entry point still needs the
fallback, one shared helper `fn metadata_of(code: &Code) -> Cow<'_, ASTMetadata>` replaces all 18
stanzas.
### 1.2 `solve_identical_trees` vs `solve_structurally_identical_trees` are near-clones with drift
The two passes share ~80% of their body line-for-line: same metadata stanza, same
reference-node loop, same "skip already mapped" check, same paired stack-descent matching children
by position and kind, same redundant `{ let after_node_id = matching_after_node.id(); … }` shadow
block. They differ only in (a) which hash map they consult (full vs structural), (b) how the
root/child operation is classified (always `Identical` vs text-compare → `Identical`/`Update`),
and (c) duplicate handling — and (c) is *accidental* drift, not a design difference:
- `solve_identical_trees.rs:91` takes `after_node_ids.iter().next()` — the documented
duplicate-collapse bug (all N before-duplicates map onto one after-node; there's a 40-line
comment plus a pinning test for it).
- `solve_structurally_identical_trees.rs:88` already fixed the same problem with
`.find(|&&id| !diff.after_node_map.contains_key(&id))`.
**Suggestion:** extract one generic pass parameterized by (hash-map accessor, operation
classifier) — the structural pass's claimed-node filter then fixes the identical pass's TODO for
free. If the full extraction feels heavy, at minimum port the `.find(unclaimed)` fix and extract
the shared child-descent loop.
### 1.3 O(n) linear scans over `diff.mapping` inside per-node loops
Both twin passes ask "is this before-node already mapped?" via
```rust
